feat(canvas): agent scope + permission gating and honest attribution (#1800 slice 3)#1827
Conversation
Gate the canvas routes by agent scope and per-project canvas permission
(slice 3 of the lead-agent identity epic), fix element attribution so a
session user is recorded by id and an agent principal by its agent id, and
stamp a uniform {kind, id} actor on every canvas event.
Docs-Reviewed: canvas route gating + attribution + event actor 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}
| author_kind=actor_kind, author_id=actor_id, | ||
| element_id=element_id, | ||
| ) | ||
| except ValueError as e: |
There was a problem hiding this comment.
WARNING: create_canvas_element does not catch CanvasPermissionError
The store's add_element now calls _check_edit_permission (store.py:130), which raises CanvasPermissionError for agents lacking can_edit_canvas. update_canvas_element and delete_canvas_element catch this and return 403, but this create handler only catches ValueError, so an agent that reaches add_element without the edit flag (e.g. if the route gate and store gate ever disagree — the stated purpose of the "defense in depth" check) would surface a 500 instead of a clean 403. Add a dedicated except CanvasPermissionError as e: returning 403, mirroring the update/delete handlers, before the except ValueError.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # Middleware normally 401s unauthenticated requests before the route | ||
| # runs; a middleware-bypassing test context reaches here, so fall back | ||
| # to a system actor (there is no real principal to attribute to). | ||
| return ("user", "system") |
There was a problem hiding this comment.
WARNING: Unauthenticated requests fall back to a privileged system actor
When request.state.user_id is unset and there is no Bearer header, this returns ("user", "system"), which is then treated as an authorized writer. ProjectCanvasStore._check_edit_permission also short-circuits for any author_kind == "user", so the system actor can read, create, update, and delete elements on ANY project's canvas without being a member. The whole gate therefore depends on the auth middleware always setting user_id or 401-ing first; a middleware bypass, misconfiguration, or a route registered without it becomes a privilege-escalation gap. Return 401 (or 403) here rather than authorizing a synthetic system principal.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if exc.status_code == 403 and exc.detail == PROJECT_SCOPE_MISMATCH_DETAIL: | ||
| return JSONResponse({"error": "not found"}, status_code=404) | ||
| raise | ||
| if cid is None: |
There was a problem hiding this comment.
SUGGESTION: Unreachable cid is None branch
check_agent_scope_for_project only returns None when there is no Authorization header, but execution only reaches this point after the auth_header.lower().startswith("bearer ") guard (line 60), so cid can never be None here. This branch is dead code; the except HTTPException above already covers the token cases (including the project-mismatch 403 that is collapsed to 404).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (this increment)
Fix these issues in Kilo Cloud Previous Review Summary (commit e97bdc1)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit e97bdc1)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (9 files)
Reviewed by hy3:free · Input: 81.3K · Output: 9K · Cached: 254.1K |
- Finding 1: add the PATCH /canvas/elements/{eid} route to the agent canvas
allowlist so a canvas_write token can update elements; the PATCH permissions
route stays owner/admin only. Flip the middleware allowlist test accordingly.
- Finding 2: gate the session branch of _authorize_canvas_actor on project
visibility (owner/admin). A non-owner collapses to the same existence-hiding
404 the agent path uses; owner/admin and user attribution are unchanged.
- Finding 4: add a can_read_canvas store gate and enforce it on the in-process
MCP read path (canvas_list_elements) so a write-only agent cannot read the
board, mirroring the write flag gate.
Docs-Reviewed: fold adversarial review findings per lead-agent-identity-and-canvas-access.md
| ps = request.app.state.project_store | ||
| project = await ps.get_project(project_id) | ||
| is_admin = bool(getattr(request.state, "is_admin", False)) | ||
| if project is not None and not is_admin and project.get("user_id") != uid: |
There was a problem hiding this comment.
SUGGESTION: Non-existent projects bypass the new gate and break existence-hiding
The project is not None guard means that when the project does not exist, the condition is False and the session user falls through to return ("user", ...). Downstream routes like list_canvas_elements then return 200 with {"elements": []} instead of 404. So an authenticated non-owner can distinguish "project exists but forbidden" (404) from "project does not exist" (200 empty) — contradicting the comment's claim that a non-owner collapses into the same existence-hiding 404 as the agent path. Collapse non-existent projects into the 404 as well:
| if project is not None and not is_admin and project.get("user_id") != uid: | |
| if project is None or (not is_admin and project.get("user_id") != uid): |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
1 participant
slice 3 of #1800 identity epic, stacked on the base branch, DO NOT MERGE.
Summary by Gitar
canvas_readandcanvas_writeagent scopes and implemented_authorize_canvas_actorto enforce gating across all canvas routes.can_read_canvasandcan_edit_canvasflags toproject_members, requiring these flags for agent actions.set_canvas_permissionto project owners and admins only.ProjectCanvasStoreto prevent bypass by direct callers.kind,id) incanvas.*events and element metadata.This will update automatically on new commits.