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

[beardthelion]

Closes #217.

What this changes

addCustomServer takes a credential id from the request body, and the add is what spends it. The
tool refresh runs before the method returns; for a custom server there is no catalogue entry, so
connectionTokenFor decrypts whatever that id names and listTools sends it to the URL from the
same request. Nothing checked which credential it was.

So an administrator could name any row in the vault. Naming one person's mcp_user_token had that
person's decrypted token arrive at an address the administrator chose, as a bearer token, during the
add, before any grant, policy check or Bot existed. GET /api/admin/credentials lists every row's
id, kind, provider and keyId, and for a user token the keyId is the person, so choosing a target
was a single read.

Only kind mcp answers "this server's own token". A user token is one person's grant and an OAuth
client identifies the deployment to a vendor. Spending either here uses a credential on behalf of
somebody who never agreed to it, which is the objection POST /api/admin/credentials already makes
when it refuses to mint those two by hand, in a comment saying so in as many words. This is the same
rule at the point they are referenced rather than created.

A credential that does not exist is refused in the same words as one of the wrong kind, so the
endpoint cannot be asked which ids are real. Two smaller things came out of the same guard, both
reachable through the route, which passes the body field through untouched: the id is shape-checked
before the lookup, because credentials.id is a uuid column and an unshaped value made the query
itself fail and handed back a database error where a refusal belongs; and an empty string now reads
as no credential rather than breaking the foreign key.

Deliberately not in this change. The field itself stays. Never accepting a caller-supplied
credentialId is the stricter option and is arguably the right one, since nothing in the app sends
it (addCustomServerMutationOptions has no callers, and the admin screen always mints a token and
points at the one it just made), but that is your call rather than mine and it is a smaller diff
than this one. Say the word and I will cut it to that.

A narrower gap this leaves open. The check is on kind, not on which server the credential
belongs to, so server A can still be pointed at server B's deployment token. I confirmed that by
running it. Both are deployment-owned rather than a person's, so it is not the cross-person case
above, but it is still a secret going to an address the caller picks. storeMcpToken writes
provider: serverId, so pinning on provider would close it; I left it out because it could break
a deployment that reuses one token across servers.

Where it runs

• New state that outlives a request? None. One select on credentials inside the existing
  call path, and no new writes.
• What happens on the second replica? Identical. The decision reads committed rows and
  depends on nothing held in the process.
• Anything serialised? The refusal runs before the existing upsert, so a concurrent add of
  the same id still resolves the way it did before: last writer wins on a row that has already
  passed the check. Nothing new races.
• Anything fanned out to a browser? No. The refusal is the response to the caller's own
  request, already rendered as a 400 by the route's existing CustomServerRefusedError branch.
• New listener, port, or schedule? None.

Boundary and audit

• Every acting call still goes through the gateway: resolve, decide, audit, then act. Untouched;
  this sits in front of a configuration write.
• New refusals and new failures each write a row. The refusal throws before the insert, so no
  configuration.changed row is written for a server that was never added. That is the point:
  the row this used to write is the one naming the credential it was about to spend.
• Nothing new is trusted from the client that the server can resolve itself. This removes trust.

Changelog

• A line under Unreleased, naming the disclosure: a deployment where somebody pointed a custom
  server at a person's connector token should treat that token as disclosed.

Proof

Against a migrated Postgres, driving the real addCustomServer with a real encrypted victim
credential and an HTTPS listener standing in for the attacker's collector.

Before:

LINK 1  add outcome        : ACCEPTED          mcp_servers rows: 1
LINK 2  Authorization: Bearer victim-refresh-token-0960cbd2
        carries the victim's plaintext secret: true
LINK 3  captured before addCustomServer returned: true

After:

LINK 1  add outcome        : refused (CustomServerRefusedError)   mcp_servers rows: 0
LINK 2  requests captured  : 0
LINK 3  captured before addCustomServer returned: false

Eight tests. Six of them are red before the change; the other two are the must-keep-working cases, a
server pointed at its own token and a server with no credential at all, which pass either way.
Removing the guard turns five red again.

One of those tests exists because of a branch the other seven missed. The add is an upsert, so an
existing server already holding its own token can be re-added naming somebody else's, and every
other test used a fresh id, so nothing reached that path. It turns out the behaviour there was
already correct and the existing pointer survives the refusal, but nothing proved it.

Corrected after first posting: this paragraph originally claimed that moving the check below the
insert "would have kept them all green," which is false. The first test asserts
expect(rows).toHaveLength(0), so an insert running before the refusal would turn it red on its own.
The branch was genuinely uncovered and the test is worth having; the justification I gave for it was
written from memory rather than from a run, and overstated.

Also exercised by hand and not kept as tests: uppercase and whitespace-padded ids are accepted and
normalise; a revoked credential of the right kind is still accepted and fails later at decryption,
which is pre-existing and leaks nothing, since decryptCredentialForUse refuses revoked rows.

829 non-integration and 195 integration tests pass. Typecheck clean on all four workspaces, lint back
to its pre-existing single info, format clean.

Note for whoever merges: #76 touches plugins/store.ts immediately after addCustomServer, where
this change sits, and the same test file in different places. Whichever lands second wants a rebase.

[davidmckayv]

Approving on the code, and on watching the tests bite.

Verified the hole rather than trusting the description. For a custom server catalogueEntry is null, so connectionTokenFor falls into the branch that decrypts whatever credential_id names with no kind check, and refreshTools runs inside addCustomServer and sends that plaintext to the URL from the same request. One admin call, any vault row, an attacker-chosen host. That is a confused deputy, and "admin-only" is not much comfort when the thing it hands over is the deployment's own model key or somebody's personal connector token.

The tests earn their place. I neutered the guard and ran the suite: five go red, including the upsert path and the id-oracle case. They are not decorative.

Two things I liked that the description undersells: refusing "wrong kind" and "no such credential" in the same words, so the endpoint cannot be asked which ids are real; and checking the UUID shape before the lookup, so a malformed id is a refusal rather than a database error surfacing as a 500.

One thing for a follow-up, not this PR. addServer — the curated path, one function above — takes credentialId with no kind check either. Not reachable today because the catalogue holds a single user-oauth entry with a fixed host, but the file's own comment invites forks to re-add the vendors that were taken out, and the hole reopens the moment one does. Worth closing while the reasoning is fresh.

Checks: typecheck, lint and format clean; CI green.