feat(client): name AuthOptions; add iss/skipIssuerMetadataValidation and OAuthClientInformationContext#2343
feat(client): name AuthOptions; add iss/skipIssuerMetadataValidation and OAuthClientInformationContext#2343felixweinberger wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 874ca25 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| --- | ||
| '@modelcontextprotocol/client': minor | ||
| --- | ||
|
|
||
| Add the public surface for the 2026-07-28 authorization requirements. New `AuthOptions` type names the `auth()` options object and adds `iss` and `skipIssuerMetadataValidation` fields. `OAuthClientProvider.clientInformation()` and `.saveClientInformation()` accept an optional `OAuthClientInformationContext` carrying the authorization server's `issuer` so providers can key persisted credentials per authorization server. All changes are additive — existing `OAuthClientProvider` implementations compile unchanged; the new fields are inert until the behavior changes that follow wire them up. |
There was a problem hiding this comment.
🔴 The PR description claims OAuthClientMetadataSchema gains application_type?: 'native' | 'web' and AuthorizationServerMetadata gains authorization_response_iss_parameter_supported?: boolean, but neither field appears anywhere in this diff or in the repo at this commit. Since this PR is meant to be the complete surface delta for the follow-up behavior PRs, either add the schema fields (which would also need a @modelcontextprotocol/core changeset) or correct the description.
Extended reasoning...
What the description promises vs. what the diff ships
The PR description states: "OAuthClientMetadataSchema gains application_type?: 'native' | 'web'; AuthorizationServerMetadata gains authorization_response_iss_parameter_supported?: boolean." However, the diff touches only four files — .changeset/auth-surface-delta.md, docs/migration.md, packages/client/src/client/auth.ts, and packages/client/src/index.ts — none of which define those schemas. The schema types named in the description (OAuthClientMetadataSchema, AuthorizationServerMetadata) live in @modelcontextprotocol/core, and no core file is modified.
Verification
At HEAD (0fdbe45, the PR commit), grepping the entire packages/ tree for application_type and authorization_response_iss_parameter_supported returns zero matches. So the fields are not merely added elsewhere in the stack — they do not exist in the repo at all after this PR. The new docs/migration.md section even lists "DCR application_type" among the 2026-07-28 requirements this surface covers, reinforcing the expectation that the field ships here.
Why this matters more than a typical description nit
The PR's entire stated purpose is to be the complete additive surface delta for the M13.1 auth bundle — "it introduces every public-API field and type the behavior PRs depend on so that each subsequent PR carries only behavior," and so that OAuthClientProvider implementers (Claude Code is the named consumer) "can be reviewed against the final shape once." Two announced surface additions being absent defeats that goal:
- Reviewers and the named downstream consumer are misled about the final API shape they are supposed to be reviewing/coding against.
- If the fields were intended but dropped from the commit, the changeset is also incomplete: the existing changeset bumps only
@modelcontextprotocol/client(minor), butapplication_typeandauthorization_response_iss_parameter_supportedwould land in@modelcontextprotocol/coreschemas, which gets no changeset entry. - The follow-up behavior PRs that wire up RFC 9207
issvalidation and DCRapplication_typewill then have to smuggle surface changes in alongside behavior — exactly what the stack structure was designed to avoid.
Step-by-step proof
- Read the PR description: it explicitly lists four surface additions —
AuthOptions(withiss/skipIssuerMetadataValidation),OAuthClientInformationContext,OAuthClientMetadataSchema.application_type, andAuthorizationServerMetadata.authorization_response_iss_parameter_supported. - Inspect the diff file list:
.changeset/auth-surface-delta.md,docs/migration.md,packages/client/src/client/auth.ts,packages/client/src/index.ts. The first two surface items are present inauth.ts/index.ts; no schema file is touched. - Run
git rev-parse HEAD→0fdbe45899d…, the exact commit the changeset bot reports for this PR. - Run
grep -r 'application_type\|authorization_response_iss_parameter_supported' packages/→ no matches anywhere in the repo. - Conclusion: two of the four announced surface additions are not delivered by this PR and do not pre-exist elsewhere.
How to fix
Either (a) add application_type?: 'native' | 'web' to OAuthClientMetadataSchema and authorization_response_iss_parameter_supported?: boolean to the AuthorizationServerMetadata/OAuth metadata schema in @modelcontextprotocol/core, plus a @modelcontextprotocol/core changeset entry — keeping the surface PR genuinely complete; or (b) correct the PR description (and, if applicable, the migration-guide framing) to drop the two fields and note which follow-up PR in the stack will introduce them.
There was a problem hiding this comment.
Thanks for the update — the PR now does ship core changes (the issuer stamp fields on OAuthTokensSchema/OAuthClientInformationSchema) and the changeset now bumps @modelcontextprotocol/core (minor), which resolves the missing-core-changeset part of this finding.
However, the two specific surface additions called out here are still absent at HEAD (f6fb29c): grepping packages/ for application_type and authorization_response_iss_parameter_supported still returns zero matches — OAuthClientMetadataSchema and OAuthMetadataSchema in packages/core/src/shared/auth.ts were touched but neither field was added — while the PR description still announces both. To close this out, either add application_type?: 'native' | 'web' to OAuthClientMetadataSchema and authorization_response_iss_parameter_supported?: boolean to the AS metadata schema (the core changeset entry now exists, so they'd be covered), or update the PR description to drop those two items and note which follow-up PR in the stack introduces them. (The description also doesn't yet mention the new issuer stamp fields, the ctx parameter on tokens()/saveTokens(), or OAuthClientFlowError, so it likely needs a refresh either way.)
felixweinberger
force-pushed
the
fweinberger/auth-0-surface
branch
from
0fdbe45 to
f6fb29c
Compare
felixweinberger
force-pushed
the
fweinberger/auth-0-surface
branch
from
f6fb29c to
d1ffd2f
Compare
| export interface OAuthClientInformationContext { | ||
| /** | ||
| * The authorization server's `issuer` identifier from its validated metadata | ||
| * document, used as the binding key for persisted credentials. | ||
| */ | ||
| issuer: string; | ||
| } |
There was a problem hiding this comment.
🟡 Naming nit: OAuthClientInformationContext is the ctx parameter type for all four credential-persistence methods — including tokens(ctx?) and saveTokens(tokens, ctx?) — so the "ClientInformation" name describes only half of its usage. Since this surface PR freezes the exported name in a minor release (renaming later is a breaking change), consider a usage-accurate name now, e.g. OAuthCredentialContext or OAuthIssuerContext.
Extended reasoning...
The naming mismatch. The new exported interface is named OAuthClientInformationContext, but its own JSDoc states it is the "Context passed to the credential-persistence methods on OAuthClientProvider — clientInformation / saveClientInformation and tokens / saveTokens." The diff confirms it appears as the ctx? parameter on all four methods (packages/client/src/client/auth.ts), and both the changeset and the new docs/migration.md section ('OAuthClientProvider credential methods receive an issuer context') describe it the same way. So the type that names client-information persistence is also the context for token persistence — half its usage doesn't match its name.
Why the name is genuinely confusing rather than just imprecise. In this codebase, "ClientInformation" specifically denotes the RFC 7591 dynamic-client-registration record (OAuthClientInformation / OAuthClientInformationMixed / OAuthClientInformationFullSchema). An implementer reading tokens(ctx?: OAuthClientInformationContext) will reasonably wonder why a client-information record's context is being supplied for token lookup, when in fact the type carries only { issuer: string } — an authorization-server binding key that is equally relevant to tokens and client credentials.
Why this is the right moment to raise it. This PR's explicit purpose is to freeze the public surface so downstream OAuthClientProvider implementers (Claude Code is the named consumer) can code against the final shape before the behavior PRs land. The type is exported from the package barrel (packages/client/src/index.ts) and ships in a minor release of @modelcontextprotocol/client. Once published, the name is frozen public API: renaming it later is a breaking change (or requires a deprecated alias kept around indefinitely). Reviewing names is exactly what a surface-only PR is for, and this is the only cheap point at which to fix it.
Concrete walk-through. 1) An implementer following the migration guide adds per-AS storage: saveTokens(tokens: OAuthTokens, ctx?: OAuthClientInformationContext). 2) Reading the signature alone, the type name suggests the context relates to the registered client-information record, not the token set. 3) They (or a reviewer) have to open the interface definition to learn it is just { issuer: string } shared by all four persistence methods. 4) After the minor release ships, every such implementation imports OAuthClientInformationContext by name; renaming it in the SDK would break those imports, so the misnomer becomes permanent.
Why nothing else catches this. It is purely a naming concern — typecheck, lint, and the barrelClean test all pass, and there is no behavioral impact, so no test will ever flag it. Only review of the surface PR can.
How to fix. Rename the interface to something covering all four methods before merging — OAuthCredentialContext, OAuthIssuerContext, or OAuthProviderContext — and update the four ctx? parameter types, the barrel export, the changeset, and the migration.md prose accordingly. This is a nit: it should not block the PR on its own, but it is far cheaper to address now than after publication.
felixweinberger
force-pushed
the
fweinberger/auth-0-surface
branch
from
d1ffd2f to
ec501fa
Compare
…, authErrors module Surface delta for the 2026-07-28 authorization requirements. All additive; existing OAuthClientProvider implementers compile unchanged. The new fields are inert until the behavior changes that follow wire them up. - Extract the inline auth() options object as exported AuthOptions and add iss (RFC 9207 callback parameter) and skipIssuerMetadataValidation (RFC 8414 §3.3 opt-out). JSDoc is non-assertive — validation lands in the follow-up commit. - OAuthClientProvider.clientInformation/saveClientInformation/tokens/ saveTokens accept an optional OAuthClientInformationContext carrying the resolved authorization-server issuer so providers can key persisted credentials per AS. - OAuthTokens and OAuthClientInformation gain an optional issuer stamp field (core/shared/auth.ts) — the slot the SDK writes before persistence so stored credentials are bound to the AS that issued them. - New packages/client/src/client/authErrors.ts with the OAuthClientFlowError base class; the flow-specific error classes from later commits land here. Claude-Session: https://claude.ai/code/session_01XBib5gRe8AMPPJhySCz3EJ
felixweinberger
force-pushed
the
fweinberger/auth-0-surface
branch
from
ec501fa to
874ca25
Compare
| /** | ||
| * {@linkcode OAuthTokens} as persisted by an `OAuthClientProvider`. Adds an | ||
| * SDK-stamped authorization-server `issuer` identifier so stored tokens are | ||
| * bound to the AS that issued them. The `issuer` field is **not** part of the | ||
| * RFC 6749 wire response and is intentionally absent from | ||
| * {@linkcode OAuthTokensSchema}; the client SDK writes it before calling | ||
| * `saveTokens`. | ||
| */ | ||
| export type StoredOAuthTokens = OAuthTokens & { issuer?: string }; | ||
|
|
||
| /** | ||
| * {@linkcode OAuthClientInformationMixed} as persisted by an | ||
| * `OAuthClientProvider`. Adds an SDK-stamped authorization-server `issuer` | ||
| * identifier so stored client credentials are bound to the AS that issued them. | ||
| * The `issuer` field is **not** part of the RFC 7591 wire response and is | ||
| * intentionally absent from {@linkcode OAuthClientInformationSchema}; the | ||
| * client SDK writes it before calling `saveClientInformation`. | ||
| */ | ||
| export type StoredOAuthClientInformation = OAuthClientInformationMixed & { issuer?: string }; |
There was a problem hiding this comment.
🟡 Documentation nit (two residual present-tense claims after the earlier softening pass): the new StoredOAuthTokens / StoredOAuthClientInformation JSDoc says "the client SDK writes it before calling saveTokens / saveClientInformation", but nothing in this PR stamps an issuer before persistence — and the new migration.md intro says implementer obligations "are listed under" the Conformance obligations section, which currently contains only an HTML comment placeholder. Suggest rewording the JSDoc to the future-tense / "currently inert" framing already used in migration.md and the changeset, and either softening the cross-reference or adding a visible one-line "populated as the behavior PRs land" note under the heading.
Extended reasoning...
What the issue is. Two spots of prose still promise behavior or content this PR does not ship, after the analogous claims elsewhere were already softened:
- Core JSDoc on the new
Stored*types (packages/core/src/shared/auth.ts:248-266): bothStoredOAuthTokensandStoredOAuthClientInformationsay theissuerfield is one "the client SDK writes ... before callingsaveTokens" / "before callingsaveClientInformation" — present tense. - migration.md cross-reference (line ~1479): the new "Authorization (2026-07-28 spec)" intro says the implementer-side obligations "are listed under [Conformance obligations for
OAuthClientProviderimplementers]", but the linked section (lines 1494-1496) contains only<!-- Filled in as the SEP-2468/2352/2350/837/2207 behavior PRs land. -->, which does not render — so the published guide points readers at an empty heading with no visible deferral note.
The code path that contradicts the JSDoc. authInternal() in packages/client/src/client/auth.ts calls provider.saveClientInformation(fullInformation) and provider.saveTokens(tokens) / saveTokens(newTokens) with the parsed wire objects unmodified; fetchToken() and registerClient() likewise never set an issuer field. The PR description and the changeset both confirm this explicitly: "No spec MUST is implemented in this PR — all new fields are inert."
Step-by-step proof.
- A provider author hovers
StoredOAuthTokensin their editor (the surface implementers actually see) and reads "the client SDK writes it before callingsaveTokens". - Following that, they key persisted token storage on
tokens.issuerin theirsaveTokensimplementation. - They run the OAuth flow against this minor release:
authInternal()callsprovider.saveTokens(tokens)wheretokens.issuerisundefined— nothing stamped it. - Every authorization server's tokens collapse into the
undefinedstorage key; the per-AS isolation the docs promise is silently defeated, with no type error or test failure to surface it. - Separately, an implementer following the migration-guide intro clicks the "Conformance obligations" link and lands on a heading with no content and no explanation, since the deferral note is an HTML comment that does not render.
Why this isn't already covered. The same overstated-prose problem was raised in earlier review rounds for the AuthOptions.iss JSDoc and the migration.md issuer-stamping sentences, and those were softened to "currently inert / once the SEP-2352 behavior change lands". The Stored* JSDoc was added in a later push and never received the same caveat, so it is the one remaining editor-visible surface still claiming the stamping happens in this release; the empty conformance section was likewise not part of any prior comment.
Why it matters. The changeset cuts a minor release of @modelcontextprotocol/client and @modelcontextprotocol/core, so both the JSDoc and the migration guide ship to npm/docs immediately, and Claude Code is the named downstream consumer of exactly these provider interfaces. The deferral itself is clearly intentional (stacked-PR placeholder) — this is purely a wording/visibility issue, hence nit severity.
How to fix. (a) Reword the two Stored* JSDoc blocks to match the migration.md/changeset framing, e.g. "...intentionally absent from OAuthTokensSchema; once the SEP-2352 behavior change lands, the client SDK will write it before calling saveTokens. The field is currently inert." (b) Either change the intro to "will be listed under ... as the behavior PRs land", or replace the HTML comment under the Conformance-obligations heading with a visible one-liner ("This list will be populated as the SEP-2468/2352/2350/837/2207 behavior PRs land.").
| */ | ||
| authorizationCode?: string; | ||
| /** | ||
| * The form-urldecoded `iss` query parameter from the authorization callback, | ||
| * if present. Passed through to RFC 9207 §2.4 issuer validation alongside | ||
| * `authorizationCode`. The validation behavior is wired up in a follow-up | ||
| * change; this field is currently inert. | ||
| */ | ||
| iss?: string; |
There was a problem hiding this comment.
🔴 AuthOptions.iss gives auth() callers a way to forward the RFC 9207 callback parameter, but the documented entry point for completing the authorization-code callback — StreamableHTTPClientTransport.finishAuth(code) / SSEClientTransport.finishAuth(code) — is unchanged and calls auth() with no way to inject iss, so transport-based clients have no path to RFC 9207 mix-up protection. Since this PR is meant to be the complete surface delta, consider extending finishAuth (e.g. finishAuth(code, { iss }?: Pick<AuthOptions, 'iss'>)) here, or documenting that transport users must call auth() directly — otherwise the SEP-2468 behavior PR will need its own public-surface change anyway.
Extended reasoning...
What the gap is. This PR adds iss?: string to the new exported AuthOptions (packages/client/src/client/auth.ts:580-588) so callers of auth() can hand the RFC 9207 iss callback parameter to the SDK. But the documented user-facing path for completing the authorization-code callback is not auth() — it is transport.finishAuth(code). Both StreamableHTTPClientTransport.finishAuth(authorizationCode: string) (packages/client/src/client/streamableHttp.ts:690) and SSEClientTransport.finishAuth(authorizationCode: string) (packages/client/src/client/sse.ts:232) are untouched by this PR and call auth(provider, { serverUrl, authorizationCode, resourceMetadataUrl, scope, fetchFn }) — there is no way to inject iss.
Why the transport flow is the one that matters. docs/client.md:195-196 explicitly instructs OAuth users: catch UnauthorizedError, complete the browser flow, then "call transport.finishAuth(code)" and reconnect. So the transport path is the primary documented flow for exactly the kind of consumer the PR description names (Claude Code as an OAuthClientProvider implementer). And iss only exists on the redirect callback URL, which the application receives — neither the transport nor the provider can derive it on their own. If the application can't pass it through finishAuth, the value can never reach authInternal().
Why this is a completeness gap in this PR rather than a follow-up concern. The PR's stated purpose is to be the complete additive surface ("introduces every public-API field and type the behavior PRs depend on so that each subsequent PR carries only behavior"), so the final shape can be reviewed once. With the surface as shipped, when the SEP-2468 behavior PR wires up RFC 9207 validation, one of two things must happen: (a) finishAuth's public signature is extended in a behavior PR — exactly the surface change this PR exists to front-load — or (b) transport users silently get no AS mix-up protection because iss has no path into the flow, while direct auth() callers do.
Step-by-step proof.
- A transport-based client (per
docs/client.md) callsclient.connect(transport), catchesUnauthorizedError, and the user completes the browser authorization flow. - The authorization server redirects back with
?code=...&state=...&iss=https://as.example(RFC 9207). The application's redirect handler holds bothcodeandiss. - The app calls
transport.finishAuth(code)— the only parameter the method accepts.isshas nowhere to go. finishAuthcallsauth(this._oauthProvider, { serverUrl, authorizationCode, resourceMetadataUrl, scope, fetchFn })(streamableHttp.ts:695-701,sse.ts:237-243) with noissfield.- When the SEP-2468 behavior lands and
authInternal()validatesoptions.issagainst the recorded issuer, this code path always seesiss === undefined— the validation either never runs or must treat "absent" as pass — so the documented primary flow gets no mix-up protection, even though the surface was supposedly frozen for it.
Why nothing else catches this. The existing review comments cover overstated/inert prose, missing schema fields announced in the description, the unused OAuthClientFlowError export, the wire-schema issuer concern (now addressed via the Stored* types), and a naming nit — none of them touch the missing iss path through finishAuth. Typecheck/lint can't flag it because nothing is wrong with the types; this is a hole in the announced surface, visible only by tracing the documented call path.
How to fix. Add an optional second parameter to both transports' finishAuth in this PR — e.g. finishAuth(authorizationCode: string, options?: Pick<AuthOptions, 'iss'>) — and forward options?.iss into the auth() call (it stays inert until the behavior PR, matching the rest of the surface). Alternatively, if the intent is that transport users call auth() directly when they need RFC 9207 validation, state that explicitly in the migration guide / docs/client.md so the omission is a documented decision rather than an accidental gap.
1 participant
d57c32e46Surface delta for the 2026-07-28 authorization requirements. Extracts the inline
auth()options object as the exportedAuthOptionstype and adds two fields:iss(the RFC 9207 callback parameter) andskipIssuerMetadataValidation(the RFC 8414 §3.3 opt-out).OAuthClientProvider.clientInformation()and.saveClientInformation()accept an optionalOAuthClientInformationContextcarrying the resolved authorization-server issuer so providers can key persisted credentials per AS.OAuthClientMetadataSchemagainsapplication_type?: 'native' | 'web';AuthorizationServerMetadatagainsauthorization_response_iss_parameter_supported?: boolean.Motivation and Context
This is the additive-only surface PR for the M13.1 auth bundle (SEP-2468 / SEP-2352 / SEP-2350 / SEP-837 / SEP-2207 — see docs/2026-06-19-m13-auth-bundle-prd.md). It introduces every public-API field and type the behavior PRs depend on so that each subsequent PR carries only behavior, and so that existing
OAuthClientProviderimplementers (Claude Code is the named consumer) can be reviewed against the final shape once.No spec MUST is implemented in this PR — all new fields are inert.
How Has This Been Tested?
pnpm typecheck:all,pnpm lint:all,pnpm --filter @modelcontextprotocol/client test. The barrelClean test confirms the new exports do not pull Node-only modules into the runtime-neutral entry. No behavior tests change.Breaking Changes
None. All new fields are optional; existing
OAuthClientProviderimplementations andauth()call sites compile unchanged.Types of changes
Checklist
Additional context
First PR in a 7-PR stack. The five behavior PRs that follow wire each field up; the closeout PR consolidates
migration.mdand the client guide. Reviewing the surface in isolation makes the additive-optional invariant (PRD §Implementation Decisions, "Surface delta") auditable independently of behavior.