diff --git a/backend/__tests__/middleware/authRateLimitGate.test.js b/backend/__tests__/middleware/authRateLimitGate.test.js new file mode 100644 index 00000000..da0f7999 --- /dev/null +++ b/backend/__tests__/middleware/authRateLimitGate.test.js @@ -0,0 +1,293 @@ +/** + * The credential endpoints need a per-IP limit — and nothing else may get one. + * + * The five `app.use('/api/auth', authRateLimiter)`-style registrations inside + * initializeRateLimiters() were inert for the same reason the general limiter + * was: they run after the routers and the error handler are already mounted. + * They could not just be moved up, either — `/api/auth` is a prefix, so a + * 5-per-window budget would have covered GET /api/auth/session and POST + * /api/auth/password-strength, which the frontend calls far more than five + * times per window. Moving them as written would have locked users out. + * + * So these tests pin both directions: the credential endpoints ARE limited, + * and the benign high-frequency endpoints under the same prefixes are NOT, + * even after many times the auth budget. Plus the two properties that make the + * budget survivable in production — only failures count, and the auth bucket is + * separate from the general /api bucket. + * + * The source half pins registration DEPTH, which is what was broken and which + * no unit test of the gate itself can catch. + */ +const fs = require('fs'); +const path = require('path'); +const express = require('express'); +const request = require('supertest'); + +let mockSettingsRows = []; + +jest.mock('../../src/database/db', () => ({ + db: jest.fn(() => ({ + whereIn: jest.fn().mockImplementation(() => Promise.resolve(mockSettingsRows)) + })) +})); + +jest.mock('../../src/utils/logger', () => ({ + info: jest.fn(), warn: jest.fn(), error: jest.fn(), debug: jest.fn() +})); + +const { createAuthRateLimitGate } = require('../../src/middleware/authRateLimitGate'); +const { createApiRateLimitGate } = require('../../src/middleware/apiRateLimitGate'); +const { + createAuthRateLimiter, + createRateLimiter, + clearSettingsCache +} = require('../../src/services/rateLimitService'); + +const setting = (key, value) => ({ setting_key: key, setting_value: JSON.stringify(value) }); + +// Mirrors the real stack: gates above the routers, unmounted so req.path keeps +// its /api prefix. `loginSucceeds` lets a test drive the skipSuccessfulRequests +// behaviour without a database. +let loginSucceeds = false; + +function mountRoutes(app) { + const fail = (res) => res.status(401).json({ error: 'Invalid credentials' }); + + // Credential endpoints — the group that must be limited. + app.post('/api/auth/admin/login', (req, res) => + (loginSucceeds ? res.json({ user: {} }) : fail(res))); + app.post('/api/auth/admin/login/mfa', (req, res) => fail(res)); + app.post('/api/auth/gallery/verify', (req, res) => fail(res)); + app.post('/api/auth/gallery/share-login', (req, res) => fail(res)); + app.post('/api/auth/gallery/:slug/client-login', (req, res) => fail(res)); + app.post('/api/setup/verify-token', (req, res) => fail(res)); + app.post('/api/setup/admin', (req, res) => fail(res)); + app.post('/api/customer/auth/login', (req, res) => fail(res)); + app.post('/api/customer/auth/password-reset', (req, res) => fail(res)); + + // Benign endpoints living under the very same prefixes the old registrations + // covered. Every one of these is called more than five times per window by a + // normal session. + app.get('/api/auth/session', (req, res) => res.json({ authenticated: false })); + app.post('/api/auth/password-strength', (req, res) => res.json({ score: 3 })); + app.post('/api/auth/logout', (req, res) => res.json({ ok: true })); + app.post('/api/auth/gallery/logout', (req, res) => res.json({ ok: true })); + app.post('/api/auth/admin/change-password', (req, res) => res.json({ ok: true })); + app.get('/api/auth/admin/sso/callback', (req, res) => res.json({ ok: true })); + app.get('/api/setup/status', (req, res) => res.json({ needsSetup: false })); + app.get('/api/customer/auth/session', (req, res) => res.json({ ok: true })); + app.get('/api/gallery/:slug/verify-token/:token', (req, res) => res.json({ valid: true })); + app.get('/api/public/settings', (req, res) => res.json({ ok: true })); +} + +async function buildApp({ withGeneralGate = false } = {}) { + const app = express(); + if (withGeneralGate) { + const generalLimiter = await createRateLimiter(); + app.use(createApiRateLimitGate(() => generalLimiter)); + } + const authLimiter = await createAuthRateLimiter(); + app.use(createAuthRateLimitGate(() => authLimiter)); + app.use(express.json()); + mountRoutes(app); + return app; +} + +beforeEach(() => { + mockSettingsRows = []; + loginSucceeds = false; + clearSettingsCache(); +}); + +describe('authRateLimitGate — credential endpoints are limited', () => { + it('429s admin login on the 6th failed attempt in the window', async () => { + const app = await buildApp(); + for (let i = 0; i < 5; i++) { + expect((await request(app).post('/api/auth/admin/login').send({})).status).toBe(401); + } + const blocked = await request(app).post('/api/auth/admin/login').send({}); + expect(blocked.status).toBe(429); + // Shape the frontend already branches on (AdminLoginPage, GalleryPage, + // CustomerLoginPage and SetupPage all check status === 429). + expect(blocked.body.error).toBe('Too many authentication attempts, please try again later.'); + expect(blocked.headers['ratelimit-limit']).toBe('5'); + }); + + it.each([ + ['/api/auth/admin/login/mfa'], + ['/api/auth/gallery/verify'], + ['/api/auth/gallery/share-login'], + ['/api/auth/gallery/some-slug/client-login'], + ['/api/setup/verify-token'], + ['/api/setup/admin'], + ['/api/customer/auth/login'], + ['/api/customer/auth/password-reset'] + ])('429s %s once the budget is spent', async (endpoint) => { + const app = await buildApp(); + for (let i = 0; i < 5; i++) { + expect((await request(app).post(endpoint).send({})).status).toBe(401); + } + expect((await request(app).post(endpoint).send({})).status).toBe(429); + }); + + it('shares one budget across the credential endpoints, so spraying is bounded', async () => { + const app = await buildApp(); + const sprayed = [ + '/api/auth/admin/login', + '/api/auth/gallery/verify', + '/api/customer/auth/login', + '/api/setup/verify-token', + '/api/auth/gallery/share-login' + ]; + for (const endpoint of sprayed) { + expect((await request(app).post(endpoint).send({})).status).toBe(401); + } + expect((await request(app).post('/api/auth/admin/login').send({})).status).toBe(429); + }); + + it('honours rate_limit_auth_max_requests from app_settings', async () => { + mockSettingsRows = [setting('rate_limit_auth_max_requests', 2)]; + const app = await buildApp(); + expect((await request(app).post('/api/auth/admin/login').send({})).status).toBe(401); + expect((await request(app).post('/api/auth/admin/login').send({})).status).toBe(401); + expect((await request(app).post('/api/auth/admin/login').send({})).status).toBe(429); + }); + + it('does nothing when rate_limit_enabled is false', async () => { + mockSettingsRows = [setting('rate_limit_enabled', false)]; + const app = await buildApp(); + for (let i = 0; i < 20; i++) { + expect((await request(app).post('/api/auth/admin/login').send({})).status).toBe(401); + } + }); + + it('only counts failures, so successful logins never consume the budget', async () => { + // This is what makes a 5-per-window per-IP budget safe behind NAT: a room + // of guests on one venue IP who all type the right password count zero. + const app = await buildApp(); + loginSucceeds = true; + for (let i = 0; i < 30; i++) { + expect((await request(app).post('/api/auth/admin/login').send({})).status).toBe(200); + } + // The budget is still fully intact for real failures. + loginSucceeds = false; + for (let i = 0; i < 5; i++) { + expect((await request(app).post('/api/auth/admin/login').send({})).status).toBe(401); + } + expect((await request(app).post('/api/auth/admin/login').send({})).status).toBe(429); + }); + + it('passes through during the boot window, before the limiter exists', async () => { + const app = express(); + app.use(createAuthRateLimitGate(() => undefined)); + mountRoutes(app); + for (let i = 0; i < 10; i++) { + expect((await request(app).post('/api/auth/admin/login').send({})).status).toBe(401); + } + }); +}); + +describe('authRateLimitGate — benign endpoints are never limited', () => { + // 40 calls each: eight times the 5-per-window auth budget. Every one of these + // would have been covered by the old app.use('/api/auth', ...) prefix. + it.each([ + ['GET', '/api/auth/session'], + ['POST', '/api/auth/password-strength'], + ['POST', '/api/auth/logout'], + ['POST', '/api/auth/gallery/logout'], + ['POST', '/api/auth/admin/change-password'], + ['GET', '/api/auth/admin/sso/callback'], + ['GET', '/api/setup/status'], + ['GET', '/api/customer/auth/session'], + ['GET', '/api/gallery/some-slug/verify-token/abc'] + ])('%s %s stays available after 40 calls', async (method, endpoint) => { + const app = await buildApp(); + for (let i = 0; i < 40; i++) { + const res = await request(app)[method.toLowerCase()](endpoint).send({}); + expect(res.status).toBe(200); + } + }); + + it('does not treat a GET on a credential path as an attempt', async () => { + // The table is method-specific: only the POST spends budget. + const app = await buildApp(); + for (let i = 0; i < 20; i++) { + // No GET handler is mounted, so a 404 proves the gate let it through + // rather than answering 429. + expect((await request(app).get('/api/auth/admin/login')).status).toBe(404); + } + expect((await request(app).post('/api/auth/admin/login').send({})).status).toBe(401); + }); + + it('matches case-insensitively, since Express routing is case-insensitive', async () => { + const app = await buildApp(); + for (let i = 0; i < 5; i++) { + expect((await request(app).post('/api/auth/admin/LOGIN').send({})).status).toBe(401); + } + expect((await request(app).post('/api/auth/admin/login').send({})).status).toBe(429); + }); +}); + +describe('authRateLimitGate — its bucket is independent of the general /api bucket', () => { + it('an exhausted general budget still leaves the login budget intact', async () => { + mockSettingsRows = [ + setting('rate_limit_max_requests', 3), + setting('rate_limit_auth_max_requests', 5) + ]; + const app = await buildApp({ withGeneralGate: true }); + + for (let i = 0; i < 3; i++) { + expect((await request(app).get('/api/public/settings')).status).toBe(200); + } + expect((await request(app).get('/api/public/settings')).status).toBe(429); + + // The exact failure the old wiring would have produced: the branding and + // settings fetches a login page makes before anyone types a password + // 429ing the login itself. Separate stores mean it cannot happen. + expect((await request(app).post('/api/auth/admin/login').send({})).status).toBe(401); + }); + + it('an exhausted login budget still leaves the general budget intact', async () => { + mockSettingsRows = [ + setting('rate_limit_max_requests', 3), + setting('rate_limit_auth_max_requests', 5) + ]; + const app = await buildApp({ withGeneralGate: true }); + + for (let i = 0; i < 5; i++) { + expect((await request(app).post('/api/auth/admin/login').send({})).status).toBe(401); + } + expect((await request(app).post('/api/auth/admin/login').send({})).status).toBe(429); + + for (let i = 0; i < 3; i++) { + expect((await request(app).get('/api/public/settings')).status).toBe(200); + } + }); +}); + +describe('server.js — the auth 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(/createAuthRateLimitGate\(/)) + .toBeLessThan(lineOf(/^app\.use\('\/api\/setup'/)); + }); + + it('registers the gate unmounted, so req.path keeps its /api prefix', () => { + expect(source).toMatch(/app\.use\(createAuthRateLimitGate\(/); + }); + + it('no longer registers authRateLimiter on a prefix from initializeRateLimiters', () => { + // This is the regression: an app.use() there runs after the error handler + // and can never see a request — and '/api/auth' as a prefix would have + // covered GET /api/auth/session at the 5-per-window auth budget. + expect(source).not.toMatch(/app\.use\('\/api\/auth',\s*authRateLimiter\)/); + expect(source).not.toMatch(/app\.use\('[^']*',\s*authRateLimiter\)/); + }); +}); diff --git a/backend/server.js b/backend/server.js index a7efbb9f..e78da6f8 100644 --- a/backend/server.js +++ b/backend/server.js @@ -90,6 +90,7 @@ 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 { createAuthRateLimitGate } = require('./src/middleware/authRateLimitGate'); const { getPublicSitePayload } = require('./src/services/publicSiteService'); const cookieParser = require('cookie-parser'); const { @@ -497,24 +498,19 @@ async function initializeRateLimiters() { generalRateLimiter = await createRateLimiter(); authRateLimiter = await createAuthRateLimiter(); - // 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. + // Neither limiter is 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. Both are reached through their gates + // below, which are registered ahead of the routers and read these variables + // 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); - app.use('/api/setup/admin', authRateLimiter); - app.use('/api/setup/verify-token', authRateLimiter); + // The five prefix registrations of authRateLimiter that used to live here + // were inert for that reason, and could not simply be moved up either: the + // widest of them mounted on the whole /api/auth router, so the 5-per-window + // auth budget would have covered GET /api/auth/session and POST + // /api/auth/password-strength, which the frontend calls far more often than + // five times per window. authRateLimitGate matches exact method + path + // instead, and counts only failed attempts. } // Rate limiting for /api. Registered HERE — above the routers — because @@ -525,6 +521,12 @@ async function initializeRateLimiters() { // counted, which matters because monitors poll them every couple of seconds. app.use(createApiRateLimitGate(() => generalRateLimiter)); +// Per-IP limit for credential-verification endpoints only, on its own bucket. +// Registered after the general gate so that an IP already over the /api budget +// is rejected there first; see authRateLimitGate for the exact endpoint table +// and why it must stay unmounted. +app.use(createAuthRateLimitGate(() => authRateLimiter)); + app.use(express.json({ limit: '50mb' })); app.use(express.urlencoded({ extended: true, limit: '50mb' })); diff --git a/backend/src/middleware/apiRateLimitGate.js b/backend/src/middleware/apiRateLimitGate.js index adab90e0..4e23710b 100644 --- a/backend/src/middleware/apiRateLimitGate.js +++ b/backend/src/middleware/apiRateLimitGate.js @@ -39,10 +39,10 @@ const EXEMPT_PREFIXES = [ // 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. +// means giving them their own bucket, which is what authRateLimitGate now does: +// a separate limiter instance with its own store, matching exact method + path +// and counting only failed attempts. They stay exempt HERE so the two budgets +// never share a counter — that sharing is the whole failure mode above. // // 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. diff --git a/backend/src/middleware/authRateLimitGate.js b/backend/src/middleware/authRateLimitGate.js new file mode 100644 index 00000000..f79a2d95 --- /dev/null +++ b/backend/src/middleware/authRateLimitGate.js @@ -0,0 +1,92 @@ +/** + * Per-IP rate limit for the credential-verification endpoints. + * + * Same boot-order problem as apiRateLimitGate, same fix: the auth limiter is + * built asynchronously from app_settings, so it was registered from inside + * initializeRateLimiters() — which runs after every router, the /api 404 + * handler and the error handler are already on the stack. Express dispatches + * in registration order, so those five app.use() calls landed below everything + * that answers a request and never executed. Auth endpoints have therefore + * never had an IP-based limit; brute force was bounded only by the per-account + * lockout in utils/authSecurity.js, which is per-identifier, not per-IP, and so + * does not bound spraying one password across many usernames or many galleries. + * + * This gate is a stable function registered at the right depth immediately and + * resolving the limiter per request, so the limit no longer depends on boot + * timing. It stays a pass-through until initializeRateLimiters() resolves. + * + * Two things it deliberately does NOT do: + * + * 1. It does not match on a prefix. The old registrations used + * app.use('/api/auth', authRateLimiter), and /api/auth is the mount point of + * the whole auth router — so the 5-per-window budget would also have covered + * GET /api/auth/session and POST /api/auth/password-strength, both of which + * the frontend calls far more than five times per window. Activating them as + * written would have locked legitimate users out. The table below is exact + * method + exact path. + * + * 2. It does not share the general limiter's bucket. Each express-rate-limit + * instance owns a MemoryStore, so authRateLimiter counts into its own per-IP + * bucket and the ordinary /api traffic a login page makes (branding, + * settings) cannot exhaust the login budget. + * + * The limiter is configured with skipSuccessfulRequests, so only *failed* + * attempts consume the budget. That is what makes a 5-per-window budget safe + * behind NAT: ten guests on one venue wifi who all type the right gallery + * password consume nothing. + * + * Register with app.use(gate) and NOT app.use('/api', gate) — Express strips + * the mount path from req.url, and these patterns are written against the full + * path. + */ + +// Exact method + path. Anchored, and case-insensitive because Express's +// "case sensitive routing" setting is off by default: POST /api/auth/admin/LOGIN +// reaches the login handler, so a case-sensitive pattern would be a free bypass. +// The optional trailing slash is there for the same reason. +const CREDENTIAL_ENDPOINTS = [ + // Admin password, and the second factor that completes the same login. + { method: 'POST', path: /^\/api\/auth\/admin\/login\/?$/i }, + { method: 'POST', path: /^\/api\/auth\/admin\/login\/mfa\/?$/i }, + // Gallery password, client PIN, share-link token. + { method: 'POST', path: /^\/api\/auth\/gallery\/verify\/?$/i }, + { method: 'POST', path: /^\/api\/auth\/gallery\/share-login\/?$/i }, + { method: 'POST', path: /^\/api\/auth\/gallery\/[^/]+\/client-login\/?$/i }, + // First-run bootstrap: both of these take the setup token. + { method: 'POST', path: /^\/api\/setup\/verify-token\/?$/i }, + { method: 'POST', path: /^\/api\/setup\/admin\/?$/i }, + // Customer portal password, and the reset that replaces it. + { method: 'POST', path: /^\/api\/customer\/auth\/login\/?$/i }, + { method: 'POST', path: /^\/api\/customer\/auth\/password-reset\/?$/i }, +]; + +/** + * @param {import('express').Request} req + * @returns {boolean} true when the request is an attempt to prove a secret. + */ +function isCredentialEndpoint(req) { + return CREDENTIAL_ENDPOINTS.some( + (endpoint) => endpoint.method === req.method && endpoint.path.test(req.path) + ); +} + +/** + * @param {() => import('express').RequestHandler|undefined} getLimiter + * Reads the current auth rate limiter. Returns undefined until + * initializeRateLimiters() has resolved. + * @returns {import('express').RequestHandler} + */ +function createAuthRateLimitGate(getLimiter) { + return function authRateLimitGate(req, res, next) { + if (!isCredentialEndpoint(req)) 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 = { createAuthRateLimitGate, isCredentialEndpoint, CREDENTIAL_ENDPOINTS }; diff --git a/backend/src/services/rateLimitService.js b/backend/src/services/rateLimitService.js index 4b6e6808..701fb328 100644 --- a/backend/src/services/rateLimitService.js +++ b/backend/src/services/rateLimitService.js @@ -207,13 +207,29 @@ async function createRateLimiter() { /** * Create auth-specific rate limiter + * + * This is a separate rateLimit() instance from createRateLimiter(), so it owns + * its own MemoryStore and therefore its own per-IP bucket. That separation is + * the point: the credential endpoints get a small budget of their own that the + * ordinary /api traffic a login page makes cannot exhaust. + * + * skipSuccessfulRequests means only failed attempts are counted, which is what + * makes a 5-per-window per-IP budget safe behind NAT — a room full of guests on + * one venue IP who all type the correct gallery password consume nothing. */ async function createAuthRateLimiter() { const config = await getRateLimitSettings(); - + return rateLimit({ windowMs: config.windowMinutes * 60 * 1000, - max: config.authMaxRequests, + // Read per request, like the general limiter, so a change to + // rate_limit_auth_max_requests in admin Settings takes effect within the + // settings cache TTL instead of needing a restart. + max: async () => { + const currentConfig = await getRateLimitSettings(); + return currentConfig.authMaxRequests; + }, + skipSuccessfulRequests: true, keyGenerator: (req) => req.ip, skip: async () => { const currentConfig = await getRateLimitSettings();