fix(realtime): stop read-only members persisting block positions (#6174)

waleedlatif1 · web-flow · commit e5d2f7d80ac0 · 2026-08-01T17:51:10.000-07:00
* fix(realtime): stop read-only members persisting block positions

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.

* test(realtime): assert the role ACL against production, not a fixture

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.
diff --git a/apps/realtime/src/handlers/operations.test.ts 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> | void
+
+function createSocket(id: string) {
+  const handlers: Record<string, Handler> = {}
+  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<typeof setupOperationsHandlers>[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
@@ -7,6 +7,7 @@
  * - Edge cases and invalid inputs
  */
 
+import { ALL_SOCKET_OPERATIONS } from '@sim/realtime-protocol/constants'
 import {
   expectPermissionAllowed,
   expectPermissionDenied,
@@ -116,9 +117,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 +139,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 +160,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 ALL_SOCKET_OPERATIONS) {
         const result = checkRolePermission('read', operation)
         expect(result.allowed).toBe(false)
         expect(result.reason).toContain('read')
@@ -209,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')
     })
   })
 
@@ -244,7 +261,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 +282,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 +354,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
@@ -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<string, string[]> = {
   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)
diff --git a/packages/testing/src/factories/permission.factory.ts b/packages/testing/src/factories/permission.factory.ts
