pn-new-crm/docs/audit-final-deferred.md

# Final audit deferred findings

> **Status update (audit-v3 round)**: most of the v2 deferred items have
> now landed. Items struck through below are completed. The remaining
> open items are bigger refactors (custom-fields per-entity routes,
> systemSettings PK reconciliation, Documenso v2 voidDocument verification,
> partial-vs-composite archived index conversion, storage-proxy port_id
> claim, Documenso webhook port_id enforcement, response-shape
> standardization, berths.current_pdf_version_id Drizzle FK).

The pre-merge audit on `feat/berth-recommender` produced ~30 findings. The
critical + high-severity items were fixed in-branch. The items below are
medium / low severity and deferred to follow-up issues so the merge isn't
held up. Each entry is self-contained — pick one off and ship it.

## Cross-cutting integration

- **EOI in-app pathway silently swallows missing `Berth Range` AcroForm field**
  — `src/lib/pdf/fill-eoi-form.ts:93`. `setText(form, 'Berth Range', ...)`
  is wrapped in a try/catch that succeeds silently when the field is
  absent. CLAUDE.md already warns ops about needing to add the field to
  the live Documenso template; this code change would make the deployment
  gap observable. Fix: when `context.eoiBerthRange` is non-empty AND the
  field is absent, log at warn level + surface a structured response field.

- **Email body merge expansion happens after token validation** —
  `src/lib/services/document-sends.service.ts:399-403`. If a merge value
  contains a `{{token}}` substring (e.g. a client name like
  `"Acme {{discount}} Inc."`), the expanded body will contain a token
  the unresolved-check missed and ships with literal braces. Fix: HTML-
  escape merge values before expansion, OR run a second
  `findUnresolvedTokens` against the expanded body.

- **Filesystem dev-fallback HMAC secret can drift across processes** —
  `src/lib/storage/filesystem.ts:328-331`. The dev-only fallback derives
  the HMAC secret from `BETTER_AUTH_SECRET`. Two CRM processes running
  with different secrets (web vs worker) reject each other's tokens.
  Fix: assert `BETTER_AUTH_SECRET` is set when filesystem backend is
  active in non-prod, or document the requirement loudly.

- **Berth PDF apply path: numeric column nulling silently drops** —
  `src/lib/services/berth-pdf.service.ts:473-475`. When
  `Number.isFinite(n)` is false the apply loop `continue`s without
  pushing to `applied` and without warning. Combined with the
  "no appliable fields supplied" check (only fires when ALL drop), partial
  silent drops are invisible. Fix: collect dropped keys and surface them.

## Multi-tenant isolation hardening

- **document_sends row stores `interestId` without verifying port match** —
  `src/lib/services/document-sends.service.ts:422`. Audit-log pollution
  rather than data exposure (the recipient lookup is port-checked already).
  Fix: when `recipient.interestId` is set, fetch with
  `and(eq(interests.id, ...), eq(interests.portId, input.portId))` and
  throw if missing.

- **Storage proxy token does not bind to port_id** —
  `src/lib/storage/filesystem.ts:73-84`. ProxyTokenPayload is `{k, e, n,
f?, c?}` with a global HMAC. The current "issuer always checks port
  first" relies on every issuer being correct in perpetuity. Fix: add a
  `p` (portId) claim and have the proxy route resolve key→owner row +
  assert `owner.portId === payload.p` before streaming.

- **Documenso webhook does not enforce port_id on document lookups** —
  `src/app/api/webhooks/documenso/route.ts:96-148`. Handlers dispatch by
  global `documensoId`. If two ports' documents were ever issued the
  same Documenso ID (replay across staging/prod, forwarded webhook from
  a foreign instance), the wrong port's interest could be mutated. The
  per-body `signatureHash` dedup is partial mitigation. Fix: either
  (a) include the originating Documenso instance/team in the lookup, or
  (b) verify `documents(documenso_id)` has a unique index port-wide.

## Recent expense work polish

- **renderReceiptHeader cursor math drifts after multi-step writes** —
  `src/lib/services/expense-pdf.service.ts:854`. After
  `doc.text(...)` with auto-flow, `doc.y` advances. Using `doc.y -
headerH + 10` after the rect+stroke block computes against the
  post-rect position; works only because pdfkit's text-after-rect
  hasn't moved y yet. Headers may misalign on the first receipt page
  after a soft page break. Fix: capture `const baseY = doc.y` before
  drawing the rect and compute all subsequent offsets relative to it.

## Settings parsing

- **`loadRecommenderSettings` rejects string-shaped JSONB booleans** —
  `src/lib/services/berth-recommender.service.ts:116`. Postgres returns
  JSONB `true/false` as JS booleans, but if an admin saves `"true"`
  via a UI that wraps the value as a string, `asBool` returns null and
  the per-port override silently falls through to defaults. Not a
  security bug; a tuning footgun. Fix: accept `"true"`/`"false"` string
  forms in `asBool`.

