fix(audit-tier-4): tenant-isolation defense-in-depth

Closes the audit's HIGH §10 + MED §§17–22 isolation footguns. None of
these are user-impactful TODAY — every site is preceded by a port-
scoped read or pre-validated by ctx.portId — but each is a future-
refactor accident waiting to happen, so the SQL itself now pins the
tenant boundary:

* mergeClients gains a callerPortId option; the route caller passes
  ctx.portId.  removeInterestBerth now requires portId and verifies
  both the interest and the berth share it before deleting the
  junction row.  All three callers updated.
* Six service mutations now scope the WHERE to (id, portId):
  form-templates update + delete, invoices.detectOverdue per-row
  update, notifications.markRead, clients.deleteRelationship.
  company-memberships uses an inArray sub-select against port
  companies (no port_id column on the table itself), covering
  updateMembership / endMembership / setPrimary.
* Port-scoped file lookups in portal.getDocumentDownloadUrl,
  reports.getDownloadUrl (file presign), berth-reservations.activate
  (contractFileId attach guard), and residential.getResidentialInterestById
  (residentialClient join).

Test status: 1168/1168 vitest, tsc clean.

Refs: docs/audit-comprehensive-2026-05-05.md HIGH §10 + MED §§17–22
(auditor-B3 Issues 1–5,7).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Matt Ciaccio
2026-05-05 20:48:13 +02:00
parent 7854cbabe4
commit 4eea4ceff9
14 changed files with 142 additions and 18 deletions

View File

