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
40 changes: 40 additions & 0 deletions tests/tools/test_mcp_oauth.py
Original file line number Diff line number Diff line change
Expand Up @@ -587,6 +587,46 @@ def test_configure_callback_port_uses_explicit_port():
assert cfg["_resolved_port"] == 54321


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.

Expand Down
49 changes: 46 additions & 3 deletions tools/mcp_oauth.py
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,41 @@ def _find_free_port() -> int:
return s.getsockname()[1]


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 []:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please validate the decoded client-info shape and keep URI port extraction inside the malformed-data fallback path. The helper promises None for unusable registrations, but these accesses can raise instead of reaching _find_free_port().

try:
parsed = urlparse(str(uri))
except (TypeError, ValueError):
continue
if (
parsed.scheme == "http"
and parsed.hostname in {"127.0.0.1", "localhost"}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This accepts a cached localhost URI, but the later metadata builder always emits http://127.0.0.1:<port>/callback. That still changes the registered URI on re-auth. Please preserve the cached authority or restrict this branch to canonical 127.0.0.1, and add a localhost regression test.

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."""
try:
Expand Down Expand Up @@ -640,13 +675,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
Expand All @@ -655,7 +698,7 @@ 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
port = requested or _cached_redirect_port(storage) or _find_free_port()
cfg["_resolved_port"] = port
_oauth_port = port # legacy consumer: _wait_for_callback reads this
return port
Expand Down Expand Up @@ -762,7 +805,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)

Expand Down
2 changes: 1 addition & 1 deletion tools/mcp_oauth_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,7 @@ def _build_provider(
"authorization."
)

_configure_callback_port(cfg)
_configure_callback_port(cfg, storage)
client_metadata = _build_client_metadata(cfg)
_maybe_preregister_client(storage, cfg, client_metadata)

Expand Down