diff --git a/runner/pipeline/fixtures/cloudflare-sandbox-stub.mjs b/runner/pipeline/fixtures/cloudflare-sandbox-stub.mjs new file mode 100644 index 00000000..9404431d --- /dev/null +++ b/runner/pipeline/fixtures/cloudflare-sandbox-stub.mjs @@ -0,0 +1,19 @@ +// Stands in for `@cloudflare/sandbox` when the router runs under plain Node +// (see worker-hooks.mjs). Structural only: `proxyToSandbox()` answering null +// is the "not a preview URL" path every API request takes, and the `Sandbox` +// class exists so index.ts can extend and export it. `getSandbox()` throws on +// purpose — every route under test in mcp-routes.test.mjs must have answered +// (or refused) before any container is involved, and a test that reaches a +// sandbox anyway should fail loudly rather than silently no-op. + +export class Sandbox {} + +export function getSandbox() { + throw new Error( + "getSandbox() called in a route test: this request should have been answered before any container was involved", + ); +} + +export async function proxyToSandbox() { + return null; +} diff --git a/runner/pipeline/fixtures/worker-hooks.mjs b/runner/pipeline/fixtures/worker-hooks.mjs new file mode 100644 index 00000000..7f7cd217 --- /dev/null +++ b/runner/pipeline/fixtures/worker-hooks.mjs @@ -0,0 +1,33 @@ +// Module hooks that make the real router (workers/api/src/index.ts) loadable +// under plain `node --experimental-strip-types --test` — registered by +// pipeline/mcp-routes.test.mjs via `module.register()` before it imports the +// worker. `node --test` runs each spec file in its own process, so nothing +// here leaks into the other pipeline specs. +// +// Two obstacles, two rewrites: +// +// - The worker's modules import each other by `.js` specifier (the shape the +// Workers bundler resolves), but the files on disk are `.ts`, and Node's +// resolver has no extension fallback — the same limitation that made +// theme-codegen.test.mjs read its subject as text. Map the extension, only +// for relative imports inside the worker's own source tree. +// +// - `@cloudflare/sandbox` imports the `cloudflare:` URL scheme at load time, +// which only exists inside workerd. The routes under test never reach a +// sandbox, so a structural stub stands in for the package. + +const SANDBOX_STUB = new URL("./cloudflare-sandbox-stub.mjs", import.meta.url).href; + +export async function resolve(specifier, context, nextResolve) { + if (specifier === "@cloudflare/sandbox") { + return { url: SANDBOX_STUB, shortCircuit: true }; + } + if ( + specifier.startsWith(".") + && specifier.endsWith(".js") + && context.parentURL?.includes("/workers/api/src/") + ) { + return nextResolve(`${specifier.slice(0, -3)}.ts`, context); + } + return nextResolve(specifier, context); +} diff --git a/runner/pipeline/mcp-create.test.mjs b/runner/pipeline/mcp-create.test.mjs index 45504392..7833492a 100644 --- a/runner/pipeline/mcp-create.test.mjs +++ b/runner/pipeline/mcp-create.test.mjs @@ -3,9 +3,13 @@ // Run: node --experimental-strip-types --test pipeline/*.test.mjs import test from "node:test"; import assert from "node:assert/strict"; +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; +import { dirname, join } from "node:path"; import { MAX_MCP_BYTES, MAX_MCP_FILES, + isMcpCreated, isMcpValidationError, isTeamEmail, validateMcpFiles, @@ -177,11 +181,35 @@ test("only demos the MCP created are updatable through it", () => { // Containment for the shared-secret trust model (security review of PR #177): the // asserted author cannot be stronger than the secret that carries it, so the path is // limited to what this service published. `forked_from` is the provenance stamp. - const fromMcp = (row) => Boolean(row.forked_from?.startsWith("mcp:")); - assert.ok(fromMcp({ forked_from: "mcp:javascript" })); - assert.ok(fromMcp({ forked_from: "mcp:react" })); + // + // `isMcpCreated` is the production predicate — imported, not re-declared here. An + // earlier version of this test asserted against its own local copy, which stayed + // green with the route guard deleted; a test of a security control has to be able + // to fail when the control goes away. + assert.ok(isMcpCreated({ forked_from: "mcp:javascript" })); + assert.ok(isMcpCreated({ forked_from: "mcp:react" })); // Anything built in the browser stays out of reach of this route. - assert.ok(!fromMcp({ forked_from: "catalog:javascript" })); - assert.ok(!fromMcp({ forked_from: null })); - assert.ok(!fromMcp({})); + assert.ok(!isMcpCreated({ forked_from: "catalog:javascript" })); + assert.ok(!isMcpCreated({ forked_from: null })); + assert.ok(!isMcpCreated({ forked_from: undefined })); + assert.ok(!isMcpCreated({})); +}); + +test("the update route calls isMcpCreated(), not a re-inlined copy of it", () => { + // Same style as pipeline/theme-codegen.test.mjs: the rule is structural, so the + // route source is read as text. Importing the predicate (above) proves what it + // decides; this proves the route still *asks* it — an inline `forked_from` check + // could drift away from the exported one while every import-based test stays green. + const root = join(dirname(fileURLToPath(import.meta.url)), ".."); + const source = readFileSync(join(root, "workers/api/src/index.ts"), "utf8"); + const start = source.indexOf('request.method === "PATCH" && parts[0] === "api" && parts[1] === "mcp"'); + assert.ok(start > -1, "the MCP update route exists in index.ts"); + const end = source.indexOf('parts[1] === "demos"', start); + const route = source.slice(start, end > -1 ? end : undefined); + assert.match(route, /isMcpCreated\(/, "the route must gate on the exported predicate"); + assert.doesNotMatch( + route, + /forked_from\?*\.\s*startsWith/, + "an inline forked_from check would no longer be what the tests import — call isMcpCreated()", + ); }); diff --git a/runner/pipeline/mcp-routes.test.mjs b/runner/pipeline/mcp-routes.test.mjs new file mode 100644 index 00000000..08f9c733 --- /dev/null +++ b/runner/pipeline/mcp-routes.test.mjs @@ -0,0 +1,284 @@ +// Route-level proof for the MCP endpoints (DEV-2501, ADR-0033): POST +// /api/mcp/demos and PATCH /api/mcp/demos/:id, driven through the REAL router — +// the default export of workers/api/src/index.ts — not through re-declared +// copies of its checks. Until this spec, nothing imported the router at all, so +// every status code and response shape it promises was untested. +// +// The worker loads under plain `node --test` via the module hooks in +// fixtures/worker-hooks.mjs (registered below, before the import). Bindings are +// in-memory fakes covering exactly what these two routes touch: D1 (with a +// recorded write log), KV, and R2. No route under test may reach a container — +// the create path is steered through createDemo()'s build-cache-hit branch +// (itself production code) by a fake build_cache row, and the sandbox stub +// throws if anything asks for a container anyway. +// +// Run: node --experimental-strip-types --test pipeline/*.test.mjs + +import test from "node:test"; +import assert from "node:assert/strict"; +import { register } from "node:module"; + +register("./fixtures/worker-hooks.mjs", import.meta.url); + +const { default: worker } = await import("../workers/api/src/index.ts"); +const { demoListQuery } = await import("../workers/api/src/demos-list.ts"); + +// ---- in-memory bindings ------------------------------------------------------ + +/** Rebuild the row a `INSERT OR REPLACE INTO demos (...) VALUES (...)` wrote: + * zip the column list with the placeholders, `?` consuming a bind and a bare + * literal (the hardcoded `revoked` 0) standing as itself. */ +function parseDemosInsert(sql, binds) { + const m = /INSERT OR REPLACE INTO demos\s*\(([^)]+)\)\s*VALUES\s*\(([^)]+)\)/s.exec(sql); + if (!m) return null; + const cols = m[1].split(",").map((s) => s.trim()); + const placeholders = m[2].split(",").map((s) => s.trim()); + let next = 0; + const row = {}; + cols.forEach((col, i) => { + row[col] = placeholders[i] === "?" ? binds[next++] : Number(placeholders[i]); + }); + return row; +} + +/** D1 fake: seeded demo rows, a recorded write log, and a build_cache that + * always hits so createDemo() takes its cached-artifact branch. Unmatched + * reads answer empty, which the budget code treats as "no spend yet". */ +function fakeD1(seedRows = []) { + const writes = []; + const demos = new Map(seedRows.map((row) => [row.id, row])); + const prepare = (sql) => { + const bound = (binds) => ({ + async first() { + if (/FROM demos WHERE id = \?/.test(sql)) return demos.get(binds[0]) ?? null; + if (/FROM build_cache/.test(sql)) return { r2_prefix: "demos/_prior-identical-build/" }; + return null; + }, + async run() { + writes.push({ sql, binds }); + const inserted = parseDemosInsert(sql, binds); + if (inserted) demos.set(inserted.id, inserted); + return { success: true, meta: {} }; + }, + async all() { + return { success: true, results: [] }; + }, + }); + return { bind: (...binds) => bound(binds), ...bound([]) }; + }; + return { db: { prepare }, writes, demos }; +} + +function fakeKV() { + const store = new Map(); + return { + async get(key, type) { + const value = store.get(key); + if (value === undefined) return null; + return type === "json" ? JSON.parse(value) : value; + }, + async put(key, value) { + store.set(key, String(value)); + }, + async delete(key) { + store.delete(key); + }, + }; +} + +function fakeR2() { + const puts = []; + return { + puts, + async put(key) { + puts.push(key); + }, + async get() { + return null; + }, + async list() { + return { objects: [] }; + }, + }; +} + +const SECRET = "test-secret"; +const AUTHOR = "dev@handsontable.com"; + +function makeEnv(seedRows = []) { + const { db, writes, demos } = fakeD1(seedRows); + const artifacts = fakeR2(); + const env = { + Sandbox: {}, + SANDBOX_BUILDER: {}, + DB: db, + CACHE: fakeKV(), + ARTIFACTS: artifacts, + MCP_SHARED_SECRET: SECRET, + LOGIN_BROKER_URL: "https://login.invalid", + EMBED_ALLOWED_ANCESTORS: "https://handsontable.com", + ERROR_REPORTING_DSN: "", + CF_VERSION_METADATA: { id: "test", tag: "test" }, + // Not the production host, so the Sentry gate in index.ts stays inert. + PREVIEW_HOST: "localhost:8787", + }; + return { env, writes, demos, artifacts }; +} + +const ctx = { + waitUntil(promise) { + Promise.resolve(promise).catch(() => {}); + }, + passThroughOnException() {}, +}; + +const FILES = { "/package.json": '{"name":"demo"}', "/index.js": "console.log(1)" }; + +const mcpHeaders = { + "Content-Type": "application/json", + "X-MCP-Secret": SECRET, + "X-Demo-Author": AUTHOR, +}; + +const createRequest = (body) => + new Request("https://demos.handsontable.com/api/mcp/demos", { + method: "POST", + headers: mcpHeaders, + body: JSON.stringify(body), + }); + +const patchRequest = (id, body = { files: FILES }) => + new Request(`https://demos.handsontable.com/api/mcp/demos/${id}`, { + method: "PATCH", + headers: mcpHeaders, + body: JSON.stringify(body), + }); + +/** A stored demo row as D1 would return it (see DemoRow in share.ts). */ +const demoRow = (overrides = {}) => ({ + id: "abc123", + title: "A demo", + description: "words", + framework: "react", + tier: 1, + ht_version: "latest", + files_hash: "hash", + r2_prefix: "demos/abc123/", + forked_from: "mcp:react", + visibility: "unlisted", + revoked: 0, + created_by: AUTHOR, + created_at: "2026-08-17T00:00:00.000Z", + updated_at: "2026-08-17T00:00:00.000Z", + revoked_at: null, + ...overrides, +}); + +// ---- create ------------------------------------------------------------------ + +test("an MCP demo without a description is refused before it is built", async () => { + const { env, writes, artifacts } = makeEnv(); + const res = await worker.fetch( + createRequest({ framework: "react", title: "Grid", files: FILES }), + env, + ctx, + ); + assert.equal(res.status, 400); + assert.match((await res.json()).error, /description is required/); + // Refused up front: nothing was written, no artifact was stored, no build ran. + assert.deepEqual(writes, [], "no D1 write may happen for a refused create"); + assert.deepEqual(artifacts.puts, [], "no artifact may be stored for a refused create"); +}); + +test("a created demo answers with the four links and its owner", async () => { + const { env } = makeEnv(); + const res = await worker.fetch( + createRequest({ framework: "react", title: "Grid", description: "A sortable grid", files: FILES }), + env, + ctx, + ); + assert.equal(res.status, 201); + const body = await res.json(); + // Exactly these keys — an agent navigates by them, so a dropped or renamed + // link is a breaking change of the MCP contract. `htVersion` joined the + // response when the version catalog moved server-side (master, ht-version.ts) + // — this assertion caught that addition, which is its job; grew, reviewed, + // admitted. + assert.deepEqual( + Object.keys(body).sort(), + ["createdBy", "editUrl", "embedUrl", "htVersion", "id", "shareUrl", "url"], + ); + assert.equal(body.url, `/d/${body.id}`); + assert.equal(body.embedUrl, `/embed/${body.id}`); + assert.equal(body.editUrl, `/edit/${body.id}`); + assert.equal(body.shareUrl, `/share/${body.id}`); + assert.equal(body.createdBy, AUTHOR); + // Concrete, not a dist-tag: the agent pins its follow-up update to this. + assert.match(body.htVersion, /^\d+\.\d+\.\d+/); +}); + +test("a created demo is written with the caller as its owner, and its owner's listing finds it", async () => { + const { env, writes, demos } = makeEnv(); + const res = await worker.fetch( + createRequest({ framework: "react", title: "Grid", description: "A sortable grid", files: FILES }), + env, + ctx, + ); + assert.equal(res.status, 201); + const { id } = await res.json(); + + const insert = writes.find((w) => /INSERT OR REPLACE INTO demos/.test(w.sql)); + assert.ok(insert, "the create route must insert a demos row"); + const row = demos.get(id); + assert.equal(row.created_by, AUTHOR, "the asserted author is the stored owner"); + assert.match(row.forked_from, /^mcp:/, "provenance is stamped, or the demo can never be MCP-updated"); + + // The row lands in the owner's "My demos": the same query GET /api/demos runs + // for scope=mine matches it (the audit's G5 concern, router-side). + const { sql, binds } = demoListQuery("mine", AUTHOR); + assert.match(sql, /LOWER\(created_by\) = \?/); + assert.equal(row.created_by.toLowerCase(), binds[0], "the stored owner matches the listing's bind"); +}); + +// ---- update: every refusal the guard chain promises --------------------------- + +test("someone else's demo is 403, even with a valid secret", async () => { + const { env, writes } = makeEnv([ + demoRow({ created_by: "other@handsontable.com", forked_from: "mcp:react" }), + ]); + const res = await worker.fetch(patchRequest("abc123"), env, ctx); + assert.equal(res.status, 403); + const body = await res.json(); + assert.equal(body.error, "forbidden"); + assert.match(body.detail, /belongs to someone else/); + assert.deepEqual(writes, [], "a refused update must not write"); +}); + +test("a browser-made demo is 403 through the MCP, and says where to edit it", async () => { + const { env, writes } = makeEnv([demoRow({ forked_from: "catalog:react" })]); + const res = await worker.fetch(patchRequest("abc123"), env, ctx); + assert.equal(res.status, 403); + const body = await res.json(); + assert.equal(body.error, "forbidden"); + assert.match(body.detail, /not created through the MCP/); + assert.match(body.detail, /\/edit\//, "the refusal points at the browser editor"); + assert.deepEqual(writes, [], "a refused update must not write"); +}); + +test("a revoked demo is gone, not rebuilt", async () => { + const { env, writes, artifacts } = makeEnv([ + demoRow({ revoked: 1, revoked_at: "2026-08-16T00:00:00.000Z" }), + ]); + const res = await worker.fetch(patchRequest("abc123"), env, ctx); + assert.equal(res.status, 410); + assert.equal((await res.json()).error, "gone"); + assert.deepEqual(writes, [], "a revoked demo must not be written to"); + assert.deepEqual(artifacts.puts, [], "a revoked demo must not get fresh artifacts"); +}); + +test("an unknown demo is 404", async () => { + const { env } = makeEnv(); + const res = await worker.fetch(patchRequest("nosuchid"), env, ctx); + assert.equal(res.status, 404); + assert.equal((await res.json()).error, "not found"); +}); diff --git a/runner/workers/api/src/index.ts b/runner/workers/api/src/index.ts index e5d17115..c71e542d 100644 --- a/runner/workers/api/src/index.ts +++ b/runner/workers/api/src/index.ts @@ -22,7 +22,7 @@ import { FRAMEWORK_DEV, BUILD_CONFIG } from "./frameworks.generated.js"; import { dependencyMetadataFingerprint } from "./dependency-metadata.js"; import { authenticate, authenticateService, sameOwner } from "./auth.js"; import { MAX_TITLE, isValidationError, validateDescription, validateTitle } from "./demo-info.js"; -import { isMcpValidationError, validateMcpFiles } from "./mcp-create.js"; +import { isMcpCreated, isMcpValidationError, validateMcpFiles } from "./mcp-create.js"; import { editorVersionRef, fetchVersionCatalog, resolveHandsontableVersion } from "./ht-version.js"; import { demoListQuery, parseDemoScope } from "./demos-list.js"; import { errorPageResponse, wantsHtmlError } from "./error-page.js"; @@ -1098,10 +1098,10 @@ export default Sentry.withSentry(sentryOptions, { // Containment, not authentication (security review of PR #177). `X-Demo-Author` is // asserted under the same shared secret that grants access to this route, so the // ownership check above catches a wrong id — it cannot withstand misuse of the secret - // itself. Restricting the path to demos this service created means a leaked secret can - // never rewrite work somebody built in the browser; the blast radius stays inside what - // the MCP published in the first place. - if (!row.forked_from?.startsWith("mcp:")) { + // itself. `isMcpCreated()` restricts the path to demos this service created (its JSDoc + // carries the full rationale); tests import that same predicate, so keep the check on + // it rather than re-inlining `forked_from` here. + if (!isMcpCreated(row)) { return json( { error: "forbidden", diff --git a/runner/workers/api/src/mcp-create.ts b/runner/workers/api/src/mcp-create.ts index fc058632..b69fe34f 100644 --- a/runner/workers/api/src/mcp-create.ts +++ b/runner/workers/api/src/mcp-create.ts @@ -90,3 +90,16 @@ export function validateMcpFiles(files: unknown): Record | McpVa export function isTeamEmail(value: unknown): value is string { return typeof value === "string" && /^[^\s@]+@handsontable\.com$/i.test(value.trim()); } + +/** + * Was this demo published by the MCP itself? The containment control from the + * security review of PR #177: the shared secret only proves *a* trusted service is + * calling — it says nothing about whose demos that service may rewrite — so the + * update route is restricted to rows the MCP created, and `forked_from` is the + * provenance stamp (`mcp:`, written by the create route and never by a + * browser save). A leaked secret can then never touch work somebody built in the + * browser; the blast radius stays inside what the MCP published in the first place. + */ +export function isMcpCreated(row: { forked_from?: string | null }): boolean { + return Boolean(row.forked_from?.startsWith("mcp:")); +}