fix(auth): treat null optional fields in token responses as absent

[claude]

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") inside exchangeAuthorization/refreshAuthorization, before any tokens are saved. Worse, "expires_in": null passed validation but silently coerced to 0 (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: null parses as undefined, not 0.

How: the exported OAuthTokensSchema is left completely untouched (still a plain ZodObject; class, .shape, .extend, z.input, and the emitted .d.ts are identical to v1.x, verified on zod 3.25.x and 4.x). Null normalization instead happens at the SDK's own response-parsing layer: a new exported OAuthTokenResponseSchema in src/shared/auth.ts wraps OAuthTokensSchema in a z.preprocess that removes null-valued optional members before validation (matching the existing ElicitResult null-leniency idiom in src/types.ts). The optional keys are derived from the schema's shape rather than hardcoded, so future optional members are covered automatically. The SDK's parse sites — executeTokenRequest in src/client/auth.ts and both token exchanges in ProxyOAuthServerProvider — now use the new schema. access_token: null and a missing token_type are still rejected, and refreshAuthorization's refresh-token preservation is hardened to { ...tokens, refresh_token: tokens.refresh_token ?? refreshToken } so it holds regardless of how the key is serialized. A changeset (patch) is included.

Note

Revised after an adversarial review: the previous approach wrapped OAuthTokensSchema itself in an object-level z.preprocess, which changed the exported symbol's class (ZodObject → ZodEffects/ZodPipe), breaking downstream consumers that call .extend() or read .shape — unacceptable in a patch release on the stable 1.x line. It also hardcoded the optional-field list.

Tests (test/shared/auth.test.ts, test/client/auth.test.ts): per-field null-as-absent with strict key-absence assertions, all-optionals-null toStrictEqual, expires_in: null never becomes 0, string expires_in still coerces, null access_token / missing token_type still rejected, a regression test pinning that OAuthTokensSchema keeps rejecting nulls and remains usable with .shape/.extend, a drift guard walking every optional member of the shape, plus e2e exchangeAuthorization (all-null optionals) and refreshAuthorization (server returns refresh_token: null, original refresh token preserved).

Counterpart for the main (v2) line: #2462. RFC 6749 doesn't sanction null for 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).

Scope of the normalization: this change deliberately covers only RFC 6749 §5.1 token responses at the SDK's token parse sites. The OAuth error-response schema (OAuthErrorResponseSchema, used by parseErrorResponse in src/client/auth.ts) intentionally remains strict about null members — consistent with the #754 registration-schema carve-out above — and can be revisited if field evidence of null-emitting servers turns up for error responses.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KXnNnp3fxQYR5HF9BUVhYP

---

Generated by Claude Code

[changeset-bot]

🦋 Changeset detected

Latest commit: d12cbb1

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

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/@modelcontextprotocol/sdk@2461

commit: d12cbb1

[claude]

I didn't find any bugs, but this changes OAuth token-response validation behavior and adds a new public export (OAuthTokenResponseSchema) on the stable 1.x line, so a maintainer should weigh the API surface and leniency policy.

What was reviewed:

• The z.preprocess normalization: null is only stripped from members whose field schema accepts undefined, so access_token: null and missing token_type still reject; expires_in: null is deleted before reaching z.coerce.number(), so it can no longer coerce to 0.
• Confirmed the exported OAuthTokensSchema symbol is untouched (still a plain ZodObject; .shape/.extend intact, pinned by a regression test).
• The refreshAuthorization spread reorder (tokens.refresh_token ?? refreshToken) is behavior-preserving given the new schema strips null keys.
• Ran both changed test files (164 tests) and npm run typecheck locally — all pass.

Extended reasoning...

Overview

This PR makes the SDK tolerant of authorization servers that serialize absent optional members of OAuth token responses as JSON null (nonconformant with RFC 6749 §5.1 but seen in the wild, e.g. Ory Hydra-style servers). It adds a new exported OAuthTokenResponseSchema in src/shared/auth.ts that wraps OAuthTokensSchema in a z.preprocess step deleting null-valued optional members before validation, and switches the SDK's three token-response parse sites (executeTokenRequest in src/client/auth.ts, and both token exchanges in ProxyOAuthServerProvider) to use it. It also hardens refreshAuthorization's refresh-token preservation and includes a patch changeset plus thorough tests.

Security risks

The change is in the OAuth client/proxy token-parsing path, which is inherently security-sensitive. The specific normalization is low-risk: it only converts null → absent for members that are already optional, derived from the schema shape rather than a hardcoded list, and required members still reject null (verified by tests). The most meaningful semantic subtlety — that stripping scope: null turns an undefined-semantics response into RFC 6749's "granted scope equals requested scope" assertion — is explicitly documented in the JSDoc and changeset, with the note that the SDK never uses tokens.scope for authorization decisions. I found no injection, bypass, or data-exposure vector. Still, loosening validation of security-relevant responses is a policy decision.

