Refuse a blank grant ref or Bot on revoke, like grant does

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

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 });
   });
 

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([]);
-  });
-});