v3.46.3
552 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Paul Nothaft
|
da44f1947b |
fix(gallery): a missing file must not take the backend down (#1128)
LocalFsStorage.get() returns an fs.createReadStream, which is lazy: it resolves immediately and opens the file on a later tick, so an ENOENT arrives after the await returned and outside the route's try/catch. An unhandled 'error' event is a process-level throw Express cannot catch — the backend exits and every gallery goes blank until the container restarts. gallery.js had ten .pipe(res) calls and zero error handlers. pipeStreamToResponse attaches the missing handler: a vanished source becomes a 404 (410 for a prepared zip), anything else a 500, and a source that dies mid-response destroys the connection rather than rewriting a status already on the wire. Headers staged for the file are cleared first — Express does not overwrite an existing Content-Type, and a surviving Cache-Control would let a transient 404 be cached as a broken tile for up to an hour. It also releases the source when a client hangs up. Applied to all eight streaming responses, not just the thumbnail route. Stable twin of #1133, reduced: the tier-race half does not apply here because ensureThumbnailAtWidth does not exist on this branch. |
||
|
Paul Nothaft
|
dc9e3cdc5e |
fix(thumbnails): regenerate external photos, and stop destroying good ones (#1129)
POST /admin/thumbnails/regenerate resolved every source as storage/events/active/<photo.path> and fs.access'd it. External and reference rows are not there — their originals live under events.external_path — so every one failed the check and was counted as an error, while the UI reported success because the response is sent before the background loop starts. It now goes through ensureThumbnail, which resolves both source kinds and writes thumbnail_path back itself. Nulling thumbnail_path stops it short-circuiting on isThumbnailValid, which matters because the old thumbnail is normally still readable at exactly the moment someone presses regenerate. And the half that destroys data: generateThumbnail deleted the target BEFORE sharp had opened the source, and again in its catch. A source that could not be read — a NAS mount that blipped — left the previous rendition gone and the database pointing at it. Across a bulk regenerate that is the whole gallery. Neither delete was needed: put stages to a temp file and renames atomically, and put is the last statement in the try so no partial object can exist. Also: videos filtered out, and the superseded rendition removed only when the storage key actually moved, compared through the same canonicalisation the backends apply. Stable twin of #1134. |
||
|
Paul Nothaft
|
6df42ab22c |
fix(preview): generate lightbox previews for external/reference photos (#1078) (#1080)
* fix(preview): generate lightbox previews for external/reference photos (#1078) Stable twin of the main-line fix. ensurePreviewImage() resolved its source only via resolvePhotoStorageKey(), which returns null for external/reference photos by design — those live on a media mount outside the managed storage tree. The null went straight into withLocalCopy(), which throws, so the preview route fell back to redirecting at the full-size original. Galleries whose photos are all external got no benefit from the preview tier (#492): guests paid 5-12 MB on every lightbox open. Add the external branch ensureThumbnail() already has: resolve via resolvePhotoFilePath() and feed the mount path to generatePreviewImage() directly, with an ext<id>_ output basename. generatePreviewImage() on this branch hardcoded path.basename(imagePath) and ignored options.outputBasename, so it needs the same one-line honouring that generateThumbnail() already does — without it two events referencing the same NAS basename collide on one preview key. Also return null rather than throwing for a row with no source_origin in a reference-mode event, whose mode falls back to the event's. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(preview): select the columns the external branch needs on bulk regenerate POST /api/admin/thumbnails/regenerate-previews selected only id, event_id, path, media_type, mime_type and preview_path, so photo.source_origin was undefined by the time ensurePreviewImage branched on it. Every external row in a reference gallery took the managed path, resolvePhotoStorageKey returned null for it, and the endpoint reported success while generating nothing. Add source_origin, external_relpath and filename to the select, plus a source-inspection test pinning the caller contract and a service-level test showing a column-starved row is indistinguishable from a managed one. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc --------- Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local> |
||
|
Paul Nothaft
|
45ffe64b7c |
fix(storage): write business documents under STORAGE_PATH, not the cwd (#1072)
Stable twin of #1070. persistDocPdf, the invoice sending and reminder writers, both contract signature writers and persistSignatureImage built their targets from `path.join(process.cwd(), 'storage', 'business-docs', ...)` and never consulted STORAGE_PATH. Both compose files pin STORAGE_PATH=/app/storage and the image's WORKDIR is /app, so on a stock deployment the two name the same directory and nothing looked wrong. Point STORAGE_PATH anywhere else and quotes, invoices, Mahnungen, contracts and signature images land outside the configured storage root: missed by the backup walker, invisible to storage accounting, and gone when the container is replaced. assertContractPdfPath moves with them. On this branch the writers and the guard are wrong together, so contract downloads currently work — migrating the writers alone would have introduced PATH_OUTSIDE_STORAGE on every newly generated contract. The guard now resolves through getStoragePath() like the writers, and keeps the legacy cwd root so contracts written before this still resolve; their absolute paths are in the database. Also on the shared resolver: the custom PDF font lookup (a font under STORAGE_PATH/fonts was never found, and the document silently fell back to the built-in face) and the two backup diagnostics, which otherwise inspect a different root than the backup walker when STORAGE_PATH is unset. No migration needed — the persisted path is stored absolute. Verified on this branch, not inferred from main: the new test is 6/6, and contract/quote/invoice/pdf/safePath suites are 213/213 both before and after the change. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local> |
||
|
Paul Nothaft
|
376311cb90 |
fix(pdf): RFC 6266-encode Content-Disposition on quote/invoice PDFs (#1024) (#1062)
Stable backport of #1055 (main:
|
||
|
Paul Nothaft
|
88fa3c5297 |
fix(storage): add S3 client timeouts so a dropped connection can't wedge uploads (#1049) (#1054)
Stable backport of #1049 (main:
|
||
|
Paul Nothaft
|
980378a17b |
feat(backup): open sqlite → pg .picpeak restore as the supported upgrade direction (#1041) (#1059)
Stable backport of #1043 (main:
|
||
|
Paul Nothaft
|
9003b34c8a |
fix(docker): default NODE_ENV=production so non-compose deploys don't fall back to SQLite (#1038) (#1040)
* fix(docker): default NODE_ENV=production so non-compose deploys don't fall back to SQLite (#1038) knexfile.js selects its config block by NODE_ENV and the `development` block defaults to sqlite3. The image never set NODE_ENV, so every deployment that doesn't go through our compose files — Kubernetes, Helm, plain `docker run` — silently ran on SQLite and ignored DB_HOST/DB_USER/DB_PASSWORD. It stayed invisible because wait-for-db.sh is shell: it reads DB_HOST directly, connects to Postgres, creates the database and logs "PostgreSQL is up" in the same container where the Node process then writes to a SQLite file. Migrations go through src/database/db.js → the same knexfile, so they also ran against SQLite, leaving the provisioned Postgres database empty. Setting the default alone would be unsafe: an affected install would flip to Postgres on its next image pull and come up against an EMPTY database, which reads as total data loss. So this adds a guard that runs before migrations touch anything: - logs the resolved engine + target at boot - refuses to start when pointed at a virgin Postgres while a populated SQLite file exists, naming the file and the .picpeak export path for moving the data, with PICPEAK_ALLOW_EMPTY_PG=true as the escape hatch - warns but boots when Postgres settings are present yet SQLite is in use Compose files already set NODE_ENV explicitly, so compose users are unaffected. Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM * fix(db): stay on SQLite instead of blocking, and add a migration path (#1038) Reworks the guard after walking through what an existing install actually experiences on its next image pull. Blocking was the wrong trade. An operator who had unknowingly been running on SQLite (because the image left NODE_ENV unset) would have pulled the fix and got a CrashLoopBackOff: data safe, galleries offline, for something they did not do. Now the boot RESOLVES the engine before migrations run and stays on whichever one holds the data: - Postgres configured but holding no galleries, while a populated SQLite file exists → keep serving from SQLite, print what happened and how to migrate. - once Postgres holds the data, the next restart switches over on its own. - an explicit DATABASE_CLIENT is always honoured. Keyed on Postgres holding DATA, not on it having tables: a stray `run-migrations` against the empty database creates every table, which would otherwise blind the check. Adds scripts/migrate-sqlite-to-postgres.js, which reuses the .picpeak export/import services rather than hand-rolling a cross-engine copy. Two additions were needed for the SQLite → Postgres direction, both opt-in and CLI-only so the upload/restore UI is untouched: - `allowEngineSwitch` relaxes the importer's same-engine guard - cross-engine row coercion: SQLite has no real date or boolean types, so its rows carry epoch numbers where Postgres wants a timestamp and 0/1 where it wants a boolean, both of which Postgres rejects outright. Driven by the TARGET schema, never guessed from the value. DELTA FROM THE BETA PR: this branch's import service has no resyncSequences() — that landed on main only. Without it a cross-engine load leaves Postgres identity sequences at 1 and the next insert collides on the primary key, so the function is backported here and called ONLY on the cross-engine path. Same-engine restores through the UI keep their current behaviour exactly. Verified end to end on this branch against a real PostgreSQL 15: a seeded SQLite install migrated across with booleans, timestamps and foreign keys intact, and the next INSERT got id 2 rather than colliding. Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM * fix(db): close four review findings on the SQLite fallback + migration (#1038) External review (codex) found four issues, all confirmed against the code and fixed here. Two of them could have cost data. 1. The engine resolver was reachable only through wait-for-db.sh. A Kubernetes manifest that sets `command`/`args`, or a plain `docker run … node server.js`, bypasses the entrypoint — exactly the deployment styles this fix targets. With NODE_ENV now baked into the image, such an install would have resolved to Postgres and come up against an empty database while its SQLite data sat there unseen. server.js now resolves the engine itself, before anything requires knexfile, via the same script the entrypoint uses. Verified by running `node server.js` directly against an install with stranded SQLite data: it logs the banner and serves SQLite. 2. Cross-engine loads double-encoded JSON. SQLite has no json type, so its json columns are TEXT holding JSON; the export dumps that as a string and serialiseJsonColumns stringified it again, storing `true` as the scalar string "true". app_settings.setting_value is json on every install, so this reshaped every migrated setting. The text is decoded before serialisation now — verified against a real Postgres: json_typeof(setting_value) is `boolean`, matching a native install exactly. 3. The migration could silently miss concurrent writes. If the backend keeps serving, rows written after the export never reach Postgres and vanish from view once the engine switches. The script now fingerprints the SQLite tables whose loss would be noticed, checks for drift BEFORE loading Postgres (so a detected race leaves the target untouched) and again after, and refuses with the exact rows that moved. It also says plainly to stop the backend first. 4. The child phases shared stdout with winston. Outside production, and whenever LOG_TO_CONSOLE=true, createPicpeak's own log line was concatenated with the archive path and the migration failed on a bogus filename. Payloads travel through a result file now; verified with LOG_TO_CONSOLE=true. Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM * fix(db): close review round 2 — six more data-safety findings (#1038) 1. The engine choice is now PINNED once the data is in Postgres. Previously the boot decided from "does Postgres hold galleries", so an operator who later deleted every gallery would be sent back to the stale pre-migration SQLite file while their settings, admins and CRM data stayed in Postgres. The migration writes a marker next to the database file (and retires the file itself by renaming it); the marker wins over any probe. 2. The migration refused to overwrite Postgres only when it held GALLERIES. A target with admins, customers, invoices or projects but no galleries was wiped without --force. Both the source and target checks now look for user data across the tables that are empty on a fresh install. 3. Same bug in the other direction: an install with no galleries but real admins/settings/customers was refused a migration it was entitled to. 4. Drift detection covered four tables and only count/max(id), so an in-place UPDATE (event edit, password change) or a write to any other table passed unnoticed. It now fingerprints every table the export carries, including max(updated_at). It still is not a substitute for stopping the backend, and the script says so rather than implying a guarantee. 5. probeSqliteData() treated an unreadable or corrupt file as "no data", which would have switched the install to an empty Postgres — the very failure this module exists to prevent. It fails closed now and stays on SQLite so the real error surfaces. 6. The "you are leaving SQLite data behind" warning was unreachable: setting DATABASE_CLIENT skipped the probes, so the branch that produces it never had the inputs. Postgres and SQLite are both probed whenever Postgres is the engine in play. Also: the final verification compares row counts for EVERY table rather than just galleries, and flags only a shortfall — the import legitimately adds an app_settings row (setSessionsValidAfter) that made the strict equality fail on a first real run. Verified against a real PostgreSQL 15 end to end, including: the marker keeps an install on Postgres after every gallery is deleted; removing the marker and restoring the file rolls back to SQLite as documented. Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM * fix(db): close review round 3 — occupancy, bootstrap admin, secrets in /tmp (#1038) 1. Both engine probes judged occupancy by GALLERIES alone. An install whose galleries were all deleted, but which still has admins, customers or accounting records, was treated as empty: on the SQLite side that meant booting the empty Postgres and appearing to lose everything; on the Postgres side it meant diverting a live install to a stale SQLite file. Both now look across the tables that are empty on a fresh install, matching the migration script. 2. The migration ran migrate-schema BEFORE checking the target, and migration 001 seeds a bootstrap admin when ADMIN_PASSWORD is set (common on legacy installs). The occupancy check then saw that admin and refused, pushing the operator towards --force against a genuinely empty database. The target is read first now. 3. probeSqliteData()'s warning went through the app logger, which writes to STDOUT when LOG_TO_CONSOLE=true — and the resolver's stdout is the protocol channel wait-for-db.sh captures, so DATABASE_CLIENT could have been set to a JSON log line. Diagnostics take an injected sink (stderr in the resolver), and the shell now validates the value it captured instead of trusting it. 4. The .picpeak archive holds password hashes, SMTP credentials and API keys in plaintext, and was only removed on the fully-successful path — any drift or import failure left it in /tmp. Every exit path removes it now. 5. A database-only migration still hauled every business-doc and upload through /tmp and back into the same volume. createPicpeak takes includeFiles:false for this path; rows move, files stay where they already are. Verified against a real PostgreSQL 15: a gallery-less install with only an admin account now stays on SQLite and migrates successfully with ADMIN_PASSWORD set; the resolver emits exactly one token on stdout with LOG_TO_CONSOLE=true and a corrupt database; a drift failure leaves Postgres untouched and no archive behind. Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM * fix(db): pin the boot to SQLite while a migration is unfinished (#1038) Review round 4. A migration that dies after touching Postgres leaves rows behind — schema creation alone seeds a bootstrap admin when ADMIN_PASSWORD is set, and a drift or row-count failure can leave a partial load. Since the occupancy probes were widened in round 3, those rows read as "Postgres is occupied", so the next restart would switch engines and hide the SQLite data that is still the database of record. The script now writes a pin file next to the database BEFORE its first Postgres write and clears it only on success (after the success marker exists, so no restart in between can pick the wrong engine). While the pin is present the resolver stays on SQLite and explains why. Verified against a real PostgreSQL 15 by reproducing the exact scenario: a migration failed mid-run with ADMIN_PASSWORD set, leaving one bootstrap admin in Postgres. With the pin the next boot resolves to sqlite3; with the pin removed it resolves to pg — the failure this closes. The subsequent successful re-run clears the pin and the boot moves to Postgres. Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM * fix(db): close review round 5 — occupancy, path drift, retry, host default (#1038) 1. A seeded bootstrap admin counted as "Postgres is occupied". core/001_init.js inserts one whenever ADMIN_PASSWORD is set, so a Postgres that was initialised once and never used would have beaten a SQLite file full of real galleries — the exact failure the guard exists to prevent, reintroduced by widening the probe in round 3. The two sides are deliberately asymmetric now: the SQLite probe counts any user data (err towards keeping data visible), the Postgres probe ignores rows that schema creation seeds (err towards requiring proof of real use). 2. The guard resolved DATABASE_PATH with its own logic while knexfile trimmed whitespace and collapsed the legacy duplicated-backend form. A path either engine normalised differently meant probing a file nobody uses, concluding there was no SQLite data, and booting an empty Postgres. The resolution now lives in one module both require. 3. Re-running after a partial migration — the documented recovery — was refused unless the operator passed the destructive-sounding --force, because the half-written rows read as target data. An unfinished run of this same script is now recognised as a safe retry. 4. wait-for-db.sh verified readiness against its own default host (`postgres`) while knexfile's production block defaults to `db`. With NODE_ENV now baked in, a bare `docker run` without DB_HOST would have passed the readiness check against one host and then dialled another. The entrypoint exports the exact connection it verified. Compose sets DB_HOST explicitly and is unaffected. Verified: a Postgres holding only a seeded admin now loses to real SQLite data; a DATABASE_PATH with surrounding whitespace resolves to the identical file in both knexfile and the guard. Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM * fix(db): close review round 6 — explicit-client bypasses, retry scope, cleanup (#1038) 1. An explicit DATABASE_CLIENT bypassed the unfinished-migration pin, because decideBootEngine honoured it first. docker-compose sets DATABASE_CLIENT=pg, so a failed migration would have restarted on a half-written Postgres on exactly the deployments that pin it. Worse in the other direction: with DATABASE_CLIENT=sqlite3, a SUCCESSFUL migration renames the source file, so the next start created a NEW, empty SQLite database and served that. The pin now outranks explicit pg (clearing the marker is the override), explicit sqlite3 is left alone since it already points at the data, and the migration refuses up front when the deployment pins anything other than pg. 2. The retry allowance was bound to the SQLite file, not to the target. An operator who repointed DB_HOST/DB_NAME between attempts could have replaced an unrelated populated database without --force. The pin records the target and the allowance only applies when it matches. 3. The printed rollback did not roll back: with data on both sides and no marker, the resolver still selects Postgres. It now spells out all three steps, including DATABASE_CLIENT=sqlite3. 4. A failure inside createPicpeak left a partial archive — plaintext hashes and credentials — in the caller-supplied temp dir, which that service deliberately does not clean. The export phase removes it on error. Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM * fix(db): close review round 7 — pin bypass on direct start, real admins (#1038) 1. server.js only ran the engine resolver when DATABASE_CLIENT was unset, so a deployment that both bypasses the entrypoint (Kubernetes `command:`) AND pins DATABASE_CLIENT=pg never consulted the migration pin — the round-6 fix was unreachable on exactly that path, and a failed migration would have served a half-populated Postgres. The resolver now also runs whenever a pin file exists. 2. Round 5 excluded admin_users from Postgres occupancy to stop a seeded bootstrap admin counting as real data. That over-corrected: an install that has completed first-run setup but has no galleries yet has exactly one user-created row — an admin — so Postgres looked empty and, with a stale SQLite file present, the boot would switch away and the admin's credentials and configuration would disappear. core/001_init.js seeds must_change_password=true; setupService writes false once a human completes setup. The FLAG, not the table, distinguishes them, and a legacy NULL counts as a real admin. Verified against a real PostgreSQL 15: a Postgres holding only the seeded row loses to real SQLite data, the same Postgres wins once setup is completed, and a server started directly with DATABASE_CLIENT=pg and a pin present comes up on SQLite with the warning. Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM * fix(db): close review round 8 — reset admins, CLI config, JSON nulls (#1038) 1. must_change_password is mutable: resetAdminPassword() re-raises it on REAL accounts (userManagementService.js:474). Round 7's discriminator therefore read a gallery-less Postgres whose only admin had been reset as an untouched bootstrap seed — and with a stale SQLite file present, the boot would have switched away and hidden those live credentials. The rule is layered now: more than one admin, any admin that has logged in, or must_change_password false all count as use. Only core/001_init.js's exact leftovers — one admin, never logged in, still flagged — read as a seed. 2. The CLI read process.env directly but never loaded the configuration the child phases get through knexfile, so running it directly (or via `docker exec`, which does not inherit wait-for-db.sh's exports) failed the pre-flight checks even with valid settings in backend/.env or /run/secrets/db_password. Both sources are loaded up front now. 3. The migration's target check counted a seeded bootstrap admin as user data while probePgData classified the identical row as empty, so migrating into a previously-initialised-but-unused Postgres demanded --force. Same rule on both sides. 4. Cross-engine JSON handling is simpler and no longer lossy. SQLite keeps json columns as TEXT holding valid JSON and Postgres accepts JSON text directly, so the correct action is to pass them through untouched. Round 1 parsed then re-serialised them to undo a double-stringify; that round-tripped the JSON literal `null` into SQL NULL, changing data and breaking NOT NULL json columns. Not serialising at all fixes both. Verified against a real PostgreSQL 15: a migrated install now carries json_typeof = null for a JSON null, object for a nested object, and boolean for a boolean — matching a native install exactly. Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM * fix(db): close review round 9 — probe error classes, marker ordering (#1038) 1. probePgData() answered every failure with "Postgres has data". That is right for an unreachable server — the app cannot run on it either way, and diverting a healthy pg install to a stale SQLite file over a transient blip would be worse — but wrong for a server that answers and then fails the query, which is what a half-built or damaged schema looks like. That is not evidence of data, and reporting it as such booted the empty Postgres and hid a populated SQLite file: the exact failure this guard exists to prevent. Reachability is now established with SELECT 1 first, so the two cases get opposite answers: unreachable → leave the configured engine alone; reachable-but-uninspectable → unproven, and the SQLite side wins if it actually holds data. 2. The success marker was written after the SQLite file was renamed away. A failure in between — a full disk — left the source retired with no marker: the next attempt reported "No SQLite database", the in-progress pin stayed, and the operator never saw the rollback path. The marker is written first and updated with the retired filename once the rename succeeds, so a failure at any point leaves everything recoverable. Verified against a real PostgreSQL 15: a reachable database whose admin_users table lacks the probed column now resolves to sqlite3 rather than hiding the data, while an unreachable host still resolves to pg. Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM * fix(db): don't fail the migration on empty SQLite-only tables (#1038) Review round 10. The final verification flagged every source table missing from Postgres, regardless of whether it held rows — and SQLite-only tables do exist: initializeDatabase() creates an `events_new` scratch table and, when its legacy column copy throws, the catch swallows the error and leaves the empty table behind (db.js:236). The importer correctly skips tables Postgres does not have, so verification then reported a mismatch AFTER the data had already landed, exited 1, and left the install pinned to SQLite with no way to finish. An absent target table only matters if the source actually had rows. Empty ones are now listed and skipped. Reproduced both ways against a real PostgreSQL 15 with an events_new table present: without the fix the run ends in "ROW COUNTS DO NOT MATCH" and leaves the in-progress pin; with it, the table is reported as skipped, the migration completes and the pin is released. Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM * fix(db): a completed migration overrides an implicit SQLite config (#1038) Review round 11. The migration allowed the one configuration it should have worried about most: DATABASE_CLIENT unset AND NODE_ENV not "production", which resolves to the development block — i.e. sqlite3. That is precisely the state the affected installs are in, since it is why they ended up on SQLite at all, so an operator can easily run the migration before fixing it. The script then renames the source database away, and the next start resolved to the implicit sqlite3, created a NEW empty database and served it — after reporting success. The success marker now overrides an IMPLICITLY resolved sqlite3 when Postgres settings are present, because the marker is durable proof of where the data actually went. An explicit DATABASE_CLIENT=sqlite3 still wins: that is the documented rollback. The script says something rather than refusing — refusing would block exactly the population this exists for. Reproduced with NODE_ENV and DATABASE_CLIENT both empty, against a real PostgreSQL 15: the migration completes, the source is renamed away, and the next boot resolves to pg with the data intact. Before this it resolved to sqlite3 and would have served an empty database. Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM * refactor(db): drop the dead reachability flag in probePgData (#1038) github-code-quality flagged `if (reachable)` as always true, and it is right: the unreachable branch returns, so everything below it runs only when the probe connected. The variable and the conditional were leftovers from a first draft that used a single catch for both failure classes. No behaviour change — the two error paths still return opposite answers. Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM * fix(db): refuse to choose when both databases hold data (#1038) Review round 12. 1. An install that ran on PostgreSQL, lost NODE_ENV/DATABASE_CLIENT, and kept working on SQLite has REAL data on both sides: old rows in Postgres, newer ones in SQLite. The stranded-data rule only protected SQLite when Postgres was empty, so pulling this fix would have booted Postgres and hidden every gallery created since the switch — the exact failure this PR exists to prevent, in a variant I had not considered. A completed migration leaves a marker saying which side is current. Without one, two populated databases are a conflict: the boot stops and prints both targets, the two DATABASE_CLIENT values that resolve it, and the migration command that merges them. This is the only deliberate refusal in the change — guessing here would hide data AND split subsequent writes across two databases. 2. probePgData was handed knexConfig.connection even when knexfile had resolved to SQLite (a completed migration whose environment still says sqlite3), so node-postgres dialled its own localhost defaults instead of DB_HOST/DB_NAME — false "unreachable" diagnostics and a needless delay on every boot. The probe target is now built from the environment when the config is not pg. The conflict is honoured by all three entry points: the resolver exits 3 with an empty stdout, wait-for-db.sh stops the container, and server.js refuses to start. Two existing tests asserted that Postgres wins when both sides hold data. They encoded the pre-conflict assumption and described a state that cannot occur after a real migration (which always leaves a marker); both now pass the marker. Found while testing: the resolver's logger shim had no .error, so the conflict path threw, was swallowed by the fallback, and silently chose Postgres — the precise outcome this refuses to make. The shim is complete now. Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM * fix(db): symmetric bootstrap rule, one resolved Postgres target (#1038) Review round 13. Both findings are consequences of earlier rounds. 1. The conflict rule added in round 12 counted an untouched SQLite bootstrap admin as data. core/001_init.js seeds one whenever ADMIN_PASSWORD is set — including into the accidental SQLite database — so a healthy Postgres install that had ever started once without NODE_ENV would have had a seeded-only SQLite file beside it, been declared a both-populated conflict, and REFUSED TO BOOT. The bootstrap discrimination is applied on both sides now; a setup-completed or logged-in admin still counts as real use on either. 2. The CLI's child phases inherited whichever knexfile block NODE_ENV selected. The development block defaults Postgres to localhost/postgres/photo_sharing, production to db/picpeak/picpeak — and this script is explicitly meant to run with NODE_ENV unset. With DB_USER/DB_NAME left to defaults it would therefore have migrated into `photo_sharing`, after which following the script's own advice to set NODE_ENV=production pointed the app at an empty `picpeak`. The target is resolved once, with production defaults, and passed explicitly to every phase — so the block knexfile happens to pick can no longer decide which database the data lands in. The pin and success marker record that same resolved identity. Verified against a real PostgreSQL 15: a live Postgres beside a seeded-only SQLite file now boots pg rather than refusing, flipping that admin to setup-completed restores the conflict, and a migration records localhost:7102/picpeak_r13b as its target rather than a defaulted guess. Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM * fix(db): one Postgres identity everywhere; protect the credentials file (#1038) Review round 14. Three of the six findings were the same defect as round 13's, surfacing through paths that fix did not cover: the connection used to PROBE or MIGRATE could differ from the one the application then OPENS, because knexfile's development block points Postgres at localhost/postgres/photo_sharing while production uses db/picpeak/picpeak. 1. server.js exported only DATABASE_CLIENT=pg after the resolver decided, so knexfile filled in host/user/database from whichever block NODE_ENV selected. With SQLite already retired by a migration, that meant opening an empty database. The whole connection is pinned now. 2. Two defaults existed for DB_HOST: wait-for-db.sh resolves and exports `postgres`, knexfile's production block says `db`. Since the entrypoint exports its value, `postgres` is what a running container actually uses — so a `docker exec` migration, which inherits neither, has to agree with that, not with the default that is only reached when the entrypoint did not run. 3. The migration's Postgres phases inherited an unset NODE_ENV and therefore the development block, which ignores DB_SSL entirely — a managed Postgres requiring TLS could never be migrated into. The phases run with production semantics now. 4. core/001_init.js writes data/ADMIN_CREDENTIALS.txt, and that data directory belongs to the SOURCE install. Bootstrapping the Postgres schema replaced the operator's real credentials file with ones for a temporary admin the import immediately discards. The file is preserved across the phase, including when it fails. 5. The boot line described knexConfig, so an install redirected to Postgres by a migration marker still logged "Database engine: sqlite (...)", contradicting the warning printed one line earlier. 6. On a both-populated conflict resolveBootEngine returns client:null, and both migration runners told the operator their data was in "null" and to set DATABASE_CLIENT=null. They now present the two real choices. Verified against a real PostgreSQL 15: a migrated install started directly with NODE_ENV unset now logs `postgres (localhost:7102/picpeak_r14)` and opens it, where before it would have gone to the development block's photo_sharing. Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM * refactor(db): resolve the PostgreSQL target in exactly one place (#1038) Rounds 13 and 14 both traced back to the same thing, each time through a caller the previous fix had not covered: three different defaults existed for the same connection. knexfile development : localhost / postgres / photo_sharing knexfile production : db / picpeak / picpeak wait-for-db.sh : postgres / picpeak / picpeak (and it EXPORTS them) So a process that probed or migrated against one could hand over to a process that opened another. Patching each caller was not converging — the guard, then the CLI's child phases, then server.js — so this deletes the divergence instead. `src/utils/pgConnection.js` now owns the resolution and knexfile's development and production blocks both derive from it, as does the engine guard. Same shape as the earlier sqlitePath.js extraction, for the same reason. The database NAME is what made this dangerous: a wrong host or user fails loudly at connect time, while a wrong name connects fine and presents an empty installation. BEHAVIOUR CHANGE: with DATABASE_CLIENT=pg and no DB_* variables, a non-production environment now resolves to postgres/picpeak/picpeak instead of localhost/postgres/photo_sharing. Deployments are unaffected — compose sets these explicitly and wait-for-db.sh exports them — but a local machine running Postgres bare now needs DB_HOST=localhost DB_USER=postgres DB_NAME=photo_sharing (or DATABASE_CLIENT=sqlite3, which is what backend/.env already uses). The failure mode of getting this wrong is a refused connection, not a silently empty database. Side effect worth having: DB_SSL is now honoured whatever NODE_ENV says, so the managed-Postgres case is fixed at the root rather than by forcing production semantics onto the migration's child phases. The test block keeps its own photo_sharing_test default — isolation is the point there. Verified: every block plus the guard resolve identically from the same environment; explicit DB_* still wins; production's pool tuning is preserved; and a full SQLite → PostgreSQL migration with NODE_ENV unset lands in the right database with JSON shapes intact. Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM * fix(db): two more components that guessed the database instead of asking (#1038) Both found while sweeping for copies of the connection defaults. Checked in detail first — one of my suspicions about them was wrong. scripts/set-admin-password.js hand-rolled its own knex config while all four sibling scripts (reset-admin-password, create-admin, show-admin-credentials, reset-admin-mfa) use the application's connection. Two consequences: - it read DB_CLIENT, a variable nothing else in this codebase sets, so it defaulted to Postgres and could not work on a SQLite install at all; - it defaulted to database `picpeak_dev`, a name no other component uses. It now uses `require('../src/database/db')` like its siblings, so it follows whatever engine the install actually runs on. Timestamps are written as ISO strings because it reaches SQLite now, where raw Date objects are the documented landmine. NOT changed: the script's "all existing sessions have been invalidated" notice is accurate — auth.js compares token iat against password_changed_at — and it deliberately leaves must_change_password alone, which is right for an operator choosing a password rather than being issued one. routes/adminSystem.js re-derived three things the live connection already knows, and each could disagree with it: - the engine, from DATABASE_CLIENT || 'sqlite3' — so a Postgres install without an explicit DATABASE_CLIENT took the SQLite branch; - the Postgres database, from DB_NAME || 'picpeak'; - the SQLite file, from a hardcoded ../../data/photo_sharing.db that ignored DATABASE_PATH entirely. All three now come from db.client.config, with pg_database_size(current_database()). Verified: set-admin-password works on SQLite (new hash verifies, old rejected) and still on PostgreSQL; and on a SQLite install with a custom DATABASE_PATH the size logic reports the real database (1,748,992 bytes) where the old code reported a different file entirely (1,851,392) — or 0 where that path does not exist. Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM * fix(db): bind the migration marker to its target; fix a phantom table (#1038) Review round 15. 1. The marker records `host:port/database`, but only its EXISTENCE was checked. Repoint DB_NAME or DB_HOST at a different, empty PostgreSQL after migrating and the marker would vouch for that one too — booting it, presenting an empty installation, and suppressing the SQLite fallback while the real data sits in the recorded target and the renamed rollback copy. The marker is compared against the current connection now, and a mismatch stops the boot with both targets named and the two ways out. 2. `incoming_invoices` is not a table — supplier documents live in `inbound_documents` (core migration 124). Both occupancy lists skip tables that do not exist, so those records were silently not protecting anything: an install whose only remaining data was inbound documents could be switched away from, or overwritten without --force. Verified every other name in the lists against the live schema at the same time. Verified: a marker naming picpeak_original with picpeak_mk configured refuses with exit 3 and prints both; making them agree boots pg. Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM --------- Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local> |
||
|
Paul Nothaft
|
de459c701f |
fix(feedback): persist guest feedback settings, unshadow the guest route (#1030) (#1032)
Enabling Guest Feedback on an event could silently do nothing.
1. `updateEventFeedbackSettings` spread the request body straight into the
knex UPDATE. The admin event form posts its whole client-side state,
including three keys that were never columns on event_feedback_settings
(`enable_rate_limiting`, `rate_limit_window_minutes`,
`rate_limit_max_requests`), so the write threw and the route answered 500.
Writable columns are now whitelisted; identity columns and timestamps stay
server-managed.
2. EventDetailsPage swallowed that 500 in a bare `catch {}` ("Error already
handled by mutation" — it is a different request), so the admin was left
looking at "Event updated successfully" while the toggle never persisted.
The error is surfaced now and the settings query is invalidated on success.
3. gallery.js declared a duplicate `GET /:slug/feedback-settings`. server.js
mounts galleryRoutes before galleryFeedback, so it shadowed the real
handler and dropped the per-guest caps (#655) from the guest payload — the
gallery could never render the favorite/like limits or their counters.
Timestamps are written as ISO strings so they round-trip on both engines.
Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
|
||
|
Paul Nothaft
|
8b6cd3c74f |
fix(gallery): coerce SQLite 0/1 booleans in the guest surface (#1028) (#1037)
SQLite stores booleans as 0/1, Postgres as true/false. The guest gallery
compared strictly against `true`/`false`, so every flag read backwards on
SQLite installs:
allow_downloads: 0 !== false → true (header Download button shown
with downloads disabled)
allow_user_uploads: 1 === true → false (upload button hidden with
uploads enabled)
Worse, all five download guards used `allow_downloads === false`, which never
fires against a stored 0 — so on SQLite "Allow photo downloads = off" was
inert end to end: single photo, download-all, download-selected, download-jobs
and the job-status poll all kept serving, as did the secure-images download
route. Per-category blocking (#640) was ignored for the same reason, the
protection toggles (right-click, devtools, canvas, watermark) reported false
while enabled, overlay_protection was stuck on, and show_feedback_to_guests
leaked feedback with the setting off.
The /info endpoint was already correct — it checks 0/'0' explicitly. The two
payloads had simply drifted. Everything now goes through parseBooleanInput
(utils/parsers.js), which normalises both engines and takes a per-column
default so legacy NULL rows keep their documented behaviour.
Tests run on the SQLite harness, so they assert the real engine values. Every
one of them fails on the unfixed code — the payload assertions return the
inverted value, and the guard assertions never get their 403 (the request
proceeds to serve instead).
Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
|
||
|
Paul Nothaft
|
2bdb1204fe |
fix(slideshow): stop "no crop" fit letterboxing a pre-cropped frame (stable) (#1015) (#1019)
Backport of #1018 to the curated channel; the reporter on #1015 is running v3.45.14. The slideshow resolved its image as preview_url || hero_url || url. preview_url is only emitted when lightbox_preview_enabled is on (default false), so a default install fell through to hero_url — the 1920x1080 fit:'cover' centre crop built for gallery header banners. object-fit: contain then letterboxed an already-cropped 16:9 frame. Emits slideshow_url (same aspect-preserved preview tier) unconditionally for image photos; the show prefers it and never falls back to hero_url. preview_url stays gated so the lightbox opt-in is unchanged. |
||
|
Paul Nothaft
|
bf9bd76278 |
fix(security): vet the destination project when linking a deal (stable) (#992)
Backport of #991. stable carried the identical code path and the same missing guards. A scoped admin could point a quote or contract at a project they do not own — the quote/contract create+update paths pass a body-supplied projectId with no ownership check, and linkDealToProject's lineage guard is skipped when the deal has no event yet. On an ownerless project this escalated to a read once the quote converted to an event. Vetted at the service choke point, ahead of both the null-deal early return and the customer check. 404 PROJECT_NOT_FOUND throughout. super_admin unaffected. |
||
|
Paul Nothaft
|
2d0e6ab2dc |
fix(projects): stop the cockpit offering email controls the API rejects (stable) (#977)
Closes #969 on stable. Backport of #976. The cockpit's email feed rendered preview/resend/cancel/retry/send-now for every mail regardless of role or permission, producing 404s (CRM document mail has no event_id; project ownership does not imply event ownership) and 403s (preview needs events.view, the 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. A missing canAct reads as false. |
||
|
Paul Nothaft
|
cc49f6997a |
fix(auth): fail closed when the adminAuth roles join errors (stable) (#975)
Closes #968 on stable. Backport of #974. The roles-join fallback in adminAuth fabricated role_name='super_admin' on ANY database error, so a transient fault silently granted super_admin for its duration. Gate it on isMissingRolesSchema(), moved to utils/dbErrors.js and shared with apiTokenAuth, with the predicate tightened to trust SQLSTATE 42P01/42703 on Postgres and exact driver phrasing on SQLite. |
||
|
Paul Nothaft
|
fecc18cbc8 |
fix(security): enforce project ownership on project + project-email routes (stable) (#966)
* 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 (cherry picked from commit 688e318850db1b5f4ea2a4ae3c0fcf0fc137620d) * docs(security): drop the stale ownership JSDoc left by the rebase (GHSA-wrg5) Rebasing onto stable (which had gained scopeEventsQuery from #963) 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 <paul@MacStudio-von-Paul.local> |
||
|
Paul Nothaft
|
7f27e6771f |
fix(security): escape brand tokens, block tracker redirects, trim logo diagnostic (stable) (#967)
* 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
(cherry picked from commit 093480a753ff3d4b6ed48dd9f1108f975c8e0d47)
* 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
(cherry picked from commit c6b95d3cd1cb28e5c2828d29d4d63fadad981dcf)
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
|
||
|
Paul Nothaft
|
4e99897313 |
fix(security): enforce event ownership on the v1 API surface (GHSA-9697) (stable) (#963)
* 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 (cherry picked from commit 53d1e5d1b3148a7f4067308b08fcdf8ddab0a39f) --------- Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local> |
||
|
Paul Nothaft
|
ccab9024d4 |
fix(security): bound inbound-mail resources, redact secrets from logs (stable) (#965)
* 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. (stable) * 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 (cherry picked from commit 9a54b6f0231c3285df4c4865eb846e63e1ed0dda) --------- Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local> |
||
|
Paul Nothaft
|
11f9f584de |
fix(security): scope dashboard stats/analytics/activity to the caller's events (stable) (#964)
* 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
(cherry picked from commit 459e9e42434defd0dc7b87246e4d894dd47dcc56)
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
|
||
|
Paul Nothaft
|
3b88036fda |
fix(security): backup/restore hardening — public-dir DB dump, restore path allowlist, gunzip bound, manifest keying (stable) (#962)
* 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
(cherry picked from commit 292a5b784acd7f47099aa234c1c2ea00050fca97)
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
|
||
|
Paul Nothaft
|
5d5db4e766 |
fix(security): authz/ownership gaps (token binding, auth revocation, feedback/customer ownership, token logging) (stable) (#951)
* 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 <paul@MacStudio-von-Paul.local> |
||
|
Paul Nothaft
|
e5dccf1664 |
fix(security): neutralize spreadsheet formulas in all CSV/export cell-writers (CSV injection cluster) (#949)
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local> |
||
|
Paul Nothaft
|
bfafecedc7 |
fix(security): unauth share_token leak (HIGH) + restore path-traversal, logo file-read (stable) (#947)
* 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 <paul@MacStudio-von-Paul.local> |
||
|
Paul Nothaft
|
2462ba6897 |
fix(security): close authorization/ownership gaps (token scope, mass-assignment, category hero, project docs) (stable) (#944)
* 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 <paul@MacStudio-von-Paul.local> |
||
|
Paul Nothaft
|
90275f88e9 |
fix(security): resolve DNS before vetting external hostnames (SSRF cluster) (stable) (#942)
* 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 <paul@MacStudio-von-Paul.local> |
||
|
Paul Nothaft
|
34a7b1c013 |
fix(security): block guest access to hidden/client-only photos across bulk + secure routes (stable) (#940)
* 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 <paul@MacStudio-von-Paul.local> |
||
|
Paul Nothaft
|
fc99e2b233 |
fix(uploads): prevent cross-photo contamination from filename collisions and non-atomic writes (#931) (stable) (#934)
* 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) * chore(deps): promote p-limit to a direct dependency for the watermark limiter (#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 <paul@MacStudio-von-Paul.local> |
||
|
Paul Nothaft
|
60cbda5b22 |
fix(security): close GHSA-g94x (cross-gallery photo read) + GHSA-pv6w (admin DB export) (stable) (#925)
* fix(security): close two access-control advisories (GHSA-g94x, GHSA-pv6w) (stable) 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. Stable port of #924. secureImages on stable has no reveal-mode block, so only the token-binding checks are added; the backup export gate is identical. * 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 <paul@MacStudio-von-Paul.local> |
||
|
Paul Nothaft
|
a27d19b4d1 |
fix(admin): expose view/download counters in the admin photos list (#895 follow-up) (stable) (#915)
* fix(admin): expose view/download counters in the admin photos list (#895 follow-up) (stable) 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. Includes the one-line chunkedUploadService unref from #911 so the test suite can mount adminPhotos regardless of merge order (identical change, merges cleanly either way). * 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 <paul@MacStudio-von-Paul.local> |
||
|
Paul Nothaft
|
d68d84e5c8 |
fix(admin): serve videos with their real MIME type in the admin photo view (#908) (stable) (#911)
* fix(admin): serve videos with their real MIME type in the admin photo view (#908) (stable) 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. * 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): 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): 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. --------- Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local> |
||
|
Paul Nothaft
|
90b589a88e |
fix(analytics): make per-photo view/download counters actually count (#895) (stable) (#905)
* fix(analytics): make per-photo view/download counters actually count (#895) (stable) 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 <paul@MacStudio-von-Paul.local> |
||
|
Paul Nothaft
|
d868aac703 |
fix(security): close 5 Trivy alerts — postcss/tar bumps + drop npm from the runtime image (stable) (#879)
* 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. (stable) * 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. (stable) |
||
|
Paul Nothaft
|
a27c705e39 |
fix(backup): make backup settings actually apply (#871) (stable) (#875)
* fix(backup): make backup settings actually apply (#871) (stable) - 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. * fix(backup): close the review gaps in the settings wiring (stable) - 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. (stable) * 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. (stable) |
||
|
Paul Nothaft
|
39696d42fe |
fix(security): bump backend deps to close all 14 open Trivy code-scanning alerts (stable) (#870)
* fix(security): bump backend deps to close all open Trivy code-scanning alerts (stable) - 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 (stable) 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 (stable) 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 (stable) --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 Nothaft
|
50f5ca1d5b |
fix(security): read the password-complexity key the settings UI writes (stable) (#844)
* 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
|
8978acdb49 |
fix(events): accept hero_logo_visible: null on create/update (#822)
hero_logo_visible is nullable — null means "inherit the global branding_logo_display_hero toggle" (#756, migration 152). But the create and update validators used `.optional()` without `{ nullable: true }`, which only skips `undefined`; an explicit `null` still ran `.isBoolean()` and failed with HTTP 400 "Invalid value". Saving an event with `hero_logo_visible: null` (the inherit state the frontend sends) was rejected on v3.45.2. - Both routes: `body('hero_logo_visible').optional({ nullable: true }).isBoolean()`, matching the already-correct `hero_logo_size` rule next to it. - Create handler: guard on `!= null` instead of `!== undefined` so an explicit null stores NULL (inherit) rather than being coerced to 0/false by formatBoolean on SQLite. The update handler already did `=== null ? null`. Left hero_logo_position on plain `.optional()` on purpose: its column is NOT NULL (no inherit migration) and its handler always resolves to a concrete value via `|| brandingDefaults`, so null is genuinely invalid there — allowing it would trade the 400 for a 500. Adds smoke tests: PUT accepts hero_logo_visible: null and stores NULL; a non-boolean value is still rejected. |
||
|
Paul Nothaft
|
64bcd0ab9f |
fix(update): target docker-compose.production.yml in dashboard update steps
Production installs use docker-compose.production.yml (the README's documented
path, pinned GHCR images, no dev services), but the dashboard's update
instructions emitted bare `docker compose pull` / `up -d`. Bare `docker compose`
operates on docker-compose.yml — a different, build-based stack — so a
production user who followed the steps:
- never pulled/recreated their real containers (stayed on the old version,
e.g. stuck on 3.44.0 after "updating" to 3.45.2), and
- started the dev-only mailhog service that docker-compose.yml defines
(reported restart-looping).
The backend runs inside a container and can't stat the host's compose files, but
docker-compose.production.yml passes PICPEAK_RELEASE_CHANNEL into the backend env
and docker-compose.yml does not. detectEnvironment() now derives
isProductionCompose from it, and the Docker update steps prepend
`-f docker-compose.production.yml` when set. The non-production branch keeps the
bare commands but the warning now tells users to add `-f docker-compose.production.yml`
if they installed with it.
Also gates the mailhog service in docker-compose.yml behind a `dev` compose
profile so a plain `docker compose up -d` never starts it (opt in with
`docker compose --profile dev up -d`). Nothing depends on it (SMTP_HOST comes
from .env), so gating is safe. Verified: `docker compose config` lists mailhog
only with `--profile dev`; production compose is unchanged.
Adds unit tests for the production-vs-default command generation.
|
||
|
Paul Nothaft
|
9ee3ff45d0 |
fix(security): remove unguarded legacy /api/events router (GHSA-4j34-x562-5vfq)
The legacy gallery router mounted at /api/events exposed create/list/update/
delete/extend guarded by adminAuth ALONE — no requirePermission, no
requireEventOwnership. adminAuth only checks the token is a valid type:'admin'
session, which every back-office role holds, down to read-only `viewer`. So any
non-super-admin account could:
- GET /api/events → every gallery's bcrypt password_hash, share_token, and
client name/email (the list handler selects * and mapEventForApi keeps
those columns),
- PUT /api/events/:id → reset any gallery's password (full takeover),
- DELETE /api/events/:id → delete any gallery,
all bypassing the per-photographer ownership isolation the canonical
/api/admin/events router enforces. Affects any instance with more than the
single super_admin.
Fix: remove the legacy router entirely (mount + require + src/routes/events.js).
It was a superseded duplicate of /api/admin/events and unused by the frontend
EXCEPT for one live route — POST /:id/extend (the "Extend expiration" UI action,
which hit /api/events/:id/extend via the api client's /api base). That route is
migrated to the canonical mount as POST /api/admin/events/:id/extend with the
same guards as every other gallery mutation (adminAuth + requirePermission
('events.edit') + requireEventOwnership), and the frontend is repointed to it.
Behaviour of the extend itself is unchanged (expires_at + reactivate).
Verified end-to-end on a booted instance: /api/events (all methods) now 404;
/api/admin/events/:id/extend returns 401 unauth, 200 for the owner, 403 for a
non-owning editor; the full login→create→extend flow works. Adds a regression
test pinning the router removal and the extend ownership check.
|
||
|
Paul Nothaft
|
b00a16159e |
fix(security): harden .picpeak restore operator-preservation (GHSA-qxfx follow-up)
The req.admin.id fix activated reinjectCurrentAdmin(); hardening its preservation logic (found across Codex review rounds of #811): - MFA hijack: reinject wrote back only password_hash/is_active/ must_change_password, leaving a crafted backup's two_factor_* on the operator's row — it could strip or replace their second factor. The email- matched row is now updated with the operator's full AUTH set (login identity, password, and all two_factor_* columns). Relationship/audit FKs (role_id, created_by) are deliberately NOT forced from the snapshot: on a cross-instance restore those pre-restore ids may be absent from the backup and would dangle the FK (SQLite rolls back at commit); the restored row keeps its own valid values. - Cross-instance restore rollback / FK safety: reinject matched only by email, so a backup shipping a different admin with the default `admin` username hit UNIQUE(username) and rolled the whole restore back; email and username could even collide on two different rows. Reconciliation is now non-destructive: the email-matching row is updated in place (id preserved → restored FKs like events.created_by stay valid); any different row holding the operator's username is RENAMED, not deleted (deletion would fire ON DELETE actions / dangle references); only when no row has the operator's email is a fresh row inserted, with created_by nulled and an explicit max(id)+1 id (batchInsert left the Postgres identity sequence unadvanced, so a sequence-based insert could collide). - Stale session after restore: admin_users ids shift on restore, but the operator's live JWT is bound only to decoded.id (IP logged not enforced; the backup controls password_changed_at). The route now revokes the token (result checked and logged) and clears the admin cookie; the client redirects to a fresh login via a sessionInvalidated flag. Cookie clear is the unconditional guarantee. Adds SQLite-backed reinject regression tests (in-place login/MFA restore with id and FK columns preserved, username-only rename, email+username on different rows, clean insert with created_by nulled) and the frontend redirect on sessionInvalidated. Deferred (design decisions / pre-existing, need a Postgres test env — see PR discussion): global "invalidate all pre-restore sessions" cutoff; preserving the operator's ROLE semantics across an RBAC-table replace; and resyncing Postgres identity sequences after any restore (batchInsert leaves them behind max(id) — pre-existing, affects every restored table). |
||
|
Paul Nothaft
|
dcfcb67f9b |
fix(security): sanitize chunked-upload filename (GHSA-pc72-jf53-w28j)
The chunked video upload stored req.body.filename unmodified and later built the merged path as path.join(tempDir, uploadMeta.filename). path.join does not neutralise '../', so a filename like '../../uploads/logos/evil.svg' escaped the temp dir on merge and overwrote arbitrary files. Requires admin with photos.upload. Fix: path.basename() the client filename in initializeUpload() and reject names that collapse to nothing. Adds a regression test. |
||
|
Paul Nothaft
|
cde0b465a9 |
fix(security): reject ZIP-slip entries in archive/backup restore (GHSA-jfhw-fj23-fx6x)
node-stream-zip's extract(null, root) writes each entry to path.join(root, entry.name) without neutralising '../', so a crafted archive entry named '../../uploads/logos/evil.svg' escaped the target dir and overwrote arbitrary files (logos, .env, route files → RCE on source deploys). Requires admin with archives.restore. Adds assertZipEntriesWithin() to utils/safePath.js — a lexical containment check run on the entry list BEFORE extract() — and guards both extract sinks: adminArchives.js (the reported route) and picpeakImportService.js (the sibling .picpeak import, same sink). Adds unit tests for traversal, absolute-path, and sibling-prefix entries. |
||
|
Paul Nothaft
|
28f69e4bf3 |
fix(security): share-login must not bypass gallery password (GHSA-9hmx-68vc-qpqw)
POST /auth/gallery/share-login validated only the 128-bit share token and then
minted a full type:'gallery' access token regardless of require_password —
computing requiresPassword at the end only to echo it, never enforce it. Anyone
holding a gallery's share link could read and download every photo in a
password-protected gallery via a direct API call, no password needed.
Fix: compute requiresPassword before minting; for a password-protected gallery
return { requires_password: true } with NO token and NO cookie. The client then
goes through /gallery/verify, which does bcrypt.compare the password. The public
(no-password) auto-login path is unchanged. The frontend already falls through
to the password prompt when share-login returns no token/event.
Adds route regression test covering the bypass, the public path, and bad tokens.
|
||
|
Paul Nothaft
|
1cf82d81a7 |
fix(security): preserve current admin on .picpeak restore (GHSA-qxfx-4493-4v8f)
adminAuth populates req.admin, not req.user, so currentAdminId was always undefined in the /api/admin/picpeak/import handler. reinjectCurrentAdmin() then had no account to preserve and the admin_users table was fully replaced by the uploaded backup — a crafted .picpeak let any admin with backup.restore take over every admin account (critical). One-line fix: pass req.admin.id. Closes GHSA-qxfx-4493-4v8f and its duplicate GHSA-pjp6-jcrj-3cr5. |
||
|
Paul Nothaft
|
a71b9b5ed7 |
Merge pull request #769 from Luca-Timo/feat/messages-email-client
feat(messages): unified Messages email client (flag-gated, default off) |
||
|
Luca
|
1e08a4fb15 |
fix(messages): PR #769 nits — server-side search, bare-email recipient, DE i18n
- Search now hits the backend (debounced) so results aren't truncated to the first loaded page: /received gains a `q` filter (sender/subject); the frontend passes the debounced term to every list query. The instant client-side filter stays for responsiveness. - Reply/compose recipient extracts the bare address from a "Name <addr>" From header (extractEmail) — also used for the customer-lookup key. - Added the full de + en `messages.*` and `email.customerMailbox.*` translation namespaces (were English inline-fallbacks only). Swiss-German spelling. |
||
|
Luca
|
bb235e72e5 |
fix(messages): PR #769 review — escape reply sender (XSS), gate backend routes, exact customer match
- BLOCKER: stored XSS via inbound sender display name. The reply stub built raw
HTML with the unsanitized From name and set it as innerHTML on the composer's
contentEditable (admin origin) → onerror JS ran on Reply. Now HTML-escape
from_address in the stub AND DOMPurify-sanitize the composer body before
innerHTML (defense in depth).
- Gate the NEW Messages routes with requireFeatureFlag('messaging') per-route
(queue/:id, received/:id, item/*, identities, accounts, accounts/test, send)
— NOT the shared /email mount, so the pre-existing email-config endpoints stay
ungated.
- DocumentActionModal auto-picks a customer only on an EXACT email match
(customer search is prefix/fuzzy), else leaves the picker to the admin.
|
||
|
Luca
|
99d5996561 |
feat(messages): search bar + Archive/Delete with Archived & Deleted folders
- Search box in the header filters the current folder's list (sender/subject),
client-side; works across the merged Archived/Deleted views too.
- Archive and Delete are now implemented as soft moves: migration 157 adds
mailbox_state ('active'|'archived'|'deleted') to email_queue + received_emails.
Archive → 'archived', Delete → 'deleted' (trash). Restore → 'active'. Deleting
FROM the Deleted folder is permanent (hard row delete).
- New cross-account system folders Archived + Deleted (merge sent + received of
that state, sorted by date). Normal folders now exclude archived/deleted.
- Backend: /queue + /received gain a `state` filter (default active + legacy
NULL); new POST /item/:kind/:id/state (archive/delete/restore) and DELETE
/item/:kind/:id (purge, email.edit).
- Toolbar Archive/Delete wired; Restore + "Delete permanently" shown in the
system folders.
Frontend build + migration boot (157) verified.
|
||
|
Luca
|
c8cb4c88ca |
harden(messages): SSRF guard on mailbox host, strict sandbox + sanitizer, per-account TLS
Pre-upstream review hardening: - /accounts + /accounts/test now reject private/internal IMAP/SMTP hosts via isPrivateIP(), matching /config + /incoming-config (SSRF). - QueueDetail body iframe uses sandbox="" (script-less, no same-origin) like the inbound pane, instead of allow-same-origin. - /send sanitizer drops the <style> tag + data: scheme to match the stricter inbound sanitizeBody allowlist. - Per-account SMTP transport sets tls.rejectUnauthorized explicitly. |
||
|
Luca
|
9596342d6a |
Merge pull request #764 from Luca-Timo/fix/dunning-backfill-on-enable
fix(workflows): backfill existing invoices + anchor dunning grace to due date when enabled (#750) |
||
|
Luca
|
f9c2b4ed75 |
fix(messages): dynamic addresses, branding accent, compose/sync, per-identity SMTP
Addresses dev-test feedback: - Sidebar + reading-pane addresses (rechnungen@ / hello@ / no-reply@) are now read from the mail config via GET /admin/email/identities, not hardcoded. - Highlight/selection now uses the branding accent (bg-accent-soft / text-on-accent-soft / accent-dark) instead of hardcoded blue, so it follows the admin's CI colour like the sidebar. - Header gains "New message" (compose) and "Sync" (poll mailboxes now) buttons. - Composer modal enlarged (920px, taller editable body). - Customer mailbox (hello@) now has BOTH incoming (IMAP) and outgoing (SMTP) settings — migration 156 adds smtp_* + from_* to mail_accounts; emailProcessor.sendRawEmail takes an accountKey and sends via that mailbox's SMTP identity (falls back to the global from). Manual/reply sends from the Messages UI use the 'customers' identity, so replies come from hello@. Frontend build + migration boot (156) verified. |