diff --git a/CHANGELOG.md b/CHANGELOG.md index cbee9e84..b7bf4a20 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,45 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### An MCP token is spent only by its own server, and only at the address it was given + +Pointing a server at a credential is the one place this deployment takes a reference to a stored +secret rather than the secret itself. Everywhere else, the value was typed into the same request that +stores it: a Bot's key is minted from what an administrator pasted and the id it gets is nobody's to +choose. So this is the one field where which secret and which address could be made to disagree, and +the add settles the disagreement by spending the credential: the tool refresh runs before the call +returns and sends what it decrypts to the URL from that same request. + +Two ways they could disagree, and both are now refused. A server could be pointed at any `mcp` +credential in the vault, including one minted for a different vendor, so a token given to one server +was deliverable to another. And re-adding a server with a different URL rewrote the address while +keeping the credential, so the same token could be sent somewhere else entirely with no +cross-server trick at all: the token really did belong to that server, and only the address moved. + +The second is why the first was not enough on its own. A credential now has to belong to the server +it is attached to, and a server that already holds one cannot be re-added at a different address. +Correcting a title or retrying an interrupted add sends the same URL and is unaffected. A server +holding no credential can still be re-addressed, because there is nothing to misdirect. Moving a +server that does hold one means removing it and adding it again with the token the new address is +meant to have, which is the honest description of what has happened anyway. + +This matters more than "an administrator could misconfigure something". A stored credential cannot +be read back by anybody, by design: the credentials screen answers that a credential exists and +never what it is. These two shapes were the way around that, so a deployment where somebody has +used them should treat the credentials involved as disclosed and rotate them. + +A token also stops outliving the server it was minted for. Re-adding a server without naming a +credential used to clear the pointer while leaving the credential live, and removing a server retires +its token by reading it off that pointer, so a cleared one meant the token survived its server and +could be attached to a freshly created one at any address, where there was no longer a stored address +to compare against. Three ordinary acts in a row and the binding above stopped meaning anything. The +pointer now survives a re-add that names none, removal therefore finds and retires it, and a retired +credential is refused rather than quietly attached to fail on its next call. + +Curated servers keep working as they did. Their URL comes from the catalogue rather than the +request, and a per-instance hostname is matched against the vendor's own anchored pattern before +anything is stored, so re-adding one cannot point it at an address of the caller's choosing. + ### Knowledge searches instead of guessing A package can say which of its skills each coworker gets, and the fintech example gives Knowledge the @@ -59,6 +98,34 @@ this port has to reach it another way**, which is what publishing it on every in This does not reach back in time. A deployment that has been running with the two on one network should assume a Bot could have read or written the database, and look at the trail with that in mind. +### A curated MCP server is pointed at its own kind of credential too + +Adding a server by URL was made to check which credential it is being pointed at. Adding one from the +catalogue, the other half of the same screen, took the same field from the same request and stored it +unread, so a credential of any kind could be attached to a curated server and spent by the refresh +that runs before the add returns. + +Worth being plain about the reach, because it is narrower than the path beside it. The column is a +foreign key, so an id naming nothing was already refused by the database, and the one entry in the +catalogue is reached with each person's own Google account, whose OAuth client is registered through +its own call and sent to an address pinned in code. Nothing could be delivered to an address a caller +chose. What was reachable was a credential of the wrong kind being accepted and spent on behalf of +somebody who never agreed to it, and a malformed id arriving as a database error rather than as a +refusal. + +The rule now comes from the entry: a server the deployment holds one token for takes that token, and +a server answered as the person asking takes no credential when it is added, because its client +arrives through the call that mints it. Both add paths ask the same question in the same words, so a +credential that does not exist and one of the wrong kind are still refused identically and the +endpoint cannot be used to ask which ids are real. Adding a curated server the way the admin screen +does is unchanged. + +Adding a curated server that is already there no longer clears the credential it points at. The +column holds the OAuth client that registering one put there, and re-adding the server to change an +instance host said nothing about that client, but cleared it anyway: the credential row was left +behind with nothing pointing at it and nothing to revoke it, and everybody who had connected their +account was told the deployment has no client registered. A re-add that names no credential now +leaves the one that is there alone. ### Name the private addresses an agent may live at diff --git a/server/src/plugins/catalogue.ts b/server/src/plugins/catalogue.ts index 3c81ef4d..43a31d99 100644 --- a/server/src/plugins/catalogue.ts +++ b/server/src/plugins/catalogue.ts @@ -181,6 +181,21 @@ const PATTERNS = new Map( ]), ); +/** + * Which kind of credential this entry's server record may be pointed at, or null when it takes none + * from the caller. + * + * Beside the entry rather than at the call site, because it is a property of the vendor's auth and + * not of the request. `deployment-bearer` is the only kind that means "one token this deployment + * holds for this server", which is what `mcp` names in the vault. A `user-oauth` server is answered + * with the asker's own grant and its OAuth client is registered through its own call, which mints + * the credential itself, so an id offered when the server is added is never the right one whatever + * kind it names. A server needing no credential takes none. + */ +export function serverCredentialKind(entry: CatalogueEntry): "mcp" | null { + return entry.auth.kind === "deployment-bearer" ? "mcp" : null; +} + export function catalogueEntry(key: string): CatalogueEntry | null { return BY_KEY.get(key) ?? null; } diff --git a/server/src/plugins/routes.ts b/server/src/plugins/routes.ts index 164242b7..58d07f67 100644 --- a/server/src/plugins/routes.ts +++ b/server/src/plugins/routes.ts @@ -178,7 +178,12 @@ export function createPluginRoutes( }); return context.json({ server }); } catch (error) { - if (error instanceof CatalogueEntryUnknownError) { + // A refused credential is the administrator's mistake to correct, so it comes back as a + // refusal with its reason rather than as a 500 the way an unmapped throw would. + if ( + error instanceof CatalogueEntryUnknownError || + error instanceof CustomServerRefusedError + ) { return context.json({ error: error.message }, 400); } throw error; diff --git a/server/src/plugins/store.ts b/server/src/plugins/store.ts index c54d775f..a7a27ea2 100644 --- a/server/src/plugins/store.ts +++ b/server/src/plugins/store.ts @@ -30,6 +30,7 @@ import { classifyTool, customUrlRefusal, resolveServerUrl, + serverCredentialKind, } from "./catalogue"; import { McpServerError } from "./mcp"; import { transportFor } from "./transport"; @@ -560,6 +561,84 @@ export function createPluginStore(options: PluginStoreOptions) { return { token: minted.accessToken }; } + /** + * The credential a server is being pointed at is of the kind that server can spend. + * + * Both add paths dereference the pointer before they return, so this is checked where the pointer + * is accepted rather than where it is used. `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; 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. + * + * 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. + * + * 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. + */ + async function requireCredentialOfKind( + serverTitle: string, + serverId: string, + credentialId: string, + kind: "mcp" | null, + ): Promise { + /* + * A server that takes no credential when it is added is refused here rather than at the caller, + * so that offering an id is one question with one answer wherever it is asked. The wording says + * what is true of both kinds that reach it: a `user-oauth` server's client arrives through the + * call that mints it, and a server needing no credential has nothing to be given. + */ + if (!kind) { + throw new CustomServerRefusedError( + `${serverTitle} takes no credential when it is added.`, + ); + } + + 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, + ); + /* + * Live, as well as the right kind and the right owner. + * + * A revoked credential cannot be decrypted, so attaching one only ever produced a server that + * fails on its next call. Refusing it here says so at the moment somebody can still act on it, + * and it closes the case where a token was retired precisely because it should stop being used. + */ + const [named] = looksLikeId + ? await database + .select({ + kind: credentialRows.kind, + provider: credentialRows.provider, + }) + .from(credentialRows) + .where( + and( + eq(credentialRows.id, credentialId), + isNull(credentialRows.revokedAt), + ), + ) + : []; + + /* + * Whose it is, as well as what it is. + * + * `provider` is the server a token was minted for: `storeMcpToken` sets it to the server id and + * is the only way the plugins screen makes one. Without this, any `mcp` row in the vault could + * be attached to any server, and since the refresh spends it against that server's address, a + * token given to one vendor was deliverable to another. Reading a credential back is otherwise + * impossible by design, so this closes the one field that accepts a reference to a secret rather + * than the secret itself. + */ + if (named?.kind !== kind || named.provider !== serverId) { + throw new CustomServerRefusedError( + "That is not a credential this server can use. Add the server's own token instead.", + ); + } + } + async function requireServer(serverId: string) { const [row] = await database .select() @@ -598,6 +677,27 @@ export function createPluginStore(options: PluginStoreOptions) { const resolved = resolveServerUrl(input.key, input.instanceHost); if (!resolved) throw new CatalogueEntryUnknownError(input.key); + /* + * The pointer is checked here for the same reason it is on the path below: the refresh that + * runs before this returns dereferences whatever it names. + * + * What that reaches is narrower on this path, because the URL is the catalogue's rather than + * the caller's, so a credential cannot be delivered to an address somebody chose. That is a + * property of today's catalogue rather than of this function: the one entry it holds is + * `user-oauth`, and the catalogue's own comment invites a fork to re-add the vendors that were + * taken out. The first `deployment-bearer` entry restores the full shape, so the check belongs + * here now rather than in the review that re-adds one. + */ + const credentialId = input.credentialId?.trim() || undefined; + if (credentialId) { + await requireCredentialOfKind( + resolved.entry.title, + resolved.entry.key, + credentialId, + serverCredentialKind(resolved.entry), + ); + } + await database .insert(mcpServers) .values({ @@ -605,14 +705,24 @@ export function createPluginStore(options: PluginStoreOptions) { title: resolved.entry.title, vendor: resolved.entry.vendor, url: resolved.url, - credentialId: input.credentialId ?? null, + credentialId: credentialId ?? null, addedBy: input.by, }) .onConflictDoUpdate({ target: mcpServers.id, set: { url: resolved.url, - credentialId: input.credentialId ?? null, + /* + * Left alone when the caller sends none, rather than cleared. + * + * `registerOAuthClient` keeps the client it minted in this column, and adding the server + * again to change an instance host is not a statement about that client. Clearing it + * orphaned the credential row, which nothing then revokes, and told everybody who had + * connected that the deployment has no OAuth client registered. There is no longer a way + * to hand it back through this call either, since a `user-oauth` entry now refuses a + * credential id, so the pointer has to survive here. + */ + ...(credentialId ? { credentialId } : {}), addedBy: input.by, updatedAt: new Date(), }, @@ -691,30 +801,51 @@ export function createPluginStore(options: PluginStoreOptions) { * 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. */ + /* + * A credential is spent at the address it was given to, or not spent. + * + * Adding a server that is already here rewrites its URL, and the refresh that follows sends + * whatever credential it holds to the new one, in the same call. That is the same disclosure + * as naming another server's token and it needs no trick at all: the token really does belong + * to this server, and only the address moved. A check on whose credential it is cannot see it, + * which is why this rule is here and not folded into that one. + * + * Refused rather than repaired, because the two harmless readings of the request are both + * served by something else. Correcting a title or retrying an interrupted add sends the same + * URL and is unaffected, and genuinely moving a server means the vendor is at a new address, + * where the honest act is to remove it and add it again with the token that address is + * supposed to hold. + * + * Only this path. A curated server's URL comes from the catalogue rather than the request, so + * the most a caller can influence is an instance hostname, and that is matched against the + * vendor's own anchored pattern before anything is stored. Re-adding one cannot point it at an + * address of the caller's choosing, which is the whole of what this refuses. + */ const credentialId = input.credentialId?.trim() || undefined; + const [existing] = await database + .select({ url: mcpServers.url, credentialId: mcpServers.credentialId }) + .from(mcpServers) + .where(eq(mcpServers.id, input.id)); + + if ( + existing && + existing.url !== input.url && + (existing.credentialId || credentialId) + ) { + throw new CustomServerRefusedError( + `${input.id} is already here at a different address and holds a credential. Remove it and add it again, with the token the new address is meant to have.`, + ); + } + 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.", - ); - } + // Always `mcp`: a server added by URL is reached with the one token the deployment holds for + // it, whatever the vendor is, because nothing here knows the vendor. + await requireCredentialOfKind( + input.title, + input.id, + credentialId, + "mcp", + ); } await database @@ -733,7 +864,21 @@ export function createPluginStore(options: PluginStoreOptions) { set: { title: input.title, url: input.url, - credentialId: credentialId ?? null, + /* + * Kept when the caller names none, rather than cleared, for a reason beyond tidiness. + * + * Clearing it left the credential live with nothing pointing at it, and `removeServer` + * retires a token by reading it off the row: with the pointer gone it revoked nothing + * and deleted the server, so the token outlived the server it was minted for. It could + * then be attached to a freshly created server at any address, because the rule above + * compares against a row that no longer existed. Three ordinary acts, and the address + * this server was entrusted to stopped meaning anything. + * + * So the pointer survives, `removeServer` finds it, and a removed server's token is + * dead rather than loose. Detaching a token without removing the server is not a thing + * this endpoint does, and nothing asks it to. + */ + ...(credentialId ? { credentialId } : {}), addedBy: input.by, updatedAt: new Date(), }, diff --git a/server/tests/plugin-catalogue.test.ts b/server/tests/plugin-catalogue.test.ts index a122a5a7..d432442d 100644 --- a/server/tests/plugin-catalogue.test.ts +++ b/server/tests/plugin-catalogue.test.ts @@ -1,11 +1,13 @@ import { describe, expect, test } from "bun:test"; import { CATALOGUE, + type CatalogueEntry, catalogueEntry, classifyTool, customUrlRefusal, hostAdmissible, resolveServerUrl, + serverCredentialKind, } from "../src/plugins/catalogue"; /** @@ -288,3 +290,43 @@ describe("a URL an administrator typed", () => { expect(customUrlRefusal("not a url")).toBe("That is not a URL."); }); }); + +describe("which credential a curated server is given", () => { + /** + * A synthetic entry, because the catalogue holds one vendor today and it is `user-oauth`. + * + * The shared-token branch is the one a fork re-enables when it puts a removed vendor back, which + * is the case this rule exists for, so it is exercised here rather than left to be discovered + * then. The other side of the same argument is why the entry is written out in full rather than + * spread from a real one: what is under test is the auth kind deciding the answer. + */ + const sharedToken: CatalogueEntry = { + key: "shared-token-vendor", + title: "Vendor", + vendor: "Vendor", + summary: "A server the deployment holds one token for.", + host: "https://mcp.vendor.example", + path: "/mcp", + auth: { kind: "deployment-bearer" }, + writeTools: [], + docsUrl: "https://vendor.example/docs", + }; + + test("a shared-token server takes the deployment's own token for it", () => { + expect(serverCredentialKind(sharedToken)).toBe("mcp"); + }); + + test("a server reached as the asker takes no credential from the caller", () => { + // Its OAuth client arrives through registerOAuthClient, which mints the credential itself. An id + // offered here is therefore never the right one, whatever kind it names. + const drive = catalogueEntry("google-drive"); + expect(drive?.auth.kind).toBe("user-oauth"); + expect(serverCredentialKind(drive as CatalogueEntry)).toBeNull(); + }); + + test("a server that needs no credential takes none", () => { + expect( + serverCredentialKind({ ...sharedToken, auth: { kind: "none" } }), + ).toBeNull(); + }); +}); diff --git a/server/tests/plugin-credential-binding.integration.test.ts b/server/tests/plugin-credential-binding.integration.test.ts new file mode 100644 index 00000000..1427b6f8 --- /dev/null +++ b/server/tests/plugin-credential-binding.integration.test.ts @@ -0,0 +1,328 @@ +import { + afterAll, + afterEach, + beforeAll, + describe, + expect, + test, +} from "bun:test"; +import { randomUUID } from "node:crypto"; +import { eq, inArray, like } from "drizzle-orm"; +import { createAuditStore } from "../src/audit"; +import { encryptSecret } from "../src/credentials"; +import { createDatabase } from "../src/db/client"; +import { credentials, mcpServers, mcpTools } from "../src/db/schema"; +import { + CustomServerRefusedError, + createPluginStore, +} from "../src/plugins/store"; +import { TEST_POOL } from "./support/database"; + +/** + * Which address a stored credential may be spent against, and whose it has to be. + * + * Pointing a server at a credential is the one place this deployment accepts a *reference* to a + * secret rather than the secret itself. Everywhere else that a stored value is spent, the value was + * typed into the same request that stores it: `storeAgentAuth` mints its own row from the key an + * administrator pasted and hands back an id nobody chose. So this is the field where "which secret" + * and "which address" can be made to disagree, and the add is what settles the disagreement, because + * the refresh runs before it returns and sends what it decrypts to the URL from that same request. + * + * Two rules, and the second is the one that matters. Naming another server's token was accepted, so + * a credential could be spent by a server it was never given to. And re-adding a server with a + * different URL rewrote the address while keeping the credential, so the same token could be sent + * somewhere else entirely without any cross-server trick at all. Closing only the first leaves the + * second, which is why they are one question here rather than two. + */ + +const database = createDatabase( + process.env.DATABASE_URL ?? + "postgres://openbot:openbot@localhost:5432/openbot", + TEST_POOL, +); + +const KEY = `${"x".repeat(43)}=`; +const tag = randomUUID().slice(0, 8); +const serverId = `binding-${tag}`; +const otherServerId = `binding-other-${tag}`; +const ownCredentialId = randomUUID(); +const otherCredentialId = randomUUID(); +const OWN_TOKEN = `sk-own-${tag}`; +const OTHER_TOKEN = `sk-other-${tag}`; +const LEGITIMATE_URL = "https://legit.vendor.example/mcp"; +const CHOSEN_URL = "https://collector.attacker.example/mcp"; + +const store = createPluginStore({ + database, + auditStore: createAuditStore(database), + credentials: { + readSecret: async (id: string) => { + const [row] = await database + .select({ + encryptedValue: credentials.encryptedValue, + revokedAt: credentials.revokedAt, + }) + .from(credentials) + .where(eq(credentials.id, id)); + return row ?? null; + }, + create: async () => { + throw new Error("this suite does not write credentials"); + }, + /** + * A real revoke, unlike the other suites here, because the chain below turns on whether removing + * a server actually retires its token. Stubbing this to throw would make the test prove nothing + * about the case it exists for. + */ + revoke: async (id: string) => { + await database + .update(credentials) + .set({ revokedAt: new Date() }) + .where(eq(credentials.id, id)); + }, + } as never, + encryptionKey: KEY, + policy: () => ({ mode: "enforce", deny: [], allow: ["true"] }), +}); + +/** + * What left the deployment, so a refusal can be shown to have stopped the send rather than reported + * on it afterwards. The vendors here do not exist, so a real request would fail anyway; what this + * captures is whether one was attempted at all, and what it carried. + */ +let sent: { url: string; authorization: string | null }[] = []; +const realFetch = globalThis.fetch; + +beforeAll(async () => { + globalThis.fetch = (async (input: RequestInfo | URL, init?: RequestInit) => { + const request = + input instanceof Request ? input : new Request(input as string, init); + sent.push({ + url: request.url, + authorization: request.headers.get("authorization"), + }); + return new Response("{}", { status: 500 }); + }) as typeof fetch; + + const encrypted = async (value: string) => encryptSecret(KEY, value); + await database.insert(credentials).values([ + { + id: ownCredentialId, + kind: "mcp", + // How `storeMcpToken` records whose token this is: the server it was minted for. + provider: serverId, + keyId: `mcp-${serverId}`, + encryptedValue: await encrypted(OWN_TOKEN), + metadata: {}, + }, + { + id: otherCredentialId, + kind: "mcp", + provider: otherServerId, + keyId: `mcp-${otherServerId}`, + encryptedValue: await encrypted(OTHER_TOKEN), + metadata: {}, + }, + ]); +}); + +afterEach(() => { + sent = []; +}); + +afterAll(async () => { + globalThis.fetch = realFetch; + await database.delete(mcpTools).where(like(mcpTools.serverId, `binding-%`)); + await database.delete(mcpServers).where(like(mcpServers.id, `binding-%`)); + await database + .delete(credentials) + .where(inArray(credentials.id, [ownCredentialId, otherCredentialId])); +}); + +async function storedUrl(id: string) { + const [row] = await database + .select({ url: mcpServers.url }) + .from(mcpServers) + .where(eq(mcpServers.id, id)); + return row?.url ?? null; +} + +describe("a credential is spent only by the server it belongs to", () => { + test("another server's token is refused", async () => { + await expect( + store.addCustomServer({ + id: serverId, + title: "Collector", + url: CHOSEN_URL, + credentialId: otherCredentialId, + by: "admin@example.com", + }), + ).rejects.toBeInstanceOf(CustomServerRefusedError); + + // The refusal is the whole point only if it happens before the send. + expect(sent).toEqual([]); + expect(await storedUrl(serverId)).toBeNull(); + }); + + test("the server's own token is accepted", async () => { + const added = await store.addCustomServer({ + id: serverId, + title: "Collector", + url: LEGITIMATE_URL, + credentialId: ownCredentialId, + by: "admin@example.com", + }); + + expect(added.id).toBe(serverId); + expect(await storedUrl(serverId)).toBe(LEGITIMATE_URL); + // This is the case the field exists for, so the token does go out, to the address it was given. + expect(sent[0]?.url).toContain("legit.vendor.example"); + expect(sent[0]?.authorization).toContain(OWN_TOKEN); + }); +}); + +describe("a credential is spent only at the address it was given", () => { + test("re-adding the server at a different address is refused", async () => { + // The case a check on whose credential it is cannot see: the token really does belong to this + // server. What changed is where the server points, and the add would spend the credential + // against the new address in the same call. + await expect( + store.addCustomServer({ + id: serverId, + title: "Collector", + url: CHOSEN_URL, + credentialId: ownCredentialId, + by: "admin@example.com", + }), + ).rejects.toBeInstanceOf(CustomServerRefusedError); + + expect(sent).toEqual([]); + expect(await storedUrl(serverId)).toBe(LEGITIMATE_URL); + }); + + test("re-adding it at the address it already has still works", async () => { + // Adding twice is not an attack and must stay ordinary: it is how a title is corrected and how + // an interrupted add is retried. + const added = await store.addCustomServer({ + id: serverId, + title: "Collector, renamed", + url: LEGITIMATE_URL, + credentialId: ownCredentialId, + by: "admin@example.com", + }); + + expect(added.title).toBe("Collector, renamed"); + expect(await storedUrl(serverId)).toBe(LEGITIMATE_URL); + }); + + test("a server holding no credential can still be re-addressed", async () => { + // Nothing to misdirect, so nothing to refuse. The rule is about spending a secret somewhere it + // was not entrusted to, not about URLs being immutable. + const openServerId = `binding-open-${tag}`; + await store.addCustomServer({ + id: openServerId, + title: "Open", + url: LEGITIMATE_URL, + by: "admin@example.com", + }); + + const moved = await store.addCustomServer({ + id: openServerId, + title: "Open", + url: CHOSEN_URL, + by: "admin@example.com", + }); + + expect(moved.id).toBe(openServerId); + expect(await storedUrl(openServerId)).toBe(CHOSEN_URL); + expect(sent.every((call) => call.authorization === null)).toBe(true); + }); +}); + +/** + * The way a token used to outlive the server it belonged to, and become spendable again. + * + * Three ordinary administrative acts in a row, none of them suspicious on its own. This is the shape + * that makes "a credential belongs to its server" and "a server keeps its address" both true and + * still not enough: the address rule only fires when a row is already here, so anything that gets + * the row out of the way while the token stays live reopens the same door. + */ +describe("a token does not outlive the server it was given to", () => { + const holderId = `binding-holder-${tag}`; + const holderCredentialId = randomUUID(); + const HOLDER_TOKEN = `sk-holder-${tag}`; + + beforeAll(async () => { + await database.insert(credentials).values({ + id: holderCredentialId, + kind: "mcp", + provider: holderId, + keyId: `mcp-${holderId}`, + encryptedValue: await encryptSecret(KEY, HOLDER_TOKEN), + metadata: {}, + }); + }); + + afterAll(async () => { + // The server row first: it holds a foreign key onto the credential, so the other order is + // refused by the database rather than by anything this suite is testing. + await database.delete(mcpTools).where(eq(mcpTools.serverId, holderId)); + await database.delete(mcpServers).where(eq(mcpServers.id, holderId)); + await database + .delete(credentials) + .where(eq(credentials.id, holderCredentialId)); + }); + + test("re-adding without a token keeps the one the server already holds", async () => { + // Clearing it was the first link: the row stops naming the credential, so nothing later knows + // the credential belongs to anything, and nothing retires it. + await store.addCustomServer({ + id: holderId, + title: "Holder", + url: LEGITIMATE_URL, + credentialId: holderCredentialId, + by: "admin@example.com", + }); + + await store.addCustomServer({ + id: holderId, + title: "Holder, renamed", + url: LEGITIMATE_URL, + by: "admin@example.com", + }); + + const [row] = await database + .select({ credentialId: mcpServers.credentialId }) + .from(mcpServers) + .where(eq(mcpServers.id, holderId)); + expect(row?.credentialId).toBe(holderCredentialId); + }); + + test("removing the server retires its token", async () => { + await store.removeServer(holderId, "admin@example.com"); + + const [row] = await database + .select({ revokedAt: credentials.revokedAt }) + .from(credentials) + .where(eq(credentials.id, holderCredentialId)); + expect(row?.revokedAt).not.toBeNull(); + }); + + test("a retired token cannot be attached to a server again", async () => { + // The end of the chain. Even with the row gone, so the address rule has nothing to compare + // against, the credential itself is no longer spendable. + sent = []; + + await expect( + store.addCustomServer({ + id: holderId, + title: "Holder", + url: CHOSEN_URL, + credentialId: holderCredentialId, + by: "admin@example.com", + }), + ).rejects.toBeInstanceOf(CustomServerRefusedError); + + expect(sent).toEqual([]); + }); +}); diff --git a/server/tests/plugin-curated-credential.integration.test.ts b/server/tests/plugin-curated-credential.integration.test.ts new file mode 100644 index 00000000..65c9171c --- /dev/null +++ b/server/tests/plugin-curated-credential.integration.test.ts @@ -0,0 +1,261 @@ +import { afterAll, beforeAll, describe, expect, test } from "bun:test"; +import { randomUUID } from "node:crypto"; +import { eq, inArray } from "drizzle-orm"; +import { createAuditStore } from "../src/audit"; +import { encryptSecret } from "../src/credentials"; +import { createDatabase } from "../src/db/client"; +import { credentials, mcpServers, mcpTools } from "../src/db/schema"; +import { CATALOGUE, serverCredentialKind } from "../src/plugins/catalogue"; +import { + CustomServerRefusedError, + createPluginStore, +} from "../src/plugins/store"; +import { TEST_POOL } from "./support/database"; + +/** + * Which credential a curated server is allowed to be pointed at. + * + * `addCustomServer` was given this rule and `addServer`, one function above it, was not: it takes the + * same `credentialId` from the same administrator's request and stored it unread. The two paths are + * a pair, and a guard on one of them is a guard on the path somebody happened to look at. + * + * What is reachable today is narrower than the custom case and worth stating rather than dressing + * up. `mcp_servers.credential_id` is a real foreign key, so an id naming nothing is refused by the + * database, and the one entry in the catalogue is `user-oauth`, whose client is registered through + * `registerOAuthClient` and sent to a pinned vendor address. What is left is a credential of the + * wrong kind being accepted and spent, a malformed id arriving as a database error where a refusal + * belongs, and the whole hole reopening the moment a fork re-adds a `deployment-bearer` vendor, + * which the catalogue's own comment invites. + */ + +const database = createDatabase( + process.env.DATABASE_URL ?? + "postgres://openbot:openbot@localhost:5432/openbot", + TEST_POOL, +); + +const store = createPluginStore({ + database, + auditStore: createAuditStore(database), + credentials: { + readSecret: async () => null, + create: async () => { + throw new Error("this suite does not write credentials"); + }, + revoke: async () => { + throw new Error("this suite does not revoke credentials"); + }, + }, + encryptionKey: "x".repeat(44), + policy: () => ({ mode: "enforce", deny: [], allow: ["true"] }), +}); + +/** The catalogue key under test. Real, because which credential it takes is a property of the entry. */ +const serverId = "google-drive"; +const suffix = randomUUID().slice(0, 8); +const deploymentCredentialId = randomUUID(); +const personalCredentialId = randomUUID(); +const oauthClientCredentialId = randomUUID(); + +/** + * Whether this deployment already had the server, and what it pointed at. + * + * The id is a real catalogue key rather than a suite-scoped one, so on a database somebody is using + * it is their configured server. It is removed only when this suite is what created it, and left + * pointing where it pointed before when it is not. + */ +let existing: { credentialId: string | null } | null = null; + +beforeAll(async () => { + const [row] = await database + .select({ credentialId: mcpServers.credentialId }) + .from(mcpServers) + .where(eq(mcpServers.id, serverId)); + existing = row ?? null; + + const encrypted = await encryptSecret(`${"A".repeat(43)}=`, "not-read-here"); + await database.insert(credentials).values([ + { + id: deploymentCredentialId, + kind: "mcp", + provider: serverId, + keyId: `mcp-${serverId}-${suffix}`, + encryptedValue: encrypted, + metadata: {}, + }, + { + id: oauthClientCredentialId, + kind: "mcp_oauth_client", + provider: serverId, + keyId: `oauth-client-${serverId}-${suffix}`, + encryptedValue: encrypted, + metadata: {}, + }, + { + id: personalCredentialId, + kind: "mcp_user_token", + provider: serverId, + // 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: {}, + }, + ]); +}); + +afterAll(async () => { + if (existing) { + await database + .update(mcpServers) + .set({ credentialId: existing.credentialId }) + .where(eq(mcpServers.id, serverId)); + } else { + await database.delete(mcpTools).where(eq(mcpTools.serverId, serverId)); + await database.delete(mcpServers).where(eq(mcpServers.id, serverId)); + } + await database + .delete(credentials) + .where( + inArray(credentials.id, [ + deploymentCredentialId, + oauthClientCredentialId, + personalCredentialId, + ]), + ); +}); + +describe("a curated server may only be pointed at its own kind of credential", () => { + test("somebody else's connector token is refused, and nothing is written", async () => { + await expect( + store.addServer({ + key: serverId, + 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 dereferences. + const rows = await database + .select({ id: mcpServers.id }) + .from(mcpServers) + .where(eq(mcpServers.id, serverId)); + expect(rows).toHaveLength(existing ? 1 : 0); + }); + + test("a deployment token is refused for a vendor reached as the person asking", async () => { + // The right kind for a shared-token server and the wrong thing entirely for this one. Drive is + // answered with each person's own grant, and the deployment's OAuth client is registered through + // its own call, so there is no credential for this path to be given at all. + await expect( + store.addServer({ + key: serverId, + credentialId: deploymentCredentialId, + by: "admin@example.com", + }), + ).rejects.toBeInstanceOf(CustomServerRefusedError); + }); + + test("a malformed credential id is a refusal rather than a database error", async () => { + // `credentials.id` is a uuid column, so a value that is not one makes the query itself fail and + // the administrator gets a 500 where a refusal belongs. The same was true of the custom path + // before its shape check, and it is the reason that check reads the shape before the lookup. + const refused = store + .addServer({ + key: serverId, + credentialId: "not-a-uuid", + by: "admin@example.com", + }) + .catch((error: Error) => error); + expect(await refused).toBeInstanceOf(CustomServerRefusedError); + }); + + test("adding it again leaves the registered OAuth client where it was", async () => { + /* + * `registerOAuthClient` keeps the client it minted in this column, and adding the server again + * to change an instance host says nothing about that client. Clearing it orphaned a credential + * row that nothing revokes and told everybody who had connected that the deployment has no + * client registered, and there is no way to hand it back through this call now that a + * `user-oauth` entry refuses a credential id. + */ + await store.addServer({ key: serverId, by: "admin@example.com" }); + await database + .update(mcpServers) + .set({ credentialId: oauthClientCredentialId }) + .where(eq(mcpServers.id, serverId)); + + await store.addServer({ key: serverId, by: "admin@example.com" }); + + const [row] = await database + .select({ credentialId: mcpServers.credentialId }) + .from(mcpServers) + .where(eq(mcpServers.id, serverId)); + expect(row?.credentialId).toBe(oauthClientCredentialId); + + // Put it back, so the case below reads the column this suite left rather than this one. + await database + .update(mcpServers) + .set({ credentialId: null }) + .where(eq(mcpServers.id, serverId)); + }); + + test("adding the server without a credential still works", async () => { + // The case that must keep passing, so the refusals above are a rule and not a wall. This is also + // how the admin screen adds this vendor: it sends no credential and registers the OAuth client + // afterwards. + const added = await store.addServer({ + key: serverId, + by: "admin@example.com", + }); + expect(added.id).toBe(serverId); + + const [row] = await database + .select({ credentialId: mcpServers.credentialId }) + .from(mcpServers) + .where(eq(mcpServers.id, serverId)); + expect(row?.credentialId).toBeNull(); + }); +}); + +/** + * Every entry the catalogue actually holds, asked the same question. + * + * The shared-token branch cannot be reached today: the catalogue is frozen in code and its one entry + * is reached as the person asking. Rather than add a seam to this store so a test can invent an + * entry, the check is written over whatever the catalogue contains, so the branch starts being + * exercised the moment somebody re-adds one of the vendors that were taken out. That is the review + * where it matters, and this is the test that will be sitting there when it happens. + */ +describe("every curated entry is asked which credential it takes", () => { + test("the catalogue's own entries decide it, whatever they are", async () => { + expect(CATALOGUE.length).toBeGreaterThan(0); + + for (const entry of CATALOGUE) { + const kind = serverCredentialKind(entry); + + if (kind === null) { + // Takes none from the caller, so any id is refused, including one of the right kind. + await expect( + store.addServer({ + key: entry.key, + credentialId: deploymentCredentialId, + by: "admin@example.com", + }), + ).rejects.toBeInstanceOf(CustomServerRefusedError); + continue; + } + + // A shared-token entry takes the deployment's token for that server and nothing else. The + // fixture credential belongs to a different server, so it is refused on ownership, which is + // the branch a wrong pointer would take. + await expect( + store.addServer({ + key: entry.key, + credentialId: deploymentCredentialId, + by: "admin@example.com", + }), + ).rejects.toBeInstanceOf(CustomServerRefusedError); + } + }); +}); diff --git a/server/tests/plugin-routes.integration.test.ts b/server/tests/plugin-routes.integration.test.ts new file mode 100644 index 00000000..e25aab01 --- /dev/null +++ b/server/tests/plugin-routes.integration.test.ts @@ -0,0 +1,268 @@ +import { afterAll, beforeAll, describe, expect, test } from "bun:test"; +import { randomUUID } from "node:crypto"; +import { eq, inArray } from "drizzle-orm"; +import { createApp } from "../src/app"; +import { createAuditStore } from "../src/audit"; +import { loadConfig } from "../src/config"; +import { encryptSecret } from "../src/credentials"; +import { createDatabase } from "../src/db/client"; +import { credentials, mcpServers, mcpTools } from "../src/db/schema"; +import { createPluginStore } from "../src/plugins/store"; +import { TEST_POOL } from "./support/database"; +import { testEnvironment } from "./support/environment"; + +/** + * The whole path an administrator's request actually takes, with nothing stubbed between the request + * and the row. + * + * The two halves are covered on their own: the store's refusals against a real database, and the + * route's mapping of them against a stubbed store. Both passing does not prove the pair is wired + * together, and the failure that would live in the gap is quiet in exactly the way that matters: a + * refusal that reaches the browser as a 500 reads as a broken deployment rather than a correctable + * mistake, and a refusal that stops short of the write leaves a row pointing at a credential the + * next refresh spends. So this asks the question end to end and then looks in the table. + */ + +const database = createDatabase( + process.env.DATABASE_URL ?? + "postgres://openbot:openbot@localhost:5432/openbot", + TEST_POOL, +); + +const store = createPluginStore({ + database, + auditStore: createAuditStore(database), + credentials: { + // Never read: Drive's tool list is in this deployment's own code, so the add path here reaches + // no vault. Loud rather than absent, so a call that starts reaching one is named. + readSecret: async () => { + throw new Error("this suite does not read credentials"); + }, + create: async () => { + throw new Error("this suite does not write credentials"); + }, + revoke: async () => { + throw new Error("this suite does not revoke credentials"); + }, + }, + encryptionKey: "x".repeat(44), + policy: () => ({ mode: "enforce", deny: [], allow: ["true"] }), +}); + +const ADMIN = { + id: "admin-1", + email: "admin@openbot.test", + name: "An Administrator", + image: null, +}; + +function request( + body: unknown, + role: "admin" | "user" = "admin", + path = "/api/plugins/servers", +) { + const app = createApp( + loadConfig(testEnvironment()), + { + handler: () => new Response(null, { status: 204 }), + api: { getSession: async () => ({ user: ADMIN }) }, + } as never, + { rolesForUser: async () => [role] }, + // Positions 4-14 are the other stores; the real one is 15, pluginStore. + ...(Array.from({ length: 11 }) as never[]), + store as never, + ); + + return app.request(`http://openbot.test${path}`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }); +} + +const serverId = "google-drive"; +const suffix = randomUUID().slice(0, 8); +const personalCredentialId = randomUUID(); +const customServerId = `route-custom-${suffix}`; +const foreignCredentialId = randomUUID(); +const ownCredentialId = randomUUID(); + +/** What this deployment already had, so a database somebody is using is left as it was found. */ +let existing: { credentialId: string | null } | null = null; + +beforeAll(async () => { + const [row] = await database + .select({ credentialId: mcpServers.credentialId }) + .from(mcpServers) + .where(eq(mcpServers.id, serverId)); + existing = row ?? null; + + const encrypted = await encryptSecret(`${"A".repeat(43)}=`, "not-read-here"); + await database.insert(credentials).values([ + { + id: personalCredentialId, + kind: "mcp_user_token", + provider: serverId, + keyId: `user_someone_else_${suffix}`, + encryptedValue: encrypted, + metadata: {}, + }, + { + id: foreignCredentialId, + kind: "mcp", + // Minted for a different server, which is what makes it somebody else's to spend. + provider: `route-elsewhere-${suffix}`, + keyId: `mcp-elsewhere-${suffix}`, + encryptedValue: encrypted, + metadata: {}, + }, + { + id: ownCredentialId, + kind: "mcp", + provider: customServerId, + keyId: `mcp-${customServerId}`, + encryptedValue: encrypted, + metadata: {}, + }, + ]); +}); + +afterAll(async () => { + if (existing) { + await database + .update(mcpServers) + .set({ credentialId: existing.credentialId }) + .where(eq(mcpServers.id, serverId)); + } else { + await database.delete(mcpTools).where(eq(mcpTools.serverId, serverId)); + await database.delete(mcpServers).where(eq(mcpServers.id, serverId)); + } + await database.delete(mcpTools).where(eq(mcpTools.serverId, customServerId)); + await database.delete(mcpServers).where(eq(mcpServers.id, customServerId)); + await database + .delete(credentials) + .where( + inArray(credentials.id, [ + personalCredentialId, + foreignCredentialId, + ownCredentialId, + ]), + ); +}); + +async function serverRow() { + const [row] = await database + .select({ credentialId: mcpServers.credentialId }) + .from(mcpServers) + .where(eq(mcpServers.id, serverId)); + return row ?? null; +} + +describe("adding a curated server over HTTP", () => { + test("a credential of the wrong kind is refused, and nothing is written", async () => { + const before = await serverRow(); + + const response = await request({ + key: serverId, + credentialId: personalCredentialId, + }); + + // Not a 500. An administrator who picked the wrong row is told what to do about it. + expect(response.status).toBe(400); + expect((await response.json()).error).toContain( + "takes no credential when it is added", + ); + + // And the refusal stopped the write rather than reporting on it. + expect(await serverRow()).toEqual(before); + }); + + test("a malformed credential id is refused the same way, not as a database error", async () => { + const response = await request({ key: serverId, credentialId: "nonsense" }); + + expect(response.status).toBe(400); + }); + + test("the add the admin screen makes still works and writes the row", async () => { + const response = await request({ key: serverId }); + + expect(response.status).toBe(200); + expect((await response.json()).server.id).toBe(serverId); + // Whatever the column held before, not null: an add that names no credential leaves a registered + // OAuth client alone, so asserting null here would pass on a fresh database and fail on the one + // deployment shape that behaviour exists for. + expect(await serverRow()).toEqual({ + credentialId: existing?.credentialId ?? null, + }); + }); + + test("somebody who is not an administrator is refused before the store", async () => { + const response = await request({ key: serverId }, "user"); + + expect(response.status).toBe(403); + }); +}); + +/** + * The same two rules, asked over HTTP against the real store. + * + * Both are refusals an administrator has to be able to act on, so what they must never be is a 500: + * "something went wrong" sends somebody to look at the deployment when the answer is to pick a + * different token or remove the server first. + */ +describe("adding a server by URL over HTTP", () => { + const custom = "/api/plugins/servers/custom"; + + test("another server's token is refused rather than spent", async () => { + const response = await request( + { + id: customServerId, + title: "Collector", + url: "https://collector.attacker.example/mcp", + credentialId: foreignCredentialId, + }, + "admin", + custom, + ); + + expect(response.status).toBe(400); + + const rows = await database + .select({ id: mcpServers.id }) + .from(mcpServers) + .where(eq(mcpServers.id, customServerId)); + expect(rows).toHaveLength(0); + }); + + test("re-addressing a server that holds a token is refused", async () => { + const added = await request( + { + id: customServerId, + title: "Collector", + url: "https://legit.vendor.example/mcp", + credentialId: ownCredentialId, + }, + "admin", + custom, + ); + expect(added.status).toBe(200); + + const moved = await request( + { + id: customServerId, + title: "Collector", + url: "https://collector.attacker.example/mcp", + credentialId: ownCredentialId, + }, + "admin", + custom, + ); + expect(moved.status).toBe(400); + + const [row] = await database + .select({ url: mcpServers.url }) + .from(mcpServers) + .where(eq(mcpServers.id, customServerId)); + expect(row?.url).toBe("https://legit.vendor.example/mcp"); + }); +}); diff --git a/server/tests/plugin-routes.test.ts b/server/tests/plugin-routes.test.ts new file mode 100644 index 00000000..cace693d --- /dev/null +++ b/server/tests/plugin-routes.test.ts @@ -0,0 +1,105 @@ +import { describe, expect, test } from "bun:test"; +import { createApp } from "../src/app"; +import { loadConfig } from "../src/config"; +import { + CatalogueEntryUnknownError, + CustomServerRefusedError, +} from "../src/plugins/store"; +import { testEnvironment } from "./support/environment"; + +/** + * What a refused add looks like to the administrator who made it. + * + * The store's refusals are tested where they are decided. What is worth pinning here is the mapping, + * because an unmapped throw leaves the route on its default path: the refusal becomes a 500, the + * screen says something went wrong, and a correctable mistake reads as a broken deployment. The + * curated route mapped one refusal and not the other, which is exactly the shape that is invisible + * until somebody hits it. + */ + +const ADMIN = { + id: "admin-1", + email: "admin@openbot.test", + name: "An Administrator", + image: null, +}; + +function appWith( + addServer: () => Promise, + role: "admin" | "user" = "admin", +) { + const store = { + addServer, + // Every read the plugins surface makes on its way to the route under test. + listServers: async () => [], + listSkills: async () => [], + listGrants: async () => [], + }; + + const app = createApp( + loadConfig(testEnvironment()), + { + handler: () => new Response(null, { status: 204 }), + api: { getSession: async () => ({ user: ADMIN }) }, + } as never, + { rolesForUser: async () => [role] }, + // Positions 4-14 are the other stores; `store` is 15, pluginStore. + ...(Array.from({ length: 11 }) as never[]), + store as never, + ); + + return (body: unknown) => + app.request("http://openbot.test/api/plugins/servers", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }); +} + +describe("adding a curated server", () => { + test("a refused credential comes back as a refusal with its reason", async () => { + const request = appWith(async () => { + throw new CustomServerRefusedError( + "That is not a credential this server can use. Add the server's own token instead.", + ); + }); + + const response = await request({ + key: "google-drive", + credentialId: "11111111-1111-1111-1111-111111111111", + }); + + expect(response.status).toBe(400); + expect(await response.json()).toEqual({ + error: + "That is not a credential this server can use. Add the server's own token instead.", + }); + }); + + test("an unknown catalogue key still comes back the same way", async () => { + const request = appWith(async () => { + throw new CatalogueEntryUnknownError("nope"); + }); + + expect((await request({ key: "nope" })).status).toBe(400); + }); + + test("a failure that is not a refusal is not dressed up as one", async () => { + // The must-not case. Mapping every throw to 400 would tell an administrator to correct their + // input when the database is down, and would hide a real fault behind a message about + // credentials. + const request = appWith(async () => { + throw new Error("the database is unreachable"); + }); + + expect((await request({ key: "google-drive" })).status).toBe(500); + }); + + test("somebody who is not an administrator cannot add one at all", async () => { + const request = appWith(async () => { + throw new Error("the store must not be reached"); + }, "user"); + + expect((await request({ key: "google-drive" })).status).toBe(403); + }); +}); diff --git a/server/tests/plugin-store.integration.test.ts b/server/tests/plugin-store.integration.test.ts index bddede09..65df095d 100644 --- a/server/tests/plugin-store.integration.test.ts +++ b/server/tests/plugin-store.integration.test.ts @@ -634,6 +634,12 @@ describe("a custom server may only be pointed at its own kind of credential", () const deploymentCredentialId = randomUUID(); const personalCredentialId = randomUUID(); const oauthClientCredentialId = randomUUID(); + /** + * The upsert case gets its own token, because a credential names the server it was minted for and + * that case adds a second server id. Sharing one row across two ids is a shape `storeMcpToken` + * cannot produce: it sets the provider to the server it is minting for, every time. + */ + const upsertCredentialId = randomUUID(); const customServerId = `custom-cred-${suffix}`; const madeServerIds: string[] = []; @@ -661,6 +667,14 @@ describe("a custom server may only be pointed at its own kind of credential", () encryptedValue: encrypted, metadata: {}, }, + { + id: upsertCredentialId, + kind: "mcp", + provider: `${customServerId}-upsert`, + keyId: `${customServerId}-upsert`, + encryptedValue: encrypted, + metadata: {}, + }, { id: oauthClientCredentialId, kind: "mcp_oauth_client", @@ -818,7 +832,7 @@ describe("a custom server may only be pointed at its own kind of credential", () id, title: "Collector", url: "https://collector.example/mcp", - credentialId: deploymentCredentialId, + credentialId: upsertCredentialId, by: "admin@example.com", }); @@ -836,7 +850,7 @@ describe("a custom server may only be pointed at its own kind of credential", () .select({ credentialId: mcpServers.credentialId }) .from(mcpServers) .where(eq(mcpServers.id, id)); - expect(row?.credentialId).toBe(deploymentCredentialId); + expect(row?.credentialId).toBe(upsertCredentialId); }); test("a custom server with no credential at all still works", async () => {