Repository navigation
feat(auth): rate limit login attempts - #499
Open
rignaneseleo wants to merge 1 commit into
Open
rignaneseleo wants to merge 1 commit into
rignaneseleo wants to merge 1 commit into
Conversation
Count failed password logins on POST /auth/login in Redis over a fixed window and return 429 (RateLimitError, with Retry-After) once a counter reaches its limit. Limits are checked before the user lookup, so an unknown email and a wrong password are counted and answered the same way. - Per email (default 5 per 15 min): incremented atomically before verification so concurrent guesses cannot all pass the check, cleared on successful login. Keyed case-insensitively to match the user lookup. - Per IP (default disabled): incremented on failure only. It keys on req.ip, and Express is not configured to trust proxy headers, so on proxied installs (including the Docker image's bundled nginx) it would be one counter shared by every user. Configurable via LOGIN_RATE_LIMIT_PER_EMAIL, LOGIN_RATE_LIMIT_PER_IP and LOGIN_RATE_LIMIT_WINDOW; 0 disables a counter. Invalid values fail at startup, since a zero or non-numeric window would create counters that never expire. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
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.
Description
POST /auth/logincurrently has no limit on password guesses. This adds a failed-attempt limiter using the same Redis counter pattern asrequest-verification,request-password-resetand the password-change endpoint, and reusesRateLimitError.Behaviour
verifyCredentials, so an unknown email and a wrong password are counted the same way and get the same response (Incorrect email or password, or 429 once limited). No user enumeration through the limiter.RATE_LIMIT_EXCEEDED) with aRetry-Afterheader set to the remaining window. The web login page already shows the error message from a non-2xx response.MULTIofSET NX EX+INCR), so a burst of concurrent guesses can't all pass a read-then-increment check while bcrypt runs. A successful login clears it. The key is lowercased to match the case-insensitive user lookup, so casing can't be used to get a fresh counter.req.ip || req.socket.remoteAddress). The app doesn't set Expresstrust proxy, so behind a reverse proxy (including the nginx in the Docker image)req.ipis the proxy's address. Enabled by default, this would become one shared counter for every user of the instance, and anyone could lock out all password logins with a handful of bad attempts. So it ships off, and the docs say to enable it only when clients connect to the API directly. A configurabletrust proxysetting would make it usable on proxied installs, but that changesreq.ipapp-wide and is better as a separate change. Because it's only bumped after verification, concurrent requests can overshoot it slightly; the per-email counter still bounds guesses per account exactly.SET NX EX), so continued attempts while limited don't extend the lockout.Configuration (0 disables a counter; invalid values fail at startup, since a zero or non-numeric window would create counters that never expire). Documented in
.env.self-host.example,apps/api/.env.exampleand the self-hosting environment variables page:LOGIN_RATE_LIMIT_PER_EMAIL5LOGIN_RATE_LIMIT_PER_IP0(disabled)LOGIN_RATE_LIMIT_WINDOW900secondsTradeoff to be aware of: like any per-account limit, the per-email counter lets someone who knows an address block that account's password login for one window. OAuth login and password reset are unaffected.
Type of Change
feat:New feature (MINOR version bump)fix:Bug fix (PATCH version bump)feat!:Breaking change - new feature (MAJOR version bump)fix!:Breaking change - bug fix (MAJOR version bump)docs:Documentation update (no version bump)chore:Maintenance/dependencies (no version bump)refactor:Code refactoring (no version bump)test:Adding tests (no version bump)perf:Performance improvement (PATCH version bump)PR Title Format
feat(auth): rate limit login attemptsTesting
New
apps/api/src/controllers/__tests__/Auth.login.test.ts, run against the worker's real Postgres and Redis like the existing controller and rate-limit tests:Retry-After, including with different email casingThe limit tests fail without the controller change. The full test suite,
yarn lint(no new warnings) andtsc --noEmitforapps/apipass locally.Checklist
Related Issues
None.