Skip to content

fix(util): harden rate limiting, ACLs and RPC cache correctness - #431

Merged
wanliqun merged 26 commits into
mainfrom
codex/util-remediation
Nov 4, 2026
Merged

wanliqun merged 26 commits into
mainfrom
codex/util-remediation

Conversation

@wanliqun

@wanliqun wanliqun commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Why

The util request-control and RPC cache paths could deadlock, bypass contract ACLs, expose API keys in logs, or return successful responses containing the wrong transaction or chain view. This PR fixes local request-control and cache-client defects and adds regression coverage, together with defensive nil-handling fixes. Near-head log stitching has boundary checks; cache-internal fixes remain deferred to data-cache.

Changes

The fixes are split into focused commits in dependency order:

  • Rate limiting and credentials: generate API keys with crypto/rand; remove the lock inversion between strategy reload and limiter creation through immutable strategy snapshots; preserve fractional rates; coalesce concurrent key lookups; remove credential-bearing error/debug fields. Reject invalid limiter parameters locally and return an error for null rules.
  • ACLs and client identity: validate contract addresses for filter creation and log subscriptions; reject constrained calls when parameters cannot be extracted; escape literal ACL patterns; normalize Core addresses consistently; parse and canonicalize IPv6 client addresses correctly.
  • RPC cache correctness: preserve transaction indexes above uint32 when falling back to the full node; preserve direct single-trace GetTrace cache requests using Conflux eSpace's first flat index, guarded against signed-index overflow and the data-cache protocol's uint32 narrowing; retain data-cache's sync.StartNearHead and cache-reader APIs; return complete near-head range hits directly and use the cached end hash to fence mixed-source reads, with a local consistency retry budget.
  • Queue and metric bounds: use nonblocking, atomic relay admission and copy queued transaction bytes; classify unknown method labels against registered callbacks even when routing fails early; register remote metrics under their formatted names.
  • Shared helpers: handle arrays safely in nil checks and missing signatures safely in legacy transaction checks.

Near-head caching remains enabled where configured. sync.StartNearHead owns poller/writer construction and adapter lifecycle. Confura cancels and waits for its started workers at shutdown. The local replacement readers, writer lock wrapper, and duplicated startup pipeline have been removed. This PR does not add adapter cleanup beyond the dependency's implementation.

Dependencies and scope

No third-party dependency versions are changed. go.mod and go.sum are identical to the base branch. This includes go-conflux-util v0.7.4 and the existing confura-data-cache revision. The application continues to use its original configuration initialization and decoder; no configuration compatibility package is added.

The production changes are confined to util; rpc/acl_boundary_test.go exercises the actual HTTP context injection and middleware chain. The package inventory remains 13 packages under util/....

Existing policy and deployment questions remain outside the confirmed fixes: missing/default SID or ACL behavior, revocation and stale-on-error policy, trusted-proxy deployment, global cache ownership across networks, end-to-end client cancellation and transport retry behavior, management endpoint exposure, and the unused gas estimator. Data-cache's internal fixes are deferred: atomic reorg updates and log/hash snapshots; the nested read-lock path in GetLogsByBlockHash; matching missing nonempty topic positions; shared Topics/Data buffers; and adapter cleanup. This PR uses the pinned cache readers unchanged and does not claim to fix these upstream issues.

Compatibility notes

  • trace_get follows the Conflux eSpace node implementation: empty indices returns null; otherwise only indices[0] selects an offset in trace_transaction, and further entries are ignored. It does not interpret the array as a traceAddress path. Both caches use their original single-trace GetTrace API. Near-head calls are skipped above MaxInt; data-cache calls are skipped above MaxUint32, retaining its single-result response and lazy decoding for supported indexes. Cache misses and skipped lookups fall through to the full node with the original index array. The audit's complete-path finding was invalidated by the node implementation.
  • The local util topic matcher preserves Conflux eSpace semantics: null and empty topic groups match even if the corresponding log topic is absent; nonempty groups require a matching topic at that position. This follows LogFilterParams::matches, reached by the eSpace filter conversion. The audit's topic-arity finding was invalidated by the node implementation. This local matcher does not replace data-cache's matcher; the pinned near-head reader can still incorrectly accept logs with missing nonempty topic positions.
  • Contract ACLs validate addresses when creating eth/cfx_newFilter filters and WebSocket log subscriptions. eth/cfx_getFilterLogs and eth/cfx_getFilterChanges retain their existing filter-ID access semantics: possession of a valid ID permits reads, subject to authentication, method ACLs, and rate limits. This PR does not bind IDs to identities or revalidate a filter against later contract-ACL changes.
  • Near-head coverage: pre-read lightweight cache metadata, then scan cache logs at most once per attempt. A complete range hit, including an empty result, returns the cache's current view without node header lookups or node availability checks. A miss delegates the complete resolved range to the node. Partial hits compare the cached end hash before and after the scan. Start eviction can extend the node prefix if the returned end remains aligned; end changes or disappearance discard the attempt. In the pinned dependency, local GetBlock(..., true) returns the cached object directly; false allocates a transaction-hash list. Metadata reads do not mutate that object. No replacement cache reader or writer is installed.
  • Mixed-source alignment: compare the node's canonical hash at the cached end with the captured cache hash before and after all missing node segments. This adds two node header RPCs per successful mixed attempt. The same end aligns both an older node prefix and a newer node suffix, assuming an internally coherent cache and node view. No separate start, parent-hash, or requested-upper-bound check is added. A reorg above the cached end alone does not invalidate the merge. This is optimistic alignment: independent calls cannot exclude ABA reorgs or internally inconsistent node responses. Existing getLogs/scanLogs handler fences are unchanged.
  • Consistency retries: the first observed cache/boundary conflict starts one local one-second retry admission window. Retries wait 10ms, rebuild the entire result, and keep the initially resolved numeric heights; the caller's filter is unchanged. The window only controls whether another attempt may start. An already-started attempt uses the client's original context and configured RPC timeouts, and may complete successfully after the window. No SDK client copy or SetContext override is used. Missing or mismatched boundary blocks are conflicts. Ordinary RPC and cache errors retain their original semantics, including errors from an attempt that finishes after the window. On consistency exhaustion, discard all logs and return exactly ErrInconsistentCanonicalView (inconsistent canonical view), with one Info log containing fixed diagnostic reasons, heights, attempt count, and elapsed time. Transient retries/recovery log at Debug. These new logs omit node URLs, credentials, and raw filters. The RPC request context is not passed through this cache-client API. The one-second window does not bound total wall time: admitted RPCs retain configured timeouts, and pinned provider sleeps/cache-lock waits are not cancellable. No detached timeout goroutine is introduced. Block-hash queries retain the dependency reader without this range retry loop.
  • Key encoding and type prefixes remain compatible. Existing keys are not rotated. KeyLoader retains its existing 60-second cache, negative cache, and stale-on-error behavior.
  • Invalid rate, capacity, or window values fail the affected limiter request rather than constructing an invalid limiter.

Request-path performance follow-up

Four additional commits reduce redundant work without changing public APIs, configuration, dependency versions, or ACL policy:

  • Metrics: classify methods using a shallow message copy with Params = nil; the pinned provider checks registration first. Preserve the -32601 distinction and subscription labels. Skip classification entirely when the downstream response is nil. Counting UnmarshalJSON fixtures confirm that successful dispatch decodes once, early routing failures decode zero times, and classification leaves the original message unchanged.
  • ACL: compile Origin and method wildcard rules when each validator is created. Reload still builds a new validator. Preserve matching order, case handling, allow/deny precedence, empty-list policy, and invalid-pattern behavior. Differential tests compare decisions against the former per-request regexp implementation, including invalid UTF-8; concurrent reads are race-tested.
  • Relay: reject an already-full buffered queue before copying transaction bytes. The final nonblocking select still decides admission atomically, and successful admissions retain owned bytes. Zero-capacity rendezvous behavior is unchanged. A producer that loses a concurrent admission race may still copy once; this is not a reservation mechanism.
  • Near-head diagnostics: check the enabled log level before building retry/recovery fields. Retain one final Info log on consistency exhaustion, Debug retry/recovery logs, and the existing retry window, fences, and public error text.

