From 53943c2ef8d9dad195e5fe7e053a1f00fdee0556 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 22:05:23 -0400 Subject: [PATCH] fix(web): let a malformed _meta.ui stop the carrier lookup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit uiMeta returned undefined for both "this bag declares no ui" and "its ui is unusable", so a content item declaring `ui: null` fell through and was granted whatever a BROADER carrier asked for — wrong precedence, wrong direction to fail in. It now distinguishes the two: no `ui` key means keep looking, a declared-but-malformed one decides for "no hints". Also pin the full three-way precedence with a case carrying all three carriers at once (no prior case would have caught the first two operands being swapped), and state why a permission's object value need not be empty: `{}` is the spec's extension point, so rejecting a populated one would drop grants from servers written against a newer spec. Signed-off-by: cliffhall --- .../createAppBridgeFactory.test.ts | 61 +++++++++++++++++++ .../AppRenderer/createAppBridgeFactory.ts | 54 +++++++++------- clients/web/src/utils/sandbox-csp.test.ts | 9 +++ clients/web/src/utils/sandbox-csp.ts | 22 ++++--- 4 files changed, 117 insertions(+), 29 deletions(-) diff --git a/clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.test.ts b/clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.test.ts index 35eeb1cb..1e97877d 100644 --- a/clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.test.ts +++ b/clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.test.ts @@ -392,6 +392,67 @@ describe("createAppBridgeFactory", () => { expect(call.html).toContain("connect-src 'none'"); }); + it("prefers the content item's _meta.ui over both broader carriers", async () => { + // Pins the full three-way order — content item > result envelope > listing + // — which no single-fallback case can: swapping the first two operands + // would still satisfy them. + const result = uiResource("

x

", { + csp: { connectDomains: ["https://content.example.com"] }, + }); + (result as { _meta?: unknown })._meta = { + ui: { csp: { connectDomains: ["https://envelope.example.com"] } }, + }; + const factory = createAppBridgeFactory({ + getClient: () => fakeClient, + readResource: vi.fn().mockResolvedValue(result), + getListedResourceMeta: () => + listedMeta({ csp: { connectDomains: ["https://listed.example.com"] } }), + }); + await factory(makeIframe(), { kind: "tool", tool }); + const bridge = bridgeInstances[0]; + bridge.emit("sandboxready"); + await flush(); + + const call = bridge.sendSandboxResourceReady.mock.calls[0][0] as { + html: string; + }; + expect(call.html).toContain("connect-src https://content.example.com"); + expect(call.html).not.toContain("envelope.example.com"); + expect(call.html).not.toContain("listed.example.com"); + }); + + it("stops at a carrier whose _meta.ui is malformed rather than falling through", async () => { + // A declared-but-unusable `ui` is that carrier's answer. Falling through + // would grant whatever a BROADER carrier asked for — wrong precedence, and + // the wrong direction to fail in. + const readResource = vi + .fn() + .mockResolvedValue( + uiResource("

x

", { ui: null }, { flat: true }), + ); + const factory = createAppBridgeFactory({ + getClient: () => fakeClient, + readResource, + getListedResourceMeta: () => + listedMeta({ + permissions: { camera: {} }, + csp: { connectDomains: ["https://listed.example.com"] }, + }), + }); + await factory(makeIframe(), { kind: "tool", tool }); + const bridge = bridgeInstances[0]; + bridge.emit("sandboxready"); + await flush(); + + const call = bridge.sendSandboxResourceReady.mock.calls[0][0] as { + html: string; + permissions: unknown; + }; + expect(call.permissions).toBeUndefined(); + expect(call.html).not.toContain("listed.example.com"); + expect(call.html).toContain("connect-src 'none'"); + }); + it("ignores sandbox metadata written unnested on _meta", async () => { // `McpUiResourceMeta` describes the value of `_meta.ui`, not `_meta`. A bag // whose keys sit at the top level is not the spec shape, so it must not be diff --git a/clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.ts b/clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.ts index e088fee9..e5d3215d 100644 --- a/clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.ts +++ b/clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.ts @@ -111,21 +111,31 @@ export interface AppBridgeFactoryDeps { } /** - * The MCP Apps sandbox hints (`csp`, `permissions`, …) nested under a `_meta` - * bag's `ui` key. `McpUiResourceMeta` describes the value of `_meta.ui`, NOT - * `_meta` itself — reading `_meta` directly leaves every field `undefined`, so - * a spec-conforming app's declared `connectDomains` never reach the generated - * CSP and it renders under `connect-src 'none'` (#2055). This mirrors the tool - * side, where `getToolUiResourceUri` already reads `_meta.ui.resourceUri`. + * The MCP Apps sandbox hints (`csp`, `permissions`, …) a single `_meta` bag + * declares under its `ui` key, or undefined when that bag declares none. + * + * The two "no hints" cases are deliberately distinguished by the return type: + * a bag with no `ui` key at all returns undefined (this carrier is silent — + * keep looking), while a bag whose `ui` is malformed returns `{ meta: + * undefined }` (this carrier spoke, and what it said is unusable — stop). Were + * they conflated, a content item declaring `ui: null` would fall through to a + * broader carrier and be granted whatever *that* one asked for, which is both + * the wrong precedence and the wrong direction to fail in. + * + * `McpUiResourceMeta` describes the value of `_meta.ui`, NOT `_meta` itself — + * reading `_meta` directly leaves every field undefined, so a spec-conforming + * app's declared `connectDomains` never reach the generated CSP and it renders + * under `connect-src 'none'` (#2055). This mirrors the tool side, where + * `getToolUiResourceUri` already reads `_meta.ui.resourceUri`. */ -function uiMeta(meta: object | undefined): McpUiResourceMeta | undefined { - // `_meta` is typed with an index signature, which has no properties in common - // with a `{ ui?: unknown }` parameter — hence the cast rather than a narrower - // parameter type. Reading one key off an untyped bag is safe. - const ui = (meta as { ui?: unknown } | undefined)?.ui; - return ui !== null && typeof ui === "object" - ? (ui as McpUiResourceMeta) - : undefined; +function uiMeta( + bag: object | undefined, +): { meta: McpUiResourceMeta | undefined } | undefined { + if (bag === undefined || !("ui" in bag)) return undefined; + const ui: unknown = (bag as { ui: unknown }).ui; + const wellFormed = + ui !== null && typeof ui === "object" && !Array.isArray(ui); + return { meta: wellFormed ? (ui as McpUiResourceMeta) : undefined }; } /** @@ -140,9 +150,11 @@ function uiMeta(meta: object | undefined): McpUiResourceMeta | undefined { * documents as the static default a host reviews at connection time, and * which a read content item explicitly takes precedence over. * - * Whichever is found first wins outright — the levels are not merged, since a - * server that restates only `csp` at the more specific level means that to be - * the whole grant, not a patch over the broader one. + * The first carrier that *declares* a `ui` key decides, and its value is used + * outright — the levels are not merged, since a server that restates only + * `csp` at the more specific level means that to be the whole grant, not a + * patch over the broader one. A carrier declaring a malformed `ui` decides + * too, for "no hints"; see {@link uiMeta}. */ function extractHtmlAndMeta( result: ReadResourceResult, @@ -154,11 +166,9 @@ function extractHtmlAndMeta( for (const content of result.contents) { const text = (content as { text?: unknown }).text; if (typeof text === "string") { - return { - html: text, - meta: - uiMeta(content._meta) ?? uiMeta(result._meta) ?? uiMeta(listedMeta), - }; + const carried = + uiMeta(content._meta) ?? uiMeta(result._meta) ?? uiMeta(listedMeta); + return { html: text, meta: carried?.meta }; } } throw new Error("UI resource has no text (HTML) content"); diff --git a/clients/web/src/utils/sandbox-csp.test.ts b/clients/web/src/utils/sandbox-csp.test.ts index c6d00ebd..95b2de94 100644 --- a/clients/web/src/utils/sandbox-csp.test.ts +++ b/clients/web/src/utils/sandbox-csp.test.ts @@ -107,6 +107,15 @@ describe("approveSandboxPermissions", () => { warn.mockRestore(); }); + it("accepts a non-empty object, since `{}` is the spec's extension point", () => { + // The key's presence is the request; a field the host doesn't know can't + // narrow it, and rejecting it would drop grants from servers written + // against a newer spec. The value is normalized to `{}` on the way out. + expect(approveSandboxPermissions({ camera: { enabled: false } })).toEqual({ + camera: {}, + }); + }); + it("drops `false` without warning, and ignores unknown keys", () => { const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); expect( diff --git a/clients/web/src/utils/sandbox-csp.ts b/clients/web/src/utils/sandbox-csp.ts index c15ac8be..83c38845 100644 --- a/clients/web/src/utils/sandbox-csp.ts +++ b/clients/web/src/utils/sandbox-csp.ts @@ -106,13 +106,21 @@ const PERMISSION_KEYS = exhaustivePermissionKeys([ * 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. + * A key counts as requested only when its value is an **object** — the marker + * {@link McpUiResourcePermissions} defines for every permission is `{}`, and + * presence of that object is the request. 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. + * + * The object is not required to be *empty*. `{}` is a type with no fields yet, + * not a promise there will never be any — it is precisely the spec's extension + * point, so rejecting `{ camera: { … } }` would fail closed the day a + * permission gains an option, silently dropping grants from servers written + * against the newer spec. The value is normalized to `{}` on the way out + * either way, so an unrecognized field can't reach the proxy; what it cannot + * do is *narrow* a grant the key's presence already asked for. * * Returns undefined when nothing survives, so the notification and the * `hostCapabilities.sandbox` echo carry no permissions at all rather than an