diff --git a/CHANGELOG.md b/CHANGELOG.md index d6bc8424..e4f8f949 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -180,6 +180,16 @@ Sessions survive and nobody signs in again. access and refresh tokens use Better Auth's own encryption, keyed on `BETTER_AUTH_SECRET`. - **A failed provider registration looked like a button that did not work.** The error was rendered on the page behind the dialog, which was covering it. +- **Deleting a component in the playground could release one the build ships.** `DELETE + /api/sandboxed/:name` deleted from the shared components table by name, without checking + which kind of component the name belonged to. Naming a compiled component removed its + governance row, and the foreign keys took that component's per-Bot withholdings and its + function grants with it. Withholding is the half that fails open: a published component is + available to every Bot unless a row says otherwise, so the next catalogue announcement brought + the component back published, and available to a Bot it had deliberately been kept from. The + audit row called it `kind: "sandboxed"`. The endpoint now refuses a name this surface does not + own and answers 404, the way publishing already did. A governance row whose source is already + gone is still this surface's to clear. - **A write could follow a symlink out of the Bot's workspace.** The confinement resolved the directory a write would land in but not the name it would land on, so a link left at `notes.txt` pointing outside was followed by the write; a read through the identical link was already refused. diff --git a/server/src/components/sandboxed-routes.ts b/server/src/components/sandboxed-routes.ts index b297aad3..8e4ae226 100644 --- a/server/src/components/sandboxed-routes.ts +++ b/server/src/components/sandboxed-routes.ts @@ -110,8 +110,19 @@ export function createSandboxedRoutes( const forbidden = requireAdmin(context); if (forbidden) return forbidden; - await store.remove(context.req.param("name"), actorEmail(context)); - return context.json({ ok: true }); + // Answered like `publish`, because it is the same question: this surface owns the components it + // authored, and a name with no draft behind it is not one of them. Reporting that as "not found" + // rather than as success also stops a caller reading `{ ok: true }` as "the thing you named is + // gone", which it was not. + try { + await store.remove(context.req.param("name"), actorEmail(context)); + return context.json({ ok: true }); + } catch (error) { + if (error instanceof SandboxedNotFoundError) { + return context.json({ error: error.message }, 404); + } + throw error; + } }); return routes; diff --git a/server/src/components/sandboxed.ts b/server/src/components/sandboxed.ts index 784426f1..c9d20656 100644 --- a/server/src/components/sandboxed.ts +++ b/server/src/components/sandboxed.ts @@ -277,6 +277,38 @@ export function createSandboxedStore( }, async remove(name: string, by: string): Promise { + /* + * Refuse a name this surface does not own. + * + * `components` is shared with the compiled catalogue and the delete below is by name, with + * nothing checking which kind of component the name belonged to. So a compiled component's + * governance row could be deleted through the playground's endpoint, and the foreign keys took + * its per-Bot withholdings and its function grants with it. + * + * The withholdings are the half that fails open. A published component is available to every + * Bot unless a `component_exclusions` row says otherwise, so losing that row does not hide the + * component, it releases it — and the next catalogue announcement rewrites the component with + * `published: true`, because that is how one the build ships arrives. A deliberate "not this + * Bot" comes back as "every Bot", under an audit row saying `kind: "sandboxed"`, which is the + * one thing it was not. + * + * Asked of the governance row's `kind` rather than of `sandboxed_components`, because ownership + * is the actual question and the two answers differ in one case worth keeping: these deletes + * are not in a transaction, so a failure between them leaves a governance row with no source. + * That orphan is the "catalogue disagrees with the build" state named below, it is this + * surface's to clean up, and requiring the source row would have made it undeletable here. + */ + const [governance] = await database + .select({ kind: components.kind }) + .from(components) + .where(eq(components.name, name)) + .limit(1); + // "sandboxed" is the kind `save` writes above. A name with no row at all is refused for the + // same reason `publish` refuses one: this surface has nothing by that name to act on. + if (governance?.kind !== "sandboxed") { + throw new SandboxedNotFoundError(name); + } + // Delete both rows; a governance row pointing at a component with no source is the visible // "catalogue disagrees with the build" state. await database diff --git a/server/tests/sandboxed-components.integration.test.ts b/server/tests/sandboxed-components.integration.test.ts index 33da49fd..ed9b1445 100644 --- a/server/tests/sandboxed-components.integration.test.ts +++ b/server/tests/sandboxed-components.integration.test.ts @@ -1,11 +1,20 @@ -import { afterAll, describe, expect, test } from "bun:test"; +import { afterAll, beforeAll, describe, expect, test } from "bun:test"; import { randomUUID } from "node:crypto"; import { eq } from "drizzle-orm"; import { createAuditStore } from "../src/audit"; -import { createSandboxedStore } from "../src/components/sandboxed"; +import { + createSandboxedStore, + SandboxedNotFoundError, +} from "../src/components/sandboxed"; import { createDatabase } from "../src/db/client"; import { TEST_POOL } from "./support/database"; -import { components, sandboxedComponents } from "../src/db/schema"; +import { + agents, + componentExclusions, + componentFunctions, + components, + sandboxedComponents, +} from "../src/db/schema"; /** * A component authored in a browser can be edited freely and still reach nobody until it is @@ -146,3 +155,121 @@ describe("authoring a component without a rebuild", () => { expect(governance).toBeUndefined(); }); }); + +/** + * What this surface may delete. + * + * `components` is shared with the compiled catalogue, so a delete by name on this endpoint could + * reach a row the playground never authored. `save` refuses a name that is not a slug and `publish` + * refuses a name with no draft behind it; `remove` did neither, which is the asymmetry these pin. + */ +describe("deleting a name this surface does not own", () => { + const compiled = `chart_test_${suite}`; + const bot = `agent_test_${suite}`; + + beforeAll(async () => { + await database + .insert(agents) + .values({ id: bot, name: bot, type: "remote_ag_ui", configuration: {} }) + .onConflictDoNothing(); + // A component the build ships, as `syncCatalogue` would have written it. + await database.insert(components).values({ + name: compiled, + title: "A compiled chart", + kind: "chart", + draftDescription: "Drawn by the build.", + publishedDescription: "Drawn by the build.", + published: true, + publishedAt: new Date(), + updatedBy: "the build", + }); + // One Bot held back from it. This row is the whole of the decision: a published component is + // available to every Bot unless it exists. + await database.insert(componentExclusions).values({ + componentName: compiled, + agentId: bot, + withheldBy: "admin@openbot.local", + }); + await database.insert(componentFunctions).values({ + componentName: compiled, + functionName: "listRecentOrders", + grantedBy: "admin@openbot.local", + }); + }); + + afterAll(async () => { + await database.delete(components).where(eq(components.name, compiled)); + await database.delete(agents).where(eq(agents.id, bot)); + }); + + test("refuses a compiled component's name instead of deleting its governance", async () => { + await expect(store.remove(compiled, "admin@openbot.local")).rejects.toThrow( + SandboxedNotFoundError, + ); + + const [governance] = await database + .select() + .from(components) + .where(eq(components.name, compiled)); + expect(governance).toBeDefined(); + expect(governance?.published).toBe(true); + }); + + test("leaves the withholding that would otherwise have been released", async () => { + // The half that fails OPEN, and the reason this is worth a guard rather than a tidy-up. Losing + // this row does not hide the component from the Bot, it releases it to the Bot — and the next + // catalogue announcement rewrites the component as published, because that is how one the build + // ships arrives. A deliberate "not this Bot" would come back as "every Bot". + const withheld = await database + .select() + .from(componentExclusions) + .where(eq(componentExclusions.componentName, compiled)); + expect(withheld).toHaveLength(1); + }); + + test("leaves the function grants, which the cascade would also have taken", async () => { + // This half fails closed, so it is a capability lost rather than one gained. Asserted anyway: + // a component that silently stops being able to read is still a component that stopped working. + const granted = await database + .select() + .from(componentFunctions) + .where(eq(componentFunctions.componentName, compiled)); + expect(granted).toHaveLength(1); + }); + + test("refuses a name nothing was ever authored under", async () => { + // Answered rather than reported as success. `{ ok: true }` for a name that was never there reads + // as "the thing you named is gone", which is the one thing it does not establish. + await expect( + store.remove(`custom_never_${suite}`, "admin@openbot.local"), + ).rejects.toThrow(SandboxedNotFoundError); + }); + + test("still deletes a governance row whose source is already gone", async () => { + /* + * The orphan, and the reason the guard asks about `kind` rather than about the source row. + * + * `remove` deletes from two tables and not in one transaction, so a failure between them leaves + * exactly this: a governance row of this surface's own kind with nothing behind it. It is the + * "catalogue disagrees with the build" state, it belongs to this surface, and gating on the + * source row instead would have left it with no way to be cleared. + */ + const orphan = `custom_orphan_${suite}`; + await database.insert(components).values({ + name: orphan, + title: "A draft whose source went", + kind: "sandboxed", + draftDescription: "Authored here.", + published: false, + updatedBy: "admin@openbot.local", + }); + + await store.remove(orphan, "admin@openbot.local"); + + const [gone] = await database + .select() + .from(components) + .where(eq(components.name, orphan)); + expect(gone).toBeUndefined(); + }); +});