addCustomServer now refuses a credential id that is not the server's own token, after #214. addServer, the curated path immediately above it in server/src/plugins/store.ts, takes the same credentialId from the same admin request and checks nothing:
async addServer(input: { key: string; instanceHost?: string; credentialId?: string; by: string }) {
const resolved = resolveServerUrl(input.key, input.instanceHost);
...
credentialId: input.credentialId ?? null,What that is worth today, stated plainly rather than inflated. mcp_servers.credential_id is a real foreign key with onDelete: restrict, so an id naming nothing is rejected by the database. The catalogue holds one entry and it is user-oauth, so connectionTokenFor takes the OAuth-client branch and sends what it decrypts to Google's pinned tokenUrl, not to an address a caller chose. There is no exfiltration path here of the kind #214 closed.
What is left is still worth closing:
- A wrong-kind row is accepted and stored. Naming somebody's
mcp_user_token gets it decrypted and JSON.parsed as an OAuth client, which is a credential being spent on behalf of a person who never agreed to it, and the same objection POST /api/admin/credentials already makes when it refuses to mint those kinds by hand.
- A malformed id reaches a
uuid column, so the query itself fails and the caller gets a database error where a refusal belongs. That is the exact case the shape check in addCustomServer was added for.
- The hole reopens on the first
deployment-bearer entry. catalogue.ts says in its own comment that the removed vendors are in the history and re-adding one is a review of that vendor, so a fork that does it inherits an unchecked pointer that then goes to that vendor's host.
The kind a curated entry needs is decidable from the entry: deployment-bearer wants mcp, and a user-oauth entry gets its client through registerOAuthClient, which mints mcp_oauth_client itself, so a caller-supplied id there is not something to accept at all. The admin screen already matches that shape: it only sends credentialId on the deployment-bearer path.
Raised as a follow-up on #214 when it was approved.
One more thing on the same call, found while reading it. addServer's upsert sets credentialId: credentialId ?? null unconditionally, and registerOAuthClient keeps the client it minted in that same column. So adding an already-added curated server again, to change an instance host, clears it: the credential row is left with nothing pointing at it and nothing to revoke it, everybody who had connected gets "has no OAuth client registered for this deployment" on their next call, and the admin screen flips to Not registered. Reachable from the endpoint rather than from the screen, which removes the row first.
addCustomServernow refuses a credential id that is not the server's own token, after #214.addServer, the curated path immediately above it inserver/src/plugins/store.ts, takes the samecredentialIdfrom the same admin request and checks nothing:What that is worth today, stated plainly rather than inflated.
mcp_servers.credential_idis a real foreign key withonDelete: restrict, so an id naming nothing is rejected by the database. The catalogue holds one entry and it isuser-oauth, soconnectionTokenFortakes the OAuth-client branch and sends what it decrypts to Google's pinnedtokenUrl, not to an address a caller chose. There is no exfiltration path here of the kind #214 closed.What is left is still worth closing:
mcp_user_tokengets it decrypted andJSON.parsed as an OAuth client, which is a credential being spent on behalf of a person who never agreed to it, and the same objectionPOST /api/admin/credentialsalready makes when it refuses to mint those kinds by hand.uuidcolumn, so the query itself fails and the caller gets a database error where a refusal belongs. That is the exact case the shape check inaddCustomServerwas added for.deployment-bearerentry.catalogue.tssays in its own comment that the removed vendors are in the history and re-adding one is a review of that vendor, so a fork that does it inherits an unchecked pointer that then goes to that vendor's host.The kind a curated entry needs is decidable from the entry:
deployment-bearerwantsmcp, and auser-oauthentry gets its client throughregisterOAuthClient, which mintsmcp_oauth_clientitself, so a caller-supplied id there is not something to accept at all. The admin screen already matches that shape: it only sendscredentialIdon thedeployment-bearerpath.Raised as a follow-up on #214 when it was approved.
One more thing on the same call, found while reading it.
addServer's upsert setscredentialId: credentialId ?? nullunconditionally, andregisterOAuthClientkeeps the client it minted in that same column. So adding an already-added curated server again, to change an instance host, clears it: the credential row is left with nothing pointing at it and nothing to revoke it, everybody who had connected gets "has no OAuth client registered for this deployment" on their next call, and the admin screen flips to Not registered. Reachable from the endpoint rather than from the screen, which removes the row first.