The last open item from the #1304 review.
/disable overlapping an in-flight /enable was silently lost. While
activation generates its identity and writes its binding file the row
still reads `disabled`, so disable()'s conditional update matched no
rows, and the lease conflict raised by its tick() was swallowed as
expected noise. The admin was told participation was off; the activation
then completed and left it on. An opt-out that does nothing is the one
failure this feature cannot have.
disable() now records cancel_requested first and unconditionally —
before the case-by-case work — and enable() claims its state with a
single conditional UPDATE that tests the flag alongside the status.
Re-reading the flag and then updating would only have moved the window;
making the claim itself carry the condition closes it, so whichever of
the two lands first wins outright and the loser writes nothing.
Nothing is registered when the claim fails, so there is also nothing to
delete remotely — the cancelled activation leaves no identity behind.
The flag is cleared at the start of enable(), so a cancellation from an
earlier participation cannot veto a later deliberate opt-in.
The column is migration 202 rather than an edit to 201. 201 already
shipped on this branch and knex records it as applied, so folding the
column in would have skipped every database that had already run it and
the first /disable would have failed on a missing column. Verified both
ways: a fresh install gets the column from 201+202, and a database
migrated before 202 existed gains it when 202 arrives.
Three tests. With the condition dropped from the claim, the race case
fails and the other two pass.
Refs #1110
Three of four findings from the follow-up review.
A withdrawal is no longer clobbered by the instance-copy check. That
update was unconditional, so an opt-out arriving while the binding
lookup was in flight was replaced by identity_conflict — and tick()
stops there, so the deletion the operator asked for was never sent. It
now carries the same whereNot('deletion_pending') guard the
collector-conflict handler beside it already had.
Scheduled database backups count as a configured backup. The middleware
records /backup/* and /database-backup/* under one capability, but
`configured` read only backup_enabled, so an install whose only backup
is the scheduled database one reported used: true, configured: false —
a contradiction in the dataset this feature exists to produce.
SIGNING_KEY_UNREADABLE gets its own message. Naming the error in the
previous commit was half the job: the settings page still showed the
generic retry/disable advice, and neither action can succeed without the
original encryption material. It now says what happened and what is
actually required, in EN and DE.
NOT fixed, and reported instead: /disable overlapping an in-flight
/enable. While activation is still doing its slow work the row still
reads `disabled`, so disable's conditional update matches nothing and
the lease conflict from its tick() is swallowed — the operator is told
participation is off while activation completes and leaves it on.
Closing it properly needs a persisted cancellation flag that enable
checks before finalising: taking the lease cannot help, since it either
conflicts immediately or would block the request for the 60s lease. That
is a schema and state-machine decision for the author, not something to
restructure underneath them.
Refs #1110
Review follow-ups on #1304.
SIGNING_KEY_UNREADABLE. USAGE_ENCRYPTION_KEY defaults to JWT_SECRET, so
rotating JWT_SECRET — the correct response to a suspected compromise —
makes the stored Ed25519 key undecryptable. That surfaced as a generic
DELIVERY_FAILED which retried forever, and it silently blocks the DELETE
packet too: an operator who withdraws has their local state cleared
while the collector keeps its copy. decrypt() now tags its own failure
and deliver() reports it under its own name, without flagging an
identity conflict — an unreadable key is not evidence of a clone. The
docs already warned that losing the key breaks deletion signing; they
now name the trigger and the error.
The collector default is no longer an inline string in the constructor.
It is a declared DEFAULT_COLLECTOR_URL, since it is a deployment choice:
self-hosters point USAGE_COLLECTOR_URL at their own collector and the UI
already derives every link from whatever is configured. schema.cjs is
deliberately untouched — it is vendored byte-identical with
picpeak-usage, and its $id is a schema identity, not a delivery address.
Links in the consent dialog. It named the collector inside prose but
never linked it, so an operator deciding whether to opt in could not
open the destination or the public schema without retyping a URL. Both
are links now, built from the configured collector.
UI standards. The tab hand-rolled its surfaces as
`<section className="rounded-xl border border-theme …">` and imported
Button from a deep path; every other settings tab uses `<Card
padding="md">` from the components/common barrel. Converted, with the
feedback <form> wrapped rather than replaced so its semantics survive,
and headings given the same colour tokens as ImageSecurityTab. The
barrel pulls ErrorBoundary -> i18n/config, so the tab's test needed the
initReactI18next shim the FaceRecognitionCard test already uses.
Not changed: the delete packet reusing the current sequence. The
collector handles delete before any sequence check — "possession proof
is sufficient for deletion, including when a restored backup has a
stale sequence" (picpeak-usage server/collector.js) — so deletion is
deliberately sequence-exempt and the client is correct as written.
Refs #1110
Part B of #1264. Flag off by default, so an install that never enables it
gains no route, no nav entry and no way to mass-mail.
A campaign is a body plus a recipient rule. Queueing one writes ordinary
email_queue rows (email_type 'newsletter', origin 'campaign', new
campaign_id), so retry, rendered_html, sent_at and error_message all come
from the existing processor rather than a parallel sender. Throttling
staggers scheduled_at; the processor loop is untouched.
Two rules the service enforces: no raw HTML is ever stored (sanitized on
write and again on render, idempotently), and opt-out is checked at queue
time AND again at send time.
Migration 199 adds email_campaigns, email_campaign_recipients,
email_queue.campaign_id, customer_accounts.marketing_opt_out(_at), and
the newsletters.view / newsletters.send permissions.
Three rounds of external review are folded in, including several that
would otherwise have shipped broken:
- Campaign rows never came due on SQLite. queueEmail writes a Date, which
the sqlite3 binding stores as epoch ms; ISO text in the same column
compares as TEXT against an INTEGER, and SQLite orders every INTEGER
below every TEXT. The feature silently sent nothing there.
- The flag had no Settings card and no sidebar entry, so it could not be
enabled through the UI at all.
- Consent is per ADDRESS, not per row: two accounts sharing an inbox meant
unsubscribing stopped one and not the other, at both queue and send time.
- The unsubscribe GET mutated consent, so a mail-security scanner walking
a campaign could have unsubscribed much of the list. GET now confirms,
POST acts.
- The rate ceiling is clamped to the queue's real throughput (10/min), so
the composer's estimate stops being wrong by up to 12x.
Closes#1264
sanitizeCSS "blocked" a remote url() by prefixing it with a
/* BLOCKED URL */ COMMENT and leaving the URL in place. CSS comments are
discarded during tokenization, so the declaration a browser parsed still
carried the live URL — while adminCssTemplates returned
sanitization_warnings claiming it had been stopped. Protection that
reports success is worse than none, which is why it survived review.
Scope is narrow: sanitizeCss (lowercase, the public-site path) never
included the pattern and permits remote URLs by design — a test now pins
that. Only sanitizeCSS (uppercase) was affected; outside this repo's
newsletter branch its sole caller is adminCssTemplates.js.
Migration 200 is required, not cosmetic: gallery.js serves
css_templates.css_content VERBATIM as text/css and does not re-sanitize on
read, so fixing the write path alone would leave every existing template
serving its URL forever.
Review follow-ups replaced the regex with a small three-state lexer
(comment / string / identifier) over the RAW text, after five further
bypasses: a ")" inside a quoted url(), CSS escapes (u\72l), the HTML
comment strip JOINING tokens into a live url() after the scan, an escaped
quote desynchronising the scan, and a quote inside a comment. Escapes are
decoded only to decide, never to rewrite — a clean input now round-trips
byte-identical, which also keeps unaffected rows out of the migration's
write path.
Severity is low (writing a template needs branding.edit) but the harm is
a gallery visitor's IP reaching a third party from a page the operator
believes carries no remote requests.
show_feedback_to_guests means "don't show guests OTHER PEOPLE's feedback".
The per-viewer is_liked flag was gated on it anyway, so turning sharing
off emptied every heart the guest had set themselves, on every page load,
while the photo_feedback rows sat there intact.
The query behind the flag is filtered to the viewer (by guest_id, or by
their own IP+UA identifier), so what it returns was never aggregate data.
The colour-label block twelve lines below already documents this exact
reasoning and is correctly ungated.
Scope is just that flag — the counts beside it stay gated, with a test
pinning that the fix does not leak them back. The #1150 contract still
holds: an admin-hidden like does not read as liked.
Note for the reporter: the FILTER path was already correct
(includeGuestMatches is ungated, /my-feedback carries no gate). The empty
Likes chip was downstream of the same falsified flag, not a second bug.
Closes#1286
The business profile already carried the operator's full issuer block —
address, phone, email, website, VAT id — but none of it reached an email.
Those columns only fed the quote/invoice PDF renderer, so every outgoing
mail footer was the fixed logo + company name + copyright line.
The signature is rendered by wrapEmailHtml and nowhere else, so no
template, no per-type send path and no queue row needed a change. Two new
columns on business_profile (migration 198) carry the toggle and one
free-text legal line; everything else is read from the address fields the
operator already maintains.
Default off, with a test pinning that the disabled path is byte-identical
to a no-profile install.
Includes three rounds of external review fixes: the plain-text MIME part
also carries the signature; string booleans ('false'/'0') no longer
invert the toggle; the status line stays silent rather than asserting
"off" while unauthorised or loading; and the preview's Text tab mirrors
the send path's htmlToText fallback.
Manual Messages replies deliberately keep no signature — they bypass the
wrapper by design — and the UI copy names that exception.
Closes#1264 (Part A)
backend: qs and body-parser (array-limit bypass, isBuffer DoS).
frontend: axios 1.17 line (formToJSON recursion DoS, prototype-pollution
gadgets, maxBodyLength bypasses), dompurify, linkify-it and the transitive
set npm audit fix resolves without a major bump.
Left out on purpose: sanitize-html 2.17.7 (its htmlparser2 12 tree is
ESM-only, which Jest 29 cannot load, and the SVG SMIL advisory needs svg
tags none of our sanitizer configs allow), and the tiptap 2->3 and
react-router 6->7 majors (open redirect via <Link> needs a user-controlled
navigation target, which the SPA has none of).
Keeps the size error first, as before the allow-list landed, and pins the
allow-list gate in the size-limit suite: a .html filename is refused
whatever MIME the client declares.
- the customer contract PDF stream applies assertContractPdfPath like the
admin and public contract routes
- OG previews fall back to the site card for draft, archived and
deactivated galleries instead of leaking name, date and welcome message
- video Range requests are validated before the 206 is written; a NaN,
inverted or out-of-file range now answers 416
- share-token comparisons in gallery resolve/info use the constant-time
helper share-login already used
- middleware/photoAuth.js and the galleryAuth/photoAuth/verifyGalleryAccess
exports of middleware/auth.js were unreferenced since the static mounts
went; the auth.js copy had neither slug binding nor issuer pin, so it is
removed before anyone mounts it
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.
- maintenance mode classified paths case-sensitively while Express routes
case-insensitively, so /API/... walked past the gate
- the general rate limiter skipped anyone holding any verified JWT; a
gallery token is minted for free on password-less galleries and slideshow
links, so that was an unlimited budget for every /api route. Only admin
sessions skip now
- ?admin_preview=1 trusted a verified signature alone; it now applies the
same revocation, restore-cutoff, deactivation and password-change checks
adminAuth does, and reveal-mode reads the verified flag instead of
re-decoding the token
- the 50mb JSON limit is scoped to /api/admin and /api/v1; everything else
gets 2mb, so an unauthenticated body can no longer stall JSON.parse
- the CSRF Content-Type gate accepted multipart from any origin; cross-site
form posts are now rejected via Sec-Fetch-Site / Origin, with a Host match
fallback for same-origin installs that leave FRONTEND_URL unset
chunked-upload/init stored the client-declared mimeType on the photo row and
the gallery, secure-image and protected-image routes echoed it as
Content-Type, so a JPEG/HTML polyglot declared as text/html rendered inline
on the app origin for every guest. The admin photo route already resolved
the type safely (#908 review); that logic now lives in
utils/photoContentType and every serving route uses it.
The chunked path derives the MIME from the filename extension and requires
that extension to be on the admin allow-list, matching what the multipart
path enforces through its multer fileFilter.
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.
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.
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.
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.
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.
Codex review round 4 on #1273.
Settings → Status rendered a green check for the email processor
unconditionally, against an API field that was itself the literal 'active'.
Both ends were lying and only one of them got fixed: adminSystem started
reporting the real state in an earlier commit, but StatusTab never read it, so
the second place an admin looks to find out why mail is not arriving still said
everything was fine. It now shows stopped and degraded, with the reason.
The truncation flag missed the case it most needed to cover. The loop broke on
the 200-row report cap before the flag could be set, so 201+ overdue rows came
back as exactly 200 with scanTruncated false -- a partial report presented as
complete. It is now set whenever rows were left unexamined.
The grace-window comment claimed the processor clears ~6000 rows inside the
window. It clears on the order of 100: ten rows a pass, one pass a minute. The
comment now says so, and says why the processor's own state is reported above
the list rather than inferred from it -- "running, last pass sent 10" next to a
backlog reads very differently from "not running" next to the same backlog.
One round-4 finding is NOT fixed, deliberately, and is written up at the retry
route. Clearing scheduled_at leaves created_at at the original enqueue time, so
a retried old row appears in the waiting list immediately, looking overdue,
until the processor sends it. Restarting that clock needs a timestamp written
there and no shape works: a Date matches how queueEmail writes the column and
how processEmailQueue compares it, but jest's sandbox Dates store as
"[object Object]" (CLAUDE.md) so it cannot be tested; an ISO string tests fine
but stores as TEXT, which SQLite then orders above the numeric bound in the
processor's own pickup query, leaving the row unsendable. A requeued_at column
would settle it. Cosmetic either way, and not worth risking a stuck row.
1 more test, failing before this commit.
Codex review round 3 on #1273. The first finding reverses a round-1 fix of
mine, correctly.
Retry no longer sends. Round 1 flagged that retry was a no-op for waiting rows
and offered two remedies: give them a send-now action, or stop showing them
Retry. I took the first, and round 3 showed why it is the wrong half --
processEmailQueue claims nothing before invoking the transport, so a flush
overlapping the scheduled pass has both of them sending the same email. Saving
60 seconds is not worth a duplicate landing in a customer's inbox, and a claim
protocol would need a status no query watches plus a reaper for rows abandoned
mid-send. So retry is a reset again, as it was on main.
Waiting rows now carry no actions at all, which is the other half of that
round-1 remedy and closes a worse hole the shared table opened: Dismiss DELETEs
the queue row. Those emails have not failed and still go out once the processor
recovers, so clicking the tidy-up icon on a health warning silently cancelled a
customer's mail. The section is diagnostic; what a waiting row needs is the
processor fixed, which the panel above it now says.
The grace window runs from when a row became DUE, not from when it was queued.
A split-payment invoice created three days ago and scheduled until a minute ago
has had one minute of the processor's attention, and measuring from created_at
reported every scheduled mail as unworked the instant it came due -- which is
most of what this panel would then have been showing.
A truncated scan can no longer read as an all-clear. The scan is bounded, so a
queue larger than the budget whose head is all future-scheduled can hide a due
row past the last page read; the response now says so and the UI withholds the
green check.
The test fixtures were wrong in a way worth keeping: scheduled_at also defaults
to CURRENT_TIMESTAMP, so back-dating created_at alone built rows that cannot
exist in production -- old, but scheduled for the moment the fixture ran. The
helper now back-dates both, as the database would have.
3 more tests; the two that pin new behaviour fail before this commit, and the
reverted flush is pinned by asserting the transport is NOT invoked.
Codex review round 2 on #1273. Both findings restore the false all-clear that
round 1 set out to remove, by different routes.
Both timestamp columns default to CURRENT_TIMESTAMP, which SQLite renders as a
zone-less 'YYYY-MM-DD HH:MM:SS' in UTC -- and Date.parse reads that shape as
LOCAL time. On a TZ=America/New_York deployment a row due now looked four hours
away and never reached the waiting list; nine hours the other way, fresh mail
read as long overdue. The parser now stamps the zone the value actually
carries.
That parser moved to utils/queueTimestamps so it can be tested honestly. This
suite runs in UTC, where reading a zone-less value as local and as UTC give the
same answer, and process.env.TZ does not reliably re-bind mid-process -- my
first attempt at these tests passed against the broken code for exactly that
reason. They now force TZ in a child process, so they fail on any host.
The candidate rows are paged rather than cut off with one LIMIT. The time
filter runs in JS, so a queue holding more than a page of future-scheduled rows
-- split-payment invoices are exactly that shape -- filled the window with rows
that all got filtered out and hid the due row behind them, reporting nothing
waiting. Paging also drops the dependency on ORDER BY created_at meaning
anything, which it does not on SQLite once numeric and text timestamps mix.
Bounded at 10k scanned; past that the response is a sample, which the 200-row
cap already made it.
12 more tests. The paging one fails before this commit, and all four
naive-timestamp ones fail against the old parsing on any host.
Codex review round 2 on #1274. Both findings are the same shape as round 1:
a message that asserts more than the response supports, and sends the admin
somewhere that makes it worse.
A 5xx is no longer treated as a clean failure. createInvitation inserts the
customer_invitations row and only then queues the email, with no transaction
around the pair, so a 500 out of the queueing step leaves an OPEN invitation
behind. Telling the admin "no invitation went out, retry" there walks them into
a 409 that still queues nothing. 5xx now joins the no-response case as
unconfirmed; a plain 4xx keeps the clean-failure message, because that is the
one shape where nothing was written.
The two already-active conflicts now say so. The send-invite route returns
CUSTOMER_ALREADY_ACTIVE from its own check, but createInvitation rechecks
customer_accounts afterwards and threw a bare ConflictError -- code CONFLICT,
indistinguishable from the pending-invitation conflict. An invitation accepted
between the two checks therefore landed in the pending branch, telling the
admin to cancel an invitation that acceptance had just closed. Both conflicts
in the service now carry a code of their own, so the client reads the code
rather than inferring from the status.
4 more tests. One round-1 test changed with the behaviour it pinned: its 500
now asserts the unconfirmed message, and a new 400 case covers the clean
failure it used to stand for.
Codex review round 1 on #1273. One of the four is a real bug on every SQLite
deployment.
The waiting-row query compared `created_at` against a bound ISO string. On
SQLite that column does not hold a string: queueEmail writes a JS Date and the
native binding stores epoch ms, and SQLite orders INTEGER before TEXT
regardless of value -- so the comparison was true for EVERY row. Mail queued a
second ago read as ten minutes overdue, and a scheduled_at years in the future
read as already due. Confirmed directly against sqlite3: a 2026 row matches
`created_at <= '2020-01-01T00:00:00.000Z'`.
Binding a Date instead is not the fix, since knex hands sqlite3 a Date the same
way and jest's sandbox Dates stringify to "[object Object]" (CLAUDE.md). So the
engine-safe half of the predicate stays in SQL and the two time comparisons
move into JS behind a toMillis() that accepts all three shapes this column
really has -- Date from Postgres, ms-number from SQLite, ISO string from
fixtures and older rows. The scan is capped at 1000 pending rows ordered
oldest-first; everything overdue sorts into that window, and the response was
already capped at 200. The existing tests missed this because they store ISO
strings, which is what CLAUDE.md prescribes for jest -- so the new ones store
epoch ms, the production shape, and one mixes both in a single queue.
Retry was a no-op for the rows it most needed to help. It wrote pending /
retry_count 0 / no schedule, which is exactly what a waiting row already is:
the row came back unchanged while the toast said it had been re-queued. And
since the usual reason a row is waiting is that nothing is working the queue,
deferring it to the next pass is the one answer that cannot help. It now
follows the reset with the same single-row flush the project cockpit uses.
An idle pass no longer inherits the previous pass's totals -- the no-pending
early return skipped the lastResult assignment, so System Health kept
attributing an old sent/failed count to a run that did nothing.
"All clear" now means the whole queue is clear, which is what the PR claimed
and the code did not do. An empty waiting list is only reassuring when
something is working the queue: a processor stopped a minute ago has no overdue
rows yet either, and a green check there is the same false all-clear this
branch exists to remove.
7 more tests. The 5 that pin new behaviour fail before this commit; the SQLite
ones fail in the way the bug predicts rather than erroring.
Both new tests stub the webhook transport with a spy rather than pointing it at
a dead port: real connection attempts left open handles that destabilised
unrelated suites in the same jest worker.
Closes#1262.
"Gallery email queued" reads as a delivery confirmation, and System Health
agreed with it: "No stuck or failed emails -- all clear", while not one email
had gone out.
Both statements were true and neither was the one the admin needed. Queueing
writes an email_queue row at status='pending', retry_count 0 -- nothing more.
/failures matched only status='failed' or pending-with-retry_count>=3, so it
matched none of those rows, and there are two ordinary ways they never leave
that state:
- startEmailQueueProcessor() was never reached, so nothing polls the queue.
- Every pass returns early. processEmailQueue bails when the transporter will
not initialise, before it touches a single row, so retry_count stays 0 and
no error_message is ever written. A working SMTP test button does not
contradict this: that path builds its own transport.
adminSystem.js made it worse by reporting `emailProcessor: { status: 'active' }`
as a literal, so the one place that named the worker always said it was fine.
- emailProcessor records what each pass did -- started, lastRunAt, lastResult,
lastError -- and exports getQueueProcessorStatus(). The transporter bail and
the queue-query failure, the two silent early returns, both write lastError.
- /failures gains `waitingEmails`: pending, under the retry cap, past any
scheduled_at, and queued more than 10 minutes ago. The predicate mirrors the
processor's own pickup query, so a row listed there is one it should already
have taken; rows over the cap stay in `stuckEmails` and are not counted
twice. A future scheduled_at is left alone -- split-payment invoices and the
business-hours floor park rows deliberately.
- System Health leads with the processor's state (running / stopped /
degraded) and lists waiting emails in their own table. The all-clear now
needs both buckets empty.
- adminSystem reports the real processor state instead of the literal.
- The two "queued" toasts say the queue processor is what sends it and where
to look if it doesn't arrive.
8 route tests, all 8 failing before the change.
A guest coming back through their own already-redeemed link is the
ordinary #1265 case, and the identity the device holds is theirs. The same
link opened on a shared device that holds another guest's identity is not:
the redemption 409s, ensureIdentity() falls through to the stored identity,
and the visitor's likes are filed under the previous person.
The two cases were indistinguishable client-side, so the 409/410 body now
carries the invite's guest_id. On a mismatch the stored identity is cleared
and the visitor is asked who they are. A response without guest_id keeps
the previous behaviour.
The general /api limiter had been inert since it was written, so its 100
requests per 15 minutes per IP was never exercised against real traffic.
Applying it for the first time with that budget would have 429'd a venue
wifi NAT after roughly twenty guests per window, since every call a
gallery landing page makes before the password is typed counts. 300 keeps
the protection and clears the realistic case. An explicit app_settings
value still wins over this fallback.
The SSRF vetting is resolve-then-fetch and production-only. Say so, and
say why that is acceptable: the hostname is admin-controlled, the request
is confined to allowlisted paths and carries no PicPeak credentials, and
the S3/MinIO client already takes the same posture.
The local escapeLike copy and its comment predate 0ef51148, which stopped
escapeLikePattern() doubling single quotes. The comment was therefore
false and the helper byte-identical to the shared one. Use
escapeLikePattern() + likeWithEscape(), as every other search does.
POST /api/auth/admin/change-password and POST /api/customer/profile/password
both verify the current password before replacing it, which makes them a
credential check an attacker holding a hijacked session can drive at will:
the session's own JWT skips the general limiter as authenticated, and they
were not in the auth gate's table. Both join it. Only failures count, so
the one change a user legitimately makes costs nothing.
repairGerman gated subject, body_html and body_text on body_html alone, the
same defect Codex found in migration 194: an admin who had translated only
the subject lost it the moment the HTML still matched English, and down()
is a deliberate no-op, so the loss was unrecoverable. Each field is now
judged independently for both the translations row and the legacy _de
columns, matching 194's corrected pattern. Two tests pin the two
directions (translated subject over English body, and the reverse).
Express's `case sensitive routing` is off by default, so /API/admin/events
reaches the same handler as /api/admin/events. Both the gate's `/api/` prefix
test and rateLimitService's public-endpoint classification compared the raw
path, so simply upper-casing a letter skipped the limiter entirely.
Verified against a real Express app before fixing: /api/admin/events routes
and hits the gate; /API/admin/events and /Api/Admin/Events route and miss it.
Both now match on a lower-cased path. The auth gate added alongside was
already immune -- its patterns carry the `i` flag for exactly this reason.
Not changed: rateLimitSecurity.hasValidAdminToken's /api/admin/ test has the
same shape, but there the case-sensitive comparison fails safe -- an
upper-cased path simply does not get the admin skip, so it is rate limited
rather than exempted. Making it case-insensitive would widen a skip, so it is
left alone. maintenance.js's isAdminRoute is fail-safe for the same reason.
The five authRateLimiter registrations were inert for the same reason the
general one was -- registered below the error handler. Auth endpoints have
never had an IP limit; the 5-attempt behaviour QA observed is the per-account
lockout in authSecurity.js, which is a different mechanism and is untouched.
They could not simply be activated: app.use('/api/auth', ...) is a prefix, so
a 5-per-window budget would have covered GET /api/auth/session and
POST /api/auth/password-strength, which the frontend calls far more than five
times per window. That locks users out.
The real surface was enumerated by loading the routers and walking
router.stack rather than grepping, which showed two of the five registrations
pointed at routes that do not exist: adminAuth.js has no /login (admin login
is POST /api/auth/admin/login) and there is no /api/gallery/:slug/verify
(gallery verify is POST /api/auth/gallery/verify).
Now limited, on exact method+path: admin login, admin MFA verify, gallery
password verify, share-login, client PIN, setup verify-token, setup admin,
customer login, customer password-reset. Deliberately unlimited: session
checks, password-strength, logouts, authenticated change-password, the SSO
round-trip (a 429 on the callback breaks login from shared corporate IPs),
and one-time invite/accept-invite links.
Two choices carry the design. skipSuccessfulRequests means only failed
attempts spend budget, which is what makes 5-per-IP survivable behind NAT --
ten guests on one venue wifi all typing the correct gallery password consume
nothing -- and means a legitimate admin cannot be locked out by their own
success. And the limiter keeps its own rateLimit() instance, hence its own
store and its own per-IP bucket, with the general gate's auth exemption left
in place: sharing a counter is exactly the lockout described above.
Patterns are case-insensitive because Express's case-sensitive routing is off
by default, so POST /api/auth/admin/LOGIN reaches the login handler and a
case-sensitive pattern would have been a free bypass.
max is now read per request, so the Settings UI's rate_limit_auth_max_requests
applies without a restart, matching the general limiter.
Tests prove both directions: each credential endpoint 429s on attempt 6 with
the response shape the four login pages already branch on, each benign
endpoint still returns 200 after 40 calls, the two buckets are independent in
both directions, and 30 consecutive successful logins consume no budget.
Refs testplan REPORT.md, rate-limiter gap.
Adds varsIgnorePattern and ignoreRestSiblings to no-unused-vars, the config
recommendation left open when the lint backlog was cleared.
The "omit fields via rest spread" idiom is intentional and recurring --
adminEvents/helpers.js destructures password_hash and client_password_hash
purely to keep them out of `...rest` -- and without ignoreRestSiblings every
occurrence needs its own disable comment, which is noise that also suppresses
genuine findings on the same line. Removed the one such comment that now
exists; the explanatory comment above it stays, since the intent is not
obvious from the code.
Lint stays at 0 problems.
Refs testplan REPORT.md D1.
Two warnings, both of which turned out to be mis-stated.
Search: the name printed on every card is photos.original_filename (not
source_filename, which is the replacement-stable ingest key and is not in the
gallery payload at all), but search matched only the stored renamed filename.
So a substring the admin or guest can literally read on screen returned zero
results. Fixed on the admin Photos tab, which filters server-side -- grouped
OR, because the feedback AND/OR conditions are appended immediately below and
a bare orWhere would leak across them -- and on the Story theme's own scene
filter, which is a second independent client-side search box.
Dates: the warning read "Transfers uses DD/MM/YYYY while the rest of the app
uses long-form dot dates", but it is inverted. TransfersPage already routes
every date through useLocalizedDate and was correctly honouring the rig's own
configured general_date_format of {"format":"DD/MM/YYYY","locale":"en-GB"}.
The surfaces it was compared against are the ones ignoring the admin setting,
by passing an explicit format string that overrides it. Dropped the hardcoded
'MMM d, yyyy' from the two EventsListPage table dates so they follow the
setting like Transfers does.
AdminHeader's format(new Date(), 'PPPP') is left as-is: that is the decorative
"today" banner, where a long weekday form is a deliberate design choice rather
than a data date, and forcing it to DD/MM/YYYY would read worse.
Refs testplan REPORT.md, search-by-original-filename and transfers-date
warnings.
app.use('/api/', generalRateLimiter) lives inside initializeRateLimiters(),
which is defined at line 463 but not called until 1048 -- by which point the
routers (767+), the /api notFoundHandler (1002) and errorHandler (1029) are
already on the stack. All six app.use() calls in it therefore append BELOW the
error handler and can never see a request. generalRateLimiter had no other
registration path.
So the entire /api surface had no IP-based request limit, except the handful
of routes carrying their own inline rateLimit() (public quotes, contracts,
payment-check, transfers, the analytics proxy). The admin Settings
rate-limiting UI -- rate_limit_enabled, rate_limit_max_requests -- was writing
to a control that did nothing.
Fixed with a stable gate registered above the routers that resolves the
limiter per request, so there is no boot delay: it is a pass-through until
initializeRateLimiters() resolves, exactly matching prior behaviour.
Registered unmounted (app.use(gate), not app.use('/api', gate)) because
Express strips the mount path from req.url and rateLimitService's own logic is
written against the full path -- req.path.startsWith('/api/public/') and the
/api/(gallery|secure-images)/:slug regex it uses to find a gallery token to
skip on. Mounting it would have silently broken both.
Deliberately excluded, each for a concrete reason:
- /health and /api/health, mounted above the gate: a 2s probe is 450
req/window and would 429 the container healthcheck.
- /api/public/transfer and transfer-upload: one request per file from a link
holder with no JWT, so never skipped as authenticated; a large transfer
would be cut off mid-way. Both already have tighter per-minute limiters.
- login and gallery-verify: the limiter returns authMaxRequests (5) as their
budget but counts them into the SAME per-IP bucket as every other /api call,
so the branding and settings fetches a login page makes before anyone types
a password would 429 the login itself for a full window. Giving these a real
per-IP limit means giving them their own bucket.
Bulk gallery and admin traffic is unaffected: skip_authenticated defaults true
and cookie tokens are promoted to Authorization before the gate runs, and
skipped requests do not increment the counter.
Also adds /api/health as an alias of /health -- one handler, identical
exposure -- which silences a ~2s probe warning. Registered above the API
middleware chain deliberately: left at its original position it would have
passed through apiRequestLogger and through maintenanceMiddleware, whose
skip-list contains /health but not /api/health, so it would have 503'd during
maintenance while /health returned 200.
The tests pin registration depth by source inspection as well as behaviour,
because depth is what was broken and no unit test of the gate can catch it.
Refs testplan REPORT.md, /api/health warning; rate-limiter gap found while
fixing it.
Verified against a real SQLite connection -- each of these returned zero rows
before and the right row after:
"Sarah's" before=[] after=["Sarah's Birthday"]
"100%" before=[] after=["Summer 100% Sale"]
"Gala_" before=[] after=["Gala_Night"]
Two bugs in one helper. It did .replace(/'/g, "''"), which is SQL string-quote
doubling -- meaningless and actively corrupting for a value that is bound, so
any search containing an apostrophe matched nothing. And its \% escaping had
no ESCAPE clause on the LIKE, which is engine-dependent: honoured on Postgres,
a literal backslash on SQLite, so % and _ stayed wildcards there.
Now mirrors the correct implementation from 59666b59: escape \ % _ only, and
a new likeWithEscape(column) emits `col LIKE ? ESCAPE '\'`. Both call sites
move to whereRaw with the value still bound; the column argument is a literal,
documented in the JSDoc.
Callers checked before changing the contract: adminPhotos.js,
adminEvents/crud.js, and sqlSecurity's own addLikeCondition(), which has no
callers anywhere -- pre-existing dead export, updated to the new shape rather
than deleted.
Behavioural change: searches containing ' % _ or \ now return the right rows
instead of nothing. Case sensitivity is unchanged.
Refs testplan REPORT.md, escapeLikePattern finding.
A self-hosted Umami/Rybbit domain configured in Settings could never load: the
CSP script-src allowlist is static, and the earlier pass could only add an
admin-visible warning because nginx.conf:58 strips helmet's header and
location / serves the SPA document off disk via try_files -- so helmet can
never govern it in Docker. Verified by reading the config, not inferred; that
kills the "make helmet dynamic" option outright.
Rather than templating the CSP, the tracker is now same-origin. The script and
every endpoint it talks to are served from /api/analytics/tracker/* and
proxied server-side to the configured instance, so script-src 'self' and
connect-src 'self' already cover it. The CSP is unchanged: nothing to
template, no env var, no restart -- it takes effect when Settings is saved.
That also closes A3 structurally rather than by widening a directive.
Endpoint mapping taken from vendor sources, not guessed: Umami's
host || currentScript.src + /api/send, and Rybbit's documented
/track, /site/tracking-config/<id>, /site/<id>/feature-flags/evaluate.
data-host-url is set explicitly so a COLLECT_API_HOST-built Umami cannot
bypass the proxy. Session replay is deliberately NOT proxied: replaying
gallery pages would capture the share token (GHSA-7m6c).
nginx still needed one line, for a non-obvious reason: the static-asset regex
location outranks the plain /api prefix in nginx's matching order, so
/api/analytics/tracker/script.js resolved as a static file. Confirmed
empirically against a real nginx:alpine -- 404 before the ^~ block, 502
(proxied) after, with /assets/app.js and /api/public/settings unchanged.
The native SERVE_FRONTEND install needed no change; helmet already has 'self'
in both directives and the proxy mounts ahead of express.static.
Security boundary, since this makes the server fetch an admin-supplied URL:
closed per-provider path+method allowlist (4 paths), DNS-resolving
isHostAllowed blocking private/internal/metadata addresses in production
(matching the s3Storage prod-only precedent), base rebuilt as
origin + pathname so userinfo/query/fragment cannot smuggle anything,
redirect: 'error', cookie/authorization/referer/host never forwarded, an
HTML upstream response re-served as application/octet-stream + nosniff, and
64KB request / 2MB response / 5s timeout / 120rpm caps. X-Forwarded-For and
User-Agent are forwarded so geo and device attribution survive.
Residual, stated plainly: an unauthenticated rate-limited relay to one
admin-chosen public host on 4 paths, and TOCTOU DNS rebinding is unmitigated
as it is elsewhere in the repo.
The Umami and Rybbit panels now explain they are proxied; the Custom panel
keeps a CSP warning -- it is the one mode with nothing to proxy -- naming both
script-src and connect-src.
Refs testplan REPORT.md A2, A3.
The "hidden photo has no indicator on the admin grid" warning was not a
missing badge. The badge markup has existed since #172; the defect was in
GET /:eventId/photos, which hand-builds its response literal field by field
and never emitted `visibility` -- so the value was always undefined and
neither the grid tile nor the list row badge could render. Same omission class
as the view_count/download_count bug already commented in that file.
(The `visibility` line itself was swept into 4721bd83, whose message does not
mention it -- recording that here.)
Fixes the adjacent instance too: `processing_status` is missing from the same
mapper, so the grid's "Processing…" and "Failed"/Retry placeholders could
never render either.
On the card, reuses the existing EyeOff badge pattern from the list-view rows,
adds a tooltip on both layouts, and drops the category badge to top-9 so the
hidden badge can own the top-left corner.
Also fixes the Photo Limit spinbutton's aria-valuemax, which read 0 even with
a real cap set. Root cause: min={0} with no max -- for input[type=number]
Blink's MaxValueForRange returns DBL_MAX, fails isfinite and supplies no max,
so a11y tooling prints the default 0. Set to 2147483647, the events.photo_cap
column's real signed-32-bit ceiling (migration 074), which also stops an
out-of-range value failing only at INSERT. The sibling expires_in_days input
already had proper bounds.
Known: EventInformationCard carries the identical Photo Limit input with the
same defect; it is held by another concurrent change and follows next.
Refs testplan REPORT.md, hidden-photo and aria-valuemax warnings.
B6 -- seven gallery routes returned private, per-guest data with no
Cache-Control at all, relying on heuristic freshness. noStoreCache is mounted
per route rather than on the router, because the media routes set their own
private, max-age=1800/3600 and must keep it. Covered: /photos (own
likes/favourites/ratings, hidden photos for a client token), /people, /stats,
/verify-token/:token (an authorization decision -- a cached {valid:true}
outlives a rotated token), /show/:token/session (the response IS a credential;
it mints a gallery JWT), /show/:token/state and /download-jobs/:token (live
polls, where a cached "preparing" strands the caller). Deliberately untouched:
the photo/thumbnail/hero/preview and css-template routes, which set their own
caching, the binary downloads, and /info + /resolve, which are unauthenticated
public metadata rather than per-guest private.
ETag/304 revalidation is intact and pinned by a test: no-store stops the
browser retaining the body, not express agreeing an unchanged payload is
unchanged. That matters because the post-upload poll depends on it.
B7 -- the guest upload flow had no progress signal, so the UI polled the photo
list blind and gave up after 60s with no explanation. Adds
GET /:slug/uploads/status?ids=... rather than pending counts in the photos
payload: counts there are event-wide, so another guest's or the admin's stuck
upload would spin the notice forever and it could never say "your photo
failed".
Authorization: verifyGalleryAccess already resolves req.event from the
caller's token, and the query is scoped `.where('event_id', req.event.id)`, so
an id from another gallery matches no row -- neither a cross-event read nor an
existence oracle, since it returns all-zero counts rather than a 403/404 that
would confirm the id exists elsewhere. Slideshow tokens are denied (a kiosk
never uploads). Ids are pattern-validated, max 50. The response is counts
only: no filenames and specifically no processing_error strings, which can
carry internal paths. Not gated on allow_user_uploads, so an admin flipping
the toggle mid-flight does not strand an in-progress guest.
The frontend now finishes on the real terminal condition, refetches as each
photo lands rather than only at the end, shows a processing pill, and reports
real failures instead of silently timing out.
Refs testplan REPORT.md B6, B7.
Paul Nothaft