Keep a Bot from being named after a deployment route

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

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

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_<uuid>`, 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({

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 = {

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

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"]);
+  });
+});