Point a custom MCP server only at a credential of its own kind

CHANGELOG.md

@@ -32,6 +32,28 @@ It is narrow on purpose:
 
 Unset means none, which is what every deployment has today.
 
+### A custom MCP server can only be pointed at its own token
+
+Adding an MCP server by URL takes a credential id alongside the address, and the add is what spends
+it: the tool refresh that runs before the call returns decrypts whatever that id names and sends it
+to the address in the same request. Nothing checked which credential it was, so an administrator
+could name any row in the vault, including one person's connector token, and have that person's
+token delivered in clear text to an address of the administrator's choosing, before any Bot or grant
+was involved. The credentials screen lists every row's id and, for a connector token, the person it
+belongs to, so choosing one was a single read.
+
+A custom server now has to be pointed at a credential of its own kind, the deployment's token for
+that server. A person's connector token and the deployment's OAuth client are both refused, for the
+same reason `POST /api/admin/credentials` already refuses to create either by hand: spending one
+here uses a credential on behalf of somebody who never agreed to it. A credential that does not
+exist is refused in the same words as one of the wrong kind, so the endpoint cannot be used to ask
+which ids are real.
+
+The field is unchanged for the case it exists for, and nothing changes for a server added through
+the admin screen, which mints a token and points at the one it just made. If a deployment has a
+custom server pointing at a credential of another kind, adding it again will now be refused, and the
+answer is to give the server its own token.
+
 ### Upgrading
 
 **A deployment that sets `AGENT_COMPUTER_ALLOW_PRIVATE_HOSTS=true` with `NODE_ENV=production` no

server/src/plugins/store.ts

@@ -667,6 +667,51 @@ export function createPluginStore(options: PluginStoreOptions) {
         );
       }
 
