Fix/otp rate limit security#89
Open
24211a05q8-hue wants to merge 8 commits into
Open
Conversation
🚀 PR Received SuccessfullyHello @24211a05q8-hue, Thank you for taking the initiative to contribute to this project. Please ensure that your PR follows all project guidelines properly before requesting review.
|
📝 WalkthroughWalkthroughThis PR implements OTP request rate-limiting, attempt tracking, and temporary account lockouts (5 attempts, 15-minute locks). It adds hashed OTP storage, simplifies GitHub OAuth by removing user-creation logic and environment assertions, and refactors auth endpoints to return standardized user data shapes. Core changes span environment config, Mongoose schema, middleware, service logic, and route integration. ChangesOTP Rate-Limiting and Attempt Tracking
Sequence DiagramsequenceDiagram
participant Client
participant AuthService
participant Repository
participant Database
participant Mailer
Client->>AuthService: register(email, password)
AuthService->>AuthService: checkSignupRateLimit(email)
AuthService->>Repository: createOtp(email, 'signup')
Repository->>Database: findOneAndUpdate (upsert, reset failures)
AuthService->>Mailer: sendVerificationOtp(email)
Client->>AuthService: verifyOtp(email, otp)
AuthService->>Repository: findOtp(email, 'signup')
alt OTP is locked
AuthService-->>Client: Error - Account temporarily locked
else OTP exists and not locked
AuthService->>AuthService: compareHash(otp)
alt Max attempts exceeded
AuthService->>Repository: incrementFailures & lock
AuthService-->>Client: Error - Too many attempts
else Correct OTP
AuthService->>Repository: clearOtp(email, 'signup')
AuthService->>Repository: updateUserVerification
AuthService-->>Client: accessToken + sanitized user
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 6
🤖 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 `@server/models/Otp.js`:
- Around line 33-37: The createdAt field in the Otp mongoose schema uses
expires: 1200 (20 minutes) but OTP email templates state 10 minutes; either set
expires to 600 to match the templates or update the email templates to reflect
20 minutes—change the value in the createdAt config in server/models/Otp.js (the
createdAt field's expires option) and ensure the same canonical duration is used
in the OTP email generation code/templates so both code and copy share one
source of truth.
In `@server/modules/auth/repository.js`:
- Around line 118-137: Currently incrementOtpFailure() and lockOtp() perform two
separate writes which can race; replace them with a single atomic
findOneAndUpdate that increments failedAttempts and conditionally sets lockUntil
when the new failedAttempts meets/exceeds the threshold. Concretely, modify
incrementOtpFailure(email, purpose, { threshold = 5, lockMinutes = 15 } ) to
call Otp.findOneAndUpdate using either an aggregation-pipeline update (or a
conditional $setOnInsert/$cond in the pipeline) that does $inc: {
failedAttempts: 1 } and, if (failedAttempts + 1) >= threshold, sets lockUntil to
new Date(Date.now() + lockMinutes*60*1000); return the new document ({ new: true
}). Remove separate lockOtp usage from the service and ensure callers use the
new incrementOtpFailure signature.
In `@server/modules/auth/service.js`:
- Around line 227-257: The forgotPassword function currently throws a 404 when
AuthRepository.findUserByEmailWithoutPassword(email) returns null, which leaks
account existence; change it to always return the same success response and not
throw for missing users: keep the RATE_LIMIT_WINDOW check and otpRequestMap
behavior, but when user is null, avoid creating a real OTP or calling
sendPasswordResetOTP; instead perform equivalent-cost no-op work (e.g., generate
and bcrypt.hash a dummy OTP) so timing is similar, skip AuthRepository.createOtp
and sendPasswordResetOTP for non-existent users, and still return the same {
message: "Password reset OTP sent to your email" } result from forgotPassword.
- Around line 461-465: The controller expects AuthController.githubCallback to
return a redirect URL (used by AuthController.githubCallback ->
res.redirect(result.redirectUrl)), but the service currently returns { message,
token, user } which breaks the flow; update the service branches that build the
GitHub callback response to return an object containing redirectUrl (e.g., {
redirectUrl }) when the callback flow is used (instead of returning
token/user/message), or conditionally include redirectUrl alongside other data
only when appropriate; change the two affected return points (the branch that
currently returns message/token/user around sanitizeUser and appToken) so they
return the redirectUrl expected by AuthController.githubCallback.
- Around line 23-25: The in-memory otpRequestMap and RATE_LIMIT_WINDOW implement
a process-local limiter that is wiped on restart and can grow with
attacker-controlled keys; replace otpRequestMap with a shared rate-limit store
(e.g., Redis) and use atomic increment/expire operations (INCR + EXPIRE or a
single EVAL script) keyed by the normalized identifier (email or IP) to enforce
RATE_LIMIT_WINDOW across processes, set TTLs to automatically expire keys to
prevent unbounded growth, and update the code paths that currently read/write
otpRequestMap (look for otpRequestMap usage) to use the new shared-store client
with proper error handling and configuration.
- Around line 75-91: The register() flow resends OTP for unverified users
without checking account lock state, allowing a locked unverified account to
bypass the 15-minute lock because AuthRepository.createOtp() resets
failedAttempts/lockUntil; update the unverified branch to first check
existingUser.lockUntil (and/or failedAttempts) and if lockUntil is in the future
return an error/locked response instead of generating a new OTP, otherwise
proceed to generate/hash OTP and call AuthRepository.createOtp() and
sendVerificationOTP as before; reference existingUser.isVerified,
existingUser.lockUntil, register(), and AuthRepository.createOtp() to locate
changes.
🪄 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: e978fa3b-0828-44f4-8b1f-728ca07421ef
⛔ Files ignored due to path filters (1)
server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
server/config/env.jsserver/middlewares/rateLimiter.jsserver/models/Otp.jsserver/modules/auth/repository.jsserver/modules/auth/routes.jsserver/modules/auth/service.jsserver/modules/auth/validation.js
Comment on lines
+33
to
+37
| createdAt: { | ||
| type: Date, | ||
| default: Date.now, | ||
| expires: 1200, | ||
| }, |
There was a problem hiding this comment.
Keep OTP expiry aligned with the email copy.
expires: 1200 keeps OTPs around for 20 minutes, but both OTP email templates still tell users the code expires in 10 minutes. If 20 minutes is intentional to cover the 15-minute lockout window, the templates need to be updated from the same source of truth.
🤖 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 `@server/models/Otp.js` around lines 33 - 37, The createdAt field in the Otp
mongoose schema uses expires: 1200 (20 minutes) but OTP email templates state 10
minutes; either set expires to 600 to match the templates or update the email
templates to reflect 20 minutes—change the value in the createdAt config in
server/models/Otp.js (the createdAt field's expires option) and ensure the same
canonical duration is used in the OTP email generation code/templates so both
code and copy share one source of truth.
Comment on lines
+118
to
+137
| static async incrementOtpFailure(email, purpose) { | ||
| return await Otp.findOneAndUpdate( | ||
| { email, purpose }, | ||
| { | ||
| $inc: { failedAttempts: 1 }, | ||
| }, | ||
| { new: true } | ||
| ); | ||
| } | ||
|
|
||
| static async lockOtp(email, purpose, lockMinutes = 15) { | ||
| return await Otp.findOneAndUpdate( | ||
| { email, purpose }, | ||
| { | ||
| $set: { | ||
| lockUntil: new Date(Date.now() + lockMinutes * 60 * 1000), | ||
| }, | ||
| }, | ||
| { new: true } | ||
| ); |
There was a problem hiding this comment.
Make OTP failure counting and lockout a single atomic update.
incrementOtpFailure() and lockOtp() are used as two separate writes from server/modules/auth/service.js. Concurrent bad submissions can increment past the threshold before the lock is written, which weakens the 5-attempt cap on a security-sensitive path.
🤖 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 `@server/modules/auth/repository.js` around lines 118 - 137, Currently
incrementOtpFailure() and lockOtp() perform two separate writes which can race;
replace them with a single atomic findOneAndUpdate that increments
failedAttempts and conditionally sets lockUntil when the new failedAttempts
meets/exceeds the threshold. Concretely, modify incrementOtpFailure(email,
purpose, { threshold = 5, lockMinutes = 15 } ) to call Otp.findOneAndUpdate
using either an aggregation-pipeline update (or a conditional $setOnInsert/$cond
in the pipeline) that does $inc: { failedAttempts: 1 } and, if (failedAttempts +
1) >= threshold, sets lockUntil to new Date(Date.now() + lockMinutes*60*1000);
return the new document ({ new: true }). Remove separate lockOtp usage from the
service and ensure callers use the new incrementOtpFailure signature.
Comment on lines
+23
to
+25
| // simple in-memory rate limiter (DEV LEVEL) | ||
| const otpRequestMap = new Map(); | ||
| const RATE_LIMIT_WINDOW = 60 * 1000; // 1 min |
There was a problem hiding this comment.
The OTP request limiter is process-local and resettable.
This Map only throttles a single Node process and is wiped on restart, so scaling out or recycling the app drops the protection entirely. It also grows with attacker-controlled email keys. For public auth endpoints, this needs a shared store with expiry instead of in-memory state.
🤖 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 `@server/modules/auth/service.js` around lines 23 - 25, The in-memory
otpRequestMap and RATE_LIMIT_WINDOW implement a process-local limiter that is
wiped on restart and can grow with attacker-controlled keys; replace
otpRequestMap with a shared rate-limit store (e.g., Redis) and use atomic
increment/expire operations (INCR + EXPIRE or a single EVAL script) keyed by the
normalized identifier (email or IP) to enforce RATE_LIMIT_WINDOW across
processes, set TTLs to automatically expire keys to prevent unbounded growth,
and update the code paths that currently read/write otpRequestMap (look for
otpRequestMap usage) to use the new shared-store client with proper error
handling and configuration.
Comment on lines
+75
to
+91
| // UNVERIFIED USER EXISTS → RESEND OTP | ||
| if (existingUser && !existingUser.isVerified) { | ||
| const plainOtp = generateOTP(); | ||
| const hashedOtp = await bcrypt.hash(plainOtp, 6); | ||
|
|
||
| await AuthRepository.createOtp({ | ||
| email, | ||
| otp: hashedOtp, | ||
| purpose: "signup", | ||
| }); | ||
|
|
||
| await sendVerificationOTP(email, plainOtp); | ||
|
|
||
| return { | ||
| message: "Verification OTP resent successfully", | ||
| user: sanitizeUser(existingUser), | ||
| }; |
There was a problem hiding this comment.
register() currently bypasses the OTP lockout for unverified users.
This branch resends immediately and AuthRepository.createOtp() resets failedAttempts and lockUntil. A locked, unverified account can therefore hit /register again and get a fresh OTP without waiting out the 15-minute lock.
🔒 Suggested guard
if (existingUser && !existingUser.isVerified) {
+ const existingOtp = await AuthRepository.findOtp(email, "signup");
+
+ if (existingOtp?.lockUntil && existingOtp.lockUntil > new Date()) {
+ throw new ApiError(
+ 429,
+ `Too many failed attempts. Try again after ${OTP_LOCK_MINUTES} minutes`
+ );
+ }
+
const plainOtp = generateOTP();
const hashedOtp = await bcrypt.hash(plainOtp, 6);🤖 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 `@server/modules/auth/service.js` around lines 75 - 91, The register() flow
resends OTP for unverified users without checking account lock state, allowing a
locked unverified account to bypass the 15-minute lock because
AuthRepository.createOtp() resets failedAttempts/lockUntil; update the
unverified branch to first check existingUser.lockUntil (and/or failedAttempts)
and if lockUntil is in the future return an error/locked response instead of
generating a new OTP, otherwise proceed to generate/hash OTP and call
AuthRepository.createOtp() and sendVerificationOTP as before; reference
existingUser.isVerified, existingUser.lockUntil, register(), and
AuthRepository.createOtp() to locate changes.
Comment on lines
227
to
257
| static async forgotPassword({ email }) { | ||
| // Find user | ||
| const user = await AuthRepository.findUserByEmailWithoutPassword(email); | ||
| const key = `${email}-forgot-password`; | ||
| const last = otpRequestMap.get(key); | ||
|
|
||
| if (last && Date.now() - last < RATE_LIMIT_WINDOW) { | ||
| throw new ApiError(429, "Please wait before requesting OTP"); | ||
| } | ||
|
|
||
| otpRequestMap.set(key, Date.now()); | ||
|
|
||
| const user = | ||
| await AuthRepository.findUserByEmailWithoutPassword(email); | ||
|
|
||
| if (!user) { | ||
| throw new ApiError(404, "User not found"); | ||
| } | ||
|
|
||
| // Generate OTP | ||
| const plainOtp = generateOTP(); | ||
| const hashedOtp = await bcrypt.hash(plainOtp, 4); | ||
| const hashedOtp = await bcrypt.hash(plainOtp, 6); | ||
|
|
||
| // Store hashed OTP | ||
| await AuthRepository.createOtp({ | ||
| email, | ||
| otp: hashedOtp, | ||
| purpose: "forgot-password" | ||
| purpose: "forgot-password", | ||
| }); | ||
|
|
||
| // Send password reset email | ||
| await sendPasswordResetOTP(email, plainOtp); | ||
|
|
||
| return { | ||
| message: "Password reset OTP sent to your email" | ||
| message: "Password reset OTP sent to your email", | ||
| }; |
There was a problem hiding this comment.
Don't disclose whether an email is registered from forgotPassword().
This still returns 404 "User not found" on a public password-reset endpoint, which lets attackers enumerate valid accounts. The safer pattern is to return the same success response whether the email exists or not.
🤖 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 `@server/modules/auth/service.js` around lines 227 - 257, The forgotPassword
function currently throws a 404 when
AuthRepository.findUserByEmailWithoutPassword(email) returns null, which leaks
account existence; change it to always return the same success response and not
throw for missing users: keep the RATE_LIMIT_WINDOW check and otpRequestMap
behavior, but when user is null, avoid creating a real OTP or calling
sendPasswordResetOTP; instead perform equivalent-cost no-op work (e.g., generate
and bcrypt.hash a dummy OTP) so timing is similar, skip AuthRepository.createOtp
and sendPasswordResetOTP for non-existent users, and still return the same {
message: "Password reset OTP sent to your email" } result from forgotPassword.
Comment on lines
+461
to
+465
| return { | ||
| message: "GitHub account connected successfully", | ||
| token: appToken, | ||
| user: sanitizeUser(user), | ||
| }; |
There was a problem hiding this comment.
The GitHub callback response no longer matches the controller contract.
AuthController.githubCallback still does res.redirect(result.redirectUrl) in server/modules/auth/controller.js:87-111, but both branches here now return { message, token, user }. That breaks the callback flow instead of completing auth.
Also applies to: 486-490
🤖 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 `@server/modules/auth/service.js` around lines 461 - 465, The controller
expects AuthController.githubCallback to return a redirect URL (used by
AuthController.githubCallback -> res.redirect(result.redirectUrl)), but the
service currently returns { message, token, user } which breaks the flow; update
the service branches that build the GitHub callback response to return an object
containing redirectUrl (e.g., { redirectUrl }) when the callback flow is used
(instead of returning token/user/message), or conditionally include redirectUrl
alongside other data only when appropriate; change the two affected return
points (the branch that currently returns message/token/user around sanitizeUser
and appToken) so they return the redirectUrl expected by
AuthController.githubCallback.
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Implemented the requested review fixes and addressed the major security concerns:
$setin OTP update methodsPlease review the latest changes. Thank you!
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes