Add the ability to configure the maximum form attribute size #16432
Conversation
|
Probably a good candidate for backport. WDYT @geoand ? |
|
Yeah, I think so. If this doesn't apply cleanly, I can certainly open a PR against the |
|
@geoand isn't there more to it? Shouldn't we have an exception thrown in the case the form is over the limit? AFAICS from the issue, we just put |
|
That sounds reasonable, but I am not sure we can do that - it's something Vert.x handles. @cescoffier do you know how what @gsmet is asking for could be done? |
|
/cc @vietj |
|
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 942efd3
Test Failures⚙️ JVM Tests - JDK 11 #📦 integration-tests/kafka✖ ✖ ⚙️ JVM Tests - JDK 11 Windows #📦 extensions/mongodb-client/deployment✖ ⚙️ JVM Tests - JDK 15 #📦 integration-tests/kafka✖ ✖ |
It seems like Netty does throw an |
|
Maybe it's just me but I find this code a bit weird. I would at the very least have expected a |
|
Which code are you talking about? The Vert.x code I linked above? |
|
In any case, I don't think there is anything we can in Quarkus, so I'll merge this once CI is done. If there is some way we can integrate with Vert.x to make it fail in the case, in question, we can do it in a follow up |
|
Totally agree it doesn't block this PR but the behavior on the Vert.x side is definitely problematic. |
|
@geoand @cescoffier @gsmet I don't see how this is related to vert.x, we don't enforce any limit by default, here's a test I've just created to check this issue: @Test
public void testFormMultipartFormDataLarge() throws Exception {
router.clear();
router.route().handler(BodyHandler.create());
router.route().handler(rc -> {
MultiMap attrs = rc.request().formAttributes();
assertNotNull(attrs);
int size = 0;
assertNotNull(attrs.get("attr1"));
size += attrs.get("attr1").length();
assertNotNull(attrs.get("attr2"));
size += attrs.get("attr2").length();
assertTrue(size > 2048);
rc.response().end();
});
testRequest(HttpMethod.POST, "/?p1=foo", req -> {
Buffer buffer = Buffer.buffer();
String boundary = "dLV9Wyq26L_-JQxk6ferf-RT153LhOO";
String header =
"--" + boundary + "\r\n" +
"Content-Disposition: form-data; name=\"attr1\"\r\n\r\n" + Base64.getUrlEncoder().encodeToString(TestUtils.randomBuffer(1024).getBytes()) + "\r\n" +
"--" + boundary + "\r\n" +
"Content-Disposition: form-data; name=\"attr2\"\r\n\r\n" + Base64.getUrlEncoder().encodeToString(TestUtils.randomBuffer(1024).getBytes()) + "\r\n" +
"--" + boundary + "\r\n" +
"Content-Disposition: form-data; name=\"" + name + "\"; filename=\"file\"\r\n" +
"Content-Type: application/octet-stream\r\n" +
"Content-Transfer-Encoding: binary\r\n" +
"\r\n";
buffer.appendString(header);
buffer.appendBuffer(TestUtils.randomBuffer(50));
buffer.appendString("\r\n--" + boundary + "--\r\n");
req.headers().set("content-length", String.valueOf(buffer.length()));
req.headers().set("content-type", "multipart/form-data; boundary=" + boundary);
req.write(buffer);
}, 200, "OK", null);
}I'm sending a multipart larger than 2kb and there's no problem with that. |
|
@pmlopes the problem in your test will likely surface if |
geoand
added
the
triage/waiting-for-ci
|
@geoand that doesn't seem to be the vert.x behavior: @Test
public void testFormMultipartFormDataLarge() throws Exception {
router.clear();
router.route().handler(BodyHandler.create());
router.route().handler(rc -> {
MultiMap attrs = rc.request().formAttributes();
assertNotNull(attrs);
int size = 0;
assertNotNull(attrs.get("attr1"));
size += attrs.get("attr1").length();
assertNotNull(attrs.get("attr2"));
size += attrs.get("attr2").length();
assertTrue(size > 4096);
rc.response().end();
}).failureHandler(ctx -> {
assertNotNull(ctx.failure());
assertTrue(ctx.failure() instanceof IOException);
assertEquals("Size exceed allowed maximum capacity", ctx.failure().getMessage());
ctx.next();
});
testRequest(HttpMethod.POST, "/?p1=foo", req -> {
Buffer buffer = Buffer.buffer();
String boundary = "dLV9Wyq26L_-JQxk6ferf-RT153LhOO";
String header =
"--" + boundary + "\r\n" +
"Content-Disposition: form-data; name=\"attr1\"\r\n\r\n" + Base64.getUrlEncoder().encodeToString(TestUtils.randomBuffer(2048).getBytes()) + "\r\n" +
"--" + boundary + "\r\n" +
"Content-Disposition: form-data; name=\"attr2\"\r\n\r\n" + Base64.getUrlEncoder().encodeToString(TestUtils.randomBuffer(2048).getBytes()) + "\r\n" +
"--" + boundary + "\r\n" +
"Content-Disposition: form-data; name=\"" + name + "\"; filename=\"file\"\r\n" +
"Content-Type: application/octet-stream\r\n" +
"Content-Transfer-Encoding: binary\r\n" +
"\r\n";
buffer.appendString(header);
buffer.appendBuffer(TestUtils.randomBuffer(50));
buffer.appendString("\r\n--" + boundary + "--\r\n");
req.headers().set("content-length", String.valueOf(buffer.length()));
req.headers().set("content-type", "multipart/form-data; boundary=" + boundary);
req.write(buffer);
}, 400, "Bad Request", null);
}When the attributes are larger than 2kb then indeed a |
|
@pmlopes OK, I see, thanks for the information We aren't using the body handler in RESTEasy Reactive so I'll need to look into emulating its behavior |
|
In light of the previous comments, I will actually push changes to this PR regarding the proper HTTP response code |
|
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 942efd3
Full information is available in the Build summary check run. Test Failures⚙️ JVM Tests - JDK 11 #📦 integration-tests/kafka✖ ✖ ⚙️ JVM Tests - JDK 11 Windows #📦 extensions/smallrye-reactive-messaging-amqp/deployment✖ |
|
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 5e30eda
|
|
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 5e30eda
|
|
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 49e969c
|
Vert.x defaults to 2K and prior to this PR we didn't have a way to configure it. When the a form attribute is too large in multipart request, return HTTP 413. Fixes: quarkusio#16422
|
Rebased to get the latest CI fixes. |
|
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 19eb163
Full information is available in the Build summary check run. Test Failures⚙️ JVM Tests - JDK 11 #📦 extensions/scheduler/deployment✖ 📦 integration-tests/kafka✖ ✖ ⚙️ JVM Tests - JDK 15 #📦 integration-tests/kafka✖ ✖ |
|
Thanks! |
quarkus-bot
bot
removed
the
triage/waiting-for-ci
Failing Jobs - Building 19eb163
Full information is available in the Build summary check run. Test Failures⚙️ JVM Tests - JDK 11 #📦 integration-tests/kafka✖ ✖ ⚙️ JVM Tests - JDK 8 #📦 integration-tests/kafka✖ ✖ |
|
This is not easily backportable due to: Should we just avoid calling this method? |
|
Oh - yes, it's not entirely correct to not do this, but yeah, we can get away with it |
Vert.x defaults to 2K and prior to this PR we didn't have
a way to configure it.
Fixes: #16422