Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 2.1k
fix(client): accumulate scopes (union) on step-up authorization challenges (SEP-2350)#2265
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
9bf6518b0099a6d33d069e48aabece09bd9bb9d8245e22d4d780d80b8cbd6126d7c3ae47f25d6696a3a3File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| --- | ||
| '@modelcontextprotocol/client': patch | ||
| --- | ||
| Accumulate scopes (union) when re-authorizing after a `403 insufficient_scope` step-up challenge (SEP-2350). Previously the challenged scopes replaced the requested scope, so per-operation challenges dropped previously granted permissions. The client now requests the union of | ||
| previously granted scopes (from stored tokens), previously requested scopes, protected resource metadata scopes, provider-configured default scopes, and the newly challenged scopes, using the existing exported `computeScopeUnion` helper. | ||
| The 401 re-authorization path now preserves accumulated `scope` and `resourceMetadataUrl` context too: `UnauthorizedContext` exposes optional `scope` / `resourceMetadataUrl` fields for custom `AuthProvider.onUnauthorized` handlers, and `handleOAuthUnauthorized` folds that context into the next `auth()` call. | ||
| The `withOAuth` fetch middleware likewise unions the stored token scope with the 401 challenge scope when re-authenticating. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -53,6 +53,10 @@ export interface UnauthorizedContext { | ||
| serverUrl: URL; | ||
| /** Fetch function configured with the transport's `requestInit`, for making auth requests. */ | ||
| fetchFn: FetchLike; | ||
| /** Accumulated OAuth scope from previous challenges, if the transport has one. */ | ||
| scope?: string; | ||
| /** Resource metadata URL from previous challenges, if the transport has one. */ | ||
| resourceMetadataUrl?: URL; | ||
| } | ||
mattzcarey marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| /** | ||
| @@ -177,11 +181,12 @@ export async function handleOAuthUnauthorized( | ||
| ctx: UnauthorizedContext, | ||
| extraAuthOptions?: Pick<AuthOptions, 'skipIssuerMetadataValidation'> | ||
| ): Promise<void> { | ||
| const { resourceMetadataUrl, scope } = extractWWWAuthenticateParams(ctx.response); | ||
| const challenge = extractWWWAuthenticateParams(ctx.response); | ||
| const tokens = await provider.tokens(); | ||
| const result = await auth(provider, { | ||
| serverUrl: ctx.serverUrl, | ||
| resourceMetadataUrl, | ||
| scope, | ||
| resourceMetadataUrl: challenge.resourceMetadataUrl ?? ctx.resourceMetadataUrl, | ||
| scope: computeScopeUnion(tokens?.scope, ctx.scope, challenge.scope), | ||
| fetchFn: ctx.fetchFn, | ||
| ...extraAuthOptions | ||
mattzcarey marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| }); | ||
mattzcarey marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| import type { FetchLike } from '@modelcontextprotocol/core-internal'; | ||
| import type { OAuthClientProvider } from './auth'; | ||
| import { auth, extractWWWAuthenticateParams, UnauthorizedError } from './auth'; | ||
| import { auth, computeScopeUnion, extractWWWAuthenticateParams, isStrictScopeSuperset, UnauthorizedError } from './auth'; | ||
| /** | ||
| * Middleware function that wraps and enhances fetch functionality. | ||
| @@ -39,11 +39,13 @@ export const withOAuth = | ||
| (provider: OAuthClientProvider, baseUrl?: string | URL): Middleware => | ||
| next => { | ||
| return async (input, init) => { | ||
| let lastTokenScope: string | undefined; | ||
| const makeRequest = async (): Promise<Response> => { | ||
| const headers = new Headers(init?.headers); | ||
| // Add authorization header if tokens are available | ||
| const tokens = await provider.tokens(); | ||
| lastTokenScope = tokens?.scope; | ||
| if (tokens) { | ||
| headers.set('Authorization', `Bearer ${tokens.access_token}`); | ||
| } | ||
| @@ -60,11 +62,14 @@ export const withOAuth = | ||
| // Use provided baseUrl or extract from request URL | ||
| const serverUrl = baseUrl || (typeof input === 'string' ? new URL(input).origin : input.origin); | ||
| const unionScope = computeScopeUnion(lastTokenScope, scope); | ||
| const forceReauthorization = lastTokenScope !== undefined && isStrictScopeSuperset(unionScope, lastTokenScope); | ||
| const result = await auth(provider, { | ||
| serverUrl, | ||
| resourceMetadataUrl, | ||
| scope, | ||
| scope: unionScope, | ||
| ...(forceReauthorization ? { forceReauthorization } : {}), | ||
mattzcarey marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| fetchFn: next | ||
| }); | ||
mattzcarey marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -14,6 +14,7 @@ import type { AuthProvider, OAuthClientProvider } from './auth'; | ||
| import { | ||
| adaptOAuthProvider, | ||
| auth, | ||
| computeScopeUnion, | ||
| extractWWWAuthenticateParams, | ||
| isOAuthClientProvider, | ||
| resolveAuthorizationCallbackParams, | ||
| @@ -164,8 +165,8 @@ export class SSEClientTransport implements Transport { | ||
| this._last401Response = response; | ||
| if (response.headers.has('www-authenticate')) { | ||
| const { resourceMetadataUrl, scope } = extractWWWAuthenticateParams(response); | ||
| this._resourceMetadataUrl = resourceMetadataUrl; | ||
| this._scope = scope; | ||
| this._resourceMetadataUrl = resourceMetadataUrl ?? this._resourceMetadataUrl; | ||
| this._scope = computeScopeUnion(this._scope, scope); | ||
| } | ||
| } | ||
claude[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| @@ -180,15 +181,23 @@ export class SSEClientTransport implements Transport { | ||
| const response = this._last401Response; | ||
| this._last401Response = undefined; | ||
| this._eventSource?.close(); | ||
| this._authProvider.onUnauthorized({ response, serverUrl: this._url, fetchFn: this._fetchWithInit }).then( | ||
| // onUnauthorized succeeded → retry fresh. Its onerror handles its own onerror?.() + reject. | ||
| () => this._startOrAuth().then(resolve, reject), | ||
| // onUnauthorized failed → not yet reported. | ||
| error => { | ||
| this.onerror?.(error); | ||
| reject(error); | ||
| } | ||
| ); | ||
| this._authProvider | ||
| .onUnauthorized({ | ||
| response, | ||
| serverUrl: this._url, | ||
| fetchFn: this._fetchWithInit, | ||
| resourceMetadataUrl: this._resourceMetadataUrl, | ||
| scope: this._scope | ||
| }) | ||
| .then( | ||
| // onUnauthorized succeeded → retry fresh. Its onerror handles its own onerror?.() + reject. | ||
| () => this._startOrAuth().then(resolve, reject), | ||
| // onUnauthorized failed → not yet reported. | ||
| error => { | ||
| this.onerror?.(error); | ||
| reject(error); | ||
| } | ||
| ); | ||
| return; | ||
| } | ||
| const error = new UnauthorizedError(); | ||
| @@ -328,15 +337,17 @@ export class SSEClientTransport implements Transport { | ||
| if (response.status === 401 && this._authProvider) { | ||
| if (response.headers.has('www-authenticate')) { | ||
| const { resourceMetadataUrl, scope } = extractWWWAuthenticateParams(response); | ||
| this._resourceMetadataUrl = resourceMetadataUrl; | ||
| this._scope = scope; | ||
| this._resourceMetadataUrl = resourceMetadataUrl ?? this._resourceMetadataUrl; | ||
| this._scope = computeScopeUnion(this._scope, scope); | ||
| } | ||
| if (this._authProvider.onUnauthorized && !isAuthRetry) { | ||
| await this._authProvider.onUnauthorized({ | ||
| response, | ||
| serverUrl: this._url, | ||
| fetchFn: this._fetchWithInit | ||
| fetchFn: this._fetchWithInit, | ||
| resourceMetadataUrl: this._resourceMetadataUrl, | ||
| scope: this._scope | ||
| }); | ||
| await response.text?.().catch(() => {}); | ||
| // Purposely _not_ awaited, so we don't call onerror twice | ||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.