From ffe6f20ca00ff8ab413938191461068043a77e20 Mon Sep 17 00:00:00 2001 From: Volodymyr Vreshch Date: Sat, 15 Aug 2026 23:05:37 +0200 Subject: [PATCH] fix: never resurrect auth.json after sign-out (refresh race) --- src/commands/auth/setup.ts | 2 +- src/lib/auth/api.test.ts | 72 +++++++++++++++++++++++++++++++++++++- src/lib/auth/api.ts | 9 ++--- src/lib/fs/config.test.ts | 24 +++++++++++-- src/lib/fs/config.ts | 9 +++-- 5 files changed, 105 insertions(+), 11 deletions(-) diff --git a/src/commands/auth/setup.ts b/src/commands/auth/setup.ts index 4ce7f10..0572912 100644 --- a/src/commands/auth/setup.ts +++ b/src/commands/auth/setup.ts @@ -86,7 +86,7 @@ const disconnect = async (deps: SetupDeps): Promise => { return; } await deps.revoke(links(auth.siteFqdn).auth, auth.tokens.accessToken); - deleteAuth(); + await deleteAuth(); console.log('Disconnected - local credentials removed.'); }; diff --git a/src/lib/auth/api.test.ts b/src/lib/auth/api.test.ts index 5e0cbe8..9cfdfdd 100644 --- a/src/lib/auth/api.test.ts +++ b/src/lib/auth/api.test.ts @@ -11,7 +11,7 @@ import { TransientAuthError, } from './api.js'; import { patAuthState } from './credentials.js'; -import { readAuth, type AuthState } from '../fs/config.js'; +import { deleteAuth, readAuth, saveAuth, type AuthState } from '../fs/config.js'; import { links } from '../net/origins.js'; import { VERSION } from '../../utils/version.js'; @@ -79,6 +79,7 @@ describe('authedGet', () => { }); vi.stubGlobal('fetch', fetchMock); const auth = makeAuth(); + saveAuth(auth); // a real refresh always runs against a session already on disk const result = await authedGet<{ ok: boolean }>(auth, target, 'https://x.example/me'); expect(result.ok).toBe(true); expect(auth.tokens.accessToken).toBe('new-token'); @@ -164,6 +165,7 @@ describe('authedPost', () => { }); vi.stubGlobal('fetch', fetchMock); const auth = makeAuth(); + saveAuth(auth); const res = await authedPost(auth, target, 'https://x.example/api/memories', { name: 'a' }); expect(res.status).toBe(201); expect(auth.tokens.accessToken).toBe('new-token'); @@ -218,6 +220,7 @@ describe('introspectToken', () => { }); vi.stubGlobal('fetch', fetchMock); const auth = makeAuth(); + saveAuth(auth); const session = await introspectToken(auth, target); expect(session.userId).toBe('u1'); expect(auth.tokens.accessToken).toBe('new-token'); @@ -362,3 +365,70 @@ describe('PAT-backed AuthState', () => { expect(fetchMock).toHaveBeenCalledTimes(1); }); }); + +// Issue #236: a sign-out that lands while a token refresh is in flight must stay signed out. The +// refresh may still use its fresh tokens in memory for the request already on the wire, but the +// null re-read is the authoritative "session ended" - it is never folded back onto disk. +describe('refreshOrThrow vs a concurrent sign-out', () => { + let dir: string; + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'agentage-api-')); + process.env['AGENTAGE_CONFIG_DIR'] = dir; + }); + + afterEach(() => { + delete process.env['AGENTAGE_CONFIG_DIR']; + rmSync(dir, { recursive: true, force: true }); + vi.unstubAllGlobals(); + }); + + const refreshOnce = (onTokenCall: () => Promise): ReturnType => + vi.fn(async (url: string) => { + if (!String(url).includes('/token')) return jsonResponse(200, {}); + await onTokenCall(); // the sign-out lands while the refresh is on the wire + return jsonResponse(200, { access_token: 'new-token', expires_in: 60 }); + }); + + it('does not resurrect auth.json when deleteAuth lands during the refresh', async () => { + const auth = makeAuth(); + saveAuth(auth); + vi.stubGlobal( + 'fetch', + refreshOnce(() => deleteAuth()) + ); + await refreshOrThrow(auth, target); + expect(readAuth()).toBeNull(); // signed out stays signed out + expect(auth.tokens.accessToken).toBe('new-token'); // in-flight request still retries + }); + + it('stays signed out across 50 sign-out/refresh races', async () => { + for (let i = 0; i < 50; i++) { + const auth = makeAuth(); + saveAuth(auth); + let signOut: Promise | undefined; + vi.stubGlobal( + 'fetch', + refreshOnce(() => { + signOut = deleteAuth(); // raced, not awaited: either order must end signed out + return Promise.resolve(); + }) + ); + await refreshOrThrow(auth, target); + await signOut; + expect(readAuth()).toBeNull(); + } + }); + + it('still persists the refreshed tokens when no sign-out races it', async () => { + const auth = makeAuth(); + saveAuth(auth); + vi.stubGlobal( + 'fetch', + refreshOnce(() => Promise.resolve()) + ); + await refreshOrThrow(auth, target); + expect(readAuth()?.tokens.accessToken).toBe('new-token'); + expect(readAuth()?.clientId).toBe('c1'); // other fields preserved + }); +}); diff --git a/src/lib/auth/api.ts b/src/lib/auth/api.ts index 34d1c20..9a411a9 100644 --- a/src/lib/auth/api.ts +++ b/src/lib/auth/api.ts @@ -33,11 +33,12 @@ export const refreshOrThrow = async (auth: AuthState, links: Links): Promise { - const base = current ?? auth; - base.tokens = tokens; - return base; + if (!current) return null; + current.tokens = tokens; + return current; }); }; diff --git a/src/lib/fs/config.test.ts b/src/lib/fs/config.test.ts index 7595bd8..f71e6a4 100644 --- a/src/lib/fs/config.test.ts +++ b/src/lib/fs/config.test.ts @@ -2,6 +2,7 @@ import { mkdtempSync, readdirSync, readFileSync, rmSync, statSync, writeFileSync import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { acquireFileLock, releaseFileLock } from './file-lock.js'; import { deleteAuth, ensureConfigDir, @@ -57,10 +58,27 @@ describe('config store', () => { expect(readAuth()).toBeNull(); }); - it('deletes auth state idempotently', () => { + it('deletes auth state idempotently', async () => { saveAuth(sample); - deleteAuth(); - deleteAuth(); + await deleteAuth(); + await deleteAuth(); + expect(readAuth()).toBeNull(); + expect(readdirSync(getConfigDir()).filter((f) => f.endsWith('.lock'))).toEqual([]); + }); + + // Sign-out is serialized against mutateAuth by the same lock (issue #236): while a refresh holds + // it, deleteAuth waits instead of unlinking inside someone else's read-modify-write. + it('waits for a held lock before unlinking', async () => { + saveAuth(sample); + const target = join(getConfigDir(), 'auth.json'); + expect(acquireFileLock(target)).toBe(true); + let done = false; + const pending = deleteAuth().then(() => void (done = true)); + await new Promise((r) => setTimeout(r, 100)); + expect(done).toBe(false); + expect(readAuth()).toEqual(sample); // untouched while the other writer holds the lock + releaseFileLock(target); + await pending; expect(readAuth()).toBeNull(); }); diff --git a/src/lib/fs/config.ts b/src/lib/fs/config.ts index 121ab02..c8a67ba 100644 --- a/src/lib/fs/config.ts +++ b/src/lib/fs/config.ts @@ -61,9 +61,14 @@ export const saveAuth = (state: AuthState): void => { renameSync(tmp, path); }; -export const deleteAuth = (): void => { +// Sign-out under the same lock mutateAuth takes (issue #236), so a concurrent token refresh can +// never slip its re-read+save between this existence check and the unlink. +export const deleteAuth = async (): Promise => { + ensureConfigDir(); const path = authPath(); - if (existsSync(path)) unlinkSync(path); + await withFileLock(path, () => { + if (existsSync(path)) unlinkSync(path); + }); }; // Cross-process-safe read-modify-write on auth.json (issue #231). Under the advisory lock, re-read