fix(web): let a malformed _meta.ui stop the carrier lookup
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 <cliff@futurescale.com>
This commit is contained in:
parent
e2a88d49dd
commit
53943c2ef8
4 changed files with 117 additions and 29 deletions
|
|
@ -392,6 +392,67 @@ describe("createAppBridgeFactory", () => {
|
||||||
expect(call.html).toContain("connect-src 'none'");
|
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("<h1>x</h1>", {
|
||||||
|
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("<h1>x</h1>", { 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 () => {
|
it("ignores sandbox metadata written unnested on _meta", async () => {
|
||||||
// `McpUiResourceMeta` describes the value of `_meta.ui`, not `_meta`. A bag
|
// `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
|
// whose keys sit at the top level is not the spec shape, so it must not be
|
||||||
|
|
|
||||||
|
|
@ -111,21 +111,31 @@ export interface AppBridgeFactoryDeps {
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* The MCP Apps sandbox hints (`csp`, `permissions`, …) nested under a `_meta`
|
* The MCP Apps sandbox hints (`csp`, `permissions`, …) a single `_meta` bag
|
||||||
* bag's `ui` key. `McpUiResourceMeta` describes the value of `_meta.ui`, NOT
|
* declares under its `ui` key, or undefined when that bag declares none.
|
||||||
* `_meta` itself — reading `_meta` directly leaves every field `undefined`, so
|
*
|
||||||
* a spec-conforming app's declared `connectDomains` never reach the generated
|
* The two "no hints" cases are deliberately distinguished by the return type:
|
||||||
* CSP and it renders under `connect-src 'none'` (#2055). This mirrors the tool
|
* a bag with no `ui` key at all returns undefined (this carrier is silent —
|
||||||
* side, where `getToolUiResourceUri` already reads `_meta.ui.resourceUri`.
|
* 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 {
|
function uiMeta(
|
||||||
// `_meta` is typed with an index signature, which has no properties in common
|
bag: object | undefined,
|
||||||
// with a `{ ui?: unknown }` parameter — hence the cast rather than a narrower
|
): { meta: McpUiResourceMeta | undefined } | undefined {
|
||||||
// parameter type. Reading one key off an untyped bag is safe.
|
if (bag === undefined || !("ui" in bag)) return undefined;
|
||||||
const ui = (meta as { ui?: unknown } | undefined)?.ui;
|
const ui: unknown = (bag as { ui: unknown }).ui;
|
||||||
return ui !== null && typeof ui === "object"
|
const wellFormed =
|
||||||
? (ui as McpUiResourceMeta)
|
ui !== null && typeof ui === "object" && !Array.isArray(ui);
|
||||||
: undefined;
|
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
|
* documents as the static default a host reviews at connection time, and
|
||||||
* which a read content item explicitly takes precedence over.
|
* which a read content item explicitly takes precedence over.
|
||||||
*
|
*
|
||||||
* Whichever is found first wins outright — the levels are not merged, since a
|
* The first carrier that *declares* a `ui` key decides, and its value is used
|
||||||
* server that restates only `csp` at the more specific level means that to be
|
* outright — the levels are not merged, since a server that restates only
|
||||||
* the whole grant, not a patch over the broader one.
|
* `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(
|
function extractHtmlAndMeta(
|
||||||
result: ReadResourceResult,
|
result: ReadResourceResult,
|
||||||
|
|
@ -154,11 +166,9 @@ function extractHtmlAndMeta(
|
||||||
for (const content of result.contents) {
|
for (const content of result.contents) {
|
||||||
const text = (content as { text?: unknown }).text;
|
const text = (content as { text?: unknown }).text;
|
||||||
if (typeof text === "string") {
|
if (typeof text === "string") {
|
||||||
return {
|
const carried =
|
||||||
html: text,
|
uiMeta(content._meta) ?? uiMeta(result._meta) ?? uiMeta(listedMeta);
|
||||||
meta:
|
return { html: text, meta: carried?.meta };
|
||||||
uiMeta(content._meta) ?? uiMeta(result._meta) ?? uiMeta(listedMeta),
|
|
||||||
};
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
throw new Error("UI resource has no text (HTML) content");
|
throw new Error("UI resource has no text (HTML) content");
|
||||||
|
|
|
||||||
|
|
@ -107,6 +107,15 @@ describe("approveSandboxPermissions", () => {
|
||||||
warn.mockRestore();
|
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", () => {
|
it("drops `false` without warning, and ignores unknown keys", () => {
|
||||||
const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
|
const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||||
expect(
|
expect(
|
||||||
|
|
|
||||||
|
|
@ -106,13 +106,21 @@ const PERMISSION_KEYS = exhaustivePermissionKeys([
|
||||||
* the `unknown` parameter: this is the boundary that decides what the shape is,
|
* the `unknown` parameter: this is the boundary that decides what the shape is,
|
||||||
* not a consumer of an already-validated one.
|
* not a consumer of an already-validated one.
|
||||||
*
|
*
|
||||||
* A key counts as requested only when it carries the spec's empty-object marker
|
* A key counts as requested only when its value is an **object** — the marker
|
||||||
* (`camera: {}` — {@link McpUiResourcePermissions} defines every permission that
|
* {@link McpUiResourcePermissions} defines for every permission is `{}`, and
|
||||||
* way). Everything else fails closed: `true`, `false`, any string, `null`, a
|
* presence of that object is the request. Everything else fails closed:
|
||||||
* number, an array, and any key outside the four the proxy knows. `true` in
|
* `true`, `false`, any string, `null`, a number, an array, and any key outside
|
||||||
* particular looks like a harmless shorthand, but honoring an undocumented one
|
* the four the proxy knows. `true` in particular looks like a harmless
|
||||||
* means a server typo turns an iframe permission ON, which is the wrong
|
* shorthand, but honoring an undocumented one means a server typo turns an
|
||||||
* direction to guess in.
|
* 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
|
* 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
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue