Refuse non-string functions on POST /api/components/:name/decision with 400

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) {

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.",
+      });
+    },
+  );
+});

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.",
+    });
   });
 });