safeValidationErrors moves to utils/routeHelpers and replaces every
res.status(400).json({ errors: errors.array() }) in the routes, so no 400
body carries the submitted value any more (setup, customer auth and
customer change-password were still echoing rejected passwords).
Admin login, gallery verify, customer login/register/reset, customer
change-password and setup now cap username/slug at 255 and passwords at
MAX_PASSWORD_LENGTH at the validator, so an oversized value never reaches
the lockout lookup, bcrypt or the failed-attempt log.
Admin and customer login run one bcrypt compare on every path; the unknown
account branch used to return in microseconds against ~100ms for a wrong
password, which enumerated usernames despite the generic message.
(cherry picked from commit 40a8a9882a)
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.
(cherry picked from commit 903e471753)
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.
(cherry picked from commit 054cd6f82f)
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/<id>),
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.
(cherry picked from commit 14cd5eacb3)
Paul Nothaft