fix(server): plan 3c close-out — non-finite ids bound as NULL, trip id parsed once at every places/days/ws gate

Platform: NulSafeSqlitePlatform now renders a bound NaN as NULL and
+/-Infinity as 9e999/-9e999, matching better-sqlite3's own legacy binding
(rule 22 extension) — the program-wide fail-safe for any caller that still
passes an unguarded Number(untrusted) into a converted statement.

PlacesService.verifyTripAccess and RealtimeGateway.handleJoin stop doing
that: verifyTripAccess parses once with toRowId and threads the parsed id
through create/import/bulk/rating/export.gpx (deleting the seven
toRowId(tripId) ?? -1 sites), and handleJoin gets a Number.isFinite guard.
DaysService gets the same treatment across list/create/getDay/reorder/
insert/resyncAccommodationDays, closing the gap where a hex-spelled trip id
could renumber a real trip's days while the raw-bind inverted-accommodation
guard silently matched nothing.

DatabaseService.getTripTitle is deleted; PlacesService injects
TripsRepository directly and calls getTitle itself.

Template/cheap-item cleanup: DayAssignmentsRepository.listForTimeSort uses
$castTo instead of an unknown bridge; the duplicated TagRow interface in
Places.repository.ts is now imported from Tags.repository.ts;
AssignmentWithPlaceRow extends the guarded DayAssignmentRow instead of
restating its columns; PlaceShadowPicksRepository.totals drops its
unreachable ??-fallback branch; Places.repository.ts's updatePlace types
the NOT NULL name column as string; trip-members.service.ts's
transferOwnership threads one parsed trip id through all three writes and
corrects two stale "raw-bind seam" docstrings that were actually Number()
seams; a dead canAccessTrip mock is removed from reservations.e2e.test.ts.

Tests: the 23 new no-explicit-any warnings in TRIP-SVC-069 are typed
(lint back to 2416); HTTP and WS regression coverage for the non-finite
and non-canonical id shapes across places/days routes, mutation-proved;
TRIP-JOIN-005 asserts the exact concurrent-join settlement instead of
>= 1; a new tests/unit/db/transaction-yield.test.ts pins rule 24's
measured transaction-yield/dirty-read/nested-BEGIN behaviour.
This commit is contained in:
jubnl
2026-09-23 12:22:36 +02:00
parent 46a3246e82
commit 175eb3b11b
25 changed files with 996 additions and 205 deletions
+46 -6
View File
@@ -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);
}
}
@@ -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<DayAssignments> {
.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<DayStopRow>()
.execute();
return rows as unknown as DayStopRow[];
return rows;
}
/** AS24 — `UPDATE day_assignments SET end_day = ? WHERE id = ?`. */
@@ -73,10 +73,16 @@ export class PlaceShadowPicksRepository extends TrekRepository<PlaceShadowPicks>
*/
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 };
}
/**
+11 -13
View File
@@ -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<PlaceRow, Places> = 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<TagRow, Tags> = 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<Places> {
* 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;
@@ -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<string | null> {
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
+83 -22
View File
@@ -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<Day>(...)!`). `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<Day>(...)!`).
*
* `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<number, string | null>,
newDateById: Map<number, string | null>,
): Promise<void> {
@@ -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<void> {
private async assertNoInvertedAccommodation(tripId: number): Promise<void> {
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<number, string | null>,
): Promise<void> {
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]);
+85 -59
View File
@@ -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<boolean> {
@@ -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) {
+9 -1
View File
@@ -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);
@@ -152,9 +152,12 @@ export class TripMembersService {
// `Number(tripId)`, not the raw string: `addMember`'s INSERT needs the
// real `Primary<Trips>` 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<Trips>` 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<Trips>`) 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);
+18 -2
View File
@@ -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);
}
});
}
+5 -3
View File
@@ -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();
});
@@ -224,6 +224,7 @@ export async function createMcpTestRegistry(): Promise<McpRegistry> {
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));
+1
View File
@@ -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));
+130 -1
View File
@@ -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 });
});
});
+154 -10
View File
@@ -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
@@ -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]);
});
});
});
@@ -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);
@@ -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<void>((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');
});
});
@@ -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<Pla
await createTestTripMembersRepo(dbs.connection),
await createTestDayAssignmentsRepo(dbs.connection),
await createTestCategoriesRepo(dbs.connection),
await createTestTripsRepo(dbs.connection),
);
}
@@ -78,7 +78,7 @@ import { QueryHelpersService } from '../../../src/nest/query-helpers/query-helpe
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';
import type { AppSettingsRepository } from '../../../src/db/repositories/AppSettings.repository';
import type { UsersRepository } from '../../../src/db/repositories/Users.repository';
import { isUpdateConflict, type UpdateConflict } from '../../../src/nest/common/conflictResult';
@@ -141,6 +141,7 @@ async function makePlacesService(
await createTestTripMembersRepo(dbs.connection),
await createTestDayAssignmentsRepo(dbs.connection),
await createTestCategoriesRepo(dbs.connection),
await createTestTripsRepo(dbs.connection),
);
}
@@ -155,7 +156,9 @@ beforeAll(async () => {
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();
@@ -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.
@@ -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));
});
+47 -23
View File
@@ -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<string, unknown>;
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.
@@ -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),
);
});
+30
View File
@@ -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`