fix(audit): comprehensive 2026-05-15 audit fix wave + Documenso v2 polish
Bundles the prior session's 50-task fix sweep (Documenso v2 + EOI/signing-
progress redesign + env-to-admin migration + dev-mode banner) with the
2026-05-18 audit fix wave (3 CRITICAL, 14 HIGH, 28 MEDIUM, 6 LOW).
CRITICAL (3):
- C-01 interest-berths INNER JOIN -> LEFT JOIN so hard-deleted berths
no longer silently drop interest links
- C-02 /setup added to PUBLIC_PATHS; fresh-deploy bootstrap loop fixed
- C-03 generic PATCH /interests/[id] no longer accepts pipelineStage —
callers must go through /stage with the override-guard chain
HIGH (14/15):
- H-01 explicit ON DELETE on previously-implicit NO ACTION FKs across
interests/documents/reservations/reminders/invoices (migration 0070)
- H-02 login page reads ?redirect= param with same-origin guard
- H-03 CRM invite token moves to URL fragment so it never lands in
nginx access logs / Referer headers
- H-04 Retry-After header on sign-in-by-identifier 429 (RFC 6585 §4)
- H-05 toggleAccount writes an audit row
- H-06 upsertSetting masks any value whose key ends with _encrypted
- H-07 archiveClient cascade fires per-interest audit rows
- H-08 createSalesTransporter applies SMTP_TIMEOUTS
- H-09 AppShell stable children — viewport flip across breakpoint no
longer destroys in-progress form drafts
- H-10 portal documents page swaps Unicode glyph status icons for
Lucide CheckCircle2/XCircle/Circle + aria-labels
- H-12 list components swap alert(...) for toast.warning(...)
- H-13 5 icon-only buttons gain aria-label
- H-14 parseBody treats empty bodies as {}
- H-15 admin layout renders a 403 panel instead of silent bounce
- H-11 not applicable — mobile-search-overlay IS a mobile bottom-sheet
MEDIUM (28+):
- M-MT01-05 defense-in-depth port_id/parent-id filters on UPDATE/DELETE
WHEREs across custom-fields, notes (all 6 entity types x update +
delete), client-contacts, yacht ownerClient lookup, webhook reads
- M-D01 documents-hub realtime event-name typo (file:created -> uploaded)
- M-EM01 portal-auth emails thread through portId
- M-EM02 sendEmail accepts cc/bcc params
- M-EM04 notification_digest catalog key
- M-IN01 portal presigned download URLs use 4h TTL
- M-IN02 OpenAI client lazy-instantiated
- M-IN04 stale pdfme refs updated to pdf-lib AcroForm
- M-IN05 umami.testConnection returns tagged union
- M-L01 reservations tenure_type unified with berths
- M-L02 report-generators canonicalize stage values
- M-AU01 audit log placeholder copy fixed
- M-AU04 outcome_set / outcome_cleared distinct audit verbs
- M-NEW-2 activity feed entity name+type separator
- M-R01 portal allowlist narrowed + portal_session backstop in proxy
- M-SC02 companies archived partial index
- M-SC04 audit_logs.searchText documented as DB-managed
- M-S01 storage_s3_access_key_encrypted admin field
- M-U01 audit log empty state uses <EmptyState>
- M-U09 invoice delete dialog -> <AlertDialog>
- M-U10 toast.success on ClientForm + InterestForm create/edit
- M-U11 settings-form-card logo preview alt text
- M-U14 mobile topbar title on clients/yachts/interests/berths
- M-U15 Invoices in mobile More-sheet
LOW (6/8):
- L-AU01 severity defaults for security-relevant verbs
- L-AU02 +13 missing actions in admin audit filter
- L-AU03 +7 missing entity types in admin audit filter
- L-AU04 dead listAuditLogs stubbed
- L-D02 CLAUDE.md Owner-wins chain tightened
Bonus — Document detail polish (#67 partial, 3/6 deliverables):
- state-aware action button per signer
- watcher Add UI with display-name resolution
- cleanSignerName cleanup
Prior session work bundled in:
- Documenso v2 webhook + envelope-ID normalization + sequential signing
- SigningProgress UI redesign (avatars, per-signer state, timestamps)
- env->admin settings registry + RegistryDrivenForm + encrypted creds
- Embedded-signing card + Test connection + setup help
- Dev-mode EMAIL_REDIRECT_TO banner
- Pipeline rules admin page
- Sales email config card
- Audit log details Sheet
- EOI tab: Finalising badge, absolute timestamps, sequential indicator
- Notes pipeline_stage_at_creation (migration 0069)
- Documenso numeric ID dual-key webhook (migration 0068)
- Dimensions criterion copy (migration 0067)
Tests: 1374/1374 vitest pass. tsc clean. lint clean.
See docs/AUDIT-FIX-WAVE-2026-05-18.md for the full progress report and
the user-input items still pending.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
22
docs/audit-findings-tmp/01a-legacy-master-grep.md
Normal file
22
docs/audit-findings-tmp/01a-legacy-master-grep.md
Normal file
@@ -0,0 +1,22 @@
|
||||
# L-001 Legacy Stage Enum Master Grep — agent #12 (re-dispatch slice 1)
|
||||
|
||||
**Headline:** The 9→7 stage refactor is correctly implemented; zero bugs found across 25 files with legacy-stage-name hits.
|
||||
|
||||
**Counts:** 0 critical · 0 high · 0 medium
|
||||
|
||||
---
|
||||
|
||||
## Verdict
|
||||
|
||||
The two `stageRank` Records (`clients.service.ts:276-283`, `berth-recommender.service.ts:195-210`) intentionally include both legacy AND modern keys mapping to the same final ranks — yesterday's commit `9821106` purged the gap. The rules engine (`berth-rules-engine.ts:15-42`) and document services use legacy _trigger event_ names (`eoi_sent`/`eoi_signed`/`contract_signed`) rather than stage names — both old and new events fire correctly because they're labels for webhook/doc events, not pipeline stages.
|
||||
|
||||
## Legitimate / neutral hit categories
|
||||
|
||||
- **Historical lookup tables (designed for dual-stage support):** `clients.service.ts:276-283` `stageRank`, `berth-recommender.service.ts:195-210` `STAGE_ORDER` — both have legacy + modern keys.
|
||||
- **Refactor mapping definitions:** `constants.ts:59-65` `LEGACY_STAGE_REMAP`; `dedup/migration-transform.ts:206-212` legacy-to-legacy map for NocoDB import.
|
||||
- **Rules engine + service layer (legacy-aware design):** `berth-rules-engine.ts:15-42` (trigger event labels), `external-signing.service.ts:37-41`, `documents.service.ts:786/909/1503/1544/1574` (`evaluateRule('eoi_sent'|'eoi_signed'|'contract_signed', ...)`), `external-eoi.service.ts:138-151` (intentional legacy-aware advance branch).
|
||||
- **Schema metadata:** `db/schema/interests.ts:61-65` field names (`dateEoiSent`, `dateEoiSigned`, `dateContractSent`, `dateContractSigned`) — historical schema column names.
|
||||
- **UI display:** `email/templates/notification-digest.tsx:29` `eoi_signed: 'EOI signed'` label for historical data.
|
||||
- **Comments only:** `alert-rules.ts:83`, `interests.service.ts:938/980/1095`, `berths.service.ts:175`, `db/schema/operations.ts:98`.
|
||||
|
||||
**No silent-failure lookup tables. No rank-0 fallthrough patterns. No raw legacy enum keys leaking to the UI without remap.**
|
||||
28
docs/audit-findings-tmp/01b-legacy-rendering-surfaces.md
Normal file
28
docs/audit-findings-tmp/01b-legacy-rendering-surfaces.md
Normal file
@@ -0,0 +1,28 @@
|
||||
# L-002-011 Legacy Stage Rendering Surfaces — done in main thread (sub-agent context-thrashed)
|
||||
|
||||
**Headline:** Mostly clean. One LOW finding: report-generators stage rollup keys are raw enum without `LEGACY_STAGE_REMAP`/`canonicalizeStage` — defensive-coding gap if any active row drifts back to a legacy stage value (migration 0062 normalized, so this is theoretical).
|
||||
|
||||
**Counts:** 0 critical · 0 high · 0 medium · 1 low (defensive)
|
||||
|
||||
---
|
||||
|
||||
## 🟢 LOW L-008: Reports stage-revenue rollup uses raw `interests.pipelineStage` without `canonicalizeStage`
|
||||
|
||||
- **File:** `src/lib/services/report-generators.ts:71-76, 88-106, 124-138, 176-192`
|
||||
- **What:** `stageRevenueMap[row.stage] = ...` and `pipelineWeights[row.stage]` use the raw enum value from the SQL `groupBy(interests.pipelineStage)`. No `canonicalizeStage()` wrap.
|
||||
- **Why it matters:** Migration 0062 normalized historical data to modern values, so today active rows should all be in the 7-stage set and bucketing is correct. But if any leakage occurs (NocoDB re-import, partial migration on a future port, manual `psql` write), legacy values would be siloed into their own bucket and `pipelineWeights[legacy_value]` returns `undefined` → that bucket contributes 0 to the forecast. Silent.
|
||||
- **Suggested fix:** Wrap row.stage with `canonicalizeStage(row.stage)` from `src/lib/utils/legacy-stage.ts` before keying into `stageRevenueMap` / `pipelineWeights`.
|
||||
|
||||
---
|
||||
|
||||
## ✅ Passing checks
|
||||
|
||||
- **L-002 audit log diff** — `audit-log-list.tsx` / `audit-log-card.tsx` don't render stage values at all (just field-name keys per agent #4's AU-08 finding). No raw-enum render path exists.
|
||||
- **L-003 activity feed** — `src/components/dashboard/activity-feed.tsx:14,57` imports and uses `LEGACY_STAGE_REMAP` for the stage_change diff line.
|
||||
- **L-004 email templates** — `src/lib/email/templates/notification-digest.tsx:24` `TYPE_LABELS` includes `eoi_signed` as a _notification type_ label (the doc-status event), not a pipeline stage. Legitimate.
|
||||
- **L-005 Documenso payload** — `src/lib/services/documenso-payload.ts` and `src/lib/templates/merge-fields.ts` have zero `pipelineStage` / `pipeline_stage` references. EOI payload doesn't surface stage.
|
||||
- **L-006 public berths status filter** — already verified clean by agent #7 (IN-17). `src/lib/services/public-berths.ts:90-97` `derivePublicStatus` only branches on `sold` / `under_offer` / else `available`. No legacy enum acceptance.
|
||||
- **L-007 outbound webhook** — `webhook-dispatch.ts` is a passthrough; payload built at `interests.service.ts:919-934` (`emitToRoom` + `dispatchWebhookEvent`). New stage value is current modern (write-time enforcement). `oldStage` could be legacy if the row was historical, but that's the actual historical truth — informational.
|
||||
- **L-009 search FTS on stages** — `interests` has no FTS GIN index at all (per agent #2's SC-04 finding); migration 0057 covers only clients/yachts/residential_clients. Stage searchability via FTS is moot. (SC-04 fix should add interests FTS — when added, the GENERATED expression should use `stageLabelFor` for the stage column.)
|
||||
- **L-010 notifications** — `next-in-line-notify.service.ts:63-65` falls back to `i.pipelineStage.replace(/_/g, ' ')` when `STAGE_LABELS` lookup misses. STAGE_LABELS is the modern-only map; legacy values would render as "eoi signed" etc. Recommended switch to `stageLabelFor()` for legacy resilience, but: only fires for active interests where stage is modern, so functionally clean today.
|
||||
- **L-011 CSV importers** — Only import services are `berth-import.ts` and `document-import.ts`; neither references `pipelineStage`. No CSV stage-import path exists, so no risk of legacy value re-entry through this vector.
|
||||
26
docs/audit-findings-tmp/01c-legacy-adjacent-enums.md
Normal file
26
docs/audit-findings-tmp/01c-legacy-adjacent-enums.md
Normal file
@@ -0,0 +1,26 @@
|
||||
# L-013-020 Adjacent Enum Drift — agent #14 (re-dispatch slice 3)
|
||||
|
||||
**Headline:** Single medium finding (tenure type enum diverges between berths and reservations); all other enums consistent.
|
||||
|
||||
**Counts:** 0 critical · 0 high · 1 medium
|
||||
|
||||
---
|
||||
|
||||
## 🟡 MEDIUM L-018: Tenure type enum diverges between berths and reservations
|
||||
|
||||
- **Files:** `src/lib/db/schema/berths.ts:65` vs `src/lib/db/schema/reservations.ts:32`
|
||||
- **What:** `berths.tenureType` documents `'permanent' | 'fixed_term' | 'fee_simple' | 'strata_lot'` (4 values). `reservations.tenureType` documents `'permanent' | 'fixed_term' | 'seasonal'` (3 values). Same column name, divergent allowed values.
|
||||
- **Why it matters:** No writes indicate actual cross-table conflict yet, but the schema-comment mismatch is a trap — a future feature copying tenure between the two tables would silently accept invalid values for the receiving side.
|
||||
- **Suggested fix:** Pick a single canonical enum (likely `'permanent' | 'fixed_term' | 'fee_simple' | 'strata_lot' | 'seasonal'` as the union) and update both schemas + comments. Or rename one column to disambiguate intent.
|
||||
|
||||
---
|
||||
|
||||
## ✅ Passing checks
|
||||
|
||||
- L-013 berth status `available/under_offer/sold` — only writes are in `berth-rules-engine.ts` respecting the 3-value set
|
||||
- L-014 statusOverrideMode — `manual/automated/null`; migration 0066 normalizes legacy `'auto'` → NULL; only writers in rules-engine + reconcile-queue both respect three-state
|
||||
- L-015 outcome — `won/lost_other_marina/lost_unqualified/lost_no_response/cancelled`; only writes in `interest-outcome.service.ts`; no legacy `'completed'` outcome anywhere
|
||||
- L-016 lead category — `general_interest/specific_qualified/hot_lead`; no out-of-set writes
|
||||
- L-017 lead source — `website/manual/referral/broker`; no out-of-set writes
|
||||
- L-019 doc status (`eoiDocStatus`, `reservationDocStatus`, `contractDocStatus`) — `pending/sent/signed/declined/voided`; mark-externally-signed only writes `'signed'`; Documenso webhook routes all status updates through services consistent with the set
|
||||
- L-020 reservation/contract status — `pending/active/ended/cancelled`; only writes in `reservation-state-machine.ts`
|
||||
105
docs/audit-findings-tmp/02-multitenancy-schema.md
Normal file
105
docs/audit-findings-tmp/02-multitenancy-schema.md
Normal file
@@ -0,0 +1,105 @@
|
||||
# Multi-tenancy + Schema Audit (MT-01-11, SC-01-15) — agent #2
|
||||
|
||||
**Headline:** API port isolation structurally sound, but 5 write paths do port check in JS without re-asserting portId in WHERE (TOCTOU gaps). Schema has several FKs that are `ON DELETE NO ACTION` in DB while nullable Drizzle declarations imply SET NULL — most critically `documents.clientId` and all `berthReservations` FKs.
|
||||
|
||||
**Counts:** 0 critical · 1 high · 8 medium · 0 low.
|
||||
|
||||
---
|
||||
|
||||
## 🟠 HIGH SC-02: Multiple significant FKs missing `onDelete` — remain `ON DELETE NO ACTION`
|
||||
|
||||
- **Files:**
|
||||
- `src/lib/db/schema/interests.ts:29,32` — `interests.portId`, `interests.clientId`
|
||||
- `src/lib/db/schema/documents.ts:72,85,86` — `documents.clientId`, `documents.fileId`, `documents.signedFileId`
|
||||
- `src/lib/db/schema/reservations.ts:18,24,25,27,28,33` — all 6 `berthReservations` FKs
|
||||
- `src/lib/db/schema/operations.ts:25` — `reminders.clientId`
|
||||
- `src/lib/db/schema/financial.ts:120` — `invoices.pdfFileId`
|
||||
- `src/lib/db/schema/documents.ts:176` — `documentEvents.signerId`
|
||||
- **What:** `.references(...)` without `{ onDelete: ... }` emits `ON DELETE NO ACTION`. Confirmed in migration 0000:841 (`interests_client_id_clients_id_fk ... ON DELETE no action`).
|
||||
- **Why it matters:** Hard-deleting a parent (client, berth, yacht, file) blocks at FK level. `client-hard-delete.service.ts` manually nullifies but `berthReservations` (4 NO ACTION FKs) is not in the chain. Future maintenance trap.
|
||||
- **Suggested fix:** Add `{ onDelete: 'set null' }` for nullable FKs that should tolerate parent deletion; explicit `{ onDelete: 'restrict' }` for those that intentionally block (e.g., `interests.clientId` — design intent is archive-first).
|
||||
|
||||
## 🟡 MEDIUM MT-01: `updateDefinition` UPDATE uses only `id` in WHERE, not `and(id, portId)`
|
||||
|
||||
- **File:** `src/lib/services/custom-fields.service.ts:136-145`
|
||||
- **What:** Guard read uses `and(eq(id, fieldId), eq(portId, portId))`, but UPDATE fires with only `eq(customFieldDefinitions.id, fieldId)`.
|
||||
- **Why it matters:** TOCTOU race between read check and write.
|
||||
- **Suggested fix:** Mirror `updateTag`/`deleteTag`: add `and(eq(...id), eq(...portId, portId))` to the UPDATE WHERE.
|
||||
|
||||
## 🟡 MEDIUM MT-01: `notes.service.ts` UPDATE/DELETE missing entityId scope
|
||||
|
||||
- **File:** `src/lib/services/notes.service.ts:846-850, 869-873, 897-901`
|
||||
- **What:** All note `update()` branches verify ownership via prior SELECT, then UPDATE/DELETE on `eq(...notes.id, noteId)` alone (no `eq(yachtNotes.yachtId, entityId)` etc).
|
||||
- **Why it matters:** TOCTOU gap; risk currently low (UUIDs, no cross-entity discovery surface).
|
||||
- **Suggested fix:** Add `eq(...notes.<parent>Id, entityId)` to each UPDATE/DELETE WHERE.
|
||||
|
||||
## 🟡 MEDIUM MT-01: `clients.service.ts::updateContact` / `removeContact` UPDATE/DELETE use only `contactId`
|
||||
|
||||
- **File:** `src/lib/services/clients.service.ts:737-741, 764`
|
||||
- **What:** PortId verified in JS only; mutation has no portId guard.
|
||||
- **Suggested fix:** Add `eq(clientContacts.clientId, clientId)` to the UPDATE/DELETE WHERE.
|
||||
|
||||
## 🟡 MEDIUM MT-04: `notes.service.ts::listForYachtAggregated` ownerClientId lookup has no portId guard
|
||||
|
||||
- **File:** `src/lib/services/notes.service.ts:276-283`
|
||||
- **What:** Owner client SELECT uses only `eq(clients.id, ownerClientId)`. Yacht is verified in port but cross-port ownerClientId would still surface.
|
||||
- **Suggested fix:** Add `eq(clients.portId, portId)`.
|
||||
|
||||
## 🟡 MEDIUM MT-06: `webhooks.service.ts::getWebhook` / `updateWebhook` / `deleteWebhook` fetch by `id` only, portId checked in JS
|
||||
|
||||
- **File:** `src/lib/services/webhooks.service.ts:103-108, 133-137, 170-174`
|
||||
- **What:** Fetches full webhook row (incl. encrypted secret) before JS port check.
|
||||
- **Why it matters:** Defense-in-depth gap — secret briefly in app memory before authz check.
|
||||
- **Suggested fix:** Move portId into `findFirst` WHERE.
|
||||
|
||||
## 🟡 MEDIUM SC-01: Migration 0000 (and 0001-0023) uses bare CREATE/ALTER without IF NOT EXISTS
|
||||
|
||||
- **File:** `src/lib/db/migrations/0000_narrow_longshot.sql`
|
||||
- **What:** No `IF NOT EXISTS` guards on CREATE TABLE/INDEX. Migration 0036 also bare `ALTER TABLE ... ADD CONSTRAINT`. Later migrations (0042, 0050, 0051, 0052, 0057, 0062, 0065) use IF NOT EXISTS / DO blocks correctly.
|
||||
- **Why it matters:** Drizzle tracker prevents double-runs in normal flow, but disaster-recovery partial replay would fail.
|
||||
- **Suggested fix:** Document that 0000-0036 are not re-runnable without dropping schema first; standardize on IF NOT EXISTS / DO block pattern for all new migrations.
|
||||
|
||||
## 🟡 MEDIUM SC-03: `companies` table missing soft-delete partial index for `archivedAt`
|
||||
|
||||
- **File:** `src/lib/db/schema/companies.ts:39-45`
|
||||
- **What:** Other entities (clients, interests, yachts, berths, residentialClients, residentialInterests) have `idx_*_archived ... WHERE archived_at IS NULL` partial indexes (migration 0046). Companies missing.
|
||||
- **Suggested fix:** `CREATE INDEX IF NOT EXISTS idx_companies_archived ON companies (port_id) WHERE archived_at IS NULL;`
|
||||
|
||||
## 🟡 MEDIUM SC-04: FTS GIN indexes missing for `interests` and `berths`
|
||||
|
||||
- **File:** `src/lib/db/migrations/0057_search_fts_indexes.sql`
|
||||
- **What:** Migration 0057 creates GIN indexes for clients/yachts/residentialClients but explicitly notes companies uses ILIKE. Interests and berths also lack GIN indexes.
|
||||
- **Suggested fix:** `CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_interests_fulltext ON interests USING gin (...)` and similar for berths.
|
||||
|
||||
## 🟡 MEDIUM SC-08: `audit_logs.searchText` declared as plain column in Drizzle but is GENERATED ALWAYS in DB
|
||||
|
||||
- **File:** `src/lib/db/schema/system.ts:53-54`
|
||||
- **What:** Drizzle `tsvector('search_text')` without generated annotation. If any service auto-includes this column in an UPDATE, it errors on the generated column. `audit_logs` is insert-only so likely not hit in practice, but schema-DB mismatch.
|
||||
- **Suggested fix:** Annotate as non-updateable or add a generated-column marker.
|
||||
|
||||
## 🟡 MEDIUM SC-09: `documents.clientId` Drizzle nullable but DB is `ON DELETE NO ACTION`
|
||||
|
||||
- **File:** `src/lib/db/schema/documents.ts:72`, migration `0000_narrow_longshot.sql:814`
|
||||
- **What:** Drizzle says nullable (intent: SET NULL on parent delete); DB constraint is NO ACTION (blocks delete). Migration 0042 fixed `documents.interestId/yachtId/companyId` but missed `clientId`.
|
||||
- **Why it matters:** Client hard-delete fails unless service explicitly nulls `documents.clientId` first.
|
||||
- **Suggested fix:** Migration to mirror what 0059 did for `files.client_id` — drop and re-add FK with `ON DELETE SET NULL`.
|
||||
|
||||
---
|
||||
|
||||
## ✅ Passing checks
|
||||
|
||||
- MT-01 clean: clients/interests/invoices/documents/files/tags/companies/berth-reservations GET/PATCH/DELETE all use `and(id, portId)` SQL filter; notes-service `verifyParentBelongsToPort` correct
|
||||
- MT-04 document-folders.service.ts clean (`listTree`, `createFolder`, `renameFolder`, `moveFolder`, `deleteFolderSoftRescue` all apply `eq(documentFolders.portId, portId)`)
|
||||
- MT-05 audit.service.ts `listAuditLogs` filters by portId first
|
||||
- MT-07 settings.service.ts clean (port-specific then global fallback by design)
|
||||
- MT-08 tags.service.ts clean
|
||||
- MT-09 custom-fields read/create/delete clean (only update missed; covered above)
|
||||
- MT-11 seed.ts idempotent (`SELECT count(*) FROM companies WHERE port_id = $1` early-exit)
|
||||
- SC-02 interestBerths.berthId/interestId, files.clientId/yachtId/companyId, documents.interestId/yachtId/companyId/reservationId all have explicit onDelete
|
||||
- SC-05 doc folder sibling-name unique, entity-folder partial unique, isPrimary partial unique all present
|
||||
- SC-06 idx_brochures_default partial unique present
|
||||
- SC-07 chk_system_folder_shape present (tightened by migration 0052)
|
||||
- SC-12 Migration 0062 normalizes legacy stages, 0066 normalizes statusOverrideMode='auto' → NULL
|
||||
- SC-13 Currency code stored as text + app-level validation (consistent)
|
||||
- SC-14 Address components stored as ISO 3166-2/alpha-2 text columns (consistent)
|
||||
- SC-15 Polymorphic owner reads use service helpers (eoi-context.ts, interests.service.ts, berth-reservations.service.ts); raw column reads only in JOIN conditions
|
||||
68
docs/audit-findings-tmp/03-routes-auth.md
Normal file
68
docs/audit-findings-tmp/03-routes-auth.md
Normal file
@@ -0,0 +1,68 @@
|
||||
# Routes/Middleware/Auth Audit (R-016-029, S-09-13, S-17-19) — agent #3
|
||||
|
||||
**Headline:** 1 critical (`/setup` unreachable on fresh DB — middleware redirect loop), 3 high (post-login `?redirect=` ignored; CRM invite token in query string leaks to access logs; missing `Retry-After` on sign-in 429), 2 medium (broad portal allowlist, no OPTIONS handlers), 13 clean.
|
||||
|
||||
**Counts:** 1 critical · 3 high · 2 medium · 0 low · 13 passing
|
||||
|
||||
---
|
||||
|
||||
## 🔴 CRITICAL R-021: `/setup` missing from `PUBLIC_PATHS` — bootstrap unreachable on fresh DB
|
||||
|
||||
- **File:** `src/proxy.ts:51-73`
|
||||
- **What:** `PUBLIC_PATHS` includes `/api/v1/bootstrap/` but NOT `/setup`. Comment at lines 60-62 says login + setup pages call bootstrap status, but `/setup` itself is not exempt from the session guard. Unauthenticated user → `/setup` → middleware redirects to `/login?redirect=/setup`. Login useEffect fetches bootstrap status, calls `router.replace('/setup')` → middleware again → infinite redirect loop.
|
||||
- **Why it matters:** Fresh deployment (no super admin) is functionally deadlocked. First operator cannot reach setup without already having a session (impossible on fresh DB).
|
||||
- **Suggested fix:** Add `'/setup'` to `PUBLIC_PATHS`. `POST /api/v1/bootstrap/super-admin` already self-protects with `hasAnySuperAdmin()`.
|
||||
|
||||
## 🟠 HIGH R-017/018: CRM post-login redirect ignores `?redirect=` — deep links silently dropped
|
||||
|
||||
- **File:** `src/app/(auth)/login/page.tsx:79`
|
||||
- **What:** Middleware redirects unauthenticated → `/login?redirect=<path>`. Login page never reads `useSearchParams()`; always `router.push('/dashboard')`.
|
||||
- **Why it matters:** Email/bookmark/shared deep links into specific clients/interests silently dump to dashboard after login.
|
||||
- **Suggested fix:** Read `searchParams.get('redirect')`, validate same-origin (starts with `/`, not `//`), use as push target if valid.
|
||||
|
||||
## 🟠 HIGH R-023: CRM invite token in query string leaks to access logs
|
||||
|
||||
- **File:** `src/lib/services/crm-invite.service.ts:71,233`
|
||||
- **What:** `${env.APP_URL}/set-password?token=${raw}` — raw 32-byte token in query param. Set-password page reads via `useSearchParams()`. Portal flow was migrated to `#token=` fragment in 2026-05-14 specifically to keep tokens out of logs/Referer; CRM invite path missed the migration.
|
||||
- **Why it matters:** Every nginx/Caddy access log line for `GET /set-password?token=<raw>` persists token to disk. Forwarded to SIEM/S3/monitoring → token visible to anyone with log access. Token grants account creation.
|
||||
- **Suggested fix:** Change `createCrmInvite` + `resendCrmInvite` to emit `${env.APP_URL}/set-password#token=${encodeURIComponent(raw)}`. Update `set-password/page.tsx` to use the fragment-reading pattern from `PasswordSetForm` (`readTokenFromUrl()`) with `?token=` back-compat for outstanding tokens.
|
||||
|
||||
## 🟠 HIGH R-029: `sign-in-by-identifier` 429 missing `Retry-After`
|
||||
|
||||
- **File:** `src/app/api/auth/sign-in-by-identifier/route.ts:47-51`
|
||||
- **What:** Builds 429 response with `headers: rateLimitHeaders(rl)` which only emits `X-RateLimit-Limit/Remaining/Reset` (`src/lib/rate-limit.ts:79-85`). `enforcePublicRateLimit` adds `Retry-After`; this route uses `checkRateLimit` directly and skips it.
|
||||
- **Why it matters:** RFC 6585 §4 requires `Retry-After` on 429. Automated clients can't back off correctly. Inconsistent with other public endpoints.
|
||||
- **Suggested fix:** Add `'Retry-After': Math.max(1, Math.ceil((rl.resetAt - Date.now()) / 1000)).toString()`.
|
||||
|
||||
## 🟡 MEDIUM R-016: `/portal/` blanket allowlist removes middleware as backstop
|
||||
|
||||
- **File:** `src/proxy.ts:65`
|
||||
- **What:** `'/portal/'` in `PUBLIC_PATHS` — every `/portal/*` is exempt from middleware session check. Per-page `getPortalSession()` is the only gate.
|
||||
- **Why it matters:** Defense-in-depth gap. Per-page checks all in place today; but a future portal page added without `getPortalSession()` has no middleware backstop. Fragile vs CRM's primary middleware gate.
|
||||
- **Suggested fix:** Allowlist only the unauthenticated portal routes individually (`/portal/login`, `/portal/activate`, `/portal/reset-password`, `/portal/forgot-password`). Add middleware portal-cookie check.
|
||||
|
||||
## 🟡 MEDIUM R-028: No explicit `OPTIONS` handlers, no CORS headers
|
||||
|
||||
- **File:** All `route.ts` files under `src/app/api/`
|
||||
- **What:** No `OPTIONS` exports. No `Access-Control-Allow-*` headers anywhere. Next.js will 405 on unhandled OPTIONS.
|
||||
- **Why it matters:** Acceptable for same-origin CRM. Becomes an issue if marketing-site browser JS calls `/api/public/berths` cross-origin.
|
||||
- **Suggested fix:** Defer until cross-origin consumer exists. When marketing site lives, add explicit `Access-Control-Allow-Origin: <marketing-domain>` to public routes (not wildcard).
|
||||
|
||||
---
|
||||
|
||||
## ✅ Passing checks
|
||||
|
||||
- R-016 allow-list anchor — `startsWith('/api/public/')` correctly rejects `'/api/publicX-evil'` (no regex anchor concern)
|
||||
- S-09 open redirect on next/redirect — CRM login ignores param (no risk because unused); portal `safeNextPath()` (portal/login/page.tsx:20-27) rejects non-`/portal/` paths and `//`-protocol-relative
|
||||
- S-10 CSRF — defense-in-depth: `proxy.ts originAllowed()` (lines 104-122) rejects state-changing `/api/v1/**` where Origin/Referer don't match in prod; better-auth has its own origin check for `/api/auth/**`; dev bypass intentional
|
||||
- S-11 cookie flags — CRM: `httpOnly`, `secure` (prod), `sameSite: 'strict'` (`src/lib/auth/index.ts:107-110`); Portal: `httpOnly`, `secure` (prod), `sameSite: 'lax'` (`src/app/api/portal/auth/sign-in/route.ts:43-45`)
|
||||
- S-12 CSP — per-request nonce-based CSP via `proxy.ts:buildCspWithNonce()` for page routes in prod (`'nonce-<n>' 'strict-dynamic'`); fallback CSP in `next.config.ts:55-66`; `frame-ancestors: 'none'` + `X-Frame-Options: DENY`; HSTS, X-Content-Type-Options, Referrer-Policy, Permissions-Policy all present
|
||||
- S-13 CORS — no `Access-Control-Allow-Origin: *` anywhere (correct for same-origin CRM)
|
||||
- R-019/020 portal `client_portal_enabled` gate — `src/app/(portal)/layout.tsx:22` calls `isPortalDisabledGlobally()`; per-page `getPortalSession()` additionally guards
|
||||
- R-022 reset-password tokens — Portal: single-use `consumeToken` setting `usedAt`, 30min TTL, SHA-256 hashed in DB. Better-auth CRM: 1h TTL, `revokeSessionsOnPasswordReset: true`
|
||||
- R-023 portal half — `portal/activate/page.tsx` uses `PasswordSetForm` with `useSyncExternalStore + readTokenFromUrl()` reading `window.location.hash` client-side; SSR-safe via `null` server snapshot
|
||||
- R-025 public berths cache headers `s-maxage=300, stale-while-revalidate=60` confirmed in both list + single endpoints
|
||||
- R-026/027 public health: anonymous `{status,timestamp}` only never 503; `X-Intake-Secret` `timingSafeEqual` (lines 57-64); authenticated runs DB+Redis dep checks in parallel, 503 on either failure
|
||||
- S-17 session fixation — better-auth creates fresh session row on every sign-in; portal sign-in always issues new JWT via `createPortalToken`
|
||||
- S-18 token expiry/refresh — CRM 24h absolute, 6h sliding refresh window (`src/lib/auth/index.ts:99-103`); Portal JWT 24h checked against `passwordChangedAt` watermark per request
|
||||
- S-19 audit log tamper-resistance — `audit_logs` has no `updated_at`; no `UPDATE` calls in app code (only INSERT/SELECT and time-based retention DELETE bounded by `AUDIT_LOGS_RETENTION_DAYS`)
|
||||
92
docs/audit-findings-tmp/04-audit-log.md
Normal file
92
docs/audit-findings-tmp/04-audit-log.md
Normal file
@@ -0,0 +1,92 @@
|
||||
# Audit Log Audit (AU-01-14) — agent #4
|
||||
|
||||
**Headline:** Core write path solid; major mutations all audit; mask helper covers expected PII; FTS indexed; AU-11 fix complete. Two HIGH issues: encrypted credential ciphertext bypasses masking (key is `"value"`) and `toggleAccount` mutation is silent.
|
||||
|
||||
**Counts:** 0 critical · 2 high · 4 medium · 4 low
|
||||
|
||||
---
|
||||
|
||||
## 🟠 HIGH AU-01a: `toggleAccount` writes no audit row
|
||||
|
||||
- **File:** `src/lib/services/email-accounts.service.ts:86-116`
|
||||
- **What:** Sets `isActive` on email account with no `createAuditLog` call. `connectAccount` (line 70) and `disconnectAccount` (line 139) do, but enable/disable in between is silent.
|
||||
- **Why it matters:** Silently disabling an email account suppresses bounce-detection or reroutes replies — compliance gap on a security-relevant config change.
|
||||
- **Suggested fix:** Add `void createAuditLog({ action: 'update', entityType: 'email_account', entityId: accountId, newValue: { isActive: data.isActive }, ... })` inside `toggleAccount`.
|
||||
|
||||
## 🟠 HIGH AU-02: Encrypted credential ciphertext stored in audit log without masking
|
||||
|
||||
- **File:** `src/lib/services/settings.service.ts:66-76` + `src/lib/services/sales-email-config.service.ts:281-299`
|
||||
- **What:** `updateSalesEmailConfig` calls `upsertSetting('sales_smtp_pass_encrypted', <ciphertext>, portId, meta)`. `upsertSetting` records `newValue: { value: '<ciphertext>' }`. `maskSensitiveFields` checks JSON keys against `SENSITIVE_KEY_FRAGMENTS`; the wrapping key `"value"` isn't in the list. Ciphertext lands verbatim in `audit_logs.new_value`.
|
||||
- **Why it matters:** Audit log is readable by all admins with `admin.view_audit_log`. DB read access exfils ciphertext; if `EMAIL_CREDENTIAL_KEY` is ever compromised, the historical audit log becomes a credential store. Industry standard: store only `credentialUpdated: true` for credential changes.
|
||||
- **Suggested fix:** In `upsertSetting`, detect when key ends with `_encrypted` (or accept `redactValue?: boolean` flag) and record `newValue: { value: '[redacted]' }`.
|
||||
|
||||
## 🟡 MEDIUM AU-03: FTS `search_text` covers only 4 fields; placeholder text misleads
|
||||
|
||||
- **File:** `src/lib/db/migrations/0014_black_banshee.sql:47-55` + `src/components/admin/audit/audit-log-list.tsx:360`
|
||||
- **What:** `search_text` GENERATED ALWAYS = `action || entity_type || entity_id || user_id`. Search input placeholder reads "entity id, action, vendor…" — implies you can search inside `metadata`/`new_value`. Searching "vendor" returns zero rows silently.
|
||||
- **Suggested fix:** Change placeholder to "action name, entity id, user id…" OR add `metadata` to GENERATED expression with `jsonb_to_tsvector` (larger index).
|
||||
|
||||
## 🟡 MEDIUM AU-08: Admin audit log shows field names but no old→new diff
|
||||
|
||||
- **File:** `src/components/admin/audit/audit-log-list.tsx:290-305` + `src/components/admin/audit/audit-log-card.tsx:84-91`
|
||||
- **What:** "Changes" column renders `Object.keys(newValue).slice(0,3).join(', ')` — no old→new diff, no row-expand. Dashboard `activity-feed.tsx` has working `buildDiffLine()` with 3 diff shapes, unused here.
|
||||
- **Why it matters:** Compliance audits can't confirm before/after state from UI alone; admins must dig into raw JSON.
|
||||
- **Suggested fix:** Add row-expand or detail sheet using `buildDiffLine` from activity-feed.tsx.
|
||||
|
||||
## 🟠 AU-10: Cascade-archived interests produce no individual audit rows
|
||||
|
||||
- **File:** `src/lib/services/clients.service.ts:578-618`
|
||||
- **What:** `archiveClient` batch-archives open interests, writes ONE `entityType: 'client'` row with `newValue: { cascadedInterestIds: [...] }`. No per-interest rows. `search_text` doesn't include `new_value`, so searching for an interest ID returns nothing.
|
||||
- **Why it matters:** Auditor querying for a specific archived interest sees no archive event; must know to look at parent client row.
|
||||
- **Suggested fix:** Loop over `archivedInterestIds` and emit per-interest `createAuditLog({ action: 'archive', entityType: 'interest', entityId, metadata: { cascadeSource: 'client_archive', clientId } })` (fire-and-forget).
|
||||
|
||||
## 🟡 MEDIUM AU-12: No audit log CSV export endpoint
|
||||
|
||||
- **File:** (absent — no `src/app/api/v1/admin/audit/export/route.ts`)
|
||||
- **What:** No download button, no API. Expenses domain has reference impl at `src/app/api/v1/expenses/export/csv/route.ts`.
|
||||
- **Why it matters:** GDPR / marina licensing audits often require exports.
|
||||
- **Suggested fix:** `GET /api/v1/admin/audit/export/csv` reusing `searchAuditLogs` + filter params.
|
||||
|
||||
## 🟡 MEDIUM AU-13: Outcome change uses `action: 'update'`, not distinct verb
|
||||
|
||||
- **File:** `src/lib/services/interests.service.ts:1047-1058`
|
||||
- **What:** `setInterestOutcome`/`clearInterestOutcome` log `action: 'update'` with `metadata.type: 'outcome_set'/'outcome_cleared'`. No `outcome_change` in `AuditAction` or filter dropdown. `metadata.type` not in `search_text` — FTS can't isolate.
|
||||
- **Suggested fix:** Add `'outcome_change'` to `AuditAction` union; use in both functions; add to dropdown; add to `DEFAULT_SEVERITY_BY_ACTION` as `'warning'`.
|
||||
|
||||
## 🟢 LOW AU-14: Tier map sparse; new actions default to 'info'
|
||||
|
||||
- **File:** `src/lib/audit.ts:220-222`
|
||||
- **What:** Only 2 entries (`permission_denied: 'warning'`, `hard_delete: 'critical'`). `password_change`, `portal_activate`, `revoke_invite`, `branding.logo.uploaded`, `rule_evaluated` all default to `'info'`. Severity≥warning filter misses security-relevant events.
|
||||
- **Suggested fix:** Add `password_change/portal_activate/revoke_invite: 'warning'`. `reconcile_manual` is in `metadata.type` — add `severity: 'warning'` at the call site in `berths.service.ts`.
|
||||
|
||||
## 🟢 LOW AU-14b: Action filter dropdown missing 12 verbs
|
||||
|
||||
- **File:** `src/components/admin/audit/audit-log-list.tsx:393-415`
|
||||
- **What:** Dropdown has 20 actions; missing `branding.logo.*`, `rule_evaluated`, `revoke/resend_invite`, `request/send_gdpr_export`, `password_change`, `portal_invite/activate/password_reset_request/password_reset`. Free-text partially compensates.
|
||||
- **Suggested fix:** Add missing action verbs.
|
||||
|
||||
## 🟢 LOW AU-14c: Entity-type filter missing several domains
|
||||
|
||||
- **File:** `src/components/admin/audit/audit-log-list.tsx:88-102`
|
||||
- **What:** Missing `document_folder`, `file`, `company`, `yacht`, `email_account`, `audit_log`, `backup_job`. Free-text on `entity_type` (in tsvector) works; dropdown is convenience.
|
||||
- **Suggested fix:** Add missing entity types.
|
||||
|
||||
## 🟢 LOW AU-14d: Dead code — `listAuditLogs` (ILIKE) in `audit.service.ts`
|
||||
|
||||
- **File:** `src/lib/services/audit.service.ts`
|
||||
- **What:** `listAuditLogs` exported but zero import sites. Admin route uses `searchAuditLogs` exclusively. ILIKE search is dead.
|
||||
- **Why it matters:** Future dev might wire it up bypassing GIN index → seq scans at scale.
|
||||
- **Suggested fix:** Delete `audit.service.ts` or mark `@deprecated`.
|
||||
|
||||
---
|
||||
|
||||
## ✅ Passing
|
||||
|
||||
- AU-01 (10 sampled mutating endpoints all audit: clients/interests/companies/berths/documents/folders/tags/roles/settings/files create + update + archive)
|
||||
- AU-02 password/token fragment masking covers `password`, `passwordHash`, `token`, `secret`, `api_key`, `apikey`, `auth`, `cookie`, `credentials` recursively up to depth 4. `email-accounts.service.ts` correctly logs only `metadata: { emailAddress, provider }`; `credentialsEnc` stripped before any JSON serialization.
|
||||
- AU-04 action filter wired (exact `eq()` filter)
|
||||
- AU-05 entity-type filter wired (same path)
|
||||
- AU-06 user filter wired (UUID exact match)
|
||||
- AU-07 date-range filter (ISO strings → Date → gte/lte; UI validates inversion)
|
||||
- AU-09 reconcile_manual tag in metadata at `berths.service.ts:473`
|
||||
- AU-11 permission_denied feed filter at `src/components/dashboard/activity-feed.tsx:185-189` (`i.action !== 'permission_denied'`); admin page correctly displays them with `'bg-red-800'` badge
|
||||
52
docs/audit-findings-tmp/05-documents-files.md
Normal file
52
docs/audit-findings-tmp/05-documents-files.md
Normal file
@@ -0,0 +1,52 @@
|
||||
# Documents/Files Audit (D-01-22) — agent #5
|
||||
|
||||
**Headline:** Structurally solid across all 22 checks. One medium real-time event mismatch + 2 low documentation divergences.
|
||||
|
||||
**Counts:** 0 critical · 0 high · 1 medium · 2 low · 19 passing
|
||||
|
||||
---
|
||||
|
||||
## 🟡 MEDIUM D-01/02/03: Real-time invalidation event name mismatch after upload
|
||||
|
||||
- **File:** `src/components/documents/documents-hub.tsx:141`
|
||||
- **What:** Hub subscribes to `'file:created': [['files']]`, but emitter (`files.ts:128`) and socket-events type def (`events.ts:264`) use `'file:uploaded'`.
|
||||
- **Why it matters:** After remote upload (other session, webhook auto-deposit), hub Files sections don't auto-refresh. Local `FolderDropZone` upload bypasses this via direct `queryClient.invalidateQueries`, but remote uploads invisible until reload.
|
||||
- **Suggested fix:** Change line 141 to `'file:uploaded': [['files']]` to match `client-files-tab.tsx:32`, `company-files-tab.tsx:32`, `interest-documents-tab.tsx:62`.
|
||||
|
||||
## 🟢 LOW D-13: HubRootView has 2 sections, not 3
|
||||
|
||||
- **File:** `src/components/documents/hub-root-view.tsx:50-100`
|
||||
- **What:** Spec says 3 cards; component renders 2 ("Signing in progress" + "Recent files"). Doc-only.
|
||||
- **Suggested fix:** Update CLAUDE.md to "2 sections."
|
||||
|
||||
## 🟢 LOW D-16: `interest.yachtId` branch in chain doc spec doesn't exist in code
|
||||
|
||||
- **File:** `src/lib/services/documents.service.ts:1225-1251`
|
||||
- **What:** Spec is `doc.clientId ?? .companyId ?? .yachtId ?? interest.clientId ?? interest.yachtId`. Code stops at `interest.clientId` because `interests.clientId` is NOT NULL — so the yachtId fallback is unreachable. Comment line 1239 explains.
|
||||
- **Suggested fix:** Update CLAUDE.md to drop the unreachable trailing branch, or annotate with `// unreachable: interests.clientId is NOT NULL`.
|
||||
|
||||
---
|
||||
|
||||
## ✅ Passing checks
|
||||
|
||||
- D-01 A16 fix verified — `formStr()` returns `undefined` (not `null`) for absent FormData fields; root upload omits `folderId` correctly
|
||||
- D-02 entity-folder drag-drop carries `folderId`+`entityType`+`entityId`+typed FK
|
||||
- D-03 file picker dialog passes `folderId` (null for root) correctly
|
||||
- D-04 PDF inline preview via `PdfViewer` lazy-loaded
|
||||
- D-05 image inline preview + lightbox via `<img>` for jpeg/png/gif/webp
|
||||
- D-06 Word/Excel: `FileGrid` gates "Preview" with `PREVIEWABLE_MIMES.has(...)` so only "Download" shows; `FilePreviewDialog` never opened
|
||||
- D-07 download endpoint wraps with `withPermission('files', 'view', ...)`; `getFileById` enforces port via `file.portId !== portId`
|
||||
- D-08 `deleteFolderSoftRescue` (`src/lib/services/document-folders.service.ts:294-337`) wrapped in `db.transaction()`, re-parents folders + documents + files explicitly (no CASCADE)
|
||||
- D-09 `syncEntityFolderName` called in updateClient (clients.service.ts:554), updateCompany (companies.service.ts:187), updateYacht (yachts.service.ts:167)
|
||||
- D-10 `moveFolder` cycle prevention: rejects self at line 213, `pg_advisory_xact_lock` per port (line 233), walks ancestor chain with `seen` set, checks `cursor === folderId` at each step
|
||||
- D-11 `assertNotSystemManaged` called in renameFolder (line 172), moveFolder (line 217), deleteFolderSoftRescue (line 299)
|
||||
- D-12 `listFilesAggregatedByEntity` walks Client↔Companies (via companyMemberships INNER JOIN companies on portId)↔Yachts; cap 20 + total
|
||||
- D-14 EntityFolderView uses `useAggregatedWorkflows` (filters to INFLIGHT_STATUSES `['draft','sent','partially_signed']`); files with `signedFromDocumentId` show "View signing details"
|
||||
- D-15 `GET /api/v1/documents/[id]/signing-details` returns `{ data: { workflow, signers, events } }`; `getDocumentById` enforces portId
|
||||
- D-16 idempotency: outer gate `doc.status === 'completed' && doc.signedFileId` returns; inner `SELECT ... FOR UPDATE` re-check inside transaction
|
||||
- D-17 Defense-in-depth port at every join: `companies` INNER JOIN with `portId` (line 451), `clients` INNER JOIN with `portId` (line 497), `yachts/files` WHERE portId everywhere, LEFT JOIN `documents` with `or(eq(documents.portId, portId), isNull(documents.id))` (line 588-590). companyMemberships has no portId column but is port-scoped via INNER JOIN to companies/clients
|
||||
- D-18 `?folder=<uuid>` URL state — three-state (absent → undefined hub root, `=root` → null, `=<uuid>` → uuid); `decodeFolderParam`/`encodeFolderParam` symmetric; deep folder works
|
||||
- D-19 `ensureEntityFolder` race-safety: fast-path re-SELECT before insert; two distinct catch branches for `uniq_document_folders_entity` (re-SELECT winner) and `uniq_document_folders_sibling_name` (increment suffix)
|
||||
- D-20 magic-byte: `bufferMatchesMime` in files.ts:58 covers 8 MIME types in-server; presign-PUT only used by berth-pdf/brochure (both stream first 5 bytes + `isPdfMagic()`)
|
||||
- D-21 filename HTML-escape (`document-sends.service.ts:415-422`)
|
||||
- D-22 `streamAttachmentOrLink` size-threshold + 24h presigned URL fallback; `fallbackToLinkReason: 'size_above_threshold'` audited
|
||||
30
docs/audit-findings-tmp/06-security.md
Normal file
30
docs/audit-findings-tmp/06-security.md
Normal file
@@ -0,0 +1,30 @@
|
||||
# Security Audit (S-01-08, S-21-30) — agent #6
|
||||
|
||||
**Headline:** 1 medium finding (S-23 plaintext S3 access key ID), 19 clean.
|
||||
|
||||
## 🟡 MEDIUM S-23: S3 access key ID stored plaintext in `system_settings`
|
||||
|
||||
- **File:** `src/lib/storage/index.ts:136`, `src/components/admin/storage-admin-panel.tsx:80`
|
||||
- **What:** S3 secret key (`storage_s3_secret_key_encrypted`) is AES-encrypted, but the access key ID (`storage_s3_access_key`) is stored/read as plaintext in `system_settings`.
|
||||
- **Why it matters:** Asymmetric encryption — DB exfil exposes the IAM key ID, narrowing the attack surface for credential stuffing or confirming which IAM principal to target. The access key ID is also surfaced in admin settings API responses.
|
||||
- **Suggested fix:** Apply same `encrypt()` / `*IsSet` pattern as the secret key. Migration to re-key existing rows. Update `resolveConfig` to call `decryptIfPresent`.
|
||||
|
||||
## ✅ Passing checks
|
||||
|
||||
- S-01 XSS via client.fullName (React text node)
|
||||
- S-02 XSS via tag.name (React child, sanitized style object)
|
||||
- S-03 XSS via note.content (plain text, no markdown rendering — `whitespace-pre-wrap` is CSS only)
|
||||
- S-04 XSS via email body markdown (`src/lib/utils/markdown-email.ts` escape-then-allowlist + DOMPurify second layer in `send-document-dialog.tsx`)
|
||||
- S-05 SQL injection via search query (Drizzle parameterized; `sql.raw` only on hardcoded constants in `admin/storage/route.ts:30` and `storage/migrate.ts:149`)
|
||||
- S-06 Path traversal in folder name (DB-only, never used as filesystem path)
|
||||
- S-07 Path traversal in file name / storage key (`validateStorageKey` in `src/lib/storage/filesystem.ts:49-69` rejects `..`/absolute/empty/non-allowlist chars; `resolveKey` does `path.resolve` prefix check)
|
||||
- S-08 SSRF via webhook target URL (two-layer: `isLocalOrPrivateHost` in `src/lib/validators/webhooks.ts` blocks RFC1918+loopback+link-local+CGNAT+cloud metadata; `resolveAndCheckHost` in `src/lib/queue/workers/webhooks.ts` re-resolves DNS at dispatch — DNS rebinding-resistant)
|
||||
- S-21 SMTP credential AES-256-GCM with random IV (`src/lib/utils/encryption.ts`)
|
||||
- S-22 IMAP credential same path as SMTP
|
||||
- S-24 Privilege escalation blocked: `updateUser` in `src/lib/services/users.service.ts:294-318` does caller-superset check; permission-overrides at `src/app/api/v1/admin/users/[id]/permission-overrides/route.ts:203-210` enforce per-leaf + block self-target at line 160; role definition mutations require `requireSuperAdmin` not just `manage_users`
|
||||
- S-25 Direct ID enumeration immune (`crypto.randomUUID` everywhere)
|
||||
- S-26 Audit log read-back of own permission denials — clean (admin-only `view_audit_log`)
|
||||
- S-27 Magic-byte verification verified
|
||||
- S-28 Filename HTML-escape in download links (`src/lib/services/document-sends.service.ts:415-420`)
|
||||
- S-29 Bounce-monitor email subject parsing — clean (no IMAP bounce worker exists yet; `email-threads.service.ts` uses parameterized `ilike` for subject matching)
|
||||
- S-30 `EMAIL_REDIRECT_TO` enforced at boot via Zod `superRefine` in `src/lib/env.ts:110-117` — production with the env set causes `process.exit(1)`. Webhook worker also short-circuits to `dead_letter` when set.
|
||||
112
docs/audit-findings-tmp/07-email-integrations.md
Normal file
112
docs/audit-findings-tmp/07-email-integrations.md
Normal file
@@ -0,0 +1,112 @@
|
||||
# Email + Integrations Audit (EM-01-19, IN-01-29) — agent #7
|
||||
|
||||
**Headline:** Broadly well-implemented. Primary issue: missing SMTP timeouts on sales transporter (HIGH — risks worker starvation). Plus 8 medium gaps in portal-email portId scoping, digest catalog key, receipt scanner config, presign TTL.
|
||||
|
||||
**Counts:** 0 critical · 1 high · 8 medium · 0 low · 30 passing
|
||||
|
||||
---
|
||||
|
||||
## 🟠 HIGH EM-XX: Sales transporter missing SMTP timeouts
|
||||
|
||||
- **File:** `src/lib/services/sales-email-config.service.ts:331-337`
|
||||
- **What:** `createSalesTransporter` builds nodemailer transport with no timeout options. Compare `createTransporter` in `src/lib/email/index.ts:26-37` which uses `SMTP_TIMEOUTS = { connectionTimeout: 10_000, greetingTimeout: 10_000, socketTimeout: 30_000 }`.
|
||||
- **Why it matters:** Hung SMTP relay can stall send-out indefinitely. Email queue concurrency=5, maxAttempts=5. Without socket timeouts, one stuck TCP connection holds a worker for nodemailer's 2-min default × 5 retries = 10min/job × 5 slots = whole pool blocked for 10min by a single flaky send.
|
||||
- **Suggested fix:** Apply `SMTP_TIMEOUTS` constant to `nodemailer.createTransport` in `createSalesTransporter`.
|
||||
|
||||
## 🟡 MEDIUM EM-05a: Per-port branding not threaded into portal activation/reset emails
|
||||
|
||||
- **File:** `src/lib/services/portal-auth.service.ts:163-164`
|
||||
- **What:** `issueActivationToken` and `issuePasswordReset` call `sendEmail(email, subject, html, undefined, text)` without the 6th `portId` argument. Without `portId`, `createTransporter()` uses global env SMTP. Branding is threaded into HTML via `getBrandingShell(portId)` but the SMTP transport falls back to global.
|
||||
- **Why it matters:** Multi-port deploys: portal auth emails for port B go through global env SMTP, defeating per-port SMTP override.
|
||||
- **Suggested fix:** Pass `portId` as 6th arg to `sendEmail` in both `issueActivationToken` and the reset send.
|
||||
|
||||
## 🟡 MEDIUM EM-07: CC/BCC not supported in main `sendEmail`
|
||||
|
||||
- **File:** `src/lib/email/index.ts:54-68`
|
||||
- **What:** `SendEmailOptions` lacks `cc`/`bcc`. Sales send-out path also lacks them.
|
||||
- **Suggested fix:** Add optional `cc`/`bcc` to `SendEmailOptions`. Low urgency.
|
||||
|
||||
## 🟡 MEDIUM EM-11: Bounce-to-interest linking not implemented
|
||||
|
||||
- **File:** `src/lib/services/sales-email-config.service.ts:13` (header comment)
|
||||
- **What:** `getSalesImapConfig` exposes IMAP creds but no BullMQ worker reads IMAP. Failed deliveries don't update `document_sends.failedAt`.
|
||||
- **Suggested fix:** Wire BullMQ recurring job using imapflow to scan inbox for bounce NDRs, match against `document_sends.messageId`. Phase 7 §14.9 deferred.
|
||||
|
||||
## 🟡 MEDIUM EM-16: Notification digest uses wrong catalog key for subject resolution
|
||||
|
||||
- **File:** `src/lib/services/notification-digest.service.ts:161-169`
|
||||
- **What:** Calls `resolveSubject` with `key: 'crm_invite' as any` because `'notification_digest'` is not in `TEMPLATE_KEYS` in `src/lib/email/template-catalog.ts`.
|
||||
- **Why it matters:** Admin-set CRM invite subject override bleeds into digest emails.
|
||||
- **Suggested fix:** Add `'notification_digest'` to `TEMPLATE_KEYS`; update digest service to use it.
|
||||
|
||||
## 🟡 MEDIUM IN-11: Presigned URL TTL fixed at 900s for portal downloads
|
||||
|
||||
- **File:** `src/lib/storage/index.ts:240-254` (`presignDownloadUrl`); `src/lib/services/portal.service.ts:350` (`getDocumentDownloadUrl`)
|
||||
- **What:** `presignDownloadUrl` defaults `expirySeconds=900` (15min). Sales send-out correctly overrides to 24h. `getDocumentDownloadUrl` calls without expiry → 15min default.
|
||||
- **Why it matters:** Portal users opening their doc list and clicking after >15min get 403.
|
||||
- **Suggested fix:** Pass `expirySeconds: 4 * 3600` for portal download links, or sign on-demand from API.
|
||||
|
||||
## 🟡 MEDIUM IN-21: OpenAI receipt-scanner module-level instantiation, no credential health check
|
||||
|
||||
- **File:** `src/lib/services/receipt-scanner.ts:4`
|
||||
- **What:** `const openai = new OpenAI();` at module level reads `OPENAI_API_KEY` at import. SDK throws on first call when unset; catch returns zero-confidence empty result. No admin-visible health check.
|
||||
- **Suggested fix:** Guard `OPENAI_API_KEY` upfront with clear error. Add a health-check endpoint similar to `checkDocumensoHealth`.
|
||||
|
||||
## 🟡 MEDIUM IN-23: Receipt OCR ignores per-port config; hardcoded `gpt-4o`
|
||||
|
||||
- **File:** `src/lib/services/receipt-scanner.ts:19`
|
||||
- **What:** `model: 'gpt-4o'` hardcoded; per-port `getResolvedOcrConfig` not consulted; `aiEnabled` flag does nothing. Module-level singleton OpenAI client.
|
||||
- **Suggested fix:** Accept `portId`, call `getResolvedOcrConfig(portId)`, check `aiEnabled`, use `config.apiKey` and `config.model`. Branch on provider for OpenAI vs Anthropic.
|
||||
|
||||
## 🟡 MEDIUM IN-24: Stale "pdfme" references in comments/seed
|
||||
|
||||
- **File:** `src/lib/db/seed-data.ts:807`, `src/lib/services/document-templates.ts:573`
|
||||
- **What:** Comments still reference pdfme even though the rendering path was removed; `tiptap-validation.ts:8` confirms pdfme retired. `document-templates.ts:648-652` throws ValidationError for non-EOI templates.
|
||||
- **Suggested fix:** Update comments to reference pdf-lib AcroForm fill; remove "pdfme" from seed-data description.
|
||||
|
||||
## 🟡 MEDIUM IN-29: Umami `testConnection` throws instead of returning typed result
|
||||
|
||||
- **File:** `src/lib/services/umami.service.ts:80-101, 292`
|
||||
- **What:** `loadUmamiConfig` returns null gracefully; all public APIs return null when unconfigured. But `testConnection` throws `CodedError('UMAMI_NOT_CONFIGURED')` instead of returning `{ ok: false, error }` like `checkDocumensoHealth`.
|
||||
- **Suggested fix:** Return `{ ok: false, error: string }` to match Documenso convention.
|
||||
|
||||
---
|
||||
|
||||
## ✅ Passing checks
|
||||
|
||||
- EM-01 per-port SMTP override (`getPortEmailConfig` in `port-config.ts:136`)
|
||||
- EM-02/03 default send-froms cascade (explicit `from` → `cfg.fromAddress` → env.SMTP_FROM → `noreply@${SMTP_HOST}`)
|
||||
- EM-04 EMAIL_REDIRECT_TO subject prefix `[redirected from <orig>]`; documenso-client also applies `applyRecipientRedirect`/`applyPayloadRedirect`; env.ts:110 prod boot guard
|
||||
- EM-05 branded shell (`renderShell` in `src/lib/email/shell.ts:37`)
|
||||
- EM-06 reply-to override applied
|
||||
- EM-08 send rate limit 50/user/hour Redis sliding-window keyed `${portId}:${userId}`
|
||||
- EM-09 `streamAttachmentOrLink` threshold + filename HTML-escape pre-SMTP
|
||||
- EM-10 IMAP probe script + `getSalesImapConfig` AES-256-GCM decrypted
|
||||
- EM-12 `document_sends` audit row in success + failure branches
|
||||
- EM-13 portal activation token: 32-byte token, hash stored in `portalAuthTokens`, `#token=...` fragment to stay out of logs
|
||||
- EM-14/15 reset/invite emails wired
|
||||
- EM-17 EOI sent via Documenso (not as nodemailer attachment)
|
||||
- EM-18/19 `renderEmailBody` escape-first + `isSafeHref` (https/mailto only) + `MERGE_VALUE_ESCAPE_MAP` neutralizes markdown chars
|
||||
- IN-01 v1 template-generate path (`generateDocumentFromTemplate`)
|
||||
- IN-02 v2 envelope/create multipart (FormData with `payload` JSON + `files` Blob)
|
||||
- IN-03 v2 distribute returns `recipients[].signingUrl` in one round-trip
|
||||
- IN-04 redistribute version-aware (v2 caveat: `recipientIds` may not target single recipient — API behavior risk, not code bug)
|
||||
- IN-05 downloadSignedPdf version-aware
|
||||
- IN-06 voidDocument version-aware (idempotent on 404)
|
||||
- IN-07 placeFields v2 bulk `field/create-many` percent coords + `fieldMeta`; v1 one POST per field with pixel coords
|
||||
- IN-08 `normalizeDocument` `id ?? documentId` for both docs and recipients (handles legacy `r.Recipient` capital-R)
|
||||
- IN-09 NocoDB `pg_advisory_xact_lock` + skip rows where `updated_at > last_imported_at`
|
||||
- IN-10 S3Backend with SSE AES256, all calls wrapped in `withTimeout(30_000)`, never imports MinIO directly
|
||||
- IN-12 filesystem MULTI_NODE_DEPLOYMENT guard (boot-time throw)
|
||||
- IN-13 BullMQ exponential backoff: email/docs 5×1s, webhooks 8×30s
|
||||
- IN-14 Redis noeviction in both compose files
|
||||
- IN-15 `src/worker.ts` imports all 10 workers + SIGTERM/SIGINT graceful shutdown
|
||||
- IN-16 public berths cache `s-maxage=300, stale-while-revalidate=60`
|
||||
- IN-17 status filter Sold > Under Offer (status OR has active is_specific_interest with isNull(end_date)+outcome) > Available
|
||||
- IN-18 mooring regex `^[A-Z]+\d+$` checked pre-DB; returns 400 for malformed
|
||||
- IN-19/20 dual-mode health endpoint with `timingSafeEqual`
|
||||
- IN-22 berth-pdf-parser tier-2 is `unpdf` (not Tesseract — prior comment correction); 30s timeout
|
||||
- IN-25 `fillEoiFormFields` flatten + metadata; missing fields warn rather than throw
|
||||
- IN-26 VALID_MERGE_TOKENS allow-list including `{{eoi.berthRange}}`
|
||||
- IN-27 `formatBerthRange` handles all cases (single/contig/non-contig/cross-pontoon/dedup)
|
||||
- IN-28 portal magic-link rate-limited 10/h/IP via `enforcePublicRateLimit(req, 'portalToken')`
|
||||
55
docs/audit-findings-tmp/08-perf-behavioral.md
Normal file
55
docs/audit-findings-tmp/08-perf-behavioral.md
Normal file
@@ -0,0 +1,55 @@
|
||||
# Performance + Behavioral Audit (P-05/09/13/14, B-01-22) — agent #8
|
||||
|
||||
**Headline:** 1 critical (B-01 INNER JOIN drops hard-deleted berth links), 1 high (B-16 AppShell remount destroys form state), 1 medium (P-09a leading-wildcard ILIKE), 17 clean.
|
||||
|
||||
**Counts:** 1 critical · 1 high · 1 medium · 1 low · 17 passing
|
||||
|
||||
---
|
||||
|
||||
## 🔴 CRITICAL B-01: Hard-deleted berth causes silent data loss across interest surfaces
|
||||
|
||||
- **File:** `src/lib/services/interest-berths.service.ts:55` (`getPrimaryBerth`), `:87` (`getPrimaryBerthsForInterests`), `:140` (`listBerthsForInterest`)
|
||||
- **What:** All three helpers use `INNER JOIN berths ON berths.id = interestBerths.berthId`. When a berth is hard-deleted, the INNER JOIN silently drops the link.
|
||||
- **Why it matters:** Interest detail page shows `berthId: null`, `berthMooringNumber: null`. Kanban card shows no berth chip. EOI generation produces empty field. `archiveInterest` path that calls `getPrimaryBerth` before evaluating berth rule returns null and **skips the rule entirely**.
|
||||
- **Suggested fix:** Change all three `INNER JOIN` to `LEFT JOIN berths`. Callers already handle `null` mooringNumber. Add service-layer guard preventing hard-delete of berths with `interest_berths` rows (require unlink or soft-archive first).
|
||||
|
||||
## 🟠 HIGH B-16: AppShell remounts children on breakpoint crossing, destroying form state
|
||||
|
||||
- **File:** `src/components/layout/app-shell.tsx:58-70`
|
||||
- **What:** When `isMobile` flips on resize, the shell switches between `<MobileLayout>{children}</MobileLayout>` and the desktop `<div>...{children}...</div>`. React unmounts and remounts `children`, destroying any in-progress `useState` form drafts including `InlineEditableField`.
|
||||
- **Why it matters:** A user editing a client name on desktop who resizes past the mobile breakpoint loses unsaved draft text. Multi-step modal forms (reconcile wizard) open during resize get unmounted.
|
||||
- **Suggested fix:** Wrap shared content with stable `key`, or use CSS-only responsive layout so the children subtree never remounts. Alternatively `key={isMobile ? 'mobile' : 'desktop'}` only on the shell wrappers with `children` stable via Portal.
|
||||
|
||||
## 🟡 MEDIUM P-09a: Leading-wildcard ILIKE in `buildListQuery` prevents index use
|
||||
|
||||
- **File:** `src/lib/db/query-builder.ts`
|
||||
- **What:** List search uses `ILIKE '%term%'` with leading wildcard, defeating B-tree and trigram-prefix indexes.
|
||||
- **Why it matters:** Sequential scan on high-cardinality text columns; degrades at scale.
|
||||
- **Suggested fix:** Migrate to `pg_trgm` GIN indexes on the searched columns, or move to FTS via existing `search_text` GIN where one exists.
|
||||
|
||||
## 🟢 LOW P-14: List endpoint `limit` allows up to 1000 rows
|
||||
|
||||
- **File:** `src/lib/api/list-query.ts`
|
||||
- **What:** Generic list cap = 1000. Audit log is bounded to 200 with cursor pagination (better pattern).
|
||||
- **Why it matters:** A 1000-row response with relations can blow the 256 KB budget.
|
||||
- **Suggested fix:** Lower default cap to ~100; require explicit cursor pagination beyond.
|
||||
|
||||
---
|
||||
|
||||
## ✅ Passing checks
|
||||
|
||||
- P-05 No N+1 — all secondary fetches batched via `inArray`
|
||||
- P-13 Audit FTS uses `to_tsvector('simple')` + GIN index + `plainto_tsquery('simple')` consistently (`src/lib/services/audit-search.service.ts`, migration `0014_black_banshee.sql`)
|
||||
- B-02 Sara Laurent contract-without-yachtId renders correctly (overview tab guards yacht section; stage-gate only fires on `changeInterestStage`)
|
||||
- B-03 `activeInterestsWhere` (`src/lib/services/active-interest.ts`) used in listInterestsForBoard, getInterestStageCounts, listBerths reconcile, recommender CTE
|
||||
- B-04 / B-05 `formatBerthRange` correct: single (`A1`), contiguous (`A1-A3`), non-contiguous (`A1, A3`), cross-pontoon (`A1-A2, B5-B7`), dedup, non-canonical pass-through
|
||||
- B-07 Tier B fires only when `activeInterestCount===0 && lostCount>0`; `lost_count` aggregates `LIKE 'lost%' OR cancelled`; heat scoring gated by `tier === 'B'`; fall-through policy enforces cooldown/never_auto_recommend
|
||||
- B-08 `withPermission` (`src/lib/api/helpers.ts:328-340`) writes `permission_denied` audit row before 403 (fire-and-forget `void`)
|
||||
- B-09 Same-stage no-op `if (existing.pipelineStage === data.pipelineStage) return STAGE_NOOP;` early-returns before DB/audit/socket (`src/lib/services/interests.service.ts:847-849`)
|
||||
- B-10 Documenso webhook handles empty body / malformed JSON via try/catch returning `{ ok: false }` 200 + warning log (`src/app/api/webhooks/documenso/route.ts:176-182, 202`)
|
||||
- B-11 `status_override_mode` transitions (null/manual/automated) all have audit coverage; reconcile clears to null, rules engine writes 'automated', admin UI writes 'manual'
|
||||
- B-13 Catch-up wizard `pipelineStage === 'contract'` sends `outcome: 'won'` (`src/components/berths/catch-up-wizard.tsx:120`); reconcile route validates `z.enum(['won']).optional()`
|
||||
- B-17 Bulk-add berths wizard step state persists in `BulkAddBerthsWizard`'s `useState`; no remount between steps
|
||||
- B-18 NotesList handles 6 entity types (clients/interests/yachts/companies/residential_clients/residential_interests); `companyNotes.updatedAt` substituted via `createdAt` per CLAUDE.md
|
||||
- B-19 `InlineEditableField` present on client/yacht/company/interest/residential-client/residential-interest/berth tabs (11 files)
|
||||
- B-22 `markExternallySigned` (`src/lib/services/external-signing.service.ts:68-72`) updates `{ docStatus: 'signed', updatedAt: now }`. Note: catalog said "documentId=null, signedAt=now" but interests table has no such columns — the service is correct relative to schema.
|
||||
159
docs/audit-findings-tmp/09-ux-forms.md
Normal file
159
docs/audit-findings-tmp/09-ux-forms.md
Normal file
@@ -0,0 +1,159 @@
|
||||
# UX/Forms/Tables Audit (U-001-100, code-side) — agent #9
|
||||
|
||||
**Headline:** Generally consistent (Sheet, AlertDialog, EmptyState, requestId surfacing all good across most surfaces). 4 HIGH gaps: native `alert()` for bulk-action failures, icon-only buttons missing aria-label, unicode glyphs in portal, Vaul Drawer in mobile search overlay. Plus 14 MEDIUM gaps in form discipline + a11y + mobile nav.
|
||||
|
||||
**Counts:** 0 critical · 4 high · 14 medium · 0 low
|
||||
|
||||
---
|
||||
|
||||
## 🟠 HIGH
|
||||
|
||||
### U-059: Unicode glyphs as status icons in portal documents page
|
||||
|
||||
- **File:** `src/app/(portal)/portal/documents/page.tsx:85-89`
|
||||
- **What:** Signer status rendered as raw Unicode (`'✓'` signed, `'✗'` declined, `'○'` pending) inside colour-coded `<span>` with no `aria-label`.
|
||||
- **Why it matters:** A11y — screen readers read literal Unicode names. Per project memory: decorative unicode glyphs are explicitly flagged. `inline-stage-picker.tsx:443` comment confirms the pattern ("was ⚑ unicode glyph — replaced with a Lucide").
|
||||
- **Suggested fix:** Replace with `<CheckCircle2>` / `<XCircle>` / `<Circle>` Lucide icons + `aria-label`.
|
||||
|
||||
### U-066: Vaul Drawer used for mobile search overlay (violates Sheet doctrine)
|
||||
|
||||
- **File:** `src/components/search/mobile-search-overlay.tsx:6`
|
||||
- **What:** `import { Drawer as VaulDrawer } from 'vaul'` — search overlay is a full-screen overlay, not a bottom sheet, but uses Vaul Drawer. CLAUDE.md says Vaul is reserved for mobile-bottom-sheet only (currently `MoreSheet` only).
|
||||
- **Suggested fix:** Convert to `<Sheet side="bottom">` or `<Dialog>` fullscreen. Visualviewport handling (lines 50-89) becomes redundant once Radix dialog primitive backs it.
|
||||
|
||||
### U-076: Native `alert()` for bulk-action failure feedback in 3 lists
|
||||
|
||||
- **Files:** `src/components/interests/interest-list.tsx:146`, `src/components/companies/company-list.tsx:73`, `src/components/yachts/yacht-list.tsx:66`
|
||||
- **What:** Partial-failure feedback via `alert(...)`. `client-list.tsx:145` uses `toast.warning(...)` correctly.
|
||||
- **Why it matters:** Native alert blocks main thread, can't be styled, fires in tests without suppression.
|
||||
- **Suggested fix:** Replace with `toast.warning(...)` matching `client-list.tsx`.
|
||||
|
||||
### U-079: Icon-only buttons missing aria-label (5 sites)
|
||||
|
||||
- **Files:**
|
||||
- `src/components/notifications/notification-bell.tsx:65` (Bell icon button)
|
||||
- `src/components/files/file-grid.tsx:121` (MoreHorizontal "…" on file cards)
|
||||
- `src/components/admin/forms/form-template-list.tsx:102` (Trash button)
|
||||
- `src/components/email/email-accounts-list.tsx:159` (Trash button)
|
||||
- `src/components/companies/company-members-tab.tsx:228` (MoreHorizontal)
|
||||
- **Pattern reference (correct):** `src/components/shared/folder-actions-menu.tsx:96` uses `<span className="sr-only">More folder actions</span>`.
|
||||
- **Suggested fix:** Add `aria-label` to each, following the folder-actions-menu sr-only pattern.
|
||||
|
||||
---
|
||||
|
||||
## 🟡 MEDIUM
|
||||
|
||||
### U-009: Audit log inline div instead of EmptyState component
|
||||
|
||||
- **File:** `src/components/admin/audit/audit-log-list.tsx:524`
|
||||
- **What:** `<div><p className="text-muted-foreground">No audit log entries found.</p></div>` rather than `<EmptyState title="..." />`.
|
||||
- **Suggested fix:** Replace with `<EmptyState title="No audit log entries found." />`.
|
||||
|
||||
### U-010: Two duplicate EmptyState components with incompatible APIs
|
||||
|
||||
- **Files:** `src/components/ui/empty-state.tsx` vs `src/components/shared/empty-state.tsx`
|
||||
- **What:** `ui/` accepts `{icon: ReactNode, body, actions}`; `shared/` accepts `{icon: ElementType, description, action: {label, onClick}}`. 3 files use `ui/` (admin/reconcile-queue, documents/documents-hub, reservations/reservation-detail), 24 use `shared/`.
|
||||
- **Suggested fix:** Pick `shared/` as canonical (8× usage); migrate the 3 `ui/` callers and delete `ui/empty-state`.
|
||||
|
||||
### U-021: Required-field marker inconsistent
|
||||
|
||||
- **Files:** `src/components/clients/client-form.tsx:273`, `src/components/interests/interest-form.tsx:281`
|
||||
- **What:** Some fields use inline `*`, others have no marker; no `aria-required` on inputs; no consistent pattern.
|
||||
- **Suggested fix:** Single pattern: `<Label>Field <span aria-hidden>*</span></Label>` + `aria-required="true"` on input.
|
||||
|
||||
### U-022: Help-text discoverability inconsistent
|
||||
|
||||
- **File:** `src/components/shared/filter-bar.tsx`, `src/components/clients/client-form.tsx`
|
||||
- **What:** No tooltip pattern; some fields have always-visible muted-foreground hints, some have nothing.
|
||||
- **Suggested fix:** Document a rule (always-visible for constraints/format hints; tooltips only for icons).
|
||||
|
||||
### U-024: Cancel/dismiss without unsaved-changes warning on ClientForm/YachtForm
|
||||
|
||||
- **Files:** `src/components/clients/client-form.tsx`, `src/components/yachts/yacht-form.tsx`
|
||||
- **What:** `InterestForm.requestClose()` (line 123) checks `isDirty` and shows discard AlertDialog; `CompanyForm` also has it. ClientForm and YachtForm don't — sheet closes immediately.
|
||||
- **Suggested fix:** Add `isDirty` guard + discard AlertDialog matching InterestForm pattern.
|
||||
|
||||
### U-031: FileUploadZone size limit not surfaced as client-side check
|
||||
|
||||
- **File:** `src/components/files/file-upload-zone.tsx:170`
|
||||
- **What:** Accept attribute lists extensions; "up to 50MB" copy at line 163; no client-side size check before upload. Server-side check fails silently with "Upload failed" at line 103.
|
||||
- **Suggested fix:** Wire client-side size check before upload; show clear "File too large" message.
|
||||
|
||||
### U-044: No jump-to-page input in pagination
|
||||
|
||||
- **File:** `src/components/shared/data-table.tsx:420`
|
||||
- **Suggested fix:** Add small `<input type="number">` between Previous/Next.
|
||||
|
||||
### U-048: No column resize/reorder on DataTable
|
||||
|
||||
- **File:** `src/components/shared/data-table.tsx`
|
||||
- **What:** Visibility supported via `ColumnPicker`; widths fixed; no drag-reorder.
|
||||
- **Suggested fix:** Opt-in `enableColumnResizing` per table via TanStack Table v8 `onColumnSizingChange`.
|
||||
|
||||
### U-069: Invoice delete uses custom overlay, not AlertDialog
|
||||
|
||||
- **File:** `src/app/(dashboard)/[portSlug]/invoices/page.tsx:167`
|
||||
- **What:** Hand-rolled `<div className="fixed inset-0 bg-background/80 backdrop-blur-xs z-50 ...">` rather than `<AlertDialog>` / `<ConfirmationDialog>`. Lacks focus trap, Escape, role="alertdialog".
|
||||
- **Suggested fix:** Replace with `<ConfirmationDialog>` matching pattern elsewhere.
|
||||
|
||||
### U-074: Success toast missing on ClientForm + InterestForm create/edit
|
||||
|
||||
- **Files:** `src/components/clients/client-form.tsx:215`, `src/components/interests/interest-form.tsx:235`
|
||||
- **What:** `onSuccess` invalidates queries + closes sheet, no `toast.success()`. `ComposeDialog.onSuccess:81` does fire one.
|
||||
- **Suggested fix:** `toast.success(isEdit ? 'Client updated' : 'Client created')`.
|
||||
|
||||
### U-080: Logo preview `<img alt="">` should describe state
|
||||
|
||||
- **File:** `src/components/admin/shared/settings-form-card.tsx:420`
|
||||
- **Suggested fix:** Use `alt="Port logo preview"` or dynamic from field label.
|
||||
|
||||
### U-081: Heading hierarchy inconsistent within tab components
|
||||
|
||||
- **Files:** `email-accounts-list.tsx:114`, `interest-contract-tab.tsx:130/251/291/364` (h2 → h3 → h2 jumps)
|
||||
- **Suggested fix:** Audit each tab; standardize h2 = primary section, h3 = sub-section; never h2 after h3 at same nesting depth.
|
||||
|
||||
### U-086: DialogContent missing aria-describedby on minimal-content dialogs
|
||||
|
||||
- **File:** `src/components/email/compose-dialog.tsx:95` and ~40 other dialogs
|
||||
- **What:** Only `file-preview-dialog.tsx:82` explicitly suppresses the Radix warning.
|
||||
- **Suggested fix:** Add `<DialogDescription className="sr-only">...</DialogDescription>` or `aria-describedby={undefined}` to suppress.
|
||||
|
||||
### U-091: Mobile topbar title blank on list pages
|
||||
|
||||
- **Files:** `client-list.tsx`, `yacht-list.tsx`, `interest-list.tsx`, `berth-list.tsx`
|
||||
- **What:** `useMobileChrome` only called from detail pages. List pages leave topbar in fallback (no title, stale from previous detail page).
|
||||
- **Suggested fix:** Add `useMobileChrome({ title, showBackButton: false })` per list with cleanup pattern.
|
||||
|
||||
### U-093: Invoices missing from mobile navigation
|
||||
|
||||
- **File:** `src/components/layout/mobile/more-sheet.tsx:54`
|
||||
- **What:** Not in `MORE_GROUPS`, not in bottom tabs. Mobile users can only reach via direct URL.
|
||||
- **Suggested fix:** Add `{ label: 'Invoices', icon: FileText, segment: 'invoices' }` to Operations group.
|
||||
|
||||
---
|
||||
|
||||
## ✅ Sample passing checks
|
||||
|
||||
- U-001-008 list empty states + skeletons clean across clients/yachts/interests/berths/companies/reservations/invoices/email-threads
|
||||
- U-012 FileUploadZone drag-hover with `border-primary bg-primary/5`
|
||||
- U-023 field-level errors via react-hook-form `formState.errors` consistent
|
||||
- U-026 BulkAddBerthsWizard + CatchUpWizard persist state across step nav
|
||||
- U-027 phone E.164 via `formatAsYouType` emits `{ e164, country }`
|
||||
- U-029 native `<input type="date">` provides browser calendar + keyboard
|
||||
- U-033 Combobox keyboard nav inherited from Radix `<Command>` primitives
|
||||
- U-040 Sort indicators via `getSortIcon` (`ArrowUpDown`/`ArrowUp`/`ArrowDown`)
|
||||
- U-041/042 Filter chip dismiss + Clear-all in FilterBar
|
||||
- U-043 page size selector 25/50/100/250/All
|
||||
- U-049 virtual list via `@tanstack/react-virtual` (`virtual virtualHeightPx={640}` in audit log)
|
||||
- U-054 STAGE_BADGE in `src/lib/constants.ts:100` — 7 distinct stages with distinct Tailwind colour families
|
||||
- U-055 outcome badge: won=emerald, lost\_\*=rose, cancelled=slate
|
||||
- U-057 status-pill covers all required document statuses
|
||||
- U-060/061 button hierarchy + destructive red consistent
|
||||
- U-065 Sheet used for forms+previews on both desktop and mobile (23 components)
|
||||
- U-067 AlertDialog used for destructive confirmations (`useConfirmation`, `ArchiveConfirmDialog`, `ConfirmationDialog`, `BulkHardDeleteDialog`)
|
||||
- U-070-072 click-outside, Esc, focus-trap, focus-restore all inherited from Radix
|
||||
- U-073 toast position consistent (sonner top-right)
|
||||
- U-075 `toastError()` (`src/lib/api/toast-error.ts:43`) surfaces requestId + Copy ID action — used in 89 files
|
||||
- U-094 iOS safe-area-inset comprehensive (`pb-safe-bottom`, `pt-safe-top`, FAB `calc(env(safe-area-inset-bottom)+86px)`)
|
||||
- U-097 visualViewport handling on mobile-search-overlay
|
||||
- U-092 More sheet covers Documents/Interests/Yachts/Companies/Residential/Alerts/Reminders/Expenses/Reservations/Reports/Analytics/Settings/Admin
|
||||
Reference in New Issue
Block a user