fix(mcp): resolve admin OAuth sessions to the same server set the connect page shows - #37900
Conversation
|
@greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
Score: 4/5 Strengths:
Concerns:
None of these block merge; the fix is correctly motivated and the implementation is sound. |
Greptile SummaryThe PR aligns OAuth-admitted admin sessions with the MCP server set visible to unscoped admin keys while preserving explicit scopes, entitlement ceilings, and per-server session envelopes
Confidence Score: 5/5The PR appears safe to merge No blocking failure remains; the shared predicate preserves explicit and entitlement scopes, and both normal and fallback server-resolution paths retain the final session-resource intersection
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/auth/user_api_key_auth_mcp.py | Adds the shared admin-view predicate and applies it consistently to admitted-session tool reachability |
| litellm/proxy/_experimental/mcp_server/mcp_server_manager.py | Seeds unscoped admin access into the server union while retaining the final session-resource intersection |
| tests/test_litellm/proxy/_experimental/mcp_server/auth/test_user_api_key_auth_mcp.py | Covers admin session parity, explicit and unresolved ceilings, the non-admin negative control, and tool invocation reachability |
| tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py | Verifies that a sealed admin session remains restricted to its resource server |
Reviews (2): Last reviewed commit: "fix(mcp): bind admitted admin rows throu..." | Re-trigger Greptile
| ceiling (including an unresolved one) binds the human whatever their role, so both disqualify. | ||
| The one owner of this predicate: the server-axis registry shortcut and the tools-axis admin | ||
| channel both call it, so the two axes cannot disagree.""" | ||
| if user_api_key_auth is None or not user_api_key_has_admin_view(user_api_key_auth): |
There was a problem hiding this comment.
High: Read-only admins can execute MCP tools
user_api_key_has_admin_view includes PROXY_ADMIN_VIEW_ONLY, but this predicate is also used by _resolve_admitted_subject_tools as an allow-all execution channel. A read-only admin can therefore authorize an aggregate OAuth session and invoke tools on every MCP server without a server grant, despite that role being denied inference and state-changing operations elsewhere. Restrict the admitted-session shortcut to PROXY_ADMIN; if viewer parity is required for discovery, keep that as a separate listing-only predicate that is not used by the tool authorization path.
There was a problem hiding this comment.
Probed live: a view-only admin key already passes tools/call and inference authz today, so sessions mirror shipped key semantics. Happy to file a both-shapes follow-up
There was a problem hiding this comment.
Thanks for the detail. I can't automatically confirm this is safe to dismiss, so I'm leaving the thread open for a maintainer to make the call.
PR overviewThis pull request changes MCP admin OAuth session resolution so authenticated sessions target the same set of MCP servers displayed on the connect page. One significant authorization issue remains open: read-only administrators can use aggregate OAuth sessions to execute tools across MCP servers without individual server grants. This creates an execution authorization bypass for a role intended to have view-only access, and no reported issues have yet been addressed. Open issues (1)
Fixed/addressed: 0 · PR risk: 8/10 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…ot the credential scope clause
|
@greptileai negative-control test and docstring fix pushed, and the ceiling probe caches per user so warm requests read no rows. Can you reply with a score and why? |
|
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 179e581. Configure here.
|
QA'd this end to end on a live proxy (:4000, Postgres, two bearer-protected stub MCP servers with allow_all_keys false and zero grants), at HEAD 179e581. As master admin the connect page lists both ungranted servers as connectable, and a full DCR walk (register, authorize, token, initialize, tools/list, tools/call) serves both servers' tools with no object permission grant. The stub upstream saw the gateway-injected stored bearer. Negative control and scope checksInternal user: connect picker is empty, bridge authorize redirects with error=access_denied and no code, and an aggregate session resolves zero tools with tool calls denied. 179e581 regression: with the admin's ObjectPermission row set to the DB-default mcp_servers=[], a fresh session still resolves the full registry (connect page and a fresh DCR token both show both servers). Explicit scope: after POST /user/update with mcp_servers=[cust_stub2 id], a fresh session narrows to only cust_stub2 in both the connect page and tools/list. Note on the runbook's step 5 wording: "authorize redirects with error=access_denied" applies to the per-server bridge authorize path ( Unit tests: 809 passed, 0 failed across the two changed test files. Tested by Devin: session |
TLDR
Problem this solves:
not is_admitted_subjectinMCPServerManager.get_allowed_mcp_servers). The restriction is vacuous against an admin, who can self-grant or paste an admin virtual key and get the registry anyway, so it only produces the mismatchHow it solves it:
MCPRequestHandler.admin_view_unscopedreturns True for an admin-view role with no explicitobject_permission.mcp_serversscope and no entitlement ceiling, and both the server-axis registry resolution and the tools-axis open channel call it, for keys and admitted session subjects alike{"status": "auth_required"}in the tools/list_metaserver outcomes, so nothing on that axis needed codeThis supersedes the design direction of open PR #34585, which locks UI sessions to the admitted-subject resolver with no admin registry path
Relevant issues
Linear ticket
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
Rig: proxy from this branch on localhost:4612 with real Postgres, one stub upstream MCP server on :8933 that 401s without its bearer and counts authenticated requests, and a stub IdP on :8934. Two servers registered with
allow_all_keys: falseand zero grants:cust_bridge(oauth_delegate + dcr_bridge) andcust_oauth2(oauth2, authorization_code). The change is on the auth path, so no LLM call is involved; the upstream MCP server is a local stub per the established precedent for request-path changes (#30277, #34029)Before (base commit, same commands): the admin's dashboard
GET /v1/mcp/serverlists both servers,?connected_app_view=truemarks bothconnected_app_reachable: false(empty connect picker), and the DCR walk dies at authorizeAfter (this branch), same admin, fresh session:
Non-admin negative control (fresh internal_user session, same walk) still denies loudly:
Explicit scope still wins: after
POST /user/updatewithobject_permission.mcp_servers = [cust_oauth2_id], a fresh admin session resolves only that server. And in-scope servers with no upstream credential surface in_metainstead of vanishing:Type
🐛 Bug Fix
Changes
litellm/proxy/_experimental/mcp_server/auth/user_api_key_auth_mcp.py: newMCPRequestHandler.admin_view_unscopedpredicate (the one owner of the admin-view question), and_resolve_admitted_subject_toolstreats it as an open channel riding the user's own sourcelitellm/proxy/_experimental/mcp_server/mcp_server_manager.py:get_allowed_mcp_serversreplaces the inline four-clause admin arm and itsnot is_admitted_subjectcarve-out with the shared predicate, seeding the registry into the union so the session resource scope bounds every returnTests: the old pin
test_admitted_admin_is_scoped_to_grants_not_full_registryis rewritten to the new invariant (parametrized over PROXY_ADMIN and PROXY_ADMIN_VIEW_ONLY, asserting session equals key view), plus new pins for explicit key scope, unresolved entitlement ceiling, the tools-axis channel with a non-admin negative, the registry seed bounded by a sealed session resource scope, the DB-default empty mcp_servers row still resolving the registry, and a non-admin admitted subject never getting the registry. Each fails on the commit before the code it pinsSecond commit (179e581, from review): the credential scope clause in
admin_view_unscopedapplies only to non-admitted principals, so an admitted admin whose shared permission row exists with the DB-default empty mcp_servers list still resolves the registry, verified live on the rig (connect view flips to reachable True with the [] row in place)QA runbook
store_model_in_db: true, master keysk-1234allow_all_keys: falseand no grants (any oauth_delegate + dcr_bridge server works; a stub works too)/ui/connect?connect_flow=1or via Claude Code pointing athttps://<proxy>/<server>/mcp), and confirm the server now shows as connectableinternal_user, sign in as them, repeat step 4, and confirm the authorize step still redirects witherror=access_deniedobject_permission.mcp_serverslist and confirm a fresh session resolves only those serversFinal Attestation