Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
WalkthroughReplaces the magic-link flow with a 10-character verification-code flow: adds client and page routes for code entry, a POST verification API with per-email and per-IP rate limiting, updates email template and send logic to issue codes, adjusts login/verify redirects, and excludes /auth/email from middleware routing. Changes
Possibly related PRs
🚥 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. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@app/`(auth)/auth/email/[[...params]]/page-client.tsx:
- Around line 76-84: The success path currently only calls
router.push(data.callbackUrl) and leaves the component loading if callbackUrl is
missing; update the try block after receiving data so that if data.callbackUrl
exists you call router.push(data.callbackUrl), otherwise call
setIsLoading(false) and setError with a user-visible message (e.g., "Unable to
complete sign-in: missing callback URL") or perform a safe fallback redirect;
modify the logic around router.push, setIsLoading, and setError in the same
function where data is handled to ensure loading always stops and the user sees
feedback when callbackUrl is absent.
In `@app/api/auth/verify-code/route.ts`:
- Around line 65-83: The current flow calls getLoginCodeData(...) then
deleteLoginCodeData(...), which creates a TOCTOU race allowing the same code to
be used twice; change this to a single atomic fetch-and-delete operation (use
Redis GETDEL if available or a small Lua EVAL script) and call that from the
route instead of separate calls. Concretely: implement a new helper (e.g.,
fetchAndDeleteLoginCodeData or update getLoginCodeData to perform GETDEL/EVAL)
that returns the stored data and removes the key in one atomic step, then
replace the pair of calls in route.ts with a single call to that helper, handle
the null/undefined result the same way as before, and remove the separate
deleteLoginCodeData invocation. Ensure the returned shape still includes
callbackUrl so existing logic (const { callbackUrl } = ...) continues to work.
🧹 Nitpick comments (3)
components/emails/verification-link.tsx (1)
16-22: Avoid optionalcodeto prevent accidental placeholder emails.
Ifcodeisn’t passed, the default could ship to users, leading to invalid logins. Consider requiringcodeand only using a preview fallback for dev/testing.♻️ Suggested change
-const VerificationCodeEmail = ({ - email = "user@example.com", - code = "45PFSNUDYW", -}: { - email?: string; - code?: string; -}) => { +const VerificationCodeEmail = ({ + email = "user@example.com", + code, +}: { + email?: string; + code: string; +}) => {Also applies to: 42-47
app/(auth)/auth/email/[[...params]]/page-client.tsx (1)
218-233: Addrel="noopener noreferrer"to external links.External links with
target="_blank"should includerel="noopener noreferrer"to prevent the new page from accessingwindow.openerand for performance optimization.Proposed fix
<a href={`${process.env.NEXT_PUBLIC_MARKETING_URL}/terms`} target="_blank" + rel="noopener noreferrer" className="underline" > Terms of Service </a>{" "} and{" "} <a href={`${process.env.NEXT_PUBLIC_MARKETING_URL}/privacy`} target="_blank" + rel="noopener noreferrer" className="underline" > Privacy Policy </a>lib/emails/send-verification-request.ts (1)
15-17: Remove unused constantLOGIN_CODE_PREFIX.
LOGIN_CODE_PREFIXis defined but never used in this file. OnlyLOGIN_CODE_EMAIL_PREFIXis used for Redis key construction.Proposed fix
-// Redis key prefixes for login codes -const LOGIN_CODE_PREFIX = "login_code:"; -const LOGIN_CODE_EMAIL_PREFIX = "login_code:email:"; +// Redis key prefix for login codes +const LOGIN_CODE_EMAIL_PREFIX = "login_code:email:";
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@app/`(auth)/auth/email/[[...params]]/page-client.tsx:
- Around line 62-66: The expired-state branch is unreachable because the verify
API returns 401 for expired/missing codes; update the client-side check in the
error handling around response.ok (the block using response.status === 410 and
setIsExpired/setError) to also treat response.status === 401 or
response.data.error/messages that indicate "invalid" as expired, or
alternatively adjust the API to return 410 for expired codes; specifically
modify the conditional that references response.status === 410 and
data.error?.includes("expired") to include response.status === 401 (or the API's
exact error string) so setIsExpired(true) and setError("This code has expired or
is invalid.") are reached when the verify API returns 401.
- Around line 222-233: Update the external anchor elements that open in a new
tab (the <a> tags using `${process.env.NEXT_PUBLIC_MARKETING_URL}/terms` and
`${process.env.NEXT_PUBLIC_MARKETING_URL}/privacy` in page-client.tsx) to
include rel="noopener noreferrer" alongside target="_blank" to prevent
reverse-tabnabbing; ensure both the "Terms of Service" and "privacy" links have
this attribute added.
In `@app/api/auth/verify-code/route.ts`:
- Around line 19-31: The request currently calls .trim() and .toUpperCase()
without ensuring email/code are strings and performs presence checks before
trimming; change the validation order in route handler: after parsing body from
request.json(), first check typeof email === 'string' and typeof code ===
'string', then assign trimmed values to normalizedEmail =
email.trim().toLowerCase() and normalizedCode = code.trim().toUpperCase(), then
validate that normalizedEmail is non-empty and normalizedCode has exactly 10
characters (return NextResponse.json with a 400 for invalid input). Ensure these
string/type/length checks happen before any rate-limiting or Redis access so you
never call .trim() on non-strings or process short/whitespace-only codes.
In `@lib/emails/send-verification-request.ts`:
- Around line 81-96: The catch block in fetchAndDeleteLoginCodeData (in
send-verification-request.ts) masks Redis failures by returning null, causing
callers to treat errors as "invalid code"; change the catch to rethrow the
caught error (or throw a typed Redis/StorageError) after optional logging so
callers can return a 5xx; keep returning null only for the "not found" branch
(if (!data) return null) and do not swallow exceptions in the try/catch around
redis.getdel.
🧹 Nitpick comments (2)
lib/emails/send-verification-request.ts (1)
34-50: Consider invalidating older codes per email.
Each request stores a newemail+codekey, so multiple codes remain valid until TTL. If the intent is “latest code only,” delete existing keys for the email or store a single per‑email record that is overwritten.app/api/auth/verify-code/route.ts (1)
10-13: Prefer a trusted IP source before falling back to"unknown".
Next.js 14'sNextRequestexposes anipproperty (platform-provided by Vercel; other hosts must supply viaX-Forwarded-Foror equivalent). Usingrequest.ipbefore the"unknown"fallback reduces false throttling when proxy headers are absent and improves security by relying on platform-verified sources.💡 Recommended adjustment
function getClientIp(request: NextRequest): string { const forwarded = request.headers.get("x-forwarded-for"); const realIp = request.headers.get("x-real-ip"); - return forwarded?.split(",")[0]?.trim() || realIp || "unknown"; + return ( + forwarded?.split(",")[0]?.trim() || + realIp || + request.ip || + "unknown" + ); }
Comment on lines
+222
to
+233
| <a | ||
| href={`${process.env.NEXT_PUBLIC_MARKETING_URL}/terms`} | ||
| target="_blank" | ||
| className="underline" | ||
| > | ||
| Terms of Service | ||
| </a>{" "} | ||
| and{" "} | ||
| <a | ||
| href={`${process.env.NEXT_PUBLIC_MARKETING_URL}/privacy`} | ||
| target="_blank" | ||
| className="underline" |
Contributor
There was a problem hiding this comment.
Add rel="noopener noreferrer" to external links opened in a new tab.
Without rel, the new tab can access window.opener (reverse‑tabnabbing).
🔒 Safe link attributes
<a
href={`${process.env.NEXT_PUBLIC_MARKETING_URL}/terms`}
target="_blank"
+ rel="noopener noreferrer"
className="underline"
>
Terms of Service
</a>{" "}
@@
<a
href={`${process.env.NEXT_PUBLIC_MARKETING_URL}/privacy`}
target="_blank"
+ rel="noopener noreferrer"
className="underline"
>
Privacy Policy
</a>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <a | |
| href={`${process.env.NEXT_PUBLIC_MARKETING_URL}/terms`} | |
| target="_blank" | |
| className="underline" | |
| > | |
| Terms of Service | |
| </a>{" "} | |
| and{" "} | |
| <a | |
| href={`${process.env.NEXT_PUBLIC_MARKETING_URL}/privacy`} | |
| target="_blank" | |
| className="underline" | |
| <a | |
| href={`${process.env.NEXT_PUBLIC_MARKETING_URL}/terms`} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| className="underline" | |
| > | |
| Terms of Service | |
| </a>{" "} | |
| and{" "} | |
| <a | |
| href={`${process.env.NEXT_PUBLIC_MARKETING_URL}/privacy`} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| className="underline" | |
| > | |
| Privacy Policy | |
| </a> |
🤖 Prompt for AI Agents
In `@app/`(auth)/auth/email/[[...params]]/page-client.tsx around lines 222 - 233,
Update the external anchor elements that open in a new tab (the <a> tags using
`${process.env.NEXT_PUBLIC_MARKETING_URL}/terms` and
`${process.env.NEXT_PUBLIC_MARKETING_URL}/privacy` in page-client.tsx) to
include rel="noopener noreferrer" alongside target="_blank" to prevent
reverse-tabnabbing; ensure both the "Terms of Service" and "privacy" links have
this attribute added.
Comment on lines
81
to
96
| try { | ||
| const data = await redis.get(`${MAGIC_LINK_PREFIX}${token}`); | ||
| const key = `${LOGIN_CODE_EMAIL_PREFIX}${email.toLowerCase()}:${code.toUpperCase()}`; | ||
|
|
||
| // Use GETDEL for atomic get-and-delete operation | ||
| // This prevents race conditions where two requests could use the same code | ||
| const data = await redis.getdel(key); | ||
| if (!data) return null; | ||
|
|
||
| // Handle both string and already-parsed object (Redis client behavior) | ||
| if (typeof data === "string") { | ||
| return JSON.parse(data) as MagicLinkData; | ||
| return JSON.parse(data) as LoginCodeData; | ||
| } | ||
| return data as MagicLinkData; | ||
| return data as LoginCodeData; | ||
| } catch (error) { | ||
| console.error("Error fetching magic link data:", error); | ||
| console.error("Error fetching and deleting login code data:", error); | ||
| return null; |
Contributor
There was a problem hiding this comment.
Don’t mask Redis failures as “invalid code.”
fetchAndDeleteLoginCodeData returns null on Redis errors, which causes the API to respond 401 instead of 500—hiding outages and confusing users. Prefer rethrowing (or returning a typed error) so callers can emit a 5xx.
🛠️ Let the caller handle failures
} catch (error) {
console.error("Error fetching and deleting login code data:", error);
- return null;
+ throw error;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try { | |
| const data = await redis.get(`${MAGIC_LINK_PREFIX}${token}`); | |
| const key = `${LOGIN_CODE_EMAIL_PREFIX}${email.toLowerCase()}:${code.toUpperCase()}`; | |
| // Use GETDEL for atomic get-and-delete operation | |
| // This prevents race conditions where two requests could use the same code | |
| const data = await redis.getdel(key); | |
| if (!data) return null; | |
| // Handle both string and already-parsed object (Redis client behavior) | |
| if (typeof data === "string") { | |
| return JSON.parse(data) as MagicLinkData; | |
| return JSON.parse(data) as LoginCodeData; | |
| } | |
| return data as MagicLinkData; | |
| return data as LoginCodeData; | |
| } catch (error) { | |
| console.error("Error fetching magic link data:", error); | |
| console.error("Error fetching and deleting login code data:", error); | |
| return null; | |
| try { | |
| const key = `${LOGIN_CODE_EMAIL_PREFIX}${email.toLowerCase()}:${code.toUpperCase()}`; | |
| // Use GETDEL for atomic get-and-delete operation | |
| // This prevents race conditions where two requests could use the same code | |
| const data = await redis.getdel(key); | |
| if (!data) return null; | |
| // Handle both string and already-parsed object (Redis client behavior) | |
| if (typeof data === "string") { | |
| return JSON.parse(data) as LoginCodeData; | |
| } | |
| return data as LoginCodeData; | |
| } catch (error) { | |
| console.error("Error fetching and deleting login code data:", error); | |
| throw error; | |
| } |
🤖 Prompt for AI Agents
In `@lib/emails/send-verification-request.ts` around lines 81 - 96, The catch
block in fetchAndDeleteLoginCodeData (in send-verification-request.ts) masks
Redis failures by returning null, causing callers to treat errors as "invalid
code"; change the catch to rethrow the caught error (or throw a typed
Redis/StorageError) after optional logging so callers can return a 5xx; keep
returning null only for the "not found" branch (if (!data) return null) and do
not swallow exceptions in the try/catch around redis.getdel.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.