Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/auth-iss-server-and-overload.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@modelcontextprotocol/client": minor
"@modelcontextprotocol/server-legacy": minor
---

SEP-2468 follow-up: `transport.finishAuth()` gains a `URLSearchParams` overload (preferred) that extracts `code`/`iss`, validates `iss` first, and on mismatch throws a sanitized `IssuerMismatchError` (no callback `error_description` text); callers remain responsible for `state`. **Behavior change for `@modelcontextprotocol/server-legacy`:** `mcpAuthRouter` now advertises `authorization_response_iss_parameter_supported` (default `true`; `ProxyOAuthServerProvider` reports `false`) and the bundled authorize handler appends `iss` (RFC 9207) to every `res.redirect(...)` your `OAuthServerProvider.authorize()` issues to the client's `redirect_uri`. If your provider redirects another way (`res.writeHead`, a separate consent-page response, or a standalone `authorizationHandler({provider})` without `issuerUrl`), append `params.issuer` as `iss` yourself or set `authorizationResponseIssParameterSupported: false` — otherwise RFC 9207-compliant clients (including this SDK) will reject the callback.
Comment thread
felixweinberger marked this conversation as resolved.
11 changes: 7 additions & 4 deletions docs/migration.md
Original file line number Diff line number Diff line change
Expand Up @@ -1601,16 +1601,19 @@ New TypeScript-only aliases `StoredOAuthTokens` and `StoredOAuthClientInformatio

`transport.finishAuth()` and `auth()` now validate the `iss` parameter from the authorization callback against the issuer recorded from the authorization server's validated metadata (RFC 9207). A **mismatched** `iss` is rejected with `IssuerMismatchError` before the code is exchanged regardless of what the AS advertised; a **missing** `iss` is rejected only when the AS advertised `authorization_response_iss_parameter_supported: true`.

**You must** pass the callback URL's query parameters to the SDK so it can read `iss` alongside `code`:
**You must** pass the callback URL's query parameters to the SDK so it can read `iss` alongside `code`. The SDK does **not** validate `state`; compare it to your stored value before calling `finishAuth`:

```typescript
const url = new URL(callbackUrl);
await transport.finishAuth(url.searchParams); // SDK reads `code` + `iss`
const params = new URL(callbackUrl).searchParams;
if (params.get('state') !== expectedState) throw new Error('state mismatch');
await transport.finishAuth(params); // SDK reads `code` + `iss`
```

`transport.finishAuth(code, iss)` remains supported for back-compat. If you bypass `auth()` and call `exchangeAuthorization()` / `fetchToken()` directly, pass `iss` in the options bag — the same validation runs there.

**You must not** display or act on `error`, `error_description`, or `error_uri` from the callback URL when `IssuerMismatchError` is thrown — those values are attacker-controlled in a mix-up attack.
**You must not** display or act on `error`, `error_description`, or `error_uri` from the callback URL when `IssuerMismatchError` is thrown — those values are attacker-controlled in a mix-up attack. The `URLSearchParams` overload handles this for you; if you parse the callback yourself, suppress them.

_(`@modelcontextprotocol/server-legacy` AS implementers — **behavior change**)_ `mcpAuthRouter()` now advertises `authorization_response_iss_parameter_supported` (default `true`) and the bundled authorize handler appends `iss` to **every** redirect — success or error — that your `OAuthServerProvider.authorize()` issues to the client's `redirect_uri` **via `res.redirect(...)` on the supplied `res`**. No provider change is required when that is how you redirect. If you emit the `Location` header another way (e.g. `res.writeHead(302, { Location })`), issue the final callback redirect from a different response (e.g. after a separate consent-page POST), or wire a standalone `authorizationHandler({provider})` without `issuerUrl`, append `params.issuer` as `iss` yourself — otherwise RFC 9207-compliant clients (including this SDK's) will reject the callback with `IssuerMismatchError`. If the callback is issued by an upstream AS you proxy to, set `authorizationResponseIssParameterSupported = false` on your provider (`ProxyOAuthServerProvider` does this) so the metadata does not over-claim.

`discoverAuthorizationServerMetadata()` now rejects metadata whose `issuer` does not exactly match the URL it was fetched for (RFC 8414 §3.3). If you connect to a known-misconfigured AS, set `skipIssuerMetadataValidation: true` on `StreamableHTTPClientTransportOptions` / `SSEClientTransportOptions` (or on `AuthOptions` if you call `auth()` directly, or `skipIssuerValidation: true` on the low-level helper) — **this weakens the mix-up defense and should be treated as a temporary workaround.** It suppresses only the metadata-echo check; the callback-`iss` validation always runs (and degrades to a no-op only when `iss` is absent and the AS does not advertise support).

Expand Down
37 changes: 36 additions & 1 deletion examples/shared/src/authServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,33 @@ export function setupAuthServer(options: SetupAuthServerOptions): void {
// toNodeHandler bypasses Express methods
const betterAuthHandler = toNodeHandler(auth);

// The issuer identifier this AS publishes in its metadata; must exactly match the
// `issuer` value better-auth emits at /.well-known/oauth-authorization-server.
const issuer = authServerUrl.toString().replace(/\/$/, '');
const issuerOrigin = new URL(issuer).origin;

// RFC 9207 (SEP-2468): append `iss` to every authorization-response redirect (success
// and error) that targets the client's redirect_uri. better-auth does not emit `iss`
// itself yet, so intercept the 302 Location header. Internal hops (to /sign-in or back
// to /api/auth/mcp/authorize) are left untouched.
authApp.use('/api/auth/mcp/authorize', (_req: Request, res: ExpressResponse, next: NextFunction) => {
const originalWriteHead = res.writeHead.bind(res);
// eslint-disable-next-line @typescript-eslint/no-explicit-any
res.writeHead = function (statusCode: number, ...args: any[]) {
const headers = args.find(a => typeof a === 'object' && a !== null) as Record<string, string> | undefined;
const loc = headers?.location ?? headers?.Location ?? (res.getHeader('Location') as string | undefined);
if (statusCode >= 300 && statusCode < 400 && loc && !loc.startsWith('/') && new URL(loc).origin !== issuerOrigin) {
const u = new URL(loc);
u.searchParams.set('iss', issuer);
if (headers && 'location' in headers) headers.location = u.href;
else if (headers && 'Location' in headers) headers.Location = u.href;
else res.setHeader('Location', u.href);
}
return originalWriteHead(statusCode, ...args);
} as typeof res.writeHead;
next();
});

// DEMO ONLY: simulate the user clicking "Approve" on the consent screen.
// The SDK auth driver appends `prompt=consent` whenever it requests the
// `offline_access` scope (per OIDC §11). With a real user, better-auth
Expand Down Expand Up @@ -207,7 +234,15 @@ export function setupAuthServer(options: SetupAuthServerOptions): void {
// OAuth metadata endpoints using better-auth's built-in handlers
// Add explicit OPTIONS handler for CORS preflight
authApp.options('/.well-known/oauth-authorization-server', cors());
authApp.get('/.well-known/oauth-authorization-server', cors(), toNodeHandler(oAuthDiscoveryMetadata(auth)));
// Wrap better-auth's metadata to advertise RFC 9207 support (the `iss` middleware
// above makes that claim true).
const discoveryHandler = oAuthDiscoveryMetadata(auth);
authApp.get('/.well-known/oauth-authorization-server', cors(), async (req: Request, res: ExpressResponse) => {
const upstream = await discoveryHandler(new Request(new URL(req.originalUrl, issuer)));
const body = (await upstream.json()) as Record<string, unknown>;
body.authorization_response_iss_parameter_supported = true;
res.status(upstream.status).json(body);
});

// Body parsers for non-better-auth routes (like /sign-in)
authApp.use(express.json());
Expand Down
63 changes: 63 additions & 0 deletions packages/client/src/client/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,69 @@ export function isStrictScopeSuperset(union: string | undefined, current: string
return false;
}

/**
* Shared `finishAuth` resolver for the `(code, iss?)` and `(URLSearchParams)` overloads.
*
* For the `URLSearchParams` form, only `iss` and `code` are read up front. When a `code` is
* present the returned values flow into {@linkcode auth}, which runs
* {@linkcode validateAuthorizationResponseIssuer} against freshly-discovered metadata before
* the code is redeemed — so on mismatch the thrown {@linkcode IssuerMismatchError} carries no
* `error`/`error_description`/`error_uri` text from the callback (those are attacker-controlled
* in a mix-up). When no `code` is present (an error-shaped callback), `iss` is validated here
* against the provider's recorded discovery state — or, when the provider does not implement
* `discoveryState`, against freshly-discovered metadata mirroring what {@linkcode auth} does on
* the code-present path — **before** the callback's error parameters are read; only after that
* passes are they surfaced as an {@linkcode OAuthError}. When no issuer baseline can be
* obtained either way, a generic {@linkcode UnauthorizedError} is thrown without surfacing the
* callback's `error`/`error_description`/`error_uri`.
*
* @internal Exported for the transport `finishAuth` overloads; not part of the public barrel.
*/
export async function resolveAuthorizationCallbackParams(
codeOrParams: string | URLSearchParams,
iss: string | undefined,
provider: OAuthClientProvider,
serverUrl: string | URL,
opts?: { fetchFn?: FetchLike; resourceMetadataUrl?: URL }
): Promise<{ authorizationCode: string; iss: string | undefined }> {
if (typeof codeOrParams === 'string') {
return { authorizationCode: codeOrParams, iss };
}
const issParam = codeOrParams.get('iss') ?? undefined;
const code = codeOrParams.get('code');
if (code) {
return { authorizationCode: code, iss: issParam };
}
// No code → error response. Gate the (potentially attacker-supplied) error params on the
// issuer first. Prefer the provider's recorded discovery state; when absent, mirror auth()'s
// code-present path and run a fresh discovery so the iss gate has an authentic baseline.
const discoveryState = await provider.discoveryState?.();
let metadata = discoveryState?.authorizationServerMetadata;
if (!metadata) {
try {
const serverInfo = await discoverOAuthServerInfo(serverUrl, opts);
Comment thread
felixweinberger marked this conversation as resolved.
metadata = serverInfo.authorizationServerMetadata;
} catch {
metadata = undefined;
}
}
if (!metadata) {
// No authentic baseline → cannot prove the error params came from our AS. Do NOT surface
// attacker-controllable `error`/`error_description`/`error_uri` here.
throw new UnauthorizedError('Authorization callback failed and the issuer could not be verified');
}
validateAuthorizationResponseIssuer({
iss: issParam,
expectedIssuer: metadata.issuer,
issParameterSupported: isIssParameterSupported(metadata)
});
const error = codeOrParams.get('error');
if (error) {
throw new OAuthError(error, codeOrParams.get('error_description') ?? error, codeOrParams.get('error_uri') ?? undefined);
}
throw new UnauthorizedError('Authorization callback contained neither `code` nor `error`');
}

export type ClientAuthMethod = 'client_secret_basic' | 'client_secret_post' | 'none';

function isClientAuthMethod(method: string): method is ClientAuthMethod {
Expand Down
40 changes: 25 additions & 15 deletions packages/client/src/client/sse.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,16 @@ import type { ErrorEvent, EventSourceInit } from 'eventsource';
import { EventSource } from 'eventsource';

import type { AuthProvider, OAuthClientProvider } from './auth.js';
import { adaptOAuthProvider, auth, extractWWWAuthenticateParams, isOAuthClientProvider, UnauthorizedError } from './auth.js';
import {
adaptOAuthProvider,
auth,
extractWWWAuthenticateParams,
isOAuthClientProvider,
resolveAuthorizationCallbackParams,
UnauthorizedError
} from './auth.js';
// eslint-disable-next-line @typescript-eslint/no-unused-vars -- referenced in JSDoc {@linkcode}
import type { IssuerMismatchError } from './authErrors.js';

export class SseError extends Error {
constructor(
Expand Down Expand Up @@ -242,10 +251,15 @@ export class SSEClientTransport implements Transport {
/**
* Call this method after the user has finished authorizing via their user agent and is redirected back to the MCP client application. This will exchange the authorization code for an access token, enabling the next connection attempt to successfully auth.
*
* Prefer passing the callback URL's `searchParams` directly — the SDK extracts
* `code` and `iss` (and validates `iss` per RFC 9207) for you. The `(code, iss?)`
* **Preferred:** pass the callback URL's `searchParams` directly. The SDK extracts `code`
* and `iss`, validates `iss` against the recorded issuer (RFC 9207) **before** reading any
* other parameter, and on mismatch throws an {@linkcode IssuerMismatchError} that carries
* none of the callback's `error`/`error_description`/`error_uri` text. The `(code, iss?)`
* positional form remains supported for back-compat.
*
* The SDK does **not** validate `state`; compare it to your stored value before calling
* `finishAuth`.
*
* @param callbackParams - The `URLSearchParams` from the authorization callback URL
* (e.g. `new URL(callbackUrl).searchParams`). `code` and `iss` are read from it.
*/
Expand All @@ -261,22 +275,18 @@ export class SSEClientTransport implements Transport {
throw new UnauthorizedError('finishAuth requires an OAuthClientProvider');
}

let authorizationCode: string;
if (codeOrParams instanceof URLSearchParams) {
const code = codeOrParams.get('code');
if (!code) {
throw new UnauthorizedError('Authorization callback is missing the "code" parameter');
}
authorizationCode = code;
iss = codeOrParams.get('iss') ?? undefined;
} else {
authorizationCode = codeOrParams;
}
const { authorizationCode, iss: issParam } = await resolveAuthorizationCallbackParams(
codeOrParams,
iss,
this._oauthProvider,
this._url,
{ fetchFn: this._fetchWithInit, resourceMetadataUrl: this._resourceMetadataUrl }
);

const result = await auth(this._oauthProvider, {
serverUrl: this._url,
authorizationCode,
iss,
iss: issParam,
resourceMetadataUrl: this._resourceMetadataUrl,
scope: this._scope,
fetchFn: this._fetchWithInit,
Expand Down
31 changes: 17 additions & 14 deletions packages/client/src/client/streamableHttp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
extractWWWAuthenticateParams,
isOAuthClientProvider,
isStrictScopeSuperset,
resolveAuthorizationCallbackParams,
UnauthorizedError
} from './auth.js';
// eslint-disable-next-line @typescript-eslint/no-unused-vars -- referenced via {@linkcode} in finishAuth JSDoc
Expand Down Expand Up @@ -829,10 +830,16 @@ export class StreamableHTTPClientTransport implements Transport {
/**
* Call this method after the user has finished authorizing via their user agent and is redirected back to the MCP client application. This will exchange the authorization code for an access token, enabling the next connection attempt to successfully auth.
*
* Prefer passing the callback URL's `searchParams` directly — the SDK extracts
* `code` and `iss` (and validates `iss` per RFC 9207) for you. The `(code, iss?)`
* **Preferred:** pass the callback URL's `searchParams` directly. The SDK extracts `code`
* and `iss`, validates `iss` against the recorded issuer (RFC 9207) **before** reading any
* other parameter, and on mismatch throws an {@linkcode IssuerMismatchError} that carries
* none of the callback's `error`/`error_description`/`error_uri` text — those are
* attacker-controlled in a mix-up attack and MUST NOT be displayed. The `(code, iss?)`
* positional form remains supported for back-compat.
*
* The SDK does **not** validate `state`; compare it to your stored value before calling
* `finishAuth`.
*
* @param callbackParams - The `URLSearchParams` from the authorization callback URL
* (e.g. `new URL(callbackUrl).searchParams`). `code` and `iss` are read from it.
*/
Expand All @@ -850,22 +857,18 @@ export class StreamableHTTPClientTransport implements Transport {
throw new UnauthorizedError('finishAuth requires an OAuthClientProvider');
}

let authorizationCode: string;
if (codeOrParams instanceof URLSearchParams) {
const code = codeOrParams.get('code');
if (!code) {
throw new UnauthorizedError('Authorization callback is missing the "code" parameter');
}
authorizationCode = code;
iss = codeOrParams.get('iss') ?? undefined;
} else {
authorizationCode = codeOrParams;
}
const { authorizationCode, iss: issParam } = await resolveAuthorizationCallbackParams(
codeOrParams,
iss,
this._oauthProvider,
this._url,
{ fetchFn: this._fetchWithInit, resourceMetadataUrl: this._resourceMetadataUrl }
);

const result = await auth(this._oauthProvider, {
serverUrl: this._url,
authorizationCode,
iss,
iss: issParam,
resourceMetadataUrl: this._resourceMetadataUrl,
scope: this._scope,
fetchFn: this._fetchWithInit,
Expand Down
Loading
Loading