1366 Commits

Include renames

Author	SHA1	Message	Date
Paul Nothaft	3e46530072	fix(security): contain logo, favicon and PDF-logo unlinks to their upload directories ... Settings > Branding persisted logo_url / favicon_url verbatim and on clear unlinked path.join(storage, url) behind a startsWith('/uploads/logos/') check, which '..' segments pass. The business-profile PDF logo did the same behind a /pdf-logo-\d+\./ marker test, and used absolute values as given. Either let a settings.edit or settings.banking holder delete any file the process can reach. Both now resolve through helpers in utils/safePath that only ever name a flat leaf inside the fixed directory. The /favicon.ico streamer is narrowed the same way: it contained to the whole uploads/ root, which also holds signed contracts and transfer files.	2026-09-03 10:44:55 +02:00
Paul Nothaft	0ca0e4a922	fix(security): verify the signature before writing a token to the revocation list ... revokeToken() base64-decoded the payload without checking the signature and inserted a row keyed on id-iat-type, the same key isTokenRevoked() matches for real sessions. The logout endpoints are unauthenticated, so anyone could forge a payload naming another user's id, type and login second and log them out remotely; a far-future exp also left rows that cleanup never swept. Expiry is still ignored so logging out an expired session stays idempotent.	2026-09-03 10:44:54 +02:00
Paul Nothaft	903e471753	fix(security): stop reflecting submitted passwords in validation errors ... Codex review round 2. The 400 I added in the previous commit returned errors.array() verbatim, and express-validator puts the submitted `value` in each error -- so rejecting an oversized password echoed that password back, and re-allocated up to the 50mb body limit on an unauthenticated endpoint, partly undoing the denial-of-service fix this branch exists for. The same call appeared at seven sites in this file, five of which validate a password field: /admin/login, /gallery/verify, /gallery/:slug/client-login, /admin/change-password and /password-strength. Every failed login was returning the attempted password in its response body, where it reaches proxy logs, error monitoring and browser tooling. Fixed at all seven rather than only the one the review pointed at. Only `value` is dropped. `msg`, `path` and the rest are kept, because the two shapes express-validator produces are both consumed in the frontend -- AcceptInvite reads {field, message} from routeHelpers.validateRequest, EventDetails reads {msg, path} from raw errors.array() -- and switching auth.js to the helper's shape would have broken the latter for a reason unrelated to security. 1 more test. Backend suite: 2744 passed.	2026-09-03 09:58:58 +02:00
Paul Nothaft	054cd6f82f	fix(security): enforce the strength-endpoint validators, and stop the generator spinning ... Codex review round 1 on the batch-1 security fixes. One finding is a regression this branch introduced. generateSecurePassword retried by recursing on any candidate validatePassword rejected. The new 128-character cap makes EVERY candidate invalid once a caller asks for more than that, so `generateSecurePassword({ length: 129 })` went from returning a password to unbounded recursion and a stack overflow. It now refuses an impossible length up front, and the retry is a bounded loop rather than recursion -- every candidate failing is possible for reasons other than bad luck (a charset that cannot satisfy the configured policy), and that case deserves an error someone can act on rather than a blown stack. No caller in the repo passes a length at all; the hazard was in the exported surface. The route validators were decorative. POST /api/auth/password-strength never called validationResult(), so the length bound I added only recorded an error that nothing read: the oversized body still reached zxcvbn and the endpoint still answered 200. The cap inside validatePassword() was doing all the work. Errors are now returned as a 400 before the validator runs, which is what the previous commit claimed. Also awaited validatePasswordInContext, which is async. Unawaited, `validation` was a Promise and every field in the response -- valid, score, errors, feedback -- came back undefined. Pre-existing, in the lines this change already touches, and it made the endpoint useless for the real-time validation it exists for. 1 more test. Backend suite: 2742 passed. The 23 eslint errors in server.js are pre-existing and identical on main.	2026-09-03 09:39:50 +02:00
Paul Nothaft	14cd5eacb3	fix(security): bound password input before zxcvbn, and drop the legacy media mounts ... Two findings from the GHSA-pwx6-5pqc-c5xq scan bundle, both verified against the code and reproduced before fixing. **Unauthenticated denial of service via password strength (csf_495d53fa).** POST /api/auth/password-strength takes `body('password').notEmpty()` with no upper bound, sits behind express.json({ limit: '50mb' }), and hands the string to zxcvbn, whose matching is superlinear and runs synchronously on the event loop. Measured on this codebase, in ms of blocked loop: 128 -> 41, 512 -> 1367, 1000 -> 5097, 5000 -> did not return in two minutes. One unauthenticated request of about a kilobyte stops the whole process for five seconds; a few kilobytes stops it indefinitely. The /api/auth rate limit does not help when a single request is already enough. The cap lives in validatePassword() so it covers every caller, present and future; the route validator is defence in depth. 128 keeps the worst case at the cost of an ordinary request while staying far above any real password -- bcrypt consumes only the first 72 bytes, so length past that adds no entropy to the stored hash anyway. This is the only unauthenticated reach into zxcvbn: setup is token-gated and self-closing, and acceptInvite/adminAuth use the regex-only validator in passwordGenerator. **The /photos and /thumbnails static mounts (csf_9aa6afe6, csf_559cd5cc, csf_b14d462e, csf_547d26fa, and the gallery half of csf_34e420af).** They served the raw originals and thumbnail trees behind photoAuth, which authorises on a slug match. A static file server cannot apply per-photo rules, so everything the gallery API decides was absent: allow_downloads, per-category allow_downloads, watermarking, the resolution cap, reveal-mode windows, visibility='hidden', download logging, and the customer-assignment re-check that makes revocation immediate. photoAuth also bcrypt-compares an x-gallery-password header per request with no limiter -- both rate-limit gates return early for non-/api paths -- so the mount was an unmetered password oracle. The filenames needed to drive all of this are handed to every guest in the photos listing. Nothing builds these URLs: no reference in frontend/src, none in the email templates, and the only backend mentions are the /api/admin/photos/... API routes and a maintenance-mode prefix list. The equivalent authorised routes are /api/gallery/:slug/photo/:id and /thumbnail/:id. nginx still proxies the two locations; they now 404, which is the intent. **The /uploads mount (csf_1fc92f57).** It exposed the whole uploads/ root with no auth middleware at all, and that root also holds signed contract PDFs (uploads/contracts/signed) and client transfer files (uploads/transfers/<id>), reachable by anyone who learned or guessed a filename. Narrowed to the two public asset trees it exists for; contracts and transfers keep their own authorised routes. Removing the mounts leaves src/middleware/photoAuth.js unreferenced by application code. Left in place deliberately -- deleting it and its tests is a separate cleanup, and a smaller diff backports more safely. Backend suite: 2742 passed.	2026-09-03 09:25:53 +02:00
Paul Nothaft	dc5bccba05	chore(main): release 3.122.6-beta.0 (#1279)	2026-09-03 06:50:57 +00:00
Paul Nothaft	30fa320dc2	chore(main): release 3.122.5-beta.0 (#1276)	2026-09-02 14:53:06 +00:00
Paul Nothaft	b9c29fcf9b	Merge pull request #1274 from PicPeak/fix/1261-crm-invitation-visibility ... fix(crm): tell the admin whether a customer's invitation actually went out (#1261)	2026-09-02 16:47:05 +02:00
Paul Nothaft	ec1df704b4	Merge pull request #1273 from PicPeak/fix/1262-email-queue-visibility ... fix(email): show a queue nobody is working instead of reporting all-clear (#1262)	2026-09-02 16:46:28 +02:00
Paul Nothaft	2d403f7fb2	fix(email): wire the settings status card, and cap-aware truncation ... Codex review round 4 on #1273. Settings → Status rendered a green check for the email processor unconditionally, against an API field that was itself the literal 'active'. Both ends were lying and only one of them got fixed: adminSystem started reporting the real state in an earlier commit, but StatusTab never read it, so the second place an admin looks to find out why mail is not arriving still said everything was fine. It now shows stopped and degraded, with the reason. The truncation flag missed the case it most needed to cover. The loop broke on the 200-row report cap before the flag could be set, so 201+ overdue rows came back as exactly 200 with scanTruncated false -- a partial report presented as complete. It is now set whenever rows were left unexamined. The grace-window comment claimed the processor clears ~6000 rows inside the window. It clears on the order of 100: ten rows a pass, one pass a minute. The comment now says so, and says why the processor's own state is reported above the list rather than inferred from it -- "running, last pass sent 10" next to a backlog reads very differently from "not running" next to the same backlog. One round-4 finding is NOT fixed, deliberately, and is written up at the retry route. Clearing scheduled_at leaves created_at at the original enqueue time, so a retried old row appears in the waiting list immediately, looking overdue, until the processor sends it. Restarting that clock needs a timestamp written there and no shape works: a Date matches how queueEmail writes the column and how processEmailQueue compares it, but jest's sandbox Dates store as "[object Object]" (CLAUDE.md) so it cannot be tested; an ISO string tests fine but stores as TEXT, which SQLite then orders above the numeric bound in the processor's own pickup query, leaving the row unsendable. A requeued_at column would settle it. Cosmetic either way, and not worth risking a stuck row. 1 more test, failing before this commit.	2026-09-02 15:32:35 +02:00
Paul Nothaft	4deac229ac	fix(email): make waiting rows read-only, and time the grace from when due ... Codex review round 3 on #1273. The first finding reverses a round-1 fix of mine, correctly. Retry no longer sends. Round 1 flagged that retry was a no-op for waiting rows and offered two remedies: give them a send-now action, or stop showing them Retry. I took the first, and round 3 showed why it is the wrong half -- processEmailQueue claims nothing before invoking the transport, so a flush overlapping the scheduled pass has both of them sending the same email. Saving 60 seconds is not worth a duplicate landing in a customer's inbox, and a claim protocol would need a status no query watches plus a reaper for rows abandoned mid-send. So retry is a reset again, as it was on main. Waiting rows now carry no actions at all, which is the other half of that round-1 remedy and closes a worse hole the shared table opened: Dismiss DELETEs the queue row. Those emails have not failed and still go out once the processor recovers, so clicking the tidy-up icon on a health warning silently cancelled a customer's mail. The section is diagnostic; what a waiting row needs is the processor fixed, which the panel above it now says. The grace window runs from when a row became DUE, not from when it was queued. A split-payment invoice created three days ago and scheduled until a minute ago has had one minute of the processor's attention, and measuring from created_at reported every scheduled mail as unworked the instant it came due -- which is most of what this panel would then have been showing. A truncated scan can no longer read as an all-clear. The scan is bounded, so a queue larger than the budget whose head is all future-scheduled can hide a due row past the last page read; the response now says so and the UI withholds the green check. The test fixtures were wrong in a way worth keeping: scheduled_at also defaults to CURRENT_TIMESTAMP, so back-dating created_at alone built rows that cannot exist in production -- old, but scheduled for the moment the fixture ran. The helper now back-dates both, as the database would have. 3 more tests; the two that pin new behaviour fail before this commit, and the reverted flush is pinned by asserting the transport is NOT invoked.	2026-09-02 15:15:07 +02:00
Paul Nothaft	98aa06aeff	fix(email): read naive SQLite timestamps as UTC, and page the candidates ... Codex review round 2 on #1273. Both findings restore the false all-clear that round 1 set out to remove, by different routes. Both timestamp columns default to CURRENT_TIMESTAMP, which SQLite renders as a zone-less 'YYYY-MM-DD HH:MM:SS' in UTC -- and Date.parse reads that shape as LOCAL time. On a TZ=America/New_York deployment a row due now looked four hours away and never reached the waiting list; nine hours the other way, fresh mail read as long overdue. The parser now stamps the zone the value actually carries. That parser moved to utils/queueTimestamps so it can be tested honestly. This suite runs in UTC, where reading a zone-less value as local and as UTC give the same answer, and process.env.TZ does not reliably re-bind mid-process -- my first attempt at these tests passed against the broken code for exactly that reason. They now force TZ in a child process, so they fail on any host. The candidate rows are paged rather than cut off with one LIMIT. The time filter runs in JS, so a queue holding more than a page of future-scheduled rows -- split-payment invoices are exactly that shape -- filled the window with rows that all got filtered out and hid the due row behind them, reporting nothing waiting. Paging also drops the dependency on ORDER BY created_at meaning anything, which it does not on SQLite once numeric and text timestamps mix. Bounded at 10k scanned; past that the response is a sample, which the 200-row cap already made it. 12 more tests. The paging one fails before this commit, and all four naive-timestamp ones fail against the old parsing on any host.	2026-09-02 14:57:00 +02:00
Paul Nothaft	bc90b4db62	fix(crm): label the two invitation conflicts and stop guessing after a 5xx ... Codex review round 2 on #1274. Both findings are the same shape as round 1: a message that asserts more than the response supports, and sends the admin somewhere that makes it worse. A 5xx is no longer treated as a clean failure. createInvitation inserts the customer_invitations row and only then queues the email, with no transaction around the pair, so a 500 out of the queueing step leaves an OPEN invitation behind. Telling the admin "no invitation went out, retry" there walks them into a 409 that still queues nothing. 5xx now joins the no-response case as unconfirmed; a plain 4xx keeps the clean-failure message, because that is the one shape where nothing was written. The two already-active conflicts now say so. The send-invite route returns CUSTOMER_ALREADY_ACTIVE from its own check, but createInvitation rechecks customer_accounts afterwards and threw a bare ConflictError -- code CONFLICT, indistinguishable from the pending-invitation conflict. An invitation accepted between the two checks therefore landed in the pending branch, telling the admin to cancel an invitation that acceptance had just closed. Both conflicts in the service now carry a code of their own, so the client reads the code rather than inferring from the status. 4 more tests. One round-1 test changed with the behaviour it pinned: its 500 now asserts the unconfirmed message, and a new 400 case covers the clean failure it used to stand for.	2026-09-02 14:52:03 +02:00
Paul Nothaft	89db469f06	fix(email): compare queue timestamps in JS, and make retry actually send ... Codex review round 1 on #1273. One of the four is a real bug on every SQLite deployment. The waiting-row query compared `created_at` against a bound ISO string. On SQLite that column does not hold a string: queueEmail writes a JS Date and the native binding stores epoch ms, and SQLite orders INTEGER before TEXT regardless of value -- so the comparison was true for EVERY row. Mail queued a second ago read as ten minutes overdue, and a scheduled_at years in the future read as already due. Confirmed directly against sqlite3: a 2026 row matches `created_at <= '2020-01-01T00:00:00.000Z'`. Binding a Date instead is not the fix, since knex hands sqlite3 a Date the same way and jest's sandbox Dates stringify to "[object Object]" (CLAUDE.md). So the engine-safe half of the predicate stays in SQL and the two time comparisons move into JS behind a toMillis() that accepts all three shapes this column really has -- Date from Postgres, ms-number from SQLite, ISO string from fixtures and older rows. The scan is capped at 1000 pending rows ordered oldest-first; everything overdue sorts into that window, and the response was already capped at 200. The existing tests missed this because they store ISO strings, which is what CLAUDE.md prescribes for jest -- so the new ones store epoch ms, the production shape, and one mixes both in a single queue. Retry was a no-op for the rows it most needed to help. It wrote pending / retry_count 0 / no schedule, which is exactly what a waiting row already is: the row came back unchanged while the toast said it had been re-queued. And since the usual reason a row is waiting is that nothing is working the queue, deferring it to the next pass is the one answer that cannot help. It now follows the reset with the same single-row flush the project cockpit uses. An idle pass no longer inherits the previous pass's totals -- the no-pending early return skipped the lastResult assignment, so System Health kept attributing an old sent/failed count to a run that did nothing. "All clear" now means the whole queue is clear, which is what the PR claimed and the code did not do. An empty waiting list is only reassuring when something is working the queue: a processor stopped a minute ago has no overdue rows yet either, and a green check there is the same false all-clear this branch exists to remove. 7 more tests. The 5 that pin new behaviour fail before this commit; the SQLite ones fail in the way the bug predicts rather than erroring. Both new tests stub the webhook transport with a spy rather than pointing it at a dead port: real connection attempts left open handles that destabilised unrelated suites in the same jest worker.	2026-09-02 14:35:54 +02:00
Paul Nothaft	73d867521a	fix(email): show a queue nobody is working instead of reporting all-clear ... Closes #1262. "Gallery email queued" reads as a delivery confirmation, and System Health agreed with it: "No stuck or failed emails -- all clear", while not one email had gone out. Both statements were true and neither was the one the admin needed. Queueing writes an email_queue row at status='pending', retry_count 0 -- nothing more. /failures matched only status='failed' or pending-with-retry_count>=3, so it matched none of those rows, and there are two ordinary ways they never leave that state: - startEmailQueueProcessor() was never reached, so nothing polls the queue. - Every pass returns early. processEmailQueue bails when the transporter will not initialise, before it touches a single row, so retry_count stays 0 and no error_message is ever written. A working SMTP test button does not contradict this: that path builds its own transport. adminSystem.js made it worse by reporting `emailProcessor: { status: 'active' }` as a literal, so the one place that named the worker always said it was fine. - emailProcessor records what each pass did -- started, lastRunAt, lastResult, lastError -- and exports getQueueProcessorStatus(). The transporter bail and the queue-query failure, the two silent early returns, both write lastError. - /failures gains `waitingEmails`: pending, under the retry cap, past any scheduled_at, and queued more than 10 minutes ago. The predicate mirrors the processor's own pickup query, so a row listed there is one it should already have taken; rows over the cap stay in `stuckEmails` and are not counted twice. A future scheduled_at is left alone -- split-payment invoices and the business-hours floor park rows deliberately. - System Health leads with the processor's state (running / stopped / degraded) and lists waiting emails in their own table. The all-clear now needs both buckets empty. - adminSystem reports the real processor state instead of the literal. - The two "queued" toasts say the queue processor is what sends it and where to look if it doesn't arrive. 8 route tests, all 8 failing before the change.	2026-09-02 13:49:22 +02:00
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