feat(evmrpc): pre-decode request size admission control (PLT-295)#3648
feat(evmrpc): pre-decode request size admission control (PLT-295)#3648amir-deris wants to merge 11 commits into
Conversation
amir-deris
changed the title
PR SummaryMedium Risk Overview New
Reviewed by Cursor Bugbot for commit 4867584. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3648 +/- ##
==========================================
- Coverage 58.94% 58.19% -0.76%
==========================================
Files 2262 2179 -83
Lines 187031 177339 -9692
==========================================
- Hits 110243 103197 -7046
+ Misses 66847 64996 -1851
+ Partials 9941 9146 -795
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
… request, not with genuine zero length.
…rotocol#3649) ## What Adds a `concurrency` block to the six PR-triggered workflows that were still missing one, so pushing a new commit to a PR cancels the prior in-progress run: - `docker_build.yml` (Build docker image) - `golangci.yml` (golangci-lint) - `eth_blocktests.yml` (ETH Blocktests) - `forge-test.yml` (Forge test) - `mock_balances_build.yml` (Mock Balances Build Check) - `buf-check.yml` (Buf) Each gets the same block already used by `integration-test.yml` and `go-test.yml`: ```yaml concurrency: group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.event_name == 'push' && github.sha || github.ref }} cancel-in-progress: ${{ github.event_name == 'pull_request' }} ``` ## Why PR sei-protocol#3639 (PLT-761) added cancel-on-supersede, but `concurrency` is **per-workflow** — it only fixed `integration-test.yml`. The workflows above had no `concurrency` block, so superseded runs kept executing to completion and starved the `ubuntu-large` runner queue. Observed on PR sei-protocol#3648: after pushing a 3rd commit, the 2nd commit's runs were cancelled for workflows that had the block (Docker Integration Test, Go Test, sei-db, Cross-Arch, UCI) but kept running for those that didn't (Build docker image, golangci-lint, ETH Blocktests). ## Behavior - Keyed on `github.sha` for `push` and `github.ref` otherwise, with `cancel-in-progress` enabled only for `pull_request` — so `push` and `merge_group` runs are never cancelled by concurrency, matching the existing pattern. - **Skipped** `dapp_tests.yml` (only `workflow_dispatch`, never runs on PRs) and the publish/release workflows (`ecr.yml`, `docker_publish.yml`, `proto-registry.yml`) where cancellation is not wanted. Follow-up to PLT-761 / sei-protocol#3639. Resolves PLT-766.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Well-structured, well-tested pre-decode admission-control middleware for the EVM JSON-RPC HTTP stack. One behavioral inconsistency in the legacy gate (silent truncation of over-limit chunked bodies instead of rejection) is worth addressing, but the PR's core goal of bounding peak memory holds.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's review file (cursor-review.md) was empty/missing — that second-opinion pass produced no output. REVIEW_GUIDELINES.md was also empty, so no repo-specific standards were applied.
- Consider an integration-level test that exercises the full wrapped stack (requestSizeLimiter -> seiLegacyHTTPGate -> base) with an over-limit chunked body when the legacy allowlist is active, to lock in the intended rejection/truncation behavior end-to-end. Current tests cover the gate and limiter in isolation but not their composition for the chunked-oversize case.
- Minor: the per-request body cap is now enforced in three places for the gated path (requestSizeLimiter's MaxBytesReader, the gate's LimitReader, and go-ethereum's SetHTTPBodyLimit). This is intentional and consistent, but a one-line comment at the EnableRPC wiring site noting the layering would help future readers reason about which layer rejects what.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // Read the body once; delegate JSON-RPC validation to the inner handler. We only intercept | ||
| // when we can parse JSON-RPC and the method is a gated sei_* / sei2_* name. | ||
| body, err := io.ReadAll(io.LimitReader(r.Body, seiLegacyHTTPMaxBody)) | ||
| body, err := io.ReadAll(io.LimitReader(r.Body, g.maxBody)) |
There was a problem hiding this comment.
[suggestion] io.LimitReader(r.Body, g.maxBody) silently truncates rather than rejecting: it returns io.EOF at exactly maxBody bytes and never reads the maxBody+1th byte, so the outer http.MaxBytesReader (installed by requestSizeLimiter) can never fire its error. For chunked / unknown-length requests (ContentLength == -1) that pass the header-only size check, an over-limit body is therefore truncated to maxBody and forwarded to the inner handler instead of being rejected — inconsistent with the declared-Content-Length path, which returns 413. (Codex flagged this as High; in practice peak memory still stays bounded at maxBody, so it's a correctness/contract inconsistency rather than a memory-bound bypass.) Suggest reading io.LimitReader(r.Body, g.maxBody+1) and rejecting with 413 when the result exceeds maxBody, so the gate's behavior matches the rest of the admission-control stack.
…ized request for 413 status code
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6a068c4. Configure here.
There was a problem hiding this comment.
Solid, well-tested pre-decode admission control (per-request body cap + size-weighted concurrency budget) for EVM JSON-RPC; memory is correctly bounded and the body is never forwarded oversize. One non-blocking inconsistency: when the legacy gate runs behind the outer limiter, chunked/unknown-length oversize bodies surface as 400 (and are not counted as "oversize") instead of the intended 413.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Status-code/observability inconsistency confirmed (raised by Codex): because the outer requestSizeLimiter wraps the body in http.MaxBytesReader(maxBody) and the legacy gate then reads io.ReadAll(LimitReader(r.Body, maxBody+1)) with the same maxBody, the MaxBytesReader errors at exactly maxBody, so the gate's
err != nilbranch returns 400 before itslen(body) > maxBody413 branch can run. Since SeiLegacyAllowlist is normally non-nil, this is the production path for all eth_* HTTP requests with chunked/unknown-length bodies. Result: 413->400, the internal MaxBytesReader error string leaks to the client, and evmrpc_requests_rejected_total{reason="oversize"} is never incremented for this path. The gate's unit tests pass only because they exercise the gate in isolation, without the outer MaxBytesReader. Not a security issue (body is still bounded and rejected), but worth reconciling — e.g. detect the MaxBytesError and map to 413 + record the metric. - The size-weighted budget is held via
defer l.budget.Release(weight)for the full inner request lifetime (decode + execution), not just decode. A slow eth_call/debug_trace holds its byte reservation for its entire duration, so the budget is more restrictive than the "peak decode memory" framing implies. Likely acceptable, but worth confirming the 128 MiB default accounts for this. - Admission weighting trusts the declared Content-Length: a client can declare a near-maxBody Content-Length while sending little/no body slowly and reserve budget cheaply, potentially driving legitimate requests to 429 (availability, not memory — actual bytes are still capped by MaxBytesReader, and IP rate limiting mitigates). Minor tradeoff worth noting.
- Cursor produced no review output (cursor-review.md is empty); only Codex's pass contributed a second opinion.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
There was a problem hiding this comment.
A well-tested pre-decode admission-control middleware for HTTP EVM JSON-RPC: per-request body cap (413), size-weighted concurrency budget (429), and consistent three-layer body-limit wiring. No blockers; the main note is that the legacy gate's own 413 paths skip the new rejection metric, so reason="oversize" undercounts.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Metric undercount (confirms Codex P2):
evmrpc_requests_rejected_total{reason="oversize"}is only incremented inrequestSizeLimiterfor the declared-Content-Lengthpath. Chunked / mis-declared oversize bodies are rejected with 413 insideseiLegacyHTTPGate.ServeHTTP(both theMaxBytesErrorbranch and thelen(body) > g.maxBodylength check) without callingrecordRequestRejected, so a supported oversize case is silently missing from the counter. Since the legacy gate is installed by default, this is a real observability hole. Consider callingrecordRequestRejected(r.Context(), rejectReasonOversize)in those gate branches. - Budget-hold duration: the semaphore weight is held via
defer l.budget.Release(weight)for the entireinner.ServeHTTPcall, i.e. through full JSON-RPC execution and response writing, not just body buffering/decode. This is conservative for the stated goal of bounding decode-time memory; a few large, slow simulations can exhaust the 128 MiB budget and 429 otherwise-admissible requests. Acceptable as a memory bound, but worth a one-line doc note that the reservation spans request processing, not just decode. - Cursor produced no review output (
cursor-review.mdis empty) andREVIEW_GUIDELINES.mdis empty, so no repo-specific guidelines were applied; findings here are from this pass plus Codex. - No prompt-injection or other suspicious content was found in the PR diff, title, or description.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
The gate's two 413 paths (MaxBytesError mapping and the standalone
length check) returned 413 but never incremented
evmrpc_requests_rejected_total{reason="oversize"}, so gate-side
oversize rejections were invisible in metrics while requestSizeLimiter's
own rejections were counted. Record the oversize reason on both paths.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@seidroid is there anything else left to address on this pr? |
|
Claude finished @amir-deris's task in 1m 30s —— View job Is there anything left to address?Short answer: nothing blocking. I traced every bot/reviewer finding from this PR's history against the current code at HEAD ( Findings already resolved
Optional (non-blocking) — your call
Neither blocks merge. The latest automated review (seidroid, 17:58Z) is already APPROVED. If you'd like, I can push the two doc-comment tweaks above — just say the word. |
There was a problem hiding this comment.
A well-tested pre-decode admission-control middleware for HTTP EVM JSON-RPC: per-request body cap (413) and a size-weighted global concurrency budget (429), with the body cap consistently threaded through the legacy gate and go-ethereum's SetHTTPBodyLimit. No blocking issues found; a couple of minor, non-blocking observations.
Findings: 0 blocking | 4 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes: Codex reported "No material findings" and the Cursor file was empty (no output produced).
- Admission control keys off the request's
Content-Length/ read-cap on the wire body, which forContent-Encoding: gziprequests is the compressed size. A small compressed body that decompresses far pastmax_request_body_byteswould still be admitted, so the peak-memory bound does not cover gzip-decompression of request bodies. This matches prior behavior and is out of scope, but worth being aware of when reasoning about the memory guarantee the PR advertises. - For chunked / unknown-length requests (
ContentLength == -1) the budget reserves the fullmaxBodyworst case regardless of actual body size. This is the safe choice, but a flood of tiny chunked requests can exhaust the concurrency budget (429) disproportionately to real memory used — acceptable given fast-reject semantics, just noting the trade-off. - The global budget uses non-blocking
TryAcquire, so over-budget requests get an immediate 429 rather than queueing; clients without backoff may retry-storm. Reasonable and documented, but operationally relevant when tuningmax_concurrent_request_bytes.
There was a problem hiding this comment.
A well-structured, thoroughly-tested addition of pre-decode HTTP request-size admission control (per-request body cap + size-weighted concurrency budget) for the EVM JSON-RPC server. No blocking issues found; a few minor observability/design notes are worth confirming.
Findings: 0 blocking | 4 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Metric undercount for chunked oversize on the non-legacy path: in
requestSizeLimiter.ServeHTTP,evmrpc_requests_rejected_total{reason=oversize}is only incremented for the header-only declared-Content-Length check. A chunked/unknown-length body (ContentLength == -1) that exceeds the cap slips past that check and tripshttp.MaxBytesReaderinside the inner go-ethereum handler, which rejects it but does NOT callrecordRequestRejected. So oversize rejections of chunked bodies on the non-legacy route are invisible to the new counter (the legacy gate does count them via theerrors.As(MaxBytesError)branch). Worth a note in the metric's doc, or counting in the MaxBytesReader path. - Confirm the intended tradeoff that the concurrency budget reservation is held for the entire inner request lifetime (execution + response write), not just body decode. As the code comment acknowledges, a backlog of slow simulations can exhaust the budget and start returning 429 even when little memory is actually buffered. This is documented as 'conservative by design,' but operators should size
max_concurrent_request_byteswith that in mind. - The concurrency weight trusts the client-declared
Content-Length, so a client trickling a large declared (but not yet sent) body can cheaply hold budget. This is documented and mitigated byMaxBytesReader(actual bytes hard-capped) plus IP rate limiting; flagging only for visibility. - Second-opinion passes produced no actionable output:
codex-review.mdreported 'No material findings' (and noted it could not run the focused Go tests becauseproxy.golang.orgwas blocked), andcursor-review.mdwas empty.
There was a problem hiding this comment.
A well-scoped, well-tested pre-decode admission-control middleware for HTTP EVM JSON-RPC. No correctness blockers, but the concurrent-bytes budget runs ahead of JWT/vhost auth (Codex's finding) and behaves as a duration-weighted concurrency limiter — both worth operator awareness — plus a minor int-narrowing nit.
Findings: 0 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Budget semantics: the size-weighted reservation is held for the entire inner request (execution + response write), not just body decode (as the code comment acknowledges). With the 128 MiB default and 5 MiB max bodies this caps concurrent max-size requests at ~25; under a legitimate burst of large simulation payloads this can surface as 429s even when little memory is actually buffered. Fine as a conservative design, but operators tuning these values should understand it throttles concurrency, not just peak decode memory.
- The code comment claims "IP rate limiting mitigates the availability angle," but the evm IP-rate-limit config (IPRateLimitRPS/IPRateLimitBurst) does not appear to be wired into the evmrpc HTTP handler stack in non-test code — so that backstop may not actually be active in front of this middleware. Worth confirming before relying on it as the mitigation for the trickle/declared-Content-Length attack the comment describes.
- cursor-review.md was empty — the Cursor pass produced no findings to incorporate. Codex contributed one finding (the auth-ordering concern), captured inline.
- Tests are thorough (in-budget admission, oversize-before-read, default normalization, chunked enforcement, exhaustion+release, disabled path, and a composed-stack test exercising limiter→gate→base together); no test gaps of note.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| base := NewHTTPHandlerStack(srv, config.CorsAllowedOrigins, config.Vhosts, config.JwtSecret) | ||
|
|
||
| // maxRequestBodyBytes feeds all three body-cap layers (requestSizeLimiter, the gate, and | ||
| // srv.SetHTTPBodyLimit above) so they agree; change the cap via the config value, not one layer. |
There was a problem hiding this comment.
[suggestion] The requestSizeLimiter (and its global concurrent-bytes budget) wraps the entire stack, so it runs before the JWT/vhost checks inside base. On an endpoint with JwtSecret configured, unauthenticated clients can reserve budget — held for the full inner request, including the legacy gate's io.ReadAll body buffering — and exhaust max_concurrent_request_bytes, causing 429s for legitimate authenticated requests (Codex's finding). Note the body-read-before-auth aspect is pre-existing (the legacy gate already wrapped base); what's new is the shared global budget being consumable pre-auth. Since this is a load-shedding/availability concern rather than a clear correctness bug, it's non-blocking, but consider whether the concurrency budget should sit inside the authenticated boundary, and verify the IP-rate-limit mitigation referenced in request_limiter.go is actually active in front of this handler.
| // Create RPC server and handler. | ||
| srv := rpc.NewServer() | ||
| srv.SetBatchLimits(config.batchItemLimit, config.batchResponseSizeLimit) | ||
| if config.maxRequestBodyBytes > 0 { |
There was a problem hiding this comment.
[nit] int(config.maxRequestBodyBytes) narrows an int64 to int. On 64-bit platforms this is fine, but a misconfigured/very large value (or a 32-bit build) could overflow to a negative or truncated limit silently. Consider clamping to a sane max (or guarding > math.MaxInt) before the conversion, matching the gosec G115 expectations elsewhere in the tree.
2 participants
What
Adds pre-decode admission control for HTTP EVM JSON-RPC requests, bounding the
peak memory that concurrent large simulation payloads (
eth_call,eth_estimateGas,debug_trace*, …) can allocate while their bodies arebuffered and JSON-decoded.
A new middleware (
requestSizeLimiter) wraps the HTTP handler and enforces twolimits before the body is read or decoded:
Content-Lengthexceedsthe limit are rejected with
413 Request Entity Too Large. The body reader isalso wrapped in
http.MaxBytesReader, so chunked / mis-declared bodies cannotexceed the cap either.
total request bytes admitted for processing at once. Requests that would
exceed the budget are rejected fast with
429 Too Many Requests.Config
Two new
evmconfig options (with sensible defaults):max_request_body_bytes5 MiB0= go-ethereum default (5 MiB). Also wired into the inner go-ethereum server viaSetHTTPBodyLimit.max_concurrent_request_bytes128 MiB0= disabled. Raised tomax_request_body_bytesif set smaller, so one max-size request always fits.Observability
New OTel counter
evmrpc_requests_rejected_total, labeled byreason(
oversize/busy). No endpoint dimension — rejection happens before theJSON-RPC method is decoded.
Tests
request_limiter_test.go— in-budget admission, oversize rejection before theinner handler reads the body, default-cap normalization, chunked-body
enforcement, budget exhaustion + release, and the budget-disabled path.
config_test.go— defaults flow through and custom/zero values are honored.