pn-new-crm/docs/audit-findings-tmp/02-multitenancy-schema.md

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