1986 Commits

This Branch

This Branch

All Branches

Author	SHA1	Message	Date
Paul Nothaft	82b412f718	fix(admin): keep header-style tiles from overflowing their cards (stable) (#1423) ... Apply the header-style overflow fix to stable: size the grids from their container width and wrap long translated labels within each card.	2026-09-11 15:37:39 +02:00
Paul NothaftandPaul Nothaft	222b144eed	fix(gallery): bound the cached-zip builder's reads and cap rebuild concurrency (stable) (#1421) ... 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 <[email protected]>	2026-09-11 13:52:32 +02:00
Paul NothaftandPaul Nothaft	7382e13371	fix(gallery): let an admin preview a draft through its short share URL (stable) (#1407) ... * 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 <[email protected]>	2026-09-11 12:14:24 +02:00
Paul NothaftandPaul Nothaft	a5e797e5db	fix(gallery): bound and reclaim storage reads in the guest download routes (stable) (#1416) ... 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 <[email protected]>	2026-09-11 12:06:48 +02:00
Paul NothaftandPaul Nothaft	0732a160b5	fix(gallery): keep videos playable under enhanced and maximum protection (stable) (#1408) ... 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 <[email protected]>	2026-09-11 11:25:24 +02:00
Paul NothaftandPaul Nothaft	b869de33a5	fix(backend): check the revocation store on gallery access (stable) (#1388) ... * 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 <[email protected]>	2026-09-11 11:22:20 +02:00
Paul NothaftandPaul Nothaft	b663f2a994	fix(backend): contain and sanitize the SQLite restore source path (#1393) ... 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 <[email protected]>	2026-09-11 11:22:17 +02:00
Paul NothaftandPaul Nothaft	c008f43a7e	fix(backend): validate the S3 endpoint host before the restore download (stable) (#1391) ... * 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 <[email protected]>	2026-09-11 11:22:13 +02:00
Paul NothaftandPaul Nothaft	167755fdec	fix(backend): use the strong password generator for resets and enforce must_change_password (#1396) ... 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 <[email protected]>	2026-09-11 11:22:08 +02:00
Paul NothaftandPaul Nothaft	050eaf6481	fix(backend): require actor to hold every permission of a role they grant (stable) (#1380) ... * 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 <[email protected]>	2026-09-11 11:22:04 +02:00
Paul NothaftandPaul Nothaft	5f03d5aea6	fix(backend): validate event id before using it in the logo storage filename (stable) (#1397) ... * 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 <[email protected]>	2026-09-11 11:21:59 +02:00
Paul NothaftandPaul Nothaft	00a5c3a075	fix(backend): validate business-profile logo uploads by content, not filename (stable) (#1395) ... * 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 <[email protected]>	2026-09-11 11:21:54 +02:00
Paul NothaftandPaul Nothaft	33d6904e66	fix(backend): reject a replayed TOTP code within its validity window (stable) (#1398) ... * 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 <[email protected]>	2026-09-11 11:21:46 +02:00
Paul NothaftandPaul Nothaft	b2ae7f1c50	fix(backend): shorten payment-check token TTL and notify admin on use (#1392) ... 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 <[email protected]>	2026-09-11 11:21:42 +02:00
Paul NothaftandPaul Nothaft	1abd42e683	fix(backend): enforce event ownership on short URL deletion (#1394) ... 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 <[email protected]>	2026-09-11 11:21:38 +02:00
Paul NothaftandPaul Nothaft	024ffed1ca	fix(backend): bump sharp, nodemailer, multer, js-yaml, joi for security fixes (stable) (#1375) ... * 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 <[email protected]>	2026-09-11 11:21:34 +02:00
Paul Nothaft	e9b84b7a1b	chore(stable): release 3.46.12 (#1369) v3.46.12	2026-09-10 22:22:57 +02:00
Paul NothaftandPaul Nothaft	1316ed05b3	fix(video): try metadata extraction and thumbnail generation independently (#1372) ... * 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 <[email protected]>	2026-09-10 13:48:56 +02:00
Paul NothaftandPaul Nothaft	34207456e6	fix(backup): honor the configured database-backup destination path (#1367) ... * 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 <[email protected]>	2026-09-09 22:27:26 +02:00
Paul Nothaft	6d906349bf	chore(stable): release 3.46.11 (#1356) v3.46.11	2026-09-08 22:39:40 +02:00
Paul NothaftandPaul Nothaft	143c4035ec	docs: align stable security and backport policy (#1352) ... Co-authored-by: Paul Nothaft <[email protected]>	2026-09-08 08:33:00 +02:00
Paul Nothaft	99df3e204f	chore(stable): release 3.46.10 (#1332) v3.46.10	2026-09-07 23:03:37 +02:00
Paul Nothaft	95e3af0800	Merge pull request #1327 from PicPeak/fix/sanitize-html-2.17.7-stable ... fix(security): bump sanitize-html to 2.17.7 (stable)	2026-09-07 09:18:32 +02:00
Paul Nothaft	8421b7b668	fix(setup): require Node 22.12 for sanitize-html	2026-09-07 09:00:02 +02:00
Paul Nothaft	0f426ef699	fix(security): bump sanitize-html to 2.17.7 ... 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".	2026-09-06 23:06:45 +02:00
Paul Nothaft	be243aafe8	chore(stable): release 3.46.9 (#1283) v3.46.9	2026-09-03 22:26:45 +02:00
Paul Nothaft	3f90221f40	Merge pull request #1281 from PicPeak/fix/security-scan-batch-1-stable ... fix(security): batch 1 (stable) — zxcvbn DoS, revocation forgery, unlink traversals, stored Content-Type, edge middleware	2026-09-03 12:36:42 +02:00
Paul Nothaft	ed0a8e7656	chore(deps): apply non-breaking npm audit fixes ... 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.	2026-09-03 12:15:53 +02:00
Paul Nothaft	c89ce8e172	docs: say the upload allow-list covers every path, video extensions must be added ... 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)	2026-09-03 12:15:39 +02:00
Paul Nothaft	d81cade7cc	fix(security): harden four smaller gallery and contract paths, drop the unmounted photo auth middleware ... - 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)	2026-09-03 12:15:21 +02:00
Paul Nothaft	406c638451	fix(security): stop reflecting submitted values in validation errors everywhere, cap credential lengths, close the login timing oracle ... 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)	2026-09-03 12:14:42 +02:00
Paul Nothaft	b1369068ae	fix(security): close three middleware gaps around the API edge ... 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)	2026-09-03 12:14:02 +02:00
Paul Nothaft	a8d57f0d69	fix(security): never serve a photo under its stored MIME, and stop trusting the chunked-upload type ... 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)	2026-09-03 12:12:56 +02:00
Paul Nothaft	882101b586	fix(security): contain logo, favicon and PDF-logo unlinks to their upload directories ... 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)	2026-09-03 12:12:12 +02:00
Paul Nothaft	c6d401685f	fix(security): verify the signature before writing a token to the revocation list ... 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)	2026-09-03 12:12:12 +02:00
Paul Nothaft	6481708def	fix(security): stop reflecting submitted passwords in validation errors ... Codex review round 2. The 400 I added in the previous commit returned errors.array() verbatim, and express-validator puts the submitted `value` in each error -- so rejecting an oversized password echoed that password back, and re-allocated up to the 50mb body limit on an unauthenticated endpoint, partly undoing the denial-of-service fix this branch exists for. The same call appeared at seven sites in this file, five of which validate a password field: /admin/login, /gallery/verify, /gallery/:slug/client-login, /admin/change-password and /password-strength. Every failed login was returning the attempted password in its response body, where it reaches proxy logs, error monitoring and browser tooling. Fixed at all seven rather than only the one the review pointed at. Only `value` is dropped. `msg`, `path` and the rest are kept, because the two shapes express-validator produces are both consumed in the frontend -- AcceptInvite reads {field, message} from routeHelpers.validateRequest, EventDetails reads {msg, path} from raw errors.array() -- and switching auth.js to the helper's shape would have broken the latter for a reason unrelated to security. 1 more test. Backend suite: 2744 passed. (cherry picked from commit 903e471753)	2026-09-03 12:12:12 +02:00
Paul Nothaft	706d402c1e	fix(security): enforce the strength-endpoint validators, and stop the generator spinning ... 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)	2026-09-03 12:12:12 +02:00
Paul Nothaft	ed08ff84ff	fix(security): bound password input before zxcvbn, and drop the legacy media mounts ... 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)	2026-09-03 12:12:06 +02:00
Paul Nothaft	7fe80220f1	chore(stable): release 3.46.8 (#1250) v3.46.8	2026-09-01 22:29:02 +02:00
Paul NothaftandPaul Nothaft	ccdcdd6116	chore: keep issue screenshots out of the source tree (#1260) ... 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 <[email protected]>	2026-09-01 09:34:55 +02:00
3 people	ccc725f36e	fix(upload): let Android guests reach the camera without breaking video (#1248) ... * 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 <[email protected]> * 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 <[email protected]> Co-authored-by: Zszywany <[email protected]>	2026-09-01 09:24:38 +02:00
Paul NothaftandPaul Nothaft	fed99ac03d	fix(archives): write a real timestamp on restored photos (#1257) ... 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 <[email protected]>	2026-09-01 08:52:35 +02:00
Paul NothaftandPaul Nothaft	a01731d986	chore: remove .pyc files committed to stable by mistake (#1256) ... 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 <[email protected]>	2026-09-01 08:41:37 +02:00
Paul NothaftandPaul Nothaft	1d9f0b6c64	fix(events): apply the gallery password policy to publish (#1255) ... 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 <[email protected]>	2026-09-01 08:36:41 +02:00
3 people	261e243070	fix(archives): take the restored category from the manifest (#1240) (stable) (#1243) ... * 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 <[email protected]> * 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 <[email protected]> Co-authored-by: Marian <[email protected]>	2026-09-01 08:17:48 +02:00
3 people	5470fbe406	fix(gallery): route single-photo downloads through the storage backend (#1246) ... * 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 <[email protected]> * 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 <[email protected]> Co-authored-by: Peifu Mo <[email protected]>	2026-09-01 08:17:19 +02:00
3 people	7102687ee8	fix(events): delete stored objects when cascading an event delete (#1245) ... * 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 <[email protected]> * 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 <[email protected]> Co-authored-by: Peifu Mo <[email protected]>	2026-09-01 08:16:51 +02:00
3 people	5b69e3ec4c	fix(auth): treat zxcvbn suggestions as advice, not blocking errors (#1247) ... 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 <[email protected]> Co-authored-by: Peifu Mo <[email protected]>	2026-09-01 08:16:33 +02:00
3 people	eebca9900b	fix(auth): treat zxcvbn suggestions as advice, not blocking errors (#1247) ... 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 <[email protected]> Co-authored-by: Peifu Mo <[email protected]>	2026-09-01 08:05:57 +02:00
Paul Nothaft	c05faa50d9	chore(stable): release 3.46.7 (#1221) v3.46.7	2026-08-29 03:25:38 +02:00