forgot password page added#289
Conversation
|
@ArshiBansal is attempting to deploy a commit to the vishnukothakapu's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughLogin page credential flow now uses try/catch/finally and a const validator; UI is restructured (password toggle, OAuth/header, loading/spinner, routing). A new client-side Forgot Password page at ChangesForgot Password Authentication Flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 2
🧹 Nitpick comments (3)
app/login/page.tsx (1)
51-52: ⚡ Quick winRename the catch parameter to avoid shadowing.
The catch block parameter
errorshadows the state variableerrordeclared on line 19. Rename it toerror_errorfor clarity.♻️ Proposed fix
- } catch (error) { + } catch (err) { setError("Login failed. Please try again.");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/login/page.tsx` around lines 51 - 52, The catch block currently uses a parameter named error which shadows the component state variable error (and its setter setError) declared earlier; rename the catch parameter to something like err or _error in the try/catch (where setError("Login failed...") is called) and update any references inside that catch to use the new name so the state variable error remains unshadowed and clear (e.g., change catch(error) to catch(err) and use err if you need to log or inspect the thrown value while still calling setError as before).app/password/page.tsx (2)
88-94: ⚡ Quick winAdd a loading spinner for consistency.
The login page displays a spinner alongside the loading text, but this button only changes the text. Adding a spinner would create a consistent loading experience.
🔄 Proposed enhancement
Import the Spinner component at the top:
import { Button } from "`@/components/ui/button`"; import { Input } from "`@/components/ui/input`"; +import { Spinner } from "`@/components/ui/spinner`"; import { Navbar } from "../components/Navbar";Then update the button content:
<Button type="submit" className="w-full" disabled={loading || !email.trim()} > - {loading ? "Sending Reset Link..." : "Send Reset Link"} + {loading ? ( + <> + <Spinner className="mr-2 h-4 w-4" /> + Sending Reset Link... + </> + ) : ( + "Send Reset Link" + )} </Button>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/password/page.tsx` around lines 88 - 94, The button currently toggles text based on the loading boolean but lacks the visual spinner used elsewhere; import the shared Spinner component at the top of page.tsx and update the Button children inside the submit Button (the JSX using loading ? "Sending Reset Link..." : "Send Reset Link") to render the Spinner alongside the "Sending Reset Link..." text when loading is true (e.g., <Spinner className="mr-2" /> before the text), keep the non-loading text unchanged, and ensure spinner is only rendered when loading to preserve accessibility and disabled state behavior.
73-75: ⚡ Quick winAdd ARIA roles for accessibility.
The error and success messages should include appropriate ARIA roles so screen readers announce them to users.
♿ Proposed fix
- {error && <p className="text-sm text-red-500">{error}</p>} + {error && <p className="text-sm text-red-500" role="alert">{error}</p>} - {message && <p className="text-sm text-green-600">{message}</p>} + {message && <p className="text-sm text-green-600" role="status">{message}</p>}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/password/page.tsx` around lines 73 - 75, Wrap the error and message outputs with proper ARIA attributes so screen readers announce them: update the JSX expressions that render error and message (the {error && <p...>} and {message && <p...>} nodes) to include role="alert" and aria-live="assertive" (and aria-atomic="true") for error, and role="status" with aria-live="polite" (and aria-atomic="true") for success messages, keeping the existing classes and text content intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/password/page.tsx`:
- Line 68: Replace the unescaped apostrophe in the JSX text "Enter your email
address and we'll send you a password reset link." with the HTML entity '
so the string reads "Enter your email address and we'll send you a password
reset link."; locate the JSX/JS string containing that exact sentence in the
password page component (the text node in app/password/page.tsx) and update it
to use ' to satisfy ESLint/HTML escaping rules.
- Around line 28-36: The client in app/password/page.tsx is POSTing to
"/api/forgot-password" (see the fetch call) but there is no matching API route;
either add a Next.js route handler at app/api/forgot-password/route.ts that
accepts POST and JSON { email } and returns appropriate success/error responses,
or update the fetch URL in app/password/page.tsx to the existing reset endpoint
used elsewhere in the repo; ensure the server handler validates/sanitizes email
and returns consistent status codes the page expects.
---
Nitpick comments:
In `@app/login/page.tsx`:
- Around line 51-52: The catch block currently uses a parameter named error
which shadows the component state variable error (and its setter setError)
declared earlier; rename the catch parameter to something like err or _error in
the try/catch (where setError("Login failed...") is called) and update any
references inside that catch to use the new name so the state variable error
remains unshadowed and clear (e.g., change catch(error) to catch(err) and use
err if you need to log or inspect the thrown value while still calling setError
as before).
In `@app/password/page.tsx`:
- Around line 88-94: The button currently toggles text based on the loading
boolean but lacks the visual spinner used elsewhere; import the shared Spinner
component at the top of page.tsx and update the Button children inside the
submit Button (the JSX using loading ? "Sending Reset Link..." : "Send Reset
Link") to render the Spinner alongside the "Sending Reset Link..." text when
loading is true (e.g., <Spinner className="mr-2" /> before the text), keep the
non-loading text unchanged, and ensure spinner is only rendered when loading to
preserve accessibility and disabled state behavior.
- Around line 73-75: Wrap the error and message outputs with proper ARIA
attributes so screen readers announce them: update the JSX expressions that
render error and message (the {error && <p...>} and {message && <p...>} nodes)
to include role="alert" and aria-live="assertive" (and aria-atomic="true") for
error, and role="status" with aria-live="polite" (and aria-atomic="true") for
success messages, keeping the existing classes and text content intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 18d06991-3c40-4ea5-8723-0159167019bc
📒 Files selected for processing (2)
app/login/page.tsxapp/password/page.tsx
vishnukothakapu
added
good first issue
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
hey @ArshiBansal. The forgot password page calls fetch("/api/forgot-password") but no such API route exists ,not in this PR and not anywhere in the codebase. Submitting the form will always 404. Pls fix the issue and lmk |
|
@vedhapprakashni i am looking into it |
|
@vedhapprakashni done |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
app/api/forgot-password/route.ts (2)
3-24: 🏗️ Heavy liftAdd rate limiting to prevent abuse.
Password reset endpoints are common targets for abuse, including email bombing (spamming users with reset requests) and user enumeration attacks. Without rate limiting, an attacker can make unlimited requests.
Consider implementing:
- Per-IP rate limiting (e.g., 3 requests per 15 minutes)
- Per-email rate limiting (prevent targeting specific users)
- CAPTCHA for suspicious patterns
- Exponential backoff after repeated requests
Libraries/approaches:
- Middleware with rate limiting (e.g.,
@upstash/ratelimit,express-rate-limitpatterns)- Edge middleware rate limiting if deploying to Vercel/similar
- Redis-backed rate limiting for distributed systems
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/forgot-password/route.ts` around lines 3 - 24, The POST handler lacks rate limiting and should enforce per-IP and per-email limits before processing requests: integrate a rate limiter (e.g., `@upstash/ratelimit` or Redis-backed) and, at the top of the exported async function POST(req: Request), check both the caller IP (from headers/req) and the parsed email against rate-limiter keys (e.g., `ratelimit.consume('ip:'+ip)` and `ratelimit.consume('email:'+email)`), return NextResponse.json({ message: "Too many requests." }, { status: 429 }) when limits are exceeded, and only proceed to log/send reset actions when both checks pass; also record failures to trigger exponential backoff or CAPTCHA flow for repeated offenders.
18-23: 💤 Low valueConsider differentiating JSON parse errors from server errors.
The catch block treats all errors the same, including JSON parsing failures. This makes debugging harder and could mask issues like malformed client requests vs actual server errors.
🔧 Proposed refinement
} catch (err) { + if (err instanceof SyntaxError) { + return NextResponse.json( + { message: "Invalid request format." }, + { status: 400 }, + ); + } + console.error("Forgot password error:", err); return NextResponse.json( { message: "Internal server error." }, { status: 500 }, ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/forgot-password/route.ts` around lines 18 - 23, The catch-all in the forgot-password route's try/catch currently returns a 500 for every error; change it so JSON parse/validation errors are handled separately: when parsing the request body (the code that calls request.json()) or validating required fields in the request, detect a SyntaxError or validation failure and return NextResponse.json({ message: "Bad request" }, { status: 400 }) instead, while keeping other unexpected errors logged and returned as 500 via NextResponse.json({ message: "Internal server error." }, { status: 500 }); update the catch block around the request.json() and validation logic in route.ts (the handler function and its catch) to branch by error type/message accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/api/forgot-password/route.ts`:
- Around line 15-17: The endpoint currently returns a stubbed success response
in the forgot-password route and does not perform any reset-token generation,
storage, or email sending; either implement the complete flow in the request
handler in app/api/forgot-password/route.ts (generate a cryptographically secure
token, persist it with an expiry linked to the user record or a password_resets
table/cache, compose a reset URL, and call your email service client to send the
link), or explicitly mark the handler as a stub by adding a TODO comment and
updating the PR description/issue tracker; locate the handler around the
NextResponse.json(...) return and update the logic there (or add the TODO) so
the repository clearly reflects whether token creation/storage and email
delivery are implemented or postponed.
- Line 13: Remove the plaintext-email console.log that exposes PII: replace the
console.log("Password reset requested for:", email) in the forgot-password
handler with either no log or an anonymized identifier (e.g., compute a one-way
hash of the email using crypto.createHash('sha256') and log the hash or log only
non-PII metadata like request id or email domain). Update the code near the
console.log call (the console.log invocation and the local variable email in the
forgot-password POST handler) so logs never contain raw email addresses.
- Around line 7-12: The POST handler currently only checks for presence of email
and returns NextResponse.json on missing email; update the validation to trim
surrounding whitespace from the incoming email and validate its format (e.g.,
using a simple regex or a shared validateEmail utility) before proceeding.
Locate the email handling in the route's POST handler (where NextResponse.json
is returned on missing email) and replace the simple presence check with: 1)
email = email.trim(), 2) if empty -> return 400 as before, and 3) if
regex/validateEmail fails -> return NextResponse.json with a 400 and a message
like "Invalid email format." Ensure the same validation logic matches the
client-side behavior.
---
Nitpick comments:
In `@app/api/forgot-password/route.ts`:
- Around line 3-24: The POST handler lacks rate limiting and should enforce
per-IP and per-email limits before processing requests: integrate a rate limiter
(e.g., `@upstash/ratelimit` or Redis-backed) and, at the top of the exported async
function POST(req: Request), check both the caller IP (from headers/req) and the
parsed email against rate-limiter keys (e.g., `ratelimit.consume('ip:'+ip)` and
`ratelimit.consume('email:'+email)`), return NextResponse.json({ message: "Too
many requests." }, { status: 429 }) when limits are exceeded, and only proceed
to log/send reset actions when both checks pass; also record failures to trigger
exponential backoff or CAPTCHA flow for repeated offenders.
- Around line 18-23: The catch-all in the forgot-password route's try/catch
currently returns a 500 for every error; change it so JSON parse/validation
errors are handled separately: when parsing the request body (the code that
calls request.json()) or validating required fields in the request, detect a
SyntaxError or validation failure and return NextResponse.json({ message: "Bad
request" }, { status: 400 }) instead, while keeping other unexpected errors
logged and returned as 500 via NextResponse.json({ message: "Internal server
error." }, { status: 500 }); update the catch block around the request.json()
and validation logic in route.ts (the handler function and its catch) to branch
by error type/message accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 161ed82b-2cc3-46c9-b736-9a793a53ce94
📒 Files selected for processing (1)
app/api/forgot-password/route.ts
| if (!email) { | ||
| return NextResponse.json( | ||
| { message: "Email is required." }, | ||
| { status: 400 }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add email format validation.
The handler only checks for email presence, not format. Invalid email formats (e.g., "notanemail", "@.com") will pass validation. Additionally, the API doesn't trim whitespace while the client does, which could create inconsistencies if the client-side validation is bypassed.
✉️ Proposed fix with email validation
+const EMAIL_REGEX = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;
+
export async function POST(req: Request) {
try {
- const { email } = await req.json();
+ const { email: rawEmail } = await req.json();
+ const email = rawEmail?.trim();
if (!email) {
return NextResponse.json(
{ message: "Email is required." },
{ status: 400 },
);
}
+
+ if (!EMAIL_REGEX.test(email)) {
+ return NextResponse.json(
+ { message: "Please enter a valid email address." },
+ { status: 400 },
+ );
+ }
+
console.log("Password reset requested for:", email);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/api/forgot-password/route.ts` around lines 7 - 12, The POST handler
currently only checks for presence of email and returns NextResponse.json on
missing email; update the validation to trim surrounding whitespace from the
incoming email and validate its format (e.g., using a simple regex or a shared
validateEmail utility) before proceeding. Locate the email handling in the
route's POST handler (where NextResponse.json is returned on missing email) and
replace the simple presence check with: 1) email = email.trim(), 2) if empty ->
return 400 as before, and 3) if regex/validateEmail fails -> return
NextResponse.json with a 400 and a message like "Invalid email format." Ensure
the same validation logic matches the client-side behavior.
| { status: 400 }, | ||
| ); | ||
| } | ||
| console.log("Password reset requested for:", email); |
There was a problem hiding this comment.
Remove PII from logs.
Logging email addresses constitutes a privacy and compliance violation (GDPR, CCPA). Application logs are often aggregated, stored long-term, and accessible to multiple parties, creating an unnecessary exposure of user PII.
🔒 Proposed fix
Either remove the log entirely or use a hashed/anonymized identifier:
- console.log("Password reset requested for:", email);
+ // Log without PII for observability
+ console.log("Password reset requested");Or if you need to track requests for debugging, hash the email:
+ const crypto = require('crypto');
+ const emailHash = crypto.createHash('sha256').update(email).digest('hex').substring(0, 8);
- console.log("Password reset requested for:", email);
+ console.log("Password reset requested, hash:", emailHash);📝 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.
| console.log("Password reset requested for:", email); | |
| // Log without PII for observability | |
| console.log("Password reset requested"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/api/forgot-password/route.ts` at line 13, Remove the plaintext-email
console.log that exposes PII: replace the console.log("Password reset requested
for:", email) in the forgot-password handler with either no log or an anonymized
identifier (e.g., compute a one-way hash of the email using
crypto.createHash('sha256') and log the hash or log only non-PII metadata like
request id or email domain). Update the code near the console.log call (the
console.log invocation and the local variable email in the forgot-password POST
handler) so logs never contain raw email addresses.
| return NextResponse.json({ | ||
| message: "If the email exists, reset link sent.", | ||
| }); |
There was a problem hiding this comment.
Implement actual password reset logic or clarify scope.
The endpoint returns a success message but doesn't generate reset tokens, store them, or send emails. Submitting the form provides user feedback but performs no actual password reset operation.
If this stub is intentional (e.g., split into multiple PRs), consider:
- Adding a TODO comment documenting the missing implementation
- Updating the PR description to clarify scope
- Ensuring the issue tracker reflects that email sending is pending
If this should be complete:
- Integrate an email service (SendGrid, Resend, Nodemailer, etc.)
- Generate a cryptographically secure reset token
- Store the token with expiration (database or cache)
- Send the reset link via email
Would you like help implementing the complete password reset flow, or should this be tracked separately?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/api/forgot-password/route.ts` around lines 15 - 17, The endpoint
currently returns a stubbed success response in the forgot-password route and
does not perform any reset-token generation, storage, or email sending; either
implement the complete flow in the request handler in
app/api/forgot-password/route.ts (generate a cryptographically secure token,
persist it with an expiry linked to the user record or a password_resets
table/cache, compose a reset URL, and call your email service client to send the
link), or explicitly mark the handler as a stub by adding a TODO comment and
updating the PR description/issue tracker; locate the handler around the
NextResponse.json(...) return and update the logic there (or add the TODO) so
the repository clearly reflects whether token creation/storage and email
delivery are implemented or postponed.
|
hello @ArshiBansal pls fix these issues: The API route is a stub, not a real implementation. Right now it just logs the email and returns a success message without actually sending any email: console.log("Password reset requested for:", email); A user submitting this form will see a success message but never receive any email. The route needs to actually look up the user in the database, generate a reset token, and send an email via the existing lib/email.ts setup. console.log with the user's email should be removed. Logging PII (personally identifiable information) like email addresses to the console is a bad practice, especially in production. Missing CSRF token. The password/page.tsx still doesn't include the x-csrf-token header on the POST request. All other mutating routes in this codebase use it and the middleware will block this request in production. You can use the existing getCsrfToken() from @/lib/csrfClient. Route naming is still inconsistent. The page is at /password but the API is at /api/forgot-password. These should follow the same naming convention. pls fix and lmk |
|
Sure just give me time till tomorrow evening due to my exam that I have tomorrow morning |
3 participants
Summary
Added a Forgot Password flow to improve the login experience.
Changes Made
/password).app/password/page.tsx.Related Issue
Fixes #281
Testing
/password.Checklist
Summary by CodeRabbit