fix(security): close the case-sensitivity bypass in the API rate limiter
Express's `case sensitive routing` is off by default, so /API/admin/events reaches the same handler as /api/admin/events. Both the gate's `/api/` prefix test and rateLimitService's public-endpoint classification compared the raw path, so simply upper-casing a letter skipped the limiter entirely. Verified against a real Express app before fixing: /api/admin/events routes and hits the gate; /API/admin/events and /Api/Admin/Events route and miss it. Both now match on a lower-cased path. The auth gate added alongside was already immune -- its patterns carry the `i` flag for exactly this reason. Not changed: rateLimitSecurity.hasValidAdminToken's /api/admin/ test has the same shape, but there the case-sensitive comparison fails safe -- an upper-cased path simply does not get the admin skip, so it is rate limited rather than exempted. Making it case-insensitive would widen a skip, so it is left alone. maintenance.js's isAdminRoute is fail-safe for the same reason.
This commit is contained in:
@@ -54,6 +54,15 @@ describe('apiRateLimitGate — delegation', () => {
|
|||||||
expect(limiterCalls).toEqual(['/api/admin/events']);
|
expect(limiterCalls).toEqual(['/api/admin/events']);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('still limits an upper-cased /api path, which Express routes the same', async () => {
|
||||||
|
// Express's `case sensitive routing` is off by default, so /API/admin/events
|
||||||
|
// reaches the same handler. A case-sensitive prefix test in the gate was a
|
||||||
|
// free bypass of the limiter.
|
||||||
|
const res = await request(buildApp()).get('/API/admin/events');
|
||||||
|
expect(res.status).toBe(429);
|
||||||
|
expect(limiterCalls).toEqual(['/API/admin/events']);
|
||||||
|
});
|
||||||
|
|
||||||
it('leaves non-/api requests alone', async () => {
|
it('leaves non-/api requests alone', async () => {
|
||||||
const res = await request(buildApp()).get('/photos/x.jpg');
|
const res = await request(buildApp()).get('/photos/x.jpg');
|
||||||
expect(res.status).toBe(200);
|
expect(res.status).toBe(200);
|
||||||
|
|||||||
@@ -56,9 +56,14 @@ const AUTH_ENDPOINT_RE = /\/(auth|login|gallery\/[^/]+\/verify)$/;
|
|||||||
*/
|
*/
|
||||||
function createApiRateLimitGate(getLimiter) {
|
function createApiRateLimitGate(getLimiter) {
|
||||||
return function apiRateLimitGate(req, res, next) {
|
return function apiRateLimitGate(req, res, next) {
|
||||||
if (!req.path.startsWith('/api/')) return next();
|
// Lower-cased for matching: Express's `case sensitive routing` is off by
|
||||||
if (EXEMPT_PREFIXES.some((prefix) => req.path.startsWith(prefix))) return next();
|
// default, so `/API/admin/events` reaches the same handler as
|
||||||
if (AUTH_ENDPOINT_RE.test(req.path)) return next();
|
// `/api/admin/events`. A case-sensitive prefix test here would have been a
|
||||||
|
// free bypass of the limiter (verified against a real Express app).
|
||||||
|
const path = req.path.toLowerCase();
|
||||||
|
if (!path.startsWith('/api/')) return next();
|
||||||
|
if (EXEMPT_PREFIXES.some((prefix) => path.startsWith(prefix))) return next();
|
||||||
|
if (AUTH_ENDPOINT_RE.test(path)) return next();
|
||||||
|
|
||||||
const limiter = getLimiter();
|
const limiter = getLimiter();
|
||||||
// Boot window: the database is not up yet, so there is nothing to delegate
|
// Boot window: the database is not up yet, so there is nothing to delegate
|
||||||
|
|||||||
@@ -138,8 +138,11 @@ function shouldSkipRateLimit(req, config) {
|
|||||||
|
|
||||||
// Check if we only rate limit public endpoints
|
// Check if we only rate limit public endpoints
|
||||||
if (config.publicEndpointsOnly) {
|
if (config.publicEndpointsOnly) {
|
||||||
const isPublicEndpoint = req.path.startsWith('/api/public/') ||
|
// Lower-cased: Express routing is case-insensitive by default, so an
|
||||||
req.path.startsWith('/api/gallery/') ||
|
// upper-cased path reaches the same handler and must classify the same way.
|
||||||
|
const lowerPath = req.path.toLowerCase();
|
||||||
|
const isPublicEndpoint = lowerPath.startsWith('/api/public/') ||
|
||||||
|
lowerPath.startsWith('/api/gallery/') ||
|
||||||
isAuthEndpoint;
|
isAuthEndpoint;
|
||||||
return !isPublicEndpoint;
|
return !isPublicEndpoint;
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user