PLT-819: Wire fail-closed rate limiter + MethodParser into EVM JSON-RPC HTTP (:8545) - #3779
PLT-819: Wire fail-closed rate limiter + MethodParser into EVM JSON-RPC HTTP (:8545)#3779amir-deris wants to merge 17 commits into
Conversation
PR SummaryMedium Risk Overview Config: adds
Reviewed by Cursor Bugbot for commit 1d004a3. Bugbot is set up for automated code reviews on this repo. Configure here. |
amir-deris
changed the title
|
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}
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR cleanly wires the existing per-IP rate limiter and JSON-RPC method parser into the EVM HTTP admission stack with thorough tests, but the rejection metric records the attacker-controlled JSON-RPC method as an unbounded label, which is a metric-cardinality DoS vector that should be bounded before merge. The probe-limit passthrough is a documented, accepted Phase-1 bypass worth noting.
Findings: 1 blocking | 4 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- cursor-review.md is empty (no Cursor output produced); Codex's two findings are both incorporated below. No prompt-injection content was found in the diff or PR description.
- HTTPServer.rateLimitRegistry (evmrpc/rpcstack.go:107) is written in both server.go and EnableRPC but never read anywhere — dead state. Either remove the field or document why it's retained.
- Parse-error rejections (rate_limit_middleware.go: checkErr != nil path) return HTTP 400 but do not call recordRequestRejected, so fail-closed rejections are invisible in the reject-reason metric while rate-limit rejections are counted. Consider recording a reason there too for observability parity.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| 1, | ||
| metric.WithAttributes( | ||
| attribute.String("plane", plane), | ||
| attribute.String("method", method), |
There was a problem hiding this comment.
[blocker] method is attacker-controlled: the JSON-RPC method name is parsed straight from the request body with no allowlist/validation, and it's now recorded as an OpenTelemetry attribute on every rejection. Once an IP's bucket is exhausted (a couple of requests), an attacker can flood requests each carrying a unique random method string; each rejection creates a new rpc_rate_limit_rejected_total{plane,method} time series, causing unbounded metric-series growth that can exhaust node memory or overwhelm the metrics backend. Bound the cardinality before recording — e.g. only emit the label for a known/registered method set and collapse everything else to a constant like "other" (or drop the method label entirely). This is the more severe of Codex's two findings.
| methods, _, parseErr := g.parser.Parse(body) | ||
| switch { | ||
| case errors.Is(parseErr, ratelimiter.ErrProbeLimit): | ||
| return true, "", true, nil |
There was a problem hiding this comment.
[suggestion] Probe-limit passthrough bypass: when the method isn't found within rate_limit_probe_bytes (default 1 MiB) the request is admitted with no token charged. Because JSON object key order is attacker-controlled, a client can pad params ahead of method to push the method name past the 1 MiB probe window while staying under the 5 MiB body limit, defeating rate limiting for large-body request floods. The PR documents this as an accepted Phase-1 risk, so not a merge blocker, but worth an explicit follow-up: a body that exhausts the probe budget without yielding a method could reasonably be charged a token (or rejected) rather than passed through free. Matches Codex's first finding.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3779 +/- ##
==========================================
- Coverage 61.28% 59.70% -1.58%
==========================================
Files 2351 2254 -97
Lines 197283 186302 -10981
==========================================
- Hits 120907 111236 -9671
+ Misses 65530 65169 -361
+ Partials 10846 9897 -949
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the reported findings, I also checked whether a batch JSON-RPC request that is ultimately rejected still burns rate-limit tokens for the methods that were checked before the exhausting one (evmrpc/rate_limit.go's Check loop) — this matches requestSizeLimiter's existing atomic-reject behavior and isn't a new issue introduced by this PR, so I ruled it out as a real bug.
Extended reasoning...
Given the reported cardinality-explosion bug on the rejection metric, this PR needs human review before merge regardless; this note is purely to record that the batch-token-burn candidate was independently examined and ruled out so it isn't re-explored in a future pass.
There was a problem hiding this comment.
Previously flagged issues (unbounded method-label cardinality, rate_limit_probe_bytes overflow, and the rate-limiter probe running ahead of the size-limiter's oversize check) are fixed in the latest commits (717a425, f6c1bb5), with new regression tests covering each. Beyond the inline nit found this run, I also checked and ruled out three candidate issues: the ip_rate_limit_rps=0 bypass claim is a doc-wording gap, not a functional one (the probe still runs, no token is charged); parse errors are rejected fail-closed, not bypassed; and batch requests charge tokens per-method before the whole-batch reject, so it's not a partial-debit bug.
Extended reasoning...
This PR wires per-IP token-bucket rate limiting into the EVM JSON-RPC HTTP admission path (:8545) — a security-relevant admission-control change. It went through multiple review rounds (Cursor, seidroid, and my own prior pass) that surfaced real issues: unbounded OTel label cardinality from attacker-controlled method strings, an int64 overflow in the probe-byte clamp that could 400 every request, and the rate limiter reading probe bytes before the size-limiter's cheap Content-Length check (defeating its zero-byte-read fast-reject for oversized bodies). All three are fixed in commits 717a425 and f6c1bb5, each with a corresponding regression test (bucketRPCMethod bucketing, MaxInt64 clamp test, and TestComposedStack_OversizeContentLengthBeforeProbeRead). This run's own bug-hunting pass found only a minor nit (an unused struct field), and I additionally verified the three ruled-out candidates above from finder output. Given the security sensitivity of this admission path and the history of substantive findings, a human should do a final confirmation pass even though nothing outstanding remains open.
…instead of separate config value, updated tests
amir-deris
changed the title
There was a problem hiding this comment.
The wiring is well-structured, well-documented and unusually well tested, and the fail-closed MethodParser hardening looks correct. Two default-config interactions are blocking though: per-sub-request token charging makes any batch larger than ip_rate_limit_burst (400) unconditionally 429 even though batch_request_limit is 1000, and rate limiting is on by default while trusted_proxy_cidrs is empty, so a node behind an LB throttles all clients as one IP.
Findings: 3 blocking | 12 non-blocking | 7 posted inline
Blockers
- Rollout risk (relates to the inline comment on
RateLimitingEnabled: true): this PR turns per-IP rate limiting on by default on:8545withtrusted_proxy_cidrsempty. Any node already behind an ingress/LB/CDN will attribute every request to the proxy address and throttle the whole node to 200 rps / 400 burst. Please confirm the rollout plan (default off for one release, or an explicit upgrade note requiring operators to settrusted_proxy_cidrsbefore upgrading). - 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- The Cursor second-opinion review file (
cursor-review.md) is empty — that pass produced no output, so this review merges only Claude's and Codex's findings. - No test covers the chunked / understated-
Content-Lengthoversize path through the composed stack (the*http.MaxBytesErrorcase in the inline comment onrate_limit_middleware.go:34).TestComposedStack_OversizeContentLengthBeforeProbeReadonly exercises the header-onlyContent-Lengthcheck. - Rejections are plain-text HTTP bodies (
bad request/too many requests) rather than JSON-RPC error envelopes, soethers/web3.jsclients will surface opaque transport errors instead of-32700/-32600. This matches the existingrequestSizeLimiterbehaviour, so it is consistent — but consider a JSON-RPC envelope plus aRetry-Afterheader on 429 for the rate-limited case, since that one is expected in normal operation. - The token check happens after the full body is read, so a 429 still costs a complete body read (up to 5 MiB). The
maxConcurrentRequestBytessemaphore bounds the blast radius, but load shedding would be much cheaper if one token were charged per request by IP before reading the body, with the per-method charge applied after parsing. - The middleware sits outside
NewHTTPHandlerStack, so requests that CORS/vhost/JWT validation would reject still consume tokens and are answered by this layer before content-type validation. Probably intended (protect before auth), worth confirming. - WebSocket (
:8546) remains unprotected. That is explicitly out of scope, but it does mean the cheapest way to bypass the new HTTP limits is to use the WS port — worth ensuring 1b-ws lands in the same release train as enabling this by default. - Double parse of every request body (this middleware plus
sei_legacy_http.go's full-body parse) is acknowledged in the PR as a follow-up; noting it here so it does not get lost. - 5 suggestion(s)/nit(s) flagged inline on specific lines.
| return false, "", parseErr | ||
| } | ||
|
|
||
| for _, method := range methods { |
There was a problem hiding this comment.
[blocker] One token is charged per batch element, but the defaults are ip_rate_limit_burst = 400 and batch_request_limit = 1000. rate.Limiter.Allow() never waits, so any batch with more than Burst elements can never succeed regardless of how idle the client is: the first 400 Allow calls drain the bucket, the 401st fails, and the whole batch is rejected with 429. Batches of 401–1000 items are permitted by batch_request_limit but unconditionally fail once this ships enabled by default.
Also note that the tokens consumed by the prefix methods are not refunded on rejection, so a client retrying a large batch keeps draining its bucket and can never make progress.
Suggestions: use limiter.AllowN(now, len(methods)) (which fails atomically without partial consumption), and/or reconcile the defaults so ip_rate_limit_burst >= batch_request_limit, and/or cap the charge per request. Whichever you pick, the two defaults should be consistent with each other.
| TraceBakeSnapshotWindow: 64, | ||
| IPRateLimitRPS: 200, | ||
| IPRateLimitBurst: 400, | ||
| RateLimitingEnabled: true, |
There was a problem hiding this comment.
[blocker] Defaulting RateLimitingEnabled to true while TrustedProxyCIDRs defaults to nil (trust-none) means that on any node behind a reverse proxy / LB / k8s ingress, IPFromHTTPRequest returns the proxy's address for every request, so all clients share a single bucket and the whole node is capped at 200 rps / 400 burst. Operators who upgrade without editing app.toml get a node-wide throttle rather than per-IP protection.
The config template does warn about this, but the combination of default-on + trust-none is the risky part. Consider defaulting rate_limiting_enabled = false for the first release (flip after operators have set trusted_proxy_cidrs), or at minimum an explicit upgrade note.
|
|
||
| ip := m.gate.registry.IPFromHTTPRequest(r) | ||
| body, err := readBoundedBody(r.Body, m.gate.maxBodyBytes) | ||
| if err != nil { |
There was a problem hiding this comment.
[suggestion] Confirmed Codex's finding, and there is precedent for the fix in this same package.
requestSizeLimiter (outer) wraps the body in http.MaxBytesReader, so a chunked or understated-Content-Length body that exceeds maxBody makes readBoundedBody's io.ReadAll return *http.MaxBytesError rather than errBodyTooLarge (the LimitedReader budget is maxBytes+1, one byte past what MaxBytesReader permits). That falls into the generic branch below: HTTP 400 bad request with reason="unparseable", instead of the documented 413 / reason="oversize".
seiLegacyHTTPGate.ServeHTTP already handles exactly this with errors.As(err, &maxErr) — mirror it here:
var maxErr *http.MaxBytesError
if errors.Is(err, errBodyTooLarge) || errors.As(err, &maxErr) { /* 413 + oversize */ }Related: a client that aborts mid-upload also lands in the unparseable branch, so that metric will mix genuine malformed-JSON-RPC with transport errors. Consider a separate reason (or skipping the metric on read errors) so unparseable stays a clean signal.
| } | ||
| r.Body = io.NopCloser(bytes.NewReader(body)) | ||
|
|
||
| allowed, _, checkErr := m.gate.Check(r.Context(), ip, bytes.NewReader(body)) |
There was a problem hiding this comment.
[nit] rejectMethod is discarded here, so the only consumer of that return value is tests — the rejected method survives only as the bucketed namespace on rpc_rate_limit_rejected_total. Either log it at debug on the 429 path (useful for diagnosing which method is exhausting a client's bucket) or drop it from Check's signature.
| }() | ||
|
|
||
| lr := &io.LimitedReader{R: body, N: maxBytes + 1} | ||
| buf, err := io.ReadAll(lr) |
There was a problem hiding this comment.
[nit] io.ReadAll starts from a 512-byte buffer and re-allocates by doubling, so a 100 KB body costs ~8 allocations and copies on every request on the hot path. Since r.ContentLength is known for the non-chunked case, pre-sizing would avoid that:
buf := bytes.NewBuffer(make([]byte, 0, hint)) // hint = min(max(ContentLength,0), maxBytes)
_, err := buf.ReadFrom(lr)| config.maxRequestBodyBytes, | ||
| config.maxConcurrentRequestBytes, | ||
| ) | ||
| if config.rateLimitGate != nil && h.rateLimitRegistry == nil { |
There was a problem hiding this comment.
[nit] h.rateLimitRegistry is written here and in NewEVMHTTPServer but never read anywhere, so both assignments (and the struct field) are dead state. Either drop the field, or — if the intent is to share one registry across planes (e.g. the future WS wiring on :8546, which today would otherwise get its own independent bucket set per port) — add a comment saying so.
| 1, | ||
| metric.WithAttributes( | ||
| attribute.String("plane", plane), | ||
| attribute.String("method", bucketRPCMethod(method)), |
There was a problem hiding this comment.
[nit] The attribute key is method but bucketRPCMethod returns only the namespace (eth, debug, other). Anyone querying rpc_rate_limit_rejected_total{method="eth_call"} will get nothing back. Renaming the key to namespace (or method_namespace) would make the metric self-describing; it is cheaper to do now than after dashboards exist.
There was a problem hiding this comment.
Well-structured, well-tested rate-limiting wiring with a genuinely careful fail-closed design (body cap layers agree, oversize rejected from Content-Length before any read, low-cardinality metric bucketing). The blocking issue is that rate_limiting_enabled ships as false while the PR description claims true, so the feature is inert as merged — that needs an explicit decision, plus attention to non-atomic per-batch token charging and the unenforced burst >= batch_request_limit invariant that will permanently 429 large batches on upgraded nodes.
Findings: 2 blocking | 13 non-blocking | 7 posted inline
Blockers
- The shipped default disables the feature, contradicting the PR description. The description states
rate_limiting_enableddefaults totrue;DefaultConfigsets it tofalseandTestReadConfigRateLimitingassertsfalse. As written, this PR is inert on every node — the HTTP admission path is unchanged unless an operator opts in. This is the single most important fact about a security feature, and a reviewer reading the description would conclude the opposite. Please reconcile: either flip the default, or correct the description and state the rollout plan (which release turns it on, and on what signal). Flagged as High by Codex as well. (inline comment onevmrpc/config/config.go:342) - 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Fail-closed rejections change the JSON-RPC error contract. Bodies that are malformed, method-less, an empty batch, a top-level non-object, or a batch with one non-object element now get plaintext
HTTP 400 bad requestinstead ofHTTP 200with a JSON-RPC-32700/-32600error object. Notably, geth processes the valid elements of[{"method":"eth_call"}, null]; this middleware rejects the whole request. Since the middleware sits outsideNewHTTPHandlerStack, these responses also carry no CORS headers, so browser dApps see an opaque network error rather than the status. Consider writing a JSON-RPC error body (and running the gate inside the CORS layer) so clients can distinguish 400/413/429 from a transport failure. Same applies to the pre-existing 413/429 fromrequestSizeLimiter, so this is consistent with current behavior — worth a decision, not necessarily a change here. - Empty
trusted_proxy_cidrsdefault is a rollout footgun for proxied nodes. With the default (trust nothing), a node behind an LB/ingress resolves every request to the LB address, so all traffic shares one bucket. The moment an operator flipsrate_limiting_enabled = true, a node serving >200 rps starts 429-ing everyone, and a single abusive client starves all others. The template comment explains the setting but nothing surfaces the misconfiguration. Suggest a startuplogger.Warnwhen rate limiting is enabled andtrusted_proxy_cidrsis empty whileX-Forwarded-Foris observed, or at minimum call this out in the rollout runbook. - Rate-limited clients still pay the full admission-parse cost. Tokens are charged only after the entire body is buffered and JSON-tokenized, so an IP already over its limit still forces a 5 MiB read + full tokenization on every request. The limiter sheds downstream execution but not pre-admission CPU. Consider charging one token per request before parsing (then top up per batch element) so blocked IPs are shed cheaply.
- Throughput impact is unmeasured. Every POST now buffers the whole body and is JSON-tokenized twice on the hot path (rate limiter, then
sei_legacy_http.go's full parse) before geth decodes it a third time. The PR correctly lists the double-parse as a follow-up, but a before/aftereth_call/eth_getLogsthroughput number on the composed stack would de-risk enabling this by default. - Metric naming/doc drift. The PR description says rejections record
rpc_rate_limit_rejected_total{plane,method}; the code recordsmethod_namespace(ratelimiter/registry.go:104). Also note that adding an attribute to an existing counter changes its series identity — fine if PLT-800 is unreleased, worth confirming. - No
Retry-Afteron 429.evmrpc/rate_limit_middleware.go:59returns bare429 too many requests. Since the refill rate is known (ip_rate_limit_rps), aRetry-Afterheader would let well-behaved clients back off instead of hot-looping. Same for theserver busy429 inrequestSizeLimiter. - Cursor's second-opinion pass produced no output —
cursor-review.mdis empty, so this review merges only Claude's and Codex's findings. Codex's two findings are both incorporated (the default-false blocker, and non-atomic batch charging). - 6 suggestion(s)/nit(s) flagged inline on specific lines.
| IPRateLimitBurst: 400, | ||
| BatchRequestLimit: 1000, | ||
| IPRateLimitBurst: defaultBatchRequestLimit, | ||
| RateLimitingEnabled: false, |
There was a problem hiding this comment.
[blocker] The PR description says rate_limiting_enabled defaults to true ("stabilisation escape hatch"), but this ships false — and TestReadConfigRateLimiting asserts false, so the disabled default is deliberate. As written, none of the wiring in this PR takes effect on any node: newRateLimitMiddleware returns inner unchanged when !gate.enabled.
Please reconcile one way or the other. If shipping dark is intentional, fix the description and state which release flips it; if the intent was on-by-default, this line and the test need to change. Right now a reviewer trusting the description would believe :8545 is protected when it isn't.
| IPRateLimitRPS: 200, | ||
| IPRateLimitBurst: 400, | ||
| BatchRequestLimit: 1000, | ||
| IPRateLimitBurst: defaultBatchRequestLimit, |
There was a problem hiding this comment.
[suggestion] The "should be at least batch_request_limit" relationship is load-bearing but only documented, never enforced. Because one token is charged per batch element, any batch with more than burst elements gets a permanent 429 — it can never succeed no matter how idle the node is.
The upgrade path makes this concrete: existing nodes already have ip_rate_limit_burst = 400 written into app.toml from the old default, while batch_request_limit stays 1000. On those nodes, flipping rate_limiting_enabled = true permanently breaks every batch of >400 calls, with a 429 that gives the client no hint why.
Suggest validating at startup (error or warn + clamp burst up to batch_request_limit) rather than relying on operators reading the comment. Raising the default here doesn't help nodes with an existing config file.
| return false, "", parseErr | ||
| } | ||
|
|
||
| for _, method := range methods { |
There was a problem hiding this comment.
[suggestion] Batch charging is not atomic: tokens for elements 0..i-1 are consumed and never returned when element i is rejected, even though nothing executes. A client retrying a batch that doesn't fit can drain each partial refill indefinitely and never make progress.
This interacts badly with the fact that batch_request_limit is enforced downstream by go-ethereum (srv.SetBatchLimits, rpcstack.go:332), after this loop: a 50k-element batch inside a 5 MiB body drains the whole bucket here and returns 429 instead of a batch-too-large error.
Prefer reserving len(methods) tokens up front (limiter.AllowN) and rejecting the batch without consuming anything if the reservation fails — and consider checking the element count against batch_request_limit before charging. Same point raised by Codex.
| http.Error(w, "request body too large", http.StatusRequestEntityTooLarge) | ||
| return | ||
| } | ||
| http.Error(w, "bad request", http.StatusBadRequest) |
There was a problem hiding this comment.
[nit] This 400 path (non-oversize read errors — client disconnect, chunked-encoding error) is the only rejection in this handler that doesn't call recordRequestRejected, so those drops are invisible in evmrpc_requests_rejected_total. Either record them under a read_error reason or add a comment explaining the deliberate omission.
| return nil, errors.New("missing request body") | ||
| } | ||
| defer func() { | ||
| _, _ = io.Copy(io.Discard, body) |
There was a problem hiding this comment.
[nit] This drain is unbounded on its own: once readBoundedBody bails at maxBytes+1, the deferred io.Copy keeps pulling from the client until the body ends. In the wired stack that's safe because requestSizeLimiter has already installed http.MaxBytesReader (as the isRequestBodyTooLarge comment notes), but the doc comment explicitly contemplates the gate "running without that wrapper" — and the follow-up WS (1b-ws) and CometBFT (1c) wiring is exactly where that could happen. Consider bounding the drain (e.g. io.CopyN(io.Discard, body, someSlack)) so the helper is safe standalone.
| if err := expectEOF(dec, lr); err != nil { | ||
| return nil, false, classifyErr(err, lr) | ||
| } | ||
| if probeLimitExceeded(lr) { |
There was a problem hiding this comment.
[nit] These two probeLimitExceeded(lr) checks (here and in the '[' branch) are unreachable: expectEOF returns nil only from the errors.Is(err, io.EOF) && lr.N > 0 branch, so lr.N > 0 is guaranteed whenever control reaches this line. Over-limit bodies are already caught by classifyErr, which is what TestParse_BodyOneByteOverLimitRejected actually exercises. Either drop them or, if they're meant as a defensive assertion, say so in a comment — as-is they read like live logic.
| require.GreaterOrEqual(t, cfg.IPRateLimitBurst, cfg.BatchRequestLimit) | ||
|
|
||
| o := getDefaultOpts() | ||
| o.rateLimitingEnabled = false |
There was a problem hiding this comment.
[nit] This sets rateLimitingEnabled = false, which is also the default, so the assertion two lines down can't distinguish "the flag was read" from "the flag was ignored". Set it to true and assert true to actually cover the ReadConfig path for evm.rate_limiting_enabled (the false case is already covered by the zero-opts block above).
| if err := expectEOF(dec, lr); err != nil { | ||
| return nil, false, classifyErr(err, lr) | ||
| } | ||
| if probeLimitExceeded(lr) { | ||
| return nil, false, ErrProbeLimit | ||
| } | ||
| return []string{method}, false, nil | ||
| case '[': | ||
| out, err := readBatchMethods(dec) |
There was a problem hiding this comment.
🟡 The probeLimitExceeded(lr) checks added after each expectEOF(dec, lr) call (both the object and batch branches) are unreachable dead code: expectEOF only returns nil when lr.N > 0, so lr.N <= 0 can never hold immediately after. Genuine over-budget bodies are already caught earlier via classifyErr when expectEOF itself returns a non-nil error — confirmed by 0% coverage on both new blocks across the full test suite. Not a merge blocker, but consider removing the checks (and the probeLimitExceeded helper) to avoid misleading future maintainers into thinking they guard a real case.
Extended reasoning...
What the code does. MethodParser.Parse (ratelimiter/method_parser.go) reads a JSON-RPC body with an io.LimitedReader sized maxProbeBytes+1, so lr.N reaching 0 unambiguously signals the body exceeded the budget. After extracting the method (object case, lines ~85-94) or all batch methods (array case, lines ~96-106), it calls expectEOF(dec, lr) to confirm no trailing data follows, and — as of this PR — immediately follows a successful expectEOF with if probeLimitExceeded(lr) { return nil, _, ErrProbeLimit }, where probeLimitExceeded just tests lr.N <= 0.
Why it can never fire. expectEOF has exactly one path that returns nil: if errors.Is(err, io.EOF) && lr.N > 0 { return nil }. Every other branch returns a non-nil error (the raw decoder error, or ErrTrailingData). So by the time Parse reaches the new probeLimitExceeded(lr) check, it is only ever reached on a nil return from expectEOF, which already proves lr.N > 0 at that exact moment. Nothing reads from lr in between (no decoder calls, no I/O), so lr.N is unchanged when probeLimitExceeded runs — meaning lr.N <= 0 is always false there. This holds identically for both the object branch ({) and the batch branch ([), since they share the same expectEOF-then-check structure.
Where the real over-budget case is actually handled. When a body genuinely exceeds maxProbeBytes, the exhaustion is caught one layer up, inside expectEOF itself: once lr.N hits 0, the underlying dec.Token() read gets io.EOF/io.ErrUnexpectedEOF, the lr.N > 0 guard in expectEOF's success branch fails, and expectEOF returns that error unchanged. Parse then routes it through classifyErr(err, lr), whose (errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF)) && lr.N <= 0 branch maps it to ErrProbeLimit — before the new post-success check is ever reached. So the new blocks duplicate logic that already exists, but in a spot the control flow can't reach.
Step-by-step proof (object case). Take a body of exactly maxProbeBytes bytes worth of a valid, complete JSON-RPC object with a padded params field pushing right up to the limit, e.g. {\"method\":\"eth_call\"} sized so the whole object is consumed with a byte or two of budget left. readMethodFromObject succeeds. expectEOF calls dec.Token(), which hits real EOF with lr.N still > 0 (say lr.N == 1), so expectEOF takes its one success branch and returns nil. Control reaches if probeLimitExceeded(lr), which checks lr.N <= 0 — but lr.N == 1 here, since nothing consumed further bytes, so the condition is false and the check is a no-op. Now take a body one byte larger than maxProbeBytes: this time expectEOF's dec.Token() returns io.EOF with lr.N == 0, so the lr.N > 0 guard fails, expectEOF returns the raw io.EOF error (not nil), Parse short-circuits into classifyErr, and ErrProbeLimit is returned from there — the new probeLimitExceeded check after expectEOF is never even evaluated in this path, because the function already returned. There is no body length for which expectEOF returns nil AND lr.N <= 0 simultaneously.
Empirical confirmation. All independent verifiers and the original finder ran go test -coverprofile over the full ratelimiter suite, including the existing probe-limit edge case tests (TestParse_ProbeLimitExceeded, TestParse_TrailingDataBeyondProbeLimit, TestParse_LargeTrailingParamsHitProbeLimit, the new TestParse_BodyOneByteOverLimitRejected, etc.) — both new if probeLimitExceeded(lr) { ... } blocks show 0 executions.
Impact and fix. This has zero runtime effect: it's not a correctness bug, doesn't change behavior for any input, and doesn't affect the fail-closed contract. It's purely a maintainability concern — a future reader could reasonably assume this guards some edge case that budget exhaustion after a successful EOF check needs special handling, when in fact that state is unreachable. Recommend either deleting both blocks along with the now-unused probeLimitExceeded helper, or adding a comment explaining they are believed unreachable if there's a reason to keep them as defense-in-depth.
| if !allowed { | ||
| logger.Debug("rate limit rejected request", "ip", ip, "method", rejectMethod, "plane", m.gate.plane) | ||
| recordRequestRejected(r.Context(), rejectReasonRateLimited) | ||
| http.Error(w, "too many requests", http.StatusTooManyRequests) | ||
| return | ||
| } |
There was a problem hiding this comment.
🟡 In rateLimitMiddleware.ServeHTTP (evmrpc/rate_limit_middleware.go:56-61), the rejected JSON-RPC method name is logged raw via logger.Debug on every 429, but MethodParser only bounds the method string by the overall body cap (5 MiB default) rather than by the 128-char bucketRPCMethod cap that the OTel metric path (Registry.Allow) deliberately applies. An attacker whose bucket is already exhausted can repeatedly POST {"method":"<~5MB string>","id":1} to force large writes into the debug log on every rejection; reuse bucketRPCMethod (or hard-cap the logged length) before logging rejectMethod.
Extended reasoning...
The bug: rateLimitMiddleware.ServeHTTP at evmrpc/rate_limit_middleware.go:56-61 logs the rejection like this:
if !allowed {
logger.Debug("rate limit rejected request", "ip", ip, "method", rejectMethod, "plane", m.gate.plane)
...
}rejectMethod comes straight from RateLimitGate.Check, which in turn comes straight from MethodParser.Parse (ratelimiter/method_parser.go). Parse extracts the JSON string value of the "method" key with no independent length limit of its own — the only bound on how large that string can be is the overall probe/body cap (MaxProbeBytes, wired to max_request_body_bytes, default 5 MiB). So a single-key JSON-RPC request such as {"method":"<~5MB of characters>","id":1} parses successfully and yields a ~5MB string as methods[0].
Why existing hardening doesn't catch this: this PR does address exactly this class of problem, but only on the metrics path. Registry.Allow (ratelimiter/registry.go) records the method as an OTel attribute only after running it through bucketRPCMethod() (ratelimiter/method_bucket.go), which caps the string at 128 characters and collapses anything unrecognized or oversized to the constant "other" — this was added specifically in response to an earlier reviewer comment about unbounded/high-cardinality attacker-controlled method strings reaching telemetry. The new Debug log line at the HTTP middleware layer bypasses that same protection entirely, reintroducing an unbounded attacker-controlled value into a different sink (the log file / log-shipping pipeline) instead of metrics.
Trigger path, step by step:
- Attacker sends a few small requests to exhaust their per-IP token bucket (
ip_rate_limit_burst, default 400, or fewer with a stricter config). - Attacker then sends
POST /with body{"method":"AAAA...(~5MB of 'A's)...","id":1}— a single top-level object with exactly one key, soMethodParser.Parsesucceeds and returns that ~5MB string as the method. RateLimitGate.Checkcallsregistry.Allow(ctx, ip, plane, method), which returnsfalseimmediately (bucket already empty, no waiting sinceAllowis non-blocking) and returns that ~5MB string asrejectMethod.rateLimitMiddleware.ServeHTTPhits the!allowedbranch and callslogger.Debug("rate limit rejected request", "ip", ip, "method", rejectMethod, "plane", ...), writing the ~5MB string into the log output (when debug-level logging is enabled).- The attacker can repeat step 2 at a high rate (each such request is a cheap fail-fast rejection, not a wait), causing a stream of ~5MB debug-log writes — able to flood local disk or overwhelm a downstream log-shipping pipeline meaningfully faster than the equivalent 429 HTTP response bodies would.
Fix: reuse the existing bucketRPCMethod() helper (or otherwise hard-cap the logged string length) before passing rejectMethod into logger.Debug, mirroring what Registry.Allow already does for the metric label.
Why this is a nit and not blocking: the effect only manifests when debug-level logging is enabled (the default is info, so the log line is never serialized in the default configuration), and the amplification is roughly 1:1 — the attacker has to upload ~5 MiB to cause ~5 MiB of log output, so it's a disk/log-pipeline hardening gap under a plausible troubleshooting configuration rather than a genuine bandwidth-amplification DoS. It doesn't affect correctness of the admission decision and doesn't crash anything. Given the PR's own precedent of hardening the exact same class of value on the metrics path, this is a cheap, worthwhile fix but not merge-blocking.
There was a problem hiding this comment.
No new bugs from this pass, but this PR still has open, previously-flagged blocker-level concerns (the PR description saying rate_limiting_enabled defaults to true while the code/tests now ship it false; the per-batch token charging in RateLimitGate.Check still spends tokens one-at-a-time with no atomic AllowN reservation and no enforced ip_rate_limit_burst >= batch_request_limit relationship) that a human should weigh in on before merging.
What was reviewed this run: the admission middleware wiring (rateLimitMiddleware/RateLimitGate), the fail-closed MethodParser contract and its probe-limit paths, the config defaults/template changes, and the per-IP registry metric labeling. The probeLimitExceeded dead-code branches and the full-body-buffer memory concern were re-examined and ruled out as live issues this pass.
Extended reasoning...
Overview
This PR wires the new ratelimiter.Registry/MethodParser package into the EVM JSON-RPC HTTP admission path on :8545, adding a new pre-decode middleware (rateLimitMiddleware/RateLimitGate) that buffers the full request body, extracts JSON-RPC method name(s), applies a per-IP token bucket, and fails closed (400/413/429) on any parse or limit issue. It also adds two new config fields (rate_limiting_enabled, trusted_proxy_cidrs) and hardens MethodParser (duplicate-method rejection, body-size fail-closed).
Security risks
This is squarely admission-control / DoS-protection code for a public JSON-RPC endpoint, so correctness here has direct availability implications. The main risk isn't injection/auth but denial-of-service: if the per-IP bucket accounting is wrong, either legitimate batches become permanently unable to succeed (starvation) or the limiter fails open more than intended. The PR's own review thread (Codex/seidroid and prior automated passes) already identified concrete instances of this class of bug — batch requests charging one token per method with no atomic reservation, and a mismatch between the documented default and the shipped default for rate_limiting_enabled. Those are still present in the code as of this diff.
Level of scrutiny
High. This touches a production RPC ingress path that will run with elevated trust once enabled, introduces new fail-closed rejection semantics that change client-visible behavior (400/413/429 where previously requests would have been dispatched), and the batch-charging semantics are subtle enough that they've already produced multiple rounds of blocker-level findings from independent reviewers. This warrants a human sign-off on the batch-charging design and on reconciling the enabled-by-default question, not just a mechanical pass.
Other factors
No new bugs were surfaced by the bug-hunting system in this pass, and the test suite is extensive (config parsing, middleware composition, per-IP isolation, probe-limit edge cases). However, several previously-raised blocker/suggestion-level comments from other reviewers on this PR (the burst/batch_request_limit relationship being unenforced, non-atomic batch token charging, the description/default mismatch for rate_limiting_enabled) do not appear to have been resolved in the current diff, so a human should confirm whether those are accepted trade-offs or need a follow-up fix before merge.
| for _, method := range methods { | ||
| if !g.registry.Allow(ctx, ip, g.plane, method) { | ||
| return false, method, nil | ||
| } |
There was a problem hiding this comment.
Batch reject burns prior tokens
Medium Severity
Check admits batch methods by calling Allow per element. On a mid-batch denial the whole request is rejected, but tokens already taken for earlier methods are not returned. Clients (and any peers sharing an IP) lose budget for work that never ran; a batch just over the remaining burst can drain the bucket in one refused call.
Reviewed by Cursor Bugbot for commit c7edbde. Configure here.
There was a problem hiding this comment.
Solid, well-tested implementation of fail-closed rate-limit admission on :8545 — it ships disabled by default, the handler ordering and body-cap layering are coherent, and the startup guard for burst vs. batch limit is a nice touch. No blocking correctness or security defects found; the notes below cover load-shedding efficiency, an observability gap, an unreachable check in the parser, and behavior-change/test-coverage items to settle before flipping the flag on.
Findings: 0 blocking | 16 non-blocking | 8 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes: Codex reported "No material issues found";
cursor-review.mdis empty, i.e. that pass produced no output. Only one external pass effectively contributed. - When enabled, a single request body is now buffered twice and JSON-scanned three times (MethodParser,
wrapSeiLegacyHTTP, then go-ethereum). The PR already tracks unification as a follow-up, but worth measuring p99/alloc impact on large batches before flippingrate_limiting_enabledon fleet-wide. - Behavior change on malformed/empty bodies: an empty POST body currently gets HTTP 200 with an empty response from go-ethereum, and malformed JSON gets a JSON-RPC
-32700object; with the gate enabled both become plain-text400 bad request. LB/health probes that POST an empty body would start failing. Consider passing empty bodies straight through, and/or returning a JSON-RPC error envelope instead ofhttp.Errortext. ratelimiter.DefaultBurstis still 400 whileevmrpc/confignow defaultsip_rate_limit_burstto 1000 — two disagreeing sources of truth for the same knob.- The metric attribute emitted is
method_namespace, notmethodas the PR description states — worth correcting in the description/dashboards. Also notemethodonly feeds the metric label: the bucket is not per-method, sodebug_traceTransactioncosts the same single token asweb3_clientVersion. Fine for 1b-core, but the PR wording ("applies the per-IP token bucket per method") reads as if per-method limits exist. - Batches larger than
batch_request_limitdrain the caller's bucket at the gate before go-ethereum's batch-limit rejection ever runs, so the client sees 429 instead of the clearer batch-too-large error. Charging at mostbatch_request_limittokens (or rejecting the over-limit batch at the gate) would be tidier. - Test gaps: (a) no test that
NewEVMHTTPServer/EnableRPCactually wires the gate into the served stack — every test composes the middleware by hand, so a future refactor could silently drop it; (b) no test for the documentedrate_limiting_enabled = true+ip_rate_limit_burst = 0case (method extraction and 400/413 still apply, no 429); (c) no test thatConfigTemplaterenders valid TOML for a populatedtrusted_proxy_cidrslist. - I could not execute
go test ./ratelimiter/... ./evmrpc/...orgofmt -lin this environment (command approval denied), so the findings are from static review only; the PR's own test plan claims these pass. - 8 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
|
|
||
| ip := m.gate.registry.IPFromHTTPRequest(r) | ||
| body, err := readBoundedBody(r.Body, m.gate.maxBodyBytes) |
There was a problem hiding this comment.
[suggestion] Load shedding happens after the full body read and a complete JSON scan: an IP that is already over its limit still costs a read of up to max_request_body_bytes (5 MiB default) plus a full MethodParser pass before it gets a 429. That inverts the usual point of admission control — under a flood, the parse is the expensive part.
Consider a cheap pre-parse check on the IP's bucket (e.g. a non-consuming Tokens() < 1 probe, or reserving one token before parsing and charging the remaining n-1 after) so exhausted IPs are shed before the body is buffered and scanned.
| http.Error(w, "request body too large", http.StatusRequestEntityTooLarge) | ||
| return | ||
| } | ||
| http.Error(w, "bad request", http.StatusBadRequest) |
There was a problem hiding this comment.
[nit] This 400 path records no requestRejectedCount metric, unlike the oversize / unparseable / rate-limited paths. It fires on transport-level read errors and on body == nil, so rejections here are invisible on dashboards. Either record a reason here (a new read_error value, or reuse rejectReasonUnparseable) or note in a comment why it's deliberately unmetered.
| } | ||
|
|
||
| func (m *rateLimitMiddleware) ServeHTTP(w http.ResponseWriter, r *http.Request) { | ||
| if r.Method != http.MethodPost { |
There was a problem hiding this comment.
[nit] Non-POST requests bypass the gate entirely, so GET/OPTIONS/HEAD floods stay unmetered on :8545. They're cheap for the inner stack to answer (health-check 200 / CORS preflight / 405), so this is likely fine — but it's an intentional gap worth stating in the doc comment so the next reader doesn't assume :8545 is fully covered.
| if !allowed { | ||
| logger.Debug("rate limit rejected request", "ip", ip, "method", rejectMethod, "plane", m.gate.plane) | ||
| recordRequestRejected(r.Context(), rejectReasonRateLimited) | ||
| http.Error(w, "too many requests", http.StatusTooManyRequests) |
There was a problem hiding this comment.
[nit] Two small response-shape points: (1) no Retry-After header on the 429, which well-behaved clients use to back off (ip_rate_limit_rps gives you a computable value); (2) this middleware sits outside NewHTTPHandlerStack, so 400/413/429 responses carry no CORS headers and browser clients see an opaque network error rather than the status. requestSizeLimiter already has the same property, so this is consistency-with-precedent rather than a regression — but if browser-visible throttling matters, worth fixing for both.
| if err := expectEOF(dec, lr); err != nil { | ||
| return nil, false, classifyErr(err, lr) | ||
| } | ||
| if probeLimitExceeded(lr) { |
There was a problem hiding this comment.
[nit] This check (and the matching one in the '[' branch) is unreachable: expectEOF returns nil only when errors.Is(err, io.EOF) && lr.N > 0, so on reaching here lr.N > 0 always holds and probeLimitExceeded is always false. The over-limit cases are already covered by expectEOF returning io.EOF with lr.N <= 0 → classifyErr → ErrProbeLimit (which is what TestParse_BodyOneByteOverLimitRejected actually exercises). Either drop both checks or keep them with a comment marking them as defensive against a future expectEOF change.
| return cfg, fmt.Errorf("%s must be >= 0 (0 disables the limit), got %d", flagMaxOpenConnections, cfg.MaxOpenConnections) | ||
| } | ||
| } | ||
| if cfg.RateLimitingEnabled && cfg.IPRateLimitBurst > 0 && cfg.BatchRequestLimit > 0 && |
There was a problem hiding this comment.
[suggestion] Good guard. Worth adding a companion warning for the bigger operational footgun: rate_limiting_enabled = true with an empty trusted_proxy_cidrs behind a proxy/LB. IPFromHTTPRequest then falls back to RemoteAddr, every client collapses into the LB's single bucket, and one caller can 429 the entire node's traffic. Trust-none is the right default for a directly-exposed node so this can't be a hard error — but a startup logger.Warn when RateLimitingEnabled && len(TrustedProxyCIDRs) == 0 would catch the misconfiguration before a canary does.
| httpConfig.batchResponseSizeLimit = config.BatchResponseMaxSize | ||
| httpConfig.maxRequestBodyBytes = config.MaxRequestBodyBytes | ||
| httpConfig.maxConcurrentRequestBytes = config.MaxConcurrentRequestBytes | ||
| rateLimitRegistry, err := ratelimiter.New(config.RateLimiterConfig()) |
There was a problem hiding this comment.
[suggestion] An invalid entry in trusted_proxy_cidrs only surfaces here, at server construction, whereas every other EVM RPC config value is validated in ReadConfig. Running the equivalent CIDR validation in ReadConfig (next to the new burst/batch check) would fail fast alongside the other config errors and keep the failure mode uniform.
| t.Cleanup(func() { otel.SetMeterProvider(prev) }) | ||
|
|
||
| // Re-init metrics against the test provider. | ||
| registryMetrics.rejectedCounter = must(provider.Meter("ratelimiter").Int64Counter( |
There was a problem hiding this comment.
[nit] registryMetrics.rejectedCounter is swapped globally but never restored (the t.Cleanup only restores the meter provider), so every later test in the package records into this test's reader. Harmless today, but it makes assertions order-dependent for any future test that counts rejections. Capture the previous counter and restore it in the same t.Cleanup. Same pattern at evmrpc/rate_limit_middleware_test.go:355 for metrics.requestRejectedCount.
Superseded: latest AI review found no blocking issues.
| if cfg.RateLimitingEnabled && cfg.IPRateLimitBurst > 0 && cfg.BatchRequestLimit > 0 && | ||
| cfg.IPRateLimitBurst < cfg.BatchRequestLimit { | ||
| return cfg, fmt.Errorf( | ||
| "%s (%d) must be >= %s (%d): the rate limiter charges one token per batch element, "+ | ||
| "so a lower burst would permanently reject any full-size batch", | ||
| flagIPRateLimitBurst, cfg.IPRateLimitBurst, flagBatchRequestLimit, cfg.BatchRequestLimit, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🟡 The new burst-vs-batch validation in ReadConfig (config.go:693-700) checks IPRateLimitBurst < BatchRequestLimit but never checks IPRateLimitRPS, even though Registry.Allow treats RPS<=0 as a full bucket bypass (always allow, batch draining is impossible). This means the documented admission-only setup (rate_limiting_enabled=true, ip_rate_limit_rps=0, ip_rate_limit_burst=400, batch_request_limit=1000) now fails ReadConfig with a spurious error and blocks node startup, even though the guarded failure mode can't occur in that mode. Fix: also require cfg.IPRateLimitRPS > 0 before enforcing the burst>=batch check.
Extended reasoning...
The bug. ReadConfig (evmrpc/config/config.go:693-700) added in this PR rejects configs where RateLimitingEnabled && IPRateLimitBurst>0 && BatchRequestLimit>0 && IPRateLimitBurst<BatchRequestLimit, on the theory (documented in the new field comments) that the token bucket charges one token per batch element, so a burst smaller than the batch limit would permanently reject any full-size batch. That check never inspects IPRateLimitRPS.
Why the check is incomplete. Registry.Allow (ratelimiter/registry.go:93) short-circuits with if r.cfg.RPS <= 0 || r.cfg.Burst <= 0 { return true }. So whenever IPRateLimitRPS<=0, the token bucket is fully disabled and Allow always returns true — the per-method loop in RateLimitGate.Check never drains a token and can never reject a batch. The very failure mode the new validation guards against (batch draining causing a permanent 429) is impossible once RPS<=0. Yet ReadConfig still applies the burst-vs-batch check in that case, because it never looks at IPRateLimitRPS.
Why this contradicts documented behavior. This same PR's new doc comment on IPRateLimitRPS explicitly describes rate_limiting_enabled=true + ip_rate_limit_rps=0 as a supported "admission-only" mode: "Zero disables the token bucket (no HTTP 429 rejections). When rate_limiting_enabled is true, the admission middleware still runs: bodies are parsed and oversize/malformed requests are rejected before dispatch." A config like rate_limiting_enabled=true, ip_rate_limit_rps=0, ip_rate_limit_burst=400, batch_request_limit=1000 is exactly that mode — harmless, since the bucket is off — but it now fails ReadConfig with "ip_rate_limit_burst (400) must be >= batch_request_limit (1000)" and prevents the node from starting at all.
Why nothing catches it today. The new test TestReadConfigRateLimitingBurstBelowBatchLimitRejected only exercises ipRateLimitBurst=0 as the "disabled" case; getDefaultOpts() always sets ipRateLimitRPS=200.0, so the RPS=0-with-burst>0 combination is never exercised by any test.
Step-by-step proof.
- Operator upgrades a node that already has
ip_rate_limit_burst = 400in itsapp.toml(the old default before this PR bumped it to 1000), and separately decides to run the newly-documented admission-only mode by settingrate_limiting_enabled = trueandip_rate_limit_rps = 0(leavingbatch_request_limitat its default 1000). ReadConfigevaluates the new guard:RateLimitingEnabledis true,IPRateLimitBurst=400>0,BatchRequestLimit=1000>0, and400 < 1000— all four conditions hold, so it returnsfmt.Errorf("... must be >= ...").- This happens regardless of
IPRateLimitRPS, even though withRPS=0,Registry.Allowwould returntrueunconditionally and the batch-draining scenario the check exists to prevent cannot happen. - The node fails to start, even though the chosen configuration is explicitly documented as valid and is in fact safe.
Fix. Add && cfg.IPRateLimitRPS > 0 to the guard's condition, mirroring the existing IPRateLimitBurst > 0 check, so the burst-vs-batch validation only fires when the token bucket is actually active.
Severity. This is a real correctness gap in the new validation and it does block node startup for an operator using a mode the PR itself documents as valid — but it's narrow: rate limiting ships disabled by default, the default burst(1000)==batch(1000) so the common enabled path is unaffected, the failure is a clear, immediate startup error (not silent misbehavior or data loss), and the workaround (raise or zero out ip_rate_limit_burst) is trivial once diagnosed. Given that, this is a nit rather than a blocking issue.
| if v := opts.Get(flagRateLimitingEnabled); v != nil { | ||
| if cfg.RateLimitingEnabled, err = cast.ToBoolE(v); err != nil { | ||
| return cfg, err | ||
| } | ||
| } | ||
| if v := opts.Get(flagTrustedProxyCIDRs); v != nil { | ||
| if cfg.TrustedProxyCIDRs, err = cast.ToStringSliceE(v); err != nil { | ||
| return cfg, err | ||
| } | ||
| } |
There was a problem hiding this comment.
Note for later: as part of the configuration manager project we are creating comprehensive test coverage that asserts the behavior of config values. I know you have added lots of new config. It would be good to have a follow-up action item on these to make sure we have full test coverage over these values. I've created an AGENTS.md in #3816 that should make this simple
There was a problem hiding this comment.
Thanks for feedback. Created https://linear.app/seilabs/issue/PLT-874/ to make sure we have full coverage for the newly added config fields.
There was a problem hiding this comment.
The rate-limiter wiring is well-structured, fail-closed by design, and well-tested at the middleware/parser level, but it changes the [evm] configuration contract without updating the testutil/configtest characterization artifacts, so TestDefaultsMatchTheRecordedValues and TestManifestNamesEveryField will fail CI. Beyond that blocker there are several operability/observability gaps worth addressing before the flag is flipped on.
Findings: 4 blocking | 12 non-blocking | 8 posted inline
Blockers
evmrpc/config/config_fuzz_test.goneeds manifest rows for the two new reads —{Key: "evm.rate_limiting_enabled", Path: "RateLimitingEnabled", Cast: configtest.CastBool, Checked: true}and{Key: "evm.trusted_proxy_cidrs", Path: "TrustedProxyCIDRs", Cast: configtest.CastStringSlice, Checked: true}— plus per-row fuzz seeds.TestManifestNamesEveryField(CheckManifestCoversEveryField) requires every field ofconfig.DefaultConfigto be named by a row'sPath/AlsoWritesor exempted at the call site;RateLimitingEnabledandTrustedProxyCIDRsare neither, so it fails today. Seetestutil/configtest/AGENTS.md§"Adding a Key to an Existing Section".evmrpc/config/testdata/evm.goldenstill recordsIPRateLimitBurst = int(400)and has noRateLimitingEnabled/TrustedProxyCIDRsentries, soTestDefaultsMatchTheRecordedValuesfails. Regenerate withgo test ./evmrpc/config/ -run TestDefaultsMatchTheRecordedValues -updatein its own commit, and name the old/new burst value (400 → 1000) in the PR body — the PR description currently doesn't mention the default change at all. (Same finding as Codex's P1.)- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Cursor's second-opinion pass (
cursor-review.md) produced no output — that file is empty, so this review reflects only Claude + Codex. - Peak-memory accounting:
max_concurrent_request_byteswas sized to bound decode-time memory, butreadBoundedBodynow holds a second full copy of every in-flight body (up tomax_request_body_byteseach) that the semaphore doesn't account for, roughly doubling worst-case resident request bytes when the flag is on. Worth either charging the buffer against the same budget or documenting the new ceiling. - The same body is now scanned for
"method"twice — once byMethodParser, once byseiLegacyHTTPGate— plus a third decode in geth. The PR lists unification as a follow-up; just flagging that with a 5 MiB cap this is three full JSON walks per request on the hot path. - Rate limiting only covers
:8545; WS (:8546) and CometBFT (:26657) remain unprotected, so a client can bypass the per-IP bucket entirely by switching transports. Acknowledged in the PR as 1b-ws/1c, but the HTTP-only limiter shouldn't be described as endpoint protection until those land. ratelimiter.DefaultBurstis still 400 inratelimiter/registry.go:22whileevmrpc's default moved to 1000.ratelimiter.DefaultConfighas no in-tree consumer today, but the two now disagree — consider aligning or dropping the unused defaults.TestRateLimitMiddleware_ParseErrorRecordsRejectedMetricoverwrites the package-globalmetrics.requestRejectedCountwith not.Cleanuprestore, leaving later tests inevmrpcrecording into a discarded provider. Matches the existing pattern inratelimiter/registry_test.go, but a restore would keep it order-independent.- 6 suggestion(s)/nit(s) flagged inline on specific lines.
| IPRateLimitRPS: 200, | ||
| IPRateLimitBurst: 400, | ||
| BatchRequestLimit: 1000, | ||
| IPRateLimitBurst: defaultBatchRequestLimit, |
There was a problem hiding this comment.
[blocker] This changes the ip_rate_limit_burst default from 400 to 1000, which is a pinned contract: evmrpc/config/testdata/evm.golden still records IPRateLimitBurst = int(400), so TestDefaultsMatchTheRecordedValues fails. Regenerate the golden (go test ./evmrpc/config/ -run TestDefaultsMatchTheRecordedValues -update) and name the old/new value in the PR body per testutil/configtest/AGENTS.md §"Changing a Default". The two new fields on lines 342-343 also need golden entries.
| return cfg, err | ||
| } | ||
| } | ||
| if v := opts.Get(flagRateLimitingEnabled); v != nil { |
There was a problem hiding this comment.
[blocker] These two new reads need rows in the evmKeys manifest in evmrpc/config/config_fuzz_test.go (inserted here, in reader order), describing the read as written:
{Key: "evm.rate_limiting_enabled", Path: "RateLimitingEnabled", Cast: configtest.CastBool, Checked: true},
{Key: "evm.trusted_proxy_cidrs", Path: "TrustedProxyCIDRs", Cast: configtest.CastStringSlice, Checked: true},Without them TestManifestNamesEveryField fails — CheckManifestCoversEveryField requires every resolved field to be claimed by a row or exempted, and RateLimitingEnabled/TrustedProxyCIDRs are neither. Also add the nil + malformed fuzz seeds so an ordinary go test run reaches the new rows (the loop at config_fuzz_test.go:108 covers that automatically, but double-check the hand-written positional seeds below it still point at the keys they name after the insertion).
| return cfg, fmt.Errorf("%s must be >= 0 (0 disables the limit), got %d", flagMaxOpenConnections, cfg.MaxOpenConnections) | ||
| } | ||
| } | ||
| if cfg.RateLimitingEnabled && cfg.IPRateLimitBurst > 0 && cfg.BatchRequestLimit > 0 && |
There was a problem hiding this comment.
[suggestion] This turns a previously-valid config into a hard boot failure, and the affected population is large: every existing node has ip_rate_limit_burst = 400 materialized into its app.toml from the old default, so the first thing an operator sees after setting rate_limiting_enabled = true is a node that won't start. The error text is good, but consider either clamping burst up to batch_request_limit with a logger.Warn, or calling the two-step config change out explicitly in the rollout notes so the opt-in isn't a surprise outage.
| http.Error(w, "request body too large", http.StatusRequestEntityTooLarge) | ||
| return | ||
| } | ||
| http.Error(w, "bad request", http.StatusBadRequest) |
There was a problem hiding this comment.
[suggestion] This 400 is the only rejection path that doesn't call recordRequestRejected, so read failures (client disconnect mid-body, transport errors) are invisible in evmrpc_requests_rejected_total while every neighbouring branch is counted. Suggest recording rejectReasonUnparseable (or a new read_error reason) here for consistency.
|
|
||
| allowed, rejectMethod, checkErr := m.gate.Check(r.Context(), ip, bytes.NewReader(body)) | ||
| if checkErr != nil { | ||
| if ratelimiter.IsBodyTooLarge(checkErr) { |
There was a problem hiding this comment.
[nit] Unreachable in this stack: readBoundedBody above already bounds the body by the same m.gate.maxBodyBytes that NewRateLimitGate hands to NewMethodParser, so Parse can never return ErrProbeLimit here. Note TestRateLimitMiddleware_ProbeLimitRejected413 (90-byte body, 64-byte cap) actually exercises readBoundedBody, not this branch, despite its name — the middleware-level IsBodyTooLarge mapping is untested. Harmless defensive code, but the test name is misleading.
| return | ||
| } | ||
| recordRequestRejected(r.Context(), rejectReasonUnparseable) | ||
| http.Error(w, "bad request", http.StatusBadRequest) |
There was a problem hiding this comment.
[suggestion] Client-visible behavior change once the flag is on: geth answers a malformed or method-less JSON-RPC body with HTTP 200 + a JSON-RPC error object, whereas this returns text/plain 400. Bodies that previously got a structured -32600/-32700 (missing method, empty batch, empty POST body, trailing data) now surface as a transport error in ethers/viem/web3 rather than an RPC error. Fail-closed is the right call, but the response ought to be a JSON-RPC error envelope (or the divergence documented in the rollout notes) so integrators aren't debugging opaque 400s.
| return false, "", parseErr | ||
| } | ||
|
|
||
| for _, method := range methods { |
There was a problem hiding this comment.
[suggestion] Two issues with charging one token per element in a loop:
- Tokens are consumed on a rejected batch. If element 5 of 10 exhausts the bucket, elements 1-4 already spent tokens and the whole batch is still rejected — the caller pays and gets nothing, and a retry loop drains its own bucket faster than the configured rate.
AllowN(now, len(methods))would make the charge atomic and match the documented "whole batch rejected" semantics. batch_request_limitisn't enforced yet at this layer. geth's batch-item limit is applied insideNewHTTPHandlerStack, downstream of here, so a 100k-element batch (trivially under 5 MiB) is fully parsed and charges 100k tokens before geth rejects it for being oversized. Checkinglen(methods)against the batch limit before charging would avoid both the wasted parse and the self-inflicted bucket drain.
| if err != nil { | ||
| return nil, fmt.Errorf("evm rate limiter: %w", err) | ||
| } | ||
| httpConfig.rateLimitGate = NewRateLimitGate( |
There was a problem hiding this comment.
[suggestion] Consider a logger.Info here when config.RateLimitingEnabled is true, reporting effective RPS/burst and len(config.TrustedProxyCIDRs) — and a logger.Warn when the CIDR list is empty. The default is trust-none, so a node behind an LB that opts in collapses every client into a single bucket keyed on the LB address and starts 429ing the whole fleet at 200 rps. The config template warns about this, but the failure mode is indistinguishable from a real traffic spike at runtime, and a startup line is the cheapest way to make it diagnosable.
There was a problem hiding this comment.
Beyond the reported configtest-suite finding, this run also examined and ruled out a few other candidates: malformed/parse-error requests skipping the rate-limit token charge (rate_limit.go) — reads as intentional given the fail-closed contract, not a bypass; package-global OTel counter overwrites in registry_test.go without cleanup — confirmed harmless today, no cross-test assertion depends on prior values; and a goroutine leak from Registry construction per HTTPServer in server.go — the registry does not spawn a background goroutine.
Extended reasoning...
This run's own analysis reproduced the reported configtest-suite finding directly on the current checkout (go test ./evmrpc/config/... -run TestManifestNamesEveryField|TestDefaultsMatchTheRecordedValues fails deterministically), confirming it as a real, CI-blocking issue rather than a false positive — this alone is sufficient reason not to auto-approve. Beyond that finding, three other candidate issues raised by finder agents were examined and refuted this run (see message), so recording them here for the record without restating the inline finding.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9054c11. Configure here.
| "%s (%d) must be >= %s (%d): the rate limiter charges one token per batch element, "+ | ||
| "so a lower burst would permanently reject any full-size batch", | ||
| flagIPRateLimitBurst, cfg.IPRateLimitBurst, flagBatchRequestLimit, cfg.BatchRequestLimit, | ||
| ) |
There was a problem hiding this comment.
Burst check ignores zero RPS
Low Severity
The startup check that ip_rate_limit_burst must be at least batch_request_limit runs whenever rate limiting is enabled and burst is positive, even if ip_rate_limit_rps is zero. With RPS at or below zero the token bucket is disabled and never charges, so this rejects a documented “admission only, no 429” configuration for no functional reason.
Reviewed by Cursor Bugbot for commit 9054c11. Configure here.
There was a problem hiding this comment.
Solid, well-tested wiring of the fail-closed rate limiter into the EVM JSON-RPC HTTP path, shipped dark behind rate_limiting_enabled = false, with config characterization (fuzz key specs + golden file) correctly updated. No blocking correctness or security defects found; the notes below are about token-accounting fairness on batches, the burst-default relaxation, operator foot-guns, and some dead code / instrumentation gaps.
Findings: 0 blocking | 14 non-blocking | 7 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review file (
cursor-review.md) is empty — that pass produced no output. Codex (codex-review.md) reported no material issues. All findings here are from this pass alone. - Behavior change for existing clients: bodies that go-ethereum previously tolerated now get a plain-text HTTP 400 instead of a JSON-RPC
-32700/-32600error object. Specifically (a) trailing non-whitespace data after the top-level value, which go-ethereum'sreadBatchignores, and (b) duplicate"method"keys, whichencoding/jsonpreviously resolved to the last value. Rejecting (b) is the point of the hardening, but both are client-visible and neither is called out in the PR description's rollout notes. Consider emitting a JSON-RPC error envelope for the 400 path so clients keep parsing responses uniformly. - WebSocket (:8546) is explicitly out of scope, which means that on a node exposing both ports the per-IP HTTP limit is trivially bypassed by switching to WS. That's a legitimate deferral, but the rollout notes should state it so the 1b-ws follow-up isn't treated as cosmetic before the fleet-wide default flip.
- Every POST body is now parsed for
"method"twice when the sei-legacy allowlist is active (once byrateLimitMiddleware/MethodParser, once byseiLegacyHTTPGate's fullio.ReadAll+ decode). The PR acknowledges this as a follow-up; worth confirming the CPU cost on a 5 MiB body before enabling in canary, since both parses are on the hot admission path. ratelimiter.DefaultBurstis still 400 while the evmrpc default is now 1000 (defaultBatchRequestLimit). Two disagreeing "defaults" for the same knob is a trap for the next caller ofratelimiter.DefaultConfig; either align them or comment why they differ.- Test hygiene:
evmrpc/rate_limit_middleware_test.go:355andratelimiter/registry_test.go:87overwrite the package-level metric counters (metrics.requestRejectedCount,registryMetrics.rejectedCounter) and never restore them, unlike theotel.SetMeterProviderswap right above which does uset.Cleanup. Harmless today, but it makes any future test that asserts on these counters order-dependent. evmrpc/confignow importsratelimitersolely to construct a struct inRateLimiterConfig(). Sinceevmrpcalready imports both packages, that helper could live inevmrpcand keep the config package free of a dependency on a runtime component.- 7 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
|
|
||
| for _, method := range methods { | ||
| if !g.registry.Allow(ctx, ip, g.plane, method) { |
There was a problem hiding this comment.
[suggestion] Tokens consumed by earlier batch elements are not refunded when a later element is rejected. A 1000-element batch whose 999th element exhausts the bucket burns 998 tokens and returns 429 having served nothing — the client is throttled harder than the configured RPS implies, and an attacker can convert one request into a full bucket drain plus zero work.
Also note this charges before go-ethereum's batch_request_limit is enforced, so an over-limit batch (e.g. 5000 elements inside 5 MiB) drains up to the whole burst and returns 429 instead of the batch-limit error.
Consider reserving atomically up-front instead of per-element — e.g. limiter.AllowN(now, len(methods)) for the bucket decision, then use the per-method loop only to pick the label for the rejection metric. That also removes the need to raise the burst default to match batch_request_limit.
| IPRateLimitRPS: 200, | ||
| IPRateLimitBurst: 400, | ||
| BatchRequestLimit: 1000, | ||
| IPRateLimitBurst: defaultBatchRequestLimit, |
There was a problem hiding this comment.
[suggestion] This raises the default per-IP burst 2.5× (400 → 1000) for every deployment, not just ones that enable rate limiting — a security-relevant relaxation being made to satisfy the new burst ≥ batch_request_limit invariant rather than because 1000 is the right burst.
If the per-batch charge were capped (see the AllowN note on rate_limit.go), the invariant disappears and the 400 default can stand. If you keep the coupling, the golden-file diff is the right place to have surfaced it (and it did), but the PR description should call out the default change explicitly since it changes behavior for nodes that never opt in.
| httpConfig.batchResponseSizeLimit = config.BatchResponseMaxSize | ||
| httpConfig.maxRequestBodyBytes = config.MaxRequestBodyBytes | ||
| httpConfig.maxConcurrentRequestBytes = config.MaxConcurrentRequestBytes | ||
| rateLimitRegistry, err := ratelimiter.New(config.RateLimiterConfig()) |
There was a problem hiding this comment.
[suggestion] Worth a startup warning here: if an operator sets rate_limiting_enabled = true on a node behind a proxy/LB but leaves trusted_proxy_cidrs empty (the default), IPFromHTTPRequest resolves every request to the LB's address, so all clients share a single 200 RPS bucket — an effective self-DoS that will look like a node problem, not a config problem.
A logger.Warn when RateLimitingEnabled && len(TrustedProxyCIDRs) == 0 would make this diagnosable at boot instead of during an incident. The config template documents the hazard, but nothing in the runtime path does.
| http.Error(w, "request body too large", http.StatusRequestEntityTooLarge) | ||
| return | ||
| } | ||
| http.Error(w, "bad request", http.StatusBadRequest) |
There was a problem hiding this comment.
[suggestion] This rejection path records no metric, unlike every other reject branch in this handler (oversize/unparseable/rate_limited all call recordRequestRejected). A body read failure here — client abort, truncated chunked transfer — becomes an invisible 400. Recording rejectReasonUnparseable (or a new reason) would keep the rejection accounting complete.
| return | ||
| } | ||
| if !allowed { | ||
| logger.Debug("rate limit rejected request", "ip", ip, "method", rejectMethod, "plane", m.gate.plane) |
There was a problem hiding this comment.
[nit] rejectMethod is an unvalidated, attacker-controlled string from the request body, bounded only by max_request_body_bytes — a client can send a multi-megabyte "method" value and have it written verbatim into the log line (and it may contain newlines). Debug level limits the blast radius, but logging bucketRPCMethod(rejectMethod) or a truncated form would be safer for the same diagnostic value.
| if !allowed { | ||
| logger.Debug("rate limit rejected request", "ip", ip, "method", rejectMethod, "plane", m.gate.plane) | ||
| recordRequestRejected(r.Context(), rejectReasonRateLimited) | ||
| http.Error(w, "too many requests", http.StatusTooManyRequests) |
There was a problem hiding this comment.
[nit] Consider setting Retry-After on the 429. The bucket refills at a known rate (ip_rate_limit_rps), so a hint is cheap to compute and lets well-behaved clients back off instead of hot-looping — which matters most precisely when the limiter is engaging.
| if err := expectEOF(dec, lr); err != nil { | ||
| return nil, false, classifyErr(err, lr) | ||
| } | ||
| if probeLimitExceeded(lr) { |
There was a problem hiding this comment.
[nit] This check (and its twin at line 104) is unreachable: expectEOF returns nil only via the errors.Is(err, io.EOF) && lr.N > 0 branch, so on the success path lr.N > 0 already holds and probeLimitExceeded(lr) is always false. Over-limit bodies are caught by classifyErr/expectEOF returning the error instead — which is what TestParse_BodyOneByteOverLimitRejected actually exercises.
Either drop both checks or, if they're intended as a guard against expectEOF changing, say so in a comment — as written they read like the mechanism enforcing the whole-body contract when they aren't.
Superseded: latest AI review found no blocking issues.
| if err != nil { | ||
| if isRequestBodyTooLarge(err) { | ||
| recordRequestRejected(r.Context(), rejectReasonOversize) | ||
| http.Error(w, "request body too large", http.StatusRequestEntityTooLarge) | ||
| return | ||
| } | ||
| http.Error(w, "bad request", http.StatusBadRequest) | ||
| return | ||
| } |
There was a problem hiding this comment.
🟡 In rateLimitMiddleware.ServeHTTP (evmrpc/rate_limit_middleware.go:34-42), the read-error rejection path (body==nil or a transport read error/client disconnect mid-body) calls http.Error with a 400 but never calls recordRequestRejected, unlike every other rejection branch in this file. This makes that class of rejection invisible in the evmrpc_requests_rejected_total metric; fix by recording rejectReasonUnparseable (or a dedicated reason) before the http.Error call.
Extended reasoning...
The bug. rateLimitMiddleware.ServeHTTP in evmrpc/rate_limit_middleware.go calls readBoundedBody(r.Body, m.gate.maxBodyBytes) and branches on the returned error:
body, err := readBoundedBody(r.Body, m.gate.maxBodyBytes)
if err != nil {
if isRequestBodyTooLarge(err) {
recordRequestRejected(r.Context(), rejectReasonOversize)
http.Error(w, "request body too large", http.StatusRequestEntityTooLarge)
return
}
http.Error(w, "bad request", http.StatusBadRequest) // <-- no recordRequestRejected
return
}The else branch (line 40) is reached whenever readBoundedBody returns a non-oversize error — which happens when body == nil (readBoundedBody's own nil guard) or when the underlying io.ReadAll hits a genuine transport error (client disconnects mid-upload, TCP reset, etc.). In that branch, the handler responds with HTTP 400 but skips the recordRequestRejected call entirely.
Why this stands out. Every other rejection path in this same file records a reason before responding: the oversize path just above it (rejectReasonOversize), the ratelimiter.IsBodyTooLarge / parse-error path a few lines below (rejectReasonUnparseable), and the rate-limited path (rejectReasonRateLimited). The read-error 400 is the sole exception, so it is the only rejection type invisible in the evmrpc_requests_rejected_total OTel counter that this same PR introduces and instruments for every sibling branch.
Step-by-step proof.
- A client opens a POST to
:8545, starts streaming a JSON-RPC body, then drops the TCP connection before finishing (or sendsContent-Lengththat promises more bytes than actually arrive). readBoundedBody'sio.ReadAll(lr)returns a non-EOF read error from the underlying connection.readBoundedBodyreturns(nil, err)witherrbeing that transport error — noterrBodyTooLargeand not an*http.MaxBytesError, soisRequestBodyTooLarge(err)is false.- Control falls to the plain
http.Error(w, "bad request", http.StatusBadRequest)branch, and the request is rejected without ever touchingrecordRequestRejected. - On a dashboard built from
evmrpc_requests_rejected_total{reason=...}, this rejection simply doesn't appear under any reason label, while a functionally similar oversize or malformed-body rejection would show up underoversizeorunparseable.
Why nothing else catches this. The PR's own test suite (rate_limit_middleware_test.go) has TestRateLimitMiddleware_ParseErrorRejected and TestRateLimitMiddleware_ParseErrorRecordsRejectedMetric, but both exercise the malformed-JSON path through ratelimiter.MethodParser (a distinct branch, further down, that does call recordRequestRejected(rejectReasonUnparseable)). There is no test that forces readBoundedBody itself to return a non-oversize error (e.g. via a reader that errors mid-stream), so the gap in this specific branch isn't exercised.
The fix is small: call recordRequestRejected(r.Context(), rejectReasonUnparseable) (or introduce a dedicated rejectReasonReadError constant, which would be more precise since these are genuinely transport-layer failures rather than parse failures) immediately before the http.Error(w, "bad request", ...) call at line 40, mirroring the pattern used by every sibling branch in the same function.
On severity. This is a real, confirmed inconsistency, but it is purely an observability/metrics gap — the request is still correctly rejected with the correct HTTP 400 status; only the metric emission is missing. No incorrect behavior, crash, or data loss results, and the path is dominated by client-side transport failures (disconnects mid-body) rather than server-driven admission decisions. It doesn't block or delay this PR; it's a good follow-up nit, and multiple independent reviewer bots flagged the same line as [nit]/[suggestion] rather than a blocker.
There was a problem hiding this comment.
Solid, well-tested Phase 1b wiring of the fail-closed rate limiter into the EVM JSON-RPC HTTP path; it ships disabled by default and I found no correctness regressions. The notable gaps are hardening/observability: oversize and unreadable bodies (and non-POST) are never charged to the per-IP bucket, one rejection path records no metric, and one 413 branch is unreachable/untested.
Findings: 0 blocking | 16 non-blocking | 9 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes: Codex reported "No material issues found in the reviewed diff." The Cursor review file (
cursor-review.md) is empty — that pass produced no output, so it contributed nothing to this synthesis. - Once enabled, a request's
"method"is now extracted three times:MethodParserin the gate, the full parse insei_legacy_http.go, and go-ethereum's own decode.ratelimiter.skipValue(unchanged by this PR, but newly on the hot path) doesdec.Decode(&json.RawMessage), which copies every skipped value — a 5 MiBeth_sendRawTransactionbody allocates a full 5 MiB copy just to be skipped. AToken()-based depth-counting skip would allocate nothing. Worth measuring before flippingrate_limiting_enabledtotruefleet-wide; the PR already lists unification as a follow-up. - Client-visible behavior change when enabled: malformed bodies, duplicate
"method"keys, and empty-body POSTs now get a plain-text HTTP 400/413 instead of a JSON-RPC error object. ethers/web3 surface that as an opaque transport error rather than an RPC error. This is the intended fail-closed design, but it belongs in release notes — and worth confirming no LB/health check POSTs an empty body to :8545. - The 429 response sets no
Retry-Afterheader. Adding one (derived from RPS/burst) lets well-behaved clients back off instead of hot-retrying. - No test exercises the gate through the real
NewEVMHTTPServer/EnableRPChandler chain — every middleware test constructsnewRateLimitMiddlewaredirectly. A single end-to-end test asserting a 429 through the assembled stack would catch a future refactor that dropsconfig.rateLimitGatefromEnableRPC(the field is unexported, so nothing else would fail). ratelimiter.Newallocates the 50k-entry expirable LRU unconditionally inNewEVMHTTPServer, even whenrate_limiting_enabled = false(the default for every node). Trivial memory, but easy to skip.- Test hygiene:
evmrpc/rate_limit_middleware_test.goandratelimiter/registry_test.goboth overwrite package-level metric globals (metrics.requestRejectedCount,registryMetrics.rejectedCounter) without restoring them, so those counters keep writing into the test's ManualReader for the remainder of the package's tests. Restore them int.Cleanupalongside the meter provider. - 9 suggestion(s)/nit(s) flagged inline on specific lines.
| body, err := readBoundedBody(r.Body, m.gate.maxBodyBytes) | ||
| if err != nil { | ||
| if isRequestBodyTooLarge(err) { | ||
| recordRequestRejected(r.Context(), rejectReasonOversize) |
There was a problem hiding this comment.
[suggestion] Oversize rejections (and the non-POST bypass at line 27) never charge the per-IP bucket, so they're a free-of-charge path past the rate limiter.
Concretely: a chunked request (Content-Length: -1) with a 5 MiB+1 body sails past requestSizeLimiter's header check, reserves maxBody from the max_concurrent_request_bytes semaphore for the whole read, gets fully read off the socket here, then 413s — with zero tokens consumed. Repeat concurrently and ~26 such requests exhaust the default 128 MiB budget, so legitimate traffic gets server busy 429s while the attacker's bucket stays full.
Check already charges MethodInvalid for parse errors for exactly this reason; doing the same before the 413 (and ideally for non-POST) would close the gap. Not a regression versus today's behavior, so non-blocking, but it undercuts the stated fail-closed goal.
| http.Error(w, "request body too large", http.StatusRequestEntityTooLarge) | ||
| return | ||
| } | ||
| http.Error(w, "bad request", http.StatusBadRequest) |
There was a problem hiding this comment.
[nit] This branch is the only rejection path in the middleware that doesn't call recordRequestRejected, so read failures are invisible in evmrpc_requests_rejected_total. It also reports a client that aborted mid-body (or a connection reset) as bad request, which is misleading in logs/dashboards. Consider a distinct reason value (e.g. read_error) so these are separable from genuinely malformed JSON.
|
|
||
| allowed, rejectMethod, checkErr := m.gate.Check(r.Context(), ip, bytes.NewReader(body)) | ||
| if checkErr != nil { | ||
| if ratelimiter.IsBodyTooLarge(checkErr) { |
There was a problem hiding this comment.
[nit] This branch is unreachable. readBoundedBody above guarantees len(body) <= m.gate.maxBodyBytes, and NewRateLimitGate constructs the parser with that same limit, so Parse's LimitedReader (N = limit+1) can never hit lr.N <= 0 and never returns ErrProbeLimit from this call site.
Worth noting TestRateLimitMiddleware_ProbeLimitRejected413 doesn't cover it either — with maxBodyBytes = 64 and a ~75-byte body, the 413 comes from readBoundedBody/errBodyTooLarge, not from the parser. Either drop this branch or add a comment marking it as defensive-only, so a future reader doesn't assume it's live and tested.
| } | ||
|
|
||
| ip := m.gate.registry.IPFromHTTPRequest(r) | ||
| body, err := readBoundedBody(r.Body, m.gate.maxBodyBytes) |
There was a problem hiding this comment.
[suggestion] io.ReadAll starts at a 512-byte buffer and grows by repeated realloc+copy, so a 5 MiB body costs ~13 reallocations and roughly 2x peak transient bytes — on every POST, once this is enabled. r.ContentLength is known here and already validated <= maxBody by the outer requestSizeLimiter, so pre-sizing (bytes.Buffer + Grow(min(ContentLength, maxBytes)), falling back to the current path for ContentLength < 0) removes that churn from the hot path.
| return false, "", parseErr | ||
| } | ||
|
|
||
| for _, method := range methods { |
There was a problem hiding this comment.
[suggestion] batch_request_limit isn't consulted at this layer. readBatchMethods has no element cap, so a 5 MiB body of [{"method":"a"},...] produces ~350k parsed strings (plus a json.RawMessage copy per skipped value) before a single token is checked — and the request is then rejected downstream anyway for exceeding the 1000-element batch limit.
The token loop itself is self-bounding (it breaks at the first denial, so at most burst+1 Allow calls), so this is allocation waste rather than a lock-contention problem. Bailing out of the parse once the element count exceeds batch_request_limit would make the gate reject early and cheaply instead of doing the full parse for a request that can't succeed.
| httpConfig.batchResponseSizeLimit = config.BatchResponseMaxSize | ||
| httpConfig.maxRequestBodyBytes = config.MaxRequestBodyBytes | ||
| httpConfig.maxConcurrentRequestBytes = config.MaxConcurrentRequestBytes | ||
| rateLimitRegistry, err := ratelimiter.New(config.RateLimiterConfig()) |
There was a problem hiding this comment.
[suggestion] This is the biggest operational footgun in the PR: trusted_proxy_cidrs defaults to empty (trust none), so for any node behind an LB/ingress IPFromHTTPRequest returns the proxy's address and every client collapses into one shared bucket. An operator who flips rate_limiting_enabled = true without also setting the CIDRs silently caps the whole node at 200 RPS aggregate and 429s most legitimate traffic.
The TOML comment documents it, but a startup logger.Warn here when RateLimitingEnabled && len(TrustedProxyCIDRs) == 0 would make it hard to miss during the staging/canary rollout.
| IPRateLimitRPS: 200, | ||
| IPRateLimitBurst: 400, | ||
| BatchRequestLimit: 1000, | ||
| IPRateLimitBurst: defaultBatchRequestLimit, |
There was a problem hiding this comment.
[nit] Aliasing the burst default to defaultBatchRequestLimit raises the per-IP burst 400 → 1000 (a 2.5x increase in the allowed non-batch burst, not just batch headroom) and couples the two defaults permanently: a future change to batch_request_limit would silently move ip_rate_limit_burst with it. Two separate constants, with the invariant enforced only by the validation at line 693, keeps the intent explicit.
| return cfg, fmt.Errorf("%s must be >= 0 (0 disables the limit), got %d", flagMaxOpenConnections, cfg.MaxOpenConnections) | ||
| } | ||
| } | ||
| if cfg.RateLimitingEnabled && cfg.IPRateLimitBurst > 0 && cfg.BatchRequestLimit > 0 && |
There was a problem hiding this comment.
[nit] Good hard failure with an actionable message. One note on charging model: with burst == batch_request_limit, a single full-size batch drains the entire bucket, and at RPS 200 that client is throttled for ~5s before the next batch. If large batches are a normal client pattern on mainnet, consider charging one token per request (or a sub-linear weight per batch) rather than one per element — otherwise batching, which is cheaper for the node than N separate requests, is penalized harder than not batching.
| // suitable for OTel/Prometheus metrics. Attacker-controlled method strings | ||
| // collapse to rpcMethodBucketOther. | ||
| func bucketRPCMethod(method string) string { | ||
| if method == MethodInvalid { |
There was a problem hiding this comment.
[nit] MethodInvalid is the plain string "invalid", and this early return means a client that literally sends "method":"invalid" lands in the same method_namespace="invalid" bucket as genuinely unparseable bodies — so the metric can't distinguish "malformed request" from "weird but well-formed method name". A sentinel that can't collide with a real JSON-RPC method (e.g. "<invalid>") removes the ambiguity.
2 participants
Summary
Implements Phase 1b-core of the RPC endpoint protection work: wires
ratelimiter.Registry+ratelimiter.MethodParser(from PLT-800 / #3755) into the EVM JSON-RPC HTTP admission path (:8545). WebSocket (:8546) is out of scope — tracked as 1b-ws follow-up.Also hardens
MethodParserand its HTTP wiring to a fail-closed contract (no fallback on oversize or parse failure).Rate limiting
ratelimiter.Allownow takes amethodargument; rejections recordrpc_rate_limit_rejected_total{plane,method}with low-cardinality namespace bucketing (ratelimiter/method_bucket.go).evmrpc/config):rate_limiting_enabled(defaultfalse— ships dark; operators opt in per node) andtrusted_proxy_cidrs(default empty / trust-none). AConfig.RateLimiterConfig()helper builds theratelimiter.Configused by admission.rate_limiting_enabled = true(and configuretrusted_proxy_cidrsif behind a proxy/LB) to enable it. Flipping the default totruefleet-wide is a follow-up once we've validated behavior in staging/canary.max_request_body_bytes(default 5 MiB) — no separate probe-bytes knob. One cap for size limiting, rate-limit parsing, and go-ethereum's HTTP body limit.rateLimitMiddleware(evmrpc/rate_limit_middleware.go) +RateLimitGate(evmrpc/rate_limit.go):requestSizeLimiter → rateLimitMiddleware → wrapSeiLegacyHTTP → NewHTTPHandlerStackmax_request_body_bytes), extracts JSON-RPC method name(s) viaMethodParser, applies the per-IP token bucket (method is passed through only to label the rejection metric — see per-method note below), then replays the buffered body to downstream handlers."request body too large"— body exceeds cap (rejectReasonOversize)"bad request"— malformed JSON-RPC / duplicate"method"/ other parse errors (rejectReasonUnparseable)"too many requests"— per-IP bucket exhausted (rejectReasonRateLimited)*ratelimiter.Registryis constructed perHTTPServerinstance inNewEVMHTTPServer.MethodParser hardening (
ratelimiter/)doc.go) documents the fail-closed contract."method"keys →ErrDuplicateMethod(reject, not charge a cheap method while dispatching an expensive one).ErrProbeLimitmeans body exceeds cap; callers reject (HTTP 413), never fall back to full decode.IsBodyTooLarge(err)helper for HTTP mapping.Out of scope / follow-ups
:8546) wiring — tracked as 1b-ws.:26657) wiring — tracked as 1c.sei_legacy_http.go's existing full-body parse (a single request is currently parsed for"method"twice by two independent code paths).ratelimiter.Registryis keyed on IP only (getOrCreate(ip));methodis threaded throughAllowsolely to label therpc_rate_limit_rejected_total{method_namespace}metric. A cheapeth_chainIdspammer and aneth_getLogsabuser from the same IP currently share one bucket — there's no per-method or per-class rate/limit split yet.MethodParsergives us the pre-decode method name needed to do that, but the policy itself is Phase 4 (method class taxonomy + per-class semaphores,4a/4bin the work breakdown), gated behind Phase 2 (deadline enforcement) soaking in prod first.Test plan
ratelimiter:Allowwith method label on OTel counter; duplicate/mixed-case method rejection; body-over-limit →ErrProbeLimitevmrpc: allow under limit, burst then 429, per-IP isolation, batch counts all methods, whole-batch reject, oversize → 413, parse-error → 400, disabled bypass, non-POST passthrough, body drained on rejectionrate_limiting_enabled/trusted_proxy_cidrsparse correctly;RateLimiterConfig()helpergo test ./ratelimiter/... ./evmrpc/...