feat(http): add SizeLimitHandler to enforce request body size limit#1
Open
feat(http): add SizeLimitHandler to enforce request body size limit#1
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds configurable per-service request-size limits: HttpService now wraps its ServletContextHandler with Jetty SizeLimitHandler and uses new Args/config keys and CommonParameter fields; multiple HTTP/JSON‑RPC services set maxRequestSize. Deprecates Util.checkBodySize and adds integration tests validating 200 vs 413 and UTF‑8 byte-length enforcement. Changes
Sequence DiagramsequenceDiagram
participant Client
participant SizeLimitHandler
participant ServletContextHandler
participant EchoServlet
Client->>SizeLimitHandler: POST /endpoint (body bytes)
alt body ≤ MaxMessageSize
SizeLimitHandler->>ServletContextHandler: forward request
ServletContextHandler->>EchoServlet: dispatch request
EchoServlet-->>Client: 200 OK (echo)
else body > MaxMessageSize
SizeLimitHandler-->>Client: 413 Payload Too Large
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 3 files
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
framework/src/test/java/org/tron/common/jetty/SizeLimitHandlerTest.java (2)
40-43: Add a multibyte UTF-8 test to match the stated behavior.Line 42 claims byte-based UTF-8 enforcement, but current tests only use single-byte ASCII payloads. Add one case with multibyte characters to prove the contract.
Also applies to: 119-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/test/java/org/tron/common/jetty/SizeLimitHandlerTest.java` around lines 40 - 43, Add a new test in SizeLimitHandlerTest (e.g. testUtf8MultibyteByteLimit) that constructs request bodies containing multibyte UTF-8 characters (for example "€" or an emoji) whose Java char count differs from UTF-8 byte count, then send requests against the existing size-limit handler asserting that a payload whose UTF-8 byte length is within the configured limit returns 200 and one whose UTF-8 byte length exceeds the limit returns 413; update or add both the earlier test block around lines ~40–43 and the later assertions around ~119–122 to include these multibyte cases so the test suite verifies byte-based (not char-based) enforcement.
127-133: Close HTTP responses explicitly using try-with-resources.The current code consumes the entity but does not close the
CloseableHttpResponse. Per Apache HttpClient 4.x documentation, closing the response object is mandatory to ensure proper resource deallocation. Usetry-with-resourceswithCloseableHttpResponse:Proposed refactor
+import org.apache.http.client.methods.CloseableHttpResponse; ... private int post(HttpEntity entity) throws Exception { HttpPost req = new HttpPost(serverUri); req.setEntity(entity); - HttpResponse resp = client.execute(req); - EntityUtils.consume(resp.getEntity()); - return resp.getStatusLine().getStatusCode(); + try (CloseableHttpResponse resp = client.execute(req)) { + EntityUtils.consume(resp.getEntity()); + return resp.getStatusLine().getStatusCode(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/test/java/org/tron/common/jetty/SizeLimitHandlerTest.java` around lines 127 - 133, The post method does not close the HTTP response; update the post(HttpEntity entity) method to use a try-with-resources block that obtains a CloseableHttpResponse from client.execute(req) (replace HttpResponse with CloseableHttpResponse) and ensure EntityUtils.consume(resp.getEntity()) is called inside the try so the response is closed automatically; keep returning resp.getStatusLine().getStatusCode() but read it from the closed-response variable inside the try block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/src/main/java/org/tron/common/application/HttpService.java`:
- Around line 67-70: The maxMessageSize returned by
Args.getInstance().getMaxMessageSize() is used directly to construct a
SizeLimitHandler in HttpService, but Jetty only supports values >= -1 and a
value of 0 is nonsensical; add a startup guard in HttpService before new
SizeLimitHandler(...) that validates the value (require maxMessageSize == -1 ||
maxMessageSize >= 1), and if invalid throw an IllegalArgumentException (or log
error and exit) with a clear message referencing maxMessageSize so the process
fails fast and apiServer.setHandler(...) is never called with an unsupported
size.
In `@framework/src/test/java/org/tron/common/jetty/SizeLimitHandlerTest.java`:
- Line 92: The test calls httpService.start() (in SizeLimitHandlerTest) but
doesn't wait for the returned CompletableFuture<Boolean> to complete; update the
setup to synchronously wait for startup (e.g., call start().get(...) or
start().join() with a reasonable timeout and assert the result is true) and
propagate or fail the test on interruption/exception so the test only proceeds
after the service has fully started.
---
Nitpick comments:
In `@framework/src/test/java/org/tron/common/jetty/SizeLimitHandlerTest.java`:
- Around line 40-43: Add a new test in SizeLimitHandlerTest (e.g.
testUtf8MultibyteByteLimit) that constructs request bodies containing multibyte
UTF-8 characters (for example "€" or an emoji) whose Java char count differs
from UTF-8 byte count, then send requests against the existing size-limit
handler asserting that a payload whose UTF-8 byte length is within the
configured limit returns 200 and one whose UTF-8 byte length exceeds the limit
returns 413; update or add both the earlier test block around lines ~40–43 and
the later assertions around ~119–122 to include these multibyte cases so the
test suite verifies byte-based (not char-based) enforcement.
- Around line 127-133: The post method does not close the HTTP response; update
the post(HttpEntity entity) method to use a try-with-resources block that
obtains a CloseableHttpResponse from client.execute(req) (replace HttpResponse
with CloseableHttpResponse) and ensure EntityUtils.consume(resp.getEntity()) is
called inside the try so the response is closed automatically; keep returning
resp.getStatusLine().getStatusCode() but read it from the closed-response
variable inside the try block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3995e317-a92d-45c4-ac9b-e5d7296b806c
📒 Files selected for processing (3)
framework/src/main/java/org/tron/common/application/HttpService.javaframework/src/main/java/org/tron/core/services/http/Util.javaframework/src/test/java/org/tron/common/jetty/SizeLimitHandlerTest.java
bladehan1
changed the title
bladehan1
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (2)
framework/src/test/java/org/tron/common/jetty/SizeLimitHandlerTest.java (2)
102-103: Redundant Args configuration.These lines set max message sizes on
Args, butTestHttpServiceandTestJsonRpcServicereceive their limits directly via constructor parameters (lines 106, 111) and don't read fromArgs. This code has no effect on test behavior and could be removed for clarity.Suggested removal
Args.setParam(new String[]{"-d", temporaryFolder.newFolder().toString()}, TestConstants.TEST_CONF); - Args.getInstance().setHttpMaxMessageSize(HTTP_MAX_BODY_SIZE); - Args.getInstance().setJsonRpcMaxMessageSize(JSONRPC_MAX_BODY_SIZE); int httpPort = PublicMethod.chooseRandomPort();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/test/java/org/tron/common/jetty/SizeLimitHandlerTest.java` around lines 102 - 103, Remove the redundant global Args configuration that sets HTTP_MAX_BODY_SIZE and JSONRPC_MAX_BODY_SIZE via Args.getInstance().setHttpMaxMessageSize(...) and setJsonRpcMaxMessageSize(...); these calls have no effect because TestHttpService and TestJsonRpcService receive their limits directly through constructor parameters (see TestHttpService and TestJsonRpcService instantiations using HTTP_MAX_BODY_SIZE and JSONRPC_MAX_BODY_SIZE), so delete those two set* calls to avoid confusion and clarify the test setup.
31-42: Documentation claims default value testing, but no test verifies defaults.Line 41 states the test covers "Default values are 4x
GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE", but all tests use explicit limits passed to the service constructors. Consider either adding a test that verifies the actual default configuration or removing this claim from the documentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/test/java/org/tron/common/jetty/SizeLimitHandlerTest.java` around lines 31 - 42, The Javadoc claims default limits are 4x GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE but no test verifies that; either add a unit test in SizeLimitHandlerTest that constructs the service using the no-argument/default constructor (or calls HttpService.initContextHandler() without passing explicit limits), retrieves the configured SizeLimitHandler (or exercises the endpoint with payloads sized at GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE * 4 and *4+1) and asserts the behavior matches the 4x default, or simply remove the bullet "Default values are 4x GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE" from the class Javadoc if you don’t want to add a runtime verification; look for the test class SizeLimitHandlerTest and the HttpService initContextHandler()/constructor usages to implement the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@framework/src/test/java/org/tron/common/jetty/SizeLimitHandlerTest.java`:
- Around line 102-103: Remove the redundant global Args configuration that sets
HTTP_MAX_BODY_SIZE and JSONRPC_MAX_BODY_SIZE via
Args.getInstance().setHttpMaxMessageSize(...) and setJsonRpcMaxMessageSize(...);
these calls have no effect because TestHttpService and TestJsonRpcService
receive their limits directly through constructor parameters (see
TestHttpService and TestJsonRpcService instantiations using HTTP_MAX_BODY_SIZE
and JSONRPC_MAX_BODY_SIZE), so delete those two set* calls to avoid confusion
and clarify the test setup.
- Around line 31-42: The Javadoc claims default limits are 4x
GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE but no test verifies that; either add a unit
test in SizeLimitHandlerTest that constructs the service using the
no-argument/default constructor (or calls HttpService.initContextHandler()
without passing explicit limits), retrieves the configured SizeLimitHandler (or
exercises the endpoint with payloads sized at GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE
* 4 and *4+1) and asserts the behavior matches the 4x default, or simply remove
the bullet "Default values are 4x GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE" from the
class Javadoc if you don’t want to add a runtime verification; look for the test
class SizeLimitHandlerTest and the HttpService initContextHandler()/constructor
usages to implement the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16fd687d-e769-4afd-9430-005c0b361dbb
📒 Files selected for processing (1)
framework/src/test/java/org/tron/common/jetty/SizeLimitHandlerTest.java
bladehan1
force-pushed
the
feat/request_size
branch
from
24abc3a to
3048750
Compare
There was a problem hiding this comment.
4 issues found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="framework/src/main/java/org/tron/core/config/args/Args.java">
<violation number="1" location="framework/src/main/java/org/tron/core/config/args/Args.java:477">
P2: Reject zero for `node.rpc.maxMessageSize`; allowing 0 creates an unusable RPC payload limit.</violation>
<violation number="2" location="framework/src/main/java/org/tron/core/config/args/Args.java:487">
P2: `node.http.maxMessageSize` should remain strictly positive; allowing 0 makes HTTP endpoints reject normal request bodies.</violation>
<violation number="3" location="framework/src/main/java/org/tron/core/config/args/Args.java:494">
P2: `node.jsonrpc.maxMessageSize` should reject 0 to avoid a silently unusable JSON-RPC API limit.</violation>
</file>
<file name="framework/src/test/java/org/tron/common/jetty/SizeLimitHandlerTest.java">
<violation number="1" location="framework/src/test/java/org/tron/common/jetty/SizeLimitHandlerTest.java:75">
P2: `body.getBytes()` uses the platform default charset, making UTF-8 byte-length assertions environment-dependent.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
…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
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.
bladehan1
force-pushed
the
feat/request_size
branch
from
64ca1bd to
bb14edd
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
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