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)); + }); +});