fix(mcp): rework the credential-field lifecycle so the OAuth app is upstream-scoped

This commit is contained in:
Tin 2026-07-10 13:03:10 -07:00
parent 9bff278efe
commit a2f8e80a7b
8 changed files with 531 additions and 63 deletions

View file

@ -0,0 +1,54 @@
import React from "react";
import { describe, it, expect } from "vitest";
import { render, screen } from "@testing-library/react";
import { Form } from "antd";
import PassthroughAuthorizeSection from "./PassthroughAuthorizeSection";
const WithForm: React.FC<{ children: React.ReactNode }> = ({ children }) => {
const [form] = Form.useForm();
return <Form form={form}>{children}</Form>;
};
const noopFlow = { startOAuthFlow: () => {}, status: "idle", error: null, tokenResponse: null };
describe("PassthroughAuthorizeSection credential-class-aware copy", () => {
it("shows keep-existing copy when the credential class is unchanged (true_passthrough <-> oauth_delegate)", () => {
render(
<WithForm>
<PassthroughAuthorizeSection
authType="oauth_delegate"
oauthFlow={noopFlow}
isEditing
savedAuthType="true_passthrough"
/>
</WithForm>,
);
expect(screen.getByPlaceholderText("Leave blank to keep the currently saved app (if any)")).toBeInTheDocument();
expect(screen.getByPlaceholderText("Leave blank to keep the currently saved secret (if any)")).toBeInTheDocument();
});
it("shows the discard warning copy when switching from a different class (oauth2 -> true_passthrough)", () => {
render(
<WithForm>
<PassthroughAuthorizeSection
authType="true_passthrough"
oauthFlow={noopFlow}
isEditing
savedAuthType="oauth2"
/>
</WithForm>,
);
expect(screen.getByPlaceholderText("Leave blank to use dynamic client registration")).toBeInTheDocument();
expect(screen.getByPlaceholderText("Leave blank for public clients / PKCE")).toBeInTheDocument();
expect(screen.getByText(/Switching the auth type discards the previously saved app/)).toBeInTheDocument();
});
it("shows the keep+warn banner when the upstream may no longer match", () => {
render(
<WithForm>
<PassthroughAuthorizeSection authType="true_passthrough" oauthFlow={noopFlow} appMayNotMatchUpstream />
</WithForm>,
);
expect(screen.getByText(/registered for the previous upstream/)).toBeInTheDocument();
});
});

View file