+      /*
+       * The pointer is checked here because the add is what dereferences it.
+       *
+       * `refreshTools` runs before this method returns, and for a custom server there is no
+       * catalogue entry, so `connectionTokenFor` decrypts whatever `credential_id` names and
+       * `listTools` sends it to the URL from this same request. An unchecked pointer therefore is
+       * not "a wrong token later", it is this call delivering that secret to an address the caller
+       * chose, before any grant, policy check or Bot exists.
+       *
+       * `mcp` is the only kind that answers "this server's own token". A `mcp_user_token` is one
+       * person's grant and a `mcp_oauth_client` identifies the deployment to a vendor; neither is
+       * this deployment's bearer token for this server, and spending either here would be using a
+       * credential on behalf of somebody who never agreed to it. `POST /api/admin/credentials`
+       * already refuses to mint those two by hand for that reason, and its comment says so; this is
+       * the same objection at the point they are referenced rather than created.
+       *
+       * One message for both "wrong kind" and "no such credential", deliberately. A caller who can
+       * tell those apart can ask this endpoint which credential ids are real.
+       */
+      const credentialId = input.credentialId?.trim() || undefined;
+      if (credentialId) {
+        /*
+         * The shape is checked before the lookup because `credentials.id` is a `uuid` column, so a
+         * value that is not one makes the query itself fail rather than return no rows, and the
+         * caller gets a database error where a refusal belongs. The same was true of the foreign key
+         * before this guard existed.
+         */
+        const looksLikeId =
+          /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i.test(
+            credentialId,
+          );
+        const [named] = looksLikeId
+          ? await database
+              .select({ kind: credentialRows.kind })
+              .from(credentialRows)
+              .where(eq(credentialRows.id, credentialId))
+          : [];
+
+        if (named?.kind !== "mcp") {
+          throw new CustomServerRefusedError(
+            "That is not a credential this server can use. Add the server's own token instead.",
+          );
+        }
+      }
+
       await database
         .insert(mcpServers)
         .values({
@@ -675,15 +720,15 @@ export function createPluginStore(options: PluginStoreOptions) {
           vendor: new URL(input.url).hostname,
           url: input.url,
           provenance: "custom",
-          credentialId: input.credentialId ?? null,
+          credentialId: credentialId ?? null,
           addedBy: input.by,
         })
         .onConflictDoUpdate({
           target: mcpServers.id,
           set: {
             title: input.title,
             url: input.url,
-            credentialId: input.credentialId ?? null,
+            credentialId: credentialId ?? null,
             addedBy: input.by,
             updatedAt: new Date(),
           },

server/tests/plugin-store.integration.test.ts

@@ -1,18 +1,24 @@
 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
 import { randomUUID } from "node:crypto";
-import { and, eq, inArray, sql } from "drizzle-orm";
+import { and, eq, inArray, like, sql } from "drizzle-orm";
 import { createAuditStore } from "../src/audit";
+import { encryptSecret } from "../src/credentials";
 import type { ActionPolicy } from "../src/computer/policy";
 import { createDatabase } from "../src/db/client";
 import { TEST_POOL } from "./support/database";
 import {
   agents,
   auditEvents,
+  credentials,
   mcpServers,
   mcpTools,
   pluginGrants,
 } from "../src/db/schema";
-import { createPluginStore, PluginRefusedError } from "../src/plugins/store";
+import {
+  createPluginStore,
+  CustomServerRefusedError,
+  PluginRefusedError,
+} from "../src/plugins/store";
 
 /**
  * The two questions a tool call has to pass, and the row each answer leaves behind.
@@ -519,3 +525,239 @@ describe("a grant on a tool the vendor no longer lists", () => {
     expect(drive?.withdrawn.map((row) => row.ref)).not.toContain(ref);
   });
 });
+
+/**
+ * Which credential a custom server is allowed to be pointed at.
+ *
+ * `addCustomServer` takes the pointer from the request body, and the add itself dereferences it: the
+ * refresh that follows decrypts whatever it names and sends it to the URL from the same request. So
+ * the pointer is the whole control. An administrator naming somebody's `mcp_user_token` was enough
+ * to have that person's decrypted token delivered to an address the administrator chose, before any
+ * grant, policy check or Bot existed.
+ *
+ * `POST /api/admin/credentials` already refuses to *mint* a `mcp_user_token` by hand, and says why:
+ * it would be "creating a credential attributed to a person who never agreed to it". Pointing at one
+ * spends that credential on the same person's behalf, which is the same objection.
+ */
+describe("a custom server may only be pointed at its own kind of credential", () => {
+  const suffix = randomUUID().slice(0, 8);
+  const deploymentCredentialId = randomUUID();
+  const personalCredentialId = randomUUID();
+  const oauthClientCredentialId = randomUUID();
+  const customServerId = `custom-cred-${suffix}`;
+  const madeServerIds: string[] = [];
+
+  beforeAll(async () => {
+    const encrypted = await encryptSecret(
+      `${"A".repeat(43)}=`,
+      "not-read-here",
+    );
+    await database.insert(credentials).values([
+      {
+        id: deploymentCredentialId,
+        kind: "mcp",
+        provider: customServerId,
+        keyId: customServerId,
+        encryptedValue: encrypted,
+        metadata: {},
+      },
+      {
+        id: personalCredentialId,
+        kind: "mcp_user_token",
+        provider: "google-drive",
+        // For a user token the key is the person, which is what makes one pickable by name from the
+        // administrator's own credential list.
+        keyId: `user_someone_else_${suffix}`,
+        encryptedValue: encrypted,
+        metadata: {},
+      },
+      {
+        id: oauthClientCredentialId,
+        kind: "mcp_oauth_client",
+        provider: "google-drive",
+        keyId: "google-drive",
+        encryptedValue: encrypted,
+        metadata: {},
+      },
+    ]);
+  });
+
+  afterAll(async () => {
+    // By prefix, not by the ids this suite meant to make: before the fix the refused adds succeed,
+    // and a row left behind holds a foreign key onto the credentials deleted just below.
+    await database
+      .delete(mcpServers)
+      .where(like(mcpServers.id, `${customServerId}%`));
+    await database
+      .delete(credentials)
+      .where(
+        inArray(credentials.id, [
+          deploymentCredentialId,
+          personalCredentialId,
+          oauthClientCredentialId,
+        ]),
+      );
+  });
+
+  test("somebody else's connector token is refused, and no server is written", async () => {
+    const id = `${customServerId}-personal`;
+    await expect(
+      store.addCustomServer({
+        id,
+        title: "Collector",
+        url: "https://collector.example/mcp",
+        credentialId: personalCredentialId,
+        by: "admin@example.com",
+      }),
+    ).rejects.toBeInstanceOf(CustomServerRefusedError);
+
+    // The refusal has to stop the write, not merely report on it: a row here is a pointer the next
+    // refresh would dereference.
+    const rows = await database
+      .select({ id: mcpServers.id })
+      .from(mcpServers)
+      .where(eq(mcpServers.id, id));
+    expect(rows).toHaveLength(0);
+  });
+
+  test("the deployment's OAuth client is refused too", async () => {
+    // Not a per-person secret, but not this server's token either, and handing a vendor its own
+    // client secret as a bearer token is the mistake `refreshTools` was already changed to avoid.
+    const id = `${customServerId}-client`;
+    await expect(
+      store.addCustomServer({
+        id,
+        title: "Collector",
+        url: "https://collector.example/mcp",
+        credentialId: oauthClientCredentialId,
+        by: "admin@example.com",
+      }),
+    ).rejects.toBeInstanceOf(CustomServerRefusedError);
+  });
+
+  test("a credential that does not exist is refused the same way", async () => {
+    // Same message as the wrong-kind refusal on purpose. A caller who can tell "wrong kind" from
+    // "no such row" can ask this endpoint which ids are real, which is a vault oracle.
+    const id = `${customServerId}-missing`;
+    const missing = store.addCustomServer({
+      id,
+      title: "Collector",
+      url: "https://collector.example/mcp",
+      credentialId: randomUUID(),
+      by: "admin@example.com",
+    });
+    await expect(missing).rejects.toBeInstanceOf(CustomServerRefusedError);
+
+    const wrongKind = store
+      .addCustomServer({
+        id: `${customServerId}-kind-message`,
+        title: "Collector",
+        url: "https://collector.example/mcp",
+        credentialId: personalCredentialId,
+        by: "admin@example.com",
+      })
+      .catch((error: Error) => error.message);
+    const missingMessage = await missing.catch((error: Error) => error.message);
+    expect(await wrongKind).toBe(missingMessage);
+  });
+
+  test("the server's own token still works", async () => {
+    // The case that must keep passing, so the refusal above is a rule and not a wall. The URL is
+    // unreachable and that is fine: a failed refresh is recorded on the row rather than thrown.
+    madeServerIds.push(customServerId);
+    const added = await store.addCustomServer({
+      id: customServerId,
+      title: "Collector",
+      url: "https://collector.example/mcp",
+      credentialId: deploymentCredentialId,
+      by: "admin@example.com",
+    });
+    expect(added.id).toBe(customServerId);
+
+    const [row] = await database
+      .select({ credentialId: mcpServers.credentialId })
+      .from(mcpServers)
+      .where(eq(mcpServers.id, customServerId));
+    expect(row?.credentialId).toBe(deploymentCredentialId);
+  });
+
+  test("a credential id that is not an id is refused, not a database error", async () => {
+    // `credentials.id` is a uuid column, so an unshaped value makes the lookup itself fail. The
+    // route passes the body field through untouched, so this is reachable with one curl.
+    for (const notAnId of ["not-a-uuid", "' OR 1=1 --"]) {
+      await expect(
+        store.addCustomServer({
+          id: `${customServerId}-shape`,
+          title: "Collector",
+          url: "https://collector.example/mcp",
+          credentialId: notAnId,
+          by: "admin@example.com",
+        }),
+      ).rejects.toBeInstanceOf(CustomServerRefusedError);
+    }
+  });
+
+  test("an empty credential id reads as no credential", async () => {
+    // Not the same as a wrong one. An empty string used to reach the insert and break the foreign
+    // key; the honest reading is that the administrator named nothing.
+    const id = `${customServerId}-empty`;
+    madeServerIds.push(id);
+    const added = await store.addCustomServer({
+      id,
+      title: "Collector",
+      url: "https://collector.example/mcp",
+      credentialId: "",
+      by: "admin@example.com",
+    });
+    expect(added.id).toBe(id);
+
+    const [row] = await database
+      .select({ credentialId: mcpServers.credentialId })
+      .from(mcpServers)
+      .where(eq(mcpServers.id, id));
+    expect(row?.credentialId).toBeNull();
+  });
+
+  test("re-adding an existing server cannot repoint it at a refused credential", async () => {
+    // The add is an upsert, so the dangerous shape is not only a new server: an existing one that
+    // already holds its own token can be re-added naming somebody else's. The guard has to run
+    // before the write, and the pointer already on the row has to survive the refusal.
+    const id = `${customServerId}-upsert`;
+    madeServerIds.push(id);
+    await store.addCustomServer({
+      id,
+      title: "Collector",
+      url: "https://collector.example/mcp",
+      credentialId: deploymentCredentialId,
+      by: "admin@example.com",
+    });
+
+    await expect(
+      store.addCustomServer({
+        id,
+        title: "Collector",
+        url: "https://collector.example/mcp",
+        credentialId: personalCredentialId,
+        by: "admin@example.com",
+      }),
+    ).rejects.toBeInstanceOf(CustomServerRefusedError);
+
+    const [row] = await database
+      .select({ credentialId: mcpServers.credentialId })
+      .from(mcpServers)
+      .where(eq(mcpServers.id, id));
+    expect(row?.credentialId).toBe(deploymentCredentialId);
+  });
+
+  test("a custom server with no credential at all still works", async () => {
+    const id = `${customServerId}-none`;
+    madeServerIds.push(id);
+    const added = await store.addCustomServer({
+      id,
+      title: "Collector",
+      url: "https://collector.example/mcp",
+      by: "admin@example.com",
+    });
+    expect(added.id).toBe(id);
+  });
+});