Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/auth-sep-2352-credential-isolation.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@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()`.
Comment on lines +1 to +5

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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.

3 changes: 3 additions & 0 deletions docs/migration-SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ The OAuth client flow additionally throws dedicated classes from `@modelcontextp
| `exchangeAuthorization()` / `refreshAuthorization()` / `fetchToken()` / `requestJwtAuthorizationGrant()` / `exchangeJwtAuthGrant()` non-https token endpoint | `InsecureTokenEndpointError` (`tokenEndpoint`) |
| RFC 9207 `iss` mismatch / RFC 8414 §3.3 issuer-echo mismatch | `IssuerMismatchError` (`kind`, `expected`, `received`) |
| Transport 403 `insufficient_scope` with `onInsufficientScope: 'throw'`, or default mode without an `OAuthClientProvider` | `InsufficientScopeError` (`requiredScope`, `resourceMetadataUrl`, `errorDescription`) |
| `auth()` callback leg: discovery resolves a different AS than the recorded redirect target | `AuthorizationServerMismatchError` (`recordedIssuer`, `currentIssuer`) |

Update OAuth error handling:

Expand Down Expand Up @@ -595,6 +596,8 @@ OAuth callback handling: pass the callback URL's `URLSearchParams` to `transport

Token-exchange / refresh now refuse to send credentials to a non-`https:` token endpoint (loopback `localhost` / `127.0.0.1` / `::1` exempt), throwing `InsecureTokenEndpointError` with no opt-out. `auth()` surfaces this on every path including refresh — switch any plain-`http:` AS on a non-loopback host to TLS.

`auth()` stamps an `issuer` field onto every value it passes to `saveTokens()` / `saveClientInformation()` and threads `{ issuer }` as the `ctx` argument to those methods plus `tokens()` / `clientInformation()` (SEP-2352). On read, a stored value whose `issuer` names a different AS is treated as `undefined` and the flow re-registers / re-authorizes. Round-trip the stored object verbatim and you're protected; multi-AS providers key storage on `ctx.issuer`. `OAuthClientProvider.saveAuthorizationServerUrl()` / `authorizationServerUrl()` are `@deprecated` (still written for back-compat, never read by the SDK). `ClientCredentialsProvider` / `PrivateKeyJwtProvider` / `StaticPrivateKeyJwtProvider` / `CrossAppAccessProvider` gain `expectedIssuer?: string` and no longer define `saveClientInformation()`.

No code changes required; wire-behavior note: on a 2026-07-28 Streamable HTTP connection, aborting an in-flight client request (caller `signal` / timeout) closes that request's SSE response stream as the spec cancellation signal — `notifications/cancelled` is no longer POSTed
there. 2025-era connections and stdio at any era still send `notifications/cancelled`. Custom `Transport` implementations that open one underlying request per outbound message and honor `TransportSendOptions.requestSignal` may declare `readonly hasPerRequestStream = true` to opt
into the same routing.
Expand Down
18 changes: 18 additions & 0 deletions docs/migration.md
Original file line number Diff line number Diff line change
Expand Up @@ -1560,10 +1560,28 @@

The GET listen-stream open path now applies the same step-up handling as the POST send path.

### Credentials are bound to the issuing authorization server (SEP-2352)

`auth()` now stamps an `issuer` field onto every value it passes to `saveTokens()` and `saveClientInformation()`, and passes `{ issuer }` as the second (`ctx`) argument to `tokens()` / `saveTokens()` / `clientInformation()` / `saveClientInformation()`. On read, a stored value whose `issuer` stamp names a different authorization server is treated as `undefined` — the flow re-registers / re-authorizes exactly as if nothing were stored. A `client_id` or `refresh_token` issued by one authorization server is therefore never sent to another.

**Round-trip the stored object unchanged and you're protected** — single-slot storage works. To hold credentials for several authorization servers at once, key your storage on `ctx.issuer` instead (and treat **`ctx === undefined` as "return the most-recently-saved token set"** — the transport's per-request `Authorization: Bearer` read (`adaptOAuthProvider().token()`) calls `tokens()` with no `ctx`).

Implement `discoveryState()` / `saveDiscoveryState()` so the callback leg can verify it is exchanging the authorization code at the same AS the redirect targeted; without it the SDK `console.warn`s once per callback. **`discoveryState` must persist with the same durability as `codeVerifier`** — it has to survive the redirect round-trip (page navigation, app restart). A provider that implements `saveDiscoveryState()` but cannot return it on the callback leg fails closed with `AuthorizationServerMismatchError`. Hosts that drive 401 handling themselves (custom middleware / `withOAuthRetry`) should call `provider.invalidateCredentials?.('discovery')` on a fresh 401 so a changed `authorization_servers` list is picked up on re-discovery. The built-in `StreamableHTTPClientTransport` / `SSEClientTransport` 401 path does not currently invalidate discovery state.

