fix(hooks): drain POSIX hook stdin without PATH (#10885)
Reviewed with an independent reproduction on macOS and Ubuntu 20.04. Fixed five test files that asserted the old literal strings, and switched the reader to `command -p cat` so it also survives hosts without /bin/cat.
This commit is contained in:
parent
cdd5ceb72b
commit
17fc40eae6
|
|
@ -1,6 +1,11 @@
|
|||
export type PosixHookEmptyPayloadPolicy = 'exit' | 'empty-object'
|
||||
|
||||
export const POSIX_HOOK_STDIN_DRAIN_COMMAND = 'cat >/dev/null 2>&1 || :'
|
||||
// Why: a stripped PATH must not stop a hook from consuming stdin, or the agent
|
||||
// sees exit 127 and a broken pipe mid-write (#8110). `command -p` resolves from
|
||||
// the shell's built-in default PATH, so it also survives hosts without /bin/cat
|
||||
// (NixOS) and ignores a worktree-local `cat` that could capture the payload.
|
||||
export const POSIX_HOOK_STDIN_READER = '{ command -p cat 2>/dev/null || cat; }'
|
||||
export const POSIX_HOOK_STDIN_DRAIN_COMMAND = `${POSIX_HOOK_STDIN_READER} >/dev/null 2>&1 || :`
|
||||
|
||||
// Why: every POSIX hook must own stdin before any no-op exit; sharing this
|
||||
// prelude prevents agent templates from inventing different drain semantics.
|
||||
|
|
@ -9,7 +14,12 @@ export function buildPosixHookPayloadCapture(
|
|||
): string[] {
|
||||
const emptyPayloadLines =
|
||||
emptyPayloadPolicy === 'empty-object' ? [" payload='{}'"] : [' exit 0']
|
||||
return ['payload=$(cat)', 'if [ -z "$payload" ]; then', ...emptyPayloadLines, 'fi']
|
||||
return [
|
||||
`payload=$(${POSIX_HOOK_STDIN_READER})`,
|
||||
'if [ -z "$payload" ]; then',
|
||||
...emptyPayloadLines,
|
||||
'fi'
|
||||
]
|
||||
}
|
||||
|
||||
export const WINDOWS_HOOK_STDIN_DRAIN_LABEL = 'orca_agent_hook_drain_stdin'
|
||||
|
|
|
|||
|
|
@ -31,6 +31,7 @@ import {
|
|||
writeHooksJson,
|
||||
type HooksConfig
|
||||
} from './installer-utils'
|
||||
import { POSIX_HOOK_STDIN_DRAIN_COMMAND } from './hook-stdin-contract'
|
||||
|
||||
let tmpDir: string
|
||||
let configPath: string
|
||||
|
|
@ -357,7 +358,7 @@ describe('wrapPosixHookCommand', () => {
|
|||
it('produces a guarded command that no-ops when the script is missing', () => {
|
||||
const cmd = wrapPosixHookCommand('/does/not/exist.sh')
|
||||
expect(cmd).toBe(
|
||||
"if [ -f '/does/not/exist.sh' ] && [ -r '/does/not/exist.sh' ] && [ -x '/does/not/exist.sh' ]; then /bin/sh '/does/not/exist.sh'; else cat >/dev/null 2>&1 || :; fi"
|
||||
`if [ -f '/does/not/exist.sh' ] && [ -r '/does/not/exist.sh' ] && [ -x '/does/not/exist.sh' ]; then /bin/sh '/does/not/exist.sh'; else ${POSIX_HOOK_STDIN_DRAIN_COMMAND}; fi`
|
||||
)
|
||||
})
|
||||
|
||||
|
|
@ -375,7 +376,7 @@ describe('wrapPosixHookCommand', () => {
|
|||
// /bin/sh as a single argument.
|
||||
const cmd = wrapPosixHookCommand("/path/with'quote/x.sh")
|
||||
expect(cmd).toBe(
|
||||
"if [ -f '/path/with'\\''quote/x.sh' ] && [ -r '/path/with'\\''quote/x.sh' ] && [ -x '/path/with'\\''quote/x.sh' ]; then /bin/sh '/path/with'\\''quote/x.sh'; else cat >/dev/null 2>&1 || :; fi"
|
||||
`if [ -f '/path/with'\\''quote/x.sh' ] && [ -r '/path/with'\\''quote/x.sh' ] && [ -x '/path/with'\\''quote/x.sh' ]; then /bin/sh '/path/with'\\''quote/x.sh'; else ${POSIX_HOOK_STDIN_DRAIN_COMMAND}; fi`
|
||||
)
|
||||
})
|
||||
|
||||
|
|
@ -384,7 +385,7 @@ describe('wrapPosixHookCommand', () => {
|
|||
ORCA_COPILOT_HOOK_EVENT: 'UserPromptSubmit'
|
||||
})
|
||||
expect(cmd).toBe(
|
||||
"if [ -f '/does/not/exist.sh' ] && [ -r '/does/not/exist.sh' ] && [ -x '/does/not/exist.sh' ]; then ORCA_COPILOT_HOOK_EVENT='UserPromptSubmit' /bin/sh '/does/not/exist.sh'; else cat >/dev/null 2>&1 || :; fi"
|
||||
`if [ -f '/does/not/exist.sh' ] && [ -r '/does/not/exist.sh' ] && [ -x '/does/not/exist.sh' ]; then ORCA_COPILOT_HOOK_EVENT='UserPromptSubmit' /bin/sh '/does/not/exist.sh'; else ${POSIX_HOOK_STDIN_DRAIN_COMMAND}; fi`
|
||||
)
|
||||
})
|
||||
|
||||
|
|
@ -558,7 +559,7 @@ describe('wrapWindowsGitBashHookCommand', () => {
|
|||
expect(
|
||||
wrapWindowsGitBashHookCommand('C:\\Users\\alice\\.orca\\agent-hooks\\claude-hook.cmd')
|
||||
).toBe(
|
||||
"if [ -f 'C:/Users/alice/.orca/agent-hooks/claude-hook.cmd' ]; then 'C:/Users/alice/.orca/agent-hooks/claude-hook.cmd'; else cat >/dev/null 2>&1 || :; fi"
|
||||
`if [ -f 'C:/Users/alice/.orca/agent-hooks/claude-hook.cmd' ]; then 'C:/Users/alice/.orca/agent-hooks/claude-hook.cmd'; else ${POSIX_HOOK_STDIN_DRAIN_COMMAND}; fi`
|
||||
)
|
||||
})
|
||||
|
||||
|
|
|
|||
|
|
@ -2,7 +2,7 @@
|
|||
// matrix catches an unread early exit without duplicating template assertions.
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import { spawn } from 'node:child_process'
|
||||
import { existsSync, mkdtempSync, readFileSync, readdirSync, rmSync } from 'node:fs'
|
||||
import { existsSync, mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync } from 'node:fs'
|
||||
import { tmpdir } from 'node:os'
|
||||
import { join } from 'node:path'
|
||||
import type { SFTPWrapper } from 'ssh2'
|
||||
|
|
@ -82,6 +82,7 @@ import {
|
|||
wrapWindowsGitBashHookCommand,
|
||||
wrapWindowsHookCommand
|
||||
} from './installer-utils'
|
||||
import { POSIX_HOOK_STDIN_READER } from './hook-stdin-contract'
|
||||
import { createAgentHookMemorySftp } from './agent-hook-memory-sftp.test-fixture'
|
||||
|
||||
const REMOTE_HOME = '/home/dev'
|
||||
|
|
@ -155,6 +156,7 @@ const LOCAL_INSTALLERS = [
|
|||
type HookRun = {
|
||||
exitCode: number | null
|
||||
stdinErrors: NodeJS.ErrnoException[]
|
||||
stdout: string
|
||||
}
|
||||
|
||||
function runHookProcess(
|
||||
|
|
@ -163,8 +165,9 @@ function runHookProcess(
|
|||
env: NodeJS.ProcessEnv
|
||||
): Promise<HookRun> {
|
||||
return new Promise((resolve, reject) => {
|
||||
const child = spawn(executable, args, { env, stdio: ['pipe', 'ignore', 'ignore'] })
|
||||
const child = spawn(executable, args, { env, stdio: ['pipe', 'pipe', 'ignore'] })
|
||||
const stdinErrors: NodeJS.ErrnoException[] = []
|
||||
let stdout = ''
|
||||
const timeout = setTimeout(() => {
|
||||
child.kill('SIGKILL')
|
||||
reject(new Error('hook did not finish after stdin closed'))
|
||||
|
|
@ -173,10 +176,13 @@ function runHookProcess(
|
|||
clearTimeout(timeout)
|
||||
reject(error)
|
||||
})
|
||||
child.stdout.on('data', (chunk: Buffer) => {
|
||||
stdout += chunk.toString()
|
||||
})
|
||||
child.stdin.on('error', (error: NodeJS.ErrnoException) => stdinErrors.push(error))
|
||||
child.on('close', (exitCode) => {
|
||||
clearTimeout(timeout)
|
||||
resolve({ exitCode, stdinErrors })
|
||||
resolve({ exitCode, stdinErrors, stdout })
|
||||
})
|
||||
child.stdin.end(LARGE_PAYLOAD)
|
||||
})
|
||||
|
|
@ -274,7 +280,9 @@ describe('Windows managed hook stdin structure', () => {
|
|||
copilot.indexOf('if (-not $env:ORCA_AGENT_HOOK_PORT')
|
||||
)
|
||||
const kimi = readFileSync(join(hooksDir, 'kimi-hook.sh'), 'utf8')
|
||||
expect(kimi.indexOf('payload=$(cat)')).toBeLessThan(kimi.indexOf('exit 0'))
|
||||
expect(kimi.indexOf(`payload=$(${POSIX_HOOK_STDIN_READER})`)).toBeLessThan(
|
||||
kimi.indexOf('exit 0')
|
||||
)
|
||||
} finally {
|
||||
homedirMock.mockImplementation(() => process.env.HOME ?? tmpdir())
|
||||
if (previousGrokHome === undefined) {
|
||||
|
|
@ -370,7 +378,7 @@ describe.skipIf(process.platform === 'win32')('managed hook stdin lifecycle', ()
|
|||
it('captures stdin before every possible whole-script success exit', async () => {
|
||||
const scripts = await generatePosixScripts()
|
||||
for (const [agent, script] of scripts) {
|
||||
const captureIndex = script.indexOf('payload=$(cat)')
|
||||
const captureIndex = script.indexOf(`payload=$(${POSIX_HOOK_STDIN_READER})`)
|
||||
const firstExitIndex = script.indexOf('exit 0')
|
||||
expect(captureIndex, `${agent} payload capture`).toBeGreaterThanOrEqual(0)
|
||||
expect(firstExitIndex, `${agent} first success exit`).toBeGreaterThan(captureIndex)
|
||||
|
|
@ -393,6 +401,50 @@ describe.skipIf(process.platform === 'win32')('managed hook stdin lifecycle', ()
|
|||
}
|
||||
})
|
||||
|
||||
it('does not need PATH to capture or drain POSIX hook stdin', async () => {
|
||||
const scripts = await generatePosixScripts()
|
||||
for (const [agent, script] of scripts) {
|
||||
const result = await runPosixHook(script, { PATH: '' })
|
||||
expect(result.exitCode, `${agent} exit code`).toBe(0)
|
||||
expect(result.stdinErrors, `${agent} stdin errors`).toHaveLength(0)
|
||||
}
|
||||
|
||||
const missing = await runPosixHook(wrapPosixHookCommand('/missing/orca-hook.sh'), { PATH: '' })
|
||||
expect(missing.exitCode, 'missing script launcher exit code').toBe(0)
|
||||
expect(missing.stdinErrors, 'missing script launcher stdin errors').toHaveLength(0)
|
||||
})
|
||||
|
||||
// Why: an unread stdin still exits 0, so exit codes alone cannot prove the
|
||||
// reader consumed the payload. Assert the captured byte count directly.
|
||||
it.each([
|
||||
['empty PATH', ''],
|
||||
// Why: /bin/cat is absent on NixOS-style hosts, so an absolute path alone is
|
||||
// not enough; the reader must fall back to the shell's default PATH.
|
||||
['PATH without coreutils', '/nonexistent'],
|
||||
// Why: a worktree-local `cat` must never receive the hook payload.
|
||||
['PATH whose first cat is a decoy', '']
|
||||
])('captures the whole payload with %s', async (label, pathValue) => {
|
||||
const decoyDir = mkdtempSync(join(tmpdir(), 'orca-hook-stdin-decoy-'))
|
||||
try {
|
||||
let effectivePath = pathValue
|
||||
if (label === 'PATH whose first cat is a decoy') {
|
||||
const decoy = join(decoyDir, 'cat')
|
||||
writeFileSync(decoy, '#!/bin/sh\nexit 0\n', { mode: 0o755 })
|
||||
effectivePath = decoyDir
|
||||
}
|
||||
const result = await runHookProcess(
|
||||
'/bin/sh',
|
||||
['-c', `payload=$(${POSIX_HOOK_STDIN_READER}); printf '%s' "${'${#payload}'}"`],
|
||||
{ ...hookEnvironment(), PATH: effectivePath }
|
||||
)
|
||||
expect(result.exitCode, `${label} exit code`).toBe(0)
|
||||
expect(result.stdinErrors, `${label} stdin errors`).toHaveLength(0)
|
||||
expect(result.stdout, `${label} captured bytes`).toBe(String(LARGE_PAYLOAD.length))
|
||||
} finally {
|
||||
rmSync(decoyDir, { recursive: true, force: true })
|
||||
}
|
||||
})
|
||||
|
||||
it('drains before Claude skips hooks imported by Devin', async () => {
|
||||
const script = (await generatePosixScripts()).get('claude claude-hook.sh')
|
||||
expect(script).toBeDefined()
|
||||
|
|
|
|||
|
|
@ -16,6 +16,7 @@ vi.mock('os', async () => {
|
|||
})
|
||||
|
||||
import { AntigravityHookService } from './hook-service'
|
||||
import { POSIX_HOOK_STDIN_READER } from '../agent-hooks/hook-stdin-contract'
|
||||
import { createManagedCommandMatcher } from '../agent-hooks/installer-utils'
|
||||
|
||||
const ANTIGRAVITY_SCRIPT_FILE_NAME =
|
||||
|
|
@ -94,7 +95,7 @@ describe('AntigravityHookService', () => {
|
|||
expect(script).not.toContain('[string]::IsNullOrWhiteSpace($inputData)) { exit 0 }')
|
||||
} else {
|
||||
expect(script).toContain('hook_event_name=${ORCA_ANTIGRAVITY_EVENT}')
|
||||
expect(script).toContain('payload=$(cat)')
|
||||
expect(script).toContain(`payload=$(${POSIX_HOOK_STDIN_READER})`)
|
||||
expect(script).toContain("payload='{}'")
|
||||
expect(script).not.toContain('if [ -z "$payload" ]; then\n exit 0\nfi')
|
||||
// Why: payload is piped to curl via stdin (`payload@-`) so it never lands
|
||||
|
|
|
|||
|
|
@ -5,6 +5,7 @@ import { tmpdir } from 'node:os'
|
|||
import { dirname, join, win32 as pathWin32 } from 'node:path'
|
||||
|
||||
import { MANAGED_HOOK_TIMEOUT_SECONDS } from '../agent-hooks/installer-utils'
|
||||
import { POSIX_HOOK_STDIN_DRAIN_COMMAND } from '../agent-hooks/hook-stdin-contract'
|
||||
import {
|
||||
computeTrustKey,
|
||||
computeTrustedHash,
|
||||
|
|
@ -77,7 +78,7 @@ function getManagedTrustEntry(
|
|||
}
|
||||
|
||||
function expectedManagedCommand(scriptPath: string): string {
|
||||
return `if [ -f '${scriptPath}' ] && [ -r '${scriptPath}' ]; then /bin/sh '${scriptPath}'; else cat >/dev/null 2>&1 || :; fi`
|
||||
return `if [ -f '${scriptPath}' ] && [ -r '${scriptPath}' ]; then /bin/sh '${scriptPath}'; else ${POSIX_HOOK_STDIN_DRAIN_COMMAND}; fi`
|
||||
}
|
||||
|
||||
describe('Codex WSL runtime hook install', () => {
|
||||
|
|
|
|||
|
|
@ -16,6 +16,7 @@ vi.mock('os', async () => {
|
|||
})
|
||||
|
||||
import { CursorHookService } from './hook-service'
|
||||
import { POSIX_HOOK_STDIN_READER } from '../agent-hooks/hook-stdin-contract'
|
||||
|
||||
const CURSOR_EVENTS = [
|
||||
'beforeSubmitPrompt',
|
||||
|
|
@ -79,7 +80,7 @@ describe('CursorHookService', () => {
|
|||
} else {
|
||||
// Why: payload is piped to curl via stdin (`payload@-`) so it never lands
|
||||
// on the curl command line (EDR oversized-command-line false positive).
|
||||
expect(script).toContain('payload=$(cat)')
|
||||
expect(script).toContain(`payload=$(${POSIX_HOOK_STDIN_READER})`)
|
||||
expect(script).toContain('printf \'%s\' "$payload" | curl')
|
||||
expect(script).toContain('--data-urlencode "payload@-"')
|
||||
expect(script).not.toContain('--data-urlencode "payload=${payload}"')
|
||||
|
|
|
|||
|
|
@ -16,6 +16,7 @@ vi.mock('os', async () => {
|
|||
})
|
||||
|
||||
import { getGrokToolEventMatcherForTests, GrokHookService } from './hook-service'
|
||||
import { POSIX_HOOK_STDIN_READER } from '../agent-hooks/hook-stdin-contract'
|
||||
|
||||
const GROK_SCRIPT_FILE_NAME = process.platform === 'win32' ? 'grok-hook.cmd' : 'grok-hook.sh'
|
||||
const WINDOWS_POWERSHELL_LAUNCHER =
|
||||
|
|
@ -97,7 +98,7 @@ describe('GrokHookService', () => {
|
|||
} else {
|
||||
// Why: payload is piped to curl via stdin (`payload@-`) so it never lands
|
||||
// on the curl command line (EDR oversized-command-line false positive).
|
||||
expect(script).toContain('payload=$(cat)')
|
||||
expect(script).toContain(`payload=$(${POSIX_HOOK_STDIN_READER})`)
|
||||
expect(script).toContain('printf \'%s\' "$payload" | curl')
|
||||
expect(script).toContain('--data-urlencode "payload@-"')
|
||||
expect(script).toContain('${#GROK_HOME}" -le 4096')
|
||||
|
|
|
|||
Loading…
Reference in New Issue