-
Notifications
You must be signed in to change notification settings - Fork 0
Open
Description
Severity: Medium
Component: Express Rate Limit Middleware
Status: Requires Review
Description
While rate limiting is implemented on /api routes (100 requests per window), the assessment encountered multiple 429 responses during testing, indicating the current limits may be too restrictive for legitimate use while potentially not restrictive enough for targeted attacks.
Evidence from Testing
From multiple test files:
- Rate limit headers:
X-RateLimit-Limit: 100,X-RateLimit-Remaining: 0 Retry-Afterheader present in 429 responses- Testing was significantly impacted by rate limiting (many 429s)
Issues to Review
- Window Duration: What is the current rate limit window? (15 min observed from logs)
- Endpoint-Specific Limits: Heavy endpoints like
/api/repositoriesmay need lower limits - Admin Endpoint Limits: Should admin endpoints have separate, stricter limits?
- Bypass Prevention: Is rate limiting based on IP only? Can it be bypassed via proxies?
- Error Handling: 429 responses should include Retry-After header ✓ (already present)
Current Implementation Gaps
- Generic 100 requests/window for all
/apiroutes - No endpoint-specific limits based on operation cost
- Potentially no differentiation between authenticated/unauthenticated users
- May not account for requests from behind proxies/CDNs
Recommended Improvements
// apps/backend/src/middleware/rateLimits.ts
import rateLimit from 'express-rate-limit';
import RedisStore from 'rate-limit-redis';
import { redis } from '../config/redis';
// General API rate limit
export const generalApiLimit = rateLimit({
windowMs: 15 * 60 * 1000, // 15 minutes
max: 100,
standardHeaders: true,
legacyHeaders: false,
message: 'Too many requests from this IP, please try again later.',
skip: (req) => {
// Skip rate limit for admin with valid token
return req.get('X-Admin-Token') === process.env.ADMIN_TOKEN;
},
...(process.env.REDIS_URL && {
store: new RedisStore({
client: redis,
prefix: 'rl:general:'
})
})
});
// Heavy operation rate limit (cloning, analysis)
export const heavyOperationLimit = rateLimit({
windowMs: 15 * 60 * 1000,
max: 10, // Much more restrictive
message: 'Repository analysis operations are rate limited. Please try again later.',
keyGenerator: (req) => {
// Combine IP with repoUrl to allow multiple repos but limit same repo
const repoUrl = req.body?.repoUrl || req.query?.repoUrl || 'unknown';
return `${req.ip}:${repoUrl}`;
},
...(process.env.REDIS_URL && {
store: new RedisStore({
client: redis,
prefix: 'rl:heavy:'
})
})
});
// Admin endpoint rate limit
export const adminLimit = rateLimit({
windowMs: 15 * 60 * 1000,
max: 20,
message: 'Too many admin requests.',
...(process.env.REDIS_URL && {
store: new RedisStore({
client: redis,
prefix: 'rl:admin:'
})
})
});
// Apply limits
app.use('/api', generalApiLimit);
app.use('/api/repositories', heavyOperationLimit);
app.use('/api/commits/stream', heavyOperationLimit);
app.use('/api/commits/cache', adminLimit);
app.use('/metrics', adminLimit);Recommended Configuration
// config/rateLimits.ts
export const RATE_LIMITS = {
// Standard API calls
general: {
windowMs: 15 * 60 * 1000, // 15 minutes
max: 100 // 100 requests
},
// Repository analysis (expensive)
heavyOps: {
windowMs: 60 * 60 * 1000, // 1 hour
max: 10 // 10 repositories per hour
},
// Admin operations
admin: {
windowMs: 15 * 60 * 1000,
max: 50
},
// Auth/validation failures
authFailure: {
windowMs: 15 * 60 * 1000,
max: 5 // Only 5 failed attempts
}
};Testing
# Test rate limit on general API
for i in {1..110}; do
curl -s http://localhost:3001/api/ -o /dev/null -w "%{http_code}\n"
done
# First 100 should succeed, then 429s
# Test heavy operation limit
for i in {1..15}; do
curl -X POST http://localhost:3001/api/repositories \
-H "Content-Type: application/json" \
-d '{"repoUrl":"https://github.com/octocat/Hello-World.git"}' \
-w "%{http_code}\n"
sleep 1
done
# Should hit limit faster than general APIMetadata
Metadata
Assignees
Labels
No labels