fix(arc-consensus-types): validate derived WebSocket port and preserve URL components in Display

[Kewe63]

Summary

Refs #323.

This fixes SyncEndpointUrl cases that could either panic later when deriving the WebSocket URL or lose URL components during Display output and parse -> Display -> parse round-tripping.

---

Changes

• Reject HTTP URLs with explicit port 65535 when no WebSocket override is provided.
• Preserve HTTP path, query, and fragment components in Display output.
• Preserve WebSocket override path, query, and fragment components in Display output.
• Keep the existing compact WebSocket override output for same-host/no-path cases.
• Keep IPv6 display behavior covered without adding custom host formatting; url::Url::host_str() already emits bracketed IPv6 hosts.
• Add regression tests for:
  • derived WebSocket port overflow
  • HTTP and WebSocket path/query/fragment preservation
  • existing IPv6 display behavior
  • existing round-trip behavior with path-based WebSocket overrides

---

Why

Previously this parsed successfully:

http://localhost:65535/

but calling websocket() later would panic while deriving the WebSocket port through checked_add(1).expect("port overflow").

Display also rebuilt endpoint strings from selected components instead of preserving the full parsed URL shape. The HTTP side dropped values such as /api/v1?key=value, and the WebSocket override side dropped query and fragment components such as ?token=abc#fragment.

This moves the invalid derived-port case to parse-time validation and makes Display preserve the URL components currently covered by this PR. The broader Display serialization-vs-log-format decision and the userinfo / eager WebSocket derivation cases remain part of the wider #323 discussion.

---

Verification

Regression sabotage check:
Temporarily removed the new derived-port validation call and ran:

cargo +1.94.0 test -p arc-consensus-types parse_rejects_http_port_that_would_overflow_derived_websocket_port -- --nocapture

Result: FAILED as expected — called Result::unwrap_err() on an Ok value for http://localhost:65535/

Focused tests after restoring fix:

cargo +1.94.0 test -p arc-consensus-types rpc_sync -- --nocapture

Result: 24 passed; 0 failed

Formatting:

cargo +1.94.0 fmt -p arc-consensus-types --check

Result: passed

Lint:

CARGO_BUILD_JOBS=1 cargo +1.94.0 clippy -p arc-consensus-types --all-targets -- -D warnings

Result: passed

Whitespace:

git diff --check

Result: passed

---

Checklist

• Tests pass — 24/24, confirmed regression test fails without the derived-port validation
• cargo fmt / cargo clippy clean
• git diff --check clean
• Follows Conventional Commits
• Changes scoped to this partial fix only

---

Risk & Impact

Low. The port-65535 rejection only affects URLs that would already panic at websocket() call time when no explicit WebSocket override is provided; this fix moves that failure to parse time. The Display fix is additive: it preserves HTTP and WebSocket path/query/fragment components that were previously silently dropped, while keeping the compact output for the existing same-host/no-path WebSocket override case.

This PR intentionally uses Refs #323 rather than Refs #323 because #323 now also tracks broader follow-up decisions around userinfo handling, eager WebSocket derivation, and whether Display should be treated as serialization or log/debug formatting.

Refs: #323.

[osr21]

Reviewed at 2d1885b. Source review only — there's no Rust toolchain in my environment, so nothing below was executed. I verified against the repo source, the url version pinned in Cargo.lock (2.5.8), and that crate's published source.

Three changes here, and they are not equal in value. Two fix real bugs. The third fixes a bug I don't think exists.

1. Port 65535 — real, and the description understates it

Confirmed at rpc_sync.rs:70: websocket() derives the port via http_port.checked_add(1).expect("port overflow").

Because that's an explicit .expect() rather than raw +, it panics in every build profile. That's worth spelling out, because the obvious assumption is the opposite: this workspace's [profile.release] sets lto, opt-level, codegen-units, and strip, but not overflow-checks. Had the original been written http_port + 1, release builds would have silently wrapped to port 0 — a follower quietly dialling the wrong port instead of crashing.

It wasn't written that way. Whoever wrote checked_add already saw the overflow and chose to fail loudly; this PR completes the thought by moving the failure to parse time, where it belongs. Worth crediting in the description rather than presenting it as an unguarded panic.

2. Path / query / fragment in Display — real

Old Display rebuilt the HTTP side from scheme + host + port only, so /api/v1?key=value was genuinely dropped. Straightforwardly correct, and the test demonstrates it.

3. IPv6 brackets — I believe this is a phantom, and it introduces a small regression

The description states:

