Refuse a non-string tool name or args on POST /api/agent-tools/call with 400

server/src/agents/callback-token.ts

@@ -216,6 +216,65 @@ export type CallVerdict =
   | { ok: true; botId: string; actorId: string }
   | { ok: false; status: 401 | 403; reason: string };
 
+/**
+ * What a framework Bot asks to run, parsed out of the request body.
+ *
+ * The route used to check `if (!body?.name)` and then call `body.name.replace`, so a
+ * non-string name (`123`, `{}`, `["mcp__x"]`) passed the truthiness guard and threw
+ * `TypeError: body.name.replace is not a function` inside the `try`, which the catch
+ * answered as a 200 tool refusal with the marker text. A caller error looked like a tool
+ * saying no, with no audit row and no 400. `args` had the same shape problem: anything
+ * non-null (`"str"`, `42`, `[]`) flowed straight into `callTool` as a `Record`.
+ *
+ * Parsed here, pure and unit-tested, so the route answers 400 before auth output or the
+ * store ever sees the values. The `mcp__server__tool` to `server/tool` mapping lives here
+ * too, so the route never calls a string method on untrusted input again.
+ */
+export type AgentToolCallInput = {
+  /** The store-shaped tool ref, with the `mcp__` prefix mapped. */
+  ref: string;
+  /** The tool arguments, defaulting to `{}` when absent. */
+  args: Record<string, unknown>;
+};
+
+export function parseAgentToolCallInput(
+  body: unknown,
+): { ok: true; value: AgentToolCallInput } | { ok: false; error: string } {
+  if (!body || typeof body !== "object" || Array.isArray(body)) {
+    return { ok: false, error: "A tool is required." };
+  }
+  const name = (body as { name?: unknown }).name;
+  if (typeof name !== "string" || !name.trim()) {
+    return { ok: false, error: "A tool is required." };
+  }
+  const rawArgs = (body as { args?: unknown }).args;
+  if (rawArgs === undefined) {
+    return {
+      ok: true,
+      value: {
+        ref: name
+          .trim()
+          .replace(/^mcp__/, "")
+          .replace("__", "/"),
+        args: {},
+      },
+    };
+  }
+  if (!rawArgs || typeof rawArgs !== "object" || Array.isArray(rawArgs)) {
+    return { ok: false, error: "Tool arguments must be an object." };
+  }
+  return {
+    ok: true,
+    value: {
+      ref: name
+        .trim()
+        .replace(/^mcp__/, "")
+        .replace("__", "/"),
+      args: rawArgs as Record<string, unknown>,
+    },
+  };
+}
+
 /**
  * May this call proceed, and as whom?
  *

server/src/app.ts

@@ -3,7 +3,11 @@ import { Hono } from "hono";
 import { bodyLimit } from "hono/body-limit";
 import { serveStatic } from "hono/bun";
 import { MAX_IMAGE_BYTES } from "../../shared/attachments";
-import { authoriseAgentCall, sameToken } from "./agents/callback-token";
+import {
+  authoriseAgentCall,
+  parseAgentToolCallInput,
+  sameToken,
+} from "./agents/callback-token";
 import type { BotAccessCheck } from "./agents/profile-policy";
 import type { AgentProfileStore } from "./agents/profile-store";
 import { createAgentRoutes } from "./agents/routes";
@@ -1285,15 +1289,15 @@ export function createApp(
         return context.json({ error: verdict.reason }, verdict.status);
       }
 
-      if (!body?.name) {
-        return context.json({ error: "A tool is required." }, 400);
+      const parsedCall = parseAgentToolCallInput(body);
+      if (!parsedCall.ok) {
+        return context.json({ error: parsedCall.error }, 400);
       }
 
       try {
         const result = await pluginStore.callTool({
-          // The model is offered `mcp__server__tool`; the store speaks `server/tool`.
-          ref: body.name.replace(/^mcp__/, "").replace("__", "/"),
-          args: body.args ?? {},
+          ref: parsedCall.value.ref,
+          args: parsedCall.value.args,
           botId: verdict.botId,
           // From the assertion, never the body: this is the name the audit row will carry.
           actorId: verdict.actorId,

server/tests/agent-tool-call-input.test.ts

@@ -0,0 +1,110 @@
+import { describe, expect, test } from "bun:test";
+import { parseAgentToolCallInput } from "../src/agents/callback-token";
+
+/**
+ * The shape a framework Bot's tool call arrives in, nailed down.
+ *
+ * The route used to guard with `if (!body?.name)` and then call `body.name.replace`,
+ * so a truthy non-string (`123`, `{}`, `["mcp__x"]`) sailed through and threw a TypeError
+ * inside the try, which the catch returned as a 200 refusal with the marker text. A caller
+ * error looked like a tool saying no. `args` was never checked at all.
+ */
+
+describe("parseAgentToolCallInput", () => {
+  test("accepts a plain tool name with absent args", () => {
+    expect(
+      parseAgentToolCallInput({ name: "server/tool", run: "signed" }),
+    ).toEqual({
+      ok: true,
+      value: { ref: "server/tool", args: {} },
+    });
+  });
+
+  test("trims the name and maps the mcp__ prefix the model is offered", () => {
+    expect(
+      parseAgentToolCallInput({
+        name: "  mcp__server__tool  ",
+        args: { q: 1 },
+        run: "signed",
+      }),
+    ).toEqual({
+      ok: true,
+      value: { ref: "server/tool", args: { q: 1 } },
+    });
+  });
+
+  test("keeps extra body fields out of the parsed value", () => {
+    const parsed = parseAgentToolCallInput({
+      name: "server/tool",
+      args: {},
+      run: "signed",
+      botId: "forged",
+      actorId: "forged",
+    });
+    expect(parsed).toEqual({
+      ok: true,
+      value: { ref: "server/tool", args: {} },
+    });
+  });
+
+  test.each([[null], [undefined], ["name"], [42], [true], [[]]])(
+    "refuses a non-object body: %p",
+    (body) => {
+      expect(parseAgentToolCallInput(body)).toEqual({
+        ok: false,
+        error: "A tool is required.",
+      });
+    },
+  );
+
+  test.each([
+    ["missing", {}, "A tool is required."],
+    ["null", { name: null }, "A tool is required."],
+    ["a number", { name: 123 }, "A tool is required."],
+    ["an object", { name: {} }, "A tool is required."],
+    ["an array", { name: ["mcp__server__tool"] }, "A tool is required."],
+    ["a boolean", { name: true }, "A tool is required."],
+    ["empty", { name: "" }, "A tool is required."],
+    ["whitespace", { name: "   " }, "A tool is required."],
+  ])("refuses %s name with 400", (_label, body, error) => {
+    expect(parseAgentToolCallInput(body)).toEqual({ ok: false, error });
+  });
+
+  test.each([
+    ["a string", "str"],
+    ["a number", 42],
+    ["null", null],
+    ["an array", []],
+    ["an array of pairs", [["q", 1]]],
+    ["a boolean", true],
+  ])("refuses %s args with 400", (_label, args) => {
+    expect(parseAgentToolCallInput({ name: "server/tool", args })).toEqual({
+      ok: false,
+      error: "Tool arguments must be an object.",
+    });
+  });
+
+  test("accepts an empty object for args", () => {
+    expect(parseAgentToolCallInput({ name: "s/t", args: {} })).toEqual({
+      ok: true,
+      value: { ref: "s/t", args: {} },
+    });
+  });
+
+  test("a name that trims to nothing is still a missing tool", () => {
+    expect(parseAgentToolCallInput({ name: "\n\t " })).toEqual({
+      ok: false,
+      error: "A tool is required.",
+    });
+  });
+
+  test("never calls a string method on the input: numbers do not throw", () => {
+    expect(() =>
+      parseAgentToolCallInput({ name: 123, args: 42 }),
+    ).not.toThrow();
+    expect(parseAgentToolCallInput({ name: 123, args: 42 })).toEqual({
+      ok: false,
+      error: "A tool is required.",
+    });
+  });
+});