Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[UNDERTOW-2337] Multipart form-data larger than 16KiB is not available through Servlet getParameter API #1541

Merged
merged 2 commits into from
Feb 13, 2024

Conversation

gaol
Copy link
Contributor

@gaol gaol commented Jan 10, 2024

@Laurens-W
Copy link

@fl4via
Copy link
Member

fl4via commented Feb 12, 2024

Hi @Laurens-W! I tried running the reproducer, but I am not managing to run it successfully with a simple post command. What am I supposed to see with the reproducer?
A simple curl command is returning an error, without sending big enough data:

[fla@fla bin]$ curl -H 'Content-Type: application/json' \
-d '{ "title":"foo","body":"bar", "id": 1}' \
-X POST \
localhost:8080/api/some-request
{"timestamp":"2024-02-12T07:53:32.555+00:00","status":415,"error":"Unsupported Media Type","path":"/api/some-request"}

@fl4via fl4via added bug fix Contains bug fix(es) under verification Currently being verified (running tests, reviewing) before posting a review to contributor labels Feb 12, 2024
@Laurens-W
Copy link

Hey @fl4via! I added a postman collection to the reproducer with a successful and failing request :)

@fl4via
Copy link
Member

fl4via commented Feb 12, 2024

Hi @Laurens-W! I tried running the reproducer, but I am not managing to run it successfully with a simple post command. What am I supposed to see with the reproducer? A simple curl command is returning an error, without sending big enough data: [fla@fla bin]$ curl -H 'Content-Type: application/json' \ -d '{ "title":"foo","body":"bar", "id": 1}' \ -X POST \ localhost:8080/api/some-request {"timestamp":"2024-02-12T07:53:32.555+00:00","status":415,"error":"Unsupported Media Type","path":"/api/some-request"}

@Laurens-W I see that I was running it in the wrong way and with the wrong curl command. Now I see:

2024-02-12T08:14:46.255-03:00 WARN 375459 --- [ XNIO-1 task-2] .w.s.m.s.DefaultHandlerExceptionResolver : Resolved [org.springframework.web.method.annotation.MethodArgumentConversionNotSupportedException: Failed to convert value of type 'org.springframework.web.multipart.support.StandardMultipartHttpServletRequest$StandardMultipartFile' to required type 'com.example.demo.DemoApplication$SomeObject'; Cannot convert value of type 'org.springframework.web.multipart.support.StandardMultipartHttpServletRequest$StandardMultipartFile' to required type 'com.example.demo.DemoApplication$SomeObject': no matching editors or conversion strategy found]

That happens when uploading a small file, it doesnt' have to be big.

@fl4via
Copy link
Member

fl4via commented Feb 12, 2024

Hey @fl4via! I added a postman collection to the reproducer with a successful and failing request :)

Thanks a lot @Laurens-W ! I'll give it a try!

@Laurens-W
Copy link

@fl4via Basically the issue describes that the request part data is not available through the getParameter method, but Spring uses getPart and that throws a Resolved [org.springframework.web.bind.MissingServletRequestParameterException: Required request parameter 'object' for method parameter type String is not present]
It's caused by the Part being written to a file but the content-disposition not having a filename here:
https://github.com/spring-projects/spring-framework/blob/b61552b9df8e3f18f901fd30996ae0ec0f222060/spring-web/src/main/java/org/springframework/web/multipart/support/StandardMultipartHttpServletRequest.java#L91-L112

@fl4via
Copy link
Member

fl4via commented Feb 12, 2024

Thanks for the information @Laurens-W . I am having a look at the reproducer (it runs as you described and I can see both the success and failure scenarios) to see how to properly fix this.

@fl4via fl4via added next release This PR will be merged before next release or has already been merged (for payload double check) waiting CI check Ready to be merged but waiting for CI check and removed under verification Currently being verified (running tests, reviewing) before posting a review to contributor labels Feb 12, 2024
@fl4via
Copy link
Member

fl4via commented Feb 12, 2024

@Laurens-W indeed, I investigated the reproducer, Undertow, and I can see what is the problem. The fix provided by this PR solves the issue, not adding a fileName, but instead this check allows the return of a non-null parameter.

To formalize that the commit fixes the issue with getParameterValues(), I added a check to the test case to verify that the parameter is being returned correctly, as expected.

If you want to give it a try, feel free to download the undertow from master branch after I merge this PR, and change your reproducer by adding the following dependencies to the pom.xml file (or you can wait a few hours for the next release and point to 2.3.11.Final instead):
<dependency> <groupId>io.undertow</groupId> <artifactId>undertow-core</artifactId> <version>2.3.11.Final-SNAPSHOT</version> </dependency> <dependency> <groupId>io.undertow</groupId> <artifactId>undertow-servlet</artifactId> <version>2.3.11.Final-SNAPSHOT</version> </dependency>

They should come before the other dependencies to overwrite the version. You will see that the test passes. If in the future you spot any failure related to this PR, please let me know. Thanks for the reproducer and for responding quickly to my inquiries @Laurens-W . The postman collection was very helpful!

@fl4via fl4via removed the waiting CI check Ready to be merged but waiting for CI check label Feb 13, 2024
@fl4via fl4via merged commit 70cb55e into undertow-io:master Feb 13, 2024
25 checks passed
@Laurens-W
Copy link

@fl4via No problem! Glad you could look into it and confirm the exception in Spring is based on the same issue!

@fl4via fl4via removed the next release This PR will be merged before next release or has already been merged (for payload double check) label Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix Contains bug fix(es)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants