Skip to content

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

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

feat(http): add SizeLimitHandler to enforce request body size limit#6658
bladehan1 wants to merge 18 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

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

16 files changed, +617 / -8

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

…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
@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
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>
@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
…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.
@waynercheung
Copy link
Copy Markdown
Collaborator

Re-checking the current PR head (6f0a0f0), I still do not see the changes described in the reply on this branch yet.

Specifically:

  • SizeLimitHandlerTest still contains the banner-style section comments, and the stale EchoServlet reference is still present.
  • I do not see the new testJsonRpcSizeLimitIntegration() in JsonrpcServiceTest.
  • The synthetic /jsonrpc tests in SizeLimitHandlerTest are still present.
  • Util.checkBodySize() is still using body.getBytes() with the platform default charset.

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.

@bladehan1
Copy link
Copy Markdown
Collaborator Author

@waynercheung

The push was a little late, but you can view it now.
As mentioned above, Util.checkBodySize() no longer adds body.getBytes.
Another issue has been updated.

…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.
Comment thread framework/src/main/java/org/tron/core/config/args/Args.java
Comment thread framework/src/test/java/org/tron/core/services/http/UtilTest.java Outdated
Comment thread framework/src/test/java/org/tron/core/jsonrpc/JsonrpcServiceTest.java Outdated
@waynercheung
Copy link
Copy Markdown
Collaborator

[NIT] A few of the newly added test comments use decorative non-ASCII punctuation such as and . This is not a correctness issue, and Google Java Style does allow Unicode where it improves readability, but these particular symbols are decorative rather than semantic. For consistency with the rest of the Java codebase, I’d prefer plain ASCII punctuation (->, -) in comments. The UTF-8 test literals themselves (for example / 测试地址) are of course justified and should stay.

@waynercheung
Copy link
Copy Markdown
Collaborator

[DISCUSS] node.rpc.maxMessageSize already has a stronger fail-fast guard because the gRPC API ultimately requires an int (<= Integer.MAX_VALUE). By contrast, node.http.maxMessageSize and node.jsonrpc.maxMessageSize are currently accepted as any non-negative long. I’m not convinced they should be forced to the same Integer.MAX_VALUE cap, but do we want some operator guardrail here as well (for example a startup warning or a documented recommended upper bound for suspiciously large values)? Very large HTTP / JSON-RPC limits materially weaken the intended OOM protection, and values above Integer.MAX_VALUE also exceed the effective range of the deprecated checkBodySize() fallback.

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

bladehan1 commented Apr 20, 2026

Done. Replaced all and with -> and - in newly added test comments across SizeLimitHandlerTest and JsonrpcServiceTest.

@bladehan1
Copy link
Copy Markdown
Collaborator Author

bladehan1 commented Apr 20, 2026

[DISCUSS] node.rpc.maxMessageSize already has a stronger fail-fast guard because the gRPC API ultimately requires an int (<= Integer.MAX_VALUE). By contrast, node.http.maxMessageSize and node.jsonrpc.maxMessageSize are currently accepted as any non-negative long. I’m not convinced they should be forced to the same Integer.MAX_VALUE cap, but do we want some operator guardrail here as well (for example a startup warning or a documented recommended upper bound for suspiciously large values)? Very large HTTP / JSON-RPC limits materially weaken the intended OOM protection, and val

Valid concern. For this PR the focus is OOM baseline protection — adding an upper-bound guardrail here would expand scope and potentially delay the core fix. A startup warning for suspiciously large values (e.g. > 128 MB) is worth a follow-up, but I'd prefer to land this first.

@waynercheung
Copy link
Copy Markdown
Collaborator

For this PR the focus is OOM baseline protection — adding an upper-bound guardrail here would expand scope and potentially delay the core fix.

[SHOULD] I agree this is not the same kind of hard API constraint as gRPC, but I still think the config layer should fail fast for HTTP / JSON-RPC values above Integer.MAX_VALUE in this PR.

Reason: although Jetty accepts long, the current java-tron request-processing path does not realistically support arbitrarily large bodies. Many downstream paths still materialize request bodies into String / byte[], and once httpMaxMessageSize > Integer.MAX_VALUE, the deprecated checkBodySize() fallback also becomes ineffective.

Since this PR is already touching the shared maxMessageSize parsing/validation block, aligning the effective supported range here would be a small change with good operational safety benefits.

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