feat: add OTP based password recovery system#100
Conversation
|
@nihar-ux18 is attempting to deploy a commit to the Akash Santra 's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end email OTP–based password recovery. Backend: user schema adds OTP fields, rate limiters updated, Brevo OTP email sender added, auth controllers implement send/verify OTP with bcrypt hashing and cooldown, routes expose endpoints. Frontend: store manages OTP state and actions, ForgotPassword page implements 3-step recovery (email → OTP entry → password reset) with optional fallback to security questions. Comprehensive integration tests validate the full flow. ChangesOTP-based Password Recovery
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 4
🧹 Nitpick comments (2)
frontend/src/pages/ForgotPassword.jsx (1)
175-180: ⚡ Quick winKeep a visible focus style on the resend button.
focus:outline-noneremoves the only keyboard focus indicator here, and there is no replacementfocus-visiblestyling.Suggested fix
- className="text-xs font-bold text-primary hover:underline focus:outline-none" + className="text-xs font-bold text-primary hover:underline focus-visible:outline focus-visible:outline-2 focus-visible:outline-offset-2 focus-visible:outline-primary"🤖 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 `@frontend/src/pages/ForgotPassword.jsx` around lines 175 - 180, The resend button currently removes the keyboard focus indicator via the className entry (it includes focus:outline-none) — update the button rendered where handleSendOtp is used (the element using onClick={handleSendOtp} and disabled={isSendingOtp}) to remove focus:outline-none and add a visible focus-visible/focus style (for example a focus-visible:ring or focus:ring with an accessible ring color and offset) so keyboard users retain a clear focus indicator while preserving existing styling and hover behavior.backend/test/auth/otp.test.js (1)
63-63: ⚡ Quick winConsider more resilient async waiting pattern.
The
wait(20)pattern gives time for fire-and-forget email operations to complete, but a fixed 20ms delay could theoretically cause flakiness in slower CI environments or under load.Consider increasing the timeout (e.g.,
wait(100)) for better reliability, or implementing a polling pattern if stricter guarantees are needed.🔄 Low-effort improvement: increase timeout margin
-await wait(20); +await wait(100);This provides more headroom for async operations across different test environments.
Also applies to: 75-75, 101-101, 140-140, 156-156, 184-184
🤖 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 `@backend/test/auth/otp.test.js` at line 63, The short fixed sleep wait(20) in backend/test/auth/otp.test.js is flaky; replace the 20ms sleeps with a more resilient strategy—either increase the margin (e.g., change wait(20) to wait(100) for all occurrences of wait in this file) or implement a polling/retry helper that checks the expected condition (e.g., email sent flag or mock call count) until success or timeout; update every occurrence of wait(...) in this file (including the instances currently using 20ms) to use the chosen approach so tests are stable under CI load.
🤖 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 `@backend/src/controllers/auth.controller.js`:
- Around line 397-401: The cooldown branch currently returns a distinct 429 and
countdown using timeSinceLastSent/secondsRemaining and res.status(429) only when
the account exists, allowing email enumeration; change the endpoint (e.g., the
OTP request handler where timeSinceLastSent and secondsRemaining are computed)
so that requests for both existing and non-existing accounts return the same
generic response and status (do not return a different 429/countdown only for
valid accounts). Ensure the handler always returns a uniform message like "If an
account exists, an OTP has been sent" (and a consistent HTTP status) regardless
of account presence or cooldown, while enforcing cooldown server‑side without
revealing it in the response.
In `@backend/src/lib/sendEmail.js`:
- Line 42: The console.log in the OTP sending flow leaks sensitive data (OTP and
full recipient email); update the logging in the sendEmail/sendOtp function to
remove the raw otp and email from logs—log only non-sensitive info such as “OTP
sent” with a masked or hashed email or a request id, and use the existing app
logger (e.g., processLogger or logger) instead of printing secrets to stdout;
locate the console.log(`[OTP Verification] Sent OTP code: ${otp} to ${email}`)
and replace it with a privacy-safe log entry.
In `@backend/src/routes/auth.route.js`:
- Line 33: The /verify-otp endpoint is unthrottled; add a rate-limiter
middleware and attach it to the router.post("/verify-otp", ...) call to prevent
brute-force OTP attempts. Import or require an express rate-limiter (e.g.,
rateLimit from "express-rate-limit"), create a limiter instance (e.g.,
otpRateLimiter) configured for short window + low max attempts (suitable for
6-digit OTPs), then change the route to use the limiter before the verifyOtp
handler (router.post("/verify-otp", otpRateLimiter, verifyOtp)). Ensure the
limiter returns proper 429 responses and customize the message if needed.
In `@frontend/src/pages/ForgotPassword.jsx`:
- Around line 10-17: The component now always uses the OTP flow
(sendOtp/verifyOtp) and removed the legacy security-question branch; restore
conditional rendering so users who rely on security-question recovery still see
that UI and logic. Update ForgotPassword.jsx to detect which recovery modes are
available from useAuthStore (e.g., presence of sendOtp/verifyOtp vs.
security-question handlers like fetchSecurityQuestion/verifySecurityAnswer or a
flag such as supportsSecurityQuestion) and render either the OTP flow (using
sendOtp, verifyOtp, resetPassword, isSendingOtp, isVerifyingOtp,
isResettingPassword) or the security-question flow (reintroduce the prior JSX
and handlers) accordingly; ensure handlers call the correct store methods and
preserve the previous state/validation logic that existed in the 124-190 region
so both flows work and are reachable.
---
Nitpick comments:
In `@backend/test/auth/otp.test.js`:
- Line 63: The short fixed sleep wait(20) in backend/test/auth/otp.test.js is
flaky; replace the 20ms sleeps with a more resilient strategy—either increase
the margin (e.g., change wait(20) to wait(100) for all occurrences of wait in
this file) or implement a polling/retry helper that checks the expected
condition (e.g., email sent flag or mock call count) until success or timeout;
update every occurrence of wait(...) in this file (including the instances
currently using 20ms) to use the chosen approach so tests are stable under CI
load.
In `@frontend/src/pages/ForgotPassword.jsx`:
- Around line 175-180: The resend button currently removes the keyboard focus
indicator via the className entry (it includes focus:outline-none) — update the
button rendered where handleSendOtp is used (the element using
onClick={handleSendOtp} and disabled={isSendingOtp}) to remove
focus:outline-none and add a visible focus-visible/focus style (for example a
focus-visible:ring or focus:ring with an accessible ring color and offset) so
keyboard users retain a clear focus indicator while preserving existing styling
and hover behavior.
🪄 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: b9643848-df07-4905-a64c-e92f95479e0c
⛔ Files ignored due to path filters (3)
backend/package-lock.jsonis excluded by!**/package-lock.jsonfrontend/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
backend/src/controllers/auth.controller.jsbackend/src/lib/sendEmail.jsbackend/src/middleware/rateLimiter.jsbackend/src/models/user.model.jsbackend/src/routes/auth.route.jsbackend/test/auth/checkAuth.test.jsbackend/test/auth/login.test.jsbackend/test/auth/otp.test.jsbackend/test/auth/signup.test.jsbackend/test/message/message.test.jsbackend/test/socket/socket.test.jsfrontend/src/pages/ForgotPassword.jsxfrontend/src/store/useAuthStore.js
|
Restored the legacy security question recovery flow alongside the OTP recovery flow to preserve the original PR behavior. Also updated the forgot password flow to support switching between both recovery methods and addressed OTP email enumeration concerns. |
Akash504-ai
left a comment
Akash504-ai
left a comment
There was a problem hiding this comment.
Thanks @nihar-ux18 for the contribution. Could you share a screen recording of the entire flow? It will help me verify the end-to-end functionality before completing the review.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/routes/auth.route.js (1)
27-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix duplicate /verify-security and /reset-password route registrations bypassing
forgotPasswordLimiter.
/verify-securityand/reset-passwordare each registered twice inbackend/src/routes/auth.route.js: the first pair usesverifySecurityAnswers/resetPasswordwith no limiter, while the second pair addsforgotPasswordLimiter. BecauseverifySecurityAnswersandresetPasswordare(req, res)handlers that don’t callnext(), the first matching registration handles the request and the limiter-wrapped duplicates never run—leaving both endpoints unthrottled.🔒️ Proposed fix
-router.post("/verify-security", verifySecurityAnswers); -router.post("/reset-password", resetPassword); router.post("/verify-security", forgotPasswordLimiter, verifySecurityAnswers); router.post("/reset-password", forgotPasswordLimiter, resetPassword);🤖 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 `@backend/src/routes/auth.route.js` around lines 27 - 30, Remove the duplicate unprotected route registrations so the limiter is actually applied: delete the two lines that call router.post("/verify-security", verifySecurityAnswers) and router.post("/reset-password", resetPassword) and keep the limiter-wrapped registrations router.post("/verify-security", forgotPasswordLimiter, verifySecurityAnswers) and router.post("/reset-password", forgotPasswordLimiter, resetPassword); verify there are no other duplicate registrations of verifySecurityAnswers or resetPassword in the same module so the forgotPasswordLimiter is always reached.
🧹 Nitpick comments (1)
backend/src/controllers/auth.controller.js (1)
135-152: 💤 Low valueRemove the duplicate input-type guard.
Lines 135-142 already reject non-string
passwordbefore thetry, so the identical check at Lines 145-152 is unreachable dead code (and the two messages diverge: "Invalid input" vs "Invalid input types").♻️ Proposed cleanup
if ( typeof email !== "string" || typeof password !== "string" ) { return res.status(400).json({ message: "Invalid input", }); } try { - if ( - typeof email !== "string" || - typeof password !== "string" - ) { - return res.status(400).json({ - message: "Invalid input types", - }); - } - const user = await User.findOne({ email: email.toLowerCase(), }).select("+password");🤖 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 `@backend/src/controllers/auth.controller.js` around lines 135 - 152, Duplicate input-type guard for email/password exists: remove the redundant conditional inside the try block (the second typeof checks that return "Invalid input types") so only the initial pre-try guard remains; ensure any remaining error/return messages use the same wording ("Invalid input") or the desired canonical message, and keep the try block for runtime/auth logic only (refer to the email and password variables and the try block surrounding them to locate the redundant check).
🤖 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 `@backend/src/controllers/auth.controller.js`:
- Around line 549-576: The getSecurityQuestions handler lacks validation and a
try/catch: validate that req.body.email exists and is a string before calling
email.toLowerCase, then wrap the async body (including the User.findOne call and
access to user.securityQuestions.length) in a try/catch; on invalid input return
a 400 with a clear message, if user not found or has no securityQuestions return
400/404 as appropriate, and in the catch log the error and return a 500 JSON
error response so rejections don’t leave the request hanging.
---
Outside diff comments:
In `@backend/src/routes/auth.route.js`:
- Around line 27-30: Remove the duplicate unprotected route registrations so the
limiter is actually applied: delete the two lines that call
router.post("/verify-security", verifySecurityAnswers) and
router.post("/reset-password", resetPassword) and keep the limiter-wrapped
registrations router.post("/verify-security", forgotPasswordLimiter,
verifySecurityAnswers) and router.post("/reset-password", forgotPasswordLimiter,
resetPassword); verify there are no other duplicate registrations of
verifySecurityAnswers or resetPassword in the same module so the
forgotPasswordLimiter is always reached.
---
Nitpick comments:
In `@backend/src/controllers/auth.controller.js`:
- Around line 135-152: Duplicate input-type guard for email/password exists:
remove the redundant conditional inside the try block (the second typeof checks
that return "Invalid input types") so only the initial pre-try guard remains;
ensure any remaining error/return messages use the same wording ("Invalid
input") or the desired canonical message, and keep the try block for
runtime/auth logic only (refer to the email and password variables and the try
block surrounding them to locate the redundant check).
🪄 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: 2d7fdc1d-0817-46ff-bdb0-31fee861161a
⛔ Files ignored due to path filters (2)
.DS_Storeis excluded by!**/.DS_Storebackend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
backend/src/controllers/auth.controller.jsbackend/src/lib/sendEmail.jsbackend/src/routes/auth.route.jsbackend/test/auth/checkAuth.test.jsbackend/test/auth/login.test.jsbackend/test/auth/signup.test.jsbackend/test/message/message.test.jsfrontend/src/pages/ForgotPassword.jsxtest.txt
✅ Files skipped from review due to trivial changes (1)
- test.txt
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/test/auth/checkAuth.test.js
- backend/test/message/message.test.js
- backend/test/auth/signup.test.js
- backend/src/lib/sendEmail.js
- frontend/src/pages/ForgotPassword.jsx
|
Uploading Paso-otp.mp4… Implemented OTP-based password recovery with secure OTP hashing, expiry validation, reset session handling, and email enumeration protection. Also addressed multiple CodeRabbit review suggestions related to security and error handling. I’m currently working through the remaining review comments and will fix the pending CodeRabbit suggestions soon. |
|
Hi @Akash504-ai ! I've completed the requested changes and resolved the CodeRabbit review comments. The latest commits contain all the fixes discussed during review. Could you please review the updated PR when convenient? Thank you! |
|
Could you share a screen recording of the entire flow? |
|
Hi @Akash504-ai, I've implemented the requested OTP password recovery flow and recorded a short demo showing: OTP generation Demo video: Please let me know if any further changes are needed. Thanks! |
|
Thanks @nihar-ux18 for sharing the demo. I'll review complete flow locally before proceeding with the final review. I'll let you know if any further changes are required. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/pages/ForgotPassword.jsx (1)
189-193:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStep 2 message doesn't reflect the recovery method.
When
recoveryMethod === "security", line 191 still displays "Enter the 6-digit OTP sent to your email" instead of a message about answering security questions.💡 Proposed fix
<p className="text-base-content/50 text-xs mt-1"> {step === 1 && "Verify your email to continue"} - {step === 2 && "Enter the 6-digit OTP sent to your email"} + {step === 2 && recoveryMethod === "otp" && "Enter the 6-digit OTP sent to your email"} + {step === 2 && recoveryMethod === "security" && "Answer your security questions"} {step === 3 && "Set your new account password"} </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 `@frontend/src/pages/ForgotPassword.jsx` around lines 189 - 193, The step 2 help text always shows the OTP message; update the render logic in ForgotPassword.jsx (the element that uses step and recoveryMethod) so when step === 2 and recoveryMethod === "security" it shows a security-question prompt (e.g., "Answer your security questions to continue"), otherwise show the OTP message; implement this by changing the second conditional (the expression currently producing "Enter the 6-digit OTP sent to your email") to branch on recoveryMethod (or extract a small helper like getStepMessage(step, recoveryMethod)) so the correct message renders for security vs email recovery.
🤖 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 `@frontend/src/pages/ForgotPassword.jsx`:
- Around line 248-258: The click handler for fetchSecurityQuestions uses a stale
closure over securityQuestions, causing setAnswers(new
Array(securityQuestions.length).fill("")) to use an outdated length; fix by
removing the direct initialization from the onClick and instead add a useEffect
that watches recoveryMethod and securityQuestions and calls setAnswers(prev =>
prev.length === securityQuestions.length ? prev : new
Array(securityQuestions.length).fill("")) when recoveryMethod === "security" and
securityQuestions.length > 0; keep fetchSecurityQuestions,
setRecoveryMethod("security") and setStep(2) in the onClick but let the
useEffect synchronize answers after securityQuestions updates.
---
Outside diff comments:
In `@frontend/src/pages/ForgotPassword.jsx`:
- Around line 189-193: The step 2 help text always shows the OTP message; update
the render logic in ForgotPassword.jsx (the element that uses step and
recoveryMethod) so when step === 2 and recoveryMethod === "security" it shows a
security-question prompt (e.g., "Answer your security questions to continue"),
otherwise show the OTP message; implement this by changing the second
conditional (the expression currently producing "Enter the 6-digit OTP sent to
your email") to branch on recoveryMethod (or extract a small helper like
getStepMessage(step, recoveryMethod)) so the correct message renders for
security vs email recovery.
🪄 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: 06b98329-6637-4a89-a724-e53bd3e66687
📒 Files selected for processing (5)
backend/.env.examplebackend/src/lib/sendEmail.jsbackend/test/auth/otp.test.jsfrontend/src/pages/ForgotPassword.jsxfrontend/src/store/useAuthStore.js
💤 Files with no reviewable changes (1)
- frontend/src/store/useAuthStore.js
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/src/lib/sendEmail.js
- backend/test/auth/otp.test.js
Akash504-ai
left a comment
Akash504-ai
left a comment
There was a problem hiding this comment.
Thanks for the contribution! I reviewed the implementation locally and pushed a few follow-up fixes/improvements to the PR branch after testing the flow. Appreciate your work on this feature.
|
Thank you @Akash504-ai for reviewing and merging the PR! I appreciate the feedback and the follow-up improvements. It was a great learning experience contributing to the project. |
|
Thanks again for the contribution, @nihar-ux18! Appreciate the effort and collaboration throughout the review process. Looking forward to your future contributions 👍. |
2 participants
Description
This PR adds an OTP-based password recovery system as a secure alternative to security-question-based password reset.
Features Added
Security Improvements
Backend Changes
Modified:
Added:
Frontend Changes
Modified:
Testing Done
Tested:
Notes
The existing security-question-based flow was kept intact for backward compatibility.
Summary by CodeRabbit