sec: lock down 5 cross-tenant FK gaps from fifth-pass review
1. HIGH — reminders.create/updateReminder accepted clientId/interestId/
berthId from the body and persisted them with no port check; getReminder
then hydrated the row via Drizzle relations (no port filter on the
join), so a port-A user with reminders:create could exfiltrate any
port-B client/interest/berth row by guessing its UUID. New
assertReminderFksInPort gates create + update.
2. HIGH — listRecommendations(interestId, _portId) discarded portId
entirely; the route GET /api/v1/interests/[id]/recommendations
forwarded the URL id straight through. A port-A user with
interests:view could read any other tenant's recommended berths
(mooring numbers, dimensions, status). Service now verifies the
interest belongs to portId and joins berths filtered by port.
3. HIGH — Berth waiting list. The PATCH route did not pre-check that
the berth belonged to ctx.portId — a port-A user with
manage_waiting_list could reorder a port-B berth's queue. Separately,
updateWaitingList accepted arbitrary entries[].clientId and inserted
them without verifying tenancy, polluting the table with foreign-port
FKs. Both gaps closed.
4. MEDIUM — setEntityTags (clients/companies/yachts/interests/berths)
accepted any tagId and inserted into the join table. The tags table
is per-port but the join only carries a single-column FK. The
downstream getById join `tags ON join.tag_id = tags.id` has no port
filter, so a foreign tag's name + color render in the requesting port.
Helper now batch-validates tagIds belong to portId before insert.
5. MEDIUM — /api/v1/custom-fields/[entityId] PUT had no withPermission
gate (any role, including viewer, could write) and didn't validate
that the URL entityId pointed at a port-scoped entity of the field
definition's entityType. Route now uses
withPermission('clients','view'/'edit',…); service validates the
entityId per resolved entityType (client/interest/berth/yacht/company)
against portId.
Test mocks updated to cover the new entity-port-scope check.
818 vitest tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -8,7 +8,7 @@ import { reorderWaitingListSchema } from '@/lib/validators/interests';
|
||||
import { getWaitingList, updateWaitingList } from '@/lib/services/berths.service';
|
||||
import { errorResponse, NotFoundError } from '@/lib/errors';
|
||||
import { db } from '@/lib/db';
|
||||
import { berthWaitingList } from '@/lib/db/schema/berths';
|
||||
import { berths, berthWaitingList } from '@/lib/db/schema/berths';
|
||||
|
||||
// GET /api/v1/berths/[id]/waiting-list
|
||||
export const GET = withAuth(
|
||||
@@ -47,11 +47,17 @@ export const PATCH = withAuth(
|
||||
const body = await parseBody(req, reorderWaitingListSchema);
|
||||
const berthId = params.id!;
|
||||
|
||||
// Tenant scope: refuse to reorder a foreign-port berth's waiting
|
||||
// list. The route's URL id and the entry id are otherwise enough
|
||||
// for any user with manage_waiting_list to mutate any tenant's
|
||||
// queue ordering.
|
||||
const berthRow = await db.query.berths.findFirst({
|
||||
where: and(eq(berths.id, berthId), eq(berths.portId, ctx.portId)),
|
||||
});
|
||||
if (!berthRow) throw new NotFoundError('Berth');
|
||||
|
||||
const entry = await db.query.berthWaitingList.findFirst({
|
||||
where: and(
|
||||
eq(berthWaitingList.id, body.entryId),
|
||||
eq(berthWaitingList.berthId, berthId),
|
||||
),
|
||||
where: and(eq(berthWaitingList.id, body.entryId), eq(berthWaitingList.berthId, berthId)),
|
||||
});
|
||||
if (!entry) throw new NotFoundError('Waiting list entry');
|
||||
|
||||
|
||||
@@ -1,11 +1,18 @@
|
||||
import { NextRequest, NextResponse } from 'next/server';
|
||||
|
||||
import { withAuth } from '@/lib/api/helpers';
|
||||
import { withAuth, withPermission } from '@/lib/api/helpers';
|
||||
import { errorResponse, NotFoundError } from '@/lib/errors';
|
||||
import { setValuesSchema } from '@/lib/validators/custom-fields';
|
||||
import { getValues, setValues } from '@/lib/services/custom-fields.service';
|
||||
|
||||
export const GET = withAuth(async (_req: NextRequest, ctx, params) => {
|
||||
// Custom-field values live on top of a port-scoped entity (client, yacht,
|
||||
// interest, berth, company). Reading the values is in scope for any role
|
||||
// that can view clients (the most common surface); writing requires the
|
||||
// equivalent edit permission. The service-layer also re-validates the
|
||||
// entityId against the field definition's entityType + portId so a
|
||||
// caller cannot poke values onto an arbitrary or foreign-port entity.
|
||||
export const GET = withAuth(
|
||||
withPermission('clients', 'view', async (_req: NextRequest, ctx, params) => {
|
||||
try {
|
||||
const { entityId } = params;
|
||||
if (!entityId) throw new NotFoundError('Entity');
|
||||
@@ -15,9 +22,11 @@ export const GET = withAuth(async (_req: NextRequest, ctx, params) => {
|
||||
} catch (error) {
|
||||
return errorResponse(error);
|
||||
}
|
||||
});
|
||||
}),
|
||||
);
|
||||
|
||||
export const PUT = withAuth(async (req: NextRequest, ctx, params) => {
|
||||
export const PUT = withAuth(
|
||||
withPermission('clients', 'edit', async (req: NextRequest, ctx, params) => {
|
||||
try {
|
||||
const { entityId } = params;
|
||||
if (!entityId) throw new NotFoundError('Entity');
|
||||
@@ -42,4 +51,5 @@ export const PUT = withAuth(async (req: NextRequest, ctx, params) => {
|
||||
} catch (error) {
|
||||
return errorResponse(error);
|
||||
}
|
||||
});
|
||||
}),
|
||||
);
|
||||
|
||||
@@ -2,10 +2,11 @@ import { and, eq, gte, lte, inArray } from 'drizzle-orm';
|
||||
|
||||
import { db } from '@/lib/db';
|
||||
import { berths, berthTags, berthWaitingList, berthMaintenanceLog } from '@/lib/db/schema/berths';
|
||||
import { clients } from '@/lib/db/schema/clients';
|
||||
import { tags } from '@/lib/db/schema/system';
|
||||
import { createAuditLog, type AuditMeta } from '@/lib/audit';
|
||||
import { diffEntity } from '@/lib/entity-diff';
|
||||
import { NotFoundError } from '@/lib/errors';
|
||||
import { NotFoundError, ValidationError } from '@/lib/errors';
|
||||
import { buildListQuery } from '@/lib/db/query-builder';
|
||||
import { emitToRoom } from '@/lib/socket/server';
|
||||
import { setEntityTags } from '@/lib/services/entity-tags.helper';
|
||||
@@ -401,6 +402,21 @@ export async function updateWaitingList(
|
||||
});
|
||||
if (!existing) throw new NotFoundError('Berth');
|
||||
|
||||
// Validate every supplied clientId belongs to portId. Without this
|
||||
// check, a port-A admin could insert port-B clientIds into the
|
||||
// waiting list — corrupting reportable data and creating a join
|
||||
// surface that hydrates foreign-tenant client rows.
|
||||
if (data.entries.length > 0) {
|
||||
const clientIds = [...new Set(data.entries.map((e) => e.clientId))];
|
||||
const validClients = await db
|
||||
.select({ id: clients.id })
|
||||
.from(clients)
|
||||
.where(and(inArray(clients.id, clientIds), eq(clients.portId, portId)));
|
||||
if (validClients.length !== clientIds.length) {
|
||||
throw new ValidationError('One or more clients are not in this port');
|
||||
}
|
||||
}
|
||||
|
||||
// Replace entire waiting list
|
||||
await db.delete(berthWaitingList).where(eq(berthWaitingList.berthId, id));
|
||||
|
||||
|
||||
@@ -266,6 +266,57 @@ export async function setValues(
|
||||
throw new ValidationError('Custom field validation failed', errors);
|
||||
}
|
||||
|
||||
// Tenant scope: verify entityId actually points at a port-scoped row of
|
||||
// the entity type the field definitions target. Without this gate, any
|
||||
// authenticated user could write custom-field rows pointing at arbitrary
|
||||
// entityIds (or none at all) — polluting customFieldValues and creating
|
||||
// a join surface that could later leak data.
|
||||
const entityTypes = new Set(
|
||||
values
|
||||
.map((v) => definitionMap.get(v.fieldId)?.entityType)
|
||||
.filter((t): t is string => Boolean(t)),
|
||||
);
|
||||
for (const entityType of entityTypes) {
|
||||
const { eq: drizzleEq, and: drizzleAnd } = await import('drizzle-orm');
|
||||
let exists = false;
|
||||
if (entityType === 'client') {
|
||||
const { clients } = await import('@/lib/db/schema/clients');
|
||||
const row = await db.query.clients.findFirst({
|
||||
where: drizzleAnd(drizzleEq(clients.id, entityId), drizzleEq(clients.portId, portId)),
|
||||
});
|
||||
exists = Boolean(row);
|
||||
} else if (entityType === 'interest') {
|
||||
const { interests } = await import('@/lib/db/schema/interests');
|
||||
const row = await db.query.interests.findFirst({
|
||||
where: drizzleAnd(drizzleEq(interests.id, entityId), drizzleEq(interests.portId, portId)),
|
||||
});
|
||||
exists = Boolean(row);
|
||||
} else if (entityType === 'berth') {
|
||||
const { berths } = await import('@/lib/db/schema/berths');
|
||||
const row = await db.query.berths.findFirst({
|
||||
where: drizzleAnd(drizzleEq(berths.id, entityId), drizzleEq(berths.portId, portId)),
|
||||
});
|
||||
exists = Boolean(row);
|
||||
} else if (entityType === 'yacht') {
|
||||
const { yachts } = await import('@/lib/db/schema/yachts');
|
||||
const row = await db.query.yachts.findFirst({
|
||||
where: drizzleAnd(drizzleEq(yachts.id, entityId), drizzleEq(yachts.portId, portId)),
|
||||
});
|
||||
exists = Boolean(row);
|
||||
} else if (entityType === 'company') {
|
||||
const { companies } = await import('@/lib/db/schema/companies');
|
||||
const row = await db.query.companies.findFirst({
|
||||
where: drizzleAnd(drizzleEq(companies.id, entityId), drizzleEq(companies.portId, portId)),
|
||||
});
|
||||
exists = Boolean(row);
|
||||
} else {
|
||||
throw new ValidationError(`Unsupported custom-field entity type: ${entityType}`);
|
||||
}
|
||||
if (!exists) {
|
||||
throw new ValidationError(`${entityType} not found in this port`);
|
||||
}
|
||||
}
|
||||
|
||||
// Upsert all values
|
||||
const results = await Promise.all(
|
||||
values.map(async ({ fieldId, value }) => {
|
||||
|
||||
@@ -13,11 +13,14 @@
|
||||
* the audit-log payload to `newValue: { tagIds }` across all entity types.
|
||||
*/
|
||||
|
||||
import { eq, type Column } from 'drizzle-orm';
|
||||
import { and, eq, inArray, type Column } from 'drizzle-orm';
|
||||
import type { PgTable } from 'drizzle-orm/pg-core';
|
||||
|
||||
import { db } from '@/lib/db';
|
||||
import { tags } from '@/lib/db/schema/system';
|
||||
import { withTransaction } from '@/lib/db/utils';
|
||||
import { createAuditLog, type AuditMeta } from '@/lib/audit';
|
||||
import { ValidationError } from '@/lib/errors';
|
||||
import { emitToRoom } from '@/lib/socket/server';
|
||||
|
||||
interface SetEntityTagsArgs<TJoin extends PgTable> {
|
||||
@@ -51,6 +54,23 @@ export async function setEntityTags<TJoin extends PgTable>(
|
||||
): Promise<{ entityId: string; tagIds: string[] }> {
|
||||
const { joinTable, entityColumn, tagColumn, entityId, portId, tagIds, meta, entityType } = args;
|
||||
|
||||
// Tenant scope: every supplied tagId must belong to the caller's port.
|
||||
// The tags table is per-port (`tags.port_id`) but the join tables only
|
||||
// have a single-column FK to `tags.id` — without this guard, a port-A
|
||||
// caller could splice a port-B tag UUID onto their own entity. The
|
||||
// entity's GET handler joins `tags ON join.tag_id = tags.id` with no
|
||||
// port filter, so the foreign tag's name and color render in port A.
|
||||
if (tagIds.length > 0) {
|
||||
const uniqueTagIds = [...new Set(tagIds)];
|
||||
const validTags = await db
|
||||
.select({ id: tags.id })
|
||||
.from(tags)
|
||||
.where(and(inArray(tags.id, uniqueTagIds), eq(tags.portId, portId)));
|
||||
if (validTags.length !== uniqueTagIds.length) {
|
||||
throw new ValidationError('One or more tags are not in this port');
|
||||
}
|
||||
}
|
||||
|
||||
await withTransaction(async (tx) => {
|
||||
await tx.delete(joinTable).where(eq(entityColumn, entityId));
|
||||
if (tagIds.length > 0) {
|
||||
|
||||
@@ -147,7 +147,16 @@ export async function generateRecommendations(interestId: string, portId: string
|
||||
|
||||
// ─── List Recommendations ─────────────────────────────────────────────────────
|
||||
|
||||
export async function listRecommendations(interestId: string, _portId: string) {
|
||||
export async function listRecommendations(interestId: string, portId: string) {
|
||||
// Verify the interest belongs to the caller's port. Without this gate,
|
||||
// any user with `interests:view` could pass a foreign-port interestId
|
||||
// and receive that tenant's recommended berths (mooring numbers,
|
||||
// dimensions, status — operational data they should not see).
|
||||
const interest = await db.query.interests.findFirst({
|
||||
where: and(eq(interests.id, interestId), eq(interests.portId, portId)),
|
||||
});
|
||||
if (!interest) throw new NotFoundError('Interest');
|
||||
|
||||
const rows = await db
|
||||
.select({
|
||||
id: berthRecommendations.id,
|
||||
@@ -167,7 +176,7 @@ export async function listRecommendations(interestId: string, _portId: string) {
|
||||
})
|
||||
.from(berthRecommendations)
|
||||
.innerJoin(berths, eq(berthRecommendations.berthId, berths.id))
|
||||
.where(eq(berthRecommendations.interestId, interestId))
|
||||
.where(and(eq(berthRecommendations.interestId, interestId), eq(berths.portId, portId)))
|
||||
.orderBy(berthRecommendations.matchScore);
|
||||
|
||||
return rows.reverse(); // highest score first
|
||||
|
||||
@@ -2,6 +2,7 @@ import { and, eq, lte, gte, desc, asc, inArray, sql, isNull } from 'drizzle-orm'
|
||||
|
||||
import { db } from '@/lib/db';
|
||||
import { reminders, interests, clients } from '@/lib/db/schema';
|
||||
import { berths } from '@/lib/db/schema/berths';
|
||||
import { createAuditLog, type AuditMeta } from '@/lib/audit';
|
||||
import { NotFoundError, ValidationError } from '@/lib/errors';
|
||||
import { emitToRoom } from '@/lib/socket/server';
|
||||
@@ -107,6 +108,50 @@ export async function getUpcomingReminders(portId: string, days: number = 14) {
|
||||
.orderBy(asc(reminders.dueAt));
|
||||
}
|
||||
|
||||
/**
|
||||
* Validate that the supplied subject FKs (clientId / interestId / berthId)
|
||||
* all point at rows inside the caller's port. Without this guard, a
|
||||
* reminder created with a foreign-port FK would later be hydrated with
|
||||
* `with: { client, interest, berth }` joins (no port filter on the
|
||||
* relation) — leaking the foreign-tenant rows back to the attacker.
|
||||
*/
|
||||
async function assertReminderFksInPort(
|
||||
portId: string,
|
||||
fks: { clientId?: string | null; interestId?: string | null; berthId?: string | null },
|
||||
): Promise<void> {
|
||||
const checks: Array<Promise<void>> = [];
|
||||
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.interestId) {
|
||||
checks.push(
|
||||
db.query.interests
|
||||
.findFirst({
|
||||
where: and(eq(interests.id, fks.interestId), eq(interests.portId, portId)),
|
||||
})
|
||||
.then((row) => {
|
||||
if (!row) throw new ValidationError('interestId 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');
|
||||
}),
|
||||
);
|
||||
}
|
||||
await Promise.all(checks);
|
||||
}
|
||||
|
||||
// ─── CRUD ────────────────────────────────────────────────────────────────────
|
||||
|
||||
export async function getReminder(id: string, portId: string) {
|
||||
@@ -119,6 +164,12 @@ export async function getReminder(id: string, portId: string) {
|
||||
}
|
||||
|
||||
export async function createReminder(portId: string, data: CreateReminderInput, meta: AuditMeta) {
|
||||
await assertReminderFksInPort(portId, {
|
||||
clientId: data.clientId,
|
||||
interestId: data.interestId,
|
||||
berthId: data.berthId,
|
||||
});
|
||||
|
||||
const [reminder] = await db
|
||||
.insert(reminders)
|
||||
.values({
|
||||
@@ -186,6 +237,13 @@ export async function updateReminder(
|
||||
if (data.interestId !== undefined) updates.interestId = data.interestId;
|
||||
if (data.berthId !== undefined) updates.berthId = data.berthId;
|
||||
|
||||
// Re-validate any subject-FK changes against the caller's port.
|
||||
await assertReminderFksInPort(portId, {
|
||||
clientId: data.clientId,
|
||||
interestId: data.interestId,
|
||||
berthId: data.berthId,
|
||||
});
|
||||
|
||||
const [updated] = await db
|
||||
.update(reminders)
|
||||
.set(updates)
|
||||
|
||||
@@ -12,6 +12,13 @@ vi.mock('@/lib/db', () => ({
|
||||
db: {
|
||||
query: {
|
||||
customFieldDefinitions: { findMany: vi.fn(), findFirst: vi.fn() },
|
||||
// Entity-port-scope checks added by the security fix; default to a
|
||||
// truthy row so existing assertions still focus on validation logic.
|
||||
clients: { findFirst: vi.fn().mockResolvedValue({ id: 'entity-1', portId: 'port-1' }) },
|
||||
interests: { findFirst: vi.fn().mockResolvedValue({ id: 'entity-1', portId: 'port-1' }) },
|
||||
berths: { findFirst: vi.fn().mockResolvedValue({ id: 'entity-1', portId: 'port-1' }) },
|
||||
yachts: { findFirst: vi.fn().mockResolvedValue({ id: 'entity-1', portId: 'port-1' }) },
|
||||
companies: { findFirst: vi.fn().mockResolvedValue({ id: 'entity-1', portId: 'port-1' }) },
|
||||
},
|
||||
insert: vi.fn(),
|
||||
update: vi.fn(),
|
||||
@@ -20,6 +27,12 @@ vi.mock('@/lib/db', () => ({
|
||||
},
|
||||
}));
|
||||
|
||||
vi.mock('@/lib/db/schema/clients', () => ({ clients: {} }));
|
||||
vi.mock('@/lib/db/schema/interests', () => ({ interests: {} }));
|
||||
vi.mock('@/lib/db/schema/berths', () => ({ berths: {} }));
|
||||
vi.mock('@/lib/db/schema/yachts', () => ({ yachts: {} }));
|
||||
vi.mock('@/lib/db/schema/companies', () => ({ companies: {} }));
|
||||
|
||||
vi.mock('@/lib/audit', () => ({
|
||||
createAuditLog: vi.fn().mockResolvedValue(undefined),
|
||||
}));
|
||||
@@ -84,7 +97,11 @@ beforeEach(() => {
|
||||
});
|
||||
|
||||
/** Convenience: call setValues with a single field/value pair. */
|
||||
async function validate(fieldType: string, value: unknown, extras?: { isRequired?: boolean; selectOptions?: string[] }) {
|
||||
async function validate(
|
||||
fieldType: string,
|
||||
value: unknown,
|
||||
extras?: { isRequired?: boolean; selectOptions?: string[] },
|
||||
) {
|
||||
(db.query.customFieldDefinitions.findMany as ReturnType<typeof vi.fn>).mockResolvedValue([
|
||||
makeDefinition(fieldType, extras),
|
||||
]);
|
||||
@@ -182,7 +199,9 @@ describe('custom field validation — select', () => {
|
||||
});
|
||||
|
||||
it('rejects an option not in the list', async () => {
|
||||
await expect(validate('select', 'XL', { selectOptions: options })).rejects.toBeInstanceOf(ValidationError);
|
||||
await expect(validate('select', 'XL', { selectOptions: options })).rejects.toBeInstanceOf(
|
||||
ValidationError,
|
||||
);
|
||||
});
|
||||
|
||||
it('error message lists the valid options', async () => {
|
||||
@@ -203,11 +222,15 @@ describe('custom field validation — select', () => {
|
||||
|
||||
describe('custom field validation — required vs optional null', () => {
|
||||
it('required field: null value → throws ValidationError', async () => {
|
||||
await expect(validate('text', null, { isRequired: true })).rejects.toBeInstanceOf(ValidationError);
|
||||
await expect(validate('text', null, { isRequired: true })).rejects.toBeInstanceOf(
|
||||
ValidationError,
|
||||
);
|
||||
});
|
||||
|
||||
it('required field: undefined value → throws ValidationError', async () => {
|
||||
await expect(validate('text', undefined, { isRequired: true })).rejects.toBeInstanceOf(ValidationError);
|
||||
await expect(validate('text', undefined, { isRequired: true })).rejects.toBeInstanceOf(
|
||||
ValidationError,
|
||||
);
|
||||
});
|
||||
|
||||
it('non-required field: null value → succeeds (no error)', async () => {
|
||||
|
||||
Reference in New Issue
Block a user