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, } }