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 17 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 |
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
…ndler Add tests to cover scenarios raised in PR review: - Chunked (no Content-Length) requests within/exceeding the limit - Servlet broad catch(Exception) absorbing streaming BadMessageException - SizeLimitHandler behavior when limit is 0 (rejects all non-empty bodies) Replace EchoServlet with BroadCatchServlet to mirror real servlet chain. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace getInt() with getMemorySize().toBytes() for maxMessageSize, httpMaxMessageSize and jsonRpcMaxMessageSize config parsing. This enables human-readable size values (e.g. 4m, 128MB) while maintaining backward compatibility with raw byte values. - maxMessageSize (gRPC): keep int field, validate <= Integer.MAX_VALUE for gRPC API compatibility, add positive value validation - httpMaxMessageSize / jsonRpcMaxMessageSize: widen to long, matching Jetty SizeLimitHandler's long parameter type - Add config.conf documentation for all three size parameters Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Change validation from <= 0 to < 0 for all three size limit configs. Zero is a valid value meaning "reject all non-empty request bodies", with consistent semantics in both gRPC (maxInboundMessageSize >= 0) and Jetty SizeLimitHandler (limit >= 0 && size > limit). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add tests proving the two enforcement layers measure identical byte counts for normal JSON payloads (ASCII and UTF-8). For bodies with \r\n line endings, checkBodySize measures fewer bytes than wire — a safe direction that never causes false rejections. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Zero is a valid value for all three maxMessageSize parameters, so the config documentation should say "non-negative" not "positive". Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…iable name Initialize HttpService.maxRequestSize to 4MB so future subclasses that omit the assignment get a safe limit instead of 0 (reject all requests). Rename defaultHttpMaxMessageSize to defaultMaxMessageSize since it serves as the fallback for both HTTP and JSON-RPC limits. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add comprehensive ArgsTest coverage for getMemorySize() parsing: - Human-readable sizes across KB/MB/GB tiers (binary and SI units) - Raw integer backward compatibility - Zero-value acceptance - Error paths: exceeds int max, negative value, invalid unit, non-numeric Document zero-value behavior in config.conf for all three maxMessageSize entries: "Setting to 0 rejects all non-empty request bodies". Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
[SHOULD] With the updated PR scope, I no longer see the chunked |
|
[NIT] Note also that one comment has already drifted from the implementation: the Javadoc on |
Done. Removed all banner-style ruler comments and fixed the stale |
Agreed — the PR inserts SizeLimitHandler on the JSON-RPC path, so we should verify no regression on the real stack. Added
Scenario 3 deserves explanation: The silent swallowing in Also cleaned up See latest push. |
…Handler tests Add testJsonRpcSizeLimitIntegration() in JsonrpcServiceTest using the Spring-injected FullNodeJsonRpcHttpService (real JsonRpcServlet + jsonrpc4j) to verify SizeLimitHandler does not introduce regressions. Covers: normal request passthrough, Content-Length oversized 413, and chunked oversized behavior (200 empty body due to RateLimiterServlet absorbing BadMessageException). Clean up SizeLimitHandlerTest: remove 3 redundant testJsonRpcBody* tests that used BroadCatchServlet (cannot represent real jsonrpc4j chain), rename TestJsonRpcService to SecondHttpService, remove banner-style ruler comments, fix stale EchoServlet Javadoc reference, and remove HTML tags from Javadoc.
|
Re-checking the current PR head ( Specifically:
If there is a newer push on another branch or commit, please point me to it. Otherwise I think these threads should stay open until the code is actually updated in this PR. |
|
The push was a little late, but you can view it now. |
…nd fix comment attribution Add getMaxRequestSize()/setMaxRequestSize() to HttpService so tests use compile-safe accessors instead of Field.setAccessible(true). Correct comments attributing exception swallowing to RateLimiterServlet when it is actually jsonrpc4j that silently absorbs the BadMessageException.
| if (rpcMaxMessageSize < 0 || rpcMaxMessageSize > Integer.MAX_VALUE) { | ||
| throw new TronError("node.rpc.maxMessageSize must be non-negative and <= " | ||
| + Integer.MAX_VALUE + ", got: " + rpcMaxMessageSize, PARAMETER_INIT); | ||
| } |
There was a problem hiding this comment.
if (PARAMETER.httpMaxMessageSize <= 0) {
throw new TronError("node.http.maxMessageSize must be > 0, got: "
+ PARAMETER.httpMaxMessageSize, PARAMETER_INIT);
}
There was a problem hiding this comment.
I don’t think we should change this to <= 0 in this PR. The current implementation, config docs, and tests all explicitly treat 0 as a valid value meaning “reject all non-empty request bodies”, not as an invalid configuration.
Changing that now would be a behavior change rather than a local validation cleanup, and would require updating the documented semantics and the related tests as well.
| Util.checkBodySize(exceedsHttpLimit); | ||
| Assert.fail("expected exception for body exceeding httpMaxMessageSize"); | ||
| } catch (Exception e) { | ||
| Assert.assertTrue(e.getMessage().contains("200")); |
There was a problem hiding this comment.
Suggest use
Assert.assertThrows(Exception.class,
() -> Util.checkBodySize(exceedsHttpLimit));
Assert.assertTrue(e.getMessage().contains("200"));
to remove the try/catch block
| public void testJsonRpcSizeLimitIntegration() { | ||
| long testLimit = 1024; | ||
| try { | ||
| long originalLimit = fullNodeJsonRpcHttpService.getMaxRequestSize(); |
There was a problem hiding this comment.
[NIT] originalLimit is restored only in the inner finally, which is reached only after fullNodeJsonRpcHttpService.start() succeeds. If start() throws, this shared bean keeps the overridden limit for the rest of the class. Please move the setMaxRequestSize(originalLimit) restore into the outer finally so the test always leaves the service in its original state.
|
[NIT] A few of the newly added test comments use decorative non-ASCII punctuation such as |
|
[DISCUSS] |
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, preventing OOM denial-of-service from oversized payloads.Changes
node.http.maxMessageSizeandnode.jsonrpc.maxMessageSizeas independent, configurable size limits4m,128MB) via HOCONgetMemorySize()for all three size configsSizeLimitHandlerintoHttpService.initContextHandler()as the outermost handlerHttpServicesubclass (4 HTTP + 3 JSON-RPC) setsmaxRequestSizefrom the corresponding config gettermaxRequestSizewith a safe 4MB default to prevent silent reject-all if a future subclass omits the assignmentUtil.checkBodySize()— updated to usehttpMaxMessageSize, retained as fallback for backward compatibilitynode.rpc.maxMessageSizenow also supports human-readable sizes (backward compatible — bare integers still treated as bytes)Why 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:
Scope and known limitations
This PR's primary goal is OOM protection, not uniform HTTP 413 responses.
Chunked transfer behavior differs by servlet type due to a pre-existing exception handling design in the servlet chain:
Content-Lengthexceeds limitContent-Length, exceeds limit{"Error":"BadMessageException"}Root cause:
SizeLimitHandlertruncates body read at the limit and throwsBadMessageException(RuntimeException) during streaming.catch(Exception)→Util.processError()writes error JSON to the response body, so the client sees200 + {"Error":"..."}.200 + empty body.This is a pre-existing behavior of the jsonrpc4j library, not introduced by this PR. Any exception during request body parsing produces the same 200 empty body result, with or without SizeLimitHandler. OOM protection is effective in all cases — the body read is truncated regardless of the response status code. Returning 200 + empty body for malformed/oversized chunked requests is acceptable: it does not affect normal requests, increases attacker difficulty, and the alternative — pre-reading the request body to trigger the size limit (catch the exception to report the error), then wrapping the already-read body into an
HttpServletRequestWrapperto continue the normal request flow — adds significant complexity for marginal benefit.Closes #6604
This PR has been tested by
SizeLimitHandlerTest(10 tests): boundary, independent limits, UTF-8 byte counting, chunked transfer, zero-limit, checkBodySize consistencyJsonrpcServiceTest.testJsonRpcSizeLimitIntegration: real JSON-RPC integration test covering normal passthrough, Content-Length oversized (413), and chunked oversized (200 empty body)ArgsTest(5 new tests): human-readable sizes (KB/MB/GB × binary/SI), raw integer backward compatibility, zero-value, error paths (exceeds int max, negative, invalid unit, non-numeric)UtilTest: checkBodySize uses httpMaxMessageSizeFollow-up
Fix chunked oversized JSON-RPC to return proper error— Won't fix: exception is swallowed inside jsonrpc4j, not in our servlet code. Fixing requires pre-reading the request body to trigger the size limit (catch the exception to report the error), then wrapping the already-read body into anHttpServletRequestWrapperto continue the normal request flow — significant test complexity for marginal benefit. Current behavior (200 + empty body) is acceptable.Util.checkBodySize()callers once SizeLimitHandler is stableChanged files
16 files changed, +617 / -8
HttpServicemaxRequestSizefield (default 4MB), wireSizeLimitHandlerininitContextHandler()Args/ConfigKey/CommonParameternode.http.maxMessageSizeandnode.jsonrpc.maxMessageSize; refactor all three to usegetMemorySize()maxRequestSizefrom protocol-specific config getterUtil.checkBodySize()@Deprecated, switch tohttpMaxMessageSizeconfig.confSizeLimitHandlerTestJsonrpcServiceTesttestJsonRpcSizeLimitIntegration— real JSON-RPC integration testArgsTestUtilTest