fix(agent-ui): stop html previews from escaping their iframe sandbox - #1938
Open
ulivz wants to merge 1 commit into
Open
fix(agent-ui): stop html previews from escaping their iframe sandbox#1938ulivz wants to merge 1 commit into
ulivz wants to merge 1 commit into
Conversation
`allow-scripts` together with `allow-same-origin` voids the sandbox for content whose origin resolves to this page. Preview frames carry model-authored HTML through `srcDoc`, which inherits the embedder's origin, so that HTML could read and write `parent.document`, read the UI's storage and issue same-origin requests to the local agent API. No user interaction was needed: writing an `.html` file opens the panel and flips it to rendered mode on its own. Preview frames now run with `allow-scripts` alone, in an opaque origin. The parent can no longer reach `contentDocument`, so streaming updates moved from writing into the frame's DOM to alternating two frames' `srcDoc`, revealing the incoming document once it has loaded. Embedded tools keep `allow-same-origin` only when their URL is a cross-origin http(s) target, where it preserves the tool's own origin without granting any reach into the UI. Same-origin, relative, unparsable and non-http URLs get the strict policy.
✅ Deploy Preview for agent-tars-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for tarko ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
multimodal/tarko/agent-uirenders model-authored HTML into iframes whosesandboxattribute contained bothallow-scriptsandallow-same-origin. Per the HTML spec that combination is not a sandbox at all whenever the framed content's origin resolves to the embedder's own origin — andsrcdocdocuments inherit the embedder's origin by definition. Chromium says so itself:Four frames were affected: the streaming HTML preview, the fullscreen HTML preview, and the two embed frames.
The preview path needs no user interaction. A file-writing tool call whose path ends in
.htmlopens the workspace panel, and the panel switches itself torenderedmode, which hands the file's contents straight to the preview frame. Shared/replay pages are a second delivery surface.Impact
Script inside such a frame is same-origin with the UI, so it can read and write
parent.document, read the UI'slocalStorage, and issue same-origin requests to the agent server that serves the UI — using the victim's own browser context. Because the agent exposes tools that run commands, driving that API is the escalation path, which is why this is treated as a code-execution issue rather than "just" DOM access.Measured in Chromium against a real HTTP origin, a
srcDocframe with the old attribute pair reportedwindow.originequal to the embedder's origin and then successfully read the embedder'sdocument.title, read an embedder-scope JS global, wrote the embedder'sdocument.title(verified on the embedder side, not self-reported), read the embedder'slocalStorage, and completed a same-originfetch. In that configuration the frame's "own" storage and the embedder's storage are literally the same object — verified in both directions.Fix
allow-same-originmeans "let this document keep its own origin". Whether that is safe depends entirely on what its own origin is, so the two frame kinds are treated differently. The policy now lives in one place,src/common/constants/iframeSandbox.ts.HTML previews (
ThrottledHtmlRenderer,FullscreenModal) — always strict,allow-scriptsonly. Their content arrives viasrcDocand therefore inherits this page's origin; there is no legitimate need for a one-shot preview to keep origin-keyed state. Scripts still execute, now in an opaque origin.Embed frames (
EmbedFrameRenderer) — decided per URL byresolveEmbedFrameSandbox(src). A cross-originhttp(s)target keepsallow-same-origin; a same-origin, relative, unparsable or non-http(s)URL (javascript:,data:,blob:,about:) gets the strict policy, and any parse failure falls through to strict.allow-forms allow-popups allow-modalsis retained in both cases because embedded tools drive their own forms, popups and dialogs.Keeping the flag for cross-origin targets is not a concession: it does not grant embedder access. The framed page keeps its own remote origin, which is still cross-origin to the UI, and parent access stays refused with
SecurityError: ... Blocked a frame with origin "<remote origin>" from accessing a cross-origin frame.What it does preserve is the embedded tool's ability to use its ownlocalStorage,sessionStorage, cookies and same-origin requests. Dropping it there would buy no protection and would break embedded tools outright — measured:SecurityError: ... The document is sandboxed and lacks the 'allow-same-origin' flag.for storage,TypeError: Failed to fetchplus a CORS rejection fromorigin 'null'for the tool's own-origin requests. Embed URLs also come from deployment-side web UI configuration rather than model output, so they are not on the zero-click path that motivates this change.Streaming preview mechanism
The old streaming path had the parent write into the frame:
iframeDoc.open()/write(),tempDiv.innerHTMLfor appended chunks, and asrcDocfallback. An opaque origin makescontentDocumentunreachable, so that mechanism had to go.ThrottledHtmlRenderernow keeps two stacked frames and alternates them: incoming content goes into the hidden frame'ssrcDoc, and the frames swap visibility on that frame'sloadevent, so a partially parsed or empty document is never shown. Streaming commits stay throttled to one per 200 ms as before, and a non-streaming change commits immediately. A guard covers the case where the hidden frame already holds byte-identical content, which would otherwise produce noloadevent and no swap.Verification
Mechanism-level, before/after, same payload, only the
sandboxattribute differing:allow-scripts allow-same-originallow-scriptswindow.originnullparent.document.titleparent.document.titlelocalStoragefetch200+ bodyTypeError: Failed to fetchVerbatim after-fix errors:
SecurityError: Failed to read a named property 'document' from 'Window': Blocked a frame with origin "null" from accessing a cross-origin frame.and, for storage,SecurityError: Failed to read the 'localStorage' property from 'Window': The document is sandboxed and lacks the 'allow-same-origin' flag.Theorigin "null"in those messages is the opaque origin taking effect. The parent side also confirms isolation:iframe.contentDocumentnow readsnull, where before it returned a live document.Component-level, driving the real components in a browser with fabricated props:
<script>in the previewed document still executes. Finalsrcdocis byte-identical to the final input, and the resulting DOM is character-identical to what a plainsrcDociframe produces from the same string. Throttling verified: 14 chunks at 40 ms produced 5 document loads, final content still complete.sandbox="allow-scripts", all sections, scripts ran.allow-same-origin, and the tool'slocalStorage,sessionStorage, cookies,fetch/XHR and WebSocket (correctOriginheader) all work, whileparent.document,parent.locationandtop.locationremain blocked. Same-origin and relative targets: attribute excludesallow-same-originand parent access is blocked.allow-forms,allow-popupsandallow-modalsverified working under both policies. The full URL matrix (http(s)cross-origin, protocol-relative, same-origin absolute, root-relative, path-relative,javascript:,data:,blob:,about:blank,file:,ws:, unparsable, empty) resolves as intended.Build artifact,
multimodal/tarko/agent-ui-builder/static/:Minification hoists the two policies to module-level variables referenced by all four frames;
allow-same-originis appended at runtime only on the cross-origin embed branch. In the source tree the token now appears only in explanatory comments.tsc --noEmitfor the package: zero new errors against the base commit, and one pre-existing error removed (TS2551 Property 'srcDoc' does not exist on type 'HTMLIFrameElement', from the deleted imperative fallback). The 43 remaining errors are pre-existing and in untouched files.viteston the repo is unchanged versus the base commit (same 10 pre-existing collection failures from unbuilt sibling packages; 700/700 individual tests pass). Prettier clean. This package has no tests or lint script of its own, so the security property rests on the browser measurements and the artifact check above.Behaviour differences, stated plainly
Replacing a document is not identical to mutating one, and two differences are user-perceptible:
body { background-color: white }inside the frame after every write, overriding the previewed document's own stylesheet — that inline override is gone, so a document that sets a dark background now renders dark. Previously such a document could render white-on-white and be unreadable. The wrapper keeps its white background for documents that set none.100vhbox.Not in this change
EmbedFrameRendererhas a pre-existing quirk unrelated to this fix: when it first renders with an emptysrcit returns early, so itsResizeObserveris never attached and a later non-emptysrcrenders unscaled. ItsisFullscreenbranch is also currently unreachable; both of its frames read the same resolved policy, so the conditional applies consistently if that branch is ever wired up.