fix(web): harden the app-origin paths and correct three docs
Copilot review round 1 on #2100. - POST /api/app-document: `null` and `[1,2]` are valid JSON, so the destructure that followed threw OUTSIDE the parse try/catch and turned a malformed body into a 500 instead of the documented 400. - app-origin controller: the error handler cleared `server` but not `origin`, and `publish` gates on `origin` — so an error after a successful listen kept minting URLs on a dead port and the browser never took the srcdoc fallback. Clear both, and drop the now-unfetchable documents. - sandbox proxy: drop a stale `srcdoc` before navigating to a dedicated origin (srcdoc wins over src), and restore the opaque-origin posture — strip `allow-same-origin`, reset `innerOrigin` — before assigning `srcdoc` on the fallback path. Not reachable today (each render remounts the iframe), but the cost of being wrong about that later is an isolation boundary. - publishAppDocument tests: replace two `as unknown as Response` casts with real `Response`s. - docs: `frame-ancestors` admits the proxy AND the Inspector page (every ancestor is checked); the shared helper falls back to loopback, not `'none'`; the showcase origin is `127.0.0.1`, the default bind address. - tests: null/array body, post-listen error, and both factories receiving a working `publishAppDocument`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
GitHub
parent
41c063b9ec
commit
fa441da85f
10 changed files with 188 additions and 20 deletions
|
|
@ -172,7 +172,7 @@ For the scripted version of the same flow (`--app-info` probe → deep link →
|
|||
|
||||
That field is how a server asks its host for a stable, dedicated origin. Without one, an App renders into a `srcdoc` frame sandboxed without `allow-same-origin`, so its document has an *opaque* origin and every request it makes carries `Origin: null` — which no CORS policy, OAuth callback, or API-key allowlist can admit ([#2056](https://github.com/modelcontextprotocol/inspector/issues/2056)).
|
||||
|
||||
Open the Apps tab and run `mcp_app_demo`. The widget renders identically to `mcp-app-http.json` — the difference is not visual. Inspect the inner iframe in devtools: on this server it is served from `http://localhost:6276/app-document/<id>` and `location.origin` is that real origin, where on `mcp-app-http.json` it is `about:srcdoc` with an origin of `null`.
|
||||
Open the Apps tab and run `mcp_app_demo`. The widget renders identically to `mcp-app-http.json` — the difference is not visual. Inspect the inner iframe in devtools: on this server it is served from `http://127.0.0.1:6276/app-document/<id>` and `location.origin` is that real origin, where on `mcp-app-http.json` it is `about:srcdoc` with an origin of `null`. (The host is whatever the Inspector bound to — `127.0.0.1` by default, an *address* rather than the name `localhost`, for the reason `resolve-bind-host.ts` documents.)
|
||||
|
||||
The spec makes `domain`'s format **host-dependent**, and the Inspector owns no domain infrastructure — so it reads any non-empty value as a *request* rather than an address, and answers with a real loopback origin of its own. See [MCP App dedicated origins](./clients/web/README.md#mcp-app-dedicated-origins-metauidomain) for the full contract, including what the one shared origin does and does not isolate, and how every failure falls back to the default render rather than blanking the app.
|
||||
|
||||
|
|
|
|||
|
|
@ -160,7 +160,11 @@ Inspector owns no domain infrastructure — it cannot serve
|
|||
- The app document is served from there under an unguessable path, its
|
||||
per-app CSP delivered as a real response **header** (stronger than the
|
||||
`<meta>` the `srcdoc` path relies on) plus a `frame-ancestors` that admits
|
||||
only the sandbox proxy.
|
||||
only the two origins in its real ancestor chain — the sandbox proxy that
|
||||
frames it, **and** the Inspector page that frames the proxy. Both are
|
||||
required, not belt-and-braces: `frame-ancestors` is checked against every
|
||||
ancestor, so omitting the Inspector's own origin blocks the frame outright
|
||||
(see `appDocumentEmbedders`).
|
||||
- The inner iframe is granted `allow-same-origin` on this path **only** —
|
||||
that is what makes the origin real rather than opaque. It is not a
|
||||
weakening of #1565: the listener is on its own port, so the app is
|
||||
|
|
|
|||
|
|
@ -273,8 +273,15 @@ export function createAppOriginController(
|
|||
console.error("App origin server error:", err);
|
||||
}
|
||||
// Degrade rather than reject: `publish` then returns null and the
|
||||
// renderer keeps working on the default srcdoc path.
|
||||
// renderer keeps working on the default srcdoc path. Clearing
|
||||
// `origin` is what makes that true — `publish` gates on it, not on
|
||||
// `server`, so an error arriving AFTER a successful listen would
|
||||
// otherwise keep minting URLs on a dead listener and the browser
|
||||
// would never take the promised fallback. Dropping the documents
|
||||
// with it releases their memory, since nothing can fetch them now.
|
||||
server = null;
|
||||
origin = null;
|
||||
documents.clear();
|
||||
settle({ port: 0, url: "" });
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -79,9 +79,11 @@ export function sandboxFrameAncestors(allowedOrigins?: string[]): string {
|
|||
* {@link LOOPBACK_FRAME_ANCESTORS} when nothing valid remains.
|
||||
*
|
||||
* Shared with the app-origin server (#2056), which restricts a published app
|
||||
* document to being framed by the sandbox proxy. Same grammar, same failure
|
||||
* mode — a directive whose only source is invalid degrades to `'none'` and
|
||||
* blocks the frame — so the two derive it here rather than each rolling one.
|
||||
* document to being framed by the sandbox proxy and the Inspector page above
|
||||
* it. Same grammar, same fallback — an all-invalid list yields the loopback
|
||||
* family rather than an empty source list, since a `frame-ancestors` with no
|
||||
* sources means `'none'` and would block the frame outright — so the two
|
||||
* derive it here rather than each rolling one.
|
||||
*/
|
||||
export function frameAncestorsDirective(origins?: string[]): string {
|
||||
const valid = (origins ?? []).filter((o) => CSP_HOST_SOURCE.test(o));
|
||||
|
|
|
|||
|
|
@ -391,6 +391,24 @@ vi.mock(
|
|||
},
|
||||
);
|
||||
|
||||
// --- publishAppDocument spy (#2056) -----------------------------------------
|
||||
// The dedicated-origin publisher App hands both bridge factories. Mocked at the
|
||||
// module so a test can assert the wiring is live — the factory's own tests
|
||||
// inject the dep and cannot see App failing to supply it.
|
||||
const publishAppDocumentMock =
|
||||
vi.fn<
|
||||
(
|
||||
doc: { html: string; csp?: string },
|
||||
opts: { baseUrl: string; authToken?: string },
|
||||
) => Promise<string | null>
|
||||
>();
|
||||
vi.mock("./lib/publishAppDocument", () => ({
|
||||
publishAppDocument: (
|
||||
doc: { html: string; csp?: string },
|
||||
opts: { baseUrl: string; authToken?: string },
|
||||
) => publishAppDocumentMock(doc, opts),
|
||||
}));
|
||||
|
||||
// --- InspectorView double ---------------------------------------------------
|
||||
// Surfaces each piece of session-scoped state under test and exposes buttons
|
||||
// that invoke the App's connect / call-tool / get-prompt / read-resource /
|
||||
|
|
@ -3463,4 +3481,40 @@ describe("App MCP App listed-resource metadata wiring (#2055)", () => {
|
|||
).toBeUndefined();
|
||||
}
|
||||
});
|
||||
|
||||
it("hands BOTH bridge factories a working publishAppDocument (#2056)", async () => {
|
||||
// Both call sites pass the same `publishDocument`, and the end-to-end smoke
|
||||
// only covers the Apps-screen one — so without this the elicitation
|
||||
// factory's `publishAppDocument` could be dropped and every test stays
|
||||
// green while a domain-declaring elicitation app silently loses its real
|
||||
// origin. Assert it is not merely present but actually reaches the lib.
|
||||
publishAppDocumentMock.mockResolvedValue(
|
||||
"http://127.0.0.1:6276/app-document/deadbeef",
|
||||
);
|
||||
|
||||
renderWithMantine(<App />);
|
||||
|
||||
const appsFactory = () =>
|
||||
appBridgeFactoryDeps.find((d) => !d.advertiseElicitation);
|
||||
const elicitationFactory = () =>
|
||||
appBridgeFactoryDeps.find((d) => d.advertiseElicitation === true);
|
||||
await waitFor(() => {
|
||||
expect(appsFactory()).toBeDefined();
|
||||
expect(elicitationFactory()).toBeDefined();
|
||||
});
|
||||
|
||||
for (const deps of [appsFactory(), elicitationFactory()]) {
|
||||
publishAppDocumentMock.mockClear();
|
||||
await expect(
|
||||
deps?.publishAppDocument?.({
|
||||
html: "<p>app</p>",
|
||||
csp: "default-src 'none'",
|
||||
}),
|
||||
).resolves.toBe("http://127.0.0.1:6276/app-document/deadbeef");
|
||||
expect(publishAppDocumentMock).toHaveBeenCalledWith(
|
||||
{ html: "<p>app</p>", csp: "default-src 'none'" },
|
||||
expect.objectContaining({ baseUrl: expect.any(String) }),
|
||||
);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -10,12 +10,16 @@
|
|||
import { describe, it, expect, vi } from "vitest";
|
||||
import { publishAppDocument } from "./publishAppDocument";
|
||||
|
||||
/**
|
||||
* A real `Response`, not a shaped literal. The platform implementation is
|
||||
* available here, so building one keeps `ok`/`status`/`json()` honest rather
|
||||
* than asserting a hand-written object is a Response.
|
||||
*/
|
||||
function jsonResponse(body: unknown, status = 200): Response {
|
||||
return {
|
||||
ok: status >= 200 && status < 300,
|
||||
return new Response(JSON.stringify(body), {
|
||||
status,
|
||||
json: async () => body,
|
||||
} as unknown as Response;
|
||||
headers: { "Content-Type": "application/json" },
|
||||
});
|
||||
}
|
||||
|
||||
describe("publishAppDocument", () => {
|
||||
|
|
@ -88,13 +92,11 @@ describe("publishAppDocument", () => {
|
|||
});
|
||||
|
||||
it("resolves null when the response body is not JSON", async () => {
|
||||
const fetchFn = vi.fn<typeof fetch>().mockResolvedValue({
|
||||
ok: true,
|
||||
status: 200,
|
||||
json: async () => {
|
||||
throw new Error("not json");
|
||||
},
|
||||
} as unknown as Response);
|
||||
// A real 200 whose body is not JSON, so `json()` rejects for the reason
|
||||
// the production path has to survive rather than because a stub threw.
|
||||
const fetchFn = vi
|
||||
.fn<typeof fetch>()
|
||||
.mockResolvedValue(new Response("not json", { status: 200 }));
|
||||
await expect(
|
||||
publishAppDocument(
|
||||
{ html: "<p>app</p>" },
|
||||
|
|
|
|||
|
|
@ -84,6 +84,20 @@ describe("createRemoteApp POST /api/app-document", () => {
|
|||
expect(res.status).toBe(400);
|
||||
});
|
||||
|
||||
it.each([
|
||||
["a JSON null body", "null"],
|
||||
["a JSON array body", "[1,2]"],
|
||||
])("rejects %s with 400 rather than a 500", async (_label, raw) => {
|
||||
// Both parse cleanly, so the try/catch above sees nothing — destructuring
|
||||
// them is what used to throw, outside it, turning a bad shape into a 500.
|
||||
const publish = vi
|
||||
.fn<(doc: { html: string; csp?: string }) => { url: string } | null>()
|
||||
.mockReturnValue({ url: "http://x/y" });
|
||||
const res = await appWith(publish).request(post(raw));
|
||||
expect(res.status).toBe(400);
|
||||
expect(publish).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it.each([
|
||||
["missing html", {}],
|
||||
["empty html", { html: "" }],
|
||||
|
|
|
|||
|
|
@ -337,6 +337,58 @@ describe("createAppOriginController", () => {
|
|||
}
|
||||
});
|
||||
|
||||
it("stops minting URLs when the listener errors AFTER it bound", async () => {
|
||||
// `publish` gates on `origin`, not on `server`. An error arriving once the
|
||||
// listener is live must therefore clear `origin` too — otherwise the
|
||||
// browser keeps being handed URLs on a dead port and never takes the
|
||||
// srcdoc fallback the caller is promised.
|
||||
vi.resetModules();
|
||||
let emitter!: EventEmitter;
|
||||
vi.doMock("node:http", async () => {
|
||||
const actual =
|
||||
await vi.importActual<typeof import("node:http")>("node:http");
|
||||
return {
|
||||
...actual,
|
||||
createServer: () => {
|
||||
emitter = new EventEmitter();
|
||||
return Object.assign(emitter, {
|
||||
listen: (_p: number, _h: string, cb?: () => void) =>
|
||||
setImmediate(() => cb?.()),
|
||||
address: () => ({
|
||||
address: "127.0.0.1",
|
||||
family: "IPv4",
|
||||
port: 6276,
|
||||
}),
|
||||
close: (cb?: () => void) => cb?.(),
|
||||
});
|
||||
},
|
||||
};
|
||||
});
|
||||
const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {});
|
||||
try {
|
||||
const mod = await import("../../../../server/app-origin-controller.js");
|
||||
const c = mod.createAppOriginController({
|
||||
port: 6276,
|
||||
host: "127.0.0.1",
|
||||
});
|
||||
await c.start();
|
||||
expect(c.getOrigin()).toBe("http://127.0.0.1:6276");
|
||||
expect(c.publish({ html: "<p>x</p>" })).not.toBeNull();
|
||||
|
||||
emitter.emit(
|
||||
"error",
|
||||
Object.assign(new Error("boom"), { code: "EPIPE" }),
|
||||
);
|
||||
|
||||
expect(c.getOrigin()).toBeNull();
|
||||
expect(c.publish({ html: "<p>x</p>" })).toBeNull();
|
||||
} finally {
|
||||
errorSpy.mockRestore();
|
||||
vi.doUnmock("node:http");
|
||||
vi.resetModules();
|
||||
}
|
||||
});
|
||||
|
||||
it("gives up after one retry when the dynamic fallback also fails", async () => {
|
||||
// Guards the `retriedDynamic` latch. A real `listen(0)` effectively always
|
||||
// succeeds, so a claimed port only ever produces ONE EADDRINUSE — under
|
||||
|
|
|
|||
|
|
@ -254,6 +254,14 @@
|
|||
// delivered as a real response header there (stronger than the
|
||||
// <meta> the srcdoc path relies on), so no wrapping applies here
|
||||
// — this proxy still never parses the untrusted bytes.
|
||||
//
|
||||
// Drop any `srcdoc` a previous render left behind first: it takes
|
||||
// precedence over `src`, so a frame that had rendered an opaque
|
||||
// document would keep showing it and never load this URL. Not
|
||||
// reachable today (each render remounts the iframe, which reloads
|
||||
// this document), but the cost of being wrong about that later is
|
||||
// an app silently rendering the wrong bytes.
|
||||
inner.removeAttribute("srcdoc");
|
||||
innerOrigin = srcUrl.origin;
|
||||
inner.src = srcUrl.href;
|
||||
} else if (typeof html === "string") {
|
||||
|
|
@ -265,6 +273,20 @@
|
|||
// boundary). Known trade-off: a small set of libraries (e.g.
|
||||
// CesiumJS Map) don't initialise correctly under srcdoc — that
|
||||
// is accepted in exchange for the isolation guarantee.
|
||||
//
|
||||
// Restore the opaque-origin posture first. A frame that
|
||||
// previously took the dedicated-origin path still carries
|
||||
// `allow-same-origin`, and `innerOrigin` still names that origin;
|
||||
// assigning `srcdoc` under those would put the untrusted document
|
||||
// in THIS proxy's realm — the exact escape the double-iframe
|
||||
// model exists to prevent. The sandbox attribute is read at
|
||||
// navigation time and assigning `srcdoc` is the navigation, so
|
||||
// stripping it here takes effect for the document being loaded.
|
||||
const fallbackTokens = (inner.getAttribute("sandbox") || "")
|
||||
.split(/\s+/)
|
||||
.filter((t) => t && t.toLowerCase() !== "allow-same-origin");
|
||||
inner.setAttribute("sandbox", fallbackTokens.join(" "));
|
||||
innerOrigin = "null";
|
||||
inner.srcdoc = html;
|
||||
}
|
||||
} else {
|
||||
|
|
|
|||
|
|
@ -749,13 +749,24 @@ export function createRemoteApp(
|
|||
if (!publish) {
|
||||
return c.json({ error: "App-origin hosting is not available" }, 503);
|
||||
}
|
||||
let body: { html?: unknown; csp?: unknown };
|
||||
let parsed: unknown;
|
||||
try {
|
||||
body = (await c.req.json()) as { html?: unknown; csp?: unknown };
|
||||
parsed = await c.req.json();
|
||||
} catch {
|
||||
return c.json({ error: "Invalid JSON body" }, 400);
|
||||
}
|
||||
const { html, csp } = body;
|
||||
// `null` and `[1,2]` are both valid JSON, so `c.req.json()` resolving is
|
||||
// not proof there is an object to destructure — and destructuring `null`
|
||||
// here would throw OUTSIDE the try above, turning a malformed body into a
|
||||
// 500 instead of the 400 every other bad shape gets.
|
||||
if (
|
||||
parsed === null ||
|
||||
typeof parsed !== "object" ||
|
||||
Array.isArray(parsed)
|
||||
) {
|
||||
return c.json({ error: "Invalid JSON body" }, 400);
|
||||
}
|
||||
const { html, csp } = parsed as { html?: unknown; csp?: unknown };
|
||||
if (typeof html !== "string" || html === "") {
|
||||
return c.json({ error: "Missing html" }, 400);
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue