diff --git a/.changeset/oauth-tokens-null-optional-fields.md b/.changeset/oauth-tokens-null-optional-fields.md new file mode 100644 index 0000000000..a674c12cec --- /dev/null +++ b/.changeset/oauth-tokens-null-optional-fields.md @@ -0,0 +1,25 @@ +--- +'@modelcontextprotocol/core-internal': patch +'@modelcontextprotocol/client': patch +'@modelcontextprotocol/core': patch +'@modelcontextprotocol/server-legacy': patch +--- + +OAuth token responses with null-valued optional members no longer fail +validation. Some authorization servers serialize absent optional members as +JSON `null` (nonconformant with RFC 6749 §5.1, but common in the wild); +previously `refresh_token`, `scope`, or `id_token` set to `null` failed token +exchange and refresh, and `expires_in: null` silently coerced to `0`, yielding +an instantly-expired token. The SDK's own token-response parse sites (client +token exchange/refresh, JWT-grant cross-app exchange, and the server-legacy +proxy provider) now validate with a new `OAuthTokenResponseSchema` that removes +null-valued optional members before validation, so they are strictly absent +from the parsed output. The exported `OAuthTokensSchema` is unchanged — still a +plain object schema that rejects nulls, with its `.shape`/`.extend` and input +types intact. `refreshAuthorization` additionally hardens its merge with the +previously-stored refresh token, so an explicitly `undefined` `refresh_token` +in a parsed response can never clobber the preserved token. Note that a +stripped null `scope` is thereafter indistinguishable from an omitted `scope` +— which RFC 6749 §5.1 defines as an assertion that the granted scope is +identical to the requested scope — so consumers should not infer the granted +scope from its absence. diff --git a/packages/client/src/client/auth.ts b/packages/client/src/client/auth.ts index 9ebc6fd251..28c86448a0 100644 --- a/packages/client/src/client/auth.ts +++ b/packages/client/src/client/auth.ts @@ -22,7 +22,7 @@ import { OAuthErrorResponseSchema, OAuthMetadataSchema, OAuthProtectedResourceMetadataSchema, - OAuthTokensSchema, + OAuthTokenResponseSchema, OpenIdProviderDiscoveryMetadataSchema, resourceUrlFromServerUrl, stampErrorBrands @@ -2100,7 +2100,7 @@ export async function executeTokenRequest( const json: unknown = await response.json(); try { - return OAuthTokensSchema.parse(json); + return OAuthTokenResponseSchema.parse(json); } catch (parseError) { // Some OAuth servers (e.g., GitHub) return error responses with HTTP 200 status. // Check for error field only if token parsing failed. @@ -2215,7 +2215,7 @@ export async function refreshAuthorization( }); // Preserve original refresh token if server didn't return a new one - return { refresh_token: refreshToken, ...tokens }; + return { ...tokens, refresh_token: tokens.refresh_token ?? refreshToken }; } /** diff --git a/packages/client/src/client/crossAppAccess.ts b/packages/client/src/client/crossAppAccess.ts index a7703dbf15..d7f75d28cb 100644 --- a/packages/client/src/client/crossAppAccess.ts +++ b/packages/client/src/client/crossAppAccess.ts @@ -9,7 +9,7 @@ */ import type { FetchLike } from '@modelcontextprotocol/core-internal'; -import { IdJagTokenExchangeResponseSchema, OAuthErrorResponseSchema, OAuthTokensSchema } from '@modelcontextprotocol/core-internal'; +import { IdJagTokenExchangeResponseSchema, OAuthErrorResponseSchema, OAuthTokenResponseSchema } from '@modelcontextprotocol/core-internal'; import type { ClientAuthMethod } from './auth'; import { applyClientAuthentication, assertSecureTokenEndpoint, discoverAuthorizationServerMetadata } from './auth'; @@ -298,7 +298,7 @@ export async function exchangeJwtAuthGrant(options: { const responseBody = await response.json(); // Validate response using core schema - const parseResult = OAuthTokensSchema.safeParse(responseBody); + const parseResult = OAuthTokenResponseSchema.safeParse(responseBody); if (!parseResult.success) { throw new Error(`Invalid token response: ${parseResult.error.message}`); } diff --git a/packages/client/test/client/auth.test.ts b/packages/client/test/client/auth.test.ts index 62c6faed9a..6487546740 100644 --- a/packages/client/test/client/auth.test.ts +++ b/packages/client/test/client/auth.test.ts @@ -2005,6 +2005,39 @@ describe('OAuth Authorization', () => { expect(body.get('redirect_uri')).toBe('http://localhost:3000/callback'); expect(body.get('resource')).toBe('https://api.example.com/mcp-server'); }); + it('treats null optional fields as absent (some auth servers serialize absent members as null)', async () => { + mockFetch.mockResolvedValueOnce({ + ok: true, + status: 200, + json: async () => ({ + access_token: 'access123', + token_type: 'Bearer', + expires_in: null, + scope: null, + refresh_token: null, + id_token: null + }) + }); + + const tokens = await exchangeAuthorization('https://auth.example.com', { + clientInformation: validClientInfo, + authorizationCode: 'code123', + codeVerifier: 'verifier123', + redirectUri: 'http://localhost:3000/callback', + resource: new URL('https://api.example.com/mcp-server') + }); + + // toStrictEqual pins the null-valued members as strictly ABSENT keys, not + // present-but-undefined: refreshAuthorization spreads the parsed response + // when merging with stored tokens, so a present `refresh_token: undefined` + // key would clobber the preserved refresh token. It also pins that + // expires_in: null did not coerce to 0 (an instantly-expired token). + expect(tokens).toStrictEqual({ + access_token: 'access123', + token_type: 'Bearer' + }); + }); + it('exchanges code for tokens with auth', async () => { mockFetch.mockResolvedValueOnce({ ok: true, @@ -2257,6 +2290,26 @@ describe('OAuth Authorization', () => { expect(tokens).toEqual({ refresh_token: refreshToken, ...validTokens }); }); + it('keeps the existing refresh token when the server returns refresh_token: null', async () => { + mockFetch.mockResolvedValueOnce({ + ok: true, + status: 200, + json: async () => ({ ...validTokens, refresh_token: null }) + }); + + const refreshToken = 'refresh123'; + const tokens = await refreshAuthorization('https://auth.example.com', { + clientInformation: validClientInfo, + refreshToken + }); + + // The null must not clobber the preserved token when the parsed response + // is merged with the previously-stored tokens (it would then be persisted + // via saveTokens, silently destroying the stored refresh token). + expect(tokens.refresh_token).toBe(refreshToken); + expect(tokens).toStrictEqual({ ...validTokens, refresh_token: refreshToken }); + }); + it('validates token response schema', async () => { mockFetch.mockResolvedValueOnce({ ok: true, diff --git a/packages/core-internal/src/shared/auth.ts b/packages/core-internal/src/shared/auth.ts index e21076d817..733ce3c953 100644 --- a/packages/core-internal/src/shared/auth.ts +++ b/packages/core-internal/src/shared/auth.ts @@ -143,6 +143,42 @@ export const OAuthTokensSchema = z }) .strip(); +/** + * OAuth 2.1 token response as received over the wire from an authorization server. + * + * Some authorization servers serialize absent optional members as JSON `null` + * (nonconformant with RFC 6749 §5.1, but common in the wild). Null-valued + * optional members are normalized to absent — the key is removed, mirroring + * ElicitResult's `content` null normalization — before {@linkcode OAuthTokensSchema} + * validates, so valid-but-sloppy responses parse and `expires_in: null` never + * reaches `z.coerce.number()` (`Number(null) === 0` would yield an + * instantly-expired token). Removing the key (rather than mapping it to + * `undefined`) matters: callers such as `refreshAuthorization` spread the parsed + * response when merging with previously-stored tokens, and a present-but-undefined + * `refresh_token` key would clobber the preserved value. + * + * Per RFC 6749 §5.1, an absent `scope` member is a positive assertion that + * the granted scope is identical to the scope the client requested, so + * stripping `scope: null` converts a response with undefined semantics into + * that assertion. This has no bearing on enforcement — the SDK never uses + * `tokens.scope` for authorization decisions, and the resource server remains + * authoritative — but consumers must not derive granted-scope conclusions + * from the member's absence. Consumers that need the authoritative grant + * should use token introspection instead. + */ +export const OAuthTokenResponseSchema = z.preprocess(value => { + if (value === null || typeof value !== 'object' || Array.isArray(value)) { + return value; + } + const normalized: Record = { ...value }; + for (const [key, memberSchema] of Object.entries(OAuthTokensSchema.shape)) { + if (normalized[key] === null && memberSchema.safeParse(undefined).success) { + delete normalized[key]; + } + } + return normalized; +}, OAuthTokensSchema); + /** * RFC 8693 §2.2.1 Token Exchange response for ID-JAG tokens. * diff --git a/packages/core-internal/test/shared/auth.test.ts b/packages/core-internal/test/shared/auth.test.ts index 4561d8fb11..e2cdab509d 100644 --- a/packages/core-internal/test/shared/auth.test.ts +++ b/packages/core-internal/test/shared/auth.test.ts @@ -1,6 +1,10 @@ +import * as z from 'zod/v4'; + import { OAuthClientMetadataSchema, OAuthMetadataSchema, + OAuthTokenResponseSchema, + OAuthTokensSchema, OpenIdProviderMetadataSchema, OptionalSafeUrlSchema, SafeUrlSchema @@ -100,6 +104,179 @@ describe('OpenIdProviderMetadataSchema', () => { }); }); +describe('OAuthTokensSchema', () => { + it('parses a fully-populated token response unchanged', () => { + const tokens = OAuthTokensSchema.parse({ + access_token: 'access123', + id_token: 'id123', + token_type: 'Bearer', + expires_in: 3600, + scope: 'read write', + refresh_token: 'refresh123' + }); + + expect(tokens).toEqual({ + access_token: 'access123', + id_token: 'id123', + token_type: 'Bearer', + expires_in: 3600, + scope: 'read write', + refresh_token: 'refresh123' + }); + }); + + it('parses a token response with optional fields absent', () => { + const tokens = OAuthTokensSchema.parse({ + access_token: 'access123', + token_type: 'Bearer' + }); + + expect(tokens.access_token).toBe('access123'); + expect(tokens.token_type).toBe('Bearer'); + expect(tokens.id_token).toBeUndefined(); + expect(tokens.expires_in).toBeUndefined(); + expect(tokens.scope).toBeUndefined(); + expect(tokens.refresh_token).toBeUndefined(); + }); + + it('rejects null-valued members (null normalization lives in OAuthTokenResponseSchema)', () => { + expect( + OAuthTokensSchema.safeParse({ + access_token: 'access123', + token_type: 'Bearer', + refresh_token: null + }).success + ).toBe(false); + expect( + OAuthTokensSchema.safeParse({ + access_token: null, + token_type: 'Bearer' + }).success + ).toBe(false); + }); + + it('remains a plain ZodObject with an introspectable shape (regression pin: consumers use .shape/.extend)', () => { + expect(OAuthTokensSchema).toBeInstanceOf(z.ZodObject); + expect(Object.keys(OAuthTokensSchema.shape).sort()).toEqual([ + 'access_token', + 'expires_in', + 'id_token', + 'refresh_token', + 'scope', + 'token_type' + ]); + + const extended = OAuthTokensSchema.extend({ example_extension: z.string() }); + expect( + extended.parse({ + access_token: 'access123', + token_type: 'Bearer', + example_extension: 'ext' + }).example_extension + ).toBe('ext'); + + // Optional members must stay optional in z.input — a compile-time pin + // (a schema-level preprocess would degrade these keys to required `unknown`). + const minimalInput: z.input = { + access_token: 'access123', + token_type: 'Bearer' + }; + expect(OAuthTokensSchema.parse(minimalInput)).toStrictEqual({ + access_token: 'access123', + token_type: 'Bearer' + }); + }); +}); + +describe('OAuthTokenResponseSchema', () => { + it.each(['refresh_token', 'scope', 'id_token'] as const)( + 'treats a null %s as absent (some auth servers serialize absent members as null)', + field => { + const tokens = OAuthTokenResponseSchema.parse({ + access_token: 'access123', + token_type: 'Bearer', + [field]: null + }); + + expect(field in tokens).toBe(false); + } + ); + + it('strips all null-valued optional members so the keys are strictly absent', () => { + const tokens = OAuthTokenResponseSchema.parse({ + access_token: 'access123', + token_type: 'Bearer', + expires_in: null, + scope: null, + refresh_token: null, + id_token: null + }); + + // toStrictEqual distinguishes absent keys from present-but-undefined keys. + // Key absence is load-bearing: refreshAuthorization spreads the parsed + // response when merging with previously-stored tokens, and a present + // `refresh_token: undefined` key would clobber the preserved refresh token. + expect(tokens).toStrictEqual({ + access_token: 'access123', + token_type: 'Bearer' + }); + }); + + it('treats a null expires_in as absent rather than coercing it to 0 (an instantly-expired token)', () => { + const tokens = OAuthTokenResponseSchema.parse({ + access_token: 'access123', + token_type: 'Bearer', + expires_in: null + }); + + expect('expires_in' in tokens).toBe(false); + expect(tokens.expires_in).not.toBe(0); + }); + + it('still coerces string expires_in values to numbers', () => { + const tokens = OAuthTokenResponseSchema.parse({ + access_token: 'access123', + token_type: 'Bearer', + expires_in: '3600' + }); + + expect(tokens.expires_in).toBe(3600); + }); + + it('still rejects a null access_token (only optional members are normalized)', () => { + expect( + OAuthTokenResponseSchema.safeParse({ + access_token: null, + token_type: 'Bearer' + }).success + ).toBe(false); + }); + + it('still rejects a missing token_type', () => { + expect( + OAuthTokenResponseSchema.safeParse({ + access_token: 'access123' + }).success + ).toBe(false); + }); + + it('normalizes a null in every optional member of OAuthTokensSchema (drift guard for future members)', () => { + for (const [key, memberSchema] of Object.entries(OAuthTokensSchema.shape)) { + if (!memberSchema.safeParse(undefined).success) { + continue; // Required member — nulls must keep failing, covered above. + } + + const tokens = OAuthTokenResponseSchema.parse({ + access_token: 'access123', + token_type: 'Bearer', + [key]: null + }); + + expect(key in tokens).toBe(false); + } + }); +}); + describe('OAuthClientMetadataSchema', () => { it('validates client metadata with safe URLs', () => { const metadata = { diff --git a/packages/server-legacy/src/auth/providers/proxyProvider.ts b/packages/server-legacy/src/auth/providers/proxyProvider.ts index 347589198a..8d484c5488 100644 --- a/packages/server-legacy/src/auth/providers/proxyProvider.ts +++ b/packages/server-legacy/src/auth/providers/proxyProvider.ts @@ -1,5 +1,5 @@ import type { FetchLike, OAuthClientInformationFull, OAuthTokenRevocationRequest, OAuthTokens } from '@modelcontextprotocol/core-internal'; -import { OAuthClientInformationFullSchema, OAuthTokensSchema } from '@modelcontextprotocol/core-internal'; +import { OAuthClientInformationFullSchema, OAuthTokenResponseSchema } from '@modelcontextprotocol/core-internal'; import type { Response } from 'express'; import type { OAuthRegisteredClientsStore } from '../clients'; @@ -194,7 +194,7 @@ export class ProxyOAuthServerProvider implements OAuthServerProvider { } const data = await response.json(); - return OAuthTokensSchema.parse(data); + return OAuthTokenResponseSchema.parse(data); } async exchangeRefreshToken( @@ -235,7 +235,7 @@ export class ProxyOAuthServerProvider implements OAuthServerProvider { } const data = await response.json(); - return OAuthTokensSchema.parse(data); + return OAuthTokenResponseSchema.parse(data); } async verifyAccessToken(token: string): Promise {