fix(reservations): answer an id that resolves to nothing, instead of crashing

The scoping guard asked whether a referenced id belongs to a different trip.
An id that belongs to no row at all cannot, so it passed, reached the raw
INSERT/UPDATE, and SQLite's own foreign-key enforcement threw. Nothing caught
it, so a stale place_id from a client-side race came back as a bare 500 with no
actionable message — where the same route answers a clean 400 for an id that
does exist somewhere else.

A second, disjoint guard now names the ids that resolve to nothing and answers
400. It deliberately does not cover accommodation_id: that column has no row to
find once a shortened trip cascades its stay away, and refusing the write there
would make the booking permanently uneditable.

The controller guard reads the request body, which is not enough on its own —
an update rebinds whatever the row already held, so a stored id whose row is
gone reaches SQL untouched. day_id, end_day_id, place_id and assignment_id are
all declared ON DELETE SET NULL, so a reference that resolves to nothing is
precisely the state that cascade leaves behind and is now cleared to NULL, the
same way a dangling accommodation_id already heals. day_accommodations' day
columns are NOT NULL and have nothing to heal to, so those are refused.

The plugin surface ran neither guard. It now refuses an unresolvable id, which
turns a crash into an error; it still accepts an id from another trip, because
taking that away belongs to the plugin contract and not to a bug fix. The
reservation MCP tools already validated every referenced id against the trip.

Closes #2355

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

@@ -172,11 +172,22 @@ export class ReservationsController {
    * 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.
+   *
+   * Two questions, two answers. Reaching into another trip is the older one and
+   * keeps its wording. An id that resolves to nothing at all is the other half
+   * of the same rule — it used to reach the statement and come back as the bare
+   * 500 SQLite's foreign keys produce (#2355) — and saying "not part of this
+   * trip" about an id that is part of nothing would send the caller looking in
+   * the wrong place.
    */
   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);
     }
+    const unknown = this.reservations.unresolvedReferences(tripId, body as never);
+    if (unknown.length > 0) {
+      throw new HttpException({ error: `Unknown reference: ${unknown.join(', ')}` }, 400);
+    }
   }
 }

server/src/nest/reservations/reservations.rpc.ts

