-
Notifications
You must be signed in to change notification settings - Fork 18
feat: add IP-based rate limiting for login and reset-password #3329
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 7 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
5590158
feat: add IP-based rate limiting to login and password reset endpoints
jbair06 de3a3bd
fix: trim x-forwarded-for IP before using as throttle key
jbair06 2f962e3
feat: add IP-based rate limiting for login and reset-password
jbair06 d8c2822
fix: mock new IP throttler guards in auth.controller.spec
jbair06 177fe1c
test: add unit tests for extractClientIp utility
jbair06 0a6d937
fix: run EmailThrottlerGuard before credential/rate guards in auth ro…
jbair06 e2e9bcb
fix: use named ioredis import and correct EXPIRE NX version note
jbair06 a7b3dd1
fix: loosen default IP-based login throttle limits
jbair06 129677e
fix: don't flag login fields invalid on 429 rate-limit response
jbair06 4a4c775
test: add unit tests for the new IP-based auth guards
jbair06 724e739
fix: loosen new IP-based throttle limits for e2e CI runs
jbair06 b162df8
feat: replace ad-hoc IP extraction with a Cloudflare-trust IP resolver
jbair06 5db84c8
refactor: use ipaddr.js for IP validation/normalization instead of ha…
jbair06 d00a657
refactor: tighten IpResolverService's validity/fallback handling
jbair06 263089f
Merge remote-tracking branch 'origin/main' into ip-rate-limit-reset-p…
jbair06 1e94941
refactor: parse req.ip once for the fallback path, not twice
jbair06 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| /* eslint-disable @typescript-eslint/no-explicit-any */ | ||
| import { HttpException, HttpStatus, Inject, Injectable } from '@nestjs/common'; | ||
| import { ConfigService } from '@nestjs/config'; | ||
| import { Reflector } from '@nestjs/core'; | ||
| import { seconds, ThrottlerGuard, ThrottlerStorage } from '@nestjs/throttler'; | ||
| import { extractClientIp } from '@app/common'; | ||
|
|
||
| @Injectable() | ||
| export class IpLoginThrottlerGuard extends ThrottlerGuard { | ||
| constructor( | ||
| @Inject(ConfigService) configService: ConfigService, | ||
| @Inject(ThrottlerStorage) storageService: ThrottlerStorage, | ||
| reflector: Reflector, | ||
| ) { | ||
| super( | ||
| { | ||
| throttlers: [ | ||
| { | ||
| name: 'login-ip-minute', | ||
| ttl: seconds(60), | ||
| limit: Number(configService.get('LOGIN_IP_MINUTE_LIMIT', 10)), | ||
| }, | ||
| { | ||
| name: 'login-ip-ten-second', | ||
| ttl: seconds(10), | ||
| limit: Number(configService.get('LOGIN_IP_TEN_SECOND_LIMIT', 2)), | ||
| }, | ||
| ], | ||
| }, | ||
| storageService, | ||
| reflector, | ||
| ); | ||
| } | ||
|
|
||
| protected getTracker(req: Record<string, any>): Promise<string> { | ||
| const clientIp = extractClientIp(req); | ||
| if (!clientIp) { | ||
| throw new HttpException('Unable to determine client IP', HttpStatus.INTERNAL_SERVER_ERROR); | ||
| } | ||
| return Promise.resolve(clientIp); | ||
| } | ||
| } | ||
42 changes: 42 additions & 0 deletions
42
back-end/apps/api/src/guards/ip-reset-password-throttler.guard.ts
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| /* eslint-disable @typescript-eslint/no-explicit-any */ | ||
| import { HttpException, HttpStatus, Inject, Injectable } from '@nestjs/common'; | ||
| import { ConfigService } from '@nestjs/config'; | ||
| import { Reflector } from '@nestjs/core'; | ||
| import { seconds, ThrottlerGuard, ThrottlerStorage } from '@nestjs/throttler'; | ||
| import { extractClientIp } from '@app/common'; | ||
|
|
||
| @Injectable() | ||
| export class IpResetPasswordThrottlerGuard extends ThrottlerGuard { | ||
| constructor( | ||
| @Inject(ConfigService) configService: ConfigService, | ||
| @Inject(ThrottlerStorage) storageService: ThrottlerStorage, | ||
| reflector: Reflector, | ||
| ) { | ||
| super( | ||
| { | ||
| throttlers: [ | ||
| { | ||
| name: 'reset-ip-minute', | ||
| ttl: seconds(60), | ||
| limit: Number(configService.get('RESET_IP_MINUTE_LIMIT', 5)), | ||
| }, | ||
| { | ||
| name: 'reset-ip-ten-second', | ||
| ttl: seconds(10), | ||
| limit: Number(configService.get('RESET_IP_TEN_SECOND_LIMIT', 1)), | ||
| }, | ||
| ], | ||
| }, | ||
| storageService, | ||
| reflector, | ||
| ); | ||
| } | ||
|
|
||
| protected getTracker(req: Record<string, any>): Promise<string> { | ||
| const clientIp = extractClientIp(req); | ||
| if (!clientIp) { | ||
| throw new HttpException('Unable to determine client IP', HttpStatus.INTERNAL_SERVER_ERROR); | ||
| } | ||
| return Promise.resolve(clientIp); | ||
| } | ||
| } | ||
51 changes: 51 additions & 0 deletions
51
back-end/apps/api/src/guards/ip-reset-password-unique-email.guard.ts
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| import { CanActivate, ExecutionContext, HttpException, HttpStatus, Inject, Injectable } from '@nestjs/common'; | ||
| import { ConfigService } from '@nestjs/config'; | ||
| import { extractClientIp } from '@app/common'; | ||
| import { Redis } from 'ioredis'; | ||
|
|
||
| const TEN_MINUTES_SECONDS = 600; | ||
|
|
||
| @Injectable() | ||
| export class IpResetPasswordUniqueEmailGuard implements CanActivate { | ||
| private readonly redis: Redis; | ||
| private readonly limit: number; | ||
|
|
||
| constructor(@Inject(ConfigService) configService: ConfigService) { | ||
| this.redis = new Redis(configService.getOrThrow('REDIS_URL')); | ||
| this.limit = Number(configService.get('RESET_IP_UNIQUE_EMAIL_LIMIT', 3)); | ||
| } | ||
|
|
||
| async canActivate(context: ExecutionContext): Promise<boolean> { | ||
| const req = context.switchToHttp().getRequest(); | ||
|
|
||
| const ip = extractClientIp(req); | ||
| if (!ip) { | ||
| throw new HttpException('Unable to determine client IP', HttpStatus.INTERNAL_SERVER_ERROR); | ||
| } | ||
|
|
||
| const email: string = req.body?.email; | ||
| if (!email) return true; // let EmailThrottlerGuard handle the missing email case | ||
|
|
||
| const key = `reset:ip-unique-email:${ip}`; | ||
|
|
||
| // 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); | ||
| } | ||
|
|
||
| // Add email to the set and set TTL on first entry. | ||
| // The 'NX' option sets the expiry only if one is not already present, preserving | ||
| // the fixed 10-minute window. Requires Redis 7.0+. | ||
| 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); | ||
|
steven-sheehy marked this conversation as resolved.
Outdated
|
||
| if (countAfter > this.limit) { | ||
| throw new HttpException('Too Many Requests', HttpStatus.TOO_MANY_REQUESTS); | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
| } | ||
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| import { extractClientIp } from './extractClientIp'; | ||
|
|
||
| describe('extractClientIp', () => { | ||
| it('returns the CF-Connecting-IP header when present', () => { | ||
| const req = { | ||
| headers: { 'cf-connecting-ip': '203.0.113.9' }, | ||
| ip: '127.0.0.1', | ||
| }; | ||
|
|
||
| expect(extractClientIp(req)).toBe('203.0.113.9'); | ||
| }); | ||
|
|
||
| it('trims whitespace from the CF-Connecting-IP header', () => { | ||
| const req = { | ||
| headers: { 'cf-connecting-ip': ' 203.0.113.9 ' }, | ||
| ip: '127.0.0.1', | ||
| }; | ||
|
|
||
| expect(extractClientIp(req)).toBe('203.0.113.9'); | ||
| }); | ||
|
|
||
| it('prefers CF-Connecting-IP over X-Forwarded-For and req.ip', () => { | ||
| const req = { | ||
| headers: { | ||
| 'cf-connecting-ip': '203.0.113.9', | ||
| 'x-forwarded-for': '198.51.100.1, 10.0.0.1', | ||
| }, | ||
| ip: '127.0.0.1', | ||
| }; | ||
|
|
||
| expect(extractClientIp(req)).toBe('203.0.113.9'); | ||
| }); | ||
|
|
||
| it('falls back to X-Forwarded-For when CF-Connecting-IP is absent', () => { | ||
| const req = { | ||
| headers: { 'x-forwarded-for': '198.51.100.1, 10.0.0.1' }, | ||
| ip: '127.0.0.1', | ||
| }; | ||
|
|
||
| expect(extractClientIp(req)).toBe('198.51.100.1'); | ||
| }); | ||
|
|
||
| it('uses the leftmost value and trims it from a multi-value X-Forwarded-For header', () => { | ||
| const req = { | ||
| headers: { 'x-forwarded-for': ' 198.51.100.1 , 10.0.0.1, 10.0.0.2' }, | ||
| ip: '127.0.0.1', | ||
| }; | ||
|
|
||
| expect(extractClientIp(req)).toBe('198.51.100.1'); | ||
| }); | ||
|
|
||
| it('ignores an empty CF-Connecting-IP header and falls back to X-Forwarded-For', () => { | ||
| const req = { | ||
| headers: { 'cf-connecting-ip': ' ', 'x-forwarded-for': '198.51.100.1' }, | ||
| ip: '127.0.0.1', | ||
| }; | ||
|
|
||
| expect(extractClientIp(req)).toBe('198.51.100.1'); | ||
| }); | ||
|
|
||
| it('ignores an empty X-Forwarded-For header and falls back to req.ip', () => { | ||
| const req = { | ||
| headers: { 'x-forwarded-for': '' }, | ||
| ip: '127.0.0.1', | ||
| }; | ||
|
|
||
| expect(extractClientIp(req)).toBe('127.0.0.1'); | ||
| }); | ||
|
|
||
| it('falls back to req.ip when headers are missing entirely', () => { | ||
| const req = { headers: {}, ip: '127.0.0.1' }; | ||
|
|
||
| expect(extractClientIp(req)).toBe('127.0.0.1'); | ||
| }); | ||
|
|
||
| it('falls back to req.ip when CF-Connecting-IP is provided as a header array (string[])', () => { | ||
| const req = { | ||
| headers: { 'cf-connecting-ip': ['203.0.113.9', '203.0.113.10'] }, | ||
| ip: '127.0.0.1', | ||
| }; | ||
|
|
||
| expect(extractClientIp(req)).toBe('127.0.0.1'); | ||
| }); | ||
|
|
||
| it('falls back to req.ip when X-Forwarded-For is provided as a header array (string[])', () => { | ||
| const req = { | ||
| headers: { 'x-forwarded-for': ['198.51.100.1', '10.0.0.1'] }, | ||
| ip: '127.0.0.1', | ||
| }; | ||
|
|
||
| expect(extractClientIp(req)).toBe('127.0.0.1'); | ||
| }); | ||
|
|
||
| it('returns undefined when no IP can be determined', () => { | ||
| const req = { headers: {}, ip: undefined }; | ||
|
|
||
| expect(extractClientIp(req)).toBeUndefined(); | ||
| }); | ||
| }); |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| /** | ||
| * Extracts the real client IP from a request. | ||
| * | ||
| * Resolution order: | ||
| * 1. CF-Connecting-IP — set by Cloudflare; not client-controllable. | ||
| * 2. X-Forwarded-For — first (leftmost) value, which is the original client | ||
| * IP appended by the first proxy in the chain. | ||
| * 3. req.ip — Express fallback (direct connection address). | ||
| * | ||
| * Note: X-Forwarded-For is client-controllable when no trusted proxy is | ||
| * configured. CF-Connecting-IP takes precedence precisely because Cloudflare | ||
| * strips and rewrites it, making it safe to trust. | ||
| */ | ||
| export function extractClientIp(req: Record<string, any>): string | undefined { | ||
|
jbair06 marked this conversation as resolved.
Outdated
|
||
| const cf = req.headers?.['cf-connecting-ip']; | ||
| if (cf && typeof cf === 'string' && cf.trim()) { | ||
| return cf.trim(); | ||
|
steven-sheehy marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| const forwarded = req.headers?.['x-forwarded-for']; | ||
| if (forwarded && typeof forwarded === 'string') { | ||
|
steven-sheehy marked this conversation as resolved.
Outdated
|
||
| const first = forwarded.split(',')[0].trim(); | ||
| if (first) return first; | ||
| } | ||
|
|
||
| return req.ip; | ||
|
steven-sheehy marked this conversation as resolved.
Outdated
|
||
| } | ||
|
steven-sheehy marked this conversation as resolved.
Outdated
|
||
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
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.