feat(public-api): add Redis-backed login lockout#164
feat(public-api): add Redis-backed login lockout#164Nitin-kumar-yadav1307 wants to merge 5 commits into
Conversation
📝 WalkthroughWalkthroughThis PR implements Redis-backed login lockout protection across the authentication system. A new lockout utility module provides atomic failure tracking and lock state management; the common package exports these helpers; the auth controller integrates pre-checks, failure recording, and post-success cleanup; and test suites validate the behavior through mocked lockout functions and new assertions. ChangesLogin Lockout Protection
Sequence Diagram(s)sequenceDiagram
participant Client
participant LoginHandler
participant LockoutService
participant Credentials
participant Response
Client->>LoginHandler: POST /login
LoginHandler->>LockoutService: checkLockout(projectId, email)
alt Already locked
LockoutService-->>LoginHandler: {locked: true, retryAfterSeconds: N}
LoginHandler->>Response: 423 Locked Out
else Not locked
LoginHandler->>Credentials: validate password
alt Credential invalid
LoginHandler->>LockoutService: recordFailedAttempt(projectId, email)
alt Threshold reached
LockoutService-->>LoginHandler: {locked: true, retryAfterSeconds: N}
LoginHandler->>Response: 423 Locked Out
else Below threshold
LoginHandler->>Response: 400 Invalid credentials
end
else Credential valid
LoginHandler->>LockoutService: clearLockout(projectId, email)
LoginHandler->>Response: 200 Login success
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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)
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
Adds Redis-backed per-email login lockout to the public-api authentication flow, aiming to slow brute-force attempts by locking a (projectId, email) pair after repeated failures.
Changes:
- Introduces a shared
loginLockoututility inpackages/commonusing Redis + Lua for atomic lockout checks/updates. - Integrates lockout check + failed-attempt tracking into
userAuth.controller.jslogin flow and clears lockout state on successful login/signup/password reset. - Updates auth test suites to mock the new common exports and adds login lockout behavior tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/common/src/utils/loginLockout.js | New Redis/Lua-based lockout utility (check, record failure, clear). |
| packages/common/src/index.js | Re-exports lockout helpers from @urbackend/common. |
| apps/public-api/src/controllers/userAuth.controller.js | Adds lockout enforcement to login; clears lockout after successful signup/login/reset. |
| apps/public-api/src/tests/userAuth.social.test.js | Extends @urbackend/common mock with lockout helpers. |
| apps/public-api/src/tests/userAuth.refresh.test.js | Extends @urbackend/common mock with lockout helpers. |
| apps/public-api/src/tests/userAuth.email.test.js | Adds lockout expectations/tests and updates mocks/imports accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const rawResult = await redis.eval(atomicCheckLua, 1, lockKey, String(LOCKOUT_SECONDS)); | ||
| const [lockedRaw, ttlRaw] = Array.isArray(rawResult) ? rawResult : [0, 0]; | ||
| const locked = Number(lockedRaw) === 1; | ||
| const retryAfterSeconds = locked ? (Number(ttlRaw) || LOCKOUT_SECONDS) : 0; |
| local lockExists = redis.call('GET', lockKey) | ||
| if lockExists then | ||
| local lockTtl = redis.call('TTL', lockKey) | ||
| if lockTtl < 0 then | ||
| lockTtl = lockoutSeconds | ||
| end | ||
| return { maxAttempts, 1, lockTtl } |
| let lockStatus = { locked: false, retryAfterSeconds: 0 }; | ||
| try { | ||
| lockStatus = await checkLockout(projectId, normalizedEmail); | ||
| } catch (lockErr) { | ||
| console.error('[login-lockout] checkLockout failed:', lockErr?.message || lockErr); | ||
| return sendLockoutServiceError(); | ||
| } |
|
@Nitin-kumar-yadav1307 |
Nitin-kumar-yadav1307
force-pushed
the
feature/public-api-login-lockout
branch
from
dc50a4f to
5bd019c
Compare
|
you can ignore frontend lint errors |
![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
🧹 Nitpick comments (1)
apps/public-api/src/__tests__/userAuth.email.test.js (1)
370-421: ⚡ Quick winAdd a login test for lockout-service failure branches (503).
Please add cases where
checkLockoutandrecordFailedAttemptreject, and assert the login handler returns the expected service-unavailable response. This validates the outage-hardening behavior introduced in this PR.🤖 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 370 - 421, Add two new tests for controller.login that simulate lockout-service failures: one where checkLockout rejects and one where recordFailedAttempt rejects; in each test, mock checkLockout or recordFailedAttempt to return a rejected promise, call controller.login with an appropriate req/res (reuse makeReq/makeRes), and assert that res.status was called with 503 and that downstream actions like bcrypt.compare or further recordFailedAttempt calls are not invoked as appropriate. Locate the existing login tests referencing checkLockout, recordFailedAttempt, and controller.login to mirror their setup and assertions so the new tests validate the service-unavailable behavior when the lockout service throws.
🤖 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 1059-1064: sendAuthError currently returns a raw { error: message
} JSON in the fallback path; change it to consistently use the AppError envelope
or the standard API envelope: inside sendAuthError (referencing sendAuthError,
AppError, next, res) always prefer routing errors through next(new
AppError(statusCode, message)); if next is not a function, respond with the
standardized error shape { success: false, data: {}, message } instead of {
error: message } so all login error responses follow the project's contract and
no raw DB/error details are leaked.
---
Nitpick comments:
In `@apps/public-api/src/__tests__/userAuth.email.test.js`:
- Around line 370-421: Add two new tests for controller.login that simulate
lockout-service failures: one where checkLockout rejects and one where
recordFailedAttempt rejects; in each test, mock checkLockout or
recordFailedAttempt to return a rejected promise, call controller.login with an
appropriate req/res (reuse makeReq/makeRes), and assert that res.status was
called with 503 and that downstream actions like bcrypt.compare or further
recordFailedAttempt calls are not invoked as appropriate. Locate the existing
login tests referencing checkLockout, recordFailedAttempt, and controller.login
to mirror their setup and assertions so the new tests validate the
service-unavailable behavior when the lockout service throws.
🪄 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: c009e0a5-6094-4e0e-8a44-895de9241a41
📒 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
|
that means we done with it ? |
|
@coderabbitai |
|
Tip For best results, initiate chat on the files or code changes.
🔐 Redis-Backed Login LockoutThis PR adds brute-force protection to the public-api login flow using Redis. Here's the breakdown: Core behavior:
New utility (
Integration into
Resilience:
Tests:
|
|
@coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me check the current state of the PR before giving a recommendation. 🏁 Script executed: Length of output: 37665
✅ Passing
|
|
@Nitin-kumar-yadav1307 |
Adds Redis-backed per-email login lockout (projectId+email). Key fixes since initial push:
EXPIRElock keys when TTL < 0 to avoid indefinite locks.retryAfterSeconds.sendAuthErrornow routes vianext(new AppError(...))or falls back to{ success: false, data: {}, message }.@urbackend/commonforPlatformEvent,DeveloperActivity, and activity/reliability queue helpers to avoid consumer breakages.main, resolved conflicts, and force-pushed updated branch.All focused public-api auth tests pass locally (22 tests).
Requesting review and merge when convenient.