Backport of the two halves that landed on main, which together are what the
issue asked for: a per-build read cap plus a separate build-concurrency cap.
Per build: the builder opened one storage read per photo and handed each to
archiver, which drains them one at a time — so every read past the one being
written parked an S3 socket holding unread bytes. Nothing reclaimed them:
archiver's abort() does not touch source streams, and the SDK arms its socket
timeout on a 3s delay then clears it as soon as response headers land, so a
fast response never gets one. Reads are now capped at two and destroyed on
every exit, including the invalidated one, which previously cleaned up nothing
at all. Invalidation also cancels an in-flight build directly rather than
leaving a note for the loop, which matters once the loop can be parked waiting
for a slot a stalled archive will never free.
Across builds: invalidateAll() invalidates every event holding a cached zip and
each invalidate() arms its own debounce timer in the same tick, so they all
fired together and every one started building at once. Background rebuilds now
run two at a time. The cap is on that path only — a foreground generateZip,
where a guest is waiting on the download, is never queued behind a burst.
Two deliberate differences from the main twins. The read cap uses the shared
archiveStreamGuard helper this branch already has rather than main's inline
copy — same contract, less duplicated code. And main's stop() drain has no
counterpart here because this branch has no stop(), so that machinery is left
out rather than carried as dead code.
Relates to issue 1399
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* fix(gallery): let an admin preview a draft through its short share URL (stable)
Stable twin of the main-branch fix.
/resolve/:identifier filtered drafts out through ACTIVE_EVENT_FILTER and
/:slug/verify-token/:token repeated the filter inline, so with "use short
gallery URLs" on the admin's own View Gallery link answered "Gallery Not
Found" for an unpublished gallery. With the setting off the link carries the
slug, /info serves it, and the preview worked — which is why this looked like
a short-URL bug rather than a draft one.
The mechanism differs from main by branch: stable identifies an admin preview
by a signed admin JWT in ?preview=, so this uses isAdminPreview, the same
predicate /info already uses for its draft gate. Both routes now match /info
rather than being stricter than the branch they live on.
The draft lookup only runs after isAdminPreview accepts the caller, so the
published path keeps its single query and an unverified caller never learns
the draft exists. GHSA-rh8r is unchanged and pinned by test: a bare slug
lookup still never returns share_token.
Relates to issue 1386
* fix(gallery): carry the admin preview credential to the API on stable
External review found the backend half of the previous commit was unreachable:
`preview=` appeared in exactly two places in the whole frontend — building the
View Gallery link and reading the token — and nothing forwarded it into the API
calls the gallery page then makes. So the new /resolve fallback exited at its
guard for every real browser request, and the /info draft escape that has been
there all along was equally inert. Draft preview on this branch was broken for
both URL forms, not just short ones.
The request interceptor now forwards the credential as x-admin-preview, and
isAdminPreview accepts it there as well as in ?preview=. A header rather than a
query parameter because the credential is the admin's own session JWT, and
query strings reach nginx access logs, browser history and Referer headers.
?preview= stays accepted: the gallery PAGE url is what the browser navigates
to, and hand-built links rely on it.
The tests only exercised ?preview=, which the browser never sends on an API
call — so they passed while the feature stayed broken end to end. They now
cover the header transport across /resolve, /verify-token and /info.
Relates to issue 1386
* fix(gallery): authenticate the draft preview by the admin cookie on stable
The header transport in the previous commit could not work. `admin_token`
appears exactly once in this frontend — the read inside getPreviewToken() —
and nothing ever writes it: AdminAuthContext stores only admin_user and the
JWT lives in an HttpOnly cookie. So getPreviewToken() always returned null,
the View Gallery link was built as `?preview=` with an empty value, and every
transport downstream had nothing to carry. Draft preview on this branch has
never worked from the UI, by either URL form.
The machinery was already there: verifyGalleryAccess drops the is_draft
constraint for a preview in three places. Only delivery was missing.
isAdminPreview now also accepts `admin_preview=1` as an intent flag,
authenticated by the admin_token cookie the browser already sends. That fixes
every caller at once, including the native fetch() in AuthenticatedImage and
AuthenticatedVideo, which bypasses the axios interceptor entirely — without
the flag on the media URL a preview loaded its metadata and then showed no
thumbnails, hero or lightbox media at all. The flag alone authorizes nothing:
with no valid admin token the check fails closed.
`?preview=<jwt>` keeps working for hand-built links, but nothing emits it any
more, so the admin's own session JWT no longer travels in a query string where
nginx access logs, browser history and Referer headers can see it.
getPreviewToken() is deleted along with its now-orphaned import.
Relates to issue 1386
* fix(gallery): carry the preview flag on every non-axios gallery URL
Third review round found the flag still missing on the paths that never touch
the axios interceptor:
- PhotoLightbox renders VideoPlayer, which assigns the photo URL straight to
<video src>. Draft video playback 404'd. The previous commit had put the flag
in AuthenticatedVideo, which has no consumers on this branch at all — dead
code fixing nothing. Reverted; VideoPlayer carries it now, for both src and
poster.
- savePhotoToDevice builds a native anchor from api.getUri(), and
downloadAllPhotos uses a direct anchor when a zip is ready. Both downloads
404'd inside a preview.
The three call sites plus AuthenticatedImage now share utils/adminPreview.ts
rather than repeating the check. It refuses absolute URLs, and the flag is
applied while the URL is still relative — buildResourceUrl can turn it
absolute in split deployments, which would have dropped it silently.
Relates to issue 1386
* fix(gallery): authorize the admin preview against the event, not just the token
isAdminPreview verified the JWT signature and `type === 'admin'` and checked
nothing else — not that the account still exists, not that the token is
unrevoked, and not that this admin may see this event. verifyGalleryAccess
then dropped the is_draft constraint on that basis, so any valid admin token
previewed any draft gallery and its photos, including one created by a
different photographer and including an account whose role grants neither
events.view nor photos.view. main closes this through access.authorize; this
applies the same rule where this branch keeps its checks.
The predicate could not simply be tightened in place: it runs while the event
lookup is being shaped, before there is an event to authorize against. So it
splits in two. previewClaimed() stays synchronous and signature-only, and its
one legitimate use is deciding whether the lookup includes drafts.
verifyAdminPreview(req, event) then applies the real rules — revocation, an
active account, ownership (super_admin, ownerless, or own event) and
events.view + photos.view — and assertDraftPreviewAllowed gates every loaded
event behind it. Both query branches in verifyGalleryAccess converge on one
`if (!event)`, so two gates cover all three lookups.
Fails closed on a transient database fault in the revocation or permission
check, rather than treating an error as a pass. The roles-table fallback
mirrors adminAuth: an install predating that schema has admins but no role to
check, so ownership is the only gate that applies there.
The suite previously carried a test documenting the hole — "accepts any valid
admin token, matching /info on this branch". That is replaced by the three
cases it was standing in for: a non-owning admin, an admin with no gallery
permissions, and a deactivated account, each 404 now and 200 before.
Relates to issue 1411
* fix(gallery): close two gaps in the draft-preview authorization
Found by a fourth review round.
verify-token selected its own columns and omitted created_by, so
verifyAdminPreview saw an ownerless event and allowed any admin holding
events.view and photos.view — including one who does not own the draft, and
while /resolve and /info were correctly refusing them. The ownership check was
running; it just had nothing to check against.
savePhotoToDevice applied the preview flag to the output of api.getUri().
With an absolute VITE_API_URL that is an absolute URL, which withAdminPreview
refuses by design, so the flag was silently dropped and desktop and Android
preview downloads 404'd. Applied to the relative path before getUri expands it.
Relates to issue 1386
Relates to issue 1411
* fix(gallery): keep an admin draft preview out of the guest share-login flow (stable)
Twin of the main-branch fix. Making verify-token pass for a draft preview
opened a path that did not exist before it: the gallery bootstrap then called
shareLinkLogin, whose share lookup excludes drafts, so it 404'd and recorded a
failed login attempt against the caller's IP on the way out. Five preview opens
inside the attempt window locked share-link logins out for that IP — for real
guests too, and after publishing.
An admin preview needs no guest session: the admin cookie plus admin_preview=1
already authorizes every gallery call. The preview path loads the gallery
directly and never touches the login endpoint.
Relates to issue 1386
* test(gallery): move the preview revocation tests onto the new predicates
The revocation hardening that landed in the meantime shipped unit tests
against isAdminPreview, which this branch replaces with previewClaimed plus
verifyAdminPreview. They were asserting the old shape — including which where()
calls the event lookup makes — so they broke on the merge.
Rewritten against the contract that actually matters rather than the query
shape: previewClaimed is signature-only by design and deliberately does not
consult revocation, and verifyAdminPreview refuses a revoked token, fails
closed when the revocation store cannot be read, and refuses when there is no
event to authorize against. The end-to-end case is asserted through
verifyGalleryAccess: a revoked preview token widens the lookup and still gets
404 for the draft.
Found by CI, not locally — these live in backend/src/__tests__, a second test
root that the suites I had been running do not cover.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
Backport of the main-branch fix. Both guest-facing download routes append one
storage read per photo and hand them to archiver, which drains them one at a
time — so every read past the one being written parks an S3 socket holding
unread bytes, and nothing reclaims them. archiver's abort() does not touch its
source streams, and the SDK arms its socket timeout on a 3s delay then clears
it as soon as response headers land, so a fast response never gets one.
This is the mechanism behind the incident reported against the cached-zip
builder: pooled sockets held with unread bytes, uploads and gallery reads
starved behind them, a process restart the only way out. These two routes need
no admin credentials to reach — any gallery guest can trigger them, and closing
the tab mid-download was enough to strand every appended-but-undrained read.
utils/archiveStreamGuard caps reads in flight at 2 and destroys whatever is
still open on every exit, including the client disconnect. A cancelled download
returns without reaching finalize(), which would otherwise reject with ABORTED
and make the catch send JSON over a response whose ZIP headers had already
gone out. A read that dies while still queued is reported so the archive is
aborted rather than hanging when it reaches a dead stream.
downloadZipService still has the same pattern on this branch and is deliberately
untouched here — that is the cached-zip builder, whose fix is a separate PR on
main and not yet backported.
Local-filesystem installs are unaffected: they take archiver's file-path branch
and open no sockets.
Relates to issue 1399
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
Stable twin of the main-branch fix.
Once an event left `standard` protection, both halves of the video path were
routed through /api/secure-images, and neither half can carry a video: the
lightbox drops the emitted `{{token}}` template straight into a <video>
element and nothing substitutes the placeholder, while the secure-images route
pipes every byte through sharp, which throws on an mp4. The /photo/:photoId
route bounced to that same endpoint before reaching its own video branch —
isVideo was computed and then ignored — so there was no way through.
Videos now keep the JWT route at every protection level, on both sides. Not a
new exposure: thumbnails of those same videos have always been served from it,
and a valid gallery token is still required. Still images are unaffected and
keep bouncing to the secure endpoint.
VideoPlayer had no `error` listener, so all of this rendered as a poster
frozen at "0:00 / 0:00" behind a play button that did nothing —
indistinguishable from a codec the browser cannot decode, which is the other
common cause (HEVC/H.265 phone footage plays in Safari and nowhere else). It
now surfaces the failure and names the codec case.
The frontend half is identical to main; the backend half is hand-ported
because stable keeps these routes in the monolithic routes/gallery.js.
Relates to issue 1370
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* fix(backend): check the revocation store on gallery access
Gallery logout wrote to revoked_tokens correctly, but
verifyGalleryAccess never read it back — a logged-out gallery JWT
kept working until natural expiry. Admin auth already calls
isTokenRevoked(); gallery was the outlier.
Note: this gap was independently closed on main via a broader gallery-
access refactor (PR #1357), so no main-branch fix is needed there —
this is a stable-only backport of the same protection.
* fix(backend): check revocation on the admin-preview gallery token too
isAdminPreview() decoded the ?preview= admin JWT but never checked
isTokenRevoked — a revoked admin session kept working via a preview
link indefinitely. Same gap this branch already closed for the main
gallery-token path (GHSA-q7f7-gjx8-mf6h), just in the sibling
admin-preview check within the same file.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
The restore flow accepted an unvalidated database.backup_file from
the manifest (absolute paths and traversal both worked, and no
containment check enforced the configured backup root), then
interpolated it unescaped into a `sqlite3 .restore '<path>'` command,
letting an attacker-chosen source file replace the live database.
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* fix(backend): validate the S3 endpoint host before the restore download
downloadFileFromS3() built an S3StorageAdapter and called .download()
directly, skipping the isHostAllowed() private-IP/DNS-rebinding guard
that testConnection() applies elsewhere — an admin with backup.restore
could point the configured S3 endpoint at an internal/metadata address
for unauthenticated egress via the server.
Backport of 89c6a673bf from main
(GHSA-vm2x-c628-3cx5) to stable; cherry-picked cleanly, no adaptation
needed.
* fix(backend): pin the restore S3 download to its validated DNS resolution
isHostAllowed() was check-then-connect: the AWS SDK re-resolves the
endpoint hostname independently when it actually connects, so a DNS
rebinding condition between the preflight check and the real
connection could still reach a private/internal address.
Stable didn't yet have the pinnedRequestOptions() primitive main's
webhookDeliveryWorker.js uses for this, so it's ported here as
utils/pinnedRequest.js, plus an additive
validateExternalUrlWithAddresses() in networkValidation.js that
returns the resolved address set (existing exports/behavior
untouched). Both get wired into the S3Client's requestHandler for the
restore download path only.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
Stable backport of 9bca4046 (main) for GHSA-h4w8-57xq-53fx.
Admin password reset generated a ~2^21-entropy password from a small
wordlist (generateReadablePassword) instead of the already-available
generateSecurePassword(16), and must_change_password was written on
reset but never checked by any route-blocking logic — a reset admin
could keep using the old/weak password indefinitely since the flag
only ever reached the frontend as a response field.
adminAuth() (backend/src/middleware/auth.js) is adapted for stable's
inline admin-lookup query (main's equivalent goes through the
sessionAccess.admin() abstraction, which doesn't exist on this branch):
both the roles-join select and its missing-roles-table fallback select
now also fetch must_change_password, and a flagged admin gets 403
MUST_CHANGE_PASSWORD on every adminAuth-gated route except
/api/admin/auth/change-password and /api/admin/auth/logout (verified
against this branch's actual routes/adminAuth.js).
resetAdminPassword() (backend/src/services/userManagementService.js)
now calls generateSecurePassword(16), which already exists on stable
with the same signature as main. generateReadablePassword itself is
left untouched since it's still used by the separate gallery-password
reset path (routes/adminEvents/resets.js).
Frontend already renders MandatoryPasswordChangeModal off
user.mustChangePassword (AdminAuthContext/AdminLayout), so this is
purely a server-side backstop, same as on main.
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* fix(backend): require actor to hold every permission of a role they grant
Any admin with `users.edit` could grant an arbitrary non-super_admin
role — including one carrying far more permissions than they
themselves hold — via PUT /api/admin/users/:id. The role-change path
never called the existing assertActorMayGrant() guard that already
protects role create/edit.
* fix(backend): require actor to hold every permission of a role they grant
Backport of the same fix on main (#1378), adapted to stable's schema —
stable has no custom-role-creation service or roles.manage containment
helper yet, so assertActorMayGrant() is added locally in
userManagementService.js instead of reused from elsewhere.
Any admin with `users.edit` could grant an arbitrary non-super_admin
role — including one carrying far more permissions than they
themselves hold — via PUT /api/admin/users/:id.
* fix(backend): apply the same role-grant guard to admin invitations (stable)
Stable counterpart of the main-branch fix: createInvitation() only
blocked granting super_admin — the same users.create-holder-can-invite-
into-any-role escalation that updateAdminUser() was fixed for
(GHSA-rv8w-m6mx-7j4q) was still open via POST /admin/users/invite.
Reuses stable's local assertActorMayGrant().
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* fix(backend): validate event id before using it in the logo storage filename
The multer filename callback built the stored path directly from
req.params.id with no integer validation, letting a traversal payload
in the route param escape the intended uploads/logos/events/
directory — most directly reachable via a super_admin session, since
requireEventOwnership short-circuits with no DB lookup for that role.
* fix(backend): validate contract id before using it in the signed-PDF storage filename
Backport of the same fix on main: same pattern as the event-logo fix
(GHSA-9q5j-vqfw-32hr) in a different file this branch never touched —
multer's filename callback ran before express-validator's :id check,
letting a traversal payload escape uploads/contracts/signed/.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* fix(backend): validate business-profile logo uploads by content, not filename
The upload route skipped the shared validateFileType() helper every
sibling upload route uses, and derived the stored extension from the
client-supplied filename. A file could declare an image MIME type
while carrying an executable/HTML extension and arbitrary content,
then be served same-origin via the mass-assignable logoPath field.
* fix(backend): content-sniff business-profile logo uploads too
fileFilter paired the claimed MIME type against the extension but
never verified the actual bytes matched, unlike other upload routes
that already call validateFileContent(). Defense-in-depth: the
extension-confusion XSS itself was already closed (stored extension
is derived from the validated MIME, not client input), this closes
the remaining gap where declared-vs-actual content can still diverge.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* fix(backend): reject a replayed TOTP code within its validity window
verifyTotp() was stateless — otplib's window:1 tolerance meant the
same 6-digit code could complete two independent logins inside its
~90s validity window. Track each admin's last-consumed step and
reject a code that doesn't advance past it.
* fix(backend): make the TOTP replay-tracking persist atomic
Backport of the same fix on main: the persist for
two_factor_last_used_step is now a conditional UPDATE (only advances
the step, checked via affected-row count) instead of a plain
unconditional write, closing a TOCTOU race where two concurrent
requests carrying the same captured code could both pass before
either UPDATE landed.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
The unauthenticated payment-check magic link (intentional, matches
publicQuotes.js) had a 30-day token lifetime and wrote to the invoice
ledger silently. Shortened the TTL and added a best-effort admin
notification on every write via this route, so the no-login
convenience stays but an admin always sees the action happen.
(cherry picked from commit 262c3a4705)
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
Backport of 980a8642a2 from main
(GHSA-9h7q-2jpf-vj85). GET and POST for an event's short URLs both
required requireEventOwnership; DELETE only checked events.edit
permission, letting any admin holding that permission delete another
tenant's branded gallery short URL. Resolve the short URL's event
first, then apply the same ownership check the other routes use.
Adapted for stable: main's ownership guard uses a canAccessEvent(admin,
event) helper factored out of middleware/ownership.js that doesn't
exist on this older branch. Inlined the equivalent predicate directly
(the same "no owner, or admin owns it" check requireEventOwnership
itself uses here) instead of adding the helper.
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* fix(backend): bump sharp, nodemailer, multer, js-yaml, joi for security fixes
Backport of the same dependency bump on main (#1374). Resolves the
same 12 code-scanning alerts flagged on stable's backend deps: sharp
libheif RCE, nodemailer address-parser ReDoS + domain-validation
bypasses, multer upload DoS/race conditions, js-yaml parsing DoS, and
joi prototype pollution. All patch/minor bumps within the currently
used major version.
* fix(backend): set multer's fieldArrayIndexLimit to actually close CVE-2026-82333
The advisory is explicit that the 2.3.0 version bump alone doesn't
remediate the array-index DoS — an app must also set
limits.fieldArrayIndexLimit. Set it on every multer instance, sized to
what each route's form actually needs.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* fix(video): try metadata extraction and thumbnail generation independently
processUploadedVideo() gated everything behind isValidVideo(), which
rejects the whole video if ffprobe can't read even one of
duration/width/height -- common on some iPhone/Lightroom-exported
MP4s (issue 1370). Callers (photoProcessor.js's processPhoto and
processUploadedPhotos) already catch that throw and fall back to a
static placeholder thumbnail plus a metadata-only retry (codex
review of #845), but that fallback never got a REAL thumbnail even
when generateVideoThumbnail() would have succeeded on its own --
thumbnailing doesn't need valid duration/width/height, it just seeks
and grabs a frame.
processUploadedVideo now tries metadata extraction and thumbnail
generation independently, keeping whichever succeeds instead of
discarding both on a single failed field. The callers' existing
throw handling stays as a backstop.
Also: extractVideoMetadata stored duration as 0 (not null) whenever
ffprobe had no duration field, masking "unknown" as a fake real
zero-second clip and defeating downstream `duration != null` checks
meant to skip an untrustworthy value.
Relates to issue 1370
* fix(video): fall back to the SVG placeholder when thumbnail generation fails
processUploadedVideo could return success with thumbnailKey: null
when only thumbnail generation failed. The gallery grid
(GridGalleryLayout/JustifiedGalleryLayout) falls back to
`photo.thumbnail_url || photo.url` when there's no thumbnail, so
AuthenticatedImage downloaded the full original video and tried to
render it as an <img> -- a broken tile and a potentially huge
fetch just from opening the gallery.
Falls back to the same ffmpeg-free SVG placeholder the callers
already generate for a total processing failure, so a bare
thumbnail-generation failure degrades to that placeholder too,
never to "no thumbnail at all".
Found by codex review.
* fix(video): avoid a SQLite connection deadlock in the placeholder fallback
generateVideoPlaceholder() unconditionally called getThumbnailSettings(),
which queries the database directly (not through any active transaction).
videoProcessor.js's new placeholder fallback can run from inside
processUploadedPhotos' open per-file SQLite transaction (chunked video
upload) -- knex's default SQLite pool has exactly one connection, so
that second, un-transacted query deadlocks against the transaction
holding it, timing out after acquireConnectionTimeout (60s). Reproduced
directly against an isolated SQLite db.
generateVideoPlaceholder now skips the settings lookup entirely when
the caller supplies explicit width/height, and the video fallback
passes the same DEFAULT_THUMBNAIL_WIDTH/HEIGHT the settings lookup
would have fallen back to anyway (now exported for reuse).
Found by codex review.
* fix(video): throw when neither a real thumbnail nor the placeholder can be produced
processUploadedVideo returned success with thumbnailKey: null when
both the real thumbnail AND the SVG placeholder failed -- a total,
systemic failure (storage backend down, disk full), not a quirk of
one file. On stable, which doesn't have the #845 call-site fallback,
this silently completed the video with no thumbnail at all instead
of the retryable 'failed' status a throw here produces. On main,
the pre-existing #845 fallback already absorbed this exact case
(no behavior change there) -- verified against codex's own
git-blame check of the pre-PR stable code before applying this.
Now throws in that case, restoring the pre-existing "let the caller
mark it failed and retryable" behavior for a genuinely unrecoverable
video, while keeping every partial-failure case (the vast majority)
resolving with whatever succeeded.
Found by codex review.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* fix(backup): stop ignoring the configured database-backup destination path
databaseBackupService.getBackupConfig() returns the raw
database_backup_*-prefixed setting keys, but backup() and
startScheduledBackups() destructured unprefixed names off that
object (destinationPath, compress, enabled, schedule,
retentionDays, emailOnSuccess/Failure). None of those keys ever
existed on the config object, so every read silently fell through
to its hardcoded default.
The visible symptom (reported in issue 1365): the inline database
dump that runs before every file backup (default ON) always tried
to create /backup/database, regardless of what an admin configured,
and died with EACCES on the read-only default path — before the
file backup's own (correctly wired) backup_destination_path was
ever reached. The standalone scheduled database-backup runner had
the same bug: config.enabled was always undefined, so it silently
never started regardless of database_backup_enabled.
Also fixes saveManifestToLocal's manifest-directory fallback,
which hardcoded /backup instead of matching the sane
getStoragePath()/backups default used everywhere else for a
missing backup_destination_path.
Relates to issue 1365
* fix(backup): reject a database-backup destination inside a public static mount
Making database_backup_destination_path actually take effect
reopens a GHSA-jw8m-43r2-jqrm-class exfiltration path: that
setting is writable via PUT /api/admin/database-backup/config
under backup.create alone (the built-in admin role has it
without settings.edit or backup.restore), with no path
validation. Before this fix the setting was silently ignored
(the destructuring bug), so pointing it at the public
uploads/logos or fonts mount was harmless; now that it is
honored, it needed the same defense GHSA-jw8m already applies
to the per-request override.
Rejects the setting at both the config write (immediate 400)
and, defensively, at backup() time before mkdir.
Found by codex review.
* fix(backup): close two gaps codex round 2 found in the destination guard
- The public-roots list missed the bundled fallback fonts dir
(backend/assets/fonts, also mounted at /fonts, and nodejs-owned
per the Dockerfile's COPY --chown so it's writable at runtime).
- The comparison was case-sensitive; on a case-insensitive-but-
preserving filesystem (APFS, NTFS, Docker Desktop bind mounts of
either) STORAGE_PATH/UPLOADS/Logos names the same directory as
uploads/logos on disk. Now compares lowercased.
- database_backup_retention_days reached cleanupOldBackups
unvalidated. A value <= 0 pushes the cutoff to today or the
future, deleting every completed backup on the next scheduled
run -- a backup.create holder achieving what backup.delete
gates on the manual /cleanup route. Rejected at config-write
time (400) and defensively inside cleanupOldBackups itself.
- The scheduled-backup cron callback closed over retention_days
from schedule-start time; a retention-only /config update
(which doesn't restart the schedule) ran stale until restart.
Re-reads it on every tick instead.
Found by codex review, round 2.
* fix(backup): resolve symlinks and add the all-in-one frontend dir to the destination guard
Codex round 3 found two more bypasses of the public-root guard,
both specific to the all-in-one image (Dockerfile.aio):
- /app/frontend/dist (FRONTEND_DIR) ships nodejs-owned and is
served unauthenticated as the built SPA -- missing from the
protected-roots list.
- /app/storage is a symlink to /data/storage (the actual
STORAGE_PATH). A destination given as /app/storage/uploads/logos
passed the guard's lexical path.resolve() comparison while
resolving, on disk, to the exact same directory as the protected
STORAGE_PATH/uploads/logos. isUnderPubliclyServableRoot now
resolves symlinks in whatever prefix of each path already
exists (resolveRealish) before comparing, rather than relying on
path.resolve() alone.
Also restores three fs.mkdir spies in the test file that were
never un-spied, which silently leaked a rejected mock into any
later test doing a real fs.mkdir -- exactly what the new symlink
test needed to set up its fixture.
Found by codex review, round 3.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
Trivy flags the backend image on two sanitize-html advisories, both
fixed upstream:
- CVE-2026-63670 (fixed 2.17.6): a literal solidus after a raw-text end
tag (`</textarea/>`) is treated as text by htmlparser2 and re-emitted
unescaped, so disallowed markup passes when textarea or xmp is in
allowedTags.
- CVE-2026-84371 (fixed 2.17.7): an SVG SMIL animation whose
attributeName selects href lets the sibling values/from/to/by
attributes carry URLs past the scheme policy.
2.17.5 -> 2.17.7, exact pin as before. The new version brings its own
htmlparser2 12 / domhandler 6 / domutils 4 / dom-serializer 3 /
entities 8 tree under node_modules/sanitize-html; nothing else in the
lock moves.
That tree is ESM-only, so the backend now needs unflagged require(esm):
Node 20.19+ or 22.12+. The image is node:22-alpine and CI runs 22, but
engines.node still admitted 22.0-22.11, where require('sanitize-html')
throws ERR_REQUIRE_ESM at startup (publicSiteService loads it during
initialisation). engines is now ^20.19.0 || >=22.12.0 and the native
setup script's Node check enforces the same range instead of accepting
any 22.x. On the supported versions the sanitiser behaves identically
to 2.17.5 on the tracker and newsletter fixtures.
Jest 29's CommonJS registry cannot evaluate ESM either, so every suite
importing a route or service that uses the sanitiser would fail at
import. jest.config.js now maps `sanitize-html` to jest.sanitizeHtml.js,
which hands that one module to Node's real loader via
process.getBuiltinModule('module') — a plain require('module') inside
Jest is Jest's wrapper and returns an empty object for this package.
Verified against a real 2.17.7 install: the sanitiser suites and a
settings route suite pass; without the mapper they fail with "Cannot
use import statement outside a module".
Stable twin of the main commit: backend qs/body-parser, frontend axios,
dompurify, linkify-it and the transitive set npm audit fix resolves without
a major bump. sanitize-html 2.17.7 (ESM-only parser tree, Jest 29 cannot
load it; the advisory needs svg tags no sanitizer config allows) and the
tiptap / react-router majors are left out, as on main.
Settings help text for Allowed File Types (EN, DE). The reference page
lives in the docs repository (PicPeak/docs#18).
(cherry picked from commit f3b062a3, locale files only)
- 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
(cherry picked from commit 835312e8e6)
safeValidationErrors moves to utils/routeHelpers and replaces every
res.status(400).json({ errors: errors.array() }) in the routes, so no 400
body carries the submitted value any more (setup, customer auth and
customer change-password were still echoing rejected passwords).
Admin login, gallery verify, customer login/register/reset, customer
change-password and setup now cap username/slug at 255 and passwords at
MAX_PASSWORD_LENGTH at the validator, so an oversized value never reaches
the lockout lookup, bcrypt or the failed-attempt log.
Admin and customer login run one bcrypt compare on every path; the unknown
account branch used to return in microseconds against ~100ms for a wrong
password, which enumerated usernames despite the generic message.
(cherry picked from commit 40a8a9882a)
Stable port of the main commit; the admin-preview and maintenance-gate
items do not exist on this branch.
- 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
- 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
(cherry picked from commit 839bf4e4, adapted)
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.
(cherry picked from commit 063977d97d)
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.
(cherry picked from commit 3e46530072)
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.
(cherry picked from commit 0ca0e4a922)
Codex review round 2. The 400 I added in the previous commit returned
errors.array() verbatim, and express-validator puts the submitted `value` in
each error -- so rejecting an oversized password echoed that password back, and
re-allocated up to the 50mb body limit on an unauthenticated endpoint, partly
undoing the denial-of-service fix this branch exists for.
The same call appeared at seven sites in this file, five of which validate a
password field: /admin/login, /gallery/verify, /gallery/:slug/client-login,
/admin/change-password and /password-strength. Every failed login was returning
the attempted password in its response body, where it reaches proxy logs, error
monitoring and browser tooling. Fixed at all seven rather than only the one the
review pointed at.
Only `value` is dropped. `msg`, `path` and the rest are kept, because the two
shapes express-validator produces are both consumed in the frontend -- AcceptInvite
reads {field, message} from routeHelpers.validateRequest, EventDetails reads
{msg, path} from raw errors.array() -- and switching auth.js to the helper's
shape would have broken the latter for a reason unrelated to security.
1 more test. Backend suite: 2744 passed.
(cherry picked from commit 903e471753)
Codex review round 1 on the batch-1 security fixes. One finding is a
regression this branch introduced.
generateSecurePassword retried by recursing on any candidate validatePassword
rejected. The new 128-character cap makes EVERY candidate invalid once a caller
asks for more than that, so `generateSecurePassword({ length: 129 })` went from
returning a password to unbounded recursion and a stack overflow. It now
refuses an impossible length up front, and the retry is a bounded loop rather
than recursion -- every candidate failing is possible for reasons other than
bad luck (a charset that cannot satisfy the configured policy), and that case
deserves an error someone can act on rather than a blown stack. No caller in
the repo passes a length at all; the hazard was in the exported surface.
The route validators were decorative. POST /api/auth/password-strength never
called validationResult(), so the length bound I added only recorded an error
that nothing read: the oversized body still reached zxcvbn and the endpoint
still answered 200. The cap inside validatePassword() was doing all the work.
Errors are now returned as a 400 before the validator runs, which is what the
previous commit claimed.
Also awaited validatePasswordInContext, which is async. Unawaited, `validation`
was a Promise and every field in the response -- valid, score, errors, feedback
-- came back undefined. Pre-existing, in the lines this change already touches,
and it made the endpoint useless for the real-time validation it exists for.
1 more test. Backend suite: 2742 passed. The 23 eslint errors in server.js are
pre-existing and identical on main.
(cherry picked from commit 054cd6f82f)
Two findings from the GHSA-pwx6-5pqc-c5xq scan bundle, both verified against
the code and reproduced before fixing.
**Unauthenticated denial of service via password strength (csf_495d53fa).**
POST /api/auth/password-strength takes `body('password').notEmpty()` with no
upper bound, sits behind express.json({ limit: '50mb' }), and hands the string
to zxcvbn, whose matching is superlinear and runs synchronously on the event
loop. Measured on this codebase, in ms of blocked loop: 128 -> 41, 512 -> 1367,
1000 -> 5097, 5000 -> did not return in two minutes. One unauthenticated
request of about a kilobyte stops the whole process for five seconds; a few
kilobytes stops it indefinitely. The /api/auth rate limit does not help when a
single request is already enough.
The cap lives in validatePassword() so it covers every caller, present and
future; the route validator is defence in depth. 128 keeps the worst case at
the cost of an ordinary request while staying far above any real password --
bcrypt consumes only the first 72 bytes, so length past that adds no entropy to
the stored hash anyway. This is the only unauthenticated reach into zxcvbn:
setup is token-gated and self-closing, and acceptInvite/adminAuth use the
regex-only validator in passwordGenerator.
**The /photos and /thumbnails static mounts (csf_9aa6afe6, csf_559cd5cc,
csf_b14d462e, csf_547d26fa, and the gallery half of csf_34e420af).**
They served the raw originals and thumbnail trees behind photoAuth, which
authorises on a slug match. A static file server cannot apply per-photo rules,
so everything the gallery API decides was absent: allow_downloads, per-category
allow_downloads, watermarking, the resolution cap, reveal-mode windows,
visibility='hidden', download logging, and the customer-assignment re-check
that makes revocation immediate. photoAuth also bcrypt-compares an
x-gallery-password header per request with no limiter -- both rate-limit gates
return early for non-/api paths -- so the mount was an unmetered password
oracle. The filenames needed to drive all of this are handed to every guest in
the photos listing.
Nothing builds these URLs: no reference in frontend/src, none in the email
templates, and the only backend mentions are the /api/admin/photos/... API
routes and a maintenance-mode prefix list. The equivalent authorised routes are
/api/gallery/:slug/photo/:id and /thumbnail/:id. nginx still proxies the two
locations; they now 404, which is the intent.
**The /uploads mount (csf_1fc92f57).** It exposed the whole uploads/ root with
no auth middleware at all, and that root also holds signed contract PDFs
(uploads/contracts/signed) and client transfer files (uploads/transfers/<id>),
reachable by anyone who learned or guessed a filename. Narrowed to the two
public asset trees it exists for; contracts and transfers keep their own
authorised routes.
Removing the mounts leaves src/middleware/photoAuth.js unreferenced by
application code. Left in place deliberately -- deleting it and its tests is a
separate cleanup, and a smaller diff backports more safely.
Backend suite: 2742 passed.
(cherry picked from commit 14cd5eacb3)
Preventative twin of the main-side cleanup. This branch has no stray images
to remove — it just gets the same guard, so the two branches agree and a
backport cannot carry one across.
Two PR screenshots landed at main's repo root in #1241 and shipped as part of
the source tree. Screenshots belong on a `screenshots/*` branch, which is how
every other UI change here has attached its evidence.
Anchored with a leading slash so docs/ keeps its own images and test-assets/
keeps the fixtures the e2e specs load. Verified no tracked file on this branch
matches the new patterns.
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* fix(upload): let Android guests reach the camera without breaking video
Stable twin of #1244 (which replaces #1117). The reporter is on 3.46.1, so
this branch is where the bug is actually being hit.
Recent Android versions route an <input> whose accept list is entirely
image/video types to the system photo picker, which has no camera entry —
so a guest standing at the event can only pick an existing photo, not take
one. Including a type that picker can't handle forces the general chooser,
which does offer the camera.
Gated on the Android UA: iOS and desktop pickers behave correctly and would
only gain a selectable PDF that addFiles then rejects. No image-only guard
— #1117 added one that broke video uploads outright on any install
configured for them, and it was redundant anyway, since
extensionsToMimeTypes only emits types it has a mapping for and the
existing allowlist check already rejects a picked PDF.
The premise — that this actually surfaces the camera option on Android — is
taken at the reporter's description level and still needs confirmation on a
device.
Co-authored-by: Zszywany <Zszywany@users.noreply.github.com>
* fix(upload): use android/allowCamera instead of .pdf for the chooser fallback
Same mechanism, better token. Chrome on Android 14/15 sends an input whose
accept list is all media types to the photo picker, which has no camera tile;
adding a value that picker cannot satisfy makes it fall back to the general
chooser, which does offer the camera.
`.pdf` achieves that but advertises PDFs as selectable — pick one and the
existing allowlist check answers "Invalid file type", which is a dead end we
put in front of the guest ourselves. `android/allowCamera` is the token the
workaround converged on: not a real MIME type, matches no file, so it flips
the picker without offering anything.
Neither token ever widened what is accepted — addFiles validates against
extensionsToMimeTypes, which only emits types it has a mapping for — but not
showing the guest a choice that cannot work is worth the one-line change.
Verified in a browser rather than asserted: the real component rendered under
an Android UA emits
image/jpeg,image/png,image/webp,android/allowCamera
and under a desktop UA
image/jpeg,image/png,image/webp
with the visible modal identical in both, and the format hint still reading
"JPG, JPEG, PNG, WEBP" — the token does not leak into anything a guest sees.
* fix(upload): keep the camera token off Firefox for Android
External review round. The gate was a bare /Android/i, which Firefox for
Android matches — so it received a token invented to reroute Chromium's photo
picker, a picker it does not use. The doc comment two lines up already said
Firefox behaves correctly; the code did not agree with it.
Inert at best, and at worst it perturbs a chooser that was working. Narrowed
to Android minus Firefox, which is the Chromium-family set the behaviour was
actually observed on (Chrome and Edge, Android 14/15), with a UA test to pin
it.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
Co-authored-by: Zszywany <Zszywany@users.noreply.github.com>
Brings this branch in line with main, which fixed it in passing.
The archive restore inserted photos with a bare Date for uploaded_at. Inside
jest the sqlite3 binding's type dispatch misses sandbox-created Dates and
stores the literal string "[object Object]", so every restored photo got a
garbage timestamp. Verified on this branch rather than assumed:
bare Date -> "[object Object]"
toISOString -> "2026-09-01T06:48:41.915Z"
Production writes Dates as ms-numbers and is unaffected, which is exactly why
it survives unnoticed — it only corrupts what tests read back, so a future
test asserting on a restored photo's date would have believed it.
The regression test fails against the previous line.
Not touched: the category insert a few lines up has the same shape, but it is
identical on main, so fixing it here alone would re-open the divergence this
commit closes. Worth one small PR against both branches.
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
My doing, in #1247: I built that stable twin in a working tree that still
held untracked bytecode from main's ML sidecar, and a `git add -A` swept 16
.pyc files in alongside the two real ones.
Stable never carried the ignore rule because the sidecar itself is main-only
— which is precisely why nothing stopped it here. Added, so a shared working
tree cannot repeat it.
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
Stable twin of #1253. This branch has only the publish door —
send-gallery-email is #1235, main-only — so the same gap exists here in one
place rather than two.
/publish re-hashes password_hash from a plaintext the admin re-types in the
publish dialog, validated with nothing but express-validator's
isLength({min:6}). So the configured complexity — moderate by default,
meaning 8 characters plus upper, lower and a digit — governed event creation
and password reset while this door accepted 'aaaaaa' and made it the live
gallery password.
Not an escalation: it needs admin auth plus events.edit. It is a policy gap,
the admin UI advertising a complexity level this write path did not enforce.
BEHAVIOUR CHANGE: an API-only consumer publishing with a sub-policy password
now gets 400, with the same body shape event creation returns (error,
details, score, feedback).
3 tests, including that the rejection happens BEFORE the write — the gallery
keeps its old hash and stays a draft — and that a publish carrying no
password at all is untouched.
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* fix(archives): take the restored category from the manifest (#1240) (stable)
Stable twin of #1240. The reporter hit this on 3.46.7 — stable — with 596
photos restored and 0 categories, so this is the branch the bug was actually
found on.
Stable carries the manifest on both sides already: archiveService selects
`photo_categories.name as category_name` and serialises it, and the restore
route builds manifestByFilename. It just never read the category out of it,
deriving one from the ZIP's first path segment instead. Archives store photos
as they sit on disk, so an event whose photos live in the gallery root
produces a flat zip, no category resolves, and every photo comes back with
category_id null — silently, behind a 200.
Carries the whole of #1240, not a subset: the manifest-first resolution and
the shared resolveCategoryId from Marian's commit, plus the follow-up that
makes the manifest authoritative when it says "no category" — an entry with a
null category_name is a photo that was genuinely uncategorized, and falling
through to the directory contradicted the record being restored from. That
matters because the directory is not a category: entry names are the storage
key minus events/active/{slug}, so a real archive yields `individual/` and
`collages/`, and reading the first segment invents categories with those
names.
Re-verified on stable rather than assumed: all four tests pass here, and three
of them fail against stable's current route, with the legacy no-manifest
fallback passing either way. The two changed files are byte-identical to main.
Co-authored-by: Marian <lippitz.marian@yahoo.de>
* fix(archives): keep the stable twin to stable's schema, and close two category holes
External review on #1243 caught that this twin was ported wrong and that the
category resolver has two holes the main PR shares.
PORTED WRONG. I took main's whole adminArchives.js rather than applying the
category change to stable's, which dragged in main-only face cleanup:
photo_faces and event_people have migrations on main and none on stable, so
every permanent archive deletion would have thrown a missing-table error —
after the ZIP was already unlinked, leaving the event archived with its
archive gone and a 500 back. Rebuilt from stable's file with only the category
change; the diff against stable is now the fix and nothing else.
GLOBAL CATEGORIES WERE CLONED. Seeded categories (Ceremony, Reception) have
event_id NULL, so an event-only lookup missed them and created a second row —
and is_global defaults to TRUE, so that duplicate then appeared in every other
event's category list. The lookup now uses the same visibility rule the photo
routes use (own rows OR global), and anything it does create is explicitly
is_global false.
ORIGINAL-FILENAME ARCHIVES MATCHED NOTHING. With
general_use_original_filenames_for_downloads on at archive time, archiveService
names each ZIP entry after the original filename while the manifest stays keyed
by photos.filename — so the lookup missed every entry and those archives lost
categories exactly as before the fix. The manifest is now indexed by
original_filename as well, without letting it shadow a real filename key.
7 tests, three of them new; each new one fails against the un-fixed route and
the legacy no-manifest fallback passes throughout.
* fix(archives): sanitized original names and deterministic category scope
Round 2 of external review on #1243.
The original_filename index used the raw column, but archiveService runs the
name through sanitizeForZipEntry() before writing the entry — so an original
containing a slash or control byte was emitted under a different name than the
manifest records, and the lookup missed it. Both spellings are indexed now,
using the same helper the writer uses.
Not total, and the comment says so: uniquifyZipNames() appends `_1` when two
photos in one event share an original name, and that suffix cannot be
reconstructed from the manifest. Those fall through to the directory exactly
as they did before this fix — no worse, just not better. Closing it needs the
emitted name recorded at archive time, which is a writer change and a new
archive format.
The category lookup used one OR-query with .first(). An event-scoped category
and a global one may share a name — the category API permits it — so the
engine picked whichever, and a photo could be silently reassigned to the
global row, losing event-local settings like allow_downloads. Two queries now,
event-scoped first: the event's own row is the more specific answer.
9 tests, two new; both fail against the un-fixed route.
* fix(archives): don't adopt another event's legacy row, don't guess an alias
Round 3 of external review on #1243.
The global fallback matched on is_global alone. The very bug fixed here left
rows behind on upgraded instances — event-owned AND is_global true, because
the column defaults true — so restoring event B could adopt event A's
leftover, tying B's photos to a category that disappears when A is deleted.
The fallback now requires event_id IS NULL: genuinely global, not merely
flagged.
The original-filename alias map collapsed rows that share a basename.
archiveService treats `individual/IMG.jpg` and `collages/IMG.jpg` as distinct
paths and suffixes neither, so both manifest rows claimed one alias and
whichever won handed the other photo someone else's category. An alias claimed
by more than one row is now dropped and logged, so those photos fall back to
the directory: an unresolved category is recoverable, a confidently wrong one
is not.
11 tests, two new; both fail against the un-fixed route.
* fix(archives): make the manifest lookup order-independent and collision-safe
Two bugs found by an external review round, both in the manifest index.
The canonical map silently kept the last row for a duplicated
photos.filename. That column is not unique within an event — s3AutoImporter
takes path.basename(entry.key) and dedupes by path, so two imported files in
different subfolders both land as IMG_1234.jpg with different paths. At
restore both ZIP entries reduce to the same basename, so one photo got the
other's category. Contested names are dropped now, like ambiguous aliases
already were.
The alias pass could also evict a canonical key: when one row's
original_filename equalled another row's filename, the collision was marked
ambiguous and the sweep deleted the canonical entry. The comment two lines
above says a real filename key is authoritative and must never be
overwritten — the code did the opposite, and which way it went depended on
manifest iteration order, since the archive query has no ORDER BY.
Split into two passes so canonical names are claimed first and aliases only
fill names no canonical row wanted.
* fix(archives): treat a canonical/alias name clash as ambiguous, resolve categories lazily
Round-2 findings, one of which corrects my own round-1 fix.
Round 1 made a canonical filename outrank any alias. That is the wrong
tiebreak: when photo A's filename equals photo B's original_filename, which
file the ZIP actually emitted under that name depends on whether
original-filename archiving was on at archive time — with it ON the entry is
B's, with it OFF it is A's — and the manifest does not record the mode.
Preferring either silently mislabels the other half of the time, so the name
is dropped and both fall through to the directory. What the two-pass split
still buys is determinism: the archive query has no ORDER BY, so this used to
be a coin flip between dropping the name and overwriting it.
Categories are resolved inside the !existingPhoto branch. resolveCategoryId
find-or-CREATES, and archiveEvent retains photo rows, so restoring an archive
whose rows still exist created a category from the stale manifest name that
nothing then used — renaming a category while its event was archived left the
old name behind as an empty duplicate.
Not fixed: two event-scoped categories may share a display name with distinct
slugs, and the .first() lookup then picks either row, so manifest entries from
both collapse onto one id and can inherit the wrong allow_downloads. Detecting
it is easy; resolving it correctly needs a stable category identifier in the
manifest, which is a writer change and an archive-format bump.
* fix(archives): make a duplicate category name deterministic, and log it
Round-2 finding. Two event-scoped categories may share a display name when
their slugs differ, and the .first() lookup then picked one arbitrarily —
manifest entries for both collapsed onto a single id and half the photos
inherited the wrong per-category settings, allow_downloads above all.
Fixing it properly needs a stable category identifier in the manifest: a
writer change, an archive-format bump, and no help at all for archives
already written. Not worth building before knowing it happens. So the
collision is surfaced instead — a warning naming the category and the row
count — and the tiebreak is made deterministic (lowest id) so at least a
re-run lands the same way twice.
If this never fires in real logs, the format change was not worth making. If
it does, this is the evidence for it.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
Co-authored-by: Marian <lippitz.marian@yahoo.de>
* fix(gallery): route single-photo downloads through the storage backend
Stable twin of #1048. The route resolved a local filesystem path
unconditionally and handed it to res.sendFile. On an S3/R2 deployment
managed photos are never on local disk, so every per-photo download failed
— while download-all and secure-images worked, because they already went
through getStorage(). That asymmetry is why it went unnoticed: the gallery
looks healthy until a guest clicks the download button on one photo.
Because sendFile is called WITH a callback, Express does not send a
response when the file is missing and the callback only logs — the request
does not 404, it hangs until the client gives up. The new tests pin this:
all five backend-path cases time out against the current implementation.
- watermark branch: materialize a tmp local copy via withLocalCopy in S3
mode and hand applyWatermark the copy's PATH, so its path-keyed cache
still applies. Same pattern the zip builders in this file already use.
- pass-through branch: local disk keeps res.sendFile, which emits
Content-Length, Accept-Ranges, ETag and Last-Modified and answers Range
with a 206. Sharing one bare stream.pipe(res) with S3 would silently drop
all of it, and a resumed download would append a second full body onto
the partial file. On S3 the parts that matter are reproduced via stat()
and getRange().
- external/reference photos keep the local-path fallback unchanged —
resolvePhotoStorageKey returns null for them.
Ranges are parsed defensively: an unchecked parse yields NaN bounds and a
206 with a nonsense Content-Range, which corrupts a resumed download rather
than failing it. Malformed or unsatisfiable ranges fall back to a 200.
Written against stable's shape rather than cherry-picked — main's version
delegates to renderPhotoForDownload (#858), which does not exist here.
Co-authored-by: Peifu Mo <peipeimo@users.noreply.github.com>
* fix(gallery): open the stream before staging download headers, honour If-Range
Both from an external review round on #1048.
stat() succeeding does not mean get() will — a concurrent delete or replace,
or a transient backend error, lands between them. The fetch was awaited
AFTER the headers went out, so the range branch had already called
writeHead(206) and the outer catch could only throw ERR_HTTP_HEADERS_SENT
(in practice the request hangs), while the full branch would have sent its
500 JSON underneath the staged image/jpeg attachment headers.
Opening the stream first also lets a vanished object answer 404 and a
transient failure answer 500.
If-Range: emitting Last-Modified without honouring the validator built from
it is the dangerous half. A client resuming after the object was replaced
would get 206 from the NEW bytes and splice two versions into one corrupt
file. A non-matching validator now falls back to a full 200.
* fix(gallery): HEAD without egress, classify render failures, stage 206 headers
Round-2 findings from the external reviewer on #1048, ported.
Express routes HEAD through this GET handler and Node discards the body, but
the pipe still drains the whole object out of S3 first — a metadata probe
cost a full transfer in egress and latency. Everything a HEAD needs is
already in stat().
The watermark branch reported every failure as 404. It can equally fail
because getToFile timed out, tmp filled up, or sharp died; calling that
"photo not found" misleads the guest and hides the incident.
The 206 path uses status()+set() instead of writeHead(), which commits
immediately and left a stream erroring at byte zero with no outcome but a
destroyed connection. Staged headers flush on first write, so that case now
returns a clean retryable status.
pipeStreamToResponse also cleared Content-Type, Content-Length, ETag and
Content-Disposition but not the range headers, so the 500 went out still
advertising Content-Range — telling a resuming client the error body IS the
partial content.
* fix(gallery): answer HEAD before the counters
Round-3 finding on #1048, ported. The HEAD short-circuit was inside the
storage branch, below both the download_count increment / access_logs insert
and the watermark path — so a download manager's metadata probe counted as a
real download, and on a watermarked gallery it also pulled the original from
S3 and ran sharp over it to build a body Node then discards.
HEAD now leaves right after the access checks. Content-Length is included
only when the photo ships untransformed and the size is readable from stat();
a watermark changes the length and the only way to learn it is to do the work
this branch exists to avoid.
Uses stable's inline watermark resolution — resolveWatermarkSettings comes
from downloadRendition (#858), which does not exist on this branch.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
Co-authored-by: Peifu Mo <peipeimo@users.noreply.github.com>
* fix(events): delete stored objects when cascading an event delete
Stable twin of #1051. Deleting an event removed its database rows but left
every stored file behind: deleteEventCascade() cleaned up with fs.rm over
{STORAGE_PATH}/events/{active,archived}/{slug}, and on an S3-compatible
backend those paths don't exist locally — the call succeeds against nothing
and the real objects stay in the bucket, unreferenced by any row, invisible
in the UI, and billed every month.
Measured on a v3.45.16 install against Cloudflare R2, deleting one
403-photo event: bucket object count 5,400 before and 5,400 after, while
referenced rows dropped from 3,425 to 2,746.
Keys are collected BEFORE the transaction removes the photo rows — once
they are gone nothing records which objects belonged to the event, and only
a full-bucket audit against the whole database could find them again — and
deleted AFTER the commit, so a rolled-back delete can never destroy files
for an event that still exists.
Includes photo.watermark_path and event.archive_path, both storage-backed
and both previously fs.unlink-only. event.hero_logo_path is deliberately
excluded: multer writes logos to local disk with diskStorage regardless of
backend, so they are never bucket objects.
Reference/external photos are left alone — resolvePhotoStorageKey returns
null for them and PicPeak does not own those bytes.
This branch carries the higher priority of the pair: unlike main, stable's
deleteEventCascade never calls getStorage() at all, and the leak costs real
money for every month it goes unfixed.
Co-authored-by: Peifu Mo <peipeimo@users.noreply.github.com>
* fix(events): sweep the Download All cache, and delete objects concurrently
Both from an external review round on #1051.
The pre-built "Download All" zip (events.download_zip_path) lives under
events/active/{slug}/.download-cache/. On local disk the recursive fs.rm
already covered it, which is exactly why it was easy to miss — on S3 that
prefix is not a directory, nothing covered it, and it is gallery-sized.
downloadZipService exposes a cleanup() documented as "used on event
deletion" that the cascade never called.
download_jobs (main's #173) does not exist on this branch, so the
per-job archives main also sweeps have no counterpart here.
Deletes now run through a bounded pool instead of one await per key: a
400-photo gallery owns well over a thousand objects, and that many
sequential DeleteObject round trips runs to minutes — long enough for a
proxy to time the request out AFTER the commit, leaving the event deleted
and the sweep half-finished.
* fix(events): never delete a derivative another gallery still uses
Round-2 findings from the external reviewer on #1051, ported.
Canonical thumbnail/hero/preview keys are not event-scoped: the basename is
the photo's filename, and filenames are not unique across events. A legacy
gallery can share a canonical derivative with a photo in another event, and
deleting it here blanked a surviving gallery's tile. Derived keys are now
checked against photos outside this event and anything still referenced is
left alone; if the check itself fails, every derivative is kept — an orphan
costs storage, a deleted derivative costs someone else's gallery. Originals
need no check, their keys embed the slug.
Also cancel any in-flight or debounced Download All build before snapshotting
paths, via downloadZipService.cleanup() — the service's own entry point for
event deletion. A builder that started before the delete would otherwise
upload a gallery-sized zip after the sweep and write its path onto a row that
no longer exists.
* revert(events): drop the Download All build cancellation
It broke CI on this branch: the backend job went from ~2 minutes to
exceeding its 10-minute budget, twice, reproducibly.
downloadZipService.cleanup() reaches getStorage() through _cleanup(), and in
a suite where the S3 backend is configured but unreachable every cascade
delete then pays the adapter's retry backoff. The full suite passes locally
against SQLite, which is why this only showed up in CI.
The race it addressed is real but narrow — a builder that started before the
delete uploads its zip after the sweep and writes the path onto a row that
no longer exists, orphaning one object. That is a cheaper problem than an
unrunnable test suite, so it goes back to being a documented follow-up
rather than shipping behind a timeout.
The shared-derivative guard from the same review round stays: that one
prevented deleting a surviving gallery's thumbnail.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
Co-authored-by: Peifu Mo <peipeimo@users.noreply.github.com>
Stable twin of #1050. passwordValidation.js is byte-identical between the
branches, so this is the same change verbatim.
validatePassword() appended zxcvbn's feedback.suggestions to the errors
array unconditionally, and validity is errors.length === 0 — so any
password that merely earned a suggestion was rejected even when it
satisfied every configured rule. The effective policy was stricter than
the configured complexity level and invisible to the admin.
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
Co-authored-by: Peifu Mo <peipeimo@users.noreply.github.com>
Stable twin of #1050. passwordValidation.js is byte-identical between the
branches, so this is the same change verbatim.
validatePassword() appended zxcvbn's feedback.suggestions to the errors
array unconditionally, and validity is errors.length === 0 — so any
password that merely earned a suggestion was rejected even when it
satisfied every configured rule. The effective policy was stricter than
the configured complexity level and invisible to the admin.
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
Co-authored-by: Peifu Mo <peipeimo@users.noreply.github.com>
Stable twin of the same fix on main.
The dropdown rendered as `value="0"` and adminPhotos.js:1001 skips '0', so no
category condition was applied and the whole event came back. The branch that
does the work sits four lines below, keyed on the literal 'uncategorized' that
nothing was sending.
Silent by nature — a full list reads as 'nothing to narrow' rather than 'the
filter did not run' — which is why it survived this long.
The reporter in #1209 is on 3.46.4, so this is the branch that reaches them.
Tests both ends of the contract, since the bug was the pairing rather than
either half.
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
Stable twin of #1204.
The capture-date backfill committed its result keyed on the row id alone. It
snapshots every candidate up front, then walks them one at a time reading
originals off S3 or a NAS mount — a pass that can run for many minutes.
replacePhoto, reachable from the replace_by_name upload path, swaps a NEW file
under an existing row and rewrites path/filename. A replacement landing inside
that window carries no date of its own, so captured_at was still NULL, the
whereNull guard passed, and the previous file's EXIF date was written onto the
new photo. Silent: nothing errored, the run reported it as a success, and the
gallery just sorted that photo to the wrong place.
Fenced on path and filename as well as the id, so a replaced row matches zero
rows and is skipped. The candidate query already selects both columns, so no
query change. Knex renders a null value in the object form as `is null` on both
the pg and sqlite3 clients, so a row with a NULL path still matches itself.
Those skipped candidates are now counted rather than dropped. replacePhoto is
not the only writer of path/filename — eventRenameService rewrites both on an
event rename, which is not a content change — and another writer filling
captured_at first lands in the same place. Without a counter they fell out of
the run's arithmetic entirely: success + noExif + failed no longer added up to
the count the operator was shown when they started the job.
The card shows the count only when it is non-zero, and states what is known —
changed by something else, not updated — rather than promising a retry: for the
already-dated case there is nothing to retry, and the Missing Capture Date
figure above is what says whether work is left. Locale coverage: en, de, fr, sl,
with the defaultValue carrying the rest.
Regression test: a replacement landing mid-run leaves captured_at NULL and is
not counted as updated. Verified to fail against the unfenced code on this
branch.
Stable twin of #1194.
generateThumbnail, generateHeroImage and generatePreviewImage went straight
from sharp(imagePath) to .resize(), so a photo whose Orientation tag is not 1 —
routine for portrait shots on bodies that tag rather than rotate the sensor
data — was resized from the raw frame and came out sideways. The same pipelines
then call .withMetadata(false), stripping the tag from the output, so nothing
downstream could correct it either. The download path already had this right,
which is why the same photo looked correct on download and rotated in the
gallery.
watermarkService had it too, and it is the one a guest actually sees:
gallery.js serves photos.watermark_path ahead of the original when branding
watermarking is on. Two details there — metadata() is read from a separate
unrotated handle, because .rotate() does not change what it reports and every
use of those numbers is positioning; and the composite offsets are floored,
because getPositionCoordinates returns fractional pixels, sharp rejects them,
and applyWatermark catches its own error and silently returns the image
unwatermarked.
The rotate is unconditional in the thumbnail and hero generators — neither
passes `animated: true`, so both already flatten a multi-frame source and
guarding there would protect an animation that was being discarded anyway.
generatePreviewImage keeps the guard, since it genuinely does preserve
animation.
photos.width/height were stored from sharp's metadata, which reports pixels as
STORED, not displayed. For orientation 5-8 those are swapped, so a portrait
photo landed in the database as landscape and masonry sized its tile with the
wrong aspect ratio on top of the image being unrotated. A shared
orientedDimensions() helper now does the conversion at all eight image ingest
sites. The video path is deliberately untouched: its dimensions come from
ffprobe, where EXIF orientation does not apply.
Existing rows keep their pre-rotation dimensions until reprocessed; the backfill
for those is #1199 on main and is not ported here yet.
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* fix(admin): make "Storage used" report storage used (#1164)
Stable twin of #1170.
The tile summed photos.size_bytes — the catalogued size of the ORIGINALS,
which in reference mode live on external storage and have no relationship to
the disk PicPeak runs on. The reporter's tile read ~80 GB against 21 GB of
real usage. Worse than the label: the same number drove the soft-limit warning
bar and, via /storage/info, the recommended soft limit — so a reference-mode
install got a disk-capacity recommendation computed from bytes that are not on
the disk.
- new localStorageUsage service walks the storage root and reports the total
plus a breakdown. Walking rather than summing DB columns is the point:
thumbnail/preview/hero rows record a key and never a byte count, and orphans
from a deleted event or an interrupted import are real bytes.
- the external media root is excluded when it sits inside the storage root.
Its compose default is <storage>/external-media, where the NAS is
bind-mounted — a plain directory, not a symlink — so walking it would put
every referenced original back into a figure whose purpose is to leave them
out. Symlinks are not followed either.
- .download-cache gets its own line: it lives inside the event directory, so
the naive rule files a multi-GB zip as photography.
- concurrent cold-cache callers share one walk; the dashboard, /storage/info
and the sidebar are routinely requested together.
- S3 installs keep the catalogued figure and the walk is skipped before it
runs, since the objects are in the bucket and STORAGE_PATH holds only
incidental local files.
- an absent measurement reads as "unavailable" and a partial one is marked
`+` across the dashboard, analytics, sidebar and status tab — a floor
silently compared against a soft limit reads as "safely under".
Verified on this branch: 11 new service tests, dashboardScope updated for the
changed contract, full suite leaves the same 5 pre-existing failures as
origin/stable. Frontend 20 files / 104 tests, tsc clean.
* fix(admin): tell "no disk to measure" apart from "the measurement failed" (#1164)
External review found both of these on this branch.
Both were reported as `storage_measurement: 'catalog'`, so a failed local walk
made the dashboard claim the objects live in S3. They are different things —
one is a fact about the install, the other is a fault — and there is now an
`unavailable` state for the second.
The analytics percentage could reach the billions. `safeSoftLimit` fell back to
`storageUsed || 1`, and on S3 that is null → 1, while the figure beside it came
from `catalogedBytes`. An editor or viewer holds `analytics.view` but not
`settings.view`, so `/storage/info` 403s for them and `storageInfo` is
undefined — which is exactly when that fallback fires. It now falls back to the
measured figure, and suppresses the percentage entirely when there is no real
limit rather than dividing usage by itself and always reading 100%.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* fix(gallery): stop the lightbox loading originals to display a photo (#1166)
Stable twin of #1169.
The lightbox read preview_url, which the server only emits once an admin has
flipped lightbox_preview_enabled — off by default. So a stock install fell
straight through to url, the untouched original: a reporter measured 16.5 MB
for a photo whose preview is 345 KB. The lightbox renders its neighbours too,
so opening one photo pulled three originals.
slideshow_url is the same /preview/:id URL, watermark query included, and has
been emitted unconditionally for images since #1015. Preferring it fixes every
existing install with no migration and no admin action.
Two other surfaces bypass PhotoLightbox entirely and had the same bug:
- premium galleries build their own slides with `src: photo.url`. Fixing that
also required carrying the photo id on the slide, because the download
handler recovered the photo by matching slide.src against photo.url — a
derivative src would have made Download a silent no-op.
- the Story layout rendered the full original as its GRID TILE, at
object-cover in a small card, and its hero rendered one as a full-bleed
background when hero_url exists for exactly that. Cards now use the preview
tier (not the thumbnail: thumbnail_fit is seeded to 'cover', so a thumbnail
would be cropped a second time and reframe every photo) and only load once
within 200px of the viewport, since every card mounts at page load.
GIF, APNG and PNG keep the original: generatePreviewImage encodes JPEG, which
has neither a second frame nor an alpha channel. The backend fix that removes
this list is the next commit in this stack.
Divergence from the main twin: no responsive `?w=` tiers. #1095 is main-only,
so `lightboxImageUrl` here selects a URL and nothing more. It lives in
`imageTiers.ts` under the same path main uses, so that backporting #1095 later
merges into this file rather than landing beside it.
Verified on this branch: 8 new tests; frontend suite 21 files / 113 tests,
tsc clean.
* fix(gallery): make the Story hero fix actually work on external galleries (#1166)
External review. Same two fixes as the main twin.
hero_url was inert for external media. ensureHeroImage only ever called
resolvePhotoStorageKey, which returns null for external/reference photos by
design — and that null was handed straight to withLocalCopy, which throws, so
the hero route caught it and redirected to the full ORIGINAL. #1078 fixed
exactly this shape for ensurePreviewImage and nobody carried it across. It
stayed invisible until this PR pointed the Story hero at hero_url: on a
managed gallery that is a real saving, on a reference-mode gallery it quietly
changed nothing.
Needed one extra piece here that main already had: generateHeroImage on this
branch ignores outputBasename and always derives the key from the source
basename, so two events referencing the same NAS filename would clobber each
other's hero. It now honours the option, matching generateThumbnail and
generatePreviewImage.
The format bypass trusted mime_type, which is not trustworthy: migration 039
backfilled every pre-existing photo to image/jpeg regardless of what it was,
and adminExternalMedia inserts rows with no mime_type at all — so a
mislabelled PNG sailed past the guard and came back flattened. It now checks
the filename extension as well.
* fix(previews): preserve alpha and animation in the preview tier
Stable twin of #1171. Stacked on the #1166 twin, whose format bypass this
removes.
generatePreviewImage encoded JPEG unconditionally. JPEG has no alpha channel
and no second frame, so a transparent PNG came back flattened onto a solid
background and an animated GIF came back as its first frame — for every
consumer of this tier, not just the lightbox. It was only invisible by default
because the lightbox served originals.
Sources with alpha, or more than one page, are now encoded as WebP, which
carries both and is still far smaller than the original. Ordinary photos stay
JPEG.
- the output extension matches what was written. A PNG source previously
produced `preview_foo.png` holding JPEG bytes; harmless while the route
hard-coded image/jpeg, wrong once the encoding varies. Existing keys keep
working — they are still JPEG and still served as such.
- the preview route derives Content-Type from the key. With nosniff set,
mislabelling would show a broken image rather than being silently corrected.
The watermark branch re-encodes to JPEG and now says so.
The frontend guess-by-MIME goes away entirely, including the case it could
never get right: a still and an animated WebP declare the same type.
Divergence from the main twin: no width-tier case. The responsive `?w=`
renditions (#1095) are main-only, so this branch has a single canonical
preview per photo.
Verified on this branch: 5 new backend tests against real Sharp output;
frontend 21 files / 114 tests; full backend suite leaves the same 5
pre-existing failures as origin/stable.
* fix(previews): retire the legacy preview keys, and stop mislabelling watermarked ones
External review. Same two defects as the main twin.
Legacy keys collide with the new naming. The old generator kept the SOURCE
basename verbatim while always writing JPEG, so a `.webp` upload produced
`previews/preview_shot.webp` holding a JPEG. The claim that pre-existing keys
have no .webp suffix was simply wrong. The route now derives Content-Type from
the key and the response carries nosniff, so every photo uploaded as WebP
would have rendered as a broken image in the lightbox. Legacy `.png` keys are
wrong the other way: flattened JPEGs of what may have been transparent
sources, which isPreviewValid would have let stand forever.
Migration 178 clears photos.preview_path outright — all of it, not just the
suspicious extensions, because a `.jpg` key can equally be a flattened
rendition and nothing in the key says so. Previews regenerate lazily on next
view under the new encoder.
The watermark branch mislabelled its output. applyWatermark PRESERVES the
source format on this branch too (watermarkService.js: png stays png, webp
stays webp), and its input is the preview — so the output already matches the
key the header was derived from. Forcing image/jpeg mislabelled every
watermarked WebP preview, and nosniff means the browser would not correct it.
Numbered 178, not 176: this stack does not carry the external-media
migrations, but that stack takes 176 and 177 on this same branch, and two
files sharing a numeric prefix would be confusing even though both would run.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* fix(gallery): stop the lightbox loading originals to display a photo (#1166)
Stable twin of #1169.
The lightbox read preview_url, which the server only emits once an admin has
flipped lightbox_preview_enabled — off by default. So a stock install fell
straight through to url, the untouched original: a reporter measured 16.5 MB
for a photo whose preview is 345 KB. The lightbox renders its neighbours too,
so opening one photo pulled three originals.
slideshow_url is the same /preview/:id URL, watermark query included, and has
been emitted unconditionally for images since #1015. Preferring it fixes every
existing install with no migration and no admin action.
Two other surfaces bypass PhotoLightbox entirely and had the same bug:
- premium galleries build their own slides with `src: photo.url`. Fixing that
also required carrying the photo id on the slide, because the download
handler recovered the photo by matching slide.src against photo.url — a
derivative src would have made Download a silent no-op.
- the Story layout rendered the full original as its GRID TILE, at
object-cover in a small card, and its hero rendered one as a full-bleed
background when hero_url exists for exactly that. Cards now use the preview
tier (not the thumbnail: thumbnail_fit is seeded to 'cover', so a thumbnail
would be cropped a second time and reframe every photo) and only load once
within 200px of the viewport, since every card mounts at page load.
GIF, APNG and PNG keep the original: generatePreviewImage encodes JPEG, which
has neither a second frame nor an alpha channel. The backend fix that removes
this list is the next commit in this stack.
Divergence from the main twin: no responsive `?w=` tiers. #1095 is main-only,
so `lightboxImageUrl` here selects a URL and nothing more. It lives in
`imageTiers.ts` under the same path main uses, so that backporting #1095 later
merges into this file rather than landing beside it.
Verified on this branch: 8 new tests; frontend suite 21 files / 113 tests,
tsc clean.
* fix(gallery): make the Story hero fix actually work on external galleries (#1166)
External review. Same two fixes as the main twin.
hero_url was inert for external media. ensureHeroImage only ever called
resolvePhotoStorageKey, which returns null for external/reference photos by
design — and that null was handed straight to withLocalCopy, which throws, so
the hero route caught it and redirected to the full ORIGINAL. #1078 fixed
exactly this shape for ensurePreviewImage and nobody carried it across. It
stayed invisible until this PR pointed the Story hero at hero_url: on a
managed gallery that is a real saving, on a reference-mode gallery it quietly
changed nothing.
Needed one extra piece here that main already had: generateHeroImage on this
branch ignores outputBasename and always derives the key from the source
basename, so two events referencing the same NAS filename would clobber each
other's hero. It now honours the option, matching generateThumbnail and
generatePreviewImage.
The format bypass trusted mime_type, which is not trustworthy: migration 039
backfilled every pre-existing photo to image/jpeg regardless of what it was,
and adminExternalMedia inserts rows with no mime_type at all — so a
mislabelled PNG sailed past the guard and came back flattened. It now checks
the filename extension as well.
* test(gallery): the hero fixture follows the root-relative relpath contract (#1166)
Same fix as the main twin: external_relpath has been resolved from
EXTERNAL_MEDIA_ROOT rather than from event.external_path since #1163 landed,
and this fixture still carried the base-relative form, so the two tests stopped
resolving the moment that stack merged. Production was never affected.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
Stable twin of #1184. Both photo sweeps tracked whether they were running in a
module-level variable, which is invisible to every other replica: a status poll
routed to an idle replica reports isRunning false while another is mid-run, and
the next POST starts a second pass over the whole library.
Migration 179 adds one row per job, claimed with a conditional UPDATE whose
affected-row count is the answer. The lease is fenced on a per-claim token so a
runner superseded by a stale takeover cannot renew a claim it has lost or
release one it no longer owns; renewal runs on a timer spanning the claim
through release, since one hung NAS read can outlast the stale window inside a
single iteration. maintenance_jobs is excluded from .picpeak archives.
Gated on settings.edit / settings.view rather than main's system.manage, which
does not exist on this branch — they are what settings.edit was later split
into, so both branches let the same people through.
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* fix(external-media): record captured_at on import and add a backfill (#1172)
Stable twin of #1179.
External media never went through photoProcessor, so captured_at stayed NULL
for every externally imported photo. The gallery's "Date Taken" sort then
degraded into import order through its own COALESCE fallback — a library
imported in two batches showed the first days of a trip after the last ones.
Ported whole:
- adminExternalMedia.js reads the capture date at import, off the file it has
already opened for the dimensions. Best-effort, like the dimensions.
- A backfill endpoint for photos imported before this, so existing installs
can fix historical rows rather than only new imports. Managed originals go
through resolvePhotoStorageKey + withLocalCopy so S3 installs work; archived
events are excluded because archiving deletes their originals; the run flag
is claimed before the candidate query so two POSTs cannot both start.
- gallery.js carries photos.id as a tiebreaker on all three sorts. A bulk
import writes hundreds of rows inside the same second, so uploaded_at ties
are the normal case and the grid reshuffled between page loads.
One deliberate difference from main: the backfill is gated on settings.edit /
settings.view rather than system.manage / system.view, which do not exist on
this branch. They are what settings.edit was later split into, and main's
migration 175 projects every settings.edit holder forward onto system.manage,
so both branches let exactly the same people through.
* fix(external-media): gate the status card on the permission the button needs (#1172)
The built-in admin role holds settings.view but not settings.edit
(056_add_role_permissions_table.js:63), and StatusTab renders its card and
enabled button purely on a successful status payload. Gating the status
endpoint on settings.view therefore showed every admin a Backfill button whose
every click 403s with no error surfaced.
* fix(gallery): make the Date Taken sort correct on SQLite (#1172)
Same defect as the main twin: photos.captured_at holds three storage classes on
SQLite — an epoch-millisecond integer from managed uploads (photoProcessor.js:441
hands knex a Date), ISO text from external imports and the backfill, and null
falling through to uploaded_at's 'YYYY-MM-DD HH:MM:SS' text. SQLite sorts
INTEGER before TEXT unconditionally, so a 2027 capture came back before a 2020
one, and within the text values the 'T' separator outranked the space.
Normalised in the ORDER BY; Postgres keeps the plain COALESCE, its column being
a real timestamp. Regression tests drive the real gallery route on real SQLite.
* fix(gallery): normalise epoch-integer uploaded_at too, and stop polling a 403 (#1172)
Both follow-ups from the main twin's review, ported.
uploaded_at is not always text on SQLite: a .picpeak restore can carry epoch
milliseconds in from an install that stored them that way, and the fallback
branch read it with substr(), comparing '1830297600000' against
'2020-01-01 00:00:00' as text. Both columns now get the integer/real branch.
The status card also polled every ten seconds regardless of permission. On this
branch that hits every built-in admin — they hold settings.view but not
settings.edit — so each would have had a 403 and a logged denial every ten
seconds for a panel they were never shown.
* style: quote convention in the capture-sort test (#1172)
* fix(capture-dates): skip watcher-imported videos, and make the status counts consistent (#1172)
All three follow-ups from the main twin, ported: the three-marker video filter
(fileWatcher sets type/mime but not media_type, so those rows sat in the
backlog forever), the single-aggregate status counts (two queries could report
a negative backlog mid-import), and the card's render gated on settings.edit as
well as the cached payload.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* fix(external-media): store external paths from the media root (#1163)
Stable twin of #1168. Stacked on the #1162 twin, which supplies
deleteDuplicatePhotos.
Importing a second folder into an event silently invalidated every photo
already in it. external_relpath was stored relative to events.external_path,
and every import overwrites that column, so the older rows were rebased onto
the new folder. Nothing errored and the grid still rendered — thumbnails are
written to local storage during the import while the base path is still
correct — so only the things that need the original broke. The reporter had
7547 of 8004 rows pointing into the void.
- external_relpath is now relative to EXTERNAL_MEDIA_ROOT, so a row is
self-describing and nothing an admin does to the event can move it.
- migration 177 folds each event's base into its rows. Where the current
resolution is missing it walks up for an ancestor holding a file of the same
name AND the size the import recorded — existence alone would let a deleted
file adopt an unrelated namesake and serve the wrong original. Rows it
cannot place keep resolving where they resolve today, and the probe is
skipped entirely when the mount is unreachable.
- probing is read-only and runs first; the rewrites and the marker commit
together, so an interrupted fold cannot be folded twice.
- rewrites are staged through a per-row parking value, because a final path
can equal another row's current one; and migration 177 re-throws without the
driver's error code, which run-migrations-safe would otherwise read as
"schema already exists".
- the fold also runs after a .picpeak restore, since knex_migrations is
excluded from the archive, and a failure there is reported rather than
presented as a clean restore.
- drops the duplicate-leaf-segment guess in photoResolver, which papered over
this same double-prefixing.
Divergence from the main twin: no face-scan requeue reordering. Face
recognition is main-only, so the hazard of queueing rows against unconverted
paths does not exist on this branch — in picpeakImportService or in
restoreService.
Verified on this branch: 23 new tests pass, and the four suites carrying
base-relative fixtures were updated. Full suite leaves the same 5 pre-existing
failures as origin/stable, unchanged.
* fix(external-media): the fold's staging value must be storable on Postgres (#1163)
External review found this on this branch first; it was on both.
The two-pass rewrite parks each row on a temporary value, and that value was
written with a leading NUL. SQLite stores NUL in TEXT without complaint;
Postgres rejects it outright — "invalid byte sequence for encoding UTF8: 0x00"
— so migration 177 rolled back on exactly the installs that need the two-pass
repair, and only on the engine most of them run. Restores hit the same wall
and reported the conversion as failed.
The prefix is ordinary text now. Adds a gated Postgres test alongside the
existing picpeakRestorePg one, because a SQLite-only suite structurally cannot
catch this class: restoring the NUL makes exactly the two-pass repair case
fail with that error, and nothing else.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* fix(external-media): one row per external file per event (#1162)
Stable twin of #1167.
Two overlapping import-external runs against the same event inserted every
file twice. The route checked for an existing external_relpath and then
inserted, with an fs.stat and a sharp().metadata() read sitting in between — a
window wide enough for both runs to see "not there". A reporter's event held
8004 rows for 6012 distinct paths. Nothing at the storage layer stopped it:
migration 041 created only a NON-unique (event_id, source_origin) index.
- migration 176 removes the existing duplicates and adds a partial unique
index on (event_id, external_relpath), verified against the catalog
afterwards — a failed CREATE INDEX raises 23505 on Postgres, which
run-migrations-safe treats as "schema already exists" and would record as
applied on an install that never got the index.
- dependent rows are removed explicitly rather than by cascade: PicPeak never
sets `PRAGMA foreign_keys = ON`, so on SQLite the declared CASCADE is inert
and a bare delete strands feedback and access-log rows. Guest feedback moves
to the survivor instead of being discarded, keyed on guest identity the way
feedbackService defines it, and the survivor's denormalized counters are
recomputed.
- the route treats a unique violation as a skip, so a writer this process
cannot see converges instead of duplicating, and a second import while one
is running gets a 409.
- a .picpeak taken before migration 176 carries exactly these duplicates, and
suspending FK enforcement does not suspend a unique index — so the restore
drops the index for the load and rebuilds it after running the same dedupe.
Divergences from the main twin, both because the feature is absent here:
faces (no faceProcessor, so no purgePhotoFaces reconciliation — the rows are
still deleted so nothing dangles), admin marks, transfer membership, and
photos.view_count/download_count. The service guards each on hasTable /
hasColumn, so those branches simply do not fire.
Verified on this branch: 36 new tests pass; full suite leaves the same 5
pre-existing failures as origin/stable, unchanged.
* fix(external-media): invalidate the download zip when duplicates are removed (#1162)
External review. Same fix as the main twin.
The pre-built "download everything" archive still contained the duplicate rows
the dedupe had just deleted, so guests kept receiving them. Every ordinary
photo-deletion path calls downloadZipService.invalidate for exactly this
reason.
The columns are cleared rather than the service being called: that service
carries debounce timers and a regeneration queue, which a migration should not
start. getZipInfo already treats a cleared record as a cache miss and rebuilds
on the next request. The stale object is left in storage, as elsewhere.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
Stable twin of #1153.
Everything in the system treats a hidden row as absent, but the per-viewer is_liked heart read the row without looking at is_hidden — so a like the photographer had hidden still showed as liked on a photo whose like_count was zero.
Making that agree exposes the second half: the duplicate check behind like/favorite toggling did not skip hidden rows either, so the now-empty heart, when clicked, found the hidden row and toggled it OFF. Skipping hidden rows there makes the click create a fresh, visible row.
Also carried from review: the per-guest caps, /my-feedback and getEventFeedbackSummary no longer count hidden rows, and unhiding collapses the guest's replacement — skipped when there is no stable identity, since that fallback was 'guest_identifier IS NULL', i.e. other visitors' rows.
Not carried: the my_color_label badge (colour labels are #1044) and the clearScope / singleValueScope visibility fix, neither of which exists on this branch.
Merged with admin privileges: the author cannot self-approve.
Stable twin of #1147, filter half only.
Every filter token on /photos is an OR of two halves: what THIS viewer marked, and what ANYONE marked. The fields built from the second half are gated on show_feedback_to_guests; the filter was not, so with the setting off ?filter=liked still returned exactly the photos other people liked — the membership instead of the count, one token at a time.
The half it left standing was also the wrong half: it read guest_identifier from the guest_id query parameter, which never matched anything, and accepting a caller-supplied identifier was a way back through the gate. Resolved from the request now, hidden rows excluded to match what the viewer can see.
Not carried: the color: token and the photo_admin_marks concurrent-write fix — colour labels and admin marks are not on this branch.
Merged with admin privileges: the author cannot self-approve.
Stable twin of #1152. The reporter is on v3.46.1, so this branch is where the bug was actually seen.
showLogout was hard-coded true, so a gallery with no password showed a Logout button, and clicking it stranded the visitor on the loading skeleton — GalleryPage's auto-login is a one-shot latch that never re-fires.
The button is gated at both call sites, including the full-page layouts which render it on the callback rather than a flag. accessLevel and viaCustomer now come from /auth/session instead of per-tab sessionStorage, which silently downgraded a PIN-client session in a second tab. The public-gallery branch shows a reason and a Retry once auto-login has run and failed, instead of a skeleton that never stops.
Carries the full main fix including viaCustomer, even though reveal mode does not exist here, so the branches do not drift.
Merged with admin privileges: the author cannot self-approve.
Stable twin of #1151.
The script here carried the identical defect: it computed `storage/events/active/<photo.path>` and fs.access'd it, which does not exist for external or reference rows. #1129 already landed on this branch, so the route was fixed and the script was the remaining half.
Resolution goes through ensureThumbnail, which stable already exports with the external branch intact. Also carried: videos skipped on every marker, skip-vs-generate asked from isThumbnailValid, and a nonzero exit when a photo could not be built.
Not carried: the responsive-tier backfill — THUMBNAIL_WIDTHS and ensureThumbnailAtWidth are #1095/#1109 and do not exist on this branch.
Merged with admin privileges: the author cannot self-approve.
Two independent causes of the same symptom — an aspect-ratio layout that does
not lay anything out.
gallery-premium discarded the tile height MasonryPhotoAlbum computed from
photos.width/height and set height:auto on both card and image, so the rendered
shape came from whatever rendition was served. With thumbnail_fit seeded 'cover'
every rendition is square, so the masonry drew identical squares.
The bundled CSS templates pinned images to a fixed pixel height, which beats the
.h-full utility six of the seven layouts rely on. Elegant Dark is seeded
is_default, so that was the out-of-the-box result for any layout other than
grid/timeline.
Migrations 052/053 corrected for fresh installs; 175 repairs the rows already
seeded. The repair is whitespace-tolerant because sanitizeCSS strips newlines
from any template ever saved through the editor, matches the height property
with a lookbehind so line-height/max-height are untouched, handles grouped
selectors and skips nested rules.
Stable twin of #1135.
LocalFsStorage.get() returns an fs.createReadStream, which is lazy: it resolves
immediately and opens the file on a later tick, so an ENOENT arrives after the
await returned and outside the route's try/catch. An unhandled 'error' event is
a process-level throw Express cannot catch — the backend exits and every gallery
goes blank until the container restarts.
gallery.js had ten .pipe(res) calls and zero error handlers.
pipeStreamToResponse attaches the missing handler: a vanished source becomes a
404 (410 for a prepared zip), anything else a 500, and a source that dies
mid-response destroys the connection rather than rewriting a status already on
the wire. Headers staged for the file are cleared first — Express does not
overwrite an existing Content-Type, and a surviving Cache-Control would let a
transient 404 be cached as a broken tile for up to an hour. It also releases the
source when a client hangs up.
Applied to all eight streaming responses, not just the thumbnail route.
Stable twin of #1133, reduced: the tier-race half does not apply here because
ensureThumbnailAtWidth does not exist on this branch.
POST /admin/thumbnails/regenerate resolved every source as
storage/events/active/<photo.path> and fs.access'd it. External and reference
rows are not there — their originals live under events.external_path — so every
one failed the check and was counted as an error, while the UI reported success
because the response is sent before the background loop starts.
It now goes through ensureThumbnail, which resolves both source kinds and writes
thumbnail_path back itself. Nulling thumbnail_path stops it short-circuiting on
isThumbnailValid, which matters because the old thumbnail is normally still
readable at exactly the moment someone presses regenerate.
And the half that destroys data: generateThumbnail deleted the target BEFORE
sharp had opened the source, and again in its catch. A source that could not be
read — a NAS mount that blipped — left the previous rendition gone and the
database pointing at it. Across a bulk regenerate that is the whole gallery.
Neither delete was needed: put stages to a temp file and renames atomically, and
put is the last statement in the try so no partial object can exist.
Also: videos filtered out, and the superseded rendition removed only when the
storage key actually moved, compared through the same canonicalisation the
backends apply.
Stable twin of #1134.
Closes#1105.
iOS Safari zooms the whole page in when a focused form control computes to
under 16px, and it does not zoom back out. Unlocking a gallery is a
client-side transition rather than a document navigation, so the zoom the
password field triggers carries straight into the gallery: the layout pans
horizontally and the header actions sit off-screen until the visitor
pinch-zooms out by hand. A real page load would have reset it.
`.input` and `.input-themed` are `text-sm`, so the field is 14px on every
phone, and GalleryPage inverts the breakpoint on top of that
(`text-sm sm:text-base` — 14px below 640px, where iOS zooms, and 16px above
it, where it never does).
Keyed to the POINTER, not a width. The zoom depends on the computed font size
and a touch device, never on how wide the viewport is — and a phone in
landscape is 667-956 CSS px, above any width you could call "phone". Measured
on the admin login page, which has no `sm:` override:
main portrait 390x844 14px zooms
main landscape 844x390 14px zooms
main iPad 820x1180 14px zooms
fixed all three 16px
fixed desktop (mouse) 14px unchanged, no zoom off touch
One media query rather than flipping each call site. `.input` alone backs 334
`<Input>` usages, but there are also ~440 raw inputs, selects and textareas
carrying their own `text-sm`, and Tailwind utilities sit in a later layer than
@layer components — so a fix at the component definition misses most controls
and any new `text-sm` silently reintroduces the bug.
The `:not()` on each selector is load-bearing, not decoration: it buys the
specificity to beat a utility class. Measured in a browser —
input.text-sm 16px (0,2,1 beats .text-sm)
select.text-sm 14px (0,0,1 loses)
textarea.text-sm 14px (0,0,1 loses)
24 selects and textareas in the tree carry `text-sm`, so the bare form would
have left them zooming. Checkbox and radio stay excluded so font-size never
sizes their box.
max(16px, 1em, 1rem) is a FLOOR, not a size. A flat 16px would make controls
that are already bigger smaller: Typography -> Large sets --font-size-base to
18px on body, so anything inheriting it would be clamped down and the setting
quietly ignored. Each term covers a case the others miss:
normal (body 16) 16px Large theme (body 18) 18px
Small theme (body 14) 16px browser default 20px 20px
The viewport meta is deliberately left alone: `maximum-scale=1` would suppress
the zoom by disabling pinch-to-zoom for everyone.
Stable twin of #1083, scoped to what exists on this branch.
docker-build.yml — set ignore-unfixed on both Trivy steps. Stable has
the backend and frontend legs only (no aio, no ml), so two steps here
against four on main. Base-image CVEs with no released fix are not
actionable: the Dockerfiles already run `apt-get upgrade -y` behind a
CACHEBUST, so a fix lands in the next build automatically. Reporting
them buries anything someone can actually act on.
backend — deepmerge-ts <8.0.0 has a stack-exhaustion advisory
(CVE-2026-40345, high) reached via mailparser -> html-to-text, which
pins ^7.1.5 so npm cannot get there alone. Stable carries the same
mailparser ^3.9.9 and the same 3-high exposure as main. Not reachable
in our code: html-to-text only feeds deepmerge-ts its options object,
never parsed email content. npm audit on this branch goes 3 high -> 0.
The ml/Dockerfile half of #1083 has no counterpart here — the face
sidecar does not exist on stable, so there is nothing to drift.
Verified on stable itself rather than assuming main's results carry:
npm audit 3 high -> 0, html-to-text exercised end-to-end through
simpleParser, and jest at 1577 passed. The 5 failing suites (20 tests)
fail identically on clean origin/stable with these changes stashed.
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* fix(preview): generate lightbox previews for external/reference photos (#1078)
Stable twin of the main-line fix. ensurePreviewImage() resolved its source
only via resolvePhotoStorageKey(), which returns null for external/reference
photos by design — those live on a media mount outside the managed storage
tree. The null went straight into withLocalCopy(), which throws, so the
preview route fell back to redirecting at the full-size original. Galleries
whose photos are all external got no benefit from the preview tier (#492):
guests paid 5-12 MB on every lightbox open.
Add the external branch ensureThumbnail() already has: resolve via
resolvePhotoFilePath() and feed the mount path to generatePreviewImage()
directly, with an ext<id>_ output basename.
generatePreviewImage() on this branch hardcoded path.basename(imagePath) and
ignored options.outputBasename, so it needs the same one-line honouring that
generateThumbnail() already does — without it two events referencing the same
NAS basename collide on one preview key.
Also return null rather than throwing for a row with no source_origin in a
reference-mode event, whose mode falls back to the event's.
Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc
* fix(preview): select the columns the external branch needs on bulk regenerate
POST /api/admin/thumbnails/regenerate-previews selected only id, event_id,
path, media_type, mime_type and preview_path, so photo.source_origin was
undefined by the time ensurePreviewImage branched on it. Every external row in
a reference gallery took the managed path, resolvePhotoStorageKey returned null
for it, and the endpoint reported success while generating nothing.
Add source_origin, external_relpath and filename to the select, plus a
source-inspection test pinning the caller contract and a service-level test
showing a column-starved row is indistinguishable from a managed one.
Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
Stable twin of #1070.
persistDocPdf, the invoice sending and reminder writers, both contract
signature writers and persistSignatureImage built their targets from
`path.join(process.cwd(), 'storage', 'business-docs', ...)` and never
consulted STORAGE_PATH. Both compose files pin STORAGE_PATH=/app/storage
and the image's WORKDIR is /app, so on a stock deployment the two name
the same directory and nothing looked wrong. Point STORAGE_PATH anywhere
else and quotes, invoices, Mahnungen, contracts and signature images
land outside the configured storage root: missed by the backup walker,
invisible to storage accounting, and gone when the container is
replaced.
assertContractPdfPath moves with them. On this branch the writers and
the guard are wrong together, so contract downloads currently work —
migrating the writers alone would have introduced PATH_OUTSIDE_STORAGE
on every newly generated contract. The guard now resolves through
getStoragePath() like the writers, and keeps the legacy cwd root so
contracts written before this still resolve; their absolute paths are
in the database.
Also on the shared resolver: the custom PDF font lookup (a font under
STORAGE_PATH/fonts was never found, and the document silently fell back
to the built-in face) and the two backup diagnostics, which otherwise
inspect a different root than the backup walker when STORAGE_PATH is
unset.
No migration needed — the persisted path is stored absolute.
Verified on this branch, not inferred from main: the new test is 6/6,
and contract/quote/invoice/pdf/safePath suites are 213/213 both before
and after the change.
Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
Stable twin of #1071.
Three specs acquire an admin token with `const body = await res.json();
return body.token`. On this branch too the admin login sets the JWT as
the httpOnly `admin_token` cookie and responds with `res.json({ user })`
— verified in auth.js on stable, not assumed from main — so the token is
undefined and each spec fails at its first assertion, before exercising
anything it was written to cover.
Cookie and Authorization: Bearer are interchangeable server-side, so the
helpers read the value back out of the context cookie jar and keep
threading it as a Bearer. Every downstream call is unchanged.
Verification is weaker than the main twin's, deliberately: the three
spec files are byte-identical to the ones measured there (0 passed /
6 failed before, 3 passed / 3 failed after, against a live stack), and
they compile and enumerate on this branch. Standing up a full stable
compose stack to re-measure test-only changes was not worth it — say the
word if you want that done before merge.
The remaining failures are UI staleness, not auth, and are not addressed
here. No CI workflow runs tests/e2e on this branch either, which is why
this rotted unnoticed.
Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
Stable backport of #1055 (main: 3a11e6eb). Change content is byte-identical
to the main twin; cherry-picked clean, no resolutions needed.
The six quote/invoice PDF endpoints interpolated buildPdfFilename()'s result
straight into `inline; filename="${filename}"`. That result deliberately
preserves non-ASCII (it doubles as the PDF's internal Title metadata), and
HTTP header values are latin1, so a customer label reaching the header
directly failed in one of two ways:
- U+0080-U+00FF (ä ö ü ß — every German umlaut): no throw. The raw byte
goes out and the client reads back a mangled name. Silent corruption.
- above U+00FF (Polish ł, Czech ř, Turkish ş, €, Cyrillic, CJK, emoji):
Node's setHeader rejects it with ERR_INVALID_CHAR. The throw lands
after the PDF buffer is already rendered, so the request 500s.
This corrects the issue's diagnosis: it reported umlauts as the 500 case,
but umlauts are inside latin1 and mangle rather than throw.
Route all six through buildContentDisposition(), which emits an ASCII
fallback plus the RFC 5987 filename*=UTF-8'' form. Also stops sanitiseSegment
splitting surrogate pairs at its 80-unit cap — a dangling high surrogate makes
encodeURIComponent throw URIError inside the helper, reaching the same 500 a
different way (found by external review on the main twin).
Verified on this branch: 14/14 in the new suite, 151/151 across the nine
surrounding pdf/filename/quote/invoice suites, lint clean.
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
Stable backport of #1049 (main: 3600231d). Change content is byte-identical
to the main twin.
S3StorageAdapter built its S3Client with no requestHandler timeouts, and the
SDK's defaults wait indefinitely. When a connection is dropped silently (no
FIN/RST — NAT/LB idle reaps, transient faults), the in-flight request hangs
forever and every subsequent storage operation queues behind it process-wide;
only a restart recovers. _retryOperation never ran because the promise it
wraps never settled.
Configure connectionTimeout (120s) and socketTimeout (60s) on the request
handler, overridable via STORAGE_S3_CONNECTION_TIMEOUT /
STORAGE_S3_SOCKET_TIMEOUT, and add TimeoutError to the retryable list so the
existing backoff engages.
socketTimeout rather than requestTimeout: the latter is a total-duration cap
that would kill legitimate large uploads and only warns without
throwOnRequestTimeout. Both values are deliberately generous — connectionTimeout
covers time queuing for a socket from the agent pool (maxSockets 50), so a
short value expires while merely waiting in line and breaks reads.
Reported against v3.45.16 on Cloudflare R2: wedged roughly every 40 minutes
with serial uploads, every 15-20 with 4 parallel uploaders.
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
Stable backport of #1043 (main: 8809564a).
sqlite → pg restore is allowed for anyone holding backup.restore, from the
upload UI and the CLI alike, gated by the manifest-direction rule in
validateManifest. pg → sqlite stays refused, with an error naming the
supported direction. allowEngineSwitch is removed rather than kept alongside:
one gate, no way to drive the refused direction.
Two resolutions were needed against stable rather than a clean cherry-pick,
both from known main/stable divergences:
- replaceAllTables has no roleSnapshot parameter on this branch, so the call
keeps stable's 4-arg signature while taking the derived { crossEngine }.
- resyncSequences was guarded by `if (allowEngineSwitch)`, which this change
removes — leaving an undefined reference. It now runs unconditionally,
matching main. That also closes a stable-only gap: a same-engine pg → pg
restore previously left identity sequences stale, so the next natural
insert collided on the primary key.
Also exports resyncSequences (the function already existed here, main already
exports it) so the cross-engine suite can drive the post-restore fixup.
Verified on this branch: all four picpeak suites green on SQLite, and 20/20
against a real Postgres 15 with the PICPEAK_PG_TEST_URL-gated cases executing.
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
Stable backport of #1056 (main: 18b1e0f6). Change content is byte-identical
to the main twin.
The .picpeak restore suites gate their Postgres cases behind
PICPEAK_PG_TEST_URL and describe.skip themselves out when it is unset. The
variable is set in no workflow, so those cases have never run in CI.
On this branch the effect lands together with the #1041 backport, which brings
picpeakCrossEngine.test.js and its three real-Postgres stored-value cases —
stable has no picpeakRestorePg.test.js, so before that PR this wires up a
service nothing reads yet. Merging it first keeps the two twins mirroring
their main counterparts one-for-one instead of folding both into one PR.
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* fix(docker): default NODE_ENV=production so non-compose deploys don't fall back to SQLite (#1038)
knexfile.js selects its config block by NODE_ENV and the `development` block
defaults to sqlite3. The image never set NODE_ENV, so every deployment that
doesn't go through our compose files — Kubernetes, Helm, plain `docker run` —
silently ran on SQLite and ignored DB_HOST/DB_USER/DB_PASSWORD.
It stayed invisible because wait-for-db.sh is shell: it reads DB_HOST directly,
connects to Postgres, creates the database and logs "PostgreSQL is up" in the
same container where the Node process then writes to a SQLite file. Migrations
go through src/database/db.js → the same knexfile, so they also ran against
SQLite, leaving the provisioned Postgres database empty.
Setting the default alone would be unsafe: an affected install would flip to
Postgres on its next image pull and come up against an EMPTY database, which
reads as total data loss. So this adds a guard that runs before migrations
touch anything:
- logs the resolved engine + target at boot
- refuses to start when pointed at a virgin Postgres while a populated
SQLite file exists, naming the file and the .picpeak export path for
moving the data, with PICPEAK_ALLOW_EMPTY_PG=true as the escape hatch
- warns but boots when Postgres settings are present yet SQLite is in use
Compose files already set NODE_ENV explicitly, so compose users are unaffected.
Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM
* fix(db): stay on SQLite instead of blocking, and add a migration path (#1038)
Reworks the guard after walking through what an existing install actually
experiences on its next image pull.
Blocking was the wrong trade. An operator who had unknowingly been running on
SQLite (because the image left NODE_ENV unset) would have pulled the fix and
got a CrashLoopBackOff: data safe, galleries offline, for something they did
not do. Now the boot RESOLVES the engine before migrations run and stays on
whichever one holds the data:
- Postgres configured but holding no galleries, while a populated SQLite file
exists → keep serving from SQLite, print what happened and how to migrate.
- once Postgres holds the data, the next restart switches over on its own.
- an explicit DATABASE_CLIENT is always honoured.
Keyed on Postgres holding DATA, not on it having tables: a stray
`run-migrations` against the empty database creates every table, which would
otherwise blind the check.
Adds scripts/migrate-sqlite-to-postgres.js, which reuses the .picpeak
export/import services rather than hand-rolling a cross-engine copy. Two
additions were needed for the SQLite → Postgres direction, both opt-in and
CLI-only so the upload/restore UI is untouched:
- `allowEngineSwitch` relaxes the importer's same-engine guard
- cross-engine row coercion: SQLite has no real date or boolean types, so its
rows carry epoch numbers where Postgres wants a timestamp and 0/1 where it
wants a boolean, both of which Postgres rejects outright. Driven by the
TARGET schema, never guessed from the value.
DELTA FROM THE BETA PR: this branch's import service has no resyncSequences()
— that landed on main only. Without it a cross-engine load leaves Postgres
identity sequences at 1 and the next insert collides on the primary key, so
the function is backported here and called ONLY on the cross-engine path.
Same-engine restores through the UI keep their current behaviour exactly.
Verified end to end on this branch against a real PostgreSQL 15: a seeded
SQLite install migrated across with booleans, timestamps and foreign keys
intact, and the next INSERT got id 2 rather than colliding.
Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM
* fix(db): close four review findings on the SQLite fallback + migration (#1038)
External review (codex) found four issues, all confirmed against the code and
fixed here. Two of them could have cost data.
1. The engine resolver was reachable only through wait-for-db.sh. A Kubernetes
manifest that sets `command`/`args`, or a plain `docker run … node
server.js`, bypasses the entrypoint — exactly the deployment styles this fix
targets. With NODE_ENV now baked into the image, such an install would have
resolved to Postgres and come up against an empty database while its SQLite
data sat there unseen. server.js now resolves the engine itself, before
anything requires knexfile, via the same script the entrypoint uses.
Verified by running `node server.js` directly against an install with
stranded SQLite data: it logs the banner and serves SQLite.
2. Cross-engine loads double-encoded JSON. SQLite has no json type, so its json
columns are TEXT holding JSON; the export dumps that as a string and
serialiseJsonColumns stringified it again, storing `true` as the scalar
string "true". app_settings.setting_value is json on every install, so this
reshaped every migrated setting. The text is decoded before serialisation
now — verified against a real Postgres: json_typeof(setting_value) is
`boolean`, matching a native install exactly.
3. The migration could silently miss concurrent writes. If the backend keeps
serving, rows written after the export never reach Postgres and vanish from
view once the engine switches. The script now fingerprints the SQLite tables
whose loss would be noticed, checks for drift BEFORE loading Postgres (so a
detected race leaves the target untouched) and again after, and refuses with
the exact rows that moved. It also says plainly to stop the backend first.
4. The child phases shared stdout with winston. Outside production, and
whenever LOG_TO_CONSOLE=true, createPicpeak's own log line was concatenated
with the archive path and the migration failed on a bogus filename. Payloads
travel through a result file now; verified with LOG_TO_CONSOLE=true.
Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM
* fix(db): close review round 2 — six more data-safety findings (#1038)
1. The engine choice is now PINNED once the data is in Postgres. Previously the
boot decided from "does Postgres hold galleries", so an operator who later
deleted every gallery would be sent back to the stale pre-migration SQLite
file while their settings, admins and CRM data stayed in Postgres. The
migration writes a marker next to the database file (and retires the file
itself by renaming it); the marker wins over any probe.
2. The migration refused to overwrite Postgres only when it held GALLERIES. A
target with admins, customers, invoices or projects but no galleries was
wiped without --force. Both the source and target checks now look for user
data across the tables that are empty on a fresh install.
3. Same bug in the other direction: an install with no galleries but real
admins/settings/customers was refused a migration it was entitled to.
4. Drift detection covered four tables and only count/max(id), so an in-place
UPDATE (event edit, password change) or a write to any other table passed
unnoticed. It now fingerprints every table the export carries, including
max(updated_at). It still is not a substitute for stopping the backend, and
the script says so rather than implying a guarantee.
5. probeSqliteData() treated an unreadable or corrupt file as "no data", which
would have switched the install to an empty Postgres — the very failure this
module exists to prevent. It fails closed now and stays on SQLite so the real
error surfaces.
6. The "you are leaving SQLite data behind" warning was unreachable: setting
DATABASE_CLIENT skipped the probes, so the branch that produces it never had
the inputs. Postgres and SQLite are both probed whenever Postgres is the
engine in play.
Also: the final verification compares row counts for EVERY table rather than
just galleries, and flags only a shortfall — the import legitimately adds an
app_settings row (setSessionsValidAfter) that made the strict equality fail on
a first real run.
Verified against a real PostgreSQL 15 end to end, including: the marker keeps
an install on Postgres after every gallery is deleted; removing the marker and
restoring the file rolls back to SQLite as documented.
Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM
* fix(db): close review round 3 — occupancy, bootstrap admin, secrets in /tmp (#1038)
1. Both engine probes judged occupancy by GALLERIES alone. An install whose
galleries were all deleted, but which still has admins, customers or
accounting records, was treated as empty: on the SQLite side that meant
booting the empty Postgres and appearing to lose everything; on the Postgres
side it meant diverting a live install to a stale SQLite file. Both now look
across the tables that are empty on a fresh install, matching the migration
script.
2. The migration ran migrate-schema BEFORE checking the target, and migration
001 seeds a bootstrap admin when ADMIN_PASSWORD is set (common on legacy
installs). The occupancy check then saw that admin and refused, pushing the
operator towards --force against a genuinely empty database. The target is
read first now.
3. probeSqliteData()'s warning went through the app logger, which writes to
STDOUT when LOG_TO_CONSOLE=true — and the resolver's stdout is the protocol
channel wait-for-db.sh captures, so DATABASE_CLIENT could have been set to a
JSON log line. Diagnostics take an injected sink (stderr in the resolver),
and the shell now validates the value it captured instead of trusting it.
4. The .picpeak archive holds password hashes, SMTP credentials and API keys in
plaintext, and was only removed on the fully-successful path — any drift or
import failure left it in /tmp. Every exit path removes it now.
5. A database-only migration still hauled every business-doc and upload through
/tmp and back into the same volume. createPicpeak takes includeFiles:false
for this path; rows move, files stay where they already are.
Verified against a real PostgreSQL 15: a gallery-less install with only an admin
account now stays on SQLite and migrates successfully with ADMIN_PASSWORD set;
the resolver emits exactly one token on stdout with LOG_TO_CONSOLE=true and a
corrupt database; a drift failure leaves Postgres untouched and no archive
behind.
Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM
* fix(db): pin the boot to SQLite while a migration is unfinished (#1038)
Review round 4. A migration that dies after touching Postgres leaves rows
behind — schema creation alone seeds a bootstrap admin when ADMIN_PASSWORD is
set, and a drift or row-count failure can leave a partial load. Since the
occupancy probes were widened in round 3, those rows read as "Postgres is
occupied", so the next restart would switch engines and hide the SQLite data
that is still the database of record.
The script now writes a pin file next to the database BEFORE its first Postgres
write and clears it only on success (after the success marker exists, so no
restart in between can pick the wrong engine). While the pin is present the
resolver stays on SQLite and explains why.
Verified against a real PostgreSQL 15 by reproducing the exact scenario: a
migration failed mid-run with ADMIN_PASSWORD set, leaving one bootstrap admin
in Postgres. With the pin the next boot resolves to sqlite3; with the pin
removed it resolves to pg — the failure this closes. The subsequent successful
re-run clears the pin and the boot moves to Postgres.
Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM
* fix(db): close review round 5 — occupancy, path drift, retry, host default (#1038)
1. A seeded bootstrap admin counted as "Postgres is occupied". core/001_init.js
inserts one whenever ADMIN_PASSWORD is set, so a Postgres that was
initialised once and never used would have beaten a SQLite file full of real
galleries — the exact failure the guard exists to prevent, reintroduced by
widening the probe in round 3. The two sides are deliberately asymmetric now:
the SQLite probe counts any user data (err towards keeping data visible),
the Postgres probe ignores rows that schema creation seeds (err towards
requiring proof of real use).
2. The guard resolved DATABASE_PATH with its own logic while knexfile trimmed
whitespace and collapsed the legacy duplicated-backend form. A path either
engine normalised differently meant probing a file nobody uses, concluding
there was no SQLite data, and booting an empty Postgres. The resolution now
lives in one module both require.
3. Re-running after a partial migration — the documented recovery — was refused
unless the operator passed the destructive-sounding --force, because the
half-written rows read as target data. An unfinished run of this same script
is now recognised as a safe retry.
4. wait-for-db.sh verified readiness against its own default host (`postgres`)
while knexfile's production block defaults to `db`. With NODE_ENV now baked
in, a bare `docker run` without DB_HOST would have passed the readiness check
against one host and then dialled another. The entrypoint exports the exact
connection it verified. Compose sets DB_HOST explicitly and is unaffected.
Verified: a Postgres holding only a seeded admin now loses to real SQLite data;
a DATABASE_PATH with surrounding whitespace resolves to the identical file in
both knexfile and the guard.
Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM
* fix(db): close review round 6 — explicit-client bypasses, retry scope, cleanup (#1038)
1. An explicit DATABASE_CLIENT bypassed the unfinished-migration pin, because
decideBootEngine honoured it first. docker-compose sets DATABASE_CLIENT=pg,
so a failed migration would have restarted on a half-written Postgres on
exactly the deployments that pin it. Worse in the other direction: with
DATABASE_CLIENT=sqlite3, a SUCCESSFUL migration renames the source file, so
the next start created a NEW, empty SQLite database and served that. The pin
now outranks explicit pg (clearing the marker is the override), explicit
sqlite3 is left alone since it already points at the data, and the migration
refuses up front when the deployment pins anything other than pg.
2. The retry allowance was bound to the SQLite file, not to the target. An
operator who repointed DB_HOST/DB_NAME between attempts could have replaced
an unrelated populated database without --force. The pin records the target
and the allowance only applies when it matches.
3. The printed rollback did not roll back: with data on both sides and no
marker, the resolver still selects Postgres. It now spells out all three
steps, including DATABASE_CLIENT=sqlite3.
4. A failure inside createPicpeak left a partial archive — plaintext hashes and
credentials — in the caller-supplied temp dir, which that service
deliberately does not clean. The export phase removes it on error.
Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM
* fix(db): close review round 7 — pin bypass on direct start, real admins (#1038)
1. server.js only ran the engine resolver when DATABASE_CLIENT was unset, so a
deployment that both bypasses the entrypoint (Kubernetes `command:`) AND
pins DATABASE_CLIENT=pg never consulted the migration pin — the round-6 fix
was unreachable on exactly that path, and a failed migration would have
served a half-populated Postgres. The resolver now also runs whenever a pin
file exists.
2. Round 5 excluded admin_users from Postgres occupancy to stop a seeded
bootstrap admin counting as real data. That over-corrected: an install that
has completed first-run setup but has no galleries yet has exactly one
user-created row — an admin — so Postgres looked empty and, with a stale
SQLite file present, the boot would switch away and the admin's credentials
and configuration would disappear.
core/001_init.js seeds must_change_password=true; setupService writes false
once a human completes setup. The FLAG, not the table, distinguishes them,
and a legacy NULL counts as a real admin.
Verified against a real PostgreSQL 15: a Postgres holding only the seeded row
loses to real SQLite data, the same Postgres wins once setup is completed, and
a server started directly with DATABASE_CLIENT=pg and a pin present comes up on
SQLite with the warning.
Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM
* fix(db): close review round 8 — reset admins, CLI config, JSON nulls (#1038)
1. must_change_password is mutable: resetAdminPassword() re-raises it on REAL
accounts (userManagementService.js:474). Round 7's discriminator therefore
read a gallery-less Postgres whose only admin had been reset as an untouched
bootstrap seed — and with a stale SQLite file present, the boot would have
switched away and hidden those live credentials. The rule is layered now:
more than one admin, any admin that has logged in, or must_change_password
false all count as use. Only core/001_init.js's exact leftovers — one admin,
never logged in, still flagged — read as a seed.
2. The CLI read process.env directly but never loaded the configuration the
child phases get through knexfile, so running it directly (or via
`docker exec`, which does not inherit wait-for-db.sh's exports) failed the
pre-flight checks even with valid settings in backend/.env or
/run/secrets/db_password. Both sources are loaded up front now.
3. The migration's target check counted a seeded bootstrap admin as user data
while probePgData classified the identical row as empty, so migrating into a
previously-initialised-but-unused Postgres demanded --force. Same rule on
both sides.
4. Cross-engine JSON handling is simpler and no longer lossy. SQLite keeps json
columns as TEXT holding valid JSON and Postgres accepts JSON text directly,
so the correct action is to pass them through untouched. Round 1 parsed then
re-serialised them to undo a double-stringify; that round-tripped the JSON
literal `null` into SQL NULL, changing data and breaking NOT NULL json
columns. Not serialising at all fixes both.
Verified against a real PostgreSQL 15: a migrated install now carries
json_typeof = null for a JSON null, object for a nested object, and boolean for
a boolean — matching a native install exactly.
Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM
* fix(db): close review round 9 — probe error classes, marker ordering (#1038)
1. probePgData() answered every failure with "Postgres has data". That is right
for an unreachable server — the app cannot run on it either way, and
diverting a healthy pg install to a stale SQLite file over a transient blip
would be worse — but wrong for a server that answers and then fails the
query, which is what a half-built or damaged schema looks like. That is not
evidence of data, and reporting it as such booted the empty Postgres and hid
a populated SQLite file: the exact failure this guard exists to prevent.
Reachability is now established with SELECT 1 first, so the two cases get
opposite answers: unreachable → leave the configured engine alone;
reachable-but-uninspectable → unproven, and the SQLite side wins if it
actually holds data.
2. The success marker was written after the SQLite file was renamed away. A
failure in between — a full disk — left the source retired with no marker:
the next attempt reported "No SQLite database", the in-progress pin stayed,
and the operator never saw the rollback path. The marker is written first
and updated with the retired filename once the rename succeeds, so a failure
at any point leaves everything recoverable.
Verified against a real PostgreSQL 15: a reachable database whose admin_users
table lacks the probed column now resolves to sqlite3 rather than hiding the
data, while an unreachable host still resolves to pg.
Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM
* fix(db): don't fail the migration on empty SQLite-only tables (#1038)
Review round 10. The final verification flagged every source table missing from
Postgres, regardless of whether it held rows — and SQLite-only tables do exist:
initializeDatabase() creates an `events_new` scratch table and, when its legacy
column copy throws, the catch swallows the error and leaves the empty table
behind (db.js:236). The importer correctly skips tables Postgres does not have,
so verification then reported a mismatch AFTER the data had already landed,
exited 1, and left the install pinned to SQLite with no way to finish.
An absent target table only matters if the source actually had rows. Empty ones
are now listed and skipped.
Reproduced both ways against a real PostgreSQL 15 with an events_new table
present: without the fix the run ends in "ROW COUNTS DO NOT MATCH" and leaves
the in-progress pin; with it, the table is reported as skipped, the migration
completes and the pin is released.
Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM
* fix(db): a completed migration overrides an implicit SQLite config (#1038)
Review round 11. The migration allowed the one configuration it should have
worried about most: DATABASE_CLIENT unset AND NODE_ENV not "production", which
resolves to the development block — i.e. sqlite3. That is precisely the state
the affected installs are in, since it is why they ended up on SQLite at all,
so an operator can easily run the migration before fixing it. The script then
renames the source database away, and the next start resolved to the implicit
sqlite3, created a NEW empty database and served it — after reporting success.
The success marker now overrides an IMPLICITLY resolved sqlite3 when Postgres
settings are present, because the marker is durable proof of where the data
actually went. An explicit DATABASE_CLIENT=sqlite3 still wins: that is the
documented rollback.
The script says something rather than refusing — refusing would block exactly
the population this exists for.
Reproduced with NODE_ENV and DATABASE_CLIENT both empty, against a real
PostgreSQL 15: the migration completes, the source is renamed away, and the
next boot resolves to pg with the data intact. Before this it resolved to
sqlite3 and would have served an empty database.
Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM
* refactor(db): drop the dead reachability flag in probePgData (#1038)
github-code-quality flagged `if (reachable)` as always true, and it is right:
the unreachable branch returns, so everything below it runs only when the probe
connected. The variable and the conditional were leftovers from a first draft
that used a single catch for both failure classes.
No behaviour change — the two error paths still return opposite answers.
Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM
* fix(db): refuse to choose when both databases hold data (#1038)
Review round 12.
1. An install that ran on PostgreSQL, lost NODE_ENV/DATABASE_CLIENT, and kept
working on SQLite has REAL data on both sides: old rows in Postgres, newer
ones in SQLite. The stranded-data rule only protected SQLite when Postgres
was empty, so pulling this fix would have booted Postgres and hidden every
gallery created since the switch — the exact failure this PR exists to
prevent, in a variant I had not considered.
A completed migration leaves a marker saying which side is current. Without
one, two populated databases are a conflict: the boot stops and prints both
targets, the two DATABASE_CLIENT values that resolve it, and the migration
command that merges them. This is the only deliberate refusal in the change —
guessing here would hide data AND split subsequent writes across two
databases.
2. probePgData was handed knexConfig.connection even when knexfile had resolved
to SQLite (a completed migration whose environment still says sqlite3), so
node-postgres dialled its own localhost defaults instead of DB_HOST/DB_NAME —
false "unreachable" diagnostics and a needless delay on every boot. The probe
target is now built from the environment when the config is not pg.
The conflict is honoured by all three entry points: the resolver exits 3 with an
empty stdout, wait-for-db.sh stops the container, and server.js refuses to start.
Two existing tests asserted that Postgres wins when both sides hold data. They
encoded the pre-conflict assumption and described a state that cannot occur
after a real migration (which always leaves a marker); both now pass the marker.
Found while testing: the resolver's logger shim had no .error, so the conflict
path threw, was swallowed by the fallback, and silently chose Postgres — the
precise outcome this refuses to make. The shim is complete now.
Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM
* fix(db): symmetric bootstrap rule, one resolved Postgres target (#1038)
Review round 13. Both findings are consequences of earlier rounds.
1. The conflict rule added in round 12 counted an untouched SQLite bootstrap
admin as data. core/001_init.js seeds one whenever ADMIN_PASSWORD is set —
including into the accidental SQLite database — so a healthy Postgres install
that had ever started once without NODE_ENV would have had a seeded-only
SQLite file beside it, been declared a both-populated conflict, and REFUSED
TO BOOT. The bootstrap discrimination is applied on both sides now; a
setup-completed or logged-in admin still counts as real use on either.
2. The CLI's child phases inherited whichever knexfile block NODE_ENV selected.
The development block defaults Postgres to localhost/postgres/photo_sharing,
production to db/picpeak/picpeak — and this script is explicitly meant to run
with NODE_ENV unset. With DB_USER/DB_NAME left to defaults it would therefore
have migrated into `photo_sharing`, after which following the script's own
advice to set NODE_ENV=production pointed the app at an empty `picpeak`.
The target is resolved once, with production defaults, and passed explicitly
to every phase — so the block knexfile happens to pick can no longer decide
which database the data lands in. The pin and success marker record that same
resolved identity.
Verified against a real PostgreSQL 15: a live Postgres beside a seeded-only
SQLite file now boots pg rather than refusing, flipping that admin to
setup-completed restores the conflict, and a migration records
localhost:7102/picpeak_r13b as its target rather than a defaulted guess.
Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM
* fix(db): one Postgres identity everywhere; protect the credentials file (#1038)
Review round 14. Three of the six findings were the same defect as round 13's,
surfacing through paths that fix did not cover: the connection used to PROBE or
MIGRATE could differ from the one the application then OPENS, because
knexfile's development block points Postgres at localhost/postgres/photo_sharing
while production uses db/picpeak/picpeak.
1. server.js exported only DATABASE_CLIENT=pg after the resolver decided, so
knexfile filled in host/user/database from whichever block NODE_ENV selected.
With SQLite already retired by a migration, that meant opening an empty
database. The whole connection is pinned now.
2. Two defaults existed for DB_HOST: wait-for-db.sh resolves and exports
`postgres`, knexfile's production block says `db`. Since the entrypoint
exports its value, `postgres` is what a running container actually uses — so
a `docker exec` migration, which inherits neither, has to agree with that,
not with the default that is only reached when the entrypoint did not run.
3. The migration's Postgres phases inherited an unset NODE_ENV and therefore the
development block, which ignores DB_SSL entirely — a managed Postgres
requiring TLS could never be migrated into. The phases run with production
semantics now.
4. core/001_init.js writes data/ADMIN_CREDENTIALS.txt, and that data directory
belongs to the SOURCE install. Bootstrapping the Postgres schema replaced the
operator's real credentials file with ones for a temporary admin the import
immediately discards. The file is preserved across the phase, including when
it fails.
5. The boot line described knexConfig, so an install redirected to Postgres by a
migration marker still logged "Database engine: sqlite (...)", contradicting
the warning printed one line earlier.
6. On a both-populated conflict resolveBootEngine returns client:null, and both
migration runners told the operator their data was in "null" and to set
DATABASE_CLIENT=null. They now present the two real choices.
Verified against a real PostgreSQL 15: a migrated install started directly with
NODE_ENV unset now logs `postgres (localhost:7102/picpeak_r14)` and opens it,
where before it would have gone to the development block's photo_sharing.
Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM
* refactor(db): resolve the PostgreSQL target in exactly one place (#1038)
Rounds 13 and 14 both traced back to the same thing, each time through a caller
the previous fix had not covered: three different defaults existed for the same
connection.
knexfile development : localhost / postgres / photo_sharing
knexfile production : db / picpeak / picpeak
wait-for-db.sh : postgres / picpeak / picpeak (and it EXPORTS them)
So a process that probed or migrated against one could hand over to a process
that opened another. Patching each caller was not converging — the guard, then
the CLI's child phases, then server.js — so this deletes the divergence instead.
`src/utils/pgConnection.js` now owns the resolution and knexfile's development
and production blocks both derive from it, as does the engine guard. Same shape
as the earlier sqlitePath.js extraction, for the same reason.
The database NAME is what made this dangerous: a wrong host or user fails
loudly at connect time, while a wrong name connects fine and presents an empty
installation.
BEHAVIOUR CHANGE: with DATABASE_CLIENT=pg and no DB_* variables, a
non-production environment now resolves to postgres/picpeak/picpeak instead of
localhost/postgres/photo_sharing. Deployments are unaffected — compose sets
these explicitly and wait-for-db.sh exports them — but a local machine running
Postgres bare now needs DB_HOST=localhost DB_USER=postgres DB_NAME=photo_sharing
(or DATABASE_CLIENT=sqlite3, which is what backend/.env already uses). The
failure mode of getting this wrong is a refused connection, not a silently empty
database.
Side effect worth having: DB_SSL is now honoured whatever NODE_ENV says, so the
managed-Postgres case is fixed at the root rather than by forcing production
semantics onto the migration's child phases.
The test block keeps its own photo_sharing_test default — isolation is the point
there.
Verified: every block plus the guard resolve identically from the same
environment; explicit DB_* still wins; production's pool tuning is preserved;
and a full SQLite → PostgreSQL migration with NODE_ENV unset lands in the right
database with JSON shapes intact.
Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM
* fix(db): two more components that guessed the database instead of asking (#1038)
Both found while sweeping for copies of the connection defaults. Checked in
detail first — one of my suspicions about them was wrong.
scripts/set-admin-password.js hand-rolled its own knex config while all four
sibling scripts (reset-admin-password, create-admin, show-admin-credentials,
reset-admin-mfa) use the application's connection. Two consequences:
- it read DB_CLIENT, a variable nothing else in this codebase sets, so it
defaulted to Postgres and could not work on a SQLite install at all;
- it defaulted to database `picpeak_dev`, a name no other component uses.
It now uses `require('../src/database/db')` like its siblings, so it follows
whatever engine the install actually runs on. Timestamps are written as ISO
strings because it reaches SQLite now, where raw Date objects are the documented
landmine.
NOT changed: the script's "all existing sessions have been invalidated" notice
is accurate — auth.js compares token iat against password_changed_at — and it
deliberately leaves must_change_password alone, which is right for an operator
choosing a password rather than being issued one.
routes/adminSystem.js re-derived three things the live connection already knows,
and each could disagree with it:
- the engine, from DATABASE_CLIENT || 'sqlite3' — so a Postgres install
without an explicit DATABASE_CLIENT took the SQLite branch;
- the Postgres database, from DB_NAME || 'picpeak';
- the SQLite file, from a hardcoded ../../data/photo_sharing.db that ignored
DATABASE_PATH entirely.
All three now come from db.client.config, with pg_database_size(current_database()).
Verified: set-admin-password works on SQLite (new hash verifies, old rejected)
and still on PostgreSQL; and on a SQLite install with a custom DATABASE_PATH the
size logic reports the real database (1,748,992 bytes) where the old code
reported a different file entirely (1,851,392) — or 0 where that path does not
exist.
Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM
* fix(db): bind the migration marker to its target; fix a phantom table (#1038)
Review round 15.
1. The marker records `host:port/database`, but only its EXISTENCE was checked.
Repoint DB_NAME or DB_HOST at a different, empty PostgreSQL after migrating
and the marker would vouch for that one too — booting it, presenting an empty
installation, and suppressing the SQLite fallback while the real data sits in
the recorded target and the renamed rollback copy. The marker is compared
against the current connection now, and a mismatch stops the boot with both
targets named and the two ways out.
2. `incoming_invoices` is not a table — supplier documents live in
`inbound_documents` (core migration 124). Both occupancy lists skip tables
that do not exist, so those records were silently not protecting anything:
an install whose only remaining data was inbound documents could be switched
away from, or overwritten without --force. Verified every other name in the
lists against the live schema at the same time.
Verified: a marker naming picpeak_original with picpeak_mk configured refuses
with exit 3 and prints both; making them agree boots pg.
Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
Enabling Guest Feedback on an event could silently do nothing.
1. `updateEventFeedbackSettings` spread the request body straight into the
knex UPDATE. The admin event form posts its whole client-side state,
including three keys that were never columns on event_feedback_settings
(`enable_rate_limiting`, `rate_limit_window_minutes`,
`rate_limit_max_requests`), so the write threw and the route answered 500.
Writable columns are now whitelisted; identity columns and timestamps stay
server-managed.
2. EventDetailsPage swallowed that 500 in a bare `catch {}` ("Error already
handled by mutation" — it is a different request), so the admin was left
looking at "Event updated successfully" while the toggle never persisted.
The error is surfaced now and the settings query is invalidated on success.
3. gallery.js declared a duplicate `GET /:slug/feedback-settings`. server.js
mounts galleryRoutes before galleryFeedback, so it shadowed the real
handler and dropped the per-guest caps (#655) from the guest payload — the
gallery could never render the favorite/like limits or their counters.
Timestamps are written as ISO strings so they round-trip on both engines.
Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
SQLite stores booleans as 0/1, Postgres as true/false. The guest gallery
compared strictly against `true`/`false`, so every flag read backwards on
SQLite installs:
allow_downloads: 0 !== false → true (header Download button shown
with downloads disabled)
allow_user_uploads: 1 === true → false (upload button hidden with
uploads enabled)
Worse, all five download guards used `allow_downloads === false`, which never
fires against a stored 0 — so on SQLite "Allow photo downloads = off" was
inert end to end: single photo, download-all, download-selected, download-jobs
and the job-status poll all kept serving, as did the secure-images download
route. Per-category blocking (#640) was ignored for the same reason, the
protection toggles (right-click, devtools, canvas, watermark) reported false
while enabled, overlay_protection was stuck on, and show_feedback_to_guests
leaked feedback with the setting off.
The /info endpoint was already correct — it checks 0/'0' explicitly. The two
payloads had simply drifted. Everything now goes through parseBooleanInput
(utils/parsers.js), which normalises both engines and takes a per-column
default so legacy NULL rows keep their documented behaviour.
Tests run on the SQLite harness, so they assert the real engine values. Every
one of them fails on the unfixed code — the payload assertions return the
inverted value, and the guard assertions never get their 403 (the request
proceeds to serve instead).
Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
Clearing a gallery's expiration failed on every SQLite install with
SQLITE_CONSTRAINT: NOT NULL constraint failed: events.expires_at
surfacing in the admin UI as "Failed to update event".
Migration 061 added the event_require_event_date / event_require_expiration
settings and dropped the NOT NULL on both columns — but only for Postgres. It
skipped SQLite on the premise that "SQLite doesn't enforce NOT NULL as
strictly", which is untrue, so "never expires" was never reachable there. The
#426 work that allows clearing the expiration on edit therefore never worked
on SQLite either.
Migration 174 finishes 061 for SQLite. Knex implements .alter() on SQLite by
recreating the table; migration 073 already does that on `events`, so the path
is well-trodden. Postgres is skipped — it was handled in 061 and .alter() there
would needlessly rewrite a column that is already correct.
The test asserts against the real engine (the Jest harness runs SQLite) and
reproduces the reporter's exact error without the migration.
Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
main already ignores `backend/storage/` wholesale; stable only ignored
`backend/storage/business-docs/`. A dev instance writes event photos,
thumbnails and previews into backend/storage/, so `git add -A` on this
branch sweeps 17 runtime artifacts into the commit.
Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
Backport of #1018 to the curated channel; the reporter on #1015 is running v3.45.14.
The slideshow resolved its image as preview_url || hero_url || url. preview_url is only emitted when lightbox_preview_enabled is on (default false), so a default install fell through to hero_url — the 1920x1080 fit:'cover' centre crop built for gallery header banners. object-fit: contain then letterboxed an already-cropped 16:9 frame.
Emits slideshow_url (same aspect-preserved preview tier) unconditionally for image photos; the show prefers it and never falls back to hero_url. preview_url stays gated so the lightbox opt-in is unchanged.
Backport of #1013 to the curated channel. Both are production dependencies of the backend image (npm ci --omit=dev):
- nanoid 3.3.16 -> 3.3.18 (CVE-2026-67213, infinite loop in customAlphabet)
- js-yaml 4.3.0 -> 4.3.1 (GHSA-5p4m-2wfm-xmqj, quadratic CPU in !!omap resolution)
Stable reported no open alerts only because its last Trivy scan ran on 2026-08-04 with v3.45.14, before either advisory was published — the vulnerable versions were present in the lockfile regardless.
Lockfile-only; the existing ^ ranges already permitted both fixes.
Backport of #991. stable carried the identical code path and the same missing
guards.
A scoped admin could point a quote or contract at a project they do not own —
the quote/contract create+update paths pass a body-supplied projectId with no
ownership check, and linkDealToProject's lineage guard is skipped when the deal
has no event yet. On an ownerless project this escalated to a read once the
quote converted to an event.
Vetted at the service choke point, ahead of both the null-deal early return and
the customer check. 404 PROJECT_NOT_FOUND throughout. super_admin unaffected.
Backport of #987. stable carried the same vulnerable versions.
brace-expansion 5.0.8 -> 5.0.9 CVE-2026-69152 (high)
ip-address 10.2.0 -> 10.4.0 CVE-2026-69192 (high), CVE-2026-54272,
CVE-2026-69198 (medium) — SSRF and
trust-boundary bypasses
postcss 8.5.18 -> 8.5.23 CVE-2026-69153 (medium)
Lockfile holds exactly one entry per package, all at or above the fixed
version; the image installs via npm ci --omit=dev.
Closes#969 on stable. Backport of #976.
The cockpit's email feed rendered preview/resend/cancel/retry/send-now for every mail regardless of role or permission, producing 404s (CRM document mail has no event_id; project ownership does not imply event ownership) and 403s (preview needs events.view, the write actions need email.send).
getProjectOverview now stamps each email with an authoritative canAct, mirroring filterOwnedEventIds; created_by is selected only for that check and stripped before the response. A missing canAct reads as false.
Closes#968 on stable. Backport of #974.
The roles-join fallback in adminAuth fabricated role_name='super_admin' on ANY database error, so a transient fault silently granted super_admin for its duration. Gate it on isMissingRolesSchema(), moved to utils/dbErrors.js and shared with apiTokenAuth, with the predicate tightened to trust SQLSTATE 42P01/42703 on Postgres and exact driver phrasing on SQLite.
* fix(security): enforce project ownership (GHSA-wrg5, GHSA-93x4)
Project routes authorized on generic events.view / events.edit with NO
ownership check, so an editor-like admin could enumerate, read, update and
aggregate projects belonging to other admins' events. The project email
endpoints keyed on an email_queue id alone — any admin with events.view /
email.send could preview, resend, cancel or retry ANY queued mail by walking
ids.
The earlier 'needs a migration, deferred' assessment was wrong in one
direction and right in another: ownership IS derivable transitively via
events.project_id -> events.created_by, but only for projects that already
have a linked event. A brand-new EMPTY project has no derivable owner, which
is exactly where the create -> attach flow starts. So migration 167 adds
projects.created_by (backfilled from the single linked event owner, skipping
ambiguous multi-owner projects) and createProject finally persists the adminId
it was already being passed.
- ownedProjectIds(): union of the stored owner and the transitive path, so
pre-167 rows and new empty projects both resolve. Reads created_by
defensively so an instance that hasn't run 167 falls back to the transitive
rule instead of throwing.
- requireProjectOwnership on detail/update/attach-event/attach-quote/
attach-contract/overview; list filtered by an id allowlist (empty array
means 'owns nothing' and must return no rows, hence null-vs-[] care).
- POST /:id/events also validates the INCOMING eventId — owning the project
is not enough, or an editor could pull a foreign event in and read its
rolled-up documents via /:id/overview.
- Queued-email routes scoped via email_queue.event_id. CRM document mail has
event_id NULL and no ownable parent here, so a scoped caller is denied
rather than guessed into access. 404 (not 403) so it isn't an id oracle.
Note: adminEmail.js:315/332 let any email.view/edit admin archive or delete
any email_queue row — the same class, pre-existing and outside these two
advisories. Left untouched and reported rather than silently widened.
* fix(security): codex round 2 — make the stored project owner authoritative (GHSA-wrg5)
The first predicate union'd 'any linked event I can see' with the stored
owner, which opened two holes:
- A project owned by admin B containing ONE legacy ownerless event became
readable by every admin — and /:id/overview aggregates B's other events,
invoices and emails, so a single legacy event exposed the whole project.
- Migration 167 deliberately leaves multi-owner (ambiguous) projects NULL
rather than guessing an owner. A NULL owner was then treated as
'everyone's', so exactly those mixed projects became globally accessible.
Now: the stored created_by wins outright, and a project without a usable
stored owner only derives access when EVERY linked event is accessible (and at
least one exists). A created_by pointing at a hard-deleted admin degrades to
'no usable owner' so the project falls back to its events instead of being
locked away — no ON DELETE SET NULL migration needed. A project with neither a
usable owner nor linked events stays super_admin-only: failing closed beats
failing open, and a super_admin can reassign it.
Also returns a knex SUBQUERY rather than a materialised id list, so a large
project count can't hit the driver's bind-parameter limit.
* fix(security): codex round 3 — enforce deal-lineage ownership on project attach (GHSA-wrg5)
requireProjectOwnership vets only the DESTINATION project, while attaching a
quote or contract cascades through linkDealToProject — which re-points every
event the deal produced into that project. An editor could therefore create an
empty project of their own, attach another admin's quote, and pull that admin's
events (plus the invoices, emails and gallery that roll up with them) into a
project they own and can read via /:id/overview. The single-customer guard did
not stand in the way: an unassigned project ADOPTS the deal's customer rather
than rejecting it.
linkDealToProject now refuses to move lineage events the actor cannot own, and
assignDocument cascades BEFORE stamping the document so a refused attach leaves
nothing half-applied (the old order committed the foreign document into the
caller's project and only then declined the cascade). The quote/contract
create+update paths, which reach the same cascade with an arbitrary project_id,
thread their adminId through as well; isSuperAdmin() resolves the role for them
and fails closed when it cannot.
Events are the only ownership signal a deal carries — quotes and contracts have
no created_by in this schema — so a lineage that produced no event still cannot
be attributed. That is a property of the CRM model, noted in the code.
Claude-Session: https://claude.ai/code/session_01F211U4dDbEj4zXiyKbi9me
(cherry picked from commit 688e318850db1b5f4ea2a4ae3c0fcf0fc137620d)
* docs(security): drop the stale ownership JSDoc left by the rebase (GHSA-wrg5)
Rebasing onto stable (which had gained scopeEventsQuery from #963) replayed the
round-1 doc block above round-2's replacement, leaving a comment that describes
the ORIGINAL union rule — "a project is the caller's when … it has at least one
linked event they own" — directly above the code that deliberately no longer
does that. That union is the hole round 2 closed; a comment asserting it is
worse than none.
Claude-Session: https://claude.ai/code/session_01F211U4dDbEj4zXiyKbi9me
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* fix(security): escape brand tokens, block tracker redirects, trim logo diagnostic (GHSA-j347, mw76, 29vm)
GHSA-j347 — buildCachedPayload sanitizes the operator's HTML and THEN runs
applyBrandTokens over the result with a plain String.replace, so any markup in
a token value reached the public origin unfiltered. The default templates
interpolate tokens into text AND into quoted attributes
(<img src="{{brand_logo_url}}" alt="{{company_name}} logo">,
href="mailto:{{support_email}}"), so a value could close the attribute and
inject. Token values are now HTML-escaped on substitution, mirroring
galleryOgService's escapeHtml. sanitizeBrandUrl's case-sensitive literal
'javascript:' check (which 'JavaScript:' walked straight past) is replaced by
an http/https scheme allowlist; relative logo paths are unaffected.
Writer is settings.edit (super_admin only) and the CSP blocks inline script,
so this is defence-in-depth — but sanitize-then-substitute is a real ordering
bug regardless.
GHSA-mw76 — the SSRF decline STANDS: self-hosted operators legitimately point
analytics at private addresses, so connection-time IP blocking would break real
deployments. Fixed only the narrow leak: undici strips
Authorization/Cookie/Proxy-Authorization/Host across a cross-origin redirect,
but umamiAdapter sends a CUSTOM x-umami-api-key header, which would be replayed
verbatim to the redirect target. Both adapters now use redirect: 'error'.
GHSA-29vm — the logo diagnostic echoed absolute storage roots, process.cwd()
and absolute candidate paths. It now reports candidates relative to
<STORAGE>/<CWD_STORAGE>, which answers the same 'which candidate existed'
question. It also still advertised the raw-absolute candidate that GHSA-c7x5
removed from resolveLogoFile, so it was misreporting what the resolver tries —
aligned with the real candidate list.
publicSiteService.test.js expectation updated: an '&' in a company name is now
emitted as '&'. Renders identically; the raw payload string differs.
* fix(security): codex round 2 — stop the remaining logo-path disclosure, mirror the resolver (GHSA-29vm)
- sources[].value was still echoed verbatim. branding_logo_path is stored
ABSOLUTE by multer, so relativising only resolvedTo and the candidate paths
left the filesystem layout going out anyway. It is now relativised too.
- Round 1 dropped the raw-absolute candidate on the grounds that GHSA-c7x5
removed it from resolveLogoFile — but the c7x5 follow-up RE-ADDED it (kept,
subject to the containment filter, so a legitimate multer path still
resolves). The diagnostic therefore reported every candidate as missing for
a contained absolute logo while resolvedTo named the file. It now mirrors the
resolver, containment filter included.
One deliberate cosmetic divergence, commented in place: for an absolute value
the resolver also tries path.join(root, value-minus-leading-slash), which can
never exist and would re-embed the absolute path this endpoint must stop
echoing. Omitted; every candidate that can actually match is still shown.
* fix(security): codex round 3 — mirror the resolver for root-relative logo paths (GHSA-29vm)
The logo diagnostic skipped the `<STORAGE>/<value>` candidates whenever
path.isAbsolute(value) was true. That test cannot distinguish a multer disk
path from a root-relative URL such as `/custom/logo.png`, and for the URL form
resolveLogoFile.generateCandidates() does try `<STORAGE>/custom/logo.png` and
can resolve it — so the endpoint reported "no source candidate exists" about a
logo that renders fine, and collapsed the configured value to its basename.
The stripped joins are now built unconditionally, exactly as the resolver does.
Disclosure stays closed: every candidate still passes the containment filter and
redact() rewrites survivors to `<STORAGE>/…`, never an absolute host path.
Claude-Session: https://claude.ai/code/session_01F211U4dDbEj4zXiyKbi9me
(cherry picked from commit 093480a753ff3d4b6ed48dd9f1108f975c8e0d47)
* fix(security): gate the logo stripped-joins on containment, not isAbsolute (GHSA-29vm)
The previous commit dropped the isAbsolute() gate entirely and regressed
logoDiagnostic's own disclosure assertion: for a genuine multer disk path,
path.join(root, value-minus-leading-slash) yields
`<STORAGE>/tmp/…/storage/custom/logo.png`, and redact() only rewrites the
LEADING root — so the inner absolute path went straight back into the payload.
The right discriminator is not "is this absolute" (which cannot separate a disk
path from a root-relative URL) but "does the value already resolve inside a
storage root". If it does, it is a real disk path, the raw candidate already
covers it, and the stripped join is the double-prefixed junk that can never
exist. If it does not — the `/custom/logo.png` URL form — the stripped join is
exactly what resolveLogoFile resolves, and is shown.
Covered by a new case asserting both halves: the candidate appears for the URL
form, and the payload still contains neither the storage root nor cwd.
Claude-Session: https://claude.ai/code/session_01F211U4dDbEj4zXiyKbi9me
(cherry picked from commit c6b95d3cd1cb28e5c2828d29d4d63fadad981dcf)
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* 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 <paul@MacStudio-von-Paul.local>
* 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 <paul@MacStudio-von-Paul.local>
* 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 <paul@MacStudio-von-Paul.local>
* 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 <paul@MacStudio-von-Paul.local>
* fix(security): redact gallery share tokens from analytics page-view tracking (GHSA-7m6c)
* fix(security): codex round-1 — actually disable raw auto-tracking (GHSA-7m6c)
The previous patch was inert: App.tsx passed autoTrack:true (so Umami's
data-auto-track=false was never set) and the sanitized trackPageView had no
caller (useAnalytics sits outside <Router>), so the raw token URL still hit
the collector.
- Umami: drop autoTrack:true → data-auto-track=false; page views now come
from a sanitized manual tracker.
- Rybbit: its initial-load auto pageview can't be intercepted client-side, so
use native data-mask-patterns=['/gallery/**'] to strip the token on every
auto-tracked view; skip manual tracking for it to avoid double counting.
- Mount <AnalyticsRouteTracker/> INSIDE <Router> so manual tracking runs.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* 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 <paul@MacStudio-von-Paul.local>
* 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 <paul@MacStudio-von-Paul.local>
* 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 <paul@MacStudio-von-Paul.local>
* 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 <paul@MacStudio-von-Paul.local>
* 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 <paul@MacStudio-von-Paul.local>
* 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 <paul@MacStudio-von-Paul.local>
* fix(admin): stop marking events expired up to 24h early (#909) (stable)
differenceInDays truncates to whole days, so an event expiring in a few
hours returned 0 and three admin surfaces treated it as gone:
- EventsListPage: status chip said 'Expired' (days <= 0) while the
public gallery — which compares real timestamps — correctly showed
'expires in X hours'. This is the reporter's exact symptom.
- EventDetailsPage: same isExpired math on the detail view.
- AdminDashboard: the expiring-soon card showed '0 days left' on the
final day.
Expired is now gated on the actual timestamp (expires_at <= now), and
the countdown chips use ceiling days so the last day reads '1 day
left' instead of flipping to Expired/0.
* fix(admin): drop already-expired events from the dashboard card (#909 review round)
The expiring-soon card ran Math.max(1, ceil(delta)), so an event that
expired while the dashboard sat open (its query isn't polled) showed
'1 day left' indefinitely from the stale cached row. Expired rows are
now filtered out before render; the delta is therefore always positive
and the clamp is gone.
* fix(admin): refresh expiry status live at the boundary (#909 review round 2)
Two review findings on the admin expiry surfaces:
- The dashboard 'expiring soon' card, list badges, and detail banner are
all computed inline from Date.now() at render, so a page left open
across an event's expiry kept showing 'active'/'1 day left' until an
unrelated render — which for editor/viewer roles (no health poll)
never happens.
- My round-1 client-side filter on the dashboard desynced the visible
list from the cached total/stat ('no events expiring' beside 'view
all N').
Both are fixed by new useExpiryRefresh: it fires once at the soonest
future expiry (setTimeout, overflow-guarded). The dashboard refetches
its expiring + stats queries — the backend already excludes expired
events, so rows/total/stats come back consistent (filter removed). The
list and detail pages bump a tick so the inline badges recompute. Hooks
are placed above the loading early-returns (rules-of-hooks is disabled
in eslint, so this was a latent crash otherwise).
* fix(admin): expiry-refresh precision + filtered refetch (#909 review round 3)
Three refinements to round-2's live-expiry work:
- useExpiryRefresh now re-arms past setTimeout's ~24.8-day overflow
limit (capped wake-up that re-evaluates) instead of dropping the timer,
so a page mounted for weeks still updates.
- The dashboard requests the expiring list ordered by expires_at asc, so
the five shown rows ARE the soonest to expire — the timer schedules
against the true next boundary even when >5 events are expiring
(getEvents gains optional sortBy/sortOrder; backend already whitelists
expires_at).
- EventsListPage refetches instead of only re-rendering at the boundary:
under the 'expiring' filter the backend drops expired rows, so a plain
tick would leave a stale 'Expired' row + total. refetch keeps rows and
totals correct under every filter.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* ci: batch stable releases into one daily version (stable)
The stable release PR was auto-merged the instant it went green, so a
day with N bugfixes produced N patch releases (3.45.8 AND 3.45.9 on
2026-07-29 alone) — N upgrade notifications for stable users and N
full Docker build cycles.
Fixes now accumulate in release-please's rolling release PR and are
cut as ONE version per day by release-stable-daily.yml (18:00 UTC).
Approval/merge mechanics are unchanged from the inline step (#719):
approve as github-actions[bot], auto-merge as the PAT so the merge
triggers the tag-cutting run.
- Urgent fix? workflow_dispatch the daily job or merge the release PR
by hand — the schedule is a default, not a gate.
- Beta is untouched: instant beta releases are load-bearing for
same-day reporter verification.
- schedule only fires from the default branch; the stable copy of the
new workflow is inert and exists to keep branches in sync.
* ci: harden the daily stable-release cut (review round) (stable)
Mirror of the #919 hardening — fork-PR head-name spoof (require
--base stable + same-repo head) and no longer swallowing the
auto-merge-enable failure on the sole automatic stable cut.
* ci: accept an immediately-merged release PR as success (review round 2) (stable)
Mirror of #919: MERGED state = success (the normal 18:00 case where
checks were already green and --auto merges immediately), pending
auto-merge = success, still-open-no-auto-merge = real failure.
* ci: read release-PR state + auto-merge in one snapshot (review round 3) (stable)
Mirror of #919 — collapse the two racing gh pr view calls into one.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
* 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 <paul@MacStudio-von-Paul.local>
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 <paul@MacStudio-von-Paul.local>
* 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).
Triggers the correct stable release from the stable branch (manifest
3.44.0 -> 3.45.0). Same fix as #774 (which fixes it on main for future
promotes); merging this to stable is what re-runs release-please
correctly for the promote that mis-fired as v2.7.0.
Same one-liner as #772 — adds stable to tests.yml push/pull_request
filters so the required backend/frontend checks report on this PR
instead of hanging on 'Expected — Waiting for status to be reported'.
Merge main (v3.83.0-beta.0) into stable to cut the next stable release.
Conflicts resolved toward main (the promoted code); stable release-control
files (manifest, CHANGELOG) restored separately.
- 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.
CustomerPicker uses its 'label' prop as the selected-customer chip text, so
passing the static 'Customer' string hid the actual name. Pass the resolved
customer's name as label and add a separate field heading.
The toolbar doc buttons now open a real document-action flow instead of just
loading an email template:
- DocumentActionModal resolves the customer from the message's sender address
(customers/search); if no match, the CustomerPicker lets you search or create
a passive customer inline.
- Create new -> jumps to the real editor prefilled with the customer
(quotes/contracts/bills ?customerAccountId=), so numbering, line items and PDF
all come from the existing CRM. Gallery opens the event editor.
- Select existing -> lists that customer's quotes/contracts/invoices and drops
the chosen document number into a reply composer.
- Toolbar buttons are gated by the global feature flags (quotes/contracts/bills).
Adds the missing customerAccountId prefill to ContractEditorPage (quotes + bills
already had it). Frontend-only; reuses existing endpoints. Build verified.
Addresses dev-test feedback:
- Sidebar + reading-pane addresses (rechnungen@ / hello@ / no-reply@) are now
read from the mail config via GET /admin/email/identities, not hardcoded.
- Highlight/selection now uses the branding accent (bg-accent-soft /
text-on-accent-soft / accent-dark) instead of hardcoded blue, so it follows
the admin's CI colour like the sidebar.
- Header gains "New message" (compose) and "Sync" (poll mailboxes now) buttons.
- Composer modal enlarged (920px, taller editable body).
- Customer mailbox (hello@) now has BOTH incoming (IMAP) and outgoing (SMTP)
settings — migration 156 adds smtp_* + from_* to mail_accounts;
emailProcessor.sendRawEmail takes an accountKey and sends via that mailbox's
SMTP identity (falls back to the global from). Manual/reply sends from the
Messages UI use the 'customers' identity, so replies come from hello@.
Frontend build + migration boot (156) verified.
The Messaging FeatureCard was a hardcoded-disabled 'roadmap' placeholder
(no-op toggle), so the messaging flag could never be turned on — the Messages
sidebar item + page stayed hidden. Wire the toggle to setFlag, mark it 'new',
and describe the actual admin Messages client.
The CRM action buttons and Reply now open a send-composer, not a silent
templated send.
- New send-composer (MessageComposer): loads the rendered template (via
previewTemplate) or a reply stub into a fully-editable body — the admin can
rewrite it or drop a note anywhere before sending. On send it goes out as-is
(server-sanitized), no template re-render.
- Backend: emailProcessor.sendRawEmail() sends admin-edited HTML via the
configured SMTP identity; POST /admin/email/send sanitizes + sends + records
the message in email_queue as a 'manual' send.
- Migration 155: email_queue.origin ('system' default | 'manual'). The Sent
stream now splits by origin — Automated ▸ Sent = system, Customers ▸ Sent =
the human/edited messages (which finally populates that folder). /queue gains
an origin filter + returns origin.
- Toolbar wired: Reply enabled on inbound customer mail (prefilled + quoted);
Create Quote/Contract/Invoice open the composer with that template loaded;
Gallery opens a blank compose. Accounting/Forward/Archive/Delete stay disabled
(later phases). After send, jumps to Customers ▸ Sent.
Deferred to a later phase: two-way IMAP write-back; per-identity SMTP (manual
sends currently use the global from address). Frontend build + migration boot
verified.
Address review on #764: backfillDunningRuns emitted invoice.sent without a
target, so enabling dunning would also enroll every historical open invoice
into any custom invoice.sent flow. Pass the enabled flow's id through to
emitWorkflowEvent so the backfill only touches dunning. Also note the
computeWakeAt both-fields (untilVar + delay) behaviour change in its comment.
Second inbound mailbox and real message bodies for the Messages viewer.
Backend:
- Migration 154: mail_accounts table (additional inbound mailboxes beyond the
primary accounting IMAP) + received_emails.{account_key,to_address,body_html,
body_text}. Additive/guarded.
- emailIntakeService now polls the accounting mailbox AND every enabled
mail_accounts row. Extracted pollAccountOnce(cfg, {accountKey, routeToExpenses});
accounting keeps its exact attachment->expenses behavior, customer mail is
logged with its body and NOT routed to accounting. Inbound HTML is sanitized
server-side (sanitize-html) on ingest.
- adminEmail: /received gains an account filter + returns account_key/to_address
(bodies excluded from the list); new GET /received/:id returns the body;
GET/POST /accounts + /accounts/test manage the extra mailboxes.
Frontend:
- Customers inbox now pulls the hello@ mailbox; reading pane renders the
sanitized body in a strict (script-less, no same-origin) sandboxed iframe.
Accounting inbox shows bodies too. Toolbar context keys off the mailbox.
- CustomerMailboxCard in Settings -> Email (behind the messaging flag) to
configure + test the hello@ IMAP box.
No behavior change to the existing accounting inbound flow. Frontend build +
migration boot verified.
New admin "Messages" page — a three-pane mail viewer over the mail picpeak
already stores, feature-flagged behind `messaging` (default off):
- Sidebar account tree: All mail / Customers (hello@) / Accounting (rechnungen@)
/ Automated (no-reply@), matching the agreed IA.
- Automated + All Sent = email_queue (listQueue); Accounting + All Inbox =
received_emails (listReceived). Customers folders show an explanatory empty
state pending the hello@ mailbox (Phase 2).
- Reading pane renders the sent body from rendered_html (migration 119) in a
sandboxed iframe; new GET /admin/email/queue/:id returns body + cc +
attachment filenames (disk paths never exposed).
- Received supplier invoices: envelope + rasterized PDF viewer reusing the
accounting inbound blob endpoint, plus "Open in Accounting inbox".
- Context toolbar (Reply/Forward/Create Quote-Contract-Gallery-Invoice /
Book-as-expense-Re-bill) present but disabled — wired in later phases.
Reuses email.service, accounting inbound blob endpoint, RequireFeature +
PermissionGate (email.view), Tailwind dark: theming. No schema change.
Enabling the invoice-dunning built-in suppressed the legacy reminder ladder
but only created runs for invoices sent AFTER enabling — already-sent unpaid
invoices got dunned by neither. Now:
- Turning dunning ON enrolls every open sent/overdue unpaid invoice via
emitWorkflowEvent('invoice.sent') (engine.backfillDunningRuns(), wired into
the workflow enable toggle). Idempotent via the per-(flow,entity) dedup.
- The grace wait is anchored to the invoice's due date: computeWakeAt now
treats { untilVar, delayDays } as "var + offset" (was var-only OR now+offset),
and the built-in's waitGrace becomes { untilVar: 'dueDate', delayDays:
firstDays } (seed v6 -> v7). An already-overdue invoice duns on its real
timeline instead of restarting a fresh grace clock.
Note: the due-date-anchored graph applies to freshly seeded built-ins; an
already-admin-enabled dunning workflow still enrolls via backfill but keeps its
current grace timing until re-seeded.
Resolves 6 file conflicts arising from main carrying 7 weeks of
stable-channel work (security backports, release-please cuts, README
rewrite #281) that hadn't been forward-merged into beta.
Resolution per file:
- backend/package.json + package-lock.json — kept beta's version.
Beta is the superset; it intentionally drops `handlebars` (PR #367
removed the runtime require; the dep was the source of 2 criticals
+ 8 highs). Security-pinned versions (axios 1.15.2, nodemailer ^8,
i18next-http-backend ^3.0.2, multer ^2.0.2, tar >=7.5.13) already
match across both branches — no security regression.
- frontend/package.json + package-lock.json — kept beta's version.
Superset of main (adds marked, @types/node, i18next-cli, memfs,
i18n CLI scripts). Same security versions on both sides.
- README.md — kept main's version. PR #281 was an explicit cleanup
("shorter, cleaner, less AI-sounding"); beta had grown the file by
326 lines ad-hoc during the freeze. Preserving the rewrite.
- CHANGELOG.md — kept main's version. Release-please regenerates from
conventional commits on its next stable cut, so beta's accumulated
entries will roll into the new v3.55.0 release block automatically.
Auto-merged files carrying main's session-invalidation fix (#245)
flowed cleanly into beta's versions — sessionTimeout.js, adminAuth.js,
and the test files all merged without conflict, meaning beta had
already absorbed equivalent changes by independent paths.
CI on the underlying merge state was green on PR #568 prior to this
resolution; will re-run automatically on push.
Closes the open Trivy code-scanning alerts for app-side dependencies.
The npm-bundled CVEs in /usr/local/lib/node_modules/npm (picomatch,
brace-expansion, ip-address inside the Node image itself) are deferred
to a separate Node-base-image PR — they're build-environment-side and
need their own compatibility testing.
| Package | From | To | CVEs cleared |
|---|---|---|---|
| axios (backend + frontend) | 1.14.0 | 1.15.2 | CVE-2026-42264 (HIGH), CVE-2026-42043 (HIGH), CVE-2026-42035 (HIGH), CVE-2026-42033 (HIGH), CVE-2026-42044, CVE-2026-42042, CVE-2026-42041, CVE-2026-42040, CVE-2026-42039, CVE-2026-42038, CVE-2026-42037, CVE-2026-42036, CVE-2026-42034, CVE-2026-40175, CVE-2025-62718 |
| nodemailer (backend) | ^7.0.13 | ^8.0.5 | GHSA-vvjj-xcjg-gr5g, GHSA-c7w3-x93f-qmm8 |
| i18next-http-backend (frontend) | ^3.0.2 | ^3.0.5 | CVE-2026-41691 |
| uuid (backend) | ^11.1.0 | ^11.1.1 | CVE-2026-41907 |
| postcss (frontend, devDep) | ^8.4.21 | ^8.5.10 | CVE-2026-41305 |
For transitives whose direct parents haven't released a version that
picks up the patched range, pinned via npm overrides:
| Package | Min | CVE |
|---|---|---|
| follow-redirects (backend + frontend) | >=1.16.0 | GHSA-r4q5-vmmm-2653 |
| fast-xml-parser (backend) | >=5.7.0 | CVE-2026-41650 |
| @tootallnate/once (backend) | >=3.0.1 | CVE-2026-3449 |
| ip-address (backend) | >=10.1.1 | CVE-2026-42338 |
PR #268 originally pinned axios to 1.14.0 to avoid a supply-chain
attack on a specific compromised version range. The 1.15.x series
are post-incident upstream releases — clean. Confirmed with the
maintainer before bumping.
* `npx tsc --noEmit` (frontend) — clean
* `npx vite build` (frontend) — clean (~4s, existing bundle-size
warning, not new)
* Backend module-load smoke test — all critical modules load
(`auth`, `adminAuth` middleware, `emailProcessor`, `recaptcha`,
`storage`) with the new axios + nodemailer
* Lockfile re-verification — every targeted CVE now resolves to
the patched version range
* npm-bundled CVEs inside `/usr/local/lib/node_modules/npm/` —
picomatch CVE-2026-33671 (HIGH), CVE-2026-33672, brace-expansion
CVE-2026-33750, ip-address (npm-internal) CVE-2026-42338. These
live in the Node base image and require a Node base image bump
with its own compatibility testing — separate PR.
Targeting `beta` so the bumps go through the normal release-please
flow before promotion to `main`.
Stable release promoting the entire `beta` channel to `main`. Brings
~300 commits of features, fixes, and infrastructure improvements that
have been baked on the beta channel since v2.6.5.
## Major themes since v2.6.5
* Multi-administrator support with RBAC (super admin / admin / editor)
* Async upload pipeline (background worker pool for sharp/ffmpeg/EXIF/
watermark/webhooks; bytes-on-wire returns 202)
* Self-hosted webfonts (filesystem-driven scanner; replaces Google Fonts
CDN; GDPR-compliant)
* 8-token CI palette + force color mode (full theming across admin and
public site, with WCAG-safe contrast helpers)
* Native multi-arch Docker images (Apple Silicon + ARM64 Linux native)
* Native S3 storage backend (S3 + S3-compatible providers)
* Comprehensive video support (MP4/WebM/MOV upload, stream, play)
* Outbound webhooks for event/photo lifecycle (HMAC-signed)
* Gallery layout overhaul (decoupled header style, banner option,
theme-aware skeletons, lazy-loaded folder tree picker)
* Multilingual email templates (EN/DE/NL/PT/RU translations table)
* Bulk operations (delete with password gate, archive)
* Photo dimensions backfill (true masonry layout)
* Customer client access (review area before guest share)
* Image security (devtools detection, watermarking, right-click,
secure thumbnails)
## Notable bug fixes from beta
* `/auth/session` symmetry — three rounds of fixes (#350, #355, #363,
#398) for the admin-login redirect-loop family
* Email template renderer: handle {{#if}} conditionals, fix CSS leak in
plain-text fallback, gate publish-from-draft password placeholder,
gate external_url in public response
* Caller/template variable drift across gallery_created,
expiration_warning, archive_complete, gallery_expired
* Full-URL gallery_link in all email types (was path-only in 3 sites)
* ffmpeg/ffprobe via apk for Alpine compatibility (was glibc-bundled)
* Admin events search and counters not bounded to first 100 (#346)
## Conflict resolution notes
* `README.md` — kept main's leaner v2.6.5 rewrite (#281); added a
Contributors section adapted from PR #393.
* `DEPLOYMENT_GUIDE.md` — beta version (more recent, includes External
Media docs already backported to main).
* `CHANGELOG.md` — new 3.42.1 entry leads, beta's 3.x history follows,
main's 2.x entries appended below a divider so the historical chain
is preserved.
* `package.json` (backend + frontend) — beta's structure with version
bumped from `3.42.1-beta.0` → `3.42.1`.
* `package-lock.json` (backend + frontend) — regenerated via
`npm install --package-lock-only`.
* `.release-please-manifest.json` — bumped from `2.6.5` → `3.42.1` so
the next release-please run on main starts from the correct base.
## Pre-flight checks
* Frontend `tsc --noEmit` — clean
* Frontend `vite build` — clean (~3.5s, 2.6 MB main chunk; existing
warning about chunking, not new)
* Backend `npm test` — pre-existing failures in 6 integration suites
(DB-fixture-dependent, not regressions)
* Frontend `vitest` — pre-existing failures in
ThemeCustomizerEnhanced.test.tsx (missing QueryClientProvider after
PR #390 added useQuery; not a regression of this merge)
The pre-existing test failures are tracked as separate follow-ups and
do not block this release promotion.
Add the missing "External Media Library" chapter to DEPLOYMENT_GUIDE.md
that was referenced in the TOC but never written. Covers configuration,
Docker volume mounting, folder structure, usage workflow, limitations,
and troubleshooting.
Closes#270
Axios versions 1.14.1 and 0.30.4 were compromised on March 31, 2026
with a RAT dropper attributed to North Korean threat actor. Pin to
exact 1.14.0 (latest safe release) to prevent resolution to compromised
versions. See https://github.com/axios/axios/issues/10604
- Set password_changed_at when changing password via adminAuth route so
existing JWT tokens are rejected by the auth middleware check
- Enforce session timeout on first request with unseen tokens by checking
token iat against configured timeout (prevents bypass after server restart)
- Convert camelCase roleId/isActive to snake_case role_id/is_active in
frontend updateUser service (fixes silent role update failures)
Resolves GHSA-rqg3-47p5-vgwg
@@ -163,13 +163,14 @@ PicPeak runs on two long-lived branches:
| Branch | Role | What targets it |
|---|---|---|
| **`main`** | Active development. The next release is being assembled here. | Feature PRs. Most bugfix PRs. |
| **`stable`** | Curated release channel. Production-recommended. | Urgent bugfix backports only — small, surgical PRs that land cleanly without dragging in unrelated changes. |
| **`stable`** | Curated release channel. Production-recommended. | Security fixes and regular bugfix backports, kept small and free of unrelated features. |
### Which branch should my PR target?
- **New feature** → target `main`.
- **Bugfix that ONLY affects active dev** → target `main`.
- **Bugfix that current stable users need** → open a small PR against `main`, AND a separate small PR against `stable` with the same change. Keep both surgical so each lands cleanly.
- **Bugfix that current stable users need** → target `main`; regular bug fixes are generally backported automatically to `stable`. Maintainers handle conflicts or create a separate focused backport PR when needed.
- **Security vulnerability** → report privately using [SECURITY.md](SECURITY.md). Security fixes are always released on both `stable` and `main`; coordinate any fix with the maintainers before opening a public PR.
**Hard rule on PR scope**: bugfix PRs against `stable` must be small enough to backport without conflict. Omnibus PRs (e.g. five unrelated sub-features) are fine for `main`, but never for `stable` — they make the next `main → stable` merge painful and break the "stable is always shippable" invariant.
@@ -187,6 +188,6 @@ See [RELEASING.md](RELEASING.md) for the full operational doc (promotion criteri
- Create an [issue](https://github.com/PicPeak/picpeak/issues) for bugs or features
- Join [discussions](https://github.com/PicPeak/picpeak/discussions) for questions
- Security issues: Open a [security issue](https://github.com/PicPeak/picpeak/issues/new?labels=security) on GitHub
- Security vulnerabilities: Follow the [security policy](SECURITY.md) and use [private vulnerability reporting](https://github.com/PicPeak/picpeak/security/advisories/new)
On first start with no `ADMIN_PASSWORD` set, PicPeak has **no admin account yet** and greets you with an in-browser setup screen — no credentials in `.env`:
1. Open **http://localhost:3000/admin** — you'll be redirected to `/setup`.
2.Grab the **one-time setup token** from the backend logs (it's also saved to `data/SETUP_TOKEN`):
2.Read the **one-time setup token** from the 0600 file the backend writes it to
(it is deliberately *not* printed to the logs — that would leave a live
3. Paste the token, set your admin **email + password**, and you're in. The token is single-use, and the setup screen closes permanently once an admin exists.
> Prefer the old behaviour? Set `ADMIN_PASSWORD` in `.env` and PicPeak auto-creates the admin on first boot instead (credentials written to `data/ADMIN_CREDENTIALS.txt`).
@@ -62,13 +62,18 @@ The actual mechanics, in order:
## Hotfix path (backport to current stable)
If a critical bug or security issue affects the current stable and `main` has moved too far for a full promotion to be appropriate, backport just the fix:
Regular bug fixes are generally backported automatically from `main` to `stable`. Keep backports focused on the fix, without unrelated features, and resolve conflicts manually when needed.
**Security fixes are always released on both `stable` and `main`.** Do not wait for a full promotion to deliver a security update. A fix first applied to `stable` must also be forward-ported to `main`; a fix first applied to `main` must also reach `stable`. See [SECURITY.md](SECURITY.md) for the support policy.
When a backport needs manual handling:
1. Create a `security/cve-backport-X.Y.Z` or `fix/critical-X.Y.Z` branch off `stable`.
2. Cherry-pick or hand-write the minimal fix.
3. Open a PR to `stable` with the smallest possible diff.
4. After merge, release-please will propose a patch-level stable release (e.g. `v3.55.1`).
5. **Forward-port the fix to `main`** if it isn't already there. Otherwise the next full promotion will reintroduce the bug.
6. For security fixes, verify that the fix has been published through **both** release channels; merging the code is only part of delivery.
PR #412 ("backport 18 dependency CVE patches from beta") is a worked example of this path (predates the rename; the mechanics are unchanged).
@@ -170,10 +170,12 @@ If you installed with `picpeak-setup.sh` and gave an `--admin-password`, your ad
If you started PicPeak **without** setting `ADMIN_PASSWORD` (e.g. a plain `docker compose up`), there's **no admin yet** and you create it in the browser:
1. Open `http://your-server:3000/admin` — you'll land on a setup screen.
2.Get the **one-time setup token** from the backend logs (also saved to `data/SETUP_TOKEN`):
2.Read the **one-time setup token** from the 0600 file the backend writes it to
(it is not logged — that would leave a live credential in `docker logs`):
constsettle=async()=>{for(leti=0;i<60;i++){awaitnewPromise((r)=>setTimeout(r,50));consts=awaitstatus();if(!s.body.isRunning)returns;}thrownewError('backfill did not settle');};
Some files were not shown because too many files have changed in this diff
Show More
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.