mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-22 16:25:58 +00:00
fix(mcp): reuse cached oauth redirect port
This commit is contained in:
parent
b5bd0ef38b
commit
58010c8b3d
3 changed files with 95 additions and 7 deletions
|
|
@ -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.
|
||||
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue