Skip to content

feat(http): add SizeLimitHandler to enforce request body size limit#6658

Open
bladehan1 wants to merge 21 commits intotronprotocol:developfrom
bladehan1:feat/request_size
Open

feat(http): add SizeLimitHandler to enforce request body size limit#6658
bladehan1 wants to merge 21 commits intotronprotocol:developfrom
bladehan1:feat/request_size

Conversation

@bladehan1
Copy link
Copy Markdown
Collaborator

@bladehan1 bladehan1 commented Apr 8, 2026

What does this PR do?

Add Jetty SizeLimitHandler at 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

  • Introduce node.http.maxMessageSize and node.jsonrpc.maxMessageSize as independent, configurable size limits
  • Default: 4 MB, consistent with gRPC defaults
  • Support human-readable size values (e.g. 4m, 128MB) via HOCON getMemorySize() for all three size configs
  • Wire SizeLimitHandler into HttpService.initContextHandler() as the outermost handler
  • Each HttpService subclass (4 HTTP + 3 JSON-RPC) sets maxRequestSize from the corresponding config getter
  • Initialize maxRequestSize with a safe 4MB default to prevent silent reject-all if a future subclass omits the assignment
  • Deprecate Util.checkBodySize() — updated to use httpMaxMessageSize, retained as fallback for backward compatibility
  • Existing node.rpc.maxMessageSize now 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:

  1. OOM protection — oversized payloads are never fully buffered into memory
  2. Streaming enforcement — limits are enforced during read, not after full buffering
  3. Unified coverage — all HTTP and JSON-RPC endpoints are protected by a single mechanism
  4. Independent tuning — operators can configure HTTP and JSON-RPC limits separately

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:

Request type HTTP Servlets (132) JSON-RPC Servlet
Content-Length exceeds limit 413 (Jetty rejects before dispatch) 413 (same)
Chunked / no Content-Length, exceeds limit 200 + error JSON {"Error":"BadMessageException"} 200 + empty body

Root cause: SizeLimitHandler truncates body read at the limit and throws BadMessageException (RuntimeException) during streaming.

  • HTTP servlets have their own catch(Exception)Util.processError() writes error JSON to the response body, so the client sees 200 + {"Error":"..."}.
  • JsonRpcServlet delegates to jsonrpc4j, which internally catches exceptions during request parsing and returns an empty response → client sees 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 HttpServletRequestWrapper to continue the normal request flow — adds significant complexity for marginal benefit.

Closes #6604

Migration

This PR makes node.http.maxMessageSize and node.jsonrpc.maxMessageSize independent of node.rpc.maxMessageSize. Previously, HTTP servlets enforced their body limit through Util.checkBodySize(), which read the gRPC limit (node.rpc.maxMessageSize); JSON-RPC had no body-size validation at all.

Behavior change:

Limit Before After
HTTP body bounded by node.rpc.maxMessageSize (via Util.checkBodySize) node.http.maxMessageSize, default 4 MB
JSON-RPC body not validated node.jsonrpc.maxMessageSize, default 4 MB
gRPC message node.rpc.maxMessageSize, default 4 MB unchanged

Affected deployments: those that previously set node.rpc.maxMessageSize above 4 MB (for example 8m) 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 through node.rpc.maxMessageSize so that the new "independent per-protocol limit" contract stays clean.

Required action for affected operators:

node {
  rpc.maxMessageSize     = 8m   # gRPC, unchanged
  http.maxMessageSize    = 8m   # NEW: set explicitly to preserve prior HTTP body limit
  jsonrpc.maxMessageSize = 8m   # NEW: opt in to enable JSON-RPC body validation at the desired size
}

Deployments that did not customize node.rpc.maxMessageSize are 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 consistency
  • JsonrpcServiceTest.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 httpMaxMessageSize

Follow-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 an HttpServletRequestWrapper to continue the normal request flow — significant test complexity for marginal benefit. Current behavior (200 + empty body) is acceptable.
  • Remove Util.checkBodySize() callers once SizeLimitHandler is stable

Changed files

