diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d39d387..62ad8aa6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,15 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### A component whose name has a stray space is the same component + +The catalogue announcement asked whether each component's `name`, `title`, `kind` and `description` +were more than whitespace, and then published the untrimmed strings. A `name` is a component's +identity -- it is what `syncCatalogue` compares against what is already published, what `decide` and +`listForAgent` look up, and what a grant names -- so a build shipping `" weatherPanel "` added a +second catalogue row beside `weatherPanel`: published, ungranted by anybody, and impossible to hold +a Bot back from under the name people use. The four fields are now stored as the strings the guard +approved. ### A password with a `%` in it says so, instead of failing as `URI error` `DATABASE_URL` is taken apart before it reaches Bun, and each part is percent-decoded. A part diff --git a/server/src/components/routes.ts b/server/src/components/routes.ts index 970ea812..a858bd2a 100644 --- a/server/src/components/routes.ts +++ b/server/src/components/routes.ts @@ -105,7 +105,19 @@ export function createComponentRoutes( ) { return []; } - return [{ name, title, kind, description }]; + // Trimmed, because that is the string the guard above just approved. A component's `name` is + // its identity -- `syncCatalogue` compares it against what is already published, `decide` and + // `listForAgent` look it up by it, and a grant names it -- so publishing " weatherPanel " + // adds a second component beside `weatherPanel` that nobody has granted and no Bot can be + // held back from by the name people use. + return [ + { + name: name.trim(), + title: title.trim(), + kind: kind.trim(), + description: description.trim(), + }, + ]; }); const { added } = await store.syncCatalogue(valid); diff --git a/server/tests/component-catalogue.test.ts b/server/tests/component-catalogue.test.ts new file mode 100644 index 00000000..a46fb9cb --- /dev/null +++ b/server/tests/component-catalogue.test.ts @@ -0,0 +1,109 @@ +import { describe, expect, test } from "bun:test"; +import type { MiddlewareHandler } from "hono"; +import { Hono } from "hono"; +import type { AppVariables } from "../src/auth/guards"; +import { createComponentRoutes } from "../src/components/routes"; +import type { CatalogueEntry, ComponentStore } from "../src/components/store"; + +/** + * What a build's announcement is allowed to put in the catalogue. + * + * A component's `name` is its identity: `syncCatalogue` compares it against the names already + * published, `decide` and `listForAgent` look it up by it, and a grant names it. A name that + * differs from another only by the spaces around it is therefore a second, separate component that + * nobody granted and no Bot can be held back from by the name people actually use. + */ + +const asSignedIn: MiddlewareHandler<{ Variables: AppVariables }> = async ( + context, + next, +) => { + context.set("actor", { id: "u1", email: "someone@openbot.test" }); + return next(); +}; + +function harness() { + const published: CatalogueEntry[] = []; + const store = { + syncCatalogue: async (entries: CatalogueEntry[]) => { + published.push(...entries); + return { added: entries.map((entry) => entry.name) }; + }, + } as unknown as ComponentStore; + + const app = new Hono().route( + "/components", + createComponentRoutes(store, asSignedIn, undefined, async () => true), + ); + + return { + published, + announce: (components: unknown[]) => + app.request("http://openbot.local/components/catalogue", { + method: "PUT", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ components }), + }), + }; +} + +function entry(over: Record = {}) { + return { + name: "weatherPanel", + title: "Weather", + kind: "panel", + description: "The forecast where the reader is.", + ...over, + }; +} + +describe("a build announcing what it can draw", () => { + test("publishes an ordinary component unchanged", async () => { + const { published, announce } = harness(); + await announce([entry()]); + expect(published).toEqual([ + { + name: "weatherPanel", + title: "Weather", + kind: "panel", + description: "The forecast where the reader is.", + }, + ]); + }); + + test("publishes a padded name as the name, not as a second component", async () => { + // The guard already asks whether the name is more than whitespace; it asks that of the trimmed + // string and then publishes the untrimmed one, so " weatherPanel " and "weatherPanel" are two + // catalogue rows and only one of them is the component anybody grants. + const { published, announce } = harness(); + await announce([entry({ name: " weatherPanel " })]); + expect(published[0]?.name).toBe("weatherPanel"); + }); + + test("publishes a padded title and description as the reader will see them", async () => { + const { published, announce } = harness(); + await announce([ + entry({ title: " Weather\t", description: "\n The forecast. " }), + ]); + expect(published[0]?.title).toBe("Weather"); + expect(published[0]?.description).toBe("The forecast."); + }); + + test("publishes a padded kind as the kind", async () => { + const { published, announce } = harness(); + await announce([entry({ kind: " panel " })]); + expect(published[0]?.kind).toBe("panel"); + }); + + test("still refuses an entry that is only whitespace", async () => { + const { published, announce } = harness(); + await announce([entry({ name: " " })]); + expect(published).toEqual([]); + }); + + test("still refuses a list that is not a list", async () => { + const { announce } = harness(); + const response = await announce(undefined as unknown as unknown[]); + expect(response.status).toBe(400); + }); +});