From ff27624e0248f6187bcc8d86289622961a7fe4b2 Mon Sep 17 00:00:00 2001 From: Maurice Date: Sun, 16 Aug 2026 21:37:21 +0200 Subject: [PATCH] fix(reservations): scope the per-day position write to the trip updatePositions has two branches. The legacy one updates `WHERE id = ? AND trip_id = ?`; the per-day one, three lines below it, bound the caller's ids straight into reservation_day_positions and never touched the tripId it was handed. That table has no trip_id of its own and its two foreign keys only require the ids to exist, so nothing further down caught a pair that did not belong together. The trip now comes from the join, so the row materialises only when the reservation and the day agree on it. Doing that in the statement rather than as a pre-check also turns a stale id into a quiet no-op instead of a foreign-key error surfacing as a 500, and lets an omitted day_plan_position bind the way the wire contract has always said it may. reorder_reservations reports a day that is not on the trip, the way its sibling tools in that file already do, and a migration clears rows whose reservation and day disagree. --- server/src/db/migrations.ts | 18 ++++ .../src/nest/reservations/reservations.mcp.ts | 7 ++ .../nest/reservations/reservations.service.ts | 21 ++++- server/tests/e2e/reservations.e2e.test.ts | 88 +++++++++++++++++++ .../integration/leg-mode-incoming.test.ts | 26 ++++-- .../tests/unit/db/migration-hygiene.test.ts | 2 + 6 files changed, 150 insertions(+), 12 deletions(-) diff --git a/server/src/db/migrations.ts b/server/src/db/migrations.ts index dfa0b72c9..6fc504466 100644 --- a/server/src/db/migrations.ts +++ b/server/src/db/migrations.ts @@ -3891,6 +3891,24 @@ function runMigrations(db: Database.Database): void { db.exec('ALTER TABLE budget_items ADD COLUMN place_id INTEGER REFERENCES places(id) ON DELETE SET NULL DEFAULT NULL'); } }, + // A reservation_day_positions row only makes sense when its reservation and + // its day are on the same trip. The table carries no trip_id and its two + // foreign keys only ask that the ids exist, so pairs that never belonged + // together could accumulate; the writer refuses them now, and this clears + // whatever an older build let through. Appended LAST — the array is + // index-addressed against schema_version. + () => { + db.exec(` + DELETE FROM reservation_day_positions + WHERE rowid IN ( + SELECT rdp.rowid + FROM reservation_day_positions rdp + JOIN reservations r ON r.id = rdp.reservation_id + JOIN days d ON d.id = rdp.day_id + WHERE d.trip_id <> r.trip_id + ) + `); + }, ]; if (currentVersion < migrations.length) { diff --git a/server/src/nest/reservations/reservations.mcp.ts b/server/src/nest/reservations/reservations.mcp.ts index 4e7114b0c..f8b3926ba 100644 --- a/server/src/nest/reservations/reservations.mcp.ts +++ b/server/src/nest/reservations/reservations.mcp.ts @@ -468,6 +468,13 @@ export class ReservationsMcp { if (this.auth.isDemoUser(ctx.userId)) return demoDenied(); if (!this.reservations.verifyTripAccess(tripId, ctx.userId)) return noAccess(); if (!this.guards.hasTripPermission('reservation_edit', tripId, ctx.userId)) return permissionDenied(); + + // The service scopes the write to the trip on its own, so a foreign id is + // already harmless — say so rather than reporting a success that moved + // nothing, the way the sibling tools above do. + if (dayId && !this.days.getDay(dayId, tripId)) + return errorResult('dayId does not belong to this trip.'); + this.reservations.updatePositions(tripId, positions, dayId); this.guards.safeBroadcast(tripId, 'reservation:positions', { positions, dayId }); return ok({ success: true }); diff --git a/server/src/nest/reservations/reservations.service.ts b/server/src/nest/reservations/reservations.service.ts index 3f0a814db..15ebbaa2e 100644 --- a/server/src/nest/reservations/reservations.service.ts +++ b/server/src/nest/reservations/reservations.service.ts @@ -527,13 +527,26 @@ export class ReservationsService { return { reservation, accommodationCreated }; } - updatePositions(tripId: string | number, positions: { id: number; day_plan_position: number }[], dayId?: number | string | null) { + updatePositions(tripId: string | number, positions: { id: number; day_plan_position?: number }[], dayId?: number | string | null) { if (dayId) { - // Per-day positions for multi-day reservations - const stmt = this.db.prepare('INSERT OR REPLACE INTO reservation_day_positions (reservation_id, day_id, position) VALUES (?, ?, ?)'); + // Per-day positions for multi-day reservations, scoped the way the legacy + // branch below already scopes its update. The table carries no trip_id and + // its two foreign keys only ask that the ids exist, so the trip has to come + // from the join: the row materialises only when the reservation and the day + // agree on it. Doing that in the statement rather than as a pre-check also + // makes a stale id a quiet no-op instead of a foreign-key error surfacing + // as a 500. + const stmt = this.db.prepare(` + INSERT OR REPLACE INTO reservation_day_positions (reservation_id, day_id, position) + SELECT r.id, d.id, ? + FROM reservations r + JOIN days d ON d.trip_id = r.trip_id + WHERE r.id = ? AND d.id = ? AND r.trip_id = ? + `); this.db.transaction(() => { for (const item of positions) { - stmt.run(item.id, dayId, item.day_plan_position); + // position is NOT NULL while the wire contract leaves the value optional. + stmt.run(item.day_plan_position ?? 0, item.id, dayId, tripId); } }); } else { diff --git a/server/tests/e2e/reservations.e2e.test.ts b/server/tests/e2e/reservations.e2e.test.ts index bcac65060..7907ae5ca 100644 --- a/server/tests/e2e/reservations.e2e.test.ts +++ b/server/tests/e2e/reservations.e2e.test.ts @@ -185,4 +185,92 @@ describe('Reservations + accommodations e2e (real auth guard + temp SQLite, real expect(res.status).toBe(400); expect(res.body).toEqual({ error: 'place_id, start_day_id, and end_day_id are required' }); }); + + // The per-day branch of updatePositions used to bind the caller's ids straight + // into reservation_day_positions without ever looking at the tripId it was + // handed, so a request authorised on your own trip could rewrite the ordering + // of someone else's. The legacy branch three lines below it always scoped by + // trip_id — these pin that the two behave the same way now. + describe('positions are confined to the trip in the URL', () => { + let foreignTripId: number; + let foreignReservationId: number; + let foreignDayId: number; + let ownDayId: number; + // Days are unique per (trip_id, day_number) and this block seeds a fresh pair + // for every case, so the numbers have to keep climbing. + let dayNumber = 100; + + beforeEach(() => { + db.prepare( + "INSERT OR IGNORE INTO users (id, username, email, password_hash, role, password_version) VALUES (2, 'victim', 'victim@example.test', 'x', 'user', 0)", + ).run(); + foreignTripId = Number(db.prepare("INSERT INTO trips (user_id, title) VALUES (2, 'Someone else')").run().lastInsertRowid); + foreignReservationId = Number(db.prepare("INSERT INTO reservations (trip_id, title, type) VALUES (?, 'Secret', 'other')").run(foreignTripId).lastInsertRowid); + dayNumber += 1; + foreignDayId = Number(db.prepare("INSERT INTO days (trip_id, day_number, date) VALUES (?, ?, '2026-05-01')").run(foreignTripId, dayNumber).lastInsertRowid); + ownDayId = Number(db.prepare("INSERT INTO days (trip_id, day_number, date) VALUES (?, ?, '2026-05-01')").run(tripId, dayNumber).lastInsertRowid); + }); + + function positionRow(reservationId: number, dayId: number) { + return db.prepare('SELECT position FROM reservation_day_positions WHERE reservation_id = ? AND day_id = ?').get(reservationId, dayId); + } + + it('writes nothing when both ids belong to another trip', async () => { + const res = await request(server) + .put(`/api/trips/${tripId}/reservations/positions`) + .set('Cookie', sessionCookie(1)) + .send({ positions: [{ id: foreignReservationId, day_plan_position: 999 }], day_id: foreignDayId }); + expect(res.status).toBe(200); + expect(positionRow(foreignReservationId, foreignDayId)).toBeUndefined(); + }); + + it('writes nothing when only the reservation is foreign', async () => { + const res = await request(server) + .put(`/api/trips/${tripId}/reservations/positions`) + .set('Cookie', sessionCookie(1)) + .send({ positions: [{ id: foreignReservationId, day_plan_position: 5 }], day_id: ownDayId }); + expect(res.status).toBe(200); + expect(positionRow(foreignReservationId, ownDayId)).toBeUndefined(); + }); + + it('writes nothing when only the day is foreign', async () => { + const ownReservationId = Number(db.prepare("INSERT INTO reservations (trip_id, title, type) VALUES (?, 'Mine', 'other')").run(tripId).lastInsertRowid); + const res = await request(server) + .put(`/api/trips/${tripId}/reservations/positions`) + .set('Cookie', sessionCookie(1)) + .send({ positions: [{ id: ownReservationId, day_plan_position: 5 }], day_id: foreignDayId }); + expect(res.status).toBe(200); + expect(positionRow(ownReservationId, foreignDayId)).toBeUndefined(); + }); + + it('still stores a position when both ids are on the trip', async () => { + const ownReservationId = Number(db.prepare("INSERT INTO reservations (trip_id, title, type) VALUES (?, 'Mine', 'other')").run(tripId).lastInsertRowid); + const res = await request(server) + .put(`/api/trips/${tripId}/reservations/positions`) + .set('Cookie', sessionCookie(1)) + .send({ positions: [{ id: ownReservationId, day_plan_position: 3 }], day_id: ownDayId }); + expect(res.status).toBe(200); + expect(positionRow(ownReservationId, ownDayId)).toEqual({ position: 3 }); + }); + + // An id that exists nowhere used to raise a foreign-key error, which the + // exception filter turned into a 500 — telling the caller apart from the + // 200 a real id returns, for any id on the instance. + it('answers a nonexistent reservation the same way it answers a foreign one', async () => { + const res = await request(server) + .put(`/api/trips/${tripId}/reservations/positions`) + .set('Cookie', sessionCookie(1)) + .send({ positions: [{ id: 999999, day_plan_position: 1 }], day_id: ownDayId }); + expect(res.status).toBe(200); + }); + + it('does not fall over when day_plan_position is omitted', async () => { + const ownReservationId = Number(db.prepare("INSERT INTO reservations (trip_id, title, type) VALUES (?, 'Mine', 'other')").run(tripId).lastInsertRowid); + const res = await request(server) + .put(`/api/trips/${tripId}/reservations/positions`) + .set('Cookie', sessionCookie(1)) + .send({ positions: [{ id: ownReservationId }], day_id: ownDayId }); + expect(res.status).toBe(200); + }); + }); }); diff --git a/server/tests/integration/leg-mode-incoming.test.ts b/server/tests/integration/leg-mode-incoming.test.ts index 4fb2739f6..eabe889dd 100644 --- a/server/tests/integration/leg-mode-incoming.test.ts +++ b/server/tests/integration/leg-mode-incoming.test.ts @@ -41,24 +41,34 @@ describe('incoming_leg_transport_mode migration', () => { // skipped on every one of them. Undoing what the LAST migration did and // rewinding one version is the only way to prove it still runs. // - // >>> Appending a migration? Re-point the DROP below at the column yours - // >>> adds. That is the whole maintenance cost of this guard. - const TRAILING_TABLE = 'budget_items'; - const TRAILING_COLUMN = 'place_id'; - + // >>> Appending a migration? Re-point the "undo" below at whatever yours + // >>> does. That is the whole maintenance cost of this guard. The current + // >>> trailing slot deletes reservation_day_positions rows whose reservation + // >>> and day sit on different trips, so undoing it means putting one back. const upgraded = new Database(':memory:'); upgraded.exec('PRAGMA foreign_keys = ON'); createTables(upgraded); runMigrations(upgraded); const { version } = upgraded.prepare('SELECT version FROM schema_version').get() as { version: number }; - upgraded.exec(`ALTER TABLE ${TRAILING_TABLE} DROP COLUMN ${TRAILING_COLUMN}`); + + upgraded.prepare("INSERT INTO users (id, username, email, password_hash) VALUES (1, 'a', 'a@test.local', 'x')").run(); + upgraded.prepare("INSERT INTO users (id, username, email, password_hash) VALUES (2, 'b', 'b@test.local', 'x')").run(); + const tripA = upgraded.prepare("INSERT INTO trips (user_id, title) VALUES (1, 'A')").run().lastInsertRowid; + const tripB = upgraded.prepare("INSERT INTO trips (user_id, title) VALUES (2, 'B')").run().lastInsertRowid; + const reservationOnA = upgraded.prepare("INSERT INTO reservations (trip_id, title, type) VALUES (?, 'r', 'other')").run(tripA).lastInsertRowid; + const dayOnA = upgraded.prepare("INSERT INTO days (trip_id, day_number, date) VALUES (?, 1, '2026-01-01')").run(tripA).lastInsertRowid; + const dayOnB = upgraded.prepare("INSERT INTO days (trip_id, day_number, date) VALUES (?, 1, '2026-01-01')").run(tripB).lastInsertRowid; + const insertPosition = upgraded.prepare('INSERT INTO reservation_day_positions (reservation_id, day_id, position) VALUES (?, ?, ?)'); + insertPosition.run(reservationOnA, dayOnB, 1); // the mismatched pair the migration clears + insertPosition.run(reservationOnA, dayOnA, 2); // a legitimate one it must leave alone + upgraded.prepare('UPDATE schema_version SET version = ?').run(version - 1); runMigrations(upgraded); - const cols = upgraded.prepare(`PRAGMA table_info(${TRAILING_TABLE})`).all() as { name: string }[]; - expect(cols.map(c => c.name)).toContain(TRAILING_COLUMN); + const rows = upgraded.prepare('SELECT day_id FROM reservation_day_positions ORDER BY day_id').all() as { day_id: number }[]; + expect(rows).toEqual([{ day_id: Number(dayOnA) }]); expect(upgraded.prepare('SELECT version FROM schema_version').get()).toEqual({ version }); upgraded.close(); }); diff --git a/server/tests/unit/db/migration-hygiene.test.ts b/server/tests/unit/db/migration-hygiene.test.ts index 358318260..44b4ecd88 100644 --- a/server/tests/unit/db/migration-hygiene.test.ts +++ b/server/tests/unit/db/migration-hygiene.test.ts @@ -135,6 +135,8 @@ const ALLOWED_DESTRUCTIVE: Record = { 'Atlas enclave fix: DELETE ... WHERE place_id IN (places inside specific enclave boxes) — invalidate stale region cache; re-resolved on next request.', 'DELETE FROM visited_regions': 'Atlas geoBoundaries swap (#1119): DELETE ... WHERE id = ? — after UPDATE OR IGNORE re-codes a manually-marked region to its current code, drop only the single leftover row whose UNIQUE(user_id, region_code) collision caused the update to be skipped (a duplicate of a region the user already has).', + 'DELETE FROM reservation_day_positions': + 'DELETE ... WHERE the row joins a reservation and a day sitting on different trips. The table has no trip_id and its two foreign keys only require the ids to exist, so a pair that never belonged together was storable; the writer scopes to the trip now and this clears what earlier builds allowed. Bounded by the join — a row whose reservation and day agree on their trip is untouched.', }; describe('migration hygiene — destructive operation guard', () => {