From e62f463296695cb263fb357a776700fd404d4750 Mon Sep 17 00:00:00 2001 From: jubnl Date: Thu, 24 Sep 2026 06:05:30 +0200 Subject: [PATCH] =?UTF-8?q?refactor(server):=20plan=204=20=E2=80=94=20unif?= =?UTF-8?q?y=20the=20trip-access=20predicate's=20Kysely=20copies?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TripsRepository.findForViewer/listForUser/activeTrip each hand-wrote the identical trip-access boolean (t.user_id = ? OR m.user_id IS NOT NULL) through the Kysely expression builder. Task 7's security review (M1) evaluated a shared helper and set it aside for time, falling back to the TRIPREPO-041 cross-method parity test as a regression guard instead. Extracted the boolean into one generic helper, tripAccessExpr (_shared/trip-access.ts), following the reservation-visibility.ts publicReservationExpr precedent: fully generic over since the predicate has no correlated subquery, so it composes across TripSelectKyselyDB and ActiveTripKyselyDB without needing a narrower fixed-alias shape. The LEFT JOIN itself stays hand-written at each call site — tripSelectQuery's own docstring already explains why a shared join helper can't be typed across these differently-joined outer queries. feeds.service.ts and memories-access.service.ts no longer duplicate this predicate at all (Task 2's facade-inline sweep already converted both onto TripsRepository.findAccessible/getFeedTokenIfReachable/ listReachableActiveTrips, which read through the canonical MikroORM QueryBuilder form, accessibleTripsQuery) — the remaining duplication this task closes is the three Kysely copies inside this file. TRIPREPO-041 (64/64 in the file) stays green as the unification's own regression guard. --- .../src/db/repositories/Trips.repository.ts | 7 ++-- .../db/repositories/_shared/trip-access.ts | 41 +++++++++++++++++++ .../db/repositories/Trips.repository.test.ts | 27 ++++++++---- 3 files changed, 63 insertions(+), 12 deletions(-) create mode 100644 server/src/db/repositories/_shared/trip-access.ts 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);