diff --git a/docs/AUDIT-2026-05-12.md b/docs/AUDIT-2026-05-12.md index b37b7d10..01c9bba7 100644 --- a/docs/AUDIT-2026-05-12.md +++ b/docs/AUDIT-2026-05-12.md @@ -1660,7 +1660,7 @@ primary berth, refreshing". `user.email` directly on the Better-Auth `user` row but never deletes the target user's existing sessions. Concurrent sessions of the affected user keep working under the new email (because Better-Auth indexes sessions by -user_id, not email) — that's fine. **But the _previous_ email is now free** +user*id, not email) — that's fine. \*\*But the \_previous* email is now free\*\* to be claimed by a fresh signup before the admin sends the "your email was changed" notice. There's no unique constraint that prevents an attacker from re-registering as `old@example.com` and taking over outgoing identity @@ -1688,13 +1688,13 @@ Fix: `CREATE UNIQUE INDEX user_email_changes_one_pending ON user_email_changes ### H-6. Email-confirm token isn't atomically consumed `src/app/api/v1/me/email/confirm/[token]/route.ts:28-57`. Three separate -statements: SELECT pending, UPDATE user, UPDATE pending.applied_at. No +statements: SELECT pending, UPDATE user, UPDATE pending.applied*at. No transaction wrapper. A user who double-clicks the email link (or a link preview pre-fetcher like Outlook SafeLinks) fires two near-simultaneous GETs. Both pass the `appliedAt IS NULL` check, both flip `user.email` (idempotent — same value), both mark applied. Functional, but the second audit-log entry is misleading. More importantly: if the second click -arrives 200ms later AND the user re-fired a _different_ change in between +arrives 200ms later AND the user re-fired a \_different* change in between that the first click happened to apply, you've stomped state. Fix: single transaction, `SELECT … FOR UPDATE` the pending row, branch on @@ -5332,7 +5332,7 @@ Either (a) collect every user on the port and run the same `deepMerge` chain per ### H4. Cross-port email collisions are non-deterministic -`public/interests/route.ts:90-114`: when a client_contact with the same email exists on a _different_ port, the code creates a new client. But `tx.query.clientContacts.findFirst` returns "any matching row" with no `ORDER BY` — subsequent submissions may pick either port's row first. Net: same email used cross-port, then resubmitted to the original port, can spawn 2nd/3rd same-port clients. Fix: filter the lookup by joining to `clients.port_id`, or scope the contact lookup to clients owned by the target port from the start. +`public/interests/route.ts:90-114`: when a client*contact with the same email exists on a \_different* port, the code creates a new client. But `tx.query.clientContacts.findFirst` returns "any matching row" with no `ORDER BY` — subsequent submissions may pick either port's row first. Net: same email used cross-port, then resubmitted to the original port, can spawn 2nd/3rd same-port clients. Fix: filter the lookup by joining to `clients.port_id`, or scope the contact lookup to clients owned by the target port from the start. ### H5. `portName` hardcoded as `'Port Nimara'` in four call sites @@ -6099,10 +6099,10 @@ of unrelated context. ### C3. `src/lib/services/notes.service.ts` — 1121 lines, near-pure duplication 6 entity-type branches per operation (clients / interests / yachts / companies -/ residential_clients / residential_interests). The `create` function alone +/ residential*clients / residential_interests). The `create` function alone (lines 689–846) is 158 lines of 6 copy-pasted insert-then-profile-lookup blocks; same for `update` (847–1019) and `deleteNote` (1020+). A -`tableForEntity()` dispatcher is _defined_ at line 82 then immediately silenced +`tableForEntity()` dispatcher is \_defined* at line 82 then immediately silenced (`void tableForEntity;` line 98) — i.e. the abstraction was started, abandoned, and the dead helper left in place. Aggregated listers (`listForClient/Yacht/ Company/ResidentialClientAggregated`) are 4 near-identical 100-line bodies. @@ -6404,10 +6404,12 @@ Every cross-port-aware code path re-derives "which ports can this user touch?" f ## 33. S3 vs internal DB pathing + storage routing audit (storage-pathing-auditor) # Audit — S3 vs Internal DB Pathing + Storage Routing + Scope: `src/lib/storage/*`, every `getStorageBackend()` consumer, migration script, magic-byte enforcement, encryption-at-rest boundary. Date: 2026-05-12 ## Boundary summary (what lives where) + - **In DB (Postgres):** file metadata only — `files.storage_path`, `berth_pdf_versions.storage_key`, `brochure_versions.storage_key`, `gdpr_exports.storage_key`, `backup_jobs.storage_path`, user-avatar FK (`user_profiles.avatar_file_id` → `files`), document signing state (`documents.signed_file_id`). AES-256-GCM-encrypted **secrets**: `system_settings.storage_s3_secret_key_encrypted`, `storage_proxy_hmac_secret_encrypted`, `email_accounts.credentials_enc`, `webhooks.secret`, `ocr_config.api_key_encrypted`. No BYTEA / JSONB blobs found (`grep BYTEA → 0`). - **In backend (S3/filesystem):** every uploaded blob — signed PDFs (`buildStoragePath(slug,'eoi-signed',…)`), per-berth PDFs (`berths/{id}/…`), brochures, avatars, GDPR exports, pg_dump backups, expense receipts, generated reports, template source PDFs, send-out attachment fallbacks. - **Routing:** `getStorageBackend()` reads global `system_settings.storage_backend` ('s3'|'filesystem'), caches by config fingerprint, invalidated via `resetStorageBackendCache()` on settings write or migration flip. Code never imports `minio/Client` outside `s3.ts` (verified — only legacy `buildStoragePath` helper survives in `src/lib/minio/index.ts`). Interface methods: put/get/head/delete/listByPrefix/presignUpload/presignDownload — both backends implement all 7. @@ -6415,62 +6417,79 @@ Date: 2026-05-12 ## CRITICAL ### C1. `backup_jobs.storage_path` missing from `TABLES_WITH_STORAGE_KEYS` — silent backup loss on backend swap + `src/lib/storage/migrate.ts:55-60` lists only `files`, `berth_pdf_versions`, `brochure_versions`, `gdpr_exports`. `backup_jobs.storage_path` (pg_dump artefacts written by `src/lib/services/backup.service.ts:54+72`) is **not** in the list. Flipping S3 → filesystem (or vice-versa) leaves every historical database backup pointing at the old backend — `getBackupDownloadUrl(id)` will 404 / NoSuchKey, and the admin won't know until they try to restore. This is the worst category of data loss because backups are the recovery path of last resort. The comment in `migrate.ts:51` calls out `report_snapshots` as a future addition but mentions nothing about `backup_jobs`. **Add `{ table: 'backup_jobs', keyColumn: 'storage_path', pkColumn: 'id' }` and ship the line with a smoke test.** ### C2. Orphan-blob risk: every `backend.put` runs outside the `db.insert(files)` transaction + Pattern repeated across 9+ services (`files.ts:68-92`, `documents.service.ts:833-854` and `1134-1183`, `external-eoi.service.ts:71-96` — comment at L67-70 explicitly acknowledges "orphan reaper handles those" but **no reaper exists**, `invoices.ts:603`, `document-templates.ts:537,674`, `reports.service.ts:231`, `gdpr-export.service.ts:169`, `backup.service.ts:62`, `berth-pdf.service.ts:229`). Sequence is: PUT bytes → DB INSERT. If insert fails or the process dies in between, the blob is permanent and unreferenced. Only `handleDocumentCompleted` (`documents.service.ts:1110`) has an early-return idempotency gate; the rest leak. Over months of operation an S3 prefix accumulates dozens-to-hundreds of orphans that pay storage cost forever and survive every backup-restore. **Add an orphan-reaper maintenance job** that walks `listByPrefix()` against the union of all `storage_*` columns and deletes blobs older than 24 h without a DB pointer. Also wrap the `put + insert` pairs in a try/catch that explicitly deletes on insert failure (cheap defense in depth). ### C3. S3 backend stores blobs without server-side encryption (SSE-S3 / SSE-KMS) + `S3Backend.put()` (`src/lib/storage/s3.ts:191`) passes only `Content-Type` to `client.putObject`. No `x-amz-server-side-encryption` header. Bytes-at-rest encryption depends entirely on the bucket's default-encryption policy, which is invisible to the application — a customer who provisions a MinIO/B2/R2 bucket without server-side encryption gets cleartext signed contracts, GDPR exports, and `pg_dump` archives sitting on disk. Same audit posture as SMTP/IMAP creds (which **are** AES-GCM in the DB) demands the same guarantee for the blob plane. **Either add `ServerSideEncryption: 'AES256'` to every `putObject` call, or surface a boot-time check that asserts the bucket has default-encryption enabled and refuses to start otherwise** (similar to the `MULTI_NODE_DEPLOYMENT` guard on FilesystemBackend). ## HIGH ### H1. Berth-PDF presigned-upload keys are not port-scoped + `src/app/api/v1/berths/[id]/pdf-upload-url/handlers.ts:58` builds `berths/{berthId}/uploads/{uuid}_{name}` — no leading `${portSlug}/`. Result: the optional port-binding (`p` field on the HMAC token, enforced in `filesystem.ts:184-188` and documented in `index.ts:43-49`) cannot be wired here, and the storage-key namespacing convention diverges from `buildStoragePath` (which always prefixes the port slug). Tenant isolation today relies on the up-front `berths.portId === ctx.portId` check before mint, but the defense-in-depth port-binding is unwired. **Normalise the key to `${portSlug}/berths/...` and pass `portSlug` into `backend.presignUpload`.** ### H2. `presignDownload` callers never pass `portSlug` — port-binding token guard is dead code + `presignDownloadUrl(...)` (`storage/index.ts:233`) accepts `portSlug` and only 1 of ~12 callers uses it. `files.ts:117,128`, `backup.service.ts:115`, `portal.service.ts:351`, `reports.service.ts:170`, `gdpr-export.service.ts:224,282` all pass undefined. The filesystem-proxy GET will therefore accept any valid HMAC token regardless of the storage-key's port prefix. The check is genuinely defensible (see `filesystem.ts:179`) but never engaged. **Plumb the active port slug through every call site, or remove the optional `p` field and the verifier code so the contract isn't misleading.** ### H3. `S3Backend.put` and `backup.service` buffer entire blobs into memory + `s3.ts:187` (`Buffer.isBuffer(body) ? body : await streamToBuffer(body)`) and `backup.service.ts:60-62` (concatenates the entire pg_dump dump into memory before put). For a multi-GB database dump the worker OOMs. Comment at `s3.ts:184-187` explicitly says "typical files are under 50MB" but `runPgDump` writes a dump file whose size scales with the tenant. **Use `client.fPutObject` (file-path streaming) for backups; for streamable callers expose a `putStream(key, stream, sizeBytes, opts)` overload that pipes without `streamToBuffer`.** ### H4. Migrator's `copyAndVerify` double-buffers every blob and has no streaming hash + `storage/migrate.ts:170-204` reads source → Buffer, sha256, put, then re-reads target → Buffer, sha256 again. For a 5 GB pg_dump (see C1 — once added) this allocates ~10 GB of heap. The sha256-verify round-trip is the right idea; **pipe through `crypto.createHash` on both legs**, never buffer. ### H5. `S3Backend.presignUpload` lacks content-type / content-length binding + `s3.ts:249-256` only calls `presignedPutObject(bucket, key, expiry)`. The signed URL does not bind `Content-Type` or `Content-Length` — a browser can PUT 1 GB of arbitrary bytes against an EOI-signed key. Caps and magic-byte checks fire only on the **register** call afterwards (`registerBrochureVersion` and `uploadBerthPdf` HEAD-then-stream-first-5-bytes path). That's sufficient for the two consumers today, but the gate is one-deep — a future caller that forgets to wire a register endpoint exposes raw S3 directly. **Switch to MinIO `presignedPostPolicy` with `content-length-range` + `Content-Type` conditions so the binding is on the signature itself.** ## MEDIUM ### M1. CLAUDE.md drift on "TABLES_WITH_STORAGE_KEYS populated in 9a5ba87" + CLAUDE.md says the migrator covers "every blob in `files`, `berth_pdf_versions`, `brochure_versions`, `gdpr_exports`". Verified true — but **backup_jobs is the missing 5th** (see C1). Update the doc + add a unit test that asserts the array matches the set of tables with a `storage_*` column. ### M2. `email-compose.service.ts:124` reads attachment bytes into a Buffer + Each attachment under the `email_attach_threshold_mb` cap is fetched via `storage.get(...)` and concatenated. With multiple recipients × multiple attachments the worker holds N × size MB simultaneously. **Stream into `nodemailer`'s `content: ` API directly.** ### M3. UUID storage keys never check existence before put (no `If-None-Match: "*"`) + `crypto.randomUUID()` collision is astronomical, but a buggy caller passing a duplicate key (or a re-run of a worker after a partial DB rollback) silently overwrites. **Cheap belt: pass `If-None-Match: '*'` (S3) or `O_EXCL` (filesystem) — surfaces double-writes loudly.** ### M4. Per-port S3 routing not possible / `listByPrefix` unbounded + Storage config rows are global (`portId IS NULL`). Multi-tenant can't direct port-A vs port-B to separate buckets / KMS keys. `listByPrefix` returns every key in one array — script-only today but a footgun if called with empty prefix in production. **Document the global-config assumption; add a cursor variant before any per-port-bucket customer lands.** ### M5. `storage_filesystem_root` change invalidates outstanding HMAC tokens silently + Cache swaps, but tokens minted under the old root still verify HMAC; `resolveKeyForProxy` then 404s under the new root. Customer download links emailed an hour earlier break with no warning. **Either refuse runtime root changes, or warn in admin UI.** ### M6. Avatar URLs re-presign every 15 min — browser cache broken + No CDN / `s-maxage` fronts hot reads. Per-page avatar GET burns a presign + S3 round-trip. **Issue 24 h URLs for `category='avatar'`, or front with the Next.js Image route.** ### M7. Verified clean + - `withTimeout(...)` wraps every minio call (s3.ts L143/150/190/203/219/237/285/292/300); `system-monitoring.service.ts:153` adds its own 5 s deadline. **No bare minio calls escape.** ✓ - `MULTI_NODE_DEPLOYMENT` guard reads `env.MULTI_NODE_DEPLOYMENT` (zod-coerced, `env.ts:80`), test at `filesystem-backend.test.ts:139`. ✓ ### M8. Magic-byte enforcement + - **In-server uploads:** `files.ts:58` (`bufferMatchesMime`), `berth-pdf.service.ts:218` (`isPdfMagic`). ✓ - **Presigned-PUT post-upload register:** `brochures.service.ts:258` (first-5-byte stream + `%PDF-`), `berth-pdf.service.ts:259` (`readFirstBytes` + `isPdfMagic`). ✓ - **Filesystem proxy PUT:** inline check `route.ts:220` when token's `c=application/pdf`. ✓ - **S3 direct PUT:** no inline check (relies on the register endpoint). Acceptable per CLAUDE.md, but document divergence: a future S3 consumer that forgets to call register leaks the gate. ## Verified-clean (informational) + - No BYTEA / binary-JSONB blob columns. ✓ - Single canonical key format mismatch (`storage_path` vs `storage_key`) is documented + handled by per-table column mapping. ✓ - `validateStorageKey` rejects traversal, absolute paths, dotfiles, and >1024 chars. ✓ @@ -6480,6 +6499,7 @@ No CDN / `s-maxage` fronts hot reads. Per-page avatar GET burns a presign + S3 r - All blob keys are UUID-namespaced — collision-safe, not deterministic-audit-style. ✓ ## Recommended ordering + 1. **C1** (one-line fix + smoke test) before any backend migration ships. 2. **C2** orphan reaper — cron job behind `maintenance` worker. 3. **C3** SSE-S3 — single-line putObject change + bucket-policy assertion at boot. diff --git a/docs/AUDIT-TRIAGE.md b/docs/AUDIT-TRIAGE.md index 2b87f5c1..b27225c6 100644 --- a/docs/AUDIT-TRIAGE.md +++ b/docs/AUDIT-TRIAGE.md @@ -8,14 +8,14 @@ Companion to `AUDIT-2026-05-12.md`. Every line below is a real finding from the Anything here is a foot-gun that's actively armed in production right now. -| # | What | Where | Why now | -|---|---|---|---| -| 0.1 | Build a real `db:migrate` runner | new tsx script | `pnpm db:push` silently skips `CREATE INDEX CONCURRENTLY` (6 indexes in 0052 never created) and skips 2 structural constraints. Every other "migration X exists" claim is unverifiable until this is fixed. | -| 0.2 | `EMAIL_REDIRECT_TO` prod refusal in `src/lib/env.ts` | env zod refine | One stray env value silently funnels every outbound (invites, EOI, portal magic links, contracts) to a single inbox. Only signal today is `logger.debug`. | -| 0.3 | Admin self-target audit-log retention + alerting | audit_logs metadata + retention cron | `audit_logs.metadata` not in `maskSensitiveFields`, no retention cron. PII grows unbounded; rotated-admin compromise is invisible. | -| 0.4 | Resolve-identifier hit-path still echoes the real email | `/api/auth/resolve-identifier/route.ts` | Rate-limit is in (just shipped), but on a hit we still return the canonical email. Replace with a server-side signIn proxy that takes `{identifier, password}` and never returns the email at all. | -| 0.5 | Orphan-blob windows in 9+ services | `documents`, `brochures`, `invoices`, `gdpr-export`, `backup`, `berth-pdf`… | Every `storage.put` runs outside the `db.insert(files)` tx. "Reaper handles it" comment is wrong — no reaper exists. Months of operation = hundreds of orphans. | -| 0.6 | `backup_jobs.storage_path` missing from `TABLES_WITH_STORAGE_KEYS` | `src/lib/storage/migrate.ts:55-60` | Flip the storage backend → silently orphans every pg_dump. Last-resort recovery path goes dark. | +| # | What | Where | Why now | +| --- | ------------------------------------------------------------------ | --------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| 0.1 | Build a real `db:migrate` runner | new tsx script | `pnpm db:push` silently skips `CREATE INDEX CONCURRENTLY` (6 indexes in 0052 never created) and skips 2 structural constraints. Every other "migration X exists" claim is unverifiable until this is fixed. | +| 0.2 | `EMAIL_REDIRECT_TO` prod refusal in `src/lib/env.ts` | env zod refine | One stray env value silently funnels every outbound (invites, EOI, portal magic links, contracts) to a single inbox. Only signal today is `logger.debug`. | +| 0.3 | Admin self-target audit-log retention + alerting | audit_logs metadata + retention cron | `audit_logs.metadata` not in `maskSensitiveFields`, no retention cron. PII grows unbounded; rotated-admin compromise is invisible. | +| 0.4 | Resolve-identifier hit-path still echoes the real email | `/api/auth/resolve-identifier/route.ts` | Rate-limit is in (just shipped), but on a hit we still return the canonical email. Replace with a server-side signIn proxy that takes `{identifier, password}` and never returns the email at all. | +| 0.5 | Orphan-blob windows in 9+ services | `documents`, `brochures`, `invoices`, `gdpr-export`, `backup`, `berth-pdf`… | Every `storage.put` runs outside the `db.insert(files)` tx. "Reaper handles it" comment is wrong — no reaper exists. Months of operation = hundreds of orphans. | +| 0.6 | `backup_jobs.storage_path` missing from `TABLES_WITH_STORAGE_KEYS` | `src/lib/storage/migrate.ts:55-60` | Flip the storage backend → silently orphans every pg_dump. Last-resort recovery path goes dark. | --- @@ -23,15 +23,15 @@ Anything here is a foot-gun that's actively armed in production right now. Anything here puts the company in a regulator finding or a court case. -| # | What | Where | -|---|---|---| -| 1.1 | GDPR Article-15 export bundle is incomplete | `gdpr-bundle-builder.ts` — missing portal_users, email_threads/messages, document_sends, reminders, files, scratchpadNotes, client_merge_log, contact_log, website_submissions, form_submissions | -| 1.2 | Right-to-be-forgotten doesn't actually erase | `client-hard-delete.service.ts` — verbatim PII survives in `email_messages.body_html`, `files`, `document_sends.recipient_email` | -| 1.3 | Activation/reset tokens travel in `?token=` URL query strings | portal-auth flow — leaks to browser history, proxy logs, Referer headers | -| 1.4 | `error_events.request_body_excerpt` redacts password/token but not email/phone/name/dob/address | error-classifier sanitizer | -| 1.5 | `audit_logs` no retention cron + IP captured on routine events | `lib/audit.ts` — lawful-basis-questionable | -| 1.6 | S3 backend ships without `ServerSideEncryption` header | `S3Backend.put` — signed contracts, GDPR exports, pg_dumps cleartext at rest unless bucket default is set | -| 1.7 | `audit_logs.metadata` carries raw PII (full emails) at portal-auth, crm-invite, hard-delete, email-accounts service sites | `maskSensitiveFields` skips metadata | +| # | What | Where | +| --- | ------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| 1.1 | GDPR Article-15 export bundle is incomplete | `gdpr-bundle-builder.ts` — missing portal_users, email_threads/messages, document_sends, reminders, files, scratchpadNotes, client_merge_log, contact_log, website_submissions, form_submissions | +| 1.2 | Right-to-be-forgotten doesn't actually erase | `client-hard-delete.service.ts` — verbatim PII survives in `email_messages.body_html`, `files`, `document_sends.recipient_email` | +| 1.3 | Activation/reset tokens travel in `?token=` URL query strings | portal-auth flow — leaks to browser history, proxy logs, Referer headers | +| 1.4 | `error_events.request_body_excerpt` redacts password/token but not email/phone/name/dob/address | error-classifier sanitizer | +| 1.5 | `audit_logs` no retention cron + IP captured on routine events | `lib/audit.ts` — lawful-basis-questionable | +| 1.6 | S3 backend ships without `ServerSideEncryption` header | `S3Backend.put` — signed contracts, GDPR exports, pg_dumps cleartext at rest unless bucket default is set | +| 1.7 | `audit_logs.metadata` carries raw PII (full emails) at portal-auth, crm-invite, hard-delete, email-accounts service sites | `maskSensitiveFields` skips metadata | --- @@ -39,87 +39,87 @@ Anything here puts the company in a regulator finding or a court case. Anything where the dashboard or a PDF lies to the user about money. -| # | What | Where | -|---|---|---| -| 2.1 | `pipelineValueUsd` sums mixed currencies as USD | `dashboard.service.ts:39-51`, KPI cards, pipeline-value tile, revenue forecast | -| 2.2 | Revenue PDF "TOTAL COMPLETED REVENUE" includes lost + cancelled | `report-generators.ts:126-140` — no outcome filter | -| 2.3 | Pipeline PDF crashes because `stageCounts` is missing `.groupBy()` | `report-generators.ts` | -| 2.4 | Hot-deals widget rank ladder uses wrong stage names (`'in_comms'`, `'deposit_10'`) | `dashboard.service.ts:198-208`, `hot-deals-card.tsx:26-36` | -| 2.5 | "Active interest" means **4 different things** across dashboard / kanban / hot deals / PDFs | extract `activeInterestsWhere(portId)` helper | -| 2.6 | Occupancy rate: KPI uses `berths.status`, analytics timeline uses `berth_reservations` — two different numbers on same dashboard | `dashboard.service.ts` | -| 2.7 | Revenue PDF unweighted vs dashboard weighted-by-`pipeline_weights` — will never reconcile | `report-generators.ts` | -| 2.8 | `expenses.amountUsd` snapshot uses edit-time rate not `expenseDate`; nulls when Frankfurter is down | `expenses.service.ts` | -| 2.9 | `convert()` rounds 2dp regardless of currency (JPY broken); invoice math has no rounding (sub-cent drift) | `currency.service.ts`, invoice math | +| # | What | Where | +| --- | -------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------ | +| 2.1 | `pipelineValueUsd` sums mixed currencies as USD | `dashboard.service.ts:39-51`, KPI cards, pipeline-value tile, revenue forecast | +| 2.2 | Revenue PDF "TOTAL COMPLETED REVENUE" includes lost + cancelled | `report-generators.ts:126-140` — no outcome filter | +| 2.3 | Pipeline PDF crashes because `stageCounts` is missing `.groupBy()` | `report-generators.ts` | +| 2.4 | Hot-deals widget rank ladder uses wrong stage names (`'in_comms'`, `'deposit_10'`) | `dashboard.service.ts:198-208`, `hot-deals-card.tsx:26-36` | +| 2.5 | "Active interest" means **4 different things** across dashboard / kanban / hot deals / PDFs | extract `activeInterestsWhere(portId)` helper | +| 2.6 | Occupancy rate: KPI uses `berths.status`, analytics timeline uses `berth_reservations` — two different numbers on same dashboard | `dashboard.service.ts` | +| 2.7 | Revenue PDF unweighted vs dashboard weighted-by-`pipeline_weights` — will never reconcile | `report-generators.ts` | +| 2.8 | `expenses.amountUsd` snapshot uses edit-time rate not `expenseDate`; nulls when Frankfurter is down | `expenses.service.ts` | +| 2.9 | `convert()` rounds 2dp regardless of currency (JPY broken); invoice math has no rounding (sub-cent drift) | `currency.service.ts`, invoice math | --- ## Tier 3 — Customer-visible polish (embarrassing in front of clients) -| # | What | Where | -|---|---|---| -| 3.1 | "Interest" / "lead" / "prospect" / "deal" used interchangeably in client-facing UI | `berth-detail-header.tsx`, `berth-tabs.tsx` "Deal Documents", `client-interests-tab.tsx`, `interest-tabs.tsx` | -| 3.2 | Portal renders raw machine enums to clients ("EOI: waiting_for_signatures", "hot lead") | `/portal/interests/page.tsx:80` | -| 3.3 | 16 destructive flows use native `window.confirm()` | cancel signing envelope, delete files, archive interest/company/yacht | -| 3.4 | Signing-status labels diverge across 5 surfaces (Hub / list / interest-tab / SigningProgress / notification-digest / realtime-toast) | normalize through one helper | -| 3.5 | 6× "Save" button variants ("Save" / "Save Changes" / "Save changes") + 6× "Saving..." vs "Saving…" | sweep | -| 3.6 | Live Documenso template missing `Berth Range` field — every multi-berth EOI ships with primary mooring only | Documenso admin | -| 3.7 | URL interpolations in every email template are unescaped (`href="${data.link}"`) — a `"` in any URL breaks out | escape + scheme allow-list in `shell.ts` | -| 3.8 | Admin email-template subject editor silently does nothing on 5 of 8 templates | wire `overrides.subject` | -| 3.9 | `/admin/email` Signature/Footer HTML fields write keys the shell never reads | wire `cfg.footerHtml` or delete fields | -| 3.10 | Mobile scan PWA "Save expense" sits flush against iPhone home indicator | safe-area-inset on ScanShell `
` | +| # | What | Where | +| ---- | ------------------------------------------------------------------------------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------- | +| 3.1 | "Interest" / "lead" / "prospect" / "deal" used interchangeably in client-facing UI | `berth-detail-header.tsx`, `berth-tabs.tsx` "Deal Documents", `client-interests-tab.tsx`, `interest-tabs.tsx` | +| 3.2 | Portal renders raw machine enums to clients ("EOI: waiting_for_signatures", "hot lead") | `/portal/interests/page.tsx:80` | +| 3.3 | 16 destructive flows use native `window.confirm()` | cancel signing envelope, delete files, archive interest/company/yacht | +| 3.4 | Signing-status labels diverge across 5 surfaces (Hub / list / interest-tab / SigningProgress / notification-digest / realtime-toast) | normalize through one helper | +| 3.5 | 6× "Save" button variants ("Save" / "Save Changes" / "Save changes") + 6× "Saving..." vs "Saving…" | sweep | +| 3.6 | Live Documenso template missing `Berth Range` field — every multi-berth EOI ships with primary mooring only | Documenso admin | +| 3.7 | URL interpolations in every email template are unescaped (`href="${data.link}"`) — a `"` in any URL breaks out | escape + scheme allow-list in `shell.ts` | +| 3.8 | Admin email-template subject editor silently does nothing on 5 of 8 templates | wire `overrides.subject` | +| 3.9 | `/admin/email` Signature/Footer HTML fields write keys the shell never reads | wire `cfg.footerHtml` or delete fields | +| 3.10 | Mobile scan PWA "Save expense" sits flush against iPhone home indicator | safe-area-inset on ScanShell `
` | --- ## Tier 4 — Authz / cross-tenant integrity -| # | What | Where | -|---|---|---| +| # | What | Where | +| --- | ---------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------ | | 4.1 | Port admin with only `admin.manage_users` can grant other users any leaf they don't hold themselves (sock-puppet escalation) | permission-overrides PUT + `updateUser` role reassignment — require caller-superset before write | -| 4.2 | `/api/v1/alerts` GET is ungated | add `admin.view_audit_log` | -| 4.3 | Webhooks bypass the platform-error pipeline entirely | `documenso/route.ts` — `captureErrorEvent` on handler throw, apply to all webhook routes | -| 4.4 | Search graph-expansion writes into all merged buckets without re-checking per-bucket `view` permission | `search.service.ts:1893-1915` — gate each merge call | -| 4.5 | "Convert to client" writes prefill qs params no consumer reads; inquiry_id linkage dropped forever | inquiry-inbox triage flow | -| 4.6 | Inquiry email dedup is case-sensitive (capital-letter resubmits = duplicate client+yacht+interest) | `lower()` on `clientContacts.value === data.email` | +| 4.2 | `/api/v1/alerts` GET is ungated | add `admin.view_audit_log` | +| 4.3 | Webhooks bypass the platform-error pipeline entirely | `documenso/route.ts` — `captureErrorEvent` on handler throw, apply to all webhook routes | +| 4.4 | Search graph-expansion writes into all merged buckets without re-checking per-bucket `view` permission | `search.service.ts:1893-1915` — gate each merge call | +| 4.5 | "Convert to client" writes prefill qs params no consumer reads; inquiry_id linkage dropped forever | inquiry-inbox triage flow | +| 4.6 | Inquiry email dedup is case-sensitive (capital-letter resubmits = duplicate client+yacht+interest) | `lower()` on `clientContacts.value === data.email` | --- ## Tier 5 — Concurrency / data races -| # | What | Where | -|---|---|---| -| 5.1 | `handleDocumentCompleted` idempotency gate is TOCTTOU under webhook+poll race — duplicate files rows + orphan blob | `documents.service.ts:1100-1253` — `SELECT … FOR UPDATE` or pre-claim transition | -| 5.2 | **Zero BullMQ `jobId` usage repo-wide** — every queue.add is unkeyed, any double-fire creates a duplicate job | every `queue.add` site | -| 5.3 | `advanceStageIfBehind` reads stage outside any lock — parallel DOCUMENT_SIGNED + DOCUMENT_COMPLETED double-run berth rules | wrap in tx | -| 5.4 | `moveFolder` cycle check outside a tx — two concurrent moves can create A↔B cycles | wrap in tx | -| 5.5 | Berth-PDF upload writes blob *before* acquiring advisory lock — orphans on tx-rollback | reorder | -| 5.6 | `user_email_changes` has no partial unique index on pending rows — spam-email vector | add partial unique | +| # | What | Where | +| --- | -------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------- | +| 5.1 | `handleDocumentCompleted` idempotency gate is TOCTTOU under webhook+poll race — duplicate files rows + orphan blob | `documents.service.ts:1100-1253` — `SELECT … FOR UPDATE` or pre-claim transition | +| 5.2 | **Zero BullMQ `jobId` usage repo-wide** — every queue.add is unkeyed, any double-fire creates a duplicate job | every `queue.add` site | +| 5.3 | `advanceStageIfBehind` reads stage outside any lock — parallel DOCUMENT_SIGNED + DOCUMENT_COMPLETED double-run berth rules | wrap in tx | +| 5.4 | `moveFolder` cycle check outside a tx — two concurrent moves can create A↔B cycles | wrap in tx | +| 5.5 | Berth-PDF upload writes blob _before_ acquiring advisory lock — orphans on tx-rollback | reorder | +| 5.6 | `user_email_changes` has no partial unique index on pending rows — spam-email vector | add partial unique | --- ## Tier 6 — Perf / scale (silent today, painful at 10× traffic) -| # | What | Where | -|---|---|---| -| 6.1 | Documents tab opens with ~50 sequential queries via fetchWorkflowGroupRows | `documents.service.ts` | -| 6.2 | Recharts statically imported in `widget-registry.tsx` — every dashboard chart in initial bundle (~80-150KB) | lazy import | -| 6.3 | `DataTable` rebuilds `allColumns` every render (no useMemo) — resets TanStack internal state | memo | -| 6.4 | `tiptap-to-pdfme.ts` (571 lines) ships to client just to re-export TEMPLATE_VARIABLES | split | -| 6.5 | `listUsers` runs 2 sequential queries with no pagination, returns all super-admins globally | paginate | -| 6.6 | `command-search` invalidates 2 queries every dropdown open — defeats its own 30s staleTime | drop invalidates | +| # | What | Where | +| --- | ----------------------------------------------------------------------------------------------------------- | ---------------------- | +| 6.1 | Documents tab opens with ~50 sequential queries via fetchWorkflowGroupRows | `documents.service.ts` | +| 6.2 | Recharts statically imported in `widget-registry.tsx` — every dashboard chart in initial bundle (~80-150KB) | lazy import | +| 6.3 | `DataTable` rebuilds `allColumns` every render (no useMemo) — resets TanStack internal state | memo | +| 6.4 | `tiptap-to-pdfme.ts` (571 lines) ships to client just to re-export TEMPLATE_VARIABLES | split | +| 6.5 | `listUsers` runs 2 sequential queries with no pagination, returns all super-admins globally | paginate | +| 6.6 | `command-search` invalidates 2 queries every dropdown open — defeats its own 30s staleTime | drop invalidates | --- ## Tier 7 — Build / deploy hardening -| # | What | Where | -|---|---|---| -| 7.1 | No `.dockerignore` → 7.6 GB build context, secrets/.env leak risk via `COPY . .` | add | +| # | What | Where | +| --- | --------------------------------------------------------------------------------------------------------------- | -------------- | +| 7.1 | No `.dockerignore` → 7.6 GB build context, secrets/.env leak risk via `COPY . .` | add | | 7.2 | `socket.io` + `@socket.io/redis-adapter` not in `serverExternalPackages`; runner stage installs no runtime deps | next.config.ts | -| 7.3 | Prod CSP keeps `'unsafe-inline'` on script-src | tighten | -| 7.4 | `Dockerfile.dev` runs as root | non-root user | -| 7.5 | Compose has no memory/CPU/log-rotation limits | add | -| 7.6 | `@types/node@^25` against Node-20 runtime — type checker greenlights APIs that don't exist | pin to ^20 | -| 7.7 | `node:20-alpine` base image at/past EOL | bump to 22 | +| 7.3 | Prod CSP keeps `'unsafe-inline'` on script-src | tighten | +| 7.4 | `Dockerfile.dev` runs as root | non-root user | +| 7.5 | Compose has no memory/CPU/log-rotation limits | add | +| 7.6 | `@types/node@^25` against Node-20 runtime — type checker greenlights APIs that don't exist | pin to ^20 | +| 7.7 | `node:20-alpine` base image at/past EOL | bump to 22 | --- @@ -143,10 +143,11 @@ Already on `feat/documents-folders`: ## Tier 9 — Nice-to-haves + AI opportunities (not blocking) Forward-looking (improvements-auditor): + - **AI-where-it-actually-helps:** semantic search across notes + email threads, auto-summarise client history on detail-page open, anomaly detection on expenses paired with existing OCR. - **What NOT to AI-ify:** legal docs, EOI/contract field merges, money flow, regulatory text. - **Subtle UX wins:** keyboard shortcuts (j/k list nav, e to edit), smarter defaults (last-used port/currency/source), undo for accidental archives, "what changed since I last looked" digest. --- -*Pick a tier and we open it.* +_Pick a tier and we open it._ diff --git a/src/app/api/auth/resolve-identifier/route.ts b/src/app/api/auth/resolve-identifier/route.ts index 0a0e9918..3092bdcf 100644 --- a/src/app/api/auth/resolve-identifier/route.ts +++ b/src/app/api/auth/resolve-identifier/route.ts @@ -53,10 +53,7 @@ export async function POST(req: NextRequest) { const ip = clientIp(req); const rl = await checkRateLimit(ip, rateLimiters.auth); if (!rl.allowed) { - return NextResponse.json( - { email: '' }, - { status: 429, headers: rateLimitHeaders(rl) }, - ); + return NextResponse.json({ email: '' }, { status: 429, headers: rateLimitHeaders(rl) }); } const body = (await req.json().catch(() => ({}))) as { identifier?: string }; diff --git a/src/app/api/v1/admin/users/[id]/permission-overrides/route.ts b/src/app/api/v1/admin/users/[id]/permission-overrides/route.ts index 48686a70..93cc60bc 100644 --- a/src/app/api/v1/admin/users/[id]/permission-overrides/route.ts +++ b/src/app/api/v1/admin/users/[id]/permission-overrides/route.ts @@ -106,10 +106,7 @@ export const GET = withAuth( let baseline: RolePermissions | null = null; if (!profile.isSuperAdmin) { const portRole = await db.query.userPortRoles.findFirst({ - where: and( - eq(userPortRoles.userId, targetUserId), - eq(userPortRoles.portId, portId), - ), + where: and(eq(userPortRoles.userId, targetUserId), eq(userPortRoles.portId, portId)), }); if (portRole) { const role = await db.query.roles.findFirst({ @@ -171,10 +168,7 @@ export const PUT = withAuth( // never apply, but it still consumes a unique slot and confuses // future audits. const targetPortRole = await db.query.userPortRoles.findFirst({ - where: and( - eq(userPortRoles.userId, targetUserId), - eq(userPortRoles.portId, portId), - ), + where: and(eq(userPortRoles.userId, targetUserId), eq(userPortRoles.portId, portId)), }); if (!targetPortRole) { throw new NotFoundError('User not assigned to this port'); diff --git a/src/app/api/v1/me/route.ts b/src/app/api/v1/me/route.ts index aad056e9..e1994755 100644 --- a/src/app/api/v1/me/route.ts +++ b/src/app/api/v1/me/route.ts @@ -21,12 +21,7 @@ const updateProfileSchema = z.object({ * Uniqueness is checked below before the UPDATE — collisions surface * as a 409 with a friendly message. */ - username: z - .union([ - z.string().transform((s) => s.trim().toLowerCase()), - z.null(), - ]) - .optional(), + username: z.union([z.string().transform((s) => s.trim().toLowerCase()), z.null()]).optional(), phone: z.string().nullable().optional(), // Refuse `javascript:` / `data:` schemes — z.string().url() lets them // through and `` would otherwise be a stored-XSS diff --git a/src/components/admin/users/user-form.tsx b/src/components/admin/users/user-form.tsx index f5674a60..ed91a587 100644 --- a/src/components/admin/users/user-form.tsx +++ b/src/components/admin/users/user-form.tsx @@ -212,150 +212,150 @@ export function UserForm({ open, onOpenChange, user, onSuccess }: UserFormProps) -
-
-
- - setFirstName(e.target.value)} - placeholder="Jane" - required - /> -
-
- - setLastName(e.target.value)} - placeholder="Doe" - required - /> -
-
- -
- - setDisplayName(e.target.value)} - placeholder={fullName || 'Jane Doe'} - required - /> -

- How this user appears across the app — usually their full name, but they can pick a - nickname. -

-
- -
- - setEmail(e.target.value)} - placeholder="user@example.com" - required - /> - {isEdit && email.trim().toLowerCase() !== originalEmail.toLowerCase() ? ( -

- You'll be asked to confirm — the original address will receive an automated - notice that you, the admin, changed their sign-in email. -

- ) : isEdit ? ( -

- Changing this address is an admin-only override; the user will be notified at the - old address. -

- ) : null} -
- - {!isEdit && ( -
- - setPassword(e.target.value)} - placeholder="Min 12 characters" - minLength={12} - required - /> -
- )} - -
- - -
- -
- - -
- -
-
- -

- Grant this user access to residential clients and interests in addition to their - primary role. -

-
- -
- - {isEdit && ( -
-
- -

Disabled users cannot sign in.

+ +
+
+ + setFirstName(e.target.value)} + placeholder="Jane" + required + /> +
+
+ + setLastName(e.target.value)} + placeholder="Doe" + required + /> +
- -
- )} - {error &&

{error}

} +
+ + setDisplayName(e.target.value)} + placeholder={fullName || 'Jane Doe'} + required + /> +

+ How this user appears across the app — usually their full name, but they can pick + a nickname. +

+
- - - - - +
+ + setEmail(e.target.value)} + placeholder="user@example.com" + required + /> + {isEdit && email.trim().toLowerCase() !== originalEmail.toLowerCase() ? ( +

+ You'll be asked to confirm — the original address will receive an automated + notice that you, the admin, changed their sign-in email. +

+ ) : isEdit ? ( +

+ Changing this address is an admin-only override; the user will be notified at + the old address. +

+ ) : null} +
+ + {!isEdit && ( +
+ + setPassword(e.target.value)} + placeholder="Min 12 characters" + minLength={12} + required + /> +
+ )} + +
+ + +
+ +
+ + +
+ +
+
+ +

+ Grant this user access to residential clients and interests in addition to their + primary role. +

+
+ +
+ + {isEdit && ( +
+
+ +

Disabled users cannot sign in.

+
+ +
+ )} + + {error &&

{error}

} + + + + + + diff --git a/src/components/admin/users/user-permission-matrix.tsx b/src/components/admin/users/user-permission-matrix.tsx index 5527471f..118145a4 100644 --- a/src/components/admin/users/user-permission-matrix.tsx +++ b/src/components/admin/users/user-permission-matrix.tsx @@ -2,7 +2,12 @@ import { useEffect, useState } from 'react'; -import { Accordion, AccordionContent, AccordionItem, AccordionTrigger } from '@/components/ui/accordion'; +import { + Accordion, + AccordionContent, + AccordionItem, + AccordionTrigger, +} from '@/components/ui/accordion'; import { Button } from '@/components/ui/button'; import { Label } from '@/components/ui/label'; import { ScrollArea } from '@/components/ui/scroll-area'; diff --git a/src/components/search/command-search.tsx b/src/components/search/command-search.tsx index 4a5854f7..4688c0aa 100644 --- a/src/components/search/command-search.tsx +++ b/src/components/search/command-search.tsx @@ -961,8 +961,7 @@ export function buildFlatRows(args: BuildFlatRowsArgs): FlatRow[] { bucket: 'residentialInterests', icon: TrendingUp, label: i.clientName, - sub: - STAGE_LABELS[i.pipelineStage as PipelineStage] ?? i.pipelineStage.replace(/_/g, ' '), + sub: STAGE_LABELS[i.pipelineStage as PipelineStage] ?? i.pipelineStage.replace(/_/g, ' '), href: `/${portSlug}/residential/interests/${i.id}`, }); } diff --git a/src/components/settings/user-settings.tsx b/src/components/settings/user-settings.tsx index 77ff7f0c..ffbe7e15 100644 --- a/src/components/settings/user-settings.tsx +++ b/src/components/settings/user-settings.tsx @@ -165,7 +165,9 @@ export function UserSettings() { setOriginalUsername(next ?? ''); setUsername(next ?? ''); setUsernameMsg( - next ? `Username updated. You can now sign in with @${next} or your email.` : 'Username cleared.', + next + ? `Username updated. You can now sign in with @${next} or your email.` + : 'Username cleared.', ); } catch (err: unknown) { setUsernameMsg(err instanceof Error ? err.message : 'Failed to save username'); @@ -377,11 +379,13 @@ export function UserSettings() { > {saving === 'username' ? 'Saving…' : 'Save username'} - {usernameMsg && {usernameMsg}} + {usernameMsg && ( + {usernameMsg} + )}

- Optional alias you can use to sign in instead of your email. 2–30 lowercase - letters, digits, dot, underscore, or hyphen. + Optional alias you can use to sign in instead of your email. 2–30 lowercase letters, + digits, dot, underscore, or hyphen.

diff --git a/src/lib/audit.ts b/src/lib/audit.ts index 591437f5..2a5b0b88 100644 --- a/src/lib/audit.ts +++ b/src/lib/audit.ts @@ -143,7 +143,10 @@ export async function createAuditLog(params: AuditLogParams): Promise { fieldChanged: params.fieldChanged ?? null, oldValue: maskSensitiveFields(params.oldValue) ?? null, newValue: maskSensitiveFields(params.newValue) ?? null, - metadata: params.metadata ?? null, + // Mask metadata too — the audit found portal-auth, crm-invite, + // hard-delete, and email-accounts services were writing raw emails + // into this column. + metadata: maskSensitiveFields(params.metadata) ?? null, ipAddress: params.ipAddress ?? null, userAgent: params.userAgent ?? null, severity, diff --git a/src/lib/email/index.ts b/src/lib/email/index.ts index 76603bdd..2755cbdd 100644 --- a/src/lib/email/index.ts +++ b/src/lib/email/index.ts @@ -151,10 +151,21 @@ export async function sendEmail( ...(resolvedAttachments.length > 0 ? { attachments: resolvedAttachments } : {}), }); - logger.debug( - { messageId: info.messageId, to: effectiveTo, originalTo: requestedTo, subject, portId }, - env.EMAIL_REDIRECT_TO ? 'Email sent (redirected)' : 'Email sent', - ); + // When EMAIL_REDIRECT_TO is set we elevate to `warn` so the dev-only + // safety net is visible in any logger config. Prod boot already refuses + // when both are set (see env.ts superRefine) — this catches the dev / + // staging window where someone left it in a .env by mistake. + if (env.EMAIL_REDIRECT_TO) { + logger.warn( + { messageId: info.messageId, to: effectiveTo, originalTo: requestedTo, subject, portId }, + 'Email sent (REDIRECTED via EMAIL_REDIRECT_TO — recipient overridden)', + ); + } else { + logger.debug( + { messageId: info.messageId, to: effectiveTo, originalTo: requestedTo, subject, portId }, + 'Email sent', + ); + } return info; } diff --git a/src/lib/env.ts b/src/lib/env.ts index 99b0e636..6101a042 100644 --- a/src/lib/env.ts +++ b/src/lib/env.ts @@ -81,6 +81,21 @@ const envSchema = z.object({ .enum(['true', 'false']) .default('false') .transform((v) => v === 'true'), +}).superRefine((env, ctx) => { + // CRITICAL safety net: EMAIL_REDIRECT_TO is a dev/test feature that + // silently rewrites every outbound recipient. Leaving it set in prod + // funnels every customer email (invites, EOIs, portal magic links, + // contracts) to a single inbox. The audit caught this had only a + // `logger.debug` line as forensic trail. Refuse boot when both are + // simultaneously set in production. + if (env.NODE_ENV === 'production' && env.EMAIL_REDIRECT_TO) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ['EMAIL_REDIRECT_TO'], + message: + 'EMAIL_REDIRECT_TO must NOT be set in production — it silently rewrites every outbound email recipient. Unset it before deploying.', + }); + } }); export type Env = z.infer; diff --git a/src/lib/storage/migrate.ts b/src/lib/storage/migrate.ts index 2c0564ed..40c4d640 100644 --- a/src/lib/storage/migrate.ts +++ b/src/lib/storage/migrate.ts @@ -57,6 +57,10 @@ export const TABLES_WITH_STORAGE_KEYS: StorageKeyTable[] = [ { table: 'berth_pdf_versions', keyColumn: 'storage_key', pkColumn: 'id' }, { table: 'brochure_versions', keyColumn: 'storage_key', pkColumn: 'id' }, { table: 'gdpr_exports', keyColumn: 'storage_key', pkColumn: 'id' }, + // Last-resort recovery: pg_dump artefacts from the BackupService. The + // audit caught these were missing — flipping the storage backend used + // to silently orphan every backup, dark-blacking the recovery path. + { table: 'backup_jobs', keyColumn: 'storage_path', pkColumn: 'id' }, ]; const ADVISORY_LOCK_KEY = 0xc7000a01;