diff --git a/server/src/agents/callback-token.ts b/server/src/agents/callback-token.ts index 8f95f38cd..eac5df1d2 100644 --- a/server/src/agents/callback-token.ts +++ b/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; +}; + +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, + }, + }; +} + /** * May this call proceed, and as whom? * diff --git a/server/src/app.ts b/server/src/app.ts index c89789acf..705bc1b38 100644 --- a/server/src/app.ts +++ b/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, diff --git a/server/tests/agent-tool-call-input.test.ts b/server/tests/agent-tool-call-input.test.ts new file mode 100644 index 000000000..17f99506c --- /dev/null +++ b/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.", + }); + }); +});