From bc1bbfb066fb544d5f9102cdcb49c999491c1923 Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Tue, 2 Sep 2025 08:15:55 -0500 Subject: [PATCH] Respect usernames when finding matching credentials in the plaintext store (#15620) We're not respecting the username when searching for a match, which is no good! --- crates/uv-auth/src/middleware.rs | 73 +++++++++++++++++- crates/uv-auth/src/store.rs | 107 ++++++++++++++++++++++----- crates/uv/src/commands/auth/token.rs | 2 +- crates/uv/tests/it/auth.rs | 100 +++++++++++++++++++++++++ 4 files changed, 263 insertions(+), 19 deletions(-) diff --git a/crates/uv-auth/src/middleware.rs b/crates/uv-auth/src/middleware.rs index 8c877bcda..443389e2a 100644 --- a/crates/uv-auth/src/middleware.rs +++ b/crates/uv-auth/src/middleware.rs @@ -596,7 +596,7 @@ impl AuthMiddleware { // Text credential store support. } else if let Some(credentials) = self.text_store.get().and_then(|text_store| { debug!("Checking text store for credentials for {url}"); - text_store.get_credentials(&url::Url::from(url.clone())).cloned() + text_store.get_credentials(url, credentials.as_ref().and_then(|credentials| credentials.username())).cloned() }) { debug!("Found credentials in text store for {url}"); Some(credentials) @@ -2272,6 +2272,77 @@ mod tests { Ok(()) } + #[test(tokio::test)] + async fn test_text_store_by_username() -> Result<(), Error> { + let username = "testuser"; + let password = "testpass"; + let wrong_username = "wronguser"; + + let server = start_test_server(username, password).await; + let base_url = Url::parse(&server.uri())?; + + let mut store = TextCredentialStore::default(); + let service = crate::Service::try_from(base_url.to_string()).unwrap(); + let credentials = + crate::Credentials::basic(Some(username.to_string()), Some(password.to_string())); + store.insert(service.clone(), credentials); + + let client = test_client_builder() + .with( + AuthMiddleware::new() + .with_cache(CredentialsCache::new()) + .with_text_store(Some(store)), + ) + .build(); + + // Request with matching username should succeed + let url_with_username = format!( + "{}://{}@{}", + base_url.scheme(), + username, + base_url.host_str().unwrap() + ); + let url_with_port = if let Some(port) = base_url.port() { + format!("{}:{}{}", url_with_username, port, base_url.path()) + } else { + format!("{}{}", url_with_username, base_url.path()) + }; + + assert_eq!( + client.get(&url_with_port).send().await?.status(), + 200, + "Request with matching username should succeed" + ); + + // Request with non-matching username should fail + let url_with_wrong_username = format!( + "{}://{}@{}", + base_url.scheme(), + wrong_username, + base_url.host_str().unwrap() + ); + let url_with_port = if let Some(port) = base_url.port() { + format!("{}:{}{}", url_with_wrong_username, port, base_url.path()) + } else { + format!("{}{}", url_with_wrong_username, base_url.path()) + }; + + assert_eq!( + client.get(&url_with_port).send().await?.status(), + 401, + "Request with non-matching username should fail" + ); + + // Request without username should succeed + assert_eq!( + client.get(server.uri()).send().await?.status(), + 200, + "Request with no username should succeed" + ); + + Ok(()) + } + fn create_request(url: &str) -> Request { Request::new(Method::GET, Url::parse(url).unwrap()) } diff --git a/crates/uv-auth/src/store.rs b/crates/uv-auth/src/store.rs index ef58f9418..079bbaf21 100644 --- a/crates/uv-auth/src/store.rs +++ b/crates/uv-auth/src/store.rs @@ -265,10 +265,10 @@ impl TextCredentialStore { Ok(()) } - /// Get credentials for a given URL. - /// Uses realm-based prefix matching following RFC 7235 and 7230 specifications. - /// Credentials are matched by finding the most specific prefix that matches the request URL. - pub fn get_credentials(&self, url: &Url) -> Option<&Credentials> { + /// Get credentials for a given URL and username. + /// + /// The most specific URL prefix match in the same [`Realm`] is returned, if any. + pub fn get_credentials(&self, url: &Url, username: Option<&str>) -> Option<&Credentials> { let request_realm = Realm::from(url); // Perform an exact lookup first @@ -276,7 +276,10 @@ impl TextCredentialStore { // TODO(zanieb): We could also return early here if we can't normalize to a `Service` if let Ok(url_service) = Service::try_from(DisplaySafeUrl::from(url.clone())) { if let Some(credential) = self.credentials.get(&url_service) { - return Some(credential); + // If a username is provided, it must match + if username.is_none() || username == credential.username() { + return Some(credential); + } } } @@ -296,6 +299,11 @@ impl TextCredentialStore { continue; } + // If a username is provided, it must match + if username.is_some() && username != credential.username() { + continue; + } + // Update our best matching credential based on prefix length let specificity = service.url().path().len(); if best.is_none_or(|(best_specificity, _, _)| specificity > best_specificity) { @@ -372,16 +380,16 @@ mod tests { let service = Service::from_str("https://example.com").unwrap(); store.insert(service.clone(), credentials.clone()); let url = Url::parse("https://example.com/").unwrap(); - assert!(store.get_credentials(&url).is_some()); + assert!(store.get_credentials(&url, None).is_some()); let url = Url::parse("https://example.com/path").unwrap(); - let retrieved = store.get_credentials(&url).unwrap(); + let retrieved = store.get_credentials(&url, None).unwrap(); assert_eq!(retrieved.username(), Some("user")); assert_eq!(retrieved.password(), Some("pass")); assert!(store.remove(&service).is_some()); let url = Url::parse("https://example.com/").unwrap(); - assert!(store.get_credentials(&url).is_none()); + assert!(store.get_credentials(&url, None).is_none()); } #[test] @@ -407,12 +415,12 @@ password = "pass2" let store = TextCredentialStore::from_file(temp_file.path()).unwrap(); let url = Url::parse("https://example.com/").unwrap(); - assert!(store.get_credentials(&url).is_some()); + assert!(store.get_credentials(&url, None).is_some()); let url = Url::parse("https://test.org/").unwrap(); - assert!(store.get_credentials(&url).is_some()); + assert!(store.get_credentials(&url, None).is_some()); let url = Url::parse("https://example.com").unwrap(); - let cred = store.get_credentials(&url).unwrap(); + let cred = store.get_credentials(&url, None).unwrap(); assert_eq!(cred.username(), Some("testuser")); assert_eq!(cred.password(), Some("testpass")); @@ -448,7 +456,7 @@ password = "pass2" for url_str in matching_urls { let url = Url::parse(url_str).unwrap(); - let cred = store.get_credentials(&url); + let cred = store.get_credentials(&url, None); assert!(cred.is_some(), "Failed to match URL with prefix: {url_str}"); } @@ -461,7 +469,7 @@ password = "pass2" for url_str in non_matching_urls { let url = Url::parse(url_str).unwrap(); - let cred = store.get_credentials(&url); + let cred = store.get_credentials(&url, None); assert!(cred.is_none(), "Should not match non-prefix URL: {url_str}"); } } @@ -485,7 +493,7 @@ password = "pass2" for url_str in matching_urls { let url = Url::parse(url_str).unwrap(); - let cred = store.get_credentials(&url); + let cred = store.get_credentials(&url, None); assert!( cred.is_some(), "Failed to match URL in same realm: {url_str}" @@ -501,7 +509,7 @@ password = "pass2" for url_str in non_matching_urls { let url = Url::parse(url_str).unwrap(); - let cred = store.get_credentials(&url); + let cred = store.get_credentials(&url, None); assert!( cred.is_none(), "Should not match URL in different realm: {url_str}" @@ -525,12 +533,77 @@ password = "pass2" // Should match the most specific prefix let url = Url::parse("https://example.com/api/v1/users").unwrap(); - let cred = store.get_credentials(&url).unwrap(); + let cred = store.get_credentials(&url, None).unwrap(); assert_eq!(cred.username(), Some("specific")); // Should match the general prefix for non-specific paths let url = Url::parse("https://example.com/api/v2").unwrap(); - let cred = store.get_credentials(&url).unwrap(); + let cred = store.get_credentials(&url, None).unwrap(); assert_eq!(cred.username(), Some("general")); } + + #[test] + fn test_username_exact_url_match() { + let mut store = TextCredentialStore::default(); + let url = Url::parse("https://example.com").unwrap(); + let service = Service::from_str("https://example.com").unwrap(); + let user1_creds = Credentials::basic(Some("user1".to_string()), Some("pass1".to_string())); + store.insert(service.clone(), user1_creds.clone()); + + // Should return credentials when username matches + let result = store.get_credentials(&url, Some("user1")); + assert!(result.is_some()); + assert_eq!(result.unwrap().username(), Some("user1")); + assert_eq!(result.unwrap().password(), Some("pass1")); + + // Should not return credentials when username doesn't match + let result = store.get_credentials(&url, Some("user2")); + assert!(result.is_none()); + + // Should return credentials when no username is specified + let result = store.get_credentials(&url, None); + assert!(result.is_some()); + assert_eq!(result.unwrap().username(), Some("user1")); + } + + #[test] + fn test_username_prefix_url_match() { + let mut store = TextCredentialStore::default(); + + // Add credentials with different usernames for overlapping URL prefixes + let general_service = Service::from_str("https://example.com/api").unwrap(); + let specific_service = Service::from_str("https://example.com/api/v1").unwrap(); + + let general_creds = Credentials::basic( + Some("general_user".to_string()), + Some("general_pass".to_string()), + ); + let specific_creds = Credentials::basic( + Some("specific_user".to_string()), + Some("specific_pass".to_string()), + ); + + store.insert(general_service, general_creds); + store.insert(specific_service, specific_creds); + + let url = Url::parse("https://example.com/api/v1/users").unwrap(); + + // Should match specific credentials when username matches + let result = store.get_credentials(&url, Some("specific_user")); + assert!(result.is_some()); + assert_eq!(result.unwrap().username(), Some("specific_user")); + + // Should match the general credentials when requesting general_user (falls back to less specific prefix) + let result = store.get_credentials(&url, Some("general_user")); + assert!( + result.is_some(), + "Should match general_user from less specific prefix" + ); + assert_eq!(result.unwrap().username(), Some("general_user")); + + // Should match most specific when no username specified + let result = store.get_credentials(&url, None); + assert!(result.is_some()); + assert_eq!(result.unwrap().username(), Some("specific_user")); + } } diff --git a/crates/uv/src/commands/auth/token.rs b/crates/uv/src/commands/auth/token.rs index c47a8eea8..6078de301 100644 --- a/crates/uv/src/commands/auth/token.rs +++ b/crates/uv/src/commands/auth/token.rs @@ -48,7 +48,7 @@ pub(crate) async fn token( .await .ok_or_else(|| anyhow::anyhow!("Failed to fetch credentials for {display_url}"))?, AuthBackend::TextStore(store, _lock) => store - .get_credentials(url) + .get_credentials(url, Some(&username)) .cloned() .ok_or_else(|| anyhow::anyhow!("Failed to fetch credentials for {display_url}"))?, }; diff --git a/crates/uv/tests/it/auth.rs b/crates/uv/tests/it/auth.rs index fa4a2daed..f1e9ed7d2 100644 --- a/crates/uv/tests/it/auth.rs +++ b/crates/uv/tests/it/auth.rs @@ -1266,3 +1266,103 @@ fn token_text_store_strips_simple_suffix() { " ); } + +#[test] +fn token_text_store_username() { + let context = TestContext::new_with_versions(&[]); + + // Login with specific username + context + .auth_login() + .arg("https://example.com/simple") + .arg("--username") + .arg("testuser") + .arg("--password") + .arg("testpass") + .assert() + .success(); + + // Retrieve token with matching username + uv_snapshot!(context.auth_token() + .arg("https://example.com/simple") + .arg("--username") + .arg("testuser"), @r" + success: true + exit_code: 0 + ----- stdout ----- + testpass + + ----- stderr ----- + " + ); + + // Retrieve token without username + uv_snapshot!(context.auth_token() + .arg("https://example.com/simple"), @r" + success: false + exit_code: 2 + ----- stdout ----- + + ----- stderr ----- + error: Failed to fetch credentials for https://example.com/simple + " + ); + + // Try to retrieve token with different username - should fail + uv_snapshot!(context.auth_token() + .arg("https://example.com/simple") + .arg("--username") + .arg("wronguser"), @r" + success: false + exit_code: 2 + ----- stdout ----- + + ----- stderr ----- + error: Failed to fetch credentials for wronguser@https://example.com/simple + " + ); + + // Login with token (no username) + context + .auth_login() + .arg("https://token.example.com/simple") + .arg("--token") + .arg("test-token") + .assert() + .success(); + + // Retrieve token without specifying username - should work + uv_snapshot!(context.auth_token() + .arg("https://token.example.com/simple"), @r" + success: true + exit_code: 0 + ----- stdout ----- + test-token + + ----- stderr ----- + " + ); + + // Login with username at different URL + context + .auth_login() + .arg("https://userexample.com/simple") + .arg("--username") + .arg("testuser") + .arg("--password") + .arg("testpass") + .assert() + .success(); + + // Retrieve token without username should fail + uv_snapshot!(context.auth_token() + .arg("https://userexample.com/simple"), @r" + success: false + exit_code: 2 + ----- stdout ----- + + ----- stderr ----- + error: Failed to fetch credentials for https://userexample.com/simple + " + ); +}