feat(auth): support multiple Google OAuth callback URLs for web + mobile#134
Merged
gaboesquivel merged 7 commits intomainfrom Mar 10, 2026
Merged
feat(auth): support multiple Google OAuth callback URLs for web + mobile#134gaboesquivel merged 7 commits intomainfrom
gaboesquivel merged 7 commits intomainfrom
Conversation
- Add OAUTH_GOOGLE_CALLBACK_URLS (comma-separated) with backward compat for OAUTH_GOOGLE_CALLBACK_URL - Client sends redirect_uri in authorize-url request; server validates against allowlist - Store redirectUri in verification meta for exchange step - useOAuthLogin/useOAuthLink accept optional redirectUri for Google (mobile custom schemes) - Update providers and auth docs
- authentication.mdx: add Google redirect endpoints, link-authorize-url with redirect_uri, callback pages - account-linking.mdx: document meta.redirectUri and Google multi-URL flow - vercel.mdx: add OAuth env vars pointer to Authentication doc
|
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 a Google OAuth redirect-based PKCE flow (authorize, exchange, link-authorize), persists redirectUri in verification.meta, exposes googleRedirect in the providers API, updates env/config, adds Fastify routes/tests and Next.js callback, and updates client hooks/UI to support redirect fallback alongside One Tap. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/Browser
participant NextJS as Next.js (callback)
participant Fastify as Fastify API
participant Google as Google OAuth
participant DB as Database
rect rgba(100,150,240,0.5)
Note over Client,NextJS: Start authorization
Client->>NextJS: Click "Sign in with Google" (or link)
NextJS->>Fastify: GET /auth/oauth/google/authorize-url?redirect_uri=...
Fastify->>Fastify: validate redirect_uri, generate state & PKCE
Fastify->>DB: store oauth_state (meta: redirectUri, codeVerifier)
Fastify-->>NextJS: { redirectUrl }
NextJS->>Google: Redirect user to Google (state, code_challenge)
Google-->>Client: Consent and redirect with code & state
Client->>NextJS: GET /auth/callback/oauth/google?code=...&state=...
end
rect rgba(150,220,130,0.5)
Note over NextJS,Fastify: Exchange & session
NextJS->>Fastify: POST /auth/oauth/google/exchange { code, state }
Fastify->>DB: validate & consume oauth_state, read redirectUri & codeVerifier
Fastify->>Google: POST token exchange (code, code_verifier, redirect_uri)
Google-->>Fastify: tokens (access_token, refresh_token, id_token)
Fastify->>Google: GET userinfo (Bearer access_token)
Google-->>Fastify: userinfo (email, verified)
Fastify->>DB: link/create user, store encrypted account tokens, create refresh session
Fastify-->>NextJS: { token, refreshToken, redirectTo }
NextJS-->>Client: Set auth cookies and redirect to redirectTo
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/next/app/(dashboard)/settings/(profile)/linked-accounts-section.tsx (1)
141-147:⚠️ Potential issue | 🟠 MajorGate Google linking on backend availability, not the static allowlist.
Line 144 only checks whether
providerIdis inlinkableProviders. With this PR, Google redirect linking has its own availability bit (googleRedirect) separate from'google'here makes the button clickable even when the backend can only answerOAUTH_NOT_CONFIGURED. Please drive Google’s enabled state from the providers payload instead of the hard-coded list.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/app/`(dashboard)/settings/(profile)/linked-accounts-section.tsx around lines 141 - 147, The LinkProviderButton currently determines canLink using the hard-coded linkableProviders array; instead, derive whether a provider can be linked from the backend providers payload and pass that to useOAuthLink. Update LinkProviderButton to consume the providers availability (e.g., a prop or context containing the providers payload) and compute canLink using that payload for providerId (use something like providers[providerId]?.enabled or providers.includes(providerId)), then call useOAuthLink only when the backend says the provider is available (and fall back safely for others). Refer to linkableProviders, LinkProviderButton, providerId, and useOAuthLink when making the change.
🧹 Nitpick comments (1)
packages/react/src/hooks/use-oauth-link.ts (1)
14-30: Consider simplifying the conditional logic for better readability.The current implementation has Google appearing in both the special-case branch (with redirectUri) and the provider map (without redirectUri). While functionally correct, this could be refactored to reduce cognitive load.
♻️ Optional refactor for clarity
mutationFn: async () => { - const data = - provider === 'google' && redirectUri - ? await client.auth.oauth.google.linkAuthorizeUrl({ - query: { - // biome-ignore lint/style/useNamingConvention: OAuth API expects redirect_uri - redirect_uri: redirectUri, - }, - throwOnError: true, - }) - : await ( - { - github: client.auth.oauth.github.linkAuthorizeUrl, - google: client.auth.oauth.google.linkAuthorizeUrl, - facebook: client.auth.oauth.facebook.linkAuthorizeUrl, - twitter: client.auth.oauth.twitter.linkAuthorizeUrl, - } as const - )[provider]({ throwOnError: true }) + const providerMethods = { + github: client.auth.oauth.github.linkAuthorizeUrl, + google: client.auth.oauth.google.linkAuthorizeUrl, + facebook: client.auth.oauth.facebook.linkAuthorizeUrl, + twitter: client.auth.oauth.twitter.linkAuthorizeUrl, + } as const + + const options = + provider === 'google' && redirectUri + ? { + query: { + // biome-ignore lint/style/useNamingConvention: OAuth API expects redirect_uri + redirect_uri: redirectUri, + }, + throwOnError: true as const, + } + : { throwOnError: true as const } + + const data = await providerMethods[provider](options) const url = data?.redirectUrl🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/hooks/use-oauth-link.ts` around lines 14 - 30, The conditional is confusing because google is handled twice; refactor use-oauth-link by building a single args object (include redirect_uri only when provider === 'google' and redirectUri is defined) and then call the appropriate linkAuthorizeUrl from client.auth.oauth via a provider map; e.g., prepare const args = { throwOnError: true, ...(provider === 'google' && redirectUri ? { query: { redirect_uri: redirectUri } } : {}) } and invoke ( { github: client.auth.oauth.github.linkAuthorizeUrl, google: client.auth.oauth.google.linkAuthorizeUrl, facebook: client.auth.oauth.facebook.linkAuthorizeUrl, twitter: client.auth.oauth.twitter.linkAuthorizeUrl } as const )[provider](args) so linkAuthorizeUrl is called once for all providers.
🤖 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/authorize-url.ts`:
- Around line 62-81: Generate a per-request PKCE code_verifier, compute its S256
code_challenge, persist the code_verifier (or its hash) alongside
state/stateHash in the same verification record inserted via getDb().insert(...
) (use the existing verification/meta object to store code_challenge and/or
code_verifier hash and expiresAt), add
redirectUrl.searchParams.set('code_challenge', <challenge>) and
.set('code_challenge_method','S256') when building the Google authorize URL (and
mirror this change in link-authorize-url), and update
apps/fastify/src/routes/auth/oauth/google/exchange.ts to require and validate
the submitted code_verifier by computing/verifying the S256 match against the
stored code_challenge before minting a session.
In `@apps/fastify/src/routes/auth/oauth/google/exchange.ts`:
- Around line 213-224: When reusing an existing account row in exchange.ts
(inside the existingAccount branch), also persist the token expiry and any other
token metadata returned by encryptAccountTokens so the DB stays in sync with the
newly stored credentials; update the db.update(account).set(...) call (the same
block that sets accessToken and updatedAt) to also set accessTokenExpiresAt (and
any other fields returned by encryptAccountTokens, e.g., refreshToken or scope
if present) using values from the encrypted result or accountData.
In `@apps/fastify/src/routes/auth/oauth/google/link-authorize-url.ts`:
- Around line 49-58: The /link-authorize-url endpoint currently only checks for
GOOGLE_CLIENT_ID and callback URLs (googleClientId, allowedUrls, defaultUrl) but
not GOOGLE_CLIENT_SECRET, allowing a successful redirect that later fails in the
exchange route; update the gate in link-authorize-url.ts to use the same
configuration check as the exchange route by requiring env.GOOGLE_CLIENT_SECRET
(in addition to GOOGLE_CLIENT_ID and callback URL(s)) and return the same 503
OAUTH_NOT_CONFIGURED response when the secret is missing so users cannot
initiate an OAuth flow that will inevitably fail in the exchange handler.
In `@apps/next/app/auth/callback/oauth/google/route.ts`:
- Around line 50-56: translateOAuthError currently doesn't handle the new
TOKEN_EXCHANGE_FAILED code emitted by the Google exchange route, so
token-exchange failures fall back to the generic oauth_failed; update the
translateOAuthError function to explicitly detect the TOKEN_EXCHANGE_FAILED
condition (compare against the raw message and/or ApiError body) and return a
distinct error key such as "token_exchange_failed" (or your project's canonical
key) so the redirect in route.ts uses that specific message instead of
oauth_failed.
In `@apps/next/app/auth/login/login-actions.tsx`:
- Around line 194-205: The handleGoogleClick callback currently shows
oauth_not_configured when Google init is slow; change its guards so it does not
report "not configured" while GIS is still booting: first check if
!isGoogleReady and handle that case by returning early (e.g., disable the button
or show a loading/info toast) instead of falling through to the
configured-check, then only show the oauth_not_configured toast when
isGoogleReady is true and isGoogleRedirectConfigured is false; update the logic
in handleGoogleClick (which references oneTapSkipped, isGoogleReady,
isGoogleRedirectConfigured, startOAuthLogin, promptGoogle) accordingly.
In `@apps/next/app/auth/login/use-google-one-tap.ts`:
- Around line 199-206: The prompt useCallback currently only handles the case
where window.google.accounts.id exists or clientId is missing, so when clientId
is present but the Google script failed to load (loadError === true) it silently
no-ops; update the prompt function (the useCallback named prompt that references
handledRef and handleMoment) to check loadError first and, if true, call the
fallback (invoke onSkipped if provided) and/or show an error toast via
toast.error(getAuthErrorMessage('oauth_not_configured') or a more specific
message) so users are guided to the redirect fallback instead of nothing
happening.
In `@apps/next/lib/auth/auth-error-messages.ts`:
- Line 29: Remove the orphaned mapping entry for the exact key "Could not
retrieve email from Google" from the auth error mappings (the error map
constant/object that contains "'Could not retrieve verified email from Google':
'email_required'"); it is dead code because the backend only emits the
"verified" variant—delete the redundant line so only the correct "Could not
retrieve verified email from Google" -> "email_required" mapping remains.
---
Outside diff comments:
In `@apps/next/app/`(dashboard)/settings/(profile)/linked-accounts-section.tsx:
- Around line 141-147: The LinkProviderButton currently determines canLink using
the hard-coded linkableProviders array; instead, derive whether a provider can
be linked from the backend providers payload and pass that to useOAuthLink.
Update LinkProviderButton to consume the providers availability (e.g., a prop or
context containing the providers payload) and compute canLink using that payload
for providerId (use something like providers[providerId]?.enabled or
providers.includes(providerId)), then call useOAuthLink only when the backend
says the provider is available (and fall back safely for others). Refer to
linkableProviders, LinkProviderButton, providerId, and useOAuthLink when making
the change.
---
Nitpick comments:
In `@packages/react/src/hooks/use-oauth-link.ts`:
- Around line 14-30: The conditional is confusing because google is handled
twice; refactor use-oauth-link by building a single args object (include
redirect_uri only when provider === 'google' and redirectUri is defined) and
then call the appropriate linkAuthorizeUrl from client.auth.oauth via a provider
map; e.g., prepare const args = { throwOnError: true, ...(provider === 'google'
&& redirectUri ? { query: { redirect_uri: redirectUri } } : {}) } and invoke ( {
github: client.auth.oauth.github.linkAuthorizeUrl, google:
client.auth.oauth.google.linkAuthorizeUrl, facebook:
client.auth.oauth.facebook.linkAuthorizeUrl, twitter:
client.auth.oauth.twitter.linkAuthorizeUrl } as const )[provider](args) so
linkAuthorizeUrl is called once for all providers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bffdfdd9-b2e4-44be-aa0b-77989807e417
⛔ Files ignored due to path filters (10)
apps/fastify/.env-sampleis excluded by!**/.env*apps/next/.env.developmentis excluded by!**/.env*apps/next/.env.local.exampleis excluded by!**/.env*apps/next/.env.productionis excluded by!**/.env*apps/next/.env.stagingis excluded by!**/.env*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.ts
📒 Files selected for processing (23)
apps/docu/content/docs/architecture/account-linking.mdxapps/docu/content/docs/architecture/authentication.mdxapps/docu/content/docs/deployment/vercel.mdxapps/fastify/eslint.config.mjsapps/fastify/openapi/openapi.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/google/authorize-url.test.tsapps/fastify/src/routes/auth/oauth/google/authorize-url.tsapps/fastify/src/routes/auth/oauth/google/exchange.test.tsapps/fastify/src/routes/auth/oauth/google/exchange.tsapps/fastify/src/routes/auth/oauth/google/link-authorize-url.tsapps/fastify/src/routes/auth/oauth/providers.test.tsapps/fastify/src/routes/auth/oauth/providers.tsapps/next/app/(dashboard)/settings/(profile)/linked-accounts-section.tsxapps/next/app/auth/callback/oauth/google/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-link.tspackages/react/src/hooks/use-oauth-login.tspackages/react/src/hooks/use-oauth-providers.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/eslint.config.mjs`:
- Around line 12-15: The ESLint rule 'max-lines' in the rules object should not
be loosened to 350; revert or change the 'max-lines' setting back to the repo
cap of 300 and do not create a blanket exemption for route files. Instead,
refactor the large Fastify route handler (see
src/routes/auth/oauth/google/exchange.ts) by extracting logical pieces of the
exchange flow into small helper functions/modules (e.g., token parsing, provider
calls, error handling, response building) so the route handler stays under 300
lines; keep the 'rules' entry as 'max-lines': ['error', { max: 300,
skipBlankLines: true, skipComments: true }] and move implementation changes into
helpers referenced from the route.
In `@apps/next/app/auth/login/login-actions.tsx`:
- Around line 194-205: Current logic blocks running Google One Tap when
isGoogleReady is true but isGoogleRedirectConfigured is false by showing
oauth_not_configured; update handleGoogleClick so that the redirect fallback
(useRedirect) only triggers startOAuthLogin('google') and returns, but do not
show the oauth_not_configured error or block promptGoogle() based on
isGoogleRedirectConfigured — i.e., remove the branch that calls
toast.error(getAuthErrorMessage('oauth_not_configured')) and instead: if
useRedirect then startOAuthLogin('google') and return; else if (!isGoogleReady)
return; else call promptGoogle(); keep references to oneTapSkipped,
isGoogleReady, isGoogleRedirectConfigured, startOAuthLogin, promptGoogle,
toast.error/getAuthErrorMessage for locating the code to change.
In `@apps/next/lib/auth/auth-error-messages.ts`:
- Around line 29-39: The Google-specific error strings are mapped to generic
codes that render GitHub-branded text; update the mappings in
auth-error-messages.ts so Google errors map to provider-specific codes (e.g.,
add keys like 'google_fetch_user_failed' and 'google_oauth_failed' instead of
reusing 'fetch_user_failed'/'oauth_failed'), and add corresponding entries in
authErrorMessages (or ensure authErrorMessages has provider-specific keys) so
Google callback errors render "Google ..." not "GitHub ..."; look for the
keys/values around 'Failed to fetch Google user', 'Google sign-in failed',
'fetch_user_failed', 'oauth_failed' and authErrorMessages.oauth_failed to apply
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 260d03bc-9b84-43d1-a1a7-fc3a20bc6b1d
📒 Files selected for processing (9)
apps/fastify/eslint.config.mjsapps/fastify/src/routes/auth/oauth/google/authorize-url.tsapps/fastify/src/routes/auth/oauth/google/exchange.tsapps/fastify/src/routes/auth/oauth/google/link-authorize-url.tsapps/next/app/(dashboard)/settings/(profile)/linked-accounts-section.tsxapps/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-link.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/next/app/(dashboard)/settings/(profile)/linked-accounts-section.tsx
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
apps/fastify/src/lib/oauth-google.ts (1)
47-55: ThrowErrorinstances here, not plain objects.These object literals lose stack context and already trip the lint rule in this file. Wrap the provider payload in a typed
Errorsubclass instead, then narrow oninstanceofin the route handler.Proposed refactor
+class GoogleOAuthError<T> extends Error { + constructor( + message: string, + readonly status: number, + readonly details: T, + ) { + super(message) + this.name = 'GoogleOAuthError' + } +} + export async function fetchGoogleTokens(input: { code: string codeVerifier: string @@ }) const tokenData = (await tokenRes.json()) as GoogleTokenResponse - if (!tokenRes.ok) throw { status: tokenRes.status, tokenData } - if (tokenData.error || !tokenData.access_token) throw { status: 400, tokenData } + if (!tokenRes.ok) + throw new GoogleOAuthError('Google token exchange failed', tokenRes.status, tokenData) + if (tokenData.error || !tokenData.access_token) + throw new GoogleOAuthError('Google token exchange failed', 400, tokenData) return tokenData } @@ }) const gUser = (await userRes.json()) as GoogleUser - if (!userRes.ok) throw { status: userRes.status, gUser } - if (!gUser?.id) throw { status: 400, gUser } + if (!userRes.ok) + throw new GoogleOAuthError('Google user info fetch failed', userRes.status, gUser) + if (!gUser?.id) + throw new GoogleOAuthError('Google user info payload missing id', 400, gUser) return gUser }Also applies to: 60-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/lib/oauth-google.ts` around lines 47 - 55, Replace the plain object throws in the token exchange logic with actual Error instances (or a small typed Error subclass) so stack traces are preserved and can be narrowed with instanceof; specifically change the throws after parsing tokenRes/tokenData in oauth-google.ts (the token exchange surrounding tokenRes, tokenData and the subsequent error checks) to throw a new Error subclass that wraps the provider payload (status and tokenData) and use that class in the route handler to detect provider errors; apply the same pattern to the second similar block (lines ~60-67) that currently throws plain objects.
🤖 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/exchange.ts`:
- Around line 61-65: When consuming the OAuth state (use the value at
stateRecord.meta.redirectUri), re-validate that redirectUri against the current
allowlist (allowedUrls derived from env.OAUTH_GOOGLE_CALLBACK_URLS /
env.OAUTH_GOOGLE_CALLBACK_URL) instead of trusting the stored value; in the
exchange handler (where stateRecord is read) check that
stateRecord.meta.redirectUri is included in allowedUrls (or equals defaultUrl)
and abort the exchange with an error if not, applying the same guard at the
other consumption site noted (around the code near line 87) so stale or tampered
state cannot bypass the current callback whitelist.
- Around line 110-115: The code currently returns a hardcoded 400 for
token-exchange failures; instead read and use the upstream status from the error
object (the local variable e cast from the helper error) so transient/5xx and
rate-limit/429 responses are preserved; specifically set reply.code to e.status
when e.status is a 4xx/429, and normalize server errors (e.status >= 500) to 503
to allow retry behavior, and apply the same change to the other identical branch
that handles token exchange failures (the other branch around lines 129-133).
In `@apps/next/app/auth/login/login-actions.tsx`:
- Around line 29-33: The Google button is currently gated only by
isGoogleConfigured so deployments with googleRedirect=true but google=false
can’t start sign-in; update the OAuthButtons props and usage to accept a
googleRedirect (or googleRedirectEnabled) boolean alongside isGoogleConfigured,
pass the parent-level googleRedirect into OAuthButtons, and change the disable
condition for the Google button (in the UI and any prop checks around
handleGoogleClick / startOAuthLogin) to disable only when both
isGoogleConfigured === false AND googleRedirect === false; update all places
referencing OAuthButtons and the Google button logic (e.g., the
startOAuthLogin/onGoogleClick wiring and any isGoogleConfigured checks) to use
the new combined condition so redirect-only setups enable the button and still
call handleGoogleClick which performs the redirect.
In `@apps/next/lib/auth/auth-error-messages.ts`:
- Around line 55-58: translateOAuthError currently only partly rewrites
body.code for Google and can leave GitHub-branded keys in getAuthErrorMessage;
change the function so it first resolves the base error key (using
messageToKey[raw] or body.code) and then, if the provider is 'google', apply a
second mapping that overrides specific keys to Google-specific variants (e.g.,
map 'token_exchange_failed' -> 'google_token_exchange_failed', and ensure you
add 'google_email_required' to auth-error-messages.ts alongside
'email_required'); update translateOAuthError to reference messageToKey,
body.code, and provider so all Google overrides are applied in one place after
key resolution.
---
Nitpick comments:
In `@apps/fastify/src/lib/oauth-google.ts`:
- Around line 47-55: Replace the plain object throws in the token exchange logic
with actual Error instances (or a small typed Error subclass) so stack traces
are preserved and can be narrowed with instanceof; specifically change the
throws after parsing tokenRes/tokenData in oauth-google.ts (the token exchange
surrounding tokenRes, tokenData and the subsequent error checks) to throw a new
Error subclass that wraps the provider payload (status and tokenData) and use
that class in the route handler to detect provider errors; apply the same
pattern to the second similar block (lines ~60-67) that currently throws plain
objects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ff8639c0-487c-4269-865d-e9e7d76c2321
📒 Files selected for processing (6)
apps/fastify/eslint.config.mjsapps/fastify/src/lib/oauth-google.tsapps/fastify/src/routes/auth/oauth/google/exchange.tsapps/next/app/auth/callback/oauth/google/route.tsapps/next/app/auth/login/login-actions.tsxapps/next/lib/auth/auth-error-messages.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/next/app/auth/callback/oauth/google/route.ts
- Revert max-lines to 300, extract exchange/template helpers - Remove oauth_not_configured toast, use oauth_failed_google for Google callback - Fix no-throw-literal in oauth-google
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/fastify/src/routes/auth/oauth/google/exchange.ts (2)
88-88:⚠️ Potential issue | 🟠 MajorRe-validate the consumed
redirectUriagainst the current allowlist.Line 88 still trusts
stateRecord.meta?.redirectUrionce the state is loaded. That means a stale or malformed state record can bypass the currentOAUTH_GOOGLE_CALLBACK_URLSpolicy unless you re-check the finalredirectUriagainstallowedUrlsbefore exchanging the code.As per coding guidelines,
SECURITY: Ensure server-side validation (never trust client-side validation alone).🤖 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/exchange.ts` at line 88, The code currently trusts stateRecord.meta?.redirectUri (assigned to redirectUri) without re-checking it against the current allowlist (OAUTH_GOOGLE_CALLBACK_URLS / allowedUrls); update the OAuth exchange flow to validate the resolved redirectUri against allowedUrls before proceeding to exchange the code: after computing redirectUri (from stateRecord.meta?.redirectUri ?? defaultUrl) call the existing allowlist check (or implement a small helper like isAllowedRedirect(redirectUri, allowedUrls)), and if it fails log and return an error (or fall back to defaultUrl) and abort the token exchange; ensure you reference redirectUri, stateRecord, defaultUrl and allowedUrls in the check so stale/malformed state cannot bypass policy.
105-108:⚠️ Potential issue | 🟠 MajorDon’t infer HTTP status from the helper message text.
Line 108 checks
includes('timeout'), butbuildTokenExchangeError()returns"Token exchange timed out", so token-exchange timeouts currently fall through as 400 instead of 504. These branches also collapse the structured upstream status into 400 for non-timeout failures. Carry an explicit status or timeout flag from the helper and map that here.Also applies to: 116-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/exchange.ts` around lines 105 - 108, The code currently infers HTTP status from tokenErr.message text; change the flow so buildTokenExchangeError(...) returns an explicit numeric status (e.g., status) or an explicit timeout flag (e.g., isTimeout) on the tokenErr object, and then use that field when setting the reply code in this handler (replace the message.includes('timeout') logic with reply.code(tokenErr.status ?? (tokenErr.isTimeout ? 504 : 400))). Update both places that handle tokenErr (the block around buildTokenExchangeError at lines ~105–108 and the similar block around ~116–118) so structured upstream statuses and timeouts are preserved instead of being inferred from the message. Ensure tokenErr is still sent in the response body.
🧹 Nitpick comments (3)
apps/fastify/src/routes/reference/template-scripts.ts (2)
175-178: Add a brief comment explaining the intentional empty catch.The empty catch block is a valid pattern for "try to parse, ignore if fails" but can be confusing to readers. A brief comment improves clarity.
💡 Suggested improvement
try { const body = await res.json(); msg = body.message || msg; - } catch {} + } catch { /* ignore parse errors, use default message */ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/reference/template-scripts.ts` around lines 175 - 178, The empty catch after attempting to parse the response JSON (the try around await res.json() that sets msg = body.message || msg) is intentional to ignore JSON parse errors; add a brief inline comment above the catch explaining this intent (e.g., "ignore parse errors — fallback to default msg") so readers understand why the catch is empty and no error handling is performed.
156-157: Consider validating that the code contains only digits.The HTML pattern attribute (
\\d*) provides some validation, but the JavaScript only checks length. Adding a digits-only check would improve UX by catching invalid input before the API call.💡 Suggested improvement
const code = input?.value?.trim(); - if (!code || code.length !== 6) return; + if (!code || !/^\\d{6}$/.test(code)) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/reference/template-scripts.ts` around lines 156 - 157, The current validation only checks length after trimming (const code = input?.value?.trim(); if (!code || code.length !== 6) return;) — update this to ensure the code is digits-only as well (e.g. test with a regex like /^\d{6}$/) and return early if the test fails; locate the validation around the const code = input?.value?.trim() and replace the length-only check with a digits-plus-length check so non-numeric input is rejected before the API call.apps/fastify/src/routes/reference/template.ts (1)
18-18: Consider pinning the Scalar CDN version for production stability.Using
@latestmeans the script can change without notice, potentially breaking functionality or introducing security issues. Consider pinning to a specific version (e.g.,@1.x.x) for predictable behavior.💡 Suggested change
- <script src="https://cdn.jsdelivr.net/npm/@scalar/api-reference@latest/dist/browser/standalone.js"></script> + <script src="https://cdn.jsdelivr.net/npm/@scalar/api-reference@1.25.0/dist/browser/standalone.js"></script>Replace
1.25.0with the actual version currently in use or the latest stable version.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/reference/template.ts` at line 18, Replace the dynamic CDN specifier in the script tag string "@scalar/api-reference@latest/dist/browser/standalone.js" with a pinned version (for example "@scalar/api-reference@1.25.0/dist/browser/standalone.js" or a semver-stable spec like "@scalar/api-reference@1.x.x/...") so the template served by the reference route uses a fixed package version; update the script src in the template that contains that string and optionally add a comment or TODO to document the chosen pinned version and process for updating it.
🤖 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/reference/template-scripts.ts`:
- Around line 24-28: The current use of window.getComputedStyle(configureButton,
':hover') is invalid for pseudo-classes; change the approach so the Configure
button actively captures its hover color into a variable and the link uses that
value: add mouseenter/mouseleave listeners on configureButton that save its
computed backgroundColor (e.g., hoverBg =
getComputedStyle(configureButton).backgroundColor on
configureButton.onmouseenter and clear onmouseleave) and then update
link.onmouseenter to use that saved hoverBg (fallback to style.backgroundColor
if empty); keep link.onmouseleave setting link.style.backgroundColor back to
style.backgroundColor and reference the existing configureButton, link,
link.onmouseenter/leave handlers, and getComputedStyle call to locate where to
change.
---
Duplicate comments:
In `@apps/fastify/src/routes/auth/oauth/google/exchange.ts`:
- Line 88: The code currently trusts stateRecord.meta?.redirectUri (assigned to
redirectUri) without re-checking it against the current allowlist
(OAUTH_GOOGLE_CALLBACK_URLS / allowedUrls); update the OAuth exchange flow to
validate the resolved redirectUri against allowedUrls before proceeding to
exchange the code: after computing redirectUri (from
stateRecord.meta?.redirectUri ?? defaultUrl) call the existing allowlist check
(or implement a small helper like isAllowedRedirect(redirectUri, allowedUrls)),
and if it fails log and return an error (or fall back to defaultUrl) and abort
the token exchange; ensure you reference redirectUri, stateRecord, defaultUrl
and allowedUrls in the check so stale/malformed state cannot bypass policy.
- Around line 105-108: The code currently infers HTTP status from
tokenErr.message text; change the flow so buildTokenExchangeError(...) returns
an explicit numeric status (e.g., status) or an explicit timeout flag (e.g.,
isTimeout) on the tokenErr object, and then use that field when setting the
reply code in this handler (replace the message.includes('timeout') logic with
reply.code(tokenErr.status ?? (tokenErr.isTimeout ? 504 : 400))). Update both
places that handle tokenErr (the block around buildTokenExchangeError at lines
~105–108 and the similar block around ~116–118) so structured upstream statuses
and timeouts are preserved instead of being inferred from the message. Ensure
tokenErr is still sent in the response body.
---
Nitpick comments:
In `@apps/fastify/src/routes/reference/template-scripts.ts`:
- Around line 175-178: The empty catch after attempting to parse the response
JSON (the try around await res.json() that sets msg = body.message || msg) is
intentional to ignore JSON parse errors; add a brief inline comment above the
catch explaining this intent (e.g., "ignore parse errors — fallback to default
msg") so readers understand why the catch is empty and no error handling is
performed.
- Around line 156-157: The current validation only checks length after trimming
(const code = input?.value?.trim(); if (!code || code.length !== 6) return;) —
update this to ensure the code is digits-only as well (e.g. test with a regex
like /^\d{6}$/) and return early if the test fails; locate the validation around
the const code = input?.value?.trim() and replace the length-only check with a
digits-plus-length check so non-numeric input is rejected before the API call.
In `@apps/fastify/src/routes/reference/template.ts`:
- Line 18: Replace the dynamic CDN specifier in the script tag string
"@scalar/api-reference@latest/dist/browser/standalone.js" with a pinned version
(for example "@scalar/api-reference@1.25.0/dist/browser/standalone.js" or a
semver-stable spec like "@scalar/api-reference@1.x.x/...") so the template
served by the reference route uses a fixed package version; update the script
src in the template that contains that string and optionally add a comment or
TODO to document the chosen pinned version and process for updating it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9329cf90-ec57-48e4-b8cb-c8437d6e13ec
📒 Files selected for processing (9)
apps/fastify/eslint.config.mjsapps/fastify/src/lib/oauth-google.tsapps/fastify/src/routes/auth/oauth/google/exchange-helpers.tsapps/fastify/src/routes/auth/oauth/google/exchange.tsapps/fastify/src/routes/reference/template-scripts.tsapps/fastify/src/routes/reference/template-styles.tsapps/fastify/src/routes/reference/template.tsapps/next/app/auth/callback/oauth/google/route.tsapps/next/app/auth/login/login-actions.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/next/app/auth/callback/oauth/google/route.ts
- apps/next/app/auth/login/login-actions.tsx
- Re-validate redirectUri from state against allowedUrls in Google exchange - Add googleOverrides map and google_email_required message - Refactor translateOAuthError: resolve base key then apply provider overrides
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/next/lib/auth/auth-error-messages.ts (1)
82-87:⚠️ Potential issue | 🟡 MinorAdd Google overrides for the remaining known base codes.
knownOAuthCodesstill acceptsfetch_user_failedandoauth_failed, butgoogleOverridesdoes not rewrite either one. Becausebody.codetakes precedence overmessageToKey[raw]at Line 102, those valid Google inputs still resolve to the GitHub-branded keys.🔧 Suggested fix
const googleOverrides: Record<string, string> = { token_exchange_failed: 'google_token_exchange_failed', + fetch_user_failed: 'google_fetch_user_failed', user_info_failed: 'google_fetch_user_failed', email_required: 'google_email_required', + oauth_failed: 'oauth_failed_google', }Also applies to: 90-104
🤖 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 82 - 87, googleOverrides is missing mappings for the remaining base OAuth error keys (e.g., fetch_user_failed and oauth_failed), so Google-sent body.code values fall back to GitHub-branded keys via messageToKey/body.code precedence; update the googleOverrides Record to include entries mapping "fetch_user_failed" -> "google_fetch_user_failed" and "oauth_failed" -> "google_oauth_failed" (using the exact base key strings used in knownOAuthCodes/messageToKey) so Google-specific inputs resolve to the correct google_* keys.apps/fastify/src/routes/auth/oauth/google/exchange.ts (1)
110-115:⚠️ Potential issue | 🟠 MajorPreserve the upstream Google status in both catch blocks.
Line 113 and Line 123 still collapse every non-timeout upstream failure into
400. That makes Google401/429/5xxresponses look like permanent client errors and leaves the declared503path effectively unreachable.Also applies to: 120-124
🤖 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/exchange.ts` around lines 110 - 115, The catch blocks currently map all non-timeout token exchange failures to 400; instead, when buildTokenExchangeError(err) returns an error carrying the upstream Google HTTP status (e.g., err.status or err.upstreamStatus), pass that status through to reply.code so 401/429/5xx are preserved; otherwise fall back to the existing timeout->504 / default->400 logic. Update both catch blocks that call buildTokenExchangeError and reply.code to compute status = tokenErr.status || tokenErr.upstreamStatus || (tokenErr.message.includes('timeout') ? 504 : 400) and use reply.code(status).send(tokenErr).
🤖 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/exchange.ts`:
- Around line 87-95: When stateRecord.meta indicates an OAuth link flow
(isLinkMode === true) but linkUserId is missing, explicitly fail closed instead
of falling through: in the handler that reads const { isLinkMode, linkUserId,
stateRecord } add a guard that when isLinkMode is truthy and linkUserId is falsy
you immediately reply with a 401/INVALID_STATE error (same structure used for
other invalid-state responses) to prevent the request from continuing to the
normal sign-in/create branches; apply the same guard logic to the related branch
handling around the sign-in/create flow so oauth_link_state without meta.userId
is always rejected.
- Around line 182-205: When updating an existingAccount, don’t overwrite the
stored refresh token with null when Google omits refresh_token; instead preserve
the DB value. Keep building accountData from tokenData but change the update
logic (the db.update(account).set({...}) block) so refreshToken is only set to
the newly encrypted refresh token when tokenData.refresh_token exists; otherwise
leave the column as account.refreshToken (the existingAccount DB value). Use the
result of encryptAccountTokens for the new value when present: set refreshToken:
tokenData.refresh_token ? encrypted.refreshToken ?? null : account.refreshToken.
---
Duplicate comments:
In `@apps/fastify/src/routes/auth/oauth/google/exchange.ts`:
- Around line 110-115: The catch blocks currently map all non-timeout token
exchange failures to 400; instead, when buildTokenExchangeError(err) returns an
error carrying the upstream Google HTTP status (e.g., err.status or
err.upstreamStatus), pass that status through to reply.code so 401/429/5xx are
preserved; otherwise fall back to the existing timeout->504 / default->400
logic. Update both catch blocks that call buildTokenExchangeError and reply.code
to compute status = tokenErr.status || tokenErr.upstreamStatus ||
(tokenErr.message.includes('timeout') ? 504 : 400) and use
reply.code(status).send(tokenErr).
In `@apps/next/lib/auth/auth-error-messages.ts`:
- Around line 82-87: googleOverrides is missing mappings for the remaining base
OAuth error keys (e.g., fetch_user_failed and oauth_failed), so Google-sent
body.code values fall back to GitHub-branded keys via messageToKey/body.code
precedence; update the googleOverrides Record to include entries mapping
"fetch_user_failed" -> "google_fetch_user_failed" and "oauth_failed" ->
"google_oauth_failed" (using the exact base key strings used in
knownOAuthCodes/messageToKey) so Google-specific inputs resolve to the correct
google_* keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 720776ca-9412-4999-ace5-c3f40e18b45a
📒 Files selected for processing (2)
apps/fastify/src/routes/auth/oauth/google/exchange.tsapps/next/lib/auth/auth-error-messages.ts
- add isLinkMode/linkUserId guard to fail closed on invalid link state - preserve refresh token when Google omits it on existing accounts - pass through upstream HTTP status (401/429/5xx) in token/user catch blocks - add 429 to exchange response schema for rate limits
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
Documentation
UI / Client
Tests