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', () => {