mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-08-25 14:53:34 +00:00
feat(bridge): wire v1 security controls into engine v2 adapter
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) <[email protected]>
This commit is contained in:
@@ -304,7 +304,7 @@ impl Agent {
|
||||
self.deps.workspace.as_ref()
|
||||
}
|
||||
|
||||
pub(super) fn hooks(&self) -> &Arc<HookRegistry> {
|
||||
pub(crate) fn hooks(&self) -> &Arc<HookRegistry> {
|
||||
&self.deps.hooks
|
||||
}
|
||||
|
||||
|
||||
+161
-36
@@ -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<ToolRegistry>,
|
||||
safety: Arc<SafetyLayer>,
|
||||
hooks: Arc<HookRegistry>,
|
||||
/// Tools the user has approved with "always" (persists within session).
|
||||
auto_approved: RwLock<HashSet<String>>,
|
||||
}
|
||||
|
||||
impl EffectBridgeAdapter {
|
||||
pub fn new(tools: Arc<ToolRegistry>, safety: Arc<SafetyLayer>) -> Self {
|
||||
Self { tools, safety }
|
||||
pub fn new(
|
||||
tools: Arc<ToolRegistry>,
|
||||
safety: Arc<SafetyLayer>,
|
||||
hooks: Arc<HookRegistry>,
|
||||
) -> 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<ActionResult, EngineError> {
|
||||
// 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::<serde_json::Value>(&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<Vec<ActionDef>, 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)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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());
|
||||
|
||||
Reference in New Issue
Block a user