From 067f0ac9a83dbc2407912980b261f2913bb789c3 Mon Sep 17 00:00:00 2001 From: nehaprasad-dev Date: Sat, 30 May 2026 00:22:02 +0530 Subject: [PATCH] fix(mcp): require Google Drive OAuth client credentials and real login Google Drive MCP rejects dynamic client registration; validate client_id, run an authenticated probe on login, and fail fast on registration errors. Co-authored-by: Cursor --- hermes_cli/mcp_config.py | 250 ++++++++++++++++++---- tests/hermes_cli/test_mcp_config.py | 12 ++ tests/tools/test_mcp_oauth.py | 32 +++ tests/tools/test_mcp_tool_401_handling.py | 9 + tools/mcp_oauth.py | 63 ++++++ tools/mcp_oauth_manager.py | 4 + tools/mcp_tool.py | 35 ++- 7 files changed, 354 insertions(+), 51 deletions(-) diff --git a/hermes_cli/mcp_config.py b/hermes_cli/mcp_config.py index ed9d7b5f6dbc..d07d6edb67ca 100644 --- a/hermes_cli/mcp_config.py +++ b/hermes_cli/mcp_config.py @@ -205,6 +205,116 @@ async def _probe(): return tools_found +_OAUTH_PROBE_TOOL_NAMES = ( + "list_recent_files", + "search_files", + "get_file_metadata", +) + + +def _pick_oauth_probe_tool(tool_names: List[str]) -> str: + """Pick a lightweight tool to force the OAuth flow during ``mcp login``.""" + for preferred in _OAUTH_PROBE_TOOL_NAMES: + if preferred in tool_names: + return preferred + skip = frozenset({ + "list_prompts", "get_prompt", "list_resources", "read_resource", + }) + for name in tool_names: + if name not in skip: + return name + return tool_names[0] + + +def _authenticate_mcp_server( + name: str, + config: dict, + connect_timeout: float = 300, +) -> None: + """Connect and invoke one tool so OAuth runs before the session ends. + + Tool listing alone does not authenticate (e.g. Google Drive MCP exposes + tools without a token). This call must succeed for ``mcp login`` to + report success. + """ + from tools.mcp_tool import ( + _ensure_mcp_loop, + _run_on_mcp_loop, + _stop_mcp_loop, + _connect_server, + ) + + _ensure_mcp_loop() + + async def _auth(): + server = await asyncio.wait_for( + _connect_server(name, config), timeout=connect_timeout, + ) + try: + tool_names = [t.name for t in server._tools] + if not tool_names: + raise RuntimeError("server reported no tools") + probe_tool = _pick_oauth_probe_tool(tool_names) + async with server._rpc_lock: + await server.session.call_tool(probe_tool, arguments={}) + finally: + await server.shutdown() + + try: + _run_on_mcp_loop(_auth(), timeout=connect_timeout + 30) + except BaseException as exc: + raise _unwrap_exception_group(exc) from None + finally: + _stop_mcp_loop() + + +def _prompt_google_drive_oauth(server_config: dict, url: str) -> bool: + """Collect Google Drive MCP OAuth credentials into ``server_config``.""" + from tools.mcp_oauth import ( + McpOAuthConfigError, + default_oauth_config_for_url, + is_google_drive_mcp_url, + ) + + if not is_google_drive_mcp_url(url): + return True + + print() + _info( + "Google Drive MCP requires your own OAuth client (Desktop app) from " + "Google Cloud Console — dynamic registration is not supported." + ) + _info("See: https://developers.google.com/workspace/drive/api/guides/configure-mcp-server") + + oauth_cfg = dict(server_config.get("oauth") or {}) + oauth_cfg.update(default_oauth_config_for_url(url)) + + client_id = oauth_cfg.get("client_id") or _prompt("OAuth client ID") + if not client_id: + _error("OAuth client ID is required for Google Drive MCP.") + return False + oauth_cfg["client_id"] = client_id.strip() + + client_secret = oauth_cfg.get("client_secret") + if not client_secret: + client_secret = _prompt("OAuth client secret", password=True) + if not client_secret: + _error("OAuth client secret is required for Google Drive MCP.") + return False + oauth_cfg["client_secret"] = client_secret.strip() + + server_config["auth"] = "oauth" + server_config["oauth"] = oauth_cfg + + try: + from tools.mcp_oauth import validate_oauth_config + validate_oauth_config(name, url, oauth_cfg) + except McpOAuthConfigError as exc: + _error(str(exc)) + return False + return True + + def _unwrap_exception_group(exc: BaseException) -> Exception: """Extract the root-cause exception from anyio TaskGroup wrappers. @@ -284,54 +394,81 @@ def cmd_mcp_add(args): # ── Authentication ──────────────────────────────────────────────── - if url and auth_type == "oauth": - print() - _info(f"Starting OAuth flow for '{name}'...") - oauth_ok = False - try: - from tools.mcp_oauth_manager import get_manager - oauth_auth = get_manager().get_or_build_provider(name, url, None) - if oauth_auth: - server_config["auth"] = "oauth" - _success("OAuth configured (tokens will be acquired on first connection)") - oauth_ok=True + if url: + from tools.mcp_oauth import is_google_drive_mcp_url + + use_oauth = auth_type == "oauth" or is_google_drive_mcp_url(url) + + if use_oauth: + if is_google_drive_mcp_url(url): + if not _prompt_google_drive_oauth(server_config, url): + _info("Cancelled.") + return else: - _warning("OAuth setup failed — MCP SDK auth module not available") - except Exception as exc: - _warning(f"OAuth error: {exc}") - - if not oauth_ok: - _info("This server may not support OAuth.") - if _confirm("Continue without authentication?", default=True): - # Don't store auth: oauth — server doesn't support it - pass + server_config["auth"] = "oauth" + + print() + _info(f"Starting OAuth flow for '{name}'...") + oauth_ok = False + try: + from tools.mcp_oauth_manager import get_manager + oauth_auth = get_manager().get_or_build_provider( + name, url, server_config.get("oauth"), + ) + oauth_ok = oauth_auth is not None + if not oauth_ok: + _warning("OAuth setup failed — MCP SDK auth module not available") + except Exception as exc: + _warning(f"OAuth error: {exc}") + + if oauth_ok: + try: + _authenticate_mcp_server(name, server_config) + from tools.mcp_oauth import HermesTokenStorage + if HermesTokenStorage(name).has_cached_tokens(): + _success("OAuth complete — tokens saved") + else: + _warning( + "Connected but no OAuth tokens were saved. Run " + f"`hermes mcp login {name}` after fixing credentials." + ) + except Exception as exc: + _warning(f"OAuth authorization did not complete: {exc}") + elif not is_google_drive_mcp_url(url): + _info("This server may not support OAuth.") + if _confirm("Continue without authentication?", default=True): + server_config.pop("auth", None) + server_config.pop("oauth", None) + else: + _info("Cancelled.") + return else: _info("Cancelled.") return - elif url: - # Prompt for API key / Bearer token for HTTP servers - print() - _info(f"Connecting to {url}") - needs_auth = _confirm("Does this server require authentication?", default=True) - if needs_auth: - if auth_type == "header" or not auth_type: - env_key = _env_key_for_server(name) - existing_key = get_env_value(env_key) - if existing_key: - _success(f"{env_key}: already configured") - api_key = existing_key - else: - api_key = _prompt("API key / Bearer token", password=True) - if api_key: - save_env_value(env_key, api_key) - _success(f"Saved to {display_hermes_home()}/.env as {env_key}") - - # Set header with env var interpolation - if api_key or existing_key: - server_config["headers"] = { - "Authorization": f"Bearer ${{{env_key}}}" - } + else: + # Prompt for API key / Bearer token for HTTP servers + print() + _info(f"Connecting to {url}") + needs_auth = _confirm("Does this server require authentication?", default=True) + if needs_auth: + if auth_type == "header" or not auth_type: + env_key = _env_key_for_server(name) + existing_key = get_env_value(env_key) + if existing_key: + _success(f"{env_key}: already configured") + api_key = existing_key + else: + api_key = _prompt("API key / Bearer token", password=True) + if api_key: + save_env_value(env_key, api_key) + _success(f"Saved to {display_hermes_home()}/.env as {env_key}") + + # Set header with env var interpolation + if api_key or existing_key: + server_config["headers"] = { + "Authorization": f"Bearer ${{{env_key}}}" + } # ── Discovery: connect and list tools ───────────────────────────── @@ -616,7 +753,15 @@ def cmd_mcp_login(args): _info("Use `hermes mcp remove` + `hermes mcp add` to reconfigure auth.") return - # Wipe both disk and in-memory cache so the next probe forces a fresh + oauth_cfg = server_config.get("oauth") + try: + from tools.mcp_oauth import validate_oauth_config + validate_oauth_config(name, url, oauth_cfg) + except Exception as exc: + _error(str(exc)) + return + + # Wipe both disk and in-memory cache so the next login forces a fresh # OAuth flow. try: from tools.mcp_oauth_manager import get_manager @@ -628,7 +773,20 @@ def cmd_mcp_login(args): print() _info(f"Starting OAuth flow for '{name}'...") - # Probe triggers the OAuth flow (browser redirect + callback capture). + try: + _authenticate_mcp_server(name, server_config) + except Exception as exc: + _error(f"Authentication failed: {exc}") + return + + from tools.mcp_oauth import HermesTokenStorage + if not HermesTokenStorage(name).has_cached_tokens(): + _error( + "OAuth flow finished but no tokens were saved. Check " + "oauth.client_id / oauth.client_secret in config.yaml." + ) + return + try: tools = _probe_single_server(name, server_config) if tools: @@ -636,7 +794,7 @@ def cmd_mcp_login(args): else: _success("Authenticated (server reported no tools)") except Exception as exc: - _error(f"Authentication failed: {exc}") + _warning(f"Tokens saved but tool discovery failed: {exc}") # ─── hermes mcp configure ──────────────────────────────────────────────────── diff --git a/tests/hermes_cli/test_mcp_config.py b/tests/hermes_cli/test_mcp_config.py index e136f1b3c0fc..ed75605585ac 100644 --- a/tests/hermes_cli/test_mcp_config.py +++ b/tests/hermes_cli/test_mcp_config.py @@ -600,3 +600,15 @@ def test_login_rejects_stdio_server(self, tmp_path, capsys): out = capsys.readouterr().out assert "no URL" in out or "not an OAuth" in out + def test_login_rejects_google_drive_without_client_id(self, tmp_path, capsys): + _seed_config(tmp_path, { + "drive": { + "url": "https://drivemcp.googleapis.com/mcp/v1", + "auth": "oauth", + }, + }) + from hermes_cli.mcp_config import cmd_mcp_login + cmd_mcp_login(_make_args(name="drive")) + out = capsys.readouterr().out + assert "client_id" in out + diff --git a/tests/tools/test_mcp_oauth.py b/tests/tools/test_mcp_oauth.py index b858127cd074..a20a8d3727fa 100644 --- a/tests/tools/test_mcp_oauth.py +++ b/tests/tools/test_mcp_oauth.py @@ -14,9 +14,13 @@ from tools.mcp_oauth import ( HermesTokenStorage, + McpOAuthConfigError, OAuthNonInteractiveError, build_oauth_auth, + default_oauth_config_for_url, + is_google_drive_mcp_url, remove_oauth_tokens, + validate_oauth_config, _find_free_port, _can_open_browser, _is_interactive, @@ -829,3 +833,31 @@ async def instant_sleep(_): asyncio.run(_wait_for_callback()) err = capsys.readouterr().err assert "skip" in err.lower() + + +class TestGoogleDriveMcpOAuth: + def test_is_google_drive_mcp_url(self): + assert is_google_drive_mcp_url("https://drivemcp.googleapis.com/mcp/v1") + assert not is_google_drive_mcp_url("https://mcp.linear.app/mcp") + + def test_default_oauth_config_includes_drive_scopes(self): + cfg = default_oauth_config_for_url("https://drivemcp.googleapis.com/mcp/v1") + assert "drive.readonly" in cfg["scope"] + assert "drive.file" in cfg["scope"] + + def test_validate_requires_client_id_for_google_drive(self, tmp_path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + with pytest.raises(McpOAuthConfigError, match="client_id"): + validate_oauth_config( + "drive", + "https://drivemcp.googleapis.com/mcp/v1", + None, + ) + + def test_validate_passes_with_client_id(self, tmp_path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + validate_oauth_config( + "drive", + "https://drivemcp.googleapis.com/mcp/v1", + {"client_id": "abc", "client_secret": "secret"}, + ) diff --git a/tests/tools/test_mcp_tool_401_handling.py b/tests/tools/test_mcp_tool_401_handling.py index a60d2049f65a..70163ecdffe7 100644 --- a/tests/tools/test_mcp_tool_401_handling.py +++ b/tests/tools/test_mcp_tool_401_handling.py @@ -23,6 +23,15 @@ def test_is_auth_error_detects_oauth_flow_error(): assert _is_auth_error(OAuthFlowError("expired")) is True +def test_is_auth_error_detects_oauth_registration_error(): + from tools.mcp_tool import _is_auth_error + try: + from mcp.client.auth.exceptions import OAuthRegistrationError + except ImportError: + pytest.skip("OAuthRegistrationError not in this MCP SDK version") + assert _is_auth_error(OAuthRegistrationError("Registration failed: 400")) is True + + def test_is_auth_error_detects_oauth_non_interactive(): from tools.mcp_tool import _is_auth_error from tools.mcp_oauth import OAuthNonInteractiveError diff --git a/tools/mcp_oauth.py b/tools/mcp_oauth.py index 832a6f5945f4..ec04e083c3d4 100644 --- a/tools/mcp_oauth.py +++ b/tools/mcp_oauth.py @@ -85,6 +85,67 @@ class OAuthNonInteractiveError(RuntimeError): """Raised when OAuth requires browser interaction in a non-interactive env.""" +class McpOAuthConfigError(ValueError): + """OAuth config is incomplete or invalid for the target MCP server.""" + + +# Google's hosted Drive MCP rejects dynamic client registration (DCR). +_GOOGLE_DRIVE_MCP_HOST = "drivemcp.googleapis.com" +_GOOGLE_DRIVE_MCP_SCOPES = ( + "https://www.googleapis.com/auth/drive.readonly " + "https://www.googleapis.com/auth/drive.file" +) +_GOOGLE_DRIVE_MCP_DOCS = ( + "https://developers.google.com/workspace/drive/api/guides/configure-mcp-server" +) + + +def is_google_drive_mcp_url(server_url: str) -> bool: + """Return True if ``server_url`` points at Google's hosted Drive MCP.""" + try: + host = urlparse(server_url).netloc.lower() + except (ValueError, AttributeError): + return False + return host == _GOOGLE_DRIVE_MCP_HOST or host.endswith("." + _GOOGLE_DRIVE_MCP_HOST) + + +def default_oauth_config_for_url(server_url: str) -> dict[str, Any]: + """Return suggested ``oauth:`` block fields for known hosted MCP servers.""" + if is_google_drive_mcp_url(server_url): + return {"scope": _GOOGLE_DRIVE_MCP_SCOPES} + return {} + + +def validate_oauth_config( + server_name: str, + server_url: str, + oauth_config: dict | None, +) -> None: + """Raise :class:`McpOAuthConfigError` when required OAuth fields are missing. + + Google's Drive MCP does not support dynamic client registration. Hermes + must have a pre-registered ``client_id`` (and usually ``client_secret``) + before the browser authorization flow can start. + """ + if not is_google_drive_mcp_url(server_url): + return + + cfg = oauth_config or {} + if cfg.get("client_id"): + return + + client_info = _read_json(HermesTokenStorage(server_name)._client_info_path()) + if client_info and client_info.get("client_id"): + return + + raise McpOAuthConfigError( + "Google Drive MCP requires oauth.client_id and oauth.client_secret from a " + "Google Cloud OAuth client (Desktop app). Dynamic client registration is " + "not supported by drivemcp.googleapis.com. See " + f"{_GOOGLE_DRIVE_MCP_DOCS}" + ) + + # --------------------------------------------------------------------------- # Module-level state # --------------------------------------------------------------------------- @@ -750,6 +811,8 @@ def build_oauth_auth( ) return None + validate_oauth_config(server_name, server_url, oauth_config) + cfg = dict(oauth_config or {}) # copy — we mutate _resolved_port storage = HermesTokenStorage(server_name) diff --git a/tools/mcp_oauth_manager.py b/tools/mcp_oauth_manager.py index 6a4573a8677d..6f889b4c4081 100644 --- a/tools/mcp_oauth_manager.py +++ b/tools/mcp_oauth_manager.py @@ -420,6 +420,10 @@ def _build_provider( if not _OAUTH_AVAILABLE: return None + from tools.mcp_oauth import validate_oauth_config + + validate_oauth_config(server_name, entry.server_url, entry.oauth_config) + cfg = dict(entry.oauth_config or {}) storage = HermesTokenStorage(server_name) diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py index 75c1c5e86338..21ff6240dff5 100644 --- a/tools/mcp_tool.py +++ b/tools/mcp_tool.py @@ -1794,6 +1794,11 @@ def _get_auth_error_types() -> tuple: types.extend([OAuthFlowError, OAuthTokenError]) except ImportError: pass + try: + from mcp.client.auth.exceptions import OAuthRegistrationError + types.append(OAuthRegistrationError) + except ImportError: + pass try: # Older MCP SDK variants exported this from mcp.client.auth import UnauthorizedError # type: ignore @@ -1801,8 +1806,8 @@ def _get_auth_error_types() -> tuple: except ImportError: pass try: - from tools.mcp_oauth import OAuthNonInteractiveError - types.append(OAuthNonInteractiveError) + from tools.mcp_oauth import McpOAuthConfigError, OAuthNonInteractiveError + types.extend([OAuthNonInteractiveError, McpOAuthConfigError]) except ImportError: pass try: @@ -1931,12 +1936,32 @@ async def _recover(): # needs_reauth error. Bumps the circuit breaker so the model stops # retrying the tool. _bump_server_error(server_name) + reauth_hint = ( + f"Run `hermes mcp login {server_name}` (or delete the tokens " + f"file under ~/.hermes/mcp-tokens/ and restart)." + ) + try: + from tools.mcp_oauth import is_google_drive_mcp_url, McpOAuthConfigError + with _lock: + srv = _servers.get(server_name) + server_url = (srv._config.get("url") or "") if srv else "" + if isinstance(exc, McpOAuthConfigError) or ( + is_google_drive_mcp_url(server_url) + and "Registration failed" in str(exc) + ): + reauth_hint = ( + "Add oauth.client_id and oauth.client_secret from a Google Cloud " + "Desktop OAuth client, then run " + f"`hermes mcp login {server_name}`. See " + "https://developers.google.com/workspace/drive/api/guides/configure-mcp-server" + ) + except ImportError: + pass return json.dumps({ "error": ( f"MCP server '{server_name}' requires re-authentication. " - f"Run `hermes mcp login {server_name}` (or delete the tokens " - f"file under ~/.hermes/mcp-tokens/ and restart). Do NOT retry " - f"this tool — ask the user to re-authenticate." + f"{reauth_hint} Do NOT retry this tool — ask the user to " + f"re-authenticate." ), "needs_reauth": True, "server": server_name,