web: enforce per-app _meta.ui.csp + surface resource-read failures by cliffhall · Pull Request #1652 · modelcontextprotocol/inspector

cliffhall · 2026-07-11T16:53:58Z

Wires per-app CSP enforcement into the app bridge and surfaces resource-read failures (re-implementation of the relevant slice of PR #1510).

Changes

createAppBridgeFactory builds each app's _meta.ui.csp host-side via the sandbox-csp library (approveCspSources → buildSandboxCspPolicy) and wraps the untrusted widget HTML with wrapSandboxedHtml, so the CSP <meta> is always the first <head> child before the HTML reaches the sandboxed iframe. The approved (post-filter) csp + permissions are echoed back through hostCapabilities.sandbox. The csp is no longer sent inline via sendSandboxResourceReady.
A per-app copy of HOST_CAPABILITIES keeps the sandbox echo from mutating the shared constant.
New optional onResourceError dep routes read/post failures to the caller (always console.error'd too) instead of a silent blank frame.

Error display in AppsScreen (data-app-error) is intentionally out of scope here — it belongs to the app-status issue (#1572). This PR only surfaces the error via the factory dep.

Tests

Updated/added factory tests: CSP wrapping, unsafe-source dropping, csp echo via hostCapabilities, and onResourceError/console.error on read failure, no-text content, and non-Error rejection.
Per-file coverage ≥90 on all four dimensions; full npm run ci passes locally.

Stacking

Stacked on #1565 (v2/1565-sandbox-hardening). Review/merge that first.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw

…e-read failures - createAppBridgeFactory now builds each app's `_meta.ui.csp` host-side via the sandbox-csp library (approveCspSources → buildSandboxCspPolicy) and wraps the untrusted widget HTML with wrapSandboxedHtml so the CSP <meta> is always the first <head> child before the HTML reaches the sandboxed iframe. The approved (post-filter) csp + permissions are echoed back via hostCapabilities.sandbox so the view sees what was granted. The csp is no longer sent inline through sendSandboxResourceReady — it is enforced via the wrapped meta. - A per-app copy of HOST_CAPABILITIES prevents the sandbox echo from mutating the shared constant. - New optional `onResourceError` dep on AppBridgeFactoryDeps routes resource read/post failures to the caller (also always console.error'd) instead of leaving a silent blank frame. Closes #1566 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw

cliffhall · 2026-07-11T21:44:30Z

@claude review

claude · 2026-07-11T21:44:45Z

Claude finished @cliffhall's task in 4m 15s —— View job

Review: per-app CSP enforcement + resource-error surfacing

Overall this is a clean, well-documented change. The host-side CSP construction (approveCspSources → buildSandboxCspPolicy → wrapSandboxedHtml) is the right shape: the policy <meta> is guaranteed to be the first <head> child of a host-authored shell, and the untrusted markup lands in <body>, so the app can't push the meta inert. Moving CSP out of the inline sendSandboxResourceReady payload and into the wrapped srcdoc is a solid hardening. The per-app copy of HOST_CAPABILITIES correctly avoids mutating the shared constant, and the error path is a real improvement over a silent blank frame. Tests cover the important branches. A few things worth a look:

1. The sandbox echo depends on `AppBridge` reading `hostCapabilities` lazily (verify)

createAppBridgeFactory.ts:171 mutates hostCapabilities.sandbox inside the sandboxready handler, which fires after bridge.connect(transport) at line 199. This only reaches the view if AppBridge holds a live reference to the capabilities object and reads it when it responds to ui/initialize — the code comment (:167-169) states this is the intended ordering, and it's correct if that assumption holds.

The risk: if the installed @modelcontextprotocol/ext-apps (^1.7.1) snapshots/clones capabilities at construction or at connect() time, the echoed sandbox block would be silently stale (undefined), and no test would catch it — the unit mock (createAppBridgeFactory.test.ts:42-45) just stashes the constructor arg by reference, so ctorArgs[2].sandbox reflects the later mutation regardless of what the real bridge does. Please confirm the real AppBridge reads capabilities lazily (or that the AppsScreen Storybook play test exercises the actual echo end-to-end). If it snapshots, the fix is to pass sandbox in at construction and update it, or otherwise ensure the value is set before the handshake response is generated.

2. Permissions are echoed/forwarded unfiltered while CSP is filtered

CSP goes through approveCspSources (injection-safe filtering), but meta?.permissions is passed through verbatim both to sendSandboxResourceReady (:177) and the hostCapabilities.sandbox echo (:172). If untrusted permissions values ultimately land in the iframe sandbox/allow attribute, they'd want the same allowlist screening CSP sources get. This may be intentional — the sibling #1565 sandbox-hardening work owns the sandbox-attribute layer — but it's worth a one-line comment noting where permission values are validated, so the asymmetry doesn't read as an oversight.

3. Minor: `onResourceError` is defined but not yet wired

Confirmed App.tsx:704 doesn't pass onResourceError, so today the only observable effect of a read failure is the console.error. That matches the PR description (display is deferred to #1572) and the dep is correctly optional — just flagging that the user-facing benefit isn't live until #1572 lands.

Nits

extractHtmlAndMeta (:90) returns the first content block with a string text. That matches the resource contract, but silently ignores a blob-only or multi-block resource — the "no text" error message covers the empty case, which is the important one.
Test naming/coverage is good; the "drops an unsafe app-supplied CSP source" case (:189) nicely proves the injection-terminator is stripped end-to-end into the wrapped html.

None of these are blocking on their own — #1 is the one I'd confirm before merge, since a stale echo would fail silently.
· v2/1566-per-app-csp

Address the claude[bot] review's finding #2: add a comment explaining why `csp` is injection-filtered (approveCspSources) while `permissions` is passed through verbatim — permissions is a structured boolean object consumed only by the sandbox proxy's buildAllowAttribute() (fixed key→token mapping), so it never reaches the iframe sandbox/allow attribute as raw text. No logic change. Also merges v2/1565-sandbox-hardening (review fixes) up the stack. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw

cliffhall · 2026-07-11T21:54:38Z

Thanks — great catch prioritizing #1. Responses:

1. AppBridge reads hostCapabilities lazily — confirmed, no change needed. I traced the installed @modelcontextprotocol/ext-apps bundle:

The host-side AppBridge constructor stores the capabilities object by reference: this._client=X; this._hostInfo=Y; this._capabilities=Z (no clone/snapshot).
getCapabilities(){ return this._capabilities } returns that same reference.
The ui/initialize request handler builds its result with hostCapabilities: this.getCapabilities(), i.e. it reads the live object at handshake-response time.

So mutating hostCapabilities.sandbox inside the sandboxready handler (which fires before the view sends ui/initialize) is reflected in the initialize result — the echo is live, not stale. The ordering the code comment relies on holds against the real bridge. Kept as-is.

2. Permissions echoed/forwarded unfiltered while CSP is filtered — applied (comment). Added a comment at the mutation site (345395b) explaining the asymmetry: csp sources are string-interpolated into the CSP <meta> content, so they need injection filtering; permissions is a structured boolean object (camera/microphone/geolocation/clipboardWrite) whose only consumer is the sandbox proxy's buildAllowAttribute(), which maps each known key to a fixed Permissions-Policy token and ignores anything else — untrusted values can't reach the iframe sandbox/allow attribute as raw text. The sandbox-attribute layer (and the allow-same-origin strip) is owned by #1565.

3. onResourceError not yet wired — intentional for this PR. Correct; display is deferred to #1572 (which does wire it, to a red toast in App.tsx). This PR only introduces the optional dep, as described.

Nits (extractHtmlAndMeta first-text-block, test naming) — acknowledged, no change; the empty/no-text case is the important one and is covered.

npm run ci is green.

cliffhall · 2026-07-11T21:54:45Z

@claude review

claude · 2026-07-11T21:54:58Z

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

Re-review: per-app CSP enforcement + resource-error surfacing

Read changed files at current HEAD
Re-check the three prior review points (CSP wiring, permissions asymmetry comment, error path)
Review tests
Post review

The three points from the earlier review are all addressed, and the added asymmetry comment (345395b) is accurate against the sandbox-csp lib — I confirmed each of its claims:

Progress notifications #1 (lazy capabilities read): your trace matches the intended ordering. The mutation of hostCapabilities.sandbox in the sandboxready handler is a live update to the same object the bridge holds by reference, so the ui/initialize result reflects the approved config. ✅
Request logging #2 (CSP-vs-permissions asymmetry): the comment at createAppBridgeFactory.ts:171-180 correctly describes the split — csp sources are string-interpolated into the <meta content> (via buildSandboxCspPolicy) and so pass through approveCspSources (SAFE_CSP_SOURCE rejects ;"<>), while permissions is a structured boolean object consumed only by the proxy's buildAllowAttribute(). Reads correctly now, not as an oversight. ✅
Completion requests #3 (onResourceError unwired): correctly optional; deferral to web: app lifecycle status + machine-readable data attributes on the Apps screen #1572 is clear. ✅

The host-authored shell (wrapSandboxedHtml) with the CSP <meta> as the guaranteed-first <head> child, plus escapeHtmlAttr as defense-in-depth over the already-filtered sources, is a solid layering. No blocking issues.

One nit — the per-app-copy isolation isn't locked in by a test

createAppBridgeFactory.ts:141 uses { ...HOST_CAPABILITIES } specifically so the sandbox echo can't mutate the shared constant. But no test asserts that invariant: the suite reads bridge.ctorArgs[2] (the same object that got mutated), so if the spread were dropped — const hostCapabilities = HOST_CAPABILITIES — every existing test would still pass while HOST_CAPABILITIES.sandbox silently leaked across apps (and across tool renders). Since HOST_CAPABILITIES is exported, a one-liner closes the gap:

import { createAppBridgeFactory, HOST_CAPABILITIES } from "./createAppBridgeFactory";

it("does not mutate the shared HOST_CAPABILITIES when echoing the approved sandbox", async () => {
  const readResource = vi.fn().mockResolvedValue(
    uiResource("<h1>x</h1>", { csp: { connectDomains: ["https://api.example.com"] } }),
  );
  const factory = createAppBridgeFactory({ getClient: () => fakeClient, readResource });
  await factory(makeIframe(), tool);
  bridgeInstances[0].emit("sandboxready");
  await flush();
  expect(HOST_CAPABILITIES.sandbox).toBeUndefined();
});

Non-blocking — the current behavior is correct; this just prevents a silent regression of the exact bug the copy guards against. Fix this →

Everything else — the CSP wrapping, unsafe-source drop, echo, and the three error branches (onResourceError present/omitted, no-text, non-Error rejection) — is well covered. LGTM once you decide on the nit.
· v2/1566-per-app-csp

Add a test asserting the shared HOST_CAPABILITIES.sandbox stays undefined after a sandboxready run sets hostCapabilities.sandbox — guards against a silent regression if the per-app spread copy were ever dropped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw

cliffhall · 2026-07-11T22:03:12Z

Applied the nit in a follow-up commit: added does not mutate the shared HOST_CAPABILITIES when echoing the approved sandbox — imports HOST_CAPABILITIES and asserts .sandbox stays undefined after a sandboxready run, locking in the per-app-copy isolation. All 12 factory tests pass and npm run ci is green. Thanks for the reviews!

cliffhall added the v2 Issues and PRs for v2 label Jul 11, 2026

cliffhall linked an issue Jul 11, 2026 that may be closed by this pull request

web: enforce per-app _meta.ui.csp in the app bridge + surface resource-read failures #1566

Open

Merge branch 'v2/1565-sandbox-hardening' into v2/1566-per-app-csp

8437a1c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

web: enforce per-app _meta.ui.csp + surface resource-read failures#1652

web: enforce per-app _meta.ui.csp + surface resource-read failures#1652
cliffhall wants to merge 4 commits into
v2/1565-sandbox-hardeningfrom
v2/1566-per-app-csp

cliffhall commented Jul 11, 2026

Uh oh!

cliffhall commented Jul 11, 2026

Uh oh!

claude Bot commented Jul 11, 2026 •

edited

Loading

Uh oh!

cliffhall commented Jul 11, 2026

Uh oh!

cliffhall commented Jul 11, 2026

Uh oh!

claude Bot commented Jul 11, 2026 •

edited

Loading

Uh oh!

cliffhall commented Jul 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

cliffhall commented Jul 11, 2026

Changes

Tests

Stacking

Uh oh!

cliffhall commented Jul 11, 2026

Uh oh!

claude Bot commented Jul 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review: per-app CSP enforcement + resource-error surfacing

1. The sandbox echo depends on AppBridge reading hostCapabilities lazily (verify)

2. Permissions are echoed/forwarded unfiltered while CSP is filtered

3. Minor: onResourceError is defined but not yet wired

Nits

Uh oh!

cliffhall commented Jul 11, 2026

Uh oh!

cliffhall commented Jul 11, 2026

Uh oh!

claude Bot commented Jul 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Re-review: per-app CSP enforcement + resource-error surfacing

One nit — the per-app-copy isolation isn't locked in by a test

Uh oh!

cliffhall commented Jul 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

claude Bot commented Jul 11, 2026 •

edited

Loading

1. The sandbox echo depends on `AppBridge` reading `hostCapabilities` lazily (verify)

3. Minor: `onResourceError` is defined but not yet wired

claude Bot commented Jul 11, 2026 •

edited

Loading