SyncEndpointUrl accepts endpoints that later panic or fail Display round-tripping

[dumanoglu1]

Summary

SyncEndpointUrl accepts HTTP endpoints that can later panic in websocket() or lose information when formatted and reparsed.

Cases

Maximum explicit HTTP port panics

let endpoint: SyncEndpointUrl = "http://localhost:65535".parse()?;
let _ = endpoint.websocket();

Parsing succeeds, but deriving the WebSocket URL executes http_port.checked_add(1).expect("port overflow") and panics. Invalid derived-port combinations should be rejected during parsing instead of failing later at runtime.

HTTP path and query are lost by Display

let endpoint: SyncEndpointUrl =
    "https://rpc.example.com/api/v1?key=value,wss=ws.example.com/websocket".parse()?;
let reparsed: SyncEndpointUrl = endpoint.to_string().parse()?;
assert_eq!(endpoint, reparsed);

Display reconstructs the HTTP side from only scheme, host, and port, so /api/v1?key=value is discarded. This violates the round-trip property already exercised by the module's tests.

IPv6 hosts are emitted without brackets

For an endpoint such as http://[::1]:8545,ws=8546, host_str() returns ::1 and Display writes it directly into the authority. The resulting string is not a valid bracketed IPv6 URL and cannot be reparsed reliably.

Expected behavior

• accepted endpoint values should not panic when websocket() is called
• parse -> Display -> parse should preserve the endpoint
• IPv6 authorities should remain valid and bracketed

Proposed fix

Validate the derived WebSocket port during parsing and use URL-aware serialization so the HTTP path/query/fragment and IPv6 host syntax are preserved. Add focused regression tests for all three cases while retaining current canonical output for existing IPv4/default-port inputs.

[osr21]

Assessed against main and the url version pinned in Cargo.lock (2.5.8). Source review only — there's no Rust toolchain in my environment, so nothing below was executed; it's read from the repo source and the url crate's published source.

Two of the three cases are real. The third isn't, and PR #330 has already built a fix for it. There are also two further cases of exactly the same class that the issue misses — one of which breaks the round-trip property for the majority of real configs, including the endpoint this repo ships as a default.

Case 1 — port 65535: confirmed, and the reasoning is stronger than stated

Accurate, and you quoted the right expression. Worth adding why it's a clean crash rather than something worse: checked_add(1).expect("port overflow") is an unconditional panic, independent of build profile. This workspace's [profile.release] sets lto, opt-level, codegen-units, and strip but not overflow-checks — so had that line been a plain http_port + 1, release builds would have silently wrapped to port 0 and produced a follower quietly dialling the wrong port. Whoever wrote checked_add already saw this; the bug is that the check fires at call time instead of parse time.

Blast radius is narrow — it needs RPC-sync/follow mode plus an explicitly configured :65535 endpoint — but it's the only case here that can take a node down, which is worth separating from the rest.

Case 2 — path/query lost by Display: confirmed

Correct as written. Display rebuilt the HTTP side from scheme + host + port only, so /api/v1?key=value was genuinely discarded.

Case 3 — IPv6 brackets: I believe this one is incorrect

The claim is that host_str() returns ::1 and Display writes it unbracketed. url 2.5.8's own documentation for Url::host_str states the opposite:

Return the string representation of the host (domain or IP address) for this URL, if any. […] IPv6 addresses are given between [ and ] brackets.

The same holds one level down: Host's Display impl in host.rs explicitly writes "[", then write_ipv6(addr), then "]". So the existing code should already emit http://[::1]:8545 and the authority should already be valid.

This matters because #330 implemented a fix for it — a host_for_display helper that swaps url's WHATWG serializer for std's Ipv6Addr Display. Those differ on IPv4-mapped addresses (::ffff:127.0.0.1 vs ::ffff:7f00:1), so the remedy for a non-existent canonicalisation bug introduces a real one. I've left the detail on that PR.

Quickest way to settle it definitively: add http://[::1]:8545,ws=8546 as a Display test against unmodified main. If it already passes, the case can be struck.

