Skip to content

fix(mcp): keep PKCE verifier in memory instead of on disk #1201

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
TheGreatAxios wants to merge 3 commits into from
Closed

fix(mcp): keep PKCE verifier in memory instead of on disk #1201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6ecc8a4
fix(mcp): keep PKCE verifier in memory instead of on disk
TheGreatAxios Sep 28, 2026
9f9c6ba
fix(mcp): propagate sibling logout and scrub legacy verifier at startup
TheGreatAxios Sep 28, 2026
f55e37f
fix(mcp): harden auth disk boundary and guard refresh-vs-reset race
TheGreatAxios Sep 28, 2026
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
38 changes: 31 additions & 7 deletions src/mcp/add-server.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,11 @@ describe("persistMCPServerEnabled", () => {
mcpServers: [linearHTTP],
}),
);
await saveAuthState(linearAuth, { codeVerifier: "keep-me" }, home);
await saveAuthState(
linearAuth,
{ tokens: { access_token: "keep-me", token_type: "bearer" } },
home,
);
const writer = createGlobalSettingsWriter(path);

expect(
Expand All @@ -365,7 +369,7 @@ describe("persistMCPServerEnabled", () => {
showPromptCost: true,
mcpServers: [{ ...linearHTTP, enabled: false }],
});
expect((await loadAuthState(linearAuth, home)).codeVerifier).toBe(
expect((await loadAuthState(linearAuth, home)).tokens?.access_token).toBe(
"keep-me",
);

Expand Down Expand Up @@ -447,7 +451,11 @@ describe("persistMCPServerRemoved", () => {
],
}),
);
await saveAuthState(linearAuth, { codeVerifier: "linear-secret" }, home);
await saveAuthState(
linearAuth,
{ tokens: { access_token: "linear-secret", token_type: "bearer" } },
home,
);
const writer = createGlobalSettingsWriter(path);

