feat: add OAuth 2.1 / OIDC provider support#3103
Conversation
Enable the platform to act as an OAuth 2.1 authorization server so downstream clients (CLI, SDK, MCP) can authenticate via standard OAuth flows. Changes: - Add @better-auth/oauth-provider plugin with jwt() support - Register OIDC discovery endpoints (/.well-known/openid-configuration, /.well-known/oauth-authorization-server/*) - Add OAuth schema tables (oauth_client, oauth_access_token, oauth_refresh_token, oauth_consent, jwks) with Drizzle relations - Extend login page to detect OAuth authorize redirects and resume the flow after authentication - Add customAccessTokenClaims with tenantId and email claims Known limitation: customAccessTokenClaims uses getInitialOrganization (first org by membership date). Multi-org users will need an org selection flow in a follow-up (post-login org picker + token re-issuance). Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
🦋 Changeset detectedLatest commit: bb36226 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
TL;DR — Turns Inkeep Agents into a standards-compliant OAuth 2.1 / OpenID Connect provider so external applications can authenticate users via Inkeep using authorization code + PKCE flows. Adds 5 new database tables for OAuth state, wires the Key changes
Summary ｜ 12 files ｜ 2 commits ｜ base: OAuth provider schema and database tables
The
Auth plugin configuration and custom claims
The
Custom access token claims inject
OIDC discovery endpoints
Both delegate to the Better Auth instance via the
OAuth-aware login flow
Three authentication paths are handled:
|
|
TL;DR — Adds OAuth 2.1 / OIDC provider capabilities so the Inkeep Agents platform can act as an identity provider for external clients. Integrates the Key changes
Summary ｜ 15 files ｜ 4 commits ｜ base: OAuth 2.1 / OIDC provider plugin integration
The
OAuth data model and migration
The Drizzle ORM schema in
OAuth consent page with auth guard
The
OAuth-aware login page
The redirect priority chain is now: OAuth authorize URL > invitation ID > return URL > home. For SSO/social login, the callback URL loops back through the login page with the original OAuth search params so the post-login
|
There was a problem hiding this comment.
Severity: MAJOR — missing consent page will break the OAuth flow at runtime; missing indexes will degrade performance under load.
This PR introduces a well-structured OAuth 2.1 / OIDC provider using @better-auth/oauth-provider. The schema, relations, and discovery endpoint wiring are all solid. The disabledPaths: ['/token'] to block the JWT plugin's session-to-JWT exchange while leaving /oauth2/token active is a good security call. The client_secret hashing via SHA-256 is handled correctly by the plugin.
Key issues to address before merge:
- Missing consent page —
oauthProvider({ consentPage: '.../consent' })references a page that does not exist inagents-manage-ui/src/app/consent/. OAuth clients requesting consent will hit a 404. - No indexes on new OAuth tables — 10 foreign key columns across 4 tables have no indexes. CASCADE deletes and common query patterns (token lookup by
client_id, consent byuser_id + client_id) will require sequential scans. - JWKS private keys stored as plaintext — The
jwks.private_keycolumn stores key material as unencrypted text. If the database is compromised, all issued tokens can be forged. This is abetter-authdesign choice but should be tracked for hardening (TDE, KMS, or a custom adapter). - CORS on discovery endpoints —
/.well-known/openid-configurationfalls underdefaultCorsConfigwith restricted origins. Per RFC 8414, these should be publicly accessible. External OAuth clients fetching discovery metadata from different origins will be blocked. - Missing changeset — This PR adds a new runtime feature to
agents-coreandagents-apibut has no changeset entry.
Claude Opus ｜ 𝕏
| jwt(), | ||
| oauthProvider({ | ||
| loginPage: `${env.INKEEP_AGENTS_MANAGE_UI_URL || 'http://localhost:3000'}/login`, | ||
| consentPage: `${env.INKEEP_AGENTS_MANAGE_UI_URL || 'http://localhost:3000'}/consent`, |
There was a problem hiding this comment.
The consentPage points to ${env.INKEEP_AGENTS_MANAGE_UI_URL}/consent, but no page exists at agents-manage-ui/src/app/consent/. OAuth flows that require user consent will redirect to a 404. Either create the consent page or, if consent is always skipped (skipConsent: true on all registered clients), document that assumption explicitly.
| if (!user) return {}; | ||
| const org = await getInitialOrganization(config.dbClient, user.id); | ||
| return { | ||
| 'https://inkeep.com/tenantId': org?.id ?? undefined, |
There was a problem hiding this comment.
org?.id ?? undefined — when org is null (user has no org membership), this emits 'https://inkeep.com/tenantId': undefined. Depending on the JWT serializer, undefined is either omitted or serialized as null. If downstream consumers rely on this claim's presence to determine authorization, a missing claim could silently grant or deny access. Consider returning an empty object early or throwing when org is null, depending on whether org-less users should be able to complete the OAuth flow at all.
| appName: 'Inkeep Agents', | ||
| baseURL: config.baseURL, | ||
| secret: config.secret, | ||
| disabledPaths: ['/token'], |
There was a problem hiding this comment.
Good call disabling the JWT plugin's /token endpoint. This forces token issuance through the OAuth 2.1 flow exclusively (/oauth2/token), which enforces client authentication, PKCE, and scope validation. Worth a brief inline note (or commit message mention) so future maintainers don't remove this thinking it's a mistake.
| export const jwks = pgTable('jwks', { | ||
| id: text('id').primaryKey(), | ||
| publicKey: text('public_key').notNull(), | ||
| privateKey: text('private_key').notNull(), |
There was a problem hiding this comment.
The private_key column stores JWK key material as plaintext. If the runtime database is compromised, an attacker can sign arbitrary access/ID tokens. This is better-auth's default behavior — not something to fix in this PR — but it should be tracked as a follow-up hardening item (database-level TDE, application-level envelope encryption, or external KMS/HSM for key storage).
| export const oauthRefreshToken = pgTable('oauth_refresh_token', { | ||
| id: text('id').primaryKey(), | ||
| token: text('token').notNull(), | ||
| clientId: text('client_id') | ||
| .notNull() | ||
| .references(() => oauthClient.clientId, { onDelete: 'cascade' }), | ||
| sessionId: text('session_id').references(() => session.id, { | ||
| onDelete: 'set null', | ||
| }), | ||
| userId: text('user_id') | ||
| .notNull() | ||
| .references(() => user.id, { onDelete: 'cascade' }), | ||
| referenceId: text('reference_id'), | ||
| expiresAt: timestamp('expires_at'), | ||
| createdAt: timestamp('created_at'), | ||
| revoked: timestamp('revoked'), | ||
| authTime: timestamp('auth_time'), | ||
| scopes: text('scopes').array().notNull(), | ||
| }); | ||
|
|
||
| export const oauthAccessToken = pgTable('oauth_access_token', { | ||
| id: text('id').primaryKey(), | ||
| token: text('token').unique(), | ||
| clientId: text('client_id') | ||
| .notNull() | ||
| .references(() => oauthClient.clientId, { onDelete: 'cascade' }), | ||
| sessionId: text('session_id').references(() => session.id, { | ||
| onDelete: 'set null', | ||
| }), | ||
| userId: text('user_id').references(() => user.id, { onDelete: 'cascade' }), | ||
| referenceId: text('reference_id'), | ||
| refreshId: text('refresh_id').references(() => oauthRefreshToken.id, { | ||
| onDelete: 'cascade', | ||
| }), | ||
| expiresAt: timestamp('expires_at'), | ||
| createdAt: timestamp('created_at'), | ||
| scopes: text('scopes').array().notNull(), | ||
| }); |
There was a problem hiding this comment.
None of the new OAuth tables have indexes on their foreign key columns (client_id, user_id, session_id, refresh_id). PostgreSQL does not auto-create indexes for FK columns (only for PK/unique constraints). This means:
ON DELETE CASCADEfromuser/session/oauth_clientwill sequential-scan the child tables- Token lookup by
client_id, consent checks by(client_id, user_id), and refresh token revocation byuser_idwill all be slow
At minimum, add indexes on: oauth_access_token(client_id), oauth_access_token(user_id), oauth_refresh_token(client_id), oauth_refresh_token(user_id), oauth_consent(client_id, user_id), and oauth_client(user_id). Compare with existing auth tables which all have explicit indexes (e.g., session_userId_idx, account_userId_idx).
| app.get('/.well-known/openid-configuration', (c) => { | ||
| return openIdConfigHandler(c.req.raw); | ||
| }); | ||
| app.get('/.well-known/oauth-authorization-server/*', (c) => { | ||
| return authServerMetadataHandler(c.req.raw); | ||
| }); |
There was a problem hiding this comment.
These OIDC/OAuth discovery endpoints fall under defaultCorsConfig, which restricts origins to the manage UI domain and same-base-domain origins. Per RFC 8414 and OpenID Connect Discovery 1.0, these are public metadata endpoints expected to be accessible cross-origin. External OAuth clients (running on different domains) fetching /.well-known/openid-configuration from a browser will be blocked by CORS.
Consider adding a permissive CORS config for these routes specifically:
app.use('/.well-known/openid-configuration', cors({ origin: '*', maxAge: 86400 }));
app.use('/.well-known/oauth-authorization-server/*', cors({ origin: '*', maxAge: 86400 }));| // when redirecting unauthenticated users to the login page. After login, | ||
| // we must redirect back to the authorize endpoint with those same params | ||
| // so the OAuth flow can resume and issue the authorization code. | ||
| const isOAuthFlow = searchParams.has('client_id'); |
There was a problem hiding this comment.
searchParams.has('client_id') is a broad heuristic — any link to /login?client_id=foo will trigger OAuth redirect behavior, even if the client_id is invalid or malicious. The redirect ultimately goes to the API's /api/auth/oauth2/authorize which validates the client, so there's no open-redirect vulnerability here, but it does change the login UX (skipping router.replace in favor of window.location.href). Consider adding a comment noting that server-side validation of the client_id is the security boundary, not this client-side check.
| // so the OAuth flow can resume and issue the authorization code. | ||
| const isOAuthFlow = searchParams.has('client_id'); | ||
| const oauthAuthorizeUrl = isOAuthFlow | ||
| ? `${PUBLIC_INKEEP_AGENTS_API_URL}/api/auth/oauth2/authorize?${searchParams.toString()}` |
There was a problem hiding this comment.
searchParams.toString() forwards ALL query parameters to the authorize endpoint. If someone adds extra parameters to the login URL (e.g., ?client_id=x&prompt=none), they all get passed through. This is likely fine since @better-auth/oauth-provider should ignore unknown params, but it's worth being aware of — especially for parameters like prompt or login_hint that have defined OAuth semantics.
| }, | ||
| }, | ||
| session: { | ||
| storeSessionInDatabase: true, |
There was a problem hiding this comment.
This setting only has an effect when secondaryStorage (e.g., Redis) is configured — without it, sessions are always stored in the database. Since there's no secondaryStorage in the current config, this is a no-op today. It's good as a defensive measure for future-proofing, but the PR description implies it's a behavioral change.
There was a problem hiding this comment.
PR Review Summary
(8) Total Issues | Risk: High
This PR adds OAuth 2.1/OIDC provider support via Better Auth's oauth-provider plugin. While the implementation follows Better Auth patterns correctly, there are several critical and major issues that should be addressed before merging—particularly around the missing consent page, credential storage, and multi-org tenant claims.
🔴❗ Critical (1) ❗🔴
Inline Comments:
- 🔴 Critical:
packages/agents-core/src/auth/auth.ts:208Missing/consentpage will break OAuth flows requiring user consent
🟠⚠️ Major (4) ⚠️ 🟠
🟠 1) packages/agents-core/src/auth/auth-schema.ts:86-92 JWT private keys stored in plaintext
Issue: The jwks table stores JWT signing private keys in plaintext. These keys can sign access tokens for any OAuth client, making them high-value targets.
Why: If the database is compromised (SQL injection, backup leak, insider access), attackers can forge arbitrary access tokens and impersonate any user. This is a cryptographic key storage concern (CWE-321).
Fix: Verify if Better Auth supports custom key providers or encrypted storage. Consider application-level encryption with keys in a secret manager (AWS KMS, Vault). At minimum, ensure database encryption at rest is enabled and document this as an accepted operational requirement.
Refs:
🟠 2) packages/agents-core/src/auth/auth.ts:210-217 Multi-org tenantId claim uses non-deterministic organization selection
Issue: customAccessTokenClaims uses getInitialOrganization() which returns the user's first organization by membership createdAt date. For multi-org users, the access token embeds an arbitrary tenantId regardless of the intended authorization context.
Why: Downstream systems relying on the https://inkeep.com/tenantId claim will receive the wrong tenant context for multi-org users. This could lead to data access under incorrect tenant permissions or cross-tenant exposure if tokens are used in a different org context. The PR acknowledges this limitation but ships it as-is.
Fix: Consider scoping OAuth clients to specific organizations (derive tenantId from the client registration). Alternatively, add an org selection step during the OAuth consent flow. At minimum, create a tracked issue for the multi-org picker flow and add a prominent code comment.
Refs:
🟠 3) system No automated tests for security-critical OAuth functionality
Issue: This PR adds a significant security-critical authentication subsystem (OAuth 2.1 provider, JWT signing, custom claims, token storage) with no accompanying unit or integration tests.
Why: The customAccessTokenClaims function contains critical business logic determining what claims are embedded in access tokens. The OIDC discovery endpoints must return spec-compliant responses. The login page OAuth redirect logic handles the critical handoff between authentication and authorization. Without tests:
- Regressions in claim generation could cause cross-tenant access issues
- Malformed discovery responses would break all OAuth clients
- Incorrect redirect handling could cause authentication loops
Fix: Add tests covering:
customAccessTokenClaimscallback (null user, user with org, user without org, multi-org user)- OIDC discovery endpoints (
/.well-known/openid-configurationreturns required fields) - Login page OAuth flow detection and redirect construction
- Schema cascade delete behavior for OAuth tables
Refs:
Inline Comments:
- 🟠 Major:
packages/agents-core/src/auth/auth.ts:210-217Multi-org tenantId claim issue (detailed above) - 🟠 Major:
packages/agents-core/src/auth/auth-schema.ts:86-92JWT private keys in plaintext - 🟠 Major:
packages/agents-core/src/auth/auth-schema.ts:129Missing unique constraint on refresh token
🟡 Minor (2) 🟡
🟡 1) packages/agents-core/src/auth/auth-schema.ts:94-97 OAuth client secrets stored in plaintext
Issue: The oauth_client table stores clientSecret in plaintext. Client secrets authenticate OAuth clients during token exchange.
Why: Database compromise would expose all OAuth client secrets. While client secrets are less sensitive than user credentials, storing them hashed provides defense-in-depth. This may be a Better Auth framework limitation.
Fix: Verify if Better Auth supports hashed client secret storage. If not, document as accepted risk and ensure secrets are never logged.
Refs:
🟡 2) packages/agents-core/src/auth/auth-schema.ts:122 PKCE not enforced by default
Issue: The oauth_client table has requirePKCE: boolean as optional, defaulting to null/false. OAuth 2.1 mandates PKCE for all authorization code flows.
Why: Without PKCE enforcement, the authorization code flow is vulnerable to code interception attacks, especially for public clients.
Fix: Consider defaulting require_pkce to true in the schema or enforcing PKCE at the provider configuration level.
Refs:
Inline Comments:
- 🟡 Minor:
agents-manage-ui/src/app/login/page.tsx:56-59OAuth flow detection relies solely onclient_idpresence
💭 Consider (1) 💭
💭 1) packages/agents-core/src/auth/auth-schema.ts OAuth tables lack indexes for common query patterns
Issue: New OAuth tables have FK references but no explicit indexes on commonly queried columns like userId, sessionId, or expiresAt. Postgres does not auto-create indexes for FK columns.
Why: Token lookup, revocation, and cleanup operations may degrade as data grows.
Fix: Add indexes for oauth_access_token(userId, sessionId, expiresAt), oauth_refresh_token(userId, sessionId, expiresAt), oauth_consent(userId).
🚫 REQUEST CHANGES
Summary: This PR introduces important OAuth 2.1/OIDC provider capability, but has blocking issues that should be addressed:
-
Critical: The missing
/consentpage will break OAuth flows for clients requiring consent. Either create the page or ensure all initial clients useskipConsent: truewith documentation. -
Major: The multi-org tenantId claim behavior needs explicit acknowledgment—consider scoping clients to orgs or creating a tracked issue for the org-picker flow.
-
Major: Security-critical auth code should have test coverage. At minimum, add tests for
customAccessTokenClaimsand OIDC discovery endpoints. -
Major: Add
.unique()constraint tooauth_refresh_token.tokento matchoauth_access_token.token.
The credential storage concerns (JWKS private keys, client secrets) may be Better Auth framework limitations—verify and document if so.
Discarded (5)
| Location | Issue | Reason Discarded |
|---|---|---|
auth.ts:96 |
disabledPaths: ['/token'] may conflict with existing flows |
Better Auth's oauth-provider uses /oauth2/token, not /token. Verified deviceAuthorization uses different path. |
auth.ts:175 |
storeSessionInDatabase: true is a new setting |
Informational - this makes existing behavior explicit, not a change. Better Auth defaults to DB storage with drizzleAdapter. |
createApp.ts:136-141 |
OIDC discovery endpoints lack explicit CORS | Discovery endpoints are public metadata. Global CORS fallback is appropriate. Not a security issue. |
.changeset |
Minor version bump may be incorrect | Minor is appropriate for additive capability with schema migration. No breaking changes to existing API. |
auth-schema.ts:127-145 |
Refresh tokens stored in plaintext | While hashing would be more secure, this is likely a Better Auth framework requirement for token refresh flow. Marked as lower priority than JWKS keys. |
Reviewers (6)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-security-iam |
7 | 2 | 0 | 0 | 2 | 0 | 3 |
pr-review-tests |
6 | 1 | 0 | 0 | 0 | 0 | 5 |
pr-review-architecture |
5 | 1 | 1 | 0 | 0 | 0 | 3 |
pr-review-appsec |
5 | 1 | 0 | 0 | 1 | 0 | 3 |
pr-review-breaking-changes |
5 | 0 | 0 | 0 | 0 | 0 | 5 |
pr-review-standards |
3 | 1 | 0 | 0 | 2 | 0 | 0 |
| Total | 31 | 6 | 1 | 0 | 5 | 0 | 19 |
Note: High discard rate due to significant overlap across reviewers on the same issues (JWKS storage, consent page, multi-org claims raised by 3-4 reviewers each).
| jwt(), | ||
| oauthProvider({ | ||
| loginPage: `${env.INKEEP_AGENTS_MANAGE_UI_URL || 'http://localhost:3000'}/login`, | ||
| consentPage: `${env.INKEEP_AGENTS_MANAGE_UI_URL || 'http://localhost:3000'}/consent`, |
There was a problem hiding this comment.
🔴 CRITICAL: Missing /consent page will break OAuth flows
Issue: The OAuth provider is configured with consentPage pointing to /consent, but no consent page exists in agents-manage-ui/src/app/consent/. OAuth flows requiring user consent will fail with a 404.
Why: When Better Auth redirects to the consent page during an OAuth authorization flow, users will hit a 404 error. While skipConsent: true can be set on individual OAuth clients, any third-party client registration or explicit consent requirement will break the flow entirely. This is a blocking issue for OAuth flows that require consent.
Fix: Either:
- Create the consent page at
agents-manage-ui/src/app/consent/page.tsxthat displays requested scopes and allows approve/deny - If consent is not needed for initial use cases (internal clients only), ensure all registered clients have
skipConsent: trueand document this limitation prominently
Refs:
| customAccessTokenClaims: async ({ user }) => { | ||
| if (!user) return {}; | ||
| const org = await getInitialOrganization(config.dbClient, user.id); | ||
| return { | ||
| 'https://inkeep.com/tenantId': org?.id ?? undefined, | ||
| 'https://inkeep.com/email': user.email, | ||
| }; | ||
| }, |
There was a problem hiding this comment.
🟠 MAJOR: Multi-org tenantId claim uses non-deterministic "first org" heuristic
Issue: The customAccessTokenClaims callback uses getInitialOrganization() which returns the user's first organization by membership creation date. For multi-org users, the access token will contain their earliest org's tenantId regardless of the intended authorization context.
Why: OAuth access tokens are long-lived and cached by clients. If a user belongs to multiple orgs and the token embeds the wrong tenantId:
- Downstream consumers may authorize actions against the wrong tenant
- Data access could occur under incorrect tenant permissions
- This is acknowledged in the PR description but represents significant technical debt
Fix: Consider these options:
- Short-term: Scope OAuth clients to specific organizations and derive tenantId from the client registration rather than user membership (query
oauth_clientfor its associated org) - Medium-term: Add an org selection step during the OAuth consent flow that captures the selected org in session state
- At minimum: Add a TODO/FIXME comment in code and create a tracked issue for the multi-org org-picker flow
Refs:
| export const jwks = pgTable('jwks', { | ||
| id: text('id').primaryKey(), | ||
| publicKey: text('public_key').notNull(), | ||
| privateKey: text('private_key').notNull(), | ||
| createdAt: timestamp('created_at').notNull(), | ||
| expiresAt: timestamp('expires_at'), | ||
| }); |
There was a problem hiding this comment.
🟠 MAJOR: JWT private keys stored in plaintext in database
Issue: The jwks table stores JWT signing private keys in plaintext (privateKey: text('private_key').notNull()). These keys can sign access tokens for any OAuth client.
Why: If the database is compromised (SQL injection, backup leak, insider access), attackers can forge arbitrary access tokens and impersonate any user. This is a high-value target for credential theft.
Fix: This may be a Better Auth framework limitation, but consider:
- Verify: Check if Better Auth supports custom key storage providers or encrypted storage adapters
- Encrypt at rest: Use application-level encryption with a key stored in a secret manager (AWS KMS, HashiCorp Vault)
- Document: If this is an accepted risk, document the security assumption and ensure database encryption at rest is enabled
- Long-term: Consider using a dedicated key management service for JWT signing
Refs:
|
|
||
| export const oauthRefreshToken = pgTable('oauth_refresh_token', { | ||
| id: text('id').primaryKey(), | ||
| token: text('token').notNull(), |
There was a problem hiding this comment.
🟠 MAJOR: Missing unique constraint on oauth_refresh_token.token
Issue: The oauth_refresh_token table has a token column marked as NOT NULL but lacks a unique constraint, unlike oauth_access_token.token which has .unique().
Why: Refresh tokens must be unique to prevent token collision attacks. Without a unique constraint:
- Duplicate refresh tokens could exist in the database
- Token lookups could return the wrong record
- Revoking one token might leave a duplicate active
Fix:
| token: text('token').notNull(), | |
| token: text('token').notNull().unique(), |
Refs:
| const isOAuthFlow = searchParams.has('client_id'); | ||
| const oauthAuthorizeUrl = isOAuthFlow | ||
| ? `${PUBLIC_INKEEP_AGENTS_API_URL}/api/auth/oauth2/authorize?${searchParams.toString()}` | ||
| : null; |
There was a problem hiding this comment.
🟡 Minor: OAuth flow detection relies solely on client_id presence
Issue: The OAuth flow is detected by checking searchParams.has('client_id') only. However, client_id could appear in query params for other reasons (analytics, deep links, etc.), which could incorrectly trigger OAuth redirect behavior.
Why: If a non-OAuth URL contains client_id in the query string, authenticated users would be unexpectedly redirected to /api/auth/oauth2/authorize instead of their intended destination.
Fix: Consider checking for multiple OAuth-specific parameters to reduce false positives:
| const isOAuthFlow = searchParams.has('client_id'); | |
| const oauthAuthorizeUrl = isOAuthFlow | |
| ? `${PUBLIC_INKEEP_AGENTS_API_URL}/api/auth/oauth2/authorize?${searchParams.toString()}` | |
| : null; | |
| const isOAuthFlow = searchParams.has('client_id') && searchParams.has('redirect_uri'); | |
| const oauthAuthorizeUrl = isOAuthFlow | |
| ? `${PUBLIC_INKEEP_AGENTS_API_URL}/api/auth/oauth2/authorize?${searchParams.toString()}` | |
| : null; |
Refs:
Preview URLsUse these stable preview aliases for testing this PR:
These point to the same Vercel preview deployment as the bot comment, but they stay stable and easier to find. Raw Vercel deployment URLs
|
Add /consent page that Better Auth's oauth-provider plugin redirects to when an OAuth client requires user consent. Displays the requesting application name and requested scopes, and POSTs the user's decision back to the /api/auth/oauth2/consent endpoint. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low (delta only)
This is a delta review scoped to the changes since the last automated review (b059082 → 391d34fa). The delta adds 2 files implementing the OAuth consent page that was flagged as CRITICAL missing in the prior review.
✅ Critical Issue Resolved
The missing /consent page that would have broken OAuth flows has been implemented. The consent page:
- ✅ Properly handles loading and unauthenticated states
- ✅ Displays requested scopes with human-readable labels
- ✅ Sends consent decisions to
/api/auth/oauth2/consentwith credentials included - ✅ Handles redirect responses and error states correctly
- ✅ Uses existing UI component library (Card, Button, Alert, ShieldCheck icon)
- ✅ Wraps in Suspense for client-side
useSearchParams()
💭 Consider (2) 💭
Inline Comments:
- 💭 Consider:
consent/layout.tsx:8UseLayoutProps<'/consent'>to match codebase pattern - 💭 Consider:
consent/page.tsx:70-77Redirect unauthenticated users to login with OAuth params preserved
🕐 Pending Recommendations (Prior Review)
The following issues from prior reviews remain applicable (not addressed in this delta):
- 🟠 JWT private keys stored in plaintext — Better Auth framework limitation, document as accepted risk
- 🟠 Multi-org tenantId claim uses non-deterministic org selection — Acknowledged in PR description as known limitation
- 🟠 No automated tests for OAuth functionality — Security-critical code should have test coverage
- 🟡 Missing unique constraint on refresh token —
oauth_refresh_token.tokenshould have.unique() - 🟡 Missing indexes on OAuth tables — FK columns lack indexes for efficient queries
💡 APPROVE WITH SUGGESTIONS
Summary: The critical missing consent page has been added. The implementation is solid and follows existing UI patterns. The two minor suggestions (layout typing, unauthenticated redirect) are quality-of-life improvements, not blockers. The prior major/minor issues from earlier reviews (plaintext keys, multi-org claims, missing tests, missing indexes) remain applicable and should be tracked for follow-up.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
2 | 0 | 2 | 0 | 2 | 5 | 0 |
| Total | 2 | 0 | 2 | 0 | 2 | 5 | 0 |
Note: Delta review — sub-reviewers not dispatched for 2-file UI-only change that resolves prior critical feedback.
| title: 'Authorize Application', | ||
| } satisfies Metadata; | ||
|
|
||
| export default function Layout({ children }: { children: ReactNode }) { |
There was a problem hiding this comment.
💭 Consider: Layout typing pattern
Issue: The layout uses { children: ReactNode } instead of the codebase's typed LayoutProps<'/consent'> pattern.
Why: Other layouts in the codebase use LayoutProps from the Next.js typed routes system (see login/layout.tsx, no-organization/layout.tsx), which provides better type safety for route-specific props.
Fix:
export default function Layout({ children }: LayoutProps<'/consent'>) {
return children;
}Refs:
| You must be signed in to authorize an application. | ||
| </p> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
💭 Consider: Redirect unauthenticated users to login
Issue: When an unauthenticated user lands on the consent page, they see a static message: "You must be signed in to authorize an application." However, Better Auth may pass OAuth query parameters (like client_id, redirect_uri) that would be lost.
Why: If a user's session expires mid-flow or they navigate directly to /consent, they should be redirected to /login with the OAuth params preserved so the flow can resume after authentication.
Fix: Consider redirecting to login with the OAuth params:
if (!isAuthenticated) {
const loginUrl = `/login?${searchParams.toString()}`;
window.location.href = loginUrl;
return null;
}This mirrors the pattern in login/page.tsx where OAuth params are preserved through the authentication flow.
Refs:
- Use LayoutProps<'/consent'> to match codebase convention - Redirect unauthenticated users to /login with OAuth query params preserved so the flow resumes automatically after sign-in - Use window.location.assign() instead of href assignment to satisfy React Compiler Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low (delta only)
This is a delta review scoped to changes since the last automated review (391d34fa → bb3622604). The single commit addresses both "Consider" items from the prior review.
✅ Prior Feedback Addressed
Both suggestions from the previous review have been implemented correctly:
| Prior Suggestion | Resolution |
|---|---|
💭 Use LayoutProps<'/consent'> to match codebase pattern |
✅ layout.tsx now uses typed LayoutProps<'/consent'> instead of { children: ReactNode } |
| 💭 Redirect unauthenticated users to login with OAuth params preserved | ✅ page.tsx now uses useEffect to detect unauthenticated state and router.replace() to /login?${searchParams.toString()} |
Implementation quality:
- The redirect logic correctly waits for
isSessionLoadingto complete before checking auth state - OAuth query params are preserved through
searchParams.toString() - Loading state shows spinner while auth check and redirect are in progress
- Uses
router.replacefor the redirect (appropriate for this flow — no back-button to consent page)
🕐 Pending Recommendations (Prior Reviews)
The following issues from prior reviews remain applicable to the overall PR (not introduced in this delta):
- 🟠 JWT private keys stored in plaintext — Better Auth framework limitation, document as accepted risk
- 🟠 Multi-org tenantId claim uses non-deterministic org selection — Acknowledged in PR description as known limitation
- 🟠 No automated tests for OAuth functionality — Security-critical code should have test coverage
- 🟡 Missing unique constraint on refresh token —
oauth_refresh_token.tokenshould have.unique() - 🟡 Missing indexes on OAuth tables — FK columns lack indexes for efficient queries
💡 APPROVE WITH SUGGESTIONS
Summary: The delta cleanly addresses both prior "Consider" suggestions. The OAuth consent page now properly redirects unauthenticated users to login while preserving OAuth query params, and follows the codebase's typed layout pattern. The outstanding Major/Minor issues from earlier reviews (plaintext keys, multi-org claims, missing tests, missing indexes, refresh token unique constraint) remain applicable to the overall PR and should be tracked for follow-up or addressed before merge.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
0 | 0 | 0 | 0 | 0 | 5 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 5 | 0 |
Note: Delta review — sub-reviewers not dispatched for 2-file UI-only change that addresses prior feedback.
Summary
@better-auth/oauth-providerplugin withjwt()support, enabling the platform to act as an OAuth 2.1 authorization server/.well-known/openid-configuration,/.well-known/oauth-authorization-server/*)oauth_client,oauth_access_token,oauth_refresh_token,oauth_consent,jwks) with Drizzle relations and migrationclient_idin query params) and resume the flow after authenticationcustomAccessTokenClaimswithtenantIdandemailclaims for downstream OAuth clientsKnown limitation
customAccessTokenClaimsusesgetInitialOrganization(first org by membership date). Multi-org users will get their earliest org'stenantIdin the access token. A proper org selection flow (post-login org picker + token re-issuance) should be added when there's a real multi-org customer need.Test plan
/.well-known/openid-configurationreturns valid OIDC metadatatenantIdandemailclaimspnpm db:migrate)🤖 Generated with Claude Code