Missing case A — userinfo is silently dropped

Same class as case 2, and more likely to be hit in practice than IPv6. validate_http_scheme only checks the scheme, so credentials parse fine, but Display never emits them:

let endpoint: SyncEndpointUrl = "http://user:pass@localhost:8545,ws=8546".parse()?;
// Display -> "http://localhost:8545,ws=8546"   (user:pass@ gone)
let reparsed: SyncEndpointUrl = endpoint.to_string().parse()?;
assert_eq!(endpoint, reparsed); // fails

Basic-auth RPC endpoints are a common shape for hosted providers, so this is a realistic follow endpoint. Note the flip side: for a value that gets logged, dropping credentials is arguably the desirable behaviour — which is a good argument for deciding deliberately whether Display is a serialisation format or a log format, rather than letting it be an accident of which components got hand-copied.

Missing case B — the derived-vs-explicit distinction is erased, so round-trip fails for most configs

This is the one I'd rank highest, because it defeats the issue's own stated expectation in the common case.

Display writes the ,<scheme>= segment unconditionally, even when ws is None. Since PartialEq is derived over { http: Url, ws: Option<Url> }, None != Some(_):

let endpoint: SyncEndpointUrl = "http://localhost:8545".parse()?;  // ws: None
// Display -> "http://localhost:8545,ws=8546"
let reparsed: SyncEndpointUrl = endpoint.to_string().parse()?;     // ws: Some(ws://localhost:8546/)
assert_eq!(endpoint, reparsed); // fails

So parse -> Display -> parse is not identity for any endpoint without an explicit WS override. That includes both values in default_rpc_sync_endpoint (config.rs:198) — http://localhost:8545 for localdev and https://rpc.testnet.arc.io/ for testnet. The shipped defaults fail the invariant.

The reason nobody noticed: every existing round-trip assertion uses an input that already carries an explicit override (,wss=rpc.testnet.example.com/websocket), and #330's two new ones do too (,wss=ws.example.com/websocket and ,ws=8546). display_http_only and display_https_only cover the derived case but only assert the output string — they never reparse. The invariant is untested in exactly the place it breaks.

In fairness this is semantically benign: ws: None and ws: Some(derived) yield identical websocket() results, and Display is idempotent from its first output onward. But it's structurally unequal, which is what the issue asks for.

On the proposed fix — your instinct was better than the implementation

The issue proposes "use URL-aware serialization," which is the right call and would have closed cases 2, 3, and A in one stroke: self.http.as_str() already emits userinfo, path, query, fragment, and bracketed canonical IPv6, because Url is its own serialiser. #330 instead hand-assembles the components, which is why it needed a bespoke IPv6 helper and still drops userinfo.

A complete fix is roughly two decisions:

1. HTTP side: emit self.http.as_str() rather than rebuilding it. Real cost to weigh: as_str() omits default ports, so https://rpc.example.com:443/... becomes https://rpc.example.com/.... Both reparse equal, but several existing assertions pin the :443 form and would need updating.
2. WS side: emit the ,<scheme>= segment only when self.ws.is_some(). That fixes case B and makes the round-trip genuinely identity.

Both change Display's canonical output, so they're a deliberate format decision rather than a patch — which is easier to justify given the point below.

Severity calibration

Worth stating plainly, since the issue frames all three cases together: SyncEndpointUrl::Display appears to have no production consumer. The connection path uses the typed accessors (peers.rs:54-58 calls url.http() and url.websocket()); the %endpoint tracing field in rpc_sync/client.rs:136 is a &Url, not this type (client.rs:86, :94, :204); and follow_endpoints is #[serde(skip)], so Display isn't used to persist or reload config. Configuration enters exclusively through FromStr, which was never broken.

That makes case 1 a genuine crash fix and cases 2/A/B a latent-trap cleanup — worth doing, since a Display/FromStr pair that loses data will eventually bite whoever first serialises one, but not a live production defect today.