fix(mcp): deny MCP access when a named entitlement cannot be read - #35160
Conversation
Greptile SummaryThe PR makes MCP authorization deny access when a principal names an entitlement that cannot be loaded.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/auth/user_api_key_auth_mcp.py | Separates named-but-unreadable entitlements from indeterminate lookups and propagates the former to the central deny paths. |
| tests/test_litellm/proxy/_experimental/mcp_server/auth/test_user_api_key_auth_mcp.py | Adds regression coverage for server and tool authorization across organization, end-user, agent, key-authenticated, and keyless cases. |
Reviews (2): Last reviewed commit: "fix(mcp): deny MCP access when a named e..." | Re-trigger Greptile
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
The The sole failure is Evidence it is base:
Tracked as LIT-4975. Leaving it red rather than pushing a no-op to re-roll, since a re-run will fail the same way until the base is fixed. |
|
Root cause found for the
|
An MCP permission level answers which servers and tools it permits, and a level that answers nothing places no restriction. Key auth was reading a lookup FAULT as that same answer, so the end user, agent and org ceilings quietly disappeared for as long as one lasted, while the keyless gateway-admitted path failed closed on the very same fault. Those levels now separate the two fault classes the user level already did. A principal row that NAMES an object_permission_id whose contents cannot be read is a known entitlement with unknown contents, so it denies. A lookup that fails before we can tell whether the principal is entitled at all still places no ceiling, that being the state which existed before the level did; denying there would refuse MCP to the majority of callers, who have no such entitlement configured. The keyless path is unchanged. Resolves LIT-4960
705af58 to
e00bca4
Compare
|
|
473f43d
into
litellm_internal_staging
|
Rebased onto current staging and squashed to one commit; head is now One behavior-preserving change on top of the approved diff: the two deny paths now branch inside the resolvers' existing Mapped directory is 2802 passing against the new base's 2792, with the same 23 pre-existing failures on both. @greptileai please review the current head e00bca4 |
3 participants
TLDR
Problem this solves:
How it solves it:
Relevant issues
Linear ticket
Resolves LIT-4960
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
Live proxy on port 4960 against its own postgres, with a real upstream MCP server (deepwiki, no mocks). Two virtual keys, each in a team that grants the server; the first key's organization carries an MCP entitlement, the second key's organization carries none. The entitlement is then made unreadable, which is the fault the fix is about, by corrupting the stored permission row so it no longer deserializes; the link from the organization to it stays intact, so the gateway still knows the entitlement exists
Setup (captured at 551e5d0, both builds share it)
Before, at 551e5d0
and the proxy log says what happened
After, at 705af58, same database, same request
Negative control, at 705af58, the key whose organization has no entitlement at all is untouched
Second negative control, at 705af58, restoring the entitlement to a readable state serves the first key again, so this is not a blanket deny
Type
🐛 Bug Fix
Changes
An MCP permission level answers which servers and tools it permits, and a level that answers nothing places no restriction. For a key-authenticated caller the end user, agent and organization levels were reading a lookup fault as that same answer, so a ceiling an operator had configured quietly disappeared for as long as the fault lasted and the caller kept the wider scope its key grants. The keyless gateway-admitted path already failed closed at all of the same points, so the two caller shapes gave opposite answers to one fault
Those three levels now separate the two fault classes, which is what the per-user level added in #35146 already does; that level therefore needs no change here. A principal row that names an
object_permission_idwhose contents cannot be read is a known entitlement with unknown contents, so it raisesUnloadableEntitlementErrorand the resolver denies, on the servers axis and the tools axis alike. A lookup that fails before we can tell whether the principal is entitled at all still places no ceiling, because that is the state that existed before the level did, and denying there would refuse MCP to the majority of callers, who have no such entitlement configured, during any cold-cache or database fault. Blanket fail-closed was considered and rejected on exactly that ground: it trades a global availability regression for a partial fixOne helper,
_load_named_object_permission, is the single place that fault is minted, so the three levels cannot drift on what counts as evidence of an entitlement. The contract, including why key auth and the keyless path diverge, is written into theMCPRequestHandlerclass docstring and restated per level, since it was undocumented product behavior until nowThe keyless path is unchanged; it denied on any fault before and still does. No existing test changed behavior: rebased onto
3c2264cfac, the mapped directory reports 2802 passing against that base's 2792, with the same 23 pre-existing failures on both, so the 10 added tests are the whole delta. Each new deny is mutation-checked: reverting this file to the base fails the five deny tests and leaves the five no-ceiling controls green, and dropping either the tool-axis deny or the org re-raise fails a test on its ownThe two new deny paths reuse the resolvers' existing
exceptblocks rather than adding clauses of their own, so the type-discipline budget sees no new mutable-collection construction; the resolved answers and the log lines are the same either wayFinal Attestation