Revert "Add Redis-backed per-email login lockout after 5 failures" - #162
Conversation
Render
Destroyed
📝 WalkthroughWalkthroughThis PR removes the Redis-based login lockout feature from the authentication system. The change deletes the lockout utility module, removes lockout function exports from the common package, simplifies the login controller to skip lockout checks and failed-attempt tracking, and updates all related test mocks and assertions to reflect the removal. ChangesRemove Login Lockout Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
Reverts the previously added Redis-backed, per-email login lockout (5 failures → 15 minute lock) by removing the shared lockout utility and backing out its usage in the Public API auth flows and tests.
Changes:
- Deletes the
loginLockoutRedis/Lua-based utility frompackages/common. - Removes
checkLockout/recordFailedAttempt/clearLockoutexports from@urbackend/common. - Backs out lockout enforcement/cleanup in
userAuth.controllerand updates Jest mocks/tests accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/common/src/utils/loginLockout.js | Removes the Redis-backed per-email lockout implementation entirely. |
| packages/common/src/index.js | Removes lockout utility exports from the common package surface. |
| apps/public-api/src/controllers/userAuth.controller.js | Removes lockout checks and failed-attempt tracking from signup/login/reset-password flows. |
| apps/public-api/src/tests/userAuth.social.test.js | Updates common-module mocks to no longer include lockout functions. |
| apps/public-api/src/tests/userAuth.refresh.test.js | Updates common-module mocks to no longer include lockout functions. |
| apps/public-api/src/tests/userAuth.email.test.js | Removes lockout-related mocks/assertions and drops lockout-specific test cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1089,43 +1060,10 @@ module.exports.login = async (req, res, next) => { | |||
|
|
|||
| const user = await Model.findOne({ email: normalizedEmail }).select('+password'); | |||
|
|
|||
| if (!user) { | |||
| let failedStatus = { locked: false, retryAfterSeconds: 0, attempts: 0 }; | |||
| try { | |||
| // Fail-open: if Redis is unavailable, do not block login on attempt tracking. | |||
| failedStatus = await recordFailedAttempt(projectId, normalizedEmail); | |||
| } catch (attemptErr) { | |||
| console.error('[login-lockout] recordFailedAttempt failed (user missing):', attemptErr?.message || attemptErr); | |||
| } | |||
|
|
|||
| if (failedStatus.locked) { | |||
| return sendAuthError(423, `Account temporarily locked. Try again in ${failedStatus.retryAfterSeconds} seconds.`); | |||
| } | |||
| return sendAuthError(400, 'Invalid email or password'); | |||
| } | |||
| if (!user) return res.status(400).json({ error: "Invalid email or password" }); | |||
|
|
|||
| const validPass = await bcrypt.compare(password, user.password); | |||
| if (!validPass) { | |||
| let failedStatus = { locked: false, retryAfterSeconds: 0, attempts: 0 }; | |||
| try { | |||
| // Fail-open: if Redis is unavailable, do not block login on attempt tracking. | |||
| failedStatus = await recordFailedAttempt(projectId, normalizedEmail); | |||
| } catch (attemptErr) { | |||
| console.error('[login-lockout] recordFailedAttempt failed (invalid password):', attemptErr?.message || attemptErr); | |||
| } | |||
|
|
|||
| if (failedStatus.locked) { | |||
| return sendAuthError(423, `Account temporarily locked. Try again in ${failedStatus.retryAfterSeconds} seconds.`); | |||
| } | |||
| return sendAuthError(400, 'Invalid email or password'); | |||
| } | |||
|
|
|||
| try { | |||
| // Fail-open: if Redis is unavailable, do not block successful login. | |||
| await clearLockout(projectId, normalizedEmail); | |||
| } catch (clearErr) { | |||
| console.error('[login-lockout] clearLockout failed:', clearErr?.message || clearErr); | |||
| } | |||
| if (!validPass) return res.status(400).json({ error: "Invalid email or password" }); | |||
|
|
|||
| const sessionManager = require("./utils/session.manager"); | ||
| const planLimits = require("./utils/planLimits"); | ||
| const AppError = require("./utils/AppError"); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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)
1049-1066:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftReverting per-email login lockout reintroduces a brute-force / credential-stuffing exposure on
/api/userAuth/login.After this revert, a failed login simply returns
400 Invalid email or passwordwith no per-email throttling, no failed-attempt tracking, and no lockout. I don't see any compensating control on this route in this file (no rate-limit middleware, no CAPTCHA), andassertRefreshRateLimitsis wired only intorefreshToken— not login. Combined with the consistent error message for "user not found" vs "wrong password" (which is otherwise good), an attacker can iterate password guesses against any known email at full request rate.The PR description is just "Reverts #160" with no rationale. Before merging, please confirm at least one of the following:
- There is an equivalent compensating control already in front of this route (e.g., express-rate-limit / WAF / API-gateway throttle keyed on email or IP+email) — and link to it in the PR body.
- The original lockout had a concrete defect that justified an emergency revert, and a follow-up issue is tracked to re-land protection.
- Otherwise, keep the lockout (or land a slimmer fix) rather than fully reverting.
If the revert is being done because the lockout was itself abusable (e.g., attacker-induced denial of service by locking victim accounts), that's worth fixing forward (lock by IP+email tuple, exponential backoff, sliding window) rather than removing the control entirely.
Want me to draft a minimal
express-rate-limit-based per-email throttle for this route, or open a tracking issue for re-landing lockout with the abuse-resistance hardening above?🤖 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 1049 - 1066, The login route (module.exports.login) currently has no per-email/IP throttling after the revert, reintroducing brute-force risk; either (A) restore the previous per-email lockout/failed-attempt tracking logic used before the revert (re-enable the code that increments failed attempts on the user record or cache and enforces temporary lockout inside login and resets on successful auth), or (B) add a slim express-rate-limit middleware applied to /api/userAuth/login that keys limits by normalizedEmail (and optionally IP+email) to throttle guesses; if an external mitigation already exists, state its location in the PR instead of changing code and link to it (note: assertRefreshRateLimits is only used in refreshToken — do not rely on it for login).
🧹 Nitpick comments (1)
apps/public-api/src/__tests__/userAuth.email.test.js (1)
318-361: 💤 Low valueTest alignment looks correct, but security coverage on
loginis now thin.The mock cleanup (Line 102) and the removal of
checkLockout/recordFailedAttempt/clearLockoutassertions correctly mirror the controller revert — no orphaned expectations remain. That said, thelogindescribe now only covers the happy path and a single wrong-password case; there is no test that would catch a regression in any future brute-force protection on this endpoint.If the controller-side concern about restoring a compensating control is addressed (rate limit / re-landed lockout / etc.), please bring back a focused test (e.g., "Nth failed attempt is throttled / locked") at that point. No change needed in this PR purely for the revert mechanics.
🤖 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/__tests__/userAuth.email.test.js` around lines 318 - 361, The test suite for the login endpoint now only covers success and a single wrong-password case and lacks coverage for brute-force/lockout behavior; when you reintroduce any compensating control (e.g., rate-limit or account lockout) in the controller.login flow, add a focused test in this file (inside the describe('login') block) that simulates repeated failed logins and asserts the lockout/throttle behavior: use makeReq/makeRes and mockModel.findOne to return the user, mock bcrypt.compare to return false repeatedly, and assert that the controller calls the lockout-related helpers (checkLockout, recordFailedAttempt, clearLockout) or returns the throttled/locked response (e.g., status indicating lockout) once the Nth failed attempt threshold is reached.
🤖 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.
Outside diff comments:
In `@apps/public-api/src/controllers/userAuth.controller.js`:
- Around line 1049-1066: The login route (module.exports.login) currently has no
per-email/IP throttling after the revert, reintroducing brute-force risk; either
(A) restore the previous per-email lockout/failed-attempt tracking logic used
before the revert (re-enable the code that increments failed attempts on the
user record or cache and enforces temporary lockout inside login and resets on
successful auth), or (B) add a slim express-rate-limit middleware applied to
/api/userAuth/login that keys limits by normalizedEmail (and optionally
IP+email) to throttle guesses; if an external mitigation already exists, state
its location in the PR instead of changing code and link to it (note:
assertRefreshRateLimits is only used in refreshToken — do not rely on it for
login).
---
Nitpick comments:
In `@apps/public-api/src/__tests__/userAuth.email.test.js`:
- Around line 318-361: The test suite for the login endpoint now only covers
success and a single wrong-password case and lacks coverage for
brute-force/lockout behavior; when you reintroduce any compensating control
(e.g., rate-limit or account lockout) in the controller.login flow, add a
focused test in this file (inside the describe('login') block) that simulates
repeated failed logins and asserts the lockout/throttle behavior: use
makeReq/makeRes and mockModel.findOne to return the user, mock bcrypt.compare to
return false repeatedly, and assert that the controller calls the
lockout-related helpers (checkLockout, recordFailedAttempt, clearLockout) or
returns the throttled/locked response (e.g., status indicating lockout) once the
Nth failed attempt threshold is reached.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 902abd06-4c49-4815-8646-596fdf404f4c
📒 Files selected for processing (6)
apps/public-api/src/__tests__/userAuth.email.test.jsapps/public-api/src/__tests__/userAuth.refresh.test.jsapps/public-api/src/__tests__/userAuth.social.test.jsapps/public-api/src/controllers/userAuth.controller.jspackages/common/src/index.jspackages/common/src/utils/loginLockout.js
💤 Files with no reviewable changes (4)
- apps/public-api/src/tests/userAuth.refresh.test.js
- packages/common/src/index.js
- apps/public-api/src/tests/userAuth.social.test.js
- packages/common/src/utils/loginLockout.js
Reverts #160
Summary by CodeRabbit