85 lines
4.4 KiB
Markdown
85 lines
4.4 KiB
Markdown
|
# Final audit deferred findings
|
||
|
|
|
||
|
|
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`.
|