fix(node): reject non-http transaction relay URLs - #336
Conversation
There was a problem hiding this comment.
Disclosure: I'm not affiliated with Circle — an external community contributor, not a maintainer. I have no write access to this repository, so the APPROVED state on this review carries no merge authority and is advisory only. Please treat it as one contributor's technical assessment and defer to Circle maintainers for the binding review.
Reviewed at 3eb2185 against base 97f8da0. I verified the underlying gap in detail on #335, so this focuses on the patch itself. Source review only — no Rust toolchain in my environment, so I traced types and call paths rather than running cargo nextest.
The fix is correct and the scheme predicate has no hole. Since rust-url and Node's URL both implement the WHATWG URL Standard, I ran the awkward inputs through equivalent parser semantics. The two that look like holes aren't: http:///tmp/upstream collapses the extra slash to host tmp, and http:a:8545 normalizes to http://a:8545/ — both are genuine HTTP URLs reqwest can drive, so accepting them is right. Empty-host (http://) and out-of-range-port (http://a:99999) never reach the scheme check because Url::parse rejects them first. Scheme is lowercased during parsing, so HTTPS://... is not falsely rejected. matches!(url.scheme(), "http" | "https") is exactly the right predicate.
It is non-breaking against the existing suite. I checked each sibling test: the default-empty, trim/skip-blanks, not-a-url, and --rpc.forwarder conflict tests are all unaffected, and the modified order test still asserts ordering. Because validation happens after clap, the check also covers values supplied via ARC_TX_RELAYS, not just the flag — worth stating explicitly since the env path is documented in docs/tx-forwarding.md and is easy to overlook.
And the new test will actually run: .github/workflows/ci.yml:91 executes cargo nextest run --locked --workspace --exclude arc-test-integration, which covers this crate. That's not true of every test directory in this repo, so it's worth noting the coverage lands where it counts.
The one thing I'd want resolved before merge is a changelog entry, not a code change
This converts a configuration that currently starts into one that aborts at startup. A node with, say, --arc.tx.relays http://good:8545,ws://stale.example runs today — the bad entry just fails at forward time and failover skips past it — and after this change refuses to boot. Fail-fast is the right call, but it is an operator-visible behaviour change rather than a pure bug fix.
The repo already has a settled convention for precisely this class of change. CHANGELOG.md v0.8.0 carries:
[Config] Explicit invalid CL environment values now fail startup. … malformed values and zero values for settings that must be positive abort startup.
…under ### For Node Operators, with a matching BREAKING_CHANGES.md section. This PR is the same shape and currently adds no changelog entry at all.
That said, I checked the blast radius and it is genuinely small, which is why I'm approving rather than requesting changes. --arc.tx.relays does not exist in v0.6.0, v0.7.1, v0.7.2, or v0.7.3 — it first appears in v0.8.0, the current release. So the entire population of affected operators is those who adopted a brand-new flag and pointed it at a non-HTTP URL that has never worked. A ### Fixes line is probably proportionate; I'd only escalate to ### For Node Operators + BREAKING_CHANGES.md if maintainers read the v0.8.0 adoption as non-trivial.
This recency actually argues for merging sooner rather than later: the tightening is cheapest now, while almost nobody could be relying on the lenient behaviour.
Three non-blocking suggestions
1. State the constraint where operators will look for it. The clap doc comment for arc_tx_relays (crates/node/src/main.rs:297) describes ordering and failover but says nothing about permitted schemes, so --help won't explain the new rejection. One line — "Each URL must use HTTP or HTTPS." — closes the loop between the error and the documentation. docs/tx-forwarding.md:83 has the same gap; its examples are all https://, which implies the rule without stating it.
2. Pin the mixed valid/invalid case. Nothing asserts what http://good:8545,file:///bad does. Because the closure collects into eyre::Result<Vec<String>>, collect() short-circuits on the first Err and the whole list is rejected rather than the bad entry being dropped and the good one kept. That is the correct behaviour and it is the single thing distinguishing fail-fast from silent-skip — but a future refactor toward leniency would keep every test in this PR green while reversing the intent.
3. Update the contract comment in TxRelays::new. crates/evm-node/src/rpc_middleware.rs:507 still reads "URLs are pre-validated at the CLI boundary; any that fail to parse here are skipped." After this PR the guarantee is stronger — parses and is HTTP(S). That comment is the written contract between the two layers and is what a future reader will trust instead of re-deriving the CLI validation, so leaving it stale is the most likely route to this gap reappearing. Worth noting the inner filter_map silently drops unparseable URLs and (!clients.is_empty()).then(...) returns None if all are dropped — meaning the layer below still degrades quietly. This PR makes that unreachable from the CLI path; the comment should say so.
Clean, minimal, correctly scoped patch with a test that lands in CI — approving on the code. The changelog line is the only thing I'd genuinely like to see added.
|
Nice fix. I found two small follow-up points that would make the regression coverage and contract clearer:
These are small, but they would better lock in the intended fail-fast behavior and prevent the old silent-skip semantics from creeping back in. |
@huklaa — agreed on both, and they're worth doing. For @Kewe63's benefit: these are the same two items as suggestions #2 and #3 in my review above from earlier today, so the outstanding list hasn't grown — it's still those two, plus the One correction on the framing, because it changes what the mixed test is actually for. There were never silent-skip semantics at the CLI boundary
// crates/node/src/main.rs:397-408 (base)
.map(|s| {
url::Url::parse(s)
.map(|_| s.to_string())
.map_err(|e| eyre::eyre!("invalid --arc.tx.relays entry {s:?}: {e}"))
})
.collect() // -> eyre::Result<Vec<String>>, short-circuits on first ErrCollecting into The silent-skip you're thinking of is real, but it lives one layer down in // crates/evm-node/src/rpc_middleware.rs:520-530
.filter_map(|u| match reqwest::Url::parse(u) {
Ok(url) => Some(RpcClient::new_http_with_client(http.clone(), url)),
Err(e) => { tracing::warn!(...); None } // <- drops
})
...
(!clients.is_empty()).then(|| Self { ... }) // <- None = relaying silently disabledTwo layers, opposite semantics. Practical consequence: the mixed valid/invalid test can't be regression coverage in the usual sense — there's no lost behaviour to restore, and anyone who goes looking for one in On the doc comment — the stale clause is the second one, not the firstThe comment at
This PR strengthens the first (now: parses and is HTTP/HTTPS). The second stays literally true as a description of the code — I checked the reachability: The changelog entry is still the one thing I'd want before mergeNeither of these two points covers it, so flagging that it hasn't been addressed. My approval stands — none of this is a code-correctness objection. Same caveat as before: no |
3 participants
Summary
Fixes #335
--arc.tx.relaysentryhttpandhttpsrelay URLsfile,ftp,ws, andwssURLs during startupProblem
build_tx_relays()previously validated relay entries using only:url::Url::parse(s)
This verifies URL syntax but not compatibility with the downstream transport.
As a result, configurations such as:
--arc.tx.relays file:///tmp/upstream
were accepted and returned as valid relay entries.
The downstream implementation constructs each relay with an explicitly HTTP-based client:
RpcClient::new_http_with_client(...)
using
reqwest::Client. Unsupported schemes therefore failed only when transaction forwarding was attempted, rather than being rejected during startup validation.Changes
build_tx_relays()now:httpandhttps;invalid --arc.tx.relays entryerror for unsupported schemes;No forwarding, failover, timeout, or RPC client behavior was changed.
Regression Coverage
The existing relay-order test now verifies a mixed HTTP/HTTPS list:
http://a:8545,https://b.example
A new regression test verifies that the following schemes are rejected:
file:///tmp/upstream
ftp://relay.example
ws://relay.example
wss://relay.example
The existing tests for malformed URLs, trimming, blank entries, relay order, timeout parsing, and
--rpc.forwarderconflicts remain in place.RED Verification
Before the production fix, the unsupported-scheme regression test failed:
transaction relay upstreams must use HTTP or HTTPS:
["file:///tmp/upstream"]
test tests::test_build_tx_relays_rejects_non_http_urls ... FAILED
The HTTP/HTTPS acceptance test continued to pass.
How to Test
cargo +1.94.0 test -p arc-node-execution transaction_relay -- --nocapture
cargo +1.94.0 test -p arc-node-execution --bin arc-node-execution --tests
cargo clippy -p arc-node-execution --bin arc-node-execution --tests -- -D warnings
cargo fmt -p arc-node-execution -- --check
git diff --check
Results:
Note: The final linked test binary was built with reduced linker memory usage because the standard test link exceeded the available WSL memory:
CARGO_BUILD_JOBS=1 cargo +1.94.0 rustc
-p arc-node-execution
--bin arc-node-execution
--profile test
--
-C debuginfo=0
-C link-arg=-Wl,--no-keep-memory
Local verification used Rust 1.94.0 because the local pinned 1.93.0 installation is currently broken. CI should provide the authoritative pinned-toolchain result.
Scope and Risk
Risk is low.
The change is limited to the CLI configuration boundary in:
crates/node/src/main.rs
It does not alter:
Checklist
Impact
Type: 🐛 Bug fix
Fixes: #335