From d4b3a1338f87514bda9260043fe788def9cf7c41 Mon Sep 17 00:00:00 2001 From: Matt Ciaccio Date: Tue, 5 May 2026 05:31:33 +0200 Subject: [PATCH] fix(security): scope berth-pdf service entrypoints by portId MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-merge security review caught a cross-tenant authorization bypass in the per-berth PDF endpoints (HIGH severity, confidence 10): GET /api/v1/berths/[id]/pdf-versions POST /api/v1/berths/[id]/pdf-versions POST /api/v1/berths/[id]/pdf-upload-url POST /api/v1/berths/[id]/pdf-versions/[versionId]/rollback POST /api/v1/berths/[id]/pdf-versions/parse-results/apply Each handler looked up the target berth by id only — `eq(berths.id, ...)`. withAuth resolves ctx.portId from the user-controlled X-Port-Id header (only verifying the user has SOME role on that port), and withPermission('berths', 'view'|'edit', ...) is a coarse capability check, not a row-level grant. A rep with berths:edit on Port A could supply a Port B berth UUID and: - list + receive 15-min presigned download URLs to every PDF version - mint an upload URL targeting `berths//uploads/...` - POST a new version (overwriting current_pdf_version_id on foreign berth) - rollback to any prior version on a foreign berth - apply rep-confirmed parse-result fields onto a foreign berth's columns Sibling routes (waiting-list etc.) already pair the id filter with `eq(berths.portId, ctx.portId)`, so this was an omission, not design. Fix: - Push `portId: string` into uploadBerthPdf, listBerthPdfVersions, rollbackToVersion, applyParseResults, reconcilePdfWithBerth. - Each function now filters the berth lookup with `and(eq(berths.id, ...), eq(berths.portId, portId))` and throws NotFoundError on mismatch (no foreign-port disclosure). - Inline the same `and(...)` filter in the pdf-upload-url handler. - Every handler passes ctx.portId through. Coverage: - New `cross-port tenant guard` test exercises every entrypoint with a foreign-port id and asserts NotFoundError. - 1164/1164 vitest passing. Typecheck clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../v1/berths/[id]/pdf-upload-url/handlers.ts | 12 +- .../[versionId]/rollback/handlers.ts | 4 +- .../v1/berths/[id]/pdf-versions/handlers.ts | 5 +- .../parse-results/apply/handlers.ts | 9 +- src/lib/services/berth-pdf.service.ts | 41 ++++- tests/integration/berth-pdf-versions.test.ts | 153 +++++++++++++----- 6 files changed, 170 insertions(+), 54 deletions(-) diff --git a/src/app/api/v1/berths/[id]/pdf-upload-url/handlers.ts b/src/app/api/v1/berths/[id]/pdf-upload-url/handlers.ts index 8047650..f77ad9a 100644 --- a/src/app/api/v1/berths/[id]/pdf-upload-url/handlers.ts +++ b/src/app/api/v1/berths/[id]/pdf-upload-url/handlers.ts @@ -12,7 +12,7 @@ import { NextResponse } from 'next/server'; import { type RouteHandler } from '@/lib/api/helpers'; import { db } from '@/lib/db'; import { berths } from '@/lib/db/schema/berths'; -import { eq } from 'drizzle-orm'; +import { and, eq } from 'drizzle-orm'; import { errorResponse, NotFoundError, ValidationError } from '@/lib/errors'; import { getMaxUploadMb } from '@/lib/services/berth-pdf.service'; import { getStorageBackend } from '@/lib/storage'; @@ -24,13 +24,19 @@ interface PostBody { sizeBytes?: number; } -export const postHandler: RouteHandler = async (req, _ctx, params) => { +export const postHandler: RouteHandler = async (req, ctx, params) => { try { const body = (await req.json()) as Partial; const fileName = (body.fileName ?? '').trim(); if (!fileName) throw new ValidationError('fileName is required'); - const berthRow = await db.query.berths.findFirst({ where: eq(berths.id, params.id!) }); + // Tenant-scoped berth lookup. Without `eq(berths.portId, ctx.portId)` a + // rep with berths:edit on port A could mint an upload URL targeting a + // port-B berth (the storage key namespace would land under that berth's + // id, leaking access). + const berthRow = await db.query.berths.findFirst({ + where: and(eq(berths.id, params.id!), eq(berths.portId, ctx.portId)), + }); if (!berthRow) throw new NotFoundError('Berth'); const maxMb = await getMaxUploadMb(berthRow.portId); diff --git a/src/app/api/v1/berths/[id]/pdf-versions/[versionId]/rollback/handlers.ts b/src/app/api/v1/berths/[id]/pdf-versions/[versionId]/rollback/handlers.ts index 30e67e0..a816a23 100644 --- a/src/app/api/v1/berths/[id]/pdf-versions/[versionId]/rollback/handlers.ts +++ b/src/app/api/v1/berths/[id]/pdf-versions/[versionId]/rollback/handlers.ts @@ -4,9 +4,9 @@ import { type RouteHandler } from '@/lib/api/helpers'; import { errorResponse } from '@/lib/errors'; import { rollbackToVersion } from '@/lib/services/berth-pdf.service'; -export const postHandler: RouteHandler = async (_req, _ctx, params) => { +export const postHandler: RouteHandler = async (_req, ctx, params) => { try { - const result = await rollbackToVersion(params.id!, params.versionId!); + const result = await rollbackToVersion(params.id!, params.versionId!, ctx.portId); return NextResponse.json({ data: result }); } catch (error) { return errorResponse(error); diff --git a/src/app/api/v1/berths/[id]/pdf-versions/handlers.ts b/src/app/api/v1/berths/[id]/pdf-versions/handlers.ts index 6fc895d..53355ff 100644 --- a/src/app/api/v1/berths/[id]/pdf-versions/handlers.ts +++ b/src/app/api/v1/berths/[id]/pdf-versions/handlers.ts @@ -25,9 +25,9 @@ interface PostBody { }; } -export const getHandler: RouteHandler = async (_req, _ctx, params) => { +export const getHandler: RouteHandler = async (_req, ctx, params) => { try { - const versions = await listBerthPdfVersions(params.id!); + const versions = await listBerthPdfVersions(params.id!, ctx.portId); return NextResponse.json({ data: versions }); } catch (error) { return errorResponse(error); @@ -48,6 +48,7 @@ export const postHandler: RouteHandler = async (req, ctx, params) => { } const result = await uploadBerthPdf({ berthId: params.id!, + portId: ctx.portId, storageKey: body.storageKey, fileName: body.fileName, fileSizeBytes: body.fileSizeBytes, diff --git a/src/app/api/v1/berths/[id]/pdf-versions/parse-results/apply/handlers.ts b/src/app/api/v1/berths/[id]/pdf-versions/parse-results/apply/handlers.ts index 078d73d..a6bc022 100644 --- a/src/app/api/v1/berths/[id]/pdf-versions/parse-results/apply/handlers.ts +++ b/src/app/api/v1/berths/[id]/pdf-versions/parse-results/apply/handlers.ts @@ -9,14 +9,19 @@ interface PostBody { fieldsToApply: Partial; } -export const postHandler: RouteHandler = async (req, _ctx, params) => { +export const postHandler: RouteHandler = async (req, ctx, params) => { try { const body = (await req.json()) as Partial; if (!body.versionId) throw new ValidationError('versionId is required'); if (!body.fieldsToApply || typeof body.fieldsToApply !== 'object') { throw new ValidationError('fieldsToApply must be an object'); } - const result = await applyParseResults(params.id!, body.versionId, body.fieldsToApply); + const result = await applyParseResults( + params.id!, + body.versionId, + body.fieldsToApply, + ctx.portId, + ); return NextResponse.json({ data: result }); } catch (error) { return errorResponse(error); diff --git a/src/lib/services/berth-pdf.service.ts b/src/lib/services/berth-pdf.service.ts index dd6e79b..a6c6105 100644 --- a/src/lib/services/berth-pdf.service.ts +++ b/src/lib/services/berth-pdf.service.ts @@ -137,6 +137,13 @@ export async function getMaxUploadMb(portId: string): Promise { export interface UploadBerthPdfArgs { berthId: string; + /** + * Acting tenant. Every public service entrypoint requires this so the berth + * lookup can be scoped to `(berth.id, port_id)` — without it a rep with + * berths:edit on port A could supply a port B berth UUID and write/read + * cross-tenant data. NotFoundError on mismatch. + */ + portId: string; /** Already-uploaded storage key (the upload-url endpoint generated it) OR * undefined to make this service compute one. */ storageKey?: string; @@ -175,7 +182,11 @@ export interface UploadBerthPdfResult { */ export async function uploadBerthPdf(args: UploadBerthPdfArgs): Promise { // 1. Resolve the berth + port for size-cap lookup. - const berthRow = await db.query.berths.findFirst({ where: eq(berths.id, args.berthId) }); + // Tenant-scoped lookup — NotFoundError when the berth lives in a different + // port so a rep on port A cannot upload PDFs against port B's berths. + const berthRow = await db.query.berths.findFirst({ + where: and(eq(berths.id, args.berthId), eq(berths.portId, args.portId)), + }); if (!berthRow) throw new NotFoundError('Berth'); const maxMb = await getMaxUploadMb(berthRow.portId); const maxBytes = maxMb * 1024 * 1024; @@ -378,8 +389,12 @@ function serializeParseResult(parse: ParseResult): Record { export async function reconcilePdfWithBerth( berthId: string, parsed: ParseResult, + /** Tenant scope. NotFoundError on cross-port lookups. */ + portId: string, ): Promise { - const berthRow = await db.query.berths.findFirst({ where: eq(berths.id, berthId) }); + const berthRow = await db.query.berths.findFirst({ + where: and(eq(berths.id, berthId), eq(berths.portId, portId)), + }); if (!berthRow) throw new NotFoundError('Berth'); const fields = parsed.fields; @@ -440,9 +455,13 @@ export async function applyParseResults( berthId: string, versionId: string, fieldsToApply: Partial, + /** Tenant scope. NotFoundError when berth lives in a different port. */ + portId: string, opts: { confirmMooringMismatch?: boolean } = {}, ): Promise<{ updatedFields: Array }> { - const berthRow = await db.query.berths.findFirst({ where: eq(berths.id, berthId) }); + const berthRow = await db.query.berths.findFirst({ + where: and(eq(berths.id, berthId), eq(berths.portId, portId)), + }); if (!berthRow) throw new NotFoundError('Berth'); const versionRow = await db.query.berthPdfVersions.findFirst({ where: and(eq(berthPdfVersions.id, versionId), eq(berthPdfVersions.berthId, berthId)), @@ -520,8 +539,14 @@ export interface BerthPdfVersionListItem { parseEngine: ParserEngine | null; } -export async function listBerthPdfVersions(berthId: string): Promise { - const berthRow = await db.query.berths.findFirst({ where: eq(berths.id, berthId) }); +export async function listBerthPdfVersions( + berthId: string, + /** Tenant scope. NotFoundError when berth lives in a different port. */ + portId: string, +): Promise { + const berthRow = await db.query.berths.findFirst({ + where: and(eq(berths.id, berthId), eq(berths.portId, portId)), + }); if (!berthRow) throw new NotFoundError('Berth'); const rows = await db @@ -569,12 +594,16 @@ export async function listBerthPdfVersions(berthId: string): Promise { const versionRow = await db.query.berthPdfVersions.findFirst({ where: and(eq(berthPdfVersions.id, versionId), eq(berthPdfVersions.berthId, berthId)), }); if (!versionRow) throw new NotFoundError('Berth PDF version'); - const berthRow = await db.query.berths.findFirst({ where: eq(berths.id, berthId) }); + const berthRow = await db.query.berths.findFirst({ + where: and(eq(berths.id, berthId), eq(berths.portId, portId)), + }); if (!berthRow) throw new NotFoundError('Berth'); if (berthRow.currentPdfVersionId === versionId) { diff --git a/tests/integration/berth-pdf-versions.test.ts b/tests/integration/berth-pdf-versions.test.ts index 05d6503..624f36a 100644 --- a/tests/integration/berth-pdf-versions.test.ts +++ b/tests/integration/berth-pdf-versions.test.ts @@ -16,6 +16,7 @@ import { afterEach, beforeEach, describe, expect, it } from 'vitest'; import { applyParseResults, + listBerthPdfVersions, reconcilePdfWithBerth, rollbackToVersion, uploadBerthPdf, @@ -69,6 +70,7 @@ describe('uploadBerthPdf', () => { const result = await uploadBerthPdf({ berthId: berth.id, + portId: port.id, buffer: fakePdf(), fileName: 'A1.pdf', uploadedBy: 'test-user', @@ -94,6 +96,7 @@ describe('uploadBerthPdf', () => { await expect( uploadBerthPdf({ berthId: berth.id, + portId: port.id, buffer: Buffer.from('not a pdf at all'), fileName: 'spoof.pdf', uploadedBy: 'test-user', @@ -106,12 +109,14 @@ describe('uploadBerthPdf', () => { const berth = await makeBerth({ portId: port.id }); await uploadBerthPdf({ berthId: berth.id, + portId: port.id, buffer: fakePdf(), fileName: 'v1.pdf', uploadedBy: 'test', }); const second = await uploadBerthPdf({ berthId: berth.id, + portId: port.id, buffer: fakePdf(), fileName: 'v2.pdf', uploadedBy: 'test', @@ -127,15 +132,19 @@ describe('reconcilePdfWithBerth', () => { portId: port.id, overrides: { mooringNumber: 'A1', lengthFt: null, bowFacing: null }, }); - const result = await reconcilePdfWithBerth(berth.id, { - engine: 'ocr', - fields: { - lengthFt: { value: 200, confidence: 0.9, engine: 'ocr' }, - bowFacing: { value: 'East', confidence: 0.9, engine: 'ocr' }, + const result = await reconcilePdfWithBerth( + berth.id, + { + engine: 'ocr', + fields: { + lengthFt: { value: 200, confidence: 0.9, engine: 'ocr' }, + bowFacing: { value: 'East', confidence: 0.9, engine: 'ocr' }, + }, + meanConfidence: 0.9, + warnings: [], }, - meanConfidence: 0.9, - warnings: [], - }); + port.id, + ); const fields = result.autoApplied.map((a) => a.field).sort(); expect(fields).toEqual(['bowFacing', 'lengthFt']); expect(result.conflicts).toHaveLength(0); @@ -147,15 +156,19 @@ describe('reconcilePdfWithBerth', () => { portId: port.id, overrides: { mooringNumber: 'A1', lengthFt: '100', bowFacing: 'West' }, }); - const result = await reconcilePdfWithBerth(berth.id, { - engine: 'ocr', - fields: { - lengthFt: { value: 200, confidence: 0.8, engine: 'ocr' }, - bowFacing: { value: 'East', confidence: 0.8, engine: 'ocr' }, + const result = await reconcilePdfWithBerth( + berth.id, + { + engine: 'ocr', + fields: { + lengthFt: { value: 200, confidence: 0.8, engine: 'ocr' }, + bowFacing: { value: 'East', confidence: 0.8, engine: 'ocr' }, + }, + meanConfidence: 0.8, + warnings: [], }, - meanConfidence: 0.8, - warnings: [], - }); + port.id, + ); expect(result.conflicts.map((c) => c.field).sort()).toEqual(['bowFacing', 'lengthFt']); }); @@ -165,14 +178,18 @@ describe('reconcilePdfWithBerth', () => { portId: port.id, overrides: { mooringNumber: 'A1', lengthFt: '200' }, }); - const result = await reconcilePdfWithBerth(berth.id, { - engine: 'ocr', - fields: { - lengthFt: { value: 201, confidence: 0.9, engine: 'ocr' }, // +0.5% + const result = await reconcilePdfWithBerth( + berth.id, + { + engine: 'ocr', + fields: { + lengthFt: { value: 201, confidence: 0.9, engine: 'ocr' }, // +0.5% + }, + meanConfidence: 0.9, + warnings: [], }, - meanConfidence: 0.9, - warnings: [], - }); + port.id, + ); expect(result.conflicts).toHaveLength(0); expect(result.autoApplied).toHaveLength(0); }); @@ -183,14 +200,18 @@ describe('reconcilePdfWithBerth', () => { portId: port.id, overrides: { mooringNumber: 'A1' }, }); - const result = await reconcilePdfWithBerth(berth.id, { - engine: 'ocr', - fields: { - mooringNumber: { value: 'B5', confidence: 0.9, engine: 'ocr' }, + const result = await reconcilePdfWithBerth( + berth.id, + { + engine: 'ocr', + fields: { + mooringNumber: { value: 'B5', confidence: 0.9, engine: 'ocr' }, + }, + meanConfidence: 0.9, + warnings: [], }, - meanConfidence: 0.9, - warnings: [], - }); + port.id, + ); expect(result.warnings.some((w) => /B5/.test(w) && /A1/.test(w))).toBe(true); }); }); @@ -204,18 +225,24 @@ describe('applyParseResults', () => { }); const upload = await uploadBerthPdf({ berthId: berth.id, + portId: port.id, buffer: fakePdf(), fileName: 'A1.pdf', uploadedBy: 'test', }); - await applyParseResults(berth.id, upload.versionId, { - lengthFt: 200, - bowFacing: 'East', - // unknown / non-allowlisted column should be silently dropped: - // @ts-expect-error — testing the allowlist - hackThePlanet: 'pwn', - }); + await applyParseResults( + berth.id, + upload.versionId, + { + lengthFt: 200, + bowFacing: 'East', + // unknown / non-allowlisted column should be silently dropped: + // @ts-expect-error — testing the allowlist + hackThePlanet: 'pwn', + }, + port.id, + ); const refreshed = await db.query.berths.findFirst({ where: eq(berths.id, berth.id) }); expect(refreshed?.lengthFt).toBe('200'); @@ -236,12 +263,14 @@ describe('rollbackToVersion', () => { const berth = await makeBerth({ portId: port.id }); const v1 = await uploadBerthPdf({ berthId: berth.id, + portId: port.id, buffer: fakePdf(), fileName: 'v1.pdf', uploadedBy: 'test', }); const v2 = await uploadBerthPdf({ berthId: berth.id, + portId: port.id, buffer: fakePdf(), fileName: 'v2.pdf', uploadedBy: 'test', @@ -250,7 +279,7 @@ describe('rollbackToVersion', () => { let refreshed = await db.query.berths.findFirst({ where: eq(berths.id, berth.id) }); expect(refreshed?.currentPdfVersionId).toBe(v2.versionId); - const result = await rollbackToVersion(berth.id, v1.versionId); + const result = await rollbackToVersion(berth.id, v1.versionId, port.id); expect(result.versionNumber).toBe(1); refreshed = await db.query.berths.findFirst({ where: eq(berths.id, berth.id) }); @@ -262,10 +291,56 @@ describe('rollbackToVersion', () => { const berth = await makeBerth({ portId: port.id }); const v1 = await uploadBerthPdf({ berthId: berth.id, + portId: port.id, buffer: fakePdf(), fileName: 'v1.pdf', uploadedBy: 'test', }); - await expect(rollbackToVersion(berth.id, v1.versionId)).rejects.toThrow(/already current/); + await expect(rollbackToVersion(berth.id, v1.versionId, port.id)).rejects.toThrow( + /already current/, + ); + }); +}); + +describe('cross-port tenant guard', () => { + it('rejects every berth-pdf service call when berthId belongs to a different port', async () => { + const portA = await makePort(); + const portB = await makePort(); + const berthA = await makeBerth({ portId: portA.id }); + + // Seed a version under port A so list/apply/rollback have something + // they could in theory return. + const v1 = await uploadBerthPdf({ + berthId: berthA.id, + portId: portA.id, + buffer: fakePdf(), + fileName: 'A.pdf', + uploadedBy: 'test', + }); + + // Port B caller passing port A's berth id must hit NotFoundError on + // every entrypoint — including read-only listing, which previously + // returned 15-min presigned download URLs to the foreign port's PDFs. + await expect(listBerthPdfVersions(berthA.id, portB.id)).rejects.toThrow(/Berth/); + await expect(rollbackToVersion(berthA.id, v1.versionId, portB.id)).rejects.toThrow(/Berth/); + await expect( + applyParseResults(berthA.id, v1.versionId, { lengthFt: 99 }, portB.id), + ).rejects.toThrow(/Berth/); + await expect( + uploadBerthPdf({ + berthId: berthA.id, + portId: portB.id, + buffer: fakePdf(), + fileName: 'B-cross.pdf', + uploadedBy: 'test', + }), + ).rejects.toThrow(/Berth/); + await expect( + reconcilePdfWithBerth( + berthA.id, + { engine: 'ocr', fields: {}, meanConfidence: 1, warnings: [] }, + portB.id, + ), + ).rejects.toThrow(/Berth/); }); });