[Fix] MCP broker OAuth endpoint access controls - #26142
Conversation
There was a problem hiding this comment.
High: Missing authentication on /register endpoint
This PR correctly adds user_api_key_auth to the /server/oauth/{server_id}/authorize and /server/oauth/{server_id}/token broker endpoints, and adds scheme validation on redirect_uri. However, it leaves the /server/oauth/{server_id}/register endpoint unauthenticated, which is inconsistent with the other two endpoints in the same OAuth flow and allows unauthenticated callers to trigger outbound requests to MCP server registration URLs.
Greptile SummaryThis PR hardens the MCP broker OAuth endpoints by requiring a valid LiteLLM bearer token on
Confidence Score: 4/5Safe to merge after confirming that requiring LiteLLM API-key auth on the OAuth broker /authorize and /token endpoints is intentional and will not break existing MCP client flows. One P1 backwards-compatibility concern remains: locking the OAuth broker endpoints behind user_api_key_auth may break MCP clients that reach these endpoints during a standard OAuth 2.0 authorization-code flow without a LiteLLM API key. The security intent is clear and the other changes (IP propagation, redirect_uri validation) are clean, but the P1 design question warrants author confirmation before merge. litellm/proxy/management_endpoints/mcp_management_endpoints.py — the auth dependency placement on the OAuth broker endpoints needs clarification.
|
| Filename | Overview |
|---|---|
| litellm/proxy/management_endpoints/mcp_management_endpoints.py | Adds user_api_key_auth to /authorize, /token, and /register broker endpoints. Each endpoint now carries a duplicate Depends (route-level + parameter), and locking OAuth flow endpoints behind LiteLLM API-key auth may break existing MCP clients. |
| litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py | Adds redirect_uri scheme validation (http/https only) in authorize_with_server, blocking open-redirect via non-HTTP schemes before the URI is embedded in the encrypted state. |
| tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py | Four test stubs updated to pass request=request to _get_cached_temporary_mcp_server_or_404, correctly reflecting the new signature; no coverage is weakened. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[MCP Client Request] --> B[user_api_key_auth - route dep]
B --> C[user_api_key_auth - param dep\nnote: runs twice per request]
C --> D[_get_cached_temporary_mcp_server_or_404]
D --> E{Temp session cache hit?}
E -- Yes --> G[Return MCPServer]
E -- No --> F[MCPServerManager fallback\nget by ID or name\nnow passes caller IP from request]
F --> G
G --> H{Endpoint?}
H -- /authorize --> I[Validate redirect_uri scheme\nhttp or https only\nthen proxy to upstream]
H -- /token --> J[Exchange auth code\nwith upstream OAuth server]
H -- /register --> K[Register OAuth application\nwith upstream OAuth server]
Reviews (2): Last reviewed commit: "fix: add access control to register endp..." | Re-trigger Greptile
There was a problem hiding this comment.
High: Missing authentication on /register endpoint
This PR adds dependencies=[Depends(user_api_key_auth)] to both /authorize and /token OAuth endpoints, but the /server/oauth/{server_id}/register endpoint was not updated. An unauthenticated caller can hit this endpoint to trigger server-side HTTP requests to the MCP server's registration URL.
| @@ -1443,7 +1453,7 @@ async def mcp_token( | |||
| include_in_schema=False, | |||
There was a problem hiding this comment.
High: Missing authentication on register endpoint
The /authorize and /token endpoints both received dependencies=[Depends(user_api_key_auth)] in this PR, but /register was not updated. An unauthenticated caller can hit this endpoint to trigger server-side HTTP POST requests to whatever registration_url is configured on the MCP server.
| include_in_schema=False, | |
| include_in_schema=False, | |
| dependencies=[Depends(user_api_key_auth)], |
| @router.post( | ||
| "/server/oauth/{server_id}/token", | ||
| include_in_schema=False, | ||
| dependencies=[Depends(user_api_key_auth)], | ||
| ) | ||
| async def mcp_token( | ||
| request: Request, | ||
| server_id: str, | ||
| user_api_key_dict: UserAPIKeyAuth = Depends(user_api_key_auth), | ||
| grant_type: str = Form(...), |
There was a problem hiding this comment.
Adding auth to the OAuth token endpoint may break the standard MCP OAuth flow
The /token endpoint (and similarly /authorize) is an OAuth 2.0 broker endpoint intended to be called by MCP clients (e.g. Claude Desktop) as part of the RFC 6749 authorization code exchange. MCP clients performing this flow will not possess a LiteLLM bearer token — they carry OAuth client credentials instead. Requiring user_api_key_auth here means any MCP client that reaches the token exchange step without a pre-existing LiteLLM API key will receive a 401 and fail silently.
This is a backwards-incompatible change per the team's policy: existing MCP OAuth integrations will break without any feature flag or migration path. If the intent is for these endpoints to be broker flows invoked only by already-authenticated LiteLLM users, that should be explicitly documented; otherwise consider whether the token and authorize endpoints should remain unauthenticated (protected only by the encrypted state / PKCE parameters) to preserve RFC 6749 compliance.
Rule Used: What: avoid backwards-incompatible changes without... (source)
…oint_auth [Fix] MCP broker OAuth endpoint access controls
Summary
mcp_authorizeandmcp_tokenmanagement endpoints now require a valid bearer token, matching the pattern used by every other management endpoint in this file.redirect_uriis validated for a permitted scheme (http/https) before it is embedded in the encrypted state object, blocking non-HTTP schemes._get_cached_temporary_mcp_server_or_404now accepts the request and extracts the client IP when falling back to the server name lookup, so IP-based access controls are consistently applied across all code paths._get_cached_temporary_mcp_server_or_404(server_id, request=request)signature.Testing
uv run pytest tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py— 93 passed/authorizeand/tokenreturn 401;javascript:anddata:redirect URIs return 400; authenticated requests with a valid server ID proceed normally.Type
🐛 Bug Fix
✅ Test