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
13 changes: 9 additions & 4 deletions litellm/proxy/_experimental/mcp_server/mcp_server_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -608,12 +608,15 @@ def _resolve_oauth2_flow(
) -> Optional[Literal["client_credentials", "authorization_code"]]:
"""Infer oauth2_flow from field shape when the value is omitted.

Not called directly by security sites; they go through ``effective_oauth2_flow``
(boolean/enum decisions) or ``resolve_oauth2_flow_for_request`` (the egress object
backstop), which are the single choke points for request-time resolution. DB rows
SECURITY-SENSITIVE: this is the shape-inference engine both request-time security
helpers delegate to, so it is what decides M2M-vs-interactive for an unstamped row.
Always access it through ``effective_oauth2_flow`` (boolean/enum decisions) or
``resolve_oauth2_flow_for_request`` (the egress object backstop), which are the single
choke points for request-time resolution; do not call it directly from security sites
and do not weaken its M2M-shape branch without accounting for those callers. DB rows
are stamped at write time and by the startup backfill, config servers must declare
oauth2_flow (validated at load), and both builds read the value verbatim via
``_explicit_oauth2_flow``. Delete this whole request-time layer once the backstop
``_explicit_oauth2_flow``. Delete this whole request-time layer only once the backstop
warning stays silent in production.
"""
if oauth2_flow in ("client_credentials", "authorization_code"):
Expand Down Expand Up @@ -4626,6 +4629,7 @@ async def _noop(session):
authorization_url=server.authorization_url,
token_url=server.token_url,
registration_url=server.registration_url,
oauth2_flow=server.oauth2_flow,
allow_all_keys=server.allow_all_keys,
instructions=server.instructions,
timeout=server.timeout,
Expand Down Expand Up @@ -4729,6 +4733,7 @@ def _build_mcp_server_table(self, server: MCPServer) -> LiteLLM_MCPServerTable:
authorization_url=server.authorization_url,
token_url=server.token_url,
registration_url=server.registration_url,
oauth2_flow=server.oauth2_flow,
allow_all_keys=server.allow_all_keys,
available_on_public_internet=server.available_on_public_internet,
delegate_auth_to_upstream=server.delegate_auth_to_upstream,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6282,3 +6282,45 @@ def test_resolve_for_request_null_m2m_shape_copies_client_credentials(self, capl
assert "no persisted oauth2_flow" in joined
assert "next proxy boot" not in joined
assert "will NOT self-heal" in joined


def test_build_mcp_server_table_carries_oauth2_flow():
"""GET /v1/mcp/server (list and by-id) serves registry servers through this
conversion; dropping oauth2_flow here blinds the dashboard to the persisted
flow, so the edit page cannot prefill and M2M gating never activates."""
manager = MCPServerManager()
server = MCPServer(
server_id="flow-table-server",
name="flow_table_server",
server_name="flow_table_server",
alias="flow_table_server",
url="https://up.example.com/mcp",
transport=MCPTransport.http,
auth_type=MCPAuth.oauth2,
oauth2_flow="client_credentials",
)

table = manager._build_mcp_server_table(server)

assert table.oauth2_flow == "client_credentials"


def test_build_mcp_server_table_carries_null_oauth2_flow():
"""A legacy row the backfill left unstamped must surface as oauth2_flow=None in
the GET response, so the dashboard maps it to undefined and prompts the admin to
choose a flow rather than showing a guessed default."""
manager = MCPServerManager()
server = MCPServer(
server_id="null-flow-server",
name="null_flow_server",
server_name="null_flow_server",
alias="null_flow_server",
url="https://up.example.com/mcp",
transport=MCPTransport.http,
auth_type=MCPAuth.oauth2,
oauth2_flow=None,
)

table = manager._build_mcp_server_table(server)

assert table.oauth2_flow is None
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import React from "react";
import { render, screen } from "@testing-library/react";
import { describe, it, expect, vi } from "vitest";
import MCPServerCard from "./MCPServerCard";
import type { MCPServer } from "./types";

const baseServer: MCPServer = {
server_id: "srv-1",
server_name: "demo_server",
alias: "demo_server",
transport: "http",
url: "https://example.com/mcp",
auth_type: "oauth2",
} as MCPServer;

function renderCard(overrides: Partial<MCPServer>) {
render(<MCPServerCard server={{ ...baseServer, ...overrides } as MCPServer} onClick={vi.fn()} />);
}

describe("MCPServerCard OAuth flow indicator", () => {
it("shows the 'OAuth flow not set' badge for an oauth2 server with no oauth2_flow", () => {
renderCard({ auth_type: "oauth2", oauth2_flow: null });
expect(screen.getByText("OAuth flow not set")).toBeInTheDocument();
});

it("does not show the badge once oauth2_flow is set (client_credentials)", () => {
renderCard({ auth_type: "oauth2", oauth2_flow: "client_credentials" });
expect(screen.queryByText("OAuth flow not set")).not.toBeInTheDocument();
});

it("does not show the badge once oauth2_flow is set (authorization_code)", () => {
renderCard({ auth_type: "oauth2", oauth2_flow: "authorization_code" });
expect(screen.queryByText("OAuth flow not set")).not.toBeInTheDocument();
});

it("does not show the badge for a non-oauth2 server", () => {
renderCard({ auth_type: "api_key", oauth2_flow: null });
expect(screen.queryByText("OAuth flow not set")).not.toBeInTheDocument();
});

it("does not show the badge for a delegate (PKCE passthrough) server", () => {
renderCard({ auth_type: "oauth2", oauth2_flow: null, delegate_auth_to_upstream: true });
expect(screen.queryByText("OAuth flow not set")).not.toBeInTheDocument();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
MoreOutlined,
ThunderboltOutlined,
} from "@ant-design/icons";
import type { MCPServer } from "./types";
import { AUTH_TYPE, type MCPServer } from "./types";
import { getMaskedAndFullUrl } from "./utils";

const { Text } = Typography;
Expand Down Expand Up @@ -47,7 +47,7 @@
onByokConnect,
onOpenFillFields,
onDelete,
}) => {

Check warning on line 50 in ui/litellm-dashboard/src/components/mcp_tools/MCPServerCard.tsx

View workflow job for this annotation

GitHub Actions / frontend-lint

Arrow function has a complexity of 46. Maximum allowed is 20
const alias = server.alias || server.server_name || "";
const name = server.server_name || alias || server.server_id;
// Logo is sourced exclusively from the admin-set `mcp_info.logo_url`.
Expand All @@ -57,6 +57,14 @@
const transport = server.transport || "http";
const displayTransport = server.spec_path && transport !== "stdio" ? "openapi" : transport;
const authType = server.auth_type || "none";
// An oauth2 server with no persisted oauth2_flow was never classified as M2M vs
// interactive; flag it so an admin can set it from the edit page (see the OAuth
// Flow Type selector) instead of leaving LiteLLM to fall back to a default.
// Delegate (PKCE passthrough) servers authenticate upstream and route to
// passthrough regardless of oauth2_flow, so the classification does not apply to
// them and they are not flagged.
const oauthFlowUnset =
server.auth_type === AUTH_TYPE.OAUTH2 && !server.oauth2_flow && !server.delegate_auth_to_upstream;
const status = server.status || "unknown";
const healthTone = HEALTH_TONE[status] ?? HEALTH_TONE.unknown;
const isPublic = server.available_on_public_internet;
Expand Down Expand Up @@ -141,7 +149,7 @@
>
<div className="flex items-start gap-3">
{logoUrl ? (
<img

Check warning on line 152 in ui/litellm-dashboard/src/components/mcp_tools/MCPServerCard.tsx

View workflow job for this annotation

GitHub Actions / frontend-lint

Using `<img>` could result in slower LCP and higher bandwidth. Consider using `<Image />` from `next/image` or a custom image loader to automatically optimize images. This may incur additional usage or cost from your provider. See: https://nextjs.org/docs/messages/no-img-element
src={logoUrl}
alt={`${name} logo`}
className="h-10 w-10 shrink-0 rounded-sm object-contain"
Expand Down Expand Up @@ -203,6 +211,16 @@
/>
<Tag className="m-0">{displayTransport.toUpperCase()}</Tag>
<Tag className="m-0">{authType}</Tag>
{oauthFlowUnset && (
<Tooltip title="This OAuth server has no flow set (Machine-to-Machine vs Interactive). Open it and choose an OAuth Flow Type so LiteLLM authenticates it as you intend.">
<Tag color="warning" className="m-0">
<span className="inline-flex items-center gap-1">
<ExclamationCircleFilled />
OAuth flow not set
</span>
</Tag>
</Tooltip>
)}
<Tag color={isPublic ? "green" : "orange"} className="m-0">
<span className="inline-flex items-center gap-1">
<span className={`h-1.5 w-1.5 rounded-full ${isPublic ? "bg-green-500" : "bg-orange-500"}`} />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
},
}));

const mockOauth: { tokenResponse: any } = { tokenResponse: null };

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

View workflow job for this annotation

GitHub Actions / frontend-lint

Unexpected any. Specify a different type
vi.mock("@/hooks/useMcpOAuthFlow", () => ({
useMcpOAuthFlow: () => ({
startOAuthFlow: vi.fn(),
Expand All @@ -46,7 +46,7 @@
onToolAllowlistInteraction,
onToolNameToDisplayNameChange,
onToolNameToDescriptionChange,
}: any) => (

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

View workflow job for this annotation

GitHub Actions / frontend-lint

Unexpected any. Specify a different type
<div
data-testid="mcp-tool-config"
data-existing-allowed-tools={JSON.stringify(existingAllowedTools)}
Expand Down Expand Up @@ -87,9 +87,9 @@
const mockIsTokenValid = vi.fn();
const mockSetToken = vi.fn();
vi.mock("@/utils/mcpTokenStore", () => ({
getToken: (...args: any[]) => mockGetToken(...args),

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

View workflow job for this annotation

GitHub Actions / frontend-lint

Unexpected any. Specify a different type
isTokenValid: (...args: any[]) => mockIsTokenValid(...args),

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

View workflow job for this annotation

GitHub Actions / frontend-lint

Unexpected any. Specify a different type
setToken: (...args: any[]) => mockSetToken(...args),

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

View workflow job for this annotation

GitHub Actions / frontend-lint

Unexpected any. Specify a different type
}));

// ── fixtures ──────────────────────────────────────────────────────────────────
Expand Down Expand Up @@ -886,7 +886,7 @@
};

// Mount before the server is loaded (mirrors landing on the page mid OAuth return).
const { rerender } = render(<MCPServerEdit mcpServer={{ server_id: "" } as any} {...props} />);

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

View workflow job for this annotation

GitHub Actions / frontend-lint

Unexpected any. Specify a different type
expect(screen.queryByDisplayValue("https://example.com/mcp")).not.toBeInTheDocument();

// Server data arrives; the form must repopulate rather than staying blank.
Expand Down Expand Up @@ -1039,7 +1039,7 @@
});
});

