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 1 commit
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 @@ -19,7 +19,7 @@
use core::fmt;
use std::str::FromStr;

use url::Url;
use url::{Host, Url};

/// A parsed endpoint URL for RPC synchronization.
///
Expand Down 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 @@ -153,17 +169,23 @@ impl FromStr for SyncEndpointUrl {

impl fmt::Display for SyncEndpointUrl {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let host = self.http.host_str().expect("validated host");
let host = host_for_display(&self.http);
let http_port = self.http.port_or_known_default().expect("validated port");
let ws_url = self.websocket();
let ws_host = ws_url.host_str().expect("validated host");
let ws_host = host_for_display(&ws_url);

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 != "/";
Expand All @@ -189,6 +211,13 @@ impl fmt::Display for SyncEndpointUrl {
}
}

fn host_for_display(url: &Url) -> String {
match url.host().expect("validated host") {
Host::Ipv6(addr) => format!("[{addr}]"),

@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 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.

host => host.to_string(),
}
}

#[cfg(test)]
mod tests {
use super::*;
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_path_and_query() {
let endpoint: SyncEndpointUrl =
"https://rpc.example.com/api/v1?key=value,wss=ws.example.com/websocket"
.parse()
.unwrap();

assert_eq!(
endpoint.to_string(),
"https://rpc.example.com:443/api/v1?key=value,wss=ws.example.com/websocket"
);
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