Merge pull request #861 from corbitsdev/cl-7547-keep-mcp-oauth-tokens-after-browser-auth-across-sessions

Keep MCP OAuth tokens after browser auth across sessions

Author: TheGreatAxios

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(

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<string, Promise<unknown>>();
 
+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<T>(path: string, op: () => Promise<T>): Promise<T> {
+  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<T>(path: string, op: () => Promise<T>): Promise<T> {
+  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<void> {
   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<void> {
   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,49 +231,20 @@ export async function updateAuthState(
   home: string = homedir(),
 ): Promise<MCPAuthState> {
   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(
   identity: MCPAuthIdentity,
   home: string = homedir(),
 ): Promise<void> {
   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<void> {

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";

src/mcp/oauth-provider.ts

@@ -198,6 +198,7 @@ export async function createOAuthProvider(
       return stored.tokens;
     },
     saveTokens(tokens: OAuthTokens): Promise<void> {
+      stored.tokens = tokens;
       return apply((state) => {
         state.tokens = tokens;
       });
@@ -220,8 +221,14 @@ export async function createOAuthProvider(
     },
     async resetAuthorization(): Promise<void> {
       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)) {