diff --git a/tests/tools/test_mcp_failure_classification.py b/tests/tools/test_mcp_failure_classification.py index 2bd8454c53..b3d87bc03a 100644 --- a/tests/tools/test_mcp_failure_classification.py +++ b/tests/tools/test_mcp_failure_classification.py @@ -14,7 +14,8 @@ import logging import pytest from tools.mcp_tool_errors import ( - InvalidMcpUrlError, NonMcpEndpointError, _classify_mcp_failure, _unwrap_exception_group) + InvalidMcpUrlError, NonMcpEndpointError, _auth_error_detail, _classify_mcp_failure, _mcp_call_failed_message, + _unwrap_exception_group) from tools.mcp_tool import MCPServerTask @@ -66,6 +67,67 @@ class TestClassifyMcpFailure: assert _classify_mcp_failure(g) == "permanent" +# ── _auth_error_detail / _mcp_call_failed_message ──────────────────────────── + +class TestAuthErrorSurfacing: + def test_registration_detail_surfaces(self): + """The actionable registration guidance raised by the DCR guard must + reach the needs_reauth tool error (GH#78190).""" + pytest.importorskip("mcp.client.auth") + from mcp.client.auth import OAuthRegistrationError + + exc = OAuthRegistrationError( + "MCP OAuth 'gmail': this provider does not support automatic " + "client registration ... add it under config.yaml " + "mcp_servers..oauth (client_id, client_secret), then run " + "`hermes mcp login gmail`." + ) + detail = _auth_error_detail(exc) + assert detail.startswith(" ") + assert "config.yaml" in detail + assert "client_id" in detail + + def test_registration_detail_unwraps_group(self): + """The transport raises the auth error inside a TaskGroup wrapper — + the detail must still be found.""" + pytest.importorskip("mcp.client.auth") + from mcp.client.auth import OAuthRegistrationError + + exc = OAuthRegistrationError("config.yaml guidance") + assert "config.yaml" in _auth_error_detail(_group(exc)) + + def test_non_registration_no_detail(self): + assert _auth_error_detail(ValueError("nope")) == "" + assert _auth_error_detail(_group(TimeoutError("t"))) == "" + + def test_mcp_call_failed_message_unwraps_group(self): + """The generic error path must surface the group root cause instead + of the opaque TaskGroup wrapper text.""" + pytest.importorskip("mcp.client.auth") + from mcp.client.auth import OAuthRegistrationError + + exc = OAuthRegistrationError("registration guidance text") + msg = _mcp_call_failed_message(_group(exc)) + assert "OAuthRegistrationError" in msg + assert "registration guidance text" in msg + assert "TaskGroup" not in msg + + def test_needs_reauth_tool_error_carries_guidance(self, monkeypatch): + """Through the real recovery handler: an unrecoverable registration + failure returns needs_reauth WITH the guard's guidance.""" + pytest.importorskip("mcp.client.auth") + import json + from mcp.client.auth import OAuthRegistrationError + from tools import mcp_tool_handlers as handlers + + monkeypatch.setattr(handlers._loop, "_run_on_mcp_loop", lambda *_a, **_k: False) + out = json.loads(handlers._handle_auth_error_and_retry( + "gmail", OAuthRegistrationError("Create an OAuth client (config.yaml)."), + lambda: None, "call_tool")) + assert out["needs_reauth"] is True + assert "requires re-authentication. Create an OAuth client (config.yaml). Run" in out["error"] + + # ── Keepalive failure log surfaces the root cause ──────────────────────────── @pytest.mark.no_isolate diff --git a/tests/tools/test_mcp_oauth.py b/tests/tools/test_mcp_oauth.py index 610e434fa0..0597cf742c 100644 --- a/tests/tools/test_mcp_oauth.py +++ b/tests/tools/test_mcp_oauth.py @@ -1104,3 +1104,34 @@ def test_humanize_non_registration_403_passthrough(): ) is None ) + + +def test_humanize_404_registration_error_guidance(): + """A 404 on the registration endpoint surfaces pre-registered-client + guidance instead of the raw SDK traceback (GH#78190 — Google's hosted + Gmail/Drive MCP servers answer the SDK's guessed /register with 404).""" + from tools.mcp_oauth import humanize_oauth_registration_error + + msg = humanize_oauth_registration_error( + "gmail", + "Registration failed: 404 Error 404 (Not Found)!!1", + server_url="https://gmailmcp.googleapis.com/mcp/v1", + ) + assert msg is not None + assert "does not support automatic client registration" in msg + assert "client_id" in msg + assert "hermes mcp login gmail" in msg + + +def test_humanize_404_non_registration_passthrough(): + """A 404 that has nothing to do with registration stays raw.""" + from tools.mcp_oauth import humanize_oauth_registration_error + + assert ( + humanize_oauth_registration_error( + "srv", + RuntimeError("HTTP 404: no such tool"), + server_url="https://example.com/mcp", + ) + is None + ) diff --git a/tests/tools/test_mcp_oauth_bidirectional.py b/tests/tools/test_mcp_oauth_bidirectional.py index 6ee6703610..3b8481c000 100644 --- a/tests/tools/test_mcp_oauth_bidirectional.py +++ b/tests/tools/test_mcp_oauth_bidirectional.py @@ -234,3 +234,277 @@ async def _noop_callback() -> tuple[str, str | None]: raise AssertionError( "callback handler should not be invoked in bidirectional-generator tests" ) + + +@pytest.mark.asyncio +async def test_fabricated_dcr_registration_rejected_before_network(tmp_path, monkeypatch): + """The bridge must raise instead of sending the SDK's *fabricated* DCR POST. + + Google's hosted Gmail/Drive MCP servers advertise no RFC 7591 + ``registration_endpoint`` in their ASM, so the SDK falls back to guessing + ``POST {server_origin}/register`` (``create_client_registration_request``, + mcp/client/auth/utils.py) — a URL those servers answer with an opaque 404 + (GH#78190). The guard in ``HermesMCPOAuthProvider`` must intercept that + guessed request and raise an actionable ``OAuthRegistrationError`` before + any network call, instead of letting the gateway burn the reconnect + ladder on an unrecoverable registration failure. + + Drive the full discovery dance (401 -> PRM -> ASM without + registration_endpoint -> fabricated registration POST) through the + generator manually, exactly like the other tests in this file. + """ + from tools.mcp_tool import sdk_httpx + httpx = sdk_httpx() + from mcp.client.auth import OAuthRegistrationError + from mcp.shared.auth import OAuthClientMetadata + from pydantic import AnyUrl + + from tools.mcp_oauth import HermesTokenStorage + from tools.mcp_oauth_manager import _HERMES_PROVIDER_CLS, reset_manager_for_tests + + assert _HERMES_PROVIDER_CLS is not None, "SDK OAuth types must be available" + + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + reset_manager_for_tests() + + # Empty storage: no tokens, no client_info — the SDK's 401 branch must + # take the registration path (this is the cold gateway state for a + # provider whose cached client is gone). + storage = HermesTokenStorage("srv") + + metadata = OAuthClientMetadata( + redirect_uris=[AnyUrl("http://127.0.0.1:12345/callback")], + client_name="Hermes Agent", + ) + provider = _HERMES_PROVIDER_CLS( + server_name="srv", + server_url="https://example.com/mcp", + client_metadata=metadata, + storage=storage, + redirect_handler=_noop_redirect, + callback_handler=_noop_callback, + ) + + req = httpx.Request("POST", "https://example.com/mcp") + flow = provider.async_auth_flow(req) + + # 1. Initial outbound MCP request. + outbound = await flow.__anext__() + assert outbound is not None + + # 2. 401 with a resource_metadata pointer -> SDK starts PRM discovery. + fake_401 = httpx.Response( + 401, + request=outbound, + headers={ + "www-authenticate": ( + 'Bearer resource_metadata="https://example.com/.well-known/' + 'oauth-protected-resource"' + ) + }, + ) + prm_request = await flow.asend(fake_401) + assert isinstance(prm_request, httpx.Request) + assert prm_request.method == "GET" + + # 3. Serve PRM pointing at a fake authorization server. + prm_response = httpx.Response( + 200, + request=prm_request, + headers={"Content-Type": "application/json"}, + json={ + "authorization_servers": ["https://auth.example.com"], + "resource": "https://example.com/mcp", + "bearer_methods_supported": ["header"], + }, + ) + asm_request = await flow.asend(prm_response) + assert isinstance(asm_request, httpx.Request) + assert asm_request.method == "GET" + + # 4. Serve ASM with NO registration_endpoint (the Google Gmail/Drive + # MCP class of provider). + asm_response = httpx.Response( + 200, + request=asm_request, + headers={"Content-Type": "application/json"}, + json={ + "issuer": "https://auth.example.com", + "authorization_endpoint": "https://auth.example.com/authorize", + "token_endpoint": "https://auth.example.com/token", + "response_types_supported": ["code"], + "code_challenge_methods_supported": ["S256"], + }, + ) + + # 5. The SDK now wants to POST the fabricated registration URL — the + # bridge must raise the actionable error instead of yielding it. + with pytest.raises(OAuthRegistrationError) as excinfo: + await flow.asend(asm_response) + text = str(excinfo.value) + assert "does not support automatic client registration" in text + assert "https://example.com/register" in text + assert "config.yaml" in text + assert "hermes mcp login srv" in text + await flow.aclose() + + +@pytest.mark.asyncio +async def test_advertised_registration_endpoint_passes_through(tmp_path, monkeypatch): + """A server that *advertises* a registration endpoint must NOT be blocked. + + Real-world case: Linear's MCP ASM advertises + ``registration_endpoint: https://mcp.linear.app/register`` — the same + origin+path the SDK would fabricate. The guard's AND condition (exact + fabricated URL **and** no advertised registration_endpoint) must let + such servers through; otherwise every legitimate DCR flow breaks. + """ + from tools.mcp_tool import sdk_httpx + httpx = sdk_httpx() + from mcp.shared.auth import OAuthClientMetadata + from pydantic import AnyUrl + + from tools.mcp_oauth import HermesTokenStorage + from tools.mcp_oauth_manager import _HERMES_PROVIDER_CLS, reset_manager_for_tests + + assert _HERMES_PROVIDER_CLS is not None, "SDK OAuth types must be available" + + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + reset_manager_for_tests() + + storage = HermesTokenStorage("srv") + metadata = OAuthClientMetadata( + redirect_uris=[AnyUrl("http://127.0.0.1:12345/callback")], + client_name="Hermes Agent", + ) + provider = _HERMES_PROVIDER_CLS( + server_name="srv", + server_url="https://example.com/mcp", + client_metadata=metadata, + storage=storage, + redirect_handler=_noop_redirect, + callback_handler=_noop_callback, + ) + + req = httpx.Request("POST", "https://example.com/mcp") + flow = provider.async_auth_flow(req) + + outbound = await flow.__anext__() + fake_401 = httpx.Response( + 401, + request=outbound, + headers={ + "www-authenticate": ( + 'Bearer resource_metadata="https://example.com/.well-known/' + 'oauth-protected-resource"' + ) + }, + ) + prm_request = await flow.asend(fake_401) + prm_response = httpx.Response( + 200, + request=prm_request, + headers={"Content-Type": "application/json"}, + json={ + "authorization_servers": ["https://auth.example.com"], + "resource": "https://example.com/mcp", + "bearer_methods_supported": ["header"], + }, + ) + asm_request = await flow.asend(prm_response) + + # ASM DOES advertise a registration endpoint — at origin+/register, + # exactly the URL the SDK would otherwise fabricate (Linear's ASM does + # this: https://mcp.linear.app/register). + asm_response = httpx.Response( + 200, + request=asm_request, + headers={"Content-Type": "application/json"}, + json={ + "issuer": "https://auth.example.com", + "authorization_endpoint": "https://auth.example.com/authorize", + "token_endpoint": "https://auth.example.com/token", + "registration_endpoint": "https://example.com/register", + "response_types_supported": ["code"], + "code_challenge_methods_supported": ["S256"], + }, + ) + + # The registration POST must be yielded (not blocked): the SDK uses the + # advertised endpoint and the guard must pass it through. + registration_request = await flow.asend(asm_response) + assert isinstance(registration_request, httpx.Request) + assert registration_request.method == "POST" + assert str(registration_request.url) == "https://example.com/register" + + # No OAuthRegistrationError may have been raised along the way. (The + # dance ends here: a 200 registration response would hand control to the + # browser-redirect flow, which is outside this test's scope.) + await flow.aclose() + + +@pytest.mark.asyncio +async def test_guard_ignores_non_fabricated_post_paths(tmp_path, monkeypatch): + """A POST to a *different* path must never be blocked by the guard. + + The guard is exact-URL matched: only the SDK's fabricated + ``{origin}/register`` POST is rejected when the ASM lacks a + registration_endpoint. A server-side registration at ``/foo/register`` + or any other path passes through untouched. Undiscovered metadata (a + blocked ASM fetch) never trips the guard: the SDK's guess may be a real + endpoint, and the discovery-context error explains a failure there. + """ + from tools.mcp_tool import sdk_httpx + httpx = sdk_httpx() + from mcp.shared.auth import OAuthClientMetadata, OAuthMetadata + from pydantic import AnyUrl + + from tools.mcp_oauth import HermesTokenStorage + from tools.mcp_oauth_manager import _HERMES_PROVIDER_CLS, reset_manager_for_tests + + assert _HERMES_PROVIDER_CLS is not None, "SDK OAuth types must be available" + + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + reset_manager_for_tests() + + provider = _HERMES_PROVIDER_CLS( + server_name="srv", + server_url="https://example.com/mcp", + client_metadata=OAuthClientMetadata( + redirect_uris=[AnyUrl("http://127.0.0.1:12345/callback")], + client_name="Hermes Agent", + ), + storage=HermesTokenStorage("srv"), + redirect_handler=_noop_redirect, + callback_handler=_noop_callback, + ) + + # No ASM discovered: even the exact guessed URL passes through. + assert provider.context.oauth_metadata is None + await provider._maybe_reject_fabricated_registration( + httpx.Request("POST", "https://example.com/register") + ) # must not raise + + # ASM discovered without registration_endpoint: only the exact + # origin+/register POST is refused; everything else passes. + provider.context.oauth_metadata = OAuthMetadata( + issuer="https://auth.example.com", + authorization_endpoint="https://auth.example.com/authorize", + token_endpoint="https://auth.example.com/token", + ) + for method, url in ( + ("POST", "https://example.com/foo/register"), + ("POST", "https://example.com/register/"), + ("POST", "https://example.com/registers"), + ("POST", "https://other.example.com/register"), + ("POST", "https://example.com:8443/register"), + ("GET", "https://example.com/register"), + ): + await provider._maybe_reject_fabricated_registration( + httpx.Request(method, url) + ) # must not raise + from mcp.client.auth import OAuthRegistrationError + with pytest.raises(OAuthRegistrationError): + await provider._maybe_reject_fabricated_registration( + httpx.Request("POST", "https://example.com:443/register") + ) diff --git a/tools/mcp_oauth.py b/tools/mcp_oauth.py index 1a05f37f8f..4e58be3f1c 100644 --- a/tools/mcp_oauth.py +++ b/tools/mcp_oauth.py @@ -1195,7 +1195,7 @@ def _maybe_preregister_client(storage: "HermesTokenStorage", cfg: dict, client_m def humanize_oauth_registration_error( server_name: str, exc: BaseException | str, *, server_url: str | None = None) -> str | None: - """Turn a DCR 403/Forbidden into a useful next step; None for anything else so the caller keeps the + """Turn a DCR 403/Forbidden or 404 into a useful next step; None for anything else so the caller keeps the original text. Figma gates DCR on exact ``client_name`` (auto-set to ``Claude Code``), so this fires when the user overrode it or an older Hermes is running.""" msg = str(exc) @@ -1203,6 +1203,15 @@ def humanize_oauth_registration_error( from tools.mcp_oauth_provider import _DISCOVERY_CONTEXT_LEAD if msg.startswith(_DISCOVERY_CONTEXT_LEAD): # the 403 there is the metadata fetch, not a DCR refusal return None + # A 404 on registration (Google's hosted Gmail/Drive MCP servers answer the SDK's guessed /register with + # one) is the same permanent "no RFC 7591 DCR" class as a 403 refusal (#78190). + if ("404" in msg and ("not found" in lowered or "/register" in lowered) + and any(k in lowered for k in ("regist", "dcr", "dynamic client"))): + return ( + f"'{server_name}' does not support automatic client registration — the registration request returned " + "404 (typical for Google's hosted Gmail/Drive MCP servers and other providers without RFC 7591 dynamic " + "client registration). Create an OAuth client for this provider and add it under config.yaml as " + f"`oauth: {{client_id: ..., client_secret: ...}}`, then re-run:\n hermes mcp login {server_name}") looks_like_registration = ("403" in msg or "forbidden" in lowered) and ( any(k in lowered for k in ("regist", "dcr", "dynamic client")) or lowered.strip() in {"forbidden", "403 forbidden", "http 403: forbidden"} diff --git a/tools/mcp_oauth_manager.py b/tools/mcp_oauth_manager.py index 9f3a2a8339..c9aa2c7b49 100644 --- a/tools/mcp_oauth_manager.py +++ b/tools/mcp_oauth_manager.py @@ -206,6 +206,38 @@ class HermesMCPOAuthProvider(HermesProviderMixin, *_SDK_BASES): except Exception as exc: # pragma: no cover — must not throw self._log_nonfatal("invalid_client detection", exc) + async def _maybe_reject_fabricated_registration(self, outgoing: Any) -> None: + """Refuse to send the SDK's *fabricated* dynamic client registration (#78190). + + When the discovered authorization-server metadata (RFC 8414) has no ``registration_endpoint``, + ``create_client_registration_request`` (mcp/client/auth/utils.py) guesses ``POST {origin}/register``. + Providers without RFC 7591 DCR — Google's hosted Gmail/Drive MCP servers among them — answer with an + opaque 404 and the gateway burns its reconnect ladder on an unrecoverable failure. Their supported + path is a pre-registered client (``oauth.client_id`` / ``client_secret``), so raise that guidance + instead of sending the request. + + Fires only when the metadata WAS discovered and lacks the endpoint, and the request is a POST to the + exact guessed URL. Undiscovered metadata (a WAF-blocked fetch) keeps the SDK's guess, so a real + ``/register`` still works and the discovery-context error (#113771) still explains a failure. + """ + meta = getattr(self.context, "oauth_metadata", None) + if meta is None or getattr(meta, "registration_endpoint", None) or getattr(outgoing, "method", None) != "POST": + return + from urllib.parse import urljoin + url = outgoing.url + # Same URL type as the request (httpx or httpx2), so default-port handling matches on both sides. + expected = type(url)(urljoin(self.context.get_authorization_base_url(self.context.server_url), "/register")) + if (url.scheme, url.host, url.port, url.path) != (expected.scheme, expected.host, expected.port, expected.path): + return + from mcp.client.auth.oauth2 import OAuthRegistrationError + name = self._hermes_server_name + raise OAuthRegistrationError( + f"MCP OAuth '{name}': this provider does not support automatic client registration (its authorization " + f"server advertises no registration_endpoint), so the SDK's fallback would POST {expected} and fail " + "(Google's hosted Gmail/Drive MCP servers and similar providers return 404 for it). Create an OAuth " + "client for this provider and add it under config.yaml mcp_servers..oauth (client_id, " + f"client_secret), then run `hermes mcp login {name}`.") + async def async_auth_flow(self, request): # type: ignore[override] try: # pre-flow hook: reload from disk if it changed (non-fatal on error) await get_manager().invalidate_if_disk_changed(self._hermes_server_name, hermes_home=self._hermes_home) @@ -234,6 +266,12 @@ class HermesMCPOAuthProvider(HermesProviderMixin, *_SDK_BASES): sent_access_token = tokens.access_token if tokens is not None else None self.context.lock.release() resource_lock_released = True + else: + try: # refuse the SDK's guessed DCR POST before it hits the network + await self._maybe_reject_fabricated_registration(outgoing) + except Exception: + await inner.aclose() # release the SDK's context.lock now, not at GC + raise incoming = yield outgoing if resource_lock_released: await self.context.lock.acquire() diff --git a/tools/mcp_tool_errors.py b/tools/mcp_tool_errors.py index 0003d5a4a7..757cde7316 100644 --- a/tools/mcp_tool_errors.py +++ b/tools/mcp_tool_errors.py @@ -11,7 +11,7 @@ import os import re from typing import Any, List, Optional from urllib.parse import urlparse -from tools.mcp_tool_common import _sanitize_error, _core +from tools.mcp_tool_common import _exc_str, _sanitize_error, _core from tools.mcp_tool_node_abi import NodeAbiMismatchError logger = logging.getLogger("tools.mcp_tool") @@ -154,6 +154,26 @@ def _contains_only_cancellation(exc: BaseException) -> bool: return isinstance(exc, asyncio.CancelledError) +def _auth_error_detail(exc: BaseException) -> str: + """`` `` when *exc* (or its group root) is the SDK's ``OAuthRegistrationError``, else ``""``. The DCR + guard in ``HermesMCPOAuthProvider`` raises one naming the next step (pre-register an OAuth client), and the + generic needs-reauth text alone would hide it from the model (#78190).""" + try: + from mcp.client.auth import OAuthRegistrationError + except ImportError: # pragma: no cover — SDK required in CI + return "" + root = _unwrap_exception_group(exc) + text = str(root).strip() if isinstance(root, OAuthRegistrationError) else "" + return f" {_sanitize_error(text)}" if text else "" + + +def _mcp_call_failed_message(exc: BaseException) -> str: + """Generic tool-error text naming the group root cause: the transport's TaskGroup wrapper ``str()`` is + "unhandled errors in a TaskGroup (1 sub-exception)", which tells the model nothing.""" + root = _unwrap_exception_group(exc) + return _sanitize_error(f"MCP call failed: {type(root).__name__}: {_exc_str(root)}") + + def _classify_mcp_failure(exc: BaseException) -> str: """``'permanent'`` (``run()`` parks instead of burning the retry ladder: auth 401/403, NonMcpEndpointError, InvalidMcpUrlError, missing stdio command, native addon built for another diff --git a/tools/mcp_tool_handlers.py b/tools/mcp_tool_handlers.py index 54f81aa737..5eaf591791 100644 --- a/tools/mcp_tool_handlers.py +++ b/tools/mcp_tool_handlers.py @@ -21,7 +21,8 @@ from tools.mcp_tool_content import ( _MCP_HARD_RESULT_CAP_CHARS, _cache_mcp_audio_block, _cache_mcp_image_block, _mcp_result_with_native_images, _render_mcp_dropped_block_notice, _render_mcp_resource_block, _strip_reserved_meta_keys, _truncate_mcp_text_result) -from tools.mcp_tool_errors import _is_auth_error, _is_session_expired_error +from tools.mcp_tool_errors import ( + _auth_error_detail, _is_auth_error, _is_session_expired_error, _mcp_call_failed_message) logger = logging.getLogger("tools.mcp_tool") _MISSING = object() @@ -29,7 +30,7 @@ _MISSING = object() declaration.on_change = invalidate_check_fn_cache _NEEDS_REAUTH_MSG = ( - "MCP server '{s}' requires re-authentication. Run `hermes mcp login {s}` (or delete the tokens file under " + "MCP server '{s}' requires re-authentication.{detail} Run `hermes mcp login {s}` (or delete the tokens file under " "~/.hermes/mcp-tokens/ and restart). Do NOT retry this tool — ask the user to re-authenticate.") _STDIO_NO_RESPAWN_MSG = ( "MCP server '{s}' stdio subprocess had exited (this is not a timeout — the call never reached the server). A " @@ -210,7 +211,8 @@ def _handle_auth_error_and_retry(server_name: str, exc: BaseException, retry_cal result = _retry_once(server_name, retry_call, op_description, "auth recovery") if result is not None: return result - return _strike(server_name, _NEEDS_REAUTH_MSG.format(s=server_name), needs_reauth=True, server=server_name) + return _strike(server_name, _NEEDS_REAUTH_MSG.format(s=server_name, detail=_auth_error_detail(exc)), + needs_reauth=True, server=server_name) def _handle_session_expired_and_retry(server_name: str, exc: BaseException, retry_call, op_description: str, @@ -345,7 +347,7 @@ def _dispatch(server_name: str, server: Any, op: str, call, tool_timeout: float, if recovered is not None: return recovered on_final_failure(exc) - return tool_error(_sanitize_error(f"MCP call failed: {type(exc).__name__}: {_exc_str(exc)}")) + return tool_error(_mcp_call_failed_message(exc)) @asynccontextmanager