Repository navigation
test(mcp): verify scoped execution and OAuth credential isolation - #41731
Conversation
|
bugbot run |
|
@greptileai please review current head daff22a, focusing on historical regression evidence, actual upstream execution assertions, and legacy test removal scope |
|
|
bugbot run |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@greptileai please review current head f83992f after the full-result health assertion repair, and refresh the current-tip confidence summary |
… rejections Co-Authored-By: bot_apk <apk@cognition.ai>
|
|
Co-Authored-By: bot_apk <apk@cognition.ai>
|
@greptileai @cursor review Please review the current SDK2 fixture changes and new permission, credential-isolation, expiry, and revocation tests. |
|
@greptileai Please review commit a41b60c, especially server-qualified virtual calls, both permission variants, and the revised credential-failure assertions. |
|
@greptileai Please review the latest-main merge at 5b9f3d4 and confirm the scoped MCP regressions still have no actionable findings. |
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 5b9f3d4. Configure here.
TLDR
Problem this solves:
How it solves it:
User Flow
Before: a gateway admin who limits keys to specific MCP servers and stores per-user upstream tokens gets the right answers today, but nothing replays these calls against a live gateway, so a later release can break them unnoticed
urland different aliases, then POST https://litellm-domain/key/generate withobject_permission.mcp_serversnaming only the first serveradd,multiplyandfailtools{"server_id": "<first server id>", "name": "add", "arguments": {"a": 3, "b": 5}}and get 200 with8. The same call naming the second server's id returns 403 "not allowed", and the upstream server receives no requestaddcall returns 200 with that user's own bearer token reaching the upstream{"detail": "Unauthorized"}with awww-authenticatechallenge and nothing reaches the upstream, while the second user's calls still return 200addcall with"guardrails": ["<blocking guardrail>"]in the body and gets 400 with the guardrail's message. The upstream tool never runs, andmultiplystill returns 200 with15After: the same calls return the same answers, and CI now replays them against a live gateway so a release that changes one of them fails before it ships
urland different aliases, then POST https://litellm-domain/key/generate withobject_permission.mcp_serversnaming only the first serveradd,multiplyandfailtools{"server_id": "<first server id>", "name": "add", "arguments": {"a": 3, "b": 5}}and get 200 with8. The same call naming the second server's id returns 403 "not allowed", and the upstream server receives no requestaddcall returns 200 with that user's own bearer token reaching the upstream{"detail": "Unauthorized"}with awww-authenticatechallenge and nothing reaches the upstream, while the second user's calls still return 200addcall with"guardrails": ["<blocking guardrail>"]in the body and gets 400 with the guardrail's message. The upstream tool never runs, andmultiplystill returns 200 with15Relevant issues
Consumes merged SDK2 compatibility work in #41718. Complements principal coverage in #38680 and retains #41909's ownership of real OAuth consent and restart scenarios. No production code or dependency constraints change
Affected release
Linear ticket
Contributes to LIT-4506. The ten-guard inventory retains explicit UI, JWT and stateful limitations. This PR does not claim full ticket closure
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
uv run pytest tests/test_litellm/<your_test_file>.py -v. Leave the suites (make test-unit-*,make test-unit) to CI: it finishes in ~15 minutes where a laptop takes an hour or more@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
This PR only adds and removes tests, so there is no end-user behavior to flip between the two legs. The proof has two parts: the same gateway calls answer the same way at the merge base and at the tip (the author's runs below), and the new tests are non-void, shown by an independent run at the tip plus one disabled production guard per test, each of which makes its test fail (the verification section below)
Author's runs: live gateway, isolated PostgreSQL/Redis, Python 3.12.13 and MCP SDK 2.2.0. The existing integration wrapper grants test entitlement; it does not verify license enforcement. The upstream is an owned SDK arithmetic server, so no model call is needed. Main uses the PR's SDK2-compatible upstream fixtures for this comparison, with unchanged main product source
The matching curl runs below passed on both revisions. Separately, the original PR tip
e0b6baefailed five assertions in a live run; the repaired tests use the actual public error envelopes and search-provided qualified tool namesSetup:
$KEYis a non-master key granted$ALLOWED_IDonly.$VIRTUAL_KEYhas the same grant plus tool search enabled. Both registered servers share one URL and use distinct synthetic bearer tokens.$ALLOWED_TOOLis the qualified name returned by search;$FORBIDDEN_TOOLis the other registered server's qualified name. OAuth users have separate scoped keys and saved synthetic upstream tokensBefore (974e4f1)
Scoped discovery and execution
curl -sS http://127.0.0.1:14506/mcp-rest/tools/list -H "Authorization: Bearer $KEY": 200, exactly the granted server's three tools/mcp-rest/tools/callwith{"server_id":"$ALLOWED_ID","name":"add","arguments":{"a":3,"b":5}}: 200,isError=false, result8, one upstream execution. Substitute$FORBIDDEN_ID: 403, zero upstream requests$VIRTUAL_KEYand{"name":"mcp_tool_search","arguments":{"query":"add","top_k":10}}: only$ALLOWED_TOOL. Call{"name":"mcp_tool_call","arguments":{"tool_name":"$ALLOWED_TOOL","arguments":{"a":3,"b":5}}}: 200, result8. Substitute$FORBIDDEN_TOOL: 403, zero upstream requestsTool failure
/mcp-rest/tools/callwith$KEYand{"server_id":"$ALLOWED_ID","name":"fail","arguments":{}}: 200,isError=true, textError executing tool failRemoved static credentials
/v1/mcp/server/mcp-rest/tools/list?server_id=$ALLOWED_ID: 500 withdetail.error=internal. POST the originaladdcall: 500 withrequires a usable upstream credential. Both produce zero upstream requestsOAuth isolation and revocation
addcall: both return 200 and8; observed upstream bearer tokens differ by user{"detail":"Unauthorized"}, with zero upstream requestsaddcall: 200 and8, using that user's original upstream bearer tokenAfter (5b9f3d4)
Scoped discovery and execution
curl -sS http://127.0.0.1:14506/mcp-rest/tools/list -H "Authorization: Bearer $KEY": 200, exactly the granted server's three tools/mcp-rest/tools/callwith{"server_id":"$ALLOWED_ID","name":"add","arguments":{"a":3,"b":5}}: 200,isError=false, result8, one upstream execution. Substitute$FORBIDDEN_ID: 403, zero upstream requests$VIRTUAL_KEYand{"name":"mcp_tool_search","arguments":{"query":"add","top_k":10}}: only$ALLOWED_TOOL. Call{"name":"mcp_tool_call","arguments":{"tool_name":"$ALLOWED_TOOL","arguments":{"a":3,"b":5}}}: 200, result8. Substitute$FORBIDDEN_TOOL: 403, zero upstream requestsTool failure
/mcp-rest/tools/callwith$KEYand{"server_id":"$ALLOWED_ID","name":"fail","arguments":{}}: 200,isError=true, textError executing tool failRemoved static credentials
/v1/mcp/server/mcp-rest/tools/list?server_id=$ALLOWED_ID: 500 withdetail.error=internal. POST the originaladdcall: 500 withrequires a usable upstream credential. Both produce zero upstream requestsOAuth isolation and revocation
addcall: both return 200 and8; observed upstream bearer tokens differ by user{"detail":"Unauthorized"}, with zero upstream requestsaddcall: 200 and8, using that user's original upstream bearer tokenLocal validation:
BASE_REF=origin/main make lintpassed all lint, formatting, strict-rule, type-discipline, test-quality and basedpyright budget gates. The affected guardrail, credential resolver and discovery-outcome backend files passed 134 tests. The full extensions suite passed all 18 tests in 210.05 seconds at the current tip. Combined integration and standalone SDK2 CLI coverage measured 182/182 changed executable lines (100%) and 46/48 branches (95.8%) inside touched functions. The two uncovered branches are pre-existing empty-body and second-receive paths in the upstream recorder, outside the changed SDK setup and security scenarios. No exclusions or budgets were changed. Completed current-tip Codecov reports 82.13% repository line coverage after all relevant uploads, with CI passing. Its production-only patch contains zero measured changed lines; the separately measured 100% figure covers this test-only diffIndependent verification (reviewer's runs)
Each run boots the proxy from the worktree at the named commit with
--num_workers 1, a fresh Postgres database, its own Redis and the SDK upstream fromtests/integration/_support/upstream.py, all on random ports, mirroring the CircleCIintegration-extensionsjob (Python 3.14.3, pytest 9.0.3, mcp 2.2.0)The extensions suite at a41b60c:
18 passed, exit 0, all seven new node ids included. At 5b9f3d4, after the main merge, with the venv resynced to litellm-enterprise 0.1.69 and litellm-proxy-extras 0.4.100:18 passed in 178.76s, exit 0, andtest_assert_ci_coverage.py34 passed. The diff against the new merge base 974e4f1 is the same nine files, and no main commit since the merge base touchestests/integration,tests/mcp_testsorlitellm/proxy/_experimental/mcp_server, so the mutation results below carry overNon-void check, one production guard disabled per run at a41b60c, worktree clean after each revert. Every mutation made its target test fail
get_allowed_mcp_serversreturns every server[revoke]pre_mcp_callguardrail hook skippedRefreshingTokenStore._is_expiredalways false[expire]tests/mcp_tests/mcp_e2e_upstream_server.pyhas no in-repo runner, so it was booted from the worktree venv on a randomMCP_PORTwithMCP_HOST=127.0.0.1. Under mcp 2.2.0 the movedrun(...)kwargs bind the requested host and port,initializewith alocalhostHost header is accepted (DNS rebinding protection off),tools/listreturnsaddandmultiply, andtools/call add {"a":3,"b":5}returns8. Session handling is unchanged: the server was stateful before this PR as well.tests/test_litellm/test_assert_ci_coverage.py(the other reader ofcontracts.json): 34 passed at the tip/live-pr-risk. Breaking: none, no production code changes. Backward incompatible: none, the upstream fixture keeps its transport, tool set and defaults. Regression risk: none left unexercised. Dependency graph:
tests/integration/_support/mcp.pyis imported bycompatibility/test_persisted_toolsets.pyandobservability/test_guardrail_effects.py(both run in the suite above),contracts.jsonis read bytests/integration/run.py,_support/manifest.py,test_assert_ci_coverage.pyand.circleci/scripts/verify_integration_browser.py(the first three ran here, the last only lists browser contracts),mcp_e2e_upstream_server.pyis imported only by_support/mcp.py(booted live above), and the two deleted files are referenced nowhere. Not verified: nothing skippedCircleCI at 5b9f3d4: pipeline 89891.
build_and_test43/44 green, the one red beingproxy_e2e_anthropic_messages_tests(job 2191862), whose two failing teststest_bedrock_invoke_messages_with_all_beta_headers[bedrock-claude-opus-4.5-bedrock]and[bedrock-converse-claude-sonnet-4.5-bedrock_converse]get Bedrock's 400invalid beta flagon main's last four scheduled pipelines too (89818, 89834, 89873, 89877, the same two tests each time, green at 89785 on 2026-09-18), all before this branch's merge base, tracked in LIT-8149.integration7/8 withintegration-extensions(the suite this PR adds to) green andintegration-costred ontest_case_bills_expected_cost[fireworks_ai-accounts-fireworks-models-deepseek-v4p1-flash-fallback_cache_read_at_input_rate](job 2191821), which main's pipeline 89877 (job 2191123, at 2886b8e, an ancestor of the merge base) fails with the same0.0012456 != 0.0021672; this PR does not touchtests/integration/cost_calculationor the cost map, and the open #41999 repins that case. Neither red is a required check, and both are recorded as Low caveats belowType
✅ Test
Caveats (if any)
No Severe, High or Medium caveat is left: the PR ships tests only, and the four items the earlier body ranked Medium are scope boundaries or pinned shipped behavior rather than gaps in what this diff ships, so they are recorded as Low with the ticket that owns each
Low
internalthat fix(mcp): fail closed on missing upstream credentials #41364 deliberately ships and LIT-7945 release-noted; moving it to a 4xx auth error later updates one assertionproxy_e2e_anthropic_messages_testsis red on main too, tracked in LIT-8149, unrelated to this diffintegration-costfails one fireworks cache-read case on main too, repinned in test(integration): endpoint, breakdown component and failure support in the cost harness #41999, unrelated to this diffmain()has no in-repo test; smoke-booted hereFinal Attestation