1251 Commits

Author	SHA1	Message	Date
Paul Nothaft	ca8050899b	chore(main): release 3.122.4-beta.0 (#1270)	2026-09-02 08:56:21 +00:00
Paul Nothaft	f722bdaf4b	Merge pull request #1268 from PicPeak/fix/1265-guest-identity-persistence ... fix(guests): keep guest identity across a tab close (#1265)	2026-09-02 10:49:15 +02:00
Paul Nothaft	63fa05b181	chore(main): release 3.122.3-beta.0 (#1269)	2026-09-02 08:27:26 +00:00
Paul Nothaft	f2f40893c1	fix(guests): drop a stored identity when a spent invite names someone else ... A guest coming back through their own already-redeemed link is the ordinary #1265 case, and the identity the device holds is theirs. The same link opened on a shared device that holds another guest's identity is not: the redemption 409s, ensureIdentity() falls through to the stored identity, and the visitor's likes are filed under the previous person. The two cases were indistinguishable client-side, so the 409/410 body now carries the invite's guest_id. On a mismatch the stored identity is cleared and the visitor is asked who they are. A response without guest_id keeps the previous behaviour.	2026-09-02 10:22:55 +02:00
Paul Nothaft	19e125d814	fix(security): raise the general limiter's fallback budget to 300 ... The general /api limiter had been inert since it was written, so its 100 requests per 15 minutes per IP was never exercised against real traffic. Applying it for the first time with that budget would have 429'd a venue wifi NAT after roughly twenty guests per window, since every call a gallery landing page makes before the password is typed counts. 300 keeps the protection and clears the realistic case. An explicit app_settings value still wins over this fallback.	2026-09-02 09:43:11 +02:00
Paul Nothaft	23a433f411	docs(analytics): state the tracker proxy's trust model ... The SSRF vetting is resolve-then-fetch and production-only. Say so, and say why that is acceptable: the hostname is admin-controlled, the request is confined to allowlisted paths and carries no PicPeak credentials, and the S3/MinIO client already takes the same posture.	2026-09-02 09:43:11 +02:00
Paul Nothaft	a912817ec8	refactor(archives): use the shared LIKE escape helpers ... The local escapeLike copy and its comment predate 0ef51148, which stopped escapeLikePattern() doubling single quotes. The comment was therefore false and the helper byte-identical to the shared one. Use escapeLikePattern() + likeWithEscape(), as every other search does.	2026-09-02 09:43:11 +02:00
Paul Nothaft	5a0c9f53b0	fix(security): rate-limit the password-change endpoints per IP too ... POST /api/auth/admin/change-password and POST /api/customer/profile/password both verify the current password before replacing it, which makes them a credential check an attacker holding a hijacked session can drive at will: the session's own JWT skips the general limiter as authenticated, and they were not in the auth gate's table. Both join it. Only failures count, so the one change a user legitimately makes costs nothing.	2026-09-02 09:43:11 +02:00
Paul Nothaft	4515632300	fix(migrations): judge each German field on its own in migration 195 ... repairGerman gated subject, body_html and body_text on body_html alone, the same defect Codex found in migration 194: an admin who had translated only the subject lost it the moment the HTML still matched English, and down() is a deliberate no-op, so the loss was unrecoverable. Each field is now judged independently for both the translations row and the legacy _de columns, matching 194's corrected pattern. Two tests pin the two directions (translated subject over English body, and the reverse).	2026-09-02 09:43:11 +02:00
Paul Nothaft	a929affd7e	fix(security): close the case-sensitivity bypass in the API rate limiter ... Express's `case sensitive routing` is off by default, so /API/admin/events reaches the same handler as /api/admin/events. Both the gate's `/api/` prefix test and rateLimitService's public-endpoint classification compared the raw path, so simply upper-casing a letter skipped the limiter entirely. Verified against a real Express app before fixing: /api/admin/events routes and hits the gate; /API/admin/events and /Api/Admin/Events route and miss it. Both now match on a lower-cased path. The auth gate added alongside was already immune -- its patterns carry the `i` flag for exactly this reason. Not changed: rateLimitSecurity.hasValidAdminToken's /api/admin/ test has the same shape, but there the case-sensitive comparison fails safe -- an upper-cased path simply does not get the admin skip, so it is rate limited rather than exempted. Making it case-insensitive would widen a skip, so it is left alone. maintenance.js's isAdminRoute is fail-safe for the same reason.	2026-09-02 09:43:11 +02:00
Paul Nothaft	50e8ed6e58	fix(security): apply per-IP rate limiting to credential endpoints ... The five authRateLimiter registrations were inert for the same reason the general one was -- registered below the error handler. Auth endpoints have never had an IP limit; the 5-attempt behaviour QA observed is the per-account lockout in authSecurity.js, which is a different mechanism and is untouched. They could not simply be activated: app.use('/api/auth', ...) is a prefix, so a 5-per-window budget would have covered GET /api/auth/session and POST /api/auth/password-strength, which the frontend calls far more than five times per window. That locks users out. The real surface was enumerated by loading the routers and walking router.stack rather than grepping, which showed two of the five registrations pointed at routes that do not exist: adminAuth.js has no /login (admin login is POST /api/auth/admin/login) and there is no /api/gallery/:slug/verify (gallery verify is POST /api/auth/gallery/verify). Now limited, on exact method+path: admin login, admin MFA verify, gallery password verify, share-login, client PIN, setup verify-token, setup admin, customer login, customer password-reset. Deliberately unlimited: session checks, password-strength, logouts, authenticated change-password, the SSO round-trip (a 429 on the callback breaks login from shared corporate IPs), and one-time invite/accept-invite links. Two choices carry the design. skipSuccessfulRequests means only failed attempts spend budget, which is what makes 5-per-IP survivable behind NAT -- ten guests on one venue wifi all typing the correct gallery password consume nothing -- and means a legitimate admin cannot be locked out by their own success. And the limiter keeps its own rateLimit() instance, hence its own store and its own per-IP bucket, with the general gate's auth exemption left in place: sharing a counter is exactly the lockout described above. Patterns are case-insensitive because Express's case-sensitive routing is off by default, so POST /api/auth/admin/LOGIN reaches the login handler and a case-sensitive pattern would have been a free bypass. max is now read per request, so the Settings UI's rate_limit_auth_max_requests applies without a restart, matching the general limiter. Tests prove both directions: each credential endpoint 429s on attempt 6 with the response shape the four login pages already branch on, each benign endpoint still returns 200 after 40 calls, the two buckets are independent in both directions, and 30 consecutive successful logins consume no budget. Refs testplan REPORT.md, rate-limiter gap.	2026-09-02 09:43:11 +02:00
Paul Nothaft	30ac4140af	chore(backend): teach eslint the rest-sibling omission idiom ... Adds varsIgnorePattern and ignoreRestSiblings to no-unused-vars, the config recommendation left open when the lint backlog was cleared. The "omit fields via rest spread" idiom is intentional and recurring -- adminEvents/helpers.js destructures password_hash and client_password_hash purely to keep them out of `...rest` -- and without ignoreRestSiblings every occurrence needs its own disable comment, which is noise that also suppresses genuine findings on the same line. Removed the one such comment that now exists; the explanatory comment above it stays, since the intent is not obvious from the code. Lint stays at 0 problems. Refs testplan REPORT.md D1.	2026-09-02 09:43:11 +02:00
Paul Nothaft	15fdd70a08	fix(search): match the original filename, and honour the date-format setting ... Two warnings, both of which turned out to be mis-stated. Search: the name printed on every card is photos.original_filename (not source_filename, which is the replacement-stable ingest key and is not in the gallery payload at all), but search matched only the stored renamed filename. So a substring the admin or guest can literally read on screen returned zero results. Fixed on the admin Photos tab, which filters server-side -- grouped OR, because the feedback AND/OR conditions are appended immediately below and a bare orWhere would leak across them -- and on the Story theme's own scene filter, which is a second independent client-side search box. Dates: the warning read "Transfers uses DD/MM/YYYY while the rest of the app uses long-form dot dates", but it is inverted. TransfersPage already routes every date through useLocalizedDate and was correctly honouring the rig's own configured general_date_format of {"format":"DD/MM/YYYY","locale":"en-GB"}. The surfaces it was compared against are the ones ignoring the admin setting, by passing an explicit format string that overrides it. Dropped the hardcoded 'MMM d, yyyy' from the two EventsListPage table dates so they follow the setting like Transfers does. AdminHeader's format(new Date(), 'PPPP') is left as-is: that is the decorative "today" banner, where a long weekday form is a deliberate design choice rather than a data date, and forcing it to DD/MM/YYYY would read worse. Refs testplan REPORT.md, search-by-original-filename and transfers-date warnings.	2026-09-02 09:43:10 +02:00
Paul Nothaft	b0f33c1744	fix(security): actually apply the general API rate limiter ... app.use('/api/', generalRateLimiter) lives inside initializeRateLimiters(), which is defined at line 463 but not called until 1048 -- by which point the routers (767+), the /api notFoundHandler (1002) and errorHandler (1029) are already on the stack. All six app.use() calls in it therefore append BELOW the error handler and can never see a request. generalRateLimiter had no other registration path. So the entire /api surface had no IP-based request limit, except the handful of routes carrying their own inline rateLimit() (public quotes, contracts, payment-check, transfers, the analytics proxy). The admin Settings rate-limiting UI -- rate_limit_enabled, rate_limit_max_requests -- was writing to a control that did nothing. Fixed with a stable gate registered above the routers that resolves the limiter per request, so there is no boot delay: it is a pass-through until initializeRateLimiters() resolves, exactly matching prior behaviour. Registered unmounted (app.use(gate), not app.use('/api', gate)) because Express strips the mount path from req.url and rateLimitService's own logic is written against the full path -- req.path.startsWith('/api/public/') and the /api/(gallery|secure-images)/:slug regex it uses to find a gallery token to skip on. Mounting it would have silently broken both. Deliberately excluded, each for a concrete reason: - /health and /api/health, mounted above the gate: a 2s probe is 450 req/window and would 429 the container healthcheck. - /api/public/transfer and transfer-upload: one request per file from a link holder with no JWT, so never skipped as authenticated; a large transfer would be cut off mid-way. Both already have tighter per-minute limiters. - login and gallery-verify: the limiter returns authMaxRequests (5) as their budget but counts them into the SAME per-IP bucket as every other /api call, so the branding and settings fetches a login page makes before anyone types a password would 429 the login itself for a full window. Giving these a real per-IP limit means giving them their own bucket. Bulk gallery and admin traffic is unaffected: skip_authenticated defaults true and cookie tokens are promoted to Authorization before the gate runs, and skipped requests do not increment the counter. Also adds /api/health as an alias of /health -- one handler, identical exposure -- which silences a ~2s probe warning. Registered above the API middleware chain deliberately: left at its original position it would have passed through apiRequestLogger and through maintenanceMiddleware, whose skip-list contains /health but not /api/health, so it would have 503'd during maintenance while /health returned 200. The tests pin registration depth by source inspection as well as behaviour, because depth is what was broken and no unit test of the gate can catch it. Refs testplan REPORT.md, /api/health warning; rate-limiter gap found while fixing it.	2026-09-02 09:43:10 +02:00
Paul Nothaft	a89057df1d	fix(search): stop escapeLikePattern corrupting bound search values ... Verified against a real SQLite connection -- each of these returned zero rows before and the right row after: "Sarah's" before=[] after=["Sarah's Birthday"] "100%" before=[] after=["Summer 100% Sale"] "Gala_" before=[] after=["Gala_Night"] Two bugs in one helper. It did .replace(/'/g, "''"), which is SQL string-quote doubling -- meaningless and actively corrupting for a value that is bound, so any search containing an apostrophe matched nothing. And its \% escaping had no ESCAPE clause on the LIKE, which is engine-dependent: honoured on Postgres, a literal backslash on SQLite, so % and _ stayed wildcards there. Now mirrors the correct implementation from 59666b59: escape \ % _ only, and a new likeWithEscape(column) emits `col LIKE ? ESCAPE '\'`. Both call sites move to whereRaw with the value still bound; the column argument is a literal, documented in the JSDoc. Callers checked before changing the contract: adminPhotos.js, adminEvents/crud.js, and sqlSecurity's own addLikeCondition(), which has no callers anywhere -- pre-existing dead export, updated to the new shape rather than deleted. Behavioural change: searches containing ' % _ or \ now return the right rows instead of nothing. Case sensitivity is unchanged. Refs testplan REPORT.md, escapeLikePattern finding.	2026-09-02 09:43:10 +02:00
Paul Nothaft	34685505be	fix(analytics): serve self-hosted trackers same-origin so CSP stops blocking ... A self-hosted Umami/Rybbit domain configured in Settings could never load: the CSP script-src allowlist is static, and the earlier pass could only add an admin-visible warning because nginx.conf:58 strips helmet's header and location / serves the SPA document off disk via try_files -- so helmet can never govern it in Docker. Verified by reading the config, not inferred; that kills the "make helmet dynamic" option outright. Rather than templating the CSP, the tracker is now same-origin. The script and every endpoint it talks to are served from /api/analytics/tracker/* and proxied server-side to the configured instance, so script-src 'self' and connect-src 'self' already cover it. The CSP is unchanged: nothing to template, no env var, no restart -- it takes effect when Settings is saved. That also closes A3 structurally rather than by widening a directive. Endpoint mapping taken from vendor sources, not guessed: Umami's host || currentScript.src + /api/send, and Rybbit's documented /track, /site/tracking-config/<id>, /site/<id>/feature-flags/evaluate. data-host-url is set explicitly so a COLLECT_API_HOST-built Umami cannot bypass the proxy. Session replay is deliberately NOT proxied: replaying gallery pages would capture the share token (GHSA-7m6c). nginx still needed one line, for a non-obvious reason: the static-asset regex location outranks the plain /api prefix in nginx's matching order, so /api/analytics/tracker/script.js resolved as a static file. Confirmed empirically against a real nginx:alpine -- 404 before the ^~ block, 502 (proxied) after, with /assets/app.js and /api/public/settings unchanged. The native SERVE_FRONTEND install needed no change; helmet already has 'self' in both directives and the proxy mounts ahead of express.static. Security boundary, since this makes the server fetch an admin-supplied URL: closed per-provider path+method allowlist (4 paths), DNS-resolving isHostAllowed blocking private/internal/metadata addresses in production (matching the s3Storage prod-only precedent), base rebuilt as origin + pathname so userinfo/query/fragment cannot smuggle anything, redirect: 'error', cookie/authorization/referer/host never forwarded, an HTML upstream response re-served as application/octet-stream + nosniff, and 64KB request / 2MB response / 5s timeout / 120rpm caps. X-Forwarded-For and User-Agent are forwarded so geo and device attribution survive. Residual, stated plainly: an unauthenticated rate-limited relay to one admin-chosen public host on 4 paths, and TOCTOU DNS rebinding is unmitigated as it is elsewhere in the repo. The Umami and Rybbit panels now explain they are proxied; the Custom panel keeps a CSP warning -- it is the one mode with nothing to proxy -- naming both script-src and connect-src. Refs testplan REPORT.md A2, A3.	2026-09-02 09:43:10 +02:00
Paul Nothaft	fe5ac9162d	fix(photos): emit visibility and processing_status from the list mapper ... The "hidden photo has no indicator on the admin grid" warning was not a missing badge. The badge markup has existed since #172; the defect was in GET /:eventId/photos, which hand-builds its response literal field by field and never emitted `visibility` -- so the value was always undefined and neither the grid tile nor the list row badge could render. Same omission class as the view_count/download_count bug already commented in that file. (The `visibility` line itself was swept into 4721bd83, whose message does not mention it -- recording that here.) Fixes the adjacent instance too: `processing_status` is missing from the same mapper, so the grid's "Processing…" and "Failed"/Retry placeholders could never render either. On the card, reuses the existing EyeOff badge pattern from the list-view rows, adds a tooltip on both layouts, and drops the category badge to top-9 so the hidden badge can own the top-left corner. Also fixes the Photo Limit spinbutton's aria-valuemax, which read 0 even with a real cap set. Root cause: min={0} with no max -- for input[type=number] Blink's MaxValueForRange returns DBL_MAX, fails isfinite and supplies no max, so a11y tooling prints the default 0. Set to 2147483647, the events.photo_cap column's real signed-32-bit ceiling (migration 074), which also stops an out-of-range value failing only at INSERT. The sibling expires_in_days input already had proper bounds. Known: EventInformationCard carries the identical Photo Limit input with the same defect; it is held by another concurrent change and follows next. Refs testplan REPORT.md, hidden-photo and aria-valuemax warnings.	2026-09-02 09:43:10 +02:00
Paul Nothaft	a28f96b304	fix(gallery): no-store private JSON, and give guest uploads a real status ... B6 -- seven gallery routes returned private, per-guest data with no Cache-Control at all, relying on heuristic freshness. noStoreCache is mounted per route rather than on the router, because the media routes set their own private, max-age=1800/3600 and must keep it. Covered: /photos (own likes/favourites/ratings, hidden photos for a client token), /people, /stats, /verify-token/:token (an authorization decision -- a cached {valid:true} outlives a rotated token), /show/:token/session (the response IS a credential; it mints a gallery JWT), /show/:token/state and /download-jobs/:token (live polls, where a cached "preparing" strands the caller). Deliberately untouched: the photo/thumbnail/hero/preview and css-template routes, which set their own caching, the binary downloads, and /info + /resolve, which are unauthenticated public metadata rather than per-guest private. ETag/304 revalidation is intact and pinned by a test: no-store stops the browser retaining the body, not express agreeing an unchanged payload is unchanged. That matters because the post-upload poll depends on it. B7 -- the guest upload flow had no progress signal, so the UI polled the photo list blind and gave up after 60s with no explanation. Adds GET /:slug/uploads/status?ids=... rather than pending counts in the photos payload: counts there are event-wide, so another guest's or the admin's stuck upload would spin the notice forever and it could never say "your photo failed". Authorization: verifyGalleryAccess already resolves req.event from the caller's token, and the query is scoped `.where('event_id', req.event.id)`, so an id from another gallery matches no row -- neither a cross-event read nor an existence oracle, since it returns all-zero counts rather than a 403/404 that would confirm the id exists elsewhere. Slideshow tokens are denied (a kiosk never uploads). Ids are pattern-validated, max 50. The response is counts only: no filenames and specifically no processing_error strings, which can carry internal paths. Not gated on allow_user_uploads, so an admin flipping the toggle mid-flight does not strand an in-progress guest. The frontend now finishes on the real terminal condition, refetches as each photo lands rather than only at the end, shows a processing pill, and reports real failures instead of silently timing out. Refs testplan REPORT.md B6, B7.	2026-09-02 09:43:10 +02:00
Paul Nothaft	7c9baff751	fix(upload): scope category ids, stop temp-file leaks, split the video cap ... Four related fixes on the admin upload/photo path. B5 -- PATCH /photos/:photoId and POST /photos/bulk-update took any parseInt(...) > 0 straight into the update with no existence or scope check, so a photo could be moved into another event's category. The upload route already validated `event_id = X OR is_global` per #500/#525; extracted that query as findScopedCategory() and used it on all three routes so the 400 body is byte-identical. 0/negative/'individual'/'collage'/null still clear without a lookup, so the clear path costs no extra query. B9 -- three distinct temp-file leaks, not one. The validator's size branch never unlinked; the cleanup lived in the final handler, unreachable on any 400; and multer's `destination` callback runs per file and overwrote req.tempUploadPath, so even the success path only ever removed the last file's directory. Now: discardUploadedFiles() runs on every 4xx and the 500 (ENOENT tolerated, and files are only dropped when the whole request is being rejected, so the passing path is untouched); cleanup registered before multer so it also covers multer's own LIMIT_FILE_SIZE return; one directory per request. B8 -- the admin uploader filtered on MIME only, so an oversized file was uploaded in full before the server's 400. Mirrors UserPhotoUpload's existing per-file toast-and-drop. C4 -- general_max_file_size_mb was a single cap for photos and videos, so the 50MB default meant admins could not upload ordinary video without also raising the photo limit. Adds general_max_video_size_mb (default 500MB, clamped by the same 10GB MAX_ALLOWED_FILE_SIZE_MB ceiling, read per request, 60s cache), editable in Settings -> General. Photo uploads are protected from regressing by keeping multer's type-blind limit at max(photoCap, videoCap) and moving the per-kind decision into validateUploadContent, where file.mimetype exists. It 400s with the existing message shape, so an oversized photo is still rejected with the identical body it produced when multer did the rejecting. Known gap: chunked-upload/init still applies the photo cap to video. Making it video-aware would change an existing assertion that pins a 200MB video init being rejected under a 1MB general cap. No component calls that path today and the direction is strict rather than a bypass, so it is left as-is. Guest video uploads still share the single cap in gallery.js. Refs testplan REPORT.md B5, B8, B9, C4.	2026-09-02 09:43:10 +02:00
Paul Nothaft	57dd084763	fix(events): add archive_size to the immutable column deny-set ... IMMUTABLE_EVENT_COLUMNS is documented as a COMPLETE deny-set that new server-managed columns must be added to. archive_size is written by archiveService from the zip's real byte count and is what the archives list now sorts and displays, so an events.edit holder could otherwise set a cosmetic size on a non-archived event. Follow-up to 59666b59, which added the column.	2026-09-02 09:43:10 +02:00
Paul Nothaft	42ba8351c1	fix(middleware): log ownership lookup failures; drop dead auth surface ... ownership.js caught a lookup failure, returned 500 and logged nothing -- the file had no logger import, so a failing ownership check was invisible in the logs. Added logging matching photoAuth.js/permissions.js ({ error, stack } plus the relevant id), response behaviour unchanged. Fixed both swallowed catches: requireEventOwnership, the reported one, and the byte-identical requireProjectOwnership. Also removes AdminAuthContext.updatePasswordChanged, now dead -- superseded by the deliberate full-page reload in onSuccess, with zero callers left. setMustChangePassword and mustChangePassword stay; nothing else orphaned. Refs testplan REPORT.md B13, B16.	2026-09-02 09:43:10 +02:00
Paul Nothaft	103863cbab	fix(quotes): enforce the status state machine, and correct the table ... VALID_QUOTE_TRANSITIONS was a complete-looking quote state machine that nothing consulted, so status changes were unvalidated. Mapping every writer of quotes.status (quoteService.js is the only one -- dealsService, projectService, adminDashboard and customer.js all read) showed the table itself was wrong: six legitimate transitions were missing. sendQuote allows draft/declined/expired -> sent but the table had draft only; adminAcceptQuote allows draft/sent/expired but had sent only; adminDeclineQuote allows draft/sent/expired but had draft/sent; recordResponse had no same-status entry. Enforcing it as written would have broken accept-on-behalf from a draft, resend-after-decline, every expired revival and the 15-minute response-toggle window. So the table is reconciled to reality first, then assertQuoteTransition() (409, QUOTE_INVALID_TRANSITION) is called at all seven sites. Two things worth carrying forward. Nothing in the codebase ever sets 'expired' -- the header comment says "set by the scheduler" and there is no such scheduler; sent -> expired is retained as documented intent only. And the backstop's added value is narrow: every reachable invalid transition is already caught by a call site's own better-worded guard, which fires first. What it newly catches is a status the machine has never heard of -- a legacy or corrupt row like 'cancelled' sails through adminAcceptQuote's guard, which only excludes accepted/declined/converted, and used to be silently overwritten. That is what the new tests pin. Refs testplan REPORT.md B4.	2026-09-02 09:43:10 +02:00
Paul Nothaft	a7d45ddd0d	fix(workflows): restore the once-per-process seed guard ... `booted` was assigned but never read, so the guard's early return was missing and the builtin workflow seeder ran on every call. Impact was wasteful, not harmful: seedOneBuiltin is idempotent -- it keys on builtin_key and returns early when adminOwned or storedVersion >= def.version, writing a graph only on a fresh insert or a version bump. So repeat calls cost a lookup per builtin plus a graph rebuild, with no duplicate rows. `booted = true` stays inside the try, so a seed that never got off the ground (workflows table not migrated, DB down) leaves the flag clear and retries. A per-builtin failure is still swallowed by the inner catch and does not block the flag, unchanged. Restoring the guard broke workflowEngine.test.js, which calls the boot seeder seven times in one worker and needs the second call to run in two of them. Followed the existing _backupPathsBoot/_restoreSettingsBoot precedent: exported _resetBootForTests(). Refs testplan REPORT.md B3.	2026-09-02 09:43:10 +02:00
Paul Nothaft	41e1de7818	fix(email): repair and seed the gallery lifecycle templates ... Correction: the reported premise held for only one of the three templates, verified by running the core migration set against an empty database. - expiration_warning is German-is-English on every fresh install, exactly as reported. Repaired with migration 194's pattern verbatim. - gallery_expired and archive_complete are NOT German-is-English -- they do not exist at all. Their master rows are inserted only by migrations/legacy/ 010+020, which never run on a fresh install, so 075/099/106/108 seeded zero translations for them (they key off a master row that is not there). A fresh install's email_templates holds 17 keys and neither is among them. The consequence is worse than a translation gap: expirationChecker's sendGalleryExpiredEmails and archiveService's completion mail both hit "Email template not found", retry three times and die silently in email_queue on every expiry and every archive. So 195 also seeds those two (master row + en/de translations + category), but only when the master row is absent -- it never overwrites. English follows legacy 028, which emailProcessor's own comments call the shipped copy; German follows legacy 026's wording. Both are restructured into the plain unstyled shape the other core-seeded templates use, so wrapEmailHtml's configurable palette governs styling rather than hard-coded hex. The support-contact line is wrapped in {{#if support_email}} because getSupportEmail() can return ''. 196 adds the {{#if welcome_message}} block that nl/pt/ru/fr/es/sl already have in gallery_created but en and de lack, so the photographer's personal note was silently dropped for those two locales even though the value is passed at send time. safeTemplateReplace does resolve {{#if}} before variable substitution, so this is a real conditional -- there is a test rendering the migrated body both ways. HTML body only, matching the other locales: emailProcessor rewrites welcome_message through formatWelcomeMessage (escape + nl2br) once for both bodies, so the text part would print literal <br /> and &amp;. Both migrations keep 194's conservative condition -- rewrite only while the German is still byte-identical to English or empty -- so admin-edited and legacy-translated installs are untouched. Idempotent, guarded, no-op down(). Known gap, documented in 195's header: the two newly seeded templates get en/de only. nl/pt/ru/fr/es/sl fall back to en via processTemplate's fallback chain, which is strictly better than today's hard failure but is not real localisation. Refs testplan REPORT.md B1, B2.	2026-09-02 09:43:10 +02:00
Paul Nothaft	afc5779ce7	fix(events): return 409 instead of 500 when a slug is taken ... Correction to the reported cause: both create paths already loop `while (await db('events').where({ slug }).first())` before inserting, so a sequential duplicate never 500s -- it gets -1 appended. The 500 is purely the read-then-insert race: two concurrent creates for the same name+date both clear the check and the loser's INSERT trips events_slug_unique. isDuplicateSlugError(), built on the existing utils/dbErrors.isUniqueViolation, is wired into the catch of POST / and POST /:id/duplicate -> 409 { code: 'EVENT_SLUG_TAKEN' }. The predicate is deliberately narrower than isUniqueViolation: on PG it matches err.constraint, on SQLite the specific "UNIQUE constraint failed: ... events.slug" text. A loose message test would misfire because knex prefixes the whole INSERT -- which always names slug -- to err.message, and events has other unique columns (share_token). PUT /:id cannot collide: slug is in IMMUTABLE_EVENT_COLUMNS. No other adminEvents sub-router writes slug. CreateEventPage already toasts data.error, so no frontend change is needed. The test makes the race deterministic without timers: it hooks knex's `query` event and injects the colliding row the instant the route issues its slug-existence SELECT. The route then spends a full bcrypt hash before its own INSERT, so the injected row always lands first. Refs testplan REPORT.md B10.	2026-09-02 09:43:10 +02:00
Paul Nothaft	da6e34d6a3	fix(archives): sort and total on real archive sizes, escape LIKE wildcards ... Closes the three trade-offs the server-side archives query deliberately accepted. C1 -- the sorted number and the displayed number are now the same one. There was no archive_size column, so the Size column came from a per-row fs.stat done after pagination while the sort fell back to summed photo bytes: the list could be ordered by a number the user was not looking at. Adds events.archive_size (bigInteger -- int4's 2.1GB ceiling is the same limit that forced the restore path off adm-zip), written at archive time from archive.pointer(), which is the exact byte count the completion email already reports. The route now sorts and displays that column and no longer touches the filesystem. The migration backfills by stat-ing every archive_path where the column is null, outside the column guard so a half-finished run self-heals; unstatable rows (missing zip, S3-backed storage) stay null, order last via COALESCE and display 0 -- exactly what the old fs.stat produced for a file it could not read. Restore nulls it alongside archive_path. Accepted: the list no longer notices a zip deleted out of band and shows the last recorded size. The detail route still stats the real file. C2 -- escape \ % _ in the bound value plus an explicit ESCAPE '\'. The ESCAPE clause is load-bearing rather than decorative: SQLite has no default LIKE escape character, so without it the escaped pattern matches literal backslashes and the search silently returns nothing on SQLite while working on Postgres. The value stays bound; no interpolation. C3 -- the four stat cards aggregated only the current page, so every total was wrong for any dataset past page one. The list response now carries totals { archives, photos, archiveSize } computed with the same applyFilters() closure as pagination.total, so cards and footer cannot drift. Two aggregate queries: archive_size sums on the unjoined events query (joining photos multiplies it by photo count) and photos count on the joined one, both read back through Number() for pg's bigint-as-string. The "Showing X of Y" line moved out of the totalPages > 1 guard so it survives a single-page result, now gated on total > 0 so a zero-result search does not render "Showing 1 to 0 of 0"; only the page controls stay conditional. Test fixtures deliberately order zip sizes differently from summed photo bytes, so the sort test can only pass on the right column. Refs testplan REPORT.md C1, C2, C3.	2026-09-02 09:43:10 +02:00
Paul Nothaft	0ae424ff42	test(migrations): pin migration 194's per-field guard ... The per-field fix landed without a test for the case it exists for: an admin-translated subject over a still-English body, and the reverse.	2026-09-02 09:43:02 +02:00
Paul Nothaft	77b11ab874	fix(upload): enforce the chunked-upload cap on bytes received, not declared ... The init route checked the client-declared fileSize against general_max_file_size_mb, but nothing checked what then came through the chunk route: a client could declare `fileSize: 1` and stream any amount, and completeUpload only logged the size mismatch before handing the merged file on. The cap the earlier commit added at init was therefore a gate with no fence. The service now carries the cap from init and enforces it on the running byte total per chunk (aborting the upload once crossed, since the chunks on disk are already over the limit), rejects chunk indices outside the announced range, and re-checks the merged file as a backstop. Both routes answer 413/400 for these instead of a blanket 500.	2026-09-02 09:33:34 +02:00
Paul Nothaft	814f205da0	fix(feedback): make the "block" severity tier actually reject ... The block level is advertised as "comment is rejected immediately", but every non-approved comment was saved with is_approved = false instead of the submission being refused. moderateText now sets an explicit `blocked: true` on the blocking-violation branch -- branching on the reason string in the route would have been fragile -- and the route 400s with code COMMENT_BLOCKED and stores nothing. Everything else that is not approved (moderate/high, the spam and caps checks, and the "Moderation system error" fallback) deliberately omits the flag and keeps the held-for-moderation path, so a moderation failure still fails safe. Also fixes an adjacent defect that made the tier split unobservable: feedbackService.submitFeedback ignored feedbackData.is_approved entirely and hard-derived is_approved from moderate_comments. So a moderate/high word-filter hit on an event with moderation switched OFF was published immediately -- the route's `feedbackData.is_approved = false` was dead code. Now honoured one-directionally: a caller-supplied false is respected, but nothing a caller passes can RELAX the event's setting. That deliberately leaves the route's reputation.autoApprove -> is_approved = true branch inert rather than letting a trusted guest bypass an event's moderation setting. Refs testplan REPORT.md B11. (cherry picked from commit b1b57b1615aaf02fe76e789a86b7e11933288d77)	2026-09-02 09:30:49 +02:00
Paul Nothaft	da8fcc82ef	fix: per-field template guard, LIKE escaping, wait for all uploads ... Codex review round 1 on #1266. Migration 194 gated all three German fields on body_html alone, so an admin who had translated only the subject would lose it the moment the HTML still matched English -- and down() is a deliberate no-op, making that loss unrecoverable. Each field is now judged independently, for both the translations table and the legacy _de columns. Archives search escapes LIKE wildcards. % and _ are literal characters to the client-side includes() this replaced but wildcards to LIKE, so searching "100%" matched every archive and reported a nonsense total. The ESCAPE clause is load-bearing: SQLite has no default LIKE escape character, so without it the escaped pattern matches literal backslashes there while working on PG. The post-upload poll waits for every queued file. Each is processed independently, so stopping at the first new photo left the rest of a multi-file upload hidden until a manual refresh -- the exact symptom the polling was added to prevent. UserPhotoUpload now reports how many files the server accepted. (The latter two are superseded by stronger fixes in #1267 -- the upload-status endpoint and the shared escape helper -- but each PR has to be correct on its own.)	2026-09-02 08:41:36 +02:00
Paul Nothaft	6e5755de02	fix(types): resolve the TypeScript build:check backlog ... 74 errors -> 1. No suppressions: zero `any`, `as unknown as`, `@ts-ignore` or non-null `!` added, and tsconfig is untouched. Each error was triaged as "the type is wrong" vs "the code is wrong" and fixed on that side. Live bugs the checker was pointing at: - admin.service.ts TS1117 duplicate key: admin_password_reset was defined twice and the later one won at runtime. Removed it so the earlier entry wins, which matches the actual emitter in userManagementService.js and carries the email fallback. - PhotoGridWithLayouts dropped allowReactions from its prop type, so the Premium layout's reactions never activated even though GalleryView passes it and GalleryPremiumLayout reads it. - SlideshowPage's poll never copied `order` into next/prev, so live play-order changes never reached a running kiosk. - CustomerLayout compared branding_force_color_mode against 'auto', which is never persisted (only 'dark'|'light'|null), so the customer portal always picked the light logo even in OS dark mode. - EmailConfigPage rendered lang.flag, but SUPPORTED_LANGUAGES exposes Flag, a component -- so nothing rendered. And editing a language with no translation yet spread undefined, storing a partial object missing required fields. - publicQuotes.js projected only 6 line-item fields, omitting parentLineItemId/parentPosition/detailsText, so the migration-119 sub-item hierarchy and details text could never render on the customer-facing quote page -- the frontend code for it was unreachable. It reads from the same quoteService.getQuoteById the admin route uses, where those fields are present; adminQuotes.js projects all three. Fixed the projection rather than adding fields to the frontend type, which would have compiled while leaving the feature broken. - DuplicateEventDialog's helper text was silently dropped: LocalizedDateInput had no helperText prop. Added, mirroring Input.tsx incl. aria-describedby. - ThemeEditorModal/EventThemeSection still passed isPreviewMode, a prop 822be9a9 deliberately removed but missed at these two call sites. - GalleryPage's hero-photo injection was dead: /gallery/:slug/info does not return hero_photo_id (only /photos does) and GalleryView already does it correctly. Removed the dead block rather than adding a field the API never sends. Stale types corrected against the backend route that produces each payload: GalleryInfo (allow_downloads, allow_user_uploads), GalleryData.event (download_zip_ready), UpdateEventData (client_access_enabled, client_password, regenerate_client_token), InvoiceSummary (replacesInvoiceId), ExportOptions (mark_source, plus a snake_case ExportFilter matching the actual wire format), customer.service contracts, AdminUser timestamps widened to string|null, formatMoney currency widened to match its own (currency || 'CHF') guard, faceCropStyle dimensions widened to match its !photoWidth guard, DEFAULT_FLAGS faces, logo_position 'sidepanel', and the hand-rolled t() props replaced with i18next's TFunction in four files. Unused symbols were checked before deletion; UpdateInstructionsDialog's targetVersion prop was completed rather than deleted (declared and passed but never rendered -- now the fallback before the query resolves). Left unfixed, deliberately: GalleryStoryLayout's handleOpenFeedback (TS6133). It is the only caller of setSelectedPhotoForFeedback and is itself never called, so StoryFeedbackSheet can never open on the Story theme. Wiring it needs a new affordance on StoryPhotoCard (no sibling layout exposes one to copy) and deleting it would orphan the sheet -- a product decision, not a type fix. Note PhotoLightbox on the same layout already handles feedback, so the sheet may simply be superseded. Refs testplan REPORT.md #22 (Part 1.3.04).	2026-09-01 17:09:24 +02:00
Paul Nothaft	9143997f8e	style(backend): clear the eslint backlog to zero ... 929 problems (928 errors, 1 warning) -> 0, exit 0. Rule breakdown, which corrects the report's premise -- `indent` dominated, not `quotes`: indent 719, quotes 68, no-unused-vars 54, no-empty 36, no-useless-escape 22, no-case-declarations 17, no-inner-declarations 6, no-control-regex 5, no-useless-catch 1, no-console 1 (warn). --fix handled only indent + quotes (719+68 = exactly the "fixable" count). no-useless-escape was NOT auto-fixable in this eslint version, so the one genuinely risky class never went through the autofixer -- all 22 were done by hand. Two mechanical proofs on the autofix diff: a token-level AST diff (espree, before vs after) shows exactly 68 differing tokens, all quotes, with the 719 indent fixes producing zero token changes; and a cooked-value diff of every string/template/regex literal shows 0 differences. Regex escapes: eslint was correctly conservative and did not flag the load-bearing ones -- \- in [^a-zA-Z0-9_\-\.] (unescaping makes an invalid reversed _ -> . range) or in [!@#$%^&*()_+\-=...] (would become a + -> = range silently matching ",-."). Every removal was a \/ \[ or \. inside a character class; all 11 old/new pairs were brute-forced over 794 inputs with 0 mismatches. Manual fixes: no-empty were all deliberate best-effort catches around activity logging, annotated rather than restructured; no-case-declarations braced in two adminBackup switches; no-inner-declarations converted to const arrows after checking no call precedes the declaration and no this/arguments use; no-control-regex and no-console got targeted disables with stated reasons; one `catch (e) { throw e; }` wrapper removed. Two unused bindings were near-misses worth noting: secureStatic.js's `fullPath` is a path-traversal guard (safePathJoin throws on escape) and restoreService.js's `backupManifest` is the throw-on-corrupt-manifest gate before a rollback -- deleting either would have silently removed a check. Only the bindings were dropped; the calls stay. Two real bugs found and deliberately preserved with a comment plus a narrow disable rather than deleted, since deleting would erase the evidence: _workflowSeedBoot.js's `booted` is written but never read, so the intended once-per-process guard is missing its early return and workflows re-seed on every call; and quoteService.js's VALID_QUOTE_TRANSITIONS is a full state machine nothing consults, so quote status changes are unvalidated. Backend test suite: 253 suites / 2552 tests passing, 0 failures, before and after. Refs testplan REPORT.md #22 (Part 1.2.02).	2026-09-01 16:46:34 +02:00
Paul Nothaft	73b08a7b5c	fix(email): give gallery_created a real German translation ... translations.de for gallery_created was the English copy word for word, while nl/pt/ru/fr/es/sl are all localized. This is the mail sent on every gallery creation, so German-default installs have been silently mailing English. Root cause chain, fresh installs only: 001_init seeds the English template; 059 introduces the multilingual columns and fills subject_de/body_html_de/ body_text_de from their _en counterparts (its own comment: "Copy to German as default"); 075 then materialises exactly those columns as the `de` row. The real German only ever existed in migrations/legacy/026, and run-migrations.js runs core/ only for fresh installs -- so every install created since 059 has the English-as-German row. A code-only fix would have changed nothing: knex will not re-run 059/075, so existing installs would keep the bad row forever. Fixed as a content migration following the repo's precedent for template repairs (094, 172). Conservative about what it touches: the German row is rewritten only while it is still byte-identical to English (or empty) -- precisely the broken state -- so a legacy install whose German came from 026, or any admin-edited template, is left alone. Also repairs the legacy _de columns, which are still emailProcessor's fallback path. Idempotent, hasTable-guarded, no-op down() (reverting would restore English-as-German). Placeholder parity with the English original is exact and test-asserted: host_name, event_name, event_date, gallery_link, gallery_password, expiry_date. Two related gaps found but deliberately not fixed, both outside the reported bug: expiration_warning, gallery_expired and archive_complete are German-is- English on fresh installs through the identical 059 mechanism (legacy 026 fixed all four). And nl/pt/ru/fr/es/sl additionally wrap a {{#if welcome_message}} block that the English original lacks, even though welcome_message is passed at send time -- so EN and now DE drop the photographer's personal note. That is an English-side gap needing its own decision. Refs testplan REPORT.md #16 (Part 3, J.04).	2026-09-01 16:40:02 +02:00
Paul Nothaft	fc7cb226f4	fix(archives): run search, filter and sort server-side ... ArchivesPage fetched one 20-row page and then filtered and sorted only that array in memory, while "Showing X of 802" / "Page 1 of 41" kept reporting the full unfiltered count. Searching for an archive that exists but is not on the current page returned a false "0 results" with no hint the search was page-scoped. The backend did not support the params (it read only page/limit and hardcoded orderBy archived_at desc), so all three are new. Follows adminEvents/crud.js for the shape and customerAccountsService for the case-insensitive predicate: whereRaw with a bound parameter, never interpolated, and sortBy whitelisted to date/name/size before it reaches orderBy. The same applyFilters() closure runs against both the count query and the row query, so the total cannot drift from the rows again. Frontend mirrors EventsListPage: 300ms debounce, reset to page 1 on any query change, placeholderData so keystrokes don't flash the spinner. Two interpretation calls: - sortBy=size orders by summed photo bytes, not the zip's on-disk size. The Size column comes from a per-row fs.stat done after pagination and there is no archive_size column, so a global sort on the real zip size would stat all 802 files per request. Ordering is near-identical except for rows whose zip is missing. Adding events.archive_size would be a migration, out of scope. - No LIKE-metacharacter escaping. escapeLikePattern() does .replace(/'/g,"''"), which corrupts a bound value ("Sarah's Birthday"), and its backslash escaping is a no-op on SQLite without an ESCAPE clause. Matched customerAccountsService instead. A literal % typed by an admin acts as a wildcard in a read-only search; no injection risk. Pre-existing and untouched: the four stat cards still aggregate the current page only. Refs testplan REPORT.md #9 (Part 3, I.01).	2026-09-01 16:31:09 +02:00
Paul Nothaft	3f6c81a846	fix(photos): treat category_id 0 as uncategorized instead of storing it ... Genuine product bug, found behind the adminPhotos.reference suite (which was failing for an unrelated reason -- see below). parseInt('0') is 0 and !isNaN(0) is true, so a '0' category_id was written literally. photo_categories.id is an increments() column, so 0 can never be a real category, and every read path already assumes it cannot happen: the list mapper does `category_id || type` (0 is falsy, renders as uncategorized) and the list filter explicitly skips '0'. The result was a filter black hole -- the photo matches no numeric category filter, and misses the "uncategorized" filter too because that is whereNull(). Displayed as uncategorized, reachable by nothing. null rather than a 400: unparseable input ('abc' -> NaN) already falls through to null, so 400ing on '0' while silently accepting 'abc' would be incoherent, and '0' is just the HTML <select> shape where the "none" option carries value="0". Fixed at all three call sites that share the branch -- PATCH /photos/:photoId, POST /photos/bulk-update, and the upload route, where the dangling 0 was written at creation time and the scope-validation guard (`if (parsedCategoryId && ...)`) skipped on the falsy 0 and let it in unvalidated. Only the PATCH one was behind the failing test; leaving the other two would have left the bad state creatable. The suite's 3 failures were all masked by a fixture gap, not this bug: it stubs middleware/auth but not middleware/permissions, so requirePermission's admin_users JOIN roles query hit tables the fixture never creates and every request 500'd before reaching a handler. Stub it, bring the photos fixture up to the 7 migrations it had drifted behind, and correct a stale 200 that became 202 when uploads went async in 851744c3. Known adjacent gap, not fixed (wider than this bug): PATCH and bulk-update accept any positive category_id with no existence or scope check, unlike the upload route which validates event_id = X OR is_global per #500/#525 -- so a photo can be PATCHed into another event's category. Refs testplan REPORT.md #22 (Part 1.2.01).	2026-09-01 16:30:46 +02:00
Paul Nothaft	1d84c738d8	test: repair four stale backend suites ... All four asserted contracts the product has since moved past. No genuine product bugs behind any of them; assertions were tightened, not loosened. adminAuth (3 tests): never mounted errorHandler, so ConflictError/ ValidationError arrived as empty Express defaults. The route also checks username before email, so the "email conflict" fixture was hitting the username branch. Mount the handler, fix the fixture, match the real response shapes. backupService.enhanced (12 tests): three stacked drifts -- the db mock had no .returning(), so every runBackup threw at the insert; ensureDatabaseDumpForBackup now lazily requires ./databaseBackup inside the run, which fails under mock-fs; and the rsync path moved from exec(shell string) to spawnAsync('rsync', args) with an isHostAllowed SSRF preflight. Also updates getBackupStatus to its current shape (frontend aliases, nextScheduledRun null when no schedule is enabled, #871). adminSettings.logo: POST /logo gained requirePermission('settings.edit'); the hand-rolled db mock returns a bare Promise from select(), so the permission lookup threw a TypeError into a 500. Mock the permissions middleware alongside the already-mocked auth. crmMintPaths (2 tests): macOS-only. The expected prefix was realpath'd while the services persist under the raw STORAGE_PATH -- identical on Linux CI (/var vs /private/var only diverges on macOS), which is why it passed there. The comment justifying the realpath referenced process.cwd() behaviour the services no longer have. Refs testplan REPORT.md #22 (Part 1.2.01).	2026-09-01 16:29:03 +02:00
Paul Nothaft	c5c5a6b0c8	fix(webhooks): write delivery timestamps as ISO strings ... Applies the repo's documented Jest+SQLite guidance (CLAUDE.md) to the webhook delivery path, which was the last one still passing raw Date objects into knex writes. Under jest those store as the literal string "[object Object]", so next_retry_at came back NaN and the retry/backoff test could not assert on it. Production (PG, and SQLite outside jest) was unaffected. Convert the timestamp writes -- and the `next_retry_at <=` due comparison, which has to stay type-consistent with them -- to .toISOString(), matching the existing precedent in downloadJobService.js. Refs testplan REPORT.md #22 (Part 1.2.01).	2026-09-01 16:29:03 +02:00
Paul Nothaft	5fa04e647e	fix(categories): validate category name length instead of 500ing ... photo_categories.name is varchar(100). Neither the input nor the route checked length, so a 267-char name hit a raw Postgres "value too long", came back as a 500, and the form silently stayed open with no toast. Add isLength({ max: 100 }) to POST / and PUT /:id (the update route had the identical gap) so it returns the route family's normal 400 { errors: [...] } shape that the toast helper already renders, and maxLength={100} on the three category-name inputs (create + inline edit in CategoryManager, create in EventCategoryManager). Refs testplan REPORT.md #4 (Part 7.01).	2026-09-01 16:23:28 +02:00
Paul Nothaft	6f7aa59fad	fix(feedback): align word-filter severity vocabulary with the admin UI ... WordFilterManager.tsx sends low/moderate/high/block; the validator only accepted mild/moderate/severe, so 3 of the 4 UI levels 400'd with "Invalid severity level" -- including "block", the strongest advertised tier. Aligning isIn() alone would have made "block" accepted but semantically inert: feedbackModeration.js branches on 'severe'/'moderate', so "block" would fall through to the flag-only branch and behave as the weakest level. Map the UI vocabulary onto the existing outcomes instead, per the legend the UI itself renders: block -> reject, moderate/high -> needs approval, low -> flag only. 'severe' stays an accepted alias in the blocking predicate so any row written through the old validator (the field is optional, so a direct API caller could have stored one) keeps blocking. No data migration needed: the column is a bare varchar(20) default 'moderate' with no CHECK, no enum and no seed rows, and 'mild' already lands in the flag-only branch that 'low' now means. Refs testplan REPORT.md #2 (Part 3, J.11).	2026-09-01 16:23:22 +02:00
Paul Nothaft	e18ab0d842	fix(upload): enforce the configured per-file size limit on admin uploads ... getMaxFileSizeBytes() (general_max_file_size_mb, default 50MB) was only read by adminSettings.js to display the value. The admin upload routes streamed against a hardcoded ceiling instead, so the dropzone's "max. 50MB pro Datei" was never enforced: - adminPhotos.js POST /:eventId/upload -> 10GB hardcoded - adminPhotos.js POST /:eventId/chunked-upload/init -> 10GB hardcoded - v1/events.js POST /events/:id/photos -> 100MB hardcoded Resolve the cap per request (it is admin-configurable at runtime) and build the multer instance from it, mirroring what gallery.js and adminTransfers.js already do. The 400 names the configured limit and reuses gallery.js's exact error string so the frontend surfaces it identically. getMaxFileSizeBytes() clamps to MAX_ALLOWED_FILE_SIZE_MB, so the 10GB hard ceiling still bounds everything. gallery.js (guest upload) already enforced this correctly and is unchanged -- the report's claim that it did not is stale. Interpretation: general_max_file_size_mb is a single per-file cap with no photo/video split, and gallery.js already applies it blanket to guest video uploads, so admin video uploads now share it too. On a default install that means a 200MB video needs the setting raised first -- which is what the UI has been advertising all along. Refs testplan REPORT.md #1 (Part 7.06).	2026-09-01 16:23:04 +02:00
Paul Nothaft	884580d849	chore(main): release 3.122.2-beta.0 (#1258)	2026-09-01 07:30:45 +00:00
Paul Nothaft	7e6bfbecb2	chore(main): release 3.122.1-beta.0 (#1254)	2026-09-01 06:35:22 +00:00
Paul Nothaft	6938bad107	fix(events): apply the gallery password policy to publish and send-later (#1253) ... Both routes re-hash password_hash from a plaintext the admin re-types, and both validated it with nothing but express-validator's isLength({min:6}). So the configured complexity — moderate by default, meaning 8 chars plus upper, lower and a digit — governed creation and reset while these two doors accepted 'aaaaaa' and made it the live gallery password. Fixed for both at once, deliberately. Fixing only the newer send-later route would have made a quiet-publish password valid at publish time and rejected by send-later, leaving the admin unable to mail a gallery that is already live under exactly that password. Not an escalation — it needs admin auth plus events.edit, and such an admin could already set the same weak password through /publish. It is a policy gap: the UI promised a complexity level these two endpoints did not enforce. BEHAVIOUR CHANGE: an API-only consumer publishing with a sub-policy password now gets 400 with the same body shape event creation returns (error, details, score, feedback) instead of silently weakening the gallery. Two existing test fixtures had to change for the same reason — their intent was that the supplied password is carried and persisted, not that a weak one is accepted. Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-09-01 08:30:17 +02:00
Paul Nothaft	a35d2bad66	fix(archives): restore categories for original-filename archives on main too (#1252) ... main's #1240 landed the manifest lookup in its first form; the hardening that followed only ever reached stable, via #1243. So main still silently loses every category when restoring an archive written while general_use_original_filenames_for_downloads was on: archiveService names each ZIP entry after the ORIGINAL filename while the manifest stays keyed by the internal photos.filename, so the lookup misses every entry. Ported as one unit rather than piecemeal, since a third variant of this function helps nobody: - index by original_filename, and by sanitizeForZipEntry(original_filename) as the ZIP would actually have written it - two passes, canonical names claimed before any alias, so the result no longer depends on manifest iteration order (the archive query has no ORDER BY) - a name two rows both claim is dropped rather than guessed — including the canonical/alias clash, where which file the ZIP emitted depends on a naming mode the manifest does not record - globals count as existing, event-scoped rows win over them, and the global arm requires event_id IS NULL so one event's legacy row can't be adopted by another event's restore - an invented category is explicitly is_global false; the column defaults to TRUE, so a restore was leaking this event's naming into every gallery - categories resolve inside the !existingPhoto branch, so a restore that skips its inserts stops creating unused rows from stale manifest names - a duplicate category name is logged and resolved by lowest id instead of engine order main-only code is untouched: the face-data cleanup (#1074, #1132) and the uploaded_at toISOString fix both survive — stable still has the bare new Date() there, which is the documented Jest/SQLite landmine and worth a separate look. 15 tests, ported from #1243. Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-09-01 08:30:02 +02:00
Paul Nothaft	63f3fb4629	chore(main): release 3.122.0-beta.0 (#1251)	2026-09-01 06:27:11 +00:00
Paul Nothaft	1ef2b3c85b	feat(events): publish without notifying, and send the gallery email later (#1235) (#1241) ... * feat(events): publish without notifying, and send the gallery email later (#1235) Publishing queued the gallery_created email whenever any customer email existed, with no opt-out. A photographer working with a client who has no address yet — the Instagram-team case in discussion #1086 — had to type their OWN address into the required field, publish, receive the client-facing email themselves, and hand the link over by DM. Turning off `event_require_customer_email` is not the answer either: that is global, and the same photographer usually does collect addresses. Two halves, because a checkbox alone is only half a workflow: - `notify_customer` on publish, default TRUE. Absent means notify, so the v1 API, an older frontend and any script keep behaving exactly as before. When false the gallery goes live and nothing is queued — not the gallery_created email, not the assigned-customer-account notice, not WhatsApp. Publishing still logs activity and still fires the event.published webhook, because those describe a state change rather than a message to a customer. - POST /:id/send-gallery-email for an already-published gallery. Deliberately not restricted to galleries published quietly: re-sending is a normal thing to want (spam folder, wrong address since corrected) and refusing would push people to unpublish and republish, changing gallery state to work around a mail problem. Refused for a draft, whose link would not work yet, and for an event with no recipient. The email composition is now one helper shared by both, so an email sent a week later is identical to one sent at publish. UI: a checkbox in the publish dialog (checked by default, hidden when nobody would be notified anyway), and a "Send gallery email" action on published galleries that have a recipient. The password field follows the checkbox — unchecking it means nothing is being sent, so there is no plaintext to carry and no reason to demand it. EN + DE strings. 7 integration tests. Two fail without the change, verified by forcing notifyCustomer true and re-running; the rest pin the default, the draft and no-recipient refusals, and that a gallery with no recipient still publishes. * fix(events): make the publish dialog description follow the checkbox (#1235) Caught by screenshotting it. With "Send the gallery email now" unchecked, the paragraph above still read "...and sends the notification email to tina@example.com" while the control directly beneath it said nothing would be sent — the dialog contradicted itself at exactly the moment the admin is deciding whether anything goes out. It now reads "No email will be sent — you can send it later from this page." when the box is clear. EN + DE. * fix(events): close six gaps in publish-quietly found by external review (#1235) TWO CORRECTIONS TO MY OWN VERIFICATION FIRST. `npx tsc --noEmit` in frontend/ is a NO-OP — the root tsconfig is solution-style with references and no include, so it checks nothing. Every "tsc clean" I claimed on this branch came from that. The real check, `tsc -p tsconfig.app.json`, showed two TS2339s I had introduced: `event.host_email` does not exist on the frontend Event type, which the admin API normalises away. Both recipient checks now use `customer_email`. PASSWORD ON SEND-LATER. The action promised to send the link and password but always called the endpoint without one, so a protected gallery got the "(set at creation)" sentinel — unusable — and this is most needed right after a quiet publish, the path that never collects a password. New SendGalleryEmailDialog asks for it, same shape and reasoning as the publish dialog (#627). Galleries with no password skip the field. WHATSAPP-ONLY GALLERIES COULD NOT PUBLISH QUIETLY. willNotify ignored customer_phone, so a phone-only gallery hid the opt-out AND told the admin nothing would be sent — while publish queued the WhatsApp anyway. Phone now counts, with its own description line. ASSIGNED-ACCOUNT NOTICES COULD NOT BE SENT LATER. The dialog promised it; the endpoint rejected anything without an inline recipient. It now falls through to the same customer-account path publish uses. EDITORS COULD NOT SEE THE ACTION. The send button was nested inside the events.archive gate, so the default editor role — events.edit, no archive — never saw a button for an endpoint it is allowed to call. Separate gates now. DEAD LINKS. The endpoint only checked is_draft, so an archived, inactive or expired gallery would send a link the gallery middleware rejects. All three are refused with a reason. 9 backend tests (2 new), 22 across the event suites. eslint clean on every changed frontend file; crud.js keeps its 2 pre-existing errors. * fix(events): persist the send-later password, and fix a long-standing isGalleryPublic misuse (#1235) Round 2 of external review. THE EMAIL COULD CARRY A PASSWORD THE GALLERY REJECTS. The send-later dialog invites "or pick a new one", but the route queued that plaintext without touching password_hash — so the customer got credentials that do not open the gallery. Worse than the sentinel it replaced, because it looks usable. The route now hashes and persists first, exactly as publish does. isGalleryPublic TAKES A VALUE, NOT AN EVENT — and this is pre-existing. normalizeRequirePassword returns its default for anything that is not a boolean/number/string, so isGalleryPublic(event) is ALWAYS false and `requirePassword` was always true. The publish dialog on main has demanded a password for public galleries for exactly this reason. Both call sites now pass event.require_password. Fixing the older one alongside mine rather than leaving a broken copy one line above a fixed one. ASSIGNED-ACCOUNT GALLERIES HAD NO BUTTON. The route falls through to the customer-account notice when there is no inline email, and the publish dialog promises that notice can be sent later — but the button only appeared with a customer_email, making the promise unkeepable. WHATSAPP CLAIM SOFTENED. Publish only queues WhatsApp when the config exists and is enabled, which the dialog cannot see. It now says the customer is notified there "if WhatsApp is configured" rather than asserting a send. 10 backend tests (1 new, covering the rehash). eslint clean on every changed frontend file; crud.js keeps its 2 pre-existing errors. * fix(events): don't reset the password for an account-only notice, hide unusable actions (#1235) Round 3 of external review. The first is a harm my own round-2 fix introduced. PASSWORD RESET FOR NOTHING. Round 2 persisted the supplied password before knowing which mail would go out. For a protected gallery with no inline email but assigned accounts, the dialog still demands a password, the hash was rewritten, and then the fallback sent customer_gallery_assigned — which links to the customer portal and never mentions a password. Net effect: the live gallery password silently changed and everyone holding the old one was locked out, in exchange for nothing. It is now persisted only when the mail that carries it is actually being sent. BUTTONS THE BACKEND WOULD REFUSE. The send action rendered for expired and inactive galleries, and counted assigned accounts the endpoint filters out as inactive — walking the admin through a dialog to reach a generic error toast. The card now mirrors the endpoint's eligibility rules, and only active accounts count toward having a recipient. 11 backend tests (1 new, pinning that the hash is untouched on the account path), 24 across the event suites. tsc and eslint clean on the changed files. * fix(events): make the send-later action agree with what the endpoint will do Three findings from an external review round, all the same shape: the UI predicted the endpoint's behaviour and got it wrong. GET /admin/events/:id mapped customer_accounts without is_active, so the "only ACTIVE accounts count" filter in OverviewTab compared undefined and excluded nothing. A gallery whose only assignments were deactivated showed the send action, and the endpoint then filtered every recipient and returned 400. is_active is exposed now, and the count applies the same predicate the fallback uses — active AND holding an address. is_active is coerced through toBoolean rather than compared with === false. On the default SQLite backend it comes back as 0, and 0 === false is false, so an inactive gallery kept offering a send that parseBooleanInput then rejected. Same class as #1028. The password prompt is gated on there being an inline recipient. With no customer_email the backend takes the account fallback, which sends customer_gallery_assigned — a portal link that never mentions a password — and deliberately skips the rehash. Asking for one there blocked the send behind a six-character value nothing consumes, and the dialog's promise that it would be rehashed was false. Frontend suite: 291 passed. tsc and eslint clean. * fix(events): don't mail a portal link to a customer who cannot sign in Round-2 finding from the external review. A passive customer — created directly and never invited — is an active account with a real address whose password_hash IS NULL. The account fallback happily mailed it customer_gallery_assigned, which links to /customer/dashboard, and customerAuth rejects login without a hash: the link goes to a door that will not open. Worse than failing, the route counted it and reported success, so the admin believed the customer had been told. getAssignmentsForEvent now derives can_sign_in (the predicate, never the hash) and the three call sites share one canReceiveGalleryNotice helper — publish, send-later, and the payload the UI predicts from all have to agree or the button appears and then 400s. The UI mirrors it. Sending passive customers an invitation instead of skipping them is the better product answer, and a separate feature. Refusing visibly beats a silent non-delivery in the meantime. Test asserts the refusal; it fails without the can_sign_in arm. --------- Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-09-01 08:18:00 +02:00
peipeimo	bb2f709fdd	fix: single-photo gallery downloads 404 on S3 storage backends (#1048) ... * fix(gallery): route single-photo downloads through the storage backend The route resolved a local filesystem path unconditionally and handed it to res.sendFile. On an S3/R2 deployment managed photos are never on local disk, so every per-photo download failed — while download-all and secure-images worked, because they already went through getStorage(). That asymmetry is why it went unnoticed: the gallery looks healthy until a guest clicks the download button on one photo. Measured rather than assumed: because sendFile is called WITH a callback, Express does not send a response when the file is missing and the callback only logs. The request does not 404, it hangs until the client gives up. The new tests pin this — all five backend-path cases time out against the previous implementation. Two existing pieces do the work, so this mostly deletes code: - renderPhotoForDownload (#858) already owns resize-then-watermark ordering and the storage fetch, and the zip builders in this same file already use it. The inline duplicate of that logic goes. - the pass-through case branches on storage.kind(). Local disk keeps res.sendFile: it emits Content-Length, Accept-Ranges, ETag and Last-Modified and answers Range with a 206, and sharing one bare stream.pipe(res) with S3 would silently drop all of it — a resumed download would append a second full body onto the partial file. On S3 the parts that matter for a download are reproduced via stat() and getRange(). Ranges are parsed defensively; an unchecked parse yields NaN bounds and a 206 with a nonsense Content-Range, which corrupts a resumed download rather than failing it. Malformed or unsatisfiable ranges fall back to a 200. The pre-stream 404s now run before any image header is staged, so the error goes out as JSON instead of a .jpg attachment containing JSON. Co-authored-by: peipeimo <peipeimo@users.noreply.github.com> * fix(gallery): open the stream before staging download headers, honour If-Range Both from an external review round on this PR. stat() succeeding does not mean get() will — a concurrent delete or replace, or a transient backend error, lands between them. The fetch was awaited AFTER the headers went out, so: - the range branch had already called writeHead(206), leaving the outer catch nothing to do but throw ERR_HTTP_HEADERS_SENT. In practice the request hangs: the new regression test sat for the full 120s jest timeout against the previous code instead of returning. - the full branch would have sent its 500 JSON underneath the staged image/jpeg attachment headers — a .jpg file full of JSON, which is the exact failure this PR set out to stop doing on the 404 paths. Opening the stream first also lets a vanished object answer 404 and a transient failure answer 500, instead of both surfacing as a broken body. If-Range: emitting Last-Modified without honouring the validator built from it is the dangerous half of the feature. A client resuming after the object was replaced — the watcher re-importing a swapped file, an admin re-upload — would get 206 from the NEW bytes and splice two versions into one corrupt file. A validator that does not match now falls back to a full 200. 4 new tests; 3 of them fail against the previous commit, the fourth is the matching-validator control that must keep returning 206. * fix(gallery): HEAD without egress, classify render failures, stage 206 headers Round-2 findings from the external reviewer. Express routes HEAD through this GET handler and Node discards the body, but the pipe still drains the whole object out of S3 first — a metadata probe from a download manager cost a full transfer in egress and latency. Everything a HEAD needs is already in stat(). renderPhotoForDownload rejections were all reported as 404. It can equally fail because getToFile timed out, tmp filled up, or sharp died; calling that "photo not found" misleads the guest and hides the incident from us. Now classified the same way the pass-through branch already does. The 206 path uses status()+set() instead of writeHead(). writeHead commits the response immediately, so a stream that resolved and then errored before its first chunk left pipeStreamToResponse able only to destroy the connection. Staged headers flush on the first body write, so an error at byte zero now returns a clean retryable status with keep-alive intact. Credit to the reviewer for the correction — I had assumed deferring the commit required buffering. Writing the test for that surfaced one more: pipeStreamToResponse cleared Content-Type, Content-Length, ETag and Content-Disposition but not the range headers, so the 500 went out still advertising Content-Range: bytes 0-9/40 — telling a resuming client the error body IS the partial content. Not taken: binding response metadata to a fetched object version. That needs an ETag/versionId on the storage abstraction and conditional GETs in both adapters; the reviewer agreed it belongs in its own PR rather than blocking this one. Backend suites: 485 passed. * fix(gallery): answer HEAD before the counters and the render Round-3 finding. The HEAD short-circuit was inside the storage branch, which sits below both the download_count increment / access_logs insert and renderPhotoForDownload — so a download manager's metadata probe was recorded as a real download, and on a watermarked or resized gallery it also pulled the original from S3 and ran sharp over it to build a body Node then throws away. HEAD now leaves the handler right after the access checks, with no side effects and no bytes read. Content-Length is included only when the photo ships untransformed and the size is readable from stat(); a watermark or resize changes the length and the only way to learn the new one is to do the work this branch exists to avoid. HEAD may omit it. Not taken, again: binding the read to the statted object version. The reviewer already agreed in a follow-up that it needs an ETag/versionId on the storage abstraction plus conditional GETs in both adapters, and belongs in its own PR. Re-raising it does not change that. Tests assert the probe moves neither download_count nor access_logs. --------- Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local> Co-authored-by: peipeimo <peipeimo@users.noreply.github.com>	2026-09-01 08:17:32 +02:00
peipeimo	202c553a08	fix(events): delete stored objects when cascading an event delete (#1051) ... * fix(events): delete stored objects when cascading an event delete * fix(events): sweep watermarks and the archive zip on cascade delete too Two more objects in the same class as the originals: both are written through the storage backend, both were only ever removed with fs.unlink, so both outlive the event on S3. - photo.watermark_path — a canonical key, deleted via getStorage() on the single-photo path (watermarkService.deleteWatermarkFile) and on archive (archiveService.js:227). The cascade neither selected nor removed it. - event.archive_path — written by storage.putFromFile (archiveService.js:160) and typically the largest single object an event owns. event.hero_logo_path is deliberately NOT included: multer writes logos to local disk with diskStorage regardless of backend (adminEvents/logo.js:19-28), so they are never bucket objects and the existing fs.unlink is correct. Collect into a Set — an unresized gallery can carry one object in both hero_path and preview_path, and the second delete would log a spurious failure. * fix(events): sweep the download caches, and delete objects concurrently Both from an external review round on this PR. The download caches are the subtle case: the pre-built "Download All" zip (events.download_zip_path) and one zip per custom-resolution download job (download_jobs.zip_path) both live under events/active/{slug}/.download-cache/. On local disk the recursive fs.rm already covered them, which is exactly why they were easy to miss — on S3 that prefix is not a directory, nothing covered them, and both are gallery-sized. downloadZipService exposes a cleanup() documented as "used on event deletion" that the cascade never called. The job rows are read before the transaction for the same reason the photo rows are: download_jobs.event_id is ON DELETE CASCADE, so on Postgres they vanish with the event and take their keys with them. Guarded with hasTable so a pre-#173 install doesn't abort the delete. Deletes now run through a bounded pool instead of one await per key. A 400-photo gallery owns ~1600 objects once derived tiers are counted, and that many sequential DeleteObject round trips runs to minutes — long enough for a proxy to time the request out AFTER the commit, leaving the event deleted and the sweep half-finished. A pool rather than Promise.all over every key, so the fan-out can't exhaust the S3 client's connection pool. * fix(events): never delete a derivative another gallery still uses Round-2 findings from the external reviewer. Canonical thumbnail/hero/preview keys are not event-scoped: the basename is the photo's filename (imageProcessor passes no outputBasename for managed photos, so the key is thumbnails/thumb_w300_<filename>), and filenames are not unique across events — the responsive-tier code says so in as many words, which is why THOSE keys carry a p{id}_ prefix. A legacy gallery can therefore share a canonical derivative with a photo in another event, and deleting it here blanked a surviving gallery's tile. Derived keys are now checked against photos outside this event and anything still referenced is left alone; if the check itself fails, every derivative is kept. An orphan costs storage, a deleted derivative costs someone else's gallery. Originals need no check — their keys embed the slug. Also cancel any in-flight or debounced Download All build before snapshotting paths. A builder that started before the delete would otherwise upload a gallery-sized zip after the sweep and write its path onto a row that no longer exists, orphaning it permanently. downloadZipService.cleanup() is the service's own entry point for this and does all three things: bumps the version so an in-flight build discards its result, clears the debounce so nothing rebuilds for a deleted event, and removes the current object. * revert(events): drop the Download All build cancellation Reverted for the same reason as on the stable twin, where it was caught: downloadZipService.cleanup() reaches getStorage() through _cleanup(), so where the S3 backend is configured but unreachable every cascade delete pays the adapter's retry backoff. On stable that took the backend CI job from ~2 minutes to past its 10-minute budget, twice, reproducibly. This branch's suite happened not to trip it, but the same cost lands in the request path of a real delete — and the twins have to carry the same code. The race it addressed is narrow and costs one orphaned zip; documented as a follow-up instead. The shared-derivative guard from the same review round stays — that one prevented deleting a surviving gallery's thumbnail. --------- Co-authored-by: Peifu Mo <peipeimo@Peifus-MacBook-Pro.local> Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>	2026-09-01 08:17:04 +02:00
Paul Nothaft	bdeb5a2151	chore(main): release 3.121.4-beta.0 (#1249)	2026-09-01 06:10:24 +00:00
peipeimo	4f352dec39	fix(auth): treat zxcvbn suggestions as advice, not blocking errors (#1050) ... validatePassword() appended zxcvbn's feedback.suggestions to the errors array unconditionally, and validity is errors.length === 0 — so any password that merely earned a suggestion was rejected even when it satisfied every configured rule. The effective policy was stricter than the configured complexity level and invisible to the admin. Suggestions now surface only alongside a real strength failure. They stay available to callers in result.feedback.suggestions, so a UI can still show them as guidance while typing. The weak-password fixture is assembled from parts rather than inlined: an 8-char alphanumeric literal next to validatePassword( reads as a hardcoded credential to the required GitGuardian check. Both fixtures pin their zxcvbn score — the compliant one is load-bearing at exactly the moderate minimum (2), and a future zxcvbn bump promoting it to 3 would leave the test green while no longer covering the bug. Co-authored-by: Peifu Mo <peipeimo@users.noreply.github.com>	2026-09-01 08:05:42 +02:00