diff --git a/CHANGELOG.md b/CHANGELOG.md index fc493d2d0..9febb9a50 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,16 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### A playground component's Published switch publishes its source too + +The Published switch on an admin component page called the generic publication endpoint for every +kind. For a browser-authored component that endpoint promoted the description and marked it +published without copying the playground draft, so Bots were offered a component the renderer could +not draw. Playground components now publish and withdraw both rows in one transaction; a missing or +empty description or HTML is refused instead of leaving a half-published component, and repeating +an unchanged publish no longer advances the revision. + + ### A proxy password containing `%` no longer stops every shell command A proxy password with a `%` that does not start an escape, such as `p%zz`, made decoding it throw. diff --git a/server/src/app.ts b/server/src/app.ts index a3b04854f..ca7f16a7a 100644 --- a/server/src/app.ts +++ b/server/src/app.ts @@ -1537,7 +1537,13 @@ export function createApp( if (componentStore) { app.route( "/api/components", - createComponentRoutes(componentStore, requireUser, auditStore, canUseBot), + createComponentRoutes( + componentStore, + requireUser, + auditStore, + canUseBot, + sandboxedStore, + ), ); } diff --git a/server/src/components/routes.ts b/server/src/components/routes.ts index f7a1e4228..8c0e95a8e 100644 --- a/server/src/components/routes.ts +++ b/server/src/components/routes.ts @@ -6,7 +6,16 @@ import { recordAuditEvent } from "../audit"; import type { AppVariables } from "../auth/guards"; import { requireAdmin } from "../auth/guards"; import { DATA_FUNCTIONS, dataFunction } from "./functions"; -import { ComponentNotFoundError, type ComponentStore } from "./store"; +import { + SandboxedNotFoundError, + SandboxedPublicationRefusedError, + type SandboxedStore, +} from "./sandboxed"; +import { + ComponentNotFoundError, + type ComponentStore, + SandboxedPublicationRequiredError, +} from "./store"; /** * The local development actor, which is not a row in `users`. @@ -39,6 +48,14 @@ export function createComponentRoutes( * behind `requireAdmin`. */ canUseBot: BotAccessCheck, + /** + * The source half of a playground component's publication. + * + * The generic switch is the only publication UI for both kinds, so this route owns the choice to + * delegate. The component store still refuses a generic write for a sandboxed row, which keeps a + * caller that reaches `publish` directly from creating the same half-published state. + */ + sandboxedStore?: SandboxedStore, ) { const routes = new Hono<{ Variables: AppVariables }>(); @@ -485,6 +502,7 @@ export function createComponentRoutes( } const published = body.published; + let recordedBySource = false; try { if (published) { await store.publish(name, context.var.actor.email); @@ -492,12 +510,41 @@ export function createComponentRoutes( await store.unpublish(name, context.var.actor.email); } } catch (error) { - if (error instanceof ComponentNotFoundError) { + if (error instanceof SandboxedPublicationRequiredError) { + if (!sandboxedStore) { + return context.json( + { + error: + "The playground source is unavailable, so the component was not published.", + }, + 409, + ); + } + try { + if (published) { + await sandboxedStore.publish(name, context.var.actor.email); + } else { + await sandboxedStore.unpublish(name, context.var.actor.email); + } + recordedBySource = true; + } catch (sourceError) { + if (sourceError instanceof SandboxedPublicationRefusedError) { + return context.json({ error: sourceError.message }, 409); + } + if (sourceError instanceof SandboxedNotFoundError) { + return context.json({ error: sourceError.message }, 404); + } + throw sourceError; + } + } else if (error instanceof ComponentNotFoundError) { return context.json({ error: error.message }, 404); + } else { + throw error; } - throw error; } + if (recordedBySource) return context.json({ published }); + await audit( context, published ? "component.published" : "component.unpublished", diff --git a/server/src/components/sandboxed.ts b/server/src/components/sandboxed.ts index c9d206565..e900786d1 100644 --- a/server/src/components/sandboxed.ts +++ b/server/src/components/sandboxed.ts @@ -1,3 +1,4 @@ +import { isDeepStrictEqual } from "node:util"; import { asc, eq } from "drizzle-orm"; import { type AuditStore, recordAuditEvent } from "../audit"; import type { Database } from "../db/client"; @@ -73,9 +74,21 @@ export class SandboxedNameRefusedError extends Error { } } +/** A playground component cannot be published as a drawable thing until its source is usable. */ +export class SandboxedPublicationRefusedError extends Error { + constructor(message: string) { + super(message); + this.name = "SandboxedPublicationRefusedError"; + } +} + const iso = (value: Date | string | null): string | null => value === null ? null : value instanceof Date ? value.toISOString() : value; +function sameJson(left: unknown, right: unknown): boolean { + return isDeepStrictEqual(left, right); +} + /** * The one place a sandboxed component's name is decided. * @@ -238,42 +251,156 @@ export function createSandboxedStore( * nobody wants and both would be reachable if this were two endpoints. */ async publish(name: string, by: string): Promise { - const row = await requireRow(name); + const outcome = await database.transaction(async (transaction) => { + const [row] = await transaction + .select() + .from(sandboxedComponents) + .where(eq(sandboxedComponents.name, name)) + .limit(1) + .for("update"); + if (!row) { + throw new SandboxedPublicationRefusedError( + `${name} has no stored playground source to publish.`, + ); + } - await database - .update(sandboxedComponents) - .set({ - publishedDescription: row.draftDescription, - publishedHtml: row.draftHtml, - publishedCss: row.draftCss, - publishedJsFunctions: row.draftJsFunctions, - publishedArgumentSchema: row.draftArgumentSchema, - published: true, - publishedAt: new Date(), - revision: row.revision + 1, - updatedAt: new Date(), - }) - .where(eq(sandboxedComponents.name, name)); + const [governance] = await transaction + .select() + .from(components) + .where(eq(components.name, name)) + .limit(1) + .for("update"); + if (governance?.kind !== "sandboxed") { + throw new SandboxedPublicationRefusedError( + `${name} has no playground governance row to publish.`, + ); + } - await database - .update(components) - .set({ - publishedDescription: row.draftDescription, - published: true, - publishedAt: new Date(), - updatedBy: by, - updatedAt: new Date(), - }) - .where(eq(components.name, name)); + if (!row.draftDescription.trim()) { + throw new SandboxedPublicationRefusedError( + `${name} needs a description before it can be published.`, + ); + } + if (!row.draftHtml.trim()) { + throw new SandboxedPublicationRefusedError( + `${name} needs rendered HTML before it can be published.`, + ); + } + + const unchanged = + row.published && + governance.published && + row.publishedDescription === row.draftDescription && + row.publishedHtml === row.draftHtml && + row.publishedCss === row.draftCss && + row.publishedJsFunctions === row.draftJsFunctions && + sameJson(row.publishedArgumentSchema, row.draftArgumentSchema); + if (unchanged) return { record: toRecord(row), changed: false }; + + const now = new Date(); + const revision = row.revision + 1; + await transaction + .update(sandboxedComponents) + .set({ + publishedDescription: row.draftDescription, + publishedHtml: row.draftHtml, + publishedCss: row.draftCss, + publishedJsFunctions: row.draftJsFunctions, + publishedArgumentSchema: row.draftArgumentSchema, + published: true, + publishedAt: now, + revision, + updatedAt: now, + }) + .where(eq(sandboxedComponents.name, name)); + + await transaction + .update(components) + .set({ + publishedDescription: row.draftDescription, + published: true, + publishedAt: now, + updatedBy: by, + updatedAt: now, + }) + .where(eq(components.name, name)); + + const [updated] = await transaction + .select() + .from(sandboxedComponents) + .where(eq(sandboxedComponents.name, name)) + .limit(1); + if (!updated) { + throw new SandboxedPublicationRefusedError( + `${name} disappeared while it was being published.`, + ); + } + return { record: toRecord(updated), changed: true }; + }); + + if (outcome.changed) { + await recordAuditEvent(auditStore, { + eventType: "component.published", + targetType: "component", + targetId: name, + payload: { + actor: by, + kind: "sandboxed", + revision: outcome.record.revision, + }, + }); + } + return outcome.record; + }, + + /** + * Withdraw a playground component from every Bot. + * + * The source and the governance row are two halves of one publication, so they are withdrawn + * together. The published columns stay put: re-publishing the same draft is a decision to make + * it drawable again, not a request to reconstruct source that was deliberately retained. + */ + async unpublish(name: string, by: string): Promise { + const changed = await database.transaction(async (transaction) => { + const [governance] = await transaction + .select() + .from(components) + .where(eq(components.name, name)) + .limit(1) + .for("update"); + if (governance?.kind !== "sandboxed") { + throw new SandboxedNotFoundError(name); + } + + const [row] = await transaction + .select() + .from(sandboxedComponents) + .where(eq(sandboxedComponents.name, name)) + .limit(1) + .for("update"); + if (!governance.published && !row?.published) return false; + + const now = new Date(); + if (row) { + await transaction + .update(sandboxedComponents) + .set({ published: false, updatedAt: now }) + .where(eq(sandboxedComponents.name, name)); + } + await transaction + .update(components) + .set({ published: false, updatedBy: by, updatedAt: now }) + .where(eq(components.name, name)); + return true; + }); + if (!changed) return; await recordAuditEvent(auditStore, { - eventType: "component.published", + eventType: "component.unpublished", targetType: "component", targetId: name, - payload: { actor: by, kind: "sandboxed", revision: row.revision + 1 }, + payload: { actor: by, kind: "sandboxed" }, }); - - return toRecord(await requireRow(name)); }, async remove(name: string, by: string): Promise { diff --git a/server/src/components/store.ts b/server/src/components/store.ts index 49310f615..c9d818aa2 100644 --- a/server/src/components/store.ts +++ b/server/src/components/store.ts @@ -49,6 +49,21 @@ export class ComponentNotFoundError extends Error { } } +/** + * The generic publication path found a playground component. + * + * Its source is not in this table and a generic write would publish the governance half alone. The + * route catches this and delegates to the sandboxed store, which owns the two-row transaction. + */ +export class SandboxedPublicationRequiredError extends Error { + constructor(name: string) { + super( + `${name} is a playground component and must be published from its source.`, + ); + this.name = "SandboxedPublicationRequiredError"; + } +} + /** What a build says it can draw. The app announces this; the server keeps no copy of its own. */ export type CatalogueEntry = { name: string; @@ -307,6 +322,9 @@ export function createComponentStore(database: Database): ComponentStore { async publish(name, by) { const row = await requireComponent(name); + if (row.kind === "sandboxed") { + throw new SandboxedPublicationRequiredError(name); + } await database .update(components) .set({ @@ -321,7 +339,10 @@ export function createComponentStore(database: Database): ComponentStore { }, async unpublish(name, by) { - await requireComponent(name); + const row = await requireComponent(name); + if (row.kind === "sandboxed") { + throw new SandboxedPublicationRequiredError(name); + } await database .update(components) .set({ published: false, updatedBy: by, updatedAt: new Date() }) diff --git a/server/tests/sandboxed-publication.integration.test.ts b/server/tests/sandboxed-publication.integration.test.ts new file mode 100644 index 000000000..46d7e8299 --- /dev/null +++ b/server/tests/sandboxed-publication.integration.test.ts @@ -0,0 +1,246 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { randomUUID } from "node:crypto"; +import { eq, sql } from "drizzle-orm"; +import type { MiddlewareHandler } from "hono"; +import { Hono } from "hono"; +import { createAuditStore } from "../src/audit"; +import type { AppVariables } from "../src/auth/guards"; +import { createComponentRoutes } from "../src/components/routes"; +import { createSandboxedStore } from "../src/components/sandboxed"; +import { createComponentStore } from "../src/components/store"; +import { createDatabase } from "../src/db/client"; +import { auditEvents, components, sandboxedComponents } from "../src/db/schema"; +import { TEST_POOL, testDatabaseUrl } from "./support/database"; + +/** + * The Published switch on a playground component is the same switch as every other component's, but + * publishing one is not the same act. The source lives beside the governance row, and a publication + * that writes only the governance row offers every Bot a component that cannot draw. + * + * These tests cross the real route and both stores against Postgres. A mocked store could prove the + * route called something; it could not prove the two rows commit together, the revision is stable on + * a retry, or a failed governance write leaves the source unpublished. + */ + +const database = createDatabase(testDatabaseUrl(), TEST_POOL); +const auditStore = createAuditStore(database); +const componentStore = createComponentStore(database); +const sandboxedStore = createSandboxedStore(database, auditStore); +const fixtureNames = new Set(); + +const ADMIN = { + id: `user_${randomUUID().slice(0, 8)}`, + email: "admin@openbot.test", + role: "admin", +} as const; + +const asAdmin: MiddlewareHandler<{ Variables: AppVariables }> = async ( + context, + next, +) => { + context.set("actor", { ...ADMIN }); + await next(); +}; + +const routes = new Hono().route( + "/components", + createComponentRoutes( + componentStore, + asAdmin, + auditStore, + async () => true, + sandboxedStore, + ), +); + +function fixture(label: string) { + const slug = `${label}_${randomUUID().slice(0, 8)}`; + const name = `custom_${slug}`; + fixtureNames.add(name); + return { slug, name }; +} + +function postPublication(name: string, published: boolean) { + return routes.request( + `http://t/components/${encodeURIComponent(name)}/publication`, + { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ published }), + }, + ); +} + +async function saveDraft( + slug: string, + html = "

