diff --git a/.changeset/oauth-tokens-null-optional-fields.md b/.changeset/oauth-tokens-null-optional-fields.md new file mode 100644 index 0000000000..ceab7f31b4 --- /dev/null +++ b/.changeset/oauth-tokens-null-optional-fields.md @@ -0,0 +1,38 @@ +--- +'@modelcontextprotocol/core': patch +'@modelcontextprotocol/core-internal': patch +'@modelcontextprotocol/client': patch +'@modelcontextprotocol/server-legacy': patch +'@modelcontextprotocol/codemod': 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` (defined in +`@modelcontextprotocol/core`'s auth schema module and forwarded through +`core-internal`'s re-export shim) 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, with its `.shape`/`.extend` and input types intact: it +rejects `null` for its string-typed optional members, though `expires_in: null` +still coerces to `0` there (use `OAuthTokenResponseSchema` for raw wire +input). `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. + +`OAuthTokenResponseSchema` is also a public export from +`@modelcontextprotocol/core`'s root, since the sibling v1 release exports it +from `@modelcontextprotocol/sdk/shared/auth.js` and migrating code needs a v2 +home for it. The v1-to-v2 codemod's auth schema allowlist now includes the +name, so `import { OAuthTokenResponseSchema } from +'@modelcontextprotocol/sdk/shared/auth.js'` rewrites to +`@modelcontextprotocol/core` alongside the other auth schema constants. diff --git a/docs/advanced/wire-schemas.md b/docs/advanced/wire-schemas.md index 8ce47bda53..d59d31b4a3 100644 --- a/docs/advanced/wire-schemas.md +++ b/docs/advanced/wire-schemas.md @@ -4,7 +4,7 @@ shape: how-to # Wire schemas -`@modelcontextprotocol/core` exports the **wire schemas** — the exact Zod constants the SDK validates protocol and OAuth payloads against — for code that holds raw JSON instead of SDK objects. +`@modelcontextprotocol/core` exports the **wire schemas** — the exact Zod constants the SDK validates protocol and OAuth payloads against — for code that holds raw JSON instead of SDK objects. (One qualification: at the SDK's own token-response parse sites, `OAuthTokensSchema` is applied through the null-tolerant `OAuthTokenResponseSchema` wrapper — see the note in the OAuth section below.) ## Validate a wire payload @@ -129,7 +129,7 @@ A document missing a required endpoint fails the parse; a valid one comes back t https://auth.example.com/token ``` -The group follows the same naming convention: `OAuthTokensSchema` for token responses, `OAuthProtectedResourceMetadataSchema` for protected-resource metadata, `OpenIdProviderDiscoveryMetadataSchema` for OpenID provider discovery. +The group follows the same naming convention: `OAuthTokensSchema` for token responses, `OAuthProtectedResourceMetadataSchema` for protected-resource metadata, `OpenIdProviderDiscoveryMetadataSchema` for OpenID provider discovery. Note that the SDK's own token-response parse sites validate with `OAuthTokenResponseSchema` — also exported from `@modelcontextprotocol/core` — a null-tolerant wrapper around `OAuthTokensSchema` that drops null-valued optional members before validating. The plain `OAuthTokensSchema` rejects `null` for its string-typed members, but `expires_in: null` coerces to `0` there, so prefer `OAuthTokenResponseSchema` when validating raw wire input. ## Get the TypeScript types, guards and errors from the SDK packages diff --git a/packages/client/src/client/auth.ts b/packages/client/src/client/auth.ts index 9ebc6fd251..d6a765b274 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 @@ -1314,7 +1314,14 @@ async function authInternal( fetchFn }); - await provider.saveTokens({ ...newTokens, issuer }, infoCtx); + // RFC 6749 §5.1/§6: the SDK sends no `scope` parameter on refresh, so a + // response without `scope` (including a null-stripped one) asserts the grant + // is unchanged — preserve the stored scope instead of erasing the recorded + // grant (the 403 insufficient_scope step-up unions it to avoid losing + // previously-granted permissions). The conditional spread keeps `scope` + // strictly absent when neither side has one — never present-but-undefined. + const preservedScope = newTokens.scope ?? tokens.scope; + await provider.saveTokens({ ...newTokens, ...(preservedScope !== undefined && { scope: preservedScope }), issuer }, infoCtx); return 'AUTHORIZED'; } catch (error) { // A non-TLS token endpoint is a configuration error — re-authorizing cannot @@ -2100,7 +2107,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 +2222,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 49f010d626..fcb6104fe5 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..544d398f34 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, @@ -3035,6 +3088,188 @@ describe('OAuth Authorization', () => { expect(body.get('refresh_token')).toBe('refresh123'); }); + // RFC 6749 §5.1/§6: the SDK sends no `scope` parameter on refresh, so a response + // without `scope` (or with `scope: null`, stripped by OAuthTokenResponseSchema) + // asserts the grant is unchanged. The stored scope must survive the refresh — + // otherwise, after a restart, the insufficient_scope step-up would compute its + // scope union without the original grant and force interactive re-authorization. + const mockRefreshFetchWithTokenResponse = (tokenResponse: Record) => { + mockFetch.mockImplementation(url => { + const urlString = url.toString(); + + if (urlString.includes('/.well-known/oauth-protected-resource')) { + return Promise.resolve({ + ok: true, + status: 200, + json: async () => ({ + resource: 'https://api.example.com/mcp-server', + authorization_servers: ['https://auth.example.com'] + }) + }); + } else if (urlString.includes('/.well-known/oauth-authorization-server')) { + return Promise.resolve({ + ok: true, + status: 200, + json: async () => ({ + issuer: 'https://auth.example.com', + authorization_endpoint: 'https://auth.example.com/authorize', + token_endpoint: 'https://auth.example.com/token', + response_types_supported: ['code'], + code_challenge_methods_supported: ['S256'] + }) + }); + } else if (urlString.includes('/token')) { + return Promise.resolve({ + ok: true, + status: 200, + json: async () => tokenResponse + }); + } + + return Promise.resolve({ ok: false, status: 404 }); + }); + }; + + it('keeps the stored scope when the refresh response returns scope: null', async () => { + mockRefreshFetchWithTokenResponse({ + access_token: 'new-access123', + token_type: 'Bearer', + expires_in: 3600, + scope: null + }); + + (mockProvider.clientInformation as Mock).mockResolvedValue({ + client_id: 'test-client', + client_secret: 'test-secret' + }); + (mockProvider.tokens as Mock).mockResolvedValue({ + access_token: 'old-access', + refresh_token: 'refresh123', + scope: 'read write' + }); + (mockProvider.saveTokens as Mock).mockResolvedValue(undefined); + + const result = await auth(mockProvider, { + serverUrl: 'https://api.example.com/mcp-server' + }); + + expect(result).toBe('AUTHORIZED'); + expect(mockProvider.saveTokens).toHaveBeenCalledWith( + expect.objectContaining({ + access_token: 'new-access123', + refresh_token: 'refresh123', + scope: 'read write' + }), + expect.anything() + ); + }); + + it('keeps the stored scope when the refresh response omits scope', async () => { + mockRefreshFetchWithTokenResponse({ + access_token: 'new-access123', + token_type: 'Bearer', + expires_in: 3600 + }); + + (mockProvider.clientInformation as Mock).mockResolvedValue({ + client_id: 'test-client', + client_secret: 'test-secret' + }); + (mockProvider.tokens as Mock).mockResolvedValue({ + access_token: 'old-access', + refresh_token: 'refresh123', + scope: 'read write' + }); + (mockProvider.saveTokens as Mock).mockResolvedValue(undefined); + + const result = await auth(mockProvider, { + serverUrl: 'https://api.example.com/mcp-server' + }); + + expect(result).toBe('AUTHORIZED'); + expect(mockProvider.saveTokens).toHaveBeenCalledWith( + expect.objectContaining({ + access_token: 'new-access123', + refresh_token: 'refresh123', + scope: 'read write' + }), + expect.anything() + ); + }); + + it('stores the scope the refresh response grants when one is present', async () => { + mockRefreshFetchWithTokenResponse({ + access_token: 'new-access123', + token_type: 'Bearer', + expires_in: 3600, + scope: 'read' + }); + + (mockProvider.clientInformation as Mock).mockResolvedValue({ + client_id: 'test-client', + client_secret: 'test-secret' + }); + (mockProvider.tokens as Mock).mockResolvedValue({ + access_token: 'old-access', + refresh_token: 'refresh123', + scope: 'read write' + }); + (mockProvider.saveTokens as Mock).mockResolvedValue(undefined); + + const result = await auth(mockProvider, { + serverUrl: 'https://api.example.com/mcp-server' + }); + + expect(result).toBe('AUTHORIZED'); + // A scope the server does return is authoritative (a refresh may narrow the + // grant, RFC 6749 §6) — preservation only applies to an absent member. + expect(mockProvider.saveTokens).toHaveBeenCalledWith( + expect.objectContaining({ + access_token: 'new-access123', + scope: 'read' + }), + expect.anything() + ); + }); + + it('saves a payload without a scope key when neither the response nor the stored tokens have one', async () => { + mockRefreshFetchWithTokenResponse({ + access_token: 'new-access123', + token_type: 'Bearer', + expires_in: 3600 + }); + + (mockProvider.clientInformation as Mock).mockResolvedValue({ + client_id: 'test-client', + client_secret: 'test-secret' + }); + (mockProvider.tokens as Mock).mockResolvedValue({ + access_token: 'old-access', + refresh_token: 'refresh123' + }); + (mockProvider.saveTokens as Mock).mockResolvedValue(undefined); + + const result = await auth(mockProvider, { + serverUrl: 'https://api.example.com/mcp-server' + }); + + expect(result).toBe('AUTHORIZED'); + // Scope preservation must not manufacture a present-but-undefined `scope` + // key — the exact key shape this PR's normalization exists to prevent. + // toStrictEqual distinguishes an absent key from one set to undefined. + // The last saveTokens call is the refresh save (the first is the + // SEP-2352 issuer back-stamp of the legacy unstamped token set). + const savedTokens = (mockProvider.saveTokens as Mock).mock.lastCall![0]; + expect('scope' in savedTokens).toBe(false); + expect(savedTokens).toStrictEqual({ + access_token: 'new-access123', + token_type: 'Bearer', + expires_in: 3600, + refresh_token: 'refresh123', + issuer: 'https://auth.example.com' + }); + }); + it('skips default PRM resource validation when custom validateResourceURL is provided', async () => { const mockValidateResourceURL = vi.fn().mockResolvedValue(undefined); const providerWithCustomValidation = { diff --git a/packages/codemod/src/migrations/v1-to-v2/mappings/authSchemaNames.ts b/packages/codemod/src/migrations/v1-to-v2/mappings/authSchemaNames.ts index 89a9083b15..5120df17da 100644 --- a/packages/codemod/src/migrations/v1-to-v2/mappings/authSchemaNames.ts +++ b/packages/codemod/src/migrations/v1-to-v2/mappings/authSchemaNames.ts @@ -4,7 +4,9 @@ // set (the corresponding TYPES, e.g. OAuthTokens, resolve by context to @modelcontextprotocol/client | // /server). This is the v1 auth-schema set — a SUBSET of core's auth exports. v2-only auth schemas // (e.g. IdJagTokenExchangeResponseSchema) are exported by core but NOT listed here: v1 never had -// them, so there is nothing to migrate. test/v1-to-v2/authSchemaNames.test.ts asserts every name here is +// them, so there is nothing to migrate. OAuthTokenResponseSchema, the null-tolerant token-response +// wrapper, IS listed: v1 exports it from sdk/shared/auth.js and core exports it publicly, so its +// imports route to core like the rest. test/v1-to-v2/authSchemaNames.test.ts asserts every name here is // exported by core (so the rewritten import resolves). Keep alphabetized. export const AUTH_SCHEMA_NAMES: ReadonlySet = new Set([ 'OAuthClientInformationFullSchema', @@ -14,6 +16,7 @@ export const AUTH_SCHEMA_NAMES: ReadonlySet = new Set([ 'OAuthErrorResponseSchema', 'OAuthMetadataSchema', 'OAuthProtectedResourceMetadataSchema', + 'OAuthTokenResponseSchema', 'OAuthTokenRevocationRequestSchema', 'OAuthTokensSchema', 'OpenIdProviderDiscoveryMetadataSchema', diff --git a/packages/codemod/test/v1-to-v2/authSchemaNames.test.ts b/packages/codemod/test/v1-to-v2/authSchemaNames.test.ts index 97044c3b8c..d56140a2c7 100644 --- a/packages/codemod/test/v1-to-v2/authSchemaNames.test.ts +++ b/packages/codemod/test/v1-to-v2/authSchemaNames.test.ts @@ -21,8 +21,11 @@ describe('AUTH_SCHEMA_NAMES (codemod auth schema-routing allowlist)', () => { const notExportedByCore = [...AUTH_SCHEMA_NAMES].filter(name => !coreAuthExports.has(name)); expect(notExportedByCore).toEqual([]); - // The v1 auth-schema set is frozen; pin its size so an accidental add/remove is caught. - expect(AUTH_SCHEMA_NAMES.size).toBe(11); + // Pin the v1 auth-schema set's size so an accidental add/remove is caught. 12 = the 11 + // original v1 exports plus OAuthTokenResponseSchema (the null-tolerant token-response + // wrapper, public in v1 and exported from core's barrel). + expect(AUTH_SCHEMA_NAMES.size).toBe(12); + expect(AUTH_SCHEMA_NAMES.has('OAuthTokenResponseSchema')).toBe(true); }); it('keeps the no-v2-home auth schemas OUT of the routing allowlist', () => { diff --git a/packages/core-internal/src/shared/auth.ts b/packages/core-internal/src/shared/auth.ts index 62995b8f65..876dc9ee03 100644 --- a/packages/core-internal/src/shared/auth.ts +++ b/packages/core-internal/src/shared/auth.ts @@ -30,6 +30,7 @@ export { OAuthErrorResponseSchema, OAuthMetadataSchema, OAuthProtectedResourceMetadataSchema, + OAuthTokenResponseSchema, OAuthTokenRevocationRequestSchema, OAuthTokensSchema, OpenIdProviderDiscoveryMetadataSchema, 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/core/src/auth.ts b/packages/core/src/auth.ts index e21076d817..f628f85bb5 100644 --- a/packages/core/src/auth.ts +++ b/packages/core/src/auth.ts @@ -143,6 +143,46 @@ 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 — 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. + * This shares the null-leniency goal of ElicitResult's `content` normalization + * but uses a stronger mechanic: ElicitResult maps `null` to a + * present-but-`undefined` member, whereas here the key must be strictly absent. + * + * 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 little bearing on enforcement — the resource + * server remains authoritative — but the SDK does use `tokens.scope` + * conservatively in the 403 `insufficient_scope` step-up path, where an + * absent scope is treated as the empty set and forces a fresh authorization + * request rather than a refresh. Consumers must not derive granted-scope + * conclusions from the member's absence; those 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/src/index.ts b/packages/core/src/index.ts index e8ddda2909..26c3677c41 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -184,11 +184,14 @@ export { // Auth schemas (OAuth / OpenID / IdJag) — kept as a SEPARATE group from the MCP spec schemas above, // mirroring the SDK's own spec-vs-auth split (these live in src/auth.ts, not src/schemas.ts, -// and are registered as `authSchemas` in core-internal's specTypeSchema.ts). This group is EXACTLY core-internal's +// and are registered as `authSchemas` in core-internal's specTypeSchema.ts). This group is core-internal's // `authSchemas` set — every auth schema that has a public spec type (so `isSpecType.OAuthTokens`, -// `isSpecType.IdJagTokenExchangeResponse`, etc. exist). The typeless internal URL field-validators +// `isSpecType.IdJagTokenExchangeResponse`, etc. exist). OAuthTokenResponseSchema is the one addition +// on top of that set: it introduces no spec type of its own (its output type is OAuthTokens) and +// stays out of `authSchemas`, but it is public v1 API and the v1-to-v2 codemod routes its imports +// here, so it needs this public home. The typeless internal URL field-validators // (SafeUrlSchema, OptionalSafeUrlSchema) are not auth schemas and stay out. The coreSchemas test -// asserts this group stays in sync with core-internal's `authSchemas`. +// asserts this group stays in sync with core-internal's `authSchemas` plus that one addition. export { IdJagTokenExchangeResponseSchema, OAuthClientInformationFullSchema, @@ -198,6 +201,7 @@ export { OAuthErrorResponseSchema, OAuthMetadataSchema, OAuthProtectedResourceMetadataSchema, + OAuthTokenResponseSchema, OAuthTokenRevocationRequestSchema, OAuthTokensSchema, OpenIdProviderDiscoveryMetadataSchema, diff --git a/packages/core/test/coreSchemas.test.ts b/packages/core/test/coreSchemas.test.ts index 3efcfdbd79..a0773fe38e 100644 --- a/packages/core/test/coreSchemas.test.ts +++ b/packages/core/test/coreSchemas.test.ts @@ -4,7 +4,7 @@ import { fileURLToPath } from 'node:url'; import { describe, expect, it } from 'vitest'; import * as core from '../src/index'; -import { CursorSchema, InitializeRequestSchema, OAuthTokensSchema } from '../src/index'; +import { CursorSchema, InitializeRequestSchema, OAuthTokenResponseSchema, OAuthTokensSchema } from '../src/index'; function readCore(relativePath: string): string { return readFileSync(fileURLToPath(new URL(relativePath, import.meta.url)), 'utf8'); @@ -22,6 +22,11 @@ describe('@modelcontextprotocol/core', () => { expect(InitializeRequestSchema.safeParse({}).success).toBe(false); expect(OAuthTokensSchema.safeParse({}).success).toBe(false); expect(OAuthTokensSchema.safeParse({ access_token: 'tok', token_type: 'Bearer' }).success).toBe(true); + // The null-tolerant wrapper is real too: null optional members parse as absent. + expect(OAuthTokenResponseSchema.parse({ access_token: 'tok', token_type: 'Bearer', refresh_token: null })).toStrictEqual({ + access_token: 'tok', + token_type: 'Bearer' + }); }); it('re-exports exactly core’s spec + OAuth schemas — no internal helpers (drift guard)', () => { @@ -51,7 +56,10 @@ describe('@modelcontextprotocol/core', () => { const authObj = specTypeSrc.slice(authStart, specTypeSrc.indexOf('} as const', authStart)); const authSchemas = exportedSchemaConsts(authObj, /\b(\w+Schema)\b/g); - const expected = [...specSchemas, ...authSchemas].sort(); + // OAuthTokenResponseSchema is the one export on top of the two groups: it has no spec type + // of its own (its output type is OAuthTokens) so it is not in `authSchemas`, and it is + // public v1 API that the v1-to-v2 codemod routes here, so core's barrel must export it. + const expected = [...specSchemas, ...authSchemas, 'OAuthTokenResponseSchema'].sort(); const exported = Object.keys(core).sort(); // Exact match, both directions: a new core spec/auth schema missing here fails (we forgot to // re-export it), and any internal helper / non-spec symbol that leaks here also fails. 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 {