Component Changes
HttpService Add maxRequestSize field (default 4MB), wire SizeLimitHandler in initContextHandler()
Args / ConfigKey / CommonParameter Parse node.http.maxMessageSize and node.jsonrpc.maxMessageSize; refactor all three to use getMemorySize(); extract parseMaxMessageSize private helper to dedupe range-check / cast logic; comment legacy maxMessageSize field as the gRPC counterpart
7 service subclasses Set maxRequestSize from protocol-specific config getter
Util.checkBodySize() Mark @Deprecated, switch to httpMaxMessageSize
config.conf Add documented config examples for HTTP, RPC, JSON-RPC size limits with zero-value behavior
SizeLimitHandlerTest New: 10 tests covering HTTP limits, boundary, UTF-8, chunked, independence, zero-limit, checkBodySize consistency
JsonrpcServiceTest New: testJsonRpcSizeLimitIntegration — real JSON-RPC integration test
ArgsTest New: 5 tests for human-readable parsing (KB/MB/GB), error paths
UtilTest New: checkBodySize consistency test

@xxo1shine
Copy link
Copy Markdown
Collaborator

@bladehan1 One observation on the config validation in Args.java: the guard currently rejects <= 0 values with TronError, but there is no upper-bound check. An operator who accidentally sets node.http.maxMessageSize = 2147483647 would silently run with a 2 GB limit. Adding a reasonable maximum (e.g. 128 MB) with an explicit warn-or-throw would make misconfiguration more visible.

@bladehan1
Copy link
Copy Markdown
Collaborator Author

@xxo1shine
Thanks for the review. I think that when users manually configure values, it's unnecessary to set all boundaries except for error-prone values. Especially for values ​​that are obviously likely to have problems, setting boundaries is unnecessary.

Comment thread framework/src/main/java/org/tron/core/services/http/Util.java
@halibobo1205 halibobo1205 added this to the GreatVoyage-v4.8.2 milestone Apr 9, 2026
@halibobo1205 halibobo1205 added topic:api rpc/http related issue Improvement labels Apr 9, 2026
@bladehan1 bladehan1 force-pushed the feat/request_size branch from 24abc3a to 3048750 Compare April 9, 2026 10:43
Comment thread framework/src/main/java/org/tron/core/config/args/Args.java Outdated
Comment thread framework/src/main/java/org/tron/core/config/args/Args.java Outdated
Comment thread framework/src/main/java/org/tron/core/config/args/Args.java Outdated
Comment thread framework/src/main/resources/config.conf
@waynercheung
Copy link
Copy Markdown
Collaborator

[SHOULD] With the updated PR scope, I no longer see the chunked 413 gap itself as a contract mismatch, since the description now explicitly scopes this PR to OOM protection rather than uniform HTTP status behavior. The remaining gap is test realism: SizeLimitHandlerTest still does not exercise the production JSON-RPC stack, because the /jsonrpc path is backed by a synthetic BroadCatchServlet, not the real JsonRpcServlet -> jsonrpc4j chain. Since the PR still claims protection for JSON-RPC endpoints, I strongly recommend either adding one real JSON-RPC integration case or making the test naming/comments explicit that this is validating generic HttpInput interception rather than end-to-end JSON-RPC behavior.

Comment thread framework/src/main/java/org/tron/core/services/http/Util.java
@waynercheung
Copy link
Copy Markdown
Collaborator

waynercheung commented Apr 14, 2026

[NIT] SizeLimitHandlerTest relies heavily on banner-style ruler comments such as // -- ... -------- throughout the file. This style adds visual noise without structural value, and is harder to maintain - new tests may not fit neatly into an existing section, and the dashes need manual alignment. The usual best practice here is to rely on descriptive test names, minimal section comments, and helper methods / smaller test classes for grouping, rather than heavy visual separators.

Note also that one comment has already drifted from the implementation: the Javadoc on testChunkedBodyWithinLimit still reads "succeed (EchoServlet)", but the servlet under test is now BroadCatchServlet. Please update that reference while cleaning up the comments.

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
checkBodySize() was enforcing maxMessageSize (gRPC limit) instead of
httpMaxMessageSize, causing the independent HTTP size setting to be
ineffective at the servlet layer.
…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.
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
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).
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.
Zero is a valid value for all three maxMessageSize parameters,
so the config documentation should say "non-negative" not "positive".
…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.
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.
Copy link
Copy Markdown
Collaborator

