diff --git a/src/components/ProviderManager.test.tsx b/src/components/ProviderManager.test.tsx index 3db67c3e3e..a5472c4e22 100644 --- a/src/components/ProviderManager.test.tsx +++ b/src/components/ProviderManager.test.tsx @@ -300,14 +300,18 @@ function mockProviderManagerDependencies( updateProviderProfile?: (...args: any[]) => unknown setActiveProviderProfile?: (...args: any[]) => unknown useCodexOAuthFlow?: (options: { - onAuthenticated: (tokens: { - accessToken: string - refreshToken: string - accountId?: string - idToken?: string - apiKey?: string - }, persistCredentials: (options?: { profileId?: string }) => void) => - void | Promise + onAuthenticated: ( + tokens: { + accessToken: string + refreshToken: string + accountId?: string + idToken?: string + apiKey?: string + }, + persistCredentials: (options?: { + profileId?: string + }) => { warning?: string } | void, + ) => void | Promise }) => { state: 'starting' | 'waiting' | 'error' authUrl?: string @@ -1384,6 +1388,96 @@ test('ProviderManager first-run Codex OAuth switches the current session after l await mounted.dispose() }) +test('ProviderManager first-run Codex OAuth surfaces credential storage warnings', async () => { + delete process.env.CLAUDE_CODE_SIMPLE + delete process.env.CLAUDE_CODE_USE_GITHUB + delete process.env.GITHUB_TOKEN + delete process.env.GH_TOKEN + + const onDone = mock(() => {}) + const applySavedProfileToCurrentSession = mock(async () => null) + const persistCredentials = mock(() => ({ + warning: 'Warning: Storing credentials in plaintext.', + })) + const setActiveProviderProfile = mock((profileId: string) => ({ + id: profileId, + provider: 'openai', + name: 'Codex OAuth', + baseUrl: 'https://chatgpt.com/backend-api/codex', + model: 'codexplan', + apiKey: '', + })) + const addProviderProfile = mock((payload: { + provider: string + name: string + baseUrl: string + model: string + apiKey?: string + }) => ({ + id: 'provider_codex_oauth', + provider: payload.provider, + name: payload.name, + baseUrl: payload.baseUrl, + model: payload.model, + apiKey: payload.apiKey, + })) + + mockProviderManagerDependencies( + () => undefined, + async () => undefined, + { + addProviderProfile, + applySavedProfileToCurrentSession, + setActiveProviderProfile, + useCodexOAuthFlow: ({ onAuthenticated }) => { + React.useEffect(() => { + void onAuthenticated({ + accessToken: 'oauth-access-token', + refreshToken: 'oauth-refresh-token', + accountId: 'acct_oauth', + }, persistCredentials) + }, [onAuthenticated]) + + return { + state: 'waiting', + authUrl: 'https://chatgpt.com/codex', + browserOpened: true, + } + }, + }, + ) + + const nonce = `${Date.now()}-${Math.random()}` + const { ProviderManager } = await import(`./ProviderManager.js?ts=${nonce}`) + const mounted = await mountProviderManager(ProviderManager, { + mode: 'first-run', + onDone, + }) + + await waitForFrameOutput( + mounted.getOutput, + frame => frame.includes('Set up provider') && frame.includes('Codex OAuth'), + ) + + await navigateToPreset(mounted.stdin, 'Codex OAuth') + mounted.stdin.write('\r') + + await waitForCondition(() => onDone.mock.calls.length > 0) + + expect(persistCredentials).toHaveBeenCalledWith({ + profileId: 'provider_codex_oauth', + }) + expect(onDone).toHaveBeenCalledWith( + expect.objectContaining({ + action: 'saved', + message: + 'Codex OAuth configured. OpenClaude switched to it for this session with warnings: Warning: Storing credentials in plaintext.', + }), + ) + + await mounted.dispose() +}) + test('ProviderManager first-run Codex OAuth reports next-startup fallback when session activation fails', async () => { delete process.env.CLAUDE_CODE_SIMPLE delete process.env.CLAUDE_CODE_USE_GITHUB diff --git a/src/components/ProviderManager.tsx b/src/components/ProviderManager.tsx index 8f00333ade..cbf09c6f0e 100644 --- a/src/components/ProviderManager.tsx +++ b/src/components/ProviderManager.tsx @@ -110,6 +110,11 @@ type Screen = | 'select-edit' | 'select-delete' +type CodexOAuthPersistenceResult = { warning?: string } +type PersistCodexOAuthCredentials = (options?: { + profileId?: string +}) => CodexOAuthPersistenceResult | void + type DraftField = | 'name' | 'baseUrl' @@ -600,23 +605,32 @@ function CodexOAuthSetup({ onConfigured, }: { onBack: () => void - onConfigured: (tokens: { - accessToken: string - refreshToken: string - accountId?: string - idToken?: string - apiKey?: string - }, persistCredentials: (options?: { profileId?: string }) => void) => void | Promise + onConfigured: ( + tokens: { + accessToken: string + refreshToken: string + accountId?: string + idToken?: string + apiKey?: string + }, + persistCredentials: PersistCodexOAuthCredentials, + ) => void | Promise }): React.ReactNode { - const handleAuthenticated = React.useCallback(async (tokens: { - accessToken: string - refreshToken: string - accountId?: string - idToken?: string - apiKey?: string - }, persistCredentials: (options?: { profileId?: string }) => void) => { - await onConfigured(tokens, persistCredentials) - }, [onConfigured]) + const handleAuthenticated = React.useCallback( + async ( + tokens: { + accessToken: string + refreshToken: string + accountId?: string + idToken?: string + apiKey?: string + }, + persistCredentials: PersistCodexOAuthCredentials, + ) => { + await onConfigured(tokens, persistCredentials) + }, + [onConfigured], + ) useKeybinding('confirm:no', onBack) const status = useCodexOAuthFlow({ @@ -1078,17 +1092,22 @@ export function ProviderManager({ mode, onDone }: Props): React.ReactNode { return clearStartupProviderOverrides() } + function formatWarningsForMessage(warnings: string[]): string { + const joined = warnings.join('; ') + return /[.!?]$/.test(joined.trim()) ? joined : `${joined}.` + } + function buildCodexOAuthActivationMessage(options: { prefix: string activationWarning: string | null warnings: string[] }): string { if (options.activationWarning) { - return `${options.prefix}. Saved for next startup. Warning: ${options.warnings.join('; ')}.` + return `${options.prefix}. Saved for next startup. Warning: ${formatWarningsForMessage(options.warnings)}` } if (options.warnings.length > 0) { - return `${options.prefix}. OpenClaude switched to it for this session with warnings: ${options.warnings.join('; ')}.` + return `${options.prefix}. OpenClaude switched to it for this session with warnings: ${formatWarningsForMessage(options.warnings)}` } return `${options.prefix}. OpenClaude switched to it for this session.` @@ -2470,7 +2489,13 @@ export function ProviderManager({ mode, onDone }: Props): React.ReactNode { return } - persistCredentials({ profileId: saved.id }) + const persistenceResult = persistCredentials({ + profileId: saved.id, + }) + const storageWarning = + persistenceResult && typeof persistenceResult === 'object' + ? persistenceResult.warning + : null const settingsOverrideError = clearStartupProviderOverrideFromUserSettings() const activationWarning = await activateCodexOAuthSession(tokens) @@ -2478,6 +2503,7 @@ export function ProviderManager({ mode, onDone }: Props): React.ReactNode { setStoredCodexOAuthProfileId(saved.id) refreshProfiles() const warnings = [ + storageWarning, activationWarning, settingsOverrideError ? `could not clear startup provider override (${settingsOverrideError})` diff --git a/src/components/useCodexOAuthFlow.test.tsx b/src/components/useCodexOAuthFlow.test.tsx index 716058b588..3bd7eaf9e0 100644 --- a/src/components/useCodexOAuthFlow.test.tsx +++ b/src/components/useCodexOAuthFlow.test.tsx @@ -133,13 +133,21 @@ test('does not persist credentials when downstream setup rejects', async () => { ) function Harness(): React.ReactNode { - const handleAuthenticated = React.useCallback(onAuthenticated, [onAuthenticated]) + const handleAuthenticated = React.useCallback(onAuthenticated, [ + onAuthenticated, + ]) const status = useCodexOAuthFlow({ onAuthenticated: handleAuthenticated, deps, }) - return {status.state === 'error' ? status.message : status.state} + return ( + + {status.state === 'error' + ? `Codex OAuth failed: ${status.message}` + : status.state} + + ) } const streams = createTestStreams() @@ -195,7 +203,9 @@ test('persists credentials with profile linkage after downstream setup succeeds' ) function Harness(): React.ReactNode { - const handleAuthenticated = React.useCallback(onAuthenticated, [onAuthenticated]) + const handleAuthenticated = React.useCallback(onAuthenticated, [ + onAuthenticated, + ]) useCodexOAuthFlow({ onAuthenticated: handleAuthenticated, deps, @@ -230,3 +240,152 @@ test('persists credentials with profile linkage after downstream setup succeeds' await Bun.sleep(0) } }) + +test('returns a successful storage warning without entering the error state', async () => { + const saveCodexCredentials = mock(() => ({ + success: true, + warning: 'Warning: Storing credentials in plaintext.', + })) + let storageWarning: string | undefined + const onAuthenticated = mock( + async ( + _tokens: typeof TOKENS, + persistCredentials: ( + options?: { profileId?: string }, + ) => { warning?: string } | void, + ) => { + const result = persistCredentials({ + profileId: 'profile_codex_oauth', + }) + storageWarning = + result && typeof result === 'object' ? result.warning : undefined + }, + ) + const cleanup = mock(() => {}) + const deps = { + createOAuthService: () => ({ + async startOAuthFlow( + onAuthorizationUrl: (authUrl: string) => void | Promise, + ) { + await onAuthorizationUrl('https://chatgpt.com/codex') + return TOKENS + }, + cleanup, + }), + openBrowser: async () => true, + saveCodexCredentials, + isBareMode: () => false, + } + + const { useCodexOAuthFlow } = await import( + `./useCodexOAuthFlow.js?real-persist-warning-${Date.now()}-${Math.random()}` + ) + + function Harness(): React.ReactNode { + const handleAuthenticated = React.useCallback(onAuthenticated, [onAuthenticated]) + const status = useCodexOAuthFlow({ + onAuthenticated: handleAuthenticated, + deps, + }) + + return ( + + {status.state === 'error' + ? `Codex OAuth failed: ${status.message}` + : status.state} + + ) + } + + const streams = createTestStreams() + const root = await createRoot({ + stdout: streams.stdout as unknown as NodeJS.WriteStream, + stdin: streams.stdin as unknown as NodeJS.ReadStream, + patchConsole: false, + }) + root.render() + + try { + await waitForCondition(() => saveCodexCredentials.mock.calls.length === 1) + await Bun.sleep(0) + expect(storageWarning).toBe('Warning: Storing credentials in plaintext.') + expect(extractLastFrame(streams.getOutput())).not.toContain( + 'Codex OAuth failed', + ) + } finally { + root.unmount() + streams.stdin.end() + streams.stdout.end() + await Bun.sleep(0) + } +}) + +test('reports credential persistence failures without token values', async () => { + const saveCodexCredentials = mock(() => ({ + success: false, + warning: 'secure storage unavailable', + })) + const onAuthenticated = mock( + async ( + _tokens: typeof TOKENS, + persistCredentials: (options?: { profileId?: string }) => void, + ) => { + persistCredentials({ profileId: 'profile_codex_oauth' }) + }, + ) + const cleanup = mock(() => {}) + const deps = { + createOAuthService: () => ({ + async startOAuthFlow( + onAuthorizationUrl: (authUrl: string) => void | Promise, + ) { + await onAuthorizationUrl('https://chatgpt.com/codex') + return TOKENS + }, + cleanup, + }), + openBrowser: async () => true, + saveCodexCredentials, + isBareMode: () => false, + } + + const { useCodexOAuthFlow } = await import( + `./useCodexOAuthFlow.js?real-persist-failure-${Date.now()}-${Math.random()}` + ) + + function Harness(): React.ReactNode { + const handleAuthenticated = React.useCallback(onAuthenticated, [onAuthenticated]) + const status = useCodexOAuthFlow({ + onAuthenticated: handleAuthenticated, + deps, + }) + + return {status.state === 'error' ? status.message : status.state} + } + + const streams = createTestStreams() + const root = await createRoot({ + stdout: streams.stdout as unknown as NodeJS.WriteStream, + stdin: streams.stdin as unknown as NodeJS.ReadStream, + patchConsole: false, + }) + root.render() + + try { + await waitForCondition(() => + extractLastFrame(streams.getOutput()).includes( + 'secure storage unavailable', + ), + ) + const frame = extractLastFrame(streams.getOutput()) + expect(frame).toContain('secure storage unavailable') + for (const value of Object.values(TOKENS)) { + expect(frame).not.toContain(value) + } + } finally { + root.unmount() + streams.stdin.end() + streams.stdout.end() + await Bun.sleep(0) + } +}) diff --git a/src/components/useCodexOAuthFlow.ts b/src/components/useCodexOAuthFlow.ts index f0de299af9..bacf387357 100644 --- a/src/components/useCodexOAuthFlow.ts +++ b/src/components/useCodexOAuthFlow.ts @@ -22,7 +22,7 @@ export type CodexOAuthFlowStatus = type PersistCodexOAuthCredentials = (options?: { profileId?: string -}) => void +}) => { warning?: string } type CodexOAuthFlowDependencies = { createOAuthService?: () => Pick< @@ -106,6 +106,7 @@ export function useCodexOAuthFlow(options: { 'Codex OAuth succeeded, but credentials could not be saved securely.', ) } + return { warning: saved.warning } } await onAuthenticated(tokens, persistCredentials) diff --git a/src/utils/codexCredentials.test.ts b/src/utils/codexCredentials.test.ts index 5fc6b62111..da235a78ff 100644 --- a/src/utils/codexCredentials.test.ts +++ b/src/utils/codexCredentials.test.ts @@ -19,9 +19,46 @@ describe('codexCredentials', () => { const originalSimple = process.env.CLAUDE_CODE_SIMPLE const originalCodeKey = process.env.CODEX_API_KEY const originalFetch = globalThis.fetch + let mockedPlainTextStorageState: Record | null = null + let mockedPlainTextStorageUpdateResult: { + success: boolean + warning?: string + } = { + success: true, + warning: 'Warning: Storing credentials in plaintext.', + } + let mockedPlainTextStorageDeleteResult = true + let mockedPlainTextStorageUpdates: Record[] = [] beforeEach(async () => { await acquireSharedMutationLock('utils/codexCredentials.test.ts') + mockedPlainTextStorageState = null + mockedPlainTextStorageUpdateResult = { + success: true, + warning: 'Warning: Storing credentials in plaintext.', + } + mockedPlainTextStorageDeleteResult = true + mockedPlainTextStorageUpdates = [] + + mock.module('./secureStorage/plainTextStorage.js', () => ({ + plainTextStorage: { + read: () => mockedPlainTextStorageState, + readAsync: async () => mockedPlainTextStorageState, + update: (next: Record) => { + mockedPlainTextStorageUpdates.push(next) + if (mockedPlainTextStorageUpdateResult.success) { + mockedPlainTextStorageState = next + } + return mockedPlainTextStorageUpdateResult + }, + delete: () => { + if (mockedPlainTextStorageDeleteResult) { + mockedPlainTextStorageState = null + } + return mockedPlainTextStorageDeleteResult + }, + }, + })) }) afterEach(() => { @@ -62,15 +99,16 @@ describe('codexCredentials', () => { expect(result.warning).toContain('Bare mode') }) - test('saveCodexCredentials refuses plaintext fallback when native secure storage is unavailable', async () => { + test('saveCodexCredentials allows plaintext fallback when native secure storage is unavailable', async () => { delete process.env.CLAUDE_CODE_SIMPLE - mock.module('./secureStorage/index.js', () => ({ - getSecureStorage: (options?: { allowPlainTextFallback?: boolean }) => { + let nativeStorageState: Record | null = null + const getSecureStorage = mock( + (options?: { allowPlainTextFallback?: boolean }) => { expect(options?.allowPlainTextFallback).toBe(false) return { - read: () => null, - readAsync: async () => null, + read: () => nativeStorageState, + readAsync: async () => nativeStorageState, update: () => ({ success: false, warning: @@ -79,20 +117,287 @@ describe('codexCredentials', () => { delete: () => true, } }, + ) + + mock.module('./secureStorage/index.js', () => ({ + getSecureStorage, + })) + + // @ts-expect-error cache-busting query string for Bun module mocks + const { readCodexCredentials, saveCodexCredentials } = await import( + './codexCredentials.js?save-plaintext-fallback' + ) + + const result = saveCodexCredentials({ + accessToken: 'fallback-access-token', + refreshToken: 'fallback-refresh-token', + accountId: 'acct_123', + }) + + expect(getSecureStorage).toHaveBeenCalled() + expect(result.success).toBe(true) + expect(result.warning).toBe('Warning: Storing credentials in plaintext.') + expect(readCodexCredentials()).toMatchObject({ + accessToken: 'fallback-access-token', + refreshToken: 'fallback-refresh-token', + accountId: 'acct_123', + }) + }) + + test('saveCodexCredentials keeps plaintext fallback scoped to Codex credentials', async () => { + delete process.env.CLAUDE_CODE_SIMPLE + + const nativeStorageState: Record = { + mcpOAuth: { + server: { + serverName: 'Server', + serverUrl: 'https://example.test', + accessToken: 'native-mcp-access-token', + refreshToken: 'native-mcp-refresh-token', + expiresAt: Date.now() + 60_000, + clientSecret: 'native-mcp-client-secret', + }, + }, + trustedDeviceToken: 'native-trusted-device-token', + pluginSecrets: { + plugin: { + secret: 'native-plugin-secret', + }, + }, + } + mockedPlainTextStorageState = { + pluginSecrets: { + alreadyPlaintext: { + secret: 'existing-plaintext-secret', + }, + }, + } + let attemptedNativeWrite: Record | undefined + + mock.module('./secureStorage/index.js', () => ({ + getSecureStorage: (options?: { allowPlainTextFallback?: boolean }) => { + expect(options?.allowPlainTextFallback).toBe(false) + return { + read: () => nativeStorageState, + readAsync: async () => nativeStorageState, + update: (next: Record) => { + attemptedNativeWrite = next + return { success: false, warning: 'native write failed' } + }, + delete: () => true, + } + }, })) // @ts-expect-error cache-busting query string for Bun module mocks const { saveCodexCredentials } = await import( - './codexCredentials.js?save-no-plaintext-fallback' + './codexCredentials.js?save-scoped-plaintext-fallback' ) const result = saveCodexCredentials({ - accessToken: 'token', + accessToken: 'codex-access-token', + refreshToken: 'codex-refresh-token', + accountId: 'acct_123', + }) + + expect(result.success).toBe(true) + expect(result.warning).toBe('Warning: Storing credentials in plaintext.') + expect(attemptedNativeWrite?.mcpOAuth).toBe(nativeStorageState.mcpOAuth) + expect(mockedPlainTextStorageState?.codex).toMatchObject({ + accessToken: 'codex-access-token', + refreshToken: 'codex-refresh-token', + accountId: 'acct_123', + }) + expect(mockedPlainTextStorageState?.pluginSecrets).toEqual({ + alreadyPlaintext: { + secret: 'existing-plaintext-secret', + }, + }) + expect(mockedPlainTextStorageState).not.toHaveProperty('mcpOAuth') + expect(mockedPlainTextStorageState).not.toHaveProperty('trustedDeviceToken') + }) + + test('saveCodexCredentials fails closed when native Codex would shadow scoped fallback', async () => { + delete process.env.CLAUDE_CODE_SIMPLE + + const nativeStorageState: Record = { + codex: { + accessToken: 'native-codex-access-token', + refreshToken: 'native-codex-refresh-token', + accountId: 'acct_old', + }, + mcpOAuth: { + server: { + serverName: 'Server', + serverUrl: 'https://example.test', + accessToken: 'native-mcp-access-token', + expiresAt: Date.now() + 60_000, + clientSecret: 'native-mcp-client-secret', + }, + }, + } + + mock.module('./secureStorage/index.js', () => ({ + getSecureStorage: (options?: { allowPlainTextFallback?: boolean }) => { + expect(options?.allowPlainTextFallback).toBe(false) + return { + read: () => nativeStorageState, + readAsync: async () => nativeStorageState, + update: () => ({ success: false, warning: 'native write failed' }), + delete: () => true, + } + }, + })) + + // @ts-expect-error cache-busting query string for Bun module mocks + const { saveCodexCredentials } = await import( + './codexCredentials.js?save-fail-closed-shadowed-fallback' + ) + + const result = saveCodexCredentials({ + accessToken: 'codex-access-token', + refreshToken: 'codex-refresh-token', accountId: 'acct_123', }) expect(result.success).toBe(false) - expect(result.warning).toContain('without plaintext fallback') + expect(result.warning).toBe('native write failed') + expect(mockedPlainTextStorageUpdates).toHaveLength(0) + expect(mockedPlainTextStorageState).toBeNull() + }) + + test('saveCodexCredentials fails when stale native Codex cannot be removed after fallback write', async () => { + delete process.env.CLAUDE_CODE_SIMPLE + + const nativeStorageState: Record = { + codex: { + accessToken: 'native-codex-access-token', + refreshToken: 'native-codex-refresh-token', + accountId: 'acct_old', + }, + } + let nativeDeleteAttempts = 0 + + mock.module('./secureStorage/index.js', () => ({ + getSecureStorage: (options?: { allowPlainTextFallback?: boolean }) => { + expect(options?.allowPlainTextFallback).toBe(false) + return { + read: () => nativeStorageState, + readAsync: async () => nativeStorageState, + update: () => ({ success: false, warning: 'native write failed' }), + delete: () => { + nativeDeleteAttempts += 1 + return false + }, + } + }, + })) + + // @ts-expect-error cache-busting query string for Bun module mocks + const { readCodexCredentials, saveCodexCredentials } = await import( + './codexCredentials.js?save-fail-closed-stale-native-delete' + ) + + const result = saveCodexCredentials({ + accessToken: 'codex-access-token', + refreshToken: 'codex-refresh-token', + accountId: 'acct_123', + }) + + expect(result.success).toBe(false) + expect(result.warning).toBe( + 'Codex credentials were written to plaintext fallback, but stale native secure storage could not be removed.', + ) + expect(nativeDeleteAttempts).toBe(1) + expect(mockedPlainTextStorageState?.codex).toMatchObject({ + accessToken: 'codex-access-token', + refreshToken: 'codex-refresh-token', + accountId: 'acct_123', + }) + expect(readCodexCredentials()?.accessToken).toBe( + 'native-codex-access-token', + ) + }) + + test('saveCodexCredentials warns when native save succeeds but plaintext fallback cleanup fails', async () => { + delete process.env.CLAUDE_CODE_SIMPLE + + let nativeStorageState: Record | null = null + mockedPlainTextStorageState = { + codex: { + accessToken: 'plaintext-codex-access-token', + refreshToken: 'plaintext-codex-refresh-token', + accountId: 'acct_plaintext', + }, + } + mockedPlainTextStorageDeleteResult = false + + mock.module('./secureStorage/index.js', () => ({ + getSecureStorage: (options?: { allowPlainTextFallback?: boolean }) => { + expect(options?.allowPlainTextFallback).toBe(false) + return { + read: () => nativeStorageState, + readAsync: async () => nativeStorageState, + update: (next: Record) => { + nativeStorageState = next + return { success: true } + }, + delete: () => true, + } + }, + })) + + // @ts-expect-error cache-busting query string for Bun module mocks + const { readCodexCredentials, saveCodexCredentials } = await import( + './codexCredentials.js?save-native-success-plaintext-cleanup-fails' + ) + + const result = saveCodexCredentials({ + accessToken: 'native-codex-access-token', + refreshToken: 'native-codex-refresh-token', + accountId: 'acct_native', + }) + + expect(result.success).toBe(true) + expect(result.warning).toBe( + 'Codex credentials were saved, but stale plaintext fallback credentials could not be removed.', + ) + expect(readCodexCredentials()?.accessToken).toBe( + 'native-codex-access-token', + ) + expect(mockedPlainTextStorageState?.codex).toMatchObject({ + accessToken: 'plaintext-codex-access-token', + }) + }) + + test('saveCodexCredentials rejects incomplete credential blobs', async () => { + delete process.env.CLAUDE_CODE_SIMPLE + + const getSecureStorage = mock(() => ({ + read: () => null, + readAsync: async () => null, + update: () => ({ success: true }), + delete: () => true, + })) + + mock.module('./secureStorage/index.js', () => ({ + getSecureStorage, + })) + + // @ts-expect-error cache-busting query string for Bun module mocks + const { saveCodexCredentials } = await import( + './codexCredentials.js?save-incomplete' + ) + + const result = saveCodexCredentials({ + accessToken: '', + refreshToken: 'fallback-refresh-token', + accountId: 'acct_123', + }) + + expect(result.success).toBe(false) + expect(result.warning).toBe('Codex credentials are incomplete.') + expect(getSecureStorage).not.toHaveBeenCalled() }) test('refreshCodexAccessTokenIfNeeded refreshes expired stored credentials', async () => { @@ -513,6 +818,53 @@ describe('codexCredentials', () => { expect(readCodexCredentials()?.profileId).toBe('profile_codex_oauth') }) + test('clearCodexCredentials does not copy unrelated native secrets to plaintext when native update fails', async () => { + delete process.env.CLAUDE_CODE_SIMPLE + + const nativeStorageState: Record = { + codex: { + accessToken: 'access-old', + refreshToken: 'refresh-old', + accountId: 'acct_old', + }, + mcpOAuth: { + server: { + serverName: 'Server', + serverUrl: 'https://example.test', + accessToken: 'native-mcp-access-token', + expiresAt: Date.now() + 60_000, + clientSecret: 'native-mcp-client-secret', + }, + }, + trustedDeviceToken: 'native-trusted-device-token', + } + mockedPlainTextStorageState = null + + mock.module('./secureStorage/index.js', () => ({ + getSecureStorage: (options?: { allowPlainTextFallback?: boolean }) => { + expect(options?.allowPlainTextFallback).toBe(false) + return { + read: () => nativeStorageState, + readAsync: async () => nativeStorageState, + update: () => ({ success: false, warning: 'native write failed' }), + delete: () => true, + } + }, + })) + + // @ts-expect-error cache-busting query string for Bun module mocks + const { clearCodexCredentials } = await import( + './codexCredentials.js?clear-scoped-plaintext-fallback' + ) + + const result = clearCodexCredentials() + + expect(result.success).toBe(false) + expect(result.warning).toBe('native write failed') + expect(mockedPlainTextStorageUpdates).toHaveLength(0) + expect(mockedPlainTextStorageState).toBeNull() + }) + test('refreshCodexAccessTokenIfNeeded uses async secure-storage reads in its request path', async () => { delete process.env.CLAUDE_CODE_SIMPLE delete process.env.CODEX_API_KEY @@ -558,6 +910,7 @@ describe('codexCredentials', () => { test('refreshCodexAccessTokenIfNeeded keeps a cooldown in memory when secure storage cannot persist it', async () => { delete process.env.CLAUDE_CODE_SIMPLE delete process.env.CODEX_API_KEY + mockedPlainTextStorageUpdateResult = { success: false } const expiredToken = makeJwt({ exp: Math.floor((Date.now() - 60_000) / 1000), diff --git a/src/utils/codexCredentials.ts b/src/utils/codexCredentials.ts index 3df3bfe5cd..6f3260b8a0 100644 --- a/src/utils/codexCredentials.ts +++ b/src/utils/codexCredentials.ts @@ -1,6 +1,10 @@ import { isBareMode } from './envUtils.js' import { createCombinedAbortSignal } from './combinedAbortSignal.js' -import { getSecureStorage } from './secureStorage/index.js' +import { + getSecureStorage, + type SecureStorageData, +} from './secureStorage/index.js' +import { plainTextStorage } from './secureStorage/plainTextStorage.js' import { asTrimmedString, CODEX_REFRESH_URL, @@ -13,6 +17,10 @@ import { export const CODEX_STORAGE_KEY = 'codex' as const const CODEX_TOKEN_REFRESH_SKEW_MS = 60_000 const CODEX_TOKEN_REFRESH_RETRY_COOLDOWN_MS = 60_000 +const CODEX_FALLBACK_NATIVE_DELETE_FAILED_WARNING = + 'Codex credentials were written to plaintext fallback, but stale native secure storage could not be removed.' +const CODEX_PLAINTEXT_CLEANUP_FAILED_WARNING = + 'Codex credentials were saved, but stale plaintext fallback credentials could not be removed.' export type CodexCredentialBlob = { apiKey?: string @@ -39,7 +47,7 @@ let inFlightCodexRefresh: | null = null let inMemoryLastRefreshFailureAt: number | null = null -function getCodexSecureStorage() { +function getCodexPrimarySecureStorage() { return getSecureStorage({ allowPlainTextFallback: false }) } @@ -94,6 +102,103 @@ function normalizeCodexCredentialBlob( } } +function getRecord(data: SecureStorageData | null | undefined): Record< + string, + unknown +> { + return data && typeof data === 'object' + ? (data as Record) + : {} +} + +function hasStoredCodexRecord( + data: SecureStorageData | null | undefined, +): boolean { + return Object.prototype.hasOwnProperty.call(getRecord(data), CODEX_STORAGE_KEY) +} + +function hasNonCodexStorageFields( + data: SecureStorageData | null | undefined, +): boolean { + return Object.keys(getRecord(data)).some(key => key !== CODEX_STORAGE_KEY) +} + +function readCodexFromPlainTextStorage(): CodexCredentialBlob | undefined { + try { + const data = plainTextStorage.read() + return normalizeCodexCredentialBlob(data?.[CODEX_STORAGE_KEY]) + } catch { + return undefined + } +} + +async function readCodexFromPlainTextStorageAsync(): Promise< + CodexCredentialBlob | undefined +> { + try { + const data = await plainTextStorage.readAsync() + return normalizeCodexCredentialBlob(data?.[CODEX_STORAGE_KEY]) + } catch { + return undefined + } +} + +// Do not use the generic secure-storage fallback for Codex writes. Its update() +// path receives the whole storage document, so a native-write failure can copy +// unrelated native-only secrets into plaintext. +function writeCodexToPlainTextStorage( + codex: CodexCredentialBlob, +): { success: boolean; warning?: string } { + try { + const previous = plainTextStorage.read() || {} + const next = { + ...getRecord(previous), + [CODEX_STORAGE_KEY]: codex, + } + return plainTextStorage.update(next as SecureStorageData) + } catch { + return { success: false } + } +} + +function removeCodexFromPlainTextStorage(): { + success: boolean + warning?: string +} { + try { + const previous = plainTextStorage.read() + if (!hasStoredCodexRecord(previous)) { + return { success: true } + } + + const next = { ...getRecord(previous) } + delete next[CODEX_STORAGE_KEY] + + if (Object.keys(next).length === 0) { + return plainTextStorage.delete() + ? { success: true } + : { + success: false, + warning: CODEX_PLAINTEXT_CLEANUP_FAILED_WARNING, + } + } + + const result = plainTextStorage.update(next as SecureStorageData) + return result.success + ? { success: true } + : { + success: false, + warning: + result.warning ?? CODEX_PLAINTEXT_CLEANUP_FAILED_WARNING, + } + } catch { + return { + success: false, + warning: CODEX_PLAINTEXT_CLEANUP_FAILED_WARNING, + } + } +} + function shouldRefreshCodexToken(blob: CodexCredentialBlob): boolean { const expiresAt = parseJwtExpiryMs(blob.accessToken) ?? parseJwtExpiryMs(blob.idToken) @@ -151,11 +256,14 @@ export function readCodexCredentials(): CodexCredentialBlob | undefined { if (isBareMode()) return undefined try { - const data = getCodexSecureStorage().read() - return normalizeCodexCredentialBlob(data?.codex) + const data = getCodexPrimarySecureStorage().read() + const primaryCodex = normalizeCodexCredentialBlob(data?.codex) + if (primaryCodex) return primaryCodex } catch { - return undefined + // Fall through to the Codex-only plaintext fallback. } + + return readCodexFromPlainTextStorage() } export async function readCodexCredentialsAsync(): Promise< @@ -164,11 +272,14 @@ export async function readCodexCredentialsAsync(): Promise< if (isBareMode()) return undefined try { - const data = await getCodexSecureStorage().readAsync() - return normalizeCodexCredentialBlob(data?.codex) + const data = await getCodexPrimarySecureStorage().readAsync() + const primaryCodex = normalizeCodexCredentialBlob(data?.codex) + if (primaryCodex) return primaryCodex } catch { - return undefined + // Fall through to the Codex-only plaintext fallback. } + + return readCodexFromPlainTextStorageAsync() } export function isCodexRefreshFailureCoolingDown( @@ -193,23 +304,66 @@ export function saveCodexCredentials( return { success: false, warning: 'Codex credentials are incomplete.' } } - const secureStorage = getCodexSecureStorage() - const previous = secureStorage.read() || {} - const previousCodex = normalizeCodexCredentialBlob(previous[CODEX_STORAGE_KEY]) + const secureStorage = getCodexPrimarySecureStorage() + const previous = secureStorage.read() + const previousNativeCodex = normalizeCodexCredentialBlob( + previous?.[CODEX_STORAGE_KEY], + ) + const previousCodex = previousNativeCodex ?? readCodexFromPlainTextStorage() const next = { - ...(previous as Record), + ...getRecord(previous), [CODEX_STORAGE_KEY]: { ...normalized, profileId: normalized.profileId ?? previousCodex?.profileId, lastRefreshAt: normalized.lastRefreshAt ?? Date.now(), }, } - const result = secureStorage.update(next as typeof previous) + const result = secureStorage.update(next as SecureStorageData) if (result.success) { const storedCodex = normalizeCodexCredentialBlob(next[CODEX_STORAGE_KEY]) inMemoryLastRefreshFailureAt = storedCodex?.lastRefreshFailureAt ?? null + const cleanupResult = removeCodexFromPlainTextStorage() + if (!cleanupResult.success) { + return { + success: true, + warning: + cleanupResult.warning ?? CODEX_PLAINTEXT_CLEANUP_FAILED_WARNING, + } + } + return result } - return result + + // If native storage still contains a Codex record and unrelated secrets, a + // plaintext Codex-only fallback would be shadowed by the stale native record. + // Deleting the native document would remove unrelated secrets, so fail closed. + if (previousNativeCodex && hasNonCodexStorageFields(previous)) { + return result + } + + const fallbackResult = writeCodexToPlainTextStorage( + next[CODEX_STORAGE_KEY] as CodexCredentialBlob, + ) + if (fallbackResult.success) { + if (previousNativeCodex && !secureStorage.delete()) { + return { + success: false, + warning: CODEX_FALLBACK_NATIVE_DELETE_FAILED_WARNING, + } + } + + if (!previousNativeCodex && !hasNonCodexStorageFields(previous)) { + secureStorage.delete() + } + + const storedCodex = normalizeCodexCredentialBlob(next[CODEX_STORAGE_KEY]) + inMemoryLastRefreshFailureAt = storedCodex?.lastRefreshFailureAt ?? null + return { + success: true, + warning: fallbackResult.warning, + } + } + + return fallbackResult.warning ? fallbackResult : result } export function attachCodexProfileIdToStoredCredentials(profileId: string): { @@ -255,14 +409,33 @@ export function clearCodexCredentials(): { return { success: true } } - const secureStorage = getCodexSecureStorage() - const previous = secureStorage.read() || {} - const next = { ...(previous as Record) } + const secureStorage = getCodexPrimarySecureStorage() + const previous = secureStorage.read() + const previousCodex = normalizeCodexCredentialBlob( + previous?.[CODEX_STORAGE_KEY], + ) + + if (!previousCodex) { + const plaintextResult = removeCodexFromPlainTextStorage() + if (plaintextResult.success) { + inMemoryLastRefreshFailureAt = null + } + return plaintextResult + } + + const next = { ...getRecord(previous) } delete next[CODEX_STORAGE_KEY] - const result = secureStorage.update(next as typeof previous) - if (result.success) { - inMemoryLastRefreshFailureAt = null + const result = secureStorage.update(next as SecureStorageData) + if (!result.success) { + return result } + + const plaintextResult = removeCodexFromPlainTextStorage() + if (!plaintextResult.success) { + return plaintextResult + } + + inMemoryLastRefreshFailureAt = null return result } @@ -285,7 +458,8 @@ export async function refreshCodexAccessTokenIfNeeded(options?: { return { refreshed: false } } - if (!current.refreshToken) { + const refreshToken = current.refreshToken + if (!refreshToken) { return { refreshed: false, credentials: current } } @@ -308,7 +482,7 @@ export async function refreshCodexAccessTokenIfNeeded(options?: { const body = new URLSearchParams({ client_id: getCodexOAuthClientId(), grant_type: 'refresh_token', - refresh_token: current.refreshToken, + refresh_token: refreshToken, }) const { signal, cleanup } = createCombinedAbortSignal(undefined, {