draft

", + description = "Draws the draft.", +) { + return sandboxedStore.save({ + slug, + title: "A test card", + description, + html, + css: "p { color: red }", + jsFunctions: "", + argumentSchema: { type: "object" }, + sampleArguments: {}, + by: ADMIN.email, + }); +} + +async function governanceOf(name: string) { + const [row] = await database + .select() + .from(components) + .where(eq(components.name, name)); + return row; +} + +async function sourceOf(name: string) { + const [row] = await database + .select() + .from(sandboxedComponents) + .where(eq(sandboxedComponents.name, name)); + return row; +} + +afterEach(async () => { + for (const name of fixtureNames) { + await database + .delete(sandboxedComponents) + .where(eq(sandboxedComponents.name, name)); + await database.delete(components).where(eq(components.name, name)); + } + fixtureNames.clear(); +}); + +describe("the generic Published switch on a sandboxed component", () => { + test("publishes the source and the governance row together", async () => { + const { slug, name } = fixture("publish"); + await saveDraft(slug); + + const response = await postPublication(name, true); + + expect(response.status).toBe(200); + await expect(response.json()).resolves.toEqual({ published: true }); + const source = await sourceOf(name); + const governance = await governanceOf(name); + expect(source?.published).toBeTrue(); + expect(source?.publishedHtml).toBe("

