Add permission rule patterns: per-argument allow and deny lists

index d7b301b..6582c2e 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -1793,7 +1793,7 @@ mod tui { // mutating tool consults policy and may pause on the user's // y/a/n answer (delivered over a oneshot from the event loop). use crate::permissions::{self, Decision, TUI_SESSION}; - let output = match permissions::decision_for(TUI_SESSION, &tc.name) { + let output = match permissions::decision_for(TUI_SESSION, &tc.name, &tc.arguments) { Decision::Allow => crate::tools::execute_tool(&tc.name, &tc.arguments).await, Decision::Deny(reason) => { log::info!(" ✗ {} denied by policy", tc.name); @@ -1813,7 +1813,11 @@ mod tui { crate::tools::execute_tool(&tc.name, &tc.arguments).await } Ok(ApprovalChoice::Session) => { - permissions::grant_for_session(TUI_SESSION, &tc.name); + permissions::grant_for_session( + TUI_SESSION, + &tc.name, + &tc.arguments, + ); crate::tools::execute_tool(&tc.name, &tc.arguments).await } Ok(ApprovalChoice::Deny) => {

index 1631a63..97f898e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1372,7 +1372,11 @@ impl SiGitAgent { // Permission gate: read-only tools pass straight through; a // mutating tool consults policy and may ask the client. - let output = match permissions::decision_for(&session_id.to_string(), &tc.name) { + let output = match permissions::decision_for( + &session_id.to_string(), + &tc.name, + &tc.arguments, + ) { permissions::Decision::Allow => { tools::execute_tool(&tc.name, &tc.arguments).await } @@ -1541,7 +1545,11 @@ impl SiGitAgent { match selected.option_id.0.as_ref() { "allow_once" => PermissionVerdict::Approved, "allow_session" => { - permissions::grant_for_session(&session_id.to_string(), tool_name); + permissions::grant_for_session( + &session_id.to_string(), + tool_name, + arguments, + ); PermissionVerdict::Approved } _ => PermissionVerdict::Denied(permissions::user_denial(tool_name)),

index c631102..375f6a4 100644 --- a/src/permissions.rs +++ b/src/permissions.rs @@ -10,12 +10,30 @@ //! a message telling the model to present a plan instead. Toggled via //! `/plan on|off` (TUI and ACP). //! 2. **Session grants** — "always allow this session", recorded when the user -//! picks that option in an approval prompt. -//! 3. **Per-tool override** — `[permissions.tools]` in `settings.toml`, e.g. +//! picks that option in an approval prompt. For `run_command` the grant is +//! scoped to the command's first two whitespace-separated tokens (approving +//! `git push origin main` records `run_command(git push)`); other tools +//! record the bare tool name. +//! 3. **Rule lists** — `[permissions.rules]` in `settings.toml`: ordered +//! `deny` and `allow` lists of rules shaped `tool_name` or +//! `tool_name(argument_pattern)`, e.g. `run_command(git status)`, +//! `run_command(cargo *)`, `edit_file(src/*)`. The pattern matches the +//! command string for `run_command` and the path argument for the +//! file-mutating tools; for tools with no obvious argument (MCP tools, +//! unknown tools) only a bare `tool_name` rule matches. `deny` is checked +//! before `allow`, so a deny always beats an allow matching the same call. +//! 4. **Per-tool override** — `[permissions.tools]` in `settings.toml`, e.g. //! `run_command = "ask"`, `edit_file = "allow"`, `delete_file = "deny"`. -//! 4. **Default mode** — `[permissions] default = "ask"|"allow"|"deny"` in +//! 5. **Default mode** — `[permissions] default = "ask"|"allow"|"deny"` in //! `settings.toml`; `ask` on a fresh install. //! +//! Pattern matching (rules and session grants share it): `*` is a glob-style +//! wildcard (the `glob` tool's translator). A pattern ending in `*` matches +//! everything from the wildcard on — `run_command(cargo *)` covers +//! `cargo build src/main.rs`. A pattern without a trailing `*` must be the +//! whole argument or end at a whitespace boundary: `run_command(git status)` +//! matches `git status` and `git status --short` but not `git status-x`. +//! //! The `SIGIT_PERMISSIONS` env var (`allow`/`ask`/`deny`) overrides the stored //! default without writing the file — the escape hatch for ACP clients that //! cannot answer `session/request_permission` and for CI/headless runs. @@ -31,7 +49,10 @@ use std::collections::{HashMap, HashSet}; use std::sync::{Mutex, OnceLock}; +use regex::Regex; + use crate::settings::{self, PermissionMode}; +use crate::tools::glob_to_regex; /// Session key used by the interactive TUI, which only ever has one session. /// The TUI (`chat.rs`) is `#[cfg(unix)]`, so this is its only consumer and is @@ -130,14 +151,89 @@ pub fn approval_preview(arguments: &str) -> String { format!("{shown}… [+{} more chars]", total - MAX_CHARS) } -/// Policy check for one tool call. See the module docs for the layering. -pub fn decision_for(session: &str, tool_name: &str) -> Decision { +// ── Rule patterns ──────────────────────────────────────────────────────────── +// A rule is `tool_name` or `tool_name(argument_pattern)`; rules from +// `[permissions.rules]` and session grants share this matcher. + +/// Split a rule into tool name and optional argument pattern: +/// `run_command(git *)` → `("run_command", Some("git *"))`; +/// `edit_file` → `("edit_file", None)`. +fn parse_rule(rule: &str) -> (&str, Option<&str>) { + let rule = rule.trim(); + if let Some(open) = rule.find('(') + && let Some(inner) = rule[open + 1..].strip_suffix(')') + { + return (rule[..open].trim(), Some(inner)); + } + (rule, None) +} + +/// Compile a rule's argument pattern. Reuses the `glob` tool's translator +/// (`*`, `**`, `?`, `{a,b}`), then swaps its end anchor for rule semantics: +/// a pattern ending in `*` matches everything from the wildcard on (prefix +/// semantics — `cargo *` covers `cargo build src/main.rs`), while any other +/// pattern must be the whole argument or end at a whitespace boundary +/// (`git status` matches `git status --short` but not `git status-x`). +fn pattern_regex(pattern: &str) -> Option<Regex> { + let anchored = glob_to_regex(pattern); + let body = anchored.strip_suffix('$').unwrap_or(&anchored); + let source = if pattern.ends_with('*') { + body.to_string() + } else { + format!("{body}(?:$|\\s)") + }; + Regex::new(&source).ok() +} + +/// Whether one rule covers one tool call. A bare `tool_name` rule matches any +/// call of that tool; a pattern rule additionally needs the call's matchable +/// argument to fit the pattern — so a pattern rule never matches a tool that +/// has no matchable argument (an unreadable rule must not widen access). +fn rule_matches(rule: &str, tool_name: &str, argument: Option<&str>) -> bool { + let (rule_tool, pattern) = parse_rule(rule); + if rule_tool != tool_name { + return false; + } + match (pattern, argument) { + (None, _) => true, + (Some(pattern), Some(argument)) => { + pattern_regex(pattern).is_some_and(|re| re.is_match(argument)) + } + (Some(_), None) => false, + } +} + +/// The argument a rule pattern is matched against, extracted from the tool +/// call's raw JSON arguments: the command string for `run_command`, the path +/// for the file-mutating tools. Read-only tools never reach the matcher, and +/// other tools (MCP, unknown) have no obvious single argument, so they return +/// `None` and are governed only by bare `tool_name` rules. +fn matchable_argument(tool_name: &str, arguments: &str) -> Option<String> { + let key = match tool_name { + "run_command" => "command", + "edit_file" | "create_file" | "multi_edit" | "delete_file" | "create_directory" => "path", + _ => return None, + }; + let value: serde_json::Value = serde_json::from_str(arguments).ok()?; + Some(value.get(key)?.as_str()?.to_string()) +} + +/// Policy check for one tool call. `arguments` is the call's raw JSON argument +/// string, consulted by rule patterns and granular session grants. See the +/// module docs for the layering. +pub fn decision_for(session: &str, tool_name: &str, arguments: &str) -> Decision { if classify(tool_name) == ToolRisk::ReadOnly { return Decision::Allow; } + let argument = matchable_argument(tool_name, arguments); let (plan_mode, granted) = with_session(session, |s| { - (s.plan_mode, s.always_allow.contains(tool_name)) + ( + s.plan_mode, + s.always_allow + .iter() + .any(|grant| rule_matches(grant, tool_name, argument.as_deref())), + ) }); if plan_mode { @@ -147,6 +243,26 @@ pub fn decision_for(session: &str, tool_name: &str) -> Decision { return Decision::Allow; } + let rules = settings::permission_rules(); + if let Some(rule) = rules + .deny + .iter() + .find(|rule| rule_matches(rule, tool_name, argument.as_deref())) + { + return Decision::Deny(format!( + "`{tool_name}` is denied by the permission rule `{rule}` in settings.toml. \ + Do not retry it; work without this tool or ask the user to change \ + the policy." + )); + } + if rules + .allow + .iter() + .any(|rule| rule_matches(rule, tool_name, argument.as_deref())) + { + return Decision::Allow; + } + match settings::permission_mode_for(tool_name) { PermissionMode::Allow => Decision::Allow, PermissionMode::Ask => Decision::Ask, @@ -158,13 +274,33 @@ pub fn decision_for(session: &str, tool_name: &str) -> Decision { } } -/// Record an "always allow this session" grant for a tool. -pub fn grant_for_session(session: &str, tool_name: &str) { +/// Record an "always allow this session" grant for a tool call. For +/// `run_command` the grant is scoped to the command's first two +/// whitespace-separated tokens (approving `git push origin main` records +/// `run_command(git push)`, covering the `git push …` family only); other +/// tools record the bare tool name, matching any call. +pub fn grant_for_session(session: &str, tool_name: &str, arguments: &str) { + let grant = session_grant_rule(tool_name, arguments); with_session(session, |s| { - s.always_allow.insert(tool_name.to_string()); + s.always_allow.insert(grant); }); } +/// The rule string recorded for one approved call (see [`grant_for_session`]). +/// Falls back to the bare tool name when the command is absent or empty, which +/// grants the whole tool — exactly what the pre-granular behavior was. +fn session_grant_rule(tool_name: &str, arguments: &str) -> String { + if tool_name == "run_command" + && let Some(command) = matchable_argument(tool_name, arguments) + { + let prefix: Vec<&str> = command.split_whitespace().take(2).collect(); + if !prefix.is_empty() { + return format!("{tool_name}({})", prefix.join(" ")); + } + } + tool_name.to_string() +} + /// Toggle plan mode for a session. Returns the new state. pub fn set_plan_mode(session: &str, enabled: bool) -> bool { with_session(session, |s| { @@ -199,7 +335,8 @@ pub fn reset_all() { map.clear(); } -/// One-line status summary for `/permissions` and `/status`. +/// Status summary for `/permissions` and `/status`: the default mode, plan +/// mode, the granular session grants, and the active rule lists. pub fn describe(session: &str) -> String { let plan = if plan_mode(session) { "on" } else { "off" }; let default = settings::permission_default(); @@ -213,9 +350,20 @@ pub fn describe(session: &str) -> String { } else { granted }; + let rules = settings::permission_rules(); + let render = |list: &[String]| { + if list.is_empty() { + "none".to_string() + } else { + list.join(", ") + } + }; format!( "permissions: default={default} | plan mode: {plan} | session grants: {granted}\n\ - read-only tools always run; configure [permissions] in settings.toml" + rules: deny: {} | allow: {}\n\ + read-only tools always run; configure [permissions] in settings.toml", + render(&rules.deny), + render(&rules.allow), ) } @@ -232,6 +380,10 @@ mod tests { .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()); let dir = std::env::temp_dir().join(format!("sigit_perm_tests_{}", std::process::id())); + // Start from an empty sandbox: a settings.toml written by an earlier + // test in this process (e.g. one storing rule lists) must not leak + // into the next. + let _ = std::fs::remove_dir_all(&dir); // SAFETY: process-global env mutation, serialized by ENV_TEST_LOCK; the // other env-touching tests re-set these before reading. unsafe { std::env::set_var("SIGIT_CONFIG_DIR", &dir) }; @@ -239,6 +391,20 @@ mod tests { guard } + /// Persist rule lists into the sandboxed settings.toml. + fn store_rules(allow: &[&str], deny: &[&str]) { + let mut settings = settings::load(); + settings.permissions.rules.allow = allow.iter().map(|s| s.to_string()).collect(); + settings.permissions.rules.deny = deny.iter().map(|s| s.to_string()).collect(); + settings::store(&settings).unwrap(); + } + + /// `decision_for` on a `run_command` call with the given command string. + fn run_command_decision(session: &str, command: &str) -> Decision { + let args = serde_json::json!({ "command": command }).to_string(); + decision_for(session, "run_command", &args) + } + #[test] fn read_only_tools_always_allowed() { let _guard = env_guard(); @@ -254,7 +420,7 @@ mod tests { "command_output", ] { assert_eq!(classify(tool), ToolRisk::ReadOnly, "{tool}"); - assert_eq!(decision_for("t-ro", tool), Decision::Allow, "{tool}"); + assert_eq!(decision_for("t-ro", tool, "{}"), Decision::Allow, "{tool}"); } } @@ -283,10 +449,10 @@ mod tests { reset_session(session); set_plan_mode(session, true); assert!(matches!( - decision_for(session, "run_command"), + run_command_decision(session, "ls"), Decision::Deny(_) )); - assert_eq!(decision_for(session, "read_file"), Decision::Allow); + assert_eq!(decision_for(session, "read_file", "{}"), Decision::Allow); set_plan_mode(session, false); reset_session(session); } @@ -296,12 +462,21 @@ mod tests { let _guard = env_guard(); let session = "t-grant"; reset_session(session); - grant_for_session(session, "edit_file"); - assert_eq!(decision_for(session, "edit_file"), Decision::Allow); + let args = r#"{"path":"src/a.rs","old_text":"a","new_text":"b"}"#; + grant_for_session(session, "edit_file", args); + assert_eq!(decision_for(session, "edit_file", args), Decision::Allow); + // Non-run_command grants record the bare tool name: any path is covered. + assert_eq!( + decision_for(session, "edit_file", r#"{"path":"docs/other.md"}"#), + Decision::Allow + ); // Other tools are unaffected by the grant. - assert_ne!(decision_for(session, "delete_file"), Decision::Allow); + assert_ne!( + decision_for(session, "delete_file", r#"{"path":"src/a.rs"}"#), + Decision::Allow + ); reset_session(session); - assert_ne!(decision_for(session, "edit_file"), Decision::Allow); + assert_ne!(decision_for(session, "edit_file", args), Decision::Allow); } #[test] @@ -326,12 +501,152 @@ mod tests { let _guard = env_guard(); let session = "t-rank"; reset_session(session); - grant_for_session(session, "edit_file"); + let args = r#"{"path":"src/a.rs"}"#; + grant_for_session(session, "edit_file", args); set_plan_mode(session, true); assert!(matches!( - decision_for(session, "edit_file"), + decision_for(session, "edit_file", args), Decision::Deny(_) )); reset_session(session); } + + #[test] + fn rules_gate_run_command_by_argument() { + let _guard = env_guard(); + let session = "t-rules"; + reset_session(session); + store_rules(&["run_command(git *)"], &["run_command(git push*)"]); + + assert_eq!(run_command_decision(session, "git status"), Decision::Allow); + assert!(matches!( + run_command_decision(session, "git push"), + Decision::Deny(_) + )); + assert!(matches!( + run_command_decision(session, "git push --force"), + Decision::Deny(_) + )); + assert_eq!( + run_command_decision(session, "cargo test"), + Decision::Ask, + "an unmatched command falls through to the default mode" + ); + reset_session(session); + } + + #[test] + fn deny_rule_beats_matching_allow_rule() { + let _guard = env_guard(); + let session = "t-deny-wins"; + reset_session(session); + store_rules(&["run_command(git *)"], &["run_command(git *)"]); + match run_command_decision(session, "git status") { + Decision::Deny(reason) => assert!( + reason.contains("run_command(git *)"), + "the denial must name the rule, got: {reason}" + ), + other => panic!("expected a deny, got {other:?}"), + } + reset_session(session); + } + + #[test] + fn rule_pattern_wildcard_and_prefix_edges() { + // Whole-token prefix: a pattern without a trailing `*` matches at a + // whitespace boundary or the end, never mid-token. + let rule = "run_command(git status)"; + assert!(rule_matches(rule, "run_command", Some("git status"))); + assert!(rule_matches( + rule, + "run_command", + Some("git status --short") + )); + assert!(!rule_matches(rule, "run_command", Some("git status-x"))); + assert!(!rule_matches(rule, "run_command", Some("git statu"))); + assert!(!rule_matches(rule, "run_command", Some("xgit status"))); + + // Trailing `*`: everything from the wildcard on matches. + let rule = "run_command(git push*)"; + assert!(rule_matches(rule, "run_command", Some("git push"))); + assert!(rule_matches(rule, "run_command", Some("git pushx"))); + assert!(rule_matches(rule, "run_command", Some("git push --force"))); + assert!(!rule_matches(rule, "run_command", Some("git pus"))); + let rule = "run_command(cargo *)"; + assert!(rule_matches( + rule, + "run_command", + Some("cargo test --locked") + )); + assert!( + rule_matches(rule, "run_command", Some("cargo build --bin src/x")), + "a trailing `*` also covers arguments containing `/`" + ); + assert!(!rule_matches(rule, "run_command", Some("cargo"))); + + // A rule only applies to its own tool. + assert!(!rule_matches(rule, "delete_file", Some("cargo test"))); + // Bare tool rules match any call, including argument-less tools. + assert!(rule_matches("run_command", "run_command", Some("anything"))); + assert!(rule_matches("mcp__srv__tool", "mcp__srv__tool", None)); + // A pattern rule never matches a tool without a matchable argument. + assert!(!rule_matches("mcp__srv__tool(x)", "mcp__srv__tool", None)); + } + + #[test] + fn run_command_session_grant_is_scoped_to_command_prefix() { + let _guard = env_guard(); + let session = "t-grant-scope"; + reset_session(session); + grant_for_session( + session, + "run_command", + r#"{"command":"git push origin main"}"#, + ); + // The grant is `run_command(git push)`: the `git push …` family only. + assert_eq!(run_command_decision(session, "git push"), Decision::Allow); + assert_eq!( + run_command_decision(session, "git push --force-with-lease"), + Decision::Allow + ); + assert_eq!(run_command_decision(session, "git pull"), Decision::Ask); + assert_eq!(run_command_decision(session, "git pushx"), Decision::Ask); + assert_eq!(run_command_decision(session, "rm -rf /"), Decision::Ask); + + // A single-token command grants that token's family. + grant_for_session(session, "run_command", r#"{"command":"ls"}"#); + assert_eq!(run_command_decision(session, "ls -la"), Decision::Allow); + assert_eq!(run_command_decision(session, "lsof"), Decision::Ask); + reset_session(session); + } + + #[test] + fn file_tool_rules_match_on_path() { + let _guard = env_guard(); + let session = "t-file-rules"; + reset_session(session); + store_rules(&["edit_file(src/*)"], &["delete_file(src/*)"]); + assert_eq!( + decision_for( + session, + "edit_file", + r#"{"path":"src/main.rs","old_text":"a","new_text":"b"}"# + ), + Decision::Allow + ); + assert_eq!( + decision_for(session, "edit_file", r#"{"path":"docs/readme.md"}"#), + Decision::Ask, + "a path outside the rule falls through to the default mode" + ); + assert!(matches!( + decision_for(session, "delete_file", r#"{"path":"src/main.rs"}"#), + Decision::Deny(_) + )); + assert_eq!( + decision_for(session, "delete_file", r#"{"path":"docs/readme.md"}"#), + Decision::Ask + ); + reset_session(session); + } }

index 716c4d8..3afd80a 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -66,8 +66,29 @@ impl std::fmt::Display for PermissionMode { } } +/// The `[permissions.rules]` table: ordered allow/deny lists of rule strings. +/// A rule is `tool_name` or `tool_name(argument_pattern)`; the pattern is +/// matched against the command string for `run_command` and the path for the +/// file-mutating tools (see `crate::permissions` for the matching semantics). +/// +/// ```toml +/// [permissions.rules] +/// allow = ["run_command(git *)", "edit_file(src/*)"] +/// deny = ["run_command(git push*)"] +/// ``` +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Default)] +pub struct PermissionRules { + /// Rules that let a matching call run without asking. + #[serde(default)] + pub allow: Vec<String>, + /// Rules that block a matching call. Checked before `allow`, so a deny + /// always beats an allow that matches the same call. + #[serde(default)] + pub deny: Vec<String>, +} + /// The `[permissions]` table: a default mode for mutating tools plus per-tool -/// overrides, e.g. +/// overrides and argument-level rule lists, e.g. /// /// ```toml /// [permissions] @@ -76,6 +97,10 @@ impl std::fmt::Display for PermissionMode { /// [permissions.tools] /// edit_file = "allow" /// delete_file = "deny" +/// +/// [permissions.rules] +/// allow = ["run_command(cargo *)"] +/// deny = ["run_command(git push*)"] /// ``` #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Default)] pub struct PermissionSettings { @@ -87,6 +112,10 @@ pub struct PermissionSettings { /// `mcp__<server>__<tool>` name). #[serde(default)] pub tools: BTreeMap<String, PermissionMode>, + /// Argument-level allow/deny rules, consulted after session grants and + /// before the per-tool overrides. + #[serde(default)] + pub rules: PermissionRules, } /// Persisted preferences. New fields must carry `#[serde(default)]` so older @@ -188,6 +217,11 @@ pub fn permission_default() -> PermissionMode { load().permissions.default } +/// The stored `[permissions.rules]` allow/deny lists. +pub fn permission_rules() -> PermissionRules { + load().permissions.rules +} + /// The effective permission mode for one tool: its `[permissions.tools]` /// override when present, else the default (see [`permission_default`]). pub fn permission_mode_for(tool_name: &str) -> PermissionMode { @@ -274,6 +308,25 @@ mod tests { "unrecognized env value falls back to stored setting" ); + // Permission rules: absent on a fresh file, and they survive a + // store/load round trip without disturbing the other settings. + assert_eq!(permission_rules(), PermissionRules::default()); + let mut settings = load(); + settings.permissions.rules.allow = vec![ + "run_command(git *)".to_string(), + "edit_file(src/*)".to_string(), + ]; + settings.permissions.rules.deny = vec!["run_command(git push*)".to_string()]; + store(&settings).unwrap(); + let reloaded = load(); + assert_eq!(reloaded.permissions.rules, settings.permissions.rules); + assert_eq!(permission_rules(), settings.permissions.rules); + assert_eq!( + reloaded.permissions.tools.get("delete_file"), + Some(&PermissionMode::Deny), + "storing rules preserves the per-tool overrides" + ); + unsafe { std::env::remove_var(PERMISSIONS_ENV) }; unsafe { std::env::remove_var(LOCAL_INFERENCE_ENV) }; unsafe { std::env::remove_var("SIGIT_CONFIG_DIR") };

index 09c7032..ff2bf45 100644 --- a/src/tools.rs +++ b/src/tools.rs @@ -1346,9 +1346,10 @@ fn exec_multi_edit(arguments: &str) -> String { /// Translate a shell-style glob into an anchored regex. Supports `*` /// (non-separator run), `**` (any number of directories), `?` (one /// non-separator), and `{a,b}` alternation; everything else is matched -/// literally. Used both by the `glob` tool (against relative paths) and by -/// `search_files`' `file_glob` filter (against bare file names). -fn glob_to_regex(glob: &str) -> String { +/// literally. Used by the `glob` tool (against relative paths), by +/// `search_files`' `file_glob` filter (against bare file names), and by +/// `crate::permissions` rule patterns (which re-anchor the result). +pub(crate) fn glob_to_regex(glob: &str) -> String { let chars: Vec<char> = glob.chars().collect(); let mut re = String::from("^"); let mut brace_depth = 0usize;