fix(ui): show indirectly granted and name-keyed MCP servers in the tool matrix - #35154
yassin-berriai merged 10 commits into
Conversation
|
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryThis PR makes MCP permission editing reflect the backend’s effective grants rather than only directly selected server IDs.
Confidence Score: 5/5The PR appears safe to merge; no actionable new defects or outstanding previous findings remain. The effective-grant resolver, editor write path, and team-save retention logic consistently account for direct, inherited, toolset, name-keyed, and ambiguous grants. The prior equivalent-key finding is fixed, the backend-name finding was correctly withdrawn, and the team-retention threads were resolved or explicitly confirmed as intended behavior.
|
| Filename | Overview |
|---|---|
| ui/litellm-dashboard/src/components/mcp_server_management/effectiveMcpServers.ts | Centralizes effective-server resolution, identifier precedence, equivalent-key normalization, and toolset-derived permissions. |
| ui/litellm-dashboard/src/components/mcp_server_management/MCPToolPermissions.tsx | Renders direct and inherited MCP grants accurately, locks toolset-owned tools, and routes edits through normalized permission writes. |
| ui/litellm-dashboard/src/components/team/TeamInfo.tsx | Retains tool allowlists for the effective team grant set and safely reloads unified access-group grants when local inventory is incomplete. |
| ui/litellm-dashboard/src/components/permissions/MCPServerPermissions.tsx | Aligns server lookup and displayed allowlists with ID, name, and alias resolution. |
| ui/litellm-dashboard/src/components/Teams.tsx | Includes selected MCP toolsets in team creation and passes complete selections to the tool matrix. |
| ui/litellm-dashboard/src/app/(dashboard)/agents/_components/add_agent_form.tsx | Persists selected MCP toolsets in agent object permissions. |
| ui/litellm-dashboard/src/components/mcp_tools/McpCrudPermissionPanel.tsx | Prevents locked toolset-derived grants from being changed through individual or group controls. |
Reviews (9): Last reviewed commit: "fix(ui): guard MCP default write on tool..." | Re-trigger Greptile
|
Fixed the P1 on equivalent permission keys in dac8138 You were right that a write was authoritative for one key rather than for the server. A server can be named by several The resolver now reports, per server, the key an edit keeps, the equivalent keys it supersedes, and the union those keys currently allow. The editor renders the union, so a card shows what the backend actually enforces, and every write goes through one function that writes the kept key and drops the superseded ones; after an edit there is exactly one entry per server and it matches what the admin sees. The kept key is the one already in the map, preferring the id when both are present, so a name-keyed grant still round-trips to its own key instead of being silently rewritten to an id the operator never chose. One deliberate exception, because collapsing blindly repeats the same bug in the other direction: a key that also names a DIFFERENT server, which happens when two servers share a name, is never dropped, since dropping it would strip that other server no one was editing of its restriction. Such a key stays and the card keeps showing the union it contributes, so the state stays visible rather than silently altered. That case has its own test. New tests, each mutation-checked: the union read; superseded keys reported; a write collapsing onto the kept key; a single-key server and every other server left untouched; the shared-name key never dropped; an entry added for a server that had none; one card rendering the union of both keys; a deselect removing the tool from every equivalent key and leaving one entry; and one card badged by its strongest grant when a key and an access group both name the server. Five single-line mutations confirm they bite: never superseding kills 3, dropping the shared-name guard kills 1, reading one key instead of the union kills 4, writing one key instead of collapsing kills 1, never appending an entry kills 3. Reverting both components to the parent commit fails 15 of 15 new tests while all 14 pre-existing ones pass. Suites: 54 pass in the two component directories, 484 across every directory this PR touches. Lint 0 errors, all six budgets keep headroom, build compiles. On the badge question implicit in your review: dedup is by On the |
|
The implementation is thorough and addresses everything correctly. Walking through the key pieces: Equivalent-key union/collapse (
Inherited-server default-allowlist gate ( const isDirect = entry.source.kind === "direct";
const unrestricted = mcpAllowedToolsFor(entry.server, latestPermissions) === undefined;
if (isDirect && unrestricted && fetchedTools.length > 0) {Correct — inherited servers no longer get a narrowing default written on open. Dedup by strength —
The known limitation on empty access groups (group selected, no member servers in the fetched list → silent gap until a membership endpoint exists) is correctly scoped and documented. The warning banners on fetch failure cover the more dangerous case where the server list itself couldn't load. This all looks correct. The mutation evidence and test coverage are convincing. The one thing I'd have the maintainer confirm before merge is the LGTM on the logic. |
![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: 6 · PR risk: 0/10 |
UI capturesDriven against a live proxy on a real MCP server, one restart per bundle. Both bundles were built from this branch's worktree; the before bundle is the whole Fixture: a config-defined MCP server A server reached only through an access group now renders, badged and editable. This is the case the editor could not show at all: the team holds no direct server, so the matrix was empty and an admin had no way to see or clear the allowlist that was in force.
Note the tool state is the real one: A name-keyed grant resolves. Same key, same frame, before and after. A config-defined server has a null alias, so the label interpolated it literally; the lookup was also id-only, so a name-keyed allowlist could not be found. before, at
after
The failure-path banner is deliberately not screenshotted; it is pinned by two tests that fail without it, which is tighter evidence than a frame of a blocked request would be. |
|
Both findings addressed in 71f3978 Finding 1, shared-name keys. Correct, and I agree it is not resolvable by any filter: retaining the key is right for the neighbouring server and wrong for the server being edited. Retaining stays the default because it is the only direction that cannot widen anyone access; dropping the key would silently strip a restriction from a server nobody was editing. The cost of that choice was invisible, which was the actual defect, so the editor now states it: the resolver reports ambiguous keys separately from superseded ones, and the card names them and says their tools stay allowed until the servers no longer share the name, which points at the fix an admin can apply (rename or split) rather than leaving them to infer it from an edit that bounces back. The PR body now states which direction was chosen and why. One residual I want on the record rather than buried: a key naming a server that the fetched server list does not include cannot be detected at all, so the notice cannot fire for it. That is inherent to resolving membership on the client and is documented in the body next to the empty-group limit. Two tests, both mutation-checked: the notice renders naming the ambiguous key when two servers share a name; and it does not render when every key names one server. Removing the notice kills the first; calling every extra key ambiguous kills the second plus the superseded-keys test. Finding 2, the TeamInfo save filter. Verified rather than assumed, and it is a pre-existing gap, not a regression from this PR. The filter at Test and check state at this SHA: 56 pass in the two component directories, 486 across every directory this PR touches, 0 eslint errors, all six lint budgets with headroom, build compiles. Reverting both components to the parent fails 16 of the 17 new tests, with the one exception noted in the body being the guard that has to pass when the notice is correctly absent. |
|
Both findings are addressed correctly. Finding 1 — shared-name keys: The resolver cleanly separates Finding 2 — TeamInfo filter: Verified pre-existing, not a regression. The delta makes the gap reachable from a new control but does not change what the handler stores, and the LIT-4961 sequencing note in the PR body is the right way to handle it. Agreed that landing these together (or that one first) is the cleaner order. The implementation is solid. LGTM. |
|
Filed the underlying data-layer issue as LIT-4982 so this PR is not asked to carry a backend argument: a name- or alias-keyed |
|
Test-only hardening in 2c5600e, no production diff The shared-name tests were coin flips and I measured it rather than assuming otherwise. An identifier can resolve to several servers, so a first-match implementation is right in one catalog order and wrong in the other, which means a single fixture only catches it when it happens to be written in the wrong-answer order: Both shared-name tests now run over both catalog orders, so the mutant fails the same named case every time instead of depending on how a later fixture happens to be ordered. The Worth stating as the general rule, since it is the root cause one level below the grant-inference bugs this PR is about: using a single-answer primitive for a multi-answer question is itself an inference that the first match is the only one. Any lookup resolving an identifier against a collection that can hold several matches needs its tests parameterized over every ordering, otherwise the suite reads as coverage while checking nothing. State at this SHA: 59 pass in the two component directories, 489 across every directory this PR touches, 0 eslint errors, all six lint budgets with headroom, build compiles. Nothing else changed; the production code is identical to 71f3978. |
|
Confirmed — the commit touches only the two test files (48 changes total, zero production lines), and the hardening is exactly right. The underlying principle you've stated is worth internalizing as a general rule: any lookup resolving an identifier against a collection where multiple matches are possible is implicitly a first-match decision, and a single fixture only detects a regression in first-match logic when the fixture happens to be ordered so the wrong answer comes first. Parameterizing over both orders makes the kill deterministic by construction rather than by accident. The The production logic at 71f3978 is unchanged. This hardening commit is a straightforward sign-off. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Same note as on #35153, since the Confidence Score at the top of this PR is also a stale artifact rather than a verdict on the current code. That Greptile has reviewed twice since without re-emitting a number, which it only prints on a summary comment. Its most recent review, against the current head, independently verified the test-only claim ("the commit touches only the two test files, zero production lines") and signs off: "This hardening commit is a straightforward sign-off." I verified that claim separately too: So the score is lower than the code it is attached to. Worth stating because the usual reason to distrust a low score is under-reviewed code, and this is the reverse. Re-triggering from the Greptile app would produce a current number; neither a re-request nor a push does, and both have been tried, the test-only push being a clean isolation of that variable. CI is green: 74 checks pass, zero fail. #35153 is stacked on this one. |
e719019 to
04a1b26
Compare
|
@greptileai please review the current head 04a1b26 Both open veria findings are fixed in that commit, and both were real; I verified each against the backend rather than arguing the layer below rejects it. Identifier collisions: Toolset grants: the backend unions a toolset's tools with the map ( Eight single-line mutations pin the round (baseline 81 passed; each mutant fails between 1 and 10 tests), the mutation script asserts each edit really applied, and both new lookups are asserted over every collection order. 138 pass across the three component directories, 701 across every directory this PR touches, 0 eslint errors, all six budgets with headroom, build compiles. Note for anyone reading the Confidence Score at the top: it was posted against |
04a1b26 to
f84c7fe
Compare
|
@greptileai please review the current head f84c7fe Second security round. veria marked the two identifier and toolset findings addressed and opened two more; one is fixed here, one is deliberately elsewhere. The Medium was real and was a gap in this PR's own shared-name guard, not a limit of it. The guard inspected the keys an edit supersedes, assuming the key it keeps names one server. When a shared key is a server's only entry it becomes the key an edit writes and The Low is the TeamInfo save filter, which is byte-identical at this branch's merge-base and is removed by #35153, whose base branch is this one. Stacked rather than parallel, so they land in order; doing it here too would leave the child inheriting a conflict for nothing. Nine mutations now, each killed, baseline 85. 705 pass across every directory this PR touches, 0 eslint errors, all six budgets with headroom, build compiles. One existing test changed as a consequence and is stronger for it: the twin's card correctly carries the shared-key notice now, so it asserts both cards say it rather than exactly one. |
f84c7fe to
bdb07af
Compare
|
@greptileai rebased onto current staging, no scope change. Please re-review at head bdb07af; the earlier score is orphaned. |
…ol matrix The MCP tool permission editor was fed the direct server list only, so a server a principal reaches through an access group or a toolset never appeared in the matrix. That single blind spot produced two opposite bugs depending on how a save handler filtered mcp_tool_permissions: filtering by the selected servers deletes an indirect server's allowlist, and because a missing entry means "no restriction from this level", the principal silently gains every tool on it; not filtering leaves a stale entry that keeps a removed access group's server reachable, since a server named under mcp_tool_permissions is entitled on purpose. The editor now resolves the selected access groups and toolsets to their servers and renders them alongside the direct ones, badged with where the grant comes from, so an admin can see and clear an inherited server's tools like any other. Resolution reuses the data the selector already loads: access groups resolve from each server's mcp_access_groups, toolsets from the toolset's own tool list. When that data cannot be loaded the editor says so instead of rendering an empty list, because an absent inherited server reads as "there are none". Servers named only by an mcp_tool_permissions key are listed too, which is what makes a leftover entry visible; the opt-out sentinel still renders nothing, since it short-circuits the backend resolver to zero servers. Opening the editor no longer applies the delete-blocked-by-default allowlist to an inherited server. Writing an entry for one would narrow a grant the admin never touched just by opening the form; direct servers keep that default. Both components also matched on server_id alone, while the backend accepts a server id, name or alias interchangeably. A grant or allowlist written by API or config with a name rendered as a selected server with no tools under it, which reads as "this server has no tools". Matching now covers all three identifiers, and an edit writes back to the key the entry already uses rather than forking a second id-keyed entry. The same mismatch could also put one server under several keys at once, its id and its name for instance. The backend unions every key's list, so reading one key understated what was in force and writing one key left the others granting. The resolver now reports, per server, the key an edit keeps, the equivalent keys it supersedes, and the union those keys allow; the card renders the union and every write goes through one function that writes the kept key and drops the superseded ones. A key that also names a DIFFERENT server, which happens when two servers share a name, is never dropped, because dropping it would strip the neighbouring server's restriction; the card names such a key and says its tools stay allowed until the servers no longer share the name, so an admin is told rather than left to infer it from an edit that bounces back. A third divergence from the backend sat in the same matching. The backend resolves an identifier with exact-id precedence: a string that is a registry server id names that server and stops, and only a string that is no server's id falls back to name and alias, which can name several. Matching all three fields at once meant a server merely named after another server's id joined the matrix as if it had been selected, and because it landed there as a directly selected server it also received the delete-blocked default write on open. Since an mcp_tool_permissions key is itself a grant source, saving then handed out a server nobody granted, with no admin gesture involved. Identifier resolution now mirrors the backend's precedence, and a key is read as this server's only when it resolves back to it, so an entry that belongs to the id's owner is neither read into this server's allowlist nor overwritten by an edit made against it. A toolset grant was also invisible to the tool matrix. The backend unions a toolset's tools with whatever mcp_tool_permissions allows, so a toolset-only grant restricts the server to that toolset's tools; the editor read the map alone, found no entry and rendered every tool on the server as allowed. Deselecting one from that state wrote all the others as a permission entry, and the union turned a revocation into a grant of every tool the toolset never included. The resolved entry now carries the toolset's tools, so the matrix opens on what is actually in force, the delete-blocked default is withheld from a server a toolset restricts, and a write keeps out the tools only the toolset accounts for so a grant that ends with the toolset does not become a standing one. Those tools cannot be revoked from this screen at all, since the backend unions them in; they render allowed and locked and the card says which of them a toolset holds open and where to go to revoke them. That guard originally covered only the keys an edit supersedes, on the assumption that the key it keeps names one server. It does not when a shared key is a server's only entry: it then becomes the key an edit writes, and writing it moves the other server's allowlist too, which is the widening the guard exists to prevent. The key an edit writes is now the first one naming this server and no other, falling back to the server's own id, so a shared key is never written through and an edit against one card cannot reach the server behind the other. Both cards say the shared key holds tools open, since neither can revoke them. No owner's save handler changes here. With the full effective set now available to the editor, the key and team handlers can filter against it instead of guessing, which makes the internal-user surface's unfiltered save redundant Resolves LIT-4963 Resolves LIT-4958
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
@greptileai conflicts with litellm_internal_staging resolved, no scope change. Please re-review the current head |
| toolPermissions: Readonly<Record<string, readonly string[]>>, | ||
| allServers: readonly MCPServer[], | ||
| ): readonly string[] => | ||
| [server.server_id, server.server_name, server.alias].filter( |
There was a problem hiding this comment.
When short MCP tool prefixes are enabled, the backend accepts the server's generated name as a permission key, but this resolver checks only server_id, server_name, and alias. The editor therefore neither displays nor removes tools granted through that key, causing a deselected tool to remain allowed.
There was a problem hiding this comment.
In default mode name is alias, server_name or server_id, all matched here. The short-prefix value is never returned by the server list API, so it needs a backend change first
There was a problem hiding this comment.
That’s correct. In the default server-list response, the resolver covers all identifiers exposed by the API (server_id, server_name, and alias), while the short-prefix value is not available to the client. Supporting that key requires a backend/API change first, so this is not actionable for this PR. I’m withdrawing the comment; the UI can handle it once the backend exposes the generated name.
|
Pre-existing backend semantics; out of scope here. The editor now surfaces such stale entries as a visible card so admins can revoke them |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…rvers Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
@greptileai the TeamInfo save filter now keeps allowlists for the effective server set, with before and after UI captures added |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
@greptileai please review head ffd8dff: standing tool-permission grants survive team saves and partially granted shared keys are split |
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…#35153) * fix(ui): filter team MCP tool allowlists against the effective server set Saving a team filtered mcp_tool_permissions down to the directly selected servers. A server reached through an access group or a toolset is never in that list, so any save dropped its entry, including a save that only changed the team alias. Because the resolver unions tool-permission keys into the entitled server set and treats a missing entry as "no restriction from this level", the team kept the server and lost the tool allowlist on it Filtering on the direct list alone cannot get this right in either direction. Keeping every entry a level did not directly select leaves a removed access group's server reachable through its own stale entry, which breaks revocation. Dropping on deselection alone widens a server that an access group still supplies The save handler now resolves the effective server set with resolveEffectiveMcpServers and keeps an entry only when something other than the entry itself still grants that server: a direct selection, a selected access group, or a selected toolset. Unified access group ids are added when that selection is untouched, since the loaded server list is then still accurate When the server or toolset list cannot be resolved, every entry is kept and the admin is told the allowlists were saved unchanged. Pruning on incomplete knowledge is the direction that silently widens, so it only happens when the editor can show the server became unreachable. A failed lookup and a changed access group selection are separate cases in a tagged union, so the notice names what actually happened instead of describing the intentional one as a failure, and both hooks gate the filter symmetrically so a save fired before toolsets settle cannot resolve against an empty toolset list Resolves LIT-4961 * fix(ui): resolve team MCP grants from access group metadata and refuse unsafe saves Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix(ui): resolve team access group grants from team info when the access group list is role-gated Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix(ui): match every selected access group by id instead of by count Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix(ui): reload team access group grants at save time Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix(ui): keep frontend lint budget within limit Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * test(ui): cover a standing allowlist no group grant covers at load or save Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * refactor(ui): keep MCP grant inputs in named variables for the lint budget Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…s, fix flat view Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
@greptileai please review head 05ccfb2, which folds in #35153 and the three Bugbot fixes |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 05ccfb2. Configure here.
4 participants
TLDR
Problem this solves:
How it solves it:
mcp_toolsets, so a toolset picked at create time is storedUser Flow
Before: an admin saving an unrelated team setting silently removes the tool allowlist of a server the team reaches through an access group
deploy_trackerand the access groupops_readonly, but the tool matrix has one card,deploy_tracker;issue_trackerand itslist_issuesallowlist are not on screenmcp_tool_permissionswith onlydeploy_tracker; theissue_trackerentry is gonedelete_repoonissue_tracker, which the allowlist used to blockAfter: the same save keeps the allowlist, and the matrix shows the inherited server
deploy_trackerandissue_trackerbadged "Via access group: ops_readonly", withlist_issuesthe only tool checked on the seconddeploy_tracker: [create_issue]andissue_tracker: [list_issues]delete_repoforissue_trackerRelevant issues
Linear ticket
Resolves LIT-4963
Resolves LIT-4961
Resolves LIT-4958
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
npx vitest run src/components/team/TeamInfo.test.tsx src/components/mcp_server_management/MCPToolPermissions.test.tsxinui/litellm-dashboard@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
Live proxy on http://localhost:4000 (
python litellm/proxy/proxy_cli.py --config litellm/proxy/dev_config.yaml --detailed_debug --reload --use_v2_migration_resolver), dashboard vianpm run devinui/litellm-dashboard, and a real streamable-HTTP MCP server on http://localhost:8765/mcp exposinglist_issues,create_issueanddelete_repo. Two MCP servers registered against it:deploy_trackerandissue_tracker, the latter in access groupops_readonly. Teammcp-demo-team(id86c0bc2f-6e5f-43b2-a682-88cc13408167) hasdeploy_trackerdirectly,ops_readonlyas an access group, andmcp_tool_permissionsof{"deploy_tracker": ["create_issue"], "issue_tracker": ["list_issues"]}, soissue_trackeris reachable only through the group and its allowlist is keyed by nameShared check, run before each save
Before (bd10977)
Team editor shows the inherited server
deploy_trackerandops_readonly, but the tool matrix below it has a single card,deploy_tracker.issue_trackerand itslist_issuesallowlist are nowhere on screenSaving the team keeps the inherited allowlist
issue_trackerentry is gone, so the team now reaches every tool on that server, includingdelete_repoAfter (94cc52e; the later commits change the save filter, the toolset load guard, the flat view and the create payloads, each covered by unit tests, and this run has not been repeated at 05ccfb2)
Team editor shows the inherited server
issue_trackercarries the badgeVIA ACCESS GROUP: OPS_READONLYand reads1/3 allowedlist_issueson andcreate_issue,delete_repooff, matching the stored name-keyed entrySaving the team keeps the inherited allowlist
The virtual key surface renders the same inherited card. A key granted
ops_readonlyshowsissue_trackervia the group withlist_issueson at http://localhost:3000/api-keys/ (key row, Settings, Edit Settings)Unit test and mutation evidence
Mutation check on the resolver and editor, reverting
MCPToolPermissions.tsxandMCPServerPermissions.tsxto the parent commit and leaving the tests in place; every new test fails and every pre-existing one passes (16 failed | 15 passed). Further single-line mutations, each killed by the tests that pin it: badge always null kills 3; dropping the inherited default-write gate kills 1; removing both failure banners kills 2; id-only permission key kills 4; id-only identifier match kills 6; dropping access-group resolution kills 5; dropping toolset resolution kills 2; dropping tool-permission rows kills 2; removing the opt-out short-circuit kills 1; never superseding an equivalent key kills 3; dropping the shared-name guard kills 1; reading one key instead of the union kills 4; writing one key instead of collapsing kills 1; never appending an entry for a server that had none kills 3; dropping the shared-name notice kills 1; calling every extra key ambiguous kills 2; no exact-id precedence kills 10; permission keys not checked against the catalog kills 4; toolset tools left out of the allowed set kills 4; write copies toolset tools into the entry kills 3; toolset tools not locked kills 3; panel ignores locked tools kills 1; group toggle ignores locked tools kills 1; shared key allowed as the written key kills 6. Collision cases run over both catalog orders so a first-match regression cannot hide behind fixture orderLint and format
Type
🐛 Bug Fix
Changes
effectiveMcpServers.tsis new and holds the resolution as a pure function: it mirrors the backend'sdirect + access_group + tool_perm + toolsetunion, resolves an access group from each server's ownmcp_access_groupsand a toolset from the toolset's tool list (both already loaded byMCPServerSelector, so no new endpoint and no extra request), matches a server by id, name or alias, and returns themcp_tool_permissionskey each server's allowlist actually lives under. Exported so a save handler can filter against the same effective set rather than guessingKnown limit of resolving membership on the client: the warning fires when the server list cannot be fetched, not when it is fetched successfully and a selected group turns out to have no member servers, so a group left empty by a misconfiguration still presents as a group with no servers. A group-membership endpoint would close that gap and is worth considering on its own
MCPToolPermissions.tsxrenders that set. A non-direct server carries a badge naming its source (Via access group: <name>,Via toolset: <name>,Via tool permissions) and a dashed border, and its tools are editable and clearable like any other server's. Reads and writes go through the resolved permission key, so a name-keyed entry round-trips instead of forking into a second id-keyed one. When the server list or toolset list fails to load, a warning renders in place of the missing rows; failing to render an inherited server is safer than an empty list that reads as "there are none". Opening the editor no longer applies the delete-blocked-by-default allowlist to an inherited server, which would have narrowed a grant nobody edited; direct servers keep that default. Theno-mcp-serversopt-out still renders nothing, since it short-circuits the backend resolver to zero serversA server can be named by several
mcp_tool_permissionskeys at once, its id and its name for instance, and the backend unions every key's list. So the resolver reports, per server, the key an edit keeps, the equivalent keys it supersedes, and the union those keys currently allow; the editor renders that union, and every write goes through one function that writes the kept key and drops the superseded ones. After an edit there is exactly one entry per server and it says what the admin sees. The kept key is the one already in the map, preferring the id when both are present, so a name-keyed grant round-trips to its own key rather than being rewritten to an id nobody chose. One exception, because collapsing blindly would repeat the bug in the other direction: a key that also names a DIFFERENT server, which happens when two servers share a name, is never dropped, since dropping it would strip that other server's restriction. Retaining is the direction that cannot widen anyone's access, so it is the one taken, and it has a cost worth naming: the retained key keeps granting its tools to the server being edited, so an admin cannot revoke those tools from this screen. Rather than leave that to be inferred from an edit that bounces back, the card names the ambiguous key and says its tools stay allowed until the servers no longer share the name, which points at the fix an admin can apply. A key that names a server the fetched list does not include cannot be detected at all, which is the residual limit of resolving this on the client. Worth stating plainly about the whole case: because a name-keyed entry is unioned into every server that answers to that name, such a key is ambiguous by construction, so the notice mitigates a symptom and the shape of a real fix is a separate question, tracked on LIT-4982; the notice does not block on itIdentifier resolution follows the backend's precedence rather than comparing three fields at once.
expand_permission_listtreats a string that is a registry server id as naming exactly that server and stops there; only a string that is no server's id falls through to name and alias, which can name several. Matching all three at once meant a server whose name or alias happened to equal another server's id joined the matrix as if it had been selected, and since it landed there as a direct server it also received the delete-blocked default write on open. Anmcp_tool_permissionskey is itself a grant source, so saving then handed out a server nobody granted, with no admin gesture involved. The same precedence decides which keys belong to a server: a key is read into a server's allowlist only when it resolves back to that server, so an entry that belongs to the id's owner is neither counted as this server's grant nor overwritten by an edit made hereA toolset grant is now part of what the card shows. The backend unions a toolset's tools with whatever
mcp_tool_permissionsallows (key_direct_tools | key_toolset_tools), so a toolset-only grant restricts the server to that toolset's tools; reading the map alone found no entry and rendered every tool on the server as allowed. Deselecting one from that state wrote all the others as a permission entry, and the union turned a revocation gesture into a grant of every tool the toolset never included. The resolved entry now carriestoolsetToolsalongsidekeyedTools, so the matrix opens on what is in force; the delete-blocked default is withheld from a server a toolset restricts, for the same reason it is withheld from an inherited one but in the opposite direction, since there it would widen rather than narrow; and a write withholds the tools only the toolset accounts for, so a grant that ends with the toolset does not quietly become a standing one while a tool the entry already held is not dropped either. Those tools cannot be revoked from this screen at all, so they render allowed and disabled and the card names them and points at the toolset. Scope worth stating: this union is key-scoped in the backend (mcp_toolsetsis read only on the key path), so the team surface never had the escalation, only the displayThe shared-name guard covers the key an edit writes, not only the ones it supersedes. It was written on the assumption that the kept key names one server, which fails when a shared key is a server's only entry: it becomes the key an edit writes,
ambiguousKeysis derived from the secondary keys and comes back empty, and allowing a tool on one card writes the shared key, which the backend unions into the other server too. The key an edit writes is now the first that names this server and no other, falling back to the server's own id, so a shared key is never written through and an edit creates an id-keyed entry instead of moving the neighbour's allowlist. Splitting the shared key into per-server entries was the alternative and was not taken, since it edits a server the admin is not looking at, which is the direction this PR refuses everywhere else.allowedToolsstill reports what the shared key contributes, because that is what the backend enforces here, and both cards carry the notice, since neither can revoke itMCPServerPermissions.tsxlooks up the server by id, name or alias, unions the allowlist the same way, and its display name falls back through alias, server name and idThe five call sites (keys create and edit, teams create and edit, agents) pass the access groups and toolsets they already hold
TeamInfo.tsxno longer filters the submittedmcp_tool_permissionsdown to the directly selected server ids, which dropped an inherited server's allowlist on every team save whether or not the admin touched it (LIT-4961). The save handler now resolves the same effective set the editor renders (direct servers, access groups, toolsets, plus the servers behind the team's unified access groups) and keeps every entry that resolves to one of those servers, by id, name or alias. An entry whose key matches no known server is kept as well, since dropping what cannot be resolved is the direction that widens access. If the server list, toolset list or access group list failed to load or is still loading, the save refuses with a toast instead of guessing, because a filter run against an empty inventory would drop everything. Tests cover the indirect grant, alias and name keys, a unified access group, an unknown identifier, and the unresolved-inventory refusalTwo cases sharpen that filter. A server the team reached only through an
mcp_tool_permissionsentry when the editor loaded is a standing grant the backend honours on purpose, sostandingToolPermissionServerIdsresolves the loaded object permission the same way and adds those servers to the granted set; a server that is tool-permission-only after the edit because the admin just removed the group, toolset or direct grant that supplied it is still dropped, which is the stale-entry case LIT-4963 describes. And a key that names several servers (duplicate names) used to be kept whole as long as one match stayed granted, which left a deselected duplicate reachable through the shared key.retainedMcpToolPermissionsnow splits such a key into id-keyed entries for the servers that remain granted, unioning with any entry already under that id, and drops the shared key; a shared key whose matches are all still granted is left as it was. Tests cover both the standing grant on an unrelated save and the splitThis branch also carries the LIT-4961 change from #35153 (commit 389ef42). When the team's unified access group selection is unchanged but the group list did not resolve, the save reloads the team's
access_group_mcp_server_idsinstead of refusing, and only refuses when the selection changed and cannot be resolved.standingToolPermissionServerIdstakes those loaded team group server ids into account as wellThree follow-ups from Bugbot on
MCPToolPermissions.tsxand the create forms. The tool fetch effect now waits foruseMCPToolsetsto settle, and the delete-blocked default is skipped when toolsets failed to load while some are selected, so the default can no longer be written against an empty toolset list and widen a toolset-restricted server. The flat list view read a missing entry as[]while the CRUD panel read it as allow-all, so a single click there replaced an unrestricted grant with a one-tool allowlist; it now starts from every tool checked. Team create and agent create passedselectedToolsetsto the matrix but never wroteobject_permission.mcp_toolsets, so the toolset the matrix locked tools for was gone after create; both payloads now carry itQA runbook
Final Attestation
Note
Medium Risk
Changes permission editing and team/key/agent save payloads for MCP tool allowlists; mistakes could drop or widen grants, though the PR adds extensive tests and refuses saves when resolution data is incomplete.
Overview
Fixes MCP tool permission editing so the UI matches how the backend resolves grants (direct servers, access groups, toolsets, and existing
mcp_tool_permissionskeys).effectiveMcpServersis new shared logic that resolves the effective server set, unions equivalent id/name/alias keys, applies backend-style id-first identifier matching, and centralizes writes so edits collapse onto one key without widening access via shared names or toolset-only tools.MCPToolPermissionsnow lists inherited servers with source badges, shows load warnings when server or toolset data is missing, locks toolset-granted tools, skips inappropriate delete-by-default allowlists for inherited or toolset-restricted servers, and handles the no-MCP-servers opt-out internally. Call sites for teams, keys, and agents pass access groups and toolsets and persistmcp_toolsetson create where applicable.Team save no longer drops allowlists for indirectly granted servers: it retains permissions for the resolved grant set (including standing tool-permission-only grants), splits ambiguous shared-name keys when a duplicate is removed, and blocks saves with tool permissions while MCP inventory is still loading or failed.
MCPServerPermissions(read-only view) resolves display names and tool lists the same way.McpCrudPermissionPanelgainslockedToolsfor non-editable grants.Reviewed by Cursor Bugbot for commit 05ccfb2. Bugbot is set up for automated code reviews on this repo. Configure here.
Link to Devin session: https://app.devin.ai/sessions/a2bbed1daa344692b1bb861a909a45e1
Open in Devin Desktop: https://app.devin.ai/desktop/session/a2bbed1daa344692b1bb861a909a45e1?variant=devin
Requested by: @yassin-berriai