diff --git a/src/auth/oauth/oauth.test.ts b/src/auth/oauth/oauth.test.ts index aa6f225e7..09461ce5b 100644 --- a/src/auth/oauth/oauth.test.ts +++ b/src/auth/oauth/oauth.test.ts @@ -1,12 +1,12 @@ import { afterEach, describe, expect, test } from "bun:test"; -import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { mkdir, mkdtemp, readFile, rm, rmdir, unlink, utimes, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { baseTokensFromResponse, postToken, type OAuthClientConfig } from "./client.js"; import { startOAuthLogin } from "./login.js"; import { createTokenSession } from "./session.js"; -import { createAuthStore, type AuthProfile, type BaseTokens } from "./store.js"; +import { createAuthStore, type AuthProfile, type BaseTokens, withStoreFileLock } from "./store.js"; const config: OAuthClientConfig = { clientId: "client-id", @@ -134,6 +134,291 @@ describe("createAuthStore", () => { } }); + test("concurrent saveProfile calls preserve all profiles", async () => { + const home = await mkdtemp(join(tmpdir(), "oauth-lock-")); + try { + const store = createAuthStore({ + filename: "test-auth.json", + isTokens: isTestTokens, + }); + + // Fire concurrent saves for distinct profile names. Without the store + // lock the last writer would silently drop profiles saved by others. + const count = 20; + const saves = Array.from({ length: count }, (_, i) => + store.saveProfile( + { + name: `p-${i}`, + tokens: { access: `a-${i}`, refresh: `r-${i}`, expiresAt: i }, + createdAt: i, + }, + home, + ), + ); + await Promise.all(saves); + + const profiles = await store.listProfiles(home); + expect(profiles).toHaveLength(count); + for (let i = 0; i < count; i++) { + const p = profiles.find((p) => p.name === `p-${i}`); + expect(p).toBeDefined(); + expect(p!.tokens.access).toBe(`a-${i}`); + } + } finally { + await rm(home, { recursive: true, force: true }); + } + }); + + test("concurrent updateTokens and saveProfile do not lose profiles", async () => { + const home = await mkdtemp(join(tmpdir(), "oauth-lock-")); + try { + const store = createAuthStore({ + filename: "test-auth.json", + isTokens: isTestTokens, + }); + + // Seed two profiles. + await store.saveProfile( + { name: "a", tokens: { access: "a0", refresh: "r0", expiresAt: 1 }, createdAt: 1 }, + home, + ); + await store.saveProfile( + { name: "b", tokens: { access: "b0", refresh: "r0", expiresAt: 1 }, createdAt: 2 }, + home, + ); + + // Concurrently update tokens for both profiles plus a full save of a third profile. + await Promise.all([ + store.updateTokens("a", { access: "a1", refresh: "r1", expiresAt: 10 }, home), + store.updateTokens("b", { access: "b1", refresh: "r1", expiresAt: 10 }, home), + store.saveProfile( + { name: "c", tokens: { access: "c1", refresh: "r1", expiresAt: 10 }, createdAt: 3 }, + home, + ), + ]); + + const profiles = await store.listProfiles(home); + const names = profiles.map((p) => p.name).sort(); + expect(names).toEqual(["a", "b", "c"]); + expect(await store.loadProfile("a", home)).toEqual({ + name: "a", + tokens: { access: "a1", refresh: "r1", expiresAt: 10 }, + createdAt: 1, + }); + expect(await store.loadProfile("b", home)).toEqual({ + name: "b", + tokens: { access: "b1", refresh: "r1", expiresAt: 10 }, + createdAt: 2, + }); + expect(await store.loadProfile("c", home)).toEqual({ + name: "c", + tokens: { access: "c1", refresh: "r1", expiresAt: 10 }, + createdAt: 3, + }); + } finally { + await rm(home, { recursive: true, force: true }); + } + }); + + test("lock directory is cleaned up after operations", async () => { + const home = await mkdtemp(join(tmpdir(), "oauth-lock-cleanup-")); + try { + const store = createAuthStore({ + filename: "test-auth.json", + isTokens: isTestTokens, + }); + + await store.saveProfile( + { name: "x", tokens: { access: "a", refresh: "r", expiresAt: 1 }, createdAt: 1 }, + home, + ); + + const lockDir = join(home, ".corbits", "test-auth.json.lock"); + await expect(readFile(join(lockDir, "owner"), "utf8")).rejects.toThrow(); + } finally { + await rm(home, { recursive: true, force: true }); + } + }); + + test("concurrent removeProfile does not leave stale profiles", async () => { + const home = await mkdtemp(join(tmpdir(), "oauth-lock-")); + try { + const store = createAuthStore({ + filename: "test-auth.json", + isTokens: isTestTokens, + }); + + const count = 10; + for (let i = 0; i < count; i++) { + await store.saveProfile( + { + name: `r-${i}`, + tokens: { access: `a-${i}`, refresh: `r-${i}`, expiresAt: i }, + createdAt: i, + }, + home, + ); + } + + // Concurrently remove all profiles. + const removals = Array.from({ length: count }, (_, i) => store.removeProfile(`r-${i}`, home)); + const results = await Promise.all(removals); + + // Each removal should have returned exactly one name. + for (const removed of results) { + expect(removed).toHaveLength(1); + } + + expect(await store.listProfiles(home)).toEqual([]); + } finally { + await rm(home, { recursive: true, force: true }); + } + }); + + test("malformed stale lock is recovered automatically", async () => { + const home = await mkdtemp(join(tmpdir(), "oauth-stale-lock-")); + try { + const store = createAuthStore({ + filename: "test-auth.json", + isTokens: isTestTokens, + }); + + const lockDir = join(home, ".corbits", "test-auth.json.lock"); + await mkdir(lockDir, { recursive: true, mode: 0o700 }); + const ownerFile = join(lockDir, "owner"); + await writeFile(ownerFile, "malformed"); + const past = new Date(Date.now() - 60_000); // 60s in the past + await utimes(ownerFile, past, past); + + // The store should detect the stale lock, remove it, and succeed. + await store.saveProfile( + { name: "after-stale", tokens: { access: "a", refresh: "r", expiresAt: 1 }, createdAt: 1 }, + home, + ); + + const profiles = await store.listProfiles(home); + expect(profiles).toHaveLength(1); + expect(profiles[0]).toBeDefined(); + expect(profiles[0]!.name).toBe("after-stale"); + } finally { + await rm(home, { recursive: true, force: true }); + } + }); + + test("an old lock owned by a live process is not displaced", async () => { + const home = await mkdtemp(join(tmpdir(), "oauth-live-lock-")); + try { + const lockPath = join(home, ".corbits", "test-auth.json.lock"); + await mkdir(lockPath, { recursive: true, mode: 0o700 }); + const ownerFile = join(lockPath, "owner"); + await writeFile(ownerFile, JSON.stringify({ pid: process.pid, owner: "live-holder" })); + const past = new Date(Date.now() - 60_000); + await utimes(ownerFile, past, past); + + let entered = false; + const waiting = withStoreFileLock(lockPath, async () => { + entered = true; + }); + await Bun.sleep(150); + + expect(entered).toBe(false); + await unlink(ownerFile); + await rmdir(lockPath); + await waiting; + expect(entered).toBe(true); + } finally { + await rm(home, { recursive: true, force: true }); + } + }); + + test("lock cleanup leaves a replacement lock owned by another holder", async () => { + const home = await mkdtemp(join(tmpdir(), "oauth-lock-owner-")); + try { + const lockPath = join(home, ".corbits", "test-auth.json.lock"); + const replacement = JSON.stringify({ pid: process.pid, owner: "replacement" }); + + await withStoreFileLock(lockPath, async () => { + await unlink(join(lockPath, "owner")); + await rmdir(lockPath); + await mkdir(lockPath, { mode: 0o700 }); + await writeFile(join(lockPath, "owner"), replacement); + }); + + expect(await readFile(join(lockPath, "owner"), "utf8")).toBe(replacement); + } finally { + await rm(home, { recursive: true, force: true }); + } + }); + + test("two stale reclaimers cannot enter the critical section together", async () => { + const home = await mkdtemp(join(tmpdir(), "oauth-stale-reclaimers-")); + try { + const lockPath = join(home, ".corbits", "test-auth.json.lock"); + await mkdir(lockPath, { recursive: true, mode: 0o700 }); + const ownerFile = join(lockPath, "owner"); + await writeFile(ownerFile, JSON.stringify({ pid: 2_147_483_647, owner: "dead" })); + const past = new Date(Date.now() - 60_000); + await utimes(ownerFile, past, past); + + let active = 0; + let entered = 0; + let maxActive = 0; + const contender = () => + withStoreFileLock(lockPath, async () => { + active += 1; + entered += 1; + maxActive = Math.max(maxActive, active); + await Bun.sleep(100); + active -= 1; + }); + + await Promise.all([contender(), contender()]); + + expect(entered).toBe(2); + expect(maxActive).toBe(1); + } finally { + await rm(home, { recursive: true, force: true }); + } + }); + + test("different home directories do not contend on the same lock", async () => { + const homeA = await mkdtemp(join(tmpdir(), "oauth-home-a-")); + const homeB = await mkdtemp(join(tmpdir(), "oauth-home-b-")); + try { + const store = createAuthStore({ + filename: "test-auth.json", + isTokens: isTestTokens, + }); + + // Concurrent saves to different homes should not block each other. + await Promise.all([ + store.saveProfile( + { name: "from-a", tokens: { access: "a", refresh: "r", expiresAt: 1 }, createdAt: 1 }, + homeA, + ), + store.saveProfile( + { name: "from-b", tokens: { access: "b", refresh: "r", expiresAt: 2 }, createdAt: 2 }, + homeB, + ), + ]); + + const profilesA = await store.listProfiles(homeA); + const profilesB = await store.listProfiles(homeB); + + expect(profilesA).toHaveLength(1); + expect(profilesB).toHaveLength(1); + const firstA = profilesA[0]; + const firstB = profilesB[0]; + expect(firstA).toBeDefined(); + expect(firstB).toBeDefined(); + expect(firstA!.name).toBe("from-a"); + expect(firstB!.name).toBe("from-b"); + } finally { + await rm(homeA, { recursive: true, force: true }); + await rm(homeB, { recursive: true, force: true }); + } + }); + test("drops invalid profile entries instead of wedging on them", async () => { const home = await mkdtemp(join(tmpdir(), "oauth-store-")); try { diff --git a/src/auth/oauth/store.ts b/src/auth/oauth/store.ts index e109023b9..0953eb1ed 100644 --- a/src/auth/oauth/store.ts +++ b/src/auth/oauth/store.ts @@ -1,6 +1,8 @@ -import { mkdir, readFile, rename, writeFile } from "node:fs/promises"; +import { randomUUID } from "node:crypto"; +import { mkdir, open, readFile, rename, rmdir, stat, unlink, writeFile } from "node:fs/promises"; import { homedir } from "node:os"; import { dirname, join } from "node:path"; +import { type } from "arktype"; import { SETTINGS_DIR_NAME } from "../../branding.js"; // On-disk store for named OAuth profiles. A user may hold multiple subscriptions @@ -43,6 +45,178 @@ interface AuthFile { profiles: Record>; } +const LOCK_RETRY_MS = 50; +const LOCK_TIMEOUT_MS = 15_000; +const STALE_LOCK_MS = 10_000; + +const LockOwner = type({ + pid: "number.integer > 0", + owner: "string", +}); + +interface LockSnapshot { + contents: string; + mtimeMs: number; +} + +const LOCK_OWNER_FILE = "owner"; +const LOCK_RECLAIM_DIR = "reclaim"; + +function isErrno(error: unknown, code: string): boolean { + return ( + typeof error === "object" && + error !== null && + "code" in error && + (error as { code?: unknown }).code === code + ); +} + +async function readLockSnapshot(path: string): Promise { + try { + const ownerPath = join(path, LOCK_OWNER_FILE); + const contents = await readFile(ownerPath, "utf8"); + const info = await stat(ownerPath); + return { contents, mtimeMs: info.mtimeMs }; + } catch (error) { + if (isErrno(error, "ENOENT")) return undefined; + throw error; + } +} + +function isProcessAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (error) { + return isErrno(error, "EPERM"); + } +} + +async function removeDirectory(path: string): Promise { + try { + await rmdir(path); + return true; + } catch (error) { + if (isErrno(error, "ENOENT") || isErrno(error, "ENOTEMPTY")) return false; + throw error; + } +} + +async function claimLockForRemoval(path: string): Promise { + try { + await mkdir(join(path, LOCK_RECLAIM_DIR), { mode: 0o700 }); + return true; + } catch (error) { + if (isErrno(error, "EEXIST") || isErrno(error, "ENOENT")) return false; + throw error; + } +} + +async function removeOwnedLock(path: string, expectedContents: string): Promise { + const deadline = Date.now() + LOCK_TIMEOUT_MS; + while (!(await claimLockForRemoval(path))) { + if (Date.now() >= deadline) return false; + await Bun.sleep(LOCK_RETRY_MS); + } + let removedOwner = false; + try { + const current = await readLockSnapshot(path); + if (current === undefined || current.contents !== expectedContents) return false; + await unlink(join(path, LOCK_OWNER_FILE)); + removedOwner = true; + } finally { + await removeDirectory(join(path, LOCK_RECLAIM_DIR)); + if (removedOwner) await removeDirectory(path); + } + return removedOwner; +} + +async function createLock(path: string, contents: string): Promise { + await mkdir(path, { mode: 0o700 }); + const ownerPath = join(path, LOCK_OWNER_FILE); + try { + const handle = await open(ownerPath, "wx", 0o600); + try { + await handle.writeFile(contents); + } finally { + await handle.close(); + } + } catch (cause) { + await unlink(ownerPath).catch(() => {}); + await removeDirectory(path).catch(() => false); + throw new Error(`Failed to create store lock: ${path}`, { cause }); + } +} + +function parseLockOwner(snapshot: LockSnapshot | undefined): typeof LockOwner.infer | undefined { + if (snapshot === undefined) return undefined; + try { + const parsed = LockOwner(JSON.parse(snapshot.contents)); + return parsed instanceof type.errors ? undefined : parsed; + } catch { + // Locks written by interrupted or older clients have no usable owner metadata. + return undefined; + } +} + +async function isStaleLock(path: string, snapshot: LockSnapshot | undefined): Promise { + let mtimeMs = snapshot?.mtimeMs; + if (mtimeMs === undefined) { + try { + mtimeMs = (await stat(path)).mtimeMs; + } catch (error) { + if (isErrno(error, "ENOENT")) return false; + throw error; + } + } + if (Date.now() - mtimeMs <= STALE_LOCK_MS) return false; + const owner = parseLockOwner(snapshot); + return owner === undefined || !isProcessAlive(owner.pid); +} + +async function removeStaleLock(path: string): Promise { + const initial = await readLockSnapshot(path); + if (!(await isStaleLock(path, initial))) return false; + if (!(await claimLockForRemoval(path))) return false; + let shouldRemove = false; + try { + const snapshot = await readLockSnapshot(path); + if (!(await isStaleLock(path, snapshot))) return false; + if (snapshot !== undefined) await unlink(join(path, LOCK_OWNER_FILE)); + shouldRemove = true; + } finally { + await removeDirectory(join(path, LOCK_RECLAIM_DIR)); + if (shouldRemove) await removeDirectory(path); + } + return shouldRemove; +} + +export async function withStoreFileLock(path: string, fn: () => Promise): Promise { + await mkdir(dirname(path), { recursive: true, mode: 0o700 }); + const contents = JSON.stringify({ pid: process.pid, owner: randomUUID() }); + const deadline = Date.now() + LOCK_TIMEOUT_MS; + + while (true) { + try { + await createLock(path, contents); + break; + } catch (error) { + if (!isErrno(error, "EEXIST")) throw error; + + if (await removeStaleLock(path)) continue; + if (Date.now() >= deadline) throw new Error(`Timed out acquiring store lock: ${path}`); + + await Bun.sleep(LOCK_RETRY_MS); + } + } + + try { + return await fn(); + } finally { + await removeOwnedLock(path, contents); + } +} + function isProfile( value: unknown, isTokens: (value: unknown) => value is TTokens, @@ -104,6 +278,11 @@ export function createAuthStore( await rename(tmp, path); } + // Serialise read-modify-write operations across concurrent CLI processes. + function withStoreLock(home: string, fn: () => Promise): Promise { + return withStoreFileLock(`${authPath(home)}.lock`, fn); + } + return { authPath, async listProfiles(home: string = homedir()): Promise[]> { @@ -118,30 +297,36 @@ export function createAuthStore( return file.profiles[name]; }, async saveProfile(profile: AuthProfile, home: string = homedir()): Promise { - const file = await readAuthFile(home); - file.profiles[profile.name] = profile; - await writeAuthFile(file, home); + return withStoreLock(home, async () => { + const file = await readAuthFile(home); + file.profiles[profile.name] = profile; + await writeAuthFile(file, home); + }); }, // Persist refreshed tokens for an existing profile, preserving createdAt. A // no-op if the profile no longer exists (e.g. removed in another session). async updateTokens(name: string, tokens: TTokens, home: string = homedir()): Promise { - const file = await readAuthFile(home); - const existing = file.profiles[name]; - if (existing === undefined) return; - file.profiles[name] = { ...existing, tokens }; - await writeAuthFile(file, home); + return withStoreLock(home, async () => { + const file = await readAuthFile(home); + const existing = file.profiles[name]; + if (existing === undefined) return; + file.profiles[name] = { ...existing, tokens }; + await writeAuthFile(file, home); + }); }, async removeProfile(name: string | undefined, home: string = homedir()): Promise { - const file = await readAuthFile(home); - if (name === undefined) { - const removed = Object.keys(file.profiles); - await writeAuthFile({ profiles: {} }, home); - return removed; - } - if (file.profiles[name] === undefined) return []; - Reflect.deleteProperty(file.profiles, name); - await writeAuthFile(file, home); - return [name]; + return withStoreLock(home, async () => { + const file = await readAuthFile(home); + if (name === undefined) { + const removed = Object.keys(file.profiles); + await writeAuthFile({ profiles: {} }, home); + return removed; + } + if (file.profiles[name] === undefined) return []; + Reflect.deleteProperty(file.profiles, name); + await writeAuthFile(file, home); + return [name]; + }); }, }; }