feat(http): add SizeLimitHandler to enforce request body size limit#6658
Conversation
|
@bladehan1 One observation on the config validation in |
|
@xxo1shine |
24abc3a to
3048750
Compare
|
[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 |
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".
…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.
…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.
…, finally guard - Replace try/catch/fail pattern with Assert.assertThrows in UtilTest and ArgsTest (4 cases) - Replace non-ASCII punctuation (→, —) with ASCII (->, -) in newly added test comments - Hoist originalLimit before outer try in testJsonRpcSizeLimitIntegration so restore executes even when start() throws
Align node.http.maxMessageSize and node.jsonrpc.maxMessageSize with the existing node.rpc.maxMessageSize rule: reject values > Integer.MAX_VALUE at startup with TronError. Downstream body materialization is int-bounded (String/byte[]/StringBuilder all cap at Integer.MAX_VALUE), so config values beyond that produced an ambiguous silent mismatch; fail-fast is safer than a runtime NegativeArraySizeException or truncation surprise. Update config.conf docblocks (http/rpc/jsonrpc) to state the full constraint. Replace the obsolete 2Gi success assertion in testMaxMessageSizeHumanReadable, add testHttpMaxMessageSizeExceedsIntMax and testJsonRpcMaxMessageSizeExceedsIntMax mirroring the existing rpc case.
64ca1bd to
bb14edd
Compare
waynercheung
left a comment
There was a problem hiding this comment.
[DISCUSS] One compatibility point is worth calling out explicitly in the PR description / release notes: before this change, many existing HTTP servlet paths were effectively bounded by node.rpc.maxMessageSize via Util.checkBodySize(). After introducing independent node.http.maxMessageSize, deployments that previously increased only node.rpc.maxMessageSize (for example to 8m) will fall back to the new 4 MB HTTP default unless they set node.http.maxMessageSize explicitly.
I'm not suggesting we restore rpcMaxMessageSize as the HTTP fallback, because that would blur the new "independent per-protocol config" contract again. But the migration impact should be documented clearly so operators do not get a silent behavior change on upgrade.
Suggest adding a short Migration subsection to the PR description, for example:
If your deployment previously customized
node.rpc.maxMessageSize(for example= 8m), HTTP endpoints were implicitly bounded by that value throughUtil.checkBodySize(). After this change, setnode.http.maxMessageSize(andnode.jsonrpc.maxMessageSizeif applicable) explicitly if you want to preserve the previous effective limit.Default deployments (all three at 4 MB) are unaffected.
Note: JSON-RPC endpoints previously had no unified pre-ingress size limit, so
node.jsonrpc.maxMessageSize = 4mis a new enforcement point. Operators should review client request peak sizes before upgrade.
Agreed, this is a real silent behavior change on upgrade and should not stay implicit. I've added a
I deliberately did not add a migration note to |
Dedupe three near-identical maxMessageSize parsing blocks in applyConfigParams() (rpc, http, jsonrpc) into a single private static helper. Helper returns long to match the wider-typed http/jsonrpc fields; the rpc assignment retains an explicit (int) cast at the call site to make the gRPC int32 hard constraint visible. Net effect: ~16 lines removed, error messages guaranteed consistent across the three protocols, and adding a future protocol limit becomes a one-liner instead of a copy-paste block. Also add a brief comment on CommonParameter.maxMessageSize clarifying that the legacy field name refers to the gRPC limit, alongside the explicit httpMaxMessageSize / jsonRpcMaxMessageSize siblings.
|
Both addressed — pushed as a single follow-up commit. 1. Legacy naming comment on Added a brief two-line comment on 2. Extracted Net effect: ~16 lines removed across the three blocks, error messages now guaranteed consistent (the previous string-concat blocks had subtly diverged on line breaking), and adding a future protocol limit becomes a one-liner. One adjustment to your suggested signature: the helper returns Resulting call sites: long defaultMaxMessageSize = GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE;
PARAMETER.maxMessageSize = (int) parseMaxMessageSize(
config, ConfigKey.NODE_RPC_MAX_MESSAGE_SIZE, defaultMaxMessageSize);
PARAMETER.httpMaxMessageSize = parseMaxMessageSize(
config, ConfigKey.NODE_HTTP_MAX_MESSAGE_SIZE, defaultMaxMessageSize);
PARAMETER.jsonRpcMaxMessageSize = parseMaxMessageSize(
config, ConfigKey.NODE_JSONRPC_MAX_MESSAGE_SIZE, defaultMaxMessageSize);
|
Resolves conflicts from upstream's ConfigBeanFactory refactor (c977f82) by adapting maxMessageSize logic to the new model: - Add maxMessageSize fields to NodeConfig.HttpConfig/JsonRpcConfig. - Add reference.conf defaults for the two new keys, using human-readable 4M form (consistent with the human-readable input the parsing layer already accepts; numeric byte form would require operators to do mental math). - Inside NodeConfig.fromConfig, pre-normalize the three maxMessageSize paths (rpc/http/jsonrpc): parse via Config.getMemorySize so values like "4m" / "128MB" become numeric bytes before ConfigBeanFactory's primitive int/long binding, and validate non-negative and <= Integer.MAX_VALUE so failures point at the user-facing config path. This matches BlockConfig.fromConfig's validation-in-fromConfig convention and keeps Args.applyNodeConfig as a pure bean-to-PARAMETER bridge. - Drop ConfigKey.java (deleted upstream); the two NODE_*_MAX_MESSAGE_SIZE constants are no longer needed since keys are derived from bean field names. Tests: ArgsTest (18), NodeConfigTest (29), SizeLimitHandlerTest, UtilTest, JsonrpcServiceTest all pass under the merged tree; framework checkstyleMain and checkstyleTest pass.
|
Resolved conflict with the develop branch. |
Pulls in 12 new upstream commits since the previous merge. Conflicts resolved: - ArgsTest.java: keep upstream's testAllowShieldedTransactionApiDefault alongside our 7 maxMessageSize tests; one closing brace at end of class. Other touchpoints in our hot zone (Args.java, NodeConfig.java, reference.conf, NodeConfigTest.java, Util.java) auto-merged cleanly; maxMessageSize logic (normalize/parse in NodeConfig.fromConfig, HttpConfig/JsonRpcConfig fields, reference.conf 4M defaults) is preserved. Compile: :common:compileJava + :framework:compileJava + :framework:compileTestJava all SUCCESSFUL.
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
Migration
This PR makes
node.http.maxMessageSizeandnode.jsonrpc.maxMessageSizeindependent ofnode.rpc.maxMessageSize. Previously, HTTP servlets enforced their body limit throughUtil.checkBodySize(), which read the gRPC limit (node.rpc.maxMessageSize); JSON-RPC had no body-size validation at all.Behavior change:
node.rpc.maxMessageSize(viaUtil.checkBodySize)node.http.maxMessageSize, default 4 MBnode.jsonrpc.maxMessageSize, default 4 MBnode.rpc.maxMessageSize, default 4 MBAffected deployments: those that previously set
node.rpc.maxMessageSizeabove 4 MB (for example8m) and relied on the same limit applying to HTTP bodies. After upgrading, HTTP and JSON-RPC fall back to the new 4 MB default unless the matching key is set explicitly. The HTTP fallback was intentionally not preserved throughnode.rpc.maxMessageSizeso that the new "independent per-protocol limit" contract stays clean.Required action for affected operators:
Deployments that did not customize
node.rpc.maxMessageSizeare unaffected (all three default to 4 MB).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
HttpServicemaxRequestSizefield (default 4MB), wireSizeLimitHandlerininitContextHandler()Args/ConfigKey/CommonParameternode.http.maxMessageSizeandnode.jsonrpc.maxMessageSize; refactor all three to usegetMemorySize(); extractparseMaxMessageSizeprivate helper to dedupe range-check / cast logic; comment legacymaxMessageSizefield as the gRPC counterpartmaxRequestSizefrom protocol-specific config getterUtil.checkBodySize()@Deprecated, switch tohttpMaxMessageSizeconfig.confSizeLimitHandlerTestJsonrpcServiceTesttestJsonRpcSizeLimitIntegration— real JSON-RPC integration testArgsTestUtilTest