From c00290560592cba1918dc257839604d20b6a45ea Mon Sep 17 00:00:00 2001 From: Ayush7614 Date: Tue, 15 Sep 2026 14:29:34 +0530 Subject: [PATCH] Refuse mistyped skill tools and whitespace grant ids with 400 POST /skills silently dropped non-string tool entries, so a client bug installed a skill declaring nothing and answered success, and a non-boolean global could mint a deployment-wide skill. Every tool entry must now be a non-empty string and global a boolean. Component grant and function routes accepted whitespace-only ids, writing grant rows and audit rows naming nothing; all three now trim and refuse blanks before the store. --- server/src/components/routes.ts | 13 +- server/src/plugins/routes.ts | 21 +- ...component-grants-skills-validation.test.ts | 192 ++++++++++++++++++ 3 files changed, 222 insertions(+), 4 deletions(-) create mode 100644 server/tests/component-grants-skills-validation.test.ts diff --git a/server/src/components/routes.ts b/server/src/components/routes.ts index 404e1bf9f..84cfdd1cb 100644 --- a/server/src/components/routes.ts +++ b/server/src/components/routes.ts @@ -382,6 +382,11 @@ export function createComponentRoutes( const name = context.req.param("name"); const functionName = context.req.param("function"); + // An empty function name would revoke zero rows yet answer `revoked:true` with an audit row + // naming nothing. Refused at the edge like the grant path. + if (!functionName.trim()) { + return context.json({ error: "A function is required." }, 400); + } await store.revokeFunction(name, functionName); await audit(context, "component.function_revoked", name, { function: functionName, @@ -397,7 +402,9 @@ export function createComponentRoutes( const body = (await context.req.json().catch(() => null)) as { agentId?: unknown; } | null; - const agentId = typeof body?.agentId === "string" ? body.agentId : ""; + // A whitespace-only id is truthy and would be written as a grant row naming nothing. + const agentId = + typeof body?.agentId === "string" ? body.agentId.trim() : ""; if (!agentId) { return context.json({ error: "The Bot is required." }, 400); } @@ -420,6 +427,10 @@ export function createComponentRoutes( const name = context.req.param("name"); const agentId = context.req.param("agentId"); + // Revoking `" "` would delete zero rows yet answer `revoked:true` with an audit row. + if (!agentId.trim()) { + return context.json({ error: "The Bot is required." }, 400); + } try { await store.revoke(name, agentId, context.var.actor.email); } catch (error) { diff --git a/server/src/plugins/routes.ts b/server/src/plugins/routes.ts index f02f52279..b81f85379 100644 --- a/server/src/plugins/routes.ts +++ b/server/src/plugins/routes.ts @@ -622,15 +622,30 @@ export function createPluginRoutes( /* * Absent leaves the declarations alone, so a caller that predates this field does not silently * clear one. An array, including an empty one, says what the skill needs now. + * + * Every entry must be a non-empty string: silently dropping mistyped entries would turn a + * client bug into a skill that declares nothing and answers success. `global` must be a + * boolean when present, so the string `"yes"` cannot create a deployment-wide skill. */ - if (body.tools !== undefined && !Array.isArray(body.tools)) { + if (body.global !== undefined && typeof body.global !== "boolean") { return context.json( - { error: "Tools are a list of serverId/toolName references." }, + { error: "Global must be true or false when it is present." }, 400, ); } + if (body.tools !== undefined) { + if ( + !Array.isArray(body.tools) || + body.tools.some((ref) => typeof ref !== "string" || !ref.trim()) + ) { + return context.json( + { error: "Tools are a list of serverId/toolName references." }, + 400, + ); + } + } const tools = Array.isArray(body.tools) - ? body.tools.filter((ref): ref is string => typeof ref === "string") + ? (body.tools as string[]).map((ref) => ref.trim()) : undefined; try { diff --git a/server/tests/component-grants-skills-validation.test.ts b/server/tests/component-grants-skills-validation.test.ts new file mode 100644 index 000000000..08a855c7e --- /dev/null +++ b/server/tests/component-grants-skills-validation.test.ts @@ -0,0 +1,192 @@ +import { describe, expect, test } from "bun:test"; +import type { MiddlewareHandler } from "hono"; +import { Hono } from "hono"; +import type { AppVariables } from "../src/auth/guards"; +import type { BotAccessCheck } from "../src/plugins/routes"; +import { createPluginRoutes } from "../src/plugins/routes"; +import type { PluginStore } from "../src/plugins/store"; +import { createComponentRoutes } from "../src/components/routes"; + +const requireUser: MiddlewareHandler<{ Variables: AppVariables }> = async ( + context, + next, +) => { + context.set("actor", { + id: "user-1", + email: "user@openbot.test", + role: "admin", + }); + await next(); +}; +const canUseBot: BotAccessCheck = async () => true; + +function pluginAppWith(calls: { skills: unknown[] }) { + const store = { + installSkill: async (input: unknown) => { + calls.skills.push(input); + return { ok: true }; + }, + listSkills: async () => [], + } as unknown as PluginStore; + return createPluginRoutes(store, requireUser, canUseBot); +} + +function componentAppWith(calls: { + grants: unknown[]; + revokes: unknown[]; + revokeFunctions: unknown[]; +}) { + const store = { + grant: async (name: unknown, agentId: unknown) => { + calls.grants.push({ name, agentId }); + }, + revoke: async (name: unknown, agentId: unknown) => { + calls.revokes.push({ name, agentId }); + }, + revokeFunction: async (name: unknown, fn: unknown) => { + calls.revokeFunctions.push({ name, fn }); + }, + }; + const app = new Hono<{ Variables: AppVariables }>(); + app.use(requireUser); + app.route( + "/", + createComponentRoutes(store as never, requireUser, undefined, canUseBot), + ); + return app; +} + +const skillBody = { + slug: "my-skill", + title: "My skill", + instructions: "Do the thing.", +}; + +/** + * `tools` used to silently drop mistyped entries, so `{"tools":[123,null,{}]}` installed a skill + * declaring nothing and answered success. Every entry must now be a non-empty string, and + * `global` must be a boolean, so `"yes"` cannot mint a deployment-wide skill. + */ +describe("POST /api/plugins/skills tools/global", () => { + test.each([ + ["a number entry", [123]], + ["a null entry", [null]], + ["an object entry", [{}]], + ["a whitespace entry", [" "]], + ["a mixed list", ["a/b", 42]], + ])("refuses tools with %s and installs nothing", async (_n, tools) => { + const calls = { skills: [] as unknown[] }; + const response = await pluginAppWith(calls).request( + "http://openbot.test/skills", + { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ ...skillBody, tools }), + }, + ); + expect(response.status).toBe(400); + expect(await response.json()).toEqual({ + error: "Tools are a list of serverId/toolName references.", + }); + expect(calls.skills).toEqual([]); + }); + + test.each([ + ["a string global", "yes"], + ["a number global", 1], + ])("refuses %s and installs nothing", async (_n, global) => { + const calls = { skills: [] as unknown[] }; + const response = await pluginAppWith(calls).request( + "http://openbot.test/skills", + { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ ...skillBody, global }), + }, + ); + expect(response.status).toBe(400); + expect(calls.skills).toEqual([]); + }); + + test("trims tool refs on the happy path", async () => { + const calls = { skills: [] as unknown[] }; + const response = await pluginAppWith(calls).request( + "http://openbot.test/skills", + { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ ...skillBody, tools: [" a/b "] }), + }, + ); + expect(response.status).toBe(200); + expect(calls.skills[0]).toMatchObject({ tools: ["a/b"] }); + }); +}); + +/** + * A whitespace-only Bot id is truthy and used to pass the grant check, writing a grant row (or + * an audit row on revoke) naming nothing. Param routes never checked at all. + */ +describe("component grants/functions", () => { + test("refuses a whitespace agentId on grant", async () => { + const calls = { + grants: [], + revokes: [], + revokeFunctions: [], + } as unknown as { + grants: unknown[]; + revokes: unknown[]; + revokeFunctions: unknown[]; + }; + const response = await componentAppWith(calls).request( + "http://openbot.test/widget/grants", + { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ agentId: " " }), + }, + ); + expect(response.status).toBe(400); + expect(calls.grants).toEqual([]); + }); + + test("trims the agentId on grant", async () => { + const calls = { + grants: [], + revokes: [], + revokeFunctions: [], + } as unknown as { + grants: unknown[]; + revokes: unknown[]; + revokeFunctions: unknown[]; + }; + const response = await componentAppWith(calls).request( + "http://openbot.test/widget/grants", + { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ agentId: " bot-1 " }), + }, + ); + expect(response.status).toBe(200); + expect(calls.grants).toEqual([{ name: "widget", agentId: "bot-1" }]); + }); + + test("refuses a whitespace agentId on revoke", async () => { + const calls = { + grants: [], + revokes: [], + revokeFunctions: [], + } as unknown as { + grants: unknown[]; + revokes: unknown[]; + revokeFunctions: unknown[]; + }; + const response = await componentAppWith(calls).request( + "http://openbot.test/widget/grants/%20%20%20", + { method: "DELETE" }, + ); + expect(response.status).toBe(400); + expect(calls.revokes).toEqual([]); + }); +});