feat(canvas): gate stream + snapshot + keepalive recheck (#1800 slice 4)#1830
feat(canvas): gate stream + snapshot + keepalive recheck (#1800 slice 4)#1830jaylfc wants to merge 1 commit into
Conversation
Require canvas_read (scope + can_read_canvas member flag) for agent principals on GET /canvas/stream, /canvas/snapshot.png and /canvas/snapshot.tldr, reusing _authorize_canvas_actor in read mode. Snapshots stay read-scope only: a canvas_write-only token is 403. On the SSE keepalive tick, re-verify an agent principal's can_read_canvas flag and close the stream if it was cleared, so a revoked agent cannot hold a long-lived stream open. Tests: SSE connect as agent with and without scope+flag, snapshot fetch as agent (read allowed, write-only 403), stream closes after the read flag is cleared; session owner/admin behavior unchanged. Docs-Reviewed: stream + snapshot gating with keepalive recheck per lead-agent-identity-and-canvas-access.md
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| # SSE open (slice 4). Session users are unaffected. | ||
| if actor_kind == "agent": | ||
| member = await ps.get_member(project_id, actor_id) | ||
| if not (member or {}).get("can_read_canvas"): |
There was a problem hiding this comment.
WARNING: Flag recheck is bypassed while canvas events keep flowing, so a revoked agent can hold a long-lived stream open on a busy canvas
The can_read_canvas recheck lives inside the except asyncio.TimeoutError branch, which only runs when no event arrives for 10s. On an actively-updated canvas, queue.get() keeps returning canvas.* events, this branch never executes, and a revoked/removed agent keeps receiving real event frames indefinitely — which contradicts the stated goal that "a revoked agent cannot hold a long-lived stream open."
Consider re-verifying the flag before yielding event data (or on a time-based cadence that runs every iteration regardless of idle), and/or unsubscribing the queue when the flag is cleared. As written, only an idle revoked stream is actually closed.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| closed = False | ||
| try: | ||
| while True: | ||
| await asyncio.wait_for(gen.__anext__(), timeout=12) |
There was a problem hiding this comment.
SUGGESTION: Strengthen revocation test to cover a busy canvas
This test publishes a single canvas.permission_changed event and then asserts the generator closes within 12s, which only exercises the idle keepalive recheck. It does not assert that no further canvas.* event frames are delivered after revocation, and it would still pass on a canvas that emits events continuously (the exact scenario the production WARNING above describes), because the loop simply consumes frames until the timeout-based close.
Consider publishing additional canvas.* events after _grant_canvas(..., read=False) and asserting that no event frames are received post-revocation (only the final StopAsyncIteration), so the test would catch the busy-canvas bypass.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Reviewed by hy3:free · Input: 76K · Output: 9.6K · Cached: 168K |
|
Superseded by #1838, which landed all seven slices as one consistent set on dev (with the CI-caught active-handle migration-order fix folded). Closing. |
1 participant
Slice 4 of the lead-agent identity epic (taOS project board task tsk-lid4).
principals on GET /canvas/stream, /canvas/snapshot.png and
/canvas/snapshot.tldr. Reuses _authorize_canvas_actor in read mode.
can_read_canvas flag and close the stream if it was cleared, so a
revoked agent cannot hold a long-lived stream open.
DO NOT MERGE
Docs-Reviewed: stream + snapshot gating with keepalive recheck per
lead-agent-identity-and-canvas-access.md