fix(client): invalidate credentials and re-register when the authorization server changes (SEP-2352)#2271
fix(client): invalidate credentials and re-register when the authorization server changes (SEP-2352)#2271mattzcarey wants to merge 8 commits into
Conversation
🦋 Changeset detectedLatest commit: f592d55 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
e848b15 to
d16ebfa
Compare
|
Rebased on current main and pushed I also found and fixed one cached-discovery edge case while reviewing the draft: when a 401 retry passes an explicit Verification:
|
|
Addressed both SEP-2352 review threads in Changes:
Verified:
|
…ation server changes (SEP-2352)
b44f038 to
f592d55
Compare
There was a problem hiding this comment.
Thanks — the latest revisions address my earlier findings (token-vs-client invalidation gating, PRM-fallback false positives, trailing-slash normalization, code-exchange re-invalidation), and I found no new issues in this pass. Leaving final review to a maintainer since this changes destructive credential-invalidation behavior against a draft spec (SEP-2352) and the trigger condition is the design call you flagged in the description.
Extended reasoning...
Overview
This PR implements SEP-2352 authorization-server binding in the OAuth client (packages/client/src/client/auth.ts): after discovery, the previously recorded AS identity is compared against the freshly resolved one, and on a genuine change the SDK invalidates tokens (always) and the client registration (unless the client_id is a portable CIMD HTTPS URL or an authorization code is being exchanged), forcing re-registration with the new AS. It also adds an authorizationServerSource discriminator to discovery results, refreshes cached discovery on explicit 401 challenges, adds identity normalization, JSDoc guidance, a changeset, and ~770 lines of new tests.
Security risks
The change sits squarely in security-sensitive territory: it deliberately destroys persisted OAuth client registrations and tokens based on discovery results, and it reduces cross-AS credential reuse (a real protection). The risk surface is false-positive invalidation (credential loss / lockout on ASes without open DCR) and false negatives (stale credentials reused against a new AS). Several rounds of review iterated on exactly these edge cases — transient PRM failures, fallback-origin identities, CIMD token handling, cache clobbering, trailing-slash issuer drift, and re-invalidation during code exchange — and the current revision addresses all of them with corresponding regression tests. No injection or data-exposure concerns; the remaining risk is behavioral.
Level of scrutiny
High. This is production-critical client auth logic with destructive side effects, implementing a draft spec section, and the author explicitly flags the invalidation trigger as needing a maintainer direction call before leaving draft. That is a design decision, not something a bot approval should short-circuit. The discovery branching in authInternal has also grown noticeably more complex through the review iterations, which itself deserves a maintainer's judgment on whether the complexity is warranted in the SDK versus provider-land.
Other factors
The bug-hunting pass on the current revision found no new issues, the new test suite covers the previously identified failure modes (AS change, same AS, CIMD, transient PRM fallback, code exchange after migration, trailing-slash identities), and the changeset wording was updated to match the shipped CIMD/token behavior. The author reports clean build/typecheck/lint/test runs. None of that changes the conclusion that a human maintainer should make the final call on the invalidation-trigger semantics and overall approach.
Implements SEP-2352 (authorization server binding & migration) for the OAuth client. Draft because the invalidation trigger is behavioral and needs maintainer sign-off — see "Behavioral risk" below.
Spec
Draft spec,
basic/authorization/client-registration.mdx, "Authorization Server Binding":What changed
authInternal(discovery block,packages/client/src/client/auth.ts): after discovery resolves the authorization server, the previously recorded AS identity (cacheddiscoveryState.authorizationServerUrl/ cached metadataissuer/provider.authorizationServerUrl?.()) is compared against the freshly resolved identity. On a mismatch, the SDK callsprovider.invalidateCredentials?.('client')and('tokens'); the existing flow then sees no client information and re-registers with the new AS.client_idis an HTTPS URL (CIMD), no invalidation occurs — those client IDs are explicitly portable across ASes.invalidateCredentials('client')but does not implementsaveClientInformationwill get a thrown error ("OAuth client information must be saveable for dynamic registration") rather than silent reuse, matching the spec's SHOULD-surface-an-error guidance.OAuthClientProvider.clientInformation/saveClientInformation: implementations SHOULD persist client credentials keyed by the ASissuer.Canonical comparison key (flagging for maintainers)
The spec says credentials are keyed by the AS
issueridentifier. The comparison uses the validated AS metadataissuerwhere available, falling back to the AS URL when metadata is absent — under RFC 8414 the two coincide. To avoid false-positive invalidation when they textually diverge (trailing slash, issuer-vs-discovery-URL drift), a match on either the issuer or the AS URL is treated as "same AS". Happy to tighten this to issuer-only if preferred.Behavioral risk (flagging for a direction call)
Silently dropping credentials on AS change could surprise providers whose
discoveryStateis flaky — e.g. a provider that intermittently returns a stale or empty discovery state whileauthorizationServerUrl()still reports the old AS would trigger invalidation and a re-registration round-trip. Flagging the trigger condition for a direction call before this leaves draft.Land order
auth.ts/auth.test.ts. This branch is independent (offmain, not stacked); expect trivial test-file rebases depending on merge order.issvalidation) PR will touchauthInternal's code-exchange path and will rebase over this one.Validation
pnpm build:all,pnpm typecheck:all,pnpm lint:all— cleanpnpm --filter @modelcontextprotocol/client test— 370/370 pass (3 new tests in a standalonedescribeappended at the end ofauth.test.ts: AS change → invalidate + re-register; same AS → no invalidation; CIMD client_id + AS change → no invalidation, credentials reused)@modelcontextprotocol/clientDownstream impact
cloudflare/agents'
DurableObjectOAuthClientProvideralready implementsinvalidateCredentials('client')so this works; its storage keys client info per server-name+client_id rather than per-issuer — fully honoring SEP-2352's keying guidance would be an optional agents-side follow-up, not a break.Closes #2201