2225 Commits

This Branch

This Branch

All Branches

Author	SHA1	Message	Date
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	a87e688484	Merge pull request #1277 from PicPeak/fix/1275-hybrid-pointer-input-mode ... fix(gallery): follow the input in use, not the device's primary pointer (#1275)	2026-09-03 08:45:28 +02:00
Paul Nothaft	0b6b8fbdb0	fix(gallery): follow the input in use, not the device's primary pointer ... Closes #1275. Follow-up to #1263. `matchMedia('(hover: none) and (pointer: coarse)')` answers "what is this device's primary pointer", which on anything with both inputs is the wrong question. A touchscreen laptop reports fine+hover, so a finger tap was handled as a click: the photo opened with no reveal step and the tile's own actions needed a hover a finger cannot produce. An iPad with a trackpad reports the opposite, so a mouse click was handled as a tap and opening a photo took two of them while hovering did nothing. Pointer events carry the answer per interaction. useInputMode holds one module-level mode fed by a single window-level listener pair, so every tile agrees and the listener count does not scale with the grid. The primary-pointer query stays as the opening guess -- it is right for the two single-input cases that are most of the traffic, a phone and a desktop -- and the first real interaction corrects it on a hybrid. pointermove matters as much as pointerdown: a mouse announces itself by approaching, and the mode has to be right BEFORE the click, not as a consequence of it. A pen is grouped with touch, since it taps rather than hovers on most hardware and being wrong that way costs only a reveal step. GalleryPremiumLayout's touch rules move off the media query onto a data-input-mode attribute the layout sets, for the same reason: on a touchscreen laptop the query stayed false and a finger could never reach the checkbox or like button, and on an iPad with a trackpad it stayed true and both were stuck on permanently. The #1263 guarantee is unchanged and pinned by a test that walks all three modes: a control that cannot be seen cannot be hit, whichever input is in use. Verified in the running app under Chrome touch emulation, on a device advertising a coarse primary pointer -- the iPad-with-trackpad case. A mouse merely moving switched the grid to hover semantics and revealed the overlay, and a subsequent tap switched it back; the premium layout's attribute followed, with its checkbox reachable under touch and hidden-but-inert under mouse. The mirror case (finger on a fine-primary device) cannot be staged in Chrome, which couples touch emulation to a coarse primary pointer, so it rests on the jsdom tests. 12 tests: 8 on the store, 4 more on PhotoCard. 3 of the 4 fail without the per-interaction mode; the fourth is the #1263 no-regression guard and holds on both sides by design.	2026-09-02 20:04:15 +02:00
Paul Nothaft	30fa320dc2	chore(main): release 3.122.5-beta.0 (#1276) v3.122.5-beta.0	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	db197e7685	Merge pull request #1272 from PicPeak/fix/1263-mobile-photo-tap-collisions ... fix(gallery): stop invisible overlay controls swallowing mobile taps (#1263)	2026-09-02 16:46:00 +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	6bb12c6612	fix(crm): stop the invitation UI claiming more than it can know ... Codex review round 1 on #1274. Three places where this branch replaced one overclaim with another. The badge tooltip said "Invitation sent". createInvitation inserts the customer_invitations row and only then queues the email, with no transaction around the pair, so an open invitation does not prove an email_queue row exists -- and even when it does, delivery is the queue processor's business minutes later. The tooltip now describes the invitation link itself and points at System health, which is the same distinction #1273 draws. Both conflicts are 409 and were being treated as one. Migration-era send-invite returns 409 with code CUSTOMER_ALREADY_ACTIVE when the customer already has a password, which happens if an open invitation for that address is accepted between createDirect and sendInvite. There is then no invitation row to cancel, so directing the admin to the Invitations tab points them at something that does not exist. The code is now read before the message is chosen. A dropped connection or a timeout rejects with no `response` at all, and the request may well have succeeded server-side. Saying "no invitation went out" there sends the admin into a retry that then 409s, which is the same trap the CUSTOMER_ALREADY_ACTIVE case sets. That branch is now explicitly unconfirmed and says where the answer is. 3 more tests, all 3 failing before this commit.	2026-09-02 14:16:24 +02:00
Paul Nothaft	d0274886e6	fix(gallery): decide the overlay by pointer capability, not viewport width ... Codex review round 1 on #1272. Both findings are consequences of extending the Grid/Justified tap-to-reveal model to every layout: what those two layouts got away with, because they were the only ones using it, becomes wrong once Masonry, Mosaic and Timeline inherit it. The hover variants no longer hide behind `md:`. On a fine pointer under 768px `isTouchDevice` is false, so nothing reveals the overlay, and the `md:` prefix disabled the only hover variants there were -- the controls stayed `opacity-0 pointer-events-none` with no way to reach them. Grid and Justified already behaved that way, but Masonry, Mosaic and Timeline had unprefixed `group-hover:` and revealed at any width, so this was a regression for them. Width was never the real question: what the breakpoint was standing in for is that :hover latches on a touchscreen once a tile is tapped. So the variants are emitted for pointer devices only and withheld on touch, which says that directly. detectCoarsePointer no longer ORs the touch fallbacks over matchMedia. matchMedia describes the PRIMARY pointer; `ontouchstart` and `maxTouchPoints` only say a touchscreen exists somewhere, which is equally true of a touchscreen laptop or a docked tablet being driven by its mouse. OR-ing them classified those as touch-only, so an ordinary click merely revealed the overlay and opening a photo took two clicks. The fallbacks now stand in only where matchMedia is absent, which is what the comment already claimed. 3 more tests, all 3 failing before this commit.	2026-09-02 14:15:05 +02:00
Paul Nothaft	1b8e5f83d7	fix(crm): tell the admin whether a customer's invitation actually went out ... Closes #1261. "Invite customer" is two calls: createDirect, then sendInvite. The mode wiring is right -- CustomerManagementPage passes mode='invite' and InlineCustomerCreate does call sendInvite -- so the reported symptom is not a missed branch. It is that nothing downstream distinguishes the outcomes. Three things could not be told apart afterwards: - The success toast claimed "portal invitation sent". sendInvite only queues an email_queue row; whether it was delivered is decided minutes later by the queue processor. The toast now says queued, and says what sends it. - When sendInvite failed, the warning read "Invitation email failed -- retry from the customer detail page", which sounds like the mail bounced. What actually remains is a PASSIVE customer with no invitation at all, so it says that instead. A 409 is now separated out: that means an invitation for the address is already open and the RE-invite was refused, so the customer is invited and telling them to retry sends them the wrong way. - The customers table rendered a customer whose invitation never went out identically to one the admin created as passive on purpose -- both showed only "Passive - admin only". Passive customers with an open invitation now show "Invitation pending", matched case-insensitively because customer_invitations lowercases the address while customer_accounts keeps what the admin typed. The invitations list was already being fetched for the tab; this only cross-references it. Active customers are left alone: they have portal access, so a stale invitation row for their address says nothing about them. 7 tests; the 5 that assert the new behaviour all fail before the change, and the 2 negative controls pass on both sides.	2026-09-02 13:49:31 +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	c0d34796cd	fix(gallery): stop invisible overlay controls swallowing mobile taps ... Closes #1263. A tap on a photo tile did one of three things depending on where the finger landed: opened the photo, downloaded it, or liked it. The cause is that `opacity-0` hides pixels but not hit-testing. The overlay's View/Download/Like buttons and the selection checkbox were rendered at opacity 0 and left fully tappable; each one calls stopPropagation, so hitting an unseen button both fired its action and suppressed the tile's own open. On a pointer device hover reveals the controls before anyone can click them, so the gap never showed. On a touchscreen there is no hover, so in Masonry, Mosaic and Timeline the controls were invisible for good and tappable for good. Visibility and hit-testing now move together. PhotoCard computes both from one place, so every layout that uses it gets the same rule instead of passing its own opacity classes: - `touchAware` is gone. It gated the tap-to-reveal state machine, and only Grid and Justified opted in -- which is why those two behaved and the other three did not. Every PhotoCard layout is touch-aware now: first tap reveals the controls, second tap on a control acts, second tap elsewhere opens the photo. Pointer devices keep hover semantics unchanged. - The pointer reading moved from an effect into the initial state. As an effect it landed a mount-time render between the tile measurement in useLayoutEffect and the image mount that measurement gates, remounting every card once -- caught by the #1095 regression test, which is the reason that test exists. It also now degrades to ontouchstart/maxTouchPoints where matchMedia is absent, since every layout runs this path now. Two more instances of the same class, outside PhotoCard: - GalleryPremiumLayout's checkbox and like button are CSS-hidden the same way. They get pointer-events alongside opacity, and because that layout has no reveal gesture, a `(hover: none)` block shows both outright at a finger-sized target rather than leaving them unreachable. - PhotoGrid's download button called `onClick={onDownload}` with no stopPropagation, so downloading also opened the lightbox. Verified on a mobile viewport with real touch emulation: at rest the tile centre now hits the image rather than an unseen Download button, and one tap reveals the controls instead of downloading the file. 5 tests, all 5 failing before the change.	2026-09-02 13:49:10 +02:00
Paul Nothaft	ca8050899b	chore(main): release 3.122.4-beta.0 (#1270) v3.122.4-beta.0	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) v3.122.3-beta.0	2026-09-02 08:27:26 +00:00
Paul Nothaft	28f14955e2	fix(gallery): clear the guest identity on gallery logout ... The gallery password is one shared secret per event and does not distinguish people. With the guest identity outliving the tab, logging out and letting the next person enter that password greeted them by the previous guest's name, with "forget me" - which erases that guest's selections server-side - one click away. Logout is the leaving-this-device signal, so it now drops the local identity too. Server row untouched.	2026-09-02 10:22:55 +02: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	7a1ea842e4	fix(guests): read identity from whichever store holds it, write it as a pair ... Two defects in the storage fallback, both reproduced: The quota fallback repointed reads at sessionStorage through module state, which a reload discards. The next page load probed localStorage, passed the one-byte probe, tried to promote the pair and was refused on the same quota, swallowed that, and read an empty localStorage: the identity sat one store over, unreadable, and the guest re-registered. Reads are now read-through: primary store first, sessionStorage second, promoting into the primary only when it will take the pair and leaving it where it fits when it will not. No module state has to remember which store won. The migration wrote the token before the profile, so a store that accepted the first write and refused the second left a token with no profile: x-guest-token was sent while the provider prompted to register, producing a second row with two live tokens. Every write is now profile-first and rolls back on failure, so a store holds the whole pair or none of it.	2026-09-02 10:22:55 +02:00
Paul Nothaft	7d51aa3db9	fix(guests): don't answer feedback with a stale identity mid-invite ... Last open finding from codex round 3 on #1268. Invite redemption is async and the gallery stays interactive while it runs, so a like clicked in that window resolved against the persisted identity and was filed under the wrong guest permanently. ensureIdentity() now waits on the in-flight redemption and re-reads the result before falling back to the stored identity or the prompt.	2026-09-02 10:22:55 +02:00
Paul Nothaft	e9babf65e7	fix(guests): rebuild consumers on identity switch; repair fallback reads ... Codex review round 3 on #1268. Three of these were defects in the round 1-2 fixes themselves. Consumers holding local feedback state are now rebuilt on an identity switch. Invalidating queries was not enough: six gallery layouts seed their liked set behind a mount-only likedSeededRef ('so refetches don't clobber in-session optimistic toggles') and PhotoLightbox keeps its own copy, so a refetch left the previous guest's hearts on screen. The provider re-keys its subtree, which covers all seven without touching them. Deliberately only on a switch away from an established identity -- remounting on first sign-in would tear down the gallery under the click that triggered the prompt and drop the pending action. The storage fallback now repoints reads. storeGuestIdentity wrote to sessionStorage when localStorage rejected the real write but left resolvedStorage on localStorage, so every later read missed: x-guest-token was never sent and the identity vanished on reload. The fallback looked like it worked while achieving nothing. Clearing an identity now notifies this tab. Native storage events fire only in other documents, so the interceptor dropping a server-rejected identity left the provider still showing that guest and ensureIdentity() still handing it out. A same-tab event completes the loop. Cross-tab adoption resolves pending callers. A tab parked on the prompt awaiting ensureIdentity() while another tab registers now completes exactly as register() does, instead of hanging forever and registering a second guest if the visitor submits the still-open prompt.	2026-09-02 10:22:55 +02:00
Paul Nothaft	f3f37a8c77	fix(guests): invite wins over stored identity; clear server-rejected ones ... Codex review round 2 on #1268. Four findings, all reachable only because the identity now persists. An explicit ?invite= now takes precedence. The redeem effect skipped when an identity already existed, which was harmless while identity died with the tab. Persisted, it means opening guest B's invite on a browser where guest A once visited restores A, never redeems B's invite, and files B's likes under A. A ref keeps it to one redemption per token. Guest-scoped caches are invalidated when the identity changes. my-feedback, gallery-photos and photo-feedback are keyed by slug and photo id, never by guest, so they outlived an identity change and showed the previous guest's likes while requests already carried the new token. Now reachable three ways: another tab, 'Not you?', and an invite redeemed over an existing identity. An identity the server has rejected is dropped. resolveGuest nulls req.guest for a soft-deleted or merged-away row even when the JWT is validly signed and unexpired, and the route answers GUEST_IDENTITY_REQUIRED — no client-side expiry check can catch that. Self-limiting when identity died with the tab; persisted, it would fail every like for up to 30 days while the footer still showed the guest's name. The write fallback now covers the real write, not just the probe. A one-byte probe fits in a nearly-full store that still rejects a JWT plus profile, which left the context believing it was signed in with nothing persisted.	2026-09-02 10:22:55 +02:00
Paul Nothaft	51db1e09e9	fix(guests): expire stale tokens, sync tabs, survive unwritable storage ... Codex review round 1 on #1268. All three findings are consequences of the storage move itself. Expired tokens now read as absent. GUEST_TOKEN_TTL is 30 days and sessionStorage almost never survived that long, so 'stored but expired' was unreachable before; persisting the token makes it routine. Nothing else clears it -- the 401 handler in config/api.ts only drops gallery_event_<slug> -- so the visitor was shown as signed in while every like 401'd, and ensureIdentity() short-circuited so recovery was never offered. The signature is still the server's business; an unparseable token is left alone. Tabs now stay in step. localStorage is shared where sessionStorage gave each tab its own copy, so 'Not you?' or a registration in one tab silently changed the token every other tab sends while they still displayed the old name -- their likes would land on the new guest, the exact misattribution this branch set out to stop. A storage listener rehydrates the others. Storage is probed for writability, not just readability. A store that reads but throws on setItem (quota, private mode) sailed past the read-only guard, and storeGuestIdentity threw after the server had created the guest: failed registration, retry, duplicate row. Writes are also wrapped so a storage failure degrades to a per-session identity instead of rejecting registration.	2026-09-02 10:22:55 +02:00
Paul Nothaft	a21c4d3bf5	fix(guests): keep guest identity across a tab close ... Closes #1265. The guest JWT and profile lived in sessionStorage, so the practical lifetime of an identity was "until this tab closes". GUEST_TOKEN_TTL was raised to 30 days in #1216 specifically to stop identity churn, but it governs how long the token stays valid, not how long the browser keeps it -- so it was almost never reached. A guest who closed the tab and came back through the same emailed link got the registration prompt again, and the ?invite= token in that link is single-use and already redeemed, so it could not put them back. Typing the same name inserted a second gallery_guests row: their earlier likes then belonged to an identity they could no longer act as, and could not be removed. Moved to localStorage, which is the reporter's suggestion and the one that lines up with the TTL that already exists. This does not reopen the objection #1216 raised. Deduplicating on a typed email was rejected there because anyone knowing an address could claim that person's identity, and answering differently for a known address leaks which addresses are in the gallery. This grants nothing to anyone -- it only stops the browser discarding a token it was already given. Gallery ACCESS stays in sessionStorage (galleryAuthStorage.ts) and is untouched, so a returning visitor still has to pass the gallery password before a stored identity means anything. Two things the storage swap alone would have got wrong: - Anyone with a gallery open at upgrade time would be treated as a new guest on their next reload -- the exact duplicate-row bug this fixes, fired once per in-flight guest. getGuestToken/getGuestIdentity now move a pre-#1265 sessionStorage entry across on first read. It moves rather than copies, and a fresh registration in the current tab always wins over a stale copy. clearGuestIdentity clears both stores, so "forget me" cannot be undone by a leftover being migrated back. - Identity now surviving a tab close means a second person on a shared device can be greeted by the previous visitor's name. Their only exit was "Forget me", which soft-deletes the guest row and anonymizes their feedback -- it would erase the wrong person's selections. Added a non-destructive signOut() and a "Not you?" control next to it, which only clears the identity on this device. Storage access already funnelled through one getStorage() accessor, so the swap is a one-line change there; it falls back to sessionStorage when localStorage throws (Safari private mode, blocked by policy) rather than dropping identity entirely. 6 tests. 3 fail against the old implementation, including the core "survives a tab close" case; the other 3 pin the migration and the both-stores clear. Note: the new "Not you?" string is added to en and de. i18n:ci is already red on main (11,405 missing keys) because the extractor there manages six locales while only en/de are kept at parity; this adds 4 entries of that same class. PR #1267 fixes the check itself.	2026-09-02 10:22:55 +02:00
Paul Nothaft	cad699a5be	Merge pull request #1267 from PicPeak/fix/testplan-followups ... fix: close the remaining 2026-09-01 QA items, warnings and follow-on defects	2026-09-02 10:22:08 +02:00
Paul Nothaft	9b63399c48	Merge pull request #1266 from PicPeak/fix/testplan-2026-09-01 ... fix: resolve the 2026-09-01 QA run findings (#1-#21) and repo-health debt	2026-09-02 10:18:39 +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	4646d5de69	i18n: drop the keys this branch orphaned ... Cleaning up after our own changes, not pre-existing dead keys. - gallery.feedback.* (10 keys) -- StoryFeedbackSheet was their only consumer and it was removed in 3ef4bd8c as an unreachable duplicate of the lightbox. - cssTemplates.title and settings.moderation.wordFilters -- orphaned by b80ce73e, which removed the component-side heading on the tabs that rendered a heading identical to the shell's. - settings.analytics.customCspWarningText -- superseded by customOnlyCspWarningText in 9251745a, which was deliberately a new key so the stale pre-proxy string could not win over the new inline default. The sibling customCspWarning title is still in use and stays. Each verified to have zero t() references in src before removal. Key-diff against HEAD: en/de -13, the six partial locales -12 (they never had customCspWarningText), 0 changed and 0 added in any of the eight. removeUnusedKeys is false by design, so this had to be a deliberate pass.	2026-09-02 09:43:10 +02:00
Paul Nothaft	4c6ca49b17	fix(gallery): restore the download CTA under headerStyle "none" ... The report asked whether this was intentional. It is collateral damage from the #386 swap, not intent. There are two header download affordances. GalleryView sets showDownloadAll={false} unconditionally -- "replaced by the new showHeaderDownload (#386)" -- and passes showHeaderDownload={allowDownloads}. GalleryLayout renders HeaderDownloadButton in the standard, minimal and hero branches, but the isNoHeader branch only ever had the now-dead showDownloadAll button. Net effect: zero download CTA on headerStyle 'none'. The comment claiming intent -- "Intentionally NOT shown in the no-header variant where the gallery is fully chromeless by design" -- is factually wrong about its own branch: isNoHeader renders the menu button, headerExtra (upload button, countdown timer) and logout. It is a functional-controls bar, not chromeless. The sentence predates the #386 swap, when showDownloadAll still gave that bar a download button. Renders HeaderDownloadButton in that branch in the same slot order as the other three; it is icon-only below sm, so it fits the compact bar. Removed the two now-false comments. Beta themes are unaffected: gallery-premium and gallery-story return from an earlier branch that never mounts GalleryLayout and get download-all via their own onDownloadEverything prop, so there is no double CTA. Refs testplan REPORT.md, headerStyle:none download-CTA warning.	2026-09-02 09:43:10 +02:00
Paul Nothaft	504a8b6fae	fix(events): honour ?tab=, show a load error, and stop lying about uploads ... Three warnings on the event-details surface. ?tab= deep links were ignored -- activeTab was hardcoded to 'overview' and nothing read or wrote the search param, unlike Settings. Mirrors SettingsPage's pattern exactly (module-level key list + type guard, seed useState from the param, write-back and reflect-back effects), plus a snap-back for the `guests` tab, which only renders when identity_mode is 'guest' -- a deep link to it on any other event would otherwise show a tab bar with no content. The snap-back is guarded on the query's isLoading so it cannot fire against undefined settings and kill a legitimate deep link. Worth recording: the two effects ping-pong infinitely if activeTab and a valid URL tab disagree at mount, which is exactly the pre-fix state. The seeding is what makes them agree, so the fix is also what makes the pair safe. Offline Photos tab rendered the "no media uploaded yet" empty state on a failed fetch, because `data: photos = []` makes a rejected query indistinguishable from an empty one -- a user could reasonably think their photos were gone. Threaded isError through and added a third branch, reusing TaxReportPage's existing error-with-retry shape. Needed no new keys. The spurious "Upload completed successfully" toast was in the host, not the uploader: PhotosTab hung toast.success off PhotoUpload's onUploadComplete, which is documented as a grid-refresh signal and fires as soon as the transfer loop exits -- including when the request 400'd on the photo cap or every file was rejected by magic-byte validation. PhotoUpload's own toasts were already correct. Removed it, and added a real partial-success branch reporting the actual split instead of a plain "Upload complete!". The guest uploader had a variant of the same bug in a different place: its toast is gated on successCount, but successCount++ fired on any resolved request -- and the upload route answers 202 with count: 0 and an errors[] entry when the file is refused. So a refused guest photo produced "Upload completed successfully (1 photos)" and pushed a useless upload_id into the processing poll. Now gated on count. Refs testplan REPORT.md, ?tab= / offline-empty-state / spurious-toast warnings.	2026-09-02 09:43:10 +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	413290af3e	i18n: fail safe on empty strings, normalise German to Sie ... D2 -- returnEmptyString: false. i18next defaults it to true, so an empty translation was returned as valid and rendered as blank UI instead of falling back to English. Verified safe first: zero empty-string values across all 8 locales, no addResourceBundle or runtime resource injection, no public/locales for the HTTP backend, and the three t(key, '') call sites resolve against key families fully populated in en and de. D3 -- German formality normalised to Sie throughout, 101 strings. There is no deliberate du island: Sie outnumbered du roughly 6:1 (~390 vs 65 addressed strings), every namespace with more than ten addressed strings was Sie-dominant, and the guest gallery plus all public/billing surfaces were already 100% Sie. Even customer.*, the reported offender, was internally mixed rather than consistently du. Two detection passes: du-pronouns (now zero) and du-imperatives without a pronoun (Klicke…, Aktiviere…, Wähle…). Placeholders verified mechanically unchanged. Left alone: ten 1st-person progress labels (Lade Benutzer…, Prüfe…, Teste Verbindung…) -- those are label style, not address, and normalising two of ten would have made it worse. C7 -- removeUnusedKeys stays false, but the comment now carries measured evidence instead of an estimate. The honest attempt was made: 61 preserve globs derived mechanically from all 82 dynamic key templates in src (far more than the 5 families previously named) plus 17 constant-table prefixes cut removals from 422 to 158. Two things still block it. 47 of the remainder are the base form of a plural key that src does pass to t(); i18next tries the _other suffix first so nothing visibly breaks, but covering them needs a literal pattern per key and forgetting one silently deletes a live key -- exactly the failure the flag prevents. And pruning is not idempotent: run for real, extract had to run three times before --ci --dry-run came back clean, each pass uncovering another removal, so i18n:ci would fail on a correct tree until someone ran extract enough times. Also adds the three settings.analytics keys that 9251745a referenced in AnalyticsTab without adding (proxiedNotice, proxiedNoticeText, customOnlyCspWarningText) -- en from the source defaults, de translated. Refs testplan REPORT.md C7, D2, D3.	2026-09-02 09:43:10 +02:00
Paul Nothaft	758dc005df	fix(events): rename a shadowing local and bound the photo-cap input ... Two small fixes in one file. The local `mode` at line 323 collided with the info-banner `mode` the i18next TS resolver reads at line 490, so the extractor emitted four keys the code can never request (events.infoBanner.mode_managed / mode_reference and the promoBanner pair) -- the real modes are inherit|custom|off. Renamed to sourceMode; the four phantom keys are dropped from the locale files. Also bounds the Photo Limit input, the twin of the one fixed in e5f6085a: min={0} with no max makes input[type=number] report aria-valuemax="0", and an out-of-range value only failed at INSERT. Set to the events.photo_cap column's real signed-32-bit ceiling. Refs testplan REPORT.md B15 and the aria-valuemax warning.	2026-09-02 09:43:10 +02:00
Paul Nothaft	be39929476	fix(accounting): allow creating a customer from the picker ... With Accounting on and CRM/customerPortal off, the picker renders but there was still no way to create the first customer: /admin/clients/accounts and every CRM editor with inline-create are feature-gated, and the picker's empty-state hint pointed at that unreachable page. Reuses the existing InlineCustomerCreate that CustomerPicker already mounts for the CRM editors. The affordance is gated on customers.create, matching the backend, where POST /admin/customers is permission-gated and not flag-gated. mode is 'passive' when customerPortal is off -- a portal invitation would email a link to a login that does not exist -- and 'both' when it is on. On success the customer is appended to the selection, which is what the accounting call sites' next.slice(-1) already expects. The noResults hint pointing at the hidden page is replaced by two keys: one naming the button, one for admins without the permission. Refs testplan REPORT.md B12.	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