diff --git a/apps/api/src/modules/accounting/deposit-fk-ownership.spec.ts b/apps/api/src/modules/accounting/deposit-fk-ownership.spec.ts new file mode 100644 index 0000000..0fd56e8 --- /dev/null +++ b/apps/api/src/modules/accounting/deposit-fk-ownership.spec.ts @@ -0,0 +1,58 @@ +import { describe, it, expect, vi } from 'vitest'; +import { BadRequestException } from '@nestjs/common'; +import { Test } from '@nestjs/testing'; +import { DepositService } from './deposit.service'; +import { DRIZZLE } from '../../database/database.module'; +import { WebhookService } from '../webhook/webhook.service'; +import { FolioService } from '../folio/folio.service'; + +const A = 'aaaaaaaa-0000-4000-a000-000000000001'; + +function mkDbSeq(rows: any[][]) { + let i = 0; + return { + select: vi.fn().mockReturnValue({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockImplementation(() => Promise.resolve(rows[i++] ?? [])), + }), + }), + insert: vi.fn().mockReturnValue({ + values: vi.fn().mockReturnValue({ returning: vi.fn().mockResolvedValue([{ id: 'd-1' }]) }), + }), + }; +} + +async function mkSvc(db: any) { + const mod = await Test.createTestingModule({ + providers: [ + DepositService, + { provide: DRIZZLE, useValue: db }, + { provide: WebhookService, useValue: { emit: vi.fn() } }, + { provide: FolioService, useValue: {} }, + ], + }).compile(); + return mod.get(DepositService); +} + +describe('DepositService — cross-tenant FK ownership', () => { + it('rejects when dto.reservationId belongs to another property', async () => { + const db = mkDbSeq([[]]); // reservation FK empty + const svc = await mkSvc(db); + await expect( + svc.recordDeposit({ propertyId: A, reservationId: 'foreign-r', amount: '100', currencyCode: 'USD' } as any), + ).rejects.toBeInstanceOf(BadRequestException); + expect(db.insert).not.toHaveBeenCalled(); + }); + + it('rejects when dto.paymentId belongs to another property (reservation OK)', async () => { + const db = mkDbSeq([ + [{ id: 'r-1' }], // reservation FK OK + [], // payment FK empty + ]); + const svc = await mkSvc(db); + await expect( + svc.recordDeposit({ propertyId: A, reservationId: 'r-1', paymentId: 'foreign-p', amount: '100', currencyCode: 'USD' } as any), + ).rejects.toBeInstanceOf(BadRequestException); + expect(db.insert).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/api/src/modules/accounting/deposit.service.ts b/apps/api/src/modules/accounting/deposit.service.ts index 97f49b0..636674c 100644 --- a/apps/api/src/modules/accounting/deposit.service.ts +++ b/apps/api/src/modules/accounting/deposit.service.ts @@ -6,7 +6,7 @@ import { } from '@nestjs/common'; import { eq, and, sql } from 'drizzle-orm'; import Decimal from 'decimal.js'; -import { depositLedgerEntries } from '@telivityhaip/database'; +import { depositLedgerEntries, reservations, payments } from '@telivityhaip/database'; import { DRIZZLE } from '../../database/database.module'; import { WebhookService } from '../webhook/webhook.service'; import { FolioService } from '../folio/folio.service'; @@ -34,6 +34,23 @@ export class DepositService { ) {} async recordDeposit(dto: RecordDepositDto) { + // FK ownership (security audit follow-on): caller-supplied reservationId + // and paymentId must belong to dto.propertyId. Schema FK only constrains + // the row id, so without this a deposit could be attached cross-tenant. + if (dto.reservationId) { + const [r] = await this.db + .select({ id: reservations.id }) + .from(reservations) + .where(and(eq(reservations.id, dto.reservationId), eq(reservations.propertyId, dto.propertyId))); + if (!r) throw new BadRequestException(`reservation ${dto.reservationId} not found in this property`); + } + if (dto.paymentId) { + const [p] = await this.db + .select({ id: payments.id }) + .from(payments) + .where(and(eq(payments.id, dto.paymentId), eq(payments.propertyId, dto.propertyId))); + if (!p) throw new BadRequestException(`payment ${dto.paymentId} not found in this property`); + } const [entry] = await this.db .insert(depositLedgerEntries) .values({ diff --git a/apps/api/src/modules/agent/agent-fk-ownership.spec.ts b/apps/api/src/modules/agent/agent-fk-ownership.spec.ts new file mode 100644 index 0000000..c686f44 --- /dev/null +++ b/apps/api/src/modules/agent/agent-fk-ownership.spec.ts @@ -0,0 +1,43 @@ +import { describe, it, expect, vi } from 'vitest'; +import { BadRequestException } from '@nestjs/common'; +import { Test } from '@nestjs/testing'; +import { AgentController } from './agent.controller'; +import { DRIZZLE } from '../../database/database.module'; +import { AgentService } from './agent.service'; + +/** + * Cross-tenant FK ownership for AgentController.createReview — flagged by the + * security re-audit. A caller-supplied dto.reservationId could be inserted into + * guest_reviews even when it belongs to another property. + */ +const A = 'aaaaaaaa-0000-4000-a000-000000000001'; + +describe('AgentController — createReview cross-tenant reservationId', () => { + it('rejects when dto.reservationId belongs to another property', async () => { + const db: any = { + select: vi.fn().mockReturnValue({ + from: vi.fn().mockReturnValue({ where: vi.fn().mockResolvedValue([]) }), + }), + insert: vi.fn(), + }; + const mod = await Test.createTestingModule({ + controllers: [AgentController], + providers: [ + { provide: DRIZZLE, useValue: db }, + { provide: AgentService, useValue: { runAgent: vi.fn() } }, + ], + }).compile(); + const ctrl = mod.get(AgentController); + + await expect( + ctrl.createReview(A, { + source: 'tripadvisor', + guestName: 'Anon', + rating: 5, + reviewText: 'great', + reservationId: 'foreign-r', + } as any), + ).rejects.toBeInstanceOf(BadRequestException); + expect(db.insert).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/api/src/modules/agent/agent.controller.ts b/apps/api/src/modules/agent/agent.controller.ts index c3b0d64..9b25b80 100644 --- a/apps/api/src/modules/agent/agent.controller.ts +++ b/apps/api/src/modules/agent/agent.controller.ts @@ -9,10 +9,11 @@ import { Query, Inject, ParseUUIDPipe, + BadRequestException, } from '@nestjs/common'; import { ApiTags, ApiOperation, ApiQuery } from '@nestjs/swagger'; import { eq, and, desc } from 'drizzle-orm'; -import { guestReviews } from '@telivityhaip/database'; +import { guestReviews, reservations } from '@telivityhaip/database'; import { DRIZZLE } from '../../database/database.module'; import { Roles } from '../auth/roles.decorator'; import { CurrentUser, type AuthUser } from '../auth/current-user.decorator'; @@ -119,6 +120,18 @@ export class AgentController { @Param('propertyId', ParseUUIDPipe) propertyId: string, @Body() dto: CreateReviewDto, ) { + // FK ownership (security audit follow-on): caller-supplied reservationId + // must belong to this propertyId. Without this, a review could be attached + // to a foreign tenant's reservation (cross-tenant FK write into guest_reviews). + if (dto.reservationId) { + const [r] = await this.db + .select({ id: reservations.id }) + .from(reservations) + .where(and(eq(reservations.id, dto.reservationId), eq(reservations.propertyId, propertyId))); + if (!r) { + throw new BadRequestException(`reservation ${dto.reservationId} not found in this property`); + } + } const [review] = await this.db .insert(guestReviews) .values({ diff --git a/apps/api/src/modules/cashier/cashier-fk-ownership.spec.ts b/apps/api/src/modules/cashier/cashier-fk-ownership.spec.ts new file mode 100644 index 0000000..f849501 --- /dev/null +++ b/apps/api/src/modules/cashier/cashier-fk-ownership.spec.ts @@ -0,0 +1,50 @@ +import { describe, it, expect, vi } from 'vitest'; +import { BadRequestException } from '@nestjs/common'; +import { Test } from '@nestjs/testing'; +import { CashierService } from './cashier.service'; +import { DRIZZLE } from '../../database/database.module'; +import { WebhookService } from '../webhook/webhook.service'; + +/** + * Cross-tenant FK ownership for CashierService.recordMovement — flagged by the + * security re-audit. A cashier could pass a foreign-property reservationId and + * attribute the cash movement (and downstream reporting) to that tenant. + */ +const A = 'aaaaaaaa-0000-4000-a000-000000000001'; + +describe('CashierService — recordMovement cross-tenant FK ownership', () => { + it('rejects when dto.reservationId belongs to another property', async () => { + // findSessionById first (returns open session), then reservations FK check (empty). + let i = 0; + const seq: any[][] = [ + [{ id: 's-1', propertyId: A, status: 'open' }], // findSessionById + [], // reservations FK empty + ]; + const db: any = { + select: vi.fn().mockReturnValue({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockImplementation(() => Promise.resolve(seq[i++] ?? [])), + }), + }), + insert: vi.fn(), + }; + const mod = await Test.createTestingModule({ + providers: [ + CashierService, + { provide: DRIZZLE, useValue: db }, + { provide: WebhookService, useValue: { emit: vi.fn() } }, + ], + }).compile(); + const svc = mod.get(CashierService); + + await expect( + svc.recordMovement('s-1', { + propertyId: A, + reservationId: 'foreign-r', + type: 'payment', + amount: '50.00', + } as any), + ).rejects.toBeInstanceOf(BadRequestException); + expect(db.insert).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/api/src/modules/cashier/cashier.service.ts b/apps/api/src/modules/cashier/cashier.service.ts index f2f0507..b213bda 100644 --- a/apps/api/src/modules/cashier/cashier.service.ts +++ b/apps/api/src/modules/cashier/cashier.service.ts @@ -10,6 +10,7 @@ import { cashDrawers, cashDrawerSessions, cashMovements, + reservations, } from '@telivityhaip/database'; import { DRIZZLE } from '../../database/database.module'; import { WebhookService } from '../webhook/webhook.service'; @@ -132,6 +133,19 @@ export class CashierService { throw new BadRequestException('Cannot record a movement on a closed session'); } + // FK ownership (security audit follow-on): the cashier could pass a + // reservationId belonging to another property, attributing the movement + // (and any downstream reporting / audit trail) cross-tenant. + if (dto.reservationId) { + const [r] = await this.db + .select({ id: reservations.id }) + .from(reservations) + .where(and(eq(reservations.id, dto.reservationId), eq(reservations.propertyId, dto.propertyId))); + if (!r) { + throw new BadRequestException(`reservation ${dto.reservationId} not found in this property`); + } + } + const [movement] = await this.db .insert(cashMovements) .values({ diff --git a/apps/api/src/modules/channel/channel-mapping-fk-ownership.spec.ts b/apps/api/src/modules/channel/channel-mapping-fk-ownership.spec.ts new file mode 100644 index 0000000..4fbb411 --- /dev/null +++ b/apps/api/src/modules/channel/channel-mapping-fk-ownership.spec.ts @@ -0,0 +1,95 @@ +import { describe, it, expect, vi } from 'vitest'; +import { BadRequestException } from '@nestjs/common'; +import { Test } from '@nestjs/testing'; +import { ChannelService } from './channel.service'; +import { DRIZZLE } from '../../database/database.module'; +import { WebhookService } from '../webhook/webhook.service'; +import { ChannelAdapterFactory } from './channel-adapter.factory'; + +/** + * Cross-tenant FK ownership for ChannelService.create + update mappings — the + * roomTypeMapping / ratePlanMapping JSON is operator-supplied. Without a + * write-time check, a misconfigured (or malicious) mapping pointing at a + * foreign-tenant id would be persisted on the connection, ultimately producing + * cross-tenant reservation writes on inbound OTA pushes. (Inbound-reservation + * already has its own READ-time guard — this is defense in depth.) + */ +const A = 'aaaaaaaa-0000-4000-a000-000000000001'; + +function mkDb(rows: any[][]) { + let i = 0; + return { + select: vi.fn().mockReturnValue({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockImplementation(() => Promise.resolve(rows[i++] ?? [])), + }), + }), + insert: vi.fn().mockReturnValue({ + values: vi.fn().mockReturnValue({ returning: vi.fn().mockResolvedValue([{ id: 'conn-1' }]) }), + }), + update: vi.fn(), + }; +} + +async function mkSvc(db: any) { + const mod = await Test.createTestingModule({ + providers: [ + ChannelService, + { provide: DRIZZLE, useValue: db }, + { provide: WebhookService, useValue: { emit: vi.fn() } }, + { provide: ChannelAdapterFactory, useValue: { getAdapter: vi.fn().mockReturnValue({}) } }, + ], + }).compile(); + return mod.get(ChannelService); +} + +describe('ChannelService — cross-tenant mapping FK ownership', () => { + it('create() rejects when ratePlanMapping contains a foreign ratePlanId', async () => { + // ratePlans lookup → empty (the one mapped id is not in this property). + const db = mkDb([[]]); + const svc = await mkSvc(db); + await expect( + svc.create({ + propertyId: A, + channelCode: 'booking_com', + channelName: 'Booking.com', + adapterType: 'booking_com', + ratePlanMapping: [{ ratePlanId: 'foreign-rp', channelRateCode: 'SM-BAR' }], + roomTypeMapping: [], + } as any), + ).rejects.toBeInstanceOf(BadRequestException); + expect(db.insert).not.toHaveBeenCalled(); + }); + + it('create() rejects when roomTypeMapping contains a foreign roomTypeId (ratePlanMapping OK or empty)', async () => { + const db = mkDb([[]]); // roomTypes lookup empty + const svc = await mkSvc(db); + await expect( + svc.create({ + propertyId: A, + channelCode: 'expedia', + channelName: 'Expedia', + adapterType: 'expedia', + ratePlanMapping: [], + roomTypeMapping: [{ roomTypeId: 'foreign-rt', channelRoomCode: 'EX-STD' }], + } as any), + ).rejects.toBeInstanceOf(BadRequestException); + expect(db.insert).not.toHaveBeenCalled(); + }); + + it('update() rejects when the new mapping introduces a foreign roomTypeId', async () => { + // findById first (returns the existing connection), then roomTypes lookup empty. + const seq: any[][] = [ + [{ id: 'conn-1', propertyId: A, ratePlanMapping: [], roomTypeMapping: [] }], + [], + ]; + const db = mkDb(seq); + const svc = await mkSvc(db); + await expect( + svc.update('conn-1', A, { + roomTypeMapping: [{ roomTypeId: 'foreign-rt', channelRoomCode: 'X' }], + } as any), + ).rejects.toBeInstanceOf(BadRequestException); + expect(db.update).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/api/src/modules/channel/channel.service.ts b/apps/api/src/modules/channel/channel.service.ts index 09ab916..f0a49d7 100644 --- a/apps/api/src/modules/channel/channel.service.ts +++ b/apps/api/src/modules/channel/channel.service.ts @@ -2,9 +2,10 @@ import { Injectable, Inject, NotFoundException, + BadRequestException, } from '@nestjs/common'; -import { eq, and } from 'drizzle-orm'; -import { channelConnections } from '@telivityhaip/database'; +import { eq, and, inArray } from 'drizzle-orm'; +import { channelConnections, ratePlans, roomTypes } from '@telivityhaip/database'; import { DRIZZLE } from '../../database/database.module'; import { WebhookService } from '../webhook/webhook.service'; import { ChannelAdapterFactory } from './channel-adapter.factory'; @@ -23,6 +24,13 @@ export class ChannelService { // Validate adapter type exists this.adapterFactory.getAdapter(dto.adapterType); + // FK ownership (security audit follow-on): the mapping JSON references PMS + // ratePlan / roomType ids. A misconfigured (or maliciously edited) mapping + // pointing at a foreign-tenant id would later cause cross-tenant reservation + // writes on inbound OTA pushes (inbound-reservation.service has its own + // read-time guard, but write-time validation is defense in depth). + await this.assertMappingOwnership(dto.propertyId, dto.ratePlanMapping, dto.roomTypeMapping); + const [connection] = await this.db .insert(channelConnections) .values({ @@ -90,6 +98,13 @@ export class ChannelService { async update(id: string, propertyId: string, dto: UpdateChannelConnectionDto) { await this.findById(id, propertyId); + // FK ownership (security audit follow-on): same rationale as create() — + // an update could otherwise swap the mapping to reference a foreign tenant's + // rate plans / room types. + if (dto.ratePlanMapping !== undefined || dto.roomTypeMapping !== undefined) { + await this.assertMappingOwnership(propertyId, dto.ratePlanMapping, dto.roomTypeMapping); + } + const updates: Record = { updatedAt: new Date() }; if (dto.channelName !== undefined) updates['channelName'] = dto.channelName; if (dto.status !== undefined) updates['status'] = dto.status; @@ -168,4 +183,44 @@ export class ChannelService { }) .where(and(eq(channelConnections.id, id), eq(channelConnections.propertyId, propertyId))); } + + /** + * Verify every ratePlanId in `ratePlanMapping` and every roomTypeId in + * `roomTypeMapping` belongs to `propertyId`. Closes the cross-tenant mapping + * write path flagged in the security re-audit — the inbound-reservation + * service already does this on the read side; this is defense in depth so the + * row never lands in the DB in the first place. + */ + private async assertMappingOwnership( + propertyId: string, + ratePlanMapping: Array<{ ratePlanId: string }> | undefined, + roomTypeMapping: Array<{ roomTypeId: string }> | undefined, + ): Promise { + const rpIds = (ratePlanMapping ?? []).map((m) => m.ratePlanId).filter(Boolean); + const rtIds = (roomTypeMapping ?? []).map((m) => m.roomTypeId).filter(Boolean); + + if (rpIds.length) { + const rows = await this.db + .select({ id: ratePlans.id }) + .from(ratePlans) + .where(and(inArray(ratePlans.id, rpIds), eq(ratePlans.propertyId, propertyId))); + const found = new Set(rows.map((r: any) => r.id)); + const foreign = rpIds.filter((id) => !found.has(id)); + if (foreign.length) { + throw new BadRequestException(`rate plan(s) ${foreign.join(', ')} not found in this property`); + } + } + + if (rtIds.length) { + const rows = await this.db + .select({ id: roomTypes.id }) + .from(roomTypes) + .where(and(inArray(roomTypes.id, rtIds), eq(roomTypes.propertyId, propertyId))); + const found = new Set(rows.map((r: any) => r.id)); + const foreign = rtIds.filter((id) => !found.has(id)); + if (foreign.length) { + throw new BadRequestException(`room type(s) ${foreign.join(', ')} not found in this property`); + } + } + } } diff --git a/apps/api/src/modules/channel/inbound-reservation-fk-ownership.spec.ts b/apps/api/src/modules/channel/inbound-reservation-fk-ownership.spec.ts new file mode 100644 index 0000000..2829f72 --- /dev/null +++ b/apps/api/src/modules/channel/inbound-reservation-fk-ownership.spec.ts @@ -0,0 +1,114 @@ +import { describe, it, expect, vi } from 'vitest'; +import { BadRequestException } from '@nestjs/common'; +import { Test } from '@nestjs/testing'; +import { InboundReservationService } from './inbound-reservation.service'; +import { DRIZZLE } from '../../database/database.module'; +import { ChannelService } from './channel.service'; +import { ChannelAdapterFactory } from './channel-adapter.factory'; +import { AriService } from './ari.service'; +import { WebhookService } from '../webhook/webhook.service'; + +/** + * Cross-tenant FK ownership for channel inbound reservations (follow-on to + * audit #4-6). The `channelConnections.config.roomTypeMapping` / + * `ratePlanMapping` JSON is operator-supplied and could (mis)map to a foreign + * tenant's id. Before this fix, the resolved id was written straight into + * `reservations` — a live cross-tenant write path. + */ +const A = 'aaaaaaaa-0000-4000-a000-000000000001'; + +function mkDbSeq(rows: any[][]) { + let i = 0; + return { + select: vi.fn().mockReturnValue({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockImplementation(() => Promise.resolve(rows[i++] ?? [])), + }), + }), + insert: vi.fn().mockReturnValue({ + values: vi.fn().mockReturnValue({ returning: vi.fn().mockResolvedValue([{}]) }), + }), + update: vi.fn(), + transaction: vi.fn().mockImplementation(async (cb: any) => cb({} as any)), + }; +} + +async function mkSvc(db: any) { + const mod = await Test.createTestingModule({ + providers: [ + InboundReservationService, + { provide: DRIZZLE, useValue: db }, + { provide: ChannelService, useValue: {} }, + { provide: ChannelAdapterFactory, useValue: {} }, + { provide: AriService, useValue: {} }, + { provide: WebhookService, useValue: { emit: vi.fn() } }, + ], + }).compile(); + return mod.get(InboundReservationService); +} + +describe('InboundReservationService — channel mapping FK ownership', () => { + // Use the private method via cast — that's the bypass point. + const conn = { + id: 'conn-1', + propertyId: A, + roomTypeMapping: [{ roomTypeId: 'foreign-rt', channelRoomCode: 'SM-STD' }], + ratePlanMapping: [{ ratePlanId: 'foreign-rp', channelRateCode: 'SM-BAR' }], + }; + const reservation: any = { + externalConfirmation: 'X1', + channelCode: 'siteminder', + channelRoomCode: 'SM-STD', + channelRateCode: 'SM-BAR', + arrivalDate: '2026-07-01', + departureDate: '2026-07-03', + adults: 2, + totalAmount: 300, + currencyCode: 'USD', + guestFirstName: 'A', + guestLastName: 'B', + }; + + it('handleNewReservation REJECTS when mapping points at a foreign roomTypeId', async () => { + // First select = FK check on roomTypes → [] (foreign). + const db = mkDbSeq([[]]); + const svc = await mkSvc(db); + await expect( + (svc as any).handleNewReservation(conn, reservation), + ).rejects.toBeInstanceOf(BadRequestException); + // CRITICAL: the transaction MUST NOT have run — no booking/reservation row written. + expect(db.transaction).not.toHaveBeenCalled(); + }); + + it('handleNewReservation REJECTS when mapping points at a foreign ratePlanId (roomType OK)', async () => { + const db = mkDbSeq([ + [{ id: 'foreign-rt' }], // roomTypes FK OK (this would mean rt was actually same-property — for the test we pretend) + [], // ratePlans FK empty + ]); + // Use a conn where roomTypeId mapping resolves to a "valid" id but ratePlanId is foreign. + const okRtConn = { + ...conn, + roomTypeMapping: [{ roomTypeId: 'rt-1', channelRoomCode: 'SM-STD' }], + }; + const svc = await mkSvc(db); + await expect( + (svc as any).handleNewReservation(okRtConn, reservation), + ).rejects.toBeInstanceOf(BadRequestException); + expect(db.transaction).not.toHaveBeenCalled(); + }); + + // Codex re-audit follow-up: handleModification has the same FK check but no test. + it('handleModification REJECTS when mapping points at a foreign roomTypeId', async () => { + // 1) existing reservation lookup → row in same property; 2) roomTypes FK → empty (foreign). + const db = mkDbSeq([ + [{ id: 'r-1', propertyId: A, bookingId: 'b-1' }], + [], + ]); + const svc = await mkSvc(db); + await expect( + (svc as any).handleModification(conn, reservation, { id: 'b-1' }), + ).rejects.toBeInstanceOf(BadRequestException); + // The reservations update MUST NOT have run. + expect(db.update).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/api/src/modules/channel/inbound-reservation.service.spec.ts b/apps/api/src/modules/channel/inbound-reservation.service.spec.ts index 074ed3d..2ca1051 100644 --- a/apps/api/src/modules/channel/inbound-reservation.service.spec.ts +++ b/apps/api/src/modules/channel/inbound-reservation.service.spec.ts @@ -42,17 +42,22 @@ describe('InboundReservationService', () => { }; beforeEach(() => { - // Track which .from() table is being queried + // Track which .from() table is being queried. + // After the FK-ownership fix in inbound-reservation.service, handleNewReservation + // now runs TWO extra selects (roomTypes + ratePlans same-property checks) BEFORE + // the transaction. Each must return a row so the FK guard passes and we reach + // the actual booking/guest flow the test is asserting on. let selectCallCount = 0; mockDb = { select: vi.fn().mockImplementation(() => ({ from: vi.fn().mockImplementation(() => ({ where: vi.fn().mockImplementation(() => { selectCallCount++; - // Default: channelConnections lookup returns connection, bookings returns empty if (selectCallCount === 1) return Promise.resolve([mockConnection]); if (selectCallCount === 2) return Promise.resolve([]); // no existing booking (dedup check) - if (selectCallCount === 3) return Promise.resolve([]); // no existing guest by email + if (selectCallCount === 3) return Promise.resolve([{ id: 'rt-1' }]); // roomTypes FK OK + if (selectCallCount === 4) return Promise.resolve([{ id: 'rp-1' }]); // ratePlans FK OK + if (selectCallCount === 5) return Promise.resolve([]); // no existing guest by email return Promise.resolve([]); }), })), @@ -217,14 +222,19 @@ describe('InboundReservationService', () => { describe('processInboundReservation - modified', () => { it('should update an existing reservation', async () => { + // After FK-ownership fix, handleModification runs 2 extra selects (roomTypes + // + ratePlans same-property checks). Sequence: connection, existing booking, + // existing reservation, roomTypes FK, ratePlans FK. let callCount = 0; mockDb.select.mockImplementation(() => ({ from: vi.fn().mockReturnValue({ where: vi.fn().mockImplementation(() => { callCount++; - if (callCount === 1) return Promise.resolve([mockConnection]); // connection - if (callCount === 2) return Promise.resolve([{ id: 'booking-1', confirmationNumber: 'CH-123' }]); // existing booking - if (callCount === 3) return Promise.resolve([{ id: 'res-1', bookingId: 'booking-1', guestId: 'guest-1' }]); // existing reservation + if (callCount === 1) return Promise.resolve([mockConnection]); + if (callCount === 2) return Promise.resolve([{ id: 'booking-1', confirmationNumber: 'CH-123' }]); + if (callCount === 3) return Promise.resolve([{ id: 'res-1', bookingId: 'booking-1', guestId: 'guest-1' }]); + if (callCount === 4) return Promise.resolve([{ id: 'rt-1' }]); // roomTypes FK OK + if (callCount === 5) return Promise.resolve([{ id: 'rp-1' }]); // ratePlans FK OK return Promise.resolve([]); }), }), @@ -303,15 +313,19 @@ describe('InboundReservationService', () => { describe('guest resolution', () => { it('should find existing guest by email', async () => { + // After FK-ownership fix, handleNewReservation runs 2 extra selects (roomTypes + // + ratePlans FK checks) BEFORE the guest-by-email lookup. let callCount = 0; const existingGuest = { id: 'guest-existing', firstName: 'John', email: 'john@example.com' }; mockDb.select.mockImplementation(() => ({ from: vi.fn().mockReturnValue({ where: vi.fn().mockImplementation(() => { callCount++; - if (callCount === 1) return Promise.resolve([mockConnection]); // connection + if (callCount === 1) return Promise.resolve([mockConnection]); if (callCount === 2) return Promise.resolve([]); // no existing booking - if (callCount === 3) return Promise.resolve([existingGuest]); // existing guest by email + if (callCount === 3) return Promise.resolve([{ id: 'rt-1' }]); // roomTypes FK OK + if (callCount === 4) return Promise.resolve([{ id: 'rp-1' }]); // ratePlans FK OK + if (callCount === 5) return Promise.resolve([existingGuest]); // existing guest by email return Promise.resolve([]); }), }), diff --git a/apps/api/src/modules/channel/inbound-reservation.service.ts b/apps/api/src/modules/channel/inbound-reservation.service.ts index 7041b1a..a3ae35d 100644 --- a/apps/api/src/modules/channel/inbound-reservation.service.ts +++ b/apps/api/src/modules/channel/inbound-reservation.service.ts @@ -1,6 +1,6 @@ -import { Injectable, Inject, ConflictException, NotFoundException } from '@nestjs/common'; +import { Injectable, Inject, ConflictException, NotFoundException, BadRequestException } from '@nestjs/common'; import { eq, and } from 'drizzle-orm'; -import { bookings, reservations, guests, channelConnections } from '@telivityhaip/database'; +import { bookings, reservations, guests, channelConnections, roomTypes, ratePlans } from '@telivityhaip/database'; import { DRIZZLE } from '../../database/database.module'; import { ChannelService } from './channel.service'; import { ChannelAdapterFactory } from './channel-adapter.factory'; @@ -105,6 +105,13 @@ export class InboundReservationService { const roomTypeId = this.resolveRoomTypeId(conn, reservation.channelRoomCode); const ratePlanId = this.resolveRatePlanId(conn, reservation.channelRateCode); + // FK ownership (security audit follow-on): the channel connection's + // roomTypeMapping/ratePlanMapping JSON is operator-supplied. A misconfigured + // (or maliciously edited) mapping could point at a foreign-tenant id. Verify + // BOTH resolved ids belong to THIS connection's propertyId before writing. + await this.assertSamePropertyFk(roomTypes, roomTypeId, propertyId, 'room type'); + await this.assertSamePropertyFk(ratePlans, ratePlanId, propertyId, 'rate plan'); + // Calculate nights const arrival = new Date(reservation.arrivalDate); const departure = new Date(reservation.departureDate); @@ -220,6 +227,12 @@ export class InboundReservationService { const roomTypeId = this.resolveRoomTypeId(conn, reservation.channelRoomCode); const ratePlanId = this.resolveRatePlanId(conn, reservation.channelRateCode); + // FK ownership (security audit follow-on): same as handleNewReservation — + // verify the operator-supplied channel mapping points at THIS connection's + // propertyId before mutating the reservation. + await this.assertSamePropertyFk(roomTypes, roomTypeId, propertyId, 'room type'); + await this.assertSamePropertyFk(ratePlans, ratePlanId, propertyId, 'rate plan'); + const arrival = new Date(reservation.arrivalDate); const departure = new Date(reservation.departureDate); const nights = Math.ceil((departure.getTime() - arrival.getTime()) / (1000 * 60 * 60 * 24)); @@ -460,6 +473,27 @@ export class InboundReservationService { return mapping.ratePlanId; } + /** + * Verify a caller- or mapping-supplied FK row belongs to the SAME property as + * the channel connection. Mirrors the same check in ReservationService — the + * channel mapping JSON is operator-supplied and must not be allowed to point + * at another tenant's room type / rate plan. + */ + private async assertSamePropertyFk( + table: { id: any; propertyId: any }, + id: string, + propertyId: string, + label: string, + ): Promise { + const [row] = await this.db + .select({ id: table.id }) + .from(table) + .where(and(eq(table.id, id), eq(table.propertyId, propertyId))); + if (!row) { + throw new BadRequestException(`${label} ${id} not found in this property (channel mapping points at foreign tenant)`); + } + } + private generateConfirmationNumber(): string { const prefix = 'CH'; const timestamp = Date.now().toString(36).toUpperCase(); diff --git a/apps/api/src/modules/connect/connect-booking-fk-ownership.spec.ts b/apps/api/src/modules/connect/connect-booking-fk-ownership.spec.ts new file mode 100644 index 0000000..9e08d67 --- /dev/null +++ b/apps/api/src/modules/connect/connect-booking-fk-ownership.spec.ts @@ -0,0 +1,54 @@ +import { describe, it, expect, vi } from 'vitest'; +import { BadRequestException } from '@nestjs/common'; +import { Test } from '@nestjs/testing'; +import { ConnectBookingService } from './connect-booking.service'; +import { DRIZZLE } from '../../database/database.module'; +import { AvailabilityService } from '../reservation/availability.service'; +import { WebhookService } from '../webhook/webhook.service'; + +/** + * Cross-tenant FK ownership for ConnectBookingService.modify — flagged by the + * security re-audit. An OTAIP agent could pass dto.roomTypeId or dto.ratePlanId + * pointing at a foreign tenant's row, and the modify path would re-read the rate + * plan by bare id and apply the changes to the reservation. + */ +const A = 'aaaaaaaa-0000-4000-a000-000000000001'; + +describe('ConnectBookingService — modify cross-tenant FK ownership', () => { + // The modify() method has a complex pre-amble; we test ONLY that the FK guard + // trips before the rate re-calculation and the reservation update. + it('REJECTS when dto.roomTypeId belongs to another property', async () => { + // Sequence of selects modify() performs leading up to the FK check: + // 1) findByConfirmation → booking + // 2) reservation lookup → reservation + // 3) my new roomTypes FK check → empty + let i = 0; + const seq: any[][] = [ + [{ id: 'b-1', propertyId: A, confirmationNumber: 'HAIP-1' }], + [{ id: 'r-1', bookingId: 'b-1', roomTypeId: 'rt-1', ratePlanId: 'rp-1', arrivalDate: '2026-07-01', departureDate: '2026-07-03', totalAmount: '100.00' }], + [], + ]; + const db: any = { + select: vi.fn().mockReturnValue({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockImplementation(() => Promise.resolve(seq[i++] ?? [])), + }), + }), + update: vi.fn(), + }; + const mod = await Test.createTestingModule({ + providers: [ + ConnectBookingService, + { provide: DRIZZLE, useValue: db }, + { provide: AvailabilityService, useValue: { searchAvailability: vi.fn().mockResolvedValue([]) } }, + { provide: WebhookService, useValue: { emit: vi.fn() } }, + ], + }).compile(); + const svc = mod.get(ConnectBookingService); + + await expect( + svc.modify('HAIP-1', { roomTypeId: 'foreign-rt' } as any), + ).rejects.toBeInstanceOf(BadRequestException); + expect(db.update).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/api/src/modules/connect/connect-booking.service.ts b/apps/api/src/modules/connect/connect-booking.service.ts index 4927a27..c4d2d54 100644 --- a/apps/api/src/modules/connect/connect-booking.service.ts +++ b/apps/api/src/modules/connect/connect-booking.service.ts @@ -278,6 +278,18 @@ export class ConnectBookingService { const newRoomTypeId = dto.roomTypeId ?? reservation.roomTypeId; const newRatePlanId = dto.ratePlanId ?? reservation.ratePlanId; + // FK ownership (security audit follow-on): the caller (an OTAIP agent) could + // pass a roomTypeId / ratePlanId that belongs to another property. Verify + // both belong to the booking's property BEFORE the rate re-calculation and + // the reservation update. + if (dto.roomTypeId) { + const [rt] = await this.db + .select({ id: roomTypes.id }) + .from(roomTypes) + .where(and(eq(roomTypes.id, newRoomTypeId), eq(roomTypes.propertyId, booking.propertyId))); + if (!rt) throw new BadRequestException(`room type ${newRoomTypeId} not found in this property`); + } + // Re-check availability const availability = await this.availabilityService.searchAvailability( booking.propertyId, @@ -294,14 +306,14 @@ export class ConnectBookingService { throw new BadRequestException('No availability for modified dates/room type'); } - // Re-calculate rate + // Re-calculate rate — same-property scoped (was bare-id before). const [ratePlan] = await this.db .select() .from(ratePlans) - .where(eq(ratePlans.id, newRatePlanId)); + .where(and(eq(ratePlans.id, newRatePlanId), eq(ratePlans.propertyId, booking.propertyId))); if (!ratePlan) { - throw new NotFoundException(`Rate plan ${newRatePlanId} not found`); + throw new NotFoundException(`Rate plan ${newRatePlanId} not found in this property`); } const arrival = new Date(newCheckIn); diff --git a/apps/api/src/modules/folio/folio-fk-ownership.spec.ts b/apps/api/src/modules/folio/folio-fk-ownership.spec.ts new file mode 100644 index 0000000..b6a26ca --- /dev/null +++ b/apps/api/src/modules/folio/folio-fk-ownership.spec.ts @@ -0,0 +1,65 @@ +import { describe, it, expect, vi } from 'vitest'; +import { BadRequestException } from '@nestjs/common'; +import { Test } from '@nestjs/testing'; +import { FolioService } from './folio.service'; +import { DRIZZLE } from '../../database/database.module'; +import { WebhookService } from '../webhook/webhook.service'; +import { TaxService } from '../tax/tax.service'; + +/** + * Cross-tenant FK ownership for FolioService.create (follow-on to audit #4-6, + * flagged by Codex re-audit as the same anti-pattern on `reservationId` and + * `bookingId`). + */ +const A = 'aaaaaaaa-0000-4000-a000-000000000001'; + +function mkDbSeq(rows: any[][]) { + let i = 0; + return { + select: vi.fn().mockReturnValue({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockImplementation(() => Promise.resolve(rows[i++] ?? [])), + }), + }), + insert: vi.fn().mockReturnValue({ + values: vi.fn().mockReturnValue({ returning: vi.fn().mockResolvedValue([{ id: 'f-1' }]) }), + }), + }; +} + +async function mkSvc(db: any) { + const mod = await Test.createTestingModule({ + providers: [ + FolioService, + { provide: DRIZZLE, useValue: db }, + { provide: WebhookService, useValue: { emit: vi.fn() } }, + { provide: TaxService, useValue: {} }, + ], + }).compile(); + return mod.get(FolioService); +} + +describe('FolioService — cross-tenant FK ownership', () => { + it('rejects when dto.reservationId belongs to another property', async () => { + const db = mkDbSeq([ + [], // reservation FK check empty → foreign + ]); + const svc = await mkSvc(db); + await expect( + svc.create({ propertyId: A, reservationId: 'foreign-r', guestId: 'g', type: 'guest' } as any), + ).rejects.toBeInstanceOf(BadRequestException); + expect(db.insert).not.toHaveBeenCalled(); + }); + + it('rejects when dto.bookingId belongs to another property (reservation OK)', async () => { + const db = mkDbSeq([ + [{ id: 'r-1' }], // reservation FK OK + [], // booking FK empty → foreign + ]); + const svc = await mkSvc(db); + await expect( + svc.create({ propertyId: A, reservationId: 'r-1', bookingId: 'foreign-b', guestId: 'g', type: 'guest' } as any), + ).rejects.toBeInstanceOf(BadRequestException); + expect(db.insert).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/api/src/modules/folio/folio.service.ts b/apps/api/src/modules/folio/folio.service.ts index a671c5a..d421bf9 100644 --- a/apps/api/src/modules/folio/folio.service.ts +++ b/apps/api/src/modules/folio/folio.service.ts @@ -6,7 +6,7 @@ import { } from '@nestjs/common'; import { eq, and, sql, gte, lte } from 'drizzle-orm'; import Decimal from 'decimal.js'; -import { folios, charges, payments } from '@telivityhaip/database'; +import { folios, charges, payments, reservations, bookings } from '@telivityhaip/database'; import { DRIZZLE } from '../../database/database.module'; import { WebhookService } from '../webhook/webhook.service'; import { TaxService } from '../tax/tax.service'; @@ -27,6 +27,24 @@ export class FolioService { async create(dto: CreateFolioDto, tx?: any) { const db = tx ?? this.db; + // FK ownership (security audit follow-on): the caller supplies reservationId + // and bookingId in the DTO. Without scoping these to dto.propertyId, a caller + // at property A could attach a folio to property B's reservation/booking. + // (Guest is intentionally cross-property by design per CLAUDE.md.) + if (dto.reservationId) { + const [r] = await db + .select({ id: reservations.id }) + .from(reservations) + .where(and(eq(reservations.id, dto.reservationId), eq(reservations.propertyId, dto.propertyId))); + if (!r) throw new BadRequestException(`reservation ${dto.reservationId} not found in this property`); + } + if (dto.bookingId) { + const [b] = await db + .select({ id: bookings.id }) + .from(bookings) + .where(and(eq(bookings.id, dto.bookingId), eq(bookings.propertyId, dto.propertyId))); + if (!b) throw new BadRequestException(`booking ${dto.bookingId} not found in this property`); + } const folioNumber = await this.generateFolioNumber(dto.propertyId, tx); const [folio] = await db .insert(folios) diff --git a/apps/api/src/modules/groups/allotment-fk-ownership.spec.ts b/apps/api/src/modules/groups/allotment-fk-ownership.spec.ts new file mode 100644 index 0000000..3cec233 --- /dev/null +++ b/apps/api/src/modules/groups/allotment-fk-ownership.spec.ts @@ -0,0 +1,106 @@ +import { describe, it, expect, vi } from 'vitest'; +import { BadRequestException } from '@nestjs/common'; +import { Test } from '@nestjs/testing'; +import { AllotmentService } from './allotment.service'; +import { DRIZZLE } from '../../database/database.module'; +import { WebhookService } from '../webhook/webhook.service'; +import { AvailabilityService } from '../reservation/availability.service'; + +const A = 'aaaaaaaa-0000-4000-a000-000000000001'; + +function mkDbSeq(rows: any[][]) { + let i = 0; + return { + select: vi.fn().mockReturnValue({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockImplementation(() => Promise.resolve(rows[i++] ?? [])), + }), + }), + insert: vi.fn().mockReturnValue({ + values: vi.fn().mockReturnValue({ returning: vi.fn().mockResolvedValue([{ id: 'blk-1' }]) }), + }), + update: vi.fn(), + }; +} + +async function mkSvc(db: any) { + const mod = await Test.createTestingModule({ + providers: [ + AllotmentService, + { provide: DRIZZLE, useValue: db }, + { provide: WebhookService, useValue: { emit: vi.fn() } }, + { provide: AvailabilityService, useValue: {} }, + ], + }).compile(); + return mod.get(AllotmentService); +} + +describe('AllotmentService — cross-tenant FK ownership', () => { + it('rejects when dto.groupProfileId belongs to another property', async () => { + const db = mkDbSeq([[]]); // group profile FK empty + const svc = await mkSvc(db); + await expect( + svc.createBlock({ + propertyId: A, + groupProfileId: 'foreign-gp', + name: 'Block', + startDate: '2026-07-01', + endDate: '2026-07-05', + } as any), + ).rejects.toBeInstanceOf(BadRequestException); + expect(db.insert).not.toHaveBeenCalled(); + }); + + it('rejects when dto.ratePlanId belongs to another property (groupProfile OK)', async () => { + const db = mkDbSeq([ + [{ id: 'gp-1' }], // group profile OK + [], // rate plan FK empty + ]); + const svc = await mkSvc(db); + await expect( + svc.createBlock({ + propertyId: A, + groupProfileId: 'gp-1', + ratePlanId: 'foreign-rp', + name: 'Block', + startDate: '2026-07-01', + endDate: '2026-07-05', + } as any), + ).rejects.toBeInstanceOf(BadRequestException); + expect(db.insert).not.toHaveBeenCalled(); + }); + + // updateBlock was the same anti-pattern — accepting dto.ratePlanId without + // verifying same-property, redirecting a block at a foreign tenant's plan. + it('updateBlock rejects when dto.ratePlanId belongs to another property', async () => { + const db = mkDbSeq([ + [{ id: 'blk-1', propertyId: A, status: 'tentative' }], // findBlockById + [], // ratePlans FK empty + ]); + const svc = await mkSvc(db); + await expect( + svc.updateBlock('blk-1', A, { ratePlanId: 'foreign-rp' } as any), + ).rejects.toBeInstanceOf(BadRequestException); + expect(db.update).not.toHaveBeenCalled(); + }); + + // setInventory() previously wrote a cross-property allotmentBlockInventory row + // with a foreign roomTypeId — the availability lookup returned 0 sellable but + // a roomsAllotted=0 request would still pass and persist the bad FK. + it('setInventory rejects when dto.roomTypeId belongs to another property', async () => { + // findBlockById first (returns the block in propertyId A), THEN FK check on roomTypes (empty = foreign). + const db = mkDbSeq([ + [{ id: 'blk-1', propertyId: A, status: 'tentative' }], // findBlockById + [], // roomTypes FK empty + ]); + const svc = await mkSvc(db); + await expect( + svc.setInventory('blk-1', A, { + stayDate: '2026-07-02', + roomTypeId: 'foreign-rt', + roomsAllotted: 0, + } as any), + ).rejects.toBeInstanceOf(BadRequestException); + expect(db.insert).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/api/src/modules/groups/allotment.service.spec.ts b/apps/api/src/modules/groups/allotment.service.spec.ts index 501c04b..3b12efb 100644 --- a/apps/api/src/modules/groups/allotment.service.spec.ts +++ b/apps/api/src/modules/groups/allotment.service.spec.ts @@ -127,11 +127,9 @@ describe('AllotmentService', () => { describe('setInventory', () => { it('inserts a new inventory row within sellable availability', async () => { - // First select() returns the block; later selects (existing row lookup) - // also resolve to the same data, but no existing inv row in this path. + // After the FK-ownership fix in setInventory, the select sequence is now: + // 1=findBlockById → block, 2=roomTypes FK check → row, 3+=inventory lookup → empty. const db = createMockDb([mockBlock]); - // Make the "existing inventory" lookup return empty by overriding select - // to return block for first call, then [] for the inventory lookup. let call = 0; db.select = vi.fn().mockImplementation(() => ({ from: vi.fn().mockReturnValue({ @@ -142,8 +140,9 @@ describe('AllotmentService', () => { orderBy: vi.fn().mockResolvedValue([]), then: (resolve: any) => { call++; - // call 1 = findBlockById -> block; subsequent = inventory lookup -> empty - return resolve(call === 1 ? [mockBlock] : []); + if (call === 1) return resolve([mockBlock]); // findBlockById + if (call === 2) return resolve([{ id: 'rt-001' }]); // roomTypes FK OK + return resolve([]); // inventory lookup empty }, }), }), @@ -180,7 +179,9 @@ describe('AllotmentService', () => { where: vi.fn().mockReturnValue({ then: (resolve: any) => { call++; - return resolve(call === 1 ? [mockBlock] : []); + if (call === 1) return resolve([mockBlock]); // findBlockById + if (call === 2) return resolve([{ id: 'rt-001' }]); // roomTypes FK OK + return resolve([]); // existing inventory empty }, }), }), diff --git a/apps/api/src/modules/groups/allotment.service.ts b/apps/api/src/modules/groups/allotment.service.ts index 43cd022..bc48c06 100644 --- a/apps/api/src/modules/groups/allotment.service.ts +++ b/apps/api/src/modules/groups/allotment.service.ts @@ -8,6 +8,9 @@ import { eq, and, sql, inArray, lt } from 'drizzle-orm'; import { allotmentBlocks, allotmentBlockInventory, + groupProfiles, + ratePlans, + roomTypes, } from '@telivityhaip/database'; import { DRIZZLE } from '../../database/database.module'; import { WebhookService } from '../webhook/webhook.service'; @@ -29,6 +32,25 @@ export class AllotmentService { if (new Date(dto.endDate) <= new Date(dto.startDate)) { throw new BadRequestException('endDate must be after startDate'); } + // FK ownership (security audit follow-on): caller-supplied groupProfileId + // and (optional) ratePlanId must belong to dto.propertyId. Schema FK only + // constrains row id. + const [gp] = await this.db + .select({ id: groupProfiles.id }) + .from(groupProfiles) + .where(and(eq(groupProfiles.id, dto.groupProfileId), eq(groupProfiles.propertyId, dto.propertyId))); + if (!gp) { + throw new BadRequestException(`group profile ${dto.groupProfileId} not found in this property`); + } + if (dto.ratePlanId) { + const [rp] = await this.db + .select({ id: ratePlans.id }) + .from(ratePlans) + .where(and(eq(ratePlans.id, dto.ratePlanId), eq(ratePlans.propertyId, dto.propertyId))); + if (!rp) { + throw new BadRequestException(`rate plan ${dto.ratePlanId} not found in this property`); + } + } const [block] = await this.db .insert(allotmentBlocks) .values({ @@ -107,6 +129,18 @@ export class AllotmentService { async updateBlock(id: string, propertyId: string, dto: UpdateBlockDto) { await this.findBlockById(id, propertyId); + // FK ownership (security audit follow-on): updateBlock takes an optional + // ratePlanId. Without scoping to propertyId, a caller could redirect a block + // at a foreign tenant's rate plan. + if (dto.ratePlanId) { + const [rp] = await this.db + .select({ id: ratePlans.id }) + .from(ratePlans) + .where(and(eq(ratePlans.id, dto.ratePlanId), eq(ratePlans.propertyId, propertyId))); + if (!rp) { + throw new BadRequestException(`rate plan ${dto.ratePlanId} not found in this property`); + } + } const [updated] = await this.db .update(allotmentBlocks) .set({ ...dto, updatedAt: new Date() }) @@ -128,6 +162,17 @@ export class AllotmentService { `Cannot set inventory on a ${block.status} block`, ); } + // FK ownership (security audit follow-on): the caller's roomTypeId must + // belong to this propertyId. Without this, a foreign room type id still + // inserts an allotment row (availability lookup just returns 0 sellable, + // and roomsAllotted=0 would pass — writing a cross-property FK). + const [rt] = await this.db + .select({ id: roomTypes.id }) + .from(roomTypes) + .where(and(eq(roomTypes.id, dto.roomTypeId), eq(roomTypes.propertyId, propertyId))); + if (!rt) { + throw new BadRequestException(`room type ${dto.roomTypeId} not found in this property`); + } // Sellable availability for that single night/room-type. The availability // engine already nets out other blocks' member reservations (they hold diff --git a/apps/api/src/modules/groups/rooming-list-fk-ownership.spec.ts b/apps/api/src/modules/groups/rooming-list-fk-ownership.spec.ts new file mode 100644 index 0000000..2d0a99b --- /dev/null +++ b/apps/api/src/modules/groups/rooming-list-fk-ownership.spec.ts @@ -0,0 +1,55 @@ +import { describe, it, expect, vi } from 'vitest'; +import { Test } from '@nestjs/testing'; +import { RoomingListService } from './rooming-list.service'; +import { DRIZZLE } from '../../database/database.module'; +import { WebhookService } from '../webhook/webhook.service'; +import { ReservationService } from '../reservation/reservation.service'; +import { AllotmentService } from './allotment.service'; +import { GroupProfileService } from './group-profile.service'; + +/** + * Cross-tenant FK ownership for RoomingListService.importRoomingList — flagged by + * the security re-audit. A caller-supplied entry.roomTypeId could insert a + * cross-property FK into rooming_list_entries. The fix records the entry as an + * error and continues the batch (matches existing per-row error handling). + */ +const A = 'aaaaaaaa-0000-4000-a000-000000000001'; + +describe('RoomingListService — cross-tenant entry.roomTypeId', () => { + it('does NOT insert when entry.roomTypeId belongs to another property — records as error', async () => { + // FK check returns [] (foreign), then we never proceed to insert this row. + const db: any = { + select: vi.fn().mockReturnValue({ + from: vi.fn().mockReturnValue({ where: vi.fn().mockResolvedValue([]) }), + }), + insert: vi.fn(), + update: vi.fn(), + }; + const mod = await Test.createTestingModule({ + providers: [ + RoomingListService, + { provide: DRIZZLE, useValue: db }, + { provide: WebhookService, useValue: { emit: vi.fn() } }, + { provide: ReservationService, useValue: {} }, + { provide: AllotmentService, useValue: { + findBlockById: vi.fn().mockResolvedValue({ id: 'blk-1', startDate: '2026-07-01', endDate: '2026-07-05' }), + } }, + { provide: GroupProfileService, useValue: {} }, + ], + }).compile(); + const svc = mod.get(RoomingListService); + + const out = await svc.importRoomingList('blk-1', A, { + entries: [ + { guestName: 'Foreign Tenant', roomTypeId: 'foreign-rt' } as any, + ], + } as any); + + expect(db.insert).not.toHaveBeenCalled(); + // The bad entry is surfaced as an error in the per-row results — the batch + // is not aborted (matches existing rooming-list error semantics). + expect(out.errors).toBe(1); + expect(out.results[0].status).toBe('error'); + expect(String(out.results[0].error)).toContain('foreign-rt'); + }); +}); diff --git a/apps/api/src/modules/groups/rooming-list.service.spec.ts b/apps/api/src/modules/groups/rooming-list.service.spec.ts index 4537638..06c01a6 100644 --- a/apps/api/src/modules/groups/rooming-list.service.spec.ts +++ b/apps/api/src/modules/groups/rooming-list.service.spec.ts @@ -18,9 +18,15 @@ const mockBlock = { function createMockDb() { // Each insert returns a fresh entry row; updates resolve. + // FK-ownership check on roomTypes runs before insert per row — return a row so + // the existing tests reach the booking-creation path under test. let entrySeq = 0; return { - select: vi.fn(), + select: vi.fn().mockReturnValue({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockResolvedValue([{ id: 'rt-1' }]), + }), + }), insert: vi.fn().mockImplementation(() => ({ values: vi.fn().mockReturnValue({ returning: vi.fn().mockImplementation(() => { diff --git a/apps/api/src/modules/groups/rooming-list.service.ts b/apps/api/src/modules/groups/rooming-list.service.ts index 18b206e..2dc3d15 100644 --- a/apps/api/src/modules/groups/rooming-list.service.ts +++ b/apps/api/src/modules/groups/rooming-list.service.ts @@ -1,6 +1,6 @@ -import { Injectable, Inject } from '@nestjs/common'; +import { Injectable, Inject, BadRequestException } from '@nestjs/common'; import { eq, and } from 'drizzle-orm'; -import { roomingListEntries } from '@telivityhaip/database'; +import { roomingListEntries, roomTypes } from '@telivityhaip/database'; import { DRIZZLE } from '../../database/database.module'; import { WebhookService } from '../webhook/webhook.service'; import { ReservationService } from '../reservation/reservation.service'; @@ -31,6 +31,27 @@ export class RoomingListService { const results: any[] = []; for (const entry of dto.entries) { + // FK ownership (security audit follow-on): the caller-supplied entry.roomTypeId + // must belong to this property before we insert. Schema FK only constrains + // the row id — without this, a foreign roomTypeId would persist a + // cross-property FK in rooming_list_entries. + if (entry.roomTypeId) { + const [rt] = await this.db + .select({ id: roomTypes.id }) + .from(roomTypes) + .where(and(eq(roomTypes.id, entry.roomTypeId), eq(roomTypes.propertyId, propertyId))); + if (!rt) { + // Record the bad row as an error and skip — matches the rest of the + // batch's per-row error handling rather than aborting the whole import. + results.push({ + guestName: entry.guestName, + status: 'error', + error: `room type ${entry.roomTypeId} not found in this property`, + }); + errors++; + continue; + } + } // Insert the entry as pending first so it is always recorded. const [row] = await this.db .insert(roomingListEntries) diff --git a/apps/api/src/modules/housekeeping/housekeeping-dashboard.spec.ts b/apps/api/src/modules/housekeeping/housekeeping-dashboard.spec.ts index 6af8790..4aaf36f 100644 --- a/apps/api/src/modules/housekeeping/housekeeping-dashboard.spec.ts +++ b/apps/api/src/modules/housekeeping/housekeeping-dashboard.spec.ts @@ -213,11 +213,21 @@ describe('HousekeepingService — Dashboard & Analytics', () => { { id: 'room-002' }, ]); const db = createDashboardDb(); - // Override select for duplicate check — return empty (no existing tasks) + // Each room iteration runs through the where→then mock (in order): + // 1) duplicate stayover check (empty), + // 2) FK ownership check on rooms in create() (audit #6 — must return a row), + // 3) ADA accessible lookup inside generateChecklist (empty). + // The VIP reservation lookup uses leftJoin and goes through a different path + // (still []). So the where.then path cycles every 3 calls. + let nWhere = 0; db.select = vi.fn().mockImplementation(() => ({ from: vi.fn().mockReturnValue({ where: vi.fn().mockReturnValue({ - then: (resolve: any) => resolve([]), // no existing stayover task + then: (resolve: any) => { + nWhere++; + const inRoom = ((nWhere - 1) % 3) + 1; + resolve(inRoom === 2 ? [{ id: 'room-fk' }] : []); + }, orderBy: vi.fn().mockReturnValue({ limit: vi.fn().mockReturnValue({ then: (resolve: any) => resolve([]), diff --git a/apps/api/src/modules/housekeeping/housekeeping-fk-ownership.spec.ts b/apps/api/src/modules/housekeeping/housekeeping-fk-ownership.spec.ts new file mode 100644 index 0000000..97d422f --- /dev/null +++ b/apps/api/src/modules/housekeeping/housekeeping-fk-ownership.spec.ts @@ -0,0 +1,79 @@ +import { describe, it, expect, vi } from 'vitest'; +import { BadRequestException } from '@nestjs/common'; +import { Test } from '@nestjs/testing'; +import { HousekeepingService } from './housekeeping.service'; +import { DRIZZLE } from '../../database/database.module'; +import { WebhookService } from '../webhook/webhook.service'; +import { RoomStatusService } from '../room/room-status.service'; + +/** + * Cross-tenant FK ownership for HousekeepingService.create (security audit #6). + * Before the fix: dto.roomId from the caller was used to create a task and + * generate a checklist without first verifying the room belongs to dto.propertyId. + */ +const A = 'aaaaaaaa-0000-4000-a000-000000000001'; + +describe('HousekeepingService — create cross-tenant FK ownership (audit #6)', () => { + // Codex audit note: the explicit-checklist branch skips generateChecklist(). + // Both branches must be denied; we cover both. + it('rejects when dto.roomId belongs to another property (explicit-checklist branch)', async () => { + const db: any = { + select: vi.fn().mockReturnValue({ + from: vi.fn().mockReturnValue({ where: vi.fn().mockResolvedValue([]) }), + }), + insert: vi.fn(), + }; + const mod = await Test.createTestingModule({ + providers: [ + HousekeepingService, + { provide: DRIZZLE, useValue: db }, + { provide: WebhookService, useValue: { emit: vi.fn() } }, + { provide: RoomStatusService, useValue: {} }, + ], + }).compile(); + const svc = mod.get(HousekeepingService); + + await expect( + svc.create({ + propertyId: A, + roomId: 'foreign-room', + type: 'checkout', + serviceDate: '2026-07-01', + checklist: [], + } as any), + ).rejects.toBeInstanceOf(BadRequestException); + expect(db.insert).not.toHaveBeenCalled(); + }); + + it('rejects when dto.roomId belongs to another property (auto-checklist branch)', async () => { + // No checklist supplied → generateChecklist would run. The FK guard must + // STILL trip first, before any room/reservation lookup happens. + const db: any = { + select: vi.fn().mockReturnValue({ + from: vi.fn().mockReturnValue({ where: vi.fn().mockResolvedValue([]) }), + }), + insert: vi.fn(), + }; + const mod = await Test.createTestingModule({ + providers: [ + HousekeepingService, + { provide: DRIZZLE, useValue: db }, + { provide: WebhookService, useValue: { emit: vi.fn() } }, + { provide: RoomStatusService, useValue: {} }, + ], + }).compile(); + const svc = mod.get(HousekeepingService); + + await expect( + svc.create({ + propertyId: A, + roomId: 'foreign-room', + type: 'checkout', + serviceDate: '2026-07-01', + } as any), + ).rejects.toBeInstanceOf(BadRequestException); + expect(db.insert).not.toHaveBeenCalled(); + // Exactly one select (the FK check). If generateChecklist ran, we'd see more. + expect(db.select).toHaveBeenCalledTimes(1); + }); +}); diff --git a/apps/api/src/modules/housekeeping/housekeeping.service.spec.ts b/apps/api/src/modules/housekeeping/housekeeping.service.spec.ts index 30319b0..6658b37 100644 --- a/apps/api/src/modules/housekeeping/housekeeping.service.spec.ts +++ b/apps/api/src/modules/housekeeping/housekeeping.service.spec.ts @@ -170,9 +170,11 @@ describe('HousekeepingService — CRUD', () => { }); it('should create task with default checklist template when none provided', async () => { - // DB calls: 1=room lookup (isAccessible), 2=reservation+guest lookup (VIP), 3=insert + // DB calls: 1=FK ownership (room is same-property), 2=room lookup + // (isAccessible), 3=reservation+guest lookup (VIP), 4=insert const db = createMockDb({ selectResult: [ + [{ id: 'room-001' }], // FK ownership check — room belongs to dto.propertyId [{ isAccessible: false }], // room lookup [], // no incoming reservation ], @@ -194,6 +196,7 @@ describe('HousekeepingService — CRUD', () => { const expectedChecklist = [...CHECKLIST_TEMPLATES.checkout, ...ADA_EXTRA_ITEMS]; const db = createMockDb({ selectResult: [ + [{ id: 'room-001' }], // FK ownership check (audit #6) [{ isAccessible: true }], // room is ADA accessible [], // no incoming reservation ], @@ -214,6 +217,7 @@ describe('HousekeepingService — CRUD', () => { it('should add VIP extra items when next guest is VIP', async () => { const db = createMockDb({ selectResult: [ + [{ id: 'room-001' }], // FK ownership check (audit #6) [{ isAccessible: false }], // room not accessible [{ vipLevel: 'gold' }], // VIP guest incoming ], diff --git a/apps/api/src/modules/housekeeping/housekeeping.service.ts b/apps/api/src/modules/housekeeping/housekeeping.service.ts index 4c69f01..6b78c7b 100644 --- a/apps/api/src/modules/housekeeping/housekeeping.service.ts +++ b/apps/api/src/modules/housekeeping/housekeeping.service.ts @@ -33,6 +33,19 @@ export class HousekeepingService { ) {} async create(dto: CreateTaskDto) { + // FK ownership (security audit #6): verify roomId belongs to dto.propertyId + // BEFORE generating the checklist or inserting the task. Without this, a + // caller at property A could create a housekeeping task pointing at a room + // in property B (and the checklist generator would happily read B's room + // metadata — though the inner generate() lookups are now also scoped). + const [room] = await this.db + .select({ id: rooms.id }) + .from(rooms) + .where(and(eq(rooms.id, dto.roomId), eq(rooms.propertyId, dto.propertyId))); + if (!room) { + throw new BadRequestException(`room ${dto.roomId} not found in this property`); + } + let checklist = dto.checklist; if (!checklist) { diff --git a/apps/api/src/modules/rate-plan/rate-plan-fk-ownership.spec.ts b/apps/api/src/modules/rate-plan/rate-plan-fk-ownership.spec.ts new file mode 100644 index 0000000..f760e67 --- /dev/null +++ b/apps/api/src/modules/rate-plan/rate-plan-fk-ownership.spec.ts @@ -0,0 +1,51 @@ +import { describe, it, expect, vi } from 'vitest'; +import { BadRequestException } from '@nestjs/common'; +import { Test } from '@nestjs/testing'; +import { RatePlanService } from './rate-plan.service'; +import { DRIZZLE } from '../../database/database.module'; + +const A = 'aaaaaaaa-0000-4000-a000-000000000001'; + +describe('RatePlanService — cross-tenant FK ownership', () => { + it('update() rejects when dto.parentRatePlanId belongs to another property', async () => { + const db: any = { + select: vi.fn().mockReturnValue({ + from: vi.fn().mockReturnValue({ where: vi.fn().mockResolvedValue([]) }), // foreign + }), + update: vi.fn(), + }; + const mod = await Test.createTestingModule({ + providers: [RatePlanService, { provide: DRIZZLE, useValue: db }], + }).compile(); + const svc = mod.get(RatePlanService); + + await expect( + svc.update('rp-1', A, { parentRatePlanId: 'foreign-rp' } as any), + ).rejects.toBeInstanceOf(BadRequestException); + expect(db.update).not.toHaveBeenCalled(); + }); + + it('rejects when dto.roomTypeId belongs to another property', async () => { + const db: any = { + select: vi.fn().mockReturnValue({ + from: vi.fn().mockReturnValue({ where: vi.fn().mockResolvedValue([]) }), // foreign + }), + insert: vi.fn(), + }; + const mod = await Test.createTestingModule({ + providers: [RatePlanService, { provide: DRIZZLE, useValue: db }], + }).compile(); + const svc = mod.get(RatePlanService); + + await expect( + svc.create({ + propertyId: A, + roomTypeId: 'foreign-rt', + name: 'BAR', + type: 'standard', + baseAmount: '100.00', + } as any), + ).rejects.toBeInstanceOf(BadRequestException); + expect(db.insert).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/api/src/modules/rate-plan/rate-plan.service.spec.ts b/apps/api/src/modules/rate-plan/rate-plan.service.spec.ts index 9eea9a6..9b760db 100644 --- a/apps/api/src/modules/rate-plan/rate-plan.service.spec.ts +++ b/apps/api/src/modules/rate-plan/rate-plan.service.spec.ts @@ -174,7 +174,13 @@ describe('RatePlanService', () => { describe('create', () => { it('should reject derived rate without parent', async () => { const mockDb = { - select: vi.fn(), + // FK ownership check on roomTypes now runs first — return a row so + // we proceed to the derived-without-parent BadRequest path. + select: vi.fn().mockReturnValue({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockResolvedValue([{ id: 'rt-001' }]), + }), + }), insert: vi.fn(), update: vi.fn(), delete: vi.fn(), diff --git a/apps/api/src/modules/rate-plan/rate-plan.service.ts b/apps/api/src/modules/rate-plan/rate-plan.service.ts index accb28f..d2ef263 100644 --- a/apps/api/src/modules/rate-plan/rate-plan.service.ts +++ b/apps/api/src/modules/rate-plan/rate-plan.service.ts @@ -5,7 +5,7 @@ import { BadRequestException, } from '@nestjs/common'; import { eq, and } from 'drizzle-orm'; -import { ratePlans, rateRestrictions } from '@telivityhaip/database'; +import { ratePlans, rateRestrictions, roomTypes } from '@telivityhaip/database'; import { DRIZZLE } from '../../database/database.module'; import { CreateRatePlanDto } from './dto/create-rate-plan.dto'; import { UpdateRatePlanDto } from './dto/update-rate-plan.dto'; @@ -19,6 +19,17 @@ export class RatePlanService { // --- Rate Plans --- async create(dto: CreateRatePlanDto) { + // FK ownership (security audit follow-on): roomTypeId is required on the + // DTO. Without scoping to dto.propertyId, a caller at property A could + // create a rate plan pointing at property B's room type. The existing + // derived-parent check below already scopes parentRatePlanId. + const [rt] = await this.db + .select({ id: roomTypes.id }) + .from(roomTypes) + .where(and(eq(roomTypes.id, dto.roomTypeId), eq(roomTypes.propertyId, dto.propertyId))); + if (!rt) { + throw new BadRequestException(`room type ${dto.roomTypeId} not found in this property`); + } if (dto.type === 'derived') { if (!dto.parentRatePlanId || !dto.derivedAdjustmentType || !dto.derivedAdjustmentValue) { throw new BadRequestException( @@ -72,6 +83,19 @@ export class RatePlanService { } async update(id: string, propertyId: string, dto: UpdateRatePlanDto) { + // FK ownership (security audit follow-on): UpdateRatePlanDto omits propertyId + // and roomTypeId but NOT parentRatePlanId. Without scoping that FK to the + // request's propertyId, a caller could re-parent a rate plan onto another + // tenant's chain. Verify before the update. + if (dto.parentRatePlanId) { + const [parent] = await this.db + .select({ id: ratePlans.id }) + .from(ratePlans) + .where(and(eq(ratePlans.id, dto.parentRatePlanId), eq(ratePlans.propertyId, propertyId))); + if (!parent) { + throw new BadRequestException(`parent rate plan ${dto.parentRatePlanId} not found in this property`); + } + } const [ratePlan] = await this.db .update(ratePlans) .set({ ...dto, updatedAt: new Date() }) diff --git a/apps/api/src/modules/reservation/reservation-fk-ownership.spec.ts b/apps/api/src/modules/reservation/reservation-fk-ownership.spec.ts new file mode 100644 index 0000000..d37e581 --- /dev/null +++ b/apps/api/src/modules/reservation/reservation-fk-ownership.spec.ts @@ -0,0 +1,137 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { BadRequestException } from '@nestjs/common'; +import { Test } from '@nestjs/testing'; +import { ReservationService } from './reservation.service'; +import { DRIZZLE } from '../../database/database.module'; +import { AvailabilityService } from './availability.service'; +import { FolioService } from '../folio/folio.service'; +import { RoomStatusService } from '../room/room-status.service'; +import { PaymentService } from '../payment/payment.service'; +import { WebhookService } from '../webhook/webhook.service'; + +/** + * Cross-tenant FK ownership tests for ReservationService (security audit #4). + * + * Before the fix: dto.roomTypeId / dto.ratePlanId from the caller were inserted + * blindly. A caller at property A could reference property B's rate plan and + * leak its details back through the read join. Now the service verifies each FK + * belongs to the SAME propertyId as the request before any insert/update. + */ +const A = 'aaaaaaaa-0000-4000-a000-000000000001'; + +/** + * Sequenced-select mock. `create()` runs: + * 1) guest lookup (NotFoundException if empty) + * 2) roomTypes FK ownership check (BadRequestException if empty) ← the audit fix + * 3) ratePlans FK ownership check (BadRequestException if empty) ← the audit fix + * `modify()` runs: + * 1) findByIdRaw on reservations + * 2) roomTypes FK ownership (only if dto.roomTypeId) + * 3) ratePlans FK ownership (only if dto.ratePlanId) + */ +function mkDbSeq(selectResults: any[][]) { + let i = 0; + return { + select: vi.fn().mockImplementation(() => ({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockImplementation(() => Promise.resolve(selectResults[i++] ?? [])), + }), + })), + insert: vi.fn().mockReturnValue({ + values: vi.fn().mockReturnValue({ returning: vi.fn().mockResolvedValue([{}]) }), + }), + update: vi.fn(), + transaction: vi.fn().mockImplementation(async (cb: any) => cb({} as any)), + }; +} + +async function mkService(db: any) { + const mod = await Test.createTestingModule({ + providers: [ + ReservationService, + { provide: DRIZZLE, useValue: db }, + { provide: AvailabilityService, useValue: { searchAvailability: vi.fn().mockResolvedValue([]) } }, + { provide: FolioService, useValue: {} }, + { provide: RoomStatusService, useValue: {} }, + { provide: PaymentService, useValue: {} }, + { provide: WebhookService, useValue: { emit: vi.fn() } }, + ], + }).compile(); + return mod.get(ReservationService); +} + +describe('ReservationService — cross-tenant FK ownership (audit #4)', () => { + it('create() rejects when dto.roomTypeId belongs to another property', async () => { + // selects in order: guest (found), roomTypes FK check (empty = foreign). + const db = mkDbSeq([ + [{ id: 'g', isDnr: false }], // 1) guest lookup OK + [], // 2) FK check on roomTypes → not in this property + ]); + const svc = await mkService(db); + + await expect( + svc.create({ + propertyId: A, + roomTypeId: 'foreign-room-type', + ratePlanId: 'plan-1', + arrivalDate: '2026-07-01', + departureDate: '2026-07-03', + totalAmount: '300.00', + currencyCode: 'USD', + guestId: 'g', + } as any), + ).rejects.toBeInstanceOf(BadRequestException); + expect(db.insert).not.toHaveBeenCalled(); + }); + + it('create() rejects when dto.ratePlanId belongs to another property (roomType OK)', async () => { + const db = mkDbSeq([ + [{ id: 'g', isDnr: false }], // 1) guest lookup OK + [{ id: 'rt-1' }], // 2) FK check on roomTypes OK + [], // 3) FK check on ratePlans → not in this property + ]); + const svc = await mkService(db); + + await expect( + svc.create({ + propertyId: A, + roomTypeId: 'rt-1', + ratePlanId: 'foreign-plan', + arrivalDate: '2026-07-01', + departureDate: '2026-07-03', + totalAmount: '300.00', + currencyCode: 'USD', + guestId: 'g', + } as any), + ).rejects.toBeInstanceOf(BadRequestException); + expect(db.insert).not.toHaveBeenCalled(); + }); + + it('modify() rejects when dto.roomTypeId belongs to another property', async () => { + const db = mkDbSeq([ + // 1) findByIdRaw → reservation in propertyId A + [{ id: 'r-1', propertyId: A, status: 'confirmed', arrivalDate: '2026-07-01', departureDate: '2026-07-03', roomTypeId: 'rt-1' }], + // 2) FK check on roomTypes → empty (foreign) + [], + ]); + const svc = await mkService(db); + await expect( + svc.modify('r-1', A, { roomTypeId: 'foreign-room-type' } as any), + ).rejects.toBeInstanceOf(BadRequestException); + expect(db.transaction).not.toHaveBeenCalled(); + }); + + it('modify() rejects when dto.ratePlanId belongs to another property', async () => { + const db = mkDbSeq([ + // 1) findByIdRaw → reservation in propertyId A + [{ id: 'r-1', propertyId: A, status: 'confirmed', arrivalDate: '2026-07-01', departureDate: '2026-07-03', roomTypeId: 'rt-1' }], + // 2) FK check on ratePlans → empty (foreign) + [], + ]); + const svc = await mkService(db); + await expect( + svc.modify('r-1', A, { ratePlanId: 'foreign-rate-plan' } as any), + ).rejects.toBeInstanceOf(BadRequestException); + expect(db.transaction).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/api/src/modules/reservation/reservation.service.ts b/apps/api/src/modules/reservation/reservation.service.ts index 9eb6207..5beca60 100644 --- a/apps/api/src/modules/reservation/reservation.service.ts +++ b/apps/api/src/modules/reservation/reservation.service.ts @@ -67,6 +67,13 @@ export class ReservationService { // Generate confirmation number const confirmationNumber = `HAIP-${Date.now().toString(36).toUpperCase()}-${randomUUID().slice(0, 4).toUpperCase()}`; + // FK ownership (security audit #4): the caller supplies roomTypeId AND + // ratePlanId in the DTO. Without scoping these to dto.propertyId, a caller + // at property A could reference property B's rate plan / room type and + // leak its details back on read. Verify same-property before any insert. + await this.assertSamePropertyFk(roomTypes, dto.roomTypeId, dto.propertyId, 'room type'); + await this.assertSamePropertyFk(ratePlans, dto.ratePlanId, dto.propertyId, 'rate plan'); + // TOCTOU: availability check + insert run inside the same transaction so the // race window between "there's space" and "we wrote the booking" is minimized. // Postgres default isolation is READ COMMITTED, so concurrent txs can still @@ -782,8 +789,17 @@ export class ReservationService { updates['nights'] = nights; } - if (dto.roomTypeId) updates['roomTypeId'] = dto.roomTypeId; - if (dto.ratePlanId) updates['ratePlanId'] = dto.ratePlanId; + // FK ownership (security audit #4): if the caller is moving the reservation + // to a different room type or rate plan, verify each FK belongs to the same + // tenant before the update. + if (dto.roomTypeId) { + await this.assertSamePropertyFk(roomTypes, dto.roomTypeId, propertyId, 'room type'); + updates['roomTypeId'] = dto.roomTypeId; + } + if (dto.ratePlanId) { + await this.assertSamePropertyFk(ratePlans, dto.ratePlanId, propertyId, 'rate plan'); + updates['ratePlanId'] = dto.ratePlanId; + } if (dto.totalAmount) updates['totalAmount'] = dto.totalAmount; if (dto.adults !== undefined) updates['adults'] = dto.adults; if (dto.children !== undefined) updates['children'] = dto.children; @@ -1017,6 +1033,28 @@ export class ReservationService { ); } + /** + * Verify a caller-supplied FK belongs to the SAME property as the request. + * Without this, a caller at property A could pass a rate-plan / room-type id + * from property B and the row would happily insert (the schema FK doesn't + * enforce same-property because the FK is on the row id alone). Closes the + * cross-tenant integrity hole flagged as #4 in the security audit. + */ + private async assertSamePropertyFk( + table: { id: any; propertyId: any }, + id: string, + propertyId: string, + label: string, + ): Promise { + const [row] = await this.db + .select({ id: table.id }) + .from(table) + .where(and(eq(table.id, id), eq(table.propertyId, propertyId))); + if (!row) { + throw new BadRequestException(`${label} ${id} not found in this property`); + } + } + private async findByIdRaw(id: string, propertyId: string) { const [reservation] = await this.db .select() diff --git a/apps/api/src/modules/room/room-fk-ownership.spec.ts b/apps/api/src/modules/room/room-fk-ownership.spec.ts new file mode 100644 index 0000000..78ae01b --- /dev/null +++ b/apps/api/src/modules/room/room-fk-ownership.spec.ts @@ -0,0 +1,57 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { BadRequestException } from '@nestjs/common'; +import { Test } from '@nestjs/testing'; +import { RoomService } from './room.service'; +import { DRIZZLE } from '../../database/database.module'; + +/** + * Cross-tenant FK ownership for RoomService.createRoom (security audit #5). + * Before the fix: dto.roomTypeId from the caller was inserted blindly. A caller + * at property A could insert a room pointing at property B's room type + * (cross-tenant link the DB FK alone does not block). + */ +const A = 'aaaaaaaa-0000-4000-a000-000000000001'; + +describe('RoomService — createRoom cross-tenant FK ownership (audit #5)', () => { + it('rejects when dto.roomTypeId belongs to another property', async () => { + const db = { + // FK check returns [] → foreign room type. + select: vi.fn().mockReturnValue({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockResolvedValue([]), + }), + }), + insert: vi.fn(), + }; + const mod = await Test.createTestingModule({ + providers: [RoomService, { provide: DRIZZLE, useValue: db }], + }).compile(); + const svc = mod.get(RoomService); + + await expect( + svc.createRoom({ propertyId: A, roomTypeId: 'foreign-rt', roomNumber: '101' } as any), + ).rejects.toBeInstanceOf(BadRequestException); + expect(db.insert).not.toHaveBeenCalled(); + }); + + it('allows when dto.roomTypeId is same-property (insert runs)', async () => { + const db: any = { + select: vi.fn().mockReturnValue({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockResolvedValue([{ id: 'rt-1' }]), + }), + }), + insert: vi.fn().mockReturnValue({ + values: vi.fn().mockReturnValue({ returning: vi.fn().mockResolvedValue([{ id: 'room-1' }]) }), + }), + }; + const mod = await Test.createTestingModule({ + providers: [RoomService, { provide: DRIZZLE, useValue: db }], + }).compile(); + const svc = mod.get(RoomService); + + const out = await svc.createRoom({ propertyId: A, roomTypeId: 'rt-1', roomNumber: '101' } as any); + expect(out).toEqual({ id: 'room-1' }); + expect(db.insert).toHaveBeenCalled(); + }); +}); diff --git a/apps/api/src/modules/room/room.service.ts b/apps/api/src/modules/room/room.service.ts index b2e5753..7a8ebb5 100644 --- a/apps/api/src/modules/room/room.service.ts +++ b/apps/api/src/modules/room/room.service.ts @@ -1,4 +1,4 @@ -import { Injectable, Inject, NotFoundException } from '@nestjs/common'; +import { Injectable, Inject, NotFoundException, BadRequestException } from '@nestjs/common'; import { eq, and } from 'drizzle-orm'; import { rooms, roomTypes } from '@telivityhaip/database'; import { DRIZZLE } from '../../database/database.module'; @@ -43,6 +43,16 @@ export class RoomService { // --- Rooms --- async createRoom(dto: CreateRoomDto) { + // FK ownership (security audit #5): verify the caller's roomTypeId belongs + // to dto.propertyId before insert. Schema FK alone permits cross-tenant + // links because it only constrains row id. + const [rt] = await this.db + .select({ id: roomTypes.id }) + .from(roomTypes) + .where(and(eq(roomTypes.id, dto.roomTypeId), eq(roomTypes.propertyId, dto.propertyId))); + if (!rt) { + throw new BadRequestException(`room type ${dto.roomTypeId} not found in this property`); + } const [room] = await this.db.insert(rooms).values(dto).returning(); return room; }