# Audit-final v2 (post-merge platform-wide pass) deferred findings

A second comprehensive audit (security, routes, DB, integrations, UI/UX)
ran after the merge. The high-impact items landed in commit
`fix(audit-final-v2): platform-wide hardening` (or similar). Items below
are deferred follow-ups.

## Routes / API

- **Saved-views routes lack `withPermission`** —
  `src/app/api/v1/saved-views/[id]/route.ts:4-5` and
  `src/app/api/v1/saved-views/route.ts:24`. Convention is
  `withAuth(withPermission(...))`. Verify the service applies
  `(ctx.userId, ctx.portId)` ownership filtering, then add either an
  explicit owner-only comment or wrap with a benign permission gate.

- **Custom-fields permission resource hardcoded to `clients`** —
  `src/app/api/v1/custom-fields/[entityId]/route.ts:15,29`. Custom fields
  attach to client / yacht / interest / berth / company, but the route
  always checks `clients.view` / `clients.edit`. A user with
  `companies.view` can read confidential company custom-field values via
  this endpoint (the service-level `customFieldDefinitions.portId` filter
  prevents cross-tenant access but not cross-resource within a tenant).
  Fix: split into per-entity routes, OR resolve `entityType` and gate on
  the matching permission inline.

- **`alerts/[id]/acknowledge|dismiss` ungated** —
  `src/app/api/v1/alerts/[id]/acknowledge/route.ts:6` etc. only `withAuth`,
  no `withPermission`. Verify the service requires user ownership; if
  not, gate on `reports.view_dashboard` or similar.

- **Public POST routes bypass service layer** —
  `src/app/api/public/interests/route.ts`, `…/website-inquiries/route.ts`,
  `…/residential-inquiries/route.ts`. These do extensive `tx.insert(...)`
  with hand-rolled audit logs (`userId: null as unknown as string`).
  Extract a `publicInterestService.create(...)` so the same code path is
  unit-testable and port-id discipline is uniform. Verify
  `audit_logs.user_id` is nullable (the cast pattern signals it is, but
  enforce in schema if not).

- **Inconsistent response shapes** — most endpoints return `{ data: ... }`,
  but `notifications/[notificationId]` returns `{ success: true }`,
  `website-inquiries` returns `{ id, deduped }`. Document a convention in
  CLAUDE.md and migrate.

- **`req.json()` without `parseBody` helper** — admin custom-fields
  routes use `await req.json(); schema.parse(body)` directly instead of
  the project's `parseBody(req, schema)` helper. Migrate for uniform
  400 error shapes.

## Documenso integration

- **v2 voidDocument endpoint may not match real API** —
  `src/lib/services/documenso-client.ts:450-466`. The audit flagged that
  Documenso 2.x exposes envelope deletion as
  `POST /api/v2/envelope/delete` with `{ envelopeId }` body, not
  `DELETE /api/v2/envelope/{id}`. The unit test mocks fetch so it can't
  catch the real shape. Verify against a live Documenso 2.x instance
  (`pnpm exec playwright test --project=realapi`) before flipping any
  port to v2.

- **Webhook dedup vs per-recipient signed events** —
  `src/app/api/webhooks/documenso/route.ts:103-110`. The top-level
  `signatureHash` (sha256 of raw body) blocks exact replays, but a
  duplicate webhook delivery for a multi-recipient document with a
  re-encoded body will go through the per-recipient loop. Make
  `documentEvents.signatureHash` unique cover the suffixed values OR add
  a composite unique index `(documensoDocumentId, recipientEmail, eventType)`.

- **v1 `placeFields` per-field POST has no retry** —
  `src/lib/services/documenso-client.ts:374-398`. A single transient 500
  mid-loop leaves the document with a partial field set. Add 3-attempt
  exponential backoff on 5xx + voidDocument on final failure.

## Storage

- **S3 backend has no startup bucket-exists check** —
  `src/lib/storage/s3.ts:100-111`. A typo'd bucket name surfaces as a
  500 inside a user-facing request rather than at boot. Add
  `await client.bucketExists(bucket)` in `S3Backend.create` with a clear
  error message.

- **Storage cache fingerprint includes encrypted secret** —
  `src/lib/storage/index.ts:158-159`. After a key rotation the old
  cached client survives until `resetStorageBackendCache()` is called
  (already called via the settings-write hook). Document the
  invariant or fingerprint on a content-hash that excludes encrypted
  material.

