fix(audit): close 9 leak/DoS findings from 2026-05-13 security review#30
Merged
Conversation
CRITICAL - graphql: sanitize sqlx errors (block/blocks/transaction); log full error internally, return generic "internal server error" to client. - /metrics: split off main router; bind on 127.0.0.1:9080 by default (INDEXER_API_METRICS_BIND configurable). Public Caddy proxy can no longer expose Prometheus metrics. HIGH - etherscan: sanitize sqlx errors in txlist + getblocknobytime envelopes; log internally, return "database error". - readyz: static "down: pg error" / "down: cache error" on the wire; details to tracing. - coinblast worker: cap known_non_curves at 10_000 entries with arbitrary eviction over cap. Stops unbounded HashSet growth on chains with many spurious Buy/Sell-topic-shaped emitters. - etherscan txlist: reject startblock/endblock/page when non-default rather than silently ignoring (returned wrong-window results). MEDIUM - sqlx workspace dep: default-features = false + explicit feature list; drops `any` so resolver no longer pulls sqlx-mysql via macros-core during compilation. cargo audit RUSTSEC-2023-0071 (rsa Marvin) ignored with a justification — sqlx-macros-core still lists sqlx-mysql as an optional dep so it stays in Cargo.lock even though never compiled. - BlockHeight::as_u64 returns Option<u64> instead of panicking on negative. Sync uses BlockHeight(-1) as cursor sentinel; previously a future caller could panic-kill the worker. Provider/block_writer call sites updated to handle None explicitly. - bin/api.rs: documented the CorsLayer::permissive() assumption (no-auth public read API); flag for revisit if mandatory auth is added.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR hardens the indexer API and processing layers: it sanitizes database and cache errors to generic client responses while logging details, moves the metrics endpoint to a dedicated loopback-only listener with a new metrics_router, makes BlockHeight::as_u64() fallible and updates callers to validate heights, bounds an in-memory cache in coinblast, and constrains SQLx features while adding RUSTSEC-2023-0071 to advisory ignores. Sequence Diagram(s)sequenceDiagram
participant ApiServer
participant TcpListener
participant MetricsRouter
participant Client
ApiServer->>TcpListener: bind(INDEXER_API_METRICS_BIND) (loopback-only)
Client->>TcpListener: connect / request /metrics
TcpListener->>MetricsRouter: dispatch request
MetricsRouter->>Client: metrics response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/api.rs`:
- Around line 46-53: Parse and validate the indexer_api_metrics_bind value as a
SocketAddr early (the config field indexer_api_metrics_bind /
default_metrics_bind and the code that spawns the Prometheus metrics listener)
and reject any non-loopback addresses before creating the listener: use
std::net::SocketAddr::from_str (or to_socket_addrs) to parse the bind string,
check socket.ip().is_loopback(), and return an error or disable metrics if
false; ensure the validation happens at config/initialization time (before the
listener spawn) and surface a clear error mentioning INDEXER_API_METRICS_BIND so
callers cannot bind 0.0.0.0 or any external interface.
In `@crates/api/src/lib.rs`:
- Around line 121-126: The code currently drops RouterConfig.metrics_handle
silently (let _ = cfg.metrics_handle;) which hides an important breaking change;
update make_router (or the compatibility shim handling RouterConfig) to detect
when cfg.metrics_handle is Some(_) and either log a clear warning via the
existing logger or return/raise an error (fail fast) indicating /metrics is no
longer exposed, referencing RouterConfig.metrics_handle and metrics_router so
callers see the explicit behavior change; ensure the chosen behavior is
consistent with project conventions (use process or crate logger if available,
or return Err from make_router) so callers are not silently downgraded.
In `@crates/api/src/routes/etherscan.rs`:
- Around line 88-91: The return statement that builds the unsupported-param
error is formatted in a way that fails rustfmt; update the expression that uses
format! and unsupported.join to a single, properly formatted call (e.g. ensure
the join uses ", " and the format! call is on one line) so the return
err(format!(...)) expression in etherscan.rs compiles with cargo fmt --check;
locate the use of err, format!, and unsupported.join and reformat that return
accordingly.
In `@crates/sync/src/block_writer.rs`:
- Line 67: The single long chained call for the block_height field
(block_height: b.block.height.as_u64().expect("written block heights are
non-negative"),) exceeds rustfmt line length; break the chain across lines or
introduce a temporary variable. For example, compute let height =
b.block.height.as_u64().expect("written block heights are non-negative"); then
use block_height: height, or place the call on multiple lines like
b.block.height.as_u64() .expect("written block heights are non-negative"), to
satisfy cargo fmt --check in block_writer.rs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 00dc29b9-94b1-4b57-8334-1b758a99363b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (12)
.github/workflows/cargo-audit.ymlCargo.tomlbin/api.rscrates/api/src/graphql.rscrates/api/src/lib.rscrates/api/src/routes/etherscan.rscrates/api/src/routes/readyz.rscrates/chain/src/provider.rscrates/coinblast/src/worker.rscrates/domain/src/ids.rscrates/sync/src/block_writer.rsdeny.toml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/sync/src/block_writer.rs`:
- Around line 67-71: The current code uses b.block.height.as_u64().expect(...)
which can panic; change it to a non-panicking, best-effort extraction (e.g.,
map_or / unwrap_or / match on as_u64()) and substitute a safe fallback value
(like 0 or None) instead of panicking, and emit a warning/metric via the same
observability path (logger/telemetry) when the height cannot be converted;
update the block_height assignment (the expression starting with
b.block.height.as_u64()) to perform this graceful handling so analytics remain
non-fatal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 99bf51fa-e4c0-4f79-8e5b-ea1ed215cde3
📒 Files selected for processing (2)
crates/api/src/routes/etherscan.rscrates/sync/src/block_writer.rs
CR review on PR #30: - bin/api.rs: parse INDEXER_API_METRICS_BIND at startup; bail if not loopback (was a path back to the unauth-public-metrics exposure this PR was meant to close). - crates/api/src/lib.rs: tracing::warn when caller still passes RouterConfig.metrics_handle so the silent-drop is visible in logs.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/api.rs`:
- Line 133: The code currently calls tokio::net::TcpListener::bind(&bind).await
which re-parses the address string; use the already-parsed SocketAddr variable
(parsed) instead to avoid re-parsing and potential panics—replace the bind call
to pass parsed (the SocketAddr validated earlier) directly to TcpListener::bind
so it uses the existing parsed value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1b15f8c4-3b01-4a47-88d7-87acef25b565
📒 Files selected for processing (2)
bin/api.rscrates/api/src/lib.rs
| } | ||
| let mh = metrics_handle.clone(); | ||
| tokio::spawn(async move { | ||
| match tokio::net::TcpListener::bind(&bind).await { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Use the already-parsed SocketAddr instead of re-parsing the string.
parsed is already validated at lines 121-124. Passing it directly to TcpListener::bind() avoids re-parsing and is cleaner.
Suggested change
let mh = metrics_handle.clone();
+ let bind_addr = parsed;
tokio::spawn(async move {
- match tokio::net::TcpListener::bind(&bind).await {
+ match tokio::net::TcpListener::bind(bind_addr).await {
Ok(listener) => {
- tracing::info!(addr = %bind, "api: metrics listener up (loopback)");
+ tracing::info!(addr = %bind_addr, "api: metrics listener up (loopback)");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| match tokio::net::TcpListener::bind(&bind).await { | |
| let mh = metrics_handle.clone(); | |
| let bind_addr = parsed; | |
| tokio::spawn(async move { | |
| match tokio::net::TcpListener::bind(bind_addr).await { | |
| Ok(listener) => { | |
| tracing::info!(addr = %bind_addr, "api: metrics listener up (loopback)"); | |
| if let Err(e) = | |
| axum::serve(listener, metrics_router(mh).into_make_service()).await | |
| { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/api.rs` at line 133, The code currently calls
tokio::net::TcpListener::bind(&bind).await which re-parses the address string;
use the already-parsed SocketAddr variable (parsed) instead to avoid re-parsing
and potential panics—replace the bind call to pass parsed (the SocketAddr
validated earlier) directly to TcpListener::bind so it uses the existing parsed
value.
CR review on PR #30: - block_writer.rs: replace .expect() on block height conversion with match → warn-and-skip pattern. Analytics is best-effort already (comment line 62-63); don't escalate a never-hit-but-possible conversion miss to a worker panic.
CR-pushed clippy strict-warnings: collapsible_if on the LRU eviction guard. Use let-chain syntax so the .cloned() doesn't fire when set is under cap.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the CRITICAL + HIGH + MEDIUM findings from the 2026-05-13 security audit. LOW (request-id propagation, GraphQL depth limit) deferred.
CRITICAL
crates/api/src/graphql.rs:201-250— sanitize sqlx errors inblock/blocks/transactionresolvers via newsanitize_db_err()helper. Mirrors the REST-sideApiError::user_message()pattern; full error to tracing, generic"internal server error"to the client.crates/api/src/lib.rs:119-138+bin/api.rs:99-138—/metricsno longer merged into the public router. Newmetrics_router()export bound by the bin to a separate loopback listener (default127.0.0.1:9080, configurable viaINDEXER_API_METRICS_BIND). Public Caddy proxy can no longer expose Prometheus metrics.HIGH
crates/api/src/routes/etherscan.rs:90,141,144— sanitize sqlx errors intxlist+getblocknobytimeenvelopes via newdb_err()helper; tracing keeps the full error.crates/api/src/routes/readyz.rs:34,44— wire-side messages are static"down: pg error"/"down: cache error"; details go to tracing.crates/coinblast/src/worker.rs:59,189-204— capknown_non_curvesat 10_000 entries (KNOWN_NON_CURVES_MAX); arbitrary eviction over cap via newcache_non_curve()helper. Stops unbounded HashSet growth.crates/api/src/routes/etherscan.rs:35-92—txlistnow rejects non-defaultstartblock/endblock/pagewithstatus=0 message="param not supported: ..."rather than silently ignoring (correctness bug).MEDIUM
Cargo.toml:45— sqlx workspace dep set todefault-features = false+ explicit feature list.cargo tree -e features | grep sqlx-mysqlis empty post-change.RUSTSEC-2023-0071(rsa Marvin) ignored indeny.toml+.github/workflows/cargo-audit.ymlwith a justification:sqlx-macros-corestill listssqlx-mysqlas an optional dep so the resolver pins it inCargo.lockeven though no feature path enables compilation. Drop the ignore once sqlx upstream restructures.crates/domain/src/ids.rs:23—BlockHeight::as_u64()returnsOption<u64>instead ofpanic!on negative. Sync usesBlockHeight(-1)as cursor sentinel; a future caller could have killed the worker. Call sites incrates/sync/src/block_writer.rs+crates/chain/src/provider.rsupdated to either.expect("real chain height")(writer; receives a fully-resolved block) or surfaceChainError::InvalidArgument(provider).bin/api.rs:108-117— documented theCorsLayer::permissive()assumption (no-auth public read API). Flag to revisit if mandatory auth is added.Verification
RUSTFLAGS=\"-D warnings\" cargo build --release— passescargo test --workspace --no-fail-fast— all green (addedblock_height_sentinel_no_panictest)cargo audit --ignore RUSTSEC-2023-0071— clean (only pre-existing unmaintainedpaste/derivativewarnings)Test plan
INDEXER_API_METRICS_BIND=127.0.0.1:9080on staging — confirm/metricsserves on loopback only, public host returns 404.block(height: 999999999)with PG down — response contains"internal server error"not raw sqlx text./readyzwith PG down — body has"pg": "down: pg error", tracing log carries the full error.?module=account&action=txlist&address=0x...&startblock=100→status=0 message="param not supported: startblock".Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Chores