describe("MCPServerEdit oauth2_flow preservation", () => {
describe("MCPServerEdit oauth2_flow selector", () => {
beforeEach(() => {
vi.clearAllMocks();
});
Expand Down Expand Up @@ -1078,19 +1078,155 @@
expect(payload).not.toHaveProperty("oauth2_flow");
});

it("never writes oauth2_flow over an explicit client_credentials row", async () => {
it("re-writes an explicit client_credentials row with its own prefilled value", async () => {
const payload = await saveAndGetPayload({
oauth2_flow: "client_credentials",
token_url: "https://idp.example.com/oauth/token",
});
expect(payload).not.toHaveProperty("oauth2_flow");
expect(payload.oauth2_flow).toBe("client_credentials");
});

it("never writes oauth2_flow over the DCR authorization_code stamp", async () => {
it("re-writes the DCR authorization_code stamp with its own prefilled value", async () => {
const payload = await saveAndGetPayload({
oauth2_flow: "authorization_code",
token_url: "https://idp.example.com/oauth/token",
});
expect(payload).not.toHaveProperty("oauth2_flow");
expect(payload.oauth2_flow).toBe("authorization_code");
Comment thread
tin-berri marked this conversation as resolved.
});

it("persists client_credentials when the admin selects M2M on a legacy null-flow row", async () => {
vi.mocked(networking.updateMCPServer).mockResolvedValue({ ...interactiveOAuthServer });

render(
<MCPServerEdit
mcpServer={{
...interactiveOAuthServer,
token_url: "https://idp.example.com/oauth/token",
oauth2_flow: null,
}}
accessToken="access-token"
onCancel={vi.fn()}
onSuccess={vi.fn()}
availableAccessGroups={[]}
/>,
);

await selectAntOption("OAuth Flow Type", "Machine-to-Machine (M2M)");

const saveButtons = screen.getAllByRole("button", { name: "Save Changes" });
await act(async () => {
fireEvent.click(saveButtons[0]);
});

await waitFor(() => {
expect(networking.updateMCPServer).toHaveBeenCalledTimes(1);
});

const [, payload] = vi.mocked(networking.updateMCPServer).mock.calls[0];
expect(payload.oauth2_flow).toBe("client_credentials");
});

it("persists authorization_code when the admin selects Interactive on a legacy null-flow row", async () => {
vi.mocked(networking.updateMCPServer).mockResolvedValue({ ...interactiveOAuthServer });

render(
<MCPServerEdit
mcpServer={{
...interactiveOAuthServer,
token_url: "https://idp.example.com/oauth/token",
oauth2_flow: null,
}}
accessToken="access-token"
onCancel={vi.fn()}
onSuccess={vi.fn()}
availableAccessGroups={[]}
/>,
);

await selectAntOption("OAuth Flow Type", "Interactive (PKCE)");

const saveButtons = screen.getAllByRole("button", { name: "Save Changes" });
await act(async () => {
fireEvent.click(saveButtons[0]);
});

await waitFor(() => {
expect(networking.updateMCPServer).toHaveBeenCalledTimes(1);
});

const [, payload] = vi.mocked(networking.updateMCPServer).mock.calls[0];
expect(payload.oauth2_flow).toBe("authorization_code");
});
});

describe("MCPServerEdit OAuth flow prefill display", () => {
beforeEach(() => {
vi.clearAllMocks();
});

function renderEdit(server: Record<string, unknown>) {
render(
<MCPServerEdit
mcpServer={{ ...interactiveOAuthServer, ...server }}
accessToken="access-token"
onCancel={vi.fn()}
onSuccess={vi.fn()}
availableAccessGroups={[]}
/>,
);
}

it("shows the placeholder and preselects nothing for a null-flow server (prompts the user to define it)", () => {
renderEdit({ oauth2_flow: null, token_url: "https://idp.example.com/oauth/token" });

// The select renders its placeholder (undefined value), not a guessed option.
expect(screen.getByText("Select OAuth flow")).toBeInTheDocument();
// Neither flow is preselected as the current value.
expect(screen.queryByText("Machine-to-Machine (M2M)")).not.toBeInTheDocument();
expect(screen.queryByText("Interactive (PKCE)")).not.toBeInTheDocument();
});

it("prefills Machine-to-Machine (M2M) for a stored client_credentials server", () => {
renderEdit({ oauth2_flow: "client_credentials" });

expect(screen.getByText("Machine-to-Machine (M2M)")).toBeInTheDocument();
expect(screen.queryByText("Select OAuth flow")).not.toBeInTheDocument();
});

it("prefills Interactive (PKCE) for a stored authorization_code server", () => {
renderEdit({ oauth2_flow: "authorization_code" });

expect(screen.getByText("Interactive (PKCE)")).toBeInTheDocument();
expect(screen.queryByText("Select OAuth flow")).not.toBeInTheDocument();
});

it("warns when a server has no OAuth flow set", () => {
renderEdit({ oauth2_flow: null, token_url: "https://idp.example.com/oauth/token" });

expect(screen.getByText("This server has no OAuth flow set")).toBeInTheDocument();
});

it("does not warn when the flow is already set", () => {
renderEdit({ oauth2_flow: "client_credentials" });

expect(screen.queryByText("This server has no OAuth flow set")).not.toBeInTheDocument();
});

it("does not warn for a delegate (PKCE passthrough) server even with no flow set", () => {
renderEdit({ oauth2_flow: null, delegate_auth_to_upstream: true });

expect(screen.queryByText("This server has no OAuth flow set")).not.toBeInTheDocument();
});

it("clears the warning once the admin selects a flow", async () => {
renderEdit({ oauth2_flow: null, token_url: "https://idp.example.com/oauth/token" });

expect(screen.getByText("This server has no OAuth flow set")).toBeInTheDocument();

await selectAntOption("OAuth Flow Type", "Machine-to-Machine (M2M)");

await waitFor(() => {
expect(screen.queryByText("This server has no OAuth flow set")).not.toBeInTheDocument();
});
});
});
Loading
Loading