From 4642715993c4edf67e4a8ae1a929b1373cfde9be Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 1 Aug 2026 17:33:53 -0700 Subject: [PATCH 1/2] fix(realtime): stop read-only members persisting block positions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The socket operation ACL granted the `read` role block.updatePosition and blocks.batchUpdatePositions on the premise that they were ephemeral cursor sync. They are not: both are followed by persistWorkflowOperation, which UPDATEs workflow_blocks.positionX/positionY and bumps workflow.updatedAt. A member holding only `read` on a workspace could therefore permanently rewrite the coordinates of every block of every workflow in it — a write across the read/write boundary. Live cursors ride their own `cursor-update` event, and the smooth-drag broadcast is the UNCOMMITTED position path, which returns before persisting and never consults the role table — so the read role needs no grant at all. --- apps/realtime/src/handlers/operations.test.ts | 181 ++++++++++++++++++ .../src/middleware/permissions.test.ts | 43 +++-- apps/realtime/src/middleware/permissions.ts | 24 ++- 3 files changed, 225 insertions(+), 23 deletions(-) create mode 100644 apps/realtime/src/handlers/operations.test.ts diff --git a/apps/realtime/src/handlers/operations.test.ts b/apps/realtime/src/handlers/operations.test.ts new file mode 100644 index 00000000000..a386a81b677 --- /dev/null +++ b/apps/realtime/src/handlers/operations.test.ts @@ -0,0 +1,181 @@ +/** + * @vitest-environment node + * + * End-to-end guard for the socket operation ACL: the security boundary is not the + * role table on its own but whether a role reaches `persistWorkflowOperation`. + * These tests drive the real handler with the real permission middleware (only the + * database and the workspace authorizer are mocked) and assert on the persist call, + * because that is what durably rewrites `workflow_blocks`. + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { IRoomManager } from '@/rooms' + +const { mockAuthorizeWorkflow, mockPersist, mockAssertMutable } = vi.hoisted(() => ({ + mockAuthorizeWorkflow: vi.fn(), + mockPersist: vi.fn(), + mockAssertMutable: vi.fn(), +})) + +vi.mock('@sim/platform-authz/workflow', () => ({ + authorizeWorkflowByWorkspacePermission: mockAuthorizeWorkflow, + assertWorkflowMutable: mockAssertMutable, + WorkflowLockedError: class WorkflowLockedError extends Error {}, +})) + +vi.mock('@/database/operations', () => ({ + persistWorkflowOperation: mockPersist, +})) + +import { setupOperationsHandlers } from '@/handlers/operations' + +const WORKFLOW_ID = 'wf-acl' +const BLOCK_ID = 'block-1' + +type Handler = (payload: unknown) => Promise | void + +function createSocket(id: string) { + const handlers: Record = {} + const toEmit = vi.fn() + const socket = { + id, + userId: `user-${id}`, + userName: 'Test User', + on: vi.fn((event: string, handler: Handler) => { + handlers[event] = handler + }), + emit: vi.fn(), + to: vi.fn().mockReturnValue({ emit: toEmit }), + } + return { socket, handlers, toEmit } +} + +function createRoomManager(socketId: string, role: string): IRoomManager { + return { + isReady: () => true, + getRoomForSocket: vi.fn().mockResolvedValue({ type: 'workflow', id: WORKFLOW_ID }), + getUserSession: vi + .fn() + .mockResolvedValue({ userId: `user-${socketId}`, userName: 'Test User' }), + hasRoom: vi.fn().mockResolvedValue(true), + getRoomUsers: vi.fn().mockResolvedValue([{ socketId, userId: `user-${socketId}`, role }]), + updateUserActivity: vi.fn().mockResolvedValue(undefined), + updateRoomLastModified: vi.fn().mockResolvedValue(undefined), + } as unknown as IRoomManager +} + +/** A committed single-block move — the form that writes `workflow_blocks`. */ +function committedPositionUpdate() { + return { + operationId: 'op-1', + operation: 'update-position', + target: 'block', + timestamp: Date.now(), + payload: { id: BLOCK_ID, position: { x: 999_999, y: 999_999 }, commit: true }, + } +} + +/** The batch form, which persists with no `commit` flag at all. */ +function batchPositionUpdate() { + return { + operationId: 'op-2', + operation: 'batch-update-positions', + target: 'blocks', + timestamp: Date.now(), + payload: { updates: [{ id: BLOCK_ID, position: { x: 1, y: 2 } }] }, + } +} + +function setup(id: string, role: string) { + const { socket, handlers, toEmit } = createSocket(id) + setupOperationsHandlers( + socket as unknown as Parameters[0], + createRoomManager(id, role) + ) + return { socket, handlers, toEmit } +} + +describe('workflow operation ACL', () => { + beforeEach(() => { + vi.clearAllMocks() + mockAssertMutable.mockResolvedValue(undefined) + mockPersist.mockResolvedValue(undefined) + }) + + describe('read-only member', () => { + beforeEach(() => { + mockAuthorizeWorkflow.mockResolvedValue({ allowed: true, workspacePermission: 'read' }) + }) + + it('cannot persist a committed block position', async () => { + // Unique socket id per test: the permission cache is module-global and keyed + // by (user, workflow), so sharing a user across roles would hit a warm entry. + const { socket, handlers } = setup('sock-read-1', 'read') + + await handlers['workflow-operation'](committedPositionUpdate()) + + expect(mockPersist).not.toHaveBeenCalled() + expect(socket.emit).toHaveBeenCalledWith( + 'operation-forbidden', + expect.objectContaining({ type: 'INSUFFICIENT_PERMISSIONS' }) + ) + }) + + it('cannot persist a batch position update', async () => { + const { socket, handlers } = setup('sock-read-2', 'read') + + await handlers['workflow-operation'](batchPositionUpdate()) + + expect(mockPersist).not.toHaveBeenCalled() + expect(socket.emit).toHaveBeenCalledWith( + 'operation-forbidden', + expect.objectContaining({ type: 'INSUFFICIENT_PERMISSIONS' }) + ) + }) + + it('still relays an UNCOMMITTED position update without persisting it', async () => { + // The smooth-drag broadcast is deliberately ungated (it never persists), and + // tightening the ACL must not turn it into an error. + const { socket, handlers, toEmit } = setup('sock-read-3', 'read') + + await handlers['workflow-operation']({ + ...committedPositionUpdate(), + payload: { id: BLOCK_ID, position: { x: 5, y: 6 }, commit: false }, + }) + + expect(mockPersist).not.toHaveBeenCalled() + expect(toEmit).toHaveBeenCalledWith('workflow-operation', expect.anything()) + expect(socket.emit).not.toHaveBeenCalledWith( + 'operation-forbidden', + expect.objectContaining({ type: 'INSUFFICIENT_PERMISSIONS' }) + ) + }) + }) + + describe('write member (positive control)', () => { + beforeEach(() => { + mockAuthorizeWorkflow.mockResolvedValue({ allowed: true, workspacePermission: 'write' }) + }) + + it('persists a committed block position', async () => { + const { handlers } = setup('sock-write-1', 'write') + + await handlers['workflow-operation'](committedPositionUpdate()) + + expect(mockPersist).toHaveBeenCalledWith( + WORKFLOW_ID, + expect.objectContaining({ operation: 'update-position' }) + ) + }) + + it('persists a batch position update', async () => { + const { handlers } = setup('sock-write-2', 'write') + + await handlers['workflow-operation'](batchPositionUpdate()) + + expect(mockPersist).toHaveBeenCalledWith( + WORKFLOW_ID, + expect.objectContaining({ operation: 'batch-update-positions' }) + ) + }) + }) +}) diff --git a/apps/realtime/src/middleware/permissions.test.ts b/apps/realtime/src/middleware/permissions.test.ts index 130b1a262a2..ec3af1287e3 100644 --- a/apps/realtime/src/middleware/permissions.test.ts +++ b/apps/realtime/src/middleware/permissions.test.ts @@ -116,9 +116,10 @@ describe('checkRolePermission', () => { }) describe('read role', () => { - it('should only allow update-position for read role', () => { + it('should deny update-position for read role (it persists block coordinates)', () => { const result = checkRolePermission('read', 'update-position') - expectPermissionAllowed(result) + expectPermissionDenied(result, 'read') + expectPermissionDenied(result, 'update-position') }) it('should deny batch-add-blocks operation for read role', () => { @@ -137,9 +138,10 @@ describe('checkRolePermission', () => { expectPermissionDenied(result, 'read') }) - it('should allow batch-update-positions operation for read role', () => { + it('should deny batch-update-positions for read role (it persists block coordinates)', () => { const result = checkRolePermission('read', 'batch-update-positions') - expectPermissionAllowed(result) + expectPermissionDenied(result, 'read') + expectPermissionDenied(result, 'batch-update-positions') }) it('should deny replace-state operation for read role', () => { @@ -157,11 +159,11 @@ describe('checkRolePermission', () => { expectPermissionDenied(result, 'read') }) - it('should deny all write operations for read role', () => { - const readAllowedOps = ['update-position', 'batch-update-positions'] - const writeOperations = SOCKET_OPERATIONS.filter((op) => !readAllowedOps.includes(op)) - - for (const operation of writeOperations) { + it('grants the read role NO operation at all', () => { + // Every operation reaching this gate is persisted, so a read-only member must + // hold none of them — including the position updates that used to be granted + // here on the mistaken premise that they were ephemeral cursor sync. + for (const operation of SOCKET_OPERATIONS) { const result = checkRolePermission('read', operation) expect(result.allowed).toBe(false) expect(result.reason).toContain('read') @@ -244,7 +246,7 @@ describe('checkRolePermission', () => { readAllowed: false, }, { operation: 'update', adminAllowed: true, writeAllowed: true, readAllowed: false }, - { operation: 'update-position', adminAllowed: true, writeAllowed: true, readAllowed: true }, + { operation: 'update-position', adminAllowed: true, writeAllowed: true, readAllowed: false }, { operation: 'update-name', adminAllowed: true, writeAllowed: true, readAllowed: false }, { operation: 'toggle-enabled', adminAllowed: true, writeAllowed: true, readAllowed: false }, { operation: 'update-parent', adminAllowed: true, writeAllowed: true, readAllowed: false }, @@ -265,7 +267,7 @@ describe('checkRolePermission', () => { operation: 'batch-update-positions', adminAllowed: true, writeAllowed: true, - readAllowed: true, + readAllowed: false, }, { operation: 'replace-state', adminAllowed: true, writeAllowed: true, readAllowed: false }, ] @@ -337,21 +339,32 @@ describe('checkWorkflowOperationPermission', () => { expect(result.reason).toMatch(/revoked/i) }) - it('denies writes after a downgrade to read but still allows position updates', async () => { + it('denies every persisted operation after a downgrade to read, positions included', async () => { mockAuthorize.mockResolvedValue({ allowed: true, workspacePermission: 'read' }) const denied = await checkWorkflowOperationPermission(userId, workflowId, 'update', 'write') expect(denied.allowed).toBe(false) expect(denied.role).toBe('read') - const allowed = await checkWorkflowOperationPermission( + // A committed position update writes workflow_blocks, so a downgraded member + // loses it too — this used to be allowed and was the escalation path. + const position = await checkWorkflowOperationPermission( userId, workflowId, 'update-position', 'write' ) - expect(allowed.allowed).toBe(true) - expect(allowed.role).toBe('read') + expect(position.allowed).toBe(false) + expect(position.role).toBe('read') + + const batch = await checkWorkflowOperationPermission( + userId, + workflowId, + 'batch-update-positions', + 'write' + ) + expect(batch.allowed).toBe(false) + expect(batch.role).toBe('read') }) it('caches the role within the TTL to avoid a DB read on every operation', async () => { diff --git a/apps/realtime/src/middleware/permissions.ts b/apps/realtime/src/middleware/permissions.ts index 7d88ed4b865..69892a590a6 100644 --- a/apps/realtime/src/middleware/permissions.ts +++ b/apps/realtime/src/middleware/permissions.ts @@ -56,17 +56,25 @@ const WRITE_OPERATIONS: string[] = [ WORKFLOW_OPERATIONS.REPLACE_STATE, ] -// Read role can only update positions (for cursor sync, etc.) -const READ_OPERATIONS: string[] = [ - BLOCK_OPERATIONS.UPDATE_POSITION, - BLOCKS_OPERATIONS.BATCH_UPDATE_POSITIONS, -] - -// Define operation permissions based on role +/** + * Operation permissions by role. + * + * `read` grants NOTHING. Every operation that reaches this gate is durably + * persisted — `handlers/operations.ts` follows an allowed check with + * `persistWorkflowOperation`, which writes `workflow_blocks` / `workflow` rows — + * so granting a read-only member any entry here is a write, not a read. + * + * This previously listed `updatePosition` + `batchUpdatePositions` as readable + * "for cursor sync", which they are not: live cursors ride their own + * `cursor-update` event (`handlers/presence.ts`), and the smooth-drag broadcast + * is the UNCOMMITTED position path, which returns before persisting and never + * consults this table at all. The only thing the grant actually enabled was a + * read-only collaborator permanently rewriting block coordinates. + */ const ROLE_PERMISSIONS: Record = { admin: [...ADMIN_ONLY_OPERATIONS, ...WRITE_OPERATIONS], write: WRITE_OPERATIONS, - read: READ_OPERATIONS, + read: [], } // Check if a role allows a specific operation (no DB query, pure logic) From 0fd8b1f1943866af58cd6f71cce8feb0e50ac056 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 1 Aug 2026 17:43:38 -0700 Subject: [PATCH 2/2] test(realtime): assert the role ACL against production, not a fixture MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shared ROLE_ALLOWED_OPERATIONS fixture still listed the two position operations for the read role, and three tests compared that fixture against itself — so they certified whatever it said, including the grants this PR removes. They now assert checkRolePermission over the protocol's complete operation list (the fixture's copy omits subblock/variable/admin-only ops), plus one test that pins the fixture to the production ACL so the two cannot drift apart again. --- .../src/middleware/permissions.test.ts | 59 ++++++++++++------- .../src/factories/permission.factory.ts | 9 ++- 2 files changed, 45 insertions(+), 23 deletions(-) diff --git a/apps/realtime/src/middleware/permissions.test.ts b/apps/realtime/src/middleware/permissions.test.ts index ec3af1287e3..c109259f390 100644 --- a/apps/realtime/src/middleware/permissions.test.ts +++ b/apps/realtime/src/middleware/permissions.test.ts @@ -7,6 +7,7 @@ * - Edge cases and invalid inputs */ +import { ALL_SOCKET_OPERATIONS } from '@sim/realtime-protocol/constants' import { expectPermissionAllowed, expectPermissionDenied, @@ -163,7 +164,7 @@ describe('checkRolePermission', () => { // Every operation reaching this gate is persisted, so a read-only member must // hold none of them — including the position updates that used to be granted // here on the mistaken premise that they were ephemeral cursor sync. - for (const operation of SOCKET_OPERATIONS) { + for (const operation of ALL_SOCKET_OPERATIONS) { const result = checkRolePermission('read', operation) expect(result.allowed).toBe(false) expect(result.reason).toContain('read') @@ -211,28 +212,42 @@ describe('checkRolePermission', () => { }) describe('permission hierarchy verification', () => { - it('should verify admin has same permissions as write', () => { - const adminOps = ROLE_ALLOWED_OPERATIONS.admin - const writeOps = ROLE_ALLOWED_OPERATIONS.write - - // Admin and write should have same operations - expect(adminOps).toEqual(writeOps) - }) - - it('should verify read is a subset of write permissions', () => { - const readOps = ROLE_ALLOWED_OPERATIONS.read - const writeOps = ROLE_ALLOWED_OPERATIONS.write - - for (const op of readOps) { - expect(writeOps).toContain(op) + // These assert the PRODUCTION ACL over the protocol's complete operation list. + // They used to compare the shared test fixture against itself, which certified + // whatever the fixture said — including, for a while, the read-role grants that + // let a read-only member persist block positions. + + it('grants admin everything write has, plus the admin-only operations', () => { + for (const operation of ALL_SOCKET_OPERATIONS) { + if (checkRolePermission('write', operation).allowed) { + expect(checkRolePermission('admin', operation).allowed).toBe(true) + } + } + // Strictly greater: at least one operation admin holds and write does not. + const adminOnly = ALL_SOCKET_OPERATIONS.filter( + (operation) => + checkRolePermission('admin', operation).allowed && + !checkRolePermission('write', operation).allowed + ) + expect(adminOnly.length).toBeGreaterThan(0) + }) + + it('grants read nothing, so it is trivially a subset of write', () => { + const readAllowed = ALL_SOCKET_OPERATIONS.filter( + (operation) => checkRolePermission('read', operation).allowed + ) + expect(readAllowed).toEqual([]) + }) + + it('keeps the shared fixture in step with the production ACL', () => { + // The fixture is a convenience mirror; drift between it and the real table is + // what made the stale read grants look intentional. + for (const operation of ALL_SOCKET_OPERATIONS) { + const fixtureAllows = ROLE_ALLOWED_OPERATIONS.read.includes( + operation as (typeof ROLE_ALLOWED_OPERATIONS.read)[number] + ) + expect(fixtureAllows).toBe(checkRolePermission('read', operation).allowed) } - }) - - it('should verify read has minimal permissions', () => { - const readOps = ROLE_ALLOWED_OPERATIONS.read - expect(readOps).toHaveLength(2) - expect(readOps).toContain('update-position') - expect(readOps).toContain('batch-update-positions') }) }) diff --git a/packages/testing/src/factories/permission.factory.ts b/packages/testing/src/factories/permission.factory.ts index 78de7f85f8a..0ea5ea8c397 100644 --- a/packages/testing/src/factories/permission.factory.ts +++ b/packages/testing/src/factories/permission.factory.ts @@ -307,11 +307,18 @@ export type SocketOperation = (typeof SOCKET_OPERATIONS)[number] /** * Operations allowed for each role. + * + * A convenience mirror for fixtures — NOT the authority. The real ACL lives in + * `apps/realtime/src/middleware/permissions.ts`; assert against + * `checkRolePermission` rather than this table, or a drift between the two turns + * into a test that certifies whatever the fixture happens to say. (`read` listed + * the two position operations here while production had already granted them for + * real; both are persisted writes and neither role should hold them.) */ export const ROLE_ALLOWED_OPERATIONS: Record = { admin: SOCKET_OPERATIONS, write: SOCKET_OPERATIONS, - read: [BLOCK_OPERATIONS.UPDATE_POSITION, BLOCKS_OPERATIONS.BATCH_UPDATE_POSITIONS], + read: [], } /**