From fc39f9a498e2978b2cb10df4b7e021b4a6abd931 Mon Sep 17 00:00:00 2001 From: Ben Barclay Date: Fri, 17 Jul 2026 13:56:59 +1000 Subject: [PATCH] fix(mcp): reject invalid dashboard oauth callbacks --- hermes_cli/web_server.py | 18 +++++- tests/hermes_cli/test_mcp_dashboard_oauth.py | 62 ++++++++++++++++++++ tests/tools/test_mcp_dashboard_oauth.py | 38 +++++++++++- tools/mcp_dashboard_oauth.py | 13 ++++ 4 files changed, 128 insertions(+), 3 deletions(-) diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index 0b9d8d9f884..3cef7afa95c 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -11327,6 +11327,7 @@ async def test_mcp_server(name: str, profile: Optional[str] = None): _MCP_DASHBOARD_OAUTH_TTL = 15 * 60 +_MAX_PENDING_MCP_OAUTH_FLOWS = 8 _mcp_oauth_flows: dict[str, "DashboardOAuthFlow"] = {} @@ -11429,6 +11430,15 @@ async def auth_mcp_server(name: str, request: Request, profile: Optional[str] = _require_token(request) _gc_mcp_oauth_flows() + pending = sum( + flow.status in {"starting", "authorization_required"} + for flow in _mcp_oauth_flows.values() + ) + if pending >= _MAX_PENDING_MCP_OAUTH_FLOWS: + raise HTTPException( + status_code=429, + detail="Too many MCP OAuth flows are already in progress", + ) with _profile_scope(profile): servers = _get_mcp_servers() if name not in servers: @@ -11487,7 +11497,13 @@ async def mcp_oauth_callback( try: flow.deliver_callback(code=code, state=state, error=error) except ValueError as exc: - return HTMLResponse("

OAuth callback rejected

The callback was already used.

", status_code=409) + reason = str(exc) + status_code = 409 if "already received" in reason else 400 + return HTMLResponse( + "

OAuth callback rejected

" + "

The callback was invalid or already used.

", + status_code=status_code, + ) if error: return HTMLResponse("

Authorization failed

Return to Hermes for details.

", status_code=400) return HTMLResponse("

Authorization received

You can close this tab and return to Hermes.

") diff --git a/tests/hermes_cli/test_mcp_dashboard_oauth.py b/tests/hermes_cli/test_mcp_dashboard_oauth.py index b569fd70b88..142b4ba54ac 100644 --- a/tests/hermes_cli/test_mcp_dashboard_oauth.py +++ b/tests/hermes_cli/test_mcp_dashboard_oauth.py @@ -54,6 +54,8 @@ def test_hosted_auth_start_returns_public_authorization_url(monkeypatch): def test_hosted_callback_is_public_and_delivers_code(): + import asyncio + from hermes_cli import web_server from hermes_cli.dashboard_auth.public_paths import PUBLIC_API_PATHS from tools.mcp_dashboard_oauth import DashboardOAuthFlow @@ -64,6 +66,11 @@ def test_hosted_callback_is_public_and_delivers_code(): profile=None, redirect_uri="https://agent.example/api/mcp/oauth/callback/flow-public", ) + asyncio.run( + flow.publish_authorization_url( + "https://idp.example/authorize?state=expected" + ) + ) web_server._mcp_oauth_flows[flow.flow_id] = flow assert "/api/mcp/oauth/callback" not in PUBLIC_API_PATHS @@ -75,6 +82,8 @@ def test_hosted_callback_is_public_and_delivers_code(): def test_hosted_callback_bypasses_gated_cookie_auth(monkeypatch): + import asyncio + from starlette.testclient import TestClient from hermes_cli import web_server @@ -86,6 +95,11 @@ def test_hosted_callback_bypasses_gated_cookie_auth(monkeypatch): profile=None, redirect_uri="https://agent.example/api/mcp/oauth/callback/flow-gated", ) + asyncio.run( + flow.publish_authorization_url( + "https://idp.example/authorize?state=expected" + ) + ) web_server._mcp_oauth_flows[flow.flow_id] = flow monkeypatch.setattr(web_server.app.state, "auth_required", True, raising=False) @@ -97,6 +111,54 @@ def test_hosted_callback_bypasses_gated_cookie_auth(monkeypatch): assert flow._callback == ("abc", "expected") +def test_hosted_callback_rejects_wrong_state_before_waking_sdk(): + import asyncio + + from hermes_cli import web_server + from tools.mcp_dashboard_oauth import DashboardOAuthFlow + + flow = DashboardOAuthFlow( + flow_id="flow-state-route", + server_name="reports", + profile=None, + redirect_uri="https://agent.example/api/mcp/oauth/callback/flow-state-route", + ) + asyncio.run( + flow.publish_authorization_url( + "https://idp.example/authorize?state=expected-state" + ) + ) + web_server._mcp_oauth_flows[flow.flow_id] = flow + + response = _client().get( + "/api/mcp/oauth/callback/flow-state-route?code=attacker&state=wrong" + ) + assert response.status_code == 400 + assert flow._callback is None + + +def test_hosted_auth_start_bounds_pending_flow_registry(): + from hermes_cli import web_server + from tools.mcp_dashboard_oauth import DashboardOAuthFlow + + client = _client() + client.post( + "/api/mcp/servers", + json={"name": "reports", "url": "https://mcp.example/mcp", "auth": "oauth"}, + ) + for index in range(web_server._MAX_PENDING_MCP_OAUTH_FLOWS): + flow = DashboardOAuthFlow( + flow_id=f"existing-{index}", + server_name="reports", + profile=None, + redirect_uri=f"https://agent.example/callback/{index}", + ) + web_server._mcp_oauth_flows[flow.flow_id] = flow + + response = client.post("/api/mcp/servers/reports/auth") + assert response.status_code == 429 + + def test_flow_status_does_not_expose_authorization_code(): from hermes_cli import web_server from tools.mcp_dashboard_oauth import DashboardOAuthFlow diff --git a/tests/tools/test_mcp_dashboard_oauth.py b/tests/tools/test_mcp_dashboard_oauth.py index 35976d4e965..77e9630e340 100644 --- a/tests/tools/test_mcp_dashboard_oauth.py +++ b/tests/tools/test_mcp_dashboard_oauth.py @@ -28,6 +28,31 @@ def test_dashboard_flow_exposes_authorization_url_and_accepts_callback(): assert asyncio.run(flow.wait_for_callback()) == ("code-1", "s1") +def test_dashboard_flow_rejects_wrong_state_without_consuming_callback(): + from tools.mcp_dashboard_oauth import DashboardOAuthFlow + + flow = DashboardOAuthFlow( + flow_id="flow-state", + server_name="reports", + profile=None, + redirect_uri="https://agent.example/mcp/oauth/callback/flow-state", + ) + asyncio.run( + flow.publish_authorization_url( + "https://idp.example/authorize?state=expected-state" + ) + ) + + with pytest.raises(ValueError, match="state mismatch"): + flow.deliver_callback(code="attacker", state="wrong-state", error=None) + + flow.deliver_callback(code="legitimate", state="expected-state", error=None) + assert asyncio.run(flow.wait_for_callback()) == ( + "legitimate", + "expected-state", + ) + + def test_dashboard_flow_rejects_second_callback(): from tools.mcp_dashboard_oauth import DashboardOAuthFlow @@ -37,6 +62,11 @@ def test_dashboard_flow_rejects_second_callback(): profile=None, redirect_uri="https://agent.example/mcp/oauth/callback/flow-2", ) + asyncio.run( + flow.publish_authorization_url( + "https://idp.example/authorize?state=state" + ) + ) flow.deliver_callback(code="first", state="state", error=None) with pytest.raises(ValueError, match="already received"): flow.deliver_callback(code="second", state="state", error=None) @@ -83,11 +113,15 @@ def test_mcp_oauth_helpers_use_dashboard_flow_without_loopback_port(): metadata = _build_client_metadata(cfg) assert str(metadata.redirect_uris[0]) == flow.redirect_uri - asyncio.run(_make_redirect_handler(0)("https://idp.example/authorize")) + asyncio.run( + _make_redirect_handler(0)( + "https://idp.example/authorize?state=state-4" + ) + ) flow.deliver_callback(code="code-4", state="state-4", error=None) assert asyncio.run(_make_callback_waiter(0)()) == ("code-4", "state-4") - assert flow.authorization_url == "https://idp.example/authorize" + assert flow.authorization_url == "https://idp.example/authorize?state=state-4" def test_manager_build_allows_dashboard_flow_without_tty(tmp_path, monkeypatch): diff --git a/tools/mcp_dashboard_oauth.py b/tools/mcp_dashboard_oauth.py index d436066544c..112049e4b4c 100644 --- a/tools/mcp_dashboard_oauth.py +++ b/tools/mcp_dashboard_oauth.py @@ -9,11 +9,13 @@ from __future__ import annotations import asyncio import contextvars +import secrets import threading import time from contextlib import contextmanager from dataclasses import dataclass, field from typing import Iterator +from urllib.parse import parse_qs, urlparse @dataclass @@ -27,12 +29,17 @@ class DashboardOAuthFlow: authorization_url: str | None = None error: str | None = None tools: list[dict] = field(default_factory=list) + expected_state: str | None = field(default=None, init=False) _callback: tuple[str, str | None] | None = field(default=None, init=False, repr=False) _callback_error: str | None = field(default=None, init=False, repr=False) _authorization_ready: threading.Event = field(default_factory=threading.Event, init=False, repr=False) _callback_ready: threading.Event = field(default_factory=threading.Event, init=False, repr=False) async def publish_authorization_url(self, url: str) -> None: + state = parse_qs(urlparse(url).query).get("state", [None])[0] + if not state: + raise ValueError("OAuth authorization URL did not include state") + self.expected_state = state self.authorization_url = url self.status = "authorization_required" self._authorization_ready.set() @@ -54,6 +61,12 @@ class DashboardOAuthFlow: ) -> None: if self._callback_ready.is_set(): raise ValueError("OAuth callback already received") + if ( + self.expected_state is None + or state is None + or not secrets.compare_digest(self.expected_state, state) + ): + raise ValueError("OAuth callback state mismatch") if error: self._callback_error = error elif code: