feat: add IP-based rate limiting for login and reset-password - #3329
Conversation
Fixes #3328 Signed-off-by: John Bair <john.bair@swirldslabs.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: John Bair <john.bair@swirldslabs.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fixes #3328 Signed-off-by: John Bair <john.bair@swirldslabs.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3329 +/- ##
=======================================
Coverage 99.98% 99.98%
=======================================
Files 209 212 +3
Lines 6601 6650 +49
Branches 1228 1205 -23
=======================================
+ Hits 6600 6649 +49
Misses 1 1
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds IP-keyed rate limiting and an IP→unique-email constraint to the authentication flows to mitigate credential stuffing and password-reset spam (per issue #3328), using a shared client-IP extraction utility and Redis-backed throttling.
Changes:
- Introduces
extractClientIp()and applies it to the globalIpThrottlerGuard. - Adds
IpLoginThrottlerGuardandIpResetPasswordThrottlerGuardand wires them into/auth/loginand/auth/reset-password. - Adds
IpResetPasswordUniqueEmailGuardbacked by a Redis set + TTL window, and documents the Redis 7.0+ prerequisite.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| back-end/README.md | Documents Redis 7.0+ requirement for EXPIRE ... NX usage in the new reset-password unique-email guard. |
| back-end/libs/common/src/utils/index.ts | Exposes the new extractClientIp utility via the common utils barrel export. |
| back-end/libs/common/src/utils/extractClientIp.ts | Adds shared client-IP resolution (CF-Connecting-IP → X-Forwarded-For → req.ip). |
| back-end/apps/api/src/guards/ip-throttler.guard.ts | Updates global throttling tracker to use extractClientIp. |
| back-end/apps/api/src/guards/ip-reset-password-unique-email.guard.ts | Adds Redis-set-backed “max unique emails per IP per window” guard for reset-password. |
| back-end/apps/api/src/guards/ip-reset-password-throttler.guard.ts | Adds IP-based throttling limits for reset-password endpoint. |
| back-end/apps/api/src/guards/ip-login-throttler.guard.ts | Adds IP-based throttling limits for login endpoint. |
| back-end/apps/api/src/guards/index.ts | Exports new IP throttler/unique-email guards. |
| back-end/apps/api/src/auth/auth.controller.ts | Applies new guards to /auth/login and /auth/reset-password. |
Suppressed comments (5)
back-end/libs/common/src/utils/extractClientIp.ts:19
req.headers[...]values can bestring[](Node/Express typings allow this). Today, ifcf-connecting-iporx-forwarded-foris provided as an array, this helper ignores it and falls back toreq.ip, which can change rate-limit tracking keys unexpectedly.
const cf = req.headers?.['cf-connecting-ip'];
if (cf && typeof cf === 'string' && cf.trim()) {
return cf.trim();
}
back-end/apps/api/src/guards/ip-reset-password-unique-email.guard.ts:27
- The unique-email limit can be bypassed by varying email casing/whitespace, and
req.body.emailmay not be a string (guards run before DTO validation pipes). Normalize + validate before writing to Redis so the set truly tracks unique emails per IP.
const email: string = req.body?.email;
if (!email) return true; // let EmailThrottlerGuard handle the missing email case
back-end/apps/api/src/guards/ip-reset-password-unique-email.guard.ts:44
- When concurrent requests exceed the unique-email limit, this guard throws but leaves the extra email(s) in the Redis set. That can make the set size permanently exceed the configured limit for the rest of the window, causing over-blocking beyond the intended "max N unique emails" constraint.
await this.redis.sadd(key, email);
await this.redis.expire(key, TEN_MINUTES_SECONDS, 'NX');
// Re-check after adding to handle concurrent requests
const countAfter = await this.redis.scard(key);
back-end/apps/api/src/auth/auth.controller.ts:139
IpResetPasswordUniqueEmailGuardcurrently runs before the IP and email throttlers, so it can consume unique-email slots even for requests that would be rejected by throttling (or by missing/invalid emails). Reordering keeps the unique-email set accurate and reduces unnecessary Redis work.
@UseGuards(IpResetPasswordUniqueEmailGuard, IpResetPasswordThrottlerGuard, EmailThrottlerGuard)
back-end/apps/api/src/guards/ip-reset-password-unique-email.guard.ts:18
- This guard adds new, security-relevant rate-limiting behavior (Redis set semantics, TTL handling, and concurrency edge cases) but has no corresponding unit tests. The guards directory already has Jest coverage for similar components (e.g.,
email-throttler.guard.spec.ts,ip-throttler.guard.spec.ts).
async canActivate(context: ExecutionContext): Promise<boolean> {
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Signed-off-by: John Bair <john.bair@swirldslabs.com>
Covers CF-Connecting-IP precedence, X-Forwarded-For multi-value parsing, and header string[] fallback cases per Copilot review. Signed-off-by: John Bair <john.bair@swirldslabs.com>
…utes Guards short-circuit in declaration order, so placing EmailThrottlerGuard last let failed attempts skip the per-email throttle. Moved it first on login and reset-password so it always counts the attempt. Signed-off-by: John Bair <john.bair@swirldslabs.com>
Match the named `{ Redis }` import used elsewhere in the repo, and
clarify that Redis <7.0 errors on EXPIRE ... NX rather than silently
skipping it.
Signed-off-by: John Bair <john.bair@swirldslabs.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
back-end/apps/api/src/guards/ip-reset-password-unique-email.guard.ts:16
RESET_IP_UNIQUE_EMAIL_LIMITis parsed withNumber(...)without validating the result. If the env var is misconfigured (e.g., empty or non-numeric),this.limitbecomesNaNand the guard will never trip, effectively disabling the unique-email protection.
constructor(@Inject(ConfigService) configService: ConfigService) {
this.redis = new Redis(configService.getOrThrow('REDIS_URL'));
this.limit = Number(configService.get('RESET_IP_UNIQUE_EMAIL_LIMIT', 3));
}
back-end/apps/api/src/guards/ip-reset-password-unique-email.guard.ts:18
IpResetPasswordUniqueEmailGuardintroduces new Redis-backed security logic (unique-email tracking, TTL behavior, and limit enforcement) but has no unit tests, while other throttler guards in this folder do (e.g.,email-throttler.guard.spec.ts,ip-throttler.guard.spec.ts). Adding tests would help prevent regressions around race conditions and TTL semantics.
async canActivate(context: ExecutionContext): Promise<boolean> {
back-end/apps/api/src/guards/ip-reset-password-unique-email.guard.ts:47
- The current SCARD→SADD→SCARD flow is not safe under concurrency: multiple parallel requests can all pass
countBefore < limit, add different emails, and then fail after the add. Those rejected requests still permanently grow the Redis set for the window, causing an IP to be blocked longer than intended (self-inflicted DoS) and making behavior dependent on race timing.
// Check current unique email count before adding
const countBefore = await this.redis.scard(key);
if (countBefore >= this.limit) {
throw new HttpException('Too Many Requests', HttpStatus.TOO_MANY_REQUESTS);
}
back-end/libs/common/src/utils/extractClientIp.ts:19
extractClientIpignores validstring[]header shapes (possible forIncomingHttpHeaders) and also re-parsesX-Forwarded-Foreven when Express has already computed a trustedreq.ips/req.ipvalue. This can lead to using the wrong tracker IP (e.g., proxy IP or a spoofed header) and undermines the reliability of rate limiting.
const cf = req.headers?.['cf-connecting-ip'];
if (cf && typeof cf === 'string' && cf.trim()) {
return cf.trim();
}
2 attempts per 10s was easy for a legit user to trip since the guard throttles by shared IP, not per account. Raise defaults to 5/10s and 20/min, and document the new IP throttle env vars in example.env. Signed-off-by: John Bair <john.bair@swirldslabs.com>
Only mark the email/password inputs invalid on a real 401, so a rate-limited attempt doesn't look like a bad-credentials error. Signed-off-by: John Bair <john.bair@swirldslabs.com>
Covers IpLoginThrottlerGuard and IpResetPasswordThrottlerGuard's getTracker IP resolution, and IpResetPasswordUniqueEmailGuard's missing-IP, missing-email, under-limit, at-limit, and race-condition paths. Brings apps/api/src/guards to 100% coverage. Signed-off-by: John Bair <john.bair@swirldslabs.com>
Shared E2E logs in many users rapidly from one CI runner IP, tripping LOGIN_IP_*/RESET_IP_* limits that weren't in the existing throttle-loosening block. Signed-off-by: John Bair <john.bair@swirldslabs.com>
Swaps the spoofable extractClientIp()/XFF-CIDR approach for an IpResolverService that only trusts CF-Connecting-IP, plus fixes for the PR review's reset-password guard gaps (limit validation, email normalization, full-lockout behavior). Signed-off-by: John Bair <john.bair@swirldslabs.com>
…nd-rolled logic Replaces the regex-based normalizeIp/isInternalAddress helpers with ipaddr.js (already a transitive dep via proxy-addr, now direct) for validation, RFC 5952 canonicalization, and range classification -- and folds both into private IpResolverService methods since neither had any other consumer. Signed-off-by: John Bair <john.bair@swirldslabs.com>
Removes a redundant double validity-check on the header path, and makes an invalid req.ip fall back to the fixed '0.0.0.0' bucket instead of passing an unvalidated string through as the rate-limit key. Signed-off-by: John Bair <john.bair@swirldslabs.com>
…assword Signed-off-by: John Bair <john.bair@swirldslabs.com> # Conflicts: # back-end/package.json
Merges toSafeFallback and isInternal into one resolveFallback method so the fallback IP is validated/parsed a single time instead of once for the canonical string and again for the range classification. Signed-off-by: John Bair <john.bair@swirldslabs.com>
Adds IP-keyed throttler guards to
POST /auth/loginandPOST /auth/reset-password, plus a unique-email-per-IP guard for password reset backed by a Redis set.Fixes #3328
See the issue for full details on limits and env vars.