[bladehan1]

What does this PR do?

Fix premature rejection of valid unknown-length request bodies in the shared Jetty HTTP service. With a 4 MiB limit, the same 2–3 MiB body can pass with Content-Length but fail when sent chunked.

Install RequestSizeLimitHandler, which extends HandlerWrapper directly and owns the request-limit check. Preserve the Content-Length precheck; for unknown-length bodies, use HttpInput.getContentReceived() through addInterceptor(), preserving existing input interceptors and returning each Content unchanged. Delegate through super.handle(), retaining its null-handler check. Configured limits, the size > limit boundary and existing public error behavior remain unchanged.

Test clients and sockets use 15-second connection/read timeouts, including the Apache client's connection-pool acquisition timeout.

Why are these changes required?

Root cause

In Jetty 9.4.58.v20250814, SizeLimitHandler.LimitInterceptor.readFrom() accumulates content.remaining() on every interception. The interceptor contract allows the same Content to pass through repeatedly. Because remaining() is a snapshot of unread bytes, this counts bytes more than once and makes rejection depend on buffering, chunk boundaries and application read patterns. The known Content-Length precheck is unaffected.

Alternatives and selected design

Option	Assessment
Increase the limit or compensate for client chunk size	Moves the failure threshold without fixing repeated counting; the error factor is not constant.
Rely only on Util.checkBodySize()	Checks after full-body loading and character conversion; cannot provide equivalent streaming protection.
Wrap ServletInputStream / Reader	Requires covering both APIs and asynchronous reads; consumption-based counting also changes the metric from received bytes.
Copy the full Jetty handler	Adds maintenance of unrelated upstream implementation details.
Upgrade to Jetty 12	A broader JDK/Servlet/API migration rather than a bounded fix.
Use Jetty's received-byte counter	Selected: Jetty increments it for newly arrived content and resets it per request. It avoids duplicate counting without another full-body buffer or dependency upgrade.

Counter timing and request isolation

Jetty updates its received-byte counter when new content arrives, before invoking the input interceptor. Updates and reads use the same input monitor; re-intercepting existing content does not increment the counter. Chunked accounting covers payload bytes, excluding framing and trailers.

Concurrent HTTP/1 connections have separate HttpInput instances. Keep-alive connections may reuse an input, but Jetty resets its counters and clears the old interceptor for the next request. The shared handler stores only the immutable limit; each interceptor captures its request's input.

This PR has been tested by:

All 28 focused Jetty/JSON-RPC tests passed. One-byte regression tests distinguish the received-byte implementation from both a consumed-byte counter and the original Jetty handler. Additional coverage checks concurrent-request isolation, keep-alive counter reset, null-handler delegation and test-helper read timeouts.

clean build -x test, all three required module Checkstyle tasks and the Math check over tracked Java sources passed. These are scoped validation results; no completed full-repository test-suite pass or sustained overload/recovery result is claimed.

Component test matrix

The matrix includes committed regressions and supplemental local probes. Supplemental probes are component-level evidence, not a claim that every production deployment was exercised. Detailed validation evidence.

Area / scenarios	Result and evidence boundary
Known-length and chunked size boundaries	Empty, below limit, exact limit and over-limit cases covered. SizeLimitHandlerTest: 23/23 passed, including existing regressions.
Core premature-rejection regression	Same 2/3 MiB bodies under a 4 MiB limit compared across CL/chunked. Independent candidate matrix: 143/143 size assertions matched; original handler prematurely rejected 41 corresponding cases.
Chunking and application reads	Multiple chunk sizes, Reader, small/large InputStream reads, UTF-8 and a slow-send sample covered; accepted probe bodies checked against byte counts and SHA-256.
Request lifecycle and component composition	Continuous requests, same-connection successful requests, two-service limit isolation and preservation of an existing input interceptor covered. Concurrent overload/recovery is not established.
Zero and disabled handler limits	Zero permits empty bodies only; non-empty CL fails before dispatch and chunked fails during reading. Direct handler construction with -1 disables this check; it does not imply node configuration accepts negative values.
CL differs from uploaded bytes	Over-limit declarations rejected without servlet dispatch whether no/less/more content was sent. Short bodies produced idle timeout or Early EOF. Extra bytes were parsed as a subsequent request: invalid suffix gave 200 then 400; valid second request gave two 200 responses.
TE/CL conflicts	Original and replacement probes returned parser-level 400 without servlet dispatch, including when CL exceeded the limit. The zero-limit/CL/TE edge probe set passed 26/26 assertions across both implementations.
Real HTTP query and JSON-RPC paths	3 MiB CL/chunked requests passed through the real GetBlockByNumServlet with a mocked Wallet. JsonrpcServiceTest.testJsonRpcSizeLimitIntegration passed through JsonRpcServlet/jsonrpc4j.
Protocol and error-path compatibility	HTTP/1.0 CL boundaries, HTTP/1.1 CL/chunked, Jetty's HTTP/1.0 chunked compatibility sample, malformed chunk reads, parser 400 and URI 414 covered. Compatibility acceptance is not a standards guarantee.

Follow up

Uniform HTTP 413 handling should be addressed separately. Some ordinary HTTP servlets catch body-read exceptions and return HTTP 200 with an error body; this PR preserves that behavior.

Extra details

Test scope and boundaries

Area	Current behavior and reason for the test scope
Response-size boundaries, output streaming and committed-response errors	The request-only handler has no response-limit parameter or output interceptor. Output enforcement requires a separate implementation and output-specific tests.
Asynchronous ReadListener consumption	No setReadListener/startAsync registration was found in the production sources checked. Current regression tests exercise the synchronous Reader/InputStream consumers used here; an asynchronous consumer must bring its own integration tests when introduced.
Small reads versus abandoning a body	Multiple small InputStream reads are covered, as are short uploads, idle timeout and Early EOF. Application-side abandonment/draining of a body was not separately tested. The fix reads Jetty's received-byte counter without consuming or buffering Content; it does not introduce a drain/abort policy or promise to inspect an upload that the application never reads. A complete drain/abort lifecycle suite is therefore outside this counter-fix acceptance scope.
Starting every production service/port	The production subclasses use the same HttpService.initContextHandler() installation path; no subclass override of that method was found. Existing per-service limit selection is unchanged. Shared-handler tests, two-service limit isolation and real HTTP/JSON-RPC integration cover the changed mechanism. Repeating identical byte-count tests on every port is not required to validate that shared implementation; this does not claim every service deployment was exercised.
Complete configuration-loading combinations	This PR changes neither configuration parsing/defaults nor the selection of a service's limit. Tests cover the handler's positive/zero/disabled values and independent service limits. Configuration migration and a full loader cross-product are not new behaviors introduced by this fix.
gzip / transformed input	No request GzipHandler/GzipFilter is installed in the checked Jetty service chain. There is no decompression path in this change to test. Introducing decompression would require defining and testing both encoded and decoded size limits.
Sustained load, resource exhaustion and recovery	Those suites establish broader capacity and resilience claims, not whether repeated interceptor calls count the same bytes twice. This change reuses Jetty's existing per-request counter and adds no full-body buffer, executor or store. Boundary, repeated-request and interceptor-composition tests validate the changed mechanism; long-running pressure/recovery testing is not an acceptance gate for this bounded fix, and no general OOM/DoS guarantee is claimed.
Jetty HTTP/2	The current Jetty HTTP API does not install an HTTP/2 connector. Its body/frame/stream tests are not applicable and are not pending work. gRPC's HTTP/2 transport does not use this handler.

Component impact matrix

Component / boundary	Impact and validation scope
RequestSizeLimitHandler / HttpInput	Changes unknown-length request accounting to received bytes. CL prechecks and the size > limit boundary remain. Direct component and socket coverage listed above.
Shared HttpService	All subclasses using this base handler inherit the replacement: FullNode/Solidity/PBFT HTTP, JSON-RPC HTTP and Admin HTTP. Representative HTTP/JSON-RPC and simulated-service tests passed; this is not a claim that every production port was individually started.
Filters, servlets and existing input interceptors	Downstream dispatch is preserved; interceptor-chain and representative servlet tests passed. Complete authentication/rate-limit/filter combinations were not individually exercised.
Parser and connection framing	CL framing, malformed-input handling and TE/CL rejection remain Jetty-owned. Timeout/EOF and valid/invalid subsequent-request probes confirm representative behavior.
Configuration and application fallback	No keys/defaults are changed. HTTP and JSON-RPC keep their existing per-service limit selection; Util.checkBodySize() remains a separate post-read check. Complete configuration-loading validation is not claimed.
Error handling	CL rejection remains pre-dispatch 413. Oversized chunked requests in the current JsonRpcServlet propagate as HTTP 413: the servlet reads the network body before passing the cached body to jsonrpc4j. Some ordinary HTTP servlets catch streaming exceptions and can return HTTP 200 with an error/empty body; this PR does not standardize those responses.
Response limiting	Not implemented by this request-only handler; it has no response-limit parameter or output interceptor.
gRPC, IPC, HTTP/2 and consensus	No change to their implementations or limits; gRPC/IPC do not traverse this Jetty request handler. Jetty HTTP/2 is not enabled; no consensus/state-transition logic changes.

[waynercheung]

Verified the root cause against Jetty 9.4.58: HttpInput can pass the same Content to the interceptor repeatedly (on arrival and on subsequent reads), and LimitInterceptor adds remaining() each time, so unread bytes are counted more than once. getContentReceived() is incremented once per addContent and reset per request, which avoids this.

Local run (JDK 17): SizeLimitHandlerTest 23/23 and JsonrpcServiceTest#testJsonRpcSizeLimitIntegration pass. Mutants for the original handler, super.handle(), setInterceptor, the Content-Length precheck and the streaming check are all killed. A one-byte-read probe reproduces the early rejection with the original handler and confirms the fix.

No runtime correctness issue found. Please address the Javadoc note and the description fix below before merge.

[SHOULD] Please correct the error-handling row in the description: JsonRpcServlet reads the network body before invoking jsonrpc4j, and oversized chunked requests propagate as HTTP 413, as asserted by testJsonRpcSizeLimitIntegration. HTTP 200 with an error body can still occur in HTTP servlets that swallow the exception; it does not describe the current JSON-RPC body-reading path.

[bladehan1]

Addressed the review feedback in 6b9526938e:

• Added one-byte-read regressions that distinguish the received counter from both the consumed-counter variant and Jetty's original handler.
• Added concurrent request-isolation and keep-alive counter-reset coverage.
• Renamed the handler to RequestSizeLimitHandler and documented the request-only scope and intentional parent-handler bypass.
• Corrected the error-handling row: the current JsonRpcServlet network-body path propagates oversized chunked requests as HTTP 413, before jsonrpc4j is invoked. The existing real JSON-RPC integration test verifies this behavior.

Validation on the code merged with upstream release_v4.8.3 at ea34f6f0c8: 30 selected tests passed (28 request-limit/JSON-RPC regressions plus the two previously failing Manager/plugins tests); all three required module Checkstyle tasks passed. The received implementation passes both one-byte mutation checks, while the consumed-counter and original-handler variants fail their respective regression assertions.

A pre-merge clean build passed. The pre-merge full test run was interrupted to merge upstream and run the selected tests directly; no completed full-suite pass is claimed for the merged head.

@0xbigapple @waynercheung, could you please review the updated commit? Both inline threads have detailed replies.

[0xbigapple]

LGTM

[SeriousCoding789]

[NIT] Some of the new tests can block instead of failing if the server stops responding. postChunked opens an HttpURLConnection without setConnectTimeout / setReadTimeout (both default to unlimited). postForBody goes through the shared client from HttpClients.createDefault() (line 181), which has no socket timeout. sendRaw (line 645), used by the new HTTP/1.0 and malformed-chunk tests, opens a plain Socket without setSoTimeout. None of the new test methods have @Test(timeout = ...). A method-level timeout, or connect/read timeouts on these clients, would turn a hang into a test failure. RequestSizeLimitHandlerTest already avoids this with setSoTimeout(15000) on its sockets.

[bladehan1]

@SeriousCoding789 Addressed in 9be048ad1b.

Added 15-second connect/read timeouts to the HttpURLConnection and raw Socket helpers. The shared Apache HTTP client now also has 15-second connect, connection-pool acquisition and socket timeouts.

The 28 focused Jetty/JSON-RPC tests passed. Supplemental local fault probes invoked the actual three test helpers against loopback servers that accepted connections but never responded; all three raised SocketTimeoutException after approximately 15 seconds. These probes validate read timeouts; connection timeout configuration was checked separately.

[SeriousCoding789]

[NIT] A few assertions pin Jetty or test-servlet behavior that is independent of the counter fix:

• testHttp10ChunkedRequestCompatibility (line 521) checks that Jetty accepts Transfer-Encoding: chunked on an HTTP/1.0 request, which HTTP/1.0 does not define.
• testMalformedChunkFailsDuringBodyRead (line 530) checks Jetty's exception text (Early EOF) and the 200 status produced by the test servlet's catch-all.
• RequestSizeLimitHandlerTest line 207 (assertSame(inputs.get("/reuse0"), inputs.get("/reuse" + i))) checks that keep-alive requests reuse the same HttpInput instance.

If a Jetty upgrade changes any of these, the tests would fail even though the size limit still works. Would it make sense to drop the first two, or label them as documenting current behavior? For the keep-alive test, Assume.assumeTrue(...) on the instance reuse would keep it as a precondition of the scenario rather than a requirement, while the per-request received assertions still verify the counter reset.

