fix(mcp): handle 400 auth errors, clear auth mode after OAuth, trim tokens (#1158)

* fix(mcp): handle 400 auth errors, clear auth mode after OAuth, trim tokens Three bugs prevented MCP server authentication (e.g. GitHub MCP) from working correctly: 1. **400 treated as auth-required**: GitHub's MCP endpoint returns 400 "Authorization header is badly formatted" instead of 401 when auth is missing. Broadened auth detection in activate_mcp, send_request, and discover_via_401 to also match 400+authorization errors. 2. **Auth mode not cleared after OAuth callback**: The OAuth callback handler and setup submit handler did not call clear_auth_mode(), leaving pending_auth on the thread. The next user message was intercepted as a token instead of triggering an LLM turn. 3. **Token trimming**: Tokens with leading/trailing whitespace or newlines produced malformed Authorization headers. Now trimmed before storage (configure) and before use (build_request_headers). Adds E2E tests with a mock MCP server (JSON-RPC + OAuth discovery + DCR + token exchange) covering install -> activate -> OAuth callback -> LLM turn lifecycle, plus a GitHub-style 400 error variant. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix(mcp): add TTL to PendingAuth and clear auth mode on all failure paths Auth mode (pending_auth on a Thread) had no timeout and several code paths that failed to clear it, causing user messages to be swallowed indefinitely. This adds defense-in-depth: - Add created_at + 5-minute TTL to PendingAuth; auto-clear on next message if expired (safety net for edge cases like user closing browser mid-OAuth) - Clear auth mode on OAuth callback failure paths (unknown/consumed state, expired flow) - Move clear_auth_mode before configure() match in setup_submit so it runs on failure too (addresses Copilot review feedback) Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix(ci): exclude test hunks from unwrap/assert pre-commit check The pre-commit safety script only excluded files in tests/ but not #[cfg(test)] mod tests blocks inside src/ files. Use the git diff @@ hunk header context (which includes the enclosing function name) to detect and skip test hunks. Also removes unnecessary // safety: comments from test assertions. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix: restore formatting in test assertions The replace_all edit that removed // safety: comments collapsed newlines. Restore proper line breaks. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix: address Copilot review - tighten pre-commit filter, document TTL sync - pre-commit-safety.sh: only exclude `mod tests` hunks (not `fn test_*`) to avoid hiding unwrap/assert in production functions like test_server() - session.rs: extract AUTH_MODE_TTL_SECS constant and add doc comment linking to OAUTH_FLOW_EXPIRY to prevent silent drift [skip-regression-check] Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix(mcp): return error on expired auth input, clear auth on all OAuth paths - When auth mode TTL expires and the user sends a message (possibly a pasted token), return an explicit "expired, please retry" response instead of forwarding the content to the LLM/history - Add clear_auth_mode() to all early-return paths in oauth_callback_handler (provider error, missing state/code, no extension manager) Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> --------- Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
2026-08-26 15:40:18 +00:00 · 2026-03-15 05:42:49 +00:00
parent 27e21fdabe
commit 62d16e69ac
9 changed files with 760 additions and 29 deletions
@@ -443,6 +443,11 @@ async fn fetch_resource_metadata(url: &str) -> Result<ProtectedResourceMetadata,
 }

 /// Try to discover OAuth metadata via 401 challenge response.
