fix(ui): stop a deselected MCP server keeping its grant on a virtual key - #36840
yassin-berriai merged 3 commits into
Conversation
|
|
Greptile SummaryThe PR updates virtual-key MCP entitlement serialization so deselected server allowlists are removed, empty maps are persisted, unavailable catalogs block ambiguous revocations, and all-proxy grants retain applicable restrictions
Confidence Score: 5/5The PR appears safe to merge No blocking failure remains
|
| Filename | Overview |
|---|---|
| ui/litellm-dashboard/src/components/mcp_server_management/mcpEntitlement.ts | Centralizes MCP selection and allowlist normalization, including indirect grants, identifier aliases, and all-proxy handling |
| ui/litellm-dashboard/src/components/templates/key_info_view.tsx | Applies normalized MCP entitlements unconditionally and blocks ambiguous permission saves while the server catalog is unavailable |
| ui/litellm-dashboard/src/components/templates/key_info_view.test.tsx | Adds regression coverage for allowlist pruning, retention, clearing, catalog failure, and preservation of sibling object permissions |
| ui/litellm-dashboard/src/app/(dashboard)/users/_components/view_users/user_info_view.tsx | Reuses the shared MCP entitlement helper instead of maintaining a duplicate implementation |
| ui/litellm-dashboard/src/components/templates/KeyInfoView.handleKeyUpdate.test.tsx | Supplies the MCP-server hook mock required by the updated key editor |
Reviews (3): Last reviewed commit: "fix(ui): stop a deselected MCP server ke..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| * The `object_permission` a save sends, derived from what the editor currently shows. | ||
| * | ||
| * A tool allowlist is what narrows a grant and an absent one reads as no restriction, so dropping | ||
| * an entry is the direction that widens. An entry is kept when an access group or toolset the admin | ||
| * retained could still supply its server, and dropped once nothing indirect survives, which is what | ||
| * makes removing a grant actually remove it. | ||
| * | ||
| * A tool-permission key may be a server id, a name or an alias: the gateway normalizes all three | ||
| * before looking up the allowlist, so an entry written by the API or by config can use any of them. | ||
| * `allServers` is what resolves a key to its servers, plural: names and aliases are not unique, and | ||
| * the gateway unions such a key into EVERY server answering to it, so the entry is kept while any | ||
| * one of them is still granted. Resolving to the first match instead would make the outcome depend | ||
| * on catalog order and could drop a restriction that was also covering a server still granted. A key | ||
| * that resolves to nothing is kept too, since a server we cannot identify is one we cannot confirm | ||
| * was deselected; that also covers a catalog that has not loaded or failed to load, where every key | ||
| * is unresolvable and nothing is pruned. | ||
| */ | ||
| export const extractMcpEntitlement = ( | ||
| formValues: Record<string, unknown>, |
There was a problem hiding this comment.
The new helper narrates its filtering implementation in an oversized source comment, while the associated payload test uses Record<string, any>; this duplicates maintainable behavior in prose and removes static checking from the submitted payload shape.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Kept both. The comment records which filter direction widens access, a CLAUDE.md carve-out. The any matches this file's existing submit mock.
There was a problem hiding this comment.
Understood on both. The comment documents a non-obvious security invariant (which direction of the filter widens access) rather than narrating implementation, which is exactly the carve-out. And keeping the accessor typed consistently with the file's existing Record<string, any> submit mock is the right call — tightening one against the other would be inconsistent rather than safer. Resolved.
32d6228 to
3379489
Compare
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
3379489 to
69ebe73
Compare
|
@greptileai please re-review at 69ebe73. Catalog-unavailable finding taken: the editor now refuses that save. Reasoning in the description under Review notes. |
The key editor sent `mcp_tool_permissions` unfiltered, and the MCP resolver counts a server named only under `mcp_tool_permissions` as entitled, unioning `tool_perm_servers` into `all_servers` at four sites in `user_api_key_auth_mcp.py`. Deselecting a server, or removing the access group that supplied it, therefore left a stale entry that kept the key reaching that server with its old tool allowlist attached. Reuse `extractMcpEntitlement`, which already landed for the internal-user surface, so the key surface drops an entry only once the server is known and no longer granted, and keeps it whenever a retained access group or toolset could still supply it. The helper moves to a shared module so the key template does not import a users page component. Setting the map unconditionally is part of the same fix: the old `Object.keys(...).length > 0` guard let the previous map ride through the `object_permission` spread, which filtering to an empty map would otherwise hit in exactly the case the fix is for.
69ebe73 to
baf11a9
Compare
|
@greptileai please re-review at baf11a9. Fixed the ui-unit-tests break and made the all-proxy grant retain allowlists. Details in the description. |
…ning tool permissions Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
Verified on a live proxy with two real MCP servers: deselecting a server prunes only its allowlist, retained groups and toolsets keep theirs, emptied allowlists persist. Live evidenceNot coveredThe all-proxy-servers sentinel is unreachable from the key edit form, which builds |
3 participants
TLDR
Problem this solves:
How it solves it:
User Flow
Before: an admin removes an MCP server from a virtual key, the key keeps reaching that server
After: the same removal actually revokes the server
Scope
The Linear ticket is a roll-up over ten issues, so state plainly which one this PR closes and which it does not, rather than letting the reader infer that a roll-up closing means all of it shipped. Status re-derived against staging rather than taken from the ticket's present tense:
So this PR closes the key half of the fourth item only. The roll-up should stay open until #35153 and #35154 land.
Relevant issues
Linear ticket
Resolves LIT-5512
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
There are no screenshots here on purpose, and the reason is the defect itself. The stale grant is invisible in the UI by construction:
MCPServerPermissionsbuilds its list frommcp_serversandmcp_access_groups, then looks upmcpToolPermissions[item.value]only for servers already in that list, so an entry whose server is absent is never iterated and renders nowhere. The key detail page is therefore byte-identical before and after, and a before/after pair would be two identical screenshots. That invisibility is why this bug survived: it is a live grant the admin can neither see nor edit.This PR also adds no rendered surface at all. The diff is a new helper module, a save handler and tests, and it is net negative inside the component, so the edit form looks the same either way. What changes is the entitlement the save produces, which is what the evidence below measures.
Live proxy against real Postgres and two real HTTP MCP servers, no mocks. Both legs run against the same proxy and the same database, from the same armed starting state, and differ only in the payload the key editor sends on save. That payload is the whole change, so the gateway is a fixed control here.
The gateway caches a key's resolved permissions for about a minute, so every read below is taken after a 75 second settle. Reading sooner returns the pre-save answer and makes both legs look identical, which is how this first read as "no difference".
Proof captured at
baf11a9399, whose parent262ed530f8is the before tree.git show 262ed530f8:ui/litellm-dashboard/src/components/templates/key_info_view.tsx | grep -c extractMcpEntitlementreturns 0, so the before tree genuinely lacks the fix.Setup:
The key is granted both servers, with one allowed tool on each:
Armed state, read as the key itself:
Before. The admin deselects
lit5512_wiki. This is the save the unfixed editor sends, with the allowlist map passed through untouched:lit5512_wikiis gone from the granted servers, yetread_wiki_contentsis still served: the leftover allowlist entry is itself a grant, so the revoked server is still reachable.After. Same armed state, same admin action, and this is the save the fixed editor sends:
The revocation now lands. The remaining server keeps exactly the allowlist the admin left on it.
Video of the UI legs, walked through the key editor against that same live proxy with two real MCP upstreams, is in the Slack thread: https://berriaillm.slack.com/archives/C0AE9HJQUHG/p1786662123427539?thread_ts=1786662123.427539&cid=C0AE9HJQUHG. It was captured at
dba4ea87f6, the head at the time, while the curl legs above were captured before and afterbaf11a9399. A comment on this PR carries the same per-case output plus the screenshot.The later commit that resolves retained groups and toolsets per server changes only which payload the editor computes, not how the proxy resolves it, so the same two legs above are the live evidence: the entitlement the second leg sends is exactly what the group case now produces once the retained group does not cover the removed server. That mapping is pinned by unit tests on both the helper and the save handler rather than re-curled.
Type
🐛 Bug Fix
Caveats (if any)
Review notes
Also fixed after the first pass, found by re-reading my own contract rather than from a review: the rule is meant to drop an entry only once its server is no longer granted, but selecting "All MCP Servers" left every allowlist key unresolvable against a catalog of real servers and dropped them all, widening tool access on servers that were already restricted. The all-proxy grant can supply any server, so it now retains entries exactly like an access group does.
Taken, from the first Greptile pass: the filter resolves each allowlist key against the MCP server catalog and keeps any key it cannot resolve, so an unavailable catalog made every key unresolvable, pruned nothing, and let a revocation save as a no-op while reporting success. I reproduced it before accepting it, with the catalog hook returning no data, and the save went through carrying both stale entries. The editor now refuses that save rather than reporting a success it did not perform. Keeping the unresolved-key fallback itself is deliberate: names and aliases are not unique and the gateway unions such a key into every server answering to it, so dropping an unresolvable entry could revoke a restriction still covering a granted server.
The ticket describes the clearing defect as an admin unticking the last tool, submitting an empty map that a
length > 0guard then discards. That is not reachable: unticking the last tool submits{server: []}, which has length one and passes the guard, and the tool matrix never deletes a key from the map, so what the editor submits is always a superset of what was stored. The guard was dead code.The two lines therefore have to change together, which is worth stating because a reviewer will not see the coupling: filtering alone leaves the clear path broken, and removing the guard alone prunes nothing. The regression tests pin the interaction rather than assuming it. Each line was mutated separately and each mutant dies on its own: reinstating the guard while keeping the filter fails only the deselect-everything test, and disabling the filter while keeping the guard removed fails three others.
Which hop loses the entries is worth stating, because the obvious reading is wrong and it is what makes this separate work from #35154. The entries are not lost on the read path: the resolver has them, the API returns them in
object_permission.mcp_tool_permissions, and the page receives them. They are lost at RENDER, because the tool matrix filters to directly selected servers, and that is #35154's fix. The hop with the authorization consequence is the WRITE: the page sends those never-rendered entries straight back on save, and since a server named only undermcp_tool_permissionsis entitled in the resolver, an entry the admin can neither see nor edit stays a live grant that survives the removal. Fixing the render hop alone would leave that in place.Both directions of the defect were swept, since a one-directional sweep cannot see its own mirror image. Direction one, the UI reads something the API never resolves, is the models half and returned clean because it already shipped. Direction two, the API returns something the UI never renders, is what this PR fixes: the API returns
mcp_tool_permissionsentries for indirectly granted servers, the tool matrix never renders them because it filters to directly selected servers, and they are load-bearing grants in the resolver. Naming the clean direction matters, because a clean sweep reads like proof of absence when it may only be proof that nobody looked.Sweeping the sibling surfaces: key creation builds
object_permissionfrom scratch rather than spreading stored state, so it cannot inherit a stale entry, but it never filters the allowlist either. Selecting a server, letting the matrix populate it, then deselecting it still leaves an entry, and a brand new key comes out entitled to a server the admin removed before saving. That surface omits empty fields to mean "unset", so the same helper does not drop in unchanged and forcing it there would change more than this fix should. Left for a follow-up rather than widened into this PR.It stops being dead the moment the filter lands, because filtering out the last entry produces exactly the empty map the guard discarded, which would have defeated the fix in the one case it exists for. So both lines change together, which is the conclusion the ticket reached from a premise that does not hold.
Inline findings
Three inline threads, adjudicated individually. Note that GitHub re-anchors
commit_idto the current head, so a fixed finding reads as freshly filed against the very commit that fixes it;original_commit_idis the field that tells the truth.Unavailable catalog preserves revoked grants (P1, security). Taken, and already fixed. Filed against
32d6228ca4; the guard landed in69ebe7300eand is live atkey_info_view.tsx:211. The editor now refuses a save it cannot compute rather than reporting a success it did not perform.Deselected server grant is retained when an unrelated group survives. Taken, and now fixed here rather than deferred. Retention is resolved per server instead of per selection: an entry survives while its own server is directly selected, carries one of the retained access groups in
mcp_access_groups, or appears as atools[].server_idof a retained toolset, so a group covering only some other server no longer keeps a deselected server callable. My earlier reading that the editor cannot resolve membership was wrong. The catalog already carries both edges,mcp_access_groupsper server andserver_idper toolset tool, which is the same membership the gateway resolves against, so #35154's rendering work is not a prerequisite. The safe direction is preserved where information is genuinely missing: an unresolvable identifier still keeps its entry, since names and aliases are not unique, and a selected toolset absent from the catalog prunes nothing rather than guessing, because a missing entry reads as "no restriction" and over-pruning is the widening direction.Oversized comment and
Record<string, any>in the test (P2). Kept, on both halves. The comment is not narration of the implementation, it records which direction of a permission filter widens access and why an unresolvable key is retained, which is the "very complex business logic" carve-out in CLAUDE.md and is the reasoning a future editor most needs before touching this rule. It also moved verbatim from #35146 rather than being written here. Theanysits in a test-local payload accessor in a file whose submit mock is already typedRecord<string, any>; tightening one accessor against that would be inconsistent rather than safer, and the repo's no-explicit-any budget is unchanged by this PR.Final Attestation
Link to Devin session: https://app.devin.ai/sessions/81574b7220ed40419806ac0b41929672
Requested by: @yassin-berriai