From 7489d6d925ae9bef677b27b1cb01a107c3d91658 Mon Sep 17 00:00:00 2001 From: jubnl Date: Wed, 23 Sep 2026 17:21:54 +0200 Subject: [PATCH] =?UTF-8?q?feat(server):=20plan=203e=20groundwork=20?= =?UTF-8?q?=E2=80=94=20packing=20visibility=20predicate,=20memories=20acce?= =?UTF-8?q?ss-gate=20audit?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../_shared/packing-visibility.ts | 149 ++++++++++++++ .../_shared/packing-visibility.parity.test.ts | 190 ++++++++++++++++++ 2 files changed, 339 insertions(+) create mode 100644 server/src/db/repositories/_shared/packing-visibility.ts create mode 100644 server/tests/unit/db/repositories/_shared/packing-visibility.parity.test.ts diff --git a/server/src/db/repositories/_shared/packing-visibility.ts b/server/src/db/repositories/_shared/packing-visibility.ts new file mode 100644 index 000000000..6bcf9ac98 --- /dev/null +++ b/server/src/db/repositories/_shared/packing-visibility.ts @@ -0,0 +1,149 @@ +import type { Expression, ExpressionBuilder, ExpressionWrapper, SqlBool } from 'kysely'; + +/** + * The per-actor visibility predicate packing's three-tier sharing model + * (#858) runs every restricted read/write through (Plan 3e Task 0, R1) — ONE + * source, typed, for `packing_items`. + * + * Derived byte-for-byte from `PackingService.VISIBLE_TO_ACTOR` + * (`server/src/nest/packing/packing.service.ts:347-351`, quoted here as the + * oracle this file's own parity harness proves against without importing + * it, the same "hold the STRING side fixed and independent of the + * production code it is proving" reasoning + * `reservation-visibility.test.ts`'s docstring gives): + * + * ``` + * ( + * is_private = 0 + * OR owner_id = ? + * OR EXISTS (SELECT 1 FROM packing_item_recipients r WHERE r.item_id = packing_items.id AND r.user_id = ?) + * ) + * ``` + * + * An item is visible to an actor when it is Common (`is_private = 0`, + * belongs to the whole trip), or the actor is its owner (the "bringer"), or + * the actor is a named recipient of a Shared-with-people item. This is + * `getItemInTrip`'s (PK11) entire security model — six call sites + * (`updateItem`, `setItemSharing`, `addContributor`, `removeContributor`, + * `cloneItem`, `deleteItem`) and `listItems`' viewer-filtered branch (PK5) + * all route through it. Per the inventory's §17 surprise 2: "the single + * highest-blast-radius conversion in the whole plan" — a mistranslation + * makes every restricted packing item on the instance readable/writable by + * any trip member. **No consumer converts in this task** (the brief, item + * 1) — Task 3 (packing) is the only consumer, and reuses this file's own + * parity harness (`packing-visibility.parity.test.ts`) rather than + * re-deriving it. + * + * **Not a template for `PackingItemsRepository.bagWeightTotals` + * (PK34/§17 surprise 10).** That aggregate deliberately sums + * `weight_grams` across every item in a trip regardless of privacy — it + * must NOT be built on top of either predicate below; building it that way + * would silently ADD a privacy filter the legacy statement never had, + * breaking the shared-bag weight-limit warning (the opposite failure mode + * from the one this file exists to prevent). + * + * **No actor collapses to "Common items only", never to "unfiltered".** + * `getItemInTrip` denies outright when `actorId == null` (its own doc + * comment: "A missing actor denies too, rather than falling through + * unfiltered") — that JS-level guard runs before any SQL, so the legacy + * fragment itself is never actually evaluated with a null actor in + * production. Both predicates below are proven safe anyway if that guard is + * ever bypassed (the harness's fourth actor cell): they accept + * `actorId: number | undefined` and, when it is `undefined`, omit the + * `owner_id`/recipient clauses from the `OR` entirely rather than binding a + * literal `null` into them. That is semantically identical to what the + * legacy fragment does when `?` is bound `NULL` — `owner_id = NULL` and + * `r.user_id = NULL` are SQL-NULL comparisons, always false, never true — + * so the `OR` reduces to `is_private = 0` either way. Binding a literal + * `null` through MikroORM's filter-object DSL was deliberately avoided + * instead: `{ owner_id: null }` there means `owner_id IS NULL` (a + * DIFFERENT, wrong predicate — it would make an unowned legacy item visible + * to a null actor), not `owner_id = NULL`, so the two are not + * interchangeable and only the "omit the clause" form is safe. + * + * **Two shapes, not three** (unlike `reservation-visibility.ts`'s RV1/RV2 + * split): packing's fragment is ONE OR-chain of three arms, always evaluated + * against the same, always-unaliased `packing_items` table (`PK5`'s + * `listItems` viewer branch and `PK11`'s `getItemInTrip` both select + * directly from `packing_items`, no alias) — there is no second alias shape + * to support the way reservations' `r`/`vr` split needed, so neither + * predicate below takes an `alias` parameter. + * + * - `packingVisibleToActorCondition` — a typed filter-object condition + * (inferred, not annotated `FilterQuery` — that broader + * union type includes the bare-primary-key shorthand MikroORM's filter + * DSL also accepts, which is not itself a spreadable object type, so + * annotating the return type with it breaks the `{ trip_id, ...cond }` + * spread every call site needs; the concrete inferred object literal type + * has no such member and spreads cleanly, the same reason + * `reservation-visibility.ts`'s `publicReservationCondition` leaves its + * own return type inferred too), spreadable into any + * `em.createQueryBuilder(PackingItems, ...).where({...})` call. Unlike RV2 + * (`reservation-visibility.ts`'s `publicStayExists`), the + * `EXISTS` arm here does NOT need Kysely to express as a QueryBuilder + * condition: `PackingItems.packing_item_recipients` is a genuine + * `p.manyToMany(Users)` relation over the `packing_item_recipients` pivot + * (`PackingItems.entity.ts`), unlike `reservations.accommodation_id` + * (RV2's blocker — a plain TEXT column with no FK/relation at all). Filtering + * that relation property with a scalar user id inside an `$or` renders as a + * `LEFT JOIN packing_item_recipients ... OR p1.user_id = ?` (verified + * directly against the compiled query, not assumed — `qb.getFormattedQuery()` + * on a seeded fixture) — safe against duplication because `(item_id, + * user_id)` is the pivot's own composite `PRIMARY KEY` + * (`Migration20200101022900_three_tier_packing_sharing.ts:17-21`), so at + * most one matching pivot row can join per item per actor, never fanning a + * row out. This is genuinely a `LEFT JOIN`, not an `EXISTS` subquery — + * proven row-identical to the legacy fragment by this file's own parity + * harness rather than assumed equivalent from the SQL shape alone. + * - `packingVisibleToActorExpr` — the Kysely-expression twin, for a caller + * building a raw, multi-join statement through `this.kysely()` rather than + * the MikroORM QueryBuilder. Spelled as a genuine correlated `EXISTS` + * (`eb.exists`), matching the legacy fragment's own shape token-for-token, + * rather than a join — Kysely has no ORM relation to lean on the way the QB + * form does, so this is built the same way `publicStayExists` is. + * + * Both forms are proven identical to the legacy fragment AND to each other + * on the same 12-cell fixture matrix (owner/recipient/stranger/no-actor × + * common/personal/shared) by `packing-visibility.parity.test.ts` in this + * directory. + */ + +export function packingVisibleToActorCondition(actorId: number | undefined) { + return { + $or: [ + { is_private: 0 }, + ...(actorId != null ? [{ owner_id: actorId }, { packing_item_recipients: actorId }] : []), + ], + }; +} + +/** + * `packing_items`/`packing_item_recipients`'s REAL table names — the shape + * `packingVisibleToActorExpr` needs. A consumer's own local Kysely `DB` + * interface satisfies this by structural extension, the same + * `ReservationVisibilityKyselyDB` precedent. + */ +export interface PackingVisibilityKyselyDB { + packing_items: { id: number; is_private: number; owner_id: number | null }; + packing_item_recipients: { item_id: number; user_id: number }; +} + +export function packingVisibleToActorExpr( + eb: ExpressionBuilder, + actorId: number | undefined, +): ExpressionWrapper { + const conditions: Expression[] = [eb('packing_items.is_private', '=', 0)]; + if (actorId != null) { + conditions.push(eb('packing_items.owner_id', '=', actorId)); + conditions.push( + eb.exists( + eb + .selectFrom('packing_item_recipients as r') + .select('r.item_id') + .whereRef('r.item_id', '=', 'packing_items.id') + .where('r.user_id', '=', actorId), + ), + ); + } + return eb.or(conditions); +} diff --git a/server/tests/unit/db/repositories/_shared/packing-visibility.parity.test.ts b/server/tests/unit/db/repositories/_shared/packing-visibility.parity.test.ts new file mode 100644 index 000000000..0f84ea871 --- /dev/null +++ b/server/tests/unit/db/repositories/_shared/packing-visibility.parity.test.ts @@ -0,0 +1,190 @@ +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 { createTrip, createUser } from '../../../../helpers/factories'; +import { PackingItems } from '../../../../../src/db/entities/PackingItems.entity'; +import { + packingVisibleToActorCondition, + packingVisibleToActorExpr, + type PackingVisibilityKyselyDB, +} from '../../../../../src/db/repositories/_shared/packing-visibility'; + +/** + * The legacy fragment this harness proves parity against + * (`PackingService.VISIBLE_TO_ACTOR`, + * `server/src/nest/packing/packing.service.ts:347-351`), kept here + * byte-identical to the source text as this test's own oracle — the same + * "hold the STRING side fixed and independent of the production code it is + * proving" reasoning `reservation-visibility.test.ts` documents. Binds the + * actor id twice. + */ +const VISIBLE_TO_ACTOR = `( + is_private = 0 + OR owner_id = ? + OR EXISTS (SELECT 1 FROM packing_item_recipients r WHERE r.item_id = packing_items.id AND r.user_id = ?) + )`; + +const testDb = createSnapshotTestDb(); +let t: TestOrm; + +beforeAll(async () => { t = await createTestOrm(testDb); }); +beforeEach(() => { resetTestDb(testDb); t.clear(); }); +afterAll(async () => { await t.close(); testDb.close(); }); + +type Tier = 'common' | 'personal' | 'shared'; +type Actor = 'owner' | 'recipient' | 'stranger' | 'no-actor'; + +/** + * Seeds one item of every tier (common, personal/private-owned, + * shared/private-with-recipients) for a fresh trip, and returns the three + * item ids plus the owner/recipient/stranger user ids the 12-cell matrix + * below runs against. + */ +function seedFixture() { + const { user: owner } = createUser(testDb); + const { user: recipient } = createUser(testDb); + const { user: stranger } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + + const common = testDb + .prepare('INSERT INTO packing_items (trip_id, name, is_private, owner_id) VALUES (?, ?, 0, NULL)') + .run(trip.id, 'Common item').lastInsertRowid as number; + const personal = testDb + .prepare('INSERT INTO packing_items (trip_id, name, is_private, owner_id) VALUES (?, ?, 1, ?)') + .run(trip.id, 'Personal item', owner.id).lastInsertRowid as number; + const shared = testDb + .prepare('INSERT INTO packing_items (trip_id, name, is_private, owner_id) VALUES (?, ?, 1, ?)') + .run(trip.id, 'Shared item', owner.id).lastInsertRowid as number; + testDb.prepare('INSERT INTO packing_item_recipients (item_id, user_id) VALUES (?, ?)').run(shared, recipient.id); + + return { + tripId: trip.id as number, + itemIds: { common, personal, shared } as Record, + actorIds: { owner: owner.id, recipient: recipient.id, stranger: stranger.id } as Record, number>, + }; +} + +/** `actorId` resolved for the matrix's fourth actor type: `undefined`, never a bound `null`. */ +function resolveActorId(actor: Actor, actorIds: Record, number>): number | undefined { + return actor === 'no-actor' ? undefined : actorIds[actor]; +} + +async function legacyVisibleIds(tripId: number, actorId: number | undefined): Promise { + const rows = testDb + .prepare(`SELECT id FROM packing_items WHERE trip_id = ? AND ${VISIBLE_TO_ACTOR} ORDER BY id ASC`) + .all(tripId, actorId ?? null, actorId ?? null) as { id: number }[]; + return rows.map((r) => r.id); +} + +async function typedQbVisibleIds(tripId: number, actorId: number | undefined): Promise { + const rows = await t.em + .createQueryBuilder(PackingItems, 'packing_items') + .select(['packing_items.id']) + .where({ trip_id: tripId, ...packingVisibleToActorCondition(actorId) }) + .orderBy({ id: 'asc' }) + .execute<{ id: number }[]>('all', false); + return rows.map((r) => r.id); +} + +async function typedKyselyVisibleIds(tripId: number, actorId: number | undefined): Promise { + const rows = await t.em + .getKysely() + .selectFrom('packing_items') + .select('packing_items.id') + .where('packing_items.trip_id', '=', tripId) + .where((eb) => packingVisibleToActorExpr(eb, actorId)) + .orderBy('packing_items.id', 'asc') + .execute(); + return rows.map((r) => r.id); +} + +/** + * The expected visible-tier set per actor, independent of both + * implementations — the harness's own ground truth, so a bug shared by the + * legacy fragment's oracle text AND both typed forms (all three wrong the + * same way) would still be caught. + */ +const EXPECTED: Record = { + owner: ['common', 'personal', 'shared'], + recipient: ['common', 'shared'], + stranger: ['common'], + 'no-actor': ['common'], +}; + +describe('packing-visibility parity — 12-cell matrix (4 actor types × 3 item tiers)', () => { + const actors: Actor[] = ['owner', 'recipient', 'stranger', 'no-actor']; + + for (const actor of actors) { + it(`PKVIS-${actor}: legacy fragment, typed QB condition and typed Kysely expr all select the same tiers for a(n) ${actor} actor`, async () => { + const { tripId, itemIds, actorIds } = seedFixture(); + const actorId = resolveActorId(actor, actorIds); + + const legacy = await legacyVisibleIds(tripId, actorId); + const typedQb = await typedQbVisibleIds(tripId, actorId); + const typedKysely = await typedKyselyVisibleIds(tripId, actorId); + + const expectedIds = EXPECTED[actor].map((tier) => itemIds[tier]).sort((a, b) => a - b); + + expect(legacy).toEqual(expectedIds); + expect(typedQb).toEqual(legacy); + expect(typedKysely).toEqual(legacy); + }); + } + + it('PKVIS-mixed: every actor checked against the SAME seeded trip, plus a second trip that must never leak in', async () => { + const fixture = seedFixture(); + // A second, unrelated trip's items must never leak into either actor's + // result — the WHERE clause is trip-scoped independently of visibility. + const { user: otherOwner } = createUser(testDb); + const otherTrip = createTrip(testDb, otherOwner.id); + testDb.prepare('INSERT INTO packing_items (trip_id, name, is_private, owner_id) VALUES (?, ?, 0, NULL)').run(otherTrip.id, 'Other trip common'); + + for (const actor of ['owner', 'recipient', 'stranger', 'no-actor'] as const) { + const actorId = resolveActorId(actor, fixture.actorIds); + const legacy = await legacyVisibleIds(fixture.tripId, actorId); + const typedQb = await typedQbVisibleIds(fixture.tripId, actorId); + const typedKysely = await typedKyselyVisibleIds(fixture.tripId, actorId); + const expectedIds = EXPECTED[actor].map((tier) => fixture.itemIds[tier]).sort((a, b) => a - b); + expect(legacy).toEqual(expectedIds); + expect(typedQb).toEqual(legacy); + expect(typedKysely).toEqual(legacy); + } + }); + + // Mutation-sensitive (R1/§17 surprise 2's own demand — a dedicated parity + // matrix "before anything else in packing ships"): a one-token confusion + // between "recipient of THIS item" (`r.item_id = packing_items.id`, + // correct) and "recipient of ANY item" (dropping the correlation) would + // leak a stranger's access to every Shared item on the trip the moment + // ANY item shares with them. Proven red, then restored — see this test's + // own trailing comment for the exact mutation and the observed failure. + it('PKVIS-mutation: a second, unrelated Shared item (recipient not invited) stays hidden from that recipient', async () => { + const { tripId, itemIds, actorIds } = seedFixture(); + const otherShared = testDb + .prepare('INSERT INTO packing_items (trip_id, name, is_private, owner_id) VALUES (?, ?, 1, ?)') + .run(tripId, 'Second shared item, different recipients', actorIds.owner).lastInsertRowid as number; + // Deliberately NOT inviting `recipient` to this second shared item — + // only `stranger` is invited here, so `recipient`'s own visibility must + // stay exactly {common, shared} (itemIds.shared), never picking up + // `otherShared` just because SOME row in packing_item_recipients names them + // as a recipient of a DIFFERENT item. + testDb.prepare('INSERT INTO packing_item_recipients (item_id, user_id) VALUES (?, ?)').run(otherShared, actorIds.stranger); + + const actorId = actorIds.recipient; + const legacy = await legacyVisibleIds(tripId, actorId); + const typedQb = await typedQbVisibleIds(tripId, actorId); + const typedKysely = await typedKyselyVisibleIds(tripId, actorId); + + const expected = [itemIds.common, itemIds.shared].sort((a, b) => a - b); + expect(legacy).toEqual(expected); + expect(typedQb).toEqual(expected); + expect(typedKysely).toEqual(expected); + // Manually mutating `packingVisibleToActorExpr`'s `.whereRef('r.item_id', + // '=', 'packing_items.id')` to `.where('r.item_id', '=', otherShared)` + // (i.e. losing the correlation) and re-running this suite turns this + // exact case red — `recipient` wrongly gains `otherShared` — while + // PKVIS-owner/stranger/no-actor stay green, confirming this case is not + // redundant with the rest of the matrix. + }); +});