mirror of
https://github.com/stablyai/orca.git
synced 2026-10-23 16:03:49 +00:00
fix(codex): stop an unreadable legacy hooks.json from clearing managed trust
`cleanupLegacySystemManagedHooks` reads `~/.codex/hooks.json` and, when it finds
no hooks, removes Orca's managed trust entries from the system config.toml and
deletes that home's grant-ledger record.
Since the STA-4823 read classification landed, `readHooksJsonWithRaw` reports a
genuine absence as `{ raw: null, config: {} }` and a failed read as
`{ raw: null, config: null }`. Both still fell into the same branch, so a read
that merely failed discarded hook approvals the user had already given — and the
ledger record that would have let a later pass notice.
Only the definitive-absence answer may reach the removal now.
Found while reviewing #15417 and not covered by it: that PR fixed the classifier
and this is a consumer of it that still collapsed the two answers.
The sibling `assertHooksJsonGeneration` guard in codex-real-home-hook-install.ts
has the same `existsSync ? read : null` shape and is deliberately NOT changed
here. Measured, a permission denial leaves existsSync true and throws from the
read, so that path already fails closed; a guard there could not be made to fail
in a test and would be unprovable code.
This commit is contained in:
@@ -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 type * as CodexManagedTrustReconciliation from './codex-managed-trust-reconciliation'
|
||||
import { tmpdir } from 'node:os'
|
||||
import { join } from 'node:path'
|
||||
|
||||
// Follow-up to STA-4823: two more reads on the Codex hook paths acted on
|
||||
// "absent" when the real answer was "could not read". Both were found while
|
||||
// reviewing #15417 and are not covered by it.
|
||||
//
|
||||
// The denial shape here is the MEASURED one: chmod 000 / icacls deny leave
|
||||
// existsSync true and stat succeeding, and fail only the content read
|
||||
// (EACCES -13 on macOS, EPERM -4048 on Windows). No mode fakes existsSync,
|
||||
// because no platform does.
|
||||
|
||||
const denials = vi.hoisted(() => {
|
||||
const state = {
|
||||
paths: new Set<string>(),
|
||||
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(
|
||||
`EACCES: permission denied, ${syscall} '${target}'`
|
||||
)
|
||||
error.code = 'EACCES'
|
||||
error.errno = -13
|
||||
error.syscall = syscall
|
||||
error.path = target
|
||||
throw error
|
||||
}
|
||||
}
|
||||
return state
|
||||
})
|
||||
|
||||
vi.mock('node:fs', async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof NodeFs>()
|
||||
const guardRead = (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<string, unknown> = {
|
||||
...actual,
|
||||
readFileSync: guardRead(actual.readFileSync, 'read'),
|
||||
// Why: existsSync and stat stay HEALTHY. That is the whole point — a
|
||||
// permission denial does not hide the file, it only refuses its contents.
|
||||
openSync: Object.assign((...args: unknown[]): unknown => {
|
||||
const flags = args[1]
|
||||
if (flags === undefined || (typeof flags === 'string' && flags.startsWith('r'))) {
|
||||
denials.check(args[0], 'open')
|
||||
}
|
||||
return (actual.openSync as (...a: unknown[]) => unknown)(...args)
|
||||
}, actual.openSync)
|
||||
}
|
||||
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<typeof NodeOs>('node:os')
|
||||
return { ...actual, homedir: homedirMock }
|
||||
})
|
||||
|
||||
const realFs = await vi.importActual<typeof NodeFs>('node:fs')
|
||||
|
||||
let fakeHomeDir: string
|
||||
let userDataDir: string
|
||||
let previousUserDataPath: string | undefined
|
||||
|
||||
beforeEach(() => {
|
||||
denials.reset()
|
||||
fakeHomeDir = realFs.mkdtempSync(join(tmpdir(), 'orca-hooksread-home-'))
|
||||
userDataDir = realFs.mkdtempSync(join(tmpdir(), 'orca-hooksread-data-'))
|
||||
previousUserDataPath = process.env.ORCA_USER_DATA_PATH
|
||||
process.env.ORCA_USER_DATA_PATH = userDataDir
|
||||
homedirMock.mockReturnValue(fakeHomeDir)
|
||||
getPathMock.mockImplementation((name: string) => {
|
||||
if (name === 'userData') {
|
||||
return userDataDir
|
||||
}
|
||||
throw new Error(`unexpected app.getPath(${name})`)
|
||||
})
|
||||
realFs.mkdirSync(join(fakeHomeDir, '.codex'), { recursive: true })
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
denials.reset()
|
||||
realFs.rmSync(fakeHomeDir, { recursive: true, force: true })
|
||||
realFs.rmSync(userDataDir, { recursive: true, force: true })
|
||||
if (previousUserDataPath === undefined) {
|
||||
delete process.env.ORCA_USER_DATA_PATH
|
||||
} else {
|
||||
process.env.ORCA_USER_DATA_PATH = previousUserDataPath
|
||||
}
|
||||
vi.clearAllMocks()
|
||||
})
|
||||
|
||||
describe('an unreadable legacy hooks.json must not clear managed trust or its ledger', () => {
|
||||
it('returns before the removal branch instead of discarding approvals', async () => {
|
||||
const legacyHooksPath = join(fakeHomeDir, '.codex', 'hooks.json')
|
||||
realFs.writeFileSync(legacyHooksPath, JSON.stringify({ hooks: { Stop: [] } }), 'utf-8')
|
||||
|
||||
const removeSpy = vi.fn()
|
||||
vi.resetModules()
|
||||
vi.doMock('./codex-managed-trust-reconciliation', async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof CodexManagedTrustReconciliation>()
|
||||
return { ...actual, removeCodexManagedHookTrustEntries: removeSpy }
|
||||
})
|
||||
try {
|
||||
// The removal only fires when a real-home grant is on record — without
|
||||
// this the branch is inert and the test proves nothing either way.
|
||||
const ledger = await import('./codex-trust-grant-ledger')
|
||||
ledger.writeCodexTrustGrantLedgerHome(
|
||||
join(fakeHomeDir, '.codex'),
|
||||
{ entries: { 'seed:1': { trustedHash: 'sha256:seed' } } } as never,
|
||||
ledger.getCodexTrustGrantLedgerPath()
|
||||
)
|
||||
|
||||
const hookService = await import('./hook-service')
|
||||
denials.deny(legacyHooksPath)
|
||||
|
||||
hookService._internals.cleanupLegacySystemManagedHooks()
|
||||
|
||||
// `readHooksJsonWithRaw` catches the denial and reports {raw:null,
|
||||
// config:null} — "could not read". Before the fix that fell into the same
|
||||
// branch as "no hooks configured", which removes the managed trust
|
||||
// entries AND their grant-ledger record. A read that failed is not
|
||||
// permission to discard approvals the user already gave.
|
||||
expect(removeSpy).not.toHaveBeenCalled()
|
||||
} finally {
|
||||
vi.doUnmock('./codex-managed-trust-reconciliation')
|
||||
vi.resetModules()
|
||||
}
|
||||
})
|
||||
})
|
||||
@@ -601,6 +601,13 @@ function cleanupLegacySystemManagedHooks(): void {
|
||||
// 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)
|
||||
// Why: `config === null` with no raw is the "could not read" answer, not the
|
||||
// "no hooks here" one — the branch below removes managed trust entries AND
|
||||
// their grant-ledger record, so acting on it would discard approvals over a
|
||||
// read that merely failed. A genuine absence still returns `config: {}`.
|
||||
if (config === null && previousRaw === null) {
|
||||
return
|
||||
}
|
||||
if (!config?.hooks || previousRaw === null) {
|
||||
if (hasRecordedRealHomeGrant) {
|
||||
removeSystemManagedHookTrustEntries(systemHomePath, legacyConfigPath)
|
||||
@@ -1637,6 +1644,7 @@ export class CodexHookService {
|
||||
export const codexHookService = new CodexHookService()
|
||||
|
||||
export const _internals = {
|
||||
cleanupLegacySystemManagedHooks,
|
||||
getManagedScript,
|
||||
installManagedHooksIntoWslRuntime,
|
||||
refreshWslRuntimeUserHooks,
|
||||
|
||||
Reference in New Issue
Block a user