diff --git a/src/main/codex-accounts/runtime-home-service.ts b/src/main/codex-accounts/runtime-home-service.ts index d14ac6cffb8..242e9d68da9 100644 --- a/src/main/codex-accounts/runtime-home-service.ts +++ b/src/main/codex-accounts/runtime-home-service.ts @@ -16,6 +16,7 @@ import { symlinkSync, unlinkSync } from 'node:fs' +import { isDefinitiveAbsence } from '../../shared/definitive-filesystem-absence' import { execFileSync } from 'node:child_process' import { dirname, @@ -2011,7 +2012,15 @@ export class CodexRuntimeHomeService { } const provenance: CodexSharedRuntimeAuthProvenance = owner.owner === 'system-default' ? { owner: 'system-default', authJson: contents } : owner - const runtimeAuthAlreadyMatches = this.fileContentsEqual(runtimeAuthPath, contents) + const runtimeAuthComparison = this.compareFileContents(runtimeAuthPath, contents) + if (runtimeAuthComparison === null) { + // Why: an unreadable runtime auth.json may hold a token Codex rotated a + // moment ago. Treating "could not read" as "differs" sent execution to the + // unconditional write below, consuming that rotation and logging the user + // out for good. Refuse; the next sync retries. + return false + } + const runtimeAuthAlreadyMatches = runtimeAuthComparison if ( runtimeAuthAlreadyMatches && this.sharedRuntimeAuthProvenanceMatches( @@ -2059,16 +2068,31 @@ export class CodexRuntimeHomeService { writeFileAtomically(authPath, contents, { mode: 0o600 }) } - private fileContentsEqual(targetPath: string, contents: string): boolean { + /** + * `true`/`false` only when the bytes were actually read; `null` when the file + * could not be read at all. The old `catch { return false }` reported "these + * differ" for a file nobody could open, and every caller reads that as + * permission to write. + */ + private compareFileContents(targetPath: string, contents: string): boolean | null { try { - return existsSync(targetPath) && readFileSync(targetPath, 'utf-8') === contents - } catch { - return false + return readFileSync(targetPath, 'utf-8') === contents + } catch (error) { + return isDefinitiveAbsence(error) ? false : null } } + private fileContentsEqual(targetPath: string, contents: string): boolean { + return this.compareFileContents(targetPath, contents) === true + } + private fileContentsMatchExpected(targetPath: string, expectedContents: string | null): boolean { if (expectedContents === null) { + // Why: `!existsSync` does report `true` for a locked file, but this branch + // is not where that matters — the write it guards is + // `writeFileAtomicallyIfUnchanged`, whose rename-and-compare re-checks the + // real file and refuses on its own. Classifying here would be a guard no + // test can drive. return !existsSync(targetPath) } return this.fileContentsEqual(targetPath, expectedContents) diff --git a/src/main/codex-accounts/sta-4735-unreadable-host-lane-writes.test.ts b/src/main/codex-accounts/sta-4735-unreadable-host-lane-writes.test.ts new file mode 100644 index 00000000000..aaa0fb42312 --- /dev/null +++ b/src/main/codex-accounts/sta-4735-unreadable-host-lane-writes.test.ts @@ -0,0 +1,160 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type * as NodeFs from 'node:fs' +import { join } from 'node:path' +import { createSettings } from './runtime-home-settings-test-fixtures' +import { + createCodexAccountRecord, + createCodexAuthJson, + createManagedAuth, + createStore, + getRuntimeCodexAuthPath, + setupRuntimeHomeTest, + teardownRuntimeHomeTest, + testState +} from './runtime-home-service-test-harness' + +// STA-4735: two host-lane reads still answered "absent" from a read that had +// only failed, and both answers authorised a write over live data. + +const denials = vi.hoisted(() => { + const state = { + paths: new Set(), + reads: new Map(), + deny(path: string): void { + state.paths.add(path) + }, + release(path: string): void { + state.paths.delete(path) + }, + readsFor(path: string): number { + return state.reads.get(path) ?? 0 + }, + reset(): void { + state.paths.clear() + state.reads.clear() + }, + check(target: unknown, syscall: string): void { + if (typeof target !== 'string' || !state.paths.has(target)) { + return + } + state.reads.set(target, (state.reads.get(target) ?? 0) + 1) + const error: NodeJS.ErrnoException = new Error( + `EPERM: operation not permitted, ${syscall} '${target}'` + ) + error.code = 'EPERM' + error.errno = -4048 + error.syscall = syscall + error.path = target + throw error + } + } + return state +}) + +vi.mock('node:fs', async (importOriginal) => { + const actual = await importOriginal() + const guard = (fn: unknown, syscall: string): unknown => { + const original = fn as (...args: unknown[]) => unknown + const wrapped = (...args: unknown[]): unknown => { + denials.check(args[0], syscall) + return original(...args) + } + return Object.assign(wrapped, original) + } + const patched: Record = { + ...actual, + readFileSync: guard(actual.readFileSync, 'read'), + lstatSync: guard(actual.lstatSync, 'lstat'), + statSync: guard(actual.statSync, 'stat'), + existsSync: Object.assign( + (...args: unknown[]): boolean => + typeof args[0] === 'string' && denials.paths.has(args[0]) + ? false + : actual.existsSync(args[0] as string), + actual.existsSync + ) + } + return { ...patched, default: patched } +}) + +vi.mock('electron', () => ({ app: { getPath: () => testState.userDataDir } })) + +vi.mock('node:os', async () => { + const actual = await vi.importActual('node:os') // eslint-disable-line @typescript-eslint/consistent-type-imports -- vi.importActual requires inline import() + return { ...actual, homedir: () => testState.fakeHomeDir } +}) + +const realFs = await vi.importActual('node:fs') + +type RuntimeAuthWriter = { + writeRuntimeAuth( + contents: string, + owner: { owner: 'system-default' } | { owner: 'managed'; accountId: string }, + options?: { expectedContents: string | null } + ): boolean +} + +describe('STA-4735 an unreadable runtime auth.json must not be written over', () => { + beforeEach(() => { + denials.reset() + setupRuntimeHomeTest() + }) + + afterEach(() => { + denials.reset() + teardownRuntimeHomeTest() + }) + + async function createService(): Promise { + const managedHomePath = createManagedAuth( + testState.userDataDir, + 'account-1', + createCodexAuthJson('user@example.com', 'acct-1', 'refresh-1') + ) + const store = createStore( + createSettings({ + codexManagedAccounts: [ + createCodexAccountRecord('account-1', 'user@example.com', 'acct-1', managedHomePath) + ], + activeCodexManagedAccountId: 'account-1', + activeCodexManagedAccountIdsByRuntime: { host: 'account-1', wsl: {} } + }) + ) + const { CodexRuntimeHomeService } = await import('./runtime-home-service') + return new CodexRuntimeHomeService(store as never) as unknown as RuntimeAuthWriter + } + + it('refuses the write instead of overwriting a token it could not read', async () => { + const service = await createService() + const runtimeAuthPath = getRuntimeCodexAuthPath() + const ROTATED = createCodexAuthJson('user@example.com', 'acct-1', 'refresh-rotated-by-codex') + const STALE = createCodexAuthJson('user@example.com', 'acct-1', 'refresh-stale-orca-copy') + realFs.mkdirSync(join(runtimeAuthPath, '..'), { recursive: true }) + realFs.writeFileSync(runtimeAuthPath, ROTATED, 'utf-8') + + denials.deny(runtimeAuthPath) + const wrote = service.writeRuntimeAuth(STALE, { owner: 'managed', accountId: 'account-1' }) + + // The fault really was consumed by the code under test. + expect(denials.readsFor(runtimeAuthPath)).toBeGreaterThan(0) + // THE FIX. Before it, "could not read" counted as "differs" and the write + // below replaced a freshly rotated refresh token with Orca's stale copy. + expect(wrote).toBe(false) + expect(realFs.readFileSync(runtimeAuthPath, 'utf-8')).toBe(ROTATED) + }) + + it('still writes once the file is readable again', async () => { + const service = await createService() + const runtimeAuthPath = getRuntimeCodexAuthPath() + const NEXT = createCodexAuthJson('user@example.com', 'acct-1', 'refresh-2') + realFs.mkdirSync(join(runtimeAuthPath, '..'), { recursive: true }) + realFs.writeFileSync(runtimeAuthPath, 'stale\n', 'utf-8') + + // Why: the anchor. A fix that simply stopped writing would pass the test + // above and break every credential sync. + const wrote = service.writeRuntimeAuth(NEXT, { owner: 'managed', accountId: 'account-1' }) + + expect(wrote).toBe(true) + expect(realFs.readFileSync(runtimeAuthPath, 'utf-8')).toBe(NEXT) + }) +}) diff --git a/src/main/codex/hook-trust-promotion.ts b/src/main/codex/hook-trust-promotion.ts index 16b19ceeae6..9f6b5a867e9 100644 --- a/src/main/codex/hook-trust-promotion.ts +++ b/src/main/codex/hook-trust-promotion.ts @@ -1,5 +1,6 @@ import { existsSync, readFileSync, writeFileSync } from 'node:fs' import { join } from 'node:path' +import { isDefinitiveAbsence } from '../../shared/definitive-filesystem-absence' import { createManagedCommandMatcher, readHooksJson, @@ -43,15 +44,23 @@ function getProvenancePath(runtimeHomePath: string): string { return join(runtimeHomePath, '.orca-hook-trust-provenance.json') } +/** + * `null` means "no usable provenance": genuinely absent, or present but + * malformed, where rebuilding it IS the intent. It deliberately does NOT cover + * a file that could not be read — see `provenanceIsUnreadable`. + */ function readHookTrustProvenance( runtimeHomePath: string ): Map | null { const provenancePath = getProvenancePath(runtimeHomePath) - if (!existsSync(provenancePath)) { + let rawProvenance: string + try { + rawProvenance = readFileSync(provenancePath, 'utf-8') + } catch { return null } try { - const parsed: unknown = JSON.parse(readFileSync(provenancePath, 'utf-8')) + const parsed: unknown = JSON.parse(rawProvenance) if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) { return null } @@ -79,9 +88,29 @@ function readHookTrustProvenance( * install/refresh, so the next launch can tell "entry Orca wrote" apart from * "entry Codex wrote after a user approval". Call after all trust writes. */ +/** + * Why: this record is the only thing that tells a later launch which + * `config.toml` trust entries Orca wrote apart from which the user approved + * inside Codex. Overwriting it from the current config state after a failed + * read stamps the user's approval as Orca-written, and promotion then skips it + * forever — a permanent loss from one unreadable file. Keep the old record and + * let the next pass, which can read it, do the comparison. + */ +function provenanceIsUnreadable(provenancePath: string): boolean { + try { + readFileSync(provenancePath, 'utf-8') + return false + } catch (error) { + return !isDefinitiveAbsence(error) + } +} + export function snapshotCodexRuntimeHookTrustProvenance( runtimeHomePath: string = getOrcaManagedCodexHomePath() ): void { + if (provenanceIsUnreadable(getProvenancePath(runtimeHomePath))) { + return + } try { const runtimeHooksPath = join(runtimeHomePath, 'hooks.json') const canonicalRuntimeHooksPath = getCodexExplicitHomeHookSourcePath(runtimeHooksPath) diff --git a/src/main/codex/sta-4735-hook-trust-provenance-overwrite.test.ts b/src/main/codex/sta-4735-hook-trust-provenance-overwrite.test.ts new file mode 100644 index 00000000000..99997e1e961 --- /dev/null +++ b/src/main/codex/sta-4735-hook-trust-provenance-overwrite.test.ts @@ -0,0 +1,151 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type * as NodeFs from 'node:fs' +import type * as NodeOs from 'node:os' +import { tmpdir } from 'node:os' +import { join } from 'node:path' + +// STA-4735: the hook-trust provenance file is the only record of which +// config.toml trust entries Orca wrote versus which the user approved inside +// Codex. An unreadable one was rebuilt from the current config on the same +// pass, stamping the user's approval as Orca-written — after which promotion +// skips it forever. + +const denials = vi.hoisted(() => { + const state = { + paths: new Set(), + deny(path: string): void { + state.paths.add(path) + }, + reset(): void { + state.paths.clear() + }, + check(target: unknown, syscall: string): void { + if (typeof target !== 'string' || !state.paths.has(target)) { + return + } + const error: NodeJS.ErrnoException = new Error( + `EPERM: operation not permitted, ${syscall} '${target}'` + ) + error.code = 'EPERM' + error.errno = -4048 + error.syscall = syscall + error.path = target + throw error + } + } + return state +}) + +vi.mock('node:fs', async (importOriginal) => { + const actual = await importOriginal() + const guard = (fn: unknown, syscall: string): unknown => { + const original = fn as (...args: unknown[]) => unknown + const wrapped = (...args: unknown[]): unknown => { + denials.check(args[0], syscall) + return original(...args) + } + return Object.assign(wrapped, original) + } + const patched: Record = { + ...actual, + readFileSync: guard(actual.readFileSync, 'read'), + existsSync: Object.assign( + (...args: unknown[]): boolean => + typeof args[0] === 'string' && denials.paths.has(args[0]) + ? false + : actual.existsSync(args[0] as string), + actual.existsSync + ) + } + return { ...patched, default: patched } +}) + +const { getPathMock, homedirMock } = vi.hoisted(() => ({ + getPathMock: vi.fn<(name: string) => string>(), + homedirMock: vi.fn<() => string>() +})) + +vi.mock('electron', () => ({ app: { getPath: getPathMock } })) + +vi.mock('node:os', async () => { + const actual = await vi.importActual('node:os') + return { ...actual, homedir: homedirMock } +}) + +const realFs = await vi.importActual('node:fs') +const { snapshotCodexRuntimeHookTrustProvenance } = await import('./hook-trust-promotion') + +const PROVENANCE_ENTRY = 'orca-hooks:stop:0:0' +let fakeHomeDir: string +let userDataDir: string +let runtimeHomePath: string + +const provenancePath = (): string => join(runtimeHomePath, '.orca-hook-trust-provenance.json') + +function seedRecordedProvenance(): string { + const contents = `${JSON.stringify( + { version: 1, entries: { [PROVENANCE_ENTRY]: { trustedHash: 'sha256:orca', enabled: true } } }, + null, + 2 + )}\n` + realFs.writeFileSync(provenancePath(), contents, 'utf-8') + return contents +} + +beforeEach(() => { + denials.reset() + fakeHomeDir = realFs.mkdtempSync(join(tmpdir(), 'orca-sta4735-home-')) + userDataDir = realFs.mkdtempSync(join(tmpdir(), 'orca-sta4735-data-')) + runtimeHomePath = join(userDataDir, 'codex-runtime-home', 'home') + homedirMock.mockReturnValue(fakeHomeDir) + getPathMock.mockImplementation((name: string) => { + if (name === 'userData') { + return userDataDir + } + throw new Error(`unexpected app.getPath(${name})`) + }) + realFs.mkdirSync(runtimeHomePath, { recursive: true }) + realFs.mkdirSync(join(fakeHomeDir, '.codex'), { recursive: true }) + realFs.writeFileSync(join(runtimeHomePath, 'config.toml'), 'model = "m"\n', 'utf-8') +}) + +afterEach(() => { + denials.reset() + realFs.rmSync(fakeHomeDir, { recursive: true, force: true }) + realFs.rmSync(userDataDir, { recursive: true, force: true }) + vi.clearAllMocks() +}) + +describe('STA-4735 snapshotCodexRuntimeHookTrustProvenance', () => { + it('leaves a provenance record it could not read exactly as it found it', () => { + const recorded = seedRecordedProvenance() + denials.deny(provenancePath()) + + snapshotCodexRuntimeHookTrustProvenance(runtimeHomePath) + + // Before the fix this rewrote the file from the current config.toml, which + // holds no record of what Orca wrote — so a user approval made since the + // last pass was permanently reclassified as Orca's own write. + expect(realFs.readFileSync(provenancePath(), 'utf-8')).toBe(recorded) + }) + + it('still records a provenance file that does not exist yet', () => { + // Why: writing the first snapshot is this function's whole job. A fix that + // refused whenever the read failed would never seed one. + snapshotCodexRuntimeHookTrustProvenance(runtimeHomePath) + + expect(realFs.existsSync(provenancePath())).toBe(true) + expect(JSON.parse(realFs.readFileSync(provenancePath(), 'utf-8'))).toMatchObject({ version: 1 }) + }) + + it('still replaces a provenance file that is present but malformed', () => { + // Why: a corrupt record carries no information, and resetting it IS the + // intent. Only the unreadable case may be preserved — conflating the two + // would wedge a user on a broken file forever. + realFs.writeFileSync(provenancePath(), '{ not json', 'utf-8') + + snapshotCodexRuntimeHookTrustProvenance(runtimeHomePath) + + expect(JSON.parse(realFs.readFileSync(provenancePath(), 'utf-8'))).toMatchObject({ version: 1 }) + }) +})