From ba89b61b3f24802678c5bbc2e18a76f7cbc34e46 Mon Sep 17 00:00:00 2001 From: Matt Ciaccio Date: Wed, 29 Apr 2026 04:14:09 +0200 Subject: [PATCH] fix(security): port-scope clientId/berthId/yachtId on interests + clientRelationships MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pass-6 findings — both MEDIUM cross-tenant FK injection. - interests.service: createInterest/updateInterest/linkBerth accepted clientId/berthId/yachtId from the request body without verifying the referenced row belongs to the caller's port. getInterestById joins clients/berths/yachtTags on these FKs without a port filter, so a port-A caller could splice a foreign-port id and surface that tenant's clientName, mooringNumber, or yacht ownership on read. New assertInterestFksInPort helper guards all three surfaces. - clients.service.createRelationship: accepted clientBId from the body without a port check; the relationship list endpoint joins clients without filtering by port, so the foreign client's name + email would render in the relationships tab. Now verifies clientBId belongs to portId and rejects self-relationships. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lib/services/clients.service.ts | 15 +- src/lib/services/interests.service.ts | 56 +++++++ ...client-relationship-port-isolation.test.ts | 74 +++++++++ .../interests-port-fk-isolation.test.ts | 150 ++++++++++++++++++ 4 files changed, 294 insertions(+), 1 deletion(-) create mode 100644 tests/integration/client-relationship-port-isolation.test.ts create mode 100644 tests/integration/interests-port-fk-isolation.test.ts diff --git a/src/lib/services/clients.service.ts b/src/lib/services/clients.service.ts index fb9b57f..9825983 100644 --- a/src/lib/services/clients.service.ts +++ b/src/lib/services/clients.service.ts @@ -13,7 +13,7 @@ import { yachts } from '@/lib/db/schema/yachts'; import { berthReservations } from '@/lib/db/schema/reservations'; import { tags } from '@/lib/db/schema/system'; import { createAuditLog, type AuditMeta } from '@/lib/audit'; -import { NotFoundError } from '@/lib/errors'; +import { NotFoundError, ValidationError } from '@/lib/errors'; import { isPortalEnabledForPort } from '@/lib/services/portal-auth.service'; import { setEntityTags } from '@/lib/services/entity-tags.helper'; import { emitToRoom } from '@/lib/socket/server'; @@ -663,11 +663,24 @@ export async function createRelationship( data: { clientBId: string; relationshipType: string; description?: string }, meta: AuditMeta, ) { + if (data.clientBId === clientId) { + throw new ValidationError('A client cannot have a relationship to themselves'); + } + const client = await db.query.clients.findFirst({ where: eq(clients.id, clientId), }); if (!client || client.portId !== portId) throw new NotFoundError('Client'); + // Tenant scope: clientBId arrives from the request body. Without this check + // a port-A caller could splice a port-B client UUID onto their own client's + // relationship row; the GET handler joins clientRelationships → clients with + // no port filter and would surface the foreign client's name + email. + const otherClient = await db.query.clients.findFirst({ + where: and(eq(clients.id, data.clientBId), eq(clients.portId, portId)), + }); + if (!otherClient) throw new ValidationError('clientBId not found in this port'); + const [rel] = await db .insert(clientRelationships) .values({ portId, clientAId: clientId, ...data }) diff --git a/src/lib/services/interests.service.ts b/src/lib/services/interests.service.ts index 11bc926..d2f1a34 100644 --- a/src/lib/services/interests.service.ts +++ b/src/lib/services/interests.service.ts @@ -23,6 +23,49 @@ import type { // ─── Types ──────────────────────────────────────────────────────────────────── +// ─── Port-scope FK validator ───────────────────────────────────────────────── + +// Tenant scope: every FK referenced from an interest body — clientId, berthId, +// and yachtId — must belong to the caller's port. Without this, a body-supplied +// foreign-port id would create an interest that joins through these FKs and +// surfaces foreign-tenant data on subsequent reads (clientName, berth mooring +// number, yacht ownership). assertYachtBelongsToClient still runs separately to +// enforce the additional ownership invariant. +async function assertInterestFksInPort( + portId: string, + fks: { clientId?: string | null; berthId?: string | null; yachtId?: string | null }, +): Promise { + const checks: Array> = []; + if (fks.clientId) { + checks.push( + db.query.clients + .findFirst({ where: and(eq(clients.id, fks.clientId), eq(clients.portId, portId)) }) + .then((row) => { + if (!row) throw new ValidationError('clientId not found in this port'); + }), + ); + } + if (fks.berthId) { + checks.push( + db.query.berths + .findFirst({ where: and(eq(berths.id, fks.berthId), eq(berths.portId, portId)) }) + .then((row) => { + if (!row) throw new ValidationError('berthId not found in this port'); + }), + ); + } + if (fks.yachtId) { + checks.push( + db.query.yachts + .findFirst({ where: and(eq(yachts.id, fks.yachtId), eq(yachts.portId, portId)) }) + .then((row) => { + if (!row) throw new ValidationError('yachtId not found in this port'); + }), + ); + } + await Promise.all(checks); +} + // ─── Yacht ownership validator ─────────────────────────────────────────────── async function assertYachtBelongsToClient( @@ -262,6 +305,12 @@ export async function getInterestById(id: string, portId: string) { // ─── Create ─────────────────────────────────────────────────────────────────── export async function createInterest(portId: string, data: CreateInterestInput, meta: AuditMeta) { + await assertInterestFksInPort(portId, { + clientId: data.clientId, + berthId: data.berthId, + yachtId: data.yachtId, + }); + if (data.yachtId) { await assertYachtBelongsToClient(portId, data.yachtId, data.clientId); } @@ -338,6 +387,11 @@ export async function updateInterest( throw new NotFoundError('Interest'); } + await assertInterestFksInPort(portId, { + berthId: data.berthId && data.berthId !== existing.berthId ? data.berthId : null, + yachtId: data.yachtId && data.yachtId !== existing.yachtId ? data.yachtId : null, + }); + if (data.yachtId && data.yachtId !== existing.yachtId) { await assertYachtBelongsToClient(portId, data.yachtId, existing.clientId); } @@ -571,6 +625,8 @@ export async function linkBerth(id: string, portId: string, berthId: string, met throw new NotFoundError('Interest'); } + await assertInterestFksInPort(portId, { berthId }); + const [updated] = await db .update(interests) .set({ berthId, updatedAt: new Date() }) diff --git a/tests/integration/client-relationship-port-isolation.test.ts b/tests/integration/client-relationship-port-isolation.test.ts new file mode 100644 index 0000000..1f11579 --- /dev/null +++ b/tests/integration/client-relationship-port-isolation.test.ts @@ -0,0 +1,74 @@ +/** + * clients.service.createRelationship — tenant-FK validation tests. + * + * Covers the fix that requires both clientAId (the URL id) and clientBId + * (the body id) to belong to the caller's port. The list endpoint joins + * clientRelationships → clients without a port filter, so a foreign-port + * clientBId would surface that client's name in the relationship payload. + */ +import { describe, it, expect, beforeAll } from 'vitest'; + +describe('clients.service — createRelationship port isolation', () => { + let createRelationship: typeof import('@/lib/services/clients.service').createRelationship; + + let makePort: typeof import('../helpers/factories').makePort; + let makeClient: typeof import('../helpers/factories').makeClient; + let makeAuditMeta: typeof import('../helpers/factories').makeAuditMeta; + + beforeAll(async () => { + const svc = await import('@/lib/services/clients.service'); + createRelationship = svc.createRelationship; + + const factories = await import('../helpers/factories'); + makePort = factories.makePort; + makeClient = factories.makeClient; + makeAuditMeta = factories.makeAuditMeta; + }); + + it('rejects createRelationship when clientBId belongs to a foreign port', async () => { + const portA = await makePort(); + const portB = await makePort(); + const localClient = await makeClient({ portId: portA.id }); + const foreignClient = await makeClient({ portId: portB.id }); + + await expect( + createRelationship( + localClient.id, + portA.id, + { clientBId: foreignClient.id, relationshipType: 'family' }, + makeAuditMeta({ portId: portA.id }), + ), + ).rejects.toThrow(/clientBId not found in this port/); + }); + + it('rejects creating a self-relationship', async () => { + const portA = await makePort(); + const localClient = await makeClient({ portId: portA.id }); + + await expect( + createRelationship( + localClient.id, + portA.id, + { clientBId: localClient.id, relationshipType: 'family' }, + makeAuditMeta({ portId: portA.id }), + ), + ).rejects.toThrow(/cannot have a relationship to themselves/); + }); + + it('accepts createRelationship when both clients are in the same port', async () => { + const portA = await makePort(); + const clientA = await makeClient({ portId: portA.id }); + const clientB = await makeClient({ portId: portA.id }); + + const rel = await createRelationship( + clientA.id, + portA.id, + { clientBId: clientB.id, relationshipType: 'family' }, + makeAuditMeta({ portId: portA.id }), + ); + + expect(rel.clientAId).toBe(clientA.id); + expect(rel.clientBId).toBe(clientB.id); + expect(rel.portId).toBe(portA.id); + }); +}); diff --git a/tests/integration/interests-port-fk-isolation.test.ts b/tests/integration/interests-port-fk-isolation.test.ts new file mode 100644 index 0000000..a60fb52 --- /dev/null +++ b/tests/integration/interests-port-fk-isolation.test.ts @@ -0,0 +1,150 @@ +/** + * interests.service tenant-FK validation tests. + * + * Covers the security fix that pins clientId/berthId/yachtId on interest + * create/update + linkBerth to the caller's port. Without these guards a + * port-A caller could splice a port-B FK onto their own interest and + * surface foreign-tenant data through the join surfaces in getInterestById. + */ +import { describe, it, expect, beforeAll } from 'vitest'; + +describe('interests.service — port-scope FK validation', () => { + let createInterest: typeof import('@/lib/services/interests.service').createInterest; + let updateInterest: typeof import('@/lib/services/interests.service').updateInterest; + let linkBerth: typeof import('@/lib/services/interests.service').linkBerth; + + let makePort: typeof import('../helpers/factories').makePort; + let makeClient: typeof import('../helpers/factories').makeClient; + let makeBerth: typeof import('../helpers/factories').makeBerth; + let makeYacht: typeof import('../helpers/factories').makeYacht; + let makeAuditMeta: typeof import('../helpers/factories').makeAuditMeta; + + beforeAll(async () => { + const svc = await import('@/lib/services/interests.service'); + createInterest = svc.createInterest; + updateInterest = svc.updateInterest; + linkBerth = svc.linkBerth; + + const factories = await import('../helpers/factories'); + makePort = factories.makePort; + makeClient = factories.makeClient; + makeBerth = factories.makeBerth; + makeYacht = factories.makeYacht; + makeAuditMeta = factories.makeAuditMeta; + }); + + it('rejects createInterest with a foreign-port clientId', async () => { + const portA = await makePort(); + const portB = await makePort(); + const foreignClient = await makeClient({ portId: portB.id }); + + await expect( + createInterest( + portA.id, + { + clientId: foreignClient.id, + pipelineStage: 'open', + tagIds: [], + reminderEnabled: false, + }, + makeAuditMeta({ portId: portA.id }), + ), + ).rejects.toThrow(/clientId not found in this port/); + }); + + it('rejects createInterest with a foreign-port berthId', async () => { + const portA = await makePort(); + const portB = await makePort(); + const localClient = await makeClient({ portId: portA.id }); + const foreignBerth = await makeBerth({ portId: portB.id }); + + await expect( + createInterest( + portA.id, + { + clientId: localClient.id, + berthId: foreignBerth.id, + pipelineStage: 'open', + tagIds: [], + reminderEnabled: false, + }, + makeAuditMeta({ portId: portA.id }), + ), + ).rejects.toThrow(/berthId not found in this port/); + }); + + it('rejects createInterest with a foreign-port yachtId', async () => { + const portA = await makePort(); + const portB = await makePort(); + const localClient = await makeClient({ portId: portA.id }); + const foreignClient = await makeClient({ portId: portB.id }); + const foreignYacht = await makeYacht({ + portId: portB.id, + ownerType: 'client', + ownerId: foreignClient.id, + }); + + await expect( + createInterest( + portA.id, + { + clientId: localClient.id, + yachtId: foreignYacht.id, + pipelineStage: 'open', + tagIds: [], + reminderEnabled: false, + }, + makeAuditMeta({ portId: portA.id }), + ), + ).rejects.toThrow(/yachtId not found in this port/); + }); + + it('rejects updateInterest swapping in a foreign-port berthId', async () => { + const portA = await makePort(); + const portB = await makePort(); + const localClient = await makeClient({ portId: portA.id }); + const foreignBerth = await makeBerth({ portId: portB.id }); + + const interest = await createInterest( + portA.id, + { + clientId: localClient.id, + pipelineStage: 'open', + tagIds: [], + reminderEnabled: false, + }, + makeAuditMeta({ portId: portA.id }), + ); + + await expect( + updateInterest( + interest.id, + portA.id, + { berthId: foreignBerth.id }, + makeAuditMeta({ portId: portA.id }), + ), + ).rejects.toThrow(/berthId not found in this port/); + }); + + it('rejects linkBerth with a foreign-port berthId', async () => { + const portA = await makePort(); + const portB = await makePort(); + const localClient = await makeClient({ portId: portA.id }); + const foreignBerth = await makeBerth({ portId: portB.id }); + + const interest = await createInterest( + portA.id, + { + clientId: localClient.id, + pipelineStage: 'open', + tagIds: [], + reminderEnabled: false, + }, + makeAuditMeta({ portId: portA.id }), + ); + + await expect( + linkBerth(interest.id, portA.id, foreignBerth.id, makeAuditMeta({ portId: portA.id })), + ).rejects.toThrow(/berthId not found in this port/); + }); +});