Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughAdds account-linking: change-email flows and OAuth provider linking/unlinking. Introduces DB verification types and consumedAt, guardrail utilities, backend routes for request/verify/link/unlink, frontend components/hooks, email templates, OpenAPI updates, and OAuth state validation/consumption. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant Backend
participant DB
participant Email
participant Session
User->>UI: Request email change (new email)
UI->>Backend: POST /account/email/change/request {email, callbackUrl}
Backend->>DB: ensure rate limit, check email conflict
Backend->>DB: create verification (token hash, verificationId)
Backend->>Email: send change-email (code + link)
Email-->>User: deliver code/link
User->>UI: Submit code/link
UI->>Backend: POST /account/email/change/verify {token or verificationId}
Backend->>DB: validate verification, check expiry, delete/consume record
Backend->>DB: transaction: update user.email, mark verified, rotate refresh token
Backend->>Session: issue new access/refresh tokens
Backend->>Email: send notification to old email (if changed)
Session-->>UI: return tokens
sequenceDiagram
participant User
participant UI
participant LinkAuth
participant OAuthProvider
participant FrontendCallback
participant ExchangeRoute
participant DB
User->>UI: Click "Link Provider"
UI->>LinkAuth: GET /auth/oauth/{provider}/link-authorize-url
LinkAuth->>DB: rate-limit check, insert oauth_link_state (state hash, meta.userId, codeVerifier)
LinkAuth-->>UI: return provider authorize URL with raw state
UI->>OAuthProvider: Redirect user to provider with state
OAuthProvider-->>FrontendCallback: redirect back with code & state
FrontendCallback->>ExchangeRoute: POST /auth/oauth/{provider}/exchange {code, state}
ExchangeRoute->>DB: validateAndConsumeOAuthState(stateHash) -> isLinkMode, linkUserId
ExchangeRoute->>DB: check PROVIDER_ALREADY_LINKED and fetch/create target user
ExchangeRoute->>OAuthProvider: exchange code for tokens
ExchangeRoute->>DB: upsert account row for provider, associate to user
ExchangeRoute->>DB: delete or mark state consumed
ExchangeRoute-->>FrontendCallback: return tokens + redirectTo
FrontendCallback->>UI: redirect to redirectTo (e.g., /settings?linked=ok)
UI->>DB: refresh session/user to show new linked provider
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 (4)
apps/next/eslint.config.js (1)
6-9: Consider if this override is necessary.Per coding guidelines, UI components are exempt from the 300-line file size limit. Since
profile-section.tsxis a UI component, this explicit override may be redundant. However, keeping it provides explicit documentation of expected file size.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/eslint.config.js` around lines 6 - 9, The ESLint override in eslint.config.js specifically targets 'app/(dashboard)/settings/(profile)/profile-section.tsx' to relax the 'max-lines' rule; remove this override entry to avoid redundant special-casing (since UI components are exempt from the 300-line limit), or alternatively preserve it but replace it with a clearer approach: either broaden the files pattern to match all UI components (e.g., a directory pattern) or add an inline comment explaining why 'profile-section.tsx' needs the explicit 'max-lines' override; target the override object that contains the files array and the 'max-lines' rule to apply the chosen change.apps/next/app/auth/callback/oauth/github/route.ts (1)
30-35: Consider a shared OAuth callback success helper.This redirect construction plus cookie-setting is now repeated across the GitHub, Facebook, and Twitter callback routes. A small helper for the success path would reduce drift in redirect/cookie behavior as more providers or callback variants get added.
🤖 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 30 - 35, The redirect + cookie-setting block used in OAuth callbacks is duplicated; extract it into a shared helper (e.g., handleOAuthCallbackSuccess or sendOAuthSuccessRedirect) that accepts the Request/Response context, the provider response, and tokens, constructs the redirect using NextResponse.redirect(new URL(getOAuthRedirectTarget(response), request.url), 303), calls setAuthCookiesOnResponse on that response with tokens, and returns the redirect Response; then replace the duplicated code in the GitHub, Facebook, and Twitter callback routes with a single call to this helper to centralize behavior and reduce drift.apps/next/app/(dashboard)/settings/(profile)/linked-accounts-section.tsx (1)
20-25: Align the rendered provider list with the provider map.
providerLabelsand the unlink path already account forconfiguredProvidersonly renders GitHub/Facebook/Twitter. If Google-linked accounts can already show up inlinkedAccounts, users won't be able to manage them here; otherwise the extra Google plumbing is just dead state. Consider deriving the rendered list from a single typed source of truth.Also applies to: 60-60
🤖 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 20 - 25, The rendered provider list is out of sync with providerLabels (Google present in providerLabels/unlink logic but not in configuredProviders); fix by deriving the UI list from a single source of truth such as Object.keys(providerLabels) (or a typed enum) instead of hard-coded configuredProviders so all providers (including 'google') are rendered consistently; update the rendering logic that uses configuredProviders and the unlink handler (the unlink path code and any checks against linkedAccounts) to accept and handle 'google', and adjust types for configuredProviders/linkedAccounts to include 'google' so the map/filter and unlink functions correctly show and remove Google-linked accounts.apps/fastify/src/routes/auth/oauth/github/exchange.ts (1)
98-225: Extract the new link-resolution block into a shared helper.This addition pushes the route past the 300-line cap and duplicates most of the provider-link resolution now living in the other OAuth exchange handlers. Pull the state validation and user/account resolution into a shared helper before the providers drift.
As per coding guidelines, "LINTING: File size limit is 300 lines (exempt: test files and UI components)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/auth/oauth/github/exchange.ts` around lines 98 - 225, The route now contains duplicated state validation and user/account resolution logic (isLinkMode, linkUserId, deletion of verification, token/user/email handling, existingAccount checks, user creation using randomUUID and generateFunnyUsername) which pushes the file >300 lines; extract that block into a shared helper (e.g., resolveOAuthStateAndUser or resolveAndLinkOAuthAccount) that accepts the db, stateRecord (or state id), provider identifier ('github'), accountId, ghUser/email and returns { user, existingAccount, isLinkMode } or throws/replies on errors; replace the in-place logic in exchange.ts with a call to that helper and reuse it from other OAuth exchange handlers to remove duplication and keep files under the linting size limit.
🤖 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/docu/content/docs/architecture/authentication.mdx`:
- Line 361: The summary of callback pages is stale: update the listed OAuth
callback route in the sentence that currently enumerates
`/auth/callback/magiclink`, `/auth/callback/change-email`,
`/auth/callback/oauth/github`, `/auth/callback/web3`, `/auth/callback/passkey`
so it no longer hard-codes only GitHub; replace `/auth/callback/oauth/github`
with a generic `/auth/callback/oauth/[provider]` (or explicitly enumerate all
currently supported providers such as `/auth/callback/oauth/github`,
`/auth/callback/oauth/facebook`, `/auth/callback/oauth/twitter`) and ensure the
rest of the sentence (and the parallel mention of `/auth/logout`) remains
unchanged.
- Line 327: The docs incorrectly state that GET
/auth/oauth/{github,facebook,twitter}/link-authorize-url is available to "Bearer
— JWT or API key" clients, but the handler reads request.session.user.id
(session-only), so update the bullet to indicate session-based auth only (e.g.,
"Session required — cookie/JWT session (no API key)") and change the auth note
on that route to "Session" instead of "Bearer (JWT or API key)"; ensure the
route entry for GET /auth/oauth/.../link-authorize-url returns { redirectUrl }
and keeps the same rate limit but clearly documents that API-key clients cannot
initiate the flow.
In `@apps/fastify/eslint.config.mjs`:
- Around line 6-18: Remove the per-file ESLint overrides from eslint.config.mjs
(the block that lists src/routes/auth/oauth/*/exchange.ts and
src/lib/oauth-twitter.ts) and restore the repo default max-lines/max-params
rules; refactor the large route handlers (exchange.ts files) and the oauth
helper (oauth-twitter.ts) by extracting shared logic into smaller helper
functions/modules (e.g., create separate oauth/token-parsing,
oauth/provider-client, and oauth/error-handling utilities) so each file falls
under the 300-line limit and no per-file lint exemptions are required.
In `@apps/fastify/src/routes/account/email/change/request.ts`:
- Around line 105-135: Wrap the render() and
fastify.emailProvider.emails.send(...) calls in a try/catch and, on any error,
delete the previously inserted verification row (match by verificationId and
type 'change_email' and identifier) using the same db instance before rethrowing
the error; ensure you perform the cleanup whether render() fails or email
sending fails so orphaned rows don't count toward the rate limit, and keep the
insertion of tokenPlain conditional behavior unchanged.
In `@apps/fastify/src/routes/auth/oauth/facebook/exchange.ts`:
- Around line 77-82: The exchange path currently accepts verification.type
'oauth_link_state' and, after loading stateRecord.meta.userId, issues JWTs
without validating the current session; to fix this require the active session
to match the linked user before honoring link mode: when handling
oauth_link_state (and in the same logic branches referenced at lines 98-99,
183-221, 290-295), check request.session (or session.userId) equals
stateRecord.meta.userId (linkUserId) and reject the exchange if not;
alternatively only allow oauth_link_state through if request.session exists and
matches linkUserId, otherwise treat it as invalid state and deny/token error.
Ensure the verification.where clause (verification.type) still distinguishes
'oauth_state' vs 'oauth_link_state' but gate the link flow with the session
equality check before issuing fresh JWTs.
In `@apps/fastify/src/routes/auth/oauth/facebook/link-authorize-url.ts`:
- Around line 55-65: The current per-hour cap in link-authorize-url.ts is
counting live verification rows (query using verification, oauth_link_state and
identifier `link:${userId}`) but those rows are deleted by the exchange handler
so the limit can be bypassed; change the design to use a durable attempt log or
mark consumed attempts instead of deleting them: add or reuse a persistent
table/column (e.g., verification_attempts or add consumedAt on verification) and
record an immutable attempt entry for each start (or set consumedAt but keep the
row until window expires), then change the counting query (the code that
computes recentCount and uses linkAuthorizeUrlPerUserPerHour) to count those
durable attempt records within the one-hour window so completed flows don't
remove entries used for rate-limiting.
In `@apps/fastify/src/routes/auth/oauth/github/exchange.ts`:
- Around line 77-82: When handling verification.type === 'oauth_link_state' (the
logic that reads stateRecord.meta.userId into linkUserId) ensure the callback is
bound to the current session: check request.session?.user.id === linkUserId and
fail/redirect if not, and do not proceed to mint JWTs or create a fresh login
session from that code path; instead return a link-confirmation/redirect
response that requires the existing session to complete linking. Apply this same
session-check and no-token-minting restriction to every branch that handles
'oauth_link_state' (the blocks that resolve linkUserId from
stateRecord.meta.userId and then fall through to create the normal JWT response)
so minting functions (e.g., anything that creates JWTs/tokens or sets a
logged-in session) are never called for link-state handling unless the session
user id matches linkUserId.
In `@apps/fastify/src/routes/auth/oauth/twitter/exchange.ts`:
- Around line 98-100: Detect when stateRecord.type === 'oauth_link_state' and
ensure stateRecord.meta?.userId is present before consuming the DB row; if
meta.userId is missing, immediately return/throw INVALID_STATE and do not call
db.delete(verification).where(eq(verification.id, stateRecord.id)). Concretely,
modify the logic around isLinkMode and linkUserId to validate linkUserId and
short-circuit with INVALID_STATE when absent (before calling db.delete or
invoking runTwitterExchangeTx), so runTwitterExchangeTx only ever receives a
valid linkUserId for link flows.
In `@apps/fastify/src/routes/auth/oauth/twitter/link-authorize-url.ts`:
- Around line 62-72: The hourly-cap check currently counts only live
verification rows of type 'oauth_link_state' (recentCount) which gets deleted on
successful exchange, allowing bypass; change the design so attempts are
append-only or consumed rows are retained: either (A) create/ use an append-only
table (e.g., link_attempts) and switch the counting query in the link-authorize
flow to count rows in that table for identifier `link:${userId}`, and insert a
new attempt row on state creation (and on completion), or (B) stop deleting
verification rows in the exchange handler and instead update a consumed
flag/timestamp on the verification row (modify the exchange handler that deletes
oauth_link_state to perform an UPDATE to set consumedAt/consumed=true and
optionally insert an audit row), then update the count query (recentCount) to
count both consumed and unconsumed verification rows created within the hour;
update functions referenced: the code creating the state (link-authorize-url)
and the exchange handler that currently deletes the state (exchange.ts) to
implement the chosen approach.
In `@apps/fastify/src/routes/auth/session/user.ts`:
- Around line 88-92: The query fetching accountRows via
db.select(...).from(account).where(eq(account.userId, userId)) can return
multiple rows per providerId; deduplicate providerId values before assigning
linkedAccounts so the response contains unique providers only. Modify the
mapping in the block that defines linkedAccounts (using accountRows) to produce
a unique list—e.g., collect providerId into a Set or group by providerId, then
map to objects like { providerId }—so linkedAccounts contains one entry per
provider.
In `@apps/next/app/`(dashboard)/settings/(profile)/change-email-block.tsx:
- Around line 32-35: The handleVerify callback currently awaits
changeEmail.verify(...) but swallows rejections; wrap the call in a try/catch
inside handleVerify to catch errors from changeEmail.verify({ token:
code.trim(), email: newEmail.trim()}) and display failure feedback (e.g., call
the existing toast/error utility or set a local error state used by the
component). Specifically, modify the handleVerify function to validate inputs,
try { await changeEmail.verify(...) ; proceed on success } catch (err) { show a
toast.error or setError(err.message || 'Verification failed') } so the user sees
a clear error when the code is invalid or expired.
In `@apps/next/app/auth/callback/change-email/route.ts`:
- Around line 32-37: The createClient is using JWT mode with a no-op
onTokensRefreshed so refreshed tokens are lost; implement onTokensRefreshed to
accept the new tokens and store them in a variable (e.g.,
refreshedAuthToken/refreshedRefreshToken) accessible to the route handler, then
when returning either the success or error redirect response apply those
refreshed tokens to the response (set the cookies or headers) instead of the
original authToken/refreshToken; update the onTokensRefreshed callback in the
createClient call to capture tokens and ensure the final Response returned by
the route uses those captured tokens when setting cookies.
In `@packages/react/src/hooks/use-change-email.ts`:
- Around line 58-62: The onSuccess handler calls config?.onVerifySuccess?.(data)
but does not await it, causing
queryClient.invalidateQueries(['auth','session','user']) and
['auth','session','jwt'] to run against stale cookies; make the onSuccess
callback async and await the result of config.onVerifySuccess (i.e., await
config?.onVerifySuccess?.(data)) before calling queryClient.invalidateQueries so
the cookie/write performed in the caller (e.g., change-email-block) completes
first.
- Around line 65-71: The requestChange function currently swallows all errors
from requestMutation.mutateAsync, causing callers like ChangeEmailBlock to treat
failures as successes; modify requestChange (in use-change-email.ts) to not
silently catch errors—either remove the try/catch so the mutateAsync error
propagates, or catch and re-throw the caught error (or return a typed { success:
boolean, error?: Error } result and update callers to branch on it); ensure the
function's signature and callers (e.g., ChangeEmailBlock) are updated
accordingly so 409/429/provider failures are handled as failures rather than
advancing the UI.
In `@packages/react/src/hooks/use-oauth-link.ts`:
- Around line 19-22: The call to endpoints[provider]() in use-oauth-link is
swallowing HTTP errors and causing a misleading validation error; update the
invocation to pass throwOnError: true so the API client throws on non-2xx
responses (e.g., change the call to endpoints[provider]({ throwOnError: true })
or equivalent) and let that error surface instead of proceeding to the
redirectUrl validation; keep the rest of the validation (url check and error
message) as-is.
---
Nitpick comments:
In `@apps/fastify/src/routes/auth/oauth/github/exchange.ts`:
- Around line 98-225: The route now contains duplicated state validation and
user/account resolution logic (isLinkMode, linkUserId, deletion of verification,
token/user/email handling, existingAccount checks, user creation using
randomUUID and generateFunnyUsername) which pushes the file >300 lines; extract
that block into a shared helper (e.g., resolveOAuthStateAndUser or
resolveAndLinkOAuthAccount) that accepts the db, stateRecord (or state id),
provider identifier ('github'), accountId, ghUser/email and returns { user,
existingAccount, isLinkMode } or throws/replies on errors; replace the in-place
logic in exchange.ts with a call to that helper and reuse it from other OAuth
exchange handlers to remove duplication and keep files under the linting size
limit.
In `@apps/next/app/`(dashboard)/settings/(profile)/linked-accounts-section.tsx:
- Around line 20-25: The rendered provider list is out of sync with
providerLabels (Google present in providerLabels/unlink logic but not in
configuredProviders); fix by deriving the UI list from a single source of truth
such as Object.keys(providerLabels) (or a typed enum) instead of hard-coded
configuredProviders so all providers (including 'google') are rendered
consistently; update the rendering logic that uses configuredProviders and the
unlink handler (the unlink path code and any checks against linkedAccounts) to
accept and handle 'google', and adjust types for
configuredProviders/linkedAccounts to include 'google' so the map/filter and
unlink functions correctly show and remove Google-linked accounts.
In `@apps/next/app/auth/callback/oauth/github/route.ts`:
- Around line 30-35: The redirect + cookie-setting block used in OAuth callbacks
is duplicated; extract it into a shared helper (e.g., handleOAuthCallbackSuccess
or sendOAuthSuccessRedirect) that accepts the Request/Response context, the
provider response, and tokens, constructs the redirect using
NextResponse.redirect(new URL(getOAuthRedirectTarget(response), request.url),
303), calls setAuthCookiesOnResponse on that response with tokens, and returns
the redirect Response; then replace the duplicated code in the GitHub, Facebook,
and Twitter callback routes with a single call to this helper to centralize
behavior and reduce drift.
In `@apps/next/eslint.config.js`:
- Around line 6-9: The ESLint override in eslint.config.js specifically targets
'app/(dashboard)/settings/(profile)/profile-section.tsx' to relax the
'max-lines' rule; remove this override entry to avoid redundant special-casing
(since UI components are exempt from the 300-line limit), or alternatively
preserve it but replace it with a clearer approach: either broaden the files
pattern to match all UI components (e.g., a directory pattern) or add an inline
comment explaining why 'profile-section.tsx' needs the explicit 'max-lines'
override; target the override object that contains the files array and the
'max-lines' rule to apply the chosen change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e9d01a78-809a-4a5d-8c9e-abaa3f08c273
⛔ Files ignored due to path filters (5)
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 (36)
apps/docu/content/docs/architecture/account-linking.mdxapps/docu/content/docs/architecture/authentication.mdxapps/docu/content/docs/architecture/index.mdxapps/docu/content/docs/architecture/meta.jsonapps/fastify/eslint.config.mjsapps/fastify/openapi/openapi.jsonapps/fastify/src/db/schema/tables/auth-attempts.tsapps/fastify/src/db/schema/tables/verification.tsapps/fastify/src/lib/auth-guardrails.tsapps/fastify/src/lib/email.tsapps/fastify/src/lib/oauth-twitter.tsapps/fastify/src/routes/account/email/change/request.tsapps/fastify/src/routes/account/email/change/verify.tsapps/fastify/src/routes/account/link/oauth/unlink.tsapps/fastify/src/routes/auth/oauth/facebook/exchange.tsapps/fastify/src/routes/auth/oauth/facebook/link-authorize-url.tsapps/fastify/src/routes/auth/oauth/github/exchange.tsapps/fastify/src/routes/auth/oauth/github/link-authorize-url.tsapps/fastify/src/routes/auth/oauth/twitter/exchange.tsapps/fastify/src/routes/auth/oauth/twitter/link-authorize-url.tsapps/fastify/src/routes/auth/session/user.tsapps/next/app/(dashboard)/settings/(profile)/change-email-block.tsxapps/next/app/(dashboard)/settings/(profile)/linked-accounts-section.tsxapps/next/app/(dashboard)/settings/(profile)/profile-section.tsxapps/next/app/auth/callback/change-email/route.tsapps/next/app/auth/callback/oauth/facebook/route.tsapps/next/app/auth/callback/oauth/github/route.tsapps/next/app/auth/callback/oauth/twitter/route.tsapps/next/eslint.config.jsapps/next/lib/auth/callback-utils.tspackages/email/emails/change-email.tsxpackages/email/emails/email-changed-notification.tsxpackages/react/src/hooks/use-change-email.tspackages/react/src/hooks/use-oauth-link.tspackages/react/src/hooks/use-oauth-unlink.tspackages/react/src/index.ts
- Docs: callback routes enum (github,facebook,twitter), link-authorize-url session-only - Change email request: try/catch with verification cleanup on render/send error - OAuth link: session validation for oauth_link_state, pass Bearer from callback - Durable rate limit: consumedAt on verification, update instead of delete for oauth_link_state - Session user: deduplicate linkedAccounts by providerId - Change email block: try/catch for request/verify, toast on error - Change email callback: onTokensRefreshed to capture refreshed tokens - use-change-email: await onVerifySuccess, propagate requestChange errors - use-oauth-link: throwOnError for API client - Extract validateAndConsumeOAuthState helper, remove max-lines override - linked-accounts-section: derive providers from providerLabels, Google coming soon - Twitter exchange: validate linkUserId before consume
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
apps/docu/content/docs/architecture/authentication.mdx (1)
136-139:⚠️ Potential issue | 🟡 MinorKeep the narrative section session-only here too.
This paragraph still describes
GET /auth/oauth/{provider}/link-authorize-urlas(Bearer)/Bearer required, but the endpoint table at Line 327 and the route handlers require an authenticated session user, not an API key. Please mirror the “session/JWT required, no API key” wording here so the two sections don’t contradict each other.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/docu/content/docs/architecture/authentication.mdx` around lines 136 - 139, Update the narrative to match the endpoint table and route handlers by changing the authentication wording for GET /auth/oauth/{provider}/link-authorize-url from "(Bearer)" to "session/JWT required, no API key" (or similar session-only phrasing); keep the rest of the flow description (oauth_link_state stored with meta.userId, Exchange logic branching on oauth_link_state vs oauth_state, link mode using meta.userId, and 409 PROVIDER_ALREADY_LINKED behavior) unchanged so the document is consistent with the route handlers.
🤖 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/lib/oauth-exchange-state.ts`:
- Around line 25-33: The current select and later consume flow (stateRecord from
verification where verification.value = stateHash and type in
['oauth_state','oauth_link_state']) is racy and allows double-use; modify the
load-and-consume to be atomic by including consumedAt IS NULL in the initial
WHERE (i.e., only select records with consumedAt == null) and then perform the
consume as an UPDATE that sets consumedAt (and any other consumed metadata) with
a WHERE that includes the same id/value AND consumedAt IS NULL (or do both
actions inside a transaction and use RETURNING or check affected row count) so
that concurrent requests cannot both succeed; update the logic around the
consumption code that writes consumedAt (the later block around lines 71-76) to
check the UPDATE affected rows and fail if zero to ensure single-use.
In `@apps/fastify/src/routes/account/email/change/request.ts`:
- Around line 83-91: The handler currently lets a user request an email change
to the same address because the check only rejects when another user has that
email; add a server-side no-op guard: after fetching existingByEmail (and/or
loading the current user by userId), if normalizedEmail equals the current
user's email (i.e., existingByEmail && existingByEmail.id === userId or compare
against fetched currentUser.email), return an error (e.g., 400 with code
'EMAIL_NOT_CHANGED' and a clear message) and do not create the change_email
verification or send mail; update the logic around existingByEmail,
normalizedEmail and userId to short-circuit in that case.
In `@apps/fastify/src/routes/auth/oauth/github/exchange.ts`:
- Around line 187-200: The GitHub exchange path (the select -> insert -> select
block using users, variable u, randomUUID(), and generateFunnyUsername()) is
racy and must be made idempotent: replace the current create flow with the same
find-or-create flow used by the Facebook exchange or perform an upsert+reselect
(insert ... onConflict do nothing or update, then reselect by email or conflict
key) so concurrent callbacks cannot create duplicate users; ensure you still
generate a username when inserting and after the upsert re-query the users table
(eq(users.email, email) or eq(users.id, newUserId)) and throw only if the final
select returns no user.
In `@apps/next/app/`(dashboard)/settings/(profile)/change-email-block.tsx:
- Around line 50-57: The inputs rely only on placeholders so they lack
programmatic labels; add accessible names by giving each Input a stable id and
either a visible <label htmlFor="..."> or an aria-label/aria-labelledby prop.
Concretely, update the verification code Input (the one with value={code} and
onChange={e => setCode(...)}) to include id="verification-code" and a matching
<label> or aria-label="Verification code (6 digits)"; do the same for the
new-email Input (the one handling the new email, e.g., value/email and its
setter) with id="new-email" and label or aria-label="New email address" so
screen readers get a stable name.
---
Duplicate comments:
In `@apps/docu/content/docs/architecture/authentication.mdx`:
- Around line 136-139: Update the narrative to match the endpoint table and
route handlers by changing the authentication wording for GET
/auth/oauth/{provider}/link-authorize-url from "(Bearer)" to "session/JWT
required, no API key" (or similar session-only phrasing); keep the rest of the
flow description (oauth_link_state stored with meta.userId, Exchange logic
branching on oauth_link_state vs oauth_state, link mode using meta.userId, and
409 PROVIDER_ALREADY_LINKED behavior) unchanged so the document is consistent
with the route handlers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5af0eb48-54d8-4292-9358-40c0b33b1b4b
⛔ Files ignored due to path filters (3)
apps/fastify/src/db/migrations/0016_easy_chameleon.sqlis excluded by!apps/fastify/src/db/migrations/**apps/fastify/src/db/migrations/meta/0016_snapshot.jsonis excluded by!apps/fastify/src/db/migrations/**apps/fastify/src/db/migrations/meta/_journal.jsonis excluded by!apps/fastify/src/db/migrations/**
📒 Files selected for processing (18)
apps/docu/content/docs/architecture/authentication.mdxapps/fastify/eslint.config.mjsapps/fastify/src/db/schema/tables/verification.tsapps/fastify/src/lib/oauth-exchange-state.tsapps/fastify/src/routes/account/email/change/request.tsapps/fastify/src/routes/auth/oauth/facebook/exchange.tsapps/fastify/src/routes/auth/oauth/github/exchange.tsapps/fastify/src/routes/auth/oauth/twitter/exchange.tsapps/fastify/src/routes/auth/session/user.tsapps/next/app/(dashboard)/settings/(profile)/change-email-block.tsxapps/next/app/(dashboard)/settings/(profile)/linked-accounts-section.tsxapps/next/app/auth/callback/change-email/route.tsapps/next/app/auth/callback/oauth/facebook/route.tsapps/next/app/auth/callback/oauth/github/route.tsapps/next/app/auth/callback/oauth/twitter/route.tsapps/next/eslint.config.jspackages/react/src/hooks/use-change-email.tspackages/react/src/hooks/use-oauth-link.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/next/app/auth/callback/change-email/route.ts
- apps/next/app/auth/callback/oauth/github/route.ts
- oauth-exchange-state: atomic consume with consumedAt IS NULL to prevent double-use - account/email/change: add EMAIL_NOT_CHANGED guard when new email equals current - github exchange: use findOrCreateUserByEmail for idempotent user creation - change-email-block: add id and aria-label to verification code and new-email inputs - docu: align link-authorize-url auth wording (session/JWT, no API key)
This was referenced Mar 10, 2026
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