mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-10 22:41:41 +00:00
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
This commit is contained in:
parent
aa351311c0
commit
b304620311
4 changed files with 77 additions and 15 deletions
|
|
@ -239,7 +239,8 @@ const CreateMCPServer: React.FC<CreateMCPServerProps> = ({
|
|||
// 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<string, unknown> = {}) => {
|
||||
setOauthAccessToken(null);
|
||||
clearTools();
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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, unknown>): string => {
|
||||
const credentials = (values.credentials ?? {}) as Record<string, unknown>;
|
||||
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<string, unknown>):
|
|||
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
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue