v3.101.3-beta.0
594
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Paul Nothaft
|
a607cea110 |
fix(auth): issuer-tag the oversize SSO logout marker (#798) (#1010)
Phase 3 validated a stored ID token hint against the currently configured issuer, but the oversize path never got that check: an ID token above the 3.9KB cookie limit was stored as the bare string 'sso', which collapsed to an undefined hint at logout and skipped validation entirely. Changing the issuer while such a session was live bounced the user to the new IdP on logout. Stores sso.<base64url(issuer)> instead and moves all marker interpretation into buildEndSessionUrl: raw ID token -> iss/aud-validated hint, issuer-tagged marker -> round-trip without a hint, anything else -> no round-trip. Every branch fails closed. Refs #798. |
||
|
LucaandLuca-Timo
|
2e495d7c48 |
feat(transfers): add PicTransfer — cross-event file transfers (#998)
Closes #997. Send original files from any event as a token-protected download link, with an optional client-upload channel. Strictly opt-in behind a new `transfers` feature flag, default OFF. Migrations 170-172 (transfers, transfer_files, transfer_extra_files, transfer_uploads, transfer_recipients, transfer_downloads, default settings and two email templates) — all hasTable/hasColumn-guarded and idempotent, with destructive statements confined to down(). Backend: transferService (CRUD, 256-bit download token, 6-char upload token, cross-event ZIP streaming of originals), admin CRUD routes, and two public token routes. transferCleanupService runs an hourly retention sweep; source-event photos are never touched. All three routers fail closed via requireFeatureFlag('transfers'). Review closed two ownership blockers, both the same root cause — permissions used where ownership was needed: - photoIds arrived from the request body and were validated only for existence, so a scoped admin could bundle any event's originals and hand them out through the public download token. filterOwnedPhotoIds now resolves ids to their events and gates them through filterOwnedEventIds, on both the create and add-files paths. - The transfer list was unscoped and carried each row's download token, so any admin with events.view could read another's token and fetch their originals. The list is now scoped by created_by, the token/url fields are stripped from the list payload, and a single router.use('/:id', requireTransferOwnership) covers all twelve /:id routes, 404ing foreign and missing alike. The admin photo picker filters its event list to the same rule, so the UI stops offering picks the API would discard. Fork-PR workflows had not been approved since the fix commits, so the PR's green checks were stale against the pre-fix head. Verified by dispatching tests.yml against the actual head: backend and frontend both green. Follow-up: neither ownership guard has a regression test yet. Co-authored-by: Luca-Timo <[email protected]> |
||
|
Luca
|
f00661511c |
feat(gallery): admin preview skips the password on protected galleries (#981)
Closes #868. A logged-in admin opening a published, password-protected gallery is let straight in, mirroring the existing draft-visibility bypass. Mechanism: an explicit ?admin_preview=1 intent flag AND a verified admin session read from the httpOnly admin_token cookie (or an admin-typed Bearer) — never a token from the URL. This retires the old ?preview=<raw-admin-JWT> scheme, which leaked a 24h admin token into the address bar, referrers and proxy logs. Per-request bypass only: no gallery JWT is minted, the password endpoint is never reached so the login_attempts lockout buckets stay clean, and admin previews are excluded from guest analytics (access_logs, download counts, per-photo view_count, notification bells). Review (two rounds) closed three blockers and two concerns: - Transport: verifyGalleryAccess now resolves admin preview before any gallery credential, and isAdminPreview reads the admin cookie first and type-checks every candidate — so an admin Bearer no longer 403s on the type gate, and a coexisting gallery session can no longer shadow the admin cookie. - Reveal mode (#838) is a second consumer of isAdminPreview; its bypass is unchanged, only the transport moves. revealMode.test.js updated off the retired scheme and now carries a coexisting gallery Bearer. - Admin previews no longer inflate per-photo view counts, and the internal photo redirects preserve the flag via withPreview() so they still authorise. - Happy path: GalleryPage renders GalleryView directly for a preview instead of attempting the public empty-password auto-login, which 401'd against a genuinely protected gallery and stranded the page on the skeleton. The backend job timed out once at the 10-minute CI limit; a re-run completed in 2m02s, in line with main's ~2m10s baseline, so that was a runner flake rather than a hang. |
||
|
Paul Nothaft
|
137a42f259 |
feat(admin): surface the registry move through the update check (#993)
Relates to #985 — does NOT close it. Adds registryMigrationRequired to the update-check payload (stable channel below 3.45.0) and an amber block in UpdateNotification explaining that the retired registry path still responds, so `docker compose pull` appears to succeed while serving the same frozen build. Known limitation, established in review and merged deliberately: this cannot reach the operators #985 describes. PicPeak is self-hosted, so the update-check code runs inside the operator's own image — a v3.44.0 install runs v3.44.0's backend forever, and the only external call returns release metadata, not logic. Every build containing this predicate is >= 3.45.0, where it is false by definition. The release-notes fallback fails too: the changelog modal shipped 2026-05-29, two days after the freeze. Correct for any future rename, no runtime cost, but #985 stays open — the population it describes still has no in-app channel. Viable routes are external (retired GHCR package description, repo README, docs). '0.0.0' is excluded from the predicate: that is getCurrentVersion's fallback for an unreadable package.json, i.e. a broken install, not a pre-rename one. |
||
|
Paul Nothaft
|
0c8ad6bbed |
fix(security): vet the destination project when linking a deal (#991)
linkDealToProject re-points a deal's quotes, contracts and events into `projectId`. Its lineage guard vets the SOURCE events and its comment assumed the route had vetted the destination — true only for attachDocumentToProject. quoteService.create/update and contract crud.create/update take `projectId` straight from the request body behind quotes.manage / contracts.manage, which are permissions, not ownership; adminQuotes.js and adminContracts.js carry no ownership guard at all. The lineage guard did not cover it: it is skipped when the deal has produced no event yet, which is the state of a newly created quote, and an unassigned destination ADOPTS the deal's customer rather than rejecting it. A scoped admin could therefore write into another admin's project, and on an OWNERLESS project (created_by IS NULL — legacy rows migration 167 could not attribute) escalate to a read: once the quote converts to an event it becomes the project's only linked event, which is the condition ownedProjectsSubquery's second branch grants ownership on. Vetted at the service choke point all four callers share, ahead of both the null-deal early return (callers write project_id before calling, and deal_uuid is nullable) and the customer check (whose 422 vs 404 was an enumeration oracle). 404 PROJECT_NOT_FOUND throughout. super_admin unaffected. |
||
|
Luca
|
165cebdb5c |
feat(accounting): re-bill proof attachment, CRM panel & hours↔re-bills cross-add (#979)
Closes #866. Three features, all behind the `incomingInvoices` feature flag: 1. Attach the stored supplier proof PDF to the client-invoice email when a captured invoice is re-billed/passed through, as a SEPARATE attachment so invoice immutability holds. Global default (off), per-customer tri-state override, and per-file selection in a new Send dialog. A missing proof at issue time stamps inbound_documents.proof_attach_error rather than silently dropping, and never blocks the send. Proof filename is a configurable template with {INVOICE} {SUPPLIER} {YEAR} {MONTH} {SEQ}/{SEQ:0Nd} tokens. 2. Re-bills & passthrough panel under CRM → Customer, grouped Open/Sent/Paid with status derived from the linked invoice lifecycle rather than a duplicated column. 3. Cross-add dialog rolling open hours and open re-bills into one invoice, symmetric from both entry points. The two stay distinct, contiguous line groups — never merged into shared line items. Migration 169 is additive, hasColumn-guarded and idempotent. Review (two rounds) closed two concerns: - Storno stranding: nothing cleared inbound_documents.billed_invoice_id when a covering invoice was cancelled, so a Storno'd re-bill showed as Open in the new panel while every billing path filters on that column being NULL — the supplier cost could never be re-billed. releaseRebillsForCancelledInvoice now detaches the linkage on both invoice-cancel paths, with a regression test on the issued-cancel path. - Permission gating: the new controls rendered on data presence alone while their endpoints require accounting.view / accounting.manage / customers.edit. Now gated at both the query and render layers. Known follow-up: two cross-add counter queries are gated on a permission their endpoint does not check (HoursSection.tsx:174, CustomerCrmPanels.tsx:270) — degrades safely, one line each. |
||
|
Paul Nothaft
|
67592fc569 |
fix(projects): stop the cockpit offering email controls the API rejects (#976)
Closes #969. The cockpit's email feed rendered preview/resend/cancel/retry/send-now for every mail, consulting neither the caller's role nor their permissions, producing controls that always failed: 404 - requireOwnedQueuedEmail scopes queued mail through email_queue.event_id AND ownership of that event. CRM document mail carries no event_id; and project ownership does not imply event ownership, so a project the caller owns can hold another admin's event. 403 - preview needs events.view but the four write actions need email.send. getProjectOverview now stamps each email with an authoritative canAct, mirroring filterOwnedEventIds; created_by is selected only for that check and stripped before the response. The cockpit reads canAct and combines it with email.send. A missing canAct reads as false. Regression from the GHSA-93x4 fix in #960/#966, which added the ownership middleware. |
||
|
Paul Nothaft
|
6699855c93 |
fix(auth): fail closed when the adminAuth roles join errors (#974)
Closes #968. The roles-join fallback in adminAuth fabricated role_name='super_admin' on ANY database error, so a transient fault (connection reset, deadlock, statement timeout, pool exhaustion) silently granted super_admin for its duration. roleName is the sole discriminator for every ownership check, so this inverted the authorization model rather than failing the request. Gate the fallback on isMissingRolesSchema(), moved to utils/dbErrors.js and shared with apiTokenAuth. The predicate was also tightened: knex prefixes the failing SQL to err.message and that SQL always names `roles`, so the old /roles/i gate was vacuous and a generic /does not exist/ could accept unrelated faults. Now trusts SQLSTATE 42P01/42703 on Postgres and exact driver phrasing on SQLite. |
||
|
Paul NothaftandPaul Nothaft
|
7c0c0a5b7f |
fix(security): enforce project ownership on project + project-email routes (GHSA-wrg5, GHSA-93x4) (#960)
* fix(security): enforce project ownership (GHSA-wrg5, GHSA-93x4) Project routes authorized on generic events.view / events.edit with NO ownership check, so an editor-like admin could enumerate, read, update and aggregate projects belonging to other admins' events. The project email endpoints keyed on an email_queue id alone — any admin with events.view / email.send could preview, resend, cancel or retry ANY queued mail by walking ids. The earlier 'needs a migration, deferred' assessment was wrong in one direction and right in another: ownership IS derivable transitively via events.project_id -> events.created_by, but only for projects that already have a linked event. A brand-new EMPTY project has no derivable owner, which is exactly where the create -> attach flow starts. So migration 167 adds projects.created_by (backfilled from the single linked event owner, skipping ambiguous multi-owner projects) and createProject finally persists the adminId it was already being passed. - ownedProjectIds(): union of the stored owner and the transitive path, so pre-167 rows and new empty projects both resolve. Reads created_by defensively so an instance that hasn't run 167 falls back to the transitive rule instead of throwing. - requireProjectOwnership on detail/update/attach-event/attach-quote/ attach-contract/overview; list filtered by an id allowlist (empty array means 'owns nothing' and must return no rows, hence null-vs-[] care). - POST /:id/events also validates the INCOMING eventId — owning the project is not enough, or an editor could pull a foreign event in and read its rolled-up documents via /:id/overview. - Queued-email routes scoped via email_queue.event_id. CRM document mail has event_id NULL and no ownable parent here, so a scoped caller is denied rather than guessed into access. 404 (not 403) so it isn't an id oracle. Note: adminEmail.js:315/332 let any email.view/edit admin archive or delete any email_queue row — the same class, pre-existing and outside these two advisories. Left untouched and reported rather than silently widened. * fix(security): codex round 2 — make the stored project owner authoritative (GHSA-wrg5) The first predicate union'd 'any linked event I can see' with the stored owner, which opened two holes: - A project owned by admin B containing ONE legacy ownerless event became readable by every admin — and /:id/overview aggregates B's other events, invoices and emails, so a single legacy event exposed the whole project. - Migration 167 deliberately leaves multi-owner (ambiguous) projects NULL rather than guessing an owner. A NULL owner was then treated as 'everyone's', so exactly those mixed projects became globally accessible. Now: the stored created_by wins outright, and a project without a usable stored owner only derives access when EVERY linked event is accessible (and at least one exists). A created_by pointing at a hard-deleted admin degrades to 'no usable owner' so the project falls back to its events instead of being locked away — no ON DELETE SET NULL migration needed. A project with neither a usable owner nor linked events stays super_admin-only: failing closed beats failing open, and a super_admin can reassign it. Also returns a knex SUBQUERY rather than a materialised id list, so a large project count can't hit the driver's bind-parameter limit. * fix(security): codex round 3 — enforce deal-lineage ownership on project attach (GHSA-wrg5) requireProjectOwnership vets only the DESTINATION project, while attaching a quote or contract cascades through linkDealToProject — which re-points every event the deal produced into that project. An editor could therefore create an empty project of their own, attach another admin's quote, and pull that admin's events (plus the invoices, emails and gallery that roll up with them) into a project they own and can read via /:id/overview. The single-customer guard did not stand in the way: an unassigned project ADOPTS the deal's customer rather than rejecting it. linkDealToProject now refuses to move lineage events the actor cannot own, and assignDocument cascades BEFORE stamping the document so a refused attach leaves nothing half-applied (the old order committed the foreign document into the caller's project and only then declined the cascade). The quote/contract create+update paths, which reach the same cascade with an arbitrary project_id, thread their adminId through as well; isSuperAdmin() resolves the role for them and fails closed when it cannot. Events are the only ownership signal a deal carries — quotes and contracts have no created_by in this schema — so a lineage that produced no event still cannot be attributed. That is a property of the CRM model, noted in the code. Claude-Session: https://claude.ai/code/session_01F211U4dDbEj4zXiyKbi9me * docs(security): drop the stale ownership JSDoc left by the rebase (GHSA-wrg5) Rebasing onto main (which had gained scopeEventsQuery from #957) replayed the round-1 doc block above round-2's replacement, leaving a comment that describes the ORIGINAL union rule — "a project is the caller's when … it has at least one linked event they own" — directly above the code that deliberately no longer does that. That union is the hole round 2 closed; a comment asserting it is worse than none. Claude-Session: https://claude.ai/code/session_01F211U4dDbEj4zXiyKbi9me --------- Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul NothaftandPaul Nothaft
|
164129b8f5 |
fix(security): escape brand tokens, block tracker redirects, trim logo diagnostic (GHSA-j347, mw76, 29vm) (#961)
* fix(security): escape brand tokens, block tracker redirects, trim logo diagnostic (GHSA-j347, mw76, 29vm)
GHSA-j347 — buildCachedPayload sanitizes the operator's HTML and THEN runs
applyBrandTokens over the result with a plain String.replace, so any markup in
a token value reached the public origin unfiltered. The default templates
interpolate tokens into text AND into quoted attributes
(<img src="{{brand_logo_url}}" alt="{{company_name}} logo">,
href="mailto:{{support_email}}"), so a value could close the attribute and
inject. Token values are now HTML-escaped on substitution, mirroring
galleryOgService's escapeHtml. sanitizeBrandUrl's case-sensitive literal
'javascript:' check (which 'JavaScript:' walked straight past) is replaced by
an http/https scheme allowlist; relative logo paths are unaffected.
Writer is settings.edit (super_admin only) and the CSP blocks inline script,
so this is defence-in-depth — but sanitize-then-substitute is a real ordering
bug regardless.
GHSA-mw76 — the SSRF decline STANDS: self-hosted operators legitimately point
analytics at private addresses, so connection-time IP blocking would break real
deployments. Fixed only the narrow leak: undici strips
Authorization/Cookie/Proxy-Authorization/Host across a cross-origin redirect,
but umamiAdapter sends a CUSTOM x-umami-api-key header, which would be replayed
verbatim to the redirect target. Both adapters now use redirect: 'error'.
GHSA-29vm — the logo diagnostic echoed absolute storage roots, process.cwd()
and absolute candidate paths. It now reports candidates relative to
<STORAGE>/<CWD_STORAGE>, which answers the same 'which candidate existed'
question. It also still advertised the raw-absolute candidate that GHSA-c7x5
removed from resolveLogoFile, so it was misreporting what the resolver tries —
aligned with the real candidate list.
publicSiteService.test.js expectation updated: an '&' in a company name is now
emitted as '&'. Renders identically; the raw payload string differs.
* fix(security): codex round 2 — stop the remaining logo-path disclosure, mirror the resolver (GHSA-29vm)
- sources[].value was still echoed verbatim. branding_logo_path is stored
ABSOLUTE by multer, so relativising only resolvedTo and the candidate paths
left the filesystem layout going out anyway. It is now relativised too.
- Round 1 dropped the raw-absolute candidate on the grounds that GHSA-c7x5
removed it from resolveLogoFile — but the c7x5 follow-up RE-ADDED it (kept,
subject to the containment filter, so a legitimate multer path still
resolves). The diagnostic therefore reported every candidate as missing for
a contained absolute logo while resolvedTo named the file. It now mirrors the
resolver, containment filter included.
One deliberate cosmetic divergence, commented in place: for an absolute value
the resolver also tries path.join(root, value-minus-leading-slash), which can
never exist and would re-embed the absolute path this endpoint must stop
echoing. Omitted; every candidate that can actually match is still shown.
* fix(security): codex round 3 — mirror the resolver for root-relative logo paths (GHSA-29vm)
The logo diagnostic skipped the `<STORAGE>/<value>` candidates whenever
path.isAbsolute(value) was true. That test cannot distinguish a multer disk
path from a root-relative URL such as `/custom/logo.png`, and for the URL form
resolveLogoFile.generateCandidates() does try `<STORAGE>/custom/logo.png` and
can resolve it — so the endpoint reported "no source candidate exists" about a
logo that renders fine, and collapsed the configured value to its basename.
The stripped joins are now built unconditionally, exactly as the resolver does.
Disclosure stays closed: every candidate still passes the containment filter and
redact() rewrites survivors to `<STORAGE>/…`, never an absolute host path.
Claude-Session: https://claude.ai/code/session_01F211U4dDbEj4zXiyKbi9me
* fix(security): gate the logo stripped-joins on containment, not isAbsolute (GHSA-29vm)
The previous commit dropped the isAbsolute() gate entirely and regressed
logoDiagnostic's own disclosure assertion: for a genuine multer disk path,
path.join(root, value-minus-leading-slash) yields
`<STORAGE>/tmp/…/storage/custom/logo.png`, and redact() only rewrites the
LEADING root — so the inner absolute path went straight back into the payload.
The right discriminator is not "is this absolute" (which cannot separate a disk
path from a root-relative URL) but "does the value already resolve inside a
storage root". If it does, it is a real disk path, the raw candidate already
covers it, and the stripped join is the double-prefixed junk that can never
exist. If it does not — the `/custom/logo.png` URL form — the stripped join is
exactly what resolveLogoFile resolves, and is shown.
Covered by a new case asserting both halves: the candidate appears for the URL
form, and the payload still contains neither the storage root nor cwd.
Claude-Session: https://claude.ai/code/session_01F211U4dDbEj4zXiyKbi9me
---------
Co-authored-by: Paul Nothaft <[email protected]>
|
||
|
Paul NothaftandPaul Nothaft
|
e2ce95ee48 |
fix(security): enforce event ownership on the v1 API surface (GHSA-9697) (#957)
* fix(security): enforce event ownership on the v1 API surface (GHSA-9697) Migration 081 documents the intent — 'the token's effective permissions are the intersection of the user's role permissions and the token's own scope flags' — but it was never implemented. - apiTokenAuth selected only id/username/email/role_id, so req.admin.roleName was undefined. Every ownership helper keys on roleName, so the v1 surface could not tell a super_admin from a demoted viewer. Now joins roles and emits the same req.admin shape adminAuth does, including the roles-table-missing upgrade fallback. - No v1 route applied any ownership predicate: GET /events listed every event on the instance, and GET /events/:id/share-link returned ANY event's share_token — the gallery access credential, same class as GHSA-rh8r. List is now scoped via a new scopeEventsQuery helper; the three :id routes (detail, photo upload, share-link) use the existing requireEventOwnership. Not a breaking change: tokens are minted by super_admins, who bypass ownership. It closes the case where a token's owner is later demoted — userManagementService never touches api_tokens, so the token outlived the demotion with full read of every gallery's share token. events.category.test.js stubbed apiTokenAuth without roleName; giving the stub super_admin keeps requireEventOwnership from issuing a DB query and desyncing that suite's sequenced dbMock. * fix(security): codex round 2 — intersect v1 token scopes with role permissions (GHSA-9697) Ownership scoping alone left half the documented control missing. Migration 081 defines a token's effective permissions as the INTERSECTION of the owner's role permissions and the token's scope flags; requireApiScope only ever checked the scope half. A token minted while its owner was super_admin therefore kept write access after the owner was demoted to viewer — userManagementService never touches api_tokens, so the token outlives the demotion, and ownership scoping does not help because the demoted owner still owns their events. Adds requirePermission to all six v1 routes (events.create on create, events.view on the reads, photos.upload on upload). It keys on req.admin.id, which apiTokenAuth already populates. The two existing v1 suites mock the database, so a real permission lookup 500s — they now mock the permissions middleware as pass-through, matching how they already mock apiTokenAuth. Those suites cover route logic; the intersection is pinned by the new v1TokenPermissions suite. * fix(security): codex round 3 — fail closed on the roles-join fallback (GHSA-9697) The round-2 fix loaded the token owner's role so the v1 ownership checks could tell a super_admin from a demoted viewer, and mirrored adminAuth's roles-table-missing fallback. That fallback assigns role_name = 'super_admin', and the catch around it was unconditional — so ANY failure of the joined query (connection reset, deadlock, statement timeout) elevated the token owner to super_admin as long as the simpler fallback query then succeeded. A restricted owner could ride that into listing, reading and share-tokening every event on the instance, which is the exact hole GHSA-9697 closes. The fallback is now reached only for an error that genuinely names a missing roles table/column (PG 42P01/42703 or the SQLite/MySQL wording); anything else propagates to the 500 handler. Claude-Session: https://claude.ai/code/session_01F211U4dDbEj4zXiyKbi9me --------- Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul NothaftandPaul Nothaft
|
1b4e5fee3e |
fix(security): bound inbound-mail resources, redact secrets from logs (GHSA-2qf9, pgmp, r794) (#959)
* fix(security): bound inbound-mail resources, redact secrets from logs (GHSA-2qf9, pgmp, r794) GHSA-2qf9 — emailIntakeService downloaded, parsed and persisted every message with no size, attachment-count or attachment-byte limit, reachable unauthenticated by anyone who can email the operator's mailbox: - fetch the envelope with `size` (same cheap pass) and refuse an oversized message BEFORE downloading its source; - cap attachment count and cumulative attachment bytes; - limits env-overridable, defaults generous for real supplier invoices. The teeth were in the dedup key. received_emails.message_id is varchar(512) UNIQUE, and the failure path wrote `err-<uid>-<Date.now()>`, which can never match the envelope-derived messageId the dedup pass compares against — so an oversized (or overlong-Message-ID) mail was re-downloaded every poll forever, and an OOM-kill/restart just resumed the loop. Size-skips are now recorded under the REAL message id, and overlong ids collapse to a stable sha256 key that always fits the column. GHSA-pgmp / r794 — new sanitizeForLog() util (key-name deny-set, recursive, cycle-safe) applied to the three request-body log sites in adminEvents/crud.js, plus sanitizeValidationErrors() because express-validator's errors.array() embeds the SUBMITTED value per field — a rejected plaintext password was still logged. Scope is wider than filed: the update path also logged client_password_hash and a LIVE client_share_token bearer credential. Also: the one-time setup token was logged at warn AND printed to stdout on every first boot, putting a live first-admin credential in combined.log, security.log and `docker logs`. It is now written to the 0600 token file and only surfaced when that write fails — the last-resort path it existed for. * fix(security): codex round 3 — repair the first-run token recovery flow (GHSA-r794) Two regressions from keeping the setup token out of the logs. 1. server.js decided whether to print the token by calling existsSync() on the candidate path. That answers a different question than "did the write succeed": a stale, read-only or directory-shaped SETUP_TOKEN reports as present, so the banner suppressed the live token and pointed the operator at content that is not it — leaving the current token only in combined.log under default production logging. setupService now records the path the write actually produced and exposes it via writtenSetupTokenFile(). 2. The setup screen, its EN/DE strings, README, SIMPLE_SETUP and .env.example all still told first-time users to run `docker compose logs backend | grep -i "setup token"`. On the normal path that command now returns a path banner and no credential, so the documented browser-first onboarding could not be completed. They now point at `docker compose exec backend cat /app/data/SETUP_TOKEN`, with the log fallback described as what it is — the failure path. Claude-Session: https://claude.ai/code/session_01F211U4dDbEj4zXiyKbi9me --------- Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul NothaftandPaul Nothaft
|
da855cfef9 |
fix(security): scope dashboard stats/analytics/activity to the caller's events (GHSA-c2jj, gqx7, jhcf) (#958)
* fix(security): scope dashboard endpoints to the caller's events (GHSA-c2jj, gqx7, jhcf)
/dashboard/stats, /analytics and /activity are gated only by analytics.view,
which the editor role holds — but the events LIST restricts editors to their
own rows (adminEvents/crud.js: roleName === 'editor' -> created_by =
admin.id). So an editor saw instance-wide totals, and via /analytics
topGalleries other admins' gallery NAMES and SLUGS (the public gallery URL
component), for events invisible to them everywhere else.
- stats: all 10 aggregates scoped (events by id, photos/access_logs by
event_id).
- analytics: all 8 series/aggregates scoped, including topGalleries. The
external tracker device breakdown reports instance-wide data with no event
filter, so a scoped caller falls through to the access_logs heuristic
instead, which IS scoped.
- activity: feed scoped. activity_logs.event_id is nullable and the join is a
leftJoin, so system-level rows (logins, settings changes) are deliberately
excluded for a scoped caller — those are precisely the cross-admin actions
the advisory is about.
Scoping keys on 'editor' to mirror the events list exactly, so the admin
role's dashboard is unchanged. filterOwnedEventIds uses the broader
'!== super_admin' rule; the two conventions disagree in this codebase and
matching the list is the no-regression choice.
* fix(security): codex round 2 — fix activity misattribution, scope via subquery (GHSA-jhcf, c2jj, gqx7)
- expenseService passed adminId as logActivity's THIRD positional parameter,
which is eventId — so admin ids were being written into
activity_logs.event_id. The /activity scoping filter trusts that column, and
admin/event id sequences overlap, so a foreign admin's expense metadata could
surface under an editor's event. All 11 calls now pass null for eventId and
the admin as the actor, which is what they meant.
- Dashboard scoping now uses a SUBQUERY instead of pluck()+whereIn. An editor
owning more events than the driver's bind-parameter limit (~999 SQLite,
65535 Postgres) would have turned all three endpoints into 500s once each id
became a placeholder; below the limit it still re-sent the full list for each
of the ~10 aggregates per request.
Note: two billInboundNow() calls also end in ', adminId)' but have an unrelated
signature — verified untouched.
* fix(security): codex round 3 — correct legacy accounting activity rows (GHSA-jhcf)
expenseService called logActivity(type, metadata, adminId), but logActivity's
third positional parameter is eventId. Every expense / incoming-invoice entry
therefore stored the ACTING ADMIN'S ID in activity_logs.event_id.
Round 2 scoped the activity feed with
`WHERE activity_logs.event_id IN (SELECT id FROM events WHERE created_by = me)`,
which does nothing about the rows already on disk. Admin ids and event ids are
small integers from the same range, so on any upgraded instance an editor who
owns the event whose id happens to equal another admin's id is served that
admin's accounting activity, verbatim metadata included — GHSA-jhcf, still
live. Migration 168 re-attributes those rows (event_id holds exactly the actor
id that was lost) and then clears event_id so the scope predicate can no longer
match them. All ten activity types are emitted by expenseService and nothing
else, so no row with a genuine event_id is touched.
Also: the round-2 rewrite passed `{ type: 'admin', id: adminId }`
unconditionally, which stored actor_type='admin' with a null id for the
automated mailbox intake (emailIntakeService calls recordInboundDocument with
no adminId). adminActor() restores 'system' attribution for those.
Claude-Session: https://claude.ai/code/session_01F211U4dDbEj4zXiyKbi9me
---------
Co-authored-by: Paul Nothaft <[email protected]>
|
||
|
Paul NothaftandPaul Nothaft
|
0d4c30884e |
fix(security): backup/restore hardening — public-dir DB dump, restore path allowlist, gunzip bound, manifest keying (#956)
* fix(security): stop caller-chosen database backup destination (GHSA-jw8m)
POST /api/admin/database-backup/backup forwarded req.body straight into
databaseBackupService.backup(), which merges options over config:
const { destinationPath = '/backup/database', ... } = { ...config, ...options }
destinationPath is not a persistable setting — the /config allowlist only
accepts database_backup_* keys — so the request body was its only source.
The built-in `admin` role holds backup.create but neither settings.edit nor
backup.restore, so it could aim a full DB dump (admin bcrypt hashes, gallery
password hashes, encrypted SMTP creds) at the PUBLIC /uploads static mount
(server.js mounts it with no auth middleware) and then fetch it
unauthenticated. Filed low; it is a privilege escalation to unauthenticated
disclosure.
Forward only the real knobs, and only when present so absent keys can't
override config defaults via spread.
* fix(security): backup/restore hardening — restore path allowlist, gunzip bound, manifest checksum keying (GHSA-fw4c, h652, hgp8)
- adminRestore /validate + /start: constrain caller-supplied source and
manifestPath to the operator-configured backup roots — the SAME set the
restore wizard discovers from — so disaster recovery from a rescued mount
still works, with RESTORE_ALLOWED_ROOTS as an escape hatch (GHSA-fw4c).
- restoreService.decompressFile: bound the EXPANDED size and abort the
pipeline when exceeded; default 50 GB, RESTORE_MAX_DECOMPRESSED_BYTES
overrides (GHSA-h652).
- backupManifest: BACKUP_MANIFEST_KEY upgrades new manifests to a keyed
HMAC (GHSA-hgp8). Deliberately opt-in and verify-if-present — the key
cannot live in the database because the database is inside the backup, so
a mandatory HMAC would lock operators out of the exact disaster-recovery
case this exists for.
Also fixes a pre-existing bug found while testing hgp8: the checksum passed
Object.keys().sort() as JSON.stringify's second argument, which is an array
REPLACER (a property allowlist applied at every depth), not a key sorter. All
nested keys — path, size, per-file checksum — were dropped before hashing, so
the file list sat outside the integrity check entirely and a manifest path
could be rewritten to ../../etc/passwd without disturbing the digest. Now
hashes a recursively-canonicalized copy, with the legacy serialization
accepted on validation so existing backups stay restorable.
* fix(security): codex round 2 — unbreak the restore wizard, share checksum verification, guard downgrades
- adminRestore: `source` is usually a SOURCE TYPE ('local'|'s3'|'upload'),
not a path — restoreService branches on those literals. The containment
check treated it as a path, so path.resolve('local') fell outside the
backup roots and BOTH /validate and /start returned 400, blocking every
normal restore. Type tokens are now excluded from the path check.
- backupManifest: extracted verifyManifestChecksum() as the single source of
truth for the legacy/keyed fallbacks. restoreService.performPreRestoreValidation
recomputed the digest itself with the default canonical+keyed settings,
which rejected EVERY backup written before this batch. It now delegates.
- backupManifest: guard the algorithm downgrade — with a key configured, an
attacker able to rewrite the backup store could strip checksum_algorithm,
edit the manifest and recompute a plain SHA-256 that verified. Opt-in via
BACKUP_MANIFEST_REQUIRE_KEYED so pre-key backups keep restoring by default.
* fix(security): codex round 3 — close two manifest-verification fail-opens (GHSA-hgp8)
verifyManifestChecksum returned valid for a manifest with no
verification.total_checksum at all, and restoreService only called it when
that field was present. Deleting the field was therefore a complete bypass of
the keying work: no digest check, no downgrade guard, no
BACKUP_MANIFEST_REQUIRE_KEYED. Every manifest this codebase writes stamps the
field, so an absent one now fails validation, and the call site invokes the
verifier unconditionally.
Second fail-open: the strict-mode rejection of an unkeyed manifest was gated on
`&& key`, so with BACKUP_MANIFEST_REQUIRE_KEYED=true and no BACKUP_MANIFEST_KEY
configured a plain SHA-256 manifest sailed through. Strict mode is a statement
about the operator's manifests, not about the host — it is exactly the fresh
disaster-recovery box that lacks the secret. The rejection no longer depends on
a key being present.
Claude-Session: https://claude.ai/code/session_01F211U4dDbEj4zXiyKbi9me
---------
Co-authored-by: Paul Nothaft <[email protected]>
|
||
|
Paul NothaftandPaul Nothaft
|
c2ce12c039 |
fix(security): authz/ownership gaps (token binding, auth revocation, feedback/customer ownership, token logging) (#950)
* fix(security): close authz/ownership gaps (secure-download binding, photo-auth+logout revocation, feedback/customer ownership, token logging) * fix(security): codex round-1 — complete admin-token invalidation + preserve foreign assignments - photoAuth: mirror adminAuth's active-admin lookup + iat<password_changed_at check in the admin branch, so a deactivated admin or a pre-password-change token can no longer fetch every photo (GHSA-x55x was only revoke+cutoff). - adminAuth logout: revoke req.token (the token adminAuth authenticated with, cookie OR header) instead of header-only, and clear the auth cookie — a cookie-based logout previously left the JWT live (GHSA-cjqh). - adminCustomers PUT /:id/events: preserve the customer's existing assignments to events the caller does NOT own, so a restricted admin can't revoke another admin's customer-event links via full-list replacement. * fix(security): codex round-2 — don't 403 legit restricted-admin assignment edits The Manage-galleries dialog submits the full initial assignment list, so a restricted admin editing a customer that already has a foreign assignment hit the denied.length 403 before the preservation logic ran. Reject only NEWLY-supplied foreign/nonexistent ids; retain foreign ids the customer is already assigned to (they can't be added or removed by a non-owner). --------- Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul NothaftandPaul Nothaft
|
8f91c2ca99 |
fix(security): neutralize spreadsheet formulas in all CSV/export cell-writers (CSV injection cluster) (#948)
Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul NothaftandPaul Nothaft
|
9050affd8d |
fix(security): unauth share_token leak (HIGH) + restore path-traversal, logo file-read, branding path keys (#946)
* fix(security): stop unauth share_token leak + block restore path-traversal, logo-path file read, branding path keys * test: update resolveLogoFile for the c7x5 containment (reject outside-storage absolute paths, keep inside) * fix(security): codex round-1 — escape LIKE wildcards in share-link resolve, keep in-storage absolute logos, guard restore verification - shareLinkService: escape %/_ in the link_partial LIKE fallback so an anonymous /resolve/____… wildcard can't match an arbitrary share_link and leak its bearer token (reopened GHSA-rh8r). Explicit ESCAPE for SQLite. - resolveLogoFile: re-add the raw absolute candidate but keep it subject to the storage-root containment filter (GHSA-c7x5) so legit in-storage absolute logos resolve while /etc/passwd stays rejected. - restoreService: apply the same pathEscapes guard in post-restore verification so a skipped traversal entry isn't fs.access'd/hashed. --------- Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul NothaftandPaul Nothaft
|
82d68711cf |
fix(security): close authorization/ownership gaps (token scope, mass-assignment, category hero, project docs) (#943)
* fix(security): close authorization/ownership gaps (token scope, mass-assignment, category hero, project docs) * fix(security): block archive columns in event mass-assignment per review * fix(security): comprehensive event mass-assignment denylist + deal-cascade cross-domain permission gate (codex r2) * fix(security): case-insensitive complete event denylist + project_id + empty-update no-op (codex r3) --------- Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul NothaftandPaul Nothaft
|
b7005692b3 |
fix(security): resolve DNS before vetting external hostnames (SSRF cluster) (#941)
* fix(security): resolve DNS before vetting external hostnames (SSRF cluster) * fix(security): harden SSRF fix per review (rsync backup path, S3 config-save, webhook transient-DNS retry) * fix(security): S3 endpoint validation on any endpoint update + no-connect on unresolved webhook host (codex r2) --------- Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul NothaftandPaul Nothaft
|
8a87c9274b |
fix(security): block guest access to hidden/client-only photos across bulk + secure routes (#939)
* fix(security): block guest access to hidden/client-only photos across bulk + secure routes * fix(security): harden hidden-photo fix per review (stale ZIP cache, legacy token mint, SQLite bool, client rebuild) * fix(security): invalidate ZIP cache on photo visibility/category change (codex r2) * fix(security): recheck photo visibility at signed/secure serve time (TOCTOU) + invalidate ZIP on client visibility change (codex r3) --------- Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul NothaftandPaul Nothaft
|
defeae9634 |
fix(uploads): prevent cross-photo contamination from filename collisions and non-atomic writes (#931) (#933)
* fix(uploads): prevent cross-photo contamination from filename collisions and non-atomic writes (#931) * test: pin the suffixed photo filename format in the NFD pipeline suite (#931) * test: make the suffix-uniqueness check deterministic-in-practice (#931) * fix(uploads): widen the anti-collision suffix to 48 bits (#931) * fix(uploads): hide staging files from list() + share one watermark limiter process-wide (#931) * fix(uploads): reclaim orphaned staging files + revalidate watermark settings in queued jobs (#931) --------- Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul NothaftandPaul Nothaft
|
08ff9f20e7 |
feat(gallery): per-event toggle to hide the logo on the password page (#894) (#928)
* feat(gallery): per-event toggle to hide the logo on the password page (#894) * fix(admin): harden login_logo_visible coercion for SQLite + string booleans (#894) --------- Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul NothaftandPaul Nothaft
|
03087c798c |
fix(security): close GHSA-g94x (cross-gallery photo read) + GHSA-pv6w (admin DB export) (#924)
* fix(security): close two access-control advisories (GHSA-g94x, GHSA-pv6w) GHSA-g94x-8vv8-3c9f (HIGH) — the secure-image VIEW route (/secure-images/:slug/secure/:photoId/:token) validated only the token signature and took the gallery/photo from the URL, so a token minted on any PUBLIC gallery read every other gallery's photos with no password (its download sibling has verifyGalleryAccess; the view route can't — it serves via <img src> with no header). Bind the token to its scope instead: the URL photoId must equal the token's minted photoId (photos belong to exactly one gallery, and minting is gallery-scoped), and the gallery embedded in the token's sessionId must equal the URL gallery. GHSA-pv6w-rj34-wj9v (MEDIUM) — GET /admin/backup/picpeak/export dumps every table unredacted (bcrypt hashes, 2FA, SMTP/SSO/WhatsApp/webhook/S3 secrets) and was gated only by backup.create, which the built-in admin role holds. Gate it behind super_admin, matching the restore side (backup.restore, already admin-denied) and the masked config APIs. Regression tests pin both: cross-gallery token reads 403 (photo and gallery checks), backup export 403 for admin / passes for super_admin. * test: stub requireSuperAdmin in the backup masking mock adminBackup now calls requireSuperAdmin() at load (GHSA-pv6w export gate), and backupSecretMasking mocks the permissions module — add the new function to the mock so the module loads. * fix(security): review follow-ups on the export gate (GHSA-pv6w) - test: place the mocked export in its own mkdtemp dir. The route recursively deletes path.dirname(filePath) after download, so a stub in bare os.tmpdir() made the super_admin test wipe the whole temp root — other jest workers' DB files included (latent CI flake). - ui: hide PicpeakExportCard from non-super_admins. The role keeps settings.view + backup.create, so after the gate its Download button always 403'd with a generic toast; gate the card on role super_admin to match the endpoint. * fix(security): keep the token-mismatch audit values within varchar(20) (GHSA-g94x review) image_access_logs.access_type is varchar(20) (migration 038), but 'token_gallery_mismatch' is 22 chars — on Postgres the audit write threw value-too-long and logImageAccess swallowed it, so the security event went unrecorded (the 403 still fired; log is best-effort). Shorten to 'photo_mismatch' / 'gallery_mismatch' (14/16). --------- Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul NothaftandPaul Nothaft
|
252475fce2 |
fix(admin): code-review follow-ups on #910/#916 (MIME resolver + expiry reactivity) (#921)
* fix(admin): own-property lookup in the extension MIME map (#908 review round) A client-controlled filename ending in .constructor / .__proto__ / .toString made EXTENSION_TO_MIME[ext] return an inherited Object.prototype member (truthy), and the downstream extMime.startsWith threw — a permanent 500 on the admin view for that photo instead of the JPEG / mp4 fallback. hasOwnProperty-gated now; test pins both a .constructor image and a .__proto__ video. * fix(admin): drop already-expired events from the dashboard card (#909 review round) The expiring-soon card ran Math.max(1, ceil(delta)), so an event that expired while the dashboard sat open (its query isn't polled) showed '1 day left' indefinitely from the stale cached row. Expired rows are now filtered out before render; the delta is therefore always positive and the clamp is gone. * fix(admin): honor safe stored image MIME for auto-imported formats (#908 review round 2) My previous round made the image side map-only to dodge the migration 039 image/jpeg backfill and image/svg+xml — but that regressed the S3 auto-importer (STORAGE_AUTO_IMPORT), which stores correct types for avif/bmp/tiff/heic whose extensions aren't in EXTENSION_TO_MIME. Those now served as image/jpeg (JPEG-labelled non-JPEG bytes). Precedence is now mapped-extension (still corrects the 039 backfill on PNGs) -> stored MIME IF in a safe raster allowlist (avif/bmp/tiff/heic + the mapped ones) -> image/jpeg. Allowlist, not a regex: image/svg+xml stays excluded (scriptable inline). Tests pin avif preserved and svg degraded to jpeg. * fix(admin): refresh expiry status live at the boundary (#909 review round 2) Two review findings on the admin expiry surfaces: - The dashboard 'expiring soon' card, list badges, and detail banner are all computed inline from Date.now() at render, so a page left open across an event's expiry kept showing 'active'/'1 day left' until an unrelated render — which for editor/viewer roles (no health poll) never happens. - My round-1 client-side filter on the dashboard desynced the visible list from the cached total/stat ('no events expiring' beside 'view all N'). Both are fixed by new useExpiryRefresh: it fires once at the soonest future expiry (setTimeout, overflow-guarded). The dashboard refetches its expiring + stats queries — the backend already excludes expired events, so rows/total/stats come back consistent (filter removed). The list and detail pages bump a tick so the inline badges recompute. Hooks are placed above the loading early-returns (rules-of-hooks is disabled in eslint, so this was a latent crash otherwise). * fix(admin): allow any header-safe raster MIME, deny svg/xml (#908 review round 3) The round-2 hand-listed Set kept missing formats the S3 auto-importer stores (apng/ico/jxl beyond avif/bmp/tiff). Replace it with a regex: honor image/<token> EXCEPT the scriptable svg / *+xml family. Covers every current and future raster type in one rule while still blocking inline-scriptable svg and header injection. Tests pin apng + x-icon preserved, svg still degraded to jpeg. * fix(admin): expiry-refresh precision + filtered refetch (#909 review round 3) Three refinements to round-2's live-expiry work: - useExpiryRefresh now re-arms past setTimeout's ~24.8-day overflow limit (capped wake-up that re-evaluates) instead of dropping the timer, so a page mounted for weeks still updates. - The dashboard requests the expiring list ordered by expires_at asc, so the five shown rows ARE the soonest to expire — the timer schedules against the true next boundary even when >5 events are expiring (getEvents gains optional sortBy/sortOrder; backend already whitelists expires_at). - EventsListPage refetches instead of only re-rendering at the boundary: under the 'expiring' filter the backend drops expired rows, so a plain tick would leave a stale 'Expired' row + total. refetch keeps rows and totals correct under every filter. --------- Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul NothaftandPaul Nothaft
|
aca3c8e4bc |
fix(admin): expose view/download counters in the admin photos list (#895 follow-up) (#914)
* fix(admin): expose view/download counters in the admin photos list (#895 follow-up) st-ivan's re-test after #904: statistics panel and event summary now agree, but the per-image Engagement column still shows 0. Root cause: the admin photos LIST endpoint maps rows to an explicit response object that includes like/comment/rating/favorite counts but never included view_count or download_count — the grid reads photo.view_count ?? 0, so the column showed 0 regardless of what the DB counted. This mapper, not stale data, is also why per-image downloads always displayed 0 in the original report. Suite extended with a list-endpoint assertion (beacon + download, then the admin list reflects 1/1 and untouched photos 0/0). The skip test now neutralizes the route's background pre-zip build, whose async ENOENT against the intentionally missing file could land mid-suite. * test: widen the fire-and-forget settle window (#895 follow-up) The 100ms settle was marginal on loaded CI runners — the counter increments are deliberately fire-and-forget, and the 909 PRs flaked on exactly these assertions. 400ms keeps the suite fast while giving slow runners room. --------- Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul NothaftandPaul Nothaft
|
67c56c5b61 |
fix(admin): serve videos with their real MIME type in the admin photo view (#908) (#910)
* fix(admin): serve videos with their real MIME type in the admin photo view (#908) The admin view route built Content-Type from the filename extension — image/<ext> — which is invalid for videos (image/mp4). The admin player fetches this URL into a blob that inherits the type, and browsers refuse to play a <video> blob labeled image/*: blank/grey preview, while download (which already uses photo.mime_type) worked fine. Stored mime_type now wins; videos without one fall back to video/mp4, images to the extension, and extensionless files to image/jpeg instead of the equally invalid bare 'image/'. Also unrefs chunkedUploadService's module-level hourly cleanup interval: it kept Jest from exiting for any suite requiring adminPhotos (it's why adminPhotos.reference sits on the CI ignore list). Production behavior unchanged — the HTTP listener keeps the process alive. New adminPhotoContentType suite pins all four MIME cases. * fix(admin): harden admin photo Content-Type resolution (#908 review round) External review findings, all verified: - The header is now ALWAYS image/* or video/*. photos.mime_type is never echoed verbatim unless it is a video/ type — the chunked-upload path stores the client-sent MIME unvalidated, so a stored text/html served inline under the app origin was a same-origin XSS hazard. - MIME-less videos map from the extension via the shared EXTENSION_TO_MIME (.mov → video/quicktime, .webm → video/webm) instead of a blanket video/mp4 that would mislabel them. - Images ignore the stored MIME entirely: migration 039 backfilled image/jpeg onto every legacy row (PNGs included), so trusting it would regress previously-correct extension-derived types. Extension wins, normalized (jpg → image/jpeg). Suite extended to 8 MIME cases including the XSS guard and the 039-backfill immunity. * fix(admin): validate stored video MIME as a full header-safe token (#908 review round 2) A prefix check let malformed client-stored values through: 'video/mp4\r\nX: y' makes res.setHeader throw ERR_INVALID_CHAR — a permanent 500 for that photo — and a bare 'video/' is an invalid type. Strict /^video\/[\w.+-]+$/ now gates the stored value; anything else falls back to the extension map. Two new tests pin both shapes. * fix(admin): map-only image Content-Type — no raw extension interpolation (#908 review round 3) image/${ext} could synthesize image/svg+xml (scriptable when served inline) or header-invalid values from client-controlled chunked-upload filenames. The shared EXTENSION_TO_MIME map is now the allowlist on the image side too; unmapped extensions serve as image/jpeg — browsers sniff image bytes in img/blob contexts, so a mislabel is harmless where an injected type is not. --------- Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul NothaftandPaul Nothaft
|
78116e2e8b |
fix(analytics): make per-photo view/download counters actually count (#895) (#904)
* fix(analytics): make per-photo view/download counters actually count (#895) Three stacked defects behind 'per-image stats stay 0': - photos.view_count had NO writer anywhere — the admin IMAGES table and photo viewer display it, so it was permanently 0. It now increments when the full-size photo or its preview tier is served, excluding the slideshow kiosk (migration 138 design) and follow-up video Range requests (seeks are not views). Fire-and-forget so analytics can never fail the byte-serving path. - Zip downloads (download-all, presigned download-all, download-selected) never incremented per-photo download_count — only single-photo downloads did, so zip-heavy galleries showed 0 forever. The zip routes now bump exactly the photos that went into the archive (the prebuilt-zip path mirrors the archive builders' category filter). - Every admin surface used a different definition of 'downloads', which is the reporter's 46 vs 45 vs 0: event details counted only action='download' (no zips at all), the dashboard counted download+download_all but silently EXCLUDED download_selected and download_all_presigned. All queries now share one action set: download, download_all, download_all_presigned, download_selected. New photoEngagementCounters suite pins all of it (7 tests). * fix(analytics): count views via an explicit lightbox beacon (#895 review round) External review flagged that request-level view counting is wrong in both directions: the lightbox preloads prev/next neighbours (3 fetches per open) while a preloaded neighbour promoted by a swipe is never re-fetched (#505 keeps the DOM node), and enhanced/maximum galleries never hit /photo at all (bytes come from /api/secure-images). - Views now count via POST /:slug/photo/:photoId/view, fired by the lightbox exactly when a photo becomes the visible slide; the serving-route increments are removed. Covers protected galleries and the preview tier uniformly; slideshow kiosk stays excluded. - bumpEventDownloadCounts mirrors downloadZipService._build (ALL event photos) — the category filter mismatched the prebuilt zip's actual contents. (That the builder ignores per-category allow_downloads is a separate pre-existing issue.) - Zip loops count only successfully appended entries, with a pre-append storage stat: a lazy stream's async error bypassed the per-photo catch and hung the whole response — pre-existing bug, now fixed. Suite extended to 9 tests (beacon semantics, serve-does-not-count, skipped-entry exclusion). * fix(analytics): fire the view beacon from the premium lightbox too (#895 review round 2) gallery-premium events use yet-another-react-lightbox inside GalleryPremiumLayout instead of PhotoLightbox, so the layout never counted views. yarl's on.view fires on open and on every slide change — identical semantics to the PhotoLightbox beacon. Also documents the accepted prebuilt-zip approximation: _build can skip entries whose watermark step fails and still publish the archive; counting those exactly would need a persisted zip manifest. * perf(analytics): skip the per-entry zip preflight on S3 (#895 review round 3) The pre-append source check exists for LocalFs's lazy createReadStream (async error would kill the whole zip response). S3's get() awaits GetObject and rejects inside the loop's try/catch on a missing key, so a HEAD per entry was a redundant serial round trip — 500 extra HEADs on a 500-photo zip. --------- Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul NothaftandPaul Nothaft
|
6a048d08bd |
feat(feedback): let guests remove their star rating (#884) (#893)
* feat(feedback): let guests remove their star rating (#884) Clicking your current rating again clears it. rating: 0 is the wire contract: the validator now accepts 0, and the service deletes the guest's rating row (instead of storing a 0 that would drag the photo average down) and recalculates photo stats. The lightbox stars send 0 on a same-star click; PhotoRating already did, but the backend rejected it with a 400 until now. * fix(feedback): harden the rating-clear path (#884 review round) External review follow-ups: numerically normalize the clear sentinel so a numeric-string "0" can't slip into the update/insert paths (validator now also toInt()s), delete the full guest-scoped rating set on clear so racy duplicate rows can't survive in the average (same defense as the reaction path), and refresh the visible average/count after the identity-modal submit path like the direct paths do. * fix(feedback): round-2 review fixes for rating clear (#884) - Clear sentinel matches only an explicit 0 / "0" — malformed input (undefined, NaN, garbage strings) can no longer delete a rating. - Lightbox survives the photo list shrinking while open (clearing your rating under the Rated filter drops the photo on refetch): index is re-anchored and the lightbox closes when the list empties, instead of crashing on an out-of-range index. - Story layout gets the same same-star-to-clear behavior, keyed off the session-local my-rating map, and an explicit 0 no longer falls back to displaying the photo average. * fix(feedback): refresh guest-scoped caches after rating changes (#884 review round 3) - GalleryView's onFeedbackChange now also invalidates ['my-feedback', slug]: in guest identity mode the Rated/Liked filter membership and chip counts come from that query (#538), so a cleared rating never left the Rated filter until the 30s staleTime lapsed. - PhotoRating invalidates gallery-photos + my-feedback on success: the parent refetch fires optimistically in onMutate and could capture pre-mutation state, with nothing refreshing after the server accepted. --------- Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul Nothaft
|
08be2b84f1 |
fix(security): close 5 Trivy alerts — postcss/tar bumps + drop npm from the runtime image (#878)
* fix(security): close the 5 open Trivy alerts — dep bumps + drop npm from the runtime image Backend deps: - postcss 8.5.10 -> 8.5.18 (CVE-2026-45623, GHSA-r28c-9q8g-f849; the pin exists to force sanitize-html's transitive copy onto a fixed version) - tar pin/override >=7.5.16 -> >=7.5.21, resolves 7.5.22 (GHSA-r292-9mhp-454m) Runtime image: - Remove the npm CLI from the final stage instead of upgrading it: npm's bundled node_modules ship tar 7.5.19 and brace-expansion 5.0.7 (no npm release bundles the fixed versions — checked 11.18.0 and 12.0.1), and npm never runs in production. wait-for-db.sh now invokes the migration runners via node directly. This ends the recurring npm-bundled-CVE alert class; the previous 'npm install -g npm@11' line was itself a patch for the last batch. * fix(restore): run post-restore migrations via node — the image ships no npm restoreService still shelled out to 'npm run migrate:safe' after a restore; with npm removed from the runtime image that would ENOENT into the non-fatal catch, silently leaving a restored older backup on a schema behind the running code until the next container restart. Invoke migrations/run-migrations-safe.js through node directly, matching wait-for-db.sh. The PR #596 source-contract test now pins the new invocation. |
||
|
Paul Nothaft
|
a2e723413e |
fix(backup): make backup settings actually apply (#871) (#874)
* fix(backup): make backup settings actually apply (#871) - Wire the What-to-Backup toggles into the walker: honor backup_include_thumbnails / backup_include_photos (opt-out, default ON) and accept the UI's backup_include_archives spelling for the archived gate (the engine expected _archived, so the Archives checkbox silently never worked). - Fix the 167.6 TB dashboard size: file_size_bytes is a bigint that node-postgres returns as a string, and the S3 path concatenated it onto the byte counter; coerce to Number at the source. - Compute the real next scheduled run (cron-parser) and return it as nextBackup; the UI read a field the API never sent and rendered a hardcoded 'Not scheduled'. A named schedule label now beats the stray default cron the UI always sent, which silently turned weekly schedules into daily 03:00 runs. - Never back up filesystem noise (.nfs* silly-renames, .DS_Store, Thumbs.db) and honor backup_exclude_patterns in the walker (previously rsync-only). - Remove the compression/encryption toggles from the configuration UI: no backend implementation exists, and collecting an encryption passphrase while uploading plaintext is a false promise. Closes #871 * fix(backup): close the review gaps in the settings wiring - The UI's backup_include_archives now beats the migration-seeded backup_include_archived: every install has the singular key seeded true, so the alias-only-when-absent lookup made unchecking Archives a no-op. - rsync destinations now receive the de-selected What-to-Backup paths and the noise filters as anchored --exclude args; previously rsync synced the whole storage root and the walker's selection only shaped the manifest, which then misreported what was actually transferred. - Escape regex metacharacters in the walker's glob matcher: '.nfs*' compiled to /^.nfs.*$/ whose leading dot matched any character, so files like anfs-photo.jpg were silently dropped from backups. - The Backup Coverage report now uses the same gate as the walker (new 'skipped-by-setting' status) instead of re-implementing it without the opt-out toggles and the archives alias. * fix(backup): make the coverage diagnostics agree with the walker - The coverage table shows the alias-aware flag value the gate actually used, instead of the seeded backup_include_archived shadowed by the UI's plural key (true next to a 'Gated off' badge). - skipped-by-setting paths are now counted in the coverage summary (backend, TS contract, summary card, EN/DE locales) so the totals reconcile again when Photos or Thumbnails is unchecked. - The form's thumbnail default now matches the backend's never-saved fallback (include): the checkbox no longer shows 'off' while thumbnails are being backed up, and saving an unrelated setting no longer flips the backup scope. * fix(backup): keep custom crons, exclude disabled rows from rsync, normalize flag display - Saving a named schedule no longer wipes the stored custom cron: the backend already prefers the label, so the cron field stays inert for named schedules and is preserved for switching back to Custom. A custom schedule now validates the 5-field expression before saving (the backend silently fell back to daily 02:00 on a blank value). - resolveExcludedBackupPaths now also returns rows disabled via include_in_default, so rsync excludes them; the enabled-only loader hid them and rsync transferred their contents anyway. - The coverage table normalizes flag values like the walker does — Boolean('false') displayed true beside a gated-off badge. |
||
|
Paul Nothaft
|
38b8d476d1 |
fix(security): bump backend deps to close all 14 open Trivy code-scanning alerts (#869)
* fix(security): bump backend deps to close all open Trivy code-scanning alerts - axios 1.16.0 -> 1.18.1 (GHSA-gcfj-64vw-6mp9 high + 10 medium advisories) - sharp 0.34.3 -> 0.35.3 (GHSA-f88m-g3jw-g9cj, inherited libvips CVEs) - mailparser 3.9.9 -> 3.9.14 (pulls linkify-it 5.0.2, CVE-2026-59887) - brace-expansion override >=5.0.6 -> >=5.0.7 (CVE-2026-13149) - body-parser 1.20.4 -> 1.20.6 via lockfile refresh (CVE-2026-12590) * fix(images): migrate removed sharp failOnError option and enforce Node >=20.9 sharp 0.35 drops the deprecated failOnError constructor option, so recoverably corrupt images would start failing upload validation and thumbnail generation; use the failOn: 'none' equivalent instead. sharp 0.35 also requires Node >=20.9: declare it in engines and make picpeak-setup.sh compare the full version instead of only the major, so native installs on Node 20.3-20.8 upgrade instead of breaking. * fix(setup): align the Node floor with the whole dependency tree and gate native updates html-to-text@10 needs Node >=20.19 and the glob/minimatch family excludes Node 21, so declare engines as ^20.19.0 || >=22 and enforce the same range in picpeak-setup.sh. Also run install_nodejs at the start of update_native_installation so existing native installs on an old Node get upgraded before the service is stopped, instead of restarting broken. * fix(setup): make the update-path Node gate actually work --update dispatches before detect_os, so install_nodejs saw an empty PACKAGE_MANAGER, matched no install branch, and reported success on the old runtime. Detect the OS on demand and re-verify the installed version afterwards, failing loudly (before the service is stopped) when the runtime still misses the engines range, e.g. a Node 21 that package managers refuse to downgrade. |
||
|
Paul NothaftandPaul Nothaft
|
219d07b04a |
feat(auth): OIDC logout-to-IdP — phase 3 (#798) (#865)
* feat(auth): OIDC logout-to-IdP — phase 3 (#798) RP-initiated logout behind a new oidc_logout_from_idp setting: logging out of PicPeak also ends the IdP session. The SSO callback stores the raw ID token in an HttpOnly cookie (also the marker that the session came in via SSO — local-password sessions never bounce to the IdP); /logout builds the end_session URL from discovery metadata with id_token_hint + post_logout_redirect_uri + client_id and returns it as ssoLogoutUrl for the frontend to navigate to. Any failure (no end_session_endpoint, IdP unreachable, feature off) degrades to the plain local logout. Settings surface exposes the toggle plus the computed post-logout redirect URI to register at the IdP. Session timeouts deliberately stay local-only. 6 integration tests over the mock IdP; live-verified against Keycloak 26 (logout ends the Keycloak session, no confirmation prompt). * fix(auth): harden the SSO logout marker cookie (#798 phase 3) Codex review round 1: - Derive the oidc_id_token cookie options from the shared cookie policy (COOKIE_SAMESITE / COOKIE_DOMAIN / secure resolution) — hardcoded Lax meant split-origin deployments running on SameSite=None never sent the marker to the cross-site /logout XHR, silently disabling logout-to-IdP. - Oversized ID tokens (>3.9KB) now store a bare 'sso' marker instead of no cookie, so the claimed client_id-only end-session fallback actually happens; /logout only passes the value as id_token_hint when it is a real JWT. - establishAdminSession clears any stale marker on every fresh login — sessions can die without /logout (deactivation, expiry, restore), and a surviving marker would bounce a later local-password session to the IdP. The SSO callback re-sets the marker for its own session. Tests: oversized-token marker + hint-less end-session URL, stale-marker cleared on local login; helper updated for the clear+set cookie pair. * fix(auth): validate the logout hint against the current OIDC config (#798 phase 3) Codex review round 2: an ID token stored at login can outlive an issuer/client config change; sending it to the newly configured IdP as id_token_hint strands the user on the IdP's error page (providers validate iss/aud on the hint). buildEndSessionUrl now decodes the hint (no verification — routing only): different issuer → skip the round-trip entirely (the session belongs to another IdP); same issuer but changed client → keep the round-trip, drop the unusable hint. Two tests pin both paths. --------- Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul NothaftandPaul Nothaft
|
323dcae917 |
fix(gallery): block password form in Instagram in-app browser and unmask login errors (#863)
* fix(gallery): block password form in Instagram in-app browser (#654) Field reports show gallery password login still failing inside Instagram's IAB after the #656 input-attribute/trim defenses. Three changes: - Replace the advisory amber banner with a red blocking state: the password form is hidden in the Instagram IAB and replaced with platform-specific "open in external browser" instructions plus a copy-link button (clipboard API with execCommand fallback). A "try anyway" link restores the form as an escape hatch. - Stop masking non-password failures as "incorrect password": a request that never got a response (offline, webview killed it) now reports a connection error, and a reCAPTCHA 400 reports a verification failure — both previously fell through to the wrong-password message and sent guests chasing the wrong cause. - Strip invisible Unicode (zero-width chars, word joiner, BOM, soft hyphen) from the submitted password in addition to trimming — these ride along when the password is copy-pasted out of a chat app and fail byte-exact bcrypt compare server-side. * fix(gallery): retry login with typed password + honor execCommand result (#654) Codex review round 1: - Stored passwords can legitimately contain the invisible code points the sanitizer strips (e.g. ZWJ emoji sequences) — creation paths don't normalize. On a 401 where the sanitized form differs from the typed (trimmed) input, retry once with the typed value. Skipped when a reCAPTCHA token is in play (single-use). - document.execCommand('copy') signals failure via its return value, not by throwing — only show "Link copied" when it returns true. * fix(gallery): move invisible-char password fallback server-side (#654) Codex review round 2: the client-side retry either burned the single-use reCAPTCHA token (making exotic-but-valid passwords impossible to enter with reCAPTCHA on) or burned failed-attempt lockout quota on every rescued login. Doing the fallback as a second bcrypt compare inside the same gallery/verify request eliminates both: exact bytes are compared first (stored passwords containing e.g. ZWJ emoji keep working), the sanitized form only on mismatch, and trackFailedAttempt only fires when both fail. Frontend goes back to plain trim-on-submit; the client-side sanitizer util and retry are removed. 7 integration tests pin the contract. --------- Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul NothaftandPaul Nothaft
|
c6ec93eef9 |
fix(dates): normalize SQLite epoch timestamps at remaining API surfaces (#485 follow-up) (#857)
The audit #485 called for: on SQLite (native installs), timestamp columns written with a raw `new Date()` through knex store epoch-ms numbers; Postgres returns ISO strings. Frontend code written against Postgres calls parseISO() on them — parseISO(number) throws and crashes the page. #485 fixed admin Users and listed api tokens / photos / activity as out-of-scope follow-ups. Verified crash on main: Timeline gallery layout parseISO(uploaded_at) against photos written by the archive-RESTORE path (raw Date). Other raw-write surfaces (api_tokens last_used_at/revoked_at, email_queue) degrade rather than crash but violate the ISO contract. - extract toIso() from adminUsers.js into utils/dateNormalize.js (contract unchanged — the 10 existing #485 tests still pin it) - write-side: archive-restore uploaded_at, api-token last_used_at / revoked_at, email_queue created_at/sent_at now write ISO strings - read-side (heals existing corrupted rows): gallery /photos normalizes uploaded_at/captured_at; api-tokens list normalizes all four timestamp fields - frontend defence-in-depth: Timeline layout parses uploaded_at tolerantly (typeof guard) for stale caches / old backends - 2 regression tests seed literal epoch numbers and assert the API serves ISO strings activity_logs turned out safe (created_at comes from the DB default, not a raw Date) — left untouched. Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul NothaftandPaul Nothaft
|
2f05fcc39d |
feat(gallery): reveal mode — hide gallery from guests until reveal (#838) (#856)
* feat(gallery): reveal mode — hide gallery from guests until reveal (#838) Guests can upload during the event but see no photos until the host reveals the gallery, manually ("Reveal now") or at a scheduled time. - migration 165: events.reveal_mode / reveal_at / revealed_at. Effective visibility is computed at REQUEST time (reveal_at <= now opens the gate exactly on schedule); the minutely scheduler only stamps revealed_at durably and emits a gallery.revealed workflow trigger - server-side enforcement in gallery.js: /photos returns the event shell with photos: [] + hidden_until_reveal for plain guests; image/download/stats endpoints 403 with GALLERY_HIDDEN (photo IDs are sequential — listing-only gating would be probeable); feedback-summary gated too. Slideshow tokens (surprise beamer), client access and the admin preview bypass; the guest upload route stays open - admin: reveal toggle + optional scheduled datetime next to the guest upload settings, status line and "Reveal now" button on the overview; re-enabling the toggle clears revealed_at so a gallery can re-hide - guest UI: upload-only view (hero, friendly message, scheduled time, upload button) for every layout; i18n for all 8 locales - timestamps written as ISO strings — the SQLite driver stringifies raw Date objects into garbage; ISO round-trips on both engines - 14 integration tests over minted gallery/slideshow/client/admin tokens * fix(gallery): reveal/re-arm semantics + upload button i18n key (#838) - "Reveal now" also clears a pending reveal_at: the schedule is consumed, so the full-form admin save can't accidentally re-hide a revealed gallery with a stale future date - setting a FUTURE reveal_at on a revealed gallery re-arms hiding — the one intentional way to re-hide without double-toggling the mode - guest upload button uses the existing upload.uploadPhotos key (gallery.uploadPhotos never existed; the button showed EN everywhere) * fix(gallery): close reveal bypasses from review round 1 (#838) - the hero-derivative route and the secure-images token-mint + secure-download routes are now reveal-gated: hero serves a 1920px derivative of ANY sequential photo id and secure tokens fetch originals — both were open bypasses while hidden. blockHiddenGallery moved to utils/revealMode.js and shared - customer-portal tokens (via:'customer', no accessLevel) now bypass reveal mode — they are the host/customer, not a guest, and were getting the upload-only view - an open hidden guest view refetches exactly at reveal_at plus a 60s fallback poll, so the gallery appears without a manual reload - gallery.revealed added to the workflow editor's trigger picker so the advertised notification hook is reachable in the UI - migration 165 guards each column independently (partial-state safe) * fix(gallery): reveal round 2 — remaining bypass surfaces + lifecycle edges (#838) - legacy /api/images router reveal-gated (view, secure-token + signed-url minting), and the signed-URL SERVE path re-checks hidden state via a backward-compatible bypass flag in the token payload - secure-image tokens record revealBypass at mint and are re-validated at serve time — a re-hide kills in-flight guest tokens within the request, while slideshow/client tokens keep working - OG metadata and the unauthenticated /og cover fall back to the brand logo / 404 while hidden — no hero-photo spoiler for social crawlers - photo-feedback GET/POST reveal-gated (sequential ids were enumerable); /my-feedback returns the empty back-compat shape (rows leak filename + storage path) - the reveal scheduler skips drafts — no premature stamp/notification for unpublished galleries - emitWorkflowEvent gains an additive dedupSuffix; both reveal emitters pass the reveal timestamp so a re-hidden gallery's second reveal fires workflows again instead of deduping into silence * fix(gallery): reveal round 3 — schedule consumption + two-way client sync (#838) - the scheduler now consumes reveal_at when stamping (matching "Reveal now"), and re-arming via a partial API update clears a stale PAST schedule — previously {reveal_mode:true} without reveal_at could instantly re-open the gate through the leftover date - /photos exposes reveal_armed so an open VISIBLE gallery keeps a 60s poll while the mode is on — a re-hide now propagates to open clients in both directions, not just hidden→visible Codex round-3 claim about timestamp-without-timezone drift on non-UTC Postgres was verified FALSE: knex's table.timestamp() creates timestamptz on PG (confirmed via information_schema on a live install), which stores absolute instants regardless of server TZ. --------- Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul NothaftandPaul Nothaft
|
3d6c9848dc |
feat(feedback): emoji reactions on photos (#839) (#855)
* feat(feedback): emoji reactions on photos (#839) Per-photo emoji reactions from a fixed curated set (❤️ 😂 😍 👏 🎉), one reaction per guest per photo — same emoji toggles off, another switches in place. Stored as feedback_type='reaction' rows with per-guest scoping identical to likes (guest_id when present, device hash otherwise). - migration 164: allow_reactions toggle (default on, still gated by the opt-in feedback_enabled master switch), photo_feedback.reaction value column, denormalized photos.reaction_count - emoji whitelist enforced in the route validator AND the service (shared constants/reactions.js, mirrored in the frontend) - per-emoji tallies + my_feedback.reaction in the photo feedback endpoint; hidden-by-moderator reactions leave all counts - reactions ride the existing rate limiting (like-tier), guest identity modes, and moderation actions; long + pivot exports carry the emoji - gallery: reaction bar in the photo feedback panel (grid lightbox); admin: allow_reactions toggle next to likes, analytics tile, create/duplicate event paths - i18n for all 8 locales; 9 service-level tests * fix(feedback): reach reactions without comments; numeric analytics totals (#839) - the lightbox feedback-panel toggle was gated on allow_comments only — with comments off the new reaction bar was unreachable; the gate now opens for comments OR reactions - the analytics summary now coerces Postgres string counts to numbers: total_feedback concatenated instead of adding ("00006") * fix(feedback): harden reactions from review round 1 (#839) - per-emoji tallies are gated on show_feedback_to_guests — with sharing off a guest sees only their own selection, no aggregate counts - reaction toggle/switch operate on the guest-scoped row SET, so rows duplicated by the (like-parity) check-then-insert race collapse on the next interaction instead of counting twice - rate-limit defaults merge UNDER the persisted settings object — stored rows predating the reaction key otherwise dropped it to the generic 100/h fallback - optimistic revert uses the pre-mutation value via mutation context; the onError closure sees the post-optimistic render, so the old revert froze the wrong state on failed toggles * fix(feedback): review round 2 — hide reaction_count with sharing off, admin list shows emoji (#839) - summary.reaction_count is gated on show_feedback_to_guests like the per-emoji map, keeping the "no aggregates while sharing is off" promise consistent - the admin feedback list renders the reaction emoji on reaction rows and the type filter gains a Reactions option (7 locales; es has no types block and falls back to EN defaults) * fix(feedback): register reaction activity types with translated labels (#839) photo_reaction / guest_feedback_reaction are logged by the submission paths but were absent from the frontend activity-type union and the admin.activities label maps — the recent-activity feed would have shown the raw identifiers. All 8 locales. * feat(feedback): reactions in guest CRM and the premium gallery layout (#839) - guest CRM: per-guest reaction counts in the list aggregation and a Reacted tab (photo grid with emoji badges) + stats card in the guest detail modal; picks/aggregate/exports stay selection-only by design - premium layout: its own yet-another-react-lightbox now gets a fixed reaction-bar overlay (per-photo fetch, optimistic switch) — reactions were otherwise unreachable in this layout since it bypasses the shared PhotoLightbox - allowReactions threaded through the layout feedbackOptions; guest i18n keys for the 7 locales that carry the guests block * fix(feedback): portal the premium reaction bar to document.body (#839) Inside the layout tree an ancestor stacking context (framer-motion transforms) painted the bar under yarl's body-level portal — visible but unclickable, every tap landed on the slide image. As a direct body child the z-index 10000 genuinely wins over yarl's 9999. Verified by clicking through in the running app. --------- Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul NothaftandPaul Nothaft
|
f8a95d29d2 |
feat(auth): OIDC role mapping + login policy — phase 2 (#798) (#854)
* feat(auth): OIDC role mapping + login policy — phase 2 (#798) Role mapping: configurable dot-path roles claim (Keycloak realm_access.roles, Authentik/Pocket ID groups, Entra roles), IdP-value → PicPeak-role mapping table validated against the roles table, re-evaluated on every SSO login with highest-priority-wins on multiple matches. The last active super_admin is never demoted. Optional require-mapped-role policy refuses logins whose token maps to no role (sso_error=no_role). Login policy: oidc_disable_local_login makes the API refuse password logins (403 LOCAL_LOGIN_DISABLED) and the login page render SSO-only; only effective while SSO is enabled+configured, and OIDC_BREAK_GLASS=true always re-opens local login. Public settings expose the EFFECTIVE flag only. Settings UI: Role-mapping card (claim path, mapping rows editor, strict toggle) and Login-policy card with break-glass hint, EN+DE. 14 new integration tests over the mock IdP. * fix(auth): harden phase-2 review findings (#798) - memoize the scrypt-derived OIDC key and serve /public/settings from a 10s-TTL flag cache — the unauthenticated endpoint no longer pays a 13-key config read + blocking scryptSync per request (login route still checks uncached) - make the last-super-admin demotion guard atomic (FOR UPDATE on the active super rows) — concurrent mapped callbacks could previously both count 2 and demote both supers - own-property lookup in role mapping: IdP values like `constructor` now count as unmapped instead of corrupting the roles query - SsoTab clears oidc_disable_local_login in the same save that turns SSO off — the full-form payload otherwise hit the server-side 400 * fix(auth): guarantee break-glass reachability for SSO-only mode (#798) - wire OIDC_BREAK_GLASS and OIDC_ENCRYPTION_KEY through the quick-start docker-compose.yml env allowlist (production compose already passes .env via env_file) and document both in .env.example - refuse enabling oidc_disable_local_login unless an active local-password super_admin exists: OIDC_BREAK_GLASS only re-opens the password route, which OIDC-owned accounts can never use, and settings.edit is super_admin-only — an all-OIDC instance would be unrecoverable during an IdP outage * fix(auth): close SSO-only lockout gaps from review round 3 (#798) - role sync never demotes the last active LOCAL-password super_admin (an OIDC-owned super does not count as break-glass), and isLocalLoginDisabled() disarms itself when no such account remains — self-healing against manual demotion/deactivation/deletion paths - the local-super save-time check now validates the MERGED state, so re-enabling SSO with a stored disable flag is checked too - ALL oidc_* keys are reserved from the generic settings upserts/reads (prefix match) — policy and mapping invariants can only go through the validated PUT /sso - /admin/login/mfa re-checks the policy so an mfa_pending token minted before the flip cannot complete into a local session --------- Co-authored-by: Paul Nothaft <[email protected]> |
||
|
Paul Nothaft
|
a6a3c9f9f8 |
fix(crm): pass trx to logActivity inside transactions — audit rows silently lost on SQLite (#851)
* fix(crm): pass trx to logActivity inside transactions — audit rows were silently lost on SQLite createContract, updateContract, createStorno and reissueInvoice called logActivity() (contract paths also adminActor()) from inside a knex transaction without the trx executor — the pattern db.js:648's comment explicitly warns about. On single-connection SQLite the audit insert waits on a second pool connection while the trx holds the only one: a 60s acquire-timeout stall per call, then logActivity's catch swallows the failure and the audit row is silently lost. Postgres unaffected. Fix mirrors the one call site that already did it right (contract_created_from_quote, conversions.js): resolve the audit actor before the transaction opens and pass trx as logActivity's executor so the insert rides the transaction's connection. Verified NOT affected (logActivity outside any trx, unchanged): cancelContract, contract_converted_to_event, contract_signed_by_customer, contract_sent, invoice_sent/_cancelled(draft)/_released/monthly_bill. Found by the #587 integration-test work (PR #850, which shrank the pool acquire timeout to tolerate the stall — that workaround can be dropped once both land). * fix(crm): run reissueInvoice's createInvoice without a wrapping transaction (codex review of #851) The round-1 fix passed trx to the reissue audit call — but that point was never reached on single-connection SQLite: createInvoice internally reads via the global connection (businessProfileService.getProfile, getAppSetting, bank-account resolution), so the outer trx deadlocked first and aborted the replacement AFTER the Storno had already committed and been emailed. createInvoice's five other callers all run it without a trx; reissue now does the same and backlinks afterwards. Trade-off documented in code: replacement + backlink are no longer atomic — a crash between them leaves a visible draft without replaces_invoice_id, which beats the guaranteed stall. New regression test drives a full cancel+reissue on the SQLite harness and pins the invoice_reissued audit row. * fix(crm): restore the reissue transaction by routing createInvoice's reads through trx (codex review of #851, round 2) Round 2 was right that dropping the wrapping transaction traded the deadlock for orphan drafts: createInvoice inserts the invoice row and claims a sequence number BEFORE line-item validation can throw, so a failed reissue would persist partial state after the Storno committed. Proper fix: the transaction is back, and every read inside createInvoice now rides it — getProfile and resolveBankAccountForCurrency gained an optional conn param (default db, all other callers unchanged), getAppSetting calls pass trx (crm_invoice_round_total + the resolveNetDays default the regression test flushed out), and the invoice_created audit uses the trx executor. The reissue regression test now proves a full cancel+reissue commits atomically on single-connection SQLite. |
||
|
Paul Nothaft
|
cb5b319f10 |
feat(notifications): surface guest activity in the admin bell (#849)
* feat(notifications): surface guest activity in the admin bell (#746) Favorites already reached activity_logs (feedbackService), but gallery opens and downloads only landed in access_logs — invisible in the notification bell. Now: - gallery_opened on the guest photo-list route, debounced in-memory to one notification per event per 6h (the endpoint fires per page load; per-hit notifications would spam the bell). Slideshow traffic stays excluded, matching the analytics exclusion. - gallery_downloaded on all four download paths (streamed + pre-zipped + presigned download-all, download-selected) with scope metadata. - Frontend: locale entries for galleryOpened/galleryDownloaded (and photoFavorite, which previously fell through to the generic 'system activity' line) in all 8 languages — resolved via the existing smart camelCase fallback, no switch cases needed. Distinct bell icons per type. * fix(notifications): single-photo download activity + render per-type bell icons (codex review of #849) - The per-photo Save route (GET /:slug/download/:photoId) only wrote to access_logs — the most common download path never reached the bell. Now emits gallery_downloaded with scope 'single', debounced to one notification per event per hour: a guest saving 30 photos is one signal, not thirty (exact counts stay in access_logs/analytics). - getNotificationStyle's icon names were dead — AdminHeader hard-coded <Bell> for every row. Added an icon map so gallery opens (Eye), downloads (Download), favorites (Heart) and the pre-existing style names render their intended icons. * fix(notifications): notify after successful delivery, complete the icon map (codex review of #849, round 2) - Single-photo notification now fires on res 'finish' with status < 400: emitting up-front logged downloads that then 404ed/failed AND burned the 1h debounce window against the next real download. - Icon map completed over every name getNotificationStyle returns (grep-verified) — settings/user/mail/etc. styles render their declared icons instead of falling back to Bell. Deliberately NOT taken from the review: DB-backed debounce state for multi-worker deployments. The backend's current deployment contract is single-process (no PM2 cluster in-repo; multi-replica explicitly parked in #799 — chunked-upload/session state is process-local for the same reason). Worst case under a future multi-worker setup is N notifications per window, which degrades, not breaks; a shared-store debounce belongs to the #799 phase-3 work. * fix(notifications): attribute client sessions, log cached-ZIP after finish, add Trash2 icon (codex review of #849, round 3) - gallery_opened/gallery_downloaded now carry the real actor: client sessions (accessLevel 'client') are recorded as 'customer' instead of being mislabeled 'guest' — #746 explicitly covers client activity, so they are attributed, not excluded. - Cached-ZIP streaming path logs on res 'finish' (< 400) like the single-photo path — piping is not delivery. The presigned-redirect and on-the-fly-archiver paths keep their existing timing (redirect handoff / post-finalize). - Trash2 added to the icon map (customer_erased, bulk_delete_completed no longer fall back to Bell — the grep that built the map missed the digit in the name). * fix(notifications): dashboard formatting, portal dedup, actor-aware wording, archiver finish-hooks (codex review of #849, confirmation round) - activity_logs feed TWO surfaces: the dashboard's Recent Activity used admin.activities.<type> keys that didn't exist, rendering raw identifiers — added gallery_opened/gallery_downloaded entries in all 8 locales. - Customer-portal opens already log customer_event_access at the access-token mint; the ensuing /photos call no longer double-notifies (client sessions surface via downloads only). - gallery_downloaded formatting is actor-aware: customer sessions render 'Customer downloaded…' (new galleryDownloadedCustomer key ×8) instead of 'A guest…'. - Both on-the-fly ZIP paths (download-all fallback + download-selected) notify on res 'finish' < 400 — archive.finalize() ends Archiver's input, not the HTTP transfer. * fix(notifications): key customer dedup/attribution on portal provenance, neutral favorite wording (codex review of #849, final round) The previous dedup was inverted: portal-minted tokens carry via:'customer' but NO accessLevel (they run as guest), while PIN-client logins carry accessLevel:'client' and log nothing else. So PIN clients' only open signal was suppressed while portal opens still double- notified and portal downloads read as guest activity. verifyGalleryAccess now surfaces req.viaCustomer; gallery_opened dedups on THAT (portal only), and galleryActor treats via-customer OR accessLevel-client as 'customer'. photoFavorite wording is actor-neutral across all 8 locales — feedbackService logs favorites without an actor, so claiming 'a guest' was wrong for customer favorites. |
||
|
Paul Nothaft
|
e8dad4b40d |
feat(slideshow): guest-scannable share-link QR overlay (#848)
* feat(slideshow): guest-scannable share-link QR overlay (#837) - Global settings (Settings → Slideshow): slideshow_qr_enabled/position/ opacity/size — same option shape and cascade as the watermark. - Per-event tri-state show_qr (migration 163): NULL inherits the global, true/false force on/off; editable in the per-event slideshow card. - State endpoint ships the QR as a PNG data URI (cached per share URL — the 3s projector poll never re-encodes), so the kiosk needs no QR lib and no extra authenticated request. - Kiosk renders the QR in a white padded corner box so it stays scannable on any photo. - i18n: en + de (the slideshow namespace has no other locales yet). * fix(slideshow): persist per-event QR override, show QR on empty shows, bound the QR cache (codex review of #848) - OverviewTab never passed event.show_qr into the settings card (and the Event type lacked the field), so a stored true/false override always displayed as 'inherit' and the next save silently reset it to NULL. - The QR overlay was nested inside the photos.length > 0 branch — an empty or category-filtered live gallery showed only 'Waiting for photos', exactly when 'scan to add the first photos' matters most. Now rendered for any running show. - slideshowQrCache: insertion-order eviction at 50 entries — rotated tokens and past events no longer accumulate base64 PNGs forever. * fix(slideshow): derive the QR origin from the kiosk request when the base is loopback (codex review of #848, round 2) With the compose-default FRONTEND_URL=http://localhost:3000 (or no base configured) the overlay QR sent scanning phones to their own localhost. The state poll comes from the kiosk browser itself, so its Host header + protocol (trust proxy is configured) are exactly the public origin guests can reach — used whenever the configured base is missing or loopback. Mirrors the ?origin= fallback #847 uses for the admin-side QR downloads. * fix(slideshow): kiosk passes its origin for the QR fallback (codex review of #848, round 3) req.get('host') is not the browser origin behind the standard proxies — frontend/nginx.conf forwards $host with the port stripped, so a compose LAN deployment on :3000 encoded port 80. The kiosk now sends window.location.origin with the session/state calls (validated server-side, same pattern as #847's admin downloads); the Host-derived origin remains as second fallback. * fix(slideshow): reject loopback kiosk origins, throttle QR regeneration per event (codex review of #848, confirmation round) - A loopback window.location.origin from the kiosk is no more guest-reachable than the loopback base it would replace — rejected; when no reachable URL remains the overlay is suppressed entirely (no QR beats a QR that sends phones to their own localhost). New test pins the suppression. - The QR cache is keyed by event id with a 60s regeneration throttle: the origin is caller-influenced when the base is loopback, so URL-keyed caching let a slideshow-link holder force a fresh QRCode.toDataURL per request via unique origins — a cheap CPU exhaustion path. Encode rate is now bounded per event regardless of input. QR margin also raised to the 4-module spec quiet zone, matching #847. * fix(slideshow): never serve a mismatched cached QR + single-flight encoding (codex review of #848, final round) - A slideshow-token holder could poison the projector's QR: an attacker-origin entry cached per event was served to the legitimate kiosk for the rest of the throttle window. A cached artifact is now only served when its URL matches the request; mismatches inside the window suppress the overlay briefly instead of showing foreign content. - Cold-cache stampede closed: concurrent polls share one in-flight encode promise instead of each scheduling a 512px render. Rejected from the same round (false positive, verified empirically): the loopback regex claim — /^https?:\/\/(localhost|127\.)/ matches 'http://localhost:3000' and '127.0.0.1:port' just fine (no trailing slash required), and the suppression test runs green. |
||
|
Paul Nothaft
|
60cdd07085 |
feat(events): gallery QR code + printable table-card/poster PDFs (#847)
* feat(events): gallery QR code + printable table-card/poster PDFs (#836) - GET /api/admin/events/:id/qr — share-link QR as PNG (128-2048px) or SVG, inline or attachment; adminAuth + events.view + ownership. - GET /api/admin/events/:id/qr-print — pdfkit-rendered A6 table card / A4 poster with event name, QR, localized caption (8 locales; Cyrillic falls back to English — built-in Helvetica has no Cyrillic glyphs) and the share URL as footer. - Event detail: QR section in ShareLinkCard with live preview (blob fetch — Bearer auth) and PNG/SVG/table-card/poster downloads; print language follows the admin UI language. i18n keys in all 8 locales. - qrcode + pdfkit were already dependencies (MFA / CRM PDFs). * fix(events): QR origin fallback, Unicode PDF font, bounded layout, stale-preview guard (codex review of #847) - QR URLs: prefer the configured public base, but fall back to the admin browser's origin (passed as ?origin=, validated) when the base is missing or localhost — mirrors buildShareLinkUrl so the QR encodes the same URL the card displays instead of an unusable localhost target. - PDFs render with the bundled IBM Plex Sans TTFs (Latin+Cyrillic+Greek) instead of WinAnsi-only Helvetica: Cyrillic event names no longer silently disappear, and the caption's English-fallback hack is gone. - Fixed vertical layout: title gets a bounded two-line ellipsis region and all positions derive from constants, so long event names can't push the QR/caption over the footer; URL footer bounded too. - ShareLinkCard preview: stale-response guard — a late blob response after unmount/event-switch is revoked instead of leaking and overwriting the newer event's QR. * fix(events): bundle complete IBM Plex Sans for QR PDFs + IPv6 loopback fallback (codex review of #847, round 2) Round 2 caught that the pre-existing assets/fonts/IBM-Plex-Sans/ files are 270-glyph Latin SUBSETS — my round-1 font swap didn't actually fix Cyrillic titles and regressed the ru caption. Now bundling the complete IBM Plex Sans 400/700 TTFs (1019 glyphs, Latin+Cyrillic+Greek — cmap verified via fontkit, rendering verified on a generated PDF) under assets/fonts/IBM-Plex-Sans-Full/ with the OFL license alongside. ~400 KB total; source: IBM/plex release zip @ibm/[email protected]. Also: LOCAL_BASE_RE now recognizes IPv6 loopback ([::1]) so a FRONTEND_URL of http://[::1]:3000 falls back to the browser origin like the frontend's own URL logic does. Note for a follow-up: the CRM invoice/quote PDFs use the same Latin-only subsets and share the Cyrillic gap. * fix(events): responsive QR card that survives preview failures (codex review of #847, round 3) - The QR section keys off share-link availability instead of a loaded preview: a transient failure of the preview request no longer hides every download button until reload; a placeholder tile renders in place of the image. - Preview + actions stack on phone widths and the button grid drops to one column below sm, so 'Tischkarte (A6)'-length labels don't overflow. * fix(events): QR encodes the stored share_link + spec quiet zone (codex review of #847, confirmation round) - The QR target is now the STORED share_link — exactly what the card displays and the admin copies. Rebuilding from current slug/token/ short-URL setting could diverge for legacy absolute links or events created under a different short-URL setting; a printed QR encoding a different URL than the card is a permanent mistake. Rebuild remains only as fallback when no share_link is stored. - QR margin back to the library's 4-module default for all generated assets — the spec's quiet zone; margin 2 risks scan failures when the printout sits against colored surroundings. * fix(events): bare share_link tokens resolve as /gallery/<token> in QR URLs (codex review of #847, final round) Quote-/contract-converted events persist share_link as the raw token — the frontend's buildShareLinkUrl prefixes those with /gallery/, but the QR path normalization only added a leading slash, encoding <origin>/<token> into every image/PDF for such events. Now mirrors the frontend exactly. * test(events): 30s timeout for the print-PDF cases (CI fix) The poster PDF now embeds the full IBM Plex Sans TTFs (~200 KB each); font parsing + subsetting exceeds jest's 5s default on slower CI runners — the suite went red on exactly that test after the font commit. |
||
|
Paul Nothaft
|
8337a716b1 |
fix(file-watcher): bound concurrent photo processing (#846)
* fix(file-watcher): bound concurrent photo processing chokidar fires 'add' once per file — with no ignoreInitial option the boot scan fires it for every existing file, and a bulk drop into the watch folder fires it for every new one at once. Each handler runs DB lookups plus (for new files) a full sharp pipeline; sharp.concurrency(2) only caps libvips threads WITHIN one operation, not the number of parallel pipelines, so unbounded handlers can OOM small hosts. Gate both 'add' and 'unlink' through a shared p-limit (FILE_WATCHER_CONCURRENCY, default 2, floor 1) — mass deletes otherwise burst DB work and ZIP-cache invalidation the same way. p-limit is pinned to ^3.1.0, the last CommonJS release. Adapted from the filpgame fork (426ca491) — thanks @filpgame; extended to cover 'unlink', documented in .env.example, plus a lock-in test for the existing Sharp cache/concurrency caps this bound relies on. * chore(compose): pass FILE_WATCHER_CONCURRENCY into the backend container (codex review of #846) The backend service uses an explicit environment list (no env_file), so the documented override never reached the container in the default compose deployments. Added to both compose files + root .env.example. |
||
|
Paul Nothaft
|
0310c46fdd |
fix(uploads): keep videos when thumbnail generation fails (#845)
* fix(uploads): keep videos when thumbnail generation fails processUploadedVideo() (ffmpeg probe + thumbnail) was unguarded in both pipeline paths, while the image branch next to each already survives its thumbnail failures: - processUploadedPhotos (sync): the throw failed the whole upload — the video was lost. - processPhoto (async worker, the path real uploads take): the throw marked the row 'failed', and the guest gallery only lists 'complete' — the video became permanently invisible despite being fully uploaded. Both call sites now fall back to extractVideoMetadata() alone and keep the video without a preview; if even the probe fails, the video is kept with no metadata. Idea from the munin92 fork (2026-07-02), reimplemented for both paths + regression test. * fix(uploads): placeholder thumbnail for rescued videos (codex review of #845) A completed video with a NULL thumbnail made the gallery grid fetch the ORIGINAL video file as an <img> blob (thumbnail_url || url) — a potentially multi-GB download for a broken tile. Both fallback paths now generate the existing sharp-rendered play-button placeholder (generateVideoPlaceholder — ffmpeg-free), so rescued videos get a real tile. Test asserts the placeholder key lands in thumbnail_path. |
||
|
Paul Nothaft
|
8060fedf6a |
fix(security): read the password-complexity key the settings UI writes (#843)
* fix(security): read the password-complexity key the settings UI writes The settings UI saves the admin's complexity choice as security_password_complexity (useSettingsState.ts prefixes security_ to password_complexity), but getPasswordComplexitySettings() queried security_password_complexity_level — written by nothing — so the setting was silently ignored and password validation always used the 'moderate' default. Spotted in the filpgame fork (their main, 2026-07-14). * fix(security): accept the Postgres json-column shape of the complexity value (codex review of #843) On SQLite the TEXT column returns the JSON-stringified value ('"very_strong"'), but on Postgres (production default) setting_value is a json column and arrives already decoded ('very_strong') — the bare JSON.parse threw and the outer catch silently fell back to 'moderate' again. Parse with fallback, mirroring getAppSetting's documented pattern; test now covers both driver shapes + the empty-value default. |
||
|
Paul Nothaft
|
2f4b8a64c0 |
chore(backend): ignore runtime storage in git/docker, remove dead getSafeFilename
Follow-ups from the codex review of #834: - .gitignore: backend/storage/ is runtime-generated (media, previews, thumbnails, business docs) and was only partially ignored — E2E runs left it dangling as untracked, which is how ~12 MB of artifacts nearly landed in a commit. Ignore the whole directory (nothing under it is tracked); replaces the narrower business-docs rule. - backend/.dockerignore: the granular storage/* rules missed storage/previews, so locally generated previews were copied into production images. Exclude storage entirely — the Dockerfile creates the needed directories itself (RUN mkdir -p, Dockerfile:96). - fileSecurityUtils.js: remove getSafeFilename — zero callers across the repo, and its private extension whitelist silently drifted from the real validation paths (see #834), which is exactly the trap dead security code sets. |
||
|
Paul Nothaft
|
d7ba781c0f |
Merge remote-tracking branch 'origin/main' into feat/guest-upload-dng-raw
# Conflicts: # backend/src/services/uploadSettings.js # backend/src/utils/fileSecurityUtils.js # frontend/src/utils/fileTypes.ts |
||
|
Paul Nothaft
|
ee9d2f70d3 |
Merge pull request #832 from PicPeak/feat/guest-upload-heic-dynamic-hint
feat(uploads): HEIC/HEIF support + dynamic format hint on guest upload (#821) |
||
|
Paul Nothaft
|
d0ccadbc99 |
fix(uploads): RAW derivative key collision, watermark skip, dev exiftool (codex review of #833 round 2)
- Derivative key collision: processUploadedPhotos/replacePhoto passed the client-supplied original filename as the RAW output basename, but thumbnails/ heroes/previews are global keys — two galleries uploading IMG_0001.dng would overwrite each other's derivative. Use the unique stored newFilename instead. (processPhoto already used the unique photo.filename.) - Watermark: the watermark path opens the original with sharp, which can't decode RAW, so it fell back to the original bytes and recorded the copy as watermarked. Skip RAW in generateForPhoto (like videos) so the watermark state stays honest until RAW watermarking is properly supported. - exiftool added to Dockerfile.dev so dev/native runtimes don't accept a DNG then fail it with ENOENT. |
||
|
Paul Nothaft
|
b743ea0398 |
fix(uploads): apply RAW extraction in the actual async ingest path (codex review of #833)
The RAW/DNG extraction was only wired into processUploadedPhotos() (the synchronous path), but real uploads queue to 'pending' and are handled by the background worker → processPhoto(), which generated the thumbnail + dimensions directly from the DNG (both fail) and then marked the photo 'complete' — success with no thumbnail. Wire withProcessableImage() into processPhoto() (the live path) and into photoReplacementService.replacePhoto() (replace-by-name), so all three ingest paths extract the embedded JPEG preview for RAW. Updates the processPhoto test's imageProcessor mock with the new withProcessableImage dependency (pass-through for ordinary images). |
||
|
Paul Nothaft
|
808d305549 |
fix(gallery): serve JPEG preview for non-displayable originals in lightbox (codex review of #832)
The lightbox falls back to photo.url (the ORIGINAL) when preview_url is null, which happens by default (lightbox_preview_enabled=false). For HEIC/HEIF/DNG the original bytes aren't renderable in an <img>, so the lightbox showed a broken image. Now force preview_url for those formats (by MIME or extension) regardless of the toggle, so the browser always gets the generated JPEG preview. Covers DNG too (forward-compatible with #833). EXPERIMENTAL caveat unchanged: whether the preview actually renders still depends on the backend decoding the source — HEVC-in-HEIC on the prod Alpine image is unverified, DNG needs exiftool (#833). Documented on the PR. |