Merge pull request #6434 from PrinceThummar011/fix/5598-oauth-expired-state

fix(mcp): return dedicated expired page on unknown or expired OAuth callback state (#5598)

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."""

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