mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-11 22:51:28 +00:00
fix(mcp): keep admin-declared app credentials through OAuth invalidation for the client-forwarded modes
This commit is contained in:
parent
931b617a51
commit
57051d36d6
6 changed files with 202 additions and 1 deletions
|
|
@ -1,7 +1,7 @@
|
|||
{
|
||||
"@typescript-eslint/no-explicit-any": 1978,
|
||||
"complexity": 129,
|
||||
"local/no-large-inline-object-arg": 512,
|
||||
"local/no-large-inline-object-arg": 514,
|
||||
"local/no-long-condition-chain": 233,
|
||||
"max-depth": 59,
|
||||
"no-console": 15
|
||||
|
|
|
|||
|
|
@ -503,6 +503,110 @@ describe("CreateMCPServer", () => {
|
|||
},
|
||||
);
|
||||
|
||||
it("preserves admin-entered app credentials when the URL changes after authorize for true_passthrough", async () => {
|
||||
await selectHttpTransport();
|
||||
|
||||
const user = userEvent.setup({ delay: null });
|
||||
await user.type(getServerNameInput(), "CF_Keep_Server");
|
||||
await user.type(screen.getByPlaceholderText("https://your-mcp-server.com"), "https://example.com/mcp");
|
||||
|
||||
await selectAntOption("Authentication", "True Passthrough (no LiteLLM auth)");
|
||||
|
||||
await user.type(
|
||||
screen.getByPlaceholderText("Leave blank to use dynamic client registration"),
|
||||
"org-app-client-id",
|
||||
);
|
||||
await user.type(screen.getByPlaceholderText("Leave blank for public clients / PKCE"), "org-app-secret");
|
||||
|
||||
await waitFor(() => expect(oauthHook.onTokenReceived).toBeTruthy());
|
||||
await act(async () => {
|
||||
oauthHook.onTokenReceived!({ access_token: "upstream-tok", token_type: "Bearer" }, undefined);
|
||||
});
|
||||
|
||||
// Editing the URL after authorize invalidates the held token (identity change), but the
|
||||
// declared app is config, not minted material: it must survive the invalidation instead of
|
||||
// being silently reset, or the server would persist without the configured app.
|
||||
await act(async () => {
|
||||
fireEvent.change(screen.getByPlaceholderText("https://your-mcp-server.com"), {
|
||||
target: { value: "https://other.example.com/mcp" },
|
||||
});
|
||||
});
|
||||
|
||||
vi.mocked(networking.createMCPServer).mockResolvedValue({
|
||||
server_id: "kept-app-server",
|
||||
server_name: "CF_Keep_Server",
|
||||
alias: "CF_Keep_Server",
|
||||
url: "https://other.example.com/mcp",
|
||||
transport: "http",
|
||||
auth_type: "true_passthrough",
|
||||
created_at: "2024-01-01T00:00:00Z",
|
||||
created_by: "user-1",
|
||||
updated_at: "2024-01-01T00:00:00Z",
|
||||
updated_by: "user-1",
|
||||
});
|
||||
|
||||
await act(async () => {
|
||||
fireEvent.click(screen.getByRole("button", { name: "Add MCP Server" }));
|
||||
});
|
||||
|
||||
await waitFor(() => expect(networking.createMCPServer).toHaveBeenCalledTimes(1));
|
||||
const [, payload] = vi.mocked(networking.createMCPServer).mock.calls[0];
|
||||
expect(payload.url).toBe("https://other.example.com/mcp");
|
||||
expect(payload.credentials).toEqual({
|
||||
client_id: "org-app-client-id",
|
||||
client_secret: "org-app-secret",
|
||||
});
|
||||
expect(JSON.stringify(payload)).not.toContain("upstream-tok");
|
||||
});
|
||||
|
||||
it("wipes oauth2-minted credentials when the auth type switches to a client-forwarded mode", async () => {
|
||||
await selectHttpTransport();
|
||||
|
||||
const user = userEvent.setup({ delay: null });
|
||||
await user.type(getServerNameInput(), "Switch_Server");
|
||||
await user.type(screen.getByPlaceholderText("https://your-mcp-server.com"), "https://example.com/mcp");
|
||||
|
||||
await selectAntOption("Authentication", "OAuth");
|
||||
|
||||
// The oauth2 onTokenReceived branch writes the fetched token AND the DCR client into
|
||||
// form.credentials; both are minted for the oauth2 identity.
|
||||
await waitFor(() => expect(oauthHook.onTokenReceived).toBeTruthy());
|
||||
await act(async () => {
|
||||
oauthHook.onTokenReceived!(
|
||||
{ access_token: "oauth2-minted-tok", refresh_token: "oauth2-minted-refresh", token_type: "Bearer" },
|
||||
{ clientId: "dcr-minted-client", clientSecret: "dcr-minted-secret" },
|
||||
);
|
||||
});
|
||||
|
||||
// Switching into a client-forwarded mode changes the identity with auth_type in the changed
|
||||
// values, so the preserve carve-out must NOT apply: the minted material would otherwise ride
|
||||
// into a mode that now persists credentials onto the server row.
|
||||
await selectAntOption("Authentication", "True Passthrough (no LiteLLM auth)");
|
||||
|
||||
vi.mocked(networking.createMCPServer).mockResolvedValue({
|
||||
server_id: "switched-server",
|
||||
server_name: "Switch_Server",
|
||||
alias: "Switch_Server",
|
||||
url: "https://example.com/mcp",
|
||||
transport: "http",
|
||||
auth_type: "true_passthrough",
|
||||
created_at: "2024-01-01T00:00:00Z",
|
||||
created_by: "user-1",
|
||||
updated_at: "2024-01-01T00:00:00Z",
|
||||
updated_by: "user-1",
|
||||
});
|
||||
|
||||
await act(async () => {
|
||||
fireEvent.click(screen.getByRole("button", { name: "Add MCP Server" }));
|
||||
});
|
||||
|
||||
await waitFor(() => expect(networking.createMCPServer).toHaveBeenCalledTimes(1));
|
||||
const [, payload] = vi.mocked(networking.createMCPServer).mock.calls[0];
|
||||
expect(payload.credentials).toBeUndefined();
|
||||
expect(JSON.stringify(payload)).not.toContain("dcr-minted-client");
|
||||
expect(JSON.stringify(payload)).not.toContain("oauth2-minted-tok");
|
||||
});
|
||||
|
||||
it("should not show auth value field when None auth type is selected", async () => {
|
||||
await selectHttpTransport();
|
||||
|
||||
|
|
|
|||
|
|
@ -18,6 +18,7 @@ import {
|
|||
getOAuthAuthorizationIdentity,
|
||||
CLEARED_ON_INVALIDATION,
|
||||
isHeldOAuthTokenStale,
|
||||
preservedDeclaredAppCredentials,
|
||||
} from "./types";
|
||||
import OAuthFormFields from "./OAuthFormFields";
|
||||
import TruePassthroughWarning from "./TruePassthroughWarning";
|
||||
|
|
@ -248,7 +249,15 @@ const CreateMCPServer: React.FC<CreateMCPServerProps> = ({
|
|||
clearTools();
|
||||
resetOAuthFlow();
|
||||
setAuthorizedIdentity(undefined);
|
||||
const keptAppCredentials = preservedDeclaredAppCredentials(
|
||||
form.getFieldValue("auth_type"),
|
||||
"auth_type" in changedValues,
|
||||
form.getFieldValue("credentials"),
|
||||
);
|
||||
form.resetFields([...CLEARED_ON_INVALIDATION]);
|
||||
if (keptAppCredentials) {
|
||||
form.setFieldsValue({ credentials: keptAppCredentials });
|
||||
}
|
||||
const preserved = Object.fromEntries(
|
||||
CLEARED_ON_INVALIDATION.filter((key) => key in changedValues).map((key) => [key, changedValues[key]]),
|
||||
);
|
||||
|
|
|
|||
|
|
@ -1423,6 +1423,60 @@ describe("MCPServerEdit (OAuth token persistence on save)", () => {
|
|||
},
|
||||
);
|
||||
|
||||
it.each([["true_passthrough"], ["oauth_delegate"]])(
|
||||
"preserves admin-entered app credentials when the URL changes after authorize for the %s mode",
|
||||
async (authType) => {
|
||||
vi.mocked(networking.updateMCPServer).mockResolvedValue({
|
||||
...interactiveOAuthServer,
|
||||
auth_type: authType,
|
||||
});
|
||||
|
||||
render(
|
||||
<MCPServerEdit
|
||||
mcpServer={{ ...interactiveOAuthServer, auth_type: authType }}
|
||||
accessToken="access-token"
|
||||
userID="user-1"
|
||||
onCancel={vi.fn()}
|
||||
onSuccess={vi.fn()}
|
||||
availableAccessGroups={[]}
|
||||
/>,
|
||||
);
|
||||
|
||||
const user = userEvent.setup({ delay: null });
|
||||
await user.type(
|
||||
screen.getByPlaceholderText("Leave blank to use dynamic client registration"),
|
||||
"org-app-client-id",
|
||||
);
|
||||
await user.type(screen.getByPlaceholderText("Leave blank for public clients / PKCE"), "org-app-secret");
|
||||
|
||||
act(() => {
|
||||
mockOauth.onTokenReceived?.({ access_token: "cf-tok", token_type: "bearer" });
|
||||
});
|
||||
|
||||
// The URL edit invalidates the held browser token (removeToken fires), but the declared app
|
||||
// is config and must survive the invalidation into the update payload.
|
||||
await act(async () => {
|
||||
fireEvent.change(screen.getByPlaceholderText("https://your-mcp-server.com"), {
|
||||
target: { value: "https://other.example.com/mcp" },
|
||||
});
|
||||
});
|
||||
expect(mockRemoveToken).toHaveBeenCalledWith("oauth_server_1", "user-1");
|
||||
|
||||
await act(async () => {
|
||||
fireEvent.click(screen.getAllByRole("button", { name: "Save Changes" })[0]);
|
||||
});
|
||||
|
||||
await waitFor(() => expect(networking.updateMCPServer).toHaveBeenCalledTimes(1));
|
||||
const [, payload] = vi.mocked(networking.updateMCPServer).mock.calls[0];
|
||||
expect(payload.url).toBe("https://other.example.com/mcp");
|
||||
expect(payload.credentials).toMatchObject({
|
||||
client_id: "org-app-client-id",
|
||||
client_secret: "org-app-secret",
|
||||
});
|
||||
expect(JSON.stringify(payload)).not.toContain("cf-tok");
|
||||
},
|
||||
);
|
||||
|
||||
it("forwards a newly authorized browser-held token for tool loading before the form is saved", async () => {
|
||||
// Regression: fetchTools keyed the browser-held decision off the saved mcpServer.auth_type, so
|
||||
// after switching the form to true_passthrough and authorizing, the fresh token was not sent as
|
||||
|
|
|
|||
|
|
@ -8,6 +8,7 @@ import {
|
|||
getOAuthAuthorizationIdentity,
|
||||
CLEARED_ON_INVALIDATION,
|
||||
isHeldOAuthTokenStale,
|
||||
preservedDeclaredAppCredentials,
|
||||
OAUTH_FLOW,
|
||||
MCP_OAUTH2_FLOW_M2M,
|
||||
MCP_OAUTH2_FLOW_INTERACTIVE,
|
||||
|
|
@ -409,7 +410,15 @@ const MCPServerEdit: React.FC<MCPServerEditProps> = ({
|
|||
}
|
||||
setTools([]);
|
||||
resetOAuthFlow();
|
||||
const keptAppCredentials = preservedDeclaredAppCredentials(
|
||||
getEffectiveAuthType(),
|
||||
"auth_type" in changedValues,
|
||||
form.getFieldValue("credentials"),
|
||||
);
|
||||
form.resetFields([...CLEARED_ON_INVALIDATION]);
|
||||
if (keptAppCredentials) {
|
||||
form.setFieldsValue({ credentials: keptAppCredentials });
|
||||
}
|
||||
const preserved = Object.fromEntries(
|
||||
CLEARED_ON_INVALIDATION.filter((key) => key in changedValues).map((key) => [key, changedValues[key]]),
|
||||
);
|
||||
|
|
|
|||
|
|
@ -96,6 +96,31 @@ export const getOAuthAuthorizationIdentity = (values: Record<string, unknown>):
|
|||
// edit forms so what gets wiped cannot drift.
|
||||
export const CLEARED_ON_INVALIDATION = ["credentials"] as const;
|
||||
|
||||
// The carve-out to the wipe above for the client-forwarded token modes: their onTokenReceived branch
|
||||
// never writes minted material into form.credentials, so for them the field only ever holds the
|
||||
// admin-DECLARED upstream app (persisted as server config since the modes joined
|
||||
// AUTH_TYPES_REQUIRING_CREDENTIALS), and an intra-mode identity change (e.g. a URL edit after
|
||||
// Authorize) must not silently discard it. Two guards make the preserve safe: it never applies when
|
||||
// auth_type itself changed (the previous mode's onTokenReceived may have written a fetched token or
|
||||
// DCR client into the same field, and those are minted for the old mode), and it only ever keeps the
|
||||
// declared-app keys, so token-shaped keys can never ride through a preserve. Shared by the create and
|
||||
// edit forms so the carve-out cannot drift.
|
||||
const DECLARED_APP_CREDENTIAL_KEYS = ["client_id", "client_secret"] as const;
|
||||
|
||||
export const preservedDeclaredAppCredentials = (
|
||||
authType: string | null | undefined,
|
||||
authTypeChanged: boolean,
|
||||
credentials: Record<string, unknown> | null | undefined,
|
||||
): Record<string, string> | undefined => {
|
||||
if (!isClientForwardedTokenMode(authType) || authTypeChanged || !credentials) return undefined;
|
||||
const kept = Object.fromEntries(
|
||||
DECLARED_APP_CREDENTIAL_KEYS.filter((key) => typeof credentials[key] === "string" && credentials[key] !== "").map(
|
||||
(key) => [key, credentials[key] as string],
|
||||
),
|
||||
);
|
||||
return Object.keys(kept).length > 0 ? kept : undefined;
|
||||
};
|
||||
|
||||
// True when a token was authorized in this session (authorizedIdentity recorded at mint time) and the
|
||||
// form's current identity no longer matches it. Every invalidation decision in both forms goes through
|
||||
// this single check: onValuesChange for user edits, and an explicit recheck after any programmatic
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue