diff --git a/backend/__tests__/routes/passwordStrengthDos.test.js b/backend/__tests__/routes/passwordStrengthDos.test.js new file mode 100644 index 00000000..3759a0f0 --- /dev/null +++ b/backend/__tests__/routes/passwordStrengthDos.test.js @@ -0,0 +1,49 @@ +/** + * POST /api/auth/password-strength is unauthenticated and feeds its body into + * zxcvbn, whose matching is superlinear and runs synchronously on the event + * loop. Behind express.json({ limit: '50mb' }) that made a single request a + * whole-process denial of service: measured on this codebase, 1,000 characters + * blocked for ~5 seconds and 5,000 did not return in two minutes. + * + * The control is the length cap inside validatePassword(), so it holds for + * every caller. These tests pin the cap itself rather than the route, and use + * a wall-clock ceiling that only an unbounded zxcvbn call can breach. + */ +const { validatePassword, MAX_PASSWORD_LENGTH } = require('../../src/utils/passwordValidation'); + +describe('password validation length cap (zxcvbn DoS)', () => { + it('rejects an over-length password without doing superlinear work', () => { + const huge = 'aA1!'.repeat(MAX_PASSWORD_LENGTH); // 4x the cap + const started = Date.now(); + const result = validatePassword(huge); + const elapsed = Date.now() - started; + + expect(result.valid).toBe(false); + expect(result.errors.join(' ')).toMatch(/at most 128 characters/); + // Unbounded, this input would not return for minutes. + expect(elapsed).toBeLessThan(250); + }); + + it('is bounded at the cap itself, the worst input it will still analyse', () => { + const atCap = 'aA1!'.repeat(MAX_PASSWORD_LENGTH / 4); + expect(atCap).toHaveLength(MAX_PASSWORD_LENGTH); + + // 128 was chosen so the worst input the validator will still analyse costs + // about as much as an ordinary request (~41ms measured); 512 cost 1.4s. + const started = Date.now(); + validatePassword(atCap); + expect(Date.now() - started).toBeLessThan(1000); + }); + + it('still accepts an ordinary strong password', () => { + const result = validatePassword('Tr0ub4dour&3-horse-battery'); + expect(result.valid).toBe(true); + }); + + it('applies the cap through the context wrapper too', async () => { + const { validatePasswordInContext } = require('../../src/utils/passwordValidation'); + const huge = 'aA1!'.repeat(MAX_PASSWORD_LENGTH); + const result = await validatePasswordInContext(huge, 'admin', {}); + expect(result.valid).toBe(false); + }); +}); diff --git a/backend/server.js b/backend/server.js index e78da6f8..84e45283 100644 --- a/backend/server.js +++ b/backend/server.js @@ -596,14 +596,35 @@ const secureStatic = require('./src/middleware/secureStatic'); const storagePath = process.env.STORAGE_PATH || path.join(__dirname, '../storage'); process.env.EXTERNAL_MEDIA_ROOT = process.env.EXTERNAL_MEDIA_ROOT || '/external-media'; -// Static file serving for photos (protected) -app.use('/photos', require('./src/middleware/photoAuth'), setCorsHeaders, secureStatic(path.join(storagePath, 'events/active'))); +// The /photos and /thumbnails static mounts are gone. +// +// They served the raw originals tree and the thumbnail tree behind photoAuth +// alone, which authorises on a slug match. A static file server cannot apply +// the rules the gallery API applies per photo, so everything the API decides +// was simply absent here: allow_downloads, per-category allow_downloads, +// watermarking, the resolution cap, reveal-mode windows, visibility='hidden', +// download logging, and the customer-assignment re-check that lets an admin +// revoke access immediately. The filenames needed to exercise it are handed to +// every guest in the photos listing. +// +// Nothing builds these URLs: no reference in frontend/src, none in the email +// templates, and the only backend mentions are the /api/admin/photos/... API +// routes and a maintenance-mode prefix list. nginx still proxies /photos and +// /thumbnails; those locations now 404, which is the intended outcome. +// +// Serving these safely would mean reimplementing per-photo authorisation and +// image processing inside a static handler -- i.e. the gallery API, which +// already exists at /api/gallery/:slug/photo/:id and /thumbnail/:id. -// Static file serving for thumbnails (protected) -app.use('/thumbnails', require('./src/middleware/photoAuth'), setCorsHeaders, secureStatic(path.join(storagePath, 'thumbnails'))); - -// Static file serving for uploads (public - logos, favicons) -app.use('/uploads', setCorsHeaders, secureStatic(path.join(storagePath, 'uploads'))); +// Static file serving for uploads. +// +// Narrowed to the two public asset trees. The mount used to expose the whole +// uploads/ root with no auth middleware at all, and that root also holds +// signed contract PDFs (uploads/contracts/signed) and client transfer files +// (uploads/transfers/) -- both reachable by anyone who learned or guessed +// a filename. Those are served by their own authorised routes. +app.use('/uploads/logos', setCorsHeaders, secureStatic(path.join(storagePath, 'uploads/logos'))); +app.use('/uploads/favicons', setCorsHeaders, secureStatic(path.join(storagePath, 'uploads/favicons'))); // Static file serving for self-hosted webfonts (public — gallery visitors // load these via @font-face). Replaces the previous Google Fonts CDN diff --git a/backend/src/routes/auth.js b/backend/src/routes/auth.js index b4d6ae78..3d341edb 100644 --- a/backend/src/routes/auth.js +++ b/backend/src/routes/auth.js @@ -33,6 +33,7 @@ const { getClientIp } = require('../utils/requestIp'); const { sanitizePasswordInput } = require('../utils/passwordInput'); const { validatePasswordInContext, + MAX_PASSWORD_LENGTH, getBcryptRounds, logPasswordValidationFailure } = require('../utils/passwordValidation'); @@ -952,8 +953,16 @@ router.post('/admin/change-password', [ }); // Password strength check endpoint (for real-time validation) +// +// Unauthenticated, and it feeds the request body straight into zxcvbn, whose +// matching is superlinear and synchronous. Without the length bound a single +// request stops the event loop for the whole process -- ~5s at 1,000 +// characters and unbounded past that. validatePassword() enforces the same cap +// for every caller; this one keeps the oversized body from being accepted at +// the edge at all. router.post('/password-strength', [ - body('password').notEmpty(), + body('password').isString().isLength({ min: 1, max: MAX_PASSWORD_LENGTH }) + .withMessage(`Password must be 1-${MAX_PASSWORD_LENGTH} characters`), body('context').isIn(['admin', 'gallery']).optional() ], async (req, res) => { try { diff --git a/backend/src/utils/passwordValidation.js b/backend/src/utils/passwordValidation.js index 09dba192..666e8943 100644 --- a/backend/src/utils/passwordValidation.js +++ b/backend/src/utils/passwordValidation.js @@ -7,6 +7,21 @@ const zxcvbn = require('zxcvbn'); const logger = require('./logger'); // Configuration +// zxcvbn's matching is superlinear in the input length and runs synchronously +// on the event loop, so an unbounded password is a denial-of-service primitive +// rather than a slow request. The reachable caller is +// POST /api/auth/password-strength, which is unauthenticated and sits behind +// express.json({ limit: '50mb' }) -- one request stops the whole process. +// +// Measured on this codebase (ms of blocked event loop per call): +// 64 -> 12 128 -> 41 192 -> 105 256 -> 218 +// 384 -> 632 512 -> 1367 1000 -> 5097 5000 -> did not return in 2 min +// +// 128 keeps the worst case at the cost of an ordinary request while staying +// far above any real password: bcrypt consumes only the first 72 bytes, so +// anything longer already adds no entropy to the stored hash. +const MAX_PASSWORD_LENGTH = 128; + const PASSWORD_CONFIG = { minLength: 8, // Reduced from 12 to 8 for better usability requireUppercase: true, @@ -34,6 +49,18 @@ const COMMON_PASSWORDS = [ function validatePassword(password, options = {}) { const config = { ...PASSWORD_CONFIG, ...options }; const errors = []; + + // Bail before any superlinear work touches the string. This is the guard for + // every caller, including ones added later -- the per-route length validator + // is defence in depth, not the control. + if (typeof password === 'string' && password.length > MAX_PASSWORD_LENGTH) { + return { + valid: false, + errors: [`Password must be at most ${MAX_PASSWORD_LENGTH} characters`], + score: 0, + feedback: {}, + }; + } // Check if password exists if (!password || typeof password !== 'string') { @@ -366,6 +393,7 @@ function logPasswordValidationFailure(context, errors, metadata = {}) { } module.exports = { + MAX_PASSWORD_LENGTH, validatePassword, validatePasswordInContext, generateSecurePassword,