From 229162e895ebdfe37893198851c0d01010015592 Mon Sep 17 00:00:00 2001 From: Ben Barclay Date: Fri, 17 Jul 2026 13:42:43 +1000 Subject: [PATCH 1/7] fix(mcp): complete OAuth through hosted dashboards --- hermes_cli/dashboard_auth/middleware.py | 1 + hermes_cli/web_server.py | 220 ++++++++++++------- tests/hermes_cli/test_mcp_dashboard_oauth.py | 120 ++++++++++ tests/tools/test_mcp_dashboard_oauth.py | 131 +++++++++++ tools/mcp_dashboard_oauth.py | 110 ++++++++++ tools/mcp_oauth.py | 20 ++ tools/mcp_oauth_manager.py | 13 +- tools/mcp_tool.py | 22 ++ web/src/lib/api.ts | 15 +- web/src/lib/mcp-dashboard-oauth.test.ts | 84 +++++++ web/src/lib/mcp-dashboard-oauth.ts | 51 +++++ web/src/pages/McpPage.tsx | 19 +- 12 files changed, 716 insertions(+), 90 deletions(-) create mode 100644 tests/hermes_cli/test_mcp_dashboard_oauth.py create mode 100644 tests/tools/test_mcp_dashboard_oauth.py create mode 100644 tools/mcp_dashboard_oauth.py create mode 100644 web/src/lib/mcp-dashboard-oauth.test.ts create mode 100644 web/src/lib/mcp-dashboard-oauth.ts diff --git a/hermes_cli/dashboard_auth/middleware.py b/hermes_cli/dashboard_auth/middleware.py index caa1b3a6e5dd0..5c029cac62d1c 100644 --- a/hermes_cli/dashboard_auth/middleware.py +++ b/hermes_cli/dashboard_auth/middleware.py @@ -53,6 +53,7 @@ "/auth/logout", "/login", "/api/auth/providers", + "/api/mcp/oauth/callback/", "/assets/", "/favicon.ico", "/ds-assets/", diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index 2a5ef1ea93763..97e3c828d23a6 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -587,7 +587,8 @@ async def auth_middleware(request: Request, call_next): if getattr(request.app.state, "auth_required", False): return await call_next(request) path = request.url.path - if path.startswith("/api/") and path not in _PUBLIC_API_PATHS: + is_mcp_oauth_callback = path.startswith("/api/mcp/oauth/callback/") + if path.startswith("/api/") and path not in _PUBLIC_API_PATHS and not is_mcp_oauth_callback: if not _has_valid_session_token(request) and not _has_valid_query_token(request, path): return JSONResponse( status_code=401, @@ -11325,98 +11326,73 @@ def _probe_scoped(): } -@app.post("/api/mcp/servers/{name}/auth") -async def auth_mcp_server(name: str, profile: Optional[str] = None): - """Run the OAuth flow for an HTTP MCP server (opens the system browser). - - Mirrors ``hermes mcp login``: wipe cached OAuth state so the probe forces - a fresh browser flow, connect, then verify a token actually landed on disk - (some providers serve tools/list unauthenticated — see - ``_reauth_oauth_server``). Blocks until the browser flow completes, so it - runs in a worker thread. ``auth: oauth`` is persisted only on success. - """ +_MCP_DASHBOARD_OAUTH_TTL = 15 * 60 +_mcp_oauth_flows: dict[str, "DashboardOAuthFlow"] = {} + + +def _gc_mcp_oauth_flows() -> None: + cutoff = time.time() - _MCP_DASHBOARD_OAUTH_TTL + stale = [ + flow_id + for flow_id, flow in _mcp_oauth_flows.items() + if getattr(flow, "created_at", 0) < cutoff + ] + for flow_id in stale: + _mcp_oauth_flows.pop(flow_id, None) + + +def _mcp_oauth_callback_url(request: Request, flow_id: str) -> str: + """Build the externally reachable callback URL for a dashboard flow.""" + from urllib.parse import urlparse, urlunparse + + from hermes_cli.dashboard_auth.prefix import prefix_from_request, resolve_public_url + + suffix = f"/api/mcp/oauth/callback/{flow_id}" + public_url = resolve_public_url() + if public_url: + return f"{public_url}{suffix}" + base = urlparse(str(request.base_url)) + prefix = prefix_from_request(request) + return urlunparse(base._replace(path=f"{prefix}{suffix}", params="", query="", fragment="")) + + +def _run_dashboard_mcp_oauth(flow, cfg: dict) -> None: + """Run the normal MCP probe with dashboard redirect/callback handlers.""" from hermes_cli.mcp_config import ( - _get_mcp_servers, _oauth_tokens_present, _probe_single_server, _save_mcp_server, ) - - with _profile_scope(profile): - servers = _get_mcp_servers() - if name not in servers: - raise HTTPException(status_code=404, detail=f"Server '{name}' not found") - - cfg = dict(servers[name]) - if not cfg.get("url"): - raise HTTPException( - status_code=400, - detail="stdio servers authenticate via env keys, not OAuth", - ) - # A server carrying `headers` uses API-key/bearer auth; a 401 there is a bad - # key, not an OAuth prompt. Refuse rather than rewrite it to `auth: oauth` - # and corrupt a working header-auth config. (Explicit `auth: oauth` is fine.) - if cfg.get("headers") and cfg.get("auth") != "oauth": - raise HTTPException( - status_code=400, - detail="This server uses header/API-key auth, not OAuth — check its key.", - ) - cfg["auth"] = "oauth" - - def _run(): + try: + from tools.mcp_dashboard_oauth import dashboard_oauth_flow from tools.mcp_oauth import HermesTokenStorage, force_interactive_oauth + from tools.mcp_oauth_manager import get_manager - # Home-only scope, not _profile_scope: this blocks on the browser flow - # for up to minutes; holding the shared skills lock that whole time - # would freeze every other endpoint. Config writes here (_save_mcp_server) - # resolve HERMES_HOME via the contextvar override, which is all they need. - with _config_profile_scope(profile), force_interactive_oauth(): - storage = HermesTokenStorage(name) - # Snapshot before clearing: a re-auth wipes cached state to force a - # fresh consent, but if the flow fails we must NOT leave the user - # worse off than before — restore the working token on any failure. + with ( + _config_profile_scope(flow.profile), + force_interactive_oauth(), + dashboard_oauth_flow(flow), + ): + storage = HermesTokenStorage(flow.server_name) backup = storage.snapshot() try: - from tools.mcp_oauth_manager import get_manager - - get_manager().remove(name) - except Exception: - pass # No cached state to clear — fine. - try: - # The default 30s connect timeout would kill the flow while the - # user is still on the consent screen — give the browser - # round-trip the full callback window (300s in mcp_oauth) plus - # headroom so the connect wrapper can't pre-empt it. Honor a - # larger configured connect_timeout when the user set one. - try: - _cfg_timeout = float(cfg.get("connect_timeout", 0)) - except (TypeError, ValueError): - _cfg_timeout = 0.0 + get_manager().remove(flow.server_name) tools = _probe_single_server( - name, cfg, connect_timeout=max(_cfg_timeout, 315) + flow.server_name, + cfg, + connect_timeout=max(float(cfg.get("connect_timeout", 0) or 0), 315), ) + if not _oauth_tokens_present(flow.server_name): + raise RuntimeError( + "The server responded, but no OAuth token was obtained — " + "this provider may require a manually-registered OAuth client." + ) + _save_mcp_server(flow.server_name, cfg) + flow.tools = [{"name": t, "description": d} for t, d in tools] + flow.mark_approved() except Exception: storage.restore(backup) raise - if not _oauth_tokens_present(name): - storage.restore(backup) - return { - "ok": False, - "error": ( - "The server responded, but no OAuth token was obtained — " - "this provider may require a manually-registered OAuth " - "client (see `hermes mcp login`)." - ), - "tools": [], - } - _save_mcp_server(name, cfg) - return { - "ok": True, - "tools": [{"name": t, "description": d} for t, d in tools], - } - - try: - return await asyncio.to_thread(_run) except Exception as exc: msg = str(exc) # Providers that gate RFC 7591 registration to pre-approved clients @@ -11426,13 +11402,95 @@ def _run(): lowered = msg.lower() if "403" in msg and ("regist" in lowered or "forbidden" in lowered): msg = ( - f"'{name}' only allows pre-approved OAuth clients — it rejected " + f"'{flow.server_name}' only allows pre-approved OAuth clients — it rejected " "client registration (403), so no browser flow can start. " "Options: add a pre-registered client to this server's entry " "(oauth: {client_id: ..., client_secret: ...}), or use the " "provider's stdio / API-key server instead." ) - return {"ok": False, "error": msg, "tools": []} + flow.mark_error(msg) + finally: + # Dashboard auth builds a provider with a public callback URI and bridge + # handlers. Evict that one-shot provider after completion; persisted + # tokens/client registration remain for the normal runtime rebuild. + try: + from tools.mcp_oauth_manager import get_manager + + get_manager().evict(flow.server_name) + except Exception: + pass + + +@app.post("/api/mcp/servers/{name}/auth") +async def auth_mcp_server(name: str, request: Request, profile: Optional[str] = None): + """Start MCP OAuth and hand the authorization URL to the dashboard browser.""" + from hermes_cli.mcp_config import _get_mcp_servers + from tools.mcp_dashboard_oauth import DashboardOAuthFlow + + _require_token(request) + _gc_mcp_oauth_flows() + with _profile_scope(profile): + servers = _get_mcp_servers() + if name not in servers: + raise HTTPException(status_code=404, detail=f"Server '{name}' not found") + cfg = dict(servers[name]) + if not cfg.get("url"): + raise HTTPException(status_code=400, detail="stdio servers authenticate via env keys, not OAuth") + if cfg.get("headers") and cfg.get("auth") != "oauth": + raise HTTPException(status_code=400, detail="This server uses header/API-key auth, not OAuth") + cfg["auth"] = "oauth" + + flow_id = secrets.token_urlsafe(24) + flow = DashboardOAuthFlow( + flow_id=flow_id, + server_name=name, + profile=profile, + redirect_uri=_mcp_oauth_callback_url(request, flow_id), + ) + _mcp_oauth_flows[flow_id] = flow + threading.Thread( + target=_run_dashboard_mcp_oauth, + args=(flow, cfg), + daemon=True, + name=f"mcp-oauth-{name}", + ).start() + try: + await flow.wait_for_authorization_url(timeout=30) + except Exception as exc: + flow.mark_error(str(exc)) + return flow.snapshot() + + +@app.get("/api/mcp/oauth/flows/{flow_id}") +async def mcp_oauth_flow_status(flow_id: str, request: Request): + _require_token(request) + _gc_mcp_oauth_flows() + flow = _mcp_oauth_flows.get(flow_id) + if flow is None: + raise HTTPException(status_code=404, detail="OAuth flow not found or expired") + snapshot = flow.snapshot() + snapshot["tools"] = flow.tools + return snapshot + + +@app.get("/api/mcp/oauth/callback/{flow_id}") +async def mcp_oauth_callback( + flow_id: str, + code: Optional[str] = None, + state: Optional[str] = None, + error: Optional[str] = None, +): + _gc_mcp_oauth_flows() + flow = _mcp_oauth_flows.get(flow_id) + if flow is None: + return HTMLResponse("

OAuth flow expired

Return to Hermes and try again.

", status_code=404) + 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) + 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.

