diff --git a/docs/AUDIT-2026-05-12.md b/docs/AUDIT-2026-05-12.md new file mode 100644 index 00000000..89231780 --- /dev/null +++ b/docs/AUDIT-2026-05-12.md @@ -0,0 +1,5938 @@ +# Port Nimara CRM — Comprehensive Platform Audit + +**Generated:** 2026-05-12 (session run) +**Branch:** `feat/documents-folders` +**Method:** 19 parallel audit agents on Claude Opus 4.7, read-only static analysis. Each agent owned a single domain and wrote a CRITICAL/HIGH/MEDIUM-grouped report. This document consolidates the reports and overlays the fixes already shipped during the session. + +--- + +## How to read this document + +1. **Executive summary** lists every CRITICAL finding (must address before production), per domain. +2. **Already fixed in this session** is a manifest of the changes I shipped while the audit was running. Don't re-fix these. +3. **Cross-cutting priority queue** is the top ~15 highest-impact findings across the entire codebase, ordered. Tackle these first. +4. **Per-domain reports** below contain the full text of every agent's report verbatim — useful when you sit down to actually fix a specific area. +5. **Methodology + agent roster** appendix at the bottom lists who looked at what. + +Severity is the auditor's judgment, not mine — I have not re-graded findings. Treat anything tagged CRITICAL as a real block on shipping. + +--- + +## Executive summary + +### CRITICAL findings (must address) + +| # | Domain | File | Issue | Status | +|---|---|---|---|---| +| 1 | Security | `src/app/api/v1/admin/users/[id]/permission-overrides/route.ts` | Admins could grant themselves every permission leaf via self-target | **FIXED this session** | +| 2 | Security | `src/app/api/auth/resolve-identifier/route.ts` | Username enumeration via hit/miss response shape + no rate limit | **FIXED this session** | +| 3 | Services | `src/lib/services/users.service.ts` (admin email-change) | `account.accountId` not updated → user can't sign in with either old or new email after admin rotation; sessions also not revoked | **FIXED this session** | +| 4 | Observability | `src/lib/services/search-nav-catalog.ts` | 10 NAV_CATALOG entries pointed at routes that don't exist (`/admin/audit-log`, `/admin/error-events`, `/user-settings`, 7×`/settings/`) | **FIXED this session** | +| 5 | Auth flow | `src/middleware.ts` | Token-gated email confirm/cancel routes blocked by session 401 | **FIXED this session** | +| 6 | Email | `src/lib/env.ts` + `src/lib/email/index.ts` | `EMAIL_REDIRECT_TO` has no `NODE_ENV=production` guard — a stray prod env value silently funnels every email to one inbox | Open | +| 7 | Email | every template | URL interpolations into `href="…"` and link text are unescaped — a `"` in any URL breaks out, no scheme rejection | Open | +| 8 | Data model | `src/lib/db/migrations/0052_audit_critical_fixes.sql` | `CREATE INDEX CONCURRENTLY` silently never runs because there's no real `db:migrate` runner — six composite indexes missing in prod | Open | +| 9 | Data model | `db:push` flow | Two structural constraints (berths.current_pdf_version_id circular FK, system_settings NULLS NOT DISTINCT) not in `db:push`; fresh-deploy diverges from prod | Open | +| 10 | Services | `documents.service.ts: handleDocumentCompleted` | Orphan-blob window — failure between `storage.put` and `documents.update` leaves the blob and marks status='completed' with no `signedFileId` | Open | +| 11 | GDPR | `src/lib/services/gdpr-bundle-builder.ts` | Article-15 export missing portal_users, email_threads/messages, document_sends, reminders, files, scratchpadNotes, client_merge_log, contact_log, website_submissions, form_submissions | Open | +| 12 | GDPR | `src/lib/services/client-hard-delete.service.ts` | "Right to be forgotten" doesn't actually erase — verbatim PII survives in email_messages.body_html, files, document_sends.recipient_email forever | Open | +| 13 | GDPR | `src/app/api/auth/resolve-identifier/route.ts` (post-fix) | Still echoes the real canonical email on a successful username hit (rate-limited but enumerable) | Partial — see Open follow-ups | +| 14 | GDPR | `audit_logs.metadata` field | Not covered by `maskSensitiveFields`; raw PII (emails, IPs, names) accumulates unbounded with no retention cron | Open | +| 15 | Observability | `src/app/api/webhooks/documenso/route.ts` | Webhook handler bypasses the platform-error pipeline entirely — admin/errors silent on Documenso webhook crashes | Open | +| 16 | UI/UX | 16 sites use native `window.confirm()` | Bypasses `ConfirmationDialog` / `AlertDialog` for destructive flows (cancel signing, delete files, archive interest/company/yacht…) | Open | +| 17 | Documenso | `documenso-client.ts` v1↔v2 routing | (Pending full report) | In progress | +| 18 | Concurrency | (see report) | Various race windows on multi-rep edits + partial-unique-index inserts | Open | + +### HIGH-priority queue + +Listed after CRITICALs in the priority queue section below. + +--- + +## Already fixed in this session + +These changes are on the `feat/documents-folders` branch (post-commit `660553c` and onward). Do not re-fix. + +### Security +- **Self-target privilege escalation block** — `src/app/api/v1/admin/users/[id]/permission-overrides/route.ts` now refuses `PUT` when `targetUserId === ctx.userId`. Additionally, the body now sanitises against a canonical `ALLOWED_RESOURCE_ACTIONS` allow-list mirroring `RolePermissions`, so unknown resource/action keys are stripped before write. Cross-tenant pollution check added (refuses overrides for users without a `user_port_roles` row in the caller's port). +- **Username enumeration kill** — `src/app/api/auth/resolve-identifier/route.ts` now (a) shares the `auth` 5-per-15-min rate-limit bucket keyed by client IP, (b) returns a synthetic `@auth.invalid` email on miss so hit and miss are indistinguishable in shape. (Note: GDPR auditor flagged the hit-path still echoes a real canonical email — still an information leak that's worth a deeper redesign; see Open follow-ups.) +- **Email-change account/session rotation** — `src/lib/services/users.service.ts` now also updates `account.accountId` for the `credential` provider (Better Auth's actual login key) AND revokes every active `session` row when an admin rotates a user's email. Previously the user could not sign in with either old or new email after rotation. +- **Middleware unblocks token-gated email routes** — `src/middleware.ts` adds `/api/v1/me/email/confirm/` and `/api/v1/me/email/cancel/` to `PUBLIC_PATHS` so the confirm/cancel links work in a fresh browser without an existing session. + +### Search + navigation +- **NAV_CATALOG dead-link sweep** — `src/lib/services/search-nav-catalog.ts` corrected 10 entries that pointed to non-existent routes. `/admin/audit-log` → `/admin/audit`, `/admin/error-events` → `/admin/errors`, `/user-settings` → `/settings/profile`, and the 7 phantom `/settings/` entries redirected to their real `/admin/` homes. +- **Topbar global search extended** — every admin sub-card now indexed in `NAV_CATALOG` with curated `keywords` (client portal, ai scoring, pipeline weights, recommender heat weights, etc.). Results sort to the bottom of the cmd-K dropdown, beneath entity hits. +- **Admin sections page search** — `src/components/admin/admin-sections-browser.tsx` `AdminSection` gained a `keywords?: string[]` field, populated for System Settings (mirrors `KNOWN_SETTINGS`), AI configuration, OCR, Users, and Website analytics. `filteredMatches` haystack now includes those keywords. + +### User management +- **Disable / enable button** — third Power/PowerOff action button on the desktop user list + matching dropdown item on the mobile card. Backed by `userProfiles.isActive` (already enforced by `withAuth` → 403 on disabled accounts). +- **UserForm tabs + permissions matrix** — UserForm now wraps Profile & role + Permissions in tabs. New `UserPermissionMatrix` component renders the full `RolePermissions` shape with three-state per-leaf toggle (Inherit / Grant / Deny). The matrix is `role="radiogroup"` + `aria-checked` per option, and shows an amber callout explaining that overrides save on their own button. Dirty-state tracked via originalOverrides comparison. +- **First/last name + admin email change** — UserForm collects first + last name (canonical) alongside displayName. Email change behind an AlertDialog confirmation; on confirm sends an automated notice to the prior address (new template `src/lib/email/templates/admin-email-change.ts`). +- **Phone formatting** — UserForm swaps the bare tel input for the shared `PhoneInput` (country combobox + AsYouType + E.164 storage). + +### Optional username sign-in +- Migration `0054_user_profiles_username.sql` adds `username` column (2..30 chars, regex `^[a-z0-9._-]{2,30}$`, partial unique index on `LOWER(username)`). +- Login page now accepts email OR username via `/api/auth/resolve-identifier`. +- Self-service username card on `src/components/settings/user-settings.tsx`. +- `/api/v1/me` PATCH now accepts username with allow-list + reserved-name check + uniqueness check before write. + +### Per-user permission overrides +- Migration `0055_user_permission_overrides.sql` adds the table. +- Effective-permissions resolver in `src/lib/api/helpers.ts` now layers user overrides on top of role + port-role overrides + residential toggle. +- `GET / PUT /api/v1/admin/users/[id]/permission-overrides` endpoints. + +### Role + enum normalization +- `formatRole()` + `ROLE_LABELS` in `src/lib/constants.ts` — replaces the ad-hoc `humanizeRole` in `sidebar.tsx` and `prettifyRoleName` in `role-list.tsx`. user-list, user-card, role-list, user-form now render "Sales Agent" instead of "sales_agent". +- `formatOutcome()` + `OUTCOME_LABELS` for interest outcomes. Updated `client-columns.tsx`, `realtime-toasts.tsx`, `interest-detail-header.tsx`, `command-search.tsx`. +- Pipeline stage normalization extended to: `next-in-line-notify.service.ts`, `command-search.tsx` (interest + residential interest bucket), `yacht-tabs.tsx`, `interest-picker.tsx`, `ai.ts` worker email body, `pipeline-report.ts` + `revenue-report.ts` PDF generators. + +### Auto-memory +- Saved feedback memory: "Be thorough — audit everything that ends in a user-facing notification". (Memory subsystem is /Users/matt/.claude/projects/...) + +--- + +## Cross-cutting priority queue + +Tackle in this order. C-prefix = CRITICAL still open; H-prefix = HIGH. + +1. **[C] Wire a real `db:migrate` runner** — without it, `0052_audit_critical_fixes.sql` silently never creates 6 composite indexes (data-model C1). Recommended: a tsx script that reads migrations in order, splits on `--> statement-breakpoint`, runs `CREATE INDEX CONCURRENTLY` outside a tx, and tracks state in a `__drizzle_migrations` table. Same script gives you `db:migrate:status` for prod readiness. +2. **[C] Add `EMAIL_REDIRECT_TO` prod guard** — `src/lib/env.ts` should refine to reject when `NODE_ENV === 'production'`, and `src/lib/email/index.ts` should `logger.warn` at boot when set (not debug). 5 minutes of work, prevents an extremely-bad-day class of incident. +3. **[C] Fix orphan-blob window in `handleDocumentCompleted`** — `src/lib/services/documents.service.ts:1100-1253`. Wrap the storage.put + files.insert + documents.update sequence in a transaction or a saga with a compensating delete. The current catch-block path also incorrectly marks `status='completed'` with no `signedFileId`, hiding the failure from reps. +4. **[C] Escape URLs in email templates** — every template in `src/lib/email/templates/*` inlines `${data.link}` etc. into href/text without escaping. Move all template rendering through a shared `escapeUrl` helper and add scheme allow-listing (http(s) only). +5. **[C] Eliminate the 16 native `window.confirm()` calls** — each one is a destructive flow that bypasses `ConfirmationDialog` / `AlertDialog`. ui-ux-auditor lists the sites; high-leverage UX fix. +6. **[C] GDPR export completeness** — `gdpr-bundle-builder.ts` must include portal_users, email_threads/messages, document_sends, reminders, files, scratchpadNotes, client_merge_log, contact_log, website_submissions, form_submissions. This is a regulator-finding-level gap. +7. **[C] Right-to-be-forgotten actually erase** — `client-hard-delete.service.ts` currently nullifies FKs but leaves verbatim PII in email_messages.body_html, files, document_sends.recipient_email. Add a true wipe path (or document the limitation in the legal text and gate the feature behind a "we cannot fully erase X" warning). +8. **[C] Add `user_permission_overrides.user_id` FK + onDelete='set null' on nullable client refs** — data-model H1+H2. Migration 0056. +9. **[C] Resolve-identifier hit-path still leaks email** — replace the API entirely with a server-side signIn proxy that takes `{identifier, password}` and never returns the canonical email at all. Current rate-limited hit still echoes real emails to anyone with a guessable username. +10. **[H] Re-audit `audit_logs.metadata` masking** — extend `maskSensitiveFields` to cover `audit_logs.metadata`; add a 90-day retention cron (mirroring `error_events`). +11. **[H] Webhook → error pipeline** — `documenso/route.ts` should `captureErrorEvent` on handler crash. Apply the same to every other webhook route. +12. **[H] Wire admin email-template subject editor** — 5 of 8 templates ignore `overrides.subject`; admins see "Saved" with zero effect. `email-auditor` H1+H2. +13. **[H] Wire admin signature/footer fields** — `/admin/email` writes `email_signature_html` + `email_footer_html` which the shell never reads. Either delete or wire. +14. **[H] PII redaction in audit/error pipeline** — `error_events.request_body_excerpt` sanitizer redacts password/token but not email/phone/name/dob/address. +15. **[H] Notification email worker XSS** — `workers/notifications.ts:65-71` interpolates `notif.description` and `notif.link` into HTML unescaped. Apply `escapeHtml` + URL allow-list. + +--- + +## Per-domain reports + +Each section below is the agent's report verbatim. File:line refs reference the repo as it stands at the start of the audit session — some have already been addressed (see "Already fixed in this session" above). + +--- + +## 1. Security + API + auth audit (security-auditor + early api-security run) + +Two reports — the team-spawned `security-auditor` and an earlier standalone run. Both included verbatim. + +### Report A: security-auditor (team) + +# Security / API / Auth Audit — `feat/documents-folders` branch + +Read-only audit of the `pn-crm` repo. Scope: auth wrappers, tenant scoping, +public/webhook endpoints, the just-shipped username-resolve + permission- +overrides + admin email-change flows, CSRF posture, audit-log coverage. + +No **CRITICAL** issues found — auth helpers (`withAuth` / `withPermission` / +`requireSuperAdmin`) are applied consistently across `src/app/api/v1/**`, +public endpoints all use timing-safe secret compares + per-IP rate limits, +and the Documenso webhook idempotency + per-port secret resolution is sound. +The findings below are HIGH / MEDIUM. + +--- + +## HIGH + +### H1. `resolve-identifier` leaks username→email mapping AND has no rate limit +**File:** `src/app/api/auth/resolve-identifier/route.ts` (lines 25–58) + +The route's own docstring claims it "pairs with the global login-attempt +limiter" — but no `enforcePublicRateLimit` / `checkRateLimit` is actually +called in the handler. Unauthenticated attackers can POST `{identifier:"matt"}` +at unbounded volume; on a hit the response is `{email:"matt@letsbe.solutions"}`, +on a miss the response echoes the raw input. That makes existence +trivially decidable (response contains `@` ↔ hit), and on a hit the caller +*also* learns the actual email address. Usernames are typically far more +guessable than emails (first names, social handles), so this becomes a one- +way `username → email` harvester usable for downstream phishing / password +spraying. **Fix:** wrap with `enforcePublicRateLimit(req, 'portalSignIn', +identifier.toLowerCase())` (or a new `loginIdentifier` bucket) AND stop +echoing the resolved email — either return `{ok:true}` and require the +caller to POST `(username,password)` together to a single sign-in endpoint +that does the lookup server-side, or return an opaque short-lived token that +Better Auth's sign-in step can redeem internally. + +### H2. Admin email-change leaves `emailVerified` true → account takeover via reset +**File:** `src/lib/services/users.service.ts` (lines 233–262, 355–387) + +`updateUser` rotates `user.email` directly when an admin edits the address +(line 246–247) but never resets `emailVerified`. A hostile or compromised +admin can point any victim's account at an attacker-controlled mailbox, then +trigger the existing "forgot password" flow on the new address and silently +hijack the account; the existing `notifyAdminEmailChange` notice fires to +the *old* address fire-and-forget and is documented as non-blocking +("failure to send doesn't roll back"). There is *also* no `createAuditLog` +specifically for the email-change — the generic update audit at line 287 +buries the change inside `newValue: data` rather than emitting a dedicated +`email_change` action that monitoring can alert on. **Fix:** when +`wantsEmailChange`, set `emailVerified: false` in the Better Auth user +update, write a dedicated `severity: 'warning'` audit row with +`{oldEmail, newEmail, changedBy}`, and require the recipient to click the +existing `/api/v1/me/email/confirm/[token]` flow before the rotation +applies — i.e. mint a `user_email_changes` row rather than direct-UPDATE. + +### H3. Permission-overrides PUT accepts arbitrary keys → JSONB pollution + deep-merge surprise +**File:** `src/app/api/v1/admin/users/[id]/permission-overrides/route.ts` +(lines 31–35, 97–141) + +`updateOverridesSchema` is `z.record(z.string(), z.record(z.string(), z.boolean()))` — no allow-list against the known `RolePermissions` resource/action keys. An admin (or a stolen admin session) can persist arbitrary keys into `user_permission_overrides.permission_overrides`. Two concrete impacts: (a) future deep-merge logic that maps unknown keys into newly added resources promotes the rogue keys silently (silent privilege creep when new permissions ship); (b) the JSONB can be bloated to harm downstream readers. **Fix:** validate against `KNOWN_PERMISSION_LEAVES` derived from `RolePermissions` (resource → action set), reject unknown keys with `ValidationError`, and bound the merged blob size as `/api/v1/me/route.ts` already does for `preferences`. The GET handler is fine — it only reads what was already persisted. + +### H4. `/api/v1/me/email/confirm|cancel/[token]` is unreachable for logged-out users (middleware 401) +**File:** `src/app/api/v1/me/email/cancel/[token]/route.ts`, +`src/app/api/v1/me/email/confirm/[token]/route.ts`, +`src/middleware.ts` (PUBLIC_PATHS list, line 8–20) + +The handlers correctly skip `withAuth` ("the token IS the proof") but +`/api/v1/me/email/...` is not in `PUBLIC_PATHS`, so `middleware.ts` returns +a 401 JSON for any unauthenticated request — exactly the case a user +clicking the confirm link from email on a different device will hit. End +result: every confirm/cancel click from a logged-out browser fails with +"Authentication required". Also, the GET request applies an irreversible +state mutation with no CSRF guard (the origin-check in middleware only fires +for `STATE_CHANGING_METHODS`). **Fix:** move these handlers under +`/api/auth/email-change/{confirm,cancel}/[token]` so they're covered by the +`/api/auth/` PUBLIC_PATHS prefix, OR add `/api/v1/me/email/` to +`PUBLIC_PATHS`. Convert the GET mutation to a POST landing page (one-click +confirm form) so cross-site image/prefetch tags can't silently flip state. + +--- + +## MEDIUM + +### M1. Direct `Schema.parse(body)` instead of `parseBody(req, schema)` +**Files:** `src/app/api/v1/admin/custom-fields/[fieldId]/route.ts:18-19`, +`src/app/api/v1/search/route.ts:11`, +`src/app/api/v1/files/upload/route.ts:21`, +`src/app/api/v1/companies/[id]/members/[mid]/handlers.ts:29`, +`src/app/api/public/website-inquiries/route.ts:97-98`, +`src/app/api/public/residential-inquiries/route.ts:51-52`, +`src/app/api/public/interests/route.ts:47-48`, +`src/app/api/portal/auth/{sign-in,forgot-password,reset-password,activate,change-password}/route.ts`, +`src/app/api/auth/{set-password,resolve-identifier}/route.ts`. + +CLAUDE.md explicitly requires `parseBody` so the 400 envelope + field- +errors shape stays uniform (the frontend's `toastError` hook depends on +it). Most of these are caught by an outer try/catch that routes ZodError +into `errorResponse`, which masks the issue — but the response shape +diverges (a thrown ZodError becomes a generic 500 unless `errorResponse` +maps it). Admin route `custom-fields/[fieldId]` is the worst case: a +malformed PATCH body 500s instead of 400-with-field-errors. **Fix:** swap +to `parseBody(req, schema)` in the admin/internal routes; the portal / +public auth routes intentionally use `safeParse` + manual `ValidationError` +mapping and can be left as-is. + +### M2. CSRF origin check disabled in development +**File:** `src/middleware.ts` (line 80) + +`process.env.NODE_ENV !== 'development'` gates the origin check. If a +production deployment is ever booted with `NODE_ENV=development` +accidentally (shell export leakage, container override, "debug deploy"), +all CSRF defense-in-depth is silently off — SameSite=Lax still helps but +isn't enough for legacy browsers / extension contexts. **Fix:** key the +bypass on an explicit `DISABLE_CSRF_FOR_LAN=1` env var that's defaulted to +unset and refused in `lib/env.ts` when `NODE_ENV==='production'`. + +### M3. Permission-override audit log lacks severity escalation +**File:** `src/app/api/v1/admin/users/[id]/permission-overrides/route.ts:124-134` + +Changing user permission grants is exactly the action an attacker would +take after compromising an admin; the audit row should be emitted with +`severity:'warning'` (matching the `email_change_cancelled` precedent in +`src/app/api/v1/me/email/cancel/[token]/route.ts:46`) so the audit UI's +default filter surfaces it. Today it's a vanilla `action:'update'` lost in +the noise. + +### M4. `/api/public/interests` audit row stores client phone in `metadata` +**File:** `src/app/api/public/interests/route.ts:254-271` + +The audit row's `newValue` and surrounding `metadata` capture `ip` plus +foreign keys, which is fine, but `data.phone` is held in scope and could +easily slip in during a future edit. Today the row is OK; flag as a place +to add a regression test. (Not a finding to act on, just a watch-list item +for the broader audit team.) + +### M5. Filesystem storage proxy: token leak via Referer +**File:** `src/app/api/storage/[token]/route.ts:42-119` + +`Cache-Control: private, no-store` is set on the response, but the URL +itself (with the HMAC token in the path) leaks via the `Referer` header +when the downloaded asset is opened inside a browser tab that then +navigates to a third-party link. Single-use replay protection mitigates +reuse, but a token still-in-window is good for one stolen download. **Fix:** +either rotate to a POST-with-token-in-body form (breaks ``), +or set `Referrer-Policy: no-referrer` on the response and document that +issuers should mint with the shortest possible expiry. Lower-impact +because filesystem mode is single-tenant per the boot guard. + +### M6. `/api/v1/clients/bulk-hard-delete` lacks per-IP rate-limit +**File:** `src/app/api/v1/clients/bulk-hard-delete/route.ts` (no `withRateLimit`) + +The sibling `bulk-hard-delete-request/route.ts` is wrapped in `withRateLimit` +but the actual delete endpoint is not. A compromised admin session could +fan out hundreds of irrevocable hard-deletes in a tight loop with no +limiter to slow it down. **Fix:** add `withRateLimit('destructiveBulk', ...)` +or similar with a 5/minute cap; the existing audit row will still be +emitted, but the limiter caps the blast radius. + +--- + +## Verified clean (no finding) + +- `withAuth` / `withPermission` / `requireSuperAdmin` applied uniformly: + every `route.ts` under `src/app/api/v1/**` was checked; the only files + without the wrappers are `me/email/{confirm,cancel}/[token]/route.ts` + (covered by H4) which intentionally use bearer-token auth. +- `withAuth` enforces port-context via `X-Port-Id` header / preferences, + never from body (helpers.ts:160–168). +- Documenso webhook: timing-safe per-port secret resolution, replay guard + via `signatureHash` unique index, per-handler `portScope` forwarded so a + documensoId reused across ports can't cross-mutate. +- Public website-intake: timing-safe `verifySecret` with length-equal + buffer pad, refusal-by-default when `WEBSITE_INTAKE_SECRET` unset, per-IP + rate-limit, unknown port slug → generic 400 (no input echo). +- Raw `sql\`...\`` usage scanned across `src/lib/services` and + `src/app/api`: every interpolation is via Drizzle's parameter binding + (`sql\`... ${foo} ...\``); no string concatenation gaps found. +- Storage proxy upload (PUT) does HMAC verify + single-use replay + size cap + + PDF magic-byte enforcement before disk write. + +— security-auditor (read-only audit; no source files edited) + +### Report B: api-security (standalone earlier run) + +# API + Auth + Security Audit – Port Nimara CRM + +Scope: `src/app/api/**`, `src/lib/api/helpers.ts`, `src/lib/auth/**`, `src/middleware.ts`, +plus the newly-added permission-overrides and resolve-identifier flows. + +## CRITICAL + +### 1. Privilege escalation via `PUT /api/v1/admin/users/[id]/permission-overrides` +`src/app/api/v1/admin/users/[id]/permission-overrides/route.ts:97-141` + +The PUT handler gates only on `withPermission('admin', 'manage_users', …)` and never +verifies that `params.id !== ctx.userId`. Any user who holds `admin.manage_users` can +target their own userId and write a `userPermissionOverrides` row that grants every +leaf (`{ admin: { manage_users: true, manage_settings: true, … }, … }`). Because +`withAuth` deep-merges `userOverride.permissionOverrides` last in the chain +(`src/lib/api/helpers.ts:227-238`), the row wins over the base role and instantly +escalates the caller to admin-of-everything on the next request. The companion +`removeUserFromPort` service in `src/lib/services/users.service.ts:319` does have a +self-target guard — the same guard is missing here. Fix: in the PUT handler, throw +`ForbiddenError` when `targetUserId === ctx.userId && !ctx.isSuperAdmin`, and require +super-admin to flip `admin.*` leaves (or any leaf that the calling user cannot already +grant). Tier-2 fix: rotate this row to require super-admin outright; admin-of-port +shouldn't be able to mint persistent overrides for peers anyway. + +### 2. `/api/auth/resolve-identifier` has no rate-limit — username enumeration +`src/app/api/auth/resolve-identifier/route.ts:25-59` + +The endpoint is unauthenticated, sits behind `/api/auth/*` (so the middleware +origin check is skipped per `src/middleware.ts:46-49`), and does NO rate-limit / +throttling. The header comment claims it "pairs with the global login-attempt +limiter" but that limiter is only triggered when the *subsequent* sign-in call +runs — an attacker hitting just this endpoint with a wordlist is unconstrained. +While the response shape is the same on hit and miss (`{ email: }`), +the *content* differs: a hit returns an `@`-bearing email, a miss returns the +unchanged raw input. So with one HTTP call per candidate an attacker +deterministically learns which usernames map to real accounts; they then funnel +only the validated emails into the rate-limited sign-in flow, defeating the +per-account brute-force ceiling. Fix: wrap in `enforcePublicRateLimit(req, +'portalSignIn', normalized)` (or a new bucket like `usernameResolve` with ~10/15min +per-IP), and consider returning a constant fake-email when the username doesn't +resolve so hit/miss are indistinguishable at the response-body level too. + +## HIGH + +### 3. `permission-overrides` PUT does not validate the override shape +`src/app/api/v1/admin/users/[id]/permission-overrides/route.ts:31-34, 97-141` + +`updateOverridesSchema` is `z.record(z.string(), z.record(z.string(), z.boolean()))` — +any resource name and any action key is accepted. This stores garbage in +`user_permission_overrides.permission_overrides` forever, and silently typo'd +keys (`'clien_ts.view'`) won't take effect but won't 400 either. More +importantly, there is no allow-list against the `RolePermissions` shape defined +in `src/lib/db/schema/users.ts:6`, so a future code path that does +`Object.keys(permissions).forEach(…)` could be surprised by a foreign resource +appearing in the merged map. Fix: derive a Zod allow-list at module load from +the canonical `RolePermissions` shape (the same `VALID_MERGE_TOKENS` pattern the +templates code uses) and reject unknown resource/action keys with 400. + +### 4. `permission-overrides` PUT writes for users not assigned to the current port +`src/app/api/v1/admin/users/[id]/permission-overrides/route.ts:97-122` + +The PUT inserts/updates a `(userId, portId)` row without first verifying that +`targetUserId` actually has a `user_port_roles` row for `ctx.portId`. An admin at +port A can mint override rows for users belonging only to port B (the row is keyed +on the admin's portId, so it's a "future override that would activate if the user +ever joins this port"). Functionally inert today, but pollutes the override table +across tenants and breaks the implicit "you can only manage users in your port" +invariant the rest of the admin/users routes enforce. The GET path does the +implicit validation by failing the port-role lookup; the PUT should mirror it. +Fix: `findFirst` on `userPortRoles` with `(targetUserId, ctx.portId)` first; 404 +if missing, mirroring `updateUser` at `src/lib/services/users.service.ts:216-219`. + +### 5. Email-change confirm endpoint cannot be aborted after compromise window +`src/app/api/v1/me/email/confirm/[token]/route.ts:42-57` + +Token-based unauthenticated swap. The flow looks otherwise correct (sha256- +hashed token, expiry, single-use via `appliedAt`, race-checked uniqueness). What's +missing: when a confirmation completes, all *other* outstanding `userEmailChanges` +rows for the same `userId` should be cancelled, and all existing Better Auth +sessions for that user should be revoked. Today, if an attacker compromises the +account, requests an email change to attacker-owned address, and the victim +spots the cancel email but races against the attacker — once the attacker +confirms, the victim's cancel link still works on the *other* pending row but +not on the now-applied change, and the attacker's existing CRM session +(`pn-crm.session_token`) survives the swap. Fix: in the confirm handler, after +the email UPDATE, also `db.delete(sessions).where(eq(sessions.userId, +pending.userId))` (or whatever the Better-Auth session table is called) and +mark all other open `userEmailChanges` rows for that user as cancelled. Mirror +the cancel-handler behaviour. Severity is HIGH not CRITICAL because the +attacker needs the session in the first place. + +### 6. Public `/api/auth/[...all]` audits the attempted email but doesn't bound brute-force timing +`src/app/api/auth/[...all]/route.ts:100-146` + +Better Auth handles sign-in rate-limiting internally (it has a built-in limiter +when configured), but I see no explicit `enforcePublicRateLimit` wrapper around +this catch-all. The `loginAttempt` bucket I expected in `src/lib/rate-limit.ts` +isn't present in the listing; the closest is `portalSignIn`, which is wired only +to the *portal* sign-in handler, not the CRM sign-in. If Better Auth's default +limiter isn't actively configured in `src/lib/auth/index.ts:55-113` (and I don't +see a `rateLimit:` block there), the CRM login endpoint is effectively +unrate-limited and the resolve-identifier finding compounds into a real +brute-force window. Fix: add an `enforcePublicRateLimit(req, 'crmSignIn', +attemptedEmail)` call inside `withAuthAudit` before forwarding to +`upstream.POST(forwardReq)` when `isSignIn`, keyed per-email; declare the bucket +in `rate-limit.ts` mirroring `portalSignIn`'s shape. + +## MEDIUM + +### 7. CRM `updateUser` cross-tenant email change has no notification when target is super-admin +`src/lib/services/users.service.ts:236-262` + +When an admin at port A updates a user (including a super-admin who happens to +have a port-role row at port A), the email-change flow flips Better Auth's +identity instantly with only a courtesy email to the prior address. There's no +challenge / token round-trip — the admin acts unilaterally. Self-service email +change (`/api/v1/me/email`) DOES require token confirmation; admin-initiated +should at least block when the target is a super-admin or require the change to +go through the same confirm-token flow. Fix: gate `wantsEmailChange` on +`!profile.isSuperAdmin || ctx.isSuperAdmin` and/or always use the token flow +even for admin-initiated changes. + +### 8. `permission-overrides` PUT does not write audit log atomically with the DB write +`src/app/api/v1/admin/users/[id]/permission-overrides/route.ts:111-134` + +The `existing` row is read, then conditionally update-or-insert, but two +concurrent PUTs against the same `(userId, portId)` race: both see `existing` +as the same value, both call `update`, second writer wins silently with a +last-write audit log that's missing the intermediate state. Severity is medium +because the audit log still captures both writers' new values and there's no +correctness invariant broken — just a forensic gap. Fix: wrap the read + +update/insert in `withTransaction` with `FOR UPDATE` (or use an upsert with +`returning('old')`-equivalent semantics) and log `oldValue` from the locked row. + +### 9. Documenso webhook returns 200 on every failure including dedup, which masks crashes +`src/app/api/webhooks/documenso/route.ts:264-268` + +The handler's outermost `try/catch` logs `err` but always returns 200. That's +the correct posture for *signature*-invalid traffic (don't leak signal), but +also masks downstream handler crashes — Documenso will never retry a 5xx +because it never sees one. The handlers are documented as idempotent +(`handleDocumentCompleted` early-returns on duplicate completion), so a retry +storm wouldn't double-write, but the missing retry signal turns one transient +DB failure into a permanently dropped event. Fix: return 500 on the catch +branch so Documenso retries; keep 200 for *secret*-invalid (line 100) and +dedup (line 123) since those are intentional no-ops. + +### 10. `withAuth` deep-merge: permission overrides only ADD permissions, never EXPLICITLY DENY +`src/lib/api/helpers.ts:73-98, 233-238` + +`deepMerge` does a recursive shallow assignment — `userOverride.permissionOverrides` +overwrites leaves wholesale. So `{clients: {view: false}}` works as a deny. +However the override is keyed by *resource → action map*, and the override row +stores `Partial`. There's no "tri-state" (inherit/grant/deny) +expressed at the DB layer — the comment in the route says "use null at a leaf +to clear an override" but the Zod schema only accepts `z.boolean()` per leaf, +not null. So the UI cannot actually clear an override leaf via this endpoint +without removing the resource key entirely from the JSON. Worth aligning the +schema with the documented contract. Fix: accept `z.union([z.boolean(), +z.null()])` and strip null leaves server-side before writing. + +### 11. Origin check disabled in dev — but `process.env.NODE_ENV` check is per-process +`src/middleware.ts:79-89` + +CSRF defense-in-depth is skipped when `NODE_ENV !== 'production'`. The +dev/staging boundary is correct in principle, but `staging` deployments +typically run with `NODE_ENV=production`, while CI / preview-builds may not. +Worth confirming the Dockerfile (`Dockerfile`) sets `NODE_ENV=production` on +any environment that's reachable from the internet. Note also that the +fallback at `src/middleware.ts:68-69` allows a request with neither Origin nor +Referer through — this is correct for server-side fetches but means any HTTP +client that strips both headers (curl with `-H "Origin:"`) bypasses the check. +Combined with SameSite=strict cookies the residual risk is low. + +### 12. `me/email` confirm/cancel tokens are URL-only — referer leakage risk +`src/app/api/v1/me/email/route.ts:88-89, src/app/api/v1/me/email/confirm/[token]/route.ts:24-35` + +The confirm/cancel URLs are emailed as `${baseUrl}/api/v1/me/email/confirm/${rawToken}`. +The user clicks from their inbox; the email client opens the URL in a browser +which then renders `/settings?emailChange=confirmed` (a redirect). If +`/settings` makes any third-party request before navigating away, the Referer +header carries the full confirm URL including the token. The token is +single-use and short-lived, so the post-redirect exposure window is small, but +defensively the route should `Referrer-Policy: no-referrer` on the redirect +response. Fix: `res.headers.set('Referrer-Policy', 'no-referrer')` on the +`NextResponse.redirect(...)` call. + +## Summary + +Two CRITICAL findings: self-targetable permission-overrides escalation +(finding 1) and unlimited username harvesting at `/api/auth/resolve-identifier` +(finding 2). Both are direct consequences of the recently-added routes that +prompted this audit. The remainder are mostly hardening — the v1/* surface +overall is well-disciplined: nearly every route under `/api/v1/**` flows +through `withAuth(withPermission(...))`, body parsing consistently uses +`parseBody` (only public/auth handlers use raw `req.json()` for documented +reasons), and the few raw `sql\`…\`` usages I sampled +(`admin/website-submissions`, `admin/document-sends`, `search/recently-viewed`) +all interpolate via the parameterized tag form rather than string concat. +Multi-tenant scoping looks consistent — services accept `ctx.portId` and the +defense-in-depth pattern is well-applied (e.g. the berth-recommender note in +CLAUDE.md). The Documenso webhook receiver has solid replay/dedup/secret +discipline. + +--- + +## 2. UI/UX consistency + accessibility audit (ui-ux-auditor) + +# UI/UX Consistency + Accessibility Audit + +Scope: Form patterns, dialog/sheet/drawer choices, mobile parity, enum leakage, empty/loading states, badge tones, a11y, plus the recently added surfaces (UserForm tabs, UserList Power toggle, UserPermissionMatrix, Login identifier field, user settings username card). + +--- + +## CRITICAL + +### C1 — `window.confirm()` / `confirm()` used for destructive flows (>=15 sites) +Files using native browser confirm instead of `ConfirmationDialog` (which wraps `AlertDialog`): +- `src/components/clients/contacts-editor.tsx:115` — remove contact +- `src/components/clients/client-files-tab.tsx:50` — delete file +- `src/components/yachts/yacht-list.tsx:187` — archive yacht (bulk) +- `src/components/admin/document-templates/template-version-history.tsx:54` — restore older version +- `src/components/shared/addresses-editor.tsx:77` — remove address +- `src/components/documents/document-detail.tsx:160` — cancel/void signing envelope +- `src/components/interests/interest-list.tsx:314` — archive interest +- `src/components/interests/interest-tabs.tsx:483` — outcome/archival flow +- `src/components/interests/interest-eoi-tab.tsx:299` — cancel EOI +- `src/components/interests/interest-reservation-tab.tsx:313` — cancel contract +- `src/components/interests/interest-contact-log-tab.tsx:222` — delete contact log +- `src/components/interests/interest-contract-tab.tsx:310` — cancel contract +- `src/components/interests/interest-documents-tab.tsx:80` — delete file +- `src/components/companies/company-files-tab.tsx:50` — delete file +- `src/components/companies/company-list.tsx:201` — archive company +- `src/components/documents/document-list.tsx:136` — delete document + +**Why it matters:** native confirm cannot be styled, bypasses our `` keyboard semantics, no focus trap, no destructive-action red styling, fails focus-return after dismiss; inconsistent with the rest of the app which uses `ConfirmationDialog`. Several of these are catastrophic (cancel signing envelope, hard-delete file, archive company). +**Fix:** replace each with `` matching the pattern in `user-list.tsx`. + +### C2 — UserForm "Permissions" tab silently drops unsaved overrides +`src/components/admin/users/user-form.tsx:204-212` and `user-permission-matrix.tsx:175-191`. +The matrix has its own "Save overrides" button; the parent Sheet's "Save changes" only persists Profile-tab fields. `onSaveStateChange` is declared in the matrix props but **never passed** by `user-form.tsx` (line 206), so the parent has no idea overrides are dirty. A user who toggles Inherit/Grant/Deny then clicks "Save changes" loses everything when the Sheet closes — no warning, no toast. +**Fix:** lift `overrides` state to `user-form.tsx`, persist both endpoints inside `persist()`, or track dirty state via `onSaveStateChange` and block Sheet close with an AlertDialog. + +--- + +## HIGH + +### H1 — Raw enum render via `.replace(/_/g, ' ')` outside `constants.ts` (40+ sites) +Examples (not exhaustive): +- `src/components/documents/documents-hub.tsx:292`, `document-detail.tsx:204,210,386`, `entity-folder-view.tsx:63`, `hub-root-view.tsx:69`, `signing-details-dialog.tsx:123` — `status`, `eventType`, `documentType` +- `src/components/reservations/reservation-detail.tsx:230,285,339` — `tenureType`, agreement status +- `src/components/berths/berth-status-suggestion-dialog.tsx:61,65` +- `src/components/expenses/expense-detail.tsx:229,233`, `expense-card.tsx:71`, `expense-columns.tsx:121`, `expense-form-dialog.tsx:257,278`, `expense-filters.tsx:16` +- `src/components/admin/audit/audit-log-list.tsx:234-235`, `roles/role-list.tsx:223,239`, `roles/role-form.tsx:123` +- `src/components/admin/users/user-permission-matrix.tsx:101` — local `formatAction` duplicates pattern +- `src/components/dashboard/source-conversion-chart.tsx:60`, `activity-feed.tsx:34,44` +- `src/components/scan/scan-shell.tsx:227,242` +- `src/components/interests/linked-berths-list.tsx:94`, `interest-tabs.tsx:40` +- `src/app/(portal)/portal/{my-yachts,documents,interests}/page.tsx` — portal-side enum leakage +- `src/components/search/command-search.tsx:939,965` — fallback after `STAGE_LABELS` + +**Fix:** route through `stageLabel`, `formatRole`, `formatOutcome`, `formatSource` (already in `constants.ts`); add `formatDocumentStatus`, `formatTenureType`, `formatEventType`, `formatExpenseCategory`, `formatPaymentMethod`, `formatBerthStatus`, `formatPermissionAction` to `constants.ts` and replace call-sites. Removes "manage memberships" / "Eoi Signed" inconsistencies. + +### H2 — Mobile parity: 18 list components have no `cardRender` +DataTable already supports `cardRender`; without it the mobile view falls back to a raw horizontal-scroll table (bad UX on iOS): +- `src/components/reservations/reservation-list.tsx`, `berth-reservations-list.tsx` +- `src/components/website-analytics/top-list.tsx` +- `src/components/shared/notes-list.tsx` +- `src/components/residential/residential-clients-list.tsx`, `residential-interests-list.tsx` +- `src/components/documents/document-list.tsx` +- `src/components/interests/linked-berths-list.tsx`, `recommendation-list.tsx` +- `src/components/email/email-accounts-list.tsx`, `email-threads-list.tsx` +- `src/components/reports/reports-list.tsx` +- `src/components/admin/document-templates/template-list.tsx`, `forms/form-template-list.tsx`, `roles/role-list.tsx`, `tags/tag-list.tsx`, `ports/port-list.tsx` + +**Fix:** add cardRender mirroring desktop columns. `UserCard`/`ClientCard`/`InterestCard` are good templates. + +### H3 — User settings phone field is unbound on load +`src/components/settings/user-settings.tsx:69-92` — `loadProfile()` reads `firstName`, `lastName`, `email`, etc., but **never reads `phone`** into state. Yet `saveProfile()` at line 143 sends `phone: phone || null`, which **clears the user's stored phone on every save**. Also `country as never` cast at line 298 is unsound — when no country is selected the PhoneInput shows a US flag even for European users. +**Fix:** add `phone` to MeResponse + `setPhone(res.data.profile?.phone ?? '')`. Store country alongside phone (the PhoneInput value is `{e164, country}` — persist the parsed country). + +### H4 — UserPermissionMatrix three-state toggle has no a11y semantics +`user-permission-matrix.tsx:247-267` — three sibling `