-
Notifications
You must be signed in to change notification settings - Fork 3.8k
fix(client/auth): discard stored client registrations whose secret has expired #3264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -109,6 +109,22 @@ def check_registration_usable(client_info: OAuthClientInformationFull) -> None: | |
| ) | ||
|
|
||
|
|
||
| def stored_registration_expired(client_info: OAuthClientInformationFull) -> bool: | ||
| """Whether a stored registration's minted secret has lapsed and can no longer authenticate. | ||
|
|
||
| RFC 7591 requires `client_secret_expires_at` whenever a secret is issued, with ``0`` | ||
| meaning the secret never expires. Once a non-zero expiry passes, every token-endpoint | ||
| interaction authenticating with that secret fails with ``invalid_client`` — and with no | ||
| RFC 7592 rotation endpoint, re-registration is the only standard recovery. The lapse | ||
| only matters for registrations that authenticate with the minted secret: ``none`` (or | ||
| an absent method) sends no secret, and `private_key_jwt` signs an assertion instead. | ||
| """ | ||
| if client_info.token_endpoint_auth_method not in _SECRET_TOKEN_ENDPOINT_AUTH_METHODS: | ||
| return False | ||
| expires_at = client_info.client_secret_expires_at | ||
| return expires_at is not None and expires_at != 0 and expires_at < int(time.time()) | ||
|
|
||
|
|
||
| class PKCEParameters(BaseModel): | ||
| """PKCE (Proof Key for Code Exchange) parameters.""" | ||
|
|
||
|
|
@@ -548,9 +564,23 @@ async def _handle_refresh_response(self, response: httpx2.Response) -> bool: | |
| return False | ||
|
|
||
| async def _initialize(self) -> None: | ||
| """Load stored tokens and client info.""" | ||
| """Load stored tokens and client info. | ||
|
|
||
| Stored client information whose minted secret has expired (RFC 7591 | ||
| `client_secret_expires_at`) is treated as absent: reusing it can only produce | ||
| `invalid_client` at the token endpoint — even interactive re-authorization ends in | ||
| the same failure, permanently — so it is discarded here and the next 401 flow | ||
| re-registers (or resolves CIMD), overwriting the dead record in storage. Any still | ||
| stored tokens are kept: a live access token keeps working without client | ||
| authentication, and with no client info the refresh path (which would present the | ||
| lapsed secret) is skipped. | ||
| """ | ||
| self.context.current_tokens = await self.context.storage.get_tokens() | ||
| self.context.client_info = await self.context.storage.get_client_info() | ||
| client_info = await self.context.storage.get_client_info() | ||
| if client_info is not None and stored_registration_expired(client_info): | ||
| logger.debug("Stored client registration secret has expired; discarding so the next flow re-registers") | ||
| client_info = None | ||
| self.context.client_info = client_info | ||
|
claude[bot] marked this conversation as resolved.
|
||
| self._initialized = True | ||
|
|
||
| def _add_auth_header(self, request: httpx2.Request) -> None: | ||
|
|
@@ -604,6 +634,18 @@ async def async_auth_flow(self, request: httpx2.Request) -> AsyncGenerator[httpx | |
| # Perform full OAuth flow | ||
| try: | ||
| # OAuth flow must be inline due to generator constraints | ||
|
|
||
| # A registration whose minted secret lapsed mid-session (after | ||
| # _initialize already loaded it) can no longer authenticate either — | ||
| # discard it here too, so Step 4 re-registers instead of running an | ||
| # interactive authorization doomed to fail `invalid_client` at the | ||
| # token endpoint. | ||
| if self.context.client_info is not None and stored_registration_expired(self.context.client_info): | ||
| logger.debug( | ||
| "Stored client registration secret has expired; discarding so this flow re-registers" | ||
| ) | ||
|
Comment on lines
+638
to
+646
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The new expiry discards (in Extended reasoning...What the bug isBoth new expiry-discard sites set Why the PR's scoping rationale doesn't cover thisThe PR's "stored tokens are kept" reasoning (a live access token keeps working without client authentication, and with no client info Step-by-step proof (consequence 1 — cross-issuer refresh-token presentation)Mid-session variant, verified against the code:
(Note: the fresh-restart variant of this trace does not fire — Consequence 2 — stale
|
||
| self.context.client_info = None | ||
|
Comment on lines
+639
to
+647
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The new mid-session Extended reasoning...The residual gapThis PR's in-flow expiry re-check (src/mcp/client/auth/oauth2.py:639-647) closes the previously-flagged mid-session gap for the 401 path — but 1. Refresh path (minor, self-healing)
Recovery does happen in the same flow: 2. 403
|
||
|
|
||
| www_auth_resource_metadata_url = extract_resource_metadata_from_www_auth(response) | ||
|
|
||
| # Step 1: Discover protected resource metadata (SEP-985 with fallback support) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.