From d76d955dec061ebc455828bb87f81e5d1d809161 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 17 Jul 2026 00:36:06 -0700 Subject: [PATCH] fix(codex): snapshot hooks.json bytes+parse in one read to close real-home clobber race The install/sweep/legacy-cleanup paths parsed hooks.json, then did a separate later read to capture the previous bytes for the pre-write generation guard. A concurrent save (second Orca instance or the user editing the file) could land between the parse and that second read and be silently overwritten. readHooksJsonWithRaw returns the raw bytes and parse from a single read so the guard compares against exactly what it parsed. Adds a regression test that mutates hooks.json mid-RPC and asserts the sweep aborts without clobbering. --- config/tsconfig.cli.json | 1 + src/main/agent-hooks/hooks-json-read.ts | 37 +++++++++++++++++ src/main/agent-hooks/installer-utils.test.ts | 23 +++++++++++ src/main/agent-hooks/installer-utils.ts | 22 +++------- .../codex-real-home-hook-install.test.ts | 41 +++++++++++++++++++ .../codex/codex-real-home-hook-install.ts | 15 ++++--- src/main/codex/hook-service.ts | 8 ++-- 7 files changed, 122 insertions(+), 25 deletions(-) create mode 100644 src/main/agent-hooks/hooks-json-read.ts diff --git a/config/tsconfig.cli.json b/config/tsconfig.cli.json index 3ebd71aa41f..11020ab0206 100644 --- a/config/tsconfig.cli.json +++ b/config/tsconfig.cli.json @@ -5,6 +5,7 @@ "../src/shared/**/*", "../src/main/agent-hooks/hook-stdin-contract.ts", "../src/main/agent-hooks/hook-config-write-path.ts", + "../src/main/agent-hooks/hooks-json-read.ts", "../src/main/agent-hooks/installer-utils.ts", "../src/main/agent-hooks/installer-utils-remote.ts", "../src/main/agent-hooks/managed-agent-hook-controls.ts", diff --git a/src/main/agent-hooks/hooks-json-read.ts b/src/main/agent-hooks/hooks-json-read.ts new file mode 100644 index 00000000000..c8536fad0b4 --- /dev/null +++ b/src/main/agent-hooks/hooks-json-read.ts @@ -0,0 +1,37 @@ +import { existsSync, readFileSync } from 'node:fs' +import type { HooksConfig } from './installer-utils' + +export function isPlainObject(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value) +} + +export type HooksJsonSnapshot = { + /** null when the file does not exist or could not be read. */ + raw: string | null + config: HooksConfig | null +} + +// Why: generation guards abort a mutation when the file no longer matches the +// bytes it was derived from; the raw snapshot and the parse must come from one +// read or a concurrent save can slip between them unnoticed. +export function readHooksJsonWithRaw(configPath: string): HooksJsonSnapshot { + if (!existsSync(configPath)) { + return { raw: null, config: {} } + } + let raw: string + try { + raw = readFileSync(configPath, 'utf-8') + } catch { + return { raw: null, config: null } + } + try { + const parsed = JSON.parse(raw) + return { raw, config: isPlainObject(parsed) ? parsed : null } + } catch { + return { raw, config: null } + } +} + +export function readHooksJson(configPath: string): HooksConfig | null { + return readHooksJsonWithRaw(configPath).config +} diff --git a/src/main/agent-hooks/installer-utils.test.ts b/src/main/agent-hooks/installer-utils.test.ts index 7fac297ae86..466792009c7 100644 --- a/src/main/agent-hooks/installer-utils.test.ts +++ b/src/main/agent-hooks/installer-utils.test.ts @@ -25,6 +25,7 @@ import { wrapPosixHookCommand, wrapWindowsCmdHookCommand, wrapWindowsGitBashHookCommand, + readHooksJsonWithRaw, wrapWindowsHookCommand, writeManagedScript, writeHooksJson, @@ -43,6 +44,28 @@ afterEach(() => { rmSync(tmpDir, { recursive: true, force: true }) }) +describe('readHooksJsonWithRaw', () => { + it('returns the parsed config together with the exact bytes it came from', () => { + const contents = '{"hooks": {"Stop": []}, "custom": 1}\n' + writeFileSync(configPath, contents, 'utf-8') + + expect(readHooksJsonWithRaw(configPath)).toEqual({ + raw: contents, + config: { hooks: { Stop: [] }, custom: 1 } + }) + }) + + it('reports a missing file as an empty config with no raw bytes', () => { + expect(readHooksJsonWithRaw(configPath)).toEqual({ raw: null, config: {} }) + }) + + it('keeps the raw bytes when the contents are not a JSON object', () => { + writeFileSync(configPath, 'not json\n', 'utf-8') + + expect(readHooksJsonWithRaw(configPath)).toEqual({ raw: 'not json\n', config: null }) + }) +}) + describe('writeHooksJson', () => { it('updates a symlink target without replacing the hook config link', () => { const targetPath = join(tmpDir, 'dotfiles-hooks.json') diff --git a/src/main/agent-hooks/installer-utils.ts b/src/main/agent-hooks/installer-utils.ts index 63cadabe534..c73e1536e4f 100644 --- a/src/main/agent-hooks/installer-utils.ts +++ b/src/main/agent-hooks/installer-utils.ts @@ -63,22 +63,12 @@ export function buildManagedCommandDefinition(command: string): HookDefinition { return { command, timeout: MANAGED_HOOK_TIMEOUT_SECONDS } } -export function isPlainObject(value: unknown): value is Record { - return typeof value === 'object' && value !== null && !Array.isArray(value) -} - -export function readHooksJson(configPath: string): HooksConfig | null { - if (!existsSync(configPath)) { - return {} - } - - try { - const parsed = JSON.parse(readFileSync(configPath, 'utf-8')) - return isPlainObject(parsed) ? parsed : null - } catch { - return null - } -} +export { + isPlainObject, + readHooksJson, + readHooksJsonWithRaw, + type HooksJsonSnapshot +} from './hooks-json-read' // Why: callers in install/remove need to match not just the exact current // managed command, but also stale entries pointing at old script paths — e.g. diff --git a/src/main/codex/codex-real-home-hook-install.test.ts b/src/main/codex/codex-real-home-hook-install.test.ts index 6fdf852bf6b..d20730a4d51 100644 --- a/src/main/codex/codex-real-home-hook-install.test.ts +++ b/src/main/codex/codex-real-home-hook-install.test.ts @@ -88,6 +88,7 @@ beforeEach(() => { afterEach(() => { rebaseInternals.setSessionRunnerSync(null) + rebaseInternals.resetRetryState() rmSync(fakeHomeDir, { recursive: true, force: true }) rmSync(userDataDir, { recursive: true, force: true }) if (previousUserDataPath === undefined) { @@ -405,6 +406,46 @@ describe('ensureRealHomeCodexHookState (opt-out sweep)', () => { expect(readRealHooksJson().hooks?.Stop).toEqual([{ hooks: [before] }, { hooks: [after] }]) }) + it('aborts without writing when hooks.json changes during the trust inspection', () => { + grantSucceeds() + const before = { type: 'command', command: 'before.sh' } + writeFileSync( + getRealHooksJsonPath(), + `${JSON.stringify({ hooks: { Stop: [{ hooks: [before] }] } }, null, 2)}\n` + ) + ensureRealHomeCodexHookState({ hooksEnabled: true, userDataPath: userDataDir }) + const installed = readRealHooksJson() + const after = { type: 'command', command: 'after.sh' } + installed.hooks!.Stop!.push({ hooks: [after] }) + writeFileSync(getRealHooksJsonPath(), `${JSON.stringify(installed, null, 2)}\n`) + const userTrustToml = '[hooks.state."x:stop:0:0"]\ntrusted_hash = "user"\n' + writeFileSync(getRealConfigTomlPath(), userTrustToml, 'utf-8') + const concurrentSave = `${JSON.stringify({ hooks: { Stop: [{ hooks: [before] }] } }, null, 2)}\n` + const operations: string[] = [] + rebaseInternals.setSessionRunnerSync((request) => { + operations.push(request.operation) + // A user save (or a second Orca instance) lands while the RPC runs. + writeFileSync(getRealHooksJsonPath(), concurrentSave, 'utf-8') + return { + outcome: 'inspected', + moves: request.moves.map((move) => ({ + ...move, + reportedOldKey: move.oldKey, + wasTrusted: true, + enabled: true + })) + } + }) + + expect(ensureRealHomeCodexHookState({ hooksEnabled: false, userDataPath: userDataDir })).toBe( + 'unavailable' + ) + + expect(operations).toEqual(['inspect-user-hook-trust']) + expect(readFileSync(getRealHooksJsonPath(), 'utf-8')).toBe(concurrentSave) + expect(readFileSync(getRealConfigTomlPath(), 'utf-8')).toBe(userTrustToml) + }) + it('removes only Orca entries and reports the removed lane', () => { grantSucceeds() const userStop = { diff --git a/src/main/codex/codex-real-home-hook-install.ts b/src/main/codex/codex-real-home-hook-install.ts index 30c0704734a..24e295560d2 100644 --- a/src/main/codex/codex-real-home-hook-install.ts +++ b/src/main/codex/codex-real-home-hook-install.ts @@ -5,7 +5,7 @@ import { buildManagedCommandHook, createManagedCommandMatcher, MANAGED_HOOK_TIMEOUT_SECONDS, - readHooksJson, + readHooksJsonWithRaw, removeManagedCommands, writeHooksJson, writeManagedScript, @@ -119,7 +119,10 @@ function installRealHomeCodexHook(userDataPath: string): RealHomeCodexHookLane { const material = getCodexManagedHookInstallMaterial() const hooksJsonPath = getRealHomeHooksJsonPath() const hooksWritePath = resolveHooksJsonWritePath(hooksJsonPath) - const config = readHooksJson(hooksJsonPath) + // Why: the generation guard compares against these bytes before writing; a + // separate later read would let a concurrent save land between parse and + // snapshot and be silently overwritten by the stale parse. + const { raw: previousRaw, config } = readHooksJsonWithRaw(hooksJsonPath) if (!config) { // Why: an unparseable user file must never be clobbered; without a hook // entry the managed lane keeps status working for this host. @@ -168,7 +171,6 @@ function installRealHomeCodexHook(userDataPath: string): RealHomeCodexHookLane { } } - const previousRaw = existsSync(hooksJsonPath) ? readFileSync(hooksJsonPath, 'utf-8') : null const previousMode = previousRaw === null ? undefined : statSync(hooksWritePath).mode backupRealHomeHooksJsonOnce(userDataPath, previousRaw) // Why: unknown top-level fields belong to the user (other managers' @@ -268,8 +270,10 @@ function getInstallRetryAfterMs(reason: CodexTrustGrantFallbackReason): number { function sweepRealHomeCodexHook(): RealHomeCodexHookLane { const hooksJsonPath = getRealHomeHooksJsonPath() - const config = readHooksJson(hooksJsonPath) - if (!config?.hooks) { + // Why: single read — the pre-write generation guard must compare against + // the exact bytes this sweep's parse came from. + const { raw: previousRaw, config } = readHooksJsonWithRaw(hooksJsonPath) + if (!config?.hooks || previousRaw === null) { return 'removed' } const isManagedCommand = createManagedCommandMatcher(getCodexManagedScriptFileName()) @@ -295,7 +299,6 @@ function sweepRealHomeCodexHook(): RealHomeCodexHookLane { } if (removedAny) { const hooksWritePath = resolveHooksJsonWritePath(hooksJsonPath) - const previousRaw = readFileSync(hooksJsonPath, 'utf-8') const previousMode = statSync(hooksWritePath).mode mutateRealHomeHooksPreservingUserTrust({ sourcePath: hooksJsonPath, diff --git a/src/main/codex/hook-service.ts b/src/main/codex/hook-service.ts index 957bfd669f1..9feec736280 100644 --- a/src/main/codex/hook-service.ts +++ b/src/main/codex/hook-service.ts @@ -11,6 +11,7 @@ import { hookDefinitionHasManagedCommand, MANAGED_HOOK_TIMEOUT_SECONDS, readHooksJson, + readHooksJsonWithRaw, removeManagedCommands, wrapPosixHookCommand, wrapWindowsCmdHookCommand, @@ -605,8 +606,10 @@ function cleanupLegacySystemManagedHooks(): void { const systemHomePath = getSystemCodexHomePath() const hasRecordedRealHomeGrant = readCodexTrustGrantLedgerHomeForReconciliation(systemHomePath) !== null - const config = readHooksJson(legacyConfigPath) - if (!config?.hooks) { + // Why: the pre-write guard below compares against these bytes; a separate + // later read would let a concurrent save land between parse and snapshot. + const { raw: previousRaw, config } = readHooksJsonWithRaw(legacyConfigPath) + if (!config?.hooks || previousRaw === null) { if (hasRecordedRealHomeGrant) { removeSystemManagedHookTrustEntries(systemHomePath, legacyConfigPath) } @@ -648,7 +651,6 @@ function cleanupLegacySystemManagedHooks(): void { // Why: this is the user's system hooks file, not Orca's runtime copy. // Remove only stale Orca hook entries and preserve other managers' metadata. const hooksWritePath = resolveHooksJsonWritePath(legacyConfigPath) - const previousRaw = readFileSync(legacyConfigPath, 'utf-8') const previousMode = statSync(hooksWritePath).mode mutateRealHomeHooksPreservingUserTrust({ sourcePath: legacyConfigPath,