diff --git a/crates/update/src/http.rs b/crates/update/src/http.rs index 72db1fae..660f7ee9 100644 --- a/crates/update/src/http.rs +++ b/crates/update/src/http.rs @@ -163,7 +163,7 @@ impl Http { { req = req.header("Authorization", &format!("Bearer {token}")); } - let mut resp = req.call().map_err(|e| format!("couldn't reach GitHub ({e})"))?; + let mut resp = req.call().map_err(|e| format!("couldn't reach GitHub ({})", call_failed(&e)))?; let header = |name: &str| resp.headers().get(name).and_then(|v| v.to_str().ok()).map(str::to_string); let status = resp.status().as_u16(); match status { @@ -191,8 +191,11 @@ impl Http { pub fn get_feed(&self, url: &str) -> Result { let failed = |why: String| FeedError { why, throttled: false, retry_after: None }; let agent = if is_loopback(url) { &self.direct } else { &self.feed }; - let mut resp = - agent.get(url).header("Accept", "application/json").call().map_err(|e| failed(format!("couldn't reach {} ({e})", host(url))))?; + let mut resp = agent + .get(url) + .header("Accept", "application/json") + .call() + .map_err(|e| failed(format!("couldn't reach {} ({})", host(url), call_failed(&e))))?; // Seconds only; an HTTP date falls back to our own backoff. let retry_after = resp.headers().get("retry-after").and_then(|v| v.to_str().ok()).and_then(|r| r.trim().parse::().ok()); match resp.status().as_u16() { @@ -204,7 +207,7 @@ impl Http { /// GET a small text file (following redirects). pub fn get_text(&self, url: &str) -> Result, String> { let agent = if is_loopback(url) { &self.direct } else { &self.small }; - let mut resp = agent.get(url).call().map_err(|e| format!("couldn't reach {} ({e})", host(url)))?; + let mut resp = agent.get(url).call().map_err(|e| format!("couldn't reach {} ({})", host(url), call_failed(&e)))?; match resp.status().as_u16() { 200 => read_text_capped(&mut resp, MAX_SMALL).map(Some).map_err(|e| format!("unreadable answer ({e})")), 404 => Ok(None), @@ -217,7 +220,7 @@ impl Http { /// on the body when the length wasn't announced or was wrong. pub fn get_bytes(&self, url: &str, max: usize) -> Result>, String> { let agent = if is_loopback(url) { &self.direct } else { &self.small }; - let mut resp = agent.get(url).call().map_err(|e| format!("couldn't reach {} ({e})", host(url)))?; + let mut resp = agent.get(url).call().map_err(|e| format!("couldn't reach {} ({})", host(url), call_failed(&e)))?; match resp.status().as_u16() { 200 => { let announced = resp.headers().get("content-length").and_then(|v| v.to_str().ok()).and_then(|v| v.parse::().ok()); @@ -236,7 +239,7 @@ impl Http { /// Where `url` redirects to, without following it. pub fn redirect_target(&self, url: &str) -> Result, String> { - let resp = self.no_redirect.head(url).call().map_err(|e| format!("couldn't reach {} ({e})", host(url)))?; + let resp = self.no_redirect.head(url).call().map_err(|e| format!("couldn't reach {} ({})", host(url), call_failed(&e)))?; match resp.status().as_u16() { 300..=399 => Ok(resp.headers().get("location").and_then(|v| v.to_str().ok()).map(str::to_string)), 404 => Ok(None), @@ -351,7 +354,7 @@ impl Http { Ok(r) => r, // Asking again gets the same answer. Err(ureq::Error::RequireHttpsOnly(to)) => { - return Err(AttemptError::Fatal(format!("the download led to plain HTTP ({to}), so it wasn't fetched"))); + return Err(AttemptError::Fatal(format!("the download led to plain HTTP at {}, so it wasn't fetched", host(&to)))); } Err(e) => return Err(retry(format!("couldn't connect: {e}"), false)), }; @@ -618,7 +621,19 @@ fn refused_until(status: u16, remaining: Option<&str>, reset: Option, retry /// The host of `url` for error messages, or all of it when it has no scheme. fn host(url: &str) -> &str { - url.split_once("://").and_then(|(_, rest)| rest.split('/').next()).unwrap_or(url) + let Some((_, rest)) = url.split_once("://") else { return url }; + let authority = rest.split(['/', '?', '#']).next().unwrap_or_default(); + // Never `user:password@`. + authority.rsplit_once('@').map_or(authority, |(_, host)| host) +} + +/// Why a request failed, for a message. A hop refused for plain HTTP names only its host: +/// ureq's own message holds the whole link, and a redirect's link can carry a signed query. +fn call_failed(e: &ureq::Error) -> String { + match e { + ureq::Error::RequireHttpsOnly(to) => format!("refused plain HTTP to {}", host(to)), + e => e.to_string(), + } } /// Reads a text body, rejecting one whose *decoded* size exceeds `max`. @@ -661,6 +676,8 @@ pub(crate) mod tests { assert_eq!(host("https://api.github.com/repos/storytold/wordcraft"), "api.github.com"); assert_eq!(host("http://127.0.0.1:8080"), "127.0.0.1:8080"); assert_eq!(host("not a url"), "not a url"); + assert_eq!(host("http://objects.example.com?X-Amz-Signature=abc"), "objects.example.com"); + assert_eq!(host("http://user:secret@example.com/x"), "example.com"); } #[test] @@ -1138,17 +1155,19 @@ pub(crate) mod tests { // ureq checks every hop, redirects included, before it connects; `.invalid` never // resolves, so an agent that tried would fail on DNS instead. let http = Http::build(crate::TEST_AGENT, true).unwrap(); - let refused = |r: Result<_, String>| r.is_err_and(|e| e.contains("https only")); - assert!(refused(http.get_api("http://api.example.invalid/repos/x/y/releases", None).map(|_| ()))); - assert!(refused(http.get_text("http://example.invalid/SHA256SUMS.txt").map(|_| ()))); + let refused = |r: Result<_, String>| r.is_err_and(|e| e.contains("refused plain HTTP to example.invalid") && !e.contains("secret")); + assert!(refused(http.get_api("http://example.invalid/repos/x/y/releases?secret", None).map(|_| ()))); + assert!(refused(http.get_text("http://example.invalid/SHA256SUMS.txt?secret").map(|_| ()))); + // The scheme is case-insensitive, and an upper-case one isn't a loopback link either. + assert!(refused(http.get_text("HTTP://example.invalid/SHA256SUMS.txt").map(|_| ()))); assert!(refused(http.get_bytes("http://example.invalid/icon.png", 100).map(|_| ()))); assert!(refused(http.redirect_target("http://example.invalid/releases/latest").map(|_| ()))); - assert!(http.get_feed("http://example.invalid/feed.json").is_err_and(|e| e.why.contains("https only"))); + assert!(http.get_feed("http://example.invalid/feed.json").is_err_and(|e| e.why.contains("refused plain HTTP"))); // A download fails at once rather than retrying until it pauses. let dir = tempfile::tempdir().unwrap(); let (_, sha) = body(); - let result = http.download("http://example.invalid/file.bin", &dir.path().join("file.bin"), 10, &sha, &Stop::default(), &mut |_| {}); - assert!(result.is_err_and(|e| e.contains("led to plain HTTP"))); + let result = http.download("http://example.invalid/file.bin?secret", &dir.path().join("file.bin"), 10, &sha, &Stop::default(), &mut |_| {}); + assert_eq!(result.unwrap_err(), "the download led to plain HTTP at example.invalid, so it wasn't fetched"); } #[test] diff --git a/crates/update/tests/https_only.rs b/crates/update/tests/https_only.rs new file mode 100644 index 00000000..059433f0 --- /dev/null +++ b/crates/update/tests/https_only.rs @@ -0,0 +1,52 @@ +//! `Http::new` as an app gets it: an integration test builds the library without `cfg(test)`, so +//! this is the release configuration (or the `local-mirror` one, with that feature on). + +use std::io::{BufRead, BufReader, Write}; +use std::net::TcpListener; +use std::sync::Arc; +use std::sync::atomic::{AtomicUsize, Ordering}; + +use craft_update::http::{Fetched, Http}; + +/// A plain-HTTP server on loopback that answers every request with `{}`; counts the requests. +fn serve() -> (String, Arc) { + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let base = format!("http://{}", listener.local_addr().unwrap()); + let seen = Arc::new(AtomicUsize::new(0)); + let count = seen.clone(); + std::thread::spawn(move || { + for stream in listener.incoming() { + let Ok(mut stream) = stream else { return }; + let mut reader = BufReader::new(stream.try_clone().unwrap()); + let mut line = String::new(); + while reader.read_line(&mut line).unwrap_or(0) > 0 && line != "\r\n" { + line.clear(); + } + count.fetch_add(1, Ordering::SeqCst); + let _ = write!(stream, "HTTP/1.1 200 OK\r\nContent-Length: 2\r\nConnection: close\r\n\r\n{{}}"); + } + }); + (base, seen) +} + +#[test] +fn the_remote_agents_take_plain_http_only_with_a_local_mirror() { + let (base, seen) = serve(); + let http = Http::new("craft-update-tests/0").unwrap(); + // `get_api` and `redirect_target` always use the agents for remote hosts. + let api = http.get_api(&format!("{base}/repos/storytold/x/releases"), None); + let redirect = http.redirect_target(&format!("{base}/releases/latest")); + if cfg!(feature = "local-mirror") { + assert!(matches!(api, Ok(Fetched::Body { .. })), "{api:?}"); + assert!(matches!(redirect, Err(ref e) if e.contains("HTTP 200")), "{redirect:?}"); + assert_eq!(seen.load(Ordering::SeqCst), 2); + } else { + assert!(api.as_ref().is_err_and(|e| e.contains("refused plain HTTP")), "{api:?}"); + assert!(redirect.as_ref().is_err_and(|e| e.contains("refused plain HTTP")), "{redirect:?}"); + // An upper-case scheme isn't a loopback link, so it goes to a remote agent too. + assert!(http.get_text(&format!("HTTP{}", &base[4..])).is_err_and(|e| e.contains("refused plain HTTP"))); + assert_eq!(seen.load(Ordering::SeqCst), 0, "a refused request never connects"); + } + // A loopback link goes to the direct agents, which serve plain HTTP in every build. + assert_eq!(http.get_text(&format!("{base}/SHA256SUMS.txt")).unwrap().as_deref(), Some("{}")); +}