-
Notifications
You must be signed in to change notification settings - Fork 1.7k
SEP-2207: Refresh token guidance #1523
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
base: main
Are you sure you want to change the base?
Changes from all commits
0ea0a0a
b0943bc
3a872b6
cef10af
8939679
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -467,6 +467,38 @@ | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Selects scopes per the MCP spec and augment for refresh token support. | ||
| */ | ||
| export function determineScope(options: { | ||
| requestedScope?: string; | ||
| resourceMetadata?: OAuthProtectedResourceMetadata; | ||
| authServerMetadata?: AuthorizationServerMetadata; | ||
| clientMetadata: OAuthClientMetadata; | ||
| }): string | undefined { | ||
| const { requestedScope, resourceMetadata, authServerMetadata, clientMetadata } = options; | ||
|
|
||
| // Scope selection priority (MCP spec): | ||
| // 1. WWW-Authenticate header scope | ||
| // 2. PRM scopes_supported | ||
| // 3. clientMetadata.scope (SDK fallback) | ||
| // 4. Omit scope parameter | ||
| let effectiveScope = requestedScope || resourceMetadata?.scopes_supported?.join(' ') || clientMetadata.scope; | ||
|
|
||
| // SEP-2207: Append offline_access when the AS advertises it | ||
| // and the client supports the refresh_token grant. | ||
| if ( | ||
| effectiveScope && | ||
| authServerMetadata?.scopes_supported?.includes('offline_access') && | ||
| !effectiveScope.split(' ').includes('offline_access') && | ||
|
Check notice on line 493 in packages/client/src/client/auth.ts
|
||
| clientMetadata.grant_types?.includes('refresh_token') | ||
| ) { | ||
| effectiveScope = `${effectiveScope} offline_access`; | ||
| } | ||
|
|
||
| return effectiveScope; | ||
| } | ||
|
|
||
| async function authInternal( | ||
| provider: OAuthClientProvider, | ||
| { | ||
|
|
@@ -555,15 +587,16 @@ | |
| await provider.saveResourceUrl?.(String(resource)); | ||
| } | ||
|
|
||
| // Apply scope selection strategy (SEP-835): | ||
| // 1. WWW-Authenticate scope (passed via `scope` param) | ||
| // 2. PRM scopes_supported | ||
| // 3. Client metadata scope (user-configured fallback) | ||
| // The resolved scope is used consistently for both DCR and the authorization request. | ||
| const resolvedScope = scope || resourceMetadata?.scopes_supported?.join(' ') || provider.clientMetadata.scope; | ||
| // Scope selection used consistently for DCR and the authorization request. | ||
| const resolvedScope = determineScope({ | ||
| requestedScope: scope, | ||
| resourceMetadata, | ||
| authServerMetadata: metadata, | ||
| clientMetadata: provider.clientMetadata | ||
| }); | ||
|
|
||
| // Handle client registration if needed | ||
| let clientInformation = await Promise.resolve(provider.clientInformation()); | ||
|
Check warning on line 599 in packages/client/src/client/auth.ts
|
||
|
Comment on lines
+590
to
599
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Nit: Extended reasoning...What the bug isIn How fetchToken handles scope differentlyInside Step-by-step proof for non-interactive flowConsider a
Why the practical impact is minimalFor authorization_code flows (the primary target of this PR), For client_credentials flows, RFC 6749 §4.4.3 states the AS "MUST NOT issue a refresh token," making Suggested fixPass
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @felixweinberger happy to update this if you want. Was trying to be surgical, but the review is probably accurate. lmk |
||
| if (!clientInformation) { | ||
| if (authorizationCode !== undefined) { | ||
| throw new Error('Existing OAuth client information is required when exchanging an authorization code'); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟣 Nit (pre-existing):
startAuthorization()at line 1259 usesscope?.includes('offline_access')(substring match on the string), while the newdetermineScope()correctly uses.split(' ').includes('offline_access')(exact token match). A hypothetical scope like"no_offline_access"would incorrectly triggerprompt=consent. Consider updating line 1259 toscope?.split(' ').includes('offline_access')for consistency.Extended reasoning...
Bug Analysis
The new
determineScope()function (line 493) correctly checks whetheroffline_accessis present in a space-delimited scope string using exact token matching:However, the consumer of this scope —
startAuthorization()at line 1259 — checks for the same scope token usingString.prototype.includes(), which is a substring match:How it manifests
Consider a scope string like
"mcp:read no_offline_access". ThedetermineScope()function would correctly determine thatoffline_accessis not present as a discrete scope token. But when this scope string reachesstartAuthorization(), the substring check"mcp:read no_offline_access".includes("offline_access")returnstrue, causingprompt=consentto be incorrectly appended to the authorization URL.Step-by-step proof
determineScope()returns"mcp:read no_offline_access"(no augmentation, sinceoffline_accessis not a discrete token).authInternal()passes this asscope: resolvedScopetostartAuthorization().startAuthorization(), line 1259 evaluates:"mcp:read no_offline_access"?.includes("offline_access")→true.prompt=consentis appended to the authorization URL — incorrectly, sinceoffline_accesswas never actually requested.Why existing code does not prevent it
The
determineScope()function uses the correct exact-token approach, but it cannot control how downstream consumers check the scope string. ThestartAuthorization()function predates this PR and was not updated.Impact
This is a pre-existing issue — the substring check in
startAuthorization()was there before this PR. However, this PR makesoffline_accessappear in scope strings more frequently via the SEP-2207 augmentation logic, which means the code path at line 1259 gets exercised more often. In practice, a scope token containing "offline_access" as a substring is extremely unlikely in real OAuth deployments, so the practical impact is negligible.Fix
Replace the substring check on line 1259 with an exact token match:
This is a one-line change that aligns
startAuthorization()with the correct pattern already used indetermineScope().There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felixweinberger I'm happy to update this as well if you like, but we don't need to increase the scope here unnecessarily.