Add per-IP rate limiting to CometBFT RPC HTTP - #3911
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3911 +/- ##
==========================================
- Coverage 59.48% 58.32% -1.17%
==========================================
Files 2325 2227 -98
Lines 198660 186707 -11953
==========================================
- Hits 118180 108889 -9291
+ Misses 69240 67529 -1711
+ Partials 11240 10289 -951
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Use kebab-case [rpc] keys so viper reads operator config.toml, align CheckURI parse failures with CheckPOST (400 vs 429), and stop inspect from panicking on invalid trusted-proxy CIDRs when rate limiting is enabled. Co-authored-by: Cursor <cursoragent@cursor.com>
POST / admission failures that clients must decode now respond with JSON-RPC error objects (including HTTP 429 on throttle) instead of plain text. Add coverage for oversize bodies, malformed JSON, burst validation, and GET / method catalog passthrough. Co-authored-by: Cursor <cursoragent@cursor.com>
amir-deris
changed the title
PR SummaryMedium Risk Overview The gate uses the shared
Reviewed by Cursor Bugbot for commit ff787ad. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5dd7838. Configure here.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The per-IP admission gate is well-tested and correctly fail-closed on malformed bodies, but as written it rejects URI-style POST requests (a supported CometBFT access pattern) with 400, silently converts max-body-bytes = 0 (unlimited) into a 1 MB 413 limit while draining oversize bodies unbounded, and leaves a free unmetered path via empty-body / and OPTIONS. Config validation also lets the master switch be enabled with zero/negative rates, which disables throttling without any signal.
Findings: 3 blocking | 13 non-blocking | 11 posted inline
Blockers
- None at the file/PR level.
- 3 blocking issue(s) flagged inline on specific lines.
Non-blocking
- The Cursor second-opinion file (
cursor-review.md) is empty — that pass produced no output, so this review merges only Claude's and Codex's findings. - Client-visible hardening not called out in the PR description:
MethodParseris fail-closed, so bodies CometBFT previously tolerated now get a single HTTP 400 instead of per-element JSON-RPC errors — missingmethod, duplicatemethodkeys, empty batch[], and trailing data after the top-level value. Worth a release note for operators enabling the gate. - Metric usefulness on the new plane is limited:
bucketRPCMethodonly recognizes EVM-style namespaces, so almost every CometBFT method (status,block,broadcast_tx_sync, and all path-derived URI names) collapses tomethod_namespace="other". Onlyabci_*maps to a real namespace. Consider adding the CometBFT method names/namespaces toknownRPCNamespaces, or the metric will not tell an operator what is being throttled. - Test gaps worth closing: (1) URI-style POST (
POST /status?height=1, form-encoded POST) through the gate; (2)max-body-bytes = 0(unlimited) config, which is where the gate's own 1 MB limit and the unbounded drain bite; (3) X-Forwarded-For / trusted-proxy IP resolution end-to-end through the middleware (currently only covered inside theratelimiterpackage); (4) a batch larger thanburst, whichAllowNrejects unconditionally. RateLimitGate.enabledis alwaystrueat both production call sites, soNewRateLimitMiddleware's nil check and everyif !g.enabledearly return express the same "off" state twice. Per AGENTS.md ("guard at the choke point"), a nil gate is the single choke point; dropping the flag removes three dead branches and one redundant way to misconfigure the gate in future callers.- 8 suggestion(s)/nit(s) flagged inline on specific lines.
…led. Non-positive max-body-bytes must stay unlimited like MaxBytesHandler. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@seidroid review |
There was a problem hiding this comment.
This revision closes the earlier URI-routing, max-body-bytes = 0, unbounded-drain, catalog-exemption, per-listener-registry and inspect fail-open findings, and the CORS/gate ordering is now correct in both env.go and inspect. One unmetered path remains: every OPTIONS request bypasses the gate while CometBFT's mux serves RPC on any HTTP verb, so the limiter is bypassable by changing the method.
Findings: 1 blocking | 5 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Config validation still accepts negative
ip-rate-limit-rps/ip-rate-limit-burstwhilerate-limiting-enabled = true, andRegistry.Allowtreats non-positive values as "always allow". Noting the author's decision to match evmrpc, the practical footgun is the zero case: an operator addingrate-limiting-enabled = trueto a pre-existingconfig.tomlwith none of the new keys getsRPS = 0, Burst = 0from Viper, passesValidateBasic, and runs with the middleware active but no throttling at all — no error, no log line. A startup warning when the switch is on and the bucket is disabled would cost nothing. - Rejection metrics are near-useless on this plane:
bucketRPCMethodonly recognizes EVM-style namespaces, sostatus,block,broadcast_tx_sync,catalog, and every path-derived URI name collapse tomethod_namespace="other"(onlyabci_*maps). Adding the CometBFT namespaces toknownRPCNamespaceswould let an operator see what is actually being throttled. - Test gaps worth closing: (1) X-Forwarded-For / trusted-proxy IP resolution end-to-end through the middleware (currently only covered inside
ratelimiter); (2) a JSON-RPC batch larger thanburst, whichAllowNrejects unconditionally — withRequestBatchSizeLimit = 10and burst ≥ 10 enforced this is safe today, but nothing pins it; (3) gate enabled withip-rate-limit-rps = 0, the config where admission runs but nothing throttles. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| // isCometBFTRateLimitExemptRequest reports requests that should bypass the gate. | ||
| func isCometBFTRateLimitExemptRequest(r *http.Request) bool { | ||
| return r.Method == http.MethodOptions |
There was a problem hiding this comment.
[blocker] This exempts every OPTIONS request on every path, and CometBFT does not restrict routes by HTTP method: rpc_func.go:28 registers mux.HandleFunc("/"+name, ...) and makeHTTPHandler never checks req.Method — it just calls parseURLParams (which reads query and form params) and dispatches. Path / is the same: handleInvalidJSONRPCPaths only compares the path.
So with the gate enabled, OPTIONS /status?height=1, OPTIONS /block?height=1, OPTIONS /broadcast_tx_sync?tx=0x..., and OPTIONS / with a JSON-RPC body are all fully served without consuming a token. Changing the verb defeats the whole feature.
Nothing else catches these: CORS is disabled by default (CORSAllowedOrigins empty), and when it is enabled cors.Handler now wraps the gate from the outside (env.go:409, inspect/rpc/rpc.go:88) and terminates genuine preflights — those with Access-Control-Request-Method — before they ever reach this middleware. A bare OPTIONS /status has no such header and passes straight through rs/cors to the mux.
The exemption is therefore unnecessary for preflight and only creates the bypass. Drop it and let OPTIONS fall through to the catalog/URI/POST branches (or charge it a token under a fixed label). TestRateLimitMiddleware_OPTIONSExempt currently pins the bypass and should be inverted.
| } | ||
| switch r.Method { | ||
| case http.MethodGet, http.MethodHead, http.MethodPost: | ||
| return r.ContentLength == 0 |
There was a problem hiding this comment.
[suggestion] Keying "is this the catalog page" on ContentLength == 0 makes the behavior transport-dependent. A body-less request sent with chunked transfer-encoding (or any request where the length is unknown) has ContentLength == -1, so it falls to the JSON-RPC branch, MethodParser.Parse fails on the empty stream, and the client gets HTTP 400 — where makeJSONRPCHandler previously served the endpoint list for len(b) == 0. Same for verbs outside this switch (PUT/DELETE on / with an empty body), which the base handler also treated as catalog requests.
Deciding after the body is read removes the dependence on the framing: on path /, if the buffered body is empty, charge the catalog bucket and forward; otherwise parse it as JSON-RPC. That is also the same signal the inner handler uses, so the two can't disagree. (Codex flags the chunked case as well.)
| // RateLimitGate applies per-IP token-bucket rate limiting for CometBFT RPC HTTP | ||
| // requests. POST JSON-RPC bodies are parsed with MethodParser before full decode; | ||
| // GET URI routes are accounted by path-derived method names. | ||
| type RateLimitGate struct { |
There was a problem hiding this comment.
[suggestion] This file is a near-verbatim copy of evmrpc/rate_limit.go: the struct fields, chargeAdmissionRejection, and CheckPOST (vs. Check) are identical line for line, differing only in the maxBodyBytes <= 0 fallback and the plane string. The fail-closed charging rule — parse error charges MethodInvalid, and an exhausted bucket converts the parse error into a rate-limit rejection — is the load-bearing invariant of the whole design, and it now exists in two places that must be kept in sync by hand.
Both packages already import ratelimiter; hoisting the shared gate there (with plane and body-limit policy as parameters) would make it one invariant instead of a convention. Related: enabled is true at both production call sites (env.go:397, inspect/rpc/rpc.go:84), and NewRateLimitMiddleware already returns inner unchanged for a nil gate — so a nil gate is the single choke point and the flag adds three dead if !g.enabled branches plus a second way to misconfigure the gate. Same point as the earlier review; still applies.
1 participant
Summary
Adds optional per-IP rate-limit admission middleware to CometBFT RPC HTTP (
:26657)./— JSON-RPC bodies are parsed for method names before dispatch; oversize/malformed requests are rejected at the gate (HTTP 413/400). Rate-limit rejections return JSON-RPC error objects with HTTP 429 so clients can decode them normally./status,/websockethandshake) — charged by path-derived method name. Empty-body GET/HEAD/POST on/bypass the gate so the RPC method catalog page keeps working.GET /websocket) is rate-limited (methodwebsocket). Frames after upgrade are not covered by this middleware.trusted-proxy-cidrslogs an error and disables the gate instead of panicking.New
[rpc]config keys:ip-rate-limit-rps0disables throttlingip-rate-limit-burstrate-limiting-enabledtrusted-proxy-cidrsX-Forwarded-Forheaders are trusted for client IP resolutionTest plan
go test ./sei-tendermint/rpc/jsonrpc/server/ -run TestRateLimitgo test ./sei-tendermint/config/ -run TestRPCConfig< 10ValidateBasic rejection/method catalog passthrough and HTML catalog through registered routes