fix: add OTP brute-force protection and rate limiting#85
Open
24211a05q8-hue wants to merge 5 commits into
Open
fix: add OTP brute-force protection and rate limiting#8524211a05q8-hue wants to merge 5 commits into
24211a05q8-hue wants to merge 5 commits into
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.
|
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 48 minutes and 9 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughRefactors auth: explicit dotenv loading and env validation; OTP model gains failure/lock fields and TTL removed; adds otpLimiter middleware; repository adds OTP safety helpers and uses ChangesOTP Verification with Lockout & Rate Limiting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 7
🤖 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/config/env.js`:
- Around line 6-18: The env validation list in server/config/env.js is missing
the GitHub OAuth variables used by AuthService.getGithubAuthorizationUrl() and
AuthService.handleGithubCallback(); add "GITHUB_CLIENT_ID",
"GITHUB_CLIENT_SECRET", and "GITHUB_CALLBACK_URL" to the requiredEnvVars array
so startup fails fast when those env vars are absent (keep the existing
validation/exit behavior intact).
In `@server/models/Otp.js`:
- Around line 32-35: The Otp model currently defines createdAt without a TTL and
the auth flows (repository.findOtp, service.verifyOtp, service.resetPassword)
don’t enforce expiry; update the Otp schema (createdAt) to include an expires
TTL (or create a TTL index) using the configured OTP lifetime, and additionally
enforce expiry in repository.findOtp (or in verifyOtp/resetPassword) by
filtering on createdAt (e.g., only return OTPs newer than now - OTP_TTL) so
stale OTP docs cannot be used; also ensure createOtp behavior (Otp.deleteMany)
remains but is not the only mechanism relying on expiry.
In `@server/modules/auth/repository.js`:
- Around line 76-88: createOtp currently deletes-then-saves which is non-atomic;
add a unique compound index on { email, purpose } in server/models/Otp.js (e.g.,
schema.index({ email: 1, purpose: 1 }, { unique: true })) and change createOtp
to perform a single atomic upsert using Otp.findOneAndUpdate({ email, purpose },
{ $set: { otp, failedAttempts: 0, lockUntil: null } }, { upsert: true, new:
true, setDefaultsOnInsert: true }) instead of Otp.deleteMany + save; this
ensures deterministic behavior for findOtp and prevents duplicate documents
under concurrent requests.
In `@server/modules/auth/service.js`:
- Around line 466-469: The jwt.verify call in handleGithubCallback is discarding
the decoded payload (the signed "state" containing mode and userId) so the
callback can't differentiate connect vs login flows; change the jwt.verify usage
in handleGithubCallback (and the similar block covering lines ~507-525) to
capture the decoded token result (e.g., const decoded = jwt.verify(state,
process.env.JWT_SECRET)) and then read decoded.mode and decoded.userId to drive
the /github/connect vs login behavior, preserving existing error handling and
expiry checks.
- Around line 507-517: The code currently calls generateAccessToken using
possibly undefined claims after AuthRepository.findUserByGithubId returns null;
update the logic around AuthRepository.findUserByGithubId / generateAccessToken
so that you first check if user is null/undefined and, if so, fail early
(respond/throw with a 401 or 404 as your API convention dictates) instead of
minting an app token; only call generateAccessToken and proceed to return
success when user is present (use the existing user?._id, user?.email,
user?.role only after confirming user exists).
- Around line 389-403: The resendOtp flow (resendOtp) deletes and recreates the
OTP record via AuthRepository.deleteOtp and AuthRepository.createOtp which
resets failedAttempts and lockUntil and allows bypassing the cooldown; change
the logic to preserve lockout state by first reading the existing OTP (e.g.,
AuthRepository.getOtp or fetching by email/purpose), then either update the
existing record's otp/hash and expiry (leave failedAttempts and lockUntil
untouched) or, if you must recreate, carry over failedAttempts and lockUntil
into the new create payload; also ensure generateOTP and bcrypt.hash are used
only after confirming the user is not currently locked (lockUntil in the future)
to prevent issuing a new OTP during lockout.
- Around line 316-339: The forgot-password OTP flow (AuthRepository.findOtp ->
otpRecord -> bcrypt.compare) currently skips brute-force protections: before
calling bcrypt.compare check otpRecord.lockUntil and if lockUntil > now throw
the same ApiError; after a failed bcrypt.compare increment the record's
failedAttempts via the repository (e.g., AuthRepository.incrementFailedAttempts
or AuthRepository.updateOtp) and if failedAttempts reaches the configured
threshold set otpRecord.lockUntil to now + lockDuration and persist that change;
on a successful compare reset failedAttempts (and clear lockUntil) and persist;
ensure all updates use the AuthRepository methods so the lockout behaviour
mirrors the signup verification flow and still throws ApiError(400, "Invalid or
expired OTP") for failures.
🪄 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: 65cb400e-7875-4c2f-bc9a-903bd077c0e9
⛔ 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
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.
OTP Security Enhancement – Brute Force Protection
Summary
This PR improves the security of the OTP verification system by adding protection against brute-force attacks and unlimited OTP attempts.
Changes Made
rateLimiter.js)Security Improvements
NOTES
This implementation follows standard authentication security practices used in production systems to prevent OTP abuse and ensure safe verification flow.
SCREEN SHOTS
Summary by CodeRabbit
Security
New Features
Bug Fixes