Skip to content
Open
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
6 changes: 6 additions & 0 deletions litellm/models/mcp_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,12 @@ class LiteLLM_MCPServerTable(LiteLLMPydanticObjectBase):
byok_description: List[str] = Field(default_factory=list)
byok_api_key_help_url: Optional[str] = None
has_user_credential: Optional[bool] = None
has_configured_client: Optional[bool] = Field(
default=None,
description=(
"Response-only indicator that the stored (redacted) credentials include an OAuth client_id; never persisted"
),
)
source_url: Optional[str] = None
timeout: Optional[float] = None
max_concurrent_requests: Optional[int] = None
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4988,6 +4988,7 @@ def _build_mcp_server_table(self, server: MCPServer) -> LiteLLM_MCPServerTable:
is_byok=server.is_byok,
byok_description=server.byok_description,
byok_api_key_help_url=server.byok_api_key_help_url,
has_configured_client=bool(server.client_id),
source_url=server.source_url,
instructions=server.instructions,
timeout=server.timeout,
Expand Down
23 changes: 22 additions & 1 deletion litellm/proxy/management_endpoints/mcp_management_endpoints.py
Original file line number Diff line number Diff line change
Expand Up @@ -473,13 +473,31 @@ async def get_cached_temporary_mcp_server(
def _redact_mcp_credentials(
mcp_server: LiteLLM_MCPServerTable,
) -> LiteLLM_MCPServerTable:
"""Return a copy of the MCP server object with credentials removed."""
"""Return a copy of the MCP server object with credentials removed.

Stamps ``has_configured_client`` before redacting so the admin edit form
can tell that a stored OAuth app exists without ever seeing its value
(the URL-change "app may not match upstream" warning needs exactly this
bit; the stored ``client_id`` itself is encrypted and never returned).
Derives from the credentials blob when the object carries one (DB reads),
otherwise preserves a truthy flag already stamped upstream
(``_build_mcp_server_table`` on the registry list path, whose tables
never include the blob).
"""

try:
redacted_server = mcp_server.model_copy(deep=True)
except AttributeError:
redacted_server = mcp_server.copy(deep=True) # type: ignore[attr-defined]

stored_credentials = getattr(mcp_server, "credentials", None)
stored_client_id = stored_credentials.get("client_id") if isinstance(stored_credentials, dict) else None
setattr(
redacted_server,
"has_configured_client",
bool(stored_client_id or getattr(mcp_server, "has_configured_client", None)),
)

if hasattr(redacted_server, "credentials"):
setattr(redacted_server, "credentials", None)

Expand Down Expand Up @@ -548,6 +566,8 @@ def _sanitize_mcp_server_for_non_admin(
# admin configured. Non-admins get the per-user vars they must fill in
# from the dedicated /user-env-vars/status endpoint instead.
sanitized.env_vars = None
# Only the admin edit form needs the stored-app indicator.
sanitized.has_configured_client = None
return sanitized

def _sanitize_mcp_server_list_for_non_admin(
Expand Down Expand Up @@ -591,6 +611,7 @@ def _sanitize_mcp_server_for_virtual_key(

sanitized.health_check_error = None
sanitized.last_health_check = None
sanitized.has_configured_client = None

sanitized.created_by = None
sanitized.updated_by = None
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7335,3 +7335,44 @@ def test_build_mcp_server_table_carries_null_oauth2_flow():
table = manager._build_mcp_server_table(server)

assert table.oauth2_flow is None


def test_build_mcp_server_table_stamps_has_configured_client():
"""The list endpoint serves registry servers through this conversion WITHOUT the
credentials blob, so the redaction layer cannot see the stored client there. The
build must stamp has_configured_client from the registry's decrypted client_id or
the edit form never learns a saved OAuth app exists (its URL-change "app may not
match upstream" warning would stay silent for stored apps)."""
manager = MCPServerManager()
server = MCPServer(
server_id="stored-app-server",
name="stored_app_server",
server_name="stored_app_server",
alias="stored_app_server",
url="https://up.example.com/mcp",
transport=MCPTransport.http,
auth_type=MCPAuth.true_passthrough,
client_id="org-slack-app-client-id",
client_secret="org-slack-app-secret",
)

table = manager._build_mcp_server_table(server)

assert table.has_configured_client is True


def test_build_mcp_server_table_has_configured_client_false_without_client():
manager = MCPServerManager()
server = MCPServer(
server_id="no-app-server",
name="no_app_server",
server_name="no_app_server",
alias="no_app_server",
url="https://up.example.com/mcp",
transport=MCPTransport.http,
auth_type=MCPAuth.true_passthrough,
)

table = manager._build_mcp_server_table(server)

assert table.has_configured_client is False
Original file line number Diff line number Diff line change
Expand Up @@ -5376,3 +5376,118 @@ async def test_edit_mcp_server_snapshot_failure_skips_purge_but_edit_succeeds():

assert result.server_id == server_id
mock_purge.assert_not_awaited()


def test_redact_stamps_has_configured_client_from_stored_blob():
"""The GET redacts credentials to null, so the edit form cannot see a stored OAuth
app; has_configured_client is the non-secret existence bit the URL-change "app may
not match upstream" warning keys on. Redaction must stamp it from the blob it is
about to remove, and must not leak or mutate the blob itself."""
from litellm.proxy.management_endpoints.mcp_management_endpoints import (
_redact_mcp_credentials,
)

server = generate_mock_mcp_server_db_record()
server.credentials = {"client_id": "encrypted-client", "client_secret": "encrypted-secret"}

redacted = _redact_mcp_credentials(server)

assert redacted.has_configured_client is True
assert redacted.credentials is None
assert server.credentials == {"client_id": "encrypted-client", "client_secret": "encrypted-secret"}


@pytest.mark.parametrize(
"credentials",
[None, {"auth_value": "top-secret"}, {"client_id": ""}],
ids=["no-blob", "no-client-in-blob", "empty-client-id"],
)
def test_redact_stamps_has_configured_client_false_without_stored_client(credentials):
from litellm.proxy.management_endpoints.mcp_management_endpoints import (
_redact_mcp_credentials,
)

server = generate_mock_mcp_server_db_record()
server.credentials = credentials

redacted = _redact_mcp_credentials(server)

assert redacted.has_configured_client is False
assert redacted.credentials is None


def test_redact_preserves_build_time_has_configured_client():
"""The list endpoint serves registry servers whose table objects never carry the
credentials blob; _build_mcp_server_table stamps the flag instead. Redaction must
preserve that stamp rather than resetting it to False for lack of a blob."""
from litellm.proxy.management_endpoints.mcp_management_endpoints import (
_redact_mcp_credentials,
)

server = generate_mock_mcp_server_db_record()
server.credentials = None
server.has_configured_client = True

redacted = _redact_mcp_credentials(server)

assert redacted.has_configured_client is True


def test_sanitized_views_drop_has_configured_client():
"""Only the admin edit form needs the stored-app indicator; the non-admin and
virtual-key discovery views must not reveal whether an OAuth app is configured."""
import litellm.proxy.management_endpoints.mcp_management_endpoints as mgmt

server = generate_mock_mcp_server_db_record()
server.credentials = {"client_id": "encrypted-client"}

assert mgmt._sanitize_mcp_server_for_non_admin(server).has_configured_client is None
assert mgmt._sanitize_mcp_server_for_virtual_key(server).has_configured_client is None


@pytest.mark.asyncio
async def test_fetch_single_mcp_server_returns_has_configured_client():
"""End to end through GET /v1/mcp/server/{id}: a stored client surfaces only as
has_configured_client=True while the credentials stay redacted."""
mock_server = generate_mock_mcp_server_db_record(server_id="server-1", alias="Server 1")
mock_server.credentials = {"client_id": "encrypted-client", "client_secret": "encrypted-secret"}

mock_prisma_client = MagicMock()

mock_health_result = generate_mock_mcp_server_db_record(server_id="server-1", alias="Server 1")
mock_health_result.status = "healthy"
mock_health_result.last_health_check = datetime.now()
mock_health_result.health_check_error = None

mock_user_auth = generate_mock_user_api_key_auth(user_role=LitellmUserRoles.PROXY_ADMIN)

with (
patch(
"litellm.proxy.management_endpoints.mcp_management_endpoints.get_prisma_client_or_throw",
return_value=mock_prisma_client,
),
patch(
"litellm.proxy.management_endpoints.mcp_management_endpoints.get_mcp_server",
AsyncMock(return_value=mock_server),
),
patch(
"litellm.proxy.management_endpoints.mcp_management_endpoints.global_mcp_server_manager.health_check_server",
AsyncMock(return_value=mock_health_result),
),
patch(
"litellm.proxy.management_endpoints.mcp_management_endpoints._user_has_admin_view",
return_value=True,
),
):
from litellm.proxy.management_endpoints.mcp_management_endpoints import (
fetch_mcp_server,
)

result = await fetch_mcp_server(
request=_make_mock_request(),
server_id="server-1",
user_api_key_dict=mock_user_auth,
)

assert result.has_configured_client is True
assert result.credentials is None
Original file line number Diff line number Diff line change
Expand Up @@ -51,4 +51,21 @@ describe("PassthroughAuthorizeSection credential-class-aware copy", () => {
);
expect(screen.getByText(/registered for the previous upstream/)).toBeInTheDocument();
});

it("hides the keep+warn banner while the remove-stored-app checkbox is checked", () => {
render(
<WithForm>
<PassthroughAuthorizeSection
authType="true_passthrough"
oauthFlow={noopFlow}
isEditing
savedAuthType="true_passthrough"
appMayNotMatchUpstream
removeStoredApp
onRemoveStoredAppChange={() => {}}
/>
</WithForm>,
);
expect(screen.queryByText(/registered for the previous upstream/)).not.toBeInTheDocument();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,11 @@ export default function PassthroughAuthorizeSection({
and is never saved to LiteLLM. An OAuth app configured below IS saved with the server, so internal users who
authorize from the Tools page go through it.
</p>
{appMayNotMatchUpstream && (
{appMayNotMatchUpstream && !removeStoredApp && (
<p className="text-sm text-amber-600">
You changed the upstream URL or endpoints; the OAuth app entered here was registered for the previous upstream
and may not be valid. Update the client ID, or clear it to use dynamic client registration.
You changed the upstream URL or endpoints; the OAuth app configured for this server was registered for the
previous upstream and may not be valid. Enter a client ID registered for the new upstream, or remove the app
to use dynamic client registration.
</p>
)}
<Form.Item
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@
}));

vi.mock("./mcp_connection_status", () => ({
default: ({ tools }: { tools?: any[] }) => (

Check warning on line 89 in ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.test.tsx

View workflow job for this annotation

GitHub Actions / frontend-lint

Unexpected any. Specify a different type
<div data-testid="mcp-connection-status" data-tool-count={tools?.length ?? 0} />
),
}));
Expand Down Expand Up @@ -242,7 +242,7 @@
});

// Leave auth value empty and submit
vi.mocked(networking.createMCPServer).mockResolvedValue({

Check warning on line 245 in ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.test.tsx

View workflow job for this annotation

GitHub Actions / frontend-lint

Object literal with 10 properties passed inline as an argument; assign it to a named variable first
server_id: "new-server-1",
server_name: "Test_Server",
alias: "Test_Server",
Expand Down Expand Up @@ -284,7 +284,7 @@
});

// Leave auth value empty and submit
vi.mocked(networking.createMCPServer).mockResolvedValue({

Check warning on line 287 in ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.test.tsx

View workflow job for this annotation

GitHub Actions / frontend-lint

Object literal with 10 properties passed inline as an argument; assign it to a named variable first
server_id: "new-server-1",
server_name: "Test_Server",
alias: "Test_Server",
Expand Down Expand Up @@ -328,7 +328,7 @@
const authInput = screen.getByPlaceholderText("Enter token or secret");
await user.type(authInput, "my-secret-key");

vi.mocked(networking.createMCPServer).mockResolvedValue({

Check warning on line 331 in ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.test.tsx

View workflow job for this annotation

GitHub Actions / frontend-lint

Object literal with 10 properties passed inline as an argument; assign it to a named variable first
server_id: "new-server-1",
server_name: "My_Server",
alias: "My_Server",
Expand Down Expand Up @@ -802,7 +802,9 @@
});

// Keep + warn: the app stays in the field, and a non-blocking warning appears.
expect(screen.getByText(/OAuth app entered here was registered for the previous upstream/)).toBeInTheDocument();
expect(
screen.getByText(/OAuth app configured for this server was registered for the previous upstream/),
).toBeInTheDocument();
});

it("keeps client_secret when only client_id is edited after a client-forwarded authorize", async () => {
Expand Down Expand Up @@ -891,7 +893,7 @@

await selectAntOption("Authentication", "None");

vi.mocked(networking.createMCPServer).mockResolvedValue({

Check warning on line 896 in ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.test.tsx

View workflow job for this annotation

GitHub Actions / frontend-lint

Object literal with 10 properties passed inline as an argument; assign it to a named variable first
server_id: "new-server-1",
server_name: "No_Auth_Server",
alias: "No_Auth_Server",
Expand Down Expand Up @@ -973,7 +975,7 @@
const limitInput = screen.getByPlaceholderText("e.g. 10");
await user.type(limitInput, "5");

vi.mocked(networking.createMCPServer).mockResolvedValue({

Check warning on line 978 in ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.test.tsx

View workflow job for this annotation

GitHub Actions / frontend-lint

Object literal with 10 properties passed inline as an argument; assign it to a named variable first
server_id: "new-server-1",
server_name: "Limited_Server",
alias: "Limited_Server",
Expand Down Expand Up @@ -1026,7 +1028,7 @@
target: { value: "te-client-secret" },
});

vi.mocked(networking.createMCPServer).mockResolvedValue({

Check warning on line 1031 in ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.test.tsx

View workflow job for this annotation

GitHub Actions / frontend-lint

Object literal with 10 properties passed inline as an argument; assign it to a named variable first
server_id: "new-server-te",
server_name: "TE_Server",
alias: "TE_Server",
Expand Down Expand Up @@ -1117,7 +1119,7 @@
fireEvent.click(screen.getByRole("button", { name: "Disable all tools" }));
});

vi.mocked(networking.createMCPServer).mockResolvedValue({

Check warning on line 1122 in ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.test.tsx

View workflow job for this annotation

GitHub Actions / frontend-lint

Object literal with 10 properties passed inline as an argument; assign it to a named variable first
server_id: "new-server-1",
server_name: "Locked_Down_Server",
alias: "Locked_Down_Server",
Expand Down Expand Up @@ -1262,7 +1264,7 @@
});

it("includes token_validation in payload when token_validation_json is filled with valid JSON", async () => {
vi.mocked(networking.createMCPServer).mockResolvedValue({

Check warning on line 1267 in ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.test.tsx

View workflow job for this annotation

GitHub Actions / frontend-lint

Object literal with 10 properties passed inline as an argument; assign it to a named variable first
server_id: "new-server-oauth",
server_name: "OAuth_Server",
alias: "OAuth_Server",
Expand Down Expand Up @@ -1329,7 +1331,7 @@

await waitFor(() => expect(oauthHook.reset).toHaveBeenCalled());

vi.mocked(networking.createMCPServer).mockResolvedValue({

Check warning on line 1334 in ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.test.tsx

View workflow job for this annotation

GitHub Actions / frontend-lint

Object literal with 10 properties passed inline as an argument; assign it to a named variable first
server_id: "new-server-oauth",
server_name: "Url_Change_Server",
alias: "Url_Change_Server",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1549,6 +1549,103 @@ describe("MCPServerEdit (OAuth token persistence on save)", () => {
expect(screen.getByText(/registered for the previous upstream/)).toBeInTheDocument();
});

it("warns after a URL change when the stored app is redacted (has_configured_client, blank fields)", async () => {
// The real GET redacts credentials to null, so the form holds no client even though the server
// has a saved app. has_configured_client is the backend's non-secret "a client exists" bit; the
// warning must fire from it, otherwise keep-existing silently keeps an app registered for the
// old upstream and the admin is never told.
render(
<MCPServerEdit
mcpServer={{
...interactiveOAuthServer,
auth_type: "true_passthrough",
credentials: null,
has_configured_client: true,
}}
accessToken="access-token"
userID="user-1"
onCancel={vi.fn()}
onSuccess={vi.fn()}
availableAccessGroups={[]}
/>,
);

expect(screen.queryByText(/registered for the previous upstream/)).not.toBeInTheDocument();

await act(async () => {
fireEvent.change(screen.getByPlaceholderText("https://your-mcp-server.com"), {
target: { value: "https://different.example.com/mcp" },
});
});

expect(screen.getByText(/registered for the previous upstream/)).toBeInTheDocument();
});

it("hides the stored-app warning while the remove checkbox is checked and restores it on uncheck", async () => {
// Removal writes an explicit-null credential on save, so nothing kept can mismatch; unchecking
// returns to keep-existing, where the mismatch concern is live again.
render(
<MCPServerEdit
mcpServer={{
...interactiveOAuthServer,
auth_type: "true_passthrough",
credentials: null,
has_configured_client: true,
}}
accessToken="access-token"
userID="user-1"
onCancel={vi.fn()}
onSuccess={vi.fn()}
availableAccessGroups={[]}
/>,
);

await act(async () => {
fireEvent.change(screen.getByPlaceholderText("https://your-mcp-server.com"), {
target: { value: "https://different.example.com/mcp" },
});
});
expect(screen.getByText(/registered for the previous upstream/)).toBeInTheDocument();

const removeCheckbox = screen.getByRole("checkbox", { name: /Remove the saved OAuth app on save/ });
fireEvent.click(removeCheckbox);
expect(screen.queryByText(/registered for the previous upstream/)).not.toBeInTheDocument();

fireEvent.click(removeCheckbox);
expect(screen.getByText(/registered for the previous upstream/)).toBeInTheDocument();
});

it("does not warn from has_configured_client after a cross-class auth switch", async () => {
// Saved oauth2 server with a stored client (e.g. a persisted DCR app). Switching to
// true_passthrough is a cross-class change: blanks mean "no app" and the stored app is replaced
// on save, so a URL change has nothing kept to warn about.
render(
<MCPServerEdit
mcpServer={{
...interactiveOAuthServer,
auth_type: "oauth2",
credentials: null,
has_configured_client: true,
}}
accessToken="access-token"
userID="user-1"
onCancel={vi.fn()}
onSuccess={vi.fn()}
availableAccessGroups={[]}
/>,
);

await selectAntOption("Authentication", "True Passthrough (no LiteLLM auth)");

await act(async () => {
fireEvent.change(screen.getByPlaceholderText("https://your-mcp-server.com"), {
target: { value: "https://different.example.com/mcp" },
});
});

expect(screen.queryByText(/registered for the previous upstream/)).not.toBeInTheDocument();
});

it("preserves a stored client_id on OAuth-resume restore even when the saved snapshot is token-only", async () => {
// Post-redirect restore: the sessionStorage snapshot carries only a minted token (no client keys),
// while the loaded server has a stored client_id. The restore must merge the server's declared app
Expand Down
Loading
Loading