diff --git a/docs/guide/architecture.md b/docs/guide/architecture.md index 6a0a148aab..ba84cd0868 100644 --- a/docs/guide/architecture.md +++ b/docs/guide/architecture.md @@ -96,8 +96,12 @@ path's ordering. ## The Claude Code hook flow The live flow lives in `hook_cmd.rs`. The agent sends a PreToolUse JSON payload -on stdin; `run_claude` reads it (size-limited, failing closed on a malformed or -oversized payload) and hands the parsed value to +on stdin; `run_claude` reads it with a short deadline and a size limit. Empty +EOF is a no-op. Malformed, oversized, unreadable, or timed-out payloads fail +closed with a Claude deny verdict. Copilot and Cursor share the same bounded +stdin reader but fail open on read failure because their host permission engines +remain the backstop; Gemini shares the bounded reader and fails closed with a +Gemini deny verdict. Parsed Claude payloads are handed to `process_claude_payload_with_gate`. The critical ordering property: the **defence-in-depth gates run on the RAW @@ -107,7 +111,7 @@ a flagged command from the gate. ```mermaid flowchart TD A[PreToolUse JSON on stdin] --> B{Parse payload} - B -- malformed / oversized --> DENY1[Deny - fail closed] + B -- malformed / oversized / timeout --> DENY1[Deny - fail closed] B -- ok --> C{Extract /tool_input/command} C -- absent --> IGN1[Ignore - non-Bash tool] C -- present but not a string --> DENY2[Deny - malformed payload] diff --git a/src/hooks/hook_cmd.rs b/src/hooks/hook_cmd.rs index 28f36cd9ab..977c62eba8 100644 --- a/src/hooks/hook_cmd.rs +++ b/src/hooks/hook_cmd.rs @@ -14,21 +14,48 @@ use super::{supply_chain_gate, tirith_gate}; use anyhow::{Context, Result}; use serde_json::{json, Value}; use std::io::{self, Read, Write}; +use std::sync::mpsc; +use std::thread; +use std::time::Duration; use crate::discover::registry::{has_heredoc, rewrite_command}; const STDIN_CAP: usize = 1_048_576; // 1 MiB +const STDIN_READ_TIMEOUT: Duration = Duration::from_millis(500); fn read_stdin_limited() -> Result { - let mut input = String::new(); - io::stdin() - .take((STDIN_CAP + 1) as u64) - .read_to_string(&mut input) - .context("Failed to read stdin")?; - if input.len() > STDIN_CAP { - anyhow::bail!("hook stdin exceeds {} byte limit", STDIN_CAP); - } - Ok(input) + read_stdin_limited_with_timeout(io::stdin(), STDIN_READ_TIMEOUT) +} + +fn read_stdin_limited_with_timeout(reader: R, timeout: Duration) -> Result +where + R: Read + Send + 'static, +{ + let (tx, rx) = mpsc::channel(); + thread::spawn(move || { + let mut input = String::new(); + let result = reader + .take((STDIN_CAP + 1) as u64) + .read_to_string(&mut input) + .context("Failed to read stdin") + .and_then(|_| { + if input.len() > STDIN_CAP { + anyhow::bail!("hook stdin exceeds {} byte limit", STDIN_CAP); + } + Ok(input) + }); + let _ = tx.send(result); + }); + + match rx.recv_timeout(timeout) { + Ok(result) => result, + Err(mpsc::RecvTimeoutError::Timeout) => { + anyhow::bail!("hook stdin read timed out after {}ms", timeout.as_millis()) + } + Err(mpsc::RecvTimeoutError::Disconnected) => { + anyhow::bail!("hook stdin reader disconnected") + } + } } // ── Copilot hook (VS Code + Copilot CLI) ────────────────────── @@ -46,7 +73,15 @@ enum HookFormat { /// Run the Copilot preToolUse hook. /// Auto-detects VS Code Copilot Chat vs Copilot CLI format. pub fn run_copilot() -> Result<()> { - let input = read_stdin_limited()?; + let input = match read_stdin_limited() { + Ok(input) => input, + Err(e) => { + // Fail OPEN: Copilot remains the permission backstop and a hook + // read timeout must not wedge the shell tool invocation. + let _ = writeln!(io::stderr(), "[contextcrawler hook] {e}"); + return Ok(()); + } + }; let input = input.trim(); if input.is_empty() { @@ -867,7 +902,16 @@ fn strip_leading_bom(input: &str) -> &str { /// Run the Cursor Agent hook natively. pub fn run_cursor() -> Result<()> { - let input = read_stdin_limited()?; + let input = match read_stdin_limited() { + Ok(input) => input, + Err(e) => { + // Fail OPEN: Cursor's own permission engine is the backstop, and + // `{}` is the existing pass-through shape for malformed payloads. + let _ = writeln!(io::stderr(), "[contextcrawler hook] {e}"); + let _ = writeln!(io::stdout(), "{{}}"); + return Ok(()); + } + }; let input = strip_leading_bom(&input).trim(); if input.is_empty() { @@ -1168,14 +1212,11 @@ mod tests { // gate (mirrors Tirith default-off). Auto-allow only happens on a true // `Allow` verdict, which the unattestable gate downgrades to `Ask`. let allow = vec!["*".to_string()]; - let check = move |c: &str| { - permissions::check_command_with_rules(c, &[], &[], &allow) - }; + let check = move |c: &str| permissions::check_command_with_rules(c, &[], &[], &allow); let v: Value = serde_json::from_str(&claude_input(cmd)).unwrap(); match process_claude_payload_with_gate(&v, check, |_| GateDecision::Proceed) { PayloadAction::Rewrite { output, .. } => { - output.pointer("/hookSpecificOutput/permissionDecision") - == Some(&json!("allow")) + output.pointer("/hookSpecificOutput/permissionDecision") == Some(&json!("allow")) } _ => false, } @@ -1192,9 +1233,7 @@ mod tests { fn test_live_substitution_never_auto_allows() { assert!(!auto_allowed_on_live_path("git status `whoami`")); assert!(!auto_allowed_on_live_path("git log --pretty=$(whoami)")); - assert!(!auto_allowed_on_live_path( - "git log --pretty=\"$(whoami)\"" - )); + assert!(!auto_allowed_on_live_path("git log --pretty=\"$(whoami)\"")); } #[test] diff --git a/tests/hook_stdin_timeout.rs b/tests/hook_stdin_timeout.rs new file mode 100644 index 0000000000..6b70dd335a --- /dev/null +++ b/tests/hook_stdin_timeout.rs @@ -0,0 +1,102 @@ +//! Regression tests for hook stdin that stays open without sending a payload +//! or EOF (#200). + +use std::path::PathBuf; +use std::process::{Child, Command, Stdio}; +use std::thread; +use std::time::{Duration, Instant}; + +fn binary_path() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_contextcrawler")) +} + +fn spawn_hook(name: &str) -> Child { + Command::new(binary_path()) + .arg("hook") + .arg(name) + .env("CONTEXTCRAWLER_TEST_MODE", "1") + .env("RTK_TELEMETRY_DISABLED", "1") + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .unwrap_or_else(|e| panic!("spawn contextcrawler hook {name}: {e}")) +} + +fn wait_quickly(mut child: Child, hook: &str) -> std::process::Output { + let deadline = Instant::now() + Duration::from_secs(2); + while Instant::now() < deadline { + if child + .try_wait() + .unwrap_or_else(|e| panic!("{hook}: try_wait failed: {e}")) + .is_some() + { + return child + .wait_with_output() + .unwrap_or_else(|e| panic!("{hook}: wait_with_output failed: {e}")); + } + thread::sleep(Duration::from_millis(25)); + } + let _ = child.kill(); + panic!("{hook}: hook did not exit before timeout with stdin held open"); +} + +fn run_open_stdin_without_payload(hook: &str) -> std::process::Output { + let mut child = spawn_hook(hook); + let _held_open = child.stdin.take().expect("child stdin pipe"); + wait_quickly(child, hook) +} + +fn run_empty_eof(hook: &str) -> std::process::Output { + let mut child = spawn_hook(hook); + drop(child.stdin.take()); + wait_quickly(child, hook) +} + +#[test] +fn claude_open_stdin_without_payload_fails_closed_without_hanging() { + let output = run_open_stdin_without_payload("claude"); + assert!(output.status.success()); + let stdout = String::from_utf8_lossy(&output.stdout); + let v: serde_json::Value = + serde_json::from_str(stdout.trim()).expect("Claude timeout must emit JSON"); + assert_eq!(v["hookSpecificOutput"]["permissionDecision"], "deny"); +} + +#[test] +fn gemini_open_stdin_without_payload_fails_closed_without_hanging() { + let output = run_open_stdin_without_payload("gemini"); + assert!(output.status.success()); + let stdout = String::from_utf8_lossy(&output.stdout); + let v: serde_json::Value = + serde_json::from_str(stdout.trim()).expect("Gemini timeout must emit JSON"); + assert_eq!(v["decision"], "deny"); +} + +#[test] +fn copilot_open_stdin_without_payload_fails_open_without_hanging() { + let output = run_open_stdin_without_payload("copilot"); + assert!(output.status.success()); + assert!( + output.stdout.is_empty(), + "Copilot timeout should pass through silently, got stdout: {}", + String::from_utf8_lossy(&output.stdout) + ); +} + +#[test] +fn empty_eof_remains_clean_noop_for_pass_through_hooks() { + for hook in ["claude", "copilot"] { + let output = run_empty_eof(hook); + assert!( + output.status.success(), + "{hook}: status {:?}", + output.status + ); + assert!( + output.stdout.is_empty(), + "{hook}: empty EOF should not emit stdout, got {}", + String::from_utf8_lossy(&output.stdout) + ); + } +}