For IPv6 hosts, host_str() returned ::1 and Display emitted it without brackets, producing an invalid authority.

url 2.5.8's own documentation for Url::host_str says 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.

So the pre-fix Display already emitted http://[::1]:8545. Details inline on the relevant lines, including why the replacement is not merely redundant but slightly worse for IPv4-mapped addresses.

The tell is your own methodology. You ran a sabotage check on the port fix — removed the validation, confirmed the test failed for the right reason. That's genuinely good practice and more than most PRs do. It just wasn't applied to the other two changes, and the IPv6 test is precisely the one where it would have caught a no-op: that assertion passes identically with and without host_for_display.

Severity calibration: what actually depends on Display

Since the PR is framed around round-tripping, it's worth being precise about who consumes that string. As far as I can trace, SyncEndpointUrl::Display has no production caller:

• The connection path uses the typed accessors, not the string — peers.rs:54-58 calls url.http() and url.websocket() and stores real Url values.
• The %endpoint tracing field at rpc_sync/client.rs:136 is not this type; those functions take endpoint: &Url (client.rs:86, :94, :204).
• follow_endpoints is #[serde(skip)] in the CLI config, so Display is not used to persist or reload configuration.
• The follow_endpoints.join(",") in quake/src/info.rs:67 is quake's own string-typed manifest field, not this type.

That doesn't make change #2 wrong — a Display/FromStr pair that loses data is a latent trap, and fixing it while the invariant is cheap to hold is right. But it does mean #1 is a crash fix and #2/#3 are hygiene, whereas the PR presents all three as equivalent. Config parsing is the only external entry point, and it goes through FromStr, which was never broken.

Request changes — narrowly

To be clear about scope: I'd land #1 and #2 as-is. The single ask is to drop host_for_display and keep host_str() (or justify it if I've misread the crate — the sabotage check settles it in one run). I'm requesting changes rather than commenting because it's a concrete code removal plus a factual correction to the description, both of which are easier to fix before merge than after.

Noting for transparency: I don't have push access here, so this state is advisory and carries no merge-gating power.

[osr21]

This is the line I'd remove, for two independent reasons.

(a) It's redundant. Host's own Display impl in url 2.5.8 already brackets IPv6 — host.rs writes "[", then write_ipv6(addr), then "]". So the fallback arm on the next line (host => host.to_string()) would produce [::1] entirely on its own. And upstream of that, Url::host_str() — what the old code used — is documented as returning IPv6 hosts already bracketed. Both the old code and the simplified new code produce the same string.

(b) Where it isn't redundant, it's a regression. format!("[{addr}]") formats via std's Ipv6Addr Display, which special-cases IPv4-mapped addresses into dotted-quad form (::ffff:127.0.0.1). url's write_ipv6 is the WHATWG IPv6 serializer — pure longest-zero-run compression, no IPv4-mapped case — so it yields ::ffff:7f00:1.

For http://[::ffff:127.0.0.1]:8545, the parsed Url canonicalises the host to ::ffff:7f00:1, but this helper now prints ::ffff:127.0.0.1. Nothing fails: both forms reparse to the same Url, so PartialEq still holds and the round-trip test would still pass. But Display output no longer matches the URL's canonical serialisation — which is the exact class of mismatch this PR sets out to eliminate.

Simplest resolution is to delete the helper and keep self.http.host_str().expect("validated host"), which is already canonical and already bracketed. If you prefer going through the enum, url.host().expect("validated host").to_string() is equivalent and stays canonical — just without the hand-rolled Ipv6 arm.

[osr21]

This function is well placed and correctly scoped — gating on has_ws_override is right, since websocket() only derives a port when self.ws is None, so an explicit override genuinely makes port 65535 safe. Nice that you didn't over-reject.

One thing worth promoting into the PR description: this doesn't only move a panic earlier, and the pre-existing code deserves some credit. websocket() uses checked_add(1).expect("port overflow"), which is an unconditional panic — it does not depend on overflow-checks, which this workspace's [profile.release] doesn't enable. If that line had been a plain http_port + 1, release builds would have wrapped silently to port 0 and produced a follower dialling the wrong port rather than a crash. The existing checked_add is what made this a loud failure; your change makes it a validated one.

Minor: the message interpolates u16::MAX through '{}' when the rejected value is by definition 65535. Not worth a round-trip on its own, but if you touch this again, quoting the actual http.port() would make the error read more naturally alongside the URL that triggered it.

[osr21]

I believe this assertion passes both with and without the host_for_display change.

