mirror of
https://github.com/nearai/ironclaw.git
synced 2026-09-03 08:06:01 +08:00
* feat(context): add approval_context field to JobContext Add approval_context to JobContext so tools can propagate approval information when executing sub-tools. This enables tools like build_software to properly check approvals for shell, write_file, etc. - Add approval_context: Option<ApprovalContext> field to JobContext - Add with_approval_context() builder method - Add check_approval_in_context() helper for tools to verify permissions - Default JobContext now includes autonomous approval context Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(worker): check job-level approval context before executing tools Move job context fetch before approval check and add job-level approval context checking. Job-level context takes precedence over worker-level, allowing tools like build_software to set specific allowed sub-tools while maintaining the fallback to worker-level approval for normal operations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(scheduler): propagate approval_context to JobContext Store approval_context from dispatch into JobContext so it's available to tools during execution. This completes the chain: scheduler -> job context -> tools -> sub-tools. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(builder): use approval context for sub-tool execution Update build_software to create a JobContext with build-specific approval permissions and check approval before executing sub-tools. This allows the builder to work in autonomous contexts (web UI, routines) while maintaining security by only allowing specific build-related tools. Allowed tools: shell, read_file, write_file, list_dir, apply_patch, http Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(db): initialize approval_context as None in job restoration When restoring jobs from database, set approval_context to None. The context will be populated by the scheduler on next dispatch if needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: add comprehensive approval context tests Add tests for: - JobContext default includes approval_context - with_approval_context() builder method - Autonomous context blocks Always-approved tools unless explicitly allowed - autonomous_with_tools allows specific tools - Builder tool approval context configuration Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): address critical approval context security issues This commit addresses all security concerns raised in PR review: 1. Revert JobContext::default() to approval_context: None - Previously set ApprovalContext::autonomous() which was too permissive - Secure default requires explicit opt-in for autonomous execution - Any code using JobContext::default() now correctly blocks non-Never tools 2. Fix check_approval_in_context() to match worker behavior - Previously returned Ok(()) when approval_context was None (insecure) - Now uses ApprovalContext::is_blocked_or_default() for consistency - Prevents privilege escalation through sub-tool execution paths 3. Remove "http" from builder's allowed tools - Building software doesn't require direct http tool access - Shell commands (cargo, npm, pip) handle dependency fetching - Reduces attack surface for builder tool execution 4. Update tests to reflect new secure defaults - Tests now verify JobContext::default() blocks non-Never tools - New test added for secure default behavior Security review references: - Issue #1: JobContext::default() behavioral change - Issue #3: check_approval_in_context more permissive than worker check - Issue #4: Builder allows http without justification Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(worker): implement additive approval semantics for job + worker checks This addresses the remaining security review concern from PR #1125. Previously, the worker used "precedence" semantics where job-level approval context would completely bypass worker-level checks. This meant a tool's job-level context could potentially override worker-level restrictions. Changes: - Worker now checks BOTH job-level AND worker-level approval contexts - Tool is blocked if EITHER level blocks it (additive/intersection semantics) - Maintains defense in depth: job-level cannot bypass worker-level restrictions Tests added: - test_additive_approval_semantics_both_levels_must_approve: verifies job-level blocks take effect even when worker-level allows - test_additive_approval_worker_block_overrides_job_allow: verifies worker-level blocks take effect even when job-level allows - test_additive_approval_both_levels_allow: verifies tool is allowed only when both levels approve Security review reference: - Issue #3 from @G7CNF: "document or enforce additive semantics for job + worker approval checks" - Issue #2 from @zmanian: "Job-level context bypasses worker-level entirely" Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): address PR #1125 review feedback - Restore requirement-aware is_blocked() semantics: Never and UnlessAutoApproved tools pass in autonomous context, Only Always tools require explicit allowlist entry - Use AutonomousUnavailable error (with descriptive reason) instead of generic AuthRequired for approval blocking in worker - Deduplicate approval_context propagation in scheduler dispatch (single update_context_and_get call instead of duplicated blocks) - Remove http from builder tool allowlist (shell handles network) - Add TODO comments for serde(skip) losing approval_context on DB restore in both libsql and postgres backends - Add tests: Never tools in additive model, builder unlisted tool blocking Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(worker): remove duplicate approval check and use normalized params - Remove pre-existing worker-level-only approval check (lines 561-567) that duplicated the new additive check, using a different error type and missing job-level context - Use normalized_params (not raw params) for requires_approval() so parameter-dependent approval (e.g. shell destructive detection) works correctly with coerced values - Remove unused autonomous_unavailable_error import - Add comment documenting unreachable else branch in scheduler Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: ilblackdragon@gmail.com <ilblackdragon@gmail.com>
288 lines
8.7 KiB
Rust
288 lines
8.7 KiB
Rust
//! Tests for tool approval context propagation.
|
|
//!
|
|
//! Verifies that:
|
|
//! - JobContext carries approval_context through the execution chain
|
|
//! - Builder sub-tools use proper approval checks
|
|
//! - Worker checks job-level approval context
|
|
|
|
use ironclaw::context::JobContext;
|
|
use ironclaw::tools::{
|
|
ApprovalContext, ApprovalRequirement, Tool, ToolError, ToolOutput, check_approval_in_context,
|
|
};
|
|
|
|
/// A simple test tool that requires approval.
|
|
#[derive(Debug)]
|
|
struct TestTool {
|
|
approval_req: ApprovalRequirement,
|
|
}
|
|
|
|
#[async_trait::async_trait]
|
|
impl Tool for TestTool {
|
|
fn name(&self) -> &str {
|
|
"test_tool"
|
|
}
|
|
|
|
fn description(&self) -> &str {
|
|
"A test tool for approval checking"
|
|
}
|
|
|
|
fn parameters_schema(&self) -> serde_json::Value {
|
|
serde_json::json!({
|
|
"type": "object",
|
|
"properties": {},
|
|
})
|
|
}
|
|
|
|
async fn execute(
|
|
&self,
|
|
_params: serde_json::Value,
|
|
_ctx: &JobContext,
|
|
) -> Result<ToolOutput, ToolError> {
|
|
Ok(ToolOutput::text("ok", std::time::Duration::from_millis(1)))
|
|
}
|
|
|
|
fn requires_approval(&self, _params: &serde_json::Value) -> ApprovalRequirement {
|
|
self.approval_req
|
|
}
|
|
}
|
|
|
|
#[test]
|
|
fn test_job_context_default_has_no_approval_context() {
|
|
let ctx = JobContext::default();
|
|
assert!(
|
|
ctx.approval_context.is_none(),
|
|
"JobContext::default() should NOT have approval_context set for security"
|
|
);
|
|
}
|
|
|
|
#[test]
|
|
fn test_job_context_with_approval_context() {
|
|
let ctx =
|
|
JobContext::new("Test", "Test job").with_approval_context(ApprovalContext::autonomous());
|
|
assert!(ctx.approval_context.is_some());
|
|
}
|
|
|
|
#[test]
|
|
fn test_approval_context_autonomous_allows_unless_auto_approved() {
|
|
let ctx =
|
|
JobContext::new("Test", "Test job").with_approval_context(ApprovalContext::autonomous());
|
|
let tool = TestTool {
|
|
approval_req: ApprovalRequirement::UnlessAutoApproved,
|
|
};
|
|
|
|
// Check should pass for UnlessAutoApproved in autonomous context
|
|
check_approval_in_context(
|
|
&ctx,
|
|
"test_tool",
|
|
tool.requires_approval(&serde_json::json!({})),
|
|
)
|
|
.expect("UnlessAutoApproved should be allowed in autonomous context");
|
|
}
|
|
|
|
#[test]
|
|
fn test_approval_context_autonomous_blocks_always() {
|
|
let ctx =
|
|
JobContext::new("Test", "Test job").with_approval_context(ApprovalContext::autonomous());
|
|
let tool = TestTool {
|
|
approval_req: ApprovalRequirement::Always,
|
|
};
|
|
|
|
// Check should fail for Always in autonomous context without explicit allow
|
|
let result = check_approval_in_context(
|
|
&ctx,
|
|
"test_tool",
|
|
tool.requires_approval(&serde_json::json!({})),
|
|
);
|
|
assert!(
|
|
result.is_err(),
|
|
"Always should be blocked in autonomous context"
|
|
);
|
|
assert!(matches!(result, Err(ToolError::NotAuthorized(_))));
|
|
}
|
|
|
|
#[test]
|
|
fn test_approval_context_autonomous_with_tools_allows_specific() {
|
|
let ctx = JobContext::new("Test", "Test job").with_approval_context(
|
|
ApprovalContext::autonomous_with_tools(["shell".to_string(), "read_file".to_string()]),
|
|
);
|
|
let tool = TestTool {
|
|
approval_req: ApprovalRequirement::Always,
|
|
};
|
|
|
|
// shell should be allowed (explicitly listed)
|
|
check_approval_in_context(
|
|
&ctx,
|
|
"shell",
|
|
tool.requires_approval(&serde_json::json!({})),
|
|
)
|
|
.expect("Listed tool should be allowed");
|
|
|
|
// read_file should be allowed (explicitly listed)
|
|
check_approval_in_context(
|
|
&ctx,
|
|
"read_file",
|
|
tool.requires_approval(&serde_json::json!({})),
|
|
)
|
|
.expect("Listed tool should be allowed");
|
|
|
|
// write_file should be blocked (not listed)
|
|
let result = check_approval_in_context(
|
|
&ctx,
|
|
"write_file",
|
|
tool.requires_approval(&serde_json::json!({})),
|
|
);
|
|
assert!(result.is_err(), "Non-listed Always tool should be blocked");
|
|
}
|
|
|
|
#[test]
|
|
fn test_builder_tools_approval_context() {
|
|
// Verify the builder creates the correct approval context
|
|
let ctx = JobContext::new("Test", "Test job").with_approval_context(
|
|
ApprovalContext::autonomous_with_tools([
|
|
"shell".into(),
|
|
"read_file".into(),
|
|
"write_file".into(),
|
|
"list_dir".into(),
|
|
"apply_patch".into(),
|
|
]),
|
|
);
|
|
|
|
let tool = TestTool {
|
|
approval_req: ApprovalRequirement::Always,
|
|
};
|
|
|
|
// All build tools should be allowed
|
|
for tool_name in &[
|
|
"shell",
|
|
"read_file",
|
|
"write_file",
|
|
"list_dir",
|
|
"apply_patch",
|
|
] {
|
|
check_approval_in_context(
|
|
&ctx,
|
|
tool_name,
|
|
tool.requires_approval(&serde_json::json!({})),
|
|
)
|
|
.unwrap_or_else(|e| panic!("Builder tool '{}' should be allowed, got: {}", tool_name, e));
|
|
}
|
|
|
|
// Non-build tools should be blocked
|
|
let result = check_approval_in_context(
|
|
&ctx,
|
|
"create_job",
|
|
tool.requires_approval(&serde_json::json!({})),
|
|
);
|
|
assert!(result.is_err(), "Non-build Always tool should be blocked");
|
|
}
|
|
|
|
#[test]
|
|
fn test_default_context_blocks_non_never_tools() {
|
|
// JobContext::default() has no approval_context, which should block
|
|
// all non-Never tools (secure default)
|
|
let ctx = JobContext::default();
|
|
|
|
let tool = TestTool {
|
|
approval_req: ApprovalRequirement::UnlessAutoApproved,
|
|
};
|
|
|
|
// UnlessAutoApproved should be blocked with no approval_context
|
|
let result = check_approval_in_context(
|
|
&ctx,
|
|
"test_tool",
|
|
tool.requires_approval(&serde_json::json!({})),
|
|
);
|
|
assert!(
|
|
result.is_err(),
|
|
"UnlessAutoApproved should be blocked with no approval_context"
|
|
);
|
|
|
|
let always_tool = TestTool {
|
|
approval_req: ApprovalRequirement::Always,
|
|
};
|
|
let result = check_approval_in_context(
|
|
&ctx,
|
|
"test_tool",
|
|
always_tool.requires_approval(&serde_json::json!({})),
|
|
);
|
|
assert!(
|
|
result.is_err(),
|
|
"Always should be blocked with no approval_context"
|
|
);
|
|
|
|
// Never should still be allowed
|
|
let never_tool = TestTool {
|
|
approval_req: ApprovalRequirement::Never,
|
|
};
|
|
check_approval_in_context(
|
|
&ctx,
|
|
"test_tool",
|
|
never_tool.requires_approval(&serde_json::json!({})),
|
|
)
|
|
.expect("Never should be allowed even with no approval_context");
|
|
}
|
|
|
|
#[test]
|
|
fn test_never_tools_allowed_in_additive_model() {
|
|
// Never tools (echo, time, etc.) should always be allowed regardless of
|
|
// approval context configuration - they don't need to be in the allowlist.
|
|
let ctx = JobContext::new("Test", "Test job").with_approval_context(
|
|
ApprovalContext::autonomous_with_tools(["shell".to_string()]),
|
|
);
|
|
|
|
// A Never tool not in the allowlist should still be allowed
|
|
check_approval_in_context(&ctx, "echo", ApprovalRequirement::Never)
|
|
.expect("Never tools should pass without being in the allowlist");
|
|
|
|
// And of course a Never tool in the allowlist should also be allowed
|
|
check_approval_in_context(&ctx, "shell", ApprovalRequirement::Never)
|
|
.expect("Never tools in the allowlist should also pass");
|
|
|
|
// But an Always tool not in the allowlist should be blocked
|
|
let result = check_approval_in_context(&ctx, "echo", ApprovalRequirement::Always);
|
|
assert!(
|
|
result.is_err(),
|
|
"Always tools NOT in allowlist should be blocked"
|
|
);
|
|
}
|
|
|
|
#[test]
|
|
fn test_builder_execute_build_tool_blocks_unlisted_tool() {
|
|
// The builder creates a context with specific allowed tools.
|
|
// Tools NOT in the builder's allowlist should be blocked.
|
|
let builder_ctx = JobContext::new("Build", "Building software").with_approval_context(
|
|
ApprovalContext::autonomous_with_tools([
|
|
"shell".into(),
|
|
"read_file".into(),
|
|
"write_file".into(),
|
|
"list_dir".into(),
|
|
"apply_patch".into(),
|
|
]),
|
|
);
|
|
|
|
// Builder-allowed tools should pass
|
|
for tool_name in &[
|
|
"shell",
|
|
"read_file",
|
|
"write_file",
|
|
"list_dir",
|
|
"apply_patch",
|
|
] {
|
|
check_approval_in_context(&builder_ctx, tool_name, ApprovalRequirement::Always)
|
|
.unwrap_or_else(|e| {
|
|
panic!("Builder tool '{}' should be allowed, got: {}", tool_name, e)
|
|
});
|
|
}
|
|
|
|
// Tools NOT in builder's allowlist should be blocked (e.g., http, create_job, message)
|
|
for tool_name in &["http", "create_job", "message", "secret_save"] {
|
|
let result =
|
|
check_approval_in_context(&builder_ctx, tool_name, ApprovalRequirement::Always);
|
|
assert!(
|
|
result.is_err(),
|
|
"Tool '{}' should be blocked by builder context (not in allowlist)",
|
|
tool_name
|
|
);
|
|
}
|
|
}
|