fix(oauth): Three oauth discovery and registration issues

Author: gpeal

crates/rmcp/src/transport/auth.rs

@@ -243,11 +243,11 @@ impl AuthorizationManager {
 
     /// discover oauth2 metadata
     pub async fn discover_metadata(&self) -> Result<AuthorizationMetadata, AuthError> {
-        if let Some(metadata) = self.try_discover_from_base(&self.base_url).await? {
+        if let Some(metadata) = self.try_discover_oauth_server(&self.base_url).await? {
             return Ok(metadata);
         }
 
-        if let Some(metadata) = self.discover_via_resource_metadata().await? {
+        if let Some(metadata) = self.discover_oauth_server_via_resource_metadata().await? {
             return Ok(metadata);
         }
 
@@ -499,7 +499,9 @@ impl AuthorizationManager {
                     }
                 }
             }
-            Err(e) => Err(AuthError::TokenExchangeFailed(e.to_string())),
+            Err(e) => {
+                return Err(AuthError::TokenExchangeFailed(e.to_string()));
+            }
         };
 
         // get expires_in from token response
@@ -596,10 +598,8 @@ impl AuthorizationManager {
             Ok(response)
         }
     }
-}
 
-impl AuthorizationManager {
-    async fn try_discover_from_base(
+    async fn try_discover_oauth_server(
         &self,
         base_url: &Url,
     ) -> Result<Option<AuthorizationMetadata>, AuthError> {
@@ -648,15 +648,15 @@ impl AuthorizationManager {
         Ok(Some(metadata))
     }
 
-    async fn discover_via_resource_metadata(
+    async fn discover_oauth_server_via_resource_metadata(
         &self,
     ) -> Result<Option<AuthorizationMetadata>, AuthError> {
         let Some(resource_metadata_url) = self.fetch_resource_metadata_url().await? else {
             return Ok(None);
         };
 
         let Some(resource_metadata) = self
-            .fetch_resource_metadata_document(&resource_metadata_url)
+            .fetch_resource_metadata_from_url(&resource_metadata_url)
             .await?
         else {
             return Ok(None);
@@ -695,14 +695,16 @@ impl AuthorizationManager {
                 continue;
             }
 
-            if let Some(metadata) = self.try_discover_from_base(&candidate_url).await? {
+            if let Some(metadata) = self.try_discover_oauth_server(&candidate_url).await? {
                 return Ok(Some(metadata));
             }
         }
 
         Ok(None)
     }
 
+    /// Extract the resource metadata url from the WWW-Authenticate header value.
+    /// https://www.rfc-editor.org/rfc/rfc9728.html#name-use-of-www-authenticate-for
     async fn fetch_resource_metadata_url(&self) -> Result<Option<Url>, AuthError> {
         let response = match self
             .http_client
@@ -731,7 +733,9 @@ impl AuthorizationManager {
             let Ok(value_str) = value.to_str() else {
                 continue;
             };
-            if let Some(url) = Self::extract_resource_metadata_url(value_str, &self.base_url) {
+            if let Some(url) =
+                Self::extract_resource_metadata_url_from_header(value_str, &self.base_url)
+            {
                 parsed_url = Some(url);
                 break;
             }
@@ -740,7 +744,7 @@ impl AuthorizationManager {
         Ok(parsed_url)
     }
 
-    async fn fetch_resource_metadata_document(
+    async fn fetch_resource_metadata_from_url(
         &self,
         resource_metadata_url: &Url,
     ) -> Result<Option<ResourceServerMetadata>, AuthError> {
@@ -779,15 +783,16 @@ impl AuthorizationManager {
         Ok(Some(metadata))
     }
 
-    fn extract_resource_metadata_url(header: &str, base_url: &Url) -> Option<Url> {
-        let lower_header = header.to_ascii_lowercase();
-        let needle = "resource_metadata=";
+    /// Extracts a url following `resource_metadata=` in a header value
+    fn extract_resource_metadata_url_from_header(header: &str, base_url: &Url) -> Option<Url> {
+        let header_lowercase = header.to_ascii_lowercase();
+        let fragment_key = "resource_metadata=";
         let mut search_offset = 0;
 
-        while let Some(pos) = lower_header[search_offset..].find(needle) {
-            let global_pos = search_offset + pos + needle.len();
+        while let Some(pos) = header_lowercase[search_offset..].find(fragment_key) {
+            let global_pos = search_offset + pos + fragment_key.len();
             let value_slice = &header[global_pos..];
-            if let Some((value, consumed)) = Self::parse_auth_param_value(value_slice) {
+            if let Some((value, consumed)) = Self::parse_next_header_value(value_slice) {
                 if let Ok(url) = Url::parse(&value) {
                     return Some(url);
                 }
@@ -805,14 +810,23 @@ impl AuthorizationManager {
         None
     }
 
-    fn parse_auth_param_value(value: &str) -> Option<(String, usize)> {
-        let trimmed = value.trim_start();
-        let leading_ws = value.len() - trimmed.len();
-
-        if trimmed.starts_with('"') {
+    /// Parses an authentication parameter value from a `WWW-Authenticate` header fragment.
+    /// The header fragment should start with the header value after the `=` character and then
+    /// reads until the value ends.
+    ///
+    /// Returns the extracted value together with the number of bytes consumed from the provided
+    /// fragment. Quoted values support escaped characters (e.g. `\"`). The parser skips leading
+    /// whitespace before reading either a quoted or token value. If no well-formed value is found,
+    /// `None` is returned.
+    fn parse_next_header_value(header_fragment: &str) -> Option<(String, usize)> {
+        let trimmed = header_fragment.trim_start();
+        let leading_ws = header_fragment.len() - trimmed.len();
+
+        if let Some(stripped) = trimmed.strip_prefix('"') {
             let mut escaped = false;
             let mut result = String::new();
-            for (idx, ch) in trimmed[1..].char_indices() {
+            #[allow(clippy::manual_strip)]
+            for (idx, ch) in stripped.char_indices() {
                 if escaped {
                     result.push(ch);
                     escaped = false;
@@ -834,34 +848,6 @@ impl AuthorizationManager {
     }
 }
 
-#[cfg(test)]
-mod tests {
-    use super::AuthorizationManager;
-    use url::Url;
-
-    #[test]
-    fn parses_resource_metadata_parameter() {
-        let header = r#"Bearer error="invalid_request", error_description="missing token", resource_metadata="https://example.com/.well-known/oauth-protected-resource/api""#;
-        let base = Url::parse("https://example.com/api").unwrap();
-        let parsed = AuthorizationManager::extract_resource_metadata_url(header, &base);
-        assert_eq!(
-            parsed.unwrap().as_str(),
-            "https://example.com/.well-known/oauth-protected-resource/api"
-        );
-    }
-
-    #[test]
-    fn parses_relative_resource_metadata_parameter() {
-        let header = r#"Bearer error="invalid_request", resource_metadata="/.well-known/oauth-protected-resource/api""#;
-        let base = Url::parse("https://example.com/api").unwrap();
-        let parsed = AuthorizationManager::extract_resource_metadata_url(header, &base);
-        assert_eq!(
-            parsed.unwrap().as_str(),
-            "https://example.com/.well-known/oauth-protected-resource/api"
-        );
-    }
-}
-
 /// oauth2 authorization session, for guiding user to complete the authorization process
 pub struct AuthorizationSession {
     pub auth_manager: AuthorizationManager,
@@ -1180,6 +1166,59 @@ impl OAuthState {
 #[cfg(test)]
 mod tests {
     use super::AuthorizationManager;
+    use url::Url;
+
+    #[test]
+    fn parses_resource_metadata_parameter() {
+        let header = r#"Bearer error="invalid_request", error_description="missing token", resource_metadata="https://example.com/.well-known/oauth-protected-resource/api""#;
+        let base = Url::parse("https://example.com/api").unwrap();
+        let parsed = AuthorizationManager::extract_resource_metadata_url_from_header(header, &base);
+        assert_eq!(
+            parsed.unwrap().as_str(),
+            "https://example.com/.well-known/oauth-protected-resource/api"
+        );
+    }
+
+    #[test]
+    fn parses_relative_resource_metadata_parameter() {
+        let header = r#"Bearer error="invalid_request", resource_metadata="/.well-known/oauth-protected-resource/api""#;
+        let base = Url::parse("https://example.com/api").unwrap();
+        let parsed = AuthorizationManager::extract_resource_metadata_url_from_header(header, &base);
+        assert_eq!(
+            parsed.unwrap().as_str(),
+            "https://example.com/.well-known/oauth-protected-resource/api"
+        );
+    }
+
+    #[test]
+    fn parse_auth_param_value_handles_quoted_string() {
+        let fragment = r#""example", realm="foo""#;
+        let parsed = AuthorizationManager::parse_next_header_value(fragment).unwrap();
+        assert_eq!(parsed.0, "example");
+        assert_eq!(parsed.1, 9);
+    }
+
+    #[test]
+    fn parse_auth_param_value_handles_escaped_quotes_and_whitespace() {
+        let fragment = r#"   "a\"b\\c" ,next=value"#;
+        let parsed = AuthorizationManager::parse_next_header_value(fragment).unwrap();
+        assert_eq!(parsed.0, r#"a"b\c"#);
+        assert_eq!(parsed.1, 12);
+    }
+
+    #[test]
+    fn parse_auth_param_value_handles_token_values() {
+        let fragment = "  token,next";
+        let parsed = AuthorizationManager::parse_next_header_value(fragment).unwrap();
+        assert_eq!(parsed.0, "token");
+        assert_eq!(parsed.1, 7);
+    }
+
+    #[test]
+    fn parse_auth_param_value_returns_none_for_unterminated_quotes() {
+        let fragment = r#""unterminated,value"#;
+        assert!(AuthorizationManager::parse_next_header_value(fragment).is_none());
+    }
 
     #[test]
     fn well_known_paths_root() {