From b61df181d349dbf98062e82814ccb0d8f8348ba8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=A8=8B=E5=BA=8F=E5=91=98=E9=98=BF=E6=B1=9F=28Relakkes?= =?UTF-8?q?=29?= Date: Mon, 18 May 2026 23:08:48 +0800 Subject: [PATCH] Prevent env-pinned OpenAI OAuth reads from resurrecting stale secure tokens When OPENAI_CODEX_OAUTH_FILE is set, the override file must stay the only source of truth. Reads now short-circuit to that file path, sync caching keys off the resolved override path, and failed atomic writes remove their tmp files before returning control. Constraint: OPENAI_CODEX_OAUTH_FILE must override shared secure storage even when the file is missing or corrupt Rejected: Keep zero-arg memoization and rely on manual cache clears | callers can switch override paths without touching the cache Confidence: high Scope-risk: narrow Directive: Do not reintroduce secure-storage fallback while the override env var is set Tested: bun test src/services/openaiAuth/storage.test.ts src/server/__tests__/haha-openai-oauth-service.test.ts Tested: bun test src/services/openaiAuth/storage.test.ts src/server/__tests__/haha-openai-oauth-service.test.ts src/server/__tests__/haha-openai-oauth-api.test.ts Tested: bun run check:server Tested: git diff --check --- src/services/openaiAuth/storage.test.ts | 134 +++++++++++++++++++++++- src/services/openaiAuth/storage.ts | 76 +++++++++----- 2 files changed, 185 insertions(+), 25 deletions(-) diff --git a/src/services/openaiAuth/storage.test.ts b/src/services/openaiAuth/storage.test.ts index fd080869..a03ee2e3 100644 --- a/src/services/openaiAuth/storage.test.ts +++ b/src/services/openaiAuth/storage.test.ts @@ -1,4 +1,4 @@ -import { afterEach, beforeEach, describe, expect, test } from 'bun:test' +import { afterEach, beforeEach, describe, expect, spyOn, test } from 'bun:test' import * as fs from 'fs' import * as fsp from 'fs/promises' import * as os from 'os' @@ -10,30 +10,67 @@ import { getOpenAIOAuthTokensAsync, saveOpenAIOAuthTokens, } from './storage.js' +import { plainTextStorage } from '../../utils/secureStorage/plainTextStorage.js' +import type { OpenAIOAuthTokens } from './types.js' describe('OpenAI OAuth desktop token file storage', () => { let tmpDir: string let tokenPath: string let originalTokenFile: string | undefined + let originalConfigDir: string | undefined + let originalHome: string | undefined + let originalUserProfile: string | undefined beforeEach(async () => { tmpDir = await fsp.mkdtemp(path.join(os.tmpdir(), 'openai-oauth-storage-')) tokenPath = path.join(tmpDir, 'openai-oauth.json') originalTokenFile = process.env.OPENAI_CODEX_OAUTH_FILE + originalConfigDir = process.env.CLAUDE_CONFIG_DIR + originalHome = process.env.HOME + originalUserProfile = process.env.USERPROFILE + process.env.CLAUDE_CONFIG_DIR = tmpDir + process.env.HOME = tmpDir + process.env.USERPROFILE = tmpDir process.env.OPENAI_CODEX_OAUTH_FILE = tokenPath clearOpenAIOAuthTokenCache() }) afterEach(async () => { + plainTextStorage.delete() if (originalTokenFile === undefined) { delete process.env.OPENAI_CODEX_OAUTH_FILE } else { process.env.OPENAI_CODEX_OAUTH_FILE = originalTokenFile } + if (originalConfigDir === undefined) { + delete process.env.CLAUDE_CONFIG_DIR + } else { + process.env.CLAUDE_CONFIG_DIR = originalConfigDir + } + if (originalHome === undefined) { + delete process.env.HOME + } else { + process.env.HOME = originalHome + } + if (originalUserProfile === undefined) { + delete process.env.USERPROFILE + } else { + process.env.USERPROFILE = originalUserProfile + } clearOpenAIOAuthTokenCache() await fsp.rm(tmpDir, { recursive: true, force: true }) }) + function seedSecureStorage(tokens: OpenAIOAuthTokens): void { + delete process.env.OPENAI_CODEX_OAUTH_FILE + clearOpenAIOAuthTokenCache() + expect( + plainTextStorage.update({ openaiCodexOauth: tokens }).success, + ).toBe(true) + process.env.OPENAI_CODEX_OAUTH_FILE = tokenPath + clearOpenAIOAuthTokenCache() + } + test('reads desktop token file synchronously', async () => { await fsp.writeFile( tokenPath, @@ -119,4 +156,99 @@ describe('OpenAI OAuth desktop token file storage', () => { expect(deleteOpenAIOAuthTokens()).toBe(true) expect(fs.existsSync(tokenPath)).toBe(false) }) + + test('returns null from both getters when env override is set but file is missing', async () => { + seedSecureStorage({ + accessToken: 'secure-access', + refreshToken: 'secure-refresh', + expiresAt: 4_100_000_000_000, + }) + + expect(getOpenAIOAuthTokens()).toBeNull() + await expect(getOpenAIOAuthTokensAsync()).resolves.toBeNull() + }) + + test('returns null from both getters when env override file contains corrupt json', async () => { + seedSecureStorage({ + accessToken: 'secure-access', + refreshToken: 'secure-refresh', + expiresAt: 4_100_000_000_000, + }) + await fsp.writeFile(tokenPath, '{ definitely-not-json', 'utf-8') + + expect(getOpenAIOAuthTokens()).toBeNull() + await expect(getOpenAIOAuthTokensAsync()).resolves.toBeNull() + }) + + test('does not fall back to secure storage after deleting env override file', async () => { + seedSecureStorage({ + accessToken: 'secure-access', + refreshToken: 'secure-refresh', + expiresAt: 4_100_000_000_000, + }) + await fsp.writeFile( + tokenPath, + JSON.stringify({ + accessToken: 'desktop-access', + refreshToken: 'desktop-refresh', + expiresAt: 4_100_000_000_000, + }), + 'utf-8', + ) + + expect(deleteOpenAIOAuthTokens()).toBe(true) + expect(getOpenAIOAuthTokens()).toBeNull() + await expect(getOpenAIOAuthTokensAsync()).resolves.toBeNull() + }) + + test('reloads sync tokens when OPENAI_CODEX_OAUTH_FILE changes without clearing cache', async () => { + const tokenPathA = path.join(tmpDir, 'openai-oauth-a.json') + const tokenPathB = path.join(tmpDir, 'openai-oauth-b.json') + await fsp.writeFile( + tokenPathA, + JSON.stringify({ + accessToken: 'access-a', + refreshToken: 'refresh-a', + expiresAt: 4_100_000_000_001, + }), + 'utf-8', + ) + await fsp.writeFile( + tokenPathB, + JSON.stringify({ + accessToken: 'access-b', + refreshToken: 'refresh-b', + expiresAt: 4_100_000_000_002, + }), + 'utf-8', + ) + + process.env.OPENAI_CODEX_OAUTH_FILE = tokenPathA + expect(getOpenAIOAuthTokens()?.accessToken).toBe('access-a') + + process.env.OPENAI_CODEX_OAUTH_FILE = tokenPathB + expect(getOpenAIOAuthTokens()?.accessToken).toBe('access-b') + }) + + test('cleans up tmp file if desktop token rename fails', async () => { + const renameSyncSpy = spyOn(fs, 'renameSync').mockImplementation(() => { + const error = new Error('rename failed') as NodeJS.ErrnoException + error.code = 'EXDEV' + throw error + }) + + const result = saveOpenAIOAuthTokens({ + accessToken: 'fresh-access', + refreshToken: 'fresh-refresh', + expiresAt: 4_100_000_000_000, + }) + + renameSyncSpy.mockRestore() + + expect(result.success).toBe(false) + const tmpFiles = (await fsp.readdir(tmpDir)).filter((name) => + name.startsWith('openai-oauth.json.tmp.'), + ) + expect(tmpFiles).toEqual([]) + }) }) diff --git a/src/services/openaiAuth/storage.ts b/src/services/openaiAuth/storage.ts index 666085ab..0c4eff51 100644 --- a/src/services/openaiAuth/storage.ts +++ b/src/services/openaiAuth/storage.ts @@ -13,6 +13,8 @@ type SecureStorageShape = Record & { openaiCodexOauth?: OpenAIOAuthTokens } +const SECURE_STORAGE_CACHE_KEY = '__secure-storage__' + function getDesktopTokenFilePath(): string | null { const filePath = process.env[OPENAI_CODEX_OAUTH_FILE_ENV_KEY]?.trim() return filePath ? filePath : null @@ -43,8 +45,7 @@ function normalizeTokenFile(value: unknown): OpenAIOAuthTokens | null { } } -function readDesktopTokenFileSync(): OpenAIOAuthTokens | null { - const filePath = getDesktopTokenFilePath() +function readDesktopTokenFileSync(filePath = getDesktopTokenFilePath()): OpenAIOAuthTokens | null { if (!filePath) return null try { @@ -57,8 +58,9 @@ function readDesktopTokenFileSync(): OpenAIOAuthTokens | null { } } -async function readDesktopTokenFileAsync(): Promise { - const filePath = getDesktopTokenFilePath() +async function readDesktopTokenFileAsync( + filePath = getDesktopTokenFilePath(), +): Promise { if (!filePath) return null try { @@ -78,11 +80,26 @@ function writeDesktopTokenFileSync(tokens: OpenAIOAuthTokens): boolean { fs.mkdirSync(path.dirname(filePath), { recursive: true }) const tmpFile = `${filePath}.tmp.${process.pid}.${Date.now()}` - fs.writeFileSync(tmpFile, JSON.stringify(tokens, null, 2) + '\n', { - mode: 0o600, - }) - fs.renameSync(tmpFile, filePath) - return true + let renamed = false + + try { + fs.writeFileSync(tmpFile, JSON.stringify(tokens, null, 2) + '\n', { + mode: 0o600, + }) + fs.renameSync(tmpFile, filePath) + renamed = true + return true + } finally { + if (!renamed) { + try { + fs.rmSync(tmpFile, { force: true }) + } catch (cleanupError) { + if ((cleanupError as NodeJS.ErrnoException).code !== 'ENOENT') { + logError(cleanupError) + } + } + } + } } export function saveOpenAIOAuthTokens(tokens: OpenAIOAuthTokens): { @@ -110,23 +127,34 @@ export function saveOpenAIOAuthTokens(tokens: OpenAIOAuthTokens): { } } -export const getOpenAIOAuthTokens = memoize((): OpenAIOAuthTokens | null => { - const desktopTokens = readDesktopTokenFileSync() - if (desktopTokens) return desktopTokens +const getOpenAIOAuthTokensCached = memoize( + (cacheKey: string): OpenAIOAuthTokens | null => { + if (cacheKey !== SECURE_STORAGE_CACHE_KEY) { + return readDesktopTokenFileSync(cacheKey) + } - try { - const storage = getSecureStorage() - const data = storage.read() as SecureStorageShape | null - return data?.openaiCodexOauth ?? null - } catch (error) { - logError(error) - return null - } -}) + try { + const storage = getSecureStorage() + const data = storage.read() as SecureStorageShape | null + return data?.openaiCodexOauth ?? null + } catch (error) { + logError(error) + return null + } + }, +) + +export function getOpenAIOAuthTokens(): OpenAIOAuthTokens | null { + return getOpenAIOAuthTokensCached( + getDesktopTokenFilePath() ?? SECURE_STORAGE_CACHE_KEY, + ) +} export async function getOpenAIOAuthTokensAsync(): Promise { - const desktopTokens = await readDesktopTokenFileAsync() - if (desktopTokens) return desktopTokens + const filePath = getDesktopTokenFilePath() + if (filePath) { + return readDesktopTokenFileAsync(filePath) + } try { const storage = getSecureStorage() @@ -139,7 +167,7 @@ export async function getOpenAIOAuthTokensAsync(): Promise