diff --git a/CHANGELOG.md b/CHANGELOG.md index 1df02a3e..de47d231 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/server/src/plugins/store.ts b/server/src/plugins/store.ts index 745f736a..50eac40f 100644 --- a/server/src/plugins/store.ts +++ b/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,7 +720,7 @@ 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({ @@ -683,7 +728,7 @@ export function createPluginStore(options: PluginStoreOptions) { set: { title: input.title, url: input.url, - credentialId: input.credentialId ?? null, + credentialId: credentialId ?? null, addedBy: input.by, updatedAt: new Date(), }, diff --git a/server/tests/plugin-store.integration.test.ts b/server/tests/plugin-store.integration.test.ts index b76c4597..8fa8d27f 100644 --- a/server/tests/plugin-store.integration.test.ts +++ b/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); + }); +});