-
-
Notifications
You must be signed in to change notification settings - Fork 11.7k
fix(mcp): remove auth gate from OAuth broker authorize and token endpoints #27106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Sameerlite
wants to merge
12
commits into
litellm_internal_staging
from
litellm_fix_mcp_oauth_broker_auth
Closed
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
2a1d166
fix(mcp): remove auth gate from OAuth broker authorize and token endp…
Sameerlite c846418
refactor(mcp): move OAuth tests into tests/mcp_tests so they run unde…
Sameerlite 374c415
revert(ci): remove redundant MCP OAuth CI step, tests/mcp_tests is al…
Sameerlite 5bd5ba6
test(mcp): add HTTP-layer regression tests for management broker auth…
Sameerlite 8a7dda5
fix(mcp): restrict unauthenticated OAuth broker bypass to temp-sessio…
Sameerlite a969c97
Fix MCP OAuth test fixture configuration
cursoragent 38c7cc3
fix(mcp): optional broker auth + temp-session authorize test
Sameerlite 167e6c3
Merge pull request #27164 from BerriAI/litellm_internal_staging
Sameerlite b854823
Fix failing tests
Sameerlite 5881afc
Fix optional MCP OAuth broker auth headers
cursoragent 23fc23e
fix: check all auth headers in _try_resolve_mcp_oauth_broker_user
cursoragent 8eac609
Merge pull request #27179 from BerriAI/litellm_internal_staging
Sameerlite File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -147,6 +147,7 @@ def validate_tool_name(name: str) -> _ToolNameValidationResult: # type: ignore[ | |
| MCPUserCredentialResponse, | ||
| NewMCPServerRequest, | ||
| RejectMCPServerRequest, | ||
| SpecialHeaders, | ||
| SpecialMCPServerName, | ||
| UpdateMCPServerRequest, | ||
| UserAPIKeyAuth, | ||
|
|
@@ -1492,9 +1493,57 @@ async def add_session_mcp_server( | |
|
|
||
| return _redact_mcp_credentials(temp_record) | ||
|
|
||
| async def _try_resolve_mcp_oauth_broker_user( | ||
| request: Request, | ||
| ) -> Optional[UserAPIKeyAuth]: | ||
| """ | ||
| Optional proxy credentials for ``/authorize`` and ``/token``. | ||
|
|
||
| When absent, unauthenticated access is still allowed for **temp-cache** | ||
| servers only (browser OAuth). When present, global-registry access | ||
| follows admin / allowlist rules via ``_get_cached_temporary_mcp_server_or_404``. | ||
|
|
||
| Only non-empty **string** values in any recognised auth header trigger a | ||
| full auth pipeline import (tests and mocks may attach MagicMock headers). | ||
| The recognised headers match those checked by | ||
| ``user_api_key_auth_from_request_headers``: ``Authorization``, | ||
| ``API-Key``, ``x-api-key``, ``x-goog-api-key``, | ||
| ``Ocp-Apim-Subscription-Key``, and ``x-litellm-api-key``. | ||
| """ | ||
| try: | ||
| headers = request.headers | ||
| except Exception: | ||
| return None | ||
| raw: object = None | ||
| for header_name in ( | ||
| SpecialHeaders.openai_authorization.value, | ||
| SpecialHeaders.azure_authorization.value, | ||
| SpecialHeaders.anthropic_authorization.value, | ||
| SpecialHeaders.google_ai_studio_authorization.value, | ||
| SpecialHeaders.azure_apim_authorization.value, | ||
| SpecialHeaders.custom_litellm_api_key.value, | ||
| ): | ||
| for key in (header_name, header_name.lower()): | ||
| try: | ||
| candidate = headers.get(key) | ||
| except Exception: | ||
| candidate = None | ||
| if isinstance(candidate, str) and candidate.strip(): | ||
| raw = candidate | ||
| break | ||
| if isinstance(raw, str): | ||
| break | ||
| if not isinstance(raw, str) or not raw.strip(): | ||
| return None | ||
| from litellm.proxy.auth.user_api_key_auth import ( | ||
| user_api_key_auth_from_request_headers, | ||
| ) | ||
|
|
||
| return await user_api_key_auth_from_request_headers(request) | ||
|
|
||
| async def _get_cached_temporary_mcp_server_or_404( | ||
| server_id: str, | ||
| user_api_key_dict: UserAPIKeyAuth, | ||
| user_api_key_dict: Optional[UserAPIKeyAuth] = None, | ||
| request: Optional[Request] = None, | ||
| ) -> MCPServer: | ||
| server = await get_cached_temporary_mcp_server(server_id) | ||
|
|
@@ -1516,12 +1565,37 @@ async def _get_cached_temporary_mcp_server_or_404( | |
| detail={"error": f"MCP server {server_id} not found"}, | ||
| ) | ||
|
|
||
| # Per-server access policy mirrors `fetch_mcp_server`: admin-view | ||
| # callers are unrestricted; non-admins must have the server in their | ||
| # allowed-servers set. Temporary cached servers come from the | ||
| # admin-only `/server/oauth/session` setup flow and are not exposed | ||
| # to non-admins. | ||
| if not _user_has_admin_view(user_api_key_dict): | ||
| # Access-control for the OAuth broker endpoints. | ||
| # | ||
| # Unauthenticated callers (browser-initiated OAuth, no API key): | ||
| # - Temp-cache servers: allowed. These are created by admins via the | ||
| # admin-only /server/oauth/session endpoint specifically to drive | ||
| # this browser flow. The LiteLLM UI always creates a temp session | ||
| # before calling /authorize, so all legitimate browser flows use a | ||
| # temp server_id. | ||
| # - Global-registry servers: rejected (403). Allowing unauthenticated | ||
| # access to global-registry servers would let any caller invoke the | ||
| # proxy's OAuth broker with the server's stored client_secret, while | ||
| # authenticated non-admins without allowlist access receive 403 — | ||
| # an unintended privilege inversion. | ||
| # | ||
| # Authenticated callers: | ||
| # - Admins: unrestricted. | ||
| # - Non-admins: temp servers are always denied (temp sessions are | ||
| # admin-internal); global servers require allowlist membership. | ||
| if user_api_key_dict is None: | ||
| if not resolved_from_temp_cache: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_403_FORBIDDEN, | ||
| detail={ | ||
| "error": ( | ||
| "Unauthenticated access to global-registry MCP server " | ||
| f"{server_id} is not permitted. " | ||
| "Pass a valid API key or use a session-scoped server ID." | ||
| ) | ||
| }, | ||
| ) | ||
| elif not _user_has_admin_view(user_api_key_dict): | ||
|
Sameerlite marked this conversation as resolved.
|
||
| if resolved_from_temp_cache: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_403_FORBIDDEN, | ||
|
|
@@ -1542,12 +1616,10 @@ async def _get_cached_temporary_mcp_server_or_404( | |
| @router.get( | ||
| "/server/oauth/{server_id}/authorize", | ||
| include_in_schema=False, | ||
| dependencies=[Depends(user_api_key_auth)], | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like an insecure workaround, will message on Slack |
||
| ) | ||
| async def mcp_authorize( | ||
| request: Request, | ||
| server_id: str, | ||
| user_api_key_dict: UserAPIKeyAuth = Depends(user_api_key_auth), | ||
| client_id: Optional[str] = None, | ||
| redirect_uri: str = Query(...), | ||
| state: str = "", | ||
|
|
@@ -1556,8 +1628,9 @@ async def mcp_authorize( | |
| response_type: Optional[str] = None, | ||
| scope: Optional[str] = None, | ||
| ): | ||
| user_api_key_dict = await _try_resolve_mcp_oauth_broker_user(request) | ||
| mcp_server = await _get_cached_temporary_mcp_server_or_404( | ||
| server_id, user_api_key_dict, request=request | ||
| server_id, user_api_key_dict=user_api_key_dict, request=request | ||
| ) | ||
| # Use the server's stored client_id when the caller doesn't supply one | ||
| resolved_client_id = mcp_server.client_id or client_id or "" | ||
|
|
@@ -1587,12 +1660,10 @@ async def mcp_authorize( | |
| @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(...), | ||
| code: Optional[str] = Form(None), | ||
| redirect_uri: Optional[str] = Form(None), | ||
|
|
@@ -1602,8 +1673,9 @@ async def mcp_token( | |
| refresh_token: Optional[str] = Form(None), | ||
| scope: Optional[str] = Form(None), | ||
| ): | ||
| user_api_key_dict = await _try_resolve_mcp_oauth_broker_user(request) | ||
| mcp_server = await _get_cached_temporary_mcp_server_or_404( | ||
| server_id, user_api_key_dict, request=request | ||
| server_id, user_api_key_dict=user_api_key_dict, request=request | ||
| ) | ||
| resolved_client_id = mcp_server.client_id or client_id or "" | ||
| if not resolved_client_id: | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.