From 27ed05254e60a5b2daab15f2fb7a092ea1033f56 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sun, 23 Aug 2026 21:38:47 -0500 Subject: [PATCH 1/3] Point a custom MCP server only at a credential of its own kind Adding a custom server takes a credential id from the request body, and the add is what spends it: refreshTools runs before the method returns, and for a custom server there is no catalogue entry, so connectionTokenFor decrypts whatever the id names and listTools sends it to the URL from the same request. Nothing checked which credential it was. So an administrator could name any row in the vault. Naming one person's mcp_user_token had that person's decrypted token arrive at an administrator-chosen address as a bearer token, during the add, before any grant, policy check or Bot existed. GET /api/admin/credentials lists every row's id, kind, provider and keyId, and for a user token the keyId is the person, so picking a target was one read. Only kind mcp answers "this server's own token". A user token is one person's grant and an OAuth client identifies the deployment to a vendor. Spending either here uses a credential on behalf of somebody who never agreed to it, which is the same objection POST /api/admin/credentials already makes when it refuses to mint those two by hand. A credential that does not exist is refused in the same words as one of the wrong kind, so the endpoint cannot be asked which ids are real. The id is also shape-checked before the lookup, because credentials.id is a uuid column and an unshaped value made the query itself fail, handing back a database error where a refusal belongs. An empty string now reads as no credential rather than breaking the foreign key. Five tests, red before the change. Removing the guard turns three of them red again, and the two that must keep working, a server with its own token and a server with none, pass either way. --- server/src/plugins/store.ts | 49 +++- server/tests/plugin-store.integration.test.ts | 215 +++++++++++++++++- 2 files changed, 260 insertions(+), 4 deletions(-) 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..3c660039 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,208 @@ 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("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); + }); +}); From f5edddb4f34011c1abe39d1fa0a2148cd789d5a8 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sun, 23 Aug 2026 21:38:47 -0500 Subject: [PATCH 2/3] Note the custom MCP server credential check in the changelog Names the disclosure explicitly: a deployment where somebody pointed a custom server at a person's connector token should treat that token as disclosed. --- CHANGELOG.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) 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 From 5f1688ec195f2cbee70cb93624c2d799cdeb334e Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sun, 23 Aug 2026 21:43:04 -0500 Subject: [PATCH 3/3] Cover the upsert path, where the guard is easiest to lose The add is an upsert, so an existing server holding its own token can be re-added naming somebody else's. The guard already runs before the write and the existing pointer survives the refusal, but nothing held that: every other test used a fresh id, so moving the check below the insert would have kept them all green. --- server/tests/plugin-store.integration.test.ts | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/server/tests/plugin-store.integration.test.ts b/server/tests/plugin-store.integration.test.ts index 3c660039..8fa8d27f 100644 --- a/server/tests/plugin-store.integration.test.ts +++ b/server/tests/plugin-store.integration.test.ts @@ -718,6 +718,37 @@ describe("a custom server may only be pointed at its own kind of credential", () 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);