web: sandbox hardening — opaque-origin inner frame, srcdoc-only, frame-ancestors

[cliffhall]

Closes #1565

Hardens the MCP Apps sandbox isolation model (re-implementation of the relevant slice of PR #1510).

Changes

• Opaque-origin inner frame — the untrusted-widget iframe is sandboxed without allow-same-origin (default allow-scripts allow-forms), and any server-supplied sandbox value has allow-same-origin stripped case-insensitively. The widget runs under an opaque origin and cannot reach the proxy's DOM to bypass its own CSP via window.parent.
• srcdoc-only delivery — replaces document.write; srcdoc is the delivery path that works without allow-same-origin.
• Relay identity check — the inner frame's origin serialises as the literal "null"; the unforgeable check is event.source === inner.contentWindow, with the "null" origin asserted as defense-in-depth.
• frame-ancestors header — sandbox-controller.ts serves the proxy with Content-Security-Policy: frame-ancestors http://127.0.0.1:* http://localhost:*. Fetch directives are deliberately omitted so a srcdoc iframe's inherited policy container does not intersect with / override the per-app CSP.

Tests

• sandbox-controller.test.ts asserts the frame-ancestors header is present and that default-src/connect-src are absent.
• MCP Apps demo integration test still passes end-to-end.
• Full npm run ci passes locally (coverage gate, all smokes, Storybook).

Stacking

First PR in the Wave 2 Apps-host stack. Branches off v2/main; subsequent PRs (#1566 …) stack on this branch.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 20s —— View job

---

Review: sandbox hardening (PR #1650)

• Read the changed files
• Analyze security model (opaque-origin, srcdoc, frame-ancestors, relay identity)
• Review tests
• Post review

This is a well-reasoned security hardening. The four moves — dropping allow-same-origin, srcdoc-only delivery, the event.source === inner.contentWindow identity proof, and the frame-ancestors-only proxy header — fit together correctly, and the inline comments are unusually good at documenting why. Below are the things worth confirming; none are blockers.

✅ What's correct and notable

• Omitting fetch directives from the proxy CSP is the right call. A srcdoc iframe inherits its embedder's CSP policy container, and CSPs combine per-directive as "most restrictive wins." A default-src/connect-src on the proxy header would be inherited by the inner document and intersect with the per-app <meta> CSP from sandbox-csp.ts, silently breaking each app's own allowlist. The comment in sandbox-controller.ts:50-58 captures this precisely. Good.
• frame-ancestors inheritance is benign here. Even though the inner srcdoc doc inherits frame-ancestors http://127.0.0.1:* http://localhost:*, the proxy's own origin (a localhost/127.0.0.1 port) matches the wildcard-port allowlist, so the inner frame is still allowed to be embedded by the proxy. Worth being aware of, but it works out.
• The allow-same-origin strip is robust against token-splitting tricks. JS \s is a superset of the ASCII whitespace the HTML sandbox parser tokenizes on, and .toLowerCase() covers the ASCII-case-insensitive match — so no crafted token containing allow-same-origin can survive the filter yet be re-read as that token by the parser.

🟡 Worth considering (non-blocking)

1. safeSandbox can silently disable scripting (fail-closed, but surprising). In sandbox_proxy.html:167-171, if a server supplies a sandbox value that omits allow-scripts (e.g. "allow-same-origin" alone → safeSandbox === ""), the inner frame is set to the most-restrictive sandbox and the app never runs. This is safe (fails closed) and largely matches prior behavior, but a hostile/buggy server could brick an app with no diagnostic. Consider guaranteeing allow-scripts remains, or at least logging when the sanitized value drops it. Fix this →

2. Security-critical inline-script logic has no direct unit test. The strip, the srcdoc assignment, and the INNER_ORIGIN === "null" check all live in the sandbox_proxy.html inline script and are only exercised indirectly by the MCP Apps demo integration test. The strip in particular is the primary new defensive boundary — a focused test (extracting the token-filter into a testable helper, or asserting proxy behavior in the integration suite) would guard against regressions. Understood that inline HTML scripts are awkward to cover under the ≥90 gate; flagging for completeness.

3. Unrelated lockfile churn. All four package-lock.json files drop "version": "2.0.0" and clients/launcher/package-lock.json gains ~550 lines of @emnapi/* / @napi-rs/wasm-runtime optional deps. None of this relates to sandbox hardening — it looks like incidental npm install regeneration. The version drop is consistent with the "single version number" invariant, but consider splitting the launcher lock changes out so the security diff stays focused (and to make the added transitive deps a deliberate, reviewable change rather than a side effect).

Nits

• sandbox_proxy.html:100 — the window.top?.alert(...) self-test relies on window.top throwing a SecurityError; unchanged by this PR and still correct, just noting it depends on the proxy being cross-origin to the top frame (different port), which the sandbox-controller guarantees.

Overall: solid, defensible isolation model with good documentation. My only substantive asks are optional (test coverage for the strip, and keeping the lockfile noise out of the security diff).
· branch v2/1565-sandbox-hardening

[cliffhall]

Thanks for the thorough review. Addressed in cdd166d:

1. safeSandbox can silently disable scripting — Applied (as the "log when dropped" option). After stripping allow-same-origin, the proxy now console.warns when the sanitized token list lacks allow-scripts, so a fail-closed sandbox is diagnosable rather than silent. I kept it fail-closed rather than force-injecting allow-scripts, so a server that deliberately narrows the sandbox still gets what it asked for — just with a visible diagnostic.

2. No direct unit test for the inline strip — Declined, with reason. The strip / srcdoc assignment / INNER_ORIGIN === "null" checks live in the inline script of sandbox_proxy.html, which is served as a self-contained static page and can't be imported or executed under happy-dom. Extracting the token-filter into an external module would change the trusted-proxy delivery model (the proxy must ship as one static file the sandbox controller serves verbatim). It stays covered indirectly by the mcp-app-demo integration test. Flagging as a known coverage limitation, consistent with your note.

3. Unrelated lockfile churn — Applied. Reverted clients/{cli,launcher,tui,web}/package-lock.json to their v2/main state; the @emnapi/@napi-rs additions and dropped version field were incidental npm install regeneration in my worktree, not deliberate dep changes. The diff is now sandbox-only.

Nit (window.top self-test): acknowledged, no change — unchanged by this PR and still correct given the proxy is cross-origin (different port) to the top frame.

npm run ci is green after the changes.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 8s —— View job

---

Re-review: sandbox hardening (PR #1650)

• Read the updated files (sandbox_proxy.html, sandbox-controller.ts, tests)
• Verify the three feedback items from the prior round
• Check for regressions / new edge cases
• Post review

Thanks for the fast turnaround, @cliffhall. I re-checked the commit cdd166df against origin/v2/main — all three items land cleanly, and the reasoning for the one you declined is sound. Ship it.

✅ Verified resolutions

1. safeSandbox silent-brick → now diagnosable. sandbox_proxy.html:167-180 filters into safeTokens, then console.warns when the sanitized list lacks allow-scripts before applying it. I like that you kept it fail-closed (a server that deliberately narrows the sandbox still gets exactly what it asked for) and only added the diagnostic — that's the right trade-off. I also traced the empty-string edge (sandbox: ""): "".split(/\s+/) → [""] → filtered to [], so the warn fires and the frame gets the most-restrictive sandbox="" — fail-closed and visible. Good.

2. No direct unit test for the inline strip → declined, reasonably. Agreed. The strip / srcdoc / INNER_ORIGIN logic lives in the verbatim-served static proxy, which can't be imported under happy-dom, and extracting it into a module would change the trusted-proxy delivery model. Indirect coverage via the mcp-app-demo integration test plus the well-documented inline comments is an acceptable state for a security boundary that must ship as one static file. Noted as a known limitation, not a blocker.

3. Lockfile churn → fully reverted. Confirmed with a direct diff against base — all four lockfiles are now byte-identical to v2/main:

clients/cli/package-lock.json: 0
clients/launcher/package-lock.json: 0
clients/tui/package-lock.json: 0
clients/web/package-lock.json: 0

The diff is now sandbox-only. 👍

Nits from last round

• window.top self-test (sandbox_proxy.html:96-110) — acknowledged, no change, still correct.
• The new console.warn is [Sandbox]-prefixed, matching the existing console.error relay diagnostics — consistent, no action needed.

Nothing further from me. The isolation model (opaque-origin inner frame · srcdoc-only · event.source === inner.contentWindow identity proof · frame-ancestors-only proxy CSP) is coherent, and the inline comments explaining why fetch directives are omitted from the proxy header remain the standout part of this change. LGTM. ✅
· branch v2/1565-sandbox-hardening

[cliffhall]

Superseded by the wave rollup PR #1660 (retargeted to v2/main), which contains this change as part of the full cumulative diff. Closing this per-issue PR in favor of the rollup so the wave merges as one PR. Branch retained until #1660 merges; the linked issue will be closed and its board card moved to Done when the rollup lands.