diff --git a/hooks/README.md b/hooks/README.md index 8b0b645be..63ec3c7d7 100644 --- a/hooks/README.md +++ b/hooks/README.md @@ -131,6 +131,12 @@ export ECC_DISABLED_HOOKS="pre:bash:tmux-reminder,post:edit:typecheck" # bypass the Bash dispatcher or config-protection checks. export ECC_HOOK_INPUT_MAX_BYTES=524288 +# Stop waiting for hook input after this many idle milliseconds when the +# caller never closes stdin (default: 3000; 0 disables the deadline). +# Input that is not yet a complete JSON object is treated as incomplete, so +# fail-closed safety hooks block instead of hanging until the harness kills them. +export ECC_HOOK_INPUT_IDLE_TIMEOUT_MS=5000 + # Disable only GateGuard during setup or recovery export ECC_GATEGUARD=off diff --git a/scripts/hooks/bash-hook-dispatcher.js b/scripts/hooks/bash-hook-dispatcher.js index 5a13202dc..d31e99fad 100644 --- a/scripts/hooks/bash-hook-dispatcher.js +++ b/scripts/hooks/bash-hook-dispatcher.js @@ -2,6 +2,7 @@ 'use strict'; const { isHookEnabled } = require('../lib/hook-flags'); +const { readStdinRaw } = require('./hook-input'); const { buildPreToolUseAdditionalContext, combineAdditionalContext, @@ -72,21 +73,6 @@ const POST_BASH_HOOKS = [ }, ]; -function readStdinRaw() { - return new Promise(resolve => { - let raw = ''; - process.stdin.setEncoding('utf8'); - process.stdin.on('data', chunk => { - if (raw.length < MAX_STDIN) { - const remaining = MAX_STDIN - raw.length; - raw += chunk.substring(0, remaining); - } - }); - process.stdin.on('end', () => resolve(raw)); - process.stdin.on('error', () => resolve(raw)); - }); -} - function normalizeHookResult(previousRaw, output) { if (typeof output === 'string' || Buffer.isBuffer(output)) { return { @@ -183,7 +169,9 @@ function runPostBash(rawInput) { async function main() { const mode = process.argv[2]; - const raw = await readStdinRaw(); + // The shared reader also settles on close and on an idle deadline, so a + // parent that never closes stdin cannot hang the dispatcher (#3665). + const { raw } = await readStdinRaw(process.stdin, { maxStdin: MAX_STDIN }); const result = mode === 'post' ? runPostBash(raw) diff --git a/scripts/hooks/hook-input.js b/scripts/hooks/hook-input.js index 682dffc6b..8926f74dc 100644 --- a/scripts/hooks/hook-input.js +++ b/scripts/hooks/hook-input.js @@ -3,6 +3,9 @@ const { StringDecoder } = require('string_decoder'); const DEFAULT_MAX_STDIN = 1024 * 1024; +// Stay below the shortest per-hook timeout in hooks/hooks.json (5 s) so a +// stalled stdin is settled by the reader, not by the harness killing the hook. +const DEFAULT_STDIN_IDLE_TIMEOUT_MS = 3000; function resolveMaxStdin(value, options = {}) { const writeDiagnostic = options.writeDiagnostic || (() => {}); @@ -24,8 +27,38 @@ function resolveMaxStdin(value, options = {}) { return parsed; } +function resolveStdinIdleTimeout(value, options = {}) { + const writeDiagnostic = options.writeDiagnostic || (() => {}); + if (value === undefined || value === '') return DEFAULT_STDIN_IDLE_TIMEOUT_MS; + + const parsed = Number(value); + if (!Number.isSafeInteger(parsed) || parsed < 0) { + writeDiagnostic( + '[Hook] ECC_HOOK_INPUT_IDLE_TIMEOUT_MS must be a non-negative safe integer; using the 3000 ms default\n' + ); + return DEFAULT_STDIN_IDLE_TIMEOUT_MS; + } + return parsed; +} + +// A JSON object cannot be the prefix of a longer valid document, so a payload +// that already parses as one is complete even though EOF never arrived. +function isCompleteJsonObject(text) { + try { + const parsed = JSON.parse(text); + return parsed !== null && typeof parsed === 'object'; + } catch { + return false; + } +} + function readStdinRaw(stream = process.stdin, options = {}) { const maxStdin = options.maxStdin || DEFAULT_MAX_STDIN; + const idleTimeoutMs = options.idleTimeoutMs === undefined + ? resolveStdinIdleTimeout(process.env.ECC_HOOK_INPUT_IDLE_TIMEOUT_MS) + : options.idleTimeoutMs; + const writeDiagnostic = options.writeDiagnostic + || (stream === process.stdin ? message => process.stderr.write(message) : () => {}); const decoder = new StringDecoder('utf8'); let raw = ''; let acceptedBytes = 0; @@ -33,7 +66,20 @@ function readStdinRaw(stream = process.stdin, options = {}) { return new Promise(resolve => { let settled = false; - stream.on('data', chunk => { + let idleTimer = null; + const clearIdleTimer = () => { + if (idleTimer) clearTimeout(idleTimer); + idleTimer = null; + }; + const armIdleTimer = () => { + if (!(idleTimeoutMs > 0)) return; + clearIdleTimer(); + idleTimer = setTimeout(onIdleTimeout, idleTimeoutMs); + // Never keep the process alive just to wait for this deadline. + if (typeof idleTimer.unref === 'function') idleTimer.unref(); + }; + const onData = chunk => { + armIdleTimer(); const buffer = Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk); if (typeof options.onChunk === 'function') options.onChunk(buffer); const remaining = Math.max(0, maxStdin - acceptedBytes); @@ -43,10 +89,11 @@ function readStdinRaw(stream = process.stdin, options = {}) { acceptedBytes += accepted.length; } if (accepted.length < buffer.length) truncated = true; - }); + }; const finish = () => { if (settled) return; settled = true; + clearIdleTimer(); if (!truncated) raw += decoder.end(); resolve({ raw, truncated }); }; @@ -55,16 +102,44 @@ function readStdinRaw(stream = process.stdin, options = {}) { truncated = true; finish(); }; + // The writer kept stdin open without sending EOF (#3665). Stop reading + // and release the stream so a held-open pipe cannot keep the hook alive. + // Anything short of a complete JSON object is marked incomplete so safety + // hooks remain fail closed. + function onIdleTimeout() { + idleTimer = null; + if (settled) return; + stream.removeListener('data', onData); + if (typeof stream.pause === 'function') stream.pause(); + if (!truncated) { + const pending = raw + decoder.end(); + if (isCompleteJsonObject(pending)) { + raw = pending; + settled = true; + resolve({ raw, truncated: false }); + return; + } + raw = pending; + } + writeDiagnostic( + `[Hook] stdin stayed open without EOF for ${idleTimeoutMs} ms; treating the hook input as incomplete\n` + ); + finishIncomplete(); + } + stream.on('data', onData); stream.once('end', finish); // A transport error or premature close can leave a syntactically plausible // prefix behind. Mark it incomplete so safety hooks remain fail closed. stream.once('error', finishIncomplete); stream.once('close', finishIncomplete); + armIdleTimer(); }); } module.exports = { DEFAULT_MAX_STDIN, + DEFAULT_STDIN_IDLE_TIMEOUT_MS, readStdinRaw, - resolveMaxStdin + resolveMaxStdin, + resolveStdinIdleTimeout }; diff --git a/tests/hooks/hook-input.test.js b/tests/hooks/hook-input.test.js index b9e84e518..1989ee33e 100644 --- a/tests/hooks/hook-input.test.js +++ b/tests/hooks/hook-input.test.js @@ -5,12 +5,20 @@ 'use strict'; const assert = require('assert'); +const path = require('path'); +const { spawn } = require('child_process'); const { PassThrough } = require('stream'); -const { readStdinRaw } = require('../../scripts/hooks/hook-input'); +const { + DEFAULT_STDIN_IDLE_TIMEOUT_MS, + readStdinRaw, + resolveStdinIdleTimeout +} = require('../../scripts/hooks/hook-input'); const { run: runConfigProtection } = require('../../scripts/hooks/config-protection'); const TEST_STDIN_LIMIT = 1024; const STREAM_SETTLEMENT_TIMEOUT_MS = 500; +const TEST_IDLE_TIMEOUT_MS = 50; +const PRE_BASH_DISPATCHER = path.join(__dirname, '..', '..', 'scripts', 'hooks', 'pre-bash-dispatcher.js'); async function test(name, fn) { try { @@ -56,6 +64,34 @@ async function readFromClosedStream(partialInput) { }); } +function settleWithin(promise, ms, label) { + return new Promise((resolve, reject) => { + const timer = setTimeout(() => reject(new Error(`${label} did not settle within ${ms} ms`)), ms); + promise.then( + result => { + clearTimeout(timer); + resolve(result); + }, + error => { + clearTimeout(timer); + reject(error); + } + ); + }); +} + +// The writer keeps the stream open and never sends EOF (#3665). +function readFromHeldOpenStream(chunks, options = {}) { + const stream = new PassThrough(); + const resultPromise = readStdinRaw(stream, { + maxStdin: TEST_STDIN_LIMIT, + idleTimeoutMs: TEST_IDLE_TIMEOUT_MS, + ...options + }); + for (const chunk of chunks) stream.write(chunk); + return { stream, resultPromise }; +} + async function runTests() { console.log('\nHook input reader tests:'); @@ -118,6 +154,137 @@ async function runTests() { passed++; else failed++; + if ( + await test('held-open stream with no input settles as truncated after the idle deadline', async () => { + const { resultPromise } = readFromHeldOpenStream([]); + const result = await settleWithin(resultPromise, STREAM_SETTLEMENT_TIMEOUT_MS, 'readStdinRaw'); + + assert.deepStrictEqual(result, { raw: '', truncated: true }); + }) + ) + passed++; + else failed++; + + if ( + await test('held-open stream with a partial request settles as truncated', async () => { + const partialInput = '{"tool_name":"Write","tool_input":{'; + const { resultPromise } = readFromHeldOpenStream([partialInput]); + const result = await settleWithin(resultPromise, STREAM_SETTLEMENT_TIMEOUT_MS, 'readStdinRaw'); + + assert.strictEqual(result.raw, partialInput); + assert.strictEqual(result.truncated, true); + }) + ) + passed++; + else failed++; + + if ( + await test('held-open stream with a complete JSON object settles as complete', async () => { + const input = '{"tool_name":"Bash","tool_input":{"command":"ls"}}'; + const { resultPromise } = readFromHeldOpenStream([input.slice(0, 10), input.slice(10)]); + const result = await settleWithin(resultPromise, STREAM_SETTLEMENT_TIMEOUT_MS, 'readStdinRaw'); + + assert.deepStrictEqual(result, { raw: input, truncated: false }); + }) + ) + passed++; + else failed++; + + if ( + await test('a complete JSON object past the byte cap stays truncated on the idle deadline', async () => { + const input = JSON.stringify({ padding: 'x'.repeat(TEST_STDIN_LIMIT) }); + const { resultPromise } = readFromHeldOpenStream([input]); + const result = await settleWithin(resultPromise, STREAM_SETTLEMENT_TIMEOUT_MS, 'readStdinRaw'); + + assert.strictEqual(result.truncated, true); + assert.strictEqual(Buffer.byteLength(result.raw), TEST_STDIN_LIMIT); + }) + ) + passed++; + else failed++; + + if ( + await test('each chunk resets the idle deadline', async () => { + const { stream, resultPromise } = readFromHeldOpenStream([]); + let settled = false; + resultPromise.then(() => { + settled = true; + }); + for (const part of ['{"a":', '1', '}']) { + await new Promise(resolve => setTimeout(resolve, TEST_IDLE_TIMEOUT_MS / 2)); + stream.write(part); + } + assert.strictEqual(settled, false, 'reader settled while chunks were still arriving'); + stream.end(); + + assert.deepStrictEqual(await resultPromise, { raw: '{"a":1}', truncated: false }); + }) + ) + passed++; + else failed++; + + if ( + await test('idleTimeoutMs 0 disables the deadline', async () => { + const { stream, resultPromise } = readFromHeldOpenStream([], { idleTimeoutMs: 0 }); + let settled = false; + resultPromise.then(() => { + settled = true; + }); + await new Promise(resolve => setTimeout(resolve, TEST_IDLE_TIMEOUT_MS * 3)); + assert.strictEqual(settled, false, 'reader settled with the deadline disabled'); + stream.end('{}'); + + assert.deepStrictEqual(await resultPromise, { raw: '{}', truncated: false }); + }) + ) + passed++; + else failed++; + + if ( + await test('resolveStdinIdleTimeout validates ECC_HOOK_INPUT_IDLE_TIMEOUT_MS', async () => { + const diagnostics = []; + const writeDiagnostic = message => diagnostics.push(message); + + assert.strictEqual(resolveStdinIdleTimeout(undefined), DEFAULT_STDIN_IDLE_TIMEOUT_MS); + assert.strictEqual(resolveStdinIdleTimeout(''), DEFAULT_STDIN_IDLE_TIMEOUT_MS); + assert.strictEqual(resolveStdinIdleTimeout('750'), 750); + assert.strictEqual(resolveStdinIdleTimeout('0'), 0); + assert.strictEqual(resolveStdinIdleTimeout('-1', { writeDiagnostic }), DEFAULT_STDIN_IDLE_TIMEOUT_MS); + assert.strictEqual(resolveStdinIdleTimeout('soon', { writeDiagnostic }), DEFAULT_STDIN_IDLE_TIMEOUT_MS); + assert.strictEqual(diagnostics.length, 2); + assert.match(diagnostics[0], /ECC_HOOK_INPUT_IDLE_TIMEOUT_MS/); + }) + ) + passed++; + else failed++; + + if ( + await test('pre-bash dispatcher exits fail closed when stdin is never closed', async () => { + const child = spawn(process.execPath, [PRE_BASH_DISPATCHER], { + env: { ...process.env, ECC_HOOK_INPUT_IDLE_TIMEOUT_MS: '100', ECC_HOOK_PROFILE: 'standard' }, + stdio: ['pipe', 'ignore', 'pipe'] + }); + let stderr = ''; + child.stderr.on('data', chunk => { + stderr += chunk; + }); + child.stdin.write('{"tool_name":"Bash","tool_input":{'); + const exitCode = await settleWithin( + new Promise(resolve => child.once('exit', code => resolve(code))), + 5000, + 'pre-bash-dispatcher' + ).finally(() => { + child.stdin.destroy(); + child.kill(); + }); + + assert.strictEqual(exitCode, 2); + assert.match(stderr, /stdin stayed open without EOF for 100 ms/); + }) + ) + passed++; + else failed++; + console.log(`\nPassed: ${passed}`); console.log(`Failed: ${failed}\n`); process.exitCode = failed > 0 ? 1 : 0;