Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Kewe63 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2d1885b
fix: validate sync endpoint URL roundtrips
Kewe63 Sep 3, 2026
0e628fc
fix(types): address sync endpoint review feedback
Kewe63 Sep 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 73 additions & 9 deletions crates/types/src/rpc_sync.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,21 @@ fn validate_ws_scheme(scheme: &str) -> Result<(), eyre::Report> {
Ok(())
}

fn validate_derived_ws_port(http: &Url, has_ws_override: bool) -> Result<(), eyre::Report> {

@osr21 osr21 Sep 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

if has_ws_override {
return Ok(());
}

if matches!(http.port(), Some(u16::MAX)) {
return Err(eyre::eyre!(
"Invalid HTTP URL port '{}': derived WebSocket port would overflow.",
u16::MAX
));
}

Ok(())
}

/// Parses a WebSocket override in the format `<scheme>=<value>`.
///
/// The value after `=` can be:
Expand Down Expand Up @@ -142,6 +157,7 @@ impl FromStr for SyncEndpointUrl {
Url::parse(http_part).map_err(|e| eyre::eyre!("Failed to parse HTTP URL: {e}"))?;

validate_http_scheme(http.scheme())?;
validate_derived_ws_port(&http, ws_part.is_some())?;

let ws = ws_part
.map(|part| parse_ws_override(part, &http))
Expand All @@ -158,19 +174,26 @@ impl fmt::Display for SyncEndpointUrl {
let ws_url = self.websocket();
let ws_host = ws_url.host_str().expect("validated host");

@osr21 osr21 Sep 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.


write!(
f,
"{}://{host}:{http_port},{}=",
self.http.scheme(),
ws_url.scheme()
)?;
write!(f, "{}://{host}:{http_port}", self.http.scheme())?;
let http_path = self.http.path();
if http_path != "/" {
write!(f, "{http_path}")?;
}
if let Some(query) = self.http.query() {
write!(f, "?{query}")?;
}
if let Some(fragment) = self.http.fragment() {
write!(f, "#{fragment}")?;
}
write!(f, ",{}=", ws_url.scheme())?;

let ws_path = ws_url.path();
let has_path = ws_path != "/";
let has_suffix = has_path || ws_url.query().is_some() || ws_url.fragment().is_some();

@osr21 osr21 Sep 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.


if ws_host != host || has_path {
// Include the host when it differs or when a path is present
// (a bare port + path like `443/websocket` mis-parses as a hostname)
if ws_host != host || has_suffix {
// Include the host when it differs or when extra URL components are
// present (a bare port plus path/query/fragment mis-parses as a host).
write!(f, "{ws_host}")?;
if let Some(ws_port) = ws_url.port() {
write!(f, ":{ws_port}")?;
Expand All @@ -184,6 +207,12 @@ impl fmt::Display for SyncEndpointUrl {
if has_path {
write!(f, "{ws_path}")?;
}
if let Some(query) = ws_url.query() {

@osr21 osr21 Sep 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

write!(f, "?{query}")?;
}
if let Some(fragment) = ws_url.fragment() {
write!(f, "#{fragment}")?;
}

Ok(())
}
Expand Down Expand Up @@ -349,6 +378,41 @@ mod tests {
assert_eq!(url.websocket().as_str(), "wss://ws.example.com:1212/");
}

#[test]
fn parse_rejects_http_port_that_would_overflow_derived_websocket_port() {
let err = "http://localhost:65535"
.parse::<SyncEndpointUrl>()
.unwrap_err();

assert!(err
.to_string()
.contains("derived WebSocket port would overflow"));
}

#[test]
fn display_preserves_http_and_websocket_path_query_and_fragment() {
let endpoint: SyncEndpointUrl =
"https://rpc.example.com/api/v1?key=value#http-fragment,wss=ws.example.com/websocket?token=abc#ws-fragment"
.parse()
.unwrap();

assert_eq!(
endpoint.to_string(),
"https://rpc.example.com:443/api/v1?key=value#http-fragment,wss=ws.example.com/websocket?token=abc#ws-fragment"
);
let reparsed: SyncEndpointUrl = endpoint.to_string().parse().unwrap();
assert_eq!(endpoint, reparsed);
}

#[test]
fn display_brackets_ipv6_hosts() {
let endpoint: SyncEndpointUrl = "http://[::1]:8545,ws=8546".parse().unwrap();

assert_eq!(endpoint.to_string(), "http://[::1]:8545,ws=8546");

@osr21 osr21 Sep 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

let reparsed: SyncEndpointUrl = endpoint.to_string().parse().unwrap();
assert_eq!(endpoint, reparsed);
}

#[test]
fn parse_wss_with_host_port_and_path_override() {
let url: SyncEndpointUrl = "https://example.com,wss=ws.example.com:8546/websocket"
Expand Down