diff --git a/.changeset/oauth-token-response-null-optional-fields.md b/.changeset/oauth-token-response-null-optional-fields.md new file mode 100644 index 0000000000..9dde5116ec --- /dev/null +++ b/.changeset/oauth-token-response-null-optional-fields.md @@ -0,0 +1,8 @@ +--- +'@modelcontextprotocol/sdk': patch +--- + +The client (and the proxy server provider) now normalize JSON `null` values in optional members of OAuth token responses to absent before validation, via a new exported `OAuthTokenResponseSchema` used at the SDK's own parse sites. Some authorization servers serialize absent +optional members as `null` (nonconformant with RFC 6749 §5.1); previously such responses failed validation (`refresh_token`, `scope`, `id_token`) or coerced `expires_in: null` to `0`, an instantly-expired token. The exported `OAuthTokensSchema` is unchanged. 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. `refreshAuthorization` now also +preserves the previous refresh token whenever the response does not carry a new one, regardless of how the `refresh_token` key is serialized. diff --git a/src/client/auth.ts b/src/client/auth.ts index dd3976f712..cabb280806 100644 --- a/src/client/auth.ts +++ b/src/client/auth.ts @@ -16,7 +16,7 @@ import { OAuthClientInformationFullSchema, OAuthMetadataSchema, OAuthProtectedResourceMetadataSchema, - OAuthTokensSchema + OAuthTokenResponseSchema } from '../shared/auth.js'; import { checkResourceAllowed, resourceUrlFromServerUrl } from '../shared/auth-utils.js'; import { @@ -434,8 +434,8 @@ function discardIfIssuerMismatch(stored: T | null return issuersMatch(stored.issuer, issuer) ? stored : undefined; } -// `issuer` is added by the client when it stores a value; authorization server responses are parsed without it. -const TokenResponseSchema = OAuthTokensSchema.omit({ issuer: true }); +// `issuer` is added by the client when it stores a value; authorization server responses are parsed without it +// (token responses go through OAuthTokenResponseSchema, which omits `issuer` the same way). const RegistrationResponseSchema = OAuthClientInformationFullSchema.omit({ issuer: true }); function boundElsewhereError(stored: { issuer?: string }, issuer: string): Error { @@ -1363,7 +1363,7 @@ async function executeTokenRequest( throw await parseErrorResponse(response); } - return TokenResponseSchema.parse(await response.json()); + return OAuthTokenResponseSchema.parse(await response.json()); } /** @@ -1457,7 +1457,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/src/server/auth/providers/proxyProvider.ts b/src/server/auth/providers/proxyProvider.ts index 9e8e67ea82..b4cbfa7196 100644 --- a/src/server/auth/providers/proxyProvider.ts +++ b/src/server/auth/providers/proxyProvider.ts @@ -5,15 +5,15 @@ import { OAuthClientInformationFullSchema, OAuthTokenRevocationRequest, OAuthTokens, - OAuthTokensSchema + OAuthTokenResponseSchema } from '../../../shared/auth.js'; import { AuthInfo } from '../types.js'; import { AuthorizationParams, OAuthServerProvider } from '../provider.js'; import { ServerError } from '../errors.js'; import { FetchLike } from '../../../shared/transport.js'; -// `issuer` is added by a client when it stores a value; upstream responses are parsed without it. -const TokenResponseSchema = OAuthTokensSchema.omit({ issuer: true }); +// `issuer` is added by a client when it stores a value; upstream responses are parsed without it +// (token responses go through OAuthTokenResponseSchema, which omits `issuer` the same way). const RegistrationResponseSchema = OAuthClientInformationFullSchema.omit({ issuer: true }); export type ProxyEndpoints = { @@ -192,7 +192,7 @@ export class ProxyOAuthServerProvider implements OAuthServerProvider { } const data = await response.json(); - return TokenResponseSchema.parse(data); + return OAuthTokenResponseSchema.parse(data); } async exchangeRefreshToken( @@ -233,7 +233,7 @@ export class ProxyOAuthServerProvider implements OAuthServerProvider { } const data = await response.json(); - return TokenResponseSchema.parse(data); + return OAuthTokenResponseSchema.parse(data); } async verifyAccessToken(token: string): Promise { diff --git a/src/shared/auth.ts b/src/shared/auth.ts index 99959be189..166a463956 100644 --- a/src/shared/auth.ts +++ b/src/shared/auth.ts @@ -143,6 +143,55 @@ export const OAuthTokensSchema = z }) .strip(); +/** {@link OAuthTokensSchema} as it appears on the wire: without the client-only `issuer` stamp. */ +const OAuthTokensWireSchema = OAuthTokensSchema.omit({ issuer: true }); + +/** + * Schema for parsing OAuth 2.1 token responses received from an authorization + * server. + * + * Some authorization servers serialize absent optional members as JSON null + * (e.g. `"refresh_token": null`), which is nonconformant with RFC 6749 §5.1. + * We normalize null to undefined for leniency: null values in optional + * members are normalized to absent before validation, so that (a) otherwise + * valid responses from such servers parse, and (b) `expires_in: null` never + * reaches `z.coerce.number()`, which would coerce it to 0 — an + * instantly-expired token. Null values in required members are still + * rejected, non-object input is passed through unchanged, and the strict + * {@link OAuthTokensSchema} is unaffected. + * + * 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. + * + * The client-only `issuer` stamp is not part of the wire format: like the other + * response parse paths, this schema validates against {@link OAuthTokensSchema} + * without its `issuer` member, so an authorization server cannot inject the + * value the client binds stored credentials to. + */ +export const OAuthTokenResponseSchema = z.preprocess( + data => { + if (data === null || typeof data !== 'object' || Array.isArray(data)) { + return data; + } + const normalized: Record = { ...data }; + for (const [key, fieldSchema] of Object.entries(OAuthTokensWireSchema.shape)) { + if (normalized[key] === null && fieldSchema.safeParse(undefined).success) { + delete normalized[key]; + } + } + return normalized; + }, + // `issuer` is added by the client when it stores a value; authorization server + // responses are parsed without it (and `.strip()` then drops an incoming key). + OAuthTokensWireSchema +); + /** * OAuth 2.1 error response */ diff --git a/test/client/auth.test.ts b/test/client/auth.test.ts index 45ec2806d0..3abaf67e54 100644 --- a/test/client/auth.test.ts +++ b/test/client/auth.test.ts @@ -1602,6 +1602,36 @@ 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('accepts a token response with all optional fields null', async () => { + mockFetch.mockResolvedValueOnce({ + ok: true, + status: 200, + json: async () => ({ + access_token: 'access123', + token_type: 'Bearer', + id_token: null, + expires_in: null, + scope: null, + refresh_token: null + }) + }); + + const tokens = await exchangeAuthorization('https://auth.example.com', { + clientInformation: validClientInfo, + authorizationCode: 'code123', + codeVerifier: 'verifier123', + redirectUri: 'http://localhost:3000/callback' + }); + + // The null members must be truly absent from the result, not + // present with an undefined value. + expect(tokens).toStrictEqual({ + access_token: 'access123', + token_type: 'Bearer' + }); + }); + it('exchanges code for tokens with auth', async () => { mockFetch.mockResolvedValueOnce({ ok: true, @@ -1852,6 +1882,30 @@ describe('OAuth Authorization', () => { expect(tokens).toEqual({ refresh_token: refreshToken, ...validTokens }); }); + it('keeps the existing refresh token if the server returns a null refresh_token', async () => { + mockFetch.mockResolvedValueOnce({ + ok: true, + status: 200, + json: async () => ({ + access_token: 'newaccess123', + token_type: 'Bearer', + refresh_token: null + }) + }); + + const refreshToken = 'refresh123'; + const tokens = await refreshAuthorization('https://auth.example.com', { + clientInformation: validClientInfo, + refreshToken + }); + + expect(tokens).toStrictEqual({ + access_token: 'newaccess123', + token_type: 'Bearer', + refresh_token: refreshToken + }); + }); + it('validates token response schema', async () => { mockFetch.mockResolvedValueOnce({ ok: true, diff --git a/test/server/auth/providers/proxyProvider.test.ts b/test/server/auth/providers/proxyProvider.test.ts index 30975fcd8b..29102442ff 100644 --- a/test/server/auth/providers/proxyProvider.test.ts +++ b/test/server/auth/providers/proxyProvider.test.ts @@ -226,6 +226,56 @@ describe('Proxy OAuth Server Provider', () => { expect(tokens).toEqual(mockTokenResponse); }); + describe('null optional fields in upstream response', () => { + // Some authorization servers (e.g. AWS Cognito) serialize absent optional + // members as JSON null instead of omitting them + const nullFieldTokenResponse = { + access_token: 'new-access-token', + token_type: 'Bearer', + expires_in: null, + refresh_token: null, + scope: null, + id_token: null + }; + + beforeEach(() => { + (global.fetch as Mock).mockImplementation(() => + Promise.resolve({ + ok: true, + json: () => Promise.resolve(nullFieldTokenResponse) + }) + ); + }); + + it('exchanges authorization code and treats null fields as absent', async () => { + const tokens = await provider.exchangeAuthorizationCode(validClient, 'test-code', 'test-verifier'); + + expect(tokens).toStrictEqual({ + access_token: 'new-access-token', + token_type: 'Bearer' + }); + expect('refresh_token' in tokens).toBe(false); + expect('expires_in' in tokens).toBe(false); + expect('scope' in tokens).toBe(false); + expect('id_token' in tokens).toBe(false); + expect(tokens.expires_in).toBeUndefined(); + }); + + it('exchanges refresh token and treats null fields as absent', async () => { + const tokens = await provider.exchangeRefreshToken(validClient, 'test-refresh-token', ['read', 'write']); + + expect(tokens).toStrictEqual({ + access_token: 'new-access-token', + token_type: 'Bearer' + }); + expect('refresh_token' in tokens).toBe(false); + expect('expires_in' in tokens).toBe(false); + expect('scope' in tokens).toBe(false); + expect('id_token' in tokens).toBe(false); + expect(tokens.expires_in).toBeUndefined(); + }); + }); + it.each(['https://upstream.example.com', 42])('does not forward an issuer of %j from the upstream token response', async issuer => { (global.fetch as Mock).mockImplementation(() => Promise.resolve({ diff --git a/test/shared/auth.test.ts b/test/shared/auth.test.ts index c4ecab59d5..628add5573 100644 --- a/test/shared/auth.test.ts +++ b/test/shared/auth.test.ts @@ -3,8 +3,11 @@ import { OAuthMetadataSchema, OpenIdProviderMetadataSchema, OAuthClientMetadataSchema, + OAuthTokensSchema, + OAuthTokenResponseSchema, OptionalSafeUrlSchema } from '../../src/shared/auth.js'; +import * as z from 'zod/v4'; describe('SafeUrlSchema', () => { it('accepts valid HTTPS URLs', () => { @@ -100,6 +103,160 @@ describe('OpenIdProviderMetadataSchema', () => { }); }); +describe('OAuthTokensSchema', () => { + it('round-trips a fully-populated token response', () => { + const tokens = OAuthTokensSchema.parse({ + access_token: 'access-123', + id_token: 'id-456', + token_type: 'Bearer', + expires_in: 3600, + scope: 'read write', + refresh_token: 'refresh-789' + }); + + expect(tokens).toEqual({ + access_token: 'access-123', + id_token: 'id-456', + token_type: 'Bearer', + expires_in: 3600, + scope: 'read write', + refresh_token: 'refresh-789' + }); + }); + + it('accepts a token response with optional fields absent', () => { + const tokens = OAuthTokensSchema.parse({ + access_token: 'access-123', + token_type: 'Bearer' + }); + + expect(tokens.access_token).toBe('access-123'); + 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(); + }); + + // The strict schema is deliberately unchanged: null optional members are + // only normalized by OAuthTokenResponseSchema below. + it.each(['refresh_token', 'scope', 'id_token'])('still rejects a null %s', field => { + expect(() => + OAuthTokensSchema.parse({ + access_token: 'access-123', + token_type: 'Bearer', + [field]: null + }) + ).toThrow(); + }); + + it('remains an object schema usable with .shape and .extend', () => { + // Regression test: wrapping the exported schema (e.g. in + // z.preprocess) would change its class and break downstream + // consumers that call .extend() or introspect .shape. + expect(OAuthTokensSchema.shape.access_token).toBeDefined(); + + const extended = OAuthTokensSchema.extend({ example: z.string() }); + const parsed = extended.parse({ + access_token: 'access-123', + token_type: 'Bearer', + example: 'value' + }); + expect(parsed.example).toBe('value'); + }); +}); + +describe('OAuthTokenResponseSchema', () => { + it.each(['refresh_token', 'scope', 'id_token'])('treats null %s as absent', field => { + const tokens = OAuthTokenResponseSchema.parse({ + access_token: 'access-123', + token_type: 'Bearer', + [field]: null + }); + + expect(field in tokens).toBe(false); + }); + + it('treats null expires_in as absent instead of coercing it to 0', () => { + const tokens = OAuthTokenResponseSchema.parse({ + access_token: 'access-123', + token_type: 'Bearer', + expires_in: null + }); + + expect('expires_in' in tokens).toBe(false); + expect(tokens.expires_in).not.toBe(0); + }); + + it('still coerces a string expires_in to a number', () => { + const tokens = OAuthTokenResponseSchema.parse({ + access_token: 'access-123', + token_type: 'Bearer', + expires_in: '3600' + }); + + expect(tokens.expires_in).toBe(3600); + }); + + it('accepts a token response with all optional fields null', () => { + const tokens = OAuthTokenResponseSchema.parse({ + access_token: 'access-123', + token_type: 'Bearer', + id_token: null, + expires_in: null, + scope: null, + refresh_token: null + }); + + // toStrictEqual: the null members must be truly absent from the + // output, not present with an undefined value, so that spreads like + // `{ ...tokens, refresh_token: tokens.refresh_token ?? previous }` + // (and plain spreads over defaults) keep the previous value. + expect(tokens).toStrictEqual({ + access_token: 'access-123', + token_type: 'Bearer' + }); + }); + + it('still rejects a null access_token', () => { + expect(() => + OAuthTokenResponseSchema.parse({ + access_token: null, + token_type: 'Bearer' + }) + ).toThrow(); + }); + + it('still rejects a missing token_type', () => { + expect(() => + OAuthTokenResponseSchema.parse({ + access_token: 'access-123' + }) + ).toThrow(); + }); + + it('treats null as absent for every optional member of OAuthTokensSchema', () => { + // Drift guard: if a new optional member is added to + // OAuthTokensSchema, it is automatically covered by the + // null-normalization without updating any field list. + const optionalFields = Object.entries(OAuthTokensSchema.shape) + .filter(([, fieldSchema]) => fieldSchema.safeParse(undefined).success) + .map(([field]) => field); + + expect(optionalFields.length).toBeGreaterThan(0); + + for (const field of optionalFields) { + const tokens = OAuthTokenResponseSchema.parse({ + access_token: 'access-123', + token_type: 'Bearer', + [field]: null + }); + + expect(field in tokens).toBe(false); + } + }); +}); + describe('OAuthClientMetadataSchema', () => { it('validates client metadata with safe URLs', () => { const metadata = {