@@ -38,6 +38,7 @@ export class ReservationsRpc {
     const input = parsed.data as Record<string, unknown>;
     this.requireValidEndpoints(input.endpoints);
     this.guards.requireTripEdit(tripId, actor, RESERVATION_EDIT_ACTION);
+    this.requireResolvableReferences(tripId, input);
     const { reservation, accommodationCreated } = this.reservations.create(String(tripId), input as never);
     if (accommodationCreated) this.realtime.broadcast(tripId, 'accommodation:created', {}, undefined);
     const i = input as { title?: string; type?: string; create_budget_entry?: unknown };
@@ -59,6 +60,7 @@ export class ReservationsRpc {
     this.guards.requireTripEdit(tripId, actor, RESERVATION_EDIT_ACTION);
     const current = this.reservations.getReservation(String(reservationId), String(tripId));
     if (!current) throw new ForbiddenResource(`no reservation ${reservationId} on trip ${tripId}`);
+    this.requireResolvableReferences(tripId, input);
     const { reservation, accommodationChanged } = this.reservations.update(String(reservationId), String(tripId), input as never, current as never);
     if (accommodationChanged) this.realtime.broadcast(tripId, 'accommodation:updated', {}, undefined);
     const cur = current as { title: string; type?: string };
@@ -97,6 +99,22 @@ export class ReservationsRpc {
     if (!parsed.success) throw new BadParams(`invalid endpoints: ${schemaMessage(parsed.error)}`);
   }
 
+  /**
+   * An id that resolves to nothing is a foreign-key error the plugin reads as a
+   * crash (#2355), so it is named here instead. Only that case: a plugin has
+   * always been able to point at another trip's day or place through this
+   * surface, and taking that away is a decision for the plugin contract, not a
+   * side effect of a bug fix — so the ids the REST route refuses for being
+   * somebody else's are filtered back out.
+   */
+  private requireResolvableReferences(tripId: number, input: Record<string, unknown>): void {
+    const elsewhere = new Set(this.reservations.referencesOutsideTrip(String(tripId), input as never));
+    const unknown = this.reservations
+      .unresolvedReferences(String(tripId), input as never)
+      .filter((field) => !elsewhere.has(field));
+    if (unknown.length > 0) throw new BadParams(`unknown reference: ${unknown.join(', ')}`);
+  }
+
   /** Fire-and-forget, exactly as the REST controller sends it, so it never blocks the write. */
   private notifyBooking(actingUserId: number, tripId: number, booking: string, type: string): void {
     this.reservations.notifyBookingChange(tripId, actingUserId, booking, type);

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

@@ -1,4 +1,4 @@
-import { Injectable } from '@nestjs/common';
+import { BadRequestException, Injectable } from '@nestjs/common';
 import { DatabaseService, type TripAccess } from '../database/database.service';
 import type { TrekWsPayload, TrekWsTripEventName } from '@trek/shared';
 import { RealtimeService } from '../realtime/realtime.service';
@@ -565,6 +565,104 @@ export class ReservationsService {
     return offenders;
   }
 
+  /**
+   * Name every id in the body that resolves to nothing on this trip.
+   *
+   * A second guard rather than a stricter referencesOutsideTrip: that one
+   * answers "does this id belong to someone else", and a row that exists
+   * nowhere cannot. accommodation_id depends on that answer staying no, or a
+   * booking whose stay was cascaded away could never be saved again (#522).
+   * The fields here are the ones carrying a real foreign key, where an id that
+   * resolves to nothing is not a gap but a constraint failure, and SQLite
+   * raising it reaches the caller as a bare 500 (#2355). accommodation_id is
+   * absent by design; do not complete the list.
+   *
+   * Only a truthy id is looked up. The write paths below coerce 0 and '' to
+   * NULL before they reach SQL, so naming one here would turn a body that
+   * stores a null today into a 400.
+   *
+   * Returns the offending field names, empty when the body is clean. An id
+   * that belongs to another trip is named here too — the REST controller asks
+   * the older guard first, so that case keeps its own answer.
+   */
+  unresolvedReferences(tripId: string | number, data: CreateReservationData | UpdateReservationData): string[] {
+    const offenders: string[] = [];
+    const onTrip = (table: 'days' | 'places', id: unknown) =>
+      !!this.db.get(`SELECT id FROM ${table} WHERE id = ? AND trip_id = ?`, id, tripId);
+
+    if (data.day_id && !onTrip('days', data.day_id)) offenders.push('day_id');
+    if (data.end_day_id && !onTrip('days', data.end_day_id)) offenders.push('end_day_id');
+    if (data.place_id && !onTrip('places', data.place_id)) offenders.push('place_id');
+    if (data.assignment_id) {
+      // An assignment belongs to a trip through its day, the same join the
+      // other guard walks.
+      const row = 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,
+      );
+      if (!row) offenders.push('assignment_id');
+    }
+
+    // Only a hotel booking writes the stay row, so only there do these ids
+    // reach SQL. Any other body carries them as dead weight today, and a 400
+    // on a write that currently succeeds is not what this guard is for.
+    if (data.create_accommodation && data.type === 'hotel') {
+      const acc = data.create_accommodation;
+      const errors = this.accommodations.validateAccommodationRefs(
+        tripId, acc.place_id || undefined, acc.start_day_id || undefined, acc.end_day_id || undefined,
+      );
+      for (const { field } of errors) offenders.push(`create_accommodation.${field}`);
+    }
+
+    return offenders;
+  }
+
+  /** Is there still a row behind this id? Existence only — which trip it sits
+   *  on is the guards' question, and they answer it before the write. */
+  private referenceExists(table: 'days' | 'places' | 'day_assignments', id: unknown): boolean {
+    return !!this.db.get(`SELECT id FROM ${table} WHERE id = ?`, id);
+  }
+
+  /**
+   * An id whose row is gone reads as no id at all.
+   *
+   * day_id, end_day_id, place_id and assignment_id are declared ON DELETE SET
+   * NULL, so a reference that resolves to nothing is precisely the state the
+   * cascade leaves behind, and clearing it is what the column already promises.
+   * Binding it instead is the foreign-key error that arrives as a bare 500
+   * (#2355), and an update rebinds whatever the row already held, so a guard
+   * on the body alone never reaches it.
+   */
+  private resolvedOrNull(table: 'days' | 'places' | 'day_assignments', id: number | null): number | null {
+    return id != null && this.referenceExists(table, id) ? id : null;
+  }
+
+  /**
+   * day_accommodations.start_day_id and end_day_id are NOT NULL, so there is
+   * nothing to heal an unresolvable one to and it can only be refused. The
+   * write surfaces name the field long before this; this is the floor under
+   * the importers and the plugin host, and it refuses rather than skipping so
+   * an edit is never dropped in silence.
+   *
+   * Both days are already known to be set where this is called. A place is
+   * not: the booking form writes stays that never had one.
+   *
+   * BadRequestException rather than a domain error class, because the filter
+   * already knows what to do with it: a caller that reaches this over HTTP gets
+   * the same 400 { error } the controller's own guard sends, and not the 500 an
+   * unplaceable class would collapse to (#2355). Its message survives, so the
+   * importer that logs and moves on still names the field.
+   */
+  private requireResolvableStay(acc: CreateAccommodation): void {
+    const missing: string[] = [];
+    if (acc.place_id && !this.referenceExists('places', acc.place_id)) missing.push('place_id');
+    if (!this.referenceExists('days', acc.start_day_id)) missing.push('start_day_id');
+    if (!this.referenceExists('days', acc.end_day_id)) missing.push('end_day_id');
+    if (missing.length > 0) {
+      throw new BadRequestException(`Unknown reference: ${missing.map((field) => `create_accommodation.${field}`).join(', ')}`);
+    }
+  }
+
   /** 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 } {
@@ -594,6 +692,7 @@ export class ReservationsService {
     if (type === 'hotel' && !resolvedAccommodationId && create_accommodation) {
       const { place_id: accPlaceId, start_day_id, end_day_id, check_in, check_out, confirmation: accConf } = create_accommodation;
       if (start_day_id && end_day_id) {
+        this.requireResolvableStay(create_accommodation);
         const accResult = this.db.run(
           'INSERT INTO day_accommodations (trip_id, place_id, start_day_id, end_day_id, check_in, check_out, confirmation) VALUES (?, ?, ?, ?, ?, ?, ?)',
           tripId, accPlaceId || null, start_day_id, end_day_id, check_in || null, check_out || null, accConf || confirmation_number || null
@@ -621,6 +720,11 @@ export class ReservationsService {
       resolvedEndDayId = this.resolveDayIdFromTime(tripId, reservation_end_time);
     }
 
+    resolvedDayId = this.resolvedOrNull('days', resolvedDayId);
+    resolvedEndDayId = this.resolvedOrNull('days', resolvedEndDayId);
+    const resolvedPlaceId = this.resolvedOrNull('places', place_id || null);
+    const resolvedAssignmentId = this.resolvedOrNull('day_assignments', assignment_id || null);
+
     const result = this.db.run(`
     INSERT INTO reservations (trip_id, day_id, end_day_id, place_id, assignment_id, title, reservation_time, reservation_end_time, location, confirmation_number, notes, url, status, type, accommodation_id, metadata, needs_review)
     VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
@@ -628,8 +732,8 @@ export class ReservationsService {
       tripId,
       resolvedDayId,
       resolvedEndDayId,
-      place_id || null,
-      assignment_id || null,
+      resolvedPlaceId,
+      resolvedAssignmentId,
       title,
       reservation_time || null,
       reservation_end_time || null,
@@ -740,6 +844,7 @@ export class ReservationsService {
     if (type === 'hotel' && create_accommodation) {
       const { place_id: accPlaceId, start_day_id, end_day_id, check_in, check_out, confirmation: accConf } = create_accommodation;
       if (start_day_id && end_day_id) {
+        this.requireResolvableStay(create_accommodation);
         if (resolvedAccId) {
           this.db.run(
             'UPDATE day_accommodations SET place_id = ?, start_day_id = ?, end_day_id = ?, check_in = ?, check_out = ?, confirmation = ? WHERE id = ?',
@@ -804,6 +909,11 @@ export class ReservationsService {
       nextEndDayId = current.end_day_id ?? null;
     }
 
+    nextDayId = this.resolvedOrNull('days', nextDayId);
+    nextEndDayId = this.resolvedOrNull('days', nextEndDayId);
+    const nextPlaceId = this.resolvedOrNull('places', place_id !== undefined ? (place_id || null) : (current.place_id ?? null));
+    const nextAssignmentId = this.resolvedOrNull('day_assignments', assignment_id !== undefined ? (assignment_id || null) : (current.assignment_id ?? null));
+
     this.db.run(`
     UPDATE reservations SET
       title = COALESCE(?, title),
@@ -833,8 +943,8 @@ export class ReservationsService {
       url !== undefined ? (url || null) : (current as Reservation & { url?: string | null }).url,
       nextDayId,
       nextEndDayId,
-      place_id !== undefined ? (place_id || null) : current.place_id,
-      assignment_id !== undefined ? (assignment_id || null) : current.assignment_id,
+      nextPlaceId,
+      nextAssignmentId,
       status || null,
       type || null,
       resolvedAccId,