Keep concurrent MCP OAuth writes from wiping tokens

src/mcp/auth-store.test.ts

@@ -0,0 +1,60 @@
+import { describe, expect, test } from "bun:test";
+import { mkdtemp, readFile } from "node:fs/promises";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { loadAuthState, saveAuthState, updateAuthState } from "./auth-store.js";
+
+async function tempHome(): Promise<string> {
+  return mkdtemp(join(tmpdir(), "mcp-auth-"));
+}
+
+describe("mcp auth-store", () => {
+  test("updateAuthState merges concurrent field writes without losing tokens", 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);
+
+    // 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.
+    const writes = await Promise.all([
+      updateAuthState("linear", (state) => {
+        state.tokens = {
+          access_token: "tok",
+          token_type: "bearer",
+          expires_in: 3600,
+          refresh_token: "ref",
+        };
+      }, home),
+      updateAuthState("linear", (state) => {
+        state.codeVerifier = "verifier-from-other-session";
+      }, 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(writes[0].tokens?.access_token === "tok" || writes[1].tokens?.access_token === "tok").toBe(true);
+  });
+
+  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),
+      ),
+    );
+    const final = await loadAuthState("linear", home);
+    expect(final.codeVerifier?.startsWith("v")).toBe(true);
+    // No leftover temp files from failed renames.
+    const dir = join(home, ".corbits", "mcp-auth");
+    const raw = await readFile(join(dir, "linear.json"), "utf8");
+    expect(JSON.parse(raw).codeVerifier).toBe(final.codeVerifier);
+  });
+});

src/mcp/auth-store.ts

@@ -1,6 +1,6 @@
 import { mkdir, readFile, rename, writeFile } from "node:fs/promises";
 import { homedir } from "node:os";
-import { join } from "node:path";
+import { dirname, join } from "node:path";
 import type { OAuthClientInformationFull, OAuthTokens } from "@modelcontextprotocol/sdk/shared/auth.js";
 import { SETTINGS_DIR_NAME } from "../branding.js";
 
@@ -46,11 +46,59 @@ export async function loadAuthState(serverName: string, home: string = homedir()
   return {};
 }
 
+// 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;
+
+// Serialize read-modify-write per auth file so two OAuth provider instances for
+// the same server cannot clobber each other's fields (classic lost-update: one
+// session's saveCodeVerifier wiping another's just-written tokens).
+const updateChains = new Map<string, Promise<unknown>>();
+
+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`;
+  await writeFile(tmp, JSON.stringify(state, null, 2), { mode: 0o600 });
+  await rename(tmp, path);
+}
+
 // Tokens are credentials, so the directory and file are restricted to the owner.
+// Full replace — prefer updateAuthState when mutating a single field so concurrent
+// writers merge instead of last-writer-wins on a stale snapshot.
 export async function saveAuthState(serverName: string, state: MCPAuthState, home: string = homedir()): Promise<void> {
   const path = authFilePath(serverName, home);
-  await mkdir(mcpAuthDir(home), { recursive: true, mode: 0o700 });
-  const tmp = `${path}.${process.pid}.tmp`;
-  await writeFile(tmp, JSON.stringify(state, null, 2), { mode: 0o600 });
-  await rename(tmp, path);
+  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;
+}
+
+// Load → mutate → save under the per-file chain. Mutator receives a mutable
+// snapshot of the latest on-disk state; the returned object is what was written.
+export async function updateAuthState(
+  serverName: string,
+  mutator: (state: MCPAuthState) => void,
+  home: string = homedir(),
+): Promise<MCPAuthState> {
+  const path = authFilePath(serverName, home);
+  const previous = updateChains.get(path) ?? Promise.resolve();
+  const run = previous.then(
+    async () => {
+      const state = await loadAuthState(serverName, home);
+      mutator(state);
+      await writeAuthFile(path, state);
+      return state;
+    },
+    async () => {
+      const state = await loadAuthState(serverName, home);
+      mutator(state);
+      await writeAuthFile(path, state);
+      return state;
+    },
+  );
+  updateChains.set(path, run.then(() => undefined, () => undefined));
+  return run;
 }

src/mcp/oauth-provider.test.ts

@@ -0,0 +1,130 @@
+import { describe, expect, test } from "bun:test";
+import { mkdtemp } from "node:fs/promises";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { loadAuthState, saveAuthState } from "./auth-store.js";
+import { createOAuthProvider } from "./oauth-provider.js";
+
+async function tempHome(): Promise<string> {
+  return mkdtemp(join(tmpdir(), "mcp-oauth-"));
+}
+
+const clientInfo = (port: number) => ({
+  client_id: `client-on-${String(port)}`,
+  redirect_uris: [`http://127.0.0.1:${String(port)}/callback`],
+  client_id_issued_at: 1,
+  token_endpoint_auth_method: "none" as const,
+  grant_types: ["authorization_code", "refresh_token"],
+  response_types: ["code"],
+  client_name: "interchange-code",
+});
+
+async function syncValue<T>(value: T | Promise<T>): Promise<T> {
+  return await value;
+}
+
+describe("createOAuthProvider", () => {
+  test("drops stale DCR client when redirect port changed and no tokens exist", async () => {
+    const home = await tempHome();
+    await saveAuthState("linear", {
+      clientInformation: clientInfo(60435),
+      codeVerifier: "old-verifier",
+    }, home);
+
+    const provider = await createOAuthProvider({
+      serverName: "linear",
+      redirectUrl: "http://127.0.0.1:62000/callback",
+      onAuthURL: () => undefined,
+      home,
+    });
+
+    expect(await syncValue(provider.clientInformation())).toBeUndefined();
+    const disk = await loadAuthState("linear", home);
+    expect(disk.clientInformation).toBeUndefined();
+    expect(disk.codeVerifier).toBeUndefined();
+  });
+
+  test("keeps registered client and tokens when only the loopback port changed", async () => {
+    const home = await tempHome();
+    await saveAuthState("linear", {
+      clientInformation: clientInfo(60435),
+      tokens: {
+        access_token: "live",
+        token_type: "bearer",
+        expires_in: 3600,
+        refresh_token: "refresh",
+      },
+    }, home);
+
+    const provider = await createOAuthProvider({
+      serverName: "linear",
+      redirectUrl: "http://127.0.0.1:62000/callback",
+      onAuthURL: () => undefined,
+      home,
+    });
+
+    expect((await syncValue(provider.clientInformation()))?.client_id).toBe("client-on-60435");
+    expect((await syncValue(provider.tokens()))?.access_token).toBe("live");
+  });
+
+  test("concurrent saveTokens and saveCodeVerifier from two providers keep both fields", async () => {
+    const home = await tempHome();
+    await saveAuthState("linear", { clientInformation: clientInfo(1) }, home);
+
+    const a = await createOAuthProvider({
+      serverName: "linear",
+      redirectUrl: "http://127.0.0.1:1/callback",
+      onAuthURL: () => undefined,
+      home,
+    });
+    const b = await createOAuthProvider({
+      serverName: "linear",
+      redirectUrl: "http://127.0.0.1:1/callback",
+      onAuthURL: () => undefined,
+      home,
+    });
+
+    await Promise.all([
+      a.saveTokens({
+        access_token: "tok-a",
+        token_type: "bearer",
+        expires_in: 60,
+        refresh_token: "ref-a",
+      }),
+      b.saveCodeVerifier("verifier-b"),
+    ]);
+
+    const disk = await loadAuthState("linear", home);
+    expect(disk.tokens?.access_token).toBe("tok-a");
+    expect(disk.codeVerifier).toBe("verifier-b");
+  });
+
+  test("resetAuthorization clears client when redirect no longer matches registration", async () => {
+    const home = await tempHome();
+    await saveAuthState("linear", {
+      clientInformation: clientInfo(60435),
+      tokens: {
+        access_token: "live",
+        token_type: "bearer",
+        expires_in: 1,
+        refresh_token: "r",
+      },
+      codeVerifier: "v",
+    }, home);
+
+    const provider = await createOAuthProvider({
+      serverName: "linear",
+      redirectUrl: "http://127.0.0.1:62000/callback",
+      onAuthURL: () => undefined,
+      home,
+    });
+
+    // Tokens present → client kept at create. Reset simulates failed refresh.
+    await provider.resetAuthorization();
+    expect(await syncValue(provider.tokens())).toBeUndefined();
+    expect(await syncValue(provider.clientInformation())).toBeUndefined();
+    const disk = await loadAuthState("linear", home);
+    expect(disk.clientInformation).toBeUndefined();
+    expect(disk.tokens).toBeUndefined();
+  });
+});

src/mcp/oauth-provider.ts

@@ -1,6 +1,6 @@
 import type { OAuthClientProvider } from "@modelcontextprotocol/sdk/client/auth.js";
 import type { OAuthClientInformationFull, OAuthClientInformationMixed, OAuthClientMetadata, OAuthTokens } from "@modelcontextprotocol/sdk/shared/auth.js";
