From 724e61277544f7da013305e3dd787279b3dcc0fe Mon Sep 17 00:00:00 2001 From: Ayush7614 Date: Sun, 13 Sep 2026 10:45:00 +0530 Subject: [PATCH] Refuse a malformed catalogue entry on PUT /api/components/catalogue with 400 --- server/src/components/routes.ts | 48 +++++-- .../tests/component-catalogue-entries.test.ts | 135 ++++++++++++++++++ server/tests/component-catalogue.test.ts | 9 +- 3 files changed, 178 insertions(+), 14 deletions(-) create mode 100644 server/tests/component-catalogue-entries.test.ts diff --git a/server/src/components/routes.ts b/server/src/components/routes.ts index a858bd2a2..79ae65049 100644 --- a/server/src/components/routes.ts +++ b/server/src/components/routes.ts @@ -87,8 +87,29 @@ export function createComponentRoutes( return context.json({ error: "A list of components is required." }, 400); } - const valid = entries.flatMap((entry) => { - if (!entry || typeof entry !== "object") return []; + /* + * All or nothing, and a 400 names the entry. This used to drop malformed entries and + * answer 200 with whatever was left, so a deploy that typo'd `kind` as an object or + * sent a blank `description` got a success response while publishing nothing: `{added: []}` + * is also what "already in sync" looks like. The operator found out from a missing + * component, not from the API. A build announcing an empty catalogue sends `[]`, which + * still syncs to nothing and answers 200. + */ + const valid: { + name: string; + title: string; + kind: string; + description: string; + }[] = []; + for (const [index, entry] of entries.entries()) { + if (!entry || typeof entry !== "object") { + return context.json( + { + error: `Component at index ${index} needs a name, a title, a kind and a description.`, + }, + 400, + ); + } const { name, title, kind, description } = entry as Record< string, unknown @@ -103,22 +124,25 @@ export function createComponentRoutes( typeof description !== "string" || !description.trim() ) { - return []; + return context.json( + { + error: `Component at index ${index} needs a name, a title, a kind and a description.`, + }, + 400, + ); } // 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(), - }, - ]; - }); + valid.push({ + name: name.trim(), + title: title.trim(), + kind: kind.trim(), + description: description.trim(), + }); + } const { added } = await store.syncCatalogue(valid); // Only arrivals are recorded. Announcing happens on every page load, and a row per load would diff --git a/server/tests/component-catalogue-entries.test.ts b/server/tests/component-catalogue-entries.test.ts new file mode 100644 index 000000000..e313918b2 --- /dev/null +++ b/server/tests/component-catalogue-entries.test.ts @@ -0,0 +1,135 @@ +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"; + +/** + * A build's announcement is a claim about what exists, so a malformed entry is a 400. + * + * The route used to drop entries that failed the shape check and answer 200 with the rest, + * so `{"components": [{"name": 123}, "oops", null]}` returned `{added: []}` — the same body + * as "already in sync". A deploy that typo'd a field published nothing and was told success. + * An empty list still means "nothing to announce" and answers 200; anything present must + * be complete, and the error names its index. + */ + +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("announcing a catalogue with malformed entries", () => { + test("an empty list still syncs to nothing with 200", async () => { + const { published, announce } = harness(); + const response = await announce([]); + expect(response.status).toBe(200); + await expect(response.json()).resolves.toEqual({ added: [] }); + expect(published).toEqual([]); + }); + + test("a two-entry list with one malformed entry syncs neither", async () => { + const { published, announce } = harness(); + const response = await announce([entry(), entry({ kind: {} })]); + expect(response.status).toBe(400); + await expect(response.json()).resolves.toEqual({ + error: + "Component at index 1 needs a name, a title, a kind and a description.", + }); + expect(published).toEqual([]); + }); + + test.each([ + ["a number", 123], + ["a string", "oops"], + ["null", null], + ["an array", []], + ])("refuses a non-object entry %s at its index", async (_name, bad) => { + const { published, announce } = harness(); + const response = await announce([entry(), bad]); + expect(response.status).toBe(400); + await expect(response.json()).resolves.toEqual({ + error: + "Component at index 1 needs a name, a title, a kind and a description.", + }); + expect(published).toEqual([]); + }); + + test.each([ + ["a numeric name", { name: 123 }], + ["a null title", { title: null }], + ["a blank kind", { kind: " " }], + ["an empty description", { description: "" }], + ["a missing description", { description: undefined }], + ["an object kind", { kind: {} }], + ])("refuses an entry with %s at index 0", async (_name, over) => { + const { published, announce } = harness(); + const clean = entry(); + const body = { ...clean }; + for (const [key, value] of Object.entries(over)) { + if (value === undefined) delete body[key as keyof typeof body]; + else (body as Record)[key] = value; + } + const response = await announce([body]); + expect(response.status).toBe(400); + await expect(response.json()).resolves.toEqual({ + error: + "Component at index 0 needs a name, a title, a kind and a description.", + }); + expect(published).toEqual([]); + }); + + test("three valid entries still publish together", async () => { + const { published, announce } = harness(); + const response = await announce([ + entry(), + entry({ name: "newsPanel", title: "News" }), + entry({ name: "clockPanel", title: "Clock" }), + ]); + expect(response.status).toBe(200); + expect(published.map((row) => row.name)).toEqual([ + "weatherPanel", + "newsPanel", + "clockPanel", + ]); + }); +}); diff --git a/server/tests/component-catalogue.test.ts b/server/tests/component-catalogue.test.ts index a46fb9cbf..beb4acae5 100644 --- a/server/tests/component-catalogue.test.ts +++ b/server/tests/component-catalogue.test.ts @@ -95,9 +95,14 @@ describe("a build announcing what it can draw", () => { expect(published[0]?.kind).toBe("panel"); }); - test("still refuses an entry that is only whitespace", async () => { + test("refuses an entry that is only whitespace instead of dropping it", async () => { const { published, announce } = harness(); - await announce([entry({ name: " " })]); + const response = await announce([entry({ name: " " })]); + expect(response.status).toBe(400); + await expect(response.json()).resolves.toEqual({ + error: + "Component at index 0 needs a name, a title, a kind and a description.", + }); expect(published).toEqual([]); });