fix: address PR #1158 review feedback

- Remove 403 from auth-required detection (403 = forbidden, not
  unauthenticated; re-triggering OAuth on 403 creates a loop)
- Compute msg.to_ascii_lowercase() once in send_request retry guard
- Fix discover_via_401 doc comment: GitHub doesn't send WWW-Authenticate
  on 400; discovery falls through to RFC 9728 strategy 2/3
- Update error message from "Expected 401" to "Expected 401 or 400"
- Strengthen E2E 400 test assertion: check auth_url/awaiting_token
  fields instead of loose message string matching

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: ilblackdragon

src/extensions/manager.rs

@@ -2837,10 +2837,8 @@ impl ExtensionManager {
             let msg_lower = msg.to_ascii_lowercase();
             if msg_lower.contains("requires authentication")
                 || msg.contains("401")
-                || msg.contains("403")
                 || (msg.contains("400")
-                    && (msg_lower.contains("authorization")
-                        || msg_lower.contains("authenticate")))
+                    && (msg_lower.contains("authorization") || msg_lower.contains("authenticate")))
             {
                 ExtensionError::AuthRequired
             } else {

src/tools/mcp/auth.rs

@@ -409,9 +409,10 @@ async fn fetch_resource_metadata(url: &str) -> Result<ProtectedResourceMetadata,
 
 /// Try to discover OAuth metadata via 401 challenge response.
 ///
-/// Some servers (e.g. GitHub MCP) may return 400 instead of 401 but still
-/// include a `WWW-Authenticate` header with discovery metadata, so we also
-/// check for the header on 400 responses.
+/// 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?;
 
@@ -435,7 +436,7 @@ async fn discover_via_401(server_url: &str) -> Result<AuthorizationServerMetadat
     // 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()
         )));
     }
@@ -445,10 +446,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(format!(
-                "No WWW-Authenticate header in {} response",
-                status
-            ))
+            AuthError::DiscoveryFailed(format!("No WWW-Authenticate header in {} response", status))
         })?;
 
     let resource_metadata_url = parse_resource_metadata_url(www_auth).ok_or_else(|| {

src/tools/mcp/client.rs

@@ -302,9 +302,10 @@ impl McpClient {
                 Err(ToolError::ExternalService(ref msg))
                     if msg.contains("401")
                         || msg.contains("Unauthorized")
-                        || (msg.contains("400")
-                            && (msg.to_ascii_lowercase().contains("authorization")
-                                || msg.to_ascii_lowercase().contains("authenticate"))) =>
+                        || (msg.contains("400") && {
+                            let lower = msg.to_ascii_lowercase();
+                            lower.contains("authorization") || lower.contains("authenticate")
+                        }) =>
                 {
                     if attempt == 0
                         && let Some(ref secrets) = self.secrets
@@ -984,9 +985,7 @@ mod tests {
     // "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 crate::secrets::{CreateSecretParams, DecryptedSecret, Secret, SecretError, SecretRef};
         use uuid::Uuid;
 
         // In-memory secrets store that returns a whitespace-only string for the token.
@@ -1000,11 +999,7 @@ mod tests {
             ) -> Result<Secret, SecretError> {
                 unimplemented!()
             }
-            async fn get(
-                &self,
-                _user_id: &str,
-                _name: &str,
-            ) -> Result<Secret, SecretError> {
+            async fn get(&self, _user_id: &str, _name: &str) -> Result<Secret, SecretError> {
                 unimplemented!()
             }
             async fn get_decrypted(
@@ -1041,8 +1036,7 @@ mod tests {
         let secrets: Arc<dyn crate::secrets::SecretsStore + Send + Sync> =
             Arc::new(EmptyTokenStore);
 
-        let client =
-            McpClient::new_authenticated(config, session_manager, secrets, "test-user");
+        let client = McpClient::new_authenticated(config, session_manager, secrets, "test-user");
 
         let headers = client.build_request_headers().await.unwrap();
         assert!(
@@ -1056,9 +1050,7 @@ mod tests {
     // 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 crate::secrets::{CreateSecretParams, DecryptedSecret, Secret, SecretError, SecretRef};
         use uuid::Uuid;
 
         struct PaddedTokenStore;
@@ -1071,11 +1063,7 @@ mod tests {
             ) -> Result<Secret, SecretError> {
                 unimplemented!()
             }
-            async fn get(
-                &self,
-                _user_id: &str,
-                _name: &str,
-            ) -> Result<Secret, SecretError> {
+            async fn get(&self, _user_id: &str, _name: &str) -> Result<Secret, SecretError> {
                 unimplemented!()
             }
             async fn get_decrypted(
@@ -1112,8 +1100,7 @@ mod tests {
         let secrets: Arc<dyn crate::secrets::SecretsStore + Send + Sync> =
             Arc::new(PaddedTokenStore);
 
-        let client =
-            McpClient::new_authenticated(config, session_manager, secrets, "test-user");
+        let client = McpClient::new_authenticated(config, session_manager, secrets, "test-user");
 
         let headers = client.build_request_headers().await.unwrap();
         assert_eq!(

tests/e2e/scenarios/test_mcp_auth_flow.py

@@ -242,11 +242,12 @@ async def test_mcp_400_activate_triggers_auth(ironclaw_server, mock_llm_server):
     )
     data = r.json()
 
-    # Should NOT return a raw activation error like "400 Bad Request"
-    # Instead should return auth_url or awaiting_token
-    message = data.get("message", "")
-    assert "400" not in message or "auth" in message.lower(), (
-        f"400 error should trigger auth flow, not raw error: {data}"
+    # The 400 should be treated as auth-required, returning an auth_url
+    # or awaiting_token — not a raw "400 Bad Request" activation error.
+    auth_url = data.get("auth_url")
+    awaiting_token = data.get("awaiting_token")
+    assert auth_url is not None or awaiting_token, (
+        f"400 auth error should trigger auth flow (auth_url or awaiting_token), got: {data}"
     )