Skip to content

feat: rate limit authentication endpoints #115

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.

Sign up for GitHub

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

Open
GitAddRemote wants to merge 1 commit into
base: main
Choose a base branch
from
+68 −3
Open

feat: rate limit authentication endpoints #115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions backend/.env.example
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,16 @@ USE_REDIS_CACHE=true
PORT=3001
APP_NAME=STATION BACKEND

# Rate Limiting (TTL in milliseconds)
THROTTLE_TTL=60000
THROTTLE_LIMIT=100
AUTH_LOGIN_THROTTLE_TTL=60000
AUTH_LOGIN_THROTTLE_LIMIT=10
AUTH_REGISTER_THROTTLE_TTL=60000
AUTH_REGISTER_THROTTLE_LIMIT=5
AUTH_FORGOT_THROTTLE_TTL=60000
AUTH_FORGOT_THROTTLE_LIMIT=5
Comment on lines +20 to +28
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example config calls these values “TTL in milliseconds”. To avoid misconfiguration, consider making the unit expectation unambiguous (e.g., rename vars to *_TTL_MS or adjust the comment to match the throttler’s expected units) and ensure the values used in AppModule/AuthController are consistent with that unit.

Copilot uses AI. Check for mistakes.

# UEX Sync Configuration
UEX_SYNC_ENABLED=true
UEX_CATEGORIES_SYNC_ENABLED=true
Expand Down
1 change: 1 addition & 0 deletions backend/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
"@nestjs/platform-express": "^10.0.0",
"@nestjs/schedule": "^6.0.1",
"@nestjs/swagger": "^7.4.2",
"@nestjs/throttler": "^6.5.0",
"@nestjs/typeorm": "^10.0.2",
"@types/bcrypt": "^6.0.0",
"axios": "^1.13.2",
Expand Down
21 changes: 20 additions & 1 deletion backend/src/app.module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import { Module, DynamicModule } from '@nestjs/common';
import { APP_GUARD } from '@nestjs/core';
import { TypeOrmModule } from '@nestjs/typeorm';
import { ConfigModule, ConfigService } from '@nestjs/config';
import { CacheModule } from '@nestjs/cache-manager';
import { ScheduleModule } from '@nestjs/schedule';
import { ThrottlerModule, ThrottlerGuard } from '@nestjs/throttler';
import { redisStore } from 'cache-manager-redis-yet';
import { UsersModule } from './modules/users/users.module';
import { AuthModule } from './modules/auth/auth.module';
Expand Down Expand Up @@ -36,6 +38,17 @@ if (!isTest) {
ConfigModule.forRoot({
isGlobal: true,
}),
ThrottlerModule.forRootAsync({
imports: [ConfigModule],
inject: [ConfigService],
useFactory: (configService: ConfigService) => [
{
name: 'default',
ttl: configService.get<number>('THROTTLE_TTL', 60000),
limit: configService.get<number>('THROTTLE_LIMIT', 100),
},
],
Comment on lines +44 to +50
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ThrottlerModule is configured from ConfigService, but these values may be strings when sourced from environment variables. To prevent passing string/NaN values into the throttler config, parse/validate THROTTLE_TTL and THROTTLE_LIMIT explicitly (e.g., Number(...) + finite checks) before returning them from the factory.

Suggested change
useFactory: (configService: ConfigService) => [
{
name: 'default',
ttl: configService.get<number>('THROTTLE_TTL', 60000),
limit: configService.get<number>('THROTTLE_LIMIT', 100),
},
],
useFactory: (configService: ConfigService) => {
const throttleTtl = Number(
configService.get<string | number>('THROTTLE_TTL', 60000),
);
const throttleLimit = Number(
configService.get<string | number>('THROTTLE_LIMIT', 100),
);
return [
{
name: 'default',
ttl: Number.isFinite(throttleTtl) ? throttleTtl : 60000,
limit: Number.isFinite(throttleLimit) ? throttleLimit : 100,
},
];
},

Copilot uses AI. Check for mistakes.
}),
...conditionalImports,
CacheModule.registerAsync({
isGlobal: true,
Expand Down Expand Up @@ -118,6 +131,12 @@ if (!isTest) {
OrgInventoryModule,
],
controllers: [AppController],
providers: [AppService],
providers: [
AppService,
{
provide: APP_GUARD,
useClass: ThrottlerGuard,
},
Comment on lines +134 to +139
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the default ThrottlerGuard without proxy/trust configuration means the rate-limit key is typically derived from req.ip. If this service is deployed behind a reverse proxy/load balancer, all clients may appear to come from the proxy IP unless trust proxy/X-Forwarded-For handling is configured. Consider using a custom ThrottlerGuard (override tracker) and/or ensuring the underlying HTTP adapter is configured to trust the proxy in production.

Copilot uses AI. Check for mistakes.
],
})
export class AppModule {}
20 changes: 19 additions & 1 deletion backend/src/modules/auth/auth.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
ApiBody,
ApiBearerAuth,
} from '@nestjs/swagger';
import { Throttle } from '@nestjs/throttler';
import { AuthService } from './auth.service';
import { LocalAuthGuard } from './local-auth.guard';
import { JwtAuthGuard } from './jwt-auth.guard';
Expand Down Expand Up @@ -43,6 +44,12 @@ export class AuthController {
})
@ApiResponse({ status: 200, description: 'Successfully logged in' })
@ApiResponse({ status: 401, description: 'Invalid credentials' })
@Throttle({
default: {
ttl: parseInt(process.env['AUTH_LOGIN_THROTTLE_TTL'] ?? '60000'),
limit: parseInt(process.env['AUTH_LOGIN_THROTTLE_LIMIT'] ?? '10'),
},
Comment on lines +47 to +51
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The per-route throttling config reads directly from process.env and uses parseInt without validation. If an env var is set to a non-numeric value, parseInt can yield NaN or partially-parse (e.g. '10s' -> 10), which can cause unexpected throttling behavior at runtime. Consider parsing with Number(...) + Number.isFinite checks (or a small helper) and failing fast on invalid config, and prefer using ConfigService-derived constants for consistency with the global throttler config.

Copilot uses AI. Check for mistakes.
})
@UseGuards(LocalAuthGuard)
@Post('login')
async login(@Request() req: ExpressRequest) {
Comment on lines +47 to 55
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New rate-limiting behavior on auth endpoints isn’t covered by tests yet. Since there’s already extensive e2e coverage for /auth routes, it would be valuable to add an e2e case that exceeds the limit and asserts a 429 response (and presence/format of Retry-After if expected).

Copilot uses AI. Check for mistakes.
Expand All @@ -52,7 +59,12 @@ export class AuthController {
@ApiOperation({ summary: 'Register new user' })
@ApiResponse({ status: 201, description: 'User successfully registered' })
@ApiResponse({ status: 400, description: 'Invalid input data' })
// Registration Route: No guards required
@Throttle({
default: {
ttl: parseInt(process.env['AUTH_REGISTER_THROTTLE_TTL'] ?? '60000'),
limit: parseInt(process.env['AUTH_REGISTER_THROTTLE_LIMIT'] ?? '5'),
},
Comment on lines +62 to +66
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as the login throttle block: env parsing here can silently produce NaN/incorrect values (parseInt partial parsing, no radix/finite checks). Please centralize and validate these numeric configs (e.g., parse once at module load) so invalid env configuration fails fast and behavior is predictable.

Copilot uses AI. Check for mistakes.
})
@Post('register')
async register(@Body() userDto: UserDto) {
return this.authService.register(userDto);
Expand Down Expand Up @@ -88,6 +100,12 @@ export class AuthController {
description:
'If an account with that email exists, a password reset link has been sent',
})
@Throttle({
default: {
ttl: parseInt(process.env['AUTH_FORGOT_THROTTLE_TTL'] ?? '60000'),
limit: parseInt(process.env['AUTH_FORGOT_THROTTLE_LIMIT'] ?? '5'),
},
Comment on lines +103 to +107
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same env parsing/validation concern for forgot-password throttling: parseInt on env vars can yield NaN or partially-parsed numbers. Recommend validating these values and/or sourcing them via ConfigService-derived constants to avoid misconfiguration leading to unexpected throttle windows/limits.

Copilot uses AI. Check for mistakes.
})
@Post('forgot-password')
async forgotPassword(@Body() forgotPasswordDto: ForgotPasswordDto) {
return this.authService.requestPasswordReset(forgotPasswordDto.email);
Expand Down
19 changes: 18 additions & 1 deletion pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading