diff --git a/routes/mcp/mcp_routes.py b/routes/mcp/mcp_routes.py index 0e2b30276..5d14ba8c3 100644 --- a/routes/mcp/mcp_routes.py +++ b/routes/mcp/mcp_routes.py @@ -114,6 +114,18 @@ def _mcp_oauth_redirect_uri() -> str: return REDIRECT_URI +def _is_legacy_google_flow(identifier: str) -> bool: + """Determine whether an identifier is the server_id of a configured Google OAuth MCP server.""" + if not identifier: + return False + db = SessionLocal() + try: + srv = db.query(McpServer).filter(McpServer.id == identifier).first() + return bool(srv and srv.oauth_config) + finally: + db.close() + + def setup_mcp_routes(mcp_manager: McpManager): """Setup MCP routes with the provided manager.""" @@ -487,10 +499,14 @@ def setup_mcp_routes(mcp_manager: McpManager): db.close() @router.get("/oauth/callback") - async def oauth_callback(code: str, state: str, request: Request): + async def oauth_callback(code: str = "", state: str = "", request: Request = None): """Handle OAuth callback. Generic MCP OAuth flows resolve via the pending-state registry; Google flows fall through to the legacy path.""" - require_admin(request) + if not state: + return HTMLResponse(_oauth_result_page("Error", "No OAuth state parameter found in the callback URL."), status_code=400) + if not code: + return HTMLResponse(_oauth_result_page("Error", "No authorization code found in the URL. Make sure you copied the full URL from your browser."), status_code=400) + from src.mcp_oauth import resolve_pending if resolve_pending(state, code): return HTMLResponse(_oauth_result_page( @@ -498,8 +514,16 @@ def setup_mcp_routes(mcp_manager: McpManager): "The MCP server is connecting. You can close this window and return to Odysseus.", success=True, )) - # Legacy Google path: state is the server_id - return await _exchange_and_connect(state, code, request) + + # Check if this callback belongs to a legacy Google flow vs expired generic flow + if _is_legacy_google_flow(state): + require_admin(request) + return await _exchange_and_connect(state, code, request) + + return HTMLResponse(_oauth_result_page( + "Authorization Expired", + "This authorization round has expired. Press Reconnect in Odysseus and try again.", + ), status_code=400) @router.post("/oauth/exchange/{server_id}") async def oauth_exchange(server_id: str, request: Request, callback_url: str = Form(...)): @@ -514,18 +538,28 @@ def setup_mcp_routes(mcp_manager: McpManager): except Exception: return HTMLResponse(_oauth_result_page("Error", "Invalid URL format."), status_code=400) + state = params.get("state", [None])[0] + if not state: + return HTMLResponse(_oauth_result_page("Error", "No OAuth state parameter found in the callback URL."), status_code=400) + # Generic MCP OAuth: if the pasted URL carries a state we are waiting on, # resolve it directly (the background connect finishes the handshake). - state = params.get("state", [None])[0] from src.mcp_oauth import resolve_pending - if state and resolve_pending(state, code): + if resolve_pending(state, code): return HTMLResponse(_oauth_result_page( "Authorization Successful", "The MCP server is connecting. You can close this window and return to Odysseus.", success=True, )) - return await _exchange_and_connect(server_id, code, request) + # Check if this exchange belongs to a legacy Google flow vs expired generic flow + if _is_legacy_google_flow(server_id): + return await _exchange_and_connect(server_id, code, request) + + return HTMLResponse(_oauth_result_page( + "Authorization Expired", + "This authorization round has expired. Press Reconnect in Odysseus and try again.", + ), status_code=400) async def _exchange_and_connect(server_id: str, code: str, request: Request): """Exchange auth code for tokens and connect the MCP server.""" diff --git a/tests/test_mcp_oauth.py b/tests/test_mcp_oauth.py index 6a8e21876..a0ba8a6a2 100644 --- a/tests/test_mcp_oauth.py +++ b/tests/test_mcp_oauth.py @@ -264,3 +264,231 @@ def test_launchers_export_the_port_they_serve_on(): text = (_repo_root() / name).read_text(encoding="utf-8") assert uvicorn_flag in text, f"{name}: launcher no longer passes {uvicorn_flag}" assert export in text, f"{name}: serves on a port the app cannot read back" + + +# ── Callback & Exchange Expired State Handling ─────────────────── + +def test_oauth_callback_expired_state_returns_dedicated_expired_page(monkeypatch): + """An unknown or expired generic MCP OAuth state in the callback must return + the dedicated 'Authorization Expired' page instead of a bare 403 'Admin only' + or a 404 'Server not found'.""" + from unittest.mock import MagicMock + from fastapi import FastAPI, HTTPException + from fastapi.testclient import TestClient + from routes.mcp import mcp_routes + + # Simulate unauthenticated request that would fail admin check if invoked + def _deny_admin(request): + raise HTTPException(403, "Admin only") + + monkeypatch.setattr(mcp_routes, "require_admin", _deny_admin) + + # Empty DB / no matching server + class FakeQuery: + def filter(self, *a): + return self + + def first(self): + return None + + class FakeSession: + def query(self, *a): + return FakeQuery() + + def close(self): + pass + + monkeypatch.setattr(mcp_routes, "SessionLocal", lambda: FakeSession()) + + app = FastAPI() + manager = MagicMock() + router = mcp_routes.setup_mcp_routes(manager) + app.include_router(router) + client = TestClient(app) + + # Both code and state are provided, but state is not in _pending (expired/unknown) + resp = client.get("/api/mcp/oauth/callback?code=some-code&state=expired-state-123") + assert resp.status_code == 400 + assert "Authorization Expired" in resp.text + assert ( + "This authorization round has expired. Press Reconnect in Odysseus and try again." + in resp.text + ) + # Ensure it did not return the bare 403 "Admin only" + assert "Admin only" not in resp.text + + +def test_oauth_callback_active_pending_state_succeeds_without_admin(monkeypatch): + """An active pending generic MCP OAuth state resolves without requiring admin auth.""" + from unittest.mock import MagicMock + from fastapi import FastAPI, HTTPException + from fastapi.testclient import TestClient + from routes.mcp import mcp_routes + + # Admin check fails if invoked + def _deny_admin(request): + raise HTTPException(403, "Admin only") + + monkeypatch.setattr(mcp_routes, "require_admin", _deny_admin) + + app = FastAPI() + manager = MagicMock() + router = mcp_routes.setup_mcp_routes(manager) + app.include_router(router) + client = TestClient(app) + + async def go(): + fut = mcp_oauth.register_pending("pending-round-1") + resp = client.get("/api/mcp/oauth/callback?code=auth-code-xyz&state=pending-round-1") + assert resp.status_code == 200 + assert "Authorization Successful" in resp.text + code, state = await asyncio.wait_for(fut, timeout=1) + assert code == "auth-code-xyz" + assert state == "pending-round-1" + + asyncio.run(go()) + + +def test_oauth_callback_legacy_google_server_requires_admin(monkeypatch): + """Legacy Google OAuth callbacks (where state is server_id of a server with + oauth_config) still enforce require_admin.""" + import json + from unittest.mock import MagicMock + from fastapi import FastAPI, HTTPException + from fastapi.testclient import TestClient + from routes.mcp import mcp_routes + from core.database import McpServer + + # Server exists in DB with Google oauth_config + google_srv = McpServer( + id="google-srv-1", + name="Google Workspace", + oauth_config=json.dumps({"provider": "google", "keys_file": "k.json", "token_file": "t.json"}), + ) + + class FakeQuery: + def filter(self, *a): + return self + + def first(self): + return google_srv + + class FakeSession: + def query(self, *a): + return FakeQuery() + + def close(self): + pass + + monkeypatch.setattr(mcp_routes, "SessionLocal", lambda: FakeSession()) + + # When require_admin denies, 403 is raised for the legacy Google path + def _deny_admin(request): + raise HTTPException(403, "Admin only") + + monkeypatch.setattr(mcp_routes, "require_admin", _deny_admin) + + app = FastAPI() + manager = MagicMock() + router = mcp_routes.setup_mcp_routes(manager) + app.include_router(router) + client = TestClient(app) + + resp = client.get("/api/mcp/oauth/callback?code=code-123&state=google-srv-1") + assert resp.status_code == 403 + assert "Admin only" in resp.text + + +def test_oauth_exchange_expired_generic_state_returns_expired_page(monkeypatch): + """Remote paste-back flow for a generic MCP server returns the dedicated expired + page when the state is no longer pending, instead of 'No OAuth config.'.""" + from unittest.mock import MagicMock + from fastapi import FastAPI + from fastapi.testclient import TestClient + from routes.mcp import mcp_routes + from core.database import McpServer + + monkeypatch.setattr(mcp_routes, "require_admin", lambda r: None) + + # Server exists in DB without oauth_config (generic MCP server) + generic_srv = McpServer( + id="http-srv-1", + name="Remote HTTP MCP", + oauth_config=None, + ) + + class FakeQuery: + def filter(self, *a): + return self + + def first(self): + return generic_srv + + class FakeSession: + def query(self, *a): + return FakeQuery() + + def close(self): + pass + + monkeypatch.setattr(mcp_routes, "SessionLocal", lambda: FakeSession()) + + app = FastAPI() + manager = MagicMock() + router = mcp_routes.setup_mcp_routes(manager) + app.include_router(router) + client = TestClient(app) + + resp = client.post( + "/api/mcp/oauth/exchange/http-srv-1", + data={"callback_url": "http://localhost:7000/api/mcp/oauth/callback?code=code-1&state=expired-state"}, + ) + assert resp.status_code == 400 + assert "Authorization Expired" in resp.text + assert ( + "This authorization round has expired. Press Reconnect in Odysseus and try again." + in resp.text + ) + assert "No OAuth config." not in resp.text + + +def test_oauth_callback_missing_state_returns_malformed_error_not_expired(): + """Missing state parameter in callback must return a bad-request error and must + not be treated as an expired authorization round.""" + from unittest.mock import MagicMock + from fastapi import FastAPI + from fastapi.testclient import TestClient + from routes.mcp import mcp_routes + + app = FastAPI() + manager = MagicMock() + router = mcp_routes.setup_mcp_routes(manager) + app.include_router(router) + client = TestClient(app) + + resp = client.get("/api/mcp/oauth/callback?code=some-code") + assert resp.status_code == 400 + assert "No OAuth state parameter found in the callback URL." in resp.text + assert "Authorization Expired" not in resp.text + assert "This authorization round has expired" not in resp.text + + +def test_oauth_callback_missing_code_returns_malformed_error_not_expired(): + """Missing code parameter in callback must return a bad-request error and must + not be treated as an expired authorization round.""" + from unittest.mock import MagicMock + from fastapi import FastAPI + from fastapi.testclient import TestClient + from routes.mcp import mcp_routes + + app = FastAPI() + manager = MagicMock() + router = mcp_routes.setup_mcp_routes(manager) + app.include_router(router) + client = TestClient(app) + + resp = client.get("/api/mcp/oauth/callback?state=some-state") + assert resp.status_code == 400 + assert "No authorization code found in the URL." in resp.text + assert "Authorization Expired" not in resp.text + assert "This authorization round has expired" not in resp.text