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 85398340b0..617fd4bf29 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 { @@ -1255,7 +1255,7 @@ async function executeTokenRequest( throw await parseErrorResponse(response); } - return OAuthTokensSchema.parse(await response.json()); + return OAuthTokenResponseSchema.parse(await response.json()); } /** @@ -1349,7 +1349,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 855856c89e..9b9129be5b 100644 --- a/src/server/auth/providers/proxyProvider.ts +++ b/src/server/auth/providers/proxyProvider.ts @@ -5,7 +5,7 @@ import { OAuthClientInformationFullSchema, OAuthTokenRevocationRequest, OAuthTokens, - OAuthTokensSchema + OAuthTokenResponseSchema } from '../../../shared/auth.js'; import { AuthInfo } from '../types.js'; import { AuthorizationParams, OAuthServerProvider } from '../provider.js'; @@ -188,7 +188,7 @@ export class ProxyOAuthServerProvider implements OAuthServerProvider { } const data = await response.json(); - return OAuthTokensSchema.parse(data); + return OAuthTokenResponseSchema.parse(data); } async exchangeRefreshToken( @@ -229,7 +229,7 @@ export class ProxyOAuthServerProvider implements OAuthServerProvider { } const data = await response.json(); - return OAuthTokensSchema.parse(data); + return OAuthTokenResponseSchema.parse(data); } async verifyAccessToken(token: string): Promise { diff --git a/src/shared/auth.ts b/src/shared/auth.ts index c546c8608b..604b504469 100644 --- a/src/shared/auth.ts +++ b/src/shared/auth.ts @@ -138,6 +138,42 @@ export const OAuthTokensSchema = z }) .strip(); +/** + * 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. + */ +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(OAuthTokensSchema.shape)) { + if (normalized[key] === null && fieldSchema.safeParse(undefined).success) { + delete normalized[key]; + } + } + return normalized; +}, OAuthTokensSchema); + /** * OAuth 2.1 error response */ diff --git a/test/client/auth.test.ts b/test/client/auth.test.ts index 6b70fbe942..ec5c678485 100644 --- a/test/client/auth.test.ts +++ b/test/client/auth.test.ts @@ -1582,6 +1582,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, @@ -1832,6 +1862,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/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 = {