From b304620311b3f449b84d37d20ebdc84cf8d4cb20 Mon Sep 17 00:00:00 2001 From: Tin Date: Thu, 9 Jul 2026 14:33:51 -0700 Subject: [PATCH] fix(ui): compare url and spec_path independently in the OAuth authorization identity The identity used to pick the audience from spec_path only when values.transport was OPENAPI, but the create form keeps transport in component state rather than form values, so spec_path edits on OpenAPI servers never invalidated a held token. Comparing url and spec_path independently mirrors the backend's mcp_oauth_token_identity and fires regardless of whether transport is present. Invalidation now also wipes only credentials; the admin-typed endpoint fields are kept --- .../mcp_tools/create_mcp_server.tsx | 3 +- .../mcp_tools/mcp_server_edit.test.tsx | 27 ++++++++++++++ .../src/components/mcp_tools/types.test.tsx | 27 ++++++++++++++ .../src/components/mcp_tools/types.tsx | 35 +++++++++++-------- 4 files changed, 77 insertions(+), 15 deletions(-) diff --git a/ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.tsx b/ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.tsx index 24b8e9eaafb..eb48fd02474 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.tsx @@ -239,7 +239,8 @@ const CreateMCPServer: React.FC = ({ // Discard the held browser-authorized token and its tool preview when the authorization identity // changes (or the modal closes). The CLEARED_ON_INVALIDATION form fields (shared with the edit form // via types.tsx) are reset too; whatever the admin just changed (passed via changedValues) is - // re-applied so the invalidation never wipes their in-flight edit. + // re-applied so the invalidation never wipes their in-flight edit. Admin-typed endpoint fields are + // left alone (see CLEARED_ON_INVALIDATION). const clearHeldOAuthToken = (changedValues: Record = {}) => { setOauthAccessToken(null); clearTools(); diff --git a/ui/litellm-dashboard/src/components/mcp_tools/mcp_server_edit.test.tsx b/ui/litellm-dashboard/src/components/mcp_tools/mcp_server_edit.test.tsx index 4f3d7b69b01..e5daf90e992 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/mcp_server_edit.test.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/mcp_server_edit.test.tsx @@ -511,6 +511,33 @@ describe("MCPServerEdit OAuth token invalidation", () => { expect(mockRemoveToken).toHaveBeenCalledWith("oauth_server_1", undefined); }); + it("keeps the admin's in-flight endpoint edits when the token is invalidated", async () => { + // Regression: invalidation used to form.resetFields the endpoint fields; with the edit Form's + // initialValues that silently reverted an admin-corrected token_url back to the saved (wrong) + // value while still looking plausible. Only credentials (the minted material) may be wiped. + renderOAuthEdit(); + + const tokenUrlInput = screen.getByPlaceholderText("https://example.com/oauth/token"); + await act(async () => { + fireEvent.change(tokenUrlInput, { target: { value: "https://corrected.example.com/token" } }); + }); + + act(() => { + mockOauth.onTokenReceived?.({ access_token: "tok-1" }); + }); + mockOauth.reset.mockClear(); + + const urlInput = screen.getByPlaceholderText("https://your-mcp-server.com"); + await act(async () => { + fireEvent.change(urlInput, { target: { value: "https://moved.example.com/mcp" } }); + }); + + await waitFor(() => expect(mockOauth.reset).toHaveBeenCalled()); + expect((screen.getByPlaceholderText("https://example.com/oauth/token") as HTMLInputElement).value).toBe( + "https://corrected.example.com/token", + ); + }); + it("keeps a session-authorized token on an http to sse switch with the same url", async () => { // Same url means the same resource/audience (RFC 8707): the minted token is still valid, so a // pure transport swap between the two MCP wire protocols must not force a re-authorize. diff --git a/ui/litellm-dashboard/src/components/mcp_tools/types.test.tsx b/ui/litellm-dashboard/src/components/mcp_tools/types.test.tsx index 846bcfc9e0b..c6faca1fb51 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/types.test.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/types.test.tsx @@ -7,9 +7,36 @@ import { handleTransport, handleAuth, getMcpOAuthMode, + getOAuthAuthorizationIdentity, + isHeldOAuthTokenStale, oauth2FlowToFormValue, } from "./types"; +describe("getOAuthAuthorizationIdentity", () => { + // Regression: the identity used to pick the audience from spec_path only when values.transport was + // OPENAPI, but the create form keeps transport in component state, so values.transport was absent and + // spec_path edits on OpenAPI servers never invalidated a held token. + it("changes when spec_path changes even when transport is absent from form values", () => { + const authorized = { auth_type: AUTH_TYPE.OAUTH2, spec_path: "https://a.example.com/openapi.json" }; + const edited = { auth_type: AUTH_TYPE.OAUTH2, spec_path: "https://b.example.com/openapi.json" }; + expect(getOAuthAuthorizationIdentity(edited)).not.toBe(getOAuthAuthorizationIdentity(authorized)); + expect(isHeldOAuthTokenStale(edited, getOAuthAuthorizationIdentity(authorized))).toBe(true); + }); + + it("changes when url changes", () => { + const authorized = { auth_type: AUTH_TYPE.OAUTH2, url: "https://a.example.com/mcp" }; + const edited = { auth_type: AUTH_TYPE.OAUTH2, url: "https://b.example.com/mcp" }; + expect(getOAuthAuthorizationIdentity(edited)).not.toBe(getOAuthAuthorizationIdentity(authorized)); + }); + + it("is stable across non-mint fields", () => { + const authorized = { auth_type: AUTH_TYPE.OAUTH2, url: "https://a.example.com/mcp", server_name: "one" }; + const renamed = { auth_type: AUTH_TYPE.OAUTH2, url: "https://a.example.com/mcp", server_name: "two" }; + expect(getOAuthAuthorizationIdentity(renamed)).toBe(getOAuthAuthorizationIdentity(authorized)); + expect(isHeldOAuthTokenStale(renamed, getOAuthAuthorizationIdentity(authorized))).toBe(false); + }); +}); + describe("handleTransport", () => { it("should default to SSE when transport is null", () => { expect(handleTransport(null)).toBe(TRANSPORT.SSE); diff --git a/ui/litellm-dashboard/src/components/mcp_tools/types.tsx b/ui/litellm-dashboard/src/components/mcp_tools/types.tsx index e894cf4b8dc..3eba8b30968 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/types.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/types.tsx @@ -58,20 +58,24 @@ export const OAUTH_FLOW = { }; // The fields that determine which upstream OAuth token "Authorize & Fetch" mints: the resource/audience -// (url), the OAuth mode/grant (auth_type, oauth_flow_type), the OAuth client and requested scope -// (credentials.client_id / client_secret / scopes), and the authorization-server endpoints -// (authorization_url / token_url / registration_url). Grounded in RFC 8707 / RFC 8693 and the MCP auth -// spec: an access token is bound to exactly this tuple (resource/audience + scope + client + issuer), so +// (url, or spec_path for OpenAPI servers), the OAuth mode/grant (auth_type, oauth_flow_type), the OAuth +// client and requested scope (credentials.client_id / client_secret / scopes), and the authorization-server +// endpoints (authorization_url / token_url / registration_url). Grounded in RFC 8707 / RFC 8693 and the MCP +// auth spec: an access token is bound to exactly this tuple (resource/audience + scope + client + issuer), so // a previously authorized token is stale if and only if this identity changes and must be re-minted. -// Deliberately EXCLUDES: transport (http<->sse on the same url is the same audience; a transport switch -// only matters when it changes the target url, which `target` already captures), delegate_auth_to_upstream -// (a downstream-usage toggle that is never sent to the authorize request), and all metadata/RBAC/routing -// fields. Shared by the create and edit forms so their invalidation logic cannot drift. +// url and spec_path are compared independently rather than selected by transport: the create form keeps +// transport in component state, not in form values, so a transport-conditional target would silently pin the +// audience to a missing url and never fire for spec_path edits on OpenAPI servers. Mirrors the backend's +// mcp_oauth_token_identity. Deliberately EXCLUDES: transport itself (http<->sse on the same url is the same +// audience; a switch to/from OpenAPI shows up as url/spec_path changes because each form clears the field the +// new transport does not use), delegate_auth_to_upstream (a downstream-usage toggle that is never sent to the +// authorize request), and all metadata/RBAC/routing fields. Shared by the create and edit forms so their +// invalidation logic cannot drift. export const getOAuthAuthorizationIdentity = (values: Record): string => { const credentials = (values.credentials ?? {}) as Record; - const target = values.transport === TRANSPORT.OPENAPI ? values.spec_path : values.url; const identity = { - target: typeof target === "string" ? target : null, + url: typeof values.url === "string" ? values.url : null, + spec_path: typeof values.spec_path === "string" ? values.spec_path : null, auth_type: values.auth_type ?? null, oauth_flow_type: values.oauth_flow_type ?? null, client_id: credentials.client_id ?? null, @@ -84,10 +88,13 @@ export const getOAuthAuthorizationIdentity = (values: Record): return JSON.stringify(identity); }; -// The form fields wiped when a held OAuth token is invalidated: the fetched token + DCR client live in -// `credentials`, and the three endpoint fields were discovered by the authorize flow, so all of them are -// stale together with the token. Shared by the create and edit forms so what gets wiped cannot drift. -export const CLEARED_ON_INVALIDATION = ["credentials", "authorization_url", "token_url", "registration_url"] as const; +// The form fields wiped when a held OAuth token is invalidated: only `credentials`, which holds the +// minted material (the fetched token + DCR client). The authorization/token/registration endpoint +// fields are deliberately NOT wiped: nothing programmatic ever writes them (upstream discovery happens +// backend-side), so they only ever hold admin input, and resetting them would wipe it (create) or +// silently revert it to the saved record (edit, whose Form has initialValues). Shared by the create and +// edit forms so what gets wiped cannot drift. +export const CLEARED_ON_INVALIDATION = ["credentials"] as const; // 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