From 680c90b3a8f19662b54b0ba235e05944a94c7a62 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 18:31:39 -0500 Subject: [PATCH] clawhdf5-remote: redirects are followed safely MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- crates/clawhdf5-remote/src/error.rs | 6 + crates/clawhdf5-remote/src/http.rs | 177 ++++++++++++++++-- crates/clawhdf5-remote/tests/common/server.rs | 37 +++- crates/clawhdf5-remote/tests/http.rs | 55 ++++++ 4 files changed, 260 insertions(+), 15 deletions(-) diff --git a/crates/clawhdf5-remote/src/error.rs b/crates/clawhdf5-remote/src/error.rs index 20494cd..6e3c551 100644 --- a/crates/clawhdf5-remote/src/error.rs +++ b/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" diff --git a/crates/clawhdf5-remote/src/http.rs b/crates/clawhdf5-remote/src/http.rs index 14f221e..9fd10c6 100644 --- a/crates/clawhdf5-remote/src/http.rs +++ b/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 - /// `Authorization`). + /// Extra headers sent with every request to the URL's own origin (for + /// 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) -> 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 { + 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 { - let mut req = self - .agent - .get(&self.url) - .header("Accept-Encoding", "identity"); + let mut req = self.agent.get(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 => {} } - for (k, v) in &self.options.headers { - req = req.header(k, v); + 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, 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, 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, RemoteError> { self.requests.fetch_add(1, Ordering::Relaxed); - let resp = self - .request(Some((start, end - 1))) - .call() - .map_err(transport)?; + let resp = self.call(Some((start, end - 1)))?; 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")); + } } diff --git a/crates/clawhdf5-remote/tests/common/server.rs b/crates/clawhdf5-remote/tests/common/server.rs index 2cdd7c1..1708084 100644 --- a/crates/clawhdf5-remote/tests/common/server.rs +++ b/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>, + /// (path, headers) of every counted request, header names lowercase. + pub seen: Mutex)>>, /// 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)); diff --git a/crates/clawhdf5-remote/tests/http.rs b/crates/clawhdf5-remote/tests/http.rs index c61ee6e..08119f3 100644 --- a/crates/clawhdf5-remote/tests/http.rs +++ b/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}"); +}