OAuth client: keep refresh_token on non-rotating refresh; restore same-origin issuer binding#2946
Conversation
A refresh response may omit `refresh_token` when the AS does not rotate refresh tokens; the client is expected to keep using the previously issued one. The handler was overwriting the stored token wholesale, dropping the prior `refresh_token` and forcing a full re-authorization at the next expiry. Carry it forward alongside the existing scope carry-forward. Also stamp the SEP-2352 issuer on a same-origin fallback registration #2936 stopped recording the issuer when DCR fell back to the resource origin's `/register`, to avoid binding a registration to a PRM-advertised AS we never reached. That over-suppressed the legacy same-origin embedded-AS case (no PRM, root ASM at the resource origin without `registration_endpoint`), where the fallback `/register` is on the discovered issuer's own host and the binding was genuinely established. Restore the stamp when the fallback origin matches the discovered issuer's origin so a later AS migration still triggers discard and re-registration.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs — both changes look consistent with RFC 6749 §6 and the SEP-2352 binding semantics, and the new tests cover the carry-forward, rotation, and same-origin stamping cases — but since this adjusts OAuth credential-binding behavior it's worth a human pass.
Extended reasoning...
Overview
This PR makes two small follow-up changes to the OAuth client (src/mcp/client/auth/oauth2.py) from review on #2936: (1) _handle_refresh_response now carries the prior refresh_token forward when the AS omits it on refresh (non-rotating AS, RFC 6749 §6), alongside the existing scope carry-forward; and (2) the SEP-2352 issuer stamp is restored for fallback dynamic client registrations when the resource-origin /register fallback is on the discovered issuer's own origin (legacy same-origin embedded AS). Three new tests in tests/client/test_auth.py cover the omitted/rotated refresh_token cases and the same-origin stamping flow.
Security risks
The changes are in OAuth client auth code. The refresh_token carry-forward only persists a token the client already held, and a returned (rotated) refresh_token still takes precedence — low risk. The issuer-stamp change broadens when stored client credentials are bound to a discovered issuer; the gate requires the fallback origin to equal the discovered issuer's origin, which matches the binding actually established. Stamping more precisely strengthens (rather than weakens) the SEP-2352 protection against silently reusing credentials after an AS migration. No injection, data-exposure, or auth-bypass vectors are apparent.
Level of scrutiny
Although the diff is small, well-reasoned, and well-tested, it modifies authorization/credential-binding logic in a widely used client library, which per policy warrants a human reviewer's sign-off rather than automated approval — especially the issuer-binding condition, which encodes nuanced SEP-2352 semantics that a maintainer familiar with that SEP should confirm.
Other factors
The bug-hunting pass found no issues. The new tests exercise the changed paths directly, and the PR description notes the existing cross-origin parametrized test still asserts the issuer is left unset when the fallback hits a different host, guarding against a regression in the opposite direction.
2 participants
Two small follow-ups to #2936, from review on that PR.
Motivation and Context
Carry an omitted
refresh_tokenforward on refresh. RFC 6749 §6 allows the AS to omitrefresh_tokenfrom a refresh response when it does not rotate refresh tokens (common in practice)._handle_refresh_responsewas overwriting the stored token wholesale, dropping the still-valid priorrefresh_tokenfrom both memory and storage, so the next expiry forced a full interactive re-authorization (or failed for headless clients). Carries it forward alongside the existing scope carry-forward.Restore the SEP-2352 issuer stamp for same-origin fallback registrations. #2936 stopped recording the issuer when dynamic client registration fell back to the resource origin's
/register, to avoid binding a registration to a PRM-advertised AS we never reached. That gate also dropped the stamp in the legacy same-origin embedded-AS case — no PRM, root authorization-server metadata at the resource origin withoutregistration_endpoint— where the fallback/registeris on the discovered issuer's own host and the binding was genuinely established. Without the stamp, a later AS migration silently reuses the old credentials (credentials_match_issuerreturns true for an unset issuer) instead of re-registering. The stamp is now also recorded when the fallback origin matches the discovered issuer's origin.How Has This Been Tested?
test_handle_refresh_response_carries_prior_scope_and_refresh_token_when_omitted— refresh response with neitherscopenorrefresh_tokenkeeps both prior valuestest_handle_refresh_response_adopts_rotated_refresh_token_when_returned— a returnedrefresh_tokenstill replaces the prior onetest_issuer_is_stamped_when_same_origin_fallback_register_is_on_the_discovered_issuer— PRM 404 → root ASM at the resource origin withoutregistration_endpoint→ fallback DCR → persisted record is bound to the discovered issuerBreaking Changes
None.
Types of changes
Checklist
AI Disclaimer