From 7e357c14bff7af6073130daa9b1bc2d7a055e2ff Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 21 Aug 2026 23:07:15 -0500 Subject: [PATCH] Keep a Bot from being named after a deployment route The computer router's bot-access guard steps aside for the paths that are about the deployment rather than about a Bot. It has to: Hono matches `/*` against zero segments, so `/policy` and `/fleet` arrive at `/:botId/*` as Bot ids, and nothing in the request tells them apart from a Bot that really is called that. A Bot with one of those ids is therefore served to anybody who can sign in, and the guard is not bypassed so much as never asked. Closed at the three places it is reachable. A package id is the only way a Bot gets a chosen id, since everything created through the API is `agent_`, so the YAML is refused first. That is also the answer an operator can act on, and it sits with this file's other cross-file checks rather than adding a lookup to a per-request guard. Refusing the file does not remove a Bot that already has the name. Nothing here deletes a canonical agent when a package stops declaring it, so a deployment that once shipped `policy` keeps the row after the operator corrects the YAML. So the table is checked too, inside the synchronise transaction, and a deployment holding one refuses to start rather than serving it. And the guard itself now steps aside only for the deployment path, not for everything beneath it. Both routes are a single segment, so `/policy/status` is `/:botId/status` and nothing else; skipping the subtree was the actual hole, and it stays closed whether or not such a Bot can be created. The list moves into its own module because both ends now have to agree on it: adding a third deployment route should reserve the name at the same moment it starts being answered. --- server/src/computer/deployment-routes.ts | 14 ++ server/src/computer/routes.ts | 21 +-- server/src/tenant-package.ts | 36 ++++- .../agent-profile-store.integration.test.ts | 29 ++++ server/tests/bot-access.test.ts | 83 +++++++++++ .../deployment-route-bot-end-to-end.test.ts | 136 ++++++++++++++++++ server/tests/tenant-package.test.ts | 101 +++++++++++++ 7 files changed, 410 insertions(+), 10 deletions(-) create mode 100644 server/src/computer/deployment-routes.ts create mode 100644 server/tests/deployment-route-bot-end-to-end.test.ts diff --git a/server/src/computer/deployment-routes.ts b/server/src/computer/deployment-routes.ts new file mode 100644 index 00000000..a46daff2 --- /dev/null +++ b/server/src/computer/deployment-routes.ts @@ -0,0 +1,14 @@ +/** + * The paths under the computer router that are about the deployment rather than about a Bot. + * + * Its own file because two places have to agree on it and they sit at opposite ends of the system. + * The router steps aside for these names, and the tenant package refuses to give a Bot one. Kept in + * one list so that adding a third deployment route reserves the name at the same moment it starts + * being answered, rather than opening the hole again and waiting for somebody to notice. + * + * The reason there is anything to agree on: the bot-access middleware matches `/:botId/*`, and + * Hono's `/*` matches zero segments, so a single-segment path like `/policy` arrives as a Bot id. + * Nothing in the request distinguishes the two, so a Bot that really is called `policy` would be + * indistinguishable from the deployment route and would be served without the guard being asked. + */ +export const DEPLOYMENT_ROUTES = new Set(["policy", "fleet"]); diff --git a/server/src/computer/routes.ts b/server/src/computer/routes.ts index 23688664..b2be2c51 100644 --- a/server/src/computer/routes.ts +++ b/server/src/computer/routes.ts @@ -14,6 +14,7 @@ import { WorkspaceRefusedError, WorkspaceRequestError, } from "./gateway"; +import { DEPLOYMENT_ROUTES } from "./deployment-routes"; import { type PolicyStore, parseActionPolicy } from "./policy-store"; /** @@ -26,13 +27,6 @@ import { type PolicyStore, parseActionPolicy } from "./policy-store"; * Every computer call goes through the gateway. That is the governance seam: each acting route in * this file passes through a policy decision and audit row before it reaches the computer. */ -/** - * Paths under this router that are about the deployment rather than about a Bot. - * - * The bot-access middleware matches `/:botId/*`, and Hono's `/*` matches zero segments, so a - * single-segment path like `/policy` reaches it as a Bot id. These have their own guards. - */ -const DEPLOYMENT_ROUTES = new Set(["policy", "fleet"]); export function createComputerRoutes( gateway: ComputerGateway, @@ -59,14 +53,23 @@ export function createComputerRoutes( routes.use("/:botId/*", requireUser, async (context, next) => { const botId = context.req.param("botId"); /* - * `/policy` is this router's own, and it is not about a Bot. + * `/policy` and `/fleet` are this router's own, and they are not about a Bot. * * Hono matches `/*` against zero segments, so `/policy` arrives here as a Bot called "policy", * `canUseBot` quite correctly says there is no such Bot, and the Boundaries screen answers 404 * for everybody including an administrator. Named rather than inferred from the segment count, * because a second deployment-wide route added later should have to think about this line. + * + * The name alone is not enough to step aside on, though. Both deployment routes are a single + * segment, so `/policy/status` is `/:botId/status` and nothing more: skipping the whole subtree + * would hand the computer of a Bot called `policy` to anybody who can sign in, with the guard + * never asked rather than merely bypassed. Bot ids are reserved against these names where a Bot + * is created, so such a Bot should not exist; this is the half that holds if one ever does. */ - if (botId && DEPLOYMENT_ROUTES.has(botId)) return next(); + const path = context.req.path.replace(/\/+$/, ""); + if (botId && DEPLOYMENT_ROUTES.has(botId) && path.endsWith(`/${botId}`)) { + return next(); + } if (botId && !(await canUseBot(context.var.actor, botId))) { return context.json({ error: "There is no such Bot." }, 404); diff --git a/server/src/tenant-package.ts b/server/src/tenant-package.ts index 2360b446..be31f56e 100644 --- a/server/src/tenant-package.ts +++ b/server/src/tenant-package.ts @@ -1,7 +1,8 @@ +import { DEPLOYMENT_ROUTES } from "./computer/deployment-routes"; import { createHash } from "node:crypto"; import { readFile } from "node:fs/promises"; import { join } from "node:path"; -import { desc, eq, isNull } from "drizzle-orm"; +import { desc, eq, inArray, isNull } from "drizzle-orm"; import { parse } from "yaml"; import type { Database } from "./db/client"; import { @@ -268,6 +269,20 @@ export function validateTenantPackage(files: PackageFiles): TenantPackage { throw new Error("agent.type must be built-in or remote-ag-ui"); } const id = requiredString(agent.id, "agent.id"); + /* + * A Bot may not be named after a deployment route. + * + * The computer router's bot-access guard steps aside for those names, and a request cannot + * tell a Bot called `policy` from `/policy` itself, so such a Bot would be served to anybody + * who can sign in without the guard ever being asked. A package id is the only way a Bot gets + * a chosen id, everything created through the API being `agent_`, so refusing it here + * closes it rather than moving it. + */ + if (DEPLOYMENT_ROUTES.has(id)) { + throw new Error( + `agent.id "${id}" is reserved for a deployment route and cannot name a Bot`, + ); + } if (type === "remote_ag_ui") { const endpoint = typeof agent.endpoint === "string" ? agent.endpoint.trim() : ""; @@ -417,6 +432,25 @@ export async function synchronizeTenantPackage( tenantPackage: LoadedTenantPackage, ) { return database.transaction(async (transaction) => { + /* + * A Bot already holding one of those names, from a package that declared it before this was + * refused. Validation covers the file, and nothing here removes a canonical agent when a package + * stops declaring one, so correcting the YAML leaves the row and the router goes on stepping + * aside for its path. Checked inside the transaction so a deployment in that state does not come + * up half-synchronised, and refused rather than renamed because whose Bot that is, and what + * points at it, is not this function's to decide. + */ + const reserved = await transaction + .select({ id: agentTable.id }) + .from(agentTable) + .where(inArray(agentTable.id, [...DEPLOYMENT_ROUTES])); + if (reserved.length > 0) { + const names = reserved.map((agent) => `"${agent.id}"`).join(", "); + throw new Error( + `Bot ${names} is reserved for a deployment route and cannot exist; rename or remove it before this deployment can start`, + ); + } + const [deploymentPackage] = await transaction .insert(deploymentPackages) .values({ diff --git a/server/tests/agent-profile-store.integration.test.ts b/server/tests/agent-profile-store.integration.test.ts index 92c82908..32fc248f 100644 --- a/server/tests/agent-profile-store.integration.test.ts +++ b/server/tests/agent-profile-store.integration.test.ts @@ -14,6 +14,7 @@ import type { AgentProfile, CreateAgentInput, } from "../src/agents/profile-types"; +import { DEPLOYMENT_ROUTES } from "../src/computer/deployment-routes"; import { createDatabase } from "../src/db/client"; import { TEST_POOL } from "./support/database"; import { @@ -634,6 +635,34 @@ describe("agent profile store integration", () => { expect(rows).toHaveLength(0); }); + /* + * The id a caller gets is never one they chose. + * + * The reserved-id checks in `tenant-package.ts` rest on this: a package is the only place a Bot id + * is written by a person, so refusing the reserved names there closes them everywhere. The day + * this route lets a caller name their own Bot, that stops being true, and this is the test that + * says so rather than the reader who happens to notice. + */ + test("mints its own id rather than taking one, for a create and for a copy", async () => { + const owner = await createUser(); + const created = await store.create(owner, { + name: `Created ${randomUUID()}`, + title: "Created Title", + roleDescription: "Created role description.", + visibility: "private", + } as CreateAgentInput); + createdAgentIds.push(created.id); + const copy = await store.duplicate(owner, created.id); + createdAgentIds.push(copy.id); + + for (const id of [created.id, copy.id]) { + expect(id).toMatch( + /^agent_[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/, + ); + expect(DEPLOYMENT_ROUTES.has(id)).toBe(false); + } + }); + test("creates a caller-owned remote AG-UI profile with the requested visibility", async () => { const owner = await createUser(); const input: CreateAgentInput = { diff --git a/server/tests/bot-access.test.ts b/server/tests/bot-access.test.ts index 8aff3574..543b677a 100644 --- a/server/tests/bot-access.test.ts +++ b/server/tests/bot-access.test.ts @@ -137,6 +137,89 @@ describe("the computer surface", () => { }); }); +/* + * The deployment paths, and everything that merely starts with one. + * + * `/policy` and `/fleet` are this router's own and are not about a Bot, so the guard steps aside for + * them. What it must not step aside for is the subtree: `/policy/status` is `/:botId/status` with a + * Bot called `policy`, and treating the whole subtree as deployment-owned hands that Bot's computer + * to anybody who can sign in without the guard being asked at all. Bot ids are reserved against + * these names at the other end of the system so no such Bot can exist; this is the half that holds + * if one ever does. + */ +describe("a path that starts with a deployment route", () => { + function app(role: "user" | "admin" = "user") { + const reached: string[] = []; + const asked: string[] = []; + const gateway = { + status: async (botId: string) => { + reached.push(`status:${botId}`); + return { botId, state: "ready" }; + }, + screenshot: async (botId: string) => { + reached.push(`screenshot:${botId}`); + return { image: "" }; + }, + computers: async () => [], + } as never; + const routes = createComputerRoutes( + gateway, + { get: () => ({ mode: "enforce", deny: [], allow: [] }) } as never, + signedIn("somebody", role), + // Denies everything, so anything that answers got past the guard rather than through it. + async (_actor, botId: string) => { + asked.push(botId); + return false; + }, + ); + return { reached, asked, hono: new Hono().route("/api/computers", routes) }; + } + + for (const [name, path] of [ + ["policy", "/api/computers/policy/status"], + ["fleet", "/api/computers/fleet/status"], + ["policy, deeper", "/api/computers/policy/computers"], + ] as const) { + test(`refuses ${name} as a Bot path, and asks first`, async () => { + const { hono, reached, asked } = app(); + const response = await hono.request(path); + + expect(response.status).toBe(404); + expect(reached).toEqual([]); + // Asked, rather than skipped: the guard is what produced the 404. + expect(asked.length).toBe(1); + }); + } + + test("still serves the fleet listing itself", async () => { + // The permissive half. A guard that refused these would have closed the hole by breaking the + // two routes it exists to let through. + const { hono, asked } = app("admin"); + const response = await hono.request("http://t/api/computers/fleet"); + + expect(response.status).toBe(200); + expect(asked).toEqual([]); + }); + + test("still serves the policy route itself", async () => { + const { hono, asked } = app("admin"); + const response = await hono.request("http://t/api/computers/policy"); + + expect(response.status).toBe(200); + expect(asked).toEqual([]); + }); + + test("a trailing slash is not a way back into the subtree", async () => { + // `/policy/` matches no route in this router either way, which is the answer wanted here. What + // this pins is that it never reaches the computer as a Bot called `policy`. + const { hono, reached } = app("admin"); + const response = await hono.request("http://t/api/computers/policy/"); + + expect(reached).toEqual([]); + expect(response.status).toBe(404); + }); +}); + 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 diff --git a/server/tests/deployment-route-bot-end-to-end.test.ts b/server/tests/deployment-route-bot-end-to-end.test.ts new file mode 100644 index 00000000..043a76c6 --- /dev/null +++ b/server/tests/deployment-route-bot-end-to-end.test.ts @@ -0,0 +1,136 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import type { MiddlewareHandler } from "hono"; +import type { AuditEventInput, AuditStore } from "../src/audit"; +import type { AppVariables, AuthenticatedActor } from "../src/auth/guards"; +import { createComputerGateway } from "../src/computer/gateway"; +import type { PolicyStore } from "../src/computer/policy-store"; +import type { ComputerProvider } from "../src/computer/provider"; +import { createComputerRoutes } from "../src/computer/routes"; + +/** + * A Bot named after a deployment route, over a real socket. + * + * The router's guard steps aside for `/policy` and `/fleet`, which are its own paths and not about a + * Bot. The question this answers is what a request UNDER one of those names reaches, and only a + * listener can say: a test that reads the URL it passed to a stubbed fetch is describing its own + * argument, while the thing being prevented is a request arriving at a computer that nobody was + * asked about. So this drives the real router through the real gateway and the real transport, and + * asks the far side what it got. + * + * The reserved-id checks in `tenant-package.ts` mean no such Bot can be declared or survive a start, + * which is the fix. This is the layer underneath it: if a Bot with that id ever did exist, the guard + * still has to be the thing that answers. + */ + +const servers: Array<{ stop: (force?: boolean) => void }> = []; + +afterEach(() => { + for (const server of servers.splice(0)) server.stop(true); +}); + +const TOKEN = "computer-token-for-this-test"; + +function serveComputer() { + const received: string[] = []; + const server = Bun.serve({ + port: 0, + fetch: async (request) => { + received.push(new URL(request.url).pathname); + return Response.json({ ok: true, state: "ready" }); + }, + }); + servers.push(server); + return { received, baseUrl: `http://127.0.0.1:${server.port}` }; +} + +/** Answers for one Bot only, the way the store's access filter does. */ +function appFor(baseUrl: string, permitted: string) { + const asked: string[] = []; + const rows: AuditEventInput[] = []; + const provider: ComputerProvider = { + name: "test", + isolation: "per-bot", + locate: async () => baseUrl, + status: async (botId) => ({ botId, state: "ready" }), + stop: async () => ({ wasRunning: true }), + reset: async () => ({ cleared: true }), + list: async () => [], + }; + const gateway = createComputerGateway({ + provider, + auditStore: { + insert: async (event: AuditEventInput) => { + rows.push(event); + }, + } as unknown as AuditStore, + policy: () => ({ mode: "enforce", deny: [], allow: ["true"] }), + token: TOKEN, + }); + const actor: AuthenticatedActor = { + id: "somebody", + email: "somebody@openbot.test", + role: "user", + }; + const asActor: MiddlewareHandler<{ Variables: AppVariables }> = async ( + context, + next, + ) => { + context.set("actor", actor); + await next(); + }; + return { + asked, + app: createComputerRoutes( + gateway, + {} as PolicyStore, + asActor, + async (_who, botId) => { + asked.push(botId); + return botId === permitted; + }, + ), + }; +} + +describe("a Bot named after a deployment route, end to end", () => { + test("a permitted Bot's read reaches the computer", async () => { + // The control. Everything below asserts that nothing arrived, which proves nothing unless a + // request that should arrive does. + const { received, baseUrl } = serveComputer(); + const { app, asked } = appFor(baseUrl, "bot-1"); + + const response = await app.request("http://openbot.test/bot-1/read"); + + expect(response.status).toBe(200); + expect(received).toEqual(["/read"]); + expect(asked).toEqual(["bot-1"]); + }); + + for (const name of ["policy", "fleet"]) { + test(`nothing under "${name}" reaches the computer without the guard answering`, async () => { + const { received, baseUrl } = serveComputer(); + // `name` is the permitted Bot, so a request that gets past the guard would be forwarded and + // arrive. Nothing arriving therefore means the guard, not the gateway, ended the request. + const { app, asked } = appFor(baseUrl, "never-this-bot"); + + const response = await app.request(`http://openbot.test/${name}/read`); + + expect(response.status).toBe(404); + expect(received).toEqual([]); + expect(asked).toEqual([name]); + }); + } + + test("a Bot called policy is served the moment the guard permits it, and not before", async () => { + // The other direction, so the refusal above is shown to be the guard's answer rather than a + // route that cannot be reached at all. + const { received, baseUrl } = serveComputer(); + const { app, asked } = appFor(baseUrl, "policy"); + + const response = await app.request("http://openbot.test/policy/read"); + + expect(response.status).toBe(200); + expect(received).toEqual(["/read"]); + expect(asked).toEqual(["policy"]); + }); +}); diff --git a/server/tests/tenant-package.test.ts b/server/tests/tenant-package.test.ts index 911e9684..f29499dd 100644 --- a/server/tests/tenant-package.test.ts +++ b/server/tests/tenant-package.test.ts @@ -167,6 +167,53 @@ describe("tenant YAML validation", () => { ).toThrow("agent.role_description must be a non-empty string"); }); + /* + * A Bot named after a deployment route is refused before it can exist. + * + * `/:botId/*` under the computer router carries the guard that asks whether this person may act as + * the Bot in the path, and it steps aside for the paths that are about the deployment rather than + * about a Bot. Hono matches `/*` against zero segments, so those arrive as Bot ids and there is + * nothing else to tell them apart by. A package free to name a Bot `policy` therefore hands that + * Bot's computer surface to everybody who can sign in, with the guard never consulted at all. + * + * Refused here rather than guarded there, because a package id is the only way a Bot can have a + * chosen id: everything created through the API is `agent_`. Refusing at load is also the + * answer an operator can act on, and it is where this package's other cross-file checks already + * live. + */ + for (const reserved of ["policy", "fleet"]) { + test(`rejects an agent whose id is the deployment route "${reserved}"`, () => { + expect(() => + validateTenantPackage({ + brand: "tenant: { id: fintech, product_name: Ledgerline }", + agents: `agents: [{ id: ${reserved}, name: Knowledge, title: Company Knowledge, role_description: Answer company questions., type: built-in, system_prompt: Answer from knowledge. }]`, + channels: "channels: []", + model: + "model: { provider: openai, credential_secret_ref: openai-key, default_model: gpt-4.1 }", + knowledge: "sources: []", + themeCss: "", + }), + ).toThrow(/reserved/i); + }); + } + + test("an id that merely contains a reserved name is fine", () => { + // The collision is exact: `/policy-desk/status` is a Bot path and reaches the guard normally. + const tenantPackage = validateTenantPackage({ + brand: "tenant: { id: fintech, product_name: Ledgerline }", + agents: + "agents: [{ id: policy-desk, name: Policy Desk, title: Policy, role_description: Answer policy questions., type: built-in, system_prompt: Answer from policy. }]", + channels: "channels: []", + model: + "model: { provider: openai, credential_secret_ref: openai-key, default_model: gpt-4.1 }", + knowledge: "sources: []", + themeCss: "", + }); + expect(tenantPackage.agents.map((agent) => agent.id)).toEqual([ + "policy-desk", + ]); + }); + test("parses an explicit avatar seed and leaves an omitted seed undefined", () => { const tenantPackage = validateTenantPackage({ brand: "tenant: { id: fintech, product_name: Ledgerline }", @@ -367,6 +414,60 @@ describe("tenant package agent profile synchronization", () => { }); }); + /* + * The row a corrected package leaves behind. + * + * Refusing the id in the YAML closes the way a Bot gets that name, not a Bot that already has it: + * nothing here deletes a canonical agent when a package stops declaring it, so a deployment that + * once shipped `policy` keeps the row after the operator renames it, and the computer router goes + * on stepping aside for that path. So the table is checked as well as the file, and a deployment + * holding one refuses to start rather than serving it to everybody who can sign in. + */ + test("refuses to synchronize while a Bot named after a deployment route exists", async () => { + await database.insert(agents).values({ + id: "policy", + name: "Left behind by an older package", + type: "built_in", + configuration: {}, + }); + createdAgentIds.push("policy"); + + const agent = packageAgent(); + await expect( + synchronizeTenantPackage(database, loadedPackage(agent)), + ).rejects.toThrow(/reserved for a deployment route/i); + + // The refusal is the whole answer: nothing of the package is half-applied behind it. + const [applied] = await database + .select() + .from(agents) + .where(eq(agents.id, agent.id)); + expect(applied).toBeUndefined(); + }); + + test("synchronizes normally when no such Bot exists", async () => { + // The permissive half, so the check above is proved to be about the reserved id and not about + // any pre-existing row. + await database.insert(agents).values({ + id: `policy-desk-${randomUUID()}`, + name: "An ordinary Bot", + type: "built_in", + configuration: {}, + }); + const agent = packageAgent(); + const deploymentPackage = await synchronizeTenantPackage( + database, + loadedPackage(agent), + ); + createdAgentIds.push(agent.id); + createdPackageIds.push(deploymentPackage.id); + const [applied] = await database + .select() + .from(agents) + .where(eq(agents.id, agent.id)); + expect(applied).toBeDefined(); + }); + test("resynchronizes and undeletes an existing package profile", async () => { const agent = packageAgent({ avatarSeed: "old-avatar" }); const tenantPackage = loadedPackage(agent);