From fc1de3a3d0a9eb11ba3b4a58cc9bd33ebcdf8424 Mon Sep 17 00:00:00 2001 From: hartsock Date: Tue, 2 Jun 2026 21:29:50 -0400 Subject: [PATCH] feat(core): add CommandInterceptor exec/open hook to ShellExtensions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Embedders of brush-core could not confine command execution in-process. The path-separator dispatch branch in commands.rs (e.g. `/bin/rm`, `./x`) bypasses both the PATH search and the builtin table, so any name- or PATH-based gate an embedder might apply is trivially defeated. ShellExtensions carried only ErrorFormatter (presentation), with no authority seam. Add an optional CommandInterceptor component to ShellExtensions, mirroring the existing ErrorFormatter pattern, with two default-allow hooks: - before_exec(program, args) -> ExecDecision, called at the single external- spawn funnel (execute_external_command), so BOTH dispatch branches — including the path-separator branch — are covered. - before_open(path, write) -> OpenDecision, called in Shell::open_file, the single choke point for all filesystem-path opens (redirections + source/.). On Deny, execs fail with a new ErrorKind::ExecDenied (exit code 126) and opens fail with a PermissionDenied io::Error surfaced by the existing redirection / source error wrapping. Neither panics nor silently skips. The change is purely additive: ShellExtensionsImpl gains a second type param that defaults to DefaultCommandInterceptor (allow-all), so DefaultShellExtensions and all existing call sites are source-compatible and byte-for-byte identical in behavior. The full existing suite (including the YAML bash-compat and redirection cases) passes unchanged. Motivation: agent-bridle, an object-capability confinement layer for LLM coding agents. The agent harness is a confused deputy (full ambient authority + untrusted instructions); the ocap remedy enforces attenuated capabilities at the point of use. An embedded brush is the script-execution surface, so enforcement must live inside the shell before the spawn/open — and must cover the path-separator bypass. Includes brush-core/tests/command_interceptor_tests.rs, which proves a custom interceptor denies `rm`, denies the absolute-path `/bin/rm` (the load-bearing path-separator case), allows `/bin/true`, denies writes outside an allowed dir, and allows read-only source opens. Upstream contribution drafted at docs/upstream/reubeno-brush-exec-open-hook.md (not yet filed). Assisted-by: Claude Opus 4.8 (1M context) Co-Authored-By: Claude Opus 4.8 (1M context) --- Cargo.lock | 1 + brush-core/Cargo.toml | 5 + brush-core/src/commands.rs | 17 ++ brush-core/src/error.rs | 7 + brush-core/src/extensions.rs | 105 ++++++- brush-core/src/shell.rs | 22 ++ brush-core/src/shell/builder.rs | 4 + brush-core/src/shell/fs.rs | 52 ++++ brush-core/tests/command_interceptor_tests.rs | 261 ++++++++++++++++++ 9 files changed, 470 insertions(+), 4 deletions(-) create mode 100644 brush-core/tests/command_interceptor_tests.rs diff --git a/Cargo.lock b/Cargo.lock index ba59fe3e5..7cb613ef1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -433,6 +433,7 @@ dependencies = [ "async-recursion", "async-trait", "bon", + "brush-builtins", "brush-parser", "cached", "cfg-if", diff --git a/brush-core/Cargo.toml b/brush-core/Cargo.toml index d3f59d289..198b4c064 100644 --- a/brush-core/Cargo.toml +++ b/brush-core/Cargo.toml @@ -85,5 +85,10 @@ uuid = { version = "1.23.1", features = ["js"] } [dev-dependencies] anyhow = "1.0.102" +# Used only by integration tests that need a realistically-populated builtin +# table (e.g. `echo`, `.`/`source`) to exercise the capability-interceptor +# hooks. This is a dev-dependency cycle (brush-builtins depends on brush-core), +# which Cargo permits for dev-dependencies. +brush-builtins = { version = "^0.2.0", path = "../brush-builtins" } pretty_assertions = { version = "1.4.1", features = ["unstable"] } tempfile = "3.27.0" diff --git a/brush-core/src/commands.rs b/brush-core/src/commands.rs index 0bd9c3b30..2f3184158 100644 --- a/brush-core/src/commands.rs +++ b/brush-core/src/commands.rs @@ -579,6 +579,23 @@ pub(crate) fn execute_external_command( }) .collect::>(); + // Give the configured command interceptor (capability confinement) a chance + // to deny this external spawn. This site is the single funnel for *all* + // external commands, including the path-separator branch (e.g. `/bin/rm`, + // `./x`) that bypasses PATH and the builtin table, so a policy here cannot + // be circumvented by spelling the command differently. + { + use crate::extensions::CommandInterceptor as _; + let hook_args: Vec = cmd_args.iter().map(|s| (*s).clone()).collect(); + if let extensions::ExecDecision::Deny(reason) = context + .shell + .command_interceptor() + .before_exec(executable_path, hook_args.as_slice()) + { + return Err(error::ErrorKind::ExecDenied(context.command_name.clone(), reason).into()); + } + } + // Before we lose ownership of the open files, figure out if stdin will be a terminal. let child_stdin_is_terminal = context .try_fd(openfiles::OpenFiles::STDIN_FD) diff --git a/brush-core/src/error.rs b/brush-core/src/error.rs index 4c0088a3c..bc715e332 100644 --- a/brush-core/src/error.rs +++ b/brush-core/src/error.rs @@ -318,6 +318,12 @@ pub enum ErrorKind { /// A glob pattern failed to match any files (failglob). #[error("no match: {0}")] NoMatch(String), + + /// Execution of an external command was denied by the configured + /// command interceptor (capability confinement). The first field is the + /// program; the second is the reason supplied by the interceptor. + #[error("{0}: execution denied: {1}")] + ExecDenied(String, String), } /// Trait implementable by built-in commands to represent errors. @@ -363,6 +369,7 @@ impl From<&ErrorKind> for results::ExecutionExitCode { ErrorKind::FunctionParseError(..) => Self::InvalidUsage, ErrorKind::TestCommandParseError(..) => Self::InvalidUsage, ErrorKind::FailedToExecuteCommand(..) => Self::CannotExecute, + ErrorKind::ExecDenied(..) => Self::CannotExecute, ErrorKind::FunctionNameShadowsSpecialBuiltin { .. } => Self::InvalidUsage, ErrorKind::IoError(io_err) => io_err.into(), ErrorKind::BuiltinError(inner, ..) => inner.as_exit_code(), diff --git a/brush-core/src/extensions.rs b/brush-core/src/extensions.rs index 487f89fc0..592e4803d 100644 --- a/brush-core/src/extensions.rs +++ b/brush-core/src/extensions.rs @@ -1,5 +1,7 @@ //! Definition of shell behavior traits and defaults. +use std::path::Path; + use crate::{Shell, error, extensions}; /// Trait for static shell extensions. Collects all associated types needed to @@ -7,21 +9,33 @@ use crate::{Shell, error, extensions}; pub trait ShellExtensions: Clone + Default + Send + Sync + 'static { /// Type of the error behavior implementation. type ErrorFormatter: ErrorFormatter; + + /// Type of the command-interceptor (capability-confinement) implementation. + /// + /// This component allows an embedding host to observe — and optionally + /// *deny* — external command execution and file opens as they happen, + /// in-process. See [`CommandInterceptor`] for the available hooks. + type CommandInterceptor: CommandInterceptor; } /// Shell extensions implementation constructed from component types. #[derive(Clone, Default)] -pub struct ShellExtensionsImpl { - _marker: std::marker::PhantomData, +pub struct ShellExtensionsImpl< + EF: ErrorFormatter = DefaultErrorFormatter, + CI: CommandInterceptor = DefaultCommandInterceptor, +> { + _marker: std::marker::PhantomData<(EF, CI)>, } -impl ShellExtensions for ShellExtensionsImpl { +impl ShellExtensions for ShellExtensionsImpl { type ErrorFormatter = EF; + type CommandInterceptor = CI; } /// Default shell extensions implementation. /// This is a type alias for the most common shell configuration. -pub type DefaultShellExtensions = ShellExtensionsImpl; +pub type DefaultShellExtensions = + ShellExtensionsImpl; /// Trait for defining shell error behaviors. pub trait ErrorFormatter: Clone + Default + Send + Sync + 'static { @@ -47,6 +61,89 @@ pub struct DefaultErrorFormatter; impl ErrorFormatter for DefaultErrorFormatter {} +/// Decision returned by [`CommandInterceptor::before_exec`] to control whether +/// an external command is allowed to spawn. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum ExecDecision { + /// Allow the external command to be spawned (the default). + Allow, + /// Deny the external command. The contained string explains why; it is + /// surfaced to the shell as an [`error::Error`] and the command does not + /// run. + Deny(String), +} + +/// Decision returned by [`CommandInterceptor::before_open`] to control whether +/// a file may be opened. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum OpenDecision { + /// Allow the file to be opened (the default). + Allow, + /// Deny opening the file. The contained string explains why; it is surfaced + /// to the shell as an [`error::Error`] and the file is not opened. + Deny(String), +} + +/// Trait for intercepting potentially-sensitive shell operations so an +/// embedding host can apply capability confinement (object-capability style +/// authority attenuation) *in-process*. +/// +/// The default implementation ([`DefaultCommandInterceptor`]) allows +/// everything, making it byte-for-byte equivalent to a shell with no +/// interceptor at all. Embedders supply their own implementation via the +/// [`ShellExtensions::CommandInterceptor`] associated type to enforce a policy. +/// +/// # Why this exists +/// +/// Without these hooks, a hosting process cannot reliably confine command +/// execution in-process: a command whose name contains a path separator (e.g. +/// `/bin/rm` or `./script`) bypasses both the `PATH` search and the builtin +/// table and is executed directly. [`before_exec`](Self::before_exec) is called +/// at *every* external-spawn site — including that path-separator branch — so a +/// policy here cannot be circumvented by spelling the command differently. +pub trait CommandInterceptor: Clone + Default + Send + Sync + 'static { + /// Called immediately before an external command is spawned, at every spawn + /// site (including the path-separator branch that bypasses `PATH` and the + /// builtin table). Returning [`ExecDecision::Deny`] prevents the command + /// from running and fails it with an error. + /// + /// # Arguments + /// + /// * `program` - The program that is about to be executed. For commands + /// resolved via `PATH` this is the resolved absolute path; for + /// path-separator commands it is the path as written by the user. + /// * `args` - The argument strings that would be passed to the program + /// (not including `argv[0]`). + fn before_exec(&self, program: &str, args: &[String]) -> ExecDecision { + let _ = (program, args); + ExecDecision::Allow + } + + /// Called immediately before a file is opened via a filesystem path + /// (redirections and `source`/`.`). Returning [`OpenDecision::Deny`] + /// prevents the file from being opened and fails the operation with an + /// error. + /// + /// # Arguments + /// + /// * `path` - The absolute path that is about to be opened. + /// * `write` - Whether the open requests write access (`true`) or is + /// read-only (`false`). + fn before_open(&self, path: &Path, write: bool) -> OpenDecision { + let _ = (path, write); + OpenDecision::Allow + } +} + +/// Default command-interceptor implementation: allows all execs and opens. +/// +/// A shell configured with this interceptor behaves identically to a shell with +/// no interception at all. +#[derive(Clone, Default)] +pub struct DefaultCommandInterceptor; + +impl CommandInterceptor for DefaultCommandInterceptor {} + /// Trait for placeholder behavior (stub for future extension). pub trait PlaceholderBehavior: Clone + Default + Send + Sync + 'static {} diff --git a/brush-core/src/shell.rs b/brush-core/src/shell.rs index 11d02bfaa..758deac46 100644 --- a/brush-core/src/shell.rs +++ b/brush-core/src/shell.rs @@ -61,6 +61,13 @@ pub struct Shell Clone for Shell { fn clone(&self) -> Self { Self { error_formatter: self.error_formatter.clone(), + command_interceptor: self.command_interceptor.clone(), traps: self.traps.clone(), open_files: self.open_files.clone(), working_dir: self.working_dir.clone(), @@ -212,6 +220,7 @@ impl Shell { // Instantiate the shell with some defaults. let mut shell = Self { error_formatter: options.error_formatter, + command_interceptor: options.command_interceptor, open_files: openfiles::OpenFiles::new(), options: runtime_options, name: options.shell_name, @@ -349,6 +358,14 @@ impl Shell { pub(crate) const fn last_exit_status_change_count(&self) -> usize { self.last_exit_status_change_count } + + /// Returns a reference to the shell's configured command interceptor + /// (capability-confinement) behavior. Hosts rarely need to call this + /// directly; the shell consults it automatically before spawning external + /// commands and opening files. + pub const fn command_interceptor(&self) -> &SE::CommandInterceptor { + &self.command_interceptor + } } #[inherent::inherent] @@ -548,3 +565,8 @@ impl ShellState for Shell { fn default_error_formatter() -> EF { EF::default() } + +#[cfg(feature = "serde")] +fn default_command_interceptor() -> CI { + CI::default() +} diff --git a/brush-core/src/shell/builder.rs b/brush-core/src/shell/builder.rs index 3f99a9249..600d8bf71 100644 --- a/brush-core/src/shell/builder.rs +++ b/brush-core/src/shell/builder.rs @@ -146,6 +146,9 @@ pub struct CreateOptions Default for Shell { fn default() -> Self { Self { error_formatter: SE::ErrorFormatter::default(), + command_interceptor: SE::CommandInterceptor::default(), traps: traps::TrapHandlerConfig::default(), open_files: openfiles::OpenFiles::default(), working_dir: PathBuf::default(), diff --git a/brush-core/src/shell/fs.rs b/brush-core/src/shell/fs.rs index de4b75cff..077418538 100644 --- a/brush-core/src/shell/fs.rs +++ b/brush-core/src/shell/fs.rs @@ -195,6 +195,23 @@ impl crate::Shell { let path_to_open = self.absolute_path(path.as_ref()); + // Consult the configured command interceptor (capability confinement) + // before opening. This is the single choke point through which all + // filesystem-path opens flow (redirections and `source`/`.`), so a + // policy applied here covers every path-based open in the shell. + { + use crate::extensions::CommandInterceptor as _; + let write = open_options_request_write(options); + if let crate::extensions::OpenDecision::Deny(reason) = + self.command_interceptor().before_open(&path_to_open, write) + { + return Err(std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + format!("open denied: {reason}"), + )); + } + } + // See if this is a reference to a file descriptor. These paths should // reflect the shell's current execution fds, which can differ from the // host process fds after redirections like here-docs. @@ -242,3 +259,38 @@ fn shell_fd_path_to_fd(path: &Path) -> Option { None } } + +/// Best-effort determination of whether an [`std::fs::OpenOptions`] requests +/// write access (including append). Used solely to inform the capability +/// interceptor's `before_open` hook of write-intent. +/// +/// `std::fs::OpenOptions` exposes no getters for its configured flags, so we +/// inspect its `Debug` representation, which is documented to render the +/// `write` and `append` booleans. If the format ever changes such that we +/// cannot determine intent, we conservatively report `true` (write) so that a +/// confinement policy never under-reports access. +fn open_options_request_write(options: &std::fs::OpenOptions) -> bool { + let rendered = format!("{options:?}"); + + // Reads the boolean value of a `name: ` field from the rendered + // debug string without slicing on byte offsets (which could panic on + // multi-byte boundaries). + let flag = |name: &str| -> Option { + let needle = format!("{name}: "); + let tail = rendered.split_once(&needle)?.1; + if tail.starts_with("true") { + Some(true) + } else if tail.starts_with("false") { + Some(false) + } else { + None + } + }; + + match (flag("write"), flag("append")) { + // We could read both flags: write access iff either is set. + (Some(write), Some(append)) => write || append, + // Could not parse the expected fields; fail safe toward "write". + _ => true, + } +} diff --git a/brush-core/tests/command_interceptor_tests.rs b/brush-core/tests/command_interceptor_tests.rs new file mode 100644 index 000000000..9cfac2fe6 --- /dev/null +++ b/brush-core/tests/command_interceptor_tests.rs @@ -0,0 +1,261 @@ +//! Integration tests for the `CommandInterceptor` capability-confinement hooks +//! on `ShellExtensions` (`before_exec` / `before_open`). +//! +//! These tests prove that an embedding host can deny external command execution +//! and file opens *in-process*, including for commands whose name contains a +//! path separator (e.g. `/bin/rm`). That path-separator branch historically +//! bypassed both the PATH search and the builtin table, so confining it is the +//! whole point of `before_exec`. +//! +//! These tests target unix. Denied commands use `/bin/rm` — its existence is +//! irrelevant because `before_exec` denies it *before* any spawn. Commands that +//! are actually executed use `/usr/bin/true`, which exists on both Linux and +//! macOS (note: `/bin/true` is absent on macOS). +#![cfg(unix)] +#![cfg(test)] +#![allow(clippy::panic_in_result_fn)] + +use std::path::{Path, PathBuf}; +use std::sync::{Arc, Mutex}; + +use anyhow::Result; +use brush_core::extensions::{ + CommandInterceptor, ErrorFormatter, ExecDecision, OpenDecision, ShellExtensions, +}; + +/// An interceptor that denies any external program whose basename is in a deny +/// list, and denies write-opens of any path that is not under `allowed_write_dir`. +/// All decisions are recorded so tests can assert the hook actually fired. +#[derive(Clone, Default)] +struct PolicyInterceptor { + denied_basenames: Arc>, + allowed_write_dir: Arc>>, + exec_calls: Arc>>, + open_calls: Arc>>, +} + +impl PolicyInterceptor { + fn new(denied_basenames: &[&str], allowed_write_dir: Option) -> Self { + Self { + denied_basenames: Arc::new(denied_basenames.iter().map(|s| (*s).to_string()).collect()), + allowed_write_dir: Arc::new(Mutex::new(allowed_write_dir)), + exec_calls: Arc::new(Mutex::new(Vec::new())), + open_calls: Arc::new(Mutex::new(Vec::new())), + } + } + + fn exec_calls(&self) -> Vec { + self.exec_calls.lock().unwrap().clone() + } + + fn open_calls(&self) -> Vec<(PathBuf, bool)> { + self.open_calls.lock().unwrap().clone() + } +} + +impl CommandInterceptor for PolicyInterceptor { + fn before_exec(&self, program: &str, _args: &[String]) -> ExecDecision { + self.exec_calls.lock().unwrap().push(program.to_string()); + + // Match on the basename so that `rm` and `/bin/rm` are both caught. + let basename = Path::new(program) + .file_name() + .map_or_else(|| program.to_string(), |s| s.to_string_lossy().into_owned()); + + if self.denied_basenames.iter().any(|d| d == &basename) { + ExecDecision::Deny(format!("'{basename}' is not permitted by policy")) + } else { + ExecDecision::Allow + } + } + + fn before_open(&self, path: &Path, write: bool) -> OpenDecision { + self.open_calls + .lock() + .unwrap() + .push((path.to_path_buf(), write)); + + if !write { + return OpenDecision::Allow; + } + + match self.allowed_write_dir.lock().unwrap().as_ref() { + Some(dir) if path.starts_with(dir) => OpenDecision::Allow, + Some(_) => OpenDecision::Deny(format!( + "writes to {} are outside the permitted directory", + path.display() + )), + None => OpenDecision::Allow, + } + } +} + +/// Wire `PolicyInterceptor` into a `ShellExtensions` bundle that otherwise uses +/// brush's default behaviors. +#[derive(Clone, Default)] +struct PolicyExtensions; + +impl ShellExtensions for PolicyExtensions { + type ErrorFormatter = DefaultFormatter; + type CommandInterceptor = PolicyInterceptor; +} + +#[derive(Clone, Default)] +struct DefaultFormatter; +impl ErrorFormatter for DefaultFormatter {} + +/// Builds a shell that uses the provided interceptor, skipping profile/rc and +/// environment inheritance so the test is hermetic, but with the default +/// builtin table registered (so `echo`, `.`/`source`, etc. behave normally). +async fn shell_with_interceptor( + interceptor: PolicyInterceptor, +) -> Result> { + let builtins = + brush_builtins::default_builtins::(brush_builtins::BuiltinSet::BashMode); + + let mut shell = brush_core::Shell::builder_with_extensions::() + .command_interceptor(interceptor) + .builtins(builtins) + .do_not_inherit_env(true) + .skip_well_known_vars(true) + .build() + .await?; + + // Provide a deterministic PATH so bare-name external commands can resolve. + run(&mut shell, "export PATH=/bin:/usr/bin").await?; + + Ok(shell) +} + +async fn run(shell: &mut brush_core::Shell, cmd: &str) -> Result { + let params = shell.default_exec_params(); + let result = shell + .run_string(cmd, &brush_core::SourceInfo::default(), ¶ms) + .await?; + Ok(u8::from(result.exit_code)) +} + +/// `before_exec` must deny a command referenced by bare name (resolved via PATH). +#[tokio::test] +async fn denies_bare_name_command() -> Result<()> { + let interceptor = PolicyInterceptor::new(&["rm"], None); + let mut shell = shell_with_interceptor(interceptor.clone()).await?; + + let code = run(&mut shell, "rm /tmp/does-not-matter").await?; + assert_ne!(code, 0, "denied `rm` must report a non-zero exit code"); + + let exec_calls = interceptor.exec_calls(); + assert!( + exec_calls.iter().any(|p| p.ends_with("rm")), + "before_exec should have been consulted for `rm`; saw: {exec_calls:?}" + ); + Ok(()) +} + +/// The load-bearing test: a command containing a path separator (`/bin/rm`) +/// historically bypassed PATH and the builtin table. `before_exec` must still +/// fire for it, proving the bypass is closed. +#[tokio::test] +async fn denies_absolute_path_command_closing_path_separator_bypass() -> Result<()> { + let interceptor = PolicyInterceptor::new(&["rm"], None); + let mut shell = shell_with_interceptor(interceptor.clone()).await?; + + let code = run(&mut shell, "/bin/rm /tmp/does-not-matter").await?; + assert_ne!( + code, 0, + "denied `/bin/rm` (path-separator branch) must report a non-zero exit code" + ); + + let exec_calls = interceptor.exec_calls(); + assert!( + exec_calls.iter().any(|p| p == "/bin/rm"), + "before_exec must be consulted for the path-separator command `/bin/rm`; saw: {exec_calls:?}" + ); + Ok(()) +} + +/// A permitted command must still run normally — the default decision is Allow. +#[tokio::test] +async fn allows_permitted_command() -> Result<()> { + let interceptor = PolicyInterceptor::new(&["rm"], None); + let mut shell = shell_with_interceptor(interceptor.clone()).await?; + + // `/usr/bin/true` is a path-separator command too, so this also proves the + // Allow path of the path-separator branch. We use `/usr/bin/true` rather than + // `/bin/true` because the latter does not exist on macOS (where `true` lives + // only at `/usr/bin/true`); `/usr/bin/true` is present on both Linux and macOS. + let code = run(&mut shell, "/usr/bin/true").await?; + assert_eq!(code, 0, "permitted `/usr/bin/true` should succeed"); + + let exec_calls = interceptor.exec_calls(); + assert!( + exec_calls.iter().any(|p| p == "/usr/bin/true"), + "before_exec should have observed `/usr/bin/true`; saw: {exec_calls:?}" + ); + Ok(()) +} + +/// `before_open` must deny an output redirection that writes outside the +/// permitted directory, while allowing one inside it. +#[tokio::test] +async fn denies_write_outside_allowed_dir() -> Result<()> { + let allowed = tempfile::tempdir()?; + let forbidden = tempfile::tempdir()?; + + let interceptor = PolicyInterceptor::new(&[], Some(allowed.path().to_path_buf())); + let mut shell = shell_with_interceptor(interceptor.clone()).await?; + + // Writing inside the allowed dir is permitted. + let allowed_file = allowed.path().join("ok.txt"); + let code = run(&mut shell, &format!("echo hi > {}", allowed_file.display())).await?; + assert_eq!(code, 0, "write inside the allowed dir should succeed"); + assert!( + allowed_file.exists(), + "the permitted file should have been created" + ); + + // Writing outside the allowed dir is denied. + let forbidden_file = forbidden.path().join("nope.txt"); + let code = run( + &mut shell, + &format!("echo hi > {}", forbidden_file.display()), + ) + .await?; + assert_ne!(code, 0, "write outside the allowed dir must fail"); + assert!( + !forbidden_file.exists(), + "the forbidden file must NOT have been created" + ); + + let open_calls = interceptor.open_calls(); + assert!( + open_calls.iter().any(|(p, w)| *w && p == &forbidden_file), + "before_open should have been consulted (with write=true) for the forbidden path; saw: {open_calls:?}" + ); + Ok(()) +} + +/// Reads must be allowed by this policy (write=false), proving the `write` flag +/// is threaded correctly and read-only opens aren't accidentally denied. +#[tokio::test] +async fn allows_read_open() -> Result<()> { + let dir = tempfile::tempdir()?; + let script = dir.path().join("snippet.sh"); + std::fs::write(&script, "X=hello\n")?; + + // allowed_write_dir is set to a *different* dir, so if reads were + // misclassified as writes they would be denied. + let other = tempfile::tempdir()?; + let interceptor = PolicyInterceptor::new(&[], Some(other.path().to_path_buf())); + let mut shell = shell_with_interceptor(interceptor.clone()).await?; + + let code = run(&mut shell, &format!(". {}", script.display())).await?; + assert_eq!(code, 0, "sourcing (read-only open) should be permitted"); + + let open_calls = interceptor.open_calls(); + assert!( + open_calls.iter().any(|(p, w)| !*w && p == &script), + "before_open should have observed a read-only open of the sourced file; saw: {open_calls:?}" + ); + Ok(()) +}