From 286dd86f6b0294b7fafb0c10b347b1efb7380fc6 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 21:36:06 -0400 Subject: [PATCH] refactor(web): type the permissions approver as the boundary it is MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit approveSandboxPermissions screens untrusted `_meta`, so it takes `unknown` and decides the shape itself rather than claiming its input is already an McpUiResourcePermissions — which had forced its own tests to cast malformed fixtures through `as unknown as`. Also stop honoring `true` as a permission marker. The spec defines every permission as an object (`camera: {}`), so `true` was an undocumented shorthand, and honoring it means a server typo turns an iframe permission on. Only the object marker grants now; arrays are rejected too. Signed-off-by: cliffhall --- clients/web/src/utils/sandbox-csp.test.ts | 32 +++++++++---------- clients/web/src/utils/sandbox-csp.ts | 38 ++++++++++++++--------- 2 files changed, 39 insertions(+), 31 deletions(-) diff --git a/clients/web/src/utils/sandbox-csp.test.ts b/clients/web/src/utils/sandbox-csp.test.ts index 899ebd08..c6d00ebd 100644 --- a/clients/web/src/utils/sandbox-csp.test.ts +++ b/clients/web/src/utils/sandbox-csp.test.ts @@ -7,7 +7,6 @@ import { escapeHtmlAttr, wrapSandboxedHtml, } from "./sandbox-csp"; -import type { McpUiResourcePermissions } from "@modelcontextprotocol/ext-apps/app-bridge"; describe("SAFE_CSP_SOURCE", () => { it.each([ @@ -84,28 +83,27 @@ describe("approveCspSources", () => { }); describe("approveSandboxPermissions", () => { - it("keeps the spec's empty-object marker and `true` as shorthand", () => { + it("keeps the spec's empty-object marker for each known key", () => { expect( - approveSandboxPermissions({ - camera: {}, - microphone: true as unknown as Record, - }), - ).toEqual({ camera: {}, microphone: {} }); + approveSandboxPermissions({ camera: {}, clipboardWrite: {} }), + ).toEqual({ camera: {}, clipboardWrite: {} }); }); - 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. + it("drops a truthy non-marker the proxy would have honored", () => { + // 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. `true` is rejected too: it is not a marker the + // spec defines, and guessing in the granting direction is the wrong bet. const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); expect( approveSandboxPermissions({ camera: "false", + microphone: true, geolocation: 1, - microphone: null, - } as unknown as McpUiResourcePermissions), + clipboardWrite: [], + }), ).toBeUndefined(); - expect(warn).toHaveBeenCalledTimes(3); + expect(warn).toHaveBeenCalledTimes(4); warn.mockRestore(); }); @@ -116,15 +114,17 @@ describe("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", () => { + it("returns undefined when there is nothing to grant", () => { expect(approveSandboxPermissions(undefined)).toBeUndefined(); + expect(approveSandboxPermissions(null)).toBeUndefined(); + expect(approveSandboxPermissions("camera")).toBeUndefined(); expect(approveSandboxPermissions({})).toBeUndefined(); }); }); diff --git a/clients/web/src/utils/sandbox-csp.ts b/clients/web/src/utils/sandbox-csp.ts index b3283335..c15ac8be 100644 --- a/clients/web/src/utils/sandbox-csp.ts +++ b/clients/web/src/utils/sandbox-csp.ts @@ -96,38 +96,46 @@ const PERMISSION_KEYS = exhaustivePermissionKeys([ ]); /** - * 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*. + * Filter an app-supplied permissions bag 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" }` + * unvalidated bag lets a non-marker 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. + * `allow` attribute on. `_meta` is untrusted input of no declared type, hence + * the `unknown` parameter: this is the boundary that decides what the shape is, + * not a consumer of an already-validated one. + * + * A key counts as requested only when it carries the spec's empty-object marker + * (`camera: {}` — {@link McpUiResourcePermissions} defines every permission that + * way). Everything else fails closed: `true`, `false`, any string, `null`, a + * number, an array, and any key outside the four the proxy knows. `true` in + * particular looks like a harmless shorthand, but honoring an undocumented one + * means a server typo turns an iframe permission ON, which is the wrong + * direction to guess in. * * 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, + permissions: unknown, ): McpUiResourcePermissions | undefined { - if (!permissions) return undefined; + if (typeof permissions !== "object" || permissions === null) return undefined; + const bag: Record = { ...permissions }; const approved: McpUiResourcePermissions = {}; let granted = false; for (const key of PERMISSION_KEYS) { - const requested: unknown = permissions[key]; + const requested = bag[key]; + if (requested === undefined || requested === false) continue; if ( - requested === true || - (typeof requested === "object" && requested !== null) + typeof requested === "object" && + requested !== null && + !Array.isArray(requested) ) { approved[key] = {}; granted = true; - } else if (requested !== undefined && requested !== false) { + } else { console.warn( `[mcp-app sandbox] dropping unrecognized "${key}" permission value:`, requested,