feat(auth): migrate from mcp() to oauthProvider() for OAuth 2.1#1324
Conversation
📝 WalkthroughWalkthroughAdds Better Auth oauth-provider integration: new well-known OAuth and OpenID routes, protected-resource handlers, JWT-based token handling and verification, replaces MCP-based auth with jwt/oauthProvider plugins, updates DB schema for OAuth2 (clients/tokens/consents/JWKS), and refactors client-side consent flow to use authClient. Changes
Sequence DiagramsequenceDiagram
participant Client as OAuth Client
participant API as API Server
participant Auth as Auth Server
participant DB as Database
participant JWKS as JWKS Endpoint
Client->>API: GET /.well-known/oauth-authorization-server
API->>Auth: oauthProviderAuthServerMetadata(auth)
Auth-->>API: Authorization server metadata
API-->>Client: Discovery document
Client->>API: POST /token (client grant)
API->>Auth: token request / verify client
Auth->>DB: lookup oauth_clients / sessions
DB-->>Auth: client/session data
Auth->>JWKS: sign token (RS256)
JWKS-->>Auth: key material
Auth-->>API: JWT access_token
API-->>Client: { access_token, refresh_token }
Client->>API: GET /.well-known/openid-configuration
API->>Auth: oauthProviderOpenIdConfigMetadata(auth)
Auth-->>API: OpenID configuration
API-->>Client: OpenID config
Client->>API: GET protected resource (Bearer JWT)
API->>JWKS: fetch public key
JWKS-->>API: public key
API->>API: verifyAccessToken(jwt)
API->>DB: resolve user/organization from claims
DB-->>API: user/org info
API-->>Client: protected resource / MCP context
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@apps/api/src/app/api/agent/`[transport]/route.ts:
- Around line 66-69: The catch block that currently swallows errors (the one
logging "[mcp/auth] Access token verification failed") should capture the thrown
error (e.g., change to catch (err)) and include the error details in the log;
update the handler in route.ts where token verification occurs to call
console.error("[mcp/auth] Access token verification failed:", err) or equivalent
structured logging so the actual error (expired token, invalid signature,
JWKS/network error) is recorded before returning undefined.
In `@apps/web/src/app/oauth/consent/components/ConsentForm/ConsentForm.tsx`:
- Around line 59-67: The form currently blocks consent when selectedOrgId is
empty even if organizations.length === 0; update the component to explicitly
handle the zero-organizations case by (1) adding a top-level guard that detects
organizations.length === 0 and either shows a clear error/CTA (e.g., "You are
not part of any organization — create or join an organization") or
redirects/calls an onNoOrg handler, and (2) change handleConsent to only require
selectedOrgId when organizations.length > 0 (i.e., replace the unconditional
check of selectedOrgId with a conditional that skips the validation if
organizations is empty); reference the selectedOrgId state, handleConsent
function, and the organizations array when making these changes.
- Around line 85-96: The success path after calling authClient.oauth2.consent
currently only redirects when data?.uri exists, leaving the UI stuck in loading
if uri is falsy; update the consent handling in ConsentForm (the
authClient.oauth2.consent call) to ensure setIsLoading(false) is called when
consent succeeds but data?.uri is undefined (and optionally surface an error or
throw to trigger existing error handling). Specifically, after checking
consentError and before/after the if (data?.uri) block, call setIsLoading(false)
(and if you prefer, set an error or throw new Error("No redirect URI returned")
to reuse the catch path) so the "Authorizing..." state is cleared.
In `@apps/web/src/app/oauth/consent/page.tsx`:
- Around line 58-61: The myOrganizations query currently returns full
organization records; update the code that constructs its result (the mapping
over memberships returned by trpc.user.myOrganizations.query in page.tsx) to
return only { id: organization.id, name: organization.name, slug:
organization.slug, logo: organization.logo } so the consumer receives the
minimal shape expected by ConsentForm, and then edit the ConsentForm interface
to remove unused properties (delete slug and logo from the interface so it
matches what the component actually renders).
- Line 11: Change the searchParams type to match Next.js (Promise<Record<string,
string | string[] | undefined>>) and guard/coerce the extracted query values
before using them: in the handler/component where you read params.client_id and
params.scope (and any other param), check typeof === "string" and assign to
local clientId/scope variables (or undefined otherwise) so scope?.split(" ") and
similar operations never run on arrays or undefined.
In `@packages/auth/src/server.ts`:
- Around line 146-162: The consentReferenceId callback currently throws a plain
Error when no activeOrganizationId is present (in postLogin), which can surface
as an opaque 500 to OAuth clients; change this to throw or return an OAuth-aware
error that includes an OAuth error code and description (e.g., access_denied
with a message) using the OAuth provider's error class or a structure the
provider recognizes so the client receives a proper OAuth error response; update
consentReferenceId (and related postLogin handling) to use that OAuth error
type/structure and preserve the existing type cast on session.
In `@packages/db/drizzle/0017_switch_to_oauth2.sql`:
- Around line 1-4: Remove the human-added comment at the top of the
auto-generated migration (the line beginning with "-- Drop old MCP OAuth
tables...") in 0017_switch_to_oauth2.sql so the migration matches Drizzle's
autogenerated output; either regenerate the migration with drizzle-kit generate
to produce a clean file or simply delete that comment line from the SQL file
(keep the DROP TABLE statements intact) and ensure 0017_snapshot.json remains
consistent.
In `@packages/db/src/schema/auth.ts`:
- Around line 190-207: The oauth_refresh_tokens table is missing a uniqueness
constraint on the token column; update the oauthRefreshTokens table definition
to make token unique (matching oauthAccessTokens) by adding .unique() to the
token column declaration in the oauthRefreshTokens schema so that
oauthRefreshTokens.token is declared as text("token").unique(). This change
should be applied within the oauthRefreshTokens table definition to enforce
uniqueness and create an index for efficient lookups.
🧹 Nitpick comments (8)
apps/web/src/app/oauth/consent/page.tsx (1)
19-23: Double-await ofsearchParamsacross branches is fine but could be simplified.
searchParamsis awaited on line 20 inside the!sessionbranch, and again on line 25 outside it. Since the early branch redirects (never falls through), there's no actual double-evaluation issue. However, hoisting the await above the session check would reduce duplication and make the flow clearer.Optional refactor
export default async function ConsentPage({ searchParams }: ConsentPageProps) { + const params = await searchParams; const session = await auth.api.getSession({ headers: await headers(), }); if (!session) { - const params = await searchParams; const returnUrl = `/oauth/consent?${new URLSearchParams(params).toString()}`; redirect(`/sign-in?redirect=${encodeURIComponent(returnUrl)}`); } - const params = await searchParams; const { client_id, scope } = params;apps/web/src/app/oauth/consent/components/ConsentForm/ConsentForm.tsx (2)
73-83:setActiveside effect persists even if consent subsequently fails.If
authClient.organization.setActivesucceeds (line 75) butauthClient.oauth2.consentfails (line 85), the user's active organization has been changed without completing the OAuth flow. On retry, this is likely harmless (setActive would be called again with the same value), but it's worth noting as a subtle side effect.Consider wrapping both calls so that if consent fails, you either document this as expected behavior or attempt to restore the previous active org.
204-216: Consider extracting the magic number20to a named constant.Line 212 uses a raw
20as a threshold for deciding whether to show a client ID verbatim or replace it with "External Application." A named constant would improve readability.Suggested refactor
+const MAX_DISPLAY_CLIENT_ID_LENGTH = 20; + function getClientDisplayName(clientId: string): string { const knownClients: Record<string, string> = { "claude-code": "Claude Code", "superset-desktop": "Superset Desktop", }; if (knownClients[clientId]) { return knownClients[clientId]; } - if (clientId.length > 20) { + if (clientId.length > MAX_DISPLAY_CLIENT_ID_LENGTH) { return "External Application"; } return clientId; }As per coding guidelines, "Extract hardcoded magic numbers, strings, and enums to named constants at module top instead of leaving them inline in logic."
apps/api/src/app/.well-known/oauth-protected-resource/route.ts (1)
1-7: Consider extracting the sharedprotectedResourceHandlerconfig to reduce duplication.This file and
apps/api/src/app/.well-known/oauth-protected-resource/[...path]/route.tsare identical. While trivial, you could extract the handler to a shared file and re-export from both routes.That said, this is a very common Next.js App Router pattern for catch-all routes and the duplication is only ~4 lines, so this is purely a nice-to-have.
apps/api/src/app/api/agent/[transport]/route.ts (2)
42-63: Consider validating JWT payload shape instead of rawascasts.The payload from
verifyAccessTokenis treated as trusted with multipleas string/as string[]assertions (lines 50, 57, 62). Since this is external data (JWT claims), a malformed or unexpected token shape could silently pass incorrect types through. A lightweight Zod schema or manualtypeofguards would align with the guideline to validate external API data as untrusted.
19-29: Hardcoded sentinel strings could be extracted to named constants.
"session","mcp:full", and"mcp-client"appear as inline magic strings in both auth paths. Per coding guidelines, consider extracting these to module-level constants for consistency and grep-ability.Also applies to: 55-65
packages/db/drizzle/0017_switch_to_oauth2.sql (2)
5-11: Private key material stored as plaintext in the database.The
jwksstable storesprivate_keyas plaintext. While this is dictated by better-auth's oauth-provider schema, ensure the database enforces encryption at rest and that DB access is tightly scoped. If your threat model requires it, consider application-level encryption of the private key column or using a KMS-backed approach.
45-70: Consider adding indexes for token lookup and user-scoped queries.
oauth_refresh_tokens.tokenlacks an index — refresh token exchanges require lookup by token value.- Neither
oauth_refresh_tokens.user_idnoroauth_access_tokens.user_idare indexed — revocation queries (e.g., "revoke all tokens for user X") would require sequential scans.If the library queries by these columns (likely), adding indexes in a follow-up migration would improve performance as the token tables grow.
| const { data, error: consentError } = await authClient.oauth2.consent({ | ||
| accept, | ||
| scope: accept ? scopes.join(" ") : undefined, | ||
| }); | ||
|
|
||
| const data = await response.json(); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error( | ||
| data.error_description || data.message || "Failed to process consent", | ||
| ); | ||
| if (consentError) { | ||
| throw new Error(consentError.message ?? "Failed to process consent"); | ||
| } | ||
|
|
||
| const redirectUrl = data.redirectURI || data.redirectTo; | ||
| if (redirectUrl) { | ||
| window.location.href = redirectUrl; | ||
| if (data?.uri) { | ||
| window.location.href = data.uri; | ||
| } |
There was a problem hiding this comment.
Missing setIsLoading(false) on the success path when data?.uri is falsy.
If authClient.oauth2.consent succeeds but data?.uri is undefined or empty, neither the redirect (line 95) nor the catch block executes. The button stays in the "Authorizing..." state permanently with no error shown.
Suggested fix
if (data?.uri) {
window.location.href = data.uri;
+ } else {
+ setError("No redirect URI received from server");
+ setIsLoading(false);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { data, error: consentError } = await authClient.oauth2.consent({ | |
| accept, | |
| scope: accept ? scopes.join(" ") : undefined, | |
| }); | |
| const data = await response.json(); | |
| if (!response.ok) { | |
| throw new Error( | |
| data.error_description || data.message || "Failed to process consent", | |
| ); | |
| if (consentError) { | |
| throw new Error(consentError.message ?? "Failed to process consent"); | |
| } | |
| const redirectUrl = data.redirectURI || data.redirectTo; | |
| if (redirectUrl) { | |
| window.location.href = redirectUrl; | |
| if (data?.uri) { | |
| window.location.href = data.uri; | |
| } | |
| const { data, error: consentError } = await authClient.oauth2.consent({ | |
| accept, | |
| scope: accept ? scopes.join(" ") : undefined, | |
| }); | |
| if (consentError) { | |
| throw new Error(consentError.message ?? "Failed to process consent"); | |
| } | |
| if (data?.uri) { | |
| window.location.href = data.uri; | |
| } else { | |
| setError("No redirect URI received from server"); | |
| setIsLoading(false); | |
| } |
🤖 Prompt for AI Agents
In `@apps/web/src/app/oauth/consent/components/ConsentForm/ConsentForm.tsx` around
lines 85 - 96, The success path after calling authClient.oauth2.consent
currently only redirects when data?.uri exists, leaving the UI stuck in loading
if uri is falsy; update the consent handling in ConsentForm (the
authClient.oauth2.consent call) to ensure setIsLoading(false) is called when
consent succeeds but data?.uri is undefined (and optionally surface an error or
throw to trigger existing error handling). Specifically, after checking
consentError and before/after the if (data?.uri) block, call setIsLoading(false)
(and if you prefer, set an error or throw new Error("No redirect URI returned")
to reuse the catch path) so the "Authorizing..." state is cleared.
| client_id?: string; | ||
| scope?: string; | ||
| }>; | ||
| searchParams: Promise<Record<string, string>>; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/web/src/app/oauth/consent/page.tsx | head -100Repository: superset-sh/superset
Length of output: 2984
🌐 Web query:
Next.js 15 App Router page searchParams type definition
💡 Result:
In Next.js 15 (App Router), the page.tsx props are now Promises:
export default async function Page({
params,
searchParams,
}: {
params: Promise<{ slug: string }>
searchParams: Promise<{ [key: string]: string | string[] | undefined }>
}) {
const { slug } = await params
const { q } = await searchParams
}
``` ([nextjs.org](https://nextjs.org/docs/app/api-reference/file-conventions/page))
If you want a **generated, route-aware type** (strict `params` keys, typed from the route literal), use the **global** helper:
```ts
export default async function Page(props: PageProps<'/blog/[slug]'>) {
const { slug } = await props.params
const query = await props.searchParams
}
``` ([nextjs.org](https://nextjs.org/docs/app/api-reference/file-conventions/page))
Docs note: in Next.js 15, `params` (Page/Layout) and `searchParams` (Page) are Promises. ([nextjs.org](https://nextjs.org/docs/messages/next-prerender-sync-params))
Citations:
- 1: https://nextjs.org/docs/app/api-reference/file-conventions/page
- 2: https://nextjs.org/docs/app/api-reference/file-conventions/page
- 3: https://nextjs.org/docs/messages/next-prerender-sync-params
---
</details>
**`searchParams` type is narrower than Next.js 15 actually provides.**
Next.js 15 App Router pages receive `searchParams` typed as `Promise<Record<string, string | string[] | undefined>>`. Using `Promise<Record<string, string>>` silently discards the possibility of array values (e.g., `?scope=a&scope=b`) and `undefined`. This can lead to runtime errors if a query param is repeated or absent — `params.client_id` is typed as `string` but could be `string[]` or `undefined`. Line 58's `scope?.split(" ")` would throw if `scope` were an array instead of a string.
<details>
<summary>Suggested fix</summary>
```diff
interface ConsentPageProps {
- searchParams: Promise<Record<string, string>>;
+ searchParams: Promise<Record<string, string | string[] | undefined>>;
}Then when extracting values, coerce to string:
const clientId = typeof params.client_id === "string" ? params.client_id : undefined;
const scope = typeof params.scope === "string" ? params.scope : undefined;🤖 Prompt for AI Agents
In `@apps/web/src/app/oauth/consent/page.tsx` at line 11, Change the searchParams
type to match Next.js (Promise<Record<string, string | string[] | undefined>>)
and guard/coerce the extracted query values before using them: in the
handler/component where you read params.client_id and params.scope (and any
other param), check typeof === "string" and assign to local clientId/scope
variables (or undefined otherwise) so scope?.split(" ") and similar operations
never run on arrays or undefined.
| const scopes = scope?.split(" ").filter(Boolean) ?? ["openid"]; | ||
|
|
||
| const trpc = await api(); | ||
| const userOrganizations = await trpc.user.myOrganizations.query(); | ||
|
|
||
| const extendedSession = session.session as typeof session.session & { | ||
| activeOrganizationId?: string | null; | ||
| }; | ||
| const defaultOrgId = | ||
| extendedSession.activeOrganizationId ?? userOrganizations[0]?.id; | ||
| const organizations = await trpc.user.myOrganizations.query(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "myOrganizations" --type=ts -C 5Repository: superset-sh/superset
Length of output: 2622
🏁 Script executed:
cat -n packages/trpc/src/router/user/user.ts | head -50Repository: superset-sh/superset
Length of output: 1926
🏁 Script executed:
fd -e tsx ConsentFormRepository: superset-sh/superset
Length of output: 133
🏁 Script executed:
fd -e ts organizations.ts packages/dbRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
cat -n apps/web/src/app/oauth/consent/components/ConsentForm/ConsentForm.tsxRepository: superset-sh/superset
Length of output: 7002
🏁 Script executed:
fd -e ts -e sql schema packages/db/srcRepository: superset-sh/superset
Length of output: 96
🏁 Script executed:
rg -n "organizations" packages/db/src/schema -A 20Repository: superset-sh/superset
Length of output: 29821
🏁 Script executed:
sed -n '32,45p' packages/trpc/src/router/user/user.tsRepository: superset-sh/superset
Length of output: 466
🏁 Script executed:
rg -n "myOrganizations" packages/trpc/src/router --type=ts -A 15 -B 2Repository: superset-sh/superset
Length of output: 1284
Extract only required fields in myOrganizations query to match ConsentForm's Organization interface.
The procedure returns full organization records with extra fields (createdAt, metadata, stripeCustomerId), but ConsentForm expects { id: string; name: string; slug: string; logo: string | null }. Additionally, slug and logo are defined in the interface but never used in the component's render logic. Transform the return value to include only the required fields:
return memberships.map((m) => ({
id: m.organization.id,
name: m.organization.name,
slug: m.organization.slug,
logo: m.organization.logo,
}));Then update ConsentForm's interface to match what's actually rendered (remove unused slug and logo).
🤖 Prompt for AI Agents
In `@apps/web/src/app/oauth/consent/page.tsx` around lines 58 - 61, The
myOrganizations query currently returns full organization records; update the
code that constructs its result (the mapping over memberships returned by
trpc.user.myOrganizations.query in page.tsx) to return only { id:
organization.id, name: organization.name, slug: organization.slug, logo:
organization.logo } so the consumer receives the minimal shape expected by
ConsentForm, and then edit the ConsentForm interface to remove unused properties
(delete slug and logo from the interface so it matches what the component
actually renders).
| postLogin: { | ||
| // Org selection is handled in the consent page, so never redirect to a separate page | ||
| page: `${env.NEXT_PUBLIC_WEB_URL}/oauth/consent`, | ||
| shouldRedirect: () => false, | ||
| consentReferenceId: ({ session }) => { | ||
| const activeOrganizationId = ( | ||
| session as { activeOrganizationId?: string } | ||
| ).activeOrganizationId; | ||
| if (!activeOrganizationId) { | ||
| throw new Error("Organization must be selected before consent"); | ||
| } | ||
| return activeOrganizationId; | ||
| }, | ||
| }, | ||
| customAccessTokenClaims: ({ referenceId }) => ({ | ||
| organizationId: referenceId ?? undefined, | ||
| }), |
There was a problem hiding this comment.
Throwing a plain Error in consentReferenceId may produce an opaque 500 for OAuth clients.
Line 155: if no activeOrganizationId is set, this throws a generic Error. Depending on how @better-auth/oauth-provider handles exceptions in postLogin callbacks, the OAuth client may receive an unhelpful 500 Internal Server Error instead of a meaningful OAuth error response (e.g., access_denied with a description).
Consider whether this scenario can actually occur in the normal flow — if the consent page always sets the active org before proceeding, this is just a safety net and the plain Error is acceptable.
Also, the type cast on Line 151-153 is a reasonable workaround given the plugin's callback typing.
🤖 Prompt for AI Agents
In `@packages/auth/src/server.ts` around lines 146 - 162, The consentReferenceId
callback currently throws a plain Error when no activeOrganizationId is present
(in postLogin), which can surface as an opaque 500 to OAuth clients; change this
to throw or return an OAuth-aware error that includes an OAuth error code and
description (e.g., access_denied with a message) using the OAuth provider's
error class or a structure the provider recognizes so the client receives a
proper OAuth error response; update consentReferenceId (and related postLogin
handling) to use that OAuth error type/structure and preserve the existing type
cast on session.
| -- Drop old MCP OAuth tables (replaced by @better-auth/oauth-provider) | ||
| DROP TABLE IF EXISTS "auth"."oauth_access_tokens" CASCADE;--> statement-breakpoint | ||
| DROP TABLE IF EXISTS "auth"."oauth_consents" CASCADE;--> statement-breakpoint | ||
| DROP TABLE IF EXISTS "auth"."oauth_applications" CASCADE;--> statement-breakpoint |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check journal for this migration entry
echo "=== Journal entries ==="
cat packages/db/drizzle/meta/_journal.json 2>/dev/null | head -100
echo ""
echo "=== Files in meta directory ==="
ls -la packages/db/drizzle/meta/
echo ""
echo "=== Full migration file content ==="
cat packages/db/drizzle/0017_switch_to_oauth2.sqlRepository: superset-sh/superset
Length of output: 8424
Remove the manual comment from this auto-generated migration file.
This migration file is registered in Drizzle's metadata system (0017_snapshot.json exists), confirming it was auto-generated by drizzle-kit generate. The human-authored comment on line 1 (-- Drop old MCP OAuth tables (replaced by @better-auth/oauth-provider)) violates the project guideline that prohibits manually editing files in packages/db/drizzle/.
Drizzle Kit does not add explanatory comments—such comments indicate post-generation modification. Either regenerate the migration to remove the comment or delete it manually before merging.
🤖 Prompt for AI Agents
In `@packages/db/drizzle/0017_switch_to_oauth2.sql` around lines 1 - 4, Remove the
human-added comment at the top of the auto-generated migration (the line
beginning with "-- Drop old MCP OAuth tables...") in 0017_switch_to_oauth2.sql
so the migration matches Drizzle's autogenerated output; either regenerate the
migration with drizzle-kit generate to produce a clean file or simply delete
that comment line from the SQL file (keep the DROP TABLE statements intact) and
ensure 0017_snapshot.json remains consistent.
| export const oauthRefreshTokens = authSchema.table("oauth_refresh_tokens", { | ||
| id: uuid("id").primaryKey().defaultRandom(), | ||
| token: text("token").notNull(), | ||
| clientId: text("client_id") | ||
| .notNull() | ||
| .references(() => oauthClients.clientId, { onDelete: "cascade" }), | ||
| sessionId: uuid("session_id").references(() => sessions.id, { | ||
| onDelete: "set null", | ||
| }), | ||
| userId: uuid("user_id") | ||
| .notNull() | ||
| .references(() => users.id, { onDelete: "cascade" }), | ||
| referenceId: text("reference_id"), | ||
| expiresAt: timestamp("expires_at"), | ||
| createdAt: timestamp("created_at"), | ||
| revoked: timestamp("revoked"), | ||
| scopes: text("scopes").array().notNull(), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for oauthRefreshTokens usage
rg "oauthRefreshTokens" -t ts -t tsx --no-heading -B2 -A2 2>/dev/null | head -100Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Look for refresh token queries or operations
rg "refresh.*token" -i -t ts -t tsx -B2 -A2 2>/dev/null | grep -v node_modules | head -120Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check if there are any existing indexes or constraints in the auth schema
rg "\.unique\(\)|\.index\(" packages/db/src/schema/auth.ts -B1 -A1Repository: superset-sh/superset
Length of output: 703
🏁 Script executed:
# Search for better-auth references to understand how it handles tokens
rg "better-auth|@better-auth" -t ts -t tsx --no-heading -B1 -A1 2>/dev/null | head -80Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Read the oauthRefreshTokens definition
cat -n packages/db/src/schema/auth.ts | sed -n '190,210p'Repository: superset-sh/superset
Length of output: 974
🏁 Script executed:
# Search for any query patterns on oauthRefreshTokens
rg "refresh" -i -t ts -t tsx --no-heading 2>/dev/null | grep -i token | head -50Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Look for .where conditions that filter on token fields
rg "where.*token|token.*where" -i -t ts -t tsx -B2 -A2 2>/dev/null | head -80Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check if better-auth is a dependency
fd -e "package.json" -x grep -l "better-auth" {} \;Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Read oauthAccessTokens to see if it has unique constraint
cat -n packages/db/src/schema/auth.ts | sed -n '209,225p'Repository: superset-sh/superset
Length of output: 870
🏁 Script executed:
# Search for any database queries or operations files
fd -e "db" -type d | head -10Repository: superset-sh/superset
Length of output: 234
🏁 Script executed:
# Look for server-side procedures or handlers that might use refresh tokens
rg "refresh" -t ts -t tsx -i 2>/dev/null | head -100Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check for migrations related to oauth refresh tokens
fd -name "*.sql" -o -name "*.ts" | xargs grep -l "oauth_refresh_tokens" 2>/dev/null | head -20Repository: superset-sh/superset
Length of output: 294
🏁 Script executed:
# Search for actual token lookups - look for queries by token value
rg "\.where\(|\.select\(|query" -t ts -B1 -A3 2>/dev/null | grep -i "token" -B2 -A2 | head -100Repository: superset-sh/superset
Length of output: 2486
🏁 Script executed:
# Check if there are any drizzle query files in the db package
find packages/db -name "*.ts" -type f | head -20Repository: superset-sh/superset
Length of output: 489
🏁 Script executed:
# Look at the full auth schema to understand all token patterns
cat packages/db/src/schema/auth.ts | grep -A5 "token"Repository: superset-sh/superset
Length of output: 1653
Add unique constraint to oauthRefreshTokens.token for data integrity and consistency.
The similar table oauthAccessTokens defines its token field as text("token").unique(), but oauthRefreshTokens omits the unique constraint. Since refresh tokens are validated and revoked by their token value, duplicate tokens could theoretically exist and cause integrity issues. Additionally, lookups without a unique index will require sequential scans.
Fix
- token: text("token").notNull(),
+ token: text("token").notNull().unique(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const oauthRefreshTokens = authSchema.table("oauth_refresh_tokens", { | |
| id: uuid("id").primaryKey().defaultRandom(), | |
| token: text("token").notNull(), | |
| clientId: text("client_id") | |
| .notNull() | |
| .references(() => oauthClients.clientId, { onDelete: "cascade" }), | |
| sessionId: uuid("session_id").references(() => sessions.id, { | |
| onDelete: "set null", | |
| }), | |
| userId: uuid("user_id") | |
| .notNull() | |
| .references(() => users.id, { onDelete: "cascade" }), | |
| referenceId: text("reference_id"), | |
| expiresAt: timestamp("expires_at"), | |
| createdAt: timestamp("created_at"), | |
| revoked: timestamp("revoked"), | |
| scopes: text("scopes").array().notNull(), | |
| }); | |
| export const oauthRefreshTokens = authSchema.table("oauth_refresh_tokens", { | |
| id: uuid("id").primaryKey().defaultRandom(), | |
| token: text("token").notNull().unique(), | |
| clientId: text("client_id") | |
| .notNull() | |
| .references(() => oauthClients.clientId, { onDelete: "cascade" }), | |
| sessionId: uuid("session_id").references(() => sessions.id, { | |
| onDelete: "set null", | |
| }), | |
| userId: uuid("user_id") | |
| .notNull() | |
| .references(() => users.id, { onDelete: "cascade" }), | |
| referenceId: text("reference_id"), | |
| expiresAt: timestamp("expires_at"), | |
| createdAt: timestamp("created_at"), | |
| revoked: timestamp("revoked"), | |
| scopes: text("scopes").array().notNull(), | |
| }); |
🤖 Prompt for AI Agents
In `@packages/db/src/schema/auth.ts` around lines 190 - 207, The
oauth_refresh_tokens table is missing a uniqueness constraint on the token
column; update the oauthRefreshTokens table definition to make token unique
(matching oauthAccessTokens) by adding .unique() to the token column declaration
in the oauthRefreshTokens schema so that oauthRefreshTokens.token is declared as
text("token").unique(). This change should be applied within the
oauthRefreshTokens table definition to enforce uniqueness and create an index
for efficient lookups.
saddlepaddle
force-pushed
the
opencode-support
branch
from
2702331 to
9949d3b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/web/src/app/oauth/consent/components/ConsentForm/ConsentForm.tsx`:
- Around line 85-94: The current flow calls authClient.organization.setActive
with selectedOrgId before performing the consent request, which can persist an
unwanted org switch if the subsequent consent call fails; change the flow so
setActive is executed only after the consent call succeeds, or implement a
rollback: capture the previous active org id before calling setActive and, if
consent (the subsequent consent method) throws, revert by calling
authClient.organization.setActive with the saved previous org id and
log/propagate the original error. Ensure you reference
authClient.organization.setActive, selectedOrgId and the consent call when
updating the logic.
In `@packages/db/src/schema/auth.ts`:
- Around line 156-235: Add missing secondary indexes to improve lookup
performance: modify the table definitions for oauthRefreshTokens,
oauthAccessTokens, and oauthConsents to create indexes on the referenced
columns. Specifically, add indexes on oauthRefreshTokens.userId,
oauthRefreshTokens.clientId, and oauthRefreshTokens.sessionId; on
oauthAccessTokens.userId, oauthAccessTokens.clientId,
oauthAccessTokens.sessionId, and oauthAccessTokens.refreshId; and add a
composite index on oauthConsents for (clientId, userId). Update the respective
authSchema table declarations (oauthRefreshTokens, oauthAccessTokens,
oauthConsents) to include index definitions for those column names so queries
use the new indexes.
- Around line 276-282: The DB schema declares the table as jwkss via
authSchema.table("jwkss", ...) which conflicts with better-auth's expected jwks
table name; rename the table identifier and definition from "jwkss" to "jwks"
(update the authSchema.table call and any references to the jwkss constant) and
then create a migration to rename the table using drizzle-kit generate
--name="rename_jwkss_to_jwks" so runtime and migrations stay in sync.
🧹 Nitpick comments (6)
apps/api/src/app/api/agent/[transport]/route.ts (2)
42-64: Validate the token payload as untrusted external data instead of using bare type assertions.The JWT payload is external input, but its fields are accessed with
as string/as string[]casts that bypass runtime checks. If the token shape ever deviates (e.g.,organizationIdis a number,scopearray contains non-strings,azpis an object), the code silently propagates invalid data into the MCP context.A lightweight Zod schema (or equivalent guard) at this boundary would catch malformed tokens early and produce clear error messages:
Suggested approach
+import { z } from "zod"; + +const accessTokenPayloadSchema = z.object({ + sub: z.string(), + organizationId: z.string(), + scope: z.union([z.string(), z.array(z.string())]).optional(), + azp: z.string().optional(), +}); + // inside the try block: - if (!payload?.sub || !payload.organizationId) { - console.error( - "[mcp/auth] Access token missing sub or organizationId claim", - ); - return undefined; - } - - const scopes = Array.isArray(payload.scope) - ? (payload.scope as string[]) - : typeof payload.scope === "string" - ? payload.scope.split(" ") - : []; - - return { - token: bearerToken, - clientId: (payload.azp as string) ?? "mcp-client", - scopes, + const parsed = accessTokenPayloadSchema.safeParse(payload); + if (!parsed.success) { + console.error("[mcp/auth] Invalid access token payload", parsed.error.flatten()); + return undefined; + } + + const { sub, organizationId, scope, azp } = parsed.data; + const scopes = Array.isArray(scope) ? scope : scope ? scope.split(" ") : []; + + return { + token: bearerToken, + clientId: azp ?? "mcp-client", + scopes, extra: { mcpContext: { - userId: payload.sub, - organizationId: payload.organizationId as string, + userId: sub, + organizationId, } satisfies McpContext, }, };As per coding guidelines, "Validate external API data as untrusted by handling missing fields, unknown enums, and unexpected shapes with tolerant parsing and explicit fallbacks" and "Use Zod schemas for validating tRPC inputs and API route bodies at boundaries."
36-40: Consider extracting the JWKS URL and verification constants.The JWKS path (
/api/auth/jwks), default client ID ("mcp-client"on line 57), and the audience array are inline magic strings. Per guidelines, extract these to named constants at module top for easier maintenance and grep-ability.apps/web/src/app/oauth/consent/components/ConsentForm/ConsentForm.tsx (2)
199-214:handleConsentcalled with raw booleans — consider named actions for clarity.Lines 204 and 211 pass
true/falsetohandleConsent. Per coding guidelines on boolean blindness, a small refactor improves readability:Optional refactor
- onClick={() => handleConsent(false)} + onClick={() => handleConsent({ accept: false })} ... - onClick={() => handleConsent(true)} + onClick={() => handleConsent({ accept: true })}with the corresponding signature change:
-const handleConsent = async (accept: boolean) => { +const handleConsent = async ({ accept }: { accept: boolean }) => {As per coding guidelines, "Avoid boolean blindness by using named properties in options objects instead of passing raw boolean values like
doThing(true, false, true)".
225-237: Extract magic number20to a named constant.Line 233 uses a bare
20to decide whether to show the rawclientIdor "External Application". A named constant clarifies intent.Suggested change
+const MAX_DISPLAY_CLIENT_ID_LENGTH = 20; + function getClientDisplayName(clientId: string): string { const knownClients: Record<string, string> = { "claude-code": "Claude Code", "superset-desktop": "Superset Desktop", }; if (knownClients[clientId]) { return knownClients[clientId]; } - if (clientId.length > 20) { + if (clientId.length > MAX_DISPLAY_CLIENT_ID_LENGTH) { return "External Application"; } return clientId; }As per coding guidelines, "Extract hardcoded magic numbers, strings, and enums to named constants at module top instead of leaving them inline in logic".
packages/auth/src/server.ts (1)
160-162:customAccessTokenClaimswill silently omitorganizationIdwhenreferenceIdis nullish.If
referenceIdisnullorundefined(e.g., consent was granted without an org selection somehow), the resulting JWT will have noorganizationIdclaim, and downstream MCP tool calls that depend on it will fail with an opaque error.This is likely guarded by the
consentReferenceIdthrow on Line 155, but the two callbacks are loosely coupled — if the provider ever skipsconsentReferenceIdfor certain grant types (e.g., client_credentials), tokens would silently lack the claim.Consider adding a defensive log or explicit check here:
Suggested tweak
- customAccessTokenClaims: ({ referenceId }) => ({ - organizationId: referenceId ?? undefined, - }), + customAccessTokenClaims: ({ referenceId }) => { + if (!referenceId) { + console.warn("[auth/oauth] Access token issued without organizationId (no referenceId)"); + } + return { + organizationId: referenceId ?? undefined, + }; + },packages/db/drizzle/0017_switch_to_oauth2.sql (1)
45-56: Consider adding indexes on token lookup columns for query performance.Neither
oauth_refresh_tokens.tokennoroauth_access_tokens.client_id/user_idhave indexes. Token validation on every API request will query these tables bytokenvalue — without an index, this becomes a sequential scan as the table grows.Since this is an auto-generated file, any index additions should be made in the Drizzle schema definitions (not here), then regenerated.
Also notable:
oauth_access_tokens.token(Line 60) is nullable but has a unique constraint — this means multiple rows withNULLtokens are allowed by Postgres. Confirm this is the intended behavior from@better-auth/oauth-provider.#!/bin/bash # Check if any indexes are defined in the Drizzle schema for these tables rg -n 'oauth_access_tokens|oauth_refresh_tokens' --type=ts -g '!drizzle/**' -g '!node_modules/**' -C 3 | head -60Also applies to: 58-70
| if (accept) { | ||
| const { error: setActiveError } = | ||
| await authClient.organization.setActive({ | ||
| organizationId: selectedOrgId, | ||
| }); | ||
| if (setActiveError) { | ||
| throw new Error( | ||
| setActiveError.message ?? "Failed to set organization", | ||
| ); | ||
| } |
There was a problem hiding this comment.
authClient.organization.setActive is called before consent — verify this doesn't persist the org switch if the user later abandons or the consent call fails.
If setActive succeeds but the subsequent consent call throws, the user's active organization has already been changed server-side. On retry or navigating away, they may find themselves in a different org than expected. Consider calling setActive only after consent succeeds, or rolling back on failure.
🤖 Prompt for AI Agents
In `@apps/web/src/app/oauth/consent/components/ConsentForm/ConsentForm.tsx` around
lines 85 - 94, The current flow calls authClient.organization.setActive with
selectedOrgId before performing the consent request, which can persist an
unwanted org switch if the subsequent consent call fails; change the flow so
setActive is executed only after the consent call succeeds, or implement a
rollback: capture the previous active org id before calling setActive and, if
consent (the subsequent consent method) throws, revert by calling
authClient.organization.setActive with the saved previous org id and
log/propagate the original error. Ensure you reference
authClient.organization.setActive, selectedOrgId and the consent call when
updating the logic.
| export const oauthClients = authSchema.table("oauth_clients", { | ||
| id: uuid("id").primaryKey().defaultRandom(), | ||
| clientId: text("client_id").notNull().unique(), | ||
| clientSecret: text("client_secret"), | ||
| disabled: boolean("disabled").default(false), | ||
| skipConsent: boolean("skip_consent"), | ||
| enableEndSession: boolean("enable_end_session"), | ||
| scopes: text("scopes").array(), | ||
| userId: uuid("user_id").references(() => users.id, { onDelete: "cascade" }), | ||
| createdAt: timestamp("created_at"), | ||
| updatedAt: timestamp("updated_at"), | ||
| name: text("name"), | ||
| uri: text("uri"), | ||
| icon: text("icon"), | ||
| contacts: text("contacts").array(), | ||
| tos: text("tos"), | ||
| policy: text("policy"), | ||
| softwareId: text("software_id"), | ||
| softwareVersion: text("software_version"), | ||
| softwareStatement: text("software_statement"), | ||
| redirectUris: text("redirect_uris").array().notNull(), | ||
| postLogoutRedirectUris: text("post_logout_redirect_uris").array(), | ||
| tokenEndpointAuthMethod: text("token_endpoint_auth_method"), | ||
| grantTypes: text("grant_types").array(), | ||
| responseTypes: text("response_types").array(), | ||
| public: boolean("public"), | ||
| type: text("type"), | ||
| referenceId: text("reference_id"), | ||
| metadata: jsonb("metadata"), | ||
| }); | ||
|
|
||
| export const oauthAccessTokens = authSchema.table( | ||
| "oauth_access_tokens", | ||
| { | ||
| id: uuid("id").primaryKey().defaultRandom(), | ||
| accessToken: text("access_token").unique(), | ||
| refreshToken: text("refresh_token").unique(), | ||
| accessTokenExpiresAt: timestamp("access_token_expires_at"), | ||
| refreshTokenExpiresAt: timestamp("refresh_token_expires_at"), | ||
| clientId: text("client_id").references(() => oauthApplications.clientId, { | ||
| onDelete: "cascade", | ||
| }), | ||
| userId: uuid("user_id").references(() => users.id, { onDelete: "cascade" }), | ||
| scopes: text("scopes"), | ||
| createdAt: timestamp("created_at"), | ||
| updatedAt: timestamp("updated_at"), | ||
| }, | ||
| (table) => [ | ||
| index("oauth_access_tokens_client_id_idx").on(table.clientId), | ||
| index("oauth_access_tokens_user_id_idx").on(table.userId), | ||
| ], | ||
| ); | ||
| export const oauthRefreshTokens = authSchema.table("oauth_refresh_tokens", { | ||
| id: uuid("id").primaryKey().defaultRandom(), | ||
| token: text("token").notNull(), | ||
| clientId: text("client_id") | ||
| .notNull() | ||
| .references(() => oauthClients.clientId, { onDelete: "cascade" }), | ||
| sessionId: uuid("session_id").references(() => sessions.id, { | ||
| onDelete: "set null", | ||
| }), | ||
| userId: uuid("user_id") | ||
| .notNull() | ||
| .references(() => users.id, { onDelete: "cascade" }), | ||
| referenceId: text("reference_id"), | ||
| expiresAt: timestamp("expires_at"), | ||
| createdAt: timestamp("created_at"), | ||
| revoked: timestamp("revoked"), | ||
| scopes: text("scopes").array().notNull(), | ||
| }); | ||
|
|
||
| export const oauthConsents = authSchema.table( | ||
| "oauth_consents", | ||
| { | ||
| id: uuid("id").primaryKey().defaultRandom(), | ||
| clientId: text("client_id").references(() => oauthApplications.clientId, { | ||
| onDelete: "cascade", | ||
| }), | ||
| userId: uuid("user_id").references(() => users.id, { onDelete: "cascade" }), | ||
| scopes: text("scopes"), | ||
| createdAt: timestamp("created_at"), | ||
| updatedAt: timestamp("updated_at"), | ||
| consentGiven: boolean("consent_given"), | ||
| }, | ||
| (table) => [ | ||
| index("oauth_consents_client_id_idx").on(table.clientId), | ||
| index("oauth_consents_user_id_idx").on(table.userId), | ||
| ], | ||
| ); | ||
| export const oauthAccessTokens = authSchema.table("oauth_access_tokens", { | ||
| id: uuid("id").primaryKey().defaultRandom(), | ||
| token: text("token").unique(), | ||
| clientId: text("client_id") | ||
| .notNull() | ||
| .references(() => oauthClients.clientId, { onDelete: "cascade" }), | ||
| sessionId: uuid("session_id").references(() => sessions.id, { | ||
| onDelete: "set null", | ||
| }), | ||
| userId: uuid("user_id").references(() => users.id, { onDelete: "cascade" }), | ||
| referenceId: text("reference_id"), | ||
| refreshId: uuid("refresh_id").references(() => oauthRefreshTokens.id, { | ||
| onDelete: "cascade", | ||
| }), | ||
| expiresAt: timestamp("expires_at"), | ||
| createdAt: timestamp("created_at"), | ||
| scopes: text("scopes").array().notNull(), | ||
| }); | ||
|
|
||
| export const oauthConsents = authSchema.table("oauth_consents", { | ||
| id: uuid("id").primaryKey().defaultRandom(), | ||
| clientId: text("client_id") | ||
| .notNull() | ||
| .references(() => oauthClients.clientId, { onDelete: "cascade" }), | ||
| userId: uuid("user_id").references(() => users.id, { onDelete: "cascade" }), | ||
| referenceId: text("reference_id"), | ||
| scopes: text("scopes").array().notNull(), | ||
| createdAt: timestamp("created_at"), | ||
| updatedAt: timestamp("updated_at"), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's check if better-auth or oauth-provider documentation mentions index expectations
if [ -d "node_modules/better-auth" ]; then
echo "=== Checking better-auth package.json ==="
cat node_modules/better-auth/package.json | jq '.version' 2>/dev/null || echo "Package not found or can't read"
fi
# Check for migration file
echo -e "\n=== Looking for migration files ==="
fd "0017" --type f 2>/dev/null || echo "Migration file not found"
# Search for any migration files in the db directory
echo -e "\n=== All migration files ==="
fd "\.sql$" packages/db/src/migrations 2>/dev/null | head -20 || echo "No migrations dir or files"
# Check the schema file itself for index definitions
echo -e "\n=== Checking schema auth.ts for index definitions ==="
if [ -f "packages/db/src/schema/auth.ts" ]; then
grep -n "index\|Index" packages/db/src/schema/auth.ts | head -20
fiRepository: superset-sh/superset
Length of output: 1041
🏁 Script executed:
#!/bin/bash
# Let's check the actual migration SQL files to see what was created
echo "=== Listing migration files ==="
ls -la packages/db/src/migrations/ 2>/dev/null || echo "No migrations directory"
echo -e "\n=== Recent migration files content ==="
ls -1t packages/db/src/migrations/*.sql 2>/dev/null | head -5 | while read file; do
echo "=== File: $file ==="
head -100 "$file"
echo ""
doneRepository: superset-sh/superset
Length of output: 159
🏁 Script executed:
#!/bin/bash
# Check better-auth library for OAuth table schema and index expectations
echo "=== Checking for better-auth schema references ==="
if [ -d "node_modules/better-auth" ]; then
find node_modules/better-auth -name "*.ts" -o -name "*.js" | xargs grep -l "oauth.*index\|oauth_.*createIndex" 2>/dev/null | head -10
fi
# Search the entire codebase for references to OAuth table indexes
echo -e "\n=== Searching codebase for index definitions on oauth tables ==="
rg "oauthAccessTokens|oauthRefreshTokens|oauthConsents" --type ts -A 10 packages/db/src/schema/ 2>/dev/null | grep -i "index" || echo "No index definitions found"Repository: superset-sh/superset
Length of output: 207
🏁 Script executed:
#!/bin/bash
# Check the exact schema definition to confirm the structure
echo "=== Current auth.ts schema (lines 156-235) ==="
sed -n '156,235p' packages/db/src/schema/auth.ts 2>/dev/null || cat packages/db/src/schema/auth.ts | sed -n '156,235p' 2>/dev/null || echo "Cannot read file"Repository: superset-sh/superset
Length of output: 3086
🏁 Script executed:
#!/bin/bash
# Read the migration SQL file
echo "=== Migration SQL file content ==="
cat packages/db/drizzle/0017_switch_to_oauth2.sqlRepository: superset-sh/superset
Length of output: 4768
🏁 Script executed:
#!/bin/bash
# Search for better-auth OAuth provider documentation or schema info
echo "=== Checking better-auth in node_modules ==="
ls -la node_modules/better-auth 2>/dev/null | head -20
echo -e "\n=== Looking for OAuth schema or index references in better-auth ==="
find node_modules/better-auth -type f -name "*.ts" -o -name "*.js" 2>/dev/null | xargs grep -l "oauth" | head -10
echo -e "\n=== Searching better-auth for database schema/index info ==="
find node_modules/better-auth -type f \( -name "*schema*" -o -name "*oauth*" \) 2>/dev/null | head -15Repository: superset-sh/superset
Length of output: 239
🏁 Script executed:
#!/bin/bash
# Check if there are any docs or README files mentioning OAuth indexes
echo "=== Searching for OAuth documentation ==="
find . -name "*.md" -type f -exec grep -l "oauth.*index\|oauth.*performance" {} \; 2>/dev/null | head -10Repository: superset-sh/superset
Length of output: 105
🏁 Script executed:
#!/bin/bash
# Check package.json for better-auth dependency
echo "=== Checking package.json for better-auth ==="
cat package.json | jq '.dependencies | select(. != null) | keys[]' 2>/dev/null | grep -i "auth\|better" || echo "Not found in package.json"
# Check all package.json files
echo -e "\n=== Searching all package.json files for better-auth ==="
find . -name "package.json" -type f -exec grep -l "better-auth\|@better-auth" {} \; 2>/dev/nullRepository: superset-sh/superset
Length of output: 351
🏁 Script executed:
#!/bin/bash
# Search the codebase for better-auth imports and usage
echo "=== Searching for better-auth imports ==="
rg "better-auth|oauth-provider" --type ts --type tsx --type js --type json 2>/dev/null | head -30
echo -e "\n=== Searching for runtime index creation patterns ==="
rg "createIndex|ensureIndex|CREATE INDEX" --type ts --type tsx 2>/dev/null | head -20Repository: superset-sh/superset
Length of output: 160
🏁 Script executed:
#!/bin/bash
# Check if better-auth is in package.json files
echo "=== Package dependencies in packages/db/package.json ==="
if [ -f "packages/db/package.json" ]; then
cat packages/db/package.json | jq '.dependencies' 2>/dev/null
fiRepository: superset-sh/superset
Length of output: 264
Add indexes to OAuth tables for query performance.
The oauthRefreshTokens, oauthAccessTokens, and oauthConsents tables have no secondary indexes despite frequent lookups on userId, clientId, and sessionId. Missing indexes will cause full table scans during token validation and consent checks.
Add indexes for these columns:
oauthRefreshTokens:(userId),(clientId),(sessionId)oauthAccessTokens:(userId),(clientId),(sessionId),(refreshId)oauthConsents:(clientId, userId)(composite for consent dedup)
🤖 Prompt for AI Agents
In `@packages/db/src/schema/auth.ts` around lines 156 - 235, Add missing secondary
indexes to improve lookup performance: modify the table definitions for
oauthRefreshTokens, oauthAccessTokens, and oauthConsents to create indexes on
the referenced columns. Specifically, add indexes on oauthRefreshTokens.userId,
oauthRefreshTokens.clientId, and oauthRefreshTokens.sessionId; on
oauthAccessTokens.userId, oauthAccessTokens.clientId,
oauthAccessTokens.sessionId, and oauthAccessTokens.refreshId; and add a
composite index on oauthConsents for (clientId, userId). Update the respective
authSchema table declarations (oauthRefreshTokens, oauthAccessTokens,
oauthConsents) to include index definitions for those column names so queries
use the new indexes.
| export const jwkss = authSchema.table("jwkss", { | ||
| id: uuid("id").primaryKey().defaultRandom(), | ||
| publicKey: text("public_key").notNull(), | ||
| privateKey: text("private_key").notNull(), | ||
| createdAt: timestamp("created_at").defaultNow().notNull(), | ||
| expiresAt: timestamp("expires_at"), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check better-auth for jwkss/jwks expectations
echo "=== Searching for jwkss in better-auth ==="
rg -n "jwkss" node_modules/@better-auth/ 2>/dev/null | head -20
echo -e "\n=== Searching for jwks table/schema references in better-auth ==="
rg -n "jwks" node_modules/@better-auth/ --type ts 2>/dev/null | grep -E "(table|schema|name)" | head -20
echo -e "\n=== Broader search for jwks in better-auth ==="
rg -n "jwks" node_modules/@better-auth/ 2>/dev/null | head -30Repository: superset-sh/superset
Length of output: 221
🏁 Script executed:
#!/bin/bash
# Search the codebase for jwkss usage
echo "=== Searching for jwkss in codebase ==="
rg -n "jwkss" --type ts packages/ 2>/dev/null
echo -e "\n=== Searching for better-auth imports/references ==="
rg -n "better-auth" --type ts packages/ 2>/dev/null | head -30
echo -e "\n=== Checking auth schema file for context ==="
cat -n packages/db/src/schema/auth.ts | sed -n '270,290p'
echo -e "\n=== Searching for oauth-provider references ==="
rg -n "oauth-provider\|oauthProvider" --type ts packages/ 2>/dev/null | head -20Repository: superset-sh/superset
Length of output: 1937
🏁 Script executed:
#!/bin/bash
# Check how oauthProvider is configured in server.ts
echo "=== Checking oauth-provider configuration in server.ts ==="
cat -n packages/auth/src/server.ts | head -100
echo -e "\n=== Search for any schema or table configuration with oauth-provider ==="
rg -n "oauthProvider\|oauth-provider" -A 5 -B 5 packages/auth/src/server.ts 2>/dev/null
echo -e "\n=== Check package.json for better-auth version ==="
rg -n "better-auth" package.json 2>/dev/nullRepository: superset-sh/superset
Length of output: 4263
🏁 Script executed:
#!/bin/bash
# Search for more context about jwkss table - check if there are comments or migration history
echo "=== Full auth.ts file around jwkss definition ==="
cat -n packages/db/src/schema/auth.ts | sed -n '270,295p'
echo -e "\n=== Check all exported tables/exports in auth.ts ==="
rg -n "^export const" packages/db/src/schema/auth.ts 2>/dev/null
echo -e "\n=== Search git history or comments mentioning jwkss ==="
rg -n "jwks" packages/db/src/ 2>/dev/nullRepository: superset-sh/superset
Length of output: 1581
🌐 Web query:
better-auth oauth-provider jwkss table schema expected name
💡 Result:
Better Auth expects the JWKS table to be named jwks (singular, not jwkss). The JWT plugin is what creates/uses it. [1]
Default jwks schema (JWT plugin): id (PK), publicKey, privateKey, createdAt, and optionally expiresAt (for expiring keys). [1]
Rename jwkss table to jwks to match better-auth's expected schema.
Better Auth's oauth-provider plugin expects the table to be named jwks (singular), not jwkss. The current name is redundant since "JWKS" already stands for "JSON Web Key Set". Update the table definition and generate a migration with drizzle-kit generate --name="rename_jwkss_to_jwks".
🤖 Prompt for AI Agents
In `@packages/db/src/schema/auth.ts` around lines 276 - 282, The DB schema
declares the table as jwkss via authSchema.table("jwkss", ...) which conflicts
with better-auth's expected jwks table name; rename the table identifier and
definition from "jwkss" to "jwks" (update the authSchema.table call and any
references to the jwkss constant) and then create a migration to rename the
table using drizzle-kit generate --name="rename_jwkss_to_jwks" so runtime and
migrations stay in sync.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
saddlepaddle
force-pushed
the
opencode-support
branch
from
9949d3b to
eb88595
Compare
Replace the deprecated better-auth `mcp()` plugin with `oauthProvider()` from `@better-auth/oauth-provider`, implementing a proper OAuth 2.1 authorization server for MCP client authentication. Key changes: - Switch server plugin from `mcp()` to `oauthProvider()` with JWT-based access tokens, dynamic client registration, and org-scoped claims - Add `oauthProviderClient()` to auth client for consent/continue flows - Add `.well-known` discovery endpoints (oauth-authorization-server, openid-configuration, oauth-protected-resource) with catch-all routes for path-relative MCP client discovery - Use `verifyAccessToken` from `better-auth/oauth2` for JWKS-based token verification in the MCP route handler - Simplify consent page by removing manual org picker (org selection now handled via postLogin.consentReferenceId) - Add new DB schema tables for OAuth clients, consents, refresh tokens, and JWKS - Remove deprecated `oauth-org-scope-endpoint` helper - Remove hardcoded OAuth config from `.mcp.json` (auto-discovered now)
saddlepaddle
force-pushed
the
opencode-support
branch
from
eb88595 to
2a6fa1e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/src/app/oauth/consent/page.tsx (2)
59-59:⚠️ Potential issue | 🟡 MinorEmpty
scopestring bypasses the["openid"]default.If the query string contains
scope=(empty value),"".split(" ")→[""]→.filter(Boolean)→[]. Since[]is truthy, the?? ["openid"]fallback never triggers, resulting in an empty scopes array passed toConsentForm.Suggested fix
- const scopes = scope?.split(" ").filter(Boolean) ?? ["openid"]; + const parsed = scope?.split(" ").filter(Boolean); + const scopes = parsed?.length ? parsed : ["openid"];
69-71:⚠️ Potential issue | 🟡 MinorRemove unnecessary type assertion —
activeOrganizationIdis already part of the session type.The inline
as typeof session.session & { activeOrganizationId?: ... }cast is redundant. The auth library'scustomSessionhook already extends the session withactiveOrganizationId(line 464-469 ofpackages/auth/src/server.ts), and it's a real database column. Many other files accesssession?.session?.activeOrganizationIddirectly without assertions. Simply remove the cast and trust the type system.
🤖 Fix all issues with AI agents
In `@packages/auth/src/server.ts`:
- Around line 160-162: The token issuer currently sets customAccessTokenClaims:
({ referenceId }) => ({ organizationId: referenceId ?? undefined }), which can
emit a token lacking organizationId and thus fail downstream; change
customAccessTokenClaims to validate referenceId and fail issuance when missing
(e.g., throw or return an error) so tokens are never issued without
organizationId, referencing the customAccessTokenClaims function and the
referenceId/organizationId fields so reviewers can locate and update the
issuance error path and any callers that need to handle the thrown/rejected
error.
- Around line 139-140: Because anonymous dynamic client registration is enabled
via allowDynamicClientRegistration and allowUnauthenticatedClientRegistration,
update the OAuth provider config to explicitly enforce a redirect_uri allowlist
and stronger rate limits: add a redirect URI validation hook (e.g., provide a
redirectUriAllowlist or a validateRedirectUri callback passed into the provider
config used in server.ts) that only accepts approved domain-specific/regex URIs
and reject any other values during client registration, and set the provider's
rateLimit config (override the default 5/60s) to a stricter policy appropriate
for your deployment; ensure these settings are applied where
allowDynamicClientRegistration and allowUnauthenticatedClientRegistration are
defined so anonymous registrations must pass redirect URI validation and rate
limits.
🧹 Nitpick comments (2)
packages/db/src/schema/auth.ts (1)
156-185: No indexes onoauthClients— at minimum add one onuserId.Other tables in this schema (e.g.,
sessions,accounts,members) consistently define indexes on theiruserIdforeign key columns.oauthClientsomits this, which will cause sequential scans when querying clients by user. Convert to the three-argumentauthSchema.table(...)form and add an index.Suggested fix
-export const oauthClients = authSchema.table("oauth_clients", { +export const oauthClients = authSchema.table("oauth_clients", { id: uuid("id").primaryKey().defaultRandom(), ... metadata: jsonb("metadata"), -}); +}, (table) => [ + index("oauth_clients_user_id_idx").on(table.userId), +]);apps/api/src/app/api/agent/[transport]/route.ts (1)
34-65: JWT payload fields are treated as trusted — consider light validation.The JWT payload is external data. Fields like
payload.organizationId(line 62) andpayload.azp(line 57) are cast tostringwithout runtime type checks. If the token was issued by a misconfigured or compromised server, non-string values could propagate silently.A lightweight guard would tighten this:
Suggested hardening
- if (!payload?.sub || !payload.organizationId) { + if ( + !payload?.sub || + typeof payload.sub !== "string" || + !payload.organizationId || + typeof payload.organizationId !== "string" + ) {This aligns with the coding guideline: "Validate external API data as untrusted by handling missing fields, unknown enums, and unexpected shapes with tolerant parsing and explicit fallbacks."
| allowDynamicClientRegistration: true, | ||
| allowUnauthenticatedClientRegistration: true, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/auth/src/server.ts | head -200 | tail -100Repository: superset-sh/superset
Length of output: 3701
🏁 Script executed:
cd packages/auth && find . -name "*.json" | grep -E "(package|tsconfig)" | head -5Repository: superset-sh/superset
Length of output: 94
🌐 Web query:
@better-auth/oauth-provider redirect_uri restrictions rate limiting dynamic client registration
💡 Result:
redirect_uri restrictions (OAuth Provider plugin)
- OAuth clients must be registered with an allowlist of
redirect_uris(stored asredirectUris), and clients are created/registered by supplying that list. (better-auth.com) - For the provider’s login/consent redirect screens, Better Auth verifies a signed query derived from the initial
/oauth2/authorizerequest; if you add custom query params to your own pages, you’re expected to append them after the signed portion (aftersig). (canary.better-auth.com)
Rate limiting
- Global Better Auth rate limiting (when enabled) defaults to 60s window / 100 requests in production; it’s disabled by default in development unless you enable it. (better-auth.com)
- The OAuth Provider additionally defines per-endpoint, per-IP limits (only effective when global rate limiting is enabled). Defaults shown in the canary docs:
/oauth2/token: 60s / 20/oauth2/authorize: 60s / 30/oauth2/introspect: 60s / 100/oauth2/revoke: 60s / 30/oauth2/register: 60s / 5/oauth2/userinfo: 60s / 60
and these can be overridden per endpoint viaoauthProvider({ rateLimit: { ... } })or disabled per-endpoint (fall back to global) by setting an endpoint tofalse. (canary.better-auth.com)
Dynamic client registration
- The plugin supports RFC 7591 dynamic client registration. (canary.better-auth.com)
- Enable it with
allowDynamicClientRegistration: true. (canary.better-auth.com) - You can also allow unauthenticated registration for public clients only via
allowUnauthenticatedClientRegistration: true(noted as planned for deprecation depending on MCP standardization debates). (canary.better-auth.com) - Some RFC7591 fields are explicitly “not yet supported” (notably
jwksandjwks_uri). (canary.better-auth.com)
Citations:
- 1: https://www.better-auth.com/docs/plugins/oauth-provider
- 2: https://canary.better-auth.com/docs/plugins/oauth-provider
- 3: https://www.better-auth.com/docs/concepts/rate-limit?utm_source=openai
- 4: https://canary.better-auth.com/docs/plugins/oauth-provider
- 5: https://canary.better-auth.com/docs/plugins/oauth-provider
- 6: https://canary.better-auth.com/docs/plugins/oauth-provider
- 7: https://canary.better-auth.com/docs/plugins/oauth-provider
- 8: https://canary.better-auth.com/docs/plugins/oauth-provider
🏁 Script executed:
rg -i "redirect_uri|redirectUri" packages/auth/src/ -A 3 -B 3Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
rg -i "allowDynamicClientRegistration|client.*registration" packages/auth/src/ -A 5 -B 5Repository: superset-sh/superset
Length of output: 793
Verify that dynamic client registration enforces restricted redirect_uris and consider stricter rate limits.
allowDynamicClientRegistration: true with allowUnauthenticatedClientRegistration: true allows anonymous callers to register OAuth clients. While @better-auth/oauth-provider provides default rate limiting (5 registrations per 60 seconds per IP on /oauth2/register), this alone is insufficient to prevent abuse.
Ensure that:
- Client registration requires and validates
redirect_uriallowlists—clients should only be able to register with restricted, domain-specific redirect URIs to prevent phishing and token-confusion attacks. - Consider configuring stricter rate limits via the
rateLimitconfig option if the default 5 per 60s is insufficient for your use case.
Both features are supported by the library; they should be explicitly configured and enforced rather than relying on defaults.
🤖 Prompt for AI Agents
In `@packages/auth/src/server.ts` around lines 139 - 140, Because anonymous
dynamic client registration is enabled via allowDynamicClientRegistration and
allowUnauthenticatedClientRegistration, update the OAuth provider config to
explicitly enforce a redirect_uri allowlist and stronger rate limits: add a
redirect URI validation hook (e.g., provide a redirectUriAllowlist or a
validateRedirectUri callback passed into the provider config used in server.ts)
that only accepts approved domain-specific/regex URIs and reject any other
values during client registration, and set the provider's rateLimit config
(override the default 5/60s) to a stricter policy appropriate for your
deployment; ensure these settings are applied where
allowDynamicClientRegistration and allowUnauthenticatedClientRegistration are
defined so anonymous registrations must pass redirect URI validation and rate
limits.
| customAccessTokenClaims: ({ referenceId }) => ({ | ||
| organizationId: referenceId ?? undefined, | ||
| }), |
There was a problem hiding this comment.
organizationId claim can be undefined if referenceId is missing — tokens will fail downstream verification.
customAccessTokenClaims emits organizationId: referenceId ?? undefined. If referenceId is falsy, the issued JWT won't contain an organizationId claim, but route.ts (line 42) rejects tokens without it:
if (!payload?.sub || !payload.organizationId) { return undefined; }This means a token would be issued successfully but immediately fail on use. Either guard here (throw/reject the token issuance) or make the downstream check lenient — silent success followed by silent failure is hard to debug.
Proposed fix — fail loudly at issuance time
- customAccessTokenClaims: ({ referenceId }) => ({
- organizationId: referenceId ?? undefined,
- }),
+ customAccessTokenClaims: ({ referenceId }) => {
+ if (!referenceId) {
+ throw new Error("Cannot issue access token without organizationId (referenceId)");
+ }
+ return { organizationId: referenceId };
+ },🤖 Prompt for AI Agents
In `@packages/auth/src/server.ts` around lines 160 - 162, The token issuer
currently sets customAccessTokenClaims: ({ referenceId }) => ({ organizationId:
referenceId ?? undefined }), which can emit a token lacking organizationId and
thus fail downstream; change customAccessTokenClaims to validate referenceId and
fail issuance when missing (e.g., throw or return an error) so tokens are never
issued without organizationId, referencing the customAccessTokenClaims function
and the referenceId/organizationId fields so reviewers can locate and update the
issuance error path and any callers that need to handle the thrown/rejected
error.
1 participant
Summary
mcp()plugin withoauthProvider()from@better-auth/oauth-providerfor a proper OAuth 2.1 authorization server.well-knowndiscovery endpoints with catch-all routes for MCP client path-relative discoveryverifyAccessTokenpostLogin.consentReferenceId+customAccessTokenClaims(org ID embedded as JWT claim)oauth-org-scope-endpointand hardcoded OAuth config from.mcp.json.well-knownnow)Test plan
bun run lint:fix— cleanbun run typecheck— 20/20 packages passbun test— 1205 pass, 0 fail.well-known/oauth-protected-resource→.well-known/oauth-authorization-server→ registers dynamically → login → consent → token exchange → JWT withorganizationIdclaim → MCP tools workSummary by CodeRabbit
New Features
Improvements
Documentation