fix(client): deprecate omitting expectedIssuer and apply the SEP-2352 issuer check consistently - #2887
Conversation
… issuer check consistently Constructing ClientCredentialsProvider, PrivateKeyJwtProvider, StaticPrivateKeyJwtProvider or CrossAppAccessProvider without `expectedIssuer` is deprecated: the constructor logs a console.warn and that call signature is marked @deprecated. Nothing else says which authorization server constructor-supplied credentials belong to, so without it they go to whichever one the MCP server advertises. An empty or null value throws at construction. Docs, examples and tests pass it. SEP-2352 follow-ups: - auth() throws AuthorizationServerMismatchError instead of registering again when the stored client information is bound to a different authorization server and the provider implements addClientAuthentication(). That authentication belongs to the stored registration and executeTokenRequest() would still present it after the new registration. - fetchToken() throws AuthorizationServerMismatchError, before preparing or sending anything, when the client information is bound to a different authorization server than the one it is called with. It used to read the client information without the check. - OAuthTokensSchema and OAuthClientInformationSchema accept the optional `issuer`, so a provider that reads storage back through them keeps the stamp instead of losing it to .strip(). auth() overwrites it on every save. - A `null` clientInformation() counts as nothing stored. AuthorizationServerMismatchError's message no longer assumes the authorization-code callback, since static-credential providers meet it too. Its fields are unchanged.
🦋 Changeset detectedLatest commit: fa9a0c6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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/core
@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}
There was a problem hiding this comment.
Beyond the inline findings, I also checked the reordering in fetchToken() (the clientInformation() read now happens before prepareTokenRequest()): none of the SDK's built-in providers derive client information from prepareTokenRequest() side effects, so the earlier read does not change what is sent. I also checked the dual-mode example's new OAUTH_EXPECTED_ISSUER requirement against its runner and README — the variable is documented and set where the story is invoked.
Extended reasoning...
Findings-present ruled-out note only. The change touches the client OAuth path (auth.ts issuer-binding checks, provider constructors, core schemas), which is security-sensitive, and the inline findings already signal that a human should look.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🔴
packages/core/src/auth.ts— Consumers compiling withexactOptionalPropertyTypes: trueget new type errors on providers that type stored values with the wire types, which compiled on the base branch. Addingissuer: z.string().optional().catch(undefined)givesOAuthTokensandOAuthClientInformationanissuer?: string | undefinedkey. The aliases at packages/core/src/auth.ts:285 and :295 intersect that with{ issuer?: string }, which under exact optional types narrows toissuer?: string, soOAuthTokensis no longer assignable toStoredOAuthTokens. …Why this was flagged
…Fix: define both Stored aliases so the wire types stay assignable to them (e.g.
type StoredOAuthTokens = OAuthTokens, or& { issuer?: string | undefined }), and reword the JSDoc at :281-283 and :292-293, which still saysissueris intentionally absent from the schema.A consumer with
exactOptionalPropertyTypes: trueimplementsOAuthClientProviderthe way docs/clients/oauth.md:44-58 and examples/cli-client/host/auth.ts:45,57 do:tokens(): OAuthTokens | undefinedandclientInformation(): OAuthClientInformationMixed | undefined. The interface at packages/client/src/client/auth.ts:278 and :305 requiresStoredOAuthClientInformation/StoredOAuthTokens. On the base branchOAuthTokenshad noissuerkey, so it was assignable toOAuthTokens & { issuer?: string }. After the diff,OAuthTokensSchemaat packages/core/src/auth.ts:151 andOAuthClientInformationSchemaat :238 inferissuer?: string | undefined(Zod 4.optional()adds| undefined;.catchkeeps the inner optionality). The intersection at :285 / :295 makes the property `(string | undefined) &…Verification: normal — triggered when a downstream consumer compiles with
exactOptionalPropertyTypes: trueand types its provider'stokens()/clientInformation()with the wire types, exactly as the SDK's own docs and examples do (docs/clients/oauth.md:44,64-66 returnsOAuthTokens | undefined; examples/cli-client/host/auth.ts:45,53 and examples/oauth/simpleOAuthClientProvider.ts:53,61 declare… | normal —… -
🟡
test/conformance/src/everythingClient.ts— nit: the SDK's own conformance client still constructs three providers withoutexpectedIssuer, so every conformance auth run now logs the new deprecation warning the PR adds. checkedExpectedIssuer in packages/client/src/client/authExtensions.ts:100 warns on each such construction. Fix: migrate every leftover construction in the repo to passexpectedIssuer(or derive the AS issuer from the conformance context/discovery), which covers the 3 sites listed. Same instruction at 3 sites (test/conformance/src/everythingClient.ts:567, test/conformance/src/everythingClient.ts:600, test/conformance/src/everythingClient.ts:643). [also at: packages/client/src/client/authExtensions.ts:174 - nit: REVIEW.md Completeness asks to grep for surviving instances of a replaced pattern: this PR deprecates the no-expectedIssuerconstructor signature (this overload now warns on every construction) and migrates docs, examples, unit and e2e tests, but test/conformance/src/everythingClient.ts…; packages/client/src/client/authExtensions.ts:176 - nit: the SDK's own conformance client still uses the now-deprecated constructor signature, so every conformance run logs the new deprecation warning three times.]Why this was flagged
The PR deprecates omitting
expectedIssuerand updates docs, examples, unit and e2e tests to pass it, but test/conformance/src/everythingClient.ts:567 (PrivateKeyJwtProvider), :600 (ClientCredentialsProvider) and :643 (CrossAppAccessProvider) are not updated. Each now hits theexpectedIssuer === undefinedbranch of checkedExpectedIssuer at packages/client/src/client/authExtensions.ts:99-105 and prints '[mcp-sdk] OmittingexpectedIssueris deprecated...' to stderr on every conformance scenario run. On the base branch those constructions were silent. The conformance context schema at everythingClient.ts:43-78 carries no AS issuer field, so the harness would need to supply one or the client…Verification: nit. Trigger: any conformance client auth run (CI
pnpm run test:conformance:client:allin .github/workflows/conformance.yml:32-33 runs auth/client-credentials-jwt, auth/client-credentials-basic, auth/cross-app-access-complete-flow, auth/enterprise-managed-authorization). Mechanism verified: the diff addscheckedExpectedIssuerat packages/client/src/client/authExtensions.ts:102-113, which… -
🟡
packages/core/src/auth.ts— nit: readers of theStoredOAuthTokens/StoredOAuthClientInformationJSDoc are told the opposite of what this diff ships. packages/core/src/auth.ts:281-283 and :291-293 still sayissueris "intentionally absent from the wire-response schema", but the same diff addsissuertoOAuthTokensSchema(auth.ts:146) andOAuthClientInformationSchema(auth.ts:238). Fix: reword both JSDoc blocks so they match the new schemas (the field is now an optional, non-wire stamp the schemas accept andauth()overwrites on save), covering both aliases.Why this was flagged
A provider author reads the published typedoc for StoredOAuthTokens at packages/core/src/auth.ts:278-285 or StoredOAuthClientInformation at :287-295. Both comments state the issuer field is "intentionally absent from the wire-response schema". This diff adds
issuer: z.string().optional().catch(undefined)to OAuthTokensSchema at :146 and to OAuthClientInformationSchema at :238, so the claim is now false in the same file. On the base branch the comment and schema agreed. The migration guide (docs/migration/upgrade-to-v2.md:1284-1286) was updated to say the schemas keep the stamp, so the two public docs now contradict each other; nothing fails at runtime.Verification: nit. Documentation-only contradiction introduced by this diff. The diff adds
issuer: z.string().optional().catch(undefined)toOAuthTokensSchema(/home/claude/typescript-sdk/packages/core/src/auth.ts:153, not :146 as the candidate says — 146 is the hunk header) and toOAuthClientInformationSchema(:238), both with a new inline comment "Not part of the wire format ... added by the client's… -
🟡
packages/client/src/client/authExtensions.ts— nit: CLAUDE.md's formatting bullet says 2-space indentation; every added line in this diff (e.g. the newcheckedExpectedIssuerbody, the constructor overloads, the schema fields, test blocks) is indented with 4 spaces. The repository's.prettierrc.jsonsetstabWidth: 4andpnpm lint:allenforces it, so the CLAUDE.md bullet appears stale rather than the code wrong. Fix: reconcile the two — most likely update the CLAUDE.md bullet to 4-space so it matches.prettierrc.json; do not reindent the code, which would fail Prettier.Why this was flagged
Nothing fails at runtime and nothing fails lint: the added code matches the enforced Prettier configuration (tabWidth 4). The only issue is the contradiction between the CLAUDE.md formatting bullet and the formatter config; filed for completeness so the maintainers can fix the stale guidance.
Verification: Root CLAUDE.md at base e780e13 (Code Style Guidelines) reads verbatim "- Formatting: 2-space indentation, semicolons required, single quotes preferred". The diff adds 4-space-indented lines throughout packages/client/src/client/authExtensions.ts, e.g. the new
checkedExpectedIssuerbody: " if (expectedIssuer === undefined) {" / " console.warn(" (4 and 8 spaces), plus the…
…ration with custom client authentication as it was With `issuer` in the schemas, OAuthTokens and OAuthClientInformation infer `issuer?: string | undefined`, which under exactOptionalPropertyTypes was no longer assignable to the Stored* aliases' `issuer?: string`. The aliases now use the same type. The addClientAuthentication() condition is dropped: it also stopped URL-based client IDs and DCR clients whose key is published in their metadata from moving to a new authorization server, and the bundled providers already throw because they have no saveClientInformation().
|
On the findings in the review body: the |
There was a problem hiding this comment.
I re-reviewed the revised push (698083a) and found no bugs; the dropped addClientAuthentication condition in auth() resolves the CIMD re-stamp concern from the earlier round. Because this changes the OAuth client's issuer-binding checks and the core credential schemas, a maintainer look is still worthwhile before merge.
What was reviewed:
auth()at packages/client/src/client/auth.ts:1288 is back to throwing only for providers withoutsaveClientInformation(), so SEP-991 and DCR providers with custom client authentication can move AS again as on base.- The new
fetchToken()guard at auth.ts:2448-2454 reads client information once, throws beforeprepareTokenRequest/addClientAuthentication/network on a mismatched stamp, and passes the checked value through;nullfromclientInformation()is treated as nothing stored. issuer: z.string().optional().catch(undefined)on the two core schemas plus theissuer?: string | undefinedalias change keep the inferred wire types assignable to the Stored aliases underexactOptionalPropertyTypes; the round-trip test assertsissuer: nullparses toundefined.- Constructor overloads,
checkedExpectedIssuer()warn/throw semantics,.examples.tsregions, docs and e2e scenarios are consistent with each other.
Extended reasoning...
The diff touches the client OAuth flow (packages/client/src/client/auth.ts fetchToken and authInternal issuer checks), the four bundled static-credential providers in authExtensions.ts, the AuthorizationServerMismatchError message, and the OAuthTokensSchema/OAuthClientInformationSchema in @ modelcontextprotocol/core, plus tests, examples, docs and a changeset. It is security-sensitive: it decides when stored client credentials may be presented to an authorization server. The follow-up commit removed the blanket addClientAuthentication guard that the prior review flagged as breaking CIMD and DCR-with-custom-auth providers, and the current guard matches base behaviour for those; new unit tests cover the fetchToken mismatch, schema round-trip and null handling. The hunt ran to a dry streak with no findings, but auth-path changes and a public core schema change warrant a human maintainer's confirmation rather than automated approval.
`issuer` on stored tokens and client information is written by `auth()` when it saves them. Token and registration responses are now parsed without it, in the client and in the server-legacy proxy provider, so the exported helpers return the same fields as before the schemas accepted `issuer`. `fetchToken()` reads the provider's client information again after the token request is prepared when the first read returned nothing, and applies the same check to it. A stored `issuer` that is not a string is treated as not set. The changeset is bumped to minor because it deprecates a constructor signature.
There was a problem hiding this comment.
I re-reviewed the latest pushes and found no bugs; since this changes the SEP-2352 credential-binding checks in the client auth flow, a human look is still worthwhile.
What was reviewed:
- The blanket
addClientAuthenticationre-registration guard flagged earlier is gone fromauthInternal, so the CIMD (SEP-991) path re-stamps and continues as on the base branch. - Non-string stored
issuer(null, number, object) now reads as unstamped in bothauthInternaland the newfetchToken()check; the{ ...stored, issuer: undefined }copy is back-stamped and saved byauth(). - Checked every remaining
OAuthTokensSchema/OAuthClientInformationFullSchemaparse of an AS response inpackages/for a site missingwithoutIssuer— none left;withoutIssuerstays in thecore-internalbarrel only. - The second
clientInformation()read infetchToken()only fires when the first returned nothing and goes through the same mismatch check.
Extended reasoning...
The diff deprecates omitting expectedIssuer on the four static-credential providers, adds an issuer-stamp check to fetchToken(), widens the two core Zod schemas to accept an optional issuer, and strips issuer from AS token/registration responses before parsing in the client, crossAppAccess and the server-legacy proxy provider. It touches security-sensitive OAuth client authentication paths (which authorization server receives client secrets, JWT assertions and refresh tokens). The bug hunt ran dry with no findings, the earlier flagged CIMD regression and null-stamp TypeError were addressed by the follow-up commits, and the lint-disable pattern for Zod .catch() matches existing usage in schemas.ts. Deferring rather than approving because the change alters auth-flow semantics across several packages and warrants maintainer judgment on the behavioural changes (new console.warn, new throw in fetchToken()).
…ction The bundled providers already reject an empty or `null` `expectedIssuer` when they are constructed. A value of another type, such as a `URL` object from a JavaScript caller, is now rejected there too, with the same error.
Resolves conflicts with #2887, which applies the SEP-2352 issuer check consistently: OAuthTokensSchema gained a client-only issuer member, and the wire parse sites now strip any AS-supplied issuer via withoutIssuer() before validating. The resolution composes both behaviors at every token parse site — executeTokenRequest, the cross-app exchangeJwtAuthGrant, and both proxyProvider exchanges now call OAuthTokenResponseSchema.parse(withoutIssuer(body)), so the AS-supplied issuer is stripped first and null-valued optional members are then normalized to absent. The OAuthTokensSchema shape-pin test is extended to cover the new issuer member; the scope-preservation and refresh hardening from this branch are untouched by the merge. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01KXnNnp3fxQYR5HF9BUVhYP
claude
Bot
added
the
v2
2 participants
Follow-ups to the SEP-2352 credential binding from #2286.
Motivation and Context
expectedIssuer.ClientCredentialsProvider,PrivateKeyJwtProvider,StaticPrivateKeyJwtProviderandCrossAppAccessProviderhold credentials the SDK did not obtain itself, so nothing butexpectedIssuersays which authorization server they belong to. Without it they are used with whichever one the MCP server advertises. Omitting it is now deprecated: oneconsole.warnper construction, and that constructor signature is marked@deprecated(editors strike through only the calls that omit it; everything still compiles). An empty ornullvalue throws at construction (it already failed later, insideauth()). Behaviour is otherwise unchanged. Docs, examples and tests pass it.The issuer stamp now also applies in three more places:
saveClientInformation()andaddClientAuthentication(), whose stored registration is bound to another authorization server, was registered again, andexecuteTokenRequest()then presented the custom authentication to the new server anyway.auth()now throwsAuthorizationServerMismatchError.fetchToken()readclientInformation()without the check. It now throwsAuthorizationServerMismatchError, beforeprepareTokenRequest()or any request, on a mismatch.OAuthTokensSchemaandOAuthClientInformationSchemastrippedissuer, so a provider that reads storage back through them lost the stamp on every read and was never bound. They now accept the optional field.auth()overwrites it on every save, so a value in a token or registration response never becomes the stamp of what it stores.Also: a
nullclientInformation()counts as nothing stored;AuthorizationServerMismatchError's message no longer assumes the authorization-code callback, since static-credential providers meet it too (fields unchanged); theskipIssuerMetadataValidationJSDoc says the uncheckedissueris also what stamps andexpectedIssuerare compared with.How Has This Been Tested?
New tests in
packages/client/test/client/auth.test.tsandauthExtensions.test.ts; each fails onmainbefore the change except thenullguard.pnpm check:all, unit, e2e, bun/deno integration, client conformance and the example stories pass.Breaking Changes
None to the API: constructor overloads, and one optional field on two schemas/types in
@modelcontextprotocol/core. Behavioural:console.warnwhen one of the four providers is constructed withoutexpectedIssuer.saveClientInformation()andaddClientAuthentication()getAuthorizationServerMismatchErrorinstead of a new registration when the authorization server changes. Clear the stored client information (invalidateCredentials('client')) to move.fetchToken()callers getAuthorizationServerMismatchErrorwhen the client information is bound elsewhere.issuerin a token or registration response is no longer stripped by the two schemas.Types of changes
Checklist
AI Disclaimer