From 63b7faa5dc2a9bcc29de0d4e9c23e0153b84347f Mon Sep 17 00:00:00 2001 From: brennanb2025 Date: Sun, 5 Jul 2026 12:00:44 -0700 Subject: [PATCH] refactor(agent-hooks): keep the token as an argv header, not off-argv via curl MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first cut of this change moved the hook token off argv along with the payload — first via `-H @-` (header from stdin), which curl <7.55 silently drops (breaking agent statuses on 2013-2017-era SSH remotes), then via `-K -` (config from stdin), which works on old curl but reads stdin as a curl *config file*: a newline in the token could smuggle a `url =`/`output =` directive, so it needed a UUID-charset guard to be safe. That whole mechanism existed only to hide a per-session, loopback-only UUID that a same-user process already reads from the 0600 endpoint file, and that the Windows path already leaves on argv. The mitigation was out of proportion to the asset. Send the token as a plain `-H` header on argv instead. The security wins that matter are unchanged: the endpoint file is still parsed never sourced (the code-exec fix), the payload — the prompt and tool output — is still off argv in a private temp file, and `--noproxy 127.0.0.1` still blocks loopback proxy exfil. The token's argv exposure is same-user only and now matches the documented Windows behavior. The PR no longer changes any curl version requirement. Also adds service-level script-pinning tests for gemini/copilot/droid and fixes two Windows-only test bugs (devin cmd assertions, kimi USERPROFILE). STA-1284 Co-authored-by: Orca --- .../agent-hooks/managed-hook-script.test.ts | 6 ++-- src/main/agent-hooks/managed-hook-script.ts | 32 +++++++++++-------- src/main/claude/hook-service.test.ts | 10 +++--- src/main/copilot/hook-service.test.ts | 22 +++++++++++++ src/main/cursor/hook-service.test.ts | 4 +-- src/main/devin/hook-service.test.ts | 22 +++++++++---- src/main/droid/hook-service.test.ts | 12 +++++++ src/main/gemini/hook-service.test.ts | 21 ++++++++++++ src/main/kimi/hook-service.test.ts | 19 ++++++++--- 9 files changed, 111 insertions(+), 37 deletions(-) diff --git a/src/main/agent-hooks/managed-hook-script.test.ts b/src/main/agent-hooks/managed-hook-script.test.ts index 4f4b5ed6bdb..20a46fd7c88 100644 --- a/src/main/agent-hooks/managed-hook-script.test.ts +++ b/src/main/agent-hooks/managed-hook-script.test.ts @@ -20,10 +20,8 @@ describe('buildPosixManagedHookScript — static invariants', () => { expect(script).toContain('done < "$ORCA_AGENT_HOOK_ENDPOINT"') }) - it('keeps the token off the command line (streamed as a header via stdin)', () => { - expect(script).not.toContain('X-Orca-Agent-Hook-Token: ${ORCA_AGENT_HOOK_TOKEN}') - expect(script).toContain('printf \'X-Orca-Agent-Hook-Token: %s\\n\' "$ORCA_AGENT_HOOK_TOKEN" |') - expect(script).toContain('-H @-') + it('sends the token as a header (matching the other hook transports)', () => { + expect(script).toContain('-H "X-Orca-Agent-Hook-Token: ${ORCA_AGENT_HOOK_TOKEN}"') }) it('keeps the payload off the command line (read from a temp file)', () => { diff --git a/src/main/agent-hooks/managed-hook-script.ts b/src/main/agent-hooks/managed-hook-script.ts index 90e33ff5646..bf4e09c8e67 100644 --- a/src/main/agent-hooks/managed-hook-script.ts +++ b/src/main/agent-hooks/managed-hook-script.ts @@ -10,10 +10,11 @@ import type { AgentHookSource } from '../../shared/agent-hook-relay' // it (`. "$file"` / `call "%file%"`), so any shell/batch written into that // path ran with the agent's privileges. We only ever copy the four known // `ORCA_AGENT_HOOK_*` values out of it. -// 2. The hook token and the raw payload never appear on the process command -// line. `ps`/`/proc//cmdline` are readable by other same-UID (and, on -// many systems, cross-UID) processes, so the token went to a header read -// from stdin and the payload to a private temp file read via `@file`. +// 2. The raw payload never appears on the process command line. `ps`/ +// `/proc//cmdline` are readable by other same-UID processes, so the +// payload (the user's prompt and tool output) goes to a private temp file +// read via `@file`. The token stays an argv header — see the note on +// `buildPosixSecureCurlPostLines` for why that is deliberate. // `--noproxy 127.0.0.1` keeps the loopback POST from being redirected through a // configured `http_proxy`/`ALL_PROXY`, which would send the token and payload // off-box. @@ -77,13 +78,17 @@ export function buildPosixHookGuardLines(): string[] { // Why: capture the payload into a private temp file (mktemp is 0600) and post // it with `--data-urlencode payload@` so the raw prompt never lands on -// argv. This also keeps tens-of-KB tool output off the curl command line, which -// avoids EDR command-line-length false positives (the concern #4475 fixed via a -// stdin pipe). We use a temp file rather than that stdin pipe because stdin is -// needed for the token: it is streamed into curl as a header via `-H @-`, so the -// secret stays off argv AND off disk. Non-secret identifiers -// (paneKey/tabId/worktreeId/…) stay as argv fields — they are not sensitive and -// keeping them there avoids escaping filesystem paths into the temp file. +// argv. This keeps tens-of-KB tool output off the curl command line, which +// avoids EDR command-line-length false positives (the concern #4475 fixed). +// The token stays an argv header: it is a low-value, per-session, loopback-only +// credential already present in the 0600 endpoint file, so its `ps` exposure +// matches the Windows path's documented same-user exposure. Moving it off argv +// on the curl versions we still support would mean routing it through curl's +// config-file loader (the only pre-7.55 stdin-header mechanism), which adds a +// config-directive injection surface out of proportion to what a fake-status +// loopback token is worth. Non-secret identifiers (paneKey/tabId/worktreeId/…) +// stay as argv fields — they are not sensitive and keeping them there avoids +// escaping filesystem paths into the temp file. export function buildPosixSecureCurlPostLines(options: PosixManagedHookScriptOptions): string[] { const { source, extraDataFields = [], emptyPayload = 'exit' } = options const emptyLines = @@ -103,12 +108,11 @@ export function buildPosixSecureCurlPostLines(options: PosixManagedHookScriptOpt 'trap \'rm -f "$__orca_payload_file"\' EXIT', 'cat > "$__orca_payload_file"', ...emptyLines, - 'printf \'X-Orca-Agent-Hook-Token: %s\\n\' "$ORCA_AGENT_HOOK_TOKEN" | ' + - `curl -sS -X POST "http://127.0.0.1:\${ORCA_AGENT_HOOK_PORT}/hook/${source}" \\`, + `curl -sS -X POST "http://127.0.0.1:\${ORCA_AGENT_HOOK_PORT}/hook/${source}" \\`, ' --noproxy 127.0.0.1 \\', ' --connect-timeout 0.5 --max-time 1.5 \\', ' -H "Content-Type: application/x-www-form-urlencoded" \\', - ' -H @- \\', + ' -H "X-Orca-Agent-Hook-Token: ${ORCA_AGENT_HOOK_TOKEN}" \\', ' --data-urlencode "paneKey=${ORCA_PANE_KEY}" \\', ' --data-urlencode "tabId=${ORCA_TAB_ID}" \\', ' --data-urlencode "launchToken=${ORCA_AGENT_LAUNCH_TOKEN}" \\', diff --git a/src/main/claude/hook-service.test.ts b/src/main/claude/hook-service.test.ts index 1167dfdacab..3ecfbd80e11 100644 --- a/src/main/claude/hook-service.test.ts +++ b/src/main/claude/hook-service.test.ts @@ -269,12 +269,12 @@ describe('ClaudeHookService.installRemote', () => { const script = fs.files.get('/home/dev/.orca/agent-hooks/claude-hook.sh') expect(script).toContain('#!/bin/sh') expect(script).toContain('DEVIN_PROJECT_DIR') - // Why: payload posts from a private temp file and the token streams in as a - // header via stdin, so neither the prompt nor the token lands on the curl - // command line (also avoids EDR oversized-command-line false positives). - // The endpoint file is parsed, never sourced. + // Why: the payload (the prompt/tool output) posts from a private temp file + // so it never lands on the curl command line (also avoids EDR + // oversized-command-line false positives). The endpoint file is parsed, + // never sourced. The token is a low-value loopback header on argv. expect(script).toContain('--data-urlencode "payload@${__orca_payload_file}"') - expect(script).toContain('-H @-') + expect(script).toContain('-H "X-Orca-Agent-Hook-Token: ${ORCA_AGENT_HOOK_TOKEN}"') expect(script).not.toContain('--data-urlencode "payload=${payload}"') expect(script).not.toContain('. "$ORCA_AGENT_HOOK_ENDPOINT"') expect(fs.modes.get('/home/dev/.orca/agent-hooks/claude-hook.sh')).toBe(0o755) diff --git a/src/main/copilot/hook-service.test.ts b/src/main/copilot/hook-service.test.ts index 68ae69629c4..8958d776306 100644 --- a/src/main/copilot/hook-service.test.ts +++ b/src/main/copilot/hook-service.test.ts @@ -117,6 +117,28 @@ describe('CopilotHookService', () => { } }) + // Why: the shared-builder tests use synthetic options, so copilot's + // agent-specific stdout prelude and hookEventName field are only pinned here. + it.skipIf(process.platform === 'win32')( + 'managed script prints {} first and posts the hookEventName field', + () => { + new CopilotHookService().install() + const script = readFileSync(join(tmpDir, '.orca', 'agent-hooks', 'copilot-hook.sh'), 'utf-8') + // Copilot parses hook stdout; {} must be emitted before any guard can + // exit the script. + expect(script.indexOf("printf '{}\\n'")).toBeGreaterThan(-1) + expect(script.indexOf("printf '{}\\n'")).toBeLessThan( + script.indexOf('done < "$ORCA_AGENT_HOOK_ENDPOINT"') + ) + expect(script).toContain('--data-urlencode "hookEventName=${ORCA_COPILOT_HOOK_EVENT}"') + expect(script).toContain('/hook/copilot') + expect(script).not.toContain('. "$ORCA_AGENT_HOOK_ENDPOINT"') + expect(script).toContain('-H "X-Orca-Agent-Hook-Token: ${ORCA_AGENT_HOOK_TOKEN}"') + expect(script).toContain('--data-urlencode "payload@${__orca_payload_file}"') + expect(script).toContain('--noproxy 127.0.0.1') + } + ) + it('preserves user-authored hooks and sweeps stale managed entries', () => { const configPath = join(copilotHome, 'hooks', 'orca.json') mkdirSync(join(copilotHome, 'hooks'), { recursive: true }) diff --git a/src/main/cursor/hook-service.test.ts b/src/main/cursor/hook-service.test.ts index bcf7a5618d9..3d507fe45e5 100644 --- a/src/main/cursor/hook-service.test.ts +++ b/src/main/cursor/hook-service.test.ts @@ -77,8 +77,8 @@ describe('CursorHookService', () => { if (process.platform === 'win32') { expect(script).toContain('%SystemRoot%\\System32\\curl.exe') } else { - // Payload goes to a private temp file (off argv); token is streamed as a - // header via `-H @-`; the endpoint file is parsed, never sourced. + // Payload goes to a private temp file (off argv); the endpoint file is + // parsed, never sourced. expect(script).toContain('--data-urlencode "payload@${__orca_payload_file}"') expect(script).not.toContain('. "$ORCA_AGENT_HOOK_ENDPOINT"') } diff --git a/src/main/devin/hook-service.test.ts b/src/main/devin/hook-service.test.ts index 850b1ee8ed4..a4072d6b11d 100644 --- a/src/main/devin/hook-service.test.ts +++ b/src/main/devin/hook-service.test.ts @@ -64,13 +64,21 @@ describe('DevinHookService', () => { } const script = readFileSync(getDevinManagedScriptPath(), 'utf8') expect(script).toContain('/hook/devin') - // Why: payload posts from a private temp file and the token streams in as a - // header via stdin, so neither lands on the curl command line (also avoids - // EDR oversized-command-line false positives). Endpoint file parsed, not sourced. - expect(script).toContain('--data-urlencode "payload@${__orca_payload_file}"') - expect(script).toContain('-H @-') - expect(script).not.toContain('--data-urlencode "payload=${payload}"') - expect(script).not.toContain('. "$ORCA_AGENT_HOOK_ENDPOINT"') + if (process.platform === 'win32') { + // Why: cmd has no mktemp, so the payload streams from stdin and the + // endpoint file is parsed by the for/f subroutine, never call-executed. + expect(script).toContain('--data-urlencode "payload@-"') + expect(script).toContain('call :__orcaLoadEndpoint') + expect(script).not.toContain('call "%ORCA_AGENT_HOOK_ENDPOINT%"') + } else { + // Why: the payload posts from a private temp file so it never lands on + // the curl command line (also avoids EDR oversized-command-line false + // positives). Endpoint file parsed, not sourced. Token is a loopback header. + expect(script).toContain('--data-urlencode "payload@${__orca_payload_file}"') + expect(script).toContain('-H "X-Orca-Agent-Hook-Token: ${ORCA_AGENT_HOOK_TOKEN}"') + expect(script).not.toContain('--data-urlencode "payload=${payload}"') + expect(script).not.toContain('. "$ORCA_AGENT_HOOK_ENDPOINT"') + } }) it('preserves unrelated keys in Devin config when installing hooks', () => { diff --git a/src/main/droid/hook-service.test.ts b/src/main/droid/hook-service.test.ts index ef31754a7e7..e2f28d32d01 100644 --- a/src/main/droid/hook-service.test.ts +++ b/src/main/droid/hook-service.test.ts @@ -109,6 +109,18 @@ describe('DroidHookService', () => { } ) + // Why: nothing else pins the droid script body at the service level; assert + // the secure post shape so a builder-call regression fails here too. + it.skipIf(process.platform === 'win32')('managed script posts securely to /hook/droid', () => { + expect(new DroidHookService().install().state).toBe('installed') + const script = readFileSync(join(homeDir, '.orca', 'agent-hooks', 'droid-hook.sh'), 'utf8') + expect(script).toContain('/hook/droid') + expect(script).not.toContain('. "$ORCA_AGENT_HOOK_ENDPOINT"') + expect(script).toContain('-H "X-Orca-Agent-Hook-Token: ${ORCA_AGENT_HOOK_TOKEN}"') + expect(script).toContain('--data-urlencode "payload@${__orca_payload_file}"') + expect(script).toContain('--noproxy 127.0.0.1') + }) + it('reports partial when Factory has hooks disabled globally', () => { const configPath = join(homeDir, '.factory', 'settings.json') mkdirSync(dirname(configPath), { recursive: true }) diff --git a/src/main/gemini/hook-service.test.ts b/src/main/gemini/hook-service.test.ts index 1a80c1568bf..f901af6b094 100644 --- a/src/main/gemini/hook-service.test.ts +++ b/src/main/gemini/hook-service.test.ts @@ -188,4 +188,25 @@ describe('GeminiHookService', () => { : new RegExp(managedHookFileName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')) ) }) + + // Why: the shared-builder tests use synthetic options, so gemini's + // agent-specific stdout prelude is only pinned here. + it.skipIf(process.platform === 'win32')( + 'managed script prints {} before the endpoint parse and posts securely', + () => { + expect(new GeminiHookService().install().state).toBe('installed') + const script = readFileSync(join(homeDir, '.orca', 'agent-hooks', 'gemini-hook.sh'), 'utf8') + // Gemini parses hook stdout as JSON; {} must be emitted before any guard + // can exit the script. + expect(script.indexOf('printf "{}\\n"')).toBeGreaterThan(-1) + expect(script.indexOf('printf "{}\\n"')).toBeLessThan( + script.indexOf('done < "$ORCA_AGENT_HOOK_ENDPOINT"') + ) + expect(script).toContain('/hook/gemini') + expect(script).not.toContain('. "$ORCA_AGENT_HOOK_ENDPOINT"') + expect(script).toContain('-H "X-Orca-Agent-Hook-Token: ${ORCA_AGENT_HOOK_TOKEN}"') + expect(script).toContain('--data-urlencode "payload@${__orca_payload_file}"') + expect(script).toContain('--noproxy 127.0.0.1') + } + ) }) diff --git a/src/main/kimi/hook-service.test.ts b/src/main/kimi/hook-service.test.ts index aeb188b2b8c..4e56e31eaec 100644 --- a/src/main/kimi/hook-service.test.ts +++ b/src/main/kimi/hook-service.test.ts @@ -8,16 +8,20 @@ import { KIMI_HOOK_EVENTS } from './kimi-hook-config-toml' // Why: getSharedManagedScriptPath() writes the managed script under // homedir()/.orca, and getKimiHome() honors KIMI_CODE_HOME. Point both at a // temp dir so the local install/remove cycle never touches the real ~/.orca or -// ~/.kimi-code. os.homedir() resolves $HOME on POSIX (verified at write time). +// ~/.kimi-code. os.homedir() resolves $HOME on POSIX and USERPROFILE on +// Windows, so both must be pinned. let home: string let originalHome: string | undefined +let originalUserProfile: string | undefined let originalKimiHome: string | undefined beforeEach(() => { home = mkdtempSync(join(tmpdir(), 'orca-kimi-hook-')) originalHome = process.env.HOME + originalUserProfile = process.env.USERPROFILE originalKimiHome = process.env.KIMI_CODE_HOME process.env.HOME = home + process.env.USERPROFILE = home process.env.KIMI_CODE_HOME = join(home, '.kimi-code') }) @@ -27,6 +31,11 @@ afterEach(() => { } else { process.env.HOME = originalHome } + if (originalUserProfile === undefined) { + delete process.env.USERPROFILE + } else { + process.env.USERPROFILE = originalUserProfile + } if (originalKimiHome === undefined) { delete process.env.KIMI_CODE_HOME } else { @@ -55,11 +64,11 @@ describe('KimiHookService', () => { // The managed script must exist and POST to the Kimi hook endpoint. const script = readFileSync(scriptPath(), 'utf-8') expect(script).toContain('/hook/kimi') - // Why: payload posts from a private temp file and the token streams in as a - // header via stdin, so neither lands on the curl command line (also avoids - // EDR oversized-command-line false positives). Endpoint file parsed, not sourced. + // Why: the payload posts from a private temp file so it never lands on the + // curl command line (also avoids EDR oversized-command-line false + // positives). Endpoint file parsed, not sourced. Token is a loopback header. expect(script).toContain('--data-urlencode "payload@${__orca_payload_file}"') - expect(script).toContain('-H @-') + expect(script).toContain('-H "X-Orca-Agent-Hook-Token: ${ORCA_AGENT_HOOK_TOKEN}"') expect(script).not.toContain('--data-urlencode "payload=${payload}"') expect(script).not.toContain('. "$ORCA_AGENT_HOOK_ENDPOINT"') // The command Kimi runs points at the managed script via sh.