fix(audit-v2): platform-wide post-merge hardening across 5 domains
Five-domain audit (security, routes, DB, integrations, UI/UX) ran after
the cf37d09 merge. Critical + high-impact items landed here; deferred
medium/low items indexed in docs/audit-final-deferred.md (now organised
into a "Audit-final v2" section).
Security:
- Storage proxy tokens now bind to op (`'get'` vs `'put'`). A long-lived
download URL minted by `presignDownload` for an emailed brochure can no
longer be replayed against the proxy PUT to overwrite the original
storage object. `verifyProxyToken` requires `expectedOp` and rejects
mismatches; legacy tokens missing `op` fail-closed. Regression tests
added.
- Markdown email merge values are now markdown-escaped (`[`, `]`, `(`,
`)`, `*`, `_`, `\`, backticks, braces) before substitution into the
rep-authored body. A malicious value like `[click here](https://evil)`
stored in `client.fullName` no longer survives `escapeHtml` to render
as a real `<a href>` in the outbound email. Phishing-via-merge-field
closed; regression tests added.
- Middleware now performs an Origin/Referer check on
POST/PUT/PATCH/DELETE to `/api/v1/**`. Defense-in-depth on top of
better-auth's SameSite=Lax cookie. Webhooks/public/auth/portal routes
exempt as they don't carry the session cookie.
Routes:
- Template management routes were calling `withPermission('documents',
'manage', ...)` — but `documents` doesn't have a `manage` action. The
registry has `document_templates.manage`. Every non-superadmin was
getting 403'd on the seven template endpoints. Fixed across the
/admin/templates surface.
- Custom-fields permission resource is hardcoded to `clients` regardless
of which entity (yacht/company/etc.) the values belong to. Documented
as deferred (requires per-entity routes).
DB:
- documentSends: every parent FK (client_id, interest_id, berth_id,
brochure_id, brochure_version_id) now uses ON DELETE SET NULL so the
audit trail outlasts hard-deletes. The denormalized columns
(recipient_email, document_kind, body_markdown, from_address) were
added precisely for this. Migration 0035.
- Polymorphic discriminators on yachts.current_owner_type and
invoices.billing_entity_type now have CHECK constraints — typos like
`'clients'` vs `'client'` were silently inserting unreachable rows
before. Migration 0036.
Integrations:
- Email attachment resolution (`src/lib/email/index.ts`) was importing
MinIO directly instead of `getStorageBackend()`. Filesystem-backend
deployments would have broken every email-with-attachment send. Now
routes through the pluggable abstraction per CLAUDE.md.
- Documenso DOCUMENT_OPENED webhook filter relaxed: v2 may omit
`readStatus` or send lowercase, so an event that was the SIGNAL of an
open was being silently dropped. Now treats any recipient on a
DOCUMENT_OPENED event as opened.
UI/UX:
- Expense detail used to render `receiptFileIds` as opaque UUID badges —
reps couldn't view the receipt they uploaded. Now renders an image
thumbnail (via `/api/v1/files/[id]/preview`) plus a Download link for
PDFs. Closed the "where's my receipt?" loop in the expense flow.
- Expense detail Edit + Archive buttons now `<PermissionGate>` and the
archive mutation surfaces success/error toasts instead of silent 403s.
- Brochures admin: setDefault/archive/create mutations now have onError
toasts (only onSuccess existed before).
- Removed broken bulk-upload link in scan/page (route doesn't exist;
used a raw `<a>` triggering a full reload to a 404).
Test status: 1168/1168 vitest passing. tsc clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Matt Ciaccio
@@ -82,3 +82,189 @@ headerH + 10` after the rect+stroke block computes against the
|
||||
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.
|
||||
|
||||
Reference in New Issue
Block a user