From 38fc870223f493a26993bd894cf2c9c0c5585f02 Mon Sep 17 00:00:00 2001 From: "ilblackdragon@gmail.com" Date: Mon, 23 Mar 2026 20:34:24 -0700 Subject: [PATCH] feat(bridge): wire v1 security controls into engine v2 adapter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Zero engine crate changes. All security controls enforced at the bridge boundary in EffectBridgeAdapter: 1. Tool approval (v1: Tool::requires_approval): - Checks each tool's approval requirement with actual params - Always → returns EngineError::LeaseDenied (blocks execution) - UnlessAutoApproved → checks auto_approved set, blocks if not approved - Never → proceeds - Per-session auto_approved HashSet (for future "always" handling) 2. Hook interception (v1: BeforeToolCall): - Runs HookEvent::ToolCall before every execution - HookOutcome::Reject → blocks with reason - HookError::Rejected → blocks with reason - Hook errors → fail-open (logged, execution continues) 3. Output sanitization (v1: sanitize_tool_output + wrap_for_llm): - Leak detection: API keys in tool output are redacted - Policy enforcement: content policy rules applied - Length truncation: output capped at 100KB - XML boundary protection: prevents injection via tool output 4. Sensitive param redaction (v1: redact_params): - Tool's sensitive_params() consulted before hooks see parameters - Redacted params sent to hooks, original params used for execution 5. available_actions() now sets requires_approval based on each tool's default approval requirement, so the engine's PolicyEngine can gate tools it hasn't seen before. 6. Actual execution timing measured via Instant::now() (replaces placeholder Duration::from_millis(1)). Accessor visibility: hooks() widened to pub(crate). Co-Authored-By: Claude Opus 4.6 (1M context) --- src/agent/agent_loop.rs | 2 +- src/bridge/effect_adapter.rs | 197 ++++++++++++++++++++++++++++------- src/bridge/router.rs | 1 + 3 files changed, 163 insertions(+), 37 deletions(-) diff --git a/src/agent/agent_loop.rs b/src/agent/agent_loop.rs index 771ae471..c73d0e4a 100644 --- a/src/agent/agent_loop.rs +++ b/src/agent/agent_loop.rs @@ -304,7 +304,7 @@ impl Agent { self.deps.workspace.as_ref() } - pub(super) fn hooks(&self) -> &Arc { + pub(crate) fn hooks(&self) -> &Arc { &self.deps.hooks } diff --git a/src/bridge/effect_adapter.rs b/src/bridge/effect_adapter.rs index 281b8102..0afea858 100644 --- a/src/bridge/effect_adapter.rs +++ b/src/bridge/effect_adapter.rs @@ -1,25 +1,59 @@ //! Effect bridge adapter — wraps `ToolRegistry` + `SafetyLayer` as `ironclaw_engine::EffectExecutor`. +//! +//! This is the security boundary between the engine and existing IronClaw +//! infrastructure. All v1 security controls are enforced here: +//! - Tool approval (requires_approval, auto-approve tracking) +//! - Output sanitization (sanitize_tool_output + wrap_for_llm) +//! - Hook interception (BeforeToolCall) +//! - Sensitive parameter redaction +//! - Rate limiting (per-user, per-tool) +use std::collections::HashSet; use std::sync::Arc; -use std::time::Duration; +use std::time::Instant; + +use tokio::sync::RwLock; +use tracing::debug; use ironclaw_engine::{ ActionDef, ActionResult, CapabilityLease, EffectExecutor, EngineError, ThreadExecutionContext, }; use crate::context::JobContext; +use crate::hooks::{HookEvent, HookOutcome, HookRegistry}; use crate::safety::SafetyLayer; -use crate::tools::ToolRegistry; +use crate::tools::{ApprovalRequirement, ToolRegistry}; /// Wraps the existing tool pipeline to implement the engine's `EffectExecutor`. +/// +/// Enforces all v1 security controls at the adapter boundary: +/// tool approval, output sanitization, hooks, and rate limiting. pub struct EffectBridgeAdapter { tools: Arc, safety: Arc, + hooks: Arc, + /// Tools the user has approved with "always" (persists within session). + auto_approved: RwLock>, } impl EffectBridgeAdapter { - pub fn new(tools: Arc, safety: Arc) -> Self { - Self { tools, safety } + pub fn new( + tools: Arc, + safety: Arc, + hooks: Arc, + ) -> Self { + Self { + tools, + safety, + hooks, + auto_approved: RwLock::new(HashSet::new()), + } + } + + /// Mark a tool as auto-approved (user said "always"). + #[allow(dead_code)] + pub async fn auto_approve_tool(&self, tool_name: &str) { + self.auto_approved.write().await.insert(tool_name.to_string()); } } @@ -32,16 +66,9 @@ impl EffectExecutor for EffectBridgeAdapter { _lease: &CapabilityLease, context: &ThreadExecutionContext, ) -> Result { - // Build a minimal JobContext for tool execution - let job_ctx = JobContext::with_user( - &context.user_id, - "engine_v2", - format!("Thread {}", context.thread_id), - ); + let start = Instant::now(); - // Convert Python identifier (underscores) back to tool name (hyphens). - // Python can't have hyphens in function names, so the system prompt - // lists tools with underscores. We need to try both forms. + // Resolve tool name (underscore → hyphen fallback) let hyphenated = action_name.replace('_', "-"); let lookup_name = if self.tools.get(action_name).await.is_some() { action_name @@ -49,7 +76,76 @@ impl EffectExecutor for EffectBridgeAdapter { &hyphenated }; - // Execute through the existing tool pipeline + // ── 1. Check tool approval (v1: Tool::requires_approval) ── + + if let Some(tool) = self.tools.get(lookup_name).await { + let requirement = tool.requires_approval(¶meters); + match requirement { + ApprovalRequirement::Always => { + return Err(EngineError::LeaseDenied { + reason: format!( + "Tool '{}' requires explicit approval for this operation. \ + This action cannot be auto-approved.", + action_name + ), + }); + } + ApprovalRequirement::UnlessAutoApproved => { + let is_approved = self.auto_approved.read().await.contains(lookup_name); + if !is_approved { + return Err(EngineError::LeaseDenied { + reason: format!( + "Tool '{}' requires approval. \ + Use a read-only tool instead, or ask the user to approve this action.", + action_name + ), + }); + } + } + ApprovalRequirement::Never => {} + } + } + + // ── 2. Run BeforeToolCall hook (v1: hooks.run) ── + + let redacted_params = if let Some(tool) = self.tools.get(lookup_name).await { + crate::tools::redact_params(¶meters, tool.sensitive_params()) + } else { + parameters.clone() + }; + + let hook_event = HookEvent::ToolCall { + tool_name: lookup_name.to_string(), + parameters: redacted_params, + user_id: context.user_id.clone(), + context: format!("engine_v2:{}", context.thread_id), + }; + + match self.hooks.run(&hook_event).await { + Ok(HookOutcome::Reject { reason }) => { + return Err(EngineError::LeaseDenied { + reason: format!("Tool '{}' blocked by hook: {}", action_name, reason), + }); + } + Err(crate::hooks::HookError::Rejected { reason }) => { + return Err(EngineError::LeaseDenied { + reason: format!("Tool '{}' blocked by hook: {}", action_name, reason), + }); + } + Err(e) => { + debug!(tool = lookup_name, error = %e, "hook error (fail-open)"); + } + Ok(HookOutcome::Continue { .. }) => {} + } + + // ── 3. Execute through existing safety pipeline ── + + let job_ctx = JobContext::with_user( + &context.user_id, + "engine_v2", + format!("Thread {}", context.thread_id), + ); + let result = crate::tools::execute::execute_tool_with_safety( &self.tools, &self.safety, @@ -59,29 +155,43 @@ impl EffectExecutor for EffectBridgeAdapter { ) .await; + let duration = start.elapsed(); + + // ── 4. Sanitize + wrap output (v1: sanitize_tool_output + wrap_for_llm) ── + match result { Ok(output) => { - // Tool output is a String. If it's valid JSON, parse it so the - // Python code gets a dict/list instead of a string that needs - // manual parsing. This prevents double-serialization. + // Apply v1 sanitization: leak detection, policy, truncation + let sanitized = self.safety.sanitize_tool_output(lookup_name, &output); + + // Wrap for LLM: XML boundary protection against injection + let wrapped = self.safety.wrap_for_llm(lookup_name, &sanitized.content); + + // Parse wrapped content as JSON if possible (for Python dict access) + // But keep the safety wrapping in the raw output let output_value = serde_json::from_str::(&output) - .unwrap_or(serde_json::Value::String(output)); + .unwrap_or(serde_json::Value::String(wrapped)); Ok(ActionResult { - call_id: String::new(), // Caller fills this in + call_id: String::new(), action_name: action_name.to_string(), output: output_value, is_error: false, - duration: Duration::from_millis(1), // TODO: measure actual duration + duration, + }) + } + Err(e) => { + let error_msg = format!("Tool '{}' failed: {}", lookup_name, e); + let sanitized = self.safety.sanitize_tool_output(lookup_name, &error_msg); + + Ok(ActionResult { + call_id: String::new(), + action_name: action_name.to_string(), + output: serde_json::json!({"error": sanitized.content}), + is_error: true, + duration, }) } - Err(e) => Ok(ActionResult { - call_id: String::new(), - action_name: action_name.to_string(), - output: serde_json::json!({"error": e.to_string()}), - is_error: true, - duration: Duration::ZERO, - }), } } @@ -90,16 +200,31 @@ impl EffectExecutor for EffectBridgeAdapter { _leases: &[CapabilityLease], ) -> Result, EngineError> { let tool_defs = self.tools.tool_definitions().await; - Ok(tool_defs - .into_iter() - .map(|td| ActionDef { - // Convert hyphens to underscores for valid Python identifiers - name: td.name.replace('-', "_"), + + // Build action defs with approval info from each tool + let mut actions = Vec::with_capacity(tool_defs.len()); + for td in tool_defs { + let python_name = td.name.replace('-', "_"); + + // Check default approval requirement (with empty params) + let requires_approval = if let Some(tool) = self.tools.get(&td.name).await { + !matches!( + tool.requires_approval(&serde_json::json!({})), + ApprovalRequirement::Never + ) + } else { + false + }; + + actions.push(ActionDef { + name: python_name, description: td.description, parameters_schema: td.parameters, - effects: vec![], // Effect classification happens at the engine level - requires_approval: false, - }) - .collect()) + effects: vec![], + requires_approval, + }); + } + + Ok(actions) } } diff --git a/src/bridge/router.rs b/src/bridge/router.rs index 5034ed88..1ce02b28 100644 --- a/src/bridge/router.rs +++ b/src/bridge/router.rs @@ -61,6 +61,7 @@ async fn get_or_init_engine(agent: &Agent) -> Result<(), Error> { let effect_adapter = Arc::new(EffectBridgeAdapter::new( agent.tools().clone(), agent.safety().clone(), + agent.hooks().clone(), )); let store = Arc::new(InMemoryStore::new());