Two things reported on the reformatted consent modal.
The green box is gone. Setting "what is never included" apart as a
tinted panel broke the rhythm of the sections and read as an arbitrary
highlight rather than emphasis. All six sections are uniform now; the
icon and heading are enough to tell them apart.
The green bars across the disclosure were a focus ring, not a border.
showModal() focuses the first focusable descendant, which since the
reformat was the scrollable region I had given tabIndex={0} — so its
inset ring was drawn for every user the moment the dialog opened, and
because the dialog clips its sides a full-width inset ring appears as
two coloured bars. Focus now goes to the dialog itself, which is also
the better screen-reader behaviour: the title is announced on open, and
the region's ring appears only when someone deliberately tabs to it. It
is a thinner, softer ring for that case. The dialog suppresses its own
ring, since that focus is programmatic rather than keyboard navigation.
The collector shown in the transport sentence was never wrong: it
interpolates the configured collector, and the screenshots showing
http://127.0.0.1:9 were taken on a rig deliberately pointed at a dead
loopback port so they could not reach production. Re-checked with
USAGE_COLLECTOR_URL unset: the sentence reads
https://usage.picpeak.app and both links resolve there.
Refs #1110
It was seven anonymous paragraphs in one scrolling block, with the title
and the buttons scrolling away with them. The scroll container is
keyboard-focusable, and unstyled it drew a default focus ring, so the
disclosure also looked like a giant textarea.
Now: a fixed header carrying the icon, title and purpose; a scroll
region with six labelled sections, each with a small heading and icon so
the thing can be scanned rather than only read; and a fixed footer with
the consent checkbox and the actions, which no longer scroll out of
reach on a short screen. "What is never included" is set apart as a
tinted block, since it is the part that answers the question an operator
actually has. The focus ring is now a deliberate inset ring on a
labelled region, which is correct for keyboard use instead of an
accident that looked like a form field.
Dark mode is fixed as part of this, and it was my own doing: the dialog
used `bg-theme-surface`, which does not follow dark mode, and the
section text I added carries dark: variants. Light surface plus
near-white text is unreadable. The surface is class-driven now —
neutral-800, which is what `.card` resolves to in dark and what the rest
of the admin UI uses. Verified in both themes through the app's own
theme toggle rather than by forcing the class, which is what produced
the misleading half-state the first time I looked.
Six section headings added in EN and DE.
Refs #1110
Three items, one of which explains an error seen in the app.
"The operation could not be completed" could come from a config typo.
status() called collectorUrl() bare, and that throws on a bare hostname,
a path, a query, or http in production. The settings page renders one
generic failure when its status query errors, so a misconfigured
USAGE_COLLECTOR_URL replaced the whole tab with that sentence — no
cause, and no way to read the status or withdraw, because every control
there sits behind that call. The URL is now reported as
collector_error: 'INVALID_COLLECTOR_URL' beside the real state, the tab
says what is wrong and how to fix it, and the links are only rendered
when there is somewhere to point them.
gallery_layouts reported grid for every preset-themed install.
color_theme holds either a theme object or the NAME of a preset — the
theme picker stores names, and eventTypeService seeds them
(`theme_preset: 'corporateTimeline'`). Only reading value.galleryLayout
made masonry, timeline, mosaic and the two gallery presets invisible.
Names now resolve, and an event with no theme of its own resolves
through the global one instead of being counted as grid. Only the
name -> layout mapping is duplicated, not the presets;
frontend/src/types/theme.types.ts stays the source of truth, and an
unknown name reports `other` so a preset added later degrades to
"something else" rather than quietly inflating the grid count.
custom_css missed CSS applied through a template. An enabled
css_templates row applied via events.css_template_id is gallery styling
by the same definition as the settings fields — the Custom CSS tab is
where both are authored — but neither the snapshot nor the middleware
saw it, so those installs reported custom_css entirely false. Existence
only; template contents are never read.
Eleven tests. Reverting each fix in turn fails 3, 1 and 3 of them.
Refs #1110
Third and last window in the same race, and again in my own fix.
locked() claims the lease and reads the row in two separate statements.
Reading the cancellation counter from inside that callback meant a
/disable completing in the gap was adopted as this activation's own
baseline and silently absorbed — the counter matched, the claim
succeeded, and registration went ahead after the operator had withdrawn.
The baseline is now read before the lease is taken, which inverts it:
every increment from that point on is later than the value the claim
tests for, so the claim fails and the withdrawal wins. An increment from
before the read is a withdrawal the operator already completed, and a
deliberate opt-in afterwards should not be vetoed by it.
The test for this passed against the bug on its first two attempts. It
stubbed the state read to increment the counter AFTER reading the row,
so both the broken and the fixed version saw the old value and behaved
identically. The withdrawal has to land before the read returns for the
row to carry it — which is the whole point of the window. It now fails
without the fix.
Refs #1110
Follow-up review on the previous commit, including a hole in that
commit's own fix.
The cancellation flag became a counter. Clearing a boolean needed a
write of its own, and a /disable landing between the lease and that
write was erased — the same race one level down. enable() now records
the counter it started with and claims only if it is unchanged, so no
clearing write exists to lose. It also fixes the case a boolean could
not express at all: a stale cancellation already set, and a fresh one
arriving mid-activation, are indistinguishable as flags and obvious as
counts. Migration 203, separate from 202 for the reason 202 was separate
from 201 — knex will not re-run an applied migration.
deliver() re-checks immediately before dispatch. The existing check ran
before the binding lookup, which is asynchronous, so a withdrawal that
COMPLETED during it still had its registration or report sent
afterwards. Not an already-in-flight request — a new one started after
the operator had withdrawn.
The outbox writes in tick() and command() are conditional on still being
active. /disable clears pending_packet without holding the lease, so an
unconditional write put a report — or a feedback body and name — back
into an outbox the withdrawal had just emptied, where deliver() would
then leave it, since it declines to send anything but the delete.
Per-item name consent resets with the item. `named` stayed checked after
submitting, so the next item carried the previous name automatically,
contradicting the anonymous-by-default promise the disclosure makes for
each item. The remembered name stays in preferences; attaching it is
decided again each time.
Two of these tests were worthless when first written and are noted
because the pattern keeps recurring: the pre-dispatch case passed
without the guard because an empty report payload failed schema
validation during signing, so nothing reached the collector for reasons
unrelated to the check. With a valid payload it fails without the guard
and passes with it. Same for the counter: dropping it from the claim
fails two.
Refs #1110
The last open item from the #1304 review.
/disable overlapping an in-flight /enable was silently lost. While
activation generates its identity and writes its binding file the row
still reads `disabled`, so disable()'s conditional update matched no
rows, and the lease conflict raised by its tick() was swallowed as
expected noise. The admin was told participation was off; the activation
then completed and left it on. An opt-out that does nothing is the one
failure this feature cannot have.
disable() now records cancel_requested first and unconditionally —
before the case-by-case work — and enable() claims its state with a
single conditional UPDATE that tests the flag alongside the status.
Re-reading the flag and then updating would only have moved the window;
making the claim itself carry the condition closes it, so whichever of
the two lands first wins outright and the loser writes nothing.
Nothing is registered when the claim fails, so there is also nothing to
delete remotely — the cancelled activation leaves no identity behind.
The flag is cleared at the start of enable(), so a cancellation from an
earlier participation cannot veto a later deliberate opt-in.
The column is migration 202 rather than an edit to 201. 201 already
shipped on this branch and knex records it as applied, so folding the
column in would have skipped every database that had already run it and
the first /disable would have failed on a missing column. Verified both
ways: a fresh install gets the column from 201+202, and a database
migrated before 202 existed gains it when 202 arrives.
Three tests. With the condition dropped from the claim, the race case
fails and the other two pass.
Refs #1110
It appears on the dashboard and settings only. It is an invitation, not
an alert, so it belongs on pages an admin opens deliberately rather than
on top of whatever task they are in the middle of.
The activity ticker deliberately did NOT move with it. That ticker is
what triggers the daily rollup — the backend has no scheduler — so
tying it to the banner would have stopped reporting for an admin who
works on Events and never opens the dashboard, and stopped it entirely
for a participating install, where the banner never renders at all. The
effect stays mounted on every admin page and only the visible aside is
scoped. Two tests pin exactly that, because it is the kind of thing a
later refactor would helpfully "clean up".
Highlighted like the migration banner it sits under: tinted surface,
border, icon, a title line above the body. It was previously the same
neutral surface as the page behind it and read as filler.
"Not now" is now "Ignore". The button calls dismiss(), which persists
notice_dismissed on the server — the invitation never comes back. "Not
now" promised otherwise. The label says what happens and a hint says
where to join later.
Only shown while participation is off. activation_pending,
deletion_pending and identity_conflict are in-flight states the settings
page explains properly; inviting someone to join in the middle of their
own withdrawal would be worse than saying nothing.
The first version of these tests was worthless: the negative cases
asserted absence after waiting only for the status call, so the
component was still rendering null for want of data and every one passed
with the gates removed. They now wait for the query cache to fill.
Removing the route gate fails 4; removing the status gate fails 6.
Refs #1110
Three of four findings from the follow-up review.
A withdrawal is no longer clobbered by the instance-copy check. That
update was unconditional, so an opt-out arriving while the binding
lookup was in flight was replaced by identity_conflict — and tick()
stops there, so the deletion the operator asked for was never sent. It
now carries the same whereNot('deletion_pending') guard the
collector-conflict handler beside it already had.
Scheduled database backups count as a configured backup. The middleware
records /backup/* and /database-backup/* under one capability, but
`configured` read only backup_enabled, so an install whose only backup
is the scheduled database one reported used: true, configured: false —
a contradiction in the dataset this feature exists to produce.
SIGNING_KEY_UNREADABLE gets its own message. Naming the error in the
previous commit was half the job: the settings page still showed the
generic retry/disable advice, and neither action can succeed without the
original encryption material. It now says what happened and what is
actually required, in EN and DE.
NOT fixed, and reported instead: /disable overlapping an in-flight
/enable. While activation is still doing its slow work the row still
reads `disabled`, so disable's conditional update matches nothing and
the lease conflict from its tick() is swallowed — the operator is told
participation is off while activation completes and leaves it on.
Closing it properly needs a persisted cancellation flag that enable
checks before finalising: taking the lease cannot help, since it either
conflicts immediately or would block the request for the 60s lease. That
is a schema and state-machine decision for the author, not something to
restructure underneath them.
Refs #1110
Review follow-ups on #1304.
SIGNING_KEY_UNREADABLE. USAGE_ENCRYPTION_KEY defaults to JWT_SECRET, so
rotating JWT_SECRET — the correct response to a suspected compromise —
makes the stored Ed25519 key undecryptable. That surfaced as a generic
DELIVERY_FAILED which retried forever, and it silently blocks the DELETE
packet too: an operator who withdraws has their local state cleared
while the collector keeps its copy. decrypt() now tags its own failure
and deliver() reports it under its own name, without flagging an
identity conflict — an unreadable key is not evidence of a clone. The
docs already warned that losing the key breaks deletion signing; they
now name the trigger and the error.
The collector default is no longer an inline string in the constructor.
It is a declared DEFAULT_COLLECTOR_URL, since it is a deployment choice:
self-hosters point USAGE_COLLECTOR_URL at their own collector and the UI
already derives every link from whatever is configured. schema.cjs is
deliberately untouched — it is vendored byte-identical with
picpeak-usage, and its $id is a schema identity, not a delivery address.
Links in the consent dialog. It named the collector inside prose but
never linked it, so an operator deciding whether to opt in could not
open the destination or the public schema without retyping a URL. Both
are links now, built from the configured collector.
UI standards. The tab hand-rolled its surfaces as
`<section className="rounded-xl border border-theme …">` and imported
Button from a deep path; every other settings tab uses `<Card
padding="md">` from the components/common barrel. Converted, with the
feedback <form> wrapped rather than replaced so its semantics survive,
and headings given the same colour tokens as ImageSecurityTab. The
barrel pulls ErrorBoundary -> i18n/config, so the tab's test needed the
initReactI18next shim the FaceRecognitionCard test already uses.
Not changed: the delete packet reusing the current sequence. The
collector handles delete before any sequence check — "possession proof
is sufficient for deletion, including when a restored backup has a
stale sequence" (picpeak-usage server/collector.js) — so deletion is
deliberately sequence-exempt and the client is correct as written.
Refs #1110
Fifth bypass, and the same root cause as the first: sanitizeCSS
validated, then kept rewriting.
`<[^>]*>` deletes the span it matches, and `<">` takes a quote with it.
So `--x:x<">;background:url(https://evil.example/p.gif);--y:x<">` was
scanned with the url() safely inside a string, and the tag strip below
then removed the quotes that made it so — shipping a live remote
background with no warning.
The file already carried the rule: "any pass that can join tokens has to
happen before validation, not after." It has now been broken three
separate times — by the HTML-comment strip (#1290), the control-
character strip, and the tag strip. Rather than fix a third instance in
place, the URL scan is now the LAST step, so what is validated is always
the bytes that get served.
All eight known bypass classes are pinned, together with the legitimate
data: URI, quoted font stack and escaped selector that must survive
untouched.
Refs #1264
The fourth bypass found in this review, and the one no lexer fix
reaches: sanitizing runs on the stored body, but safeTemplateReplace
rewrites it afterwards, so the string that was validated is not the
string that is sent.
A conditional inside a style attribute can delete the very quoting that
made a url() inert:
style="--x:x{{#if company_name}}'{{/if}};background:url(https://evil…)"
At write time the url() genuinely sits inside a CSS string and is
correctly left alone. Expanding the conditional for a recipient with no
company name removes both quotes and the background goes live —
confirmed end to end against the real functions.
The style-attribute pass now runs again on the substituted output.
Substitution cannot introduce a `"` (values are HTML-escaped), so the
attribute match still holds. body_css is not substituted, so the
<style> block cannot be rewritten after its check and needs nothing.
This is the case the removed newsletter pass had been covering. Rather
than reinstating a second definition of "disallowed", the one definition
now runs at both points where the content changes.
Refs #1264
Third bypass of this scanner found in one review pass, and the same
shape as the others: the lexer and a browser disagreeing about where a
token begins.
JavaScript's `\s` matches U+00A0; CSS whitespace is exactly space, tab,
LF, CR and FF. Skipping an NBSP as whitespace let the scanner read the
quote after it as a legitimate quoted data: URI and swallow a remote
url() inside that "string" —
.a{background:url(<NBSP>"data:image/png);background:url(https://evil…);--x:");}
came through untouched, with no warning, and survived re-sanitising. A
browser treats NBSP as an ordinary character, so that is an UNQUOTED
url-token ending at the first `)`, leaving the remote background live.
All three token readers now use an explicit CSS whitespace class.
Ordinary spacing around a data: URI still works, and is pinned.
Refs #1264
My previous commit introduced this. Handling `\` outside strings before
readIdentifier meant a LEADING escape was eaten before the url check
saw it: `\75` is the CSS escape for `u`, so `.a{background:\75rl(...)}`
is url() to a browser and passed through untouched, with no warning —
a bypass the base version did not have. An escape mid-identifier
(`u\72l`) was unaffected, which is why the first tests missed it.
The escape branch now runs AFTER readIdentifier, which already decodes
leading escapes itself. What is left for it is the case it was added
for: `\'`, which must not be read as opening a string.
Both spellings are pinned, along with the legitimate escaped selector
and data: URI that must survive untouched.
Refs #1264
Two review findings, both about the warning saying things that are not
true.
The duration contradicted the rate beside it. `rate` came from the live
draft while `minutes` came from the resolution the server computed from
the SAVED rate, so any unsaved edit produced a mismatched pair: 120
recipients switched from 10/min to 1/min still claimed 12 minutes rather
than 120. Recomputed from the rate the send will actually use — queueing
persists the draft first — with the server's own formula and the same
1..10 clamp, so the two numbers cannot disagree.
The queue claim was overstated. "other email queues behind it" is only
true when the campaign saturates the queue: scheduled_at is staggered
and processEmailQueue excludes future rows before taking its batch, so a
campaign paced below the 10/minute ceiling leaves capacity for a
password reset on the next tick. Now says other email CAN be delayed
behind it, which is what the shared queue actually guarantees.
The new test fails against the previous pairing.
Both found by review against the correct base, and both are cases the
second stripRemoteCssUrls pass had been catching before this PR removed
it. Verified against the real functions before and after.
An escaped quote outside a string. `\'` is an escaped identifier
character, not a string opener, but the scanner stepped onto the
apostrophe, entered string mode and copied the rest of the stylesheet
unexamined — so `.hero{--marker:\';background:url(https://evil/p.gif)}`
kept a live remote URL. Escapes are now consumed as a unit outside
strings.
An unterminated quote. Trusting one meant a single stray apostrophe
disabled scanning for everything after it. An unclosed quote is a parse
error, so the safe reading is to emit it as an ordinary character and
keep scanning; a newline also ends a string, as it does in CSS.
The entity mismatch behind the second case. sanitize-html writes `"`
inside an attribute as `"`, so the scanner and the recipient's
browser disagreed about where strings begin: in
`style="font-family:"don't";background:url(...)"` the browser
decodes first, reads the apostrophe as ordinary text inside a real
string, and fetches the background — a tracking pixel by another name.
Style attributes are now decoded before scanning and re-encoded after,
which also stops the old code silently deleting quotes from the value.
Also detaches the image handlers before releasing the canvas source.
That one did NOT reproduce: measured in both Chromium and WebKit,
neither fires `error` when the attribute is removed after a successful
load. Applied anyway because the ordering is free and the failure it
would cause is silent — canvasFailed set, the canvas swapped for an
<img>, and the image decoded a second time, the exact opposite of what
the release is for.
Refs #1264, #1287
Closes#1300.
Fragmentation was configurable, stored per event, served to the gallery
client, and consumed by nothing. It was not unbuilt scaffolding — both
halves exist and are individually coherent — but they were never
connected, and they disagree: the server cut a fixed 3x3 grid while the
client reassembled a 4x4 one, so wiring them together as they stood
would have produced scrambled images rather than protection.
Removed rather than finished, because finishing it buys nothing. The
client fetches the whole image and then redraws it in pieces on a
canvas, so the full original has already crossed the wire before any
"protection" is applied — that is obfuscation, not a control. The
per-fragment canvas work also lands on mobile, which is the memory
profile under investigation in #1287.
Goes: secureImageService.fragmentImageBuffer and its branch, the
?fragment=N delivery path and handleFragmentedImage in secureImages,
the fragmented-JSON response in protectedImages, fragmentation_level in
the gallery payload, the default_fragmentation_level setting, the PUT
validator, the ProtectedImage fragment renderer, and the operator
control with its strings in all eight locales.
No migration. `events.fragmentation_level` and the app_settings row stay
— dropping a column is irreversible and the stored values are harmless
once nothing reads them. If they should go, that is a deliberate data
decision and its own migration.
`fragmentGrid` on AuthenticatedImage and the layouts is deliberately
untouched: #1299 already removes it as part of the inert prop surface,
and doing it here would only collide.
Replaces the six per-field .not().isArray() guards from the previous
commit. Those were too narrow, and arbitrarily so.
PUT /:id spreads req.body into `updates` (crud.js:1631) and passes it to
.update() (:1990) with only targeted deletes in between — there is no
column allow-list. express-validator applies isInt/isIn/isBoolean
element-wise to arrays, so a single-element array satisfies its field
validator and survives the whole way to the column. That is true of all
44 validated fields, not of the protection block I happened to be
looking at; seven of them also run through formatBoolean, where [false]
reads as true.
So the guard belongs where the body is spread, not on chosen fields.
`customer_account_ids` is the only field legitimately an array — it has
an isArray() validator and its own element rules — and it is deleted
from `updates` before the write, so exempting it costs nothing.
Tested across the protection fields and two outside that block, plus the
customer_account_ids exemption. With the guard's condition disabled,
exactly those six array cases fail and the other 15 in the suite pass.
Refs #1296
The composer already paces sends — the rate is clamped to what the queue
can actually drain and `scheduled_at` is staggered — and the help text
said so. But pacing answers the wrong risk. Spam filtering reacts to a
domain's volume and reputation, not to the interval between messages, so
a throttled send of several hundred near-identical mails from a domain
that normally emits only gallery notifications is exactly the shape that
gets junked or blocked. Nothing told the operator that.
Adds a warning above the queue button once the resolved recipient count
reaches 50, covering what actually goes wrong: the reputation hit, that
it damages delivery of transactional email too, the SPF/DKIM/DMARC
prerequisite, and the suggestion to split a first campaign.
It also states the cost of the send in the operator's terms — the real
duration at the chosen rate, and that gallery invitations and password
resets queue behind it, because the email queue is global rather than
per-campaign.
50 is deliberately low: the operators who most need this are the ones
sending their first campaign.
The existing rate hint now mentions looking like spam, not only being
rate-limited.
PUT /:id has the same weakness the create chains just had:
express-validator runs isIn/isBoolean/isInt element-wise, so
`image_quality: [72]` satisfies every check and stays an array. This
handler spreads req.body straight into the update, so the array reached
a scalar column — a PG insert error, and `[false]` read as true.
Covers all six fields in that block, not only the four this PR is about.
enable_devtools_protection and overlay_protection sit in the same list
with the identical flaw, and leaving two known holes next to four closed
ones would have been the odd choice.
Refs #1296
Round-four review follow-ups.
Every reader of app_settings now shares decodeSettingValue. The previous
commit taught the GET handler to decode, which on a legacy SQLite install
made the tab show devtools protection as disabled while
readBooleanSetting — parsing once, getting the string 'false', rejecting
it — left new galleries with it enabled. A decoder used by only some
readers is worse than none, because the UI and the behaviour disagree.
readBooleanSetting, getImageSecurityDefaults, the v1 devtools fallback
and the settings GET all use it now.
Standalone contract conversion covered. contract/conversions.js takes
Path B and inserts its own event row when the contract has no source
quote, so signed standalone contracts were the last path still landing
on the migration-038 column defaults.
Refs #1296
Round-three review follow-ups.
getImageSecurityDefaults now accepts a transaction, the way getAppSetting
two lines above it already does. quoteService.convertToEvent called it
from inside db.transaction() through the global db; sqlite3 runs a
single-connection pool, so that read would have waited on the connection
its own transaction was holding until the acquire timeout, and the
helper's catch would then have swallowed the error and dropped the
defaults silently.
The double-encoding is fixed where it starts. GET
/admin/image-security/settings returned setting_value undecoded, so it
shipped "true" to a tab that types the field as boolean — and since the
tab PUTs the whole object back through JSON.stringify, every save
wrapped another layer around values nobody edited. It decodes now, so a
round trip is idempotent. The tab is the only consumer of that endpoint.
The reader unwraps to any depth instead of four. The depth on an
existing install is however many times someone opened that tab, which is
not a number to cap. It terminates because each parse of a string is
strictly shorter than its input.
Refs #1296
Round-two review follow-ups.
Settings survive the tab round trip. GET returns setting_value without
decoding it and ImageSecurityTab PUTs the whole fetched object back
through JSON.stringify, so on SQLite one visit to the tab re-encodes
every value it read. A single parse then yields the string "true", the
type checks reject it, and the defaults go quietly dead — the exact bug
this change exists to fix, returning by a different route. The reader
now unwraps until the value stops being a JSON string, bounded.
Array overrides rejected. express-validator applies isInt/isIn/isBoolean
element-wise, so `image_quality: [72]` passed the chain and arrived
still an array — a PG insert error, and `[false]` coerced to true by
formatBoolean. Both create routes now use .not().isArray(), and the
shared resolver ignores non-scalars for any future caller.
Two more creation paths covered. quoteService.convertToEvent builds its
own events row, so CRM-converted galleries fell back to column defaults.
/:id/duplicate copies fifteen source columns including
enable_devtools_protection but missed these four, so duplicating a
'maximum' gallery produced a 'standard' one — a duplicate now inherits
the source's values, not the current globals, since copying the gallery
is the point.
The PUT /:id chain has the same array weakness. Pre-existing and outside
this fix; left alone deliberately.
Refs #1296
Review follow-up on the sanitizer dedup.
sanitizeCSS already carried the rule — "any pass that can join tokens has
to happen before validation, not after" — written above the URL scan to
explain why it runs after the HTML-comment strip. The control-character
strip is exactly such a pass and sat eleven lines below it.
So `u<CTRL>rl(https://tracker.example/p.gif)` was scanned as clean, and
the strip below then joined it into a live remote request with no
warning. Newlines are control characters here too, so `u\nrl(...)` did
it without an exotic byte. Verified against the real function before and
after: all five variants returned a live remote url() and now return
`none` plus the blocked-URL warning.
This PR is what exposed it. Dropping newsletterService's second
stripRemoteCssUrls pass was right — the duplicate hid a defect in the
shared sanitizer rather than fixing it — but it removed the belt that
was catching this for the newsletter path. Fixing the ordering fixes it
for every caller instead of restoring the second pass.
Refs #1264
Review follow-up. The release only ran from the effect cleanup, so it
fired on unmount or a src change — while the commit message and the test
header both explained that grid tiles never unmount, which is the whole
reason the decode piles up. For the case the change exists for, it never
ran at all.
drawToCanvas now reports whether it drew, and the source Image is
released as soon as the pixels are on the canvas. Nothing redraws from
imageRef afterwards; drawToCanvas has exactly one caller. The cleanup
stays as the fallback for the paths onload cannot cover: the draw
failed, or the source changed before onload fired.
The new test pins release while still mounted, on the same src. It fails
against the previous version.
Refs #1287
Review follow-ups on the #1296 fix.
The defaults were resolved only in the admin POST / handler. POST
/api/v1/events builds its own insert and resolved just the devtools
setting, so an API-created gallery still fell back to the column
defaults — the same split that made #592 a separate bug from #317, about
to be repeated. Both paths now share resolveImageSecurityColumns().
An explicitly supplied value now wins over the global default. The
create routes never accepted these four fields at all, though PUT /:id
has validated them all along, so a client sending protection_level on
create had it silently dropped. The previous comment claimed the spread
ordering preserved a request value; there was no request value to
preserve, and a later spread would have overridden one anyway.
Settings validation no longer leans on parseInt, which rescues '72oops',
72.5 and [72] into valid-looking integers. The settings PUT stores
whatever JSON it is handed without validating values, so those really
can reach the resolver.
fragmentation_level is still stored and consumed by no renderer —
ProtectedImage hardcodes a 4-grid and secureImageService a 3x3. Noted in
the API docs rather than silently implied to work.
Refs #1296
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
Four controls in Settings → Image security were written, reloaded and
rendered as toggles, and read by nothing:
default_protection_level → events.protection_level
default_image_quality → events.image_quality
enable_canvas_rendering → events.use_canvas_rendering
default_fragmentation_level → events.fragmentation_level
Each maps onto a column migration 038 already created, and each is
labelled "… by default". `enable_devtools_protection` was the only one of
the five ever wired (#317), and its plumbing is the pattern this follows.
Reported for enable_canvas_rendering by @leonlivevocalist-svg while
instrumenting #1287 — the setting was globally true on their install and
zero canvas elements were created. Checking the neighbours found three
more of the same, so fixing one and leaving three would have been worse
than leaving all four.
CREATION-TIME ONLY, deliberately. Applying these to existing events would
silently change live galleries on upgrade: an install with
enable_canvas_rendering already on would flip every grid to canvas
rendering, which is memory-expensive at scale and is the exact profile
under investigation in #1287. New events inherit; existing rows are
untouched.
A missing or malformed value yields no key, so creation falls through to
the column default exactly as before — including the ranges, where an
out-of-range quality or fragmentation level is ignored rather than
clamped into something the operator did not choose. `false` is carried
through rather than dropped as falsy, or "off" would be unreachable.
The spread sits after the explicit columns so a value supplied by the
request still wins.
12 tests: the mapping, the false case, seven malformed inputs falling
through, partial configuration, and that a settings failure cannot block
event creation.
Grid was the only layout passing `lazy` without an `inViewRootMargin`, so
PhotoCard ran its observer at the IntersectionObserver default of `0px`
with `threshold: 0.1`. A tile could not begin loading until a tenth of it
was already on screen — there was no lead at all.
The gallery owner's account of the symptom is that defect's exact shape:
spinning the scroll wheel outran loading by roughly 50 images, then it
caught up. Outrun-then-recover is what a zero-width pre-load band looks
like from a chair.
This is the one thing in that investigation that does not rest on the
reporter's instrumented runs, which they have since withdrawn after
finding their automation harness ran in a hidden pane — `innerHeight: 0`,
so nothing could intersect and no tile could ever load. The missing
margin is visible in the source regardless.
Percent, not vh. `rootMargin` accepts only px and percentages, and a `vh`
value throws SyntaxError at construction, which would have taken down
every Grid gallery. Verified in Chrome:
'100% 0px' → accepted
'100px 0px' → accepted
'100vh 0px' → SyntaxError: rootMargin must be specified in pixels or percent
A percentage resolves against the root's own box, so 100% is one viewport
height of lead in each direction — viewport-relative, which a fixed 100px
like Justified's is not. A phone and a 4K desktop scroll past very
different amounts of grid per gesture.
Deliberately NOT included: a sweep for cards left un-loaded after
scrolling settles. That was aimed at permanent loss from `triggerOnce`,
and the owner's observation that tiles do come back on desktop argues
against it. Complexity chasing a symptom nobody has reproduced outside a
broken harness.
Three guard tests, including one on the unit, since the failure mode of
getting that wrong is a gallery that does not render at all.
Two follow-ups to yesterday's merges. Both were already known; neither
depends on the open question in #1287.
1. Canvas mode pinned every decoded image for the component's lifetime.
`AuthenticatedImage` keeps a detached Image in `imageRef` so drawToCanvas
can read it. The effect cleanup nulled onload/onerror and never cleared
that ref, so the Image — and the decode behind it — stayed held by a live
JS reference. A decoded <img> in the document is evictable under memory
pressure; one held by a ref is not.
That is not academic at gallery scale. The photo grid is NOT virtualised,
so a 546-photo event mounts 546 of these and none ever unmount — nothing
was ever released. The ref is cleared and the src dropped, so the browser
can reclaim without waiting for GC.
This is NOT presented as the fix for #1287. That investigation is still
open: the reporter has since shown the backend idle during a stall and
the renderer itself unresponsive for 45s, which rules out the theories
tried so far. This is a real leak on the same path, worth fixing on its
own terms while that question is settled.
2. newsletterService no longer carries its own remote-url() stripper.
It was added because the shared sanitizeCSS "blocked" remote URLs with a
CSS comment that parsers discard. #1290 replaced that with a lexer, so
the local copy is dead weight — and two definitions of "disallowed" would
drift apart. Verified the shared function covers every case the local one
did, including the quoted-paren and CSS-escape forms found in review.
Three tests on the release path, two of which fail without the fix:
unmount clears the ref and drops the src, the blob URL is revoked, and a
src change releases the previous image rather than accumulating one
pinned decode per photo a recycled tile has shown.
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
show_feedback_to_guests means "don't show guests OTHER PEOPLE's feedback".
The per-viewer is_liked flag was gated on it anyway, so turning sharing
off emptied every heart the guest had set themselves, on every page load,
while the photo_feedback rows sat there intact.
The query behind the flag is filtered to the viewer (by guest_id, or by
their own IP+UA identifier), so what it returns was never aggregate data.
The colour-label block twelve lines below already documents this exact
reasoning and is correctly ungated.
Scope is just that flag — the counts beside it stay gated, with a test
pinning that the fix does not leak them back. The #1150 contract still
holds: an admin-hidden like does not read as liked.
Note for the reporter: the FILTER path was already correct
(includeGuestMatches is ungated, /my-feedback carries no gate). The empty
Likes chip was downstream of the same falsified flag, not a second bug.
Closes#1286
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).
Keeps the size error first, as before the allow-list landed, and pins the
allow-list gate in the size-limit suite: a .html filename is refused
whatever MIME the client declares.
- the customer contract PDF stream applies assertContractPdfPath like the
admin and public contract routes
- OG previews fall back to the site card for draft, archived and
deactivated galleries instead of leaking name, date and welcome message
- video Range requests are validated before the 206 is written; a NaN,
inverted or out-of-file range now answers 416
- share-token comparisons in gallery resolve/info use the constant-time
helper share-login already used
- middleware/photoAuth.js and the galleryAuth/photoAuth/verifyGalleryAccess
exports of middleware/auth.js were unreferenced since the static mounts
went; the auth.js copy had neither slug binding nor issuer pin, so it is
removed before anyone mounts it
safeValidationErrors moves to utils/routeHelpers and replaces every
res.status(400).json({ errors: errors.array() }) in the routes, so no 400
body carries the submitted value any more (setup, customer auth and
customer change-password were still echoing rejected passwords).
Admin login, gallery verify, customer login/register/reset, customer
change-password and setup now cap username/slug at 255 and passwords at
MAX_PASSWORD_LENGTH at the validator, so an oversized value never reaches
the lockout lookup, bcrypt or the failed-attempt log.
Admin and customer login run one bcrypt compare on every path; the unknown
account branch used to return in microseconds against ~100ms for a wrong
password, which enumerated usernames despite the generic message.
- maintenance mode classified paths case-sensitively while Express routes
case-insensitively, so /API/... walked past the gate
- the general rate limiter skipped anyone holding any verified JWT; a
gallery token is minted for free on password-less galleries and slideshow
links, so that was an unlimited budget for every /api route. Only admin
sessions skip now
- ?admin_preview=1 trusted a verified signature alone; it now applies the
same revocation, restore-cutoff, deactivation and password-change checks
adminAuth does, and reveal-mode reads the verified flag instead of
re-decoding the token
- the 50mb JSON limit is scoped to /api/admin and /api/v1; everything else
gets 2mb, so an unauthenticated body can no longer stall JSON.parse
- the CSRF Content-Type gate accepted multipart from any origin; cross-site
form posts are now rejected via Sec-Fetch-Site / Origin, with a Host match
fallback for same-origin installs that leave FRONTEND_URL unset
chunked-upload/init stored the client-declared mimeType on the photo row and
the gallery, secure-image and protected-image routes echoed it as
Content-Type, so a JPEG/HTML polyglot declared as text/html rendered inline
on the app origin for every guest. The admin photo route already resolved
the type safely (#908 review); that logic now lives in
utils/photoContentType and every serving route uses it.
The chunked path derives the MIME from the filename extension and requires
that extension to be on the admin allow-list, matching what the multipart
path enforces through its multer fileFilter.
Settings > Branding persisted logo_url / favicon_url verbatim and on clear
unlinked path.join(storage, url) behind a startsWith('/uploads/logos/')
check, which '..' segments pass. The business-profile PDF logo did the same
behind a /pdf-logo-\d+\./ marker test, and used absolute values as given.
Either let a settings.edit or settings.banking holder delete any file the
process can reach.
Both now resolve through helpers in utils/safePath that only ever name a
flat leaf inside the fixed directory. The /favicon.ico streamer is narrowed
the same way: it contained to the whole uploads/ root, which also holds
signed contracts and transfer files.
revokeToken() base64-decoded the payload without checking the signature and
inserted a row keyed on id-iat-type, the same key isTokenRevoked() matches
for real sessions. The logout endpoints are unauthenticated, so anyone could
forge a payload naming another user's id, type and login second and log them
out remotely; a far-future exp also left rows that cleanup never swept.
Expiry is still ignored so logging out an expired session stays idempotent.
Codex review round 2. The 400 I added in the previous commit returned
errors.array() verbatim, and express-validator puts the submitted `value` in
each error -- so rejecting an oversized password echoed that password back, and
re-allocated up to the 50mb body limit on an unauthenticated endpoint, partly
undoing the denial-of-service fix this branch exists for.
The same call appeared at seven sites in this file, five of which validate a
password field: /admin/login, /gallery/verify, /gallery/:slug/client-login,
/admin/change-password and /password-strength. Every failed login was returning
the attempted password in its response body, where it reaches proxy logs, error
monitoring and browser tooling. Fixed at all seven rather than only the one the
review pointed at.
Only `value` is dropped. `msg`, `path` and the rest are kept, because the two
shapes express-validator produces are both consumed in the frontend -- AcceptInvite
reads {field, message} from routeHelpers.validateRequest, EventDetails reads
{msg, path} from raw errors.array() -- and switching auth.js to the helper's
shape would have broken the latter for a reason unrelated to security.
1 more test. Backend suite: 2744 passed.
Paul Nothaft
