update: test Http::new's HTTPS-only setting, and keep refused links out of messages

An integration test builds the library without cfg(test), so it checks the release configuration: the remote agents refuse plain HTTP (an upper-case scheme too) without connecting, loopback links still work, and with local-mirror the remote agents take plain HTTP.

A refused hop's message names only the host. ureq's message holds the whole link, and a redirect to the file host carries a signed query. host() also drops a query and any user:password@.

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<String, FeedError> {
         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::<i64>().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<Option<String>, 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<Option<Vec<u8>>, 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::<u64>().ok());
@@ -236,7 +239,7 @@ impl Http {
 
     /// Where `url` redirects to, without following it.
     pub fn redirect_target(&self, url: &str) -> Result<Option<String>, 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<i64>, 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]

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<AtomicUsize>) {
+    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("{}"));
+}