fix(mcp): reject invalid dashboard oauth callbacks

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("<h1>OAuth callback rejected</h1><p>The callback was already used.</p>", status_code=409)
+        reason = str(exc)
+        status_code = 409 if "already received" in reason else 400
+        return HTMLResponse(
+            "<h1>OAuth callback rejected</h1>"
+            "<p>The callback was invalid or already used.</p>",
+            status_code=status_code,
+        )
     if error:
         return HTMLResponse("<h1>Authorization failed</h1><p>Return to Hermes for details.</p>", status_code=400)
     return HTMLResponse("<h1>Authorization received</h1><p>You can close this tab and return to Hermes.</p>")

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

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):

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: