mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-11 03:38:38 +00:00
fix(mcp): show the upstream-mismatch warning on edit and return undefined from withoutMintedTokenCredentials so a restore never blanks a stored client
This commit is contained in:
parent
1e78e96c5d
commit
1be846680a
5 changed files with 67 additions and 7 deletions
|
|
@ -748,20 +748,23 @@ const CreateMCPServer: React.FC<CreateMCPServerProps> = ({
|
|||
// form's post-reset state (not the pre-reset snapshot, which still holds the discarded token).
|
||||
// Editing the client fields is the admin managing/acknowledging the app, so it always dismisses
|
||||
// the "may not match upstream" warning regardless of the stale-token branch below.
|
||||
// Editing the client fields is the admin managing/acknowledging the app, so it dismisses the "may
|
||||
// not match upstream" warning. Otherwise a url/endpoint change while a declared app is present keeps
|
||||
// the app but flags that it may not match the new upstream (the "keep + warn" behavior). This is
|
||||
// independent of the held-token stale check below so it fires even without an authorize this session.
|
||||
if ("credentials" in changedValues) {
|
||||
setAppMayNotMatchUpstream(false);
|
||||
}
|
||||
if (isHeldOAuthTokenStale(form.getFieldsValue(true), authorizedIdentity)) {
|
||||
// A url/endpoint change while a declared app is present keeps the app but flags that it may not
|
||||
// match the new upstream (the "keep + warn" behavior); a client-key edit is handled above.
|
||||
} else {
|
||||
const upstreamChanged = ["url", "spec_path", "authorization_url", "token_url", "registration_url"].some(
|
||||
(key) => key in changedValues,
|
||||
);
|
||||
const hasDeclaredApp = preservedDeclaredAppCredentials(form.getFieldValue("credentials")) !== undefined;
|
||||
clearHeldOAuthToken(changedValues);
|
||||
if (upstreamChanged && hasDeclaredApp && !("credentials" in changedValues)) {
|
||||
if (upstreamChanged && hasDeclaredApp) {
|
||||
setAppMayNotMatchUpstream(true);
|
||||
}
|
||||
}
|
||||
if (isHeldOAuthTokenStale(form.getFieldsValue(true), authorizedIdentity)) {
|
||||
clearHeldOAuthToken(changedValues);
|
||||
setFormValues(form.getFieldsValue(true));
|
||||
return;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1519,6 +1519,35 @@ describe("MCPServerEdit (OAuth token persistence on save)", () => {
|
|||
expect(payload.credentials).toEqual({ client_id: null, client_secret: null });
|
||||
});
|
||||
|
||||
it("warns that the saved app may not match after a URL change on a client-forwarded server", async () => {
|
||||
render(
|
||||
<MCPServerEdit
|
||||
mcpServer={{
|
||||
...interactiveOAuthServer,
|
||||
auth_type: "true_passthrough",
|
||||
credentials: { client_id: "stored-client" },
|
||||
}}
|
||||
accessToken="access-token"
|
||||
userID="user-1"
|
||||
onCancel={vi.fn()}
|
||||
onSuccess={vi.fn()}
|
||||
availableAccessGroups={[]}
|
||||
/>,
|
||||
);
|
||||
|
||||
// No warning until the upstream changes.
|
||||
expect(screen.queryByText(/registered for the previous upstream/)).not.toBeInTheDocument();
|
||||
|
||||
await act(async () => {
|
||||
fireEvent.change(screen.getByPlaceholderText("https://your-mcp-server.com"), {
|
||||
target: { value: "https://different.example.com/mcp" },
|
||||
});
|
||||
});
|
||||
|
||||
// Keep + warn parity with the create form: the stored app is kept, and the banner appears.
|
||||
expect(screen.getByText(/registered for the previous upstream/)).toBeInTheDocument();
|
||||
});
|
||||
|
||||
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
|
||||
|
|
|
|||
|
|
@ -79,6 +79,9 @@ const MCPServerEdit: React.FC<MCPServerEditProps> = ({
|
|||
const [searchValue, setSearchValue] = useState<string>("");
|
||||
const [aliasManuallyEdited, setAliasManuallyEdited] = useState(false);
|
||||
const [removeStoredApp, setRemoveStoredApp] = useState(false);
|
||||
// Set when the upstream identity (url/endpoints) changed while a declared app is present, so the
|
||||
// section warns that the saved app may not match the new upstream (the app is kept, not wiped).
|
||||
const [appMayNotMatchUpstream, setAppMayNotMatchUpstream] = useState(false);
|
||||
const [allowedTools, setAllowedTools] = useState<string[]>([]);
|
||||
const [hasToolAllowlistInteraction, setHasToolAllowlistInteraction] = useState(false);
|
||||
const [toolNameToDisplayName, setToolNameToDisplayName] = useState<Record<string, string>>({});
|
||||
|
|
@ -445,6 +448,21 @@ const MCPServerEdit: React.FC<MCPServerEditProps> = ({
|
|||
};
|
||||
|
||||
const handleFormValuesChange = (changedValues: Record<string, unknown>) => {
|
||||
// Editing the client fields dismisses the "may not match upstream" warning; otherwise a url/endpoint
|
||||
// change while a declared app is present keeps the app but flags that it may not match the new
|
||||
// upstream (the "keep + warn" behavior). Mirrors the create form; independent of the held-token
|
||||
// stale check so it fires even without an authorize this session (the stored app is for the old url).
|
||||
if ("credentials" in changedValues) {
|
||||
setAppMayNotMatchUpstream(false);
|
||||
} else {
|
||||
const upstreamChanged = ["url", "spec_path", "authorization_url", "token_url", "registration_url"].some(
|
||||
(key) => key in changedValues,
|
||||
);
|
||||
const hasDeclaredApp = preservedDeclaredAppCredentials(form.getFieldValue("credentials")) !== undefined;
|
||||
if (upstreamChanged && hasDeclaredApp) {
|
||||
setAppMayNotMatchUpstream(true);
|
||||
}
|
||||
}
|
||||
if (isHeldOAuthTokenStale(form.getFieldsValue(true), authorizedIdentityRef.current)) {
|
||||
clearHeldOAuthToken(changedValues);
|
||||
}
|
||||
|
|
@ -1086,6 +1104,7 @@ const MCPServerEdit: React.FC<MCPServerEditProps> = ({
|
|||
savedAuthType={mcpServer.auth_type}
|
||||
removeStoredApp={removeStoredApp}
|
||||
onRemoveStoredAppChange={setRemoveStoredApp}
|
||||
appMayNotMatchUpstream={appMayNotMatchUpstream}
|
||||
/>
|
||||
</>
|
||||
)}
|
||||
|
|
|
|||
|
|
@ -215,6 +215,12 @@ describe("withoutMintedTokenCredentials", () => {
|
|||
};
|
||||
expect(withoutMintedTokenCredentials(mixed)).toEqual({ client_id: "a", client_secret: "b", scopes: ["read"] });
|
||||
});
|
||||
|
||||
it("returns undefined (not {}) when only minted keys are present, so a restore never blanks the fields", () => {
|
||||
expect(withoutMintedTokenCredentials({ access_token: "t", refresh_token: "r", expires_in: 3600 })).toBeUndefined();
|
||||
// A declared client is always kept, so a stored client_id can never be overwritten with empty.
|
||||
expect(withoutMintedTokenCredentials({ client_id: "x", access_token: "t" })).toEqual({ client_id: "x" });
|
||||
});
|
||||
});
|
||||
|
||||
describe("credentialAuthClass", () => {
|
||||
|
|
|
|||
|
|
@ -128,9 +128,12 @@ export const withoutMintedTokenCredentials = (
|
|||
credentials: Record<string, unknown> | null | undefined,
|
||||
): Record<string, unknown> | undefined => {
|
||||
if (!credentials) return undefined;
|
||||
return Object.fromEntries(
|
||||
const kept = Object.fromEntries(
|
||||
Object.entries(credentials).filter(([key]) => !(MINTED_TOKEN_CREDENTIAL_KEYS as readonly string[]).includes(key)),
|
||||
);
|
||||
// Return undefined (not {}) when only minted keys were present, so a restore spreads `credentials:
|
||||
// undefined` (the fields keep their placeholder / keep-existing state) rather than blanking them.
|
||||
return Object.keys(kept).length > 0 ? kept : undefined;
|
||||
};
|
||||
|
||||
// The client-forwarded modes share one credential class (same declared app, same authorize relay), so
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue