From 1dc7c429e113c1ac89badeda2148443369a06f84 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Wed, 19 Aug 2026 23:05:00 -0500 Subject: [PATCH 1/3] Ask whether the person may act as the Bot they named `canAccessAgent` says a coworker is reachable when it is public, or the person owns it, or they administer the deployment. `canRunAgent` is exported as its alias. Both are called by their own unit test and by nothing else: the roster and the runtime get the rule from the store's read filter, and the surfaces that act as a Bot never asked at all. So `requireUser` was the whole gate on acting. Any signed-in account could name any Bot and reset its computer, drive its pages, read its workspace, watch its screen, and fire its granted MCP tools against the deployment's stored credential, including for a private coworker belonging to somebody else. The Bot travels in the path for the computer and in the body for a tool call, and neither was resolved against a row. The check is asked once per surface rather than per route. On the computer that is a `use` on `/:botId/*`, which is also where the session guard now lives, so a route added later cannot forget either. Reads are gated with actions: a screenshot of somebody's Bot is whatever page it is signed into. The tool call asks before the grant is looked up, because the grant says the Bot may use the tool and says nothing about who is asking. The live-screen socket asks after the session guard it already had. A missing Bot and somebody else's Bot answer the same way, so this cannot be used to find out which coworkers a deployment has. The answer comes from the store's own `get`, which already filters on the same policy, rather than a second copy of the rule that could drift. A deployment with no profile store has no agents table and so no private Bot to protect, and keeps working. --- server/src/agents/profile-policy.ts | 14 ++ server/src/app.ts | 28 +++- server/src/computer/routes.ts | 70 ++++++--- server/src/index.ts | 11 +- server/src/plugins/routes.ts | 13 ++ server/tests/bot-access.test.ts | 223 ++++++++++++++++++++++++++++ 6 files changed, 333 insertions(+), 26 deletions(-) create mode 100644 server/tests/bot-access.test.ts diff --git a/server/src/agents/profile-policy.ts b/server/src/agents/profile-policy.ts index bdf4a67..76bc132 100644 --- a/server/src/agents/profile-policy.ts +++ b/server/src/agents/profile-policy.ts @@ -23,3 +23,17 @@ export function canManageAgent( } export const canRunAgent = canAccessAgent; + +/** + * Whether this person may act as this Bot. + * + * Injected rather than imported, so a surface that acts as a Bot depends on the question and not on + * the agents table. It also keeps the answer in one place: the store's read path already filters on + * {@link canAccessAgent}, so asking it is the same rule the roster and the runtime already apply, + * rather than a second copy that can drift from them. + */ +export type BotAccessCheck = ( + /** The whole actor, not just the id: an administrator reaches every Bot, and a role tells us. */ + actor: AgentActor, + botId: string, +) => Promise; diff --git a/server/src/app.ts b/server/src/app.ts index 1f949d2..95835d6 100644 --- a/server/src/app.ts +++ b/server/src/app.ts @@ -2,6 +2,7 @@ import type { Hono as HonoApp, MiddlewareHandler } from "hono"; import { Hono } from "hono"; import { serveStatic } from "hono/bun"; import { authoriseAgentCall } from "./agents/callback-token"; +import type { BotAccessCheck } from "./agents/profile-policy"; import type { AgentProfileStore } from "./agents/profile-store"; import { createAgentRoutes } from "./agents/routes"; import { @@ -539,13 +540,33 @@ export function createApp( app.route("/", copilotHandler); } + /** + * May this person act as this Bot? + * + * The store's own read path already applies `canAccessAgent`, so asking it for the Bot is the same + * question the roster and the runtime ask, rather than a second copy of the rule. + * + * A deployment with no profile store has no agents table and therefore no private Bot to protect: + * its Bots come from the tenant package and are public to everybody who can sign in. Answering yes + * there keeps that deployment working without weakening one that has owners. + */ + const canUseBot: BotAccessCheck = agentProfileStore + ? async (actor, botId) => + (await agentProfileStore.get(actor, botId)) !== null + : async () => true; + // The Bot computer. Acting on a page needs the gateway and the policy it enforces, so both arrive // together or the routes are not mounted. An ungoverned computer is not a reduced feature. It is // the one shape of this feature that must not exist. if (computerGateway && computerPolicy) { app.route( "/api/computers", - createComputerRoutes(computerGateway, computerPolicy, requireUser), + createComputerRoutes( + computerGateway, + computerPolicy, + requireUser, + canUseBot, + ), ); } @@ -580,7 +601,10 @@ export function createApp( } if (pluginStore) { - app.route("/api/plugins", createPluginRoutes(pluginStore, requireUser)); + app.route( + "/api/plugins", + createPluginRoutes(pluginStore, requireUser, canUseBot), + ); } /* diff --git a/server/src/computer/routes.ts b/server/src/computer/routes.ts index 5f000ad..92703f5 100644 --- a/server/src/computer/routes.ts +++ b/server/src/computer/routes.ts @@ -1,5 +1,6 @@ import type { Context, MiddlewareHandler } from "hono"; import { Hono } from "hono"; +import type { BotAccessCheck } from "../agents/profile-policy"; import type { AppVariables } from "../auth/guards"; import { requireAdmin } from "../auth/guards"; import { @@ -29,15 +30,38 @@ export function createComputerRoutes( gateway: ComputerGateway, policyStore: PolicyStore, requireUser: MiddlewareHandler<{ Variables: AppVariables }>, + /** + * Whether the caller may act as the Bot in the path. Required rather than optional, so a new + * deployment cannot be wired up without an answer to it. + */ + canUseBot: BotAccessCheck, ) { const routes = new Hono<{ Variables: AppVariables }>(); - routes.get("/:botId/status", requireUser, async (context) => { + /** + * Every route under a Bot id, in one place. + * + * The Bot travels in the path, so each route would otherwise have to remember to ask, and the one + * that forgot would be the whole surface. Reads are gated as well as actions: a screenshot of + * somebody's Bot is whatever page it is signed into. + * + * The answer is the same for a Bot that does not exist and one belonging to somebody else, so this + * cannot be used to find out which Bots a deployment has. + */ + routes.use("/:botId/*", requireUser, async (context, next) => { + const botId = context.req.param("botId"); + if (botId && !(await canUseBot(context.var.actor, botId))) { + return context.json({ error: "There is no such Bot." }, 404); + } + await next(); + }); + + routes.get("/:botId/status", async (context) => { const botId = context.req.param("botId"); return context.json(await gateway.status(botId)); }); - routes.get("/:botId/screenshot", requireUser, async (context) => { + routes.get("/:botId/screenshot", async (context) => { try { return context.json(await gateway.screenshot(context.req.param("botId"))); } catch (error) { @@ -45,7 +69,7 @@ export function createComputerRoutes( } }); - routes.get("/:botId/read", requireUser, async (context) => { + routes.get("/:botId/read", async (context) => { try { return context.json(await gateway.read(context.req.param("botId"))); } catch (error) { @@ -53,7 +77,7 @@ export function createComputerRoutes( } }); - routes.post("/:botId/navigate", requireUser, async (context) => { + routes.post("/:botId/navigate", async (context) => { const body = (await context.req.json().catch(() => null)) as { url?: unknown; } | null; @@ -88,7 +112,7 @@ export function createComputerRoutes( } }); - routes.post("/:botId/snapshot", requireUser, async (context) => { + routes.post("/:botId/snapshot", async (context) => { try { return context.json(await gateway.snapshot(context.req.param("botId"))); } catch (error) { @@ -102,7 +126,7 @@ export function createComputerRoutes( * Each one hands the gateway the Bot, the actor and the input, and does no checking * of its own beyond the shape of the request. Where a decision gets made is a single place. */ - routes.post("/:botId/click", requireUser, (context) => + routes.post("/:botId/click", (context) => act(context, (botId, actor, body, signal) => { const ref = asRef(body); if (!ref) return badRef; @@ -110,7 +134,7 @@ export function createComputerRoutes( }), ); - routes.post("/:botId/type", requireUser, (context) => + routes.post("/:botId/type", (context) => act(context, (botId, actor, body, signal) => { const ref = asRef(body); if (!ref) return badRef; @@ -130,7 +154,7 @@ export function createComputerRoutes( }), ); - routes.post("/:botId/key", requireUser, (context) => + routes.post("/:botId/key", (context) => act(context, (botId, actor, body, signal) => { if (typeof body?.key !== "string" || !body.key) { return { error: "A key name is required, such as Enter or Tab." }; @@ -148,7 +172,7 @@ export function createComputerRoutes( }), ); - routes.post("/:botId/scroll", requireUser, (context) => + routes.post("/:botId/scroll", (context) => act(context, (botId, actor, body) => gateway.scroll(botId, actor, { ...(typeof body?.deltaY === "number" ? { deltaY: body.deltaY } : {}), @@ -160,7 +184,7 @@ export function createComputerRoutes( * Who has the wheel. Polled by the surface next to the screen, so the person sees the Bot ask for * help without reloading anything. */ - routes.get("/:botId/control", requireUser, async (context) => { + routes.get("/:botId/control", async (context) => { try { return context.json(await gateway.control(context.req.param("botId"))); } catch (error) { @@ -168,7 +192,7 @@ export function createComputerRoutes( } }); - routes.post("/:botId/control/request", requireUser, (context) => + routes.post("/:botId/control/request", (context) => act(context, (botId, actor, body) => gateway.requestHelp( botId, @@ -187,7 +211,7 @@ export function createComputerRoutes( * it holds a list. `:botId` is still there because every route under this router has it and the * gateway wants somebody to attribute the call to. */ - routes.get("/:botId/computers", requireUser, async (context) => { + routes.get("/:botId/computers", async (context) => { try { return context.json(await gateway.computers()); } catch (error) { @@ -196,25 +220,25 @@ export function createComputerRoutes( }); /** Stop the browser, keep the logins. */ - routes.post("/:botId/computers/stop", requireUser, (context) => + routes.post("/:botId/computers/stop", (context) => act(context, (botId, actor) => gateway.stopComputer(botId, actor)), ); /** Delete the profile. Every login goes with it, which is the point and also the danger. */ - routes.post("/:botId/computers/reset", requireUser, (context) => + routes.post("/:botId/computers/reset", (context) => act(context, (botId, actor) => gateway.resetComputer(botId, actor)), ); - routes.post("/:botId/control/take", requireUser, (context) => + routes.post("/:botId/control/take", (context) => act(context, (botId, actor) => gateway.takeControl(botId, actor)), ); - routes.post("/:botId/control/release", requireUser, (context) => + routes.post("/:botId/control/release", (context) => act(context, (botId, actor) => gateway.releaseControl(botId, actor)), ); /** The Bot asking for a value it must not be told. */ - routes.post("/:botId/control/secret", requireUser, (context) => + routes.post("/:botId/control/secret", (context) => act(context, (botId, actor, body) => { if (typeof body?.ref !== "string" || !body.ref) { return { @@ -243,7 +267,7 @@ export function createComputerRoutes( * route rather than a `kind` on the input route below, so that grepping for where a secret can enter * this server returns exactly one place. */ - routes.post("/:botId/human/secret", requireUser, (context) => + routes.post("/:botId/human/secret", (context) => act(context, (botId, actor, body) => { if (typeof body?.text !== "string" || !body.text) { return { error: "A value is required." }; @@ -259,7 +283,7 @@ export function createComputerRoutes( * The takeover is the audited event; what the person typed during it is deliberately unrecorded, * because the reason a takeover exists is to let them enter the thing nothing else should keep. */ - routes.post("/:botId/human/:kind", requireUser, async (context) => { + routes.post("/:botId/human/:kind", async (context) => { const kind = context.req.param("kind"); if ( kind !== "click" && @@ -286,7 +310,7 @@ export function createComputerRoutes( }); /** The Bot's files. Through the gateway, like every other acting call. */ - routes.post("/:botId/files/list", requireUser, (context) => + routes.post("/:botId/files/list", (context) => act(context, (botId, actor, body) => gateway.listFiles(botId, actor, { ...(typeof body?.path === "string" && body.path.trim() @@ -296,7 +320,7 @@ export function createComputerRoutes( ), ); - routes.post("/:botId/files/read", requireUser, (context) => + routes.post("/:botId/files/read", (context) => act(context, (botId, actor, body) => { if (typeof body?.path !== "string" || !body.path.trim()) { return { error: "A file path is required." }; @@ -312,7 +336,7 @@ export function createComputerRoutes( * request. `timeoutMs` is passed through and capped by the computer rather than here, so one place * owns the limit. */ - routes.post("/:botId/exec", requireUser, (context) => + routes.post("/:botId/exec", (context) => act(context, (botId, actor, body, signal) => { if (typeof body?.command !== "string" || !body.command.trim()) { return { error: "A command is required." }; @@ -334,7 +358,7 @@ export function createComputerRoutes( }), ); - routes.post("/:botId/files/write", requireUser, (context) => + routes.post("/:botId/files/write", (context) => act(context, (botId, actor, body) => { if (typeof body?.path !== "string" || !body.path.trim()) { return { error: "A file path is required." }; diff --git a/server/src/index.ts b/server/src/index.ts index 38e062f..beeace1 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -452,10 +452,19 @@ serve({ } // The session guard, applied by hand because middleware does not run on an upgrade. An // unauthenticated socket here would be the whole point of the proxy defeated. - const actor = await identifyUser(request).catch(() => null); + const actor = await resolveRequestActor(request).catch(() => null); if (!actor) { return new Response("Sign in first.", { status: 401 }); } + // And which Bot, which the guard above does not answer. This socket carries that Bot's screen, + // so signing in is not enough: without this, anybody signed in watches anybody's Bot work. + if ( + !(await agentProfileStore + .get({ id: actor.id, role: actor.role }, streamBotId) + .catch(() => null)) + ) { + return new Response("There is no such Bot.", { status: 404 }); + } /* * Through the gateway, not the provider. * diff --git a/server/src/plugins/routes.ts b/server/src/plugins/routes.ts index 59c6c2a..d33b6f5 100644 --- a/server/src/plugins/routes.ts +++ b/server/src/plugins/routes.ts @@ -1,5 +1,6 @@ import type { MiddlewareHandler } from "hono"; import { Hono } from "hono"; +import type { BotAccessCheck } from "../agents/profile-policy"; import type { AppVariables } from "../auth/guards"; import { requireAdmin } from "../auth/guards"; import { CATALOGUE } from "./catalogue"; @@ -30,6 +31,11 @@ import { export function createPluginRoutes( store: PluginStore, requireUser: MiddlewareHandler<{ Variables: AppVariables }>, + /** + * Whether the caller may act as the Bot they named. Required rather than optional, so a deployment + * cannot end up calling somebody else's tools by leaving an argument off. + */ + canUseBot: BotAccessCheck, ) { const routes = new Hono<{ Variables: AppVariables }>(); @@ -353,6 +359,13 @@ export function createPluginRoutes( return context.json({ error: "A tool and a Bot are required." }, 400); } + // Asked before the grant is looked up, and before anything reaches a vendor. The grant says this + // Bot may use the tool; it says nothing about whether this person may act as this Bot, and the + // call goes out on the deployment's own credential either way. + if (!(await canUseBot(context.var.actor, body.agentId))) { + return context.json({ error: "There is no such Bot." }, 404); + } + try { const result = await store.callTool({ ref: body.ref, diff --git a/server/tests/bot-access.test.ts b/server/tests/bot-access.test.ts new file mode 100644 index 0000000..b2e8e6a --- /dev/null +++ b/server/tests/bot-access.test.ts @@ -0,0 +1,223 @@ +import { describe, expect, test } from "bun:test"; +import type { MiddlewareHandler } from "hono"; +import { Hono } from "hono"; +import type { AppVariables } from "../src/auth/guards"; +import { createComputerRoutes } from "../src/computer/routes"; +import { createPluginRoutes } from "../src/plugins/routes"; + +/** + * Whether the person asking may act as the Bot they named. + * + * `requireUser` answers "is this a signed-in person", which is a different question and the only one + * these surfaces used to ask. A Bot id travels in the URL for the computer and in the body for a tool + * call, so without this a signed-in person acts as any Bot in the deployment, including a private one + * belonging to somebody else: they reset its browser, drive its pages, and fire its granted MCP tools + * against the deployment's own credential. + * + * The rule itself is not new. `canAccessAgent` has always said public, or owner, or administrator, + * and the store's read path has always filtered on it. These are the callers that never asked. + */ + +/** A signed-in person with the base role, which is the lowest privilege that gets past the guard. */ +function signedIn( + id: string, + role: "user" | "admin" = "user", +): MiddlewareHandler<{ Variables: AppVariables }> { + return async (context, next) => { + context.set("actor", { id, email: `${id}@openbot.test`, role }); + await next(); + }; +} + +/** + * Owner sees their own Bot, an administrator sees every Bot, nobody else sees it. Stands in for the + * store's access filter, which decides the same three ways. + */ +const ownedBy = + (owner: string) => + async (actor: { id: string; role: string }, botId: string) => + botId === "sales" && (actor.id === owner || actor.role === "admin"); + +describe("the computer surface", () => { + function app(actorId: string, role: "user" | "admin" = "user") { + const reached: string[] = []; + const gateway = { + resetComputer: async (_c: string, botId: string) => { + reached.push(`reset:${botId}`); + return { reset: true, botId }; + }, + read: async (botId: string) => { + reached.push(`read:${botId}`); + return { text: "a page" }; + }, + } as never; + const client = { + forBot: () => ({ + screenshot: async () => { + reached.push("screenshot"); + return { image: "" }; + }, + }), + status: async (botId: string) => { + reached.push(`status:${botId}`); + return { botId, state: "ready" }; + }, + } as never; + + const routes = createComputerRoutes( + client, + gateway, + { get: () => ({ mode: "enforce", deny: [], allow: [] }) } as never, + signedIn(actorId, role), + ownedBy("owner"), + ); + return { + reached, + hono: new Hono().route("/api/computers", routes), + }; + } + + test("lets the owner act on their own Bot", async () => { + const { hono, reached } = app("owner"); + const response = await hono.request( + "http://t/api/computers/sales/computers/reset", + { method: "POST" }, + ); + + expect(response.status).toBe(200); + expect(reached).toEqual(["reset:sales"]); + }); + + test("refuses somebody else's Bot, and does not act first", async () => { + const { hono, reached } = app("stranger"); + const response = await hono.request( + "http://t/api/computers/sales/computers/reset", + { method: "POST" }, + ); + + expect(response.status).toBe(404); + // The refusal has to happen before the gateway is called. A check that runs after the browser + // has already been wiped is not a check. + expect(reached).toEqual([]); + }); + + // Reading is not a lesser question here. A screenshot of somebody's Bot mid-task is the contents + // of whatever page it is signed into. + test.each([ + ["/api/computers/sales/read", "GET"], + ["/api/computers/sales/screenshot", "GET"], + ["/api/computers/sales/status", "GET"], + ])("refuses %s for somebody else's Bot", async (path, method) => { + const { hono, reached } = app("stranger"); + const response = await hono.request(`http://t${path}`, { method }); + + expect(response.status).toBe(404); + expect(reached).toEqual([]); + }); + + // An administrator already reaches every Bot everywhere else in the product. This must not become + // the one surface where they cannot. + test("still lets an administrator act on any Bot", async () => { + const { hono, reached } = app("someone-else", "admin"); + const response = await hono.request( + "http://t/api/computers/sales/computers/reset", + { method: "POST" }, + ); + + expect(response.status).toBe(200); + expect(reached).toEqual(["reset:sales"]); + }); + + test("says nothing about whether that Bot exists", async () => { + const { hono } = app("stranger"); + const missing = await hono.request( + "http://t/api/computers/no-such-bot/read", + ); + const private_ = await hono.request("http://t/api/computers/sales/read"); + + // Same answer either way, so the surface is not a way to enumerate other people's Bots. + expect(private_.status).toBe(missing.status); + expect(await private_.text()).toBe(await missing.text()); + }); +}); + +describe("the computer surface, unauthenticated", () => { + // The access middleware carries the session guard for everything under a Bot id, so the guard has + // to still refuse a caller with no session at all, and refuse it before anything is asked about a + // Bot. + test("refuses before it asks whose Bot it is", async () => { + const asked: string[] = []; + const reached: string[] = []; + const routes = createComputerRoutes( + {} as never, + { + read: async (botId: string) => { + reached.push(botId); + return { text: "" }; + }, + } as never, + { get: () => ({ mode: "enforce", deny: [], allow: [] }) } as never, + async (context) => + context.json({ error: "Authentication required." }, 401), + async (_actor, botId) => { + asked.push(botId); + return true; + }, + ); + const hono = new Hono().route("/api/computers", routes); + + const response = await hono.request("http://t/api/computers/sales/read"); + + expect(response.status).toBe(401); + expect(asked).toEqual([]); + expect(reached).toEqual([]); + }); +}); + +describe("calling a tool as a Bot", () => { + function app(actorId: string) { + const called: string[] = []; + const store = { + callTool: async (input: { ref: string; botId: string }) => { + called.push(`${input.ref}@${input.botId}`); + return { ok: true }; + }, + listServers: async () => [], + listSkills: async () => [], + } as never; + + return { + called, + hono: new Hono().route( + "/api/plugins", + createPluginRoutes(store, signedIn(actorId), ownedBy("owner")), + ), + }; + } + + test("lets the owner call a tool as their own Bot", async () => { + const { hono, called } = app("owner"); + const response = await hono.request("http://t/api/plugins/call", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ ref: "mcp__slack__post", agentId: "sales" }), + }); + + expect(response.status).toBe(200); + expect(called).toEqual(["mcp__slack__post@sales"]); + }); + + test("refuses a tool call as somebody else's Bot, and does not call it", async () => { + const { hono, called } = app("stranger"); + const response = await hono.request("http://t/api/plugins/call", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ ref: "mcp__slack__post", agentId: "sales" }), + }); + + expect(response.status).toBe(404); + // The grant belongs to the Bot, so the vendor call would have gone out on the deployment's + // credential. Nothing may reach the vendor before the caller is checked. + expect(called).toEqual([]); + }); +}); From 912f90e2ea70f4e42b6f3a14c7fa77a821be9fca Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Wed, 19 Aug 2026 23:25:55 -0500 Subject: [PATCH 2/3] Ask the same question on the component surface The check landed on the computer, the tool call and the live-screen socket, and the components surface names a Bot the same way and was left open. Three routes: what a Bot may draw, whether it may draw one now, and the data function that runs when it does. All three took the Bot from the request behind `requireUser`, and the last one executes, so a caller borrowing a coworker borrowed whatever its components had been granted. Also `GET /api/plugins/for/:agentId`, which lists what a Bot holds. Same shape as the component listing, and the same disclosure: it says which tools somebody else's private coworker has been given. Granting stays where it was. An administrator putting a component on a Bot is a different question and `requireAdmin` already answers it. The two existing suites that build these routers now pass a permissive check, since what they cover is the decision and the grant rather than who may ask. --- server/src/app.ts | 2 +- server/src/components/routes.ts | 32 ++++- server/src/plugins/routes.ts | 12 +- server/tests/bot-access.test.ts | 124 ++++++++++++++++-- server/tests/component-decision.test.ts | 6 +- .../tests/skill-ownership.integration.test.ts | 14 +- 6 files changed, 161 insertions(+), 29 deletions(-) diff --git a/server/src/app.ts b/server/src/app.ts index 95835d6..6234ce6 100644 --- a/server/src/app.ts +++ b/server/src/app.ts @@ -596,7 +596,7 @@ export function createApp( if (componentStore) { app.route( "/api/components", - createComponentRoutes(componentStore, requireUser, auditStore), + createComponentRoutes(componentStore, requireUser, auditStore, canUseBot), ); } diff --git a/server/src/components/routes.ts b/server/src/components/routes.ts index 0039099..b1854d6 100644 --- a/server/src/components/routes.ts +++ b/server/src/components/routes.ts @@ -1,5 +1,6 @@ import type { MiddlewareHandler } from "hono"; import { Hono } from "hono"; +import type { BotAccessCheck } from "../agents/profile-policy"; import type { AuditStore } from "../audit"; import { recordAuditEvent } from "../audit"; import type { AppVariables } from "../auth/guards"; @@ -31,7 +32,13 @@ const DEV_ACTOR_EMAIL = "dev@openbot.local"; export function createComponentRoutes( store: ComponentStore, requireUser: MiddlewareHandler<{ Variables: AppVariables }>, - auditStore?: AuditStore, + auditStore: AuditStore | undefined, + /** + * Whether the caller may act as the Bot they named. What a Bot may draw, and the data a drawing + * reads, are facts about that Bot; an administrator granting one is a separate question and stays + * behind `requireAdmin`. + */ + canUseBot: BotAccessCheck, ) { const routes = new Hono<{ Variables: AppVariables }>(); @@ -116,11 +123,13 @@ export function createComponentRoutes( * Deliberately says nothing about the components this Bot does NOT hold. A list of everything it * is missing would be a list the surface could accidentally register. */ - routes.get("/for-agent/:agentId", requireUser, async (context) => - context.json({ - components: await store.listForAgent(context.req.param("agentId")), - }), - ); + routes.get("/for-agent/:agentId", requireUser, async (context) => { + const agentId = context.req.param("agentId"); + if (!(await canUseBot(context.var.actor, agentId))) { + return context.json({ error: "There is no such Bot." }, 404); + } + return context.json({ components: await store.listForAgent(agentId) }); + }); /** * May this Bot use this component, right now? @@ -140,6 +149,11 @@ export function createComponentRoutes( if (!agentId) { return context.json({ error: "The Bot is required." }, 400); } + // Asked before the grant is, because the grant belongs to the Bot and says nothing about who is + // asking on its behalf. + if (!(await canUseBot(context.var.actor, agentId))) { + return context.json({ error: "There is no such Bot." }, 404); + } const functions = Array.isArray(body?.functions) ? body.functions.filter( (entry): entry is string => typeof entry === "string", @@ -213,6 +227,11 @@ export function createComponentRoutes( 400, ); } + // Before the grant, and before anything runs. This is the route that executes, so borrowing a + // Bot here borrows whatever its components were granted. + if (!(await canUseBot(context.var.actor, agentId))) { + return context.json({ error: "There is no such Bot." }, 404); + } const refuse = async (reason: string) => { await audit(context, "component.function_refused", name, { @@ -325,7 +344,6 @@ export function createComponentRoutes( if (!agentId) { return context.json({ error: "The Bot is required." }, 400); } - try { await store.grant(name, agentId); } catch (error) { diff --git a/server/src/plugins/routes.ts b/server/src/plugins/routes.ts index d33b6f5..e37cbfb 100644 --- a/server/src/plugins/routes.ts +++ b/server/src/plugins/routes.ts @@ -338,9 +338,15 @@ export function createPluginRoutes( }); /** What one Bot holds. The runtime reads this to decide what to offer a model. */ - routes.get("/for/:agentId", requireUser, async (context) => - context.json(await store.listForAgent(context.req.param("agentId"))), - ); + routes.get("/for/:agentId", requireUser, async (context) => { + const agentId = context.req.param("agentId"); + // A grant list is a fact about the Bot it belongs to. Left open it says which tools somebody + // else's private coworker has been given. + if (!(await canUseBot(context.var.actor, agentId))) { + return context.json({ error: "There is no such Bot." }, 404); + } + return context.json(await store.listForAgent(agentId)); + }); /** * Call a tool, as a Bot. diff --git a/server/tests/bot-access.test.ts b/server/tests/bot-access.test.ts index b2e8e6a..8aff357 100644 --- a/server/tests/bot-access.test.ts +++ b/server/tests/bot-access.test.ts @@ -2,6 +2,7 @@ import { describe, expect, test } from "bun:test"; import type { MiddlewareHandler } from "hono"; import { Hono } from "hono"; import type { AppVariables } from "../src/auth/guards"; +import { createComponentRoutes } from "../src/components/routes"; import { createComputerRoutes } from "../src/computer/routes"; import { createPluginRoutes } from "../src/plugins/routes"; @@ -42,22 +43,18 @@ describe("the computer surface", () => { function app(actorId: string, role: "user" | "admin" = "user") { const reached: string[] = []; const gateway = { - resetComputer: async (_c: string, botId: string) => { + resetComputer: async (botId: string) => { reached.push(`reset:${botId}`); - return { reset: true, botId }; + return { cleared: true }; }, read: async (botId: string) => { reached.push(`read:${botId}`); return { text: "a page" }; }, - } as never; - const client = { - forBot: () => ({ - screenshot: async () => { - reached.push("screenshot"); - return { image: "" }; - }, - }), + screenshot: async (botId: string) => { + reached.push(`screenshot:${botId}`); + return { image: "" }; + }, status: async (botId: string) => { reached.push(`status:${botId}`); return { botId, state: "ready" }; @@ -65,7 +62,6 @@ describe("the computer surface", () => { } as never; const routes = createComputerRoutes( - client, gateway, { get: () => ({ mode: "enforce", deny: [], allow: [] }) } as never, signedIn(actorId, role), @@ -149,7 +145,6 @@ describe("the computer surface, unauthenticated", () => { const asked: string[] = []; const reached: string[] = []; const routes = createComputerRoutes( - {} as never, { read: async (botId: string) => { reached.push(botId); @@ -182,6 +177,10 @@ describe("calling a tool as a Bot", () => { called.push(`${input.ref}@${input.botId}`); return { ok: true }; }, + listForAgent: async (agentId: string) => { + called.push(`list:${agentId}`); + return { mcp: [], skills: [] }; + }, listServers: async () => [], listSkills: async () => [], } as never; @@ -207,6 +206,23 @@ describe("calling a tool as a Bot", () => { expect(called).toEqual(["mcp__slack__post@sales"]); }); + // What a Bot holds is a fact about that Bot, the same as its components. Left open, this says which + // tools somebody else's private coworker has been granted. + test("refuses to list what somebody else's Bot holds", async () => { + const { hono, called } = app("stranger"); + const response = await hono.request("http://t/api/plugins/for/sales"); + + expect(response.status).toBe(404); + expect(called).toEqual([]); + }); + + test("lets the owner list what their own Bot holds", async () => { + const { hono } = app("owner"); + const response = await hono.request("http://t/api/plugins/for/sales"); + + expect(response.status).toBe(200); + }); + test("refuses a tool call as somebody else's Bot, and does not call it", async () => { const { hono, called } = app("stranger"); const response = await hono.request("http://t/api/plugins/call", { @@ -221,3 +237,87 @@ describe("calling a tool as a Bot", () => { expect(called).toEqual([]); }); }); + +describe("components, which a Bot answers with", () => { + function app(actorId: string) { + const touched: string[] = []; + const store = { + listForAgent: async (agentId: string) => { + touched.push(`list:${agentId}`); + return [{ name: "chart" }]; + }, + decide: async (name: string, agentId: string) => { + touched.push(`decide:${name}:${agentId}`); + return { allowed: true }; + }, + mayCall: async () => true, + callFunction: async () => { + touched.push("callFunction"); + return { rows: [] }; + }, + } as never; + + return { + touched, + hono: new Hono().route( + "/api/components", + createComponentRoutes( + store, + signedIn(actorId), + undefined, + ownedBy("owner"), + ), + ), + }; + } + + test("lets the owner ask about their own Bot", async () => { + const { hono, touched } = app("owner"); + const response = await hono.request( + "http://t/api/components/for-agent/sales", + ); + + expect(response.status).toBe(200); + expect(touched).toEqual(["list:sales"]); + }); + + // What a Bot may draw is a fact about that Bot. Listing it for a coworker somebody else owns says + // which components they have been granted, which is the same leak the roster refuses. + test("refuses to list somebody else's Bot components", async () => { + const { hono, touched } = app("stranger"); + const response = await hono.request( + "http://t/api/components/for-agent/sales", + ); + + expect(response.status).toBe(404); + expect(touched).toEqual([]); + }); + + test("refuses a decision asked as somebody else's Bot", async () => { + const { hono, touched } = app("stranger"); + const response = await hono.request( + "http://t/api/components/chart/decision", + { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ agentId: "sales" }), + }, + ); + + expect(response.status).toBe(404); + expect(touched).toEqual([]); + }); + + // The one that runs something. A grant belongs to the Bot, so without this the caller borrows it. + test("refuses a data function called as somebody else's Bot", async () => { + const { hono, touched } = app("stranger"); + const response = await hono.request("http://t/api/components/chart/call", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ agentId: "sales", function: "rows", args: {} }), + }); + + expect(response.status).toBe(404); + expect(touched).toEqual([]); + }); +}); diff --git a/server/tests/component-decision.test.ts b/server/tests/component-decision.test.ts index ad2b639..a16b8aa 100644 --- a/server/tests/component-decision.test.ts +++ b/server/tests/component-decision.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from "bun:test"; -import { Hono } from "hono"; import type { MiddlewareHandler } from "hono"; +import { Hono } from "hono"; import type { AppVariables } from "../src/auth/guards"; import { createComponentRoutes } from "../src/components/routes"; import type { ComponentStore } from "../src/components/store"; @@ -34,7 +34,9 @@ const asSignedIn: MiddlewareHandler<{ Variables: AppVariables }> = async ( function app() { return new Hono().route( "/components", - createComponentRoutes(store, asSignedIn), + // These cover the decision itself, so every Bot here is one the caller may use. Whether they may + // is `bot-access.test.ts`. + createComponentRoutes(store, asSignedIn, undefined, async () => true), ); } diff --git a/server/tests/skill-ownership.integration.test.ts b/server/tests/skill-ownership.integration.test.ts index b62c503..cdb44a4 100644 --- a/server/tests/skill-ownership.integration.test.ts +++ b/server/tests/skill-ownership.integration.test.ts @@ -180,10 +180,16 @@ function routesAs(actor: { email: string; role: "admin" | "user"; }) { - return createPluginRoutes(store as never, async (context, next) => { - context.set("actor", actor as never); - await next(); - }); + return createPluginRoutes( + store as never, + async (context, next) => { + context.set("actor", actor as never); + await next(); + }, + // These cover who may GRANT a skill, which is a separate question from who may act as the Bot it + // is granted to. That one is `bot-access.test.ts`. + async () => true, + ); } const asAlice = () => From c4df502b2cefbb612dd7a73818627cd693965948 Mon Sep 17 00:00:00 2001 From: David McKay Date: Fri, 21 Aug 2026 07:50:18 -0700 Subject: [PATCH 3/3] Give the existing computer-routes suite a permissive access check It builds the router directly and predates the argument, so the middleware had nothing to call. What it covers is the gateway seam rather than who may act as the Bot. --- server/tests/computer-routes.test.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/server/tests/computer-routes.test.ts b/server/tests/computer-routes.test.ts index 641abe8..bc20e01 100644 --- a/server/tests/computer-routes.test.ts +++ b/server/tests/computer-routes.test.ts @@ -19,7 +19,14 @@ describe("computer routes", () => { _context, next, ) => next(); - const routes = createComputerRoutes(gateway, policyStore, requireUser); + // Permissive: what this covers is the gateway seam, not who may act as the Bot. That question + // has its own suite in bot-access.test.ts. + const routes = createComputerRoutes( + gateway, + policyStore, + requireUser, + async () => true, + ); const response = await routes.request( "http://openbot.test/bot-17/screenshot",