fix(security): cross-tenant FK ownership in reservation/room/housekeeping (audit #4, #5, #6)

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();
+  });
+});

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({

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();
+  });
+});

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({

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();
+  });
+});

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({

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();
+  });
+});