From be1847bc889f69335db9e3fd9c94b227ee580878 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Du=C5=A1an=20Mili=C4=87evi=C4=87?= Date: Sat, 20 Jun 2026 17:41:28 -0500 Subject: [PATCH] =?UTF-8?q?fix(security):=20scope=20connect=20modify()=20g?= =?UTF-8?q?uest=20write=20=E2=80=94=20fork=20shared=20guest=20instead=20of?= =?UTF-8?q?=20overwriting?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up: findOrCreateGuest reuse was scoped to the property, but modify() still updated the cross-property guests row by bare id, so a caller could rename a guest shared with another tenant (PII poison on legacy shared rows). Now: if the guest is linked to any OTHER property, fork a property-local copy and repoint this reservation; otherwise update in place. Co-Authored-By: Claude Opus 4.8 --- .../connect/connect-booking.service.spec.ts | 65 +++++++++++++++++++ .../connect/connect-booking.service.ts | 49 ++++++++++++-- 2 files changed, 108 insertions(+), 6 deletions(-) diff --git a/apps/api/src/modules/connect/connect-booking.service.spec.ts b/apps/api/src/modules/connect/connect-booking.service.spec.ts index 63c017a..d7e5f1e 100644 --- a/apps/api/src/modules/connect/connect-booking.service.spec.ts +++ b/apps/api/src/modules/connect/connect-booking.service.spec.ts @@ -419,6 +419,71 @@ describe('ConnectBookingService', () => { expect(mockAvailabilityService.searchAvailability).toHaveBeenCalled(); }); + it('forks a property-local guest on name change when the guest is shared with another property', async () => { + let selectCallCount = 0; + mockDb.select.mockImplementation(() => ({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockImplementation(() => { + selectCallCount++; + if (selectCallCount === 1) return Promise.resolve([{ id: 'booking-1', propertyId: 'prop-1' }]); + if (selectCallCount === 2) return Promise.resolve([{ + id: 'res-1', bookingId: 'booking-1', guestId: 'guest-shared', + status: 'confirmed', totalAmount: '399.98', + arrivalDate: '2024-06-01', departureDate: '2024-06-03', + }]); + if (selectCallCount === 3) return Promise.resolve([{ id: 'res-other' }]); // guest linked at ANOTHER property + if (selectCallCount === 4) return Promise.resolve([{ id: 'guest-shared', firstName: 'John', lastName: 'Smith', email: 'j@x.com' }]); + return Promise.resolve([]); + }), + }), + })); + + let insertCount = 0; + mockDb.insert.mockImplementation(() => ({ + values: vi.fn().mockReturnValue({ + returning: vi.fn().mockImplementation(() => { + insertCount++; + return Promise.resolve([{ id: 'guest-forked', firstName: 'Johnny', lastName: 'Smith' }]); + }), + }), + })); + + const result = await service.modify('HAIP-123', { guestFirstName: 'Johnny' }); + + expect(result.success).toBe(true); + // A shared guest must NOT be overwritten in place — a property-local copy is forked. + expect(insertCount).toBe(1); + }); + + it('updates the guest in place on name change when the guest is NOT shared', async () => { + let selectCallCount = 0; + mockDb.select.mockImplementation(() => ({ + from: vi.fn().mockReturnValue({ + where: vi.fn().mockImplementation(() => { + selectCallCount++; + if (selectCallCount === 1) return Promise.resolve([{ id: 'booking-1', propertyId: 'prop-1' }]); + if (selectCallCount === 2) return Promise.resolve([{ + id: 'res-1', bookingId: 'booking-1', guestId: 'guest-1', + status: 'confirmed', totalAmount: '399.98', + arrivalDate: '2024-06-01', departureDate: '2024-06-03', + }]); + if (selectCallCount === 3) return Promise.resolve([]); // no other-property links → not shared + return Promise.resolve([]); + }), + }), + })); + + let insertCount = 0; + mockDb.insert.mockImplementation(() => ({ + values: vi.fn().mockReturnValue({ returning: vi.fn().mockImplementation(() => { insertCount++; return Promise.resolve([{ id: 'x' }]); }) }), + })); + + const result = await service.modify('HAIP-123', { guestFirstName: 'Johnny' }); + + expect(result.success).toBe(true); + expect(insertCount).toBe(0); // updated in place, no fork + }); + it('should reject modification of cancelled reservation', async () => { let selectCallCount = 0; mockDb.select.mockImplementation(() => ({ diff --git a/apps/api/src/modules/connect/connect-booking.service.ts b/apps/api/src/modules/connect/connect-booking.service.ts index e4c5622..a03996a 100644 --- a/apps/api/src/modules/connect/connect-booking.service.ts +++ b/apps/api/src/modules/connect/connect-booking.service.ts @@ -1,5 +1,5 @@ import { Injectable, Inject, NotFoundException, BadRequestException } from '@nestjs/common'; -import { eq, and } from 'drizzle-orm'; +import { eq, and, ne } from 'drizzle-orm'; import Decimal from 'decimal.js'; import { bookings, reservations, guests, ratePlans, roomTypes, folios, rooms } from '@telivityhaip/database'; import { DRIZZLE } from '../../database/database.module'; @@ -263,15 +263,52 @@ export class ConnectBookingService { const previousAmountDec = new Decimal(reservation.totalAmount); const previousAmount = previousAmountDec.toNumber(); - // Handle guest detail updates + // Handle guest detail updates. The `guests` row is cross-property by design, + // so overwriting it in place would corrupt the profile as seen by OTHER + // properties that share this guest. Only mutate in place when the guest is + // NOT linked to any other property; otherwise fork a property-local copy and + // repoint this reservation, leaving the shared row untouched. if (dto.guestFirstName || dto.guestLastName) { const guestUpdate: Record = {}; if (dto.guestFirstName) guestUpdate['firstName'] = dto.guestFirstName; if (dto.guestLastName) guestUpdate['lastName'] = dto.guestLastName; - await this.db - .update(guests) - .set(guestUpdate) - .where(eq(guests.id, reservation.guestId)); + + const otherPropertyLinks = await this.db + .select({ id: reservations.id }) + .from(reservations) + .where( + and( + eq(reservations.guestId, reservation.guestId), + ne(reservations.propertyId, booking.propertyId), + ), + ); + + if (otherPropertyLinks.length > 0) { + const [current] = await this.db + .select() + .from(guests) + .where(eq(guests.id, reservation.guestId)); + const [forked] = await this.db + .insert(guests) + .values({ + firstName: guestUpdate['firstName'] ?? current?.firstName, + lastName: guestUpdate['lastName'] ?? current?.lastName, + email: current?.email ?? null, + phone: current?.phone ?? null, + loyaltyNumber: current?.loyaltyNumber ?? null, + }) + .returning(); + await this.db + .update(reservations) + .set({ guestId: forked.id, updatedAt: new Date() }) + .where(eq(reservations.id, reservation.id)); + reservation.guestId = forked.id; + } else { + await this.db + .update(guests) + .set(guestUpdate) + .where(eq(guests.id, reservation.guestId)); + } } // Handle simple field updates