AuthenticatedImage accepted the whole image-protection prop surface and
discarded it in a `void unusedProps` block. Callers computed those props
from the event's protection level and passed them in good faith, so
raising the level produced canvas rendering (via the layouts' own OR on
`protectionLevel === 'maximum'`) and nothing else the level implies.
Removes them from the interface and from every call site, so the props
state what the component actually does. Two survive because they are
real: `useCanvasRendering`, and `onProtectionViolation` — which #1297
listed as inert but which does fire, from the canvas context-menu
handler. `useWatermark` is removed as well; #1297 did not list it (it sat
outside the `unusedProps` block) but it was equally dead.
Removal rather than implementation is deliberate. The implementation
these props describe already exists in `ProtectedImage`, which is
exported from the barrel and rendered nowhere. Wiring it in is a product
decision about what protection level should mean, not a side effect of a
cleanup.
Analytics payloads inside the surviving onProtectionViolation handlers
keep their photoId/protectionLevel fields.
Refs #1297
Part B of #1264. Flag off by default, so an install that never enables it
gains no route, no nav entry and no way to mass-mail.
A campaign is a body plus a recipient rule. Queueing one writes ordinary
email_queue rows (email_type 'newsletter', origin 'campaign', new
campaign_id), so retry, rendered_html, sent_at and error_message all come
from the existing processor rather than a parallel sender. Throttling
staggers scheduled_at; the processor loop is untouched.
Two rules the service enforces: no raw HTML is ever stored (sanitized on
write and again on render, idempotently), and opt-out is checked at queue
time AND again at send time.
Migration 199 adds email_campaigns, email_campaign_recipients,
email_queue.campaign_id, customer_accounts.marketing_opt_out(_at), and
the newsletters.view / newsletters.send permissions.
Three rounds of external review are folded in, including several that
would otherwise have shipped broken:
- Campaign rows never came due on SQLite. queueEmail writes a Date, which
the sqlite3 binding stores as epoch ms; ISO text in the same column
compares as TEXT against an INTEGER, and SQLite orders every INTEGER
below every TEXT. The feature silently sent nothing there.
- The flag had no Settings card and no sidebar entry, so it could not be
enabled through the UI at all.
- Consent is per ADDRESS, not per row: two accounts sharing an inbox meant
unsubscribing stopped one and not the other, at both queue and send time.
- The unsubscribe GET mutated consent, so a mail-security scanner walking
a campaign could have unsubscribed much of the list. GET now confirms,
POST acts.
- The rate ceiling is clamped to the queue's real throughput (10/min), so
the composer's estimate stops being wrong by up to 12x.
Closes#1264
sanitizeCSS "blocked" a remote url() by prefixing it with a
/* BLOCKED URL */ COMMENT and leaving the URL in place. CSS comments are
discarded during tokenization, so the declaration a browser parsed still
carried the live URL — while adminCssTemplates returned
sanitization_warnings claiming it had been stopped. Protection that
reports success is worse than none, which is why it survived review.
Scope is narrow: sanitizeCss (lowercase, the public-site path) never
included the pattern and permits remote URLs by design — a test now pins
that. Only sanitizeCSS (uppercase) was affected; outside this repo's
newsletter branch its sole caller is adminCssTemplates.js.
Migration 200 is required, not cosmetic: gallery.js serves
css_templates.css_content VERBATIM as text/css and does not re-sanitize on
read, so fixing the write path alone would leave every existing template
serving its URL forever.
Review follow-ups replaced the regex with a small three-state lexer
(comment / string / identifier) over the RAW text, after five further
bypasses: a ")" inside a quoted url(), CSS escapes (u\72l), the HTML
comment strip JOINING tokens into a live url() after the scan, an escaped
quote desynchronising the scan, and a quote inside a comment. Escapes are
decoded only to decide, never to rewrite — a clean input now round-trips
byte-identical, which also keeps unaffected rows out of the migration's
write path.
Severity is low (writing a template needs branding.edit) but the harm is
a gallery visitor's IP reaching a third party from a page the operator
believes carries no remote requests.
tailwind.config.js had plugins: [] and @tailwindcss/typography was never
installed, so every prose / prose-neutral / dark:prose-invert class in the
app resolved to nothing. Preflight, which IS active, resets h1-h6 to
inherit size and weight and strips list-style from ul/ol — so an applied
<h2> rendered pixel-identical to the <p> it replaced.
The editor was never broken. The toolbar highlighted because
editor.isActive('heading') correctly returned true; only the CSS to show
it was missing. That also explains why pasting rendered rich text worked:
it carries inline styles.
Ten surfaces rely on these classes, including the PUBLIC CMS pages — so
impressum/datenschutz were serving unstyled headings to visitors too.
Review follow-ups: prose colours are mapped to the theme tokens wherever
.text-theme marks theme-owned text (a dark gallery preset sets
--color-text but no .dark class, so dark:prose-invert never engages and
headings would have gone near-black on dark); code blocks inherit rather
than being scaled twice; and H5/H6 get explicit rules, since the plugin
only styles h1-h4.
Closes#1288
Hardening for the large-gallery stall. The reporter could not isolate the
cause and neither could I from static reading; these are two defects that
are wrong independently of whether they are the whole story.
Gallery grids are NOT virtualized: a 546-photo event puts 546 PhotoCards
in the DOM, each mounting its own bare fetch. Two problems there: no cap,
and a cleanup that only set a flag while the request kept running.
withImageFetchSlot now holds requests to six in flight, with the BODY read
inside the slot — fetch resolves on headers, so releasing there would have
bounded header round-trips and nothing else. Teardown aborts via
AbortController.
A request queued indefinitely is PENDING, not failed, which is why the
failure left no console error, no failed request and nothing in the
backend log.
Review follow-ups: three tiers (current lightbox slide > neighbour
prefetch > grid thumbnails), because a single FIFO put the image the user
just clicked behind hundreds of thumbnails; and a synchronous throw now
releases its slot instead of permanently draining the pool.
If it recurs, capture performance.getEntriesByType('resource') for the
stalled thumbnails — a queued request shows responseStart === 0.
Relates to #1287
The business profile already carried the operator's full issuer block —
address, phone, email, website, VAT id — but none of it reached an email.
Those columns only fed the quote/invoice PDF renderer, so every outgoing
mail footer was the fixed logo + company name + copyright line.
The signature is rendered by wrapEmailHtml and nowhere else, so no
template, no per-type send path and no queue row needed a change. Two new
columns on business_profile (migration 198) carry the toggle and one
free-text legal line; everything else is read from the address fields the
operator already maintains.
Default off, with a test pinning that the disabled path is byte-identical
to a no-profile install.
Includes three rounds of external review fixes: the plain-text MIME part
also carries the signature; string booleans ('false'/'0') no longer
invert the toggle; the status line stays silent rather than asserting
"off" while unauthorised or loading; and the preview's Text tab mirrors
the send path's htmlToText fallback.
Manual Messages replies deliberately keep no signature — they bypass the
wrapper by design — and the UI copy names that exception.
Closes#1264 (Part A)
Adds docs/upload-file-types.md: the single Allowed File Types setting, every
path it governs, the extension-to-MIME table, how to enable video, and what
changed for chunked-upload/init (declared mimeType ignored, extension must
be allowed, 400 File type not allowed). README links to it, and the Settings
help text in EN and DE now says the list covers all upload paths and that
video extensions must be added explicitly.
backend: qs and body-parser (array-limit bypass, isBuffer DoS).
frontend: axios 1.17 line (formToJSON recursion DoS, prototype-pollution
gadgets, maxBodyLength bypasses), dompurify, linkify-it and the transitive
set npm audit fix resolves without a major bump.
Left out on purpose: sanitize-html 2.17.7 (its htmlparser2 12 tree is
ESM-only, which Jest 29 cannot load, and the SVG SMIL advisory needs svg
tags none of our sanitizer configs allow), and the tiptap 2->3 and
react-router 6->7 majors (open redirect via <Link> needs a user-controlled
navigation target, which the SPA has none of).
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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 da9ceb14. Swept the
whole of each file rather than the cited lines: 3 in SystemHealthPage, 5 in
CrmOverviewSection (two of them h3 elements with no colour class at all), 17
in HoursSection. All three are admin-only -- call sites verified as
AdminDashboard, CustomerDetailPage, HoursLoggingPage and /admin/system-health
-- so no customer-portal or public-token surface is affected, where these
utilities are correct by design. Zero themed utilities remain in the three
files.
Refs testplan REPORT.md B14.
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