From f5cf6ea53d8503c2a41d05e0e0aa07b430ef528a Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 16 Jul 2026 01:16:23 -0700 Subject: [PATCH] fix: harden real-home hook cleanup --- .../codex-real-home-hook-install.test.ts | 40 +++++++++++++------ .../codex/codex-real-home-hook-install.ts | 6 +++ src/main/codex/config-toml-trust.test.ts | 18 +++++++++ src/main/codex/config-toml-trust.ts | 6 ++- .../codex/hook-service-trust-grant.test.ts | 21 ++++++++++ src/main/codex/hook-service.ts | 2 +- src/main/index.ts | 6 +-- 7 files changed, 82 insertions(+), 17 deletions(-) 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 0a33ae69016..b49dcf691ea 100644 --- a/src/main/codex/codex-real-home-hook-install.test.ts +++ b/src/main/codex/codex-real-home-hook-install.test.ts @@ -119,7 +119,7 @@ describe('ensureRealHomeCodexHookState (install)', () => { expect(plan.managedEntries.every((entry) => entry.groupIndex === 0)).toBe(true) }) - it('appends LAST and preserves user entries, unknown fields, and trust positions', () => { + it('keeps the managed lane for unknown top-level fields Codex cannot load', () => { grantSucceeds() const userConfig = { hooks: { @@ -128,27 +128,43 @@ describe('ensureRealHomeCodexHookState (install)', () => { }, _pluginManagerMetadata: { owner: 'someone-else' } } - writeFileSync(getRealHooksJsonPath(), `${JSON.stringify(userConfig, null, 2)}\n`, 'utf-8') + const original = `${JSON.stringify(userConfig, null, 2)}\n` + writeFileSync(getRealHooksJsonPath(), original, 'utf-8') const lane = ensureRealHomeCodexHookState({ hooksEnabled: true, userDataPath: userDataDir }) - expect(lane).toBe('installed') + expect(lane).toBe('unavailable') + expect(readFileSync(getRealHooksJsonPath(), 'utf-8')).toBe(original) + expect(grantMock).not.toHaveBeenCalled() + expect(existsSync(join(userDataDir, 'codex-real-home-hooks', 'hooks.json.pre-orca'))).toBe( + false + ) + }) + + it('appends LAST and preserves user entries and trust positions', () => { + grantSucceeds() + const userConfig = { + hooks: { + Stop: [{ matcher: 'deploy-*', hooks: [{ type: 'command', command: 'my-stop-hook.sh' }] }], + PreCompact: [{ hooks: [{ type: 'command', command: 'my-compact-hook.sh' }] }] + } + } + const original = `${JSON.stringify(userConfig, null, 2)}\n` + writeFileSync(getRealHooksJsonPath(), original, 'utf-8') + + expect(ensureRealHomeCodexHookState({ hooksEnabled: true, userDataPath: userDataDir })).toBe( + 'installed' + ) + const config = readRealHooksJson() - // User's Stop entry keeps position 0; Orca's entry is appended after it. expect(config.hooks?.Stop).toHaveLength(2) expect(config.hooks?.Stop?.[0]).toEqual(userConfig.hooks.Stop[0]) - // Non-managed events Orca does not subscribe to stay untouched. expect(config.hooks?.PreCompact).toEqual(userConfig.hooks.PreCompact) - // Unknown top-level metadata survives, unlike the managed-home writer. - expect(config._pluginManagerMetadata).toEqual(userConfig._pluginManagerMetadata) - // The appended entry's trust key uses its appended position. const plan = grantMock.mock.calls[0]![0] as CodexManagedTrustGrantPlan - const stopEntry = plan.managedEntries.find((entry) => entry.eventLabel === 'stop') - expect(stopEntry?.groupIndex).toBe(1) - // Pristine pre-Orca backup lands under userData, not in ~/.codex. + expect(plan.managedEntries.find((entry) => entry.eventLabel === 'stop')?.groupIndex).toBe(1) expect( readFileSync(join(userDataDir, 'codex-real-home-hooks', 'hooks.json.pre-orca'), 'utf-8') - ).toBe(`${JSON.stringify(userConfig, null, 2)}\n`) + ).toBe(original) }) it('updates a symlinked hooks.json target without replacing the symlink', () => { diff --git a/src/main/codex/codex-real-home-hook-install.ts b/src/main/codex/codex-real-home-hook-install.ts index d71e78a1ac9..8e122f69812 100644 --- a/src/main/codex/codex-real-home-hook-install.ts +++ b/src/main/codex/codex-real-home-hook-install.ts @@ -127,6 +127,12 @@ function installRealHomeCodexHook(userDataPath: string): RealHomeCodexHookLane { installRetryAfterMs = Date.now() + CODEX_TRUST_GRANT_TRANSIENT_RETRY_INTERVAL_MS return 'unavailable' } + if (Object.keys(config).some((key) => key !== 'hooks')) { + // Why: Codex rejects unknown root keys instead of ignoring them. Avoid a + // transient rewrite of a user-owned file that the trust RPC cannot load. + installRetryAfterMs = Date.now() + CODEX_TRUST_GRANT_TRANSIENT_RETRY_INTERVAL_MS + return 'unavailable' + } // Why: the same script the managed lane maintains; deploying here too keeps // host-connect ordering independent of the managed installer loop. diff --git a/src/main/codex/config-toml-trust.test.ts b/src/main/codex/config-toml-trust.test.ts index 9dd98e5bd3f..26ccd96cd33 100644 --- a/src/main/codex/config-toml-trust.test.ts +++ b/src/main/codex/config-toml-trust.test.ts @@ -1,6 +1,7 @@ /* eslint-disable max-lines -- Why: this suite keeps the hash fixture, TOML edit edge cases, and trust-state parser regressions together so Codex compatibility failures are easy to audit. */ import { afterEach, beforeEach, describe, expect, it } from 'vitest' import { + chmodSync, existsSync, lstatSync, mkdirSync, @@ -9,6 +10,7 @@ import { realpathSync, rmSync, symlinkSync, + statSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' @@ -1302,6 +1304,22 @@ describe('normalizeCodexProjectPathForRevocationLookup', () => { }) describe('removeHookTrustEntries', () => { + it.skipIf(process.platform === 'win32')('preserves restrictive config permissions', () => { + const entry: CodexTrustEntry = { + sourcePath: '/x/hooks.json', + eventLabel: 'stop', + groupIndex: 0, + handlerIndex: 0, + command: 'echo trusted' + } + upsertHookTrustEntries(configPath, [entry]) + chmodSync(configPath, 0o600) + + removeHookTrustEntries(configPath, [computeTrustKey(entry)]) + + expect(statSync(configPath).mode & 0o777).toBe(0o600) + }) + it('updates a symlink target without replacing config.toml', () => { const targetPath = join(tmpDir, 'dotfiles-config.toml') const entry: CodexTrustEntry = { diff --git a/src/main/codex/config-toml-trust.ts b/src/main/codex/config-toml-trust.ts index 316f089e570..4eb06612f33 100644 --- a/src/main/codex/config-toml-trust.ts +++ b/src/main/codex/config-toml-trust.ts @@ -5,6 +5,7 @@ import { mkdirSync, readFileSync, realpathSync, + statSync, unlinkSync, writeFileSync } from 'node:fs' @@ -798,9 +799,12 @@ export function writeConfigAtomically(configPath: string, contents: string): voi const dir = dirname(writePath) mkdirSync(dir, { recursive: true }) const tmpPath = join(dir, `.${Date.now()}-${randomUUID()}.tmp`) + const existingMode = existsSync(writePath) ? statSync(writePath).mode : undefined let renamed = false try { - writeFileSync(tmpPath, contents, 'utf-8') + // Why: real-home trust cleanup must not widen a dotfiles-managed config's + // restrictive permissions when the atomic rename installs new bytes. + writeFileSync(tmpPath, contents, { encoding: 'utf-8', mode: existingMode }) if (existsSync(writePath)) { copyFileWithWindowsRetry(writePath, `${writePath}.bak`) } diff --git a/src/main/codex/hook-service-trust-grant.test.ts b/src/main/codex/hook-service-trust-grant.test.ts index ac5278aebed..77f8e494ea1 100644 --- a/src/main/codex/hook-service-trust-grant.test.ts +++ b/src/main/codex/hook-service-trust-grant.test.ts @@ -1,11 +1,13 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { + chmodSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, + statSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' @@ -269,6 +271,25 @@ describe('CodexHookService app-server trust grant lane', () => { expect(operations).toEqual(['inspect-user-hook-trust', 'repair-user-hook-trust']) }) + it.skipIf(process.platform === 'win32')( + 'preserves restrictive real-home hooks permissions during flag-off cleanup', + () => { + prepareSystemHome() + const hooksPath = join(tmpHome, '.codex', 'hooks.json') + const material = getCodexManagedHookInstallMaterial() + writeFileSync( + hooksPath, + `${JSON.stringify({ hooks: { Stop: [{ hooks: [{ type: 'command', command: material.command }] }] } }, null, 2)}\n` + ) + chmodSync(hooksPath, 0o600) + installCodexLikeGrantRunner() + + expect(new CodexHookService().install().state).toBe('installed') + + expect(statSync(hooksPath).mode & 0o777).toBe(0o600) + } + ) + it('does not accept a ledger hash after the recorded Codex binary stamp changes', () => { prepareSystemHome() installCodexLikeGrantRunner() diff --git a/src/main/codex/hook-service.ts b/src/main/codex/hook-service.ts index 6da68c331ee..fbc6ded0fad 100644 --- a/src/main/codex/hook-service.ts +++ b/src/main/codex/hook-service.ts @@ -663,7 +663,7 @@ function cleanupLegacySystemManagedHooks(): void { // never replace that newer dotfiles generation with our stale parse. throw new Error('System Codex hooks changed during trust repair') } - writeHooksJson(hooksWritePath, { ...config, hooks: nextHooks }) + writeHooksJson(hooksWritePath, { ...config, hooks: nextHooks }, { preserveMode: true }) }, restoreHooks: () => writeFileAtomically(hooksWritePath, previousRaw, { mode: previousMode }) }) diff --git a/src/main/index.ts b/src/main/index.ts index 0a3bcab2848..43970bf11a8 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -1791,12 +1791,12 @@ app.whenReady().then(async () => { ) // Why: while the real-home lane owns ~/.codex/hooks.json, the legacy // system-home sweep inside managed installs would delete the entry the - // real-home installer just appended. Flag OFF or hooks off re-arms the sweep, - // which is also what removes the entry again on downgrade or opt-out. + // real-home installer just appended. Flag OFF, hooks off, or an incapable + // trust lane re-arms the sweep so downgrade, opt-out, and rollback converge. setSystemCodexHomeHookSweepSuppressed( () => codexRuntimeHome !== null && - codexRuntimeHome.isHostSystemDefaultRealHomeSelected() && + codexRuntimeHome.isHostSystemDefaultRealHome() && isAgentStatusHooksEnabled(store?.getSettings()) ) codexAccounts = new CodexAccountService(store, rateLimits, codexRuntimeHome)