feat(http): add SizeLimitHandler to enforce request body size limit#6658
feat(http): add SizeLimitHandler to enforce request body size limit#6658bladehan1 wants to merge 8 commits intotronprotocol:developfrom
Conversation
…imit Add SizeLimitHandler at the Jetty server level to reject oversized request bodies before they are fully buffered into memory. This prevents OOM attacks via arbitrarily large HTTP payloads that bypass the existing application-level Util.checkBodySize() check (which reads the entire body first) and the JSON-RPC interface (which had no size validation).
Introduce node.http.maxMessageSize and node.jsonrpc.maxMessageSize to allow HTTP and JSON-RPC services to enforce separate request body size limits via Jetty SizeLimitHandler, decoupled from gRPC config. - Default: 4 * GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE (16 MB) - Validation: reject <= 0 with TronError(PARAMETER_INIT) at startup - Each HttpService subclass sets its own maxRequestSize in constructor - SizeLimitHandlerTest covers independent limits, boundary, UTF-8 bytes
|
@bladehan1 One observation on the config validation in |
|
@xxo1shine |
| } | ||
| } | ||
|
|
||
| @Deprecated |
There was a problem hiding this comment.
Marking this helper as deprecated does not make the new HTTP limit effective yet. PostParams.getPostParams() and many servlets still call Util.checkBodySize(), and that method is still enforcing parameter.getMaxMessageSize() (the gRPC limit), not httpMaxMessageSize. So any request whose body is > node.rpc.maxMessageSize but <= node.http.maxMessageSize will pass Jetty and then still be rejected in the servlet layer, which means the new independent HTTP setting is not actually honored for a large part of the API surface.
There was a problem hiding this comment.
Good catch, thanks. checkBodySize() has been updated to use parameter.getHttpMaxMessageSize() instead of parameter.getMaxMessageSize(), so the servlet-layer fallback now honors the independent HTTP limit. See the latest push.
| ServletContextHandler context = new ServletContextHandler(ServletContextHandler.SESSIONS); | ||
| context.setContextPath(this.contextPath); | ||
| this.apiServer.setHandler(context); | ||
| SizeLimitHandler sizeLimitHandler = new SizeLimitHandler(this.maxRequestSize, -1); |
There was a problem hiding this comment.
This moves oversized-request handling in front of every servlet, so those requests never reach RateLimiterServlet.service() / Util.processError(). Today the HTTP APIs consistently set application/json and serialize failures through Util.printErrorMsg(...); after this change an over-limit body gets Jetty's default 413 response instead. That is a client-visible behavior change for existing callers, and the new test only checks status codes so it would not catch the response-format regression.
There was a problem hiding this comment.
Thanks for flagging the response-format difference. I did a detailed comparison:
Before (checkBodySize rejection):
- HTTP status: 200 OK (processError() does not set status code)
- Body:
{"Error":"java.lang.Exception : body size is too big, the limit is 4194304"}
After (SizeLimitHandler rejection):
- HTTP status: 413 Payload Too Large
- Body: Jetty default error page (non-JSON)
The format is indeed not fully compatible, but I believe this is acceptable:
-
The existing behavior is itself incorrect — returning
200 OKwith an error JSON body violates HTTP semantics. Clients that only check status codes would incorrectly treat the oversized request as successful. The new413is the proper HTTP response for this scenario. -
The trigger threshold is very high — default is 4 MB. Normal API requests are far below this. Only abnormal or malicious payloads hit this limit, so the impact surface is negligible for legitimate clients.
-
413 is a standard HTTP status code — all HTTP client libraries handle it correctly. Clients already need to handle non-JSON infrastructure errors (e.g., Jetty 503, proxy 502/504).
-
The new layer provides a real security benefit —
SizeLimitHandlerrejects during streaming, before the body is fully buffered into memory, which is the core OOM protection. Falling back to application-layer formatting would defeat this purpose.
checkBodySize() was enforcing maxMessageSize (gRPC limit) instead of httpMaxMessageSize, causing the independent HTTP size setting to be ineffective at the servlet layer. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
24abc3a to
3048750
Compare
| ServletContextHandler context = new ServletContextHandler(ServletContextHandler.SESSIONS); | ||
| context.setContextPath(this.contextPath); | ||
| this.apiServer.setHandler(context); | ||
| SizeLimitHandler sizeLimitHandler = new SizeLimitHandler(this.maxRequestSize, -1); |
There was a problem hiding this comment.
SizeLimitHandler only guarantees a pre-servlet 413 when Content-Length is known. For chunked / unknown-length requests, enforcement happens while downstream code is reading the request body. In the current codebase many HTTP handlers catch broad Exception around request.getReader(), and RateLimiterServlet also catches unexpected Exception, so the streaming over-limit exception can be absorbed before Jetty turns it into a 413. That means the PR still does not prove the “uniform 413” behavior described in the issue for requests without Content-Length.
| Assert.assertEquals(200, post(httpServerUri, new StringEntity("small body"))); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
these tests all use StringEntity, so they exercise the fixed-length / Content-Length path only. They do not cover chunked or unknown-length requests, where SizeLimitHandler enforces during body reads instead of before servlet dispatch. Since the existing servlet chain has broad catch (Exception) blocks, we need at least one end-to-end test for the streaming path against a real HTTP servlet and the JSON-RPC servlet, and it should assert that an oversized request still surfaces as HTTP 413
| int defaultHttpMaxMessageSize = GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE; | ||
| PARAMETER.httpMaxMessageSize = config.hasPath(ConfigKey.NODE_HTTP_MAX_MESSAGE_SIZE) | ||
| ? config.getInt(ConfigKey.NODE_HTTP_MAX_MESSAGE_SIZE) : defaultHttpMaxMessageSize; | ||
| if (PARAMETER.httpMaxMessageSize <= 0) { |
There was a problem hiding this comment.
The new config validation rejects non-positive values, but extremely large limits are still accepted silently. I don’t think a hard cap is mandatory, but at least warning on suspiciously large values would make misconfiguration easier to catch.
What does this PR do?
Add Jetty
SizeLimitHandlerat the server handler level to enforce request body size limits for all HTTP and JSON-RPC endpoints. Oversized requests are rejected with HTTP 413 before the body is fully buffered into memory.node.http.maxMessageSizeandnode.jsonrpc.maxMessageSizeas independent, configurable size limitsGrpcUtil.DEFAULT_MAX_MESSAGE_SIZE(4 MB), consistent with gRPC defaultsSizeLimitHandlerintoHttpService.initContextHandler()as the outermost handlerHttpServicesubclass (4 HTTP + 3 JSON-RPC) setsmaxRequestSizefrom the corresponding config getterUtil.checkBodySize()— retained as fallback for backward compatibilityWhy are these changes required?
Previously, HTTP request body size was only validated at the application layer (
Util.checkBodySize()), which reads the entire body into memory before checking. The JSON-RPC interface had no size validation at all. This allows an attacker to send arbitrarily large payloads, causing OOM and denial of service.Moving the limit to the Jetty handler chain provides:
Closes #6604
This PR has been tested by:
SizeLimitHandlerTest: boundary, independent limits, UTF-8 byte counting)ArgsTest: default value alignment)Follow up
Util.checkBodySize()callers in a follow-up PR once this is stableExtra details
Files changed: 14 (+253 / -2)
HttpServicemaxRequestSizefield, wireSizeLimitHandlerininitContextHandler()Args/ConfigKey/CommonParameternode.http.maxMessageSizeandnode.jsonrpc.maxMessageSizemaxRequestSizefrom protocol-specific getter in constructorUtil.checkBodySize()@DeprecatedSizeLimitHandlerTestArgsTest