fix(auth): preserve state on authorization error redirects

.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.

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[] = [];

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({