From 8f171453d333a8a0d1f8a2505c6f1737aa88c8fe Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 20 Aug 2026 17:13:33 +0200 Subject: [PATCH] fix(packing): keep a private item off the other members' screens over MCP MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A restricted packing item, changed through an MCP tool, went to every socket in the trip room. Ticking something off your own list through an assistant pushed that row to every other member. The REST and RPC surfaces have scoped these events since #858 — both go through emitToViewers, which delivers to the owner and the recipients and nobody else. The MCP tools called safeBroadcast, which took three arguments and had no way to say who may see the thing that changed. So safeBroadcast learned that fourth argument, and the packing tools pass viewersOf(item), which is the same answer the REST route asks for. A shared item still answers null and still goes to the room in a three-argument call, so nothing about it changes — including what the hundred-odd broadcast mocks in the suite record. The client stops trusting the wire on this too. It used to `put` whatever arrived, and the offline read hands back every cached row for the trip with no privacy filter, so a single stray event lodged another member's item in that browser permanently. It now drops a restricted item that is neither mine nor shared with me — deleting rather than skipping, so a list that was already leaked into heals itself the next time an event about that item comes past. Closes #1976 --- client/src/store/slices/remoteEventHandler.ts | 25 +++- .../remoteEventHandler/packingPrivacy.test.ts | 90 +++++++++++++++ .../mcp-shared/mcp-tool-guards.service.ts | 34 +++++- server/src/nest/packing/packing.mcp.ts | 19 ++- server/tests/unit/mcp/tools-packing.test.ts | 108 ++++++++++++++++++ 5 files changed, 266 insertions(+), 10 deletions(-) create mode 100644 client/tests/unit/remoteEventHandler/packingPrivacy.test.ts diff --git a/client/src/store/slices/remoteEventHandler.ts b/client/src/store/slices/remoteEventHandler.ts index c7dbe312e..9cdc0979a 100644 --- a/client/src/store/slices/remoteEventHandler.ts +++ b/client/src/store/slices/remoteEventHandler.ts @@ -3,6 +3,7 @@ import type { TrekWsTripEventName } from '@trek/shared' import type { TripStoreState } from '../tripStore' import type { Assignment, Place, Day, DayNote, PackingItem, TodoItem, BudgetItem, BudgetItemMember, Reservation, Trip, TripFile, WebSocketEvent } from '../../types' import { offlineDb } from '../../db/offlineDb' +import { useAuthStore } from '../authStore' type SetState = StoreApi['setState'] type GetState = StoreApi['getState'] @@ -27,8 +28,30 @@ const writeDayById: DexieWriter = async (payload, state) => { const day = payload.day as Day await _writeDayToDb(day.id, state) } +/** + * Cache a packing item — unless it is somebody else's private one (#1976). + * + * The server scopes these events to the people who may see the item, so in + * ordinary running this never has anything to refuse. It refuses anyway, for + * two reasons. A leak on the wire used to become permanent here: the write is a + * put, the offline read hands back every cached row for the trip, and nothing + * prunes, so one stray event put another member's item into this browser for + * good. And a member whose access to a shared item is withdrawn should lose the + * copy they already have, which is the same check. + * + * Deleting rather than skipping is the part that matters: skipping would leave + * a row that arrived before this existed sitting there forever. + */ const putPackingItem: DexieWriter = async payload => { - await offlineDb.packingItems.put(payload.item as PackingItem) + const item = payload.item as PackingItem + const me = useAuthStore.getState().user?.id + const mine = !item?.is_private || item.owner_id == null || item.owner_id === me + || (item.recipients || []).some(r => r.user_id === me) + if (!mine) { + await offlineDb.packingItems.delete(item.id) + return + } + await offlineDb.packingItems.put(item) } const putTodoItem: DexieWriter = async payload => { await offlineDb.todoItems.put(payload.item as TodoItem) diff --git a/client/tests/unit/remoteEventHandler/packingPrivacy.test.ts b/client/tests/unit/remoteEventHandler/packingPrivacy.test.ts new file mode 100644 index 000000000..cd285e3d4 --- /dev/null +++ b/client/tests/unit/remoteEventHandler/packingPrivacy.test.ts @@ -0,0 +1,90 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest'; + +/** + * What the offline cache is allowed to keep of another member's packing list + * (#1976). + * + * The server scopes these events now, so in ordinary running nothing here has + * anything to refuse. It refuses anyway, because a leak on the wire used to + * become permanent on this side: the write is a `put`, the offline read hands + * back every cached row for the trip without a privacy filter, and nothing ever + * prunes. One stray event put another member's item into this browser for good, + * and it surfaced every time a read fell back to the cache — offline, captive + * portal, dropped connection. + * + * So the interesting case is not that the item is skipped. It is that a row + * which arrived before any of this existed gets deleted when the next event + * about it comes past, which is what makes an already-leaked list heal itself. + */ + +const { packingItems } = vi.hoisted(() => ({ + packingItems: { put: vi.fn(async () => undefined), delete: vi.fn(async () => undefined) }, +})); + +vi.mock('../../../src/db/offlineDb', () => ({ + offlineDb: { + packingItems, + // The handler module touches these at import time only. + trips: {}, days: {}, places: {}, assignments: {}, + }, +})); + +import { useTripStore } from '../../../src/store/tripStore'; +import { useAuthStore } from '../../../src/store/authStore'; +import { resetAllStores } from '../../helpers/store'; +import { buildPackingItem } from '../../helpers/factories'; + +const ME = 7; +const SOMEONE_ELSE = 99; + +beforeEach(() => { + resetAllStores(); + packingItems.put.mockClear(); + packingItems.delete.mockClear(); + useAuthStore.setState({ user: { id: ME, username: 'me', email: 'me@example.test' } as never }); +}); + +const send = (item: unknown) => + useTripStore.getState().handleRemoteEvent({ type: 'packing:updated', item } as never); + +describe('a packing item arriving over the wire', () => { + it('is cached when it is shared with the whole trip', async () => { + send(buildPackingItem({ id: 1, is_private: 0 })); + expect(packingItems.put).toHaveBeenCalledTimes(1); + expect(packingItems.delete).not.toHaveBeenCalled(); + }); + + it('is cached when it is my own private one', async () => { + send(buildPackingItem({ id: 2, is_private: 1, owner_id: ME })); + expect(packingItems.put).toHaveBeenCalledTimes(1); + }); + + it('is cached when it is private but shared with me', async () => { + send(buildPackingItem({ + id: 3, is_private: 1, owner_id: SOMEONE_ELSE, + recipients: [{ user_id: ME, username: 'me' }], + })); + expect(packingItems.put).toHaveBeenCalledTimes(1); + }); + + /* The assertion that would have caught the leak. */ + it('is not cached when it is somebody else s private one', async () => { + send(buildPackingItem({ id: 4, is_private: 1, owner_id: SOMEONE_ELSE })); + expect(packingItems.put).not.toHaveBeenCalled(); + }); + + it('removes a copy that an earlier leak already left behind', async () => { + send(buildPackingItem({ id: 5, is_private: 1, owner_id: SOMEONE_ELSE })); + expect(packingItems.delete).toHaveBeenCalledWith(5); + }); + + /* + * A signed-out or not-yet-loaded session has no id to compare against. + * Refusing is the safe answer: there is no user whose list this could be. + */ + it('keeps a restricted item out when nobody is signed in', async () => { + useAuthStore.setState({ user: null }); + send(buildPackingItem({ id: 6, is_private: 1, owner_id: SOMEONE_ELSE })); + expect(packingItems.put).not.toHaveBeenCalled(); + }); +}); diff --git a/server/src/nest/mcp-shared/mcp-tool-guards.service.ts b/server/src/nest/mcp-shared/mcp-tool-guards.service.ts index 1324cc2ad..0a1c1a9f1 100644 --- a/server/src/nest/mcp-shared/mcp-tool-guards.service.ts +++ b/server/src/nest/mcp-shared/mcp-tool-guards.service.ts @@ -19,15 +19,41 @@ export class McpToolGuardsService { private readonly realtime: RealtimeService, ) {} - safeBroadcast(tripId: number, event: string, payload: Record): void { + /** + * Broadcast a tool's change to the trip room. + * + * `onlyUserIds` scopes delivery the way the REST routes already scope theirs: + * pass the users who may see the thing that changed, and the event is sent to + * those users' sockets rather than to everyone in the room. Omitting it keeps + * the room-wide behaviour, which is right for anything the whole trip shares. + * + * It is optional rather than required because most tools change something + * everybody can see — but a tool that touches a restricted row and forgets it + * pushes that row onto every other member's screen, which is #1976: a private + * packing item, ticked off through MCP, landed in every other member's + * IndexedDB and stayed there. + */ + safeBroadcast(tripId: number, event: string, payload: Record, onlyUserIds?: number[] | null): void { try { // Legacy MCP call sites pass event names as plain strings; route through // the facade's implementation signature (its typed overloads are for new // call sites). Runtime is a pure pass-through to src/websocket's // broadcast, so the 100+ per-file vi.mock stubs keep intercepting. - (this.realtime.broadcast as (t: number | string, e: string, p: Record) => void)( - tripId, event, { ...payload, _source: 'mcp' }, - ); + const send = this.realtime.broadcast as ( + t: number | string, e: string, p: Record, sid?: number | string, uid?: number, + ) => void; + const body = { ...payload, _source: 'mcp' }; + + // Room-wide, and called with three arguments exactly as before: the + // facade preserves its caller's arity, and padding the optionals with + // explicit undefineds would change what every existing mock records. + if (onlyUserIds == null) { + send(tripId, event, body); + return; + } + for (const uid of new Set(onlyUserIds)) { + if (uid != null) send(tripId, event, body, undefined, uid); + } } catch (err) { console.error(`[MCP] broadcast failed for ${event}:`, (err as Error | undefined)?.message ?? err); } diff --git a/server/src/nest/packing/packing.mcp.ts b/server/src/nest/packing/packing.mcp.ts index 2e713fb68..ade7fe3fd 100644 --- a/server/src/nest/packing/packing.mcp.ts +++ b/server/src/nest/packing/packing.mcp.ts @@ -61,7 +61,10 @@ export class PackingMcp { if (!this.packing.verifyTripAccess(tripId, ctx.userId)) return noAccess(); if (!this.guards.hasTripPermission('packing_edit', tripId, ctx.userId)) return permissionDenied(); const item = this.packing.createItem(tripId, { name, category: category || 'General' }, ctx.userId); - this.guards.safeBroadcast(tripId, 'packing:created', { item }); + // A tool-made item is Common today, so this asks the room. Routed through + // viewersOf anyway, so the day createItem learns to make a restricted one + // this does not have to be remembered a second time. + this.guards.safeBroadcast(tripId, 'packing:created', { item }, this.packing.viewersOf(item)); return ok({ item }); } @@ -83,7 +86,9 @@ export class PackingMcp { if (!this.guards.hasTripPermission('packing_edit', tripId, ctx.userId)) return permissionDenied(); 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 }); + // Scoped to the people who may see it, exactly as the REST route does + // (#858, #1976). A Common item answers null here and goes to the room. + this.guards.safeBroadcast(tripId, 'packing:updated', { item }, this.packing.viewersOf(item)); return ok({ item }); } @@ -104,7 +109,9 @@ export class PackingMcp { if (!this.guards.hasTripPermission('packing_edit', tripId, ctx.userId)) return permissionDenied(); const deleted = this.packing.deleteItem(tripId, itemId, ctx.userId); if (!deleted) return errorResult('Packing item not found.'); - this.guards.safeBroadcast(tripId, 'packing:deleted', { itemId }); + // deleteItem hands back the row it removed, so the delete can be scoped to + // the same people the item was ever visible to (#1976). + this.guards.safeBroadcast(tripId, 'packing:deleted', { itemId }, this.packing.viewersOf(deleted)); return ok({ success: true }); } @@ -130,7 +137,7 @@ export class PackingMcp { const bodyKeys = ['name', 'category'].filter(k => k === 'name' ? name !== undefined : category !== undefined); 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 }); + this.guards.safeBroadcast(tripId, 'packing:updated', { item }, this.packing.viewersOf(item)); return ok({ item }); } @@ -408,7 +415,9 @@ export class PackingMcp { if (!this.packing.verifyTripAccess(tripId, ctx.userId)) return noAccess(); if (!this.guards.hasTripPermission('packing_edit', tripId, ctx.userId)) return permissionDenied(); const created = this.packing.bulkImport(tripId, items, ctx.userId); - for (const item of created) this.guards.safeBroadcast(tripId, 'packing:created', { item }); + for (const item of created) { + this.guards.safeBroadcast(tripId, 'packing:created', { item }, this.packing.viewersOf(item)); + } return ok({ items: created, count: created.length }); } diff --git a/server/tests/unit/mcp/tools-packing.test.ts b/server/tests/unit/mcp/tools-packing.test.ts index 4ec5a81b8..de450f07d 100644 --- a/server/tests/unit/mcp/tools-packing.test.ts +++ b/server/tests/unit/mcp/tools-packing.test.ts @@ -438,3 +438,111 @@ describe('Resource: trek://trips/{tripId}/packing/bags', () => { }); }); }); + +// --------------------------------------------------------------------------- +// Private items over MCP (#1976) +// --------------------------------------------------------------------------- + +/** + * A restricted packing item, changed through a tool, must reach only the people + * who may see it. + * + * The REST and RPC surfaces have scoped this since #858 — they go through + * emitToViewers, which hands the event to the owner and the recipients. The MCP + * tools broadcast to the whole trip room instead, so asking an assistant to + * tick off something on your own list pushed that row to every other member. + * + * It did not stop at the wire either: the client stores what arrives + * (remoteEventHandler -> putPackingItem -> bulkPut) with no owner check, and the + * offline read path returns every cached row for the trip. So the leaked item + * stayed in the other member's IndexedDB and rendered whenever their next read + * fell back to the cache. + * + * These cases pin the wire, which is where it has to be fixed: a room-wide call + * still takes three arguments, so nothing about a shared item changes. + */ +describe('a restricted packing item over MCP', () => { + const makePrivate = (itemId: number, ownerId: number) => + testDb.prepare('UPDATE packing_items SET is_private = 1, owner_id = ? WHERE id = ?').run(ownerId, itemId); + + it('is ticked off for its owner alone, not for the whole trip', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const item = createPackingItem(testDb, trip.id); + makePrivate(item.id, user.id); + + await withHarness(user.id, async (h) => { + await h.client.callTool({ + name: 'toggle_packing_item', + arguments: { tripId: trip.id, itemId: item.id, checked: true }, + }); + }); + + // The assertion that would have caught the leak: a fifth argument naming + // the only user this may reach. + expect(broadcastMock).toHaveBeenCalledWith( + trip.id, 'packing:updated', expect.any(Object), undefined, user.id, + ); + expect(broadcastMock).not.toHaveBeenCalledWith(trip.id, 'packing:updated', expect.any(Object)); + }); + + it('is renamed for its owner alone', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const item = createPackingItem(testDb, trip.id); + makePrivate(item.id, user.id); + + await withHarness(user.id, async (h) => { + await h.client.callTool({ + name: 'update_packing_item', + arguments: { tripId: trip.id, itemId: item.id, name: 'Insulin pens' }, + }); + }); + + expect(broadcastMock).toHaveBeenCalledWith( + trip.id, 'packing:updated', expect.any(Object), undefined, user.id, + ); + expect(broadcastMock).not.toHaveBeenCalledWith(trip.id, 'packing:updated', expect.any(Object)); + }); + + /* + * The delete carries only an id, which looks harmless — but it names an id + * the other members were never told about, and it removes a row from their + * store that a leak had put there. Scoped the same way, from the row the + * delete hands back. + */ + it('is deleted for its owner alone', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const item = createPackingItem(testDb, trip.id); + makePrivate(item.id, user.id); + + await withHarness(user.id, async (h) => { + await h.client.callTool({ + name: 'delete_packing_item', + arguments: { tripId: trip.id, itemId: item.id }, + }); + }); + + expect(broadcastMock).toHaveBeenCalledWith( + trip.id, 'packing:deleted', expect.any(Object), undefined, user.id, + ); + expect(broadcastMock).not.toHaveBeenCalledWith(trip.id, 'packing:deleted', expect.any(Object)); + }); + + it('still tells the whole room about a shared one, unchanged', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const item = createPackingItem(testDb, trip.id); + + await withHarness(user.id, async (h) => { + await h.client.callTool({ + name: 'toggle_packing_item', + arguments: { tripId: trip.id, itemId: item.id, checked: true }, + }); + }); + + // Three arguments exactly, which is what every other packing case asserts. + expect(broadcastMock).toHaveBeenCalledWith(trip.id, 'packing:updated', expect.any(Object)); + }); +});