clawhdf5-remote: redirects are followed safely

ureq's defaults followed up to 10 redirects, including from https to plain
http, and forwarded the custom HttpOptions::headers (X-Api-Key, Cookie,
...) to whatever host a redirect named — only Authorization was stripped.

HttpStorage now follows redirects itself (ureq's max_redirects is 0):
- at most HttpOptions::max_redirects per request (default 5; 0 refuses
  any redirect), then RemoteError::Redirect;
- never from https to another scheme, nor to a non-http(s) URL;
- once a redirect leaves the URL's origin (scheme, host, port), none of
  the custom headers is sent any more (Authorization included);
- each hop counts as a request; errors show the target redacted.

Tests: a redirect to another local port reads the right data and the
target never sees X-Api-Key or Authorization (it did before); a
same-origin redirect keeps them; a loop stops after 6 requests; 0 refuses;
unit tests for target resolution, the https downgrade and origins.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>

crates/clawhdf5-remote/src/error.rs

@@ -158,6 +158,10 @@ pub enum RemoteError {
     ObjectStore(String),
     /// Called in a way the backend cannot serve.
     Usage(String),
+    /// A redirect that is not followed: from `https` to `http`, to
+    /// another scheme, or beyond
+    /// [`HttpOptions::max_redirects`](crate::HttpOptions).
+    Redirect(String),
     /// The file is larger than a download was allowed to be
     /// ([`download`](crate::download)).
     TooLarge {
@@ -189,6 +193,7 @@ impl RemoteError {
             RemoteError::Transport(s) => RemoteError::Transport(f(s)),
             RemoteError::ObjectStore(s) => RemoteError::ObjectStore(f(s)),
             RemoteError::Usage(s) => RemoteError::Usage(f(s)),
+            RemoteError::Redirect(s) => RemoteError::Redirect(f(s)),
             e @ RemoteError::TooLarge { .. } => e,
             RemoteError::Backend(s) => RemoteError::Backend(f(s)),
         }
@@ -221,6 +226,7 @@ impl std::fmt::Display for RemoteError {
             RemoteError::Transport(s) => write!(f, "network error: {s}"),
             RemoteError::ObjectStore(s) => write!(f, "object store: {s}"),
             RemoteError::Usage(s) => write!(f, "{s}"),
+            RemoteError::Redirect(s) => write!(f, "redirect refused: {s}"),
             RemoteError::TooLarge { len, limit } => write!(
                 f,
                 "the remote file is {len} bytes, more than the download limit of {limit} bytes"

crates/clawhdf5-remote/src/http.rs

@@ -17,6 +17,10 @@
 //!   [`HttpOptions::allow_full_download`] is set: then the file is
 //!   downloaded once, at open, and read from memory.
 //!
+//! Redirects are followed (at most [`HttpOptions::max_redirects`]), but
+//! never from `https` to `http`, and the custom
+//! [`HttpOptions::headers`] are not sent to another origin.
+//!
 //! Transient failures — connection errors, timeouts, `408`/`429`/`5xx`, and
 //! a body shorter or longer than its `Content-Range` — are retried with
 //! exponential backoff. Responses are requested with
@@ -36,7 +40,7 @@ use std::time::Duration;
 use clawhdf5_format::error::FormatError;
 use clawhdf5_format::storage::Storage;
 
-use crate::error::{Redactor, RemoteError};
+use crate::error::{Redactor, RemoteError, redact_url};
 
 /// Settings of an [`HttpStorage`].
 #[derive(Debug, Clone)]
@@ -63,9 +67,17 @@ pub struct HttpOptions {
     /// `Last-Modified`, since a change of the file could then go unnoticed
     /// (only its length is checked).
     pub require_validator: bool,
-    /// Extra headers sent with every request (for example
+    /// Extra headers sent with every request to the URL's own origin (for
-    /// `Authorization`).
+    /// example `Authorization` or `X-Api-Key`). They are never sent to
+    /// another origin a redirect leads to.
     pub headers: Vec<(String, String)>,
+    /// Redirects followed per request (0: none, a redirect is an error).
+    /// A redirect from `https` to plain `http` is always refused; once a
+    /// redirect leaves the URL's origin (scheme, host and port), the
+    /// [`headers`](Self::headers) are no longer sent. Every request of a
+    /// file follows the redirects again (the target is not remembered, as
+    /// a presigned target may expire).
+    pub max_redirects: u32,
 }
 
 impl Default for HttpOptions {
@@ -80,6 +92,7 @@ impl Default for HttpOptions {
             max_full_download: 1 << 30,
             require_validator: false,
             headers: Vec::new(),
+            max_redirects: 5,
         }
     }
 }
@@ -166,6 +179,64 @@ fn check_identity(url: &str, resp: &ureq::http::Response<ureq::Body>) -> Result<
     }
 }
 
+/// Scheme, host (lowercase, no userinfo) and port of an absolute URL.
+fn origin(url: &str) -> Option<(String, String, u16)> {
+    let (scheme, rest) = url.split_once("://")?;
+    let scheme = scheme.to_ascii_lowercase();
+    let authority = rest.split(['/', '?', '#']).next().unwrap_or("");
+    let host_port = authority.rsplit_once('@').map_or(authority, |(_, h)| h);
+    let default = match scheme.as_str() {
+        "http" => 80,
+        "https" => 443,
+        _ => return None,
+    };
+    // "[v6]:port", "host:port", or either without a port.
+    let (host, port) = match host_port.rsplit_once(':') {
+        Some((h, p)) if !p.contains(']') => (h, p.parse().ok()?),
+        _ => (host_port, default),
+    };
+    Some((scheme, host.to_ascii_lowercase(), port))
+}
+
+fn same_origin(a: &str, b: &str) -> bool {
+    matches!((origin(a), origin(b)), (Some(x), Some(y)) if x == y)
+}
+
+/// The URL a redirect from `base` to `location` goes to, if it may be
+/// followed: `http`/`https` only, and never from `https` to `http`.
+fn redirect_target(base: &str, location: &str) -> Result<String, RemoteError> {
+    let location = location.trim();
+    let (scheme, rest) = base.split_once("://").unwrap_or(("http", base));
+    let authority_end = rest.find(['/', '?', '#']).unwrap_or(rest.len());
+    let (authority, path) = rest.split_at(authority_end);
+    let target = if location.contains("://") {
+        location.to_string()
+    } else if let Some(r) = location.strip_prefix("//") {
+        format!("{scheme}://{r}")
+    } else if location.starts_with('/') {
+        format!("{scheme}://{authority}{location}")
+    } else {
+        let path = path.split(['?', '#']).next().unwrap_or("");
+        let dir = path.rsplit_once('/').map_or("", |(d, _)| d);
+        format!("{scheme}://{authority}{dir}/{location}")
+    };
+    let Some((to_scheme, _, _)) = origin(&target) else {
+        return Err(RemoteError::Redirect(format!(
+            "{} redirects to {}, which is not an http(s) URL",
+            redact_url(base),
+            redact_url(&target)
+        )));
+    };
+    if scheme.eq_ignore_ascii_case("https") && to_scheme != "https" {
+        return Err(RemoteError::Redirect(format!(
+            "{} redirects to {}: a downgrade from https is refused",
+            redact_url(base),
+            redact_url(&target)
+        )));
+    }
+    Ok(target)
+}
+
 fn transport(e: ureq::Error) -> RemoteError {
     match e {
         ureq::Error::StatusCode(code) => RemoteError::Status {
@@ -204,8 +275,10 @@ impl HttpStorage {
         } else if !lower.starts_with("http://") {
             return Err(RemoteError::UnsupportedScheme(shown.to_string()));
         }
+        // Redirects are followed by `call`, which applies our rules.
         let config = ureq::Agent::config_builder()
             .http_status_as_error(false)
+            .max_redirects(0)
             .timeout_global(Some(options.timeout))
             .build();
         let mut storage = HttpStorage {
@@ -286,14 +359,15 @@ impl HttpStorage {
         }
     }
 
+    /// One `GET` of `url`. The options' custom headers are sent only to
+    /// the URL's own origin (`trusted`).
     fn request(
         &self,
+        url: &str,
         range: Option<(u64, u64)>,
+        trusted: bool,
     ) -> ureq::RequestBuilder<ureq::typestate::WithoutBody> {
-        let mut req = self
+        let mut req = self.agent.get(url).header("Accept-Encoding", "identity");
-            .agent
-            .get(&self.url)
-            .header("Accept-Encoding", "identity");
         if let Some((a, b)) = range {
             req = req.header("Range", format!("bytes={a}-{b}"));
         }
@@ -302,12 +376,60 @@ impl HttpStorage {
             Validator::LastModified(t) => req = req.header("If-Unmodified-Since", t),
             Validator::None => {}
         }
+        if trusted {
             for (k, v) in &self.options.headers {
                 req = req.header(k, v);
             }
+        }
         req
     }
 
+    /// Send a ranged `GET`, following redirects by the rules of
+    /// [`HttpOptions::max_redirects`]: at most that many, never from
+    /// `https` to anything else, and without the custom headers once the
+    /// chain has left the URL's origin. Each hop counts as a request.
+    fn call(
+        &self,
+        range: Option<(u64, u64)>,
+    ) -> Result<ureq::http::Response<ureq::Body>, RemoteError> {
+        let mut url = self.url.clone();
+        let mut trusted = true;
+        let mut hops = 0u32;
+        loop {
+            if hops > 0 {
+                self.requests.fetch_add(1, Ordering::Relaxed);
+            }
+            let resp = self
+                .request(&url, range, trusted)
+                .call()
+                .map_err(|e| transport(e).scrubbed(&Redactor::new(&url)))?;
+            if !matches!(resp.status().as_u16(), 301 | 302 | 303 | 307 | 308) {
+                return Ok(resp);
+            }
+            let status = resp.status().as_u16();
+            let Some(location) = header(&resp, "location") else {
+                return Err(RemoteError::BadResponse(format!(
+                    "{}: status {status} without a Location",
+                    redact_url(&url)
+                )));
+            };
+            let next = redirect_target(&url, location)?;
+            if hops >= self.options.max_redirects {
+                return Err(RemoteError::Redirect(format!(
+                    "{} redirects to {}: more than HttpOptions::max_redirects ({})",
+                    redact_url(&url),
+                    redact_url(&next),
+                    self.options.max_redirects
+                )));
+            }
+            if !same_origin(&next, &self.url) {
+                trusted = false;
+            }
+            url = next;
+            hops += 1;
+        }
+    }
+
     /// Read a body of exactly `want` bytes (or up to `limit` when `want` is
     /// unknown).
     fn body(
@@ -347,7 +469,7 @@ impl HttpStorage {
     fn probe(&self) -> Result<(u64, Validator, Vec<u8>, bool), RemoteError> {
         let n = self.options.first_request.max(1);
         self.requests.fetch_add(1, Ordering::Relaxed);
-        let resp = self.request(Some((0, n - 1))).call().map_err(transport)?;
+        let resp = self.call(Some((0, n - 1)))?;
         let status = resp.status().as_u16();
         check_identity(self.redactor.shown(), &resp)?;
         let validator = match (header(&resp, "etag"), header(&resp, "last-modified")) {
@@ -418,10 +540,7 @@ impl HttpStorage {
     /// One request for `[start, end)` (inside the file, non-empty).
     fn fetch_once(&self, start: u64, end: u64) -> Result<Vec<u8>, RemoteError> {
         self.requests.fetch_add(1, Ordering::Relaxed);
-        let resp = self
+        let resp = self.call(Some((start, end - 1)))?;
-            .request(Some((start, end - 1)))
-            .call()
-            .map_err(transport)?;
         let changed =
             |why: String| RemoteError::FileChanged(format!("{}: {why}", self.redactor.shown()));
         check_identity(self.redactor.shown(), &resp)?;
@@ -594,4 +713,34 @@ mod tests {
         assert_eq!(content_range("items 0-1/2"), None);
         assert_eq!(content_range("bytes */1000"), None);
     }
+
+    #[test]
+    fn redirect_targets_and_origins() {
+        use super::{redirect_target, same_origin};
+        let t = |b: &str, l: &str| redirect_target(b, l).map_err(|e| e.to_string());
+        assert_eq!(
+            t("http://a:8/d/f.h5?x=1", "g.h5").unwrap(),
+            "http://a:8/d/g.h5"
+        );
+        assert_eq!(t("http://a/d/f.h5", "/g.h5").unwrap(), "http://a/g.h5");
+        assert_eq!(t("https://a/d/f.h5", "//b/g.h5").unwrap(), "https://b/g.h5");
+        assert_eq!(t("http://a/f", "https://b/g").unwrap(), "https://b/g");
+        let e = t("https://a/f.h5", "http://a/f.h5").unwrap_err();
+        assert!(e.contains("downgrade"), "{e}");
+        assert!(t("HTTPS://a/f.h5", "http://b/f.h5").is_err());
+        assert!(t("https://a/f", "//b/g").is_ok());
+        assert!(t("http://a/f", "ftp://b/g").is_err());
+        let e = t("https://u:pw@a/f?sig=SECRET", "http://b/g?sig=OTHER").unwrap_err();
+        assert!(
+            !e.contains("SECRET") && !e.contains("OTHER") && !e.contains("pw"),
+            "{e}"
+        );
+        assert!(same_origin("http://a/x", "http://A:80/y"));
+        assert!(same_origin("https://u:p@a:443/x", "https://a/y"));
+        assert!(!same_origin("http://a/x", "https://a/x"));
+        assert!(!same_origin("http://a:1/x", "http://a:2/x"));
+        assert!(!same_origin("http://a/x", "http://b/x"));
+        assert!(same_origin("http://[::1]:8/x", "http://[::1]:8/y"));
+        assert!(!same_origin("http://[::1]/x", "http://[::1]:8/y"));
+    }
 }

crates/clawhdf5-remote/tests/common/server.rs

@@ -57,6 +57,10 @@ pub struct Shared {
     pub force_status: AtomicU32,
     /// Answer ranges with a `Content-Range` one byte off.
     pub wrong_range: AtomicBool,
+    /// Path → `Location`: answered `302 Found` (counted as a request).
+    pub redirects: RwLock<HashMap<String, String>>,
+    /// (path, headers) of every counted request, header names lowercase.
+    pub seen: Mutex<Vec<(String, HashMap<String, String>)>>,
     /// Requests for a served path (every status); requests for other
     /// paths are not counted.
     pub requests: AtomicU64,
@@ -119,11 +123,31 @@ impl Server {
         self.shared.bytes.load(Ordering::SeqCst)
     }
 
-    /// Zero the counters and the log.
+    /// Zero the counters and the logs.
     pub fn reset(&self) {
         self.shared.requests.store(0, Ordering::SeqCst);
         self.shared.bytes.store(0, Ordering::SeqCst);
         self.shared.log.lock().unwrap().clear();
+        self.shared.seen.lock().unwrap().clear();
+    }
+
+    /// Answer `path` with a redirect to `location`.
+    pub fn redirect(&self, path: &str, location: &str) {
+        self.shared
+            .redirects
+            .write()
+            .unwrap()
+            .insert(path.to_string(), location.to_string());
+    }
+
+    /// Whether any request the server counted carried header `name`.
+    pub fn saw_header(&self, name: &str) -> bool {
+        self.shared
+            .seen
+            .lock()
+            .unwrap()
+            .iter()
+            .any(|(_, h)| h.contains_key(name))
     }
 
     /// The ranges asked for so far.
@@ -242,6 +266,16 @@ fn serve(conn: TcpStream, s: &Shared) -> std::io::Result<()> {
         let close = headers
             .get("connection")
             .is_some_and(|v| v.eq_ignore_ascii_case("close"));
+        let redirect = s.redirects.read().unwrap().get(&path).cloned();
+        if let Some(location) = redirect {
+            s.requests.fetch_add(1, Ordering::SeqCst);
+            s.seen.lock().unwrap().push((path.clone(), headers.clone()));
+            write!(
+                out,
+                "HTTP/1.1 302 Found\r\nLocation: {location}\r\nContent-Length: 0\r\n\r\n"
+            )?;
+            continue;
+        }
         let res = {
             let files = s.files.read().unwrap();
             files
@@ -257,6 +291,7 @@ fn serve(conn: TcpStream, s: &Shared) -> std::io::Result<()> {
             continue;
         };
         s.requests.fetch_add(1, Ordering::SeqCst);
+        s.seen.lock().unwrap().push((path.clone(), headers.clone()));
         let delay = s.delay_ms.load(Ordering::SeqCst);
         if delay > 0 {
             std::thread::sleep(Duration::from_millis(delay));

crates/clawhdf5-remote/tests/http.rs

@@ -722,3 +722,58 @@ fn credentials_never_appear_in_errors_or_debug() {
         "https://host:8/d/f.h5?X-Amz-Signature=REDACTED&a=REDACTED"
     );
 }
+
+/// Redirects are followed within limits: custom credential headers reach
+/// only the URL's own origin (not another port a redirect leads to), a
+/// same-origin redirect keeps them, loops end at `max_redirects`, and
+/// `max_redirects = 0` refuses any redirect. (The https→http downgrade
+/// refusal is a unit test of `redirect_target`: no TLS server here.)
+#[test]
+fn redirects_are_followed_safely() {
+    let bytes = multi_block_file();
+    let local = File::from_bytes(bytes.clone()).unwrap();
+    let want = local.dataset("big").unwrap().read_f64().unwrap();
+    let target = Server::start(vec![("/m.h5".into(), bytes.clone())]);
+    let front = Server::start(vec![("/m.h5".into(), bytes.clone())]);
+    front.redirect("/r.h5", &target.url("/m.h5"));
+    front.redirect("/s.h5", "/m.h5");
+    front.redirect("/loop.h5", "/loop.h5");
+    let mut opts = quick();
+    opts.http.headers = vec![
+        ("X-Api-Key".into(), "sekrit".into()),
+        ("Authorization".into(), "Bearer tok".into()),
+    ];
+
+    // Cross-origin (another port): followed, the headers stay behind.
+    let f = open_url_with(&front.url("/r.h5"), &opts).unwrap();
+    assert_eq!(f.dataset("big").unwrap().read_f64().unwrap(), want);
+    assert!(front.saw_header("x-api-key"), "sent to the URL's origin");
+    assert!(target.requests() > 1);
+    assert!(
+        !target.saw_header("x-api-key") && !target.saw_header("authorization"),
+        "credential headers forwarded across origins: {:?}",
+        target.shared.seen.lock().unwrap()
+    );
+
+    // Same origin: followed with the headers.
+    front.reset();
+    let f = open_url_with(&front.url("/s.h5"), &opts).unwrap();
+    assert_eq!(f.dataset("big").unwrap().read_f64().unwrap(), want);
+    let seen = front.shared.seen.lock().unwrap().clone();
+    assert!(
+        seen.iter()
+            .filter(|(p, _)| p == "/m.h5")
+            .all(|(_, h)| h.get("x-api-key").map(String::as_str) == Some("sekrit"))
+    );
+
+    // A loop ends after max_redirects (5 by default): 6 requests.
+    front.reset();
+    let e = open_url_with(&front.url("/loop.h5"), &opts).unwrap_err();
+    assert!(matches!(e, Error::Remote(RemoteError::Redirect(_))), "{e}");
+    assert_eq!(front.requests(), 6);
+
+    // No redirects allowed.
+    opts.http.max_redirects = 0;
+    let e = open_url_with(&front.url("/s.h5"), &opts).unwrap_err();
+    assert!(matches!(e, Error::Remote(RemoteError::Redirect(_))), "{e}");
+}