diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d8eeb23..02d1aa49 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,14 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### Revoking a grant with a blank ref or Bot is refused instead of reported as done + +`DELETE /api/plugins/grants` checked its query params with truthiness, and a query param is +always a string: `?ref=%20%20` is truthy, so it skipped the 400, deleted zero rows by exact +match, still wrote a `plugin_revoked` audit row naming whitespace, and answered `ok:true`. The +`POST` twin already required trimmed non-empty strings. `DELETE` requires the same now and acts +on the trimmed values, so a blank ref or Bot is a 400 with the same message, no delete, and no +audit row. ### The Bot in the box and the LangGraph Bot read a message that has a file attached A message with a file attached reached both Bots as `[object Object],[object Object]`, in place of diff --git a/server/src/plugins/routes.ts b/server/src/plugins/routes.ts index df7918e9..f02f5227 100644 --- a/server/src/plugins/routes.ts +++ b/server/src/plugins/routes.ts @@ -823,22 +823,36 @@ export function createPluginRoutes( const kind = asGrantKind(context.req.query("kind")); const ref = context.req.query("ref"); const agentId = context.req.query("agentId"); - if (!kind || !ref || !agentId) { + /* + * Query params are always strings, so truthiness is not enough: `" "` is truthy and used + * to pass this check, delete zero rows by exact match, still write a `plugin_revoked` audit + * row naming whitespace, and answer `ok:true`. The POST twin already requires non-empty + * strings; this requires the same and acts on the trimmed values. + */ + if ( + !kind || + typeof ref !== "string" || + !ref.trim() || + typeof agentId !== "string" || + !agentId.trim() + ) { return context.json( { error: "A kind, a ref and a Bot are required." }, 400, ); } + const trimmedRef = ref.trim(); + const trimmedAgentId = agentId.trim(); const refusal = await enablementRefusal( context, kind, - ref, - agentId, + trimmedRef, + trimmedAgentId, "revoke", ); if (refusal) return context.json({ error: refusal }, 403); - await store.revoke(kind, ref, agentId, actorEmail(context)); + await store.revoke(kind, trimmedRef, trimmedAgentId, actorEmail(context)); return context.json({ ok: true }); }); diff --git a/server/tests/plugin-grants-validation.test.ts b/server/tests/plugin-grants-validation.test.ts index 1a022160..e3fbe3f0 100644 --- a/server/tests/plugin-grants-validation.test.ts +++ b/server/tests/plugin-grants-validation.test.ts @@ -5,12 +5,19 @@ import type { BotAccessCheck } from "../src/plugins/routes"; import { createPluginRoutes } from "../src/plugins/routes"; import type { PluginStore } from "../src/plugins/store"; -function appWith(calls: { grants: unknown[]; toolCalls: unknown[] }) { +function appWith(calls: { + grants: unknown[]; + toolCalls: unknown[]; + revokes?: unknown[]; +}) { const store = { grant: async (kind: unknown, ref: unknown, agentId: unknown) => { calls.grants.push({ kind, ref, agentId }); return { ok: true }; }, + revoke: async (kind: unknown, ref: unknown, agentId: unknown) => { + calls.revokes?.push({ kind, ref, agentId }); + }, callTool: async (input: unknown) => { calls.toolCalls.push(input); return { ok: true }; @@ -65,6 +72,79 @@ describe("POST /api/plugins/grants", () => { }); }); +describe("DELETE /api/plugins/grants", () => { + /** + * Query params are always strings, so truthiness is not enough. + * + * `?ref=%20%20` is truthy and used to pass the presence check, delete zero rows by exact + * match, still write a `plugin_revoked` audit row naming whitespace, and answer `ok:true`. + * The POST twin already requires trimmed non-empty strings; DELETE requires the same and + * acts on the trimmed values. + */ + test.each([ + ["a whitespace ref", "?kind=mcp&ref=%20%20%20&agentId=bot-1"], + ["a whitespace agentId", "?kind=mcp&ref=tool&agentId=%20%20"], + ["a missing ref", "?kind=mcp&agentId=bot-1"], + ["a missing agentId", "?kind=mcp&ref=tool"], + ["a missing kind", "?ref=tool&agentId=bot-1"], + ])("refuses %s with 400 and never reaches the store", async (_n, query) => { + const calls = { + grants: [] as unknown[], + toolCalls: [] as unknown[], + revokes: [] as unknown[], + }; + const response = await appWith(calls).request( + `http://openbot.test/grants${query}`, + { method: "DELETE" }, + ); + + expect(response.status).toBe(400); + await expect(response.json()).resolves.toEqual({ + error: "A kind, a ref and a Bot are required.", + }); + expect(calls.revokes).toEqual([]); + }); + + test("a valid revoke still deletes and trims the values it acts on", async () => { + const calls = { + grants: [] as unknown[], + toolCalls: [] as unknown[], + revokes: [] as unknown[], + }; + const response = await appWith(calls).request( + "http://openbot.test/grants?kind=mcp&ref=%20tool%20&agentId=%20bot-1%20", + { method: "DELETE" }, + ); + + expect(response.status).toBe(200); + expect(calls.revokes).toEqual([ + { kind: "mcp", ref: "tool", agentId: "bot-1" }, + ]); + }); +}); + +describe("POST /api/plugins/call", () => { + test.each([ + ["a number ref", { ref: 123, agentId: "bot-1" }], + ["an object ref", { ref: {}, agentId: "bot-1" }], + ["a number agentId", { ref: "tool", agentId: 456 }], + ["a whitespace ref", { ref: " ", agentId: "bot-1" }], + ])("refuses %s with 400 and never reaches the store", async (_n, body) => { + const calls = { grants: [] as unknown[], toolCalls: [] as unknown[] }; + const response = await appWith(calls).request("http://openbot.test/call", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }); + + expect(response.status).toBe(400); + await expect(response.json()).resolves.toEqual({ + error: "A tool and a Bot are required.", + }); + expect(calls.toolCalls).toEqual([]); + }); +}); + describe("POST /api/plugins/skills", () => { function skillsApp(calls: { installs: unknown[] }) { const store = { @@ -161,25 +241,3 @@ describe("POST /api/plugins/skills", () => { expect(calls.installs).toHaveLength(1); }); }); - -describe("POST /api/plugins/call", () => { - test.each([ - ["a number ref", { ref: 123, agentId: "bot-1" }], - ["an object ref", { ref: {}, agentId: "bot-1" }], - ["a number agentId", { ref: "tool", agentId: 456 }], - ["a whitespace ref", { ref: " ", agentId: "bot-1" }], - ])("refuses %s with 400 and never reaches the store", async (_n, body) => { - const calls = { grants: [] as unknown[], toolCalls: [] as unknown[] }; - const response = await appWith(calls).request("http://openbot.test/call", { - method: "POST", - headers: { "content-type": "application/json" }, - body: JSON.stringify(body), - }); - - expect(response.status).toBe(400); - await expect(response.json()).resolves.toEqual({ - error: "A tool and a Bot are required.", - }); - expect(calls.toolCalls).toEqual([]); - }); -});