fix undertow throwing away request with no message#3568
fix undertow throwing away request with no message#3568ExtReMLapin wants to merge 3 commits intoArcadeData:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the HTTP server's handling of large request bodies to provide a more robust and informative error experience. Previously, oversized requests might have been silently dropped or resulted in unclear errors from the underlying Undertow server. The changes introduce a custom handler that explicitly validates request body size and returns a standardized Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request aims to provide a more informative JSON error response for HTTP requests that exceed the configured maximum body size. However, the current implementation introduces a significant Denial of Service (DoS) risk by disabling Undertow's built-in MAX_ENTITY_SIZE protection and only validating the Content-Length header, which allows chunked requests to bypass the size limit entirely. To create a robust solution, it's recommended to leverage Undertow's built-in size limiting capabilities by setting MAX_ENTITY_SIZE to the actual limit, combined with a custom exception handler to catch RequestEntityTooLargeException and produce the desired JSON response. This will ensure all request types are handled safely and correctly.
| .setServerOption(UndertowOptions.MAX_ENTITY_SIZE, maxEntitySize) | ||
| // Set to Long.MAX_VALUE so Undertow does not reject oversized requests before routing; | ||
| // the actual limit is enforced in the handler chain to return a proper 413 with JSON body | ||
| .setServerOption(UndertowOptions.MAX_ENTITY_SIZE, Long.MAX_VALUE) |
There was a problem hiding this comment.
The application disables Undertow's built-in request body size limit by setting UndertowOptions.MAX_ENTITY_SIZE to Long.MAX_VALUE. This creates a Denial of Service vulnerability as chunked requests can bypass the custom Content-Length check, potentially leading to memory exhaustion. To properly enforce request size limits, UndertowOptions.MAX_ENTITY_SIZE must be set to the actual configured limit, allowing Undertow to throw a RequestEntityTooLargeException when the limit is exceeded.
| .setServerOption(UndertowOptions.MAX_ENTITY_SIZE, Long.MAX_VALUE) | |
| .setServerOption(UndertowOptions.MAX_ENTITY_SIZE, maxEntitySize) |
| final long contentLength = exchange.getRequestContentLength(); | ||
| if (contentLength > maxEntitySize) { |
There was a problem hiding this comment.
The createBodySizeLimitHandler only checks the Content-Length header, which is insufficient. For chunked requests, exchange.getRequestContentLength() returns -1, bypassing the limit check. This, combined with MAX_ENTITY_SIZE being set to Long.MAX_VALUE, creates a Denial of Service vulnerability, potentially leading to OutOfMemoryError from excessively large chunked requests. A more robust approach is to let Undertow enforce the limit by setting UndertowOptions.MAX_ENTITY_SIZE and then provide a custom handler for the RequestEntityTooLargeException to return the desired JSON error message. This will handle all cases correctly.
There was a problem hiding this comment.
If I understand correctly, this is only to get a proper error message, right? I'm afraid that disabling Undertow protection could be dangerous. I wouldn't set the limit to Long.MAX_VALUE, maybe a more conservative approach is to set maxEntitySize+1 and check if it's > maxEntitySize. @robfrank WDYT?
There was a problem hiding this comment.
issue with maxEntitySize+1 is that it's not going to fix the problem, when undertow "catches" it, it returns NOTHING, so setting it to maxEntitySize+1 will only trigger the error message when size == maxEntitySize and above throw nothing
🧪 CI InsightsHere's what we observed from your CI run for b8869e0. 🟢 All jobs passed!But CI Insights is watching 👀 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3568 +/- ##
==========================================
+ Coverage 65.12% 65.51% +0.39%
==========================================
Files 1506 1506
Lines 103053 103063 +10
Branches 21366 21368 +2
==========================================
+ Hits 67109 67526 +417
+ Misses 26751 26305 -446
- Partials 9193 9232 +39 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
ContextConfiguration change fixes this issue : |
|
Also , beside the empty message fix, it's a runtime fix for undertow because right now if you update the setting at runtime, it won't change anything |
fixes #3567
New error :