[Feat] Add Rate Limiting to authenticate endpoint #445
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces several enhancements to the API project's security and configuration management. The changes focus on implementing rate limiting, adding Redis support, and improving user authentication mechanisms. The modifications span multiple files, including Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (4)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
No dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No dependency changes detected in pull request |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
api/src/services/user.service.ts (1)
220-264: Add Unit Tests for New Authentication LogicThe newly introduced methods (
getLoginAttempts,handlePasswordAuthentication,incrementLoginAttempts,resetLoginAttempts,saveUser) enhance the authentication flow. To ensure their reliability and correctness, consider adding unit tests covering various scenarios, including:
- Successful authentication with correct credentials.
- Authentication failures due to incorrect passwords.
- Handling of login attempt increments and resets.
- Behavior when Redis is available versus when it is not.
api/src/redis/redis.module.ts (1)
11-13: Enhance configuration validation.Consider validating the Redis password when provided to ensure secure configuration.
const redisHost = configService.get<string>('REDIS_HOST'); const redisPort = configService.get<number>('REDIS_PORT'); const redisPassword = configService.get<string>('REDIS_PASSWORD'); + +if (redisPassword === '') { + Logger.warn('Redis password is empty. This is not recommended for production.'); +}api/src/redis/user-login-attempts.storage.ts (1)
29-47: Add rate limiting metrics.Consider adding metrics for monitoring rate limiting effectiveness.
+private metrics = { + totalAttempts: 0, + blockedAttempts: 0, +}; async getUserAttempts(userKey: string): Promise<number> { + this.metrics.totalAttempts++; if (this.redisClient) { try { const attempts = await this.redisClient.get(userKey); - return attempts ? parseInt(attempts, 10) : 0; + const count = attempts ? parseInt(attempts, 10) : 0; + if (count > 0) this.metrics.blockedAttempts++; + return count; } catch (error) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
api/package.json(3 hunks)api/src/app.module.ts(4 hunks)api/src/controllers/auth.controller.ts(2 hunks)api/src/guards/custom-throttler.guard.ts(1 hunks)api/src/redis/redis.module.ts(1 hunks)api/src/redis/user-login-attempts.storage.ts(1 hunks)api/src/services/user.service.ts(2 hunks)webapp/angular.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test
🔇 Additional comments (4)
api/src/app.module.ts (1)
85-88: Global Throttling Guard Implemented SuccessfullyThe
CustomThrottlerGuardhas been correctly set as a global guard usingAPP_GUARD, ensuring that rate limiting is applied across all controllers in the application.api/src/controllers/auth.controller.ts (1)
Line range hint
183-234: Enhance error handling for rate-limited requests.The token endpoint should provide clear feedback when rate limits are exceeded.
webapp/angular.json (1)
108-109: LGTM!Disabling analytics is a reasonable choice for privacy concerns.
api/package.json (1)
35-35: LGTM! Verify Redis configuration.The added dependencies align well with the rate limiting implementation:
@nestjs/configand@nestjs/throttlerversions are compatible with NestJS v10ioredisis appropriate for distributed rate limiting storagePlease ensure Redis configuration is properly validated on startup. Run this script to check if Redis config validation exists:
Also applies to: 41-41, 55-55
✅ Verification successful
Redis configuration validation is properly implemented ✅
The codebase already includes robust Redis configuration validation in
redis.module.ts:
- Validates required
REDIS_HOSTandREDIS_PORTenvironment variables- Includes graceful fallback to in-memory storage when Redis is not configured
- Provides clear warning logs for missing configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if Redis configuration validation exists # Expected: Find Redis configuration validation in config files # Look for Redis configuration validation rg -i 'redis.*validation|validate.*redis' --type ts # Look for Redis environment variables rg -i 'redis.*env|env.*redis' --type tsLength of output: 95
Script:
#!/bin/bash # Search for Redis and Throttler configuration # Look for files containing Redis setup rg -i "redis" --type ts -A 5 # Look for ThrottlerModule configuration ast-grep --pattern 'ThrottlerModule.forRoot' -l # Look for ConfigModule setup with validation ast-grep --pattern 'ConfigModule.forRoot({ $$$ validate: $$$ $$$ })' # Find potential config files fd -e ts -e json configLength of output: 7925
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
api/src/redis/user-login-attempts.storage.ts (2)
7-7: Add type definition for storage value.Define an interface for the storage value structure to improve type safety and maintainability.
+interface LoginAttempt { + attempts: number; + expiry: number; +} -private inMemoryStorage: Map<string, { attempts: number; expiry: number }> = new Map(); +private inMemoryStorage: Map<string, LoginAttempt> = new Map();
55-62: Add error handling and metrics to cleanup process.The cleanup process should handle errors and track metrics for monitoring.
private cleanupExpiredEntries(): void { - const now = Date.now(); - for (const [key, value] of this.inMemoryStorage.entries()) { - if (value.expiry <= now) { - this.inMemoryStorage.delete(key); + try { + const now = Date.now(); + let cleanedEntries = 0; + for (const [key, value] of this.inMemoryStorage.entries()) { + if (value.expiry <= now) { + this.inMemoryStorage.delete(key); + cleanedEntries++; + } } + this.logger.debug(`Cleaned up ${cleanedEntries} expired entries`); + } catch (error) { + this.logger.error('Error during cleanup:', error.message); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
api/src/redis/user-login-attempts.storage.ts(1 hunks)api/src/services/user.service.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/services/user.service.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (2)
api/src/redis/user-login-attempts.storage.ts (2)
7-7: Consider implementing memory limits for in-memory storage.The in-memory storage should have a maximum size limit to prevent memory exhaustion attacks.
9-15: Verify Redis client connection status.The constructor should verify that the Redis client is properly connected before using it.
constructor(@Inject('REDIS_CLIENT') private readonly redisClient?: Redis) { if (!this.redisClient) { this.logger.warn('Redis client is not initialized. Using in-memory storage.'); // Set up periodic cleanup every 5 minutes setInterval(() => this.cleanupExpiredEntries(), 5 * 60 * 1000); + } else { + // Verify Redis connection + this.redisClient.ping().catch(error => { + this.logger.error('Redis connection failed:', error.message); + throw new Error('Redis connection failed'); + }); } }
Before submitting the PR, please make sure you do the following
Contributor license agreement
For us it's important to have the agreement of our contributors to use their work, whether it be code or documentation. Therefore, we are asking all contributors to sign a contributor license agreement (CLA) as commonly accepted in most open source projects. Just open the pull request and our CLA bot will prompt you briefly.
Please check our contribution guidelines for some help in the process.
Summary by CodeRabbit
Release Notes
New Features
Security Enhancements
Performance
Chores