`OAuthClientProvider.saveAuthorizationServerUrl()` / `authorizationServerUrl()` are **deprecated** — `auth()` still calls `saveAuthorizationServerUrl()` for back-compat with providers that read it internally (Cross-App Access), but the SDK never reads `authorizationServerUrl()`. The call timing changed in v2: it was post-discovery, it is now post-`saveTokens` (after a successful token exchange). Read the `issuer` stamp on stored tokens, or `ctx.issuer`, instead.

Check failure on line 1571 in docs/migration.md

View check run for this annotation

Claude / Claude Code Review

Docs/JSDoc claim saveAuthorizationServerUrl() is now called post-saveTokens, but code calls it post-discovery

The migration guide and the new JSDoc on `OAuthClientProvider.saveAuthorizationServerUrl()` claim the call timing changed to post-`saveTokens` (after a successful token exchange), but `authInternal()` actually calls `provider.saveAuthorizationServerUrl?.(issuer)` immediately after discovery resolves the issuer — before the SEP-2352 callback-leg gate, registration, and any token request. Either fix the prose/JSDoc to say the call remains post-discovery (now passing the resolved issuer instead of

The bundled `ClientCredentialsProvider`, `PrivateKeyJwtProvider`, `StaticPrivateKeyJwtProvider`, and `CrossAppAccessProvider` gain an `expectedIssuer` option that stamps the constructor-supplied credential; when set, `auth()` against any other authorization server fails before the credential leaves the process. These providers no longer define `saveClientInformation()`.

### Conformance obligations for `OAuthClientProvider` implementers

<!-- Filled in as the SEP-2352/2350/837/2207 behavior PRs land. -->

#### SEP-2352 — per-authorization-server credential isolation

**No code change required for the common case.** If your `saveTokens()` / `saveClientInformation()` persist the value passed to them verbatim and your `tokens()` / `clientInformation()` return it verbatim, the SDK-stamped `issuer` round-trips and the binding holds.

If you serialise to a custom format, persist the `issuer` field alongside the rest of the value. If you key storage by `ctx.issuer`, return `undefined` for an issuer you have no entry for, and treat **`ctx === undefined` as "return the most-recently-saved token set"** — the transport's per-request `Authorization: Bearer` read (`adaptOAuthProvider().token()`) calls `tokens()` with no `ctx`.

## Using an LLM to migrate your code

An LLM-optimized version of this guide is available at [`docs/migration-SKILL.md`](migration-SKILL.md). It contains dense mapping tables designed for tools like Claude Code to mechanically apply all the changes described above. You can paste it into your LLM context or load it as
Expand Down
35 changes: 32 additions & 3 deletions examples/oauth/simpleOAuthClientProvider.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,28 @@
import type { OAuthClientInformationMixed, OAuthClientMetadata, OAuthClientProvider, OAuthTokens } from '@modelcontextprotocol/client';
import type {
OAuthClientInformationMixed,
OAuthClientMetadata,
OAuthClientProvider,
OAuthDiscoveryState,
OAuthTokens
} from '@modelcontextprotocol/client';
import { validateClientMetadataUrl } from '@modelcontextprotocol/client';

/**
* In-memory OAuth client provider for demonstration purposes
* In production, you should persist tokens securely
* In-memory OAuth client provider for demonstration purposes.
* In production, you should persist tokens and client credentials securely.
*
* Tokens and client credentials are stored as single-slot blobs. The SDK stamps an
* `issuer` field onto every value it saves; round-tripping the blob unchanged means
* a credential issued by one authorization server is never reused at another (the
* SDK reads the stamp back as a key-not-found and re-registers / re-authorizes).
* To hold credentials for several authorization servers at once, key your storage
* on the `ctx.issuer` argument instead.
*/
export class InMemoryOAuthClientProvider implements OAuthClientProvider {
private _clientInformation?: OAuthClientInformationMixed;
private _tokens?: OAuthTokens;
private _codeVerifier?: string;
private _discoveryState?: OAuthDiscoveryState;

constructor(
private readonly _redirectUrl: string | URL,
Expand Down Expand Up @@ -66,4 +80,19 @@ export class InMemoryOAuthClientProvider implements OAuthClientProvider {
}
return this._codeVerifier;
}

saveDiscoveryState(state: OAuthDiscoveryState): void {
this._discoveryState = state;
}

discoveryState(): OAuthDiscoveryState | undefined {
return this._discoveryState;
}

invalidateCredentials(scope: 'all' | 'client' | 'tokens' | 'verifier' | 'discovery'): void {
if (scope === 'all' || scope === 'client') this._clientInformation = undefined;
if (scope === 'all' || scope === 'tokens') this._tokens = undefined;
if (scope === 'all' || scope === 'verifier') this._codeVerifier = undefined;
if (scope === 'all' || scope === 'discovery') this._discoveryState = undefined;
}
}
Loading
Loading