") class MCPEnabledToggle(BaseModel): diff --git a/tests/hermes_cli/test_mcp_dashboard_oauth.py b/tests/hermes_cli/test_mcp_dashboard_oauth.py new file mode 100644 index 0000000000000..b569fd70b88e8 --- /dev/null +++ b/tests/hermes_cli/test_mcp_dashboard_oauth.py @@ -0,0 +1,120 @@ +"""Dashboard HTTP contract for hosted MCP OAuth.""" + +from unittest.mock import patch + +import pytest + + +def _client(): + from starlette.testclient import TestClient + + from hermes_cli.web_server import app, _SESSION_HEADER_NAME, _SESSION_TOKEN + + client = TestClient(app) + client.headers[_SESSION_HEADER_NAME] = _SESSION_TOKEN + return client + + +@pytest.fixture(autouse=True) +def _clear_flows(): + from hermes_cli import web_server + + web_server._mcp_oauth_flows.clear() + yield + web_server._mcp_oauth_flows.clear() + + +def test_hosted_auth_start_returns_public_authorization_url(monkeypatch): + from hermes_cli import web_server + + client = _client() + client.post( + "/api/mcp/servers", + json={"name": "reports", "url": "https://mcp.example/mcp", "auth": "oauth"}, + ) + + def fake_worker(flow, cfg): + import asyncio + + asyncio.run(flow.publish_authorization_url("https://idp.example/authorize?state=s1")) + + monkeypatch.setattr(web_server, "_run_dashboard_mcp_oauth", fake_worker) + with patch( + "hermes_cli.dashboard_auth.prefix.resolve_public_url", + return_value="https://agent.example", + ): + response = client.post("/api/mcp/servers/reports/auth") + + assert response.status_code == 200 + body = response.json() + assert body["status"] == "authorization_required" + assert body["authorization_url"] == "https://idp.example/authorize?state=s1" + flow = web_server._mcp_oauth_flows[body["flow_id"]] + assert flow.redirect_uri == f"https://agent.example/api/mcp/oauth/callback/{body['flow_id']}" + + +def test_hosted_callback_is_public_and_delivers_code(): + from hermes_cli import web_server + from hermes_cli.dashboard_auth.public_paths import PUBLIC_API_PATHS + from tools.mcp_dashboard_oauth import DashboardOAuthFlow + + flow = DashboardOAuthFlow( + flow_id="flow-public", + server_name="reports", + profile=None, + redirect_uri="https://agent.example/api/mcp/oauth/callback/flow-public", + ) + web_server._mcp_oauth_flows[flow.flow_id] = flow + + assert "/api/mcp/oauth/callback" not in PUBLIC_API_PATHS + response = _client().get( + "/api/mcp/oauth/callback/flow-public?code=abc&state=expected" + ) + assert response.status_code == 200 + assert flow._callback == ("abc", "expected") + + +def test_hosted_callback_bypasses_gated_cookie_auth(monkeypatch): + from starlette.testclient import TestClient + + from hermes_cli import web_server + from tools.mcp_dashboard_oauth import DashboardOAuthFlow + + flow = DashboardOAuthFlow( + flow_id="flow-gated", + server_name="reports", + profile=None, + redirect_uri="https://agent.example/api/mcp/oauth/callback/flow-gated", + ) + web_server._mcp_oauth_flows[flow.flow_id] = flow + monkeypatch.setattr(web_server.app.state, "auth_required", True, raising=False) + + response = TestClient(web_server.app).get( + "/api/mcp/oauth/callback/flow-gated?code=abc&state=expected" + ) + + assert response.status_code == 200 + assert flow._callback == ("abc", "expected") + + +def test_flow_status_does_not_expose_authorization_code(): + from hermes_cli import web_server + from tools.mcp_dashboard_oauth import DashboardOAuthFlow + + flow = DashboardOAuthFlow( + flow_id="flow-status", + server_name="reports", + profile=None, + redirect_uri="https://agent.example/api/mcp/oauth/callback/flow-status", + ) + flow.authorization_url = "https://idp.example/authorize" + flow.status = "approved" + flow._callback = ("secret-code", "secret-state") + web_server._mcp_oauth_flows[flow.flow_id] = flow + + response = _client().get("/api/mcp/oauth/flows/flow-status") + assert response.status_code == 200 + body = response.json() + assert body["status"] == "approved" + assert "secret-code" not in response.text + assert "secret-state" not in response.text diff --git a/tests/tools/test_mcp_dashboard_oauth.py b/tests/tools/test_mcp_dashboard_oauth.py new file mode 100644 index 0000000000000..35976d4e965ff --- /dev/null +++ b/tests/tools/test_mcp_dashboard_oauth.py @@ -0,0 +1,131 @@ +"""Hosted-dashboard bridge for MCP OAuth browser callbacks.""" + +import asyncio + +import pytest + + +def test_dashboard_flow_exposes_authorization_url_and_accepts_callback(): + from tools.mcp_dashboard_oauth import DashboardOAuthFlow + + flow = DashboardOAuthFlow( + flow_id="flow-1", + server_name="reports", + profile=None, + redirect_uri="https://agent.example/mcp/oauth/callback/flow-1", + ) + + asyncio.run(flow.publish_authorization_url("https://idp.example/authorize?state=s1")) + assert flow.snapshot() == { + "flow_id": "flow-1", + "server_name": "reports", + "status": "authorization_required", + "authorization_url": "https://idp.example/authorize?state=s1", + "error": None, + } + + flow.deliver_callback(code="code-1", state="s1", error=None) + assert asyncio.run(flow.wait_for_callback()) == ("code-1", "s1") + + +def test_dashboard_flow_rejects_second_callback(): + from tools.mcp_dashboard_oauth import DashboardOAuthFlow + + flow = DashboardOAuthFlow( + flow_id="flow-2", + server_name="reports", + profile=None, + redirect_uri="https://agent.example/mcp/oauth/callback/flow-2", + ) + 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) + + +def test_dashboard_context_overrides_redirect_and_handlers(): + from tools.mcp_dashboard_oauth import ( + DashboardOAuthFlow, + dashboard_oauth_flow, + get_dashboard_oauth_flow, + ) + + flow = DashboardOAuthFlow( + flow_id="flow-3", + server_name="reports", + profile=None, + redirect_uri="https://agent.example/mcp/oauth/callback/flow-3", + ) + assert get_dashboard_oauth_flow() is None + with dashboard_oauth_flow(flow): + assert get_dashboard_oauth_flow() is flow + assert get_dashboard_oauth_flow() is None + + +def test_mcp_oauth_helpers_use_dashboard_flow_without_loopback_port(): + from tools.mcp_dashboard_oauth import DashboardOAuthFlow, dashboard_oauth_flow + from tools.mcp_oauth import ( + HermesTokenStorage, + _build_client_metadata, + _configure_callback_port, + _make_callback_waiter, + _make_redirect_handler, + ) + + flow = DashboardOAuthFlow( + flow_id="flow-4", + server_name="reports", + profile=None, + redirect_uri="https://agent.example/mcp/oauth/callback/flow-4", + ) + cfg = {} + with dashboard_oauth_flow(flow): + assert _configure_callback_port(cfg, HermesTokenStorage("reports")) == 0 + metadata = _build_client_metadata(cfg) + assert str(metadata.redirect_uris[0]) == flow.redirect_uri + + asyncio.run(_make_redirect_handler(0)("https://idp.example/authorize")) + 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" + + +def test_manager_build_allows_dashboard_flow_without_tty(tmp_path, monkeypatch): + from tools.mcp_dashboard_oauth import DashboardOAuthFlow, dashboard_oauth_flow + from tools.mcp_oauth_manager import MCPOAuthManager + + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + monkeypatch.setattr("tools.mcp_oauth.sys.stdin.isatty", lambda: False) + flow = DashboardOAuthFlow( + flow_id="flow-5", + server_name="reports", + profile=None, + redirect_uri="https://agent.example/api/mcp/oauth/callback/flow-5", + ) + with dashboard_oauth_flow(flow): + provider = MCPOAuthManager().get_or_build_provider( + "reports", "https://mcp.example/mcp", {} + ) + assert provider is not None + assert str(provider.context.client_metadata.redirect_uris[0]) == flow.redirect_uri + + +def test_manager_evict_preserves_persisted_oauth_state(tmp_path, monkeypatch): + from tools.mcp_oauth import HermesTokenStorage + from tools.mcp_oauth_manager import MCPOAuthManager, _ProviderEntry + + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + storage = HermesTokenStorage("reports") + storage._tokens_path().parent.mkdir(parents=True) + storage._tokens_path().write_text( + '{"access_token":"a","token_type":"Bearer"}' + ) + manager = MCPOAuthManager() + manager._entries["reports"] = _ProviderEntry( + server_url="https://mcp.example/mcp", oauth_config={} + ) + + manager.evict("reports") + + assert "reports" not in manager._entries + assert storage._tokens_path().exists() diff --git a/tools/mcp_dashboard_oauth.py b/tools/mcp_dashboard_oauth.py new file mode 100644 index 0000000000000..d436066544c32 --- /dev/null +++ b/tools/mcp_dashboard_oauth.py @@ -0,0 +1,110 @@ +"""Dashboard-mediated callback bridge for MCP OAuth. + +The MCP SDK remains responsible for discovery, DCR, PKCE, state validation and +token exchange. This module only moves the two human/browser callbacks from a +loopback listener into the already-authenticated dashboard session. +""" + +from __future__ import annotations + +import asyncio +import contextvars +import threading +import time +from contextlib import contextmanager +from dataclasses import dataclass, field +from typing import Iterator + + +@dataclass +class DashboardOAuthFlow: + flow_id: str + server_name: str + profile: str | None + redirect_uri: str + created_at: float = field(default_factory=time.time) + status: str = "starting" + authorization_url: str | None = None + error: str | None = None + tools: list[dict] = field(default_factory=list) + _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: + self.authorization_url = url + self.status = "authorization_required" + self._authorization_ready.set() + + async def wait_for_authorization_url(self, timeout: float = 30.0) -> str: + ready = await asyncio.to_thread(self._authorization_ready.wait, timeout) + if not ready: + raise TimeoutError("Timed out waiting for MCP authorization URL") + if not self.authorization_url: + raise RuntimeError(self.error or "MCP OAuth flow ended before authorization") + return self.authorization_url + + def deliver_callback( + self, + *, + code: str | None, + state: str | None, + error: str | None, + ) -> None: + if self._callback_ready.is_set(): + raise ValueError("OAuth callback already received") + if error: + self._callback_error = error + elif code: + self._callback = (code, state) + else: + self._callback_error = "OAuth callback did not include code or error" + self._callback_ready.set() + + async def wait_for_callback(self, timeout: float = 300.0) -> tuple[str, str | None]: + ready = await asyncio.to_thread(self._callback_ready.wait, timeout) + if not ready: + raise TimeoutError("Timed out waiting for MCP OAuth callback") + if self._callback_error: + raise RuntimeError(f"OAuth authorization failed: {self._callback_error}") + if self._callback is None: + raise RuntimeError("OAuth callback did not include an authorization code") + return self._callback + + def mark_approved(self) -> None: + self.status = "approved" + self.error = None + + def mark_error(self, error: str) -> None: + self.status = "error" + self.error = error + self._authorization_ready.set() + self._callback_ready.set() + + def snapshot(self) -> dict: + return { + "flow_id": self.flow_id, + "server_name": self.server_name, + "status": self.status, + "authorization_url": self.authorization_url, + "error": self.error, + } + + +_current_dashboard_flow: contextvars.ContextVar[DashboardOAuthFlow | None] = ( + contextvars.ContextVar("mcp_dashboard_oauth_flow", default=None) +) + + +@contextmanager +def dashboard_oauth_flow(flow: DashboardOAuthFlow) -> Iterator[None]: + token = _current_dashboard_flow.set(flow) + try: + yield + finally: + _current_dashboard_flow.reset(token) + + +def get_dashboard_oauth_flow() -> DashboardOAuthFlow | None: + return _current_dashboard_flow.get() diff --git a/tools/mcp_oauth.py b/tools/mcp_oauth.py index 7959852319f16..e50fcbc9c2f53 100644 --- a/tools/mcp_oauth.py +++ b/tools/mcp_oauth.py @@ -630,6 +630,13 @@ async def _redirect_handler(authorization_url: str) -> None: Opens the browser automatically when possible; always prints the URL as a fallback for headless/SSH/gateway environments. """ + from tools.mcp_dashboard_oauth import get_dashboard_oauth_flow + + dashboard_flow = get_dashboard_oauth_flow() + if dashboard_flow is not None: + await dashboard_flow.publish_authorization_url(authorization_url) + return + # Fail fast at the authorization boundary in non-interactive contexts # (systemd gateway, cron, background MCP discovery). A cached-but-unusable # token (expired/revoked, refresh rejected) makes the SDK fall through to @@ -743,6 +750,12 @@ def _make_callback_waiter(port: int): """ async def _wait() -> tuple[str, str | None]: + from tools.mcp_dashboard_oauth import get_dashboard_oauth_flow + + dashboard_flow = get_dashboard_oauth_flow() + if dashboard_flow is not None: + return await dashboard_flow.wait_for_callback() + # Reject before binding the callback listener in non-interactive # contexts. Reaching here means the SDK entered the authorization-code # flow (a valid or refreshable token would never call the callback @@ -972,6 +985,13 @@ def _configure_callback_port( consolidation PR. """ global _oauth_port + from tools.mcp_dashboard_oauth import get_dashboard_oauth_flow + + dashboard_flow = get_dashboard_oauth_flow() + if dashboard_flow is not None: + cfg["_resolved_port"] = 0 + cfg["redirect_uri"] = dashboard_flow.redirect_uri + return 0 requested = int(cfg.get("redirect_port", 0)) # Precedence: explicit config port → cached client-registration port → # fresh ephemeral port. The cached port keeps re-auth consistent with the diff --git a/tools/mcp_oauth_manager.py b/tools/mcp_oauth_manager.py index f4a37683ff6a0..087a1af91a98e 100644 --- a/tools/mcp_oauth_manager.py +++ b/tools/mcp_oauth_manager.py @@ -532,7 +532,13 @@ def _build_provider( cfg = dict(entry.oauth_config or {}) storage = HermesTokenStorage(server_name) - if not _is_interactive() and not storage.has_cached_tokens(): + from tools.mcp_dashboard_oauth import get_dashboard_oauth_flow + + if ( + get_dashboard_oauth_flow() is None + and not _is_interactive() + and not storage.has_cached_tokens() + ): raise OAuthNonInteractiveError( "MCP OAuth for " f"'{server_name}': non-interactive environment and no " @@ -576,6 +582,11 @@ def remove(self, server_name: str) -> None: server_name, ) + def evict(self, server_name: str) -> None: + """Drop only the in-process provider, preserving persisted OAuth state.""" + with self._entries_lock: + self._entries.pop(server_name, None) + # -- Disk watch ---------------------------------------------------------- async def invalidate_if_disk_changed(self, server_name: str) -> bool: diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py index 4438a715718a1..b0c1b976bcfe5 100644 --- a/tools/mcp_tool.py +++ b/tools/mcp_tool.py @@ -3775,6 +3775,27 @@ async def _scoped(): return _scoped() +def _wrap_with_dashboard_oauth_flow(coro): + """Propagate a dashboard OAuth flow onto the dedicated MCP loop task.""" + try: + from tools.mcp_dashboard_oauth import ( + dashboard_oauth_flow, + get_dashboard_oauth_flow, + ) + + flow = get_dashboard_oauth_flow() + except Exception: + return coro + if flow is None: + return coro + + async def _scoped(): + with dashboard_oauth_flow(flow): + return await coro + + return _scoped() + + def _run_on_mcp_loop(coro_or_factory, timeout: float = 30): """Schedule a coroutine on the MCP event loop and block until done. @@ -3809,6 +3830,7 @@ def _run_on_mcp_loop(coro_or_factory, timeout: float = 30): # task's own context (task-local — concurrent calls carrying different # scopes don't interfere). No-op when no override is active. coro = _wrap_with_home_override(coro) + coro = _wrap_with_dashboard_oauth_flow(coro) future = safe_schedule_threadsafe( coro, loop, diff --git a/web/src/lib/api.ts b/web/src/lib/api.ts index 2691b577d229f..114b2c89576dc 100644 --- a/web/src/lib/api.ts +++ b/web/src/lib/api.ts @@ -996,10 +996,14 @@ export const api = { body: JSON.stringify(body), }), authMcpServer: (name: string) => - fetchJSON( + fetchJSON( `/api/mcp/servers/${encodeURIComponent(name)}/auth`, { method: "POST" }, ), + getMcpOAuthFlow: (flowId: string) => + fetchJSON( + `/api/mcp/oauth/flows/${encodeURIComponent(flowId)}`, + ), removeMcpServer: (name: string) => fetchJSON<{ ok: boolean }>(`/api/mcp/servers/${encodeURIComponent(name)}`, { method: "DELETE", @@ -1465,6 +1469,15 @@ export interface McpTestResult { tools: Array<{ name: string; description: string }>; } +export interface McpOAuthFlow { + flow_id: string; + server_name: string; + status: "starting" | "authorization_required" | "approved" | "error"; + authorization_url: string | null; + error: string | null; + tools?: Array<{ name: string; description: string }>; +} + export interface MessagingPlatformEnvVar { key: string; required: boolean; diff --git a/web/src/lib/mcp-dashboard-oauth.test.ts b/web/src/lib/mcp-dashboard-oauth.test.ts new file mode 100644 index 0000000000000..7de079e0c0492 --- /dev/null +++ b/web/src/lib/mcp-dashboard-oauth.test.ts @@ -0,0 +1,84 @@ +import { describe, expect, it, vi } from "vitest"; +import { completeMcpDashboardOAuth } from "./mcp-dashboard-oauth"; + +describe("completeMcpDashboardOAuth", () => { + it("opens the authorization URL in the dashboard browser and polls to approval", async () => { + const authWindow = { location: { href: "" }, opener: {} } as unknown as Window; + const open = vi.fn().mockReturnValue(authWindow); + const start = vi.fn().mockResolvedValue({ + flow_id: "flow-1", + server_name: "reports", + status: "authorization_required", + authorization_url: "https://idp.example/authorize", + error: null, + }); + const status = vi + .fn() + .mockResolvedValueOnce({ + flow_id: "flow-1", + server_name: "reports", + status: "authorization_required", + authorization_url: "https://idp.example/authorize", + error: null, + tools: [], + }) + .mockResolvedValueOnce({ + flow_id: "flow-1", + server_name: "reports", + status: "approved", + authorization_url: "https://idp.example/authorize", + error: null, + tools: [{ name: "list_reports", description: "List reports" }], + }); + + const result = await completeMcpDashboardOAuth({ + serverName: "reports", + start, + status, + open, + sleep: async () => {}, + }); + + expect(open).toHaveBeenCalledWith( + "about:blank", + "_blank", + ); + expect(authWindow.opener).toBeNull(); + expect(authWindow.location.href).toBe("https://idp.example/authorize"); + expect(status).toHaveBeenCalledTimes(2); + expect(result.status).toBe("approved"); + }); + + it("surfaces a terminal OAuth error", async () => { + const close = vi.fn(); + await expect( + completeMcpDashboardOAuth({ + serverName: "reports", + start: async () => ({ + flow_id: "flow-2", + server_name: "reports", + status: "error", + authorization_url: null, + error: "registration denied", + }), + status: vi.fn(), + open: vi.fn().mockReturnValue({ location: { href: "" }, close }), + sleep: async () => {}, + }), + ).rejects.toThrow("registration denied"); + expect(close).toHaveBeenCalledOnce(); + }); + + it("fails before starting when the browser blocks the popup", async () => { + const start = vi.fn(); + await expect( + completeMcpDashboardOAuth({ + serverName: "reports", + start, + status: vi.fn(), + open: vi.fn().mockReturnValue(null), + }), + ).rejects.toThrow("popup was blocked"); + expect(start).not.toHaveBeenCalled(); + }); +}); diff --git a/web/src/lib/mcp-dashboard-oauth.ts b/web/src/lib/mcp-dashboard-oauth.ts new file mode 100644 index 0000000000000..62bbfb8975ed7 --- /dev/null +++ b/web/src/lib/mcp-dashboard-oauth.ts @@ -0,0 +1,51 @@ +import type { McpOAuthFlow } from "./api"; + +type CompleteOptions = { + serverName: string; + start: (name: string) => Promise; + status: (flowId: string) => Promise; + open: (url?: string | URL, target?: string, features?: string) => unknown; + sleep?: (milliseconds: number) => Promise; +}; + +const defaultSleep = (milliseconds: number) => + new Promise((resolve) => window.setTimeout(resolve, milliseconds)); + +export async function completeMcpDashboardOAuth({ + serverName, + start, + status, + open, + sleep = defaultSleep, +}: CompleteOptions): Promise { + // Open synchronously from the click handler, before the first await. Browsers + // otherwise classify the later OAuth popup as unsolicited and block it. + const authWindow = open("about:blank", "_blank") as Window | null; + if (!authWindow) { + throw new Error("OAuth popup was blocked — allow popups for this dashboard and retry"); + } + authWindow.opener = null; + let started: McpOAuthFlow; + try { + started = await start(serverName); + if (started.status === "error") { + throw new Error(started.error || "OAuth failed to start"); + } + if (!started.authorization_url) { + throw new Error("OAuth server did not provide an authorization URL"); + } + authWindow.location.href = started.authorization_url; + } catch (error) { + authWindow.close(); + throw error; + } + + for (;;) { + const current = await status(started.flow_id); + if (current.status === "approved") return current; + if (current.status === "error") { + throw new Error(current.error || "OAuth authorization failed"); + } + await sleep(1000); + } +} diff --git a/web/src/pages/McpPage.tsx b/web/src/pages/McpPage.tsx index 1939c1f5756f2..454411bf14ec2 100644 --- a/web/src/pages/McpPage.tsx +++ b/web/src/pages/McpPage.tsx @@ -27,6 +27,7 @@ import { buildMcpServerCreate, type McpTransport, } from "@/lib/mcp-server-create"; +import { completeMcpDashboardOAuth } from "@/lib/mcp-dashboard-oauth"; function isHttpUrl(value: string): boolean { return /^https?:\/\//i.test(value.trim()); @@ -183,13 +184,17 @@ export default function McpPage() { const handleAuthenticate = async (server: McpServer) => { setAuthenticating(server.name); try { - const result = await api.authMcpServer(server.name); - setTestResults((prev) => ({ ...prev, [server.name]: result })); - if (result.ok) { - showToast(`${server.name}: OAuth authentication complete`, "success"); - } else { - showToast(`${server.name}: ${result.error ?? "OAuth failed"}`, "error"); - } + const result = await completeMcpDashboardOAuth({ + serverName: server.name, + start: api.authMcpServer, + status: api.getMcpOAuthFlow, + open: window.open.bind(window), + }); + setTestResults((prev) => ({ + ...prev, + [server.name]: { ok: true, tools: result.tools ?? [] }, + })); + showToast(`${server.name}: OAuth authentication complete`, "success"); } catch (e) { showToast(`OAuth error: ${e}`, "error"); } finally { From 468aeb31a5f6896796445059f898ce1bd76eeaf5 Mon Sep 17 00:00:00 2001 From: Ben Barclay Date: Fri, 17 Jul 2026 13:56:59 +1000 Subject: [PATCH 2/7] 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 97e3c828d23a6..f2ea0bdb78c89 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -11327,6 +11327,7 @@ def _probe_scoped(): _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 b569fd70b88e8..142b4ba54ac13 100644 --- a/tests/hermes_cli/test_mcp_dashboard_oauth.py +++ b/tests/hermes_cli/test_mcp_dashboard_oauth.py @@ -54,6 +54,8 @@ def fake_worker(flow, cfg): 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 35976d4e965ff..77e9630e340a7 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 d436066544c32..112049e4b4c2f 100644 --- a/tools/mcp_dashboard_oauth.py +++ b/tools/mcp_dashboard_oauth.py @@ -9,11 +9,13 @@ 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 @@ def deliver_callback( ) -> 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: From 81198f6e0de81eac28f8424f994ab10c38980c5a Mon Sep 17 00:00:00 2001 From: Ben Barclay Date: Fri, 17 Jul 2026 14:00:09 +1000 Subject: [PATCH 3/7] fix(mcp): serialize hosted oauth reauthorization --- hermes_cli/web_server.py | 9 ++++++++ tests/hermes_cli/test_mcp_dashboard_oauth.py | 23 ++++++++++++++++++++ tests/tools/test_mcp_dashboard_oauth.py | 22 +++++++++++++++++++ tools/mcp_dashboard_oauth.py | 2 ++ 4 files changed, 56 insertions(+) diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index f2ea0bdb78c89..e6acc55428335 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -11439,6 +11439,15 @@ async def auth_mcp_server(name: str, request: Request, profile: Optional[str] = status_code=429, detail="Too many MCP OAuth flows are already in progress", ) + if any( + flow.server_name == name + and flow.status in {"starting", "authorization_required"} + for flow in _mcp_oauth_flows.values() + ): + raise HTTPException( + status_code=409, + detail=f"MCP OAuth for '{name}' is already in progress", + ) with _profile_scope(profile): servers = _get_mcp_servers() if name not in servers: diff --git a/tests/hermes_cli/test_mcp_dashboard_oauth.py b/tests/hermes_cli/test_mcp_dashboard_oauth.py index 142b4ba54ac13..fdf4be82f15f8 100644 --- a/tests/hermes_cli/test_mcp_dashboard_oauth.py +++ b/tests/hermes_cli/test_mcp_dashboard_oauth.py @@ -159,6 +159,29 @@ def test_hosted_auth_start_bounds_pending_flow_registry(): assert response.status_code == 429 +def test_hosted_auth_rejects_overlapping_flow_for_same_server(): + 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"}, + ) + existing = DashboardOAuthFlow( + flow_id="existing-reports", + server_name="reports", + profile="other-profile", + redirect_uri="https://agent.example/callback/existing", + ) + web_server._mcp_oauth_flows[existing.flow_id] = existing + + response = client.post("/api/mcp/servers/reports/auth") + + assert response.status_code == 409 + assert "already in progress" in response.text + + 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 77e9630e340a7..116612668604a 100644 --- a/tests/tools/test_mcp_dashboard_oauth.py +++ b/tests/tools/test_mcp_dashboard_oauth.py @@ -72,6 +72,28 @@ def test_dashboard_flow_rejects_second_callback(): flow.deliver_callback(code="second", state="state", error=None) +def test_dashboard_flow_cannot_resurrect_after_terminal_error(): + from tools.mcp_dashboard_oauth import DashboardOAuthFlow + + flow = DashboardOAuthFlow( + flow_id="flow-terminal", + server_name="reports", + profile=None, + redirect_uri="https://agent.example/mcp/oauth/callback/flow-terminal", + ) + flow.mark_error("start timed out") + + with pytest.raises(RuntimeError, match="already ended"): + asyncio.run( + flow.publish_authorization_url( + "https://idp.example/authorize?state=too-late" + ) + ) + + assert flow.status == "error" + assert flow.authorization_url is None + + def test_dashboard_context_overrides_redirect_and_handlers(): from tools.mcp_dashboard_oauth import ( DashboardOAuthFlow, diff --git a/tools/mcp_dashboard_oauth.py b/tools/mcp_dashboard_oauth.py index 112049e4b4c2f..da2fda39bc7ae 100644 --- a/tools/mcp_dashboard_oauth.py +++ b/tools/mcp_dashboard_oauth.py @@ -36,6 +36,8 @@ class DashboardOAuthFlow: _callback_ready: threading.Event = field(default_factory=threading.Event, init=False, repr=False) async def publish_authorization_url(self, url: str) -> None: + if self.status in {"approved", "error"}: + raise RuntimeError("OAuth flow already ended") state = parse_qs(urlparse(url).query).get("state", [None])[0] if not state: raise ValueError("OAuth authorization URL did not include state") From 87b78e407818ab98785caaaf95fd87771f49c080 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Fri, 17 Jul 2026 00:16:07 -0700 Subject: [PATCH 4/7] fix(mcp): harden hosted OAuth across profiles and clients --- apps/desktop/src/app/skills/mcp-tab.tsx | 11 +- apps/desktop/src/hermes.ts | 25 ++- .../src/lib/mcp-dashboard-oauth.test.ts | 35 ++++ apps/desktop/src/lib/mcp-dashboard-oauth.ts | 53 ++++++ hermes_cli/web_server.py | 169 ++++++++++++------ tests/hermes_cli/test_mcp_config.py | 4 +- tests/hermes_cli/test_mcp_dashboard_oauth.py | 64 ++++++- tests/tools/test_mcp_dashboard_oauth.py | 65 ++++++- tests/tools/test_mcp_oauth_manager.py | 41 ++++- tools/mcp_dashboard_oauth.py | 79 ++++---- tools/mcp_oauth.py | 2 +- tools/mcp_oauth_manager.py | 53 ++++-- tools/mcp_tool.py | 9 + web/src/lib/mcp-dashboard-oauth.test.ts | 30 ++++ web/src/lib/mcp-dashboard-oauth.ts | 3 + 15 files changed, 519 insertions(+), 124 deletions(-) create mode 100644 apps/desktop/src/lib/mcp-dashboard-oauth.test.ts create mode 100644 apps/desktop/src/lib/mcp-dashboard-oauth.ts diff --git a/apps/desktop/src/app/skills/mcp-tab.tsx b/apps/desktop/src/app/skills/mcp-tab.tsx index 3532405becfde..522261b0e1043 100644 --- a/apps/desktop/src/app/skills/mcp-tab.tsx +++ b/apps/desktop/src/app/skills/mcp-tab.tsx @@ -30,6 +30,7 @@ import { getActionStatus, getLogs, getMcpCatalog, + getMcpOAuthFlow, type HermesGateway, installMcpCatalogEntry, type McpCatalogEntry, @@ -38,6 +39,7 @@ import { testMcpServer } from '@/hermes' import { type Translations, useI18n } from '@/i18n' +import { completeMcpDesktopOAuth } from '@/lib/mcp-dashboard-oauth' import { countEnabledTools, isToolEnabled, toggleToolInServer } from '@/lib/mcp-tool-filter' import { cn } from '@/lib/utils' import { notify, notifyError } from '@/store/notifications' @@ -578,7 +580,14 @@ export function McpTab({ gateway }: { gateway: HermesGateway | null }) { setProbes(current => ({ ...current, [serverName]: 'probing' })) try { - const result = await authMcpServer(serverName) + const flow = await completeMcpDesktopOAuth({ + serverName, + start: authMcpServer, + status: getMcpOAuthFlow, + openExternal: url => window.hermesDesktop.openExternal(url) + }) + + const result: McpTestResult = { ok: true, tools: flow.tools ?? [] } // Bail if the user switched profiles mid-flow — this result is profile A's. if (profileEpoch.current !== epoch) { diff --git a/apps/desktop/src/hermes.ts b/apps/desktop/src/hermes.ts index 01c9eb6ec8573..e637e0763137a 100644 --- a/apps/desktop/src/hermes.ts +++ b/apps/desktop/src/hermes.ts @@ -709,6 +709,15 @@ export interface McpTestResult { resources?: number } +export interface McpOAuthFlow { + flow_id: string + server_name: string + status: 'starting' | 'authorization_required' | 'approved' | 'error' + authorization_url: string | null + error: string | null + tools?: { name: string; description: string }[] +} + /** Connect to the server, list its tools, disconnect. Slow (spawns/handshakes * for real) — well past the 15s default fetch timeout. */ export function testMcpServer(name: string): Promise { @@ -732,14 +741,20 @@ export function saveMcpServers(servers: Record>) }) } -/** Run the OAuth flow for an HTTP server — opens the system browser and blocks - * until the user finishes (or gives up), hence the very generous timeout. */ -export function authMcpServer(name: string): Promise { - return window.hermesDesktop.api({ +/** Start an MCP OAuth flow and return the authorization URL. */ +export function authMcpServer(name: string): Promise { + return window.hermesDesktop.api({ ...profileScoped(), path: `/api/mcp/servers/${encodeURIComponent(name)}/auth`, method: 'POST', - timeoutMs: 300_000 + timeoutMs: 60_000 + }) +} + +export function getMcpOAuthFlow(flowId: string): Promise { + return window.hermesDesktop.api({ + ...profileScoped(), + path: `/api/mcp/oauth/flows/${encodeURIComponent(flowId)}` }) } diff --git a/apps/desktop/src/lib/mcp-dashboard-oauth.test.ts b/apps/desktop/src/lib/mcp-dashboard-oauth.test.ts new file mode 100644 index 0000000000000..4473cd9e1b355 --- /dev/null +++ b/apps/desktop/src/lib/mcp-dashboard-oauth.test.ts @@ -0,0 +1,35 @@ +import { describe, expect, it, vi } from 'vitest' + +import { completeMcpDesktopOAuth } from './mcp-dashboard-oauth' + +describe('completeMcpDesktopOAuth', () => { + it('opens the returned authorization URL and polls through approval', async () => { + const openExternal = vi.fn().mockResolvedValue(undefined) + + const status = vi + .fn() + .mockResolvedValueOnce({ + flow_id: 'flow-1', server_name: 'reports', status: 'authorization_required', + authorization_url: 'https://idp.example/authorize', error: null + }) + .mockResolvedValueOnce({ + flow_id: 'flow-1', server_name: 'reports', status: 'approved', + authorization_url: 'https://idp.example/authorize', error: null, + tools: [{ name: 'list_reports', description: 'List reports' }] + }) + + const result = await completeMcpDesktopOAuth({ + serverName: 'reports', + start: vi.fn().mockResolvedValue({ + flow_id: 'flow-1', server_name: 'reports', status: 'authorization_required', + authorization_url: 'https://idp.example/authorize', error: null + }), + status, + openExternal, + sleep: async () => {} + }) + + expect(openExternal).toHaveBeenCalledWith('https://idp.example/authorize') + expect(result.status).toBe('approved') + }) +}) \ No newline at end of file diff --git a/apps/desktop/src/lib/mcp-dashboard-oauth.ts b/apps/desktop/src/lib/mcp-dashboard-oauth.ts new file mode 100644 index 0000000000000..5cd9570099df1 --- /dev/null +++ b/apps/desktop/src/lib/mcp-dashboard-oauth.ts @@ -0,0 +1,53 @@ +export interface McpOAuthFlow { + flow_id: string + server_name: string + status: 'starting' | 'authorization_required' | 'approved' | 'error' + authorization_url: string | null + error: string | null + tools?: Array<{ name: string; description: string }> +} + +interface CompleteOptions { + serverName: string + start: (name: string) => Promise + status: (flowId: string) => Promise + openExternal: (url: string) => Promise + sleep?: (milliseconds: number) => Promise +} + +const defaultSleep = (milliseconds: number) => + new Promise(resolve => window.setTimeout(resolve, milliseconds)) + +export async function completeMcpDesktopOAuth({ + serverName, + start, + status, + openExternal, + sleep = defaultSleep +}: CompleteOptions): Promise { + const started = await start(serverName) + + if (started.status === 'error') { + throw new Error(started.error || 'OAuth failed to start') + } + + if (!started.authorization_url) { + throw new Error('OAuth server did not provide an authorization URL') + } + + await openExternal(started.authorization_url) + + for (;;) { + const current = await status(started.flow_id) + + if (current.status === 'approved') { + return current + } + + if (current.status === 'error') { + throw new Error(current.error || 'OAuth authorization failed') + } + + await sleep(1000) + } +} \ No newline at end of file diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index e6acc55428335..ddb7e79b2eee9 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -11329,26 +11329,38 @@ def _probe_scoped(): _MCP_DASHBOARD_OAUTH_TTL = 15 * 60 _MAX_PENDING_MCP_OAUTH_FLOWS = 8 _mcp_oauth_flows: dict[str, "DashboardOAuthFlow"] = {} +_mcp_oauth_flows_lock = threading.Lock() +_mcp_oauth_transactions: dict[tuple[str, str], threading.Lock] = {} +_mcp_oauth_transactions_lock = threading.Lock() def _gc_mcp_oauth_flows() -> None: cutoff = time.time() - _MCP_DASHBOARD_OAUTH_TTL - stale = [ - flow_id - for flow_id, flow in _mcp_oauth_flows.items() - if getattr(flow, "created_at", 0) < cutoff - ] - for flow_id in stale: - _mcp_oauth_flows.pop(flow_id, None) + with _mcp_oauth_flows_lock: + stale = [ + flow_id + for flow_id, flow in _mcp_oauth_flows.items() + if getattr(flow, "created_at", 0) < cutoff + ] + for flow_id in stale: + _mcp_oauth_flows.pop(flow_id, None) + +def _mcp_oauth_callback_url_from_base(base_url: str, server_name: str) -> str: + from urllib.parse import quote -def _mcp_oauth_callback_url(request: Request, flow_id: str) -> str: + return f"{base_url.rstrip('/')}/api/mcp/oauth/callback/{quote(server_name, safe='')}" + + +def _mcp_oauth_callback_url(request: Request, server_name: str) -> str: """Build the externally reachable callback URL for a dashboard flow.""" from urllib.parse import urlparse, urlunparse from hermes_cli.dashboard_auth.prefix import prefix_from_request, resolve_public_url - suffix = f"/api/mcp/oauth/callback/{flow_id}" + from urllib.parse import quote + + suffix = f"/api/mcp/oauth/callback/{quote(server_name, safe='')}" public_url = resolve_public_url() if public_url: return f"{public_url}{suffix}" @@ -11357,6 +11369,12 @@ def _mcp_oauth_callback_url(request: Request, flow_id: str) -> str: return urlunparse(base._replace(path=f"{prefix}{suffix}", params="", query="", fragment="")) +def _mcp_oauth_transaction(flow) -> threading.Lock: + key = (flow.hermes_home, flow.server_name) + with _mcp_oauth_transactions_lock: + return _mcp_oauth_transactions.setdefault(key, threading.Lock()) + + def _run_dashboard_mcp_oauth(flow, cfg: dict) -> None: """Run the normal MCP probe with dashboard redirect/callback handlers.""" from hermes_cli.mcp_config import ( @@ -11365,35 +11383,47 @@ def _run_dashboard_mcp_oauth(flow, cfg: dict) -> None: _save_mcp_server, ) try: + from agent.secret_scope import ( + build_profile_secret_scope, + reset_secret_scope, + set_secret_scope, + ) + from hermes_constants import reset_hermes_home_override, set_hermes_home_override from tools.mcp_dashboard_oauth import dashboard_oauth_flow from tools.mcp_oauth import HermesTokenStorage, force_interactive_oauth from tools.mcp_oauth_manager import get_manager - with ( - _config_profile_scope(flow.profile), - force_interactive_oauth(), - dashboard_oauth_flow(flow), - ): - storage = HermesTokenStorage(flow.server_name) - backup = storage.snapshot() - try: - get_manager().remove(flow.server_name) - tools = _probe_single_server( - flow.server_name, - cfg, - connect_timeout=max(float(cfg.get("connect_timeout", 0) or 0), 315), - ) - if not _oauth_tokens_present(flow.server_name): - raise RuntimeError( - "The server responded, but no OAuth token was obtained — " - "this provider may require a manually-registered OAuth client." + home_token = set_hermes_home_override(flow.hermes_home) + secret_token = set_secret_scope(build_profile_secret_scope(Path(flow.hermes_home))) + try: + transaction = _mcp_oauth_transaction(flow) + with transaction, force_interactive_oauth(), dashboard_oauth_flow(flow): + storage = HermesTokenStorage(flow.server_name) + backup = storage.snapshot() + try: + get_manager().remove(flow.server_name, hermes_home=flow.hermes_home) + tools = _probe_single_server( + flow.server_name, + cfg, + connect_timeout=max(float(cfg.get("connect_timeout", 0) or 0), 315), ) - _save_mcp_server(flow.server_name, cfg) - flow.tools = [{"name": t, "description": d} for t, d in tools] - flow.mark_approved() - except Exception: - storage.restore(backup) - raise + if not _oauth_tokens_present(flow.server_name): + raise RuntimeError( + "The server responded, but no OAuth token was obtained — " + "this provider may require a manually-registered OAuth client." + ) + _save_mcp_server(flow.server_name, cfg) + flow.tools = [{"name": t, "description": d} for t, d in tools] + flow.mark_approved() + from tools.mcp_tool import reconnect_mcp_server + + reconnect_mcp_server(flow.server_name) + except Exception: + storage.restore(backup) + raise + finally: + reset_secret_scope(secret_token) + reset_hermes_home_override(home_token) except Exception as exc: msg = str(exc) # Providers that gate RFC 7591 registration to pre-approved clients @@ -11417,7 +11447,7 @@ def _run_dashboard_mcp_oauth(flow, cfg: dict) -> None: try: from tools.mcp_oauth_manager import get_manager - get_manager().evict(flow.server_name) + get_manager().evict(flow.server_name, hermes_home=flow.hermes_home) except Exception: pass @@ -11430,26 +11460,11 @@ 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", - ) - if any( - flow.server_name == name - and flow.status in {"starting", "authorization_required"} - for flow in _mcp_oauth_flows.values() - ): - raise HTTPException( - status_code=409, - detail=f"MCP OAuth for '{name}' is already in progress", - ) with _profile_scope(profile): servers = _get_mcp_servers() + from hermes_constants import get_hermes_home + + flow_home = str(get_hermes_home().expanduser().resolve(strict=False)) if name not in servers: raise HTTPException(status_code=404, detail=f"Server '{name}' not found") cfg = dict(servers[name]) @@ -11459,14 +11474,38 @@ async def auth_mcp_server(name: str, request: Request, profile: Optional[str] = raise HTTPException(status_code=400, detail="This server uses header/API-key auth, not OAuth") cfg["auth"] = "oauth" + with _mcp_oauth_flows_lock: + 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", + ) + if any( + flow.server_name == name + and flow.hermes_home == flow_home + and flow.status in {"starting", "authorization_required"} + for flow in _mcp_oauth_flows.values() + ): + raise HTTPException( + status_code=409, + detail=f"MCP OAuth for '{name}' is already in progress", + ) + flow_id = secrets.token_urlsafe(24) flow = DashboardOAuthFlow( flow_id=flow_id, server_name=name, profile=profile, - redirect_uri=_mcp_oauth_callback_url(request, flow_id), + hermes_home=flow_home, + redirect_uri=(cfg.get("oauth") or {}).get("redirect_uri") + or _mcp_oauth_callback_url(request, name), ) - _mcp_oauth_flows[flow_id] = flow + with _mcp_oauth_flows_lock: + _mcp_oauth_flows[flow_id] = flow threading.Thread( target=_run_dashboard_mcp_oauth, args=(flow, cfg), @@ -11492,15 +11531,31 @@ async def mcp_oauth_flow_status(flow_id: str, request: Request): return snapshot -@app.get("/api/mcp/oauth/callback/{flow_id}") +@app.get("/api/mcp/oauth/callback/{server_name}") async def mcp_oauth_callback( - flow_id: str, + server_name: str, code: Optional[str] = None, state: Optional[str] = None, error: Optional[str] = None, ): _gc_mcp_oauth_flows() - flow = _mcp_oauth_flows.get(flow_id) + with _mcp_oauth_flows_lock: + candidates = [ + flow + for flow in _mcp_oauth_flows.values() + if flow.server_name == server_name + and flow.status == "authorization_required" + ] + flow = next( + ( + candidate + for candidate in candidates + if candidate.expected_state is not None + and state is not None + and secrets.compare_digest(candidate.expected_state, state) + ), + None, + ) if flow is None: return HTMLResponse("

