diff --git a/config.example.toml b/config.example.toml index 4ee86ff3..20ea5f8e 100644 --- a/config.example.toml +++ b/config.example.toml @@ -167,9 +167,9 @@ sandbox_mode = "workspace-write" # read-only | workspace-write | danger-full-acc # prompt_suggestion = true # opt-in: show ghost-text follow-up question in composer after each turn # Typed permission rules live in a sibling `permissions.toml` file, not in -# config.toml. This schema slice is ask-only and is parsed for follow-up -# approval-flow wiring; allow/deny records and UI persistence are intentionally -# out of scope here. +# config.toml. This shape is ask-only and feeds the execution policy engine; +# allow/deny records, glob expansion, and UI persistence are intentionally out +# of scope here. # # Example ~/.codewhale/permissions.toml: # diff --git a/crates/app-server/src/lib.rs b/crates/app-server/src/lib.rs index 412a5a30..262a920c 100644 --- a/crates/app-server/src/lib.rs +++ b/crates/app-server/src/lib.rs @@ -12,7 +12,6 @@ use axum::{Json, Router}; use codewhale_agent::ModelRegistry; use codewhale_config::{CliRuntimeOverrides, ConfigStore}; use codewhale_core::Runtime; -use codewhale_execpolicy::ExecPolicyEngine; use codewhale_hooks::{HookDispatcher, JsonlHookSink, StdoutHookSink, UnixSocketHookSink}; use codewhale_mcp::McpManager; use codewhale_protocol::{ @@ -319,6 +318,7 @@ async fn app_handler( fn build_state(config_path: Option, auth_token: Option) -> Result { let store = ConfigStore::load(config_path.clone())?; let config = store.config.clone(); + let exec_policy = store.exec_policy_engine(); let registry = ModelRegistry::default(); let state_db_path = config_path @@ -349,7 +349,7 @@ fn build_state(config_path: Option, auth_token: Option) -> Resu state_store, Arc::new(ToolRegistry::default()), Arc::new(McpManager::default()), - ExecPolicyEngine::new(Vec::new(), Vec::new()), + exec_policy, hooks, ); @@ -1057,6 +1057,43 @@ mod tests { ); } + #[tokio::test] + async fn build_state_loads_permissions_into_runtime_policy() { + let tmp = tempfile::tempdir().expect("tempdir"); + let config_path = tmp.path().join("config.toml"); + fs::write(&config_path, "api_key = \"sk-deepseek-secret\"\n").expect("write config"); + fs::write( + tmp.path().join("permissions.toml"), + r#" + [[rules]] + tool = "exec_shell" + command = "cargo test" + "#, + ) + .expect("write permissions"); + + let state = build_state(Some(config_path), None).expect("state"); + let runtime = state.runtime.lock().await; + let decision = runtime + .exec_policy + .check(codewhale_execpolicy::ExecPolicyContext { + command: "cargo test --workspace", + cwd: "/workspace", + tool: Some("exec_shell"), + path: None, + ask_for_approval: codewhale_execpolicy::AskForApproval::UnlessTrusted, + sandbox_mode: Some("workspace-write"), + }) + .expect("policy check"); + + assert!(decision.allow); + assert!(decision.requires_approval); + assert_eq!( + decision.matched_rule.as_deref(), + Some("tool=exec_shell command=cargo test") + ); + } + #[test] fn non_loopback_bind_without_auth_fails_fast() { let options = AppServerOptions { @@ -1076,7 +1113,10 @@ mod tests { #[tokio::test] async fn stdio_transport_keeps_raw_config_get_for_legacy_clients() { - let state = build_state(None, None).expect("state"); + let tmp = tempfile::tempdir().expect("tempdir"); + let config_path = tmp.path().join("config.toml"); + fs::write(&config_path, "").expect("write config"); + let state = build_state(Some(config_path), None).expect("state"); { let mut cfg = state.config.write().await; cfg.api_key = Some("sk-deepseek-secret".to_string()); diff --git a/crates/config/src/lib.rs b/crates/config/src/lib.rs index 6b3e464e..b1d2d0bd 100644 --- a/crates/config/src/lib.rs +++ b/crates/config/src/lib.rs @@ -10,6 +10,7 @@ use std::sync::OnceLock; use anyhow::{Context, Result, bail}; pub use codewhale_execpolicy::ToolAskRule; +use codewhale_execpolicy::{ExecPolicyEngine, Ruleset}; use codewhale_secrets::SecretSource; pub use codewhale_secrets::Secrets; use serde::{Deserialize, Serialize}; @@ -295,6 +296,11 @@ impl PermissionsToml { pub fn is_empty(&self) -> bool { self.rules.is_empty() } + + #[must_use] + pub fn ruleset(&self) -> Ruleset { + Ruleset::user(Vec::new(), Vec::new()).with_ask_rules(self.rules.clone()) + } } impl ProvidersToml { @@ -2771,6 +2777,15 @@ impl ConfigStore { pub fn permissions_path(&self) -> PathBuf { permissions_path_for_config_path(&self.path) } + + #[must_use] + pub fn exec_policy_engine(&self) -> ExecPolicyEngine { + if self.permissions.is_empty() { + ExecPolicyEngine::new(Vec::new(), Vec::new()) + } else { + ExecPolicyEngine::with_rulesets(vec![self.permissions.ruleset()]) + } + } } /// Process-wide default [`Secrets`] façade. The first caller wins; the @@ -3614,6 +3629,54 @@ action = "mode.agent" let _ = fs::remove_dir_all(dir); } + #[test] + fn config_store_exec_policy_engine_uses_sibling_permissions() { + use std::time::{SystemTime, UNIX_EPOCH}; + + let unique = SystemTime::now() + .duration_since(UNIX_EPOCH) + .expect("clock") + .as_nanos(); + let dir = std::env::temp_dir().join(format!( + "codewhale-permissions-engine-{}-{unique}", + std::process::id() + )); + fs::create_dir_all(&dir).expect("mkdir"); + let config_path = dir.join(CONFIG_FILE_NAME); + fs::write(&config_path, "model = \"deepseek-v4-flash\"\n").expect("write config"); + fs::write( + dir.join(PERMISSIONS_FILE_NAME), + r#" + [[rules]] + tool = "exec_shell" + command = "cargo test" + "#, + ) + .expect("write permissions"); + + let store = ConfigStore::load(Some(config_path)).expect("load config store"); + let decision = store + .exec_policy_engine() + .check(codewhale_execpolicy::ExecPolicyContext { + command: "cargo test --workspace", + cwd: "/workspace", + tool: Some("exec_shell"), + path: None, + ask_for_approval: codewhale_execpolicy::AskForApproval::UnlessTrusted, + sandbox_mode: Some("workspace-write"), + }) + .expect("policy check"); + + assert!(decision.allow); + assert!(decision.requires_approval); + assert_eq!( + decision.matched_rule.as_deref(), + Some("tool=exec_shell command=cargo test") + ); + + let _ = fs::remove_dir_all(dir); + } + struct EnvGuard { deepseek_api_key: Option, deepseek_base_url: Option, diff --git a/crates/core/src/lib.rs b/crates/core/src/lib.rs index 059cc5d0..900e16b2 100644 --- a/crates/core/src/lib.rs +++ b/crates/core/src/lib.rs @@ -1097,11 +1097,12 @@ impl Runtime { ToolPayload::LocalShell { .. } => "exec_shell", _ => call.name.as_str(), }; + let policy_path = permission_path_for_call(&call); let decision = self.exec_policy.check(ExecPolicyContext { command: &command, cwd: &policy_cwd, tool: Some(policy_tool), - path: None, + path: policy_path.as_deref(), ask_for_approval: approval_mode, sandbox_mode: None, })?; @@ -1502,6 +1503,24 @@ fn preview_from_initial_history(initial_history: &InitialHistory) -> String { } } +fn permission_path_for_call(call: &ToolCall) -> Option { + match &call.payload { + ToolPayload::Function { arguments } => serde_json::from_str::(arguments) + .ok() + .and_then(|value| { + value + .get("path") + .and_then(Value::as_str) + .map(str::to_string) + }), + ToolPayload::Mcp { raw_arguments, .. } => raw_arguments + .get("path") + .and_then(Value::as_str) + .map(str::to_string), + ToolPayload::Custom { .. } | ToolPayload::LocalShell { .. } => None, + } +} + fn truncate_preview(value: &str) -> String { value.chars().take(120).collect() } @@ -1808,9 +1827,65 @@ fn job_state_status_to_runtime(status: JobStateStatus) -> JobStatus { #[cfg(test)] mod tests { use super::*; + use codewhale_tools::ToolCallSource; // ── JobManager: lifecycle ────────────────────────────────────────── + #[test] + fn permission_path_for_call_extracts_function_path_argument() { + let call = ToolCall { + name: "read_file".to_string(), + payload: ToolPayload::Function { + arguments: json!({ "path": "README.md" }).to_string(), + }, + source: ToolCallSource::Direct, + raw_tool_call_id: None, + }; + + assert_eq!( + permission_path_for_call(&call).as_deref(), + Some("README.md") + ); + } + + #[test] + fn permission_path_for_call_extracts_mcp_path_argument() { + let call = ToolCall { + name: "mcp_fs_read".to_string(), + payload: ToolPayload::Mcp { + server: "fs".to_string(), + tool: "read".to_string(), + raw_arguments: json!({ "path": "secrets/token.txt" }), + raw_tool_call_id: None, + }, + source: ToolCallSource::Direct, + raw_tool_call_id: None, + }; + + assert_eq!( + permission_path_for_call(&call).as_deref(), + Some("secrets/token.txt") + ); + } + + #[test] + fn permission_path_for_call_ignores_shell_payload() { + let call = ToolCall { + name: "exec_shell".to_string(), + payload: ToolPayload::LocalShell { + params: codewhale_protocol::LocalShellParams { + command: "cargo test".to_string(), + cwd: None, + timeout_ms: None, + }, + }, + source: ToolCallSource::Direct, + raw_tool_call_id: None, + }; + + assert_eq!(permission_path_for_call(&call), None); + } + #[test] fn enqueue_creates_queued_job_with_zero_progress() { let mut jm = JobManager::default(); diff --git a/crates/execpolicy/src/lib.rs b/crates/execpolicy/src/lib.rs index 12897037..d826e213 100644 --- a/crates/execpolicy/src/lib.rs +++ b/crates/execpolicy/src/lib.rs @@ -313,21 +313,26 @@ impl ExecPolicyEngine { self.rulesets .iter() - .flat_map(|ruleset| ruleset.ask_rules.iter()) - .filter(|rule| rule.tool == tool) - .filter(|rule| match rule.command.as_deref() { + .flat_map(|ruleset| { + ruleset + .ask_rules + .iter() + .map(move |rule| (ruleset.layer, rule)) + }) + .filter(|(_, rule)| rule.tool == tool) + .filter(|(_, rule)| match rule.command.as_deref() { Some(command) => self.arity_dict.allow_rule_matches(command, ctx.command), None => true, }) - .filter(|rule| match (rule.path.as_deref(), ctx.path) { + .filter(|(_, rule)| match (rule.path.as_deref(), ctx.path) { (Some(pattern), Some(path)) => { normalize_path_value(pattern) == normalize_path_value(path) } (Some(_), None) => false, (None, _) => true, }) - .max_by_key(|rule| ask_rule_specificity(rule)) - .cloned() + .max_by_key(|(layer, rule)| (*layer, ask_rule_specificity(rule))) + .map(|(_, rule)| rule.clone()) } /// Records an approval key for the current session so subsequent checks skip approval. @@ -377,51 +382,82 @@ impl ExecPolicyEngine { let ask_rule = self.matching_ask_rule(&ctx); - let requirement = match &ctx.ask_for_approval { - AskForApproval::Never => { - if let Some(rule) = &ask_rule { - ExecApprovalRequirement::Forbidden { - reason: format!( - "Typed ask rule '{}' requires approval, but approval policy is never.", - rule.label() - ), - } - } else { - ExecApprovalRequirement::Skip { - bypass_sandbox: false, - proposed_execpolicy_amendment: None, + let mut matched_ask_rule = None; + // Resolve a matching typed ask-rule first. Ask-rules take precedence over + // mode-based handling for everything except `Never` (which forbids, + // because no prompt can be shown) and `Reject { rules: true }` (which + // explicitly rejects rule-exceptions). This ordering is checked against + // the experimental `if let` match-guard the original PR used; it is + // reproduced here with plain control flow for edition-2024 stable. + let ask_rule_requirement = match &ctx.ask_for_approval { + AskForApproval::Never | AskForApproval::Reject { rules: true, .. } => None, + _ => ask_rule.as_ref().map(|rule| { + matched_ask_rule = Some(rule.label()); + ExecApprovalRequirement::NeedsApproval { + reason: format!("Typed ask rule '{}' requires approval.", rule.label()), + proposed_execpolicy_amendment: None, + // A typed ask-rule approval (exec/fn/MCP) must not touch + // network policy. The original PR allow-listed `ctx.cwd` as a + // network host here, which is incorrect and security-relevant: + // approving e.g. an exec rule should never create a network + // allow-entry. Emit no network amendments for ask-rule prompts. + proposed_network_policy_amendments: Vec::new(), + } + }), + }; + + let requirement = if let Some(req) = ask_rule_requirement { + req + } else { + match &ctx.ask_for_approval { + AskForApproval::Never => { + if let Some(rule) = &ask_rule { + matched_ask_rule = Some(rule.label()); + ExecApprovalRequirement::Forbidden { + reason: format!( + "Typed ask rule '{}' requires approval, but approval policy is never.", + rule.label() + ), + } + } else { + ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: None, + } } } + AskForApproval::Reject { rules, .. } if *rules => { + ExecApprovalRequirement::Forbidden { + reason: "Policy is configured to reject rule-exceptions.".to_string(), + } + } + AskForApproval::UnlessTrusted if is_trusted => ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: None, + }, + AskForApproval::OnFailure => ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: None, + }, + _ => ExecApprovalRequirement::NeedsApproval { + reason: if is_trusted { + "Approval requested by policy mode.".to_string() + } else { + "Unmatched command prefix requires approval.".to_string() + }, + proposed_execpolicy_amendment: if is_trusted { + None + } else { + Some(ExecPolicyAmendment { + prefixes: vec![first_token(ctx.command)], + }) + }, + proposed_network_policy_amendments: vec![NetworkPolicyAmendment { + host: ctx.cwd.to_string(), + action: NetworkPolicyRuleAction::Allow, + }], + }, } - AskForApproval::UnlessTrusted if is_trusted => ExecApprovalRequirement::Skip { - bypass_sandbox: false, - proposed_execpolicy_amendment: None, - }, - AskForApproval::OnFailure => ExecApprovalRequirement::Skip { - bypass_sandbox: false, - proposed_execpolicy_amendment: None, - }, - AskForApproval::Reject { rules, .. } if *rules => ExecApprovalRequirement::Forbidden { - reason: "Policy is configured to reject rule-exceptions.".to_string(), - }, - _ => ExecApprovalRequirement::NeedsApproval { - reason: if is_trusted { - "Approval requested by policy mode.".to_string() - } else { - "Unmatched command prefix requires approval.".to_string() - }, - proposed_execpolicy_amendment: if is_trusted { - None - } else { - Some(ExecPolicyAmendment { - prefixes: vec![first_token(ctx.command)], - }) - }, - proposed_network_policy_amendments: vec![NetworkPolicyAmendment { - host: ctx.cwd.to_string(), - action: NetworkPolicyRuleAction::Allow, - }], - }, }; let (allow, requires_approval) = match requirement { @@ -430,12 +466,6 @@ impl ExecPolicyEngine { ExecApprovalRequirement::Forbidden { .. } => (false, false), }; - let matched_ask_rule = if matches!(&ctx.ask_for_approval, AskForApproval::Never) { - ask_rule.map(|rule| rule.label()) - } else { - None - }; - Ok(ExecPolicyDecision { allow, requires_approval, @@ -639,7 +669,7 @@ mod tests { } #[test] - fn typed_ask_rule_is_ignored_outside_never_mode_for_now() { + fn typed_ask_rule_requires_approval_under_unless_trusted() { let engine = ExecPolicyEngine::with_rulesets(vec![ Ruleset::user(vec![], vec![]) .with_ask_rules(vec![ToolAskRule::exec_shell("cargo test")]), @@ -651,18 +681,49 @@ mod tests { assert!(decision.allow); assert!(decision.requires_approval); - assert_eq!(decision.matched_rule, None); + assert_eq!( + decision.matched_rule.as_deref(), + Some("tool=exec_shell command=cargo test") + ); match decision.requirement { ExecApprovalRequirement::NeedsApproval { - proposed_execpolicy_amendment: Some(amendment), + proposed_execpolicy_amendment, + proposed_network_policy_amendments, .. - } => assert_eq!(amendment.prefixes, vec!["cargo"]), - other => panic!("expected unchanged approval behavior, got {other:?}"), + } => { + assert_eq!(proposed_execpolicy_amendment, None); + // A typed ask-rule approval must not allow-list the cwd (or + // anything else) as a network host. See the NeedsApproval arm. + assert!( + proposed_network_policy_amendments.is_empty(), + "ask-rule approval must not propose network amendments, got {proposed_network_policy_amendments:?}" + ); + } + other => panic!("expected typed ask approval, got {other:?}"), } } #[test] - fn typed_ask_rule_does_not_change_allow_deny_precedence() { + fn typed_ask_rule_requires_approval_under_on_failure() { + let engine = ExecPolicyEngine::with_rulesets(vec![ + Ruleset::user(vec![], vec![]) + .with_ask_rules(vec![ToolAskRule::exec_shell("cargo test")]), + ]); + + let decision = engine + .check(ctx("cargo test --workspace", AskForApproval::OnFailure)) + .unwrap(); + + assert!(decision.allow); + assert!(decision.requires_approval); + assert_eq!( + decision.reason(), + "Typed ask rule 'tool=exec_shell command=cargo test' requires approval." + ); + } + + #[test] + fn typed_ask_rule_overrides_trusted_but_not_deny() { let engine = ExecPolicyEngine::with_rulesets(vec![ Ruleset::user( vec!["cargo test".to_string()], @@ -675,8 +736,11 @@ mod tests { .check(ctx("cargo test --workspace", AskForApproval::UnlessTrusted)) .unwrap(); assert!(trusted.allow); - assert!(!trusted.requires_approval); - assert_eq!(trusted.matched_rule.as_deref(), Some("cargo test")); + assert!(trusted.requires_approval); + assert_eq!( + trusted.matched_rule.as_deref(), + Some("tool=exec_shell command=cargo test") + ); let denied = engine .check(ctx("cargo test --danger", AskForApproval::Never)) @@ -690,6 +754,56 @@ mod tests { ); } + #[test] + fn typed_ask_rule_prefers_higher_layer_before_specificity() { + let engine = ExecPolicyEngine::with_rulesets(vec![ + Ruleset::agent(vec![], vec![]) + .with_ask_rules(vec![ToolAskRule::exec_shell("cargo test --workspace")]), + Ruleset::user(vec![], vec![]) + .with_ask_rules(vec![ToolAskRule::exec_shell("cargo test")]), + ]); + + let decision = engine + .check(ctx( + "cargo test --workspace --all-features", + AskForApproval::UnlessTrusted, + )) + .unwrap(); + + assert!(decision.requires_approval); + assert_eq!( + decision.matched_rule.as_deref(), + Some("tool=exec_shell command=cargo test") + ); + } + + #[test] + fn reject_rules_mode_still_forbids_matching_ask_rule() { + let engine = ExecPolicyEngine::with_rulesets(vec![ + Ruleset::user(vec![], vec![]) + .with_ask_rules(vec![ToolAskRule::exec_shell("cargo test")]), + ]); + + let decision = engine + .check(ctx( + "cargo test --workspace", + AskForApproval::Reject { + sandbox_approval: false, + rules: true, + mcp_elicitations: false, + }, + )) + .unwrap(); + + assert!(!decision.allow); + assert!(!decision.requires_approval); + assert_eq!(decision.matched_rule, None); + assert_eq!( + decision.reason(), + "Policy is configured to reject rule-exceptions." + ); + } + #[test] fn typed_ask_rule_label_wins_when_never_blocks_trusted_command() { let engine = ExecPolicyEngine::with_rulesets(vec![ diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 9ab09d9f..01e38485 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -875,8 +875,10 @@ If you are upgrading from older releases: records loaded next to `config.toml`, for example `~/.codewhale/permissions.toml`. This schema foundation accepts `[[rules]]` entries with `tool` plus optional `command` or `path` fields. - It intentionally does not accept typed allow/deny records or provide approval - UI persistence yet. + Loaded rules feed the execution policy engine and force approval in approval + modes that can ask; under `approval_policy = "never"`, matching ask rules are + rejected because no prompt can be shown. This intentionally does not accept + typed allow/deny records, glob expansion, or approval UI persistence yet. - `managed_config_path` (string, optional): managed config file loaded after user/env config. - `requirements_path` (string, optional): requirements file used to enforce allowed approval/sandbox values. - `max_subagents` (int, optional): defaults to `10` and is clamped to `1..=20`.