The full go test -mod=readonly ./... -count=1 suite and go test -mod=readonly -race ./util/... ./rpc ./rpc/handler ./virtualfilter -count=1 passed after these changes. Before implementation, the new metrics regressions reproduced duplicate decoding (two decodes on successful dispatch and one on an early routing failure), unnecessary classification for nil responses, and one transaction-copy allocation when the relay queue was full.

Microbenchmarks use Go 1.23.10 on darwin/arm64 (Apple M3 Pro), comparing cc69d043 with 7a6a8a74. Values below are medians of three 200ms runs using go test -mod=readonly ./util/rpc/middlewares ./util/acl ./util/relay -run '^$' -bench 'RegisteredMetricMethod|ACLValidation|RelayAdmission' -benchtime=200ms -count=3 -benchmem. Payload sizes denote decoded bytes; transaction and call inputs are hex-encoded JSON. These isolate the named operations and do not measure end-to-end RPC throughput.

Operation ns/op before → after B/op before → after allocs/op before → after
Metric classification, 128 KiB raw transaction 788,832 → 290 1,178,195 → 992 24 → 7
Metric classification, 128 KiB call input 1,687,080 → 281 1,178,755 → 992 27 → 7
ACL validation, 10 Origin and 10 method rules 57,645 → 1,181 127,465 → 0 1,330 → 0
Relay rejection, full queue, 128 KiB transaction 10,118 → 2.93 131,072 → 0 1 → 0
Relay admission, available queue, 32-byte transaction 38.37 → 41.56 32 → 32 1 → 1

ACL compilation cost moves to validator construction. The relay precheck adds a small cost to successful admission; the owned transaction copy remains required. Benchmark outputs and temporary evidence stay outside the repository.

Validation

All checks below use the original dependency versions and Go 1.23.10:

  • go test -mod=readonly ./... -count=1
  • go test -mod=readonly -race ./util/... ./rpc ./virtualfilter -count=1
  • Follow-up filter-read compatibility checks: go test -mod=readonly -race ./util/acl ./rpc -count=1. The new ETH/Core filter-ID regression fails against the previous validator and passes after removing the blanket rejection. The HTTP middleware test covers an allowed eth_newFilter followed by both read methods; Core creation tests still reject missing, disallowed, or mixed addresses.
  • CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -mod=readonly
  • Near-head fence/retry validation: race checks passed for ./util/..., ./rpc/handler, and ./virtualfilter; go test -mod=readonly -race ./rpc -count=1 then passed with local listening-port access for the HTTP/WebSocket fixtures. The full go test -mod=readonly ./... -count=1 suite was rerun successfully after the single-scan/retry-window changes. Regressions cover complete/empty cache hits with zero node RPCs; misses; left, right, and two-sided gaps; multiple cached blocks using only the end height; empty mixed results; reorgs during either node segment followed by a fresh successful attempt; retry admission expiry, acceptance of coherent late success, and preservation of late RPC errors; missing/mismatched boundary blocks; fixed latest heights; coverage changes between attempts; filter forwarding; and preservation of configured client contexts during concurrent requests. Controlled cache-read interleavings also cover one log scan per attempt, coverage expansion, start eviction, boundary disappearance, and reorgs before or after the scan. Final-error assertions check the exact public sentinel and one Info log without credential-bearing fields. Removing the post-read boundary check with a temporary Go overlay makes the controlled reorg tests fail on old-branch logs. These use a real in-process near-head cache and mocked RPC responses, not live-node integration. Upstream deadlock, topic matching, snapshot atomicity, and buffer ownership are not verified as fixed.
  • Follow-up trace compatibility checks: go test -mod=readonly -race ./util/rpc ./rpc/handler ./rpc -count=1. Node-derived fixtures exercise near-head, data-cache, and mocked fullnode responses; they fail against the complete-path implementation. The transaction-index regression remains covered. Direct-trace tests also assert zero bulk-trace reads and exercise MaxInt, MaxUint32, and overflowing indexes; removing the data-cache width guard reproduces a wrong trace at index zero.

Behavioral regressions cover controlled registry lock ordering; concurrent key misses and credential-bearing DB errors; HTTP, batch, and WebSocket ACL enforcement; large transaction indexes; flat trace indexes, ignored index suffixes, empty trace input, and out-of-range indexes; a branch switch during log stitching; missing required topic positions and optional wildcard positions in the local util matcher; concurrent producers with one remaining queue slot; and bounded classification of 200 untrusted method names.

