-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix: include scope parameter in OAuth authorization code token exchange #1669
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
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 |
|---|---|---|
|
|
@@ -510,6 +510,12 @@ | |
| // The resolved scope is used consistently for both DCR and the authorization request. | ||
| const resolvedScope = scope || resourceMetadata?.scopes_supported?.join(' ') || provider.clientMetadata.scope; | ||
|
|
||
| // For the token exchange, only forward scope that was explicitly requested via | ||
| // WWW-Authenticate (the `scope` parameter). Do not inject PRM scopes_supported or | ||
| // clientMetadata.scope — either can exceed what the server granted, triggering | ||
| // invalid_scope errors per RFC 6749 §4.1.3. | ||
| const tokenExchangeScope = scope; | ||
|
|
||
| // Handle client registration if needed | ||
| let clientInformation = await Promise.resolve(provider.clientInformation()); | ||
| if (!clientInformation) { | ||
|
|
@@ -562,6 +568,7 @@ | |
| metadata, | ||
| resource, | ||
| authorizationCode, | ||
| scope: tokenExchangeScope, | ||
| fetchFn | ||
| }); | ||
|
|
||
|
|
@@ -1198,14 +1205,21 @@ | |
| export function prepareAuthorizationCodeRequest( | ||
| authorizationCode: string, | ||
| codeVerifier: string, | ||
| redirectUri: string | URL | ||
| redirectUri: string | URL, | ||
| scope?: string | ||
| ): URLSearchParams { | ||
|
Check warning on line 1210 in packages/client/src/client/auth.ts
|
||
|
Comment on lines
+1208
to
1210
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...Bug Analysis
This PR added an optional Concrete exampleA caller using const tokens = await exchangeAuthorization(serverUrl, {
clientInformation,
authorizationCode: code,
codeVerifier: verifier,
redirectUri: redirectUrl,
// No way to pass scope here — the options type doesn't include it
});The resulting token request will never include a Impact assessmentThis is not a regression — Suggested fixAdd an optional const tokenRequestParams = prepareAuthorizationCodeRequest(
authorizationCode, codeVerifier, redirectUri, scope
);This would make the public API surface consistent — all three levels (low-level |
||
| return new URLSearchParams({ | ||
| const params = new URLSearchParams({ | ||
| grant_type: 'authorization_code', | ||
| code: authorizationCode, | ||
| code_verifier: codeVerifier, | ||
| redirect_uri: String(redirectUri) | ||
| }); | ||
|
|
||
| if (scope) { | ||
| params.set('scope', scope); | ||
| } | ||
|
|
||
| return params; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -1401,21 +1415,25 @@ | |
| metadata, | ||
| resource, | ||
| authorizationCode, | ||
| scope, | ||
| fetchFn | ||
| }: { | ||
| metadata?: AuthorizationServerMetadata; | ||
| resource?: URL; | ||
| /** Authorization code for the default `authorization_code` grant flow */ | ||
| authorizationCode?: string; | ||
| scope?: string; | ||
| fetchFn?: FetchLike; | ||
| } = {} | ||
| ): Promise<OAuthTokens> { | ||
| const scope = provider.clientMetadata.scope; | ||
|
|
||
| // Use provider's prepareTokenRequest if available, otherwise fall back to authorization_code | ||
| let tokenRequestParams: URLSearchParams | undefined; | ||
| if (provider.prepareTokenRequest) { | ||
| tokenRequestParams = await provider.prepareTokenRequest(scope); | ||
| // For non-interactive flows (client_credentials, jwt-bearer), include the client's | ||
| // configured scope as a default — the server has never narrowed it via an authorization | ||
| // grant, so clientMetadata.scope is the expected starting point. | ||
| const tokenRequestScope = scope ?? provider.clientMetadata.scope; | ||
| tokenRequestParams = await provider.prepareTokenRequest(tokenRequestScope); | ||
| } | ||
|
|
||
| // Default to authorization_code grant if no custom prepareTokenRequest | ||
|
|
@@ -1427,7 +1445,10 @@ | |
| throw new Error('redirectUrl is required for authorization_code flow'); | ||
| } | ||
| const codeVerifier = await provider.codeVerifier(); | ||
| tokenRequestParams = prepareAuthorizationCodeRequest(authorizationCode, codeVerifier, provider.redirectUrl); | ||
| // Only include scope in the token request when explicitly provided — do not inject | ||
| // clientMetadata.scope because the provider may have narrowed the granted scope during | ||
| // authorization and re-sending a broader scope would cause an invalid_scope error. | ||
| tokenRequestParams = prepareAuthorizationCodeRequest(authorizationCode, codeVerifier, provider.redirectUrl, scope); | ||
| } | ||
|
|
||
| const clientInformation = await provider.clientInformation(); | ||
|
|
||
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.
What is this trying to achieve?
The scope we send to
/tokenshould be the scope we sent to/authorize, so we should always use the same scope calculation logic. We recently made a change in the refresh flow to trigger an auth flow if the scope exceeds the scope on the existing token which might be relevant here.Scopes also accumulate over 403's so it would be necessary to increase the scope across multiple challenges as well.