Url::host_str() in url 2.5.8 is documented to return IPv6 hosts already enclosed in [ and ], so the pre-fix Display — which used host_str() — should already have produced exactly http://[::1]:8545,ws=8546.

You already have the right tool for settling this: you sabotage-checked the port fix by removing the validation and confirming the test failed for the right reason. Doing the same here — revert host_for_display back to host_str(), re-run this test — takes one run and definitively confirms or refutes it. My read is that it will still pass, which would mean this test asserts pre-existing behaviour rather than the behaviour of the change.

Either way it's reasonable regression coverage and worth keeping; it just isn't evidence for the third change, and the PR description currently cites it as such.

[dumanoglu1]

Heads up from the issue author side: #323 has been updated after review.

The original IPv6 case was wrong and is now struck from the issue. The remaining scope is the :65535 derived-port panic plus the Display/round-trip cases: path/query/fragment loss, userinfo loss, and the derived-vs-explicit websocket distinction.

Since this PR currently says Fixes #323, it may be safer to either cover the updated scope or narrow the closing reference so the issue does not get auto-closed while the remaining round-trip cases are still open.

[osr21]

@dumanoglu1's concern is confirmed, not hypothetical — and while checking it I found a gap in this PR's own fix that sits inside its stated scope. Both below.

1. The auto-close link is live right now

Querying the PR's registered closing references returns exactly one, still open:

closingIssuesReferences: [ { number: 323, state: OPEN } ]

So merging this into the default branch will close #323, and — per the issue's rewrite — it would do so with the userinfo and derived-vs-explicit WebSocket cases still unaddressed, while landing the host_for_display helper built for the IPv6 case that has since been struck.

Changing Fixes #323 to Refs #323 or Part of #323 in the description clears it. One caveat worth knowing: closing links can also be attached through the Development sidebar in the UI, and those are independent of the body keyword — so if the reference survives editing the description, that's where the second one is.

2. The Display fix is asymmetric — the WebSocket side still drops query and fragment

This is new, and unlike the scope discussion it's a defect in this PR's code rather than a coordination question. The HTTP side now emits path, query, and fragment. The WebSocket side emits only the path:

let ws_path = ws_url.path();
// ... host / port ...
if has_path {
    write!(f, "{ws_path}")?;   // no ws_url.query(), no ws_url.fragment()
}

And a query is reachable, because parse_ws_override builds the override with a full Url::parse:

let ws_url = Url::parse(&format!("{scheme}://{value}"))

So the components survive parsing and are then discarded on output:

let endpoint: SyncEndpointUrl =
    "https://rpc.example.com,wss=ws.example.com/websocket?token=abc".parse()?;
// Display -> "https://rpc.example.com:443,wss=ws.example.com/websocket"   (?token=abc gone)
let reparsed: SyncEndpointUrl = endpoint.to_string().parse()?;
assert_eq!(endpoint, reparsed); // fails

Authenticated WebSocket endpoints commonly carry the credential as a query parameter, so this is a realistic shape rather than a contrived one — arguably more realistic than the IPv6 case that prompted the original change.

The reason it slipped through is narrow test selection: display_preserves_http_path_and_query uses ,wss=ws.example.com/websocket, which is path-only. Adding ?token=abc to that same test input is enough to expose it.

Worth fixing here regardless of how the scope question lands, since the PR's own description promises to preserve query and fragment and currently does so on only one of the two sides.

3. On covering the updated scope — I'd argue against it

Taking @dumanoglu1's two options seriously: I think narrowing is clearly the better one, for a reason that isn't obvious from the issue thread.

The issue's revised proposal is eager WebSocket derivation in FromStr (storing ws: Url rather than ws: Option<Url>). If that lands, validate_derived_ws_port is deleted — the checked_add failure moves into parsing on its own, and the has_ws_override flag becomes unnecessary because an explicit override simply skips derivation. Expanding this PR to the full scope therefore means removing its own central mechanism.

So the cheapest sequencing looks like:

• Here: drop host_for_display (struck IPv6 case), fix the WS-side query/fragment asymmetry above, and switch Fixes → Refs. That leaves a small, self-contained, correct PR.
• Follow-up: eager derivation plus emitting the HTTP side via self.http.as_str(), which closes the panic case, the derived-vs-explicit case, and userinfo together. Note this would also supersede the hand-rolled HTTP component assembly added here — so the less this PR invests in that path, the less gets thrown away.

One thing that does not transfer to the follow-up: the WS side can't just use as_str(), because the format deliberately compresses to a bare port when the host matches the HTTP host. It will still need explicit query/fragment emission — so fixing it here is durable work, not throwaway.

