diff --git a/scripts/pre-commit-safety.sh b/scripts/pre-commit-safety.sh index a4ec3286..7f1667dc 100755 --- a/scripts/pre-commit-safety.sh +++ b/scripts/pre-commit-safety.sh @@ -136,6 +136,14 @@ fi PROD_DIFF="$DIFF_OUTPUT" # Strip hunks from test-only files (tests/ directory, *_test.rs, test_*.rs) PROD_DIFF=$(echo "$PROD_DIFF" | grep -v '^+++ b/tests/' || true) +# Strip hunks whose @@ context line indicates a test module. +# git diff includes the enclosing function/module name after @@. +# Only match `mod tests` (the conventional #[cfg(test)] module) — do NOT +# match `fn test_*` because production code can have functions named test_*. +PROD_DIFF=$(echo "$PROD_DIFF" | awk ' + /^@@ / { in_test = ($0 ~ /mod tests/) } + !in_test { print } +' || true) if echo "$PROD_DIFF" | grep -nE '^\+' \ | grep -E '\.(unwrap|expect)\(|[^_]assert(_eq|_ne)?!' \ | grep -vE 'debug_assert|// safety:|#\[cfg\(test\)\]|#\[test\]|mod tests' \ diff --git a/src/agent/agent_loop.rs b/src/agent/agent_loop.rs index 8fda4143..5ca094e4 100644 --- a/src/agent/agent_loop.rs +++ b/src/agent/agent_loop.rs @@ -838,19 +838,42 @@ impl Agent { }; if let Some(pending) = pending_auth { - match &submission { - Submission::UserInput { content } => { - return self - .process_auth_token(message, &pending, content, session, thread_id) - .await; - } - _ => { - // Any control submission (interrupt, undo, etc.) cancels auth mode + if pending.is_expired() { + // TTL exceeded — clear stale auth mode + tracing::warn!( + extension = %pending.extension_name, + "Auth mode expired after TTL, clearing" + ); + { let mut sess = session.lock().await; if let Some(thread) = sess.threads.get_mut(&thread_id) { thread.pending_auth = None; } - // Fall through to normal handling + } + // If this was a user message (possibly a pasted token), return an + // explicit error instead of forwarding it to the LLM/history. + if matches!(submission, Submission::UserInput { .. }) { + return Ok(Some(format!( + "Authentication for **{}** expired. Please try again.", + pending.extension_name + ))); + } + // Control submissions (interrupt, undo, etc.) fall through to normal handling + } else { + match &submission { + Submission::UserInput { content } => { + return self + .process_auth_token(message, &pending, content, session, thread_id) + .await; + } + _ => { + // Any control submission (interrupt, undo, etc.) cancels auth mode + let mut sess = session.lock().await; + if let Some(thread) = sess.threads.get_mut(&thread_id) { + thread.pending_auth = None; + } + // Fall through to normal handling + } } } } diff --git a/src/agent/session.rs b/src/agent/session.rs index 0c1f1fd3..4abbea61 100644 --- a/src/agent/session.rs +++ b/src/agent/session.rs @@ -12,7 +12,7 @@ use std::collections::{HashMap, HashSet}; -use chrono::{DateTime, Utc}; +use chrono::{DateTime, TimeDelta, Utc}; use serde::{Deserialize, Serialize}; use uuid::Uuid; @@ -135,6 +135,12 @@ pub enum ThreadState { /// Pending auth token request. /// +/// Auth mode TTL — must stay in sync with +/// `crate::cli::oauth_defaults::OAUTH_FLOW_EXPIRY` (5 minutes / 300 s). +/// Defined separately to avoid a session→cli module dependency. +const AUTH_MODE_TTL_SECS: i64 = 300; +const AUTH_MODE_TTL: TimeDelta = TimeDelta::seconds(AUTH_MODE_TTL_SECS); + /// When `tool_auth` returns `awaiting_token`, the thread enters auth mode. /// The next user message is intercepted before entering the normal pipeline /// (no logging, no turn creation, no history) and routed directly to the @@ -143,6 +149,16 @@ pub enum ThreadState { pub struct PendingAuth { /// Extension name to authenticate. pub extension_name: String, + /// When this auth mode was entered. Used for TTL expiry. + #[serde(default = "Utc::now")] + pub created_at: DateTime, +} + +impl PendingAuth { + /// Returns `true` if this auth mode has exceeded the TTL. + pub fn is_expired(&self) -> bool { + Utc::now() - self.created_at > AUTH_MODE_TTL + } } /// Pending tool approval request stored on a thread. @@ -298,7 +314,10 @@ impl Thread { /// Enter auth mode: next user message will be routed directly to /// the credential store, bypassing the normal pipeline entirely. pub fn enter_auth_mode(&mut self, extension_name: String) { - self.pending_auth = Some(PendingAuth { extension_name }); + self.pending_auth = Some(PendingAuth { + extension_name, + created_at: Utc::now(), + }); self.updated_at = Utc::now(); } @@ -687,15 +706,16 @@ mod tests { #[test] fn test_enter_auth_mode() { + let before = Utc::now(); let mut thread = Thread::new(Uuid::new_v4()); assert!(thread.pending_auth.is_none()); thread.enter_auth_mode("telegram".to_string()); assert!(thread.pending_auth.is_some()); - assert_eq!( - thread.pending_auth.as_ref().unwrap().extension_name, - "telegram" - ); + let pending = thread.pending_auth.as_ref().unwrap(); + assert_eq!(pending.extension_name, "telegram"); + assert!(pending.created_at >= before); + assert!(!pending.is_expired()); } #[test] @@ -705,8 +725,9 @@ mod tests { let pending = thread.take_pending_auth(); assert!(pending.is_some()); - assert_eq!(pending.unwrap().extension_name, "notion"); - + let pending = pending.unwrap(); + assert_eq!(pending.extension_name, "notion"); + assert!(!pending.is_expired()); // Should be cleared after take assert!(thread.pending_auth.is_none()); assert!(thread.take_pending_auth().is_none()); @@ -720,10 +741,25 @@ mod tests { let json = serde_json::to_string(&thread).expect("should serialize"); assert!(json.contains("pending_auth")); assert!(json.contains("openai")); + assert!(json.contains("created_at")); let restored: Thread = serde_json::from_str(&json).expect("should deserialize"); assert!(restored.pending_auth.is_some()); - assert_eq!(restored.pending_auth.unwrap().extension_name, "openai"); + let pending = restored.pending_auth.unwrap(); + assert_eq!(pending.extension_name, "openai"); + assert!(!pending.is_expired()); + } + + #[test] + fn test_pending_auth_expiry() { + let mut pending = PendingAuth { + extension_name: "test".to_string(), + created_at: Utc::now(), + }; + assert!(!pending.is_expired()); + // Backdate beyond the TTL + pending.created_at = Utc::now() - AUTH_MODE_TTL - TimeDelta::seconds(1); + assert!(pending.is_expired()); } #[test] diff --git a/src/channels/web/server.rs b/src/channels/web/server.rs index acec3842..97d32933 100644 --- a/src/channels/web/server.rs +++ b/src/channels/web/server.rs @@ -526,23 +526,33 @@ async fn oauth_callback_handler( .get("error_description") .cloned() .unwrap_or_else(|| error.clone()); + clear_auth_mode(&state).await; return oauth_error_page(&description); } let state_param = match params.get("state") { Some(s) if !s.is_empty() => s.clone(), - _ => return oauth_error_page("IronClaw"), + _ => { + clear_auth_mode(&state).await; + return oauth_error_page("IronClaw"); + } }; let code = match params.get("code") { Some(c) if !c.is_empty() => c.clone(), - _ => return oauth_error_page("IronClaw"), + _ => { + clear_auth_mode(&state).await; + return oauth_error_page("IronClaw"); + } }; // Look up the pending flow by CSRF state (atomic remove prevents replay) let ext_mgr = match state.extension_manager.as_ref() { Some(mgr) => mgr, - None => return oauth_error_page("IronClaw"), + None => { + clear_auth_mode(&state).await; + return oauth_error_page("IronClaw"); + } }; // Strip instance prefix from state for registry lookup. @@ -563,6 +573,7 @@ async fn oauth_callback_handler( lookup_key = %lookup_key, "OAuth callback received with unknown or expired state" ); + clear_auth_mode(&state).await; return oauth_error_page("IronClaw"); } }; @@ -581,6 +592,7 @@ async fn oauth_callback_handler( message: "OAuth flow expired. Please try again.".to_string(), }); } + clear_auth_mode(&state).await; return oauth_error_page(&flow.display_name); } @@ -690,6 +702,10 @@ async fn oauth_callback_handler( } } + // Clear auth mode regardless of outcome so the next user message goes + // through to the LLM instead of being intercepted as a token. + clear_auth_mode(&state).await; + // After successful OAuth, auto-activate the extension so it moves // from "Installed (Authenticate)" → "Active" without a second click. // OAuth success is independent of activation — tokens are already stored. @@ -2182,6 +2198,10 @@ async fn extensions_setup_submit_handler( "Extension manager not available (secrets store required)".to_string(), ))?; + // Clear auth mode regardless of outcome so the next user message goes + // through to the LLM instead of being intercepted as a token. + clear_auth_mode(&state).await; + match ext_mgr.configure(&name, &req.secrets).await { Ok(result) => { // Broadcast auth_completed so the chat UI can dismiss any in-progress diff --git a/src/extensions/manager.rs b/src/extensions/manager.rs index 05b07555..f3358f34 100644 --- a/src/extensions/manager.rs +++ b/src/extensions/manager.rs @@ -2864,9 +2864,16 @@ impl ExtensionManager { // Try to list and create tools. // A 401/auth error means the server requires OAuth — surface as // AuthRequired so the activate handler triggers the OAuth flow. + // Some servers (e.g. GitHub MCP) return 400 with "Authorization header + // is badly formatted" instead of 401 when auth is missing or invalid. let mcp_tools = client.list_tools().await.map_err(|e| { let msg = e.to_string(); - if msg.contains("requires authentication") || msg.contains("401") { + let msg_lower = msg.to_ascii_lowercase(); + if msg_lower.contains("requires authentication") + || msg.contains("401") + || (msg.contains("400") + && (msg_lower.contains("authorization") || msg_lower.contains("authenticate"))) + { ExtensionError::AuthRequired } else { ExtensionError::ActivationFailed(msg) @@ -3843,11 +3850,12 @@ impl ExtensionManager { secret_name, name ))); } - if secret_value.trim().is_empty() { + let trimmed_value = secret_value.trim(); + if trimmed_value.is_empty() { continue; } let params = - CreateSecretParams::new(secret_name, secret_value).with_provider(name.to_string()); + CreateSecretParams::new(secret_name, trimmed_value).with_provider(name.to_string()); self.secrets .create(&self.user_id, params) .await diff --git a/src/tools/mcp/auth.rs b/src/tools/mcp/auth.rs index 70df42ea..7a8e384f 100644 --- a/src/tools/mcp/auth.rs +++ b/src/tools/mcp/auth.rs @@ -443,6 +443,11 @@ async fn fetch_resource_metadata(url: &str) -> Result Result { validate_url_safe(server_url).await?; @@ -459,9 +464,13 @@ async fn discover_via_401(server_url: &str) -> Result Result 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 { + unimplemented!() + } + async fn get(&self, _user_id: &str, _name: &str) -> Result { + unimplemented!() + } + async fn get_decrypted( + &self, + _user_id: &str, + _name: &str, + ) -> Result { + DecryptedSecret::from_bytes(b" ".to_vec()) + } + async fn exists(&self, _user_id: &str, _name: &str) -> Result { + Ok(true) + } + async fn delete(&self, _user_id: &str, _name: &str) -> Result { + Ok(true) + } + async fn list(&self, _user_id: &str) -> Result, 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 { + Ok(true) + } + } + + let config = McpServerConfig::new("github", "https://api.githubcopilot.com/mcp/"); + let session_manager = Arc::new(McpSessionManager::new()); + let secrets: Arc = + 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 { + unimplemented!() + } + async fn get(&self, _user_id: &str, _name: &str) -> Result { + unimplemented!() + } + async fn get_decrypted( + &self, + _user_id: &str, + _name: &str, + ) -> Result { + DecryptedSecret::from_bytes(b" gho_abc123 \n".to_vec()) + } + async fn exists(&self, _user_id: &str, _name: &str) -> Result { + Ok(true) + } + async fn delete(&self, _user_id: &str, _name: &str) -> Result { + Ok(true) + } + async fn list(&self, _user_id: &str) -> Result, 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 { + Ok(true) + } + } + + let config = McpServerConfig::new("github", "https://api.githubcopilot.com/mcp/"); + let session_manager = Arc::new(McpSessionManager::new()); + let secrets: Arc = + 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" + ); + } } diff --git a/tests/e2e/mock_llm.py b/tests/e2e/mock_llm.py index 0fa0ce9f..175accf5 100644 --- a/tests/e2e/mock_llm.py +++ b/tests/e2e/mock_llm.py @@ -225,6 +225,128 @@ async def models(_request: web.Request) -> web.Response: }) +# ── Mock MCP Server ────────────────────────────────────────────────────────── +# +# Simulates an MCP server that requires OAuth. Unauthenticated requests get +# 401 + WWW-Authenticate (standard MCP flow) or 400 "Authorization header is +# badly formatted" (GitHub-style). Authenticated requests return valid +# JSON-RPC responses for initialize and tools/list. + + +async def mcp_endpoint(request: web.Request) -> web.Response: + """Handle POST /mcp — JSON-RPC MCP endpoint requiring Bearer auth.""" + auth = request.headers.get("Authorization", "") + if not auth.startswith("Bearer ") or len(auth.split(" ", 1)[1].strip()) == 0: + # Return 401 with WWW-Authenticate header for OAuth discovery + resource_meta_url = f"http://127.0.0.1:{request.app['port']}/.well-known/oauth-protected-resource" + return web.Response( + status=401, + headers={"WWW-Authenticate": f'Bearer resource_metadata="{resource_meta_url}"'}, + text="Unauthorized", + ) + return await _mcp_handle_authed(request) + + +async def mcp_endpoint_400(request: web.Request) -> web.Response: + """Handle POST /mcp-400 — MCP endpoint that returns 400 (GitHub-style). + + Simulates GitHub's MCP server which returns 400 "Authorization header + is badly formatted" instead of 401 when auth is missing or invalid. + """ + auth = request.headers.get("Authorization", "") + if not auth.startswith("Bearer ") or len(auth.split(" ", 1)[1].strip()) == 0: + return web.Response( + status=400, + text="bad request: Authorization header is badly formatted", + ) + return await _mcp_handle_authed(request) + + +async def _mcp_handle_authed(request: web.Request) -> web.Response: + """Handle an authenticated MCP JSON-RPC request.""" + body = await request.json() + method = body.get("method", "") + req_id = body.get("id") + + if method == "initialize": + return web.json_response({ + "jsonrpc": "2.0", "id": req_id, + "result": { + "protocolVersion": "2024-11-05", + "capabilities": {"tools": {}}, + "serverInfo": {"name": "mock-mcp", "version": "1.0.0"}, + }, + }) + if method == "notifications/initialized": + return web.json_response({"jsonrpc": "2.0", "id": req_id, "result": {}}) + if method == "tools/list": + return web.json_response({ + "jsonrpc": "2.0", "id": req_id, + "result": {"tools": [{ + "name": "mock_search", + "description": "A mock search tool for testing", + "inputSchema": {"type": "object", "properties": { + "query": {"type": "string"}, + }}, + }]}, + }) + return web.json_response({"jsonrpc": "2.0", "id": req_id, "error": { + "code": -32601, "message": f"Method not found: {method}", + }}) + + +async def mcp_protected_resource(request: web.Request) -> web.Response: + """GET /.well-known/oauth-protected-resource[/{path}] — RFC 9728 discovery. + + Production code appends the MCP server path after the well-known suffix + (e.g. /.well-known/oauth-protected-resource/mcp-400), so this handler + accepts an optional tail and returns a resource matching the request. + """ + port = request.app["port"] + tail = request.match_info.get("tail", "mcp") + return web.json_response({ + "resource": f"http://127.0.0.1:{port}/{tail}", + "authorization_servers": [f"http://127.0.0.1:{port}"], + }) + + +async def mcp_auth_server_metadata(request: web.Request) -> web.Response: + """GET /.well-known/oauth-authorization-server[/{path}] — OAuth metadata.""" + port = request.app["port"] + base = f"http://127.0.0.1:{port}" + return web.json_response({ + "issuer": base, + "authorization_endpoint": f"{base}/oauth/authorize", + "token_endpoint": f"{base}/oauth/token", + "registration_endpoint": f"{base}/oauth/register", + "scopes_supported": ["read", "write"], + "response_types_supported": ["code"], + "grant_types_supported": ["authorization_code", "refresh_token"], + "code_challenge_methods_supported": ["S256"], + }) + + +async def mcp_oauth_register(request: web.Request) -> web.Response: + """POST /oauth/register — Dynamic Client Registration.""" + body = await request.json() + return web.json_response({ + "client_id": "mock-mcp-client-id", + "client_name": body.get("client_name", "IronClaw"), + "redirect_uris": body.get("redirect_uris", []), + }) + + +async def mcp_oauth_token(request: web.Request) -> web.Response: + """POST /oauth/token — Token endpoint for MCP OAuth.""" + data = await request.post() + code = data.get("code", "") + return web.json_response({ + "access_token": f"mcp-token-{code}", + "token_type": "Bearer", + "expires_in": 3600, + }) + + def main(): parser = argparse.ArgumentParser() parser.add_argument("--port", type=int, default=0) @@ -236,6 +358,15 @@ def main(): app.router.add_get("/v1/models", models) app.router.add_get("/models", models) app.router.add_post("/oauth/exchange", oauth_exchange) + # Mock MCP server endpoints + app.router.add_post("/mcp", mcp_endpoint) + app.router.add_post("/mcp-400", mcp_endpoint_400) + app.router.add_get("/.well-known/oauth-protected-resource", mcp_protected_resource) + app.router.add_get("/.well-known/oauth-protected-resource/{tail:.*}", mcp_protected_resource) + app.router.add_get("/.well-known/oauth-authorization-server", mcp_auth_server_metadata) + app.router.add_get("/.well-known/oauth-authorization-server/{tail:.*}", mcp_auth_server_metadata) + app.router.add_post("/oauth/register", mcp_oauth_register) + app.router.add_post("/oauth/token", mcp_oauth_token) async def start(): runner = web.AppRunner(app) @@ -243,6 +374,7 @@ def main(): site = web.TCPSite(runner, "127.0.0.1", args.port) await site.start() port = site._server.sockets[0].getsockname()[1] + app["port"] = port # used by MCP handlers print(f"MOCK_LLM_PORT={port}", flush=True) await asyncio.Event().wait() diff --git a/tests/e2e/scenarios/test_mcp_auth_flow.py b/tests/e2e/scenarios/test_mcp_auth_flow.py new file mode 100644 index 00000000..7de2bbe6 --- /dev/null +++ b/tests/e2e/scenarios/test_mcp_auth_flow.py @@ -0,0 +1,355 @@ +"""MCP server auth flow E2E tests. + +Tests the full MCP server lifecycle: install MCP server (pointing at mock) -> +activate triggers auth (401/400 -> AuthRequired -> OAuth URL) -> OAuth callback +completes -> auth mode cleared (next message triggers LLM turn) -> MCP tools +available. + +Regression coverage for: + - 400 "Authorization header is badly formatted" treated as auth-required + - OAuth discovery via 401 + WWW-Authenticate header + - clear_auth_mode after OAuth callback (user message not swallowed) + - Token trimming (whitespace/newline in stored tokens) + +The mock_llm.py serves a mock MCP server at /mcp with full OAuth discovery +endpoints (.well-known/oauth-protected-resource, DCR, token exchange). +""" + +from urllib.parse import parse_qs, urlparse + +import httpx +import pytest + +from helpers import SEL, api_get, api_post + + +def _extract_state(auth_url: str) -> str: + """Extract the CSRF state parameter from an OAuth authorization URL.""" + parsed = urlparse(auth_url) + qs = parse_qs(parsed.query) + assert "state" in qs, f"auth_url should contain state param: {auth_url}" + return qs["state"][0] + + +async def _get_extension(base_url, name): + """Get a specific extension from the extensions list, or None.""" + r = await api_get(base_url, "/api/extensions") + for ext in r.json().get("extensions", []): + if ext["name"] == name: + return ext + return None + + +async def _ensure_removed(base_url, name): + """Remove extension if already installed.""" + ext = await _get_extension(base_url, name) + if ext: + await api_post(base_url, f"/api/extensions/{name}/remove", timeout=30) + + +# ── Section A: Install MCP Server ──────────────────────────────────────── + + +async def test_mcp_install(ironclaw_server, mock_llm_server): + """Install a mock MCP server pointing at mock_llm.py's /mcp endpoint.""" + await _ensure_removed(ironclaw_server, "mock-mcp") + + mcp_url = f"{mock_llm_server}/mcp" + r = await api_post( + ironclaw_server, + "/api/extensions/install", + json={"name": "mock-mcp", "url": mcp_url, "kind": "mcp_server"}, + timeout=30, + ) + assert r.status_code == 200 + data = r.json() + assert data.get("success") is True, f"Install failed: {data}" + + ext = await _get_extension(ironclaw_server, "mock-mcp") + assert ext is not None, "mock-mcp should appear in extensions list" + assert ext["kind"] == "mcp_server" + + +# ── Section B: Activate Triggers Auth ──────────────────────────────────── + + +async def test_mcp_activate_triggers_auth(ironclaw_server): + """Activating an unauthenticated MCP server triggers the OAuth flow. + + The mock MCP returns 401 with WWW-Authenticate when no Bearer token + is present. The activate handler should detect this as auth-required + and return an auth_url. + """ + ext = await _get_extension(ironclaw_server, "mock-mcp") + if ext is None: + pytest.skip("mock-mcp not installed") + + r = await api_post( + ironclaw_server, + "/api/extensions/mock-mcp/activate", + timeout=30, + ) + assert r.status_code == 200 + data = r.json() + + # Activation should fail with an auth_url (OAuth needed) + # OR it should return awaiting_token (manual token prompt) + auth_url = data.get("auth_url") + awaiting_token = data.get("awaiting_token") + assert auth_url is not None or awaiting_token, ( + f"Activate should require auth, got: {data}" + ) + + +# ── Section C: OAuth Round-Trip ────────────────────────────────────────── + + +async def test_mcp_oauth_callback(ironclaw_server): + """Complete the OAuth flow via setup + callback for the MCP server.""" + ext = await _get_extension(ironclaw_server, "mock-mcp") + if ext is None: + pytest.skip("mock-mcp not installed") + + # Configure with empty secrets to trigger OAuth + r = await api_post( + ironclaw_server, + "/api/extensions/mock-mcp/setup", + json={"secrets": {}}, + timeout=30, + ) + assert r.status_code == 200 + data = r.json() + + # If no auth_url, try activate to trigger it + auth_url = data.get("auth_url") + if auth_url is None: + r = await api_post( + ironclaw_server, + "/api/extensions/mock-mcp/activate", + timeout=30, + ) + data = r.json() + auth_url = data.get("auth_url") + + if auth_url is None: + # Server might have been auto-authenticated via DCR; check if active + ext = await _get_extension(ironclaw_server, "mock-mcp") + if ext and ext.get("authenticated"): + return # Already authenticated, skip callback test + pytest.skip("Could not obtain auth_url for mock-mcp") + + csrf_state = _extract_state(auth_url) + + # Hit the OAuth callback endpoint + async with httpx.AsyncClient() as client: + r = await client.get( + f"{ironclaw_server}/oauth/callback", + params={"code": "mock_mcp_code", "state": csrf_state}, + timeout=30, + follow_redirects=True, + ) + assert r.status_code == 200, f"Callback returned {r.status_code}: {r.text[:300]}" + body = r.text.lower() + assert "connected" in body or "success" in body, ( + f"Callback should indicate success: {r.text[:500]}" + ) + + +async def test_mcp_authenticated_after_oauth(ironclaw_server): + """After OAuth callback, MCP server shows authenticated=True.""" + ext = await _get_extension(ironclaw_server, "mock-mcp") + if ext is None: + pytest.skip("mock-mcp not installed") + assert ext["authenticated"] is True, ( + f"mock-mcp should be authenticated after OAuth: {ext}" + ) + + +async def test_mcp_tools_registered(ironclaw_server): + """After authentication, MCP tools appear in the extension.""" + ext = await _get_extension(ironclaw_server, "mock-mcp") + if ext is None: + pytest.skip("mock-mcp not installed") + tools = ext.get("tools", []) + assert len(tools) > 0, f"mock-mcp should have tools after auth: {ext}" + # The mock MCP serves a tool named "mock_search", prefixed with server name + tool_names = [t for t in tools if "mock_search" in t] + assert len(tool_names) > 0, f"Expected mock_search tool, got: {tools}" + + +# ── Section D: Auth Mode Cleared — LLM Turn Fires ─────────────────────── + + +async def test_mcp_auth_mode_cleared_llm_turn_fires(ironclaw_server, page): + """After OAuth completes, the next user message triggers an LLM turn. + + Regression test: previously, pending_auth was not cleared by the OAuth + callback handler, so the next user message was consumed as a token and + the LLM turn never fired. + """ + chat_input = page.locator(SEL["chat_input"]) + await chat_input.wait_for(state="visible", timeout=5000) + + assistant_sel = SEL["message_assistant"] + before_count = await page.locator(assistant_sel).count() + + # Send a normal message — should trigger LLM, not be swallowed by auth + await chat_input.fill("hello") + await chat_input.press("Enter") + + # Wait for assistant response + expected = before_count + 1 + await page.wait_for_function( + """({ assistantSelector, expectedCount }) => { + const messages = document.querySelectorAll(assistantSelector); + return messages.length >= expectedCount; + }""", + arg={"assistantSelector": assistant_sel, "expectedCount": expected}, + timeout=15000, + ) + + text = await page.locator(assistant_sel).last.inner_text() + assert len(text.strip()) > 0, "Assistant should have responded" + + +# ── Section E: GitHub-style 400 Error ───────────────────────────────────── + + +async def test_mcp_400_activate_triggers_auth(ironclaw_server, mock_llm_server): + """MCP server returning 400 "Authorization header is badly formatted" + is treated as auth-required (regression for GitHub MCP). + + Previously, only 401 triggered the auth flow. GitHub's MCP returns 400 + with "Authorization header is badly formatted" instead. + """ + await _ensure_removed(ironclaw_server, "mock-mcp-400") + + mcp_url = f"{mock_llm_server}/mcp-400" + r = await api_post( + ironclaw_server, + "/api/extensions/install", + json={"name": "mock-mcp-400", "url": mcp_url, "kind": "mcp_server"}, + timeout=30, + ) + assert r.status_code == 200 + assert r.json().get("success") is True, f"Install failed: {r.json()}" + + # Activate should detect 400 + "authorization" as auth-required + r = await api_post( + ironclaw_server, + "/api/extensions/mock-mcp-400/activate", + timeout=30, + ) + assert r.status_code == 200, f"Activate returned {r.status_code}: {r.text[:300]}" + data = r.json() + + # 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}" + ) + + +async def test_mcp_400_oauth_discovery_returns_auth_url(ironclaw_server): + """OAuth discovery succeeds for the 400-variant via RFC 9728 (strategy 2). + + Strategy 1 (discover_via_401) fails because /mcp-400 returns 400 without + a WWW-Authenticate header. Strategy 2 queries + /.well-known/oauth-protected-resource/mcp-400 (path-suffixed) and must + find the mock's wildcard route. Without that route, discovery fails + entirely and only awaiting_token (manual) is returned — no auth_url. + + This test would have failed before the wildcard .well-known routes were + added to mock_llm.py. + """ + ext = await _get_extension(ironclaw_server, "mock-mcp-400") + if ext is None: + pytest.skip("mock-mcp-400 not installed") + + # Re-activate to get a fresh auth response + r = await api_post( + ironclaw_server, + "/api/extensions/mock-mcp-400/activate", + timeout=30, + ) + assert r.status_code == 200, f"Activate returned {r.status_code}: {r.text[:300]}" + data = r.json() + + auth_url = data.get("auth_url") + assert auth_url is not None, ( + f"OAuth discovery must produce an auth_url (not just awaiting_token). " + f"Strategy 2 (RFC 9728) likely failed — check .well-known wildcard routes. " + f"Got: {data}" + ) + + +async def test_mcp_400_full_oauth_roundtrip(ironclaw_server): + """Complete OAuth round-trip for the 400-variant MCP server. + + Exercises the full path: activate → 400 detected as auth-required → + OAuth discovery via strategy 2 (path-suffixed .well-known) → DCR → + auth_url returned → callback completes token exchange → extension + authenticated with tools. + + Without the wildcard .well-known routes, OAuth discovery fails and + no auth_url is produced, so this test would fail at the csrf_state + extraction step. + """ + ext = await _get_extension(ironclaw_server, "mock-mcp-400") + if ext is None: + pytest.skip("mock-mcp-400 not installed") + + # Get a fresh auth_url via activate + r = await api_post( + ironclaw_server, + "/api/extensions/mock-mcp-400/activate", + timeout=30, + ) + data = r.json() + auth_url = data.get("auth_url") + if auth_url is None: + pytest.skip("No auth_url from activate (discovery may not have succeeded)") + + csrf_state = _extract_state(auth_url) + + # Complete OAuth callback + async with httpx.AsyncClient() as client: + r = await client.get( + f"{ironclaw_server}/oauth/callback", + params={"code": "mock_400_code", "state": csrf_state}, + timeout=30, + follow_redirects=True, + ) + assert r.status_code == 200, f"Callback returned {r.status_code}: {r.text[:300]}" + body = r.text.lower() + assert "connected" in body or "success" in body, ( + f"400-variant OAuth callback should succeed: {r.text[:500]}" + ) + + # Verify authenticated + tools loaded + ext = await _get_extension(ironclaw_server, "mock-mcp-400") + assert ext is not None, "mock-mcp-400 should still be installed" + assert ext["authenticated"] is True, ( + f"mock-mcp-400 should be authenticated after OAuth: {ext}" + ) + tools = ext.get("tools", []) + assert len(tools) > 0, f"mock-mcp-400 should have tools after auth: {ext}" + + +async def test_mcp_400_cleanup(ironclaw_server): + """Clean up the 400-variant MCP server.""" + await _ensure_removed(ironclaw_server, "mock-mcp-400") + ext = await _get_extension(ironclaw_server, "mock-mcp-400") + assert ext is None, "mock-mcp-400 should be removed" + + +# ── Section F: Cleanup ─────────────────────────────────────────────────── + + +async def test_mcp_cleanup(ironclaw_server): + """Remove mock-mcp (cleanup for other test files).""" + await _ensure_removed(ironclaw_server, "mock-mcp") + ext = await _get_extension(ironclaw_server, "mock-mcp") + assert ext is None, "mock-mcp should be removed"