hermes-admin/hermes-agent
1
0
Fork 0
Code Issues Pull Requests Actions 29 Packages Projects Releases Wiki Activity

fix(mcp-oauth): return a verdict from disk-pair install, count a missing expiry as live

Browse Source 
Two defects in the peer-adoption path of the refresh fence:

1. `_hermes_install_disk_pair` raised `_RefreshCompletedByPeer` when issuer
   binding stripped the candidate's refresh token. That is the right outcome
   for `_refresh_token` (restart the flow so the SDK lands in 401 -> full
   auth), but `_hermes_reload_tokens_after_refresh_failure` shares the helper
   and must instead treat the candidate as rejected: restore the previous
   pair and return False so the caller clears state and prompts. The helper
   now returns whether a refresh token survived binding and each caller
   decides; the one-shot adopt wrapper is inlined into `_refresh_token`.

2. `_hermes_live_ttl` treated `expires_in is None` as expired. RFC 6749 makes
   `expires_in` optional, the SDK's `is_token_valid()` is True with no expiry,
   and `_rebase_expires_in` preserves None on read, so a peer's rotated pair
   without an expiry was never adopted and we POSTed its refresh token
   anyway, burning a generation on single-use providers. None now counts as
   live; the try/except around a pydantic `int | None` field is dropped.

One new test drives the real auth flow against a peer pair with no
`expires_in` and asserts the pair is adopted with zero POSTs.
This commit is contained in:
kshitijk4poor
2026-09-15 00:59:00 +05:30
committed by kshitij
parent 2e89c5da48
commit f4bf786c57
2 changed files with 56 additions and 36 deletions
Download Patch File Download Diff File Expand all files Collapse all files

25
tests/tools/test_mcp_oauth_manager.py
View File

@@ -882,6 +882,31 @@ async def test_refresh_adopts_expired_peer_pair_and_posts_its_refresh_token(tmp_
assert provider.context.current_tokens.refresh_token == "R3"
@pytest.mark.asyncio
async def test_refresh_adopts_peer_pair_without_expiry_and_skips_the_post(tmp_path, monkeypatch):
"""A peer rotated to (A2, R2) with no ``expires_in`` (RFC 6749 optional): that pair is live.
Treating a missing expiry as expired would POST R2 needlessly and burn a
generation on a single-use provider.
"""
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
endpoint = "https://idp.example.com/oauth/token"
provider = _fenced_provider(tmp_path, monkeypatch, endpoint)
await provider.context.storage.set_tokens(_token("A2", "R2", expires_in=None))
posted = []
def responder(request):
if request.method == "POST":
posted.append(request)
return _fake_response(200, str(request.url), b"{}")
await _drive_flow(provider, responder)
assert posted == [], "a live peer pair must be adopted without presenting a refresh token"
assert (provider.context.current_tokens.access_token, provider.context.current_tokens.refresh_token) == ("A2", "R2")
@pytest.mark.asyncio
async def test_refresh_restarts_flow_when_disk_pair_is_from_another_issuer(tmp_path, monkeypatch):
"""A disk pair bound to a different issuer loses its refresh token on adoption.

67
tools/mcp_oauth_provider.py
View File

@@ -144,12 +144,22 @@ class HermesProviderMixin:
try:
# Re-read under the fence: a peer may have rotated while we waited
# for it, in which case the token we were about to POST is dead.
adopted = await self._hermes_adopt_tokens_from_disk()
if adopted and self._hermes_live_ttl() and self.context.is_token_valid():
# The peer's access token is live: presenting our copy of the
# refresh token would only burn a generation on a single-use
# provider. Skip the POST and let the flow restart.
raise _RefreshCompletedByPeer
# If disk already holds a different refresh token, the peer that held
# the fence before us won this generation; its value is the only one
# the provider will still accept, so install it even when its access
# token has already expired (the POST we build needs the new grant).
candidate = await self._hermes_rotated_candidate()
if candidate is not None:
if not self._hermes_install_disk_pair(candidate):
# Issuer binding stripped the peer's refresh token: nothing
# left to POST. Restart the flow so the SDK lands in 401 ->
# full authorization instead of raising over a dead grant.
raise _RefreshCompletedByPeer
if self._hermes_live_ttl() and self.context.is_token_valid():
# The peer's access token is live: presenting our copy of the
# refresh token would only burn a generation on a single-use
# provider. Skip the POST and let the flow restart.
raise _RefreshCompletedByPeer
return self._prepare_token_request(await super()._refresh_token())
except BaseException:
# Never hold the fence when no POST will follow.
@@ -157,17 +167,17 @@ class HermesProviderMixin:
raise
def _hermes_live_ttl(self) -> bool:
"""True when the installed token has a real, positive TTL.
"""True when the installed token is not known to be past due.
Storage clamps a past-due token to ``expires_in == 0`` on read (see
HermesTokenStorage.get_tokens); the SDK's is_token_valid() compares
``time.time() <= expiry`` and still reports True for that boundary,
which would make us adopt a token the server rejects immediately.
``expires_in`` is optional in RFC 6749: None means no expiry was
issued, which the SDK treats as valid, so it counts as live here too.
"""
try:
return int(getattr(self.context.current_tokens, "expires_in", 0) or 0) > 0
except (TypeError, ValueError):
return False
exp = getattr(self.context.current_tokens, "expires_in", None)
return exp is None or int(exp) > 0
async def _hermes_acquire_refresh_fence(self) -> None:
"""Enter the fence, or let RefreshFenceTimeout abort this attempt.
@@ -214,35 +224,17 @@ class HermesProviderMixin:
return None
return stored
def _hermes_install_disk_pair(self, tokens) -> None:
def _hermes_install_disk_pair(self, tokens) -> bool:
"""Publish a disk pair to the context and re-run issuer binding on it.
If the enforcer strips the refresh token (the pair was minted by a
different issuer) there is nothing left to refresh with: restart the
SDK flow so it lands in 401 -> full authorization instead of raising
OAuthTokenError over an unusable grant.
Returns False when the enforcer strips the refresh token (the pair was
minted by a different issuer): there is nothing left to refresh with,
and each caller decides what that means for its own flow.
"""
self.context.current_tokens = tokens
self.context.update_token_expiry(tokens)
enforce_refresh_token_issuer(self.context)
if not getattr(self.context.current_tokens, "refresh_token", None):
raise _RefreshCompletedByPeer
async def _hermes_adopt_tokens_from_disk(self) -> bool:
"""Adopt a peer's newer tokens before POSTing our own copy.
Called under the fence. If disk already holds a different refresh
token, the peer that held the fence before us won this generation;
its value is the only one the provider will still accept, so it is
installed even when its access token has already expired (the POST
we are about to build needs the new refresh token). Returns True
when the in-memory pair was replaced.
"""
candidate = await self._hermes_rotated_candidate()
if candidate is None:
return False
self._hermes_install_disk_pair(candidate)
return True
return bool(getattr(self.context.current_tokens, "refresh_token", None))
async def _initialize(self) -> None:
"""Load stored state, restore persisted server metadata when the SDK has none (so the issuer
@@ -345,8 +337,11 @@ class HermesProviderMixin:
# to be installed to be tested; a losing probe must leave the context
# exactly as it found it.
previous_tokens = self.context.current_tokens
self._hermes_install_disk_pair(candidate)
if self._hermes_live_ttl() and self.context.is_token_valid():
if (
self._hermes_install_disk_pair(candidate)
and self._hermes_live_ttl()
and self.context.is_token_valid()
):
return True
self.context.current_tokens = previous_tokens
self.context.update_token_expiry(previous_tokens)