Spend an MCP token only for its own server, and only at its own address (#238)

* 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.

* Spend an MCP token only for its own server, and only at its own address

Attaching a credential to a server is the one place this deployment accepts a
reference to a stored secret rather than the secret itself. Everywhere else the
value arrives in the request that stores it, and the id it gets is nobody's to
choose: storeAgentAuth mints its own row from the key an administrator typed. So
this is the field where which secret and which address can be made to disagree,
and the add is what settles it, because refreshTools runs before the call
returns and sends what it decrypts to the URL from the same request.

Both ways they could disagree are now refused. A credential has to belong to the
server it is attached to, which the vault already records: storeMcpToken sets
the provider to the server it mints for and is the only way the plugins screen
makes one, so nothing a deployment can reach through the UI is refused by this.
And a server that already holds a credential cannot be re-added at a different
address, which is the case a check on ownership cannot see: the token does
belong to that server, and only the address moved.

The second is why the first is not enough alone. Both delivered a stored token
to a host the caller named, before any Bot, grant or policy check existed, and a
stored credential is otherwise unreadable by design.

Refused rather than repaired, because both harmless readings are served by
something else. Correcting a title or retrying an interrupted add sends the same
URL and is untouched, a server holding no credential can still be re-addressed,
and moving one that does means removing it and adding it again with the token
the new address is meant to have.

Curated servers are unaffected: their URL comes from the catalogue rather than
the request, and an instance hostname is matched against the vendor's anchored
pattern before anything is stored.

The upsert test from #214 now mints its own token. It had reused one credential
across two server ids, which is a shape storeMcpToken cannot produce.

* Show a dot on a channel a Bot has spoken in unseen (#259)

* Give a membership a memory of when its channel was last read

* Stamp the caller's membership read, and say so in the roster

* Let a member say they have read a channel

* Carry the read marker to the app and let it be stamped

* Draw a dot on a channel a Bot has spoken in unseen

* Mark a channel read the moment it is the one on screen

* Narrow the admin and settings rails to the width their labels earn

* Keep a fast clock elsewhere from turning mark-read into a storm

* Say in the changelog what the unread dot is and is not

* Stamp a read against the message clock, not just this one

* Assert the clamped stamp without reaching through an optional chain

* Regenerate the read-marker migration behind the queue and the frames

* Refuse to mark a deleted channel read, matching the pin

* Update azure/setup-helm action to v5 (#260)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Say what a strict content-security-policy has to allow (#225)

* Point the test at the database the project actually has (#234)

* Let the API reach Intelligence and sign-in when a NetworkPolicy is on (#257)

* Refuse a credential written into the rest of the address (#230)

Refusing a credential in the userinfo closed one spelling and left the two
beside it open. A token in the query string or the fragment was accepted, and
addCustomServer writes the address it was given into mcp_servers.url and into
the configuration.changed audit payload verbatim. Redaction keys on the field
name and url is not a sensitive one, so the secret landed in an append-only row
in clear text, which is the disclosure the userinfo rule exists to prevent.

The name is read rather than matched against a list. An exact-name version of
this rule refused token and accepted auth_token, api_token, x-api-key and
X-Amz-Signature, and an operator has no way to know which spellings the check
happens to hold. Reading the name over-refuses in one direction on purpose: a
misread parameter costs a rename, a missed one cannot be deleted afterwards.
The fragment is split at the first question mark first, because a hash route or
an OAuth-style callback puts a path in front of the parameters and reading the
whole fragment as one query string turns all of it into a single name that
matches nothing.

metadata.goog is refused too, by asking the list browsing already uses rather
than keeping a second copy here. It is Google's own short alias for the metadata
server and it carries a dot and none of the suffixes this check lists, so it
read as an ordinary vendor name, while the long spelling was refused only
incidentally by the .internal test.

---------

Co-authored-by: Guido Vizoso <guido.vizoso9@gmail.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Vaibhav Zope <121456155+zopeVaibhav@users.noreply.github.com>
Co-authored-by: David McKay <davidmckayv@users.noreply.github.com>

Author: 5 people

CHANGELOG.md

@@ -192,6 +192,46 @@ read that as a stolen token and revoke the whole connection. Every plugin call t
 token now locks the credential's vault row for the length of the exchange, so a second replica waits
 rather than races, and the rotated token is written back in the same transaction that held the lock.
 Nothing to configure; a connection just stops going stale under concurrent traffic.
+
+### 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
@@ -269,6 +309,34 @@ beside `metadata.google.internal`, and it carries a dot and none of the suffixes
 it read as an ordinary vendor name. The long spelling was only ever refused incidentally, by the
 `.internal` rule. Both are now named, so the address this check was written for is refused on purpose
 rather than by luck.
+### 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

@@ -266,6 +266,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

@@ -202,7 +202,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

@@ -32,6 +32,7 @@ import {
   classifyTool,
   customUrlRefusal,
   resolveServerUrl,
+  serverCredentialKind,
 } from "./catalogue";
 import { McpServerError } from "./mcp";
 import { registerDynamicClient } from "./oauth";
@@ -1373,6 +1374,84 @@ export function createPluginStore(options: PluginStoreOptions) {
     }
   }
 
+  /**
+   * 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<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,
+      );
+    /*
+     * 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()
@@ -1411,21 +1490,52 @@ 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({
           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(),
           },
@@ -1504,30 +1614,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
@@ -1546,7 +1677,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(),
           },