From 6ecc8a4b4adad111d4caee9a7a8a4b580802165c Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sun, 27 Sep 2026 22:34:12 -0700 Subject: [PATCH 1/3] fix(mcp): keep PKCE verifier in memory instead of on disk --- src/mcp/add-server.test.ts | 38 ++++++++--- src/mcp/auth-store.test.ts | 76 +++++++++++++++------- src/mcp/auth-store.ts | 11 ++-- src/mcp/oauth-provider.test.ts | 112 ++++++++++++++++++++++++++++++--- src/mcp/oauth-provider.ts | 26 ++++---- tests/unit/mcp.test.ts | 7 +-- 6 files changed, 206 insertions(+), 64 deletions(-) diff --git a/src/mcp/add-server.test.ts b/src/mcp/add-server.test.ts index 0fa6fab61..f03cc18f9 100644 --- a/src/mcp/add-server.test.ts +++ b/src/mcp/add-server.test.ts @@ -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( @@ -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", ); @@ -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); @@ -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({ @@ -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); @@ -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( @@ -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); diff --git a/src/mcp/auth-store.test.ts b/src/mcp/auth-store.test.ts index 5a7ef057e..7e32b9a5a 100644 --- a/src/mcp/auth-store.test.ts +++ b/src/mcp/auth-store.test.ts @@ -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, @@ -52,7 +52,11 @@ 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, ), @@ -60,15 +64,14 @@ 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"); 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, @@ -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, @@ -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", @@ -135,19 +142,22 @@ 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( @@ -155,7 +165,9 @@ describe("mcp auth-store", () => { ); 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 () => { @@ -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 () => { @@ -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, ); @@ -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({}); @@ -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, ), @@ -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); }); }); diff --git a/src/mcp/auth-store.ts b/src/mcp/auth-store.ts index aa55de7a7..aa11348f2 100644 --- a/src/mcp/auth-store.ts +++ b/src/mcp/auth-store.ts @@ -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 { diff --git a/src/mcp/oauth-provider.test.ts b/src/mcp/oauth-provider.test.ts index 79c777434..c63d1133e 100644 --- a/src/mcp/oauth-provider.test.ts +++ b/src/mcp/oauth-provider.test.ts @@ -3,7 +3,7 @@ import * as fs from "node:fs"; import { readFileSync } from "node:fs"; import { chmod, mkdir, mkdtemp, readFile, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { dirname, join } from "node:path"; import { UnauthorizedError } from "@modelcontextprotocol/sdk/client/auth.js"; import { authFilePath, @@ -49,13 +49,13 @@ async function saveClient( describe("createOAuthProvider", () => { test("drops stale DCR client when redirect port changed and no tokens exist", async () => { const home = await tempHome(); - await saveAuthState( - linear, - { + await mkdir(dirname(authFilePath(linear, home)), { recursive: true }); + await writeFile( + authFilePath(linear, home), + JSON.stringify({ clientInformation: clientInfo(60435), codeVerifier: "old-verifier", - }, - home, + }), ); const provider = await createOAuthProvider({ @@ -67,9 +67,11 @@ describe("createOAuthProvider", () => { }); expect(await syncValue(provider.clientInformation())).toBeUndefined(); + expect(() => provider.codeVerifier()).toThrow( + "No PKCE code verifier saved", + ); 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 () => { @@ -102,7 +104,7 @@ describe("createOAuthProvider", () => { expect((await syncValue(provider.tokens()))?.access_token).toBe("live"); }); - test("concurrent saveTokens and saveCodeVerifier from two providers keep both fields", async () => { + test("saveCodeVerifier stays in memory and never reaches disk", async () => { const home = await tempHome(); await saveAuthState(linear, { clientInformation: clientInfo(1) }, home); @@ -133,7 +135,30 @@ describe("createOAuthProvider", () => { const disk = await loadAuthState(linear, home); expect(disk.tokens?.access_token).toBe("tok-a"); - expect(disk.codeVerifier).toBe("verifier-b"); + expect("codeVerifier" in disk).toBe(false); + expect(await readFile(authFilePath(linear, home), "utf8")).not.toContain( + "codeVerifier", + ); + expect(b.codeVerifier()).toBe("verifier-b"); + expect(() => a.codeVerifier()).toThrow("No PKCE code verifier saved"); + }); + + test("saveCodeVerifier leaves the auth file bytes unchanged", async () => { + const home = await tempHome(); + const provider = await createOAuthProvider({ + serverName: "linear", + serverURL: linear.serverURL, + redirectUrl: "http://127.0.0.1:1/callback", + onAuthURL: () => undefined, + home, + }); + await saveClient(provider, clientInfo(1)); + + const before = await readFile(authFilePath(linear, home), "utf8"); + await provider.saveCodeVerifier("pkce-local"); + + expect(provider.codeVerifier()).toBe("pkce-local"); + expect(await readFile(authFilePath(linear, home), "utf8")).toBe(before); }); test("resetAuthorization clears client when redirect no longer matches registration", async () => { @@ -148,7 +173,6 @@ describe("createOAuthProvider", () => { expires_in: 1, refresh_token: "r", }, - codeVerifier: "v", }, home, ); @@ -160,14 +184,57 @@ describe("createOAuthProvider", () => { onAuthURL: () => undefined, home, }); + await provider.saveCodeVerifier("v"); // 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(); + expect(() => provider.codeVerifier()).toThrow( + "No PKCE code verifier saved", + ); const disk = await loadAuthState(linear, home); expect(disk.clientInformation).toBeUndefined(); expect(disk.tokens).toBeUndefined(); + expect(await readFile(authFilePath(linear, home), "utf8")).not.toContain( + "codeVerifier", + ); + }); + + test("resetAuthorization scrubs a legacy on-disk verifier", async () => { + const home = await tempHome(); + await mkdir(dirname(authFilePath(linear, home)), { recursive: true }); + await writeFile( + authFilePath(linear, home), + JSON.stringify({ + clientInformation: clientInfo(60435), + tokens: { + access_token: "live", + token_type: "bearer", + expires_in: 1, + refresh_token: "r", + }, + codeVerifier: "legacy-verifier", + }), + ); + + const provider = await createOAuthProvider({ + serverName: "linear", + serverURL: linear.serverURL, + redirectUrl: "http://127.0.0.1:62000/callback", + onAuthURL: () => undefined, + home, + }); + await provider.saveCodeVerifier("memory-verifier"); + + await provider.resetAuthorization(); + + expect(() => provider.codeVerifier()).toThrow( + "No PKCE code verifier saved", + ); + expect(await readFile(authFilePath(linear, home), "utf8")).not.toContain( + "codeVerifier", + ); }); test("resetAuthorization does not delete a sibling's just-saved tokens", async () => { @@ -321,6 +388,31 @@ describe("createOAuthProvider", () => { await b.saveCodeVerifier("pkce-b"); expect(a.codeVerifier()).toBe("pkce-a"); expect(b.codeVerifier()).toBe("pkce-b"); + expect(await readFile(authFilePath(linear, home), "utf8")).not.toContain( + "codeVerifier", + ); + }); + + test("keeps live tokens when a sibling writes client-only state", 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, + }); + await saveClient(a, clientInfo(62000)); + await a.saveTokens({ + access_token: "tok-a", + token_type: "bearer", + expires_in: 3600, + refresh_token: "ref-a", + }); + + await saveAuthState(linear, { clientInformation: clientInfo(60435) }, home); + + expect((await syncValue(a.tokens()))?.access_token).toBe("tok-a"); }); test("does not adopt a different-port sibling DCR client without tokens", async () => { diff --git a/src/mcp/oauth-provider.ts b/src/mcp/oauth-provider.ts index 1878843b8..8fdcc1c48 100644 --- a/src/mcp/oauth-provider.ts +++ b/src/mcp/oauth-provider.ts @@ -70,7 +70,6 @@ function dropStaleClientRegistration( if (state.tokens !== undefined) return; if (redirectUrisInclude(state.clientInformation, redirectUrl)) return; delete state.clientInformation; - delete state.codeVerifier; } function shouldAdoptClient( @@ -90,7 +89,6 @@ function shouldAdoptClient( function assignTokens(stored: MCPAuthState, next: MCPAuthState): void { if (next.tokens !== undefined) stored.tokens = next.tokens; - else delete stored.tokens; } function assignClient(stored: MCPAuthState, next: MCPAuthState): void { @@ -132,13 +130,15 @@ export async function createOAuthProvider( // DCR are observed from disk so a sibling session's completed auth is picked // up. PKCE stays instance-local after this snapshot — a different-port sibling // must not clobber an in-progress verifier. - const stored: MCPAuthState = await updateAuthState( - identity, - (state) => { - dropStaleClientRegistration(state, opts.redirectUrl); - }, - home, - ); + const stored: MCPAuthState & { codeVerifier?: string } = + await updateAuthState( + identity, + (state) => { + dropStaleClientRegistration(state, opts.redirectUrl); + }, + home, + ); + delete stored.codeVerifier; const apply = async ( mutator: (state: MCPAuthState) => void, @@ -229,9 +229,7 @@ export async function createOAuthProvider( }, saveCodeVerifier(codeVerifier: string): Promise { stored.codeVerifier = codeVerifier; - return apply((state) => { - state.codeVerifier = codeVerifier; - }); + return Promise.resolve(); }, codeVerifier(): string { if (stored.codeVerifier === undefined) @@ -248,12 +246,14 @@ export async function createOAuthProvider( if (state.tokens?.access_token === previous) { delete state.tokens; } - delete state.codeVerifier; + // One-version migration: drop a verifier persisted by older builds. + delete (state as { codeVerifier?: string }).codeVerifier; // Next browser flow needs a client registered for *this* loopback port. if (!redirectUrisInclude(state.clientInformation, opts.redirectUrl)) { delete state.clientInformation; } }); + if (stored.tokens?.access_token === previous) delete stored.tokens; delete stored.codeVerifier; }, refreshToken: async (refreshToken: string): Promise => { diff --git a/tests/unit/mcp.test.ts b/tests/unit/mcp.test.ts index d79d57e30..050923936 100644 --- a/tests/unit/mcp.test.ts +++ b/tests/unit/mcp.test.ts @@ -373,14 +373,9 @@ describe("MCP auth store", () => { const home = await mkdtemp(join(tmpdir(), "intx-auth-")); try { expect(await loadAuthState(acmeAuthIdentity, home)).toEqual({}); - await saveAuthState( - acmeAuthIdentity, - { tokens, codeVerifier: "verifier" }, - home, - ); + await saveAuthState(acmeAuthIdentity, { tokens }, home); expect(await loadAuthState(acmeAuthIdentity, home)).toEqual({ tokens, - codeVerifier: "verifier", }); } finally { await rm(home, { recursive: true, force: true }); From 9f9c6ba7922535e44b4cb1dd65474e9280d408f1 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Mon, 28 Sep 2026 08:15:43 -0700 Subject: [PATCH 2/3] fix(mcp): propagate sibling logout and scrub legacy verifier at startup --- src/mcp/oauth-provider.test.ts | 75 +++++++++++++++++++++++++++++++++- src/mcp/oauth-provider.ts | 8 +++- 2 files changed, 81 insertions(+), 2 deletions(-) diff --git a/src/mcp/oauth-provider.test.ts b/src/mcp/oauth-provider.test.ts index c63d1133e..a9aea7bc8 100644 --- a/src/mcp/oauth-provider.test.ts +++ b/src/mcp/oauth-provider.test.ts @@ -72,6 +72,7 @@ describe("createOAuthProvider", () => { ); const disk = await loadAuthState(linear, home); expect(disk.clientInformation).toBeUndefined(); + expect((disk as { codeVerifier?: string }).codeVerifier).toBeUndefined(); }); test("keeps registered client and tokens when only the loopback port changed", async () => { @@ -237,6 +238,38 @@ describe("createOAuthProvider", () => { ); }); + test("saveTokens does not re-persist a legacy on-disk verifier", async () => { + const home = await tempHome(); + await mkdir(dirname(authFilePath(linear, home)), { recursive: true }); + await writeFile( + authFilePath(linear, home), + JSON.stringify({ + clientInformation: clientInfo(62000), + codeVerifier: "legacy-verifier", + }), + ); + + const provider = await createOAuthProvider({ + serverName: "linear", + serverURL: linear.serverURL, + redirectUrl: "http://127.0.0.1:62000/callback", + onAuthURL: () => undefined, + home, + }); + await provider.saveTokens({ + access_token: "tok-a", + token_type: "bearer", + expires_in: 3600, + refresh_token: "ref-a", + }); + + const raw = await readFile(authFilePath(linear, home), "utf8"); + expect(raw).not.toContain("codeVerifier"); + expect((await loadAuthState(linear, home)).tokens?.access_token).toBe( + "tok-a", + ); + }); + test("resetAuthorization does not delete a sibling's just-saved tokens", async () => { const home = await tempHome(); const a = await createOAuthProvider({ @@ -410,9 +443,49 @@ describe("createOAuthProvider", () => { refresh_token: "ref-a", }); - await saveAuthState(linear, { clientInformation: clientInfo(60435) }, home); + const b = await createOAuthProvider({ + serverName: "linear", + serverURL: linear.serverURL, + redirectUrl: "http://127.0.0.1:60435/callback", + onAuthURL: () => undefined, + home, + }); + await saveClient(b, clientInfo(60435)); expect((await syncValue(a.tokens()))?.access_token).toBe("tok-a"); + expect((await loadAuthState(linear, home)).tokens?.access_token).toBe( + "tok-a", + ); + }); + + test("propagates a sibling logout to an existing provider", 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:62000/callback", + onAuthURL: () => undefined, + home, + }); + await a.saveTokens({ + access_token: "tok-a", + token_type: "bearer", + expires_in: 3600, + refresh_token: "ref-a", + }); + expect((await syncValue(b.tokens()))?.access_token).toBe("tok-a"); + + await b.resetAuthorization(); + + expect(await syncValue(a.tokens())).toBeUndefined(); + expect((await loadAuthState(linear, home)).tokens).toBeUndefined(); }); test("does not adopt a different-port sibling DCR client without tokens", async () => { diff --git a/src/mcp/oauth-provider.ts b/src/mcp/oauth-provider.ts index 8fdcc1c48..c1a69fa53 100644 --- a/src/mcp/oauth-provider.ts +++ b/src/mcp/oauth-provider.ts @@ -135,6 +135,7 @@ export async function createOAuthProvider( identity, (state) => { dropStaleClientRegistration(state, opts.redirectUrl); + delete (state as { codeVerifier?: string }).codeVerifier; }, home, ); @@ -177,7 +178,12 @@ export async function createOAuthProvider( if (next === undefined) return; seenStamp = stamp; const adoptClient = shouldAdoptClient(stored, next, opts.redirectUrl); - assignTokens(stored, next); + // A durable read that succeeds with no tokens key is an explicit logout + // (sibling resetAuthorization) — clear the mirror. A missing, corrupt, or + // unreadable file reads as undefined above and keeps the mirror, as does a + // sibling client-only write that preserves tokens through the merge chain. + if (next.tokens === undefined) delete stored.tokens; + else stored.tokens = next.tokens; if (adoptClient) assignClient(stored, next); }; From f55e37fc6e983823710f0761f335768c17ba8d80 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Mon, 28 Sep 2026 08:52:14 -0700 Subject: [PATCH 3/3] fix(mcp): harden auth disk boundary and guard refresh-vs-reset race --- src/mcp/auth-store.ts | 21 ++++- src/mcp/oauth-provider.test.ts | 163 +++++++++++++++++++++++++++++++++ src/mcp/oauth-provider.ts | 39 +++++++- 3 files changed, 216 insertions(+), 7 deletions(-) diff --git a/src/mcp/auth-store.ts b/src/mcp/auth-store.ts index aa11348f2..f4a083d45 100644 --- a/src/mcp/auth-store.ts +++ b/src/mcp/auth-store.ts @@ -77,11 +77,28 @@ function isEnoent(err: unknown): boolean { ); } +function isAuthRecord(value: unknown): value is Record { + 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. diff --git a/src/mcp/oauth-provider.test.ts b/src/mcp/oauth-provider.test.ts index a9aea7bc8..515ba9520 100644 --- a/src/mcp/oauth-provider.test.ts +++ b/src/mcp/oauth-provider.test.ts @@ -738,6 +738,169 @@ describe("createOAuthProvider", () => { expect((await syncValue(provider.tokens()))?.access_token).toBe("tok"); }); + test("keeps the mirror when the auth file holds a JSON array", async () => { + const home = await tempHome(); + const provider = await createOAuthProvider({ + serverName: "linear", + serverURL: linear.serverURL, + redirectUrl: "http://127.0.0.1:1/callback", + onAuthURL: () => undefined, + home, + }); + await saveClient(provider, clientInfo(1)); + await provider.saveTokens({ access_token: "tok", token_type: "bearer" }); + + await writeFile(authFilePath(linear, home), "[]"); + + expect((await syncValue(provider.tokens()))?.access_token).toBe("tok"); + expect((await syncValue(provider.clientInformation()))?.client_id).toBe( + "client-on-1", + ); + }); + + test("treats malformed disk tokens as unreadable and never throws", async () => { + const malformed = [ + `{"tokens":null}`, + `{"tokens":"tok-a"}`, + `{"tokens":{}}`, + `{"tokens":[]}`, + ]; + for (const raw of malformed) { + const home = await tempHome(); + const provider = await createOAuthProvider({ + serverName: "linear", + serverURL: linear.serverURL, + redirectUrl: "http://127.0.0.1:1/callback", + onAuthURL: () => undefined, + home, + }); + await provider.saveTokens({ access_token: "tok", token_type: "bearer" }); + + await writeFile(authFilePath(linear, home), raw); + + expect((await syncValue(provider.tokens()))?.access_token).toBe("tok"); + expect(await syncValue(provider.clientInformation())).toBeUndefined(); + } + }); + + test("never throws when disk holds an other-port client with null tokens", async () => { + const home = await tempHome(); + const provider = await createOAuthProvider({ + serverName: "linear", + serverURL: linear.serverURL, + redirectUrl: "http://127.0.0.1:62000/callback", + onAuthURL: () => undefined, + home, + }); + await saveClient(provider, clientInfo(62000)); + await provider.saveTokens({ + access_token: "tok-a", + token_type: "bearer", + expires_in: 3600, + refresh_token: "ref-a", + }); + + await writeFile( + authFilePath(linear, home), + JSON.stringify({ + clientInformation: clientInfo(60435), + tokens: null, + }), + ); + + expect((await syncValue(provider.tokens()))?.access_token).toBe("tok-a"); + expect((await syncValue(provider.clientInformation()))?.client_id).toBe( + "client-on-62000", + ); + }); + + test("a mid-refresh sibling logout is not resurrected by the refresh write", async () => { + const home = await tempHome(); + let releaseRefresh!: () => void; + const refreshGate = new Promise((resolve) => { + releaseRefresh = resolve; + }); + let refreshPosted!: () => void; + const refreshPostedGate = new Promise((resolve) => { + refreshPosted = resolve; + }); + const fetchFn = async ( + url: string | URL, + init?: RequestInit, + ): Promise => { + const href = String(url); + if (init?.method === "POST") { + refreshPosted(); + await refreshGate; + return new Response( + JSON.stringify({ + access_token: "resurrected", + token_type: "bearer", + expires_in: 3600, + refresh_token: "ref-b", + }), + { status: 200, headers: { "content-type": "application/json" } }, + ); + } + if (href.includes("oauth-protected-resource")) { + return new Response( + JSON.stringify({ + resource: "https://mcp.linear.app/mcp", + authorization_servers: ["https://mcp.linear.app"], + }), + { status: 200, headers: { "content-type": "application/json" } }, + ); + } + if ( + href.includes("oauth-authorization-server") || + href.includes("openid-configuration") + ) { + return new Response( + JSON.stringify({ + issuer: "https://mcp.linear.app", + authorization_endpoint: "https://mcp.linear.app/authorize", + token_endpoint: "https://mcp.linear.app/oauth/token", + response_types_supported: ["code"], + }), + { status: 200, headers: { "content-type": "application/json" } }, + ); + } + return new Response(null, { status: 404 }); + }; + + const a = await createOAuthProvider({ + serverName: "linear", + serverURL: linear.serverURL, + redirectUrl: "http://127.0.0.1:62000/callback", + onAuthURL: () => undefined, + home, + fetchFn, + }); + await saveClient(a, clientInfo(62000)); + await a.saveTokens({ + access_token: "stale", + token_type: "bearer", + expires_in: 1, + refresh_token: "ref-a", + }); + const b = await createOAuthProvider({ + serverName: "linear", + serverURL: linear.serverURL, + redirectUrl: "http://127.0.0.1:62000/callback", + onAuthURL: () => undefined, + home, + }); + + const pending = a.refreshToken("ref-a"); + await refreshPostedGate; + await b.resetAuthorization(); + releaseRefresh(); + await pending; + + expect((await loadAuthState(linear, home)).tokens).toBeUndefined(); + expect(await syncValue(a.tokens())).toBeUndefined(); + }); + test("keeps the mirror through an unreadable auth file and recovers once readable", async () => { const home = await tempHome(); const provider = await createOAuthProvider({ diff --git a/src/mcp/oauth-provider.ts b/src/mcp/oauth-provider.ts index c1a69fa53..d6e201039 100644 --- a/src/mcp/oauth-provider.ts +++ b/src/mcp/oauth-provider.ts @@ -20,6 +20,7 @@ import type { import type { FetchLike } from "@modelcontextprotocol/sdk/shared/transport.js"; import { authFilePath, + isValidAuthTokens, tryLoadAuthStateSync, updateAuthState, type MCPAuthIdentity, @@ -45,9 +46,9 @@ 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); + const uris: unknown = info?.redirect_uris; + if (!Array.isArray(uris) || uris.length === 0) return true; + return (uris as unknown[]).includes(redirectUrl); } function isAbortError(err: unknown): boolean { @@ -77,12 +78,20 @@ function shouldAdoptClient( next: MCPAuthState, redirectUrl: string, ): boolean { - if (next.clientInformation === undefined) return false; + // Malformed disk must never throw or poison the mirror: adoption requires a + // client object and valid tokens. next.tokens is boundary-validated upstream, + // but the guards below keep this total even if that contract is bypassed. + if ( + typeof next.clientInformation !== "object" || + next.clientInformation === null || + Array.isArray(next.clientInformation) + ) + return false; if (redirectUrisInclude(next.clientInformation, redirectUrl)) return true; // Other-port DCR is a sibling's in-progress registration unless they also // published new tokens (completed re-auth). return ( - next.tokens !== undefined && + isValidAuthTokens(next.tokens) && next.tokens.access_token !== stored.tokens?.access_token ); } @@ -264,6 +273,16 @@ export async function createOAuthProvider( }, refreshToken: async (refreshToken: string): Promise => { try { + // Snapshot the durable access token before the network round-trip. A + // sibling resetAuthorization (logout) or rotation that lands mid-refresh + // must win over this stale grant: the post-refresh write re-checks disk + // and drops the refreshed tokens instead of resurrecting revoked ones. + // There is no server-side revocation, so the durable file is the only + // source of truth for "logged out". Sync read: refreshToken callers may + // busy-wait on microtasks alone, which an async disk read could never + // resolve behind. + const preRefreshAccess = tryLoadAuthStateSync(identity, home)?.tokens + ?.access_token; const fetchFn = opts.fetchFn; if ( authorizationServerMetadata === undefined || @@ -312,9 +331,19 @@ export async function createOAuthProvider( resource: resourceURL, ...(fetchFn === undefined ? {} : { fetchFn }), }); + let refreshSuperseded = false; await apply((state) => { + if (state.tokens?.access_token !== preRefreshAccess) { + refreshSuperseded = true; + return; + } state.tokens = tokens; }); + if (refreshSuperseded) { + const disk = tryLoadAuthStateSync(identity, home); + if (disk?.tokens !== undefined) stored.tokens = disk.tokens; + else delete stored.tokens; + } return tokens; } catch (err) { if (isAbortError(err) || err instanceof UnauthorizedError) throw err;