From dbc5ffc169d19a5f2ae55c8904f5131a40b014ec Mon Sep 17 00:00:00 2001 From: Savan Patel <58150774+Ssavan99@users.noreply.github.com> Date: Wed, 9 Sep 2026 12:30:10 -0500 Subject: [PATCH 1/4] fix(auth): preserve state on authorization error redirects authorizationHandler read `state` out of the parse result after the parse had already been checked, so any Phase-2 validation failure threw before `state` was assigned and the error redirect went out without it. RFC 6749 4.1.2.1 requires `state` on the error response whenever the request carried one. A client performing the standard CSRF check has to reject a callback that lacks it, so the underlying `invalid_request` never reaches the user. Capture `state` from the raw request parameters before validation runs. --- .../src/auth/handlers/authorize.ts | 14 ++++- .../test/auth/handlers/authorize.test.ts | 62 +++++++++++++++++++ 2 files changed, 73 insertions(+), 3 deletions(-) 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({ From 1cf1f3d0ad07ed3fd99bc29dc8a8f4437495cffa Mon Sep 17 00:00:00 2001 From: Savan Patel <58150774+Ssavan99@users.noreply.github.com> Date: Wed, 9 Sep 2026 12:32:12 -0500 Subject: [PATCH 2/4] chore: add changeset --- .../preserve-state-on-authorize-error.md | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 .changeset/preserve-state-on-authorize-error.md 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. From 84df96bf6327f6dcb00e3cc0f060730bbf7423e0 Mon Sep 17 00:00:00 2001 From: Savan Patel <58150774+Ssavan99@users.noreply.github.com> Date: Thu, 10 Sep 2026 00:21:28 -0500 Subject: [PATCH 3/4] chore: add operator brief for scheduled PR check --- BABYSIT.md | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 BABYSIT.md diff --git a/BABYSIT.md b/BABYSIT.md new file mode 100644 index 0000000000..06ecdd2696 --- /dev/null +++ b/BABYSIT.md @@ -0,0 +1,59 @@ +# Daily check brief — PR #2774 + +Operator instructions for the scheduled cloud agent. Not part of the SDK. + +## The PR + +https://github.com/modelcontextprotocol/typescript-sdk/pull/2774 — +"fix(auth): preserve state on authorization error redirects", by Ssavan99, +fixes issue #2773. Branch `fix/authorize-state-on-error-redirect` on the fork +`Ssavan99/typescript-sdk`. Upstream: `modelcontextprotocol/typescript-sdk`. + +**What it changes.** In `packages/server-legacy/src/auth/handlers/authorize.ts`, +the OAuth `state` parameter was read out of the Phase-2 parse result *after* that +parse was checked, so any validation failure threw before the assignment and the +error redirect went out without `state`. RFC 6749 §4.1.2.1 requires `state` on +the error response whenever the request carried one. The fix captures `state` +from the raw request params before validation runs. Four regression tests live in +`packages/server-legacy/test/auth/handlers/authorize.test.ts`; three fail without +the fix, the fourth is a negative control. + +## Steps + +1. `gh pr view 2774 --repo modelcontextprotocol/typescript-sdk --json state,mergedAt,mergeable,mergeStateStatus,reviewDecision,comments,reviews,statusCheckRollup` + +2. **If MERGED** — this is the finish line. Report that it merged and when, and + state clearly that the routine must be turned off by hand at + https://claude.ai/code/routines, because you cannot disable yourself. Note + that branch `fix/stdio-close-releases-pipe-handles` is ready to open next + (the repo limits new contributors to one open PR, which is why it waited). + Do not open it yourself. + +3. **If CLOSED without merging** — report why, quote the closing comment, change + nothing. + +4. **If still open** — check, in order: + - **CI failures.** Fix them. Rebase on `upstream/main` if the branch is behind + or conflicted. Verify with `pnpm install --frozen-lockfile`, then + `pnpm vitest run` in `packages/server-legacy`, plus `pnpm typecheck` and + `pnpm lint` there. Push to the fork branch only when all three are clean. + - **Maintainer review comments.** Make the code changes they ask for and push + them. Keep the diff minimal — scope creep is an explicit rejection reason + in CONTRIBUTING.md. + - **Replies.** Do NOT post any comment, review reply, or issue comment. Write + proposed replies to `REPLY-DRAFT.md` in the repo root instead and surface + them in your report. CONTRIBUTING.md requires that answers to maintainers + come from the human contributor, not from an agent. + +## Hard limits + +- Touch only PR #2774 and its branch. Open no new PRs, file no issues, comment + nowhere. +- Never force-push. +- Every push must have tests, typecheck and lint green first. +- If anything is ambiguous, stop and report rather than guessing. + +## Report + +Say plainly: merged / closed / still open; what CI says; any new maintainer +comments verbatim; what you changed and pushed, if anything; what needs a human. From fe54b46d0d64a1661dc401bffc5f4841c8f5ea6c Mon Sep 17 00:00:00 2001 From: Savan Patel <58150774+Ssavan99@users.noreply.github.com> Date: Thu, 10 Sep 2026 00:23:23 -0500 Subject: [PATCH 4/4] chore: remove operator brief committed by mistake --- BABYSIT.md | 59 ------------------------------------------------------ 1 file changed, 59 deletions(-) delete mode 100644 BABYSIT.md diff --git a/BABYSIT.md b/BABYSIT.md deleted file mode 100644 index 06ecdd2696..0000000000 --- a/BABYSIT.md +++ /dev/null @@ -1,59 +0,0 @@ -# Daily check brief — PR #2774 - -Operator instructions for the scheduled cloud agent. Not part of the SDK. - -## The PR - -https://github.com/modelcontextprotocol/typescript-sdk/pull/2774 — -"fix(auth): preserve state on authorization error redirects", by Ssavan99, -fixes issue #2773. Branch `fix/authorize-state-on-error-redirect` on the fork -`Ssavan99/typescript-sdk`. Upstream: `modelcontextprotocol/typescript-sdk`. - -**What it changes.** In `packages/server-legacy/src/auth/handlers/authorize.ts`, -the OAuth `state` parameter was read out of the Phase-2 parse result *after* that -parse was checked, so any validation failure threw before the assignment and the -error redirect went out without `state`. RFC 6749 §4.1.2.1 requires `state` on -the error response whenever the request carried one. The fix captures `state` -from the raw request params before validation runs. Four regression tests live in -`packages/server-legacy/test/auth/handlers/authorize.test.ts`; three fail without -the fix, the fourth is a negative control. - -## Steps - -1. `gh pr view 2774 --repo modelcontextprotocol/typescript-sdk --json state,mergedAt,mergeable,mergeStateStatus,reviewDecision,comments,reviews,statusCheckRollup` - -2. **If MERGED** — this is the finish line. Report that it merged and when, and - state clearly that the routine must be turned off by hand at - https://claude.ai/code/routines, because you cannot disable yourself. Note - that branch `fix/stdio-close-releases-pipe-handles` is ready to open next - (the repo limits new contributors to one open PR, which is why it waited). - Do not open it yourself. - -3. **If CLOSED without merging** — report why, quote the closing comment, change - nothing. - -4. **If still open** — check, in order: - - **CI failures.** Fix them. Rebase on `upstream/main` if the branch is behind - or conflicted. Verify with `pnpm install --frozen-lockfile`, then - `pnpm vitest run` in `packages/server-legacy`, plus `pnpm typecheck` and - `pnpm lint` there. Push to the fork branch only when all three are clean. - - **Maintainer review comments.** Make the code changes they ask for and push - them. Keep the diff minimal — scope creep is an explicit rejection reason - in CONTRIBUTING.md. - - **Replies.** Do NOT post any comment, review reply, or issue comment. Write - proposed replies to `REPLY-DRAFT.md` in the repo root instead and surface - them in your report. CONTRIBUTING.md requires that answers to maintainers - come from the human contributor, not from an agent. - -## Hard limits - -- Touch only PR #2774 and its branch. Open no new PRs, file no issues, comment - nowhere. -- Never force-push. -- Every push must have tests, typecheck and lint green first. -- If anything is ambiguous, stop and report rather than guessing. - -## Report - -Say plainly: merged / closed / still open; what CI says; any new maintainer -comments verbatim; what you changed and pushed, if anything; what needs a human.