Uh oh!
There was an error while loading. Please reload this page.
Document redirect_uri wire-format change in OAuth migration note - #2929
Conversation
redirect_uri wire-format change in OAuth migration noteUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
LGTM — docs-only addition to the migration note that accurately describes the redirect_uri wire-format change; the inline nit about wording actionability is worth a quick polish but doesn't block.
Extended reasoning...
Overview
This PR adds a single paragraph to docs/migration.md, extending the existing OAuth metadata trailing-slash note (from #2925) to call out that OAuthClientMetadata.redirect_uris now serializes path-less URIs without a trailing slash, and that authorization servers performing RFC 6749 §3.1.2.3 exact-match comparison may reject a previously registered slash-suffixed URI. No code is changed.
Security risks
None. This is a documentation-only change. The paragraph itself describes an OAuth-related behavior change, but the underlying behavior was introduced and reviewed in #2925 — this PR only documents it.
Level of scrutiny
Minimal scrutiny is appropriate: one paragraph in a migration guide, no executable code, no API surface change. The factual claims in the paragraph (serialization change, verbatim transmission in /authorize and token exchange, RFC 6749 exact-match requirement) are consistent with the SDK behavior introduced in #2925.
Other factors
The single finding is a nit about the remedy wording — "re-register the client" isn't directly actionable since the SDK only re-registers when TokenStorage.get_client_info() returns nothing; clearing the stored registration is the practical fix. That inline comment gives the author a concrete suggestion, but the note as written is not incorrect, so it doesn't warrant blocking a docs-only follow-up.
| This also changes the wire form of `OAuthClientMetadata.redirect_uris`: a path-less redirect URI | ||
| passed as a string (e.g. `redirect_uris=['http://localhost:8080']`) now serializes as | ||
| `http://localhost:8080` instead of `http://localhost:8080/`, and the client sends it verbatim in | ||
| the `/authorize` and token-exchange requests. RFC 6749 §3.1.2.3 requires authorization servers to | ||
| match redirect URIs by exact string comparison, so if you registered such a URI with a previous SDK | ||
| release (with the trailing slash) and the registration is persisted in `TokenStorage`, re-register | ||
| the client so the stored value matches what the SDK now transmits. | ||
There was a problem hiding this comment.
🟡 The instruction to "re-register the client" isn't actionable as written: the SDK only performs dynamic client registration when TokenStorage.get_client_info() returns nothing, so a persisted registration is reused as-is and there is no API to force re-registration. Suggest rewording to tell affected users to clear the stored client registration (and tokens) from their TokenStorage so the next flow registers afresh with the slash-less redirect URI.
Extended reasoning...
What the note says vs. what the SDK does. The new paragraph tells users whose registration "is persisted in TokenStorage" to "re-register the client so the stored value matches what the SDK now transmits." However, OAuthClientProvider only performs dynamic client registration when no registration is already persisted: in src/mcp/client/auth/oauth2.py, _initialize() loads client_info from storage.get_client_info() (line 484), and Step 4 of the 401 flow only registers under if not self.context.client_info: (line 596). There is no SDK API to force re-registration while a registration remains in storage, so a reader following the note literally has no way to act on it.
The code path that makes the mismatch real. After PR #2925, OAuthClientMetadata serializes a path-less redirect_uris entry without the trailing slash, and the client sends client_metadata.redirect_uris[0] verbatim in the /authorize request and the token exchange (oauth2.py lines 340 and 399). Meanwhile the authorization server still holds the old slash-suffixed registration created by a previous SDK release, and RFC 6749 §3.1.2.3 mandates exact string comparison — so the flow fails. The note correctly identifies the problem; only the remedy is unclear.
Step-by-step example. (1) A user on the previous SDK registers with redirect_uris=['http://localhost:8080']; the metadata serialized as http://localhost:8080/, and the AS stored that. The resulting OAuthClientInformationFull is persisted via TokenStorage.set_client_info(). (2) The user upgrades and tokens later expire/are cleared, triggering a fresh authorization. (3) _initialize() loads the old client_info from storage, so the registration step is skipped, but the redirect_uri query parameter now goes out as http://localhost:8080 (no slash). (4) The AS exact-matches against its stored http://localhost:8080/ and rejects the request. (5) The user reads the migration note and tries to "re-register" — but as long as get_client_info() keeps returning the old registration, the SDK never re-registers, so the failure persists.
Secondary wording issue. The phrase "so the stored value matches what the SDK now transmits" conflates two stores: the OAuthClientInformationFull persisted in the user's TokenStorage (which gates whether DCR runs at all) and the registration held by the authorization server (which is what the RFC 6749 exact-match comparison actually runs against). Re-registration fixes the AS-side record; clearing TokenStorage is what causes re-registration to happen.
Suggested fix. Reword the final clause to something like: "…clear the stored client registration from your TokenStorage (so get_client_info() returns None) and let the client perform a fresh registration with the new redirect URI — or update the registration at the authorization server out of band." This is a docs-only clarity improvement to a docs-only PR whose entire content is this paragraph; the statement isn't wrong, just not actionable, hence nit severity. All four verifiers independently confirmed the underlying SDK behavior and agreed on this characterization.
This pull request is included in pre-release v2.0.0a3 |
Follow-up to #2925. That PR added
url_preserve_empty_path=TruetoOAuthClientMetadata, which also changes the serialized form ofredirect_urison the wire — a path-less redirect URI passed as a string now drops its trailing slash, and the client sends it verbatim in/authorizeand token-exchange requests.Since RFC 6749 §3.1.2.3 mandates exact string comparison of redirect URIs at the authorization server, a registration persisted in
TokenStoragebefore the upgrade could stop matching. This extends the existing migration note to call that out and tell affected users to re-register.Spotted by the Claude review bot on #2925: #2925 (comment)
AI Disclaimer
This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.