diff --git a/.github/workflows/test-litellm-ui-lint.yml b/.github/workflows/test-litellm-ui-lint.yml index 804894b1e50..5173eb6da35 100644 --- a/.github/workflows/test-litellm-ui-lint.yml +++ b/.github/workflows/test-litellm-ui-lint.yml @@ -29,7 +29,15 @@ jobs: id: changed env: BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} run: | + # base.sha is the base branch tip from when the PR was opened, while + # actions/checkout leaves HEAD on a merge of the PR into the *current* + # base tip. "$BASE_SHA"...HEAD therefore spans every base-branch commit + # landed since, so a PR that touches no UI file still gets linted + # against hundreds of other people's files. Diff the PR head against its + # own merge base instead, which is exactly what this PR changed. + merge_base=$(git merge-base "$BASE_SHA" "$HEAD_SHA") : > "$RUNNER_TEMP/prettier_files.txt" : > "$RUNNER_TEMP/eslint_files.txt" while IFS= read -r f; do @@ -41,7 +49,7 @@ jobs: *.json | *.css | *.scss | *.md | *.mdx | *.yml | *.yaml | *.html) printf '%s\n' "$f" >> "$RUNNER_TEMP/prettier_files.txt" ;; esac - done < <(git diff --name-only --diff-filter=ACMR --relative "$BASE_SHA"...HEAD -- .) + done < <(git diff --name-only --diff-filter=ACMR --relative "$merge_base" "$HEAD_SHA" -- .) if [ -s "$RUNNER_TEMP/prettier_files.txt" ] || [ -s "$RUNNER_TEMP/eslint_files.txt" ]; then echo "has_files=true" >> "$GITHUB_OUTPUT" else diff --git a/litellm/proxy/_experimental/mcp_server/server.py b/litellm/proxy/_experimental/mcp_server/server.py index e6bc6270771..9a1fffd5a67 100644 --- a/litellm/proxy/_experimental/mcp_server/server.py +++ b/litellm/proxy/_experimental/mcp_server/server.py @@ -2727,7 +2727,7 @@ if MCP_AVAILABLE: ): raise HTTPException( status_code=403, - detail=f"User not allowed to call this tool. Allowed MCP servers: {allowed_mcp_servers}", + detail="User not allowed to call this tool.", ) standard_logging_mcp_tool_call: StandardLoggingMCPToolCall = _get_standard_logging_mcp_tool_call( diff --git a/litellm/types/mcp_server/mcp_server_manager.py b/litellm/types/mcp_server/mcp_server_manager.py index b0af22e7c3f..83416f790e6 100644 --- a/litellm/types/mcp_server/mcp_server_manager.py +++ b/litellm/types/mcp_server/mcp_server_manager.py @@ -163,6 +163,15 @@ class MCPServer(BaseModel): allow_elicitation: bool = False model_config = ConfigDict(arbitrary_types_allowed=True) + def __repr__(self) -> str: + return ( + f"MCPServer(server_id={self.server_id!r}, name={self.name!r}, " + f"transport={self.transport!r}, auth_type={self.auth_type!r})" + ) + + def __str__(self) -> str: + return self.__repr__() + @property def has_client_credentials(self) -> bool: """True if this server should use the OAuth2 client_credentials (M2M) flow. diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server.py index dff1f1d87c7..1753b0d92a8 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server.py @@ -3355,6 +3355,121 @@ async def test_call_mcp_tool_user_unauthorized_access(): assert "User not allowed to call this tool" in exc_info.value.detail +@pytest.mark.asyncio +async def test_call_mcp_tool_unauthorized_403_does_not_leak_server_credentials(): + """Regression for LIT-4703 / GH #29936. + + Calling a tool on a server the key is not scoped to must 403 with a bare + message. The prior code interpolated the caller's allowed ``List[MCPServer]`` + config objects into the 403 detail, dumping every upstream credential + (authentication_token, client_secret, AWS keys, private keys, env, + static_headers) in cleartext to the caller. + """ + from fastapi import HTTPException + + from litellm.proxy._experimental.mcp_server.server import call_mcp_tool + from litellm.proxy._types import UserAPIKeyAuth + from litellm.types.mcp_server.mcp_server_manager import MCPServer + + mock_user_auth = UserAPIKeyAuth( + api_key="test-key", + user_id="test-user", + team_id="team-basic", + object_permission_id="key-permission-123", + ) + + secret_fields = { + "authentication_token": "sk-LEAK-authtok", + "client_id": "LEAK-clientid", + "client_secret": "sk-LEAK-clientsecret", + "aws_access_key_id": "LEAK-akid", + "aws_secret_access_key": "LEAK-awssecret", + "aws_session_token": "LEAK-awssess", + "client_private_key": "LEAK-privkey", + } + allowed_server_obj = MCPServer( + server_id="allowed_server", + name="allowed_server", + server_name="allowed_server", + alias="allowed_server", + transport="http", + auth_type=MCPAuth.bearer_token, + env={"UPSTREAM_API_KEY": "LEAK-env"}, + static_headers={"X-Upstream-Auth": "LEAK-header"}, + **secret_fields, + ) + + def mock_get_server_by_id(server_id): + if server_id == "allowed_server": + return allowed_server_obj + return None + + with ( + patch( + "litellm.proxy._experimental.mcp_server.auth.user_api_key_auth_mcp.MCPRequestHandler.get_allowed_mcp_servers", + AsyncMock(return_value=["allowed_server"]), + ), + patch( + "litellm.proxy._experimental.mcp_server.server.global_mcp_server_manager.get_mcp_server_by_id", + side_effect=mock_get_server_by_id, + ), + ): + with pytest.raises(HTTPException) as exc_info: + await call_mcp_tool( + name="restricted_server-send_email", + arguments={"to": "test@example.com"}, + user_api_key_auth=mock_user_auth, + mcp_auth_header="Bearer test_token", + ) + + assert exc_info.value.status_code == 403 + assert exc_info.value.detail == "User not allowed to call this tool." + all_secret_values = list(secret_fields.values()) + ["LEAK-env", "LEAK-header"] + detail_text = str(exc_info.value.detail) + leaked = [value for value in all_secret_values if value in detail_text] + assert leaked == [], f"403 body leaked upstream credentials: {leaked}" + + +def test_mcpserver_repr_and_str_mask_credentials(): + """Regression for LIT-4703 / GH #29936. + + ``MCPServer.__repr__``/``__str__`` must never render credential fields, so a + stray f-string, log line, or list interpolation cannot leak them. Only + display is masked; ``model_dump`` serialization is unchanged. + """ + from litellm.types.mcp_server.mcp_server_manager import MCPServer + + secret_fields = { + "authentication_token": "sk-SENTINEL-authtok", + "client_id": "SENTINEL-clientid", + "client_secret": "sk-SENTINEL-clientsecret", + "aws_access_key_id": "SENTINEL-akid", + "aws_secret_access_key": "SENTINEL-awssecret", + "aws_session_token": "SENTINEL-awssess", + "client_private_key": "SENTINEL-privkey", + "client_private_key_id": "SENTINEL-privkeyid", + } + server = MCPServer( + server_id="srv1", + name="srv1", + transport="http", + auth_type=MCPAuth.bearer_token, + env={"UPSTREAM_API_KEY": "SENTINEL-env"}, + static_headers={"X-Upstream-Auth": "SENTINEL-header"}, + env_vars=[{"name": "K", "value": "SENTINEL-envvar"}], + **secret_fields, + ) + + all_secrets = list(secret_fields.values()) + ["SENTINEL-env", "SENTINEL-header", "SENTINEL-envvar"] + for text in (repr(server), str(server), repr([server]), f"{server}", f"{[server]}"): + leaked = [secret for secret in all_secrets if secret in text] + assert leaked == [], f"MCPServer rendering leaked credentials {leaked} in {text!r}" + + assert "srv1" in repr(server) + assert server.model_dump()["authentication_token"] == "sk-SENTINEL-authtok" + assert server.model_dump()["client_secret"] == "sk-SENTINEL-clientsecret" + + @pytest.mark.asyncio async def test_list_tools_filters_by_key_team_permissions(): """Test that list_tools filters tools based on key/team mcp_tool_permissions"""