From 72be1b396c4b07d2ac1b118dba546d91f91dbf40 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Wed, 14 Oct 2026 13:44:13 -0700 Subject: [PATCH 1/2] refactor(auth): extract shared provider auth helpers (C1-C7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ~250 lines of near-identical callback/login/session/store/lock twins across codex and xai collapse onto seven shared helpers, each taking a small provider spec of explicit hooks. Deliberate provider differences stay hooks, not knobs: the codex inter-process refresh lock is a serializeRefresh hook (xai has none by design), accountId merge and token expiry stay per-provider, the divergent stale horizons carry must-not-unify comments, and exact-code vs substring error matching is untouched. Lock contention still never surfaces as credential_failure. One deliberate delta: the unified file lock propagates non-ENOENT fs errors where the codex refresh lock used to swallow every fs error; the store contract was the stronger one. The propagation is pinned at both levels — this suite exercises the refresh path and file-lock.test pins the primitive. --- src/auth/bearer-headers.ts | 12 + src/auth/codex/auth-headers.ts | 6 +- src/auth/codex/callback-server.ts | 31 +-- src/auth/codex/login.ts | 19 +- src/auth/codex/refresh-lock.test.ts | 107 ++++++++- src/auth/codex/refresh-lock.ts | 173 ++------------- src/auth/codex/session.ts | 195 +++++----------- src/auth/codex/store.ts | 29 +-- src/auth/file-lock.test.ts | 221 +++++++++++++++++++ src/auth/file-lock.ts | 282 ++++++++++++++++++++++++ src/auth/oauth-scope-check.ts | 65 +++--- src/auth/provider-auth-store.ts | 21 ++ src/auth/provider-callback-server.ts | 34 +++ src/auth/provider-login.ts | 40 ++++ src/auth/provider-token-session.test.ts | 205 +++++++++++++++++ src/auth/provider-token-session.ts | 222 +++++++++++++++++++ src/auth/store.ts | 191 ++-------------- src/auth/xai/auth-headers.ts | 6 +- src/auth/xai/callback-server.ts | 29 +-- src/auth/xai/login.ts | 16 +- src/auth/xai/session.ts | 147 +++++------- src/auth/xai/store.ts | 31 +-- 22 files changed, 1391 insertions(+), 691 deletions(-) create mode 100644 src/auth/bearer-headers.ts create mode 100644 src/auth/file-lock.test.ts create mode 100644 src/auth/file-lock.ts create mode 100644 src/auth/provider-auth-store.ts create mode 100644 src/auth/provider-callback-server.ts create mode 100644 src/auth/provider-login.ts create mode 100644 src/auth/provider-token-session.test.ts create mode 100644 src/auth/provider-token-session.ts diff --git a/src/auth/bearer-headers.ts b/src/auth/bearer-headers.ts new file mode 100644 index 000000000..9c8d7f7c3 --- /dev/null +++ b/src/auth/bearer-headers.ts @@ -0,0 +1,12 @@ +export interface BearerToken { + readonly access: string; +} + +// The one header every provider carries on an inference request; provider +// extras (account ids, client identifiers) ride alongside via the spread. +export function bearerHeaders( + token: BearerToken, + extra: Record = {}, +): Record { + return { authorization: `Bearer ${token.access}`, ...extra }; +} diff --git a/src/auth/codex/auth-headers.ts b/src/auth/codex/auth-headers.ts index 7b02f2e64..c41e03568 100644 --- a/src/auth/codex/auth-headers.ts +++ b/src/auth/codex/auth-headers.ts @@ -1,3 +1,4 @@ +import { bearerHeaders } from "../bearer-headers.js"; import { CODEX_CLIENT_VERSION, CODEX_ORIGINATOR } from "./constants.js"; export function codexAuthHeadersForToken( @@ -7,11 +8,10 @@ export function codexAuthHeadersForToken( }, commandName: string, ): Record { - const headers: Record = { - authorization: `Bearer ${token.access}`, + const headers = bearerHeaders(token, { originator: CODEX_ORIGINATOR, "user-agent": `${commandName} (${CODEX_ORIGINATOR}/${CODEX_CLIENT_VERSION})`, - }; + }); // An empty account id carries no identity — sending it as a header value // would label the request with a meaningless id. Only a non-empty id rides. if (token.accountId) headers["chatgpt-account-id"] = token.accountId; diff --git a/src/auth/codex/callback-server.ts b/src/auth/codex/callback-server.ts index 5a492f37d..8d9eaa2d5 100644 --- a/src/auth/codex/callback-server.ts +++ b/src/auth/codex/callback-server.ts @@ -1,10 +1,9 @@ -import { startCallbackServer, type CallbackServer } from "@corbits/oauth-core"; - import { - authorizationDoneHtml, - callbackPageHtml, - type CallbackPageCopy, -} from "../callback-page.js"; + startProviderCallbackServer, + type CallbackServer, +} from "../provider-callback-server.js"; + +import type { CallbackPageCopy } from "../callback-page.js"; import { CODEX_CALLBACK_HOST, CODEX_CALLBACK_PATH, @@ -19,13 +18,15 @@ export async function startCodexCallbackServer( expectedState: string, copy: CallbackPageCopy, ): Promise { - return startCallbackServer(expectedState, { - port: CODEX_CALLBACK_PORT, - // Codex's registered redirect_uri uses localhost (not 127.0.0.1). - host: CODEX_CALLBACK_HOST, - path: CODEX_CALLBACK_PATH, - doneHtml: authorizationDoneHtml("Codex", copy), - failedHtml: (reason) => - callbackPageHtml({ subject: "Codex", error: reason }, copy), - }); + return startProviderCallbackServer( + "Codex", + { + port: CODEX_CALLBACK_PORT, + // Codex's registered redirect_uri uses localhost (not 127.0.0.1). + host: CODEX_CALLBACK_HOST, + path: CODEX_CALLBACK_PATH, + }, + expectedState, + copy, + ); } diff --git a/src/auth/codex/login.ts b/src/auth/codex/login.ts index 2c3f3f03f..b98587347 100644 --- a/src/auth/codex/login.ts +++ b/src/auth/codex/login.ts @@ -1,15 +1,14 @@ import { - buildAuthorizeUrl, + exchangeCodexCode, + codexOAuthConfig, + type CodexTokens, +} from "@corbits/codex-provider"; +import { openInBrowser, - startOAuthLogin, type OAuthLoginHandle, type StartOAuthLoginOptions, } from "@corbits/oauth-core"; -import { - codexOAuthConfig, - exchangeCodexCode, - type CodexTokens, -} from "@corbits/codex-provider"; +import { startProviderLogin } from "../provider-login.js"; import type { CallbackPageCopy } from "../callback-page.js"; import { saveCodexProfile } from "../../config/oauth-stores.js"; @@ -28,10 +27,10 @@ export async function startCodexLogin( opts: StartCodexLoginOptions, ): Promise { const { home, copy, ...loginOpts } = opts; - return startOAuthLogin(loginOpts, { + return startProviderLogin(loginOpts, { + oauthConfig: codexOAuthConfig, startCallbackServer: (state) => startCodexCallbackServer(state, copy), - buildAuthorizeUrl: (pkce, state) => - buildAuthorizeUrl(codexOAuthConfig, pkce, state), + // The mapper leaves expiresAt unset when the response omits expires_in. exchangeCode: async (code, verifier, now) => withDefaultCodexExpiry(await exchangeCodexCode(code, verifier, now), now), saveProfile: (profile) => saveCodexProfile(profile, home), diff --git a/src/auth/codex/refresh-lock.test.ts b/src/auth/codex/refresh-lock.test.ts index 3eda8cab3..e2359e14e 100644 --- a/src/auth/codex/refresh-lock.test.ts +++ b/src/auth/codex/refresh-lock.test.ts @@ -9,10 +9,36 @@ import { import { tmpdir } from "node:os"; import { join } from "node:path"; import { describe, expect, test } from "bun:test"; -import { - CodexRefreshLockTimeoutError, - withCodexRefreshLock, -} from "./refresh-lock.js"; +import { CodexRefreshLockTimeoutError } from "./refresh-lock.js"; + +import { withMockedModule } from "../../../testkit/mock-module.js"; + +// Non-ENOENT lock-file failures must surface raw instead of being swallowed +// (the pre-unification codex lock caught every fs error). Flags gate each +// failure to its own test; undefined keeps every call on the real module. +let failingReadPath: string | undefined; +let failingStatPath: string | undefined; +let failingUnlinkPath: string | undefined; +await withMockedModule( + import.meta.resolve("node:fs/promises"), + (real: typeof import("node:fs/promises")) => ({ + ...real, + readFile: async (...args: Parameters) => { + if (args[0] === failingReadPath) throw new Error("lock read failed"); + return real.readFile(...args); + }, + stat: async (...args: Parameters) => { + if (args[0] === failingStatPath) throw new Error("lock stat failed"); + return real.stat(...args); + }, + unlink: async (...args: Parameters) => { + if (args[0] === failingUnlinkPath) throw new Error("lock release failed"); + return real.unlink(...args); + }, + }), +); + +const { withCodexRefreshLock } = await import("./refresh-lock.js"); async function tempDir(): Promise { return mkdtemp(join(tmpdir(), "cl8628-lock-")); @@ -214,4 +240,77 @@ describe("codex refresh lock", () => { await rm(dir, { recursive: true, force: true }); } }); + + test("a non-ENOENT read failure while contending propagates raw", async () => { + const dir = await tempDir(); + try { + const lock = join(dir, "refresh.lock"); + await writeFile(lock, ""); + failingReadPath = lock; + try { + // The unified lock re-reads the holder content while contending; a + // permission/disk failure there must surface, not read as "no lock". + await expect( + withCodexRefreshLock(lock, async () => "never", { + timeoutMs: 1_000, + retryMs: 10, + }), + ).rejects.toThrow("lock read failed"); + } finally { + failingReadPath = undefined; + } + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); + + test("a non-ENOENT stat failure while deciding staleness propagates raw", async () => { + const dir = await tempDir(); + try { + const lock = join(dir, "refresh.lock"); + // Untagged content routes the staleness decision through mtime stat; + // a non-ENOENT stat failure must surface, not read as "not stale". + await writeFile(lock, ""); + failingStatPath = lock; + try { + await expect( + withCodexRefreshLock(lock, async () => "never", { + timeoutMs: 1_000, + retryMs: 10, + }), + ).rejects.toThrow("lock stat failed"); + } finally { + failingStatPath = undefined; + } + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); + + test("a non-ENOENT release failure surfaces after the work value", async () => { + const dir = await tempDir(); + try { + const lock = join(dir, "refresh.lock"); + failingUnlinkPath = lock; + try { + // The refresh succeeded, but a release that fails for a real reason + // (not a steal's ENOENT) must not vanish: the caller has to know the + // lock file is still on disk. + await expect( + withCodexRefreshLock(lock, async () => "done", { + timeoutMs: 1_000, + retryMs: 10, + }), + ).rejects.toThrow("lock release failed"); + // The failed release leaves the claim file in place for recovery. + await expect(readFile(lock, "utf8")).resolves.toContain( + String(process.pid), + ); + } finally { + failingUnlinkPath = undefined; + } + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); }); diff --git a/src/auth/codex/refresh-lock.ts b/src/auth/codex/refresh-lock.ts index 01aecfdc2..5bfdbb92e 100644 --- a/src/auth/codex/refresh-lock.ts +++ b/src/auth/codex/refresh-lock.ts @@ -1,24 +1,22 @@ -import { mkdir, open, readFile, stat, unlink } from "node:fs/promises"; -import { randomUUID } from "node:crypto"; -import { dirname } from "node:path"; +import { withFileLock } from "../file-lock.js"; // Serializes Codex OAuth refresh-and-persist sections across processes that // share one credential store. Same-process callers chain on an in-memory -// tail; cross-process callers contend on an exclusive lock file with -// crashed-holder takeover via PID liveness; the inner TokenSession dedups -// overlapping refreshes. At most one refresh grant is in flight, so a second -// refresher sees the persisted result instead of racing refresh-token -// rotation and revoking its sibling. +// tail; cross-process callers contend on an exclusive lock file with a +// crashed-holder takeover via PID liveness. At most one refresh grant is in +// flight, so a second refresher sees the persisted result instead of racing +// refresh-token rotation and revoking its sibling. // // Accepted limits: PID liveness only means something on one host in one PID // namespace (ESRCH would steal a live holder's lock, a recycled PID stalls // recovery), and one lock path per store (two spellings contend on two // files). Same-host headless runs satisfy both. -const tails = new Map>(); // Stale horizon for legacy locks that carry no holder PID (foreign writers, // pre-tagging locks). Kept far above any legitimate hold and contender -// timeout so a live holder is never declared stale mid-wait. +// timeout so a live holder is never declared stale mid-wait. The provider +// auth store's own lock horizon is deliberately smaller (5s, short auth-file +// mutations); these two horizons are pinned invariants and must not unify. const DEFAULT_STALE_MS = 120_000; export interface CodexRefreshLockOptions { @@ -54,160 +52,17 @@ export class CodexRefreshLockTimeoutError extends Error { } } -// Holder tag: the creating PID (for crashed-holder liveness) plus a unique -// token, so only the creator removes the file — a belated release after a -// stale-takeover steal would otherwise hand two holders overlapping grants. -function holderTag(): string { - return `${String(process.pid)}:${randomUUID()}`; -} - -function holderPid(content: string): number | null { - const pid = Number(content.split(":")[0]?.trim()); - return Number.isInteger(pid) && pid > 0 ? pid : null; -} - -// kill(pid, 0) liveness: EPERM is alive (other user), ESRCH/EINVAL dead, -// anything else treated as alive so a confused signal check never steals a -// live holder's lock. Only meaningful within one PID namespace. -function isPidAlive(pid: number): boolean { - try { - process.kill(pid, 0); - return true; - } catch (err) { - const code = (err as NodeJS.ErrnoException)?.code; - return code !== "ESRCH" && code !== "EINVAL"; - } -} - -async function readLockContent(lockPath: string): Promise { - try { - return await readFile(lockPath, "utf8"); - } catch { - return null; - } -} - -// Safe to take over: a tagged holder with a dead PID, or an untagged lock -// older than the stale horizon. -async function isLockStale( - lockPath: string, - staleMs: number, - content: string | null, -): Promise { - if (content === null) return false; - const pid = holderPid(content); - if (pid !== null) return !isPidAlive(pid); - try { - const info = await stat(lockPath); - return Date.now() - info.mtimeMs > staleMs; - } catch { - // The lock vanished between read and stat: not ours to take over. - return false; - } -} - -async function acquireLockFile( - lockPath: string, - timeoutMs: number, - retryMs: number, - staleMs: number, - deadline: number, -): Promise { - await mkdir(dirname(lockPath), { recursive: true, mode: 0o700 }); - for (;;) { - const tag = holderTag(); - try { - const handle = await open(lockPath, "wx", 0o600); - await handle.writeFile(tag, "utf8"); - await handle.close(); - return tag; - } catch (err) { - if ((err as NodeJS.ErrnoException)?.code !== "EEXIST") throw err; - } - // A crashed holder never releases: unlink and take over. Re-check the - // content first so a concurrent winner's fresh lock is not unlinked. - const content = await readLockContent(lockPath); - if (content !== null && (await isLockStale(lockPath, staleMs, content))) { - if ((await readLockContent(lockPath)) === content) { - await unlink(lockPath).catch(() => undefined); - } - continue; - } - if (Date.now() >= deadline) - throw new CodexRefreshLockTimeoutError(lockPath, timeoutMs); - await new Promise((resolve) => setTimeout(resolve, retryMs)); - } -} - -// Bound the in-memory queue wait by the same deadline as file contention, -// or a backed-up queue stalls far longer than timeoutMs promises. -async function waitForTail( - previous: Promise, - lockPath: string, - timeoutMs: number, - deadline: number, -): Promise { - let timer: ReturnType | undefined; - try { - await Promise.race([ - previous.catch(() => undefined), - new Promise((_, reject) => { - const remaining = deadline - Date.now(); - if (remaining <= 0) { - reject(new CodexRefreshLockTimeoutError(lockPath, timeoutMs)); - return; - } - timer = setTimeout(() => { - reject(new CodexRefreshLockTimeoutError(lockPath, timeoutMs)); - }, remaining); - }), - ]); - } finally { - clearTimeout(timer); - } -} - -/** - * Runs `work` while holding the refresh lock at `lockPath`. Same-process - * callers queue in memory, other processes on the lock file; one deadline - * covers both, so the call holds or throws within ~timeoutMs. The creating - * holder removes the file. Callers must pass one canonical path per store. - */ export async function withCodexRefreshLock( lockPath: string, work: () => Promise, options: CodexRefreshLockOptions = {}, ): Promise { const timeoutMs = options.timeoutMs ?? 30_000; - const retryMs = options.retryMs ?? 25; - const staleMs = options.staleMs ?? DEFAULT_STALE_MS; - const deadline = Date.now() + timeoutMs; - const previous = tails.get(lockPath) ?? Promise.resolve(); - let releaseTail!: () => void; - const current = new Promise((resolve) => { - releaseTail = resolve; + return withFileLock(lockPath, work, { + timeoutMs, + retryMs: options.retryMs ?? 25, + staleMs: options.staleMs ?? DEFAULT_STALE_MS, + timeoutError: (lockPath: string, timeoutMs: number) => + new CodexRefreshLockTimeoutError(lockPath, timeoutMs), }); - tails.set(lockPath, current); - try { - await waitForTail(previous, lockPath, timeoutMs, deadline); - const tag = await acquireLockFile( - lockPath, - timeoutMs, - retryMs, - staleMs, - deadline, - ); - try { - return await work(); - } finally { - // Fenced release: remove the file only while it still carries our - // tag, or a stale-takeover steal would hand two holders grants. - if ((await readLockContent(lockPath)) === tag) { - await unlink(lockPath).catch(() => undefined); - } - } - } finally { - if (tails.get(lockPath) === current) tails.delete(lockPath); - releaseTail(); - } } diff --git a/src/auth/codex/session.ts b/src/auth/codex/session.ts index a7b4b7298..db4b9baed 100644 --- a/src/auth/codex/session.ts +++ b/src/auth/codex/session.ts @@ -1,31 +1,26 @@ -import { - createTokenSession, - isTokenExpired, - OAuthProfileNotFoundError, - OAuthRefreshFailedError, - type TokenSession, -} from "@corbits/oauth-core"; +import { OAuthProfileNotFoundError } from "@corbits/oauth-core"; import { CODEX_REFRESH_SKEW_MS, refreshCodexTokens, type CodexTokens, } from "@corbits/codex-provider"; +import type { TokenSession } from "@corbits/oauth-core"; +import type { InferenceErrorLike } from "../../inference-gateway-error.js"; import { - codexAuthPath, loadCodexProfile, + codexAuthPath, updateCodexTokens, } from "../../config/oauth-stores.js"; -import type { InferenceErrorLike } from "../../inference-gateway-error.js"; import { - replaceMutableTokens, - sanitizedRefreshFailure, -} from "../token-session-boundary.js"; -import { - CodexRefreshLockTimeoutError, withCodexRefreshLock, + CodexRefreshLockTimeoutError, } from "./refresh-lock.js"; import { withDefaultCodexExpiry } from "./store.js"; +import { + createProviderTokenSession, + type TokenSessionCallContext, +} from "../provider-token-session.js"; // Raised when a Codex profile cannot yield a usable access token: it is gone, // or its refresh token was revoked/expired. Carries the profile name and a @@ -70,111 +65,53 @@ export interface CodexAccess { accountId?: string | undefined; } -function wrapCodexAuthError(name: string, err: unknown): never { - if (err instanceof OAuthProfileNotFoundError) { - throw new CodexAuthError( - name, - "missing", - `Codex profile "${name}" is not authorized. Log in again.`, - ); - } - if (err instanceof OAuthRefreshFailedError) { - const cause = err.cause; - throw new CodexAuthError( - name, - "refresh-failed", - `Codex profile "${name}" could not be refreshed (${cause instanceof Error ? cause.message : String(cause)}). Log in again.`, - ); - } - throw err; -} - -const sessions = new Map>(); - -async function refreshCodexTokensForStore( - refreshToken: string, - now: number, - previous: CodexTokens, -): Promise { - return withDefaultCodexExpiry( - await refreshCodexTokens(refreshToken, now, previous), - now, - ); -} - -/** - * Independent Codex token session for a home directory. Each session carries - * its own in-flight deduplication, so two sessions over one home behave like - * two processes sharing one credential store. - */ -export function createCodexTokenSession( - home?: string, -): TokenSession { - const refreshBasis = new WeakMap(); - const inner = createTokenSession({ - skewMs: CODEX_REFRESH_SKEW_MS, - loadProfile: (name) => loadCodexProfile(name, home), - updateTokens: async (name, tokens) => { - const winner = await updateCodexTokens( - name, - tokens, - home, - refreshBasis.get(tokens), - ); - if (winner === undefined) throw new OAuthProfileNotFoundError(name); - replaceMutableTokens(tokens, winner.tokens); - }, - // createTokenSession only passes (refresh, now); mergeRefreshed supplies - // the prior tokens the package helper needs to keep chatgpt-account-id. - refreshTokens: async (refreshToken, now) => { - try { - const refreshed = await refreshCodexTokensForStore(refreshToken, now, { - access: "", - refresh: refreshToken, - }); - refreshBasis.set(refreshed, refreshToken); - return refreshed; - } catch (error) { - throw sanitizedRefreshFailure(error, refreshToken); - } - }, - toAccess: (tokens) => ({ - access: tokens.access, - accountId: tokens.accountId, - }), - mergeRefreshed: (refreshed, previous) => { - const merged = - refreshed.accountId === undefined && previous.accountId !== undefined - ? { ...refreshed, accountId: previous.accountId } - : refreshed; - const expectedRefreshToken = refreshBasis.get(refreshed); - if (expectedRefreshToken !== undefined) - refreshBasis.set(merged, expectedRefreshToken); - return merged; - }, - }); - return { - isExpired: inner.isExpired, - getValidToken: (name, now = Date.now()) => - withSerializedCodexRefresh(home, name, now, () => - inner.getValidToken(name, now), +export const codexTokensSession = createProviderTokenSession< + CodexTokens, + CodexAccess +>({ + errorClass: CodexAuthError, + providerLabel: "Codex", + skewMs: CODEX_REFRESH_SKEW_MS, + loadProfile: loadCodexProfile, + updateTokens: updateCodexTokens, + toAccess: (tokens) => ({ + access: tokens.access, + accountId: tokens.accountId, + }), + refreshTokens: async (refreshToken, now, previous) => + // The session-driven refresh has no prior tokens to lean on; the mapper + // only needs the refresh grant itself. Staged refreshes pass the real + // prior tokens through. + withDefaultCodexExpiry( + await refreshCodexTokens( + refreshToken, + now, + previous ?? { access: "", refresh: refreshToken }, ), - }; -} + now, + ), + // Keep the stored chatgpt account id when the refresh response omits + // id_token; the shared factory carries the refresh basis for the store CAS. + mergeRefreshed: (refreshed, previous) => + refreshed.accountId === undefined && previous.accountId !== undefined + ? { ...refreshed, accountId: previous.accountId } + : refreshed, + serializeRefresh: (ctx) => serializeCodexRefresh(ctx), +}); // Refreshes for one shared credential store serialize on a lock file so two // headless runs (or two sessions in one process) cannot hold overlapping // refresh grants and revoke each other under token rotation. Fresh tokens // resolve before the lock: a stalled refresh never blocks healthy readers. -async function withSerializedCodexRefresh( - home: string | undefined, - name: string, - now: number, - refresh: () => Promise, -): Promise { +async function serializeCodexRefresh({ + home, + name, + now, + getValid, +}: TokenSessionCallContext): Promise { const profile = await loadCodexProfile(name, home); if (profile === undefined) throw new OAuthProfileNotFoundError(name); - if (!isCodexTokenExpired(profile.tokens, now)) + if (!codexTokensSession.isExpired(profile.tokens, now)) return { access: profile.tokens.access, accountId: profile.tokens.accountId, @@ -182,7 +119,7 @@ async function withSerializedCodexRefresh( try { return await withCodexRefreshLock( `${codexAuthPath(home)}.refresh.lock`, - refresh, + getValid, ); } catch (err) { // A lock-blocked refresh is contention/crash hygiene, not a dead @@ -210,17 +147,19 @@ export function codexAuthFailureDiagnostic( return null; } -function sessionFor(home?: string): TokenSession { - const key = home ?? ""; - const existing = sessions.get(key); - if (existing !== undefined) return existing; - const created = createCodexTokenSession(home); - sessions.set(key, created); - return created; +/** + * Independent Codex token session for a home directory. Each session carries + * its own in-flight deduplication and serializes inter-process refreshes on + * the credential file lock. + */ +export function createCodexTokenSession( + home?: string, +): TokenSession { + return codexTokensSession.createSession(home); } export function isCodexTokenExpired(tokens: CodexTokens, now: number): boolean { - return isTokenExpired(tokens, now, CODEX_REFRESH_SKEW_MS); + return codexTokensSession.isExpired(tokens, now); } export async function getValidCodexToken( @@ -228,24 +167,12 @@ export async function getValidCodexToken( now?: number, home?: string, ): Promise { - try { - return await sessionFor(home).getValidToken(name, now); - } catch (err) { - wrapCodexAuthError(name, err); - } + return codexTokensSession.getValidToken(name, now, home); } export async function refreshStagedCodexTokens( tokens: CodexTokens, now: number = Date.now(), ): Promise { - if (!isCodexTokenExpired(tokens, now)) return tokens; - let refreshed: CodexTokens; - try { - refreshed = await refreshCodexTokensForStore(tokens.refresh, now, tokens); - } catch (error) { - throw sanitizedRefreshFailure(error, tokens.refresh); - } - replaceMutableTokens(tokens, refreshed); - return tokens; + return codexTokensSession.refreshStaged(tokens, now); } diff --git a/src/auth/codex/store.ts b/src/auth/codex/store.ts index 039178a37..6b2e196fc 100644 --- a/src/auth/codex/store.ts +++ b/src/auth/codex/store.ts @@ -2,7 +2,7 @@ import { type } from "arktype"; import type { AuthProfile } from "@corbits/oauth-core"; import type { CodexTokens } from "@corbits/codex-provider"; -import { createAuthStore } from "../store.js"; +import { defineProviderAuthStore } from "../provider-auth-store.js"; // On-disk store for Codex OAuth profiles: a user may hold multiple // subscriptions (personal, work, ...), so credentials are keyed by a @@ -12,17 +12,6 @@ export type { CodexTokens }; export type CodexProfile = AuthProfile; -const CodexTokensShape = type({ - access: "string", - refresh: "string", - expiresAt: "number", - "accountId?": "string", -}); - -function isCodexTokens(value: unknown): value is CodexTokens { - return !(CodexTokensShape(value) instanceof type.errors); -} - // The package mapper leaves expiresAt unset when the token endpoint omits // expires_in; disk profiles need a concrete expiry so the arktype guard can // load them. 3600s matches the previous host mapper. @@ -38,10 +27,12 @@ export function withDefaultCodexExpiry( export const CODEX_AUTH_FILENAME = "codex-auth.json"; -export function createCodexAuthStore(settingsDirName: string) { - return createAuthStore({ - filename: CODEX_AUTH_FILENAME, - settingsDirName, - isTokens: isCodexTokens, - }); -} +export const createCodexAuthStore = defineProviderAuthStore({ + filename: CODEX_AUTH_FILENAME, + tokensShape: type({ + access: "string", + refresh: "string", + expiresAt: "number", + "accountId?": "string", + }), +}); diff --git a/src/auth/file-lock.test.ts b/src/auth/file-lock.test.ts new file mode 100644 index 000000000..47cc8e0a5 --- /dev/null +++ b/src/auth/file-lock.test.ts @@ -0,0 +1,221 @@ +import { + mkdtemp, + readFile, + rm, + stat, + utimes, + writeFile, +} from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { describe, expect, test } from "bun:test"; + +import { withMockedModule } from "../../testkit/mock-module.js"; + +// File-lock level pins: every credential lock (the auth store's and the +// codex refresh lock's) routes through this primitive, so its contract is +// what the two consumers' horizons and release semantics lean on. Flags gate +// each injected failure to its own test; undefined keeps every call on the +// real module. +let failingReadPath: string | undefined; +let failingUnlinkPath: string | undefined; +await withMockedModule( + import.meta.resolve("node:fs/promises"), + (real: typeof import("node:fs/promises")) => ({ + ...real, + readFile: async (...args: Parameters) => { + if (args[0] === failingReadPath) throw new Error("lock read failed"); + return real.readFile(...args); + }, + unlink: async (...args: Parameters) => { + if (args[0] === failingUnlinkPath) throw new Error("lock release failed"); + return real.unlink(...args); + }, + }), +); + +const { withFileLock } = await import("./file-lock.js"); + +async function tempDir(): Promise { + return mkdtemp(join(tmpdir(), "file-lock-")); +} + +const staleMs = 1_000; + +// The store and the refresh lock carry their own failure shapes; the shared +// primitive only delegates to the injected factory. +const timeoutError = (lockPath: string, _timeoutMs: number): Error => + new Error(`timed out for ${lockPath}`); + +describe("withFileLock", () => { + test("serializes concurrent holders and removes the lock file", async () => { + const dir = await tempDir(); + try { + const lock = join(dir, "x.lock"); + let active = 0; + let maxActive = 0; + const results = await Promise.all( + [0, 1, 2, 3, 4].map((i) => + withFileLock( + lock, + async () => { + active += 1; + maxActive = Math.max(maxActive, active); + // The file exists while held so a second caller contends on it. + await stat(lock); + await new Promise((resolve) => setTimeout(resolve, 10)); + active -= 1; + return i; + }, + { timeoutMs: 5_000, retryMs: 10, staleMs, timeoutError }, + ), + ), + ); + expect(results).toEqual([0, 1, 2, 3, 4]); + expect(maxActive).toBe(1); + await expect(stat(lock)).rejects.toThrow(); + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); + + test("reports the injected timeout error on a foreign-held lock", async () => { + const dir = await tempDir(); + try { + const lock = join(dir, "x.lock"); + await writeFile(lock, ""); + await expect( + withFileLock(lock, async () => "never", { + timeoutMs: 100, + retryMs: 10, + staleMs, + timeoutError, + }), + ).rejects.toThrow(`timed out for ${lock}`); + // A timeout never touches the holder's file. + expect(await readFile(lock, "utf8")).toBe(""); + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); + + test("takes over a stale untagged lock with the configured staleMs", async () => { + const dir = await tempDir(); + try { + const lock = join(dir, "x.lock"); + await writeFile(lock, "legacy-orphan"); + const ancient = new Date(Date.now() - 60_000); + await utimes(lock, ancient, ancient); + // The stale horizon is a caller decision: the auth store uses a much + // smaller value than the refresh lock, and neither is baked in here. + expect( + await withFileLock(lock, async () => "taken-over", { + timeoutMs: 5_000, + retryMs: 10, + staleMs, + timeoutError, + }), + ).toBe("taken-over"); + await expect(stat(lock)).rejects.toThrow(); + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); + + test("takes over a dead PID holder regardless of the stale horizon", async () => { + const dir = await tempDir(); + try { + const lock = join(dir, "x.lock"); + // Dead holder in real claim format: liveness, not age, decides. + const exited = Bun.spawn(["bun", "--version"], { + stdout: "ignore", + stderr: "ignore", + }); + await exited.exited; + await writeFile(lock, `${String(exited.pid)}:crashed`); + expect( + await withFileLock(lock, async () => "recovered", { + timeoutMs: 5_000, + retryMs: 10, + staleMs: 120_000, + timeoutError, + }), + ).toBe("recovered"); + await expect(stat(lock)).rejects.toThrow(); + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); + + test("fenced release keeps a takeover holder's file in place", async () => { + const dir = await tempDir(); + try { + const lock = join(dir, "x.lock"); + await withFileLock( + lock, + async () => { + // Simulate a stale-takeover steal landing mid-hold: the victim's + // release must leave the new holder's file alone. + await writeFile(lock, "42424242:takeover-holder"); + }, + { timeoutMs: 5_000, retryMs: 10, staleMs, timeoutError }, + ); + expect(await readFile(lock, "utf8")).toBe("42424242:takeover-holder"); + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); + + test("propagates a non-ENOENT read failure during acquisition", async () => { + const dir = await tempDir(); + try { + const lock = join(dir, "x.lock"); + await writeFile(lock, ""); + failingReadPath = lock; + try { + // Both consumers inherit this off withFileLock: a permission or disk + // error while reading holder content must surface, not read as "no + // lock" and restart a blind takeover. + await expect( + withFileLock(lock, async () => "never", { + timeoutMs: 1_000, + retryMs: 10, + staleMs, + timeoutError, + }), + ).rejects.toThrow("lock read failed"); + } finally { + failingReadPath = undefined; + } + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); + + test("surfaces a non-ENOENT release failure instead of swallowing it", async () => { + const dir = await tempDir(); + try { + const lock = join(dir, "x.lock"); + failingUnlinkPath = lock; + try { + // The work value is discarded in favor of the release outcome; with + // ENOENT-only tolerance, anything else means the lock file survived. + await expect( + withFileLock(lock, async () => "done", { + timeoutMs: 1_000, + retryMs: 10, + staleMs, + timeoutError, + }), + ).rejects.toThrow("lock release failed"); + await expect(readFile(lock, "utf8")).resolves.toContain( + String(process.pid), + ); + } finally { + failingUnlinkPath = undefined; + } + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/src/auth/file-lock.ts b/src/auth/file-lock.ts new file mode 100644 index 000000000..d1863789a --- /dev/null +++ b/src/auth/file-lock.ts @@ -0,0 +1,282 @@ +import { + mkdir, + open, + readFile, + stat, + unlink, + type FileHandle, +} from "node:fs/promises"; +import { dirname } from "node:path"; +import { setTimeout as delay } from "node:timers/promises"; + +// File-lock primitives shared by every credential lock in this repo. The +// invariants that make them safe: per-waiter unique claims name the holder, +// kill(pid, 0) liveness takes over a crashed holder's lock, an untagged lock +// older than the stale horizon is presumed orphaned, a steal re-reads before +// unlinking so a concurrent winner's fresh lock is never removed, and both a +// post-create content check and a fenced release keep a loser from deleting +// or releasing under a live winner. + +export function isErrnoCode(err: unknown, code: string): boolean { + return ( + typeof err === "object" && + err !== null && + "code" in err && + err.code === code + ); +} + +export function isEnoent(err: unknown): boolean { + return isErrnoCode(err, "ENOENT"); +} + +// Per-waiter unique claim (pid + counter): pid alone is not unique per call, +// so the counter leg tells our claim from a concurrent winner's. +let lockClaimCounter = 0; + +export function newLockClaim(): string { + return `${String(process.pid)}:${String((lockClaimCounter += 1))}`; +} + +export function holderPid(content: string): number | null { + const pid = Number(content.split(":")[0]?.trim()); + return Number.isInteger(pid) && pid > 0 ? pid : null; +} + +// kill(pid, 0) liveness: success or EPERM means alive; ESRCH/EINVAL mean +// dead. Any other failure reads as alive — never steal a live holder's lock +// on a confused signal check; the waiter times out with a recovery hint. +export function isPidAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (err) { + return !isErrnoCode(err, "ESRCH") && !isErrnoCode(err, "EINVAL"); + } +} + +// Null when the lock vanished under the waiter (a release raced the read); +// anything but ENOENT propagates — permission and disk errors must surface. +export async function readLockContent( + lockPath: string, +): Promise { + try { + return await readFile(lockPath, "utf8"); + } catch (err) { + if (isEnoent(err)) return null; + throw err; + } +} + +// True when the lock is safe to take over: a tagged holder with a dead PID +// (crashed), or a legacy untagged lock older than the stale horizon. A +// vanished lock reads as not stale; the acquire loop retries the create. +export async function isLockStale( + lockPath: string, + staleMs: number, + content: string, +): Promise { + const pid = holderPid(content); + if (pid !== null) return !isPidAlive(pid); + try { + const info = await stat(lockPath); + return Date.now() - info.mtimeMs > staleMs; + } catch (err) { + if (isEnoent(err)) return false; + throw err; + } +} + +// Same-process callers queue on an in-memory tail per lock path so a backed-up +// queue stalls no longer than its own deadline. +const tails = new Map>(); + +// Called when the deadline passes without acquiring the lock. Each consumer +// carries its own failure shape (message text, cause, recovery hint). +export type FileLockTimeoutError = ( + lockPath: string, + timeoutMs: number, + cause: unknown, +) => Error; + +export interface FileLockOptions { + /** Maximum wait for the lock before giving up. */ + timeoutMs: number; + /** Poll interval while contending. Defaults to 25. */ + retryMs?: number; + /** + * Age at which an untagged holder is presumed crashed and the lock is taken + * over. Must stay far above the longest legitimate hold and any contender + * timeout, or a waiter steals a live holder's lock. PID-tagged locks ignore + * this: a dead PID takes over, a live one never does. + */ + staleMs: number; + timeoutError: FileLockTimeoutError; +} + +// Credential locks in this repo serialize on exactly this acquire/release +// pairing: exclusive-create with a holder tag, stale takeover, and a fenced +// claim-matched release. Do not fork a variant that drops any of the three. +export async function withFileLock( + lockPath: string, + work: () => Promise, + options: FileLockOptions, +): Promise { + const retryMs = options.retryMs ?? 25; + const deadline = Date.now() + options.timeoutMs; + + const previous = tails.get(lockPath) ?? Promise.resolve(); + let releaseTail!: () => void; + const current = new Promise((resolve) => { + releaseTail = resolve; + }); + tails.set(lockPath, current); + + try { + // Bound the queue wait by the same deadline as file contention. + let timer: ReturnType | undefined; + try { + await Promise.race([ + previous.catch(() => undefined), + new Promise((_, reject) => { + const remaining = deadline - Date.now(); + if (remaining <= 0) { + reject( + options.timeoutError(lockPath, options.timeoutMs, undefined), + ); + return; + } + timer = setTimeout(() => { + reject( + options.timeoutError(lockPath, options.timeoutMs, undefined), + ); + }, remaining); + }), + ]); + } finally { + clearTimeout(timer); + } + + const claim = newLockClaim(); + let lock: FileHandle | undefined; + + await mkdir(dirname(lockPath), { recursive: true, mode: 0o700 }); + for (;;) { + try { + const handle = await open(lockPath, "wx", 0o600); + try { + await handle.writeFile(claim, "utf8"); + } catch (writeError) { + await closeQuietly(handle); + // A half-written lock would read as an untakeable claim; remove it + // and surface the original failure. + try { + await unlink(lockPath); + } catch (unlinkError) { + if (!isEnoent(unlinkError)) throw unlinkError; + } + throw writeError; + } + // A steal may have replaced our fresh file between create and write; + // the path then names a live winner. Close and re-contend — never + // unlink. + if ((await readLockContent(lockPath)) !== claim) { + await closeQuietly(handle); + await pace(options, lockPath, retryMs, deadline, undefined); + continue; + } + lock = handle; + break; + } catch (error) { + if (!isErrnoCode(error, "EEXIST")) throw error; + // A crashed holder never releases: take over a stale lock rather + // than keep waiting. A lock that vanished under the read is not + // stale — retry the exclusive create. + const content = await readLockContent(lockPath); + if ( + content !== null && + (await isLockStale(lockPath, options.staleMs, content)) + ) { + // Re-check before unlinking so a concurrent winner's fresh claim + // is never mistaken for the stale entry just observed — claims + // are unique per waiter. A steal can still interleave between + // re-read and unlink; the post-create check above detects the + // loser and re-contends. + if ((await readLockContent(lockPath)) === content) { + try { + await unlink(lockPath); + } catch (unlinkError) { + if (!isEnoent(unlinkError)) throw unlinkError; + } + } + } + // Fall through to the deadline/sleep path: a steal that lost to a + // concurrent winner paces like any other contention. + await pace(options, lockPath, retryMs, deadline, error); + } + } + + const releaseClaim = async (): Promise => { + if (lock === undefined) return; + try { + await lock.close(); + } catch { + // The work outcome owns precedence; a close failure must not mask it. + } + try { + // Fenced release: remove the file only while it still carries our + // claim, or a stale-takeover steal would hand two holders grants. + if ((await readLockContent(lockPath)) === claim) { + await unlink(lockPath); + } + } catch (error) { + // A steal racing the release removes the file first — that is a + // normal outcome; anything else surfaces after the work value. + if (!isEnoent(error)) throw error; + } + }; + + let workOutcome: + | { ok: true; value: TResult } + | { ok: false; error: unknown }; + try { + workOutcome = { ok: true, value: await work() }; + } catch (error) { + workOutcome = { ok: false, error }; + } + let releaseOutcome: { ok: true } | { ok: false; error: unknown } = { + ok: true, + }; + try { + await releaseClaim(); + } catch (error) { + releaseOutcome = { ok: false, error }; + } + if (!workOutcome.ok) throw workOutcome.error; + if (!releaseOutcome.ok) throw releaseOutcome.error; + return workOutcome.value; + } finally { + if (tails.get(lockPath) === current) tails.delete(lockPath); + releaseTail(); + } +} + +async function pace( + options: FileLockOptions, + lockPath: string, + retryMs: number, + deadline: number, + cause: unknown, +): Promise { + if (Date.now() >= deadline) + throw options.timeoutError(lockPath, options.timeoutMs, cause); + await delay(retryMs); +} + +async function closeQuietly(handle: FileHandle): Promise { + try { + await handle.close(); + } catch { + // Close failures never mask the contended state of the lock file. + } +} diff --git a/src/auth/oauth-scope-check.ts b/src/auth/oauth-scope-check.ts index 13d36319e..21ddd7abe 100644 --- a/src/auth/oauth-scope-check.ts +++ b/src/auth/oauth-scope-check.ts @@ -97,21 +97,16 @@ function classifyStatus( return unavailable(providerLabel); } -async function checkCodexScope( - tokens: CodexTokens, - commandName: string, +// One probe shape per provider: refresh staged tokens into the request +// headers, hit the provider's catalog/list endpoint, and classify the +// response. Only the headers, endpoint, and timeout differ. +async function probeOAuthScope( + providerLabel: string, + authorizeHeaders: () => Promise>, + fetchEndpoint: (headers: Record) => Promise, ): Promise { - const providerLabel = "Codex"; try { - const headers = codexAuthHeadersForToken( - await refreshStagedCodexTokens(tokens), - commandName, - ); - const url = `${CODEX_BASE_URL}${CODEX_MODELS_PATH}?client_version=${encodeURIComponent(CODEX_CLIENT_VERSION)}`; - const res = await fetch(url, { - headers, - signal: AbortSignal.timeout(SCOPE_CHECK_TIMEOUT_MS), - }); + const res = await fetchEndpoint(await authorizeHeaders()); if (res.ok) return { status: "ok" }; return classifyStatus(res.status, providerLabel); } catch (err) { @@ -121,25 +116,37 @@ async function checkCodexScope( } } +async function checkCodexScope( + tokens: CodexTokens, + commandName: string, +): Promise { + return probeOAuthScope( + "Codex", + async () => + codexAuthHeadersForToken( + await refreshStagedCodexTokens(tokens), + commandName, + ), + (headers) => + fetch( + `${CODEX_BASE_URL}${CODEX_MODELS_PATH}?client_version=${encodeURIComponent(CODEX_CLIENT_VERSION)}`, + { headers, signal: AbortSignal.timeout(SCOPE_CHECK_TIMEOUT_MS) }, + ), + ); +} + async function checkXaiScope( tokens: XaiTokens, ): Promise { - const providerLabel = "Grok"; - try { - const headers = xaiAuthHeadersForToken( - await refreshStagedXaiTokens(tokens), - ); - const res = await fetch(`${XAI_BASE_URL}/models`, { - headers, - signal: AbortSignal.timeout(XAI_TOKEN_TIMEOUT_MS), - }); - if (res.ok) return { status: "ok" }; - return classifyStatus(res.status, providerLabel); - } catch (err) { - if (isDefinitiveRefreshAuthRejection(err)) - return invalidCredentials(providerLabel); - return unavailable(providerLabel); - } + return probeOAuthScope( + "Grok", + async () => xaiAuthHeadersForToken(await refreshStagedXaiTokens(tokens)), + (headers) => + fetch(`${XAI_BASE_URL}/models`, { + headers, + signal: AbortSignal.timeout(XAI_TOKEN_TIMEOUT_MS), + }), + ); } // Probe an OAuth-issued token against the provider's own catalog/list diff --git a/src/auth/provider-auth-store.ts b/src/auth/provider-auth-store.ts new file mode 100644 index 000000000..ab2978b86 --- /dev/null +++ b/src/auth/provider-auth-store.ts @@ -0,0 +1,21 @@ +import { type, type Type } from "arktype"; +import type { BaseTokens } from "@corbits/oauth-core"; + +import { createAuthStore, type AuthStore } from "./store.js"; + +export interface ProviderAuthStoreDefinition { + filename: string; + tokensShape: Type; +} + +// The per-provider auth store definition: one filename plus the arktype +// shape guarding what may load from disk; nothing else differs. +export function defineProviderAuthStore( + definition: ProviderAuthStoreDefinition, +): (settingsDirName: string) => AuthStore { + const { filename, tokensShape } = definition; + const isTokens = (value: unknown): value is TTokens => + !(tokensShape(value) instanceof type.errors); + return (settingsDirName: string) => + createAuthStore({ filename, settingsDirName, isTokens }); +} diff --git a/src/auth/provider-callback-server.ts b/src/auth/provider-callback-server.ts new file mode 100644 index 000000000..eef06328c --- /dev/null +++ b/src/auth/provider-callback-server.ts @@ -0,0 +1,34 @@ +import { startCallbackServer, type CallbackServer } from "@corbits/oauth-core"; + +import { + authorizationDoneHtml, + callbackPageHtml, + type CallbackPageCopy, +} from "./callback-page.js"; + +export type { CallbackServer }; + +export interface ProviderCallbackServerConfig { + port: number; + host?: string | undefined; + path: string; +} + +// One loopback OAuth redirect server per provider: identical plumbing, with +// only the provider's callback target and its name in the callback pages. +export async function startProviderCallbackServer( + providerLabel: string, + config: ProviderCallbackServerConfig, + expectedState: string, + copy: CallbackPageCopy, +): Promise { + return startCallbackServer(expectedState, { + port: config.port, + // exactOptionalPropertyTypes: host only rides when a provider selects one. + ...(config.host === undefined ? {} : { host: config.host }), + path: config.path, + doneHtml: authorizationDoneHtml(providerLabel, copy), + failedHtml: (reason) => + callbackPageHtml({ subject: providerLabel, error: reason }, copy), + }); +} diff --git a/src/auth/provider-login.ts b/src/auth/provider-login.ts new file mode 100644 index 000000000..4162c801d --- /dev/null +++ b/src/auth/provider-login.ts @@ -0,0 +1,40 @@ +import { + buildAuthorizeUrl, + startOAuthLogin, + type CallbackServer, + type OAuthClientConfig, + type OAuthLoginHandle, + type StartOAuthLoginOptions, +} from "@corbits/oauth-core"; +import type { BaseTokens } from "@corbits/oauth-core"; + +export interface ProviderLoginSpec { + oauthConfig: OAuthClientConfig; + startCallbackServer: (expectedState: string) => Promise; + exchangeCode: ( + code: string, + verifier: string, + now: number, + ) => Promise; + saveProfile: (profile: { + name: string; + tokens: TTokens; + createdAt: number; + }) => Promise; +} + +// The shared PKCE login skeleton: every provider supplies its OAuth config, +// exchange, and persistence while the loopback flow stays in one place. +export function startProviderLogin( + opts: StartOAuthLoginOptions, + spec: ProviderLoginSpec, +): Promise> { + return startOAuthLogin(opts, { + startCallbackServer: spec.startCallbackServer, + buildAuthorizeUrl: (pkce, state) => + buildAuthorizeUrl(spec.oauthConfig, pkce, state), + exchangeCode: (code, verifier, now) => + spec.exchangeCode(code, verifier, now), + saveProfile: (profile) => spec.saveProfile(profile), + }); +} diff --git a/src/auth/provider-token-session.test.ts b/src/auth/provider-token-session.test.ts new file mode 100644 index 000000000..0d3865067 --- /dev/null +++ b/src/auth/provider-token-session.test.ts @@ -0,0 +1,205 @@ +import { describe, expect, test } from "bun:test"; +import { createProviderTokenSession } from "./provider-token-session.js"; + +type TestTokens = { + access: string; + refresh: string; + expiresAt: number; +}; + +type TestAccess = { access: string }; + +class TestAuthError extends Error { + readonly profile: string; + readonly reason: "missing" | "refresh-failed"; + + constructor( + profile: string, + reason: "missing" | "refresh-failed", + message: string, + ) { + super(message); + this.name = "TestAuthError"; + this.profile = profile; + this.reason = reason; + } +} + +type Profile = { name: string; createdAt: number; tokens: TestTokens }; + +function buildSpec(overrides?: { + refreshOutcome?: TestTokens | Error; + mergeRefreshed?: boolean; +}) { + // One in-memory profile store behind the same shape the disk store has, + // including the expectedRefreshToken CAS the session feeds it. + const store = new Map([ + [ + "p", + { + name: "p", + createdAt: 0, + tokens: { access: "old", refresh: "r1", expiresAt: 1_000 }, + }, + ], + ]); + const refreshCalls: { + refreshToken: string; + now: number; + previousAccess: string | undefined; + previousRefresh: string | undefined; + }[] = []; + const spec = { + errorClass: TestAuthError, + providerLabel: "Test", + skewMs: 0, + loadProfile: async (name: string) => store.get(name), + updateTokens: async ( + name: string, + tokens: TestTokens, + home: string | undefined, + expectedRefreshToken: string | undefined, + ) => { + if (name !== "p" || home !== undefined) return undefined; + const existing = store.get("p"); + if (existing === undefined) return undefined; + if ( + expectedRefreshToken !== undefined && + existing.tokens.refresh !== expectedRefreshToken + ) + return existing; + const updated = { ...existing, tokens }; + store.set("p", updated); + return updated; + }, + toAccess: (tokens: TestTokens) => ({ access: tokens.access }), + refreshTokens: async ( + refreshToken: string, + now: number, + previous?: TestTokens, + ) => { + // Capture the staged prior tokens at call time: replaceMutableTokens + // mutates the passed object in place after this hook returns. + refreshCalls.push({ + refreshToken, + now, + previousAccess: previous?.access, + previousRefresh: previous?.refresh, + }); + const outcome = overrides?.refreshOutcome; + if (outcome instanceof Error) throw outcome; + return ( + outcome ?? { access: "fresh", refresh: "r2", expiresAt: now + 10_000 } + ); + }, + }; + const api = createProviderTokenSession({ + ...spec, + ...(overrides?.mergeRefreshed + ? { + mergeRefreshed: (refreshed: TestTokens, previous: TestTokens) => ({ + ...refreshed, + access: `${refreshed.access}+${previous.refresh}`, + }), + } + : {}), + }); + return { api, store, refreshCalls }; +} + +describe("createProviderTokenSession", () => { + test("refreshes an expired profile through the provider spec", async () => { + const { api, store } = buildSpec(); + expect(await api.getValidToken("p", 5_000)).toEqual({ access: "fresh" }); + expect(store.get("p")?.tokens).toEqual({ + access: "fresh", + refresh: "r2", + expiresAt: 15_000, + }); + expect(store.get("p")?.createdAt).toBe(0); + }); + + test("a concurrent store winner is never clobbered by the CAS basis", async () => { + // Another session rotates the store to refresh "r3" while this one's + // grant (presented r1) is in flight: the loser observes the committed + // winner instead of overwriting it. + const { api, store } = buildSpec(); + const first = api.getValidToken("p", 5_000); + store.set("p", { + name: "p", + createdAt: 0, + tokens: { access: "winner", refresh: "r3", expiresAt: 15_000 }, + }); + expect(await first).toEqual({ access: "winner" }); + expect(store.get("p")?.tokens.access).toBe("winner"); + }); + + test("the per-provider merge hook runs before the store write", async () => { + const { api, store } = buildSpec({ + mergeRefreshed: true, + refreshOutcome: { access: "fresh", refresh: "r2", expiresAt: 15_000 }, + }); + await api.getValidToken("p", 5_000); + expect(store.get("p")?.tokens).toEqual({ + access: "fresh+r1", + refresh: "r2", + expiresAt: 15_000, + }); + }); + + test("staged refresh hands the prior tokens to the provider hook", async () => { + const { api, refreshCalls } = buildSpec(); + expect( + await api.refreshStaged( + { access: "old2", refresh: "r1", expiresAt: 1_000 }, + 5_000, + ), + ).toEqual({ access: "fresh", refresh: "r2", expiresAt: 15_000 }); + expect(refreshCalls.length).toBe(1); + expect(refreshCalls[0]?.refreshToken).toBe("r1"); + expect(refreshCalls[0]?.previousAccess).toBe("old2"); + }); + + test("wraps core failures into the provider error with its label", async () => { + const { api } = buildSpec(); + const missing = await api + .getValidToken("ghost", 5_000) + .catch((err: unknown) => err); + expect(missing).toBeInstanceOf(TestAuthError); + expect(missing).toMatchObject({ profile: "ghost", reason: "missing" }); + expect((missing as TestAuthError).message).toContain( + 'Test profile "ghost" is not authorized. Log in again.', + ); + + const refreshFailed = await buildSpec({ + refreshOutcome: new Error("grant rejected"), + }) + .api.getValidToken("p", 5_000) + .catch((err: unknown) => err); + expect(refreshFailed).toBeInstanceOf(TestAuthError); + expect(refreshFailed).toMatchObject({ + profile: "p", + reason: "refresh-failed", + }); + expect((refreshFailed as TestAuthError).message).toContain( + 'Test profile "p" could not be refreshed (grant rejected). Log in again.', + ); + }); + + test("absent serializeRefresh hook exposes the inner session unwrapped", async () => { + // The blocker contract: absent hook = no lock, no wrapping at all. + const { api } = buildSpec(); + const session = api.createSession(); + expect(Object.keys(session).sort()).toEqual(["getValidToken", "isExpired"]); + }); + + test("stale tokens within the skew read as expired", async () => { + const { api } = buildSpec(); + expect( + api.isExpired({ access: "a", refresh: "r", expiresAt: 5_000 }, 5_000), + ).toBe(true); + expect( + api.isExpired({ access: "a", refresh: "r", expiresAt: 6_000 }, 5_000), + ).toBe(false); + }); +}); diff --git a/src/auth/provider-token-session.ts b/src/auth/provider-token-session.ts new file mode 100644 index 000000000..0764fc888 --- /dev/null +++ b/src/auth/provider-token-session.ts @@ -0,0 +1,222 @@ +import { + createTokenSession, + isTokenExpired, + OAuthProfileNotFoundError, + OAuthRefreshFailedError, + type AuthProfile, + type BaseTokens, + type TokenSession, +} from "@corbits/oauth-core"; + +import { + replaceMutableTokens, + sanitizedRefreshFailure, +} from "./token-session-boundary.js"; + +export type ProviderAuthErrorReason = "missing" | "refresh-failed"; + +// The auth failure every provider surfaces: the profile name plus a reason +// ("missing" vs "refresh-failed") for re-login messaging. Classification is +// identity-based, so each provider keeps its own Error subclass and passes +// its constructor here. +export type ProviderAuthError = Error & { + readonly profile: string; + readonly reason: ProviderAuthErrorReason; +}; + +export type ProviderAuthErrorClass = new ( + profile: string, + reason: ProviderAuthErrorReason, + message: string, +) => ProviderAuthError; + +// Where the session posts refreshed tokens back to the store: the winner is +// this update, a concurrent refresh's value, or undefined after removal. +export type ProviderTokenStoreUpdate = ( + name: string, + tokens: TTokens, + home: string | undefined, + expectedRefreshToken: string | undefined, +) => Promise | undefined>; + +// Context one getValidToken call hands to the reconcile hook: `session` is +// the unwrapped inner session, and `getValid` the raw store-driven path. +export interface TokenSessionCallContext { + home?: string | undefined; + name: string; + now: number; + session: TokenSession; + getValid: () => Promise; +} + +export interface ProviderTokenSessionSpec { + errorClass: ProviderAuthErrorClass; + /** Profile label in re-login messaging, e.g. "Codex" or "xAI". */ + providerLabel: string; + skewMs: number; + loadProfile: ( + name: string, + home?: string, + ) => Promise | undefined>; + updateTokens: ProviderTokenStoreUpdate; + toAccess: (tokens: TTokens) => TAccess; + // Fresh tokens from a stored refresh token. `previous` rides along for + // staged refreshes (full prior tokens) and is absent on the session's + // endpoint-driven refresh. + refreshTokens: ( + refreshToken: string, + now: number, + previous?: TTokens, + ) => Promise; + // Token-shape extensions stay per provider: codex keeps the chatgpt + // account id when a refresh response omits id_token. Absent = pass-through. + mergeRefreshed?: (refreshed: TTokens, previous: TTokens) => TTokens; + // Post-wrap of getValidToken for every session over a credential home. + // Codex serializes inter-process refresh acquisitions behind a lock file; + // xAI recovers a committed concurrent winner after a lost invalid_grant + // race. Absent hook = no wrapping at all (no lock by design). + serializeRefresh?: ( + ctx: TokenSessionCallContext, + ) => Promise; +} + +export interface ProviderTokenSessionApi { + createSession: (home?: string) => TokenSession; + sessionFor: (home?: string) => TokenSession; + isExpired: (tokens: TTokens, now: number) => boolean; + getValidToken: ( + name: string, + now?: number, + home?: string, + ) => Promise; + refreshStaged: (tokens: TTokens, now?: number) => Promise; +} + +// The provider token-session lifecycle, once: per-home session cache, the +// refresh-basis WeakMap feeding the store's credential CAS, sanitized +// endpoint failures, and the profile errors. Only the spec's provider hooks +// differ between providers (see ProviderTokenSessionSpec). +export function createProviderTokenSession( + spec: ProviderTokenSessionSpec, +): ProviderTokenSessionApi { + const isExpired = (tokens: TTokens, now: number): boolean => + isTokenExpired(tokens, now, spec.skewMs); + + const sessions = new Map>(); + + // Each session carries its own in-flight deduplication, so two sessions + // over one home behave like two processes sharing one credential store. + const createSession = (home?: string): TokenSession => { + // Map each refreshed token to the raw refresh token that produced it, + // so updateTokens can CAS-compare at the store; it survives the merge + // and the mutable-token swap inside the store update. + const refreshBasis = new WeakMap(); + + const inner = createTokenSession({ + skewMs: spec.skewMs, + loadProfile: (name) => spec.loadProfile(name, home), + updateTokens: async (name, tokens) => { + const winner = await spec.updateTokens( + name, + tokens, + home, + refreshBasis.get(tokens), + ); + if (winner === undefined) throw new OAuthProfileNotFoundError(name); + replaceMutableTokens(tokens, winner.tokens); + }, + refreshTokens: async (refreshToken, now) => { + try { + const refreshed = await spec.refreshTokens(refreshToken, now); + refreshBasis.set(refreshed, refreshToken); + return refreshed; + } catch (error) { + throw sanitizedRefreshFailure(error, refreshToken); + } + }, + toAccess: spec.toAccess, + mergeRefreshed: (refreshed, previous) => { + const merged = spec.mergeRefreshed + ? spec.mergeRefreshed(refreshed, previous) + : refreshed; + const basis = refreshBasis.get(refreshed); + if (basis !== undefined) refreshBasis.set(merged, basis); + return merged; + }, + }); + + if (spec.serializeRefresh === undefined) return inner; + const serialize = spec.serializeRefresh; + const nowOrDefault = (now: number | undefined): number => + now === undefined ? Date.now() : now; + return { + isExpired: inner.isExpired, + getValidToken: (name, now) => { + const at = nowOrDefault(now); + return serialize({ + home, + name, + now: at, + session: inner, + getValid: () => inner.getValidToken(name, at), + }); + }, + }; + }; + + const sessionFor = (home?: string): TokenSession => { + const key = home ?? ""; + const existing = sessions.get(key); + if (existing !== undefined) return existing; + const created = createSession(home); + sessions.set(key, created); + return created; + }; + + // Projects the core's own errors onto the provider error: anything else is + // not ours to reclassify (e.g. a refresh lock timeout carries its own + // recovery path and must never compose into credential_failure). + const wrapAuthFailure = (name: string, err: unknown): unknown => { + if (err instanceof OAuthProfileNotFoundError) { + return new spec.errorClass( + name, + "missing", + `${spec.providerLabel} profile "${name}" is not authorized. Log in again.`, + ); + } + if (err instanceof OAuthRefreshFailedError) { + const cause = err.cause; + return new spec.errorClass( + name, + "refresh-failed", + `${spec.providerLabel} profile "${name}" could not be refreshed ` + + `(${cause instanceof Error ? cause.message : String(cause)}). Log in again.`, + ); + } + return err; + }; + + return { + createSession, + sessionFor, + isExpired, + getValidToken: async (name, now, home) => { + try { + return await sessionFor(home).getValidToken(name, now); + } catch (err) { + throw wrapAuthFailure(name, err); + } + }, + refreshStaged: async (tokens, now = Date.now()) => { + if (!isExpired(tokens, now)) return tokens; + let refreshed: TTokens; + try { + refreshed = await spec.refreshTokens(tokens.refresh, now, tokens); + } catch (error) { + throw sanitizedRefreshFailure(error, tokens.refresh); + } + replaceMutableTokens(tokens, refreshed); + return tokens; + }, + }; +} diff --git a/src/auth/store.ts b/src/auth/store.ts index 2b7bce936..e020a6e8c 100644 --- a/src/auth/store.ts +++ b/src/auth/store.ts @@ -1,17 +1,9 @@ -import { - mkdir, - open, - readFile, - rename, - stat, - unlink, - writeFile, -} from "node:fs/promises"; +import { mkdir, readFile, rename, writeFile } from "node:fs/promises"; import { homedir } from "node:os"; import { dirname, join } from "node:path"; -import { setTimeout as delay } from "node:timers/promises"; import { type } from "arktype"; import type { AuthProfile, BaseTokens } from "@corbits/oauth-core"; +import { isErrnoCode, withFileLock } from "./file-lock.js"; export type { AuthProfile, BaseTokens }; @@ -57,16 +49,15 @@ const LOCK_TIMEOUT_MS = 1_000; // Age at which a lock file with no holder PID (a foreign writer, or one // predating PID tagging) is presumed orphaned and taken over. Far above // LOCK_TIMEOUT_MS so a waiter never declares a live holder stale mid-wait. +// Deliberately smaller than the codex refresh-lock horizon (120s, refresh +// grants are long); these two horizons are pinned invariants and must not +// unify. const LOCK_STALE_MS = 5_000; // Per-call unique temp (pid + counter); pid alone is not unique if // writeAuthFile ever overlaps in-process. let tmpWriteCounter = 0; -// Per-waiter unique lock claim (pid + counter): a matching steal re-read -// names the same file, and the post-create check tells our claim from a winner's. -let lockClaimCounter = 0; - // Same-process ops on one auth file queue here so a caller's lock deadline // starts when it runs, not when it was invoked — one held-past-timeout lock // then fails only its first waiter, not the whole burst. @@ -82,15 +73,6 @@ const ProfileShape = type({ tokens: "unknown", }); -function isErrnoCode(err: unknown, code: string): boolean { - return ( - typeof err === "object" && - err !== null && - "code" in err && - err.code === code - ); -} - function isProfile( value: unknown, isTokens: (value: unknown) => value is TTokens, @@ -100,53 +82,6 @@ function isProfile( return isTokens(parsed.tokens); } -function holderPid(content: string): number | null { - const pid = Number(content.split(":")[0]?.trim()); - return Number.isInteger(pid) && pid > 0 ? pid : null; -} - -// kill(pid, 0) liveness: success or EPERM means alive; ESRCH/EINVAL mean -// dead. Any other failure reads as alive — never steal a live holder's lock -// on a confused signal check; the waiter times out with a recovery hint. -function isPidAlive(pid: number): boolean { - try { - process.kill(pid, 0); - return true; - } catch (err) { - const code = (err as NodeJS.ErrnoException)?.code; - return code !== "ESRCH" && code !== "EINVAL"; - } -} - -// Null when the lock vanished under the waiter (a release raced the read); -// anything but ENOENT propagates — permission and disk errors must surface. -async function readLockContent(lockPath: string): Promise { - try { - return await readFile(lockPath, "utf8"); - } catch (err) { - if (isErrnoCode(err, "ENOENT")) return null; - throw err; - } -} - -// True when the lock is safe to take over: a tagged holder with a dead PID -// (crashed), or a legacy untagged lock older than the stale horizon. A -// vanished lock reads as not stale; the acquire loop retries the create. -async function isLockStale( - lockPath: string, - content: string, -): Promise { - const pid = holderPid(content); - if (pid !== null) return !isPidAlive(pid); - try { - const info = await stat(lockPath); - return Date.now() - info.mtimeMs > LOCK_STALE_MS; - } catch (err) { - if (isErrnoCode(err, "ENOENT")) return false; - throw err; - } -} - function lockTimeoutError(lockPath: string, cause: unknown): Error { return new Error( `Timed out waiting for OAuth credential lock ${lockPath}. ` + @@ -155,17 +90,6 @@ function lockTimeoutError(lockPath: string, cause: unknown): Error { ); } -// Pace one contention round: throw when the deadline passed, else sleep a -// retry interval. All wait paths funnel here so steal contention never hot-spins. -async function paceLockWait( - deadline: number, - lockPath: string, - cause: unknown, -): Promise { - if (Date.now() >= deadline) throw lockTimeoutError(lockPath, cause); - await delay(LOCK_RETRY_MS); -} - export function createAuthStore( options: AuthStoreOptions, ): AuthStore { @@ -213,103 +137,14 @@ export function createAuthStore( home: string, callback: () => Promise, ): Promise { - const path = authPath(home); - const lockPath = `${path}.lock`; - await mkdir(dirname(path), { recursive: true, mode: 0o700 }); - const deadline = Date.now() + (options.lockTimeoutMs ?? LOCK_TIMEOUT_MS); - // Counter leg keeps every waiter's claim distinct. - const claim = `${process.pid}:${(lockClaimCounter += 1)}`; - let lock; - - while (true) { - try { - const handle = await open(lockPath, "wx", 0o600); - try { - await handle.writeFile(claim, "utf8"); - } catch (writeError) { - try { - await handle.close(); - } catch { - // Ignore close errors on the cleanup path; the write error below is the one the caller must see. - } - try { - await unlink(lockPath); - } catch (unlinkError) { - if (!isErrnoCode(unlinkError, "ENOENT")) throw unlinkError; - } - throw writeError; - } - // A steal may have replaced our fresh file between create and write; - // the path then names a live winner. Close and re-contend — never unlink. - if ((await readLockContent(lockPath)) !== claim) { - try { - await handle.close(); - } catch { - // The path names a live winner; close outcome must not mask the paced retry below. - } - await paceLockWait(deadline, lockPath, undefined); - continue; - } - lock = handle; - break; - } catch (error) { - if (!isErrnoCode(error, "EEXIST")) throw error; - // A crashed holder never releases: take over a stale lock rather - // than brick the store. A lock that vanished under the read is not - // stale — retry the exclusive create. - const content = await readLockContent(lockPath); - if (content !== null && (await isLockStale(lockPath, content))) { - // Re-check before unlinking so a concurrent winner's fresh claim - // is never mistaken for the stale entry just observed — claims - // are unique per waiter. A steal can still interleave between - // re-read and unlink; the post-create check then detects the - // loser and re-contends. - if ((await readLockContent(lockPath)) === content) { - try { - await unlink(lockPath); - } catch (unlinkError) { - // A concurrent winner unlinked first; retry the create. - if (!isErrnoCode(unlinkError, "ENOENT")) throw unlinkError; - } - } - // Fall through to the deadline/sleep path: a steal that lost to - // a concurrent winner paces like any other contention. - } - await paceLockWait(deadline, lockPath, error); - } - } - - let callbackOutcome: - | { ok: true; value: TResult } - | { ok: false; error: unknown }; - try { - callbackOutcome = { ok: true, value: await callback() }; - } catch (error) { - callbackOutcome = { ok: false, error }; - } - - try { - await lock.close(); - } catch { - // The callback outcome owns precedence; a close failure must not mask it. - // The unlink below still runs. - } - - let releaseOutcome: { ok: true } | { ok: false; error: unknown } = { - ok: true, - }; - try { - await unlink(lockPath); - } catch (error) { - // A stale-takeover steal may legitimately remove the file first. - if (!isErrnoCode(error, "ENOENT")) { - releaseOutcome = { ok: false, error }; - } - } - - if (!callbackOutcome.ok) throw callbackOutcome.error; - if (!releaseOutcome.ok) throw releaseOutcome.error; - return callbackOutcome.value; + const lockPath = `${authPath(home)}.lock`; + return withFileLock(lockPath, callback, { + timeoutMs: options.lockTimeoutMs ?? LOCK_TIMEOUT_MS, + retryMs: LOCK_RETRY_MS, + staleMs: LOCK_STALE_MS, + timeoutError: (lockPath, _timeoutMs, cause) => + lockTimeoutError(lockPath, cause), + }); } function enqueueAuthFileOp( diff --git a/src/auth/xai/auth-headers.ts b/src/auth/xai/auth-headers.ts index d66796dcf..660d63077 100644 --- a/src/auth/xai/auth-headers.ts +++ b/src/auth/xai/auth-headers.ts @@ -1,3 +1,4 @@ +import { bearerHeaders } from "../bearer-headers.js"; import { XAI_CLIENT_IDENTIFIER, XAI_CLIENT_VERSION, @@ -8,12 +9,11 @@ import { export function xaiAuthHeadersForToken(token: { readonly access: string; }): Record { - const headers: Record = { - authorization: `Bearer ${token.access}`, + const headers = bearerHeaders(token, { "user-agent": XAI_USER_AGENT, "x-grok-client-identifier": XAI_CLIENT_IDENTIFIER, "x-grok-client-version": XAI_CLIENT_VERSION, - }; + }); const userId = xaiUserIdFromAccessToken(token.access); if (userId !== undefined) headers["x-grok-user-id"] = userId; return headers; diff --git a/src/auth/xai/callback-server.ts b/src/auth/xai/callback-server.ts index f3788bbf7..23e9123ab 100644 --- a/src/auth/xai/callback-server.ts +++ b/src/auth/xai/callback-server.ts @@ -1,10 +1,9 @@ -import { startCallbackServer, type CallbackServer } from "@corbits/oauth-core"; - import { - authorizationDoneHtml, - callbackPageHtml, - type CallbackPageCopy, -} from "../callback-page.js"; + startProviderCallbackServer, + type CallbackServer, +} from "../provider-callback-server.js"; + +import type { CallbackPageCopy } from "../callback-page.js"; import { XAI_CALLBACK_HOST, XAI_CALLBACK_PATH, @@ -17,12 +16,14 @@ export async function startXaiCallbackServer( expectedState: string, copy: CallbackPageCopy, ): Promise { - return startCallbackServer(expectedState, { - port: XAI_CALLBACK_PORT, - host: XAI_CALLBACK_HOST, - path: XAI_CALLBACK_PATH, - doneHtml: authorizationDoneHtml("xAI", copy), - failedHtml: (reason) => - callbackPageHtml({ subject: "xAI", error: reason }, copy), - }); + return startProviderCallbackServer( + "xAI", + { + port: XAI_CALLBACK_PORT, + host: XAI_CALLBACK_HOST, + path: XAI_CALLBACK_PATH, + }, + expectedState, + copy, + ); } diff --git a/src/auth/xai/login.ts b/src/auth/xai/login.ts index 7659f103f..8a2e0dce7 100644 --- a/src/auth/xai/login.ts +++ b/src/auth/xai/login.ts @@ -1,14 +1,13 @@ -import { - buildAuthorizeUrl, - startOAuthLogin, - type OAuthLoginHandle, - type StartOAuthLoginOptions, -} from "@corbits/oauth-core"; import { exchangeXaiCode, xaiOAuthConfig, type XaiTokens, } from "@corbits/xai-provider"; +import type { + OAuthLoginHandle, + StartOAuthLoginOptions, +} from "@corbits/oauth-core"; +import { startProviderLogin } from "../provider-login.js"; import type { CallbackPageCopy } from "../callback-page.js"; import { saveXaiProfile } from "../../config/oauth-stores.js"; @@ -24,10 +23,9 @@ export async function startXaiLogin( opts: StartXaiLoginOptions, ): Promise { const { home, copy, ...loginOpts } = opts; - return startOAuthLogin(loginOpts, { + return startProviderLogin(loginOpts, { + oauthConfig: xaiOAuthConfig, startCallbackServer: (state) => startXaiCallbackServer(state, copy), - buildAuthorizeUrl: (pkce, state) => - buildAuthorizeUrl(xaiOAuthConfig, pkce, state), exchangeCode: exchangeXaiCode, saveProfile: (profile) => saveXaiProfile(profile, home), }); diff --git a/src/auth/xai/session.ts b/src/auth/xai/session.ts index f18cc2676..d602e3279 100644 --- a/src/auth/xai/session.ts +++ b/src/auth/xai/session.ts @@ -1,7 +1,4 @@ import { - createTokenSession, - isTokenExpired, - OAuthProfileNotFoundError, OAuthRefreshFailedError, type TokenSession, } from "@corbits/oauth-core"; @@ -13,9 +10,9 @@ import { import { loadXaiProfile, updateXaiTokens } from "../../config/oauth-stores.js"; import { - replaceMutableTokens, - sanitizedRefreshFailure, -} from "../token-session-boundary.js"; + createProviderTokenSession, + type TokenSessionCallContext, +} from "../provider-token-session.js"; export class XaiAuthError extends Error { readonly profile: string; @@ -37,89 +34,63 @@ export interface XaiAccess { access: string; } -function wrapXaiAuthError(name: string, err: unknown): never { - if (err instanceof OAuthProfileNotFoundError) { - throw new XaiAuthError( - name, - "missing", - `xAI profile "${name}" is not authorized. Log in again.`, - ); - } - if (err instanceof OAuthRefreshFailedError) { - const cause = err.cause; - throw new XaiAuthError( - name, - "refresh-failed", - `xAI profile "${name}" could not be refreshed (${cause instanceof Error ? cause.message : String(cause)}). Log in again.`, - ); +export const xaiTokensSession = createProviderTokenSession< + XaiTokens, + XaiAccess +>({ + errorClass: XaiAuthError, + providerLabel: "xAI", + skewMs: XAI_REFRESH_SKEW_MS, + loadProfile: loadXaiProfile, + updateTokens: updateXaiTokens, + toAccess: (tokens) => ({ access: tokens.access }), + refreshTokens: (refreshToken, now) => refreshXaiTokens(refreshToken, now), + // No inter-process lock here: xAI never rotates refresh tokens, so + // overlapping grants cannot revoke each other. What IS needed is the + // committed-winner recovery: a lost invalid_grant race must return the + // rotation another session committed instead of a re-login error. + serializeRefresh: (ctx) => recoverCommittedXaiWinner(ctx), +}); + +// On a shared credential two independent sessions may refresh at once; the +// loser's grant is rejected with invalid_grant. If a newer committed winner +// exists on disk by the time we fail, that is the shared credential's fresh +// access token — return it instead of a re-login error. +async function recoverCommittedXaiWinner({ + home, + name, + now, + getValid, + session, +}: TokenSessionCallContext): Promise { + const basis = await loadXaiProfile(name, home); + try { + return await getValid(); + } catch (error) { + if (error instanceof OAuthRefreshFailedError && basis !== undefined) { + const winner = await loadXaiProfile(name, home); + if ( + winner !== undefined && + !session.isExpired(winner.tokens, now) && + // A committed rotation always changes the access token or its + // lifetime; an unchanged profile is not a winner to recover. + (winner.tokens.access !== basis.tokens.access || + winner.tokens.expiresAt !== basis.tokens.expiresAt) + ) + return { access: winner.tokens.access }; + } + throw error; } - throw err; } -const sessions = new Map>(); - export function createXaiTokenSession( home?: string, ): TokenSession { - const refreshBasis = new WeakMap(); - const inner = createTokenSession({ - skewMs: XAI_REFRESH_SKEW_MS, - loadProfile: (name) => loadXaiProfile(name, home), - updateTokens: async (name, tokens) => { - const winner = await updateXaiTokens( - name, - tokens, - home, - refreshBasis.get(tokens), - ); - if (winner === undefined) throw new OAuthProfileNotFoundError(name); - replaceMutableTokens(tokens, winner.tokens); - }, - refreshTokens: async (refreshToken, now) => { - try { - const refreshed = await refreshXaiTokens(refreshToken, now); - refreshBasis.set(refreshed, refreshToken); - return refreshed; - } catch (error) { - throw sanitizedRefreshFailure(error, refreshToken); - } - }, - toAccess: (tokens) => ({ access: tokens.access }), - }); - return { - isExpired: inner.isExpired, - getValidToken: async (name, now = Date.now()) => { - const basis = await loadXaiProfile(name, home); - try { - return await inner.getValidToken(name, now); - } catch (error) { - if (error instanceof OAuthRefreshFailedError && basis !== undefined) { - const winner = await loadXaiProfile(name, home); - if ( - winner !== undefined && - !inner.isExpired(winner.tokens, now) && - (winner.tokens.access !== basis.tokens.access || - winner.tokens.expiresAt !== basis.tokens.expiresAt) - ) - return { access: winner.tokens.access }; - } - throw error; - } - }, - }; -} - -function sessionFor(home?: string): TokenSession { - const key = home ?? ""; - const existing = sessions.get(key); - if (existing !== undefined) return existing; - const created = createXaiTokenSession(home); - sessions.set(key, created); - return created; + return xaiTokensSession.createSession(home); } export function isXaiTokenExpired(tokens: XaiTokens, now: number): boolean { - return isTokenExpired(tokens, now, XAI_REFRESH_SKEW_MS); + return xaiTokensSession.isExpired(tokens, now); } export async function getValidXaiToken( @@ -127,24 +98,12 @@ export async function getValidXaiToken( now?: number, home?: string, ): Promise { - try { - return await sessionFor(home).getValidToken(name, now); - } catch (err) { - wrapXaiAuthError(name, err); - } + return xaiTokensSession.getValidToken(name, now, home); } export async function refreshStagedXaiTokens( tokens: XaiTokens, now: number = Date.now(), ): Promise { - if (!isXaiTokenExpired(tokens, now)) return tokens; - let refreshed: XaiTokens; - try { - refreshed = await refreshXaiTokens(tokens.refresh, now); - } catch (error) { - throw sanitizedRefreshFailure(error, tokens.refresh); - } - replaceMutableTokens(tokens, refreshed); - return tokens; + return xaiTokensSession.refreshStaged(tokens, now); } diff --git a/src/auth/xai/store.ts b/src/auth/xai/store.ts index e7c8b1053..c76baf9d6 100644 --- a/src/auth/xai/store.ts +++ b/src/auth/xai/store.ts @@ -1,30 +1,21 @@ +import { type } from "arktype"; import type { AuthProfile } from "@corbits/oauth-core"; import type { XaiTokens } from "@corbits/xai-provider"; -import { type } from "arktype"; -import { createAuthStore } from "../store.js"; +import { defineProviderAuthStore } from "../provider-auth-store.js"; export type { XaiTokens }; export type XaiProfile = AuthProfile; -const XaiTokensShape = type({ - access: "string", - refresh: "string", - expiresAt: "number", - "idToken?": "string", -}); - -function isXaiTokens(value: unknown): value is XaiTokens { - return !(XaiTokensShape(value) instanceof type.errors); -} - export const XAI_AUTH_FILENAME = "xai-auth.json"; -export function createXaiAuthStore(settingsDirName: string) { - return createAuthStore({ - filename: XAI_AUTH_FILENAME, - settingsDirName, - isTokens: isXaiTokens, - }); -} +export const createXaiAuthStore = defineProviderAuthStore({ + filename: XAI_AUTH_FILENAME, + tokensShape: type({ + access: "string", + refresh: "string", + expiresAt: "number", + "idToken?": "string", + }), +}); From 419282937cd5199206fdd67e340ccfeb512e7549 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Thu, 15 Oct 2026 14:13:41 -0700 Subject: [PATCH 2/2] refactor(auth): drop duplicate extraction tests and narration The shared lock and session helpers stay. Remove the second copy of cases already pinned by the store and refresh-lock suites, unused file-lock exports, and comments that only restated the extraction. --- src/auth/bearer-headers.ts | 2 - src/auth/codex/login.ts | 1 - src/auth/codex/refresh-lock.test.ts | 37 +--- src/auth/codex/refresh-lock.ts | 4 +- src/auth/file-lock.test.ts | 221 ------------------------ src/auth/file-lock.ts | 25 +-- src/auth/oauth-scope-check.ts | 3 - src/auth/provider-auth-store.ts | 2 - src/auth/provider-callback-server.ts | 2 - src/auth/provider-login.ts | 2 - src/auth/provider-token-session.test.ts | 186 ++------------------ src/auth/provider-token-session.ts | 14 -- src/auth/store.ts | 3 - 13 files changed, 26 insertions(+), 476 deletions(-) delete mode 100644 src/auth/file-lock.test.ts diff --git a/src/auth/bearer-headers.ts b/src/auth/bearer-headers.ts index 9c8d7f7c3..9cda856aa 100644 --- a/src/auth/bearer-headers.ts +++ b/src/auth/bearer-headers.ts @@ -2,8 +2,6 @@ export interface BearerToken { readonly access: string; } -// The one header every provider carries on an inference request; provider -// extras (account ids, client identifiers) ride alongside via the spread. export function bearerHeaders( token: BearerToken, extra: Record = {}, diff --git a/src/auth/codex/login.ts b/src/auth/codex/login.ts index 9c934f3c7..fb0635e4b 100644 --- a/src/auth/codex/login.ts +++ b/src/auth/codex/login.ts @@ -27,7 +27,6 @@ export async function startCodexLogin( return startProviderLogin(loginOpts, { oauthConfig: codexOAuthConfig, startCallbackServer: (state) => startCodexCallbackServer(state, copy), - // The mapper leaves expiresAt unset when the response omits expires_in. exchangeCode: async (code, verifier, now) => withDefaultCodexExpiry(await exchangeCodexCode(code, verifier, now), now), saveProfile: (profile) => saveCodexProfile(profile, home), diff --git a/src/auth/codex/refresh-lock.test.ts b/src/auth/codex/refresh-lock.test.ts index e2359e14e..d9b440e0d 100644 --- a/src/auth/codex/refresh-lock.test.ts +++ b/src/auth/codex/refresh-lock.test.ts @@ -13,12 +13,10 @@ import { CodexRefreshLockTimeoutError } from "./refresh-lock.js"; import { withMockedModule } from "../../../testkit/mock-module.js"; -// Non-ENOENT lock-file failures must surface raw instead of being swallowed -// (the pre-unification codex lock caught every fs error). Flags gate each -// failure to its own test; undefined keeps every call on the real module. +// Non-ENOENT read and stat failures must surface raw instead of being swallowed. +// Flags gate each failure to its own test; undefined keeps every call on the real module. let failingReadPath: string | undefined; let failingStatPath: string | undefined; -let failingUnlinkPath: string | undefined; await withMockedModule( import.meta.resolve("node:fs/promises"), (real: typeof import("node:fs/promises")) => ({ @@ -31,10 +29,6 @@ await withMockedModule( if (args[0] === failingStatPath) throw new Error("lock stat failed"); return real.stat(...args); }, - unlink: async (...args: Parameters) => { - if (args[0] === failingUnlinkPath) throw new Error("lock release failed"); - return real.unlink(...args); - }, }), ); @@ -286,31 +280,4 @@ describe("codex refresh lock", () => { await rm(dir, { recursive: true, force: true }); } }); - - test("a non-ENOENT release failure surfaces after the work value", async () => { - const dir = await tempDir(); - try { - const lock = join(dir, "refresh.lock"); - failingUnlinkPath = lock; - try { - // The refresh succeeded, but a release that fails for a real reason - // (not a steal's ENOENT) must not vanish: the caller has to know the - // lock file is still on disk. - await expect( - withCodexRefreshLock(lock, async () => "done", { - timeoutMs: 1_000, - retryMs: 10, - }), - ).rejects.toThrow("lock release failed"); - // The failed release leaves the claim file in place for recovery. - await expect(readFile(lock, "utf8")).resolves.toContain( - String(process.pid), - ); - } finally { - failingUnlinkPath = undefined; - } - } finally { - await rm(dir, { recursive: true, force: true }); - } - }); }); diff --git a/src/auth/codex/refresh-lock.ts b/src/auth/codex/refresh-lock.ts index 5bfdbb92e..99dc0ee4d 100644 --- a/src/auth/codex/refresh-lock.ts +++ b/src/auth/codex/refresh-lock.ts @@ -14,9 +14,7 @@ import { withFileLock } from "../file-lock.js"; // Stale horizon for legacy locks that carry no holder PID (foreign writers, // pre-tagging locks). Kept far above any legitimate hold and contender -// timeout so a live holder is never declared stale mid-wait. The provider -// auth store's own lock horizon is deliberately smaller (5s, short auth-file -// mutations); these two horizons are pinned invariants and must not unify. +// timeout so a live holder is never declared stale mid-wait. const DEFAULT_STALE_MS = 120_000; export interface CodexRefreshLockOptions { diff --git a/src/auth/file-lock.test.ts b/src/auth/file-lock.test.ts deleted file mode 100644 index 47cc8e0a5..000000000 --- a/src/auth/file-lock.test.ts +++ /dev/null @@ -1,221 +0,0 @@ -import { - mkdtemp, - readFile, - rm, - stat, - utimes, - writeFile, -} from "node:fs/promises"; -import { tmpdir } from "node:os"; -import { join } from "node:path"; -import { describe, expect, test } from "bun:test"; - -import { withMockedModule } from "../../testkit/mock-module.js"; - -// File-lock level pins: every credential lock (the auth store's and the -// codex refresh lock's) routes through this primitive, so its contract is -// what the two consumers' horizons and release semantics lean on. Flags gate -// each injected failure to its own test; undefined keeps every call on the -// real module. -let failingReadPath: string | undefined; -let failingUnlinkPath: string | undefined; -await withMockedModule( - import.meta.resolve("node:fs/promises"), - (real: typeof import("node:fs/promises")) => ({ - ...real, - readFile: async (...args: Parameters) => { - if (args[0] === failingReadPath) throw new Error("lock read failed"); - return real.readFile(...args); - }, - unlink: async (...args: Parameters) => { - if (args[0] === failingUnlinkPath) throw new Error("lock release failed"); - return real.unlink(...args); - }, - }), -); - -const { withFileLock } = await import("./file-lock.js"); - -async function tempDir(): Promise { - return mkdtemp(join(tmpdir(), "file-lock-")); -} - -const staleMs = 1_000; - -// The store and the refresh lock carry their own failure shapes; the shared -// primitive only delegates to the injected factory. -const timeoutError = (lockPath: string, _timeoutMs: number): Error => - new Error(`timed out for ${lockPath}`); - -describe("withFileLock", () => { - test("serializes concurrent holders and removes the lock file", async () => { - const dir = await tempDir(); - try { - const lock = join(dir, "x.lock"); - let active = 0; - let maxActive = 0; - const results = await Promise.all( - [0, 1, 2, 3, 4].map((i) => - withFileLock( - lock, - async () => { - active += 1; - maxActive = Math.max(maxActive, active); - // The file exists while held so a second caller contends on it. - await stat(lock); - await new Promise((resolve) => setTimeout(resolve, 10)); - active -= 1; - return i; - }, - { timeoutMs: 5_000, retryMs: 10, staleMs, timeoutError }, - ), - ), - ); - expect(results).toEqual([0, 1, 2, 3, 4]); - expect(maxActive).toBe(1); - await expect(stat(lock)).rejects.toThrow(); - } finally { - await rm(dir, { recursive: true, force: true }); - } - }); - - test("reports the injected timeout error on a foreign-held lock", async () => { - const dir = await tempDir(); - try { - const lock = join(dir, "x.lock"); - await writeFile(lock, ""); - await expect( - withFileLock(lock, async () => "never", { - timeoutMs: 100, - retryMs: 10, - staleMs, - timeoutError, - }), - ).rejects.toThrow(`timed out for ${lock}`); - // A timeout never touches the holder's file. - expect(await readFile(lock, "utf8")).toBe(""); - } finally { - await rm(dir, { recursive: true, force: true }); - } - }); - - test("takes over a stale untagged lock with the configured staleMs", async () => { - const dir = await tempDir(); - try { - const lock = join(dir, "x.lock"); - await writeFile(lock, "legacy-orphan"); - const ancient = new Date(Date.now() - 60_000); - await utimes(lock, ancient, ancient); - // The stale horizon is a caller decision: the auth store uses a much - // smaller value than the refresh lock, and neither is baked in here. - expect( - await withFileLock(lock, async () => "taken-over", { - timeoutMs: 5_000, - retryMs: 10, - staleMs, - timeoutError, - }), - ).toBe("taken-over"); - await expect(stat(lock)).rejects.toThrow(); - } finally { - await rm(dir, { recursive: true, force: true }); - } - }); - - test("takes over a dead PID holder regardless of the stale horizon", async () => { - const dir = await tempDir(); - try { - const lock = join(dir, "x.lock"); - // Dead holder in real claim format: liveness, not age, decides. - const exited = Bun.spawn(["bun", "--version"], { - stdout: "ignore", - stderr: "ignore", - }); - await exited.exited; - await writeFile(lock, `${String(exited.pid)}:crashed`); - expect( - await withFileLock(lock, async () => "recovered", { - timeoutMs: 5_000, - retryMs: 10, - staleMs: 120_000, - timeoutError, - }), - ).toBe("recovered"); - await expect(stat(lock)).rejects.toThrow(); - } finally { - await rm(dir, { recursive: true, force: true }); - } - }); - - test("fenced release keeps a takeover holder's file in place", async () => { - const dir = await tempDir(); - try { - const lock = join(dir, "x.lock"); - await withFileLock( - lock, - async () => { - // Simulate a stale-takeover steal landing mid-hold: the victim's - // release must leave the new holder's file alone. - await writeFile(lock, "42424242:takeover-holder"); - }, - { timeoutMs: 5_000, retryMs: 10, staleMs, timeoutError }, - ); - expect(await readFile(lock, "utf8")).toBe("42424242:takeover-holder"); - } finally { - await rm(dir, { recursive: true, force: true }); - } - }); - - test("propagates a non-ENOENT read failure during acquisition", async () => { - const dir = await tempDir(); - try { - const lock = join(dir, "x.lock"); - await writeFile(lock, ""); - failingReadPath = lock; - try { - // Both consumers inherit this off withFileLock: a permission or disk - // error while reading holder content must surface, not read as "no - // lock" and restart a blind takeover. - await expect( - withFileLock(lock, async () => "never", { - timeoutMs: 1_000, - retryMs: 10, - staleMs, - timeoutError, - }), - ).rejects.toThrow("lock read failed"); - } finally { - failingReadPath = undefined; - } - } finally { - await rm(dir, { recursive: true, force: true }); - } - }); - - test("surfaces a non-ENOENT release failure instead of swallowing it", async () => { - const dir = await tempDir(); - try { - const lock = join(dir, "x.lock"); - failingUnlinkPath = lock; - try { - // The work value is discarded in favor of the release outcome; with - // ENOENT-only tolerance, anything else means the lock file survived. - await expect( - withFileLock(lock, async () => "done", { - timeoutMs: 1_000, - retryMs: 10, - staleMs, - timeoutError, - }), - ).rejects.toThrow("lock release failed"); - await expect(readFile(lock, "utf8")).resolves.toContain( - String(process.pid), - ); - } finally { - failingUnlinkPath = undefined; - } - } finally { - await rm(dir, { recursive: true, force: true }); - } - }); -}); diff --git a/src/auth/file-lock.ts b/src/auth/file-lock.ts index d1863789a..376f0fa0e 100644 --- a/src/auth/file-lock.ts +++ b/src/auth/file-lock.ts @@ -9,14 +9,6 @@ import { import { dirname } from "node:path"; import { setTimeout as delay } from "node:timers/promises"; -// File-lock primitives shared by every credential lock in this repo. The -// invariants that make them safe: per-waiter unique claims name the holder, -// kill(pid, 0) liveness takes over a crashed holder's lock, an untagged lock -// older than the stale horizon is presumed orphaned, a steal re-reads before -// unlinking so a concurrent winner's fresh lock is never removed, and both a -// post-create content check and a fenced release keep a loser from deleting -// or releasing under a live winner. - export function isErrnoCode(err: unknown, code: string): boolean { return ( typeof err === "object" && @@ -26,7 +18,7 @@ export function isErrnoCode(err: unknown, code: string): boolean { ); } -export function isEnoent(err: unknown): boolean { +function isEnoent(err: unknown): boolean { return isErrnoCode(err, "ENOENT"); } @@ -34,11 +26,11 @@ export function isEnoent(err: unknown): boolean { // so the counter leg tells our claim from a concurrent winner's. let lockClaimCounter = 0; -export function newLockClaim(): string { +function newLockClaim(): string { return `${String(process.pid)}:${String((lockClaimCounter += 1))}`; } -export function holderPid(content: string): number | null { +function holderPid(content: string): number | null { const pid = Number(content.split(":")[0]?.trim()); return Number.isInteger(pid) && pid > 0 ? pid : null; } @@ -46,7 +38,7 @@ export function holderPid(content: string): number | null { // kill(pid, 0) liveness: success or EPERM means alive; ESRCH/EINVAL mean // dead. Any other failure reads as alive — never steal a live holder's lock // on a confused signal check; the waiter times out with a recovery hint. -export function isPidAlive(pid: number): boolean { +function isPidAlive(pid: number): boolean { try { process.kill(pid, 0); return true; @@ -57,9 +49,7 @@ export function isPidAlive(pid: number): boolean { // Null when the lock vanished under the waiter (a release raced the read); // anything but ENOENT propagates — permission and disk errors must surface. -export async function readLockContent( - lockPath: string, -): Promise { +async function readLockContent(lockPath: string): Promise { try { return await readFile(lockPath, "utf8"); } catch (err) { @@ -71,7 +61,7 @@ export async function readLockContent( // True when the lock is safe to take over: a tagged holder with a dead PID // (crashed), or a legacy untagged lock older than the stale horizon. A // vanished lock reads as not stale; the acquire loop retries the create. -export async function isLockStale( +async function isLockStale( lockPath: string, staleMs: number, content: string, @@ -114,9 +104,6 @@ export interface FileLockOptions { timeoutError: FileLockTimeoutError; } -// Credential locks in this repo serialize on exactly this acquire/release -// pairing: exclusive-create with a holder tag, stale takeover, and a fenced -// claim-matched release. Do not fork a variant that drops any of the three. export async function withFileLock( lockPath: string, work: () => Promise, diff --git a/src/auth/oauth-scope-check.ts b/src/auth/oauth-scope-check.ts index b20a8e166..4c5690150 100644 --- a/src/auth/oauth-scope-check.ts +++ b/src/auth/oauth-scope-check.ts @@ -106,9 +106,6 @@ function classifyStatus( return unavailable(providerLabel, `provider HTTP ${status}`); } -// One probe shape per provider: refresh staged tokens into the request -// headers, hit the provider's catalog/list endpoint, and classify the -// response. Only the headers, endpoint, and timeout differ. async function probeOAuthScope( providerLabel: string, authorizeHeaders: () => Promise>, diff --git a/src/auth/provider-auth-store.ts b/src/auth/provider-auth-store.ts index ab2978b86..8f2657ec2 100644 --- a/src/auth/provider-auth-store.ts +++ b/src/auth/provider-auth-store.ts @@ -8,8 +8,6 @@ export interface ProviderAuthStoreDefinition { tokensShape: Type; } -// The per-provider auth store definition: one filename plus the arktype -// shape guarding what may load from disk; nothing else differs. export function defineProviderAuthStore( definition: ProviderAuthStoreDefinition, ): (settingsDirName: string) => AuthStore { diff --git a/src/auth/provider-callback-server.ts b/src/auth/provider-callback-server.ts index eef06328c..c23d35a68 100644 --- a/src/auth/provider-callback-server.ts +++ b/src/auth/provider-callback-server.ts @@ -14,8 +14,6 @@ export interface ProviderCallbackServerConfig { path: string; } -// One loopback OAuth redirect server per provider: identical plumbing, with -// only the provider's callback target and its name in the callback pages. export async function startProviderCallbackServer( providerLabel: string, config: ProviderCallbackServerConfig, diff --git a/src/auth/provider-login.ts b/src/auth/provider-login.ts index 4162c801d..3999ee681 100644 --- a/src/auth/provider-login.ts +++ b/src/auth/provider-login.ts @@ -23,8 +23,6 @@ export interface ProviderLoginSpec { }) => Promise; } -// The shared PKCE login skeleton: every provider supplies its OAuth config, -// exchange, and persistence while the loopback flow stays in one place. export function startProviderLogin( opts: StartOAuthLoginOptions, spec: ProviderLoginSpec, diff --git a/src/auth/provider-token-session.test.ts b/src/auth/provider-token-session.test.ts index 0d3865067..1d4799cf2 100644 --- a/src/auth/provider-token-session.test.ts +++ b/src/auth/provider-token-session.test.ts @@ -7,8 +7,6 @@ type TestTokens = { expiresAt: number; }; -type TestAccess = { access: string }; - class TestAuthError extends Error { readonly profile: string; readonly reason: "missing" | "refresh-failed"; @@ -25,181 +23,31 @@ class TestAuthError extends Error { } } -type Profile = { name: string; createdAt: number; tokens: TestTokens }; - -function buildSpec(overrides?: { - refreshOutcome?: TestTokens | Error; - mergeRefreshed?: boolean; -}) { - // One in-memory profile store behind the same shape the disk store has, - // including the expectedRefreshToken CAS the session feeds it. - const store = new Map([ - [ - "p", - { - name: "p", - createdAt: 0, - tokens: { access: "old", refresh: "r1", expiresAt: 1_000 }, - }, - ], - ]); - const refreshCalls: { - refreshToken: string; - now: number; - previousAccess: string | undefined; - previousRefresh: string | undefined; - }[] = []; - const spec = { - errorClass: TestAuthError, - providerLabel: "Test", - skewMs: 0, - loadProfile: async (name: string) => store.get(name), - updateTokens: async ( - name: string, - tokens: TestTokens, - home: string | undefined, - expectedRefreshToken: string | undefined, - ) => { - if (name !== "p" || home !== undefined) return undefined; - const existing = store.get("p"); - if (existing === undefined) return undefined; - if ( - expectedRefreshToken !== undefined && - existing.tokens.refresh !== expectedRefreshToken - ) - return existing; - const updated = { ...existing, tokens }; - store.set("p", updated); - return updated; - }, - toAccess: (tokens: TestTokens) => ({ access: tokens.access }), - refreshTokens: async ( - refreshToken: string, - now: number, - previous?: TestTokens, - ) => { - // Capture the staged prior tokens at call time: replaceMutableTokens - // mutates the passed object in place after this hook returns. - refreshCalls.push({ - refreshToken, - now, - previousAccess: previous?.access, - previousRefresh: previous?.refresh, - }); - const outcome = overrides?.refreshOutcome; - if (outcome instanceof Error) throw outcome; - return ( - outcome ?? { access: "fresh", refresh: "r2", expiresAt: now + 10_000 } - ); - }, - }; - const api = createProviderTokenSession({ - ...spec, - ...(overrides?.mergeRefreshed - ? { - mergeRefreshed: (refreshed: TestTokens, previous: TestTokens) => ({ - ...refreshed, - access: `${refreshed.access}+${previous.refresh}`, - }), - } - : {}), - }); - return { api, store, refreshCalls }; -} - describe("createProviderTokenSession", () => { - test("refreshes an expired profile through the provider spec", async () => { - const { api, store } = buildSpec(); - expect(await api.getValidToken("p", 5_000)).toEqual({ access: "fresh" }); - expect(store.get("p")?.tokens).toEqual({ - access: "fresh", - refresh: "r2", - expiresAt: 15_000, - }); - expect(store.get("p")?.createdAt).toBe(0); - }); - - test("a concurrent store winner is never clobbered by the CAS basis", async () => { - // Another session rotates the store to refresh "r3" while this one's - // grant (presented r1) is in flight: the loser observes the committed - // winner instead of overwriting it. - const { api, store } = buildSpec(); - const first = api.getValidToken("p", 5_000); - store.set("p", { - name: "p", - createdAt: 0, - tokens: { access: "winner", refresh: "r3", expiresAt: 15_000 }, - }); - expect(await first).toEqual({ access: "winner" }); - expect(store.get("p")?.tokens.access).toBe("winner"); - }); - - test("the per-provider merge hook runs before the store write", async () => { - const { api, store } = buildSpec({ - mergeRefreshed: true, - refreshOutcome: { access: "fresh", refresh: "r2", expiresAt: 15_000 }, - }); - await api.getValidToken("p", 5_000); - expect(store.get("p")?.tokens).toEqual({ - access: "fresh+r1", - refresh: "r2", - expiresAt: 15_000, + test("staged refresh hands the prior tokens to the provider hook", async () => { + const seen: { + refreshToken: string; + previousAccess: string | undefined; + }[] = []; + const api = createProviderTokenSession({ + errorClass: TestAuthError, + providerLabel: "Test", + skewMs: 0, + loadProfile: async () => undefined, + updateTokens: async () => undefined, + toAccess: (tokens) => ({ access: tokens.access }), + refreshTokens: async (refreshToken, now, previous) => { + seen.push({ refreshToken, previousAccess: previous?.access }); + return { access: "fresh", refresh: "r2", expiresAt: now + 10_000 }; + }, }); - }); - test("staged refresh hands the prior tokens to the provider hook", async () => { - const { api, refreshCalls } = buildSpec(); expect( await api.refreshStaged( { access: "old2", refresh: "r1", expiresAt: 1_000 }, 5_000, ), ).toEqual({ access: "fresh", refresh: "r2", expiresAt: 15_000 }); - expect(refreshCalls.length).toBe(1); - expect(refreshCalls[0]?.refreshToken).toBe("r1"); - expect(refreshCalls[0]?.previousAccess).toBe("old2"); - }); - - test("wraps core failures into the provider error with its label", async () => { - const { api } = buildSpec(); - const missing = await api - .getValidToken("ghost", 5_000) - .catch((err: unknown) => err); - expect(missing).toBeInstanceOf(TestAuthError); - expect(missing).toMatchObject({ profile: "ghost", reason: "missing" }); - expect((missing as TestAuthError).message).toContain( - 'Test profile "ghost" is not authorized. Log in again.', - ); - - const refreshFailed = await buildSpec({ - refreshOutcome: new Error("grant rejected"), - }) - .api.getValidToken("p", 5_000) - .catch((err: unknown) => err); - expect(refreshFailed).toBeInstanceOf(TestAuthError); - expect(refreshFailed).toMatchObject({ - profile: "p", - reason: "refresh-failed", - }); - expect((refreshFailed as TestAuthError).message).toContain( - 'Test profile "p" could not be refreshed (grant rejected). Log in again.', - ); - }); - - test("absent serializeRefresh hook exposes the inner session unwrapped", async () => { - // The blocker contract: absent hook = no lock, no wrapping at all. - const { api } = buildSpec(); - const session = api.createSession(); - expect(Object.keys(session).sort()).toEqual(["getValidToken", "isExpired"]); - }); - - test("stale tokens within the skew read as expired", async () => { - const { api } = buildSpec(); - expect( - api.isExpired({ access: "a", refresh: "r", expiresAt: 5_000 }, 5_000), - ).toBe(true); - expect( - api.isExpired({ access: "a", refresh: "r", expiresAt: 6_000 }, 5_000), - ).toBe(false); + expect(seen).toEqual([{ refreshToken: "r1", previousAccess: "old2" }]); }); }); diff --git a/src/auth/provider-token-session.ts b/src/auth/provider-token-session.ts index 0764fc888..937a6ca8a 100644 --- a/src/auth/provider-token-session.ts +++ b/src/auth/provider-token-session.ts @@ -15,10 +15,6 @@ import { export type ProviderAuthErrorReason = "missing" | "refresh-failed"; -// The auth failure every provider surfaces: the profile name plus a reason -// ("missing" vs "refresh-failed") for re-login messaging. Classification is -// identity-based, so each provider keeps its own Error subclass and passes -// its constructor here. export type ProviderAuthError = Error & { readonly profile: string; readonly reason: ProviderAuthErrorReason; @@ -68,13 +64,7 @@ export interface ProviderTokenSessionSpec { now: number, previous?: TTokens, ) => Promise; - // Token-shape extensions stay per provider: codex keeps the chatgpt - // account id when a refresh response omits id_token. Absent = pass-through. mergeRefreshed?: (refreshed: TTokens, previous: TTokens) => TTokens; - // Post-wrap of getValidToken for every session over a credential home. - // Codex serializes inter-process refresh acquisitions behind a lock file; - // xAI recovers a committed concurrent winner after a lost invalid_grant - // race. Absent hook = no wrapping at all (no lock by design). serializeRefresh?: ( ctx: TokenSessionCallContext, ) => Promise; @@ -92,10 +82,6 @@ export interface ProviderTokenSessionApi { refreshStaged: (tokens: TTokens, now?: number) => Promise; } -// The provider token-session lifecycle, once: per-home session cache, the -// refresh-basis WeakMap feeding the store's credential CAS, sanitized -// endpoint failures, and the profile errors. Only the spec's provider hooks -// differ between providers (see ProviderTokenSessionSpec). export function createProviderTokenSession( spec: ProviderTokenSessionSpec, ): ProviderTokenSessionApi { diff --git a/src/auth/store.ts b/src/auth/store.ts index e020a6e8c..52c8e436a 100644 --- a/src/auth/store.ts +++ b/src/auth/store.ts @@ -49,9 +49,6 @@ const LOCK_TIMEOUT_MS = 1_000; // Age at which a lock file with no holder PID (a foreign writer, or one // predating PID tagging) is presumed orphaned and taken over. Far above // LOCK_TIMEOUT_MS so a waiter never declares a live holder stale mid-wait. -// Deliberately smaller than the codex refresh-lock horizon (120s, refresh -// grants are long); these two horizons are pinned invariants and must not -// unify. const LOCK_STALE_MS = 5_000; // Per-call unique temp (pid + counter); pid alone is not unique if