Render HTML artifacts in the guest pool - #97
Conversation
HTML file artifacts rendered in a sandboxed, opaque-origin `<iframe>` while the
agent loaded the identical asset URL in a `<webview>` guest. Everything
origin-scoped therefore behaved differently between the surface the agent
screenshots to check its own work and the surface the user reads: localStorage,
cookies, and same-origin fetch of a sibling file. Agent-authored HTML that
persists a selection, or reads its own data.json, could pass the agent's check
and fail in front of the user.
Both now render as a real origin through the same pool, which also gives the
preview real back/forward/reload and makes escape-to-root a loadURL from any
depth rather than a remount of `src`. Both reload nonces are gone.
The guest is a second target kind, `${taskId}/artifact`, opened through its own
RPC rather than the session-keyed `browser.open`, with its own storage profile
per task. It is excluded from listTargets and refused by the CDP bridge, so the
agent's target list and CDP surface are unchanged.
Two things are deliberately not here. There is no lifetime machine: the target
id is derived from the task id, so trash-task closes it and removes its profile
with no registry, and nothing else reaps it -- one webContents per task whose
artifact you opened. And FileViewer withholds Expand for HTML, so exactly one
preview is ever mounted; two hosts sharing a guest would have to agree on which
file is on screen, who owns Cmd+F, and which may paint, none of which a viewer
can answer from its own props.
Net security tightening rather than a loss: the iframe's allow list granted
camera, microphone, geolocation, clipboard and USB, and its sandbox granted
popups; the guest session denies every permission and opens no child window.
What is new is storage, scoped per task because cookies are domain-scoped and a
shared jar would be a cross-task channel. `target=_blank` still reaches the OS
browser and downloads still prompt, both restoring iframe behavior the guest's
stricter defaults would otherwise have removed.
Also fixes three pre-existing bugs in the browser panel that this shares code
with: a body-mounted guest painted straight through app-wide modals, Cmd+F died
after any overlay closed over the panel, and a load failure survived a session
switch and named the previous session's URL.
|
Warning Review limit reached
Next review available in: 58 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (32)
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| // An artifact preview has no agent driving it, so there is no authorized | ||
| // path to route a download into and nothing to synthesize a CDP event for. | ||
| // Leave it to Electron, which prompts for a location: a "Download CSV" | ||
| // button in a generated report worked on the iframe this replaced (it | ||
| // carried `allow-downloads`), and cancelling here would make it silently | ||
| // dead. The user asked for the file by clicking it. | ||
| if (!entry.sessionId) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🟨 Downloads initiated by untrusted agent-authored HTML are no longer blocked
The download guard for browser guests previously cancelled any download that the agent had not authorized a path for. For artifact-preview guests the handler now returns early without cancelling (apps/studio/src/electron-main/browser-view/downloads.ts:48-50), so any download a page in the preview starts is handed to Electron's default behavior. The HTML rendered there is agent-authored and untrusted, and it can trigger a download without a user click (e.g. a script-created anchor with download, or a Content-Disposition response), which then surfaces a save dialog. Impact is limited because Electron prompts for a location rather than writing silently, and the guest's origin is the task's own asset origin.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Closing in favour of a redesign. The direction here — a second target kind ( The three pre-existing browser-panel bugs found while building this are split out into #99, which is small and stands alone. The analysis is preserved: |
Docs only. Sets the direction before any code, after #97 showed what building the artifact preview as its own parallel surface costs. ## The claim A task holds **browser tabs**. Either the user or the agent can open one, both can see all of them, either can drive one. Today we have three things pretending to be different — the agent's browser, the user's browser panel, and the HTML artifact preview — and they're the same thing with different provenance. The agent and the user already load the same URL through the same guest pool. ## The thing in the way is one line ```ts BrowserTargetId = `${TaskId}/${StoreId.Session}` ``` A session identity doing duty as a page identity. Everything downstream inherits the 1:1 — including the CDP bridge, which fakes the whole `Target.*` domain so the agent can never discover a second page. **So the tab restriction is ours, not agent-browser's.** The CLI has `tab new` / `list` / `switch` / `close` and an auto-attach path, all funnelling into `Target.createTarget`, which we intercept and redirect. Removing the fakery *is* the feature — we don't build a tab system, we stop suppressing one. ## What's new here versus the two existing plans `lazy-browser-targets-and-multiple-tabs.md` and `browser-popups-as-agent-drivable-tabs.md` already have the substrate and the popup consumer, and both stay. This adds: - **An ownership model** — `openedBy` (provenance, fixed), `boundSessionId` (current owner, reassignable — this is hand-over), and `agentControl: { sessionId, intent }` so a tab the agent is driving says so *and says why*. Provenance and binding are deliberately separate; conflating them is what makes "hand this tab to the agent" unrepresentable today. - **A hosting decision.** Keep in-pane tabs on the `<webview>` pool, and add "open in its own window" as the second host. Comparable desktop agent apps get a much simpler ride because their view bounds are window-relative — no slot measurement, no zoom arithmetic, no z-index contest. We can't give up a browser beside the chat, but we shouldn't pay for the zoomed-layout compositing twice: popups already *force* a main-process host, and making that host a window rather than a view slotted into the pane avoids reintroducing main-process bounds tracking over a zoomed layout. - **Where state goes.** The tab set is task state (`state.json`); foreground tab stays a route search param; per-tab history isn't persisted. The `artifactPanel: {type: "file" | "browser"}` union dissolves. - **Phasing**, each step independently landable, starting with lazy target creation (no protocol or UI change, deletes three drift-prone argv flag tables). ## Open questions I'd want your read on - Does a tab belong to a session or to the task? Leaning: the task owns tabs, sessions borrow them. - Does the agent see tabs it didn't open? Leaning yes — it can already see the user's browser — but `tab list` returning the user's tabs is a real privacy surface. - Does `openedBy` survive a hand-over, or get replaced by it? <!-- devin-review-badge-begin --> --- <a href="https://app.devin.ai/review/instrument-org/instrument/pull/100" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open in Devin Review"> </picture> </a> <!-- devin-review-badge-end -->
1 participant
Replaces #94, which grew far past what this needs. Same core change, ~1,100 lines instead of ~2,600, and none of the intricate parts.
Why
HTML file artifacts rendered in a sandboxed, opaque-origin
<iframe>while the agent loaded the identical asset URL in a<webview>guest. So everything origin-scoped behaved differently between the surface the agent screenshots to check its own work and the surface the user reads:localStorage, cookies, IndexedDB, same-originfetchof a sibling file. Agent-authored HTML that persists a filter selection, or reads its owndata.json, is exactly the class of thing that passed the agent's check and failed in front of the user.Back/forward chrome is the visible payoff and the smaller half.
What's here
${taskId}/artifact, opened throughworkspace.artifactPreview.openrather than the session-keyedbrowser.open.decodeBrowserTargetIdreturns a discriminated{kind}.loadURL. Both reload nonces deleted — escape-to-root works from any depth, and reloading a navigated-to sub-page works for the first time.listTargets, refused by the CDP bridge.use-guest-navigationhook: guests painting through app-wide modals, Cmd+F dying after an overlay closes, and a load error surviving a session switch.What's deliberately not here
These are the two decisions that kept it small, and both are load-bearing:
trash-taskcloses it and removes its profile with no registry to consult. One webContents per task whose artifact you opened, for the life of the task. Render HTML artifacts in the guest pool #94 supervised these with a presence lease and a grace period; that cost an XState machine, a 391-line test, and a family of teardown races, for a resource already bounded by how many tasks you open an artifact in.FileViewerwithholds Expand for HTML. Two hosts sharing one guest have to agree on which file is on screen, who owns Cmd+F, and which may paint — none of which a viewer can answer from its own props. That single fact produced most of Render HTML artifacts in the guest pool #94's complexity and roughly five of its thirteen review findings. The panel already gives an artifact the full pane. Every other file type keeps Expand.Security: a tightening
assets.<taskId>.localhost)allowtarget=_blankstill opens in the OS browser)The profile is per task, not shared:
localStorageis origin-keyed but cookies are domain-scoped, so a page onassets.<a>.localhostcould set one forlocalhostand read it from another task's preview through a shared jar.Two things the iframe did by inheriting the host page had to be restored explicitly:
target="_blank"links (now handed toopenExternal, or they'd be silently dead with no address bar) and downloads (now prompt, rather than being cancelled by the agent-guest download guard).Verification
Driven in a running Studio: real origin confirmed,
localStorageworks and survives navigation,fetch("data.json")succeeds, zero iframes. Following an in-page link updates the toolbar and Home returns to the entry page with storage intact. Expand is absent for HTML and still present for images./json?id=omits the artifact target and its CDP upgrade is refused.pnpm check-and-test:cigreen, 22/22.Not driven: app zoom at 0.5x/2x (slot measurement is untouched and shared with the browser panel) and range requests (
serveStaticFileis consumer-agnostic and untouched).