const result = await persistMCPServerRemoved(writer, "linear", home);
Expand Down Expand Up @@ -539,7 +547,11 @@ describe("persistMCPServerRemoved", () => {
path,
JSON.stringify({ providers: {}, mcpServers: [custom] }),
);
await saveAuthState(identity, { codeVerifier: "custom-exa" }, home);
await saveAuthState(
identity,
{ tokens: { access_token: "custom-exa", token_type: "bearer" } },
home,
);
const writer = createGlobalSettingsWriter(path);

expect(await persistMCPServerRemoved(writer, "exa", home)).toMatchObject({
Expand All @@ -560,7 +572,11 @@ describe("persistMCPServerRemoved", () => {
path,
JSON.stringify({ providers: {}, mcpServers: [linearHTTP] }),
);
await saveAuthState(linearAuth, { codeVerifier: "linear-secret" }, home);
await saveAuthState(
linearAuth,
{ tokens: { access_token: "linear-secret", token_type: "bearer" } },
home,
);
const authDir = mcpAuthDir(home);
await chmod(authDir, 0o000);
const writer = createGlobalSettingsWriter(path);
Expand Down Expand Up @@ -597,7 +613,11 @@ describe("local MCP persist", () => {
localPath,
JSON.stringify({ provider: "a", mcpServers: [linearHTTP] }),
);
await saveAuthState(linearAuth, { codeVerifier: "local-linear" }, home);
await saveAuthState(
linearAuth,
{ tokens: { access_token: "local-linear", token_type: "bearer" } },
home,
);
const writer = createLocalSettingsWriter(localPath);

expect(
Expand Down Expand Up @@ -641,7 +661,11 @@ describe("local MCP persist", () => {
path,
JSON.stringify({ provider: "a", mcpServers: [linearHTTP] }),
);
await saveAuthState(linearAuth, { codeVerifier: "local-linear" }, home);
await saveAuthState(
linearAuth,
{ tokens: { access_token: "local-linear", token_type: "bearer" } },
home,
);
const authDir = mcpAuthDir(home);
await chmod(authDir, 0o000);
const writer = createLocalSettingsWriter(path);
Expand Down
76 changes: 53 additions & 23 deletions src/mcp/auth-store.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ describe("mcp auth-store", () => {
home,
);

// Reproduce the bug: one writer saves tokens while another overwrites with a
// fresh codeVerifier from a concurrent OAuth start. With full-snapshot
// persists, the verifier write wiped tokens; with updateAuthState, both land.
// Reproduce the bug: one writer saves tokens while another updates the
// client registration. With full-snapshot persists, the second write
// wiped tokens; with updateAuthState, both land.
const writes = await Promise.all([
updateAuthState(
linear,
Expand All @@ -52,23 +52,26 @@ describe("mcp auth-store", () => {
updateAuthState(
linear,
(state) => {
state.codeVerifier = "verifier-from-other-session";
state.clientInformation = {
client_id: "c2",
redirect_uris: ["http://127.0.0.1:1/callback"],
client_id_issued_at: 2,
};
},
home,
),
]);

const final = await loadAuthState(linear, home);
expect(final.tokens?.access_token).toBe("tok");
expect(final.codeVerifier).toBe("verifier-from-other-session");
expect(final.clientInformation?.client_id).toBe("c1");
expect(final.clientInformation?.client_id).toBe("c2");
expect(
writes[0].tokens?.access_token === "tok" ||
writes[1].tokens?.access_token === "tok",
).toBe(true);
});

test("overlapping updateAuthState from two processes keeps tokens and PKCE", async () => {
test("overlapping updateAuthState from two processes keeps tokens and client", async () => {
const home = await tempHome();
await saveAuthState(
linear,
Expand Down Expand Up @@ -103,7 +106,11 @@ describe("mcp auth-store", () => {
refresh_token: "ref",
};
} else {
state.codeVerifier = "verifier-from-other-session";
state.clientInformation = {
client_id: "c2",
redirect_uris: ["http://127.0.0.1:1/callback"],
client_id_issued_at: 2,
};
}
},
home,
Expand All @@ -118,7 +125,7 @@ describe("mcp auth-store", () => {
},
),
Bun.spawn(
[process.execPath, "-e", script, "--", home, "verifier", barrier],
[process.execPath, "-e", script, "--", home, "client", barrier],
{
stdout: "ignore",
stderr: "pipe",
Expand All @@ -135,27 +142,32 @@ describe("mcp auth-store", () => {

const final = await loadAuthState(linear, home);
expect(final.tokens?.access_token).toBe("tok");
expect(final.codeVerifier).toBe("verifier-from-other-session");
expect(final.clientInformation?.client_id).toBe("c1");
expect(final.clientInformation?.client_id).toBe("c2");
});

test("concurrent saveAuthState calls do not throw ENOENT on temp rename", async () => {
const home = await tempHome();
await Promise.all(
Array.from({ length: 20 }, (_, i) =>
saveAuthState(linear, { codeVerifier: `v${String(i)}` }, home),
saveAuthState(
linear,
{ tokens: { access_token: `v${String(i)}`, token_type: "bearer" } },
home,
),
),
);
const final = await loadAuthState(linear, home);
expect(final.codeVerifier?.startsWith("v")).toBe(true);
expect(final.tokens?.access_token?.startsWith("v")).toBe(true);
// No leftover temp files from failed renames.
const dir = join(home, ".corbits", "mcp-auth");
const files = await Array.fromAsync(
new Bun.Glob("linear-*.json").scan(dir),
);
expect(files).toHaveLength(1);
const raw = await readFile(join(dir, files[0] ?? "missing"), "utf8");
expect(JSON.parse(raw).codeVerifier).toBe(final.codeVerifier);
expect(JSON.parse(raw).tokens?.access_token).toBe(
final.tokens?.access_token,
);
});

test("scopes credentials to normalized endpoint identity", async () => {
Expand All @@ -175,13 +187,21 @@ describe("mcp auth-store", () => {
serverURL: "https://ONE.example:443/mcp#ignored",
};

await saveAuthState(originA, { codeVerifier: "only-a" }, home);
await saveAuthState(
originA,
{ tokens: { access_token: "only-a", token_type: "bearer" } },
home,
);

expect((await loadAuthState(originA, home)).codeVerifier).toBe("only-a");
expect((await loadAuthState(originA, home)).tokens?.access_token).toBe(
"only-a",
);
expect(await loadAuthState(originB, home)).toEqual({});
expect(await loadAuthState(pathB, home)).toEqual({});
expect(await loadAuthState(queryB, home)).toEqual({});
expect((await loadAuthState(equivalent, home)).codeVerifier).toBe("only-a");
expect((await loadAuthState(equivalent, home)).tokens?.access_token).toBe(
"only-a",
);
});

test("ignores legacy name-only auth state without modifying it", async () => {
Expand Down Expand Up @@ -211,7 +231,7 @@ describe("mcp auth-store", () => {
const longName = `${prefixName}/long`;
await saveAuthState(
{ serverName: longName, serverURL: "https://long.example/mcp" },
{ codeVerifier: "scoped-secret" },
{ tokens: { access_token: "scoped-secret", token_type: "bearer" } },
home,
);

Expand All @@ -225,8 +245,14 @@ describe("mcp auth-store", () => {

test("deleteAuthState removes an existing file", async () => {
const home = await tempHome();
await saveAuthState(linear, { codeVerifier: "secret" }, home);
expect((await loadAuthState(linear, home)).codeVerifier).toBe("secret");
await saveAuthState(
linear,
{ tokens: { access_token: "secret", token_type: "bearer" } },
home,
);
expect((await loadAuthState(linear, home)).tokens?.access_token).toBe(
"secret",
);

await deleteAuthState(linear, home);
expect(await loadAuthState(linear, home)).toEqual({});
Expand All @@ -241,13 +267,17 @@ describe("mcp auth-store", () => {

test("concurrent updateAuthState and deleteAuthState do not throw ENOENT on rename", async () => {
const home = await tempHome();
await saveAuthState(linear, { codeVerifier: "v0" }, home);
await saveAuthState(
linear,
{ tokens: { access_token: "v0", token_type: "bearer" } },
home,
);

await Promise.all([
updateAuthState(
linear,
(state) => {
state.codeVerifier = "v1";
state.tokens = { access_token: "v1", token_type: "bearer" };
},
home,
),
Expand All @@ -256,7 +286,7 @@ describe("mcp auth-store", () => {

const final = await loadAuthState(linear, home);
expect(
final.codeVerifier === undefined || final.codeVerifier === "v1",
final.tokens === undefined || final.tokens?.access_token === "v1",
).toBe(true);
});
});
32 changes: 25 additions & 7 deletions src/mcp/auth-store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,15 @@ import type {
} from "@modelcontextprotocol/sdk/shared/auth.js";
import { SETTINGS_DIR_NAME } from "../branding.js";

// Per-server OAuth state persisted between sessions. Holding the PKCE verifier is
// necessary because authorization spans a process boundary (browser round-trip);
// tokens and dynamically-registered client info let later sessions reconnect
// without any user interaction.
// Per-server OAuth state persisted between sessions. Tokens and
// dynamically-registered client info let later sessions reconnect
// without any user interaction. The PKCE verifier is intentionally absent:
// it lives only in the provider instance that started the flow and is never
// written to disk. Parsing stays tolerant of older files that still carry
// one; resetAuthorization scrubs the legacy key.
export interface MCPAuthState {
clientInformation?: OAuthClientInformationFull;
tokens?: OAuthTokens;
codeVerifier?: string;
}

export interface MCPAuthIdentity {
Expand Down Expand Up @@ -76,11 +77,28 @@ function isEnoent(err: unknown): boolean {
);
}

function isAuthRecord(value: unknown): value is Record<string, unknown> {
return typeof value === "object" && value !== null && !Array.isArray(value);
}

export function isValidAuthTokens(value: unknown): value is OAuthTokens {
return isAuthRecord(value) && typeof value.access_token === "string";
}

function parseAuthState(raw: string): MCPAuthState | undefined {
try {
const parsed: unknown = JSON.parse(raw);
if (typeof parsed === "object" && parsed !== null)
return parsed as MCPAuthState;
// The discriminator is explicit shape, not "parses to object": arrays,
// null, and primitives are corrupt. Tokens crossing from disk into memory
// must be absent (a missing key is an explicit logout write) or an object
// carrying a string access_token; anything else reads as unreadable so
// the caller keeps its in-memory mirror and never stores the malformed
// value.
if (!isAuthRecord(parsed)) return undefined;
const state = parsed as MCPAuthState;
if (state.tokens !== undefined && !isValidAuthTokens(state.tokens))
return undefined;
return state;
} catch {
// A corrupt auth file should not wedge the session; treat it as no state and
// let a fresh authorization overwrite it.
Expand Down
Loading
Loading