fix(mcp): OAuth login waits for oauth.timeout and a probe timeout names itself
`hermes mcp login`, the dashboard re-auth (web_server_mcp.py) and the Desktop
re-auth (tui_gateway/mcp_oauth_sessions.py) each bounded the login probe at
`max(connect_timeout, 315)`. The 315 was the default 300 s callback window
plus headroom, frozen: a user who set `oauth: {timeout: 3600}` still had the
probe cancelled at 315 s. That expiry was a bare `asyncio.TimeoutError`, whose
`str()` is '', so the CLI printed a blank `✗ Authentication failed:` line
(#116278, reporter's steps 5).
`tools/mcp_oauth.py::login_connect_timeout(config)` computes the bound once —
`max(connect_timeout, oauth.timeout + 15)` — and all three call sites use it.
`_probe_single_server` re-raises its wait_for expiry as a TimeoutError naming
the server, the elapsed bound and both governing knobs, so every caller's
`humanized or exc` renders a reason.
Slimmer redo of the blank-line half of #114527 (@liuhao1024): that PR races
the connect against `asyncio.wait` so an inner exception can win; with the
window now sized from oauth.timeout the callback waiter's own
`OAuth callback timed out` message wins on its own, and the described
TimeoutError covers the remaining case in one place.
Completes the timeout atom of #116278 (closed by #116658); refs #103633
Co-authored-by: liuhao1024 <sunsky.lau@gmail.com>
This commit is contained in:
@@ -446,7 +446,14 @@ def _probe_single_server(
|
||||
tools_found: List[Tuple[str, str]] = []
|
||||
|
||||
async def _probe():
|
||||
server = await asyncio.wait_for(_connect_server(name, config), timeout=connect_timeout)
|
||||
try:
|
||||
server = await asyncio.wait_for(_connect_server(name, config), timeout=connect_timeout)
|
||||
except asyncio.TimeoutError:
|
||||
# str(TimeoutError()) is '' — printed verbatim it was a blank "Authentication failed:".
|
||||
raise TimeoutError(
|
||||
f"Connecting to MCP server '{name}' timed out after {float(connect_timeout):.0f}s "
|
||||
"(bounded by connect_timeout; an OAuth login also by oauth.timeout)"
|
||||
) from None
|
||||
try:
|
||||
for t in server._tools:
|
||||
desc = getattr(t, "description", "") or ""
|
||||
@@ -848,25 +855,22 @@ def _reauth_oauth_server(name: str, server_config: dict, *, flow: str | None = N
|
||||
print()
|
||||
_info(f"Starting OAuth flow for '{name}'...")
|
||||
|
||||
# The probe triggers the OAuth flow (browser redirect + callback capture). Honor the configured
|
||||
# connect_timeout, floored at 315s (the 300s OAuth callback window + headroom) — matching the GUI
|
||||
# re-auth path in web_server.py. force_interactive_oauth: `hermes mcp login` is explicitly
|
||||
# user-initiated even when stdin isn't a TTY (desktop / agent-spawned terminals), where
|
||||
# _is_interactive() alone would refuse to open a browser.
|
||||
# The probe triggers the OAuth flow (browser redirect + callback capture). Its bound must outlast
|
||||
# the oauth.timeout callback window (plus headroom for the token exchange), or a user who raised
|
||||
# oauth.timeout still gets cut off at the old fixed floor — matching the GUI re-auth path in
|
||||
# web_server_mcp.py and tui_gateway/mcp_oauth_sessions.py. force_interactive_oauth: `hermes mcp
|
||||
# login` is explicitly user-initiated even when stdin isn't a TTY (desktop / agent-spawned
|
||||
# terminals), where _is_interactive() alone would refuse to open a browser.
|
||||
try:
|
||||
from tools.mcp_oauth import force_interactive_oauth
|
||||
from tools.mcp_oauth import force_interactive_oauth, login_connect_timeout
|
||||
|
||||
try:
|
||||
_login_connect_timeout = float(server_config.get("connect_timeout"))
|
||||
except (TypeError, ValueError):
|
||||
_login_connect_timeout = 0.0
|
||||
if selected_flow == "device":
|
||||
from tools.mcp_oauth_device import login_device
|
||||
asyncio.run(login_device(name, url, oauth_cfg))
|
||||
probe_config = {**server_config, "oauth": {**oauth_cfg, "flow": selected_flow}}
|
||||
with force_interactive_oauth():
|
||||
tools = _probe_single_server(
|
||||
name, probe_config, connect_timeout=max(_login_connect_timeout, 315.0)
|
||||
name, probe_config, connect_timeout=login_connect_timeout(probe_config)
|
||||
)
|
||||
# A clean probe is NOT proof of authentication: some servers (e.g. Google Drive) serve
|
||||
# initialize + tools/list without auth, so the flow may have failed (e.g. DCR 400 for
|
||||
|
||||
@@ -119,7 +119,7 @@ def _run_dashboard_mcp_oauth(flow, cfg: dict) -> None:
|
||||
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 import HermesTokenStorage, force_interactive_oauth, login_connect_timeout
|
||||
from tools.mcp_oauth_manager import get_manager
|
||||
|
||||
home_token = set_hermes_home_override(flow.hermes_home)
|
||||
@@ -134,9 +134,7 @@ def _run_dashboard_mcp_oauth(flow, cfg: dict) -> None:
|
||||
try:
|
||||
previous_entry = 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),
|
||||
flow.server_name, cfg, connect_timeout=login_connect_timeout(cfg)
|
||||
)
|
||||
if not _oauth_tokens_present(flow.server_name):
|
||||
raise RuntimeError(
|
||||
|
||||
50
tests/hermes_cli/test_mcp_login_window.py
Normal file
50
tests/hermes_cli/test_mcp_login_window.py
Normal file
@@ -0,0 +1,50 @@
|
||||
"""An OAuth login waits as long as ``oauth.timeout`` says, and a probe timeout names itself (#116278).
|
||||
|
||||
``hermes mcp login`` (and the dashboard / Desktop re-auth twins) bounded the login probe at a fixed
|
||||
315 s floor, so ``oauth: {timeout: 3600}`` changed nothing; the expiry then surfaced as a bare
|
||||
``asyncio.TimeoutError`` whose ``str()`` is empty — a blank ``✗ Authentication failed:`` line.
|
||||
"""
|
||||
|
||||
import asyncio
|
||||
import contextlib
|
||||
import io
|
||||
|
||||
import pytest
|
||||
|
||||
|
||||
def test_login_probe_window_follows_oauth_timeout(monkeypatch, tmp_path):
|
||||
"""Production entry point ``_reauth_oauth_server``: with ``oauth.timeout: 3600`` the probe's
|
||||
connect bound is the callback window plus exchange headroom, not the old 315 s floor."""
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
|
||||
import hermes_cli.mcp_config as mc
|
||||
|
||||
seen = {}
|
||||
|
||||
def _recording_probe(name, config, connect_timeout=None, **kw):
|
||||
seen["connect_timeout"] = connect_timeout
|
||||
raise RuntimeError("probe body replaced by recorder")
|
||||
|
||||
monkeypatch.setattr(mc, "_probe_single_server", _recording_probe)
|
||||
with contextlib.redirect_stdout(io.StringIO()):
|
||||
mc._reauth_oauth_server(
|
||||
"traveler",
|
||||
{"url": "https://mcp.example.test/mcp", "auth": "oauth",
|
||||
"oauth": {"timeout": 3600, "redirect_port": 27891}},
|
||||
)
|
||||
assert seen["connect_timeout"] == 3615.0
|
||||
|
||||
|
||||
def test_probe_timeout_names_server_and_knobs(monkeypatch):
|
||||
"""A probe that outlives its bound raises a TimeoutError whose message names the server and the
|
||||
governing settings — never the empty ``str(asyncio.TimeoutError())``."""
|
||||
import hermes_cli.mcp_config as mc
|
||||
|
||||
async def _hang(name, config):
|
||||
await asyncio.sleep(3600)
|
||||
|
||||
monkeypatch.setattr("tools.mcp_tool_discovery._connect_server", _hang)
|
||||
with pytest.raises(TimeoutError) as info:
|
||||
mc._probe_single_server("hangsrv", {"url": "https://mcp.example.test/mcp"}, connect_timeout=0.2)
|
||||
message = str(info.value)
|
||||
assert "hangsrv" in message and "timed out" in message
|
||||
assert "connect_timeout" in message and "oauth.timeout" in message
|
||||
@@ -1019,6 +1019,21 @@ def token_request_user_agent(cfg: dict) -> str | None:
|
||||
return ua.strip() if isinstance(ua, str) and ua.strip() else None
|
||||
|
||||
|
||||
def login_connect_timeout(config: dict) -> float:
|
||||
"""Connect bound for an interactive OAuth login probe (CLI ``hermes mcp login``, dashboard and
|
||||
Desktop re-auth): the server's ``connect_timeout`` or its ``oauth.timeout`` callback window
|
||||
(default 300 s) plus 15 s headroom for the token exchange, whichever is longer. A fixed 315 s
|
||||
floor here made raising ``oauth.timeout`` alone a no-op — the probe timed out first (#116278)."""
|
||||
def _seconds(value, default: float) -> float:
|
||||
try:
|
||||
return max(0.0, float(value))
|
||||
except (TypeError, ValueError):
|
||||
return default
|
||||
oauth_cfg = config.get("oauth") or {}
|
||||
return max(_seconds(config.get("connect_timeout"), 0.0),
|
||||
_seconds(oauth_cfg.get("timeout"), 300.0) + 15.0)
|
||||
|
||||
|
||||
def _configure_callback_port(cfg: dict, storage: "HermesTokenStorage | None" = None) -> int:
|
||||
"""Resolve the callback port into ``cfg['_resolved_port']`` (0 = non-loopback URI). Precedence:
|
||||
dashboard flow / cached https redirect URI → CIMD pinned port (sets ``cfg['_cimd_url']``) →
|
||||
|
||||
@@ -85,7 +85,7 @@ def _probe_with_rollback(
|
||||
server_name: str, cfg: dict, hermes_home: str, flow, reconnect_live: bool) -> None:
|
||||
"""Run the OAuth probe; on ANY failure restore the prior token file + manager entry."""
|
||||
from hermes_cli.mcp_config import _oauth_tokens_present, _probe_single_server, _save_mcp_server
|
||||
from tools.mcp_oauth import HermesTokenStorage
|
||||
from tools.mcp_oauth import HermesTokenStorage, login_connect_timeout
|
||||
from tools.mcp_oauth_manager import get_manager
|
||||
manager = get_manager()
|
||||
storage = HermesTokenStorage(server_name)
|
||||
@@ -93,8 +93,7 @@ def _probe_with_rollback(
|
||||
previous_entry = None
|
||||
try:
|
||||
previous_entry = manager.remove(server_name, hermes_home=hermes_home)
|
||||
timeout = max(float(cfg.get("connect_timeout", 0) or 0), 315)
|
||||
tools = _probe_single_server(server_name, cfg, connect_timeout=timeout)
|
||||
tools = _probe_single_server(server_name, cfg, connect_timeout=login_connect_timeout(cfg))
|
||||
if not _oauth_tokens_present(server_name):
|
||||
raise RuntimeError(
|
||||
"The server responded, but no OAuth token was obtained — "
|
||||
|
||||
@@ -391,6 +391,8 @@ Then run `hermes mcp login googledrive` — with the pre-registered client, Herm
|
||||
|
||||
**Pitfall — config auto-reload race.** When you edit `~/.hermes/config.yaml` from inside a running Hermes session, the CLI auto-reloads MCP connections with a 30s timeout. That's not enough for an interactive OAuth flow. Add the entry, then run `hermes mcp login <server>` from a fresh terminal — it waits the full 5 minutes for you to complete auth.
|
||||
|
||||
**Need longer than 5 minutes to approve?** Set `oauth.timeout` on the server entry (seconds). `hermes mcp login`, the dashboard and Desktop re-auth all wait `oauth.timeout` + 15 s (or the entry's `connect_timeout`, whichever is longer); a login that still runs out of time reports `Connecting to MCP server '<name>' timed out after Ns` naming both knobs instead of a blank failure line.
|
||||
|
||||
## mTLS / client certificates
|
||||
|
||||
Remote HTTP MCP servers that require mutual TLS (client-certificate authentication) are supported via `client_cert` / `client_key`. Hermes passes the resolved certificate to the underlying HTTP client for the TLS handshake.
|
||||
|
||||
Reference in New Issue
Block a user