Cross-tenant IDOR in reservation day-position writes (REST + MCP) via /trips/:tripId/reservations/positions

Summary

updatePositions() in server/src/services/reservationService.ts writes a row into reservation_day_positions using an attacker-supplied reservation_id and day_id without ever checking they belong to the tripId the caller is authorized on. Both the REST endpoint (PUT /trips/:tripId/reservations/positions) and the reorder_reservations MCP tool call this same unguarded function, so a user with edit rights on any trip they own can silently corrupt another trip's reservation-day ordering by supplying that victim trip's reservation_id/day_id. Verified live end-to-end with two real accounts against a local instance (below).

Details

Tested at commit e60427f813dc35f688d5d9169b79ac8c43974719, package version 3.4.1 (Docker image mauriceboe/trek:latest).

server/src/services/reservationService.ts (~line 363):

export function updatePositions(tripId: string | number, positions: { id: number; day_plan_position: number }[], dayId?: number | string) {
  if (dayId) {
    // Per-day positions for multi-day reservations
    const stmt = db.prepare('INSERT OR REPLACE INTO reservation_day_positions (reservation_id, day_id, position) VALUES (?, ?, ?)');
    const updateMany = db.transaction((items) => {
      for (const item of items) {
        stmt.run(item.id, dayId, item.day_plan_position);   // tripId never used
      }
    });
    updateMany(positions);
  } else {
    // Legacy: update global position
    const stmt = db.prepare('UPDATE reservations SET day_plan_position = ? WHERE id = ? AND trip_id = ?');
    ...

The dayId branch never references the tripId parameter it was handed, unlike the "legacy" branch right next to it (and unlike every comparable sort-order updater in the codebase for budget items, todo items, and packing items, which all filter WHERE id = ? AND trip_id = ?).

The reservation_day_positions schema (server/src/db/migrations.ts ~line 1220) has no tenant-scoped constraint - the foreign keys only require the IDs exist anywhere, not under a common trip_id:

CREATE TABLE IF NOT EXISTS reservation_day_positions (
  reservation_id INTEGER NOT NULL REFERENCES reservations(id) ON DELETE CASCADE,
  day_id INTEGER NOT NULL REFERENCES days(id) ON DELETE CASCADE,
  position REAL NOT NULL,
  PRIMARY KEY (reservation_id, day_id)
);

So a reservation_id belonging to a victim's trip plus any valid day_id (even from a third, unrelated trip) satisfies both FKs; the database layer does not block the cross-trip write.

REST entry point, server/src/nest/reservations/reservations.controller.ts (~line 81):

@Put('positions')
updatePositions(@CurrentUser() user, @Param('tripId') tripId, @Body() body, ...) {
  const trip = this.requireTrip(tripId, user);   // only checks access to :tripId
  this.requireEdit(trip, user);                  // only checks edit permission on :tripId
  ...
  this.reservations.updatePositions(tripId, body.positions, body.day_id);  // positions[].id, day_id never validated

requireTrip/requireEdit only validate the caller's membership on the tripId in the URL - never that body.positions[].id or body.day_id actually belong to that trip.

MCP entry point, server/src/mcp/tools/reservations.ts (~line 160), tool reorder_reservations:

async ({ tripId, positions, dayId }) => {
  if (!canAccessTrip(tripId, userId)) return noAccess();
  if (!hasTripPermission('reservation_edit', tripId, userId)) return permissionDenied();
  updateReservationPositions(tripId, positions, dayId);   // no ownership check on dayId or positions[].id

This is the outlier among its sibling tools in the same file - create_reservation, update_reservation, and link_hotel_accommodation all explicitly validate cross-references (getDay(day_id, tripId), placeExists(place_id, tripId), etc.) before writing; reorder_reservations skips that entirely.

The read path is correctly scoped (listReservations, same file, ~line 159): it joins reservation_day_positions to reservations filtered by WHERE r.trip_id = ?, so a maliciously-inserted row keyed by a victim's reservation_id becomes visible/live inside the victim's own trip view once written - this corrupts real itinerary ordering, it isn't an orphan row (confirmed below).

Reproduction steps

Run TREK locally (docker compose up, image mauriceboe/trek:latest, version 3.4.1).

1. Register two independent users:
3. POST /api/auth/register {"username":"trek_a","email":"trek_a@test.local","password":"...","name":"Trek A"}
4. POST /api/auth/register {"username":"trek_b","email":"trek_b@test.local","password":"...","name":"Trek B"}
6. 2. Log in as each and capture their bearer tokens via POST /api/auth/login.
7. 3. As trek_a (attacker), create a trip: POST /api/trips {"title":"Attacker Trip"} -> trip id 1.
8. 4. As trek_b (victim), create a trip: POST /api/trips {"title":"Victim Trip"} -> trip id 2, with days auto-created (ids 8-14).
9. 5. As trek_b, create a reservation on their own trip: POST /api/trips/2/reservations {"title":"Secret Client Meeting","type":"other","day_id":8} -> reservation id 1.
10. 6. Confirm baseline: GET /api/trips/2/reservations as trek_b shows reservation 1 with "day_plan_position":null and no day_positions.
11. 7. Exploit, as trek_a, targeting their own trip 1 but supplying trek_b's reservation id and day id:
13. PUT /api/trips/1/reservations/positions
14. Authorization: Bearer <trek_a's token>
15. Content-Type: application/json
    { "positions": [{ "id": 1, "day_plan_position": 999 }], "day_id": 8 }

Response: `{"success":true}` (HTTP 200).
8. As `trek_b`, re-fetch `GET /api/trips/2/reservations`. The reservation now shows `"day_positions":{"8":999}` - the value `trek_a` injected via a request scoped to a trip `trek_a` owns, with zero relationship to `trek_b` or trip `2`.
9. 9. **Enumeration oracle**: repeating step 7 with a nonexistent `positions[].id` (e.g. `999999`) returns HTTP 500 `{"error":"Internal server error"}` instead of `{"success":true}`, letting an attacker distinguish valid from invalid reservation/day IDs system-wide without any relationship to their owners.
The same request shape reproduces via the `reorder_reservations` MCP tool: `{tripId: 1, positions: [{id: 1, day_plan_position: 999}], dayId: 8}`.

### Impact

Any authenticated user can corrupt another trip's reservation ordering data without any relationship to that trip, and can use the endpoint's differential response to enumerate valid reservation/day IDs across the whole instance. This breaks tenant isolation for a write operation exposed on two independent surfaces (REST and MCP). CVSS v3.1 Base Score estimate: **5.4 (Medium)**, `CVSS:3.1/AV:N/AC:L/PR:L/UI:N/S:U/C:N/I:L/A:N`.

### Recommended fix

Scope the write exactly like the legacy branch and every other sort-order updater in the codebase - verify both IDs belong to `tripId` before touching the table, e.g.:
```ts
const dayOk = db.prepare('SELECT 1 FROM days WHERE id = ? AND trip_id = ?').get(dayId, tripId);
if (!dayOk) throw new Error('day_id does not belong to this trip');
const stmt = db.prepare(`
  INSERT OR REPLACE INTO reservation_day_positions (reservation_id, day_id, position)
  SELECT ?, ?, ? WHERE EXISTS (SELECT 1 FROM reservations WHERE id = ? AND trip_id = ?)
`);
for (const item of positions) {
  stmt.run(item.id, dayId, item.day_plan_position, item.id, tripId);
}

Apply the same ownership check inside the reorder_reservations MCP tool, matching the pattern its sibling tools already use in the same file (or simply have the MCP tool call the fixed service function).

Happy to verify a fix against my local instance. Thanks for maintaining this project.

Maintainer note (2026-08-16)

Confirmed and fixed. The per-day write derives the trip from a join now, so a foreign reservation or day id produces no row. That also removes the 500-vs-200 difference you flagged: a stale id is a no-op returning 200. reorder_reservations goes through the same path, and a migration clears rows already stored whose reservation and day disagree.

Fixed in 4.0.0. Thanks for the report and the clear reproduction.