687a1f1c2f
Working through the audit-v2 deferred backlog. Each round was tested
(typecheck + 1168/1168 vitest) before moving on.
Round 1 — DB performance + AI cost visibility:
- Add missing FK indexes Postgres doesn't auto-create on
berth_reservations.{interest_id, contract_file_id},
documents.{file_id, signed_file_id}, document_events.signer_id,
document_templates.source_file_id, form_submissions.{form_template_id,
client_id}, document_sends.{brochure_id, brochure_version_id,
sent_by_user_id}. Without these, RESTRICT-checks on parent delete +
reverse-lookups walk the child tables fully. Migration 0037.
- AI worker now writes one ai_usage_ledger row per OpenAI call so admins
can audit spend per port/user/feature and future per-port budgets have
history to read from. Failure to write is logged-not-thrown so the
user-facing email draft is unaffected.
Round 2 — Boot-time + transport hardening:
- S3 backend verifies the bucket exists at startup (or auto-creates
when MINIO_AUTO_CREATE_BUCKET=true). A typo'd bucket name now
surfaces with a clear boot error instead of a vague Minio error
inside the first user-facing request.
- Documenso v1 placeFields: 3-attempt exponential-backoff retry on 5xx
+ network errors, fail-fast on 4xx. Stops one transient flake from
leaving a document with a partial field set.
- FilesystemBackend logs a structured warn-once at boot when the dev
HMAC fallback is in effect, so two processes started with different
BETTER_AUTH_SECRET values are observable (random 401s on file
downloads otherwise).
- Logger redact paths extended to cover *.headers.{authorization,
cookie}, *.config.headers.authorization, encrypted-credential blobs
(secretKeyEncrypted, smtpPassEncrypted, etc.), the Documenso
X-Documenso-Secret header, and 2-level nested forms.
Round 3 — UI feedback + permission gates:
- Storage admin migrate dialog: success toast with row count + error
toast on both dryRun and migrate mutations.
- Invoice detail Send + Record-payment buttons wrapped in
PermissionGate (invoices.send / invoices.record_payment); both
mutations now toast on success/error.
- Admin user list Edit button wrapped in PermissionGate(admin.manage_users).
- Scan-receipt page surfaces an amber warning when OCR fails so reps
know they can fill the form manually instead of staring at a stalled
spinner; the editable form now also opens on scanMutation.isError
/ uploadedFile, not only on success.
- Email threads list now renders skeleton rows during load + shared
EmptyState for the empty case (was a single "Loading…" line).
Round 4 — Service / route correctness:
- documentSends.sent_by_user_id was a free-text NOT NULL column with no
FK. Now nullable + FK to user(id) ON DELETE SET NULL so the audit row
survives a user being hard-deleted. Migration 0038 with a defensive
null-out for any orphan ids before attaching the constraint.
- Saved-views route: documented why withAuth alone is correct (the
service strictly filters by (portId, userId) — owner-only by design).
- Public-interests audit log: replaced "userId: null as unknown as
string" cast with userId: null; AuditLogParams already accepts null
for system-generated events.
- EOI in-app PDF fill: extracted setBerthRange() that, when the
AcroForm field is missing AND the context has a non-empty range
string, logs a structured warn so the deployment gap (live Documenso
template needs the field) is observable instead of silently dropping
the multi-berth range.
Test status: 1168/1168 vitest. tsc clean. Two new migrations
(0037/0038) need pnpm db:push (or migration apply) on the dev DB.
Deferred-doc updated with the remaining open items (bigger refactors).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
279 lines
14 KiB
Markdown
279 lines
14 KiB
Markdown
# 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.
|