Discover auth-server metadata before eager token refresh - #3241
Closed
mfadul24 wants to merge 1 commit into
Closed
Conversation
Copy link
Copy Markdown
Sorry, something went wrong.
On a cold start with a cached-but-expired token (reusing a stored refresh
token before any 401), async_auth_flow refreshed before discovery ran, so
oauth_metadata was None and _refresh_token fell back to {origin}/token.
That drops any issuer path and 404s on servers whose token endpoint lives
under a path (e.g. .../oauth2/api/v1/token) — the refresh fails, tokens are
cleared, and the client is forced into interactive re-auth it may not be
able to complete. Discover AS metadata before the eager refresh.
Adds _discover_oauth_metadata (pure discovery, driven through the auth
flow) and _refresh_with_discovery, plus a regression test.
Copy link
Copy Markdown
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:633">
P1: A cold-start refresh can send credentials bound to an old authorization server to a newly discovered one. `_refresh_with_discovery` refreshes immediately after PRM/ASM discovery, but omits the issuer-binding check that the 401 discovery path uses before any token request. When `client_info.issuer` differs from the discovered AS, clear the bound client/tokens and skip refresh so the subsequent authorization flow registers/authenticates against the new issuer instead.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Sorry, something went wrong.
Comment thread
src/mcp/client/auth/oauth2.py
| except StopAsyncIteration: | ||
| break | ||
|
|
||
| refresh_response = yield await self._refresh_token() |
Copy link
Copy Markdown
There was a problem hiding this comment.
P1: A cold-start refresh can send credentials bound to an old authorization server to a newly discovered one. _refresh_with_discovery refreshes immediately after PRM/ASM discovery, but omits the issuer-binding check that the 401 discovery path uses before any token request. When client_info.issuer differs from the discovered AS, clear the bound client/tokens and skip refresh so the subsequent authorization flow registers/authenticates against the new issuer instead.
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 633:
<comment>A cold-start refresh can send credentials bound to an old authorization server to a newly discovered one. `_refresh_with_discovery` refreshes immediately after PRM/ASM discovery, but omits the issuer-binding check that the 401 discovery path uses before any token request. When `client_info.issuer` differs from the discovered AS, clear the bound client/tokens and skip refresh so the subsequent authorization flow registers/authenticates against the new issuer instead.</comment>
<file context>
@@ -577,6 +577,64 @@ async def _validate_resource_match(self, prm: ProtectedResourceMetadata) -> None
+ except StopAsyncIteration:
+ break
+
+ refresh_response = yield await self._refresh_token()
+ if not await self._handle_refresh_response(refresh_response):
+ # Refresh failed, need full re-authentication
</file context>
Sorry, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On a cold start with a cached-but-expired token (reusing a stored refresh token before any 401), async_auth_flow refreshed before discovery ran, so oauth_metadata was None and _refresh_token fell back to {origin}/token. That drops any issuer path and 404s on servers whose token endpoint lives under a path (e.g. .../oauth2/api/v1/token) — the refresh fails, tokens are cleared, and the client is forced into interactive re-auth it may not be able to complete. Discover AS metadata before the eager refresh.
Adds _discover_oauth_metadata (pure discovery, driven through the auth flow) and _refresh_with_discovery, plus a regression test.
Fixes #3240
Motivation and Context
Hit this against a hosted MCP server whose authorization server isn't at the resource origin (token endpoint https://host/oauth2/api/v1/token, not https://host/token). A headless client with cached tokens disconnects every time the short-lived access token expires: on reconnect the eager refresh at the top of async_auth_flow runs before any 401/discovery, oauth_metadata is None, and _refresh_token uses urljoin(get_authorization_base_url(server_url), "/token") → {scheme}://{netloc}/token → 404 → _handle_refresh_response clears the tokens → the flow drops to interactive auth a background client can't complete.
The same path-stripping fallback exists in _get_token_endpoint, _perform_authorization_code_grant (/authorize) and DCR (/register), but those run inside the 401 branch after discovery, so metadata is already populated there — refresh is the one that fires before discovery, which is why it's the acute, silent case. This PR fixes the refresh path; the other fallbacks are left as-is.
How Has This Been Tested?
Breaking Changes
None. On a cold start with no cached metadata the eager refresh now issues discovery requests before the refresh; once metadata is known it's a no-op, so the fast path (already-valid or already-discovered) is unchanged.
Types of changes
Checklist
Additional context
_discover_oauth_metadata is a pure-discovery async generator (PRM → AS metadata) that yields its requests through the outer httpx auth flow — noside-channel client — and only populates protected_resource_metadata / auth_server_url / oauth_metadata. _refresh_with_discovery wraps it + the existing refresh so async_auth_flow drives one sub-flow and stays under the module's complexity cap. It reuses the existing build_*_discovery_urls /handle_*_response / validate_metadata_issuer helpers, so discovery behaves exactly like the 401 path.