From efaa61d596b3472bfd9b38d6764ce27ccd54e08a Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 17 Jul 2026 02:39:16 -0700 Subject: [PATCH] fix(codex): harden managed trust sanitization --- src/main/codex-accounts/service.test.ts | 37 ++++++++++ src/main/codex-accounts/service.ts | 22 +++--- src/main/codex/config-toml-trust.test.ts | 66 +++++++++++++++++ src/main/codex/config-toml-trust.ts | 94 +++++++++++++++++++----- 4 files changed, 193 insertions(+), 26 deletions(-) diff --git a/src/main/codex-accounts/service.test.ts b/src/main/codex-accounts/service.test.ts index 032a79668af..6b2e6ec360e 100644 --- a/src/main/codex-accounts/service.test.ts +++ b/src/main/codex-accounts/service.test.ts @@ -394,6 +394,43 @@ describe('CodexAccountService config sync', () => { expectSanitizedManagedConfig() }) + it('keeps flag-off config mirroring byte-identical', async () => { + const fixture = await createCanonicalHookTrustFixture() + const canonicalConfigPath = join(testState.fakeHomeDir, '.codex', 'config.toml') + writeFileSync(canonicalConfigPath, fixture.config, 'utf-8') + const managedHomePath = createManagedHome( + testState.userDataDir, + 'account-1', + 'approval_policy = "on-request"\n' + ) + const settings = createSettings({ + codexSystemDefaultRealHomeEnabled: false, + codexManagedAccounts: [ + { + id: 'account-1', + email: 'user@example.com', + managedHomePath, + providerAccountId: null, + workspaceLabel: null, + workspaceAccountId: null, + createdAt: 1, + updatedAt: 1, + lastAuthenticatedAt: 1 + } + ] + }) + + const { CodexAccountService } = await import('./service') + new CodexAccountService( + createStore(settings) as never, + createRateLimits() as never, + createRuntimeHome() as never + ) + + expect(readFileSync(join(managedHomePath, 'config.toml'), 'utf-8')).toBe(fixture.config) + expect(readFileSync(canonicalConfigPath, 'utf-8')).toBe(fixture.config) + }) + it('rewrites relative path config values when syncing into managed homes', async () => { const canonicalConfigPath = join(testState.fakeHomeDir, '.codex', 'config.toml') writeFileSync( diff --git a/src/main/codex-accounts/service.ts b/src/main/codex-accounts/service.ts index ff56fb4d036..0fecd8e923d 100644 --- a/src/main/codex-accounts/service.ts +++ b/src/main/codex-accounts/service.ts @@ -17,6 +17,7 @@ import type { CodexRuntimeHomeService } from './runtime-home-service' import { writeFileAtomically } from './fs-utils' import { rewriteRelativePathConfigValues } from '../codex/codex-config-path-reference-rewrite' import { stripCodexManagedHookTrustEntriesFromConfig } from '../codex/codex-managed-trust-reconciliation' +import { isCodexSystemDefaultRealHomeEnabled } from '../codex/codex-real-home-flag' import { getCodexManagedHookInstallMaterial } from '../codex/hook-service' import { MANAGED_HOOK_TIMEOUT_SECONDS } from '../agent-hooks/installer-utils' import { resolveCodexCommand } from '../codex-cli/command' @@ -472,15 +473,18 @@ export class CodexAccountService { // account while preserving consistent Codex behavior. Managed homes are // real CODEX_HOMEs for `codex login`, so relative path-valued settings // must keep resolving against the home the config was read from. - const material = getCodexManagedHookInstallMaterial() - // Why: source-home Orca trust is foreign to each managed home's hooks.json. - const sanitizedConfig = stripCodexManagedHookTrustEntriesFromConfig(canonicalConfig.contents, { - runtimeHomePath: canonicalConfig.sourceHomePath, - sourcePath: canonicalConfig.sourceHooksPath, - command: material.command, - managedEventLabels: new Set(Object.values(material.eventLabel)), - timeoutSec: MANAGED_HOOK_TIMEOUT_SECONDS - }) + let sanitizedConfig = canonicalConfig.contents + if (isCodexSystemDefaultRealHomeEnabled(this.store.getSettings())) { + const material = getCodexManagedHookInstallMaterial() + // Why: source-home Orca trust is foreign to each managed home's hooks.json. + sanitizedConfig = stripCodexManagedHookTrustEntriesFromConfig(canonicalConfig.contents, { + runtimeHomePath: canonicalConfig.sourceHomePath, + sourcePath: canonicalConfig.sourceHooksPath, + command: material.command, + managedEventLabels: new Set(Object.values(material.eventLabel)), + timeoutSec: MANAGED_HOOK_TIMEOUT_SECONDS + }) + } this.writeManagedConfig( trustedManagedHomePath, rewriteRelativePathConfigValues(sanitizedConfig, canonicalConfig.sourceHomePath) diff --git a/src/main/codex/config-toml-trust.test.ts b/src/main/codex/config-toml-trust.test.ts index e36e7a1a8d7..2ee270e0cbc 100644 --- a/src/main/codex/config-toml-trust.test.ts +++ b/src/main/codex/config-toml-trust.test.ts @@ -26,7 +26,9 @@ import { normalizeCodexProjectPathForRevocationLookup, parseTrustKey, readHookTrustEntries, + readHookTrustEntriesFromContent, removeHookTrustEntries, + removeHookTrustEntriesFromContent, upsertHookTrustEntries, upsertProjectTrustLevel, upsertProjectTrustLevelInContent, @@ -1664,6 +1666,70 @@ describe('readHookTrustEntries', () => { expect(result.get(keyB)?.enabled).toBe(true) }) + it('fails closed when normalized duplicate blocks have conflicting hashes', () => { + const key = '/x/hooks.json:stop:0:0' + writeFileSync( + configPath, + [ + `[hooks.state."${key}"]`, + 'trusted_hash = "sha256:USER"', + '', + `[hooks.state.'${key}']`, + 'trusted_hash = "sha256:ORCA"', + '' + ].join('\n'), + 'utf-8' + ) + + expect(readHookTrustEntries(configPath).get(key)?.trustedHash).toBeUndefined() + }) + + it('ignores trust-looking fields inside multiline strings', () => { + const key = '/x/hooks.json:stop:0:0' + writeFileSync( + configPath, + [ + `[hooks.state."${key}"]`, + 'note = """', + 'trusted_hash = "sha256:NOT-A-FIELD"', + 'enabled = false', + '"""', + '' + ].join('\n'), + 'utf-8' + ) + + expect(readHookTrustEntries(configPath).get(key)).toEqual({ + trustedHash: undefined, + enabled: undefined + }) + }) + + it('does not accept an unterminated trusted_hash string', () => { + const key = '/x/hooks.json:stop:0:0' + writeFileSync( + configPath, + `[hooks.state."${key}"]\ntrusted_hash = "sha256:UNTERMINATED\n`, + 'utf-8' + ) + + expect(readHookTrustEntries(configPath).get(key)?.trustedHash).toBeUndefined() + }) + + it('recognizes and removes a first trust block after a leading BOM', () => { + const key = '/x/hooks.json:stop:0:0' + const content = [ + `\uFEFF[hooks.state."${key}"]`, + 'trusted_hash = "sha256:ORCA"', + '[other]', + 'value = true', + '' + ].join('\n') + + expect(readHookTrustEntriesFromContent(content).get(key)?.trustedHash).toBe('sha256:ORCA') + expect(removeHookTrustEntriesFromContent(content, [key])).toBe('[other]\nvalue = true\n') + }) + it('does not let triple quotes in comments hide later trust entries', () => { const key = '/x/hooks.json:pre_tool_use:0:0' const original = [ diff --git a/src/main/codex/config-toml-trust.ts b/src/main/codex/config-toml-trust.ts index 28445d40490..aa8bcec2497 100644 --- a/src/main/codex/config-toml-trust.ts +++ b/src/main/codex/config-toml-trust.ts @@ -599,18 +599,34 @@ export function normalizeHookTrustKeyForLookup(key: string): string { } function findTrustBlockRanges(content: string, key: string): TrustBlockRange[] { + return findTrustBlockRangesForNormalizedKeys( + content, + new Set([normalizeHookTrustKeyForLookup(key)]) + ) +} + +function findTrustBlockRangesForNormalizedKeys( + content: string, + normalizedKeys: ReadonlySet +): TrustBlockRange[] { const ranges: TrustBlockRange[] = [] - const normalizedKey = normalizeHookTrustKeyForLookup(key) + if (normalizedKeys.size === 0) { + return ranges + } let cursor = 0 let scanState = createTomlLineScanState() while (cursor < content.length) { const newlineIdx = content.indexOf('\n', cursor) const lineEnd = newlineIdx === -1 ? content.length : newlineIdx const rawLine = content.slice(cursor, lineEnd) - const line = rawLine.replace(/\r$/, '') + const lineWithoutCr = rawLine.replace(/\r$/, '') + const line = + cursor === 0 && lineWithoutCr.charCodeAt(0) === 0xfeff + ? lineWithoutCr.slice(1) + : lineWithoutCr const nextCursor = newlineIdx === -1 ? content.length : newlineIdx + 1 const headerKey = isTomlStructuralLine(scanState) ? parseHookStateHeaderKey(line) : null - if (headerKey !== null && normalizeHookTrustKeyForLookup(headerKey) === normalizedKey) { + if (headerKey !== null && normalizedKeys.has(normalizeHookTrustKeyForLookup(headerKey))) { const headerLineEnd = rawLine.endsWith('\r') ? lineEnd - 1 : lineEnd const after = content.slice(headerLineEnd) const nextHeaderRel = findNextTableHeader(after) @@ -887,11 +903,8 @@ export function removeHookTrustEntriesFromContent( content: string, keys: readonly string[] ): string { - return keys.reduce((updated, key) => removeTrustBlock(updated, key), content) -} - -function removeTrustBlock(content: string, key: string): string { - const ranges = findTrustBlockRanges(content, key) + const normalizedKeys = new Set(keys.map(normalizeHookTrustKeyForLookup)) + const ranges = findTrustBlockRangesForNormalizedKeys(content, normalizedKeys) if (ranges.length === 0) { return content } @@ -912,8 +925,39 @@ export function readHookTrustEntries(configPath: string): Map + enabled?: boolean +} { + const trustedHashes = new Set() + let enabled: boolean | undefined + let cursor = 0 + let scanState = createTomlLineScanState() + while (cursor < block.length) { + const newlineIdx = block.indexOf('\n', cursor) + const lineEnd = newlineIdx === -1 ? block.length : newlineIdx + const line = block.slice(cursor, lineEnd).replace(/\r$/, '') + if (isTomlStructuralLine(scanState)) { + const hashMatch = /^[ \t]*trusted_hash[ \t]*=[ \t]*"((?:[^"\\]|\\.)*)"[ \t]*(?:#.*)?$/.exec( + line + ) + if (hashMatch) { + trustedHashes.add(unescapeTomlString(hashMatch[1])) + } + const enabledMatch = /^[ \t]*enabled[ \t]*=[ \t]*(true|false)[ \t]*(?:#.*)?$/.exec(line) + if (enabledMatch) { + enabled = enabled !== false && enabledMatch[1] === 'true' + } + } + scanState = updateTomlLineScanState(scanState, line) + cursor = newlineIdx === -1 ? block.length : newlineIdx + 1 + } + return { trustedHashes, enabled } +} + export function readHookTrustEntriesFromContent(content: string): Map { const result = new HookTrustEntryMap() + const conflictingTrustedHashKeys = new Set() // Why: walk line-by-line so `[hooks.state."..."]` inside a `"""..."""` or // `'''...'''` multi-line string isn't mistaken for a real header. let cursor = 0 @@ -922,7 +966,11 @@ export function readHookTrustEntriesFromContent(content: string): Map 1 || + (trustedHash !== undefined && + existingState?.trustedHash !== undefined && + existingState.trustedHash !== trustedHash) + ) { + // Why: conflicting normalized duplicates are malformed and cannot prove + // which block is owned, so trust cleanup must preserve them all. + conflictingTrustedHashKeys.add(normalizedKey) + } result.set(normalizedKey, { - trustedHash: hashMatch ? unescapeTomlString(hashMatch[1]) : existingState?.trustedHash, + trustedHash: conflictingTrustedHashKeys.has(normalizedKey) + ? undefined + : (trustedHash ?? existingState?.trustedHash), // Why: Windows writes both slash variants for one hook; a disabled copy // must remain authoritative regardless of which variant appears last. enabled: - existingState?.enabled === false || enabled === false + existingState?.enabled === false || blockState.enabled === false ? false - : (enabled ?? existingState?.enabled) + : (blockState.enabled ?? existingState?.enabled) }) cursor = nextCursor continue