From bdc9c019a8e8ae7d90c5735bd4c61d04d03bc480 Mon Sep 17 00:00:00 2001 From: Matt Date: Tue, 12 May 2026 16:53:16 +0200 Subject: [PATCH] =?UTF-8?q?audit:=20append=20storage-pathing=20report=20?= =?UTF-8?q?=E2=80=94=20all=2033=20agents=20now=20inlined?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit docs/AUDIT-2026-05-12.md now contains every audit verbatim (6488 lines). Last to land was the S3-vs-internal-DB routing audit covering the storage-backend boundary, presigned-URL round-trip, magic-byte verification on both paths, migrate-storage coverage, and orphan-blob risk on transaction failure. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/AUDIT-2026-05-12.md | 1302 +++++++++++++++++++++++++++----------- 1 file changed, 926 insertions(+), 376 deletions(-) diff --git a/docs/AUDIT-2026-05-12.md b/docs/AUDIT-2026-05-12.md index 89231780..b37b7d10 100644 --- a/docs/AUDIT-2026-05-12.md +++ b/docs/AUDIT-2026-05-12.md @@ -22,26 +22,26 @@ Severity is the auditor's judgment, not mine — I have not re-graded findings. ### 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 | +| # | 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 @@ -54,39 +54,46 @@ Listed after CRITICALs in the priority queue section below. 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/...) --- @@ -142,6 +149,7 @@ 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 @@ -150,7 +158,7 @@ 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 +_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', @@ -161,6 +169,7 @@ 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 @@ -168,8 +177,8 @@ Better Auth's sign-in step can redeem internally. 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` +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 @@ -180,12 +189,14 @@ 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) @@ -208,6 +219,7 @@ 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`, @@ -230,6 +242,7 @@ 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 @@ -241,6 +254,7 @@ 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 @@ -251,6 +265,7 @@ 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 @@ -260,6 +275,7 @@ 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 @@ -273,6 +289,7 @@ 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` @@ -298,11 +315,10 @@ emitted, but the limiter caps the blast radius. - 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. +- 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. + - PDF magic-byte enforcement before disk write. — security-auditor (read-only audit; no source files edited) @@ -316,6 +332,7 @@ 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 @@ -333,15 +350,16 @@ grant). Tier-2 fix: rotate this row to require super-admin outright; admin-of-po 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 +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 +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 @@ -353,6 +371,7 @@ 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()))` — @@ -367,6 +386,7 @@ 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 @@ -381,16 +401,17 @@ 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` +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 +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, @@ -400,13 +421,14 @@ 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 +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 @@ -418,6 +440,7 @@ 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 @@ -431,6 +454,7 @@ go through the same confirm-token flow. Fix: gate `wantsEmailChange` on 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 @@ -443,24 +467,26 @@ 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 +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 +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 +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, @@ -470,6 +496,7 @@ 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 @@ -483,6 +510,7 @@ 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}`. @@ -500,7 +528,7 @@ response. Fix: `res.headers.set('Referrer-Policy', 'no-referrer')` on the 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 +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 @@ -525,7 +553,9 @@ Scope: Form patterns, dialog/sheet/drawer choices, mobile parity, enum leakage, ## 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) @@ -547,6 +577,7 @@ Files using native browser confirm instead of `ConfirmationDialog` (which wraps **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. @@ -556,7 +587,9 @@ The matrix has its own "Save overrides" button; the parent Sheet's "Save changes ## 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` @@ -572,7 +605,9 @@ Examples (not exhaustive): **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` @@ -586,18 +621,22 @@ DataTable already supports `cardRender`; without it the mobile view falls back t **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 `