fix: add OTP cooldown/rate-limiting to requestPasswordReset endpoint#154
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughController now normalizes emails (lowercase/trim) for reset/signup flows, enforces a 60s OTP cooldown via Redis (returns structured 429 when active), and tests were added to validate cooldown presence, TTL messaging, cooldown creation, and Redis key normalization. ChangesPassword-Reset Rate-Limiting and Email Normalization
Sequence DiagramsequenceDiagram
participant Client
participant API as requestPasswordReset
participant Redis
participant Mail as Email Queue
Client->>API: POST /requestPasswordReset (email)
Note over API: Normalize email to lowercase
API->>Redis: GET otp:reset:{normalizedEmail}:cooldown
Redis-->>API: cooldown_key or null
alt Cooldown Active
API-->>Client: 429 with TTL
else No Cooldown
API->>Mail: Queue reset OTP email
Mail-->>API: Queued
API->>Redis: SET otp:reset:{normalizedEmail}:cooldown EX 60
Redis-->>API: OK
API-->>Client: 200 Generic Success Message
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
apps/public-api/src/controllers/userAuth.controller.js (1)
1340-1357:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply reset cooldown uniformly to avoid account-enumeration side channel
Line 1341 returns early for unknown emails, while Lines 1345-1349 enforce cooldown only for existing users. Two rapid requests can reveal account existence (
200vs429).Suggested fix
- const user = await Model.findOne({ email: normalizedEmail }); - if (!user) { - return res.json({ message: "If that email exists, a reset code has been sent." }); - } - try { await checkPublicOtpCooldown(project._id, normalizedEmail, 'reset'); } catch (cooldownErr) { return res.status(cooldownErr.statusCode || 429).json({ error: cooldownErr.message }); } + + const user = await Model.findOne({ email: normalizedEmail }); + if (!user) { + // Keep response behavior indistinguishable for existing vs non-existing accounts + await setPublicOtpCooldown(project._id, normalizedEmail, 'reset'); + return res.json({ message: "If that email exists, a reset code has been sent." }); + }🤖 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 `@apps/public-api/src/controllers/userAuth.controller.js` around lines 1340 - 1357, Move the OTP cooldown check and the setting of the cooldown to occur before returning for unknown emails so responses are indistinguishable; specifically, call checkPublicOtpCooldown(project._id, normalizedEmail, 'reset') immediately after computing normalizedEmail (before Model.findOne) and if it passes, call setPublicOtpCooldown(project._id, normalizedEmail, 'reset') even when Model.findOne(...) returns no user, then proceed to conditionally create/store the OTP (redis.set) and enqueue email only for existing users but always return the same generic message; keep using the existing functions Model.findOne, checkPublicOtpCooldown, setPublicOtpCooldown, redis.set, and authEmailQueue.add to implement this.
🤖 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 `@apps/public-api/src/controllers/userAuth.controller.js`:
- Around line 1348-1349: The current cooldown error response returns a raw {
error: ... } object which breaks the API envelope; replace that return with the
standard envelope using res.status(cooldownErr.statusCode || 429).json({
success: false, data: {}, message: cooldownErr.message }); and ensure any places
that construct cooldownErr use the AppError class instead of raw errors so we
never expose DB/internal errors directly.
---
Outside diff comments:
In `@apps/public-api/src/controllers/userAuth.controller.js`:
- Around line 1340-1357: Move the OTP cooldown check and the setting of the
cooldown to occur before returning for unknown emails so responses are
indistinguishable; specifically, call checkPublicOtpCooldown(project._id,
normalizedEmail, 'reset') immediately after computing normalizedEmail (before
Model.findOne) and if it passes, call setPublicOtpCooldown(project._id,
normalizedEmail, 'reset') even when Model.findOne(...) returns no user, then
proceed to conditionally create/store the OTP (redis.set) and enqueue email only
for existing users but always return the same generic message; keep using the
existing functions Model.findOne, checkPublicOtpCooldown, setPublicOtpCooldown,
redis.set, and authEmailQueue.add to implement this.
🪄 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
Run ID: fa7fdb30-a73d-4698-a467-7dd98aa8a753
📒 Files selected for processing (2)
apps/public-api/src/__tests__/userAuth.email.test.jsapps/public-api/src/controllers/userAuth.controller.js
|
Last 1 change please |
|
sure i was waiting for the checks |
… status and message
|
@coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 14582
🟠 Outstanding Issue: Account-Enumeration Side Channel (
|
|
last then will merge @itsmessc |
…umeration for unknown emails
|
no worries i will always consider these comments |
|
now can you check i have added the fix |
2 participants
Fixes #143
🛠️ Type of Change
🚀 What Changed
The
requestPasswordResetendpoint had no cooldown between OTP requests, unlikesignupandresendVerificationOtpwhich already enforced a 60-second window. This allowed an attacker who knows a valid email to flood that inbox with reset emails.Changes made:
checkPublicOtpCooldownbefore sending the reset OTP — returns HTTP 429 with seconds remaining if within the 60s windowsetPublicOtpCooldownafter sending — sets a 60s Redis cooldown key (otp:cooldown:reset:<email>)normalizedEmail(email.toLowerCase().trim()) torequestPasswordResetandresetPasswordUserso OTP keys are case-insensitive and consistent🧪 Testing & Validation
Backend Verification:
npm testin thebackend/directory and all tests passed.New tests in
userAuth.email.test.js:Manual verification (PowerShell):
200 { message: "If that email exists, a reset code has been sent." }429 { error: "Please wait 57 seconds before requesting another code." }✅ Checklist
Summary by CodeRabbit
Bug Fixes
Tests