From a929affd7e737fd3d89591f2fb3fe21116479329 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Tue, 1 Sep 2026 19:19:40 +0200 Subject: [PATCH] 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. --- backend/__tests__/middleware/apiRateLimitGate.test.js | 9 +++++++++ backend/src/middleware/apiRateLimitGate.js | 11 ++++++++--- backend/src/services/rateLimitService.js | 7 +++++-- 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/backend/__tests__/middleware/apiRateLimitGate.test.js b/backend/__tests__/middleware/apiRateLimitGate.test.js index 707ee828..e1b2e22a 100644 --- a/backend/__tests__/middleware/apiRateLimitGate.test.js +++ b/backend/__tests__/middleware/apiRateLimitGate.test.js @@ -54,6 +54,15 @@ describe('apiRateLimitGate — delegation', () => { 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 () => { const res = await request(buildApp()).get('/photos/x.jpg'); expect(res.status).toBe(200); diff --git a/backend/src/middleware/apiRateLimitGate.js b/backend/src/middleware/apiRateLimitGate.js index 4e23710b..b3843ec2 100644 --- a/backend/src/middleware/apiRateLimitGate.js +++ b/backend/src/middleware/apiRateLimitGate.js @@ -56,9 +56,14 @@ const AUTH_ENDPOINT_RE = /\/(auth|login|gallery\/[^/]+\/verify)$/; */ function createApiRateLimitGate(getLimiter) { return function apiRateLimitGate(req, res, next) { - if (!req.path.startsWith('/api/')) return next(); - if (EXEMPT_PREFIXES.some((prefix) => req.path.startsWith(prefix))) return next(); - if (AUTH_ENDPOINT_RE.test(req.path)) return next(); + // Lower-cased for matching: Express's `case sensitive routing` is off by + // default, so `/API/admin/events` reaches the same handler as + // `/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(); // Boot window: the database is not up yet, so there is nothing to delegate diff --git a/backend/src/services/rateLimitService.js b/backend/src/services/rateLimitService.js index 701fb328..8e55ff29 100644 --- a/backend/src/services/rateLimitService.js +++ b/backend/src/services/rateLimitService.js @@ -138,8 +138,11 @@ function shouldSkipRateLimit(req, config) { // Check if we only rate limit public endpoints if (config.publicEndpointsOnly) { - const isPublicEndpoint = req.path.startsWith('/api/public/') || - req.path.startsWith('/api/gallery/') || + // Lower-cased: Express routing is case-insensitive by default, so an + // 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; return !isPublicEndpoint; }