diff --git a/server/src/db/repositories/Trips.repository.ts b/server/src/db/repositories/Trips.repository.ts index f4e1d6e69..aa494d9ec 100644 --- a/server/src/db/repositories/Trips.repository.ts +++ b/server/src/db/repositories/Trips.repository.ts @@ -2,6 +2,7 @@ import type { Trips } from '../entities/Trips.entity'; import { coalesceParam, currentTimestamp, nowDateOffset } from '../dialect/sql-functions'; import type { AssertRowKeys } from './_shared/rows'; import { TrekRepository } from './_shared/trek-repository'; +import { tripAccessExpr } from './_shared/trip-access'; /** What a trip-scoped request learns about the trip once access is verified. */ export interface TripAccess { @@ -405,7 +406,7 @@ export class TripsRepository extends TrekRepository { const row = await this.tripSelectQuery(user_id) .leftJoin('trip_members as m', (join) => join.onRef('m.trip_id', '=', 't.id').on('m.user_id', '=', user_id)) .where('t.id', '=', trip_id) - .where((eb) => eb.or([eb('t.user_id', '=', user_id), eb('m.user_id', 'is not', null)])) + .where((eb) => tripAccessExpr(eb, 't.user_id', 'm.user_id', user_id)) .executeTakeFirst(); return row as TripSelectRow | undefined; } @@ -419,7 +420,7 @@ export class TripsRepository extends TrekRepository { async listForUser(user_id: number, archived: number | null): Promise { let query = this.tripSelectQuery(user_id) .leftJoin('trip_members as m', (join) => join.onRef('m.trip_id', '=', 't.id').on('m.user_id', '=', user_id)) - .where((eb) => eb.or([eb('t.user_id', '=', user_id), eb('m.user_id', 'is not', null)])); + .where((eb) => tripAccessExpr(eb, 't.user_id', 'm.user_id', user_id)); if (archived !== null) query = query.where('t.is_archived', '=', archived); const rows = await query.orderBy('t.created_at', 'desc').execute(); return rows as TripSelectRow[]; @@ -459,7 +460,7 @@ export class TripsRepository extends TrekRepository { .end() .as('relevance'), ]) - .where((eb) => eb.or([eb('t.user_id', '=', user_id), eb('m.user_id', 'is not', null)])) + .where((eb) => tripAccessExpr(eb, 't.user_id', 'm.user_id', user_id)) .where('t.is_archived', '=', 0) .orderBy((eb) => eb.case() diff --git a/server/src/db/repositories/_shared/trip-access.ts b/server/src/db/repositories/_shared/trip-access.ts new file mode 100644 index 000000000..7dcd661e0 --- /dev/null +++ b/server/src/db/repositories/_shared/trip-access.ts @@ -0,0 +1,41 @@ +import type { ExpressionBuilder, ExpressionWrapper, ReferenceExpression, SqlBool } from 'kysely'; + +/** + * The trip-access predicate — "can `userId` see this trip?" — as ONE Kysely + * boolean expression, generic over the caller's own `` (the same + * fully-generic shape `reservation-visibility.ts`'s `publicReservationExpr` + * uses, for the identical reason: this predicate has no correlated + * subquery, so nothing forces the narrower fixed-alias shape + * `publicStayExists` needs). + * + * Byte-for-byte the SAME boolean `Trips.repository.ts`'s MikroORM + * QueryBuilder helper `accessibleTripsQuery` renders (`findAccessible`'s own + * canonical shape, per Plan 4 Task 8a): `t.user_id = ? OR m.user_id IS NOT + * NULL`, where `m` is a `LEFT JOIN trip_members ON m.trip_id = t.id AND + * m.user_id = ?` the caller has already built. This file does not (and + * cannot, per `tripSelectQuery`'s own docstring on why a shared join helper + * can't be typed across differently-joined outer queries) unify the JOIN + * itself — only the WHERE boolean that follows it, which is the part that + * was actually duplicated three times by hand in this program (Task 7 + * security review M1 / TRIPREPO-041): `TripsRepository.findForViewer`, + * `.listForUser` and `.activeTrip` each re-typed the identical + * `eb.or([eb('t.user_id', '=', user_id), eb('m.user_id', 'is not', null)])` + * independently. Proven identical to the MikroORM form and across all three + * Kysely call sites by `TRIPREPO-041`'s cross-method access-parity test, + * which stays green as this refactor's own regression guard (its own + * comment already said a future unification pass should). + * + * `tripUserIdRef`/`memberUserIdRef` are the caller's own already-aliased + * column references (`'t.user_id'`, `'m.user_id'` at every known call site + * today) — passed in, not hard-coded, so this helper stays usable if a + * future caller joins under different aliases, the same reasoning + * `publicReservationExpr`'s own `ref` parameter gives. + */ +export function tripAccessExpr( + eb: ExpressionBuilder, + tripUserIdRef: ReferenceExpression, + memberUserIdRef: ReferenceExpression, + userId: number, +): ExpressionWrapper { + return eb.or([eb(tripUserIdRef, '=', userId), eb(memberUserIdRef, 'is not', null)]); +} diff --git a/server/tests/unit/db/repositories/Trips.repository.test.ts b/server/tests/unit/db/repositories/Trips.repository.test.ts index 2056d1d5f..a419fd33f 100644 --- a/server/tests/unit/db/repositories/Trips.repository.test.ts +++ b/server/tests/unit/db/repositories/Trips.repository.test.ts @@ -564,18 +564,27 @@ describe('TripsRepository.insertTripCopy (TP37)', () => { // ── Task 7 security review M1, absorbed here (Task 8 touches the same file) ── // -// The trip access predicate (`t.user_id = ? OR EXISTS a trip_members row`) is +// The trip access predicate (`t.user_id = ? OR EXISTS a trip_members row`) was // written out FOUR times in this file: once as the shared QB helper // `accessibleTripsQuery` (`findAccessible`/`listAccessibleIds`), and three // more times by hand in the Kysely methods (`findForViewer`, `listForUser`, -// `activeTrip`) — a different builder API that cannot share the QB helper. -// A single Kysely `.$call()` helper for the latter three was evaluated and -// set aside for this task (real typing risk across three different `DB` -// shapes on a security-sensitive predicate, under this task's own time -// budget) in favor of the review's own stated fallback: a cross-method -// parity test proving all five agree on who can see a trip. A future task -// unifying the three Kysely copies should keep this test green as its own -// regression guard. +// `activeTrip`) — a different builder API that cannot share the QB helper. A +// single Kysely `.$call()` helper for the latter three was evaluated and set +// aside by Task 7 (real typing risk across three different `DB` shapes on a +// security-sensitive predicate, under that task's own time budget) in favor +// of the review's own stated fallback: this cross-method parity test proving +// all five agree on who can see a trip. +// +// Plan 4 Task 8a unified the three Kysely copies onto ONE shared boolean +// expression, `tripAccessExpr` (`_shared/trip-access.ts`) — fully generic +// over `` (no correlated subquery forces the narrower fixed-alias +// shape `reservation-visibility.ts`'s `publicStayExists` needs, so it +// composes across `TripSelectKyselyDB` and `ActiveTripKyselyDB` without a +// `.$call()` helper). The LEFT JOIN itself stays hand-written at each call +// site (unchanged, still three copies) — `tripSelectQuery`'s own docstring +// records why a shared join helper can't be typed across these +// differently-joined outer queries. This test stays green as the +// unification's own regression guard, exactly as it always was. describe('Cross-method access parity (Task 7 security review M1, absorbed)', () => { it('TRIPREPO-041: owner, member, stranger and an admin with no membership get the identical accessible/inaccessible verdict from all five access-checking methods', async () => { const { user: owner } = createUser(testDb);