diff --git a/src/mcp/auth-store.test.ts b/src/mcp/auth-store.test.ts index 1ca8817bf..bc68d30de 100644 --- a/src/mcp/auth-store.test.ts +++ b/src/mcp/auth-store.test.ts @@ -73,82 +73,6 @@ describe("mcp auth-store", () => { ).toBe(true); }); - test("overlapping updateAuthState from two processes keeps tokens and client registration", 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.clientInformation = { - client_id: "c1", - redirect_uris: ["http://127.0.0.1:1/callback"], - client_id_issued_at: 1, - client_name: "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, "client", 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.clientInformation?.client_name).toBe("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 9aa56276b..591357b6b 100644 --- a/src/mcp/auth-store.ts +++ b/src/mcp/auth-store.ts @@ -1,17 +1,8 @@ import { createHash } from "node:crypto"; -import { - mkdir, - open, - readFile, - rename, - stat, - unlink, - writeFile, -} from "node:fs/promises"; -import { readFileSync, writeFileSync } from "node:fs"; +import { mkdir, readFile, rename, unlink, writeFile } from "node:fs/promises"; +import { writeFileSync } from "node:fs"; import { homedir } from "node:os"; import { dirname, join } from "node:path"; -import { setTimeout as delay } from "node:timers/promises"; import { getLogger } from "@intx/log"; import type { OAuthClientInformationFull, @@ -116,15 +107,6 @@ function stateFromRaw(raw: string | undefined, path: string): MCPAuthState { return parseAuthState(raw, path) ?? {}; } -function readAuthFileSync(path: string): string | undefined { - try { - return readFileSync(path, "utf8"); - } catch (err) { - if (isEnoent(err)) return undefined; - throw err; - } -} - async function readAuthFile(path: string): Promise { try { return await readFile(path, "utf8"); @@ -142,97 +124,18 @@ export async function loadAuthState( return stateFromRaw(await readAuthFile(path), path); } -// Cache refresh: missing/unreadable/corrupt files return undefined so the -// caller keeps its in-memory mirror. Empty-on-corrupt is loadAuthState's contract. -export function tryLoadAuthStateSync( - identity: MCPAuthIdentity, - home: string = homedir(), -): MCPAuthState | undefined { - const path = authFilePath(identity, home); - let raw: string | undefined; - try { - raw = readAuthFileSync(path); - } catch { - return undefined; - } - if (raw === undefined) return undefined; - return parseAuthState(raw, path); -} - -function isEexist(err: unknown): boolean { - return ( - typeof err === "object" && - err !== null && - "code" in err && - err.code === "EEXIST" - ); -} - // pid alone is not unique per call, so concurrent saves must not share a temp // path or the second rename hits ENOENT. let tmpWriteCounter = 0; // Serialize read-modify-write per auth file so two providers for the same -// server cannot clobber each other's fields (lost-update). +// server cannot clobber each other's fields (lost-update). Same-process only: +// the chain runs op directly, with no cross-process file lock. 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), - ); + const run = previous.then(op, op); updateChains.set( path, run.then( diff --git a/src/mcp/oauth-provider.test.ts b/src/mcp/oauth-provider.test.ts index 362b303df..7fae92c23 100644 --- a/src/mcp/oauth-provider.test.ts +++ b/src/mcp/oauth-provider.test.ts @@ -249,7 +249,15 @@ describe("createOAuthProvider", () => { await deleteAuthState(linear, home); - expect(await syncValue(provider.tokens())).toBeUndefined(); + expect((await syncValue(provider.tokens()))?.access_token).toBe("tok"); + const later = await createOAuthProvider({ + serverName: "linear", + serverURL: linear.serverURL, + redirectUrl: "http://127.0.0.1:1/callback", + onAuthURL: () => undefined, + home, + }); + expect(await syncValue(later.tokens())).toBeUndefined(); }); test("keeps live tokens when the auth file is unreadable but drops them on delete", async () => { @@ -265,8 +273,8 @@ describe("createOAuthProvider", () => { await provider.saveTokens({ access_token: "tok", token_type: "bearer" }); expect((await syncValue(provider.tokens()))?.access_token).toBe("tok"); - // Transient stat failure (EACCES, file present) must not discard live - // credentials; only a real deletion (ENOENT) does. + // An unreadable directory must not discard the in-memory mirror. Deleting + // the file does not either; a provider constructed after the delete does. const dir = dirname(authFilePath(linear, home)); await chmod(dir, 0o000); try { @@ -277,7 +285,15 @@ describe("createOAuthProvider", () => { await deleteAuthState(linear, home); - expect(await syncValue(provider.tokens())).toBeUndefined(); + expect((await syncValue(provider.tokens()))?.access_token).toBe("tok"); + const later = await createOAuthProvider({ + serverName: "linear", + serverURL: linear.serverURL, + redirectUrl: "http://127.0.0.1:1/callback", + onAuthURL: () => undefined, + home, + }); + expect(await syncValue(later.tokens())).toBeUndefined(); }); test("two overlapping logins on different ports both complete without clobbering verifiers", async () => { @@ -323,11 +339,11 @@ describe("createOAuthProvider", () => { expect(raw).not.toContain("codeVerifier"); expect(raw).not.toContain("pkce-a"); expect(raw).not.toContain("pkce-b"); - // Last completer wins the durable tokens; the sibling picks them up. + // Last completer wins the durable tokens. This episode keeps its own. expect((await loadAuthState(linear, home)).tokens?.access_token).toBe( "tok-b", ); - expect((await syncValue(a.tokens()))?.access_token).toBe("tok-b"); + expect((await syncValue(a.tokens()))?.access_token).toBe("tok-a"); expect((await syncValue(b.tokens()))?.access_token).toBe("tok-b"); await expectClientOnPort(b, 60435); }); @@ -356,12 +372,13 @@ describe("createOAuthProvider", () => { home, }); - // Tokens present → client kept at create. Reset simulates failed refresh. + // Tokens present → client kept at create. Reset drops this session's + // tokens. The other port's registration stays on disk. 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.clientInformation?.client_id).toBe("client-on-60435"); expect(disk.tokens).toBeUndefined(); }); @@ -398,6 +415,51 @@ describe("createOAuthProvider", () => { ); }); + test("resetAuthorization does not delete a sibling's client registration", 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 saveClient(a, clientInfo(62000)); + await a.saveTokens({ + access_token: "tok-a", + token_type: "bearer", + expires_in: 3600, + refresh_token: "ref-a", + }); + await saveClient(b, clientInfo(60435)); + await b.saveTokens({ + access_token: "tok-b", + token_type: "bearer", + expires_in: 3600, + refresh_token: "ref-b", + }); + + await a.resetAuthorization(); + + const disk = await loadAuthState(linear, home); + expect(disk.tokens?.access_token).toBe("tok-b"); + expect(disk.clientInformation?.client_id).toBe("client-on-60435"); + expect(disk.clientInformation?.redirect_uris).toEqual([ + "http://127.0.0.1:60435/callback", + ]); + expect((await syncValue(b.clientInformation()))?.client_id).toBe( + "client-on-60435", + ); + }); + test("isolates same-name providers by endpoint and persists the same identity", async () => { const home = await tempHome(); const customURL = "https://custom.example/mcp"; @@ -489,7 +551,18 @@ describe("createOAuthProvider", () => { refresh_token: "fresh-refresh", }); - expect((await syncValue(b.tokens()))?.access_token).toBe("fresh"); + expect(await syncValue(b.tokens())).toBeUndefined(); + expect((await loadAuthState(linear, home)).tokens?.access_token).toBe( + "fresh", + ); + const later = await createOAuthProvider({ + serverName: "linear", + serverURL: linear.serverURL, + redirectUrl: "http://127.0.0.1:1/callback", + onAuthURL: () => undefined, + home, + }); + expect((await syncValue(later.tokens()))?.access_token).toBe("fresh"); }); test("does not replace an in-progress PKCE verifier from a different-port sibling", async () => { @@ -608,13 +681,14 @@ describe("createOAuthProvider", () => { refresh_token: "ref-b", }); - expect((await syncValue(a.tokens()))?.access_token).toBe("tok-b"); + expect((await syncValue(a.tokens()))?.access_token).toBe("tok-a"); const disk = await loadAuthState(linear, home); expect(disk.tokens?.access_token).toBe("tok-b"); expect(disk.clientInformation?.client_id).toBe("client-on-60435"); expect(disk.clientInformation?.redirect_uris).toEqual([ "http://127.0.0.1:60435/callback", ]); + await expectClientOnPort(a, 62000); }); test("same-port DCR rotation after a different-port sibling saveClient keeps the new client", async () => { @@ -656,18 +730,25 @@ describe("createOAuthProvider", () => { await provider.saveTokens({ access_token: "tok", token_type: "bearer" }); expect((await syncValue(provider.tokens()))?.access_token).toBe("tok"); - // Server-side removal must not leave orphaned credentials behind. + // A live provider keeps its mirror; a later construct sees the new file. await deleteAuthState(linear, home); - expect(await syncValue(provider.tokens())).toBeUndefined(); + expect((await syncValue(provider.tokens()))?.access_token).toBe("tok"); - // The provider stays live: a sibling session's fresh auth is adopted. await saveAuthState( linear, { tokens: { access_token: "reauthed", token_type: "bearer" } }, home, ); - expect((await syncValue(provider.tokens()))?.access_token).toBe("reauthed"); + expect((await syncValue(provider.tokens()))?.access_token).toBe("tok"); + const later = await createOAuthProvider({ + serverName: "linear", + serverURL: linear.serverURL, + redirectUrl: "http://127.0.0.1:1/callback", + onAuthURL: () => undefined, + home, + }); + expect((await syncValue(later.tokens()))?.access_token).toBe("reauthed"); }); test("keeps live tokens when the auth file is corrupt", async () => { @@ -722,7 +803,15 @@ describe("createOAuthProvider", () => { { tokens: { access_token: "fresh", token_type: "bearer" } }, home, ); - expect((await syncValue(provider.tokens()))?.access_token).toBe("fresh"); + expect((await syncValue(provider.tokens()))?.access_token).toBe("tok"); + const later = await createOAuthProvider({ + serverName: "linear", + serverURL: linear.serverURL, + redirectUrl: "http://127.0.0.1:1/callback", + onAuthURL: () => undefined, + home, + }); + expect((await syncValue(later.tokens()))?.access_token).toBe("fresh"); }); test("does not delete scoped state whose filename stem is another provider name", async () => { diff --git a/src/mcp/oauth-provider.ts b/src/mcp/oauth-provider.ts index 91a7332f0..c900cca3d 100644 --- a/src/mcp/oauth-provider.ts +++ b/src/mcp/oauth-provider.ts @@ -1,4 +1,3 @@ -import { statSync } from "node:fs"; import { homedir } from "node:os"; import { discoverAuthorizationServerMetadata, @@ -19,9 +18,6 @@ import type { } from "@modelcontextprotocol/sdk/shared/auth.js"; import type { FetchLike } from "@modelcontextprotocol/sdk/shared/transport.js"; import { - authFilePath, - isEnoent, - tryLoadAuthStateSync, updateAuthState, type MCPAuthIdentity, type MCPAuthState, @@ -71,52 +67,6 @@ function dropStaleClientRegistration( delete state.clientInformation; } -function shouldAdoptClient( - stored: MCPAuthState, - next: MCPAuthState, - redirectUrl: string, -): boolean { - if (next.clientInformation === undefined) return false; - if (redirectUrisInclude(next.clientInformation, redirectUrl)) return true; - // Other-port DCR is a sibling's in-progress registration unless it - // published new tokens (completed re-auth). - return ( - next.tokens !== undefined && - next.tokens.access_token !== stored.tokens?.access_token - ); -} - -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 { - if (next.clientInformation !== undefined) - stored.clientInformation = next.clientInformation; - else delete stored.clientInformation; -} - -function matchingLiveClient( - stored: MCPAuthState, - redirectUrl: string, -): OAuthClientInformationFull | undefined { - const live = stored.clientInformation; - if (live === undefined || !redirectUrisInclude(live, redirectUrl)) - return undefined; - return live; -} - -function persistMatchingLiveClient( - stored: MCPAuthState, - next: MCPAuthState, - redirectUrl: string, -): void { - const live = matchingLiveClient(stored, redirectUrl); - if (live === undefined) return; - next.clientInformation = live; -} - export async function createOAuthProvider( opts: OAuthProviderOptions, ): Promise { @@ -125,65 +75,28 @@ export async function createOAuthProvider( serverURL: opts.serverURL, }; const home = opts.home ?? homedir(); - // Scrub stale DCR under the per-file chain; tokens and matching DCR are read - // from disk so a sibling's completed auth is picked up, while PKCE stays - // instance-local so a sibling cannot clobber an in-progress verifier. - const stored: MCPAuthState = await updateAuthState( + // Scrub a legacy disk verifier under the per-file chain. A mismatched client + // is removed from this instance's memory so the new port does not reuse it. + // It is removed from disk only when a legacy codeVerifier is still stored + // beside it; a sibling's in-progress registration has no verifier on disk and + // must survive this construct. PKCE for this instance stays in memory. + const durable = await updateAuthState( identity, (state) => { + if (!("codeVerifier" in state)) return; dropStaleClientRegistration(state, opts.redirectUrl); }, home, ); + const stored: MCPAuthState = { ...durable }; + dropStaleClientRegistration(stored, opts.redirectUrl); + // Load-mutate-save one field. Memory keeps the value this provider just set; + // the write must not copy a sibling's other field back over it. const apply = async ( mutator: (state: MCPAuthState) => void, ): Promise => { - const next = await updateAuthState( - identity, - (state) => { - mutator(state); - persistMatchingLiveClient(stored, state, opts.redirectUrl); - }, - home, - ); - assignTokens(stored, next); - if (matchingLiveClient(stored, opts.redirectUrl) === undefined) { - assignClient(stored, next); - } - }; - - // Staleness guard: statSync per getter, sync read only on mtime/size change. - // Stamp only after a successful read so a failed read is retried next call. - const authPath = authFilePath(identity, home); - let seenStamp: string | undefined; - // True once durable state was seen. A transient stat failure resets the stamp - // but not this flag, so a real deletion (ENOENT) still clears tokens instead - // of looking like "never saw the file". - let observedDurable = false; - const refreshDurableFromDisk = (): void => { - let stamp: string | undefined; - try { - const stat = statSync(authPath); - stamp = `${String(stat.mtimeMs)}:${String(stat.size)}`; - } catch (err) { - if (!observedDurable) return; - seenStamp = undefined; - if (!isEnoent(err)) return; - // Auth file gone (removed elsewhere): drop in-memory tokens too; a later - // sibling save is adopted on the next read via the stamp check. - observedDurable = false; - delete stored.tokens; - return; - } - if (stamp === seenStamp) return; - const next = tryLoadAuthStateSync(identity, home); - if (next === undefined) return; - seenStamp = stamp; - observedDurable = true; - const adoptClient = shouldAdoptClient(stored, next, opts.redirectUrl); - assignTokens(stored, next); - if (adoptClient) assignClient(stored, next); + await updateAuthState(identity, mutator, home); }; let oauthState: string | undefined; @@ -212,17 +125,16 @@ export async function createOAuthProvider( }; }, clientInformation(): OAuthClientInformationMixed | undefined { - refreshDurableFromDisk(); return stored.clientInformation; }, saveClientInformation(info: OAuthClientInformationMixed): Promise { - stored.clientInformation = info as OAuthClientInformationFull; + const clientInformation = info as OAuthClientInformationFull; + stored.clientInformation = clientInformation; return apply((state) => { - state.clientInformation = info as OAuthClientInformationFull; + state.clientInformation = clientInformation; }); }, tokens(): OAuthTokens | undefined { - refreshDurableFromDisk(); return stored.tokens; }, saveTokens(tokens: OAuthTokens): Promise { @@ -248,19 +160,23 @@ export async function createOAuthProvider( async resetAuthorization(): Promise { oauthState = undefined; pkceVerifier = undefined; - // Snapshot before the disk refresh so a token-less session cannot adopt - // a sibling's credentials and then delete them. + // Compare disk to this instance's memory. Do not adopt a sibling's + // credentials into memory before deciding what to delete. const previous = stored.tokens?.access_token; - refreshDurableFromDisk(); await apply((state) => { if (state.tokens?.access_token === previous) { delete state.tokens; } - // Next browser flow needs a client registered for this loopback port. - if (!redirectUrisInclude(state.clientInformation, opts.redirectUrl)) { + // Only this instance's registration. A sibling's redirect must stay + // on disk so its tokens still have a client. + if (redirectUrisInclude(state.clientInformation, opts.redirectUrl)) { delete state.clientInformation; } }); + if (stored.tokens?.access_token === previous) { + delete stored.tokens; + } + delete stored.clientInformation; }, refreshToken: async (refreshToken: string): Promise => { try { @@ -321,9 +237,7 @@ export async function createOAuthProvider( ...tokens, issuer: authorizationServerUrl, }; - await apply((state) => { - state.tokens = boundTokens; - }); + await provider.saveTokens(boundTokens); return boundTokens; } catch (err) { if (isAbortError(err) || err instanceof UnauthorizedError) throw err;