authorizationHandler drops the OAuth state parameter on error redirects

Aperta Adatta ai principianti
#2,773 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub

Nessuno ha ancora preso questa issue.

Valutazione

Difficoltà
2/5
Tempo stimato
1-3 ore
Idoneità per principianti
88/100
Tipo di issue
Bug
Chiarezza
Specificata chiaramente
Stato di attività
Attiva
Stack tecnologico
typescript

Direzione di ricerca

Inizia da packages/server-legacy/src/auth/handlers/authorize.ts, in authorizationHandler, e segui il fallimento della validazione fino a createErrorRedirect. Aggiungi una copertura di regressione per i parametri di autorizzazione non validi con uno state fornito, quindi esegui i test auth pertinenti. Il lavoro è completo quando il reindirizzamento dell’errore conserva il valore originale di state, in conformità con RFC 6749 e il percorso di successo.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Descrizione

authorizationHandler omits state from the error redirect when authorization
parameter validation fails, so a client that validates state on the callback —
the standard CSRF check from RFC 6749 §4.1.2.1 — cannot accept or correlate the
error response. The client sees a state mismatch instead of the actual OAuth
error, and the real cause (invalid_request) never reaches the user.

RFC 6749 §4.1.2.1 requires state on the error response whenever the request
carried one:

state: REQUIRED if the "state" parameter was present in the client
authorization request. The exact value received from the client.

The success path echoes state correctly; only the error path drops it.

Reproduction
import express from 'express';
import { authorizationHandler } from '@modelcontextprotocol/sdk/server/auth/handlers/authorize.js';

const CLIENT = { client_id: 'test-client', redirect_uris: ['https://client.example.com/cb'] };
const provider = {
    clientsStore: { getClient: async id => (id === CLIENT.client_id ? CLIENT : undefined) },
    async authorize(client, params, res) {
        const u = new URL(params.redirectUri);
        u.searchParams.set('code', 'authcode');
        if (params.state) u.searchParams.set('state', params.state);
        res.redirect(302, u.href);
    }
};

const app = express();
app.use('/authorize', authorizationHandler({ provider, rateLimit: false }));
const server = app.listen(3000);

// state is echoed on success:
//   /authorize?client_id=test-client&redirect_uri=...&state=xyz789
//             &response_type=code&code_challenge=abc&code_challenge_method=S256
//   -> 302 https://client.example.com/cb?code=authcode&state=xyz789
//
// state is dropped as soon as any other parameter fails validation:
//   /authorize?client_id=test-client&redirect_uri=...&state=xyz789&response_type=code
//   -> 302 https://client.example.com/cb?error=invalid_request&error_description=...
//      (no state)

Any Phase-2 validation failure reproduces it: missing code_challenge,
code_challenge_method=plain, a non-URL resource.

Cause

In packages/server-legacy/src/auth/handlers/authorize.ts, state is read from
the parse result after the parse is checked:

let state;
try {
    const parseResult = RequestAuthorizationParamsSchema.safeParse(...);
    if (!parseResult.success) {
        throw new InvalidRequestError(parseResult.error.message);   // throws here
    }
    const { scope, code_challenge, resource } = parseResult.data;
    state = parseResult.data.state;                                 // never runs

The catch then calls createErrorRedirect(redirect_uri, error, state, issuer)
with state still undefined, and createErrorRedirect omits the parameter.

Expected

The error redirect carries state=xyz789, matching the success path and RFC 6749
§4.1.2.1.

Why this matters

I run an MCP server on this SDK in production — HTTP transport behind a public
URL, mcpAuthRouter with an OAuth read scope, remote clients — and found this
while reading through that auth path rather than from an incident. The impact is
still concrete: a client whose authorization request is malformed gets back a
callback it has to reject on CSRF grounds, so the operator sees a state-mismatch
error from the client and no trace of the real cause. It turns a one-line
parameter mistake into a blind debugging session, on the error path, which is
exactly where the diagnostic is supposed to work.

Environment
  • @modelcontextprotocol/sdk 1.29.0, and reproduced on main @ 5119ee7
  • Node 22.13.0, express 5.2.1

I have a fix with regression tests and can open a PR.

Disclosure: found and patched with AI assistance (Claude Code). I have reviewed
the diagnosis, the fix, and the tests, and can speak to any of it.

Lingua principale
TypeScript
Stelle
13.4k
Fork
2.2k
Merge medio
3g 12h
PR unite (30g)
3

Guida per i contributori

Apri la guida per i contributori

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di modelcontextprotocol/typescript-sdk

Tutte le issue di modelcontextprotocol/typescript-sdk

Issue simili

Altre issue su TypeScript

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.