fix(realtime): enforce room access continuously, not only at join - #6170
Conversation
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
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview The access revalidation sweep now decodes each Socket.IO room name ( Shared policy in A read-ticket ordering on the permission cache ( Reviewed by Cursor Bugbot for commit 54fdd4f. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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.
|
@cursor review |
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.
|
@cursor review |
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.
|
@cursor review |
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.
|
@cursor review |
|
@greptile review |
Greptile SummaryThe PR continuously revalidates authorization for every realtime room type and adds shared join, sweep, and per-frame permission enforcement.
Confidence Score: 4/5The PR is not yet safe to merge because table collaborators can retain stale peer-visible presence when removal succeeds but the subsequent broadcast fails. A reply reports the table cleanup issue as fixed, but the current retry still stores no broadcast-pending state: after successful removal and a failed broadcast, the next attempt sees the cleared mapping, treats the redundant false removal as completion, and never rebroadcasts the updated roster. Files Needing Attention: apps/realtime/src/access-revalidation.ts, apps/realtime/src/handlers/tables.ts
|
| Filename | Overview |
|---|---|
| apps/realtime/src/access-revalidation.ts | Extends authorization sweeps and deferred cleanup to every room type, but the existing broadcast-after-removal retry defect remains. |
| apps/realtime/src/handlers/tables.ts | Adds cached per-frame authorization and retry handoff for table eviction, while the handoff still loses broadcast-only cleanup state. |
| apps/realtime/src/handlers/file-doc.ts | Adds cached write gating, join-time revalidation, and local-state cleanup for revoked document collaborators. |
| apps/realtime/src/middleware/permissions.ts | Generalizes cached permission resolution across room types with ordered commits protecting against stale in-flight results. |
| apps/realtime/src/handlers/room-eviction.ts | Centralizes non-workflow eviction events and handler-local cleanup registration. |
| packages/platform-authz/src/room-policy.ts | Defines shared room membership requirements used consistently by join and continuous enforcement. |
| packages/realtime-protocol/src/events.ts | Adds the generic non-workflow room-access-revoked wire contract. |
| apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/file-doc-provider.ts | Handles fatal document-room revocation by stopping collaboration and falling back to read-only behavior. |
Sequence Diagram
sequenceDiagram
participant Client
participant Handler as Realtime handler
participant Cache as Permission cache
participant Sweep as Revalidation sweep
participant Presence as Room presence
Client->>Handler: Join room
Handler->>Cache: Record authoritative permission
Handler-->>Client: Join accepted
Sweep->>Cache: Refresh room permission
alt Access remains sufficient
Sweep-->>Client: Membership retained
else Access revoked or downgraded
Sweep-->>Client: Revocation event
Sweep->>Handler: Drop room-local binding
Sweep->>Presence: Remove presence
Presence-->>Client: Broadcast updated roster
end
Reviews (6): Last reviewed commit: "fix(realtime): close the table join wind..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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.
…e cache 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.
|
@cursor review |
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.
|
@cursor review |
check:utils bans the inline new Promise(setTimeout) form; the two stalled-join tests were the only new offenders.
|
@cursor review |
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.
|
@cursor review |
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.
|
@cursor review |
There was a problem hiding this comment.
✅ 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 54fdd4f. Configure here.
1 participant
Summary
type:iddecodes to the right workspace) instead of skipping anything that isn't a workflow.ROOM_MEMBERSHIP_ACTIONSin a new dependency-free@sim/platform-authz/room-policy) backs both the join check and the sweep, so a file-doc room keeps requiringwritein both and the two can't drift. That also makes a downgrade (write → read) evict from the editor while keeping the table room.room-access-revokedevent for non-workflow rooms; the file-doc client latches it as fatal and falls back to the read-only view instead of accepting keystrokes that go nowhere. Workflow's existingaccess-revokedpayload is untouched.Eviction stays fail-open on uncertainty: only a definitively-resolved insufficient permission evicts, so a DB blip or a hung query never kicks a live collaborator (the cold-cache fallback is now the room's own required level, not a static
readthat would have evicted every file-doc socket).Type of Change
Testing
tscclean forapps/realtime,apps/sim, and both touched packages; biome clean;check-monorepo-boundariesandcheck-realtime-prune-graphpass.Checklist