From c04e34620eda88821ccb052d9c4bbb230817bb81 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 18:30:02 -0500 Subject: [PATCH] clawhdf5-remote, h5rs: URLs' credentials are never shown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every RemoteError message and HttpStorage's Debug output held the URL as given, with any user:password@ and the query string — for a presigned S3/GCS/Azure URL, its signature or token. An application logging the error leaked the credential. - New clawhdf5_remote::redact_url: no userinfo, no fragment, query values replaced by REDACTED (plain key names kept). - HttpStorage formats every message with the redacted URL, and scrubs the URL's secret parts from errors of the HTTP client (whose texts can echo the URI); Debug shows the redacted URL. storage_for_url's and the object store URL errors are redacted too. HttpStorage::url() still returns the URL as given, documented as not for logging. - h5rs prints FILE arguments that are URLs redacted: in errors and in dump/stat/check/diff output. - The test server can force a status and send a wrong Content-Range. Tests: 404, 403 (at open and on a read), wrong Content-Range (at open and on a read), no range support, encoded body, ETag change, timeout, connection closed and bad scheme errors, Display and Debug, contain none of the secrets; h5rs likewise for every subcommand. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/clawhdf5-remote/src/error.rs | 140 ++++++++++++++++++ crates/clawhdf5-remote/src/http.rs | 84 +++++++---- crates/clawhdf5-remote/src/lib.rs | 12 +- crates/clawhdf5-remote/src/object.rs | 11 +- crates/clawhdf5-remote/tests/common/server.rs | 16 +- crates/clawhdf5-remote/tests/http.rs | 118 +++++++++++++++ crates/clawhdf5-tools/src/check.rs | 2 +- crates/clawhdf5-tools/src/diff.rs | 5 +- crates/clawhdf5-tools/src/dump.rs | 1 + crates/clawhdf5-tools/src/h5.rs | 40 ++++- crates/clawhdf5-tools/src/stat.rs | 2 +- crates/clawhdf5-tools/tests/remote.rs | 39 +++++ 12 files changed, 421 insertions(+), 49 deletions(-) diff --git a/crates/clawhdf5-remote/src/error.rs b/crates/clawhdf5-remote/src/error.rs index b2546b6..20494cd 100644 --- a/crates/clawhdf5-remote/src/error.rs +++ b/crates/clawhdf5-remote/src/error.rs @@ -2,8 +2,126 @@ use clawhdf5_format::error::FormatError; +/// `url` as it may be shown in an error, a `Debug` output or a log: no +/// userinfo (`user:password@`), no fragment, and the query string's values +/// replaced by `REDACTED` (a presigned S3/GCS/Azure URL carries its +/// signature or token there). Keys are kept when they look like plain +/// names, so a message still says which kind of URL it was. +/// +/// ``` +/// assert_eq!( +/// clawhdf5_remote::redact_url("https://me:pw@host/f.h5?X-Amz-Signature=abc&a=1#x"), +/// "https://host/f.h5?X-Amz-Signature=REDACTED&a=REDACTED" +/// ); +/// ``` +pub fn redact_url(url: &str) -> String { + let (scheme, rest) = match url.split_once("://") { + Some((s, r)) => (Some(s), r), + None => (None, url), + }; + let rest = rest.split('#').next().unwrap_or(""); + let (before_query, query) = match rest.split_once('?') { + Some((a, q)) => (a, Some(q)), + None => (rest, None), + }; + let mut out = String::with_capacity(url.len()); + if let Some(s) = scheme { + out.push_str(s); + out.push_str("://"); + let auth_end = before_query.find('/').unwrap_or(before_query.len()); + let (authority, path) = before_query.split_at(auth_end); + out.push_str( + authority + .rsplit_once('@') + .map_or(authority, |(_, host)| host), + ); + out.push_str(path); + } else { + out.push_str(before_query); + } + if let Some(q) = query { + out.push('?'); + let plain = |k: &str| { + !k.is_empty() + && k.len() <= 64 + && k.bytes() + .all(|b| b.is_ascii_alphanumeric() || matches!(b, b'-' | b'_' | b'.')) + }; + let parts: Vec = q + .split('&') + .map(|kv| { + let k = kv.split('=').next().unwrap_or(""); + if plain(k) { + format!("{k}=REDACTED") + } else { + "REDACTED".to_string() + } + }) + .collect(); + out.push_str(&parts.join("&")); + } + out +} + +/// Replaces the secret parts of one URL (its userinfo and query string, +/// and the URL itself) wherever they appear in a message — such as the +/// text of an error from the HTTP client. +#[derive(Debug, Clone)] +pub(crate) struct Redactor { + shown: String, + secrets: Vec<(String, String)>, +} + +impl Redactor { + pub(crate) fn new(url: &str) -> Redactor { + let shown = redact_url(url); + let mut secrets = vec![(url.to_string(), shown.clone())]; + let rest = url.split_once("://").map_or(url, |(_, r)| r); + let authority = rest.split(['/', '?', '#']).next().unwrap_or(""); + if let Some((userinfo, _)) = authority.rsplit_once('@') + && !userinfo.is_empty() + { + secrets.push((format!("{userinfo}@"), String::new())); + secrets.push((userinfo.to_string(), "REDACTED".into())); + } + if let Some((_, q)) = rest.split('#').next().unwrap_or("").split_once('?') + && !q.is_empty() + { + let shown_q = shown.split_once('?').map_or("", |(_, q)| q).to_string(); + secrets.push((q.to_string(), shown_q)); + for kv in q.split('&') { + if let Some((_, v)) = kv.split_once('=') + && v.len() >= 4 + { + secrets.push((v.to_string(), "REDACTED".into())); + } + } + } + Redactor { shown, secrets } + } + + /// The URL, redacted. + pub(crate) fn shown(&self) -> &str { + &self.shown + } + + /// `msg` with every secret part of the URL replaced. + pub(crate) fn scrub(&self, msg: &str) -> String { + let mut m = msg.to_string(); + for (secret, with) in &self.secrets { + if m.contains(secret.as_str()) { + m = m.replace(secret.as_str(), with); + } + } + m + } +} + /// Why a remote file could not be opened or read. /// +/// No message carries a URL's credentials: URLs appear as +/// [`redact_url`] shows them. +/// /// Inside a [`clawhdf5::File`] read these arrive as /// `clawhdf5::Error::Format(FormatError::Storage(message))`, the message /// being this error's `Display`. @@ -54,6 +172,28 @@ pub enum RemoteError { } impl RemoteError { + /// The error with every secret part of `r`'s URL scrubbed from its text. + #[cfg_attr(not(any(feature = "http", feature = "object-store")), allow(dead_code))] + pub(crate) fn scrubbed(self, r: &Redactor) -> RemoteError { + let f = |s: String| r.scrub(&s); + match self { + RemoteError::InvalidUrl(s) => RemoteError::InvalidUrl(f(s)), + RemoteError::UnsupportedScheme(s) => RemoteError::UnsupportedScheme(f(s)), + RemoteError::RangeNotSupported(s) => RemoteError::RangeNotSupported(f(s)), + RemoteError::FileChanged(s) => RemoteError::FileChanged(f(s)), + RemoteError::Status { code, what } => RemoteError::Status { + code, + what: f(what), + }, + RemoteError::BadResponse(s) => RemoteError::BadResponse(f(s)), + RemoteError::Transport(s) => RemoteError::Transport(f(s)), + RemoteError::ObjectStore(s) => RemoteError::ObjectStore(f(s)), + RemoteError::Usage(s) => RemoteError::Usage(f(s)), + e @ RemoteError::TooLarge { .. } => e, + RemoteError::Backend(s) => RemoteError::Backend(f(s)), + } + } + /// Whether retrying the same request may succeed. #[cfg_attr(not(feature = "http"), allow(dead_code))] pub(crate) fn is_transient(&self) -> bool { diff --git a/crates/clawhdf5-remote/src/http.rs b/crates/clawhdf5-remote/src/http.rs index ce3d68f..14f221e 100644 --- a/crates/clawhdf5-remote/src/http.rs +++ b/crates/clawhdf5-remote/src/http.rs @@ -36,7 +36,7 @@ use std::time::Duration; use clawhdf5_format::error::FormatError; use clawhdf5_format::storage::Storage; -use crate::error::RemoteError; +use crate::error::{Redactor, RemoteError}; /// Settings of an [`HttpStorage`]. #[derive(Debug, Clone)] @@ -107,7 +107,10 @@ enum Validator { /// documentation](self). pub struct HttpStorage { agent: ureq::Agent, + /// The URL as given, credentials and all: only ever sent to the server. url: String, + /// Shows the URL without its credentials, in every message. + redactor: Redactor, len: u64, validator: Validator, options: HttpOptions, @@ -122,7 +125,7 @@ pub struct HttpStorage { impl std::fmt::Debug for HttpStorage { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("HttpStorage") - .field("url", &self.url) + .field("url", &self.redactor.shown()) .field("len", &self.len) .field("validator", &self.validator) .field("full_download", &self.full.is_some()) @@ -181,15 +184,25 @@ impl HttpStorage { /// and the bytes that request fetched (the file's start), for a /// [`BlockCache`](crate::BlockCache) to keep. pub fn open(url: &str, options: HttpOptions) -> Result<(HttpStorage, Vec), RemoteError> { + let redactor = Redactor::new(url); + HttpStorage::open_inner(url, options, &redactor).map_err(|e| e.scrubbed(&redactor)) + } + + fn open_inner( + url: &str, + options: HttpOptions, + redactor: &Redactor, + ) -> Result<(HttpStorage, Vec), RemoteError> { + let shown = redactor.shown(); let lower = url.to_ascii_lowercase(); if lower.starts_with("https://") { if !cfg!(feature = "https") { return Err(RemoteError::UnsupportedScheme(format!( - "{url}: https:// needs the `https` feature of clawhdf5-remote" + "{shown}: https:// needs the `https` feature of clawhdf5-remote" ))); } } else if !lower.starts_with("http://") { - return Err(RemoteError::UnsupportedScheme(url.to_string())); + return Err(RemoteError::UnsupportedScheme(shown.to_string())); } let config = ureq::Agent::config_builder() .http_status_as_error(false) @@ -198,6 +211,7 @@ impl HttpStorage { let mut storage = HttpStorage { agent: ureq::Agent::new_with_config(config), url: url.to_string(), + redactor: redactor.clone(), len: 0, validator: Validator::None, options, @@ -216,14 +230,15 @@ impl HttpStorage { } if storage.options.require_validator && storage.validator == Validator::None { return Err(RemoteError::Usage(format!( - "{url}: the server sends neither a strong ETag nor Last-Modified, so a change \ + "{shown}: the server sends neither a strong ETag nor Last-Modified, so a change \ of the file could not be detected (HttpOptions::require_validator)" ))); } Ok((storage, bytes)) } - /// The URL. + /// The URL as given — with any credentials it carries, so do not log + /// it; [`redact_url`](crate::redact_url) gives a form that can be. pub fn url(&self) -> &str { &self.url } @@ -307,18 +322,20 @@ impl HttpStorage { let got = reader .take(cap.saturating_add(1)) .read_to_end(&mut buf) - .map_err(|e| RemoteError::Transport(format!("{}: reading the body: {e}", self.url))); + .map_err(|e| { + RemoteError::Transport(format!("{}: reading the body: {e}", self.redactor.shown())) + }); self.bytes.fetch_add(buf.len() as u64, Ordering::Relaxed); got?; match want { Some(n) if buf.len() as u64 != n => Err(RemoteError::BadResponse(format!( "{}: body of {} bytes, expected {n}", - self.url, + self.redactor.shown(), buf.len() ))), None if buf.len() as u64 > limit => Err(RemoteError::Usage(format!( "{}: the file is larger than HttpOptions::max_full_download ({limit} bytes)", - self.url + self.redactor.shown() ))), _ => Ok(buf), } @@ -332,7 +349,7 @@ impl HttpStorage { self.requests.fetch_add(1, Ordering::Relaxed); let resp = self.request(Some((0, n - 1))).call().map_err(transport)?; let status = resp.status().as_u16(); - check_identity(&self.url, &resp)?; + check_identity(self.redactor.shown(), &resp)?; let validator = match (header(&resp, "etag"), header(&resp, "last-modified")) { (Some(e), _) if !e.starts_with("W/") => Validator::ETag(e.to_string()), (_, Some(t)) => Validator::LastModified(t.to_string()), @@ -341,21 +358,27 @@ impl HttpStorage { match status { 206 => { let cr = header(&resp, "content-range").ok_or_else(|| { - RemoteError::BadResponse(format!("{}: 206 without Content-Range", self.url)) + RemoteError::BadResponse(format!( + "{}: 206 without Content-Range", + self.redactor.shown() + )) })?; let (a, b, total) = content_range(cr).ok_or_else(|| { - RemoteError::BadResponse(format!("{}: bad Content-Range {cr:?}", self.url)) + RemoteError::BadResponse(format!( + "{}: bad Content-Range {cr:?}", + self.redactor.shown() + )) })?; let total = total.ok_or_else(|| { RemoteError::BadResponse(format!( "{}: the server does not report the file's length (Content-Range {cr:?})", - self.url + self.redactor.shown() )) })?; if a != 0 || b >= total || b > n - 1 { return Err(RemoteError::BadResponse(format!( "{}: asked for bytes 0-{}, got Content-Range {cr:?}", - self.url, + self.redactor.shown(), n - 1 ))); } @@ -367,14 +390,15 @@ impl HttpStorage { return Err(RemoteError::RangeNotSupported(format!( "{} answered a range request with the whole file (status 200); set \ HttpOptions::allow_full_download to download it", - self.url + self.redactor.shown() ))); } let want = header(&resp, "content-length").and_then(|v| v.trim().parse().ok()); if want.is_some_and(|w: u64| w > self.options.max_full_download) { return Err(RemoteError::Usage(format!( "{}: the file is larger than HttpOptions::max_full_download ({} bytes)", - self.url, self.options.max_full_download + self.redactor.shown(), + self.options.max_full_download ))); } let bytes = self.body(resp, want, self.options.max_full_download)?; @@ -382,11 +406,11 @@ impl HttpStorage { } 416 => Err(RemoteError::Usage(format!( "{}: status 416 for the first bytes (an empty file?)", - self.url + self.redactor.shown() ))), code => Err(RemoteError::Status { code, - what: self.url.clone(), + what: self.redactor.shown().to_string(), }), } } @@ -398,14 +422,15 @@ impl HttpStorage { .request(Some((start, end - 1))) .call() .map_err(transport)?; - let changed = |why: String| RemoteError::FileChanged(format!("{}: {why}", self.url)); - check_identity(&self.url, &resp)?; + let changed = + |why: String| RemoteError::FileChanged(format!("{}: {why}", self.redactor.shown())); + check_identity(self.redactor.shown(), &resp)?; match resp.status().as_u16() { 206 => {} 200 => { return Err(RemoteError::RangeNotSupported(format!( "{} answered a range request with the whole file (status 200)", - self.url + self.redactor.shown() ))); } 412 => { @@ -417,7 +442,7 @@ impl HttpStorage { code => { return Err(RemoteError::Status { code, - what: format!("{} bytes {start}-{}", self.url, end - 1), + what: format!("{} bytes {start}-{}", self.redactor.shown(), end - 1), }); } } @@ -439,10 +464,16 @@ impl HttpStorage { Validator::None => {} } let cr = header(&resp, "content-range").ok_or_else(|| { - RemoteError::BadResponse(format!("{}: 206 without Content-Range", self.url)) + RemoteError::BadResponse(format!( + "{}: 206 without Content-Range", + self.redactor.shown() + )) })?; let (a, b, total) = content_range(cr).ok_or_else(|| { - RemoteError::BadResponse(format!("{}: bad Content-Range {cr:?}", self.url)) + RemoteError::BadResponse(format!( + "{}: bad Content-Range {cr:?}", + self.redactor.shown() + )) })?; if let Some(total) = total && total != self.len @@ -452,7 +483,7 @@ impl HttpStorage { if a != start || b != end - 1 { return Err(RemoteError::BadResponse(format!( "{}: asked for bytes {start}-{}, got Content-Range {cr:?}", - self.url, + self.redactor.shown(), end - 1 ))); } @@ -461,6 +492,7 @@ impl HttpStorage { fn fetch(&self, start: u64, end: u64) -> Result, RemoteError> { self.with_retries(|| self.fetch_once(start, end)) + .map_err(|e| e.scrubbed(&self.redactor)) } /// `[offset, offset + len)` clamped to the file, or `None` if empty. @@ -539,7 +571,7 @@ impl Storage for HttpStorage { if out.len() != ranges.len() { return Err(FormatError::Storage(format!( "{}: a parallel range read failed", - self.url + self.redactor.shown() ))); } Ok(out) diff --git a/crates/clawhdf5-remote/src/lib.rs b/crates/clawhdf5-remote/src/lib.rs index a441bab..cedbfe9 100644 --- a/crates/clawhdf5-remote/src/lib.rs +++ b/crates/clawhdf5-remote/src/lib.rs @@ -45,7 +45,7 @@ use clawhdf5::File; use clawhdf5_format::storage::Storage; pub use cache::{BlockCache, CacheConfig, CacheStats}; -pub use error::{Error, RemoteError}; +pub use error::{Error, RemoteError, redact_url}; #[cfg(feature = "http")] pub use http::{HttpOptions, HttpStats, HttpStorage}; #[cfg(feature = "object-store")] @@ -93,13 +93,13 @@ pub fn storage_for_url(url: &str, options: &Options) -> Result http_storage(url, options), "s3" | "s3a" | "gs" | "az" | "azure" | "abfs" | "abfss" | "adl" => { cloud_storage(url, &scheme, options) } - _ => Err(RemoteError::UnsupportedScheme(url.to_string()).into()), + _ => Err(RemoteError::UnsupportedScheme(redact_url(url)).into()), } } @@ -116,7 +116,8 @@ fn http_storage(url: &str, options: &Options) -> Result, Erro #[cfg(not(feature = "http"))] fn http_storage(url: &str, _options: &Options) -> Result, Error> { Err(RemoteError::UnsupportedScheme(format!( - "{url}: http(s):// needs the `http` feature of clawhdf5-remote" + "{}: http(s):// needs the `http` feature of clawhdf5-remote", + redact_url(url) )) .into()) } @@ -136,7 +137,8 @@ fn cloud_storage(url: &str, scheme: &str, _options: &Options) -> Result "azure", }; Err(RemoteError::UnsupportedScheme(format!( - "{url}: {scheme}:// needs the `{feature}` feature of clawhdf5-remote" + "{}: {scheme}:// needs the `{feature}` feature of clawhdf5-remote", + redact_url(url) )) .into()) } diff --git a/crates/clawhdf5-remote/src/object.rs b/crates/clawhdf5-remote/src/object.rs index 6bafe8d..c8c5664 100644 --- a/crates/clawhdf5-remote/src/object.rs +++ b/crates/clawhdf5-remote/src/object.rs @@ -239,13 +239,14 @@ impl Storage for ObjectStoreStorage { /// `GOOGLE_*`, `AZURE_*`). #[cfg(any(feature = "s3", feature = "gcs", feature = "azure"))] pub(crate) fn store_for_url(url: &str) -> Result<(Arc, Path), RemoteError> { + let redactor = crate::error::Redactor::new(url); let parsed = object_store::path::Path::parse( url.split_once("://") .and_then(|(_, rest)| rest.split_once('/')) .map(|(_, key)| key) .unwrap_or(""), ) - .map_err(|e| RemoteError::InvalidUrl(format!("{url}: {e}")))?; + .map_err(|e| RemoteError::InvalidUrl(format!("{}: {e}", crate::redact_url(url))))?; let scheme = url.split_once("://").map(|(s, _)| s.to_ascii_lowercase()); let store: Arc = match scheme.as_deref() { #[cfg(feature = "s3")] @@ -253,23 +254,23 @@ pub(crate) fn store_for_url(url: &str) -> Result<(Arc, Path), R object_store::aws::AmazonS3Builder::from_env() .with_url(url) .build() - .map_err(os_error)?, + .map_err(|e| os_error(e).scrubbed(&redactor))?, ), #[cfg(feature = "gcs")] Some("gs") => Arc::new( object_store::gcp::GoogleCloudStorageBuilder::from_env() .with_url(url) .build() - .map_err(os_error)?, + .map_err(|e| os_error(e).scrubbed(&redactor))?, ), #[cfg(feature = "azure")] Some("az" | "azure" | "abfs" | "abfss" | "adl") => Arc::new( object_store::azure::MicrosoftAzureBuilder::from_env() .with_url(url) .build() - .map_err(os_error)?, + .map_err(|e| os_error(e).scrubbed(&redactor))?, ), - _ => return Err(RemoteError::UnsupportedScheme(url.to_string())), + _ => return Err(RemoteError::UnsupportedScheme(crate::redact_url(url))), }; Ok((store, parsed)) } diff --git a/crates/clawhdf5-remote/tests/common/server.rs b/crates/clawhdf5-remote/tests/common/server.rs index 13f279a..2cdd7c1 100644 --- a/crates/clawhdf5-remote/tests/common/server.rs +++ b/crates/clawhdf5-remote/tests/common/server.rs @@ -52,6 +52,11 @@ pub struct Shared { /// when checking ranges) and serve zeros past its real end: a hostile /// server lying about the length. pub fake_total: AtomicU64, + /// When non-zero, answer every request for a served file with this + /// status (and an empty body). + pub force_status: AtomicU32, + /// Answer ranges with a `Content-Range` one byte off. + pub wrong_range: AtomicBool, /// Requests for a served path (every status); requests for other /// paths are not counted. pub requests: AtomicU64, @@ -256,6 +261,11 @@ fn serve(conn: TcpStream, s: &Shared) -> std::io::Result<()> { if delay > 0 { std::thread::sleep(Duration::from_millis(delay)); } + let forced = s.force_status.load(Ordering::SeqCst); + if forced != 0 { + write!(out, "HTTP/1.1 {forced} Forced\r\nContent-Length: 0\r\n\r\n")?; + continue; + } if s.fail_next .fetch_update(Ordering::SeqCst, Ordering::SeqCst, |n| n.checked_sub(1)) .is_ok() @@ -309,7 +319,11 @@ fn serve(conn: TcpStream, s: &Shared) -> std::io::Result<()> { Some(Ok((a, b))) => ( "206 Partial Content", slice_or_zeros(&data, a, b, &mut padded), - format!("Content-Range: bytes {a}-{b}/{len}\r\n"), + if s.wrong_range.load(Ordering::SeqCst) { + format!("Content-Range: bytes {}-{}/{len}\r\n", a + 1, b + 1) + } else { + format!("Content-Range: bytes {a}-{b}/{len}\r\n") + }, ), None => ("200 OK", &data[..], String::new()), }; diff --git a/crates/clawhdf5-remote/tests/http.rs b/crates/clawhdf5-remote/tests/http.rs index b4411b6..c61ee6e 100644 --- a/crates/clawhdf5-remote/tests/http.rs +++ b/crates/clawhdf5-remote/tests/http.rs @@ -604,3 +604,121 @@ fn download_is_bounded_by_its_limit_not_the_claimed_length() { ); assert_eq!(server.requests(), 0, "refused before reading"); } + +/// A URL's credentials — userinfo, and the query string of a presigned +/// URL — never appear in an error message or a `Debug` output, whatever +/// failed. +#[test] +fn credentials_never_appear_in_errors_or_debug() { + const SECRETS: [&str; 4] = ["hunter2", "SECRETSIG", "AKIDSECRET", "user:"]; + fn clean(what: &str, text: &str) { + for s in SECRETS { + assert!(!text.contains(s), "{what}: {s} leaked in {text}"); + } + } + fn check_err(what: &str, r: Result) { + let Err(e) = r else { + panic!("{what}: expected an error") + }; + clean(what, &format!("{e}")); + clean(what, &format!("{e:?}")); + } + let bytes = multi_block_file(); + let server = Server::start(vec![("/m.h5".into(), bytes.clone())]); + let secret_url = |path: &str| { + format!( + "http://user:hunter2@{}{path}?X-Amz-Credential=AKIDSECRET&X-Amz-Signature=SECRETSIG", + server.addr + ) + }; + let url = secret_url("/m.h5"); + let mut opts = quick(); + opts.http.retries = 0; + + // Opening works through such a URL, and Debug shows it redacted. + let (http, _) = HttpStorage::open(&url, opts.http.clone()).unwrap(); + let debug = format!("{http:?}"); + clean("Debug", &debug); + assert!(debug.contains("X-Amz-Signature=REDACTED"), "{debug}"); + assert_eq!(http.url(), url, "the URL itself is kept for requests"); + + check_err("404", open_url_with(&secret_url("/missing.h5"), &opts)); + server.shared.force_status.store(403, Ordering::SeqCst); + check_err("403", open_url_with(&url, &opts)); + server.shared.force_status.store(0, Ordering::SeqCst); + server.shared.wrong_range.store(true, Ordering::SeqCst); + check_err("bad range at open", open_url_with(&url, &opts)); + server.shared.wrong_range.store(false, Ordering::SeqCst); + server.shared.ignore_range.store(true, Ordering::SeqCst); + check_err("no range support", open_url_with(&url, &opts)); + server.shared.ignore_range.store(false, Ordering::SeqCst); + server.shared.gzip_label.store(true, Ordering::SeqCst); + check_err("encoded body", open_url_with(&url, &opts)); + server.shared.gzip_label.store(false, Ordering::SeqCst); + + // Errors of reads after the open, through a File. + let f = open_url_with(&url, &opts).unwrap(); + server.shared.wrong_range.store(true, Ordering::SeqCst); + check_err( + "bad range", + f.dataset("big") + .map(|d| d.read_f64()) + .and_then(|r| r) + .map_err(Error::Hdf5), + ); + server.shared.wrong_range.store(false, Ordering::SeqCst); + let f = open_url_with(&url, &opts).unwrap(); + server.shared.force_status.store(403, Ordering::SeqCst); + check_err( + "403 on a read", + f.dataset("big") + .map(|d| d.read_f64()) + .and_then(|r| r) + .map_err(Error::Hdf5), + ); + server.shared.force_status.store(0, Ordering::SeqCst); + let f = open_url_with(&url, &opts).unwrap(); + let mut other = bytes.clone(); + let n = other.len(); + other[n / 2] ^= 0xff; + server.shared.put("/m.h5", other); + check_err( + "ETag change", + f.dataset("big") + .map(|d| d.read_f64()) + .and_then(|r| r) + .map_err(Error::Hdf5), + ); + // A timeout. + let mut slow = opts.clone(); + slow.http.timeout = Duration::from_millis(100); + server.shared.delay_ms.store(1000, Ordering::SeqCst); + check_err("timeout", open_url_with(&url, &slow)); + server.shared.delay_ms.store(0, Ordering::SeqCst); + // A connection closed unanswered, a bad scheme. + let port = { + let l = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let port = l.local_addr().unwrap().port(); + std::thread::spawn(move || { + for c in l.incoming() { + drop(c); + } + }); + port + }; + check_err( + "connection closed", + open_url_with( + &format!("http://user:hunter2@127.0.0.1:{port}/a.h5?X-Amz-Signature=SECRETSIG"), + &opts, + ), + ); + check_err( + "scheme", + open_url("ftp://user:hunter2@example.com/a.h5?X-Amz-Signature=SECRETSIG"), + ); + assert_eq!( + clawhdf5_remote::redact_url("https://me:pw@host:8/d/f.h5?X-Amz-Signature=abc&a=1#frag"), + "https://host:8/d/f.h5?X-Amz-Signature=REDACTED&a=REDACTED" + ); +} diff --git a/crates/clawhdf5-tools/src/check.rs b/crates/clawhdf5-tools/src/check.rs index ea8e09c..4a36554 100644 --- a/crates/clawhdf5-tools/src/check.rs +++ b/crates/clawhdf5-tools/src/check.rs @@ -204,7 +204,7 @@ pub fn run(args: &mut Args, out: &mut Out) -> std::io::Result { } } if !quiet { - c.summary(&file, out)?; + c.summary(&crate::h5::shown(&file), out)?; } Ok(if c.panicked { 3 diff --git a/crates/clawhdf5-tools/src/diff.rs b/crates/clawhdf5-tools/src/diff.rs index af684c5..d02bb8c 100644 --- a/crates/clawhdf5-tools/src/diff.rs +++ b/crates/clawhdf5-tools/src/diff.rs @@ -196,7 +196,8 @@ pub fn run(args: &mut Args, out: &mut Out) -> std::io::Result { Err(_) => { writeln!( out.e, - "h5rs diff: object <{obj}> could not be found in <{f}>" + "h5rs diff: object <{obj}> could not be found in <{}>", + crate::h5::shown(f) )?; return Ok(2); } @@ -214,7 +215,7 @@ pub fn run(args: &mut Args, out: &mut Out) -> std::io::Result { let entries = match collected { Ok(e) => e, Err(e) => { - writeln!(out.e, "h5rs diff: {f}: {e}")?; + writeln!(out.e, "h5rs diff: {}: {e}", crate::h5::shown(f))?; return Ok(2); } }; diff --git a/crates/clawhdf5-tools/src/dump.rs b/crates/clawhdf5-tools/src/dump.rs index 4b59f00..d7076e0 100644 --- a/crates/clawhdf5-tools/src/dump.rs +++ b/crates/clawhdf5-tools/src/dump.rs @@ -98,6 +98,7 @@ pub fn run(args: &mut Args, out: &mut Out) -> std::io::Result { problems: 0, paths: OnceCell::new(), }; + let file = crate::h5::shown(&file); let fname = std::path::Path::new(&file) .file_name() .map(|s| s.to_string_lossy().into_owned()) diff --git a/crates/clawhdf5-tools/src/h5.rs b/crates/clawhdf5-tools/src/h5.rs index d840074..c130f1f 100644 --- a/crates/clawhdf5-tools/src/h5.rs +++ b/crates/clawhdf5-tools/src/h5.rs @@ -229,20 +229,21 @@ impl H5 { if !is_url(arg) { return H5::open(Path::new(arg)); } + let name = shown(arg); #[cfg(feature = "remote")] { let storage = clawhdf5_remote::storage_for_url(arg, &clawhdf5_remote::Options::default()) - .map_err(|e| Error::new(format!("{arg}: {e}")))?; + .map_err(|e| Error::new(format!("{name}: {e}")))?; let size = storage.len(); let file = File::open_storage(storage).map_err(|e| { - Error::new(format!("{arg}: not an HDF5 file this tool can open: {e}")) + Error::new(format!("{name}: not an HDF5 file this tool can open: {e}")) })?; - Ok(H5::new(PathBuf::from(arg), file, size)) + Ok(H5::new(PathBuf::from(&name), file, size)) } #[cfg(not(feature = "remote"))] Err(Error::new(format!( - "{arg}: URLs need h5rs built with the `remote` feature" + "{name}: URLs need h5rs built with the `remote` feature" ))) } @@ -256,18 +257,19 @@ impl H5 { if h5.file.contiguous_bytes().is_some() { return Ok(h5); } + let name = shown(arg); #[cfg(feature = "remote")] { let storage = clawhdf5_remote::storage_for_url(arg, &clawhdf5_remote::Options::default()) - .map_err(|e| Error::new(format!("{arg}: {e}")))?; + .map_err(|e| Error::new(format!("{name}: {e}")))?; let bytes = clawhdf5_remote::download(&*storage, max_download) - .map_err(|e| Error::new(format!("{arg}: {e}")))?; + .map_err(|e| Error::new(format!("{name}: {e}")))?; let size = bytes.len() as u64; let file = File::from_bytes(bytes).map_err(|e| { - Error::new(format!("{arg}: not an HDF5 file this tool can open: {e}")) + Error::new(format!("{name}: not an HDF5 file this tool can open: {e}")) })?; - Ok(H5::new(PathBuf::from(arg), file, size)) + Ok(H5::new(PathBuf::from(&name), file, size)) } #[cfg(not(feature = "remote"))] { @@ -777,6 +779,28 @@ pub fn split_file_arg(arg: &str) -> (String, Option) { (arg.to_string(), None) } +/// A FILE argument as it may be printed: a URL without its credentials +/// (userinfo, query string values — a presigned URL's signature), a path +/// as given. +pub fn shown(arg: &str) -> String { + if !is_url(arg) { + return arg.to_string(); + } + #[cfg(feature = "remote")] + { + clawhdf5_remote::redact_url(arg) + } + #[cfg(not(feature = "remote"))] + { + let (scheme, rest) = arg.split_once("://").unwrap_or(("", arg)); + let rest = rest.split(['?', '#']).next().unwrap_or(""); + let host_end = rest.find('/').unwrap_or(rest.len()); + let (authority, path) = rest.split_at(host_end); + let host = authority.rsplit_once('@').map_or(authority, |(_, h)| h); + format!("{scheme}://{host}{path}") + } +} + /// Whether a FILE argument is a URL (`scheme://...`) rather than a path. pub fn is_url(arg: &str) -> bool { arg.split_once("://").is_some_and(|(scheme, _)| { diff --git a/crates/clawhdf5-tools/src/stat.rs b/crates/clawhdf5-tools/src/stat.rs index 32e91fe..611fdf2 100644 --- a/crates/clawhdf5-tools/src/stat.rs +++ b/crates/clawhdf5-tools/src/stat.rs @@ -206,7 +206,7 @@ pub fn run(args: &mut Args, out: &mut Out) -> std::io::Result { if let Err(e) = walk { errors.push(e.to_string()); } - report(&h5, &file, &s, out)?; + report(&h5, &crate::h5::shown(&file), &s, out)?; for e in &errors { writeln!(out.e, "h5rs stat: {e}")?; } diff --git a/crates/clawhdf5-tools/tests/remote.rs b/crates/clawhdf5-tools/tests/remote.rs index 426d1f7..0ae0b40 100644 --- a/crates/clawhdf5-tools/tests/remote.rs +++ b/crates/clawhdf5-tools/tests/remote.rs @@ -119,3 +119,42 @@ fn check_refuses_a_remote_file_beyond_the_download_limit() { assert_eq!(rc, 2, "{out}"); assert!(out.contains("more than the download limit"), "{out}"); } + +/// A URL's credentials (userinfo, a presigned URL's query string) are not +/// printed: not in errors, not in the file name of the output. +#[test] +fn credentials_in_urls_are_not_printed() { + let tall = Path::new(env!("CARGO_MANIFEST_DIR")).join("../clawhdf5/tests/fixtures/tall.h5"); + let server = server::Server::start(vec![("/t.h5".into(), std::fs::read(&tall).unwrap())]); + let url = |path: &str| { + format!( + "http://user:hunter2@{}{path}?X-Amz-Signature=SECRETSIG", + server.addr + ) + }; + for args in [ + vec!["ls", "-r"], + vec!["dump"], + vec!["stat"], + vec!["check"], + vec!["check", "--max-download", "10"], + ] { + for path in ["/t.h5", "/missing.h5"] { + let u = url(path); + let mut a = args.clone(); + a.push(&u); + let (out, _) = h5rs(&a); + assert!( + !out.contains("hunter2") && !out.contains("SECRETSIG"), + "h5rs {}: {out}", + a.join(" ") + ); + } + } + let (out, rc) = h5rs(&["diff", tall.to_str().unwrap(), &url("/t.h5"), "/nope"]); + assert_eq!(rc, 2, "{out}"); + assert!( + !out.contains("hunter2") && !out.contains("SECRETSIG"), + "{out}" + ); +}