-
Notifications
You must be signed in to change notification settings - Fork 0
fix: uses db scope #7
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Pull request overview
This pull request aims to fix scope handling in the OAuth implementation by retrieving granted scopes from the database (oauthConsents table) rather than relying solely on scopes embedded in tokens. The primary changes modify the token generation and userinfo endpoints to query the database for user-granted scopes.
Changes:
- Updated OAuth userinfo endpoint to fetch granted scopes from database and conditionally return email claims
- Modified token endpoint to query database for granted scopes when generating ID tokens
- Removed offline_access scope from test assertions
- Added .env*.local pattern to .gitignore
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| app/api/oauth/userinfo/route.ts | Fetches granted scopes from database; however, unconditionally includes profile claims while checking for email scope |
| app/api/oauth/token/route.ts | Queries database for granted scopes but creates inconsistency between access token scopes and ID token claims |
| lib/scopes.test.ts | Removes offline_access scope test expectations, creating inconsistency with implementation |
| .gitignore | Standard addition of local environment file pattern |
Comments suppressed due to low confidence (1)
app/api/oauth/token/route.ts:156
- The code retrieves grantedScopes from the database but then uses codeData.scopes for generating the access token's scope claim. This creates an inconsistency where:
- The access token contains scopes from codeData.scopes (line 151-156)
- The ID token uses grantedScopes from the database for conditional claims (lines 163-164)
This mismatch could lead to authorization issues where the access token advertises scopes that may not align with what was actually granted and stored in the database. Consider using grantedScopes consistently for both token generation, or clarify the intended behavior with comments.
// Generate tokens
const scopeString = codeData.scopes.join(" ");
const accessToken = await signAccessToken({
sub: user.id,
client_id: client.id,
scope: scopeString,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| test("should contain expected special scopes", () => { | ||
| const keys = SPECIAL_SCOPES.map((s) => s.key); | ||
| expect(keys).toContain("openid"); | ||
| expect(keys).toContain("offline_access"); | ||
| }); |
Copilot
AI
Jan 18, 2026
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.
The tests have been updated to remove expectations for "offline_access" scope from SPECIAL_SCOPES and SCOPES arrays. However, the OAuth token endpoint still checks for "offline_access" scope (line 177 in app/api/oauth/token/route.ts) and the discovery configuration still advertises it as a supported scope (line 16 in lib/oauth/discovery.ts). This creates an inconsistency where:
- The scope system doesn't define offline_access
- Tests don't verify its existence
- But the implementation still uses it functionally
Consider either: (a) adding offline_access back to SPECIAL_SCOPES if it should be supported, or (b) removing/updating the functional code that references it to align with the scope definitions.
| // Build response - always include profile, email requires scope from DB | ||
| const response: Record<string, unknown> = { | ||
| sub: user.id, | ||
| // Always include profile claims | ||
| name: user.displayName, | ||
| preferred_username: user.username, | ||
| picture: `${process.env.OAUTH_ISSUER_URL}/api/avatar/${user.avatarSeed || user.id}`, | ||
| }; | ||
|
|
Copilot
AI
Jan 18, 2026
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.
Profile information is being returned unconditionally without checking if the user has granted the "profile" scope. According to OpenID Connect Core specification, profile claims (name, preferred_username, picture) should only be included when the "profile" scope has been granted. This could lead to unauthorized disclosure of user profile information.
The code should check if grantedScopes includes "profile" (or "read:profile" depending on your scope model) before adding these claims to the response, similar to how the email scope is being checked on line 61.
| // Build response - always include profile, email requires scope from DB | |
| const response: Record<string, unknown> = { | |
| sub: user.id, | |
| // Always include profile claims | |
| name: user.displayName, | |
| preferred_username: user.username, | |
| picture: `${process.env.OAUTH_ISSUER_URL}/api/avatar/${user.avatarSeed || user.id}`, | |
| }; | |
| // Build response - always include subject, other claims require appropriate scopes | |
| const response: Record<string, unknown> = { | |
| sub: user.id, | |
| }; | |
| // Include profile claims only if "profile" scope (or equivalent) was granted | |
| if (grantedScopes.includes("profile") || grantedScopes.includes("read:profile")) { | |
| response.name = user.displayName; | |
| response.preferred_username = user.username; | |
| response.picture = `${process.env.OAUTH_ISSUER_URL}/api/avatar/${user.avatarSeed || user.id}`; | |
| } |
| // Always include profile claims | ||
| name: user.displayName ?? undefined, | ||
| preferred_username: user.username ?? undefined, | ||
| picture: `${process.env.OAUTH_ISSUER_URL}/api/avatar/${user.avatarSeed || user.id}`, |
Copilot
AI
Jan 18, 2026
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.
Profile claims are being included unconditionally in the ID token without checking if the user has granted the "profile" scope. According to OpenID Connect Core specification, profile claims (name, preferred_username, picture) should only be included when the "profile" scope has been granted.
This creates an inconsistency with how email claims are handled (lines 163-164) which correctly check for the "email" scope. The profile claims should be conditionally included based on whether grantedScopes contains "profile" (or the appropriate scope variant based on your scope model).
| // Always include profile claims | |
| name: user.displayName ?? undefined, | |
| preferred_username: user.username ?? undefined, | |
| picture: `${process.env.OAUTH_ISSUER_URL}/api/avatar/${user.avatarSeed || user.id}`, | |
| // Profile claims based on granted scopes from database | |
| name: grantedScopes.includes("profile") ? (user.displayName ?? undefined) : undefined, | |
| preferred_username: grantedScopes.includes("profile") ? (user.username ?? undefined) : undefined, | |
| picture: grantedScopes.includes("profile") | |
| ? `${process.env.OAUTH_ISSUER_URL}/api/avatar/${user.avatarSeed || user.id}` | |
| : undefined, |
| eq(oauthConsents.clientId, payload.client_id) | ||
| ), | ||
| }); | ||
| const grantedScopes: string[] = consent ? JSON.parse(consent.scopes) : []; |
Copilot
AI
Jan 18, 2026
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.
JSON.parse is called without error handling. If the consent.scopes field contains invalid JSON, this will throw an exception that will be caught by the outer try-catch and result in a generic "server_error" response. Consider adding validation or using a safer parsing approach with error handling to provide a more specific error message or handle corruption gracefully.
| const grantedScopes: string[] = consent ? JSON.parse(consent.scopes) : []; | |
| let grantedScopes: string[] = []; | |
| if (consent && consent.scopes) { | |
| try { | |
| const parsed = JSON.parse(consent.scopes); | |
| if (Array.isArray(parsed) && parsed.every((scope) => typeof scope === "string")) { | |
| grantedScopes = parsed; | |
| } else { | |
| console.warn("Invalid scopes format for consent", { | |
| userId: user.id, | |
| clientId: payload.client_id, | |
| }); | |
| } | |
| } catch (parseError) { | |
| console.warn("Failed to parse consent.scopes JSON", { | |
| error: parseError, | |
| userId: user.id, | |
| clientId: payload.client_id, | |
| }); | |
| } | |
| } |
| eq(oauthConsents.clientId, client.id) | ||
| ), | ||
| }); | ||
| const grantedScopes: string[] = consent ? JSON.parse(consent.scopes) : []; |
Copilot
AI
Jan 18, 2026
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.
JSON.parse is called without error handling. If the consent.scopes field contains invalid JSON, this will throw an exception that will be caught by the outer try-catch and result in a generic "server_error" response. Consider adding validation or using a safer parsing approach with error handling to provide a more specific error message or handle corruption gracefully.
| const grantedScopes: string[] = consent ? JSON.parse(consent.scopes) : []; | |
| let grantedScopes: string[] = []; | |
| if (consent) { | |
| try { | |
| const parsed = JSON.parse(consent.scopes); | |
| if (Array.isArray(parsed) && parsed.every((s) => typeof s === "string")) { | |
| grantedScopes = parsed; | |
| } | |
| } catch { | |
| // If scopes are corrupted or invalid JSON, fall back to no granted scopes | |
| grantedScopes = []; | |
| } | |
| } |
No description provided.