| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
…resh Two bugs in OAuthClientProvider._initialize() combine to force interactive re-authentication on every process restart, even when a valid refresh_token is on disk and the IdP would happily exchange it. Bug 1: _initialize() loads tokens but never calls update_token_expiry(), so context.token_expiry_time stays None. is_token_valid() then short-circuits to True regardless of whether the access_token is actually expired, and the refresh-on-expiry guard in async_auth_flow never fires. Bug 2: _refresh_token() falls back to urljoin(base, '/token') when context.oauth_metadata is unset, which gives the wrong endpoint for any IdP that mounts its token endpoint at a non-root path (Hydra/Ory: /oauth/token, Auth0: /oauth/token, Keycloak: /protocol/openid-connect/token, etc.). The refresh grant silently 404s. This affects every MCP client using OAuthClientProvider against an IdP with short-lived access_tokens and a Hydra-style token endpoint — including Fold MCP, Notion, GitHub PAT-rotated OAuth, etc. Fix: - Mirror what set_tokens() does on the write path: call update_token_expiry() after loading tokens so is_token_valid() returns the correct boolean. - Call storage.load_oauth_metadata() (when implemented, via getattr guard) to pre-populate context.oauth_metadata. The TokenStorage Protocol is unchanged — load_oauth_metadata is treated as an optional extension. SDK-provided storages that don't implement it remain unaffected; downstream clients (e.g. hermes-agent's HermesTokenStorage) that already implement it get the fix transparently. Fixes modelcontextprotocol#3250 Verified live against https://mcp.fold.money with a 16-minute repro: without the fix, every restart sent an expired access_token, got a 401, and bounced the user through the full re-auth flow. With the fix, the refresh grant returns HTTP 200 transparently, fold.json mtime advances, and the MCP call returns real data. Tests: three new tests added in tests/client/test_auth.py (TestOAuthFallback class) — all 3 fail against the unpatched code (negative control) and pass against the patched code. Full test suite (5333 tests) still passes. AI disclosure: Drafted with AI assistance (GPT-class model). The bug analysis, code-path tracing, fix design, and live verification were all done by a human reviewer who understood every line.
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/mcp/client/auth/oauth2.py">
<violation number="1" location="src/mcp/client/auth/oauth2.py:580">
P2: Refresh can fail before any network call when `load_oauth_metadata()` returns non-model data, because `_initialize()` stores it without validation and `_refresh_token()` assumes an `OAuthMetadata` object. Validating/coercing `meta` to `OAuthMetadata` here keeps optional storage extensions non-fatal as intended.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Sorry, something went wrong.
| if inspect.iscoroutine(meta): | ||
| meta = await meta | ||
| if meta is not None: | ||
| self.context.oauth_metadata = meta # type: ignore[assignment] |
There was a problem hiding this comment.
P2: Refresh can fail before any network call when load_oauth_metadata() returns non-model data, because _initialize() stores it without validation and _refresh_token() assumes an OAuthMetadata object. Validating/coercing meta to OAuthMetadata here keeps optional storage extensions non-fatal as intended.
Prompt for AI agentsCheck if this issue is valid — if so, understand the root cause and fix it. At src/mcp/client/auth/oauth2.py, line 580:
<comment>Refresh can fail before any network call when `load_oauth_metadata()` returns non-model data, because `_initialize()` stores it without validation and `_refresh_token()` assumes an `OAuthMetadata` object. Validating/coercing `meta` to `OAuthMetadata` here keeps optional storage extensions non-fatal as intended.</comment>
<file context>
@@ -551,6 +552,38 @@ async def _initialize(self) -> None:
+ if inspect.iscoroutine(meta):
+ meta = await meta
+ if meta is not None:
+ self.context.oauth_metadata = meta # type: ignore[assignment]
+ except Exception:
+ # Storage implementations are user-provided; a misbehaving
</file context>
| self.context.oauth_metadata = meta # type: ignore[assignment] | |
| self.context.oauth_metadata = ( | |
| meta if isinstance(meta, OAuthMetadata) else OAuthMetadata.model_validate(meta) | |
| ) |
Sorry, something went wrong.
|
Thanks for the PR. We're tracking this fix in #3263 instead, so I'm closing this one. Feel free to reopen if this is still relevant. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Summary
Fix two bugs in OAuthClientProvider._initialize() that combine to force interactive re-authentication on every process restart, even when a valid refresh_token is on disk and the IdP would happily exchange it.
Closes #3250.
The bugs
Bug 1: _initialize() doesn't compute token_expiry_time
_initialize() loads current_tokens from storage but never calls context.update_token_expiry(token). So context.token_expiry_time stays None.
Then is_token_valid():
When token_expiry_time is None, the second clause is not None or … = True, so the function unconditionally returns True regardless of whether the access_token is expired.
The refresh-on-expiry guard in async_auth_flow:
…never fires. Expired access_tokens are sent on every request, the server returns 401, and the user lands in the full-re-auth branch.
This same fix is already applied on the write path: set_tokens() does call update_token_expiry(). The read path (_initialize) just doesn't do the same thing.
Bug 2: _refresh_token() builds the wrong endpoint URL when oauth_metadata isn't loaded
_refresh_token() picks the token endpoint like this:
For a server like https://mcp.fold.money/mcp, the fallback path produces https://mcp.fold.money/token — 404. The correct endpoint for Hydra-style servers is https://mcp.fold.money/oauth/token.
oauth_metadata is normally populated via server discovery during the 401-handling flow (after a 401). But the refresh-on-expiry path runs before any 401 — it proactively refreshes when the token is expired, with no 401 yet. So oauth_metadata is never populated, and refresh fails silently with 404.
The fix
In src/mcp/client/auth/oauth2.py, modify OAuthClientProvider._initialize() to:
The TokenStorage Protocol is unchanged — load_oauth_metadata is treated as an optional extension. No breaking changes.
Tests
Three new tests added in tests/client/test_auth.py (in the existing TestOAuthFallback class):
All 3 tests fail against the unpatched code (negative control) and pass against the patched code.
Full test suite: 5,333 tests pass, 10 skipped, 1 xfailed (pre-existing). Pyright clean on both modified files.
Live verification
Patched locally against mcp==1.28.1 on macOS (Hermes agent 0.20.0). 16-minute live repro against https://mcp.fold.money:
AI disclosure
Drafted with AI assistance (GPT-class model). The bug analysis, code path tracing, fix design, live verification, and tests were all done by a human reviewer who understood every line. The fix itself is 12 lines of source + 3 tests, all under 100 lines total.