From 45785a0aa60e99e893de8264c5df01df3db0226b Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 21:22:14 -0400 Subject: [PATCH] fix(web): screen app-supplied sandbox permissions before granting them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sandbox proxy's buildAllowAttribute() tests each permission key for truthiness, so an untrusted `_meta.ui.permissions` bag could stand a non-boolean in for a grant — `{ camera: "false" }`, a plausible way for a server to mean "off", reads as on. Nothing screened it, because before the `_meta.ui` fix no permissions reached the proxy at all. Add approveSandboxPermissions alongside approveCspSources: keep only the four keys the proxy knows, and only where the value is the spec's empty- object marker or `true`; drop anything else (warning on a malformed value, silently on an explicit `false`) and return undefined when nothing survives. Both the notification and the hostCapabilities echo now carry the approved set, so the app sees what was granted. Also document what the listing lookup can and cannot see under `paginatedLists`. Signed-off-by: cliffhall --- clients/web/src/App.tsx | 7 ++ .../AppRenderer/createAppBridgeFactory.ts | 27 ++++---- clients/web/src/utils/sandbox-csp.test.ts | 48 ++++++++++++++ clients/web/src/utils/sandbox-csp.ts | 64 ++++++++++++++++++- 4 files changed, 133 insertions(+), 13 deletions(-) diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 613ad758..59201890 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -1254,6 +1254,13 @@ function App() { const tools = toolsPagination.items; const prompts = promptsPagination.items; const resources = resourcesPagination.items; + // Whatever the resource list currently holds — every page in the default + // aggregate mode, only the pages fetched so far under `paginatedLists`. The + // listing is a documented *default* that a read content item overrides, and + // an app whose entry sits on an unfetched page simply falls back to no hints + // (`connect-src 'none'`), exactly as before this wiring existed. Walking the + // whole list to close that would issue the very requests the user opted out + // of by turning pagination on, so the setting wins. listedResourcesRef.current = resources; const { tasks, diff --git a/clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.ts b/clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.ts index 6363d14e..2480458d 100644 --- a/clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.ts +++ b/clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.ts @@ -17,6 +17,7 @@ import type { } from "@modelcontextprotocol/client"; import { approveCspSources, + approveSandboxPermissions, buildSandboxCspPolicy, wrapSandboxedHtml, } from "../../../utils/sandbox-csp"; @@ -324,23 +325,25 @@ export function createAppBridgeFactory( // sendSandboxResourceReady: the view only sends ui/initialize once it // has the HTML, so the bridge reflects this in the initialize result. const approvedCsp = approveCspSources(meta?.csp); - // NOTE on the CSP-vs-permissions asymmetry: `csp` is injection-filtered - // by approveCspSources because its sources are interpolated into the - // CSP content string. `permissions` is NOT filtered here — it is - // a structured object (camera/microphone/geolocation/clipboardWrite - // booleans), and its 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 - // therefore can't reach the iframe `sandbox`/`allow` attribute as raw - // text (that layer, and the allow-same-origin strip, is owned by the - // sandbox-hardening work in #1565), so no source-style allowlist applies. + // Both halves of the app-supplied sandbox config are screened, for + // different reasons. `csp` is injection-filtered by approveCspSources + // because its sources are interpolated into the CSP content + // string. `permissions` can't reach the `sandbox`/`allow` attribute as + // raw text — the proxy maps each known key to a fixed Permissions-Policy + // token — but it tests those keys for TRUTHINESS, so a non-boolean + // (`camera: "false"`) would read as a grant; approveSandboxPermissions + // reduces the bag to the keys actually requested. The `allow-same-origin` + // strip and the rest of that layer stay owned by #1565. + const approvedPermissions = approveSandboxPermissions( + meta?.permissions, + ); hostCapabilities.sandbox = { - permissions: meta?.permissions, + permissions: approvedPermissions, csp: approvedCsp, }; await bridge.sendSandboxResourceReady({ html: wrapSandboxedHtml(html, buildSandboxCspPolicy(approvedCsp)), - permissions: meta?.permissions, + permissions: approvedPermissions, }); } catch (err) { const error = err instanceof Error ? err : new Error(String(err)); diff --git a/clients/web/src/utils/sandbox-csp.test.ts b/clients/web/src/utils/sandbox-csp.test.ts index de7d6352..899ebd08 100644 --- a/clients/web/src/utils/sandbox-csp.test.ts +++ b/clients/web/src/utils/sandbox-csp.test.ts @@ -2,10 +2,12 @@ import { describe, expect, it, vi } from "vitest"; import { SAFE_CSP_SOURCE, approveCspSources, + approveSandboxPermissions, buildSandboxCspPolicy, escapeHtmlAttr, wrapSandboxedHtml, } from "./sandbox-csp"; +import type { McpUiResourcePermissions } from "@modelcontextprotocol/ext-apps/app-bridge"; describe("SAFE_CSP_SOURCE", () => { it.each([ @@ -81,6 +83,52 @@ describe("approveCspSources", () => { }); }); +describe("approveSandboxPermissions", () => { + it("keeps the spec's empty-object marker and `true` as shorthand", () => { + expect( + approveSandboxPermissions({ + camera: {}, + microphone: true as unknown as Record, + }), + ).toEqual({ camera: {}, microphone: {} }); + }); + + it("drops a truthy non-marker value the proxy would have honored", () => { + // The proxy's buildAllowAttribute() tests each key for truthiness, so the + // string "false" — a plausible way for a server to mean "off" — would + // otherwise switch the camera on. + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + expect( + approveSandboxPermissions({ + camera: "false", + geolocation: 1, + microphone: null, + } as unknown as McpUiResourcePermissions), + ).toBeUndefined(); + expect(warn).toHaveBeenCalledTimes(3); + warn.mockRestore(); + }); + + it("drops `false` without warning, and ignores unknown keys", () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + expect( + approveSandboxPermissions({ + camera: false, + midi: {}, + clipboardWrite: {}, + } as unknown as McpUiResourcePermissions), + ).toEqual({ clipboardWrite: {} }); + // `false` is an explicit refusal, not a malformed value. + expect(warn).not.toHaveBeenCalled(); + warn.mockRestore(); + }); + + it("returns undefined for no permissions at all", () => { + expect(approveSandboxPermissions(undefined)).toBeUndefined(); + expect(approveSandboxPermissions({})).toBeUndefined(); + }); +}); + describe("buildSandboxCspPolicy", () => { it("emits locked-down defaults for an empty approved csp", () => { const policy = buildSandboxCspPolicy({}); diff --git a/clients/web/src/utils/sandbox-csp.ts b/clients/web/src/utils/sandbox-csp.ts index b3d44004..b3283335 100644 --- a/clients/web/src/utils/sandbox-csp.ts +++ b/clients/web/src/utils/sandbox-csp.ts @@ -1,4 +1,7 @@ -import type { McpUiResourceCsp } from "@modelcontextprotocol/ext-apps/app-bridge"; +import type { + McpUiResourceCsp, + McpUiResourcePermissions, +} from "@modelcontextprotocol/ext-apps/app-bridge"; /** * Allowed shapes for a CSP source-expression supplied by an app's @@ -75,6 +78,65 @@ export function approveCspSources( return approved; } +/** The permission-key analog of {@link exhaustiveCspKeys}; same compile-time completeness proof. */ +function exhaustivePermissionKeys< + const T extends readonly (keyof McpUiResourcePermissions)[], +>( + keys: T & + (keyof McpUiResourcePermissions extends T[number] ? unknown : never), +): T { + return keys; +} + +const PERMISSION_KEYS = exhaustivePermissionKeys([ + "camera", + "microphone", + "geolocation", + "clipboardWrite", +]); + +/** + * Filter an app-supplied {@link McpUiResourcePermissions} down to the keys the + * host will forward to the sandbox proxy, keeping only those the app actually + * *requested*. + * + * The proxy's `buildAllowAttribute()` tests each key for **truthiness**, so an + * unvalidated bag lets a non-boolean stand in for a grant: `{ camera: "false" }` + * — a plausible way for a server to mean "off" — is truthy and would switch the + * `allow` attribute on. `_meta` is untrusted input, so the value is screened + * here rather than at the point of use. A key counts as requested when it + * carries the spec's empty-object marker (`camera: {}`), or `true` as the + * obvious shorthand; everything else — `false`, any string, `null`, a number — + * is dropped, as is any key outside the four the proxy knows. + * + * Returns undefined when nothing survives, so the notification and the + * `hostCapabilities.sandbox` echo carry no permissions at all rather than an + * empty object. + */ +export function approveSandboxPermissions( + permissions: McpUiResourcePermissions | undefined, +): McpUiResourcePermissions | undefined { + if (!permissions) return undefined; + const approved: McpUiResourcePermissions = {}; + let granted = false; + for (const key of PERMISSION_KEYS) { + const requested: unknown = permissions[key]; + if ( + requested === true || + (typeof requested === "object" && requested !== null) + ) { + approved[key] = {}; + granted = true; + } else if (requested !== undefined && requested !== false) { + console.warn( + `[mcp-app sandbox] dropping unrecognized "${key}" permission value:`, + requested, + ); + } + } + return granted ? approved : undefined; +} + function joinSources(list: string[] | undefined, fallback: string): string { return list && list.length > 0 ? list.join(" ") : fallback; }