From 515d826869d750a195abad81722b4c4e1bbf3394 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Mon, 24 Aug 2026 12:05:05 -0500 Subject: [PATCH] Point a curated MCP server only at a credential of its own kind Adding a server by URL checks which credential it is being pointed at. Adding one from the catalogue 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. The reach is narrower than the path beside it and worth saying so. 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 account, whose OAuth client is registered through its own call and sent to a pinned address. What was reachable is a credential of the wrong kind being accepted and spent on behalf of somebody who never agreed to it, a malformed id arriving as a database error where a refusal belongs, and the whole shape returning with the first deployment-bearer entry a fork re-adds, which the catalogue invites. Which kind an entry takes is decided beside the entry, because it is a property of the vendor's auth rather than of the request. Both add paths then ask one function the same question, so a credential that does not exist and one of the wrong kind are still refused in the same words and the endpoint cannot be asked which ids are real. The curated route maps that refusal to a 400 rather than letting it surface as a 500. Re-adding a curated server no longer clears the credential it points at. That column holds the OAuth client registering one put there, and a re-add to change an instance host said nothing about it while clearing it anyway, leaving the row orphaned and everybody who had connected told there is no client registered. --- CHANGELOG.md | 28 +++ server/src/plugins/catalogue.ts | 15 ++ server/src/plugins/routes.ts | 7 +- server/src/plugins/store.ts | 112 +++++++-- server/tests/plugin-catalogue.test.ts | 42 ++++ ...gin-curated-credential.integration.test.ts | 218 ++++++++++++++++++ .../tests/plugin-routes.integration.test.ts | 169 ++++++++++++++ server/tests/plugin-routes.test.ts | 105 +++++++++ 8 files changed, 671 insertions(+), 25 deletions(-) create mode 100644 server/tests/plugin-curated-credential.integration.test.ts create mode 100644 server/tests/plugin-routes.integration.test.ts create mode 100644 server/tests/plugin-routes.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index cbee9e84..3ee28ad6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,6 +59,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..97f0d761 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,58 @@ 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, + 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, + ); + const [named] = looksLikeId + ? await database + .select({ kind: credentialRows.kind }) + .from(credentialRows) + .where(eq(credentialRows.id, credentialId)) + : []; + + if (named?.kind !== kind) { + 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 +651,26 @@ 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, + credentialId, + serverCredentialKind(resolved.entry), + ); + } + await database .insert(mcpServers) .values({ @@ -605,14 +678,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(), }, @@ -693,28 +776,9 @@ export function createPluginStore(options: PluginStoreOptions) { */ 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.", - ); - } + // 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, credentialId, "mcp"); } await database 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-curated-credential.integration.test.ts b/server/tests/plugin-curated-credential.integration.test.ts new file mode 100644 index 00000000..4a9fd3ef --- /dev/null +++ b/server/tests/plugin-curated-credential.integration.test.ts @@ -0,0 +1,218 @@ +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 { + 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(); + }); +}); diff --git a/server/tests/plugin-routes.integration.test.ts b/server/tests/plugin-routes.integration.test.ts new file mode 100644 index 00000000..f43605b9 --- /dev/null +++ b/server/tests/plugin-routes.integration.test.ts @@ -0,0 +1,169 @@ +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") { + 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/api/plugins/servers", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }); +} + +const serverId = "google-drive"; +const suffix = randomUUID().slice(0, 8); +const personalCredentialId = 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; + + await database.insert(credentials).values({ + id: personalCredentialId, + kind: "mcp_user_token", + provider: serverId, + keyId: `user_someone_else_${suffix}`, + encryptedValue: await encryptSecret(`${"A".repeat(43)}=`, "not-read-here"), + 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, [personalCredentialId])); +}); + +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); + }); +}); 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); + }); +});