Repository navigation
[v1.x] Bind stored OAuth credentials to the authorization server that issued them #2888
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| '@modelcontextprotocol/sdk': patch | ||
| --- | ||
|
|
||
| Bind stored OAuth client credentials to the authorization server that issued them. `auth()` adds an `issuer` property to what it passes to `saveClientInformation()` / `saveTokens()` and does not reuse a stored value with a different authorization server: the client registers or | ||
| authorizes again, or `auth()` throws when the credentials cannot be re-created by registering. `OAuthTokensSchema` and `OAuthClientInformationSchema` accept the optional `issuer`. `ClientCredentialsProvider`, `PrivateKeyJwtProvider` and `StaticPrivateKeyJwtProvider` accept | ||
| `expectedIssuer`; omitting it is deprecated. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,6 +74,10 @@ export interface OAuthClientProvider { | |
| * Loads information about this OAuth client, as registered already with the | ||
| * server, or returns `undefined` if the client is not registered with the | ||
| * server. | ||
| * | ||
| * A provider that returns pre-registered credentials should include `issuer`, the URL of | ||
| * the authorization server they were registered with; {@linkcode auth} then does not | ||
| * present them to any other. | ||
| */ | ||
| clientInformation(): OAuthClientInformationMixed | undefined | Promise<OAuthClientInformationMixed | undefined>; | ||
|
|
||
|
|
@@ -84,6 +88,10 @@ export interface OAuthClientProvider { | |
| * | ||
| * This method is not required to be implemented if client information is | ||
| * statically known (e.g., pre-registered). | ||
| * | ||
| * The object carries `issuer`, the authorization server it was obtained from. Store it | ||
| * unchanged so that {@linkcode auth} does not reuse the registration with a different one. | ||
| * Client information stored without `issuer` is passed here, with it, after its first successful use. | ||
| */ | ||
| saveClientInformation?(clientInformation: OAuthClientInformationMixed): void | Promise<void>; | ||
|
|
||
|
|
@@ -96,6 +104,9 @@ export interface OAuthClientProvider { | |
| /** | ||
| * Stores new OAuth tokens for the current session, after a successful | ||
| * authorization. | ||
| * | ||
| * The object carries `issuer`, the authorization server that issued the tokens. Store it | ||
| * unchanged so that {@linkcode auth} does not present the refresh token to a different one. | ||
| */ | ||
| saveTokens(tokens: OAuthTokens): void | Promise<void>; | ||
|
|
||
|
|
@@ -392,6 +403,47 @@ export async function parseErrorResponse(input: Response | string): Promise<OAut | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Compares two authorization server identifiers, tolerating a single trailing `/` | ||
| * difference (`String(new URL(...))` is slash-suffixed, advertised values often are not). | ||
| */ | ||
| function issuersMatch(a: string, b: string): boolean { | ||
| let [x, y] = [a, b]; | ||
| try { | ||
| // Two URLs are compared as parsed, so the spelling of scheme, host or default port does not matter. | ||
| [x, y] = [new URL(a).href, new URL(b).href]; | ||
| } catch { | ||
| // Not two URLs: compared as written. | ||
| } | ||
| return x === y || (x.endsWith('/') && x.slice(0, -1) === y) || (y.endsWith('/') && y.slice(0, -1) === x); | ||
| } | ||
|
maxisbey marked this conversation as resolved.
|
||
|
|
||
| /** | ||
| * {@linkcode auth} stamps everything it passes to `saveClientInformation` / `saveTokens` with | ||
| * `issuer`, the authorization server URL used for discovery. This returns `stored` unless its | ||
| * stamp names a different authorization server, in which case the caller behaves as if nothing | ||
| * were stored. A value without a stamp (saved by an earlier version) is returned as-is, and | ||
| * is stamped once this authorization server has accepted it. | ||
| */ | ||
| function discardIfIssuerMismatch<T extends { issuer?: string }>(stored: T | null | undefined, issuer: string): T | undefined { | ||
| // `null`: a `JSON.parse(storage.getItem(...))`-style getter with nothing stored. | ||
| if (!stored) return undefined; | ||
| // A stamp that is not a string (raw storage) counts as no stamp. | ||
| if (typeof stored.issuer !== 'string') return stored.issuer == null ? stored : { ...stored, issuer: undefined }; | ||
| return issuersMatch(stored.issuer, issuer) ? stored : undefined; | ||
| } | ||
|
|
||
| // `issuer` is added by the client when it stores a value; authorization server responses are parsed without it. | ||
| const TokenResponseSchema = OAuthTokensSchema.omit({ issuer: true }); | ||
| const RegistrationResponseSchema = OAuthClientInformationFullSchema.omit({ issuer: true }); | ||
|
|
||
| function boundElsewhereError(stored: { issuer?: string }, issuer: string): Error { | ||
| return new Error( | ||
| `OAuth client information is bound to authorization server ${stored.issuer} and is not presented to ${issuer}. ` + | ||
| 'Clear the stored client information, or correct `expectedIssuer`, if the authorization server has moved.' | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Orchestrates the full auth flow with a server. | ||
| * | ||
|
|
@@ -503,6 +555,12 @@ async function authInternal( | |
| }); | ||
| } | ||
|
|
||
| // Binding key for stored credentials: the authorization server URL that discovery used | ||
| // (advertised by the resource server, restored from discoveryState(), or the legacy | ||
| // resource-origin fallback). `metadata.issuer` is deliberately not used: this version does | ||
| // not check that the metadata document echoes the URL it was fetched from. | ||
| const issuer = String(authorizationServerUrl); | ||
|
|
||
| // Send the metadata's resource indicator verbatim: `selectResourceURL` returns a parsed | ||
| // `URL`, and `URL.href` appends "/" to a pathless indicator such as `https://example.com`, | ||
| // which exact-match authorization servers reject (#1968). A URL returned by the | ||
|
|
@@ -518,8 +576,26 @@ async function authInternal( | |
| // The resolved scope is used consistently for both DCR and the authorization request. | ||
| const resolvedScope = scope || resourceMetadata?.scopes_supported?.join(' ') || provider.clientMetadata.scope; | ||
|
|
||
| // Handle client registration if needed. Client information stamped for a different | ||
| // authorization server reads back as `undefined`, so the flow re-registers exactly as if | ||
| // nothing were stored. | ||
| const storedClientInformation = await Promise.resolve(provider.clientInformation()); | ||
| let clientInformation = discardIfIssuerMismatch(storedClientInformation, issuer); | ||
| const canRegisterAgain = provider.saveClientInformation !== undefined && !!provider.redirectUrl; | ||
|
Contributor
There was a problem hiding this comment. 🟣 pre-existing, not blocking: pre-existing, security-relevant: interactive clients with a custom Why this was flaggedA provider has redirectUrl, saveClientInformation and a custom addClientAuthentication built with createPrivateKeyJwtAuth (static iss/sub = the client_id registered at AS-A, key held by the app), and its stored client information is stamped AS-A. A malicious MCP server advertises its own AS-B. At src/client/auth.ts:570 the stored value reads back undefined; src/client/auth.ts:571 now makes canRegisterAgain true because the addClientAuthentication === undefined clause was dropped, so src/client/auth.ts:612-620 registers at AS-B and the flow redirects there. AS-B auto-approves and returns a code; fetchToken at src/client/auth.ts:1519-1523 calls the custom authentication with AS-B's metadata. createPrivateKeyJwtAuth at src/client/auth-extensions.ts:38 takes the audience from metadata?.issuer, and src/client/auth.ts:545-549 states this version never compares metadata.issuer with the URL it was fetched from, so AS-B sets issuer to AS-A's and receives a signed assertion valid at AS-A for 300 s (client_credentials or any grant AS-A allows that client). The base branch leaks the same way (it… Verification: pre-existing (security-relevant). Triggering condition: an interactive provider (redirectUrl + saveClientInformation) with a custom |
||
| if (storedClientInformation && !clientInformation && !canRegisterAgain) { | ||
| // Pre-registered credentials, or a non-interactive client running on configured | ||
| // credentials: neither can be re-created by registering with this authorization server. | ||
| throw boundElsewhereError(storedClientInformation, issuer); | ||
| } | ||
| // Saved by an earlier version: bound to the first authorization server that accepts it. | ||
| const unstampedClientInformation = clientInformation?.issuer == null ? clientInformation : undefined; | ||
| const bindClientInformation = async () => { | ||
| try { | ||
| if (unstampedClientInformation) await provider.saveClientInformation?.({ ...unstampedClientInformation, issuer }); | ||
| } catch { | ||
|
maxisbey marked this conversation as resolved.
|
||
| // A provider that only expects this call after a registration keeps working, unbound. | ||
| } | ||
| }; | ||
| if (!clientInformation) { | ||
| if (authorizationCode !== undefined) { | ||
| throw new Error('Existing OAuth client information is required when exchanging an authorization code'); | ||
|
maxisbey marked this conversation as resolved.
|
||
|
|
@@ -538,9 +614,7 @@ async function authInternal( | |
|
|
||
| if (shouldUseUrlBasedClientId) { | ||
| // SEP-991: URL-based Client IDs | ||
| clientInformation = { client_id: clientMetadataUrl, issuer }; | ||
| await provider.saveClientInformation?.(clientInformation); | ||
| } else { | ||
| // Fallback to dynamic registration | ||
|
|
@@ -555,8 +629,8 @@ async function authInternal( | |
| fetchFn | ||
| }); | ||
|
|
||
| clientInformation = { ...fullInformation, issuer }; | ||
| await provider.saveClientInformation(clientInformation); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -572,11 +646,22 @@ async function authInternal( | |
| fetchFn | ||
| }); | ||
|
|
||
| await bindClientInformation(); | ||
| await provider.saveTokens({ ...tokens, issuer }); | ||
| return 'AUTHORIZED'; | ||
| } | ||
|
|
||
| // A refresh token stamped for a different authorization server reads back as `undefined`, | ||
| // so it is never posted to this one's token endpoint. | ||
| const tokens = discardIfIssuerMismatch(await provider.tokens(), issuer); | ||
| if (tokens?.refresh_token && tokens.issuer == null) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn( | ||
| "[mcp-sdk] stored OAuth tokens have no 'issuer' property (saved by an earlier version, or by a provider that " + | ||
| 'does not keep it) and are used as-is; make sure your OAuthClientProvider stores what saveTokens() and ' + | ||
| 'saveClientInformation() receive unchanged.' | ||
| ); | ||
| } | ||
|
maxisbey marked this conversation as resolved.
maxisbey marked this conversation as resolved.
|
||
|
|
||
| // Handle token refresh or new authorization | ||
| if (tokens?.refresh_token) { | ||
|
|
@@ -591,7 +676,8 @@ async function authInternal( | |
| fetchFn | ||
| }); | ||
|
|
||
| await bindClientInformation(); | ||
| await provider.saveTokens({ ...newTokens, issuer }); | ||
| return 'AUTHORIZED'; | ||
| } catch (error) { | ||
| // If this is a ServerError, or an unknown type, log it out and try to continue. Otherwise, escalate so we can fix things and retry. | ||
|
|
@@ -1276,7 +1362,7 @@ async function executeTokenRequest( | |
| throw await parseErrorResponse(response); | ||
| } | ||
|
|
||
| return TokenResponseSchema.parse(await response.json()); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -1415,6 +1501,17 @@ export async function fetchToken( | |
| fetchFn?: FetchLike; | ||
| } = {} | ||
| ): Promise<OAuthTokens> { | ||
| // Nothing is sent to an authorization server other than the one the client information is stamped for. | ||
| const readClientInformation = async () => { | ||
|
Contributor
There was a problem hiding this comment. 🟡 nit (optional): maintainers get two new async closures in the OAuth client layer with no declared return type, against the CLAUDE.md rule of explicit return types. Why this was flaggedNothing fails at runtime. The diff adds Verification: nit. Triggering condition: none at runtime; this is a repository-instruction violation only. The instruction exists as quoted: /home/claude/typescript-sdk/CLAUDE.md line 20 reads "- TypeScript: Strict type checking, ES modules, explicit return types", with no qualifier limiting it to exported or top-level functions. Both cited sites are added by this diff (git diff 289ac2c..HEAD --… | nit.… |
||
| const storedClientInformation = await provider.clientInformation(); | ||
| const checked = discardIfIssuerMismatch(storedClientInformation, String(authorizationServerUrl)); | ||
| if (storedClientInformation && !checked) { | ||
| throw boundElsewhereError(storedClientInformation, String(authorizationServerUrl)); | ||
| } | ||
| return checked; | ||
| }; | ||
| let clientInformation = await readClientInformation(); | ||
|
|
||
| const scope = provider.clientMetadata.scope; | ||
|
|
||
| // Use provider's prepareTokenRequest if available, otherwise fall back to authorization_code | ||
|
|
@@ -1435,12 +1532,13 @@ export async function fetchToken( | |
| tokenRequestParams = prepareAuthorizationCodeRequest(authorizationCode, codeVerifier, provider.redirectUrl); | ||
| } | ||
|
|
||
| // A provider may fill in its client information while the request is prepared. | ||
| clientInformation ??= await readClientInformation(); | ||
|
|
||
| return executeTokenRequest(authorizationServerUrl, { | ||
| metadata, | ||
| tokenRequestParams, | ||
| clientInformation, | ||
| addClientAuthentication: provider.addClientAuthentication, | ||
| resource, | ||
| fetchFn | ||
|
|
@@ -1495,5 +1593,5 @@ export async function registerClient( | |
| throw await parseErrorResponse(response); | ||
| } | ||
|
|
||
| return RegistrationResponseSchema.parse(await response.json()); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.