fix(mcp): delegate PKCE bypass for internal MCP servers - #27977
Conversation
Remove available_on_public_internet gating from delegate-auth-to-upstream paths so oauth2 + delegate_auth_to_upstream interactive servers behave the same when marked internal. Keeps M2M exclusion. Updates tests.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR expands the anonymous PKCE bypass for OAuth2 delegate-to-upstream MCP servers to also cover
Confidence Score: 4/5The change deliberately widens anonymous access to internal MCP servers; correctness depends entirely on upstream IdP and network controls being properly enforced. Three auth guards that previously blocked internal-only servers from the anonymous PKCE path are removed simultaneously. Any operator who previously combined available_on_public_internet: false with delegate_auth_to_upstream: true as a LiteLLM-level authentication gate for internal servers will have that gate silently removed after upgrade, with no opt-in or opt-out flag. The M2M exclusion and the authorization_code-only bypass on /token are preserved correctly, and the new tests accurately reflect the intended post-change behaviour. user_api_key_auth_mcp.py and mcp_management_endpoints.py contain the removed auth guards and warrant the most scrutiny; mcp_server_manager.py removes the third guard in get_allowed_mcp_servers.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/auth/user_api_key_auth_mcp.py | Removes the available_on_public_internet guard from _target_servers_delegate_auth_to_upstream, allowing internal-only MCP servers to bypass LiteLLM auth for tool-call requests when delegate_auth_to_upstream is True |
| litellm/proxy/_experimental/mcp_server/mcp_server_manager.py | Removes available_on_public_internet guard from get_allowed_mcp_servers anonymous allow-list, and adds _warn_internal_delegate_pkce_if_applicable warning log emitted at config and DB load time for affected servers |
| litellm/proxy/management_endpoints/mcp_management_endpoints.py | Removes available_on_public_internet guard from _mcp_oauth_user_api_key_auth, allowing internal delegate servers to bypass LiteLLM auth for /authorize and authorization_code /token PKCE flows |
| tests/test_litellm/proxy/_experimental/mcp_server/auth/test_user_api_key_auth_mcp.py | Tests inverted to assert bypass (no 401) for internal delegate servers; test_get_allowed_servers_includes_internal_delegate now expects internal servers in the allow-list |
| tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py | New TestInternalDelegatePkceWarningLog class adds good coverage for the warning log emitted at load time; M2M exclusion and public-server non-emission cases are tested |
| tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py | Test renamed and assertions inverted to confirm bypass; expected_auth variable is still constructed but never referenced in any assertion (dead code noted in previous review comment) |
| ui/litellm-dashboard/src/components/mcp_tools/MCPPermissionManagement.tsx | Adds a dashboard Alert warning when oauth2 + delegate_auth_to_upstream + available_on_public_internet=false are all set; uses antd Alert per style guide |
| CLAUDE.md | Documents the new available_on_public_internet=false + delegate_auth_to_upstream=true semantics in the MCP OAuth section |
Reviews (2): Last reviewed commit: "fix(mcp): dedupe load_servers_from_confi..." | Re-trigger Greptile
|
@greptile this is an intended design, will it be okay if we raise a warning on the UI if user tries this combination? |
|
A UI warning is a reasonable compromise for an intentional design. A few thoughts on making it effective:
With those in place, the PR looks acceptable from a security standpoint — the M2M exclusion remains intact and the bypass is scoped to the PKCE interactive flow only. |
Log verbose_logger.warning when loading oauth2 interactive servers with available_on_public_internet=false and delegate_auth_to_upstream=true (config + DB). Dashboard Alert for the same combo. CLAUDE note for operators. Tests for log and M2M skip.
|
@greptile re review |
Removes accidental duplicate alias/mcp_aliases and get_server_prefix logic (fixes PLR0915 and avoids resetting alias after mapping).
mateo-berri
left a comment
There was a problem hiding this comment.
LGTM
nonblocking nit: tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py:1707 constructs expected_auth and feeds it to the _user_api_key_auth_builder mock, but then asserts auth_builder_mock.assert_not_called() and only isinstance(result, UserAPIKeyAuth). The mock return value is never consumed. Other sibling tests (1608/1644/1693) do assert result is expected_auth. Either add the same identity assert here or drop expected_auth + patched return value
…legate_pkce fix(mcp): delegate PKCE bypass for internal MCP servers
Removes
available_on_public_internetchecks from delegate-auth-to-upstream flows so internal (available_on_public_internet: false) oauth2 interactive servers get the same anonymous PKCE bypass as public servers.Touched paths:
user_api_key_auth_mcp._target_servers_delegate_auth_to_upstreamMCPServerManager.get_allowed_mcp_serversanonymous delegate allow-list_mcp_oauth_user_api_key_authfor browser /authorize (and related) without a LiteLLM sessionM2M (
client_credentials) exclusion unchanged.Tests:


TestMCPDelegateAuthToUpstream::test_delegate_bypass_for_internal_server,test_get_allowed_servers_includes_internal_delegate,TestTemporaryMCPSessionEndpoints::test_mcp_oauth_user_api_key_auth_internal_delegate_bypasses.