Merged
Conversation
…rity - facebook/exchange: add 15s fetch timeouts, AbortError→504, atomic user upsert - providers: require CLIENT_SECRET and CALLBACK_URL for github/facebook/twitter - twitter/exchange: lookup by account first, nullable email, emailVerified=false - use-google-one-tap: shared loadScript promise per src, try/finally for onCredential - last-magic-link-email-client: add Secure cookie in production
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds multi-provider OAuth (Google One Tap, Facebook, Twitter) and a Vercel-style passkey discovery flow. New backend endpoints, DB meta field, env vars, client hooks/components, Next.js callbacks, OpenAPI updates, tests, and icons to support provider availability, authorize/exchange flows, and passkey user resolution. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Browser Client
participant Next as Next.js App
participant Fastify as Fastify Backend
participant OAuthProvider as OAuth Provider
participant DB as Database
Client->>Next: Click "Login with Provider"
Next->>Fastify: GET /auth/oauth/{provider}/authorize-url
Fastify->>Fastify: validate config, generate state/PKCE
Fastify->>DB: store oauth_state (with hashed state & code_verifier)
Fastify-->>Next: { redirectUrl }
Next->>OAuthProvider: Redirect to provider authorize endpoint
OAuthProvider->>Client: Consent flow
OAuthProvider-->>Next: Redirect callback (code, state)
Next->>Fastify: POST /auth/oauth/{provider}/exchange { code, state }
Fastify->>DB: verify & consume oauth_state
Fastify->>OAuthProvider: Exchange code for tokens
OAuthProvider-->>Fastify: access_token
Fastify->>OAuthProvider: Fetch user info
Fastify->>DB: create/link user, store encrypted tokens, create session
Fastify-->>Next: { token, refreshToken }
Next->>Client: set cookies, redirect home
sequenceDiagram
participant Client as Browser Client
participant Next as Next.js App
participant Fastify as Fastify Backend
participant GoogleAPI as Google Identity API
participant DB as Database
Client->>Next: Load login page (One Tap ready)
Next->>GoogleAPI: prompt One Tap
GoogleAPI-->>Client: credential
Client->>Next: onCredential(credential)
Next->>Fastify: POST /auth/oauth/google/verify-id-token { credential }
Fastify->>GoogleAPI: verify ID token via OAuth2Client
GoogleAPI-->>Fastify: token payload (sub,email,name)
Fastify->>DB: create/link user, create session
Fastify-->>Next: { token, refreshToken }
Next->>Client: updateAuthTokens & navigate home
sequenceDiagram
participant Client as Browser Client
participant Next as Next.js App
participant Fastify as Fastify Backend
participant DB as Database
Client->>Next: startAuthentication({ useBrowserAutofill: true })
Next->>Fastify: POST /auth/passkey/resolve-user { userHandle }
Fastify->>DB: lookup user by passkey userHandle
Fastify-->>Next: { maskedIdentifier }
Next->>Client: render PasskeyShortcut "Login as [email]"
Client->>Client: user initiates passkey ceremony
Client->>Next: send assertion
Next->>Fastify: POST /auth/passkey/exchange { assertion }
Fastify->>DB: verify assertion, create session
Fastify-->>Next: { token, refreshToken }
Next->>Client: updateAuthTokens & navigate home
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 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: 15
🧹 Nitpick comments (6)
apps/next/app/auth/callback/oauth/twitter/route.ts (1)
34-34: Addreturnbeforeredirect()for TypeScript control flow clarity.While
redirect()throws and never returns, TypeScript's control flow analysis may not recognize this, potentially causing type inference issues. Adding an explicitreturnmakes the intent clearer and ensures TypeScript understands the function terminates.♻️ Suggested change
- if (!tokens) redirect(`/auth/login?message=${encodeURIComponent('oauth_failed')}`) + if (!tokens) return redirect(`/auth/login?message=${encodeURIComponent('oauth_failed')}`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/app/auth/callback/oauth/twitter/route.ts` at line 34, The if-check that calls redirect when tokens is falsy should explicitly return the redirect call to satisfy TypeScript control-flow (change the branch in the oauth callback where it currently does if (!tokens) redirect(...)): add a leading return before redirect(...) so the function clearly terminates when tokens is missing (reference the tokens variable and the redirect(...) call in the oauth callback/route handler).apps/next/app/auth/callback/oauth/facebook/route.ts (2)
1-44: Consider extracting shared OAuth callback logic.The Twitter and Facebook callback handlers share ~90% identical code. A shared utility could reduce duplication and ensure consistent behavior across OAuth providers.
♻️ Example shared utility approach
// lib/auth/oauth-callback.ts type OAuthExchangeFn = (params: { body: { code: string; state: string } }) => Promise<unknown> export function createOAuthCallbackHandler( provider: string, exchangeFn: (client: ReturnType<typeof createClient>) => OAuthExchangeFn, errorMap: Record<string, string> ) { return async function GET(request: Request) { const { searchParams } = new URL(request.url) const code = searchParams.get('code') const state = searchParams.get('state') if (!code || !state) { return redirect(`/auth/login?message=missing_params`) } try { const response = await exchangeFn(client)({ body: { code, state } }) const tokens = extractTokens(response) if (!tokens) return redirect(`/auth/login?message=oauth_failed`) const redirectResponse = NextResponse.redirect(new URL('/', request.url), 303) setAuthCookiesOnResponse(redirectResponse, tokens) return redirectResponse } catch (error) { unstable_rethrow(error) const rawMessage = error instanceof Error ? error.message : `${provider} sign-in failed` const errorCode = errorMap[rawMessage] ?? 'oauth_failed' redirect(`/auth/login?message=${encodeURIComponent(errorCode)}`) } } }Then each route becomes:
// facebook/route.ts export const GET = createOAuthCallbackHandler( 'Facebook', (client) => client.auth.oauth.facebook.exchange, { /* facebook error map */ } )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/app/auth/callback/oauth/facebook/route.ts` around lines 1 - 44, The Facebook GET handler and mapAuthError are almost identical to other OAuth callbacks; extract the common flow into a shared factory (e.g., createOAuthCallbackHandler) that accepts provider name, an exchange function (client => client.auth.oauth.<provider>.exchange) and an error map, then replace the current GET and mapAuthError with a call to that factory while still using extractTokens and setAuthCookiesOnResponse to set cookies and redirect; ensure the factory preserves behavior around missing code/state, token extraction, NextResponse.redirect(…, 303), unstable_rethrow(error) and mapping raw error messages to provider-specific error codes.
33-33: Addreturnbeforeredirect()for TypeScript control flow clarity.Same suggestion as the Twitter route - adding
returnmakes control flow explicit.♻️ Suggested change
- if (!tokens) redirect(`/auth/login?message=${encodeURIComponent('oauth_failed')}`) + if (!tokens) return redirect(`/auth/login?message=${encodeURIComponent('oauth_failed')}`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/app/auth/callback/oauth/facebook/route.ts` at line 33, The early-return is missing when handling a failed Facebook OAuth token exchange: in the Facebook OAuth callback route where you check `if (!tokens) redirect(\`/auth/login?message=\${encodeURIComponent('oauth_failed')}\`)`, change it to `return redirect(...)` so TypeScript understands the function exits after redirect; update the same check that uses the `tokens` variable and the `redirect()` call in that route handler.packages/react/src/hooks/use-passkey-auth.ts (1)
43-47: Consider validatingredirectUrlbefore navigation.While the
redirectUrlis server-generated, adding a client-side check provides defense-in-depth against open redirect vulnerabilities if the server response were ever compromised or manipulated.🛡️ Optional hardening
const redirectUrl = 'redirectUrl' in result ? result.redirectUrl : undefined if (redirectUrl) { + // Defense-in-depth: ensure redirect stays on same origin + const url = new URL(redirectUrl, window.location.origin) + if (url.origin !== window.location.origin) { + throw new Error('Invalid redirect URL') + } window.location.assign(redirectUrl) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/hooks/use-passkey-auth.ts` around lines 43 - 47, Validate the server-provided redirectUrl before calling window.location.assign to defend against open redirects: in the use-passkey-auth code where redirectUrl is derived from result, parse it with the URL constructor in a try/catch and allow navigation only if it's a same-origin URL (compare url.origin to window.location.origin) or an allowed relative path (startsWith("/")); if the check fails, do not call window.location.assign and instead handle the fallback (e.g., log or use a default safe route). Ensure you reference redirectUrl/result and the existing window.location.assign call when making the change.packages/react/src/hooks/use-oauth-login.ts (1)
19-41: Breaking change:mutate()now requires a provider argument.Previously,
useOAuthLogin().mutate()was called without arguments (GitHub-only). Now it requires a provider string:mutate('github'). Existing consumers will get a TypeScript error and need to update their call sites.Ensure this is communicated in release notes or migration docs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/hooks/use-oauth-login.ts` around lines 19 - 41, Update the release notes / migration docs to state that useOAuthLogin's returned mutate function now requires a provider argument (e.g., mutate('github')) and that previous no-arg calls (GitHub-only) will fail; reference the hook name useOAuthLogin and the mutate(...) call in the note and provide a short example of the new call signature plus a recommended migration step for consumers to replace plain mutate() with mutate('github') or to pass the appropriate provider string.apps/fastify/src/routes/auth/passkey/resolve-user.ts (1)
38-77: Consider user enumeration risk.The endpoint returns different error codes for "invalid encoding" (400) vs "user not found" (404). This distinction could allow attackers to enumerate valid user IDs by differentiating between malformed handles and non-existent users. Given the rate limiting (10/min), this risk is mitigated but worth noting.
If stricter protection is desired, consider returning a generic 400 for both cases after validation passes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/auth/passkey/resolve-user.ts` around lines 38 - 77, The route discloses user existence by returning 400 for malformed userHandle but 404 for missing users; to prevent enumeration change the post-validation "not found or no passkey" branch in the handler (the code that queries getDb and checks row?.email) to return the same generic 400 response used for invalid encoding (same code 'INVALID_USER_HANDLE' and a neutral message, or a single generic error) instead of 404 so both malformed and non-existent handles produce identical responses; keep all existing validation (trim, regex, base64url decode) and only normalize the reply sent from the user lookup branch (the reply.code(...) after the DB query).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/fastify/src/routes/auth/oauth/facebook/authorize-url.test.ts`:
- Around line 4-13: The test is flaky because it relies on ambient Facebook
OAuth env vars; explicitly control process.env and reload the module providing
the route before calling fastify.inject. In the test for 'GET
/auth/oauth/facebook/authorize-url' (authorize-url.test.ts) wrap the request in
a module-isolation block (e.g., jest.resetModules() / jest.isolateModules()) or
re-require the server/router after deleting or setting
process.env.FACEBOOK_CLIENT_ID / process.env.FACEBOOK_CLIENT_SECRET to undefined
(or to valid values for the success case), construct the fastify instance afresh
(the same fastify variable used in the test), call fastify.inject, then restore
the original envs; this ensures the route's config is evaluated from the
controlled env and makes the 503 assertion deterministic.
In `@apps/fastify/src/routes/auth/oauth/facebook/authorize-url.ts`:
- Around line 32-38: The current guard in the authorize URL handler only checks
facebookClientId and oauthFacebookCallbackUrl but not the
FACEBOOK_CLIENT_SECRET, allowing generation of an authorize URL the server
cannot complete; update the validation in the authorize-url handler (where
facebookClientId and oauthFacebookCallbackUrl are read from env) to also require
env.FACEBOOK_CLIENT_SECRET (or a named const like facebookClientSecret) and
return the same 503/OAUTH_NOT_CONFIGURED response when it is missing so the
authorize URL is only produced when the full Facebook config (ID, SECRET, and
callback URL) is present.
In `@apps/fastify/src/routes/auth/oauth/facebook/exchange.ts`:
- Around line 143-150: The catch block handling the Facebook user info fetch
currently only checks err.name === 'AbortError' for timeouts; update that check
to also detect the request-timeout shape used elsewhere (e.g., check err.name
=== 'AbortError' || (err as any).type === 'request-timeout') so the handler that
returns reply.code(504).send({ code: 'USER_INFO_FAILED', message: 'Failed to
fetch Facebook user (timeout)' }) triggers for both timeout error forms; modify
the catch in the exchange logic where the Facebook user info is fetched to
include this combined condition.
- Around line 104-111: The catch blocks that handle timeouts in the token
exchange and user info fetch currently check for err.name === 'AbortError' and
thus never match the TimeoutError thrown by AbortSignal.timeout(); update both
catch conditions (the one that returns reply.code(504).send({ code:
'TOKEN_EXCHANGE_FAILED', ... }) and the similar user-info fetch catch) to check
for err.name === 'TimeoutError' (or use err.name === 'TimeoutError' || err.name
=== 'AbortError' for broader coverage) so timeouts return the intended 504
response instead of being re-thrown.
In `@apps/fastify/src/routes/auth/oauth/google/verify-id-token.ts`:
- Around line 57-69: The handler currently trusts payload.email and sets
emailVerified: true before confirming the Google subject; change the logic in
the verify-id-token handler so you first use payload.sub as the canonical
identity to find an existing user linked to Google (check the linked social id
using payload.sub), only if no linked account exists then consider resolving by
email, and when adopting payload.email into the user record do not set
emailVerified to true unless payload.email_verified is truthy; update any code
paths that create or update the local user/session (references: payload,
payload.sub, payload.email, payload.email_verified, emailVerified) to enforce
this order and checks to prevent minting sessions for the wrong user or marking
unverified emails as verified.
In `@apps/fastify/src/routes/auth/oauth/twitter/exchange.ts`:
- Around line 108-145: The token exchange and user fetch calls (the fetch to
'https://api.x.com/2/oauth2/token' that produces tokenRes/tokenData/accessToken
and the fetch to 'https://api.x.com/2/users/me' that produces userRes) are
unbounded and not wrapped for rejections/timeouts; wrap each fetch in a
try/catch, use AbortController to enforce a reasonable timeout, and distinguish
error types: on timeout return a 504 (e.g., code 'UPSTREAM_TIMEOUT'), on
network/fetch rejection or non-ok upstream responses return a 502 (e.g., code
'UPSTREAM_SERVICE_ERROR') rather than a 400; also log the upstream
error/response details and keep the existing handling of tokenData.error and
missing accessToken after successful responses.
- Around line 158-238: Wrap the entire provider-account flow (the initial
db.select that produces existingAccount, the conditional users insert and
subsequent select, the account insert/update that uses encryptAccountTokens, and
the sessions.insert that uses generateJti/hashToken/randomUUID) inside a single
database transaction so those steps are atomic; perform the
existingAccount/select, user creation, account upsert (or try insert and on
conflict fetch existing) and sessions insert using the transaction-bound db
instance, roll back on any error and return the appropriate reply, and ensure
you re-query within the transaction when needed rather than relying on the outer
scope existingAccount/user variables.
- Around line 198-211: The update branch only writes encrypted tokens and
updatedAt causing OAuth metadata to drift; modify the
db.update(account).set(...) in the existingAccount branch to also persist
accessTokenExpiresAt, refreshTokenExpiresAt, and scope using the values from
accountData (e.g., accountData.accessTokenExpiresAt,
accountData.refreshTokenExpiresAt, accountData.scope) so the row is fully
refreshed when encryptAccountTokens is used and existingAccount is present.
In `@apps/next/app/auth/login/login-actions.tsx`:
- Line 149: The passkey shortcut dismissal should be scoped to the discovered
email and only shown when WebAuthn is available: replace the single boolean
optedOut with a per-email check (e.g., store opted-out email id/key or a map
keyed by discoveredEmail) so flipping the opt-out only hides the shortcut for
that specific discoveredEmail, and wrap the shortcut rendering and opt-out logic
behind useWebAuthnAvailable() so the shortcut is not rendered when WebAuthn
isn't supported; update any related logic in the same component (the places
referencing optedOut and the passkey shortcut in the login-actions component) to
use the new per-email gating.
- Around line 160-166: The Google One Tap hook is initialized and used
regardless of provider availability; wire its use to the OAuth provider map and
the hook's readiness flag. Change the code around useGoogleOneTap() and
useOAuthProviders() so you only call or expose promptGoogle and isGooglePending
when google is actually configured (useOAuthProviders().google) and the hook's
isReady is true; ensure UI click handlers and the Google button check both
isGoogleConfigured and the hook's isReady before invoking promptGoogle so the
button is disabled/no-op until GIS is ready. Apply the same change pattern to
the other occurrence referenced (lines ~208-222) so both prompt usage and button
enablement depend on isGoogleConfigured && isReady.
In `@apps/next/app/auth/login/use-google-one-tap.ts`:
- Around line 34-63: The cached promise in loadPromises can be a rejected
promise and will block future loads; modify loadScript so any promise that
rejects removes its entry from loadPromises before propagating the error: after
creating the initial promise for both the existing-element and new-script
branches, wrap it with .catch(err => { loadPromises.delete(src); throw err })
(or equivalent) and store that wrapped promise in loadPromises; keep existing
early-return behavior for already-resolved cases but ensure any new promise is
wrapped so transient failures are cleared and subsequent loadScript(src) calls
can retry.
In `@apps/next/components/icons/google.tsx`:
- Around line 3-6: The SVG component hardcodes width and height after spreading
props which clobbers caller-supplied sizes; update the component signature to
destructure width and height with defaults (e.g. width = 24, height = 24) from
the incoming SVGProps and then use those width/height values on the <svg>
element while spreading the remaining props (rest) so callers can override
sizes; apply the same change to the sibling Facebook and Twitter components
(refer to the Google function/component name to locate the code).
In `@apps/next/lib/auth/last-magic-link-email-client.ts`:
- Around line 9-16: The code stores the user's email for a year in a cookie
(setLastMagicLinkEmailCookie), which leaks PII on every request; change this to
store the prefill value client-side only (e.g., localStorage or sessionStorage)
instead of document.cookie: detect client runtime (typeof window !==
'undefined'), encode the email as before, and call
localStorage.setItem(lastMagicLinkEmailCookieName, value) (or sessionStorage for
shorter lifetime) and remove the cookie assignment so the email is not sent with
requests; ensure any corresponding read logic uses localStorage/sessionStorage
instead of cookie.
In `@packages/react/src/hooks/use-oauth-providers.ts`:
- Around line 10-20: The hook currently only returns provider booleans based on
data, collapsing failures into "not configured"; update the hook
(use-oauth-providers.ts) to surface the react-query state from useQuery
(useQuery call with queryKey and queryFn) by returning isPending (map from
isLoading), isError (map from isError) and optionally error alongside the
existing github/google/facebook/twitter booleans so callers can distinguish
loading/error vs. missing configuration; follow the pattern used in
use-passkey-discovery.ts and use-user.ts when naming and returning these flags.
In `@packages/react/src/hooks/use-passkey-discovery.ts`:
- Around line 21-43: The current code swallows all errors and returns { email:
null } for both user-cancel and real request failures; change the error handling
so only the user-cancel path maps to { email: null } while network/server errors
bubble. In practice, in the startAuthentication try/catch around
startAuthentication(...) (and where you call client.auth.passkey.start), only
catch the specific user-cancel error (e.g., NotAllowedError/DOMException or the
library's user cancelled signal) and return { email: null } for that case,
otherwise rethrow the error; do not blanket-catch and swallow other exceptions.
Likewise, remove or narrow the catch around client.auth.passkey.resolveUser (the
call in client.auth.passkey.resolveUser({ body: { userHandle }, throwOnError:
true })) so that actual request failures are not converted to { email: null }
but instead propagate as errors.
---
Nitpick comments:
In `@apps/fastify/src/routes/auth/passkey/resolve-user.ts`:
- Around line 38-77: The route discloses user existence by returning 400 for
malformed userHandle but 404 for missing users; to prevent enumeration change
the post-validation "not found or no passkey" branch in the handler (the code
that queries getDb and checks row?.email) to return the same generic 400
response used for invalid encoding (same code 'INVALID_USER_HANDLE' and a
neutral message, or a single generic error) instead of 404 so both malformed and
non-existent handles produce identical responses; keep all existing validation
(trim, regex, base64url decode) and only normalize the reply sent from the user
lookup branch (the reply.code(...) after the DB query).
In `@apps/next/app/auth/callback/oauth/facebook/route.ts`:
- Around line 1-44: The Facebook GET handler and mapAuthError are almost
identical to other OAuth callbacks; extract the common flow into a shared
factory (e.g., createOAuthCallbackHandler) that accepts provider name, an
exchange function (client => client.auth.oauth.<provider>.exchange) and an error
map, then replace the current GET and mapAuthError with a call to that factory
while still using extractTokens and setAuthCookiesOnResponse to set cookies and
redirect; ensure the factory preserves behavior around missing code/state, token
extraction, NextResponse.redirect(…, 303), unstable_rethrow(error) and mapping
raw error messages to provider-specific error codes.
- Line 33: The early-return is missing when handling a failed Facebook OAuth
token exchange: in the Facebook OAuth callback route where you check `if
(!tokens)
redirect(\`/auth/login?message=\${encodeURIComponent('oauth_failed')}\`)`,
change it to `return redirect(...)` so TypeScript understands the function exits
after redirect; update the same check that uses the `tokens` variable and the
`redirect()` call in that route handler.
In `@apps/next/app/auth/callback/oauth/twitter/route.ts`:
- Line 34: The if-check that calls redirect when tokens is falsy should
explicitly return the redirect call to satisfy TypeScript control-flow (change
the branch in the oauth callback where it currently does if (!tokens)
redirect(...)): add a leading return before redirect(...) so the function
clearly terminates when tokens is missing (reference the tokens variable and the
redirect(...) call in the oauth callback/route handler).
In `@packages/react/src/hooks/use-oauth-login.ts`:
- Around line 19-41: Update the release notes / migration docs to state that
useOAuthLogin's returned mutate function now requires a provider argument (e.g.,
mutate('github')) and that previous no-arg calls (GitHub-only) will fail;
reference the hook name useOAuthLogin and the mutate(...) call in the note and
provide a short example of the new call signature plus a recommended migration
step for consumers to replace plain mutate() with mutate('github') or to pass
the appropriate provider string.
In `@packages/react/src/hooks/use-passkey-auth.ts`:
- Around line 43-47: Validate the server-provided redirectUrl before calling
window.location.assign to defend against open redirects: in the use-passkey-auth
code where redirectUrl is derived from result, parse it with the URL constructor
in a try/catch and allow navigation only if it's a same-origin URL (compare
url.origin to window.location.origin) or an allowed relative path
(startsWith("/")); if the check fails, do not call window.location.assign and
instead handle the fallback (e.g., log or use a default safe route). Ensure you
reference redirectUrl/result and the existing window.location.assign call when
making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5a7cfe4c-fb04-43e5-a87d-07594d979c45
⛔ Files ignored due to path filters (10)
apps/fastify/.env-sampleis excluded by!**/.env*apps/fastify/src/db/migrations/0011_keen_eternals.sqlis excluded by!apps/fastify/src/db/migrations/**apps/fastify/src/db/migrations/meta/0011_snapshot.jsonis excluded by!apps/fastify/src/db/migrations/**apps/fastify/src/db/migrations/meta/_journal.jsonis excluded by!apps/fastify/src/db/migrations/**packages/core/src/api-client.gen.tsis excluded by!**/*.gen.tspackages/core/src/api-wrapper.gen.tsis excluded by!**/*.gen.tspackages/core/src/gen/index.tsis excluded by!**/gen/**,!**/gen/**packages/core/src/gen/sdk.gen.tsis excluded by!**/gen/**,!**/gen/**,!**/*.gen.tspackages/core/src/gen/types.gen.tsis excluded by!**/gen/**,!**/gen/**,!**/*.gen.tspnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/pnpm-lock.yaml
📒 Files selected for processing (38)
apps/docu/content/docs/architecture/authentication.mdxapps/fastify/openapi/openapi.jsonapps/fastify/package.jsonapps/fastify/src/db/schema/tables/verification.tsapps/fastify/src/lib/env.tsapps/fastify/src/routes/auth/oauth.spec.tsapps/fastify/src/routes/auth/oauth/facebook/authorize-url.test.tsapps/fastify/src/routes/auth/oauth/facebook/authorize-url.tsapps/fastify/src/routes/auth/oauth/facebook/exchange.test.tsapps/fastify/src/routes/auth/oauth/facebook/exchange.tsapps/fastify/src/routes/auth/oauth/google/verify-id-token.test.tsapps/fastify/src/routes/auth/oauth/google/verify-id-token.tsapps/fastify/src/routes/auth/oauth/providers.test.tsapps/fastify/src/routes/auth/oauth/providers.tsapps/fastify/src/routes/auth/oauth/twitter/authorize-url.test.tsapps/fastify/src/routes/auth/oauth/twitter/authorize-url.tsapps/fastify/src/routes/auth/oauth/twitter/exchange.test.tsapps/fastify/src/routes/auth/oauth/twitter/exchange.tsapps/fastify/src/routes/auth/passkey/resolve-user.tsapps/next/app/auth/callback/oauth/facebook/route.tsapps/next/app/auth/callback/oauth/twitter/route.tsapps/next/app/auth/login/login-actions.tsxapps/next/app/auth/login/passkey-shortcut.tsxapps/next/app/auth/login/use-google-one-tap.tsapps/next/app/providers.tsxapps/next/components/icons/facebook.tsxapps/next/components/icons/github.tsxapps/next/components/icons/google.tsxapps/next/components/icons/index.tsapps/next/components/icons/passkey.tsxapps/next/components/icons/twitter.tsxapps/next/lib/auth/last-magic-link-email-client.tsapps/next/lib/env.tspackages/react/src/hooks/use-oauth-login.tspackages/react/src/hooks/use-oauth-providers.tspackages/react/src/hooks/use-passkey-auth.tspackages/react/src/hooks/use-passkey-discovery.tspackages/react/src/index.ts
- Facebook: add FACEBOOK_CLIENT_SECRET to authorize-url guard, fix flaky test with vi.mock, handle TimeoutError in exchange - Google: use payload.sub first for identity, emailVerified from payload.email_verified - Twitter: add fetch timeouts (504/502), wrap provider-account flow in transaction, persist account metadata on update - Passkey: return 400 instead of 404 in resolve-user (anti-enumeration) - OAuth callbacks: return redirect() for control flow - Login: per-email passkey opt-out, Google One Tap readiness, loadPromises retry on reject - Magic link: store prefill in localStorage instead of cookie (PII), useSyncExternalStore for prefill - Icons: allow width/height override via props - Hooks: useOAuthProviders returns isPending/isError/error, usePasskeyDiscovery selective error handling, usePasskeyAuth redirect validation, useOAuthLogin JSDoc
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
apps/fastify/src/routes/auth/oauth/google/verify-id-token.ts (1)
84-89:⚠️ Potential issue | 🔴 CriticalDon’t use
subpath is exhausted and verified.This still rejects missing-email tokens before checking the existing
(providerId, sub)link, and it still falls back tousers.emaileven whenemail_verifiedis false. That can both lock out an already-linked Google user and mint a session for a local account via an unverified Google address.As per coding guidelines, "SECURITY: Ensure server-side validation (never trust client-side validation alone)."Suggested fix
const accountId = payload.sub - const email = payload.email ?? '' + const email = payload.email const name = payload.name ?? payload.email ?? 'Google user' - const emailVerified = !!payload.email_verified - - if (!email) - return reply.code(400).send({ - code: 'EMAIL_REQUIRED', - message: 'Could not retrieve email from Google', - }) + const emailVerified = payload.email_verified === true const db = await getDb() const [existingAccount] = await db .select() .from(account) .where(and(eq(account.providerId, 'google'), eq(account.accountId, accountId))) let user: typeof users.$inferSelect | undefined if (existingAccount) { ;[user] = await db.select().from(users).where(eq(users.id, existingAccount.userId)) } if (!user) { + if (!email || !emailVerified) + return reply.code(400).send({ + code: 'EMAIL_REQUIRED', + message: 'Google account must provide a verified email', + }) const [byEmail] = await db.select().from(users).where(eq(users.email, email)) if (byEmail) user = byEmail } if (!user) { const userId = randomUUID() await db.insert(users).values({ id: userId, email, emailVerified, name, })Also applies to: 95-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/auth/oauth/google/verify-id-token.ts` around lines 84 - 89, The current logic uses email too early and falls back to users.email when email_verified is false; instead first locate a user by the Google providerId/sub pair (use accountId or payload.sub and providerId) and only if that lookup fails consider email-based flows. When falling back to email, require payload.email_verified === true before using payload.email or using users.email, and avoid setting name from email when email isn't verified; update the code around accountId, email, name, and emailVerified to: (1) try findByProvider(providerId, accountId) first, (2) if not found and payload.email_verified true then find or create by email, (3) otherwise reject the token, ensuring no unverified email is used to link or create accounts.apps/next/app/auth/login/use-google-one-tap.ts (1)
36-67:⚠️ Potential issue | 🟠 MajorRetry still hangs after the first GIS load failure.
When the initial load rejects, the map entry is cleared but the failed
<script>tag stays in the DOM. The next call falls into theelbranch and waits onload/errorevents that have already fired, so Google login is still stuck until refresh.Suggested fix
function loadScript(src: string): Promise<void> { const existing = loadPromises.get(src) if (existing) return existing - const el = document.querySelector<HTMLScriptElement>(`script[src="${src}"]`) - const rawPromise = el - ? new Promise<void>((resolve, reject) => { - if (window.google?.accounts?.id) { - resolve() - return - } - el.addEventListener('load', () => resolve(), { once: true }) - el.addEventListener( - 'error', - () => reject(new Error('Failed to load Google Identity Services')), - { once: true }, - ) - }) - : new Promise<void>((resolve, reject) => { - const script = document.createElement('script') - script.src = src - script.async = true - script.onload = () => resolve() - script.onerror = () => reject(new Error('Failed to load Google Identity Services')) - document.head.appendChild(script) - }) + if (window.google?.accounts?.id) return Promise.resolve() + + const stale = document.querySelector<HTMLScriptElement>(`script[src="${src}"]`) + if (stale) stale.remove() + + const rawPromise = new Promise<void>((resolve, reject) => { + const script = document.createElement('script') + script.src = src + script.async = true + script.onload = () => resolve() + script.onerror = () => reject(new Error('Failed to load Google Identity Services')) + document.head.appendChild(script) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/app/auth/login/use-google-one-tap.ts` around lines 36 - 67, The retry hangs because a failed <script> element remains in the DOM and subsequent calls find that element (el) and wait for load/error events that already fired; update loadScript to remove the failing script element on error so retries recreate it: in the el branch and in the created script branch, ensure the error handlers remove the script element from document (el.remove() or script.remove()) and clear the loadPromises entry (same key) before rejecting; keep the existing window.google?.accounts?.id check and the rawPromise.catch deletion but add element removal inside the error callbacks so failed tags don't block future calls in loadScript and loadPromises.
🧹 Nitpick comments (3)
apps/fastify/eslint.config.mjs (1)
5-7: Avoid disablingmax-linesentirely for this route.Turning the rule fully off for
src/routes/auth/oauth/twitter/exchange.tsremoves the guardrail permanently on a security-sensitive handler. Prefer either extracting the exchange flow into helpers/services or relaxing the limit to a higher threshold for that file instead of disabling it outright.♻️ Suggested config change
{ files: ['src/routes/auth/oauth/twitter/exchange.ts'], - rules: { 'max-lines': 'off' }, + rules: { + 'max-lines': ['error', { max: 300, skipBlankLines: true, skipComments: true }], + }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/eslint.config.mjs` around lines 5 - 7, Don't disable the 'max-lines' rule for the Twitter OAuth exchange route; either extract parts of the exchange flow into helper functions/services (e.g., move logic into an exchangeService or helpers called from the route handler) or relax the rule instead of turning it off by changing the override's rules entry from 'max-lines': 'off' to a higher threshold like 'max-lines': ['error', { max: 400, skipBlankLines: true, skipComments: true }]; update the override that targets the Twitter OAuth exchange route (the files override and its rules key) accordingly.apps/next/app/auth/callback/oauth/facebook/route.ts (1)
33-42: PreferNextResponse.redirecthere and dropunstable_rethrow.
redirect()inside thistryblock is the only reason the handler needsunstable_rethrow. Next.js still documentsunstable_rethrowas unstable and not recommended for production, so the null-token and error branches can stay on stable APIs by returningNextResponse.redirect(...)instead. (nextjs.org)♻️ Suggested simplification
-import { redirect, unstable_rethrow } from 'next/navigation' +import { redirect } from 'next/navigation' @@ - if (!tokens) return redirect(`/auth/login?message=${encodeURIComponent('oauth_failed')}`) + if (!tokens) { + return NextResponse.redirect( + new URL(`/auth/login?message=${encodeURIComponent('oauth_failed')}`, request.url), + 303, + ) + } @@ } catch (error) { - unstable_rethrow(error) const rawMessage = error instanceof Error ? error.message : 'Facebook sign-in failed' const errorCode = mapAuthError(rawMessage) - redirect(`/auth/login?message=${encodeURIComponent(errorCode)}`) + return NextResponse.redirect( + new URL(`/auth/login?message=${encodeURIComponent(errorCode)}`, request.url), + 303, + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/app/auth/callback/oauth/facebook/route.ts` around lines 33 - 42, Remove the unstable_rethrow call and return a NextResponse.redirect in the catch branch instead of calling redirect(); specifically, inside the catch block replace the unstable_rethrow(error) + redirect(`/auth/login?message=${encodeURIComponent(errorCode)}`) pattern with a returned NextResponse.redirect(new URL(`/auth/login?message=${encodeURIComponent(errorCode)}`, request.url), 303). Keep the existing logic that computes rawMessage and errorCode via mapAuthError and preserve setAuthCookiesOnResponse usage in the success path; ensure the handler consistently returns NextResponse.redirect objects (same status 303) for both the null-token and error branches.apps/next/app/auth/login/login-actions.tsx (1)
30-60: Consider extracting button props type for maintainability.The
OAuthButtonscomponent has 15 props. While acceptable, extracting the prop type would improve readability and enable reuse if needed elsewhere.♻️ Suggested improvement
+type OAuthButtonsProps = { + anyPending: boolean + setLastAuthMethod: (m: 'oauth' | 'passkey') => void + startOAuthLogin: (p: 'github' | 'facebook' | 'twitter') => void + promptGoogle: () => void + isGithubConfigured: boolean + isGoogleConfigured: boolean + isGoogleReady: boolean + isFacebookConfigured: boolean + isTwitterConfigured: boolean + isOAuthPending: boolean + isGooglePending: boolean + webauthnAvailable: boolean + startPasskeyAuth: (opts: { callbackUrl: string }) => void + isPasskeyPending: boolean +} + function OAuthButtons({ anyPending, setLastAuthMethod, ... -}: { - anyPending: boolean - ... -}) { +}: OAuthButtonsProps) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/app/auth/login/login-actions.tsx` around lines 30 - 60, Extract the inline props annotation for the OAuthButtons function into a named type (e.g., OAuthButtonsProps or IOAuthButtonsProps) and use that type in the function signature; update the declaration "function OAuthButtons({...}: { ... })" to "function OAuthButtons(props: OAuthButtonsProps)" (or keep destructuring but annotate as "function OAuthButtons({ ... }: OAuthButtonsProps)") so the 15 prop fields (anyPending, setLastAuthMethod, startOAuthLogin, promptGoogle, isGithubConfigured, isGoogleConfigured, isGoogleReady, isFacebookConfigured, isTwitterConfigured, isOAuthPending, isGooglePending, webauthnAvailable, startPasskeyAuth, isPasskeyPending) are defined once and reusable for maintainability and reuse.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/fastify/src/routes/auth/passkey/resolve-user.ts`:
- Around line 28-30: The route currently exposes users.email from the public,
unauthenticated resolve-user handler (the route using ResolveUserBodySchema with
security: []), which makes it a PII lookup; change the handler so it does NOT
return users.email directly — either return a masked identifier (implement a
small maskEmail(email) helper and return masked value or partial username) or
return only a minimal existence token/boolean and defer revealing the full email
until after passkey assertion verification. Update the response shape in the
resolve-user handler and the related response code paths (also in the similar
block around lines 61-75) to use the maskedIdentifier or existence-only response
instead of users.email, keeping ResolveUserBodySchema consumers in sync.
- Around line 21-34: The route with operationId "authPasskeyResolveUser" has
rateLimit configured but its schema responses only list 200 and 400; update the
schema to include a 429 response that matches `@fastify/rate-limit`'s actual
payload (fields: error, message, retryAfter) rather than the existing
ErrorResponseSchema shape; add or reference a new RateLimitResponseSchema (or
inline response schema) and include it under response.429 so the generated
OpenAPI accurately reflects the runtime behavior for the
config.rateLimit-enabled handler.
In `@apps/next/app/auth/callback/oauth/facebook/route.ts`:
- Around line 10-20: The mapAuthError function is returning error codes that
collide with GitHub-specific messages; update mapAuthError to emit
Facebook-specific codes (e.g., replace 'token_exchange_failed',
'fetch_user_failed', 'email_required', 'oauth_failed' with provider-specific
values like 'facebook_token_exchange_failed', 'facebook_fetch_user_failed',
'facebook_email_required', 'facebook_oauth_failed' or choose provider-neutral
names) so Facebook failures don't render GitHub remediation; modify the known
mapping inside mapAuthError to use those new keys and ensure the fallback value
is similarly provider-specific (e.g., 'facebook_oauth_failed') so the route
returns Facebook-specific error codes.
In `@apps/next/lib/auth/last-magic-link-email-client.ts`:
- Around line 22-29: getSnapshot() currently calls
localStorage.getItem(lastMagicLinkEmailStorageKey) outside the try/catch so
synchronous errors from restricted browser contexts can bubble up; wrap the
localStorage.getItem call (and the subsequent decodeURIComponent) in a try/catch
inside getSnapshot() and return undefined on any exception so
useSyncExternalStore-based renders won't crash; reference getSnapshot and
lastMagicLinkEmailStorageKey when making the change.
In `@packages/react/src/components/login-form.tsx`:
- Around line 44-49: LoginForm seeds the email state from the defaultEmail prop
only on first render, so when defaultEmail arrives after hydration it isn't
applied; add a useEffect in the LoginForm component (same pattern used for
initialError) that watches defaultEmail and calls setEmail(defaultEmail ?? '')
when defaultEmail changes (or when it becomes defined) to sync prop updates into
the email state without altering other logic; reference the email state/setEmail
and defaultEmail prop and mirror the initialError useEffect approach.
In `@packages/react/src/hooks/use-oauth-login.ts`:
- Around line 42-45: The mutation currently returns the endpoint response even
when data.redirectUrl is missing or not a valid string, causing a silent no-op;
update the code in use-oauth-login.ts (inside the function that calls
endpoints[provider]()) to validate that data?.redirectUrl is a non-empty string
and a valid URL before setting window.location.href, and if validation fails
throw a descriptive Error (e.g., "OAuth redirectUrl missing or invalid for
provider: ${provider}") so the caller/mutation receives a rejection instead of
silently succeeding.
In `@packages/react/src/hooks/use-passkey-auth.ts`:
- Around line 63-66: The onSuccess handler in use-passkey-auth.ts currently
invalidates auth queries before calling the caller's token-installing callback,
which can cause a stale unauthenticated fetch; change the order so that
variables.onSuccess?.(data) runs first (and only if data exists) and then call
queryClient.invalidateQueries for ['auth','session','user'] and
['auth','session','jwt'] afterwards, ensuring the caller persists tokens before
the auth queries are refetched.
- Around line 43-53: The redirect handling currently compares an absolute
redirect's origin to window.location.origin; instead validate against the
callback origin returned in the response (use the callbackUrl/callback origin
provided on result) so legitimate cross-origin callbacks are allowed: if result
contains a callbackUrl (or callbackOrigin) derive its origin via new
URL(result.callbackUrl).origin and compare url.origin to that value (fall back
to window.location.origin only if no callbackUrl/callbackOrigin is present),
then assign window.location to redirectUrl or '/' on parse failure as before;
update the check in the redirectUrl branch in use-passkey-auth.ts accordingly.
---
Duplicate comments:
In `@apps/fastify/src/routes/auth/oauth/google/verify-id-token.ts`:
- Around line 84-89: The current logic uses email too early and falls back to
users.email when email_verified is false; instead first locate a user by the
Google providerId/sub pair (use accountId or payload.sub and providerId) and
only if that lookup fails consider email-based flows. When falling back to
email, require payload.email_verified === true before using payload.email or
using users.email, and avoid setting name from email when email isn't verified;
update the code around accountId, email, name, and emailVerified to: (1) try
findByProvider(providerId, accountId) first, (2) if not found and
payload.email_verified true then find or create by email, (3) otherwise reject
the token, ensuring no unverified email is used to link or create accounts.
In `@apps/next/app/auth/login/use-google-one-tap.ts`:
- Around line 36-67: The retry hangs because a failed <script> element remains
in the DOM and subsequent calls find that element (el) and wait for load/error
events that already fired; update loadScript to remove the failing script
element on error so retries recreate it: in the el branch and in the created
script branch, ensure the error handlers remove the script element from document
(el.remove() or script.remove()) and clear the loadPromises entry (same key)
before rejecting; keep the existing window.google?.accounts?.id check and the
rawPromise.catch deletion but add element removal inside the error callbacks so
failed tags don't block future calls in loadScript and loadPromises.
---
Nitpick comments:
In `@apps/fastify/eslint.config.mjs`:
- Around line 5-7: Don't disable the 'max-lines' rule for the Twitter OAuth
exchange route; either extract parts of the exchange flow into helper
functions/services (e.g., move logic into an exchangeService or helpers called
from the route handler) or relax the rule instead of turning it off by changing
the override's rules entry from 'max-lines': 'off' to a higher threshold like
'max-lines': ['error', { max: 400, skipBlankLines: true, skipComments: true }];
update the override that targets the Twitter OAuth exchange route (the files
override and its rules key) accordingly.
In `@apps/next/app/auth/callback/oauth/facebook/route.ts`:
- Around line 33-42: Remove the unstable_rethrow call and return a
NextResponse.redirect in the catch branch instead of calling redirect();
specifically, inside the catch block replace the unstable_rethrow(error) +
redirect(`/auth/login?message=${encodeURIComponent(errorCode)}`) pattern with a
returned NextResponse.redirect(new
URL(`/auth/login?message=${encodeURIComponent(errorCode)}`, request.url), 303).
Keep the existing logic that computes rawMessage and errorCode via mapAuthError
and preserve setAuthCookiesOnResponse usage in the success path; ensure the
handler consistently returns NextResponse.redirect objects (same status 303) for
both the null-token and error branches.
In `@apps/next/app/auth/login/login-actions.tsx`:
- Around line 30-60: Extract the inline props annotation for the OAuthButtons
function into a named type (e.g., OAuthButtonsProps or IOAuthButtonsProps) and
use that type in the function signature; update the declaration "function
OAuthButtons({...}: { ... })" to "function OAuthButtons(props:
OAuthButtonsProps)" (or keep destructuring but annotate as "function
OAuthButtons({ ... }: OAuthButtonsProps)") so the 15 prop fields (anyPending,
setLastAuthMethod, startOAuthLogin, promptGoogle, isGithubConfigured,
isGoogleConfigured, isGoogleReady, isFacebookConfigured, isTwitterConfigured,
isOAuthPending, isGooglePending, webauthnAvailable, startPasskeyAuth,
isPasskeyPending) are defined once and reusable for maintainability and reuse.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 61874189-597a-476a-b78e-090593f5a454
⛔ Files ignored due to path filters (1)
packages/core/src/gen/types.gen.tsis excluded by!**/gen/**,!**/gen/**,!**/*.gen.ts
📒 Files selected for processing (23)
apps/fastify/eslint.config.mjsapps/fastify/openapi/openapi.jsonapps/fastify/src/routes/auth/oauth/facebook/authorize-url.test.tsapps/fastify/src/routes/auth/oauth/facebook/authorize-url.tsapps/fastify/src/routes/auth/oauth/facebook/exchange.tsapps/fastify/src/routes/auth/oauth/google/verify-id-token.tsapps/fastify/src/routes/auth/oauth/twitter/exchange.tsapps/fastify/src/routes/auth/passkey/resolve-user.tsapps/next/app/auth/callback/oauth/facebook/route.tsapps/next/app/auth/callback/oauth/twitter/route.tsapps/next/app/auth/login/login-actions.tsxapps/next/app/auth/login/use-google-one-tap.tsapps/next/components/icons/facebook.tsxapps/next/components/icons/google.tsxapps/next/components/icons/twitter.tsxapps/next/lib/auth/last-magic-link-email-client.tsapps/next/lib/auth/last-magic-link-email.tspackages/react/src/components/login-form.spec.tsxpackages/react/src/components/login-form.tsxpackages/react/src/hooks/use-oauth-login.tspackages/react/src/hooks/use-oauth-providers.tspackages/react/src/hooks/use-passkey-auth.tspackages/react/src/hooks/use-passkey-discovery.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/react/src/hooks/use-oauth-providers.ts
- packages/react/src/hooks/use-passkey-discovery.ts
- apps/next/components/icons/facebook.tsx
- apps/fastify/src/routes/auth/oauth/facebook/authorize-url.ts
- apps/fastify/src/routes/auth/oauth/twitter/exchange.ts
- apps/next/app/auth/callback/oauth/twitter/route.ts
- apps/fastify/src/routes/auth/oauth/facebook/authorize-url.test.ts
- apps/next/components/icons/twitter.tsx
- apps/fastify/src/routes/auth/oauth/facebook/exchange.ts
- Passkey resolve-user: mask email (PII), add 429 rate limit schema - Facebook OAuth: provider-specific error codes, NextResponse.redirect - GitHub OAuth: NextResponse.redirect instead of unstable_rethrow - use-oauth-login: validate redirectUrl, throw on invalid - use-passkey-auth: onSuccess before invalidation, trust server redirect - Google verify-id-token: require email_verified for email flow - last-magic-link-email-client: wrap localStorage in try/catch - LoginForm: sync defaultEmail prop after hydration - use-google-one-tap: remove failed script on error for retry - ESLint: max-lines threshold for Twitter exchange - login-actions: extract OAuthButtonsProps type
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
packages/react/src/hooks/use-passkey-auth.ts (2)
62-65:⚠️ Potential issue | 🟠 MajorAwait the token-install callback before invalidating auth queries.
Line 63 still has the same race in practice: the current caller passes an
asynconSuccess(apps/next/app/auth/login/login-actions.tsx:207-214), but this hook doesn't await it, so Lines 64-65 can refetch against the old anonymous session beforeupdateAuthTokens()finishes. Make the callbackvoid | Promise<void>and await it here before invalidating.Suggested fix
export type UsePasskeyAuthParams = { /** Absolute callback URL for redirect flow. When absent, returns tokens directly. */ callbackUrl?: string /** Called with tokens when callbackUrl is absent (direct token flow) */ - onSuccess?: (data: { token: string; refreshToken: string }) => void + onSuccess?: (data: { token: string; refreshToken: string }) => void | Promise<void> } @@ - onSuccess: (data, variables) => { - if (data) variables.onSuccess?.(data) + onSuccess: async (data, variables) => { + if (data) await variables.onSuccess?.(data) queryClient.invalidateQueries({ queryKey: ['auth', 'session', 'user'] }) queryClient.invalidateQueries({ queryKey: ['auth', 'session', 'jwt'] }) },Does TanStack Query v5 await a Promise returned from useMutation's onSuccess callback?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/hooks/use-passkey-auth.ts` around lines 62 - 65, The onSuccess handler in usePasskeyAuth should await the caller's async callback to avoid a race: change the callback type to return void | Promise<void> (update any types / interfaces used by usePasskeyAuth and the mutation options) and call await variables.onSuccess?.(data) inside the onSuccess handler before calling queryClient.invalidateQueries (i.e., in the mutation's onSuccess block in usePasskeyAuth, await variables.onSuccess?.(data) first, then invalidate ['auth','session','user'] and ['auth','session','jwt']).
43-52:⚠️ Potential issue | 🔴 CriticalRestrict redirects to the requested callback origin and safe schemes.
Line 48 only checks that
redirectUrlparses, so any absolute URL is accepted. That still reintroduces the bug from the earlier review: a server-returned redirect to the wrong origin will navigate, and a non-HTTP(S) scheme can slip through as well. Since the backend derivesredirectUrlfrom the submittedcallbackUrl(apps/fastify/src/routes/auth/passkey/verify.ts:95-125), validate the returned URL againstnew URL(callbackUrl).originand requirehttp:/https:before callingwindow.location.assign.Suggested fix
const redirectUrl = 'redirectUrl' in result ? result.redirectUrl : undefined if (redirectUrl) { - if (redirectUrl.startsWith('/')) window.location.assign(redirectUrl) - else - try { - new URL(redirectUrl) - window.location.assign(redirectUrl) - } catch { - window.location.assign('/') - } + try { + const url = new URL(redirectUrl, window.location.origin) + const expectedOrigin = callbackUrl + ? new URL(callbackUrl).origin + : window.location.origin + const isSafeProtocol = url.protocol === 'http:' || url.protocol === 'https:' + + if (!isSafeProtocol || url.origin !== expectedOrigin) { + throw new Error('Unexpected redirect target') + } + + window.location.assign(url.toString()) + } catch { + window.location.assign('/') + } return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/hooks/use-passkey-auth.ts` around lines 43 - 52, The redirect handling accepts any absolute URL; restrict redirects to only same-origin callbacks and safe schemes: when resolving redirectUrl (variable redirectUrl from result) allow relative paths (startsWith('/')) as before, but for absolute URLs parse both redirectUrl and the original callbackUrl (from result.callbackUrl or wherever submitted) and require new URL(redirectUrl).origin === new URL(callbackUrl).origin and that new URL(redirectUrl).protocol is 'http:' or 'https:' before calling window.location.assign; otherwise fall back to window.location.assign('/') to block unsafe or cross-origin schemes. Ensure you reference redirectUrl and callbackUrl in the change in use-passkey-auth.ts.apps/next/app/auth/login/login-actions.tsx (1)
168-178:⚠️ Potential issue | 🟠 MajorOnly enable Google One Tap after provider discovery.
The button gating is fixed, but the hook still initializes with
enabled=true, so GIS can load and auto-prompt even whenuseOAuthProviders().googleis false. Read provider availability first, then calluseGoogleOneTap({ enabled: isGoogleConfigured }).♻️ Suggested fix
- const { - prompt: promptGoogle, - isPending: isGooglePending, - isReady: isGoogleReady, - } = useGoogleOneTap() const { github: isGithubConfigured, google: isGoogleConfigured, facebook: isFacebookConfigured, twitter: isTwitterConfigured, } = useOAuthProviders() + const { + prompt: promptGoogle, + isPending: isGooglePending, + isReady: isGoogleReady, + } = useGoogleOneTap({ enabled: isGoogleConfigured })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/app/auth/login/login-actions.tsx` around lines 168 - 178, The Google One Tap hook is being initialized before provider discovery; change the order so you call useOAuthProviders() first, read the google flag (isGoogleConfigured), then initialize useGoogleOneTap({ enabled: isGoogleConfigured }) instead of calling useGoogleOneTap() with its default enabled behavior—update the code around useOAuthProviders and useGoogleOneTap (variables prompt: promptGoogle, isPending: isGooglePending, isReady: isGoogleReady) so the hook only loads GIS when isGoogleConfigured is true.
🧹 Nitpick comments (2)
apps/next/app/auth/callback/oauth/github/route.ts (1)
9-20: Use stable error codes instead of matching backend prose.
mapAuthErrorstill keys off exact server error strings. That makes this callback brittle: any wording change upstream collapses specific states likeinvalid_stateoroauth_not_configuredinto the genericoauth_failedpath. Prefer returning stable codes from the API/client, or centralize this translation in a shared auth callback helper so the provider routes don’t each carry their own string map.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/app/auth/callback/oauth/github/route.ts` around lines 9 - 20, mapAuthError currently matches exact backend prose which is brittle; change it to accept and map stable error codes (or to delegate to a centralized helper) instead of raw messages: update the mapAuthError(raw: string) function to first check for a standardized code field (e.g., raw could be 'missing_params', 'invalid_state', 'expired_state', 'token_exchange_failed', 'fetch_user_failed', 'email_required', 'oauth_not_configured', 'oauth_failed') and return those directly, falling back to a centralized translateAuthError(raw) helper if necessary; ensure any string-to-code mapping uses stable identifiers (all-caps or snake_case) and move that mapping into the shared auth helper so provider routes no longer depend on exact backend prose.apps/fastify/src/routes/auth/passkey/resolve-user.ts (1)
34-35: Avoid hardcoding a route window that can drift from the shared 429 payload.This route pins
timeWindowto60_000, butapps/fastify/src/plugins/rate-limit.ts:31-38computesretryAfterfromenv.RATE_LIMIT_TIME_WINDOW. If that env value changes, this endpoint will still enforce 60s while telling clients to back off for a different duration.As per coding guidelines, "apps/fastify/**: - CRITICAL: never edit db/migrations, openapi schema files directly. All migrations must be created through the typical Drizzle flow and OpenAPI is autogenerated. Always validate no discrepancies."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/auth/passkey/resolve-user.ts` around lines 34 - 35, The route hardcodes config.rateLimit.timeWindow = 60_000 in resolve-user.ts which can drift from the shared setting used by apps/fastify/src/plugins/rate-limit.ts that reads env.RATE_LIMIT_TIME_WINDOW; change the route to derive its timeWindow from the same source (import or reference the shared RATE_LIMIT_TIME_WINDOW constant or process.env.RATE_LIMIT_TIME_WINDOW parsed to a number) instead of the literal 60000 and ensure the unit (ms) matches the value used in rate-limit.ts so retryAfter and the route enforcement remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/fastify/src/routes/auth/oauth/google/verify-id-token.ts`:
- Around line 88-181: Wrap the entire user/account/session flow in a single
database transaction (use the db.transaction(...) returned by getDb()) so
creations/updates to users, account and sessions are atomic; move all DB
reads/writes (select/insert/update on users, account, sessions and the calls to
encryptAccountTokens) into that transaction callback and throw to rollback on
error. After persisting/merging the user and account inside the transaction,
delegate session creation and token signing to the existing helper
createSessionAndIssueTokens (from apps/fastify/src/lib/session.ts) instead of
manually inserting into sessions and signing JWTs in this handler; pass it the
transaction-scoped DB, user.id and any required params so token issuance happens
as part of the same transaction. Ensure you still return the tokens from the
handler after the transaction completes.
In `@apps/fastify/src/routes/auth/passkey/resolve-user.ts`:
- Around line 9-11: Update ResolveUserBodySchema to match the handler's expected
input by requiring a non-empty base64url string: set userHandle to
Type.RegEx(/^[A-Za-z0-9_-]+(={0,2})?$/) (and/or add minLength: 1) so
schema-level validation rejects the same values the handler expects;
alternatively, if you keep the handler-level checks, remove the redundant
handler validation and ensure schema validation errors are transformed to the
same error response shape the route returns (normalize to the handler's error
format) so docs, schema and runtime behavior stay consistent.
In `@apps/fastify/src/routes/schemas.ts`:
- Around line 8-12: The RateLimitResponseSchema currently types retryAfter as a
Number but the rate-limit plugin returns an integer (Math.round(...)); update
the schema so retryAfter is an integer by changing its type in
RateLimitResponseSchema from Type.Number() to Type.Integer() to keep OpenAPI
accurate and consistent with the plugin output.
In `@apps/next/app/auth/login/use-google-one-tap.ts`:
- Around line 123-142: The hook apps/next/app/auth/login/use-google-one-tap.ts
uses getAuthErrorMessage('oauth_failed') which resolves to a GitHub-specific
message; change the error key to a Google-specific or neutral key and update the
calls in this hook accordingly. Add a new mapping (e.g., 'oauth_failed_google'
with Google wording) to the auth-error-messages map and replace
getAuthErrorMessage('oauth_failed') usages in use-google-one-tap.ts (including
the early returns and the catch block around updateAuthTokens/router.push) to
use the new key, or alternatively add/point a neutral 'oauth_failed_generic' key
in auth-error-messages and use that here to avoid provider-specific copy.
In `@packages/react/src/hooks/use-oauth-login.ts`:
- Around line 43-52: The current guard calls new URL(url) but doesn't validate
the scheme, allowing javascript:, data:, blob: etc.; update the use-oauth-login
hook to parse the redirect once (const parsed = new URL(url)) and then enforce
parsed.protocol is exactly 'http:' or 'https:' (otherwise throw the same OAuth
redirectUrl missing/invalid error referencing provider), and then use the
validated parsed.href (or url) when setting window.location.href; ensure you
still check typeof url === 'string' and non-empty before parsing.
---
Duplicate comments:
In `@apps/next/app/auth/login/login-actions.tsx`:
- Around line 168-178: The Google One Tap hook is being initialized before
provider discovery; change the order so you call useOAuthProviders() first, read
the google flag (isGoogleConfigured), then initialize useGoogleOneTap({ enabled:
isGoogleConfigured }) instead of calling useGoogleOneTap() with its default
enabled behavior—update the code around useOAuthProviders and useGoogleOneTap
(variables prompt: promptGoogle, isPending: isGooglePending, isReady:
isGoogleReady) so the hook only loads GIS when isGoogleConfigured is true.
In `@packages/react/src/hooks/use-passkey-auth.ts`:
- Around line 62-65: The onSuccess handler in usePasskeyAuth should await the
caller's async callback to avoid a race: change the callback type to return void
| Promise<void> (update any types / interfaces used by usePasskeyAuth and the
mutation options) and call await variables.onSuccess?.(data) inside the
onSuccess handler before calling queryClient.invalidateQueries (i.e., in the
mutation's onSuccess block in usePasskeyAuth, await variables.onSuccess?.(data)
first, then invalidate ['auth','session','user'] and ['auth','session','jwt']).
- Around line 43-52: The redirect handling accepts any absolute URL; restrict
redirects to only same-origin callbacks and safe schemes: when resolving
redirectUrl (variable redirectUrl from result) allow relative paths
(startsWith('/')) as before, but for absolute URLs parse both redirectUrl and
the original callbackUrl (from result.callbackUrl or wherever submitted) and
require new URL(redirectUrl).origin === new URL(callbackUrl).origin and that new
URL(redirectUrl).protocol is 'http:' or 'https:' before calling
window.location.assign; otherwise fall back to window.location.assign('/') to
block unsafe or cross-origin schemes. Ensure you reference redirectUrl and
callbackUrl in the change in use-passkey-auth.ts.
---
Nitpick comments:
In `@apps/fastify/src/routes/auth/passkey/resolve-user.ts`:
- Around line 34-35: The route hardcodes config.rateLimit.timeWindow = 60_000 in
resolve-user.ts which can drift from the shared setting used by
apps/fastify/src/plugins/rate-limit.ts that reads env.RATE_LIMIT_TIME_WINDOW;
change the route to derive its timeWindow from the same source (import or
reference the shared RATE_LIMIT_TIME_WINDOW constant or
process.env.RATE_LIMIT_TIME_WINDOW parsed to a number) instead of the literal
60000 and ensure the unit (ms) matches the value used in rate-limit.ts so
retryAfter and the route enforcement remain consistent.
In `@apps/next/app/auth/callback/oauth/github/route.ts`:
- Around line 9-20: mapAuthError currently matches exact backend prose which is
brittle; change it to accept and map stable error codes (or to delegate to a
centralized helper) instead of raw messages: update the mapAuthError(raw:
string) function to first check for a standardized code field (e.g., raw could
be 'missing_params', 'invalid_state', 'expired_state', 'token_exchange_failed',
'fetch_user_failed', 'email_required', 'oauth_not_configured', 'oauth_failed')
and return those directly, falling back to a centralized translateAuthError(raw)
helper if necessary; ensure any string-to-code mapping uses stable identifiers
(all-caps or snake_case) and move that mapping into the shared auth helper so
provider routes no longer depend on exact backend prose.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e020e16d-2564-4bfe-bb95-007f57d346dd
⛔ Files ignored due to path filters (1)
packages/core/src/gen/types.gen.tsis excluded by!**/gen/**,!**/gen/**,!**/*.gen.ts
📒 Files selected for processing (16)
apps/docu/content/docs/architecture/authentication.mdxapps/fastify/eslint.config.mjsapps/fastify/openapi/openapi.jsonapps/fastify/src/routes/auth/oauth/google/verify-id-token.tsapps/fastify/src/routes/auth/passkey/resolve-user.tsapps/fastify/src/routes/schemas.tsapps/next/app/auth/callback/oauth/facebook/route.tsapps/next/app/auth/callback/oauth/github/route.tsapps/next/app/auth/login/login-actions.tsxapps/next/app/auth/login/use-google-one-tap.tsapps/next/lib/auth/auth-error-messages.tsapps/next/lib/auth/last-magic-link-email-client.tspackages/react/src/components/login-form.tsxpackages/react/src/hooks/use-oauth-login.tspackages/react/src/hooks/use-passkey-auth.tspackages/react/src/hooks/use-passkey-discovery.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/react/src/hooks/use-passkey-discovery.ts
- apps/fastify/eslint.config.mjs
- packages/react/src/components/login-form.tsx
- apps/docu/content/docs/architecture/authentication.mdx
- apps/next/app/auth/callback/oauth/facebook/route.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/react/src/hooks/use-passkey-auth.ts (1)
43-55:⚠️ Potential issue | 🟠 MajorValidate redirect URL against the expected callback origin.
The current implementation accepts any parseable absolute URL as a redirect target. For security, the client should verify the redirect URL's origin matches the
callbackUrlorigin that was originally requested.Without this check, if server responses were somehow tampered with, users could be redirected to unexpected domains.
🛡️ Proposed fix
const redirectUrl = 'redirectUrl' in result ? result.redirectUrl : undefined if (redirectUrl) { - if (redirectUrl.startsWith('/')) window.location.assign(redirectUrl) - else - try { - new URL(redirectUrl) - window.location.assign(redirectUrl) - } catch { - window.location.assign('/') - } + try { + const url = new URL(redirectUrl, window.location.origin) + const expectedOrigin = callbackUrl ? new URL(callbackUrl).origin : window.location.origin + if (url.origin !== expectedOrigin) throw new Error('Unexpected redirect origin') + window.location.assign(url.toString()) + } catch { + window.location.assign('/') + } return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/hooks/use-passkey-auth.ts` around lines 43 - 55, The redirect handling currently allows any parseable absolute URL; change it to validate that redirectUrl's origin equals the origin of the originally requested callback URL before assigning window.location; locate the redirectUrl handling in use-passkey-auth.ts and parse new URL(redirectUrl).origin and compare it to the origin derived from the original callbackUrl (or fallback to window.location.origin if callbackUrl is not present in scope), only call window.location.assign(redirectUrl) when origins match, otherwise fall back to window.location.assign('/') (or the existing safe fallback).apps/next/app/auth/login/login-actions.tsx (1)
164-182:⚠️ Potential issue | 🟠 MajorInitialize Google One Tap only when Google OAuth is actually enabled.
The button is gated now, but the hook itself still mounts unconditionally. If
useGoogleOneTap()loads GIS or auto-prompts on mount, Google-disabled deployments can still trigger that work, and Line 182 will then disable the other auth actions viaisGooglePending. Please gate the hook itself onisGoogleConfiguredand only fold Google intoanyPendingwhen Google is enabled.#!/bin/bash set -euo pipefail echo "== useGoogleOneTap definition ==" fd 'use-google-one-tap\.(ts|tsx)$' -t f | while read -r f; do echo "-- $f --" sed -n '1,220p' "$f" done echo echo "== useGoogleOneTap call sites ==" rg -n -C2 'useGoogleOneTap\(' --type=ts --type=tsx apps packages🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/app/auth/login/login-actions.tsx` around lines 164 - 182, The useGoogleOneTap hook is currently called unconditionally; change the code so useGoogleOneTap() is only invoked when google OAuth is enabled (use the isGoogleConfigured flag) and compute isGooglePending/isGoogleReady/promptGoogle only in that branch; also update the anyPending expression to include isGooglePending only when isGoogleConfigured is true (e.g., anyPending = isOAuthPending || isPasskeyPending || (isGoogleConfigured && isGooglePending)); ensure references to promptGoogle and isGoogleReady are guarded by isGoogleConfigured where used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/next/app/auth/login/login-actions.tsx`:
- Around line 202-209: The onUsePasskey callback currently calls
startPasskeyAuth and then calls updateAuthTokens in its onSuccess without
handling errors, which can leave the user WebAuthn-successful but not signed in;
wrap the updateAuthTokens call in a try/catch inside the startPasskeyAuth
onSuccess handler (referencing onUsePasskey, startPasskeyAuth, updateAuthTokens,
setLastAuthMethod, and router.push) and on failure surface the error
deterministically (e.g., set an error state or show a toast) and do NOT call
router.push('/') when updateAuthTokens fails so the UI reflects the failed token
sync.
---
Duplicate comments:
In `@apps/next/app/auth/login/login-actions.tsx`:
- Around line 164-182: The useGoogleOneTap hook is currently called
unconditionally; change the code so useGoogleOneTap() is only invoked when
google OAuth is enabled (use the isGoogleConfigured flag) and compute
isGooglePending/isGoogleReady/promptGoogle only in that branch; also update the
anyPending expression to include isGooglePending only when isGoogleConfigured is
true (e.g., anyPending = isOAuthPending || isPasskeyPending ||
(isGoogleConfigured && isGooglePending)); ensure references to promptGoogle and
isGoogleReady are guarded by isGoogleConfigured where used.
In `@packages/react/src/hooks/use-passkey-auth.ts`:
- Around line 43-55: The redirect handling currently allows any parseable
absolute URL; change it to validate that redirectUrl's origin equals the origin
of the originally requested callback URL before assigning window.location;
locate the redirectUrl handling in use-passkey-auth.ts and parse new
URL(redirectUrl).origin and compare it to the origin derived from the original
callbackUrl (or fallback to window.location.origin if callbackUrl is not present
in scope), only call window.location.assign(redirectUrl) when origins match,
otherwise fall back to window.location.assign('/') (or the existing safe
fallback).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cb329b0b-e26e-4581-b5d5-ba288e5da84d
📒 Files selected for processing (3)
apps/next/app/auth/login/login-actions.tsxpackages/react/src/components/login-form.tsxpackages/react/src/hooks/use-passkey-auth.ts
- use-passkey-auth: validate redirect against callback origin, restrict http/https - use-oauth-login: enforce http/https protocol for redirect URLs - verify-id-token: wrap user/account/session in transaction, use createSessionAndIssueTokens - resolve-user: schema pattern for userHandle, timeWindow from env - schemas: retryAfter as Type.Integer for rate limit - auth-error-messages: add oauth_failed_google, translateOAuthError helper - use-google-one-tap: use oauth_failed_google for Google-specific errors - login-actions: enable Google One Tap only when isGoogleConfigured - github route: use backend error codes via translateOAuthError
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apps/fastify/src/routes/auth/oauth/google/verify-id-token.ts (2)
129-134: UnnecessaryencryptAccountTokenscall on non-sensitive data.
encryptAccountTokens({ updatedAt: new Date() })encrypts a field that contains no sensitive token data. TheencryptAccountTokenshelper is designed for OAuth tokens (accessToken, refreshToken, idToken). Here it just returns the same object unchanged after iteration.Simplified update
if (existingAccount) { - const encrypted = encryptAccountTokens({ updatedAt: new Date() }) await tx .update(account) - .set({ updatedAt: encrypted.updatedAt ?? new Date() }) + .set({ updatedAt: new Date() }) .where(eq(account.id, existingAccount.id)) } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/auth/oauth/google/verify-id-token.ts` around lines 129 - 134, The call to encryptAccountTokens when updating only updatedAt is unnecessary; instead of calling encryptAccountTokens({ updatedAt: new Date() }) and then using encrypted.updatedAt, set updatedAt directly in the update. Modify the existingAccount branch where tx.update(account).set(...) is called to pass a plain updatedAt value (e.g., new Date()) rather than the result of encryptAccountTokens; also remove any now-unused reference to encryptAccountTokens in this code path if applicable.
145-149: Type cast for transaction is fragile but functional.The cast
tx as unknown as Awaited<ReturnType<typeof getDb>>works but may break ifcreateSessionAndIssueTokenschanges its expectations. Consider updatingcreateSessionAndIssueTokensto accept a transaction type directly, or use a shared DB transaction type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/auth/oauth/google/verify-id-token.ts` around lines 145 - 149, The current fragile cast "tx as unknown as Awaited<ReturnType<typeof getDb>>" should be removed: update the createSessionAndIssueTokens signature to accept a concrete transaction type (or a generic/type alias like DBTransaction) instead of the full DB return, and use that shared transaction type in both the caller and the implementation; then replace the casted argument with the strongly-typed tx variable. Locate createSessionAndIssueTokens and change its parameter type to the shared DB transaction type (or add a type parameter), update any internal usages to the new type, and adjust the call site in verify-id-token.ts to pass tx directly with the proper type.apps/next/lib/auth/auth-error-messages.ts (1)
14-23:messageToKeyonly maps GitHub-specific messages.The fallback map only covers GitHub error messages. If Facebook or Twitter endpoints return similar raw messages with their provider names (e.g., "Failed to fetch Facebook user"), they won't be translated and will fall back to the generic
oauth_failedwhich displays GitHub-specific copy.Consider adding provider-agnostic mappings or provider-specific entries to maintain consistent error messaging across all OAuth providers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/lib/auth/auth-error-messages.ts` around lines 14 - 23, The messageToKey mapping currently only handles GitHub-specific strings; update messageToKey to handle provider-agnostic and/or provider-specific OAuth errors by either adding entries for other providers (e.g., "Failed to fetch Facebook user", "Failed to fetch Twitter user", "Failed to exchange Facebook code for token", etc.) or normalizing incoming error text before lookup (e.g., strip provider names or match patterns like "Failed to fetch .* user") so keys like 'fetch_user_failed', 'token_exchange_failed', and 'oauth_failed' map correctly regardless of provider; modify the lookup logic to use the normalized string or pattern-matching against messageToKey to ensure consistent, non-GitHub-specific error keys are returned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react/src/hooks/use-passkey-auth.ts`:
- Around line 8-12: The UsePasskeyAuthParams type currently allows both
callbackUrl and onSuccess to be optional; change it to a discriminated union so
callers must choose one mode: define two branches (e.g. one branch with
callbackUrl: string and onSuccess?: never for redirect flow, and the other with
callbackUrl?: undefined and onSuccess: (data: { token: string; refreshToken:
string }) => void | Promise<void> for direct-token flow) and replace the
existing UsePasskeyAuthParams with this union so calls to mutate/usePasskeyAuth
are type-checked into one mutually exclusive mode.
- Around line 44-57: Remove the insecure fast-path that checks
redirectUrl.startsWith('/'); always parse redirectUrl through the URL
constructor with a base (e.g., new URL(redirectUrl, window.location.origin)) so
scheme-relative values (//host) are resolved and subject to validation; then
compute allowedOrigin from callbackUrl or window.location.origin, allow only
http/https protocols and require parsed.origin === allowedOrigin, and only then
call window.location.assign(redirectUrl) otherwise fallback to
window.location.assign('/'); update the logic around the redirectUrl and
callbackUrl checks in use-passkey-auth (the redirectUrl handling block)
accordingly.
---
Nitpick comments:
In `@apps/fastify/src/routes/auth/oauth/google/verify-id-token.ts`:
- Around line 129-134: The call to encryptAccountTokens when updating only
updatedAt is unnecessary; instead of calling encryptAccountTokens({ updatedAt:
new Date() }) and then using encrypted.updatedAt, set updatedAt directly in the
update. Modify the existingAccount branch where tx.update(account).set(...) is
called to pass a plain updatedAt value (e.g., new Date()) rather than the result
of encryptAccountTokens; also remove any now-unused reference to
encryptAccountTokens in this code path if applicable.
- Around line 145-149: The current fragile cast "tx as unknown as
Awaited<ReturnType<typeof getDb>>" should be removed: update the
createSessionAndIssueTokens signature to accept a concrete transaction type (or
a generic/type alias like DBTransaction) instead of the full DB return, and use
that shared transaction type in both the caller and the implementation; then
replace the casted argument with the strongly-typed tx variable. Locate
createSessionAndIssueTokens and change its parameter type to the shared DB
transaction type (or add a type parameter), update any internal usages to the
new type, and adjust the call site in verify-id-token.ts to pass tx directly
with the proper type.
In `@apps/next/lib/auth/auth-error-messages.ts`:
- Around line 14-23: The messageToKey mapping currently only handles
GitHub-specific strings; update messageToKey to handle provider-agnostic and/or
provider-specific OAuth errors by either adding entries for other providers
(e.g., "Failed to fetch Facebook user", "Failed to fetch Twitter user", "Failed
to exchange Facebook code for token", etc.) or normalizing incoming error text
before lookup (e.g., strip provider names or match patterns like "Failed to
fetch .* user") so keys like 'fetch_user_failed', 'token_exchange_failed', and
'oauth_failed' map correctly regardless of provider; modify the lookup logic to
use the normalized string or pattern-matching against messageToKey to ensure
consistent, non-GitHub-specific error keys are returned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0e2a2989-54c2-43ac-a27a-9ee9ca950738
📒 Files selected for processing (10)
apps/fastify/openapi/openapi.jsonapps/fastify/src/routes/auth/oauth/google/verify-id-token.tsapps/fastify/src/routes/auth/passkey/resolve-user.tsapps/fastify/src/routes/schemas.tsapps/next/app/auth/callback/oauth/github/route.tsapps/next/app/auth/login/login-actions.tsxapps/next/app/auth/login/use-google-one-tap.tsapps/next/lib/auth/auth-error-messages.tspackages/react/src/hooks/use-oauth-login.tspackages/react/src/hooks/use-passkey-auth.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/fastify/src/routes/auth/passkey/resolve-user.ts
…en, oauth mapping
- wrap updateAuthTokens in try/catch, show toast on failure, skip router.push
- UsePasskeyAuthParams discriminated union (callbackUrl | onSuccess)
- remove insecure redirectUrl.startsWith('/') fast-path, always validate via URL
- remove unnecessary encryptAccountTokens for updatedAt-only update
- DbForSession type for createSessionAndIssueTokens, drop tx cast
- add provider-agnostic OAuth error entries to messageToKey
This was referenced Mar 10, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes