Skip to content

fix(auth): preserve state parameter on error redirects (RFC 6749 §4.1.2.1) - #2842

Open
insuffer wants to merge 1 commit into
modelcontextprotocol:v1.xfrom
insuffer:fix/2773-preserve-state-on-error-redirect
Open

insuffer wants to merge 1 commit into
modelcontextprotocol:v1.xfrom
insuffer:fix/2773-preserve-state-on-error-redirect

Conversation

@insuffer

Copy link
Copy Markdown

Problem

Per RFC 6749 §4.1.2.1, when an authorization request includes a state parameter and the server redirects back with an error, the redirect MUST echo the state so the client can correlate the response (CSRF protection).

In authorize.ts, Phase 2 assigns state only after RequestAuthorizationParamsSchema.safeParse succeeds:

let state;
try {
    const parseResult = RequestAuthorizationParamsSchema.safeParse(...);
    if (!parseResult.success) {
        throw new InvalidRequestError(parseResult.error.message); // state still undefined
    }
    state = parseResult.data.state;

So any parameter failing schema validation (e.g. missing code_challenge, unsupported code_challenge_method) produces an error redirect that silently drops the client's state. Reported in #2773.

Fix

Recover state from the raw request params (only when it is a string) before throwing on parse failure, so error redirects carry it as required. Successful-path behavior is unchanged.

Tests

Two regression tests (GET and POST): valid client_id/redirect_uri + state + a param that fails RequestAuthorizationParamsSchema → expect 302 Location containing both error=invalid_request and state. Both fail on v1.x before the fix.

  • test/server/auth/handlers/authorize.test.ts: 29/29 pass
  • Full suite (npm test, e2e excluded): 1649/1649 pass — the 2 unhandled stdio-serialization errors are pre-existing on a pristine v1.x checkout (verified via stash, unrelated to this change)
  • npm run typecheck, eslint and prettier all clean

Closes #2773

@changeset-bot

changeset-bot Bot commented Sep 22, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 8abdaaf

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@insuffer
insuffer requested review from a team as code owners September 22, 2026 16:10
@pkg-pr-new

pkg-pr-new Bot commented Sep 22, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@modelcontextprotocol/sdk@2842

commit: 8abdaaf

@claude claude Bot added the v1 Issues / PRs related to v1.x label Sep 23, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v1 Issues / PRs related to v1.x

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant