diff --git a/server/src/nest/reservations/reservations.controller.ts b/server/src/nest/reservations/reservations.controller.ts index 13b5987b9..ccde3f7e1 100644 --- a/server/src/nest/reservations/reservations.controller.ts +++ b/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); + } + } } diff --git a/server/src/nest/reservations/reservations.service.ts b/server/src/nest/reservations/reservations.service.ts index 15ebbaa2e..67960dca3 100644 --- a/server/src/nest/reservations/reservations.service.ts +++ b/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); diff --git a/server/tests/unit/nest/reservations.controller.test.ts b/server/tests/unit/nest/reservations.controller.test.ts index 840435c9d..4c994d001 100644 --- a/server/tests/unit/nest/reservations.controller.test.ts +++ b/server/tests/unit/nest/reservations.controller.test.ts @@ -15,6 +15,7 @@ function makeService(overrides: Partial = {}): 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); + 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); + 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', () => { diff --git a/server/tests/unit/nest/reservations.service.test.ts b/server/tests/unit/nest/reservations.service.test.ts index 60f0d94ed..89f4310ea 100644 --- a/server/tests/unit/nest/reservations.service.test.ts +++ b/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 }); + }); +});