From 97594b3ab09bfab3d3172fa40fd8681eba6b681f Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 5 Mar 2026 01:59:38 -0500 Subject: [PATCH] Add HTTP hook executor with env var interpolation HTTP hooks (type = "http") now actually execute instead of failing with "no command specified". The executor POSTs the hook context as JSON, parses HookDecision from the response, and fails open on errors. Header values support $VAR/${VAR} interpolation gated by an allowed_env_vars whitelist on the hook definition. Renames CommandHookExecutor to HookExecutorImpl since it now handles both command and HTTP hook types. Co-Authored-By: Claude Opus 4.6 --- Cargo.lock | 36 ++ crates/arc-workflows/Cargo.toml | 2 + crates/arc-workflows/src/hook/config.rs | 58 ++- crates/arc-workflows/src/hook/executor.rs | 425 +++++++++++++++++++--- crates/arc-workflows/src/hook/runner.rs | 4 +- 5 files changed, 474 insertions(+), 51 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ee8780065..ff274984a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -335,10 +335,12 @@ dependencies = [ "futures", "git2", "indicatif", + "mockito", "nom", "predicates", "rand 0.8.5", "regex", + "reqwest 0.12.28", "scopeguard", "serde", "serde_json", @@ -776,6 +778,15 @@ version = "1.0.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b05b61dc5112cbb17e4b6cd61790d9845d13888356391624cbe7e41efeac1e75" +[[package]] +name = "colored" +version = "3.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "faf9468729b8cbcea668e36183cb69d317348c2e08e994829fb56ebfdfbaac34" +dependencies = [ + "windows-sys 0.61.2", +] + [[package]] name = "combine" version = "4.6.7" @@ -2557,6 +2568,31 @@ dependencies = [ "windows-sys 0.61.2", ] +[[package]] +name = "mockito" +version = "1.7.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "90820618712cab19cfc46b274c6c22546a82affcb3c3bdf0f29e3db8e1bb92c0" +dependencies = [ + "assert-json-diff", + "bytes", + "colored", + "futures-core", + "http", + "http-body", + "http-body-util", + "hyper", + "hyper-util", + "log", + "pin-project-lite", + "rand 0.9.2", + "regex", + "serde_json", + "serde_urlencoded", + "similar", + "tokio", +] + [[package]] name = "native-tls" version = "0.2.18" diff --git a/crates/arc-workflows/Cargo.toml b/crates/arc-workflows/Cargo.toml index 5d7431df0..c2a997722 100644 --- a/crates/arc-workflows/Cargo.toml +++ b/crates/arc-workflows/Cargo.toml @@ -44,7 +44,9 @@ console.workspace = true indicatif.workspace = true tokio-util.workspace = true tracing.workspace = true +reqwest.workspace = true [dev-dependencies] +mockito = "1" tokio = { workspace = true, features = ["test-util", "macros"] } tempfile = "3" dotenvy.workspace = true diff --git a/crates/arc-workflows/src/hook/config.rs b/crates/arc-workflows/src/hook/config.rs index bf0c3acab..8b704cff1 100644 --- a/crates/arc-workflows/src/hook/config.rs +++ b/crates/arc-workflows/src/hook/config.rs @@ -7,7 +7,12 @@ use super::types::HookEvent; #[serde(tag = "type", rename_all = "snake_case")] pub enum HookType { Command { command: String }, - Http { url: String, headers: Option> }, + Http { + url: String, + headers: Option>, + #[serde(default)] + allowed_env_vars: Vec, + }, } /// A single hook definition. @@ -170,6 +175,57 @@ url = "https://hooks.example.com/done" )); } + #[test] + fn parse_http_hook_with_allowed_env_vars() { + let toml = r#" +[[hooks]] +event = "run_start" +type = "http" +url = "https://hooks.example.com/start" +allowed_env_vars = ["API_KEY", "SECRET"] + +[hooks.headers] +Authorization = "Bearer $API_KEY" +"#; + let config: HookConfig = toml::from_str(toml).unwrap(); + let hook = &config.hooks[0]; + match hook.resolved_hook_type().unwrap() { + HookType::Http { + url, + headers, + allowed_env_vars, + } => { + assert_eq!(url, "https://hooks.example.com/start"); + assert_eq!(allowed_env_vars, vec!["API_KEY", "SECRET"]); + assert_eq!( + headers.unwrap().get("Authorization").unwrap(), + "Bearer $API_KEY" + ); + } + _ => panic!("expected Http hook type"), + } + } + + #[test] + fn parse_http_hook_allowed_env_vars_defaults_empty() { + let toml = r#" +[[hooks]] +event = "run_complete" +type = "http" +url = "https://hooks.example.com/done" +"#; + let config: HookConfig = toml::from_str(toml).unwrap(); + let hook = &config.hooks[0]; + match hook.resolved_hook_type().unwrap() { + HookType::Http { + allowed_env_vars, .. + } => { + assert!(allowed_env_vars.is_empty()); + } + _ => panic!("expected Http hook type"), + } + } + #[test] fn parse_full_hook_definition() { let toml = r#" diff --git a/crates/arc-workflows/src/hook/executor.rs b/crates/arc-workflows/src/hook/executor.rs index 2c9827c7d..25d41de9b 100644 --- a/crates/arc-workflows/src/hook/executor.rs +++ b/crates/arc-workflows/src/hook/executor.rs @@ -6,7 +6,7 @@ use async_trait::async_trait; use arc_agent::Sandbox; -use super::config::HookDefinition; +use super::config::{HookDefinition, HookType}; use super::types::{HookContext, HookDecision, HookResult}; /// Trait for executing hooks via different transports. @@ -21,10 +21,51 @@ pub trait HookExecutor: Send + Sync { ) -> HookResult; } -/// Executes hooks as shell commands (host or sandbox). -pub struct CommandHookExecutor; +/// Interpolate `$VAR` and `${VAR}` references in `value` using environment +/// variables, but only when the variable name appears in `allowed_vars`. +/// Unlisted or missing vars are replaced with the empty string. +pub fn interpolate_env_vars(value: &str, allowed_vars: &[String]) -> String { + let mut result = String::with_capacity(value.len()); + let mut chars = value.chars().peekable(); -impl CommandHookExecutor { + while let Some(ch) = chars.next() { + if ch == '$' { + let braced = chars.peek() == Some(&'{'); + if braced { + chars.next(); // consume '{' + } + + let mut var_name = String::new(); + while let Some(&c) = chars.peek() { + if braced { + if c == '}' { + chars.next(); + break; + } + } else if !c.is_ascii_alphanumeric() && c != '_' { + break; + } + var_name.push(c); + chars.next(); + } + + if !var_name.is_empty() && allowed_vars.iter().any(|v| v == &var_name) { + if let Ok(val) = std::env::var(&var_name) { + result.push_str(&val); + } + } + } else { + result.push(ch); + } + } + + result +} + +/// Executes hooks via shell commands or HTTP POST. +pub struct HookExecutorImpl; + +impl HookExecutorImpl { /// Parse a hook decision from JSON stdout and exit code. fn parse_decision(exit_code: i32, stdout: &str) -> HookDecision { if exit_code == 0 { @@ -47,31 +88,15 @@ impl CommandHookExecutor { } } } -} -#[async_trait] -impl HookExecutor for CommandHookExecutor { - async fn execute( - &self, + /// Execute a command hook (sandbox or host). + async fn execute_command( definition: &HookDefinition, + command: &str, context: &HookContext, sandbox: &dyn Sandbox, work_dir: Option<&Path>, - ) -> HookResult { - let start = Instant::now(); - let command = match definition.resolved_hook_type() { - Some(super::config::HookType::Command { ref command }) => command.clone(), - _ => { - return HookResult { - hook_name: definition.name.clone(), - decision: HookDecision::Block { - reason: Some("no command specified".into()), - }, - duration_ms: 0, - }; - } - }; - + ) -> HookDecision { let context_json = serde_json::to_string(context).unwrap_or_default(); let timeout_ms = definition.timeout().as_millis() as u64; @@ -83,8 +108,7 @@ impl HookExecutor for CommandHookExecutor { env_vars.insert("ARC_NODE_ID".to_string(), node_id.clone()); } - let decision = if definition.runs_in_sandbox() { - // Write context to a unique temp file, pass path as env var + if definition.runs_in_sandbox() { let ctx_path = format!( "/tmp/arc-hook-context-{}.json", std::time::SystemTime::now() @@ -96,7 +120,7 @@ impl HookExecutor for CommandHookExecutor { env_vars.insert("ARC_HOOK_CONTEXT".to_string(), ctx_path.clone()); } match sandbox - .exec_command(&command, timeout_ms, None, Some(&env_vars), None) + .exec_command(command, timeout_ms, None, Some(&env_vars), None) .await { Ok(result) => Self::parse_decision(result.exit_code, &result.stdout), @@ -105,23 +129,20 @@ impl HookExecutor for CommandHookExecutor { }, } } else { - // Run on host via sh -c let mut cmd = std::process::Command::new("sh"); - cmd.arg("-c").arg(&command); + cmd.arg("-c").arg(command); if let Some(wd) = work_dir { cmd.current_dir(wd); } for (k, v) in &env_vars { cmd.env(k, v); } - // Pipe context JSON to stdin cmd.stdin(std::process::Stdio::piped()); cmd.stdout(std::process::Stdio::piped()); cmd.stderr(std::process::Stdio::piped()); match cmd.spawn() { Ok(mut child) => { - // Write context to stdin if let Some(mut stdin) = child.stdin.take() { use std::io::Write; let _ = stdin.write_all(context_json.as_bytes()); @@ -141,6 +162,97 @@ impl HookExecutor for CommandHookExecutor { reason: Some(format!("command spawn failed: {e}")), }, } + } + } + + /// Execute an HTTP hook: POST context JSON and parse the response. + /// Fail-open: non-2xx and connection errors return `Proceed`. + async fn execute_http( + url: &str, + headers: &Option>, + allowed_env_vars: &[String], + context: &HookContext, + timeout: std::time::Duration, + ) -> HookDecision { + let client = reqwest::Client::builder() + .timeout(timeout) + .build() + .unwrap_or_default(); + + let mut request = client.post(url).json(context); + + if let Some(hdrs) = headers { + for (key, value) in hdrs { + let interpolated = interpolate_env_vars(value, allowed_env_vars); + request = request.header(key, interpolated); + } + } + + let response = match request.send().await { + Ok(resp) => resp, + Err(e) => { + tracing::warn!(url, error = %e, "HTTP hook request failed, proceeding"); + return HookDecision::Proceed; + } + }; + + if !response.status().is_success() { + tracing::warn!( + url, + status = response.status().as_u16(), + "HTTP hook returned non-2xx, proceeding" + ); + return HookDecision::Proceed; + } + + let body = match response.text().await { + Ok(text) => text, + Err(e) => { + tracing::warn!(url, error = %e, "HTTP hook body read failed, proceeding"); + return HookDecision::Proceed; + } + }; + + if body.trim().is_empty() { + return HookDecision::Proceed; + } + + match serde_json::from_str::(body.trim()) { + Ok(decision) => decision, + Err(e) => { + tracing::warn!(url, error = %e, "HTTP hook response parse failed, proceeding"); + HookDecision::Proceed + } + } + } +} + +#[async_trait] +impl HookExecutor for HookExecutorImpl { + async fn execute( + &self, + definition: &HookDefinition, + context: &HookContext, + sandbox: &dyn Sandbox, + work_dir: Option<&Path>, + ) -> HookResult { + let start = Instant::now(); + + let decision = match definition.resolved_hook_type() { + Some(HookType::Command { ref command }) => { + Self::execute_command(definition, command, context, sandbox, work_dir).await + } + Some(HookType::Http { + ref url, + ref headers, + ref allowed_env_vars, + }) => { + Self::execute_http(url, headers, allowed_env_vars, context, definition.timeout()) + .await + } + None => HookDecision::Block { + reason: Some("no hook type specified".into()), + }, }; let duration_ms = start.elapsed().as_millis() as u64; @@ -152,6 +264,7 @@ impl HookExecutor for CommandHookExecutor { } } + #[cfg(test)] mod tests { use super::*; @@ -178,7 +291,7 @@ mod tests { #[test] fn parse_decision_exit_0_proceed() { assert_eq!( - CommandHookExecutor::parse_decision(0, ""), + HookExecutorImpl::parse_decision(0, ""), HookDecision::Proceed ); } @@ -187,7 +300,7 @@ mod tests { fn parse_decision_exit_0_with_json() { let json = r#"{"decision": "skip", "reason": "not needed"}"#; assert_eq!( - CommandHookExecutor::parse_decision(0, json), + HookExecutorImpl::parse_decision(0, json), HookDecision::Skip { reason: Some("not needed".into()) } @@ -197,7 +310,7 @@ mod tests { #[test] fn parse_decision_exit_2_block() { assert!(matches!( - CommandHookExecutor::parse_decision(2, ""), + HookExecutorImpl::parse_decision(2, ""), HookDecision::Block { .. } )); } @@ -206,7 +319,7 @@ mod tests { fn parse_decision_exit_2_with_json() { let json = r#"{"decision": "skip", "reason": "skipping"}"#; assert_eq!( - CommandHookExecutor::parse_decision(2, json), + HookExecutorImpl::parse_decision(2, json), HookDecision::Skip { reason: Some("skipping".into()) } @@ -216,7 +329,7 @@ mod tests { #[test] fn parse_decision_exit_1_block() { assert!(matches!( - CommandHookExecutor::parse_decision(1, ""), + HookExecutorImpl::parse_decision(1, ""), HookDecision::Block { .. } )); } @@ -225,7 +338,7 @@ mod tests { fn parse_decision_exit_0_override() { let json = r#"{"decision": "override", "edge_to": "node_b"}"#; assert_eq!( - CommandHookExecutor::parse_decision(0, json), + HookExecutorImpl::parse_decision(0, json), HookDecision::Override { edge_to: "node_b".into() } @@ -234,7 +347,7 @@ mod tests { #[tokio::test] async fn command_executor_host_success() { - let executor = CommandHookExecutor; + let executor = HookExecutorImpl; let def = make_definition("exit 0"); let ctx = make_context(); let sandbox = arc_agent::LocalSandbox::new(std::env::current_dir().unwrap()); @@ -245,7 +358,7 @@ mod tests { #[tokio::test] async fn command_executor_host_failure() { - let executor = CommandHookExecutor; + let executor = HookExecutorImpl; let def = make_definition("exit 1"); let ctx = make_context(); let sandbox = arc_agent::LocalSandbox::new(std::env::current_dir().unwrap()); @@ -255,7 +368,7 @@ mod tests { #[tokio::test] async fn command_executor_host_skip_via_exit_2() { - let executor = CommandHookExecutor; + let executor = HookExecutorImpl; let def = make_definition("exit 2"); let ctx = make_context(); let sandbox = arc_agent::LocalSandbox::new(std::env::current_dir().unwrap()); @@ -265,7 +378,7 @@ mod tests { #[tokio::test] async fn command_executor_host_json_decision() { - let executor = CommandHookExecutor; + let executor = HookExecutorImpl; let def = make_definition(r#"echo '{"decision": "skip", "reason": "test skip"}'"#); let ctx = make_context(); @@ -281,7 +394,7 @@ mod tests { #[tokio::test] async fn command_executor_env_vars_set() { - let executor = CommandHookExecutor; + let executor = HookExecutorImpl; // Print env vars to stdout for verification let def = make_definition("echo $ARC_EVENT:$ARC_RUN_ID:$ARC_WORKFLOW"); let mut ctx = make_context(); @@ -292,16 +405,13 @@ mod tests { } #[tokio::test] - async fn command_executor_no_command_blocks() { - let executor = CommandHookExecutor; + async fn no_hook_type_blocks() { + let executor = HookExecutorImpl; let def = HookDefinition { name: None, event: HookEvent::StageStart, command: None, - hook_type: Some(HookType::Http { - url: "http://example.com".into(), - headers: None, - }), + hook_type: None, matcher: None, blocking: None, timeout_ms: None, @@ -312,4 +422,223 @@ mod tests { let result = executor.execute(&def, &ctx, &sandbox, None).await; assert!(matches!(result.decision, HookDecision::Block { .. })); } + + // --- interpolate_env_vars tests --- + + #[test] + fn interpolate_resolves_allowed_var() { + std::env::set_var("ARC_TEST_KEY_1", "secret123"); + let result = interpolate_env_vars( + "Bearer $ARC_TEST_KEY_1", + &["ARC_TEST_KEY_1".to_string()], + ); + assert_eq!(result, "Bearer secret123"); + std::env::remove_var("ARC_TEST_KEY_1"); + } + + #[test] + fn interpolate_resolves_braced_var() { + std::env::set_var("ARC_TEST_KEY_2", "val"); + let result = interpolate_env_vars( + "x${ARC_TEST_KEY_2}y", + &["ARC_TEST_KEY_2".to_string()], + ); + assert_eq!(result, "xvaly"); + std::env::remove_var("ARC_TEST_KEY_2"); + } + + #[test] + fn interpolate_unlisted_var_becomes_empty() { + std::env::set_var("ARC_TEST_KEY_3", "should_not_appear"); + let result = interpolate_env_vars( + "prefix-$ARC_TEST_KEY_3-suffix", + &[], + ); + assert_eq!(result, "prefix--suffix"); + std::env::remove_var("ARC_TEST_KEY_3"); + } + + #[test] + fn interpolate_missing_var_becomes_empty() { + std::env::remove_var("ARC_TEST_NOEXIST"); + let result = interpolate_env_vars( + "a$ARC_TEST_NOEXIST-b", + &["ARC_TEST_NOEXIST".to_string()], + ); + assert_eq!(result, "a-b"); + } + + #[test] + fn interpolate_no_vars_passes_through() { + assert_eq!(interpolate_env_vars("plain text", &[]), "plain text"); + } + + #[test] + fn interpolate_mixed_text() { + std::env::set_var("ARC_TEST_A", "hello"); + std::env::set_var("ARC_TEST_B", "world"); + let result = interpolate_env_vars( + "$ARC_TEST_A ${ARC_TEST_B}!", + &["ARC_TEST_A".to_string(), "ARC_TEST_B".to_string()], + ); + assert_eq!(result, "hello world!"); + std::env::remove_var("ARC_TEST_A"); + std::env::remove_var("ARC_TEST_B"); + } + + // --- HTTP hook execution tests --- + + #[tokio::test] + async fn http_hook_posts_json_and_parses_decision() { + let mut server = mockito::Server::new_async().await; + let mock = server + .mock("POST", "/hook") + .match_header("content-type", "application/json") + .with_status(200) + .with_body(r#"{"decision": "skip", "reason": "not needed"}"#) + .create_async() + .await; + + let decision = HookExecutorImpl::execute_http( + &format!("{}/hook", server.url()), + &None, + &[], + &make_context(), + std::time::Duration::from_secs(5), + ) + .await; + + mock.assert_async().await; + assert_eq!( + decision, + HookDecision::Skip { + reason: Some("not needed".into()) + } + ); + } + + #[tokio::test] + async fn http_hook_empty_2xx_returns_proceed() { + let mut server = mockito::Server::new_async().await; + let mock = server + .mock("POST", "/hook") + .with_status(200) + .with_body("") + .create_async() + .await; + + let decision = HookExecutorImpl::execute_http( + &format!("{}/hook", server.url()), + &None, + &[], + &make_context(), + std::time::Duration::from_secs(5), + ) + .await; + + mock.assert_async().await; + assert_eq!(decision, HookDecision::Proceed); + } + + #[tokio::test] + async fn http_hook_non_2xx_returns_proceed() { + let mut server = mockito::Server::new_async().await; + let mock = server + .mock("POST", "/hook") + .with_status(500) + .with_body("Internal Server Error") + .create_async() + .await; + + let decision = HookExecutorImpl::execute_http( + &format!("{}/hook", server.url()), + &None, + &[], + &make_context(), + std::time::Duration::from_secs(5), + ) + .await; + + mock.assert_async().await; + assert_eq!(decision, HookDecision::Proceed); + } + + #[tokio::test] + async fn http_hook_connection_failure_returns_proceed() { + let decision = HookExecutorImpl::execute_http( + "http://127.0.0.1:1", + &None, + &[], + &make_context(), + std::time::Duration::from_secs(1), + ) + .await; + + assert_eq!(decision, HookDecision::Proceed); + } + + #[tokio::test] + async fn http_hook_sends_interpolated_headers() { + std::env::set_var("ARC_TEST_TOKEN", "my-secret"); + + let mut server = mockito::Server::new_async().await; + let mock = server + .mock("POST", "/hook") + .match_header("authorization", "Bearer my-secret") + .with_status(200) + .with_body("") + .create_async() + .await; + + let headers = HashMap::from([ + ("Authorization".to_string(), "Bearer $ARC_TEST_TOKEN".to_string()), + ]); + + let decision = HookExecutorImpl::execute_http( + &format!("{}/hook", server.url()), + &Some(headers), + &["ARC_TEST_TOKEN".to_string()], + &make_context(), + std::time::Duration::from_secs(5), + ) + .await; + + mock.assert_async().await; + assert_eq!(decision, HookDecision::Proceed); + std::env::remove_var("ARC_TEST_TOKEN"); + } + + #[tokio::test] + async fn executor_dispatches_http_hook() { + let mut server = mockito::Server::new_async().await; + let mock = server + .mock("POST", "/hook") + .with_status(200) + .with_body(r#"{"decision": "proceed"}"#) + .create_async() + .await; + + let executor = HookExecutorImpl; + let def = HookDefinition { + name: Some("http-test".into()), + event: HookEvent::StageStart, + command: None, + hook_type: Some(HookType::Http { + url: format!("{}/hook", server.url()), + headers: None, + allowed_env_vars: vec![], + }), + matcher: None, + blocking: None, + timeout_ms: Some(5000), + sandbox: Some(false), + }; + let ctx = make_context(); + let sandbox = arc_agent::LocalSandbox::new(std::env::current_dir().unwrap()); + let result = executor.execute(&def, &ctx, &sandbox, None).await; + + mock.assert_async().await; + assert_eq!(result.decision, HookDecision::Proceed); + assert_eq!(result.hook_name.as_deref(), Some("http-test")); + } } diff --git a/crates/arc-workflows/src/hook/runner.rs b/crates/arc-workflows/src/hook/runner.rs index b3e27fe3b..0bc32ed2a 100644 --- a/crates/arc-workflows/src/hook/runner.rs +++ b/crates/arc-workflows/src/hook/runner.rs @@ -5,7 +5,7 @@ use std::sync::Arc; use arc_agent::Sandbox; use super::config::{HookConfig, HookDefinition}; -use super::executor::{CommandHookExecutor, HookExecutor}; +use super::executor::{HookExecutorImpl, HookExecutor}; use super::types::{HookContext, HookDecision}; /// Central orchestrator: filters matching hooks, executes them, merges decisions. @@ -22,7 +22,7 @@ impl HookRunner { let compiled_matchers = Self::compile_matchers(&config); Self { config, - command_executor: Arc::new(CommandHookExecutor), + command_executor: Arc::new(HookExecutorImpl), compiled_matchers, } }