Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import type {
ProjectServerOverrideEntry,
} from "@/lib/project-server-config";
import { EffectiveProtocolVersionChip } from "./shared/EffectiveProtocolVersionChip";
import { fetchServerSecrets } from "@/lib/apis/server-secrets-api";
import { useFeatureFlagEnabled } from "posthog-js/react";
import { useActiveMcpProfile } from "@/contexts/active-mcp-profile-context";

Expand Down Expand Up @@ -503,9 +504,41 @@ export function ServerDetailModal({
environment: detectEnvironment(),
});

const finalFormData = formState.buildFormData();
setIsSaving(true);
try {
// Saving an auth or header change replaces the whole stored header
// set. When that set is hidden, fetch it first so e.g. rotating a
// bearer token doesn't wipe the other saved headers.
let revealedHeaders: Record<string, string> | undefined;
if (formState.needsStoredHeaderReveal) {
if (!projectId || !hostedServerId) {
toast.error(
"Reveal saved headers before changing authentication so existing hidden headers aren't lost."
);
return;
}
try {
const secrets = await fetchServerSecrets({
projectId,
serverId: hostedServerId,
});
// A null headers payload means the stored set couldn't be read;
// merging against it would wipe the saved headers, so fail closed.
if (!secrets.headers) {
throw new Error("Stored headers missing from reveal response");
}
revealedHeaders = secrets.headers;
} catch {
toast.error(
"Couldn't load this server's saved headers to apply this change. Reveal saved headers in Advanced settings and try again."
);
return;
}
}

const finalFormData = formState.buildFormData(
revealedHeaders ? { revealedHeaders } : undefined
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferred build races form reset

Medium Severity

handleSave now awaits fetchServerSecrets before calling buildFormData, but dirty flags and edited auth/header values live in hook state that useServerForm reinitializes whenever the server prop changes. If the live server snapshot updates during that await, authDirty/headersDirty can clear and the post-fetch patch may omit the user’s auth or header change or keep the old Authorization value while still showing success.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit f3c4fc7. Configure here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This race can't occur: handleSave captures formState from the render in which Save was clicked, and buildFormData reads only closure-bound useState values from that same render (no refs). If the server prop updates during the await and the init effect resets authDirty/headersDirty, that affects the next render's formState — the in-flight handler keeps building from the values as of the click. The second finding (null headers coerced to an empty merge base) was real and is fixed in 6cce650.


Generated by Claude Code

Comment thread
cursor[bot] marked this conversation as resolved.
await onSubmit(finalFormData, server.name);
} finally {
setIsSaving(false);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ describe("useServerForm", () => {
});
});

