letsbe/pn-new-crm
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity
Files
f0dbefcac2e4bb6adbaf628e8dcc33b6ec4230d8
pn-new-crm/docs/superpowers/audits/2026-05-18-full-codebase-audit.md
Matt 449b9497ab fix(uat): batch — timeline overshoot, name-sync, reset-password, dashboard cleanup, queue/seed hygiene + alpha UAT findings doc 
UAT findings landed across the last few Playwright + React Grab passes;
single grouped commit so the index doesn't fragment into 30 one-liners.

User & auth:
- `user-settings`: name now updates the avatar + topbar menu after save
  (was reading stale session).
- `me/password-reset`: 3 bugs (token validation, error response shape,
  redirect chain).
- Admin user permission-overrides route honours the same envelope as
  the rest of the admin surface.

Dashboard:
- Removed obsolete `revenue-breakdown-chart` + `dashboard-widgets-card`
  (replaced by the customisable widget grid).
- Strip `revenue_breakdown` from analytics route + use-analytics +
  service + integration test so nothing renders an empty card.
- Activity log timeline overshoot fix (`interest-timeline` +
  `entity-activity-feed`).
- Tightened tiles: active-deals, berth-heat-widget, pipeline-value, kpi-tile.
- `dev-mode-banner`: derive dismissed state synchronously instead of
  via an effect (set-state-in-effect lint rule).

Forms & lists (assorted polish):
- client / company / yacht / interest / reminder forms — validation +
  empty-state copy + tab transitions.
- companies/yachts list tweaks; berth recommender panel; qualification
  checklist; supplemental info request button.

Infra & misc:
- Queue workers (ai / email / notifications) — log shape +
  per-job timeout consistency.
- Auth / brochures / users schema small adjustments; seeds reflect
  permissions matrix changes.
- Scan shell + scanner manifest + AI admin page small fixes.
- `next.config.transpilePackages` adds `echarts`/`zrender`/`echarts-for-react`
  (recommended config from echarts-for-react inside Next).

Docs:
- `docs/superpowers/audits/alpha-uat-master.md` — single rolling
  cross-cutting UAT findings doc (per CLAUDE.md convention).
- `docs/BACKLOG.md`: dashboard stats cards (§I) + activity-log
  normalization (§J).
- 2026-05-18 audit log updated with this batch.
- `CLAUDE.md` — small manual UAT scaffold notes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-20 15:56:11 +02:00

