Skip to content

Commit 0a4273d

Browse files
TheGreatAxiosTheGreatAxios
authored
Merge pull request #1285 from corbitsdev/cl-9001-mcp-pkce-instance-local
fix(mcp): keep PKCE verifier in provider memory instead of disk
2 parents 5e63b12 + 232eeeb commit 0a4273d

5 files changed

Lines changed: 322 additions & 65 deletions

File tree

‎src/mcp/add-server.test.ts‎

Lines changed: 31 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -291,7 +291,11 @@ describe("persistMCPServerEnabled", () => {
291291
mcpServers: [linearHTTP],
292292
}),
293293
);
294-
await saveAuthState(linearAuth, { codeVerifier: "keep-me" }, home);
294+
await saveAuthState(
295+
linearAuth,
296+
{ tokens: { access_token: "keep-me", token_type: "bearer" } },
297+
home,
298+
);
295299
const writer = createGlobalSettingsWriter(path);
296300

297301
expect(
@@ -306,7 +310,7 @@ describe("persistMCPServerEnabled", () => {
306310
showPromptCost: true,
307311
mcpServers: [{ ...linearHTTP, enabled: false }],
308312
});
309-
expect((await loadAuthState(linearAuth, home)).codeVerifier).toBe(
313+
expect((await loadAuthState(linearAuth, home)).tokens?.access_token).toBe(
310314
"keep-me",
311315
);
312316

@@ -388,7 +392,11 @@ describe("persistMCPServerRemoved", () => {
388392
],
389393
}),
390394
);
391-
await saveAuthState(linearAuth, { codeVerifier: "linear-secret" }, home);
395+
await saveAuthState(
396+
linearAuth,
397+
{ tokens: { access_token: "linear-secret", token_type: "bearer" } },
398+
home,
399+
);
392400
const writer = createGlobalSettingsWriter(path);
393401

