fix(auth): treat null optional fields in token responses as absent#2462
Draft
claude[bot] wants to merge 5 commits into
Draft
fix(auth): treat null optional fields in token responses as absent#2462claude[bot] wants to merge 5 commits into
claude[bot] wants to merge 5 commits into
Conversation
Some authorization servers serialize absent optional members as JSON null, which RFC 6749 does not sanction but is common in the wild. Previously, OAuthTokensSchema rejected refresh_token/scope/id_token when null with a Zod validation error, and expires_in: null silently coerced to 0 (Number(null) === 0), producing a token the client treated as already expired. This broke token exchange and refresh against such servers. Normalize null optional members to absent (undefined) before validation. Inferred output types are unchanged (string | undefined, number | undefined), so OAuthTokens consumers are unaffected. Related: #754 (same null-serialization pattern hitting the client registration schema).
🦋 Changeset detectedLatest commit: 4e62b73 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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: |
…in OAuthTokensSchema
Rework after review: the per-field z.preprocess mechanic left null-valued
keys present-as-undefined in the parsed output rather than absent, so
refreshAuthorization's spread over the previous refresh token was clobbered
by the explicit refresh_token: undefined — for exactly the null-emitting
servers this fix targets, every refresh silently destroyed the stored
refresh token. It also degraded z.input of the exported schema on zod <4.4.
- Revert OAuthTokensSchema to its original plain object definition,
restoring .shape/.extend/z.input for consumers (and the derived
specTypeSchemas/isSpecType input types).
- Add OAuthTokenResponseSchema, which removes null-valued optional members
(derived from the schema shape, not a hardcoded field list) before
validation, mirroring ElicitResult's null-leniency idiom, and use it at
the SDK's own token-response parse sites: executeTokenRequest, the
JWT-grant cross-app exchange, and server-legacy's proxyProvider.
- Harden refreshAuthorization's merge to
{ ...tokens, refresh_token: tokens.refresh_token ?? refreshToken } so a
present-but-undefined key can never clobber the preserved token.
- Tests now pin strict key absence (toStrictEqual / 'in' checks), null
access_token and missing token_type rejection, the exported schema's
unchanged shape/extend/input behavior, a shape-driven drift guard for
future optional members, and a refreshAuthorization e2e with
refresh_token: null that fails against the previous mechanic.
Related: #754 (same null-serialization pattern hitting the client
registration schema).
Stripping a null scope from a token response makes it indistinguishable from an omitted scope, which RFC 6749 §5.1 defines as an assertion that the granted scope is identical to the requested scope. Document that consumers must not infer the granted scope from its absence and should use token introspection for the authoritative grant.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KXnNnp3fxQYR5HF9BUVhYP
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Requested by Den Delimarsky · Slack thread
Before: connecting to an MCP server whose authorization server returns
"refresh_token": null(or"scope": null/"id_token": null) fails with a Zod validation error ("expected string, received null") insideexchangeAuthorization/refreshAuthorization, before any tokens are saved. Worse,"expires_in": nullpassed validation but silently coerced to0(Number(null) === 0), producing a token the client treats as already expired.After: those responses are accepted, and the null-valued fields are treated exactly as if they had been omitted.
expires_in: nullparses as absent, not0.How: the exported
OAuthTokensSchema(packages/core-internal/src/shared/auth.ts) is unchanged frommain— still a plain object schema that rejects nulls, with its.shape/.extendandz.inputbehavior intact (sospecTypeSchemas/isSpecTypeinput types are untouched). A newOAuthTokenResponseSchemawraps it in an object-levelz.preprocessthat removes null-valued optional members before validation — the member list is derived from the schema's shape, not hardcoded, mirroring the ElicitResultcontentnull-leniency idiom — and the SDK's own token-response parse sites now use it:executeTokenRequest(client token exchange + refresh), the JWT-grant cross-app exchange (crossAppAccess.ts), and server-legacy'sproxyProvider. Removing the key (rather than mapping it toundefined) means null members are strictly absent from the parsed output, sorefreshAuthorization's merge with previously-stored tokens keeps the prior refresh token; that merge is additionally hardened to{ ...tokens, refresh_token: tokens.refresh_token ?? refreshToken }.access_token: nulland a missingtoken_typeare still rejected.Revision after review: an adversarial review of the first version of this PR found that its per-field
z.preprocess(v => v ?? undefined, ...)mechanic left null-valued keys present with anundefinedvalue rather than absent, sorefreshAuthorization's spread of the previous refresh token was clobbered by the explicitrefresh_token: undefined— for exactly the null-emitting servers this PR targets, every refresh would have silently destroyed the stored refresh token (it also degradedz.inputof the exported schema on zod <4.4). This revision fixes that by normalizing at the SDK's parse sites via the new response schema and hardening the merge.Tests (
packages/core-internal/test/shared/auth.test.ts,packages/client/test/client/auth.test.ts): strict key-absence assertions (toStrictEqual/'field' in parsedchecks, which distinguish absent keys from present-but-undefined ones), each null optional field, all-optionals-null,expires_in: null !== 0, stringexpires_incoercion,access_token: nulland missingtoken_typerejection, regression pins that the exportedOAuthTokensSchemastill rejects nulls and keeps itsZodObjectshape/extend/input behavior, a shape-driven drift guard covering every optional member, an end-to-endexchangeAuthorizationwith an all-null-optional response, and an end-to-endrefreshAuthorizationwhere the server returnsrefresh_token: nullasserting the original refresh token is preserved (verified to fail against the previous mechanic). The changeset (patch forcore-internal,client,core,server-legacy) is updated to describe the final mechanic.RFC 6749 doesn't sanction
nullfor absent members, but real-world servers emit it anyway — see #754 for the same null-emitting-server pattern (Ory Hydra) hitting the client registration response schema (that schema is intentionally left out of scope here).Sibling PR with the same fix against
v1.x: #2461.🤖 Generated with Claude Code
https://claude.ai/code/session_01KXnNnp3fxQYR5HF9BUVhYP
Generated by Claude Code