From 14cd5eacb32ea5526e5226aa998b8a9478768fea Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Thu, 3 Sep 2026 09:14:48 +0200 Subject: [PATCH 01/13] fix(security): bound password input before zxcvbn, and drop the legacy media mounts Two findings from the GHSA-pwx6-5pqc-c5xq scan bundle, both verified against the code and reproduced before fixing. **Unauthenticated denial of service via password strength (csf_495d53fa).** POST /api/auth/password-strength takes `body('password').notEmpty()` with no upper bound, sits behind express.json({ limit: '50mb' }), and hands the string to zxcvbn, whose matching is superlinear and runs synchronously on the event loop. Measured on this codebase, in ms of blocked loop: 128 -> 41, 512 -> 1367, 1000 -> 5097, 5000 -> did not return in two minutes. One unauthenticated request of about a kilobyte stops the whole process for five seconds; a few kilobytes stops it indefinitely. The /api/auth rate limit does not help when a single request is already enough. The cap lives in validatePassword() so it covers every caller, present and future; the route validator is defence in depth. 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 length past that adds no entropy to the stored hash anyway. This is the only unauthenticated reach into zxcvbn: setup is token-gated and self-closing, and acceptInvite/adminAuth use the regex-only validator in passwordGenerator. **The /photos and /thumbnails static mounts (csf_9aa6afe6, csf_559cd5cc, csf_b14d462e, csf_547d26fa, and the gallery half of csf_34e420af).** They served the raw originals and thumbnail trees behind photoAuth, which authorises on a slug match. A static file server cannot apply per-photo rules, so everything the gallery API decides was absent: allow_downloads, per-category allow_downloads, watermarking, the resolution cap, reveal-mode windows, visibility='hidden', download logging, and the customer-assignment re-check that makes revocation immediate. photoAuth also bcrypt-compares an x-gallery-password header per request with no limiter -- both rate-limit gates return early for non-/api paths -- so the mount was an unmetered password oracle. The filenames needed to drive all of this 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. The equivalent authorised routes are /api/gallery/:slug/photo/:id and /thumbnail/:id. nginx still proxies the two locations; they now 404, which is the intent. **The /uploads mount (csf_1fc92f57).** It exposed 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/), reachable by anyone who learned or guessed a filename. Narrowed to the two public asset trees it exists for; contracts and transfers keep their own authorised routes. Removing the mounts leaves src/middleware/photoAuth.js unreferenced by application code. Left in place deliberately -- deleting it and its tests is a separate cleanup, and a smaller diff backports more safely. Backend suite: 2742 passed. --- .../routes/passwordStrengthDos.test.js | 49 +++++++++++++++++++ backend/server.js | 35 ++++++++++--- backend/src/routes/auth.js | 11 ++++- backend/src/utils/passwordValidation.js | 28 +++++++++++ 4 files changed, 115 insertions(+), 8 deletions(-) create mode 100644 backend/__tests__/routes/passwordStrengthDos.test.js 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, From 054cd6f82f08a1997ef68da56e07e04369a68f1b Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Thu, 3 Sep 2026 09:39:50 +0200 Subject: [PATCH 02/13] fix(security): enforce the strength-endpoint validators, and stop the generator spinning Codex review round 1 on the batch-1 security fixes. One finding is a regression this branch introduced. generateSecurePassword retried by recursing on any candidate validatePassword rejected. The new 128-character cap makes EVERY candidate invalid once a caller asks for more than that, so `generateSecurePassword({ length: 129 })` went from returning a password to unbounded recursion and a stack overflow. It now refuses an impossible length up front, and the retry is a bounded loop rather than recursion -- every candidate failing is possible for reasons other than bad luck (a charset that cannot satisfy the configured policy), and that case deserves an error someone can act on rather than a blown stack. No caller in the repo passes a length at all; the hazard was in the exported surface. The route validators were decorative. POST /api/auth/password-strength never called validationResult(), so the length bound I added only recorded an error that nothing read: the oversized body still reached zxcvbn and the endpoint still answered 200. The cap inside validatePassword() was doing all the work. Errors are now returned as a 400 before the validator runs, which is what the previous commit claimed. Also awaited validatePasswordInContext, which is async. Unawaited, `validation` was a Promise and every field in the response -- valid, score, errors, feedback -- came back undefined. Pre-existing, in the lines this change already touches, and it made the endpoint useless for the real-time validation it exists for. 1 more test. Backend suite: 2742 passed. The 23 eslint errors in server.js are pre-existing and identical on main. --- .../routes/passwordStrengthDos.test.js | 14 ++++++ backend/src/routes/auth.js | 12 +++++- backend/src/utils/passwordValidation.js | 43 ++++++++++++------- 3 files changed, 52 insertions(+), 17 deletions(-) diff --git a/backend/__tests__/routes/passwordStrengthDos.test.js b/backend/__tests__/routes/passwordStrengthDos.test.js index 3759a0f0..4ff60a42 100644 --- a/backend/__tests__/routes/passwordStrengthDos.test.js +++ b/backend/__tests__/routes/passwordStrengthDos.test.js @@ -40,6 +40,20 @@ describe('password validation length cap (zxcvbn DoS)', () => { expect(result.valid).toBe(true); }); + it('does not spin when a caller asks for a length the cap forbids', async () => { + // Codex review. generateSecurePassword retried by recursing on any invalid + // candidate, so the new cap made every candidate invalid for length > 128 + // and turned the call into unbounded recursion. It now refuses up front, + // and the retry loop is bounded. + const { generateSecurePassword } = require('../../src/utils/passwordValidation'); + + expect(generateSecurePassword({ length: 16 })).toHaveLength(16); + expect(generateSecurePassword({ length: MAX_PASSWORD_LENGTH })) + .toHaveLength(MAX_PASSWORD_LENGTH); + expect(() => generateSecurePassword({ length: MAX_PASSWORD_LENGTH + 1 })) + .toThrow(/at most 128/); + }); + it('applies the cap through the context wrapper too', async () => { const { validatePasswordInContext } = require('../../src/utils/passwordValidation'); const huge = 'aA1!'.repeat(MAX_PASSWORD_LENGTH); diff --git a/backend/src/routes/auth.js b/backend/src/routes/auth.js index 3d341edb..98c4f3af 100644 --- a/backend/src/routes/auth.js +++ b/backend/src/routes/auth.js @@ -966,6 +966,14 @@ router.post('/password-strength', [ body('context').isIn(['admin', 'gallery']).optional() ], async (req, res) => { try { + // The validators above only RECORD errors; without this the oversized body + // reached zxcvbn anyway and the endpoint answered 200, so the edge cap was + // decorative. The cap in validatePassword() is still the real control. + const errors = validationResult(req); + if (!errors.isEmpty()) { + return res.status(400).json({ errors: errors.array() }); + } + const { password, context = 'gallery' } = req.body; // Get user data if available (for context-aware validation) @@ -975,7 +983,9 @@ router.post('/password-strength', [ userData.email = req.admin.email; } - const validation = validatePasswordInContext(password, context, userData); + // validatePasswordInContext is async; unawaited this resolved to a Promise + // and every field below came back undefined. + const validation = await validatePasswordInContext(password, context, userData); res.json({ valid: validation.valid, diff --git a/backend/src/utils/passwordValidation.js b/backend/src/utils/passwordValidation.js index 666e8943..b7a1cbc2 100644 --- a/backend/src/utils/passwordValidation.js +++ b/backend/src/utils/passwordValidation.js @@ -349,24 +349,35 @@ function generateSecurePassword(options = {}) { if (charset.length === 0) { throw new Error('At least one character type must be included'); } - - // Generate password + + // A requested length the validator will always reject makes the retry below + // unwinnable, so say so instead of spinning. MAX_PASSWORD_LENGTH is the cap + // validatePassword() applies; anything above it fails every candidate. + if (config.length > MAX_PASSWORD_LENGTH) { + throw new Error(`length must be at most ${MAX_PASSWORD_LENGTH}`); + } + const crypto = require('crypto'); - let password = ''; - - for (let i = 0; i < config.length; i++) { - const randomIndex = crypto.randomInt(charset.length); - password += charset[randomIndex]; + + // Bounded retry rather than unbounded recursion. Every candidate failing is + // possible for reasons other than bad luck -- a charset that cannot satisfy + // the configured policy (numbers excluded while requireNumbers is on, say) + // -- and the previous `return generateSecurePassword(options)` turned that + // into a stack overflow rather than an error anyone could act on. + const MAX_ATTEMPTS = 100; + for (let attempt = 0; attempt < MAX_ATTEMPTS; attempt += 1) { + let password = ''; + for (let i = 0; i < config.length; i++) { + const randomIndex = crypto.randomInt(charset.length); + password += charset[randomIndex]; + } + if (validatePassword(password).valid) return password; } - - // Ensure password meets requirements - const validation = validatePassword(password); - if (!validation.valid) { - // Recursively generate until we get a valid password - return generateSecurePassword(options); - } - - return password; + + throw new Error( + 'Could not generate a password satisfying the configured policy — ' + + 'check that the selected character types can meet it', + ); } /** From 903e4717530e09e1421bb1a43e2e2d53e248ffae Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Thu, 3 Sep 2026 09:58:58 +0200 Subject: [PATCH 03/13] fix(security): stop reflecting submitted passwords in validation errors Codex review round 2. The 400 I added in the previous commit returned errors.array() verbatim, and express-validator puts the submitted `value` in each error -- so rejecting an oversized password echoed that password back, and re-allocated up to the 50mb body limit on an unauthenticated endpoint, partly undoing the denial-of-service fix this branch exists for. The same call appeared at seven sites in this file, five of which validate a password field: /admin/login, /gallery/verify, /gallery/:slug/client-login, /admin/change-password and /password-strength. Every failed login was returning the attempted password in its response body, where it reaches proxy logs, error monitoring and browser tooling. Fixed at all seven rather than only the one the review pointed at. Only `value` is dropped. `msg`, `path` and the rest are kept, because the two shapes express-validator produces are both consumed in the frontend -- AcceptInvite reads {field, message} from routeHelpers.validateRequest, EventDetails reads {msg, path} from raw errors.array() -- and switching auth.js to the helper's shape would have broken the latter for a reason unrelated to security. 1 more test. Backend suite: 2744 passed. --- .../routes/passwordStrengthDos.test.js | 15 ++++++++++++ backend/src/routes/auth.js | 24 +++++++++++++------ 2 files changed, 32 insertions(+), 7 deletions(-) diff --git a/backend/__tests__/routes/passwordStrengthDos.test.js b/backend/__tests__/routes/passwordStrengthDos.test.js index 4ff60a42..7e60320d 100644 --- a/backend/__tests__/routes/passwordStrengthDos.test.js +++ b/backend/__tests__/routes/passwordStrengthDos.test.js @@ -54,6 +54,21 @@ describe('password validation length cap (zxcvbn DoS)', () => { .toThrow(/at most 128/); }); + it('does not echo the rejected password back in the error body', async () => { + // Codex review round 2. express-validator's errors.array() carries the + // submitted `value`, so the 400 for an oversized password returned the + // password itself -- reflecting a credential, and re-allocating up to the + // 50mb body limit on an unauthenticated endpoint, which partly undid the + // DoS fix this branch exists for. + const src = require('fs').readFileSync( + require('path').join(__dirname, '../../src/routes/auth.js'), 'utf8'); + + // No route may hand errors.array() straight to the response. + expect(src).not.toMatch(/errors:\s*errors\.array\(\)/); + // ...and the helper that replaces it must drop `value`. + expect(src).toMatch(/safeValidationErrors\s*=\s*\(errors\)\s*=>\s*errors\.array\(\)\.map\(\(\{ value, \.\.\.rest \}\)/); + }); + it('applies the cap through the context wrapper too', async () => { const { validatePasswordInContext } = require('../../src/utils/passwordValidation'); const huge = 'aA1!'.repeat(MAX_PASSWORD_LENGTH); diff --git a/backend/src/routes/auth.js b/backend/src/routes/auth.js index 98c4f3af..f028e862 100644 --- a/backend/src/routes/auth.js +++ b/backend/src/routes/auth.js @@ -2,6 +2,16 @@ const express = require('express'); const bcrypt = require('bcrypt'); const jwt = require('jsonwebtoken'); const { body, validationResult } = require('express-validator'); + +/** + * express-validator's errors.array() carries `value` -- the submitted input -- + * so returning it verbatim reflects the caller's password back in the 400 body. + * Five routes in this file validate a password field, and the strength endpoint + * is unauthenticated behind a 50mb JSON limit, which also made the rejection + * itself an allocation amplifier. Everything except `value` is kept, so the + * response shape both frontend consumers rely on (`msg`, `path`) is unchanged. + */ +const safeValidationErrors = (errors) => errors.array().map(({ value, ...rest }) => rest); const { db, logActivity } = require('../database/db'); const { formatBoolean } = require('../utils/dbCompat'); const { verifyRecaptcha } = require('../services/recaptcha'); @@ -122,7 +132,7 @@ router.post('/admin/login', [ try { const errors = validationResult(req); if (!errors.isEmpty()) { - return res.status(400).json({ errors: errors.array() }); + return res.status(400).json({ errors: safeValidationErrors(errors) }); } const { username, password, recaptchaToken } = req.body; @@ -229,7 +239,7 @@ router.post('/admin/login/mfa', [ try { const errors = validationResult(req); if (!errors.isEmpty()) { - return res.status(400).json({ errors: errors.array() }); + return res.status(400).json({ errors: safeValidationErrors(errors) }); } const { mfaToken, code } = req.body; @@ -398,7 +408,7 @@ router.post('/gallery/verify', [ try { const errors = validationResult(req); if (!errors.isEmpty()) { - return res.status(400).json({ errors: errors.array() }); + return res.status(400).json({ errors: safeValidationErrors(errors) }); } const { slug, password, recaptchaToken } = req.body; @@ -521,7 +531,7 @@ router.post('/gallery/:slug/client-login', [ try { const errors = validationResult(req); if (!errors.isEmpty()) { - return res.status(400).json({ errors: errors.array() }); + return res.status(400).json({ errors: safeValidationErrors(errors) }); } const { slug } = req.params; @@ -597,7 +607,7 @@ router.post('/gallery/share-login', [ try { const errors = validationResult(req); if (!errors.isEmpty()) { - return res.status(400).json({ errors: errors.array() }); + return res.status(400).json({ errors: safeValidationErrors(errors) }); } const { slug, token } = req.body; @@ -882,7 +892,7 @@ router.post('/admin/change-password', [ try { const errors = validationResult(req); if (!errors.isEmpty()) { - return res.status(400).json({ errors: errors.array() }); + return res.status(400).json({ errors: safeValidationErrors(errors) }); } const { currentPassword, newPassword } = req.body; @@ -971,7 +981,7 @@ router.post('/password-strength', [ // decorative. The cap in validatePassword() is still the real control. const errors = validationResult(req); if (!errors.isEmpty()) { - return res.status(400).json({ errors: errors.array() }); + return res.status(400).json({ errors: safeValidationErrors(errors) }); } const { password, context = 'gallery' } = req.body; From 0ca0e4a9226f9a55a165ebc236c8d6d319de585f Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Thu, 3 Sep 2026 10:44:54 +0200 Subject: [PATCH 04/13] fix(security): verify the signature before writing a token to the revocation list revokeToken() base64-decoded the payload without checking the signature and inserted a row keyed on id-iat-type, the same key isTokenRevoked() matches for real sessions. The logout endpoints are unauthenticated, so anyone could forge a payload naming another user's id, type and login second and log them out remotely; a far-future exp also left rows that cleanup never swept. Expiry is still ignored so logging out an expired session stays idempotent. --- .../utils/tokenRevocation.forgery.test.js | 69 +++++++++++++++++++ backend/src/utils/tokenRevocation.js | 23 +++++-- 2 files changed, 85 insertions(+), 7 deletions(-) create mode 100644 backend/__tests__/utils/tokenRevocation.forgery.test.js diff --git a/backend/__tests__/utils/tokenRevocation.forgery.test.js b/backend/__tests__/utils/tokenRevocation.forgery.test.js new file mode 100644 index 00000000..a5b54584 --- /dev/null +++ b/backend/__tests__/utils/tokenRevocation.forgery.test.js @@ -0,0 +1,69 @@ +/** + * revokeToken() is reachable from the unauthenticated logout endpoints + * (POST /api/auth/logout, /gallery/logout, /customer-auth/logout). It used + * to base64-decode the payload without checking the signature and insert a + * row keyed on `${id}-${iat}-${type}` -- the same key isTokenRevoked() + * matches for real sessions. Anyone could therefore forge a payload naming + * another user's id, type and login second and log them out remotely, and + * with a far-future `exp` the row was never swept. + * + * The contract pinned here: only a token whose signature verifies under + * JWT_SECRET is written to revoked_tokens. Expired-but-genuine tokens are + * still accepted (logout must stay idempotent). + */ +const jwt = require('jsonwebtoken'); + +process.env.JWT_SECRET = 'revocation-forgery-test-secret'; + +const inserted = []; +jest.mock('../../src/database/db', () => { + const dbFn = () => ({ + insert(row) { + inserted.push(row); + return { onConflict: () => ({ ignore: async () => undefined }) }; + }, + }); + return { db: dbFn }; +}); +jest.mock('../../src/utils/logger', () => ({ + info: jest.fn(), warn: jest.fn(), error: jest.fn(), debug: jest.fn(), +})); + +const { revokeToken } = require('../../src/utils/tokenRevocation'); + +const iat = Math.floor(Date.now() / 1000) - 60; + +describe('revokeToken signature check', () => { + beforeEach(() => { inserted.length = 0; }); + + it('refuses a forged three-part token and writes nothing', async () => { + const forgedPayload = Buffer.from(JSON.stringify({ + id: 1, iat, type: 'admin', exp: 9e9, + })).toString('base64'); + const forged = `eyJhbGciOiJIUzI1NiJ9.${forgedPayload}.notasignature`; + + const result = await revokeToken(forged, 'user_logout'); + + expect(result).toBe(false); + expect(inserted).toHaveLength(0); + }); + + it('refuses a token signed with a different secret', async () => { + const other = jwt.sign({ id: 1, iat, type: 'admin' }, 'some-other-secret', { expiresIn: '1h' }); + expect(await revokeToken(other, 'user_logout')).toBe(false); + expect(inserted).toHaveLength(0); + }); + + it('revokes a genuine token', async () => { + const genuine = jwt.sign({ id: 1, iat, type: 'admin' }, process.env.JWT_SECRET, { expiresIn: '1h' }); + expect(await revokeToken(genuine, 'user_logout')).toBe(true); + expect(inserted).toHaveLength(1); + expect(inserted[0].token_id).toBe(`1-${iat}-admin`); + }); + + it('still revokes a genuine token that has already expired', async () => { + const expired = jwt.sign({ id: 1, iat, type: 'admin', exp: iat + 1 }, process.env.JWT_SECRET); + expect(await revokeToken(expired, 'user_logout')).toBe(true); + expect(inserted).toHaveLength(1); + }); +}); diff --git a/backend/src/utils/tokenRevocation.js b/backend/src/utils/tokenRevocation.js index b209ece9..222ba0d6 100644 --- a/backend/src/utils/tokenRevocation.js +++ b/backend/src/utils/tokenRevocation.js @@ -3,6 +3,7 @@ * Provides ability to invalidate tokens before expiration */ +const jwt = require('jsonwebtoken'); const { db } = require('../database/db'); const logger = require('./logger'); @@ -28,15 +29,23 @@ function buildTokenId(payload) { async function revokeToken(token, reason, metadata = {}) { try { - // Extract token info without full verification (it might be compromised) - const parts = token.split('.'); - if (parts.length !== 3) { - throw new Error('Invalid token format'); + // The signature MUST be verified before anything is written. The + // revocation key is `${id}-${iat}-${type}` (buildTokenId), and the + // logout endpoints are unauthenticated, so a raw base64 decode let + // anyone forge a three-part string naming another user's id, type and + // login second and insert a row that isTokenRevoked() then matched for + // that user's real session -- a remote forced logout of any admin, + // customer or gallery session, plus never-swept rows when `exp` was set + // far in the future. Expiry is ignored on purpose: revoking an already + // expired token is harmless and keeps logout idempotent. + const payload = jwt.verify(token, process.env.JWT_SECRET, { + algorithms: ['HS256'], + ignoreExpiration: true, + }); + if (!payload || typeof payload !== 'object') { + throw new Error('Invalid token payload'); } - // Decode payload - const payload = JSON.parse(Buffer.from(parts[1], 'base64').toString()); - // user_id is integer-typed in revoked_tokens; for non-admin tokens // we may not have an integer (customer) or any id at all (gallery // tokens use eventId). Coerce to null instead of letting an From 3e465300721895e34ebd676b4d65d58f7dced257 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Thu, 3 Sep 2026 10:44:55 +0200 Subject: [PATCH 05/13] fix(security): contain logo, favicon and PDF-logo unlinks to their upload directories Settings > Branding persisted logo_url / favicon_url verbatim and on clear unlinked path.join(storage, url) behind a startsWith('/uploads/logos/') check, which '..' segments pass. The business-profile PDF logo did the same behind a /pdf-logo-\d+\./ marker test, and used absolute values as given. Either let a settings.edit or settings.banking holder delete any file the process can reach. Both now resolve through helpers in utils/safePath that only ever name a flat leaf inside the fixed directory. The /favicon.ico streamer is narrowed the same way: it contained to the whole uploads/ root, which also holds signed contracts and transfer files. --- .../utils/safePathUploadedAssets.test.js | 59 +++++++++++++++++++ backend/server.js | 9 ++- backend/src/routes/adminBusinessProfile.js | 17 ++---- backend/src/routes/adminSettings.js | 19 +++--- backend/src/utils/safePath.js | 42 +++++++++++++ 5 files changed, 124 insertions(+), 22 deletions(-) create mode 100644 backend/__tests__/utils/safePathUploadedAssets.test.js diff --git a/backend/__tests__/utils/safePathUploadedAssets.test.js b/backend/__tests__/utils/safePathUploadedAssets.test.js new file mode 100644 index 00000000..07ae5d6a --- /dev/null +++ b/backend/__tests__/utils/safePathUploadedAssets.test.js @@ -0,0 +1,59 @@ +/** + * Containment for the two admin-writable "delete the old file" paths. + * + * Settings → Branding persists logo_url / favicon_url verbatim and, on + * clear, unlinked `path.join(storage, url)` after a mere prefix check. + * Business profile did the same for logo_path behind a `/pdf-logo-\d+\./` + * marker. Both let an admin delete any file the process can reach. The + * helpers below only ever name a flat leaf inside the fixed directory. + */ +const path = require('path'); +const { uploadedAssetPath, uploadedPdfLogoPath } = require('../../src/utils/safePath'); + +const root = '/srv/picpeak/storage'; + +describe('uploadedAssetPath', () => { + it('resolves a flat leaf inside the named upload directory', () => { + expect(uploadedAssetPath('/uploads/logos/logo-1.png', 'logos', root)) + .toBe(path.join(root, 'uploads', 'logos', 'logo-1.png')); + expect(uploadedAssetPath('/uploads/favicons/fav.ico', 'favicons', root)) + .toBe(path.join(root, 'uploads', 'favicons', 'fav.ico')); + }); + + it.each([ + '/uploads/logos/../../../data/picpeak.db', + '/uploads/logos/..', + '/uploads/logos/', + '/uploads/logos/sub/dir.png', + '/uploads/favicons/x.ico', // wrong kind + 'uploads/logos/logo.png', // not /-rooted + 'https://example.com/uploads/logos/logo.png', + '', + null, + 42, + ])('refuses %p', (value) => { + expect(uploadedAssetPath(value, 'logos', root)).toBeNull(); + }); +}); + +describe('uploadedPdfLogoPath', () => { + it('resolves the file the upload route writes', () => { + expect(uploadedPdfLogoPath('/uploads/logos/pdf-logo-1700000000000.png', root)) + .toBe(path.join(root, 'uploads', 'logos', 'pdf-logo-1700000000000.png')); + expect(uploadedPdfLogoPath('uploads/logos/pdf-logo-1.svg', root)) + .toBe(path.join(root, 'uploads', 'logos', 'pdf-logo-1.svg')); + }); + + it.each([ + 'pdf-logo-1./../../../../etc/target', + '/uploads/logos/pdf-logo-1./../../secret', + '/etc/pdf-logo-1.x', + '/uploads/logos/pdf-logo-1.png/../other', + '/uploads/logos/other-logo.png', + '/uploads/contracts/signed/pdf-logo-1.pdf', + '', + null, + ])('refuses %p', (value) => { + expect(uploadedPdfLogoPath(value, root)).toBeNull(); + }); +}); diff --git a/backend/server.js b/backend/server.js index 84e45283..efeb35f2 100644 --- a/backend/server.js +++ b/backend/server.js @@ -791,10 +791,15 @@ app.get( // whereas Firefox/Chrome do — so a 302 worked everywhere except // Safari. sendFile sets the right content-type from the extension. const rel = String(url).replace(/^\/+/, '').replace(/^uploads\//, ''); + // Containment is the two public asset trees, not the whole uploads/ + // root: that root also holds signed contracts and client transfer + // files, and the favicon URL is an admin-writable setting, so the + // wider check let `/uploads/contracts/signed/` be served here + // unauthenticated with a day of cache. const uploadsRoot = path.resolve(path.join(storagePath, 'uploads')); const resolved = path.resolve(path.join(uploadsRoot, rel)); - // Path containment — never serve outside the uploads dir. - if (resolved.startsWith(uploadsRoot + path.sep) && fs.existsSync(resolved)) { + const servableRoots = ['favicons', 'logos'].map((d) => path.join(uploadsRoot, d) + path.sep); + if (servableRoots.some((root) => resolved.startsWith(root)) && fs.existsSync(resolved)) { // This route streams the file directly, bypassing the secureStatic // middleware — so re-apply its SVG hardening here. An admin-uploaded // SVG favicon could contain