@@ -3,6 +3,7 @@ import { db } from '@/lib/db';
import { berthReservations, type BerthReservation } from '@/lib/db/schema/reservations';
import { berths } from '@/lib/db/schema/berths';
import { clients } from '@/lib/db/schema/clients';
import { files } from '@/lib/db/schema/documents';
import { yachts } from '@/lib/db/schema/yachts';
import { companyMemberships } from '@/lib/db/schema/companies';
import { buildListQuery } from '@/lib/db/query-builder';
@@ -169,6 +170,19 @@ export async function activate(
updatedAt: new Date(),
};
if (data.contractFileId !== undefined) {
// Verify the contract file lives in the caller's port. Without this,
// a port-A user activating a port-A reservation could attach a
// port-B file id and downstream presigned-download paths (admin
// export, portal contract download) would otherwise leak that
// foreign-port content.
if (data.contractFileId !== null) {
const contractFile = await db.query.files.findFirst({
where: and(eq(files.id, data.contractFileId), eq(files.portId, portId)),
});
if (!contractFile) {
throw new ValidationError('contract file not found in this port');
}
}
patch.contractFileId = data.contractFileId;
}
if (data.effectiveDate !== undefined) {

View File

@@ -55,6 +55,16 @@ export interface MergeOptions {
loserId: string;
/** ID of the user performing the merge (for audit + clientMergeLog.mergedBy). */
mergedBy: string;
/**
* Caller's port — defends against any future caller forgetting to
* pre-validate. Today the sole route caller pre-checks via ctx.portId,
* so this is defense-in-depth: a future bulk-import or CLI caller
* that omits the check still cannot trigger a cross-tenant merge.
* Optional for backwards compat; mergeClients enforces same-port
* regardless, but when set the assertion is "winner.portId === callerPortId"
* (and by transitive same-port rule, loser too).
*/
callerPortId?: string;
/** Per-field choice overrides. Multi-value fields (contacts, addresses,
* notes, tags) are always preserved from both sides; this only
* affects single-value scalar fields on the `clients` row. */
@@ -105,6 +115,14 @@ export async function mergeClients(opts: MergeOptions): Promise<MergeResult> {
if (winnerRow.portId !== loserRow.portId) {
throw new ValidationError('Cannot merge clients across different ports');
}
if (opts.callerPortId && winnerRow.portId !== opts.callerPortId) {
// Defense-in-depth: even though the route already pre-checks, a
// future caller that wires this service from a CLI / bulk-import
// path would otherwise be able to merge any two clients sharing a
// port (or, if the same-port check above were ever weakened, two
// arbitrary clients). Refuse upfront.
throw new ValidationError('Cannot merge clients in another port');
}
if (loserRow.mergedIntoClientId) {
throw new ConflictError('That client has already been merged into another record.');
}

View File

@@ -832,7 +832,9 @@ export async function deleteRelationship(
});
if (!rel || rel.portId !== portId) throw new NotFoundError('Relationship');
await db.delete(clientRelationships).where(eq(clientRelationships.id, relId));
await db
.delete(clientRelationships)
.where(and(eq(clientRelationships.id, relId), eq(clientRelationships.portId, portId)));
void createAuditLog({
userId: meta.userId,

View File

@@ -1,4 +1,4 @@
import { and, desc, eq, isNull, ne } from 'drizzle-orm';
import { and, desc, eq, inArray, isNull, ne } from 'drizzle-orm';
import { db } from '@/lib/db';
import { companies, companyMemberships } from '@/lib/db/schema/companies';
import type { CompanyMembership } from '@/lib/db/schema/companies';
@@ -135,7 +135,20 @@ export async function updateMembership(
const rows = await db
.update(companyMemberships)
.set({ ...data, updatedAt: new Date() })
.where(eq(companyMemberships.id, membershipId))
// Defense-in-depth: companyMemberships has no port_id column, so the
// tenant boundary lives on the parent company. Pin the WHERE to a
// sub-select of port companies — even if loadMembershipScoped were
// ever bypassed, a stray membershipId from another tenant cannot
// mutate.
.where(
and(
eq(companyMemberships.id, membershipId),
inArray(
companyMemberships.companyId,
db.select({ id: companies.id }).from(companies).where(eq(companies.portId, portId)),
),
),
)
.returning();
const updated = rows[0]!;
@@ -173,7 +186,20 @@ export async function endMembership(
const rows = await db
.update(companyMemberships)
.set({ endDate: data.endDate, updatedAt: new Date() })
.where(eq(companyMemberships.id, membershipId))
// Defense-in-depth: companyMemberships has no port_id column, so the
// tenant boundary lives on the parent company. Pin the WHERE to a
// sub-select of port companies — even if loadMembershipScoped were
// ever bypassed, a stray membershipId from another tenant cannot
// mutate.
.where(
and(
eq(companyMemberships.id, membershipId),
inArray(
companyMemberships.companyId,
db.select({ id: companies.id }).from(companies).where(eq(companies.portId, portId)),
),
),
)
.returning();
const updated = rows[0]!;
@@ -227,7 +253,16 @@ export async function setPrimary(
const rows = await tx
.update(companyMemberships)
.set({ isPrimary: true, updatedAt: new Date() })
.where(eq(companyMemberships.id, membershipId))
// Same defense-in-depth via port-companies sub-select — see updateMembership.
.where(
and(
eq(companyMemberships.id, membershipId),
inArray(
companyMemberships.companyId,
tx.select({ id: companies.id }).from(companies).where(eq(companies.portId, portId)),
),
),
)
.returning();
const updated = rows[0]!;

View File

@@ -87,7 +87,7 @@ export async function updateFormTemplate(
...(data.isActive !== undefined && { isActive: data.isActive }),
updatedAt: new Date(),
})
.where(eq(formTemplates.id, id))
.where(and(eq(formTemplates.id, id), eq(formTemplates.portId, portId)))
.returning();
if (!updated) throw new NotFoundError('Form template');
@@ -110,7 +110,9 @@ export async function updateFormTemplate(
export async function deleteFormTemplate(id: string, portId: string, meta: AuditMeta) {
await getFormTemplateById(id, portId);
await db.delete(formTemplates).where(eq(formTemplates.id, id));
await db
.delete(formTemplates)
.where(and(eq(formTemplates.id, id), eq(formTemplates.portId, portId)));
void createAuditLog({
userId: meta.userId,

View File

@@ -21,7 +21,7 @@ import { and, desc, eq, inArray } from 'drizzle-orm';
import { db } from '@/lib/db';
import { interestBerths, interests, type InterestBerth } from '@/lib/db/schema/interests';
import { berths } from '@/lib/db/schema/berths';
import { CodedError } from '@/lib/errors';
import { CodedError, NotFoundError } from '@/lib/errors';
type DbOrTx = typeof db | Parameters<Parameters<typeof db.transaction>[0]>[0];
@@ -274,8 +274,34 @@ export async function setPrimaryBerth(interestId: string, berthId: string): Prom
await upsertInterestBerth(interestId, berthId, { isPrimary: true });
}
/** Remove a berth from an interest. */
export async function removeInterestBerth(interestId: string, berthId: string): Promise<void> {
/** Remove a berth from an interest.
*
* `portId` is required for cross-port defense — `upsertInterestBerth`
* and `setPrimaryBerth` both verify the interest + berth share the
* caller's port before mutation, but the original `removeInterestBerth`
* issued a delete keyed only by (interestId, berthId), so a future
* caller that omitted its own port check could delete a junction row
* across tenants. This now mirrors the cross-check used by upsert.
*/
export async function removeInterestBerth(
interestId: string,
berthId: string,
portId: string,
): Promise<void> {
// Verify both the interest and the berth belong to the caller's
// port before issuing the delete. A tenant boundary breach would
// otherwise be a single misrouted call away.
const [interestRow, berthRow] = await Promise.all([
db.query.interests.findFirst({
where: and(eq(interests.id, interestId), eq(interests.portId, portId)),
}),
db.query.berths.findFirst({
where: and(eq(berths.id, berthId), eq(berths.portId, portId)),
}),
]);
if (!interestRow || !berthRow) {
throw new NotFoundError('interest or berth');
}
await db
.delete(interestBerths)
.where(and(eq(interestBerths.interestId, interestId), eq(interestBerths.berthId, berthId)));

View File

@@ -563,7 +563,7 @@ export async function updateInterest(
addedBy: meta.userId,
});
} else if (currentBerthId) {
await removeInterestBerth(id, currentBerthId);
await removeInterestBerth(id, currentBerthId, portId);
}
}
@@ -981,7 +981,7 @@ export async function unlinkBerth(id: string, portId: string, meta: AuditMeta) {
const oldBerthId = previousPrimary?.berthId ?? null;
if (oldBerthId) {
await removeInterestBerth(id, oldBerthId);
await removeInterestBerth(id, oldBerthId, portId);
}
const [updated] = await db

View File

@@ -768,7 +768,7 @@ export async function detectOverdue(portId: string) {
await db
.update(invoices)
.set({ status: 'overdue', updatedAt: new Date() })
.where(eq(invoices.id, inv.id));
.where(and(eq(invoices.id, inv.id), eq(invoices.portId, portId)));
const daysPastDue = Math.max(
1,

View File

@@ -216,10 +216,20 @@ export async function markRead(notificationId: string, userId: string): Promise<
throw new NotFoundError('Notification');
}
// Pin the WHERE to (id, userId, portId) — the SELECT just resolved
// notif.portId for this notification, so the update is fully tenant-
// scoped and a future caller mistake (or a cache layer that ever
// surfaced a foreign-port id) can't flip a row in another port.
await db
.update(notifications)
.set({ isRead: true })
.where(and(eq(notifications.id, notificationId), eq(notifications.userId, userId)));
.where(
and(
eq(notifications.id, notificationId),
eq(notifications.userId, userId),
eq(notifications.portId, notif.portId),
),
);
const unreadCount = await getUnreadCountValue(userId, notif.portId);
emitToRoom(`user:${userId}`, 'notification:unreadCount', { count: unreadCount });

View File

@@ -301,8 +301,13 @@ export async function getDocumentDownloadUrl(
const fileId = doc.signedFileId ?? doc.fileId;
if (!fileId) return null;
// Defense-in-depth: scope the file lookup to the same port. Today the
// doc → file relationship is tightly coupled (signedFileId/fileId are
// assigned in same-port flows), but a future Documenso-webhook bug or
// bulk-import drift could leave a foreign-port file id on a doc and
// this presign call would otherwise return a URL to those bytes.
const file = await db.query.files.findFirst({
where: eq(files.id, fileId),
where: and(eq(files.id, fileId), eq(files.portId, portId)),
});
if (!file) return null;

View File

@@ -154,8 +154,13 @@ export async function getDownloadUrl(reportId: string, portId: string) {
throw new ConflictError('Report is not ready for download');
}
// Defense-in-depth: scope the file lookup to the same port. report.fileId
// is currently assigned at generation time in the same-port flow, but
// pinning portId in the WHERE means a future bulk-import or admin-bypass
// path that ever crossed port boundaries cannot leak a foreign file via
// a presigned URL.
const file = await db.query.files.findFirst({
where: eq(files.id, report.fileId),
where: and(eq(files.id, report.fileId), eq(files.portId, portId)),
});
if (!file) {

View File

@@ -226,8 +226,14 @@ export async function getResidentialInterestById(id: string, portId: string) {
});
if (!interest) throw new NotFoundError('Residential interest');
// The residentialInterest is already port-scoped; pin the client read
// to the same port too so a future drift (a foreign-port residential
// client id ever landing on the interest) cannot leak.
const client = await db.query.residentialClients.findFirst({
where: eq(residentialClients.id, interest.residentialClientId),
where: and(
eq(residentialClients.id, interest.residentialClientId),
eq(residentialClients.portId, portId),
),
});
return { ...interest, client };