fix: only increment login rate limit on failed attempts#386
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts login rate limiting so that the Redis counter is only incremented for failed authentication attempts (invalid credentials), rather than on every login request.
Changes:
- Split rate limiting into a read-only check (
checkLoginRateLimit) and a write path (recordFailedLoginAttempt) for failures. - Update the login API route to record attempts only when credentials are invalid.
- Extend unit tests to assert failed-attempt recording behavior for success/failure/invalid-body cases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/unit/app/api/login/route.test.ts | Mocks the new ratelimit function and adds assertions around when failed attempts are recorded. |
| src/server/utils/ratelimit.ts | Refactors login rate limiting into “check only” vs “record attempt” functions and exports recordFailedLoginAttempt. |
| src/app/api/login/route.ts | Calls recordFailedLoginAttempt only on invalid credentials. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…nowledge/ts-mapped into fix/login-rate-limit-only-failed
joaquimds
force-pushed
the
fix/login-rate-limit-only-failed
branch
from
812f789 to
b7a1865
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const newCount = | ||
| results && results[0] && Array.isArray(results[0]) ? (results[0][1] as number) : 0; | ||
|
|
||
| if (newCount <= 0) { |
There was a problem hiding this comment.
rollbackLoginAttempt unconditionally calls EXPIRE after DECR, which resets the TTL. This can unintentionally extend the lockout window for any remaining failed attempts (e.g., 4 failures followed by a successful login will keep those failures “fresh” for another 15 minutes). Rollback should generally preserve the existing TTL (or only set an expiry when the key is newly created / missing a TTL).
| export async function checkForgotPasswordAttempt(ip: string): Promise<boolean> { | ||
| const count = await recordAttempt(`rate_limit:forgot_password:${ip}`); | ||
| return count <= FORGOT_PASSWORD_MAX_ATTEMPTS; |
There was a problem hiding this comment.
The DEL is executed outside the MULTI/EXEC transaction, so there’s a race where another request can INCR the same key after the DECR but before DEL, and then the DEL wipes out a non-zero count. That would undercount failed attempts and weaken rate limiting. Consider making the decrement+conditional-delete atomic (e.g., a small Lua script that does DECR and DEL-if-<=0, and manages TTL appropriately).
| export async function checkLoginAttempt(ip: string): Promise<boolean> { | ||
| const count = await recordAttempt(`rate_limit:login:${ip}`); | ||
| return count <= LOGIN_MAX_ATTEMPTS; | ||
| } | ||
|
|
There was a problem hiding this comment.
The comment about rollback says a post-expiry rollback results in a key at -1 and the next INCR producing 0, but the current implementation deletes the key when newCount <= 0. That means the next INCR would produce 1, so the comment is misleading and should be updated to match the actual behavior.
No description provided.