Probing the live collector to settle the delete-sequence question turned up
something else: usage.picpeak.app answers a valid usage.v2 registration with
INVALID_PACKET while the identical v1 flow is accepted. It does not speak v2
yet — which the deployment notes already require, but the consequence of
getting that order wrong was worse than "reports do not send".
Opting in to v2 against a v1-only collector left the installation stuck.
Registration was refused, so nothing existed at the collector at all; the row
sat in activation_pending, disable moved it to deletion_pending, retry was
futile forever, and enable refused because the row was not `disabled`. The
abandon hatch added earlier did not apply: it was gated on
SIGNING_KEY_UNREADABLE. So the most harmless possible failure — nothing
registered anywhere — was the one an operator could not clear.
The gate is now the property that actually matters: a participation the
collector has provably never accepted (sequence 0, no receipt) with a failing
delivery can be discarded, from activation_pending as well as
deletion_pending. Its receipt records `never-registered` rather than an
unconfirmed deletion, because nothing remote exists to be unsure about. A
participation the collector *did* accept keeps the old narrow gate and its
explicit warning — clearing local state while the collector still holds
reports must stay a deliberate, warned-about act.
A collector that rejects a registration or a deletion outright now reports
SCHEMA_NOT_ACCEPTED instead of DELIVERY_FAILED, and the settings page says the
collector does not accept this report version yet. Retrying cannot fix that,
and sending the operator to look for a network fault they do not have was
wrong.
Verified end to end against the live collector: v2 opt-in reports
SCHEMA_NOT_ACCEPTED, the exit is offered immediately, the receipt says
never-registered, and joining again on v1 registers, reports and withdraws
with a collector-confirmed deletion.
The dialog described what is sent and where it goes, but never said which way
the connection runs. That is the part an operator is actually being asked to
accept: opening an outbound path to someone else's service.
PicPeak sends and never pulls. One place in the service reaches the network,
it is a POST, and it requests exactly two paths — /api/envelopes, and
/api/participant/lookup only when an operator asks for their own export. No
scheduled job contacts the collector; the daily rollup is driven solely by an
authenticated admin hitting /activity. There is no route the collector could
call, and redirect: 'error' means it cannot even point a request somewhere
else. From a reply only the acknowledgement for the packet just sent is read,
with every field compared against that packet before it is accepted; the
stored copy drops the session token and no read path hands it back to the UI.
A requested export is streamed to the operator as a file and never
interpreted.
The consequence is why it belongs in the consent text and not only in the
docs: this channel cannot deliver code, configuration or content into an
installation, not even from a collector that has been taken over. It is a
security property by design rather than by convention.
usageOutboundOnly.test.js guards it by source inspection rather than
behaviour, because a behavioural test only proves that today's calls behave.
It fails the moment someone adds a second fetch, a poll for messages, a
scheduled pull, or a public route touching the usage service — verified by
injecting each of those.
A QA exploration of this branch against an isolated rig — own stub
collector, SQLite and PostgreSQL — turned up one dead end and a set of
signals and controls that did not hold up. This closes all of them.
Rotating JWT_SECRET, the documented response to a suspected compromise,
made the signing key unreadable. That was already named and documented,
but it left no way out: the delete packet can never be signed, so the
row stays deletion_pending forever, and enable() refuses because it is
not `disabled`. An operator who rotated precisely because the secret was
compromised cannot restore it, so the feature was bricked with no
control left. POST /usage/abandon is offered only in that state; it
drops the local identity and records the receipt as
`collector-unconfirmed` rather than claiming a deletion that did not
happen.
Every failed delivery was retried on the next admin request, and
/activity is open to any authenticated admin while the settings ticker
fires it every five minutes per open tab — 30 activity calls against a
rejecting collector produced 30 outbound requests. Migration 206 adds
attempts/next_attempt_at and the unattended sender honours the gate;
Retry and opt-out still send immediately, and the tab names the time of
the next automatic attempt.
Feedback, votes and portal sessions now share an installation-wide
budget of 30/hour. They are the only endpoints whose effect is outbound
traffic carrying operator-written free text, and the general limiter
skips authenticated requests by design. Reading status and withdrawing
stay unthrottled.
gallery_image_protection was true on a bare install with no galleries:
PicPeak ships default_protection_level='standard' and
enable_devtools_protection=true, so it reported fleet-wide 100% and
could never separate a decision from an untouched default. It now reads
only what deviates from the shipped defaults, and the devtools flag is
not read at all — being on by default, its only informative state is
off, which is the opposite of what the key claims.
Also:
- the export receipt counted every packet and called the total "usage
reports"; reports and participant operations are now counted and named
separately
- GET /usage/preview no longer persists the custom_css marker, so the
transparency view stops changing what will be sent
- the feedback route requires every field the packet schema requires,
so an API caller gets the missing field named instead of a bare
INVALID_PACKET from inside signing
- the German strings for this feature use "Sie" throughout, matching the
rest of the admin UI; the ignore hint says what ignoring will do
rather than stating it as already true
- the consent dialog returns focus to the control that opened it
- the long buttons wrap instead of running off a 390px viewport
- a deletion receipt is labelled as belonging to an earlier
participation while a new one is active
Regression tests cover each of these, including the delete packet's
reuse of the last accepted sequence, which was an unwritten assumption
about the collector rather than a defect.
#1298 and #1303 merged together. #1298 taught the creation paths to
resolve a fragmentation_level default; #1303 removed everything that
consumed it. Neither conflicted textually, so main ended up validating
the field on create and update, copying it on duplicate, resolving
default_fragmentation_level for it, and advertising it in the v1 API
docs — for a value nothing reads and a setting the Image Security tab no
longer exposes.
Inert rather than broken, which is exactly why it needed removing on
purpose: dead code that contradicts the PR that just deleted the feature
is how the next reader concludes fragmentation still works.
The events.fragmentation_level column and the app_settings row stay, as
#1303 decided — dropping a column is irreversible and the stored values
are harmless once nothing reads them.
Refs #1300
Three findings, one of them mine and CI-affecting.
The Postgres suite gets its own schema. CI hands every gated suite the
same PICPEAK_PG_TEST_URL and runs jest with parallel workers, and both
picpeakRestorePg and externalRelpathFoldPg drop and recreate `events`
and `app_settings` in it — so the suite I added would have destroyed
their fixtures and vice versa, intermittently. It now creates and drops
its own `usage_pg_test` schema and reaches the tables through
searchPath, which works because the service queries unqualified names.
Verified on a clean database: after the run `public` still holds zero
tables. My first attempt at this silently did not apply — the
replacement anchor had been reformatted by eslint and I printed success
without asserting the match, which is why the first "isolated" claim was
wrong.
Webhook-only installs are no longer counted as SMTP users. With
EMAIL_WEBHOOK_URL and EMAIL_WEBHOOK_SECRET set, adminEmail sends
/email/test through the webhook transport and never touches SMTP (#1225
added that path), but the rule recorded the permanent `smtp` marker
anyway. Gated on the transport that is actually configured.
Activation is written atomically with its acknowledgement. Split across
two updates, a failure or a stop between them left the row
activation_pending with pending_packet already cleared — registered with
the collector, and permanently stuck locally, because tick() has nothing
to retry from there. The register case now sets status in the same write
and is guarded precisely on activation_pending rather than merely "not
withdrawing".
Refs #1110
Two findings from the review of the current head.
Local backups no longer imply S3. markUsed derived an s3_storage marker
from "a backup ran while backup_destination_type is s3" — but the
middleware also counts /database-backup/* and /backup/picpeak/export as
backups, and those write a local file wherever scheduled backups go. So
configuring S3 and downloading a local export reported s3_storage as
USED. The middleware now tells markUsed whether the operation writes to
the configured destination, and only then is the marker derived. A wrong
`true` in this dataset is worse than a missing signal: it is a claim
about an install that nobody can check.
The ProtocolError branch was dead code. adminUsage matched on
`error.name === 'ProtocolError'`, but the class extends Error without
setting `name`, so every instance reports 'Error' — verified — and a
malformed vote or feedback payload fell through to the global handler,
which logs it as an unhandled programming error and answers
INTERNAL_ERROR in production, losing the validation code the caller
needs. Now matched with instanceof. protocol.cjs is byte-identical with
picpeak-usage (diffed against the companion repo), so the fix belongs
here rather than in the class.
An existing assertion needed updating for the new markUsed argument, and
the path split is pinned: /backup/run is destination-driven,
/database-backup/backup and /backup/picpeak/export are not.
Refs #1110
Everything about this feature had been exercised on SQLite only, which is
the engine least likely to show its problems.
Adds __tests__/integration/productUsagePg.test.js, following the gated
pattern the .picpeak restore suites use: it runs the real migrations
201-203 against a real PostgreSQL and covers what SQLite cannot answer.
node-postgres returns bigint as a STRING, and the withdrawal guard
compares `cancel_seq` — a `'1' !== 1` slip there would let an activation
complete after an opt-out, and SQLite, which hands back a number, would
never show it. Booleans are real booleans rather than 0/1, which is what
every `configured` signal in a report is built from. And markUsed takes
SELECT ... FOR UPDATE on this engine only.
Seven cases, all passing against PostgreSQL 15. Removing the
compare-and-swap condition fails the withdrawal case there too, so the
suite has teeth on that engine and not only on SQLite. CI already
provides PICPEAK_PG_TEST_URL, so these run there rather than skipping.
The collector default is harder to lose. An unset, empty or
whitespace-only USAGE_COLLECTOR_URL now falls back to
https://usage.picpeak.app — deployments that template the variable in
(docker-compose writes ${USAGE_COLLECTOR_URL:-...}) can hand over an
empty string, and that has to mean "use the default" rather than "no
collector". A value that is present but malformed is still reported as a
configuration error instead of being silently replaced: quietly
retargeting a self-hoster's collector at ours would send their reports
somewhere they did not choose.
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
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
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
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-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
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.
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.
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)
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.
Paul Nothaft
