Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions tests/tools/test_mcp_oauth.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
# ---------------------------------------------------------------------------
Expand Down
48 changes: 32 additions & 16 deletions tools/mcp_oauth.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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"
Expand All @@ -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"
Expand All @@ -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} <user>@<this-host>\n"
f" ssh -N -L {effective_port}:127.0.0.1:{effective_port} <user>@<this-host>\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",
Expand All @@ -604,27 +612,29 @@ 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
just the ``code=...&state=...`` query string) from a browser on another
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"
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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)),
)
12 changes: 9 additions & 3 deletions tools/mcp_oauth_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)),
)

Expand Down