MCP OAuth callback port conflicts and global state issues
NousResearch/hermes-agent · cluster 5344 · agent
Data as of
Nothing open on GitHub. Fix merged in #58154. Check it covers your case.
The short answer
0open issues
0open PRs
18items grouped
- Known?
- Yes. 18 related items grouped here, 6 issues already closed.
- Fix event
- A fix was merged to main in PR #58154. Check it covers your case. No release data is tracked.
- Tracker
- The maintainers' tracker marks this
partially resolved, deciding PRs #58154, #65622, #65664. That is separate from the fix event. - Still open
- Nothing open on GitHub.
These issues and fixes address port collision errors, shared global state pollution, and address-reuse problems in MCP OAuth callback handling. Additional concerns include fail-fast behavior for headless/non-interactive environments with stale or expired tokens.
Open issues0
Open PRs0
Fixes on record
-
#58154 fix(mcp): fail fast at OAuth redirect/callback boundary when non-interactive (#57836)
files:
tests/tools/test_mcp_oauth.py,tools/mcp_oauth.pyShow the diff
-
tests/tools/test_mcp_oauth.py@@ -262,6 +262,7 @@ def _run(self, coro): def test_ssh_hint_shown_on_ssh_session(self, monkeypatch, capsys): import tools.mcp_oauth as mco monkeypatch.setattr(mco, "_oauth_port", 49200) + monkeypatch.setattr(mco, "_is_interactive", lambda: True) monkeypatch.setenv("SSH_CLIENT", "1.2.3.4 1234 22") monkeypatch.delenv("SSH_TTY", raising=False) monkeypatch.setattr(mco, "_can_open_browser", lambda: False) @@ -276,6 +277,7 @@ def test_ssh_hint_shown_on_ssh_session(self, monkeypatch, capsys): def test_ssh_hint_shown_via_ssh_tty(self, monkeypatch, capsys): import tools.mcp_oauth as mco monkeypatch.setattr(mco, "_oauth_port", 49201) + monkeypatch.setattr(mco, "_is_interactive", lambda: True) monkeypatch.delenv("SSH_CLIENT", raising=False) monkeypatch.setenv("SSH_TTY", "/dev/pts/1") monkeypatch.setattr(mco, "_can_open_browser", lambda: False) @@ -289,6 +291,7 @@ def test_ssh_hint_shown_via_ssh_tty(self, monkeypatch, capsys): def test_no_ssh_hint_on_local_session(self, monkeypatch, capsys): import tools.mcp_oauth as mco monkeypatch.setattr(mco, "_oauth_port", 49202) + monkeypatch.setattr(mco, "_is_interactive", lambda: True) monkeypatch.delenv("SSH_CLIENT", raising=False) monkeypatch.delenv("SSH_TTY", raising=False) monkeypatch.setattr(mco, "_can_open_browser", lambda: True) @@ -302,6 +305,7 @@ def test_no_ssh_hint_on_local_session(self, monkeypatch, capsys): def test_no_ssh_hint_when_port_not_set(self, monkeypatch, capsys): import tools.mcp_oauth as mco monkeypatch.setattr(mco, "_oauth_port", None) + monkeypatch.setattr(mco, "_is_interactive", lambda: True) monkeypatch.setenv("SSH_CLIENT", "1.2.3.4 1234 22") monkeypatch.setattr(mco, "_can_open_browser", lambda: False) @@ -527,12 +531,19 @@ def _discovery(): class TestWaitForCallbackNoBlocking: """_wait_for_callback() must never call input() — it raises instead.""" - def test_raises_on_timeout_instead_of_input(self): - """When no auth code arrives, raises OAuthNonInteractiveError.""" + def test_raises_on_timeout_instead_of_input(self, monkeypatch): + """Interactive session: when no auth code arrives, raises on timeout. + + Marked interactive so the fail-fast non-interactive guard (#57836) + does not short-circuit — this test exercises the timeout path. + """ import tools.mcp_oauth as mod import asyncio mod._oauth_port = _find_free_port() + monkeypatch.setattr(mod, "_is_interactive", lambda: True) + # EOF on the paste reader so only the HTTP-listener timeout drives it. + monkeypatch.setattr("sys.stdin", MagicMock(readline=lambda: "")) async def instant_sleep(_seconds): pass @@ -583,6 +594,116 @@ def test_noninteractive_with_cached_tokens_no_warning(self, tmp_path, monkeypatc assert "no cached tokens found" not in caplog.text.lower() +class TestNonInteractiveFailFastAtCallbackBoundary: + """#57836: a cached-but-unusable token (expired/revoked, refresh rejected) + makes the MCP SDK fall through to the authorization-code flow even though + build_oauth_auth's token-file guard passed. In a non-interactive context + (systemd gateway, cron, background discovery) that flow must fail fast at + the redirect/callback boundary — never launch a browser flow or bind a + callback listener, and never block for the full timeout — so gateway + startup is not gated on an unusable optional MCP server, and retries do not + collide on the callback port ('Address already in use'). + """ + + def test_wait_for_callback_rejects_before_binding_when_noninteractive(self, monkeypatch): + """No listener bound and no poll loop entered when non-interactive.""" + import tools.mcp_oauth as mod + import asyncio + + mod._oauth_port = _find_free_port() + monkeypatch.setattr(mod, "_is_interactive", lambda: False) + + # Binding the callback listener or entering the poll loop is the bug. + fake_server = MagicMock(side_effect=AssertionError("must not bind callback listener")) + monkeypatch.setattr(mod, "HTTPServer", fake_server) + + async def no_sleep(_seconds): + raise AssertionError("must not wait for the callback timeout") + monkeypatch.setattr(mod.asyncio, "sleep", no_sleep) + + with pytest.raises(OAuthNonInteractiveError, match="interactive session"): + asyncio.run(mod._wait_for_callback()) + fake_server.assert_not_called() + + def test_wait_for_callback_fail_fast_holds_even_with_cached_token_file(self, tmp_path, monkeypatch): + """Guard does not depend on token-file existence. + + A stale token file on disk passes build_oauth_auth's guard, so the + callback boundary is the only place that can reject the flow. + """ + import tools.mcp_oauth as mod + import asyncio + + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + d = tmp_path / "mcp-tokens" + d.mkdir(parents=True) + (d / "example.json").write_text( + json.dumps({"access_token": "stale", "token_type": "Bearer"}) + ) + + mod._oauth_port = _find_free_port() + monkeypatch.setattr(mod, "_is_interactive", lambda: False) + monkeypatch.setattr( + mod, "HTTPServer", MagicMock(side_effect=AssertionError("must not bind")) + ) + + with pytest.raises(OAuthNonInteractiveError): + asyncio.run(mod._wait_for_callback()) + + def test_redirect_handler_rejects_and_does_not_open_browser(self, monkeypatch, capsys): + """Non-interactive redirect must not print an auth URL or open a browser.""" + import tools.mcp_oauth as mod + import asyncio + + monkeypatch.setattr(mod, "_is_interactive", lambda: False) + monkeypatch.setattr( + "webbrowser.open", MagicMock(side_effect=AssertionError("must not open browser")) + ) + + with pytest.raises(OAuthNonInteractiveError, match="browser authorization"): + asyncio.run(mod._redirect_handler("https://idp.example.com/authorize?x=1")) + + err = capsys.readouterr().err + assert "https://idp.example.com/authorize" not in err + + def test_boundary_errors_point_at_hermes_mcp_login(self, monkeypatch): + """Both boundaries emit an actionable next step.""" + import tools.mcp_oauth as mod + import asyncio + + monkeypatch.setattr(mod, "_is_interactive", lambda: False) + with pytest.raises(OAuthNonInteractiveError, match="hermes mcp login"): + asyncio.run(mod._redirect_handler("https://idp.example.com/authorize")) + + mod._oauth_port = _find_free_port() + with pytest.raises(OAuthNonInteractiveError, match="hermes mcp login"): + asyncio.run(mod._wait_for_callback()) + + def test_guard_does_not_fire_on_interactive_redirect(self, monkeypatch, capsys): + """Positive control: the fail-fast guard is scoped to the auth-code path. + + #57836 regression coverage asks that valid/refreshable OAuth keeps + working non-interactively — a good token never reaches these handlers, + so the guard must be inert once a real flow is in progress. Assert the + interactive path still prints the URL and does not raise, proving the + guard does not over-fire and swallow legitimate authorization. + """ + import tools.mcp_oauth as mod + import asyncio + + monkeypatch.setattr(mod, "_is_interactive", lambda: True) + # Local (non-SSH) interactive session with no browser available, so the + # handler falls through to the manual-URL print without opening a tab. + monkeypatch.delenv("SSH_CLIENT", raising=False) + monkeypatch.delenv("SSH_TTY", raising=False) + monkeypatch.setattr(mod, "_can_open_browser", lambda: False) + + asyncio.run(mod._redirect_handler("https://idp.example.com/authorize?x=9")) + + err = capsys.readouterr().err + assert "https://idp.example.com/authorize?x=9" in err + + # --------------------------------------------------------------------------- # Extracted helper tests (Task 3 of MCP OAuth consolidation) # --------------------------------------------------------------------------- -
tools/mcp_oauth.py@@ -167,6 +167,21 @@ def _is_interactive() -> bool: return False +def _raise_if_non_interactive(lead: str) -> None: + """Raise ``OAuthNonInteractiveError`` unless an interactive session exists. + + ``lead`` is the boundary-specific first sentence; this helper appends the + shared, actionable ``hermes mcp login`` next-step so the guidance wording + lives in one place across every non-interactive OAuth boundary (#57836). + """ + if not _is_interactive(): + raise OAuthNonInteractiveError( + f"{lead} " + "Run `hermes mcp login <server>` interactively to (re)authorize, " + "then restart or reload the gateway." + ) + + @contextmanager def force_interactive_oauth(): """Treat the current execution context as interactive despite no TTY. @@ -529,6 +544,21 @@ async def _redirect_handler(authorization_url: str) -> None: Opens the browser automatically when possible; always prints the URL as a fallback for headless/SSH/gateway environments. """ + # Fail fast at the authorization boundary in non-interactive contexts + # (systemd gateway, cron, background MCP discovery). A cached-but-unusable + # token (expired/revoked, refresh rejected) makes the SDK fall through to + # the authorization-code flow even though build_oauth_auth's token-file + # guard passed. Without this check we would print a URL and launch a + # browser flow no operator can complete, then block in _wait_for_callback + # for the full timeout. Raise before launching so gateway adapters start + # promptly and the caller can skip this server with an actionable warning. + # This intentionally re-checks interactivity here rather than trusting the + # token-file existence guard alone. See #57836. + _raise_if_non_interactive( + "MCP OAuth requires browser authorization but no interactive " + "session is available (non-interactive/background context)." + ) + msg = ( f"\n MCP OAuth: authorization required.\n" f" Open this URL in your browser:\n\n" @@ -600,6 +630,22 @@ async def _wait_for_callback() -> tuple[str, str | None]: "before _wait_for_oauth_callback" ) + # Reject before binding the callback listener in non-interactive contexts. + # Reaching here means the SDK entered the authorization-code flow (a valid + # or refreshable token would never call the callback handler), so a cached + # token file is present but unusable. Binding the listener here would block + # for the full 300s timeout and — on the next connection retry — collide + # with the still-bound/TIME_WAIT port, surfacing as + # ``OSError: [Errno 98] Address already in use``. Failing fast keeps + # gateway startup independent of an unusable optional MCP server. This + # guard holds "regardless of whether a token file exists" — the point the + # build_oauth_auth token-file guard cannot cover. See #57836. + _raise_if_non_interactive( + "OAuth callback requires an interactive session but none is " + "available (non-interactive/background context); skipping browser " + "authorization without binding a callback listener." + ) + # The callback server is already running (started in build_oauth_auth). # We just need to poll for the result. handler_cls, result = _make_callback_handler()
-
-
#65622 fix(mcp): close OAuth callback port races — closures, reuse_address, TOCTOU (salvage of #44872 + #22161)
files:
scripts/release.py,tests/tools/test_mcp_oauth.py,tools/mcp_oauth.py,tools/mcp_oauth_manager.pyShow the diff
-
scripts/release.py@@ -346,6 +346,8 @@ "[redacted:email]": "synapsesx", "[redacted:email]": "itsflownium", "[redacted:email]": "dirtyren", + "[redacted:email]": "Code-suphub", + "[redacted:email]": "Code-suphub", "[redacted:email]": "brendandebeasi", "[redacted:email]": "dorokuma", "[redacted:email]": "liuwei666888", -
tests/tools/test_mcp_oauth.py@@ -21,7 +21,7 @@ _is_interactive, _wait_for_callback, _make_callback_handler, - _redirect_handler, + _make_redirect_handler, _paste_callback_reader, ) @@ -254,20 +254,20 @@ def test_can_open_browser_true_with_display(self, monkeypatch): class TestRedirectHandlerSshHint: - """_redirect_handler must print an SSH tunnel hint on remote sessions.""" + """_make_redirect_handler must print an SSH tunnel hint on remote sessions.""" def _run(self, coro): return asyncio.get_event_loop().run_until_complete(coro) def test_ssh_hint_shown_on_ssh_session(self, monkeypatch, capsys): import tools.mcp_oauth as mco - monkeypatch.setattr(mco, "_oauth_port", 49200) monkeypatch.setattr(mco, "_is_interactive", lambda: True) monkeypatch.setenv("SSH_CLIENT", "1.2.3.4 1234 22") monkeypatch.delenv("SSH_TTY", raising=False) monkeypatch.setattr(mco, "_can_open_browser", lambda: False) - self._run(_redirect_handler("https://example.com/auth?foo=bar")) + handler = _make_redirect_handler(49200) + self._run(handler("https://example.com/auth?foo=bar")) err = capsys.readouterr().err assert "49200" in err @@ -276,13 +276,13 @@ def test_ssh_hint_shown_on_ssh_session(self, monkeypatch, capsys): def test_ssh_hint_shown_via_ssh_tty(self, monkeypatch, capsys): import tools.mcp_oauth as mco - monkeypatch.setattr(mco, "_oauth_port", 49201) monkeypatch.setattr(mco, "_is_interactive", lambda: True) monkeypatch.delenv("SSH_CLIENT", raising=False) monkeypatch.setenv("SSH_TTY", "/dev/pts/1") monkeypatch.setattr(mco, "_can_open_browser", lambda: False) - self._run(_redirect_handler("https://example.com/auth")) + handler = _make_redirect_handler(49201) + self._run(handler("https://example.com/auth")) err = capsys.readouterr().err assert "49201" in err @@ -290,26 +290,26 @@ def test_ssh_hint_shown_via_ssh_tty(self, monkeypatch, capsys): def test_no_ssh_hint_on_local_session(self, monkeypatch, capsys): import tools.mcp_oauth as mco - monkeypatch.setattr(mco, "_oauth_port", 49202) monkeypatch.setattr(mco, "_is_interactive", lambda: True) monkeypatch.delenv("SSH_CLIENT", raising=False) monkeypatch.delenv("SSH_TTY", raising=False) monkeypatch.setattr(mco, "_can_open_browser", lambda: True) monkeypatch.setattr("webbrowser.open", lambda url, **kw: True) - self._run(_redirect_handler("https://example.com/auth")) + handler = _make_redirect_handler(49202) + self._run(handler("https://example.com/auth")) err = capsys.readouterr().err assert "ssh -N -L" not in err - def test_no_ssh_hint_when_port_not_set(self, monkeypatch, capsys): + def test_no_ssh_hint_when_port_is_zero(self, monkeypatch, capsys): import tools.mcp_oauth as mco - monkeypatch.setattr(mco, "_oauth_port", None) monkeypatch.setattr(mco, "_is_interactive", lambda: True) monkeypatch.setenv("SSH_CLIENT", "1.2.3.4 1234 22") monkeypatch.setattr(mco, "_can_open_browser", lambda: False) - self._run(_redirect_handler("https://example.com/auth")) + handler = _make_redirect_handler(0) + self._run(handler("https://example.com/auth")) err = capsys.readouterr().err assert "ssh -N -L" not in err @@ -423,6 +423,103 @@ def test_port_stored_globally(self, tmp_path, monkeypatch): assert 1024 <= mod._oauth_port <= 65535 +# --------------------------------------------------------------------------- +# TOCTOU port reservation (#22161) +# --------------------------------------------------------------------------- + +class TestCallbackPortReservation: + """The socket picked at selection time stays bound until callback bind. + + _find_free_port() closed its probe socket before HTTPServer re-bound the + port, leaving a race window where another process could steal it + (#22161). _reserve_callback_port() keeps the bound socket parked in + _reserved_sockets until _wait_for_callback adopts it. + """ + + def test_reserved_port_cannot_be_stolen(self): + import socket as sock + import tools.mcp_oauth as mod + + port = mod._reserve_callback_port() + try: + # The reservation holds the bind — a competing bind must fail. + thief = sock.socket(sock.AF_INET, sock.SOCK_STREAM) + with pytest.raises(OSError): + thief.bind(("127.0.0.1", port)) + thief.close() + finally: + reserved = mod._reserved_sockets.pop(port, None) + if reserved is not None: + reserved.close() + + def test_configure_callback_port_reserves_ephemeral(self): + import tools.mcp_oauth as mod + + cfg: dict = {} + port = mod._configure_callback_port(cfg) + try: + assert cfg["_resolved_port"] == port + assert port in mod._reserved_sockets + finally: + reserved = mod._reserved_sockets.pop(port, None) + if reserved is not None: + reserved.close() + + def test_pinned_port_is_not_reserved(self): + import tools.mcp_oauth as mod + + cfg: dict = {"redirect_port": 49399} + port = mod._configure_callback_port(cfg) + assert port == 49399 + assert 49399 not in mod._reserved_sockets + + def test_reservation_pool_is_bounded(self): + import tools.mcp_oauth as mod + + ports = [mod._reserve_callback_port() for _ in range(mod._MAX_RESERVED_SOCKETS + 3)] + try: + assert len(mod._reserved_sockets) <= mod._MAX_RESERVED_SOCKETS + # newest reservations survive + assert ports[-1] in mod._reserved_sockets + finally: + for p in list(mod._reserved_sockets): + mod._reserved_sockets.pop(p).close() + + def test_wait_for_callback_adopts_reserved_socket(self, monkeypatch): + """E2E: reserve → _wait_for_callback binds the SAME socket and the + callback round-trips through it.""" + import asyncio + import threading + import urllib.request + import tools.mcp_oauth as mod + + cfg: dict = {} + port = mod._configure_callback_port(cfg) + monkeypatch.setattr(mod, "_is_interactive", lambda: False) + # Bypass the non-interactive guard — this test drives the flow directly. + monkeypatch.setattr(mod, "_raise_if_non_interactive", lambda lead: None) + + async def drive(): + task = asyncio.create_task(mod._wait_for_callback()) + await asyncio.sleep(0.2) # let the server adopt the socket + + def hit(): + urllib.request.urlopen( + f"http://127.0.0.1:{port}/callback?code=abc123&state=xyz", + timeout=5, + ) + + t = threading.Thread(target=hit, daemon=True) + t.start() + return await asyncio.wait_for(task, timeout=10) + + code, state = asyncio.run(drive()) + assert code == "abc123" + assert state == "xyz" + # Reservation was consumed by adoption. + assert port not in mod._reserved_sockets + + # --------------------------------------------------------------------------- # remove_oauth_tokens # --------------------------------------------------------------------------- @@ -661,7 +758,7 @@ def test_redirect_handler_rejects_and_does_not_open_browser(self, monkeypatch, c ) with pytest.raises(OAuthNonInteractiveError, match="browser authorization"): - asyncio.run(mod._redirect_handler("https://idp.example.com/authorize?x=1")) + asyncio.run(mod._make_redirect_handler(49300)("https://idp.example.com/authorize?x=1")) err = capsys.readouterr().err assert "https://idp.example.com/authorize" not in err @@ -673,7 +770,7 @@ def test_boundary_errors_point_at_hermes_mcp_login(self, monkeypatch): monkeypatch.setattr(mod, "_is_interactive", lambda: False) with pytest.raises(OAuthNonInteractiveError, match="hermes mcp login"): - asyncio.run(mod._redirect_handler("https://idp.example.com/authorize")) + asyncio.run(mod._make_redirect_handler(49301)("https://idp.example.com/authorize")) mod._oauth_port = _find_free_port() with pytest.raises(OAuthNonInteractiveError, match="hermes mcp login"): @@ -698,7 +795,7 @@ def test_guard_does_not_fire_on_interactive_redirect(self, monkeypatch, capsys): monkeypatch.delenv("SSH_TTY", raising=False) monkeypatch.setattr(mod, "_can_open_browser", lambda: False) - asyncio.run(mod._redirect_handler("https://idp.example.com/authorize?x=9")) + asyncio.run(mod._make_redirect_handler(49302)("https://idp.example.com/authorize?x=9")) err = capsys.readouterr().err assert "https://idp.example.com/authorize?x=9" in err -
tools/mcp_oauth.py@@ -155,6 +155,43 @@ def _find_free_port() -> int: return s.getsockname()[1] +# Bound-but-not-listening sockets reserved for pending OAuth callback flows, +# keyed by port. Holding the socket from port-selection time until +# _wait_for_callback adopts it closes the TOCTOU window where another process +# could grab the port between _find_free_port() closing its probe socket and +# HTTPServer binding minutes later (#22161). Bounded FIFO so repeated +# build_oauth_auth calls (reconnect loops) cannot leak fds. +_reserved_sockets: "dict[int, socket.socket]" = {} +_MAX_RESERVED_SOCKETS = 8 + + +def _reserve_callback_port() -> int: + """Pick an ephemeral callback port and keep its socket bound. + + Returns the port. The bound (not yet listening) socket is parked in + ``_reserved_sockets`` so no other process can bind the port before + ``_wait_for_callback`` adopts it. Adoption (or ``server_close``) owns + the socket's lifetime from there. + """ + s = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + try: + s.bind(("127.0.0.1", 0)) + except OSError: + s.close() + raise + port = s.getsockname()[1] + # Evict oldest reservations past the cap (dict preserves insertion order). + while len(_reserved_sockets) >= _MAX_RESERVED_SOCKETS: + _, stale = next(iter(_reserved_sockets.items())) + _reserved_sockets.pop(next(iter(_reserved_sockets)), None) + try: + stale.close() + except OSError: + pass + _reserved_sockets[port] = s + return port + + def _is_interactive() -> bool: """Return True if we can reasonably expect to interact with a user.""" if not _oauth_interactive_enabled.get(): @@ -538,70 +575,79 @@ def log_message(self, fmt: str, *args: Any) -> None: # --------------------------------------------------------------------------- -async def _redirect_handler(authorization_url: str) -> None: - """Show the authorization URL to the user. +def _make_redirect_handler(port: int): + """Return a redirect handler closure that closes over the given port. - Opens the browser automatically when possible; always prints the URL - as a fallback for headless/SSH/gateway environments. + Using a closure instead of reading the module-level ``_oauth_port`` avoids + cross-server state pollution when multiple MCP servers run OAuth + concurrently (fixes #44588). """ - # Fail fast at the authorization boundary in non-interactive contexts - # (systemd gateway, cron, background MCP discovery). A cached-but-unusable - # token (expired/revoked, refresh rejected) makes the SDK fall through to - # the authorization-code flow even though build_oauth_auth's token-file - # guard passed. Without this check we would print a URL and launch a - # browser flow no operator can complete, then block in _wait_for_callback - # for the full timeout. Raise before launching so gateway adapters start - # promptly and the caller can skip this server with an actionable warning. - # This intentionally re-checks interactivity here rather than trusting the - # token-file existence guard alone. See #57836. - _raise_if_non_interactive( - "MCP OAuth requires browser authorization but no interactive " - "session is available (non-interactive/background context)." - ) + async def _redirect_handler(authorization_url: str) -> None: + """Show the authorization URL to the user. - msg = ( - f"\n MCP OAuth: authorization required.\n" - f" Open this URL in your browser:\n\n" - f" {authorization_url}\n" - ) - print(msg, file=sys.stderr) - - # On a remote SSH session the OAuth provider redirects to - # http://127.0.0.1:<port>/callback, which reaches the callback server on - # the *remote* machine — not the user's local machine where the browser - # 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")): - print( - f" Remote session detected. After you authorize, the provider redirects to\n" - f" http://127.0.0.1:{_oauth_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" - f" authorizing, copy the full URL from the address bar and paste\n" - f" it at the prompt below. The pasted ``code=...&state=...`` is\n" - 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" 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", - file=sys.stderr, + Opens the browser automatically when possible; always prints the URL + as a fallback for headless/SSH/gateway environments. + """ + # Fail fast at the authorization boundary in non-interactive contexts + # (systemd gateway, cron, background MCP discovery). A cached-but-unusable + # token (expired/revoked, refresh rejected) makes the SDK fall through to + # the authorization-code flow even though build_oauth_auth's token-file + # guard passed. Without this check we would print a URL and launch a + # browser flow no operator can complete, then block in _wait_for_callback + # for the full timeout. Raise before launching so gateway adapters start + # promptly and the caller can skip this server with an actionable warning. + # This intentionally re-checks interactivity here rather than trusting the + # token-file existence guard alone. See #57836. + _raise_if_non_interactive( + "MCP OAuth requires browser authorization but no interactive " + "session is available (non-interactive/background context)." ) - if _can_open_browser(): - try: - opened = webbrowser.open(authorization_url) - if opened: - print(" (Browser opened automatically.)\n", file=sys.stderr) - else: + msg = ( + f"\n MCP OAuth: authorization required.\n" + f" Open this URL in your browser:\n\n" + f" {authorization_url}\n" + ) + print(msg, file=sys.stderr) + + # On a remote SSH session the OAuth provider redirects to + # http://127.0.0.1:<port>/callback, which reaches the callback server on + # the *remote* machine — not the user's local machine where the browser + # 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 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:{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" + f" authorizing, copy the full URL from the address bar and paste\n" + f" it at the prompt below. The pasted ``code=...&state=...`` is\n" + 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 {port}:127.0.0.1:{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", + file=sys.stderr, + ) + + if _can_open_browser(): + try: + opened = webbrowser.open(authorization_url) + if opened: + print(" (Browser opened automatically.)\n", file=sys.stderr) + else: + print(" (Could not open browser — please open the URL manually.)\n", file=sys.stderr) + except Exception: print(" (Could not open browser — please open the URL manually.)\n", file=sys.stderr) - except Exception: - print(" (Could not open browser — please open the URL manually.)\n", file=sys.stderr) - else: - print(" (Headless environment detected — open the URL manually.)\n", file=sys.stderr) + else: + print(" (Headless environment detected — open the URL manually.)\n", file=sys.stderr) + + return _redirect_handler async def _wait_for_callback() -> tuple[str, str | None]: @@ -650,9 +696,29 @@ async def _wait_for_callback() -> tuple[str, str | None]: # We just need to poll for the result. handler_cls, result = _make_callback_handler() - # Start a temporary server on the known port + # Start a temporary server on the known port, adopting the socket + # reserved at port-selection time when one exists. Holding the bound + # socket from _reserve_callback_port() until here closes the TOCTOU + # window where another process could steal the port between selection + # and bind (#22161). allow_reuse_address is set BEFORE binding (setting + # it after the constructor has already bound is a no-op) so a lingering + # TIME_WAIT socket from a previous flow cannot block the next one + # (#44590). try: - server = HTTPServer(("127.0.0.1", _oauth_port), handler_cls) + server = HTTPServer( + ("127.0.0.1", _oauth_port), handler_cls, bind_and_activate=False + ) + reserved = _reserved_sockets.pop(_oauth_port, None) + if reserved is not None: + # Adopt the reserved (already bound) socket and start listening. + server.socket.close() + server.socket = reserved + server.server_address = reserved.getsockname() + server.server_activate() + else: + server.allow_reuse_address = True + server.server_bind() + server.server_activate() 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. @@ -827,7 +893,10 @@ def _configure_callback_port(cfg: dict) -> int: """ global _oauth_port requested = int(cfg.get("redirect_port", 0)) - port = _find_free_port() if requested == 0 else requested + # Ephemeral selection reserves the bound socket until _wait_for_callback + # adopts it, closing the select→bind TOCTOU race (#22161). An explicit + # user-pinned port is used as-is. + port = _reserve_callback_port() if requested == 0 else requested cfg["_resolved_port"] = port _oauth_port = port # legacy consumer: _wait_for_callback reads this return port @@ -938,11 +1007,15 @@ def build_oauth_auth( client_metadata = _build_client_metadata(cfg) _maybe_preregister_client(storage, cfg, client_metadata) + # Use closure factories to avoid global state pollution (#44588). + resolved_port = cfg.get("_resolved_port", _oauth_port) + redirect_handler = _make_redirect_handler(resolved_port) + return OAuthClientProvider( server_url=server_url, client_metadata=client_metadata, storage=storage, - redirect_handler=_redirect_handler, + redirect_handler=redirect_handler, callback_handler=_wait_for_callback, timeout=float(cfg.get("timeout", 300)), ) -
tools/mcp_oauth_manager.py@@ -522,7 +522,7 @@ def _build_provider( _configure_callback_port, _is_interactive, _maybe_preregister_client, - _redirect_handler, + _make_redirect_handler, _wait_for_callback, ) @@ -545,13 +545,16 @@ def _build_provider( client_metadata = _build_client_metadata(cfg) _maybe_preregister_client(storage, cfg, client_metadata) + resolved_port = cfg.get("_resolved_port", 0) + redirect_handler = _make_redirect_handler(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, + redirect_handler=redirect_handler, callback_handler=_wait_for_callback, timeout=float(cfg.get("timeout", 300)), )
-
-
#65664 fix(mcp): per-flow OAuth callback waiters + clear port-in-use error (salvage of #50786, direction of #34280)
files:
scripts/release.py,tests/tools/test_mcp_oauth.py,tools/mcp_oauth.py,tools/mcp_oauth_manager.pyShow the diff
-
scripts/release.py@@ -353,6 +353,7 @@ "[redacted:email]": "synapsesx", "[redacted:email]": "itsflownium", "[redacted:email]": "dirtyren", + "[redacted:email]": "doxe0x", "[redacted:email]": "Code-suphub", "[redacted:email]": "Code-suphub", "[redacted:email]": "shuangxinniao", -
tests/tools/test_mcp_oauth.py@@ -561,6 +561,54 @@ def hit(): # Reservation was consumed by adoption. assert port not in mod._reserved_sockets + def test_concurrent_flows_keep_their_own_callback_ports(self, monkeypatch): + """#34260: flow A's waiter listens on A's port even after flow B + overwrites the legacy module-level global. + + This is the callback-side sibling of the #44588 redirect-handler fix: + without a per-flow waiter, A's callback wait would bind B's port and + A's redirect (pointing at A's port) would never be received. + """ + import asyncio + import threading + import urllib.request + import tools.mcp_oauth as mod + + monkeypatch.setattr(mod, "_is_interactive", lambda: False) + monkeypatch.setattr(mod, "_raise_if_non_interactive", lambda lead: None) + + cfg_a: dict = {} + port_a = mod._configure_callback_port(cfg_a) + waiter_a = mod._make_callback_waiter(port_a) + # Flow B configures afterwards — overwrites mod._oauth_port. + cfg_b: dict = {} + port_b = mod._configure_callback_port(cfg_b) + assert mod._oauth_port == port_b != port_a + + async def drive(): + task = asyncio.create_task(waiter_a()) + await asyncio.sleep(0.2) + + def hit(): + # The redirect goes to flow A's port — where A's waiter + # must be listening despite the clobbered global. + urllib.request.urlopen( + f"http://127.0.0.1:{port_a}/callback?code=flowA&state=sA", + timeout=5, + ) + + threading.Thread(target=hit, daemon=True).start() + return await asyncio.wait_for(task, timeout=10) + + try: + code, state = asyncio.run(drive()) + finally: + leftover = mod._reserved_sockets.pop(port_b, None) + if leftover is not None: + leftover.close() + assert code == "flowA" + assert state == "sA" + # --------------------------------------------------------------------------- # remove_oauth_tokens @@ -1470,3 +1518,22 @@ def test_poison_noop_when_no_client_file(self, tmp_path, monkeypatch): monkeypatch.setenv("HERMES_HOME", str(tmp_path)) storage = HermesTokenStorage("srv") assert storage.poison_client_registration() is False + + +def test_wait_for_callback_port_in_use_reports_clear_error(monkeypatch): + """A busy loopback callback port surfaces a clear 'already in use' error, + not a misleading 'timed out'. Guards the stale-comment fix where the branch + also wrongly claimed build_oauth_auth had started a server to poll.""" + import tools.mcp_oauth as mo + + monkeypatch.setattr(mo, "_is_interactive", lambda: True) + with patch.object(mo, "_oauth_port", 54321), patch.object( + mo, "HTTPServer", side_effect=OSError("address already in use") + ): + with pytest.raises(mo.OAuthNonInteractiveError) as excinfo: + asyncio.run(mo._wait_for_callback()) + + msg = str(excinfo.value) + assert "54321" in msg + assert "already in use" in msg + assert "timed out" not in msg -
tools/mcp_oauth.py@@ -668,21 +668,14 @@ async def _redirect_handler(authorization_url: str) -> None: async def _wait_for_callback() -> tuple[str, str | None]: - """Wait for the OAuth callback to arrive on the local callback server. + """Wait for the OAuth callback on the legacy module-level port. - 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. - - 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. + Kept for backwards compatibility with callers that never went through + :func:`build_oauth_auth`'s per-flow wiring. New code paths receive a + per-flow waiter from :func:`_make_callback_waiter` so concurrent OAuth + flows cannot cross ports (#34260). 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``. @@ -692,102 +685,129 @@ async def _wait_for_callback() -> tuple[str, str | None]: "OAuth callback port not set — build_oauth_auth must be called " "before _wait_for_oauth_callback" ) + return await _make_callback_waiter(_oauth_port)() - # Reject before binding the callback listener in non-interactive contexts. - # Reaching here means the SDK entered the authorization-code flow (a valid - # or refreshable token would never call the callback handler), so a cached - # token file is present but unusable. Binding the listener here would block - # for the full 300s timeout and — on the next connection retry — collide - # with the still-bound/TIME_WAIT port, surfacing as - # ``OSError: [Errno 98] Address already in use``. Failing fast keeps - # gateway startup independent of an unusable optional MCP server. This - # guard holds "regardless of whether a token file exists" — the point the - # build_oauth_auth token-file guard cannot cover. See #57836. - _raise_if_non_interactive( - "OAuth callback requires an interactive session but none is " - "available (non-interactive/background context); skipping browser " - "authorization without binding a callback listener." - ) - # The callback server is already running (started in build_oauth_auth). - # We just need to poll for the result. - handler_cls, result = _make_callback_handler() - - # Start a temporary server on the known port, adopting the socket - # reserved at port-selection time when one exists. Holding the bound - # socket from _reserve_callback_port() until here closes the TOCTOU - # window where another process could steal the port between selection - # and bind (#22161). allow_reuse_address is set BEFORE binding (setting - # it after the constructor has already bound is a no-op) so a lingering - # TIME_WAIT socket from a previous flow cannot block the next one - # (#44590). - try: - server = HTTPServer( - ("127.0.0.1", _oauth_port), handler_cls, bind_and_activate=False - ) - reserved = _reserved_sockets.pop(_oauth_port, None) - if reserved is not None: - # Adopt the reserved (already bound) socket and start listening. - server.socket.close() - server.socket = reserved - server.server_address = reserved.getsockname() - server.server_activate() - else: - server.allow_reuse_address = True - server.server_bind() - server.server_activate() - 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. - raise OAuthNonInteractiveError( - "OAuth callback timed out — could not bind callback port. " - "Complete the authorization in a browser first, then retry." - ) +def _make_callback_waiter(port: int): + """Return a callback waiter bound to a single OAuth flow's port. - server_thread = threading.Thread(target=server.handle_request, daemon=True) - server_thread.start() + Closing over the port (instead of reading the module-level + ``_oauth_port``) keeps concurrent OAuth flows isolated: flow A's waiter + listens on flow A's port even when flow B's ``_configure_callback_port`` + overwrites the legacy global afterwards (#34260, the callback-side + sibling of the #44588 redirect-handler fix). - # Optional paste-fallback thread: only on interactive TTYs. Reads one - # line from stdin and writes the parsed code/state into the shared - # result dict. The HTTP listener and this thread race for the result; - # whichever fills it first wins. - paste_thread: threading.Thread | None = None - if _is_interactive(): - print( - "\n Or paste the redirect URL here (or the ``?code=...&state=...`` " - "portion) and press Enter. Type ``skip`` + Enter to continue " - "without this server:", - file=sys.stderr, - flush=True, - ) - paste_thread = threading.Thread( - target=_paste_callback_reader, args=(result,), daemon=True - ) - paste_thread.start() + The waiter polls for the redirect without blocking the event loop. On an + interactive TTY it races the HTTP listener against a stdin paste fallback + so users without an SSH tunnel can paste the redirect URL (or just the + ``code=...&state=...`` query string) from a browser on another machine. - timeout = 300.0 - poll_interval = 0.5 - elapsed = 0.0 - try: - while elapsed < timeout: - if result["auth_code"] is not None or result["error"] is not None: - break - await asyncio.sleep(poll_interval) - elapsed += poll_interval - finally: - server.server_close() + Raises (when awaited): + OAuthNonInteractiveError: If the callback times out (no user present + to complete the browser auth), or in non-interactive contexts. + """ - if result["error"] == _USER_SKIPPED_SENTINEL: - raise OAuthNonInteractiveError("user_skipped") - if result["error"]: - raise RuntimeError(f"OAuth authorization failed: {result['error']}") - if result["auth_code"] is None: - raise OAuthNonInteractiveError( - "OAuth callback timed out — no authorization code received. " - "Ensure you completed the browser authorization flow." + async def _wait() -> tuple[str, str | None]: + # Reject before binding the callback listener in non-interactive + # contexts. Reaching here means the SDK entered the authorization-code + # flow (a valid or refreshable token would never call the callback + # handler), so a cached token file is present but unusable. Binding the + # listener here would block for the full 300s timeout and — on the next + # connection retry — collide with the still-bound/TIME_WAIT port, + # surfacing as ``OSError: [Errno 98] Address already in use``. Failing + # fast keeps gateway startup independent of an unusable optional MCP + # server. This guard holds "regardless of whether a token file exists" + # — the point the build_oauth_auth token-file guard cannot cover. + # See #57836. + _raise_if_non_interactive( + "OAuth callback requires an interactive session but none is " + "available (non-interactive/background context); skipping browser " + "authorization without binding a callback listener." ) - return result["auth_code"], result["state"] + handler_cls, result = _make_callback_handler() + + # Start a temporary server on this flow's port, adopting the socket + # reserved at port-selection time when one exists. Holding the bound + # socket from _reserve_callback_port() until here closes the TOCTOU + # window where another process could steal the port between selection + # and bind (#22161). allow_reuse_address is set BEFORE binding (setting + # it after the constructor has already bound is a no-op) so a lingering + # TIME_WAIT socket from a previous flow cannot block the next one + # (#44590). + try: + server = HTTPServer( + ("127.0.0.1", port), handler_cls, bind_and_activate=False + ) + reserved = _reserved_sockets.pop(port, None) + if reserved is not None: + # Adopt the reserved (already bound) socket and start listening. + server.socket.close() + server.socket = reserved + server.server_address = reserved.getsockname() + server.server_activate() + else: + server.allow_reuse_address = True + server.server_bind() + server.server_activate() + except OSError as exc: + # The loopback callback port is genuinely in use: a concurrent OAuth + # flow, a leftover listener, or a fixed `oauth.redirect_port` that + # collided. build_oauth_auth does not start its own callback server, + # so there is nothing to poll here; surface a clear, actionable error + # instead of a misleading "timed out". + raise OAuthNonInteractiveError( + f"OAuth callback port {port} is already in use ({exc}). " + "Close any other in-progress login, or set a free `oauth.redirect_port` " + "in the server config, then retry." + ) from exc + + server_thread = threading.Thread(target=server.handle_request, daemon=True) + server_thread.start() + + # Optional paste-fallback thread: only on interactive TTYs. Reads one + # line from stdin and writes the parsed code/state into the shared + # result dict. The HTTP listener and this thread race for the result; + # whichever fills it first wins. + paste_thread: threading.Thread | None = None + if _is_interactive(): + print( + "\n Or paste the redirect URL here (or the ``?code=...&state=...`` " + "portion) and press Enter. Type ``skip`` + Enter to continue " + "without this server:", + file=sys.stderr, + flush=True, + ) + paste_thread = threading.Thread( + target=_paste_callback_reader, args=(result,), daemon=True + ) + paste_thread.start() + + timeout = 300.0 + poll_interval = 0.5 + elapsed = 0.0 + try: + while elapsed < timeout: + if result["auth_code"] is not None or result["error"] is not None: + break + await asyncio.sleep(poll_interval) + elapsed += poll_interval + finally: + server.server_close() + + if result["error"] == _USER_SKIPPED_SENTINEL: + raise OAuthNonInteractiveError("user_skipped") + if result["error"]: + raise RuntimeError(f"OAuth authorization failed: {result['error']}") + if result["auth_code"] is None: + raise OAuthNonInteractiveError( + "OAuth callback timed out — no authorization code received. " + "Ensure you completed the browser authorization flow." + ) + + return result["auth_code"], result["state"] + + return _wait def _paste_callback_reader(result: dict) -> None: @@ -1048,17 +1068,18 @@ def build_oauth_auth( client_metadata = _build_client_metadata(cfg) _maybe_preregister_client(storage, cfg, client_metadata) - # Use closure factories to avoid global state pollution (#44588). + # Use closure factories to avoid global state pollution (#44588, #34260). resolved_port = cfg.get("_resolved_port", _oauth_port) redirect_handler = _make_redirect_handler( resolved_port, redirect_uri=cfg.get("redirect_uri") or None ) + callback_handler = _make_callback_waiter(resolved_port) return OAuthClientProvider( server_url=server_url, client_metadata=client_metadata, storage=storage, redirect_handler=redirect_handler, - callback_handler=_wait_for_callback, + callback_handler=callback_handler, timeout=float(cfg.get("timeout", 300)), ) -
tools/mcp_oauth_manager.py@@ -522,8 +522,8 @@ def _build_provider( _configure_callback_port, _is_interactive, _maybe_preregister_client, + _make_callback_waiter, _make_redirect_handler, - _wait_for_callback, ) if not _OAUTH_AVAILABLE: @@ -547,6 +547,7 @@ def _build_provider( resolved_port = cfg.get("_resolved_port", 0) redirect_handler = _make_redirect_handler(resolved_port) + callback_handler = _make_callback_waiter(resolved_port) return _HERMES_PROVIDER_CLS( server_name=server_name, @@ -555,7 +556,7 @@ def _build_provider( client_metadata=client_metadata, storage=storage, redirect_handler=redirect_handler, - callback_handler=_wait_for_callback, + callback_handler=callback_handler, timeout=float(cfg.get("timeout", 300)), )
-
9 PRs closed without merging
From the close comments: done elsewhere 4 · duplicate 2 · merged via another change 2 · other 1
-
#5345 fix(mcp-oauth): per-server callback ports + reuse registered redirect URI · done elsewhere →
#65622,#44607,#44685,#44872 -
#44607 Bind MCP OAuth handlers per provider · done elsewhere →
#65622,#44872,#5345 -
#44611 Reuse MCP OAuth callback listener addresses · done elsewhere →
#65622 -
#44680 fix(mcp): set SO_REUSEADDR on OAuth callback server to prevent EADDRINUSE · duplicate →
#44611 -
#44685 fix(mcp): replace global _oauth_port with per-provider closures · done elsewhere →
#65622,#44872,#5345
Show 4 more
-
#44872 fix(mcp): use per-provider closures and allow_reuse_address for OAuth · merged via another change →
13e19a9,f4c7caa,#65622,#22161 -
#54325 fix(mcp-oauth): eliminate _oauth_port global, defer server binding, restore fail-fast guards · duplicate →
#34260,#34280,#44607,#54325,#44588,#44590,#50786 - #56690 fix(mcp): fail fast on headless expired OAuth tokens · other
-
#58000 fix(mcp): fail fast at OAuth redirect/callback boundary when non-interactive (#57836) · merged via another change →
755194f,0c8441c,#58154,#57836
Closed issues · 6
- Closed#5344MCP OAuth: port collision on concurrent auth + redirect URI mismatch on restart
- Closed#34260MCP OAuth callback: module-level port global causes port collisions and structural weaknesses vs upstream
- Closed#44588OAuth redirect_handler/callback_handler use shared globals, causing cross-server state pollution
- Closed#44590OAuth: ephemeral HTTPServer per flow causes OSError Address already in use
- Closed#56673Headless MCP OAuth reconnect hangs on a no-refresh-token credential and surfaces no needs_reauth state (distinct from #18165's transport recovery)
- Closed#57836[Bug]: Headless MCP OAuth blocks gateway startup with stale cached tokens
How this cluster was grouped
-
Link evidence (
partly structural links): some members are connected by structural evidence (shared changed files, or a closing/superseded reference); the rest are joined by an automated relevance judgement over their text. - Members outside that structural core may belong to a different underlying problem.
-
Pairwise link density
0.12: the share of item pairs that carry a link, counting a structural link in full and a relevance-only link at 0.36.1.00means every pair is linked.
Pick up a Lab task
Check whether a fix covers the open issues here, with your own agent. Bring back evidence for maintainers.
Open the Lab