-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat(http): add SizeLimitHandler to enforce request body size limit #6658
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
50496d5
736f2ed
b2ade66
c41d30e
321e26a
e6b11a3
674e547
3048750
5e35c4c
aa34bfc
c64d3b2
5e3eed0
de76e5f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ | |
| import lombok.extern.slf4j.Slf4j; | ||
| import org.eclipse.jetty.server.ConnectionLimit; | ||
| import org.eclipse.jetty.server.Server; | ||
| import org.eclipse.jetty.server.handler.SizeLimitHandler; | ||
| import org.eclipse.jetty.servlet.ServletContextHandler; | ||
| import org.tron.core.config.args.Args; | ||
|
|
||
|
|
@@ -29,6 +30,8 @@ public abstract class HttpService extends AbstractService { | |
|
|
||
| protected String contextPath; | ||
|
|
||
| protected long maxRequestSize; | ||
|
|
||
| @Override | ||
| public void innerStart() throws Exception { | ||
| if (this.apiServer != null) { | ||
|
|
@@ -63,7 +66,9 @@ protected void initServer() { | |
| protected ServletContextHandler initContextHandler() { | ||
| ServletContextHandler context = new ServletContextHandler(ServletContextHandler.SESSIONS); | ||
| context.setContextPath(this.contextPath); | ||
| this.apiServer.setHandler(context); | ||
| SizeLimitHandler sizeLimitHandler = new SizeLimitHandler(this.maxRequestSize, -1); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. I've verified the behavior with tests and here's what actually happens: With Chunked (no The OOM protection goal is still met: the body read is truncated at the limit — the full oversized payload is never buffered into memory, which is the core security objective of this PR. The 413-vs-200 discrepancy for chunked requests is a consequence of the existing broad I'll add chunked transfer tests to document and assert this behavior.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bladehan1 Thanks for validating the streaming path. I think we should separate plain HTTP servlets from JSON-RPC here, because the real control flow is different. For the plain HTTP servlet endpoints, the current inconsistency looks fixable in this PR with limited blast radius. For JSON-RPC, we need to clarify the contract first. Fixed-length oversized requests are already rejected before If the intended contract is uniform 413 for oversized requests, I think HTTP should be fixed in this PR, and JSON-RPC needs an explicit integration fix in the jsonrpc4j layer (for example a custom
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we fail fast here if maxRequestSize was never initialized? This field currently relies on every HttpService subclass remembering to assign it, and the Java default of 0 turns into an accidental zero-byte limit. An explicit validation here would be safer than silently inheriting a bad default.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the suggestion. After consideration, I don't think a fail-fast check is needed here:
So the current design relies on config validation + operator intent, which I think is the right layer for this.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think allowing operator-configured Since the code now explicitly treats |
||
| sizeLimitHandler.setHandler(context); | ||
| this.apiServer.setHandler(sizeLimitHandler); | ||
| return context; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -471,8 +471,30 @@ public static void applyConfigParams( | |
| ? config.getLong(ConfigKey.NODE_RPC_MAX_CONNECTION_AGE_IN_MILLIS) | ||
| : Long.MAX_VALUE; | ||
|
|
||
| PARAMETER.maxMessageSize = config.hasPath(ConfigKey.NODE_RPC_MAX_MESSAGE_SIZE) | ||
| ? config.getInt(ConfigKey.NODE_RPC_MAX_MESSAGE_SIZE) : GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE; | ||
| long rpcMaxMessageSize = config.hasPath(ConfigKey.NODE_RPC_MAX_MESSAGE_SIZE) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add targeted |
||
| ? config.getMemorySize(ConfigKey.NODE_RPC_MAX_MESSAGE_SIZE).toBytes() | ||
| : GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE; | ||
| 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); | ||
| } | ||
| PARAMETER.maxMessageSize = (int) rpcMaxMessageSize; | ||
|
|
||
| long defaultHttpMaxMessageSize = GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE; | ||
| PARAMETER.httpMaxMessageSize = config.hasPath(ConfigKey.NODE_HTTP_MAX_MESSAGE_SIZE) | ||
| ? config.getMemorySize(ConfigKey.NODE_HTTP_MAX_MESSAGE_SIZE).toBytes() | ||
| : defaultHttpMaxMessageSize; | ||
| if (PARAMETER.httpMaxMessageSize < 0) { | ||
| throw new TronError("node.http.maxMessageSize must be non-negative, got: " | ||
| + PARAMETER.httpMaxMessageSize, PARAMETER_INIT); | ||
| } | ||
| PARAMETER.jsonRpcMaxMessageSize = config.hasPath(ConfigKey.NODE_JSONRPC_MAX_MESSAGE_SIZE) | ||
| ? config.getMemorySize(ConfigKey.NODE_JSONRPC_MAX_MESSAGE_SIZE).toBytes() | ||
| : defaultHttpMaxMessageSize; | ||
| if (PARAMETER.jsonRpcMaxMessageSize < 0) { | ||
| throw new TronError("node.jsonrpc.maxMessageSize must be non-negative, got: " | ||
| + PARAMETER.jsonRpcMaxMessageSize, PARAMETER_INIT); | ||
| } | ||
|
|
||
| PARAMETER.maxHeaderListSize = config.hasPath(ConfigKey.NODE_RPC_MAX_HEADER_LIST_SIZE) | ||
| ? config.getInt(ConfigKey.NODE_RPC_MAX_HEADER_LIST_SIZE) | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -327,10 +327,12 @@ public static Transaction packTransaction(String strTransaction, boolean selfTyp | |||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| @Deprecated | ||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Marking this helper as deprecated does not make the new HTTP limit effective yet.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch, thanks.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good analysis. I agree that long-term, a single enforcement layer is cleaner. Here's a detailed breakdown of the two layers: Are they actually measuring the same thing? I added tests in commit
For TRON's JSON API (UTF-8, mostly ASCII), the two measurements are strongly consistent. The only divergence comes from line-ending normalization via Even in the unlikely case of minor discrepancies, operators can adjust the threshold up or down to achieve their desired limit — the behavior is predictable, not erratic. Why keep both for now? Removing
This follows the standard deprecate-then-remove pattern and keeps each PR reviewable. |
||||||||||||||||||
| public static void checkBodySize(String body) throws Exception { | ||||||||||||||||||
| CommonParameter parameter = Args.getInstance(); | ||||||||||||||||||
| if (body.getBytes().length > parameter.getMaxMessageSize()) { | ||||||||||||||||||
| throw new Exception("body size is too big, the limit is " + parameter.getMaxMessageSize()); | ||||||||||||||||||
| if (body.getBytes().length > parameter.getHttpMaxMessageSize()) { | ||||||||||||||||||
| throw new Exception("body size is too big, the limit is " | ||||||||||||||||||
| + parameter.getHttpMaxMessageSize()); | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -223,6 +223,10 @@ node { | |
| solidityPort = 8091 | ||
| PBFTEnable = true | ||
| PBFTPort = 8092 | ||
|
|
||
| # The maximum request body size for HTTP API, default 4M (4194304 bytes). | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that zero is intentionally allowed, please document its concrete behavior here. “Must be non-negative” is not enough: operators may reasonably read |
||
| # Supports human-readable sizes: 4m, 4MB, 4194304. Must be non-negative. | ||
| # maxMessageSize = 4m | ||
| } | ||
|
|
||
| rpc { | ||
|
|
@@ -248,8 +252,9 @@ node { | |
| # Connection lasting longer than which will be gracefully terminated | ||
| # maxConnectionAgeInMillis = | ||
|
|
||
| # The maximum message size allowed to be received on the server, default 4MB | ||
| # maxMessageSize = | ||
| # The maximum message size allowed to be received on the server, default 4M (4194304 bytes). | ||
| # Supports human-readable sizes: 4m, 4MB, 4194304. Must be non-negative. | ||
| # maxMessageSize = 4m | ||
|
|
||
| # The maximum size of header list allowed to be received, default 8192 | ||
| # maxHeaderListSize = | ||
|
|
@@ -357,6 +362,10 @@ node { | |
| # openHistoryQueryWhenLiteFN = false | ||
|
|
||
| jsonrpc { | ||
| # The maximum request body size for JSON-RPC API, default 4M (4194304 bytes). | ||
| # Supports human-readable sizes: 4m, 4MB, 4194304. Must be non-negative. | ||
| # maxMessageSize = 4m | ||
|
|
||
| # Note: Before release_4.8.1, if you turn on jsonrpc and run it for a while and then turn it off, | ||
| # you will not be able to get the data from eth_getLogs for that period of time. Default: false | ||
| # httpFullNodeEnable = false | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This moves oversized-request handling in front of every servlet, so those requests never reach
RateLimiterServlet.service()/Util.processError(). Today the HTTP APIs consistently setapplication/jsonand serialize failures throughUtil.printErrorMsg(...); after this change an over-limit body gets Jetty's default 413 response instead. That is a client-visible behavior change for existing callers, and the new test only checks status codes so it would not catch the response-format regression.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for flagging the response-format difference. I did a detailed comparison:
Before (checkBodySize rejection):
{"Error":"java.lang.Exception : body size is too big, the limit is 4194304"}After (SizeLimitHandler rejection):
The format is indeed not fully compatible, but I believe this is acceptable:
The existing behavior is itself incorrect — returning
200 OKwith an error JSON body violates HTTP semantics. Clients that only check status codes would incorrectly treat the oversized request as successful. The new413is the proper HTTP response for this scenario.The trigger threshold is very high — default is 4 MB. Normal API requests are far below this. Only abnormal or malicious payloads hit this limit, so the impact surface is negligible for legitimate clients.
413 is a standard HTTP status code — all HTTP client libraries handle it correctly. Clients already need to handle non-JSON infrastructure errors (e.g., Jetty 503, proxy 502/504).
The new layer provides a real security benefit —
SizeLimitHandlerrejects during streaming, before the body is fully buffered into memory, which is the core OOM protection. Falling back to application-layer formatting would defeat this purpose.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oversized requests will now be rejected before they enter the existing servlet/filter pipeline. That means they will no longer go through the current application-side interceptor / rate-limit / metrics path. This may be perfectly fine, but it seems worth documenting or at least acknowledging so everyone is aware of the observability and control-path change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Acknowledged — oversized requests rejected by
SizeLimitHandler(413 before servlet dispatch) will bypassRateLimiterServletand application-level metrics/logging. This is the expected trade-off: rejecting bad traffic early saves resources, but those requests won't appear in application-layer observability.For chunked requests specifically, the broad
catch(Exception)in servlets absorbs theBadMessageException, so those DO go through the servlet pipeline and are visible in the existing error path.If Jetty-level 413 observability becomes important, a future enhancement could add a Jetty
Handleror access log filter to count rejected requests — but that's outside this PR's scope.