fix(preview): send CSP on preview responses to close RCE (#2771)#2866
fix(preview): send CSP on preview responses to close RCE (#2771)#2866Kaushik2003 wants to merge 1 commit into
Conversation
…r#2771) The preview serves workspace Markdown/HTML as text/html with no CSP. Over the legacy /preview/files/ route the document is same-origin with the unauthenticated loopback control API, so a <script> in a previewed file can fetch() PUT /api/v1/projects/{id}/config to set postCreate, which the session manager later runs via sh -c — arbitrary command execution from merely opening the Browser tab on an unvetted repo. Set a Content-Security-Policy in the shared serveWorkspacePreviewFile path, so it covers both the legacy route and the isolated *.localhost origin: - rendered Markdown: default-src 'none' (+ image/style/font allowances) — a document needs no scripts or network; - raw/built-app files (incl. .html, which cannot be sanitized): connect-src 'none' + form-action 'none', so scripts still render the page but cannot reach the loopback API. connect-src 'none' is load-bearing: the exploit needs a scripted fetch/XHR, and the config endpoint is PUT+JSON so a form cannot forge it. Documents and built-app rendering are unaffected; only a preview page's own runtime network fetch is blocked. Adds a regression test asserting the CSP on the Markdown and raw-file paths.
|
Thanks for contributing to Agent Orchestrator. This PR is being picked up by the current external contributor on-call pair: If someone is already working on this, please continue as usual. For faster context or live questions, you can also join the AO Discord. Join the session here: Come by if you want to see what is being built, ask questions, or just hang around with the community. |
| // .html, which cannot be sanitized). Scripts/styles/images still load so a | ||
| // built static app renders, but connect-src 'none' blocks fetch/XHR/WebSocket | ||
| // so page script cannot reach the loopback control API (issue #2771). | ||
| const previewFileCSP = "connect-src 'none'; form-action 'none'" |
There was a problem hiding this comment.
This raw-file policy is bypassable on the legacy same-origin /preview/files/ route, so it does not fully deliver "page script cannot reach the loopback control API" for raw .html.
connect-src 'none' only constrains fetch/XHR issued from this document's own realm. It has no frame-src/child-src/object-src/worker-src, and there is no default-src to backstop them. On the legacy route the preview is same-origin with the API (http://127.0.0.1:<port>), and the daemon sets no global X-Frame-Options/frame-ancestors (nothing in internal/httpd sets them). So a malicious index.html served under this policy (scripts run) can reach the sink through a nested same-origin iframe:
const f = document.createElement('iframe');
f.src = '/api/v1/health'; // any same-origin API doc; its response carries NO CSP
f.onload = () => f.contentWindow.fetch('/api/v1/projects/ID/config', {
method: 'PUT',
headers: { 'Content-Type': 'application/json' },
body: '{"postCreate":"..."}',
});
document.body.appendChild(f);The fetch executes in the child realm, whose response has no CSP, so the parent's connect-src 'none' does not apply. (An about:blank iframe inherits the parent policy and would be blocked, but a navigated same-origin document gets its own empty policy, which is the hole.) The Markdown policy is unaffected because default-src 'none' implies frame-src 'none'.
Suggested fix: add frame-src 'none'; child-src 'none'; object-src 'none'; worker-src 'none' to previewFileCSP (or use default-src 'none' and re-allow only script-src/style-src/img-src/font-src). Note this may restrict a built app that legitimately uses iframes/workers, so it is a real trade-off worth calling out.
What
The browser-panel preview serves workspace Markdown and HTML as
text/htmlwith no Content-Security-Policy. On the legacy/api/v1/sessions/{id}/preview/files/route the preview document is same-origin with the daemon's control API (http://127.0.0.1:<port>), so a<script>in a previewed file can call that API directly — turning "open the Browser tab on a repository" into arbitrary command execution.This PR sends a restrictive CSP on every preview response, differentiated by content type so existing functionality is preserved. The header is set in the single shared serving path (
serveWorkspacePreviewFile), so it covers both the legacy route and the newer isolated*.localhostpreview origin.Why
Fixes #2771.
The vulnerability
Nothing more than a file landing in the workspace — for example a freshly cloned repository opened for inspection — is required to reach the daemon's privileged API. The chain:
WithUnsafe())backend/internal/preview/markdown.gotext/htmlwith no CSPbackend/internal/httpd/controllers/sessions.go(serveWorkspacePreviewFile).mdin the workspace is auto-selected, so opening the preview is enough to load attacker-controlled contentbackend/internal/preview/entry.go(DiscoverEntry)backend/internal/httpd/controllers/projects.go(PUT /projects/{id}/config)postCreatereachessh -cwithcmd.Dirset to the workspacebackend/internal/session_manager/manager.go(runPostCreate)Concretely, script in a previewed file can
fetch()PUT /api/v1/projects/{id}/configto setpostCreate, which the session manager later executes viash -c.The preview loads through a top-level navigation (
frontend/src/main/browser-view-host.ts,webContents.loadURL(...)), so an HTTP-response CSP header authoritatively governs the document and the browser blocks thefetch.Root cause
The comment in
markdown.go— "preview content is workspace-local and agent-trusted" — conflates being in the workspace with being authorized to execute. Untrusted workspace content is served in a context that can reach a privileged API, with nothing instructing the browser to contain it.How
The fix
Set a CSP header in
serveWorkspacePreviewFile, the one function through which both the legacy/preview/files/route and the isolated preview origin serve content, with a policy chosen per content type. The load-bearing directive isconnect-src 'none': the exploit requires a scriptedfetch/XHR to the API, and because the config endpoint isPUT+JSON a<form>cannot forge the request either.Two policies are defined:
They are applied in the two branches of
serveWorkspacePreviewFile:The pre-existing
//nolint:gosecon the Markdown write is retained, but its justification is updated to cite the CSP rather than the "agent-trusted" assumption that no longer holds.Why CSP rather than the alternatives
WithUnsafe()) helps only the Markdown path and does nothing for raw.html, which is served verbatim. CSP covers both.postCreateis not viable: running arbitrary shell is that field's intended behavior, so there is no "malicious value" to reject.Design notes and trade-offs
default-src; scripts still run) so a built static app renders. Containment there rests onconnect-src 'none'+form-action 'none': untrusted HTML may execute in-page but cannot reach the loopback API.img-src *(rather than'self' data:) preserves README badges and remote images. The trade-off is that a previewed file can still issue imageGETs, an IP-leak channel — not the reported RCE, and tightenable later.connect-src 'none'. Because the header lives in the shared serving path, it applies uniformly to the*.localhostorigin as well. This is a deliberate defense-in-depth choice; if maintainers would prefer to let a built app fetch its own data on its isolated origin, the raw-file policy can be relaxed on that path specifically.Relationship to recent preview work
*.localhostpreview origins (Resolve root relative assets in static previews #2818) already move preview off the API's origin. This change is complementary: the legacy/preview/files/route is still same-origin with the API and still needs the header, and on the isolated origin the CSP is defense-in-depth. Setting it in the shared serving path covers both with one change.OpenWorkspaceFile(OpenRoot) path already blocks the in-workspace symlink escape, so that concern is not part of this PR.Compatibility
img-src * data:)style-src 'unsafe-inline')connect-src 'none'— the one behavioral changepostCreate/ dev-server previewFollow-up (out of scope)
postCreate) — the remaining defense-in-depth idea, separate from this fix.Related work
*.localhostpreview origins (this PR is complementary).marked + DOMPurify(separate from this daemon-side path).Testing
From
backend/:go build ./...— compiles.go test ./internal/httpd/controllers/— passes, including the newTestSessionsAPI_PreviewSendsCSP, which asserts the CSP header on both the Markdown and raw-file preview responses (and fails without this change).go vet ./internal/httpd/controllers/— clean.golangci-lint runon the changed package — 0 issues.Checklist
main(or continuing an existing PR branch)go, frontend, etc.)