[bladehan1]

@SeriousCoding789 Keeping these assertions unchanged in this bounded follow-up. The HTTP/1.0 chunked and malformed-input tests record the pinned Jetty version's compatibility/error behavior, rather than a protocol guarantee. Removing or restructuring them is not required for the inheritance/timeout changes.

The same-HttpInput assertion confirms that the keep-alive scenario actually exercises counter reset on a reused input. Replacing it with an assumption could skip that regression coverage. We can reassess these expectations when upgrading Jetty.

[SeriousCoding789]

[DISCUSS] Pre-existing and outside the scope of this PR, mentioning it only because the chunked path is touched here: the status code for an oversized body still depends on framing and endpoint. Oversized Content-Length requests get 413 before dispatch, and JSON-RPC returns 413 for oversized chunked bodies, but plain HTTP servlets catch the BadMessageException raised during the body read and answer 200 with an error JSON (the behavior testChunkedBodyExceedsLimit documents). A client that branches on the status code sees a success status for a rejected request. This was scoped out in #6658, so a separate issue might be a better place for it than this PR.

[bladehan1]

@SeriousCoding789 Regarding the HTTP 413 discussion: agreed that uniform status handling should be addressed separately; it is outside this PR. Extending HandlerWrapper directly does not change exception propagation: ordinary HTTP servlets can still catch the size exception during body reading and write an error body with HTTP 200. Unifying that behavior requires a separate error-mapping/response policy, including responses that are already committed. This follow-up changes only the inheritance structure and test timeouts, preserving current public error behavior.

Validation on 9be048ad1b:

• Candidate size matrix: 143/143; original Jetty control: 102/143, with 41 premature chunked rejections.
• Zero-limit, CL mismatch and TE/CL edge probes: 26/26 across control/candidate.
• Focused Jetty/JSON-RPC tests: 28/28, zero skipped/failures/errors.
• One-byte mutation checks: current implementation passes; consumed-counter and original-handler variants fail the expected assertions.
• Supplemental local probes: absent downstream handler and delegation at negative/zero/positive limits pass; the three actual test helpers time out against nonresponding loopback servers after approximately 15 seconds.
• clean build -x test, the three CI Checkstyle tasks, and the Math check over tracked Java sources pass.

These are scoped local checks, not production acceptance or a complete repository test-suite pass. Full-repository test attempts were stopped; no full-suite pass is claimed. Uniform HTTP 413 handling, broader configuration/deployment combinations and sustained resource-pressure testing are outside this follow-up. These supplemental probes are local evidence, not additional committed tests.

[waynercheung]

Re-reviewed 9be048ad1b. Extending HandlerWrapper preserves the request-limit condition, 413 status and exception message, while making super.handle() safe and retaining its null-handler check. The test timeouts are appropriate.

In my local run on JDK 17, all 28 focused tests passed, and targeted mutations covering counter selection, interceptor preservation, size boundaries and rejection behavior were each caught.

Agree that the existing HTTP 200 versus 413 inconsistency in ordinary HTTP servlets should be tracked separately.

[SHOULD] Please sync the description with 9be048ad1b. The handler now extends HandlerWrapper and delegates safely through super.handle(). Statements about super(requestLimit, -1), bypassing SizeLimitHandler.handle(), direct getHandler().handle() delegation, and responseLimit being fixed to -1 describe the previous implementation; the "future responseLimit support" section also assumes it. Please update those sections while keeping the root-cause explanation and the request-only scope.

Previous code review concerns are addressed. The remaining item is to align the PR description with the current implementation; I will approve once it is updated.

[bladehan1]

@waynercheung Thanks for the re-review. Updated the PR description to match the current implementation: RequestSizeLimitHandler extends HandlerWrapper and delegates through super.handle(), preserving the null-handler check. Removed the obsolete parent-bypass and responseLimit = -1 explanations, and clarified that the handler has no response-limit parameter or output interceptor.

The root-cause explanation, request-only scope and validation/impact matrices are retained; historical implementation notes and repeated explanations have been removed. Uniform HTTP 413 handling remains a separate follow-up. No code changes were needed.

[waynercheung]

Thanks for syncing the description. It now matches 9be048ad1b: the handler extends HandlerWrapper, delegates through super.handle(), and the response-limit rows describe the absence of an output limit rather than a fixed -1. The JSON-RPC 413 and request-only scope notes are kept. Code is unchanged since my last review, so my earlier local results still apply.

My remaining review concern is addressed. LGTM.