docs(audit): full codebase audit — 128 findings across 16 areas
Spawned 16-agent sonnet[1m] audit team covering schemas (people/orgs,
pipeline, docs+infra), APIs (public, admin, v1 CRUD, webhooks/auth/
storage), services (EOI/Documenso, domain, observability), background
jobs, UI (admin, entity), and cross-cutting security/performance/tests-
deps. 13 of 16 agents delivered detailed JSON reports; A1/F1/B3 audited
inline after their agents stalled. E1/E2 (admin + entity UI) couldn't
complete in a single spawn — flagged for re-attempt with narrower scope.
Top findings:
- 5 CRITICAL: send-invoice and invoice-overdue-notify silently no-op
(D1#1); 5 maintenance crons including database-backup scheduled but
unimplemented (D1#2); tenure-expiry-check ditto (D1#3); GDPR export
bundles not deleted on RTBF (C3#1, gap in A.7 shipped today);
residential_clients has no hard-delete path at all (C3#2).
- 15 HIGH including: /api/public/interests doesn't validate portId
(B1#1, cross-tenant injection); documents.documenso_id has zero
index (A3#1, every webhook is a full scan); better-auth rate limit
is in-memory (B4#1, multi-replica bypass); generateAndSignViaInApp
omits portId on Documenso calls (C1#1); custom-doc-upload calls
placeFields after distribute (C1#2); {{eoi.berthRange}} +
{{reservation.*}} tokens never resolved (C1#3); recommender SQL/JS
stage-scale off-by-one (C2#1); getClientById runs 6 queries serial
(F2#1); no CI pipeline + zero tests on client-hard-delete (F3#1,2).
- 36 medium, 53 low, 19 info.
Triage groups in the doc:
Tier S: 7 ship-stopping bugs (today)
Tier 1: ~12 high-severity items (this week)
Tier 2: ~36 medium (next sprint)
Tier 3: ~53 low (rolling)
Tier 4: re-spawn E1+E2 with narrower scope
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
333
docs/superpowers/audits/2026-05-18-full-codebase-audit.md
Normal file
333
docs/superpowers/audits/2026-05-18-full-codebase-audit.md
Normal file
@@ -0,0 +1,333 @@
|
||||
# Full Codebase Audit — 2026-05-18
|
||||
|
||||
> **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
Block a user