Skip to content

Commit 4840023

Browse files
ndycodendycode
authored
Merge pull request #339 from ndycode/audit/pr3-storage-path-correctness
preserve storage path context for deferred saves
2 parents e0516b2 + b9d7c2f commit 4840023

5 files changed

Lines changed: 244 additions & 6 deletions

File tree

‎lib/accounts.ts‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,11 @@ import {
3232
getRuntimeAccountIdentityKey,
3333
} from "./storage/identity.js";
3434
import { getCircuitBreaker } from "./circuit-breaker.js";
35+
import {
36+
getStoragePathState,
37+
runWithStoragePathState,
38+
type StoragePathState,
39+
} from "./storage/path-state.js";
3540

3641
export {
3742
extractAccountId,
@@ -261,6 +266,7 @@ export class AccountManager {
261266
private lastToastTime = 0;
262267
private saveDebounceTimer: ReturnType<typeof setTimeout> | null = null;
263268
private pendingSave: Promise<void> | null = null;
269+
private readonly storagePathState: StoragePathState;
264270

265271
static async loadFromDisk(authFallback?: OAuthAuthDetails): Promise<AccountManager> {
266272
const stored = await loadAccounts();
@@ -347,6 +353,7 @@ export class AccountManager {
347353
}
348354

349355
constructor(authFallback?: OAuthAuthDetails, stored?: AccountStorageV3 | null) {
356+
this.storagePathState = { ...getStoragePathState() };
350357
const fallbackAccountId = extractAccountId(authFallback?.access)?.trim() || undefined;
351358
const fallbackAccountEmail = sanitizeEmail(extractAccountEmail(authFallback?.access));
352359

@@ -1143,8 +1150,10 @@ export class AccountManager {
11431150
}
11441151

11451152
async saveToDisk(): Promise<void> {
1146-
await withAccountStorageTransaction(async (_current, persist) => {
1147-
await persist(this.buildStorageSnapshot());
1153+
await runWithStoragePathState(this.storagePathState, async () => {
1154+
await withAccountStorageTransaction(async (_current, persist) => {
1155+
await persist(this.buildStorageSnapshot());
1156+
});
11481157
});
11491158
}
11501159

‎lib/storage/path-state.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,3 +28,10 @@ export function setStoragePathState(state: StoragePathState): void {
2828
currentStorageState = state;
2929
storagePathStateContext.enterWith(state);
3030
}
31+
32+
export async function runWithStoragePathState<T>(
33+
state: StoragePathState,
34+
fn: () => T | Promise<T>,
35+
): Promise<T> {
36+
return await storagePathStateContext.run(state, fn);
37+
}

‎lib/storage/paths.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import { createHash } from "node:crypto";
88
import { basename, dirname, isAbsolute, join, relative, resolve, win32 } from "node:path";
99
import { homedir, tmpdir } from "node:os";
1010
import { getCodexMultiAuthDir } from "../runtime-paths.js";
11+
import { getStoragePathState } from "./path-state.js";
1112

1213
const PROJECT_MARKERS = [".git", "package.json", "Cargo.toml", "go.mod", "pyproject.toml", ".codex"];
1314
const PROJECTS_DIR = "projects";
@@ -345,11 +346,11 @@ export function resolvePath(filePath: string): string {
345346
}
346347

347348
const home = homedir();
348-
const cwd = process.cwd();
349+
const projectRoot = getStoragePathState().currentProjectRoot ?? process.cwd();
349350
const tmp = tmpdir();
350351
if (
351352
!isWithinDirectory(home, resolved) &&
352-
!isWithinDirectory(cwd, resolved) &&
353+
!isWithinDirectory(projectRoot, resolved) &&
353354
!isWithinDirectory(tmp, resolved)
354355
) {
355356
throw new Error(`Access denied: path must be within home directory, project directory, or temp directory`);

‎test/accounts.test.ts‎

Lines changed: 185 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,10 @@ import {
2222
getAccountIdentityKey,
2323
getRuntimeAccountIdentityKey,
2424
} from "../lib/storage/identity.js";
25+
import {
26+
getStoragePathState,
27+
setStoragePathState,
28+
} from "../lib/storage/path-state.js";
2529
import type { OAuthAuthDetails } from "../lib/types.js";
2630

2731
vi.mock("../lib/storage.js", async (importOriginal) => {
@@ -35,6 +39,12 @@ vi.mock("../lib/storage.js", async (importOriginal) => {
3539

3640
beforeEach(async () => {
3741
resetTrackers();
42+
setStoragePathState({
43+
currentStoragePath: null,
44+
currentLegacyProjectStoragePath: null,
45+
currentLegacyWorktreeStoragePath: null,
46+
currentProjectRoot: null,
47+
});
3848
const { saveAccounts, withAccountStorageTransaction } = await import(
3949
"../lib/storage.js"
4050
);
@@ -56,6 +66,15 @@ beforeEach(async () => {
5666
});
5767
});
5868

69+
afterEach(() => {
70+
setStoragePathState({
71+
currentStoragePath: null,
72+
currentLegacyProjectStoragePath: null,
73+
currentLegacyWorktreeStoragePath: null,
74+
currentProjectRoot: null,
75+
});
76+
});
77+
5978
describe("parseRateLimitReason", () => {
6079
it("returns quota for quota-related codes", () => {
6180
expect(parseRateLimitReason("usage_limit_reached")).toBe("quota");
@@ -1786,6 +1805,164 @@ describe("AccountManager", () => {
17861805

17871806
expect(mockSaveAccounts).toHaveBeenCalledTimes(2);
17881807
});
1808+
1809+
it("uses the manager's captured storage path state for delayed saves", async () => {
1810+
const { saveAccounts, withAccountStorageTransaction } = await import("../lib/storage.js");
1811+
const mockSaveAccounts = vi.mocked(saveAccounts);
1812+
const mockWithAccountStorageTransaction = vi.mocked(
1813+
withAccountStorageTransaction,
1814+
);
1815+
mockSaveAccounts.mockClear();
1816+
1817+
vi.useFakeTimers();
1818+
try {
1819+
const now = Date.now();
1820+
const stored = {
1821+
version: 3 as const,
1822+
activeIndex: 0,
1823+
accounts: [
1824+
{ refreshToken: "token-1", addedAt: now, lastUsed: now },
1825+
],
1826+
};
1827+
1828+
setStoragePathState({
1829+
currentStoragePath: "/repo-a/storage.json",
1830+
currentLegacyProjectStoragePath: null,
1831+
currentLegacyWorktreeStoragePath: null,
1832+
currentProjectRoot: "/repo-a",
1833+
});
1834+
const manager = new AccountManager(undefined, stored);
1835+
1836+
const seenStates: Array<ReturnType<typeof getStoragePathState>> = [];
1837+
mockWithAccountStorageTransaction.mockImplementationOnce(async (handler) => {
1838+
seenStates.push({ ...getStoragePathState() });
1839+
let current = null;
1840+
const persist = async (storage: Parameters<typeof saveAccounts>[0]) => {
1841+
current = structuredClone(storage);
1842+
await mockSaveAccounts(storage);
1843+
};
1844+
return handler(current as never, persist);
1845+
});
1846+
1847+
setStoragePathState({
1848+
currentStoragePath: "/repo-b/storage.json",
1849+
currentLegacyProjectStoragePath: null,
1850+
currentLegacyWorktreeStoragePath: null,
1851+
currentProjectRoot: "/repo-b",
1852+
});
1853+
1854+
manager.saveToDiskDebounced(50);
1855+
await vi.advanceTimersByTimeAsync(60);
1856+
1857+
expect(seenStates).toEqual([
1858+
expect.objectContaining({
1859+
currentStoragePath: "/repo-a/storage.json",
1860+
currentProjectRoot: "/repo-a",
1861+
}),
1862+
]);
1863+
expect(mockSaveAccounts).toHaveBeenCalledTimes(1);
1864+
} finally {
1865+
vi.useRealTimers();
1866+
}
1867+
});
1868+
1869+
it("keeps ambient storage path state stable during concurrent saves", async () => {
1870+
const { withAccountStorageTransaction } = await import("../lib/storage.js");
1871+
const mockWithAccountStorageTransaction = vi.mocked(
1872+
withAccountStorageTransaction,
1873+
);
1874+
const createDeferred = () => {
1875+
let resolve!: () => void;
1876+
const promise = new Promise<void>((resolvePromise) => {
1877+
resolve = resolvePromise;
1878+
});
1879+
return { promise, resolve };
1880+
};
1881+
const enteredResolvers = [createDeferred(), createDeferred()];
1882+
const releaseResolvers = [createDeferred(), createDeferred()];
1883+
const seenStates: Array<ReturnType<typeof getStoragePathState>> = [];
1884+
let callIndex = 0;
1885+
1886+
mockWithAccountStorageTransaction.mockImplementation(async (handler) => {
1887+
const currentCall = callIndex;
1888+
callIndex += 1;
1889+
seenStates.push({ ...getStoragePathState() });
1890+
enteredResolvers[currentCall]?.resolve();
1891+
if (currentCall === 0) {
1892+
await enteredResolvers[1]?.promise;
1893+
}
1894+
await releaseResolvers[currentCall]?.promise;
1895+
return handler(null, async () => undefined);
1896+
});
1897+
1898+
const now = Date.now();
1899+
const stored = {
1900+
version: 3 as const,
1901+
activeIndex: 0,
1902+
accounts: [{ refreshToken: "token-1", addedAt: now, lastUsed: now }],
1903+
};
1904+
1905+
setStoragePathState({
1906+
currentStoragePath: "/ambient/storage.json",
1907+
currentLegacyProjectStoragePath: null,
1908+
currentLegacyWorktreeStoragePath: null,
1909+
currentProjectRoot: "/ambient",
1910+
});
1911+
setStoragePathState({
1912+
currentStoragePath: "/repo-a/storage.json",
1913+
currentLegacyProjectStoragePath: null,
1914+
currentLegacyWorktreeStoragePath: null,
1915+
currentProjectRoot: "/repo-a",
1916+
});
1917+
const managerA = new AccountManager(undefined, stored);
1918+
setStoragePathState({
1919+
currentStoragePath: "/repo-b/storage.json",
1920+
currentLegacyProjectStoragePath: null,
1921+
currentLegacyWorktreeStoragePath: null,
1922+
currentProjectRoot: "/repo-b",
1923+
});
1924+
const managerB = new AccountManager(undefined, stored);
1925+
setStoragePathState({
1926+
currentStoragePath: "/ambient/storage.json",
1927+
currentLegacyProjectStoragePath: null,
1928+
currentLegacyWorktreeStoragePath: null,
1929+
currentProjectRoot: "/ambient",
1930+
});
1931+
1932+
const saveA = managerA.saveToDisk();
1933+
await enteredResolvers[0]?.promise;
1934+
const saveB = managerB.saveToDisk();
1935+
await enteredResolvers[1]?.promise;
1936+
1937+
expect(getStoragePathState()).toEqual(
1938+
expect.objectContaining({
1939+
currentStoragePath: "/ambient/storage.json",
1940+
currentProjectRoot: "/ambient",
1941+
}),
1942+
);
1943+
1944+
releaseResolvers[0]?.resolve();
1945+
await saveA;
1946+
releaseResolvers[1]?.resolve();
1947+
await saveB;
1948+
1949+
expect(seenStates).toEqual([
1950+
expect.objectContaining({
1951+
currentStoragePath: "/repo-a/storage.json",
1952+
currentProjectRoot: "/repo-a",
1953+
}),
1954+
expect.objectContaining({
1955+
currentStoragePath: "/repo-b/storage.json",
1956+
currentProjectRoot: "/repo-b",
1957+
}),
1958+
]);
1959+
expect(getStoragePathState()).toEqual(
1960+
expect.objectContaining({
1961+
currentStoragePath: "/ambient/storage.json",
1962+
currentProjectRoot: "/ambient",
1963+
}),
1964+
);
1965+
});
17891966
});
17901967

17911968
describe("constructor edge cases", () => {
@@ -2851,7 +3028,10 @@ describe("AccountManager", () => {
28513028
expect(getRuntimeTrackerKey(rotatedAccount)).toBe(trackerKey);
28523029
expect(getRuntimeAccountIdentityKey(rotatedAccount)).toBe(trackerKey);
28533030
expect(getAccountIdentityKey(rotatedAccount)).not.toBe(`${trackerKey}`);
2854-
expect(healthTracker.getScore(trackerKey, "codex:gpt-5.1")).toBe(degradedScore);
3031+
expect(healthTracker.getScore(trackerKey, "codex:gpt-5.1")).toBeCloseTo(
3032+
degradedScore,
3033+
6,
3034+
);
28553035
expect(tokenTracker.getTokens(trackerKey, "codex:gpt-5.1")).toBeLessThan(50);
28563036
});
28573037

@@ -2903,7 +3083,10 @@ describe("AccountManager", () => {
29033083
"account:account-enriched::email:enriched@example.com",
29043084
);
29053085
expect(getRuntimeTrackerKey(account)).toBe(trackerKey);
2906-
expect(healthTracker.getScore(trackerKey, "codex:gpt-5.1")).toBe(degradedScore);
3086+
expect(healthTracker.getScore(trackerKey, "codex:gpt-5.1")).toBeCloseTo(
3087+
degradedScore,
3088+
6,
3089+
);
29073090
expect(tokenTracker.getTokens(trackerKey, "codex:gpt-5.1")).toBeLessThan(50);
29083091
});
29093092

‎test/paths.test.ts‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import {
2323
findProjectRoot,
2424
resolvePath,
2525
} from "../lib/storage/paths.js";
26+
import { setStoragePathState } from "../lib/storage/path-state.js";
2627

2728
const mockedExistsSync = vi.mocked(existsSync);
2829
const mockedReadFileSync = vi.mocked(readFileSync);
@@ -53,12 +54,24 @@ describe("Storage Paths Module", () => {
5354
vi.clearAllMocks();
5455
mockedRealpathSync.mockImplementation((value) => String(value));
5556
mockedRealpathSyncNative.mockImplementation((value) => String(value));
57+
setStoragePathState({
58+
currentStoragePath: null,
59+
currentLegacyProjectStoragePath: null,
60+
currentLegacyWorktreeStoragePath: null,
61+
currentProjectRoot: null,
62+
});
5663
});
5764

5865
afterEach(() => {
5966
vi.resetAllMocks();
6067
if (_origCODEX_HOME !== undefined) process.env.CODEX_HOME = _origCODEX_HOME; else delete process.env.CODEX_HOME;
6168
if (_origCODEX_MULTI_AUTH_DIR !== undefined) process.env.CODEX_MULTI_AUTH_DIR = _origCODEX_MULTI_AUTH_DIR; else delete process.env.CODEX_MULTI_AUTH_DIR;
69+
setStoragePathState({
70+
currentStoragePath: null,
71+
currentLegacyProjectStoragePath: null,
72+
currentLegacyWorktreeStoragePath: null,
73+
currentProjectRoot: null,
74+
});
6275
});
6376

6477
describe("getConfigDir", () => {
@@ -785,6 +798,31 @@ describe("Storage Paths Module", () => {
785798
expect(() => resolvePath(tempPath)).not.toThrow();
786799
});
787800

801+
it("accepts paths within the storage state's project root even when cwd differs", () => {
802+
const cwd = process.cwd();
803+
const parent = path.dirname(cwd);
804+
const projectRoot = path.join(parent, `${path.basename(cwd)}-storage-root`);
805+
const projectPath = path.join(projectRoot, "subdir", "file.txt");
806+
const home = homedir();
807+
const tmp = tmpdir();
808+
if (
809+
projectPath.startsWith(home) ||
810+
projectPath.startsWith(tmp) ||
811+
projectPath.startsWith(cwd)
812+
) {
813+
return;
814+
}
815+
816+
setStoragePathState({
817+
currentStoragePath: path.join(projectRoot, "openai-codex-accounts.json"),
818+
currentLegacyProjectStoragePath: null,
819+
currentLegacyWorktreeStoragePath: null,
820+
currentProjectRoot: projectRoot,
821+
});
822+
823+
expect(() => resolvePath(projectPath)).not.toThrow();
824+
});
825+
788826
it("should throw for paths outside allowed directories", () => {
789827
const outsidePath = "/definitely/not/allowed/path";
790828

0 commit comments

Comments
 (0)