Skip to content

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

Open
beardthelion wants to merge 1 commit into
CopilotKit:mainfrom
beardthelion:fix/curated-server-credential-kind
Open

Point a curated MCP server only at a credential of its own kind#231
beardthelion wants to merge 1 commit into
CopilotKit:mainfrom
beardthelion:fix/curated-server-credential-kind

Conversation

@beardthelion

Copy link
Copy Markdown
Contributor

Closes #229.

#214 gave addCustomServer a rule about which credential it may be pointed at. addServer, the curated path immediately above it, took the same field from the same admin request and stored it unread.

The reach is narrower than the path beside it and worth stating rather than dressing up. mcp_servers.credential_id is a real foreign key, so an id naming nothing was already refused by the database, and the one entry in the catalogue is user-oauth, so what it decrypts goes to a pinned vendor address rather than one a caller chose. There is no exfiltration path here of the kind #214 closed. 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 reaching a uuid column so the caller gets a database error where a refusal belongs, and the whole shape returning with the first deployment-bearer entry a fork re-adds, which catalogue.ts invites in its own comment.

Which kind an entry takes is decided beside the entry, since 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 "wrong kind" and "no such credential" 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.

One thing found while reading it, fixed here because the change would otherwise make it unfixable. The upsert set credential_id unconditionally, and registerOAuthClient keeps the client it minted in that column, so re-adding a server to change an instance host cleared it: the credential row was orphaned with nothing to revoke it, and everybody who had connected was told the deployment has no client registered. Passing the id back was the only repair, and this PR refuses that on a user-oauth entry, so the pointer now survives a re-add that names no credential.

Verification. The refusals were observed failing first. Three mutations, all red: deleting the guard call from addServer fails three tests, restoring the clobbering upsert fails the OAuth-client test, and neutering the shared kind check fails all five of the custom-path tests from #214, so the extraction did not weaken code that was already reviewed. A route test covers 400 for a refusal, 400 for an unknown key, 500 for a failure that is not a refusal, and 403 for a non-admin. An integration test drives POST /api/plugins/servers through the real store against Postgres with nothing stubbed in between, and both mutations bite through that path too. Full suite 1437 pass, 0 fail, run twice for cross-run residue; build, typecheck and lint clean.

Not covered, deliberately. A curated entry whose auth is deployment-bearer, pointed at a wrong-kind credential, cannot be produced while the catalogue is frozen with one user-oauth entry, and I did not add a test-only seam to createPluginStore to reach it. It is covered as two halves instead: serverCredentialKind is unit-tested for that branch, and the lookup itself is proven load-bearing by mutation on the custom path.

Separate, not fixed here. requireCredentialOfKind checks the credential's kind but not its provider, so any mcp credential works for any server. On the custom path, where the URL is caller-chosen, that is the sharper version of the problem. It is pre-existing and closing it would refuse configurations some deployments may already run, so it wants its own issue and its own call.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A curated MCP server can be pointed at any credential in the vault, and re-adding one clears its OAuth client

1 participant