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

[waleedlatif1]

Summary

• The socket operation ACL granted the read role block.updatePosition and blocks.batchUpdatePositions, on the premise (stated in the comment) that they were ephemeral cursor sync. They are not — both are followed by persistWorkflowOperation, which UPDATEs workflow_blocks.positionX/positionY and bumps workflow.updatedAt.
• So a member holding only read on a workspace could permanently rewrite the on-canvas coordinates of every block of every workflow in it: a write across the read/write boundary, with no UI affordance to undo beyond manual re-layout. The batch form does it for every block in one call and does not even require the commit flag.
• The premise was wrong twice over: 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 the role table at all. So read needs no grant — it is now [].

Client-side this changes nothing for legitimate users: nodesDraggable and onNodeDrag/onNodeDragStop are already gated on canEdit, so a read-only member never emits these through the UI. Reaching the hole required a hand-crafted socket frame.

Type of Change

• Bug fix (security — privilege escalation / integrity)

Testing

• New handlers/operations.test.ts drives the real handler and the real permission middleware (only the DB and the workspace authorizer are mocked) and asserts on persistWorkflowOperation, since that is what actually writes the row: a read member is refused for both the committed single and the batch form, and a write member still persists both (positive control).
• A third case pins the no-regression edge: an uncommitted position update from a read member still relays and still does not persist, so tightening the ACL did not turn the ungated smooth-drag broadcast into an error.
• Existing permissions.test.ts cases that encoded the vulnerable behaviour ("should allow batch-update-positions operation for read role") are inverted, and the blanket case now asserts the read role holds no operation at all.
• Verified the suite actually catches this: restoring the two grants fails 8 tests, including the two that assert the database write is blocked.
• 273 tests pass; tsc, lint, and all 12 CI audit scripts green locally.

Notes for review

Considered and deliberately not done: gating persistWorkflowOperation itself on permissionSatisfies(role, 'write') as belt-and-braces. Every persist call in operations.ts is already downstream of the single permission gate (the only bypass, the uncommitted position path, returns before persisting), and subblocks.ts / variables.ts carry their own gates — so the runtime check would be pure redundancy, at the cost of an extra failure mode that surfaces to clients as a retryable persistence error. The invariant is instead pinned by the test asserting read grants nothing.

Residual, out of scope and lower severity: a read-only member can still broadcast uncommitted position frames (phantom moves other collaborators see, which nothing persists and a refresh clears). That path is intentionally ungated for drag latency. Happy to close it in a follow-up if you'd like — it needs a synchronous check on the hot path rather than the existing async one.

Checklist

• Code follows project style guidelines
• Self-reviewed my changes
• Tests added/updated and passing
• No new warnings introduced
• I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
docs	Skipped		Aug 2, 2026 12:43am

[cursor]

PR Summary

High Risk
Security fix on realtime authorization: incorrect ACL allowed read-only users to persist workflow block positions; the change is narrow but touches the mutating-operation gate for all collaborators.

Overview
Closes a privilege escalation where workspace read members could durably change block layout: the socket ACL treated update-position and batch-update-positions as read-safe “cursor sync,” but committed paths call persistWorkflowOperation and update workflow_blocks.

Production fix: read role permissions in permissions.ts are now empty (position ops remain on write/admin). The shared test fixture ROLE_ALLOWED_OPERATIONS.read is aligned so tests don’t mirror stale grants.

Tests: New handlers/operations.test.ts exercises the real handler + permission middleware and asserts read users never hit persistWorkflowOperation for committed/batch moves while write users still do; uncommitted position relays stay allowed without persist. permissions.test.ts inverts read allowances, asserts read has no operations over ALL_SOCKET_OPERATIONS, and checks fixture vs production ACL.

^{Reviewed by Cursor Bugbot for commit 0fd8b1f. Configure here.}

[greptile-apps]

Greptile Summary

The PR closes a realtime authorization gap by preventing read-only members from persisting block-position operations while preserving ephemeral drag broadcasts.

• Removes all persisted socket-operation grants from the read role.
• Updates the shared permission fixture to match the production read-role ACL.
• Adds handler-level coverage for denied read writes and permitted write operations.
• Strengthens permission tests against the canonical protocol operation list.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains; the previously reported stale read-role fixture now matches the production ACL and is covered by an exhaustive read-role synchronization assertion.

Important Files Changed

Filename	Overview
apps/realtime/src/handlers/operations.test.ts	Adds integration-style handler tests proving read-only position writes are rejected, ephemeral updates remain relayed, and write-role persistence still succeeds.
apps/realtime/src/middleware/permissions.test.ts	Updates read-role expectations and verifies the shared read fixture against every canonical socket operation.
apps/realtime/src/middleware/permissions.ts	Removes the persisted position-operation grants from the read role, restoring the intended read/write boundary.
packages/testing/src/factories/permission.factory.ts	Corrects the shared read-role fixture to grant no socket operations, fully addressing the previously reported stale fixture.

Sequence Diagram

sequenceDiagram
  participant Client
  participant Handler as Realtime operation handler
  participant ACL as Permission middleware
  participant DB as Workflow persistence
  Client->>Handler: Position operation
  alt Uncommitted drag update
    Handler-->>Client: Relay ephemeral update
  else Committed or batch update
    Handler->>ACL: Check current workspace role
    alt read role
      ACL-->>Handler: Denied
      Handler-->>Client: operation-forbidden
    else write or admin role
      ACL-->>Handler: Allowed
      Handler->>DB: Persist block positions
    end
  end

Loading

_{Reviews (2): Last reviewed commit: "test(realtime): assert the role ACL agai..." | Re-trigger Greptile}

[waleedlatif1]

Re: the stale shared test fixture (4/5)

Fixed in 0fd8b1f — good catch, and the underlying problem was worse than a stale value.

packages/testing's ROLE_ALLOWED_OPERATIONS still listed update-position + batch-update-positions for read, and three tests in permissions.test.ts compared that fixture against itself:

it('should verify read has minimal permissions', () => {
  const readOps = ROLE_ALLOWED_OPERATIONS.read
  expect(readOps).toHaveLength(2)          // asserts the fixture, not the ACL
  expect(readOps).toContain('update-position')
})

That reads like a security assertion but certifies whatever the fixture says — it would have passed unchanged no matter what production granted, which is part of why the grants looked intentional.

Changes:

• Fixture read: [], with a note that it is a convenience mirror and not the authority.
• The three tautologies now assert checkRolePermission (the real ACL) over ALL_SOCKET_OPERATIONS from @sim/realtime-protocol/constants. That also fixes a second staleness I hit on the way: the fixture's own operation list omits the subblock, variable and admin-only operations, so the previous "deny all for read" sweep never covered them. The authoritative list does.
• One new test pins the fixture to the production ACL, so the two cannot silently drift apart again.

Verified in both directions rather than just re-running green: reverting the production ACL fails 8 tests, and reverting only the fixture (the exact drift you flagged) fails the new pin test.

Remaining duplication worth a follow-up, deliberately not done here to keep a security fix reviewable: packages/testing hand-maintains its own copy of the operation list instead of depending on @sim/realtime-protocol. Deduping that would need a new package dependency edge, so I'd rather not land it inside this PR.

[waleedlatif1]

@greptile

[waleedlatif1]

@cursor review

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 0fd8b1f. Configure here.}