From ac449612af9aa0aa1844caf2845fe49a0c52025a Mon Sep 17 00:00:00 2001 From: PrinceThummar011 Date: Thu, 1 Oct 2026 17:57:58 +0530 Subject: [PATCH] fix(mcp): handle expired/unknown OAuth state in callback with dedicated expired page (#5598) --- routes/mcp/mcp_routes.py | 34 ++++++- tests/test_mcp_oauth.py | 186 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 216 insertions(+), 4 deletions(-) diff --git a/routes/mcp/mcp_routes.py b/routes/mcp/mcp_routes.py index 0e2b30276..385cf09e3 100644 --- a/routes/mcp/mcp_routes.py +++ b/routes/mcp/mcp_routes.py @@ -487,19 +487,32 @@ 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) from src.mcp_oauth import resolve_pending - if resolve_pending(state, code): + if state and code and 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, )) + # Legacy Google path: state is the server_id - return await _exchange_and_connect(state, code, request) + if state: + db = SessionLocal() + try: + srv = db.query(McpServer).filter(McpServer.id == state).first() + if srv and srv.oauth_config: + require_admin(request) + return await _exchange_and_connect(state, code, request) + finally: + db.close() + + 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(...)): @@ -525,6 +538,19 @@ def setup_mcp_routes(mcp_manager: McpManager): success=True, )) + db = SessionLocal() + try: + srv = db.query(McpServer).filter(McpServer.id == server_id).first() + if not srv: + return HTMLResponse(_oauth_result_page("Error", "Server not found."), status_code=404) + if not srv.oauth_config: + return HTMLResponse(_oauth_result_page( + "Authorization Expired", + "This authorization round has expired. Press Reconnect in Odysseus and try again.", + ), status_code=400) + finally: + db.close() + return await _exchange_and_connect(server_id, code, request) async def _exchange_and_connect(server_id: str, code: str, request: Request): diff --git a/tests/test_mcp_oauth.py b/tests/test_mcp_oauth.py index 6a8e21876..24f52820e 100644 --- a/tests/test_mcp_oauth.py +++ b/tests/test_mcp_oauth.py @@ -264,3 +264,189 @@ 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