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

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
 

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;
 }

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;

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<void> {
+    /*
+     * 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,21 +651,51 @@ 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({
           id: resolved.entry.key,
           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

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();
+  });
+});