OAuth flow expired

Return to Hermes and try again.

", status_code=404) try: diff --git a/tests/hermes_cli/test_mcp_config.py b/tests/hermes_cli/test_mcp_config.py index 64e24c42470fc..f79e29ff7f031 100644 --- a/tests/hermes_cli/test_mcp_config.py +++ b/tests/hermes_cli/test_mcp_config.py @@ -839,12 +839,12 @@ def test_remove_evicts_in_memory_provider(self, tmp_path, capsys, monkeypatch): mgr.get_or_build_provider( "oauth-srv", "https://example.com/mcp", None, ) - assert "oauth-srv" in mgr._entries + assert mgr._key("oauth-srv") in mgr._entries from hermes_cli.mcp_config import cmd_mcp_remove cmd_mcp_remove(_make_args(name="oauth-srv")) - assert "oauth-srv" not in mgr._entries + assert mgr._key("oauth-srv") not in mgr._entries class TestMcpLogin: diff --git a/tests/hermes_cli/test_mcp_dashboard_oauth.py b/tests/hermes_cli/test_mcp_dashboard_oauth.py index fdf4be82f15f8..1862e4373d6bd 100644 --- a/tests/hermes_cli/test_mcp_dashboard_oauth.py +++ b/tests/hermes_cli/test_mcp_dashboard_oauth.py @@ -20,8 +20,10 @@ def _clear_flows(): from hermes_cli import web_server web_server._mcp_oauth_flows.clear() + web_server.app.state.auth_required = False yield web_server._mcp_oauth_flows.clear() + web_server.app.state.auth_required = False def test_hosted_auth_start_returns_public_authorization_url(monkeypatch): @@ -50,7 +52,7 @@ def fake_worker(flow, cfg): assert body["status"] == "authorization_required" assert body["authorization_url"] == "https://idp.example/authorize?state=s1" flow = web_server._mcp_oauth_flows[body["flow_id"]] - assert flow.redirect_uri == f"https://agent.example/api/mcp/oauth/callback/{body['flow_id']}" + assert flow.redirect_uri == "https://agent.example/api/mcp/oauth/callback/reports" def test_hosted_callback_is_public_and_delivers_code(): @@ -64,7 +66,8 @@ def test_hosted_callback_is_public_and_delivers_code(): flow_id="flow-public", server_name="reports", profile=None, - redirect_uri="https://agent.example/api/mcp/oauth/callback/flow-public", + hermes_home="/tmp/hermes-test", + redirect_uri="https://agent.example/api/mcp/oauth/callback/reports", ) asyncio.run( flow.publish_authorization_url( @@ -75,7 +78,7 @@ def test_hosted_callback_is_public_and_delivers_code(): assert "/api/mcp/oauth/callback" not in PUBLIC_API_PATHS response = _client().get( - "/api/mcp/oauth/callback/flow-public?code=abc&state=expected" + "/api/mcp/oauth/callback/reports?code=abc&state=expected" ) assert response.status_code == 200 assert flow._callback == ("abc", "expected") @@ -93,7 +96,8 @@ def test_hosted_callback_bypasses_gated_cookie_auth(monkeypatch): flow_id="flow-gated", server_name="reports", profile=None, - redirect_uri="https://agent.example/api/mcp/oauth/callback/flow-gated", + hermes_home="/tmp/hermes-test", + redirect_uri="https://agent.example/api/mcp/oauth/callback/reports", ) asyncio.run( flow.publish_authorization_url( @@ -104,7 +108,7 @@ def test_hosted_callback_bypasses_gated_cookie_auth(monkeypatch): monkeypatch.setattr(web_server.app.state, "auth_required", True, raising=False) response = TestClient(web_server.app).get( - "/api/mcp/oauth/callback/flow-gated?code=abc&state=expected" + "/api/mcp/oauth/callback/reports?code=abc&state=expected" ) assert response.status_code == 200 @@ -121,7 +125,8 @@ def test_hosted_callback_rejects_wrong_state_before_waking_sdk(): flow_id="flow-state-route", server_name="reports", profile=None, - redirect_uri="https://agent.example/api/mcp/oauth/callback/flow-state-route", + hermes_home="/tmp/hermes-test", + redirect_uri="https://agent.example/api/mcp/oauth/callback/reports", ) asyncio.run( flow.publish_authorization_url( @@ -131,9 +136,9 @@ def test_hosted_callback_rejects_wrong_state_before_waking_sdk(): web_server._mcp_oauth_flows[flow.flow_id] = flow response = _client().get( - "/api/mcp/oauth/callback/flow-state-route?code=attacker&state=wrong" + "/api/mcp/oauth/callback/reports?code=attacker&state=wrong" ) - assert response.status_code == 400 + assert response.status_code == 404 assert flow._callback is None @@ -151,6 +156,7 @@ def test_hosted_auth_start_bounds_pending_flow_registry(): flow_id=f"existing-{index}", server_name="reports", profile=None, + hermes_home="/tmp/hermes-test", redirect_uri=f"https://agent.example/callback/{index}", ) web_server._mcp_oauth_flows[flow.flow_id] = flow @@ -168,10 +174,13 @@ def test_hosted_auth_rejects_overlapping_flow_for_same_server(): "/api/mcp/servers", json={"name": "reports", "url": "https://mcp.example/mcp", "auth": "oauth"}, ) + from hermes_constants import get_hermes_home + existing = DashboardOAuthFlow( flow_id="existing-reports", server_name="reports", profile="other-profile", + hermes_home=str(get_hermes_home().expanduser().resolve(strict=False)), redirect_uri="https://agent.example/callback/existing", ) web_server._mcp_oauth_flows[existing.flow_id] = existing @@ -182,6 +191,44 @@ def test_hosted_auth_rejects_overlapping_flow_for_same_server(): assert "already in progress" in response.text +def test_hosted_auth_allows_same_server_name_in_different_profiles(tmp_path, monkeypatch): + from hermes_cli import web_server + from tools.mcp_dashboard_oauth import DashboardOAuthFlow + + profile_home = tmp_path / "profiles" / "work" + profile_home.mkdir(parents=True) + monkeypatch.setattr(web_server, "_resolve_profile_dir", lambda _name: profile_home) + + existing = DashboardOAuthFlow( + flow_id="existing-default", + server_name="reports", + profile=None, + hermes_home=str(tmp_path / "default"), + redirect_uri="https://agent.example/callback/existing", + ) + web_server._mcp_oauth_flows[existing.flow_id] = existing + + def fake_worker(flow, cfg): + import asyncio + + asyncio.run(flow.publish_authorization_url("https://idp.example/authorize?state=work")) + + with patch("hermes_cli.mcp_config._get_mcp_servers", return_value={"reports": {"url": "https://mcp.example"}}), \ + patch.object(web_server, "_run_dashboard_mcp_oauth", fake_worker): + response = _client().post("/api/mcp/servers/reports/auth?profile=work") + + assert response.status_code != 409 + + +def test_callback_url_is_stable_for_a_server(): + from hermes_cli import web_server + + # The route helper's stable form must not depend on a one-time flow id. + first = web_server._mcp_oauth_callback_url_from_base("https://agent.example", "reports") + second = web_server._mcp_oauth_callback_url_from_base("https://agent.example", "reports") + assert first == second == "https://agent.example/api/mcp/oauth/callback/reports" + + def test_flow_status_does_not_expose_authorization_code(): from hermes_cli import web_server from tools.mcp_dashboard_oauth import DashboardOAuthFlow @@ -190,6 +237,7 @@ def test_flow_status_does_not_expose_authorization_code(): flow_id="flow-status", server_name="reports", profile=None, + hermes_home="/tmp/hermes-test", redirect_uri="https://agent.example/api/mcp/oauth/callback/flow-status", ) flow.authorization_url = "https://idp.example/authorize" diff --git a/tests/tools/test_mcp_dashboard_oauth.py b/tests/tools/test_mcp_dashboard_oauth.py index 116612668604a..586eb13ebe9bd 100644 --- a/tests/tools/test_mcp_dashboard_oauth.py +++ b/tests/tools/test_mcp_dashboard_oauth.py @@ -1,6 +1,7 @@ """Hosted-dashboard bridge for MCP OAuth browser callbacks.""" import asyncio +import threading import pytest @@ -12,6 +13,7 @@ def test_dashboard_flow_exposes_authorization_url_and_accepts_callback(): flow_id="flow-1", server_name="reports", profile=None, + hermes_home="/tmp/hermes-test", redirect_uri="https://agent.example/mcp/oauth/callback/flow-1", ) @@ -35,6 +37,7 @@ def test_dashboard_flow_rejects_wrong_state_without_consuming_callback(): flow_id="flow-state", server_name="reports", profile=None, + hermes_home="/tmp/hermes-test", redirect_uri="https://agent.example/mcp/oauth/callback/flow-state", ) asyncio.run( @@ -60,6 +63,7 @@ def test_dashboard_flow_rejects_second_callback(): flow_id="flow-2", server_name="reports", profile=None, + hermes_home="/tmp/hermes-test", redirect_uri="https://agent.example/mcp/oauth/callback/flow-2", ) asyncio.run( @@ -72,6 +76,39 @@ def test_dashboard_flow_rejects_second_callback(): flow.deliver_callback(code="second", state="state", error=None) +def test_dashboard_flow_accepts_only_one_concurrent_callback(): + from tools.mcp_dashboard_oauth import DashboardOAuthFlow + + flow = DashboardOAuthFlow( + flow_id="flow-race", + server_name="reports", + profile=None, + hermes_home="/tmp/hermes-test", + redirect_uri="https://agent.example/mcp/oauth/callback/flow-race", + ) + asyncio.run(flow.publish_authorization_url("https://idp.example/authorize?state=state")) + + start = threading.Barrier(3) + outcomes: list[str] = [] + + def deliver(code: str) -> None: + start.wait() + try: + flow.deliver_callback(code=code, state="state", error=None) + outcomes.append("accepted") + except ValueError: + outcomes.append("rejected") + + workers = [threading.Thread(target=deliver, args=(code,)) for code in ("one", "two")] + for worker in workers: + worker.start() + start.wait() + for worker in workers: + worker.join() + + assert sorted(outcomes) == ["accepted", "rejected"] + + def test_dashboard_flow_cannot_resurrect_after_terminal_error(): from tools.mcp_dashboard_oauth import DashboardOAuthFlow @@ -79,6 +116,7 @@ def test_dashboard_flow_cannot_resurrect_after_terminal_error(): flow_id="flow-terminal", server_name="reports", profile=None, + hermes_home="/tmp/hermes-test", redirect_uri="https://agent.example/mcp/oauth/callback/flow-terminal", ) flow.mark_error("start timed out") @@ -105,6 +143,7 @@ def test_dashboard_context_overrides_redirect_and_handlers(): flow_id="flow-3", server_name="reports", profile=None, + hermes_home="/tmp/hermes-test", redirect_uri="https://agent.example/mcp/oauth/callback/flow-3", ) assert get_dashboard_oauth_flow() is None @@ -127,6 +166,7 @@ def test_mcp_oauth_helpers_use_dashboard_flow_without_loopback_port(): flow_id="flow-4", server_name="reports", profile=None, + hermes_home="/tmp/hermes-test", redirect_uri="https://agent.example/mcp/oauth/callback/flow-4", ) cfg = {} @@ -156,6 +196,7 @@ def test_manager_build_allows_dashboard_flow_without_tty(tmp_path, monkeypatch): flow_id="flow-5", server_name="reports", profile=None, + hermes_home="/tmp/hermes-test", redirect_uri="https://agent.example/api/mcp/oauth/callback/flow-5", ) with dashboard_oauth_flow(flow): @@ -177,11 +218,31 @@ def test_manager_evict_preserves_persisted_oauth_state(tmp_path, monkeypatch): '{"access_token":"a","token_type":"Bearer"}' ) manager = MCPOAuthManager() - manager._entries["reports"] = _ProviderEntry( + manager._entries[manager._key("reports")] = _ProviderEntry( server_url="https://mcp.example/mcp", oauth_config={} ) manager.evict("reports") - assert "reports" not in manager._entries + assert manager._key("reports") not in manager._entries assert storage._tokens_path().exists() + + +def test_reconnect_mcp_server_signals_live_task(monkeypatch): + from tools import mcp_tool + + class Event: + called = False + + def set(self): + self.called = True + + class Server: + _reconnect_event = Event() + + server = Server() + monkeypatch.setitem(mcp_tool._servers, "reports", server) + monkeypatch.setattr(mcp_tool, "_mcp_loop", None) + + assert mcp_tool.reconnect_mcp_server("reports") is True + assert server._reconnect_event.called is True diff --git a/tests/tools/test_mcp_oauth_manager.py b/tests/tools/test_mcp_oauth_manager.py index 448400cad85fa..d680607aee5b2 100644 --- a/tests/tools/test_mcp_oauth_manager.py +++ b/tests/tools/test_mcp_oauth_manager.py @@ -11,6 +11,41 @@ import pytest + +def test_manager_isolates_same_named_servers_by_profile_home(tmp_path, monkeypatch): + from hermes_constants import reset_hermes_home_override, set_hermes_home_override + from tools.mcp_oauth import HermesTokenStorage + from tools.mcp_oauth_manager import MCPOAuthManager + + profile_a = tmp_path / "profile-a" + profile_b = tmp_path / "profile-b" + for home, access_token in ((profile_a, "TOKEN_A"), (profile_b, "TOKEN_B")): + token = set_hermes_home_override(home) + try: + storage = HermesTokenStorage("shared") + storage._tokens_path().parent.mkdir(parents=True, exist_ok=True) + storage._tokens_path().write_text( + '{"access_token":"%s","token_type":"Bearer","expires_in":3600}' + % access_token + ) + finally: + reset_hermes_home_override(token) + + manager = MCPOAuthManager() + providers = [] + for home in (profile_a, profile_b): + token = set_hermes_home_override(home) + try: + provider = manager.get_or_build_provider("shared", "https://mcp.example/mcp", {}) + asyncio.run(provider._initialize()) + providers.append(provider) + finally: + reset_hermes_home_override(token) + + assert providers[0] is not providers[1] + assert providers[0].context.current_tokens.access_token == "TOKEN_A" + assert providers[1].context.current_tokens.access_token == "TOKEN_B" + pytest.importorskip( "mcp.client.auth.oauth2", reason="MCP SDK 1.26.0+ required for OAuth support", @@ -166,7 +201,7 @@ def add(self, item): # noqa: A003 class _DummyProvider: context = None # forces the can_refresh=False branch - mgr._entries["srv"] = _ProviderEntry( + mgr._entries[mgr._key("srv")] = _ProviderEntry( server_url="https://example.com/mcp", oauth_config=None, provider=_DummyProvider(), @@ -214,7 +249,7 @@ async def test_handle_401_dedup_survives_even_if_task_reference_dropped(tmp_path class _DummyProvider: context = None - mgr._entries["srv"] = _ProviderEntry( + mgr._entries[mgr._key("srv")] = _ProviderEntry( server_url="https://example.com/mcp", oauth_config=None, provider=_DummyProvider(), @@ -266,7 +301,7 @@ def test_manager_fails_fast_noninteractive_without_cached_tokens(tmp_path, monke with pytest.raises(OAuthNonInteractiveError, match="non-interactive"): mgr.get_or_build_provider("linear", "https://mcp.linear.app/mcp", None) - assert mgr._entries["linear"].provider is None + assert mgr._entries[mgr._key("linear")].provider is None # --------------------------------------------------------------------------- diff --git a/tools/mcp_dashboard_oauth.py b/tools/mcp_dashboard_oauth.py index da2fda39bc7ae..a02547c63be0b 100644 --- a/tools/mcp_dashboard_oauth.py +++ b/tools/mcp_dashboard_oauth.py @@ -23,6 +23,7 @@ class DashboardOAuthFlow: flow_id: str server_name: str profile: str | None + hermes_home: str redirect_uri: str created_at: float = field(default_factory=time.time) status: str = "starting" @@ -34,17 +35,19 @@ class DashboardOAuthFlow: _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) + _lock: threading.Lock = field(default_factory=threading.Lock, init=False, repr=False) async def publish_authorization_url(self, url: str) -> None: - if self.status in {"approved", "error"}: - raise RuntimeError("OAuth flow already ended") 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() + with self._lock: + if self.status in {"approved", "error"}: + raise RuntimeError("OAuth flow already ended") + self.expected_state = state + self.authorization_url = url + self.status = "authorization_required" + self._authorization_ready.set() async def wait_for_authorization_url(self, timeout: float = 30.0) -> str: ready = await asyncio.to_thread(self._authorization_ready.wait, timeout) @@ -61,21 +64,22 @@ def deliver_callback( state: str | None, error: str | None, ) -> 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: - self._callback = (code, state) - else: - self._callback_error = "OAuth callback did not include code or error" - self._callback_ready.set() + with self._lock: + 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: + self._callback = (code, state) + else: + self._callback_error = "OAuth callback did not include code or error" + self._callback_ready.set() async def wait_for_callback(self, timeout: float = 300.0) -> tuple[str, str | None]: ready = await asyncio.to_thread(self._callback_ready.wait, timeout) @@ -88,23 +92,30 @@ async def wait_for_callback(self, timeout: float = 300.0) -> tuple[str, str | No return self._callback def mark_approved(self) -> None: - self.status = "approved" - self.error = None + with self._lock: + if self.status == "error": + raise RuntimeError("OAuth flow already ended") + self.status = "approved" + self.error = None def mark_error(self, error: str) -> None: - self.status = "error" - self.error = error - self._authorization_ready.set() - self._callback_ready.set() + with self._lock: + if self.status == "approved": + return + self.status = "error" + self.error = error + self._authorization_ready.set() + self._callback_ready.set() def snapshot(self) -> dict: - return { - "flow_id": self.flow_id, - "server_name": self.server_name, - "status": self.status, - "authorization_url": self.authorization_url, - "error": self.error, - } + with self._lock: + return { + "flow_id": self.flow_id, + "server_name": self.server_name, + "status": self.status, + "authorization_url": self.authorization_url, + "error": self.error, + } _current_dashboard_flow: contextvars.ContextVar[DashboardOAuthFlow | None] = ( diff --git a/tools/mcp_oauth.py b/tools/mcp_oauth.py index e50fcbc9c2f53..b7f317fe63954 100644 --- a/tools/mcp_oauth.py +++ b/tools/mcp_oauth.py @@ -990,7 +990,7 @@ def _configure_callback_port( dashboard_flow = get_dashboard_oauth_flow() if dashboard_flow is not None: cfg["_resolved_port"] = 0 - cfg["redirect_uri"] = dashboard_flow.redirect_uri + cfg["redirect_uri"] = cfg.get("redirect_uri") or dashboard_flow.redirect_uri return 0 requested = int(cfg.get("redirect_port", 0)) # Precedence: explicit config port → cached client-registration port → diff --git a/tools/mcp_oauth_manager.py b/tools/mcp_oauth_manager.py index 087a1af91a98e..4fabba6b5d236 100644 --- a/tools/mcp_oauth_manager.py +++ b/tools/mcp_oauth_manager.py @@ -39,6 +39,7 @@ import re import threading from dataclasses import dataclass, field +from pathlib import Path from typing import Any, Optional logger = logging.getLogger(__name__) @@ -137,6 +138,7 @@ def __init__( ): super().__init__(*args, **kwargs) self._hermes_server_name = server_name + self._hermes_home = "" # When the client_id comes from config.yaml (pre-registered), an # invalid_client rejection means the *config* is wrong — deleting # client.json would just be re-seeded from config and re-running @@ -391,7 +393,8 @@ async def async_auth_flow(self, request): # type: ignore[override] # whatever state the SDK already has. try: await get_manager().invalidate_if_disk_changed( - self._hermes_server_name + self._hermes_server_name, + hermes_home=self._hermes_home, ) except Exception as exc: # pragma: no cover — defensive logger.debug( @@ -449,7 +452,7 @@ class MCPOAuthManager: """ def __init__(self) -> None: - self._entries: dict[str, _ProviderEntry] = {} + self._entries: dict[tuple[str, str], _ProviderEntry] = {} self._entries_lock = threading.Lock() # Holds strong references to in-flight 401 handler tasks so the # event loop's weak-reference bookkeeping cannot GC them mid-run @@ -472,8 +475,9 @@ def get_or_build_provider( Returns None if the MCP SDK's OAuth support is unavailable. """ + key = self._key(server_name) with self._entries_lock: - entry = self._entries.get(server_name) + entry = self._entries.get(key) if entry is not None and entry.server_url != server_url: logger.info( "MCP OAuth '%s': URL changed from %s to %s, discarding cache", @@ -486,13 +490,25 @@ def get_or_build_provider( server_url=server_url, oauth_config=oauth_config, ) - self._entries[server_name] = entry + self._entries[key] = entry if entry.provider is None: entry.provider = self._build_provider(server_name, entry) + if entry.provider is not None: + entry.provider._hermes_home = key[0] return entry.provider + @staticmethod + def _key( + server_name: str, + hermes_home: str | Path | None = None, + ) -> tuple[str, str]: + from hermes_constants import get_hermes_home + + home = Path(hermes_home) if hermes_home is not None else get_hermes_home() + return (str(home.expanduser().resolve(strict=False)), server_name) + def _build_provider( self, server_name: str, @@ -566,14 +582,19 @@ def _build_provider( timeout=float(cfg.get("timeout", 300)), ) - def remove(self, server_name: str) -> None: + def remove( + self, + server_name: str, + *, + hermes_home: str | Path | None = None, + ) -> None: """Evict the provider from cache AND delete tokens from disk. Called by ``hermes mcp remove `` and (indirectly) by ``hermes mcp login `` during forced re-auth. """ with self._entries_lock: - self._entries.pop(server_name, None) + self._entries.pop(self._key(server_name, hermes_home), None) from tools.mcp_oauth import remove_oauth_tokens remove_oauth_tokens(server_name) @@ -582,14 +603,24 @@ def remove(self, server_name: str) -> None: server_name, ) - def evict(self, server_name: str) -> None: + def evict( + self, + server_name: str, + *, + hermes_home: str | Path | None = None, + ) -> None: """Drop only the in-process provider, preserving persisted OAuth state.""" with self._entries_lock: - self._entries.pop(server_name, None) + self._entries.pop(self._key(server_name, hermes_home), None) # -- Disk watch ---------------------------------------------------------- - async def invalidate_if_disk_changed(self, server_name: str) -> bool: + async def invalidate_if_disk_changed( + self, + server_name: str, + *, + hermes_home: str | Path | None = None, + ) -> bool: """If the tokens file on disk has a newer mtime than last-seen, force the MCP SDK provider to reload its in-memory state. @@ -600,7 +631,7 @@ async def invalidate_if_disk_changed(self, server_name: str) -> bool: """ from tools.mcp_oauth import _get_token_dir, _safe_filename - entry = self._entries.get(server_name) + entry = self._entries.get(self._key(server_name, hermes_home)) if entry is None or entry.provider is None: return False @@ -647,7 +678,7 @@ async def handle_401( the same ``failed_access_token``, only one recovery attempt fires. Others await the same future. """ - entry = self._entries.get(server_name) + entry = self._entries.get(self._key(server_name)) if entry is None or entry.provider is None: return False diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py index b0c1b976bcfe5..dcc5bc9fba580 100644 --- a/tools/mcp_tool.py +++ b/tools/mcp_tool.py @@ -3203,6 +3203,15 @@ def _signal_reconnect(server: Any) -> bool: return True +def reconnect_mcp_server(server_name: str) -> bool: + """Ask a currently-live MCP server to rebuild after external re-auth.""" + with _lock: + server = _servers.get(server_name) + if server is None: + return False + return _signal_reconnect(server) + + def _wait_for_server_session_ready( srv: "MCPServerTask", *, diff --git a/web/src/lib/mcp-dashboard-oauth.test.ts b/web/src/lib/mcp-dashboard-oauth.test.ts index 7de079e0c0492..b023b6735fa29 100644 --- a/web/src/lib/mcp-dashboard-oauth.test.ts +++ b/web/src/lib/mcp-dashboard-oauth.test.ts @@ -81,4 +81,34 @@ describe("completeMcpDashboardOAuth", () => { ).rejects.toThrow("popup was blocked"); expect(start).not.toHaveBeenCalled(); }); + + it("fails when the authorization window closes before approval", async () => { + const authWindow = { location: { href: "" }, opener: {}, closed: false } as unknown as Window; + const status = vi.fn().mockImplementation(async () => { + Object.defineProperty(authWindow, "closed", { value: true }); + return { + flow_id: "flow-closed", + server_name: "reports", + status: "authorization_required", + authorization_url: "https://idp.example/authorize", + error: null, + }; + }); + + await expect( + completeMcpDashboardOAuth({ + serverName: "reports", + start: async () => ({ + flow_id: "flow-closed", + server_name: "reports", + status: "authorization_required", + authorization_url: "https://idp.example/authorize", + error: null, + }), + status, + open: vi.fn().mockReturnValue(authWindow), + sleep: async () => {}, + }), + ).rejects.toThrow("authorization window was closed"); + }); }); diff --git a/web/src/lib/mcp-dashboard-oauth.ts b/web/src/lib/mcp-dashboard-oauth.ts index 62bbfb8975ed7..fbf1cb8dc5425 100644 --- a/web/src/lib/mcp-dashboard-oauth.ts +++ b/web/src/lib/mcp-dashboard-oauth.ts @@ -46,6 +46,9 @@ export async function completeMcpDashboardOAuth({ if (current.status === "error") { throw new Error(current.error || "OAuth authorization failed"); } + if (authWindow.closed) { + throw new Error("OAuth authorization window was closed before completion"); + } await sleep(1000); } } From 1154291946225d259bc8afbe597b02d75a464761 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Fri, 17 Jul 2026 01:00:44 -0700 Subject: [PATCH 5/7] fix(mcp): close hosted OAuth lifecycle gaps --- .../src/lib/mcp-dashboard-oauth.test.ts | 24 ++++++++++ apps/desktop/src/lib/mcp-dashboard-oauth.ts | 23 +++++++++- hermes_cli/mcp_config.py | 13 ++++-- hermes_cli/web_server.py | 30 ++++++------ tests/hermes_cli/test_mcp_config.py | 19 ++++++++ tests/hermes_cli/test_mcp_dashboard_oauth.py | 24 ++++++++++ tests/tools/test_mcp_oauth.py | 24 ++++++++++ tests/tools/test_mcp_oauth_manager.py | 28 +++++++++++ tools/mcp_dashboard_oauth.py | 8 ++++ tools/mcp_oauth.py | 46 +++++++++++++++---- tools/mcp_oauth_manager.py | 4 +- web/src/lib/mcp-dashboard-oauth.test.ts | 32 +++++++++++++ web/src/lib/mcp-dashboard-oauth.ts | 14 +++++- 13 files changed, 253 insertions(+), 36 deletions(-) diff --git a/apps/desktop/src/lib/mcp-dashboard-oauth.test.ts b/apps/desktop/src/lib/mcp-dashboard-oauth.test.ts index 4473cd9e1b355..18ea5e93ac6fb 100644 --- a/apps/desktop/src/lib/mcp-dashboard-oauth.test.ts +++ b/apps/desktop/src/lib/mcp-dashboard-oauth.test.ts @@ -32,4 +32,28 @@ describe('completeMcpDesktopOAuth', () => { expect(openExternal).toHaveBeenCalledWith('https://idp.example/authorize') expect(result.status).toBe('approved') }) + + it('retries a transient status failure', async () => { + const status = vi + .fn() + .mockRejectedValueOnce(new Error('temporary network failure')) + .mockResolvedValueOnce({ + flow_id: 'flow-2', server_name: 'reports', status: 'approved', + authorization_url: 'https://idp.example/authorize', error: null, tools: [] + }) + + const result = await completeMcpDesktopOAuth({ + serverName: 'reports', + start: vi.fn().mockResolvedValue({ + flow_id: 'flow-2', server_name: 'reports', status: 'authorization_required', + authorization_url: 'https://idp.example/authorize', error: null + }), + status, + openExternal: vi.fn().mockResolvedValue(undefined), + sleep: async () => {} + }) + + expect(result.status).toBe('approved') + expect(status).toHaveBeenCalledTimes(2) + }) }) \ No newline at end of file diff --git a/apps/desktop/src/lib/mcp-dashboard-oauth.ts b/apps/desktop/src/lib/mcp-dashboard-oauth.ts index 5cd9570099df1..2015540e9b0c5 100644 --- a/apps/desktop/src/lib/mcp-dashboard-oauth.ts +++ b/apps/desktop/src/lib/mcp-dashboard-oauth.ts @@ -13,6 +13,7 @@ interface CompleteOptions { status: (flowId: string) => Promise openExternal: (url: string) => Promise sleep?: (milliseconds: number) => Promise + maxPollFailures?: number } const defaultSleep = (milliseconds: number) => @@ -23,7 +24,8 @@ export async function completeMcpDesktopOAuth({ start, status, openExternal, - sleep = defaultSleep + sleep = defaultSleep, + maxPollFailures = 3 }: CompleteOptions): Promise { const started = await start(serverName) @@ -37,8 +39,25 @@ export async function completeMcpDesktopOAuth({ await openExternal(started.authorization_url) + let pollFailures = 0 + for (;;) { - const current = await status(started.flow_id) + let current: McpOAuthFlow + + try { + current = await status(started.flow_id) + pollFailures = 0 + } catch (error) { + pollFailures += 1 + + if (pollFailures >= maxPollFailures) { + throw error + } + + await sleep(1000) + + continue + } if (current.status === 'approved') { return current diff --git a/hermes_cli/mcp_config.py b/hermes_cli/mcp_config.py index 76dcc7e7e9c11..53ab794bfff14 100644 --- a/hermes_cli/mcp_config.py +++ b/hermes_cli/mcp_config.py @@ -264,11 +264,14 @@ def _resolve_mcp_server_config(config: dict) -> dict: """ from tools.mcp_tool import _interpolate_env_vars - try: - from hermes_cli.env_loader import load_hermes_dotenv - load_hermes_dotenv() - except Exception: # pragma: no cover — defensive - pass + from agent.secret_scope import current_secret_scope + + if current_secret_scope() is None: + try: + from hermes_cli.env_loader import load_hermes_dotenv + load_hermes_dotenv() + except Exception: # pragma: no cover — defensive + pass return _interpolate_env_vars(config) diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index ddb7e79b2eee9..e84456ed2c262 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -11415,9 +11415,6 @@ def _run_dashboard_mcp_oauth(flow, cfg: dict) -> None: _save_mcp_server(flow.server_name, cfg) flow.tools = [{"name": t, "description": d} for t, d in tools] flow.mark_approved() - from tools.mcp_tool import reconnect_mcp_server - - reconnect_mcp_server(flow.server_name) except Exception: storage.restore(backup) raise @@ -11450,6 +11447,7 @@ def _run_dashboard_mcp_oauth(flow, cfg: dict) -> None: get_manager().evict(flow.server_name, hermes_home=flow.hermes_home) except Exception: pass + flow.mark_worker_done() @app.post("/api/mcp/servers/{name}/auth") @@ -11474,9 +11472,18 @@ async def auth_mcp_server(name: str, request: Request, profile: Optional[str] = raise HTTPException(status_code=400, detail="This server uses header/API-key auth, not OAuth") cfg["auth"] = "oauth" + flow_id = secrets.token_urlsafe(24) + flow = DashboardOAuthFlow( + flow_id=flow_id, + server_name=name, + profile=profile, + hermes_home=flow_home, + redirect_uri=(cfg.get("oauth") or {}).get("redirect_uri") + or _mcp_oauth_callback_url(request, name), + ) with _mcp_oauth_flows_lock: pending = sum( - flow.status in {"starting", "authorization_required"} + not flow.worker_done for flow in _mcp_oauth_flows.values() ) if pending >= _MAX_PENDING_MCP_OAUTH_FLOWS: @@ -11487,24 +11494,13 @@ async def auth_mcp_server(name: str, request: Request, profile: Optional[str] = if any( flow.server_name == name and flow.hermes_home == flow_home - and flow.status in {"starting", "authorization_required"} + and not flow.worker_done for flow in _mcp_oauth_flows.values() ): raise HTTPException( status_code=409, detail=f"MCP OAuth for '{name}' is already in progress", ) - - flow_id = secrets.token_urlsafe(24) - flow = DashboardOAuthFlow( - flow_id=flow_id, - server_name=name, - profile=profile, - hermes_home=flow_home, - redirect_uri=(cfg.get("oauth") or {}).get("redirect_uri") - or _mcp_oauth_callback_url(request, name), - ) - with _mcp_oauth_flows_lock: _mcp_oauth_flows[flow_id] = flow threading.Thread( target=_run_dashboard_mcp_oauth, @@ -11531,7 +11527,7 @@ async def mcp_oauth_flow_status(flow_id: str, request: Request): return snapshot -@app.get("/api/mcp/oauth/callback/{server_name}") +@app.get("/api/mcp/oauth/callback/{server_name:path}") async def mcp_oauth_callback( server_name: str, code: Optional[str] = None, diff --git a/tests/hermes_cli/test_mcp_config.py b/tests/hermes_cli/test_mcp_config.py index f79e29ff7f031..e91c8a2c08cad 100644 --- a/tests/hermes_cli/test_mcp_config.py +++ b/tests/hermes_cli/test_mcp_config.py @@ -6,6 +6,7 @@ """ import argparse +import os from pathlib import Path import pytest @@ -570,6 +571,24 @@ def test_resolve_interpolates_header(self, monkeypatch): }) assert resolved["headers"]["Authorization"] == "Bearer jwt-token-xyz" + def test_active_secret_scope_does_not_load_dotenv_into_process_env( + self, tmp_path, monkeypatch + ): + from agent.secret_scope import reset_secret_scope, set_secret_scope + from hermes_cli.mcp_config import _resolve_mcp_server_config + + monkeypatch.setenv("MCP_SHARED_API_KEY", "default-secret") + token = set_secret_scope({"MCP_SHARED_API_KEY": "profile-secret"}) + try: + resolved = _resolve_mcp_server_config({ + "headers": {"Authorization": "Bearer ${MCP_SHARED_API_KEY}"}, + }) + finally: + reset_secret_scope(token) + + assert resolved["headers"]["Authorization"] == "Bearer profile-secret" + assert os.environ["MCP_SHARED_API_KEY"] == "default-secret" + def test_resolve_leaves_unset_var_literal(self, monkeypatch): from hermes_cli.mcp_config import _resolve_mcp_server_config diff --git a/tests/hermes_cli/test_mcp_dashboard_oauth.py b/tests/hermes_cli/test_mcp_dashboard_oauth.py index 1862e4373d6bd..56cc9bfba6910 100644 --- a/tests/hermes_cli/test_mcp_dashboard_oauth.py +++ b/tests/hermes_cli/test_mcp_dashboard_oauth.py @@ -229,6 +229,30 @@ def test_callback_url_is_stable_for_a_server(): assert first == second == "https://agent.example/api/mcp/oauth/callback/reports" +def test_callback_route_supports_server_names_with_slashes(): + import asyncio + + from hermes_cli import web_server + from tools.mcp_dashboard_oauth import DashboardOAuthFlow + + flow = DashboardOAuthFlow( + flow_id="flow-slash", + server_name="github/mcp", + profile=None, + hermes_home="/tmp/hermes-test", + redirect_uri="https://agent.example/api/mcp/oauth/callback/github/mcp", + ) + asyncio.run(flow.publish_authorization_url("https://idp.example/authorize?state=slash")) + web_server._mcp_oauth_flows[flow.flow_id] = flow + + response = _client().get( + "/api/mcp/oauth/callback/github/mcp?code=abc&state=slash" + ) + + assert response.status_code == 200 + assert flow._callback == ("abc", "slash") + + 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_oauth.py b/tests/tools/test_mcp_oauth.py index faaa93d49a2b6..3ce8f11d7a23b 100644 --- a/tests/tools/test_mcp_oauth.py +++ b/tests/tools/test_mcp_oauth.py @@ -1114,6 +1114,30 @@ def test_configure_callback_port_reuses_cached_client_redirect_port(tmp_path, mo assert cfg["_resolved_port"] == 57727 +def test_configure_callback_reuses_cached_https_redirect_uri(tmp_path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + from tools.mcp_oauth import ( + HermesTokenStorage, + _build_client_metadata, + _configure_callback_port, + ) + + storage = HermesTokenStorage("hosted") + storage._client_info_path().parent.mkdir(parents=True) + storage._client_info_path().write_text(json.dumps({ + "client_id": "client-123", + "redirect_uris": ["https://agent.example/api/mcp/oauth/callback/hosted"], + })) + + cfg: dict = {} + _configure_callback_port(cfg, storage) + metadata = _build_client_metadata(cfg) + + assert str(metadata.redirect_uris[0]) == ( + "https://agent.example/api/mcp/oauth/callback/hosted" + ) + + def test_configure_callback_port_explicit_overrides_cached_client_port(tmp_path, monkeypatch): """Explicit config wins over any cached registration.""" from tools.mcp_oauth import _configure_callback_port diff --git a/tests/tools/test_mcp_oauth_manager.py b/tests/tools/test_mcp_oauth_manager.py index d680607aee5b2..6343ef8ff4232 100644 --- a/tests/tools/test_mcp_oauth_manager.py +++ b/tests/tools/test_mcp_oauth_manager.py @@ -46,6 +46,34 @@ def test_manager_isolates_same_named_servers_by_profile_home(tmp_path, monkeypat assert providers[0].context.current_tokens.access_token == "TOKEN_A" assert providers[1].context.current_tokens.access_token == "TOKEN_B" + +def test_manager_explicit_home_removes_only_that_profiles_tokens(tmp_path): + from hermes_constants import reset_hermes_home_override, set_hermes_home_override + from tools.mcp_oauth import HermesTokenStorage + from tools.mcp_oauth_manager import MCPOAuthManager + + profile_a = tmp_path / "profile-a" + profile_b = tmp_path / "profile-b" + paths = [] + for home in (profile_a, profile_b): + token = set_hermes_home_override(home) + try: + storage = HermesTokenStorage("shared") + storage._tokens_path().parent.mkdir(parents=True, exist_ok=True) + storage._tokens_path().write_text('{"access_token":"x","token_type":"Bearer"}') + paths.append(storage._tokens_path()) + finally: + reset_hermes_home_override(token) + + token = set_hermes_home_override(profile_a) + try: + MCPOAuthManager().remove("shared", hermes_home=profile_b) + finally: + reset_hermes_home_override(token) + + assert paths[0].exists() + assert not paths[1].exists() + pytest.importorskip( "mcp.client.auth.oauth2", reason="MCP SDK 1.26.0+ required for OAuth support", diff --git a/tools/mcp_dashboard_oauth.py b/tools/mcp_dashboard_oauth.py index a02547c63be0b..df8dd73f1b538 100644 --- a/tools/mcp_dashboard_oauth.py +++ b/tools/mcp_dashboard_oauth.py @@ -35,6 +35,7 @@ class DashboardOAuthFlow: _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) + _worker_done: threading.Event = field(default_factory=threading.Event, init=False, repr=False) _lock: threading.Lock = field(default_factory=threading.Lock, init=False, repr=False) async def publish_authorization_url(self, url: str) -> None: @@ -117,6 +118,13 @@ def snapshot(self) -> dict: "error": self.error, } + def mark_worker_done(self) -> None: + self._worker_done.set() + + @property + def worker_done(self) -> bool: + return self._worker_done.is_set() + _current_dashboard_flow: contextvars.ContextVar[DashboardOAuthFlow | None] = ( contextvars.ContextVar("mcp_dashboard_oauth_flow", default=None) diff --git a/tools/mcp_oauth.py b/tools/mcp_oauth.py index b7f317fe63954..ef85ed2653183 100644 --- a/tools/mcp_oauth.py +++ b/tools/mcp_oauth.py @@ -131,7 +131,7 @@ class OAuthNonInteractiveError(RuntimeError): # --------------------------------------------------------------------------- -def _get_token_dir() -> Path: +def _get_token_dir(hermes_home: str | Path | None = None) -> Path: """Return the directory for MCP OAuth token files. Uses HERMES_HOME so each profile gets its own OAuth tokens. @@ -139,7 +139,7 @@ def _get_token_dir() -> Path: """ try: from hermes_constants import get_hermes_home - base = Path(get_hermes_home()) + base = Path(hermes_home) if hermes_home is not None else Path(get_hermes_home()) except ImportError: base = Path(os.environ.get("HERMES_HOME", str(Path.home() / ".hermes"))) return base / "mcp-tokens" @@ -229,6 +229,24 @@ def _cached_redirect_port(storage: "HermesTokenStorage | None") -> int | None: return None +def _cached_redirect_uri(storage: "HermesTokenStorage | None") -> str | None: + """Return a cached non-loopback redirect URI, if one was registered.""" + if storage is None: + return None + try: + data = _read_json(storage._client_info_path()) + except (AttributeError, TypeError, ValueError): + return None + for uri in (data or {}).get("redirect_uris") or []: + try: + parsed = urlparse(str(uri)) + except (TypeError, ValueError): + continue + if parsed.scheme == "https" and parsed.netloc: + return str(uri) + return None + + def _is_interactive() -> bool: """Return True if we can reasonably expect to interact with a user.""" if not _oauth_interactive_enabled.get(): @@ -370,17 +388,18 @@ class HermesTokenStorage: HERMES_HOME/mcp-tokens/.meta.json -- oauth server metadata """ - def __init__(self, server_name: str): + def __init__(self, server_name: str, *, hermes_home: str | Path | None = None): self._server_name = _safe_filename(server_name) + self._hermes_home = Path(hermes_home) if hermes_home is not None else None def _tokens_path(self) -> Path: - return _get_token_dir() / f"{self._server_name}.json" + return _get_token_dir(self._hermes_home) / f"{self._server_name}.json" def _client_info_path(self) -> Path: - return _get_token_dir() / f"{self._server_name}.client.json" + return _get_token_dir(self._hermes_home) / f"{self._server_name}.client.json" def _meta_path(self) -> Path: - return _get_token_dir() / f"{self._server_name}.meta.json" + return _get_token_dir(self._hermes_home) / f"{self._server_name}.meta.json" # -- tokens ------------------------------------------------------------ @@ -508,7 +527,7 @@ def restore(self, snapshot: dict[str, bytes]) -> None: self.remove() if not snapshot: return - token_dir = _get_token_dir() + token_dir = _get_token_dir(self._hermes_home) token_dir.mkdir(parents=True, exist_ok=True) for fname, data in snapshot.items(): path = token_dir / fname @@ -947,9 +966,13 @@ def _paste_callback_reader(result: dict) -> None: # --------------------------------------------------------------------------- -def remove_oauth_tokens(server_name: str) -> None: +def remove_oauth_tokens( + server_name: str, + *, + hermes_home: str | Path | None = None, +) -> None: """Delete stored OAuth tokens and client info for a server.""" - storage = HermesTokenStorage(server_name) + storage = HermesTokenStorage(server_name, hermes_home=hermes_home) storage.remove() logger.info("OAuth tokens removed for '%s'", server_name) @@ -992,6 +1015,11 @@ def _configure_callback_port( cfg["_resolved_port"] = 0 cfg["redirect_uri"] = cfg.get("redirect_uri") or dashboard_flow.redirect_uri return 0 + cached_redirect_uri = _cached_redirect_uri(storage) + if not cfg.get("redirect_uri") and cached_redirect_uri: + cfg["redirect_uri"] = cached_redirect_uri + cfg["_resolved_port"] = 0 + return 0 requested = int(cfg.get("redirect_port", 0)) # Precedence: explicit config port → cached client-registration port → # fresh ephemeral port. The cached port keeps re-auth consistent with the diff --git a/tools/mcp_oauth_manager.py b/tools/mcp_oauth_manager.py index 4fabba6b5d236..d5f86a203cdcd 100644 --- a/tools/mcp_oauth_manager.py +++ b/tools/mcp_oauth_manager.py @@ -597,7 +597,7 @@ def remove( self._entries.pop(self._key(server_name, hermes_home), None) from tools.mcp_oauth import remove_oauth_tokens - remove_oauth_tokens(server_name) + remove_oauth_tokens(server_name, hermes_home=hermes_home) logger.info( "MCP OAuth '%s': evicted from cache and removed from disk", server_name, @@ -636,7 +636,7 @@ async def invalidate_if_disk_changed( return False async with entry.lock: - tokens_path = _get_token_dir() / f"{_safe_filename(server_name)}.json" + tokens_path = _get_token_dir(hermes_home) / f"{_safe_filename(server_name)}.json" try: mtime_ns = tokens_path.stat().st_mtime_ns except (FileNotFoundError, OSError): diff --git a/web/src/lib/mcp-dashboard-oauth.test.ts b/web/src/lib/mcp-dashboard-oauth.test.ts index b023b6735fa29..0afaec543c00a 100644 --- a/web/src/lib/mcp-dashboard-oauth.test.ts +++ b/web/src/lib/mcp-dashboard-oauth.test.ts @@ -111,4 +111,36 @@ describe("completeMcpDashboardOAuth", () => { }), ).rejects.toThrow("authorization window was closed"); }); + + it("retries a transient status failure", async () => { + const authWindow = { location: { href: "" }, opener: {}, closed: false } as unknown as Window; + const status = vi + .fn() + .mockRejectedValueOnce(new Error("temporary network failure")) + .mockResolvedValueOnce({ + flow_id: "flow-retry", + server_name: "reports", + status: "approved", + authorization_url: "https://idp.example/authorize", + error: null, + tools: [], + }); + + const result = await completeMcpDashboardOAuth({ + serverName: "reports", + start: async () => ({ + flow_id: "flow-retry", + server_name: "reports", + status: "authorization_required", + authorization_url: "https://idp.example/authorize", + error: null, + }), + status, + open: vi.fn().mockReturnValue(authWindow), + sleep: async () => {}, + }); + + expect(result.status).toBe("approved"); + expect(status).toHaveBeenCalledTimes(2); + }); }); diff --git a/web/src/lib/mcp-dashboard-oauth.ts b/web/src/lib/mcp-dashboard-oauth.ts index fbf1cb8dc5425..a039f9056e4e0 100644 --- a/web/src/lib/mcp-dashboard-oauth.ts +++ b/web/src/lib/mcp-dashboard-oauth.ts @@ -6,6 +6,7 @@ type CompleteOptions = { status: (flowId: string) => Promise; open: (url?: string | URL, target?: string, features?: string) => unknown; sleep?: (milliseconds: number) => Promise; + maxPollFailures?: number; }; const defaultSleep = (milliseconds: number) => @@ -17,6 +18,7 @@ export async function completeMcpDashboardOAuth({ status, open, sleep = defaultSleep, + maxPollFailures = 3, }: CompleteOptions): Promise { // Open synchronously from the click handler, before the first await. Browsers // otherwise classify the later OAuth popup as unsolicited and block it. @@ -40,8 +42,18 @@ export async function completeMcpDashboardOAuth({ throw error; } + let pollFailures = 0; for (;;) { - const current = await status(started.flow_id); + let current: McpOAuthFlow; + try { + current = await status(started.flow_id); + pollFailures = 0; + } catch (error) { + pollFailures += 1; + if (pollFailures >= maxPollFailures) throw error; + await sleep(1000); + continue; + } if (current.status === "approved") return current; if (current.status === "error") { throw new Error(current.error || "OAuth authorization failed"); From 780d6325e7a03878be15b8f3938e7b511bbd6fd1 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Fri, 17 Jul 2026 01:33:59 -0700 Subject: [PATCH 6/7] fix(mcp): preserve live OAuth state during reauth --- hermes_cli/web_server.py | 34 +++++++++++-------- tests/tools/test_mcp_dashboard_oauth.py | 43 +++++++++++++++++++++++++ tests/tools/test_mcp_oauth_manager.py | 14 ++++++++ tools/mcp_dashboard_oauth.py | 1 + tools/mcp_oauth.py | 13 ++++++-- tools/mcp_oauth_manager.py | 18 +++++++++-- 6 files changed, 106 insertions(+), 17 deletions(-) diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index e84456ed2c262..e2dce9451df98 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -11398,10 +11398,15 @@ def _run_dashboard_mcp_oauth(flow, cfg: dict) -> None: try: transaction = _mcp_oauth_transaction(flow) with transaction, force_interactive_oauth(), dashboard_oauth_flow(flow): + manager = get_manager() storage = HermesTokenStorage(flow.server_name) backup = storage.snapshot() + previous_entry = None try: - get_manager().remove(flow.server_name, hermes_home=flow.hermes_home) + previous_entry = manager.remove( + flow.server_name, + hermes_home=flow.hermes_home, + ) tools = _probe_single_server( flow.server_name, cfg, @@ -11415,8 +11420,18 @@ def _run_dashboard_mcp_oauth(flow, cfg: dict) -> None: _save_mcp_server(flow.server_name, cfg) flow.tools = [{"name": t, "description": d} for t, d in tools] flow.mark_approved() + if flow.reconnect_live: + from tools.mcp_tool import reconnect_mcp_server + + reconnect_mcp_server(flow.server_name) except Exception: - storage.restore(backup) + manager.evict(flow.server_name, hermes_home=flow.hermes_home) + storage.restore(backup, only_if_absent=True) + manager.restore_entry( + flow.server_name, + previous_entry, + hermes_home=flow.hermes_home, + ) raise finally: reset_secret_scope(secret_token) @@ -11438,15 +11453,6 @@ def _run_dashboard_mcp_oauth(flow, cfg: dict) -> None: ) flow.mark_error(msg) finally: - # Dashboard auth builds a provider with a public callback URI and bridge - # handlers. Evict that one-shot provider after completion; persisted - # tokens/client registration remain for the normal runtime rebuild. - try: - from tools.mcp_oauth_manager import get_manager - - get_manager().evict(flow.server_name, hermes_home=flow.hermes_home) - except Exception: - pass flow.mark_worker_done() @@ -11458,10 +11464,11 @@ async def auth_mcp_server(name: str, request: Request, profile: Optional[str] = _require_token(request) _gc_mcp_oauth_flows() + from hermes_constants import get_hermes_home + + process_home = str(get_hermes_home().expanduser().resolve(strict=False)) with _profile_scope(profile): servers = _get_mcp_servers() - from hermes_constants import get_hermes_home - flow_home = str(get_hermes_home().expanduser().resolve(strict=False)) if name not in servers: raise HTTPException(status_code=404, detail=f"Server '{name}' not found") @@ -11480,6 +11487,7 @@ async def auth_mcp_server(name: str, request: Request, profile: Optional[str] = hermes_home=flow_home, redirect_uri=(cfg.get("oauth") or {}).get("redirect_uri") or _mcp_oauth_callback_url(request, name), + reconnect_live=flow_home == process_home, ) with _mcp_oauth_flows_lock: pending = sum( diff --git a/tests/tools/test_mcp_dashboard_oauth.py b/tests/tools/test_mcp_dashboard_oauth.py index 586eb13ebe9bd..a4c5ea6522b98 100644 --- a/tests/tools/test_mcp_dashboard_oauth.py +++ b/tests/tools/test_mcp_dashboard_oauth.py @@ -246,3 +246,46 @@ class Server: assert mcp_tool.reconnect_mcp_server("reports") is True assert server._reconnect_event.called is True + + +def test_reconnect_mcp_server_keeps_manager_entry_until_live_task_rebuilds( + tmp_path, monkeypatch +): + from tools import mcp_tool + from tools.mcp_oauth_manager import MCPOAuthManager, _ProviderEntry + + class Event: + called = False + + def set(self): + self.called = True + + class Server: + _reconnect_event = Event() + + server = Server() + manager = MCPOAuthManager() + manager._entries[manager._key("reports", tmp_path)] = _ProviderEntry( + server_url="https://mcp.example/mcp", oauth_config={} + ) + monkeypatch.setitem(mcp_tool._servers, "reports", server) + monkeypatch.setattr(mcp_tool, "_mcp_loop", None) + + assert mcp_tool.reconnect_mcp_server("reports") is True + assert manager._key("reports", tmp_path) in manager._entries + + +def test_failed_reauth_rollback_preserves_newer_oauth_state(tmp_path, monkeypatch): + from tools.mcp_oauth import HermesTokenStorage + + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + storage = HermesTokenStorage("reports") + storage._tokens_path().parent.mkdir(parents=True) + storage._tokens_path().write_text("OLD") + backup = storage.snapshot() + storage.remove() + + storage._tokens_path().write_text("FRESH") + storage.restore(backup, only_if_absent=True) + + assert storage._tokens_path().read_text() == "FRESH" diff --git a/tests/tools/test_mcp_oauth_manager.py b/tests/tools/test_mcp_oauth_manager.py index 6343ef8ff4232..3c47239f6f2de 100644 --- a/tests/tools/test_mcp_oauth_manager.py +++ b/tests/tools/test_mcp_oauth_manager.py @@ -74,6 +74,20 @@ def test_manager_explicit_home_removes_only_that_profiles_tokens(tmp_path): assert paths[0].exists() assert not paths[1].exists() + +def test_manager_can_restore_removed_entry_after_failed_reauth(tmp_path, monkeypatch): + from tools.mcp_oauth_manager import MCPOAuthManager + + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + _set_interactive_stdin(monkeypatch) + manager = MCPOAuthManager() + provider = manager.get_or_build_provider("shared", "https://mcp.example", {}) + + entry = manager.remove("shared") + manager.restore_entry("shared", entry) + + assert manager.get_or_build_provider("shared", "https://mcp.example", {}) is provider + pytest.importorskip( "mcp.client.auth.oauth2", reason="MCP SDK 1.26.0+ required for OAuth support", diff --git a/tools/mcp_dashboard_oauth.py b/tools/mcp_dashboard_oauth.py index df8dd73f1b538..31663e0f37b0d 100644 --- a/tools/mcp_dashboard_oauth.py +++ b/tools/mcp_dashboard_oauth.py @@ -25,6 +25,7 @@ class DashboardOAuthFlow: profile: str | None hermes_home: str redirect_uri: str + reconnect_live: bool = False created_at: float = field(default_factory=time.time) status: str = "starting" authorization_url: str | None = None diff --git a/tools/mcp_oauth.py b/tools/mcp_oauth.py index ef85ed2653183..a42a11a7e8d5d 100644 --- a/tools/mcp_oauth.py +++ b/tools/mcp_oauth.py @@ -522,8 +522,17 @@ def snapshot(self) -> dict[str, bytes]: pass return snap - def restore(self, snapshot: dict[str, bytes]) -> None: - """Revert to a ``snapshot()`` capture (dropping any newer partial state).""" + def restore(self, snapshot: dict[str, bytes], *, only_if_absent: bool = False) -> None: + """Revert to a snapshot without overwriting a concurrent successful write.""" + if only_if_absent and any( + path.exists() + for path in (self._tokens_path(), self._client_info_path(), self._meta_path()) + ): + logger.info( + "Skipping OAuth rollback for %s because newer state exists", + self._server_name, + ) + return self.remove() if not snapshot: return diff --git a/tools/mcp_oauth_manager.py b/tools/mcp_oauth_manager.py index d5f86a203cdcd..9d2a625c26326 100644 --- a/tools/mcp_oauth_manager.py +++ b/tools/mcp_oauth_manager.py @@ -587,14 +587,14 @@ def remove( server_name: str, *, hermes_home: str | Path | None = None, - ) -> None: + ) -> _ProviderEntry | None: """Evict the provider from cache AND delete tokens from disk. Called by ``hermes mcp remove `` and (indirectly) by ``hermes mcp login `` during forced re-auth. """ with self._entries_lock: - self._entries.pop(self._key(server_name, hermes_home), None) + entry = self._entries.pop(self._key(server_name, hermes_home), None) from tools.mcp_oauth import remove_oauth_tokens remove_oauth_tokens(server_name, hermes_home=hermes_home) @@ -602,6 +602,20 @@ def remove( "MCP OAuth '%s': evicted from cache and removed from disk", server_name, ) + return entry + + def restore_entry( + self, + server_name: str, + entry: _ProviderEntry | None, + *, + hermes_home: str | Path | None = None, + ) -> None: + """Restore a provider entry removed for a failed reauthorization.""" + if entry is None: + return + with self._entries_lock: + self._entries.setdefault(self._key(server_name, hermes_home), entry) def evict( self, From 6aba883c149c073441611f5cb5c04a179b98a4bf Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Fri, 17 Jul 2026 01:46:59 -0700 Subject: [PATCH 7/7] fix(mcp): preserve concurrent OAuth manager refresh --- hermes_cli/web_server.py | 1 - tests/tools/test_mcp_oauth_manager.py | 16 ++++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index e2dce9451df98..8dee6cce9ec44 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -11425,7 +11425,6 @@ def _run_dashboard_mcp_oauth(flow, cfg: dict) -> None: reconnect_mcp_server(flow.server_name) except Exception: - manager.evict(flow.server_name, hermes_home=flow.hermes_home) storage.restore(backup, only_if_absent=True) manager.restore_entry( flow.server_name, diff --git a/tests/tools/test_mcp_oauth_manager.py b/tests/tools/test_mcp_oauth_manager.py index 3c47239f6f2de..cfefbadeb2147 100644 --- a/tests/tools/test_mcp_oauth_manager.py +++ b/tests/tools/test_mcp_oauth_manager.py @@ -88,6 +88,22 @@ def test_manager_can_restore_removed_entry_after_failed_reauth(tmp_path, monkeyp assert manager.get_or_build_provider("shared", "https://mcp.example", {}) is provider + +def test_manager_restore_entry_preserves_newer_concurrent_entry(tmp_path, monkeypatch): + from tools.mcp_oauth_manager import MCPOAuthManager + + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + _set_interactive_stdin(monkeypatch) + manager = MCPOAuthManager() + old_provider = manager.get_or_build_provider("shared", "https://old.example", {}) + old_entry = manager.remove("shared") + new_provider = manager.get_or_build_provider("shared", "https://new.example", {}) + + manager.restore_entry("shared", old_entry) + + assert manager.get_or_build_provider("shared", "https://new.example", {}) is new_provider + assert new_provider is not old_provider + pytest.importorskip( "mcp.client.auth.oauth2", reason="MCP SDK 1.26.0+ required for OAuth support",