fix(security): actually apply the general API rate limiter

app.use('/api/', generalRateLimiter) lives inside initializeRateLimiters(),
which is defined at line 463 but not called until 1048 -- by which point the
routers (767+), the /api notFoundHandler (1002) and errorHandler (1029) are
already on the stack. All six app.use() calls in it therefore append BELOW the
error handler and can never see a request. generalRateLimiter had no other
registration path.

So the entire /api surface had no IP-based request limit, except the handful
of routes carrying their own inline rateLimit() (public quotes, contracts,
payment-check, transfers, the analytics proxy). The admin Settings
rate-limiting UI -- rate_limit_enabled, rate_limit_max_requests -- was writing
to a control that did nothing.

Fixed with a stable gate registered above the routers that resolves the
limiter per request, so there is no boot delay: it is a pass-through until
initializeRateLimiters() resolves, exactly matching prior behaviour.

Registered unmounted (app.use(gate), not app.use('/api', gate)) because
Express strips the mount path from req.url and rateLimitService's own logic is
written against the full path -- req.path.startsWith('/api/public/') and the
/api/(gallery|secure-images)/:slug regex it uses to find a gallery token to
skip on. Mounting it would have silently broken both.

Deliberately excluded, each for a concrete reason:
- /health and /api/health, mounted above the gate: a 2s probe is 450
  req/window and would 429 the container healthcheck.
- /api/public/transfer and transfer-upload: one request per file from a link
  holder with no JWT, so never skipped as authenticated; a large transfer
  would be cut off mid-way. Both already have tighter per-minute limiters.
- login and gallery-verify: the limiter returns authMaxRequests (5) as their
  budget but counts them into the SAME per-IP bucket as every other /api call,
  so the branding and settings fetches a login page makes before anyone types
  a password would 429 the login itself for a full window. Giving these a real
  per-IP limit means giving them their own bucket.

Bulk gallery and admin traffic is unaffected: skip_authenticated defaults true
and cookie tokens are promoted to Authorization before the gate runs, and
skipped requests do not increment the counter.

Also adds /api/health as an alias of /health -- one handler, identical
exposure -- which silences a ~2s probe warning. Registered above the API
middleware chain deliberately: left at its original position it would have
passed through apiRequestLogger and through maintenanceMiddleware, whose
skip-list contains /health but not /api/health, so it would have 503'd during
maintenance while /health returned 200.

The tests pin registration depth by source inspection as well as behaviour,
because depth is what was broken and no unit test of the gate can catch it.

Refs testplan REPORT.md, /api/health warning; rate-limiter gap found while
fixing it.
This commit is contained in:
Paul Nothaft
2026-09-02 09:43:10 +02:00
parent a89057df1d
commit b0f33c1744
3 changed files with 245 additions and 26 deletions
@@ -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\(/);
});
});
+55 -26
View File
@@ -89,6 +89,7 @@ const { maintenanceMiddleware } = require('./src/middleware/maintenance');
const { sessionTimeoutMiddleware } = require('./src/middleware/sessionTimeout'); const { sessionTimeoutMiddleware } = require('./src/middleware/sessionTimeout');
const { errorHandler, notFoundHandler } = require('./src/middleware/errorHandler'); const { errorHandler, notFoundHandler } = require('./src/middleware/errorHandler');
const { createRateLimiter, createAuthRateLimiter } = require('./src/services/rateLimitService'); const { createRateLimiter, createAuthRateLimiter } = require('./src/services/rateLimitService');
const { createApiRateLimitGate } = require('./src/middleware/apiRateLimitGate');
const { getPublicSitePayload } = require('./src/services/publicSiteService'); const { getPublicSitePayload } = require('./src/services/publicSiteService');
const cookieParser = require('cookie-parser'); const cookieParser = require('cookie-parser');
const { const {
@@ -266,6 +267,38 @@ app.options('/api/*', cors(corsOptions));
// SSRF/path-allowlist model. // SSRF/path-allowlist model.
app.use('/api/analytics/tracker', require('./src/routes/analyticsTrackerProxy')); 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) // Initialize rate limiters (they will be created dynamically)
let generalRateLimiter; let generalRateLimiter;
let authRateLimiter; let authRateLimiter;
@@ -463,9 +496,20 @@ async function handlePublicSiteRequest(req, res, next) {
async function initializeRateLimiters() { async function initializeRateLimiters() {
generalRateLimiter = await createRateLimiter(); generalRateLimiter = await createRateLimiter();
authRateLimiter = await createAuthRateLimiter(); authRateLimiter = await createAuthRateLimiter();
// Apply rate limiting // The general limiter is NOT registered here — an app.use() at this point
app.use('/api/', generalRateLimiter); // 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/auth', authRateLimiter);
app.use('/api/gallery/:slug/verify', authRateLimiter); app.use('/api/gallery/:slug/verify', authRateLimiter);
app.use('/api/admin/auth/login', authRateLimiter); app.use('/api/admin/auth/login', authRateLimiter);
@@ -473,7 +517,14 @@ async function initializeRateLimiters() {
app.use('/api/setup/verify-token', authRateLimiter); 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.json({ limit: '50mb' }));
app.use(express.urlencoded({ extended: true, 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 // Routes
app.use('/api/setup', setupRoutes); // public first-run bootstrap (self-closes after setup) app.use('/api/setup', setupRoutes); // public first-run bootstrap (self-closes after setup)
app.use('/api/auth', authRoutes); app.use('/api/auth', authRoutes);
@@ -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 };