fix(auth): rate-limit login and registration per client (P2) - #1121
fix(auth): rate-limit login and registration per client (P2)#1121wjc2821296948 wants to merge 7 commits into
Conversation
The `/api/auth/login` and `/api/auth/register` endpoints have no protection against credential stuffing or password spraying. An attacker who can reach the server (default bind `0.0.0.0:3001`) can run an unbounded number of guesses per second from a single IP. Bcrypt with 12 rounds makes each guess slow but does not make online brute force infeasible — over a long enough window any 8-character password falls. Add a per-client sliding-window rate limiter that defaults to 10 attempts per minute and a 60-second lockout window once the cap is hit. The limiter keys on the TCP peer address (or the first `X-Forwarded-For` entry when behind a reverse proxy), so it scales to single-user self-hosted installs without needing a shared store. Successful and failed attempts both consume a slot; the limiter does not let a misbehaving client extend a lockout by retrying. Cover the limiter with a focused unit test that verifies the under-cap, over-cap, lockout-no-extend, rolling-window, and per-client-key behaviors. Co-authored-by: cgsdn <[email protected]>
…uest When `lockoutMs` is shorter than `windowMs`, the previous lockout calculation set `blockedUntil = now + lockoutMs`, but the rolling window retained `maxAttempts` timestamps whose earliest expiry was `timestamps[0] + windowMs`. The client was told to retry in `lockoutMs` seconds, but on its next request the limiter tripped again — starting a new lockout — because the cap was still full. This produced a confusing back-off pattern in which the client could never make forward progress without burning another lockout cycle. Set `blockedUntil` to `max(now + lockoutMs, earliestRetainedTimestamp + windowMs)` so `Retry-After` always points at the next moment a request can succeed. `retryAfterSeconds` is now derived from the final `blockedUntil` value, keeping the header consistent with the body. The "lockout does not extend" test previously advanced the clock by 500 ms — still inside both the original lockout and the rolling window — so the assertion could not discriminate between a preserved lockout and a newly reset one. Advance by 1.1 s instead and assert the updated `Retry-After` header reflects the remaining lockout time. Add a focused regression test that configures `lockoutMs < windowMs` and verifies `Retry-After` returns the rolling-window expiry, not the lockout length. Co-authored-by: cgsdn <[email protected]>
…ension test CodeRabbit noted that the previous comment reversed the two timing conditions: it claimed `currentTime` was past the original lockout and still inside the rolling window, when it is in fact past the rolling window and still inside the lockout. Rewrite the explanation so the two boundaries (lockout expiry at 3000 vs. rolling-window expiry at 2000) and their relative positions to the advanced clock (2100) are described correctly. No behaviour change to the test itself. Co-authored-by: cgsdn <[email protected]>
Severity summaryP2 — This PR is split out from the previous umbrella PR (#1106) per @blackmammoth's request that each finding be reviewed in isolation. 🤖 Generated with Claude Code |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAuthentication now uses an in-memory, configurable rate limiter. The limiter protects ChangesAuthentication Rate Limiting
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthRoutes
participant RateLimiter
participant AuthHandler
Client->>AuthRoutes: POST /login or /register
AuthRoutes->>RateLimiter: Check client attempt state
alt Request allowed
RateLimiter->>AuthHandler: Forward request
AuthHandler-->>Client: Authentication response
else Limit exceeded
RateLimiter-->>Client: 429 with Retry-After
end
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@server/modules/auth/rate-limit.middleware.ts`:
- Around line 62-70: Update the rate-limit middleware’s records store and
cleanup logic around middleware, records, and AttemptRecord so entries are
evicted after both the rolling rate-limit window and blockedUntil lockout
expire. Ensure empty records are removed rather than retained, and bound the
store so requests from many distinct client keys cannot cause unbounded growth
or preserve expired address data.
- Around line 25-41: Update readClientKey to accept X-Forwarded-For only when
req.socket.remoteAddress matches an explicitly configured trusted proxy;
otherwise ignore the header and use the TCP peer address (or unknown). Do not
treat loopback addresses as implicitly trusted, and preserve the existing
forwarded-address parsing only within the trusted-proxy path.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ea97561e-4f3f-4ef5-990c-8162fb944964
📒 Files selected for processing (3)
server/modules/auth/auth.routes.tsserver/modules/auth/rate-limit.middleware.tsserver/modules/auth/tests/rate-limit.middleware.test.ts
Two CodeRabbit follow-ups on the rate-limit middleware: 1. `readClientKey` accepted `X-Forwarded-For` whenever the TCP peer was loopback, and otherwise fell back to the socket address. That conflated two distinct concepts: "is the TCP peer a trusted reverse proxy?" and "is the TCP peer on the loopback interface?". Any local process able to open a connection to the server could pick a victim IP via `X-Forwarded-For` and bypass the limiter; clients behind a real reverse proxy all shared the proxy's loopback address and therefore shared a single limiter bucket. Add an explicit `trustedProxyAddresses: string[]` option. Only when the TCP peer matches one of those addresses do we honour `X-Forwarded-For`. Loopback is NOT implicitly trusted — operators must enumerate their proxy's addresses if they run behind one. 2. `records` retained every client key indefinitely. Filtering the timestamps list of an existing record does not remove the entry from the map, so requests from many distinct addresses caused unbounded memory growth and retained IP-address data after the rate-limit window ended. Add an opportunistic sweep that runs at most once per `windowMs` and deletes every record whose `timestamps` array is empty and whose `blockedUntil` has expired. The map cannot grow without bound regardless of how many distinct client keys we see. Co-authored-by: cgsdn <[email protected]>
Three new cases: 1. `X-Forwarded-For is honoured only when the TCP peer is a trusted proxy` — verifies that with `trustedProxyAddresses: ['10.0.0.1']`, requests from that peer are bucketed by the forwarded address while requests from a non-trusted peer carrying the same header are bucketed by the TCP peer (so a spoofer cannot escape the limiter). 2. `untrusted loopback peers cannot spoof X-Forwarded-For` — even on the loopback interface, without explicit trust the limiter uses the TCP peer. Two loopback peers claiming different forwarded addresses share the same bucket. 3. `records map is bounded by expiring entries after the window passes` — pushes 50 distinct addresses through, asserts the internal `records` Map contains 50 entries, advances the clock past the window, fires one more request to trigger the sweep, and asserts only the just-touched record remains. Co-authored-by: cgsdn <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
server/modules/auth/rate-limit.middleware.ts (1)
70-86: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winExpire stale timestamps during the sweep.
record.timestampsis never filtered insweepExpiredRecords. A record with only expired timestamps remains nonempty and is never deleted. The test burst at time1000still leaves 50 records at time6000.Also compare
blockedUntilwithnow, notcutoff. The current condition retains an expired lockout for an extra rolling window.Proposed fix
- const isRecordExpired = (record: AttemptRecord, cutoff: number): boolean => - record.timestamps.length === 0 && record.blockedUntil <= cutoff; + const isRecordExpired = (record: AttemptRecord, now: number): boolean => { + const cutoff = now - windowMs; + return ( + record.timestamps.every((timestamp) => timestamp <= cutoff) && + record.blockedUntil <= now + ); + }; ... - if (isRecordExpired(record, cutoff)) { + if (isRecordExpired(record, now)) {🤖 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 `@server/modules/auth/rate-limit.middleware.ts` around lines 70 - 86, Update isRecordExpired and sweepExpiredRecords to first remove timestamps older than the active cutoff from each record, then evaluate whether the record is empty and its blockedUntil is no longer in the future relative to now. Use now, rather than cutoff, for the blockedUntil comparison, and delete records that satisfy both expiration conditions.
🤖 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 `@server/modules/auth/rate-limit.middleware.ts`:
- Around line 36-42: Update the forwarded-address parsing in the trusted-proxy
branch to process X-Forwarded-For entries from right to left, skipping addresses
in trustedProxyAddresses and returning the first untrusted address. Apply this
consistently for string and array header forms, and add a regression test
covering a trusted proxy appending the real client address after a spoofed
value.
In `@server/modules/auth/tests/rate-limit.middleware.test.ts`:
- Around line 308-315: Update the rate-limit cleanup test around
createRateLimiter and its middleware so it no longer casts the factory result to
access the closure-local records map. Either add and use an explicitly typed
inspection hook for the map, or remove the size assertions while retaining an
observable test that verifies expired entries are cleaned up after the sweep.
---
Duplicate comments:
In `@server/modules/auth/rate-limit.middleware.ts`:
- Around line 70-86: Update isRecordExpired and sweepExpiredRecords to first
remove timestamps older than the active cutoff from each record, then evaluate
whether the record is empty and its blockedUntil is no longer in the future
relative to now. Use now, rather than cutoff, for the blockedUntil comparison,
and delete records that satisfy both expiration conditions.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 259ba80f-919a-47a9-b8f4-719272b43134
📒 Files selected for processing (2)
server/modules/auth/rate-limit.middleware.tsserver/modules/auth/tests/rate-limit.middleware.test.ts
|
…ests
Two CodeRabbit follow-ups on the previous rate-limit hardening:
1. The previous trusted-proxy parser picked the leftmost comma-
separated value from `X-Forwarded-For`. That value is the most
distant hop and therefore the easiest for the real client to forge:
a malicious client can put any address they want there, the trusted
proxy appends the real client address on the right, and the parser
returns the attacker-supplied value. The attacker then rotates that
value to bypass the per-client limit.
Walk the chain from right to left, skipping any entry that is itself
in `trustedProxyAddresses` (proxy chains like `attacker → proxy1 →
proxy2 → server` are common), and return the first remaining
untrusted address. If the chain contains only trusted hops, fall
back to the TCP peer — never trust a header value the proxy chain
couldn't validate.
2. The previous test cast `limiter as unknown as { records: ... }`
to read the closure-local map. `createRateLimiter` only returned
`middleware` and `reset`, so reading `.records.size` threw at
runtime. Expose a typed `size()` inspection hook on the factory
result and update the test to use it.
Also add a regression test covering a two-hop trusted proxy chain
where the attacker forged the leftmost value and the real client
address sits on the right.
Co-authored-by: cgsdn <[email protected]>
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@server/modules/auth/tests/rate-limit.middleware.test.ts`:
- Around line 337-358: Update the proxy tests in
server/modules/auth/tests/rate-limit.middleware.test.ts at lines 337-358 to
include trusted proxy 10.0.0.1 between 198.51.100.7 and 10.0.0.2 in
X-Forwarded-For, validating trusted-hop skipping. At lines 361-389, configure
every header hop as trusted, then send the second request without
X-Forwarded-For from the same TCP peer 10.0.0.2 and assert it receives 429,
exercising TCP-peer fallback.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e0e324c-093e-48f3-b4e9-9fc64cf79036
📒 Files selected for processing (2)
server/modules/auth/rate-limit.middleware.tsserver/modules/auth/tests/rate-limit.middleware.test.ts
…TCP fallback CodeRabbit correctly noted that the two proxy-chain tests I added previously could pass without exercising the properties they claimed: 1. The "trusted proxy chain selects the nearest untrusted client address" test included `198.51.100.99, 198.51.100.7` in the header, neither of which was a configured trusted proxy. The parser would pick the rightmost untrusted entry regardless of whether the trusted-hop skip was implemented correctly, so the test gave a false sense of coverage. Update the header to include the inner trusted proxy `10.0.0.1` between the forged leftmost value and the real client: `198.51.100.99, 10.0.0.1, 198.51.100.7`. The parser must now skip `10.0.0.2` (the TCP peer) and `10.0.0.1` (the configured trusted hop) before landing on `198.51.100.7`. A buggy implementation that picks the leftmost or rightmost value without skipping trusted hops would assign the request to the wrong bucket. 2. The "trusted proxy chain with only trusted hops falls back to TCP peer" test configured only `10.0.0.1` as trusted while the header included `10.0.0.2, 10.0.0.3, 10.0.0.1`. Walking right-to-left, `10.0.0.1` was skipped (trusted) but `10.0.0.3` was NOT trusted and would be returned, so the test was actually exercising the untrusted-hop case, not the TCP-peer fallback. Configure every header hop (`10.0.0.1`, `10.0.0.2`, `10.0.0.3`) as trusted. After the first call lands on the TCP peer fallback (`10.0.0.2`), send a second request from the same TCP peer *without* the `X-Forwarded-For` header and assert it receives `429`. The second request shares the TCP-peer bucket and proves the fallback path is observable end-to-end. Co-authored-by: cgsdn <[email protected]>
|
1 participant
P2 —
/api/auth/loginand/api/auth/registerhave no rate limitingVulnerability description
The auth endpoints have no protection against credential stuffing or password spraying. An attacker who has obtained a leaked username/password list (or a single valid username) can hammer
/api/auth/loginfrom a single host at the full speed the server can answer; the bcrypt comparison takes hundreds of milliseconds, so the natural per-process throughput is only a handful of attempts per second, but with thousands of parallel hosts this is enough to enumerate a non-trivial fraction of accounts in days. There is no lockout, no exponential backoff, no captcha, and noRetry-Aftersignal to back off the attacker.Fix
Add a per-client (per-IP) sliding-window rate limiter middleware and mount it on both
/api/auth/loginand/api/auth/register. Default budget: 10 requests per 60-second window. When the budget is exceeded the middleware responds429 Too Many RequestswithRetry-After: <seconds>set to the larger of the lockout clock and the rolling-window expiry — that is, the next moment the client is guaranteed to be under the cap again, regardless of whether they're still inside the rolling window.The middleware is a single shared instance (a
Map<clientKey, { timestamps, lockoutUntil }>), so per-client isolation is preserved across requests and clients cannot starve each other.Files
server/modules/auth/rate-limit.middleware.ts(new)server/modules/auth/auth.routes.tsserver/modules/auth/tests/rate-limit.middleware.test.ts(new)Commits
45d2369—fix(auth): rate-limit login and registration per clientebff12b—fix(auth): make rate-limit Retry-After reflect the next permitted requestd41a510—test(auth): correct the simulated-time explanation in the lockout-extension test🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests