fix(auth): address reviewer feedback and harden auth flows#143
Conversation
- Fix open redirect issues in login and signup - Improve error handling and loading state safety - Ensure safe base URL resolution for auth emails - Fix date handling in token expiry helpers - Remove neon-http transaction usage and debug logs - Resolve Next.js 15 and strict TypeScript issues
✅ Deploy Preview for develop-devlovers ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughStandardizes auth request handling with try/catch/finally, adds safe redirect validation and base URL resolution, exports/adjusts date utilities, and removes transaction wrappers around paired user+token DB operations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant App as Frontend App
participant API
participant DB
User->>Browser: Submit login form
Browser->>App: onSubmit(login, returnTo)
App->>App: validate returnTo via isSafeRedirectUrl
App->>API: POST /api/auth/login (credentials)
API->>DB: verify credentials
DB-->>API: auth result
API-->>App: 200 OK (user) / error
alt login success
App->>App: check sessionStorage for pending quiz
alt pending quiz exists
App->>API: POST /api/quiz/result
API->>DB: store quiz result
DB-->>API: stored
API-->>App: quiz response
App->>Browser: store/clear sessionStorage pending result
end
App->>Browser: redirect to safe returnTo or /<locale>/dashboard
else login failure
App->>Browser: show specific error (EMAIL_NOT_VERIFIED / invalid)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✏️ Tip: You can disable this entire section by setting 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: 6
🤖 Fix all issues with AI agents
In `@frontend/app/`[locale]/forgot-password/page.tsx:
- Around line 34-37: The catch block in forgot-password/page.tsx currently
injects the caught error into the user-facing message via setError; remove the
`${err}` from the string so the displayed message remains generic (e.g.,
"Network error. Please check your connection and try again.") and log the full
error internally for diagnostics (use console.error or your app logger inside
the same catch). Update the catch surrounding the async request in the
forgot-password page where setError is called to only show the generic message
and ensure the raw error is recorded to logs but not rendered in the UI.
In `@frontend/app/`[locale]/login/page.tsx:
- Around line 14-18: The isSafeRedirectUrl function currently allows
protocol-relative URLs like "//evil.com/path" because it only checks
startsWith("/") and for "://"; update isSafeRedirectUrl to explicitly reject
protocol-relative URLs by returning false if the url startsWith("//"), keeping
the existing checks (reject non-leading-slash and strings containing "://");
reference the function name isSafeRedirectUrl and mirror the same
protocol-relative guard used on the signup page to prevent open redirects.
- Around line 48-123: The outer try/finally around the login fetch has no catch,
so network errors will bubble uncaught; add a catch block after the try to
handle errors from the initial fetch (and any code before the finally), logging
the error and calling setErrorMessage with a user-friendly message and
setErrorCode(null) as appropriate; keep the existing finally that calls
setLoading(false). Target the block around the POST to "/api/auth/login" (the
code that awaits fetch and reads res.json()), and ensure you still perform the
pending quiz flow (getPendingQuizResult) only when the request succeeds.
In `@frontend/app/`[locale]/reset-password/page.tsx:
- Around line 1-3: The export const dynamic = "force-dynamic" in
frontend/app/[locale]/reset-password/page.tsx is ignored because the file is a
Client Component ("use client"); either remove the "use client" directive to
make page.tsx a Server Component and keep the dynamic export, or move the
dynamic export to a parent server layout or create a Server Component wrapper
(e.g., export default function ResetPasswordPage() { return
<ResetPasswordClient/> } with export const dynamic = "force-dynamic") and keep
the existing client-only UI in a separate ResetPasswordClient client component.
In `@frontend/app/api/auth/password-reset/confirm/route.ts`:
- Around line 60-67: The current code runs the password update
(db.update(users).set({ passwordHash }).where(eq(users.id, userId))) and token
deletion (db.delete(passwordResetTokens).where(eq(passwordResetTokens.token,
token))) separately, causing a race where a failed delete leaves a valid reset
token; reintroduce atomicity by wrapping both operations in a transaction (use
sql.transaction(...) or your Drizzle/sql transaction helper from
`@neondatabase/serverless`) so the update and delete commit together, and if that
driver support is unavailable then at minimum reorder the operations to delete
the passwordResetTokens token first and only update the user's passwordHash
after a successful delete, or add a clear code comment documenting the accepted
tradeoff.
In `@frontend/lib/http/getBaseUrl.ts`:
- Line 8: The fallback that builds the base URL currently hardcodes "http://"
using options.host; update the getBaseUrl logic to derive the scheme instead:
prefer an explicit options.scheme or an options.forwardedProto (from
x-forwarded-proto) if present, fall back to "https://" rather than "http://",
and then concatenate with options.host; adjust any references to options.host
and the getBaseUrl function to use this scheme selection so production
environments default to HTTPS or follow the forwarded proto header.
🧹 Nitpick comments (7)
frontend/lib/http/getBaseUrl.ts (1)
5-12: Consider validating the resulting URL.The function trusts the
originandhostinputs without validation. Malicious or malformed header values could produce invalid or unsafe URLs. A quicknew URL(base)check would catch malformed inputs early.Proposed validation
const base = options.origin || process.env.NEXT_PUBLIC_APP_URL || (options.host ? `https://${options.host}` : null); if (!base) { throw new Error("Unable to determine base URL"); } + try { + new URL(base); + } catch { + throw new Error("Invalid base URL"); + } + return base.replace(/\/$/, "");frontend/app/api/auth/password-reset/route.ts (1)
52-56: Missing semicolons and potential unhandled exception.Lines 52 and 56 are missing semicolons, which is inconsistent with the rest of the file. Additionally,
resolveBaseUrlcan throw if no base URL is determined—consider wrapping in try/catch to return a controlled error response.Proposed fix
- const h = await headers() + const h = await headers(); const baseUrl = resolveBaseUrl({ origin: h.get("origin"), host: h.get("host"), - }) + });frontend/app/api/auth/resend-verification/route.ts (1)
17-18: Minor: Extra blank lines.These extra blank lines appear to be formatting noise. Consider removing for consistency.
frontend/lib/auth/password-reset.ts (1)
5-9: DuplicateaddHoursimplementation.This function is identical to the one in
frontend/lib/auth/email-verification.ts. Consider extracting it to a shared utility (e.g.,@/lib/utils/date.ts) to avoid duplication and ensure consistent behavior.Suggested approach
Create a shared utility:
// frontend/lib/utils/date.ts export function addHours(date: Date, hours: number): Date { const result = new Date(date.getTime()); result.setHours(result.getHours() + hours); return result; }Then import from both
password-reset.tsandemail-verification.ts:-export function addHours(date: Date, hours: number): Date { - const result = new Date(date.getTime()); - result.setHours(result.getHours() + hours); - return result; -} +import { addHours } from "@/lib/utils/date";frontend/lib/auth/email-verification.ts (1)
5-9: DuplicatedaddHoursutility across files.This exact implementation exists in
frontend/lib/auth/password-reset.ts(lines 4-8). Consider extracting to a shared date utility module to avoid duplication.Example shared utility location
// frontend/lib/utils/date.ts export function addHours(date: Date, hours: number): Date { const result = new Date(date.getTime()); result.setHours(result.getHours() + hours); return result; }Then import from both
email-verification.tsandpassword-reset.ts.frontend/app/api/auth/signup/route.ts (1)
10-11: Import organization suggestion.Consider grouping Next.js imports together at the top. The
headersimport fromnext/headersis separated from theNextResponseimport.Optional reorganization
-import { NextResponse } from "next/server"; +import { NextResponse } from "next/server"; +import { headers } from "next/headers"; import bcrypt from "bcryptjs"; import { z } from "zod"; import { eq } from "drizzle-orm"; import { db } from "@/db"; import { users } from "@/db/schema/users"; import { createEmailVerificationToken } from "@/lib/auth/email-verification"; import { sendVerificationEmail } from "@/lib/email/sendVerificationEmail"; -import { headers } from "next/headers"; import { resolveBaseUrl } from "@/lib/http/getBaseUrl";frontend/app/[locale]/signup/page.tsx (1)
10-19: Consider extractingisSafeRedirectUrlto a shared utility.This helper is duplicated in
login/page.tsx(and the login version is missing thestartsWith("//")check). Extracting to a shared module (e.g.,lib/auth/redirect.ts) would ensure consistent security logic across all auth pages.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
frontend/app/[locale]/forgot-password/page.tsxfrontend/app/[locale]/login/page.tsxfrontend/app/[locale]/reset-password/page.tsxfrontend/app/[locale]/signup/page.tsxfrontend/app/api/auth/password-reset/confirm/route.tsfrontend/app/api/auth/password-reset/route.tsfrontend/app/api/auth/resend-verification/route.tsfrontend/app/api/auth/signup/route.tsfrontend/app/api/auth/verify-email/route.tsfrontend/lib/auth/email-verification.tsfrontend/lib/auth/password-reset.tsfrontend/lib/http/getBaseUrl.ts
🧰 Additional context used
🧬 Code graph analysis (9)
frontend/app/api/auth/password-reset/confirm/route.ts (2)
frontend/db/schema/users.ts (1)
users(10-41)frontend/db/schema/passwordResetTokens.ts (1)
passwordResetTokens(3-18)
frontend/app/[locale]/login/page.tsx (1)
frontend/lib/quiz/guest-quiz.ts (2)
getPendingQuizResult(27-49)clearPendingQuizResult(51-54)
frontend/lib/auth/password-reset.ts (1)
frontend/lib/auth/email-verification.ts (1)
addHours(5-9)
frontend/app/api/auth/verify-email/route.ts (3)
frontend/db/index.ts (1)
db(54-54)frontend/db/schema/users.ts (1)
users(10-41)frontend/db/schema/emailVerificationTokens.ts (1)
emailVerificationTokens(3-18)
frontend/app/api/auth/password-reset/route.ts (2)
frontend/lib/http/getBaseUrl.ts (1)
resolveBaseUrl(1-15)frontend/lib/email/sendPasswordResetEmail.ts (1)
sendPasswordResetEmail(9-26)
frontend/app/[locale]/signup/page.tsx (1)
frontend/components/auth/OAuthButtons.tsx (1)
OAuthButtons(5-21)
frontend/lib/auth/email-verification.ts (1)
frontend/lib/auth/password-reset.ts (1)
addHours(5-9)
frontend/app/api/auth/signup/route.ts (2)
frontend/lib/http/getBaseUrl.ts (1)
resolveBaseUrl(1-15)frontend/lib/email/sendVerificationEmail.ts (1)
sendVerificationEmail(9-25)
frontend/app/api/auth/resend-verification/route.ts (2)
frontend/lib/http/getBaseUrl.ts (1)
resolveBaseUrl(1-15)frontend/lib/email/sendVerificationEmail.ts (1)
sendVerificationEmail(9-25)
🔇 Additional comments (20)
frontend/app/api/auth/password-reset/route.ts (1)
17-62: LGTM on the URL construction refactor.The switch to
resolveBaseUrlfor constructing the reset URL is a good improvement for consistency across auth routes. The logic correctly prioritizes origin, then falls back to environment variable or host.frontend/app/api/auth/resend-verification/route.ts (1)
59-67: LGTM on base URL resolution.The refactor to use
resolveBaseUrlis consistent with the pattern applied across other auth routes, improving maintainability and ensuring reliable URL construction across deployments.frontend/lib/auth/password-reset.ts (1)
11-23: LGTM on token creation logic.The
createPasswordResetTokenfunction correctly uses the immutableaddHourshelper and generates a secure random UUID for the token.frontend/lib/auth/email-verification.ts (1)
11-21: LGTM!Token generation and storage logic is correct. Uses
crypto.randomUUID()for secure token generation and properly calculates expiration 24 hours from creation.frontend/app/[locale]/forgot-password/page.tsx (1)
21-40: Good error handling pattern with try/catch/finally.The structure correctly ensures
setLoading(false)runs regardless of success or failure, and separates HTTP errors from network errors appropriately.frontend/app/api/auth/verify-email/route.ts (1)
46-53: Non-atomic operations may leave orphan tokens on partial failure.Removing the transaction wrapper means if the user update succeeds but the token deletion fails, the token remains in the database. While the user is correctly verified and the token would eventually expire, this could leave stale data.
If this is intentional due to neon-http transaction limitations (as mentioned in commit message), the risk is acceptable since:
- User verification state is the critical operation
- Orphan tokens expire naturally and can't cause harm
Please confirm this trade-off is acceptable for your database driver constraints.
frontend/app/api/auth/signup/route.ts (1)
67-75: LGTM! Robust base URL resolution.The
resolveBaseUrlutility correctly handles the origin/host fallback chain withNEXT_PUBLIC_APP_URLas a safe default. This ensures verification links work across different deployment environments.frontend/app/[locale]/reset-password/page.tsx (1)
31-54: LGTM! Clean error handling with try/catch/finally.The pattern correctly:
- Distinguishes HTTP errors from network failures
- Ensures loading state is always cleared
- Avoids leaking error details to users (unlike the forgot-password page)
frontend/app/[locale]/login/page.tsx (6)
7-10: LGTM!Imports are correctly structured and align with the quiz result handling logic added below.
24-26: LGTM!Clean normalization that ensures
returnTois always a string, simplifying downstream usage.
73-114: LGTM!The quiz result handling is well-structured:
- Proper try/catch/finally ensures cleanup runs regardless of success/failure
- Errors are logged but don't block the login flow
clearPendingQuizResult()in finally prevents stale data accumulation
116-120: LGTM!Safe redirect pattern with appropriate fallback. The validation approach is correct (pending the protocol-relative URL fix noted above).
173-175: LGTM!Proper dynamic aria-label for accessibility.
187-189: LGTM!Correct use of
encodeURIComponentto safely pass returnTo through the forgot-password flow.frontend/app/[locale]/signup/page.tsx (6)
24-26: LGTM!Consistent returnTo normalization pattern.
45-85: LGTM!Well-structured error handling with try/catch/finally:
- Network errors display user-friendly messages
- API errors show server-provided error or fallback
- Loading state consistently reset in finally
94-106: LGTM!Good UX improvement—hiding OAuth options after signup initiation keeps the verification message prominent.
151-158: LGTM!Simplified password input with
minLengthvalidation. Removing the toggle reduces complexity for the signup form.
123-125: LGTM!Proper URL encoding for returnTo parameter.
184-186: LGTM!Consistent returnTo encoding across all navigation links.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/[locale]/login/page.tsx (1)
138-150: Missing error handling inresendVerificationis inconsistent with PR goals.The PR objective states it wraps network requests in try/catch/finally to surface user-friendly error messages, but this function lacks that protection. If the fetch throws (network failure), users receive no feedback.
🐛 Proposed fix
async function resendVerification() { if (!email) return; - await fetch("/api/auth/resend-verification", { - method: "POST", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ email }), - }); - - setVerificationSent(true); - setErrorCode(null); - setErrorMessage(null); + try { + await fetch("/api/auth/resend-verification", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ email }), + }); + + setVerificationSent(true); + setErrorCode(null); + setErrorMessage(null); + } catch { + setErrorMessage( + "Failed to resend verification. Please try again." + ); + } }
♻️ Duplicate comments (1)
frontend/lib/http/getBaseUrl.ts (1)
1-15: Overall structure is sound, but thehttp://fallback concern remains.The function correctly prioritizes origin over environment variable over host fallback, and properly strips trailing slashes. However, the hardcoded
http://on line 8 was flagged in a previous review and remains unaddressed—this could cause issues in production HTTPS environments.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/app/[locale]/forgot-password/page.tsxfrontend/app/[locale]/login/page.tsxfrontend/app/api/auth/password-reset/confirm/route.tsfrontend/lib/http/getBaseUrl.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/[locale]/forgot-password/page.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/app/[locale]/login/page.tsx (1)
frontend/lib/quiz/guest-quiz.ts (2)
getPendingQuizResult(27-49)clearPendingQuizResult(51-54)
🔇 Additional comments (4)
frontend/app/api/auth/password-reset/confirm/route.ts (1)
61-68: Good: Token deletion before password update mitigates the race condition.The reordering correctly addresses the atomicity concern raised in previous reviews. By deleting the token first, even if the password update fails, the token cannot be reused—the user would simply need to request a new reset. This is the safer sequence given the Neon HTTP driver's transaction limitations.
Consider adding a brief comment explaining this deliberate ordering for future maintainers:
+ // Delete token first: if password update fails, user must request a new reset + // (preferred over leaving a reusable token if the update succeeds but delete fails) await db .delete(passwordResetTokens) .where(eq(passwordResetTokens.token, token));frontend/app/[locale]/login/page.tsx (3)
14-19: LGTM! The open redirect vulnerability is now properly mitigated.The
isSafeRedirectUrlfunction correctly guards against:
- Non-relative paths (line 15)
- Protocol-relative URLs like
//evil.com(line 16)- URLs with embedded protocols (line 17)
This addresses the concern from the previous review.
52-135: LGTM! Login flow now has proper error handling.The try/catch/finally structure correctly:
- Catches network errors and displays user-friendly feedback (lines 127-131)
- Ensures loading state is always reset in the finally block (line 134)
- Handles API response errors separately within the try block (lines 64-75)
This addresses the concern from the previous review about unhandled network errors.
77-119: Good error isolation for the pending quiz result flow.The nested try/catch/finally correctly:
- Isolates quiz submission failures from the main login flow
- Ensures
clearPendingQuizResult()is always called in the finally block- Logs errors for debugging without blocking the user's redirect
This prevents a failed quiz save from breaking the login experience.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
2 participants
This pull request focuses on improving the robustness and security of authentication-related pages and API routes in the frontend application. The main changes include enhanced error handling for network issues, prevention of open redirect vulnerabilities, UI improvements, and more reliable base URL resolution for email links. Additionally, there are code cleanups and minor UI adjustments for better user experience.
Security and Redirect Handling:
isSafeRedirectUrlutility tologinandsignuppages to prevent open redirect vulnerabilities by only allowing safe, relative internal paths for post-authentication redirects. (frontend/app/[locale]/login/page.tsx,frontend/app/[locale]/signup/page.tsx) (frontend/app/[locale]/login/page.tsxL7-R37, frontend/app/[locale]/signup/page.tsxR10-R36)frontend/app/[locale]/login/page.tsx,frontend/app/[locale]/signup/page.tsx) (frontend/app/[locale]/login/page.tsxL99-R123, frontend/app/[locale]/signup/page.tsxL55-R85)Error Handling Improvements:
try/catch/finallyblocks on thesignup,login,forgot-password, andreset-passwordpages to provide user-friendly error messages for network failures and to always reset loading states. (frontend/app/[locale]/signup/page.tsx,frontend/app/[locale]/login/page.tsx,frontend/app/[locale]/forgot-password/page.tsx,frontend/app/[locale]/reset-password/page.tsx) (frontend/app/[locale]/signup/page.tsxL32-R48, frontend/app/[locale]/login/page.tsxR48, frontend/app/[locale]/forgot-password/page.tsxR21-R40, frontend/app/[locale]/reset-password/page.tsxL28-R54)API Improvements:
resolveBaseUrlhelper for constructing email links, ensuring correct URLs regardless of deployment environment. (frontend/app/api/auth/password-reset/route.ts,frontend/app/api/auth/resend-verification/route.ts) [1] [2] [3]frontend/app/api/auth/password-reset/confirm/route.ts)UI and Code Cleanups:
frontend/app/[locale]/login/page.tsx,frontend/app/[locale]/signup/page.tsx,frontend/app/[locale]/reset-password/page.tsx) (frontend/app/[locale]/login/page.tsxL154-R175, frontend/app/[locale]/signup/page.tsxL114-L133, frontend/app/[locale]/reset-password/page.tsxL54-R67)frontend/app/[locale]/signup/page.tsx) (frontend/app/[locale]/signup/page.tsxL114-L133)Minor Enhancements:
dynamic = "force-dynamic"export to the reset password page to ensure correct rendering behavior. (frontend/app/[locale]/reset-password/page.tsx) (frontend/app/[locale]/reset-password/page.tsxR3-R5)These changes collectively improve the security, reliability, and user experience of the authentication flows.
Database Changes (if applicable)
How Has This Been Tested?
Screenshots (if applicable)
Checklist
Before submitting
Reviewers
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.