pioduino/uv
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

Add DisplaySafeUrl newtype to prevent leaking of credentials by default (#13560)

Browse Source 
Prior to this PR, there were numerous places where uv would leak
credentials in logs. We had a way to mask credentials by calling methods
or a recently-added `redact_url` function, but this was not secure by
default. There were a number of other types (like `GitUrl`) that would
leak credentials on display.

This PR adds a `DisplaySafeUrl` newtype to prevent leaking credentials
when logging by default. It takes a maximalist approach, replacing the
use of `Url` almost everywhere. This includes when first parsing config
files, when storing URLs in types like `GitUrl`, and also when storing
URLs in types that in practice will never contain credentials (like
`DirectorySourceUrl`). The idea is to make it easy for developers to do
the right thing and for the compiler to support this (and to minimize
ever having to manually convert back and forth). Displaying credentials
now requires an active step. Note that despite this maximalist approach,
the use of the newtype should be zero cost.

One conspicuous place this PR does not use `DisplaySafeUrl` is in the
`uv-auth` crate. That would require new clones since there are calls to
`request.url()` that return a `&Url`. One option would have been to make
`DisplaySafeUrl` wrap a `Cow`, but this would lead to lifetime
annotations all over the codebase. I've created a separate PR based on
this one (#13576) that updates `uv-auth` to use `DisplaySafeUrl` with
one new clone. We can discuss the tradeoffs there.

Most of this PR just replaces `Url` with `DisplaySafeUrl`. The core is
`uv_redacted/lib.rs`, where the newtype is implemented. To make it
easier to review the rest, here are some points of note:

* `DisplaySafeUrl` has a `Display` implementation that masks
credentials. Currently, it will still display the username when there is
both a username and password. If we think is the wrong choice, it can
now be changed in one place.
* `DisplaySafeUrl` has a `remove_credentials()` method and also a
`.to_string_with_credentials()` method. This allows us to use it in a
variety of scenarios.
* `IndexUrl::redacted()` was renamed to
`IndexUrl::removed_credentials()` to make it clearer that we are not
masking.
* We convert from a `DisplaySafeUrl` to a `Url` when calling `reqwest`
methods like `.get()` and `.head()`.
* We convert from a `DisplaySafeUrl` to a `Url` when creating a
`uv_auth::Index`. That is because, as mentioned above, I will be
updating the `uv_auth` crate to use this newtype in a separate PR.
* A number of tests (e.g., in `pip_install.rs`) that formerly used
filters to mask tokens in the test output no longer need those filters
since tokens in URLs are now masked automatically.
* The one place we are still knowingly writing credentials to
`pyproject.toml` is when a URL with credentials is passed to `uv add`
with `--raw`. Since displaying credentials is no longer automatic, I
have added a `to_string_with_credentials()` method to the `Pep508Url`
trait. This is used when `--raw` is passed. Adding it to that trait is a
bit weird, but it's the simplest way to achieve the goal. I'm open to
suggestions on how to improve this, but note that because of the way
we're using generic bounds, it's not as simple as just creating a
separate trait for that method.
This commit is contained in:
John Mumm
2025-05-27 00:05:30 +02:00
committed by GitHub
parent b80cafd5e8
commit c19a294a48
100 changed files with 1266 additions and 2249 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/uv-auth/src/cache.rs
+2 -1
View File
@@ -9,6 +9,7 @@ use tracing::trace;
use url::Url;
use uv_once_map::OnceMap;
use uv_redacted::DisplaySafeUrl;
use crate::Realm;
use crate::credentials::{Credentials, Username};
@@ -18,7 +19,7 @@ type FxOnceMap<K, V> = OnceMap<K, V, BuildHasherDefault<FxHasher>>;
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub(crate) enum FetchUrl {
/// A full index URL
Index(Url),
Index(DisplaySafeUrl),
/// A realm URL
Realm(Realm),
}
crates/uv-auth/src/credentials.rs
+8 -2
View File
@@ -3,6 +3,8 @@ use base64::read::DecoderReader;
use base64::write::EncoderWriter;
use std::borrow::Cow;
use std::fmt;
use uv_redacted::DisplaySafeUrl;
use uv_redacted::DisplaySafeUrlRef;
use netrc::Netrc;
use reqwest::Request;
@@ -141,7 +143,11 @@ impl Credentials {
/// Return [`Credentials`] for a [`Url`] from a [`Netrc`] file, if any.
///
/// If a username is provided, it must match the login in the netrc file or [`None`] is returned.
pub(crate) fn from_netrc(netrc: &Netrc, url: &Url, username: Option<&str>) -> Option<Self> {
pub(crate) fn from_netrc(
netrc: &Netrc,
url: &DisplaySafeUrlRef<'_>,
username: Option<&str>,
) -> Option<Self> {
let host = url.host_str()?;
let entry = netrc
.hosts
@@ -299,7 +305,7 @@ impl Credentials {
///
/// Any existing credentials will be overridden.
#[must_use]
pub fn apply(&self, mut url: Url) -> Url {
pub fn apply(&self, mut url: DisplaySafeUrl) -> DisplaySafeUrl {
if let Some(username) = self.username() {
let _ = url.set_username(username);
}
crates/uv-auth/src/index.rs
+4 -3
View File
@@ -2,6 +2,7 @@ use std::fmt::{self, Display, Formatter};
use rustc_hash::FxHashSet;
use url::Url;
use uv_redacted::DisplaySafeUrl;
/// When to use authentication.
#[derive(
@@ -53,10 +54,10 @@ impl Display for AuthPolicy {
// could potentially make sense for a future refactor.
#[derive(Debug, Clone, Hash, Eq, PartialEq)]
pub struct Index {
pub url: Url,
pub url: DisplaySafeUrl,
/// The root endpoint where authentication is applied.
/// For PEP 503 endpoints, this excludes `/simple`.
pub root_url: Url,
pub root_url: DisplaySafeUrl,
pub auth_policy: AuthPolicy,
}
@@ -95,7 +96,7 @@ impl Indexes {
}
/// Get the index URL prefix for a URL if one exists.
pub fn index_url_for(&self, url: &Url) -> Option<&Url> {
pub fn index_url_for(&self, url: &Url) -> Option<&DisplaySafeUrl> {
self.find_prefix_index(url).map(|index| &index.url)
}
crates/uv-auth/src/keyring.rs
+58 -21
View File
@@ -1,7 +1,7 @@
use std::{io::Write, process::Stdio};
use tokio::process::Command;
use tracing::{instrument, trace, warn};
use url::Url;
use uv_redacted::DisplaySafeUrlRef;
use uv_warnings::warn_user_once;
use crate::credentials::Credentials;
@@ -36,7 +36,11 @@ impl KeyringProvider {
/// Returns [`None`] if no password was found for the username or if any errors
/// are encountered in the keyring backend.
#[instrument(skip_all, fields(url = % url.to_string(), username))]
pub async fn fetch(&self, url: &Url, username: Option<&str>) -> Option<Credentials> {
pub async fn fetch(
&self,
url: &DisplaySafeUrlRef<'_>,
username: Option<&str>,
) -> Option<Credentials> {
// Validate the request
debug_assert!(
url.host_str().is_some(),
@@ -217,15 +221,18 @@ impl KeyringProvider {
mod tests {
use super::*;
use futures::FutureExt;
use url::Url;
#[tokio::test]
async fn fetch_url_no_host() {
let url = Url::parse("file:/etc/bin/").unwrap();
let keyring = KeyringProvider::empty();
// Panics due to debug assertion; returns `None` in production
let result = std::panic::AssertUnwindSafe(keyring.fetch(&url, Some("user")))
.catch_unwind()
.await;
let result = std::panic::AssertUnwindSafe(
keyring.fetch(&DisplaySafeUrlRef::from(&url), Some("user")),
)
.catch_unwind()
.await;
assert!(result.is_err());
}
@@ -234,9 +241,11 @@ mod tests {
let url = Url::parse("https://user:password@example.com").unwrap();
let keyring = KeyringProvider::empty();
// Panics due to debug assertion; returns `None` in production
let result = std::panic::AssertUnwindSafe(keyring.fetch(&url, Some(url.username())))
.catch_unwind()
.await;
let result = std::panic::AssertUnwindSafe(
keyring.fetch(&DisplaySafeUrlRef::from(&url), Some(url.username())),
)
.catch_unwind()
.await;
assert!(result.is_err());
}
@@ -245,15 +254,18 @@ mod tests {
let url = Url::parse("https://example.com").unwrap();
let keyring = KeyringProvider::empty();
// Panics due to debug assertion; returns `None` in production
let result = std::panic::AssertUnwindSafe(keyring.fetch(&url, Some(url.username())))
.catch_unwind()
.await;
let result = std::panic::AssertUnwindSafe(
keyring.fetch(&DisplaySafeUrlRef::from(&url), Some(url.username())),
)
.catch_unwind()
.await;
assert!(result.is_err());
}
#[tokio::test]
async fn fetch_url_no_auth() {
let url = Url::parse("https://example.com").unwrap();
let url = DisplaySafeUrlRef::from(&url);
let keyring = KeyringProvider::empty();
let credentials = keyring.fetch(&url, Some("user"));
assert!(credentials.await.is_none());
@@ -264,7 +276,9 @@ mod tests {
let url = Url::parse("https://example.com").unwrap();
let keyring = KeyringProvider::dummy([(url.host_str().unwrap(), "user", "password")]);
assert_eq!(
keyring.fetch(&url, Some("user")).await,
keyring
.fetch(&DisplaySafeUrlRef::from(&url), Some("user"))
.await,
Some(Credentials::basic(
Some("user".to_string()),
Some("password".to_string())
@@ -272,7 +286,10 @@ mod tests {
);
assert_eq!(
keyring
.fetch(&url.join("test").unwrap(), Some("user"))
.fetch(
&DisplaySafeUrlRef::from(&url.join("test").unwrap()),
Some("user")
)
.await,
Some(Credentials::basic(
Some("user".to_string()),
@@ -285,7 +302,9 @@ mod tests {
async fn fetch_url_no_match() {
let url = Url::parse("https://example.com").unwrap();
let keyring = KeyringProvider::dummy([("other.com", "user", "password")]);
let credentials = keyring.fetch(&url, Some("user")).await;
let credentials = keyring
.fetch(&DisplaySafeUrlRef::from(&url), Some("user"))
.await;
assert_eq!(credentials, None);
}
@@ -297,21 +316,33 @@ mod tests {
(url.host_str().unwrap(), "user", "other-password"),
]);
assert_eq!(
keyring.fetch(&url.join("foo").unwrap(), Some("user")).await,
keyring
.fetch(
&DisplaySafeUrlRef::from(&url.join("foo").unwrap()),
Some("user")
)
.await,
Some(Credentials::basic(
Some("user".to_string()),
Some("password".to_string())
))
);
assert_eq!(
keyring.fetch(&url, Some("user")).await,
keyring
.fetch(&DisplaySafeUrlRef::from(&url), Some("user"))
.await,
Some(Credentials::basic(
Some("user".to_string()),
Some("other-password".to_string())
))
);
assert_eq!(
keyring.fetch(&url.join("bar").unwrap(), Some("user")).await,
keyring
.fetch(
&DisplaySafeUrlRef::from(&url.join("bar").unwrap()),
Some("user")
)
.await,
Some(Credentials::basic(
Some("user".to_string()),
Some("other-password".to_string())
@@ -323,7 +354,9 @@ mod tests {
async fn fetch_url_username() {
let url = Url::parse("https://example.com").unwrap();
let keyring = KeyringProvider::dummy([(url.host_str().unwrap(), "user", "password")]);
let credentials = keyring.fetch(&url, Some("user")).await;
let credentials = keyring
.fetch(&DisplaySafeUrlRef::from(&url), Some("user"))
.await;
assert_eq!(
credentials,
Some(Credentials::basic(
@@ -337,7 +370,7 @@ mod tests {
async fn fetch_url_no_username() {
let url = Url::parse("https://example.com").unwrap();
let keyring = KeyringProvider::dummy([(url.host_str().unwrap(), "user", "password")]);
let credentials = keyring.fetch(&url, None).await;
let credentials = keyring.fetch(&DisplaySafeUrlRef::from(&url), None).await;
assert_eq!(
credentials,
Some(Credentials::basic(
@@ -351,12 +384,16 @@ mod tests {
async fn fetch_url_username_no_match() {
let url = Url::parse("https://example.com").unwrap();
let keyring = KeyringProvider::dummy([(url.host_str().unwrap(), "foo", "password")]);
let credentials = keyring.fetch(&url, Some("bar")).await;
let credentials = keyring
.fetch(&DisplaySafeUrlRef::from(&url), Some("bar"))
.await;
assert_eq!(credentials, None);
// Still fails if we have `foo` in the URL itself
let url = Url::parse("https://foo@example.com").unwrap();
let credentials = keyring.fetch(&url, Some("bar")).await;
let credentials = keyring
.fetch(&DisplaySafeUrlRef::from(&url), Some("bar"))
.await;
assert_eq!(credentials, None);
}
}
crates/uv-auth/src/lib.rs
+3 -3
View File
@@ -1,7 +1,6 @@
use std::sync::{Arc, LazyLock};
use tracing::trace;
use url::Url;
use cache::CredentialsCache;
pub use credentials::Credentials;
@@ -9,6 +8,7 @@ pub use index::{AuthPolicy, Index, Indexes};
pub use keyring::KeyringProvider;
pub use middleware::AuthMiddleware;
use realm::Realm;
use uv_redacted::DisplaySafeUrl;
mod cache;
mod credentials;
@@ -28,7 +28,7 @@ pub(crate) static CREDENTIALS_CACHE: LazyLock<CredentialsCache> =
/// Populate the global authentication store with credentials on a URL, if there are any.
///
/// Returns `true` if the store was updated.
pub fn store_credentials_from_url(url: &Url) -> bool {
pub fn store_credentials_from_url(url: &DisplaySafeUrl) -> bool {
if let Some(credentials) = Credentials::from_url(url) {
trace!("Caching credentials for {url}");
CREDENTIALS_CACHE.insert(url, Arc::new(credentials));
@@ -41,7 +41,7 @@ pub fn store_credentials_from_url(url: &Url) -> bool {
/// Populate the global authentication store with credentials on a URL, if there are any.
///
/// Returns `true` if the store was updated.
pub fn store_credentials(url: &Url, credentials: Arc<Credentials>) {
pub fn store_credentials(url: &DisplaySafeUrl, credentials: Arc<Credentials>) {
trace!("Caching credentials for {url}");
CREDENTIALS_CACHE.insert(url, credentials);
}
crates/uv-auth/src/middleware.rs
+40 -43
View File
@@ -1,7 +1,7 @@
use std::sync::{Arc, LazyLock};
use http::{Extensions, StatusCode};
use url::Url;
use uv_redacted::{DisplaySafeUrl, DisplaySafeUrlRef};
use crate::{
CREDENTIALS_CACHE, CredentialsCache, KeyringProvider,
@@ -274,6 +274,7 @@ impl Middleware for AuthMiddleware {
trace!("Checking for credentials for {url}");
(request, None)
};
let retry_request_url = DisplaySafeUrlRef::from(retry_request.url());
let username = credentials
.as_ref()
@@ -282,13 +283,13 @@ impl Middleware for AuthMiddleware {
let credentials = if let Some(index_url) = maybe_index_url {
self.cache().get_url(index_url, &username).or_else(|| {
self.cache()
.get_realm(Realm::from(retry_request.url()), username)
.get_realm(Realm::from(&*retry_request_url), username)
})
} else {
// Since there is no known index for this URL, check if there are credentials in
// the realm-level cache.
self.cache()
.get_realm(Realm::from(retry_request.url()), username)
.get_realm(Realm::from(&*retry_request_url), username)
}
.or(credentials);
@@ -307,7 +308,7 @@ impl Middleware for AuthMiddleware {
if let Some(credentials) = self
.fetch_credentials(
credentials.as_deref(),
retry_request.url(),
retry_request_url,
maybe_index_url,
auth_policy,
)
@@ -362,7 +363,7 @@ impl AuthMiddleware {
// Nothing to insert into the cache if we don't have credentials
return next.run(request, extensions).await;
};
let url = request.url().clone();
let url = DisplaySafeUrl::from(request.url().clone());
if matches!(auth_policy, AuthPolicy::Always) && credentials.password().is_none() {
return Err(Error::Middleware(format_err!("Missing password for {url}")));
}
@@ -387,8 +388,8 @@ impl AuthMiddleware {
mut request: Request,
extensions: &mut Extensions,
next: Next<'_>,
url: &str,
index_url: Option<&Url>,
url: &DisplaySafeUrl,
index_url: Option<&DisplaySafeUrl>,
auth_policy: AuthPolicy,
) -> reqwest_middleware::Result<Response> {
let credentials = Arc::new(credentials);
@@ -430,7 +431,12 @@ impl AuthMiddleware {
// Do not insert already-cached credentials
None
} else if let Some(credentials) = self
.fetch_credentials(Some(&credentials), request.url(), index_url, auth_policy)
.fetch_credentials(
Some(&credentials),
DisplaySafeUrlRef::from(request.url()),
index_url,
auth_policy,
)
.await
{
request = credentials.authenticate(request);
@@ -462,8 +468,8 @@ impl AuthMiddleware {
async fn fetch_credentials(
&self,
credentials: Option<&Credentials>,
url: &Url,
maybe_index_url: Option<&Url>,
url: DisplaySafeUrlRef<'_>,
maybe_index_url: Option<&DisplaySafeUrl>,
auth_policy: AuthPolicy,
) -> Option<Arc<Credentials>> {
let username = Username::from(
@@ -475,7 +481,7 @@ impl AuthMiddleware {
let key = if let Some(index_url) = maybe_index_url {
(FetchUrl::Index(index_url.clone()), username)
} else {
(FetchUrl::Realm(Realm::from(url)), username)
(FetchUrl::Realm(Realm::from(&*url)), username)
};
if !self.cache().fetches.register(key.clone()) {
let credentials = self
@@ -502,7 +508,7 @@ impl AuthMiddleware {
debug!("Checking netrc for credentials for {url}");
Credentials::from_netrc(
netrc,
url,
&url,
credentials
.as_ref()
.and_then(|credentials| credentials.username()),
@@ -523,17 +529,17 @@ impl AuthMiddleware {
if let Some(username) = credentials.and_then(|credentials| credentials.username()) {
if let Some(index_url) = maybe_index_url {
debug!("Checking keyring for credentials for index URL {}@{}", username, index_url);
keyring.fetch(index_url, Some(username)).await
keyring.fetch(&DisplaySafeUrlRef::from(index_url), Some(username)).await
} else {
debug!("Checking keyring for credentials for full URL {}@{}", username, url);
keyring.fetch(url, Some(username)).await
keyring.fetch(&url, Some(username)).await
}
} else if matches!(auth_policy, AuthPolicy::Always) {
if let Some(index_url) = maybe_index_url {
debug!(
"Checking keyring for credentials for index URL {index_url} without username due to `authenticate = always`"
);
keyring.fetch(index_url, None).await
keyring.fetch(&DisplaySafeUrlRef::from(index_url), None).await
} else {
None
}
@@ -558,24 +564,17 @@ impl AuthMiddleware {
}
}
fn tracing_url(request: &Request, credentials: Option<&Credentials>) -> String {
if !tracing::enabled!(tracing::Level::DEBUG) {
return request.url().to_string();
}
let mut url = request.url().clone();
fn tracing_url(request: &Request, credentials: Option<&Credentials>) -> DisplaySafeUrl {
let mut url = DisplaySafeUrl::from(request.url().clone());
if let Some(creds) = credentials {
if creds.password().is_some() {
if let Some(username) = creds.username() {
let _ = url.set_username(username);
}
let _ = url.set_password(Some("****"));
// A username on its own might be a secret token.
} else if creds.username().is_some() {
let _ = url.set_username("****");
if let Some(username) = creds.username() {
let _ = url.set_username(username);
}
if let Some(password) = creds.password() {
let _ = url.set_password(Some(password));
}
}
url.to_string()
url
}
#[cfg(test)]
@@ -1749,13 +1748,13 @@ mod tests {
let base_url_2 = base_url.join("prefix_2")?;
let indexes = Indexes::from_indexes(vec![
Index {
url: base_url_1.clone(),
root_url: base_url_1.clone(),
url: DisplaySafeUrl::from(base_url_1.clone()),
root_url: DisplaySafeUrl::from(base_url_1.clone()),
auth_policy: AuthPolicy::Auto,
},
Index {
url: base_url_2.clone(),
root_url: base_url_2.clone(),
url: DisplaySafeUrl::from(base_url_2.clone()),
root_url: DisplaySafeUrl::from(base_url_2.clone()),
auth_policy: AuthPolicy::Auto,
},
]);
@@ -1857,8 +1856,8 @@ mod tests {
let base_url = Url::parse(&server.uri())?;
let index_url = base_url.join("prefix_1")?;
let indexes = Indexes::from_indexes(vec![Index {
url: index_url.clone(),
root_url: index_url.clone(),
url: DisplaySafeUrl::from(index_url.clone()),
root_url: DisplaySafeUrl::from(index_url.clone()),
auth_policy: AuthPolicy::Auto,
}]);
@@ -1912,7 +1911,7 @@ mod tests {
}
fn indexes_for(url: &Url, policy: AuthPolicy) -> Indexes {
let mut url = url.clone();
let mut url = DisplaySafeUrl::from(url.clone());
url.set_password(None).ok();
url.set_username("").ok();
Indexes::from_indexes(vec![Index {
@@ -2104,16 +2103,14 @@ mod tests {
}
#[test]
#[tracing_test::traced_test(level = "debug")]
fn test_tracing_url() {
// No credentials
let req = create_request("https://pypi-proxy.fly.dev/basic-auth/simple");
assert_eq!(
tracing_url(&req, None),
"https://pypi-proxy.fly.dev/basic-auth/simple"
DisplaySafeUrl::parse("https://pypi-proxy.fly.dev/basic-auth/simple").unwrap()
);
// Mask username if there is a username but no password
let creds = Credentials::Basic {
username: Username::new(Some(String::from("user"))),
password: None,
@@ -2121,10 +2118,9 @@ mod tests {
let req = create_request("https://pypi-proxy.fly.dev/basic-auth/simple");
assert_eq!(
tracing_url(&req, Some(&creds)),
"https://****@pypi-proxy.fly.dev/basic-auth/simple"
DisplaySafeUrl::parse("https://user@pypi-proxy.fly.dev/basic-auth/simple").unwrap()
);
// Log username but mask password if a password is present
let creds = Credentials::Basic {
username: Username::new(Some(String::from("user"))),
password: Some(Password::new(String::from("password"))),
@@ -2132,7 +2128,8 @@ mod tests {
let req = create_request("https://pypi-proxy.fly.dev/basic-auth/simple");
assert_eq!(
tracing_url(&req, Some(&creds)),
"https://user:****@pypi-proxy.fly.dev/basic-auth/simple"
DisplaySafeUrl::parse("https://user:password@pypi-proxy.fly.dev/basic-auth/simple")
.unwrap()
);
}