From 498f38b680c0d19770689677dd4f6c7c5ba543f2 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 1 Aug 2026 12:17:00 -0700 Subject: [PATCH 01/11] fix(realtime): enforce room access continuously, not only at join MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit File-doc, table, and workspace-list rooms authorized once at JOIN and never again, so a member whose workspace access was revoked or downgraded kept live collaborative write access — including durable Yjs document writes — for the whole lifetime of an already-open socket. The access-revalidation sweep explicitly skipped every non-workflow room. - sweep every room type, authorizing each against its own resource - share one membership policy (ROOM_MEMBERSHIP_ACTIONS) between the join check and the sweep, so a file-doc room keeps requiring write in both - gate file-doc document frames and table cell selections on the cached permission, evicting on a confirmed loss of access - re-check the cached decision before a join commits, so a join that authorized just before a revocation cannot re-enter the room - never let a join's own cache write clobber a revocation recorded mid-flight - surface room-access-revoked to clients; the file-doc editor falls back to read-only instead of accepting keystrokes that go nowhere --- apps/realtime/src/access-revalidation.test.ts | 131 +++++++++-- apps/realtime/src/access-revalidation.ts | 216 +++++++++++------- apps/realtime/src/handlers/file-doc.test.ts | 107 +++++++++ apps/realtime/src/handlers/file-doc.ts | 108 ++++++++- apps/realtime/src/handlers/room-eviction.ts | 42 ++++ apps/realtime/src/handlers/room-join-auth.ts | 15 ++ apps/realtime/src/handlers/tables.test.ts | 72 ++++++ apps/realtime/src/handlers/tables.ts | 93 +++++++- .../handlers/workspace-invalidation-room.ts | 3 +- apps/realtime/src/middleware/permissions.ts | 178 ++++++++++++--- .../collaboration/file-doc-provider.ts | 31 +++ packages/platform-authz/package.json | 4 + packages/platform-authz/src/predicates.ts | 12 + packages/platform-authz/src/room-policy.ts | 44 ++++ packages/realtime-protocol/src/events.ts | 22 ++ 15 files changed, 946 insertions(+), 132 deletions(-) create mode 100644 apps/realtime/src/handlers/room-eviction.ts create mode 100644 packages/platform-authz/src/room-policy.ts diff --git a/apps/realtime/src/access-revalidation.test.ts b/apps/realtime/src/access-revalidation.test.ts index 4ea7999c301..a6ae605bffd 100644 --- a/apps/realtime/src/access-revalidation.test.ts +++ b/apps/realtime/src/access-revalidation.test.ts @@ -1,10 +1,13 @@ /** * @vitest-environment node * - * Tests for the periodic read-access re-validation sweep. The security contract: - * a socket is evicted only when its role resolves to `null` (a confirmed - * revocation), and a transient failure never evicts a still-authorized socket. + * Tests for the periodic access re-validation sweep, which covers EVERY room type + * a socket occupies. The security contract: a socket is evicted only when its + * permission definitively fails the level that room requires (a confirmed + * revocation or downgrade), and a transient failure never evicts a still-authorized + * socket. */ +import { ROOM_TYPES } from '@sim/realtime-protocol/rooms' import { beforeEach, describe, expect, it, vi } from 'vitest' const { mockResolveRole } = vi.hoisted(() => ({ @@ -12,7 +15,7 @@ const { mockResolveRole } = vi.hoisted(() => ({ })) vi.mock('@/middleware/permissions', () => ({ - resolveCurrentWorkflowRole: mockResolveRole, + resolveCurrentRoomPermission: mockResolveRole, ROLE_REVALIDATION_TTL_MS: 30_000, })) @@ -20,6 +23,7 @@ import { ACCESS_REVALIDATION_SWEEP_INTERVAL_MS, startAccessRevalidationSweep, } from '@/access-revalidation' +import { registerRoomEvictionHandler } from '@/handlers/room-eviction' import type { IRoomManager, UserPresence } from '@/rooms' interface FakeSocket { @@ -30,9 +34,9 @@ interface FakeSocket { leave: ReturnType } -function makeSocket(id: string, userId: string | undefined, workflowId?: string): FakeSocket { +function makeSocket(id: string, userId: string | undefined, room?: string): FakeSocket { const rooms = new Set([id]) - if (workflowId) rooms.add(workflowId) + if (room) rooms.add(room) return { id, userId, @@ -131,7 +135,7 @@ describe('access-revalidation sweep', () => { expect(manager.removeUserFromRoom).not.toHaveBeenCalled() }) - it('resolves with the static safe fallback and no presence reads in the scan', async () => { + it('resolves with the room safe fallback and no presence reads in the scan', async () => { const socket = makeSocket('sock-1', 'user-1', 'wf-1') const manager = makeManager([socket], [{ socketId: 'sock-1', role: 'admin' }]) mockResolveRole.mockResolvedValue('admin') @@ -140,32 +144,123 @@ describe('access-revalidation sweep', () => { await sweep.runOnce() sweep.stop() - expect(mockResolveRole).toHaveBeenCalledWith('user-1', 'wf-1', 'read') + expect(mockResolveRole).toHaveBeenCalledWith('user-1', { type: 'workflow', id: 'wf-1' }, 'read') // The security scan must stay Redis-free — presence is never consulted. expect(manager.getRoomUsers).not.toHaveBeenCalled() }) - it('never evicts a socket joined only to a non-workflow room (files/tables/file-doc)', async () => { - // The sweep shares one io with the files/tables/file-doc handlers. Those rooms are - // namespaced (`workspace-files:ws-1`, `table:t-1`), so treating every socket.rooms - // entry as a workflow id would resolve a bogus permission → null → evict the socket - // from its files/table room every pass. Non-workflow rooms must be filtered out. + it('sweeps non-workflow rooms against their own resource, not a bogus workflow id', async () => { + // The sweep shares one io with the files/tables/file-doc handlers. Their rooms are + // namespaced (`workspace-files:ws-1`, `table:t-1`), so each name is decoded and + // authorized as its own room type — the whole point of covering them at all. const filesSocket = makeSocket('sock-1', 'user-1', 'workspace-files:ws-1') const tableSocket = makeSocket('sock-2', 'user-2', 'table:t-1') const manager = makeManager([filesSocket, tableSocket]) - // Even if the role resolver would say "no access", these must never be swept. - mockResolveRole.mockResolvedValue(null) + mockResolveRole.mockResolvedValue('write') const sweep = startAccessRevalidationSweep(manager) await sweep.runOnce() sweep.stop() - expect(mockResolveRole).not.toHaveBeenCalled() + expect(mockResolveRole).toHaveBeenCalledWith( + 'user-1', + { type: 'workspace-files', id: 'ws-1' }, + 'read' + ) + expect(mockResolveRole).toHaveBeenCalledWith('user-2', { type: 'table', id: 't-1' }, 'read') + // Still authorized: nobody is evicted. expect(filesSocket.leave).not.toHaveBeenCalled() - expect(filesSocket.emit).not.toHaveBeenCalled() expect(tableSocket.leave).not.toHaveBeenCalled() - expect(tableSocket.emit).not.toHaveBeenCalled() + }) + + it('evicts a revoked socket from a presence-free workspace-files room without touching presence', async () => { + const socket = makeSocket('sock-1', 'user-1', 'workspace-files:ws-1') + const manager = makeManager([socket]) + mockResolveRole.mockResolvedValue(null) + + const sweep = startAccessRevalidationSweep(manager) + await sweep.runOnce() + sweep.stop() + + expect(socket.emit).toHaveBeenCalledWith( + 'room-access-revoked', + expect.objectContaining({ room: { type: 'workspace-files', id: 'ws-1' } }) + ) + expect(socket.leave).toHaveBeenCalledWith('workspace-files:ws-1') + // These rooms hold no room-manager presence, so nothing is owed to the cleanup lane. expect(manager.removeUserFromRoom).not.toHaveBeenCalled() + expect(manager.broadcastPresenceUpdate).not.toHaveBeenCalled() + }) + + it('evicts a revoked socket from a table room and clears its presence', async () => { + const socket = makeSocket('sock-1', 'user-1', 'table:t-1') + const manager = makeManager([socket], [{ socketId: 'sock-1', role: 'read' }]) + mockResolveRole.mockResolvedValue(null) + + const sweep = startAccessRevalidationSweep(manager) + await sweep.runOnce() + sweep.stop() + + expect(socket.leave).toHaveBeenCalledWith('table:t-1') + expect(manager.removeUserFromRoom).toHaveBeenCalledWith({ type: 'table', id: 't-1' }, 'sock-1') + expect(manager.broadcastPresenceUpdate).toHaveBeenCalledWith({ type: 'table', id: 't-1' }) + }) + + it('evicts a file-doc socket downgraded to read, and keeps its table room', async () => { + // A file-doc room IS the editor and requires `write`; a table room requires only + // `read`. One downgraded user in both rooms must lose exactly the document. + const socket = makeSocket('sock-1', 'user-1', 'workspace-file-doc:file-1') + socket.rooms.add('table:t-1') + const manager = makeManager([socket]) + mockResolveRole.mockResolvedValue('read') + + const sweep = startAccessRevalidationSweep(manager) + await sweep.runOnce() + sweep.stop() + + expect(socket.leave).toHaveBeenCalledWith('workspace-file-doc:file-1') + expect(socket.leave).not.toHaveBeenCalledWith('table:t-1') + expect(socket.emit).toHaveBeenCalledWith( + 'room-access-revoked', + expect.objectContaining({ room: { type: 'workspace-file-doc', id: 'file-1' } }) + ) + }) + + it('falls back to the room type own membership level on a cold-cache failure', async () => { + // A static 'read' fallback would have evicted every file-doc socket (which needs + // `write`) the first time the DB blipped with a cold cache. + const socket = makeSocket('sock-1', 'user-1', 'workspace-file-doc:file-1') + const manager = makeManager([socket]) + mockResolveRole.mockResolvedValue('write') + + const sweep = startAccessRevalidationSweep(manager) + await sweep.runOnce() + sweep.stop() + + expect(mockResolveRole).toHaveBeenCalledWith( + 'user-1', + { type: 'workspace-file-doc', id: 'file-1' }, + 'write' + ) + expect(socket.leave).not.toHaveBeenCalled() + }) + + it('runs the room type registered eviction handler so handler-local state is dropped', async () => { + const evicted = vi.fn() + registerRoomEvictionHandler(ROOM_TYPES.WORKSPACE_FILE_DOC, evicted) + const socket = makeSocket('sock-1', 'user-1', 'workspace-file-doc:file-1') + const manager = makeManager([socket]) + mockResolveRole.mockResolvedValue(null) + + const sweep = startAccessRevalidationSweep(manager) + await sweep.runOnce() + sweep.stop() + + expect(evicted).toHaveBeenCalledWith( + 'sock-1', + { type: 'workspace-file-doc', id: 'file-1' }, + manager.io + ) }) it('evicts only the revoked socket, not co-members of the room', async () => { diff --git a/apps/realtime/src/access-revalidation.ts b/apps/realtime/src/access-revalidation.ts index 93edaae9a0b..5bbc4381d66 100644 --- a/apps/realtime/src/access-revalidation.ts +++ b/apps/realtime/src/access-revalidation.ts @@ -1,10 +1,22 @@ import { createLogger } from '@sim/logger' -import type { AccessRevokedBroadcast } from '@sim/realtime-protocol/events' -import { parseRoomName, ROOM_TYPES } from '@sim/realtime-protocol/rooms' +import { ROOM_MEMBERSHIP_ACTIONS, satisfiesRoomMembership } from '@sim/platform-authz/room-policy' +import { + type AccessRevokedBroadcast, + ROOM_ACCESS_REVOKED_EVENT, + type RoomAccessRevokedBroadcast, +} from '@sim/realtime-protocol/events' +import { + parseRoomName, + ROOM_TYPES, + type RoomRef, + type RoomType, + roomName, +} from '@sim/realtime-protocol/rooms' import { sleep } from '@sim/utils/helpers' +import { runRoomEvictionHandler } from '@/handlers/room-eviction' import type { AuthenticatedSocket } from '@/middleware/auth' -import { ROLE_REVALIDATION_TTL_MS, resolveCurrentWorkflowRole } from '@/middleware/permissions' -import { type IRoomManager, workflowRoom as wf } from '@/rooms' +import { ROLE_REVALIDATION_TTL_MS, resolveCurrentRoomPermission } from '@/middleware/permissions' +import type { IRoomManager } from '@/rooms' const logger = createLogger('AccessRevalidation') @@ -21,14 +33,30 @@ const logger = createLogger('AccessRevalidation') export const ACCESS_REVALIDATION_SWEEP_INTERVAL_MS = ROLE_REVALIDATION_TTL_MS /** - * Non-null fallback consumed by {@link resolveCurrentWorkflowRole} only on a - * transient DB failure with a cold cache, where returning a non-null role - * (never eviction) is the safe outcome during an outage. The scan deliberately - * does not read presence for a per-socket join-time role: that would put a - * Redis dependency inside the security-critical scan lane, and the fallback's - * only job is to be non-null. + * Fallback consumed by {@link resolveCurrentRoomPermission} only on a transient DB + * failure with a cold cache, where resolving to a permission that KEEPS the socket + * (never eviction) is the safe outcome during an outage. It is the room's own + * membership level, so it satisfies that room's requirement exactly — a static + * `'read'` would have evicted every file-doc socket (which needs `write`) on a cold + * cache blip. The scan deliberately does not read presence for a per-socket + * join-time role: that would put a Redis dependency inside the security-critical + * scan lane, and the fallback's only job is to be permissive. */ -const FALLBACK_ROLE = 'read' +function fallbackRoleFor(type: RoomType): string { + return ROOM_MEMBERSHIP_ACTIONS[type] +} + +/** + * Room types whose membership is mirrored in the room manager's (Redis) presence + * state, and therefore need a presence removal + rebroadcast after an eviction. + * The workspace-files / workspace-tables invalidation rooms carry no presence at + * all, and a file-doc room's roster is pod-local in-memory state reconciled by its + * registered eviction handler — neither has anything for the cleanup lane to do. + */ +const PRESENCE_ROOM_TYPES: ReadonlySet = new Set([ + ROOM_TYPES.WORKFLOW, + ROOM_TYPES.TABLE, +]) /** * Upper bound on a single socket's authorization check inside the scan. A DB @@ -57,36 +85,39 @@ export interface AccessRevalidationSweep { } interface ScanTarget { - workflowId: string + room: RoomRef + /** The Socket.IO room name — the key the socket actually holds. */ + name: string socket: AuthenticatedSocket userId: string } /** - * Collects this pod's authenticated sockets with the workflow room each has + * Collects this pod's authenticated sockets paired with EVERY room each has * joined, in stable socket order. * * Rooms are derived from the socket's own `rooms` set (pod-local, no Redis * round-trips). A socket may occupy several rooms of different types at once - * (workflow canvas, workspace-files browser, table, file-doc), all on the same - * io — so each name is decoded with {@link parseRoomName} and only **workflow** - * rooms are swept here. Non-workflow room names are namespaced (`type:id`) and - * resolve to a non-workflow type; sweeping them as workflow ids would resolve a - * bogus permission, come back `null`, and spuriously evict the socket from its - * files/table room every pass. Only local sockets are evaluated — sockets are - * sticky to a pod, so every socket is swept by exactly one pod using that pod's - * warm role cache (mirroring the per-pod reasoning of the write-path cache). + * (workflow canvas, workspace-files browser, table, file-doc), all on the same io + * — so each name is decoded with {@link parseRoomName} and swept as its own type. + * Decoding (rather than assuming workflow) is what makes this safe: a namespaced + * `type:id` name resolves to the right resource, so it is authorized against that + * resource's workspace instead of resolving a bogus workflow permission. + * + * Only local sockets are evaluated — sockets are sticky to a pod, so every socket + * is swept by exactly one pod using that pod's warm role cache (mirroring the + * per-pod reasoning of the write-path cache). */ function collectScanTargets(io: IRoomManager['io']): ScanTarget[] { const targets: ScanTarget[] = [] for (const socket of io.sockets.sockets.values()) { const authed = socket as AuthenticatedSocket if (!authed.userId) continue - for (const room of socket.rooms) { - if (room === socket.id) continue - const ref = parseRoomName(room) - if (ref?.type !== ROOM_TYPES.WORKFLOW) continue - targets.push({ workflowId: ref.id, socket: authed, userId: authed.userId }) + for (const name of socket.rooms) { + if (name === socket.id) continue + const ref = parseRoomName(name) + if (!ref) continue + targets.push({ room: ref, name, socket: authed, userId: authed.userId }) } } return targets @@ -94,14 +125,19 @@ function collectScanTargets(io: IRoomManager['io']): ScanTarget[] { /** * Starts a per-pod loop that re-validates every connected socket's workspace - * role and evicts sockets whose access has been revoked, closing the read-side - * gap left by the join-only access check. + * permission — for EVERY room type it occupies — and evicts sockets whose access + * no longer satisfies the room, closing the gap left by the join-only access + * check. Without it, a member removed or downgraded mid-session keeps live + * collaborative access (including durable document writes) for the whole lifetime + * of an already-open socket. * - * Blip-safety: eviction fires *only* when {@link resolveCurrentWorkflowRole} - * returns `null`, which happens solely for a successful DB "no access" result or - * a previously-recorded revocation reused across a failure. A transient DB error - * against a still-authorized (or freshly-joined) user resolves to the last-known - * or fallback role, so a database blip never evicts anyone. + * Blip-safety: eviction fires *only* when {@link resolveCurrentRoomPermission} + * resolves to a permission that does not satisfy the room's membership level, + * which happens solely for a successful DB result (no access, or a level below + * the room's requirement) or a previously-recorded revocation reused across a + * failure. A transient DB error against a still-authorized (or freshly-joined) + * user resolves to the last-known or the room's own fallback level, so a database + * blip never evicts anyone. * * Liveness: the loop runs as two independently-guarded lanes. The security scan * (local sockets + DB role checks + emit/leave) touches no Redis at all; the @@ -118,7 +154,7 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR let scanRunning = false let cleanupRunning = false /** - * Round-robin cursor: the `${socketId}:${workflowId}` key of the last target + * Round-robin cursor: the `${socketId}:${roomName}` key of the last target * the previous pass processed. Each pass resumes after it, so a fixed prefix * of hanging authorization checks can never starve the sockets behind it — * every target is examined within a bounded number of passes. @@ -126,16 +162,17 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR let scanCursorKey: string | null = null /** - * Room-state cleanups owed for evicted sockets, keyed - * `${socketId}:${workflowId}`. Every eviction enqueues here (the evicted - * socket has already left the Socket.IO room, so membership scans will never - * see it again); the cleanup lane drains the queue until each removal is - * confirmed, so remaining collaborators do not keep a stale presence entry. + * Presence cleanups owed for evicted sockets, keyed `${socketId}:${roomName}`. + * Every eviction from a presence-backed room enqueues here (the evicted socket + * has already left the Socket.IO room, so membership scans will never see it + * again); the cleanup lane drains the queue until each removal is confirmed, so + * remaining collaborators do not keep a stale presence entry. */ - const pendingCleanups = new Map() + const pendingCleanups = new Map() - async function cleanupEvictedSocket(socketId: string, workflowId: string): Promise { - const key = `${socketId}:${workflowId}` + async function cleanupEvictedSocket(socketId: string, room: RoomRef): Promise { + const name = roomName(room) + const key = `${socketId}:${name}` try { // A fully-disconnected socket already had its presence removed by the // disconnect handler (removeSocketFromAllRooms), so there is nothing left to @@ -148,13 +185,15 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR } // Unlike removeUserFromRoom, this read does not swallow transport errors, - // so a Redis outage lands in the catch below and defers the cleanup. - const currentRoom = await roomManager.getRoomForSocket(socketId, ROOM_TYPES.WORKFLOW) - const currentWorkflowId = currentRoom?.id ?? null - if (currentWorkflowId !== null && currentWorkflowId !== workflowId) { - // The socket has since moved to a different workflow it can still - // access; that join's room switch already removed this room's presence - // entry, so there is nothing stale left to clean here. + // so a Redis outage lands in the catch below and defers the cleanup. The + // lookup is per room TYPE (a socket holds at most one room of each), so a + // socket that also sits in an unrelated room type is unaffected. + const currentRoom = await roomManager.getRoomForSocket(socketId, room.type) + const currentRoomId = currentRoom?.id ?? null + if (currentRoomId !== null && currentRoomId !== room.id) { + // The socket has since moved to a different room of this type that it can + // still access; that join's room switch already removed this room's + // presence entry, so there is nothing stale left to clean here. pendingCleanups.delete(key) return } @@ -162,7 +201,7 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR // Synchronous re-join guard with no awaits before the removal: if the // socket legitimately re-joined this room after the eviction (access // restored), that join re-added its presence — removal would erase it. - if (io.sockets.sockets.get(socketId)?.rooms.has(workflowId)) { + if (io.sockets.sockets.get(socketId)?.rooms.has(name)) { pendingCleanups.delete(key) return } @@ -170,7 +209,7 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR // A null mapping here is the normal case (the socket's mapping key may have // expired) and does NOT mean "skip" — the eviction still removes the presence // entry from the known target room via the explicit ref below. - const removed = await roomManager.removeUserFromRoom(wf(workflowId), socketId) + const removed = await roomManager.removeUserFromRoom(room, socketId) if (!removed) { // `false` conflates two outcomes: the entry was already gone (a no-op), or a // transport error the manager swallowed. Only retry when the socket is still mapped @@ -179,27 +218,27 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR // the presence entry is already gone, so the cleanup is complete: dropping it avoids // re-enqueuing a still-connected socket forever. (A real Redis outage throws at // getRoomForSocket and is deferred by the outer catch, never reaching here.) - if (currentWorkflowId === workflowId) { + if (currentRoomId === room.id) { throw new Error('room-state removal not confirmed') } pendingCleanups.delete(key) return } - await roomManager.broadcastPresenceUpdate(wf(workflowId)) + await roomManager.broadcastPresenceUpdate(room) pendingCleanups.delete(key) } catch (error) { - pendingCleanups.set(key, { socketId, workflowId }) + pendingCleanups.set(key, { socketId, room }) logger.warn( - `Room-state cleanup failed for evicted socket ${socketId} on ${workflowId}; will retry next sweep`, + `Room-state cleanup failed for evicted socket ${socketId} on ${name}; will retry next sweep`, error ) } } async function drainPendingCleanups(): Promise { - for (const [, { socketId, workflowId }] of pendingCleanups) { - await cleanupEvictedSocket(socketId, workflowId) + for (const [, { socketId, room }] of pendingCleanups) { + await cleanupEvictedSocket(socketId, room) } } @@ -220,23 +259,35 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR }) } - function revokeSocket(socket: AuthenticatedSocket, workflowId: string): void { + function revokeSocket(socket: AuthenticatedSocket, room: RoomRef, name: string): void { // Security-critical, pod-local, and synchronous: stop this socket receiving - // room broadcasts immediately. Room-state cleanup is only enqueued here — - // the cleanup lane performs the Redis work, so eviction never blocks on it. - const payload: AccessRevokedBroadcast = { - workflowId, - message: 'Your access to this workflow has been revoked', - timestamp: Date.now(), + // room broadcasts immediately, and drop the handler-local state that would + // otherwise still accept its frames (a file-doc socket's room binding is what + // gates its document writes). Redis presence cleanup is only ENQUEUED here — + // the cleanup lane performs that work, so eviction never blocks on it. + if (room.type === ROOM_TYPES.WORKFLOW) { + const payload: AccessRevokedBroadcast = { + workflowId: room.id, + message: 'Your access to this workflow has been revoked', + timestamp: Date.now(), + } + socket.emit('access-revoked', payload) + } else { + const payload: RoomAccessRevokedBroadcast = { + room, + message: 'Your access to this resource has been revoked', + timestamp: Date.now(), + } + socket.emit(ROOM_ACCESS_REVOKED_EVENT, payload) } - socket.emit('access-revoked', payload) - socket.leave(workflowId) + socket.leave(name) + runRoomEvictionHandler(socket.id, room, io) - logger.info( - `Revoked live access for user ${socket.userId} on workflow ${workflowId} (socket ${socket.id})` - ) + logger.info(`Revoked live access for user ${socket.userId} on ${name} (socket ${socket.id})`) - pendingCleanups.set(`${socket.id}:${workflowId}`, { socketId: socket.id, workflowId }) + if (PRESENCE_ROOM_TYPES.has(room.type)) { + pendingCleanups.set(`${socket.id}:${name}`, { socketId: socket.id, room }) + } } async function scanMemberships(): Promise { @@ -246,7 +297,7 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR let startIndex = 0 if (scanCursorKey !== null) { const cursorIndex = targets.findIndex( - ({ socket, workflowId }) => `${socket.id}:${workflowId}` === scanCursorKey + ({ socket, name }) => `${socket.id}:${name}` === scanCursorKey ) if (cursorIndex !== -1) { startIndex = (cursorIndex + 1) % targets.length @@ -256,7 +307,7 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR const deadline = Date.now() + SCAN_PASS_BUDGET_MS for (let offset = 0; offset < targets.length; offset++) { - const { workflowId, socket, userId } = targets[(startIndex + offset) % targets.length] + const { room, name, socket, userId } = targets[(startIndex + offset) % targets.length] const remainingBudget = deadline - Date.now() if (remainingBudget <= 0) { @@ -272,25 +323,30 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR // resolution keeps running in the background and is re-raced when the // rotation returns to this socket, so it is acted on once it settles. const role = await Promise.race([ - resolveCurrentWorkflowRole(userId, workflowId, FALLBACK_ROLE), + resolveCurrentRoomPermission(userId, room, fallbackRoleFor(room.type)), sleep(Math.min(SCAN_SOCKET_TIMEOUT_MS, remainingBudget)).then(() => SCAN_TIMED_OUT), ]) - if (role === SCAN_TIMED_OUT) { + // {@link SCAN_TIMED_OUT} is the only symbol this race can yield; matching on + // the type narrows it out of the permission comparison below. + if (typeof role === 'symbol') { logger.warn( - `Authorization check timed out for user ${userId} on workflow ${workflowId}; skipping this pass` + `Authorization check timed out for user ${userId} on ${name}; skipping this pass` ) - } else if (role === null) { - revokeSocket(socket, workflowId) + } else if (!satisfiesRoomMembership(role, room.type)) { + // Covers both revocation (`null`) and downgrade below the level this room + // requires — a member dropped to `read` may keep a table room but not the + // collaborative document editor, exactly as at join time. + revokeSocket(socket, room, name) } } catch (error) { - // Never evict on an unexpected error — only a definitive `null` role + // Never evict on an unexpected error — only a definitive resolved permission // evicts, so a failure here leaves the socket's access intact. logger.warn( - `Access re-validation failed for user ${userId} on workflow ${workflowId}; leaving membership intact`, + `Access re-validation failed for user ${userId} on ${name}; leaving membership intact`, error ) } finally { - scanCursorKey = `${socket.id}:${workflowId}` + scanCursorKey = `${socket.id}:${name}` } } } diff --git a/apps/realtime/src/handlers/file-doc.test.ts b/apps/realtime/src/handlers/file-doc.test.ts index d56952bc619..b2c683c590c 100644 --- a/apps/realtime/src/handlers/file-doc.test.ts +++ b/apps/realtime/src/handlers/file-doc.test.ts @@ -6,6 +6,7 @@ import { FILE_DOC_MESSAGE_TYPE, FILE_DOC_SEED, } from '@sim/realtime-protocol/file-doc' +import { ROOM_TYPES } from '@sim/realtime-protocol/rooms' import * as decoding from 'lib0/decoding' import * as encoding from 'lib0/encoding' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' @@ -38,6 +39,7 @@ import { flushAllFileDocRooms, setupWorkspaceFileDocHandlers, } from '@/handlers/file-doc' +import { recordRoomPermission } from '@/middleware/permissions' type Handler = (payload?: unknown) => Promise | void @@ -244,6 +246,30 @@ describe('setupWorkspaceFileDocHandlers', () => { expect(mockAuthorizeRoom).not.toHaveBeenCalled() }) + it('does not re-enter the room when access was revoked while the join was in flight', async () => { + // The sweep records a revocation before it evicts, so a join whose authorize + // completed just before that must not put the socket back in the document. + const { io } = createIo() + const { socket, handlers } = setup('socket-race', io, { userId: 'user-race' }) + + mockAuthorizeRoom.mockImplementation(async () => { + // Simulate the revocation landing between this join's authorize and its commit, + // exactly as the sweep would record it. + recordRoomPermission('user-race', { type: ROOM_TYPES.WORKSPACE_FILE_DOC, id: 'file-1' }, null) + return { allowed: true, status: 200, workspaceId: 'ws-1', workspacePermission: 'write' } + }) + + await handlers[FILE_DOC_EVENTS.JOIN]({ fileId: 'file-1', clientId: 1 }) + await flushMicrotasks() + + expect(socket.emit).toHaveBeenCalledWith( + FILE_DOC_EVENTS.JOIN_ERROR, + expect.objectContaining({ code: 'ACCESS_DENIED', retryable: false }) + ) + expect(socket.join).not.toHaveBeenCalled() + expect(joinSuccessFileId(socket)).toBeUndefined() + }) + it('requires write permission and reports 404 as NOT_FOUND', async () => { mockAuthorizeRoom.mockResolvedValue({ allowed: false, status: 404, workspacePermission: null }) const { io } = createIo() @@ -299,6 +325,87 @@ describe('setupWorkspaceFileDocHandlers', () => { expect(mockFetchFileDocPersist).toHaveBeenCalled() }) + it('drops document frames and evicts once the editor loses write access mid-session', async () => { + // The join-time check is not a standing right: a collaborator downgraded to `read` + // (or removed) must stop landing durable edits on the socket they already hold. + // A distinct user/file so the recorded revocation — written under fake timers, so it + // outlives this test in real time — cannot leak into siblings through the + // module-global role cache. + vi.useFakeTimers() + try { + mockFetchFileDocSeed.mockResolvedValue(seedResult('# From server')) + const { io, sent } = createIo() + const { socket, handlers } = setup('socket-revoked', io, { userId: 'user-revoked' }) + await handlers[FILE_DOC_EVENTS.JOIN]({ fileId: 'file-revoked', clientId: 1 }) + await vi.advanceTimersByTimeAsync(0) + + // Access is downgraded to read-only, and the cached join-time decision expires. + mockAuthorizeRoom.mockResolvedValue({ + allowed: false, + status: 403, + workspaceId: 'ws-1', + workspacePermission: 'read', + }) + await vi.advanceTimersByTimeAsync(31_000) + + const edit = new Y.Doc() + edit.getText(FILE_DOC_FIELD).insert(0, 'edit after revocation') + const editFrame = () => + handlers[FILE_DOC_EVENTS.MESSAGE]( + frame(FILE_DOC_MESSAGE_TYPE.SYNC, (e) => + syncProtocol.writeUpdate(e, Y.encodeStateAsUpdate(edit)) + ) + ) + + // The first frame after expiry finds nothing cached, so it is accepted and kicks + // off the authoritative re-read (never a synchronous DB wait on the relay path). + editFrame() + await vi.advanceTimersByTimeAsync(0) + + // The next frame is gated on the now-authoritative denial: dropped, and the socket + // is evicted rather than left holding the room. + editFrame() + await vi.advanceTimersByTimeAsync(0) + + expect(socket.emit).toHaveBeenCalledWith( + 'room-access-revoked', + expect.objectContaining({ room: { type: 'workspace-file-doc', id: 'file-revoked' } }) + ) + expect(socket.leave).toHaveBeenCalledWith('workspace-file-doc:file-revoked') + + // The binding is gone, so every later frame is inert — nothing is applied and + // nothing reaches the room. + const sentAfterEviction = sent.length + const emitsAfterEviction = socket.emit.mock.calls.length + editFrame() + await vi.advanceTimersByTimeAsync(0) + expect(sent.length).toBe(sentAfterEviction) + expect(socket.emit.mock.calls.length).toBe(emitsAfterEviction) + } finally { + vi.useRealTimers() + } + }) + + it('keeps relaying document frames while the cached permission still allows writing', async () => { + const { io, sent } = createIo() + const { socket, handlers } = setup('socket-1', io) + await handlers[FILE_DOC_EVENTS.JOIN]({ fileId: 'file-1', clientId: 1 }) + await flushMicrotasks() + + const before = sent.length + const edit = new Y.Doc() + edit.getText(FILE_DOC_FIELD).insert(0, 'still allowed') + handlers[FILE_DOC_EVENTS.MESSAGE]( + frame(FILE_DOC_MESSAGE_TYPE.SYNC, (e) => + syncProtocol.writeUpdate(e, Y.encodeStateAsUpdate(edit)) + ) + ) + await flushMicrotasks() + + expect(sent.slice(before).length).toBeGreaterThan(0) + expect(socket.emit).not.toHaveBeenCalledWith('room-access-revoked', expect.anything()) + }) + it('applies + fans out an agent-streamed frame (SYNC_NO_PERSIST) but never persists it', async () => { mockFetchFileDocSeed.mockResolvedValue(seedResult('# From server')) const { io, sent } = createIo() diff --git a/apps/realtime/src/handlers/file-doc.ts b/apps/realtime/src/handlers/file-doc.ts index a3d761bb40f..8580e1fa5b0 100644 --- a/apps/realtime/src/handlers/file-doc.ts +++ b/apps/realtime/src/handlers/file-doc.ts @@ -24,6 +24,11 @@ * @module */ import { createLogger } from '@sim/logger' +import { ROOM_MEMBERSHIP_ACTIONS, satisfiesRoomMembership } from '@sim/platform-authz/room-policy' +import { + ROOM_ACCESS_REVOKED_EVENT, + type RoomAccessRevokedBroadcast, +} from '@sim/realtime-protocol/events' import { FILE_DOC_EVENTS, FILE_DOC_MESSAGE_TYPE, @@ -51,8 +56,10 @@ import { REDIS_ORIGIN, REDIS_SNAPSHOT_ORIGIN, } from '@/handlers/file-doc-store' +import { registerRoomEvictionHandler } from '@/handlers/room-eviction' import { resolveRoomJoinAuth } from '@/handlers/room-join-auth' import type { AuthenticatedSocket } from '@/middleware/auth' +import { peekRoomPermission, resolveCurrentRoomPermission } from '@/middleware/permissions' import type { IRoomManager } from '@/rooms' const logger = createLogger('FileDocHandlers') @@ -837,11 +844,92 @@ function emitJoinError( }) } -function handleMessage(socket: AuthenticatedSocket, data: unknown) { +/** + * The permission occupying a file-doc room requires — `write`, since the room IS + * the collaborative editor. Sourced from the shared map so the join check, the + * per-frame gate below, and the re-validation sweep can never drift apart. + */ +const FILE_DOC_ACTION = ROOM_MEMBERSHIP_ACTIONS[ROOM_TYPES.WORKSPACE_FILE_DOC] + +/** + * Evicts a socket from its file-doc room: emit the revocation, leave the Socket.IO + * room, and drop the pod-local binding + presence. Dropping `socketToRoomName` is + * the load-bearing part — {@link handleMessage} gates every inbound frame on it, so + * once it is gone the socket cannot apply another document update, even if it keeps + * sending them. + */ +function evictFromFileDoc( + socket: AuthenticatedSocket, + io: Server, + name: string, + fileId: string +): void { + const payload: RoomAccessRevokedBroadcast = { + room: fileDocRoom(fileId), + message: 'Your access to this document has been revoked', + timestamp: Date.now(), + } + socket.emit(ROOM_ACCESS_REVOKED_EVENT, payload) + socket.leave(name) + cleanupFileDocForSocket(socket.id, io) +} + +/** + * Per-frame authorization for a socket's inbound document/awareness frames. + * + * Room membership alone is NOT a standing right to write: a collaborator removed + * from the workspace — or downgraded to `read` — must stop landing durable edits on + * an already-open socket, without waiting for the next re-validation sweep. This is + * the synchronous half of that enforcement; the sweep is the asynchronous half. + * + * Reads the shared role cache without awaiting, because this sits on the Yjs relay + * hot path (one call per keystroke-sized frame). Three outcomes: + * - fresh cached permission that satisfies `write` → accept. + * - fresh cached permission that does NOT → drop the frame and evict immediately. + * - nothing fresh cached (TTL lapsed) → accept this frame and kick off a background + * refresh, so the very next frames are gated on an authoritative read. Accepting + * is correct rather than lax: the entry was authoritative when written and every + * join records one, so the exposure is bounded by the same TTL the workflow + * write-path has always had, and the sweep evicts independently. + */ +function isFileDocWriteAllowed(socket: AuthenticatedSocket, io: Server, name: string): boolean { + const userId = socket.userId + const fileId = fileDocRooms.get(name)?.fileId + if (!userId || !fileId) return false + + const room = fileDocRoom(fileId) + const cached = peekRoomPermission(userId, room) + if (cached === undefined) { + // Single-flighted, so a burst of frames triggers at most one query. + void resolveCurrentRoomPermission(userId, room, FILE_DOC_ACTION).catch(() => {}) + return true + } + if (satisfiesRoomMembership(cached, ROOM_TYPES.WORKSPACE_FILE_DOC)) return true + + logger.warn( + `Dropping file-doc frame from user ${userId} whose access to file ${fileId} no longer permits writing` + ) + evictFromFileDoc(socket, io, name, fileId) + return false +} + +/** + * Reconciles this handler's pod-local state when the access re-validation sweep + * evicts a socket from a file-doc room (the sweep has already emitted the + * revocation and left the Socket.IO room). Scoped to the evicted room so a socket + * that has since switched documents keeps the one it legitimately holds. + */ +registerRoomEvictionHandler(ROOM_TYPES.WORKSPACE_FILE_DOC, (socketId, room, io) => { + if (socketToRoomName.get(socketId) !== roomName(room)) return + cleanupFileDocForSocket(socketId, io) +}) + +function handleMessage(socket: AuthenticatedSocket, io: Server, data: unknown) { const name = socketToRoomName.get(socket.id) if (!name) return const room = fileDocRooms.get(name) if (!room) return + if (!isFileDocWriteAllowed(socket, io, name)) return const bytes = toFileDocBytes(data) if (!bytes) return @@ -1009,7 +1097,7 @@ export function setupWorkspaceFileDocHandlers( const authorized = await resolveRoomJoinAuth({ userId, room, - action: 'write', + action: FILE_DOC_ACTION, logger, logLabel: `file-doc room for ${userId}`, messages: { @@ -1031,6 +1119,20 @@ export function setupWorkspaceFileDocHandlers( // here would leak a dead socket's room or bind the socket to the wrong document. if (socket.disconnected || joinGeneration.get(socket.id) !== generation) return + // Re-check the cached decision immediately before registering: the access + // re-validation sweep records a revocation BEFORE it evicts, so a join that + // authorized just before the revocation cannot complete afterwards and re-bind + // the socket to the document. `undefined` (nothing cached) is "unknown", never a + // denial — the authorize above is then the freshest word we have. + const recheck = peekRoomPermission(userId, room) + if ( + recheck !== undefined && + !satisfiesRoomMembership(recheck, ROOM_TYPES.WORKSPACE_FILE_DOC) + ) { + emitJoinError(socket, fileId, 'Access denied to file', 'ACCESS_DENIED', false) + return + } + const entry = getOrCreateRoom(io, room) // A client id must be owned by at most one user, or a peer could bind an active @@ -1148,7 +1250,7 @@ export function setupWorkspaceFileDocHandlers( } }) - socket.on(FILE_DOC_EVENTS.MESSAGE, (data: unknown) => handleMessage(socket, data)) + socket.on(FILE_DOC_EVENTS.MESSAGE, (data: unknown) => handleMessage(socket, io, data)) socket.on(FILE_DOC_EVENTS.LEAVE, (payload?: LeaveFileDocPayload) => { try { diff --git a/apps/realtime/src/handlers/room-eviction.ts b/apps/realtime/src/handlers/room-eviction.ts new file mode 100644 index 00000000000..4e0ece5cb80 --- /dev/null +++ b/apps/realtime/src/handlers/room-eviction.ts @@ -0,0 +1,42 @@ +import { createLogger } from '@sim/logger' +import type { RoomRef, RoomType } from '@sim/realtime-protocol/rooms' +import type { Server } from 'socket.io' + +const logger = createLogger('RoomEviction') + +/** + * Drops whatever pod-local state a handler keeps for a socket in one of its rooms. + * Called after the socket has already been removed from the Socket.IO room, so it + * only has to reconcile the handler's own bookkeeping. + */ +export type RoomEvictionHandler = (socketId: string, room: RoomRef, io: Server) => void + +const handlers = new Map() + +/** + * Registers a room type's local-state cleanup for involuntary eviction (the access + * re-validation sweep, or a per-frame gate that catches a revocation first). + * + * A registry rather than a direct import so the security-critical sweep stays + * domain-neutral: it never has to know that a file-doc room keeps an in-memory + * Y.Doc + ownership map while a table room keeps Redis presence. Handlers register + * at module load, which every handler module does at bootstrap. + */ +export function registerRoomEvictionHandler(type: RoomType, handler: RoomEvictionHandler): void { + handlers.set(type, handler) +} + +/** + * Runs the registered cleanup for an evicted socket, if the room type has one. + * Never throws — an eviction has already taken effect (the socket left the room) + * by the time this runs, so a failing cleanup must not unwind it. + */ +export function runRoomEvictionHandler(socketId: string, room: RoomRef, io: Server): void { + const handler = handlers.get(room.type) + if (!handler) return + try { + handler(socketId, room, io) + } catch (error) { + logger.warn(`Room eviction cleanup failed for socket ${socketId} on ${room.type}`, error) + } +} diff --git a/apps/realtime/src/handlers/room-join-auth.ts b/apps/realtime/src/handlers/room-join-auth.ts index a7c2a9aa24c..7b4d972d523 100644 --- a/apps/realtime/src/handlers/room-join-auth.ts +++ b/apps/realtime/src/handlers/room-join-auth.ts @@ -1,6 +1,7 @@ import type { createLogger } from '@sim/logger' import { authorizeRoom } from '@sim/platform-authz/rooms' import type { RoomRef } from '@sim/realtime-protocol/rooms' +import { recordRoomPermissionIfUnchanged, snapshotRoomPermission } from '@/middleware/permissions' type Authorized = Awaited> @@ -34,6 +35,10 @@ export async function resolveRoomJoinAuth( const { userId, room, action, logger, logLabel, messages, emitError } = params let authorized: Authorized + // Captured before the query so a decision recorded WHILE it was in flight (the + // access-revalidation sweep evicting this user) is never overwritten by this + // older read — see {@link recordRoomPermissionIfUnchanged}. + const snapshot = snapshotRoomPermission(userId, room) try { authorized = await authorizeRoom({ userId, room, action }) } catch (error) { @@ -42,6 +47,16 @@ export async function resolveRoomJoinAuth( return null } + // Feed the fresh authoritative read into the shared role cache that the access + // re-validation sweep and the per-frame write gates consult, so both start warm on + // the room this socket just joined — and so a re-granted user's join immediately + // supersedes a cached revocation instead of waiting out its TTL. A 400 (room type + // not authorizable here) resolved no permission at all and is deliberately not + // recorded. A 404 records `null`: the resource is genuinely gone. + if (authorized.status !== 400) { + recordRoomPermissionIfUnchanged(userId, room, authorized.workspacePermission, snapshot) + } + if (!authorized.allowed) { emitError({ error: authorized.status === 404 ? messages.notFound : messages.accessDenied, diff --git a/apps/realtime/src/handlers/tables.test.ts b/apps/realtime/src/handlers/tables.test.ts index 518c5ee376a..09169396ad2 100644 --- a/apps/realtime/src/handlers/tables.test.ts +++ b/apps/realtime/src/handlers/tables.test.ts @@ -20,6 +20,7 @@ vi.mock('@sim/platform-authz/rooms', () => ({ })) import { setupTablesHandlers } from '@/handlers/tables' +import { recordRoomPermission } from '@/middleware/permissions' const TABLE_ROOM = { type: ROOM_TYPES.TABLE, id: 'table-1' } @@ -174,6 +175,77 @@ describe('setupTablesHandlers', () => { }) }) + it('does not join when access was revoked while the join was in flight', async () => { + // The sweep records a revocation before it evicts, so a join whose authorize + // completed just before that must not put the socket back in the room. + const { socket, handlers } = createSocket({ id: 'socket-race', userId: 'user-race' }) + const roomManager = createRoomManager() + setupTablesHandlers(socket as unknown as SetupArg, roomManager) + + mockAuthorizeRoom.mockImplementation(async () => { + recordRoomPermission('user-race', { type: ROOM_TYPES.TABLE, id: 'table-race' }, null) + return { allowed: true, status: 200, workspaceId: 'ws-1', workspacePermission: 'admin' } + }) + + await handlers[TABLE_PRESENCE_EVENTS.JOIN]({ tableId: 'table-race' }) + + expect(socket.emit).toHaveBeenCalledWith( + TABLE_PRESENCE_EVENTS.JOIN_ERROR, + expect.objectContaining({ code: 'ACCESS_DENIED', retryable: false }) + ) + expect(socket.join).not.toHaveBeenCalled() + expect(roomManager.addUserToRoom).not.toHaveBeenCalled() + }) + + it('drops a cell selection and evicts once the viewer loses access mid-session', async () => { + // Distinct user/table so the recorded revocation cannot leak into sibling tests + // through the module-global role cache. + vi.useFakeTimers() + try { + const room = { type: ROOM_TYPES.TABLE, id: 'table-revoked' } + const { socket, handlers, toEmit } = createSocket({ id: 'socket-9', userId: 'user-9' }) + const roomManager = createRoomManager({ + getRoomForSocket: vi.fn().mockResolvedValue(room), + }) + setupTablesHandlers(socket as unknown as SetupArg, roomManager) + + await handlers[TABLE_PRESENCE_EVENTS.JOIN]({ tableId: 'table-revoked' }) + await vi.advanceTimersByTimeAsync(0) + + // Access removed, and the join-time decision expires. + mockAuthorizeRoom.mockResolvedValue({ + allowed: false, + status: 403, + workspaceId: 'ws-1', + workspacePermission: null, + }) + await vi.advanceTimersByTimeAsync(31_000) + + const cell = { + anchor: { rowId: 'row-1', columnId: 'col-a' }, + focus: { rowId: 'row-1', columnId: 'col-a' }, + } + // First selection after expiry finds nothing cached: accepted, and it kicks off the + // authoritative re-read rather than blocking the relay on a DB round-trip. + await handlers[TABLE_PRESENCE_EVENTS.CELL_SELECTION]({ cell }) + await vi.advanceTimersByTimeAsync(0) + + toEmit.mockClear() + await handlers[TABLE_PRESENCE_EVENTS.CELL_SELECTION]({ cell }) + await vi.advanceTimersByTimeAsync(0) + + expect(toEmit).not.toHaveBeenCalled() + expect(socket.emit).toHaveBeenCalledWith( + 'room-access-revoked', + expect.objectContaining({ room }) + ) + expect(socket.leave).toHaveBeenCalledWith('table:table-revoked') + expect(roomManager.removeUserFromRoom).toHaveBeenCalledWith(room, 'socket-9') + } finally { + vi.useRealTimers() + } + }) + it('drops a malformed cell selection without storing or relaying it', async () => { const { socket, handlers, toEmit } = createSocket() const roomManager = createRoomManager({ diff --git a/apps/realtime/src/handlers/tables.ts b/apps/realtime/src/handlers/tables.ts index 2474063c9a0..04aed5af1a1 100644 --- a/apps/realtime/src/handlers/tables.ts +++ b/apps/realtime/src/handlers/tables.ts @@ -1,4 +1,9 @@ import { createLogger } from '@sim/logger' +import { ROOM_MEMBERSHIP_ACTIONS, satisfiesRoomMembership } from '@sim/platform-authz/room-policy' +import { + ROOM_ACCESS_REVOKED_EVENT, + type RoomAccessRevokedBroadcast, +} from '@sim/realtime-protocol/events' import { ROOM_TYPES, type RoomRef, roomName } from '@sim/realtime-protocol/rooms' import { type JoinTablePayload, @@ -9,6 +14,7 @@ import { import { resolveAvatarUrl } from '@/handlers/avatar' import { resolveRoomJoinAuth } from '@/handlers/room-join-auth' import type { AuthenticatedSocket } from '@/middleware/auth' +import { peekRoomPermission, resolveCurrentRoomPermission } from '@/middleware/permissions' import type { IRoomManager, UserPresence } from '@/rooms' import { filterVisiblePresence, sweepStalePresence } from '@/rooms/presence-visibility' @@ -20,6 +26,41 @@ const MAX_CELL_ID_LENGTH = 200 /** The table presence room ref for a table id. */ const tableRoom = (tableId: string): RoomRef => ({ type: ROOM_TYPES.TABLE, id: tableId }) +/** + * The permission occupying a table room requires. Sourced from the shared map so + * the join check, the per-operation gate below, and the access re-validation sweep + * can never drift apart. + */ +const TABLE_ACTION = ROOM_MEMBERSHIP_ACTIONS[ROOM_TYPES.TABLE] + +/** + * Per-operation authorization for an already-joined socket, mirroring the file-doc + * gate: room membership is not a standing right, so a collaborator whose workspace + * access was revoked stops publishing presence into the room without waiting for + * the next re-validation sweep. + * + * Reads the shared role cache without awaiting (`undefined` = nothing fresh cached, + * which means "unknown", not "denied") and kicks off a background refresh in that + * case, so the exposure window is the cache TTL rather than the socket's lifetime. + * On a confirmed loss of access it evicts, which also clears the room mapping this + * handler resolves every operation through. + */ +function isTableAccessAllowed( + socket: AuthenticatedSocket, + room: RoomRef +): { allowed: boolean; revoked: boolean } { + const userId = socket.userId + if (!userId) return { allowed: false, revoked: false } + + const cached = peekRoomPermission(userId, room) + if (cached === undefined) { + void resolveCurrentRoomPermission(userId, room, TABLE_ACTION).catch(() => {}) + return { allowed: true, revoked: false } + } + if (satisfiesRoomMembership(cached, ROOM_TYPES.TABLE)) return { allowed: true, revoked: false } + return { allowed: false, revoked: true } +} + function isCellRef(value: unknown): value is TableCellRef { if (typeof value !== 'object' || value === null) return false const ref = value as { rowId?: unknown; columnId?: unknown } @@ -50,6 +91,33 @@ function normalizeCellSelection(cell: unknown): TableCellSelection | undefined { } } +/** + * Evicts a socket from a table room after a confirmed loss of access: emit the + * revocation, leave the Socket.IO room, and drop its presence so peers stop seeing + * its selection. Best-effort on the presence half — the socket has already left the + * room, and the sweep's cleanup lane retries any removal that fails here. + */ +async function evictFromTable( + socket: AuthenticatedSocket, + roomManager: IRoomManager, + room: RoomRef +): Promise { + const payload: RoomAccessRevokedBroadcast = { + room, + message: 'Your access to this table has been revoked', + timestamp: Date.now(), + } + socket.emit(ROOM_ACCESS_REVOKED_EVENT, payload) + socket.leave(roomName(room)) + logger.warn(`Evicted user ${socket.userId} from table room ${room.id}: access revoked`) + try { + await roomManager.removeUserFromRoom(room, socket.id) + await roomManager.broadcastPresenceUpdate(room, socket.id) + } catch (error) { + logger.warn(`Presence cleanup failed for evicted table socket ${socket.id}`, error) + } +} + /** * Live cell-selection presence for the table grid. Mirrors the workspace-files * join flow but is table-scoped (room id = tableId) with a bidirectional @@ -135,7 +203,7 @@ export function setupTablesHandlers(socket: AuthenticatedSocket, roomManager: IR const authorized = await resolveRoomJoinAuth({ userId, room, - action: 'read', + action: TABLE_ACTION, logger, logLabel: `table room for ${userId}`, messages: { @@ -186,6 +254,21 @@ export function setupTablesHandlers(socket: AuthenticatedSocket, roomManager: IR // awaits above bumped the generation, or the socket disconnected. Abort before registering. if (superseded()) return + // Re-check the cached decision too: the access re-validation sweep records a + // revocation BEFORE it evicts, so a join that authorized just before the + // revocation cannot complete afterwards and put the socket back in the room. + // `undefined` (nothing cached) is "unknown", never a denial. + const recheck = peekRoomPermission(userId, room) + if (recheck !== undefined && !satisfiesRoomMembership(recheck, ROOM_TYPES.TABLE)) { + socket.emit(TABLE_PRESENCE_EVENTS.JOIN_ERROR, { + tableId, + error: 'Access denied to table', + code: 'ACCESS_DENIED', + retryable: false, + }) + return + } + socket.join(roomName(room)) const presence: UserPresence = { @@ -298,6 +381,14 @@ export function setupTablesHandlers(socket: AuthenticatedSocket, roomManager: IR const room = await roomManager.getRoomForSocket(socket.id, ROOM_TYPES.TABLE) if (!room) return + // Membership was authorized at join; re-check it here so a revoked viewer + // stops publishing presence into the room mid-session. + const access = isTableAccessAllowed(socket, room) + if (!access.allowed) { + if (access.revoked) await evictFromTable(socket, roomManager, room) + return + } + // Persist so a later joiner sees this viewer's current selection in the join ack. await roomManager.updateUserActivity(room, socket.id, { cell: selection }) diff --git a/apps/realtime/src/handlers/workspace-invalidation-room.ts b/apps/realtime/src/handlers/workspace-invalidation-room.ts index aacb141e9ae..9560740c35d 100644 --- a/apps/realtime/src/handlers/workspace-invalidation-room.ts +++ b/apps/realtime/src/handlers/workspace-invalidation-room.ts @@ -1,4 +1,5 @@ import { createLogger } from '@sim/logger' +import { ROOM_MEMBERSHIP_ACTIONS } from '@sim/platform-authz/room-policy' import { type RoomRef, type RoomType, roomName } from '@sim/realtime-protocol/rooms' import { resolveRoomJoinAuth } from '@/handlers/room-join-auth' import type { AuthenticatedSocket } from '@/middleware/auth' @@ -89,7 +90,7 @@ export function setupWorkspaceInvalidationRoom( const authorized = await resolveRoomJoinAuth({ userId: socket.userId, room: ref, - action: 'read', + action: ROOM_MEMBERSHIP_ACTIONS[roomType], logger, logLabel: `${roomType} room for ${socket.userId}`, messages: { diff --git a/apps/realtime/src/middleware/permissions.ts b/apps/realtime/src/middleware/permissions.ts index 779814257bf..545214b0e28 100644 --- a/apps/realtime/src/middleware/permissions.ts +++ b/apps/realtime/src/middleware/permissions.ts @@ -1,6 +1,7 @@ import { db } from '@sim/db' import { workflow } from '@sim/db/schema' import { createLogger } from '@sim/logger' +import { authorizeRoom, type PermissionType } from '@sim/platform-authz/rooms' import { authorizeWorkflowByWorkspacePermission } from '@sim/platform-authz/workflow' import { BLOCK_OPERATIONS, @@ -12,6 +13,7 @@ import { VARIABLE_OPERATIONS, WORKFLOW_OPERATIONS, } from '@sim/realtime-protocol/constants' +import { ROOM_TYPES, type RoomRef, roomName } from '@sim/realtime-protocol/rooms' import { and, eq, isNull } from 'drizzle-orm' const logger = createLogger('SocketPermissions') @@ -85,11 +87,12 @@ export function checkRolePermission( } /** - * TTL for the per-pod role cache backing live re-validation. It gates both the - * mutating-operation checks ({@link checkWorkflowOperationPermission}) and the - * periodic read-access sweep (`access-revalidation.ts`), so a revoked or - * downgraded collaborator loses write access — and live reads — on an - * already-connected socket within a bounded window rather than until disconnect. + * TTL for the per-pod role cache backing live re-validation. It gates the + * mutating-operation checks ({@link checkWorkflowOperationPermission}), the + * per-frame room write gates (file-doc / tables), and the periodic access sweep + * (`access-revalidation.ts`), so a revoked or downgraded collaborator loses write + * access — and live reads — on an already-connected socket within a bounded + * window rather than until disconnect. */ export const ROLE_REVALIDATION_TTL_MS = 30_000 @@ -103,7 +106,13 @@ interface CachedRole { } /** - * Per-pod cache of authoritative workspace roles, keyed by `${userId}:${workflowId}`. + * Per-pod cache of authoritative workspace roles, keyed by + * `${userId}:${roomName(room)}`. + * + * The key uses the wire room name, so a workflow entry is `${userId}:${workflowId}` + * exactly as before (workflow rooms are unprefixed) while every other room type is + * namespaced by its own `type:id` — a file-doc and a table can never collide on an + * id, and one cache serves every room type. * * Socket connections are sticky to a single pod, so a socket's mutating operations are * always gated by the same pod's cache. We rely on TTL expiry (not cross-pod @@ -114,13 +123,17 @@ const roleCache = new Map() /** * In-flight resolutions keyed like {@link roleCache}. Concurrent callers for the same - * (user, workflow) share one authorization query instead of racing independent ones, so + * (user, room) share one authorization query instead of racing independent ones, so * cache writes per key are serialized — a slow, stale pre-revocation read can never * overwrite a newer recorded decision (e.g. the revocation the eviction sweep just * cached before kicking the socket). */ const inFlightRoleResolutions = new Map>() +function roleCacheKey(userId: string, room: RoomRef): string { + return `${userId}:${roomName(room)}` +} + function purgeExpiredRoles(now: number): void { for (const [key, entry] of roleCache) { if (entry.expiresAt <= now) { @@ -144,20 +157,47 @@ function recordRoleDecision(key: string, role: string | null): void { roleCache.set(key, { role, expiresAt: now + ROLE_REVALIDATION_TTL_MS }) } +/** + * Reads a user's authoritative effective workspace permission for a room, or + * `null` when they have none. Workflow rooms go through their own dedicated + * authorizer; every other room type resolves through the shared + * {@link authorizeRoom} (resource → owning workspace → effective permission). + * + * Always asks for the LOWEST level (`read`) so the resolved permission itself is + * returned rather than a boolean — callers compare it against whatever level the + * operation needs with `permissionSatisfies`. A room type that is not + * authorizable here (status 400) THROWS, so it lands in the caller's transient + * branch and can never be mistaken for a confirmed revocation. + */ +async function readAuthoritativeRoomPermission( + userId: string, + room: RoomRef +): Promise { + if (room.type === ROOM_TYPES.WORKFLOW) { + const authorization = await authorizeWorkflowByWorkspacePermission({ + workflowId: room.id, + userId, + action: 'read', + }) + return authorization.allowed ? (authorization.workspacePermission ?? null) : null + } + + const authorization = await authorizeRoom({ userId, room, action: 'read' }) + if (!authorization.allowed && authorization.status === 400) { + throw new Error(`Room type not authorizable: ${room.type}`) + } + return authorization.allowed ? (authorization.workspacePermission ?? null) : null +} + async function resolveRoleUncached( key: string, userId: string, - workflowId: string, + room: RoomRef, fallbackRole: string ): Promise { const entryBeforeQuery = roleCache.get(key) try { - const authorization = await authorizeWorkflowByWorkspacePermission({ - workflowId, - userId, - action: 'read', - }) - const role = authorization.allowed ? (authorization.workspacePermission ?? null) : null + const role = await readAuthoritativeRoomPermission(userId, room) // A fresh authoritative read (e.g. a join-time verifyWorkflowAccess) may // have recorded a decision while this query was in flight. That write is // newer than this query's read snapshot, so prefer it instead of @@ -170,7 +210,7 @@ async function resolveRoleUncached( return role } catch (error) { logger.warn( - `Failed to re-validate role for user ${userId} on workflow ${workflowId}; using last known role`, + `Failed to re-validate role for user ${userId} on ${roomName(room)}; using last known role`, error ) // Prefer the last recorded decision — even if expired, and even if it is `null` for an @@ -183,23 +223,23 @@ async function resolveRoleUncached( } /** - * Resolves a user's current workspace role for a workflow, re-reading the `permissions` - * table at most once per {@link ROLE_REVALIDATION_TTL_MS} per pod. Concurrent calls for - * the same (user, workflow) coalesce onto a single in-flight query (single-flight), so - * out-of-order cache writes cannot resurrect revoked access. + * Resolves a user's current effective workspace permission for a room, re-reading the + * `permissions` table at most once per {@link ROLE_REVALIDATION_TTL_MS} per pod. + * Concurrent calls for the same (user, room) coalesce onto a single in-flight query + * (single-flight), so out-of-order cache writes cannot resurrect revoked access. * - * Returns `null` when the user genuinely has no access (removed/revoked). On a transient - * DB failure it reuses the last recorded decision for this (user, workflow) — including a - * previously recorded revocation (`null`) — and only falls back to `fallbackRole` when no - * decision has been recorded yet, so a blip neither blocks legitimate editors nor - * resurrects already-revoked access. + * Returns `null` when the user genuinely has no access (removed/revoked, or the room's + * resource is gone). On a transient DB failure it reuses the last recorded decision for + * this (user, room) — including a previously recorded revocation (`null`) — and only + * falls back to `fallbackRole` when no decision has been recorded yet, so a blip neither + * blocks legitimate editors nor resurrects already-revoked access. */ -export async function resolveCurrentWorkflowRole( +export async function resolveCurrentRoomPermission( userId: string, - workflowId: string, + room: RoomRef, fallbackRole: string ): Promise { - const key = `${userId}:${workflowId}` + const key = roleCacheKey(userId, room) const cached = roleCache.get(key) if (cached && cached.expiresAt > Date.now()) { return cached.role @@ -210,13 +250,93 @@ export async function resolveCurrentWorkflowRole( return inFlight } - const resolution = resolveRoleUncached(key, userId, workflowId, fallbackRole).finally(() => { + const resolution = resolveRoleUncached(key, userId, room, fallbackRole).finally(() => { inFlightRoleResolutions.delete(key) }) inFlightRoleResolutions.set(key, resolution) return resolution } +/** + * Workflow-room specialization of {@link resolveCurrentRoomPermission}, kept as the + * name the workflow operation/join paths already use. + */ +export function resolveCurrentWorkflowRole( + userId: string, + workflowId: string, + fallbackRole: string +): Promise { + return resolveCurrentRoomPermission( + userId, + { type: ROOM_TYPES.WORKFLOW, id: workflowId }, + fallbackRole + ) +} + +/** + * Non-blocking read of the cached permission for a (user, room). + * + * Returns `undefined` when nothing fresh is cached — the caller must NOT read that + * as a denial; it means "unknown, ask asynchronously". Exists for the per-frame + * gates on hot relay paths (Yjs document frames, table cell selections), which must + * stay synchronous: awaiting an authorization promise per keystroke would put a + * microtask between every frame and its apply. Freshness is bounded by the same + * {@link ROLE_REVALIDATION_TTL_MS}, and the periodic sweep independently evicts a + * revoked socket, so a gate built on this is never the only line of defense. + */ +export function peekRoomPermission(userId: string, room: RoomRef): string | null | undefined { + const cached = roleCache.get(roleCacheKey(userId, room)) + if (!cached || cached.expiresAt <= Date.now()) return undefined + return cached.role +} + +/** + * Records an authoritative decision read outside this module (the room join + * authorizer) into the shared cache, so the sweep and the per-frame gates start + * warm and a re-granted user is never held out by a stale cached revocation. + * + * Unconditional: the caller's read is taken as the newest word. Use + * {@link recordRoomPermissionIfUnchanged} when the read was slow enough that a + * newer decision could have landed meanwhile. + */ +export function recordRoomPermission( + userId: string, + room: RoomRef, + permission: PermissionType | null +): void { + recordRoleDecision(roleCacheKey(userId, room), permission) +} + +/** Opaque token identifying the cache entry a caller observed before its query. */ +export type RoomPermissionSnapshot = object | undefined + +/** Captures the current cache entry for a (user, room), for {@link recordRoomPermissionIfUnchanged}. */ +export function snapshotRoomPermission(userId: string, room: RoomRef): RoomPermissionSnapshot { + return roleCache.get(roleCacheKey(userId, room)) +} + +/** + * Records an authoritative decision only if no other decision landed since + * `snapshot` was taken. + * + * Without this, a join whose authorization query started before a revocation but + * returned after the sweep recorded that revocation would overwrite it with its own + * stale "allowed", handing the socket another full TTL of access. Entry identity + * (not a timestamp) is the comparison, mirroring the in-flight ordering guard the + * cached resolver already applies to its own queries — so it is exact regardless of + * how many writes land inside one millisecond. + */ +export function recordRoomPermissionIfUnchanged( + userId: string, + room: RoomRef, + permission: PermissionType | null, + snapshot: RoomPermissionSnapshot +): void { + const key = roleCacheKey(userId, room) + if (roleCache.get(key) !== snapshot) return + recordRoleDecision(key, permission) +} + /** * Live permission gate for mutating socket operations. Re-validates the user's workspace * role against the database (cached per pod for {@link ROLE_REVALIDATION_TTL_MS}) so that @@ -279,7 +399,7 @@ export async function verifyWorkflowAccess( }) recordRoleDecision( - `${userId}:${workflowId}`, + roleCacheKey(userId, { type: ROOM_TYPES.WORKFLOW, id: workflowId }), authorization.allowed ? (authorization.workspacePermission ?? null) : null ) diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/file-doc-provider.ts b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/file-doc-provider.ts index 9b55ac7b907..84bab0733bc 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/file-doc-provider.ts +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/file-doc-provider.ts @@ -1,3 +1,7 @@ +import { + ROOM_ACCESS_REVOKED_EVENT, + type RoomAccessRevokedBroadcast, +} from '@sim/realtime-protocol/events' import { FILE_DOC_EVENTS, FILE_DOC_MESSAGE_TYPE, @@ -7,6 +11,7 @@ import { type JoinFileDocSuccess, toFileDocBytes, } from '@sim/realtime-protocol/file-doc' +import { ROOM_TYPES } from '@sim/realtime-protocol/rooms' import * as decoding from 'lib0/decoding' import * as encoding from 'lib0/encoding' import { ObservableV2 } from 'lib0/observable' @@ -131,6 +136,7 @@ export class FileDocProvider extends ObservableV2 { socket.on(FILE_DOC_EVENTS.MESSAGE, this.handleMessage) socket.on(FILE_DOC_EVENTS.JOIN_SUCCESS, this.handleJoinSuccess) socket.on(FILE_DOC_EVENTS.JOIN_ERROR, this.handleJoinError) + socket.on(ROOM_ACCESS_REVOKED_EVENT, this.handleAccessRevoked) socket.on('connect', this.handleConnect) doc.on('update', this.handleDocUpdate) awareness.on('update', this.handleAwarenessUpdate) @@ -231,6 +237,30 @@ export class FileDocProvider extends ObservableV2 { this.emit('join-error', [data]) } + /** + * The server evicted this socket from the document because the user's workspace + * access was revoked or downgraded below `write` mid-session. Nothing sent from + * here would be applied any more, so take the same path as a non-retryable + * rejection: latch fatal (stop re-joining, stop applying inbound frames) and drop + * `synced`, so the editor falls back to the read-only view of the stored content + * instead of silently accepting keystrokes that go nowhere. + */ + private handleAccessRevoked = (data: RoomAccessRevokedBroadcast) => { + if (data.room?.type !== ROOM_TYPES.WORKSPACE_FILE_DOC || data.room.id !== this.fileId) return + if (this.fatal || this.disposed) return + const error: JoinFileDocError = { + fileId: this.fileId, + error: data.message, + code: 'ACCESS_REVOKED', + retryable: false, + } + this.fatal = true + this.joinError = error + this.clearReadinessTimer() + this.setSynced(false) + this.emit('join-error', [error]) + } + private handleMessage = (data: unknown) => { // Once we've given up (a non-retryable rejection, or the connect deadline lapsed and the editor // fell back to a read-only local seed), ignore ALL inbound frames. A late SyncStep2 arriving @@ -362,6 +392,7 @@ export class FileDocProvider extends ObservableV2 { this.socket.off(FILE_DOC_EVENTS.MESSAGE, this.handleMessage) this.socket.off(FILE_DOC_EVENTS.JOIN_SUCCESS, this.handleJoinSuccess) this.socket.off(FILE_DOC_EVENTS.JOIN_ERROR, this.handleJoinError) + this.socket.off(ROOM_ACCESS_REVOKED_EVENT, this.handleAccessRevoked) this.socket.off('connect', this.handleConnect) this.doc.off('update', this.handleDocUpdate) this.doc.getMap(FILE_DOC_SEED.configMap).unobserve(this.handleConfigChange) diff --git a/packages/platform-authz/package.json b/packages/platform-authz/package.json index d8d0bca5b03..f8420a5e056 100644 --- a/packages/platform-authz/package.json +++ b/packages/platform-authz/package.json @@ -25,6 +25,10 @@ "./rooms": { "types": "./src/rooms.ts", "default": "./src/rooms.ts" + }, + "./room-policy": { + "types": "./src/room-policy.ts", + "default": "./src/room-policy.ts" } }, "scripts": { diff --git a/packages/platform-authz/src/predicates.ts b/packages/platform-authz/src/predicates.ts index ff1afa6e11d..22721192cbe 100644 --- a/packages/platform-authz/src/predicates.ts +++ b/packages/platform-authz/src/predicates.ts @@ -9,6 +9,18 @@ export const PERMISSION_RANK = { read: 1, write: 2, admin: 3 } as const satisfie number > +/** + * Type guard for a workspace permission level. Checks against + * {@link PERMISSION_RANK} rather than the DB enum so this module stays + * dependency-free (see {@link isOrgAdminRole}). Use it to narrow role strings that + * are typed `string` for legacy reasons before comparing them with + * {@link permissionSatisfies}, so an unrecognized value is handled explicitly + * instead of silently ranking below every level. + */ +export function isPermissionType(value: unknown): value is PermissionType { + return typeof value === 'string' && Object.hasOwn(PERMISSION_RANK, value) +} + /** * Whether an effective permission satisfies a required level under the * read < write < admin ordering. `null`/`undefined` (no access) never satisfies. diff --git a/packages/platform-authz/src/room-policy.ts b/packages/platform-authz/src/room-policy.ts new file mode 100644 index 00000000000..0d22ded1a3c --- /dev/null +++ b/packages/platform-authz/src/room-policy.ts @@ -0,0 +1,44 @@ +import { ROOM_TYPES, type RoomType } from '@sim/realtime-protocol/rooms' +import { isPermissionType, type PermissionType, permissionSatisfies } from './predicates' + +/** + * Realtime room membership policy: which workspace permission a user must hold to + * OCCUPY each room type, and the predicate that answers it. + * + * Deliberately its own dependency-free module (no DB client, no authorizer) so + * every enforcement point can import it: the join-time check, the per-frame gates + * on the hot relay paths, and the periodic access-revalidation sweep. One source of + * truth means the join gate and the sweep can never drift — and that drift is + * exactly what would let a downgraded member keep a room they could no longer join. + */ + +/** + * A file-doc room IS the collaborative editor, so occupying it requires `write`; + * every other room carries only reads/presence and requires `read`. Workflow is + * included for the sweep's benefit even though it authorizes through its own path + * (`authorizeWorkflowByWorkspacePermission`). + */ +export const ROOM_MEMBERSHIP_ACTIONS = { + [ROOM_TYPES.WORKFLOW]: 'read', + [ROOM_TYPES.WORKSPACE_FILES]: 'read', + [ROOM_TYPES.WORKSPACE_TABLES]: 'read', + [ROOM_TYPES.WORKSPACE_FILE_DOC]: 'write', + [ROOM_TYPES.TABLE]: 'read', +} as const satisfies Record + +/** + * Whether a resolved permission still entitles its holder to occupy a room of this + * type. `null` (no access) never does. + * + * The parameter is `string` rather than {@link PermissionType} because the realtime + * server carries roles as plain strings on presence records. A value outside the + * known levels is treated as satisfying: roles originate from the DB enum, so an + * unrecognized one means something upstream changed — and reading it as + * "insufficient" would mass-evict live collaborators. Enforcement stays reserved + * for a definitively known-insufficient permission. + */ +export function satisfiesRoomMembership(role: string | null | undefined, type: RoomType): boolean { + if (role == null) return false + if (!isPermissionType(role)) return true + return permissionSatisfies(role, ROOM_MEMBERSHIP_ACTIONS[type]) +} diff --git a/packages/realtime-protocol/src/events.ts b/packages/realtime-protocol/src/events.ts index 89b6fadb586..8fb0b4333a0 100644 --- a/packages/realtime-protocol/src/events.ts +++ b/packages/realtime-protocol/src/events.ts @@ -1,4 +1,5 @@ import type { OperationTarget, SocketOperation } from './constants' +import type { RoomRef } from './rooms' /** * Wire types for the broadcast/confirmation events the realtime Socket.IO server @@ -121,6 +122,27 @@ export interface AccessRevokedBroadcast { timestamp: number } +/** + * `room-access-revoked` broadcast — the non-workflow counterpart of + * {@link AccessRevokedBroadcast}. Emitted to a single socket when its owner's + * workspace permission no longer satisfies the room it occupies (removed, or + * downgraded below the room's required level), after the server has already + * evicted it. Carries the generic {@link RoomRef} rather than a workflow id + * because one socket can hold several rooms of different types at once, so the + * client must be told exactly which one it lost. + * + * Kept as a separate event from `access-revoked` so existing workflow clients — + * which read `data.workflowId` — are never handed a payload they would misparse. + */ +export interface RoomAccessRevokedBroadcast { + room: RoomRef + message: string + timestamp: number +} + +/** Wire event name carrying a {@link RoomAccessRevokedBroadcast}. */ +export const ROOM_ACCESS_REVOKED_EVENT = 'room-access-revoked' + /** `operation-confirmed` ack for a previously-emitted operation. */ export interface OperationConfirmedBroadcast { operationId: string From 553d1aa4c3dd60f665530c9ca029bcd38f3f3bd1 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 1 Aug 2026 16:31:46 -0700 Subject: [PATCH 02/11] refactor(realtime): one shared eviction path for revoked room access The sweep and both per-frame gates each open-coded emit + leave + local-state cleanup. Route them all through evictSocketFromRoom so they cannot diverge on what eviction means; workflow keeps its historical access-revoked payload. --- apps/realtime/src/access-revalidation.ts | 24 +++++--------- apps/realtime/src/handlers/file-doc.ts | 35 ++++---------------- apps/realtime/src/handlers/room-eviction.ts | 36 ++++++++++++++++++++- apps/realtime/src/handlers/tables.ts | 22 ++++--------- 4 files changed, 55 insertions(+), 62 deletions(-) diff --git a/apps/realtime/src/access-revalidation.ts b/apps/realtime/src/access-revalidation.ts index 5bbc4381d66..4afe096bd19 100644 --- a/apps/realtime/src/access-revalidation.ts +++ b/apps/realtime/src/access-revalidation.ts @@ -1,10 +1,6 @@ import { createLogger } from '@sim/logger' import { ROOM_MEMBERSHIP_ACTIONS, satisfiesRoomMembership } from '@sim/platform-authz/room-policy' -import { - type AccessRevokedBroadcast, - ROOM_ACCESS_REVOKED_EVENT, - type RoomAccessRevokedBroadcast, -} from '@sim/realtime-protocol/events' +import type { AccessRevokedBroadcast } from '@sim/realtime-protocol/events' import { parseRoomName, ROOM_TYPES, @@ -13,7 +9,7 @@ import { roomName, } from '@sim/realtime-protocol/rooms' import { sleep } from '@sim/utils/helpers' -import { runRoomEvictionHandler } from '@/handlers/room-eviction' +import { evictSocketFromRoom, runRoomEvictionHandler } from '@/handlers/room-eviction' import type { AuthenticatedSocket } from '@/middleware/auth' import { ROLE_REVALIDATION_TTL_MS, resolveCurrentRoomPermission } from '@/middleware/permissions' import type { IRoomManager } from '@/rooms' @@ -266,24 +262,20 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR // gates its document writes). Redis presence cleanup is only ENQUEUED here — // the cleanup lane performs that work, so eviction never blocks on it. if (room.type === ROOM_TYPES.WORKFLOW) { + // Workflow keeps its historical wire event and payload shape, which existing + // clients key off `workflowId`; every other type shares the generic path. const payload: AccessRevokedBroadcast = { workflowId: room.id, message: 'Your access to this workflow has been revoked', timestamp: Date.now(), } socket.emit('access-revoked', payload) + socket.leave(name) + runRoomEvictionHandler(socket.id, room, io) + logger.info(`Revoked live access for user ${socket.userId} on ${name} (socket ${socket.id})`) } else { - const payload: RoomAccessRevokedBroadcast = { - room, - message: 'Your access to this resource has been revoked', - timestamp: Date.now(), - } - socket.emit(ROOM_ACCESS_REVOKED_EVENT, payload) + evictSocketFromRoom(socket, room, 'Your access to this resource has been revoked', io) } - socket.leave(name) - runRoomEvictionHandler(socket.id, room, io) - - logger.info(`Revoked live access for user ${socket.userId} on ${name} (socket ${socket.id})`) if (PRESENCE_ROOM_TYPES.has(room.type)) { pendingCleanups.set(`${socket.id}:${name}`, { socketId: socket.id, room }) diff --git a/apps/realtime/src/handlers/file-doc.ts b/apps/realtime/src/handlers/file-doc.ts index 8580e1fa5b0..15ac171c68d 100644 --- a/apps/realtime/src/handlers/file-doc.ts +++ b/apps/realtime/src/handlers/file-doc.ts @@ -25,10 +25,6 @@ */ import { createLogger } from '@sim/logger' import { ROOM_MEMBERSHIP_ACTIONS, satisfiesRoomMembership } from '@sim/platform-authz/room-policy' -import { - ROOM_ACCESS_REVOKED_EVENT, - type RoomAccessRevokedBroadcast, -} from '@sim/realtime-protocol/events' import { FILE_DOC_EVENTS, FILE_DOC_MESSAGE_TYPE, @@ -56,7 +52,7 @@ import { REDIS_ORIGIN, REDIS_SNAPSHOT_ORIGIN, } from '@/handlers/file-doc-store' -import { registerRoomEvictionHandler } from '@/handlers/room-eviction' +import { evictSocketFromRoom, registerRoomEvictionHandler } from '@/handlers/room-eviction' import { resolveRoomJoinAuth } from '@/handlers/room-join-auth' import type { AuthenticatedSocket } from '@/middleware/auth' import { peekRoomPermission, resolveCurrentRoomPermission } from '@/middleware/permissions' @@ -851,29 +847,6 @@ function emitJoinError( */ const FILE_DOC_ACTION = ROOM_MEMBERSHIP_ACTIONS[ROOM_TYPES.WORKSPACE_FILE_DOC] -/** - * Evicts a socket from its file-doc room: emit the revocation, leave the Socket.IO - * room, and drop the pod-local binding + presence. Dropping `socketToRoomName` is - * the load-bearing part — {@link handleMessage} gates every inbound frame on it, so - * once it is gone the socket cannot apply another document update, even if it keeps - * sending them. - */ -function evictFromFileDoc( - socket: AuthenticatedSocket, - io: Server, - name: string, - fileId: string -): void { - const payload: RoomAccessRevokedBroadcast = { - room: fileDocRoom(fileId), - message: 'Your access to this document has been revoked', - timestamp: Date.now(), - } - socket.emit(ROOM_ACCESS_REVOKED_EVENT, payload) - socket.leave(name) - cleanupFileDocForSocket(socket.id, io) -} - /** * Per-frame authorization for a socket's inbound document/awareness frames. * @@ -909,7 +882,11 @@ function isFileDocWriteAllowed(socket: AuthenticatedSocket, io: Server, name: st logger.warn( `Dropping file-doc frame from user ${userId} whose access to file ${fileId} no longer permits writing` ) - evictFromFileDoc(socket, io, name, fileId) + // Evicting (not just dropping the frame) is what makes this stick: the registered + // eviction handler clears `socketToRoomName`, and every inbound frame is gated on + // that binding — so the socket cannot apply another document update even if it + // keeps sending them. + evictSocketFromRoom(socket, room, 'Your access to this document has been revoked', io) return false } diff --git a/apps/realtime/src/handlers/room-eviction.ts b/apps/realtime/src/handlers/room-eviction.ts index 4e0ece5cb80..bd6ebace6e6 100644 --- a/apps/realtime/src/handlers/room-eviction.ts +++ b/apps/realtime/src/handlers/room-eviction.ts @@ -1,6 +1,11 @@ import { createLogger } from '@sim/logger' -import type { RoomRef, RoomType } from '@sim/realtime-protocol/rooms' +import { + ROOM_ACCESS_REVOKED_EVENT, + type RoomAccessRevokedBroadcast, +} from '@sim/realtime-protocol/events' +import { type RoomRef, type RoomType, roomName } from '@sim/realtime-protocol/rooms' import type { Server } from 'socket.io' +import type { AuthenticatedSocket } from '@/middleware/auth' const logger = createLogger('RoomEviction') @@ -40,3 +45,32 @@ export function runRoomEvictionHandler(socketId: string, room: RoomRef, io: Serv logger.warn(`Room eviction cleanup failed for socket ${socketId} on ${room.type}`, error) } } + +/** + * Evicts one socket from one non-workflow room after a confirmed loss of access: + * tell the client, stop it receiving room broadcasts, and drop the handler-local + * state that would otherwise still accept its frames. + * + * The single eviction path shared by both enforcement points — the periodic + * re-validation sweep and the per-frame gates that catch a revocation first — so + * they cannot diverge on what "evicted" means. Everything here is synchronous and + * pod-local; any Redis presence removal is the caller's own follow-up (the sweep + * owns a retrying cleanup lane for it). + * + * Workflow rooms keep their own `access-revoked` wire event for client + * compatibility and are deliberately not routed through here. + */ +export function evictSocketFromRoom( + socket: AuthenticatedSocket, + room: RoomRef, + message: string, + io: Server +): void { + const payload: RoomAccessRevokedBroadcast = { room, message, timestamp: Date.now() } + socket.emit(ROOM_ACCESS_REVOKED_EVENT, payload) + socket.leave(roomName(room)) + runRoomEvictionHandler(socket.id, room, io) + logger.info( + `Revoked live access for user ${socket.userId} on ${roomName(room)} (socket ${socket.id})` + ) +} diff --git a/apps/realtime/src/handlers/tables.ts b/apps/realtime/src/handlers/tables.ts index 04aed5af1a1..b7bb686664e 100644 --- a/apps/realtime/src/handlers/tables.ts +++ b/apps/realtime/src/handlers/tables.ts @@ -1,9 +1,5 @@ import { createLogger } from '@sim/logger' import { ROOM_MEMBERSHIP_ACTIONS, satisfiesRoomMembership } from '@sim/platform-authz/room-policy' -import { - ROOM_ACCESS_REVOKED_EVENT, - type RoomAccessRevokedBroadcast, -} from '@sim/realtime-protocol/events' import { ROOM_TYPES, type RoomRef, roomName } from '@sim/realtime-protocol/rooms' import { type JoinTablePayload, @@ -12,6 +8,7 @@ import { type TableCellSelection, } from '@sim/realtime-protocol/table-presence' import { resolveAvatarUrl } from '@/handlers/avatar' +import { evictSocketFromRoom } from '@/handlers/room-eviction' import { resolveRoomJoinAuth } from '@/handlers/room-join-auth' import type { AuthenticatedSocket } from '@/middleware/auth' import { peekRoomPermission, resolveCurrentRoomPermission } from '@/middleware/permissions' @@ -92,24 +89,17 @@ function normalizeCellSelection(cell: unknown): TableCellSelection | undefined { } /** - * Evicts a socket from a table room after a confirmed loss of access: emit the - * revocation, leave the Socket.IO room, and drop its presence so peers stop seeing - * its selection. Best-effort on the presence half — the socket has already left the - * room, and the sweep's cleanup lane retries any removal that fails here. + * Evicts a socket from a table room after a confirmed loss of access, then drops + * its presence so peers stop seeing its selection. The presence half is + * best-effort — the socket has already left the room, and the sweep's cleanup lane + * retries any removal that fails here. */ async function evictFromTable( socket: AuthenticatedSocket, roomManager: IRoomManager, room: RoomRef ): Promise { - const payload: RoomAccessRevokedBroadcast = { - room, - message: 'Your access to this table has been revoked', - timestamp: Date.now(), - } - socket.emit(ROOM_ACCESS_REVOKED_EVENT, payload) - socket.leave(roomName(room)) - logger.warn(`Evicted user ${socket.userId} from table room ${room.id}: access revoked`) + evictSocketFromRoom(socket, room, 'Your access to this table has been revoked', roomManager.io) try { await roomManager.removeUserFromRoom(room, socket.id) await roomManager.broadcastPresenceUpdate(room, socket.id) From 28192ff2e5f301014ad3df7f426b8f7803e3f0d6 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 1 Aug 2026 16:36:14 -0700 Subject: [PATCH 03/11] fix(realtime): re-check access before workspace-list room joins too The workspace-files / workspace-tables joins committed straight from their authorize result, so a join that authorized just before a revocation could put the socket back in a room the sweep had already evicted it from. Mirrors the guard the file-doc and table joins already had. --- .../workspace-invalidation-room.test.ts | 25 +++++++++++++++++++ .../handlers/workspace-invalidation-room.ts | 19 +++++++++++++- 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/apps/realtime/src/handlers/workspace-invalidation-room.test.ts b/apps/realtime/src/handlers/workspace-invalidation-room.test.ts index 643fca696f0..2373c36fe25 100644 --- a/apps/realtime/src/handlers/workspace-invalidation-room.test.ts +++ b/apps/realtime/src/handlers/workspace-invalidation-room.test.ts @@ -19,6 +19,7 @@ vi.mock('@sim/platform-authz/rooms', () => ({ })) import { setupWorkspaceInvalidationRoom } from '@/handlers/workspace-invalidation-room' +import { recordRoomPermission } from '@/middleware/permissions' type Payload = { workspaceId?: string } @@ -157,6 +158,30 @@ describe.each([ROOM_TYPES.WORKSPACE_FILES, ROOM_TYPES.WORKSPACE_TABLES] as const expect(roomManager.broadcastPresenceUpdate).not.toHaveBeenCalled() }) + it('does not join when access was revoked while the join was in flight', async () => { + // The sweep records a revocation before it evicts, so a join whose authorize + // completed just before that must not put the socket back in the room. + const { handlers, socket } = createSocket({ id: 'socket-race', userId: 'user-race' }) + setupWorkspaceInvalidationRoom( + socket as unknown as Parameters[0], + createRoomManager(), + roomType + ) + + mockAuthorizeRoom.mockImplementation(async () => { + recordRoomPermission('user-race', { type: roomType, id: 'ws-race' }, null) + return { allowed: true, status: 200, workspaceId: 'ws-race', workspacePermission: 'admin' } + }) + + await handlers[joinEvent]({ workspaceId: 'ws-race' }) + + expect(socket.emit).toHaveBeenCalledWith( + errorEvent, + expect.objectContaining({ code: 'ACCESS_DENIED', retryable: false }) + ) + expect(socket.join).not.toHaveBeenCalled() + }) + it('leaves a previously-joined room when switching workspaces', async () => { const { socket, handlers, rooms } = createSocket() rooms.add(roomOf('ws-old')) diff --git a/apps/realtime/src/handlers/workspace-invalidation-room.ts b/apps/realtime/src/handlers/workspace-invalidation-room.ts index 9560740c35d..95e78958121 100644 --- a/apps/realtime/src/handlers/workspace-invalidation-room.ts +++ b/apps/realtime/src/handlers/workspace-invalidation-room.ts @@ -1,8 +1,9 @@ import { createLogger } from '@sim/logger' -import { ROOM_MEMBERSHIP_ACTIONS } from '@sim/platform-authz/room-policy' +import { ROOM_MEMBERSHIP_ACTIONS, satisfiesRoomMembership } from '@sim/platform-authz/room-policy' import { type RoomRef, type RoomType, roomName } from '@sim/realtime-protocol/rooms' import { resolveRoomJoinAuth } from '@/handlers/room-join-auth' import type { AuthenticatedSocket } from '@/middleware/auth' +import { peekRoomPermission } from '@/middleware/permissions' import type { IRoomManager } from '@/rooms' const logger = createLogger('WorkspaceInvalidationRoom') @@ -107,6 +108,22 @@ export function setupWorkspaceInvalidationRoom( // stale join can't leave the room the client has since switched to. if (joinGeneration !== joinAttempt || socket.disconnected) return + // Re-check the cached decision before committing: the access re-validation sweep + // records a revocation BEFORE it evicts, so a join that authorized just before the + // revocation must not complete afterwards and put the socket back in the room. + // `undefined` (nothing cached) is "unknown", never a denial — the authorize above + // is then the freshest word we have. Mirrors the file-doc and table joins. + const recheck = peekRoomPermission(socket.userId, ref) + if (recheck !== undefined && !satisfiesRoomMembership(recheck, roomType)) { + socket.emit(errorEvent, { + workspaceId, + error: 'Access denied to workspace', + code: 'ACCESS_DENIED', + retryable: false, + }) + return + } + // Leave any previously-joined room of this type (workspace switch), read straight from the // socket's native room membership so there's no presence store to keep in sync. const target = roomName(ref) From 950d8a0b0fc1cb72076586351d1c6eab2c8b428c Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 1 Aug 2026 16:40:24 -0700 Subject: [PATCH 04/11] fix(realtime): order role-cache writes by read start, not write time Two authorizations can start in one order and finish in the other, so the decision written last can come from the older read. A join that authorized before a revocation but returned after the sweep's denial would bury it, handing the socket another full cache TTL of access. Every writer now takes a monotonic ticket before it queries and yields only to a later-started read. --- apps/realtime/src/handlers/room-join-auth.ts | 12 +- .../src/middleware/permissions.test.ts | 37 +++++ apps/realtime/src/middleware/permissions.ts | 126 ++++++++++-------- 3 files changed, 116 insertions(+), 59 deletions(-) diff --git a/apps/realtime/src/handlers/room-join-auth.ts b/apps/realtime/src/handlers/room-join-auth.ts index 7b4d972d523..f54d6a81782 100644 --- a/apps/realtime/src/handlers/room-join-auth.ts +++ b/apps/realtime/src/handlers/room-join-auth.ts @@ -1,7 +1,7 @@ import type { createLogger } from '@sim/logger' import { authorizeRoom } from '@sim/platform-authz/rooms' import type { RoomRef } from '@sim/realtime-protocol/rooms' -import { recordRoomPermissionIfUnchanged, snapshotRoomPermission } from '@/middleware/permissions' +import { beginRoomPermissionRead, commitRoomPermission } from '@/middleware/permissions' type Authorized = Awaited> @@ -35,10 +35,10 @@ export async function resolveRoomJoinAuth( const { userId, room, action, logger, logLabel, messages, emitError } = params let authorized: Authorized - // Captured before the query so a decision recorded WHILE it was in flight (the - // access-revalidation sweep evicting this user) is never overwritten by this - // older read — see {@link recordRoomPermissionIfUnchanged}. - const snapshot = snapshotRoomPermission(userId, room) + // Taken before the query so this read is ordered against every other one: a + // decision from a later-started read (the access-revalidation sweep's denial) + // is never overwritten by this older result — see {@link commitRoomPermission}. + const readSeq = beginRoomPermissionRead() try { authorized = await authorizeRoom({ userId, room, action }) } catch (error) { @@ -54,7 +54,7 @@ export async function resolveRoomJoinAuth( // not authorizable here) resolved no permission at all and is deliberately not // recorded. A 404 records `null`: the resource is genuinely gone. if (authorized.status !== 400) { - recordRoomPermissionIfUnchanged(userId, room, authorized.workspacePermission, snapshot) + commitRoomPermission(userId, room, authorized.workspacePermission, readSeq) } if (!authorized.allowed) { diff --git a/apps/realtime/src/middleware/permissions.test.ts b/apps/realtime/src/middleware/permissions.test.ts index d9f0b4d15f0..130b1a262a2 100644 --- a/apps/realtime/src/middleware/permissions.test.ts +++ b/apps/realtime/src/middleware/permissions.test.ts @@ -519,6 +519,43 @@ describe('verifyWorkflowAccess role-cache refresh', () => { expect(await resolveCurrentWorkflowRole(userId, workflowId, 'read')).toBe('write') }) + it('does not let a stale join-time allow bury a sweep denial that started later', async () => { + const userId = 'vw-user-3' + const workflowId = 'vw-wf-3' + + // Hand out a controllable promise per authorization call, so the two reads can be + // started in one order and settled in the other. + const settle: Array<(value: { allowed: boolean; workspacePermission: string | null }) => void> = + [] + mockAuthorize.mockImplementation( + () => + new Promise((resolve) => { + settle.push(resolve) + }) + ) + + // A join-style verify starts FIRST (against pre-revocation state) and stalls. + const staleJoin = verifyWorkflowAccess(userId, workflowId) + for (let i = 0; i < 10 && settle.length < 1; i++) await Promise.resolve() + expect(settle).toHaveLength(1) + + // The sweep's authorization starts AFTER it, and stalls too. + const sweep = resolveCurrentWorkflowRole(userId, workflowId, 'read') + for (let i = 0; i < 10 && settle.length < 2; i++) await Promise.resolve() + expect(settle).toHaveLength(2) + + // The sweep's denial lands first, then the older join's allow. Ordering by WRITE + // time would let the join bury the denial and hand the socket another full TTL of + // access; ordering by read start keeps the denial in force. + settle[1]({ allowed: false, workspacePermission: null }) + expect(await sweep).toBeNull() + settle[0]({ allowed: true, workspacePermission: 'write' }) + await staleJoin + + mockAuthorize.mockRejectedValue(new Error('must not re-query')) + expect(await resolveCurrentWorkflowRole(userId, workflowId, 'read')).toBeNull() + }) + it('does not let a stale in-flight resolution overwrite a fresher verify decision', async () => { const userId = 'vw-user-2' const workflowId = 'vw-wf-2' diff --git a/apps/realtime/src/middleware/permissions.ts b/apps/realtime/src/middleware/permissions.ts index 545214b0e28..2795c16dfa1 100644 --- a/apps/realtime/src/middleware/permissions.ts +++ b/apps/realtime/src/middleware/permissions.ts @@ -102,9 +102,34 @@ const MAX_ROLE_CACHE_ENTRIES = 5_000 interface CachedRole { /** Authoritative workspace role, or `null` when the user has no access. */ role: string | null + /** + * The {@link beginRoomPermissionRead} ticket of the query this decision came + * from — i.e. when its DB read STARTED, not when it was written. + */ + readSeq: number expiresAt: number } +/** + * Monotonic ticket counter establishing a total order over authorization READS. + * + * Ordering by write time is not sufficient: two readers can start their queries in + * one order and finish in the other, so the decision written last can be derived + * from the older read. A join that authorized before a revocation but returned + * after the sweep's denial would then win — leaving a revoked user with another + * full TTL of access. Every writer therefore takes a ticket before it queries and + * a decision only yields to one from a strictly later-started read. + */ +let roleReadSeqCounter = 0 + +/** + * Takes a read ticket. Call immediately BEFORE issuing an authorization query, and + * pass the result to {@link commitRoomPermission} once it returns. + */ +export function beginRoomPermissionRead(): number { + return ++roleReadSeqCounter +} + /** * Per-pod cache of authoritative workspace roles, keyed by * `${userId}:${roomName(room)}`. @@ -142,19 +167,31 @@ function purgeExpiredRoles(now: number): void { } } -/** - * Records a freshly-read authoritative decision into the role cache. Every - * successful DB read of a user's workspace role goes through this — including - * the join-time {@link verifyWorkflowAccess} — so a stale cached revocation - * never outlives a newer authoritative read (e.g. a re-granted user re-joining - * within the TTL of the sweep's recorded `null`). - */ -function recordRoleDecision(key: string, role: string | null): void { +function recordRoleDecision(key: string, role: string | null, readSeq: number): void { const now = Date.now() if (roleCache.size >= MAX_ROLE_CACHE_ENTRIES) { purgeExpiredRoles(now) } - roleCache.set(key, { role, expiresAt: now + ROLE_REVALIDATION_TTL_MS }) + roleCache.set(key, { role, readSeq, expiresAt: now + ROLE_REVALIDATION_TTL_MS }) +} + +/** + * Commits a freshly-read authoritative decision, unless a decision from a + * strictly later-started read is already cached — in which case that one stands + * and is returned instead. + * + * Every successful DB read of a user's workspace role goes through this: the + * cached resolver, the join-time {@link verifyWorkflowAccess}, and the room join + * authorizer. That is what keeps a stale cached revocation from outliving a newer + * authoritative read (a re-granted user re-joining inside the sweep's recorded + * `null` TTL) while equally keeping a stale ALLOW from burying the sweep's fresher + * denial. Returns the decision that ends up in force. + */ +function commitRoleDecision(key: string, role: string | null, readSeq: number): string | null { + const existing = roleCache.get(key) + if (existing !== undefined && existing.readSeq > readSeq) return existing.role + recordRoleDecision(key, role, readSeq) + return role } /** @@ -195,19 +232,13 @@ async function resolveRoleUncached( room: RoomRef, fallbackRole: string ): Promise { - const entryBeforeQuery = roleCache.get(key) + const readSeq = beginRoomPermissionRead() try { const role = await readAuthoritativeRoomPermission(userId, room) - // A fresh authoritative read (e.g. a join-time verifyWorkflowAccess) may - // have recorded a decision while this query was in flight. That write is - // newer than this query's read snapshot, so prefer it instead of - // overwriting it with a potentially stale result. - const entryAfterQuery = roleCache.get(key) - if (entryAfterQuery !== undefined && entryAfterQuery !== entryBeforeQuery) { - return entryAfterQuery.role - } - recordRoleDecision(key, role) - return role + // Yields only to a decision from a later-STARTED read (see commitRoleDecision). + // Comparing write order instead would let a join whose authorize began before + // this one — but returned after it — bury this result. + return commitRoleDecision(key, role, readSeq) } catch (error) { logger.warn( `Failed to re-validate role for user ${userId} on ${roomName(room)}; using last known role`, @@ -291,50 +322,35 @@ export function peekRoomPermission(userId: string, room: RoomRef): string | null } /** - * Records an authoritative decision read outside this module (the room join + * Commits an authoritative decision read outside this module (the room join * authorizer) into the shared cache, so the sweep and the per-frame gates start - * warm and a re-granted user is never held out by a stale cached revocation. + * warm on the room a socket just joined. * - * Unconditional: the caller's read is taken as the newest word. Use - * {@link recordRoomPermissionIfUnchanged} when the read was slow enough that a - * newer decision could have landed meanwhile. + * `readSeq` must come from a {@link beginRoomPermissionRead} taken BEFORE the + * caller's query: the decision is discarded if a later-started read already + * committed one, so neither a stale ALLOW can bury the sweep's fresher denial nor + * a stale revocation outlive a re-grant. */ -export function recordRoomPermission( +export function commitRoomPermission( userId: string, room: RoomRef, - permission: PermissionType | null + permission: PermissionType | null, + readSeq: number ): void { - recordRoleDecision(roleCacheKey(userId, room), permission) -} - -/** Opaque token identifying the cache entry a caller observed before its query. */ -export type RoomPermissionSnapshot = object | undefined - -/** Captures the current cache entry for a (user, room), for {@link recordRoomPermissionIfUnchanged}. */ -export function snapshotRoomPermission(userId: string, room: RoomRef): RoomPermissionSnapshot { - return roleCache.get(roleCacheKey(userId, room)) + commitRoleDecision(roleCacheKey(userId, room), permission, readSeq) } /** - * Records an authoritative decision only if no other decision landed since - * `snapshot` was taken. - * - * Without this, a join whose authorization query started before a revocation but - * returned after the sweep recorded that revocation would overwrite it with its own - * stale "allowed", handing the socket another full TTL of access. Entry identity - * (not a timestamp) is the comparison, mirroring the in-flight ordering guard the - * cached resolver already applies to its own queries — so it is exact regardless of - * how many writes land inside one millisecond. + * Records a decision as the newest word, taking its read ticket at write time. + * For callers that have just observed the authoritative state with no query of + * their own to order against. */ -export function recordRoomPermissionIfUnchanged( +export function recordRoomPermission( userId: string, room: RoomRef, - permission: PermissionType | null, - snapshot: RoomPermissionSnapshot + permission: PermissionType | null ): void { - const key = roleCacheKey(userId, room) - if (roleCache.get(key) !== snapshot) return - recordRoleDecision(key, permission) + recordRoleDecision(roleCacheKey(userId, room), permission, beginRoomPermissionRead()) } /** @@ -376,6 +392,9 @@ export async function verifyWorkflowAccess( userId: string, workflowId: string ): Promise<{ hasAccess: boolean; role?: string; workspaceId?: string }> { + // Taken before the reads below, so this decision yields to any authorization + // that started later (e.g. the sweep's denial) instead of by write order. + const readSeq = beginRoomPermissionRead() try { const workflowData = await db .select({ @@ -398,9 +417,10 @@ export async function verifyWorkflowAccess( action: 'read', }) - recordRoleDecision( + commitRoleDecision( roleCacheKey(userId, { type: ROOM_TYPES.WORKFLOW, id: workflowId }), - authorization.allowed ? (authorization.workspacePermission ?? null) : null + authorization.allowed ? (authorization.workspacePermission ?? null) : null, + readSeq ) if (!authorization.allowed || !authorization.workspacePermission) { From 98d82bdbbe50b238e5e574156efeb599da98ea4b Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 1 Aug 2026 16:43:04 -0700 Subject: [PATCH 05/11] chore(realtime): drop the test-only unguarded role-cache writer Tests can express the same setup with commitRoomPermission + a read ticket, so the cache has exactly one write path and no export without a production caller. --- apps/realtime/src/handlers/file-doc.test.ts | 9 +++++++-- apps/realtime/src/handlers/tables.test.ts | 9 +++++++-- .../handlers/workspace-invalidation-room.test.ts | 9 +++++++-- apps/realtime/src/middleware/permissions.ts | 13 ------------- 4 files changed, 21 insertions(+), 19 deletions(-) diff --git a/apps/realtime/src/handlers/file-doc.test.ts b/apps/realtime/src/handlers/file-doc.test.ts index b2c683c590c..2cbf73e286b 100644 --- a/apps/realtime/src/handlers/file-doc.test.ts +++ b/apps/realtime/src/handlers/file-doc.test.ts @@ -39,7 +39,7 @@ import { flushAllFileDocRooms, setupWorkspaceFileDocHandlers, } from '@/handlers/file-doc' -import { recordRoomPermission } from '@/middleware/permissions' +import { beginRoomPermissionRead, commitRoomPermission } from '@/middleware/permissions' type Handler = (payload?: unknown) => Promise | void @@ -255,7 +255,12 @@ describe('setupWorkspaceFileDocHandlers', () => { mockAuthorizeRoom.mockImplementation(async () => { // Simulate the revocation landing between this join's authorize and its commit, // exactly as the sweep would record it. - recordRoomPermission('user-race', { type: ROOM_TYPES.WORKSPACE_FILE_DOC, id: 'file-1' }, null) + commitRoomPermission( + 'user-race', + { type: ROOM_TYPES.WORKSPACE_FILE_DOC, id: 'file-1' }, + null, + beginRoomPermissionRead() + ) return { allowed: true, status: 200, workspaceId: 'ws-1', workspacePermission: 'write' } }) diff --git a/apps/realtime/src/handlers/tables.test.ts b/apps/realtime/src/handlers/tables.test.ts index 09169396ad2..a25f88d76b8 100644 --- a/apps/realtime/src/handlers/tables.test.ts +++ b/apps/realtime/src/handlers/tables.test.ts @@ -20,7 +20,7 @@ vi.mock('@sim/platform-authz/rooms', () => ({ })) import { setupTablesHandlers } from '@/handlers/tables' -import { recordRoomPermission } from '@/middleware/permissions' +import { beginRoomPermissionRead, commitRoomPermission } from '@/middleware/permissions' const TABLE_ROOM = { type: ROOM_TYPES.TABLE, id: 'table-1' } @@ -183,7 +183,12 @@ describe('setupTablesHandlers', () => { setupTablesHandlers(socket as unknown as SetupArg, roomManager) mockAuthorizeRoom.mockImplementation(async () => { - recordRoomPermission('user-race', { type: ROOM_TYPES.TABLE, id: 'table-race' }, null) + commitRoomPermission( + 'user-race', + { type: ROOM_TYPES.TABLE, id: 'table-race' }, + null, + beginRoomPermissionRead() + ) return { allowed: true, status: 200, workspaceId: 'ws-1', workspacePermission: 'admin' } }) diff --git a/apps/realtime/src/handlers/workspace-invalidation-room.test.ts b/apps/realtime/src/handlers/workspace-invalidation-room.test.ts index 2373c36fe25..e623eafa9af 100644 --- a/apps/realtime/src/handlers/workspace-invalidation-room.test.ts +++ b/apps/realtime/src/handlers/workspace-invalidation-room.test.ts @@ -19,7 +19,7 @@ vi.mock('@sim/platform-authz/rooms', () => ({ })) import { setupWorkspaceInvalidationRoom } from '@/handlers/workspace-invalidation-room' -import { recordRoomPermission } from '@/middleware/permissions' +import { beginRoomPermissionRead, commitRoomPermission } from '@/middleware/permissions' type Payload = { workspaceId?: string } @@ -169,7 +169,12 @@ describe.each([ROOM_TYPES.WORKSPACE_FILES, ROOM_TYPES.WORKSPACE_TABLES] as const ) mockAuthorizeRoom.mockImplementation(async () => { - recordRoomPermission('user-race', { type: roomType, id: 'ws-race' }, null) + commitRoomPermission( + 'user-race', + { type: roomType, id: 'ws-race' }, + null, + beginRoomPermissionRead() + ) return { allowed: true, status: 200, workspaceId: 'ws-race', workspacePermission: 'admin' } }) diff --git a/apps/realtime/src/middleware/permissions.ts b/apps/realtime/src/middleware/permissions.ts index 2795c16dfa1..7d88ed4b865 100644 --- a/apps/realtime/src/middleware/permissions.ts +++ b/apps/realtime/src/middleware/permissions.ts @@ -340,19 +340,6 @@ export function commitRoomPermission( commitRoleDecision(roleCacheKey(userId, room), permission, readSeq) } -/** - * Records a decision as the newest word, taking its read ticket at write time. - * For callers that have just observed the authoritative state with no query of - * their own to order against. - */ -export function recordRoomPermission( - userId: string, - room: RoomRef, - permission: PermissionType | null -): void { - recordRoleDecision(roleCacheKey(userId, room), permission, beginRoomPermissionRead()) -} - /** * Live permission gate for mutating socket operations. Re-validates the user's workspace * role against the database (cached per pod for {@link ROLE_REVALIDATION_TTL_MS}) so that From 1086dc31117e033a71f30a09c069dbfc5ef858d3 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 1 Aug 2026 16:49:16 -0700 Subject: [PATCH 06/11] fix(realtime): keep handler-initiated table eviction retryable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Evicting leaves the Socket.IO room synchronously, which is also how the sweep discovers work — so a presence removal failing in the per-frame path could never be retried and left a ghost collaborator until disconnect. Failed (or unconfirmed) removals now hand off to the sweep's existing cleanup lane instead of a second retry loop. --- apps/realtime/src/access-revalidation.ts | 19 ++++++++- apps/realtime/src/handlers/room-eviction.ts | 31 ++++++++++++++ apps/realtime/src/handlers/tables.test.ts | 45 +++++++++++++++++++++ apps/realtime/src/handlers/tables.ts | 28 ++++++++++--- 4 files changed, 115 insertions(+), 8 deletions(-) diff --git a/apps/realtime/src/access-revalidation.ts b/apps/realtime/src/access-revalidation.ts index 4afe096bd19..8e0ca514812 100644 --- a/apps/realtime/src/access-revalidation.ts +++ b/apps/realtime/src/access-revalidation.ts @@ -9,7 +9,11 @@ import { roomName, } from '@sim/realtime-protocol/rooms' import { sleep } from '@sim/utils/helpers' -import { evictSocketFromRoom, runRoomEvictionHandler } from '@/handlers/room-eviction' +import { + evictSocketFromRoom, + runRoomEvictionHandler, + setEvictionCleanupSink, +} from '@/handlers/room-eviction' import type { AuthenticatedSocket } from '@/middleware/auth' import { ROLE_REVALIDATION_TTL_MS, resolveCurrentRoomPermission } from '@/middleware/permissions' import type { IRoomManager } from '@/rooms' @@ -255,6 +259,14 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR }) } + // Handler-initiated evictions (the per-frame gates) hand failed presence cleanups + // here: they have already left the Socket.IO room, so the scan can no longer + // rediscover them, and this lane is the only thing that retries. + setEvictionCleanupSink((socketId, room) => { + pendingCleanups.set(`${socketId}:${roomName(room)}`, { socketId, room }) + launchCleanups() + }) + function revokeSocket(socket: AuthenticatedSocket, room: RoomRef, name: string): void { // Security-critical, pod-local, and synchronous: stop this socket receiving // room broadcasts immediately, and drop the handler-local state that would @@ -380,7 +392,10 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR ) return { - stop: () => clearInterval(timer), + stop: () => { + clearInterval(timer) + setEvictionCleanupSink(null) + }, runOnce, } } diff --git a/apps/realtime/src/handlers/room-eviction.ts b/apps/realtime/src/handlers/room-eviction.ts index bd6ebace6e6..2a417489b64 100644 --- a/apps/realtime/src/handlers/room-eviction.ts +++ b/apps/realtime/src/handlers/room-eviction.ts @@ -46,6 +46,37 @@ export function runRoomEvictionHandler(socketId: string, room: RoomRef, io: Serv } } +/** Enqueues a presence cleanup owed for an evicted socket. */ +type EvictionCleanupSink = (socketId: string, room: RoomRef) => void + +let evictionCleanupSink: EvictionCleanupSink | null = null + +/** + * Registers the retrying presence-cleanup lane (owned by the access-revalidation + * sweep) that handler-initiated evictions hand failed cleanups to. + * + * An eviction removes the socket from the Socket.IO room synchronously, which is + * also how the sweep discovers work — so once a handler evicts, the sweep can no + * longer find that (socket, room) pair to retry a Redis removal that failed. This + * sink is the handoff that keeps the sweep's retry semantics (re-join guard, + * moved-room guard, no infinite retry) as the single implementation instead of a + * second, subtly different retry loop per handler. Unset outside a running sweep, + * where {@link requestEvictionCleanup} is a no-op. + */ +export function setEvictionCleanupSink(sink: EvictionCleanupSink | null): void { + evictionCleanupSink = sink +} + +/** + * Asks the sweep's cleanup lane to (re)try presence removal for a socket already + * evicted from `room`. Call only when the immediate best-effort removal failed — + * the happy path stays immediate so peers see the departure without waiting for a + * sweep tick. + */ +export function requestEvictionCleanup(socketId: string, room: RoomRef): void { + evictionCleanupSink?.(socketId, room) +} + /** * Evicts one socket from one non-workflow room after a confirmed loss of access: * tell the client, stop it receiving room broadcasts, and drop the handler-local diff --git a/apps/realtime/src/handlers/tables.test.ts b/apps/realtime/src/handlers/tables.test.ts index a25f88d76b8..15b0012ce0e 100644 --- a/apps/realtime/src/handlers/tables.test.ts +++ b/apps/realtime/src/handlers/tables.test.ts @@ -19,6 +19,7 @@ vi.mock('@sim/platform-authz/rooms', () => ({ authorizeRoom: mockAuthorizeRoom, })) +import { setEvictionCleanupSink } from '@/handlers/room-eviction' import { setupTablesHandlers } from '@/handlers/tables' import { beginRoomPermissionRead, commitRoomPermission } from '@/middleware/permissions' @@ -251,6 +252,50 @@ describe('setupTablesHandlers', () => { } }) + it('hands a failed presence removal to the sweep so the eviction stays retryable', async () => { + // The eviction already left the Socket.IO room, so the sweep's scan can no longer + // rediscover this socket — a failed removal here would strand a ghost collaborator + // until disconnect unless it is handed to the retrying cleanup lane. + vi.useFakeTimers() + const owed: Array<{ socketId: string; roomId: string }> = [] + setEvictionCleanupSink((socketId, room) => owed.push({ socketId, roomId: room.id })) + try { + const room = { type: ROOM_TYPES.TABLE, id: 'table-defer' } + const { socket, handlers } = createSocket({ id: 'socket-defer', userId: 'user-defer' }) + const roomManager = createRoomManager({ + getRoomForSocket: vi.fn().mockResolvedValue(room), + removeUserFromRoom: vi.fn().mockRejectedValue(new Error('redis down')), + }) + setupTablesHandlers(socket as unknown as SetupArg, roomManager) + + await handlers[TABLE_PRESENCE_EVENTS.JOIN]({ tableId: 'table-defer' }) + await vi.advanceTimersByTimeAsync(0) + + mockAuthorizeRoom.mockResolvedValue({ + allowed: false, + status: 403, + workspaceId: 'ws-1', + workspacePermission: null, + }) + await vi.advanceTimersByTimeAsync(31_000) + + const cell = { + anchor: { rowId: 'row-1', columnId: 'col-a' }, + focus: { rowId: 'row-1', columnId: 'col-a' }, + } + await handlers[TABLE_PRESENCE_EVENTS.CELL_SELECTION]({ cell }) + await vi.advanceTimersByTimeAsync(0) + await handlers[TABLE_PRESENCE_EVENTS.CELL_SELECTION]({ cell }) + await vi.advanceTimersByTimeAsync(0) + + expect(socket.leave).toHaveBeenCalledWith('table:table-defer') + expect(owed).toEqual([{ socketId: 'socket-defer', roomId: 'table-defer' }]) + } finally { + setEvictionCleanupSink(null) + vi.useRealTimers() + } + }) + it('drops a malformed cell selection without storing or relaying it', async () => { const { socket, handlers, toEmit } = createSocket() const roomManager = createRoomManager({ diff --git a/apps/realtime/src/handlers/tables.ts b/apps/realtime/src/handlers/tables.ts index b7bb686664e..7f5ecbabffa 100644 --- a/apps/realtime/src/handlers/tables.ts +++ b/apps/realtime/src/handlers/tables.ts @@ -8,7 +8,7 @@ import { type TableCellSelection, } from '@sim/realtime-protocol/table-presence' import { resolveAvatarUrl } from '@/handlers/avatar' -import { evictSocketFromRoom } from '@/handlers/room-eviction' +import { evictSocketFromRoom, requestEvictionCleanup } from '@/handlers/room-eviction' import { resolveRoomJoinAuth } from '@/handlers/room-join-auth' import type { AuthenticatedSocket } from '@/middleware/auth' import { peekRoomPermission, resolveCurrentRoomPermission } from '@/middleware/permissions' @@ -90,9 +90,14 @@ function normalizeCellSelection(cell: unknown): TableCellSelection | undefined { /** * Evicts a socket from a table room after a confirmed loss of access, then drops - * its presence so peers stop seeing its selection. The presence half is - * best-effort — the socket has already left the room, and the sweep's cleanup lane - * retries any removal that fails here. + * its presence so peers stop seeing its selection. + * + * The eviction leaves the Socket.IO room immediately, which is also how the sweep + * discovers work — so a presence removal that fails here would be unretryable and + * strand a ghost collaborator until disconnect. The removal is therefore attempted + * inline (peers see the departure at once) and handed to the sweep's retrying + * cleanup lane if it fails or reports nothing removed, which is the same signal + * the sweep treats as a deferrable failure. */ async function evictFromTable( socket: AuthenticatedSocket, @@ -101,10 +106,21 @@ async function evictFromTable( ): Promise { evictSocketFromRoom(socket, room, 'Your access to this table has been revoked', roomManager.io) try { - await roomManager.removeUserFromRoom(room, socket.id) + // `false` conflates "already gone" with a transport error the manager swallowed; + // deferring on it is harmless (the lane drops a cleanup it finds already clean) + // and is the only signal a swallowed failure gives us. + const removed = await roomManager.removeUserFromRoom(room, socket.id) + if (!removed) { + requestEvictionCleanup(socket.id, room) + return + } await roomManager.broadcastPresenceUpdate(room, socket.id) } catch (error) { - logger.warn(`Presence cleanup failed for evicted table socket ${socket.id}`, error) + logger.warn( + `Presence cleanup failed for evicted table socket ${socket.id}; deferring to the sweep`, + error + ) + requestEvictionCleanup(socket.id, room) } } From 7323300ae2d15816ee79a78aac03b8a1e5bd3ca0 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 1 Aug 2026 16:52:25 -0700 Subject: [PATCH 07/11] fix(realtime): re-resolve access at join commit instead of peeking the cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pre-commit recheck peeked the role cache, which reports an EXPIRED entry as unknown and fails open — so a join stalled longer than the cache TTL could re-enter a room the sweep had already evicted it from, including a file-doc room where the next cold-cache frame is accepted as a durable write. All three joins now re-resolve the way the workflow join always has; it is normally a cache hit, since the join's own authorize just warmed it. --- apps/realtime/src/handlers/file-doc.test.ts | 37 +++++++++++++++++++ apps/realtime/src/handlers/file-doc.ts | 30 ++++++++------- apps/realtime/src/handlers/tables.ts | 14 ++++--- .../handlers/workspace-invalidation-room.ts | 20 ++++++---- 4 files changed, 74 insertions(+), 27 deletions(-) diff --git a/apps/realtime/src/handlers/file-doc.test.ts b/apps/realtime/src/handlers/file-doc.test.ts index 2cbf73e286b..c5bce1fc7eb 100644 --- a/apps/realtime/src/handlers/file-doc.test.ts +++ b/apps/realtime/src/handlers/file-doc.test.ts @@ -275,6 +275,43 @@ describe('setupWorkspaceFileDocHandlers', () => { expect(joinSuccessFileId(socket)).toBeUndefined() }) + it('re-reads access when the cached decision expired mid-join, instead of failing open', async () => { + // A join stalled longer than the cache TTL: the sweep's denial is recorded with a + // later read ticket (so this join's own allow is correctly dropped) but has since + // expired. Peeking the cache would read that as "unknown" and let the socket back + // into the document, so the join must re-resolve against the database. + vi.useFakeTimers() + try { + const room = { type: ROOM_TYPES.WORKSPACE_FILE_DOC, id: 'file-stale' } + const { io } = createIo() + const { socket, handlers } = setup('socket-stale', io, { userId: 'user-stale' }) + + mockAuthorizeRoom.mockImplementation(async ({ action }: { action: string }) => { + // The authoritative current answer: access is gone. + if (action !== 'write') + return { allowed: false, status: 403, workspaceId: 'ws-1', workspacePermission: null } + // This join's own authorize saw the pre-revocation state, and the sweep records + // the revocation (later read ticket) while it is still in flight. + commitRoomPermission('user-stale', room, null, beginRoomPermissionRead()) + await new Promise((resolve) => setTimeout(resolve, 31_000)) + return { allowed: true, status: 200, workspaceId: 'ws-1', workspacePermission: 'write' } + }) + + const joining = handlers[FILE_DOC_EVENTS.JOIN]({ fileId: 'file-stale', clientId: 1 }) + await vi.advanceTimersByTimeAsync(31_000) + await joining + + expect(socket.emit).toHaveBeenCalledWith( + FILE_DOC_EVENTS.JOIN_ERROR, + expect.objectContaining({ code: 'ACCESS_DENIED', retryable: false }) + ) + expect(socket.join).not.toHaveBeenCalled() + expect(joinSuccessFileId(socket)).toBeUndefined() + } finally { + vi.useRealTimers() + } + }) + it('requires write permission and reports 404 as NOT_FOUND', async () => { mockAuthorizeRoom.mockResolvedValue({ allowed: false, status: 404, workspacePermission: null }) const { io } = createIo() diff --git a/apps/realtime/src/handlers/file-doc.ts b/apps/realtime/src/handlers/file-doc.ts index 15ac171c68d..a0152fd85f6 100644 --- a/apps/realtime/src/handlers/file-doc.ts +++ b/apps/realtime/src/handlers/file-doc.ts @@ -1091,25 +1091,27 @@ export function setupWorkspaceFileDocHandlers( // awareness). Resolved here so the generation guard below also covers this await. const avatarUrl = await resolveAvatarUrl(socket, userId) + // Re-check access immediately before registering, mirroring the workflow join: the + // access re-validation sweep records a revocation BEFORE it evicts, so a join that + // authorized just before the revocation must not complete afterwards and re-bind + // the socket to the document. This RE-RESOLVES rather than peeking the cache — a + // peek treats an expired entry as unknown and fails open, which a join stalled + // longer than the cache TTL would slip straight through. Normally a cache hit (this + // join's own authorize just warmed it), so it costs no extra query. + const currentPermission = await resolveCurrentRoomPermission(userId, room, FILE_DOC_ACTION) + if (!satisfiesRoomMembership(currentPermission, ROOM_TYPES.WORKSPACE_FILE_DOC)) { + logger.warn(`User ${userId} lost write access to file ${fileId} before the join completed`) + emitJoinError(socket, fileId, 'Access denied to file', 'ACCESS_DENIED', false) + return + } + // Abort a JOIN superseded during authorization/identity resolution: the socket // disconnected, or a newer JOIN (a document switch) bumped the generation. Registering // here would leak a dead socket's room or bind the socket to the wrong document. + // Last await before the commit, so nothing can interleave between the access + // re-check above and the registration below. if (socket.disconnected || joinGeneration.get(socket.id) !== generation) return - // Re-check the cached decision immediately before registering: the access - // re-validation sweep records a revocation BEFORE it evicts, so a join that - // authorized just before the revocation cannot complete afterwards and re-bind - // the socket to the document. `undefined` (nothing cached) is "unknown", never a - // denial — the authorize above is then the freshest word we have. - const recheck = peekRoomPermission(userId, room) - if ( - recheck !== undefined && - !satisfiesRoomMembership(recheck, ROOM_TYPES.WORKSPACE_FILE_DOC) - ) { - emitJoinError(socket, fileId, 'Access denied to file', 'ACCESS_DENIED', false) - return - } - const entry = getOrCreateRoom(io, room) // A client id must be owned by at most one user, or a peer could bind an active diff --git a/apps/realtime/src/handlers/tables.ts b/apps/realtime/src/handlers/tables.ts index 7f5ecbabffa..a1b80ecefca 100644 --- a/apps/realtime/src/handlers/tables.ts +++ b/apps/realtime/src/handlers/tables.ts @@ -260,12 +260,14 @@ export function setupTablesHandlers(socket: AuthenticatedSocket, roomManager: IR // awaits above bumped the generation, or the socket disconnected. Abort before registering. if (superseded()) return - // Re-check the cached decision too: the access re-validation sweep records a - // revocation BEFORE it evicts, so a join that authorized just before the - // revocation cannot complete afterwards and put the socket back in the room. - // `undefined` (nothing cached) is "unknown", never a denial. - const recheck = peekRoomPermission(userId, room) - if (recheck !== undefined && !satisfiesRoomMembership(recheck, ROOM_TYPES.TABLE)) { + // Re-check access too: the access re-validation sweep records a revocation BEFORE + // it evicts, so a join that authorized just before the revocation must not + // complete afterwards and put the socket back in the room. RE-RESOLVES rather + // than peeking — a peek treats an expired entry as unknown and fails open, which + // a join stalled longer than the cache TTL would slip through. Normally a cache + // hit (this join's own authorize just warmed it). + const currentPermission = await resolveCurrentRoomPermission(userId, room, TABLE_ACTION) + if (!satisfiesRoomMembership(currentPermission, ROOM_TYPES.TABLE)) { socket.emit(TABLE_PRESENCE_EVENTS.JOIN_ERROR, { tableId, error: 'Access denied to table', diff --git a/apps/realtime/src/handlers/workspace-invalidation-room.ts b/apps/realtime/src/handlers/workspace-invalidation-room.ts index 95e78958121..61d28670c65 100644 --- a/apps/realtime/src/handlers/workspace-invalidation-room.ts +++ b/apps/realtime/src/handlers/workspace-invalidation-room.ts @@ -3,7 +3,7 @@ import { ROOM_MEMBERSHIP_ACTIONS, satisfiesRoomMembership } from '@sim/platform- import { type RoomRef, type RoomType, roomName } from '@sim/realtime-protocol/rooms' import { resolveRoomJoinAuth } from '@/handlers/room-join-auth' import type { AuthenticatedSocket } from '@/middleware/auth' -import { peekRoomPermission } from '@/middleware/permissions' +import { resolveCurrentRoomPermission } from '@/middleware/permissions' import type { IRoomManager } from '@/rooms' const logger = createLogger('WorkspaceInvalidationRoom') @@ -108,13 +108,19 @@ export function setupWorkspaceInvalidationRoom( // stale join can't leave the room the client has since switched to. if (joinGeneration !== joinAttempt || socket.disconnected) return - // Re-check the cached decision before committing: the access re-validation sweep - // records a revocation BEFORE it evicts, so a join that authorized just before the + // Re-check access before committing: the access re-validation sweep records a + // revocation BEFORE it evicts, so a join that authorized just before the // revocation must not complete afterwards and put the socket back in the room. - // `undefined` (nothing cached) is "unknown", never a denial — the authorize above - // is then the freshest word we have. Mirrors the file-doc and table joins. - const recheck = peekRoomPermission(socket.userId, ref) - if (recheck !== undefined && !satisfiesRoomMembership(recheck, roomType)) { + // RE-RESOLVES rather than peeking — a peek treats an expired entry as unknown and + // fails open, which a join stalled longer than the cache TTL would slip through. + // Normally a cache hit (this join's own authorize just warmed it). Mirrors the + // file-doc and table joins. + const currentPermission = await resolveCurrentRoomPermission( + socket.userId, + ref, + ROOM_MEMBERSHIP_ACTIONS[roomType] + ) + if (!satisfiesRoomMembership(currentPermission, roomType)) { socket.emit(errorEvent, { workspaceId, error: 'Access denied to workspace', From f7d63875c01b95f583c13c59cd3fc249d45ada00 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 1 Aug 2026 16:58:34 -0700 Subject: [PATCH 08/11] fix(realtime): keep the join generation guard after the access re-check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The access re-resolve added in the previous commit sat AFTER the generation / superseded guard in the table and workspace-list joins, so a leave or a newer join landing during that await no longer cancelled the stale join — it would go on to leave the room the client had switched to and commit the abandoned one. The guard is now the last thing before the commit in all three handlers, as it already was for file-doc and workflow. --- apps/realtime/src/handlers/tables.ts | 10 ++-- .../workspace-invalidation-room.test.ts | 47 +++++++++++++++++++ .../handlers/workspace-invalidation-room.ts | 9 ++-- 3 files changed, 58 insertions(+), 8 deletions(-) diff --git a/apps/realtime/src/handlers/tables.ts b/apps/realtime/src/handlers/tables.ts index a1b80ecefca..811bdeb5d4d 100644 --- a/apps/realtime/src/handlers/tables.ts +++ b/apps/realtime/src/handlers/tables.ts @@ -256,10 +256,6 @@ export function setupTablesHandlers(socket: AuthenticatedSocket, roomManager: IR } } - // Final re-check before the membership commit: a LEAVE or a newer JOIN enqueued during the - // awaits above bumped the generation, or the socket disconnected. Abort before registering. - if (superseded()) return - // Re-check access too: the access re-validation sweep records a revocation BEFORE // it evicts, so a join that authorized just before the revocation must not // complete afterwards and put the socket back in the room. RE-RESOLVES rather @@ -277,6 +273,12 @@ export function setupTablesHandlers(socket: AuthenticatedSocket, roomManager: IR return } + // Final re-check before the membership commit: a LEAVE or a newer JOIN enqueued during the + // awaits above — including the access re-resolve — bumped the generation, or the socket + // disconnected. This is the LAST await before registering, so nothing can interleave + // between it and the commit. + if (superseded()) return + socket.join(roomName(room)) const presence: UserPresence = { diff --git a/apps/realtime/src/handlers/workspace-invalidation-room.test.ts b/apps/realtime/src/handlers/workspace-invalidation-room.test.ts index e623eafa9af..1dff456a08b 100644 --- a/apps/realtime/src/handlers/workspace-invalidation-room.test.ts +++ b/apps/realtime/src/handlers/workspace-invalidation-room.test.ts @@ -158,6 +158,53 @@ describe.each([ROOM_TYPES.WORKSPACE_FILES, ROOM_TYPES.WORKSPACE_TABLES] as const expect(roomManager.broadcastPresenceUpdate).not.toHaveBeenCalled() }) + it('aborts a join superseded during the access re-check await', async () => { + // The access re-resolve is an await like any other: a leave landing during it must + // still cancel this join, or the stale join would leave the room the client + // switched to and commit the abandoned one. Forced down the re-resolve's DB path + // by expiring the cached decision mid-join, so the interleaving is deterministic + // rather than dependent on microtask ordering. + vi.useFakeTimers() + try { + const { handlers, socket } = createSocket({ id: 'socket-sup', userId: 'user-sup' }) + setupWorkspaceInvalidationRoom( + socket as unknown as Parameters[0], + createRoomManager(), + roomType + ) + + let call = 0 + mockAuthorizeRoom.mockImplementation(async () => { + call += 1 + if (call === 1) { + // A later-started read commits, so this join's own decision is dropped; then + // the join stalls past the TTL so that decision is expired by re-check time. + commitRoomPermission( + 'user-sup', + { type: roomType, id: 'ws-sup' }, + 'admin', + beginRoomPermissionRead() + ) + await new Promise((resolve) => setTimeout(resolve, 31_000)) + } else { + // Second call is the re-check's re-resolve: the client leaves during it. + handlers[leaveEvent]({ workspaceId: 'ws-sup' }) + } + return { allowed: true, status: 200, workspaceId: 'ws-sup', workspacePermission: 'admin' } + }) + + const joining = handlers[joinEvent]({ workspaceId: 'ws-sup' }) + await vi.advanceTimersByTimeAsync(31_000) + await joining + + expect(call).toBe(2) + expect(socket.join).not.toHaveBeenCalled() + expect(socket.emit).not.toHaveBeenCalledWith(successEvent, expect.anything()) + } finally { + vi.useRealTimers() + } + }) + it('does not join when access was revoked while the join was in flight', async () => { // The sweep records a revocation before it evicts, so a join whose authorize // completed just before that must not put the socket back in the room. diff --git a/apps/realtime/src/handlers/workspace-invalidation-room.ts b/apps/realtime/src/handlers/workspace-invalidation-room.ts index 61d28670c65..362e37875f6 100644 --- a/apps/realtime/src/handlers/workspace-invalidation-room.ts +++ b/apps/realtime/src/handlers/workspace-invalidation-room.ts @@ -104,10 +104,6 @@ export function setupWorkspaceInvalidationRoom( }) if (!authorized) return - // A newer join started on this socket during authorize (or it dropped): abort so a - // stale join can't leave the room the client has since switched to. - if (joinGeneration !== joinAttempt || socket.disconnected) return - // Re-check access before committing: the access re-validation sweep records a // revocation BEFORE it evicts, so a join that authorized just before the // revocation must not complete afterwards and put the socket back in the room. @@ -130,6 +126,11 @@ export function setupWorkspaceInvalidationRoom( return } + // A newer join started on this socket during the awaits above — including the access + // re-resolve — or it dropped: abort so a stale join can't leave the room the client has + // since switched to. Last await before the commit, so nothing interleaves after it. + if (joinGeneration !== joinAttempt || socket.disconnected) return + // Leave any previously-joined room of this type (workspace switch), read straight from the // socket's native room membership so there's no presence store to keep in sync. const target = roomName(ref) From 2dc2d59e8d849c5427fde389b4a599c9c0526766 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 1 Aug 2026 17:02:21 -0700 Subject: [PATCH 09/11] test(realtime): use the shared sleep helper in the new join tests check:utils bans the inline new Promise(setTimeout) form; the two stalled-join tests were the only new offenders. --- apps/realtime/src/handlers/file-doc.test.ts | 3 ++- apps/realtime/src/handlers/workspace-invalidation-room.test.ts | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/apps/realtime/src/handlers/file-doc.test.ts b/apps/realtime/src/handlers/file-doc.test.ts index c5bce1fc7eb..938092d4484 100644 --- a/apps/realtime/src/handlers/file-doc.test.ts +++ b/apps/realtime/src/handlers/file-doc.test.ts @@ -7,6 +7,7 @@ import { FILE_DOC_SEED, } from '@sim/realtime-protocol/file-doc' import { ROOM_TYPES } from '@sim/realtime-protocol/rooms' +import { sleep } from '@sim/utils/helpers' import * as decoding from 'lib0/decoding' import * as encoding from 'lib0/encoding' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' @@ -293,7 +294,7 @@ describe('setupWorkspaceFileDocHandlers', () => { // This join's own authorize saw the pre-revocation state, and the sweep records // the revocation (later read ticket) while it is still in flight. commitRoomPermission('user-stale', room, null, beginRoomPermissionRead()) - await new Promise((resolve) => setTimeout(resolve, 31_000)) + await sleep(31_000) return { allowed: true, status: 200, workspaceId: 'ws-1', workspacePermission: 'write' } }) diff --git a/apps/realtime/src/handlers/workspace-invalidation-room.test.ts b/apps/realtime/src/handlers/workspace-invalidation-room.test.ts index 1dff456a08b..e487edd0a8e 100644 --- a/apps/realtime/src/handlers/workspace-invalidation-room.test.ts +++ b/apps/realtime/src/handlers/workspace-invalidation-room.test.ts @@ -2,6 +2,7 @@ * @vitest-environment node */ import { ROOM_TYPES } from '@sim/realtime-protocol/rooms' +import { sleep } from '@sim/utils/helpers' import { beforeEach, describe, expect, it, vi } from 'vitest' import type { IRoomManager } from '@/rooms' @@ -185,7 +186,7 @@ describe.each([ROOM_TYPES.WORKSPACE_FILES, ROOM_TYPES.WORKSPACE_TABLES] as const 'admin', beginRoomPermissionRead() ) - await new Promise((resolve) => setTimeout(resolve, 31_000)) + await sleep(31_000) } else { // Second call is the re-check's re-resolve: the client leaves during it. handlers[leaveEvent]({ workspaceId: 'ws-sup' }) From 9b52f631412f70104e4f7a07305318d20c6df21c Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 1 Aug 2026 17:07:55 -0700 Subject: [PATCH 10/11] fix(realtime): leave the prior table room only once the join is certain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A table switch left the previous room before the access re-check ran, so a denial there aborted the join and left the client in no table room at all — silently dropped from one it may still be allowed to occupy. The leave now happens after the re-check, matching the file-doc and workspace-list joins. --- apps/realtime/src/handlers/tables.test.ts | 50 +++++++++++++++++++++++ apps/realtime/src/handlers/tables.ts | 23 ++++++----- 2 files changed, 63 insertions(+), 10 deletions(-) diff --git a/apps/realtime/src/handlers/tables.test.ts b/apps/realtime/src/handlers/tables.test.ts index 15b0012ce0e..3df16fd9c65 100644 --- a/apps/realtime/src/handlers/tables.test.ts +++ b/apps/realtime/src/handlers/tables.test.ts @@ -3,6 +3,7 @@ */ import { ROOM_TYPES } from '@sim/realtime-protocol/rooms' import { TABLE_PRESENCE_EVENTS } from '@sim/realtime-protocol/table-presence' +import { sleep } from '@sim/utils/helpers' import { beforeEach, describe, expect, it, vi } from 'vitest' import type { IRoomManager } from '@/rooms' @@ -296,6 +297,55 @@ describe('setupTablesHandlers', () => { } }) + it('keeps the prior table room when a switch is denied at the access re-check', async () => { + // A denied switch must not silently drop the client from a table it may still be + // allowed to occupy, so the prior room is left only once the join is certain. + vi.useFakeTimers() + try { + const prior = { type: ROOM_TYPES.TABLE, id: 'table-prior' } + const { socket, handlers } = createSocket({ id: 'socket-switch', userId: 'user-switch' }) + const roomManager = createRoomManager({ + getRoomForSocket: vi.fn().mockResolvedValue(prior), + }) + setupTablesHandlers(socket as unknown as SetupArg, roomManager) + + let call = 0 + mockAuthorizeRoom.mockImplementation(async () => { + call += 1 + if (call === 1) { + // A later-started read drops this join's own decision, and the join stalls past + // the TTL so that decision is expired by re-check time — forcing the re-resolve + // down its database path below. + commitRoomPermission( + 'user-switch', + { type: ROOM_TYPES.TABLE, id: 'table-target' }, + 'admin', + beginRoomPermissionRead() + ) + await sleep(31_000) + return { allowed: true, status: 200, workspaceId: 'ws-1', workspacePermission: 'admin' } + } + // The authoritative current answer: access is gone. + return { allowed: false, status: 403, workspaceId: 'ws-1', workspacePermission: null } + }) + + const joining = handlers[TABLE_PRESENCE_EVENTS.JOIN]({ tableId: 'table-target' }) + await vi.advanceTimersByTimeAsync(31_000) + await joining + + expect(socket.emit).toHaveBeenCalledWith( + TABLE_PRESENCE_EVENTS.JOIN_ERROR, + expect.objectContaining({ code: 'ACCESS_DENIED', retryable: false }) + ) + // Neither joined the target nor abandoned the prior room. + expect(socket.join).not.toHaveBeenCalled() + expect(socket.leave).not.toHaveBeenCalled() + expect(roomManager.removeUserFromRoom).not.toHaveBeenCalled() + } finally { + vi.useRealTimers() + } + }) + it('drops a malformed cell selection without storing or relaying it', async () => { const { socket, handlers, toEmit } = createSocket() const roomManager = createRoomManager({ diff --git a/apps/realtime/src/handlers/tables.ts b/apps/realtime/src/handlers/tables.ts index 811bdeb5d4d..d6255d49e77 100644 --- a/apps/realtime/src/handlers/tables.ts +++ b/apps/realtime/src/handlers/tables.ts @@ -225,16 +225,6 @@ export function setupTablesHandlers(socket: AuthenticatedSocket, roomManager: IR // Server-authenticated avatar for the presence roster. const avatarUrl = await resolveAvatarUrl(socket, userId) - // Leave a previously-joined table room if switching tables. No generation guard is needed - // around this: serialization guarantees no concurrent op committed to a different room - // during the lookup, so `currentRoom` is the socket's genuine prior room, safe to leave. - const currentRoom = await roomManager.getRoomForSocket(socket.id, ROOM_TYPES.TABLE) - if (currentRoom && currentRoom.id !== tableId) { - socket.leave(roomName(currentRoom)) - await roomManager.removeUserFromRoom(currentRoom, socket.id) - await roomManager.broadcastPresenceUpdate(currentRoom) - } - // Reclaim presence orphaned by an ungraceful disconnect (no `disconnecting` // event fires on a pod crash; the room hashes have no TTL). Returns the roster it // read so the same-tab dedup below reuses it instead of issuing a second read. @@ -273,6 +263,19 @@ export function setupTablesHandlers(socket: AuthenticatedSocket, roomManager: IR return } + // Only now that the join is certain to proceed, leave a previously-joined table room + // if switching. Deliberately AFTER the access re-check: a denial there aborts the + // join, and leaving first would silently drop the client from a prior table it may + // still be allowed to occupy. No generation guard is needed around this — + // serialization guarantees no concurrent op committed to a different room during the + // lookup, so `currentRoom` is the socket's genuine prior room, safe to leave. + const currentRoom = await roomManager.getRoomForSocket(socket.id, ROOM_TYPES.TABLE) + if (currentRoom && currentRoom.id !== tableId) { + socket.leave(roomName(currentRoom)) + await roomManager.removeUserFromRoom(currentRoom, socket.id) + await roomManager.broadcastPresenceUpdate(currentRoom) + } + // Final re-check before the membership commit: a LEAVE or a newer JOIN enqueued during the // awaits above — including the access re-resolve — bumped the generation, or the socket // disconnected. This is the LAST await before registering, so nothing can interleave From 54fdd4f07fce35b5aea3d334ada8361b2fcd3033 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 1 Aug 2026 17:12:47 -0700 Subject: [PATCH 11/11] fix(realtime): close the table join window between re-check and commit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moving the prior-room leave after the access re-check left Redis awaits between that check and socket.join, and superseded() only watches the join generation — so a sweep revocation landing in that window could still put a revoked socket back in the room. A synchronous cache peek immediately before the commit closes it without reintroducing the await; the authoritative resolve moments earlier wrote a fresh entry, so a differing read IS the revocation being guarded. --- apps/realtime/src/handlers/tables.test.ts | 27 +++++++++++++++++++++++ apps/realtime/src/handlers/tables.ts | 26 ++++++++++++++++++++-- 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/apps/realtime/src/handlers/tables.test.ts b/apps/realtime/src/handlers/tables.test.ts index 3df16fd9c65..74310477d93 100644 --- a/apps/realtime/src/handlers/tables.test.ts +++ b/apps/realtime/src/handlers/tables.test.ts @@ -346,6 +346,33 @@ describe('setupTablesHandlers', () => { } }) + it('aborts the join when a revocation lands during the prior-room leave', async () => { + // The prior-room leave is the only await left between the authoritative access + // re-check and the commit, so a sweep revocation recorded in that window must still + // stop the join — `superseded()` alone only watches the join generation. + const prior = { type: ROOM_TYPES.TABLE, id: 'table-prior-2' } + const target = { type: ROOM_TYPES.TABLE, id: 'table-target-2' } + const { socket, handlers } = createSocket({ id: 'socket-window', userId: 'user-window' }) + const roomManager = createRoomManager({ + getRoomForSocket: vi.fn().mockResolvedValue(prior), + removeUserFromRoom: vi.fn().mockImplementation(async () => { + // The sweep records the revocation while the prior-room leave is in flight. + commitRoomPermission('user-window', target, null, beginRoomPermissionRead()) + return true + }), + }) + setupTablesHandlers(socket as unknown as SetupArg, roomManager) + + await handlers[TABLE_PRESENCE_EVENTS.JOIN]({ tableId: 'table-target-2' }) + + expect(socket.emit).toHaveBeenCalledWith( + TABLE_PRESENCE_EVENTS.JOIN_ERROR, + expect.objectContaining({ code: 'ACCESS_DENIED', retryable: false }) + ) + expect(socket.join).not.toHaveBeenCalled() + expect(roomManager.addUserToRoom).not.toHaveBeenCalled() + }) + it('drops a malformed cell selection without storing or relaying it', async () => { const { socket, handlers, toEmit } = createSocket() const roomManager = createRoomManager({ diff --git a/apps/realtime/src/handlers/tables.ts b/apps/realtime/src/handlers/tables.ts index d6255d49e77..6fad57fb6d6 100644 --- a/apps/realtime/src/handlers/tables.ts +++ b/apps/realtime/src/handlers/tables.ts @@ -278,10 +278,32 @@ export function setupTablesHandlers(socket: AuthenticatedSocket, roomManager: IR // Final re-check before the membership commit: a LEAVE or a newer JOIN enqueued during the // awaits above — including the access re-resolve — bumped the generation, or the socket - // disconnected. This is the LAST await before registering, so nothing can interleave - // between it and the commit. + // disconnected. if (superseded()) return + // The prior-room leave above is the one place this handler still awaits AFTER the + // authoritative access re-check (file-doc and the workspace-list rooms leave + // synchronously, so they have no such window). A sweep revocation landing in that + // window would otherwise let this join put a revoked socket back in the room, since + // `superseded()` only watches the join generation. A cache PEEK is the right + // instrument here and needs no await: the authoritative resolve moments ago wrote a + // fresh entry, so the only way this reads differently is a newer decision recorded + // since — exactly the revocation being guarded against. Synchronous, so nothing can + // interleave between it and the join below. + // `undefined` stays "unknown, not denied" here as everywhere else in this handler — + // only a definitively cached insufficient permission aborts a join the authoritative + // check just passed. + const finalCheck = peekRoomPermission(userId, room) + if (finalCheck !== undefined && !satisfiesRoomMembership(finalCheck, ROOM_TYPES.TABLE)) { + socket.emit(TABLE_PRESENCE_EVENTS.JOIN_ERROR, { + tableId, + error: 'Access denied to table', + code: 'ACCESS_DENIED', + retryable: false, + }) + return + } + socket.join(roomName(room)) const presence: UserPresence = {