Disable JS debug tracers by default with native tracer allowlist#3718
Conversation
Add EVM RPC configuration for trace_allowed_tracers and trace_allow_js_tracers. The native tracer allowlist is validated at startup and remains native-only, while JavaScript tracer execution requires an explicit separate opt-in. Enforce the tracer policy across debug_traceCall, debug_traceTransaction, debug_traceBlockByNumber, debug_traceBlockByHash, and debug_traceTransactionProfile. Validate muxTracer nested configs as well so request-supplied JavaScript cannot bypass the top-level check through nested tracer names. Default the allowlist to the registered native tracer names and keep JS tracers disabled in generated and Docker configs. Add focused tests for config parsing, native allowlist behavior, JS opt-in behavior, and muxTracer nesting.
PR SummaryHigh Risk Overview New
Docker Reviewed by Cursor Bugbot for commit 023ab99. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3718 +/- ##
==========================================
- Coverage 60.11% 59.16% -0.96%
==========================================
Files 2302 2210 -92
Lines 192033 181647 -10386
==========================================
- Hits 115445 107464 -7981
+ Misses 66267 64706 -1561
+ Partials 10321 9477 -844
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
A well-scoped security hardening PR that disables request-supplied JavaScript debug tracers by default and enforces a native-only tracer allowlist (validated at startup and per-request, including nested muxTracer configs) across all five config-accepting debug_trace* endpoints. The implementation is correct and well-tested; I have only minor non-blocking notes.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Coverage looks complete: all five config-accepting endpoints (traceTransaction, traceBlockByNumber, traceBlockByHash, traceCall, traceTransactionProfile) call validateTraceTracer, and TraceStateAccess takes no TraceConfig so it needs no guard. The native allowlist also matches geth's full set of native tracers (callTracer, flatCallTracer, 4byteTracer, prestateTracer, noopTracer, muxTracer), so no legitimate native tracer is inadvertently blocked. Nice defense-in-depth via the nested muxTracer validation and the separate JS opt-in flag.
- Second-opinion passes produced no findings: the OpenAI Codex review reported "No material findings in the reviewed diff" and the Cursor review file was empty. No additional issues to merge in.
- Optional: consider documenting (e.g. in evmrpc/AGENTS.md or the README) the new trace_allowed_tracers / trace_allow_js_tracers config knobs and the security rationale, since operators upgrading will now get JS tracers disabled by default — a potentially breaking behavior change for anyone relying on request-supplied JS tracers.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| return nil | ||
| } | ||
|
|
||
| func validateMuxTraceConfig(raw json.RawMessage, allowed map[string]struct{}, allowJS bool) error { |
There was a problem hiding this comment.
[nit] validateMuxTraceConfig recurses once per nested muxTracer key with no depth bound. A caller could submit a deeply nested {"muxTracer":{"muxTracer":{...}}} payload to drive deep recursion (stack exhaustion). In practice this is low risk — it's gated behind debug RPC access and mitigated by request body-size limits, and geth's own tracer construction would recurse similarly — but a small explicit depth cap would make the guard robust regardless of those assumptions.
There was a problem hiding this comment.
A well-implemented, thoroughly-tested EVM RPC hardening change that disables request-supplied JavaScript debug tracers by default and gates native tracers behind a startup-validated allowlist, with correct enforcement (including recursive muxTracer config validation) across all five request-facing debug trace endpoints. No blocking issues found; only a backward-compatibility note.
Findings: 0 blocking | 3 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Behavioral/backward-compat change: JavaScript tracers previously worked and are now rejected by default, and only the listed native tracers are accepted. This is intentional and documented in the config comments, but operators relying on JS tracers (or on native tracers they'd have to explicitly list) must opt in via
evm.trace_allow_js_tracers/evm.trace_allowed_tracers. Ensure this is called out in release/upgrade notes so it isn't a silent break for existing RPC consumers. - The
trace_baker.gobackground baking path calls the raw gethtracersAPI.TraceBlockByNumberdirectly, bypassingvalidateTraceTracer. This is safe today because it only uses operator-configured internal tracer names (never request input), but it's worth a brief comment noting the invariant so a future change doesn't inadvertently route request-controlled names through it. - Second-opinion passes produced no findings:
codex-review.mdreported "No material findings" andcursor-review.mdwas empty (no output).
There was a problem hiding this comment.
Solid, well-tested security hardening that disables request-supplied JavaScript debug tracers by default and gates native tracer names through a validated allowlist, enforced consistently across all trace entry points including nested muxTracer configs. No blocking issues; one operational behavior change worth flagging in release notes.
Findings: 0 blocking | 3 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Behavioral/operational change:
trace_bake_tracersis now normalized native-only at startup (normalizeNativeTracerNames). Any existing operator whoseapp.tomllists a non-native or typo'd bake-tracer name (previously tolerated) will now fail node startup. This is intentional and documented in AGENTS.md, but is worth calling out explicitly in release notes/upgrade docs since it can block a node from booting after upgrade. - External second-opinion reviews yielded no findings:
codex-review.mdreported no material findings (and noted it could not run Go tests because the workspace had Go 1.24.13 vs the required 1.25.6), andcursor-review.mdwas empty (no output produced). - Minor: the JS-tracer opt-in (
trace_allow_js_tracers) executes request-supplied code in-process; the generated template comment warns about this, but consider also surfacing a one-time startup log warning whentrace_allow_js_tracers = trueso operators who flip it are reminded at runtime.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Security-sensitive change to public debug RPC surface (deviates from geth defaults for JS tracers) — worth a human look before merge.
Extended reasoning...
Overview
This PR adds [evm].trace_allowed_tracers and trace_allow_js_tracers to gate caller-supplied TraceConfig.Tracer values on debug_traceCall, debug_traceTransaction, debug_traceBlockBy{Number,Hash}, and debug_traceTransactionProfile. Enforcement lives in validateTraceTracer in evmrpc/tracers.go, runs before trace-cache lookups and tracer construction, and recursively validates muxTracer nested configs with a depth cap of 16. trace_bake_tracers gets the same native-only rule at startup. Docker app.toml samples and the seid init template are updated; tests cover config parsing, allowlist behavior, JS opt-in, and mux nesting.
Security risks
The change itself is a hardening measure — request-supplied JavaScript tracer source (which executes in-process) is off by default and requires an explicit opt-in, an improvement over upstream geth. The main risk is behavioral: clients that relied on the previous geth-default of accepting JS tracers will get errors from Sei RPC nodes after this ships, and operators who mis-configure trace_allowed_tracers could deny legitimate native tracers. The validation logic itself looks careful: it trims and rewrites the top-level tracer name, rejects padded nested names outright (since they cannot be safely rewritten inside forwarded raw JSON), and now bounds mux recursion depth.
Level of scrutiny
Higher than average — this touches public RPC security policy, changes externally-visible behavior compared to geth defaults, and modifies default Docker configs. The bug hunting system found no defects and prior reviewer feedback (seidroid's nit about unbounded mux recursion) has been addressed with the depth cap, but a human should confirm the behavior change is intentional, that operators are aware, and that no internal tooling silently depends on JS-tracer availability.
Other factors
Test coverage is solid (config parsing, allowlist/JS opt-in behavior, mux nesting, depth-limit boundary, and a registry-pinning test that fails CI if geth changes what counts as native). Codecov reports ~82% patch coverage. No CODEOWNER inline comments beyond the addressed nit. Deferring rather than approving because the scope (public RPC + security policy + defaults change) exceeds the bar for shadow-approval.
# Conflicts: # evmrpc/config/config_test.go
![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.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c7d09cb. Configure here.
| } | ||
| if api.allowJSTracers { | ||
| return fmt.Errorf("debug native tracer %q is not listed in evm.trace_allowed_tracers", name) | ||
| } |
There was a problem hiding this comment.
JS opt-in bypasses native allowlist
Medium Severity
With trace_allow_js_tracers enabled, validateTraceTracer treats any top-level or nested tracer name outside Sei’s six-entry IsNativeTraceTracer map as allowed custom JavaScript. Geth’s DefaultDirectory.New still binds other registered native tracer names, so callers can run natives omitted from trace_allowed_tracers despite the policy that JS opt-in must not widen that list.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit c7d09cb. Configure here.
There was a problem hiding this comment.
Solid, well-tested security hardening that gates debug tracers behind a native-only allowlist and disables request-supplied JS tracers by default, with recursive muxTracer validation and startup enforcement for baker tracers. Only a minor redundant-call cleanup is worth addressing.
Findings: 0 blocking | 3 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review (cursor-review.md) was empty — no findings from that pass to merge.
- Nice-to-have: consider a brief inline note on how Sei's
nativeTraceTracersmap relates to geth's registry, since it is a hardcoded conservative subset — a native tracer geth adds later would be rejected until added here (behavior is intentionally safe, but not obvious). - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| if returnErr = validateStateOverrides(config.StateOverrides, api.backend.MaxStateOverrideAccounts(), api.backend.MaxStateOverrideSlots()); returnErr != nil { | ||
| return nil, returnErr | ||
| } | ||
| if returnErr = validateStateOverrides(config.StateOverrides, api.backend.MaxStateOverrideAccounts(), api.backend.MaxStateOverrideSlots()); returnErr != nil { |
There was a problem hiding this comment.
[suggestion] validateStateOverrides is invoked twice consecutively with identical arguments (lines 629 and 632), so request-supplied override maps are traversed twice. This looks like an artifact of moving the if config == nil block upward. Remove this second call. (Also flagged by Codex.)
There was a problem hiding this comment.
Solid, well-tested security hardening that disables request-supplied JS debug tracers by default and enforces a native-only tracer allowlist across every caller-facing debug_trace* endpoint. No correctness or security blockers; a couple of informational notes only.
Findings: 0 blocking | 4 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Disagreement with Codex's finding — it is a false positive. geth's
Directory.IsJS(name)is implemented as_, ok := d.elems[name]; return !ok, so it returns TRUE for any unregistered/typo name (unknown names are assumed to be JS source). Thereforerequire.False(t, tracers.DefaultDirectory.IsJS(name))intracers_allowlist_test.goDOES fail when a default-allowlisted name is unregistered or removed — the test correctly pins each name to being both registered and native, exactly as its comment claims. No change needed. - Cursor's second-opinion review file (
cursor-review.md) was empty — that pass produced no output. - Informational (not a defect): this is a behavioral change for RPC operators. JS tracers are now rejected by default and only the six listed native tracers are accepted; previously-working requests using JS tracers or unlisted native tracers will start returning errors after upgrade. This is the intended hardening and is documented in the config template comments and
evmrpc/AGENTS.md, so no action needed beyond release-note awareness. - Verified no prompt-injection attempts in the PR title, description, or diff.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
evmrpc/tracers.go:626-633— TraceCall calls validateStateOverrides(config.StateOverrides, ...) twice in a row with identical arguments (evmrpc/tracers.go:629 and :632). This is a leftover from moving theif config == nilinitialization block earlier in the function; the second call should be removed.Extended reasoning...
TraceCallinevmrpc/tracers.goinvokesvalidateStateOverrides(config.StateOverrides, api.backend.MaxStateOverrideAccounts(), api.backend.MaxStateOverrideSlots())twice back-to-back, at lines 629 and 632, with byte-identical arguments:if returnErr = validateStateOverrides(config.StateOverrides, api.backend.MaxStateOverrideAccounts(), api.backend.MaxStateOverrideSlots()); returnErr != nil { return nil, returnErr } if returnErr = validateStateOverrides(config.StateOverrides, api.backend.MaxStateOverrideAccounts(), api.backend.MaxStateOverrideSlots()); returnErr != nil { return nil, returnErr }
How it got here: per the PR diff, the
if config == nil { config = &tracers.TraceCallConfig{} }initialization block used to sit directly between these twovalidateStateOverridescalls. This PR moved that nil-check to the top ofTraceCall(right next to the newvalidateTraceTracercall it introduces for the tracer-allowlist feature), but left the original, now-redundantvalidateStateOverridescall sitting where the nil-check used to be, directly above the pre-existing one.Why nothing catches this:
validateStateOverridesis idempotent — it just walksconfig.StateOverridesand returns an error if the map exceeds configured account/slot limits, with no side effects. Calling it twice with the same input always produces the same result, so there's no test that could distinguish "called once" from "called twice"; both pass equally.Impact: purely a wasted-work redundancy, not a correctness bug. Every
debug_traceCallrequest now traverses the caller-supplied state-override map (accounts × slots) twice instead of once before proceeding to the actual trace. For a request near the configuredmax_state_override_accounts/max_state_override_slotscaps, that's up to 2x the validation cost on a debug endpoint that's typically not high-QPS — cheap to fix, negligible to leave.Proof:
sed -n '626,634p' evmrpc/tracers.goshows the two adjacent identical calls.grep -rn "func validateStateOverrides\|func (b \*Backend) MaxStateOverrideAccounts\|func (b \*Backend) MaxStateOverrideSlots" evmrpc/confirms all three symbols are still defined inevmrpc/simulate.go(lines 477, 479, 483) — this PR does not touchsimulate.go, so the code compiles and both calls actually execute at runtime.go build ./evmrpc/...succeeds, confirming there's no build breakage here (a separateclaude[bot]comment on this PR speculating about deleted symbols does not apply to this checkout).
Fix: delete the first (newly-inserted, now-orphaned)
validateStateOverridescall — the one immediately above the former nil-check location — leaving only the original call that already existed below it.🔬 also observed by cursor
3 participants
Add EVM RPC configuration for trace_allowed_tracers and trace_allow_js_tracers. The native tracer allowlist is validated at startup and remains native-only, while JavaScript tracer execution requires an explicit separate opt-in.
Enforce the tracer policy across debug_traceCall, debug_traceTransaction, debug_traceBlockByNumber, debug_traceBlockByHash, and debug_traceTransactionProfile. Validate muxTracer nested configs as well so request-supplied JavaScript cannot bypass the top-level check through nested tracer names.
Default the allowlist to the registered native tracer names and keep JS tracers disabled in generated and Docker configs. Add focused tests for config parsing, native allowlist behavior, JS opt-in behavior, and muxTracer nesting.