From c3161a05dd36635f226efc8bc0375496eb6cd67c Mon Sep 17 00:00:00 2001 From: claude_code Date: Sun, 21 Jun 2026 14:24:18 +0300 Subject: [PATCH] refactor(ws): single-snapshot move audience to close the restricted-move race (#93) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements Option 2 of #93. The restricted branch of broadcastPageMoved previously resolved its audience twice — emitToAuthorizedUsers and emitDeleteToUnauthorized each ran an independent fetchSockets + getUserIdsWithPageAccess — leaving a race window between the two snapshots where a socket could receive both the move and the delete (leak) or neither (lost compensating delete). - ws.service.ts: add emitMoveWithRestrictionSplit() that takes ONE socket snapshot and ONE authorization resolution, then partitions the room: authorized users get the moveTreeNode, everyone else (unauthorized + anonymous) get the compensating deleteTreeNode. Disjoint + complete by construction. Remove the now-unused emitToAuthorizedUsers / emitDeleteToUnauthorized; keep private broadcastToAuthorizedUsers (still used by emitRestrictedAwareToSpace). - ws-tree.service.ts: broadcastPageMoved restricted branch now drives move + delete from the single method. - specs: assert the single method is used and that fetchSockets / getUserIdsWithPageAccess are each called exactly once (single snapshot); re-route ws-service.spec to emitTreeEvent after the method removal. Co-Authored-By: Claude Opus 4.8 --- apps/server/src/ws/ws-service.spec.ts | 24 +++-- apps/server/src/ws/ws-tree.service.spec.ts | 106 +++++++++++++-------- apps/server/src/ws/ws-tree.service.ts | 22 ++--- apps/server/src/ws/ws.service.ts | 76 ++++++++------- 4 files changed, 134 insertions(+), 94 deletions(-) diff --git a/apps/server/src/ws/ws-service.spec.ts b/apps/server/src/ws/ws-service.spec.ts index c87d1493..c787347c 100644 --- a/apps/server/src/ws/ws-service.spec.ts +++ b/apps/server/src/ws/ws-service.spec.ts @@ -16,9 +16,9 @@ import { * fan-out per user, sockets with no userId skipped). * * Both private methods are exercised through their public entry points: - * spaceHasRestrictions via emitTreeEvent, broadcastToAuthorizedUsers via - * emitToAuthorizedUsers. WsService is constructed with mocked cache + repo and a - * mocked socket.io server, so no live infra is needed. + * spaceHasRestrictions via emitTreeEvent, broadcastToAuthorizedUsers via the + * restricted-page path of emitTreeEvent. WsService is constructed with mocked + * cache + repo and a mocked socket.io server, so no live infra is needed. */ describe('WsService.spaceHasRestrictions (cache lifecycle, via emitTreeEvent)', () => { @@ -127,7 +127,7 @@ describe('WsService.spaceHasRestrictions (cache lifecycle, via emitTreeEvent)', }); }); -describe('WsService.broadcastToAuthorizedUsers fan-out (via emitToAuthorizedUsers)', () => { +describe('WsService.broadcastToAuthorizedUsers fan-out (via emitTreeEvent restricted path)', () => { let service: WsService; let pagePermissionRepo: { hasRestrictedPagesInSpace: jest.Mock; @@ -167,6 +167,12 @@ describe('WsService.broadcastToAuthorizedUsers fan-out (via emitToAuthorizedUser in: serverIn, }; service.setServer(server as never); + + // Reach broadcastToAuthorizedUsers through emitTreeEvent's restricted path: + // the space has restrictions (cache miss -> repo says true) and the page has + // a restricted ancestor, so the emit is scoped to the authorized users. + pagePermissionRepo.hasRestrictedPagesInSpace.mockResolvedValue(true); + pagePermissionRepo.hasRestrictedAncestor.mockResolvedValue(true); }); it('only sockets whose userId is in getUserIdsWithPageAccess receive the event', async () => { @@ -180,7 +186,7 @@ describe('WsService.broadcastToAuthorizedUsers fan-out (via emitToAuthorizedUser ]); const data = { operation: 'moveTreeNode' }; - await service.emitToAuthorizedUsers('space-1', 'page-1', data); + await service.emitTreeEvent('space-1', 'page-1', data); // The authorized set is resolved from the candidate userIds present on the // sockets (deduped), then only those users' sockets get the event. @@ -203,7 +209,7 @@ describe('WsService.broadcastToAuthorizedUsers fan-out (via emitToAuthorizedUser ]); const data = { operation: 'moveTreeNode' }; - await service.emitToAuthorizedUsers('space-1', 'page-1', data); + await service.emitTreeEvent('space-1', 'page-1', data); // Both of the authorized user's sockets (e.g. two browser tabs) receive it. expect(tab1).toHaveBeenCalledWith('message', data); @@ -227,7 +233,7 @@ describe('WsService.broadcastToAuthorizedUsers fan-out (via emitToAuthorizedUser ]); const data = { operation: 'moveTreeNode' }; - await service.emitToAuthorizedUsers('space-1', 'page-1', data); + await service.emitTreeEvent('space-1', 'page-1', data); expect(okEmit).toHaveBeenCalledWith('message', data); expect(anonEmit).not.toHaveBeenCalled(); @@ -241,7 +247,7 @@ describe('WsService.broadcastToAuthorizedUsers fan-out (via emitToAuthorizedUser it('no sockets in the room -> no repo lookup, no emit', async () => { fetchSockets.mockResolvedValue([]); - await service.emitToAuthorizedUsers('space-1', 'page-1', { op: 'x' }); + await service.emitTreeEvent('space-1', 'page-1', { op: 'x' }); expect(pagePermissionRepo.getUserIdsWithPageAccess).not.toHaveBeenCalled(); }); @@ -252,7 +258,7 @@ describe('WsService.broadcastToAuthorizedUsers fan-out (via emitToAuthorizedUser { id: 's1', data: { userId: 'u' }, emit: jest.fn() }, ]); - await service.emitToAuthorizedUsers('space-7', 'page-1', { op: 'x' }); + await service.emitTreeEvent('space-7', 'page-1', { op: 'x' }); expect(serverIn).toHaveBeenCalledWith(getSpaceRoomName('space-7')); }); diff --git a/apps/server/src/ws/ws-tree.service.spec.ts b/apps/server/src/ws/ws-tree.service.spec.ts index e007e249..1ee8d10b 100644 --- a/apps/server/src/ws/ws-tree.service.spec.ts +++ b/apps/server/src/ws/ws-tree.service.spec.ts @@ -27,8 +27,7 @@ describe('WsTreeService', () => { let wsService: { emitTreeEvent: jest.Mock; emitToSpaceRoom: jest.Mock; - emitDeleteToUnauthorized: jest.Mock; - emitToAuthorizedUsers: jest.Mock; + emitMoveWithRestrictionSplit: jest.Mock; }; let pagePermissionRepo: { hasRestrictedAncestor: jest.Mock }; @@ -36,8 +35,7 @@ describe('WsTreeService', () => { wsService = { emitTreeEvent: jest.fn().mockResolvedValue(undefined), emitToSpaceRoom: jest.fn(), - emitDeleteToUnauthorized: jest.fn().mockResolvedValue(undefined), - emitToAuthorizedUsers: jest.fn().mockResolvedValue(undefined), + emitMoveWithRestrictionSplit: jest.fn().mockResolvedValue(undefined), }; pagePermissionRepo = { // Default: not restricted, so broadcastPageMoved skips the compensating @@ -150,14 +148,13 @@ describe('WsTreeService', () => { await service.broadcastPageMoved(event); - // Normal path: move goes to the whole room via emitTreeEvent, and neither - // the authorized-only move path nor the compensating delete fire. + // Normal path: move goes to the whole room via emitTreeEvent, and the + // single-snapshot move/delete split does not fire. expect(wsService.emitTreeEvent).toHaveBeenCalledTimes(1); - expect(wsService.emitToAuthorizedUsers).not.toHaveBeenCalled(); - expect(wsService.emitDeleteToUnauthorized).not.toHaveBeenCalled(); + expect(wsService.emitMoveWithRestrictionSplit).not.toHaveBeenCalled(); }); - it('broadcastPageMoved into a RESTRICTED subtree routes the move to authorized users only AND emits a compensating delete to unauthorized — from one fresh decision', async () => { + it('broadcastPageMoved into a RESTRICTED subtree drives the move + compensating delete from ONE single-snapshot split call', async () => { // Destination is now under a restricted ancestor. pagePermissionRepo.hasRestrictedAncestor.mockResolvedValue(true); @@ -180,11 +177,18 @@ describe('WsTreeService', () => { // which could leak the move to the whole room during the stale-cache window. expect(wsService.emitTreeEvent).not.toHaveBeenCalled(); - // The move is delivered to authorized users only. - expect(wsService.emitToAuthorizedUsers).toHaveBeenCalledTimes(1); - expect(wsService.emitToAuthorizedUsers).toHaveBeenCalledWith( - 'space-1', - 'page-1', + // BOTH the move and the compensating delete are driven from ONE call, so a + // single socket/access snapshot partitions the room (no race window). + expect(wsService.emitMoveWithRestrictionSplit).toHaveBeenCalledTimes(1); + + const [spaceId, pageId, movePayload, deletePayload] = + wsService.emitMoveWithRestrictionSplit.mock.calls[0]; + + expect(spaceId).toBe('space-1'); + expect(pageId).toBe('page-1'); + + // The move payload is the moveTreeNode for the moved page. + expect(movePayload).toEqual( expect.objectContaining({ operation: 'moveTreeNode', spaceId: 'space-1', @@ -192,20 +196,23 @@ describe('WsTreeService', () => { }), ); - // The users who lost access get a deleteTreeNode for the moved node, scoped - // to the same page id (same fresh authorized set → disjoint from the move). - expect(wsService.emitDeleteToUnauthorized).toHaveBeenCalledTimes(1); - expect(wsService.emitDeleteToUnauthorized).toHaveBeenCalledWith( - 'space-1', - 'page-1', + // The delete payload is the compensating deleteTreeNode, scoped to the same + // page id and carrying the OLD parent id (so it disappears from where it was + // last visible). + expect(deletePayload).toEqual( expect.objectContaining({ operation: 'deleteTreeNode', spaceId: 'space-1', payload: { - node: expect.objectContaining({ id: 'page-1', slugId: 'slug-1' }), + node: expect.objectContaining({ + id: 'page-1', + slugId: 'slug-1', + parentPageId: 'old-parent', + }), }, }), ); + expect(deletePayload.payload.node.parentPageId).toBe(event.oldParentId); }); it('broadcastRefetchRoot emits refetchRootTreeNodeEvent to the space room', async () => { @@ -339,7 +346,7 @@ describe('WsService.emitTreeEvent', () => { ); }); - it('emitDeleteToUnauthorized sends ONLY to sockets whose user lacks page access', async () => { + it('emitMoveWithRestrictionSplit partitions the room from one snapshot: authorized -> move, unauthorized + anonymous -> delete', async () => { pagePermissionRepo.getUserIdsWithPageAccess.mockResolvedValue(['user-ok']); const okEmit = jest.fn(); @@ -348,38 +355,49 @@ describe('WsService.emitTreeEvent', () => { const sockets = [ { id: 's1', data: { userId: 'user-ok' }, emit: okEmit }, { id: 's2', data: { userId: 'user-no' }, emit: noEmit }, - // Unauthenticated socket (no userId) — must also receive the delete. + // Unauthenticated socket (no userId) — must receive the delete. { id: 's3', data: {}, emit: anonEmit }, ]; server.in.mockReturnValue({ fetchSockets: jest.fn().mockResolvedValue(sockets), }); - const data = { operation: 'deleteTreeNode' }; - await service.emitDeleteToUnauthorized('space-1', 'page-1', data); + const movePayload = { operation: 'moveTreeNode' }; + const deletePayload = { operation: 'deleteTreeNode' }; + await service.emitMoveWithRestrictionSplit( + 'space-1', + 'page-1', + movePayload, + deletePayload, + ); - // Authorized user does NOT get the delete (they got the move instead). - expect(okEmit).not.toHaveBeenCalled(); - // Unauthorized + anonymous sockets DO get the delete. - expect(noEmit).toHaveBeenCalledWith('message', data); - expect(anonEmit).toHaveBeenCalledWith('message', data); + // Authorized socket gets ONLY the move. + expect(okEmit).toHaveBeenCalledWith('message', movePayload); + expect(okEmit).not.toHaveBeenCalledWith('message', deletePayload); + // Unauthorized + anonymous sockets get ONLY the delete. + expect(noEmit).toHaveBeenCalledWith('message', deletePayload); + expect(noEmit).not.toHaveBeenCalledWith('message', movePayload); + expect(anonEmit).toHaveBeenCalledWith('message', deletePayload); + expect(anonEmit).not.toHaveBeenCalledWith('message', movePayload); }); }); describe('move-into-restricted disjointness contract (WsTreeService + real WsService)', () => { - // CONTRACT: a move under a restricted ancestor PARTITIONS the room. The - // authorized set (gets the moveTreeNode via emitToAuthorizedUsers) and its - // complement (gets the deleteTreeNode via emitDeleteToUnauthorized) are - // disjoint and together cover every socket — and an anonymous (no-userId) - // socket lands in the delete set. We wire a REAL WsService (only its repo, - // cache and socket server mocked) so both broadcasts run against the SAME fixed - // socket set, the way they do in production. + // CONTRACT: a move under a restricted ancestor PARTITIONS the room from a + // SINGLE snapshot. emitMoveWithRestrictionSplit performs exactly one + // fetchSockets + one getUserIdsWithPageAccess; the authorized set (gets the + // moveTreeNode) and its complement (gets the deleteTreeNode) are disjoint and + // together cover every socket — and an anonymous (no-userId) socket lands in + // the delete set. We wire a REAL WsService (only its repo, cache and socket + // server mocked) so the partition runs against the SAME fixed socket set, the + // way it does in production. let treeService: WsTreeService; let pagePermissionRepo: { hasRestrictedPagesInSpace: jest.Mock; hasRestrictedAncestor: jest.Mock; getUserIdsWithPageAccess: jest.Mock; }; + let fetchSockets: jest.Mock; // Fixed room: two authorized users (one with two sockets), one unauthorized // user, one anonymous socket. @@ -429,11 +447,12 @@ describe('move-into-restricted disjointness contract (WsTreeService + real WsSer }).compile(); const wsService = module.get(WsService); + // Capture fetchSockets so the test can assert the SINGLE-snapshot contract: + // exactly one fetchSockets call drives the whole partition. + fetchSockets = jest.fn().mockResolvedValue(sockets); const server = { to: jest.fn().mockReturnValue({ emit: jest.fn() }), - in: jest.fn().mockReturnValue({ - fetchSockets: jest.fn().mockResolvedValue(sockets), - }), + in: jest.fn().mockReturnValue({ fetchSockets }), }; wsService.setServer(server as never); @@ -469,5 +488,12 @@ describe('move-into-restricted disjointness contract (WsTreeService + real WsSer // The anonymous socket specifically lands in the DELETE set, never the move. expect(deleteSet.has('s-anon')).toBe(true); expect(moveSet.has('s-anon')).toBe(false); + + // SINGLE SNAPSHOT: the whole partition (move + compensating delete) is driven + // from exactly ONE fetchSockets and exactly ONE getUserIdsWithPageAccess. + // This is what closes the race window — there is no second, independent + // snapshot that could disagree with the first. + expect(fetchSockets).toHaveBeenCalledTimes(1); + expect(pagePermissionRepo.getUserIdsWithPageAccess).toHaveBeenCalledTimes(1); }); }); diff --git a/apps/server/src/ws/ws-tree.service.ts b/apps/server/src/ws/ws-tree.service.ts index 5e052485..223fb25c 100644 --- a/apps/server/src/ws/ws-tree.service.ts +++ b/apps/server/src/ws/ws-tree.service.ts @@ -131,22 +131,20 @@ export class WsTreeService { } // Restricted case: a move can push a previously-visible page UNDER a - // restricted ancestor. Route the move to authorized users ONLY (same fresh - // getUserIdsWithPageAccess set the delete uses) and send the compensating - // delete to everyone else. Both sets come from one fresh decision, so they - // are guaranteed disjoint: authorized users get exactly the moveTreeNode, - // unauthorized users get exactly the deleteTreeNode, nobody gets both. + // restricted ancestor. The move (to authorized users) and the compensating + // delete (to everyone else) are now driven from ONE socket/access snapshot: + // emitMoveWithRestrictionSplit performs a single fetchSockets + a single + // getUserIdsWithPageAccess and partitions the room from that one snapshot. + // This eliminates the race window that existed when the move and the delete + // each resolved the audience independently — a socket could otherwise have + // landed in both sets (leaking the restricted node) or in neither (losing the + // compensating delete). Authorized users get exactly the moveTreeNode, + // everyone else (unauthorized + anonymous) gets exactly the deleteTreeNode. // // Users who LOSE visibility need the delete because otherwise the node would // linger in their tree at its old parent with its real title/slugId/icon // (existence + metadata leak). - await this.wsService.emitToAuthorizedUsers( - node.spaceId, - node.id, - movePayload, - ); - - await this.wsService.emitDeleteToUnauthorized(node.spaceId, node.id, { + await this.wsService.emitMoveWithRestrictionSplit(node.spaceId, node.id, movePayload, { operation: 'deleteTreeNode', spaceId: node.spaceId, payload: { diff --git a/apps/server/src/ws/ws.service.ts b/apps/server/src/ws/ws.service.ts index 18e20fb0..9986cd06 100644 --- a/apps/server/src/ws/ws.service.ts +++ b/apps/server/src/ws/ws.service.ts @@ -118,19 +118,42 @@ export class WsService { this.server.to(getSpaceRoomName(spaceId)).emit('message', data); } - // Broadcast `data` (a deleteTreeNode) to every socket in the space room whose - // user is NOT authorized to see `pageId`. Used to compensate a move that pushes - // a previously-visible page UNDER a restricted ancestor: authorized users get - // the moveTreeNode (via emitTreeEvent), everyone else gets a deleteTreeNode so - // the now-restricted node disappears from their tree instead of lingering with - // its real title/slugId/icon. The two event sets are disjoint by construction - // (a user is either authorized or not), so no socket receives both. - async emitDeleteToUnauthorized( + // Single-snapshot move broadcast. This is the ONE place that fans out a move + // under a restricted ancestor together with its compensating delete, resolving + // the audience EXACTLY ONCE so the two never disagree. + // + // It takes a SINGLE socket snapshot (`this.server.in(room).fetchSockets()` is + // called exactly once) and a SINGLE authorization resolution + // (`getUserIdsWithPageAccess` is called exactly once). From that one snapshot it + // partitions the room into two groups and emits to each: + // - authorized users (their userId is in the authorized set) receive + // `movePayload` (the moveTreeNode); + // - everyone else — unauthorized users AND anonymous/no-userId sockets — + // receive `deletePayload` (the compensating deleteTreeNode) so a now-hidden + // node disappears from their tree instead of lingering with its real + // title/slugId/icon. + // Because both groups are derived from the same socket array and the same + // authorized set, the partition is guaranteed DISJOINT (no socket gets both) + // and COMPLETE (every socket gets exactly one). This closes the race window + // that existed when the move and the compensating delete each ran their own + // independent fetchSockets + getUserIdsWithPageAccess: between those two + // snapshots a socket could connect/disconnect or its access change, so a socket + // could end up in both sets (leaking the restricted node, then no delete) or in + // neither (losing the compensating delete). + // + // It deliberately does NOT consult the cached spaceHasRestrictions: the caller + // (broadcastPageMoved) has already established, freshly and uncached, that the + // page is restricted, so we must not risk a stale cache fanning the move out to + // the whole room. + async emitMoveWithRestrictionSplit( spaceId: string, pageId: string, - data: any, + movePayload: any, + deletePayload: any, ): Promise { const room = getSpaceRoomName(spaceId); + + // ONE socket snapshot for the whole partition. const sockets = await this.server.in(room).fetchSockets(); if (sockets.length === 0) return; @@ -141,39 +164,26 @@ export class WsService { .filter((id): id is string => !!id), ), ); - if (userIds.length === 0) return; - const authorizedUserIds = - await this.pagePermissionRepo.getUserIdsWithPageAccess(pageId, userIds); + // ONE authorization resolution for the whole partition. + const authorizedUserIds = userIds.length + ? await this.pagePermissionRepo.getUserIdsWithPageAccess(pageId, userIds) + : []; const authorizedSet = new Set(authorizedUserIds); for (const socket of sockets) { const userId = socket.data.userId as string; - // Unauthenticated sockets (no userId) cannot see restricted content; send - // them the delete too so a leaked node can't linger. - if (!userId || !authorizedSet.has(userId)) { - socket.emit('message', data); + if (userId && authorizedSet.has(userId)) { + // Authorized: deliver the move. + socket.emit('message', movePayload); + } else { + // Unauthorized OR anonymous (no userId): deliver the compensating + // delete so the now-hidden node can't linger. + socket.emit('message', deletePayload); } } } - // Server-origin broadcast of `data` to exactly the users in the space room who - // ARE authorized to see `pageId`. This is the counterpart of - // emitDeleteToUnauthorized: both resolve the authorized set from the SAME - // fetchSockets + getUserIdsWithPageAccess call shape, so a caller that drives - // both from one decision gets two disjoint sets (authorized vs. not) with no - // socket in both. Unlike emitTreeEvent, this does NOT consult the cached - // spaceHasRestrictions: the caller already knows the page is restricted, so we - // must not risk a stale cache fanning the move out to the whole room. - async emitToAuthorizedUsers( - spaceId: string, - pageId: string, - data: any, - ): Promise { - const room = getSpaceRoomName(spaceId); - await this.broadcastToAuthorizedUsers(room, null, pageId, data); - } - private async broadcastToAuthorizedUsers( room: string, excludeSocketId: string | null,