28f14955e2bdeb8bfed1ca3b2c0111ce07e540c2
2203 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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. |
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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 |
||
|
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 |
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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). |
||
|
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. |
||
|
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.
|
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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.
|
||
|
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.
|
||
|
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. |
||
|
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. |
||
|
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.
|
||
|
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. |
||
|
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. |
||
|
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. |
||
|
Paul Nothaft
|
3acb452090 |
fix(settings): remove the duplicated section heading on 11 tabs
A generic shell heading stacked on top of each tab component's own internal
heading. The report named five tabs "at least"; auditing all 28 found 11:
downloads, sso, apiTokens, webhooks, businessProfile, crm, accounting,
whatsapp, slideshow, moderation, styling. On the first eight the two headings
resolve to the identical string -- sso and businessProfile literally render
the same key twice. The other three were near-identical stacked titles
("Moderation"/"Word Filters", "Custom CSS"/"Custom CSS Templates",
"CRM behaviour"/"CRM settings").
Clean, and left alone: general, events, categories, thumbnails, security, seo,
imageSecurity, status, analytics (its first heading is a genuine sub-section),
plus the eight already in TABS_WITH_OWN_HEADER.
Removed the component side and kept the shell heading: the shell heading is
the consistent one (icon + label + divider on ~20 tabs) and always matches the
nav item the admin clicked, and none of these components are mounted outside
SettingsPage, so nothing loses a title. Subtitles and intro copy preserved
throughout; orphaned icon imports removed.
The guard test was checked against the pre-fix blobs and does fail on them.
Refs testplan REPORT.md, "duplicate H2 section heading" warning.
|
||
|
Paul Nothaft
|
22cada9082 |
fix(ui): drop themed text colours from the last three admin surfaces
SystemHealthPage, CrmOverviewSection and HoursSection used text-theme
(color: var(--color-text)) explicitly, so on a dark-toned branding theme they
render near-invisible on the light admin background -- and because the class
is explicit it beats the AdminLayout default that protects everything else.
Converted to the neutral scale using the convention from
|
||
|
Paul Nothaft
|
bd44708a03 |
fix(contracts): widen the block-library list column
Block names ellipsized to ~4-6 characters ("Vertr...", "Bildr..."). The
tooltip added earlier made them recoverable but the list still was not
scannable.
The file's "intentionally mirrors EmailConfigPage's Templates tab" comment was
the reason the ratio was left alone. Re-evaluated: both pages render in the
same Settings shell so the ratio is shared, but the content is not. A block
tile spends a fixed ~105px of its row on the "System" badge plus the n/6 pill,
and block names are long German noun phrases; EmailConfigPage's tiles carry
one badge and short display names ("Gallery Created"). So the shared ratio is
not simply wrong -- it is wrong here. Diverged only here: lg:grid-cols-3 /
col-span-2 becomes lg:grid-cols-5 with a 2/3 split (40/60). Mobile stack
untouched, and the comment now names the divergence and why.
Refs testplan REPORT.md A5.
|
||
|
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.
|
||
|
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. |
||
|
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. |
||
|
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.
|
||
|
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. |
||
|
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. |
||
|
Paul Nothaft
|
355fe4ff43 |
fix(email): give every template a real display name in the config UI
D4 audit found defaultTemplateKeys was worse than stale sample data:
- Only .name was ever read. The subject/body/variables triple on each entry
was dead data -- and it is where {{password}} and {{expiration_date}}
originated, neither of which exists in any shipped template (they are
gallery_password and expiry_date).
- It covered 4 keys out of ~40. A fresh install already carries 17 templates,
and ~40 with the CRM flags on. Every key not in the list rendered its raw
snake_case template_key as its display name in both the sidebar and the
read-only "Template name" field -- customer_gallery_assigned,
database_backup_completed, invoice_collections_handoff, all five
event_reminder_*, and so on.
Replaced with TEMPLATE_DISPLAY_NAMES covering every key from
migrations/core/*.js plus the crm/contract/eventReminder template services,
falling back to the raw key. Drops the now-orphaned password sample value.
Adjacent drift found, not fixed (different const, and fixing it would be
scope creep): eventReminderTemplates.js inserts with category 'crm' /
subcategory 'event_reminder', neither of which is in CATEGORY_ORDER or
CORE_SUBCATEGORY_ORDER, so all five reminder templates fall through the
unknown-category fallback into core -> "other". They are visible, just filed
in the wrong bucket.
Refs testplan REPORT.md D4.
|
||
|
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 &.
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.
|
||
|
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.
|
||
|
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.
|
||
|
Paul Nothaft
|
4c2eeab2f4 |
refactor(gallery): drop the unreachable Story feedback sheet
StoryFeedbackSheet could never open: handleOpenFeedback was the only caller of setSelectedPhotoForFeedback and was itself never called. This was the last remaining build:check error (TS6133). Removed rather than wired up, on three findings: - The sheet offered nothing PhotoLightbox does not, and was strictly worse. It held comments and ratings in layout-local useState and never called feedbackService.getPhotoFeedback, so existing server-side feedback was invisible; it rendered stars and a comment form unconditionally, ignoring allow_ratings/allow_comments; and it had no reactions, colour labels, identity modal or rate-limit handling. This layout already renders PhotoLightbox with feedbackEnabled, which does all of that against the server. - It was not a mobile affordance. The CSS styled it as a fixed right-edge desktop drawer (right: 0; max-width: 28rem) with no media query. - Every sibling layout routes feedback through the lightbox. Grid, Masonry, Timeline, Mosaic and Carousel expose a per-card onQuickComment that calls onOpenPhotoWithFeedback to open the parent's lightbox on the feedback tab; none has a standalone feedback surface. The closest sibling, GalleryPremiumLayout, renders its own lightbox and deliberately voids _onOpenPhotoWithFeedback with no per-card control -- exactly the shape Story now has. Drops the component, its state and handlers, the feedbackOptions destructure (only the sheet read it) and 251 lines of orphaned CSS. savedIdentity also fed guest_name/guest_email into the like call; those were always undefined at runtime since the unreachable sheet was their only writer, so no behaviour changes. Also widens the Story nav search input, which clipped its placeholder. At the input's computed 14px the placeholder measures en 121px, de 145, ru 152, fr 174 against a 128px box -- so German was 17px over and French 46px over. 8rem -> 13rem collapsed, 12rem -> 17rem focused, keeping expand-on-focus; verified at 1280px and at the 768px breakpoint where the search appears. Refs testplan REPORT.md A1 and the gallery-story placeholder warning. |
||
|
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. |
||
|
Paul Nothaft
|
72894e22c2 |
fix(gallery): stop browser zoom tripping the devtools viewport heuristic
innerHeight is in page CSS pixels and shrinks under browser zoom; outerHeight does not. At 150-200% zoom a normal window therefore shows an absolute outer/inner gap of 400-500px, past every threshold, so an accessibility zoom read as a docked DevTools panel and - at protectionLevel "maximum" - redirected the guest off the gallery on load. Pre-existing (the previous threshold was 100px), but the rewrite kept the shape. The gap is now measured relative to a baseline taken at mount, and the baseline is re-taken whenever devicePixelRatio changes, which a zoom step does and a docked panel does not. Only a gap that grows past the threshold at a constant ratio counts. The mount-time check is dropped: a panel that is already open at load is indistinguishable from a zoomed window. |
||
|
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. |
||
|
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) |
||
|
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.) |
||
|
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
|
||
|
Paul Nothaft
|
5dbb43549c |
fix(i18n): make i18n:ci pass by fixing the extractor config
exit 1 -> exit 0.
Three findings, none of which matched the reported symptoms.
1. The two "unparseable .d.ts files" are not malformed. RestoreWizard.d.ts and
BackupHistory.d.ts are valid declaration files sitting next to their .jsx
implementations; i18next-cli feeds them to SWC as ordinary .ts modules with
no ambient flag, where an uninitialised `const` is a hard syntax error. They
should never have been scanned at all. Root cause is the input glob:
i18next-cli passes `input` straight to `glob`, which does NOT honour
`!`-prefixed negation inside the pattern list, so
'!src/**/*.{test,spec,d}.{ts,tsx}' was a silent no-op and all four .d.ts
files plus 57 test files were being scanned. Moved the exclusions to
extract.ignore, where they take effect; the extracted key set is unchanged.
2. The "missing French keys" were not English-vs-French drift. The extractor
wanted to add ~2771 keys to fr.json with value "" -- and src/i18n/config.ts
does not set returnEmptyString, whose i18next default is true, so those
empty strings would be returned as valid translations and render as blank
UI rather than falling back to English. Filling nl/pt/ru/fr with ~11000
empty strings would have been a worse regression than the failing check.
The check was demanding parity for locales this project deliberately keeps
partial, so `locales` is now ['en','de'] -- the two actually kept at parity.
nl/pt/ru/fr join sl/es as hand-maintained partial locales on
fallbackLng 'en'. No French was written.
3. de.json is a parity locale, and the extractor legitimately found 307 keys
missing from both en and de (shipped t() calls never added to the locale
files). Rather than accept 307 blank German strings these were written by
hand: 105 are _one/_other variants derived from existing German bases with
correct singular/plural, the rest translated against each section's register
(Sie on admin/public-billing surfaces, du in the customer portal to match
customer.quotes/customer.bills) reusing terms already established in de.json.
Verified: 0 interpolation-placeholder mismatches between en and de across
all 308 new keys, 0 empty and 0 key-shaped values remaining, and the diff is
strictly additive (en +308, de +307, 0 removed, 0 changed).
removeUnusedKeys is now false, replacing the dead preservePatterns: []. It
wanted to delete ~355 live keys per locale across ~90 prefixes -- families
built at runtime (admin.activities.*, admin.notificationMessages.*,
projects.status.*) or held in constant tables the extractor cannot resolve
(AdminSidebar nameKey, CrmDevelopmentPage titleKey/descKey). Covering them
would need ~30 wildcards spanning most of the key space; disabling pruning is
the same behaviour, honestly stated, with the call sites named.
Refs testplan REPORT.md #22 (Part 1.3.03).
|