---

For transparency on my earlier review state: the IPv6 point stands and item 2 adds a second concrete change, so the request stays as-is. Removing the helper and closing the WS-side asymmetry would clear it from my side — the scope/Fixes question is a maintainer call, not something I'd block on.

[Kewe63]

Thanks @dumanoglu1 and @osr21 for the detailed review and issue clarification.

I pushed a follow-up commit addressing the review feedback:

• changed the PR reference from Fixes SyncEndpointUrl accepts endpoints that later panic or fail Display round-tripping #323 to Refs SyncEndpointUrl accepts endpoints that later panic or fail Display round-tripping #323 so this partial fix does not auto-close the broader issue
• removed the custom IPv6 host formatting and went back to Url::host_str(), since url already preserves bracketed IPv6 hosts
• fixed the WebSocket override Display path to preserve query and fragment components
• expanded the regression test to cover HTTP and WebSocket path/query/fragment round-tripping together
• updated the PR description to reflect the narrower scope and the remaining SyncEndpointUrl accepts endpoints that later panic or fail Display round-tripping #323 follow-up decisions

I re-ran the focused checks after the update:

• cargo +1.94.0 test -p arc-consensus-types rpc_sync -- --nocapture — 24 passed
• cargo +1.94.0 fmt -p arc-consensus-types --check — passed
• CARGO_BUILD_JOBS=1 cargo +1.94.0 clippy -p arc-consensus-types --all-targets -- -D warnings — passed
• git diff --check — passed

This PR is now intended as a scoped partial fix for the derived-port validation and Display component preservation, while leaving the broader userinfo / eager WebSocket derivation / Display-as-serialization questions to #323 or a follow-up PR.

[osr21]

Re-reviewed at 0e628fc. Both changes I asked for are correctly implemented, and one of them goes further than I asked. Approving. Everything below is verification detail plus three non-blocking findings — one of which is actionable outside the code.

Verified fixed

host_for_display removed. The url::Host import went with it, so no dead import. host_str() now supplies both hosts, and the IPv4-mapped divergence I was worried about (::ffff:127.0.0.1 vs ::ffff:7f00:1) is gone with it, since the WHATWG serializer is doing all the work again.

WebSocket query and fragment now emitted. This closes the asymmetry I raised.

The has_suffix change is the right call, and it wasn't in my report

I flagged the missing query()/fragment() writes. Emitting them alone would have introduced a new bug, and you caught it independently:

let has_suffix = has_path || ws_url.query().is_some() || ws_url.fragment().is_some();
if ws_host != host || has_suffix {

Without widening the condition from has_path to has_suffix, a same-host override carrying only a query would have taken the bare-port branch and emitted ,wss=443?token=abc. On reparse 443?token=abc fails parse::<u16>(), falls through to Url::parse("wss://443?token=abc"), and 443 is then interpreted as a hostname — silently producing a completely different endpoint rather than an error. The widened guard is exactly what prevents that, and the revised comment says so accurately.

Test traced by hand

No cargo or rustc in my environment, so this is source review only — I stepped through display_preserves_http_and_websocket_path_query_and_fragment manually:

• split_once(',') hits the delimiter comma first (the HTTP fragment #http-fragment contains none), so the split is clean.
• parse_ws_override splits on the first = (the one in wss=), leaving ws.example.com/websocket?token=abc#ws-fragment for Url::parse, which keeps both components.
• Display emits :443 via port_or_known_default() on the HTTP side, but the WS side uses ws_url.port(), which returns None for the scheme default — hence no :443 after ws.example.com. That asymmetry is what makes the expected string correct.

The assertion and the round-trip both hold. display_brackets_ipv6_hosts also still passes, and it's now doing real work: with host_for_display gone it is the only thing standing between you and a silent regression if host_str()'s bracketing behaviour ever changed. Good that you kept it rather than deleting it along with the helper.

1. The Refs #323 edit did not actually clear the auto-close link

You made the right edit — the body now reads Refs #323 with no closing keyword, and neither commit message contains one. But GitHub still has the closing reference registered:

closingIssuesReferences: [ { number: 323, state: OPEN } ]

I re-queried this after your last body edit (19:42 UTC) and it persists. There is also no ConnectedEvent in the timeline, so this isn't a manual sidebar link that you could simply detach — it looks like GitHub didn't recompute the reference when the keyword changed.

Practical effect: as things stand, merging this into the default branch would still close #323, which is the outcome your body text explicitly says you don't want. Worth checking the Development section in the PR sidebar — if #323 is listed there, unlink it; if it isn't, another no-op body edit usually forces a recompute. Either way it's worth confirming it clears before this merges, because the description now promises the opposite of what's registered.

2. CI has not verified this yet

Your checklist reports 24/24 plus clean fmt and clippy, and the sabotage check on the derived-port validation is genuinely the right way to prove a regression test bites. Flagging only so it isn't mistaken for green CI: the Rust workflows have not run on this head.

Workflow	Status
Public CI	action_required
Build Docker	action_required
StepSecurity Required Checks	success

Everything else on the commit is a skipped release job. action_required means a maintainer has to approve workflow runs for a fork PR, so the combined status is still pending and mergeable_state is blocked. Nothing for you to fix — it just needs a maintainer click.

3. Derived WebSocket endpoints now duplicate the HTTP query and fragment (non-blocking)

New side effect of this commit, in the no-override path. websocket() derives by cloning the whole HTTP URL:

let mut ws_url = self.http.clone();   // carries path, query AND fragment

Display previously reproduced only the cloned path. It now reproduces the query and fragment too, so for an endpoint with no override at all:

let e: SyncEndpointUrl = "https://rpc.example.com/v1?apikey=SECRET".parse()?;
// before: https://rpc.example.com:443/v1?apikey=SECRET,wss=rpc.example.com/v1
// now:    https://rpc.example.com:443/v1?apikey=SECRET,wss=rpc.example.com/v1?apikey=SECRET

Two observations. The query is echoed twice, so anything credential-shaped in it gets printed twice wherever this is formatted. And a fragment is meaningless for a dialled WebSocket endpoint — fragments are never transmitted — so copying it across is pure noise.

I'd call this cosmetic rather than a defect, for a reason worth stating explicitly: Display for this type still has no production caller. peers.rs dials via http()/websocket(), the %endpoint tracing field in rpc_sync/client.rs is a plain &Url rather than this type, and follow_endpoints is #[serde(skip)]. So the blast radius today is test output.

Deliberately not asking you to "fix" it, because the obvious fix isn't obviously right: clearing the query in the derived branch of websocket() would change what actually gets dialled, and if anyone is passing an auth token as a query parameter that would break them. That's a product decision belonging to the #323 discussion, not to this PR.

What would help here is a test pinning the current behaviour, because right now nothing covers it. Every Display test uses either a bare host (display_http_only, display_https_only) or an explicit override (all the rest, including all six roundtrip inputs) — so no test exercises Display for a derived WebSocket endpoint that has a path or query. One case would make the duplication intentional and visible rather than incidental.

---

Approving on the strength of the two fixes. Item 1 is the only one I'd genuinely want resolved before merge, and it's a settings/description matter rather than a code change.

[osr21]

Confirmed this is the correct replacement for host_for_display.

Url::host_str() returns IPv6 hosts already bracketed — the WHATWG serializer wraps the compressed hextets in [ and ] — so the helper was reproducing behaviour url already provided, and the two implementations disagreed on IPv4-mapped addresses.

Worth noting that display_brackets_ipv6_hosts changes role with this commit. Previously it passed whether or not the helper existed, which is why I called it out as not discriminating. Now that the helper is gone it is the only guard on this behaviour, so it earns its place. Keeping it was the right instinct.

[osr21]

This line is the part of the change I want to single out, because it wasn't in my report and it prevents a bug that emitting query/fragment alone would have created.

Had the condition stayed has_path, a same-host override with only a query would have taken the bare-port branch below and produced ,wss=443?token=abc. Reparsing that fails value.parse::<u16>(), falls through to Url::parse("wss://443?token=abc"), and 443 becomes a hostname — a silently wrong endpoint instead of a parse error, which is strictly worse than the dropped-query bug being fixed.

Widening to has_suffix closes that off, and the reworded comment describes the real hazard.

[osr21]

These two writes are correct for the explicit-override case, which is what the new test covers.

Flagging a side effect on the derived path, where ws_url comes from websocket() cloning the entire HTTP URL rather than from parse_ws_override. The clone carries the HTTP query and fragment, so these writes now echo them into the WebSocket segment:

let e: SyncEndpointUrl = "https://rpc.example.com/v1?apikey=SECRET".parse()?;
// now renders ?apikey=SECRET twice, once per segment

Not asking for a code change — clearing the query inside websocket() would alter what actually gets dialled, which is a #323 decision. But no test currently exercises Display for a derived endpoint carrying a path or query (every Display test uses a bare host or an explicit override), so this behaviour is presently unpinned. A single case here would make it deliberate.