refactor(web): type the permissions approver as the boundary it is

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 <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-23 21:36:06 -04:00
commit 286dd86f6b
2 changed files with 39 additions and 31 deletions

View file

@ -7,7 +7,6 @@ import {
escapeHtmlAttr, escapeHtmlAttr,
wrapSandboxedHtml, wrapSandboxedHtml,
} from "./sandbox-csp"; } from "./sandbox-csp";
import type { McpUiResourcePermissions } from "@modelcontextprotocol/ext-apps/app-bridge";
describe("SAFE_CSP_SOURCE", () => { describe("SAFE_CSP_SOURCE", () => {
it.each([ it.each([
@ -84,28 +83,27 @@ describe("approveCspSources", () => {
}); });
describe("approveSandboxPermissions", () => { 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( expect(
approveSandboxPermissions({ approveSandboxPermissions({ camera: {}, clipboardWrite: {} }),
camera: {}, ).toEqual({ camera: {}, clipboardWrite: {} });
microphone: true as unknown as Record<string, never>,
}),
).toEqual({ camera: {}, microphone: {} });
}); });
it("drops a truthy non-marker value the proxy would have honored", () => { it("drops a truthy non-marker the proxy would have honored", () => {
// The proxy's buildAllowAttribute() tests each key for truthiness, so the // buildAllowAttribute() tests each key for truthiness, so the string
// string "false" — a plausible way for a server to mean "off" — would // "false" — a plausible way for a server to mean "off" — would otherwise
// otherwise switch the camera on. // 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(() => {}); const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
expect( expect(
approveSandboxPermissions({ approveSandboxPermissions({
camera: "false", camera: "false",
microphone: true,
geolocation: 1, geolocation: 1,
microphone: null, clipboardWrite: [],
} as unknown as McpUiResourcePermissions), }),
).toBeUndefined(); ).toBeUndefined();
expect(warn).toHaveBeenCalledTimes(3); expect(warn).toHaveBeenCalledTimes(4);
warn.mockRestore(); warn.mockRestore();
}); });
@ -116,15 +114,17 @@ describe("approveSandboxPermissions", () => {
camera: false, camera: false,
midi: {}, midi: {},
clipboardWrite: {}, clipboardWrite: {},
} as unknown as McpUiResourcePermissions), }),
).toEqual({ clipboardWrite: {} }); ).toEqual({ clipboardWrite: {} });
// `false` is an explicit refusal, not a malformed value. // `false` is an explicit refusal, not a malformed value.
expect(warn).not.toHaveBeenCalled(); expect(warn).not.toHaveBeenCalled();
warn.mockRestore(); 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(undefined)).toBeUndefined();
expect(approveSandboxPermissions(null)).toBeUndefined();
expect(approveSandboxPermissions("camera")).toBeUndefined();
expect(approveSandboxPermissions({})).toBeUndefined(); expect(approveSandboxPermissions({})).toBeUndefined();
}); });
}); });

View file

@ -96,38 +96,46 @@ const PERMISSION_KEYS = exhaustivePermissionKeys([
]); ]);
/** /**
* Filter an app-supplied {@link McpUiResourcePermissions} down to the keys the * Filter an app-supplied permissions bag down to the keys the host will forward
* host will forward to the sandbox proxy, keeping only those the app actually * to the sandbox proxy, keeping only those the app actually *requested*.
* *requested*.
* *
* The proxy's `buildAllowAttribute()` tests each key for **truthiness**, so an * 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 * — 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 * `allow` attribute on. `_meta` is untrusted input of no declared type, hence
* here rather than at the point of use. A key counts as requested when it * the `unknown` parameter: this is the boundary that decides what the shape is,
* carries the spec's empty-object marker (`camera: {}`), or `true` as the * not a consumer of an already-validated one.
* obvious shorthand; everything else — `false`, any string, `null`, a number — *
* is dropped, as is any key outside the four the proxy knows. * 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 * Returns undefined when nothing survives, so the notification and the
* `hostCapabilities.sandbox` echo carry no permissions at all rather than an * `hostCapabilities.sandbox` echo carry no permissions at all rather than an
* empty object. * empty object.
*/ */
export function approveSandboxPermissions( export function approveSandboxPermissions(
permissions: McpUiResourcePermissions | undefined, permissions: unknown,
): McpUiResourcePermissions | undefined { ): McpUiResourcePermissions | undefined {
if (!permissions) return undefined; if (typeof permissions !== "object" || permissions === null) return undefined;
const bag: Record<string, unknown> = { ...permissions };
const approved: McpUiResourcePermissions = {}; const approved: McpUiResourcePermissions = {};
let granted = false; let granted = false;
for (const key of PERMISSION_KEYS) { for (const key of PERMISSION_KEYS) {
const requested: unknown = permissions[key]; const requested = bag[key];
if (requested === undefined || requested === false) continue;
if ( if (
requested === true || typeof requested === "object" &&
(typeof requested === "object" && requested !== null) requested !== null &&
!Array.isArray(requested)
) { ) {
approved[key] = {}; approved[key] = {};
granted = true; granted = true;
} else if (requested !== undefined && requested !== false) { } else {
console.warn( console.warn(
`[mcp-app sandbox] dropping unrecognized "${key}" permission value:`, `[mcp-app sandbox] dropping unrecognized "${key}" permission value:`,
requested, requested,