fix(mcp): close hosted OAuth lifecycle gaps

This commit is contained in:
Teknium 2026-07-17 01:00:44 -07:00
parent 6045529724
commit ebd737f4d9
13 changed files with 261 additions and 44 deletions

View file

@ -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)
})
})

View file

@ -13,6 +13,7 @@ interface CompleteOptions {
status: (flowId: string) => Promise<McpOAuthFlow>
openExternal: (url: string) => Promise<void>
sleep?: (milliseconds: number) => Promise<void>
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<McpOAuthFlow> {
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

View file

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

View file

@ -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,

View file

@ -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

View file

@ -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

View file

@ -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

View file

@ -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",

View file

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

View file

@ -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/<server_name>.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

View file

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

View file

@ -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);
});
});

View file

@ -6,6 +6,7 @@ type CompleteOptions = {
status: (flowId: string) => Promise<McpOAuthFlow>;
open: (url?: string | URL, target?: string, features?: string) => unknown;
sleep?: (milliseconds: number) => Promise<void>;
maxPollFailures?: number;
};
const defaultSleep = (milliseconds: number) =>
@ -17,6 +18,7 @@ export async function completeMcpDashboardOAuth({
status,
open,
sleep = defaultSleep,
maxPollFailures = 3,
}: CompleteOptions): Promise<McpOAuthFlow> {
// 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");