@ -1,6 +1,6 @@
import React from "react";
import { Button, Checkbox, Form, Input } from "antd";
import { isClientForwardedTokenMode } from "./types";
import { credentialAuthClass, isClientForwardedTokenMode } from "./types";
interface PassthroughOAuthFlow {
startOAuthFlow: () => void | Promise<void>;
@ -20,25 +20,31 @@ interface PassthroughOAuthFlow {
* as declared config, so internal users' Authorize relays through the org's
* app instead of dead-ending on upstreams that cannot mint clients.
*
* Blank fields follow the same convention as the M2M credential fields: on
* create they mean "no app configured" (dynamic client registration), while on
* edit the backend's partial update keeps whatever app is already stored, so
* blanks mean "keep existing". Removing a stored app is therefore an explicit
* action (the checkbox below, edit only), which saves an explicit-null
* credential write instead of omitting the field.
* Blank fields follow the same convention as the M2M credential fields. On
* create they mean "no app configured" (dynamic client registration). On edit
* they mean "keep existing" ONLY when the credential class is unchanged: the
* backend merges a partial update within the client-forwarded class, so a
* true_passthrough <-> oauth_delegate switch keeps the stored app, but a switch
* from a different class (e.g. oauth2) replaces it, so blanks then mean "no
* app". Removing a stored app is an explicit checkbox (edit only) that writes
* an explicit-null credential.
*/
export default function PassthroughAuthorizeSection({
authType,
oauthFlow,
isEditing = false,
savedAuthType,
removeStoredApp = false,
onRemoveStoredAppChange,
appMayNotMatchUpstream = false,
}: {
authType?: string | null;
oauthFlow: PassthroughOAuthFlow;
isEditing?: boolean;
savedAuthType?: string | null;
removeStoredApp?: boolean;
onRemoveStoredAppChange?: (remove: boolean) => void;
appMayNotMatchUpstream?: boolean;
}) {
if (!isClientForwardedTokenMode(authType)) return null;
const authorizeButtonLabels: Record<string, string> = {
@ -46,9 +52,18 @@ export default function PassthroughAuthorizeSection({
exchanging: "Exchanging authorization code...",
};
const authorizeButtonLabel = authorizeButtonLabels[oauthFlow.status] ?? "Authorize & Fetch Tools (browser-only)";
const blankMeaning = isEditing
// On edit, "keep existing" only holds when the stored credential class is unchanged; a cross-class
// switch (e.g. oauth2 -> true_passthrough) replaces credentials, so blanks then mean "no app".
const classUnchanged = isEditing && credentialAuthClass(savedAuthType) === credentialAuthClass(authType);
const clientIdPlaceholder = classUnchanged
? "Leave blank to keep the currently saved app (if any)"
: "Leave blank to use dynamic client registration";
const clientSecretPlaceholder = classUnchanged
? "Leave blank to keep the currently saved secret (if any)"
: "Leave blank for public clients / PKCE";
const clientIdExtra = classUnchanged
? "Set this to make everyone authorize through a specific app; required for upstreams without dynamic client registration (e.g. a pre-registered Slack app)."
: "Switching the auth type discards the previously saved app; enter a client ID here or leave blank to use dynamic client registration.";
return (
<div className="rounded-lg border border-dashed border-gray-300 p-4 space-y-2 mb-4">
<p className="text-sm text-gray-600">
@ -57,13 +72,19 @@ export default function PassthroughAuthorizeSection({
and is never saved to LiteLLM. An OAuth app configured below IS saved with the server, so internal users who
authorize from the Tools page go through it.
</p>
{appMayNotMatchUpstream && (
<p className="text-sm text-amber-600">
You changed the upstream URL or endpoints; the OAuth app entered here was registered for the previous upstream
and may not be valid. Update the client ID, or clear it to use dynamic client registration.
</p>
)}
<Form.Item
label={<span className="text-sm font-medium text-gray-700">OAuth Client ID (optional, saved)</span>}
name={["credentials", "client_id"]}
extra="Set this to make everyone authorize through a specific app; required for upstreams without dynamic client registration (e.g. a pre-registered Slack app)."
extra={clientIdExtra}
>
<Input.Password
placeholder={blankMeaning}
placeholder={clientIdPlaceholder}
disabled={removeStoredApp}
className="rounded-lg border-gray-300 focus:border-blue-500 focus:ring-blue-500"
/>
@ -73,7 +94,7 @@ export default function PassthroughAuthorizeSection({
name={["credentials", "client_secret"]}
>
<Input.Password
placeholder="Leave blank for public clients / PKCE"
placeholder={clientSecretPlaceholder}
disabled={removeStoredApp}
className="rounded-lg border-gray-300 focus:border-blue-500 focus:ring-blue-500"
/>

View file

@ -609,6 +609,226 @@ describe("CreateMCPServer", () => {
expect(JSON.stringify(payload)).not.toContain("oauth2-minted-tok");
});
it("keeps the DCR-minted client out of form.credentials but reuses it via getCredentials", async () => {
await selectHttpTransport();
const user = userEvent.setup({ delay: null });
await user.type(getServerNameInput(), "DCR_Server");
await user.type(screen.getByPlaceholderText("https://your-mcp-server.com"), "https://example.com/mcp");
await selectAntOption("Authentication", "OAuth");
await waitFor(() => expect(oauthHook.onTokenReceived).toBeTruthy());
await act(async () => {
oauthHook.onTokenReceived!(
{ access_token: "oauth2-tok", token_type: "Bearer" },
{ clientId: "dcr-client", clientSecret: "dcr-secret" },
);
});
// The DCR client must NOT be in the form store (or it could be collected as a CF server's app),
// but getCredentials merges it so a re-authorize reuses the registered client instead of re-DCRing.
expect(oauthHook.getCredentials?.()?.client_id).toBe("dcr-client");
});
it("persists the DCR client on an oauth2 submit via the ref", async () => {
await selectHttpTransport();
const user = userEvent.setup({ delay: null });
await user.type(getServerNameInput(), "DCR_Submit_Server");
await user.type(screen.getByPlaceholderText("https://your-mcp-server.com"), "https://example.com/mcp");
await selectAntOption("Authentication", "OAuth");
await waitFor(() => expect(oauthHook.onTokenReceived).toBeTruthy());
await act(async () => {
oauthHook.onTokenReceived!(
{ access_token: "oauth2-tok", token_type: "Bearer" },
{ clientId: "dcr-client", clientSecret: "dcr-secret" },
);
});
const dcrSubmitServer = {
server_id: "dcr-submit",
server_name: "DCR_Submit_Server",
alias: "DCR_Submit_Server",
url: "https://example.com/mcp",
transport: "http",
auth_type: "oauth2",
created_at: "2024-01-01T00:00:00Z",
created_by: "user-1",
updated_at: "2024-01-01T00:00:00Z",
updated_by: "user-1",
};
vi.mocked(networking.createMCPServer).mockResolvedValue(dcrSubmitServer);
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.client_id).toBe("dcr-client");
expect(payload.credentials.client_secret).toBe("dcr-secret");
});
it("preserves the typed app across a switch between the two client-forwarded modes", async () => {
await selectHttpTransport();
const user = userEvent.setup({ delay: null });
await user.type(getServerNameInput(), "CF_Switch_Keep");
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"), "app-id");
await user.type(screen.getByPlaceholderText("Leave blank for public clients / PKCE"), "app-secret");
await waitFor(() => expect(oauthHook.onTokenReceived).toBeTruthy());
await act(async () => {
oauthHook.onTokenReceived!({ access_token: "cf-tok", token_type: "Bearer" }, undefined);
});
await selectAntOption("Authentication", "OAuth Delegate (client-supplied upstream token)");
const switched = {
server_id: "cf-switch-keep",
server_name: "CF_Switch_Keep",
alias: "CF_Switch_Keep",
url: "https://example.com/mcp",
transport: "http",
auth_type: "oauth_delegate",
created_at: "2024-01-01T00:00:00Z",
created_by: "user-1",
updated_at: "2024-01-01T00:00:00Z",
updated_by: "user-1",
};
vi.mocked(networking.createMCPServer).mockResolvedValue(switched);
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).toEqual({ client_id: "app-id", client_secret: "app-secret" });
});
it("preserves the typed app across a client-forwarded -> oauth2 -> client-forwarded round trip", async () => {
await selectHttpTransport();
const user = userEvent.setup({ delay: null });
await user.type(getServerNameInput(), "CF_Round");
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"), "app-id");
await user.type(screen.getByPlaceholderText("Leave blank for public clients / PKCE"), "app-secret");
await waitFor(() => expect(oauthHook.onTokenReceived).toBeTruthy());
await act(async () => {
oauthHook.onTokenReceived!({ access_token: "cf-tok", token_type: "Bearer" }, undefined);
});
await selectAntOption("Authentication", "OAuth");
await selectAntOption("Authentication", "True Passthrough (no LiteLLM auth)");
const cfRoundServer = {
server_id: "cf-round",
server_name: "CF_Round",
alias: "CF_Round",
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",
};
vi.mocked(networking.createMCPServer).mockResolvedValue(cfRoundServer);
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).toEqual({ client_id: "app-id", client_secret: "app-secret" });
});
it("keeps the typed app but warns when the URL changes after a client-forwarded authorize", async () => {
await selectHttpTransport();
const user = userEvent.setup({ delay: null });
await user.type(getServerNameInput(), "CF_Warn");
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"), "app-id");
await waitFor(() => expect(oauthHook.onTokenReceived).toBeTruthy());
await act(async () => {
oauthHook.onTokenReceived!({ access_token: "cf-tok", token_type: "Bearer" }, undefined);
});
await act(async () => {
fireEvent.change(screen.getByPlaceholderText("https://your-mcp-server.com"), {
target: { value: "https://other.example.com/mcp" },
});
});
// Keep + warn: the app stays in the field, and a non-blocking warning appears.
expect(screen.getByText(/OAuth app entered here was registered for the previous upstream/)).toBeInTheDocument();
});
it("keeps client_secret when only client_id is edited after a client-forwarded authorize", async () => {
await selectHttpTransport();
const user = userEvent.setup({ delay: null });
await user.type(getServerNameInput(), "CF_Keystroke");
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"), "app-id");
await user.type(screen.getByPlaceholderText("Leave blank for public clients / PKCE"), "app-secret");
await waitFor(() => expect(oauthHook.onTokenReceived).toBeTruthy());
await act(async () => {
oauthHook.onTokenReceived!({ access_token: "cf-tok", token_type: "Bearer" }, undefined);
});
// Editing only client_id fires an invalidation whose changedValues carries only the client_id
// sub-field; the preserve + deep-merge re-apply must keep client_secret from being dropped.
await user.type(screen.getByPlaceholderText("Leave blank to use dynamic client registration"), "2");
const cfKeystrokeServer = {
server_id: "cf-keystroke",
server_name: "CF_Keystroke",
alias: "CF_Keystroke",
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",
};
vi.mocked(networking.createMCPServer).mockResolvedValue(cfKeystrokeServer);
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).toEqual({ client_id: "app-id2", client_secret: "app-secret" });
});
it("replaces the token set on re-authorize instead of leaving stale siblings", async () => {
await selectHttpTransport();
const user = userEvent.setup({ delay: null });
await user.type(getServerNameInput(), "Reauth_Server");
await user.type(screen.getByPlaceholderText("https://your-mcp-server.com"), "https://example.com/mcp");
await selectAntOption("Authentication", "OAuth");
await waitFor(() => expect(oauthHook.onTokenReceived).toBeTruthy());
const firstToken = { access_token: "T1", refresh_token: "R1", scope: "read", token_type: "Bearer" };
await act(async () => {
oauthHook.onTokenReceived!(firstToken, undefined);
});
await act(async () => {
oauthHook.onTokenReceived!({ access_token: "T2", token_type: "Bearer" }, undefined);
});
const creds = oauthHook.getCredentials?.() ?? {};
expect(creds.access_token).toBe("T2");
expect(creds.refresh_token).toBeUndefined();
expect(creds.scope).toBeUndefined();
});
it("should not show auth value field when None auth type is selected", async () => {
await selectHttpTransport();

View file

@ -19,6 +19,7 @@ import {
CLEARED_ON_INVALIDATION,
isHeldOAuthTokenStale,
preservedDeclaredAppCredentials,
withoutMintedTokenCredentials,
} from "./types";
import OAuthFormFields from "./OAuthFormFields";
import TruePassthroughWarning from "./TruePassthroughWarning";
@ -109,6 +110,14 @@ const CreateMCPServer: React.FC<CreateMCPServerProps> = ({
// was fetched; undefined when no valid token is held. If any mint-relevant field diverges from this,
// the held token is stale and is discarded so the admin must re-authorize.
const [authorizedIdentity, setAuthorizedIdentity] = useState<string | undefined>(undefined);
// The DCR-minted OAuth client from an interactive (oauth2) Authorize. Held OUT of form.credentials so
// it can never be collected as a client-forwarded server's declared app; injected into the payload
// only on an oauth2 submit (where persisting the registered client is correct), and cleared on any
// invalidation or modal close. An abandoned authorize leaves it null, which is the desired asymmetry.
const dcrClientRef = React.useRef<{ client_id: string; client_secret?: string } | null>(null);
// Set when the upstream identity (url/endpoints) changed while a declared app is present, so the
// section can warn that the saved app may not match the new upstream (the app is kept, not wiped).
const [appMayNotMatchUpstream, setAppMayNotMatchUpstream] = useState(false);
// Single hook call shared by MCPConnectionStatus and MCPToolConfiguration to avoid duplicate requests.
const {
@ -150,6 +159,9 @@ const CreateMCPServer: React.FC<CreateMCPServerProps> = ({
searchValue,
aliasManuallyEdited,
logoUrl,
// Persist the identity so invalidation stays armed across the OAuth redirect round trip: a
// post-restore url/mode edit must still discard the held token instead of silently keeping it.
authorizedIdentity,
};
setSecureItem(CREATE_OAUTH_UI_STATE_KEY, JSON.stringify(uiState));
} catch (err) {
@ -165,7 +177,12 @@ const CreateMCPServer: React.FC<CreateMCPServerProps> = ({
reset: resetOAuthFlow,
} = useMcpOAuthFlow({
accessToken,
getCredentials: () => form.getFieldValue("credentials"),
// Merge the ref-held DCR client so a re-authorize reuses the registered client instead of
// re-registering; the form store itself never holds the DCR client (see onTokenReceived).
getCredentials: () => ({
...((form.getFieldValue("credentials") as Record<string, unknown> | undefined) ?? {}),
...(dcrClientRef.current ?? {}),
}),
getTemporaryPayload: () => {
const values = form.getFieldsValue(true);
const transport = values.transport || transportType;
@ -187,7 +204,9 @@ const CreateMCPServer: React.FC<CreateMCPServerProps> = ({
url,
transport: transport === TRANSPORT.OPENAPI ? "http" : transport,
auth_type: isClientForwardedTokenMode(values.auth_type) ? values.auth_type : AUTH_TYPE.OAUTH2,
credentials: values.credentials,
credentials: isClientForwardedTokenMode(values.auth_type)
? preservedDeclaredAppCredentials(values.credentials)
: values.credentials,
authorization_url: values.authorization_url,
token_url: values.token_url,
registration_url: values.registration_url,
@ -217,18 +236,31 @@ const CreateMCPServer: React.FC<CreateMCPServerProps> = ({
return;
}
const credentials = {
// The DCR-minted client is held in a ref, NOT written into form.credentials, so it can never be
// collected as a client-forwarded server's declared app; it is injected into the payload only on
// an oauth2 submit. An admin-typed client already lives in form.credentials and is left untouched.
dcrClientRef.current = registeredClient?.clientId
? {
client_id: registeredClient.clientId,
...(registeredClient.clientSecret && { client_secret: registeredClient.clientSecret }),
}
: null;
const current = (form.getFieldValue("credentials") as Record<string, unknown> | undefined) ?? {};
const nextCredentials = {
...(preservedDeclaredAppCredentials(current) ?? {}),
...(current.scopes !== undefined && { scopes: current.scopes }),
access_token: token.access_token,
...(token.refresh_token && { refresh_token: token.refresh_token }),
...(token.expires_in && { expires_in: token.expires_in }),
...(token.scope && { scope: token.scope }),
...(registeredClient?.clientId && { client_id: registeredClient.clientId }),
...(registeredClient?.clientSecret && { client_secret: registeredClient.clientSecret }),
};
form.setFieldsValue({ credentials });
// Capture the identity AFTER writing the DCR'd credentials so the held token is not spuriously
// invalidated by its own credential write.
// Path-replace (not deep-merge) so a re-authorize with fewer token fields does not leave stale
// siblings from the previous token behind; the admin-typed client keys and scopes are carried
// explicitly above.
form.setFieldValue("credentials", nextCredentials);
// Capture the identity AFTER writing the token so the held token is not spuriously invalidated by
// its own credential write.
setAuthorizedIdentity(getOAuthAuthorizationIdentity(form.getFieldsValue(true)));
NotificationsManager.success(
@ -249,15 +281,17 @@ const CreateMCPServer: React.FC<CreateMCPServerProps> = ({
clearTools();
resetOAuthFlow();
setAuthorizedIdentity(undefined);
const keptAppCredentials = preservedDeclaredAppCredentials(
form.getFieldValue("auth_type"),
"auth_type" in changedValues,
form.getFieldValue("credentials"),
);
dcrClientRef.current = null;
// Capture the admin-typed app before resetFields destroys it, then re-apply it: the app is
// upstream-scoped config, not minted material, so it survives every invalidation (the token is
// what gets discarded). Token-shaped keys are excluded by the helper's key filter.
const keptAppCredentials = preservedDeclaredAppCredentials(form.getFieldValue("credentials"));
form.resetFields([...CLEARED_ON_INVALIDATION]);
if (keptAppCredentials) {
form.setFieldsValue({ credentials: keptAppCredentials });
}
// Re-apply the in-flight edit last; rc-field-form deep-merges nested objects, so a changed
// credentials sub-field composes with the preserved sibling instead of replacing the object.
const preserved = Object.fromEntries(
CLEARED_ON_INVALIDATION.filter((key) => key in changedValues).map((key) => [key, changedValues[key]]),
);
@ -285,7 +319,20 @@ const CreateMCPServer: React.FC<CreateMCPServerProps> = ({
setTransportType(restoredTransport);
}
if (parsed.formValues) {
setPendingRestoredValues({ values: parsed.formValues, transport: restoredTransport });
// Strip minted token material from the restored credentials so a stale token never rehydrates
// into the form store (defense in depth for pre-fix snapshots); the declared app is kept.
const restoredValues = {
...parsed.formValues,
...(parsed.formValues.credentials
? { credentials: withoutMintedTokenCredentials(parsed.formValues.credentials) }
: {}),
};
setPendingRestoredValues({ values: restoredValues, transport: restoredTransport });
}
if (typeof parsed.authorizedIdentity === "string") {
// Re-arm invalidation: without this the remounted form has authorizedIdentity=undefined, so a
// post-restore mode/url edit would never fire the stale-token discard.
setAuthorizedIdentity(parsed.authorizedIdentity);
}
if (parsed.costConfig) {
setCostConfig(parsed.costConfig);
@ -511,8 +558,20 @@ const CreateMCPServer: React.FC<CreateMCPServerProps> = ({
const includeCredentials =
restValues.auth_type && AUTH_TYPES_REQUIRING_CREDENTIALS.includes(restValues.auth_type);
if (includeCredentials && credentialsPayload && Object.keys(credentialsPayload).length > 0) {
payload.credentials = credentialsPayload;
// Client-forwarded rows persist ONLY the declared app; strip any token material that lingered in
// the form (e.g. from a prior oauth2 authorize on the same session) so it can never reach the row.
const submitCredentials = isClientForwardedTokenMode(restValues.auth_type)
? preservedDeclaredAppCredentials(credentialsPayload)
: credentialsPayload;
if (includeCredentials && submitCredentials && Object.keys(submitCredentials).length > 0) {
payload.credentials = submitCredentials;
}
// An interactive (oauth2) create persists its DCR-minted client from the ref (kept out of the
// form store); reuse a re-authorize's registered client instead of re-registering.
if (restValues.auth_type === AUTH_TYPE.OAUTH2 && dcrClientRef.current) {
payload.credentials = { ...(payload.credentials ?? {}), ...dcrClientRef.current };
}
if (accessToken != null) {
@ -587,6 +646,9 @@ const CreateMCPServer: React.FC<CreateMCPServerProps> = ({
setHasToolAllowlistInteraction(false);
setAliasManuallyEdited(false);
setLogoUrl(undefined);
setAuthorizedIdentity(undefined);
dcrClientRef.current = null;
setAppMayNotMatchUpstream(false);
setModalVisible(false);
};
@ -674,12 +736,28 @@ const CreateMCPServer: React.FC<CreateMCPServerProps> = ({
const handleFormValuesChange = (changedValues: Record<string, unknown>, allValues: Record<string, unknown>) => {
// Any change to a mint-relevant field (url, auth_type, oauth_flow_type, client creds/scopes, or the
// authorization/token/registration endpoints — see getOAuthAuthorizationIdentity) makes a held token
// stale, so discard it and force a fresh authorize. When that happens, formValues must be rebuilt
// from the form's post-reset state, not the pre-reset allValues snapshot: the snapshot still holds
// the discarded token in credentials, and useTestMCPConnection reads formValues for tool preview.
if (isHeldOAuthTokenStale(allValues, authorizedIdentity)) {
// stale, so discard it and force a fresh authorize. The stale check reads getFieldsValue(true): the
// onValuesChange allValues argument holds only MOUNTED paths, so an unmounted identity field (e.g.
// an oauth_flow_type initialValue while in a client-forwarded mode) would compare as changed on
// every keystroke and churn the held token. When a clear happens, formValues is rebuilt from the
// 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.
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.
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);
setFormValues({ ...form.getFieldsValue(true), ...changedValues });
if (upstreamChanged && hasDeclaredApp && !("credentials" in changedValues)) {
setAppMayNotMatchUpstream(true);
}
setFormValues(form.getFieldsValue(true));
return;
}
setFormValues(allValues);
@ -1006,6 +1084,7 @@ const CreateMCPServer: React.FC<CreateMCPServerProps> = ({
error: oauthError,
tokenResponse: oauthTokenResponse,
}}
appMayNotMatchUpstream={appMayNotMatchUpstream}
/>
{shouldShowAuthValueField && (

View file

@ -1403,7 +1403,10 @@ describe("MCPServerEdit (OAuth token persistence on save)", () => {
screen.getByPlaceholderText("Leave blank to keep the currently saved app (if any)"),
"org-app-client-id",
);
await user.type(screen.getByPlaceholderText("Leave blank for public clients / PKCE"), "org-app-secret");
await user.type(
screen.getByPlaceholderText("Leave blank to keep the currently saved secret (if any)"),
"org-app-secret",
);
await act(async () => {
fireEvent.click(screen.getAllByRole("button", { name: "Save Changes" })[0]);
@ -1447,7 +1450,10 @@ describe("MCPServerEdit (OAuth token persistence on save)", () => {
screen.getByPlaceholderText("Leave blank to keep the currently saved app (if any)"),
"org-app-client-id",
);
await user.type(screen.getByPlaceholderText("Leave blank for public clients / PKCE"), "org-app-secret");
await user.type(
screen.getByPlaceholderText("Leave blank to keep the currently saved secret (if any)"),
"org-app-secret",
);
act(() => {
mockOauth.onTokenReceived?.({ access_token: "cf-tok", token_type: "bearer" });

View file

@ -9,6 +9,7 @@ import {
CLEARED_ON_INVALIDATION,
isHeldOAuthTokenStale,
preservedDeclaredAppCredentials,
withoutMintedTokenCredentials,
OAUTH_FLOW,
MCP_OAUTH2_FLOW_M2M,
MCP_OAUTH2_FLOW_INTERACTIVE,
@ -183,7 +184,9 @@ const MCPServerEdit: React.FC<MCPServerEditProps> = ({
url,
transport,
auth_type: isClientForwardedTokenMode(values.auth_type) ? values.auth_type : AUTH_TYPE.OAUTH2,
credentials: values.credentials,
credentials: isClientForwardedTokenMode(values.auth_type)
? preservedDeclaredAppCredentials(values.credentials)
: values.credentials,
mcp_access_groups: values.mcp_access_groups || mcpServer.mcp_access_groups,
static_headers: staticHeaders,
command: values.command,
@ -211,14 +214,18 @@ const MCPServerEdit: React.FC<MCPServerEditProps> = ({
return;
}
const credentials = {
const current = (form.getFieldValue("credentials") as Record<string, unknown> | undefined) ?? {};
const nextCredentials = {
...(preservedDeclaredAppCredentials(current) ?? {}),
...(current.scopes !== undefined && { scopes: current.scopes }),
access_token: token.access_token,
...(token.refresh_token && { refresh_token: token.refresh_token }),
...(token.expires_in && { expires_in: token.expires_in }),
...(token.scope && { scope: token.scope }),
};
form.setFieldsValue({ credentials });
// Path-replace (not deep-merge) so a re-authorize with fewer token fields does not leave stale
// siblings behind; the admin-typed client keys and scopes are carried explicitly above.
form.setFieldValue("credentials", nextCredentials);
// Re-capture after writing credentials so the token is not invalidated by its own credential write.
authorizedIdentityRef.current = getOAuthAuthorizationIdentity(form.getFieldsValue(true));
@ -336,8 +343,19 @@ const MCPServerEdit: React.FC<MCPServerEditProps> = ({
return;
}
if (parsed.formValues) {
setPendingRestoredValues({ ...mcpServer, ...parsed.formValues });
// Strip minted token material from restored credentials so a stale token never rehydrates into
// the form store (defense in depth for pre-fix snapshots); the declared app is kept.
const restoredValues = {
...mcpServer,
...parsed.formValues,
...(parsed.formValues.credentials
? { credentials: withoutMintedTokenCredentials(parsed.formValues.credentials) }
: {}),
};
setPendingRestoredValues(restoredValues);
}
// The ref is re-armed by onTokenReceived when the redirect completes the code exchange, so there
// is no separate restore-side re-arm here (writing a ref inside an effect is disallowed).
if (parsed.costConfig) {
setCostConfig(parsed.costConfig);
}
@ -411,11 +429,9 @@ const MCPServerEdit: React.FC<MCPServerEditProps> = ({
}
setTools([]);
resetOAuthFlow();
const keptAppCredentials = preservedDeclaredAppCredentials(
getEffectiveAuthType(),
"auth_type" in changedValues,
form.getFieldValue("credentials"),
);
// The admin-typed app is upstream-scoped config, not minted material, so it survives every
// invalidation; only the held token is discarded. Token-shaped keys are excluded by the filter.
const keptAppCredentials = preservedDeclaredAppCredentials(form.getFieldValue("credentials"));
form.resetFields([...CLEARED_ON_INVALIDATION]);
if (keptAppCredentials) {
form.setFieldsValue({ credentials: keptAppCredentials });
@ -862,14 +878,20 @@ const MCPServerEdit: React.FC<MCPServerEditProps> = ({
const includeCredentials =
restValues.auth_type && AUTH_TYPES_REQUIRING_CREDENTIALS.includes(restValues.auth_type);
if (includeCredentials && credentialsPayload && Object.keys(credentialsPayload).length > 0) {
payload.credentials = credentialsPayload;
// Client-forwarded rows persist ONLY the declared app; strip any token material lingering in the
// form (e.g. from a prior oauth2 authorize this session) so it can never reach the row.
const submitCredentials = isClientForwardedTokenMode(restValues.auth_type)
? preservedDeclaredAppCredentials(credentialsPayload)
: credentialsPayload;
if (includeCredentials && submitCredentials && Object.keys(submitCredentials).length > 0) {
payload.credentials = submitCredentials;
}
// Explicit removal of a saved app for the client-forwarded modes. Blank fields are the
// keep-existing convention (the backend merges partial credential updates), so removal must be
// an explicit-null write: encrypt skips nulls and the merge overrides the stored keys, which
// returns the server to dynamic client registration.
// Explicit removal of a saved app for the client-forwarded modes, applied AFTER the filter so it
// always wins. Blank fields are the keep-existing convention (the backend merges partial
// credential updates), so removal must be an explicit-null write: encrypt skips nulls and the
// merge overrides the stored keys, returning the server to dynamic client registration.
if (removeStoredApp && isClientForwardedTokenMode(restValues.auth_type)) {
payload.credentials = { client_id: null, client_secret: null };
}
@ -1061,6 +1083,7 @@ const MCPServerEdit: React.FC<MCPServerEditProps> = ({
tokenResponse: oauthTokenResponse,
}}
isEditing
savedAuthType={mcpServer.auth_type}
removeStoredApp={removeStoredApp}
onRemoveStoredAppChange={setRemoveStoredApp}
/>

View file

@ -10,6 +10,9 @@ import {
getOAuthAuthorizationIdentity,
isHeldOAuthTokenStale,
oauth2FlowToFormValue,
preservedDeclaredAppCredentials,
withoutMintedTokenCredentials,
credentialAuthClass,
} from "./types";
describe("getOAuthAuthorizationIdentity", () => {
@ -180,3 +183,45 @@ describe("oauth2FlowToFormValue", () => {
expect(oauth2FlowToFormValue(undefined)).toBeUndefined();
});
});
describe("preservedDeclaredAppCredentials", () => {
it("keeps only non-empty string declared-app keys and never token-shaped keys", () => {
expect(preservedDeclaredAppCredentials(undefined)).toBeUndefined();
expect(preservedDeclaredAppCredentials({})).toBeUndefined();
expect(preservedDeclaredAppCredentials({ client_id: 123 })).toBeUndefined();
expect(preservedDeclaredAppCredentials({ client_id: "" })).toBeUndefined();
expect(preservedDeclaredAppCredentials({ client_id: "a", access_token: "t", scopes: ["s"] })).toEqual({
client_id: "a",
});
expect(preservedDeclaredAppCredentials({ client_secret: "s" })).toEqual({ client_secret: "s" });
expect(preservedDeclaredAppCredentials({ client_id: "a", client_secret: "b", refresh_token: "r" })).toEqual({
client_id: "a",
client_secret: "b",
});
});
});
describe("withoutMintedTokenCredentials", () => {
it("drops token keys and keeps the declared app and other config", () => {
expect(withoutMintedTokenCredentials(undefined)).toBeUndefined();
const mixed = {
client_id: "a",
client_secret: "b",
access_token: "t",
refresh_token: "r",
expires_in: 3600,
scope: "read",
scopes: ["read"],
};
expect(withoutMintedTokenCredentials(mixed)).toEqual({ client_id: "a", client_secret: "b", scopes: ["read"] });
});
});
describe("credentialAuthClass", () => {
it("collapses the client-forwarded modes to one class and leaves others distinct", () => {
expect(credentialAuthClass(AUTH_TYPE.TRUE_PASSTHROUGH)).toBe("client_forwarded");
expect(credentialAuthClass(AUTH_TYPE.OAUTH_DELEGATE)).toBe("client_forwarded");
expect(credentialAuthClass(AUTH_TYPE.OAUTH2)).toBe(AUTH_TYPE.OAUTH2);
expect(credentialAuthClass(null)).toBeNull();
});
});

View file

@ -96,23 +96,25 @@ 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.
// The declared-app filter over form.credentials. It is a pure key filter with no mode/transition
// guard because the surrounding code establishes that a client_id/client_secret in form.credentials
// is ALWAYS admin-typed in every reachable state: the create form holds the DCR-minted client in a
// ref and never writes it into the form store, the edit form's onTokenReceived never writes client
// keys, and the invalidation reset clears the whole object atomically. So preserving the string
// client keys across any invalidation (URL/endpoint edit, true_passthrough<->oauth_delegate switch,
// or a round trip through another mode) is always legitimate, while the output key filter excludes
// token-shaped keys so a preserve can never carry minted material through. Shared by both forms.
const DECLARED_APP_CREDENTIAL_KEYS = ["client_id", "client_secret"] as const;
// Minted token material the oauth2 authorize path writes beside the app keys; stripped from restored
// snapshots and from any credentials that transit to the temp-session preview so a stale token never
// reaches the backend or a client-forwarded server row.
export const MINTED_TOKEN_CREDENTIAL_KEYS = ["access_token", "refresh_token", "expires_in", "scope"] 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;
if (!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],
@ -121,6 +123,24 @@ export const preservedDeclaredAppCredentials = (
return Object.keys(kept).length > 0 ? kept : undefined;
};
// Drop minted token keys, keeping everything else (the declared app plus any non-token config).
export const withoutMintedTokenCredentials = (
credentials: Record<string, unknown> | null | undefined,
): Record<string, unknown> | undefined => {
if (!credentials) return undefined;
return Object.fromEntries(
Object.entries(credentials).filter(([key]) => !(MINTED_TOKEN_CREDENTIAL_KEYS as readonly string[]).includes(key)),
);
};
// The client-forwarded modes share one credential class (same declared app, same authorize relay), so
// a switch between them must NOT be treated as an app change. Mirrors the backend _credential_auth_class
// in db.py; kept in sync so the UI's keep-existing copy and the backend's merge cannot disagree.
export const credentialAuthClass = (authType: string | null | undefined): string | null => {
if (authType === AUTH_TYPE.TRUE_PASSTHROUGH || authType === AUTH_TYPE.OAUTH_DELEGATE) return "client_forwarded";
return authType ?? null;
};
// 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