diff --git a/Cargo.lock b/Cargo.lock index 5d7f296788fcc..126e0d716ce52 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5927,6 +5927,7 @@ dependencies = [ "schemars", "serde", "serde_json", + "temp-env", "thiserror 2.0.17", "toml", "tracing", diff --git a/crates/uv-auth/src/lib.rs b/crates/uv-auth/src/lib.rs index e1fe92f17ab59..7820ed1ed3be1 100644 --- a/crates/uv-auth/src/lib.rs +++ b/crates/uv-auth/src/lib.rs @@ -44,6 +44,16 @@ pub(crate) static CREDENTIALS_CACHE: LazyLock = pub fn store_credentials_from_url(url: &DisplaySafeUrl) -> bool { if let Some(credentials) = Credentials::from_url(url) { trace!("Caching credentials for {url}"); + + // If credentials already exist in the cache for this URL, do not override them + // with URL-derived credentials. This ensures credentials provided via environment + // variables (or other higher-precedence sources) are not replaced by credentials + // embedded in an index URL. + if CREDENTIALS_CACHE.get_url(url, &Username::none()).is_some() { + trace!("Skipping caching credentials for {url}: credentials already present"); + return false; + } + CREDENTIALS_CACHE.insert(url, Arc::new(Authentication::from(credentials))); true } else { @@ -58,3 +68,32 @@ pub fn store_credentials(url: &DisplaySafeUrl, credentials: Credentials) { trace!("Caching credentials for {url}"); CREDENTIALS_CACHE.insert(url, Arc::new(Authentication::from(credentials))); } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_store_credentials_from_url_does_not_override_existing_cache() { + use crate::credentials::Credentials; + + let base = DisplaySafeUrl::parse("https://example.com/simple/real").unwrap(); + // Seed the cache with env-var-like credentials using store_credentials + let env_creds = + Credentials::basic(Some("envuser".to_string()), Some("envpass".to_string())); + store_credentials(&base, env_creds.clone()); + + // Construct a URL with embedded credentials that would otherwise override the cache + let mut url_with_creds = base.clone(); + let _ = url_with_creds.set_username("urluser"); + let _ = url_with_creds.set_password(Some("urlpass")); + + // Attempt to store credentials from the URL; expect we will not override existing env creds + assert!(!store_credentials_from_url(&url_with_creds)); + + // Ensure the cached credentials are the original env creds + let cached = CREDENTIALS_CACHE.get_url(&base, &Username::none()).unwrap(); + assert_eq!(cached.username(), env_creds.username()); + assert_eq!(cached.password(), env_creds.password()); + } +} diff --git a/crates/uv-distribution-types/Cargo.toml b/crates/uv-distribution-types/Cargo.toml index 4963cac18f9bf..000e91f62e34a 100644 --- a/crates/uv-distribution-types/Cargo.toml +++ b/crates/uv-distribution-types/Cargo.toml @@ -53,6 +53,7 @@ version-ranges = { workspace = true } [dev-dependencies] toml = { workspace = true } +temp-env = { workspace = true } [features] schemars = ["dep:schemars", "uv-redacted/schemars"] diff --git a/crates/uv-distribution-types/src/index.rs b/crates/uv-distribution-types/src/index.rs index 2090f8ba85a5b..bfe1c561b3aa0 100644 --- a/crates/uv-distribution-types/src/index.rs +++ b/crates/uv-distribution-types/src/index.rs @@ -6,6 +6,8 @@ use thiserror::Error; use url::Url; use uv_auth::{AuthPolicy, Credentials}; +// Avoid importing `uv_static` here as it's not a dependency of this crate. We construct the +// environment variable names directly using the same pattern as `uv_static::EnvVars`. use uv_redacted::DisplaySafeUrl; use uv_small_str::SmallString; @@ -346,15 +348,54 @@ impl Index { /// Retrieve the credentials for the index, either from the environment, or from the URL itself. pub fn credentials(&self) -> Option { - // If the index is named, and credentials are provided via the environment, prefer those. + // Extract credentials from the index URL, if any. + let url_credentials = Credentials::from_url(self.url.url()); + + // If the index is named, and credentials are provided via the environment, we should + // prefer any provided environment variable values. Specifically, environment variables + // should override the username in the index URL. We also merge values when possible, + // such that an environment-provided password can be combined with a URL-provided + // username (and vice-versa), while environment-provided values take precedence. if let Some(name) = self.name.as_ref() { - if let Some(credentials) = Credentials::from_env(name.to_env_var()) { - return Some(credentials); + let name_env = name.to_env_var(); + let username_env_key = format!("UV_INDEX_{name_env}_USERNAME"); + let password_env_key = format!("UV_INDEX_{name_env}_PASSWORD"); + + let username_present = std::env::var_os(&username_env_key).is_some(); + let password_present = std::env::var_os(&password_env_key).is_some(); + + if username_present || password_present { + // Read the actual environment values (may be empty string). + let username_env = std::env::var(&username_env_key).ok(); + let password_env = std::env::var(&password_env_key).ok(); + + // If the environment provided a username, use that. Otherwise, if the URL + // provided a username, use the URL's username. If neither provided one, the + // username is None. + let username = if username_present { + username_env + } else { + url_credentials + .as_ref() + .and_then(|creds| creds.username().map(ToString::to_string)) + }; + + // If the environment provided a password, use that. Otherwise, if the URL + // provided a password, use the URL's password. + let password = if password_present { + password_env + } else { + url_credentials + .as_ref() + .and_then(|creds| creds.password().map(ToString::to_string)) + }; + + return Some(Credentials::basic(username, password)); } } // Otherwise, extract the credentials from the URL. - Credentials::from_url(self.url.url()) + url_credentials } /// Resolve the index relative to the given root directory. @@ -603,4 +644,69 @@ mod tests { assert_eq!(cache_control.api.as_deref(), Some("max-age=300")); assert_eq!(cache_control.files, None); } + + #[test] + fn test_index_credentials_env_username_overrides_url_username() { + use crate::IndexUrl; + use temp_env::with_vars; + const USERNAME_ENV: &str = "UV_INDEX_PRIVATE_REGISTRY_USERNAME"; + const PASSWORD_ENV: &str = "UV_INDEX_PRIVATE_REGISTRY_PASSWORD"; + + // Set env var username and clear password env var using with_vars to ensure safe testing + with_vars( + vec![(USERNAME_ENV, Some("envuser")), (PASSWORD_ENV, None)], + || { + // Index URL includes credentials + let url = IndexUrl::from_str("https://urluser:pass@example.com/simple").unwrap(); + let mut index = Index::from(url); + index.name = Some(IndexName::new("private-registry").unwrap()); + + let creds = index.credentials().unwrap(); + assert_eq!(creds.username(), Some("envuser")); + assert_eq!(creds.password(), Some("pass")); + }, + ); + } + + #[test] + fn test_index_credentials_env_password_merges_with_url_username() { + use crate::IndexUrl; + use temp_env::with_vars; + const USERNAME_ENV: &str = "UV_INDEX_PRIVATE_REGISTRY_USERNAME"; + const PASSWORD_ENV: &str = "UV_INDEX_PRIVATE_REGISTRY_PASSWORD"; + + with_vars( + vec![(USERNAME_ENV, None), (PASSWORD_ENV, Some("envpass"))], + || { + let url = IndexUrl::from_str("https://urluser:pass@example.com/simple").unwrap(); + let mut index = Index::from(url); + index.name = Some(IndexName::new("private-registry").unwrap()); + + let creds = index.credentials().unwrap(); + assert_eq!(creds.username(), Some("urluser")); + assert_eq!(creds.password(), Some("envpass")); + }, + ); + } + + #[test] + fn test_index_credentials_env_username_empty_string_overrides_url_username() { + use crate::IndexUrl; + use temp_env::with_vars; + const USERNAME_ENV: &str = "UV_INDEX_PRIVATE_REGISTRY_USERNAME"; + const PASSWORD_ENV: &str = "UV_INDEX_PRIVATE_REGISTRY_PASSWORD"; + + with_vars( + vec![(USERNAME_ENV, Some("")), (PASSWORD_ENV, Some("envtoken"))], + || { + let url = IndexUrl::from_str("https://urluser:pass@example.com/simple").unwrap(); + let mut index = Index::from(url); + index.name = Some(IndexName::new("private-registry").unwrap()); + + let creds = index.credentials().unwrap(); + assert_eq!(creds.username(), None); + assert_eq!(creds.password(), Some("envtoken")); + }, + ); + } } diff --git a/crates/uv-git/src/credentials.rs b/crates/uv-git/src/credentials.rs index 0515609800c11..a381146019d6e 100644 --- a/crates/uv-git/src/credentials.rs +++ b/crates/uv-git/src/credentials.rs @@ -32,9 +32,45 @@ impl GitStore { pub fn store_credentials_from_url(url: &DisplaySafeUrl) -> bool { if let Some(credentials) = Credentials::from_url(url) { trace!("Caching credentials for {url}"); - GIT_STORE.insert(RepositoryUrl::new(url), credentials); + // If the store already contains credentials for this repository, don't override them + // with credentials embedded in the URL. This respects higher-precedence credential + // sources such as environment variables and keyrings. + let repo = RepositoryUrl::new(url); + if GIT_STORE.get(&repo).is_some() { + trace!("Skipping caching Git credentials for {url}: credentials already present"); + return false; + } + GIT_STORE.insert(repo, credentials); true } else { false } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_store_credentials_from_url_does_not_override_git_store() { + use uv_auth::Credentials; + + let base = DisplaySafeUrl::parse("https://github.com/astral-sh/uv").unwrap(); + let repo = RepositoryUrl::new(&base); + // Seed the store with env-var-like creds + let env_creds = + Credentials::basic(Some("envuser".to_string()), Some("envpass".to_string())); + GIT_STORE.insert(repo.clone(), env_creds.clone()); + + // URL with embedded credentials that would have overridden + let mut url_with_creds = base.clone(); + let _ = url_with_creds.set_username("urluser"); + let _ = url_with_creds.set_password(Some("urlpass")); + + assert!(!store_credentials_from_url(&url_with_creds)); + + let cached = GIT_STORE.get(&repo).unwrap(); + assert_eq!(cached.username(), env_creds.username()); + assert_eq!(cached.password(), env_creds.password()); + } +}