Spend an MCP token only for its own server, and only at its own address - #238
Open
beardthelion wants to merge 2 commits into
Open
Spend an MCP token only for its own server, and only at its own address#238beardthelion wants to merge 2 commits into
beardthelion wants to merge 2 commits into
Conversation
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.
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 CopilotKit#214 now mints its own token. It had reused one credential across two server ids, which is a shape storeMcpToken cannot produce.
beardthelion
requested review from
MikeRyanDev,
davidmckayv,
guidovizoso and
tylerslaton
as code owners
August 24, 2026 21:53
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #237.
Stacked on #231. This branch is based on that one, so its commit appears here too. Review or merge #231 first and this diff reduces to its own commit.
Attaching a credential to a server is the one place this deployment takes a reference to a stored secret rather than the secret itself, and the add is what spends it:
refreshToolsruns before the call returns and sends what it decrypts to the URL from the same request. #237 has the three ways the credential and the address could be made to disagree, each executed. This closes all three.provideralready records that, and nothing read it.storeMcpTokensets it to the server it mints for and is the only way the plugins screen makes anmcpcredential, so nothing a deployment can reach through the UI is refused by this.removeServerretires a 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 stored address left to compare against. The pointer now survives, removal therefore finds and retires it, and a retired credential is refused at attach time rather than accepted to fail on its next call.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.
Verification. Each rule was observed failing first, and the cases that must keep working passed before the fix rather than after. Five mutations, all red: removing the ownership clause fails 4 tests, the address rule 1, the surviving pointer 3, the live-credential check 1, and the custom route's refusal mapping 2. The full chain in #237 was re-run end to end against the fix: pointer survives, removal revokes, re-create refused, nothing sent. Two concurrent re-adds at different addresses were run and the second is refused. Route-level tests drive
POST /api/plugins/servers/customthrough the real store against Postgres and assert the stored URL is unchanged, not just the status. Suite 1482 pass, 0 fail; build, typecheck and lint clean.Two things a reviewer should look at deliberately.
The
deployment-bearerbranch cannot be reached while the catalogue is frozen with oneuser-oauthentry. Rather than add a seam to the store so a test could invent an entry, the curated check runs over whateverCATALOGUEholds, so the branch starts being exercised the moment somebody re-adds one of the vendors that were taken out. I confirmed that is not decorative by temporarily adding such an entry: the test then exercises the ownership branch and fails when that branch is neutered.The upsert test from #214 now mints its own token. It had reused one credential across two server ids, which is a shape
storeMcpTokencannot produce, and the new ownership rule refuses it.Not covered. Two concurrent creates of a server that does not yet exist both proceed, since neither sees a row. That is benign only because a live credential can no longer exist without its server row, which is what the third fix guarantees, and a hand-minted credential has a plaintext the administrator already knows. It is an argument rather than an execution, so it is worth a second opinion.