- **Filesystem dev HMAC silent fallback** —
  `src/lib/storage/filesystem.ts:309-332`. Two dev nodes started with
  different `BETTER_AUTH_SECRET` derive different secrets and reject
  each other's tokens. Log a one-line warn at backend boot in non-prod.

## DB schema

- **`berths.current_pdf_version_id` lacks Drizzle FK** —
  `src/lib/db/schema/berths.ts:83`. The FK exists in migration 0030
  but not in the schema source-of-truth, so `pnpm db:push` against an
  empty DB skips the constraint. Either add the FK with a deferred
  declaration or document that `db:push` is unsupported.

- **Missing indexes on FK columns** — `berthReservations.interestId`,
  `berthReservations.contractFileId`, `documents.fileId`,
  `documents.signedFileId`, `documentEvents.signerId`,
  `documentTemplates.sourceFileId`, `formSubmissions.formTemplateId`,
  `formSubmissions.clientId`, `documentSends.brochureId`,
  `documentSends.brochureVersionId`, `documentSends.sentByUserId`. Add
  `index(...)` declarations to avoid full-scan FK checks on parent
  delete.

- **`systemSettings` PK / unique-index drift** —
  `src/lib/db/schema/system.ts:119-133`. Schema declares only a
  `uniqueIndex` on `(key, port_id)` but the migration uses `key` as PK.
  `port_id` is nullable so `(key, port_id)` cannot serve as a PK with
  default NULLs-not-equal semantics. Reconcile: declare
  `primaryKey({ columns: [table.key, table.portId] })` (after making
  `portId` non-null with a sentinel) OR use partial unique indexes for
  global + per-port settings.

- **Composite vs partial archived indexes** — many tables use
  `index('idx_*_archived').on(portId, archivedAt)` when the dominant
  query is `WHERE port_id = ? AND archived_at IS NULL`. Convert to
  `index(...).on(portId).where(sql\`archived_at IS NULL\`)` partial
  indexes for smaller storage + faster planner choice.

- **`documentSends.sentByUserId` ungated FK** —
  `src/lib/db/schema/brochures.ts:118` is `notNull()` but has no FK
  reference. If a user is hard-deleted (rare; we soft-delete), an
  orphan id remains. Add `.references(() => users.id, { onDelete: 'set null' })`
  and make the column nullable. Same audit-trail rationale as the
  other documentSends FK fixes (commit 0035).

## UI/UX

- **Storage admin migration mutation lacks toasts** —
  `src/components/admin/storage-admin-panel.tsx:61-72`. Add `onSuccess`
  toast with row count + `onError` toast.

- **Invoice detail send/payment mutations lack error feedback + gates** —
  `src/components/invoices/invoice-detail.tsx:93-99,152-167`. Add
  `onError: (e) => toast.error(...)` and wrap mutating buttons in
  `<PermissionGate resource="invoices" action="send">` /
  `record_payment`.

- **Admin user list edit button ungated** —
  `src/components/admin/users/user-list.tsx:114`. Wrap in
  `<PermissionGate resource="admin" action="manage_users">`.

- **Email threads list missing skeleton** —
  `src/components/email/email-threads-list.tsx:29-45`. Use `<Skeleton>`
  rows during load + `<EmptyState>` for the empty case.

- **Scan page mutations swallow OCR errors** —
  `src/app/(dashboard)/[portSlug]/expenses/scan/page.tsx:67-87`. Add an
  inline error state for `scanMutation.isError` (the upload-side
  already does this).

- **Invoice detail uses `any` for query data** — strict-mode escape
  hatch. Define a proper response type matching the API contract.

## Security defense-in-depth

- **Storage proxy token does not bind to port_id** —
  `src/lib/storage/filesystem.ts:73-84`. Token's HMAC is global. Fix:
  add `p` (portId) claim and have the proxy resolve key→owner row +
  assert `owner.portId === payload.p`.

- **Documenso webhook does not enforce port_id** —
  `src/app/api/webhooks/documenso/route.ts:96-148`. Handlers dispatch
  by global `documensoId`. Verify `documents(documenso_id)` is unique
  port-wide OR include the originating instance/team in the lookup.

- **EOI in-app pathway silently swallows missing `Berth Range` field** —
  `src/lib/pdf/fill-eoi-form.ts:93`. Log warn when
  `context.eoiBerthRange` is non-empty AND the field is absent so the
  Documenso template deployment gap is observable.

- **AI worker has no cost-tracking ledger write** —
  `src/lib/queue/workers/ai.ts:122-177`. Persist token usage to the
  `ai_usage` ledger after every call.

- **Logger redact paths miss nested credentials** —
  `src/lib/logger.ts:5-19`. Extend redact list to cover
  `*.headers.authorization`, `**.token`, `secretKeyEncrypted`, etc.