Skip to content

Refuse a tool grant naming an app this deployment has not added - #616

Merged
davidmckayv merged 2 commits into
CopilotKit:mainfrom
Hotragn:refuse-a-grant-naming-no-app
Sep 20, 2026
Merged

davidmckayv merged 2 commits into
CopilotKit:mainfrom
Hotragn:refuse-a-grant-naming-no-app

Conversation

@Hotragn

@Hotragn Hotragn commented Sep 19, 2026

Copy link
Copy Markdown
Contributor

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 0041 migration 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:

if (kind === "mcp") {
  return actor.isAdmin ? null : "An administrator decides which Bots may reach a tool.";
}

and store.grant is a bare upsert with no lookup. So POST /api/plugins/grants with {"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:

  • Nothing shows it, for the reason that PR 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 later and the id is the same, the action names are the same, and the grant resolves.
  • Nobody granted anything for that app, and the trail has no row saying they did.

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) and tenant-package.ts:1047 (package sync), and the sync writes kind: "skill" exclusively. So the HTTP route is the whole surface.

Where it runs

  • New state that outlives a request? None. One select … limit 1 on the grant path.
  • What happens on the second replica? The same. It is a read of a committed row; two administrators granting concurrently both see the app or both do not.
  • Anything serialised? No.
  • Anything fanned out to a browser? No.
  • New listener, port, or schedule? No.

Boundary and audit

  • Every acting call still goes through the gateway. Untouched.
  • New refusals and new failures each write a row. This refusal writes none, deliberately — it is the same class as the bot branch's existing refusals, which write none either. Nothing was granted, so there is no configuration change to record; plugin_granted still goes down on every grant that succeeds.
  • Nothing new is trusted from the client. The ref is what it always was; the app it names is now resolved against this deployment's own rows instead of taken on faith.

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, from both ends.

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 intent parameter'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, under Unreleased.

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:

(fail) POST /api/plugins/grants, for an app this deployment does not have > refuses the grant and never reaches the store
 29 pass, 1 fail

Only that one fails on main. The other three pin the decisions above — that I did not over-refuse — and would pass on main too, because main refuses 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.ts30 pass, 0 fail.

Whole server suite, against a migrated openbot_test on pgvector/pgvector:pg17 (CI's image and credentials):

pass fail
main 3270 6
this branch 3274 6

+4 is exactly the four tests added. The six are identical on both and none is in a file this touches — four are tool-selection fixture model environment restoration, plus production server loader boundary and blank 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 --noEmit exits 0 with no output. Biome check on the four changed files: no fixes applied.

What is not covered

  • A ref with no tool half. "composio-slack" alone names a real app, so it passes this check, and is then inert because callTool requires 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.
  • A grant made while the app exists, for an app later removed. That is Remove a connector's grants when the connector is removed #572's path and it is handled there.

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.
@davidmckayv
davidmckayv merged commit 25c5e3d into CopilotKit:main Sep 20, 2026
17 checks passed
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.

2 participants