Skip to content

Commit 2bccf70

Browse files
jknackjknack
authored
Merge pull request #4034 from jooby-project/4031
fix: Reading body() before or after form() is not consistent across a…
2 parents 28cc2d7 + 1ac3d88 commit 2bccf70

6 files changed

Lines changed: 303 additions & 14 deletions

File tree

‎modules/jooby-jetty/src/main/java/io/jooby/internal/jetty/JettyContext.java‎

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -38,10 +38,10 @@
3838
import org.eclipse.jetty.http.HttpField;
3939
import org.eclipse.jetty.http.HttpHeader;
4040
import org.eclipse.jetty.http.HttpHeaderValue;
41-
import org.eclipse.jetty.http.MimeTypes;
4241
import org.eclipse.jetty.io.ByteBufferPool;
4342
import org.eclipse.jetty.io.Content;
4443
import org.eclipse.jetty.io.content.InputStreamContentSource;
44+
import org.eclipse.jetty.server.FormFields;
4545
import org.eclipse.jetty.server.Request;
4646
import org.eclipse.jetty.server.Response;
4747
import org.eclipse.jetty.util.BufferUtil;
@@ -145,6 +145,9 @@ public Map<String, String> cookieMap() {
145145

146146
@Override
147147
public Body body() {
148+
if (isFormLike()) {
149+
return Body.empty(this);
150+
}
148151
InputStream in = Content.Source.asInputStream(request);
149152
long len = request.getLength();
150153
if (maxRequestSize > 0) {
@@ -210,6 +213,23 @@ public QueryString query() {
210213
return query;
211214
}
212215

216+
private boolean isFormLike() {
217+
var contentType = request.getHeaders().get(HttpHeader.CONTENT_TYPE);
218+
return isFormEncoded(contentType) || isMultipart(contentType);
219+
}
220+
221+
private boolean isFormEncoded(String contentType) {
222+
return contentType != null && contentType.startsWith(MediaType.FORM_URLENCODED);
223+
}
224+
225+
private boolean isMultipart() {
226+
return isMultipart(request.getHeaders().get(HttpHeader.CONTENT_TYPE));
227+
}
228+
229+
private boolean isMultipart(String contentType) {
230+
return contentType != null && contentType.startsWith(MediaType.MULTIPART_FORMDATA);
231+
}
232+
213233
@Override
214234
public Formdata form() {
215235
if (formdata == null) {
@@ -218,10 +238,8 @@ public Formdata form() {
218238
formParam(request, formdata);
219239

220240
// Files:
221-
String contentType = request.getHeaders().get(HttpHeader.CONTENT_TYPE);
222-
if (contentType != null
223-
&& MimeTypes.Type.MULTIPART_FORM_DATA.is(
224-
HttpField.getValueParameters(contentType, null))) {
241+
if (isMultipart()) {
242+
String contentType = request.getHeaders().get(HttpHeader.CONTENT_TYPE);
225243
// Is a multipart... make sure isn't empty
226244
String boundary = MultiPart.extractBoundary(contentType);
227245
if (boundary != null) {
@@ -724,7 +742,7 @@ private void register(DeleteFileTask deleteFileTask) {
724742

725743
private void formParam(Request request, Formdata form) {
726744
try {
727-
var params = Request.getParameters(request);
745+
var params = FormFields.getFields(request);
728746
for (Fields.Field param : params) {
729747
String name = param.getName();
730748
var values = param.getValues();

‎modules/jooby-netty/src/main/java/io/jooby/internal/netty/NettyContext.java‎

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -373,10 +373,21 @@ public Value header() {
373373
return headers;
374374
}
375375

376+
private boolean hasNext(InterfaceHttpPostRequestDecoder decoder) {
377+
try {
378+
return decoder != null && decoder.hasNext();
379+
} catch (HttpPostRequestDecoder.EndOfDataDecoderException ignored) {
380+
return false;
381+
}
382+
}
383+
376384
@Override
377385
public Body body() {
378-
if (decoder != null && decoder.hasNext()) {
379-
return new NettyBody(this, (HttpData) decoder.next(), HttpUtil.getContentLength(req, -1L));
386+
if (hasNext(decoder)) {
387+
if (decoder instanceof HttpRawPostRequestDecoder) {
388+
return new NettyBody(this, (HttpData) decoder.next(), HttpUtil.getContentLength(req, -1L));
389+
}
390+
// form-like, we must use formdata() to get the body, so we return an empty body here.
380391
}
381392
return Body.empty(this);
382393
}
@@ -900,20 +911,19 @@ private void decodeForm(Formdata form) {
900911
return;
901912
}
902913
try {
903-
while (decoder.hasNext()) {
914+
while (hasNext(decoder)) {
904915
HttpData next = (HttpData) decoder.next();
905916
if (next.getHttpDataType() == InterfaceHttpData.HttpDataType.FileUpload) {
906-
var fileUpoad =
917+
var fileUpload =
907918
new NettyFileUpload(
908919
router.getTmpdir(), (io.netty.handler.codec.http.multipart.FileUpload) next);
909-
register(new DeleteFileTask(router.getLog(), fileUpoad.getFileName(), fileUpoad::close));
910-
form.put(next.getName(), fileUpoad);
920+
register(
921+
new DeleteFileTask(router.getLog(), fileUpload.getFileName(), fileUpload::close));
922+
form.put(next.getName(), fileUpload);
911923
} else {
912924
form.put(next.getName(), next.getString(UTF_8));
913925
}
914926
}
915-
} catch (HttpPostRequestDecoder.EndOfDataDecoderException x) {
916-
// ignore, silly netty
917927
} catch (Exception x) {
918928
throw SneakyThrows.propagate(x);
919929
}

‎tests/src/test/java/io/jooby/i4031/Issue4031.java‎

Lines changed: 240 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,240 @@
1+
/*
2+
* Jooby https://jooby.io
3+
* Apache License Version 2.0 https://jooby.io/LICENSE.txt
4+
* Copyright 2014 Edgar Espina
5+
*/
6+
package io.jooby.i4031;
7+
8+
import java.net.URLEncoder;
9+
import java.nio.charset.StandardCharsets;
10+
import java.util.Map;
11+
12+
import org.junit.jupiter.api.Assertions;
13+
14+
import io.jooby.exception.MissingValueException;
15+
import io.jooby.junit.ServerTest;
16+
import io.jooby.junit.ServerTestRunner;
17+
import okhttp3.FormBody;
18+
import okhttp3.MediaType;
19+
import okhttp3.MultipartBody;
20+
import okhttp3.RequestBody;
21+
22+
class Issue4031 {
23+
24+
@ServerTest
25+
public void formDataAndQueryParameters(ServerTestRunner runner) {
26+
runner
27+
.define(
28+
app ->
29+
app.post(
30+
"/",
31+
ctx -> {
32+
try {
33+
Assertions.assertAll(
34+
() ->
35+
Assertions.assertEquals(
36+
1, ctx.form().size(), "form contains one entry"),
37+
() -> Assertions.assertTrue(ctx.form().isPresent()),
38+
() -> Assertions.assertFalse(ctx.form().isMissing()),
39+
() -> Assertions.assertEquals("bar", ctx.form().get("foo").value()),
40+
() -> Assertions.assertEquals("bar", ctx.form("foo").value()),
41+
() -> Assertions.assertEquals("bar", ctx.form().toMap().get("foo")),
42+
() -> Assertions.assertEquals("bar", ctx.formMap().get("foo")),
43+
() ->
44+
Assertions.assertEquals(
45+
"?parameter1=value1", ctx.query().queryString()),
46+
() -> Assertions.assertEquals("parameter1=value1", ctx.query().value()),
47+
() ->
48+
Assertions.assertEquals("value1", ctx.query("parameter1").value()),
49+
() ->
50+
Assertions.assertEquals("value1", ctx.queryMap().get("parameter1")),
51+
() -> Assertions.assertEquals(1, ctx.queryMap().size()));
52+
return "OK";
53+
} catch (Exception ex) {
54+
ctx.setResponseCode(500);
55+
return ex.getMessage();
56+
}
57+
}))
58+
.ready(
59+
http -> {
60+
http.post(
61+
"/",
62+
new FormBody.Builder().add("foo", "bar").build(),
63+
Map.of("parameter1", "value1"),
64+
rsp -> {
65+
Assertions.assertEquals("OK", rsp.body().string());
66+
Assertions.assertEquals(200, rsp.code());
67+
});
68+
69+
// form with bad media type
70+
http.post(
71+
"/",
72+
customForm(
73+
Map.of("foo", "bar"), "application/x-www-form-urlencoded; charset=utf-8"),
74+
Map.of("parameter1", "value1"),
75+
rsp -> {
76+
Assertions.assertEquals("OK", rsp.body().string());
77+
Assertions.assertEquals(200, rsp.code());
78+
});
79+
80+
http.post(
81+
"/",
82+
customForm(
83+
Map.of("foo", "bar"), "application/x-www-form-urlencoded;charset=iso-8859-1"),
84+
Map.of("parameter1", "value1"),
85+
rsp -> {
86+
Assertions.assertEquals("OK", rsp.body().string());
87+
Assertions.assertEquals(200, rsp.code());
88+
});
89+
90+
// multipart
91+
http.post(
92+
"/",
93+
new MultipartBody.Builder()
94+
.addFormDataPart("foo", "bar")
95+
.setType(MultipartBody.FORM)
96+
.build(),
97+
Map.of("parameter1", "value1"),
98+
rsp -> {
99+
Assertions.assertEquals("OK", rsp.body().string());
100+
Assertions.assertEquals(200, rsp.code());
101+
});
102+
});
103+
}
104+
105+
private static RequestBody customForm(Map<String, Object> form, String mediaType) {
106+
var payload =
107+
form.entrySet().stream()
108+
.map(
109+
entry ->
110+
entry.getKey()
111+
+ "="
112+
+ URLEncoder.encode(entry.getValue().toString(), StandardCharsets.UTF_8))
113+
.reduce((a, b) -> a + "&" + b)
114+
.orElse("");
115+
116+
return RequestBody.create(payload, MediaType.parse(mediaType));
117+
}
118+
119+
@ServerTest
120+
public void readFormDataAfterBody(ServerTestRunner runner) {
121+
runner
122+
.define(
123+
app ->
124+
app.post(
125+
"/",
126+
ctx -> {
127+
try {
128+
Assertions.assertAll(
129+
() ->
130+
Assertions.assertEquals(
131+
1, ctx.form().size(), "form contains one entry"),
132+
() -> Assertions.assertTrue(ctx.form().isPresent()),
133+
() -> Assertions.assertFalse(ctx.form().isMissing()),
134+
() -> Assertions.assertEquals("bar", ctx.form().get("foo").value()),
135+
() -> Assertions.assertEquals("bar", ctx.form("foo").value()),
136+
() -> Assertions.assertEquals("bar", ctx.form().toMap().get("foo")),
137+
() -> Assertions.assertEquals("bar", ctx.formMap().get("foo")),
138+
// get body().value() after calling form methods
139+
() ->
140+
Assertions.assertThrows(
141+
MissingValueException.class, () -> ctx.body().value()));
142+
return "OK";
143+
} catch (Exception ex) {
144+
ctx.setResponseCode(500);
145+
return ex.getMessage();
146+
}
147+
}))
148+
.ready(
149+
http -> {
150+
http.post(
151+
"/",
152+
new FormBody.Builder().add("foo", "bar").build(),
153+
rsp -> {
154+
Assertions.assertEquals("OK", rsp.body().string());
155+
Assertions.assertEquals(200, rsp.code());
156+
});
157+
158+
http.post(
159+
"/",
160+
customForm(
161+
Map.of("foo", "bar"), "application/x-www-form-urlencoded; charset=utf-8"),
162+
rsp -> {
163+
Assertions.assertEquals("OK", rsp.body().string());
164+
Assertions.assertEquals(200, rsp.code());
165+
});
166+
167+
http.post(
168+
"/",
169+
new MultipartBody.Builder()
170+
.addFormDataPart("foo", "bar")
171+
.setType(MultipartBody.FORM)
172+
.build(),
173+
rsp -> {
174+
Assertions.assertEquals("OK", rsp.body().string());
175+
Assertions.assertEquals(200, rsp.code());
176+
});
177+
});
178+
}
179+
180+
@ServerTest
181+
public void readFormDataBeforeBody(ServerTestRunner runner) {
182+
runner
183+
.define(
184+
app ->
185+
app.post(
186+
"/",
187+
ctx -> {
188+
try {
189+
Assertions.assertAll(
190+
// get body().value() before calling form methods
191+
() ->
192+
Assertions.assertThrows(
193+
MissingValueException.class, () -> ctx.body().value()),
194+
() ->
195+
Assertions.assertEquals(
196+
1, ctx.form().size(), "form contains one entry"),
197+
() -> Assertions.assertTrue(ctx.form().isPresent()),
198+
() -> Assertions.assertFalse(ctx.form().isMissing()),
199+
() -> Assertions.assertEquals("bar", ctx.form().get("foo").value()),
200+
() -> Assertions.assertEquals("bar", ctx.form("foo").value()),
201+
() -> Assertions.assertEquals("bar", ctx.form().toMap().get("foo")),
202+
() -> Assertions.assertEquals("bar", ctx.formMap().get("foo")));
203+
return "OK";
204+
} catch (Exception ex) {
205+
ctx.setResponseCode(500);
206+
return ex.getMessage();
207+
}
208+
}))
209+
.ready(
210+
http -> {
211+
// http.post(
212+
// "/",
213+
// new FormBody.Builder().add("foo", "bar").build(),
214+
// rsp -> {
215+
// Assertions.assertEquals("OK", rsp.body().string());
216+
// Assertions.assertEquals(200, rsp.code());
217+
// });
218+
219+
http.post(
220+
"/",
221+
customForm(
222+
Map.of("foo", "bar"), "application/x-www-form-urlencoded; charset=utf-8"),
223+
rsp -> {
224+
Assertions.assertEquals("OK", rsp.body().string());
225+
Assertions.assertEquals(200, rsp.code());
226+
});
227+
228+
// http.post(
229+
// "/",
230+
// new MultipartBody.Builder()
231+
// .setType(MultipartBody.FORM)
232+
// .addFormDataPart("foo", "bar")
233+
// .build(),
234+
// rsp -> {
235+
// Assertions.assertEquals("OK", rsp.body().string());
236+
// Assertions.assertEquals(200, rsp.code());
237+
// });
238+
});
239+
}
240+
}

‎tests/src/test/java/io/jooby/test/FeaturedTest.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1555,6 +1555,13 @@ public void maxRequestSize(ServerTestRunner runner) {
15551555
assertEquals(200, rsp.code());
15561556
assertEquals(_8kb, rsp.body().string());
15571557
});
1558+
client.post(
1559+
"/request-size",
1560+
RequestBody.create(_4kb, MediaType.get("text/plain")),
1561+
rsp -> {
1562+
assertEquals(200, rsp.code());
1563+
assertEquals(_4kb, rsp.body().string());
1564+
});
15581565
// Empty
15591566
client.post(
15601567
"/request-size",

‎tests/src/test/java/io/jooby/test/TestUtil.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ public class TestUtil {
2222

2323
public static final String _8kb = _16kb.substring(0, _16kb.length() / 2);
2424

25+
public static final String _4kb = _8kb.substring(0, _8kb.length() / 2);
26+
2527
private static String readText(Path file) {
2628
try {
2729
return Files.readString(file);

0 commit comments

Comments
 (0)