fix: login and password reset requests are rate-limited per client (not in the README; there is currently no brute-force protection on the public auth routes)

This commit is contained in:
fsylvain committed 2026-09-25 05:33:02 +02:00
1 parent f3ec3d2fa2
commit cbb403011a
11 files changed
+559 -9

No files matched your search

@@ -0,0 +1,69 @@
# QA procedure: login_and_password_reset_rate_limiting
Package: `test` (black-box), extending `test/user_test.go` (`mockUserRepo`, `stubHasher`) and
`test/mail_test.go` (`mockMailRepo`).
## Test infrastructure additions needed first
1. Implement `api/middleware.NewRateLimiter(maxAttempts int, window time.Duration) func(http.Handler) http.Handler`
as an in-memory, mutex-guarded fixed-window counter keyed on `r.RemoteAddr`. Each middleware
instance is independent, so wrapping `/login` and `/password-reset` with two separate
`NewRateLimiter(...)` calls in `NewAuthRouter` gives them independent counters even with the
same `maxAttempts`/`window` values.
2. In tests, set `req.RemoteAddr = "203.0.113.5:12345"` (host:port, matching real request shape)
per simulated client before calling the router, instead of relying on `chi/middleware.RealIP`
(that middleware itself needs no test-specific double, since the middleware under test reads
`r.RemoteAddr` directly after RealIP would have run in production).
## Setup shared by every case
3. Call `validator.InitValidator()` once.
4. Build `mockUserRepo`, `mockMailRepo`, `stubHasher`, and the full `useCases` set exactly as the
other auth QA procedures do, then `auth.NewHandler(...)`.
5. Mount a fresh `chi.NewRouter()` mirroring `NewAuthRouter`: `/register`, `/logout`,
`/password-reset/confirm`, `/verify` (behind a test `jwtauth.Verifier`+`Authenticator`) wired
unprotected/protected exactly as today, plus `/login` wrapped in
`middleware.NewRateLimiter(maxAttempts, window)` and `/password-reset` wrapped in its own
separate `middleware.NewRateLimiter(maxAttempts, window)` instance. Use small, fast values per
scenario (e.g. `maxAttempts=3`, `window=time.Hour` for cases that must not expire mid-test;
`maxAttempts=1`, `window=50*time.Millisecond` for the window-reset case, with a
`time.Sleep(60*time.Millisecond)` before the final request).
6. Seed `mockUserRepo` with user "alice", a correct password hash (`stubHasher` convention
`"hashed:"+password`), where a scenario needs a valid login.
## HTTP-level checks
7. `maxAttempts=3`, window covering the whole test. From `RemoteAddr "203.0.113.5:1"`, send 3
`POST /v1/auth/login` with alice's correct credentials. Assert all three responses are `200`
with a `Set-Cookie: jwt=...` header.
8. Continuing from step 7's state, send a 4th request from the same `RemoteAddr`. Assert status
`429`, body byte-for-byte equal to `apperr.TooManyRequestsErrorMessage`, a `Retry-After` header
present, and no `Set-Cookie` header (the handler/use case never ran).
9. Fresh router/state, `maxAttempts=3`. From one `RemoteAddr`, send 3 `POST /v1/auth/login` with a
wrong password (assert each is `403`), then a 4th with the correct password. Assert the 4th is
`429`, not `200`.
10. Fresh router/state, `maxAttempts=2`. From one `RemoteAddr`, send 2 `POST /v1/auth/password-reset`
with a seeded email (assert both `200`, `mockMailRepo.SentEmails` grows by one each time), then
a 3rd. Assert the 3rd is `429` and `mockMailRepo.SentEmails` length is unchanged from after the
2nd call. Then, from the same `RemoteAddr`, send a `POST /v1/auth/login` and assert it is not
`429` (its own counter is untouched by the password-reset limiter).
11. Fresh router/state, `maxAttempts=1`. Send one `POST /v1/auth/login` from `RemoteAddr A`,
assert `200`ish (not rate-limited). Send one `POST /v1/auth/login` from a different
`RemoteAddr B`. Assert it is also not `429`.
12. Fresh router/state, `maxAttempts=1`, `window=50*time.Millisecond`. From one `RemoteAddr`, send
a first `POST /v1/auth/login` (not `429`), immediately a second (assert `429`), then
`time.Sleep(60 * time.Millisecond)`, then a third. Assert the third is not `429`.
13. Fresh router/state, `maxAttempts=1` on both limiters. From one `RemoteAddr`, send 5
`POST /v1/auth/register` requests in a row (each with a syntactically valid but distinct body
so validation doesn't confound the assertion). Assert none of them is `429`.
## Manual verification (not exercised by the Go test)
- Real behaviour behind the documented nginx reverse proxy (`docs/deployment.md`) — that
`X-Forwarded-For` is actually set and trusted, and that clients behind it are rate-limited
individually rather than collapsed onto the proxy's own address — is not exercised by an
in-process `httptest` router and should be checked by hand against a real reverse-proxy
deployment.
- Sustained load / memory growth of the in-memory per-IP counter map (whether stale entries are
ever evicted) is not covered by this QA procedure and should be reviewed separately if this
becomes a concern in production.