+///
+/// Also accepts 400 responses, since some servers return 400 for
+/// unauthenticated requests.  In practice the 400 path rarely yields a
+/// `WWW-Authenticate` header (GitHub's MCP does not), so discovery
+/// typically falls through to strategy 2 (RFC 9728) or 3 (direct).
 async fn discover_via_401(server_url: &str) -> Result<AuthorizationServerMetadata, AuthError> {
    validate_url_safe(server_url).await?;

@@ -459,9 +464,13 @@ async fn discover_via_401(server_url: &str) -> Result<AuthorizationServerMetadat

    log_redirect_if_applicable(server_url, &response);

-    if response.status().as_u16() != 401 {
+    let status = response.status().as_u16();
+
+    // Accept 401 (standard) and 400 (some servers like GitHub MCP use this).
+    // In both cases, look for WWW-Authenticate header with discovery metadata.
+    if status != 401 && status != 400 {
        return Err(AuthError::DiscoveryFailed(format!(
-            "Expected 401, got {}",
+            "Expected 401 or 400, got {}",
            response.status()
        )));
    }
@@ -471,7 +480,7 @@ async fn discover_via_401(server_url: &str) -> Result<AuthorizationServerMetadat
        .get("WWW-Authenticate")
        .and_then(|v| v.to_str().ok())
        .ok_or_else(|| {
-            AuthError::DiscoveryFailed("No WWW-Authenticate header in 401 response".to_string())
+            AuthError::DiscoveryFailed(format!("No WWW-Authenticate header in {} response", status))
        })?;

    let resource_metadata_url = parse_resource_metadata_url(www_auth).ok_or_else(|| {
@@ -275,7 +275,10 @@ impl McpClient {
            .keys()
            .any(|k| k.eq_ignore_ascii_case("authorization"));
        if !has_custom_auth && let Some(token) = self.get_access_token().await? {
-            headers.insert("Authorization".to_string(), format!("Bearer {}", token));
+            let trimmed = token.trim();
+            if !trimmed.is_empty() {
+                headers.insert("Authorization".to_string(), format!("Bearer {}", trimmed));
+            }
        }
        if let Some(ref session_manager) = self.session_manager
            && let Some(session_id) = session_manager.get_session_id(&self.server_name).await
@@ -302,7 +305,12 @@ impl McpClient {
            match result {
                Ok(response) => return Ok(response),
                Err(ToolError::ExternalService(ref msg))
-                    if msg.contains("401") || msg.contains("Unauthorized") =>
+                    if msg.contains("401")
+                        || msg.contains("Unauthorized")
+                        || (msg.contains("400") && {
+                            let lower = msg.to_ascii_lowercase();
+                            lower.contains("authorization") || lower.contains("authenticate")
+                        }) =>
                {
                    if attempt == 0
                        && let Some(ref secrets) = self.secrets
@@ -1113,4 +1121,136 @@ mod tests {
        let approval = wrapper.requires_approval(&serde_json::json!({}));
        assert_eq!(approval, ApprovalRequirement::Never);
    }
+
+    // Regression test: empty/whitespace-only tokens must not produce a
+    // malformed `Authorization: Bearer ` header (GitHub MCP returns 400
+    // "Authorization header is badly formatted" in this case).
+    #[tokio::test]
+    async fn test_build_headers_skips_empty_token() {
+        use crate::secrets::{CreateSecretParams, DecryptedSecret, Secret, SecretError, SecretRef};
+        use uuid::Uuid;
+
+        // In-memory secrets store that returns a whitespace-only string for the token.
+        struct EmptyTokenStore;
+        #[async_trait]
+        impl crate::secrets::SecretsStore for EmptyTokenStore {
+            async fn create(
+                &self,
+                _user_id: &str,
+                _params: CreateSecretParams,
+            ) -> Result<Secret, SecretError> {
+                unimplemented!()
+            }
+            async fn get(&self, _user_id: &str, _name: &str) -> Result<Secret, SecretError> {
+                unimplemented!()
+            }
+            async fn get_decrypted(
+                &self,
+                _user_id: &str,
+                _name: &str,
+            ) -> Result<DecryptedSecret, SecretError> {
+                DecryptedSecret::from_bytes(b"   ".to_vec())
+            }
+            async fn exists(&self, _user_id: &str, _name: &str) -> Result<bool, SecretError> {
+                Ok(true)
+            }
+            async fn delete(&self, _user_id: &str, _name: &str) -> Result<bool, SecretError> {
+                Ok(true)
+            }
+            async fn list(&self, _user_id: &str) -> Result<Vec<SecretRef>, SecretError> {
+                Ok(Vec::new())
+            }
+            async fn record_usage(&self, _secret_id: Uuid) -> Result<(), SecretError> {
+                Ok(())
+            }
+            async fn is_accessible(
+                &self,
+                _user_id: &str,
+                _secret_name: &str,
+                _allowed_secrets: &[String],
+            ) -> Result<bool, SecretError> {
+                Ok(true)
+            }
+        }
+
+        let config = McpServerConfig::new("github", "https://api.githubcopilot.com/mcp/");
+        let session_manager = Arc::new(McpSessionManager::new());
+        let secrets: Arc<dyn crate::secrets::SecretsStore + Send + Sync> =
+            Arc::new(EmptyTokenStore);
+
+        let client = McpClient::new_authenticated(config, session_manager, secrets, "test-user");
+
+        let headers = client.build_request_headers().await.unwrap(); // safety: test
+        assert!(
+            // safety: test
+            !headers.contains_key("Authorization"),
+            "Empty/whitespace token must not produce an Authorization header, got: {:?}",
+            headers.get("Authorization")
+        );
+    }
+
+    // Regression test: tokens with leading/trailing whitespace must be trimmed
+    // before being used in the Authorization header.
+    #[tokio::test]
+    async fn test_build_headers_trims_token() {
+        use crate::secrets::{CreateSecretParams, DecryptedSecret, Secret, SecretError, SecretRef};
+        use uuid::Uuid;
+
+        struct PaddedTokenStore;
+        #[async_trait]
+        impl crate::secrets::SecretsStore for PaddedTokenStore {
+            async fn create(
+                &self,
+                _user_id: &str,
+                _params: CreateSecretParams,
+            ) -> Result<Secret, SecretError> {
+                unimplemented!()
+            }
+            async fn get(&self, _user_id: &str, _name: &str) -> Result<Secret, SecretError> {
+                unimplemented!()
+            }
+            async fn get_decrypted(
+                &self,
+                _user_id: &str,
+                _name: &str,
+            ) -> Result<DecryptedSecret, SecretError> {
+                DecryptedSecret::from_bytes(b"  gho_abc123  \n".to_vec())
+            }
+            async fn exists(&self, _user_id: &str, _name: &str) -> Result<bool, SecretError> {
+                Ok(true)
+            }
+            async fn delete(&self, _user_id: &str, _name: &str) -> Result<bool, SecretError> {
+                Ok(true)
+            }
+            async fn list(&self, _user_id: &str) -> Result<Vec<SecretRef>, SecretError> {
+                Ok(Vec::new())
+            }
+            async fn record_usage(&self, _secret_id: Uuid) -> Result<(), SecretError> {
+                Ok(())
+            }
+            async fn is_accessible(
+                &self,
+                _user_id: &str,
+                _secret_name: &str,
+                _allowed_secrets: &[String],
+            ) -> Result<bool, SecretError> {
+                Ok(true)
+            }
+        }
+
+        let config = McpServerConfig::new("github", "https://api.githubcopilot.com/mcp/");
+        let session_manager = Arc::new(McpSessionManager::new());
+        let secrets: Arc<dyn crate::secrets::SecretsStore + Send + Sync> =
+            Arc::new(PaddedTokenStore);
+
+        let client = McpClient::new_authenticated(config, session_manager, secrets, "test-user");
+
+        let headers = client.build_request_headers().await.unwrap(); // safety: test
+        assert_eq!(
+            // safety: test
+            headers.get("Authorization").unwrap(), // safety: test
+            "Bearer gho_abc123",
+            "Token must be trimmed before use in Authorization header"
+        );
+    }
 }