feat(agent-server): Canvas Extensions REST API surface - #4395
Conversation
Adds canvas_extensions_router.py mirroring plugins_router.py / skills_router.py: install / list / get / enable-disable / uninstall, plus a bundle endpoint that re-validates entrypoint containment at serve time and busts caches across staged-refresh revisions.
…e schema
GET /canvas-extensions/installed/{name}/bundle returns a FileResponse,
which FastAPI can't derive a real OpenAPI schema for -- same known
weak-type-ratchet exception already granted to every other FileResponse
endpoint (file/download, file/archive, workspace file serving, etc.).
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR adds the Canvas Extensions REST API surface (canvas_extensions_router.py) — install/list/get/enable-disable/uninstall plus a bundle-serving endpoint — mirroring the existing plugins_router.py / skills_router.py conventions. The implementation is clean, well-tested (21 tests covering happy paths, all error mappings, path-injection rejection, symlink-escape-after-install, entrypoint-deletion, cache-busting across revisions), and follows the codebase's established patterns consistently.
Findings
No material correctness or security issues found.
Risk assessment: LOW.
The security-critical bundle endpoint is sound:
get_canvas_extension_bundle_pathre-validates the manifest and entrypoint containment on every request (not just at install time), andresolve_entrypointresolves symlinks viaPath.resolve()before checkingis_relative_to(root)+is_file(). A post-install symlink swap pointing outside the package root is correctly rejected.- The resolved (dereferenced) path is passed to
FileResponse, which mitigates the TOCTOU window between validation and file read —FileResponseopens the already-resolved real path, not the symlink, so a between-request symlink swap cannot redirect the read. - Path-parameter injection is guarded at the router level by the kebab-case
CANVAS_EXTENSION_NAME_PATTERNbefore any service-layer code runs (defense-in-depth on top ofvalidate_extension_name). - The install endpoint's broadened
except (ValidationError, ValueError, OSError)-> 422 mapping fixes a genuine bug (missing-manifestFileNotFoundErrorpreviously fell through to an unhandled 500 leaking a filesystem path), an improvement over theplugins_routerprecedent which only catchesValueError.
Non-blocking observation
get_canvas_extension_bundle_path calls CanvasExtensionInstallationInterface.load_from_dir(), which itself calls resolve_entrypoint() internally (manifest.py line 59), and then calls resolve_entrypoint() again on the returned manifest (installed.py line 230). The second call is redundant — load_from_dir already validates containment. Harmless (arguably defensive), but removing it would avoid a duplicate filesystem resolve()/stat() round-trip per bundle request. Not blocking.
HUMAN:
Adding REST API to menage extensions.
AGENT:
Why
Issue #4352 asks for the REST API surface for Canvas Extensions — the install / list / get / enable-disable / uninstall / bundle endpoints on top of the already-merged installation-persistence and staged-refresh service layer (#4350, #4351). The issue explicitly calls out
plugins_router.py/skills_router.pyas the precedent to mirror (thin router delegating to a service layer, kebab-case name-pattern path guards,Installed*Responsemodel shape, HTTPException mapping for 400/404/409/422) rather than inventing new conventions.Summary
canvas_extensions_router.py:POST /install,GET /installed,GET /installed/{name},PATCH /installed/{name},DELETE /installed/{name},GET /installed/{name}/bundle— wired intoapi.pyalongsideplugins_router/skills_router.get_canvas_extension_bundle_path()tocanvas_extensions/installed.py: re-validates the manifest and entrypoint containment against the live install path on every bundle request, not just at install time (a symlink could change between requests). The bundle response relies on Starlette's own file-stat-basedETag/Last-ModifiedplusCache-Control: no-cacheso a client never keeps serving pre-refresh content after a staged-refresh revision lands.tests/agent_server/test_canvas_extensions_router.py(21 tests): install/get/patch/delete happy paths, 400/409/422/404 error mapping (including a missing-manifestFileNotFoundErrorthat previously fell through to an unhandled 500 — fixed), disabled-by-default + smuggled-enabled-field-ignored + force-reinstall-preserves-state through the HTTP layer, path-parameter injection rejection, symlink-escape-after-install, entrypoint-deleted, and cache-busting across a force-reinstalled revision.Issue Number
Closes #4352
Linear: OSS-9021 (https://linear.app/all-hands-ai/issue/OSS-9021/canvas-extensions-rest-api-surface)
How to Test
uv run pytest tests/agent_server/test_canvas_extensions_router.py -v→ 21 passed.Full regression:
uv run pytest tests/agent_server/ -q→ 1988 passed, 13 deselected, 0 failed.uv run ruff check,uv run ruff format --check,uv run pyrighton all changed files → clean.End-to-end smoke test against a real
FastAPIapp +TestClient(not just pytest), exercising the full lifecycle with real filesystem I/O (temp install store, a real manifest + JS entrypoint on disk):Confirms: fresh install lands disabled, PATCH flips it, the bundle is served with correct auto-detected content-type and the
no-cacheheader, and uninstall actually removes the tracked entry (404 afterward).Verified route registration against the real
create_app()(not a bare router mount): all 6 endpoints resolve under/api/canvas-extensions/*.Independently reproduced (before/after) the missing-manifest bug this PR fixes: a source directory with no
canvas-extension.jsonpreviously raised an uncaughtFileNotFoundError, surfacing as a raw 500 with an internal filesystem path leaked into the response body; now returns a clean422.Video/Screenshots
Not applicable — backend-only REST API change, no GUI surface to screenshot. See the end-to-end request/response transcript above instead.
Type
Notes
check/apply) already exists in the service layer (canvas_extensions/installed.py, from the prior#4351PR) but is intentionally not exposed over HTTP here — the issue's endpoint list doesn't include it, so it's left out of scope rather than guessed at.install_path(an absolute server filesystem path) is exposed in API responses, matching the existingInstalledPluginResponse/InstalledSkillResponseconvention rather than introducing a new divergence.