feat: add rate limiter [IN-803] #1390
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a Redis-based rate limiting system for the application to protect against excessive request volumes. The implementation uses IP address hashing for GDPR compliance and supports configurable per-route and per-method rate limits.
Key Changes:
- Added rate limiter middleware that intercepts requests and enforces configurable rate limits
- Implemented IP extraction and hashing logic for GDPR compliance
- Configured Redis client integration from the crowd.dev repository
- Added rate limit headers to API responses
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/setup/rate-limiter.ts | Defines rate limiter configuration with default and route-specific limits |
| frontend/server/utils/rate-limiter.ts | Core rate limiting logic including IP extraction, route matching, and Redis operations |
| frontend/server/types/rate-limiter.ts | TypeScript type definitions for rate limiter configuration and results |
| frontend/server/middleware/rate-limiter.ts | Middleware that applies rate limiting to incoming requests |
| frontend/package.json | Added Redis client dependencies |
| frontend/nuxt.config.ts | Integrated rate limiter config and fixed comment typo |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
borfast
force-pushed
the
feat/IN-803-add-rate-limiter
branch
7 times, most recently
from
642333a to
11c9c65
Compare
borfast
force-pushed
the
feat/IN-803-add-rate-limiter
branch
4 times, most recently
from
26b57f4 to
852529c
Compare
borfast
force-pushed
the
feat/IN-803-add-rate-limiter
branch
2 times, most recently
from
f99d631 to
e8fd9f9
Compare
Signed-off-by: Raúl Santos <[email protected]>
borfast
force-pushed
the
feat/IN-803-add-rate-limiter
branch
from
e8fd9f9 to
0f32fd2
Compare
gaspergrom
left a comment
gaspergrom
left a comment
There was a problem hiding this comment.
There is no existing module to handle this?
|
Both @joanagmaia and I looked for something we could use but everything we found was too limited (could not specify limits per route or request method) and would not allow us to hash the IP addresses (which we need for compliance with privacy laws). |
| maxRequests: 5, | ||
| windowSeconds: 60, // 5 login attempts per minute | ||
| }, | ||
| { |
There was a problem hiding this comment.
can you add /api/community/[slug].post.ts also here to disable rate limiting. Its a webhook and it receives quite a lot of data from external app so we dont want to have it rate limited
There was a problem hiding this comment.
also badges endpoints shouldnt be rate limited as its necessary to display badges which users include in readme
epipav
left a comment
epipav
left a comment
There was a problem hiding this comment.
Looks good overall, added a comment about client memoization issue
| export async function getRedisClient( | ||
| redisUrl: string, | ||
| database: number, | ||
| enableReadyCheck = true, | ||
| ): Promise<RedisClientType> { | ||
| if (redisClient) { | ||
| return redisClient; | ||
| } | ||
|
|
||
| // Set the database number in the URL, replacing an existing one if present. | ||
| const url = setRedisDatabase(redisUrl, database); | ||
|
|
||
| redisClient = createClient({ | ||
| url, | ||
| socket: { | ||
| reconnectStrategy: (retries) => { | ||
| // Timeout at 3000ms | ||
| return Math.min(retries * 50, 3000); | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| redisClient.on('error', (err) => { | ||
| console.error('Redis Client Error', err); | ||
| }); | ||
|
|
||
| await redisClient.connect(); | ||
|
|
||
| // Wait for a ready state if requested | ||
| if (enableReadyCheck && !redisClient.isReady) { | ||
| await new Promise<void>((resolve) => { | ||
| redisClient!.on('ready', () => resolve()); | ||
| }); | ||
| } | ||
|
|
||
| return redisClient; | ||
| } |
There was a problem hiding this comment.
Looks like there's a small issue here
The client memoization doesn't account for changes in the database parameter, and subsequent calls will return the same client regardless of the database number
If we initialize the client with database=1, and then on the subsequent call we try getting the client for database=2, the client for database=1 will be returned instead because of the memoization
This pull request introduces a rate limiting system for the application, using Redis for tracking requests.
Because for the frontend directory the dependencies have to be installed
--ignore-workspace, the existing Redis client package from the crowd.dev repository was not reused. Instead, a light version was created.The environment variables
NUXT_REDIS_URLandNUXT_RATE_LIMITER_REDIS_DBmust be present and care must be taken so that the rate limiter does not use a database that is being used by something else.Jira ticket here.