* fix(security): enforce event ownership on the v1 API surface (GHSA-9697)
Migration 081 documents the intent — 'the token's effective permissions are
the intersection of the user's role permissions and the token's own scope
flags' — but it was never implemented.
- apiTokenAuth selected only id/username/email/role_id, so req.admin.roleName
was undefined. Every ownership helper keys on roleName, so the v1 surface
could not tell a super_admin from a demoted viewer. Now joins roles and
emits the same req.admin shape adminAuth does, including the
roles-table-missing upgrade fallback.
- No v1 route applied any ownership predicate: GET /events listed every event
on the instance, and GET /events/:id/share-link returned ANY event's
share_token — the gallery access credential, same class as GHSA-rh8r.
List is now scoped via a new scopeEventsQuery helper; the three :id routes
(detail, photo upload, share-link) use the existing requireEventOwnership.
Not a breaking change: tokens are minted by super_admins, who bypass
ownership. It closes the case where a token's owner is later demoted —
userManagementService never touches api_tokens, so the token outlived the
demotion with full read of every gallery's share token.
events.category.test.js stubbed apiTokenAuth without roleName; giving the
stub super_admin keeps requireEventOwnership from issuing a DB query and
desyncing that suite's sequenced dbMock.
* fix(security): codex round 2 — intersect v1 token scopes with role permissions (GHSA-9697)
Ownership scoping alone left half the documented control missing. Migration
081 defines a token's effective permissions as the INTERSECTION of the owner's
role permissions and the token's scope flags; requireApiScope only ever checked
the scope half. A token minted while its owner was super_admin therefore kept
write access after the owner was demoted to viewer — userManagementService
never touches api_tokens, so the token outlives the demotion, and ownership
scoping does not help because the demoted owner still owns their events.
Adds requirePermission to all six v1 routes (events.create on create,
events.view on the reads, photos.upload on upload). It keys on req.admin.id,
which apiTokenAuth already populates.
The two existing v1 suites mock the database, so a real permission lookup
500s — they now mock the permissions middleware as pass-through, matching how
they already mock apiTokenAuth. Those suites cover route logic; the
intersection is pinned by the new v1TokenPermissions suite.
* fix(security): codex round 3 — fail closed on the roles-join fallback (GHSA-9697)
The round-2 fix loaded the token owner's role so the v1 ownership checks could
tell a super_admin from a demoted viewer, and mirrored adminAuth's
roles-table-missing fallback. That fallback assigns role_name = 'super_admin',
and the catch around it was unconditional — so ANY failure of the joined query
(connection reset, deadlock, statement timeout) elevated the token owner to
super_admin as long as the simpler fallback query then succeeded. A restricted
owner could ride that into listing, reading and share-tokening every event on
the instance, which is the exact hole GHSA-9697 closes.
The fallback is now reached only for an error that genuinely names a missing
roles table/column (PG 42P01/42703 or the SQLite/MySQL wording); anything else
propagates to the 500 handler.
Claude-Session: https://claude.ai/code/session_01F211U4dDbEj4zXiyKbi9me
(cherry picked from commit 53d1e5d1b3148a7f4067308b08fcdf8ddab0a39f)
---------
Co-authored-by: Paul Nothaft <[email protected]>
* fix(security): bound inbound-mail resources, redact secrets from logs (GHSA-2qf9, pgmp, r794)
GHSA-2qf9 — emailIntakeService downloaded, parsed and persisted every message
with no size, attachment-count or attachment-byte limit, reachable
unauthenticated by anyone who can email the operator's mailbox:
- fetch the envelope with `size` (same cheap pass) and refuse an oversized
message BEFORE downloading its source;
- cap attachment count and cumulative attachment bytes;
- limits env-overridable, defaults generous for real supplier invoices.
The teeth were in the dedup key. received_emails.message_id is varchar(512)
UNIQUE, and the failure path wrote `err-<uid>-<Date.now()>`, which can never
match the envelope-derived messageId the dedup pass compares against — so an
oversized (or overlong-Message-ID) mail was re-downloaded every poll forever,
and an OOM-kill/restart just resumed the loop. Size-skips are now recorded
under the REAL message id, and overlong ids collapse to a stable sha256 key
that always fits the column.
GHSA-pgmp / r794 — new sanitizeForLog() util (key-name deny-set, recursive,
cycle-safe) applied to the three request-body log sites in adminEvents/crud.js,
plus sanitizeValidationErrors() because express-validator's errors.array()
embeds the SUBMITTED value per field — a rejected plaintext password was still
logged. Scope is wider than filed: the update path also logged
client_password_hash and a LIVE client_share_token bearer credential.
Also: the one-time setup token was logged at warn AND printed to stdout on
every first boot, putting a live first-admin credential in combined.log,
security.log and `docker logs`. It is now written to the 0600 token file and
only surfaced when that write fails — the last-resort path it existed for. (stable)
* fix(security): codex round 3 — repair the first-run token recovery flow (GHSA-r794)
Two regressions from keeping the setup token out of the logs.
1. server.js decided whether to print the token by calling existsSync() on the
candidate path. That answers a different question than "did the write
succeed": a stale, read-only or directory-shaped SETUP_TOKEN reports as
present, so the banner suppressed the live token and pointed the operator at
content that is not it — leaving the current token only in combined.log
under default production logging. setupService now records the path the
write actually produced and exposes it via writtenSetupTokenFile().
2. The setup screen, its EN/DE strings, README, SIMPLE_SETUP and .env.example
all still told first-time users to run
`docker compose logs backend | grep -i "setup token"`. On the normal path
that command now returns a path banner and no credential, so the documented
browser-first onboarding could not be completed. They now point at
`docker compose exec backend cat /app/data/SETUP_TOKEN`, with the log
fallback described as what it is — the failure path.
Claude-Session: https://claude.ai/code/session_01F211U4dDbEj4zXiyKbi9me
(cherry picked from commit 9a54b6f0231c3285df4c4865eb846e63e1ed0dda)
---------
Co-authored-by: Paul Nothaft <[email protected]>
* fix(security): scope dashboard endpoints to the caller's events (GHSA-c2jj, gqx7, jhcf)
/dashboard/stats, /analytics and /activity are gated only by analytics.view,
which the editor role holds — but the events LIST restricts editors to their
own rows (adminEvents/crud.js: roleName === 'editor' -> created_by =
admin.id). So an editor saw instance-wide totals, and via /analytics
topGalleries other admins' gallery NAMES and SLUGS (the public gallery URL
component), for events invisible to them everywhere else.
- stats: all 10 aggregates scoped (events by id, photos/access_logs by
event_id).
- analytics: all 8 series/aggregates scoped, including topGalleries. The
external tracker device breakdown reports instance-wide data with no event
filter, so a scoped caller falls through to the access_logs heuristic
instead, which IS scoped.
- activity: feed scoped. activity_logs.event_id is nullable and the join is a
leftJoin, so system-level rows (logins, settings changes) are deliberately
excluded for a scoped caller — those are precisely the cross-admin actions
the advisory is about.
Scoping keys on 'editor' to mirror the events list exactly, so the admin
role's dashboard is unchanged. filterOwnedEventIds uses the broader
'!== super_admin' rule; the two conventions disagree in this codebase and
matching the list is the no-regression choice.
* fix(security): codex round 2 — fix activity misattribution, scope via subquery (GHSA-jhcf, c2jj, gqx7)
- expenseService passed adminId as logActivity's THIRD positional parameter,
which is eventId — so admin ids were being written into
activity_logs.event_id. The /activity scoping filter trusts that column, and
admin/event id sequences overlap, so a foreign admin's expense metadata could
surface under an editor's event. All 11 calls now pass null for eventId and
the admin as the actor, which is what they meant.
- Dashboard scoping now uses a SUBQUERY instead of pluck()+whereIn. An editor
owning more events than the driver's bind-parameter limit (~999 SQLite,
65535 Postgres) would have turned all three endpoints into 500s once each id
became a placeholder; below the limit it still re-sent the full list for each
of the ~10 aggregates per request.
Note: two billInboundNow() calls also end in ', adminId)' but have an unrelated
signature — verified untouched.
* fix(security): codex round 3 — correct legacy accounting activity rows (GHSA-jhcf)
expenseService called logActivity(type, metadata, adminId), but logActivity's
third positional parameter is eventId. Every expense / incoming-invoice entry
therefore stored the ACTING ADMIN'S ID in activity_logs.event_id.
Round 2 scoped the activity feed with
`WHERE activity_logs.event_id IN (SELECT id FROM events WHERE created_by = me)`,
which does nothing about the rows already on disk. Admin ids and event ids are
small integers from the same range, so on any upgraded instance an editor who
owns the event whose id happens to equal another admin's id is served that
admin's accounting activity, verbatim metadata included — GHSA-jhcf, still
live. Migration 168 re-attributes those rows (event_id holds exactly the actor
id that was lost) and then clears event_id so the scope predicate can no longer
match them. All ten activity types are emitted by expenseService and nothing
else, so no row with a genuine event_id is touched.
Also: the round-2 rewrite passed `{ type: 'admin', id: adminId }`
unconditionally, which stored actor_type='admin' with a null id for the
automated mailbox intake (emailIntakeService calls recordInboundDocument with
no adminId). adminActor() restores 'system' attribution for those.
Claude-Session: https://claude.ai/code/session_01F211U4dDbEj4zXiyKbi9me
(cherry picked from commit 459e9e42434defd0dc7b87246e4d894dd47dcc56)
---------
Co-authored-by: Paul Nothaft <[email protected]>
* fix(security): stop caller-chosen database backup destination (GHSA-jw8m)
POST /api/admin/database-backup/backup forwarded req.body straight into
databaseBackupService.backup(), which merges options over config:
const { destinationPath = '/backup/database', ... } = { ...config, ...options }
destinationPath is not a persistable setting — the /config allowlist only
accepts database_backup_* keys — so the request body was its only source.
The built-in `admin` role holds backup.create but neither settings.edit nor
backup.restore, so it could aim a full DB dump (admin bcrypt hashes, gallery
password hashes, encrypted SMTP creds) at the PUBLIC /uploads static mount
(server.js mounts it with no auth middleware) and then fetch it
unauthenticated. Filed low; it is a privilege escalation to unauthenticated
disclosure.
Forward only the real knobs, and only when present so absent keys can't
override config defaults via spread.
* fix(security): backup/restore hardening — restore path allowlist, gunzip bound, manifest checksum keying (GHSA-fw4c, h652, hgp8)
- adminRestore /validate + /start: constrain caller-supplied source and
manifestPath to the operator-configured backup roots — the SAME set the
restore wizard discovers from — so disaster recovery from a rescued mount
still works, with RESTORE_ALLOWED_ROOTS as an escape hatch (GHSA-fw4c).
- restoreService.decompressFile: bound the EXPANDED size and abort the
pipeline when exceeded; default 50 GB, RESTORE_MAX_DECOMPRESSED_BYTES
overrides (GHSA-h652).
- backupManifest: BACKUP_MANIFEST_KEY upgrades new manifests to a keyed
HMAC (GHSA-hgp8). Deliberately opt-in and verify-if-present — the key
cannot live in the database because the database is inside the backup, so
a mandatory HMAC would lock operators out of the exact disaster-recovery
case this exists for.
Also fixes a pre-existing bug found while testing hgp8: the checksum passed
Object.keys().sort() as JSON.stringify's second argument, which is an array
REPLACER (a property allowlist applied at every depth), not a key sorter. All
nested keys — path, size, per-file checksum — were dropped before hashing, so
the file list sat outside the integrity check entirely and a manifest path
could be rewritten to ../../etc/passwd without disturbing the digest. Now
hashes a recursively-canonicalized copy, with the legacy serialization
accepted on validation so existing backups stay restorable.
* fix(security): codex round 2 — unbreak the restore wizard, share checksum verification, guard downgrades
- adminRestore: `source` is usually a SOURCE TYPE ('local'|'s3'|'upload'),
not a path — restoreService branches on those literals. The containment
check treated it as a path, so path.resolve('local') fell outside the
backup roots and BOTH /validate and /start returned 400, blocking every
normal restore. Type tokens are now excluded from the path check.
- backupManifest: extracted verifyManifestChecksum() as the single source of
truth for the legacy/keyed fallbacks. restoreService.performPreRestoreValidation
recomputed the digest itself with the default canonical+keyed settings,
which rejected EVERY backup written before this batch. It now delegates.
- backupManifest: guard the algorithm downgrade — with a key configured, an
attacker able to rewrite the backup store could strip checksum_algorithm,
edit the manifest and recompute a plain SHA-256 that verified. Opt-in via
BACKUP_MANIFEST_REQUIRE_KEYED so pre-key backups keep restoring by default.
* fix(security): codex round 3 — close two manifest-verification fail-opens (GHSA-hgp8)
verifyManifestChecksum returned valid for a manifest with no
verification.total_checksum at all, and restoreService only called it when
that field was present. Deleting the field was therefore a complete bypass of
the keying work: no digest check, no downgrade guard, no
BACKUP_MANIFEST_REQUIRE_KEYED. Every manifest this codebase writes stamps the
field, so an absent one now fails validation, and the call site invokes the
verifier unconditionally.
Second fail-open: the strict-mode rejection of an unkeyed manifest was gated on
`&& key`, so with BACKUP_MANIFEST_REQUIRE_KEYED=true and no BACKUP_MANIFEST_KEY
configured a plain SHA-256 manifest sailed through. Strict mode is a statement
about the operator's manifests, not about the host — it is exactly the fresh
disaster-recovery box that lacks the secret. The rejection no longer depends on
a key being present.
Claude-Session: https://claude.ai/code/session_01F211U4dDbEj4zXiyKbi9me
(cherry picked from commit 292a5b784acd7f47099aa234c1c2ea00050fca97)
---------
Co-authored-by: Paul Nothaft <[email protected]>
* fix(security): close authz/ownership gaps (secure-download binding, photo-auth+logout revocation, feedback/customer ownership, token logging)
* fix(security): codex round-1 — complete admin-token invalidation + preserve foreign assignments
- photoAuth: mirror adminAuth's active-admin lookup + iat<password_changed_at
check in the admin branch, so a deactivated admin or a pre-password-change
token can no longer fetch every photo (GHSA-x55x was only revoke+cutoff).
- adminAuth logout: revoke req.token (the token adminAuth authenticated with,
cookie OR header) instead of header-only, and clear the auth cookie — a
cookie-based logout previously left the JWT live (GHSA-cjqh).
- adminCustomers PUT /:id/events: preserve the customer's existing
assignments to events the caller does NOT own, so a restricted admin can't
revoke another admin's customer-event links via full-list replacement.
* fix(security): codex round-2 — don't 403 legit restricted-admin assignment edits
The Manage-galleries dialog submits the full initial assignment list, so a
restricted admin editing a customer that already has a foreign assignment hit
the denied.length 403 before the preservation logic ran. Reject only
NEWLY-supplied foreign/nonexistent ids; retain foreign ids the customer is
already assigned to (they can't be added or removed by a non-owner).
---------
Co-authored-by: Paul Nothaft <[email protected]>
* fix(security): stop unauth share_token leak + block restore path-traversal, logo-path file read, branding path keys
* test: update resolveLogoFile for the c7x5 containment (reject outside-storage absolute paths, keep inside)
* fix(security): codex round-1 — escape LIKE wildcards in share-link resolve, keep in-storage absolute logos, guard restore verification
- shareLinkService: escape %/_ in the link_partial LIKE fallback so an
anonymous /resolve/____… wildcard can't match an arbitrary share_link and
leak its bearer token (reopened GHSA-rh8r). Explicit ESCAPE for SQLite.
- resolveLogoFile: re-add the raw absolute candidate but keep it subject to
the storage-root containment filter (GHSA-c7x5) so legit in-storage
absolute logos resolve while /etc/passwd stays rejected.
- restoreService: apply the same pathEscapes guard in post-restore
verification so a skipped traversal entry isn't fs.access'd/hashed.
---------
Co-authored-by: Paul Nothaft <[email protected]>
* fix(uploads): prevent cross-photo contamination from filename collisions and non-atomic writes (#931)
* test: pin the suffixed photo filename format in the NFD pipeline suite (#931)
* chore(deps): promote p-limit to a direct dependency for the watermark limiter (#931)
* test: make the suffix-uniqueness check deterministic-in-practice (#931)
* fix(uploads): widen the anti-collision suffix to 48 bits (#931)
* fix(uploads): hide staging files from list() + share one watermark limiter process-wide (#931)
* fix(uploads): reclaim orphaned staging files + revalidate watermark settings in queued jobs (#931)
---------
Co-authored-by: Paul Nothaft <[email protected]>
* fix(security): close two access-control advisories (GHSA-g94x, GHSA-pv6w) (stable)
GHSA-g94x-8vv8-3c9f (HIGH) — the secure-image VIEW route
(/secure-images/:slug/secure/:photoId/:token) validated only the token
signature and took the gallery/photo from the URL, so a token minted on
any PUBLIC gallery read every other gallery's photos with no password
(its download sibling has verifyGalleryAccess; the view route can't —
it serves via <img src> with no header). Bind the token to its scope
instead: the URL photoId must equal the token's minted photoId (photos
belong to exactly one gallery, and minting is gallery-scoped), and the
gallery embedded in the token's sessionId must equal the URL gallery.
GHSA-pv6w-rj34-wj9v (MEDIUM) — GET /admin/backup/picpeak/export dumps
every table unredacted (bcrypt hashes, 2FA, SMTP/SSO/WhatsApp/webhook/S3
secrets) and was gated only by backup.create, which the built-in admin
role holds. Gate it behind super_admin, matching the restore side
(backup.restore, already admin-denied) and the masked config APIs.
Regression tests pin both: cross-gallery token reads 403 (photo and
gallery checks), backup export 403 for admin / passes for super_admin.
Stable port of #924. secureImages on stable has no reveal-mode block, so
only the token-binding checks are added; the backup export gate is
identical.
* fix(security): review follow-ups on the export gate (GHSA-pv6w)
- test: place the mocked export in its own mkdtemp dir. The route
recursively deletes path.dirname(filePath) after download, so a stub
in bare os.tmpdir() made the super_admin test wipe the whole temp
root — other jest workers' DB files included (latent CI flake).
- ui: hide PicpeakExportCard from non-super_admins. The role keeps
settings.view + backup.create, so after the gate its Download button
always 403'd with a generic toast; gate the card on role super_admin
to match the endpoint.
* fix(security): keep the token-mismatch audit values within varchar(20) (GHSA-g94x review)
image_access_logs.access_type is varchar(20) (migration 038), but
'token_gallery_mismatch' is 22 chars — on Postgres the audit write
threw value-too-long and logImageAccess swallowed it, so the security
event went unrecorded (the 403 still fired; log is best-effort). Shorten
to 'photo_mismatch' / 'gallery_mismatch' (14/16).
---------
Co-authored-by: Paul Nothaft <[email protected]>
* fix(admin): expose view/download counters in the admin photos list (#895 follow-up) (stable)
st-ivan's re-test after #904: statistics panel and event summary now
agree, but the per-image Engagement column still shows 0. Root cause:
the admin photos LIST endpoint maps rows to an explicit response object
that includes like/comment/rating/favorite counts but never included
view_count or download_count — the grid reads photo.view_count ?? 0,
so the column showed 0 regardless of what the DB counted. This mapper,
not stale data, is also why per-image downloads always displayed 0 in
the original report.
Suite extended with a list-endpoint assertion (beacon + download, then
the admin list reflects 1/1 and untouched photos 0/0). The skip test now
neutralizes the route's background pre-zip build, whose async ENOENT
against the intentionally missing file could land mid-suite.
Includes the one-line chunkedUploadService unref from #911 so the test
suite can mount adminPhotos regardless of merge order (identical change,
merges cleanly either way).
* test: widen the fire-and-forget settle window (#895 follow-up)
The 100ms settle was marginal on loaded CI runners — the counter
increments are deliberately fire-and-forget, and the 909 PRs flaked on
exactly these assertions. 400ms keeps the suite fast while giving slow
runners room.
---------
Co-authored-by: Paul Nothaft <[email protected]>
* fix(admin): serve videos with their real MIME type in the admin photo view (#908) (stable)
The admin view route built Content-Type from the filename extension —
image/<ext> — which is invalid for videos (image/mp4). The admin player
fetches this URL into a blob that inherits the type, and browsers
refuse to play a <video> blob labeled image/*: blank/grey preview,
while download (which already uses photo.mime_type) worked fine.
Stored mime_type now wins; videos without one fall back to video/mp4,
images to the extension, and extensionless files to image/jpeg instead
of the equally invalid bare 'image/'.
Also unrefs chunkedUploadService's module-level hourly cleanup interval:
it kept Jest from exiting for any suite requiring adminPhotos (it's why
adminPhotos.reference sits on the CI ignore list). Production behavior
unchanged — the HTTP listener keeps the process alive.
New adminPhotoContentType suite pins all four MIME cases.
* fix(admin): harden admin photo Content-Type resolution (#908 review round)
External review findings, all verified:
- The header is now ALWAYS image/* or video/*. photos.mime_type is
never echoed verbatim unless it is a video/ type — the chunked-upload
path stores the client-sent MIME unvalidated, so a stored text/html
served inline under the app origin was a same-origin XSS hazard.
- MIME-less videos map from the extension via the shared
EXTENSION_TO_MIME (.mov → video/quicktime, .webm → video/webm)
instead of a blanket video/mp4 that would mislabel them.
- Images ignore the stored MIME entirely: migration 039 backfilled
image/jpeg onto every legacy row (PNGs included), so trusting it
would regress previously-correct extension-derived types. Extension
wins, normalized (jpg → image/jpeg).
Suite extended to 8 MIME cases including the XSS guard and the
039-backfill immunity.
* fix(admin): validate stored video MIME as a full header-safe token (#908 review round 2)
A prefix check let malformed client-stored values through:
'video/mp4\r\nX: y' makes res.setHeader throw ERR_INVALID_CHAR — a
permanent 500 for that photo — and a bare 'video/' is an invalid type.
Strict /^video\/[\w.+-]+$/ now gates the stored value; anything else
falls back to the extension map. Two new tests pin both shapes.
* fix(admin): map-only image Content-Type — no raw extension interpolation (#908 review round 3)
image/${ext} could synthesize image/svg+xml (scriptable when served
inline) or header-invalid values from client-controlled chunked-upload
filenames. The shared EXTENSION_TO_MIME map is now the allowlist on the
image side too; unmapped extensions serve as image/jpeg — browsers
sniff image bytes in img/blob contexts, so a mislabel is harmless where
an injected type is not.
* fix(admin): own-property lookup in the extension MIME map (#908 review round)
A client-controlled filename ending in .constructor / .__proto__ /
.toString made EXTENSION_TO_MIME[ext] return an inherited Object.prototype
member (truthy), and the downstream extMime.startsWith threw —
a permanent 500 on the admin view for that photo instead of the JPEG /
mp4 fallback. hasOwnProperty-gated now; test pins both a .constructor
image and a .__proto__ video.
* fix(admin): honor safe stored image MIME for auto-imported formats (#908 review round 2)
My previous round made the image side map-only to dodge the migration
039 image/jpeg backfill and image/svg+xml — but that regressed the S3
auto-importer (STORAGE_AUTO_IMPORT), which stores correct types for
avif/bmp/tiff/heic whose extensions aren't in EXTENSION_TO_MIME. Those
now served as image/jpeg (JPEG-labelled non-JPEG bytes).
Precedence is now mapped-extension (still corrects the 039 backfill on
PNGs) -> stored MIME IF in a safe raster allowlist (avif/bmp/tiff/heic
+ the mapped ones) -> image/jpeg. Allowlist, not a regex: image/svg+xml
stays excluded (scriptable inline). Tests pin avif preserved and svg
degraded to jpeg.
* fix(admin): allow any header-safe raster MIME, deny svg/xml (#908 review round 3)
The round-2 hand-listed Set kept missing formats the S3 auto-importer
stores (apng/ico/jxl beyond avif/bmp/tiff). Replace it with a regex:
honor image/<token> EXCEPT the scriptable svg / *+xml family. Covers
every current and future raster type in one rule while still blocking
inline-scriptable svg and header injection. Tests pin apng + x-icon
preserved, svg still degraded to jpeg.
---------
Co-authored-by: Paul Nothaft <[email protected]>
* fix(analytics): make per-photo view/download counters actually count (#895) (stable)
Three stacked defects behind 'per-image stats stay 0':
- photos.view_count had NO writer anywhere — the admin IMAGES table and
photo viewer display it, so it was permanently 0. It now increments
when the full-size photo or its preview tier is served, excluding the
slideshow kiosk (migration 138 design) and follow-up video Range
requests (seeks are not views). Fire-and-forget so analytics can
never fail the byte-serving path.
- Zip downloads (download-all, presigned download-all,
download-selected) never incremented per-photo download_count — only
single-photo downloads did, so zip-heavy galleries showed 0 forever.
The zip routes now bump exactly the photos that went into the archive
(the prebuilt-zip path mirrors the archive builders' category filter).
- Every admin surface used a different definition of 'downloads', which
is the reporter's 46 vs 45 vs 0: event details counted only
action='download' (no zips at all), the dashboard counted
download+download_all but silently EXCLUDED download_selected and
download_all_presigned. All queries now share one action set:
download, download_all, download_all_presigned, download_selected.
New photoEngagementCounters suite pins all of it (7 tests).
* fix(analytics): count views via an explicit lightbox beacon (#895 review round)
External review flagged that request-level view counting is wrong in
both directions: the lightbox preloads prev/next neighbours (3 fetches
per open) while a preloaded neighbour promoted by a swipe is never
re-fetched (#505 keeps the DOM node), and enhanced/maximum galleries
never hit /photo at all (bytes come from /api/secure-images).
- Views now count via POST /:slug/photo/:photoId/view, fired by the
lightbox exactly when a photo becomes the visible slide; the
serving-route increments are removed. Covers protected galleries and
the preview tier uniformly; slideshow kiosk stays excluded.
- bumpEventDownloadCounts mirrors downloadZipService._build (ALL event
photos) — the category filter mismatched the prebuilt zip's actual
contents. (That the builder ignores per-category allow_downloads is a
separate pre-existing issue.)
- Zip loops count only successfully appended entries, with a pre-append
storage stat: a lazy stream's async error bypassed the per-photo
catch and hung the whole response — pre-existing bug, now fixed.
Suite extended to 9 tests (beacon semantics, serve-does-not-count,
skipped-entry exclusion).
* fix(analytics): fire the view beacon from the premium lightbox too (#895 review round 2)
gallery-premium events use yet-another-react-lightbox inside
GalleryPremiumLayout instead of PhotoLightbox, so the layout never
counted views. yarl's on.view fires on open and on every slide change —
identical semantics to the PhotoLightbox beacon.
Also documents the accepted prebuilt-zip approximation: _build can skip
entries whose watermark step fails and still publish the archive;
counting those exactly would need a persisted zip manifest.
* perf(analytics): skip the per-entry zip preflight on S3 (#895 review round 3)
The pre-append source check exists for LocalFs's lazy createReadStream
(async error would kill the whole zip response). S3's get() awaits
GetObject and rejects inside the loop's try/catch on a missing key, so
a HEAD per entry was a redundant serial round trip — 500 extra HEADs
on a 500-photo zip.
---------
Co-authored-by: Paul Nothaft <[email protected]>
Stable backport combining #860 (never reached stable) and #900:
- jest.config.js gains testTimeout: 120000 — stable still ran on Jest's
5s default for anything unpinned, while its migration chain (134 core
migrations via backports) is nearly as long as beta's.
- All 19 suite-level jest.setTimeout(30000/60000) pins raised to 120s;
local pins override the config default (#860's rationale).
- All 15 hook-ARGUMENT timeout pins on migration-booting beforeAll
hooks raised to 120s (#900's rationale — the 3.97.0-beta.0 release PR
failed on exactly this class on the beta side).
Untouched: the three suites whose pinned hooks don't run migrations
(webhookDelivery, imageProcessor.storage, storageBackend) and
publicQuotes' 30s pin on the rate-limit lockout test.
No test logic changed.
Co-authored-by: Paul Nothaft <[email protected]>
* fix(security): close the 5 open Trivy alerts — dep bumps + drop npm from the runtime image
Backend deps:
- postcss 8.5.10 -> 8.5.18 (CVE-2026-45623, GHSA-r28c-9q8g-f849; the pin
exists to force sanitize-html's transitive copy onto a fixed version)
- tar pin/override >=7.5.16 -> >=7.5.21, resolves 7.5.22
(GHSA-r292-9mhp-454m)
Runtime image:
- Remove the npm CLI from the final stage instead of upgrading it: npm's
bundled node_modules ship tar 7.5.19 and brace-expansion 5.0.7 (no npm
release bundles the fixed versions — checked 11.18.0 and 12.0.1), and
npm never runs in production. wait-for-db.sh now invokes the migration
runners via node directly. This ends the recurring npm-bundled-CVE
alert class; the previous 'npm install -g npm@11' line was itself a
patch for the last batch. (stable)
* fix(restore): run post-restore migrations via node — the image ships no npm
restoreService still shelled out to 'npm run migrate:safe' after a
restore; with npm removed from the runtime image that would ENOENT into
the non-fatal catch, silently leaving a restored older backup on a
schema behind the running code until the next container restart. Invoke
migrations/run-migrations-safe.js through node directly, matching
wait-for-db.sh. The PR #596 source-contract test now pins the new
invocation. (stable)
* fix(backup): make backup settings actually apply (#871) (stable)
- Wire the What-to-Backup toggles into the walker: honor
backup_include_thumbnails / backup_include_photos (opt-out,
default ON) and accept the UI's backup_include_archives spelling
for the archived gate (the engine expected _archived, so the
Archives checkbox silently never worked).
- Fix the 167.6 TB dashboard size: file_size_bytes is a bigint that
node-postgres returns as a string, and the S3 path concatenated it
onto the byte counter; coerce to Number at the source.
- Compute the real next scheduled run (cron-parser) and return it as
nextBackup; the UI read a field the API never sent and rendered a
hardcoded 'Not scheduled'. A named schedule label now beats the
stray default cron the UI always sent, which silently turned
weekly schedules into daily 03:00 runs.
- Never back up filesystem noise (.nfs* silly-renames, .DS_Store,
Thumbs.db) and honor backup_exclude_patterns in the walker
(previously rsync-only).
- Remove the compression/encryption toggles from the configuration
UI: no backend implementation exists, and collecting an encryption
passphrase while uploading plaintext is a false promise.
* fix(backup): close the review gaps in the settings wiring (stable)
- The UI's backup_include_archives now beats the migration-seeded
backup_include_archived: every install has the singular key seeded
true, so the alias-only-when-absent lookup made unchecking Archives
a no-op.
- rsync destinations now receive the de-selected What-to-Backup paths
and the noise filters as anchored --exclude args; previously rsync
synced the whole storage root and the walker's selection only shaped
the manifest, which then misreported what was actually transferred.
- Escape regex metacharacters in the walker's glob matcher: '.nfs*'
compiled to /^.nfs.*$/ whose leading dot matched any character, so
files like anfs-photo.jpg were silently dropped from backups.
- The Backup Coverage report now uses the same gate as the walker
(new 'skipped-by-setting' status) instead of re-implementing it
without the opt-out toggles and the archives alias.
* fix(backup): make the coverage diagnostics agree with the walker
- The coverage table shows the alias-aware flag value the gate actually
used, instead of the seeded backup_include_archived shadowed by the
UI's plural key (true next to a 'Gated off' badge).
- skipped-by-setting paths are now counted in the coverage summary
(backend, TS contract, summary card, EN/DE locales) so the totals
reconcile again when Photos or Thumbnails is unchecked.
- The form's thumbnail default now matches the backend's never-saved
fallback (include): the checkbox no longer shows 'off' while
thumbnails are being backed up, and saving an unrelated setting no
longer flips the backup scope. (stable)
* fix(backup): keep custom crons, exclude disabled rows from rsync, normalize flag display
- Saving a named schedule no longer wipes the stored custom cron: the
backend already prefers the label, so the cron field stays inert for
named schedules and is preserved for switching back to Custom. A
custom schedule now validates the 5-field expression before saving
(the backend silently fell back to daily 02:00 on a blank value).
- resolveExcludedBackupPaths now also returns rows disabled via
include_in_default, so rsync excludes them; the enabled-only loader
hid them and rsync transferred their contents anyway.
- The coverage table normalizes flag values like the walker does —
Boolean('false') displayed true beside a gated-off badge. (stable)
* fix(security): bump backend deps to close all open Trivy code-scanning alerts (stable)
- axios 1.16.0 -> 1.18.1 (GHSA-gcfj-64vw-6mp9 high + 10 medium advisories)
- sharp 0.34.3 -> 0.35.3 (GHSA-f88m-g3jw-g9cj, inherited libvips CVEs)
- mailparser 3.9.9 -> 3.9.14 (pulls linkify-it 5.0.2, CVE-2026-59887)
- brace-expansion override >=5.0.6 -> >=5.0.7 (CVE-2026-13149)
- body-parser 1.20.4 -> 1.20.6 via lockfile refresh (CVE-2026-12590)
* fix(images): migrate removed sharp failOnError option and enforce Node >=20.9 (stable)
sharp 0.35 drops the deprecated failOnError constructor option, so
recoverably corrupt images would start failing upload validation and
thumbnail generation; use the failOn: 'none' equivalent instead.
sharp 0.35 also requires Node >=20.9: declare it in engines and make
picpeak-setup.sh compare the full version instead of only the major,
so native installs on Node 20.3-20.8 upgrade instead of breaking.
* fix(setup): align the Node floor with the whole dependency tree and gate native updates (stable)
html-to-text@10 needs Node >=20.19 and the glob/minimatch family excludes
Node 21, so declare engines as ^20.19.0 || >=22 and enforce the same range
in picpeak-setup.sh. Also run install_nodejs at the start of
update_native_installation so existing native installs on an old Node get
upgraded before the service is stopped, instead of restarting broken.
* fix(setup): make the update-path Node gate actually work (stable)
--update dispatches before detect_os, so install_nodejs saw an empty
PACKAGE_MANAGER, matched no install branch, and reported success on the
old runtime. Detect the OS on demand and re-verify the installed version
afterwards, failing loudly (before the service is stopped) when the
runtime still misses the engines range, e.g. a Node 21 that package
managers refuse to downgrade.
* fix(security): read the password-complexity key the settings UI writes
The settings UI saves the admin's complexity choice as
security_password_complexity (useSettingsState.ts prefixes security_ to
password_complexity), but getPasswordComplexitySettings() queried
security_password_complexity_level — written by nothing — so the setting
was silently ignored and password validation always used the 'moderate'
default. Spotted in the filpgame fork (their main, 2026-07-14).
* fix(security): accept the Postgres json-column shape of the complexity value (codex review of #843)
On SQLite the TEXT column returns the JSON-stringified value
('"very_strong"'), but on Postgres (production default) setting_value
is a json column and arrives already decoded ('very_strong') — the bare
JSON.parse threw and the outer catch silently fell back to 'moderate'
again. Parse with fallback, mirroring getAppSetting's documented
pattern; test now covers both driver shapes + the empty-value default.
hero_logo_visible is nullable — null means "inherit the global
branding_logo_display_hero toggle" (#756, migration 152). But the create and
update validators used `.optional()` without `{ nullable: true }`, which only
skips `undefined`; an explicit `null` still ran `.isBoolean()` and failed with
HTTP 400 "Invalid value". Saving an event with `hero_logo_visible: null` (the
inherit state the frontend sends) was rejected on v3.45.2.
- Both routes: `body('hero_logo_visible').optional({ nullable: true }).isBoolean()`,
matching the already-correct `hero_logo_size` rule next to it.
- Create handler: guard on `!= null` instead of `!== undefined` so an explicit
null stores NULL (inherit) rather than being coerced to 0/false by
formatBoolean on SQLite. The update handler already did `=== null ? null`.
Left hero_logo_position on plain `.optional()` on purpose: its column is NOT
NULL (no inherit migration) and its handler always resolves to a concrete value
via `|| brandingDefaults`, so null is genuinely invalid there — allowing it
would trade the 400 for a 500.
Adds smoke tests: PUT accepts hero_logo_visible: null and stores NULL; a
non-boolean value is still rejected.
Production installs use docker-compose.production.yml (the README's documented
path, pinned GHCR images, no dev services), but the dashboard's update
instructions emitted bare `docker compose pull` / `up -d`. Bare `docker compose`
operates on docker-compose.yml — a different, build-based stack — so a
production user who followed the steps:
- never pulled/recreated their real containers (stayed on the old version,
e.g. stuck on 3.44.0 after "updating" to 3.45.2), and
- started the dev-only mailhog service that docker-compose.yml defines
(reported restart-looping).
The backend runs inside a container and can't stat the host's compose files, but
docker-compose.production.yml passes PICPEAK_RELEASE_CHANNEL into the backend env
and docker-compose.yml does not. detectEnvironment() now derives
isProductionCompose from it, and the Docker update steps prepend
`-f docker-compose.production.yml` when set. The non-production branch keeps the
bare commands but the warning now tells users to add `-f docker-compose.production.yml`
if they installed with it.
Also gates the mailhog service in docker-compose.yml behind a `dev` compose
profile so a plain `docker compose up -d` never starts it (opt in with
`docker compose --profile dev up -d`). Nothing depends on it (SMTP_HOST comes
from .env), so gating is safe. Verified: `docker compose config` lists mailhog
only with `--profile dev`; production compose is unchanged.
Adds unit tests for the production-vs-default command generation.
The legacy gallery router mounted at /api/events exposed create/list/update/
delete/extend guarded by adminAuth ALONE — no requirePermission, no
requireEventOwnership. adminAuth only checks the token is a valid type:'admin'
session, which every back-office role holds, down to read-only `viewer`. So any
non-super-admin account could:
- GET /api/events → every gallery's bcrypt password_hash, share_token, and
client name/email (the list handler selects * and mapEventForApi keeps
those columns),
- PUT /api/events/:id → reset any gallery's password (full takeover),
- DELETE /api/events/:id → delete any gallery,
all bypassing the per-photographer ownership isolation the canonical
/api/admin/events router enforces. Affects any instance with more than the
single super_admin.
Fix: remove the legacy router entirely (mount + require + src/routes/events.js).
It was a superseded duplicate of /api/admin/events and unused by the frontend
EXCEPT for one live route — POST /:id/extend (the "Extend expiration" UI action,
which hit /api/events/:id/extend via the api client's /api base). That route is
migrated to the canonical mount as POST /api/admin/events/:id/extend with the
same guards as every other gallery mutation (adminAuth + requirePermission
('events.edit') + requireEventOwnership), and the frontend is repointed to it.
Behaviour of the extend itself is unchanged (expires_at + reactivate).
Verified end-to-end on a booted instance: /api/events (all methods) now 404;
/api/admin/events/:id/extend returns 401 unauth, 200 for the owner, 403 for a
non-owning editor; the full login→create→extend flow works. Adds a regression
test pinning the router removal and the extend ownership check.
The req.admin.id fix activated reinjectCurrentAdmin(); hardening its preservation
logic (found across Codex review rounds of #811):
- MFA hijack: reinject wrote back only password_hash/is_active/
must_change_password, leaving a crafted backup's two_factor_* on the
operator's row — it could strip or replace their second factor. The email-
matched row is now updated with the operator's full AUTH set (login identity,
password, and all two_factor_* columns). Relationship/audit FKs (role_id,
created_by) are deliberately NOT forced from the snapshot: on a cross-instance
restore those pre-restore ids may be absent from the backup and would dangle
the FK (SQLite rolls back at commit); the restored row keeps its own valid
values.
- Cross-instance restore rollback / FK safety: reinject matched only by email,
so a backup shipping a different admin with the default `admin` username hit
UNIQUE(username) and rolled the whole restore back; email and username could
even collide on two different rows. Reconciliation is now non-destructive:
the email-matching row is updated in place (id preserved → restored FKs like
events.created_by stay valid); any different row holding the operator's
username is RENAMED, not deleted (deletion would fire ON DELETE actions /
dangle references); only when no row has the operator's email is a fresh row
inserted, with created_by nulled and an explicit max(id)+1 id (batchInsert
left the Postgres identity sequence unadvanced, so a sequence-based insert
could collide).
- Stale session after restore: admin_users ids shift on restore, but the
operator's live JWT is bound only to decoded.id (IP logged not enforced; the
backup controls password_changed_at). The route now revokes the token (result
checked and logged) and clears the admin cookie; the client redirects to a
fresh login via a sessionInvalidated flag. Cookie clear is the unconditional
guarantee.
Adds SQLite-backed reinject regression tests (in-place login/MFA restore with id
and FK columns preserved, username-only rename, email+username on different rows,
clean insert with created_by nulled) and the frontend redirect on
sessionInvalidated.
Deferred (design decisions / pre-existing, need a Postgres test env — see PR
discussion): global "invalidate all pre-restore sessions" cutoff; preserving the
operator's ROLE semantics across an RBAC-table replace; and resyncing Postgres
identity sequences after any restore (batchInsert leaves them behind max(id) —
pre-existing, affects every restored table).
The chunked video upload stored req.body.filename unmodified and later built
the merged path as path.join(tempDir, uploadMeta.filename). path.join does not
neutralise '../', so a filename like '../../uploads/logos/evil.svg' escaped the
temp dir on merge and overwrote arbitrary files. Requires admin with
photos.upload.
Fix: path.basename() the client filename in initializeUpload() and reject
names that collapse to nothing. Adds a regression test.
node-stream-zip's extract(null, root) writes each entry to path.join(root,
entry.name) without neutralising '../', so a crafted archive entry named
'../../uploads/logos/evil.svg' escaped the target dir and overwrote arbitrary
files (logos, .env, route files → RCE on source deploys). Requires admin with
archives.restore.
Adds assertZipEntriesWithin() to utils/safePath.js — a lexical containment
check run on the entry list BEFORE extract() — and guards both extract sinks:
adminArchives.js (the reported route) and picpeakImportService.js (the sibling
.picpeak import, same sink). Adds unit tests for traversal, absolute-path, and
sibling-prefix entries.
POST /auth/gallery/share-login validated only the 128-bit share token and then
minted a full type:'gallery' access token regardless of require_password —
computing requiresPassword at the end only to echo it, never enforce it. Anyone
holding a gallery's share link could read and download every photo in a
password-protected gallery via a direct API call, no password needed.
Fix: compute requiresPassword before minting; for a password-protected gallery
return { requires_password: true } with NO token and NO cookie. The client then
goes through /gallery/verify, which does bcrypt.compare the password. The public
(no-password) auto-login path is unchanged. The frontend already falls through
to the password prompt when share-login returns no token/event.
Adds route regression test covering the bypass, the public path, and bad tokens.
adminAuth populates req.admin, not req.user, so currentAdminId was always
undefined in the /api/admin/picpeak/import handler. reinjectCurrentAdmin()
then had no account to preserve and the admin_users table was fully replaced
by the uploaded backup — a crafted .picpeak let any admin with backup.restore
take over every admin account (critical). One-line fix: pass req.admin.id.
Closes GHSA-qxfx-4493-4v8f and its duplicate GHSA-pjp6-jcrj-3cr5.
The frontend image kept shipping vulnerable OS packages (nginx 1.28.3-r1,
curl/libcurl 8.19.0, c-ares 1.34.6) despite the apk upgrade line, for two
independent reasons:
1. The runtime stage's apk upgrade layer was cached indefinitely — the
CACHEBUST build-arg CI passes (github.run_number) was only declared in
the builder stage, and ARGs don't cross stage boundaries. Both
Dockerfiles now redeclare CACHEBUST in the runtime stage and consume it
in the apk RUN, so every build re-runs the upgrade and picks up current
Alpine security updates.
2. nginx itself can never upgrade via apk on the nginx.org-based image:
the bundled nginx-module-* packages pin the exact nginx version, so
Alpine's patched 1.28.3-r4 is unreachable (verified empirically —
apk add --upgrade nginx is a silent no-op). nginx fixes must come via
the base tag, so bump to nginx:1.30-alpine (current stable, 1.30.4 on
Alpine 3.24, same nginx.org conf.d layout — drop-in).
Verified: local image build scans clean with Trivy (0 OS findings, was 21);
container serves /health, SPA fallback, and BRAND_TITLE envsubst as non-root
nginx user.
Closes code-scanning alerts 371-374, 376-392 (nginx HTTP/2 & module CVEs,
curl CVE-2026-5773/-6276 + 6 medium, c-ares CVE-2026-33630).
- Search now hits the backend (debounced) so results aren't truncated to the
first loaded page: /received gains a `q` filter (sender/subject); the frontend
passes the debounced term to every list query. The instant client-side filter
stays for responsiveness.
- Reply/compose recipient extracts the bare address from a "Name <addr>" From
header (extractEmail) — also used for the customer-lookup key.
- Added the full de + en `messages.*` and `email.customerMailbox.*` translation
namespaces (were English inline-fallbacks only). Swiss-German spelling.
- BLOCKER: stored XSS via inbound sender display name. The reply stub built raw
HTML with the unsanitized From name and set it as innerHTML on the composer's
contentEditable (admin origin) → onerror JS ran on Reply. Now HTML-escape
from_address in the stub AND DOMPurify-sanitize the composer body before
innerHTML (defense in depth).
- Gate the NEW Messages routes with requireFeatureFlag('messaging') per-route
(queue/:id, received/:id, item/*, identities, accounts, accounts/test, send)
— NOT the shared /email mount, so the pre-existing email-config endpoints stay
ungated.
- DocumentActionModal auto-picks a customer only on an EXACT email match
(customer search is prefix/fuzzy), else leaves the picker to the admin.
- Search box in the header filters the current folder's list (sender/subject),
client-side; works across the merged Archived/Deleted views too.
- Archive and Delete are now implemented as soft moves: migration 157 adds
mailbox_state ('active'|'archived'|'deleted') to email_queue + received_emails.
Archive → 'archived', Delete → 'deleted' (trash). Restore → 'active'. Deleting
FROM the Deleted folder is permanent (hard row delete).
- New cross-account system folders Archived + Deleted (merge sent + received of
that state, sorted by date). Normal folders now exclude archived/deleted.
- Backend: /queue + /received gain a `state` filter (default active + legacy
NULL); new POST /item/:kind/:id/state (archive/delete/restore) and DELETE
/item/:kind/:id (purge, email.edit).
- Toolbar Archive/Delete wired; Restore + "Delete permanently" shown in the
system folders.
Frontend build + migration boot (157) verified.
Pre-upstream review hardening:
- /accounts + /accounts/test now reject private/internal IMAP/SMTP hosts via
isPrivateIP(), matching /config + /incoming-config (SSRF).
- QueueDetail body iframe uses sandbox="" (script-less, no same-origin) like the
inbound pane, instead of allow-same-origin.
- /send sanitizer drops the <style> tag + data: scheme to match the stricter
inbound sanitizeBody allowlist.
- Per-account SMTP transport sets tls.rejectUnauthorized explicitly.
