diff --git a/CHANGELOG.md b/CHANGELOG.md index aae4275bf..939dacd6c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,15 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### A Bot's own decline is only recorded against a Bot the caller may reach + +A Bot reports that it declined a request through the person's session, and the audit row says +`reportedBy: the Bot itself`. The route wrote that row for any agent id in the path, without asking +whether the caller could reach that Bot, so any signed-in person could put a decline, in any words, +against any coworker, one they cannot see included, and an administrator reading the trail would take +it for something the Bot said. The route now asks the store first, as every other route on a Bot +does, and answers not found for a Bot the caller cannot reach, writing nothing. + ### The engine socket the supervisor is given can be pointed somewhere else Compose mounted `/var/run/docker.sock` into the supervisor as a fixed path. That is correct for diff --git a/server/src/agents/routes.ts b/server/src/agents/routes.ts index aec1a557b..dc398f7a5 100644 --- a/server/src/agents/routes.ts +++ b/server/src/agents/routes.ts @@ -215,6 +215,21 @@ export function createAgentRoutes( return context.json({ error: "A reason is required." }, 400); } + /* + * The same question every other route here asks first: is this a Bot the caller may reach? + * + * The row says "reportedBy: the Bot itself", and the Bot reports through the person's session, + * so the trail's only way of knowing the report came from a Bot is that the person could have + * been talking to that Bot. Without this check, any signed-in person could write a decline + * against any id at all, a coworker they cannot see included, and an administrator reading the + * trail would take it for something the Bot said. Not found rather than forbidden, as the store + * answers everywhere else, so the check does not confirm which ids exist. + */ + const agent = await store.get(context.var.actor, agentId); + if (!agent) { + return context.json({ error: "Agent not found." }, 404); + } + if (auditStore) { const actor = context.var.actor; await recordAuditEvent(auditStore, { diff --git a/server/tests/bot-lifecycle-audit.test.ts b/server/tests/bot-lifecycle-audit.test.ts index 00c18128c..2ace6ea3f 100644 --- a/server/tests/bot-lifecycle-audit.test.ts +++ b/server/tests/bot-lifecycle-audit.test.ts @@ -26,6 +26,8 @@ function app(overrides: Record = {}) { }; const store = { + get: async (_actor: unknown, id: string) => + id === "bot-1" ? { id: "bot-1", name: "Sales" } : null, create: async () => ({ id: "bot-1", name: "Sales" }), update: async () => ({ id: "bot-1", name: "Sales" }), duplicate: async () => ({ id: "bot-2", name: "Sales copy" }), @@ -209,6 +211,42 @@ describe("what a Bot is, on the trail", () => { expect(rows).toHaveLength(0); }); + test("a decline is recorded against a Bot the caller may reach", async () => { + // The Bot reports through the person's session, so the row is only worth what that session + // could reach: a decline on a Bot the caller may talk to is the Bot's own word. + const { rows, hono } = app(); + + const response = await hono.request("http://t/api/agents/bot-1/declined", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ reason: "It asked me to delete the ledger." }), + }); + + expect(response.status).toBe(200); + expect(rows[0]?.eventType).toBe("bot.declined"); + expect(rows[0]?.targetId).toBe("bot-1"); + expect(rows[0]?.payload.reportedBy).toBe("the Bot itself"); + }); + + test("a decline against a Bot the caller cannot reach is not found, and writes nothing", async () => { + // Every other route here asks the store first. Without the same question, a signed-in person + // could write "the Bot itself declined" against any id at all and an administrator would read + // it as something the Bot said. + const { rows, hono } = app(); + + const response = await hono.request( + "http://t/api/agents/somebody-elses-bot/declined", + { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ reason: "Forged." }), + }, + ); + + expect(response.status).toBe(404); + expect(rows).toHaveLength(0); + }); + test("a trail that is down does not fail the change", async () => { // The Bot is already updated and the caller has been told so. const failing: AuditStore = {