diff --git a/rs/moq-auth/src/client.rs b/rs/moq-auth/src/client.rs index cd07c3f06a..2ba037af41 100644 --- a/rs/moq-auth/src/client.rs +++ b/rs/moq-auth/src/client.rs @@ -43,7 +43,7 @@ impl Client { None => false, }; if !loopback { - return Err(Error::InsecureUrl(url.to_string())); + return Err(Error::InsecureUrl(redact(&url))); } (builder, url) } @@ -53,12 +53,12 @@ impl Client { }, #[cfg(unix)] "unix" => { - let path = url.to_file_path().map_err(|()| Error::InvalidUrl(url.to_string()))?; + let path = url.to_file_path().map_err(|()| Error::InvalidUrl(redact(&url)))?; // The socket is the transport; the request target is the server's root. let target = Url::parse("http://localhost/").expect("a constant URL parses"); (builder.unix_socket(path), target) } - _ => return Err(Error::InvalidUrl(url.to_string())), + _ => return Err(Error::InvalidUrl(redact(&url))), }; Ok(Self { @@ -143,6 +143,16 @@ async fn ask(server: &S, request: &Request) -> crate::Result { Ok(grant) } +/// `url` without its userinfo, query, or fragment, any of which may carry a credential. +fn redact(url: &Url) -> String { + let mut url = url.clone(); + let _ = url.set_username(""); + let _ = url.set_password(None); + url.set_query(None); + url.set_fragment(None); + url.to_string() +} + /// The task behind a lease: re-checks on cadence and reports the end. struct Driver { server: S, @@ -458,6 +468,23 @@ mod tests { assert!(matches!(err, Error::Refused), "{err}"); } + #[tokio::test] + async fn http_error_redacts_url() { + // A freed port refuses the connection, so reqwest fails with the dialed URL attached. + let listener = std::net::TcpListener::bind("127.0.0.1:0").expect("bind"); + let address = listener.local_addr().expect("local address"); + drop(listener); + + let url = format!("http://user:pass@{address}/?jwt=secret").parse().unwrap(); + let err = Client::new(url, None).unwrap().connect(request()).await.unwrap_err(); + + assert!(matches!(err, Error::Unavailable(_)), "unexpected error: {err}"); + let printed = format!("{err} {err:?}"); + for secret in ["jwt", "secret", "user:pass"] { + assert!(!printed.contains(secret), "error leaked {secret}: {printed}"); + } + } + #[tokio::test] async fn revalidate_runs_on_cadence_and_applies_the_reply() { let log = Log::default(); @@ -639,6 +666,19 @@ mod tests { )); #[cfg(unix)] assert!(Client::new("unix:///run/moq-auth.sock".parse().unwrap(), None).is_ok()); + + // The refused URL is reported, minus anything that may carry a credential. + for url in [ + "http://user:pass@auth.example/?jwt=secret#frag", + "ftp://user:pass@auth.example/?jwt=secret#frag", + ] { + let err = Client::new(url.parse().unwrap(), None).err().expect("refused"); + let printed = format!("{err} {err:?}"); + assert!(printed.contains("auth.example/"), "{printed}"); + for secret in ["jwt", "secret", "user", "pass", "frag"] { + assert!(!printed.contains(secret), "error leaked {secret}: {printed}"); + } + } } /// Backoff leaves the driver in this same state (nothing in flight, a timer armed), diff --git a/rs/moq-auth/src/error.rs b/rs/moq-auth/src/error.rs index fcd36d3496..c32d0d06b1 100644 --- a/rs/moq-auth/src/error.rs +++ b/rs/moq-auth/src/error.rs @@ -178,8 +178,12 @@ from_message! { } #[cfg(feature = "client")] -from_message! { - reqwest::Error => Unavailable, +impl From for Error { + fn from(err: reqwest::Error) -> Self { + // reqwest prints the full URL in its error, and a dialed URL can carry + // credentials in its query or userinfo. + Self::Unavailable(message(err.without_url())) + } } pub type Result = std::result::Result; diff --git a/rs/moq-hls/src/error.rs b/rs/moq-hls/src/error.rs index a87420bcd3..2d909b7960 100644 --- a/rs/moq-hls/src/error.rs +++ b/rs/moq-hls/src/error.rs @@ -141,7 +141,9 @@ impl Error { impl From for Error { fn from(err: reqwest::Error) -> Self { - Error::Reqwest(std::sync::Arc::new(err)) + // reqwest prints the full URL in its error, and a playlist or segment URL + // routinely carries a signed token in its query. + Error::Reqwest(std::sync::Arc::new(err.without_url())) } } @@ -166,4 +168,22 @@ mod tests { assert_eq!(Error::ParsePlaylist("not a playlist".to_string()).status(), None); assert_eq!(Error::Moq(moq_net::Error::Transport("lost".to_string())).status(), None); } + + #[tokio::test] + async fn http_error_redacts_url() { + // A freed port refuses the connection, so reqwest fails with the dialed URL attached. + let listener = std::net::TcpListener::bind("127.0.0.1:0").expect("bind"); + let address = listener.local_addr().expect("local address"); + drop(listener); + + let err: Error = reqwest::get(format!("http://user:pass@{address}/media.m3u8?jwt=secret")) + .await + .expect_err("dial a closed port") + .into(); + + let printed = format!("{err} {err:?}"); + for secret in ["jwt", "secret", "user:pass"] { + assert!(!printed.contains(secret), "error leaked {secret}: {printed}"); + } + } }