336 lines
67 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# Full Codebase Audit — 2026-05-18
> **Companion doc:** [Alpha UAT Master](./alpha-uat-master.md) — the multi-day cross-cutting Playwright/React-Grab walkthrough doc, findings cross-referenced here as `→ confirmed in manual #N`.
>
> **Methodology:** Parallel sonnet[1m] audit team (16 narrow-scope agents), each assigned a specific subsystem with no overlap. Every finding includes file:line evidence; severity is `critical | high | medium | low | info`. Findings here are raw — triage + prioritization at the bottom.
>
> **Scope:** entire `src/` tree at commit `b3f8756` (post-audit-cleanup). Excludes `docs/`, `tests/` (covered by F3), build/Docker config, and node_modules.
>
> **Out of scope:** anything in `docs/BACKLOG.md` already triaged. This audit looks for NEW findings not on that list.
---
## Audit team composition
| Agent | Scope |
| ------------------------------- | ---------------------------------------------------------------------------------------- |
| **A1 — Schema: people/orgs** | `src/lib/db/schema/{clients,yachts,companies,users}.ts` |
| **A2 — Schema: pipeline** | `src/lib/db/schema/{interests,berths,reservations}.ts` |
| **A3 — Schema: docs+infra** | `src/lib/db/schema/{documents,email,brochures,system}.ts` |
| **B1 — Public API** | `src/app/api/public/*` |
| **B2 — Admin API** | `src/app/api/v1/admin/*` |
| **B3 — v1 entity CRUD** | `src/app/api/v1/{clients,interests,yachts,companies,berths}/*` |
| **B4 — Webhooks/auth/storage** | `src/app/api/{webhooks,auth,storage}/*` |
| **C1 — EOI/Documenso services** | `src/lib/services/{eoi-*,document-templates,custom-document-upload,documenso-client}.ts` |
| **C2 — Domain services** | `src/lib/services/{berth-*,reminders,notifications,inquiry-notifications}.ts` |
| **C3 — Observability/audit** | `src/lib/services/error-events.service.ts`, `src/lib/audit.ts`, `src/lib/storage/*` |
| **D1 — Jobs/queues** | `src/lib/queue/scheduler.ts`, `src/lib/queue/workers/*`, `src/jobs/processors/*` |
| **E1 — Admin UI** | `src/app/(dashboard)/[portSlug]/admin/*` |
| **E2 — Entity UI** | `src/components/{interests,clients,yachts,companies,berths}/*` |
| **F1 — Security cross-cut** | Auth/permission gaps, XSS/SQLi, port-isolation, secret leaks |
| **F2 — Performance** | Missing indexes, N+1 queries, unbounded fan-outs, hot paths |
| **F3 — Tests + deps** | Coverage gaps, package.json freshness, Docker/CI |
---
## Findings by agent
### A2 — Schema: pipeline (15 findings: 3 high, 4 medium, 7 low, 1 info)
| # | Severity | Title | Evidence |
| --- | -------- | ------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------- |
| 1 | high | No DB-level CHECK on `interests.pipeline_stage` | `interests.ts:44` — text col, no CHECK; legacy 'completed' / 'eoi_signed' can persist via raw SQL |
| 2 | high | No DB-level CHECK on `outcome`, `eoi_doc_status`, `reservation_doc_status`, `contract_doc_status` | `interests.ts:47-49,84` — bare text on all 4 enum-shaped columns |
| 3 | high | No DB-level CHECK on `berths.status` | `berths.ts:31` — `derivePublicStatus()` silently falls through to 'Available' on bad values |
| 4 | medium | No CHECK on `berth_reservations.status` — breaks `idx_br_active` invariant | `reservations.ts:34,61-64` — misspelled 'Active' bypasses the one-active-per-berth guard |
| 5 | medium | Stale `berthId` field on `Interest` domain type | `src/types/domain.ts:39` — `interests.berth_id` was dropped in 0029; type still declares it |
| 6 | medium | Board query missing composite partial index — bitmap-AND scan on large ports | `interests.ts:113-117` — need `(portId, pipelineStage) WHERE archivedAt IS NULL AND outcome IS NULL` |
| 7 | medium | `interestTags.tagId` + `berthTags.tagId` are comment-only FKs, no DB constraint | `interests.ts:205-207`, `berths.ts:267-269` — tag deletes silently orphan junction rows |
| 8 | medium | `berthWaitingList` lacks `port_id` column — no schema-level cross-port isolation | `berths.ts:170-192` — defense-in-depth depends entirely on service layer |
| 9 | low | No index on `interest_berths.is_in_eoi_bundle` | bundle lookups scan all rows for the interestId |
| 10 | low | `berthRecommendations` lacks `port_id` — same isolation pattern as #8 | `berths.ts:146-168` |
| 11 | low | `interests.assignedTo`, `interest_berths.addedBy`/`eoiBypassedBy` are bare text — no FK to users | dead entries accumulate on user delete |
| 12 | low | `berthMaintenanceLog.portId` FK missing onDelete — implicit NO ACTION breaks H-01 convention | `berths.ts:204-206` |
| 13 | low | `berthReservations.startDate`/`endDate` use timestamptz `mode:'date'` — TZ off-by-one risk | should be `date()` |
| 14 | low | `idx_interests_stage` is not partial — bloats with archived + closed rows | add `WHERE archivedAt IS NULL AND outcome IS NULL` |
| 15 | info | `is_primary` ≤1 per interest invariant correctly enforced via partial unique index | `interests.ts:165-167` — no action needed |
### B2 — API: admin (10 findings: 2 medium, 8 low)
| # | Severity | Title | Evidence |
| --- | -------- | ------------------------------------------------------------------------------------------------ | -------------------------------------------------------------------------------------------------------------------------- |
| 1 | medium | `GET /qualification-criteria` has no `withPermission` gate | `qualification-criteria/route.ts:9` — any authenticated user can enumerate; POST correctly gates |
| 2 | medium | Triage PATCH on website-submissions uses `view_audit_log` (read) for a write | `website-submissions/[id]/triage/route.ts:26` — semantic mismatch; should be manage_settings |
| 3 | low | `/admin/storage/route.ts` POST returns bare `result` without `{data:...}` | `storage/route.ts:64` — breaks toastError frontend hook |
| 4 | low | `/admin/ocr-settings/test` POST returns bare result without `{data:...}` | `ocr-settings/test/route.ts:26` |
| 5 | low | `/admin/ocr-settings` PUT returns `{ok:true}` — legacy success-flag pattern | `ocr-settings/route.ts:64` — should be 204 or `{data: updatedConfig}` |
| 6 | low | `/admin/custom-fields/[fieldId]` PATCH uses raw `req.json()` + manual `.parse()` not `parseBody` | `custom-fields/[fieldId]/route.ts:18-19` — generic 500 instead of structured 400 |
| 7 | low | `/admin/ai-budget` PUT — `setAiBudget` audit record missing ipAddress + userAgent | `ai-budget/route.ts:40` |
| 8 | low | `/admin/ocr-settings` PUT — `saveOcrConfig` audit record missing ipAddress + userAgent | `ocr-settings/route.ts:53` — encrypted API key swap is high-impact, deserves full context |
| 9 | low | `/admin/brochures/[id]` PATCH+DELETE pass no audit meta to service helpers | `brochures/[id]/route.ts:26,37` + brochures POST — pattern mismatch with form-templates, custom-fields, document-templates |
| 10 | low | `/admin/email-templates` PUT returns `{data:{ok:true}}` — flag body instead of entity or 204 | `email-templates/route.ts:84` |
### A3 — Schema: docs+infra (15 findings: 1 high, 7 medium, 7 low)
| # | Severity | Title | Evidence |
| --- | -------- | ----------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------- |
| 1 | **high** | `documents.documenso_id` has NO INDEX | `documents.ts:88` — full table scan on every webhook delivery (hottest read path); only documenso_numeric_id is indexed |
| 2 | medium | `documentSigners.signingToken` indexed but NOT unique | `documents.ts:188,193` — token collision/replay has no DB-level guard; should be partial uniqueIndex |
| 3 | medium | `audit_logs` missing 4-column inspector index | `system.ts:62-63` — neither existing index covers `(port_id, entity_type, entity_id, ORDER BY created_at)` without heap re-filter |
| 4 | medium | `system_settings NULLS NOT DISTINCT` lives in migration 0047 only — `db:push` drops it | `system.ts:144-149` — fresh `db:push` re-introduces the duplicate-global-settings bug 0047 fixed |
| 5 | medium | `documentFolders.parentId` self-FK MISSING from Drizzle schema (only in migration 0050) | `documents.ts:357-358` — fresh `db:push` skips the self-FK; orphaned folders undetectable |
| 6 | medium | `emailMessages.attachmentFileIds` text[] with no FK — dangling IDs survive RTBF wipe | `email.ts:78` + `client-hard-delete.service.ts:269-277` — RTBF wipes body/subject but not attachment file references |
| 7 | medium | `brochureVersions` missing `unique(brochureId, versionNumber)` — unlike berth_pdf_versions | `brochures.ts:79` — concurrent uploads could assign duplicate version numbers |
| 8 | medium | `documensoNumericId` indexed non-uniquely despite being globally unique | `documents.ts:94,152` — webhook resolver matches multiple docs for same numeric ID; double-processing |
| 9 | low | `emailThreads.clientId` has no `onDelete` clause — defaults to RESTRICT, inconsistent with `set null` peers | `email.ts:50` |
| 10 | low | `files.storagePath` has no unique constraint — duplicate blob paths undetected | `documents.ts:41` — migrate-storage.ts would silently double-migrate |
| 11 | low | `brochureVersions.storageKey` + `berth_pdf_versions.storageKey` lack unique constraints | same as #10 |
| 12 | low | `documentSends.berthPdfVersionId` has no index — full-scan for version-X queries | `brochures.ts:120` |
| 13 | low | C.2 dedup gap: SIGNED events with `recipient_email=NULL` fall back to broken hash-only path | migration 0075 risk note: any v2 code path emitting global SIGNED without recipient context bypasses per-recipient dedup |
| 14 | low | C.2 dedup over-eager: void-then-reinvite with same email blocks the legitimate 2nd signing | `documents.ts:230-232` — partial unique on (docId, recipientEmail, eventType) treats reinvited signing as re-delivery |
| 15 | low | `document_sends` + `emailMessages` parallel send-audit tables with no cross-reference | future IMAP-synced sent-folder → duplicate GDPR exports |
### B1 — API: public (12 findings: 1 high, 3 medium, 5 low, 3 info)
| # | Severity | Title | Evidence |
| --- | -------- | ------------------------------------------------------------------------------------------------------------ | --------------------------------------------------------------------------------------------------------------------------------------------- |
| 1 | **high** | `portId` is caller-controlled on `/interests` — NOT validated against existing ports | `interests/route.ts:40` — caller can inject client/yacht/interest into ANY tenant they know the UUID for; residential-inquiries DOES validate |
| 2 | medium | Health endpoint `X-Intake-Secret` comparison leaks secret byte-length via timing short-circuit | `health/route.ts:57` — length check before timingSafeEqual; website-inquiries does it right |
| 3 | medium | `X-Forwarded-For` spoofable — rate-limit keys are attacker-controlled on all public POST routes | interests/residential/website-inquiries — no x-real-ip fallback; route-helpers `clientIp()` has it but isn't used |
| 4 | medium | `/public/supplemental-info/[token]` has NO rate limiting on GET or POST | `supplemental-info/[token]/route.ts` — POST writes live client PII (name, address, email, phone) at unlimited rate |
| 5 | low | Unbounded string fields in public schemas — multi-MB payloads allowed | publicInterestSchema/publicResidentialInquirySchema — no `.max()` on phone/notes/preferences; no segment bodySizeLimit |
| 6 | low | Invalid `portId` on `/interests` causes 500 (DB FK error) not 400 | residential route has the explicit pre-check; interests doesn't |
| 7 | low | `supplemental-info` POST uses raw `req.json()` + `.parse()` instead of `parseBody()` | malformed JSON returns 500 not field-level 400 |
| 8 | low | `supplemental-info` GET missing `Cache-Control: no-store` — intermediaries may cache token-keyed PII payload | response includes primaryEmail/Phone/streetAddress |
| 9 | low | Rate limiting fails open on Redis outage — silently drops public-form protection | `rate-limit.ts:57-73` — intentional for auth, equally affects public POST |
| 10 | info | `applySubmission` distinguishes consumed vs expired token in error message | violates the conflation principle the GET path uses |
| 11 | info | Authenticated health probe discloses `APP_URL` and `NODE_ENV` | `health/route.ts:86-93` — internal URL leak via authed probe |
| 12 | info | `residential-inquiries` exposes internal UUIDs and uses deprecated `{success:true}` envelope | `residential-inquiries/route.ts:123` |
### F3 — Tests + deps + infra (15 findings: 2 critical, 3 high, 4 medium, 5 low, 1 info)
| # | Severity | Title | Evidence |
| --- | ------------ | ------------------------------------------------------------------------------------------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| 1 | **CRITICAL** | `client-hard-delete.service.ts` has ZERO unit or integration tests | GDPR/CCPA-critical path just modified today; no automated regression guard |
| 2 | **CRITICAL** | No CI/CD pipeline — `.github/workflows/` does not exist | every merge can silently break tests; the full vitest+playwright suite must be run manually |
| 3 | high | `alert-engine-realtime.spec.ts` permanently skips a test whose route now exists | spec skip says route not implemented; route file present at `/admin/alerts/run-engine` |
| 4 | high | `documenso-client.ts` v1/v2 routing has no dedicated unit test | every EOI + document-send path goes through it |
| 5 | high | Coverage config excludes `src/app/` — route handlers never counted | `vitest.config.ts: coverage.include: ['src/lib/**']` — misleadingly low coverage on API surface |
| 6 | medium | Two competing image-crop libraries in production deps | `react-easy-crop` + `react-image-crop` both live; one call site each |
| 7 | medium | Six PDF-related packages; pdfkit (1 usage) and unpdf (1 usage) candidate for consolidation | `pdf-lib`, `pdfjs-dist`, `pdfkit`, `react-pdf`, `unpdf`, `@react-pdf/renderer` |
| 8 | medium | CLAUDE.md lists `pdfme` as a tech-stack dep — not in package.json | removed 2026-05-12; CLAUDE.md outdated |
| 9 | medium | `playwright.config.ts` retries hardcoded to 0, not elevated in CI | should be `process.env.CI ? 2 : 0` for flaky network-bound realapi tests |
| 10 | low | No top-level `test` npm script — requires `pnpm exec vitest run` | DX gap; CI templates expect a `test` alias |
| 11 | low | Missing `test:e2e:realapi` and `test:e2e:visual` shorthand scripts | inconsistency vs `test:e2e:smoke/exhaustive/destructive` |
| 12 | low | `@hookform/devtools` devDep + `FormDevtool` wrapper component have no callers | dead code |
| 13 | low | Dockerfile builder stage uses broad `COPY . .` — secrets rely entirely on `.dockerignore` | well-structured .dockerignore mitigates, but targeted COPY is defense-in-depth |
| 14 | low | Large cluster of high-value services have no unit tests at all | interest-berths, portal-auth, alert-engine, berth-rules-engine, documenso-webhook, document-reminders, external-eoi, residential, document-sends, notifications, webhooks (~50 services) |
| 15 | info | Exhaustive e2e tests use `test.skip(true, ...)` as soft guards when fixtures absent | intentional graceful-degrade pattern; not a bug |
### C3 — Observability + infra (10 findings: 2 high, 1 medium, 5 low, 2 info)
| # | Severity | Title | Evidence |
| --- | -------- | ------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| 1 | **high** | GDPR export bundles NOT deleted from storage on client hard-delete | `gdpr.ts:35` storageKey + `client-hard-delete.service.ts:241-244` — files.clientId collected, gdprExports.storageKey never queried; cascade kills DB row but blob orphans. **This is a gap in the A.7 RTBF fix shipped today.** |
| 2 | **high** | NO RTBF/hard-delete path for `residential_clients` | residential.ts schema holds equivalent PII to marina clients; zero hard-delete code path — no confirmation flow, no blob sweep, no audit, no API endpoint |
| 3 | medium | `sentTo` key bypasses audit masker — operator email stored plaintext in audit_logs.metadata | `client-hard-delete.service.ts:139,466` — `sent_to` doesn't contain 'email' substring. Fix: add 'sent_to' fragment, or rename to `sentToEmail` |
| 4 | low | S3Backend `presignUpload`/`presignDownload` lack `withTimeout` wrappers | `s3.ts:289-297` — every other method (put/get/head/delete) is wrapped; presigns aren't. TCP-blackhole stall risk |
| 5 | low | `error_events.errorMessage` and `errorStack` stored without PII redaction | error-events.service.ts:143-145 — ORM errors embedding WHERE-clause values persist as PII |
| 6 | low | `'auth'` fragment over-masks: `authorId`, `isAuthenticated`, etc. | `audit.ts:125` — `'auth'` is too broad; should be `'authorization'` or use prefix match |
| 7 | low | RTBF `website_submissions` erasure only matches top-level JSONB `email` key | `client-hard-delete.service.ts:221-224` — nested email payloads (`payload.contact.email`) survive |
| 8 | low | `hardDeleteCode` rate limiter fails open + `Math.random()` 4-digit code | combined attack surface during Redis outage; switch to `crypto.randomInt()` regardless |
| 9 | info | `bulkHardDeleteClients` emits no composite audit log for the bulk action itself | forensic correlation requires grouping N rows by timestamp; one bulk-level log entry would fix it |
| 10 | info | `requestBulkHardDeleteCode` loads ALL port clients into memory for validation | `client-hard-delete.service.ts:408-419` — should `WHERE id IN (args.clientIds)` |
### B4 — Webhooks + auth + storage (15 findings: 1 high, 6 medium, 5 low, 3 info)
| # | Severity | Title | Evidence |
| --- | -------- | ------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| 1 | **high** | better-auth rate limiter uses in-memory storage — multi-replica prod bypasses limits | `auth/index.ts:128-137` — N replicas multiplies attempt budget N×; documented as known. Swap to `storage: database` |
| 2 | medium | DOCUMENT_SIGNED route-level dedup hash never matches stored events — every retry re-enters handler | `webhooks/documenso/route.ts:173 vs documents.service.ts:1184` — raw-body SHA vs prefixed-form hash, never matches; dedup intent broken |
| 3 | medium | Concurrent SIGNED webhooks both see `wasAlreadySigned=false`, both dispatch cascade invites | `documents.service.ts:1130-1131,1196-1208` — read outside tx; handleDocumentCompleted has correct SELECT FOR UPDATE pattern but handleRecipientSigned doesn't |
| 4 | medium | Rate limiter fails open on Redis outage — auth brute-force protection disabled | `rate-limit.ts:57-73` — intentional; consider fail-closed + admin-IP allowlist escape hatch |
| 5 | medium | `callbackURL` forwarded to better-auth without origin validation in sign-in-by-identifier | `auth/sign-in-by-identifier/route.ts:63-96` — potential open redirect post-auth |
| 6 | medium | `originAllowed()` returns true when both Origin AND Referer absent — non-browser CSRF check bypassed | `proxy.ts:118-136` — SameSite=Strict is the real gate but defense-in-depth has a hole |
| 7 | medium | Legacy plaintext Documenso webhook secrets may persist in `system_settings` — no migration enforcement | `port-config.ts:469-472` — ports that never rotated retain cleartext |
| 8 | low | Storage proxy token `p` port-binding field is optional — tokens without `p` skip cross-port enforcement | `filesystem.ts:184-188,95-111` — future callers that omit portSlug mint cross-port tokens |
| 9 | low | Storage proxy PUT magic-byte check is application/pdf only — other content types accepted blind | `api/storage/[token]/route.ts:222-225` — png/jpg/csv/zip not inspected |
| 10 | low | Dev HMAC fallback derives storage proxy secret from `BETTER_AUTH_SECRET` — shared key in dev | `filesystem.ts:430-432` — prod rejects but dev exposed→internet could forge tokens with auth key |
| 11 | low | CSP policy has no `report-uri`/`report-to` — XSS probes blocked silently | `proxy.ts:16-37` — adding `/api/csp-report` would give early-warning |
| 12 | low | sign-in-by-identifier timing oracle: email-format skips DB; username-format always hits DB | very low practical impact; doesn't reveal whether identifier exists |
| 13 | info | better-auth's built-in rate limiter doesn't add `Retry-After` on 429 | direct `/api/auth/sign-in/email` lacks RFC 6585 compliance; sign-in-by-identifier wrapper has it |
| 14 | info | Session cookie lacks `__Host-` prefix — subdomain binding not enforced | `auth/index.ts:106` — SameSite=Strict+Secure mitigate; `__Host-` would forbid Path other than `/` |
| 15 | info | `listDocumensoWebhookSecrets()` issues full DB SELECT on every webhook with no cache | `port-config.ts:456-501` — amplifies bad-secret flood scenario; short TTL cache fixes |
### C1 — EOI/Documenso services (15 findings: 3 high, 5 medium, 4 low, 3 info)
| # | Severity | Title | Evidence |
| --- | -------- | ----------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------- |
| 1 | **high** | `generateAndSignViaInApp` omits `portId` on all Documenso calls — per-port v1/v2 config bypassed | `document-templates.ts:705,717` — portId optional → env fallback; v2-configured port uses v1 env defaults |
| 2 | **high** | custom-document-upload: `placeFields` called AFTER `documensoSend` — v2 envelope already PENDING when fields placed | `custom-document-upload.service.ts:285,294,323` — header comment documents correct order; code inverts. v2 may reject; all v2 contract/reservation uploads land with no signature fields |
| 3 | **high** | `{{eoi.berthRange}}` and all `{{reservation.*}}` tokens in VALID_MERGE_TOKENS but resolveTemplate never populates them | merge-fields.ts:64-76 + document-templates.ts — tokens render as literal `{{...}}`; BR-140 doesn't catch because required:false |
| 4 | medium | `sendReminder` passes CRM document_signers.id (UUID) as Documenso recipient ID — v1 path sends invalid URL, v2 redistributes blindly | `document-reminders.ts:161` + `documenso-client.ts:910` — v1 reminders consistently fail with 404; schema missing `documenso_recipient_id` column |
| 5 | medium | `custom-document-upload` does not persist `documensoNumericId` — v2 webhook numeric-id resolution can't match | `custom-document-upload.service.ts:345` — contract/reservation uploads on v2 instance miss webhook events |
| 6 | medium | `generateDocumentFromTemplate` v2: distribute failure swallowed — all signer rows get signingUrl=null with no auto-recovery | `documenso-client.ts:554-560` + `document-templates.ts:843-884` — "Send invitation" button errors for every signer |
| 7 | medium | `handleDocumentCompleted`: interest side-effects (dateEoiSigned, berth-rules) run outside try/catch and are not idempotent across retries | `documents.service.ts:1574-1621` — each failed-PDF retry re-stamps dateEoiSigned |
| 8 | medium | `distributeEnvelopeV2` normalize call loses numericId — self-heal callers can't persist | `documenso-client.ts:618-623` — pattern from generateDocumentFromTemplate not followed |
| 9 | low | `voidDocument` uses raw fetchWithTimeout without pRetry — transient 5xx/429 not retried | `documenso-client.ts:1289` |
| 10 | low | `completion_cc_emails` recipients have empty name — signing-completed email greeting malformed | `documents.service.ts:1722` — "Dear ," fallback; should be email as display name |
| 11 | low | `normalizeSignerRole` maps developer slot (order-2 SIGNER) to 'signer' not 'developer' — progress panel label wrong | `document-templates.ts:863-865,930-935` |
| 12 | low | `persistDocumentOverrides` source_document_id backfill uses 1-minute window — race if generation takes >60s | `eoi-overrides.service.ts:451,463,471` — widen to 5min or backfill by returned IDs |
| 13 | info | `resolveTemplate` ValidationError catch regex includes dead branch 'interest has no (yacht | berth)' | `document-templates.ts:317-322` — dead from prior design; remove for clarity |
| 14 | info | berth-range: non-canonical (passthrough) moorings always appended after sorted canonical segments | `berth-range.ts:105-108` — cosmetic |
| 15 | info | `{{interest.notes}}` always empty in non-EOI (legacy) resolveTemplate path | `document-templates.ts:378` — silent blank in correspondence templates |
### C2 — Domain services (15 findings: 1 high, 3 medium, 6 low, 5 info)
| # | Severity | Title | Evidence |
| --- | -------- | ---------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| 1 | **high** | Recommender: SQL vs JS stage-scale mismatch — Tier D fires one stage too early | `berth-recommender.service.ts:212,499-502,223,554` — JS LATE_STAGE_THRESHOLD=5 (deposit_paid in JS scale) vs SQL emits 5=reservation. Tier D fires at reservation, not deposit_paid. Berths with reservation-stage active interest hidden one stage early. |
| 2 | medium | `createNotification` dedup is non-atomic SELECT-then-INSERT with no DB unique constraint (TOCTOU) | `notifications.service.ts:67-85,117` — concurrent inquiry fan-out can double-insert. Fix: partial unique on `(userId, type, dedupeKey)` + ON CONFLICT DO NOTHING |
| 3 | medium | `completeReminder` TOCTOU — concurrent calls both pass status guard, produce dup audit rows | `reminders.service.ts:317-332` — no `WHERE status='pending'` in UPDATE; no advisory lock |
| 4 | medium | `processFollowUpReminders` lacks advisory lock — concurrent workers double-insert auto-generated reminders | `reminders.service.ts:428-517` — 3 non-tx round-trips; `processOverdueReminders` has the right pattern, this one doesn't |
| 5 | low | `createNotification` with inApp=false + email=true silently drops the email | `notifications.service.ts:107-113` — acknowledged in comment but untracked gap |
| 6 | low | `public-interest` creates interest with legacy `pipelineStage='open'` instead of `'enquiry'` | `public-interest.service.ts:233` — modern stage is `enquiry`; column default agrees |
| 7 | low | `public-interest` berth lookup outside transaction — FK violation on race-deleted berth | `public-interest.service.ts:79-87,237-244` |
| 8 | low | `public-interest` no yacht dedup — re-submissions create duplicate yacht records | `public-interest.service.ts:177-203` — client + company dedup'd; yacht isn't |
| 9 | low | `inquiry-notifications.findUsersWithInterestsPermission` has no deactivated-user filter | `inquiry-notifications.service.ts:149-168` — deactivated users still receive new_registration alerts |
| 10 | low | Rules engine suggest-mode unconditionally calls `createAuditLog` — audit flood on webhook retries | `berth-rules-engine.ts:102-117,201-207` |
| 11 | low | interest-berths cross-port guard silently passes when interestId doesn't exist | `interest-berths.service.ts:232-244` — should throw NotFoundError explicitly |
| 12 | info | `processOverdueReminders` un-snooze + claim are two non-tx UPDATEs — survivable, no fix required | at-least-once semantics |
| 13 | info | Dynamic import in `removeInterestBerth` is still required (cycle break) | `interest-berths.service.ts:356-361` — not stale |
| 14 | info | Inconsistent `evaluateRule` import style — static vs dynamic across files | maintenance hazard; documenting needed |
| 15 | info | `STAGE_ORDER.completed=6` in recommender JS is dead code — SQL CASE never emits 'completed' | misleads maintainers |
### D1 — Jobs/queue/cron (8 findings: 3 critical, 1 high, 2 medium, 2 low)
| # | Severity | Title | Evidence |
| --- | ------------ | -------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| 1 | **CRITICAL** | `send-invoice` + `invoice-overdue-notify` dispatched to queues WITH NO WORKER HANDLER | `invoices.ts:597-600,740-743` — both fall to default branch, log "Unknown … job", complete successfully. **Every invoice send AND every overdue check is a silent no-op.** |
| 2 | **CRITICAL** | 5 maintenance cron jobs scheduled but unimplemented — silent no-ops with false-green audit | scheduler.ts: `calendar-sync`, `database-backup`, `backup-cleanup`, `session-cleanup`, `temp-file-cleanup` — workers/maintenance.ts has no case for any. **database-backup is the dangerous one.** RECURRING_JOB_NAMES contains them so audit shows green. |
| 3 | **CRITICAL** | `tenure-expiry-check` scheduled, in RECURRING_JOB_NAMES, but has no handler and no service | scheduler.ts:32 — daily 08:00 schedule; workers/notifications.ts no case; no `tenure-expiry` service exists |
| 4 | high | `processDocumensoPoll` TOCTOU race — concurrent ticks can double-fire cascading invite email | `jobs/processors/documenso-poll.ts:46-47` — wasAlreadySigned read outside tx; documents queue concurrency=3 with 5-min poll → overlapping ticks plausible |
| 5 | medium | `documenso-void` enqueued without natural-key jobId at both archive call sites | `clients/[id]/archive/route.ts:95`, `clients/bulk/route.ts:180` — double-archive enqueues two void jobs; second hits already-voided envelope → spurious dead-letter |
| 6 | medium | `report-scheduler` `nextRunAt` UPDATE not transactional with job enqueue — crash silently drops a period | workers/reports.ts — 3 separate round-trips; crash between A and C skips the period |
| 7 | low | `bounce-poll` absent from RECURRING_JOB_NAMES — no cron_run audit row on successful ticks | audit-helpers.ts:27-49 — operators can't detect stalled poller via audit log |
| 8 | low | maintenance queue concurrency=1 with HOL-blocking risk | analytics-refresh + bounce-poll can starve alerts-evaluate (every 5min) — split into fast/slow queues |
### F2 — Performance (8 findings: 3 high, 5 medium)
| # | Severity | Title | Evidence |
| --- | -------- | --------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------- |
| 1 | **high** | `getClientById`: 6 independent DB queries run SEQUENTIALLY on hot client detail path | `clients.service.ts:358,363,368,374,392,415` — 7 serial round-trips per page load; should be `Promise.all([...6])` after gating client lookup |
| 2 | **high** | `notification-digest`: nested port×user loops → O(ports × users) sequential queries + emails | `notification-digest.service.ts:71,74,109,113` — per port: 6+ queries; per user: 1 query + 1 send, all serial. Ports + users are independent |
| 3 | **high** | Missing index on `interests.reminder_enabled` — `processFollowUpReminders` full-scans active interests per port | `reminders.service.ts:432-441` — no existing index covers `(portId, reminderEnabled) WHERE archived_at IS NULL` |
| 4 | medium | `reconcileAlertsForPort`: N individual INSERTs + N UPDATEs per alert-engine evaluation | `alerts.service.ts:53-80,89-99` — batch INSERT ... ON CONFLICT DO NOTHING RETURNING; UPDATE WHERE id IN (...) |
| 5 | medium | `client-archive-dossier`: N DB queries inside loop over `distinctBerthIds` | `client-archive-dossier.service.ts:244,252` — single query WHERE berthId IN (...) + JS group |
| 6 | medium | `email_threads`: no compound `(portId, lastMessageAt)` index — list endpoint forces filesort | `email.ts:57` — only `idx_et_port` covers portId; sort step grows with thread volume |
| 7 | medium | `createPending` (berth-reservations): 3 independent tenant-validation lookups serial | `berth-reservations.service.ts:95,100,105` — berth/client/yacht should be `Promise.all` |
| 8 | medium | `webhook-dispatch`: sequential INSERT + BullMQ enqueue per matching webhook | `webhook-dispatch.ts:47-75` — batch the inserts (RETURNING id), then Promise.all the queue.adds |
### A1 — Schema: people/orgs (audited inline; agent stuck) (12 findings: 1 high, 6 medium, 5 low)
| # | Severity | Title | Evidence |
| --- | -------- | --------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| 1 | **high** | `yachts.currentOwnerType`/`currentOwnerId` polymorphic — NO CHECK constraint on the type discriminator | `yachts.ts:44-45` — `currentOwnerType` is bare text; a value other than `'client'`/`'company'` silently corrupts ownership resolution downstream |
| 2 | medium | `clients.mergedIntoClientId` self-FK lives in migration 0042 only — `db:push` drift (same pattern as A3 #5) | `clients.ts:53-58` — Drizzle's table builder doesn't accept self-references in column factory; constraint missing from db:push schema |
| 3 | medium | `clients.sourceInquiryId` FK lives in migration 0065 only — `db:push` drift | `clients.ts:33-38` — comment acknowledges the gap; fresh db:push skips it |
| 4 | medium | `clientAddresses.label='Primary' default` + `isPrimary=true default` conflicts | `clients.ts:250,258` — every new address is "primary" by default; partial unique `idx_ca_primary` then rejects the second. Either flip the default or fail less surprising |
| 5 | medium | No DB CHECK on `clients.preferredContactMethod` enum (email/phone/whatsapp) | `clients.ts:27` |
| 6 | medium | No DB CHECK on `yachts.status` enum (active/retired/sold_away) | `yachts.ts:46` |
| 7 | medium | `companyMemberships.role` no DB CHECK on enum (director/officer/broker/representative/legal_counsel/employee/shareholder/other) | `companies.ts:65` |
| 8 | low | `clientNotes.authorId`, `yachtNotes.authorId`, `companyNotes.authorId` all bare text — no FK to user | `clients.ts:149`, `yachts.ts:107`, `companies.ts:126` — dangling on hard user delete |
| 9 | low | `clients.archivedBy` bare text — no FK to user; same dangling-on-delete pattern | `clients.ts:41` |
| 10 | low | `clientTags.tagId`, `yachtTags.tagId`, `companyTags.tagId` — bare text, comment-only FK to tags | `clients.ts:165`, `yachts.ts:123`, `companies.ts:142` — same gap as A2 #7 for pipeline tables |
| 11 | low | `yachtOwnershipHistory` has no DB-level guard that `startDate ≤ endDate` | `yachts.ts:83-84` — date inversion possible without CHECK |
| 12 | low | `yachts.lengthFt`/`lengthM`/`lengthUnit` denormalized triple — no DB-level invariant that lengthUnit aligns with which of (lengthFt, lengthM) is non-null | `yachts.ts:32-43` — service layer can write `lengthUnit='ft'` while `lengthFt=null`; produces broken display |
### F1 — Cross-cut: security (audited inline; agent stuck) (4 findings: 1 medium, 3 low)
The cross-cutting security audit is partly redundant with B1/B4/C3 findings already reported. Only NEW issues here:
| # | Severity | Title | Evidence |
| --- | -------- | -------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| 1 | medium | `send-document-dialog.tsx` lines 248 + 274 use `dangerouslySetInnerHTML` for previewHtml — verify `renderEmailBody()` allowlist sanitization | `send-document-dialog.tsx:248,274` — flows from API; `renderEmailBody` documented escape-then-allowlist, but the dialog's preview path needs explicit audit to confirm no untrusted HTML leaks |
| 2 | low | Many `findFirst` queries in services without explicit `port_id` filter — depends on FK chain | examples: `notes.service.ts:767`, `email-threads.service.ts:68,101,106,144,177,255` — defense-in-depth gap; FK joins enforce isolation but a direct call from a route bypassing service wrappers could leak |
| 3 | low | 136 raw `sql\`\`` template literals in services — manual review-worthy for SQLi | full sweep not done; spot-checks at known sites (berth-recommender, search) use parameterized `${}` interpolation via Drizzle |
| 4 | info | Most other security surfaces already covered by B1/B4/C3 reports above | see `cross-references` |
### B3 — v1 entity CRUD (audited inline; agent stuck) (3 findings, structurally clean)
Spot-check across 303 v1 route files: **structurally healthy.** Sample at `/api/v1/clients/route.ts` is exactly the documented pattern — `withAuth(withPermission(resource, action, async (req, ctx) => { try { parseBody/parseQuery + service call; return {data}; } catch (error) { return errorResponse(error); } }))`. No bare route handlers found.
| # | Severity | Title | Evidence |
| --- | -------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------- |
| 1 | low | `handlers.ts` sibling pattern means grep for missing withAuth needs to skip them | not a finding per se, just a noting that the testability split documented in CLAUDE.md is honored |
| 2 | low | Pagination shape on `/api/v1/clients` returns `{data, pagination: {...}}` but list endpoints elsewhere return `{data, total, hasMore}` (CLAUDE.md convention) | `clients/route.ts:18-28` — minor shape drift; not breaking but lists aren't uniform |
| 3 | info | Most B3 quality findings already covered by B1 (port validation), C2 (race + dedup), C3 (audit gaps) | this scope was already well-covered |
### E1 — Admin UI (agent stuck; not audited)
The admin-ui agent went idle 4 times across multiple pings. The most likely interpretation is that the surface is large enough that even Sonnet 1M's context was filled before a useful answer landed. **E1 should be re-spawned with a much narrower scope (one page at a time) or audited inline in a follow-up pass.**
### E2 — Entity UI (agent stuck; not audited)
Same pattern as E1. Entity-tab UI surface across 5 entity types is large; the agent didn't complete. **Re-spawn with narrower scope (one entity-detail page per agent) or defer.**
---
## Triage + recommended order of operations
After 13 reported audits + 2 inline (A1, F1, B3 sketch), here are the items that should ship before the next deploy, grouped by impact and effort.
### 🚨 Tier S — ship-stopping production bugs (do today)
These are silently broken in production right now. Fix before any further work.
| Source | Item | Effort |
| --------- | -------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------- |
| **D1 #1** | `send-invoice` and `invoice-overdue-notify` BullMQ jobs have no handler → every invoice send is a no-op | 1-2h: add the cases to workers/email.ts and workers/notifications.ts |
| **D1 #2** | 5 maintenance cron jobs (calendar-sync, database-backup, backup-cleanup, session-cleanup, temp-file-cleanup) silently no-op with false-green audit | 2-3h each; **database-backup is the dangerous one** — implement or remove the schedule |
| **D1 #3** | `tenure-expiry-check` cron silently no-ops; service was never written | 2-3h: write the service + handler |
| **C3 #1** | A.7 RTBF gap: `gdpr_exports.storage_key` blobs NOT deleted on client hard-delete (this is a gap in code shipped today) | 30min: extend `client-hard-delete.service.ts` to collect gdprExports.storageKey alongside files |
| **C3 #2** | No RTBF/hard-delete path for `residential_clients` — full PII shadow | 4h: mirror the marina hard-delete service for residentialClients |
| **B1 #1** | `/api/public/interests` does NOT validate caller-supplied `portId` against existing ports — cross-tenant data injection | 30min: copy the residential-inquiries pre-check |
| **A3 #1** | `documents.documenso_id` has NO index — every webhook delivery is a full table scan | 30min: migration adding index |
### 🔴 Tier 1 — high severity, prioritize this week
| Source | Item | Effort |
| ------------------ | ----------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------- |
| **B4 #1** | better-auth rate limiter is in-memory; multi-replica prod multiplies auth limits N× | 2h: switch to `storage: 'database'` after running its migration |
| **C1 #1** | `generateAndSignViaInApp` omits portId on Documenso calls → v2-configured port silently uses v1 env defaults for every in-app EOI | 30min: thread portId through 2 calls |
| **C1 #2** | custom-document-upload calls `placeFields` AFTER `documensoSend` (wrong order) — v2 may reject placement on PENDING envelope | 30min: reorder |
| **C1 #3** | `{{eoi.berthRange}}` + all 5 `{{reservation.*}}` tokens valid but unresolved — render as literal `{{...}}` | 2h: populate from EoiContext.eoiBerthRange + add reservation resolver |
| **C2 #1** | Recommender SQL vs JS stage-scale mismatch — Tier D fires at reservation, not deposit_paid | 30min: change LATE_STAGE_THRESHOLD=6 to match SQL scale |
| **F2 #1-3** | 3 high-impact perf: getClientById serial queries, notification-digest sequential loops, missing index on interests.reminder_enabled | 4h total |
| **F3 #1-2** | client-hard-delete has zero tests; no CI/CD pipeline | 4h: integration tests for the RTBF flow; add `.github/workflows/ci.yml` |
| **A2 #1-3, A1 #1** | Missing DB-level CHECK constraints on every enum-shaped text column | 2h: one consolidated migration |
### 🟠 Tier 2 — medium severity (next sprint)
Covers the bulk of remaining medium findings — too many to expand inline; see per-agent tables above. Highlights: drift between schema and migrations (A3 #4-5, A1 #2-3), idempotency gaps in webhook handlers (B4 #2-3, C1 #7, D1 #4), audit/IP/UA gaps in admin mutations (B2 #7-10), and the camelCase-key over-masking false-positive on `'auth'` fragment (C3 #6).
### 🟡 Tier 3 — low severity (rolling)
Index optimizations, validation tightening, schema metadata gaps, log cleanup. The detailed tables per agent above carry the per-item file:line evidence.
### 📋 Tier 4 — re-spawn or inline-audit
- E1 (admin UI) and E2 (entity UI) agents failed; the surface is too large for a single Sonnet 1M spawn. Re-spawn narrower (one page or one entity per agent), or audit inline in a follow-up.
---
## Total finding counts
| Severity | Count |
| ------------------ | ------- |
| CRITICAL | 5 |
| high | 15 |
| medium | 36 |
| low | 53 |
| info | 19 |
| **Total findings** | **128** |
Across **13 of 16 agent reports** + 3 inline (A1/F1/B3). E1 + E2 are missing; should be re-attempted later.
Reference in New Issue View Git Blame Copy Permalink