5db0a76cce94de03f86295ba2bd6ba526661d16d
386
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Paul Nothaft
|
32d745b575 |
fix(usage): stop local backups implying S3 use, and make the protocol-error branch reachable
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 |
||
|
Paul Nothaft
|
c7cedb00d6 |
test(usage): prove product usage works on PostgreSQL, and harden the collector default
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 |
||
|
Paul Nothaft
|
bb76ca5375 |
fix(usage): keep the settings tab usable on a bad collector URL, and report layouts and CSS accurately
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 |
||
|
Paul Nothaft
|
9785b636a9 |
fix(usage): take the withdrawal baseline before the lease, not after it
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 |
||
|
Paul Nothaft
|
22da018e1b |
fix(usage): close the remaining withdrawal races, reset per-item name consent
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 |
||
|
Paul Nothaft
|
80e238f0ad |
fix(usage): let a withdrawal win against an activation that is still starting
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 |
||
|
Paul Nothaft
|
c043897b0e |
fix(usage): name the unreadable-key failure, unpin the collector default, align the tab
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 |
||
|
Paul Nothaft
|
1151e96144 |
fix(security): validate CSS urls last, after every pass that moves text
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 |
||
|
Paul Nothaft
|
027afb6086 |
fix(security): re-check inline CSS after template substitution
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
|
||
|
Paul Nothaft
|
4196e83a5f |
fix(security): use CSS whitespace, not JavaScript's, in the url() reader
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
|
||
|
Paul Nothaft
|
b6dc0991ce |
fix(security): check for an escaped identifier before consuming the escape
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
|
||
|
Paul Nothaft
|
1cf82746b7 |
fix(security): close two CSS url() bypasses the sanitizer dedup exposed
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
|
||
|
Paul Nothaft
|
b53e5d97b4 | feat: add opt-in product usage and feedback integration (#1110) | ||
|
Paul Nothaft
|
933f2d8e0e |
fix(security): reject array values for every field on the event update
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 |
||
|
Paul Nothaft
|
0deef2584f |
fix(security): one settings decoder, and the last creation path
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 |
||
|
Paul Nothaft
|
0e560ebb19 |
fix(security): decode settings at the API boundary and honour the transaction
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 |
||
|
Paul Nothaft
|
19c518aaa5 |
fix(security): close the remaining image-security default gaps
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 |
||
|
Paul Nothaft
|
99f54a3954 |
fix(security): strip control characters before scanning CSS for url()
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 |
||
|
Paul Nothaft
|
ab6c33d9eb |
fix(security): apply image-security defaults on every creation path
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 |
||
|
Paul Nothaft
|
8ca3610514 |
fix(security): apply the Image-security defaults instead of storing them (#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. |
||
|
Paul Nothaft
|
fc595409b4 |
feat(crm): newsletter campaigns behind a newsletters flag (#1264)
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 |
||
|
Paul Nothaft
|
a7d0972b13 |
fix(security): make the CSS sanitizer's remote-URL block actually block
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. |
||
|
Paul Nothaft
|
8f98f6bdec |
fix(gallery): show a guest their own likes when feedback sharing is off (#1286)
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 |
||
|
Paul Nothaft
|
b6e40b9a2a |
feat(email): global signature footer from the business profile (#1264)
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)
|
||
|
Paul Nothaft
|
0ac006bb95 |
fix(security): chunked-upload init checks the size cap before the type allow-list
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. |
||
|
Paul Nothaft
|
835312e8e6 |
fix(security): harden four smaller gallery and contract paths, drop the unmounted photo auth middleware
- 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 |
||
|
Paul Nothaft
|
40a8a9882a |
fix(security): stop reflecting submitted values in validation errors everywhere, cap credential lengths, close the login timing oracle
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.
|
||
|
Paul Nothaft
|
839bf4e464 |
fix(security): close four middleware gaps around the API edge
- 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 |
||
|
Paul Nothaft
|
063977d97d |
fix(security): never serve a photo under its stored MIME, and stop trusting the chunked-upload type
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
|
3e46530072 |
fix(security): contain logo, favicon and PDF-logo unlinks to their upload directories
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.
|
||
|
Paul Nothaft
|
0ca0e4a922 |
fix(security): verify the signature before writing a token to the revocation list
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. |
||
|
Paul Nothaft
|
903e471753 |
fix(security): stop reflecting submitted passwords in validation errors
Codex review round 2. The 400 I added in the previous commit returned
errors.array() verbatim, and express-validator puts the submitted `value` in
each error -- so rejecting an oversized password echoed that password back, and
re-allocated up to the 50mb body limit on an unauthenticated endpoint, partly
undoing the denial-of-service fix this branch exists for.
The same call appeared at seven sites in this file, five of which validate a
password field: /admin/login, /gallery/verify, /gallery/:slug/client-login,
/admin/change-password and /password-strength. Every failed login was returning
the attempted password in its response body, where it reaches proxy logs, error
monitoring and browser tooling. Fixed at all seven rather than only the one the
review pointed at.
Only `value` is dropped. `msg`, `path` and the rest are kept, because the two
shapes express-validator produces are both consumed in the frontend -- AcceptInvite
reads {field, message} from routeHelpers.validateRequest, EventDetails reads
{msg, path} from raw errors.array() -- and switching auth.js to the helper's
shape would have broken the latter for a reason unrelated to security.
1 more test. Backend suite: 2744 passed.
|
||
|
Paul Nothaft
|
054cd6f82f |
fix(security): enforce the strength-endpoint validators, and stop the generator spinning
Codex review round 1 on the batch-1 security fixes. One finding is a
regression this branch introduced.
generateSecurePassword retried by recursing on any candidate validatePassword
rejected. The new 128-character cap makes EVERY candidate invalid once a caller
asks for more than that, so `generateSecurePassword({ length: 129 })` went from
returning a password to unbounded recursion and a stack overflow. It now
refuses an impossible length up front, and the retry is a bounded loop rather
than recursion -- every candidate failing is possible for reasons other than
bad luck (a charset that cannot satisfy the configured policy), and that case
deserves an error someone can act on rather than a blown stack. No caller in
the repo passes a length at all; the hazard was in the exported surface.
The route validators were decorative. POST /api/auth/password-strength never
called validationResult(), so the length bound I added only recorded an error
that nothing read: the oversized body still reached zxcvbn and the endpoint
still answered 200. The cap inside validatePassword() was doing all the work.
Errors are now returned as a 400 before the validator runs, which is what the
previous commit claimed.
Also awaited validatePasswordInContext, which is async. Unawaited, `validation`
was a Promise and every field in the response -- valid, score, errors, feedback
-- came back undefined. Pre-existing, in the lines this change already touches,
and it made the endpoint useless for the real-time validation it exists for.
1 more test. Backend suite: 2742 passed. The 23 eslint errors in server.js are
pre-existing and identical on main.
|
||
|
Paul Nothaft
|
14cd5eacb3 |
fix(security): bound password input before zxcvbn, and drop the legacy media mounts
Two findings from the GHSA-pwx6-5pqc-c5xq scan bundle, both verified against
the code and reproduced before fixing.
**Unauthenticated denial of service via password strength (csf_495d53fa).**
POST /api/auth/password-strength takes `body('password').notEmpty()` with no
upper bound, sits behind express.json({ limit: '50mb' }), and hands the string
to zxcvbn, whose matching is superlinear and runs synchronously on the event
loop. Measured on this codebase, in ms of blocked loop: 128 -> 41, 512 -> 1367,
1000 -> 5097, 5000 -> did not return in two minutes. One unauthenticated
request of about a kilobyte stops the whole process for five seconds; a few
kilobytes stops it indefinitely. The /api/auth rate limit does not help when a
single request is already enough.
The cap lives in validatePassword() so it covers every caller, present and
future; the route validator is defence in depth. 128 keeps the worst case at
the cost of an ordinary request while staying far above any real password --
bcrypt consumes only the first 72 bytes, so length past that adds no entropy to
the stored hash anyway. This is the only unauthenticated reach into zxcvbn:
setup is token-gated and self-closing, and acceptInvite/adminAuth use the
regex-only validator in passwordGenerator.
**The /photos and /thumbnails static mounts (csf_9aa6afe6, csf_559cd5cc,
csf_b14d462e, csf_547d26fa, and the gallery half of csf_34e420af).**
They served the raw originals and thumbnail trees behind photoAuth, which
authorises on a slug match. A static file server cannot apply per-photo rules,
so everything the gallery API decides was absent: allow_downloads, per-category
allow_downloads, watermarking, the resolution cap, reveal-mode windows,
visibility='hidden', download logging, and the customer-assignment re-check
that makes revocation immediate. photoAuth also bcrypt-compares an
x-gallery-password header per request with no limiter -- both rate-limit gates
return early for non-/api paths -- so the mount was an unmetered password
oracle. The filenames needed to drive all of this are handed to every guest in
the photos listing.
Nothing builds these URLs: no reference in frontend/src, none in the email
templates, and the only backend mentions are the /api/admin/photos/... API
routes and a maintenance-mode prefix list. The equivalent authorised routes are
/api/gallery/:slug/photo/:id and /thumbnail/:id. nginx still proxies the two
locations; they now 404, which is the intent.
**The /uploads mount (csf_1fc92f57).** It exposed the whole uploads/ root with
no auth middleware at all, and that root also holds signed contract PDFs
(uploads/contracts/signed) and client transfer files (uploads/transfers/<id>),
reachable by anyone who learned or guessed a filename. Narrowed to the two
public asset trees it exists for; contracts and transfers keep their own
authorised routes.
Removing the mounts leaves src/middleware/photoAuth.js unreferenced by
application code. Left in place deliberately -- deleting it and its tests is a
separate cleanup, and a smaller diff backports more safely.
Backend suite: 2742 passed.
|
||
|
Paul Nothaft
|
2d403f7fb2 |
fix(email): wire the settings status card, and cap-aware truncation
Codex review round 4 on #1273. Settings → Status rendered a green check for the email processor unconditionally, against an API field that was itself the literal 'active'. Both ends were lying and only one of them got fixed: adminSystem started reporting the real state in an earlier commit, but StatusTab never read it, so the second place an admin looks to find out why mail is not arriving still said everything was fine. It now shows stopped and degraded, with the reason. The truncation flag missed the case it most needed to cover. The loop broke on the 200-row report cap before the flag could be set, so 201+ overdue rows came back as exactly 200 with scanTruncated false -- a partial report presented as complete. It is now set whenever rows were left unexamined. The grace-window comment claimed the processor clears ~6000 rows inside the window. It clears on the order of 100: ten rows a pass, one pass a minute. The comment now says so, and says why the processor's own state is reported above the list rather than inferred from it -- "running, last pass sent 10" next to a backlog reads very differently from "not running" next to the same backlog. One round-4 finding is NOT fixed, deliberately, and is written up at the retry route. Clearing scheduled_at leaves created_at at the original enqueue time, so a retried old row appears in the waiting list immediately, looking overdue, until the processor sends it. Restarting that clock needs a timestamp written there and no shape works: a Date matches how queueEmail writes the column and how processEmailQueue compares it, but jest's sandbox Dates store as "[object Object]" (CLAUDE.md) so it cannot be tested; an ISO string tests fine but stores as TEXT, which SQLite then orders above the numeric bound in the processor's own pickup query, leaving the row unsendable. A requeued_at column would settle it. Cosmetic either way, and not worth risking a stuck row. 1 more test, failing before this commit. |
||
|
Paul Nothaft
|
4deac229ac |
fix(email): make waiting rows read-only, and time the grace from when due
Codex review round 3 on #1273. The first finding reverses a round-1 fix of mine, correctly. Retry no longer sends. Round 1 flagged that retry was a no-op for waiting rows and offered two remedies: give them a send-now action, or stop showing them Retry. I took the first, and round 3 showed why it is the wrong half -- processEmailQueue claims nothing before invoking the transport, so a flush overlapping the scheduled pass has both of them sending the same email. Saving 60 seconds is not worth a duplicate landing in a customer's inbox, and a claim protocol would need a status no query watches plus a reaper for rows abandoned mid-send. So retry is a reset again, as it was on main. Waiting rows now carry no actions at all, which is the other half of that round-1 remedy and closes a worse hole the shared table opened: Dismiss DELETEs the queue row. Those emails have not failed and still go out once the processor recovers, so clicking the tidy-up icon on a health warning silently cancelled a customer's mail. The section is diagnostic; what a waiting row needs is the processor fixed, which the panel above it now says. The grace window runs from when a row became DUE, not from when it was queued. A split-payment invoice created three days ago and scheduled until a minute ago has had one minute of the processor's attention, and measuring from created_at reported every scheduled mail as unworked the instant it came due -- which is most of what this panel would then have been showing. A truncated scan can no longer read as an all-clear. The scan is bounded, so a queue larger than the budget whose head is all future-scheduled can hide a due row past the last page read; the response now says so and the UI withholds the green check. The test fixtures were wrong in a way worth keeping: scheduled_at also defaults to CURRENT_TIMESTAMP, so back-dating created_at alone built rows that cannot exist in production -- old, but scheduled for the moment the fixture ran. The helper now back-dates both, as the database would have. 3 more tests; the two that pin new behaviour fail before this commit, and the reverted flush is pinned by asserting the transport is NOT invoked. |
||
|
Paul Nothaft
|
98aa06aeff |
fix(email): read naive SQLite timestamps as UTC, and page the candidates
Codex review round 2 on #1273. Both findings restore the false all-clear that round 1 set out to remove, by different routes. Both timestamp columns default to CURRENT_TIMESTAMP, which SQLite renders as a zone-less 'YYYY-MM-DD HH:MM:SS' in UTC -- and Date.parse reads that shape as LOCAL time. On a TZ=America/New_York deployment a row due now looked four hours away and never reached the waiting list; nine hours the other way, fresh mail read as long overdue. The parser now stamps the zone the value actually carries. That parser moved to utils/queueTimestamps so it can be tested honestly. This suite runs in UTC, where reading a zone-less value as local and as UTC give the same answer, and process.env.TZ does not reliably re-bind mid-process -- my first attempt at these tests passed against the broken code for exactly that reason. They now force TZ in a child process, so they fail on any host. The candidate rows are paged rather than cut off with one LIMIT. The time filter runs in JS, so a queue holding more than a page of future-scheduled rows -- split-payment invoices are exactly that shape -- filled the window with rows that all got filtered out and hid the due row behind them, reporting nothing waiting. Paging also drops the dependency on ORDER BY created_at meaning anything, which it does not on SQLite once numeric and text timestamps mix. Bounded at 10k scanned; past that the response is a sample, which the 200-row cap already made it. 12 more tests. The paging one fails before this commit, and all four naive-timestamp ones fail against the old parsing on any host. |
||
|
Paul Nothaft
|
89db469f06 |
fix(email): compare queue timestamps in JS, and make retry actually send
Codex review round 1 on #1273. One of the four is a real bug on every SQLite deployment. The waiting-row query compared `created_at` against a bound ISO string. On SQLite that column does not hold a string: queueEmail writes a JS Date and the native binding stores epoch ms, and SQLite orders INTEGER before TEXT regardless of value -- so the comparison was true for EVERY row. Mail queued a second ago read as ten minutes overdue, and a scheduled_at years in the future read as already due. Confirmed directly against sqlite3: a 2026 row matches `created_at <= '2020-01-01T00:00:00.000Z'`. Binding a Date instead is not the fix, since knex hands sqlite3 a Date the same way and jest's sandbox Dates stringify to "[object Object]" (CLAUDE.md). So the engine-safe half of the predicate stays in SQL and the two time comparisons move into JS behind a toMillis() that accepts all three shapes this column really has -- Date from Postgres, ms-number from SQLite, ISO string from fixtures and older rows. The scan is capped at 1000 pending rows ordered oldest-first; everything overdue sorts into that window, and the response was already capped at 200. The existing tests missed this because they store ISO strings, which is what CLAUDE.md prescribes for jest -- so the new ones store epoch ms, the production shape, and one mixes both in a single queue. Retry was a no-op for the rows it most needed to help. It wrote pending / retry_count 0 / no schedule, which is exactly what a waiting row already is: the row came back unchanged while the toast said it had been re-queued. And since the usual reason a row is waiting is that nothing is working the queue, deferring it to the next pass is the one answer that cannot help. It now follows the reset with the same single-row flush the project cockpit uses. An idle pass no longer inherits the previous pass's totals -- the no-pending early return skipped the lastResult assignment, so System Health kept attributing an old sent/failed count to a run that did nothing. "All clear" now means the whole queue is clear, which is what the PR claimed and the code did not do. An empty waiting list is only reassuring when something is working the queue: a processor stopped a minute ago has no overdue rows yet either, and a green check there is the same false all-clear this branch exists to remove. 7 more tests. The 5 that pin new behaviour fail before this commit; the SQLite ones fail in the way the bug predicts rather than erroring. Both new tests stub the webhook transport with a spy rather than pointing it at a dead port: real connection attempts left open handles that destabilised unrelated suites in the same jest worker. |
||
|
Paul Nothaft
|
73d867521a |
fix(email): show a queue nobody is working instead of reporting all-clear
Closes #1262. "Gallery email queued" reads as a delivery confirmation, and System Health agreed with it: "No stuck or failed emails -- all clear", while not one email had gone out. Both statements were true and neither was the one the admin needed. Queueing writes an email_queue row at status='pending', retry_count 0 -- nothing more. /failures matched only status='failed' or pending-with-retry_count>=3, so it matched none of those rows, and there are two ordinary ways they never leave that state: - startEmailQueueProcessor() was never reached, so nothing polls the queue. - Every pass returns early. processEmailQueue bails when the transporter will not initialise, before it touches a single row, so retry_count stays 0 and no error_message is ever written. A working SMTP test button does not contradict this: that path builds its own transport. adminSystem.js made it worse by reporting `emailProcessor: { status: 'active' }` as a literal, so the one place that named the worker always said it was fine. - emailProcessor records what each pass did -- started, lastRunAt, lastResult, lastError -- and exports getQueueProcessorStatus(). The transporter bail and the queue-query failure, the two silent early returns, both write lastError. - /failures gains `waitingEmails`: pending, under the retry cap, past any scheduled_at, and queued more than 10 minutes ago. The predicate mirrors the processor's own pickup query, so a row listed there is one it should already have taken; rows over the cap stay in `stuckEmails` and are not counted twice. A future scheduled_at is left alone -- split-payment invoices and the business-hours floor park rows deliberately. - System Health leads with the processor's state (running / stopped / degraded) and lists waiting emails in their own table. The all-clear now needs both buckets empty. - adminSystem reports the real processor state instead of the literal. - The two "queued" toasts say the queue processor is what sends it and where to look if it doesn't arrive. 8 route tests, all 8 failing before the change. |
||
|
Paul Nothaft
|
5a0c9f53b0 |
fix(security): rate-limit the password-change endpoints per IP too
POST /api/auth/admin/change-password and POST /api/customer/profile/password both verify the current password before replacing it, which makes them a credential check an attacker holding a hijacked session can drive at will: the session's own JWT skips the general limiter as authenticated, and they were not in the auth gate's table. Both join it. Only failures count, so the one change a user legitimately makes costs nothing. |
||
|
Paul Nothaft
|
4515632300 |
fix(migrations): judge each German field on its own in migration 195
repairGerman gated subject, body_html and body_text on body_html alone, the same defect Codex found in migration 194: an admin who had translated only the subject lost it the moment the HTML still matched English, and down() is a deliberate no-op, so the loss was unrecoverable. Each field is now judged independently for both the translations row and the legacy _de columns, matching 194's corrected pattern. Two tests pin the two directions (translated subject over English body, and the reverse). |
||
|
Paul Nothaft
|
a929affd7e |
fix(security): close the case-sensitivity bypass in the API rate limiter
Express's `case sensitive routing` is off by default, so /API/admin/events reaches the same handler as /api/admin/events. Both the gate's `/api/` prefix test and rateLimitService's public-endpoint classification compared the raw path, so simply upper-casing a letter skipped the limiter entirely. Verified against a real Express app before fixing: /api/admin/events routes and hits the gate; /API/admin/events and /Api/Admin/Events route and miss it. Both now match on a lower-cased path. The auth gate added alongside was already immune -- its patterns carry the `i` flag for exactly this reason. Not changed: rateLimitSecurity.hasValidAdminToken's /api/admin/ test has the same shape, but there the case-sensitive comparison fails safe -- an upper-cased path simply does not get the admin skip, so it is rate limited rather than exempted. Making it case-insensitive would widen a skip, so it is left alone. maintenance.js's isAdminRoute is fail-safe for the same reason. |
||
|
Paul Nothaft
|
50e8ed6e58 |
fix(security): apply per-IP rate limiting to credential endpoints
The five authRateLimiter registrations were inert for the same reason the
general one was -- registered below the error handler. Auth endpoints have
never had an IP limit; the 5-attempt behaviour QA observed is the per-account
lockout in authSecurity.js, which is a different mechanism and is untouched.
They could not simply be activated: app.use('/api/auth', ...) is a prefix, so
a 5-per-window budget would have covered GET /api/auth/session and
POST /api/auth/password-strength, which the frontend calls far more than five
times per window. That locks users out.
The real surface was enumerated by loading the routers and walking
router.stack rather than grepping, which showed two of the five registrations
pointed at routes that do not exist: adminAuth.js has no /login (admin login
is POST /api/auth/admin/login) and there is no /api/gallery/:slug/verify
(gallery verify is POST /api/auth/gallery/verify).
Now limited, on exact method+path: admin login, admin MFA verify, gallery
password verify, share-login, client PIN, setup verify-token, setup admin,
customer login, customer password-reset. Deliberately unlimited: session
checks, password-strength, logouts, authenticated change-password, the SSO
round-trip (a 429 on the callback breaks login from shared corporate IPs),
and one-time invite/accept-invite links.
Two choices carry the design. skipSuccessfulRequests means only failed
attempts spend budget, which is what makes 5-per-IP survivable behind NAT --
ten guests on one venue wifi all typing the correct gallery password consume
nothing -- and means a legitimate admin cannot be locked out by their own
success. And the limiter keeps its own rateLimit() instance, hence its own
store and its own per-IP bucket, with the general gate's auth exemption left
in place: sharing a counter is exactly the lockout described above.
Patterns are case-insensitive because Express's case-sensitive routing is off
by default, so POST /api/auth/admin/LOGIN reaches the login handler and a
case-sensitive pattern would have been a free bypass.
max is now read per request, so the Settings UI's rate_limit_auth_max_requests
applies without a restart, matching the general limiter.
Tests prove both directions: each credential endpoint 429s on attempt 6 with
the response shape the four login pages already branch on, each benign
endpoint still returns 200 after 40 calls, the two buckets are independent in
both directions, and 30 consecutive successful logins consume no budget.
Refs testplan REPORT.md, rate-limiter gap.
|
||
|
Paul Nothaft
|
b0f33c1744 |
fix(security): actually apply the general API rate limiter
app.use('/api/', generalRateLimiter) lives inside initializeRateLimiters(),
which is defined at line 463 but not called until 1048 -- by which point the
routers (767+), the /api notFoundHandler (1002) and errorHandler (1029) are
already on the stack. All six app.use() calls in it therefore append BELOW the
error handler and can never see a request. generalRateLimiter had no other
registration path.
So the entire /api surface had no IP-based request limit, except the handful
of routes carrying their own inline rateLimit() (public quotes, contracts,
payment-check, transfers, the analytics proxy). The admin Settings
rate-limiting UI -- rate_limit_enabled, rate_limit_max_requests -- was writing
to a control that did nothing.
Fixed with a stable gate registered above the routers that resolves the
limiter per request, so there is no boot delay: it is a pass-through until
initializeRateLimiters() resolves, exactly matching prior behaviour.
Registered unmounted (app.use(gate), not app.use('/api', gate)) because
Express strips the mount path from req.url and rateLimitService's own logic is
written against the full path -- req.path.startsWith('/api/public/') and the
/api/(gallery|secure-images)/:slug regex it uses to find a gallery token to
skip on. Mounting it would have silently broken both.
Deliberately excluded, each for a concrete reason:
- /health and /api/health, mounted above the gate: a 2s probe is 450
req/window and would 429 the container healthcheck.
- /api/public/transfer and transfer-upload: one request per file from a link
holder with no JWT, so never skipped as authenticated; a large transfer
would be cut off mid-way. Both already have tighter per-minute limiters.
- login and gallery-verify: the limiter returns authMaxRequests (5) as their
budget but counts them into the SAME per-IP bucket as every other /api call,
so the branding and settings fetches a login page makes before anyone types
a password would 429 the login itself for a full window. Giving these a real
per-IP limit means giving them their own bucket.
Bulk gallery and admin traffic is unaffected: skip_authenticated defaults true
and cookie tokens are promoted to Authorization before the gate runs, and
skipped requests do not increment the counter.
Also adds /api/health as an alias of /health -- one handler, identical
exposure -- which silences a ~2s probe warning. Registered above the API
middleware chain deliberately: left at its original position it would have
passed through apiRequestLogger and through maintenanceMiddleware, whose
skip-list contains /health but not /api/health, so it would have 503'd during
maintenance while /health returned 200.
The tests pin registration depth by source inspection as well as behaviour,
because depth is what was broken and no unit test of the gate can catch it.
Refs testplan REPORT.md, /api/health warning; rate-limiter gap found while
fixing it.
|
||
|
Paul Nothaft
|
a89057df1d |
fix(search): stop escapeLikePattern corrupting bound search values
Verified against a real SQLite connection -- each of these returned zero rows before and the right row after: "Sarah's" before=[] after=["Sarah's Birthday"] "100%" before=[] after=["Summer 100% Sale"] "Gala_" before=[] after=["Gala_Night"] Two bugs in one helper. It did .replace(/'/g, "''"), which is SQL string-quote doubling -- meaningless and actively corrupting for a value that is bound, so any search containing an apostrophe matched nothing. And its \% escaping had no ESCAPE clause on the LIKE, which is engine-dependent: honoured on Postgres, a literal backslash on SQLite, so % and _ stayed wildcards there. Now mirrors the correct implementation from 59666b59: escape \ % _ only, and a new likeWithEscape(column) emits `col LIKE ? ESCAPE '\'`. Both call sites move to whereRaw with the value still bound; the column argument is a literal, documented in the JSDoc. Callers checked before changing the contract: adminPhotos.js, adminEvents/crud.js, and sqlSecurity's own addLikeCondition(), which has no callers anywhere -- pre-existing dead export, updated to the new shape rather than deleted. Behavioural change: searches containing ' % _ or \ now return the right rows instead of nothing. Case sensitivity is unchanged. Refs testplan REPORT.md, escapeLikePattern finding. |
||
|
Paul Nothaft
|
a28f96b304 |
fix(gallery): no-store private JSON, and give guest uploads a real status
B6 -- seven gallery routes returned private, per-guest data with no
Cache-Control at all, relying on heuristic freshness. noStoreCache is mounted
per route rather than on the router, because the media routes set their own
private, max-age=1800/3600 and must keep it. Covered: /photos (own
likes/favourites/ratings, hidden photos for a client token), /people, /stats,
/verify-token/:token (an authorization decision -- a cached {valid:true}
outlives a rotated token), /show/:token/session (the response IS a credential;
it mints a gallery JWT), /show/:token/state and /download-jobs/:token (live
polls, where a cached "preparing" strands the caller). Deliberately untouched:
the photo/thumbnail/hero/preview and css-template routes, which set their own
caching, the binary downloads, and /info + /resolve, which are unauthenticated
public metadata rather than per-guest private.
ETag/304 revalidation is intact and pinned by a test: no-store stops the
browser retaining the body, not express agreeing an unchanged payload is
unchanged. That matters because the post-upload poll depends on it.
B7 -- the guest upload flow had no progress signal, so the UI polled the photo
list blind and gave up after 60s with no explanation. Adds
GET /:slug/uploads/status?ids=... rather than pending counts in the photos
payload: counts there are event-wide, so another guest's or the admin's stuck
upload would spin the notice forever and it could never say "your photo
failed".
Authorization: verifyGalleryAccess already resolves req.event from the
caller's token, and the query is scoped `.where('event_id', req.event.id)`, so
an id from another gallery matches no row -- neither a cross-event read nor an
existence oracle, since it returns all-zero counts rather than a 403/404 that
would confirm the id exists elsewhere. Slideshow tokens are denied (a kiosk
never uploads). Ids are pattern-validated, max 50. The response is counts
only: no filenames and specifically no processing_error strings, which can
carry internal paths. Not gated on allow_user_uploads, so an admin flipping
the toggle mid-flight does not strand an in-progress guest.
The frontend now finishes on the real terminal condition, refetches as each
photo lands rather than only at the end, shows a processing pill, and reports
real failures instead of silently timing out.
Refs testplan REPORT.md B6, B7.
|
||
|
Paul Nothaft
|
7c9baff751 |
fix(upload): scope category ids, stop temp-file leaks, split the video cap
Four related fixes on the admin upload/photo path. B5 -- PATCH /photos/:photoId and POST /photos/bulk-update took any parseInt(...) > 0 straight into the update with no existence or scope check, so a photo could be moved into another event's category. The upload route already validated `event_id = X OR is_global` per #500/#525; extracted that query as findScopedCategory() and used it on all three routes so the 400 body is byte-identical. 0/negative/'individual'/'collage'/null still clear without a lookup, so the clear path costs no extra query. B9 -- three distinct temp-file leaks, not one. The validator's size branch never unlinked; the cleanup lived in the final handler, unreachable on any 400; and multer's `destination` callback runs per file and overwrote req.tempUploadPath, so even the success path only ever removed the last file's directory. Now: discardUploadedFiles() runs on every 4xx and the 500 (ENOENT tolerated, and files are only dropped when the whole request is being rejected, so the passing path is untouched); cleanup registered before multer so it also covers multer's own LIMIT_FILE_SIZE return; one directory per request. B8 -- the admin uploader filtered on MIME only, so an oversized file was uploaded in full before the server's 400. Mirrors UserPhotoUpload's existing per-file toast-and-drop. C4 -- general_max_file_size_mb was a single cap for photos and videos, so the 50MB default meant admins could not upload ordinary video without also raising the photo limit. Adds general_max_video_size_mb (default 500MB, clamped by the same 10GB MAX_ALLOWED_FILE_SIZE_MB ceiling, read per request, 60s cache), editable in Settings -> General. Photo uploads are protected from regressing by keeping multer's type-blind limit at max(photoCap, videoCap) and moving the per-kind decision into validateUploadContent, where file.mimetype exists. It 400s with the existing message shape, so an oversized photo is still rejected with the identical body it produced when multer did the rejecting. Known gap: chunked-upload/init still applies the photo cap to video. Making it video-aware would change an existing assertion that pins a 200MB video init being rejected under a 1MB general cap. No component calls that path today and the direction is strict rather than a bypass, so it is left as-is. Guest video uploads still share the single cap in gallery.js. Refs testplan REPORT.md B5, B8, B9, C4. |
||
|
Paul Nothaft
|
103863cbab |
fix(quotes): enforce the status state machine, and correct the table
VALID_QUOTE_TRANSITIONS was a complete-looking quote state machine that nothing consulted, so status changes were unvalidated. Mapping every writer of quotes.status (quoteService.js is the only one -- dealsService, projectService, adminDashboard and customer.js all read) showed the table itself was wrong: six legitimate transitions were missing. sendQuote allows draft/declined/expired -> sent but the table had draft only; adminAcceptQuote allows draft/sent/expired but had sent only; adminDeclineQuote allows draft/sent/expired but had draft/sent; recordResponse had no same-status entry. Enforcing it as written would have broken accept-on-behalf from a draft, resend-after-decline, every expired revival and the 15-minute response-toggle window. So the table is reconciled to reality first, then assertQuoteTransition() (409, QUOTE_INVALID_TRANSITION) is called at all seven sites. Two things worth carrying forward. Nothing in the codebase ever sets 'expired' -- the header comment says "set by the scheduler" and there is no such scheduler; sent -> expired is retained as documented intent only. And the backstop's added value is narrow: every reachable invalid transition is already caught by a call site's own better-worded guard, which fires first. What it newly catches is a status the machine has never heard of -- a legacy or corrupt row like 'cancelled' sails through adminAcceptQuote's guard, which only excludes accepted/declined/converted, and used to be silently overwritten. That is what the new tests pin. Refs testplan REPORT.md B4. |
||
|
Paul Nothaft
|
a7d45ddd0d |
fix(workflows): restore the once-per-process seed guard
`booted` was assigned but never read, so the guard's early return was missing and the builtin workflow seeder ran on every call. Impact was wasteful, not harmful: seedOneBuiltin is idempotent -- it keys on builtin_key and returns early when adminOwned or storedVersion >= def.version, writing a graph only on a fresh insert or a version bump. So repeat calls cost a lookup per builtin plus a graph rebuild, with no duplicate rows. `booted = true` stays inside the try, so a seed that never got off the ground (workflows table not migrated, DB down) leaves the flag clear and retries. A per-builtin failure is still swallowed by the inner catch and does not block the flag, unchanged. Restoring the guard broke workflowEngine.test.js, which calls the boot seeder seven times in one worker and needs the second call to run in two of them. Followed the existing _backupPathsBoot/_restoreSettingsBoot precedent: exported _resetBootForTests(). Refs testplan REPORT.md B3. |
||
|
Paul Nothaft
|
41e1de7818 |
fix(email): repair and seed the gallery lifecycle templates
Correction: the reported premise held for only one of the three templates,
verified by running the core migration set against an empty database.
- expiration_warning is German-is-English on every fresh install, exactly as
reported. Repaired with migration 194's pattern verbatim.
- gallery_expired and archive_complete are NOT German-is-English -- they do
not exist at all. Their master rows are inserted only by migrations/legacy/
010+020, which never run on a fresh install, so 075/099/106/108 seeded zero
translations for them (they key off a master row that is not there). A
fresh install's email_templates holds 17 keys and neither is among them.
The consequence is worse than a translation gap: expirationChecker's
sendGalleryExpiredEmails and archiveService's completion mail both hit
"Email template not found", retry three times and die silently in
email_queue on every expiry and every archive.
So 195 also seeds those two (master row + en/de translations + category),
but only when the master row is absent -- it never overwrites. English
follows legacy 028, which emailProcessor's own comments call the shipped
copy; German follows legacy 026's wording. Both are restructured into the
plain unstyled shape the other core-seeded templates use, so wrapEmailHtml's
configurable palette governs styling rather than hard-coded hex. The
support-contact line is wrapped in {{#if support_email}} because
getSupportEmail() can return ''.
196 adds the {{#if welcome_message}} block that nl/pt/ru/fr/es/sl already
have in gallery_created but en and de lack, so the photographer's personal
note was silently dropped for those two locales even though the value is
passed at send time. safeTemplateReplace does resolve {{#if}} before variable
substitution, so this is a real conditional -- there is a test rendering the
migrated body both ways. HTML body only, matching the other locales:
emailProcessor rewrites welcome_message through formatWelcomeMessage
(escape + nl2br) once for both bodies, so the text part would print literal
<br /> and &.
Both migrations keep 194's conservative condition -- rewrite only while the
German is still byte-identical to English or empty -- so admin-edited and
legacy-translated installs are untouched. Idempotent, guarded, no-op down().
Known gap, documented in 195's header: the two newly seeded templates get
en/de only. nl/pt/ru/fr/es/sl fall back to en via processTemplate's fallback
chain, which is strictly better than today's hard failure but is not real
localisation.
Refs testplan REPORT.md B1, B2.
|
