diff --git a/tests/tools/test_mcp_oauth.py b/tests/tools/test_mcp_oauth.py index 8f3f58ca9fba..a351fa811827 100644 --- a/tests/tools/test_mcp_oauth.py +++ b/tests/tools/test_mcp_oauth.py @@ -423,6 +423,50 @@ def test_port_stored_globally(self, tmp_path, monkeypatch): assert 1024 <= mod._oauth_port <= 65535 +class TestOAuthPortIsolation: + """Concurrent build_oauth_auth calls must get independent callback ports.""" + + def test_concurrent_providers_use_different_ports(self): + """Two providers built back-to-back capture their own ports.""" + from tools import mcp_oauth as mod + + captured_ports: list[int] = [] + + class _FakeProvider: + def __init__(self, **kwargs): + self.kwargs = kwargs + + with patch.object(mod, "_OAUTH_AVAILABLE", True), \ + patch.object(mod, "OAuthClientProvider", _FakeProvider), \ + patch.object(mod, "_is_interactive", return_value=True), \ + patch.object(mod, "_maybe_preregister_client"), \ + patch.object(mod, "HermesTokenStorage") as mock_cls: + mock_cls.return_value = MagicMock(has_cached_tokens=lambda: True) + + p1 = build_oauth_auth("server-a", "https://a.example.com/mcp") + port_a = mod._oauth_port + + p2 = build_oauth_auth("server-b", "https://b.example.com/mcp") + port_b = mod._oauth_port + + assert port_a != port_b, "back-to-back providers got the same global port" + + # Each closure must capture its own port, not the final global. + # Invoke the redirect handler and inspect the port it uses. + redirect_ports: list[int | None] = [] + + async def _spy_redirect(url: str, *, port: int | None = None) -> None: + redirect_ports.append(port) + + with patch.object(mod, "_redirect_handler", _spy_redirect): + asyncio.run(p1.kwargs["redirect_handler"]("https://a.example.com/auth")) + asyncio.run(p2.kwargs["redirect_handler"]("https://b.example.com/auth")) + + assert redirect_ports[0] == port_a + assert redirect_ports[1] == port_b + assert redirect_ports[0] != redirect_ports[1] + + # --------------------------------------------------------------------------- # remove_oauth_tokens # --------------------------------------------------------------------------- diff --git a/tools/mcp_oauth.py b/tools/mcp_oauth.py index eac9d28f5adf..877896dc584c 100644 --- a/tools/mcp_oauth.py +++ b/tools/mcp_oauth.py @@ -538,11 +538,17 @@ def log_message(self, fmt: str, *args: Any) -> None: # --------------------------------------------------------------------------- -async def _redirect_handler(authorization_url: str) -> None: +async def _redirect_handler( + authorization_url: str, *, port: int | None = None, +) -> None: """Show the authorization URL to the user. Opens the browser automatically when possible; always prints the URL as a fallback for headless/SSH/gateway environments. + + Args: + port: Callback port for this provider. When omitted, falls back + to the legacy module-level ``_oauth_port`` global. """ # Fail fast at the authorization boundary in non-interactive contexts # (systemd gateway, cron, background MCP discovery). A cached-but-unusable @@ -559,6 +565,8 @@ async def _redirect_handler(authorization_url: str) -> None: "session is available (non-interactive/background context)." ) + effective_port = port if port is not None else _oauth_port + msg = ( f"\n MCP OAuth: authorization required.\n" f" Open this URL in your browser:\n\n" @@ -572,10 +580,10 @@ async def _redirect_handler(authorization_url: str) -> None: # opened. Two ways out: paste the redirect URL back (default fallback, # offered by _wait_for_callback on interactive TTYs), or set up an SSH # port forward so the redirect tunnels through. - if _oauth_port and (os.getenv("SSH_CLIENT") or os.getenv("SSH_TTY")): + if effective_port and (os.getenv("SSH_CLIENT") or os.getenv("SSH_TTY")): print( f" Remote session detected. After you authorize, the provider redirects to\n" - f" http://127.0.0.1:{_oauth_port}/callback\n" + f" http://127.0.0.1:{effective_port}/callback\n" f" which only the listener on THIS machine can receive. Two options:\n" f"\n" f" 1. Easiest — when your browser shows a connection error after\n" @@ -584,7 +592,7 @@ async def _redirect_handler(authorization_url: str) -> None: f" enough to complete the flow.\n" f"\n" f" 2. Or forward the port first in a separate terminal:\n" - f" ssh -N -L {_oauth_port}:127.0.0.1:{_oauth_port} @\n" + f" ssh -N -L {effective_port}:127.0.0.1:{effective_port} @\n" f" then open the URL above and let it redirect normally.\n" f"\n" f" See: https://hermes-agent.nousresearch.com/docs/guides/oauth-over-ssh\n", @@ -604,12 +612,10 @@ async def _redirect_handler(authorization_url: str) -> None: print(" (Headless environment detected — open the URL manually.)\n", file=sys.stderr) -async def _wait_for_callback() -> tuple[str, str | None]: +async def _wait_for_callback(*, port: int | None = None) -> tuple[str, str | None]: """Wait for the OAuth callback to arrive on the local callback server. - Uses the module-level ``_oauth_port`` which is set by ``build_oauth_auth`` - before this is ever called. Polls for the result without blocking the - event loop. + Polls for the result without blocking the event loop. On an interactive TTY, races the HTTP listener against a stdin paste fallback so users without an SSH tunnel can copy the redirect URL (or @@ -617,14 +623,18 @@ async def _wait_for_callback() -> tuple[str, str | None]: machine and paste it back. The HTTP listener wins when the redirect reaches it first; the paste fallback wins when it doesn't. + Args: + port: Callback port for this provider. When omitted, falls back + to the legacy module-level ``_oauth_port`` global. + Raises: OAuthNonInteractiveError: If the callback times out (no user present to complete the browser auth). - RuntimeError: If ``_oauth_port`` has not been set, which would indicate - that ``build_oauth_auth`` was skipped — the asserting form below - was a silent bug when running Python with ``-O``/``-OO``. + RuntimeError: If neither ``port`` nor the global ``_oauth_port`` is + set, which would indicate that ``build_oauth_auth`` was skipped. """ - if _oauth_port is None: + effective_port = port if port is not None else _oauth_port + if effective_port is None: raise RuntimeError( "OAuth callback port not set — build_oauth_auth must be called " "before _wait_for_oauth_callback" @@ -652,7 +662,7 @@ async def _wait_for_callback() -> tuple[str, str | None]: # Start a temporary server on the known port try: - server = HTTPServer(("127.0.0.1", _oauth_port), handler_cls) + server = HTTPServer(("127.0.0.1", effective_port), handler_cls) except OSError: # Port already in use — the server from build_oauth_auth is running. # Fall back to polling the server started by build_oauth_auth. @@ -934,15 +944,21 @@ def build_oauth_auth( "initial authorization, then cached tokens will be reused." ) - _configure_callback_port(cfg) + resolved_port = _configure_callback_port(cfg) client_metadata = _build_client_metadata(cfg) _maybe_preregister_client(storage, cfg, client_metadata) + async def scoped_redirect(url: str) -> None: + await _redirect_handler(url, port=resolved_port) + + async def scoped_callback() -> tuple[str, str | None]: + return await _wait_for_callback(port=resolved_port) + return OAuthClientProvider( server_url=server_url, client_metadata=client_metadata, storage=storage, - redirect_handler=_redirect_handler, - callback_handler=_wait_for_callback, + redirect_handler=scoped_redirect, + callback_handler=scoped_callback, timeout=float(cfg.get("timeout", 300)), ) diff --git a/tools/mcp_oauth_manager.py b/tools/mcp_oauth_manager.py index 8fe1c66f865d..ccebb7deb0a3 100644 --- a/tools/mcp_oauth_manager.py +++ b/tools/mcp_oauth_manager.py @@ -541,18 +541,24 @@ def _build_provider( "authorization." ) - _configure_callback_port(cfg) + resolved_port = _configure_callback_port(cfg) client_metadata = _build_client_metadata(cfg) _maybe_preregister_client(storage, cfg, client_metadata) + async def scoped_redirect(url: str) -> None: + await _redirect_handler(url, port=resolved_port) + + async def scoped_callback() -> tuple[str, str | None]: + return await _wait_for_callback(port=resolved_port) + return _HERMES_PROVIDER_CLS( server_name=server_name, preregistered=bool(cfg.get("client_id")), server_url=entry.server_url, client_metadata=client_metadata, storage=storage, - redirect_handler=_redirect_handler, - callback_handler=_wait_for_callback, + redirect_handler=scoped_redirect, + callback_handler=scoped_callback, timeout=float(cfg.get("timeout", 300)), )