@waynercheung waynercheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 through Util.checkBodySize(). After this change, set node.http.maxMessageSize (and node.jsonrpc.maxMessageSize if 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 = 4m is a new enforcement point. Operators should review client request peak sizes before upgrade.

@bladehan1
Copy link
Copy Markdown
Collaborator Author

[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.

Agreed, this is a real silent behavior change on upgrade and should not stay implicit. I've added a ## Migration subsection to the PR description covering:

  • Before/after table for HTTP, JSON-RPC, gRPC body limits
  • Which deployments are affected (those that bumped node.rpc.maxMessageSize above 4 MB and relied on it covering HTTP)
  • A concrete node { ... } snippet showing the explicit keys to set to preserve prior behavior
  • An explicit note that the HTTP fallback through node.rpc.maxMessageSize was intentionally not preserved, in order to keep the new "independent per-protocol limit" contract clean — matching your point that restoring the fallback would blur it

I deliberately did not add a migration note to config.conf itself: config files document current behavior, not transition history, and accumulating historical notes there ages poorly. Migration docs belong in PR description / release notes, which is where this lives now.

@@ -216,6 +216,12 @@ public class CommonParameter {
public int maxMessageSize;
Copy link
Copy Markdown
Contributor

@Sunny6889 Sunny6889 Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[NIT] Please add a comments to state maxMessageSize refere to rpc max message size. To make it easy to understand comparing with the below httpMaxMessageSize jsonRpcMaxMessageSize

throw new TronError("node.jsonrpc.maxMessageSize must be non-negative and <= "
+ Integer.MAX_VALUE + ", got: "
+ PARAMETER.jsonRpcMaxMessageSize, PARAMETER_INIT);
}
Copy link
Copy Markdown
Contributor

@Sunny6889 Sunny6889 Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[NIT] The three maxMessageSize parsing blocks in applyConfigParams() are structurally identical — read from config (with fallback), range-check, cast to int, assign. Having them inlined one after another in a 500+ line method makes it easy to miss a subtle difference between the three.

Consider extracting a small private helper:

private static int parseMaxMessageSize(Config config, String key, long defaultBytes) {
       long value = config.hasPath(key)
           ? config.getMemorySize(key).toBytes()
           : defaultBytes;
       if (value < 0 || value > Integer.MAX_VALUE) {
           throw new TronError(key + " must be non-negative and <= "
               + Integer.MAX_VALUE + ", got: " + value, PARAMETER_INIT);
       }
       return (int) value;
 }

Then the three assignment lines become:

   long defaultSize = GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE;
   PARAMETER.maxMessageSize     = parseMaxMessageSize(config, NODE_RPC_MAX_MESSAGE_SIZE,     defaultSize);
   PARAMETER.httpMaxMessageSize = parseMaxMessageSize(config, NODE_HTTP_MAX_MESSAGE_SIZE,    defaultSize);
   PARAMETER.jsonRpcMaxMessageSize = parseMaxMessageSize(config, NODE_JSONRPC_MAX_MESSAGE_SIZE, defaultSize);

This removes ~20 lines of near-duplicate code, makes the error messages consistent, and makes it trivial to add a fourth protocol limit in the future without copy-paste risk.

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.
@bladehan1
Copy link
Copy Markdown
Collaborator Author

Both addressed — pushed as a single follow-up commit.

1. Legacy naming comment on maxMessageSize

Added a brief two-line comment on CommonParameter.maxMessageSize clarifying that the legacy field refers to the gRPC limit, alongside the explicit httpMaxMessageSize / jsonRpcMaxMessageSize siblings. Kept it as a plain // comment (not Javadoc) to match the rest of the field block, which is uncommented. A rename to rpcMaxMessageSize would be the cleaner long-term fix, but it's a public field with cross-module callers, so out of scope here.

2. Extracted parseMaxMessageSize helper

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 long rather than int, because httpMaxMessageSize and jsonRpcMaxMessageSize are typed long (matching Jetty's SizeLimitHandler(long, long) expectation). The rpc assignment retains an explicit (int) cast at the call site to keep the gRPC int32 hard constraint visible — same idea as the cap rationale we discussed earlier.

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);

ArgsTest (including the error-path cases for each of the three keys) passes unchanged.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Improvement topic:api rpc/http related issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Unified HTTP Request Body Size Limit

7 participants