diff --git a/backend/__tests__/middleware/apiRateLimitGate.test.js b/backend/__tests__/middleware/apiRateLimitGate.test.js new file mode 100644 index 00000000..707ee828 --- /dev/null +++ b/backend/__tests__/middleware/apiRateLimitGate.test.js @@ -0,0 +1,118 @@ +/** + * The app-wide /api rate limiter has to actually be on the stack. + * + * It used to be registered from inside initializeRateLimiters(), which runs + * after the database is up — by which time every router, the /api 404 handler + * and the error handler are already mounted. Express dispatches middleware in + * registration order, so `app.use('/api/', generalRateLimiter)` landed below + * everything that answers a request and never executed for a matched route: + * the limit was silently inert on every deployment. + * + * Two halves here. The behavioural half pins what the gate delegates and what + * it deliberately lets past. The source half pins the thing that was actually + * broken — registration DEPTH — because that only exists in server.js and no + * unit test of the gate itself can catch a regression of it. + */ +const fs = require('fs'); +const path = require('path'); +const express = require('express'); +const request = require('supertest'); + +const { createApiRateLimitGate } = require('../../src/middleware/apiRateLimitGate'); + +describe('apiRateLimitGate — delegation', () => { + let limiterCalls; + let limiter; + + // Mirrors the real stack: health above the gate, gate above the routers. + const buildApp = () => { + const app = express(); + app.get(['/health', '/api/health'], (req, res) => res.json({ status: 'ok' })); + app.use(createApiRateLimitGate(() => limiter)); + app.get('/api/admin/events', (req, res) => res.json({ ok: true })); + app.get('/api/public/transfer-upload/:token', (req, res) => res.json({ ok: true })); + app.post('/api/admin/auth/login', (req, res) => res.json({ ok: true })); + app.get('/api/gallery/:slug/verify', (req, res) => res.json({ ok: true })); + app.get('/photos/x.jpg', (req, res) => res.json({ ok: true })); + return app; + }; + + beforeEach(() => { + limiterCalls = []; + limiter = (req, res) => { + limiterCalls.push(req.path); + res.status(429).json({ error: 'Too many requests, please try again later.' }); + }; + }); + + it('sends a plain /api request through the limiter', async () => { + const res = await request(buildApp()).get('/api/admin/events'); + expect(res.status).toBe(429); + // The full path reaches the limiter — the gate must not be mounted on + // '/api', or Express would strip the prefix and break the limiter's own + // public-endpoint and gallery-slug checks. + 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); + expect(limiterCalls).toEqual([]); + }); + + it('never counts the health probes, which poll every few seconds', async () => { + const app = buildApp(); + expect((await request(app).get('/health')).status).toBe(200); + expect((await request(app).get('/api/health')).status).toBe(200); + expect(limiterCalls).toEqual([]); + }); + + it('exempts bulk client transfer, which has its own per-minute limiters', async () => { + const res = await request(buildApp()).get('/api/public/transfer-upload/abc'); + expect(res.status).toBe(200); + expect(limiterCalls).toEqual([]); + }); + + it('exempts login and gallery-verify, which would 429 on the shared bucket', async () => { + const app = buildApp(); + expect((await request(app).post('/api/admin/auth/login')).status).toBe(200); + expect((await request(app).get('/api/gallery/some-slug/verify')).status).toBe(200); + expect(limiterCalls).toEqual([]); + }); + + it('passes through during the boot window, before the limiter exists', async () => { + limiter = undefined; + const res = await request(buildApp()).get('/api/admin/events'); + expect(res.status).toBe(200); + }); +}); + +describe('server.js — the gate is registered above the routers', () => { + const source = fs.readFileSync(path.resolve(__dirname, '../../server.js'), 'utf8'); + const lines = source.split('\n'); + const lineOf = (re) => { + const i = lines.findIndex((l) => re.test(l)); + expect(i).toBeGreaterThan(-1); + return i; + }; + + it('registers the gate before the first router mount', () => { + expect(lineOf(/createApiRateLimitGate\(/)) + .toBeLessThan(lineOf(/^app\.use\('\/api\/setup'/)); + }); + + it('registers the health routes above the gate so probes are never counted', () => { + expect(lineOf(/app\.get\(\[.\/health., .\/api\/health.\]/)) + .toBeLessThan(lineOf(/createApiRateLimitGate\(/)); + }); + + it('no longer registers the general limiter from initializeRateLimiters', () => { + // This is the regression: an app.use() there runs after the error handler + // and can never see a request. + expect(source).not.toMatch(/app\.use\('\/api\/?',\s*generalRateLimiter\)/); + }); + + it('registers the gate unmounted, so req.path keeps its /api prefix', () => { + expect(source).toMatch(/app\.use\(createApiRateLimitGate\(/); + }); +}); diff --git a/backend/server.js b/backend/server.js index cac83d3c..a7efbb9f 100644 --- a/backend/server.js +++ b/backend/server.js @@ -89,6 +89,7 @@ const { maintenanceMiddleware } = require('./src/middleware/maintenance'); const { sessionTimeoutMiddleware } = require('./src/middleware/sessionTimeout'); const { errorHandler, notFoundHandler } = require('./src/middleware/errorHandler'); const { createRateLimiter, createAuthRateLimiter } = require('./src/services/rateLimitService'); +const { createApiRateLimitGate } = require('./src/middleware/apiRateLimitGate'); const { getPublicSitePayload } = require('./src/services/publicSiteService'); const cookieParser = require('cookie-parser'); const { @@ -266,6 +267,38 @@ app.options('/api/*', cors(corsOptions)); // SSRF/path-allowlist model. app.use('/api/analytics/tracker', require('./src/routes/analyticsTrackerProxy')); +// Health check endpoint. `pid` + `uptime` let monitors (and the local E2E +// watchdog) detect a silent process restart between two checks. +// +// Served at BOTH paths: /health is the canonical one, /api/health exists +// because probes reasonably assume the API lives under /api and otherwise +// produce a "route not found" warning on every poll. Same handler, same body, +// same (public) exposure — the alias adds no information. +// +// Mounted HERE, ahead of the API middleware chain, so a probe running every +// couple of seconds does not pay for — or pollute — the body parsers, the +// request logger, the maintenance gate (/health is on its skip list anyway) +// or the rate limiter's per-IP budget. +app.get(['/health', '/api/health'], async (req, res) => { + try { + await db.raw('SELECT 1'); + res.json({ + status: 'ok', + timestamp: new Date().toISOString(), + pid: process.pid, + uptime: process.uptime() + }); + } catch (error) { + logger.error('Health check failed:', error); + res.status(503).json({ + status: 'error', + timestamp: new Date().toISOString(), + pid: process.pid, + uptime: process.uptime() + }); + } +}); + // Initialize rate limiters (they will be created dynamically) let generalRateLimiter; let authRateLimiter; @@ -463,9 +496,20 @@ async function handlePublicSiteRequest(req, res, next) { async function initializeRateLimiters() { generalRateLimiter = await createRateLimiter(); authRateLimiter = await createAuthRateLimiter(); - - // Apply rate limiting - app.use('/api/', generalRateLimiter); + + // The general limiter is NOT registered here — an app.use() at this point + // runs after the routers, the /api 404 handler and the error handler are + // already on the stack, so it can never see a request. It is reached through + // apiRateLimitGate below instead, which is registered ahead of the routers + // and reads this variable per request. + // + // These authRateLimiter registrations have the same problem and are equally + // inert. They are left as-is deliberately: `/api/auth` is a prefix, and the + // limiter's 5-per-window budget applies to every route under it — including + // GET /api/auth/session and POST /api/auth/password-strength, which the + // frontend calls far more often than five times per window. Activating them + // as written would lock legitimate users out, so wiring per-IP auth limiting + // up properly is a separate change. app.use('/api/auth', authRateLimiter); app.use('/api/gallery/:slug/verify', authRateLimiter); app.use('/api/admin/auth/login', authRateLimiter); @@ -473,7 +517,14 @@ async function initializeRateLimiters() { app.use('/api/setup/verify-token', authRateLimiter); } -// Note: Rate limiters will be initialized after database connection +// Rate limiting for /api. Registered HERE — above the routers — because +// Express dispatches in registration order; see apiRateLimitGate for the full +// story. The gate is a no-op until initializeRateLimiters() resolves, and it +// must stay unmounted (no path argument) so req.path keeps its /api prefix. +// /health and /api/health are mounted above this point and so are never +// counted, which matters because monitors poll them every couple of seconds. +app.use(createApiRateLimitGate(() => generalRateLimiter)); + app.use(express.json({ limit: '50mb' })); app.use(express.urlencoded({ extended: true, limit: '50mb' })); @@ -741,28 +792,6 @@ app.get( } ); -// Health check endpoint. `pid` + `uptime` let monitors (and the local E2E -// watchdog) detect a silent process restart between two checks. -app.get('/health', async (req, res) => { - try { - await db.raw('SELECT 1'); - res.json({ - status: 'ok', - timestamp: new Date().toISOString(), - pid: process.pid, - uptime: process.uptime() - }); - } catch (error) { - logger.error('Health check failed:', error); - res.status(503).json({ - status: 'error', - timestamp: new Date().toISOString(), - pid: process.pid, - uptime: process.uptime() - }); - } -}); - // Routes app.use('/api/setup', setupRoutes); // public first-run bootstrap (self-closes after setup) app.use('/api/auth', authRoutes); diff --git a/backend/src/middleware/apiRateLimitGate.js b/backend/src/middleware/apiRateLimitGate.js new file mode 100644 index 00000000..adab90e0 --- /dev/null +++ b/backend/src/middleware/apiRateLimitGate.js @@ -0,0 +1,72 @@ +/** + * Gate that applies the app-wide general rate limiter to /api requests. + * + * Why this exists rather than a plain `app.use('/api/', generalRateLimiter)`: + * the limiter is built asynchronously (it reads its window and its budget from + * app_settings), so it does not exist yet when the routers are mounted at module + * load. It was therefore registered from inside initializeRateLimiters(), which + * runs after the database is up — long after every router, the /api 404 handler + * and the error handler are already on the stack. Express dispatches middleware + * in registration order, so that app.use() landed BELOW everything that answers + * a request and never executed: the app-wide /api budget was silently inert. + * + * This gate is a stable function that can be registered at the right depth + * immediately and resolves the limiter per request, so whether the limit applies + * no longer depends on boot timing. + * + * Register it with `app.use(gate)` and NOT `app.use('/api', gate)`: Express + * strips the mount path from req.url for the duration of a mounted middleware, + * and rateLimitService's own decisions are written against the full path + * (`req.path.startsWith('/api/public/')` for the public-endpoints-only mode, and + * the `/api/(gallery|secure-images)/:slug` regex that finds the gallery token to + * skip on). Mounting it would silently break both. + */ + +// Prefixes the general limiter must not cover. +const EXEMPT_PREFIXES = [ + // Client file transfer, both directions: one request per file, from a link + // holder who carries no admin/gallery JWT and is therefore never skipped as + // "authenticated". A 100-per-15-minutes IP budget would cut a large upload or + // download off midway. Both already carry their own per-minute limiters + // (publicTransfer.js, publicTransferUpload.js), which is the right shape for + // bulk traffic. This prefix covers /api/public/transfer-upload as well. + '/api/public/transfer', +]; + +// Login and gallery-password endpoints are exempt for a subtler reason. The +// limiter returns rate_limit_auth_max_requests (5, not 100) as the budget for +// these paths, but counts them into the SAME per-IP bucket as every other /api +// request. Five ordinary unauthenticated calls — the branding and settings +// fetches a login page makes before anyone types a password — would therefore +// 429 the login itself for a whole window. Giving them a real per-IP budget +// means giving them their own bucket, which is what the (currently unreachable) +// dedicated authRateLimiter is for; until that is wired up they keep today's +// behaviour, where brute force is bounded by the per-account lockout in +// utils/authSecurity.js. +// +// The pattern is deliberately identical to rateLimitService's own isAuthEndpoint +// check, so the exempt set is exactly the set that would get the 5 budget. +const AUTH_ENDPOINT_RE = /\/(auth|login|gallery\/[^/]+\/verify)$/; + +/** + * @param {() => import('express').RequestHandler|undefined} getLimiter + * Reads the current general rate limiter. Returns undefined until + * initializeRateLimiters() has resolved. + * @returns {import('express').RequestHandler} + */ +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(); + + const limiter = getLimiter(); + // Boot window: the database is not up yet, so there is nothing to delegate + // to. Passing through is what every request did before this fix. + if (typeof limiter !== 'function') return next(); + + return limiter(req, res, next); + }; +} + +module.exports = { createApiRateLimitGate };