Skip to content

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.

Sign up for GitHub

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

Merged
sirily11 merged 1 commit into from
Jan 18, 2026
Merged

fix: uses db scope #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,3 +47,4 @@ node_modules/
/blob-report/
/playwright/.cache/
/playwright/.auth/
.env*.local
25 changes: 17 additions & 8 deletions app/api/oauth/token/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { NextRequest, NextResponse } from "next/server";
import { db } from "@/lib/db";
import { oauthClients, oauthRefreshTokens, users } from "@/lib/db/schema";
import { oauthClients, oauthConsents, oauthRefreshTokens, users } from "@/lib/db/schema";
import { verifyPassword } from "@/lib/auth/password";
import { getOAuthCode, deleteOAuthCode } from "@/lib/redis";
import { verifyCodeChallenge } from "@/lib/oauth/pkce";
Expand Down Expand Up @@ -138,6 +138,15 @@ async function handleAuthorizationCodeGrant(
);
}

// Get granted scopes from database
const consent = await db.query.oauthConsents.findFirst({
where: and(
eq(oauthConsents.userId, user.id),
eq(oauthConsents.clientId, client.id)
),
});
const grantedScopes: string[] = consent ? JSON.parse(consent.scopes) : [];
Copy link

Copilot AI Jan 18, 2026

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.

Suggested change
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 = [];
}
}

Copilot uses AI. Check for mistakes.

// Generate tokens
const scopeString = codeData.scopes.join(" ");

Expand All @@ -150,13 +159,13 @@ async function handleAuthorizationCodeGrant(
const idToken = await signIdToken(
{
sub: user.id,
email: codeData.scopes.includes("email") ? user.email : undefined,
email_verified: codeData.scopes.includes("email") ? user.emailVerified ?? false : undefined,
name: codeData.scopes.includes("profile") ? user.displayName ?? undefined : undefined,
preferred_username: codeData.scopes.includes("profile") ? user.username ?? undefined : undefined,
picture: codeData.scopes.includes("profile")
? `${process.env.OAUTH_ISSUER_URL}/api/avatar/${user.avatarSeed || user.id}`
: undefined,
// Email claims based on granted scopes from database
email: grantedScopes.includes("email") ? user.email : undefined,
email_verified: grantedScopes.includes("email") ? user.emailVerified ?? false : undefined,
// 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}`,
Comment on lines +165 to +168
Copy link

Copilot AI Jan 18, 2026

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).

Suggested change
// 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,

Copilot uses AI. Check for mistakes.
nonce: codeData.nonce,
auth_time: Math.floor(Date.now() / 1000),
},
Expand Down
28 changes: 17 additions & 11 deletions app/api/oauth/userinfo/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { NextRequest, NextResponse } from "next/server";
import { db } from "@/lib/db";
import { users } from "@/lib/db/schema";
import { oauthConsents, users } from "@/lib/db/schema";
import { verifyAccessToken } from "@/lib/oauth/jwt";
import { eq } from "drizzle-orm";
import { and, eq } from "drizzle-orm";

async function handleUserInfo(request: NextRequest) {
try {
Expand Down Expand Up @@ -40,19 +40,25 @@ async function handleUserInfo(request: NextRequest) {
);
}

// Build response based on scopes
const scopes = payload.scope.split(" ");
// Get granted scopes from database
const consent = await db.query.oauthConsents.findFirst({
where: and(
eq(oauthConsents.userId, user.id),
eq(oauthConsents.clientId, payload.client_id)
),
});
const grantedScopes: string[] = consent ? JSON.parse(consent.scopes) : [];
Copy link

Copilot AI Jan 18, 2026

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.

Suggested change
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,
});
}
}

Copilot uses AI. Check for mistakes.

// 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}`,
};

Comment on lines +52 to 60
Copy link

Copilot AI Jan 18, 2026

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.

Suggested change
// 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}`;
}

Copilot uses AI. Check for mistakes.
if (scopes.includes("profile")) {
response.name = user.displayName;
response.preferred_username = user.username;
response.picture = `${process.env.OAUTH_ISSUER_URL}/api/avatar/${user.avatarSeed || user.id}`;
}

if (scopes.includes("email")) {
if (grantedScopes.includes("email")) {
response.email = user.email;
response.email_verified = user.emailVerified;
}
Expand Down
2 changes: 0 additions & 2 deletions lib/scopes.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ describe("SPECIAL_SCOPES", () => {
test("should contain expected special scopes", () => {
const keys = SPECIAL_SCOPES.map((s) => s.key);
expect(keys).toContain("openid");
expect(keys).toContain("offline_access");
});
Comment on lines 53 to 56
Copy link

Copilot AI Jan 18, 2026

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:

  1. The scope system doesn't define offline_access
  2. Tests don't verify its existence
  3. 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.

Copilot uses AI. Check for mistakes.
});

Expand All @@ -65,7 +64,6 @@ describe("SCOPES (generated)", () => {
test("should contain special scopes", () => {
const keys = SCOPES.map((s) => s.key);
expect(keys).toContain("openid");
expect(keys).toContain("offline_access");
});

test("should contain resource scopes with operation:resource format", () => {
Expand Down
Loading