Refuse a tool grant naming an app this deployment has not added - #616
Merged
davidmckayv merged 2 commits intoSep 20, 2026
Merged
Conversation
CopilotKit#572 closed the way these rows were made — removing a connector now takes its grants with it — and its migration deleted the ones earlier removals had left. This is the other door into the same room, found while reviewing that PR. The mcp branch of `enablementRefusal` checked the role and nothing else, and `store.grant` is a bare upsert, so `POST /grants` with a ref naming an app this deployment had never added stored the grant. It was then invisible, for the reason CopilotKit#572 gives about the rows it deleted: the surface that reports a grant nothing advertises is built per server row, and there is no row. Add that app afterwards and the id is the same, the action names are the same, and every such grant resolves, with nobody having granted anything and no row in the trail saying one was made. It is admin-only, so this is a consistency hole rather than a privilege one. But CopilotKit#572's migration is what makes "no mcp grant names an app that does not exist" true for the first time, and an invariant is not established while one writer can still break it. The HTTP route is that writer and the only one: the package sync writes skill grants, never mcp. The rule it now follows is the one the `bot` branch four lines below already states — a grant that could never do anything is refused rather than stored. THE SERVER HALF ONLY. A grant naming a tool the server has stopped advertising is a supported state, held and not offered, because what a vendor lists today is not what somebody decided yesterday. Checking the tool here would refuse a re-grant of exactly the tool an administrator is restoring. AND GRANTS ONLY. Taking something away stays unchecked, for the reason the `intent` parameter's own docblock gives, and it matters more here than anywhere else: the rows this refuses are the shape CopilotKit#572 had to write a migration to delete, so an administrator who can see a dead row has to be able to remove it rather than wait for somebody to write another one. `serverExists` is one row, one column, one limit, rather than `listServers`, which materialises every tool and every grant in the deployment to answer a question about one id.
Hotragn
requested review from
MikeRyanDev,
davidmckayv,
guidovizoso,
mxmzb and
tylerslaton
as code owners
September 19, 2026 11:51
This was referenced Sep 19, 2026
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.
What this changes
Found while reviewing #572, and this is the half that PR did not cover.
#572 closed the way orphaned mcp grants were made — removing a connector now takes its grants with it — and its
0041migration deleted the ones earlier removals had left behind. The grant route is the other door into the same room.enablementRefusal's mcp branch checked the role and nothing else:and
store.grantis a bare upsert with no lookup. SoPOST /api/plugins/grantswith{"kind":"mcp","ref":"composio-slack/SLACK_SEND_MESSAGE","agentId":"…"}stored a grant for an app this deployment never added, and every consequence #572's changelog describes follows:It is admin-only, so this is a consistency hole rather than a privilege one. The reason to close it now is that #572's migration is what makes "no mcp grant names an app that does not exist" true for the first time, and an invariant is not established while one writer can still break it.
That writer is the only one. I checked both writers of
plugin_grants:store.ts:4419(this route) andtenant-package.ts:1047(package sync), and the sync writeskind: "skill"exclusively. So the HTTP route is the whole surface.Where it runs
select … limit 1on the grant path.Boundary and audit
botbranch's existing refusals, which write none either. Nothing was granted, so there is no configuration change to record;plugin_grantedstill goes down on every grant that succeeds.The rule it now follows is the one the
botbranch four lines below already states:Two scoping decisions, each with a test that pins it:
The server half only. A grant naming a tool the server has stopped advertising is a supported state —
GrantOnWithdrawnTool, held and not offered, because what a vendor lists today is not what somebody decided yesterday. Checking the tool here would refuse a re-grant of exactly the tool an administrator is trying to restore.Grants only, never revokes. The
intentparameter's own docblock gives the reason, and it matters more here than anywhere else: the rows this refuses are the same shape #572 needed a migration to delete. If the check applied to a revoke, the reason a dead row is wrong would be the reason it can never be removed, and an administrator looking at one in the UI would have to wait for somebody to write another migration.Changelog
CHANGELOG.md, underUnreleased.Proof
Four tests in
server/tests/plugin-grants-validation.test.ts, on the existing stubbed harness — no database needed for these.Being straight about which one is the regression test. With the fix reverted and all four present:
Only that one fails on
main. The other three pin the decisions above — that I did not over-refuse — and would pass onmaintoo, becausemainrefuses nothing. They are there so the next person to touch this branch finds out if they break the withdrawn-tool case or the revoke path.On this branch:
bun test server/tests/plugin-grants-validation.test.ts→ 30 pass, 0 fail.Whole server suite, against a migrated
openbot_testonpgvector/pgvector:pg17(CI's image and credentials):main+4is exactly the four tests added. The six are identical on both and none is in a file this touches — four aretool-selection fixture model environment restoration, plusproduction server loader boundaryandblank package endpoint…. I ran the branch twice to be sure of that number: one earlier run reported 9 failures with the same six names, so those fixtures are flaky under load; two consecutive runs both gave 6.cd server && bunx tsc --noEmitexits 0 with no output. Biome check on the four changed files: no fixes applied.What is not covered
"composio-slack"alone names a real app, so it passes this check, and is then inert becausecallToolrequires both halves. Same class of dead row, different cause; it wants its own decision about whether the route should require a slash, and I did not want to widen a boundary fix into a ref-format change.