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 efb4031c..2a99c888 100644 --- a/crates/forge/src/server.rs +++ b/crates/forge/src/server.rs @@ -137,18 +137,55 @@ 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. -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://")); - match rest { - Some(r) if !r.is_empty() && 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)" - )), + .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(""); + 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 @@ -203,18 +240,58 @@ 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()); + 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"); + 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", "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()); } } diff --git a/crates/forge/tests/e2e_a.rs b/crates/forge/tests/e2e_a.rs index 619fe74c..0d67e544 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 ample", "http:///forge"] { + let (refused, err) = run(&["--public-url", bad]); + assert!(refused && err.contains("--public-url"), "{bad}: {err}"); + } let _ = std::fs::remove_dir_all(&dir); }