Skip to content

fix(web): stop an MCP App navigating its sandbox frame elsewhere - #2875

Merged
cliffhall merged 5 commits into
v2/mainfrom
v2/fix/2872-sandbox-frame-navigation
Nov 4, 2026
Merged

cliffhall merged 5 commits into
v2/mainfrom
v2/fix/2872-sandbox-frame-navigation

Conversation

@cliffhall

@cliffhall cliffhall commented Nov 4, 2026 •

Copy link
Copy Markdown
Member

Closes #2872

Fix for GHSA-h9j8-vj8g-v3gg, developed and reviewed in the advisory's private fork (inspector-GHSA-h9j8-vj8g-v3gg#1) and landed here as an ordinary PR. The commits are the fork branch's, unchanged.

An MCP App rendered on the opaque-origin (srcdoc) path could navigate its own sandbox frame to another origin. Nothing stopped it:

  • Sandbox flags only govern other browsing contexts.
  • No CSP directive governs a frame navigating itself.

The navigated page kept origin "null" and the same contentWindow, so the proxy kept treating it as the app:

  • the navigation URL carried data out despite connect-src 'none'
  • host messages (tool results) were relayed to it with "*"
  • its JSON-RPC was relayed to the host

Changes

  1. Frame policy on the proxy document, built per render (clients/web/static/sandbox_proxy.html). frame-src and child-src allow the approved frameDomains on the srcdoc path, the dedicated app origin on the _meta.ui.domain path, and 'none' otherwise.
    • It is inserted as a <meta> CSP, because the app's CSP grant is only known after the proxy page loads.
    • The response header stays frame-ancestors-only, per sandbox-controller.ts: a static header would break approved frameDomains, since srcdoc inherits it.
    • The host now passes the approved grant in the spec's csp slot of sandbox-resource-ready (createAppBridgeFactory.ts). The proxy reads only frameDomains and re-screens it.
  2. Navigation detection.
    • On the srcdoc path, a second load, or a securitypolicyviolation from the proxy's own frame policy, closes the relay in both directions.
    • It also removes the frame and posts a notifications/message (level error) to the host.
    • The violation trigger covers Firefox, which blocks the navigation without firing a second load.
  3. The false comment is corrected. It claimed "*" was safe because a srcdoc frame cannot navigate elsewhere.

Tests

  • createAppBridgeFactory.test.ts: only approved frameDomains reach the proxy.

  • New engine smoke npm run smoke:web:sandbox (scripts/smoke-web-sandbox-navigation.mjs), against the real prod-served proxy. It checks three cases:

    • a well-behaved app works
    • a navigation to an undeclared origin produces zero requests to the attacker and no relayed tools/call, and the error is reported
    • a navigation to a declared frame domain is caught and the frame removed

    It fails on the old proxy (the frame reached an undeclared origin: /attack?leak=x). It is in ENGINE_SMOKES: Chromium in CI, Firefox in the gate.

  • Manual harness, Chromium 156 and Firefox, before and after: the leak and the bridge takeover reproduce on v2/main and are blocked with this change. Nested approved frames and dedicated-origin apps still work.

  • npm run local:gate: green, including smoke:web:sandbox under both engines and Storybook 123/123.

Known residual

Navigating to a domain the app declared in frameDomains passes the policy. A fixed script in the host wrapper runs before the app's code and tells the proxy when the frame starts to leave, on beforeunload and pagehide. The proxy then closes the relay and removes the frame. It does this even when the app navigates before its own document has finished loading, with a second-load check as a backup. In practice the frame is gone before the destination's first request is sent. A relayed tools/call in that window is now unlikely, but no browser guarantee rules it out. That is still within the grant: a declared frame domain is already an egress channel, and the app could send that call itself. Closing it fully needs a MessagePort transport, which requires SDK and spec changes and is out of scope.

Lines

v2 only. v1's proxy checks the inner frame's real origin, so a navigated page's messages are rejected there.

🤖 Generated with Claude Code

cliffhall and others added 2 commits October 29, 2026 00:12
A frame sandboxed with allow-scripts may always navigate itself, and the
sandbox flags follow it, so the page it lands on keeps the opaque "null"
origin and the same contentWindow. That page passed every identity check
the sandbox proxy's relay makes, received host messages relayed with "*",
could post requests back to the host, and ran under none of the app's CSP.
The proxy's own comment claimed the opposite.

The proxy now installs a frame-src / child-src policy on its own document
before loading the inner frame, built per render from the approved
frameDomains (or the dedicated app origin), so a navigation to an
undeclared origin is blocked before any request leaves the browser. The
host passes the approved grant in the resource-ready message's spec `csp`
slot for this. As defense in depth, a second load on the srcdoc frame
closes the relay in both directions, removes the frame and reports an
error to the host through the app logging channel.

Adds smoke:web:sandbox, an engine smoke that drives the real proxy from
the prod server with a stand-in host and asserts both controls.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
Firefox blocks a frame-src-refused navigation without committing an
error page, so no second load ever fires there and the host was never
told. The proxy now also listens for the policy violation its own frame
policy raises, which both Chromium and Firefox report, and closes the
frame on whichever signal arrives first.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
@cliffhall cliffhall added the v2 Issues and PRs for v2 label Nov 4, 2026
@cliffhall
cliffhall requested a balanced review from Copilot November 4, 2026 20:42

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

An approved-domain navigation can retain the relay indefinitely when it produces the first load event, and the smoke test does not verify that declared-domain navigation actually succeeds.

2 open issues
What changed in this PR

This PR addresses #2872 by restricting MCP App sandbox navigation in the web client and closing message relays when navigation is detected.

Changes:

  • Adds per-render frame policies using approved CSP grants, plus frame removal and host error reporting on detected navigation.
  • Adds a sandbox-navigation browser smoke to the Chromium and Firefox engine suites.
  • Updates bridge tests, smoke registration, and quality-gate documentation.
File Description
scripts/​smoke-web-sandbox-navigation.mjs Adds normal, blocked-navigation, and declared-domain smoke cases.
scripts/​run-engine-smokes.test.mjs Updates assertions for sandbox smoke registration.
scripts/​run-engine-smokes.mjs Includes the sandbox smoke in engine runs.
scripts/​lib/​headless-browser.mjs Updates comments to include the new smoke.
package.json Adds the smoke:web:sandbox command.
docs/​quality-gate.md Documents sandbox coverage, engine runs, and ports.
clients/​web/​static/​sandbox_proxy.html Adds frame policies, navigation detection, and relay shutdown.
clients/​web/​src/​components/​elements/​AppRenderer/​createAppBridgeFactory.ts Passes approved CSP grants to the proxy.
clients/​web/​src/​components/​elements/​AppRenderer/​createAppBridgeFactory.test.ts Verifies that only approved frame domains reach the proxy.

Review effort: Balanced

Comment thread clients/web/static/sandbox_proxy.html Resolved
The proxy's load watch assumed the srcdoc fires the frame's first `load`.
A navigation started while the srcdoc is still loading aborts it, so the
destination's `load` is the first and was ignored, leaving the relay open
to the replacement document. The host wrapper now runs a script ahead of
the app's code that posts a departure message on `beforeunload` and
`pagehide`; the proxy closes the relay and removes the frame on it.

The sandbox smoke gains a case that navigates while a never-finishing
nested frame holds the srcdoc's load, and the declared-domain cases now
require a nested frame to the declared domain to be fetched, so a policy
refusing every navigation can no longer pass them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
@cliffhall

Copy link
Copy Markdown
Member Author

Copilot round 1 (review 5775535843): two findings, fixed in 3859e09 and answered in each thread.

  1. Navigating while the original load is pending.
    • Fix: a fixed script in the host wrapper (wrapSandboxedHtml) runs before the app's code and tells the proxy when the frame starts to leave. On the srcdoc path the proxy then closes the relay and removes the frame. The second-load check stays as a backup.
    • Events: pagehide alone isn't enough, because Chromium doesn't fire it when the navigation cuts the load short, so the script also listens for beforeunload.
    • New smoke case 4: a nested frame that never finishes loading holds the app's load open, then the app navigates to a declared domain. On the old proxy it timed out in Chromium. It now passes in Chromium and Firefox.
  2. Proving the declared navigation was allowed. The proxy now usually removes the frame before the /declared request is sent, so that request can't serve as proof. Cases 3 and 4 now first open a nested frame on the declared domain, and fail unless the listener served it. As a mutation check, I forced the proxy's frame policy to 'none', and case 3 failed.

local:gate passed (exit 0): web 9,601 tests across 474 files, Storybook 123/123, and smoke:web:sandbox OK in both engines. I updated the PR body's "Known residual" section to match.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

A critical navigation-detection bypass allows document.open() to remove departure listeners during a pending load, leaving the proxy relay open after navigation to an approved frame domain.

1 open issue
2 resolved since last review

Review effort: Balanced

Comment thread clients/web/src/utils/sandbox-csp.ts Resolved
@cliffhall

Copy link
Copy Markdown
Member Author

Copilot round 2 (review 5776227654): one finding, declined in its thread as outside #2872. A document.open() that strips the departure listeners can only keep the relay for a navigation to a domain the app declared. Such a page can do nothing the app couldn't already do, and undeclared origins are still refused by the frame policy. This is the documented "Known residual". No code change, so the review loop stops here.

CodeQL flagged JSON.stringify output spliced into script source in the
departure script and the sandbox smoke (js/bad-code-sanitization), and a
case-sensitive script-tag regex in a test (js/bad-tag-filter). The values
are host constants, but escape <, > and the line separators anyway so no
value can close the element or end a statement, and match the tag
case-insensitively.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

This browser-enforced security-boundary change needs independent Chromium/Firefox validation of CSP inheritance and navigation ordering, plus security-maintainer confirmation of the accepted declared-domain relay residual, because browser execution and private advisory review were unavailable.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

CodeQL's js/bad-tag-filter flags any script-tag regex. The wrapper writes
these bytes itself, so slice between the known tags instead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Human assessment is needed because the proxy can block approved URLs containing apostrophes, while early frame teardown can let the navigation smoke test pass without proving CSP enforcement.

No open issues 🎉

Review effort: Balanced

@cliffhall
cliffhall merged commit 9873aa8 into v2/main Nov 4, 2026
11 checks passed
@cliffhall
cliffhall deleted the v2/fix/2872-sandbox-frame-navigation branch November 4, 2026 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[GHSA-h9j8-vj8g-v3gg] - MCP App escapes its CSP by navigating its own frame

3 participants