-
-
Notifications
You must be signed in to change notification settings - Fork 11.2k
fix(ui/mcp): reset OAuth state on create-server modal close so a prior server's token no longer leaks into the next add-server session #30000
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -72,9 +72,9 @@ | |
| const [form] = Form.useForm(); | ||
| const [isLoading, setIsLoading] = useState(false); | ||
| const [costConfig, setCostConfig] = useState<MCPServerCostInfo>({}); | ||
| const [formValues, setFormValues] = useState<Record<string, any>>({}); | ||
| const [pendingRestoredValues, setPendingRestoredValues] = useState<{ | ||
| values: Record<string, any>; | ||
| transport?: string; | ||
| } | null>(null); | ||
| const [aliasManuallyEdited, setAliasManuallyEdited] = useState(false); | ||
|
|
@@ -134,6 +134,7 @@ | |
| status: oauthStatus, | ||
| error: oauthError, | ||
| tokenResponse: oauthTokenResponse, | ||
| reset: resetOAuthFlow, | ||
| } = useMcpOAuthFlow({ | ||
| accessToken, | ||
| getCredentials: () => form.getFieldValue("credentials"), | ||
|
|
@@ -265,7 +266,7 @@ | |
| const transport = prefillData.transport || ""; | ||
| setTransportType(transport); | ||
|
|
||
| const prefillValues: Record<string, any> = { | ||
| server_name: sanitizedName, | ||
| alias: sanitizedName, | ||
| description: prefillData.description || "", | ||
|
|
@@ -273,7 +274,7 @@ | |
| }; | ||
|
|
||
| if (transport === "stdio") { | ||
| const stdioObj: Record<string, any> = {}; | ||
| if (prefillData.command) stdioObj.command = prefillData.command; | ||
| if (prefillData.args && prefillData.args.length > 0) stdioObj.args = prefillData.args; | ||
| if (prefillData.env_vars && prefillData.env_vars.length > 0) { | ||
|
|
@@ -295,7 +296,7 @@ | |
| setAliasManuallyEdited(false); | ||
| }, [isModalVisible, prefillData, form]); | ||
|
|
||
| const handleCreate = async (values: Record<string, any>) => { | ||
|
Check warning on line 299 in ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.tsx
|
||
| setIsLoading(true); | ||
| try { | ||
| const { | ||
|
|
@@ -319,7 +320,7 @@ | |
|
|
||
| const credentialsPayload = | ||
| credentialValues && typeof credentialValues === "object" | ||
| ? Object.entries(credentialValues).reduce((acc: Record<string, any>, [key, value]) => { | ||
| if (value === undefined || value === null || value === "") { | ||
| return acc; | ||
| } | ||
|
|
@@ -352,12 +353,12 @@ | |
| // If it's the full mcpServers structure, extract the first server config | ||
| if (stdioConfig.mcpServers && typeof stdioConfig.mcpServers === "object") { | ||
| const serverNames = Object.keys(stdioConfig.mcpServers); | ||
| if (serverNames.length > 0) { | ||
| const firstServerName = serverNames[0]; | ||
| actualConfig = stdioConfig.mcpServers[firstServerName]; | ||
|
|
||
| // If no alias is provided, use the server name from the JSON | ||
| if (!restValues.server_name) { | ||
| restValues.server_name = firstServerName.replace(/-/g, "_"); // Replace hyphens with underscores | ||
| } | ||
| } | ||
|
|
@@ -554,12 +555,19 @@ | |
| } | ||
| }, [formValues.server_name]); | ||
|
|
||
| // Clear formValues when modal closes to reset child components | ||
| // Clear form, tools, and OAuth state when the modal closes so a previous server's | ||
| // authorization, credentials, or tool list never bleed into the next "Add New MCP | ||
| // Server" session, including when a parent dismisses the modal without routing | ||
| // through handleCancel or handleCreate. | ||
| React.useEffect(() => { | ||
| if (!isModalVisible) { | ||
| form.resetFields(); | ||
| setFormValues({}); | ||
| setOauthAccessToken(null); | ||
| clearTools(); | ||
| resetOAuthFlow(); | ||
| } | ||
| }, [isModalVisible]); | ||
| }, [isModalVisible, form, clearTools, resetOAuthFlow]); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Stale transport state after modal resetMedium Severity The modal-close effect calls Reviewed by Cursor Bugbot for commit 0e523f5. Configure here. |
||
|
|
||
| const isAdmin = isAdminRole(userRole); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,117 @@ | ||
| import { act, renderHook, waitFor } from "@testing-library/react"; | ||
| import { beforeEach, describe, expect, it, vi } from "vitest"; | ||
| import * as networking from "@/components/networking"; | ||
| import { setSecureItem } from "@/utils/secureStorage"; | ||
| import { useMcpOAuthFlow } from "./useMcpOAuthFlow"; | ||
|
|
||
| vi.mock("@/components/networking", () => ({ | ||
| exchangeMcpOAuthToken: vi.fn(), | ||
| cacheTemporaryMcpServer: vi.fn(), | ||
| registerMcpOAuthClient: vi.fn(), | ||
| buildMcpOAuthAuthorizeUrl: vi.fn(), | ||
| getProxyBaseUrl: vi.fn(() => ""), | ||
| serverRootPath: "", | ||
| })); | ||
|
|
||
| vi.mock("@/components/molecules/notifications_manager", () => ({ | ||
| default: { success: vi.fn(), error: vi.fn() }, | ||
| })); | ||
|
|
||
| const FLOW_STATE_KEY = "litellm-mcp-oauth-flow-state"; | ||
| const RESULT_KEY = "litellm-mcp-oauth-result"; | ||
|
|
||
| /** Seed the redirect result (the code returned by the IdP callback). */ | ||
| function seedResult(code: string) { | ||
| setSecureItem(RESULT_KEY, JSON.stringify({ state: "state-1", code })); | ||
| } | ||
|
|
||
| /** Seed the flow state stored before the redirect. */ | ||
| function seedFlowState() { | ||
| setSecureItem( | ||
| FLOW_STATE_KEY, | ||
| JSON.stringify({ | ||
| state: "state-1", | ||
| codeVerifier: "verifier-1", | ||
| serverId: "server-1", | ||
| clientId: "client-1", | ||
| redirectUri: "https://app.example.com/ui/mcp/oauth/callback", | ||
| flowSource: "create", | ||
| }), | ||
| ); | ||
| } | ||
|
|
||
| /** Seed storage so the hook's on-mount resume flow exchanges a code for a token. */ | ||
| function seedCompletedRedirect() { | ||
| seedResult("code-1"); | ||
| seedFlowState(); | ||
| } | ||
|
|
||
| function renderFlow(onTokenReceived = vi.fn()) { | ||
| return renderHook( | ||
| ({ onTokenReceived: cb }: { onTokenReceived: (t: any) => void }) => | ||
| useMcpOAuthFlow({ | ||
| accessToken: "admin-token", | ||
| getCredentials: () => ({}), | ||
| getTemporaryPayload: () => ({ url: "https://server-1.example.com/mcp", transport: "http" }), | ||
| onTokenReceived: cb, | ||
| flowSource: "create", | ||
| }), | ||
| { initialProps: { onTokenReceived } }, | ||
| ); | ||
| } | ||
|
|
||
| describe("useMcpOAuthFlow reset", () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| window.sessionStorage.clear(); | ||
| window.localStorage.clear(); | ||
| }); | ||
|
|
||
| it("clears a successfully fetched token so it cannot leak into the next session", async () => { | ||
| const token = { access_token: "tok-123", expires_in: 3600 }; | ||
| vi.mocked(networking.exchangeMcpOAuthToken).mockResolvedValue(token); | ||
| seedCompletedRedirect(); | ||
|
|
||
| const onTokenReceived = vi.fn(); | ||
| const { result } = renderFlow(onTokenReceived); | ||
|
|
||
| await waitFor(() => expect(result.current.status).toBe("success")); | ||
| expect(result.current.tokenResponse).toEqual(token); | ||
| expect(onTokenReceived).toHaveBeenCalledWith(token); | ||
|
|
||
| act(() => { | ||
| result.current.reset(); | ||
| }); | ||
|
|
||
| expect(result.current.status).toBe("idle"); | ||
| expect(result.current.tokenResponse).toBeNull(); | ||
| expect(result.current.error).toBeNull(); | ||
| }); | ||
|
|
||
| it("clears the in-flight guard so a callback after a mid-exchange close is not swallowed", async () => { | ||
| // First exchange hangs, mimicking the modal being closed while the token | ||
| // endpoint is still in flight. processingRef is left true at that point. | ||
| vi.mocked(networking.exchangeMcpOAuthToken).mockReturnValueOnce(new Promise<any>(() => {})); | ||
| seedFlowState(); | ||
| seedResult("code-1"); | ||
|
|
||
| const onTokenReceived1 = vi.fn(); | ||
| const { result, rerender } = renderFlow(onTokenReceived1); | ||
|
|
||
| await waitFor(() => expect(result.current.status).toBe("exchanging")); | ||
|
|
||
| act(() => { | ||
| result.current.reset(); | ||
| }); | ||
|
|
||
| // The reopened modal receives a fresh callback; it must be processed, not | ||
| // dropped by a stale in-flight guard. | ||
| const token = { access_token: "tok-2" }; | ||
| vi.mocked(networking.exchangeMcpOAuthToken).mockResolvedValueOnce(token); | ||
| seedResult("code-2"); | ||
| const onTokenReceived2 = vi.fn(); | ||
| rerender({ onTokenReceived: onTokenReceived2 }); | ||
|
|
||
| await waitFor(() => expect(onTokenReceived2).toHaveBeenCalledWith(token)); | ||
| }); | ||
| }); |


Uh oh!
There was an error while loading. Please reload this page.