The topic regression fixtures were checked against the matching function extracted from the pinned Conflux node source in a minimal Rust harness. Trace selection fixtures were also checked in a minimal Rust harness using the pinned node's selection logic with mocked transaction traces. These are source-level checks, not live-node integration tests.

Only production code, permanent regression tests, and benchmarks are included. Audit reports, temporary probes, logs, configuration backups, and generated binaries are excluded. Keep this PR ready for review, as requested; the four follow-up commits are published together.


This change is Reviewable

@wanliqun
wanliqun marked this pull request as ready for review November 4, 2026 07:13
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 4, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-04T07:15:23.563433Z cc69d04 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The metrics middleware redundantly decodes RPC arguments just to classify metric labels, adding unnecessary allocations and processing overhead, especially for large payloads.

1 open issue
What changed in this PR

This PR hardens Confura’s util request controls and RPC cache clients while preserving pinned dependencies and existing filter-ID access semantics. Cache-internal fixes remain deferred to data-cache.

Changes:

  • Strengthens rate limiting, credential handling, contract ACL checks, and client IP parsing.
  • Adds cache index guards and near-head log consistency checks with bounded retry admission.
  • Makes relay admission nonblocking, corrects metric registration, and adds regression coverage and nil-safety fixes.
File Description
util/​types.go Avoids invalid nil checks on arrays.
util/​rpc/​nearhead_test.go Tests log coverage, consistency checks, retries, and context preservation.
util/​rpc/​middlewares/​metrics.go Classifies metric labels using registered callbacks.
util/​rpc/​middlewares/​metrics_test.go Tests bounded labels for unknown methods.
util/​rpc/​middlewares/​allowlists.go Passes raw parameters to ACL validation.
util/​rpc/​handlers/​ip.go Canonicalizes IP addresses and includes range endpoints.
util/​rpc/​handlers/​ip_test.go Tests IPv6 parsing and forwarded-address handling.
util/​rpc/​cached_client_eth.go Adds cache index guards, log consistency retries, and worker shutdown waiting.
util/​rpc/​cached_client_eth_test.go Tests large indexes and Conflux trace selection semantics.
util/​relay/​relay.go Uses nonblocking queue admission and copies transaction bytes.
util/​relay/​relay_test.go Tests concurrent admission and queued-byte ownership.
util/​regex.go Escapes literal patterns without wildcards.
util/​rate/​util.go Generates limit keys with cryptographic randomness.
util/​rate/​util_test.go Tests key encoding and entropy failures.
util/​rate/​strategy.go Rejects null rules and replaces decoded options atomically.
util/​rate/​resolver.go Removes key details from strategy diagnostics.
util/​rate/​registry.go Uses strategy snapshots and validates limiter parameters.
util/​rate/​registry_reload.go Publishes strategy snapshots during reload changes.
util/​rate/​registry_concurrency_test.go Tests lock ordering, fractional rates, and invalid options.
util/​rate/​loader.go Coalesces key lookups and removes sensitive error fields.
util/​rate/​loader_test.go Tests lookup coalescing and credential-safe logging.
util/​metrics/​service/​registry.go Registers metrics under formatted names.
util/​metrics/​service/​registry_test.go Tests distinct formatted metric identities.
util/​boundary_test.go Tests topic matching and nil-handling boundaries.
util/​blockchain.go Guards missing signatures; clarifies wildcard semantics in a comment.
util/​acl/​validator.go Validates filter creation and log subscriptions; normalizes Core addresses.
util/​acl/​validator_boundary_test.go Tests ACL boundaries and filter-ID read compatibility.
rpc/​acl_boundary_test.go Exercises ACL enforcement through HTTP, batch, and WebSocket middleware.

Review effort: Balanced

Comment thread util/rpc/middlewares/metrics.go Resolved
@wanliqun
wanliqun merged commit fd29332 into main Nov 4, 2026
1 check passed
@wanliqun
wanliqun deleted the codex/util-remediation branch November 4, 2026 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants