diff --git a/server/src/components/routes.ts b/server/src/components/routes.ts index 7de833cc..cbcb3117 100644 --- a/server/src/components/routes.ts +++ b/server/src/components/routes.ts @@ -159,7 +159,8 @@ export function createComponentRoutes( agentId?: unknown; functions?: unknown; } | null; - const agentId = typeof body?.agentId === "string" ? body.agentId : ""; + const agentId = + typeof body?.agentId === "string" ? body.agentId.trim() : ""; if (!agentId) { return context.json({ error: "The Bot is required." }, 400); } @@ -168,11 +169,29 @@ export function createComponentRoutes( if (!(await canUseBot(context.var.actor, agentId))) { return context.json({ error: "There is no such Bot." }, 404); } - const functions = Array.isArray(body?.functions) - ? body.functions.filter( - (entry): entry is string => typeof entry === "string", - ) - : []; + /* + * Every entry, or a 400. This used to filter non-strings out, so + * `{"functions": [123, null, {}]}` became `[]`, the loop below never ran, and a + * governance question about X and Y was answered `allowed: true` because X and Y + * were not strings. A caller asking "may it call these" must get a verdict about the + * ones it named, not about none of them. Absent still means none. + */ + const rawFunctions = body?.functions; + if ( + rawFunctions !== undefined && + (!Array.isArray(rawFunctions) || + rawFunctions.some( + (entry) => typeof entry !== "string" || !entry.trim(), + )) + ) { + return context.json( + { error: "Functions must be a list of function names." }, + 400, + ); + } + const functions = ( + Array.isArray(rawFunctions) ? rawFunctions : [] + ) as string[]; const decision = await store.decide(name, agentId); if (!decision.allowed) { diff --git a/server/tests/component-decision-functions.test.ts b/server/tests/component-decision-functions.test.ts new file mode 100644 index 00000000..9f7bf253 --- /dev/null +++ b/server/tests/component-decision-functions.test.ts @@ -0,0 +1,119 @@ +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 { ComponentStore } from "../src/components/store"; + +/** + * A governance question must be answered about what was asked. + * + * The route used to filter non-string `functions` out, so asking about `[123, null, {}]` + * became asking about `[]`: the loop never ran and the answer was `allowed: true`. A + * caller that meant "may it call X, Y" got "yes" because X and Y were not strings. A + * whitespace Bot id had the same shape problem one line up: `" "` passed the presence + * check and fell through to a 404 instead of the 400 a blank Bot deserves. + */ + +const GRANTED = "recentRefusals"; +const WITHHELD = "botActivity"; + +function harness() { + const store = { + decide: async () => ({ allowed: true as const, description: "Published." }), + mayCall: async (_name: string, functionName: string) => + functionName === GRANTED, + } as unknown as ComponentStore; + + const asSignedIn: MiddlewareHandler<{ Variables: AppVariables }> = async ( + context, + next, + ) => { + context.set("actor", { id: "u1", email: "someone@openbot.test" }); + return next(); + }; + + const hono = new Hono().route( + "/components", + createComponentRoutes(store, asSignedIn, undefined, async () => true), + ); + return hono; +} + +async function post(body: unknown) { + const response = await harness().request( + "http://t/components/showActivityReport/decision", + { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }, + ); + return { status: response.status, json: (await response.json()) as unknown }; +} + +describe("deciding a component with functions", () => { + test("allows a granted function", async () => { + expect( + await post({ agentId: "risk-analyst", functions: [GRANTED] }), + ).toEqual({ + status: 200, + json: { allowed: true }, + }); + }); + + test("refuses a withheld function and names it", async () => { + const result = await post({ + agentId: "risk-analyst", + functions: [WITHHELD], + }); + expect(result.status).toBe(200); + expect(result.json).toMatchObject({ allowed: false }); + expect(JSON.stringify(result.json)).toContain(WITHHELD); + }); + + test("an absent functions list still asks about the component alone", async () => { + expect(await post({ agentId: "risk-analyst" })).toEqual({ + status: 200, + json: { allowed: true }, + }); + }); + + test("a padded Bot id is read for the Bot it names", async () => { + expect(await post({ agentId: " risk-analyst " })).toEqual({ + status: 200, + json: { allowed: true }, + }); + }); + + test.each([[null], [123], [" "], ["\n\t "]])( + "refuses a blank Bot id %p with 400", + async (agentId) => { + const result = await post({ agentId }); + expect(result.status).toBe(400); + expect(result.json).toEqual({ error: "The Bot is required." }); + }, + ); + + test.each([ + ["a string", "recentRefusals"], + ["a number", 123], + ["an object", {}], + ["null", null], + ["a number entry", [123]], + ["a null entry", [null]], + ["an object entry", [{}]], + ["a mixed list", [GRANTED, 123]], + ["a blank entry", [" "]], + ["an empty entry", [""]], + ])( + "refuses %s functions with 400 instead of answering allowed:true", + async (_name, functions) => { + const result = await post({ agentId: "risk-analyst", functions }); + expect(result.status).toBe(400); + expect(result.json).toEqual({ + error: "Functions must be a list of function names.", + }); + }, + ); +}); diff --git a/server/tests/component-decision.test.ts b/server/tests/component-decision.test.ts index a16b8aab..bca0f167 100644 --- a/server/tests/component-decision.test.ts +++ b/server/tests/component-decision.test.ts @@ -82,9 +82,21 @@ describe("deciding a component", () => { expect(decision.allowed).toBe(false); }); - test("ignores anything in the list that is not a name", async () => { - expect( - await decide({ agentId: "risk-analyst", functions: [1, null, {}] }), - ).toEqual({ allowed: true }); + test("refuses a list with anything that is not a name instead of ignoring it", async () => { + const response = await app().request( + "http://openbot.local/components/showActivityReport/decision", + { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + agentId: "risk-analyst", + functions: [1, null, {}], + }), + }, + ); + expect(response.status).toBe(400); + await expect(response.json()).resolves.toEqual({ + error: "Functions must be a list of function names.", + }); }); });