chore: auth conformance closeout — SEP-990 fixture, migration.md + client docs#2349
Conversation
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
e705921 to
b3547d6
Compare
b0b6bef to
48f5171
Compare
b3547d6 to
edbbd84
Compare
48f5171 to
add874f
Compare
🦋 Changeset detectedLatest commit: 78df4a3 The changes in this PR will be included in the next version bump. 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 |
|
add874f to
074b816
Compare
edbbd84 to
354f0ac
Compare
a8f2a90 to
5ae811d
Compare
354f0ac to
794b732
Compare
5ae811d to
81bc563
Compare
794b732 to
71b2687
Compare
…(issuer-stamped)
auth() now stamps an `issuer` field onto every value it passes to
saveTokens()/saveClientInformation() and threads {issuer} as the ctx
argument to tokens()/clientInformation(). On read, a stored value whose
stamp names a different authorization server is treated as undefined via
discardIfIssuerMismatch(), so a client_id or refresh_token issued by one
AS is never sent to another — the flow re-registers/re-authorizes
exactly as if nothing were stored.
This replaces the previous detection-and-throw approach with structural
keying: providers that round-trip stored values verbatim are protected
with no code change; multi-AS providers key storage on ctx.issuer.
- New discardIfIssuerMismatch() helper (exported).
- AuthorizationServerMismatchError moved to authErrors.ts; thrown only
on the callback-leg gate (discoveryState issuer \!= resolved issuer).
- console.warn on the callback leg when the provider lacks
saveDiscoveryState().
- saveAuthorizationServerUrl()/authorizationServerUrl() deprecated:
still written for back-compat (Cross-App Access), never read.
- ClientCredentialsProvider/PrivateKeyJwtProvider/
StaticPrivateKeyJwtProvider/CrossAppAccessProvider gain expectedIssuer
and no longer define saveClientInformation().
- examples/oauth: single-slot blob storage + discoveryState.
- Conformance: provider implements discoveryState/invalidateCredentials;
handle401 clears discovery on 401 so AS migration is re-probed;
runAuthClient uses versionNegotiation:auto so the
auth/authorization-server-migration scenario (and the 2026-leg
scope-step-up/scope-retry-limit) speak the modern lifecycle.
- ~6 focused unit tests + 5 e2e as-migration:* rows asserting structural
behaviour (no token POST to wrong AS because the stamp read returned
undefined).
Claude-Session: https://claude.ai/code/session_01XBib5gRe8AMPPJhySCz3EJ
81bc563 to
78df4a3
Compare
71b2687 to
9bc98d3
Compare
78df4a3
into
fweinberger/auth-1-sep2468-client
| --- | ||
| '@modelcontextprotocol/client': minor | ||
| --- | ||
|
|
||
| Per-authorization-server credential isolation (SEP-2352). `auth()` now stamps an `issuer` field onto every value it passes to `saveTokens()` / `saveClientInformation()` and threads `{ issuer }` to `tokens()` / `clientInformation()`; on read, a stored credential whose stamp names a different authorization server is treated as `undefined`, so a `client_id` / `refresh_token` issued by one AS is never sent to another. Providers that round-trip stored values verbatim are protected with no code change; multi-AS providers may key storage on `ctx.issuer`. New `discardIfIssuerMismatch()` helper and `AuthorizationServerMismatchError` (callback-leg gate). `OAuthClientProvider.saveAuthorizationServerUrl()` / `authorizationServerUrl()` are deprecated (still written, never read). `ClientCredentialsProvider`, `PrivateKeyJwtProvider`, `StaticPrivateKeyJwtProvider`, and `CrossAppAccessProvider` gain `expectedIssuer` and no longer define `saveClientInformation()`. |
There was a problem hiding this comment.
🔴 The PR is titled and described as a docs-only chore ("Two closeout pieces with no packages/ runtime change", only the 'Documentation update' box checked, "Breaking Changes: None"), but the diff as it stands ships the full SEP-2352 runtime behavior change in packages/client (issuer stamping, the new fail-closed callback-leg gate throwing AuthorizationServerMismatchError, invalidateCredentials('all') → 'client'+'tokens', expectedIssuer on four bundled providers with their saveClientInformation() removed, two new public exports) plus this changeset declaring a minor bump for @modelcontextprotocol/client. Either the branch is carrying the SEP-2352 feature commits that were meant to land separately (the changeset-bot head flipping between 78df4a3 and add874f2 suggests the base/head may be off), or the title/description/'Types of changes' need rewriting to reflect a behavior-changing minor release — please reconcile so reviewers triaging by the description don't skip the security-relevant runtime changes.
Extended reasoning...
What the inconsistency is. The PR description opens with "Two closeout pieces with no packages/ runtime change", the title is a chore: for a fixture mapping plus docs, only the Documentation update type-of-change is checked, and Breaking Changes says None. The diff, however, includes the entire SEP-2352 per-authorization-server credential-isolation behavior change: packages/client/src/client/auth.ts (issuer stamping into saveTokens/saveClientInformation, the new exported discardIfIssuerMismatch(), the fail-closed callback-leg gate that throws the new AuthorizationServerMismatchError, and invalidateCredentials('all') changed to 'client'+'tokens' on invalid_client/unauthorized_client), authErrors.ts (new public error class), authExtensions.ts (expectedIssuer options on ClientCredentialsProvider/PrivateKeyJwtProvider/StaticPrivateKeyJwtProvider/CrossAppAccessProvider and removal of their saveClientInformation() methods — an observable public-surface change), index.ts (two new public exports), the new SEP-2352 unit/e2e test suites, and .changeset/auth-sep-2352-credential-isolation.md declaring a minor bump for @modelcontextprotocol/client. Both statements cannot be true at once: a PR whose delta contains a minor-bump changeset and runtime/public-API changes is not a docs-only chore.\n\nConcrete walk-through of the reviewer impact. (1) A reviewer triages PR #2349 by its title/description and the checked 'Documentation update' box, and budgets a docs-level review. (2) They review migration.md, docs/client.md regions, and the conformance fixture wiring, and approve. (3) The merge then lands a security-relevant runtime change — including a fail-closed path: any existing OAuthClientProvider that implements saveDiscoveryState() but cannot return discovery state on the callback leg now throws AuthorizationServerMismatchError where it previously proceeded — plus a public-API removal (saveClientInformation gone from four bundled providers) and a published minor version bump, none of which were reviewed. The repo's review conventions ('Decompose by default'; read added .changeset/*.md text against the implementation in the same diff) exist precisely to prevent this.\n\nWhy this isn't caught elsewhere. Nothing in CI compares the PR description or the 'Types of changes' checklist against the diff; the changeset tooling only checks that a changeset exists for changed packages, not that the PR prose acknowledges it.\n\nAddressing the stacked-PR reading. One plausible explanation is that this is the top of a stacked auth series (fweinberger/auth-6-closeout) whose parent feature commits (78df4a3 "feat(client): SEP-2352 …", 9bc98d3, 46b6b01, …) are reviewed in their own PRs, and the runtime delta only appears here because the diff is rendered against main rather than the integration base. The changeset-bot timeline is consistent with something being off either way: at 21:20:07 it reports the head as 78df4a3 with the changeset detected, and five seconds later (21:20:12) it reports the head as add874f2 with no changeset and only @modelcontextprotocol/examples + @modelcontextprotocol/test-conformance affected. If the intended head is the latter, the PR base/head should be made to actually reflect that (retarget the base or drop the feature commits) so the rendered diff and changed-files list stop showing the packages/client changes; if the intended head is the former (the diff as currently rendered), the description, title, and 'Types of changes' are wrong and must be rewritten as a behavior-changing minor release. Either way the PR as currently presented is internally inconsistent and needs reconciling before merge — this comment is not asserting which resolution is correct, only that one of them must happen.\n\nStep-by-step proof of the contradiction. (1) Open the changed-files tab: 18 files, including five packages/client/src/test files and .changeset/auth-sep-2352-credential-isolation.md. (2) Open that changeset: it declares '@modelcontextprotocol/client': minor and describes the SEP-2352 behavior change, the deprecations, and the provider-surface changes. (3) Read the PR description: "no packages/ runtime change", 'Documentation update' only, 'Breaking Changes: None'. (4) There is no state of the world in which (1)–(2) and (3) are simultaneously accurate for the delta that will merge.\n\nHow to fix. Pick one: (a) rebase/retarget so this PR's delta really is the SEP-990 fixture + docs only (the SEP-2352 commits and this changeset land via their own reviewed PR), or (b) keep the diff and rewrite the title (feat(client): …), the summary, the 'Types of changes' (New feature, and arguably Breaking change given the providers' saveClientInformation removal and the new fail-closed throw path), and the Breaking Changes section to describe SEP-2352 — so the runtime change gets the review it warrants.
| const recordedIssuer = cachedState?.authorizationServerMetadata?.issuer ?? cachedState?.authorizationServerUrl; | ||
| if (recordedIssuer === undefined) { | ||
| if (provider.saveDiscoveryState !== undefined) { | ||
| throw new AuthorizationServerMismatchError( | ||
| 'discoveryState was not available on the callback leg; ensure your provider persists discoveryState alongside codeVerifier', | ||
| issuer | ||
| ); | ||
| } | ||
| console.warn( | ||
| '[mcp-sdk] OAuthClientProvider does not implement saveDiscoveryState()/discoveryState(); ' + | ||
| 'the SEP-2352 callback-leg authorization-server binding cannot be checked. ' + | ||
| 'Implement discoveryState (persist alongside codeVerifier) — see migration.md §SEP-2352.' | ||
| ); | ||
| } else if (!issuersMatch(recordedIssuer, issuer)) { | ||
| throw new AuthorizationServerMismatchError(recordedIssuer, issuer); |
There was a problem hiding this comment.
🔴 On the missing-discoveryState fail-closed path, AuthorizationServerMismatchError is constructed with a prose sentence ('discoveryState was not available on the callback leg; …') in the recordedIssuer slot — a typed public field documented (in authErrors.ts and the migration-SKILL.md error table) as the issuer recorded at redirect time — and the resulting message misleadingly claims the authorization server changed when the recorded state was simply lost. Consider a distinct message/constructor (or an optional recordedIssuer / dedicated error) for the lost-state case so the typed fields keep their documented semantics; the m2m expectedIssuer path also reuses this class whose message and JSDoc speak of a redirect/callback that doesn't exist in client_credentials flows.
Extended reasoning...
What the issue is. AuthorizationServerMismatchError is a new public error class whose constructor parameters are the typed, readonly fields recordedIssuer and currentIssuer. Its own JSDoc (authErrors.ts) documents recordedIssuer as "The issuer recorded in discoveryState() when the authorization redirect was issued", and the error table added to docs/migration-SKILL.md in this same PR advertises (recordedIssuer, currentIssuer) as fields consumers can inspect programmatically. But the fail-closed branch in authInternal (auth.ts:1149-1153) constructs the error as:
throw new AuthorizationServerMismatchError(
'discoveryState was not available on the callback leg; ensure your provider persists discoveryState alongside codeVerifier',
issuer
);so the recordedIssuer field carries an explanatory sentence rather than an issuer identifier.
How it manifests. The class composes its message from the two fields, so on this path the thrown error reads: "Authorization server changed between redirect and callback (redirected to "discoveryState was not available on the callback leg; ensure your provider persists discoveryState alongside codeVerifier", callback resolved "https://as-two.example.com\"); refusing to send authorization_code/code_verifier to a different token endpoint". That statement is factually wrong for this case — no authorization-server change was detected; the recorded state was simply absent — and the garbled "redirected to" clause obscures the actual remediation (persist discoveryState with the same durability as codeVerifier).
Step-by-step. (1) A provider implements saveDiscoveryState()/discoveryState() but the state doesn't survive the redirect round-trip (page navigation / fresh process). (2) The host calls finishAuth(params); auth() runs with authorizationCode set; cachedState is undefined, so recordedIssuer === undefined. (3) Because provider.saveDiscoveryState !== undefined, the gate throws AuthorizationServerMismatchError(<sentence>, issuer). (4) A host that follows the migration-SKILL table — e.g. catches the class and renders "expected ${e.recordedIssuer}, got ${e.currentIssuer}" or compares the two issuers — displays/compares a sentence where an issuer should be. The new test in auth.test.ts even pins this as observable public behavior: expect(err.recordedIssuer).toContain('discoveryState was not available on the callback leg').
Why nothing prevents it. TypeScript can't distinguish "issuer string" from "prose string", and the test asserts the current behavior rather than guarding the field's semantics. The fail-closed behavior is correct and worth keeping — only the error's shape is the problem.
Secondary instance. The m2m expectedIssuer-mismatch path (auth.ts:~1194 and the static-provider comment blocks in authExtensions.ts) reuses the same class. There both fields really are issuers, but the fixed message text ("between redirect and callback") and the class JSDoc ("thrown by auth() on the authorization-code callback leg") describe a redirect/callback that does not exist in client_credentials flows — another doc/behavior mismatch worth tightening while the class is new.
How to fix. Options, roughly in order of preference: (a) make recordedIssuer optional (string | undefined) and add a distinct message branch for the lost-state case ("discoveryState was not available on the callback leg; refusing to exchange the authorization code — ensure your provider persists discoveryState alongside codeVerifier"); (b) introduce a small dedicated error (e.g. MissingDiscoveryStateError extends OAuthClientFlowError) for the fail-closed case; or (c) at minimum, add a second constructor signature/static factory so the prose never lands in the typed field. Whichever is chosen, update the new test to assert the message rather than recordedIssuer, and consider broadening the class JSDoc/message wording to cover the expectedIssuer (non-redirect) usage. Doing this now is cheap; once the class ships as documented public API, changing field semantics becomes a breaking change.
| /** | ||
| * Saves the authorization server URL after RFC 9728 discovery. | ||
| * This method is called by {@linkcode auth} after successful discovery of the | ||
| * authorization server via protected resource metadata. | ||
| * Saves the resolved authorization-server **issuer**. Called after a successful | ||
| * token exchange (timing changed in v2: was post-discovery, now post-`saveTokens`). | ||
| * | ||
| * Providers implementing Cross-App Access or other flows that need access to | ||
| * the discovered authorization server URL should implement this method. | ||
| * | ||
| * @param authorizationServerUrl - The authorization server URL discovered via RFC 9728 | ||
| * @deprecated Superseded by the `issuer` stamp on stored tokens / client credentials | ||
| * (SEP-2352). {@linkcode auth} still **writes** this for back-compat with providers | ||
| * that read it (e.g. Cross-App Access), but the SDK never reads it. Prefer reading | ||
| * the `issuer` field on the value passed to {@linkcode saveTokens} / | ||
| * {@linkcode saveClientInformation}, or the `ctx.issuer` argument. | ||
| */ | ||
| saveAuthorizationServerUrl?(authorizationServerUrl: string): void | Promise<void>; |
There was a problem hiding this comment.
🔴 The JSDoc for saveAuthorizationServerUrl (and the matching sentence in docs/migration.md) claims the call timing changed to post-saveTokens (after a successful token exchange), but the only call site in authInternal() runs immediately after the issuer is resolved from discovery — before registration, token exchange, or any saveTokens call. The prose in both places should say the hook is still called post-discovery (now with the resolved issuer), since the pre-exchange timing is required for CrossAppAccessProvider to read the value back during prepareTokenRequest().
Extended reasoning...
What the bug is. The rewritten JSDoc for OAuthClientProvider.saveAuthorizationServerUrl (packages/client/src/client/auth.ts:407-408) says: "Called after a successful token exchange (timing changed in v2: was post-discovery, now post-saveTokens)." The same claim is repeated in docs/migration.md added by this PR: "The call timing changed in v2: it was post-discovery, it is now post-saveTokens (after a successful token exchange)." Neither claim matches the implementation in this same diff.
The actual code path. The only call site of saveAuthorizationServerUrl in the SDK is in authInternal() at packages/client/src/client/auth.ts:1134:
const issuer = metadata?.issuer ?? String(authorizationServerUrl);
const infoCtx: OAuthClientInformationContext = { issuer };
// Deprecated write-only hook, kept for providers (e.g. Cross-App Access) that read it
// internally. The SDK never reads `authorizationServerUrl()`.
await provider.saveAuthorizationServerUrl?.(issuer);This executes immediately after the issuer is resolved from discovery — before the SEP-2352 callback-leg gate, before clientInformation()/registration, before fetchToken()/refreshAuthorization(), and before any saveTokens() call. There is no second post-token-exchange call site (a grep for saveAuthorizationServerUrl in packages/client/src finds only the interface declaration, the CrossAppAccessProvider implementation, and this one call).
Why the documented timing cannot be right. The pre-exchange timing is in fact required: CrossAppAccessProvider reads the saved value back inside prepareTokenRequest(), which runs during fetchToken() (it throws "Authorization server URL not available. Ensure auth() has been called first." if the value is missing). If the hook really fired post-saveTokens, the Cross-App Access flow — including the auth/cross-app-access-complete-flow conformance scenario this PR's SEP-990 fixture mapping relies on — would break. So the code is correct and only the prose is wrong.
Step-by-step proof. (1) auth() → authInternal() runs discovery and resolves issuer (auth.ts:1128-1130). (2) The very next awaited provider call is saveAuthorizationServerUrl?.(issuer) at line 1134. (3) Only afterwards does the flow reach the callback-leg gate, registration, and the token-exchange branch where fetchToken() calls provider.prepareTokenRequest() — which for CrossAppAccessProvider reads this._authorizationServerUrl populated in step (2). (4) saveTokens() happens last (auth.ts:1267). At no point is saveAuthorizationServerUrl invoked after saveTokens.
Impact. Docs-only — no runtime behavior is affected — but both inaccurate sentences are introduced by this PR, whose stated purpose is consolidating accurate authorization migration guidance. A provider author relying on the documented post-token-exchange timing (e.g. treating the call as a token-exchange-success signal, or assuming stored tokens already exist when the hook fires) would be misled; an implementer of a Cross-App-Access-style provider might also wrongly conclude the hook fires too late to feed prepareTokenRequest().
Fix. Reword both places to describe what the code does: the hook is still called post-discovery (the only change in v2 is that it now receives the resolved issuer — RFC 8414-validated metadata issuer, falling back to the discovery URL — rather than the raw discovery URL), and remains write-only/deprecated as the rest of the JSDoc already says.
| * treat them as untrusted input when displaying or logging (this includes | ||
| * `requiredScope`, which appears in the error message). | ||
| */ | ||
| /** | ||
| * Thrown by `auth()` on the authorization-code callback leg when the | ||
| * authorization server resolved by discovery differs from the one recorded in | ||
| * `discoveryState()` at redirect time. The `authorization_code` and PKCE | ||
| * `code_verifier` are bound to the AS that minted the code (RFC 7636); sending | ||
| * them to a different AS's token endpoint is a credential-exfiltration vector. | ||
| * | ||
| * This is the only runtime check left in the SEP-2352 model — stored tokens and | ||
| * client credentials are protected structurally by the `issuer` stamp instead. | ||
| */ | ||
| export class AuthorizationServerMismatchError extends OAuthClientFlowError { | ||
| constructor( | ||
| /** The issuer recorded in `discoveryState()` when the authorization redirect was issued. */ | ||
| public readonly recordedIssuer: string, | ||
| /** The issuer resolved by discovery on this call. */ | ||
| public readonly currentIssuer: string | ||
| ) { | ||
| super( | ||
| `Authorization server changed between redirect and callback ` + | ||
| `(redirected to ${JSON.stringify(recordedIssuer)}, callback resolved ${JSON.stringify(currentIssuer)}); ` + | ||
| `refusing to send authorization_code/code_verifier to a different token endpoint` | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| export class InsufficientScopeError extends OAuthClientFlowError { |
There was a problem hiding this comment.
🟡 The new AuthorizationServerMismatchError class (with its own doc block) was inserted between the existing JSDoc for InsufficientScopeError and the export class InsufficientScopeError declaration, so that JSDoc is now orphaned and InsufficientScopeError renders as undocumented in TypeDoc/IDE hover. Move the new class (with its doc) below InsufficientScopeError — or above its doc comment — so each doc block stays adjacent to its class.
Extended reasoning...
What the issue is. In packages/client/src/client/authErrors.ts, the diff hunk @@ -134,6 +134,31 @@ inserts the new AuthorizationServerMismatchError doc block + class at exactly the point between the closing */ of the pre-existing JSDoc for InsufficientScopeError and the export class InsufficientScopeError declaration. After this PR the file reads: [InsufficientScopeError JSDoc] */ → /** [AuthorizationServerMismatchError JSDoc] */ → export class AuthorizationServerMismatchError {...} → export class InsufficientScopeError {...} (lines ~117–162).
Why it matters. JSDoc/TSDoc tooling (TypeDoc, IDE hover, api-extractor) attaches a doc comment to the declaration that immediately follows it. The orphaned InsufficientScopeError block is now immediately followed by another doc block, so it documents nothing, and export class InsufficientScopeError at line 162 has no adjacent doc comment at all — it will show up as undocumented in hover and generated docs. The lost documentation is not trivial: it explains the 403 insufficient_scope challenge semantics, the onInsufficientScope interaction, the "does not extend OAuthError" note, and the security-relevant guidance that all fields (including requiredScope, which appears in the error message) originate from the WWW-Authenticate header and must be treated as untrusted input when displayed or logged.
Step-by-step proof. (1) Open packages/client/src/client/authErrors.ts at the current head of this branch. (2) Lines ~117–136 are the original InsufficientScopeError JSDoc ("Thrown by the HTTP client transport when the server responds with 403 Forbidden ... treat them as untrusted input ..."). (3) Lines ~137–146 are the new doc block ("Thrown by auth() on the authorization-code callback leg ..."), followed by export class AuthorizationServerMismatchError at ~147. (4) export class InsufficientScopeError at ~162 is preceded only by the new class's closing brace and a blank line. (5) Hover over InsufficientScopeError in an IDE (or run TypeDoc): no documentation is shown — the original block is dropped because a doc comment can only attach to the declaration immediately after it.
Why nothing catches it. The orphaned comment is syntactically valid, the new class compiles and is correctly documented itself, and no lint rule or doc-coverage check in the repo flags a doc comment that precedes another doc comment instead of a declaration. Only generated documentation output and IDE hover reveal the regression.
Impact. Documentation/structure only — no runtime behavior changes. AuthorizationServerMismatchError itself is fine; only InsufficientScopeError (a public exported error class) loses its docs, including its untrusted-input security note.
Fix. Move the new AuthorizationServerMismatchError doc block + class either below the InsufficientScopeError class or above the InsufficientScopeError JSDoc, so each doc block is immediately followed by the class it documents. A one-edit fix with no semantic change.
4839347b9Two closeout pieces with no
packages/runtime change:auth/enterprise-managed-authorization(SEP-990) to the existingrunCrossAppAccessCompleteFlowconformance handler — the SDK already implements SEP-990 viaCrossAppAccessProvider(Implement SEP-990 Enterprise Managed OAuth #1593); only the fixture mapping was missing.docs/migration.md(one bullet per consumer-side obligation the SDK structurally cannot enforce, each cross-referencing the example that demonstrates the conformant pattern) and adds theauth_oauthClientProvider+auth_finishAuthsnippet regions toclientGuide.examples.ts/docs/client.md. Closes Cannot find module '@modelcontextprotocol/sdk/client' or its corresponding type declarations #67.Motivation and Context
SEP-990 (issue modelcontextprotocol/modelcontextprotocol#990) is already implemented in the SDK; this only wires the conformance fixture. The doc consolidation closes out M13.1 so
auth/*conformance is green andmigration.mdcarries the single consumer checklist (PRD user story 24).How Has This Been Tested?
npm run test:conformance:client:alland:2026—auth/*fully green on both legs (the only remaining client baseline entry is the unrelatedjson-schema-ref-no-deref).pnpm sync:snippets --checkconfirms the new guide regions are in sync.Breaking Changes
None.
Types of changes
Checklist
Additional context
The conformance referee pin is unchanged in this PR — the integration branch already adopted
@modelcontextprotocol/conformance@0.2.0-alpha.5via #2335, which carries the RFC 8414 §3.3-compliant backcompat mock from conformance#359. With that referee, PR 2's issuer-echo check is default-ON andauth/2025-03-26-oauth-metadata-backcompatpasses; this PR just drops the now-stale expected-failures entries.migration-SKILL.mdis unchanged: every surface delta is additive-optional (finishAuth(code)still compiles), so there is no mechanical mapping.