Merge pull request #514 from ljkx/update/https-only-agents

update: keep the remote HTTP agents and their redirects on HTTPS (#477)

crates/update/src/http.rs

@@ -65,6 +65,15 @@ impl Http {
     /// `user_agent` names the product and its version (`PhotoCraft/0.9.0`) and nothing about the
     /// person using it.
     pub fn new(user_agent: &str) -> Result<Self, String> {
+        // Tests and a local mirror serve plain HTTP on loopback, which `get_api` and
+        // `redirect_target` reach through the remote agents.
+        Self::build(user_agent, !(cfg!(feature = "local-mirror") || cfg!(test)))
+    }
+
+    /// `https_only` keeps the agents for remote hosts, and every redirect they follow, on HTTPS:
+    /// a release download that GitHub's file host redirected to `http://` is refused, not read in
+    /// plain text. The `direct` agents only ever fetch `is_loopback` links, which are plain HTTP.
+    fn build(user_agent: &str, https_only: bool) -> Result<Self, String> {
         // The OS checks each certificate chain itself: Windows fetches roots it hasn't stored yet
         // and trusts what antivirus HTTPS scanning installs. Reading the store ourselves missed
         // both ("invalid peer certificate: UnknownIssuer").
@@ -74,6 +83,7 @@ impl Http {
         let small = ureq::Agent::config_builder()
             .tls_config(tls())
             .proxy(proxy.clone())
+            .https_only(https_only)
             .timeout_global(Some(Duration::from_secs(20)))
             .http_status_as_error(false)
             .user_agent(user_agent)
@@ -82,6 +92,7 @@ impl Http {
         let feed = ureq::Agent::config_builder()
             .tls_config(tls())
             .proxy(proxy.clone())
+            .https_only(https_only)
             .timeout_connect(Some(Duration::from_secs(5)))
             .timeout_global(Some(Duration::from_secs(10)))
             .http_status_as_error(false)
@@ -91,6 +102,7 @@ impl Http {
         let no_redirect = ureq::Agent::config_builder()
             .tls_config(tls())
             .proxy(proxy.clone())
+            .https_only(https_only)
             .timeout_global(Some(Duration::from_secs(20)))
             .http_status_as_error(false)
             .max_redirects(0)
@@ -101,6 +113,7 @@ impl Http {
         let download = ureq::Agent::config_builder()
             .tls_config(tls())
             .proxy(proxy.clone())
+            .https_only(https_only)
             .timeout_connect(Some(Duration::from_secs(20)))
             .timeout_recv_response(Some(Duration::from_secs(60)))
             // ureq has no idle timeout: cap each attempt instead, and resume where it stopped.
@@ -150,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 {
@@ -178,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() {
@@ -191,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),
@@ -204,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());
@@ -223,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),
@@ -336,6 +352,10 @@ impl Http {
         }
         let mut resp = match req.call() {
             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 at {}, so it wasn't fetched", host(&to))));
+            }
             Err(e) => return Err(retry(format!("couldn't connect: {e}"), false)),
         };
         let status = resp.status().as_u16();
@@ -601,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`.
@@ -644,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]
@@ -1116,6 +1150,34 @@ pub(crate) mod tests {
         assert_eq!(part_path(Path::new("/d/photocraft.dmg"), 9), Path::new("/d/photocraft.dmg.9.part"));
     }
 
+    #[test]
+    fn the_remote_agents_refuse_plain_http_before_connecting() {
+        // 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("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("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?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]
+    fn https_only_still_reaches_a_loopback_link() {
+        let (body, _) = body();
+        let (url, _) = serve(body.clone(), vec![Reply::Full]);
+        let http = Http::build(crate::TEST_AGENT, true).unwrap();
+        assert_eq!(http.get_bytes(&url, body.len()).unwrap(), Some(body));
+    }
+
     #[test]
     fn get_bytes_rejects_oversized_payload() {
         let listener = TcpListener::bind("127.0.0.1:0").unwrap();

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