feat(agents): canvas routes join the agent-token allowlist (#1800 slice 2)#1825
Conversation
…ce 2) Add _AGENT_CANVAS_ROUTES allowlist and _is_agent_canvas_path helper so registry Bearer tokens reach the canvas element list/create/delete, snapshot PNG/TLDR, and SSE stream endpoints. PATCH elements and PATCH permissions remain session-only. Docs-Reviewed: middleware canvas-route allowlist per design doc lead-agent-identity-and-canvas-access.md
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds an exact allowlist matcher for project canvas routes and extends registry-JWT candidate handling to those routes. Tests cover permitted methods and paths, malformed or unsupported paths, request state, and unauthorized dispatch behavior. ChangesCanvas agent authorization
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthMiddleware
participant DownstreamHandler
Client->>AuthMiddleware: Bearer request to canvas route
AuthMiddleware->>AuthMiddleware: Match method and path allowlist
AuthMiddleware->>DownstreamHandler: Dispatch allowed request
AuthMiddleware-->>Client: Return 401 for denied route
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
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? |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| ("GET", re.compile(rf"^/api/projects/{_SEG}/canvas/elements$")), | ||
| ("POST", re.compile(rf"^/api/projects/{_SEG}/canvas/elements$")), | ||
| ("DELETE", re.compile(rf"^/api/projects/{_SEG}/canvas/elements/{_SEG}$")), | ||
| ("GET", re.compile(rf"^/api/projects/{_SEG}/canvas/snapshot.png$")), |
There was a problem hiding this comment.
SUGGESTION: Unescaped . in the snapshot regex is a wildcard, not a literal dot
In ^/api/projects/{_SEG}/canvas/snapshot.png$ the . matches any single character. Any path such as /api/projects/x/canvas/snapshotXpng (or a future sibling route like snapshot-apng) would also satisfy this allowlist entry. On a security allowlist this is an over-broad match. Escape the literal dot.
| ("GET", re.compile(rf"^/api/projects/{_SEG}/canvas/snapshot.png$")), | |
| ("GET", re.compile(rf"^/api/projects/{_SEG}/canvas/snapshot\.png$")), |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| ("POST", re.compile(rf"^/api/projects/{_SEG}/canvas/elements$")), | ||
| ("DELETE", re.compile(rf"^/api/projects/{_SEG}/canvas/elements/{_SEG}$")), | ||
| ("GET", re.compile(rf"^/api/projects/{_SEG}/canvas/snapshot.png$")), | ||
| ("GET", re.compile(rf"^/api/projects/{_SEG}/canvas/snapshot.tldr$")), |
There was a problem hiding this comment.
SUGGESTION: Unescaped . in the snapshot regex is a wildcard, not a literal dot
Same issue as the snapshot.png entry: .tldr should be \.tldr so only the literal suffix matches. As written, snapshot_tldr or similar could satisfy the pattern.
| ("GET", re.compile(rf"^/api/projects/{_SEG}/canvas/snapshot.tldr$")), | |
| ("GET", re.compile(rf"^/api/projects/{_SEG}/canvas/snapshot\.tldr$")), |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Incremental review (since Security note (informational, unchanged): The middleware still passes any non-local Bearer token through to the canvas routes and relies on each route handler to verify the registry JWT and its Previous Review Summary (commit 5c3d5e2)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 5c3d5e2)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files)
Security note (informational, not a defect): As with the existing Reviewed by hy3:free · Input: 72.2K · Output: 8K · Cached: 125.1K |
Adversarial review caught that snapshot.png and snapshot.tldr used an unescaped dot, so the regex treated it as a wildcard and a near-miss like snapshotXpng would match the agent-token allowlist. Escape both dots so the match is literal, and add regression tests for the near-miss forms plus the single-element GET boundary. Docs-Reviewed: harden canvas allowlist regex anchoring per lead-agent-identity-and-canvas-access.md
|
Folded an adversarial-review finding: the snapshot.png and snapshot.tldr allowlist regexes used an unescaped dot, so the pattern matched a near-miss like snapshotXpng. Escaped both dots for a literal match and added regression tests (snapshotXpng, snapshot_png, snapshotXtldr, and the single-element GET boundary). Note on the reviewer's other flag (agent-token branch sits before the first-boot onboarding gate): that ordering is pre-existing for the task and A2A agent routes, and it is not a bypass because the branch only passes through while the route itself verifies the registry JWT and fails closed, and no valid agent token can exist on an unconfigured system. Not folding it here; it can be a separate defense-in-depth note if we want the gate moved ahead of the pass-through. |
1 participant
slice 2 of #1800 identity epic, DO NOT MERGE (held for full-set review)
Summary by Gitar
_AGENT_CANVAS_ROUTESand_is_agent_canvas_pathto include specific canvas endpoints in the agent-token allowlist.AuthMiddlewareto authorize these routes via bearer tokens, explicitly excluding sensitivePATCHoperations.test_auth_middleware.pyverifying both route matching logic and middleware access control.This will update automatically on new commits.
Summary by CodeRabbit
New Features
Bug Fixes