mirror of
https://github.com/stablyai/orca.git
synced 2026-10-18 08:03:22 +00:00
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.
This commit is contained in:
@@ -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",
|
||||
|
||||
@@ -0,0 +1,37 @@
|
||||
import { existsSync, readFileSync } from 'node:fs'
|
||||
import type { HooksConfig } from './installer-utils'
|
||||
|
||||
export function isPlainObject(value: unknown): value is Record<string, unknown> {
|
||||
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
|
||||
}
|
||||
@@ -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')
|
||||
|
||||
@@ -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<string, unknown> {
|
||||
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.
|
||||
|
||||
@@ -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 = {
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user