diff --git a/server/src/db/nul-safe-sqlite-platform.ts b/server/src/db/nul-safe-sqlite-platform.ts index 432830769..bdc080ab4 100644 --- a/server/src/db/nul-safe-sqlite-platform.ts +++ b/server/src/db/nul-safe-sqlite-platform.ts @@ -51,12 +51,48 @@ import { SqlitePlatform } from '@mikro-orm/sql'; * of which SQLite string literals treat specially) round-trips exactly as * it did before this override existed. * - * Deliberately scoped to `typeof value === 'string'`: this is the M1 fix, - * not a general re-implementation of `escape`/`quoteValue`. Non-finite - * numbers rendering as bare, unquoted tokens is the SAME inlining mechanism - * (rule 22's corollary) but is already fenced off upstream by rule 21's - * `toRowId` id guards — it is not this override's job to guess at every - * caller's numeric validation. + * Deliberately scoped to `typeof value === 'string'` for the NUL half above: + * that IS the M1 fix, not a general re-implementation of `escape`/ + * `quoteValue`. + * + * **Rule 22 extension (Plan 3c Task 9, close-out review A H1):** non-finite + * numbers hit the SAME inlining mechanism, and rule 21's `toRowId` guards do + * NOT fence every caller off from it — `toRowId` is what a CONVERTED + * service calls to validate an id BEFORE it reaches a repository, but two + * call sites (`PlacesService.verifyTripAccess`, `RealtimeGateway.handleJoin`) + * still did `Number(untrusted)` with no finite check and handed the result + * straight to `TripsRepository.findAccessible`'s raw `andWhere('t.id = ?', + * [trip_id])` bind. Traced against the installed `@mikro-orm/sql` 7.2.1 + * source: `AbstractSqlPlatform.quoteValue` (`sql/AbstractSqlPlatform.js:94`) + * special-cases only a raw fragment and a plain object (JSON-stringified); + * everything else — including every `number` — falls to `this.escape(value)`. + * `SqlitePlatform.escape` (`sql/dialects/sqlite/SqlitePlatform.js:116-133`) + * DOES special-case numbers, but not usefully: `typeof value === 'number' || + * typeof value === 'bigint'` returns `'' + value` — plain string + * concatenation, which for a non-finite value gives `'NaN'`, `'Infinity'` or + * `'-Infinity'`, an UNQUOTED bareword spliced straight into the SQL text + * (finite numbers render as an ordinary numeric literal this way and are + * fine; only the three non-finite values break). SQLite's parser reads that + * bareword as a column reference (`no such column: NaN`/`Infinity`) and + * throws, a 500 where the legacy raw bind never had the problem: + * `better-sqlite3` accepts a JS `number` directly and binds it as SQLite's + * own `REAL`/`NULL` value, never as text — verified empirically against the + * installed driver (not assumed): `db.prepare('select typeof(?)').get(NaN)` + * reports `'null'` (better-sqlite3 refuses `NaN` as a bindable REAL and + * silently binds SQL `NULL` instead), while `db.prepare('select + * typeof(?)').get(Infinity)` reports `'real'` and round-trips as the REAL + * value `Infinity` — SQLite itself has no `Infinity` literal, but its + * `REAL` parser saturates an out-of-range exponent to positive/negative + * infinity (double-precision overflow), and `9e999`/`-9e999` verified to + * produce that exact value (`select 9e999` → `Infinity`), so it is the + * literal spelling that reproduces the driver's own bound value byte-for- + * byte, not an approximation. Overriding `escape` (not `quoteValue`, which + * only special-cases a raw fragment/plain object before delegating here + * anyway) catches every number this platform ever renders, program-wide, + * the same fail-safe shape as the NUL fix above — rule 21's `toRowId` + * guards remain the PRIMARY defence (a validated id never reaches this + * branch), and this is the platform's own backstop for the callers that do + * not yet guard. */ export class NulSafeSqlitePlatform extends SqlitePlatform { override escape(value: unknown): string { @@ -64,6 +100,10 @@ export class NulSafeSqlitePlatform extends SqlitePlatform { const literals = value.split('\u0000').map(part => super.escape(part)); return `(${literals.join(' || char(0) || ')})`; } + if (typeof value === 'number' && !Number.isFinite(value)) { + if (Number.isNaN(value)) return 'NULL'; + return value > 0 ? '9e999' : '-9e999'; + } return super.escape(value); } } diff --git a/server/src/db/repositories/DayAssignments.repository.ts b/server/src/db/repositories/DayAssignments.repository.ts index 1055a6918..67bbefab3 100644 --- a/server/src/db/repositories/DayAssignments.repository.ts +++ b/server/src/db/repositories/DayAssignments.repository.ts @@ -35,22 +35,7 @@ import { TrekRepository } from './_shared/trek-repository'; * `category_icon` through the LEFT JOIN even though `Categories.name` itself * is NOT NULL. */ -export interface AssignmentWithPlaceRow { - id: number; - day_id: number; - place_id: number; - order_index: number | null; - notes: string | null; - reservation_status: string | null; - reservation_notes: string | null; - reservation_datetime: string | null; - assignment_time: string | null; - assignment_end_time: string | null; - end_day: number; - leg_transport_mode: string | null; - incoming_leg_transport_mode: string | null; - accommodation_id: number | null; - created_at: string | null; +export interface AssignmentWithPlaceRow extends DayAssignmentRow { place_name: string; place_description: string | null; lat: number | null; @@ -501,8 +486,16 @@ export class DayAssignmentsRepository extends TrekRepository { .orderBy('da.order_index', 'asc') .orderBy('da.created_at', 'asc') .orderBy('da.id', 'asc') + // `$castTo` (Task 9 fix wave, B-L1): Kysely infers `located` as its own + // `SqlBool` type from `eb.and(...)`, not the `0`/`1` integer + // `DayStopRow.located` declares (the value SQLite actually returns for + // a boolean expression) — a straight `as DayStopRow[]` is rejected as + // an unrelated-type cast, which is what forced the `as unknown as` + // bridge. `$castTo` retypes the builder itself before `execute()` + // runs, so the result is `DayStopRow[]` with no intermediate `unknown`. + .$castTo() .execute(); - return rows as unknown as DayStopRow[]; + return rows; } /** AS24 — `UPDATE day_assignments SET end_day = ? WHERE id = ?`. */ diff --git a/server/src/db/repositories/PlaceShadowPicks.repository.ts b/server/src/db/repositories/PlaceShadowPicks.repository.ts index 5831071c1..47bd15e66 100644 --- a/server/src/db/repositories/PlaceShadowPicks.repository.ts +++ b/server/src/db/repositories/PlaceShadowPicks.repository.ts @@ -73,10 +73,16 @@ export class PlaceShadowPicksRepository extends TrekRepository */ async totals(): Promise<{ total: number; oldest: string | null; newest: string | null }> { const platform = this.getEntityManager().getPlatform(); + // Task 9 fix wave (B-L5): a bare aggregate `SELECT` always returns one + // row (the docstring above), so `row` is never `undefined` — the + // previous `row?.x ?? fallback` optional-chain/nullish-coalescing pair + // was the one partial branch in `src/db/repositories/**` (83.33% + // coverage), unreachable, not a real "no rows" guard. Non-null + // asserted, matching the docstring's own claim. const row = await this.qb('p') .select([countAll(platform, 'total'), minOf(platform, 'p.created_at', 'oldest'), maxOf(platform, 'p.created_at', 'newest')]) .execute<{ total: number; oldest: string | null; newest: string | null } | undefined>('get', false); - return { total: row?.total ?? 0, oldest: row?.oldest ?? null, newest: row?.newest ?? null }; + return { total: row!.total, oldest: row!.oldest, newest: row!.newest }; } /** diff --git a/server/src/db/repositories/Places.repository.ts b/server/src/db/repositories/Places.repository.ts index 8586a1e4b..f00571c14 100644 --- a/server/src/db/repositories/Places.repository.ts +++ b/server/src/db/repositories/Places.repository.ts @@ -1,8 +1,12 @@ import type { Places } from '../entities/Places.entity'; -import type { Tags } from '../entities/Tags.entity'; import { absDifference, coalesceParam, columnRef, currentTimestamp, lowerTrim } from '../dialect/sql-functions'; import { type AssertRowKeys } from './_shared/rows'; import { TrekRepository } from './_shared/trek-repository'; +// Task 9 fix wave (B-L2): `TagRow` is Tags.repository.ts's own row type, +// byte-identical to what used to be hand-mirrored here — a second +// declaration of the same shape is exactly the duplication Sonar flags and +// the program's "single source of truth" rule (root CLAUDE.md) forbids. +import type { TagRow } from './Tags.repository'; /** A `places` row as the API emits it — every scalar column of the entity. */ export interface PlaceRow { @@ -42,17 +46,6 @@ export interface PlaceRow { const _placeRowKeys: AssertRowKeys = true; -/** A `tags` row, byte-identical to the legacy `SELECT t.* FROM tags t ...` shape. */ -export interface TagRow { - id: number; - user_id: number; - name: string; - color: string | null; - created_at: string | null; -} - -const _tagRowKeys: AssertRowKeys = true; - /** PL3 (`PlacesService.list`) — a `places` row joined with its category's flat columns. */ export interface PlaceWithCategoryRow extends PlaceRow { category_name: string | null; @@ -362,7 +355,12 @@ export class PlacesRepository extends TrekRepository { * plus the timestamp stamp every write gets. */ async updatePlace(id: number, write: { - name: string | null; + // Task 9 fix wave (B-L10): `places.name` is `NOT NULL` (`PlaceRow.name: + // string`); the caller's `(name || null) ?? existingPlace.name` fold + // always resolves to a real string before this is called — `string | + // null` here was a widened lie the type checker could never catch a + // real NULL write through. + name: string; description: string | null; lat: number | null; lng: number | null; diff --git a/server/src/nest/database/database.service.ts b/server/src/nest/database/database.service.ts index 460bcbab4..91d248c78 100644 --- a/server/src/nest/database/database.service.ts +++ b/server/src/nest/database/database.service.ts @@ -93,21 +93,6 @@ export class DatabaseService { return await this.entityManager().getRepository(Trips).isOwner(tripId, userId); } - /** - * PL28 (`PlacesService.exportGpx`'s trip-title read, L3/task-4-review.md): - * `SELECT title FROM trips WHERE id = ?`, via `TripsRepository.getTitle`. - * The same narrow-helper shape as `canAccessTrip`/`isOwner`/`rosterUserIds` - * above — a caller outside the trips domain that needs one trip column - * without taking on a full `TripsRepository` DI dependency (`PlacesModule` - * would need its own `@InjectRepository(Trips)` constructor param, which - * ripples through every `new PlacesService(...)` test-helper call site; - * this stays a one-line method here instead, matching the precedent the - * three siblings above already set). - */ - async getTripTitle(tripId: number | string): Promise { - return await this.entityManager().getRepository(Trips).getTitle(tripId); - } - /** * The user ids a trip may refer to: its members plus the owner. Guests count — * a guest is a credential-less users row joined into trip_members, and #1362 diff --git a/server/src/nest/days/days.service.ts b/server/src/nest/days/days.service.ts index 1d349268a..9625418bf 100644 --- a/server/src/nest/days/days.service.ts +++ b/server/src/nest/days/days.service.ts @@ -72,12 +72,28 @@ export class DayReorderError extends Error {} * is only ever reached downstream of a successful `getDay` call in the same * request (REST/MCP/RPC all check-then-act), so `toRowId(id)!` there is not * a live 404 path, just the same non-null-assert style this file already - * used before conversion (`this.db.get(...)!`). `tripId` is the DY0 - * seam: `TripAccessGuard` already validated it with the SAME raw-bind - * escape hatch `TripsRepository.findAccessible` uses, so every id-shape a - * plain `Number(tripId)` would mis-parse relative to SQLite's affinity was - * already refused before this service's handler ever runs — safe to convert - * with a bare `Number()` downstream of that guard. + * used before conversion (`this.db.get(...)!`). + * + * `tripId` is the DY0 seam, and the claim this docstring used to make about + * it was WRONG (Task 9 whole-plan review, H2, live): `TripAccessGuard` + * validates `:tripId` with a bare `Number(tripId)` — a `Number()` seam, not + * an affinity seam — so `Number('0x12')` is `18`, a real trip's row id, and + * the guard authorises it. That is LOOSER than `toRowId`'s canonical-decimal + * check (program rule 15). Every ORM-converted entry point below used to + * re-parse with its OWN bare `Number(tripId)`, agreeing with the guard, + * while `restampReservationDates` and `assertNoInvertedAccommodation` kept + * binding the RAW ROUTE STRING on `reservations`/`day_accommodations` + * (Plan 3d survivors, an affinity seam a hex id never matches) — so one + * `PUT .../days/reorder` request could renumber a real trip's days (the + * `Number()` half agreeing with the guard) while its own + * inverted-accommodation guard silently matched zero rows (the raw-string + * half), writing a stay whose end precedes its start with no error. Every + * entry point now parses ONCE with `toRowId(tripId)` and threads that same + * value into every survivor and read; a `toRowId` miss answers the entry + * point's own legacy-shaped not-found (`list` → `{ days: [] }`, `getDay` → + * `undefined`, `reorder`/`insert`/`create` → `DayReorderError`, uncaught by + * `create`'s route into the legacy 500) rather than let the guard's wider + * `Number()` grant reach a downstream raw bind that disagrees with it. */ @Injectable() export class DaysService { @@ -174,7 +190,13 @@ export class DaysService { // ------------------------------------------------------------------------- async list(tripId: string | number) { - const days = await this.daysRepo.listByTrip(Number(tripId)); + // Rule 21 / H2: `toRowId`, not `Number()` — see the class docstring. A + // trip id `TripAccessGuard`'s looser `Number()` authorised but this + // stricter parse refuses answers the same `{ days: [] }` an affinity-seam + // raw bind would have (no row ever matches a hex/decimal-spelled id). + const tid = toRowId(tripId); + if (tid === null) return { days: [] }; + const days = await this.daysRepo.listByTrip(tid); if (days.length === 0) { return { days: [] }; @@ -218,7 +240,13 @@ export class DaysService { } async create(tripId: string | number, date?: string, notes?: string) { - const tripIdNum = Number(tripId); + // Rule 21 / H2: `toRowId`, not `Number()`. Uncaught by the controller's + // `create` handler, so a miss becomes the legacy 500 (the raw INSERT's FK + // violation on a hex/decimal-spelled trip id) — status-level parity, not + // a manufactured miss, since `create` never had an existence gate to + // answer instead (same documented exception `PlacesService.create` uses). + const tripIdNum = toRowId(tripId); + if (tripIdNum === null) throw new Error('Trip id is not a real row.'); // DY5: `DaysRepository.maxDayNumber` already folds the no-rows case to 0 // (`?? 0`); the `|| 0` here is the legacy expression kept verbatim so a // stored 0 still becomes 1 — the same outcome either fold produces, but @@ -238,7 +266,10 @@ export class DaysService { async getDay(id: string | number, tripId: string | number) { const dayId = toRowId(id); if (dayId === null) return undefined; - return await this.daysRepo.findInTrip(dayId, Number(tripId)); + // Rule 21 / H2: `toRowId`, not `Number()` — see the class docstring. + const tid = toRowId(tripId); + if (tid === null) return undefined; + return await this.daysRepo.findInTrip(dayId, tid); } async update(id: string | number, current: { notes?: string | null; title?: string | null }, fields: { notes?: string; title?: string | null }) { @@ -296,9 +327,16 @@ export class DaysService { * DY14–DY18 — Plan 3d (`reservations`/`reservation_endpoints`): stays raw on * `DatabaseService` inside the caller's `uow.transactional` block, per the * Plan 3b `UserCleanupService` precedent for cross-domain writes. + * + * `tripId: number` (Task 9 fix wave, H2): every caller now passes the ONE + * `toRowId`-parsed value its own entry point already computed, never the + * raw route string — binding the raw string here (an affinity seam) is + * exactly the mismatch that let a hex-spelled trip id's reorder skip this + * statement entirely while the `Number()`-parsed half of the same request + * acted on a real trip (see the class docstring). */ async restampReservationDates( - tripId: string | number, + tripId: number, oldDateById: Map, newDateById: Map, ): Promise { @@ -352,8 +390,12 @@ export class DaysService { * (joined to `days` only to read `day_number`), so it stays raw on * `DatabaseService` per the inventory's ruling, even though `days` itself * is owned here. + * + * `tripId: number` (Task 9 fix wave, H2): the same parsed-once value every + * other survivor in this class now takes — see `restampReservationDates`'s + * docstring for why binding the raw route string here was the live bug. */ - private async assertNoInvertedAccommodation(tripId: string | number): Promise { + private async assertNoInvertedAccommodation(tripId: number): Promise { const spans = this.db.all<{ id: number; start_no: number; end_no: number }>(` SELECT a.id, s.day_number AS start_no, e.day_number AS end_no FROM day_accommodations a @@ -385,12 +427,16 @@ export class DaysService { * converts via `DayAssignmentsRepository.reanchorToDay`, a Kysely * statement proven to join this method's ambient transaction by rollback * (see the task report). + * + * `tripId: number` (Task 9 fix wave, H2): callers now pass their own + * `toRowId`-parsed value; DY20's raw bind uses it too (previously the raw + * route string — the same affinity-seam mismatch documented on + * `restampReservationDates`). */ async resyncAccommodationDays( - tripId: string | number, + tripId: number, prevDateByDayId: Map, ): Promise { - const tripIdNum = Number(tripId); // DY20 — Plan 3d const stays = this.db.all<{ id: number; start_day_id: number; end_day_id: number }>( 'SELECT id, start_day_id, end_day_id FROM day_accommodations WHERE trip_id = ?', @@ -413,8 +459,8 @@ export class DaysService { const oldEndDate = prevDateByDayId.get(stay.end_day_id); if (oldStartDate && oldEndDate) { // DY21 - const newStart = await this.daysRepo.findByTripAndDate(tripIdNum, oldStartDate); - const newEnd = await this.daysRepo.findByTripAndDate(tripIdNum, oldEndDate); + const newStart = await this.daysRepo.findByTripAndDate(tripId, oldStartDate); + const newEnd = await this.daysRepo.findByTripAndDate(tripId, oldEndDate); if (newStart && newEnd && newStart.day_number <= newEnd.day_number && (newStart.id !== stay.start_day_id || newEnd.id !== stay.end_day_id)) { updateStay.run(newStart.id, newEnd.id, stay.id); @@ -441,7 +487,14 @@ export class DaysService { * day ids (a permutation of the current ids). */ async reorder(tripId: string | number, orderedIds: number[]) { - const tripIdNum = Number(tripId); + // Rule 21 / H2: `toRowId`, not `Number()` — see the class docstring. A + // miss throws the SAME permutation error an empty `rows` set would + // (nothing ever matches a hex/decimal-spelled id), so the controller's + // existing `DayReorderError` → 400 catch answers it, before any write — + // never the "renumber a real trip, skip the invariant check" split this + // fixes. + const tripIdNum = toRowId(tripId); + if (tripIdNum === null) throw new DayReorderError('orderedIds must be a permutation of the trip day ids.'); const rows: DayOrderRow[] = await this.daysRepo.listOrderedForReorder(tripIdNum); const existingIds = new Set(rows.map(r => r.id)); @@ -468,11 +521,11 @@ export class DaysService { newDateById.set(id, date); } - if (isDated) await this.restampReservationDates(tripId, oldDateById, newDateById); - await this.assertNoInvertedAccommodation(tripId); + if (isDated) await this.restampReservationDates(tripIdNum, oldDateById, newDateById); + await this.assertNoInvertedAccommodation(tripIdNum); }); - return await this.list(tripId); + return await this.list(tripIdNum); } /** @@ -482,7 +535,15 @@ export class DaysService { * shifted days have their dates re-stamped (same rules as reorder). */ async insert(tripId: string | number, position?: number) { - const tripIdNum = Number(tripId); + // Rule 21 / H2: `toRowId`, not `Number()` — see `reorder`'s comment above + // and the class docstring. `insert` shares `create`'s controller route + // (no `DayReorderError` catch on that path — `reorder` is the only + // handler that has one), so a plain `Error` here becomes the SAME + // legacy-shaped 500 `create`'s own miss does (the FK violation on the + // raw route string this never executes), rather than the renumber-then- + // skip-the-invariant split H2 found. + const tripIdNum = toRowId(tripId); + if (tripIdNum === null) throw new Error('Trip id is not a real row.'); const rows: DayOrderRow[] = await this.daysRepo.listOrderedForReorder(tripIdNum); const n = rows.length; const pos = Math.min(Math.max(position ?? n + 1, 1), n + 1); @@ -518,8 +579,8 @@ export class DaysService { newDateById.set(id, dates[i]); } - await this.restampReservationDates(tripId, oldDateById, newDateById); - await this.assertNoInvertedAccommodation(tripId); + await this.restampReservationDates(tripIdNum, oldDateById, newDateById); + await this.assertNoInvertedAccommodation(tripIdNum); // DY35 — `TripsRepository.setEndDate`, added this task for Task 7 to reuse. await this.tripsRepo.setEndDate(tripIdNum, dates[dates.length - 1]); diff --git a/server/src/nest/places/places.service.ts b/server/src/nest/places/places.service.ts index 3d73c1351..be6dfb8a6 100644 --- a/server/src/nest/places/places.service.ts +++ b/server/src/nest/places/places.service.ts @@ -39,12 +39,14 @@ import { PlaceRatings } from '../../db/entities/PlaceRatings.entity'; import { TripMembers } from '../../db/entities/TripMembers.entity'; import { DayAssignments } from '../../db/entities/DayAssignments.entity'; import { Categories } from '../../db/entities/Categories.entity'; +import { Trips } from '../../db/entities/Trips.entity'; import type { PlacesRepository, PlaceWithCategoryRow } from '../../db/repositories/Places.repository'; import type { TagsRepository } from '../../db/repositories/Tags.repository'; import type { PlaceRatingsRepository } from '../../db/repositories/PlaceRatings.repository'; import type { TripMembersRepository } from '../../db/repositories/TripMembers.repository'; import type { DayAssignmentsRepository } from '../../db/repositories/DayAssignments.repository'; import type { CategoriesRepository } from '../../db/repositories/Categories.repository'; +import type { TripsRepository } from '../../db/repositories/Trips.repository'; /** Rows a place delete took down with the nights booked there, for the caller to announce. */ export interface CancelledStays { @@ -155,10 +157,33 @@ export class PlacesService { @InjectRepository(TripMembers) private readonly tripMembersRepo: TripMembersRepository, @InjectRepository(DayAssignments) private readonly dayAssignmentsRepo: DayAssignmentsRepository, @InjectRepository(Categories) private readonly categoriesRepo: CategoriesRepository, + // Task 9 fix wave (B-M2 / A-L3 ruling): `DatabaseService.getTripTitle` + // does not stay — it was the one method on that class this domain's own + // PL28 comment said should convert like its siblings once concurrent + // implementers stopped colliding on this file. `Trips` is already on + // `PlacesModule`'s `forFeature` list, so this needs no module change. + @InjectRepository(Trips) private readonly tripsRepo: TripsRepository, ) {} - async verifyTripAccess(tripId: string, userId: number) { - return await this.dbs.canAccessTrip(Number(tripId), userId); + /** + * The `requireTrip` gate for 13 of the 16 routes on this controller (the + * other 3 use `TripAccessGuard` directly). `toRowId`, not `Number()` — + * Plan 3c Task 9's whole-plan review (A-H1/M1, B-H1): `Number('1.0')`/ + * `Number('1 ')`/`Number('0x1abc's NaN half)` either authorised an id the + * writes behind this gate (`toRowId(tripId) ?? -1`) then refused, or sent + * a bare `NaN` into `TripsRepository.findAccessible`'s raw bind and 500'd + * (fixed program-wide at the platform too — see `NulSafeSqlitePlatform`). + * Parsing ONCE, here, and returning the parsed `tid` for every downstream + * call to reuse (never re-parsing) closes both: a non-canonical trip id + * now answers this gate's own 404 before any read or write runs, and every + * write behind it shares the exact id the gate authorised. + */ + async verifyTripAccess(tripId: string, userId: number): Promise<(TripAccess & { tid: number }) | undefined> { + const tid = toRowId(tripId); + if (tid === null) return undefined; + const access = await this.dbs.canAccessTrip(tid, userId); + if (!access) return undefined; + return { ...access, tid }; } async canEdit(trip: Trip, user: User): Promise { @@ -275,19 +300,18 @@ export class PlacesService { transport_mode, route_geometry, route_color, stop_type, fill_percent, tags = [], } = body; - // Rule 21 (H1): one trip-id notion, parsed once, used by every write - // below — including PL5's `tagsOnTrip` roster read. Every other gate in - // this class answers the legacy not-found when the trip id doesn't parse - // canonically; create() has no existence gate to answer instead — the - // legacy raw bind (base 94c6efbbc) stored the non-canonical spelling - // straight into the `trip_id` column and let the FK constraint fail - // closed (verified live: a hex trip id 500s on create, where every other - // route 404s). `toRowId` rejects the same non-canonical spellings that - // never reach a real row; `?? -1` substitutes a trip id that can never - // exist (ids are autoincrement from 1), so the FK constraint fails the - // same way — without widening `insertPlace`'s typed `trip_id: number` - // column to accept the raw string. - const tid = toRowId(tripId) ?? -1; + // Rule 21 / M1 (Task 9 fix wave): `verifyTripAccess` now parses `tripId` + // with this SAME `toRowId` and answers 404 `Trip not found` before + // `create()` is ever reached (the controller's `requireTrip`/MCP's own + // `tripsRepo.findAccessible` gate both run first) — so `toRowId` here + // can never miss for a caller that went through the gate. The previous + // `?? -1` sentinel was ruled a defect, not parity: with the gate itself + // loosely `Number()`-parsed, a non-canonical-but-numeric id like `1.0` + // could pass the gate and still manufacture a fresh 500 here (an FK + // failure the legacy raw bind never produced for that input). Non-null + // asserted, not defaulted — a null here now means a caller skipped the + // gate, a bug to surface, not a trip id to silently coerce to -1. + const tid = toRowId(tripId)!; // PL4 — the 25-column INSERT. lat/lng/price/duration_minutes/fill_percent // use an explicit undefined check, not `||`: 0 is a legitimate value for @@ -584,11 +608,17 @@ export class PlacesService { const cancelled = noCancelledStays(); if (ids.length === 0) return { deleted: [], cancelled }; // Rule 21 (H1): the trip id gets the same `toRowId` treatment as - // `remove()`'s, parsed once and reused for every id in the loop below — - // a non-canonical trip id never matches a real place's row (the PL20 - // gate would answer "not found" for every id anyway), so this returns - // early with nothing deleted, the same shape the legacy raw bind - // produced when the trip id didn't affinity-match any row. + // `remove()`'s, parsed once and reused for every id in the loop below. + // Task 9 fix wave (A-8): the claim this comment used to make — "the + // legacy raw bind produced the same shape, because the trip id didn't + // affinity-match any row" — was WRONG for `1.0`/`1 ` (and other + // SQLite-affinity-recognised spellings): the legacy statement DID match + // those against a real trip, deleting for real. `toRowId`'s narrower + // canonical-decimal check answers "nothing deleted" for those inputs + // too, which is the deliberate rule-15 narrowing (not parity) — and, as + // of this fix wave, moot in practice: `verifyTripAccess` already 404s + // any non-canonical trip id before `removeMany` is ever reached, so this + // `tid === null` branch only fires for a caller that skipped the gate. const tid = toRowId(tripId); if (tid === null) return { deleted: [], cancelled }; const deleted: number[] = []; @@ -769,17 +799,14 @@ export class PlacesService { */ async exportGpx(tripId: string, opts: GpxExportOptions = {}): Promise<{ gpx: string; filename: string } | null> { // PL28 — `SELECT title FROM trips WHERE id = ?`, via - // `DatabaseService.getTripTitle` (Task 4 review L3: this comment - // previously said the repoint was blocked on `Trips.repository.ts` - // landing `getTitle`, which it did at `94c6efbbc` — Task 4's own parent - // commit — so the block was stale from the moment this task started). - // `getTripTitle` is a narrow `DatabaseService` helper, the same shape as - // `canAccessTrip`/`isOwner`/`rosterUserIds`: `trips` stays outside this - // domain's owned repositories, and a full `TripsRepository` - // `@InjectRepository` constructor param on `PlacesService` would ripple - // through every `new PlacesService(...)` test-helper call site for one - // read. The legacy `if (!trip) return null` maps onto `title === null`. - const title = await this.dbs.getTripTitle(tripId); + // `TripsRepository.getTitle` directly (Task 9 fix wave, B-M2 / A-L3 + // ruling): `DatabaseService.getTripTitle` was a narrow, single-caller + // delegation kept only because concurrent Task 4/7/8 implementers were + // all editing this file's constructor at once — that risk is gone, so + // it and its `entityManager()` mention are deleted, and this domain + // injects `TripsRepository` directly like its other five repositories. + // The legacy `if (!trip) return null` maps onto `title === null`. + const title = await this.tripsRepo.getTitle(tripId); if (title === null) return null; // PL29 — the waypoint projection. @@ -875,14 +902,13 @@ export class PlacesService { if (waypoints.length === 0) return null; - // Task 5 review L5 (rule 21): parsed once here, reused for the one write - // below. `buildDedupSet` keeps the raw-bind seam (L6 ruling, PL24) — - // this does not touch it. A non-canonical trip id (this route has no - // existence gate to answer "not found" instead, matching `create`'s own - // H1 fix) substitutes a trip id that can never exist, so `insertPlace` - // fails its FK constraint the same way the legacy raw bind did (base: - // 500 on a hex trip id, live-verified in the Task 5 review). - const tid = toRowId(tripId) ?? -1; + // Rule 21 / M1 (Task 9 fix wave): non-null asserted, not `?? -1` — the + // controller's `requireTrip` (`verifyTripAccess`) already parsed and + // gated this SAME `tripId` with `toRowId` before `importGpx` was ever + // called, so a miss here can only mean a caller skipped the gate (see + // `create()`'s comment for the full ruling). `buildDedupSet` keeps the + // raw-bind seam (L6 ruling, PL24) — this does not touch it. + const tid = toRowId(tripId)!; const dedup = await this.buildDedupSet(tripId); const created: PlaceWithTags[] = []; let skipped = 0; @@ -985,8 +1011,9 @@ export class PlacesService { // PL33 — `CategoriesRepository.listIdName` (Plan 3a's repository). const categories = await this.categoriesRepo.listIdName(); const categoryLookup = buildCategoryNameLookup(categories); - // Task 5 review L5 (rule 21) — same fix and reasoning as `importGpxRows`. - const tid = toRowId(tripId) ?? -1; + // Rule 21 / M1 (Task 9 fix wave) — same ruling as `importGpxRows`: the + // caller's `requireTrip` gate already `toRowId`-parsed this `tripId`. + const tid = toRowId(tripId)!; const dedup = await this.buildDedupSet(tripId); const created: PlaceWithTags[] = []; let dupCount = 0; @@ -1265,8 +1292,9 @@ export class PlacesService { tripId: string, places: { name: string; lat: number; lng: number; notes: string | null; googleFtid: string | null }[], ): Promise<{ created: PlaceWithTags[]; skipped: number }> { - // Task 5 review L5 (rule 21) — same fix and reasoning as `importGpxRows`. - const tid = toRowId(tripId) ?? -1; + // Rule 21 / M1 (Task 9 fix wave) — same ruling as `importGpxRows`: the + // caller's `requireTrip` gate already `toRowId`-parsed this `tripId`. + const tid = toRowId(tripId)!; const dedup = await this.buildDedupSet(tripId); const created: PlaceWithTags[] = []; let skipped = 0; @@ -1546,8 +1574,9 @@ export class PlacesService { return { error: 'No places with coordinates found in list', status: 400 }; } - // Task 5 review L5 (rule 21) — same fix and reasoning as `importGpxRows`. - const tid = toRowId(tripId) ?? -1; + // Rule 21 / M1 (Task 9 fix wave) — same ruling as `importGpxRows`: the + // caller's `requireTrip` gate already `toRowId`-parsed this `tripId`. + const tid = toRowId(tripId)!; const dedup = await this.buildDedupSet(tripId); const created: PlaceWithTags[] = []; let skipped = 0; @@ -1626,18 +1655,15 @@ export class PlacesService { if (place.google_place_id) return; if (typeof place.lat !== 'number' || typeof place.lng !== 'number') return; - // Task 5 review L5 (rule 21): parsed once, reused by both `fillIfEmpty` - // calls below. A non-canonical trip id (never a real client spelling — - // this whole path only ever runs right after this service's own import, - // which now also parses `tripId` once) substitutes a trip id that can - // never exist, so `fillIfEmpty`'s `WHERE id = ? AND trip_id = ?` matches - // no row — a silent no-op, matching this method's own "never throws" - // contract, not a 500 the caller (a detached background task) could not - // observe anyway. Deliberately NOT applied to `this.realtime.broadcast` - // below: every broadcast call in this domain, controller included, keys - // the room by the raw route string, and changing this one call's room - // shape alone would desync it from what the client actually joined. - const tid = toRowId(tripId) ?? -1; + // Rule 21 / M1 (Task 9 fix wave): non-null asserted — this whole path + // only ever runs right after this service's own import, downstream of + // the controller's `requireTrip` gate, which already `toRowId`-parsed + // this same `tripId` once. Deliberately NOT applied to + // `this.realtime.broadcast` below: every broadcast call in this domain, + // controller included, keys the room by the raw route string, and + // changing this one call's room shape alone would desync it from what + // the client actually joined. + const tid = toRowId(tripId)!; // Asked for a Google identity rather than for the best answer: the whole // point here is the `google_place_id` that `pickEnrichmentMatch` selects on, @@ -1743,8 +1769,8 @@ export class PlacesService { console.warn(`[Places] address backfill skipped for trip ${tripId}: ${pending.length} places exceeds the ${ADDRESS_BACKFILL_MAX_PLACES} cap`); return; } - // Task 5 review L5 (rule 21) — same fix and reasoning as `enrichOne`. - const tid = toRowId(tripId) ?? -1; + // Rule 21 / M1 (Task 9 fix wave) — same ruling as `enrichOne`. + const tid = toRowId(tripId)!; // Serial on purpose: the background lane throttles to roughly one request a // second anyway, so concurrency would only build a queue. for (const place of pending) { diff --git a/server/src/nest/realtime/realtime.gateway.ts b/server/src/nest/realtime/realtime.gateway.ts index 8e215f44f..21e087e66 100644 --- a/server/src/nest/realtime/realtime.gateway.ts +++ b/server/src/nest/realtime/realtime.gateway.ts @@ -169,8 +169,16 @@ export class RealtimeGateway const user = userOf(socket); if (!user || !message?.tripId) return undefined; + // Rule 22 / A-H1 (Task 9 fix wave): `Number.isFinite` first, the same + // guard `handleBookJoin` below already carries — without it, a + // non-numeric `tripId` (`'abc'`) becomes `NaN`, which used to reach + // `TripsRepository.findAccessible`'s raw bind as the unquoted bareword + // `NaN` and throw (`no such column: NaN`), leaving this handler's + // promise unanswered instead of the legacy `Access denied` frame. The + // platform now renders a bound `NaN` as `NULL` too (`NulSafeSqlitePlatform`), + // so this guard is defence in depth, not the only fix. const tripId = Number(message.tripId); - if (!(await this.db.canAccessTrip(tripId, user.id))) { + if (!Number.isFinite(tripId) || !(await this.db.canAccessTrip(tripId, user.id))) { return { type: 'error', message: 'Access denied' }; } joinRoom(socket, tripId); diff --git a/server/src/nest/trip-members/trip-members.service.ts b/server/src/nest/trip-members/trip-members.service.ts index 0a1c904d6..513a5eb83 100644 --- a/server/src/nest/trip-members/trip-members.service.ts +++ b/server/src/nest/trip-members/trip-members.service.ts @@ -152,9 +152,12 @@ export class TripMembersService { // `Number(tripId)`, not the raw string: `addMember`'s INSERT needs the // real `Primary` type (its docstring). Safe here — `tripId` has - // already matched a real trip through the inline `canAccessTrip` check - // above this method's controller call, which only succeeds for a string - // SQLite's own affinity rules already recognise as numeric. + // already matched a real trip through the controller's `TripAccessGuard`, + // whose own `Number(tripId)` is the seam (Task 9 fix wave, A-L1: this + // comment previously and wrongly called it a raw-bind/SQLite-affinity + // check — a `Number()` seam is WIDER, since `Number('0x10')`/ + // `Number('1e1')` coerce to a real integer where SQLite's own text + // affinity never converts a hex or exponent literal). await this.tripMembersRepo.addMember(Number(tripId), target.id, invitedByUserId); const tripTitle = await this.tripsRepo.getTitle(tripId); @@ -196,16 +199,23 @@ export class TripMembersService { const fromEmail = (await this.usersRepo.getEmail(currentOwnerId)) || ''; - // `Number(tripId)` at the two writes below that need the real - // `Primary` type (`setOwner`'s docstring / `addIgnoringConflict`'s - // docstring) — safe: this route runs behind `TripOwnerGuard`, so `tripId` - // has already matched a real trip through its raw-bind `isOwner` check. + // Task 9 fix wave (B-L4 / A-L1): all THREE writes below now take + // `trip.id` — the real `number` `findIdTitleOwner` already resolved + // above, not the route's raw `tripId` string. The comment this replaces + // claimed "`Number(tripId)` at the two writes below", but only + // `addIgnoringConflict` (which requires a real `Primary`) ever + // did that; `setOwner`/`remove` kept binding the unconverted string — + // not a live divergence (both repository methods document and test + // that raw-bind seam for their OTHER callers, so their signatures stay + // `number | string`), but the letter of rule 21 says one id, parsed + // once, threaded everywhere — and `trip.id` is already sitting right + // here, already proven to be the real row. await this.uow.transactional(async () => { - await this.tripsRepo.setOwner(tripId, newOwnerId); + await this.tripsRepo.setOwner(trip.id, newOwnerId); // The new owner is no longer a plain member… - await this.tripMembersRepo.remove(tripId, newOwnerId); + await this.tripMembersRepo.remove(trip.id, newOwnerId); // …and the former owner keeps access as a member. - await this.tripMembersRepo.addIgnoringConflict(Number(tripId), currentOwnerId, newOwnerId); + await this.tripMembersRepo.addIgnoringConflict(trip.id, currentOwnerId, newOwnerId); }); return { tripTitle: trip.title, fromEmail, toEmail: newOwner.email }; @@ -227,8 +237,12 @@ export class TripMembersService { const username = `guest-${randomUUID()}`; // `Number(tripId)` for the same reason `addMember`'s docstring gives: - // `createGuest`'s route runs behind `TripOwnerGuard`, so `tripId` has - // already matched a real trip through its raw-bind `isOwner` check. + // `createGuest`'s route runs behind `TripOwnerGuard`, whose OWN + // `Number(tripId)` is the seam that already matched a real trip (Task 9 + // fix wave, A-L1: corrected from "raw-bind `isOwner` check" — the guard + // converts with `Number()` first, a WIDER seam than a raw bind, so a + // hex-spelled trip id is authorised here too — L1's accepted widening: + // `POST .../guests` with a hex id now creates a real guest, base 500). const guestId = await this.uow.transactional(async () => { const newGuestId = await this.usersRepo.insertGuest({ username, email, display_name: display }); await this.tripMembersRepo.addMember(Number(tripId), newGuestId, invitedByUserId); diff --git a/server/src/nest/trips/trips.service.ts b/server/src/nest/trips/trips.service.ts index 30fb3f71b..18e008039 100644 --- a/server/src/nest/trips/trips.service.ts +++ b/server/src/nest/trips/trips.service.ts @@ -245,6 +245,19 @@ export class TripsService { const existing = await this.daysRepo.listOrderedForReorder(trip_id); // TP1 // Two-phase renumber to avoid UNIQUE(trip_id, day_number) collisions. + // + // Task 9 fix wave (A-L4a): the FIRST (negative) pass is not pinned by + // any test — a mutation that removes or skips it survives the suite, + // because no test's fixture forces two days to swap `day_number`s (the + // only scenario where skipping straight to the positive pass would hit + // a live UNIQUE collision on a still-occupied target number). It is + // still correct, production-necessary code, not dead code: this + // helper's callers (`generateDays`'s regenerate/compact paths) reorder + // day rows whose numbers can genuinely collide mid-pass, and the + // negative pass is what clears every target number before any of them + // is reused. Documented per the review's ruling rather than added a + // test, since the equivalent-mutation gap is the ABSENCE of a + // swap-shaped fixture, not a defect in this code. const renumber = async (days: { id: number }[]) => { for (let i = 0; i < days.length; i++) await this.daysRepo.setDayNumber(days[i].id, -(i + 1)); // TP2 for (let i = 0; i < days.length; i++) await this.daysRepo.setDayNumber(days[i].id, i + 1); // TP2 @@ -462,13 +475,16 @@ export class TripsService { // so re-stamp reservation_time to follow — same rules as reorderDays/insertDay. const newDays = await this.daysRepo.listOrderedForReorder(tripIdNum); // TP27 const newDateByDayId = new Map(newDays.map(d => [d.id, d.date])); - await this.days.restampReservationDates(tripId, prevDateByDayId, newDateByDayId); + // `tripIdNum`, not `tripId` (Task 9 fix wave, H2): `restampReservationDates` + // now takes the parsed row id, the same value every other DaysService + // survivor takes — see its own docstring. + await this.days.restampReservationDates(tripIdNum, prevDateByDayId, newDateByDayId); } else { // Default: generateDays re-dates day rows positionally; re-anchor dated bookings to // the day matching their absolute reservation_time, and accommodations (+ their // linked hotel reservations) to the days now holding their pre-change dates (#1288). await this.reservations.resyncReservationDays(tripId); - await this.days.resyncAccommodationDays(tripId, prevDateByDayId); + await this.days.resyncAccommodationDays(tripIdNum, prevDateByDayId); } }); } diff --git a/server/tests/e2e/reservations.e2e.test.ts b/server/tests/e2e/reservations.e2e.test.ts index e1b090ce8..0c00541aa 100644 --- a/server/tests/e2e/reservations.e2e.test.ts +++ b/server/tests/e2e/reservations.e2e.test.ts @@ -34,10 +34,13 @@ const { db } = vi.hoisted(() => { return { db: tmp }; }); -const { canAccessTrip } = vi.hoisted(() => ({ canAccessTrip: vi.fn() })); +// Task 9 fix wave (B-L11): `src/db/database.ts` no longer exports +// `canAccessTrip` (Plan 3c Task 0b moved it onto `TripsRepository` behind +// `TripAccessGuard`'s own `EntityManager`), so mocking it here was a dead +// no-op — the "404 when trip not accessible" cases below already delete/ +// restore the real row instead (see their own comments). vi.mock('../../src/db/database', () => ({ db, - canAccessTrip, getPlaceWithTags: vi.fn(), closeDb: () => {}, reinitialize: () => {}, @@ -92,7 +95,6 @@ describe('Reservations + accommodations e2e (real auth guard + temp SQLite, real }); beforeEach(() => { - canAccessTrip.mockImplementation((id: unknown) => db.prepare('SELECT * FROM trips WHERE id = ?').get(id)); checkPermission.mockReturnValue(true); notificationSend.mockClear(); }); diff --git a/server/tests/helpers/mcp-test-controllers.ts b/server/tests/helpers/mcp-test-controllers.ts index 799c9f821..1248306b2 100644 --- a/server/tests/helpers/mcp-test-controllers.ts +++ b/server/tests/helpers/mcp-test-controllers.ts @@ -224,6 +224,7 @@ export async function createMcpTestRegistry(): Promise { await createTestTripMembersRepo(dbService.connection), await createTestDayAssignmentsRepo(dbService.connection), await createTestCategoriesRepo(dbService.connection), + await createTestTripsRepo(dbService.connection), ); // Built after it: a hotel booking writes the stay's day stop through this one. const reservationsService = new ReservationsService(dbService, permissionsService, budgetService, realtimeService, notificationsStub(), new ReservationsReadRepository(dbService), accommodationsService, await createTestUnitOfWork(dbService.connection)); diff --git a/server/tests/helpers/plugin-host.ts b/server/tests/helpers/plugin-host.ts index c616e24b1..7726a16a7 100644 --- a/server/tests/helpers/plugin-host.ts +++ b/server/tests/helpers/plugin-host.ts @@ -146,6 +146,7 @@ export async function createPluginRpcHostFactory(dbs: DatabaseService): Promise< await createTestTripMembersRepo(dbs.connection), await createTestDayAssignmentsRepo(dbs.connection), await createTestCategoriesRepo(dbs.connection), + await createTestTripsRepo(dbs.connection), ); // After accommodations: a hotel booking writes the stay's day stop through it. const reservations = new ReservationsService(dbs, permissions, budget, realtime, notificationsStub(), new ReservationsReadRepository(dbs), accommodations, await createTestUnitOfWork(dbs.connection)); diff --git a/server/tests/integration/days.test.ts b/server/tests/integration/days.test.ts index 38c3ed512..18baa6e0f 100644 --- a/server/tests/integration/days.test.ts +++ b/server/tests/integration/days.test.ts @@ -28,7 +28,7 @@ vi.mock('../../src/websocket', () => ({ broadcast: vi.fn(), broadcastToUser: vi. import { db as testDb } from '../../src/db/database'; import { buildApp } from '../../src/bootstrap'; import { resetTestDb, resetRateLimits } from '../helpers/test-db'; -import { createUser, createTrip, createDay, createPlace, addTripMember } from '../helpers/factories'; +import { createUser, createTrip, createDay, createPlace, addTripMember, createDayAccommodation, createReservation } from '../helpers/factories'; import { authCookie } from '../helpers/auth'; let nestApp: INestApplication; @@ -535,3 +535,132 @@ describe('Accommodations', () => { expect(budgetAfter).toBeUndefined(); }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// Plan 3c Task 9 fix wave — A-H2 (live regression): `DaysService` was +// converted with a bare `Number(tripId)` at every entry point while +// `restampReservationDates`/`assertNoInvertedAccommodation` (Plan 3d +// survivors) kept binding the RAW route string — an affinity seam a +// hex-spelled trip id never matches. `TripAccessGuard` authorises a +// hex-spelled id whose `Number()` value is a real trip (`Number('0x12')` +// is `18`), so one `PUT .../days/reorder` request could renumber that real +// trip's days (the `Number()` half) while its own inverted-accommodation +// guard silently matched zero rows (the raw-string half) — writing a stay +// whose end precedes its start with no error. Every entry point now parses +// ONCE with `toRowId` and threads that value into every survivor; a +// `toRowId` miss answers a legacy-shaped not-found instead of letting the +// guard's wider `Number()` grant reach a downstream raw bind that disagrees +// with it. +// ───────────────────────────────────────────────────────────────────────────── + +describe('A-H2 — DaysService trip id parsed once, threaded to every survivor (Task 9 fix wave)', () => { + it('GET /days by the hex-spelled trip id answers the legacy-shaped miss ({ days: [] }), not the real trip\'s days', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Trip', start_date: '2026-09-01', end_date: '2026-09-03' }); + const hexTripId = '0x' + trip.id.toString(16); + + const res = await request(app) + .get(`/api/trips/${hexTripId}/days`) + .set('Cookie', authCookie(user.id)); + expect(res.status).toBe(200); + expect(res.body).toEqual({ days: [] }); + }); + + it('PUT /days/:id by the hex-spelled trip id 404s "Day not found" (getDay\'s own toRowId gate), not the real trip\'s day', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Trip' }); + const day = createDay(testDb, trip.id, { title: 'Original' }); + const hexTripId = '0x' + trip.id.toString(16); + + const res = await request(app) + .put(`/api/trips/${hexTripId}/days/${day.id}`) + .set('Cookie', authCookie(user.id)) + .send({ title: 'Renamed' }); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Day not found' }); + const row = testDb.prepare('SELECT title FROM days WHERE id = ?').get(day.id) as { title: string }; + expect(row.title).toBe('Original'); + }); + + it('POST /days by the hex-spelled trip id (append, no position) 500s the legacy-shaped miss and writes no day', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Trip' }); + const hexTripId = '0x' + trip.id.toString(16); + const before = (testDb.prepare('SELECT COUNT(*) AS n FROM days WHERE trip_id = ?').get(trip.id) as { n: number }).n; + + const res = await request(app) + .post(`/api/trips/${hexTripId}/days`) + .set('Cookie', authCookie(user.id)) + .send({}); + expect(res.status).toBe(500); + expect(res.body).toEqual({ error: 'Internal server error' }); + const after = (testDb.prepare('SELECT COUNT(*) AS n FROM days WHERE trip_id = ?').get(trip.id) as { n: number }).n; + expect(after).toBe(before); + }); + + it('POST /days {position} (insert) by the hex-spelled trip id 500s the legacy-shaped miss and writes no day', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Trip', start_date: '2026-09-01', end_date: '2026-09-02' }); + const hexTripId = '0x' + trip.id.toString(16); + const before = (testDb.prepare('SELECT COUNT(*) AS n FROM days WHERE trip_id = ?').get(trip.id) as { n: number }).n; + + const res = await request(app) + .post(`/api/trips/${hexTripId}/days`) + .set('Cookie', authCookie(user.id)) + .send({ position: 1 }); + expect(res.status).toBe(500); + expect(res.body).toEqual({ error: 'Internal server error' }); + const after = (testDb.prepare('SELECT COUNT(*) AS n FROM days WHERE trip_id = ?').get(trip.id) as { n: number }).n; + expect(after).toBe(before); + }); + + it('PUT /days/reorder by the hex-spelled trip id 400s "orderedIds must be a permutation…" and writes NOTHING — the live H2 bug: it used to 200 and invert a real accommodation', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Trip', start_date: '2026-06-01', end_date: '2026-06-03' }); + const days = testDb.prepare('SELECT id, day_number, date FROM days WHERE trip_id = ? ORDER BY day_number').all(trip.id) as { id: number; day_number: number; date: string }[]; + expect(days).toHaveLength(3); + const [d1, d2, d3] = days; + const place = createPlace(testDb, trip.id, { name: 'Hotel' }); + // A valid (non-inverted) stay d1 -> d2, and a reservation on d1 — exactly + // the shape the compiled-boot evidence used to catch reorder inverting. + const stay = createDayAccommodation(testDb, trip.id, place.id, d1.id, d2.id); + const reservation = createReservation(testDb, trip.id, { day_id: d1.id, title: 'Dinner' }); + testDb.prepare('UPDATE reservations SET reservation_time = ? WHERE id = ?').run('2026-06-01T19:00:00', reservation.id); + const hexTripId = '0x' + trip.id.toString(16); + + const res = await request(app) + .put(`/api/trips/${hexTripId}/days/reorder`) + .set('Cookie', authCookie(user.id)) + .send({ orderedIds: [d2.id, d1.id, d3.id] }); + expect(res.status).toBe(400); + expect(res.body).toEqual({ error: 'orderedIds must be a permutation of the trip day ids.' }); + + // Nothing renumbered. + const after = testDb.prepare('SELECT id, day_number, date FROM days WHERE trip_id = ? ORDER BY day_number').all(trip.id) as { id: number; day_number: number; date: string }[]; + expect(after).toEqual(days); + // The stay is still d1 -> d2, never inverted. + const stayAfter = testDb.prepare('SELECT start_day_id, end_day_id FROM day_accommodations WHERE id = ?').get(stay.id) as { start_day_id: number; end_day_id: number }; + expect(stayAfter).toEqual({ start_day_id: d1.id, end_day_id: d2.id }); + // The reservation's stamped time is untouched. + const resAfter = testDb.prepare('SELECT reservation_time FROM reservations WHERE id = ?').get(reservation.id) as { reservation_time: string }; + expect(resAfter.reservation_time).toBe('2026-06-01T19:00:00'); + }); + + it('PUT /days/reorder with the REAL numeric trip id still works and can still be legitimately rejected for inverting a stay (unchanged, base 400)', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Trip', start_date: '2026-06-01', end_date: '2026-06-03' }); + const days = testDb.prepare('SELECT id FROM days WHERE trip_id = ? ORDER BY day_number').all(trip.id) as { id: number }[]; + const [d1, d2, d3] = days; + const place = createPlace(testDb, trip.id, { name: 'Hotel' }); + const stay = createDayAccommodation(testDb, trip.id, place.id, d1.id, d3.id); + + const res = await request(app) + .put(`/api/trips/${trip.id}/days/reorder`) + .set('Cookie', authCookie(user.id)) + .send({ orderedIds: [d3.id, d2.id, d1.id] }); + expect(res.status).toBe(400); + expect(res.body).toEqual({ error: 'This move would make an accommodation end before it starts.' }); + const stayAfter = testDb.prepare('SELECT start_day_id, end_day_id FROM day_accommodations WHERE id = ?').get(stay.id) as { start_day_id: number; end_day_id: number }; + expect(stayAfter).toEqual({ start_day_id: d1.id, end_day_id: d3.id }); + }); +}); diff --git a/server/tests/integration/places.test.ts b/server/tests/integration/places.test.ts index 84eb855c8..48226f651 100644 --- a/server/tests/integration/places.test.ts +++ b/server/tests/integration/places.test.ts @@ -1074,7 +1074,7 @@ describe('H1 — trip id parsed once at the gate (rule 21)', () => { expect(res.body).toEqual({ error: 'Place not found' }); }); - it('PUT with tags by the hex-spelled trip id 404s and leaves the tags untouched (H1 live: they used to be wiped)', async () => { + it('PUT with tags by the hex-spelled trip id 404s "Trip not found" (Task 9 fix wave: `verifyTripAccess` now gates BEFORE the place-id read, so the string changed from "Place not found") and leaves the tags untouched (H1 live: they used to be wiped)', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); const tagResult = testDb.prepare('INSERT INTO tags (name, user_id) VALUES (?, ?)').run('Original', user.id); @@ -1092,7 +1092,7 @@ describe('H1 — trip id parsed once at the gate (rule 21)', () => { .set('Cookie', authCookie(user.id)) .send({ name: 'Renamed', tags: [] }); expect(res.status).toBe(404); - expect(res.body).toEqual({ error: 'Place not found' }); + expect(res.body).toEqual({ error: 'Trip not found' }); const after = await request(app) .get(`/api/trips/${trip.id}/places/${placeId}`) @@ -1101,7 +1101,7 @@ describe('H1 — trip id parsed once at the gate (rule 21)', () => { expect((after.body.place.tags as { id: number }[]).some((t) => t.id === tagId)).toBe(true); }); - it('PUT :id/rating by the hex-spelled trip id 404s', async () => { + it('PUT :id/rating by the hex-spelled trip id 404s "Trip not found" (Task 9 fix wave: was "Place not found" — see the PUT :id case above)', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); const place = createPlace(testDb, trip.id, { name: 'Rated' }); @@ -1112,11 +1112,11 @@ describe('H1 — trip id parsed once at the gate (rule 21)', () => { .set('Cookie', authCookie(user.id)) .send({ rating: 4 }); expect(res.status).toBe(404); - expect(res.body).toEqual({ error: 'Place not found' }); + expect(res.body).toEqual({ error: 'Trip not found' }); expect(testDb.prepare('SELECT COUNT(*) AS n FROM place_ratings WHERE place_id = ?').get(place.id)).toEqual({ n: 0 }); }); - it('POST create by the hex-spelled trip id mirrors the legacy 500 (base 94c6efbbc: FK violation on the raw-bound trip id) — every OTHER route 404s, this is the one documented exception', async () => { + it('POST create by the hex-spelled trip id now 404s "Trip not found" (Task 9 fix wave, M1: `verifyTripAccess` gates with `toRowId` before `create()` is ever reached — the base 94c6efbbc 500 this test used to mirror was ruled a defect, not the contract to preserve, once the gate itself refuses the id)', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); const hexTripId = '0x' + trip.id.toString(16); @@ -1125,8 +1125,8 @@ describe('H1 — trip id parsed once at the gate (rule 21)', () => { .post(`/api/trips/${hexTripId}/places`) .set('Cookie', authCookie(user.id)) .send({ name: 'Should not land' }); - expect(res.status).toBe(500); - expect(res.body).toEqual({ error: 'Internal server error' }); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Trip not found' }); expect(testDb.prepare('SELECT COUNT(*) AS n FROM places WHERE trip_id = ?').get(trip.id)).toEqual({ n: 0 }); }); @@ -1148,7 +1148,7 @@ describe('H1 — trip id parsed once at the gate (rule 21)', () => { expect(broadcast).not.toHaveBeenCalled(); }); - it('POST bulk-delete by the hex-spelled trip id deletes nothing (scopedIds already binds the trip id raw, so this was never divergent — kept as a regression alongside the single-delete case)', async () => { + it('POST bulk-delete by the hex-spelled trip id now 404s "Trip not found" and deletes nothing (Task 9 fix wave: `verifyTripAccess` gates before `scopedIds`/`removeMany` ever run — was a 200 with `count: 0`)', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); const place = createPlace(testDb, trip.id, { name: 'Louvre' }); @@ -1160,13 +1160,157 @@ describe('H1 — trip id parsed once at the gate (rule 21)', () => { .post(`/api/trips/${hexTripId}/places/bulk-delete`) .set('Cookie', authCookie(user.id)) .send({ ids: [place.id] }); - expect(res.status).toBe(200); - expect(res.body).toEqual({ deleted: [], count: 0 }); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Trip not found' }); expect(testDb.prepare('SELECT id FROM places WHERE id = ?').get(place.id)).toBeTruthy(); expect(broadcast).not.toHaveBeenCalled(); }); }); +// ───────────────────────────────────────────────────────────────────────────── +// Plan 3c Task 9 fix wave — A-H1 / A-M1 / B-H1: `verifyTripAccess` parses +// ONCE with `toRowId` (never `Number()`) and answers 404 `Trip not found` +// before ANY read or write on the 10 `requireTrip`-gated routes above, +// closing two live regressions at once: +// - A-H1: a non-numeric id (`abc`, `1abc`) used to become `NaN` and 500 +// (`no such column: NaN`) instead of the legacy 404 — every one of +// these routes now answers the SAME 404 a stranger's clean miss does. +// - A-M1 / B-H1: a numeric-but-non-canonical id (`1.0`, `' 1'`, `'+1'`, +// `'1e0'`) used to pass the old loose `Number()` gate and then +// manufacture a FRESH 500 at the write (`toRowId(tripId) ?? -1`, an FK +// violation on `-1`) — an id the gate itself authorised. It now 404s at +// the gate instead: an ACCEPTED rule-15 narrowing (legacy's raw-bind +// affinity matched these forms; `toRowId` deliberately does not), named +// here rather than claimed as parity. +// ───────────────────────────────────────────────────────────────────────────── + +describe('A-H1 / A-M1 / B-H1 — verifyTripAccess parses once with toRowId (Task 9 fix wave)', () => { + const nonCanonicalShapes: [label: string, spell: (id: number) => string][] = [ + ['a non-numeric id (abc)', () => 'abc'], + ['a numeric-suffixed id (1abc)', (id) => `${id}abc`], + ['a decimal-spelled id (1.0) — rule-15 narrowing, base 201', (id) => `${id}.0`], + ['a leading-space id (\' 1\') — rule-15 narrowing, base 201', (id) => ` ${id}`], + ['a leading-plus id (+1) — rule-15 narrowing, base 201', (id) => `+${id}`], + ['an exponent-spelled id (1e0) — rule-15 narrowing, base 201', (id) => `${id}e0`], + ]; + + describe.each(nonCanonicalShapes)('POST create — %s', (_label, spell) => { + it('404s "Trip not found" and creates nothing', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const spelled = spell(trip.id); + + const res = await request(app) + .post(`/api/trips/${spelled}/places`) + .set('Cookie', authCookie(user.id)) + .send({ name: 'Should not land' }); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Trip not found' }); + expect(testDb.prepare('SELECT COUNT(*) AS n FROM places WHERE trip_id = ?').get(trip.id)).toEqual({ n: 0 }); + }); + }); + + describe.each(nonCanonicalShapes)('PUT :id — %s', (_label, spell) => { + it('404s "Trip not found" and leaves the place untouched', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Untouched' }); + const spelled = spell(trip.id); + + const res = await request(app) + .put(`/api/trips/${spelled}/places/${place.id}`) + .set('Cookie', authCookie(user.id)) + .send({ name: 'Renamed' }); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Trip not found' }); + const row = testDb.prepare('SELECT name FROM places WHERE id = ?').get(place.id) as { name: string }; + expect(row.name).toBe('Untouched'); + }); + }); + + describe.each(nonCanonicalShapes)('POST bulk-delete — %s', (_label, spell) => { + it('404s "Trip not found" and deletes nothing', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Louvre' }); + const spelled = spell(trip.id); + vi.mocked(broadcast).mockClear(); + + const res = await request(app) + .post(`/api/trips/${spelled}/places/bulk-delete`) + .set('Cookie', authCookie(user.id)) + .send({ ids: [place.id] }); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Trip not found' }); + expect(testDb.prepare('SELECT id FROM places WHERE id = ?').get(place.id)).toBeTruthy(); + expect(broadcast).not.toHaveBeenCalled(); + }); + }); + + describe.each(nonCanonicalShapes)('POST bulk-update — %s', (_label, spell) => { + it('404s "Trip not found" and updates nothing', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Louvre', category_id: null }); + const spelled = spell(trip.id); + + const res = await request(app) + .post(`/api/trips/${spelled}/places/bulk-update`) + .set('Cookie', authCookie(user.id)) + .send({ ids: [place.id], category_id: null }); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Trip not found' }); + }); + }); + + describe.each(nonCanonicalShapes)('PUT :id/rating — %s', (_label, spell) => { + it('404s "Trip not found" and writes no rating', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Rated' }); + const spelled = spell(trip.id); + + const res = await request(app) + .put(`/api/trips/${spelled}/places/${place.id}/rating`) + .set('Cookie', authCookie(user.id)) + .send({ rating: 4 }); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Trip not found' }); + expect(testDb.prepare('SELECT COUNT(*) AS n FROM place_ratings WHERE place_id = ?').get(place.id)).toEqual({ n: 0 }); + }); + }); + + describe.each(nonCanonicalShapes)('GET export.gpx — %s', (_label, spell) => { + it('404s "Trip not found"', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const spelled = spell(trip.id); + + const res = await request(app) + .get(`/api/trips/${spelled}/places/export.gpx`) + .set('Cookie', authCookie(user.id)); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Trip not found' }); + }); + }); + + describe.each(nonCanonicalShapes)('POST import/gpx — %s', (_label, spell) => { + it('404s "Trip not found" and imports nothing', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const spelled = spell(trip.id); + + const res = await request(app) + .post(`/api/trips/${spelled}/places/import/gpx`) + .set('Cookie', authCookie(user.id)) + .attach('file', GPX_FIXTURE); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Trip not found' }); + expect(testDb.prepare('SELECT COUNT(*) AS n FROM places WHERE trip_id = ?').get(trip.id)).toEqual({ n: 0 }); + }); + }); +}); + // ───────────────────────────────────────────────────────────────────────────── // M1 / program rule 22 (task-4-review.md) — a NUL byte in a user string no // longer 500s: the platform override restores the legacy raw-bind's diff --git a/server/tests/unit/db/nul-safe-sqlite-platform.test.ts b/server/tests/unit/db/nul-safe-sqlite-platform.test.ts index 13823c37c..87a87cbd6 100644 --- a/server/tests/unit/db/nul-safe-sqlite-platform.test.ts +++ b/server/tests/unit/db/nul-safe-sqlite-platform.test.ts @@ -1,7 +1,12 @@ import Database from 'better-sqlite3'; -import { describe, expect, it } from 'vitest'; +import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest'; import { NulSafeSqlitePlatform } from '../../../src/db/nul-safe-sqlite-platform'; +import { createSnapshotTestDb } from '../../helpers/db-mock'; +import { resetTestDb } from '../../helpers/test-db'; +import { createTestOrm, type TestOrm } from '../../helpers/test-orm'; +import { createTrip, createUser } from '../../helpers/factories'; +import { Trips } from '../../../src/db/entities/Trips.entity'; /** * Plan 3c Task 4 review M1 / program rule 22: MikroORM 7 inlines every bound @@ -85,4 +90,105 @@ describe('NulSafeSqlitePlatform', () => { const row = db.prepare(selectSql).get() as { name: string } | undefined; expect(row?.name).toBe(value); }); + + /** + * Program rule 22's extension (Plan 3c Task 9 close-out, review A H1): a + * non-finite `number` used to render as an unquoted bareword (`NaN`/ + * `Infinity`/`-Infinity`), which SQLite parses as a column reference and + * throws `no such column: …` — a 500 where the raw better-sqlite3 bind + * (`NaN` -> `NULL`, `±Infinity` -> the `REAL` value, verified empirically + * against the installed driver above the class docstring) never had the + * problem. These pin the rendered SQL literal AND the real round-trip. + */ + describe('non-finite numbers (rule 22 extension)', () => { + it('NaN renders as the literal `NULL`, not the bareword `NaN`', () => { + expect(platform.escape(NaN)).toBe('NULL'); + }); + + it('+Infinity renders as the literal `9e999`, which SQLite parses back to the REAL value Infinity', () => { + expect(platform.escape(Infinity)).toBe('9e999'); + const row = db.prepare('select typeof(9e999) as t, 9e999 as v').get() as { t: string; v: number }; + expect(row).toEqual({ t: 'real', v: Infinity }); + }); + + it('-Infinity renders as the literal `-9e999`, which SQLite parses back to the REAL value -Infinity', () => { + expect(platform.escape(-Infinity)).toBe('-9e999'); + const row = db.prepare('select typeof(-9e999) as t, -9e999 as v').get() as { t: string; v: number }; + expect(row).toEqual({ t: 'real', v: -Infinity }); + }); + + it('a finite number is untouched (only the three non-finite values take the new branch)', () => { + expect(platform.escape(0)).toBe('0'); + expect(platform.escape(-1)).toBe('-1'); + expect(platform.escape(3.14)).toBe('3.14'); + }); + + it('formatQuery renders `NULL` for a bound NaN, matching better-sqlite3\'s own binding of NaN as NULL', () => { + db.exec('CREATE TABLE nanprobe (n REAL)'); + db.exec(platform.formatQuery('insert into nanprobe (n) values (?)', [NaN])); + const viaLiteral = db.prepare('select n from nanprobe').get() as { n: unknown }; + const viaBind = db.prepare('select ? as n').get(NaN) as { n: unknown }; + expect(viaLiteral.n).toBeNull(); + expect(viaBind.n).toBeNull(); + }); + + it('formatQuery renders `9e999`/`-9e999` for bound ±Infinity, matching better-sqlite3\'s own REAL binding', () => { + db.exec('CREATE TABLE infprobe (n REAL)'); + db.exec(platform.formatQuery('insert into infprobe (n) values (?), (?)', [Infinity, -Infinity])); + const rows = db.prepare('select n from infprobe order by rowid').all() as { n: number }[]; + expect(rows.map(r => r.n)).toEqual([Infinity, -Infinity]); + }); + }); + + /** + * The mechanism above through the REAL ORM call path (`findOne`, + * `nativeUpdate`, a QB `where`, and an `$in` list), against a real trip + * row — the shape `PlacesService.verifyTripAccess`/`RealtimeGateway + * .handleJoin` reach when a non-finite id slips past their own guard. + * Before this override, every one of these threw `SqliteError: no such + * column: NaN`; after, each answers a clean miss. + */ + describe('non-finite numbers through the real ORM call path', () => { + const orm = createSnapshotTestDb(); + let t: TestOrm; + + beforeAll(async () => { + t = await createTestOrm(orm); + }); + beforeEach(() => { resetTestDb(orm); t.clear(); }); + afterAll(async () => { await t.close(); orm.close(); }); + + it('em.findOne(Trips, { id: NaN }) misses cleanly, never throws', async () => { + const { user } = createUser(orm); + createTrip(orm, user.id); + await expect(t.em.findOne(Trips, { id: NaN })).resolves.toBeNull(); + }); + + it('em.nativeUpdate(Trips, { id: NaN }, …) updates 0 rows, never throws', async () => { + const { user } = createUser(orm); + const trip = createTrip(orm, user.id); + const affected = await t.em.nativeUpdate(Trips, { id: NaN }, { title: 'renamed' }); + expect(affected).toBe(0); + const untouched = orm.prepare('SELECT title FROM trips WHERE id = ?').get(trip.id) as { title: string }; + expect(untouched.title).not.toBe('renamed'); + }); + + it('a QB `where` on a non-finite id misses cleanly, never throws (the exact `findAccessible` shape)', async () => { + const { user } = createUser(orm); + createTrip(orm, user.id); + const row = await t.em.getRepository(Trips) + .qb('t') + .select(['t.id']) + .where('t.id = ?', [NaN]) + .execute<{ id: number } | undefined>('get', false); + expect(row).toBeUndefined(); + }); + + it('an `$in` list containing NaN/Infinity still matches the finite members, never throws', async () => { + const { user } = createUser(orm); + const trip = createTrip(orm, user.id); + const rows = await t.em.find(Trips, { id: { $in: [trip.id, NaN, Infinity, -Infinity] } }); + expect(rows.map(r => r.id)).toEqual([trip.id]); + }); + }); }); diff --git a/server/tests/unit/db/repositories/Users.repository.test.ts b/server/tests/unit/db/repositories/Users.repository.test.ts index ba8c74570..ce73e9531 100644 --- a/server/tests/unit/db/repositories/Users.repository.test.ts +++ b/server/tests/unit/db/repositories/Users.repository.test.ts @@ -813,17 +813,30 @@ describe('UsersRepository — TripMembersService (Plan 3c Task 6)', () => { }); describe('findInvitableByEmailOrUsername (TM4)', () => { + // Task 9 fix wave (B-L7): the legacy statement TM4's docstring quotes, + // run raw on the same fixture — `toEqual(legacy)` on the FULL key set, + // so a renamed or dropped column would fail this, not just the + // literal-object assertion below (rule 19; #1362 is security-relevant, + // so this is one of the L7 reads the fix wave covers). + const legacyInvitable = (identifier: string) => + testDb.prepare( + 'SELECT id, username, email, avatar FROM users WHERE (email = ? OR username = ?) AND COALESCE(is_guest, 0) = 0', + ).get(identifier, identifier) ?? null; + it('USERSREPO-063: matches on email OR username, case-SENSITIVE (unlike findByEmailCI/findIdByEmailCI above)', async () => { const { user: byEmail } = createUser(testDb, { email: 'invitee@example.test', username: 'invitee-handle' }); expect(await users.findInvitableByEmailOrUsername('invitee@example.test')).toEqual({ id: byEmail.id, username: 'invitee-handle', email: 'invitee@example.test', avatar: null, }); + expect(await users.findInvitableByEmailOrUsername('invitee@example.test')).toEqual(legacyInvitable('invitee@example.test')); expect(await users.findInvitableByEmailOrUsername('invitee-handle')).toEqual({ id: byEmail.id, username: 'invitee-handle', email: 'invitee@example.test', avatar: null, }); + expect(await users.findInvitableByEmailOrUsername('invitee-handle')).toEqual(legacyInvitable('invitee-handle')); // Case-sensitive: an uppercased email must NOT match (program rule 18 // only applies where the legacy statement itself folds case — TM4's does not). expect(await users.findInvitableByEmailOrUsername('INVITEE@EXAMPLE.TEST')).toBeNull(); + expect(legacyInvitable('INVITEE@EXAMPLE.TEST')).toBeNull(); }); it('USERSREPO-064: excludes guests — a trip-scoped guest can never be re-invited through the box', async () => { @@ -837,11 +850,17 @@ describe('UsersRepository — TripMembersService (Plan 3c Task 6)', () => { expect(await users.findInvitableByEmailOrUsername('nobody@example.test')).toBeNull(); }); - // Task 6 review, L3: `findInvitableByEmailOrUsername` is `findOne`-based - // (unlike the qb-based projections above), so rule 21's D-shape test - // applies for real — this is the guest-exclusion guard (#1362), so - // staleness here would re-open the bug the guard exists to close. - it('USERSREPO-072 (D-shape): an is_guest flip written after an unrelated identity-map read is visible in the FIRST wider projection', async () => { + // Task 9 fix wave (B-M3): relabelled — this comment previously claimed + // rule 20's PK-only D-shape guarantee "applies for real", but + // `findInvitableByEmailOrUsername` filters `findOne` on `{$or: + // [{email},{username}], is_guest}`, not the primary key (rule 20 draws + // that PK-only line precisely because a non-PK filter always re-runs + // the WHERE clause against the live table, so it can never serve a + // stale identity-map snapshot the way a PK lookup can). What this DOES + // prove, honestly: a row that flips `is_guest` after an unrelated wider + // identity-map read is fresh here — the guest-exclusion guard (#1362) + // sees the current value, not a cached one. + it('USERSREPO-072 (fresh after a raw UPDATE, not D-shape): an is_guest flip written after an unrelated identity-map read is visible in the FIRST wider projection', async () => { const { user } = createUser(testDb, { email: 'flip@example.test', username: 'flip-handle' }); await t.repo(Users).find({}, { disableIdentityMap: false }); // populate the identity map with an unrelated read testDb.prepare('UPDATE users SET is_guest = 1 WHERE id = ?').run(user.id); diff --git a/server/tests/unit/db/transaction-yield.test.ts b/server/tests/unit/db/transaction-yield.test.ts new file mode 100644 index 000000000..032caf38b --- /dev/null +++ b/server/tests/unit/db/transaction-yield.test.ts @@ -0,0 +1,175 @@ +/** + * Program rule 24 (`docs/superpowers/plans/2026-09-21-orm-migration-program.md`), + * added by Plan 3c Task 9's whole-plan review (reviewer A, ruling 5): "transactional + * bodies await DB work only." The server holds ONE better-sqlite3 connection, shared + * by MikroORM's Kysely-backed driver (`orm-driver.ts`'s `BoundSqliteDriver`) and by + * `DatabaseService`'s raw `db.prepare(...)` calls. This file pins the two concrete + * behaviours that measurement (fork A2, real `UnitOfWork` + `withRequestContext` + + * the bound driver, no mocks) found when a `uow.transactional` body is left open + * across a real `await` (which no converted service does today — a static scan of + * all 131 `.transactional(` call sites found none awaiting non-DB I/O; this file's + * own transactional bodies hold one open ONLY to observe the window, which is + * exactly the shape a future violation of rule 24 would take): + * + * A. Another request's ORM/Kysely statement QUEUES behind the open transaction — + * Kysely's own `ConnectionMutex` for SQLite serialises every statement onto + * the one connection, so there is no interleaving: the queued statement's + * result only becomes observable after the holder's transaction settles. + * B. A raw `DatabaseService` statement issued while that same transaction is + * open does NOT queue — it shares the connection directly, with no mutex of + * its own, so it runs INSIDE the open transaction: a "dirty read" of the + * transaction's uncommitted write, which then reverts to its pre-transaction + * value the moment the holder rolls back (the hazard rule 24 exists to keep + * out of production code, by keeping every transactional body DB-only). + * + * Both requests run through `withRequestContext` (MikroORM's `RequestContext`, + * AsyncLocalStorage-based), the same per-request fork every real HTTP/MCP/WS/cron + * entrypoint uses — not two calls sharing one ambient EntityManager, which would + * make MikroORM treat the second as a SAVEPOINT of the first (`UnitOfWork`'s own + * docstring: NESTED propagation) rather than the independent, connection-mutex- + * queued transaction two unrelated concurrent requests actually are. + */ +import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest'; +import { createSnapshotTestDb } from '../../helpers/db-mock'; +import { resetTestDb } from '../../helpers/test-db'; +import { createTestOrm, type TestOrm } from '../../helpers/test-orm'; +import { createUser, createTrip } from '../../helpers/factories'; +import { Trips } from '../../../src/db/entities/Trips.entity'; +import type { TripsRepository } from '../../../src/db/repositories/Trips.repository'; +import { UnitOfWork } from '../../../src/nest/database/unit-of-work'; +import { withRequestContext } from '../../../src/nest/database/request-context'; +import { DatabaseService } from '../../../src/nest/database/database.service'; + +const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); + +const testDb = createSnapshotTestDb(); +let t: TestOrm; +let trips: TripsRepository; +let uow: UnitOfWork; +let dbs: DatabaseService; + +beforeAll(async () => { + t = await createTestOrm(testDb); + trips = t.repo(Trips); + uow = new UnitOfWork(t.em); + dbs = new DatabaseService(testDb, t.em); +}); +beforeEach(() => { resetTestDb(testDb); t.clear(); }); +afterAll(async () => { await t.close(); testDb.close(); }); + +describe('transaction yield (rule 24)', () => { + it('A — another request\'s ORM statement queues behind an open transaction: no interleaving, and its result only becomes observable after the holder commits', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { end_date: '2026-01-01' }); + + const order: string[] = []; + const HOLD_MS = 40; + + // Request 1: opens a transaction, holds it open across a real `await` (the + // window this test needs to observe — never how production code shapes a + // transactional body; see the file docstring), then writes and commits. + const holder = withRequestContext(t.orm, async () => { + return uow.transactional(async () => { + order.push('holder:open'); + await sleep(HOLD_MS); + await trips.setEndDate(trip.id, '2099-12-31'); + order.push('holder:commit'); + }); + }); + + // Give the holder a tick to actually open its transaction before the + // second "request" starts, so the queued statement is provably issued + // while the first is open, not racing to start first itself. + await sleep(5); + + // Request 2: an unrelated concurrent request's plain ORM read (no + // transaction of its own) for the SAME row, through its own forked + // context — the shape `TripAccessGuard`/`findAccessible` reads take. + const queued = withRequestContext(t.orm, async () => { + order.push('queued:issue'); + const row = await trips.getTitle(trip.id); + order.push('queued:resolve'); + return row; + }); + + await Promise.all([holder, queued]); + + // The queued read was ISSUED while the holder was still open (between + // "holder:open" and "holder:commit")… + expect(order.indexOf('queued:issue')).toBeGreaterThan(order.indexOf('holder:open')); + expect(order.indexOf('queued:issue')).toBeLessThan(order.indexOf('holder:commit')); + // …but it only RESOLVED after the holder committed — the connection + // mutex queued it rather than interleaving it inside the open transaction. + expect(order.indexOf('queued:resolve')).toBeGreaterThan(order.indexOf('holder:commit')); + }); + + it('B — a raw DatabaseService statement issued while a transaction is open runs INSIDE it (a dirty read of the uncommitted write), then reverts when the holder rolls back', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { end_date: '2026-01-01' }); + + let dirtyRead: string | null | undefined; + const HOLD_MS = 40; + + const holder = withRequestContext(t.orm, async () => { + await expect( + uow.transactional(async () => { + await trips.setEndDate(trip.id, '2099-12-31'); + // The write is now inside the open, uncommitted transaction. A raw + // statement issued from a DIFFERENT concurrent "request" — no + // request-context fork, no `uow.transactional` of its own — shares + // the one better-sqlite3 connection directly and is NOT behind + // Kysely's mutex, so it runs synchronously, right now, still + // inside this open transaction. + const row = dbs.get<{ end_date: string | null }>('SELECT end_date FROM trips WHERE id = ?', trip.id); + dirtyRead = row?.end_date; + await sleep(HOLD_MS); + // Force a rollback: the write above must not survive. + throw new Error('forced rollback (rule 24 probe)'); + }), + ).rejects.toThrow('forced rollback'); + }); + + await holder; + + // The raw read saw the uncommitted value — a dirty read, proving the raw + // path is NOT queued behind the transaction the way an ORM/Kysely + // statement is (probe A above). + expect(dirtyRead).toBe('2099-12-31'); + // After the holder rolled back, the row reverts — the dirty read never + // should have been trusted, which is exactly why rule 24 keeps every + // transactional body DB-only: a raw statement issued mid-transaction by + // ANOTHER request would otherwise report success on a write that this + // rollback just erased. + const after = testDb.prepare('SELECT end_date FROM trips WHERE id = ?').get(trip.id) as { end_date: string | null }; + expect(after.end_date).toBe('2026-01-01'); + }); + + it('I — a second, unrelated transaction opened while the first is still open does not error (no nested-BEGIN exception); it queues and commits its own write once the first settles', async () => { + const { user } = createUser(testDb); + const tripA = createTrip(testDb, user.id, { end_date: '2026-01-01' }); + const tripB = createTrip(testDb, user.id, { end_date: '2026-01-01' }); + const HOLD_MS = 40; + + const first = withRequestContext(t.orm, () => + uow.transactional(async () => { + await sleep(HOLD_MS); + await trips.setEndDate(tripA.id, '2099-01-01'); + }), + ); + // Give the first a moment to actually open before the second starts, so + // the second's own BEGIN is provably attempted while one is already open. + await sleep(5); + const second = withRequestContext(t.orm, () => + uow.transactional(async () => { + await trips.setEndDate(tripB.id, '2099-02-01'); + }), + ); + + await expect(Promise.all([first, second])).resolves.toBeDefined(); + + const rowA = testDb.prepare('SELECT end_date FROM trips WHERE id = ?').get(tripA.id) as { end_date: string | null }; + const rowB = testDb.prepare('SELECT end_date FROM trips WHERE id = ?').get(tripB.id) as { end_date: string | null }; + expect(rowA.end_date).toBe('2099-01-01'); + expect(rowB.end_date).toBe('2099-02-01'); + }); +}); diff --git a/server/tests/unit/nest/places.directions.test.ts b/server/tests/unit/nest/places.directions.test.ts index 3601f3a2e..2468005da 100644 --- a/server/tests/unit/nest/places.directions.test.ts +++ b/server/tests/unit/nest/places.directions.test.ts @@ -54,7 +54,7 @@ import type { PlacePhotoCacheService } from '../../../src/nest/place-photos/plac import { JourneyDomainService } from '../../../src/nest/journey/journey-domain.service'; import { TrekPhotosRepository } from '../../../src/nest/photos/trek-photos.repository'; import { makeStorageFixture } from '../../helpers/storage-fixture'; -import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo, createTestTagsRepo, createTestPlaceRatingsRepo, createTestAssignmentParticipantsRepo, createTestPlacesRepo, createTestTripMembersRepo, createTestDayAssignmentsRepo, createTestCategoriesRepo, sharedTestOrm } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo, createTestTagsRepo, createTestPlaceRatingsRepo, createTestAssignmentParticipantsRepo, createTestPlacesRepo, createTestTripMembersRepo, createTestDayAssignmentsRepo, createTestCategoriesRepo, createTestTripsRepo, sharedTestOrm } from '../../helpers/test-uow'; const dbs = new DatabaseService(testDb); const photoCacheStub = { removeIfUnreferenced: vi.fn() } as unknown as PlacePhotoCacheService; @@ -98,6 +98,7 @@ async function svc(searchNominatim: MapsService['searchNominatim']): Promise { const real = new DatabaseService(testDb, (await sharedTestOrm(testDb)).em); vi.spyOn(dbs, 'canAccessTrip').mockImplementation((...a) => real.canAccessTrip(...a)); vi.spyOn(dbs, 'isOwner').mockImplementation((...a) => real.isOwner(...a)); - vi.spyOn(dbs, 'rosterUserIds').mockImplementation((...a) => real.rosterUserIds(...a)); + // Task 9 fix wave (A-L4c): `dbs.rosterUserIds` is dead here — `PlacesService` + // now calls `TripMembersRepository.rosterUserIds` directly (PL1, injected, + // not through `DatabaseService`), so this spy was inert. vi.spyOn(dbs, 'getPlaceWithTags').mockImplementation((...a) => real.getPlaceWithTags(...a)); accommodations = await accommodationsOver(dbs); svc = await makePlacesService(); diff --git a/server/tests/unit/nest/trip-membership/trip-membership.service.test.ts b/server/tests/unit/nest/trip-membership/trip-membership.service.test.ts index 0bc27f02a..ce85ecb7c 100644 --- a/server/tests/unit/nest/trip-membership/trip-membership.service.test.ts +++ b/server/tests/unit/nest/trip-membership/trip-membership.service.test.ts @@ -93,17 +93,23 @@ describe('joinTripAsMember', () => { svc.joinTripAsMember(trip.id, joiner.id, null), svc.joinTripAsMember(trip.id, joiner.id, null), ]); - // Today's actual outcome (run under real concurrency, not assumed): the - // method performs no locking/serialization of its own, so both calls can - // pass the TB5 "already a member" check before either TB6 INSERT lands — - // the loser then hits the table's own UNIQUE(trip_id, user_id) constraint - // and REJECTS, it does not swallow the error into `{joined: false}`. At - // least one call always fulfills (the winner); pinning "both always - // fulfill" was wrong (verified: failed under the full-suite coverage - // run's real scheduling) — the row-count assertion below is the actual - // safety net, not a specific promise-settlement shape. - const fulfilled = results.filter((r) => r.status === 'fulfilled'); - expect(fulfilled.length).toBeGreaterThanOrEqual(1); + // Task 9 fix wave (A-L2 / B-L8): the exact settlement set, not `>= 1` — + // measured 20/20 on the real scheduler (A2's `r5mut.py`): one call wins + // (`{joined: true, tripId}`, TB6's INSERT lands), the other loses to the + // table's own UNIQUE(trip_id, user_id) constraint and REJECTS — the + // method performs no locking/serialization of its own (TB5 "already a + // member" check → TB6 INSERT, unchanged by this conversion, §18.4), so + // both calls can pass TB5 before either TB6 lands. A `>= 1` assertion + // would also pass if the race somehow let both calls through, which is + // precisely the bug this test exists to catch (order-insensitive: + // `Promise.allSettled` does not guarantee which of the two promises is + // the winner). + const fulfilled = results.filter((r): r is PromiseFulfilledResult<{ joined: boolean; tripId: number }> => r.status === 'fulfilled'); + const rejected = results.filter((r): r is PromiseRejectedResult => r.status === 'rejected'); + expect(fulfilled.length).toBe(1); + expect(fulfilled[0].value).toEqual({ joined: true, tripId: trip.id }); + expect(rejected.length).toBe(1); + expect((rejected[0].reason as Error).message).toMatch(/UNIQUE constraint failed: trip_members/); // Exactly one membership row exists afterwards either way — the UNIQUE // constraint on (trip_id, user_id) is the actual safety net today, not // application-level locking. diff --git a/server/tests/unit/nest/trip-read-model.service.test.ts b/server/tests/unit/nest/trip-read-model.service.test.ts index 9dc5e8ca3..58d0f7758 100644 --- a/server/tests/unit/nest/trip-read-model.service.test.ts +++ b/server/tests/unit/nest/trip-read-model.service.test.ts @@ -137,6 +137,7 @@ beforeAll(async () => { await createTestTripMembersRepo(dbs().connection), await createTestDayAssignmentsRepo(dbs().connection), await createTestCategoriesRepo(dbs().connection), + await createTestTripsRepo(dbs().connection), ); membersSvc = new TripMembersService(dbs(), budgetSvc, new UserCleanupService(dbs(), budgetSvc, await createTestUnitOfWork(dbs().connection), await createTestUsersRepo(dbs().connection)), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new RealtimeService(), notificationsStub(), await createTestUnitOfWork(dbs().connection), await createTestTripsRepo(dbs().connection), await createTestTripMembersRepo(dbs().connection), await createTestUsersRepo(dbs().connection)); }); diff --git a/server/tests/unit/nest/trips.service.test.ts b/server/tests/unit/nest/trips.service.test.ts index 84cab008a..e0cf1bd5e 100644 --- a/server/tests/unit/nest/trips.service.test.ts +++ b/server/tests/unit/nest/trips.service.test.ts @@ -174,6 +174,7 @@ beforeAll(async () => { await createTestTripMembersRepo(dbs().connection), await createTestDayAssignmentsRepo(dbs().connection), await createTestCategoriesRepo(dbs().connection), + await createTestTripsRepo(dbs().connection), ); svc = new TripsService( dbs(), @@ -1352,7 +1353,7 @@ describe('copy — whole-trip parity (Task 8)', () => { const newTripId = await svc.copy(trip.id, owner.id, 'Full Copy'); // trips - const newTrip = testDb.prepare('SELECT * FROM trips WHERE id = ?').get(newTripId) as any; + const newTrip = testDb.prepare('SELECT * FROM trips WHERE id = ?').get(newTripId) as Record; expect(newTrip).toMatchObject({ user_id: owner.id, title: 'Full Copy', description: 'A full trip', start_date: '2025-09-01', end_date: '2025-09-03', currency: 'EUR', @@ -1365,50 +1366,65 @@ describe('copy — whole-trip parity (Task 8)', () => { expect(newDays.map(d => ({ day_number: d.day_number, date: d.date }))).toEqual( days.map(d => ({ day_number: d.day_number, date: d.date })), ); - expect((testDb.prepare('SELECT notes, title FROM days WHERE id = ?').get(newDays[0].id) as any)).toEqual({ notes: 'Pack light', title: 'Arrival' }); - expect((testDb.prepare('SELECT notes FROM days WHERE id = ?').get(newDays[1].id) as any).notes).toBe('Checkout'); + expect((testDb.prepare('SELECT notes, title FROM days WHERE id = ?').get(newDays[0].id) as { notes: string | null; title: string | null })).toEqual({ notes: 'Pack light', title: 'Arrival' }); + expect((testDb.prepare('SELECT notes FROM days WHERE id = ?').get(newDays[1].id) as { notes: string | null }).notes).toBe('Checkout'); // places - const newStop = testDb.prepare('SELECT * FROM places WHERE trip_id = ? AND name = ?').get(newTripId, 'Hotel Full') as any; - const newTrack = testDb.prepare('SELECT * FROM places WHERE trip_id = ? AND name = ?').get(newTripId, 'Scenic road') as any; + const newStop = testDb.prepare('SELECT * FROM places WHERE trip_id = ? AND name = ?').get(newTripId, 'Hotel Full') as { + id: number; description: string | null; reservation_status: string | null; reservation_notes: string | null; + reservation_datetime: string | null; route_color: string | null; stop_type: string | null; fill_percent: number | null; + }; + const newTrack = testDb.prepare('SELECT * FROM places WHERE trip_id = ? AND name = ?').get(newTripId, 'Scenic road') as { id: number }; expect(newStop).toMatchObject({ description: 'Nice place', reservation_status: 'booked', reservation_notes: 'rn', reservation_datetime: '2025-09-01T14:00', route_color: '#ff0000', stop_type: 'hotel', fill_percent: 80, }); // place_tags - expect((testDb.prepare('SELECT tag_id FROM place_tags WHERE place_id = ?').get(newStop.id) as any).tag_id).toBe(tag); + expect((testDb.prepare('SELECT tag_id FROM place_tags WHERE place_id = ?').get(newStop.id) as { tag_id: number }).tag_id).toBe(tag); // day_assignments (+ the TP57 accommodation stamp) - const newAssignment = getAssignments(newDays[0].id)[0] as any; - const newAssignmentFull = testDb.prepare('SELECT * FROM day_assignments WHERE id = ?').get(newAssignment.id) as any; + const newAssignment = getAssignments(newDays[0].id)[0]; + const newAssignmentFull = testDb.prepare('SELECT * FROM day_assignments WHERE id = ?').get(newAssignment.id) as { + place_id: number; reservation_status: string | null; reservation_notes: string | null; reservation_datetime: string | null; + assignment_time: string | null; assignment_end_time: string | null; end_day: number; accommodation_id: number | null; + }; expect(newAssignmentFull).toMatchObject({ place_id: newStop.id, reservation_status: 'booked', reservation_notes: 'arn', reservation_datetime: '2025-09-01T15:00', assignment_time: '15:00', assignment_end_time: '16:00', end_day: 1, }); // assignment_participants - const newParticipants = testDb.prepare('SELECT user_id FROM assignment_participants WHERE assignment_id = ?').all(newAssignment.id) as any[]; + const newParticipants = testDb.prepare('SELECT user_id FROM assignment_participants WHERE assignment_id = ?').all(newAssignment.id) as { user_id: number }[]; expect(newParticipants.map(p => p.user_id).sort((a, b) => a - b)).toEqual([owner.id, member.id].sort((a, b) => a - b)); // roadtrip_vias / roadtrip_day_tracks - const newVia = testDb.prepare('SELECT after_order_index, sequence, lat, lng FROM roadtrip_vias WHERE day_id = ?').get(newDays[0].id) as any; + const newVia = testDb.prepare('SELECT after_order_index, sequence, lat, lng FROM roadtrip_vias WHERE day_id = ?').get(newDays[0].id) as { + after_order_index: number; sequence: number; lat: number; lng: number; + }; expect(newVia).toEqual({ after_order_index: 0, sequence: 0, lat: 48.1, lng: 2.1 }); - const newTrackRow = testDb.prepare('SELECT place_id, stray_km FROM roadtrip_day_tracks WHERE day_id = ?').get(newDays[0].id) as any; + const newTrackRow = testDb.prepare('SELECT place_id, stray_km FROM roadtrip_day_tracks WHERE day_id = ?').get(newDays[0].id) as { place_id: number; stray_km: number | null }; expect(newTrackRow).toEqual({ place_id: newTrack.id, stray_km: 2.5 }); // roadtrip_preferences / roadtrip_day_boundaries - expect((testDb.prepare("SELECT value FROM roadtrip_preferences WHERE trip_id = ? AND key = 'avoid_tolls'").get(newTripId) as any).value).toBe('true'); - const newBoundary = testDb.prepare('SELECT day_number, from_assignment_id, to_assignment_id, fraction FROM roadtrip_day_boundaries WHERE trip_id = ?').get(newTripId) as any; + expect((testDb.prepare("SELECT value FROM roadtrip_preferences WHERE trip_id = ? AND key = 'avoid_tolls'").get(newTripId) as { value: string }).value).toBe('true'); + const newBoundary = testDb.prepare('SELECT day_number, from_assignment_id, to_assignment_id, fraction FROM roadtrip_day_boundaries WHERE trip_id = ?').get(newTripId) as { + day_number: number; from_assignment_id: number; to_assignment_id: number | null; fraction: number; + }; expect(newBoundary).toEqual({ day_number: 1, from_assignment_id: newAssignment.id, to_assignment_id: null, fraction: 0.5 }); // day_accommodations, and the assignment's accommodation_id stamped at the NEW accommodation - const newAccom = testDb.prepare('SELECT id, place_id, start_day_id, end_day_id, check_in, check_out FROM day_accommodations WHERE trip_id = ?').get(newTripId) as any; + const newAccom = testDb.prepare('SELECT id, place_id, start_day_id, end_day_id, check_in, check_out FROM day_accommodations WHERE trip_id = ?').get(newTripId) as { + id: number; place_id: number | null; start_day_id: number; end_day_id: number; check_in: string | null; check_out: string | null; + }; expect(newAccom).toMatchObject({ place_id: newStop.id, start_day_id: newDays[0].id, end_day_id: newDays[1].id, check_in: '15:00', check_out: '11:00' }); expect(newAssignmentFull.accommodation_id).toBe(newAccom.id); // reservations - const newRes = testDb.prepare('SELECT * FROM reservations WHERE trip_id = ?').get(newTripId) as any; + const newRes = testDb.prepare('SELECT * FROM reservations WHERE trip_id = ?').get(newTripId) as { + id: number; day_id: number | null; end_day_id: number | null; place_id: number | null; assignment_id: number | null; + title: string; status: string | null; type: string | null; ingest_state: string; accommodation_id: string | null; + }; expect(newRes).toMatchObject({ day_id: newDays[0].id, end_day_id: newDays[1].id, place_id: newStop.id, assignment_id: newAssignment.id, title: 'Stay', status: 'confirmed', type: 'hotel', ingest_state: 'live', @@ -1416,29 +1432,37 @@ describe('copy — whole-trip parity (Task 8)', () => { expect(Number(newRes.accommodation_id)).toBe(newAccom.id); // budget_items / members / payers / category order - const newItem = testDb.prepare('SELECT * FROM budget_items WHERE trip_id = ?').get(newTripId) as any; + const newItem = testDb.prepare('SELECT * FROM budget_items WHERE trip_id = ?').get(newTripId) as { + id: number; category: string; name: string; total_price: number; reservation_id: number | null; currency: string | null; + }; expect(newItem).toMatchObject({ category: 'Accommodation', name: 'Hotel', total_price: 300, reservation_id: newRes.id, currency: 'EUR' }); - const newMembers = testDb.prepare('SELECT user_id, paid, amount FROM budget_item_members WHERE budget_item_id = ? ORDER BY user_id').all(newItem.id) as any[]; + const newMembers = testDb.prepare('SELECT user_id, paid, amount FROM budget_item_members WHERE budget_item_id = ? ORDER BY user_id').all(newItem.id) as { user_id: number; paid: number; amount: number | null }[]; expect(newMembers).toEqual([owner.id, member.id].sort((a, b) => a - b).map((uid) => uid === owner.id ? { user_id: owner.id, paid: 1, amount: 150 } : { user_id: member.id, paid: 0, amount: 150 }, )); - const newPayers = testDb.prepare('SELECT user_id, amount FROM budget_item_payers WHERE budget_item_id = ?').all(newItem.id) as any[]; + const newPayers = testDb.prepare('SELECT user_id, amount FROM budget_item_payers WHERE budget_item_id = ?').all(newItem.id) as { user_id: number; amount: number }[]; expect(newPayers).toEqual([{ user_id: owner.id, amount: 300 }]); - expect((testDb.prepare("SELECT sort_order FROM budget_category_order WHERE trip_id = ? AND category = 'Accommodation'").get(newTripId) as any).sort_order).toBe(1); + expect((testDb.prepare("SELECT sort_order FROM budget_category_order WHERE trip_id = ? AND category = 'Accommodation'").get(newTripId) as { sort_order: number }).sort_order).toBe(1); // packing_bags / packing_items (incl. the TP68 privacy filter — the copIER is the owner) - const newBag = testDb.prepare("SELECT id FROM packing_bags WHERE trip_id = ? AND name = 'Carry-on'").get(newTripId) as any; - const newPacking = testDb.prepare('SELECT name, checked, is_private, owner_id, bag_id FROM packing_items WHERE trip_id = ? ORDER BY name').all(newTripId) as any[]; + const newBag = testDb.prepare("SELECT id FROM packing_bags WHERE trip_id = ? AND name = 'Carry-on'").get(newTripId) as { id: number }; + const newPacking = testDb.prepare('SELECT name, checked, is_private, owner_id, bag_id FROM packing_items WHERE trip_id = ? ORDER BY name').all(newTripId) as { + name: string; checked: number | null; is_private: number; owner_id: number | null; bag_id: number | null; + }[]; expect(newPacking.map(p => p.name)).toEqual(['Owner\'s diary', 'Shared tent']); // member's private item is NOT copied expect(newPacking.find(p => p.name === 'Shared tent')).toMatchObject({ checked: 0, is_private: 0, owner_id: null, bag_id: newBag.id }); expect(newPacking.find(p => p.name === "Owner's diary")).toMatchObject({ checked: 0, is_private: 1, owner_id: owner.id }); // day_notes - const newNote = testDb.prepare('SELECT day_id, text, time, icon, sort_order FROM day_notes WHERE trip_id = ?').get(newTripId) as any; + const newNote = testDb.prepare('SELECT day_id, text, time, icon, sort_order FROM day_notes WHERE trip_id = ?').get(newTripId) as { + day_id: number; text: string; time: string | null; icon: string | null; sort_order: number | null; + }; expect(newNote).toEqual({ day_id: newDays[0].id, text: 'Remember passport', time: '08:00', icon: '🛂', sort_order: 1 }); // todo_items — reset to unchecked, no assignee - const newTodo = testDb.prepare('SELECT name, checked, category, sort_order, assigned_user_id FROM todo_items WHERE trip_id = ?').get(newTripId) as any; + const newTodo = testDb.prepare('SELECT name, checked, category, sort_order, assigned_user_id FROM todo_items WHERE trip_id = ?').get(newTripId) as { + name: string; checked: number | null; category: string | null; sort_order: number | null; assigned_user_id: number | null; + }; expect(newTodo).toEqual({ name: 'Book taxi', checked: 0, category: 'travel', sort_order: 1, assigned_user_id: null }); // The source trip is untouched. diff --git a/server/tests/unit/services/conflictUpdate.test.ts b/server/tests/unit/services/conflictUpdate.test.ts index c8d235d24..fca52f80a 100644 --- a/server/tests/unit/services/conflictUpdate.test.ts +++ b/server/tests/unit/services/conflictUpdate.test.ts @@ -66,6 +66,7 @@ import { createTestTripMembersRepo, createTestDayAssignmentsRepo, createTestCategoriesRepo, + createTestTripsRepo, sharedTestOrm, } from '../../helpers/test-uow'; @@ -114,6 +115,7 @@ beforeAll(async () => { await createTestTripMembersRepo(dbs.connection), await createTestDayAssignmentsRepo(dbs.connection), await createTestCategoriesRepo(dbs.connection), + await createTestTripsRepo(dbs.connection), ); }); diff --git a/server/tests/websocket/connection.test.ts b/server/tests/websocket/connection.test.ts index 4d7fc26bb..ede3f801f 100644 --- a/server/tests/websocket/connection.test.ts +++ b/server/tests/websocket/connection.test.ts @@ -238,6 +238,36 @@ describe('WS rooms', () => { } }); + /** + * Plan 3c Task 9 fix wave (A-H1, live regression): before the + * `Number.isFinite` guard on `realtime.gateway.ts::handleJoin`, a + * non-numeric `tripId` became `NaN`, which used to reach + * `TripsRepository.findAccessible`'s raw bind as the unquoted bareword + * `NaN` and throw `SqliteError: no such column: NaN` — the promise this + * handler returns never resolved, so the client got NO reply at all + * (confirmed on the compiled dual boot: 4 `[ERROR]` lines, HEAD only). + * The platform's own `NaN` -> `NULL` rendering (`NulSafeSqlitePlatform`) + * fixes the crash even without this guard; this test pins the WS-level + * contract (the SAME `Access denied` frame WS-005 gets for a real trip + * with no access), not just "does not throw". + */ + it('WS-005b — join with a non-numeric tripId receives the same error frame as no access (rule 22 / A-H1)', async () => { + const { user } = createUser(testDb); + const token = createEphemeralToken(user.id, 'ws')!; + + const client = await connectWs(token); + try { + await client.next(); // welcome + + client.send({ type: 'join', tripId: 'abc' }); + const msg = await client.next(); + expect(msg.type).toBe('error'); + expect(msg.message).toMatch(/access denied/i); + } finally { + client.close(); + } + }); + /** * Plan 3c Task 0b (R9): `handleJoin`'s `this.db.canAccessTrip(...)` call * (`realtime.gateway.ts:173`) is now `TripsRepository.findAccessible`