From dcf70a1f29a1c01017e2a91a41881faaee26428c Mon Sep 17 00:00:00 2001
From: Zanie Blue <contact@zanie.dev>
Date: Thu, 6 Jun 2024 20:02:47 -0400
Subject: [PATCH] Include non-standard ports in keyring host queries (#4061)

Partially addresses https://github.com/astral-sh/uv/issues/4056

We were incorrectly omitting the port from requests to `keyring` when
falling back to a realm/host query, e.g. `localhost` was used instead of
`localhost:1234`. We still won't include "standard" ports like `80` for
an HTTP request.
---
 crates/uv-auth/src/keyring.rs    |  12 +++-
 crates/uv-auth/src/middleware.rs | 103 +++++++++++++++++++++++++++++--
 2 files changed, 106 insertions(+), 9 deletions(-)

diff --git a/crates/uv-auth/src/keyring.rs b/crates/uv-auth/src/keyring.rs
index b0114a70b..f9c611adc 100644
--- a/crates/uv-auth/src/keyring.rs
+++ b/crates/uv-auth/src/keyring.rs
@@ -63,12 +63,18 @@ impl KeyringProvider {
         };
         // And fallback to a check for the host
         if password.is_none() {
-            let host = url.host_str()?;
+            let host = if let Some(port) = url.port() {
+                format!("{}:{}", url.host_str()?, port)
+            } else {
+                url.host_str()?.to_string()
+            };
             trace!("Checking keyring for host {host}");
             password = match self.backend {
-                KeyringProviderBackend::Subprocess => self.fetch_subprocess(host, username).await,
+                KeyringProviderBackend::Subprocess => self.fetch_subprocess(&host, username).await,
                 #[cfg(test)]
-                KeyringProviderBackend::Dummy(ref store) => self.fetch_dummy(store, host, username),
+                KeyringProviderBackend::Dummy(ref store) => {
+                    self.fetch_dummy(store, &host, username)
+                }
             };
         }
 
diff --git a/crates/uv-auth/src/middleware.rs b/crates/uv-auth/src/middleware.rs
index 664988409..86f3566e0 100644
--- a/crates/uv-auth/src/middleware.rs
+++ b/crates/uv-auth/src/middleware.rs
@@ -784,7 +784,14 @@ mod tests {
                 AuthMiddleware::new()
                     .with_cache(CredentialsCache::new())
                     .with_keyring(Some(KeyringProvider::dummy([(
-                        (base_url.host_str().unwrap(), username),
+                        (
+                            format!(
+                                "{}:{}",
+                                base_url.host_str().unwrap(),
+                                base_url.port().unwrap()
+                            ),
+                            username,
+                        ),
                         password,
                     )]))),
             )
@@ -832,6 +839,40 @@ mod tests {
         Ok(())
     }
 
+    /// We include ports in keyring requests, e.g., `localhost:8000` should be distinct from `localhost`,
+    /// unless the server is running on a default port, e.g., `localhost:80` is equivalent to `localhost`.
+    /// We don't unit test the latter case because it's possible to collide with a server a developer is
+    /// actually running.
+    #[test(tokio::test)]
+    async fn test_keyring_includes_non_standard_port() -> Result<(), Error> {
+        let username = "user";
+        let password = "password";
+        let server = start_test_server(username, password).await;
+        let base_url = Url::parse(&server.uri())?;
+
+        let client = test_client_builder()
+            .with(
+                AuthMiddleware::new()
+                    .with_cache(CredentialsCache::new())
+                    .with_keyring(Some(KeyringProvider::dummy([(
+                        // Omit the port from the keyring entry
+                        (base_url.host_str().unwrap(), username),
+                        password,
+                    )]))),
+            )
+            .build();
+
+        let mut url = base_url.clone();
+        url.set_username(username).unwrap();
+        assert_eq!(
+            client.get(url).send().await?.status(),
+            401,
+            "We should fail because the port is not present in the keyring entry"
+        );
+
+        Ok(())
+    }
+
     #[test(tokio::test)]
     async fn test_credentials_in_keyring_seed() -> Result<(), Error> {
         let username = "user";
@@ -849,7 +890,17 @@ mod tests {
         );
         let client = test_client_builder()
             .with(AuthMiddleware::new().with_cache(cache).with_keyring(Some(
-                KeyringProvider::dummy([((base_url.host_str().unwrap(), username), password)]),
+                KeyringProvider::dummy([(
+                    (
+                        format!(
+                            "{}:{}",
+                            base_url.host_str().unwrap(),
+                            base_url.port().unwrap()
+                        ),
+                        username,
+                    ),
+                    password,
+                )]),
             )))
             .build();
 
@@ -954,8 +1005,28 @@ mod tests {
                 AuthMiddleware::new()
                     .with_cache(CredentialsCache::new())
                     .with_keyring(Some(KeyringProvider::dummy([
-                        ((base_url_1.host_str().unwrap(), username_1), password_1),
-                        ((base_url_2.host_str().unwrap(), username_2), password_2),
+                        (
+                            (
+                                format!(
+                                    "{}:{}",
+                                    base_url_1.host_str().unwrap(),
+                                    base_url_1.port().unwrap()
+                                ),
+                                username_1,
+                            ),
+                            password_1,
+                        ),
+                        (
+                            (
+                                format!(
+                                    "{}:{}",
+                                    base_url_2.host_str().unwrap(),
+                                    base_url_2.port().unwrap()
+                                ),
+                                username_2,
+                            ),
+                            password_2,
+                        ),
                     ]))),
             )
             .build();
@@ -1188,8 +1259,28 @@ mod tests {
                 AuthMiddleware::new()
                     .with_cache(CredentialsCache::new())
                     .with_keyring(Some(KeyringProvider::dummy([
-                        ((base_url_1.host_str().unwrap(), username_1), password_1),
-                        ((base_url_2.host_str().unwrap(), username_2), password_2),
+                        (
+                            (
+                                format!(
+                                    "{}:{}",
+                                    base_url_1.host_str().unwrap(),
+                                    base_url_1.port().unwrap()
+                                ),
+                                username_1,
+                            ),
+                            password_1,
+                        ),
+                        (
+                            (
+                                format!(
+                                    "{}:{}",
+                                    base_url_2.host_str().unwrap(),
+                                    base_url_2.port().unwrap()
+                                ),
+                                username_2,
+                            ),
+                            password_2,
+                        ),
                     ]))),
             )
             .build();