Files
ironclaw/tests/tool_approval_context.rs
Roel van der Ven 42623ed111 fix(builder): add approval context propagation for sub-tool execution (#1125)
* 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>
2026-03-30 20:53:57 -07:00

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
);
}
}