Skip to content

Commit f43b52c

Browse files
fix(realtime): evict revoked collaborators from live workflow rooms (#5917)
* fix(realtime): evict revoked collaborators from live workflow rooms via periodic read-access re-validation * fix * fix(realtime): close join/eviction race and make sweep cleanups independent Re-authorize immediately before socket.join so an in-flight join cannot reverse a sweep eviction, and run the sweep's best-effort cleanups independently so a room-state failure cannot skip the presence broadcast. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(realtime): single-flight role resolution and retry failed eviction cleanup Coalesce concurrent role resolutions per (user, workflow) so a slow stale read can never overwrite a recorded revocation, and defer failed eviction room-state cleanups into a per-sweep retry queue so collaborators are not left with a stale presence entry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(realtime): scope room removal to the target workflow and detect swallowed cleanup failures Honor the workflowIdHint as the target room in both room managers so removing a stale room cannot clobber the mapping of a room the socket has since moved to, and confirm eviction cleanup via the returned workflowId plus an unswallowed mapping read so Redis failures actually defer into the retry queue. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(realtime): treat unconfirmed removals as failures and refresh role cache on fresh verify Treat any null removal result as a failed cleanup (the sweep always passes the target room, so null only means failure — including with expired mapping keys), move the same-room rejoin guard to a synchronous check immediately before the removal, and record verifyWorkflowAccess's fresh decision into the role cache so a re-granted user is not blocked by a stale cached revocation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(realtime): prefer a mid-flight recorded role decision over the in-flight query result If a fresh authoritative read (join-time verify) records a decision while a single-flighted resolution's query is in flight, keep the recorded decision instead of overwriting it with the potentially stale result. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(realtime): isolate the security scan from the Redis cleanup lane Run the revocation scan (local sockets + DB only) and the best-effort room-state cleanup as independently-guarded lanes so a hanging Redis command can stall only presence cleanup, never revocation enforcement. Evictions now enqueue cleanup instead of awaiting it, and the scan no longer reads presence for a fallback role. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(realtime): bound authorization waits in the revocation scan Race each socket's authorization check against a per-socket timeout and cap the whole pass with a budget below the sweep interval, so a hanging DB query skips that socket for the pass (never evicting on uncertainty) instead of wedging the scan lane and starving subsequent ticks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(realtime): round-robin the revocation scan so hung checks cannot starve later sockets Resume each scan pass after the last target the previous pass processed, so a fixed prefix of hanging authorization checks can never repeatedly consume the pass budget and leave sockets behind it unexamined. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 9666533 commit f43b52c

14 files changed

Lines changed: 1102 additions & 44 deletions

File tree

Lines changed: 385 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,385 @@
1+
/**
2+
* @vitest-environment node
3+
*
4+
* Tests for the periodic read-access re-validation sweep. The security contract:
5+
* a socket is evicted only when its role resolves to `null` (a confirmed
6+
* revocation), and a transient failure never evicts a still-authorized socket.
7+
*/
8+
import { beforeEach, describe, expect, it, vi } from 'vitest'
9+
10+
const { mockResolveRole } = vi.hoisted(() => ({
11+
mockResolveRole: vi.fn(),
12+
}))
13+
14+
vi.mock('@/middleware/permissions', () => ({
15+
resolveCurrentWorkflowRole: mockResolveRole,
16+
ROLE_REVALIDATION_TTL_MS: 30_000,
17+
}))
18+
19+
import {
20+
ACCESS_REVALIDATION_SWEEP_INTERVAL_MS,
21+
startAccessRevalidationSweep,
22+
} from '@/access-revalidation'
23+
import type { IRoomManager, UserPresence } from '@/rooms'
24+
25+
interface FakeSocket {
26+
id: string
27+
userId?: string
28+
rooms: Set<string>
29+
emit: ReturnType<typeof vi.fn>
30+
leave: ReturnType<typeof vi.fn>
31+
}
32+
33+
function makeSocket(id: string, userId: string | undefined, workflowId?: string): FakeSocket {
34+
const rooms = new Set<string>([id])
35+
if (workflowId) rooms.add(workflowId)
36+
return {
37+
id,
38+
userId,
39+
rooms,
40+
emit: vi.fn(),
41+
// Socket.IO's leave removes the room from `rooms` synchronously.
42+
leave: vi.fn((room: string) => {
43+
rooms.delete(room)
44+
}),
45+
}
46+
}
47+
48+
function makeManager(sockets: FakeSocket[], presence: Partial<UserPresence>[] = []) {
49+
const socketMap = new Map(sockets.map((s) => [s.id, s]))
50+
const manager = {
51+
io: { sockets: { sockets: socketMap } },
52+
isReady: () => true,
53+
getWorkflowUsers: vi.fn().mockResolvedValue(presence),
54+
getWorkflowIdForSocket: vi.fn().mockResolvedValue(null),
55+
removeUserFromRoom: vi
56+
.fn()
57+
.mockImplementation(async (_socketId: string, workflowId?: string) => workflowId ?? null),
58+
broadcastPresenceUpdate: vi.fn().mockResolvedValue(undefined),
59+
}
60+
return manager as unknown as IRoomManager & {
61+
getWorkflowUsers: ReturnType<typeof vi.fn>
62+
getWorkflowIdForSocket: ReturnType<typeof vi.fn>
63+
removeUserFromRoom: ReturnType<typeof vi.fn>
64+
broadcastPresenceUpdate: ReturnType<typeof vi.fn>
65+
}
66+
}
67+
68+
describe('access-revalidation sweep', () => {
69+
beforeEach(() => {
70+
vi.clearAllMocks()
71+
})
72+
73+
it('evicts a socket whose role has been revoked', async () => {
74+
const socket = makeSocket('sock-1', 'user-1', 'wf-1')
75+
const manager = makeManager([socket], [{ socketId: 'sock-1', role: 'read' }])
76+
mockResolveRole.mockResolvedValue(null)
77+
78+
const sweep = startAccessRevalidationSweep(manager)
79+
await sweep.runOnce()
80+
sweep.stop()
81+
82+
expect(socket.emit).toHaveBeenCalledWith(
83+
'access-revoked',
84+
expect.objectContaining({ workflowId: 'wf-1' })
85+
)
86+
expect(socket.leave).toHaveBeenCalledWith('wf-1')
87+
expect(manager.removeUserFromRoom).toHaveBeenCalledWith('sock-1', 'wf-1')
88+
expect(manager.broadcastPresenceUpdate).toHaveBeenCalledWith('wf-1')
89+
})
90+
91+
it('keeps a socket whose access is still valid', async () => {
92+
const socket = makeSocket('sock-1', 'user-1', 'wf-1')
93+
const manager = makeManager([socket], [{ socketId: 'sock-1', role: 'write' }])
94+
mockResolveRole.mockResolvedValue('write')
95+
96+
const sweep = startAccessRevalidationSweep(manager)
97+
await sweep.runOnce()
98+
sweep.stop()
99+
100+
expect(socket.emit).not.toHaveBeenCalled()
101+
expect(socket.leave).not.toHaveBeenCalled()
102+
expect(manager.removeUserFromRoom).not.toHaveBeenCalled()
103+
})
104+
105+
it('does not evict a downgraded-but-still-authorized socket', async () => {
106+
const socket = makeSocket('sock-1', 'user-1', 'wf-1')
107+
const manager = makeManager([socket], [{ socketId: 'sock-1', role: 'admin' }])
108+
// Downgraded admin -> read still resolves to a non-null role: keep the reader.
109+
mockResolveRole.mockResolvedValue('read')
110+
111+
const sweep = startAccessRevalidationSweep(manager)
112+
await sweep.runOnce()
113+
sweep.stop()
114+
115+
expect(socket.emit).not.toHaveBeenCalled()
116+
expect(socket.leave).not.toHaveBeenCalled()
117+
})
118+
119+
it('never evicts when re-validation throws (transient failure)', async () => {
120+
const socket = makeSocket('sock-1', 'user-1', 'wf-1')
121+
const manager = makeManager([socket], [{ socketId: 'sock-1', role: 'read' }])
122+
mockResolveRole.mockRejectedValue(new Error('db unreachable'))
123+
124+
const sweep = startAccessRevalidationSweep(manager)
125+
await sweep.runOnce()
126+
sweep.stop()
127+
128+
expect(socket.emit).not.toHaveBeenCalled()
129+
expect(socket.leave).not.toHaveBeenCalled()
130+
expect(manager.removeUserFromRoom).not.toHaveBeenCalled()
131+
})
132+
133+
it('resolves with the static safe fallback and no presence reads in the scan', async () => {
134+
const socket = makeSocket('sock-1', 'user-1', 'wf-1')
135+
const manager = makeManager([socket], [{ socketId: 'sock-1', role: 'admin' }])
136+
mockResolveRole.mockResolvedValue('admin')
137+
138+
const sweep = startAccessRevalidationSweep(manager)
139+
await sweep.runOnce()
140+
sweep.stop()
141+
142+
expect(mockResolveRole).toHaveBeenCalledWith('user-1', 'wf-1', 'read')
143+
// The security scan must stay Redis-free — presence is never consulted.
144+
expect(manager.getWorkflowUsers).not.toHaveBeenCalled()
145+
})
146+
147+
it('evicts only the revoked socket, not co-members of the room', async () => {
148+
const revoked = makeSocket('sock-1', 'user-1', 'wf-1')
149+
const kept = makeSocket('sock-2', 'user-2', 'wf-1')
150+
const manager = makeManager(
151+
[revoked, kept],
152+
[
153+
{ socketId: 'sock-1', role: 'read' },
154+
{ socketId: 'sock-2', role: 'write' },
155+
]
156+
)
157+
mockResolveRole.mockImplementation(async (userId: string) =>
158+
userId === 'user-1' ? null : 'write'
159+
)
160+
161+
const sweep = startAccessRevalidationSweep(manager)
162+
await sweep.runOnce()
163+
sweep.stop()
164+
165+
expect(revoked.leave).toHaveBeenCalledWith('wf-1')
166+
expect(kept.leave).not.toHaveBeenCalled()
167+
expect(kept.emit).not.toHaveBeenCalled()
168+
})
169+
170+
it('skips unauthenticated sockets and sockets not in a workflow room', async () => {
171+
const noUser = makeSocket('sock-1', undefined, 'wf-1')
172+
const noRoom = makeSocket('sock-2', 'user-2')
173+
const manager = makeManager([noUser, noRoom])
174+
175+
const sweep = startAccessRevalidationSweep(manager)
176+
await sweep.runOnce()
177+
sweep.stop()
178+
179+
expect(mockResolveRole).not.toHaveBeenCalled()
180+
expect(noUser.leave).not.toHaveBeenCalled()
181+
expect(noRoom.leave).not.toHaveBeenCalled()
182+
})
183+
184+
it('defers failed room-state cleanup and retries it on the next pass', async () => {
185+
const socket = makeSocket('sock-1', 'user-1', 'wf-1')
186+
const manager = makeManager([socket], [{ socketId: 'sock-1', role: 'read' }])
187+
manager.removeUserFromRoom.mockRejectedValueOnce(new Error('redis down'))
188+
mockResolveRole.mockResolvedValue(null)
189+
190+
const sweep = startAccessRevalidationSweep(manager)
191+
await sweep.runOnce()
192+
193+
expect(socket.leave).toHaveBeenCalledWith('wf-1')
194+
expect(manager.broadcastPresenceUpdate).not.toHaveBeenCalled()
195+
196+
// The evicted socket left the room, so membership scans no longer see it —
197+
// the retry queue must drive the cleanup to completion.
198+
await sweep.runOnce()
199+
sweep.stop()
200+
201+
expect(manager.removeUserFromRoom).toHaveBeenCalledTimes(2)
202+
expect(manager.broadcastPresenceUpdate).toHaveBeenCalledWith('wf-1')
203+
})
204+
205+
it('defers cleanup when removal fails with expired socket mappings', async () => {
206+
const socket = makeSocket('sock-1', 'user-1', 'wf-1')
207+
const manager = makeManager([socket], [{ socketId: 'sock-1', role: 'read' }])
208+
// Mapping keys already expired (lookup resolves null) AND the removal fails
209+
// (the Redis manager swallows the transport error into null) — the failed
210+
// removal must still defer instead of reading as success.
211+
manager.removeUserFromRoom.mockResolvedValueOnce(null)
212+
mockResolveRole.mockResolvedValue(null)
213+
214+
const sweep = startAccessRevalidationSweep(manager)
215+
await sweep.runOnce()
216+
217+
expect(manager.broadcastPresenceUpdate).not.toHaveBeenCalled()
218+
219+
await sweep.runOnce()
220+
sweep.stop()
221+
222+
expect(manager.removeUserFromRoom).toHaveBeenCalledTimes(2)
223+
expect(manager.broadcastPresenceUpdate).toHaveBeenCalledWith('wf-1')
224+
})
225+
226+
it('defers cleanup when the manager swallows a removal failure into null', async () => {
227+
const socket = makeSocket('sock-1', 'user-1', 'wf-1')
228+
const manager = makeManager([socket], [{ socketId: 'sock-1', role: 'read' }])
229+
// Live mapping but the removal reports nothing removed — the Redis manager
230+
// swallows transport errors into null, so this is the only failure signal.
231+
manager.getWorkflowIdForSocket.mockResolvedValue('wf-1')
232+
manager.removeUserFromRoom.mockResolvedValueOnce(null)
233+
mockResolveRole.mockResolvedValue(null)
234+
235+
const sweep = startAccessRevalidationSweep(manager)
236+
await sweep.runOnce()
237+
238+
expect(socket.leave).toHaveBeenCalledWith('wf-1')
239+
expect(manager.broadcastPresenceUpdate).not.toHaveBeenCalled()
240+
241+
// Next pass: the removal now succeeds and the cleanup completes.
242+
await sweep.runOnce()
243+
sweep.stop()
244+
245+
expect(manager.removeUserFromRoom).toHaveBeenCalledTimes(2)
246+
expect(manager.broadcastPresenceUpdate).toHaveBeenCalledWith('wf-1')
247+
})
248+
249+
it('skips removal when the socket has since moved to a different workflow', async () => {
250+
const socket = makeSocket('sock-1', 'user-1', 'wf-1')
251+
const manager = makeManager([socket], [{ socketId: 'sock-1', role: 'read' }])
252+
// Between the membership snapshot and cleanup, the socket switched to a
253+
// workflow it can still access — removal must not touch its new presence.
254+
manager.getWorkflowIdForSocket.mockResolvedValue('wf-2')
255+
mockResolveRole.mockResolvedValue(null)
256+
257+
const sweep = startAccessRevalidationSweep(manager)
258+
await sweep.runOnce()
259+
sweep.stop()
260+
261+
expect(socket.leave).toHaveBeenCalledWith('wf-1')
262+
expect(manager.removeUserFromRoom).not.toHaveBeenCalled()
263+
expect(manager.broadcastPresenceUpdate).not.toHaveBeenCalled()
264+
})
265+
266+
it('drops a deferred cleanup when the socket legitimately re-joined the room', async () => {
267+
const socket = makeSocket('sock-1', 'user-1', 'wf-1')
268+
const manager = makeManager([socket], [{ socketId: 'sock-1', role: 'read' }])
269+
manager.removeUserFromRoom.mockRejectedValueOnce(new Error('redis down'))
270+
mockResolveRole.mockResolvedValueOnce(null)
271+
272+
const sweep = startAccessRevalidationSweep(manager)
273+
await sweep.runOnce()
274+
expect(socket.leave).toHaveBeenCalledWith('wf-1')
275+
276+
// Access restored and the socket re-joined the same room: the retry must
277+
// NOT remove the fresh presence entry that re-join created.
278+
socket.rooms.add('wf-1')
279+
mockResolveRole.mockResolvedValue('read')
280+
await sweep.runOnce()
281+
sweep.stop()
282+
283+
expect(manager.removeUserFromRoom).toHaveBeenCalledTimes(1)
284+
expect(manager.broadcastPresenceUpdate).not.toHaveBeenCalled()
285+
})
286+
287+
it('skips a socket whose authorization query hangs and still evicts the rest', async () => {
288+
vi.useFakeTimers()
289+
try {
290+
const hung = makeSocket('sock-1', 'user-1', 'wf-1')
291+
const revoked = makeSocket('sock-2', 'user-2', 'wf-1')
292+
const manager = makeManager([hung, revoked])
293+
// user-1's authorization query hangs (wedged DB connection); user-2's
294+
// resolves to a confirmed revocation.
295+
mockResolveRole.mockImplementation(async (userId: string) => {
296+
if (userId === 'user-1') return new Promise(() => {})
297+
return null
298+
})
299+
300+
const sweep = startAccessRevalidationSweep(manager)
301+
302+
// First tick starts the scan; the per-socket timeout fires at +5s and the
303+
// scan moves on to evict the revoked socket in the same pass.
304+
await vi.advanceTimersByTimeAsync(ACCESS_REVALIDATION_SWEEP_INTERVAL_MS)
305+
await vi.advanceTimersByTimeAsync(10_000)
306+
307+
expect(hung.leave).not.toHaveBeenCalled()
308+
expect(revoked.leave).toHaveBeenCalledWith('wf-1')
309+
310+
// The next tick's scan still runs — the hung query did not wedge the lane.
311+
const callsAfterFirstPass = mockResolveRole.mock.calls.length
312+
await vi.advanceTimersByTimeAsync(ACCESS_REVALIDATION_SWEEP_INTERVAL_MS)
313+
await vi.advanceTimersByTimeAsync(10_000)
314+
sweep.stop()
315+
316+
expect(mockResolveRole.mock.calls.length).toBeGreaterThan(callsAfterFirstPass)
317+
} finally {
318+
vi.useRealTimers()
319+
}
320+
})
321+
322+
it('rotates the scan start so hung checks cannot starve later sockets', async () => {
323+
vi.useFakeTimers()
324+
try {
325+
// Four hung authorization checks consume exactly the 20s pass budget
326+
// (4 × 5s per-socket timeout); the revoked socket sits behind them.
327+
const hungSockets = [1, 2, 3, 4].map((i) => makeSocket(`sock-${i}`, `user-${i}`, 'wf-1'))
328+
const revoked = makeSocket('sock-5', 'user-5', 'wf-1')
329+
const manager = makeManager([...hungSockets, revoked])
330+
mockResolveRole.mockImplementation(async (userId: string) => {
331+
if (userId === 'user-5') return null
332+
return new Promise(() => {})
333+
})
334+
335+
const sweep = startAccessRevalidationSweep(manager)
336+
337+
// First pass burns its whole budget on the hung prefix.
338+
await vi.advanceTimersByTimeAsync(ACCESS_REVALIDATION_SWEEP_INTERVAL_MS)
339+
await vi.advanceTimersByTimeAsync(25_000)
340+
expect(revoked.leave).not.toHaveBeenCalled()
341+
342+
// Second pass resumes after the last processed socket, so the revoked
343+
// socket is examined first and evicted.
344+
await vi.advanceTimersByTimeAsync(10_000)
345+
sweep.stop()
346+
347+
expect(revoked.leave).toHaveBeenCalledWith('wf-1')
348+
} finally {
349+
vi.useRealTimers()
350+
}
351+
})
352+
353+
it('keeps scanning on later ticks while a deferred cleanup hangs', async () => {
354+
vi.useFakeTimers()
355+
try {
356+
const socket = makeSocket('sock-1', 'user-1', 'wf-1')
357+
const manager = makeManager([socket], [{ socketId: 'sock-1', role: 'read' }])
358+
// A Redis outage where commands hang in the offline queue instead of
359+
// failing: the cleanup lane stalls, but scans must keep running.
360+
manager.getWorkflowIdForSocket.mockReturnValue(new Promise(() => {}))
361+
mockResolveRole.mockResolvedValue(null)
362+
363+
const sweep = startAccessRevalidationSweep(manager)
364+
365+
await vi.advanceTimersByTimeAsync(ACCESS_REVALIDATION_SWEEP_INTERVAL_MS)
366+
expect(socket.leave).toHaveBeenCalledWith('wf-1')
367+
const scansAfterFirstTick = mockResolveRole.mock.calls.length
368+
369+
// Second socket appears while the first eviction's cleanup hangs.
370+
const second = makeSocket('sock-2', 'user-2', 'wf-1')
371+
const socketMap = manager.io.sockets.sockets as unknown as Map<string, FakeSocket>
372+
socketMap.set('sock-2', second)
373+
374+
await vi.advanceTimersByTimeAsync(ACCESS_REVALIDATION_SWEEP_INTERVAL_MS)
375+
sweep.stop()
376+
377+
// The hung cleanup did not block the next scan: the new socket was
378+
// evaluated and evicted.
379+
expect(mockResolveRole.mock.calls.length).toBeGreaterThan(scansAfterFirstTick)
380+
expect(second.leave).toHaveBeenCalledWith('wf-1')
381+
} finally {
382+
vi.useRealTimers()
383+
}
384+
})
385+
})

0 commit comments

Comments
 (0)