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