fix(web): screen app-supplied sandbox permissions before granting them
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 <cliff@futurescale.com>
This commit is contained in:
parent
9f8dc82a9a
commit
45785a0aa6
4 changed files with 133 additions and 13 deletions
|
|
@ -1254,6 +1254,13 @@ function App() {
|
||||||
const tools = toolsPagination.items;
|
const tools = toolsPagination.items;
|
||||||
const prompts = promptsPagination.items;
|
const prompts = promptsPagination.items;
|
||||||
const resources = resourcesPagination.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;
|
listedResourcesRef.current = resources;
|
||||||
const {
|
const {
|
||||||
tasks,
|
tasks,
|
||||||
|
|
|
||||||
|
|
@ -17,6 +17,7 @@ import type {
|
||||||
} from "@modelcontextprotocol/client";
|
} from "@modelcontextprotocol/client";
|
||||||
import {
|
import {
|
||||||
approveCspSources,
|
approveCspSources,
|
||||||
|
approveSandboxPermissions,
|
||||||
buildSandboxCspPolicy,
|
buildSandboxCspPolicy,
|
||||||
wrapSandboxedHtml,
|
wrapSandboxedHtml,
|
||||||
} from "../../../utils/sandbox-csp";
|
} from "../../../utils/sandbox-csp";
|
||||||
|
|
@ -324,23 +325,25 @@ export function createAppBridgeFactory(
|
||||||
// sendSandboxResourceReady: the view only sends ui/initialize once it
|
// sendSandboxResourceReady: the view only sends ui/initialize once it
|
||||||
// has the HTML, so the bridge reflects this in the initialize result.
|
// has the HTML, so the bridge reflects this in the initialize result.
|
||||||
const approvedCsp = approveCspSources(meta?.csp);
|
const approvedCsp = approveCspSources(meta?.csp);
|
||||||
// NOTE on the CSP-vs-permissions asymmetry: `csp` is injection-filtered
|
// Both halves of the app-supplied sandbox config are screened, for
|
||||||
// by approveCspSources because its sources are interpolated into the
|
// different reasons. `csp` is injection-filtered by approveCspSources
|
||||||
// CSP <meta> content string. `permissions` is NOT filtered here — it is
|
// because its sources are interpolated into the CSP <meta> content
|
||||||
// a structured object (camera/microphone/geolocation/clipboardWrite
|
// string. `permissions` can't reach the `sandbox`/`allow` attribute as
|
||||||
// booleans), and its only consumer is the sandbox proxy's
|
// raw text — the proxy maps each known key to a fixed Permissions-Policy
|
||||||
// buildAllowAttribute(), which maps each known key to a fixed
|
// token — but it tests those keys for TRUTHINESS, so a non-boolean
|
||||||
// Permissions-Policy token and ignores anything else. Untrusted values
|
// (`camera: "false"`) would read as a grant; approveSandboxPermissions
|
||||||
// therefore can't reach the iframe `sandbox`/`allow` attribute as raw
|
// reduces the bag to the keys actually requested. The `allow-same-origin`
|
||||||
// text (that layer, and the allow-same-origin strip, is owned by the
|
// strip and the rest of that layer stay owned by #1565.
|
||||||
// sandbox-hardening work in #1565), so no source-style allowlist applies.
|
const approvedPermissions = approveSandboxPermissions(
|
||||||
|
meta?.permissions,
|
||||||
|
);
|
||||||
hostCapabilities.sandbox = {
|
hostCapabilities.sandbox = {
|
||||||
permissions: meta?.permissions,
|
permissions: approvedPermissions,
|
||||||
csp: approvedCsp,
|
csp: approvedCsp,
|
||||||
};
|
};
|
||||||
await bridge.sendSandboxResourceReady({
|
await bridge.sendSandboxResourceReady({
|
||||||
html: wrapSandboxedHtml(html, buildSandboxCspPolicy(approvedCsp)),
|
html: wrapSandboxedHtml(html, buildSandboxCspPolicy(approvedCsp)),
|
||||||
permissions: meta?.permissions,
|
permissions: approvedPermissions,
|
||||||
});
|
});
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
const error = err instanceof Error ? err : new Error(String(err));
|
const error = err instanceof Error ? err : new Error(String(err));
|
||||||
|
|
|
||||||
|
|
@ -2,10 +2,12 @@ import { describe, expect, it, vi } from "vitest";
|
||||||
import {
|
import {
|
||||||
SAFE_CSP_SOURCE,
|
SAFE_CSP_SOURCE,
|
||||||
approveCspSources,
|
approveCspSources,
|
||||||
|
approveSandboxPermissions,
|
||||||
buildSandboxCspPolicy,
|
buildSandboxCspPolicy,
|
||||||
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([
|
||||||
|
|
@ -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<string, never>,
|
||||||
|
}),
|
||||||
|
).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", () => {
|
describe("buildSandboxCspPolicy", () => {
|
||||||
it("emits locked-down defaults for an empty approved csp", () => {
|
it("emits locked-down defaults for an empty approved csp", () => {
|
||||||
const policy = buildSandboxCspPolicy({});
|
const policy = buildSandboxCspPolicy({});
|
||||||
|
|
|
||||||
|
|
@ -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
|
* Allowed shapes for a CSP source-expression supplied by an app's
|
||||||
|
|
@ -75,6 +78,65 @@ export function approveCspSources(
|
||||||
return approved;
|
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 {
|
function joinSources(list: string[] | undefined, fallback: string): string {
|
||||||
return list && list.length > 0 ? list.join(" ") : fallback;
|
return list && list.length > 0 ? list.join(" ") : fallback;
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue