diff --git a/.changeset/preserve-state-on-authorize-error.md b/.changeset/preserve-state-on-authorize-error.md new file mode 100644 index 0000000000..5d77c3136b --- /dev/null +++ b/.changeset/preserve-state-on-authorize-error.md @@ -0,0 +1,20 @@ +--- +'@modelcontextprotocol/server-legacy': patch +--- + +Preserve the OAuth `state` parameter on authorization error redirects. In +`authorizationHandler`, `state` was read out of the Phase-2 parse result after that +parse had already been checked, so any validation failure — a missing `code_challenge`, +an unsupported `code_challenge_method`, a non-URL `resource` — threw before the +assignment ran, and `createErrorRedirect` then built the redirect with `state` still +`undefined` and omitted the parameter. + +RFC 6749 §4.1.2.1 requires `state` on the error response whenever the authorization +request carried one. Without it a client performing the standard CSRF check has to +reject the callback, so the `invalid_request` describing the actual problem never +reaches the user: the failure surfaces as a state mismatch on the client instead, on +the error path, where the diagnostic matters most. + +`state` is now captured from the raw request parameters before validation runs. The +success path is unchanged, and a request that carried no `state` still gets an error +redirect without one. diff --git a/packages/server-legacy/src/auth/handlers/authorize.ts b/packages/server-legacy/src/auth/handlers/authorize.ts index d9c1508710..12d62e4e32 100644 --- a/packages/server-legacy/src/auth/handlers/authorize.ts +++ b/packages/server-legacy/src/auth/handlers/authorize.ts @@ -151,16 +151,24 @@ export function authorizationHandler({ provider, issuerUrl, rateLimit: rateLimit } // Phase 2: Validate other parameters. Any errors here should go into redirect responses. - let state; + let state: string | undefined; try { + const params = req.method === 'POST' ? req.body : req.query; + + // RFC 6749 4.1.2.1: the error response MUST carry `state` whenever the + // request did. Capture it before schema validation, which throws on any + // other malformed parameter and would otherwise drop it. + if (typeof params?.state === 'string') { + state = params.state; + } + // Parse and validate authorization parameters - const parseResult = RequestAuthorizationParamsSchema.safeParse(req.method === 'POST' ? req.body : req.query); + const parseResult = RequestAuthorizationParamsSchema.safeParse(params); if (!parseResult.success) { throw new InvalidRequestError(parseResult.error.message); } const { scope, code_challenge, resource } = parseResult.data; - state = parseResult.data.state; // Validate scopes let requestedScopes: string[] = []; diff --git a/packages/server-legacy/test/auth/handlers/authorize.test.ts b/packages/server-legacy/test/auth/handlers/authorize.test.ts index 9f87a4ae6f..f7181b1d75 100644 --- a/packages/server-legacy/test/auth/handlers/authorize.test.ts +++ b/packages/server-legacy/test/auth/handlers/authorize.test.ts @@ -350,6 +350,68 @@ describe('Authorization Handler', () => { }); }); + describe('State on error redirects', () => { + // RFC 6749 4.1.2.1: the error response MUST include `state` when the + // authorization request carried one, so the client can correlate the + // callback with its pending request and surface the actual error. + it('preserves state when a required parameter is missing', async () => { + const response = await supertest(app).get('/authorize').query({ + client_id: 'valid-client', + redirect_uri: 'https://example.com/callback', + response_type: 'code', + state: 'state-value-123' + }); + + expect(response.status).toBe(302); + const location = new URL(response.header.location!); + expect(location.searchParams.get('error')).toBe('invalid_request'); + expect(location.searchParams.get('state')).toBe('state-value-123'); + }); + + it('preserves state when code_challenge_method is unsupported', async () => { + const response = await supertest(app).get('/authorize').query({ + client_id: 'valid-client', + redirect_uri: 'https://example.com/callback', + response_type: 'code', + code_challenge: 'challenge123', + code_challenge_method: 'plain', + state: 'state-value-123' + }); + + expect(response.status).toBe(302); + const location = new URL(response.header.location!); + expect(location.searchParams.get('error')).toBe('invalid_request'); + expect(location.searchParams.get('state')).toBe('state-value-123'); + }); + + it('preserves state on error redirects for POST requests', async () => { + const response = await supertest(app).post('/authorize').type('form').send({ + client_id: 'valid-client', + redirect_uri: 'https://example.com/callback', + response_type: 'code', + state: 'state-value-123' + }); + + expect(response.status).toBe(302); + const location = new URL(response.header.location!); + expect(location.searchParams.get('error')).toBe('invalid_request'); + expect(location.searchParams.get('state')).toBe('state-value-123'); + }); + + it('omits state on error redirects when the request had none', async () => { + const response = await supertest(app).get('/authorize').query({ + client_id: 'valid-client', + redirect_uri: 'https://example.com/callback', + response_type: 'code' + }); + + expect(response.status).toBe(302); + const location = new URL(response.header.location!); + expect(location.searchParams.get('error')).toBe('invalid_request'); + expect(location.searchParams.has('state')).toBe(false); + }); + }); + describe('Successful authorization', () => { it('handles successful authorization with all parameters', async () => { const response = await supertest(app).get('/authorize').query({