-import { loadAuthState, saveAuthState, type MCPAuthState } from "./auth-store.js";
+import { updateAuthState, type MCPAuthState } from "./auth-store.js";
 import { MCP_CLIENT_NAME } from "../branding.js";
 
 export type OAuthProviderOptions = {
@@ -12,9 +12,45 @@ export type OAuthProviderOptions = {
 };
 export type CorbitsOAuthProvider = OAuthClientProvider & { resetAuthorization(): Promise<void> };
 
+function redirectUrisInclude(info: OAuthClientInformationFull | undefined, redirectUrl: string): boolean {
+  const uris = info?.redirect_uris;
+  if (uris === undefined || uris.length === 0) return true;
+  return uris.includes(redirectUrl);
+}
+
+// Dynamic client registration bakes in the loopback redirect_uri (ephemeral port).
+// A later session that binds a new port cannot reuse that client_id for authorize
+// / token exchange — drop the stale registration when we have no refreshable
+// tokens and must run the browser flow again.
+function dropStaleClientRegistration(state: MCPAuthState, redirectUrl: string): void {
+  if (state.tokens !== undefined) return;
+  if (redirectUrisInclude(state.clientInformation, redirectUrl)) return;
+  delete state.clientInformation;
+  delete state.codeVerifier;
+}
+
+function replaceStored(stored: MCPAuthState, next: MCPAuthState): void {
+  delete stored.clientInformation;
+  delete stored.tokens;
+  delete stored.codeVerifier;
+  if (next.clientInformation !== undefined) stored.clientInformation = next.clientInformation;
+  if (next.tokens !== undefined) stored.tokens = next.tokens;
+  if (next.codeVerifier !== undefined) stored.codeVerifier = next.codeVerifier;
+}
+
 export async function createOAuthProvider(opts: OAuthProviderOptions): Promise<CorbitsOAuthProvider> {
-  const stored: MCPAuthState = await loadAuthState(opts.serverName, opts.home);
-  const persist = (): Promise<void> => saveAuthState(opts.serverName, stored, opts.home);
+  // Load + scrub stale DCR under the per-file chain so concurrent providers see
+  // the same cleaned state. Mutations always re-read disk; this in-memory mirror
+  // only serves the SDK's sync getters (tokens / clientInformation / codeVerifier).
+  const stored: MCPAuthState = await updateAuthState(opts.serverName, (state) => {
+    dropStaleClientRegistration(state, opts.redirectUrl);
+  }, opts.home);
+
+  const apply = async (mutator: (state: MCPAuthState) => void): Promise<void> => {
+    const next = await updateAuthState(opts.serverName, mutator, opts.home);
+    replaceStored(stored, next);
+  };
+
   let oauthState: string | undefined;
   return {
     get redirectUrl(): string { return opts.redirectUrl; },
@@ -27,32 +63,40 @@ export async function createOAuthProvider(opts: OAuthProviderOptions): Promise<C
     },
     clientInformation(): OAuthClientInformationMixed | undefined { return stored.clientInformation; },
     saveClientInformation(info: OAuthClientInformationMixed): Promise<void> {
-      stored.clientInformation = info as OAuthClientInformationFull;
-      return persist();
+      return apply((state) => {
+        state.clientInformation = info as OAuthClientInformationFull;
+      });
     },
     tokens(): OAuthTokens | undefined { return stored.tokens; },
     saveTokens(tokens: OAuthTokens): Promise<void> {
-      stored.tokens = tokens;
-      return persist();
+      return apply((state) => {
+        state.tokens = tokens;
+      });
     },
     redirectToAuthorization(authorizationUrl: URL): void {
       const state = authorizationUrl.searchParams.get("state");
       if (state !== null) opts.onAuthorizationState?.(state);
       opts.onAuthURL(opts.serverName, authorizationUrl.toString());
     },
     saveCodeVerifier(codeVerifier: string): Promise<void> {
-      stored.codeVerifier = codeVerifier;
-      return persist();
+      return apply((state) => {
+        state.codeVerifier = codeVerifier;
+      });
     },
     codeVerifier(): string {
       if (stored.codeVerifier === undefined) throw new Error("No PKCE code verifier saved for this authorization.");
       return stored.codeVerifier;
     },
     resetAuthorization(): Promise<void> {
-      delete stored.tokens;
-      delete stored.codeVerifier;
       oauthState = undefined;
-      return persist();
+      return apply((state) => {
+        delete state.tokens;
+        delete state.codeVerifier;
+        // Next browser flow needs a client registered for *this* loopback port.
+        if (!redirectUrisInclude(state.clientInformation, opts.redirectUrl)) {
+          delete state.clientInformation;
+        }
+      });
     },
   };
 }