Skip to content

Commit 4642715

Browse files
committed
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.
1 parent 5686b7b commit 4642715

3 files changed

Lines changed: 225 additions & 23 deletions

File tree

Lines changed: 181 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,181 @@
1+
/**
2+
* @vitest-environment node
3+
*
4+
* End-to-end guard for the socket operation ACL: the security boundary is not the
5+
* role table on its own but whether a role reaches `persistWorkflowOperation`.
6+
* These tests drive the real handler with the real permission middleware (only the
7+
* database and the workspace authorizer are mocked) and assert on the persist call,
8+
* because that is what durably rewrites `workflow_blocks`.
9+
*/
10+
import { beforeEach, describe, expect, it, vi } from 'vitest'
11+
import type { IRoomManager } from '@/rooms'
12+
13+
const { mockAuthorizeWorkflow, mockPersist, mockAssertMutable } = vi.hoisted(() => ({
14+
mockAuthorizeWorkflow: vi.fn(),
15+
mockPersist: vi.fn(),
16+
mockAssertMutable: vi.fn(),
17+
}))
18+
19+
vi.mock('@sim/platform-authz/workflow', () => ({
20+
authorizeWorkflowByWorkspacePermission: mockAuthorizeWorkflow,
21+
assertWorkflowMutable: mockAssertMutable,
22+
WorkflowLockedError: class WorkflowLockedError extends Error {},
23+
}))
24+
25+
vi.mock('@/database/operations', () => ({
26+
persistWorkflowOperation: mockPersist,
27+
}))
28+
29+
import { setupOperationsHandlers } from '@/handlers/operations'
30+
31+
const WORKFLOW_ID = 'wf-acl'
32+
const BLOCK_ID = 'block-1'
33+
34+
type Handler = (payload: unknown) => Promise<void> | void
35+
36+
function createSocket(id: string) {
37+
const handlers: Record<string, Handler> = {}
38+
const toEmit = vi.fn()
39+
const socket = {
40+
id,
41+
userId: `user-${id}`,
42+
userName: 'Test User',
43+
on: vi.fn((event: string, handler: Handler) => {
44+
handlers[event] = handler
45+
}),
46+
emit: vi.fn(),
47+
to: vi.fn().mockReturnValue({ emit: toEmit }),
48+
}
49+
return { socket, handlers, toEmit }
50+
}
51+
52+
function createRoomManager(socketId: string, role: string): IRoomManager {
53+
return {
54+
isReady: () => true,
55+
getRoomForSocket: vi.fn().mockResolvedValue({ type: 'workflow', id: WORKFLOW_ID }),
56+
getUserSession: vi
57+
.fn()
58+
.mockResolvedValue({ userId: `user-${socketId}`, userName: 'Test User' }),
59+
hasRoom: vi.fn().mockResolvedValue(true),
60+
getRoomUsers: vi.fn().mockResolvedValue([{ socketId, userId: `user-${socketId}`, role }]),
61+
updateUserActivity: vi.fn().mockResolvedValue(undefined),
62+
updateRoomLastModified: vi.fn().mockResolvedValue(undefined),
63+
} as unknown as IRoomManager
64+
}
65+
66+
/** A committed single-block move — the form that writes `workflow_blocks`. */
67+
function committedPositionUpdate() {
68+
return {
69+
operationId: 'op-1',
70+
operation: 'update-position',
71+
target: 'block',
72+
timestamp: Date.now(),
73+
payload: { id: BLOCK_ID, position: { x: 999_999, y: 999_999 }, commit: true },
74+
}
75+
}
76+
77+
/** The batch form, which persists with no `commit` flag at all. */
78+
function batchPositionUpdate() {
79+
return {
80+
operationId: 'op-2',
81+
operation: 'batch-update-positions',
82+
target: 'blocks',
83+
timestamp: Date.now(),
84+
payload: { updates: [{ id: BLOCK_ID, position: { x: 1, y: 2 } }] },
85+
}
86+
}
87+
88+
function setup(id: string, role: string) {
89+
const { socket, handlers, toEmit } = createSocket(id)
90+
setupOperationsHandlers(
91+
socket as unknown as Parameters<typeof setupOperationsHandlers>[0],
92+
createRoomManager(id, role)
93+
)
94+
return { socket, handlers, toEmit }
95+
}
96+
97+
describe('workflow operation ACL', () => {
98+
beforeEach(() => {
99+
vi.clearAllMocks()
100+
mockAssertMutable.mockResolvedValue(undefined)
101+
mockPersist.mockResolvedValue(undefined)
102+
})
103+
104+
describe('read-only member', () => {
105+
beforeEach(() => {
106+
mockAuthorizeWorkflow.mockResolvedValue({ allowed: true, workspacePermission: 'read' })
107+
})
108+
109+
it('cannot persist a committed block position', async () => {
110+
// Unique socket id per test: the permission cache is module-global and keyed
111+
// by (user, workflow), so sharing a user across roles would hit a warm entry.
112+
const { socket, handlers } = setup('sock-read-1', 'read')
113+
114+
await handlers['workflow-operation'](committedPositionUpdate())
115+
116+
expect(mockPersist).not.toHaveBeenCalled()
117+
expect(socket.emit).toHaveBeenCalledWith(
118+
'operation-forbidden',
119+
expect.objectContaining({ type: 'INSUFFICIENT_PERMISSIONS' })
120+
)
121+
})
122+
123+
it('cannot persist a batch position update', async () => {
124+
const { socket, handlers } = setup('sock-read-2', 'read')
125+
126+
await handlers['workflow-operation'](batchPositionUpdate())
127+
128+
expect(mockPersist).not.toHaveBeenCalled()
129+
expect(socket.emit).toHaveBeenCalledWith(
130+
'operation-forbidden',
131+
expect.objectContaining({ type: 'INSUFFICIENT_PERMISSIONS' })
132+
)
133+
})
134+
135+
it('still relays an UNCOMMITTED position update without persisting it', async () => {
136+
// The smooth-drag broadcast is deliberately ungated (it never persists), and
137+
// tightening the ACL must not turn it into an error.
138+
const { socket, handlers, toEmit } = setup('sock-read-3', 'read')
139+
140+
await handlers['workflow-operation']({
141+
...committedPositionUpdate(),
142+
payload: { id: BLOCK_ID, position: { x: 5, y: 6 }, commit: false },
143+
})
144+
145+
expect(mockPersist).not.toHaveBeenCalled()
146+
expect(toEmit).toHaveBeenCalledWith('workflow-operation', expect.anything())
147+
expect(socket.emit).not.toHaveBeenCalledWith(
148+
'operation-forbidden',
149+
expect.objectContaining({ type: 'INSUFFICIENT_PERMISSIONS' })
150+
)
151+
})
152+
})
153+
154+
describe('write member (positive control)', () => {
155+
beforeEach(() => {
156+
mockAuthorizeWorkflow.mockResolvedValue({ allowed: true, workspacePermission: 'write' })
157+
})
158+
159+
it('persists a committed block position', async () => {
160+
const { handlers } = setup('sock-write-1', 'write')
161+
162+
await handlers['workflow-operation'](committedPositionUpdate())
163+
164+
expect(mockPersist).toHaveBeenCalledWith(
165+
WORKFLOW_ID,
166+
expect.objectContaining({ operation: 'update-position' })
167+
)
168+
})
169+
170+
it('persists a batch position update', async () => {
171+
const { handlers } = setup('sock-write-2', 'write')
172+
173+
await handlers['workflow-operation'](batchPositionUpdate())
174+
175+
expect(mockPersist).toHaveBeenCalledWith(
176+
WORKFLOW_ID,
177+
expect.objectContaining({ operation: 'batch-update-positions' })
178+
)
179+
})
180+
})
181+
})

