diff --git a/apps/desktop/src/lib/mcp-dashboard-oauth.test.ts b/apps/desktop/src/lib/mcp-dashboard-oauth.test.ts index 4473cd9e1b3..18ea5e93ac6 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 5cd9570099d..2015540e9b0 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 76dcc7e7e9c..53ab794bfff 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 ddb7e79b2ee..e84456ed2c2 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,27 +11472,6 @@ 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, @@ -11505,6 +11482,25 @@ async def auth_mcp_server(name: str, request: Request, profile: Optional[str] = or _mcp_oauth_callback_url(request, name), ) with _mcp_oauth_flows_lock: + pending = sum( + not flow.worker_done + 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 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", + ) _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 f79e29ff7f0..e91c8a2c08c 100644 --- a/tests/hermes_cli/test_mcp_config.py +++ b/tests/hermes_cli/test_mcp_config.py @@ -6,6 +6,7 @@ any actual MCP servers or API keys. """ import argparse +import os from pathlib import Path import pytest @@ -570,6 +571,24 @@ class TestProbeEnvResolution: }) 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 1862e4373d6..56cc9bfba69 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 faaa93d49a2..3ce8f11d7a2 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 d680607aee5..6343ef8ff42 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 a02547c63be..df8dd73f1b5 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 @@ class DashboardOAuthFlow: "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 b7f317fe639..ef85ed26531 100644 --- a/tools/mcp_oauth.py +++ b/tools/mcp_oauth.py @@ -131,7 +131,7 @@ _USER_SKIPPED_SENTINEL = "__hermes_user_skipped__" # --------------------------------------------------------------------------- -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 @@ class HermesTokenStorage: 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 4fabba6b5d2..d5f86a203cd 100644 --- a/tools/mcp_oauth_manager.py +++ b/tools/mcp_oauth_manager.py @@ -597,7 +597,7 @@ class MCPOAuthManager: 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 @@ class MCPOAuthManager: 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 b023b6735fa..0afaec543c0 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 fbf1cb8dc54..a039f9056e4 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");