it("does not replace hidden stored headers when editing auth without reveal", async () => {
it("asks for stored headers when editing auth with hidden headers and merges them into the patch", async () => {
const server = {
name: "Hidden header server",
config: {
Expand All @@ -140,13 +140,117 @@ describe("useServerForm", () => {
result.current.setBearerToken("new-token");
});

expect(result.current.buildFormData()).toMatchObject({
headers: { Authorization: "Bearer new-token" },
});
// Without the stored headers the form can't build a safe replacement
// patch, so it withholds one and flags that a reveal is needed.
expect(result.current.needsStoredHeaderReveal).toBe(true);
expect(result.current.buildFormData().secretPatch?.headers).toBeUndefined();
expect(result.current.validateForm()).toBe(
"Reveal saved headers before changing authentication so existing hidden headers aren't lost."
);
expect(result.current.validateForm()).toBeNull();

// With the stored headers supplied at save time, the patch swaps the
// Authorization header and keeps the rest.
expect(
result.current.buildFormData({
revealedHeaders: {
Authorization: "Bearer old-token",
"X-Api-Key": "secret",
},
})
).toMatchObject({
headers: {
Authorization: "Bearer new-token",
"X-Api-Key": "secret",
},
secretPatch: {
headers: {
Authorization: "Bearer new-token",
"X-Api-Key": "secret",
},
},
});
});

it("keeps the hidden Authorization header when only header rows change", async () => {
const server = {
name: "Hidden header server",
config: {
url: "https://example.com/mcp",
},
hasHeaders: true,
lastConnectionTime: new Date(),
connectionStatus: "disconnected",
retryCount: 0,
enabled: true,
} as any;

const { result } = renderHook(() => useServerForm(server));

await waitFor(() => {
expect(result.current.hasStoredHeaders).toBe(true);
});

act(() => {
result.current.addCustomHeader();
});
act(() => {
result.current.updateCustomHeader(0, "key", "X-New");
});
act(() => {
result.current.updateCustomHeader(0, "value", "fresh");
});

expect(result.current.needsStoredHeaderReveal).toBe(true);
expect(
result.current.buildFormData({
revealedHeaders: {
Authorization: "Bearer keep-me",
"X-Api-Key": "secret",
},
}).secretPatch
).toEqual({
headers: {
Authorization: "Bearer keep-me",
"X-Api-Key": "secret",
"X-New": "fresh",
},
});
});

it("drops the hidden Authorization header when auth switches away from bearer", async () => {
const server = {
name: "Hidden header server",
config: {
url: "https://example.com/mcp",
},
hasHeaders: true,
lastConnectionTime: new Date(),
connectionStatus: "disconnected",
retryCount: 0,
enabled: true,
} as any;

const { result } = renderHook(() => useServerForm(server));

await waitFor(() => {
expect(result.current.hasStoredHeaders).toBe(true);
});

act(() => {
result.current.setAuthType("oauth");
});

expect(result.current.needsStoredHeaderReveal).toBe(true);
expect(
result.current.buildFormData({
revealedHeaders: {
Authorization: "Bearer old-token",
"X-Api-Key": "secret",
},
}).secretPatch
).toEqual({
headers: {
"X-Api-Key": "secret",
},
});
});

it("sends a replacement header patch after stored headers are revealed", async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,10 @@ export function useServerForm(
const [hasStoredHeaders, setHasStoredHeaders] = useState(false);
const [envDirty, setEnvDirty] = useState(false);
const [headersDirty, setHeadersDirty] = useState(false);
// Auth edits (auth type / bearer token) are tracked apart from header-row
// edits: when hidden stored headers are merged in at save time, the saved
// Authorization header must only be dropped if the user touched auth.
const [authDirty, setAuthDirty] = useState(false);
const [envRevealed, setEnvRevealed] = useState(false);
const [headersRevealed, setHeadersRevealed] = useState(false);
const [requestTimeout, setRequestTimeout] = useState<string>("");
Expand Down Expand Up @@ -384,6 +388,7 @@ export function useServerForm(
setHasStoredHeaders(hasStoredHeadersValue);
setHeadersRevealed(headersArray.length > 0);
setHeadersDirty(false);
setAuthDirty(false);
setShowConfiguration(
headersArray.length > 0 ||
timeoutValue.trim() !== "" ||
Expand Down Expand Up @@ -468,10 +473,6 @@ export function useServerForm(
) {
return "HTTPS is required";
}

if (hasStoredHeaders && !headersRevealed && headersDirty) {
return "Reveal saved headers before changing authentication so existing hidden headers aren't lost.";
}
}

if (
Expand Down Expand Up @@ -600,7 +601,14 @@ export function useServerForm(
}
};

const buildFormData = (): ServerFormData => {
const buildFormData = (buildOptions?: {
/**
* Stored headers fetched from the secrets API at save time. Supplying
* them lets a server with hidden stored headers take an auth or header
* change without wiping the headers the form can't see.
*/
revealedHeaders?: Record<string, string>;
}): ServerFormData => {
const parsedTimeout = Number.parseInt(requestTimeout.trim(), 10);
const reqTimeout = Number.isFinite(parsedTimeout)
? parsedTimeout
Expand Down Expand Up @@ -645,8 +653,21 @@ export function useServerForm(
}

// Handle http-specific data
const revealedStoredHeaders = buildOptions?.revealedHeaders;
const headers: Record<string, string> = {};

// Seed with the stored headers so the replacement patch keeps them. The
// saved Authorization header only carries over while auth is untouched —
// once the user edits auth, the auth section below is authoritative.
if (revealedStoredHeaders) {
for (const [key, value] of Object.entries(revealedStoredHeaders)) {
if (authDirty && isAuthorizationHeader(key)) {
continue;
}
headers[key] = value;
}
}

// Add custom headers
customHeaders.forEach(({ key, value }) => {
if (key.trim()) {
Expand Down Expand Up @@ -682,9 +703,10 @@ export function useServerForm(
}
const explicitHeaders =
Object.keys(headers).length > 0 ? headers : undefined;
const canPatchHeaders = !hasStoredHeaders || headersRevealed;
const canPatchHeaders =
!hasStoredHeaders || headersRevealed || revealedStoredHeaders != null;
const secretPatch =
headersDirty && canPatchHeaders ? { headers } : undefined;
(headersDirty || authDirty) && canPatchHeaders ? { headers } : undefined;

return {
name: name.trim(),
Expand Down Expand Up @@ -736,6 +758,7 @@ export function useServerForm(
setHasStoredHeaders(false);
setEnvDirty(false);
setHeadersDirty(false);
setAuthDirty(false);
setEnvRevealed(false);
setHeadersRevealed(false);
setRequestTimeout("");
Expand Down Expand Up @@ -778,6 +801,15 @@ export function useServerForm(
);
})();

// Saving a header-affecting change replaces the whole stored header set,
// so when that set is hidden the caller must fetch it (secrets API) and
// pass it to buildFormData as `revealedHeaders` before submitting.
const needsStoredHeaderReveal =
type === "http" &&
hasStoredHeaders &&
!headersRevealed &&
(headersDirty || authDirty);

const preregisteredOauthBlocksSubmit =
type === "http" &&
authType === "oauth" &&
Expand Down Expand Up @@ -822,12 +854,12 @@ export function useServerForm(
setClearClientSecret,
bearerToken,
setBearerToken: (value: string) => {
setHeadersDirty(true);
setAuthDirty(true);
setBearerToken(value);
},
authType,
setAuthType: (value: "oauth" | "bearer" | "none") => {
setHeadersDirty(true);
setAuthDirty(true);
setAuthType(value);
},
useCustomClientId,
Expand Down Expand Up @@ -859,6 +891,7 @@ export function useServerForm(
headersDirty,
envRevealed,
headersRevealed,
needsStoredHeaderReveal,

// Toggle states
showConfiguration,
Expand Down
Loading