fix(reservations): keep every id in a booking inside its own trip

reservation_edit on the trip in the URL says the caller may write here. It
says nothing about the ids they put in the body, and the write contracts are
open records, so day_id, place_id, assignment_id and accommodation_id all
arrived unchecked. A booking created in one trip could carry another trip's
accommodation_id, and deleting it ran the cascade against that foreign row.

The MCP tools have refused foreign ids since they were written. Put the same
check where every writer reaches it, and scope the two day_accommodations
statements to the trip so a row stored before this still cannot be followed
out of it.

server/src/nest/reservations/reservations.controller.ts

@@ -68,6 +68,7 @@ export class ReservationsController {
     @Headers('x-socket-id') socketId?: string,
   ) {
     const body = rawBody as ReservationBody & { title: string };
+    this.rejectForeignReferences(tripId, body);
     const { reservation, accommodationCreated } = this.reservations.create(tripId, body as never);
     if (accommodationCreated) {
       this.reservations.broadcast(tripId, 'accommodation:created', {}, socketId);
@@ -107,6 +108,7 @@ export class ReservationsController {
     if (!current) {
       throw new HttpException({ error: 'Reservation not found' }, 404);
     }
+    this.rejectForeignReferences(tripId, body);
     const { reservation, accommodationChanged } = this.reservations.update(id, tripId, body as never, current as never);
     if (accommodationChanged) {
       this.reservations.broadcast(tripId, 'accommodation:updated', {}, socketId);
@@ -162,4 +164,19 @@ export class ReservationsController {
     this.reservations.notifyBookingChange(tripId, user.id, deleted.title, deleted.type || '');
     return { success: true };
   }
+
+  /**
+   * The write contracts are open records, so day_id, place_id, assignment_id and
+   * accommodation_id arrive unvalidated. reservation_edit on :tripId says the
+   * caller may write HERE — it says nothing about the ids they put in the body,
+   * and a foreign accommodation_id used to be stored verbatim and deleted with
+   * the reservation. The MCP tools have refused foreign ids since they were
+   * written; this is the REST half of the same rule.
+   */
+  private rejectForeignReferences(tripId: string, body: ReservationBody): void {
+    const offenders = this.reservations.referencesOutsideTrip(tripId, body as never);
+    if (offenders.length > 0) {
+      throw new HttpException({ error: `Not part of this trip: ${offenders.join(', ')}` }, 400);
+    }
+  }
 }

server/src/nest/reservations/reservations.service.ts

@@ -432,6 +432,45 @@ export class ReservationsService {
     return this.reads.getReservationWithJoins(id);
   }
 
+  /**
+   * Name every id in the body that points outside this trip.
+   *
+   * Permission on the trip in the URL does not cover the body: it carries ids
+   * of its own, and a reservation happily stores a foreign accommodation_id,
+   * whose delete cascade then follows it out of the caller's own trip. The MCP
+   * tools have validated their referenced ids since they were written; this is
+   * the same check where every writer can reach it.
+   *
+   * Returns the offending field names, empty when the body is clean.
+   */
+  referencesOutsideTrip(tripId: string | number, data: CreateReservationData | UpdateReservationData): string[] {
+    const offenders: string[] = [];
+    const inTrip = (table: 'days' | 'places' | 'day_accommodations', id: unknown) =>
+      !!this.db.get(`SELECT id FROM ${table} WHERE id = ? AND trip_id = ?`, id, tripId);
+
+    const check = (field: string, ok: boolean) => { if (!ok) offenders.push(field); };
+
+    if (data.day_id != null) check('day_id', inTrip('days', data.day_id));
+    if (data.end_day_id != null) check('end_day_id', inTrip('days', data.end_day_id));
+    if (data.place_id != null) check('place_id', inTrip('places', data.place_id));
+    if (data.accommodation_id != null) check('accommodation_id', inTrip('day_accommodations', data.accommodation_id));
+    if (data.assignment_id != null) {
+      check('assignment_id', !!this.db.get(
+        'SELECT da.id FROM day_assignments da JOIN days d ON da.day_id = d.id WHERE da.id = ? AND d.trip_id = ?',
+        data.assignment_id, tripId,
+      ));
+    }
+
+    const acc = data.create_accommodation;
+    if (acc) {
+      if (acc.place_id != null) check('create_accommodation.place_id', inTrip('places', acc.place_id));
+      if (acc.start_day_id != null) check('create_accommodation.start_day_id', inTrip('days', acc.start_day_id));
+      if (acc.end_day_id != null) check('create_accommodation.end_day_id', inTrip('days', acc.end_day_id));
+    }
+
+    return offenders;
+  }
+
   /** The accommodation insert, the reservation insert, the endpoint save and
    *  the metadata sync are one logical write — all-or-nothing. */
   create(tripId: string | number, data: CreateReservationData): { reservation: ReservationRow; accommodationCreated: boolean } {
@@ -583,7 +622,9 @@ export class ReservationsService {
     // Update or create accommodation for hotel reservations
     let resolvedAccId: number | null = accommodation_id !== undefined ? (accommodation_id || null) : (current.accommodation_id ?? null);
     if (resolvedAccId) {
-      const accExists = this.db.get('SELECT id FROM day_accommodations WHERE id = ?', resolvedAccId);
+      // Scoped to the trip on purpose: an id belonging to someone else's trip
+      // must read as absent here, not as an accommodation to write through to.
+      const accExists = this.db.get('SELECT id FROM day_accommodations WHERE id = ? AND trip_id = ?', resolvedAccId, tripId);
       if (!accExists) resolvedAccId = null;
     }
     if (type === 'hotel' && create_accommodation) {
@@ -720,8 +761,13 @@ export class ReservationsService {
 
       let accommodationDeleted = false;
       if (reservation.accommodation_id) {
-        this.db.run('DELETE FROM day_accommodations WHERE id = ?', reservation.accommodation_id);
-        accommodationDeleted = true;
+        // trip_id in the WHERE, not just the reservation's own scope: a row
+        // written before referencesOutsideTrip existed can still carry a
+        // foreign accommodation_id, and the cascade must not follow it.
+        const removed = this.db.run(
+          'DELETE FROM day_accommodations WHERE id = ? AND trip_id = ?', reservation.accommodation_id, tripId,
+        );
+        accommodationDeleted = removed.changes > 0;
       }
 
       const linkedBudget = this.db.get<{ id: number }>('SELECT id FROM budget_items WHERE trip_id = ? AND reservation_id = ?', tripId, id);

server/tests/unit/nest/reservations.controller.test.ts

@@ -15,6 +15,7 @@ function makeService(overrides: Partial<ReservationsService> = {}): Reservations
   return {
     verifyTripAccess: vi.fn().mockReturnValue(trip),
     canEdit: vi.fn().mockReturnValue(true),
+    referencesOutsideTrip: vi.fn().mockReturnValue([]),
     broadcast: vi.fn(),
     syncBudgetOnCreate: vi.fn(),
     syncBudgetOnUpdate: vi.fn(),
@@ -55,6 +56,18 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese
       expect(broadcast).toHaveBeenCalledWith('5', 'reservation:created', { reservation: { id: 9 } }, 'sock');
       expect(notifyBookingChange).toHaveBeenCalledWith('5', user.id, 'Hotel', 'lodging');
     });
+
+    it('400s on a body id belonging to another trip, without writing', () => {
+      const create = vi.fn();
+      const svc = makeService({
+        create,
+        referencesOutsideTrip: vi.fn().mockReturnValue(['accommodation_id']),
+      } as Partial<ReservationsService>);
+      const body = { title: 'Hotel', accommodation_id: 4711 };
+      expect(thrown(() => new ReservationsController(svc, airtrailLink).create(user, '5', body)))
+        .toEqual({ status: 400, body: { error: 'Not part of this trip: accommodation_id' } });
+      expect(create).not.toHaveBeenCalled();
+    });
   });
 
   describe('PUT /positions', () => {
@@ -87,6 +100,18 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese
       expect(syncBudgetOnUpdate).toHaveBeenCalledWith('5', '9', '', undefined, 'Old', 'lodging', { total_price: 50 }, 'sock');
       expect(notifyBookingChange).toHaveBeenCalledWith('5', user.id, 'Old', 'lodging');
     });
+
+    it('400s on a body id belonging to another trip, without writing', () => {
+      const update = vi.fn();
+      const svc = makeService({
+        getReservation: vi.fn().mockReturnValue({ title: 'Old', type: 'lodging' }),
+        update,
+        referencesOutsideTrip: vi.fn().mockReturnValue(['day_id', 'place_id']),
+      } as Partial<ReservationsService>);
+      expect(thrown(() => new ReservationsController(svc, airtrailLink).update(user, '5', '9', { day_id: 1, place_id: 2 })))
+        .toEqual({ status: 400, body: { error: 'Not part of this trip: day_id, place_id' } });
+      expect(update).not.toHaveBeenCalled();
+    });
   });
 
   describe('PUT /:id/travelers', () => {

server/tests/unit/nest/reservations.service.test.ts

@@ -816,3 +816,65 @@ describe('ReservationsService — quirk fixes (post-fold)', () => {
     expect(testDb.prepare('SELECT title FROM reservations WHERE id = ?').get(res.id)).toEqual({ title: 'Old' });
   });
 });
+
+describe('ReservationsService — referenced ids stay inside the trip', () => {
+  /** Attacker's trip and a victim trip they are not a member of. */
+  function twoTrips() {
+    const { user: attacker } = createUser(testDb);
+    const { user: victim } = createUser(testDb, { email: 'victim@example.test' });
+    const mine = createTrip(testDb, attacker.id, { start_date: '2030-05-01', end_date: '2030-05-03' });
+    const theirs = createTrip(testDb, victim.id, { start_date: '2030-05-01', end_date: '2030-05-03' });
+    return { mine, theirs };
+  }
+
+  it('RESV-SCOPE-001: names every foreign id in the body', () => {
+    const { mine, theirs } = twoTrips();
+    const foreignPlace = createPlace(testDb, theirs.id);
+    const foreignDay = testDb.prepare('SELECT id FROM days WHERE trip_id = ? ORDER BY day_number').get(theirs.id) as { id: number };
+    const foreignAcc = createDayAccommodation(testDb, theirs.id, foreignPlace.id, foreignDay.id, foreignDay.id);
+
+    expect(svc.referencesOutsideTrip(String(mine.id), {
+      title: 'x', day_id: foreignDay.id, place_id: foreignPlace.id, accommodation_id: foreignAcc.id,
+    })).toEqual(['day_id', 'place_id', 'accommodation_id']);
+  });
+
+  it('RESV-SCOPE-002: passes a body whose ids all live in the trip', () => {
+    const { mine } = twoTrips();
+    const place = createPlace(testDb, mine.id);
+    const day = testDb.prepare('SELECT id FROM days WHERE trip_id = ? ORDER BY day_number').get(mine.id) as { id: number };
+
+    expect(svc.referencesOutsideTrip(String(mine.id), { title: 'x', day_id: day.id, place_id: place.id })).toEqual([]);
+    expect(svc.referencesOutsideTrip(String(mine.id), { title: 'x' })).toEqual([]);
+  });
+
+  it('RESV-SCOPE-003: a stored foreign accommodation_id is not deleted with the reservation', () => {
+    const { mine, theirs } = twoTrips();
+    const foreignPlace = createPlace(testDb, theirs.id);
+    const foreignDay = testDb.prepare('SELECT id FROM days WHERE trip_id = ? ORDER BY day_number').get(theirs.id) as { id: number };
+    const foreignAcc = createDayAccommodation(testDb, theirs.id, foreignPlace.id, foreignDay.id, foreignDay.id);
+    // A row from before the check existed: it already carries the foreign id.
+    const res = createReservation(testDb, mine.id, { title: 'Hotel', type: 'hotel' });
+    testDb.prepare('UPDATE reservations SET accommodation_id = ? WHERE id = ?').run(foreignAcc.id, res.id);
+
+    const { accommodationDeleted } = svc.remove(String(res.id), String(mine.id));
+
+    expect(accommodationDeleted).toBe(false);
+    expect(testDb.prepare('SELECT id FROM day_accommodations WHERE id = ?').get(foreignAcc.id)).toBeTruthy();
+  });
+
+  it('RESV-SCOPE-004: an update carrying a foreign accommodation_id does not write through to it', () => {
+    const { mine, theirs } = twoTrips();
+    const foreignPlace = createPlace(testDb, theirs.id);
+    const foreignDay = testDb.prepare('SELECT id FROM days WHERE trip_id = ? ORDER BY day_number').get(theirs.id) as { id: number };
+    const foreignAcc = createDayAccommodation(testDb, theirs.id, foreignPlace.id, foreignDay.id, foreignDay.id, { check_in: '14:00' });
+    const res = createReservation(testDb, mine.id, { title: 'Hotel', type: 'hotel' });
+    const current = svc.getReservation(String(res.id), String(mine.id))!;
+
+    svc.update(String(res.id), String(mine.id), {
+      accommodation_id: foreignAcc.id, metadata: { check_in_time: '23:00' },
+    } as never, current);
+
+    expect(testDb.prepare('SELECT check_in FROM day_accommodations WHERE id = ?').get(foreignAcc.id)).toEqual({ check_in: '14:00' });
+    expect(testDb.prepare('SELECT accommodation_id FROM reservations WHERE id = ?').get(res.id)).toEqual({ accommodation_id: null });
+  });
+});