From 903e4717530e09e1421bb1a43e2e2d53e248ffae Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Thu, 3 Sep 2026 09:58:58 +0200 Subject: [PATCH] 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;