Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 20 additions & 15 deletions litellm/proxy/_experimental/mcp_server/db.py
Original file line number Diff line number Diff line change
Expand Up @@ -557,10 +557,12 @@ async def delete_mcp_server(
"""
Delete the mcp server from the db by server_id

The server-row delete is the commit point. Per-user env var rows have no FK
cascade, so they are cleaned up afterwards on a best-effort basis: a transient
failure there leaves only orphaned rows pointing at a now-missing server and
must not turn a successful delete into a caller-visible error.
The server-row delete is the commit point. Per-user credential and env var
rows have no FK cascade, so they are cleaned up afterwards on a best-effort
basis: a transient failure there leaves only orphaned rows pointing at a
now-missing server and must not turn a successful delete into a
caller-visible error. Each table is cleaned independently so a failure on one
still attempts the other.

Returns the deleted mcp server record if it exists, otherwise None
"""
Expand All @@ -570,17 +572,20 @@ async def delete_mcp_server(
},
)
if deleted_server is not None:
try:
await prisma_client.db.litellm_mcpuserenvvars.delete_many(
where={"server_id": server_id}
)
except Exception as e:
verbose_proxy_logger.warning(
"MCP server %s deleted but per-user env var cleanup failed; "
"orphaned rows can be removed on a later delete: %s",
server_id,
e,
)
for model, label in (
(prisma_client.db.litellm_mcpusercredentials, "credential"),
(prisma_client.db.litellm_mcpuserenvvars, "env var"),
):
try:
await model.delete_many(where={"server_id": server_id})
except Exception as e:
verbose_proxy_logger.warning(
"MCP server %s deleted but per-user %s cleanup failed; "
"orphaned rows can be removed on a later delete: %s",
server_id,
label,
e,
)
return deleted_server


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -512,12 +512,13 @@ async def exchange_token_with_server(
result = {
"access_token": access_token,
"token_type": token_response.get("token_type", "Bearer"),
"expires_in": token_response.get("expires_in", 3600),
}

if "refresh_token" in token_response and token_response["refresh_token"]:
if token_response.get("expires_in") is not None:
result["expires_in"] = token_response["expires_in"]
if token_response.get("refresh_token"):
result["refresh_token"] = token_response["refresh_token"]
if "scope" in token_response and token_response["scope"]:
if token_response.get("scope"):
result["scope"] = token_response["scope"]

# RFC 6749 §5.1: token responses must not be cached.
Expand Down
31 changes: 30 additions & 1 deletion litellm/proxy/_experimental/mcp_server/rest_endpoints.py
Original file line number Diff line number Diff line change
Expand Up @@ -386,8 +386,15 @@ async def _get_tools_for_single_server(
raw_headers: Optional[Dict[str, str]] = None,
user_api_key_auth: Optional[UserAPIKeyAuth] = None,
extra_headers: Optional[Dict[str, str]] = None,
apply_tool_filters: bool = True,
):
"""Helper function to get tools for a single server."""
"""Helper function to get tools for a single server.

When ``apply_tool_filters`` is False the raw server catalog is returned
without the allowed_tools/disallowed_tools gate or the per-key tool
permissions. This is the admin-only configuration view; every runtime
path keeps the default True so callable tools stay filtered.
"""
tools = await global_mcp_server_manager._get_tools_from_server(
server=server,
mcp_auth_header=server_auth_header,
Expand All @@ -397,6 +404,9 @@ async def _get_tools_for_single_server(
user_api_key_auth=user_api_key_auth,
)

if not apply_tool_filters:
return _create_tool_response_objects(tools, server.mcp_info)

# Always apply allowed_tools/disallowed_tools so the blacklist is
# enforced even when no allowlist is set (matches the SSE/HTTP path).
tools = filter_tools_by_allowed_tools(tools, server)
Expand Down Expand Up @@ -463,6 +473,7 @@ async def _list_tools_for_single_server(
mcp_auth_header: Optional[str],
raw_headers_from_request: dict,
user_api_key_dict: UserAPIKeyAuth,
apply_tool_filters: bool = True,
) -> dict:
"""Handle tool listing for a single server_id request."""
# Resolve a server name to its UUID if needed
Expand Down Expand Up @@ -527,6 +538,7 @@ async def _list_tools_for_single_server(
raw_headers_from_request,
user_api_key_dict,
extra_headers=user_oauth_extra_headers,
apply_tool_filters=apply_tool_filters,
)
except MCPUpstreamAuthError:
# Surface the upstream 401/403 to the caller so it can emit the
Expand All @@ -552,6 +564,14 @@ async def list_tool_rest_api(
server_id: Optional[str] = Query(
None, description="The server id to list tools for"
),
include_disabled_tools: bool = Query(
False,
description=(
"Admin only. Return the full server tool catalog without the "
"allowed_tools filter or per-key tool permissions, so the MCP "
"settings UI can configure the allowlist. Ignored for non-admins."
),
),
user_api_key_dict: UserAPIKeyAuth = Depends(user_api_key_auth),
) -> dict:
"""
Expand Down Expand Up @@ -579,6 +599,13 @@ async def list_tool_rest_api(
)

try:
# The full catalog (allowlist filter skipped) is admin-only so the
# REST endpoint can't be used to enumerate deliberately-disabled tools.
apply_tool_filters = not (
include_disabled_tools
and user_api_key_dict.user_role == LitellmUserRoles.PROXY_ADMIN
)

# Extract auth headers from request
headers = request.headers
raw_headers_from_request = dict(headers)
Expand Down Expand Up @@ -620,6 +647,7 @@ async def list_tool_rest_api(
mcp_auth_header=mcp_auth_header,
raw_headers_from_request=raw_headers_from_request,
user_api_key_dict=user_api_key_dict,
apply_tool_filters=apply_tool_filters,
)
else:
if not allowed_server_ids:
Expand Down Expand Up @@ -677,6 +705,7 @@ async def list_tool_rest_api(
raw_headers_from_request,
user_api_key_dict,
extra_headers=user_oauth_extra_headers,
apply_tool_filters=apply_tool_filters,
)
list_tools_result.extend(tools_result)
except Exception as e:
Expand Down
12 changes: 7 additions & 5 deletions litellm/proxy/management_endpoints/mcp_management_endpoints.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
import os
from dataclasses import dataclass
from datetime import datetime, timedelta, timezone
from typing import Any, Dict, Iterable, List, Literal, Optional
from typing import Any, Dict, Iterable, List, Literal, Optional, Set

from fastapi import (
APIRouter,
Expand Down Expand Up @@ -1714,11 +1714,13 @@ async def _get_cached_temporary_mcp_server_or_404(
status_code=status.HTTP_403_FORBIDDEN,
detail={"error": f"Access denied to MCP server {server_id}"},
)
allowed_server_ids = (
await global_mcp_server_manager.get_allowed_mcp_servers(
user_api_key_dict
allowed_server_ids: Set[str] = set()
for auth_context in await build_effective_auth_contexts(user_api_key_dict):
allowed_server_ids.update(
await global_mcp_server_manager.get_allowed_mcp_servers(
auth_context
)
)
)
if server.server_id not in allowed_server_ids:
raise HTTPException(
status_code=status.HTTP_403_FORBIDDEN,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""Tests for MCP OAuth discoverable endpoints"""

import json
from unittest.mock import AsyncMock, MagicMock, patch

import pytest
Expand Down Expand Up @@ -2661,3 +2662,74 @@ async def test_token_endpoint_sets_no_store_cache_control():

assert response.headers["cache-control"] == "no-store"
assert response.headers["pragma"] == "no-cache"


async def _exchange_with_upstream_token_response(upstream_body):
from fastapi import Request

from litellm.proxy._experimental.mcp_server.discoverable_endpoints import (
exchange_token_with_server,
)
from litellm.proxy._types import MCPTransport
from litellm.types.mcp import MCPAuth
from litellm.types.mcp_server.mcp_server_manager import MCPServer

server = MCPServer(
server_id="t",
name="t",
server_name="t",
alias="t",
transport=MCPTransport.http,
auth_type=MCPAuth.oauth2,
client_id="cid",
client_secret="cs",
authorization_url="https://provider.com/oauth/authorize",
token_url="https://provider.com/oauth/token",
)
mock_request = MagicMock(spec=Request)
mock_request.base_url = "https://litellm.example.com/"
mock_request.headers = {}

fake_http_response = MagicMock()
fake_http_response.json.return_value = upstream_body
fake_http_response.raise_for_status = MagicMock()
fake_http_client = MagicMock()
fake_http_client.post = AsyncMock(return_value=fake_http_response)

with patch(
"litellm.proxy._experimental.mcp_server.discoverable_endpoints.get_async_httpx_client",
return_value=fake_http_client,
):
response = await exchange_token_with_server(
request=mock_request,
mcp_server=server,
grant_type="authorization_code",
code="c",
redirect_uri="http://127.0.0.1:3000/cb",
client_id="cid",
client_secret=None,
code_verifier=None,
)
return json.loads(response.body)


@pytest.mark.asyncio
async def test_token_exchange_omits_expires_in_when_upstream_omits_it():
"""A provider that issues a non-expiring token (e.g. Slack without token
rotation) returns no ``expires_in``. The exchange must mirror that and omit
``expires_in`` rather than fabricate a 1-hour TTL, so the stored credential
is treated as non-expiring instead of dying after an hour."""
body = await _exchange_with_upstream_token_response(
{"access_token": "tok", "token_type": "Bearer"}
)
assert "expires_in" not in body


@pytest.mark.asyncio
async def test_token_exchange_passes_through_upstream_expires_in():
"""When the provider does send ``expires_in`` (e.g. Slack with token
rotation), the exchange forwards the real value unchanged."""
body = await _exchange_with_upstream_token_response(
{"access_token": "tok", "token_type": "Bearer", "expires_in": 43200}
)
assert body["expires_in"] == 43200
Original file line number Diff line number Diff line change
Expand Up @@ -872,6 +872,7 @@ def _mock_env_vars_prisma(row=None):
prisma.db.litellm_mcpuserenvvars.find_many = AsyncMock(return_value=[])
prisma.db.litellm_mcpuserenvvars.upsert = AsyncMock()
prisma.db.litellm_mcpuserenvvars.delete_many = AsyncMock()
prisma.db.litellm_mcpusercredentials.delete_many = AsyncMock()
return prisma


Expand Down Expand Up @@ -1252,6 +1253,64 @@ async def test_delete_mcp_server_succeeds_when_orphan_cleanup_fails():
prisma.db.litellm_mcpuserenvvars.delete_many.assert_awaited_once()


@pytest.mark.asyncio
async def test_delete_mcp_server_removes_orphaned_user_credentials():
"""Deleting a server must also drop every user's stored BYOK/OAuth credential
rows for it; there is no FK cascade, so skipping this leaves encrypted secrets
pointing at a now-missing server."""
from unittest.mock import AsyncMock

from litellm.proxy._experimental.mcp_server.db import delete_mcp_server

prisma = _mock_env_vars_prisma()
prisma.db.litellm_mcpservertable.delete = AsyncMock(return_value=object())

await delete_mcp_server(prisma, "srv-1")

prisma.db.litellm_mcpusercredentials.delete_many.assert_awaited_once()
call = prisma.db.litellm_mcpusercredentials.delete_many.call_args
assert call.kwargs["where"] == {"server_id": "srv-1"}


@pytest.mark.asyncio
async def test_delete_mcp_server_skips_credential_cleanup_when_server_missing():
"""A no-op delete (server not found) must not touch the credential table."""
from unittest.mock import AsyncMock

from litellm.proxy._experimental.mcp_server.db import delete_mcp_server

prisma = _mock_env_vars_prisma()
prisma.db.litellm_mcpservertable.delete = AsyncMock(return_value=None)

result = await delete_mcp_server(prisma, "srv-1")

assert result is None
prisma.db.litellm_mcpusercredentials.delete_many.assert_not_awaited()


@pytest.mark.asyncio
async def test_delete_mcp_server_credential_cleanup_failure_still_cleans_env_vars():
"""Each per-user table is cleaned independently: a failure dropping credential
rows must not skip the env var cleanup (or vice versa), and the delete must
still succeed for the caller."""
from unittest.mock import AsyncMock

from litellm.proxy._experimental.mcp_server.db import delete_mcp_server

deleted = object()
prisma = _mock_env_vars_prisma()
prisma.db.litellm_mcpservertable.delete = AsyncMock(return_value=deleted)
prisma.db.litellm_mcpusercredentials.delete_many = AsyncMock(
side_effect=Exception("connection pool exhausted")
)

result = await delete_mcp_server(prisma, "srv-1")

assert result is deleted
prisma.db.litellm_mcpusercredentials.delete_many.assert_awaited_once()
prisma.db.litellm_mcpuserenvvars.delete_many.assert_awaited_once()


# ── DB helpers: global env vars encrypted at rest ─────────────────────────


Expand Down
Loading
Loading