feat: reject auth servers lacking S256 PKCE support

Author: DaleSeo

crates/rmcp/src/transport/auth.rs

@@ -477,6 +477,9 @@ pub enum AuthError {
     #[error("Metadata error: {0}")]
     MetadataError(String),
 
+    #[error("Authorization server does not support the required PKCE code challenge method (S256)")]
+    PkceUnsupported,
+
     #[error("URL parse error: {0}")]
     UrlError(#[from] url::ParseError),
 
@@ -780,6 +783,10 @@ pub struct AuthorizationManager {
     resource_scopes: RwLock<Vec<String>>,
     /// OIDC Dynamic Client Registration `application_type` (SEP-837)
     application_type: Option<String>,
+    /// Refuse servers that omit `code_challenge_methods_supported` entirely.
+    /// Off by default (matching the reference SDKs); a server that advertises
+    /// methods without S256 is always refused regardless of this flag.
+    require_pkce_support: bool,
 }
 
 #[derive(Debug, Clone, Serialize, Deserialize)]
@@ -993,6 +1000,7 @@ impl AuthorizationManager {
             www_auth_scopes: RwLock::new(Vec::new()),
             resource_scopes: RwLock::new(Vec::new()),
             application_type: Some(DEFAULT_APPLICATION_TYPE.to_string()),
+            require_pkce_support: false,
         };
 
         Ok(manager)
@@ -1003,6 +1011,18 @@ impl AuthorizationManager {
         self.scope_upgrade_config = config;
     }
 
+    /// Strictly require the authorization server to advertise PKCE support.
+    ///
+    /// The client always sends an `S256` challenge. When the server's metadata
+    /// lists `code_challenge_methods_supported` without `S256`, authorization is
+    /// always refused with [`AuthError::PkceUnsupported`]. When the field is
+    /// omitted entirely, the default is to proceed anyway (matching the reference
+    /// SDKs); enabling this makes that case a hard error too, for full
+    /// OAuth 2.1 / MCP `MUST` compliance.
+    pub fn set_require_pkce_support(&mut self, require: bool) {
+        self.require_pkce_support = require;
+    }
+
     /// Set a custom credential store
     ///
     /// This allows you to provide your own implementation of credential storage,
@@ -1159,18 +1179,26 @@ impl AuthorizationManager {
             }
         }
 
-        // for PKCE, we always send s256 since oauth 2.1 requires servers to support it,
-        // but warn if the server metadata suggests otherwise
+        // The client always sends an S256 challenge, so validate the server's
+        // advertised PKCE methods. This mirrors the TypeScript reference SDK:
+        // a server that lists methods without S256 genuinely can't do the flow we
+        // require, so it is always refused; a server that omits the field entirely
+        // is tolerated by default (the spec says to refuse, but the reference SDKs
+        // proceed), and only refused when `require_pkce_support` is opted in.
         match &metadata.code_challenge_methods_supported {
             Some(methods) if !methods.iter().any(|m| m == "S256") => {
+                return Err(AuthError::PkceUnsupported);
+            }
+            None if self.require_pkce_support => {
+                return Err(AuthError::PkceUnsupported);
+            }
+            None => {
                 warn!(
-                    ?methods,
-                    "server does not advertise S256 in code_challenge_methods_supported, \
-                     proceeding with S256 anyway as oauth 2.1 requires it. \
-                     The server is not compliant with the specification!"
+                    "authorization server metadata omits code_challenge_methods_supported; \
+                     proceeding with an S256 challenge anyway"
                 );
             }
-            _ => {}
+            Some(_) => {}
         }
 
         Ok(())
@@ -2849,6 +2877,16 @@ impl OAuthState {
         Ok(OAuthState::Unauthorized(manager))
     }
 
+    /// Strictly require the authorization server to advertise PKCE support.
+    ///
+    /// Must be called before authorization begins, while the state is still
+    /// unauthorized. See [`AuthorizationManager::set_require_pkce_support`].
+    pub fn set_require_pkce_support(&mut self, require: bool) {
+        if let OAuthState::Unauthorized(manager) = self {
+            manager.set_require_pkce_support(require);
+        }
+    }
+
     /// Get client_id and OAuth credentials
     pub async fn get_credentials(&self) -> Result<Credentials, AuthError> {
         // return client_id and credentials
@@ -4307,22 +4345,59 @@ mod tests {
         assert!(manager.validate_server_metadata("code").is_err());
     }
 
-    #[tokio::test]
-    async fn test_validate_as_metadata_passes_without_pkce_s256() {
-        let mut manager = AuthorizationManager::new("https://example.com")
-            .await
-            .unwrap();
-        let metadata = AuthorizationMetadata {
+    fn as_metadata_with_pkce(methods: Option<Vec<String>>) -> AuthorizationMetadata {
+        AuthorizationMetadata {
             authorization_endpoint: "https://auth.example.com/authorize".to_string(),
             token_endpoint: "https://auth.example.com/token".to_string(),
             response_types_supported: Some(vec!["code".to_string()]),
-            code_challenge_methods_supported: Some(vec!["plain".to_string()]),
+            code_challenge_methods_supported: methods,
             ..Default::default()
-        };
-        manager.set_metadata(metadata);
+        }
+    }
+
+    #[tokio::test]
+    async fn test_validate_as_metadata_rejects_without_pkce_s256() {
+        let mut manager = AuthorizationManager::new("https://example.com")
+            .await
+            .unwrap();
+        manager.set_metadata(as_metadata_with_pkce(Some(vec!["plain".to_string()])));
+        assert!(matches!(
+            manager.validate_server_metadata("code"),
+            Err(AuthError::PkceUnsupported)
+        ));
+    }
+
+    #[tokio::test]
+    async fn test_validate_as_metadata_allows_absent_pkce_methods_by_default() {
+        let mut manager = AuthorizationManager::new("https://example.com")
+            .await
+            .unwrap();
+        manager.set_metadata(as_metadata_with_pkce(None));
+        assert!(manager.validate_server_metadata("code").is_ok());
+    }
+
+    #[tokio::test]
+    async fn test_validate_as_metadata_passes_with_pkce_s256() {
+        let mut manager = AuthorizationManager::new("https://example.com")
+            .await
+            .unwrap();
+        manager.set_metadata(as_metadata_with_pkce(Some(vec!["S256".to_string()])));
         assert!(manager.validate_server_metadata("code").is_ok());
     }
 
+    #[tokio::test]
+    async fn test_validate_as_metadata_rejects_absent_pkce_methods_when_strict() {
+        let mut manager = AuthorizationManager::new("https://example.com")
+            .await
+            .unwrap();
+        manager.set_require_pkce_support(true);
+        manager.set_metadata(as_metadata_with_pkce(None));
+        assert!(matches!(
+            manager.validate_server_metadata("code"),
+            Err(AuthError::PkceUnsupported)
+        ));
+    }
+
     #[tokio::test]
     async fn test_validate_as_metadata_passes_without_metadata() {
         let manager = AuthorizationManager::new("https://example.com")