100 Commits

This Branch

This Branch

All Branches

Author	SHA1	Message	Date
Paul Nothaft	54b68fe6e8	chore(main): release 3.107.3-beta.0 (#1092)	2026-08-20 05:23:01 +00:00
Paul NothaftandPaul Nothaft	576924fa57	fix(faces): scan external/reference photos instead of skipping them (#1090) (#1091) ... * fix(faces): scan external/reference photos instead of skipping them (#1090) faceProcessor short-circuited every photo with source_origin 'external' or 'reference' straight to 'skipped', before the sidecar was ever contacted. On an external-media install that is the entire library — the reporter's gallery sat at 0/3230 with every row skipped and no error, and a rescan changed nothing. The guard was correct when written: resolvePhotoStorageKey returns null for anything outside managed storage, so ensurePreviewImage could not build a preview and there was nothing to send. #1078 removed that limitation one release earlier — ensurePreviewImage now reads externals straight off the mount via resolvePhotoFilePath and writes the preview into managed storage, so the key faceProcessor already fetches through getStorage() is readable like any other. The guard outlived its reason. Photos whose source is genuinely gone still return a null preview key and land in the existing 'failed' branch, which is the honest outcome: that is a broken photo, not an unsupported one. The blanket skip was absorbing those too. No migration or manual reset needed — enqueueEvent already re-queues rows with face_status in (NULL, 'failed', 'skipped'), so previously skipped photos get picked up on the next scan. * fix(faces): queue external imports for scanning (#1090) The other half of the same bug, found by external review — and my first counter-argument against it was wrong. Managed uploads are enqueued by photoProcessor, which writes face_status 'pending' once a photo is processed (photoProcessor.js:573, commented as "the only correct place to enqueue"). External media never goes through photoProcessor at all: adminExternalMedia inserts rows directly, leaving face_status NULL. faceQueue.claimNextPhoto only claims 'pending' (faceQueue.js:64), so an import into an already-enabled event produced nothing until someone pressed Re-scan. Lifting the skip guard alone made external photos scannable but still not scanned — which looks like a complete fix right up until you import a photo. Resolved once per import rather than per file, since it is a per-event setting and the loop can run to a thousand files, and guarded on both the global flag and the per-event toggle exactly as photoProcessor guards it, so installs without the feature still never write a face_status. A failure to read the setting logs and imports anyway — the photos are the point. No video guard: walkDir only collects jpg/jpeg/png/webp, so nothing faceProcessor would skip as video can arrive through this route. * fix(faces): enqueue imports only after the event path is written External review caught a race I introduced in the previous commit. Marking rows 'pending' as they were inserted published claimable work while events.external_path still held the old value — or none at all, on a first import, since the route only writes it after the entire thumbnail loop. The face worker polls continuously, so on any import long enough to matter (the loop is ~100-300ms per photo, and the reporter's library is 6500+) it would claim those rows, resolve them against the wrong directory and mark them permanently 'failed' — a state only an explicit Re-scan clears. That is strictly worse than the unscanned photos this set out to fix. Ids are now collected during the loop and marked pending in one pass after the event path is written, chunked at 500 because SQLite caps a statement at 999 bound parameters. The test now drives the real route instead of re-implementing its logic, and observes the mid-loop state from inside the per-photo thumbnail call — the only hook that can see the window the race lived in. Verified it discriminates: deleting the enqueue fails two tests, and moving it back onto the insert fails the ordering test specifically. * fix(faces): read the face setting after the import, not before Third external-review round. The setting was captured before a loop that runs for many minutes on a large library, so an admin who enabled detection during an import left every photo imported after that moment at NULL forever — the toggle endpoint only queues rows that already existed when it fired. Ids are now collected unconditionally and the setting is evaluated immediately before the queue update, off a freshly read event row. The guard is unchanged in substance: both the global flag and the per-event toggle, so installs without the feature still never write a face_status. Test flips the toggle from inside the mocked per-photo thumbnail call, which is the same mid-loop hook the ordering test uses. --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-20 07:18:45 +02:00
Paul NothaftandPaul Nothaft	198df02d2a	test(ml): add an embedding-space fingerprint tool (#1084) (#1089) ... * test(ml): add an embedding-space fingerprint tool (#1084) requirements.txt is pinned exactly so rebuilds produce byte-identical embeddings, but nothing verified that. The API tests stub FacePipeline, so a decode/resize/kernel change could move every stored cluster without failing anything. This prints a hash per stage — decode, resize, cvtColor, YuNet detect, FaceNet forward pass — so the old and new image can be diffed on the same host before any base-image or dependency bump lands. Used it to answer the open question in #1084: Debian/Python 3.12 and Wolfi/Python 3.14 produce identical hashes at every stage, so a base swap would not invalidate stored clusters. The input is generated rather than a fixture, and the hashes are deliberately not compared across architectures — OpenCV and onnxruntime dispatch different SIMD kernels on x86 and aarch64, so this answers "did this change move the numbers", not "is every platform identical". * test(ml): fingerprint the production path, not a parallel one External review found the first cut was largely theatre. The documented command could not run: tools/ is in .dockerignore and the Dockerfile copies only app/, so the script is never inside the image. It has to be mounted — which is what I actually did when producing the numbers, while documenting something else. Three stages were fingerprinting the wrong thing: - The detector recorded "none" plus a return status, because synthetic input has no face to find. It would have stayed green through any change to YuNet or its kernels. Now the ONNX graph is driven directly, so all twelve output heads always produce numbers, and the reported thresholds are the service's (0.6/0.3) rather than FaceDetectorYN's 0.9 default. - The embedding used a hand-rolled tensor, bypassing everything that actually places a face in the embedding space: umeyama + warpAffine, BGR->RGB, per-image standardization, layout, and the L2 normalization the backend's cosine similarity depends on. It now calls _align and _embed directly. Private, deliberately — reimplementing the maths here would drift from pipeline.py and fingerprint a path nothing runs. - Decode exercised PNG, but the worker only ever receives the preview rendition, which imageProcessor.js writes as JPEG. Now a fixed JPEG, embedded as bytes so the input cannot depend on the encoder version being held still. Verified SOI/EOI-clean; the first attempt at this produced "Corrupt JPEG data: 22 extraneous bytes". Sensitivity checked rather than assumed: a one-pixel landmark nudge moves align_warp and embed and leaves decode and the detector heads alone, which is exactly the dependency structure expected. Debian/Python 3.12 vs Wolfi/Python 3.14 remain identical across all sixteen stages, so the #1084 parity conclusion still holds under the stronger check. * test(ml): measure the image's own pipeline, and the detector OpenCV runs Two more from external review, both of which let matching hashes mean less than they claimed. The documented bind mount put the checkout's app/ ahead of the image's /app/app, so comparing two images built from different revisions would have executed the same pipeline source twice and reported a match no matter how the images differed. /app now wins whenever it exists, so the tool measures the image under test however it is invoked, and the loaded path is printed as _app_source so that is auditable rather than assumed. The detector was fingerprinted through onnxruntime, but production runs cv2.FaceDetectorYN — OpenCV's own preprocessing, DNN engine and NMS/landmark decode, none of which ORT touches. An OpenCV upgrade could therefore move real landmarks, and with them alignment and embeddings, while every detector hash held still. It now runs the OpenCV path too, with the score threshold at the floor so synthetic input still yields candidates (594 here) instead of the empty result the production 0.6 gives on an image with no face. The ORT pass is kept alongside it to separate a model change from an OpenCV change. Parity across debian/3.12 and wolfi/3.14 still holds across all 19 stages, and a one-pixel landmark nudge still moves align_warp and embed and nothing else. * test(ml): cover the orchestration and progressive decode too Round three of external review found two more ways the hashes could match while production moved. The isolated stages never fed the detector's output into alignment — _align got fixed landmarks — so INPUT_LONG_EDGE resizing and the row -> landmark scaling in _one_face were invisible. process() now runs end to end on the fixture, with the pipeline's own detector threshold dropped so a faceless frame still yields rows to carry through (26 faces here). A first attempt at that still missed the resize: the embedded fixture is 48px, so `long_edge > INPUT_LONG_EDGE` never fired and changing 1920 to 960 moved nothing. It now runs a second pass with the threshold lowered under the fixture, which executes the same downscale and inverse landmark scaling without carrying a 1920px image in the source. Verified sensitive: moving that bound 32 -> 24 changes both the face count and the embedding. The fixture was also a baseline JPEG, while generatePreview writes progressive (imageProcessor.js:236/480/617) — a different path through libjpeg. Swapped for a progressive fixture, SOF2 confirmed present and SOF0 absent. 24 stages now. Debian/3.12 and Wolfi/3.14 remain identical across all of them. * test(ml): close three more false-negative paths in the fingerprint Round four of external review. All three let hashes match while production moved. INPUT_LONG_EDGE was used but never printed. The fixture is too small to trip the resize in either image, and the forced pass overrides the value in both, so a production change from 1920 to 960 moved no hash at all. It is now emitted alongside the other thresholds, where a reviewer sees it in the diff. The fixture was square, so a width/height swap in setInputSize or the resize produced identical dimensions and identical hashes. It is now 64x48. The forced-downscale pass hashed only an embedding, which is derived from separately scaled landmarks — a regression in the inverse scaling of row[0:4] would have shown up nowhere, because the normal pass runs at scale 1. That bbox is now hashed too; a wrong one is what breaks avatar crops and area calculations. Changing the fixture to 64x48 also broke the forced pass: at the old bound of 32 the downscaled frame is 32x24 and YuNet returns nothing, so the stage pinned nothing. The NO-DETECTIONS-STAGE-VACUOUS marker added last round caught it immediately rather than printing a reassuring hash of an empty result. Bound moved to 48, which still triggers the resize and still yields rows. 25 stages, no vacuous markers. Debian/3.12 and Wolfi/3.14 identical across all of them. * test(ml): hash every detection, not just the first Round five of external review. Both end-to-end passes hashed only candidate 0, so a change that moved candidates 1..n — or merely reordered them — matched as long as the count and the first candidate held. With the threshold at the floor those passes return 24 and 27 candidates, so that was most of the evidence being thrown away. Both now stack every returned face, in order, via a shared _hash_all. Stacking preserves order, so a reshuffle is caught too. Verified against the exact case: reversing candidates 1..n while leaving the count and candidate 0 untouched now moves process_embedding and process_bbox. Before this it moved nothing. * test(ml): hash every persisted field, and emit the model version Round six of external review, plus the adjacent gaps it implied. Two findings: MODEL_VERSION was never emitted, and _hash_all discarded score. Both matter to the backend rather than to the numbers — a model_version change makes faceClustering.js:190 refuse to compare new faces against existing people, forcing a rescan, and det_score decides via meetsQualityFloor (faceClustering.js:96-100) whether a face joins clustering at all. Either could change while every hash held still. Rather than fix only the two named, I checked what faceProcessor.js actually stores per face (:157-167) and covered all of it: bbox, score, yaw, pitch, blur, embedding. yaw/pitch/blur were heading for the same finding next round. One hash per field, so a diff says which thing moved rather than only that something did. model_version is emitted as a compatibility key alongside the thresholds, not hashed — it is a string, and its job is to be read. Verified: scaling score alone by 0.999 now moves process_score and nothing else. 33 stages, no vacuous markers, debian/3.12 and wolfi/3.14 still identical. * test(ml): split verdict from diagnostic, and stop masking the threshold Round seven of external review. The ORT detector hashes were being read as part of the compatibility verdict, but production never runs YuNet through onnxruntime. An ORT change touching a YuNet operator would have moved them while real behaviour was untouched, and the docstring said any difference means re-scan — so the tool could have ordered a full-gallery rescan for nothing. They are now diag_-prefixed, and the docstring states which keys carry a verdict, which are diagnostic, and which are metadata a reviewer has to read rather than diff. setScoreThreshold(1e-6) also overwrote the detector's real threshold before anything recorded it, and _thresholds.det_score only echoes config. If FacePipeline ever stopped applying DET_SCORE_THRESHOLD — falling back to OpenCV's 0.9 default — production would detect a different face set while every hash matched. The constructed value is now read first and emitted as _effective_det_score; simulating the regression makes it read 0.9 instead of 0.6. MAX_FACES is emitted for the same reason INPUT_LONG_EDGE is: the fixture never reaches the pipeline.py:138 slice, so 64 -> 128 would move no hash while real group photos persisted a different face set. 21 verdict keys, 12 diagnostic, no vacuous markers, debian/3.12 and wolfi/3.14 still identical across both sets. --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-19 22:43:58 +02:00
Paul Nothaft	3c5bedc2cf	chore(main): release 3.107.2-beta.0 (#1088)	2026-08-19 15:10:22 +00:00
Paul NothaftandPaul Nothaft	37a15e3d49	fix(faces): restore the :beta image tag and surface sidecar health (#1087) ... * fix(faces): restore the :beta image tag and surface sidecar health Both halves of what a user hit on discussions/1069: the People card sat at "Scanning… 0 of 227" for 30 minutes with no explanation, because the sidecar container could never have started. docker-build.yml — republish `:beta`. It used to come for free via `type=ref,event=branch` when the active development branch was literally named `beta`; the rename to `main` silently retired it. backend:beta has been frozen at 2026-06-29 (448da950) ever since while :main moved on, so PICPEAK_CHANNEL=beta has been serving a seven-week-old build across every image. The ml sidecar was added after the rename and so never had a `:beta` at all, which left docker-compose.production.yml:158 unable to resolve ghcr.io/picpeak/picpeak/ml for any documented channel — the image simply does not exist as :beta or :stable, only as :main and pinned versions. Tag added to all four merge jobs, gated on main. `:stable` stays absent for ml on purpose: it is gated on refs/heads/stable and the sidecar does not exist there. stable's docker-compose.production.yml carries no picpeak-ml service, so nothing can reference the missing tag. FaceRecognitionCard — show when the sidecar is unreachable. An unreachable sidecar is not an error by design: faceQueue.js:132-136 releases the photo back to `pending` and retries forever so a restart does not burn the queue. The cost was that a stopped container looked exactly like a slow scan, indefinitely, and the only signal was a backend log line rate-limited to once per five minutes. /admin/events/faces/health already existed and nothing in the frontend called it. It is now polled while a scan is in progress, and a failing check replaces the spinner with the sidecar URL, the underlying error (which distinguishes a stopped container from a token mismatch) and the command to start it. Health is only polled while a scan is running — an idle card has no reason to care whether the sidecar is up. * fix(faces): tell the three sidecar failure modes apart Follow-up to the health surface in this branch, from an external review pass. The original warning was right about "the sidecar is not working" and wrong about almost everything after that. faceClient.checkHealth now returns a `reason` rather than only a message, because the caller has to know whether photos survive: - 'unauthorized' (401) and 'rejected' (any other 4xx) both become SidecarRejectedError in classify(), which workerLoop does NOT retry — every claimed photo is marked 'failed'. Telling the admin the scan resumes on its own was simply untrue there; both now say to fix the cause and Re-scan. - 'unreachable' (refused/DNS/timeout/5xx) is the retryable one. The card also no longer cries wolf. /faces runs inference synchronously inside an `async def`, so one slow photo blocks the event loop and stalls /info past its 5s timeout — a healthy sidecar can fail a probe. Verified with an isolated uvicorn repro: a blocking call in an async handler stalled the sync /info endpoint to 5.01s. The warning now needs three consecutive failures AND no drop in `pending`. Three because a single /faces call may legitimately run to FACE_ML_TIMEOUT_MS (30s) and two probes 15s apart both fit inside that window; `pending` rather than `scanned` because scanned counts only 'done', so a run producing skipped/failed photos is progress that counter misses. A 4xx burns the queue with no backoff, so it can empty before anyone opens the card — in_progress goes false and only "227 failed" is left. The probe therefore also runs when a finished scan has failures, and the notice renders under the counts instead of replacing them. It is worded as present-tense service state, not as a claim about those specific failures: a live probe cannot know whether they came from this misconfiguration or from corrupt images earlier. Attributing them exactly needs stored face_error rows, which is a bigger change than this. Also adds the missing-token case to the unreachable text: FACE_ML_TOKEN has no default and the container refuses to start without it, so the most likely first run fails as a plain connection refusal that "just start it" does not fix. --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-19 16:49:47 +02:00
Paul NothaftandPaul Nothaft	74a8f9bf24	chore(security): shrink the ML image's CVE surface, override deepmerge-ts (#1083) ... Three Trivy cleanups off the code-scanning tab. ml/Dockerfile — install no runtime apt packages at all. Neither libgl1 nor libglib2.0-0 is needed: opencv-python-headless 4.14 bundles what it needs and `ldd .../cv2/cv2*.so` resolves fully on a bare slim base. The old comment claimed the headless wheel still links libGL, which was true of much older wheels. libgl1 was dragging in 36 transitive packages (mesa, LLVM, X11) for a service that never opens a display. Measured with `trivy image` on locally built variants: before: 165 findings — 88 low / 49 med / 19 high / 6 crit without libgl1: 133 findings — 58 low / 48 med / 19 high / 5 crit without either: 123 findings — 57 low / 46 med / 13 high / 4 crit 42 findings gone, image 1.05GB -> 774MB. Not one of the 165 had an upstream fix available, so not installing the packages is the only lever there is. docker-build.yml — set ignore-unfixed on all four Trivy steps. All 123 remaining ML findings are unfixed base-OS CVEs; Debian has them resolved in sid and pending backport to trixie, and apt-get upgrade -y behind CACHEBUST picks each one up automatically. Reporting them buries anything actionable, and suppressing them is the precondition for ever setting exit-code: 1. 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. Not reachable in our code: html-to-text only feeds deepmerge-ts its options object (html-to-text.mjs:1468, :1442), never parsed email content. npm audit goes 3 high -> 0. The lockfile also picks up the version field release-please had left at 3.103.1-beta.0, plus some "peer": true metadata npm 11.6 recomputes. Co-authored-by: Paul Nothaft <[email protected]>	2026-08-19 13:56:01 +02:00
Paul Nothaft	9f825be01e	chore(main): release 3.107.1-beta.0 (#1081)	2026-08-19 08:23:14 +00:00
Paul NothaftandPaul Nothaft	af7970b069	fix(preview): generate lightbox previews for external/reference photos (#1078) (#1079) ... * fix(preview): generate lightbox previews for external/reference photos (#1078) 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 ("LocalFsStorage: invalid relative path: null"), so the preview route fell back to redirecting at the full-size original. A gallery whose photos are all external got no benefit from the preview tier (#492) at all: guests paid 5-12 MB on every lightbox open, with nothing surfaced in the admin UI. Add the external branch ensureThumbnail() has had since #423: resolve via resolvePhotoFilePath() and feed the mount path to generatePreviewImage() directly, with an ext<id>_ output basename so two events referencing the same NAS filename can't clobber each other's preview. Also close the adjacent hole that made the failure a throw rather than the documented null: a row with no source_origin in a reference-mode event takes its mode from the event, so resolvePhotoStorageKey returns null for it too. Return null instead of handing that to withLocalCopy. 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 * style(test): single-quote the source-inspection needles Matches the repo eslint quotes rule (no avoidEscape) by dropping the nested quotes from the search strings rather than escaping them. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-19 10:17:44 +02:00
Paul Nothaft	98fe9c7699	chore(main): release 3.107.0-beta.0 (#1077)	2026-08-18 20:41:12 +00:00
Paul NothaftandPaul Nothaft	b69dd134d0	feat(faces): People in this gallery — face recognition via an optional ML sidecar (#1074) (#1075) ... * feat(ml): optional face-detection sidecar, opt-in and inert by default (#1074) First of four PRs for "People in this gallery". This one ships only the sidecar, its wiring and its CI — no schema, no backend code, no UI. Nothing in PicPeak calls it yet. picpeak-ml is a single FastAPI + onnxruntime container: three endpoints (/health, /info, /faces), no database, no volumes, no egress, no model download at runtime. Clustering, person identity and every privacy decision stay in the backend where the data already lives. Models are YuNet (detection) + FaceNet-512 (embedding), both MIT, both pinned by URL and SHA-256 and verified at build time. The licence analysis is in ml/LICENSES.md: the more accurate InsightFace weights are non-commercial-only and PicPeak's users are working photographers, so they are never baked into an image we publish. Two things worth review attention: - Alignment uses a least-squares similarity transform (Umeyama), NOT cv2.estimateAffinePartial2D. RANSAC and LMEDS exist to reject outliers among many correspondences; given five landmarks and no outliers they fit a three-point subset exactly and let the rest drift. Measured on a real off-frontal portrait: eyes and nose pinned to 0.11px, mouth corners 11.8px out on a 160px crop. Umeyama distributes it (max 6.5px, rms 5.1 vs 7.4). The failure mode is silent — a bad warp still yields 512 confident floats — so tests/test_pipeline.py pins it numerically. - FACENET_ONNX_URL has no default and the build fails loudly without it. deepface distributes FaceNet-512 as Keras .h5 only, so the ONNX is produced once by tools/convert_facenet.py and published as a release asset. Converting inside the build would drag TensorFlow through both architecture legs of every build to produce a byte-identical file. The CI jobs are gated on the FACENET_ONNX_URL repository variable and skip cleanly until it is set. Off by default, twice over: the sidecar is behind the `faces` compose profile, and the backend will gate on a `faces` feature flag that defaults to false. FACE_ML_URL defaults to http://picpeak-ml:8000 so the standard deployment needs no configuration — nothing dials that host while the flag is off, which is why a non-resolving default is harmless. Verified: 27 pytest tests green; YuNet loads and detects against a real portrait with its landmark order matching the alignment template index-for-index; both compose files validate and the faces profile is correctly excluded from a default `up`; workflow YAML parses and the job graph resolves. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(ml): pin the converter toolchain, verify parity, drop a false reproducibility claim (#1074) Ran the FaceNet-512 conversion for real and corrected what the previous commit assumed about it. The conversion works: 23,497,424 parameters, 89.6 MB ONNX, and the converted graph matches the Keras original to 2.086e-06 absolute / cosine 1.0000000000. That check is now part of the script rather than something I did once by hand — a subtly wrong graph still returns 512 plausible floats, so it refuses to leave the file on disk if parity fails. Also ran the full pipeline against both real models end to end. The embedding is L2-normalized to 1.000000, and the same face survives being re-rendered: half scale 0.973, double scale 0.984, JPEG q40 0.987, rotated 8 degrees 0.984, brightness +40 0.988. Scale invariance in particular is evidence the alignment warp is doing its job. Corrected claim: the conversion is NOT byte-reproducible. Two runs with the same pinned versions on the same machine gave different SHA-256s. The graphs are functionally identical — same 336 nodes, same 271 initializers, every weight matching to 0.000e+00 — but a few initializer names differ because tf2onnx's traced-op naming is not deterministic (Keras layer naming is deterministic; I checked). The previous commit message and README both claimed byte-identical output. They were wrong, and it matters: anyone re-running the conversion gets a different hash, and without this note that reads like tampering. The build-time SHA-256 pins one published artifact so its URL cannot start serving different bytes; validating a fresh conversion is the parity check's job. requirements-convert.txt now pins the exact set that produced the artifact, including transitive keras/protobuf/numpy, and documents that the converter needs Python 3.11 while the image runs 3.12. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * feat(faces): schema, queue, clustering and API for People in this gallery (#1074) Backend half of the feature. Migration 177, a face-detection queue, the clustering engine, the gallery and admin APIs, and the privacy wiring. No UI yet; nothing is reachable until the `faces` feature flag is on, which defaults to false. The flag is the gate, not FACE_ML_URL. That variable now has a working default (the compose service name), so its presence proves nothing about intent — if it were the gate, every install would poll a hostname that does not resolve. faceQueue re-checks the flag every tick, so turning it off stops the workers without a restart. Visibility scoping is the part worth reviewing closely. Face rows have no concept of photo visibility, but guests are restricted to photos.visibility='visible'. A raw count leaks how many hidden photos someone appears in, and an unscoped cover face renders a crop of a photo the guest may not open — with the best-scoring face being the likeliest pick, so it would happen often rather than rarely. facePeopleService recomputes both per request against the caller's own scope, and event_people.face_count_total is named to be conspicuous in a guest path. Six tests cover it, including the case where a person's photos are ALL hidden and they must vanish entirely. Face data is excluded from backups and .picpeak exports, per the decision in the thread: it is derived, so a restore re-scans rather than carrying biometrics between operators. Three separate mechanisms, because the engines cannot be filtered alike — EXCLUDED_TABLES for export, --exclude-table-data (not --exclude-table; the CREATE TABLE must survive or restore breaks on the first query) for Postgres, and DELETE + VACUUM on the temp copy for SQLite, which has no way to exclude a table from a whole-file .backup. The VACUUM is not cosmetic: without it the pages stay in the file and the claim is false on disk. Archiving now purges face data explicitly. photo_faces cascades off photos, but archive deletes neither the photo rows nor the event, so without this an archived gallery kept its biometrics indefinitely. Other decisions: clustering keeps names across a re-cluster by majority inheritance (without it, one button click silently discards every name the photographer typed); consolidation refuses to merge two people who were named differently; assignment never compares across model_version, since embeddings from two pipelines are not comparable; low-quality faces are stored but left unassigned so they show in "this photo contains" without spawning junk people. Migration is 177, not 174 — 174/175/176 landed on main while this branch was open. 29 tests green: 7 migration (idempotency, down(), cascade, and that installing it enqueues NOTHING), 11 clustering, 11 privacy/visibility. Lint clean; the pre-existing error counts in databaseBackup.js and server.js are unchanged. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * feat(faces): People strip, face filter and admin controls (#1074) Frontend half. Renders nothing anywhere unless the `faces` feature flag is on AND the photographer enabled detection for the gallery — the whole guest surface hangs off one boolean, event.people_enabled, which the server computes from the flag, the per-event toggle and the show-to-guests toggle together. Guest side: a People strip between the filter bar and the grid, circular crops from each person's cover face, an active-filter chip row, and a "Show all" bottom sheet. The face filter composes with category, search, media type and the liked/saved/rated filters in the same useMemo rather than replacing them, so "photos of Anna that I liked" works. Two people selected means AND by default — that is what picking a second face almost always asks for — with a toggle to OR that appears only once a second person is picked. Unnamed people show a photo count and never "Person 7". A number is honest about what the system knows; an invented name is not. There is a test asserting we don't do it. The strip renders nothing below two people, collapses to one line when dismissed (persisted per slug, so dismissing one gallery says nothing about the next), and appears mid-backfill with a progress line rather than blocking the gallery behind a spinner. Avatar crops are computed in ratios of the source dimensions so they survive whatever rendition the browser gets; without width/height they fall back to an uncropped thumbnail, since a wrongly-offset crop is worse than no crop. No new download endpoint: "download these N" rides the existing photoIds path, which already enforces access level and per-category permissions server-side. Adding a person_id selector would have been a second thing to authorize for no gain. Guest-facing copy never says "biometric" or "recognition" — those words describe our implementation, not the guest's experience. The sheet's footnote answers the first question every guest has (where does this go?) inline. The admin card, by contrast, is explicit: it states the controller obligation next to the toggle, and warns that scanning materializes the preview tier on galleries that never generated one, which is real CPU and disk an admin should know about before a 2,000-photo backfill. EN + DE translations. 140 frontend tests green (8 new), tsc and eslint clean. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(faces): measured match threshold, working build defaults, 89MB smaller image (#1074) Ran the Phase 0 spike that had been outstanding, published the model, and fixed what both turned up. THRESHOLD IS NOW MEASURED, NOT GUESSED. LFW's standard 1000-pair protocol run through this exact pipeline (YuNet -> Umeyama alignment -> FaceNet-512 ONNX), 100% detection on 2000 images: same person cosine 0.6958 +/- 0.1415 diff person cosine 0.0849 +/- 0.1674 separation 0.6109 peak accuracy 96.60% @ 0.405 So the pipeline separates people well — the thing I could not previously claim, since every earlier number was the same face re-rendered. Default moves 0.62 -> 0.50. The old value was a placeholder and a bad one: it gave 0% false merges but 22.4% false splits, i.e. roughly one in four same-person pairs failing to join, which fragments a gallery badly. 0.50 gives 1.0% false merge / 8.2% false split. Peak accuracy (0.405) is deliberately NOT chosen: for clustering the two errors do not cost the same. A false split is a duplicate row the photographer can merge away; a false merge puts a stranger into someone's "download my photos" — and until the Phase 2 merge/split UI ships, there is no way to undo one. So this sits on the conservative side of the optimum. The spike is committed as ml/tools/benchmark_threshold.py rather than thrown away, so "why 0.50?" has an answer in six months and a re-tune is one command. BUILD DEFAULTS. FACENET_ONNX_URL/_SHA256 now default to the published ml-models-v1 release asset, so `docker build ml/` and `docker compose --profile faces up` work with no arguments. Blanking either still fails loudly — a URL without a checksum is never acceptable, since the checksum is what makes the URL safe to trust. Found by running compose for real: it failed exactly as designed, which was correct behaviour and a bad out-of-box experience now that a canonical artifact exists. IMAGE SIZE. 389MB -> 300MB single-arch. `chown -R` after COPY rewrote every copied file into a fresh layer, duplicating the 90MB model for nothing; the user is now created before the copies and ownership set via COPY --chown. Also drops pip/setuptools from the runtime image. Measured RSS is 186MiB idle, and the container answers /faces end-to-end in well under the 80-150ms/photo the issue budgeted. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(faces): threshold 0.50 -> 0.60 from real clustering, theme-aware People strip (#1074) Both fixes come from running the feature on an actual gallery — 61 photos, 5 real identities — rather than reasoning about it. THRESHOLD. The LFW pairwise sweep in the previous commit said 0.50, and it was wrong. On a real gallery at 0.50, three of six visible clusters were contaminated: two different people merged into one strip entry, which is the exact failure that puts a stranger into someone's "download my photos". Pairwise error rates do not predict cluster purity. Greedy assignment compounds — one wrong face drags the centroid toward the midpoint between two identities, making the next wrong face likelier. A 1% pairwise false-merge rate is not a 1% chance of a clean gallery, and no amount of staring at an ROC curve would have shown that. Sweep against ground truth (5 identities): 0.50 -> 6 clusters, 3 contaminated 0.56 -> 6 clusters, 0 contaminated 0.60 -> 5 clusters, 0 contaminated <- exactly right 0.64 -> 5 clusters, 0 contaminated, fewer faces assigned 0.60 recovers the right number of people with no contamination; higher only loses coverage. Migration 177 carries the full reasoning so the next person to touch this knows why the obvious pairwise answer is the wrong one. THEME. The People strip hardcoded `text-neutral-800` for named people. On a dark gallery — which the screenshot immediately showed — that renders a named person's label almost invisibly, while UNNAMED people stayed legible. Exactly backwards. Labels, headings, the collapsed summary, the scan line and the filter chip row now read the gallery's own theme tokens (--color-text / --color-muted-text / --color-accent / --color-surface-border) like the rest of the gallery surface. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(faces): keep the mobile filter row inside the viewport (#1074) At 390px the photo count and Clear link were pushed against the right edge by ml-auto and clipped. Only apply it from the sm breakpoint up, where there is room; below that they flow after the chips. Found by screenshotting the real thing on an iPhone-sized viewport. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * feat(faces): complete Phase 1, add People management and auto-categories (#1074) Closes the two Phase 1 gaps, then builds Phase 2 and Phase 3. PHASE 1 GAPS. "Download these N" was specified, described as done in an earlier summary, and never actually built — I had verified the backend needed no new endpoint and let that stand as if the button existed. It now hands the filtered photo ids to the same path as a manual selection, so the server re-applies access level and per-category permissions on the way through. Photos in a downloads-disabled category are excluded client-side too, so the number on the button is the number the guest receives. Hidden entirely when downloads are off for the gallery. Lightbox person chips ("In this photo: Anna") are the second way into the face filter — a guest looking at a photo of themselves can act on it without scrolling back to the strip. Tapping one closes the lightbox and filters the grid behind it. PHASE 2. A People management modal over the endpoints that already existed and were already tested: rename inline, merge (multi-select, first pick is the target so the name a photographer typed survives), split via a face picker, hide, ignore. This matters more than it sounds — clustering deliberately errs toward splitting because a wrong merge puts a stranger into someone's download, and that trade only works if merging is easy. PHASE 3. Rule engine over face_count plus face-area ratio: 0 -> Details, 1 large -> Portraits, 2-5 -> Small groups, >5 -> Groups. The area ratio is what separates "a portrait of someone" from "someone is in this landscape". Three guarantees, all tested: it only ever fills an EMPTY category (enforced in the query AND re-checked in the UPDATE, so a photographer setting one mid-run still wins), everything it touches is marked auto_categorized so undo is exact, and it is a no-op unless separately enabled. Migration 178 adds the column — separate from 177, which has already run wherever this branch is deployed. Verified on the real gallery: 61 photos -> 48 portraits + 13 small groups, undo cleared exactly 61 and left the manual ones alone. Merge moved faces and removed the source. Both confirmed against the database, not just the UI. TWO BUGS THE BROWSER CAUGHT, both invisible to tsc: - The lightbox destructure never landed — my patch targeted a line that has a default value, matched nothing, and failed silently. `people` resolved to something else entirely and the chips would never have rendered. eslint's "outer scope value" warning is what surfaced it. - Admin face thumbnails 403'd because <AuthenticatedImage> attaches whatever gallery token is in session storage; an admin who has also opened one of their own galleries sends a type:"gallery" bearer to an admin route. Admin routes authenticate from the httpOnly cookie, which a plain same-origin <img> sends by itself. Worth noting AdminPhotoGrid has the same latent shape; not touched here. Also: the admin card now reports "N people (M shown to guests)" when those differ, so the settings page and the gallery stop disagreeing without explanation. 45 backend tests (8 new) and 140 frontend tests green; tsc and eslint clean. EN + DE for every new string. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * perf(faces): batch migration DDL and drop the face stack from server.js import (#1074) CI's backend job timed out at 10 minutes on the first run of this branch. Nothing failed — 132 of 182 suites passed and the wall clock ran out. Main does the same 182 in 124s, and where main has 12 suites slow enough for jest to print a duration, this branch had 77. Two changes, both worth making regardless of how much of the gap they close: - Migration 177 added its columns one ALTER TABLE at a time (four on photos, three on events, plus a separate index statement) and seeded settings with a SELECT and an INSERT per key. It now uses one alterTable per table and one SELECT plus one bulk INSERT. 178 folds its index into the same statement as its column. That chain replays in ~90 suites, so statement count there is multiplied by 90. - server.js required faceQueue at module scope, which pulls in axios and — through imageProcessor — sharp. Every supertest suite that imports server.js was paying for a module graph it never uses. Now required inside the startup block, next to the call that needs it. Honest about the evidence: locally the migration delta measures at zero (1.15s vs 1.13s for the same suite, three runs each), so batching alone does not explain an eight-minute regression. A fast local disk and many cores mask per-statement and per-import costs that a two-core runner with a shared disk does not. These are the two real costs this branch added to a path that runs in almost every suite; whether they are sufficient is a question for CI, not for another round of local speculation. 37 face tests still green after the change. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * i18n(faces): complete EN and DE coverage for the face feature (#1074) The admin card and the Features toggle were rendering entirely from inline English `defaultValue` fallbacks — 22 keys existed in no locale file at all, so a German admin saw an English consent notice, English toggles and English buttons. The gallery side was already translated; the admin side was not, and nothing in the toolchain flags this because a `defaultValue` always renders something. Adds the missing `admin.faces.*` (19), `settings.features.faces.*` (2) and shared `common.clear/saved/saveFailed` in both languages. Existing keys are left alone (setdefault, not overwrite), so the shared `common` strings other features rely on are untouched. Committed the audit as frontend/scripts/i18n-faces-audit.py rather than throwing it away: it extracts every t() key the face components actually use and diffs it against each locale, and it also reports German values that are byte-identical to English, which is the usual shape of an untranslated copy-paste. Currently: 69 keys in use, EN complete, DE complete, no identical pairs. Verified in the browser, not just in the JSON — the German card reads "61 / 61 Fotos durchsucht · 16 Personen (5 für Gäste sichtbar)" end to end. Also checked the components for hardcoded user-facing text (JSX nodes, title/aria-label/placeholder attributes) outside t(); there is none. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(faces): 13 defects from external review — coordinates, counts, erasure, races (#1074) Codex reviewed the branch against main. Thirteen findings, nine P1. I checked every one against the code and could not dismiss a single one as a false positive, so all thirteen are fixed here. THE WORST ONE: bounding boxes were stored in the wrong coordinate system. The sidecar reports coordinates in the space of the image it was HANDED — which is the ≤1920px preview, not the original — while every consumer compares them against photos.width/height, the original dimensions. A 6000px photo therefore produced boxes ~3x too small and areas ~9x too small: avatar crops landed in the wrong place and the Portraits rule could never fire. It is invisible on any photo already under 1920px, which is exactly why the demo gallery and every screenshot looked correct. Now scaled once in faceProcessor so everything downstream can assume original-image coordinates. ERASURE. The FK cascade on photo_faces is decorative on SQLite: PicPeak never enables `PRAGMA foreign_keys`, so deleting a photo left its embeddings behind. I first enabled the pragma globally and reverted it — six unrelated suites immediately failed on pre-existing dangling references, and switching it on would start rejecting inserts on every existing install. That is a real change worth making, but it is its own PR, not a rider on this one. Instead deletion purges explicitly: purgePhotoFaces in the photo paths (single, bulk, service) and photo_faces/event_people in deleteEventCascade. Tests assert this with the pragma explicitly OFF, so they can only pass if the code does the work. COUNTS. A re-scan deleted the old face rows without undoing their contribution to event_people, so counts inflated on every re-scan and ghost people survived. Now the affected people are recomputed before the replacements are assigned. My own "must not double its faces" test only checked photo_faces rows, which is why it passed throughout. RACES. A worker that finished after an admin purged the event committed its rows anyway — erasure reported success and the data reappeared. The commit is now conditional on the row still being 'processing'. And assignFaces is read-modify-write over an event's people, so two workers lost each other's updates; it is now serialised per event with an in-process mutex plus a Postgres advisory lock for the multi-pod case the queue advertises. METADATA LOSS. Merging discarded the source's name and suppression flags, so a merge could erase a typed name or un-hide someone. Reclustering remembered only people with a label, so an unnamed-but-hidden bystander came back guest-visible after one "Re-group people" — and suppression now propagates to every descendant cluster, not just the majority one. Also: export reset face_status so a restored gallery re-scans instead of claiming to be scanned forever; manual category edits clear auto_categorized so "undo automatic" cannot delete a photographer's own choice; external photos are skipped rather than failed (resolvePhotoStorageKey returns null for them by design); the gallery refetches photo memberships as a scan progresses so filtering is not stale; a failed VACUUM now fails the backup rather than publishing one that may retain biometric pages; and the ML Dockerfile's `|| true` is scoped to the uninstall — as written it was `(install && uninstall) || true`, so a failed dependency install produced a green layer and an image with no onnxruntime. Four new regression tests. Full backend suite failure set verified identical to origin/main; frontend 140 green; tsc and eslint clean. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(faces): 12 more defects from review round 2 — cross-event purge, leaks, lifecycle (#1074) Second Codex round on the same diff, now including round 1's fixes. Twelve findings, seven P1. Again none were false positives. SECURITY, AND MINE FROM ROUND 1: the bulk-delete face purge iterated the raw `photoIds` from the request instead of the event-scoped `photos` rows the handler had already validated. purgePhotoFaces has no event scope of its own, so an editor could pass another gallery's photo id and delete its face data — even though the photo deletion right below it was correctly scoped. Fixing one thing and introducing another is exactly why the second round was worth running. ANOTHER VISIBILITY LEAK, same class as the one round 1 fixed: /people returns scan progress, and getScanStatus counted every photo with a face_status — including hidden ones. Guests could read the hidden-photo count off the progress bar while the people list and covers beside it were properly scoped. Now scoped by the same predicate, with the caller passing its audience. RECLUSTER, ROUND 1'S FIX WAS INCOMPLETE. I made suppression follow every descendant but still copied the flags from the majority ANCESTOR. When reclustering merges a visible named person with a hidden one, the majority ancestor is often the visible one — republishing the hidden person's photos. Suppression is now OR-ed across every ancestor contributing faces. The name also now goes to the genuine largest descendant; the previous code took whichever cluster came first in map order, which the comment already claimed it did not. LIFECYCLE. Face data is excluded from backups and exports, but photos. face_status came across intact, so a restored install claimed every photo was scanned while holding no faces — and the worker only claims 'pending', so it stayed that way forever. Now: the SQLite backup requeues in the dump, restore requeues after the pool reinit (the Postgres path cannot rewrite rows inside pg_dump), the portable importer purges LOCAL face tables (they were excluded from the replace list, so another instance's embeddings survived an import with FK checks suspended) and requeues, and archiving disables detection so a restored archive is honestly off rather than enabled-and-empty. WRITE PATHS. Only processPhoto enqueued. The synchronous upload path (chunked-upload completion, watch-folder) left photos unscanned, and replacePhoto kept the OLD image's faces on a row now pointing at a different picture — stale identities shown on the new photo. FRONTEND. PeopleSheet and the admin manager rendered centred thumbnails and ignored the bbox, so on group photos the avatar showed whoever stood in the middle and two people from one photo were indistinguishable — in the manager whose entire job is telling faces apart. The crop maths is now one shared helper (faceCrop.ts) so the three surfaces cannot drift again. Full-page layouts (gallery-premium, gallery-story) render their own lightbox and never received the people props. Backend failure set verified identical to origin/main; frontend 140 green; tsc and eslint clean. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(faces): round 3 — five of round 2's fixes were wrong or no-ops (#1074) Third and final Codex round. Eight findings, four P1 — and the important part is that FIVE of them are defects in round 2's fixes, not in the original code. - The sync-upload enqueue I added was a silent no-op. It queried through `trx` after the transaction had already been committed, which throws "Transaction query already complete" straight into the catch I had wrapped it in. Chunked uploads and watch-folder imports were still never scanned, and the code read as though they were. Uses `db` now. - The post-restore requeue ran BEFORE the files were restored, in both the portable importer and the native restore. The face worker is live during a restore, so it could claim those rows and scan the previous instance's files, or fail them for originals not yet on disk — with nothing to requeue them afterwards. Both now run after file restoration; the native one is extracted into requeueFaceScans() and called from the full and database-only paths. - The admin face crop mixed coordinate spaces: an original-pixel bbox scaled against the THUMBNAIL's natural size. The API now returns the source dimensions alongside the box, so there is one space to reason about. - Forwarding people props through layoutProps did not make them work — the full-page layouts never destructured them. GalleryStoryLayout now threads them to its own lightbox. Genuinely new findings, all in the same class as ones already fixed: - releaseToPending updated unconditionally, so a photo purged while its sidecar request was in flight came back as 'pending' and was rescanned — biometric rows reappearing after the purge reported success. Round 2 fixed exactly this on the COMMIT path and I did not carry it to the retry path. Now guarded on 'processing'. - purgePhotoFaces left face_status alone, so a worker mid-scan still satisfied its commit guard and could write fresh faces into a photo being deleted — orphans, since the FK cascade is inert on SQLite. It now clears the claim as part of the purge. - Phase 3 was unreachable: the migration seeds face_auto_categorize_enabled false and nothing could ever write it, so the rule engine and its undo endpoint returned "disabled" in every real flow. Added GET/PUT and a toggle on the admin card, EN + DE. NOT fixed, deliberately: GalleryPremiumLayout uses yet-another-react-lightbox rather than the shared PhotoLightbox, so person chips there are a real port rather than a prop forward. Recorded as open rather than bodged. Backend failure set identical to origin/main; 41 face tests and 140 frontend tests green; i18n audit reports EN and DE complete at 71 keys. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * feat(faces): block face recognition on the all-in-one image (#1074, #1042) The single-container image cannot run this feature, so it is refused there rather than left to degrade. WHY, since the reason is not obvious from the code: the AIO image runs the backend, the frontend, SQLite and every background worker inside one container aimed at "one photographer plus guests browsing". It has no Redis, SQLite gives it a single writer, and it contains no ML sidecar to talk to. Face detection would add a second image-processing pipeline competing with Sharp for the same CPU and memory. That failure is not loud — the install just becomes slow and looks broken, which is the worst possible shape for a deployment whose whole promise is one container and no decisions. Gated on an explicit PICPEAK_SINGLE_CONTAINER marker, NOT inferred from SERVE_FRONTEND or a SQLite path: plenty of legitimate multi-container setups serve the frontend from the backend or run SQLite, and none of them should lose the feature by accident. Three layers, because the first is the only one that enforces: - faceSettings.isFeatureEnabled() returns false before consulting the flag, so a database restored from a full deployment with `faces` enabled still cannot switch it on here. - The feature-flag API forces `faces: false` in both directions, so the admin UI reflects reality instead of offering a switch that refuses to stay on. - The Features tab renders the card disabled with a plain-language reason, read from a new `single_container` field on /admin/system/version (an endpoint the admin UI already calls). Documented in ml/README.md and .env.example. Three tests pin the behaviour, including that the marker only accepts explicit truthy values. NOTE FOR PR #1068: this expects `Dockerfile.aio` to set `ENV PICPEAK_SINGLE_CONTAINER=true`. That one line lives on that branch and is not in this commit — until it lands, an AIO build would still offer the feature. Worth adding alongside the `Limits` section of docs/single-container.md. 44 face tests green; EN + DE complete at 72 keys. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * test(faces): pin the bbox coordinate space with a real scale factor (#1074) The coordinate-space bug — boxes stored in preview space while every consumer reads them as original-image pixels — had no test, and could not have been caught by the ones that existed: every photo in the demo gallery is 750px, so the scale factor was always exactly 1.0 and the correction never executed. Verified by hand first, on a real 4000x3000 upload with the face placed off-centre so a wrong crop would be unmistakable. Before the fix the stored box was 1493,204 (preview space, face actually at x≈2850-3618); after, 3110,426 — a factor of 2.083, exactly 4000/1920, landing inside the face. The admin crop then resolved to left=-395px/top=-46px on a 64px window, which is the face centred. That verification is now a test rather than a memory. Three cases: a 4000px photo must scale by 4000/1920, a 1920px photo must NOT change (the case that hid the bug), and a row with no width must fall back to unscaled rather than storing NaN. Note for anyone extending these: jest hoists mock factories above the file, so anything they close over has to be `mock`-prefixed. Getting that wrong fails at transform time with a message that does not name the variable. 47 face tests green. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-18 22:37:28 +02:00
Paul Nothaft	6ebdd13dde	chore(main): release 3.106.0-beta.0 (#1076)	2026-08-18 20:21:44 +00:00
Paul NothaftandPaul Nothaft	f22999aba6	fix(storage): write business documents under STORAGE_PATH, not the cwd (#1070) ... * fix(storage): write business documents under STORAGE_PATH, not the cwd persistDocPdf, the invoice sending and reminder writers and both contract signature writers built their target from `path.join(process.cwd(), 'storage', 'business-docs', ...)` and never consulted STORAGE_PATH. docker-compose.yml and docker-compose.production.yml both pin STORAGE_PATH=/app/storage and the image's WORKDIR is /app, so on a stock deployment the two expressions name the same directory and nothing looked wrong. Point STORAGE_PATH anywhere else and quotes, invoices, Mahnungen and contract PDFs land outside the configured storage root: missed by the backup walker, invisible to the storage accounting, and gone when the container is replaced. It also fails outright where the working directory is not writable by the runtime user. Routed all six writers through getStoragePath(), the resolver the rest of the app already uses. Two read-side sites of the same class came along: the custom PDF font lookup now checks the storage root before the legacy cwd path (a font under STORAGE_PATH/fonts was simply never found, and the document silently fell back to the built-in face), and the dev-test scratch directory follows the same root. Left alone deliberately: resolveLogoFile and adminBusinessProfile already try both roots, so their cwd reference is a legacy fallback rather than a miss. No migration needed — the persisted path is stored absolute, so rows written before this keep resolving to where those files actually are. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(storage): allow the configured contract root, and move signature images too Two holes in the previous commit, both found by review. Contract downloads would have broken. assertContractPdfPath() guards the admin unsigned/signed PDF routes and GET /api/public/contracts/:token/pdf, and it listed only <cwd>/storage/business-docs/contract. Moving the writers to STORAGE_PATH without moving that root meant every newly generated contract was refused with PATH_OUTSIDE_STORAGE — a worse failure than the bug being fixed, and only on the installs the fix was for. The configured root is now allowed alongside the cwd one, which stays for contracts written before the move; their absolute paths are in the database and still resolve. Note the sibling root on the next line already honoured STORAGE_PATH, so the helper was half-migrated already. persistSignatureImage() still wrote customer and admin signature PNGs under process.cwd(). It was missed because its path.join is spread over seven lines while the others are single-line — and the regression test compared against the single-line literal, so it reported green over a live bug. The test now collapses whitespace before matching, which is the only reason a formatting difference ever hid this. A sweep of the whole of src/ with the same normalisation confirms the remaining process.cwd()/storage references are all deliberate `STORAGE_PATH || cwd` fallbacks, not misses. Added a case that drives assertContractPdfPath against real files on disk — the guard realpaths both the file and its roots, so a test using imaginary paths proves nothing. It fails without the fix. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(storage): resolve the contract guard's root through the shared resolver The guard still built its own `STORAGE_PATH || <cwd>/storage`. That matches getStoragePath() only while STORAGE_PATH is set — with it unset the shared resolver falls back module-relative to <repo>/storage while this fell back to <cwd>/storage, and the backend is normally started from backend/, so the two name different directories. Writers and guard then disagreed about where contracts live and the download routes refused them, which is the same failure the previous commit fixed for the configured case, reappearing in the fallback case. One resolver on both sides now, which is the point of the whole change. Docblock updated to describe the three roots as they actually are. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(storage): make the fallback test safe, and align the backup diagnostics The test added in the previous commit was dangerous. To exercise the STORAGE_PATH-unset case it deleted process.env.STORAGE_PATH and then, in cleanup, recursively removed `<resolved root>/business-docs` — which with the variable unset resolves to the developer's real, gitignored <repo>/storage. Running `npm test` in a working checkout would have destroyed local business documents. This checkout has 65 MB there, including a populated business-docs tree. Rewritten to mock the shared resolver instead. That is both safe (every path stays in the tmpdir) and a sharper assertion: if the guard consumes getStoragePath() the mock moves its root, and if it went back to rolling its own expression the mock would have no effect and the test fails — which is exactly the regression being pinned. backupCoverageService and backupIntegrityService kept their own `STORAGE_PATH || cwd` roots. The backup walker itself already falls back module-relative, so with the variable unset the two diagnostics inspected a directory neither the walker nor the writers use and would report the business-docs tree as missing while it was in fact being backed up. Both now use the shared resolver. No regression: the same jest invocation over contract/quote/invoice/pdf/ backup suites gives an identical 11 failed, 24 passed before and after — those failures are a locally missing cron-parser dependency and reproduce on an unmodified tree. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-18 22:14:41 +02:00
Paul NothaftandPaul Nothaft	44adabca9e	test(e2e): read the admin JWT from the cookie, not the login body (#1071) ... Three specs acquire an admin token with `const body = await res.json(); return body.token`. The admin login has not returned a token in its body for some time — establishAdminSession() sets the JWT as the httpOnly `admin_token` cookie and responds with `res.json({ user })` — so the token was undefined and every one of them failed at the first assertion, before exercising anything they were written to cover. Server-side the cookie and an Authorization: Bearer header are interchangeable (see middleware/gallery.js, which reads the cookie first and accepts an admin-typed Bearer second), so the fix is to read the value back out of the context cookie jar and keep threading it as a Bearer. Every downstream call in these specs stays exactly as it was. Measured against a real stack, running only these three files: before 0 passed, 6 failed — all six at the token assertion after 3 passed, 3 failed The three that still fail no longer fail on auth: they get deep into the flow and then miss UI that has since changed (a settings label, a locator that no longer resolves). That is a separate and much larger staleness problem across this directory — a full run is 12 passed against roughly two dozen failures of that kind — and it is not addressed here. Worth knowing: no CI workflow runs tests/e2e at all, which is why this rotted silently while `npm run test:e2e` stayed documented in CLAUDE.md. Wiring it up is the obvious follow-up, but it has to wait until the suite is actually green, or it would just pin main red. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc Co-authored-by: Paul Nothaft <[email protected]>	2026-08-18 22:14:00 +02:00
Paul Nothaft	d62e21c1ba	chore(main): release 3.105.1-beta.0 (#1066)	2026-08-16 19:41:03 +00:00
Paul NothaftandPaul Nothaft	52db982661	fix(gallery): make per-event banner overrides actually work, both banners (#440, #932) (#1064) ... * fix(gallery): make per-event banner overrides actually work, both banners (#440, #932) The promo banner shipped with a per-event inherit/custom/off override that never reached a guest. GalleryView reads promo_mode from the /photos payload, and /photos never sent it — so every gallery resolved to 'inherit'. Setting a gallery's promo banner to "Off" did nothing; the global banner kept rendering. The info banner (#932) mirrored that shape and inherited the same gaps. Four places dropped the fields; all four now carry both banners: 1. GET /gallery/:slug/photos — send promo_mode/promo_markdown alongside the info fields. This is the fix that makes "Off" mean off. 2. POST /admin/events — the validators accepted both banners and the insert discarded them, so an API client could POST info_mode:'off', get 201, and find the row on 'inherit'. Markdown is stored only for 'custom', matching the PUT rule. 3. POST /admin/events/:id/duplicate — copy both from the source row. The dialog promises the copy "inherits the branding, behaviour, feedback, and category configuration"; a muted gallery un-muting on duplication is the opposite of that. 4. PUT /admin/events/:id — resolve the effective mode from the STORED row when a partial update sends only the markdown. Previously updates.promo_mode was undefined on such a request and the text was parked on an inherit/off gallery, then resurfaced when someone later switched it to 'custom'. The lookup is lazy: one extra query, only on that path. The two normalisation blocks are now one loop over both banners, so the pair can't drift apart again. Verified in a browser, both directions against the same global banner: promo_mode='off' -> not rendered; 'inherit' -> rendered. The /photos payload went from promo_mode ABSENT to carrying the value. * fix(gallery): thread promo into the reveal view, drop stale markdown on duplicate External review, round 1 on this PR. Two gaps in the plumbing it introduced: - The reveal-hidden branch copied only the info fields from /photos. Now that /photos carries promo too, a reveal-hidden gallery with promo_mode 'off' still fell back to 'inherit' and showed the global banner on the first load after login. Thread both banners there. - The duplicate copied markdown verbatim. A row written before the PUT normalisation landed can hold text while its mode is 'inherit'/'off', so the copy inherited hidden text that would resurface the moment someone switched it to 'custom' — violating the very invariant this PR establishes. Copy markdown only when the source mode is 'custom'. Test covers the stale-markdown source explicitly. --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-16 21:37:17 +02:00
Paul Nothaft	180f19d70d	chore(main): release 3.105.0-beta.0 (#1065)	2026-08-16 19:36:29 +00:00
Paul NothaftandPaul Nothaft	b48fa62eea	feat(gallery): info banner above the photo grid (#932) (#1063) ... * feat(gallery): info banner above the photo grid (#932) A short informational note rendered at the TOP of a gallery, above the photos. Distinct from the promotional banner (#440), which stays by the footer for marketing copy — the reporter's case is an onboarding hint ("use the menu button to filter"), which is useless below a gallery the guest has to scroll past first. Mirrors the promo feature's shape rather than inventing a second one: a global default in Settings → Branding (branding_info_markdown) plus a per-event inherit/custom/off override. Markdown via the existing MarkdownContent sanitiser — no raw HTML, no CSS injection. Empty global default means nothing renders, so upgrading changes nothing visible. Deliberately NOT included: an alignment knob (this is short helper copy, not marketing layout) and guest dismissal — the issue lists dismissal as a nice-to-have, and it needs per-guest persistence that is its own decision. Migration 176 is idempotent (hasColumn / existing-key guarded). Note on the payload plumbing: the per-event fields travel in the /photos response, not just /info. GalleryAuthContext seeds its cached event from the gallery LOGIN response — a small identity subset — so anything absent there is undefined right after a guest signs in. /photos is the payload that refreshes on every gallery load, which is why the fields were added there and why GalleryView reads them from `data.event`. Verified in a browser across all three modes; reading them from the context event instead silently collapsed every override back to 'inherit'. * fix(branding): map branding_info_markdown on read so saving can't wipe it (#932) External review caught this. BrandingSettings declared no info_markdown and formatBrandingSettings never mapped branding_info_markdown, so BrandingPage's hydration — setBrandingSettings(prev => ({ ...prev, ...formatted })) — kept the empty-string initializer instead of the persisted value. The form loaded blank and the next Save posted '' back, wiping a configured banner. Silently: the gallery keeps rendering the old copy until that save lands. This is the same bug the footer/promo fields hit in #441 + #440 / #460, which the read mapper still carries a comment about. Add the field to the interface and the mapper, and pin the round-trip for the whole editable branding set so the next field added is caught by a test rather than by a user losing copy. Verified: the new test fails 3/4 with the mapper line removed. * fix(gallery): honour the info-banner override in the reveal-hidden view (#932) External review, round 2. The hidden-until-reveal branch renders GalleryLayout with the context `event`, which is seeded from the gallery login response and carries no banner fields — so while a gallery was hidden, a per-event 'off' silently resolved to 'inherit' and the global banner appeared on a gallery the admin had muted. Resolve the fields there the same way the main render path does. The two full-page layouts (gallery-premium, gallery-story) are deliberately left alone: they return before GalleryLayout and render no header, footer or promo banner either — injecting a wrapper into layouts documented as having 'their own integrated UI' would be a design change, not a fix. --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-16 21:33:13 +02:00
Paul Nothaft	ab6ca6f82f	chore(main): release 3.104.1-beta.0 (#1061)	2026-08-16 17:34:09 +00:00
Paul NothaftandPaul Nothaft	3a11e6ebb5	fix(pdf): RFC 6266-encode Content-Disposition on quote/invoice PDFs (#1024) (#1055) ... * fix(pdf): RFC 6266-encode Content-Disposition on quote/invoice PDFs (#1024) 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. Note this corrects the issue's diagnosis: it reported umlauts as the 500 case, but umlauts are inside latin1 and mangle rather than throw. Both symptoms share this root cause and both are fixed here. Route through buildContentDisposition() (utils/filenameSanitizer, already used by secureImages.js), which emits an ASCII fallback plus the RFC 5987 `filename*=UTF-8''…` form, so the unicode name survives in browsers and the header stays legal. Applied to all six sites: adminQuotes (persisted + preview), adminInvoices (persisted + preview), customer (quote + invoice). Also correct buildPdfFilename's docstring, which advertised the preserved non-ASCII as suitable for Content-Disposition — the exact misreading that produced these call sites. * test(pdf): pin the ASCII fallback for fully non-Latin customer names (#1024) A name written entirely in another script leaves the legacy filename= token with just the document number (Q-2026-0042_.pdf) — filename* carries the real name. That's the intended trade, but it's the token a client without RFC 5987 support actually saves, so assert it stays legal, non-empty and carries the document number rather than leaving it unpinned. * fix(pdf): don't split surrogate pairs when truncating the filename (#1024) Codex review caught this. sanitiseSegment caps each segment at 80 UTF-16 code units, so a cap landing inside an astral character (emoji, rarer CJK) left a dangling high surrogate. encodeURIComponent throws URIError: URI malformed on a lone surrogate, so buildContentDisposition — the helper this PR routes the six PDF endpoints through — 500'd for e.g. company_name = 'a'.repeat(79)+'🎉', well inside the 120-char validator limit. Same 500 the PR set out to remove, reached a different way. Drop the orphaned surrogate instead of widening the cap, so the byte budget the limit exists to protect is unchanged. Tests cover both boundary cases and assert the cap semantics; they fail against the previous slice(). --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-16 19:31:19 +02:00
Paul Nothaft	959c18a864	chore(main): release 3.104.0-beta.0 (#1057)	2026-08-16 17:04:35 +00:00
Paul NothaftandPaul Nothaft	18b1e0f66e	ci(tests): run the gated real-Postgres .picpeak cases in the backend job (#1056) ... The .picpeak restore suites gate their Postgres cases behind PICPEAK_PG_TEST_URL and describe.skip themselves out when it is unset. That variable was set in no workflow, so those cases have never run in CI — the suites reported green while silently skipping the half that needs a real database: sequence resync, operator/role preservation across a cross-instance restore, and whether a coerced row lands with the right STORED VALUES rather than merely not throwing. Add a postgres:15-alpine service to the backend job (same shape schema-drift already uses) and point the variable at it. Everything else in the suite still runs on SQLite; this only un-gates the cases that were skipping. Verified against a real Postgres 15 before wiring: picpeakRestorePg 4/4 and picpeakCrossEngine 11/11 (8 of which were previously skipped across both). Matters now because #1043 opens sqlite -> pg restore to the upload UI, so the coercion layer's correctness stops being a CLI-only concern. Co-authored-by: Paul Nothaft <[email protected]>	2026-08-16 18:57:44 +02:00
Paul Nothaft	77cb65f5a2	chore(main): release 3.103.1-beta.0 (#1053)	2026-08-16 13:17:54 +00:00
Paul Nothaft	2728479d2a	chore(main): release 3.103.0-beta.0 (#1052)	2026-08-16 13:02:55 +00:00
Paul Nothaft	9b976386f9	chore(main): release 3.102.2-beta.0 (#1046)	2026-08-13 16:57:48 +00:00
Paul NothaftandPaul Nothaft	6de30e5bf1	fix(docker): default NODE_ENV=production so non-compose deploys don't fall back to SQLite (#1038) (#1039) ... * 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 (nothing did before, which is why this went unnoticed for so long) - 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. The engine-selection tests resolve knexfile in a child process with a clean cwd — dotenv.config() would otherwise let a developer's backend/.env decide the answer instead of the knexfile defaults under test. Fake credentials in the describeEngine tests are built at runtime rather than written inline, so secret scanners don't flag a literal after `password:`. 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 from the previous commit 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. Nothing moves until the operator decides. - once Postgres holds the data, the next restart switches over on its own. - an explicit DATABASE_CLIENT is always honoured. The check is 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 and strand the operator on an empty install. wait-for-db.sh resolves the engine and exports DATABASE_CLIENT before the migration step, so the runner and the server always agree. Manual migration runs (no entrypoint, no exported client) now refuse rather than build a schema in the wrong database. Adds scripts/migrate-sqlite-to-postgres.js for moving the data across. It reuses the .picpeak export/import services rather than hand-rolling a cross-engine copy — they already handle FK suspension, JSON columns and Postgres sequence resync. Two things had to be added 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. Postgres rejects both outright ("date/time field value out of range: 1786548038763"). Coercion is driven by the TARGET schema, never guessed from the value. Verified end to end against a real PostgreSQL 15: a seeded SQLite install migrated across with booleans, timestamps and foreign keys intact, and the serial sequences correctly advanced (the next INSERT got id 2, not a primary-key collision). Photo files on disk are never touched and the SQLite file is left in place as a rollback. 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 <[email protected]>	2026-08-13 18:50:56 +02:00
Paul NothaftandPaul Nothaft	89dc9623c1	fix(feedback): persist guest feedback settings, unshadow the guest route (#1030) (#1031) ... 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 <[email protected]>	2026-08-13 18:50:52 +02:00
Paul NothaftandPaul Nothaft	34ee31141b	fix(gallery): coerce SQLite 0/1 booleans in the guest surface (#1028) (#1034) ... 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 <[email protected]>	2026-08-13 18:50:48 +02:00
Paul NothaftandPaul Nothaft	671c4dbd56	fix(events): make event_date/expires_at nullable on SQLite (#1029) (#1035) ... 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 <[email protected]>	2026-08-13 18:50:42 +02:00
Paul Nothaft	e2832725ea	chore(main): release 3.102.1-beta.0 (#1026)	2026-08-11 08:41:17 +00:00
Paul Nothaft	054c342c33	chore(main): release 3.102.0-beta.0 (#1025)	2026-08-11 07:49:42 +00:00
Paul Nothaft	8e3573788b	feat(downloads): per-gallery download resolutions (#858) (#1022) ... Clients who need smaller files no longer make the photographer re-export. Two capabilities, both off by default. STANDARD RESOLUTION — the size a gallery hands out for every ordinary download (single, selected, download-all). Global default in Settings, overridable per gallery with the NULL=inherit tri-state. The pre-built download-all zip is built AT the standard resolution, so changing it invalidates those archives, including a fan-out to inheriting galleries. RESOLUTION PICKER — opt-in modal letting guests choose a different size. Custom archives are built as a DB-backed job the client polls, never cached. The picker never offers a size above the standard, and Original reappears only when the admin explicitly allows it. Resize is fit:'inside' + withoutEnlargement — aspect preserved, never upscaled — applied before the watermark, since the mark is sized relative to its input. Three rounds of external review hardened this: job archives are bound to the requester's visibility scope and re-validated at delivery, the streamed download-all path applies the cap, queue admission is bounded, and rejected resolutions no longer inflate download stats. Closes #858.	2026-08-11 09:46:46 +02:00
Paul Nothaft	02deac9f10	chore(main): release 3.101.5-beta.0 (#1021)	2026-08-10 11:36:23 +00:00
Paul Nothaft	75bfad2b6a	fix(slideshow): stop "no crop" fit letterboxing a pre-cropped frame (#1015) (#1018) ... 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, so portrait photos lost their top and bottom and 'Black Bars (No crop)' looked inert. 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. Fixes #1015.	2026-08-10 13:32:14 +02:00
Paul Nothaft	08e0098597	chore(main): release 3.101.4-beta.0 (#1016)	2026-08-10 09:03:45 +00:00
Paul Nothaft	e3830cd921	fix(deps): bump nanoid and js-yaml out of two HIGH advisories (#1013) ... 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), transitive via postcss - js-yaml 4.3.0 -> 4.3.1 (GHSA-5p4m-2wfm-xmqj, quadratic CPU in !!omap resolution), direct dependency Lockfile-only; the existing ^ ranges already permitted both fixes. Clears the two open Trivy code-scanning alerts on main.	2026-08-10 10:59:53 +02:00
Paul Nothaft	03671f3b91	test(e2e): anchor the admin login button locator so SSO doesn't break it (#1012) ... With OIDC enabled the login page also renders a 'Sign in with <provider>' button whose accessible name matches the unanchored /Sign In/ locator, so Playwright strict mode failed every test that logs in — 7 of 13 in the local smoke suite, which is also the pre-push gate. CI never hit it because its databases seed without OIDC config. Anchors the regex to the full accessible name in all six call sites.	2026-08-10 09:53:18 +02:00
Paul Nothaft	5503e6ca5e	chore(main): release 3.101.3-beta.0 (#1011)	2026-08-10 07:18:08 +00:00
Paul Nothaft	a607cea110	fix(auth): issuer-tag the oversize SSO logout marker (#798) (#1010) ... Phase 3 validated a stored ID token hint against the currently configured issuer, but the oversize path never got that check: an ID token above the 3.9KB cookie limit was stored as the bare string 'sso', which collapsed to an undefined hint at logout and skipped validation entirely. Changing the issuer while such a session was live bounced the user to the new IdP on logout. Stores sso.<base64url(issuer)> instead and moves all marker interpretation into buildEndSessionUrl: raw ID token -> iss/aud-validated hint, issuer-tagged marker -> round-trip without a hint, anything else -> no round-trip. Every branch fails closed. Refs #798.	2026-08-10 09:14:56 +02:00
Paul Nothaft	fbe1d07228	chore(main): release 3.101.2-beta.0 (#1009)	2026-08-10 06:30:01 +00:00
Paul Nothaft	1bf19a7caf	fix(branding): route the gallery footer through <PoweredBy /> (#1008) ... Closes #1003. #999 centralised the attribution so branding_hide_powered_by is honoured everywhere, but GalleryLayout kept its own inline guard. The gallery footer therefore still flashed — it kept `!brandingSettings?.hide_powered_by`, where undefined is falsy, so a white-labelled instance briefly showed the attribution on first paint, on the surface a white-label customer is most likely to see. And there were two implementations of one rule, which is the bug class #999 existed to close. The footer appends the attribution to its copyright line inside an existing <p>, so a straight swap would nest a <p> in a <p>. Added an inline variant rendering a <span> that carries the leading ' | ' itself: the separator belongs to the component, since a caller placing its own would have to repeat the visibility guard to avoid leaving a dangling separator when the attribution is hidden. No extra request — GalleryView already uses usePublicSettings(), the same hook and react-query key, so the cache is shared. The footer also picks up common.poweredBy, so it is translated rather than hardcoded English. Removes the now-unread hide_powered_by from GalleryLayout's prop type and the mapping feeding it in GalleryView. Four cases cover the variant — span not paragraph, separator present, separator hidden with the attribution when white-labelled, hidden while loading. Each was checked against the pre-fix shape: rendering a <p> or moving the separator out breaks one.	2026-08-10 08:27:28 +02:00
Paul Nothaft	4ec93107a9	chore(main): release 3.101.1-beta.0 (#1007)	2026-08-10 06:19:11 +00:00
Paul Nothaft	1c242d401f	test(transfers): pin the PicTransfer ownership guards (#1006) ... Closes #1005. The two ownership guards added during the #998 review were correct on merge but untested. They are the only thing between a scoped admin and every other admin's ORIGINAL files, since a transfer serves those over an unauthenticated token URL. 14 cases: filterOwnedPhotoIds (own / foreign / ownerless-legacy / mixed / non-existent / super_admin), addFiles gating on the same rule, listTransfers scoping plus the absence of token/upload_token/download_url/upload_url from the list payload, and getTransferOwner. Each was checked against the pre-fix behaviour rather than only passing against current code — reverting each guard in turn fails exactly the cases covering it: ownership filter 3, list scoping 1, payload strip 1, guard registered late 1. requireTransferOwnership is module-local, so its two contracts are asserted at the source following the #596 pattern: that router.use('/:id', ...) precedes every /:id route — ordering is the whole mechanism, and a late registration would guard nothing while still looking present — and that missing and foreign ids both answer 404, so the endpoint is not an existence oracle. Tests only; no production code touched.	2026-08-10 08:10:54 +02:00
Paul Nothaft	80599a5e47	chore(main): release 3.101.0-beta.0 (#1004)	2026-08-09 11:43:36 +00:00
Paul Nothaft	e2d8ec86bd	chore(main): release 3.100.2-beta.0 (#1002)	2026-08-09 09:19:07 +00:00
Paul Nothaft	1f224f4ead	chore(main): release 3.100.1-beta.0 (#996)	2026-08-04 15:33:44 +00:00
Paul Nothaft	b9e42591f5	docs: the retired registry path freezes, it does not stop serving (#995) ... Closes #985. README and migration-to-org.md both claimed the old path 'is no longer served'. It is served — ghcr.io/the-luap/picpeak/backend:latest returns a complete image, created 2026-05-27, label version: main. The registry responds normally; it just never receives anything new. That inaccuracy is what generates reports like #982. Told the path is not served, an operator runs docker compose pull, watches it succeed, runs docker rmi and pulls again, watches that succeed too, and concludes the problem lies somewhere other than their image path. Nothing reports an error anywhere; the only symptom is an update notice that never resolves. Say what actually happens — the path freezes rather than failing — and add a self-diagnosis via docker image inspect on both paths, with the 2026-05-27 date and the 'main' version label as the tells. MigrationBanner's wording is left alone: 'no longer being updated' was accurate. This is the delivery mechanism for #985. There is no in-app channel: MigrationBanner shipped a month after the freeze, the #993 update-check notice cannot fire on installs running their own frozen backend, and the changelog modal that renders release notes shipped two days after the freeze. What reaches these operators is GitHub, and the GHCR page for the retired package — which renders this README through the images' own org.opencontainers.image.source label, so the fix propagates to the dead path's own page automatically.	2026-08-04 17:30:18 +02:00
Paul Nothaft	9dc2b2166e	chore(main): release 3.100.0-beta.0 (#994)	2026-08-04 14:52:13 +00:00
Paul Nothaft	137a42f259	feat(admin): surface the registry move through the update check (#993) ... Relates to #985 — does NOT close it. Adds registryMigrationRequired to the update-check payload (stable channel below 3.45.0) and an amber block in UpdateNotification explaining that the retired registry path still responds, so `docker compose pull` appears to succeed while serving the same frozen build. Known limitation, established in review and merged deliberately: this cannot reach the operators #985 describes. PicPeak is self-hosted, so the update-check code runs inside the operator's own image — a v3.44.0 install runs v3.44.0's backend forever, and the only external call returns release metadata, not logic. Every build containing this predicate is >= 3.45.0, where it is false by definition. The release-notes fallback fails too: the changelog modal shipped 2026-05-29, two days after the freeze. Correct for any future rename, no runtime cost, but #985 stays open — the population it describes still has no in-app channel. Viable routes are external (retired GHCR package description, repo README, docs). '0.0.0' is excluded from the predicate: that is getCurrentVersion's fallback for an unreadable package.json, i.e. a broken install, not a pre-rename one.	2026-08-04 16:36:50 +02:00
Paul Nothaft	0c8ad6bbed	fix(security): vet the destination project when linking a deal (#991) ... linkDealToProject re-points a deal's quotes, contracts and events into `projectId`. Its lineage guard vets the SOURCE events and its comment assumed the route had vetted the destination — true only for attachDocumentToProject. quoteService.create/update and contract crud.create/update take `projectId` straight from the request body behind quotes.manage / contracts.manage, which are permissions, not ownership; adminQuotes.js and adminContracts.js carry no ownership guard at all. The lineage guard did not cover it: it is skipped when the deal has produced no event yet, which is the state of a newly created quote, and an unassigned destination ADOPTS the deal's customer rather than rejecting it. A scoped admin could therefore write into another admin's project, and on an OWNERLESS project (created_by IS NULL — legacy rows migration 167 could not attribute) escalate to a read: once the quote converts to an event it becomes the project's only linked event, which is the condition ownedProjectsSubquery's second branch grants ownership on. Vetted at the service choke point all four callers share, ahead of both the null-deal early return (callers write project_id before calling, and deal_uuid is nullable) and the customer check (whose 422 vs 404 was an enumeration oracle). 404 PROJECT_NOT_FOUND throughout. super_admin unaffected.	2026-08-04 16:36:12 +02:00
Paul Nothaft	083b3d86b0	chore(main): release 3.99.2-beta.0 (#989)	2026-08-04 12:40:03 +00:00
Paul Nothaft	6c03feaef5	fix(deps): bump ip-address, brace-expansion and postcss for open CVEs (#987) ... Closes Trivy code-scanning alerts #414-#418 on the backend image. brace-expansion 5.0.8 -> 5.0.9 CVE-2026-69152 (high) DoS via unbounded intermediate arrays ip-address 10.2.0 -> 10.4.0 CVE-2026-69192 (high), CVE-2026-54272 and CVE-2026-69198 (medium) — SSRF and trust-boundary bypasses. Needs 10.3.1+ to clear all three. postcss 8.5.18 -> 8.5.23 CVE-2026-69153 (medium) information disclosure via crafted sourceMappingURL ip-address and brace-expansion were already in overrides but pinned below the new fixed versions; the floors just needed raising. postcss reaches the image through sanitize-html — the direct pin is not an import, it forces the transitive copy to dedupe to a known version, so it moves with the bump. Only the backend image is affected: the frontend production stage is nginx:1.30-alpine and ships no node_modules. Each lockfile now holds exactly one entry per package, all at or above the fixed version, and the image installs via npm ci --omit=dev so the lockfile is authoritative.	2026-08-04 14:35:57 +02:00
Paul Nothaft	0ef836df19	chore(main): release 3.99.1-beta.0 (#986)	2026-08-04 12:21:08 +00:00
Paul Nothaft	4b53b64277	fix(accounting): gate cross-add counters on the permission their endpoint checks (#984) ... Closes #983. The two cross-add counter queries added in #979 were enabled on customers.edit, but neither endpoint checks that permission: HoursSection -> GET /expenses/inbound/by-customer/:id needs accounting.view CustomerCrmPanels -> GET /customers/:id/hour-entries needs customers.view An admin holding customers.edit but not the corresponding read permission fired a guaranteed 403 on every customer-detail render. It degraded safely — the count stayed at its 0 default so the cross-add was never offered, which is the right outcome for that role — so this was request noise rather than broken behaviour. Each guard now requires both: the read permission to fetch the count, and the write permission because there is no point offering the cross-add to someone who cannot create the combined invoice. No seeded role is affected: migration 123 grants accounting.view and accounting.manage together, and customers.edit projects forward from customers.create, which migration 090 always grants alongside customers.view.	2026-08-04 14:17:34 +02:00
Paul Nothaft	83d514315e	chore(main): release 3.99.0-beta.0 (#980)	2026-08-03 20:06:19 +00:00
Paul Nothaft	d66425c8ee	chore(main): release 3.98.6-beta.0 (#978)	2026-08-03 12:52:55 +00:00
Paul Nothaft	67592fc569	fix(projects): stop the cockpit offering email controls the API rejects (#976) ... Closes #969. The cockpit's email feed rendered preview/resend/cancel/retry/send-now for every mail, consulting neither the caller's role nor their permissions, producing controls that always failed: 404 - requireOwnedQueuedEmail scopes queued mail through email_queue.event_id AND ownership of that event. CRM document mail carries no event_id; and project ownership does not imply event ownership, so a project the caller owns can hold another admin's event. 403 - preview needs events.view but the four 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. The cockpit reads canAct and combines it with email.send. A missing canAct reads as false. Regression from the GHSA-93x4 fix in #960/#966, which added the ownership middleware.	2026-08-03 14:48:50 +02:00
Paul Nothaft	6699855c93	fix(auth): fail closed when the adminAuth roles join errors (#974) ... Closes #968. The roles-join fallback in adminAuth fabricated role_name='super_admin' on ANY database error, so a transient fault (connection reset, deadlock, statement timeout, pool exhaustion) silently granted super_admin for its duration. roleName is the sole discriminator for every ownership check, so this inverted the authorization model rather than failing the request. Gate the fallback on isMissingRolesSchema(), moved to utils/dbErrors.js and shared with apiTokenAuth. The predicate was also tightened: knex prefixes the failing SQL to err.message and that SQL always names `roles`, so the old /roles/i gate was vacuous and a generic /does not exist/ could accept unrelated faults. Now trusts SQLSTATE 42P01/42703 on Postgres and exact driver phrasing on SQLite.	2026-08-03 14:48:20 +02:00
Paul Nothaft	569ae39acb	chore(main): release 3.98.5-beta.0 (#973)	2026-08-02 19:27:37 +00:00
Paul NothaftandPaul Nothaft	7c0c0a5b7f	fix(security): enforce project ownership on project + project-email routes (GHSA-wrg5, GHSA-93x4) (#960) ... * 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 * docs(security): drop the stale ownership JSDoc left by the rebase (GHSA-wrg5) Rebasing onto main (which had gained scopeEventsQuery from #957) 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 <[email protected]>	2026-08-02 21:24:38 +02:00
Paul Nothaft	3fc6463873	chore(main): release 3.98.4-beta.0 (#971)	2026-08-02 19:22:22 +00:00
Paul NothaftandPaul Nothaft	164129b8f5	fix(security): escape brand tokens, block tracker redirects, trim logo diagnostic (GHSA-j347, mw76, 29vm) (#961) ... * 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 '&amp;'. 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 * 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 --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-02 21:17:17 +02:00
Paul NothaftandPaul Nothaft	e2ce95ee48	fix(security): enforce event ownership on the v1 API surface (GHSA-9697) (#957) ... * 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 --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-02 21:17:14 +02:00
Paul NothaftandPaul Nothaft	1b4e5fee3e	fix(security): bound inbound-mail resources, redact secrets from logs (GHSA-2qf9, pgmp, r794) (#959) ... * 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. * 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 --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-02 21:17:11 +02:00
Paul NothaftandPaul Nothaft	da855cfef9	fix(security): scope dashboard stats/analytics/activity to the caller's events (GHSA-c2jj, gqx7, jhcf) (#958) ... * 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 --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-02 21:17:08 +02:00
Paul NothaftandPaul Nothaft	0d4c30884e	fix(security): backup/restore hardening — public-dir DB dump, restore path allowlist, gunzip bound, manifest keying (#956) ... * 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 --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-02 21:17:05 +02:00
Paul Nothaft	acd6b453d1	chore(main): release 3.98.3-beta.0 (#954)	2026-08-02 20:25:32 +02:00
Paul NothaftandPaul Nothaft	1c8f7d58a8	fix(security): redact gallery share tokens from analytics tracking (GHSA-7m6c) (#952) ... * 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 <[email protected]>	2026-08-02 08:38:33 +02:00
Paul NothaftandPaul Nothaft	c2ce12c039	fix(security): authz/ownership gaps (token binding, auth revocation, feedback/customer ownership, token logging) (#950) ... * fix(security): close authz/ownership gaps (secure-download binding, photo-auth+logout revocation, feedback/customer ownership, token logging) * fix(security): codex round-1 — complete admin-token invalidation + preserve foreign assignments - photoAuth: mirror adminAuth's active-admin lookup + iat<password_changed_at check in the admin branch, so a deactivated admin or a pre-password-change token can no longer fetch every photo (GHSA-x55x was only revoke+cutoff). - adminAuth logout: revoke req.token (the token adminAuth authenticated with, cookie OR header) instead of header-only, and clear the auth cookie — a cookie-based logout previously left the JWT live (GHSA-cjqh). - adminCustomers PUT /:id/events: preserve the customer's existing assignments to events the caller does NOT own, so a restricted admin can't revoke another admin's customer-event links via full-list replacement. * fix(security): codex round-2 — don't 403 legit restricted-admin assignment edits The Manage-galleries dialog submits the full initial assignment list, so a restricted admin editing a customer that already has a foreign assignment hit the denied.length 403 before the preservation logic ran. Reject only NEWLY-supplied foreign/nonexistent ids; retain foreign ids the customer is already assigned to (they can't be added or removed by a non-owner). --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-02 08:38:24 +02:00
Paul NothaftandPaul Nothaft	8f91c2ca99	fix(security): neutralize spreadsheet formulas in all CSV/export cell-writers (CSV injection cluster) (#948) ... Co-authored-by: Paul Nothaft <[email protected]>	2026-08-02 08:37:56 +02:00
Paul NothaftandPaul Nothaft	9050affd8d	fix(security): unauth share_token leak (HIGH) + restore path-traversal, logo file-read, branding path keys (#946) ... * fix(security): stop unauth share_token leak + block restore path-traversal, logo-path file read, branding path keys * test: update resolveLogoFile for the c7x5 containment (reject outside-storage absolute paths, keep inside) * fix(security): codex round-1 — escape LIKE wildcards in share-link resolve, keep in-storage absolute logos, guard restore verification - shareLinkService: escape %/_ in the link_partial LIKE fallback so an anonymous /resolve/____… wildcard can't match an arbitrary share_link and leak its bearer token (reopened GHSA-rh8r). Explicit ESCAPE for SQLite. - resolveLogoFile: re-add the raw absolute candidate but keep it subject to the storage-root containment filter (GHSA-c7x5) so legit in-storage absolute logos resolve while /etc/passwd stays rejected. - restoreService: apply the same pathEscapes guard in post-restore verification so a skipped traversal entry isn't fs.access'd/hashed. --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-02 08:37:48 +02:00
Paul Nothaft	8cbb37310b	chore(main): release 3.98.2-beta.0 (#945)	2026-08-01 17:37:57 +02:00
Paul NothaftandPaul Nothaft	82d68711cf	fix(security): close authorization/ownership gaps (token scope, mass-assignment, category hero, project docs) (#943) ... * fix(security): close authorization/ownership gaps (token scope, mass-assignment, category hero, project docs) * fix(security): block archive columns in event mass-assignment per review * fix(security): comprehensive event mass-assignment denylist + deal-cascade cross-domain permission gate (codex r2) * fix(security): case-insensitive complete event denylist + project_id + empty-update no-op (codex r3) --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-01 17:36:27 +02:00
Paul NothaftandPaul Nothaft	b7005692b3	fix(security): resolve DNS before vetting external hostnames (SSRF cluster) (#941) ... * fix(security): resolve DNS before vetting external hostnames (SSRF cluster) * fix(security): harden SSRF fix per review (rsync backup path, S3 config-save, webhook transient-DNS retry) * fix(security): S3 endpoint validation on any endpoint update + no-connect on unresolved webhook host (codex r2) --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-01 17:36:21 +02:00
Paul NothaftandPaul Nothaft	8a87c9274b	fix(security): block guest access to hidden/client-only photos across bulk + secure routes (#939) ... * fix(security): block guest access to hidden/client-only photos across bulk + secure routes * fix(security): harden hidden-photo fix per review (stale ZIP cache, legacy token mint, SQLite bool, client rebuild) * fix(security): invalidate ZIP cache on photo visibility/category change (codex r2) * fix(security): recheck photo visibility at signed/secure serve time (TOCTOU) + invalidate ZIP on client visibility change (codex r3) --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-01 17:36:15 +02:00
Paul NothaftandPaul Nothaft	fe615c82e4	fix(security): bump sanitize-html to 2.17.5 (CVE-2026-53606) (#937) ... Co-authored-by: Paul Nothaft <[email protected]>	2026-08-01 17:36:09 +02:00
Paul Nothaft	cf37ad5389	chore(main): release 3.98.1-beta.0 (#935)	2026-08-01 10:32:31 +00:00
Paul NothaftandPaul Nothaft	defeae9634	fix(uploads): prevent cross-photo contamination from filename collisions and non-atomic writes (#931) (#933) ... * 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) * test: make the suffix-uniqueness check deterministic-in-practice (#931) * fix(uploads): widen the anti-collision suffix to 48 bits (#931) * fix(uploads): hide staging files from list() + share one watermark limiter process-wide (#931) * fix(uploads): reclaim orphaned staging files + revalidate watermark settings in queued jobs (#931) --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-01 12:29:12 +02:00
Paul Nothaft	2581f4af70	chore(main): release 3.98.0-beta.0 (#930)	2026-07-31 09:56:00 +02:00
Paul NothaftandPaul Nothaft	3bcded78a4	feat(gallery): multi-select feedback filters + sort direction controls (#889) (#929) ... * feat(gallery): multi-select feedback filters + sort direction controls (#889) * fix(gallery): keep mobile sidebar open while combining feedback filters (#889) * fix(gallery): generic sort icon when direction is uncontrolled (#889) --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-07-31 08:57:08 +02:00
Paul NothaftandPaul Nothaft	08ff9f20e7	feat(gallery): per-event toggle to hide the logo on the password page (#894) (#928) ... * feat(gallery): per-event toggle to hide the logo on the password page (#894) * fix(admin): harden login_logo_visible coercion for SQLite + string booleans (#894) --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-07-31 08:56:48 +02:00
Paul NothaftandPaul Nothaft	926a4a540d	feat(gallery): mouse-wheel zoom at cursor in the lightbox (#885) (#927) ... * feat(gallery): mouse-wheel zoom at cursor in the lightbox (#885) * fix(gallery): chain rapid wheel events synchronously + handle page-mode deltas (#885) --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-07-31 08:56:29 +02:00
Paul Nothaft	39d397c086	chore(main): release 3.97.6-beta.0 (#926)	2026-07-30 12:28:18 +00:00
Paul NothaftandPaul Nothaft	03087c798c	fix(security): close GHSA-g94x (cross-gallery photo read) + GHSA-pv6w (admin DB export) (#924) ... * fix(security): close two access-control advisories (GHSA-g94x, GHSA-pv6w) 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. * test: stub requireSuperAdmin in the backup masking mock adminBackup now calls requireSuperAdmin() at load (GHSA-pv6w export gate), and backupSecretMasking mocks the permissions module — add the new function to the mock so the module loads. * fix(security): review follow-ups on the export gate (GHSA-pv6w) - test: place the mocked export in its own mkdtemp dir. The route recursively deletes path.dirname(filePath) after download, so a stub in bare os.tmpdir() made the super_admin test wipe the whole temp root — other jest workers' DB files included (latent CI flake). - ui: hide PicpeakExportCard from non-super_admins. The role keeps settings.view + backup.create, so after the gate its Download button always 403'd with a generic toast; gate the card on role super_admin to match the endpoint. * fix(security): keep the token-mismatch audit values within varchar(20) (GHSA-g94x review) image_access_logs.access_type is varchar(20) (migration 038), but 'token_gallery_mismatch' is 22 chars — on Postgres the audit write threw value-too-long and logImageAccess swallowed it, so the security event went unrecorded (the 403 still fired; log is best-effort). Shorten to 'photo_mismatch' / 'gallery_mismatch' (14/16). --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-07-30 14:24:28 +02:00
Paul Nothaft	342dde3589	chore(main): release 3.97.5-beta.0 (#922)	2026-07-30 10:19:02 +00:00
Paul NothaftandPaul Nothaft	252475fce2	fix(admin): code-review follow-ups on #910/#916 (MIME resolver + expiry reactivity) (#921) ... * 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): 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): 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): 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): 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. * 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 <[email protected]>	2026-07-30 12:14:41 +02:00
Paul NothaftandPaul Nothaft	fbc18a386b	ci: batch stable releases into one daily version (#919) ... * ci: batch stable releases into one daily version 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) - P1: the daily job runs on a schedule, so a fork PR can spoof the head branch name 'release-please--branches--stable' — gh --head matches the name only. Pin --base stable AND require isCrossRepository == false so a fork PR can never be approved+auto-merged with the release PAT. - P2: this scheduled job is now the ONLY automatic stable cut, so the auto-merge-enable step no longer swallows failures (|| true); it fails loudly and verifies autoMergeRequest is actually set. A silently expired PAT would otherwise stop releases while the workflow stays green. Approve stays tolerant (re-approval can return non-zero). * ci: accept an immediately-merged release PR as success (review round 2) gh pr merge --auto merges immediately when required checks are already green — the normal 18:00 case, since fixes land hours earlier and CI passes. The autoMergeRequest verify then saw null on a MERGED PR and failed the job on the happy path. Now: MERGED = success, pending auto-merge = success, still-open-with-no-auto-merge = real failure. * ci: read release-PR state + auto-merge in one snapshot (review round 3) Two separate gh pr view calls raced: a pending auto-merge completing between them made the first read OPEN and the second read null on the now-merged PR, failing the job on a successful release. Fetch state and autoMergeRequest together. --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-07-30 12:14:21 +02:00
Paul Nothaft	55b344b531	chore(main): release 3.97.4-beta.0 (#918)	2026-07-29 20:26:14 +00:00
Paul NothaftandPaul Nothaft	487f55f2d9	fix(admin): stop marking events expired up to 24h early (#909) (#916) ... 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. Co-authored-by: Paul Nothaft <[email protected]>	2026-07-29 22:23:55 +02:00
Paul NothaftandPaul Nothaft	aca3c8e4bc	fix(admin): expose view/download counters in the admin photos list (#895 follow-up) (#914) ... * fix(admin): expose view/download counters in the admin photos list (#895 follow-up) 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. * test: widen the fire-and-forget settle window (#895 follow-up) The 100ms settle was marginal on loaded CI runners — the counter increments are deliberately fire-and-forget, and the 909 PRs flaked on exactly these assertions. 400ms keeps the suite fast while giving slow runners room. --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-07-29 22:23:38 +02:00
Paul Nothaft	888150ba2d	chore(main): release 3.97.3-beta.0 (#913)	2026-07-29 19:48:17 +00:00
Paul NothaftandPaul Nothaft	67c56c5b61	fix(admin): serve videos with their real MIME type in the admin photo view (#908) (#910) ... * fix(admin): serve videos with their real MIME type in the admin photo view (#908) 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. --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-07-29 21:32:33 +02:00
Paul Nothaft	1ee7fe7336	chore(main): release 3.97.2-beta.0 (#906)	2026-07-29 16:02:10 +00:00
Paul NothaftandPaul Nothaft	78116e2e8b	fix(analytics): make per-photo view/download counters actually count (#895) (#904) ... * fix(analytics): make per-photo view/download counters actually count (#895) Three stacked defects behind 'per-image stats stay 0': - photos.view_count had NO writer anywhere — the admin IMAGES table and photo viewer display it, so it was permanently 0. It now increments when the full-size photo or its preview tier is served, excluding the slideshow kiosk (migration 138 design) and follow-up video Range requests (seeks are not views). Fire-and-forget so analytics can never fail the byte-serving path. - Zip downloads (download-all, presigned download-all, download-selected) never incremented per-photo download_count — only single-photo downloads did, so zip-heavy galleries showed 0 forever. The zip routes now bump exactly the photos that went into the archive (the prebuilt-zip path mirrors the archive builders' category filter). - Every admin surface used a different definition of 'downloads', which is the reporter's 46 vs 45 vs 0: event details counted only action='download' (no zips at all), the dashboard counted download+download_all but silently EXCLUDED download_selected and download_all_presigned. All queries now share one action set: download, download_all, download_all_presigned, download_selected. New photoEngagementCounters suite pins all of it (7 tests). * fix(analytics): count views via an explicit lightbox beacon (#895 review round) External review flagged that request-level view counting is wrong in both directions: the lightbox preloads prev/next neighbours (3 fetches per open) while a preloaded neighbour promoted by a swipe is never re-fetched (#505 keeps the DOM node), and enhanced/maximum galleries never hit /photo at all (bytes come from /api/secure-images). - Views now count via POST /:slug/photo/:photoId/view, fired by the lightbox exactly when a photo becomes the visible slide; the serving-route increments are removed. Covers protected galleries and the preview tier uniformly; slideshow kiosk stays excluded. - bumpEventDownloadCounts mirrors downloadZipService._build (ALL event photos) — the category filter mismatched the prebuilt zip's actual contents. (That the builder ignores per-category allow_downloads is a separate pre-existing issue.) - Zip loops count only successfully appended entries, with a pre-append storage stat: a lazy stream's async error bypassed the per-photo catch and hung the whole response — pre-existing bug, now fixed. Suite extended to 9 tests (beacon semantics, serve-does-not-count, skipped-entry exclusion). * fix(analytics): fire the view beacon from the premium lightbox too (#895 review round 2) gallery-premium events use yet-another-react-lightbox inside GalleryPremiumLayout instead of PhotoLightbox, so the layout never counted views. yarl's on.view fires on open and on every slide change — identical semantics to the PhotoLightbox beacon. Also documents the accepted prebuilt-zip approximation: _build can skip entries whose watermark step fails and still publish the archive; counting those exactly would need a persisted zip manifest. * perf(analytics): skip the per-entry zip preflight on S3 (#895 review round 3) The pre-append source check exists for LocalFs's lazy createReadStream (async error would kill the whole zip response). S3's get() awaits GetObject and rejects inside the loop's try/catch on a missing key, so a HEAD per entry was a redundant serial round trip — 500 extra HEADs on a 500-photo zip. --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-07-29 17:58:54 +02:00
Paul Nothaft	5171105938	chore(main): release 3.97.1-beta.0 (#901)	2026-07-29 10:40:56 +00:00
Paul NothaftandPaul Nothaft	d9ad982373	fix(tests): raise migration-boot hook timeout pins to the 120s default (#900) ... The 3.97.0-beta.0 release PR (#899) failed its backend Tests job on slideshowPublic.test.js: bootCrmDb's full migration chain crossed the suite's explicit 30s beforeAll timeout argument on a slow runner. #860 raised the config default and the jest.setTimeout pins to 120s, but hook-ARGUMENT pins override the config default and were left behind — same time-bomb, different syntax. Every beforeAll that boots the migration chain and pinned 30s/60s is raised to 120000 (16 suites). Untouched on purpose: the three suites whose pinned hooks don't run migrations (webhookDelivery, imageProcessor.storage, storageBackend) and publicQuotes' 30s pin on the rate-limit lockout test — neither grows with the migration chain. No test logic changed. Co-authored-by: Paul Nothaft <[email protected]>	2026-07-29 12:38:26 +02:00
Paul Nothaft	564e816bec	chore(main): release 3.97.0-beta.0 (#899)	2026-07-29 09:46:14 +00:00
Paul NothaftandPaul Nothaft	6a048d08bd	feat(feedback): let guests remove their star rating (#884) (#893) ... * feat(feedback): let guests remove their star rating (#884) Clicking your current rating again clears it. rating: 0 is the wire contract: the validator now accepts 0, and the service deletes the guest's rating row (instead of storing a 0 that would drag the photo average down) and recalculates photo stats. The lightbox stars send 0 on a same-star click; PhotoRating already did, but the backend rejected it with a 400 until now. * fix(feedback): harden the rating-clear path (#884 review round) External review follow-ups: numerically normalize the clear sentinel so a numeric-string "0" can't slip into the update/insert paths (validator now also toInt()s), delete the full guest-scoped rating set on clear so racy duplicate rows can't survive in the average (same defense as the reaction path), and refresh the visible average/count after the identity-modal submit path like the direct paths do. * fix(feedback): round-2 review fixes for rating clear (#884) - Clear sentinel matches only an explicit 0 / "0" — malformed input (undefined, NaN, garbage strings) can no longer delete a rating. - Lightbox survives the photo list shrinking while open (clearing your rating under the Rated filter drops the photo on refetch): index is re-anchored and the lightbox closes when the list empties, instead of crashing on an out-of-range index. - Story layout gets the same same-star-to-clear behavior, keyed off the session-local my-rating map, and an explicit 0 no longer falls back to displaying the photo average. * fix(feedback): refresh guest-scoped caches after rating changes (#884 review round 3) - GalleryView's onFeedbackChange now also invalidates ['my-feedback', slug]: in guest identity mode the Rated/Liked filter membership and chip counts come from that query (#538), so a cleared rating never left the Rated filter until the 30s staleTime lapsed. - PhotoRating invalidates gallery-photos + my-feedback on success: the parent refetch fires optimistically in onMutate and could capture pre-mutation state, with nothing refreshing after the server accepted. --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-07-29 11:34:23 +02:00
Paul Nothaft	435c558704	chore(main): release 3.96.1-beta.0 (#898)	2026-07-29 09:30:02 +00:00
Paul NothaftandPaul Nothaft	ec66cd2684	fix(gallery): keep the lightbox toolbar from masking the photo (#888) (#892) ... The bottom info/action bar was a translucent gradient overlaying the image, hiding the lower edge of the photo. The bar is now opaque and the image area stops above it (measured via ResizeObserver, since the bar height varies with flex-wrap, the optional filename line and safe-area padding), so the photo is always fully visible. Co-authored-by: Paul Nothaft <[email protected]>	2026-07-29 11:27:29 +02:00
Paul Nothaft	9f7c644e0a	chore(main): release 3.96.0-beta.0 (#897)	2026-07-29 09:25:35 +00:00