394402
const result = await persistMCPServerRemoved(writer, "linear", home);
@@ -480,7 +488,11 @@ describe("persistMCPServerRemoved", () => {
480488
path,
481489
JSON.stringify({ providers: {}, mcpServers: [custom] }),
482490
);
483-
await saveAuthState(identity, { codeVerifier: "custom-exa" }, home);
491+
await saveAuthState(
492+
identity,
493+
{ tokens: { access_token: "custom-exa", token_type: "bearer" } },
494+
home,
495+
);
484496
const writer = createGlobalSettingsWriter(path);
485497

486498
expect(await persistMCPServerRemoved(writer, "exa", home)).toMatchObject({
@@ -501,7 +513,11 @@ describe("persistMCPServerRemoved", () => {
501513
path,
502514
JSON.stringify({ providers: {}, mcpServers: [linearHTTP] }),
503515
);
504-
await saveAuthState(linearAuth, { codeVerifier: "linear-secret" }, home);
516+
await saveAuthState(
517+
linearAuth,
518+
{ tokens: { access_token: "linear-secret", token_type: "bearer" } },
519+
home,
520+
);
505521
const authDir = mcpAuthDir(home);
506522
await chmod(authDir, 0o000);
507523
const writer = createGlobalSettingsWriter(path);
@@ -538,7 +554,11 @@ describe("local MCP persist", () => {
538554
localPath,
539555
JSON.stringify({ provider: "a", mcpServers: [linearHTTP] }),
540556
);
541-
await saveAuthState(linearAuth, { codeVerifier: "local-linear" }, home);
557+
await saveAuthState(
558+
linearAuth,
559+
{ tokens: { access_token: "local-linear", token_type: "bearer" } },
560+
home,
561+
);
542562
const writer = createLocalSettingsWriter(localPath);
543563

544564
expect(
@@ -582,7 +602,11 @@ describe("local MCP persist", () => {
582602
path,
583603
JSON.stringify({ provider: "a", mcpServers: [linearHTTP] }),
584604
);
585-
await saveAuthState(linearAuth, { codeVerifier: "local-linear" }, home);
605+
await saveAuthState(
606+
linearAuth,
607+
{ tokens: { access_token: "local-linear", token_type: "bearer" } },
608+
home,
609+
);
586610
const authDir = mcpAuthDir(home);
587611
await chmod(authDir, 0o000);
588612
const writer = createLocalSettingsWriter(path);

‎src/mcp/auth-store.test.ts‎

Lines changed: 86 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
import { describe, expect, test } from "bun:test";
22
import { mkdir, mkdtemp, readFile, writeFile } from "node:fs/promises";
33
import { tmpdir } from "node:os";
4-
import { join } from "node:path";
4+
import { dirname, join } from "node:path";
55
import {
6+
authFilePath,
67
deleteAuthState,
78
loadAuthState,
89
saveAuthState,
@@ -33,9 +34,10 @@ describe("mcp auth-store", () => {
3334
home,
3435
);
3536

36-
// Reproduce the bug: one writer saves tokens while another overwrites with a
37-
// fresh codeVerifier from a concurrent OAuth start. With full-snapshot
38-
// persists, the verifier write wiped tokens; with updateAuthState, both land.
37+
// Reproduce the bug: one writer saves tokens while another updates the
38+
// client registration from a concurrent OAuth start. With full-snapshot
39+
// persists, the registration write wiped tokens; with updateAuthState,
40+
// both land.
3941
const writes = await Promise.all([
4042
updateAuthState(
4143
linear,
@@ -52,23 +54,28 @@ describe("mcp auth-store", () => {
5254
updateAuthState(
5355
linear,
5456
(state) => {
55-
state.codeVerifier = "verifier-from-other-session";
57+
state.clientInformation = {
58+
client_id: "c1",
59+
redirect_uris: ["http://127.0.0.1:1/callback"],
60+
client_id_issued_at: 1,
61+
client_name: "other-session",
62+
};
5663
},
5764
home,
5865
),
5966
]);
6067

6168
const final = await loadAuthState(linear, home);
6269
expect(final.tokens?.access_token).toBe("tok");
63-
expect(final.codeVerifier).toBe("verifier-from-other-session");
70+
expect(final.clientInformation?.client_name).toBe("other-session");
6471
expect(final.clientInformation?.client_id).toBe("c1");
6572
expect(
6673
writes[0].tokens?.access_token === "tok" ||
6774
writes[1].tokens?.access_token === "tok",
6875
).toBe(true);
6976
});
7077

71-
test("overlapping updateAuthState from two processes keeps tokens and PKCE", async () => {
78+
test("overlapping updateAuthState from two processes keeps tokens and client registration", async () => {
7279
const home = await tempHome();
7380
await saveAuthState(
7481
linear,
@@ -103,7 +110,12 @@ describe("mcp auth-store", () => {
103110
refresh_token: "ref",
104111
};
105112
} else {
106-
state.codeVerifier = "verifier-from-other-session";
113+
state.clientInformation = {
114+
client_id: "c1",
115+
redirect_uris: ["http://127.0.0.1:1/callback"],
116+
client_id_issued_at: 1,
117+
client_name: "other-session",
118+
};
107119
}
108120
},
109121
home,
@@ -118,7 +130,7 @@ describe("mcp auth-store", () => {
118130
},
119131
),
120132
Bun.spawn(
121-
[process.execPath, "-e", script, "--", home, "verifier", barrier],
133+
[process.execPath, "-e", script, "--", home, "client", barrier],
122134
{
123135
stdout: "ignore",
124136
stderr: "pipe",
@@ -135,27 +147,33 @@ describe("mcp auth-store", () => {
135147

136148
const final = await loadAuthState(linear, home);
137149
expect(final.tokens?.access_token).toBe("tok");
138-
expect(final.codeVerifier).toBe("verifier-from-other-session");
150+
expect(final.clientInformation?.client_name).toBe("other-session");
139151
expect(final.clientInformation?.client_id).toBe("c1");
140152
});
141153

142154
test("concurrent saveAuthState calls do not throw ENOENT on temp rename", async () => {
143155
const home = await tempHome();
144156
await Promise.all(
145157
Array.from({ length: 20 }, (_, i) =>
146-
saveAuthState(linear, { codeVerifier: `v${String(i)}` }, home),
158+
saveAuthState(
159+
linear,
160+
{
161+
tokens: { access_token: `v${String(i)}`, token_type: "bearer" },
162+
},
163+
home,
164+
),
147165
),
148166
);
149167
const final = await loadAuthState(linear, home);
150-
expect(final.codeVerifier?.startsWith("v")).toBe(true);
168+
expect(final.tokens?.access_token?.startsWith("v")).toBe(true);
151169
// No leftover temp files from failed renames.
152170
const dir = join(home, ".corbits", "mcp-auth");
153171
const files = await Array.fromAsync(
154172
new Bun.Glob("linear-*.json").scan(dir),
155173
);
156174
expect(files).toHaveLength(1);
157175
const raw = await readFile(join(dir, files[0] ?? "missing"), "utf8");
158-
expect(JSON.parse(raw).codeVerifier).toBe(final.codeVerifier);
176+
expect(JSON.parse(raw).tokens).toEqual(final.tokens);
159177
});
160178

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

178-
await saveAuthState(originA, { codeVerifier: "only-a" }, home);
196+
await saveAuthState(
197+
originA,
198+
{ tokens: { access_token: "only-a", token_type: "bearer" } },
199+
home,
200+
);
179201

180-
expect((await loadAuthState(originA, home)).codeVerifier).toBe("only-a");
202+
expect((await loadAuthState(originA, home)).tokens?.access_token).toBe(
203+
"only-a",
204+
);
181205
expect(await loadAuthState(originB, home)).toEqual({});
182206
expect(await loadAuthState(pathB, home)).toEqual({});
183207
expect(await loadAuthState(queryB, home)).toEqual({});
184-
expect((await loadAuthState(equivalent, home)).codeVerifier).toBe("only-a");
208+
expect((await loadAuthState(equivalent, home)).tokens?.access_token).toBe(
209+
"only-a",
210+
);
185211
});
186212

187213
test("ignores legacy name-only auth state without modifying it", async () => {
@@ -204,14 +230,42 @@ describe("mcp auth-store", () => {
204230
});
205231
});
206232

233+
test("updateAuthState strips a legacy PKCE verifier instead of persisting it", async () => {
234+
const home = await tempHome();
235+
const path = authFilePath(linear, home);
236+
await mkdir(dirname(path), { recursive: true });
237+
await writeFile(
238+
path,
239+
JSON.stringify({
240+
tokens: { access_token: "tok", token_type: "bearer" },
241+
codeVerifier: "legacy-secret",
242+
}),
243+
);
244+
245+
const written = await updateAuthState(
246+
linear,
247+
(state) => {
248+
state.tokens = { access_token: "tok2", token_type: "bearer" };
249+
},
250+
home,
251+
);
252+
253+
expect(written.tokens?.access_token).toBe("tok2");
254+
expect("codeVerifier" in written).toBe(false);
255+
expect(await readFile(path, "utf8")).not.toContain("codeVerifier");
256+
expect((await loadAuthState(linear, home)).tokens?.access_token).toBe(
257+
"tok2",
258+
);
259+
});
260+
207261
test("bounds the display slug without weakening scoped identity", async () => {
208262
const home = await tempHome();
209263
const dir = join(home, ".corbits", "mcp-auth");
210264
const prefixName = "a".repeat(48);
211265
const longName = `${prefixName}/long`;
212266
await saveAuthState(
213267
{ serverName: longName, serverURL: "https://long.example/mcp" },
214-
{ codeVerifier: "scoped-secret" },
268+
{ tokens: { access_token: "scoped-secret", token_type: "bearer" } },
215269
home,
216270
);
217271

@@ -225,8 +279,14 @@ describe("mcp auth-store", () => {
225279

226280
test("deleteAuthState removes an existing file", async () => {
227281
const home = await tempHome();
228-
await saveAuthState(linear, { codeVerifier: "secret" }, home);
229-
expect((await loadAuthState(linear, home)).codeVerifier).toBe("secret");
282+
await saveAuthState(
283+
linear,
284+
{ tokens: { access_token: "secret", token_type: "bearer" } },
285+
home,
286+
);
287+
expect((await loadAuthState(linear, home)).tokens?.access_token).toBe(
288+
"secret",
289+
);
230290

231291
await deleteAuthState(linear, home);
232292
expect(await loadAuthState(linear, home)).toEqual({});
@@ -241,13 +301,17 @@ describe("mcp auth-store", () => {
241301

242302
test("concurrent updateAuthState and deleteAuthState do not throw ENOENT on rename", async () => {
243303
const home = await tempHome();
244-
await saveAuthState(linear, { codeVerifier: "v0" }, home);
304+
await saveAuthState(
305+
linear,
306+
{ tokens: { access_token: "v0", token_type: "bearer" } },
307+
home,
308+
);
245309

246310
await Promise.all([
247311
updateAuthState(
248312
linear,
249313
(state) => {
250-
state.codeVerifier = "v1";
314+
state.tokens = { access_token: "v1", token_type: "bearer" };
251315
},
252316
home,
253317
),
@@ -256,7 +320,7 @@ describe("mcp auth-store", () => {
256320

257321
const final = await loadAuthState(linear, home);
258322
expect(
259-
final.codeVerifier === undefined || final.codeVerifier === "v1",
323+
final.tokens === undefined || final.tokens.access_token === "v1",
260324
).toBe(true);
261325
});
262326
});

‎src/mcp/auth-store.ts‎

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,14 @@ import type {
1818
} from "@modelcontextprotocol/sdk/shared/auth.js";
1919
import { SETTINGS_DIR_NAME } from "../branding.js";
2020

21-
// Per-server OAuth state persisted between sessions. Holding the PKCE verifier is
22-
// necessary because authorization spans a process boundary (browser round-trip);
23-
// tokens and dynamically-registered client info let later sessions reconnect
24-
// without any user interaction.
21+
// Per-server OAuth state persisted between sessions. Only the durable pair —
22+
// dynamically-registered client info and tokens — reaches disk; the PKCE
23+
// verifier is instance-local transient memory in the OAuth provider, since the
24+
// browser flow starts and finishes inside one provider episode. Legacy files
25+
// may still carry a codeVerifier key, which is stripped on write.
2526
export interface MCPAuthState {
2627
clientInformation?: OAuthClientInformationFull;
2728
tokens?: OAuthTokens;
28-
codeVerifier?: string;
2929
}
3030

3131
export interface MCPAuthIdentity {
@@ -67,7 +67,7 @@ export function authFilePath(
6767
);
6868
}
6969

70-
function isEnoent(err: unknown): boolean {
70+
export function isEnoent(err: unknown): boolean {
7171
return (
7272
typeof err === "object" &&
7373
err !== null &&
@@ -150,7 +150,7 @@ let tmpWriteCounter = 0;
150150

151151
// Serialize read-modify-write per auth file so two OAuth provider instances for
152152
// the same server cannot clobber each other's fields (classic lost-update: one
153-
// session's saveCodeVerifier wiping another's just-written tokens).
153+
// session's saveTokens wiping another's just-saved client registration).
154154
const updateChains = new Map<string, Promise<unknown>>();
155155

156156
const LOCK_STALE_MS = 5_000;
@@ -220,10 +220,22 @@ function enqueueAuthFileOp<T>(path: string, op: () => Promise<T>): Promise<T> {
220220
return run;
221221
}
222222

223+
// Legacy auth files may carry a codeVerifier key from when the PKCE verifier
224+
// flowed through disk. The verifier is instance-local transient memory now —
225+
// strip the key so it never reaches disk again.
226+
function withoutLegacyVerifier(state: MCPAuthState): MCPAuthState {
227+
if (!("codeVerifier" in state)) return state;
228+
const copy: Record<string, unknown> = { ...state };
229+
delete copy.codeVerifier;
230+
return copy as MCPAuthState;
231+
}
232+
223233
async function writeAuthFile(path: string, state: MCPAuthState): Promise<void> {
224234
await mkdir(dirname(path), { recursive: true, mode: 0o700 });
225235
const tmp = `${path}.${process.pid}.${(tmpWriteCounter += 1)}.tmp`;
226-
await writeFile(tmp, JSON.stringify(state, null, 2), { mode: 0o600 });
236+
await writeFile(tmp, JSON.stringify(withoutLegacyVerifier(state), null, 2), {
237+
mode: 0o600,
238+
});
227239
await rename(tmp, path);
228240
}
229241

@@ -250,8 +262,9 @@ export async function updateAuthState(
250262
return enqueueAuthFileOp(path, async () => {
251263
const state = await loadAuthState(identity, home);
252264
mutator(state);
253-
await writeAuthFile(path, state);
254-
return state;
265+
const durable = withoutLegacyVerifier(state);
266+
await writeAuthFile(path, durable);
267+
return durable;
255268
});
256269
}
257270

0 commit comments

Comments
 (0)