From 56a967a108f43436092912d82e133cbe87ad932a Mon Sep 17 00:00:00 2001 From: Toni Bergholm Date: Fri, 23 Oct 2026 09:59:12 +0300 Subject: [PATCH 1/5] fix(forge): require a host in --public-url, bound the child wait in the startup-settings test --- crates/forge/src/server.rs | 9 +++++++-- crates/forge/tests/e2e_a.rs | 36 +++++++++++++++++++++++++++--------- 2 files changed, 34 insertions(+), 11 deletions(-) diff --git a/crates/forge/src/server.rs b/crates/forge/src/server.rs index efb4031c..c98fb684 100644 --- a/crates/forge/src/server.rs +++ b/crates/forge/src/server.rs @@ -143,10 +143,15 @@ pub fn validate_public_url(url: &str) -> Result<(), String> { let rest = url .strip_prefix("http://") .or_else(|| url.strip_prefix("https://")); + let host = rest + .and_then(|r| r.split(['/', '?', '#']).next()) + .unwrap_or(""); + // A host must come first: not empty, and not just a port or user-info. + let host_ok = !host.is_empty() && !host.starts_with([':', '@']); match rest { - Some(r) if !r.is_empty() && r.bytes().all(|b| b.is_ascii_graphic()) => Ok(()), + Some(r) if host_ok && r.bytes().all(|b| b.is_ascii_graphic()) => Ok(()), _ => Err(format!( - "--public-url {url:?} must be an http:// or https:// URL of visible ASCII characters (percent-encode anything else)" + "--public-url {url:?} must be an http:// or https:// URL with a host, of visible ASCII characters (percent-encode anything else)" )), } } diff --git a/crates/forge/tests/e2e_a.rs b/crates/forge/tests/e2e_a.rs index 619fe74c..99aeebf4 100644 --- a/crates/forge/tests/e2e_a.rs +++ b/crates/forge/tests/e2e_a.rs @@ -1176,24 +1176,42 @@ fn a_database_with_the_right_version_but_no_tables_is_not_ready() { fn serve_refuses_unusable_settings_at_startup() { let dir = std::env::temp_dir().join(format!("scl-forge-serve-args-{}", std::process::id())); let bin = env!("CARGO_BIN_EXE_sc-forge"); + // Bounded wait: if a bad setting were accepted the server would run until a signal, + // so kill and reap it instead of stalling the suite. let run = |extra: &[&str]| { - std::process::Command::new(bin) + let mut child = std::process::Command::new(bin) .args(["serve", "--listen", "127.0.0.1:0", "--data-dir"]) .arg(&dir) .args(extra) - .output() - .unwrap() + .stderr(std::process::Stdio::piped()) + .spawn() + .unwrap(); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10); + let status = loop { + if let Some(st) = child.try_wait().unwrap() { + break Some(st); + } + if std::time::Instant::now() > deadline { + child.kill().unwrap(); + let _ = child.wait(); + break None; + } + std::thread::sleep(std::time::Duration::from_millis(20)); + }; + let mut err = String::new(); + std::io::Read::read_to_string(child.stderr.as_mut().unwrap(), &mut err).unwrap(); + (status.is_some_and(|s| !s.success()), err) }; - let o = run(&[ + let (refused, err) = run(&[ "--public-url", "http://127.0.0.1:1", "--max-connections", "0", ]); - assert!( - !o.status.success() && String::from_utf8_lossy(&o.stderr).contains("--max-connections") - ); - let o = run(&["--public-url", "http://exämple.com"]); - assert!(!o.status.success() && String::from_utf8_lossy(&o.stderr).contains("--public-url")); + assert!(refused && err.contains("--max-connections"), "{err}"); + for bad in ["http://exämple.com", "http:///forge"] { + let (refused, err) = run(&["--public-url", bad]); + assert!(refused && err.contains("--public-url"), "{bad}: {err}"); + } let _ = std::fs::remove_dir_all(&dir); } From ccea0520d87aef7c0da9ee4e63a32b9012b56cdf Mon Sep 17 00:00:00 2001 From: Toni Bergholm Date: Fri, 23 Oct 2026 10:41:05 +0300 Subject: [PATCH 2/5] fix(forge): reject user-info in --public-url --- crates/forge/src/server.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/forge/src/server.rs b/crates/forge/src/server.rs index c98fb684..7834de4e 100644 --- a/crates/forge/src/server.rs +++ b/crates/forge/src/server.rs @@ -147,7 +147,9 @@ pub fn validate_public_url(url: &str) -> Result<(), String> { .and_then(|r| r.split(['/', '?', '#']).next()) .unwrap_or(""); // A host must come first: not empty, and not just a port or user-info. - let host_ok = !host.is_empty() && !host.starts_with([':', '@']); + // No user-info: credentials in a base URL would leak into every Link header, and the + // host after an `@` is easy to get empty. + let host_ok = !host.is_empty() && !host.starts_with(':') && !host.contains('@'); match rest { Some(r) if host_ok && r.bytes().all(|b| b.is_ascii_graphic()) => Ok(()), _ => Err(format!( From da257cda79bd85dddc183adf7c9dcee3fd6b759d Mon Sep 17 00:00:00 2001 From: Toni Bergholm Date: Fri, 23 Oct 2026 10:45:28 +0300 Subject: [PATCH 3/5] fix(forge): validate --public-url authority with an allowlist A DNS-style host or a valid bracketed IPv6 address plus an optional numeric port, no user-info. Replaces the denylist that kept missing edge cases (http://[]/x, http://user@/x, empty host). --- crates/forge/src/server.rs | 47 ++++++++++++++++++++++++++++++-------- 1 file changed, 38 insertions(+), 9 deletions(-) diff --git a/crates/forge/src/server.rs b/crates/forge/src/server.rs index 7834de4e..1157ca4a 100644 --- a/crates/forge/src/server.rs +++ b/crates/forge/src/server.rs @@ -137,23 +137,49 @@ fn to_response(r: Resp) -> Response> { }) } -/// Check `--public-url` at startup: an `http(s)://` URL of visible ASCII only, so -/// it can be placed in `Link` headers and bodies without surprises. +/// Is `a` a plain `host[:port]` authority? An allowlist, not a denylist: a DNS-style +/// name (letters, digits, `.`, `-`) or a bracketed IPv6 address, then an optional +/// numeric port. No user-info, no empty host. +fn valid_authority(a: &str) -> bool { + let (host_ok, port) = if let Some(rest) = a.strip_prefix('[') { + let Some((v6, after)) = rest.split_once(']') else { + return false; + }; + let port = if after.is_empty() { + None + } else { + match after.strip_prefix(':') { + Some(p) => Some(p), + None => return false, + } + }; + (v6.parse::().is_ok(), port) + } else { + let (h, p) = a.split_once(':').map_or((a, None), |(h, p)| (h, Some(p))); + let ok = !h.is_empty() + && !h.starts_with(['.', '-']) + && h.bytes() + .all(|b| b.is_ascii_alphanumeric() || b == b'.' || b == b'-'); + (ok, p) + }; + host_ok + && port + .is_none_or(|p| !p.is_empty() && p.len() <= 5 && p.bytes().all(|b| b.is_ascii_digit())) +} + +/// Check `--public-url` at startup: `http(s)://host[:port][/path]` of visible ASCII, +/// so it can be placed in `Link` headers and bodies without surprises. pub fn validate_public_url(url: &str) -> Result<(), String> { let rest = url .strip_prefix("http://") .or_else(|| url.strip_prefix("https://")); - let host = rest + let authority = rest .and_then(|r| r.split(['/', '?', '#']).next()) .unwrap_or(""); - // A host must come first: not empty, and not just a port or user-info. - // No user-info: credentials in a base URL would leak into every Link header, and the - // host after an `@` is easy to get empty. - let host_ok = !host.is_empty() && !host.starts_with(':') && !host.contains('@'); match rest { - Some(r) if host_ok && r.bytes().all(|b| b.is_ascii_graphic()) => Ok(()), + Some(r) if valid_authority(authority) && r.bytes().all(|b| b.is_ascii_graphic()) => Ok(()), _ => Err(format!( - "--public-url {url:?} must be an http:// or https:// URL with a host, of visible ASCII characters (percent-encode anything else)" + "--public-url {url:?} must be http(s)://host[:port][/path] with a DNS name or bracketed IPv6 host, no user-info, and visible ASCII only (percent-encode anything else)" )), } } @@ -213,6 +239,9 @@ mod tests { fn public_url_must_be_header_safe() { assert!(validate_public_url("http://127.0.0.1:3000").is_ok()); assert!(validate_public_url("https://git.example.com/forge").is_ok()); + assert!(validate_public_url("http://[::1]:3000").is_ok()); + assert!(validate_public_url("http://[2001:db8::1]/forge").is_ok()); + assert!(validate_public_url("http://localhost").is_ok()); for bad in [ "", "ftp://x", From 685a22974a7a6ce41c442a04b2aad949bf28ff37 Mon Sep 17 00:00:00 2001 From: Toni Bergholm Date: Fri, 23 Oct 2026 10:52:15 +0300 Subject: [PATCH 4/5] fix(forge): parse --public-url with the url crate and use the normalized base Replaces the hand-rolled validator that kept missing cases (port range, query and fragment, angle brackets, empty hostname labels, user-info, empty port): a real parse plus the literal scheme://host shape, and the normalized (percent-encoded, no trailing slash) base is what URLs in bodies and Link headers are built from. --- Cargo.lock | 1 + crates/forge/Cargo.toml | 1 + crates/forge/src/cli.rs | 5 +- crates/forge/src/server.rs | 132 +++++++++++++++++++++++-------------- 4 files changed, 89 insertions(+), 50 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 55040deb..d41f61db 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5555,6 +5555,7 @@ dependencies = [ "serde_json", "thiserror 2.0.21", "tokio", + "url", ] [[package]] diff --git a/crates/forge/Cargo.toml b/crates/forge/Cargo.toml index b9930c20..f0f2e3e5 100644 --- a/crates/forge/Cargo.toml +++ b/crates/forge/Cargo.toml @@ -30,6 +30,7 @@ rusqlite = { version = "0.40.2", features = ["bundled"] } serde_json = "1.0.154" thiserror = "2.0.18" tokio = { version = "1.54.1", features = ["rt-multi-thread", "net", "io-util", "signal", "time", "macros", "sync"] } +url = "2.5.8" [lints] workspace = true diff --git a/crates/forge/src/cli.rs b/crates/forge/src/cli.rs index 96a15037..143a7614 100644 --- a/crates/forge/src/cli.rs +++ b/crates/forge/src/cli.rs @@ -200,7 +200,8 @@ fn run_admin(cmd: AdminCmd) -> Result<()> { } fn run_serve(a: ServeArgs) -> Result<()> { - scl_forge::server::validate_public_url(&a.public_url).map_err(anyhow::Error::msg)?; + let public_url = + scl_forge::server::validate_public_url(&a.public_url).map_err(anyhow::Error::msg)?; if a.max_connections == 0 { bail!("--max-connections must be at least 1"); } @@ -225,7 +226,7 @@ fn run_serve(a: ServeArgs) -> Result<()> { cidrs, }; db::open_for_serve(&a.data_dir)?; - let app = make_app(a.data_dir, &a.public_url, proxies, a.argon2_concurrency); + let app = make_app(a.data_dir, &public_url, proxies, a.argon2_concurrency); let limits = Limits { max_body: a.max_body_bytes, max_connections: a.max_connections, diff --git a/crates/forge/src/server.rs b/crates/forge/src/server.rs index 1157ca4a..fcf01cde 100644 --- a/crates/forge/src/server.rs +++ b/crates/forge/src/server.rs @@ -137,51 +137,55 @@ fn to_response(r: Resp) -> Response> { }) } -/// Is `a` a plain `host[:port]` authority? An allowlist, not a denylist: a DNS-style -/// name (letters, digits, `.`, `-`) or a bracketed IPv6 address, then an optional -/// numeric port. No user-info, no empty host. -fn valid_authority(a: &str) -> bool { - let (host_ok, port) = if let Some(rest) = a.strip_prefix('[') { - let Some((v6, after)) = rest.split_once(']') else { - return false; - }; - let port = if after.is_empty() { - None - } else { - match after.strip_prefix(':') { - Some(p) => Some(p), - None => return false, - } - }; - (v6.parse::().is_ok(), port) - } else { - let (h, p) = a.split_once(':').map_or((a, None), |(h, p)| (h, Some(p))); - let ok = !h.is_empty() - && !h.starts_with(['.', '-']) - && h.bytes() - .all(|b| b.is_ascii_alphanumeric() || b == b'.' || b == b'-'); - (ok, p) - }; - host_ok - && port - .is_none_or(|p| !p.is_empty() && p.len() <= 5 && p.bytes().all(|b| b.is_ascii_digit())) -} - -/// Check `--public-url` at startup: `http(s)://host[:port][/path]` of visible ASCII, -/// so it can be placed in `Link` headers and bodies without surprises. -pub fn validate_public_url(url: &str) -> Result<(), String> { - let rest = url +/// Parse and normalize `--public-url`: `http(s)://host[:port][/path]` with a real host, +/// no user-info, no query or fragment. Returns the normalized base (percent-encoded, +/// no trailing slash) that every URL in a body or `Link` header is built from, so a +/// raw space or angle bracket in the path can never reach a header. +pub fn validate_public_url(input: &str) -> Result { + let bad = |why: &str| format!("--public-url {input:?} is not usable: {why}"); + // WHATWG parsing forgives extra or backward slashes (`http:///x` becomes host `x`); + // an operator typing that made a mistake, so require the literal `scheme://host` shape. + let after = input .strip_prefix("http://") - .or_else(|| url.strip_prefix("https://")); - let authority = rest + .or_else(|| input.strip_prefix("https://")); + if after.is_none_or(|r| r.starts_with(['/', '\\'])) { + return Err(bad( + "it must start with http:// or https:// followed by a host", + )); + } + // `http://@host` parses as an empty user name; any `@` before the path is user-info. + let authority = after .and_then(|r| r.split(['/', '?', '#']).next()) .unwrap_or(""); - match rest { - Some(r) if valid_authority(authority) && r.bytes().all(|b| b.is_ascii_graphic()) => Ok(()), - _ => Err(format!( - "--public-url {url:?} must be http(s)://host[:port][/path] with a DNS name or bracketed IPv6 host, no user-info, and visible ASCII only (percent-encode anything else)" - )), + if authority.contains('@') { + return Err(bad("it must not contain user-info")); + } + if authority.ends_with(':') { + return Err(bad("the port after ':' is empty")); + } + let url = url::Url::parse(input).map_err(|e| bad(&e.to_string()))?; + if !matches!(url.scheme(), "http" | "https") { + return Err(bad("the scheme must be http or https")); } + if !url.username().is_empty() || url.password().is_some() { + return Err(bad("it must not contain user-info")); + } + if url.query().is_some() || url.fragment().is_some() { + return Err(bad("it must not contain a query or fragment")); + } + match url.host() { + Some(url::Host::Domain(d)) => { + // WHATWG parsing tolerates empty labels and edge hyphens; a base URL should not. + if d.split('.') + .any(|l| l.is_empty() || l.starts_with('-') || l.ends_with('-')) + { + return Err(bad("the host has an empty or hyphen-edged label")); + } + } + Some(_) => {} + None => return Err(bad("it has no host")), + } + Ok(url.as_str().trim_end_matches('/').to_string()) } /// Serve until `shutdown` resolves, then wait up to `drain` for in-flight @@ -236,21 +240,53 @@ mod tests { use super::*; #[test] - fn public_url_must_be_header_safe() { - assert!(validate_public_url("http://127.0.0.1:3000").is_ok()); - assert!(validate_public_url("https://git.example.com/forge").is_ok()); - assert!(validate_public_url("http://[::1]:3000").is_ok()); - assert!(validate_public_url("http://[2001:db8::1]/forge").is_ok()); - assert!(validate_public_url("http://localhost").is_ok()); + fn public_url_is_parsed_and_normalized() { + for ok in [ + "http://127.0.0.1:3000", + "https://git.example.com/forge", + "http://[::1]:3000", + "http://[2001:db8::1]/forge", + "http://localhost", + ] { + assert!(validate_public_url(ok).is_ok(), "{ok}"); + } + assert_eq!( + validate_public_url("https://git.example.com/forge/").unwrap(), + "https://git.example.com/forge" + ); + assert_eq!( + validate_public_url("http://h/a b").unwrap(), + "http://h/a%20b%3Cc%3E", + "raw characters are encoded, not passed through" + ); + assert_eq!(validate_public_url("http://h").unwrap(), "http://h"); for bad in [ "", "ftp://x", "http://", + "http:///forge", + "http://:3000", + "http://@host", + "http://user@/forge", + "http://user:pw@example.com", + "http://[]/forge", + "http://[::1/forge", + "http://host:", + "http://host:abc", + "http://host:70000", + "http://host:65536", + "http://h?x", + "http://h/p#f", + "http://h#f", + "http://api..example.com", + "http://api.-example.com", + "http://-a.example.com", + "http://a-.example.com", "http://ex ample", - "http://exämple.com", "http://x\r\nSet-Cookie: a", ] { assert!(validate_public_url(bad).is_err(), "{bad:?}"); } + assert!(validate_public_url("http://host:65535").is_ok()); } } From 37074c6037579a652bcb00a0102ea84f1688fa15 Mon Sep 17 00:00:00 2001 From: Toni Bergholm Date: Fri, 23 Oct 2026 10:52:47 +0300 Subject: [PATCH 5/5] test(forge): unicode --public-url hosts normalize to punycode, use a genuinely bad URL in the startup test --- crates/forge/src/server.rs | 5 +++++ crates/forge/tests/e2e_a.rs | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/forge/src/server.rs b/crates/forge/src/server.rs index fcf01cde..2a99c888 100644 --- a/crates/forge/src/server.rs +++ b/crates/forge/src/server.rs @@ -260,6 +260,11 @@ mod tests { "raw characters are encoded, not passed through" ); assert_eq!(validate_public_url("http://h").unwrap(), "http://h"); + assert_eq!( + validate_public_url("http://exämple.com").unwrap(), + "http://xn--exmple-cua.com", + "unicode hosts become their ASCII form" + ); for bad in [ "", "ftp://x", diff --git a/crates/forge/tests/e2e_a.rs b/crates/forge/tests/e2e_a.rs index 99aeebf4..0d67e544 100644 --- a/crates/forge/tests/e2e_a.rs +++ b/crates/forge/tests/e2e_a.rs @@ -1209,7 +1209,7 @@ fn serve_refuses_unusable_settings_at_startup() { "0", ]); assert!(refused && err.contains("--max-connections"), "{err}"); - for bad in ["http://exämple.com", "http:///forge"] { + for bad in ["http://ex ample", "http:///forge"] { let (refused, err) = run(&["--public-url", bad]); assert!(refused && err.contains("--public-url"), "{bad}: {err}"); }