diff --git a/server/src/nest/packing/packing.controller.ts b/server/src/nest/packing/packing.controller.ts index a1f0bd42d..f0f06287b 100644 --- a/server/src/nest/packing/packing.controller.ts +++ b/server/src/nest/packing/packing.controller.ts @@ -151,7 +151,7 @@ export class PackingController { @Param('id') id: string, @Headers('x-socket-id') socketId?: string, ) { - const deleted = this.packing.deleteItem(tripId, id); + const deleted = this.packing.deleteItem(tripId, id, user.id); if (!deleted) { throw new HttpException({ error: 'Item not found' }, 404); } diff --git a/server/src/nest/packing/packing.mcp.ts b/server/src/nest/packing/packing.mcp.ts index 086f77391..2e713fb68 100644 --- a/server/src/nest/packing/packing.mcp.ts +++ b/server/src/nest/packing/packing.mcp.ts @@ -81,7 +81,7 @@ export class PackingMcp { if (this.auth.isDemoUser(ctx.userId)) return demoDenied(); if (!this.packing.verifyTripAccess(tripId, ctx.userId)) return noAccess(); if (!this.guards.hasTripPermission('packing_edit', tripId, ctx.userId)) return permissionDenied(); - const item = this.packing.updateItem(tripId, itemId, { checked: checked ? 1 : 0 }, ['checked']); + const item = this.packing.updateItem(tripId, itemId, { checked: checked ? 1 : 0 }, ['checked'], undefined, ctx.userId); if (!item) return errorResult('Packing item not found.'); this.guards.safeBroadcast(tripId, 'packing:updated', { item }); return ok({ item }); @@ -102,7 +102,7 @@ export class PackingMcp { if (this.auth.isDemoUser(ctx.userId)) return demoDenied(); if (!this.packing.verifyTripAccess(tripId, ctx.userId)) return noAccess(); if (!this.guards.hasTripPermission('packing_edit', tripId, ctx.userId)) return permissionDenied(); - const deleted = this.packing.deleteItem(tripId, itemId); + const deleted = this.packing.deleteItem(tripId, itemId, ctx.userId); if (!deleted) return errorResult('Packing item not found.'); this.guards.safeBroadcast(tripId, 'packing:deleted', { itemId }); return ok({ success: true }); @@ -128,7 +128,7 @@ export class PackingMcp { if (!this.packing.verifyTripAccess(tripId, ctx.userId)) return noAccess(); if (!this.guards.hasTripPermission('packing_edit', tripId, ctx.userId)) return permissionDenied(); const bodyKeys = ['name', 'category'].filter(k => k === 'name' ? name !== undefined : category !== undefined); - const item = this.packing.updateItem(tripId, itemId, { name, category }, bodyKeys); + const item = this.packing.updateItem(tripId, itemId, { name, category }, bodyKeys, undefined, ctx.userId); if (!item) return errorResult('Packing item not found.'); this.guards.safeBroadcast(tripId, 'packing:updated', { item }); return ok({ item }); diff --git a/server/src/nest/packing/packing.rpc.ts b/server/src/nest/packing/packing.rpc.ts index aa952af43..aef080f71 100644 --- a/server/src/nest/packing/packing.rpc.ts +++ b/server/src/nest/packing/packing.rpc.ts @@ -76,7 +76,7 @@ export class PackingRpc { const itemId = num(params.itemId, 'itemId'); const actor = this.guards.requireActor(ctx, 'packing item'); this.guards.requireTripEdit(tripId, actor, PACKING_EDIT_ACTION); - const deleted = this.packing.deleteItem(String(tripId), String(itemId)) as PrivacyItem | null; + const deleted = this.packing.deleteItem(String(tripId), String(itemId), actor) as PrivacyItem | null; if (!deleted) throw new ForbiddenResource(`no packing item ${itemId} on trip ${tripId}`); this.packing.emitToViewers(String(tripId), 'packing:deleted', { itemId }, deleted, undefined); return { deleted: true }; diff --git a/server/src/nest/packing/packing.service.ts b/server/src/nest/packing/packing.service.ts index c55c04549..1fef2f8a3 100644 --- a/server/src/nest/packing/packing.service.ts +++ b/server/src/nest/packing/packing.service.ts @@ -247,7 +247,9 @@ export class PackingService { ifMatch?: string, actingUserId?: number, ): unknown | UpdateConflict | null { - const item = this.db.get<{ updated_at?: string | null; owner_id?: number | null }>('SELECT * FROM packing_items WHERE id = ? AND trip_id = ?', id, tripId); + // Was a trip-scoped lookup, which let any member with packing_edit write to + // another member's restricted item (and read it back off the response). + const item = this.getItemInTrip(tripId, id, actingUserId); if (!item) return null; // Optimistic concurrency (#1135): reject a stale offline overwrite. Absent @@ -295,10 +297,33 @@ export class PackingService { // ── Three-tier sharing (#858): recipients, contributors, clone ─────────────── - /** Loads an item scoped to its trip (the trip-access check happens in the controller). */ - private getItemInTrip(tripId: string | number, id: string | number) { - return this.db.get<{ id: number; owner_id: number | null; is_private: number; name: string; category: string | null; quantity: number; weight_grams: number | null; bag_id: number | null }>( - 'SELECT * FROM packing_items WHERE id = ? AND trip_id = ?', id, tripId + /** + * The one visibility rule (#858), as a SQL fragment: Common belongs to the whole + * trip, a restricted item belongs to its owner and to the people it was shared + * with. Binds the actor id twice. + */ + private static readonly 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 = ?) + )`; + + /** + * Loads an item scoped to its trip AND to what the actor may see. + * + * Trip membership used to be the whole check here, so any member holding + * packing_edit could reach another member's Personal or Shared item by id + * through update, delete, clone or the contributor routes. It resolves through + * the visibility rule now, and an item the actor may not see comes back + * undefined — deliberately indistinguishable from one that does not exist, so + * these routes cannot be used to probe for ids. A missing actor denies too, + * rather than falling through unfiltered. + */ + private getItemInTrip(tripId: string | number, id: string | number, actorId: number | undefined) { + if (actorId == null) return undefined; + return this.db.get<{ id: number; owner_id: number | null; is_private: number; name: string; category: string | null; quantity: number; weight_grams: number | null; bag_id: number | null; updated_at?: string | null }>( + `SELECT * FROM packing_items WHERE id = ? AND trip_id = ? AND ${PackingService.VISIBLE_TO_ACTOR}`, + id, tripId, actorId, actorId, ); } @@ -313,7 +338,7 @@ export class PackingService { visibility: PackingVisibility, recipientIds: number[], ) { - const item = this.getItemInTrip(tripId, id); + const item = this.getItemInTrip(tripId, id, actingUserId); if (!item) return null; // The owner controls sharing; an unowned legacy item is claimed by the actor. if (item.owner_id != null && item.owner_id !== actingUserId) return { forbidden: true as const }; @@ -335,7 +360,7 @@ export class PackingService { /** "I can bring that too" — adds the user as a co-contributor on a Common item. */ addContributor(tripId: string | number, id: string | number, userId: number) { - const item = this.getItemInTrip(tripId, id); + const item = this.getItemInTrip(tripId, id, userId); if (!item || item.is_private !== 0) return null; // co-contribution is a Common-list concept if (item.owner_id === userId) return null; // the bringer is already covering it this.db.run("INSERT OR IGNORE INTO packing_item_contributors (item_id, user_id, status) VALUES (?, ?, 'accepted')", id, userId); @@ -343,7 +368,7 @@ export class PackingService { } removeContributor(tripId: string | number, id: string | number, userId: number) { - const item = this.getItemInTrip(tripId, id); + const item = this.getItemInTrip(tripId, id, userId); if (!item) return null; this.db.run('DELETE FROM packing_item_contributors WHERE item_id = ? AND user_id = ?', id, userId); return this.enrichItems([this.db.get('SELECT * FROM packing_items WHERE id = ?', id)])[0]; @@ -370,7 +395,7 @@ export class PackingService { * every traveller was the whole complaint in #207. */ cloneItem(tripId: string | number, id: string | number, userId: number) { - const item = this.getItemInTrip(tripId, id); + const item = this.getItemInTrip(tripId, id, userId); if (!item) return null; return this.createItem(tripId, { name: item.name, @@ -382,10 +407,12 @@ export class PackingService { }, userId); } - deleteItem(tripId: string | number, id: string | number) { + deleteItem(tripId: string | number, id: string | number, actingUserId?: number) { // Return the deleted row (not just a boolean) so callers can target the // delete broadcast at the owner when the item was private (#858). - const item = this.db.get<{ is_private?: number; owner_id?: number | null }>('SELECT * FROM packing_items WHERE id = ? AND trip_id = ?', id, tripId); + // Scoped to what the actor may see: trip membership alone used to be enough + // to delete another member's restricted item. + const item = this.getItemInTrip(tripId, id, actingUserId); if (!item) return null; this.db.run('DELETE FROM packing_items WHERE id = ?', id); @@ -577,8 +604,12 @@ export class PackingService { // ── Save as Template ────────────────────────────────────────────────────── saveAsTemplate(tripId: string | number, userId: number, templateName: string) { + // A template is a durable, shareable artifact, so it may only capture what is + // the actor's to publish: the Common list plus their own items. It used to + // take every row in the trip, restricted ones included. const items = this.db.all<{ name: string; category: string }>( - 'SELECT name, category FROM packing_items WHERE trip_id = ? ORDER BY sort_order ASC', tripId + 'SELECT name, category FROM packing_items WHERE trip_id = ? AND (is_private = 0 OR owner_id = ?) ORDER BY sort_order ASC', + tripId, userId, ); if (items.length === 0) return null; diff --git a/server/src/nest/trips/trips.service.ts b/server/src/nest/trips/trips.service.ts index e19f9ee54..032c38b15 100644 --- a/server/src/nest/trips/trips.service.ts +++ b/server/src/nest/trips/trips.service.ts @@ -689,14 +689,24 @@ export class TripsService { bagMap.set(bag.id, r.lastInsertRowid); } - const oldPacking = this.db.prepare('SELECT * FROM packing_items WHERE trip_id = ?').all(sourceTripId) as any[]; + // Only what the copier may carry over: the Common list plus their own items. + // This used to take every row and re-insert it without is_private/owner_id, + // so both fell back to the column defaults and another member's Personal or + // Shared item reappeared in the copy as a Common item visible to everyone. + // A restricted item stays restricted, and it stays owned by the copier — + // recipient rows are not carried over, and the copy has its own roster. + const oldPacking = this.db.prepare( + 'SELECT * FROM packing_items WHERE trip_id = ? AND (is_private = 0 OR owner_id = ?)' + ).all(sourceTripId, newOwnerId) as any[]; const insertPacking = this.db.prepare(` - INSERT INTO packing_items (trip_id, name, checked, category, sort_order, weight_grams, bag_id, updated_at) - VALUES (?, ?, 0, ?, ?, ?, ?, CURRENT_TIMESTAMP) + INSERT INTO packing_items (trip_id, name, checked, category, sort_order, weight_grams, bag_id, is_private, owner_id, updated_at) + VALUES (?, ?, 0, ?, ?, ?, ?, ?, ?, CURRENT_TIMESTAMP) `); for (const p of oldPacking) { + const isPrivate = p.is_private ? 1 : 0; insertPacking.run(newTripId, p.name, p.category, p.sort_order, p.weight_grams, - p.bag_id ? (bagMap.get(p.bag_id) ?? null) : null); + p.bag_id ? (bagMap.get(p.bag_id) ?? null) : null, + isPrivate, isPrivate ? newOwnerId : null); } const oldNotes = this.db.prepare('SELECT * FROM day_notes WHERE trip_id = ?').all(sourceTripId) as any[]; diff --git a/server/tests/integration/packing.test.ts b/server/tests/integration/packing.test.ts index c637aaa8e..e5e561e81 100644 --- a/server/tests/integration/packing.test.ts +++ b/server/tests/integration/packing.test.ts @@ -257,7 +257,14 @@ describe('Three-tier packing sharing (#858)', () => { addTripMember(testDb, trip.id, member.id); const created = await request(app).post(`/api/trips/${trip.id}/packing`).set('Cookie', authCookie(owner.id)).send({ name: 'Tent', visibility: 'personal' }); - const denied = await request(app).put(`/api/trips/${trip.id}/packing/${created.body.item.id}/sharing`).set('Cookie', authCookie(member.id)).send({ visibility: 'common' }); + // A member who cannot see the item at all gets 404, not 403: answering 403 + // would confirm that the id exists (GHSA-vh2h-288v-ggch). + const hidden = await request(app).put(`/api/trips/${trip.id}/packing/${created.body.item.id}/sharing`).set('Cookie', authCookie(member.id)).send({ visibility: 'common' }); + expect(hidden.status).toBe(404); + + // An item the member CAN see but does not own still answers 403. + const shared = await request(app).post(`/api/trips/${trip.id}/packing`).set('Cookie', authCookie(owner.id)).send({ name: 'Stove', visibility: 'common' }); + const denied = await request(app).put(`/api/trips/${trip.id}/packing/${shared.body.item.id}/sharing`).set('Cookie', authCookie(member.id)).send({ visibility: 'personal' }); expect(denied.status).toBe(403); }); }); diff --git a/server/tests/unit/nest/packing.service.test.ts b/server/tests/unit/nest/packing.service.test.ts index 88131ba1a..44571e593 100644 --- a/server/tests/unit/nest/packing.service.test.ts +++ b/server/tests/unit/nest/packing.service.test.ts @@ -474,11 +474,11 @@ describe('private items (#858)', () => { const trip = createTrip(testDb, user.id); const item = svc.createItem(trip.id, { name: 'Private', is_private: true }, user.id) as any; - const deleted = svc.deleteItem(trip.id, item.id) as any; + const deleted = svc.deleteItem(trip.id, item.id, user.id) as any; expect(deleted).not.toBeNull(); expect(deleted.is_private).toBe(1); expect(deleted.owner_id).toBe(user.id); - expect(svc.deleteItem(trip.id, item.id)).toBeNull(); + expect(svc.deleteItem(trip.id, item.id, user.id)).toBeNull(); }); it('PACK-SVC-018: bulkImport stamps the owner on every item', () => { @@ -541,8 +541,9 @@ describe('three-tier packing sharing (#858)', () => { const trip = createTrip(testDb, owner.id); const item = svc.createItem(trip.id, { name: 'First aid', visibility: 'personal' }, owner.id) as any; - // A non-owner is rejected. - expect((svc.setItemSharing(trip.id, item.id, friend.id, 'shared', [friend.id]) as any).forbidden).toBe(true); + // A non-owner who cannot see the item at all gets the missing-item answer, so + // the route cannot be used to confirm that the id exists (GHSA-vh2h-288v-ggch). + expect(svc.setItemSharing(trip.id, item.id, friend.id, 'shared', [friend.id])).toBeNull(); const updated = svc.setItemSharing(trip.id, item.id, owner.id, 'shared', [friend.id]) as any; expect(updated.recipients.map((r: any) => r.user_id)).toEqual([friend.id]); @@ -675,10 +676,10 @@ describe('legacy-quirk fixes', () => { const trip = createTrip(testDb, user.id); const item = svc.createItem(trip.id, { name: 'Socks', quantity: 5 }, user.id) as any; - expect((svc.updateItem(trip.id, item.id, { quantity: 0 }, ['quantity']) as any).quantity).toBe(1); - expect((svc.updateItem(trip.id, item.id, { quantity: 9999 }, ['quantity']) as any).quantity).toBe(999); + expect((svc.updateItem(trip.id, item.id, { quantity: 0 }, ['quantity'], undefined, user.id) as any).quantity).toBe(1); + expect((svc.updateItem(trip.id, item.id, { quantity: 9999 }, ['quantity'], undefined, user.id) as any).quantity).toBe(999); // Omitted key leaves the quantity unchanged. - expect((svc.updateItem(trip.id, item.id, { name: 'Wool socks' }, ['name']) as any).quantity).toBe(999); + expect((svc.updateItem(trip.id, item.id, { name: 'Wool socks' }, ['name'], undefined, user.id) as any).quantity).toBe(999); }); }); @@ -991,3 +992,84 @@ describe('Template item scoping (post-fold quirk fix)', () => { expect(svc.deleteTemplateItem(String(tplA.template.id), String(item.item.id)) as any).toEqual({}); }); }); + +/** + * Object-level authorization on a single packing item (GHSA-vh2h-288v-ggch). + * + * listItems() always filtered by the three-tier rule, but every single-item path + * resolved the row by trip id alone, so any member holding packing_edit could + * reach another member's Personal or Shared item by id. The update response even + * handed the enriched row back, which made the write a read as well. + */ +describe('packing item object-level authorization', () => { + const restrictedTrip = () => { + const { user: owner } = createUser(testDb); + const { user: intruder } = createUser(testDb); + const { user: friend } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + const personal = svc.createItem(trip.id, { name: 'Diary', visibility: 'personal' }, owner.id) as any; + const shared = svc.createItem(trip.id, { name: 'Power bank', visibility: 'shared', recipient_ids: [friend.id] }, owner.id) as any; + const common = svc.createItem(trip.id, { name: 'Tent', visibility: 'common' }, owner.id) as any; + return { owner, intruder, friend, trip, personal, shared, common }; + }; + + it('PACK-SVC-101: a non-viewer cannot update someone else\'s Personal item', () => { + const { trip, personal, intruder } = restrictedTrip(); + expect(svc.updateItem(trip.id, personal.id, { name: 'pwned' }, ['name'], undefined, intruder.id)).toBeNull(); + expect(testDb.prepare('SELECT name FROM packing_items WHERE id = ?').get(personal.id)).toEqual({ name: 'Diary' }); + }); + + it('PACK-SVC-102: a non-recipient cannot update a Shared item', () => { + const { trip, shared, intruder } = restrictedTrip(); + expect(svc.updateItem(trip.id, shared.id, { name: 'pwned' }, ['name'], undefined, intruder.id)).toBeNull(); + }); + + it('PACK-SVC-103: a non-viewer cannot delete a restricted item', () => { + const { trip, personal, intruder } = restrictedTrip(); + expect(svc.deleteItem(trip.id, personal.id, intruder.id)).toBeNull(); + expect(testDb.prepare('SELECT COUNT(*) AS n FROM packing_items WHERE id = ?').get(personal.id)).toEqual({ n: 1 }); + }); + + it('PACK-SVC-104: a non-viewer cannot clone a restricted item into their own list', () => { + const { trip, personal, intruder } = restrictedTrip(); + expect(svc.cloneItem(trip.id, personal.id, intruder.id)).toBeNull(); + }); + + it('PACK-SVC-105b: a visible item the caller does not own still reports forbidden, not missing', () => { + const { trip, common, intruder } = restrictedTrip(); + expect((svc.setItemSharing(trip.id, common.id, intruder.id, 'personal', []) as any).forbidden).toBe(true); + }); + + it('PACK-SVC-105: a non-owner cannot re-share a restricted item they cannot see', () => { + const { trip, personal, intruder } = restrictedTrip(); + expect(svc.setItemSharing(trip.id, personal.id, intruder.id, 'common', [])).toBeNull(); + expect(testDb.prepare('SELECT is_private FROM packing_items WHERE id = ?').get(personal.id)).toEqual({ is_private: 1 }); + }); + + // Missing actor must deny rather than fall through unfiltered. + it('PACK-SVC-106: an update with no actor at all is refused', () => { + const { trip, personal } = restrictedTrip(); + expect(svc.updateItem(trip.id, personal.id, { name: 'pwned' }, ['name'])).toBeNull(); + expect(svc.deleteItem(trip.id, personal.id)).toBeNull(); + }); + + it('PACK-SVC-107: the owner and the recipients keep working', () => { + const { trip, personal, shared, common, owner, friend, intruder } = restrictedTrip(); + expect(svc.updateItem(trip.id, personal.id, { name: 'Diary v2' }, ['name'], undefined, owner.id)).toBeTruthy(); + expect(svc.updateItem(trip.id, shared.id, { checked: 1 }, ['checked'], undefined, friend.id)).toBeTruthy(); + expect(svc.updateItem(trip.id, common.id, { checked: 1 }, ['checked'], undefined, intruder.id)).toBeTruthy(); + expect(svc.cloneItem(trip.id, common.id, intruder.id)).toBeTruthy(); + expect(svc.deleteItem(trip.id, personal.id, owner.id)).toBeTruthy(); + }); + + it('PACK-SVC-108: a template captures only the Common list and the actor\'s own items', () => { + const { trip, intruder } = restrictedTrip(); + const templateId = (svc.saveAsTemplate(trip.id, intruder.id, 'Snapshot') as { id: number }).id; + const rows = testDb.prepare(` + SELECT i.name FROM packing_template_items i + JOIN packing_template_categories c ON c.id = i.category_id + WHERE c.template_id = ? + `).all(templateId) as { name: string }[]; + expect(rows.map(r => r.name).sort()).toEqual(['Tent']); + }); +}); diff --git a/server/tests/unit/nest/trips.service.test.ts b/server/tests/unit/nest/trips.service.test.ts index 679dee3fa..d872b6d4e 100644 --- a/server/tests/unit/nest/trips.service.test.ts +++ b/server/tests/unit/nest/trips.service.test.ts @@ -747,6 +747,31 @@ describe('folded trip CRUD', () => { expect((testDb.prepare('SELECT title FROM trips WHERE id = ?').get(secondCopy) as any).title).toBe('Origin'); }); + /** + * Copying a trip used to take every packing row and re-insert it without + * is_private/owner_id, so both fell back to the column defaults and another + * member's Personal or Shared item reappeared in the copy as a Common item + * that everyone on the new trip could read (GHSA-vh2h-288v-ggch). + */ + it("TRIP-SVC-046b: copy leaves other members' restricted packing items behind", () => { + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + const trip = createTrip(testDb, owner.id, { title: 'Origin', start_date: '2025-06-01', end_date: '2025-06-02' }); + const ins = testDb.prepare('INSERT INTO packing_items (trip_id, name, checked, is_private, owner_id) VALUES (?, ?, 0, ?, ?)'); + ins.run(trip.id, 'Shared tent', 0, null); // Common + ins.run(trip.id, "Owner's diary", 1, owner.id); // the owner's Personal + ins.run(trip.id, "Member's meds", 1, member.id); // the copier's own Personal + + const newTripId = svc.copy(trip.id, member.id, 'Copy'); + const rows = testDb.prepare('SELECT name, is_private, owner_id FROM packing_items WHERE trip_id = ? ORDER BY name').all(newTripId) as any[]; + + // The owner's private row is gone, not relabelled as Common. + expect(rows.map(r => r.name)).toEqual(["Member's meds", 'Shared tent']); + expect(rows.find(r => r.name === 'Shared tent')).toMatchObject({ is_private: 0, owner_id: null }); + // The copier's own item stays restricted and belongs to them in the copy. + expect(rows.find(r => r.name === "Member's meds")).toMatchObject({ is_private: 1, owner_id: member.id }); + }); + it('TRIP-SVC-059: copy remaps cross-links and carries splits/participants (smoke-test I-01)', () => { const { user: owner } = createUser(testDb); const { user: friend } = createUser(testDb); diff --git a/server/tests/unit/services/conflictUpdate.test.ts b/server/tests/unit/services/conflictUpdate.test.ts index 286fdcd6e..c318c8975 100644 --- a/server/tests/unit/services/conflictUpdate.test.ts +++ b/server/tests/unit/services/conflictUpdate.test.ts @@ -147,12 +147,12 @@ describe('updateItem (packing) — optimistic concurrency', () => { it('returns a conflict when the packing token is stale', () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); - const item = packing.createItem(trip.id, { name: 'Socks' }) as { id: number; updated_at: string }; + const item = packing.createItem(trip.id, { name: 'Socks' }, user.id) as { id: number; updated_at: string }; - const stale = packing.updateItem(trip.id, item.id, { name: 'Mine' }, ['name'], '1999-01-01 00:00:00'); + const stale = packing.updateItem(trip.id, item.id, { name: 'Mine' }, ['name'], '1999-01-01 00:00:00', user.id); expect(isUpdateConflict(stale)).toBe(true); - const fresh = packing.updateItem(trip.id, item.id, { name: 'Edited' }, ['name'], item.updated_at); + const fresh = packing.updateItem(trip.id, item.id, { name: 'Edited' }, ['name'], item.updated_at, user.id); expect(isUpdateConflict(fresh)).toBe(false); expect((fresh as { name: string }).name).toBe('Edited'); });