draft

"); + expect(governance?.published).toBeTrue(); + expect(governance?.publishedDescription).toBe("Draws the draft."); + }); + + test("refuses a sandboxed governance row with no source", async () => { + const { name } = fixture("missing"); + await database.insert(components).values({ + name, + title: "A missing source", + kind: "sandboxed", + draftDescription: "Draws something.", + published: false, + updatedBy: ADMIN.email, + }); + + const response = await postPublication(name, true); + + expect(response.status).toBe(409); + expect((await governanceOf(name))?.published).toBeFalse(); + }); + + test("refuses a draft with empty HTML", async () => { + const { slug, name } = fixture("empty"); + await saveDraft(slug, " "); + + const response = await postPublication(name, true); + + expect(response.status).toBe(409); + expect((await governanceOf(name))?.published).toBeFalse(); + const source = await sourceOf(name); + expect(source?.published).toBeFalse(); + expect(source?.publishedHtml).toBeNull(); + }); + + test("refuses a draft with no description", async () => { + const { slug, name } = fixture("descriptionless"); + await saveDraft(slug, "

draft

", " "); + + const response = await postPublication(name, true); + + expect(response.status).toBe(409); + expect((await governanceOf(name))?.published).toBeFalse(); + expect((await sourceOf(name))?.published).toBeFalse(); + }); + + test("a repeated publish keeps one revision and one audit event", async () => { + const { slug, name } = fixture("repeat"); + await saveDraft(slug); + + expect((await postPublication(name, true)).status).toBe(200); + expect((await postPublication(name, true)).status).toBe(200); + + expect((await sourceOf(name))?.revision).toBe(1); + const events = await database + .select() + .from(auditEvents) + .where(eq(auditEvents.targetId, name)); + expect( + events.filter((event) => event.eventType === "component.published"), + ).toHaveLength(1); + }); + + test("a failure after the source write rolls the whole publish back", async () => { + const { slug, name } = fixture("rollback"); + await saveDraft(slug); + const functionName = `reject_${name}`; + const triggerName = `reject_${name}`; + + await database.execute( + sql.raw(` + CREATE FUNCTION ${functionName}() RETURNS trigger LANGUAGE plpgsql AS $$ + BEGIN + IF NEW.name = '${name}' THEN + RAISE EXCEPTION 'forced publication failure'; + END IF; + RETURN NEW; + END + $$; + CREATE TRIGGER ${triggerName} + BEFORE UPDATE ON components + FOR EACH ROW EXECUTE FUNCTION ${functionName}(); + `), + ); + + try { + await expect(sandboxedStore.publish(name, ADMIN.email)).rejects.toThrow(); + + const source = await sourceOf(name); + const governance = await governanceOf(name); + expect(source?.published).toBeFalse(); + expect(source?.publishedHtml).toBeNull(); + expect(source?.revision).toBe(0); + expect(governance?.published).toBeFalse(); + } finally { + await database.execute( + sql.raw(`DROP TRIGGER ${triggerName} ON components`), + ); + await database.execute(sql.raw(`DROP FUNCTION ${functionName}()`)); + } + }); + + test("unpublish withdraws the source and the governance row together", async () => { + const { slug, name } = fixture("unpublish"); + await saveDraft(slug); + await sandboxedStore.publish(name, ADMIN.email); + + const response = await postPublication(name, false); + + expect(response.status).toBe(200); + expect((await sourceOf(name))?.published).toBeFalse(); + expect((await governanceOf(name))?.published).toBeFalse(); + expect( + (await sandboxedStore.published()).map((row) => row.name), + ).not.toContain(name); + }); +});