apps/realtime/src/middleware/permissions.test.ts

Lines changed: 28 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -116,9 +116,10 @@ describe('checkRolePermission', () => {
116116
})
117117

118118
describe('read role', () => {
119-
it('should only allow update-position for read role', () => {
119+
it('should deny update-position for read role (it persists block coordinates)', () => {
120120
const result = checkRolePermission('read', 'update-position')
121-
expectPermissionAllowed(result)
121+
expectPermissionDenied(result, 'read')
122+
expectPermissionDenied(result, 'update-position')
122123
})
123124

124125
it('should deny batch-add-blocks operation for read role', () => {
@@ -137,9 +138,10 @@ describe('checkRolePermission', () => {
137138
expectPermissionDenied(result, 'read')
138139
})
139140

140-
it('should allow batch-update-positions operation for read role', () => {
141+
it('should deny batch-update-positions for read role (it persists block coordinates)', () => {
141142
const result = checkRolePermission('read', 'batch-update-positions')
142-
expectPermissionAllowed(result)
143+
expectPermissionDenied(result, 'read')
144+
expectPermissionDenied(result, 'batch-update-positions')
143145
})
144146

145147
it('should deny replace-state operation for read role', () => {
@@ -157,11 +159,11 @@ describe('checkRolePermission', () => {
157159
expectPermissionDenied(result, 'read')
158160
})
159161

160-
it('should deny all write operations for read role', () => {
161-
const readAllowedOps = ['update-position', 'batch-update-positions']
162-
const writeOperations = SOCKET_OPERATIONS.filter((op) => !readAllowedOps.includes(op))
163-
164-
for (const operation of writeOperations) {
162+
it('grants the read role NO operation at all', () => {
163+
// Every operation reaching this gate is persisted, so a read-only member must
164+
// hold none of them — including the position updates that used to be granted
165+
// here on the mistaken premise that they were ephemeral cursor sync.
166+
for (const operation of SOCKET_OPERATIONS) {
165167
const result = checkRolePermission('read', operation)
166168
expect(result.allowed).toBe(false)
167169
expect(result.reason).toContain('read')
@@ -244,7 +246,7 @@ describe('checkRolePermission', () => {
244246
readAllowed: false,
245247
},
246248
{ operation: 'update', adminAllowed: true, writeAllowed: true, readAllowed: false },
247-
{ operation: 'update-position', adminAllowed: true, writeAllowed: true, readAllowed: true },
249+
{ operation: 'update-position', adminAllowed: true, writeAllowed: true, readAllowed: false },
248250
{ operation: 'update-name', adminAllowed: true, writeAllowed: true, readAllowed: false },
249251
{ operation: 'toggle-enabled', adminAllowed: true, writeAllowed: true, readAllowed: false },
250252
{ operation: 'update-parent', adminAllowed: true, writeAllowed: true, readAllowed: false },
@@ -265,7 +267,7 @@ describe('checkRolePermission', () => {
265267
operation: 'batch-update-positions',
266268
adminAllowed: true,
267269
writeAllowed: true,
268-
readAllowed: true,
270+
readAllowed: false,
269271
},
270272
{ operation: 'replace-state', adminAllowed: true, writeAllowed: true, readAllowed: false },
271273
]
@@ -337,21 +339,32 @@ describe('checkWorkflowOperationPermission', () => {
337339
expect(result.reason).toMatch(/revoked/i)
338340
})
339341

340-
it('denies writes after a downgrade to read but still allows position updates', async () => {
342+
it('denies every persisted operation after a downgrade to read, positions included', async () => {
341343
mockAuthorize.mockResolvedValue({ allowed: true, workspacePermission: 'read' })
342344

343345
const denied = await checkWorkflowOperationPermission(userId, workflowId, 'update', 'write')
344346
expect(denied.allowed).toBe(false)
345347
expect(denied.role).toBe('read')
346348

347-
const allowed = await checkWorkflowOperationPermission(
349+
// A committed position update writes workflow_blocks, so a downgraded member
350+
// loses it too — this used to be allowed and was the escalation path.
351+
const position = await checkWorkflowOperationPermission(
348352
userId,
349353
workflowId,
350354
'update-position',
351355
'write'
352356
)
353-
expect(allowed.allowed).toBe(true)
354-
expect(allowed.role).toBe('read')
357+
expect(position.allowed).toBe(false)
358+
expect(position.role).toBe('read')
359+
360+
const batch = await checkWorkflowOperationPermission(
361+
userId,
362+
workflowId,
363+
'batch-update-positions',
364+
'write'
365+
)
366+
expect(batch.allowed).toBe(false)
367+
expect(batch.role).toBe('read')
355368
})
356369

357370
it('caches the role within the TTL to avoid a DB read on every operation', async () => {

apps/realtime/src/middleware/permissions.ts

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -56,17 +56,25 @@ const WRITE_OPERATIONS: string[] = [
5656
WORKFLOW_OPERATIONS.REPLACE_STATE,
5757
]
5858

59-
// Read role can only update positions (for cursor sync, etc.)
60-
const READ_OPERATIONS: string[] = [
61-
BLOCK_OPERATIONS.UPDATE_POSITION,
62-
BLOCKS_OPERATIONS.BATCH_UPDATE_POSITIONS,
63-
]
64-
65-
// Define operation permissions based on role
59+
/**
60+
* Operation permissions by role.
61+
*
62+
* `read` grants NOTHING. Every operation that reaches this gate is durably
63+
* persisted — `handlers/operations.ts` follows an allowed check with
64+
* `persistWorkflowOperation`, which writes `workflow_blocks` / `workflow` rows —
65+
* so granting a read-only member any entry here is a write, not a read.
66+
*
67+
* This previously listed `updatePosition` + `batchUpdatePositions` as readable
68+
* "for cursor sync", which they are not: live cursors ride their own
69+
* `cursor-update` event (`handlers/presence.ts`), and the smooth-drag broadcast
70+
* is the UNCOMMITTED position path, which returns before persisting and never
71+
* consults this table at all. The only thing the grant actually enabled was a
72+
* read-only collaborator permanently rewriting block coordinates.
73+
*/
6674
const ROLE_PERMISSIONS: Record<string, string[]> = {
6775
admin: [...ADMIN_ONLY_OPERATIONS, ...WRITE_OPERATIONS],
6876
write: WRITE_OPERATIONS,
69-
read: READ_OPERATIONS,
77+
read: [],
7078
}
7179

7280
// Check if a role allows a specific operation (no DB query, pure logic)

0 commit comments

Comments
 (0)