From efabd232a44d9f026a30470d58a2cd2e9a60b965 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Wed, 9 Sep 2026 20:17:05 -0700 Subject: [PATCH] Keep MCP OAuth tokens across overlapping session writes --- src/mcp/auth-store.test.ts | 63 ++++++++++++++++ src/mcp/auth-store.ts | 128 ++++++++++++++++++++------------- src/mcp/oauth-provider.test.ts | 31 ++++++++ src/mcp/oauth-provider.ts | 9 ++- 4 files changed, 180 insertions(+), 51 deletions(-) diff --git a/src/mcp/auth-store.test.ts b/src/mcp/auth-store.test.ts index a3c7f8507..db9b78d99 100644 --- a/src/mcp/auth-store.test.ts +++ b/src/mcp/auth-store.test.ts @@ -59,6 +59,69 @@ describe("mcp auth-store", () => { ).toBe(true); }); + test("overlapping updateAuthState from two processes keeps tokens and PKCE", async () => { + const home = await tempHome(); + await saveAuthState( + linear, + { + clientInformation: { + client_id: "c1", + redirect_uris: ["http://127.0.0.1:1/callback"], + client_id_issued_at: 1, + }, + }, + home, + ); + + const storePath = join(import.meta.dirname, "auth-store.ts"); + const barrier = join(home, "start"); + const script = ` + import { updateAuthState } from ${JSON.stringify(storePath)}; + const home = process.argv[1]; + const field = process.argv[2]; + const barrier = process.argv[3]; + const identity = { serverName: "linear", serverURL: "https://mcp.linear.app/mcp" }; + while (!(await Bun.file(barrier).exists())) await Bun.sleep(5); + await updateAuthState( + identity, + (state) => { + Bun.sleepSync(150); + if (field === "tokens") { + state.tokens = { + access_token: "tok", + token_type: "bearer", + expires_in: 3600, + refresh_token: "ref", + }; + } else { + state.codeVerifier = "verifier-from-other-session"; + } + }, + home, + ); + `; + const processes = [ + Bun.spawn([process.execPath, "-e", script, "--", home, "tokens", barrier], { + stdout: "ignore", + stderr: "pipe", + }), + Bun.spawn([process.execPath, "-e", script, "--", home, "verifier", barrier], { + stdout: "ignore", + stderr: "pipe", + }), + ]; + await Bun.sleep(50); + await writeFile(barrier, "go"); + const exitCodes = await Promise.all(processes.map((child) => child.exited)); + const errors = await Promise.all(processes.map((child) => new Response(child.stderr).text())); + expect(exitCodes, errors.join("\n")).toEqual([0, 0]); + + 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"); + }); + test("concurrent saveAuthState calls do not throw ENOENT on temp rename", async () => { const home = await tempHome(); await Promise.all( diff --git a/src/mcp/auth-store.ts b/src/mcp/auth-store.ts index 37207ff92..fd67d93f4 100644 --- a/src/mcp/auth-store.ts +++ b/src/mcp/auth-store.ts @@ -1,8 +1,9 @@ import { createHash } from "node:crypto"; -import { mkdir, readFile, rename, unlink, writeFile } from "node:fs/promises"; +import { mkdir, open, readFile, rename, stat, unlink, writeFile } from "node:fs/promises"; import { readFileSync } from "node:fs"; import { homedir } from "node:os"; import { dirname, join } from "node:path"; +import { setTimeout as delay } from "node:timers/promises"; import type { OAuthClientInformationFull, OAuthTokens, @@ -126,6 +127,10 @@ export function tryLoadAuthStateSync( return parseAuthState(raw); } +function isEexist(err: unknown): boolean { + return typeof err === "object" && err !== null && "code" in err && err.code === "EEXIST"; +} + // pid alone is not unique per call — concurrent saves in one process must not // share a temp path or the second rename hits ENOENT after the first moves it. let tmpWriteCounter = 0; @@ -135,6 +140,70 @@ let tmpWriteCounter = 0; // session's saveCodeVerifier wiping another's just-written tokens). const updateChains = new Map>(); +const LOCK_STALE_MS = 5_000; +const LOCK_RETRY_MS = 25; + +async function acquireAuthFileLock(lockPath: string) { + while (true) { + try { + return await open(lockPath, "wx", 0o600); + } catch (err) { + if (!isEexist(err)) throw err; + try { + const info = await stat(lockPath); + if (Date.now() - info.mtimeMs > LOCK_STALE_MS) { + try { + await unlink(lockPath); + } catch (unlinkErr) { + if (!isEnoent(unlinkErr)) throw unlinkErr; + } + continue; + } + } catch (statErr) { + if (isEnoent(statErr)) continue; + throw statErr; + } + await delay(LOCK_RETRY_MS); + } + } +} + +async function withAuthFileLock(path: string, op: () => Promise): Promise { + const lockPath = `${path}.lock`; + await mkdir(dirname(path), { recursive: true, mode: 0o700 }); + const lock = await acquireAuthFileLock(lockPath); + try { + return await op(); + } finally { + try { + await lock.close(); + } catch { + // Close can fail if the handle was already torn down. + } + try { + await unlink(lockPath); + } catch { + // Missing lock is fine; a leftover file is recovered as stale. + } + } +} + +function enqueueAuthFileOp(path: string, op: () => Promise): Promise { + const previous = updateChains.get(path) ?? Promise.resolve(); + const run = previous.then( + () => withAuthFileLock(path, op), + () => withAuthFileLock(path, op), + ); + updateChains.set( + path, + run.then( + () => undefined, + () => undefined, + ), + ); + return run; +} + async function writeAuthFile(path: string, state: MCPAuthState): Promise { await mkdir(dirname(path), { recursive: true, mode: 0o700 }); const tmp = `${path}.${process.pid}.${(tmpWriteCounter += 1)}.tmp`; @@ -151,19 +220,7 @@ export async function saveAuthState( home: string = homedir(), ): Promise { const path = authFilePath(identity, home); - const previous = updateChains.get(path) ?? Promise.resolve(); - const write = previous.then( - () => writeAuthFile(path, state), - () => writeAuthFile(path, state), - ); - updateChains.set( - path, - write.then( - () => undefined, - () => undefined, - ), - ); - await write; + await enqueueAuthFileOp(path, () => writeAuthFile(path, state)); } // Load → mutate → save under the per-file chain. Mutator receives a mutable @@ -174,29 +231,12 @@ export async function updateAuthState( home: string = homedir(), ): Promise { const path = authFilePath(identity, home); - const previous = updateChains.get(path) ?? Promise.resolve(); - const run = previous.then( - async () => { - const state = await loadAuthState(identity, home); - mutator(state); - await writeAuthFile(path, state); - return state; - }, - async () => { - const state = await loadAuthState(identity, home); - mutator(state); - await writeAuthFile(path, state); - return state; - }, - ); - updateChains.set( - path, - run.then( - () => undefined, - () => undefined, - ), - ); - return run; + return enqueueAuthFileOp(path, async () => { + const state = await loadAuthState(identity, home); + mutator(state); + await writeAuthFile(path, state); + return state; + }); } export async function deleteAuthState( @@ -204,19 +244,7 @@ export async function deleteAuthState( home: string = homedir(), ): Promise { const path = authFilePath(identity, home); - const previous = updateChains.get(path) ?? Promise.resolve(); - const run = previous.then( - () => unlinkAuthFile(path), - () => unlinkAuthFile(path), - ); - updateChains.set( - path, - run.then( - () => undefined, - () => undefined, - ), - ); - await run; + await enqueueAuthFileOp(path, () => unlinkAuthFile(path)); } async function unlinkAuthFile(path: string): Promise { diff --git a/src/mcp/oauth-provider.test.ts b/src/mcp/oauth-provider.test.ts index 4328a401c..a06a1c6f0 100644 --- a/src/mcp/oauth-provider.test.ts +++ b/src/mcp/oauth-provider.test.ts @@ -160,6 +160,37 @@ describe("createOAuthProvider", () => { expect(disk.tokens).toBeUndefined(); }); + test("resetAuthorization does not delete a sibling's just-saved tokens", async () => { + const home = await tempHome(); + const a = await createOAuthProvider({ + serverName: "linear", + serverURL: linear.serverURL, + redirectUrl: "http://127.0.0.1:62000/callback", + onAuthURL: () => undefined, + home, + }); + const b = await createOAuthProvider({ + serverName: "linear", + serverURL: linear.serverURL, + redirectUrl: "http://127.0.0.1:60435/callback", + onAuthURL: () => undefined, + home, + }); + + await a.saveTokens({ + access_token: "fresh", + token_type: "bearer", + expires_in: 3600, + refresh_token: "fresh-refresh", + }); + expect((await syncValue(a.tokens()))?.access_token).toBe("fresh"); + + await b.resetAuthorization(); + + expect((await syncValue(a.tokens()))?.access_token).toBe("fresh"); + expect((await loadAuthState(linear, home)).tokens?.access_token).toBe("fresh"); + }); + test("isolates same-name providers by endpoint and persists the same identity", async () => { const home = await tempHome(); const customURL = "https://custom.example/mcp"; diff --git a/src/mcp/oauth-provider.ts b/src/mcp/oauth-provider.ts index 615a3e488..9ac838f4d 100644 --- a/src/mcp/oauth-provider.ts +++ b/src/mcp/oauth-provider.ts @@ -198,6 +198,7 @@ export async function createOAuthProvider( return stored.tokens; }, saveTokens(tokens: OAuthTokens): Promise { + stored.tokens = tokens; return apply((state) => { state.tokens = tokens; }); @@ -220,8 +221,14 @@ export async function createOAuthProvider( }, async resetAuthorization(): Promise { oauthState = undefined; + // Snapshot before the disk refresh so a session that never held tokens + // cannot adopt a sibling's credentials and then delete them. + const previous = stored.tokens?.access_token; + refreshDurableFromDisk(); await apply((state) => { - delete state.tokens; + if (state.tokens?.access_token === previous) { + delete state.tokens; + } delete state.codeVerifier; // Next browser flow needs a client registered for *this* loopback port. if (!redirectUrisInclude(state.clientInformation, opts.redirectUrl)) {