From 58010c8b3d09afa86aa4367fec5a96ac11b41de6 Mon Sep 17 00:00:00 2001 From: Tim Roth Date: Wed, 10 Jun 2026 10:40:41 +0200 Subject: [PATCH] fix(mcp): reuse cached oauth redirect port --- tests/tools/test_mcp_oauth.py | 40 +++++++++++++++++++++++ tools/mcp_oauth.py | 60 +++++++++++++++++++++++++++++++---- tools/mcp_oauth_manager.py | 2 +- 3 files changed, 95 insertions(+), 7 deletions(-) diff --git a/tests/tools/test_mcp_oauth.py b/tests/tools/test_mcp_oauth.py index 8034969013b3..faaa93d49a2b 100644 --- a/tests/tools/test_mcp_oauth.py +++ b/tests/tools/test_mcp_oauth.py @@ -1094,6 +1094,46 @@ def test_maybe_preregister_client_redirect_uri_defaults_to_localhost(tmp_path, m ] +def test_configure_callback_port_reuses_cached_client_redirect_port(tmp_path, monkeypatch): + """Cached client registrations must keep using their registered port.""" + from tools.mcp_oauth import _configure_callback_port + + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + storage = HermesTokenStorage("summ") + token_dir = tmp_path / "mcp-tokens" + token_dir.mkdir(parents=True) + (token_dir / "summ.client.json").write_text(json.dumps({ + "client_id": "client-123", + "redirect_uris": ["http://127.0.0.1:57727/callback"], + })) + + cfg = {"redirect_port": 0} + port = _configure_callback_port(cfg, storage) + + assert port == 57727 + assert cfg["_resolved_port"] == 57727 + + +def test_configure_callback_port_explicit_overrides_cached_client_port(tmp_path, monkeypatch): + """Explicit config wins over any cached registration.""" + from tools.mcp_oauth import _configure_callback_port + + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + storage = HermesTokenStorage("summ") + token_dir = tmp_path / "mcp-tokens" + token_dir.mkdir(parents=True) + (token_dir / "summ.client.json").write_text(json.dumps({ + "client_id": "client-123", + "redirect_uris": ["http://127.0.0.1:57727/callback"], + })) + + cfg = {"redirect_port": 54321} + port = _configure_callback_port(cfg, storage) + + assert port == 54321 + assert cfg["_resolved_port"] == 54321 + + def test_build_oauth_auth_preserves_server_url_path(): """server_url with path is forwarded to OAuthClientProvider unmodified. diff --git a/tools/mcp_oauth.py b/tools/mcp_oauth.py index cb8917b28e91..7959852319f1 100644 --- a/tools/mcp_oauth.py +++ b/tools/mcp_oauth.py @@ -194,6 +194,41 @@ def _reserve_callback_port() -> int: return port +def _cached_redirect_port(storage: "HermesTokenStorage | None") -> int | None: + """Return the loopback callback port from cached client registration. + + OAuth providers bind a dynamically-registered ``client_id`` to the exact + redirect URI that was registered with it. If Hermes restarts and chooses a + new random callback port while reusing the stored ``client_id``, providers + such as Summ reject the authorization request with ``redirect_uri does not + match any registered URIs``. Reusing the cached redirect port keeps the + authorization request consistent with the stored client registration. + """ + if storage is None: + return None + + try: + data = _read_json(storage._client_info_path()) + except (AttributeError, TypeError, ValueError): + return None + if not data: + return None + + for uri in data.get("redirect_uris") or []: + try: + parsed = urlparse(str(uri)) + except (TypeError, ValueError): + continue + if ( + parsed.scheme == "http" + and parsed.hostname in {"127.0.0.1", "localhost"} + and parsed.path == "/callback" + and parsed.port is not None + ): + return int(parsed.port) + return None + + def _is_interactive() -> bool: """Return True if we can reasonably expect to interact with a user.""" if not _oauth_interactive_enabled.get(): @@ -915,13 +950,21 @@ def remove_oauth_tokens(server_name: str) -> None: # --------------------------------------------------------------------------- -def _configure_callback_port(cfg: dict) -> int: +def _configure_callback_port( + cfg: dict, + storage: "HermesTokenStorage | None" = None, +) -> int: """Pick or validate the OAuth callback port. Stores the resolved port into ``cfg['_resolved_port']`` so sibling helpers (and the manager) can read it from the same dict. Returns the resolved port. + Port choice precedence: + 1. explicit ``oauth.redirect_port`` config + 2. cached client registration redirect URI port + 3. newly allocated free port + NOTE: also sets the legacy module-level ``_oauth_port`` so existing calls to ``_wait_for_callback`` keep working. The legacy global is the root cause of issue #5344 (port collision on concurrent OAuth @@ -930,10 +973,15 @@ def _configure_callback_port(cfg: dict) -> int: """ global _oauth_port requested = int(cfg.get("redirect_port", 0)) - # 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 + # Precedence: explicit config port → cached client-registration port → + # fresh ephemeral port. The cached port keeps re-auth consistent with the + # redirect URI pinned at dynamic client registration (providers reject a + # mismatched URI). Only a truly fresh ephemeral pick goes through + # _reserve_callback_port(), which keeps the socket bound until + # _wait_for_callback adopts it — closing the select→bind TOCTOU race + # (#22161). Explicit and cached ports are fixed, known values and bind + # via the reuse_address path instead. + port = requested or _cached_redirect_port(storage) or _reserve_callback_port() cfg["_resolved_port"] = port _oauth_port = port # legacy consumer: _wait_for_callback reads this return port @@ -1064,7 +1112,7 @@ def build_oauth_auth( "initial authorization, then cached tokens will be reused." ) - _configure_callback_port(cfg) + _configure_callback_port(cfg, storage) client_metadata = _build_client_metadata(cfg) _maybe_preregister_client(storage, cfg, client_metadata) diff --git a/tools/mcp_oauth_manager.py b/tools/mcp_oauth_manager.py index 757dee1b1a8a..f4a37683ff6a 100644 --- a/tools/mcp_oauth_manager.py +++ b/tools/mcp_oauth_manager.py @@ -541,7 +541,7 @@ class MCPOAuthManager: "authorization." ) - _configure_callback_port(cfg) + _configure_callback_port(cfg, storage) client_metadata = _build_client_metadata(cfg) _maybe_preregister_client(storage, cfg, client_metadata)