From 054cd6f82f08a1997ef68da56e07e04369a68f1b Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Thu, 3 Sep 2026 09:39:50 +0200 Subject: [PATCH] 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', + ); } /**