Level of scrutiny

High: this is production auth code in a widely-used SDK, changes what the client accepts from authorization servers, and adds a new public export to src/shared/auth.ts on the stable 1.x line (with a v2 counterpart in #2462). Per my guidelines, auth-touching changes and new public API surface should not be shadow-approved regardless of how clean the implementation is.

Other factors

Verification performed: I ran npx vitest run test/shared/auth.test.ts test/client/auth.test.ts (164 tests pass) and npm run typecheck (clean) on the PR branch. The test coverage is strong — per-field null-as-absent with key-absence assertions, a drift guard over every optional member of the shape, a regression test pinning that OAuthTokensSchema remains a usable ZodObject, and e2e exchange/refresh tests. I also checked the preprocess edge cases: non-object input passes through unchanged, the input object is shallow-copied rather than mutated, and expires_in's .optional() short-circuits before z.coerce.number() so the optionality detection (safeParse(undefined)) is correct for coerced fields. No human reviewer has looked at the PR yet; the design tradeoff (new exported schema vs. modifying the existing one) is well-reasoned in the description but deserves maintainer sign-off.

[claude]

I didn't find any bugs, but this changes OAuth token-response validation semantics in both the client auth flow and the proxy server provider on the stable 1.x line, so it deserves a maintainer's sign-off on the leniency decision itself.

What was reviewed:

• The z.preprocess normalization: only null-valued members whose field schema accepts undefined are stripped, so access_token: null and missing token_type still reject; non-object input passes through to fail normally.
• All three SDK token parse sites (executeTokenRequest, both ProxyOAuthServerProvider exchanges) now use the new schema — no strict parse site was left behind.
• The refreshAuthorization spread reorder: tokens.refresh_token ?? refreshToken correctly preserves the prior refresh token whether the key is absent or stripped-from-null.
• Ran test/shared/auth.test.ts, test/client/auth.test.ts, and test/server/auth/providers/proxyProvider.test.ts at the PR head — all 198 tests pass.

Extended reasoning...

Overview

The PR adds a new exported OAuthTokenResponseSchema in src/shared/auth.ts that wraps the unchanged OAuthTokensSchema in a z.preprocess step stripping null-valued optional members before validation, and switches the SDK's three token-response parse sites (executeTokenRequest in src/client/auth.ts, and exchangeAuthorizationCode/exchangeRefreshToken in ProxyOAuthServerProvider) to use it. It also hardens refreshAuthorization's refresh-token preservation from a spread-order trick to an explicit ?? fallback. Extensive tests and a patch changeset are included.

Security risks

This is auth code, and the change deliberately relaxes validation of authorization-server token responses. The relaxation is narrow and well-reasoned: only optional members (derived from the schema shape, not hardcoded) are normalized, required members (access_token, token_type) still reject null, and the fix removes a real footgun where expires_in: null coerced to 0 (an instantly-expired token). The changeset itself flags the one semantic subtlety — a stripped scope: null becomes indistinguishable from an omitted scope, which RFC 6749 §5.1 defines as "granted scope equals requested scope" — and correctly notes the SDK never uses tokens.scope for authorization decisions. I found no injection, bypass, or exposure vector, but whether the SDK should accept nonconformant server responses at all is a policy call.

Level of scrutiny

High: this is OAuth client/server code on the stable 1.x release line, and the approval guidelines exclude security-sensitive auth code from bot approval regardless of implementation quality. The implementation is clean and the API-compatibility concern (keeping OAuthTokensSchema a plain ZodObject so .shape/.extend keep working downstream) was explicitly handled and regression-tested, but the leniency-vs-strictness tradeoff and the new exported symbol on a patch release warrant maintainer judgment.

Other factors

Test coverage is thorough — per-field null-as-absent with strict key-absence assertions, a drift guard over all optional shape members, negative tests pinning that required fields still reject null, and e2e tests for both exchange and refresh flows. I ran the three touched test files at the PR head and all 198 tests pass. I also verified via grep that no OAuthTokensSchema.parse call site in src/ was left on the strict schema. The bug hunting system reported no findings, and there are no prior human reviews on the thread.

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

[claude]

🟡 nit (optional): REVIEW.md prefers improving an existing API over adding a parallel one: src/shared/auth.ts adds a second public token schema, OAuthTokenResponseSchema, beside OAuthTokensSchema, so @ modelcontextprotocol/sdk/shared/auth.js now exports two ways to parse the same RFC 6749 §5.1 token JSON — one strict, one null-lenient — and downstream code parsing token responses itself has to know which to pick. Fix: fold null-as-absent into the existing schema's field definitions (e.g. per-field z.preprocess(v => v === null ? undefined : v, ...) on the optional members, keeping the ZodObject class, .shape and .extend), or keep the wire schema unexported and used only at the SDK's own parse sites.

Why this was flagged

Nothing fails at runtime. The instruction guards public API surface growth: consumers who already call OAuthTokensSchema.parse(json) on token endpoint responses keep hitting the null-rejection bug this PR fixes unless they discover and switch to the new export, and the package now carries two token schemas that must be kept in sync (the PR adds a drift-guard test for that). The author states the parallel export was chosen deliberately after an earlier revision wrapped OAuthTokensSchema itself and changed its class (breaking .extend()/.shape), and that this is a v1.x patch where z.input/.d.ts stability matters; a per-field preprocess inside the existing ZodObject would preserve the class while widening only the input type. Low impact; a maintainer judgement call on API surface.

Verification: REVIEW.md block, Checklist § API surface: "One way to do things — improving an existing API beats adding a parallel one". Diff adds at /home/claude/typescript-sdk/src/shared/auth.ts:177 export const OAuthTokenResponseSchema = z.preprocess(..., OAuthTokensWireSchema), a second public schema for parsing RFC 6749 §5.1 token JSON beside the existing exported OAuthTokensSchema (line ~120), and…

[claude]

🟡 nit (optional): CLAUDE.md asks for 2-space indentation: every added line in this PR (the new OAuthTokenResponseSchema block here, the TokenResponseSchema replacements in src/client/auth.ts and src/server/auth/providers/proxyProvider.ts, and all added tests) is indented with 4 spaces. The repo's committed .prettierrc.json sets tabWidth: 4, so the code matches the enforced formatter and it is the CLAUDE.md line that is out of date. Fix: keep the Prettier-formatted 4-space code and update the CLAUDE.md Formatting bullet to say 4-space indentation so the written rule and npm run lint agree.

Why this was flagged

Nothing fails at runtime. This is a pure documentation/convention mismatch: CLAUDE.md says 2-space, but .prettierrc.json (tabWidth 4) and npm run lint (Prettier check) enforce 4-space, so a diff that literally followed CLAUDE.md would fail lint. The added code follows the formatter, not the written instruction. Consequence is nil for consumers; the only cost is a stale instruction misleading future contributors or agents that read CLAUDE.md before .prettierrc.json.

Verification: CLAUDE.md at base 51ad4f0 (Code Style Guidelines) reads verbatim "- Formatting: 2-space indentation, semicolons required, single quotes preferred"; the added OAuthTokenResponseSchema block in src/shared/auth.ts (lines ~178-193, e.g. data => { / if (data === null || ...) and every other added line in this diff is indented with 4 spaces, contrary to the instruction as…

[claude]

🟣 pre-existing, not blocking: Clients talking to the same null-emitting authorization servers still fail on the sibling parse sites, so the fix is partial. Token responses are normalized at src/client/auth.ts:1365, but OAuthErrorResponseSchema at src/client/auth.ts:395 and RegistrationResponseSchema at src/client/auth.ts:1596 still reject null optional members. An error body like {"error":"invalid_grant","error_description":null} falls into the catch and becomes a generic ServerError, so auth() at src/client/auth.ts:467-472 never invalidates credentials or retries. Fix: apply the same null-as-absent preprocess to the error-response and registration-response parse sites (client and proxyProvider), or document why the same servers do not hit them.
A small fix can ride a push you are already making; otherwise a short reply is enough.

Why this was flagged

Trigger: an authorization server that serializes absent members as null (the population this PR targets, e.g. the Ory Hydra pattern in #754) returns a 4xx token or registration error with error_description: null, or a registration response with a null optional member. Entry: executeTokenRequest at src/client/auth.ts:1362 calls parseErrorResponse; registerClient at src/client/auth.ts:1596 calls RegistrationResponseSchema.parse. OAuthErrorResponseSchema (src/shared/auth.ts:198-202) has error_description: z.string().optional(), so null throws inside parseErrorResponse at src/client/auth.ts:395 and the catch at :398 returns a ServerError with 'Invalid OAuth error response'. auth() at src/client/auth.ts:467-472 only recovers on InvalidClientError/UnauthorizedClientError/InvalidGrantError, so the user gets a hard failure instead of credential invalidation and retry. The base branch behaves the same, but REVIEW.md Completeness asks that every leftover sibling site be flagged when a PR replaces a pattern; the PR text carves these out by assertion only. Remedy: route error and registration parses…

Verification: nit — acknowledged in diff: the PR description explicitly carves out both sibling sites ("The OAuth error-response schema ... intentionally remains strict about null members — consistent with the #754 registration-schema carve-out above"), so this is a declared scope trade-off put up for sign-off; the runtime behaviour at those sites is identical on the base branch (the diff does not touch…