From 4dc216249c8eb3b401396d9a3867506396abf337 Mon Sep 17 00:00:00 2001 From: kewton Date: Tue, 13 Oct 2026 01:00:57 +0900 Subject: [PATCH 1/3] Record the write sandbox as not applied for workflow nodes in another workspace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a workflow child node runs in a workspace other than the run's, the installed OS write sandbox is fixed to the run's workspace. Instead of applying it to the wrong workspace, record it as not applied for that node: `auto` continues with lexical checks only and `required` fails the node's Bash and verifier commands, so a node is never recorded as isolated while Bash runs in a different workspace. When the workspace matches, nothing changes. Once a mismatch is recorded, it does not revert when a later node runs in the parent's workspace; the stricter record is kept. Skip installing the sandbox (and its per-run temp directory) for read-only actions, and remove the run temp directory on every exit path. The direct-CLI SIGINT finalizer exits with `std::process::exit`, which skips `Drop`, so it releases the run temp directory itself before both of its exits (the summary path and the already-finalized path). Append a `mktemp -p "$TMPDIR"` hint to a Bash failure summary when the sandbox is applied and the failure is a temp-directory permission error. Document in SECURITY.md that runtime dependency setup runs outside the sandbox and that a workspace hardlink that pre-exists the run can be written through. 判断: Issue #659 leaves the child-node fix open (re-point the sandbox to the child workspace, or record applied=false and fail under `required`). Chose the second, smaller option: it needs no new temp directory and no profile rebuild, and keeps the call site a single reconcile step. `auto` continues with lexical checks only and `required` fails, so the record never claims isolation that was not in force. Co-authored-by: CommandCodeBot --- SECURITY.md | 10 ++ src/lib.rs | 138 ++++++++++++++++--- src/tools/bash.rs | 47 ++++++- src/tools/os_sandbox.rs | 98 ++++++++++++++ tests/issue502_os_sandbox_write.rs | 205 ++++++++++++++++++++++++++++- 5 files changed, 472 insertions(+), 26 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index 443300a0..b113ede1 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -68,6 +68,16 @@ before the shell starts. policies, so a command they admit can still start a script; only the sandbox (when applied) constrains that script's writes. Reads and network are not confined in this stage. + - Runtime dependency setup runs outside the sandbox: the setup steps that + execute `setup.py`, npm `postinstall`/`prepare` hooks, and similar + install-time scripts run before or outside the confined `sh -c` launch + points, so the sandbox does not constrain their writes. Confining dependency + setup is a later stage (#658). + - A hardlink that already exists in the workspace before the run and points at + an inode outside the workspace can still be written through. Seatbelt decides + by path, so it allows writing the workspace path even though the inode lives + outside; the sandbox only denies *creating* a new hardlink to an outside + inode. - Backend availability is probed once at run start. On macOS the backend is Seatbelt through `sandbox-exec`; under a nested sandbox it reports `rc=71` and is unavailable. Other platforms have no backend in this stage. diff --git a/src/lib.rs b/src/lib.rs index e2dde40e..ca1eb3fb 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -133,10 +133,16 @@ pub fn run_with_provider_options( let sandbox_mode = cli.sandbox; let config = Config::from_cli_with_provider_options(cli, provider_options) .map_err(cli_argument_error)?; - tools::os_sandbox::install(sandbox_mode, &config.workspace_root, &config.state_dir); - if let Some(warning) = tools::os_sandbox::startup_warning() { - eprintln!("{warning}"); - } + // Only actions that can run model/verifier Bash need the OS write sandbox and + // its per-run temp directory; read-only actions must not create one. The + // guard removes the run temp directory on every exit path. + let _sandbox_cleanup = action_uses_write_sandbox(&config.action).then(|| { + tools::os_sandbox::install(sandbox_mode, &config.workspace_root, &config.state_dir); + if let Some(warning) = tools::os_sandbox::startup_warning() { + eprintln!("{warning}"); + } + tools::os_sandbox::cleanup_guard() + }); for profile in planner::extension_profiles::registered() { for warning in &profile.warnings { eprintln!("warning: {warning}"); @@ -213,6 +219,13 @@ pub(crate) fn run_resolved_config_for_workflow(config: Config) -> anyhow::Result // the whole run. Re-entering catch_cli_run here makes the same thread re-lock that // mutex and self-deadlock. Run the resolved config directly; the outer boundary // still catches any panic raised by this child node run. + // + // The sandbox was installed once, in the parent's workspace. A child node runs + // in its own origin workspace, so reconcile the installed sandbox with that + // workspace first: when they differ the sandbox is recorded as not applied + // (and `required` fails the node) instead of confining it to the wrong place. + // When they match, this is a no-op. + tools::os_sandbox::reconcile_workspace(&config.workspace_root); run_config(config, None) } @@ -454,6 +467,16 @@ fn action_uses_workspace_lock(action: &Action) -> bool { ) } +/// Whether the action can run model Bash or verifier commands, so it needs the OS +/// write sandbox and its per-run temp directory. Read-only actions do not, and +/// must not create a temp directory. +fn action_uses_write_sandbox(action: &Action) -> bool { + !matches!( + action, + Action::Runs(_) | Action::UxDemo | Action::ModelProbe | Action::Doctor + ) +} + /// Whether the action starts and stops a run recorded in the event stream. /// /// `--ux-demo` only renders a scripted demo, so it must not record run @@ -568,24 +591,11 @@ impl DirectCommandCompletionGuard { } if signals.forever().next().is_some() { if !signal_finalized.swap(true, Ordering::AcqRel) { - let result: anyhow::Result<()> = - Err(anyhow::anyhow!("interrupted by user")); - bounded_process::reap_registered_server_children_for_workspace( - signal_config.eval_events_path.as_deref(), - "direct_cli_sigint", - &signal_config.workspace_root, - ); - emit_direct_command_stop_with_status( + let mut stdout = std::io::stdout().lock(); + if let Err(err) = finalize_interrupted_direct_command( &signal_config, &signal_command, - &result, - DirectCommandStatus::Interrupted, - ); - tui::terminal_notifications::finish_process(); - let mut stdout = std::io::stdout().lock(); - if let Err(err) = write_interrupted_headless_summary( summary_source.as_ref(), - signal_config.eval_events_path.as_deref(), &mut stdout, ) { eprintln!( @@ -595,6 +605,9 @@ impl DirectCommandCompletionGuard { // Keep stdout locked until exit so the JSON remains the final line. std::process::exit(130); } + // Another thread already finalized; still release the run temp + // directory, which `std::process::exit` would otherwise leak. + tools::os_sandbox::cleanup(); std::process::exit(130); } })); @@ -633,6 +646,39 @@ impl DirectCommandCompletionGuard { } } +/// The work the direct-CLI SIGINT finalizer runs before `std::process::exit`, +/// which skips `Drop`: reap server children, emit the interrupted stop event, +/// write the interrupted headless summary, then release the run temp directory. +/// The cleanup always runs, even when the summary write fails, so the exit leaks +/// no temp directory. +fn finalize_interrupted_direct_command( + config: &Config, + command: &str, + summary_source: Option<&headless_summary::Source>, + output: &mut impl Write, +) -> std::io::Result<()> { + let result: anyhow::Result<()> = Err(anyhow::anyhow!("interrupted by user")); + bounded_process::reap_registered_server_children_for_workspace( + config.eval_events_path.as_deref(), + "direct_cli_sigint", + &config.workspace_root, + ); + emit_direct_command_stop_with_status( + config, + command, + &result, + DirectCommandStatus::Interrupted, + ); + tui::terminal_notifications::finish_process(); + let write_result = write_interrupted_headless_summary( + summary_source, + config.eval_events_path.as_deref(), + output, + ); + tools::os_sandbox::cleanup(); + write_result.map(|_| ()) +} + fn write_interrupted_headless_summary( source: Option<&headless_summary::Source>, events_path: Option<&std::path::Path>, @@ -2029,6 +2075,41 @@ mod tests { ); } + #[test] + fn sigint_finalizer_releases_the_run_temp_dir_before_exit() { + let status = run_ignored_self_test( + "tests::sigint_finalizer_releases_the_run_temp_dir_before_exit_child", + &[], + ); + assert!(status.success(), "{status}"); + } + + #[test] + #[ignore] + fn sigint_finalizer_releases_the_run_temp_dir_before_exit_child() { + let dir = tempfile::tempdir().unwrap(); + let events = dir.path().join(".anvil/runs/test-run/events.jsonl"); + let mut cfg = config(dir.path().to_path_buf()); + cfg.eval_events_path = Some(events); + + let run_tmp = dir.path().join("sandbox/run-test"); + std::fs::create_dir_all(&run_tmp).unwrap(); + crate::tools::os_sandbox::install_for_test( + crate::tools::os_sandbox::SandboxMode::Auto, + crate::tools::os_sandbox::SandboxPolicy::new(dir.path(), &run_tmp), + crate::tools::os_sandbox::SandboxAvailability::Unavailable("test".to_string()), + ); + + let mut output = Vec::new(); + finalize_interrupted_direct_command(&cfg, "--prompt", None, &mut output).unwrap(); + + assert!( + !run_tmp.exists(), + "the SIGINT finalizer must remove the run temp directory" + ); + crate::tools::os_sandbox::reset_for_test(); + } + #[test] fn run_lifecycle_records_incomplete_stop_reason() { let dir = tempfile::tempdir().unwrap(); @@ -2134,6 +2215,25 @@ mod tests { assert!(!summary.contains("\nStatus: complete\nAction: Repl\nStop reason: completed")); } + #[test] + fn read_only_actions_do_not_need_the_write_sandbox() { + for action in [ + Action::Runs(crate::config::RunsRequest::default()), + Action::UxDemo, + Action::ModelProbe, + Action::Doctor, + ] { + assert!(!action_uses_write_sandbox(&action), "{action:?}"); + } + for action in [ + Action::Repl, + Action::Prompt("goal".into()), + Action::Workflow(PathBuf::from("def.yaml"), PathBuf::from("origin")), + ] { + assert!(action_uses_write_sandbox(&action), "{action:?}"); + } + } + fn run_ignored_self_test(test_name: &str, envs: &[(&str, &str)]) -> std::process::ExitStatus { let exe = std::env::current_exe().unwrap(); let mut command = std::process::Command::new(exe); diff --git a/src/tools/bash.rs b/src/tools/bash.rs index 5a1366e2..622ed0f1 100644 --- a/src/tools/bash.rs +++ b/src/tools/bash.rs @@ -1152,11 +1152,42 @@ fn build_summary(command: &str, kind: BashOutcomeKind, stdout: &str, stderr: &st break; } } - if lines.is_empty() { + let mut summary = if lines.is_empty() { format!("command did not succeed: {command}") } else { lines.join("\n") + }; + if let Some(guidance) = + sandbox_temp_dir_guidance(super::os_sandbox::is_applied(), stdout, stderr) + { + summary.push('\n'); + summary.push_str(guidance); + } + summary +} + +/// The guidance appended when the OS write sandbox is applied and the failure is +/// a permission error against a temp directory — what a bare `mktemp` hits on +/// macOS, where the per-user temp directory is outside the sandbox. It points at +/// the run temp directory, which the sandbox allows. +const SANDBOX_MKTEMP_GUIDANCE: &str = "note: the OS write sandbox denies the macOS per-user temp directory; use `mktemp -p \"$TMPDIR\"` (or a template under `$TMPDIR`) instead of a bare `mktemp`"; + +/// Whether to append [`SANDBOX_MKTEMP_GUIDANCE`]. It needs the sandbox to be +/// applied and a permission error against a temp directory; any other failure +/// gets no guidance. +fn sandbox_temp_dir_guidance(applied: bool, stdout: &str, stderr: &str) -> Option<&'static str> { + if !applied { + return None; } + let combined = format!("{stderr}\n{stdout}").to_ascii_lowercase(); + let permission = combined.contains("operation not permitted") + || combined.contains("permission denied") + || combined.contains("access is denied"); + let temp_dir = combined.contains("mktemp") + || combined.contains("mkstemp") + || combined.contains("/var/folders/") + || combined.contains("/tmp/"); + (permission && temp_dir).then_some(SANDBOX_MKTEMP_GUIDANCE) } #[cfg(test)] @@ -1360,6 +1391,20 @@ mod tests { assert!(truncated.len() >= MAX_STREAM_BYTES); } + #[test] + fn sandbox_temp_dir_guidance_only_for_applied_temp_dir_permission_failures() { + let denied = + "mktemp: mkstemp failed on /var/folders/ab/tmp.XXXXXX: Operation not permitted"; + let guidance = sandbox_temp_dir_guidance(true, "", denied).expect("guidance"); + assert!(guidance.contains("mktemp -p \"$TMPDIR\""), "{guidance}"); + + assert!(sandbox_temp_dir_guidance(false, "", denied).is_none()); + assert!(sandbox_temp_dir_guidance(true, "", "Error: broken").is_none()); + assert!( + sandbox_temp_dir_guidance(true, "", "cat: /etc/shadow: Permission denied").is_none() + ); + } + #[test] fn bash_test_output_extracts_failure_summary() { let dir = tempfile::tempdir().unwrap(); diff --git a/src/tools/os_sandbox.rs b/src/tools/os_sandbox.rs index 00de8785..c2fc4057 100644 --- a/src/tools/os_sandbox.rs +++ b/src/tools/os_sandbox.rs @@ -233,6 +233,71 @@ pub fn install_for_test( store(mode, Some(policy), availability) } +/// The reason recorded when a workflow child node runs in a workspace other than +/// the one the run started in, so the fixed profile would confine it to the wrong +/// workspace. +#[cfg(not(test))] +const DIFFERENT_WORKSPACE_REASON: &str = "workflow node runs in a workspace other than the run workspace; the OS write sandbox is not applied"; + +/// Reconcile the installed sandbox with the workspace a workflow child node runs +/// in. +/// +/// The profile and the write-allow list are fixed at run start to the run's +/// workspace, so applying them to a node running in another workspace would +/// confine it to the wrong place. When the node's workspace differs, record the +/// sandbox as not applied instead: `auto` continues with lexical checks only and +/// `required` fails the node's Bash and verifier commands, so the record never +/// claims isolation that was not in force. When the workspace matches, nothing +/// changes. Returns the status now in effect, or `None` when no sandbox was +/// installed. +/// +/// In a unit-test build this is a no-op, matching [`install`]. +pub fn reconcile_workspace(workspace_root: &Path) -> Option { + #[cfg(not(test))] + { + reconcile_workspace_impl(workspace_root) + } + #[cfg(test)] + { + let _ = workspace_root; + None + } +} + +#[cfg(not(test))] +fn reconcile_workspace_impl(workspace_root: &Path) -> Option { + let mut installed = INSTALLED.lock().expect("sandbox state"); + let (mode, availability, policy, mismatched) = { + let state = installed.as_ref()?; + let mismatched = state + .policy + .as_ref() + .is_some_and(|policy| policy.workspace_root != workspace_root); + ( + state.mode, + state.availability.clone(), + state.policy.clone(), + mismatched, + ) + }; + if !mismatched || !mode.uses_backend() { + return Some(SandboxStatus::from_parts(mode, &availability)); + } + if !matches!(availability, SandboxAvailability::Available) { + // The backend is already unavailable; keep the reason the run started + // with instead of overwriting it. + return Some(SandboxStatus::from_parts(mode, &availability)); + } + let availability = SandboxAvailability::Unavailable(DIFFERENT_WORKSPACE_REASON.to_string()); + let status = SandboxStatus::from_parts(mode, &availability); + *installed = Some(Installed { + policy, + mode, + availability, + }); + Some(status) +} + fn store( mode: SandboxMode, policy: Option, @@ -254,6 +319,34 @@ pub fn reset_for_test() { *installed = None; } +/// Remove the installed run temp directory. Best effort and idempotent, so it is +/// safe on every exit path (success, failure, interruption, or an unwinding +/// panic). The event stream and the workspace are untouched. +pub fn cleanup() { + let policy = { + let mut installed = INSTALLED.lock().expect("sandbox state"); + installed.as_mut().and_then(|state| state.policy.take()) + }; + if let Some(policy) = policy { + let _ = std::fs::remove_dir_all(policy.run_tmp_dir); + } +} + +/// Removes the run temp directory when dropped, so a run leaves no temp +/// directory behind regardless of how it ends. +pub struct SandboxCleanupGuard; + +impl Drop for SandboxCleanupGuard { + fn drop(&mut self) { + cleanup(); + } +} + +/// A guard that runs [`cleanup`] on drop. +pub fn cleanup_guard() -> SandboxCleanupGuard { + SandboxCleanupGuard +} + /// The status recorded at run start, if any. pub fn status() -> Option { let installed = INSTALLED.lock().expect("sandbox state"); @@ -262,6 +355,11 @@ pub fn status() -> Option { .map(|installed| SandboxStatus::from_parts(installed.mode, &installed.availability)) } +/// Whether the backend is currently applied for this run. +pub fn is_applied() -> bool { + status().is_some_and(|status| status.applied) +} + /// The one-time startup warning for `auto` when the backend is unavailable. pub fn startup_warning() -> Option { let status = status()?; diff --git a/tests/issue502_os_sandbox_write.rs b/tests/issue502_os_sandbox_write.rs index eef2dc04..49aaa128 100644 --- a/tests/issue502_os_sandbox_write.rs +++ b/tests/issue502_os_sandbox_write.rs @@ -85,18 +85,24 @@ fn backend_available() -> bool { matches!(os_sandbox::probe(), SandboxAvailability::Available) } -/// Install an available sandbox for the fixture and create its run temp dir. +/// Install an available sandbox whose write-allow list is `workspace` and create +/// its run temp dir. #[cfg(target_os = "macos")] -fn install_available(fixture: &Fixture) { - let run_tmp = run_tmp_path(fixture); - std::fs::create_dir_all(&run_tmp).unwrap(); +fn install_available_for(workspace: &Path, run_tmp: &Path) { + std::fs::create_dir_all(run_tmp).unwrap(); os_sandbox::install_for_test( SandboxMode::Auto, - SandboxPolicy::new(&fixture.root, &run_tmp), + SandboxPolicy::new(workspace, run_tmp), SandboxAvailability::Available, ); } +/// Install an available sandbox for the fixture and create its run temp dir. +#[cfg(target_os = "macos")] +fn install_available(fixture: &Fixture) { + install_available_for(&fixture.root, &run_tmp_path(fixture)); +} + fn install_unavailable(mode: SandboxMode, fixture: &Fixture, reason: &str) { os_sandbox::install_for_test( mode, @@ -341,7 +347,7 @@ fn applied_cargo_build_succeeds() { std::fs::create_dir_all(&src).unwrap(); std::fs::write( fixture.root.join("Cargo.toml"), - "[package]\nname = \"issue502-probe\"\nversion = \"0.1.0\"\nedition = \"2021\"\n", + "[package]\nname = \"issue502-probe\"\nversion = \"0.1.0\"\nedition = \"2021\"\n\n[workspace]\n", ) .unwrap(); std::fs::write(src.join("main.rs"), "fn main() {}\n").unwrap(); @@ -428,6 +434,193 @@ fn off_never_claims_isolation() { os_sandbox::reset_for_test(); } +/// A workflow child node whose workspace differs from the run's is recorded as +/// not isolated, so the fixed parent profile is never applied to the wrong +/// workspace. This is the behavior `run_resolved_config_for_workflow` drives for +/// every node. +#[test] +fn mismatched_node_workspace_records_the_sandbox_not_applied() { + let _guard = sandbox_lock(); + let fixture = fixture(); + let parent = fixture._dir.path().join("parent"); + std::fs::create_dir_all(&parent).unwrap(); + let parent_tmp = fixture._dir.path().join("parent-run-tmp"); + std::fs::create_dir_all(&parent_tmp).unwrap(); + os_sandbox::install_for_test( + SandboxMode::Auto, + SandboxPolicy::new(&parent, &parent_tmp), + SandboxAvailability::Available, + ); + + let status = os_sandbox::reconcile_workspace(&fixture.root).expect("installed"); + assert!(!status.applied, "{status:?}"); + assert!( + status + .reason + .as_deref() + .is_some_and(|reason| !reason.is_empty()), + "{status:?}" + ); + // Idempotent: a later node in the same workspace keeps the same record. + assert_eq!( + os_sandbox::reconcile_workspace(&fixture.root).expect("installed"), + status + ); + os_sandbox::reset_for_test(); +} + +/// `required` fails the node's Bash when the node workspace differs, so a +/// "required" run cannot silently continue without the sandbox. +#[test] +fn required_fails_when_the_node_workspace_differs() { + let _guard = sandbox_lock(); + let fixture = fixture(); + let parent = fixture._dir.path().join("parent"); + std::fs::create_dir_all(&parent).unwrap(); + let parent_tmp = fixture._dir.path().join("parent-run-tmp"); + std::fs::create_dir_all(&parent_tmp).unwrap(); + os_sandbox::install_for_test( + SandboxMode::Required, + SandboxPolicy::new(&parent, &parent_tmp), + SandboxAvailability::Available, + ); + + let status = os_sandbox::reconcile_workspace(&fixture.root).expect("installed"); + assert!(!status.applied, "{status:?}"); + + let outcome = run_via_bash(&fixture.root, "true"); + assert_eq!(outcome.kind, BashOutcomeKind::Blocked, "{outcome:?}"); + assert!( + outcome.summary.contains("bash_sandbox_unavailable"), + "{}", + outcome.summary + ); + os_sandbox::reset_for_test(); +} + +/// With the backend applied, a mismatched node stops being confined by the +/// parent's policy and is not claimed as isolated; the child write is no longer +/// denied on the parent's behalf. +#[cfg(target_os = "macos")] +#[test] +fn applied_mismatch_drops_the_false_isolation_claim() { + let _guard = sandbox_lock(); + if !backend_available() { + eprintln!("SKIP: sandbox backend unavailable (nested sandbox?)"); + return; + } + let fixture = fixture(); + let parent = fixture._dir.path().join("parent"); + std::fs::create_dir_all(&parent).unwrap(); + let parent_tmp = fixture._dir.path().join("parent-run-tmp"); + install_available_for(&parent, &parent_tmp); + + // Before reconciling, the parent policy confines the child to the wrong + // workspace, so a child write is denied. + let before = run_via_bash(&fixture.root, "echo child > origin.txt"); + assert!(!before.is_success(), "{before:?}"); + assert!(!fixture.root.join("origin.txt").exists()); + + let status = os_sandbox::reconcile_workspace(&fixture.root).expect("installed"); + assert!(!status.applied, "{status:?}"); + + // After reconciling, no isolation is claimed or applied for the child. + let after = run_via_bash(&fixture.root, "echo child > origin.txt"); + assert!(after.is_success(), "{after:?}"); + assert!(fixture.root.join("origin.txt").is_file()); + os_sandbox::reset_for_test(); +} + +/// The default run (origin == workspace) keeps the install untouched: the same +/// temp dir remains and the record still reports the sandbox applied. +#[cfg(target_os = "macos")] +#[test] +fn applied_reconcile_is_a_no_op_when_the_node_workspace_matches() { + let _guard = sandbox_lock(); + if !backend_available() { + eprintln!("SKIP: sandbox backend unavailable (nested sandbox?)"); + return; + } + let fixture = fixture(); + install_available(&fixture); + let run_tmp = run_tmp_path(&fixture); + + let status = os_sandbox::reconcile_workspace(&fixture.root).expect("installed"); + assert!(status.applied, "{status:?}"); + assert!(run_tmp.is_dir(), "the run temp dir must be kept"); + + let outcome = run_via_bash(&fixture.root, "echo ok > keeps.txt"); + assert!(outcome.is_success(), "{outcome:?}"); + assert!(fixture.root.join("keeps.txt").is_file()); + os_sandbox::reset_for_test(); +} + +/// A bare `mktemp` fails inside the sandbox, and its summary points at the run +/// temp directory instead of leaving the model to guess. +#[cfg(target_os = "macos")] +#[test] +fn applied_bare_mktemp_failure_summarizes_the_tmpdir_guidance() { + let _guard = sandbox_lock(); + if !backend_available() { + eprintln!("SKIP: sandbox backend unavailable (nested sandbox?)"); + return; + } + let fixture = fixture(); + install_available(&fixture); + + let outcome = run_via_bash(&fixture.root, "mktemp"); + assert_eq!(outcome.kind, BashOutcomeKind::CommandFailed, "{outcome:?}"); + assert!( + outcome.summary.contains("mktemp -p \"$TMPDIR\""), + "{}", + outcome.summary + ); + os_sandbox::reset_for_test(); +} + +/// Without an applied sandbox, the same temp-dir permission failure gets no +/// mktemp guidance. +#[test] +fn no_mktemp_guidance_without_an_applied_sandbox() { + let _guard = sandbox_lock(); + let fixture = fixture(); + install_unavailable(SandboxMode::Auto, &fixture, NESTED); + + let outcome = run_via_bash( + &fixture.root, + "printf 'mktemp: mkstemp failed: Operation not permitted\\n' >&2; exit 1", + ); + assert_eq!(outcome.kind, BashOutcomeKind::CommandFailed, "{outcome:?}"); + assert!( + !outcome.summary.contains("mktemp -p \"$TMPDIR\""), + "{}", + outcome.summary + ); + os_sandbox::reset_for_test(); +} + +/// A run creates its temp dir at install and leaves none behind after cleanup, +/// which the CLI runs on every exit path. +#[test] +fn install_creates_the_run_temp_dir_and_cleanup_removes_it() { + let _guard = sandbox_lock(); + let fixture = fixture(); + let state_dir = fixture._dir.path().join("state"); + + let status = os_sandbox::install(SandboxMode::Off, &fixture.root, &state_dir); + assert!(!status.applied); + let sandbox_dir = state_dir.join("sandbox"); + assert!(sandbox_dir.is_dir(), "install must create the run temp dir"); + assert_eq!(std::fs::read_dir(&sandbox_dir).unwrap().count(), 1); + + os_sandbox::cleanup(); + let remaining = std::fs::read_dir(&sandbox_dir) + .map(|entries| entries.count()) + .unwrap_or(0); + assert_eq!(remaining, 0, "cleanup must remove the run temp dir"); + os_sandbox::reset_for_test(); +} + fn lexical_triple(root: &Path, command: &str) -> (bool, bool, bool) { ( path_confinement_rejection(command, root).is_some(), From 0c57448f723096c163bfba39499f3306e686c2ac Mon Sep 17 00:00:00 2001 From: kewton Date: Tue, 13 Oct 2026 09:22:11 +0900 Subject: [PATCH 2/3] End the run's sandbox on cleanup so required never passes through `cleanup()` removed the policy but left `availability = Available`, so `is_applied()` stayed true and `wrap_command` passed a command through unwrapped under `required`. A child spawned between the guard drop, or the SIGINT finalizer's cleanup and `exit(130)`, could then run unconfined while the record still claimed the sandbox was applied. `cleanup()` now also moves an applied sandbox to `Unavailable("run finished")`, and `wrap_command` refuses under `required` when the backend is available but no policy is installed. Chose "mark unavailable on cleanup" over "keep the policy": it is the smaller change, leaves `auto`/`off` untouched, and avoids handing children a policy whose run temp directory was just deleted. The `wrap_command` refusal hardens the policy-missing branch so the invariant cannot silently regress. Test: cleanup_ends_the_run_without_leaving_the_sandbox_applied (Required + Available via install_for_test, then cleanup) fails on 4dc21624 and passes now. Co-authored-by: CommandCodeBot --- src/tools/os_sandbox.rs | 41 +++++++++++++++++++++++++----- tests/issue502_os_sandbox_write.rs | 32 +++++++++++++++++++++++ 2 files changed, 66 insertions(+), 7 deletions(-) diff --git a/src/tools/os_sandbox.rs b/src/tools/os_sandbox.rs index c2fc4057..97d14eef 100644 --- a/src/tools/os_sandbox.rs +++ b/src/tools/os_sandbox.rs @@ -39,6 +39,11 @@ pub const UNAVAILABLE_KIND: &str = "bash_sandbox_unavailable"; /// The reason recorded when the sandbox is explicitly disabled. const DISABLED_REASON: &str = "disabled by --sandbox off"; +/// The reason recorded once a run has finished and its sandbox is torn down. +const RUN_FINISHED_REASON: &str = "run finished; the OS write sandbox is no longer applied"; +/// The refusal reason for a `required` run whose backend is available but whose +/// policy is gone (for example after [`cleanup`]). +const MISSING_POLICY_REASON: &str = "the OS write sandbox has no policy to apply"; /// The reason recorded on platforms with no write-isolation backend in 1a. #[cfg(not(target_os = "macos"))] const NO_PLATFORM_BACKEND: &str = @@ -319,16 +324,26 @@ pub fn reset_for_test() { *installed = None; } -/// Remove the installed run temp directory. Best effort and idempotent, so it is +/// Remove the installed run temp directory and end the run's sandbox. After this +/// the sandbox is no longer reported as applied and `required` refuses instead of +/// passing a command through, so a child spawned while shutting down never runs +/// without the isolation the run promised. Best effort and idempotent, so it is /// safe on every exit path (success, failure, interruption, or an unwinding /// panic). The event stream and the workspace are untouched. pub fn cleanup() { - let policy = { - let mut installed = INSTALLED.lock().expect("sandbox state"); - installed.as_mut().and_then(|state| state.policy.take()) + let mut installed = INSTALLED.lock().expect("sandbox state"); + let Some(state) = installed.as_mut() else { + return; }; - if let Some(policy) = policy { - let _ = std::fs::remove_dir_all(policy.run_tmp_dir); + if matches!(state.availability, SandboxAvailability::Available) { + // The backend was applied; mark the run over so the applied verdict is + // no longer reported and `required` refuses instead of passing through. + state.availability = SandboxAvailability::Unavailable(RUN_FINISHED_REASON.to_string()); + } + let run_tmp_dir = state.policy.take().map(|policy| policy.run_tmp_dir); + drop(installed); + if let Some(run_tmp_dir) = run_tmp_dir { + let _ = std::fs::remove_dir_all(run_tmp_dir); } } @@ -406,7 +421,19 @@ pub fn wrap_command(command: Command) -> Result { }; match policy { Some(policy) => Ok(apply_backend(command, &policy)), - None => Ok(command), + None => { + // The state says the backend is available but no policy is + // installed (for example once the run's sandbox is torn + // down). `required` must refuse rather than pass a command + // through unconfined. + if mode == SandboxMode::Required { + Err(SandboxRefusal { + reason: MISSING_POLICY_REASON.to_string(), + }) + } else { + Ok(command) + } + } } } } diff --git a/tests/issue502_os_sandbox_write.rs b/tests/issue502_os_sandbox_write.rs index 49aaa128..85be68b2 100644 --- a/tests/issue502_os_sandbox_write.rs +++ b/tests/issue502_os_sandbox_write.rs @@ -621,6 +621,38 @@ fn install_creates_the_run_temp_dir_and_cleanup_removes_it() { os_sandbox::reset_for_test(); } +/// `cleanup` ends the run: the sandbox must no longer be reported as applied and +/// `required` must not pass a command through unwrapped, so a child spawned +/// during shutdown never runs without the confinement the run promised. +#[test] +fn cleanup_ends_the_run_without_leaving_the_sandbox_applied() { + let _guard = sandbox_lock(); + let fixture = fixture(); + let run_tmp = run_tmp_path(&fixture); + std::fs::create_dir_all(&run_tmp).unwrap(); + os_sandbox::install_for_test( + SandboxMode::Required, + SandboxPolicy::new(&fixture.root, &run_tmp), + SandboxAvailability::Available, + ); + assert!(os_sandbox::is_applied()); + + os_sandbox::cleanup(); + + assert!( + !os_sandbox::is_applied(), + "cleanup must not leave the sandbox reported as applied" + ); + let error = os_sandbox::wrap_command(std::process::Command::new("sh")) + .expect_err("required must refuse instead of passing through after cleanup"); + assert!( + error.summary().contains("bash_sandbox_unavailable"), + "{}", + error.summary() + ); + os_sandbox::reset_for_test(); +} + fn lexical_triple(root: &Path, command: &str) -> (bool, bool, bool) { ( path_confinement_rejection(command, root).is_some(), From 6c4e6f91e2a5dc3da127c78946335dad6c372c28 Mon Sep 17 00:00:00 2001 From: kewton Date: Tue, 13 Oct 2026 09:42:29 +0900 Subject: [PATCH 3/3] Add unit tests for the missing-policy wrap_command decision Cover the branch where the backend is available but the run's policy is gone: `required` refuses with `bash_sandbox_unavailable` and `auto` continues as before. The applied integration test cannot reach this branch because `cleanup` also marks the sandbox unavailable, so cargo mutants' `==` -> `!=` mutation in that branch went uncaught. The tests set `INSTALLED` directly under the module's existing serialized test state, so no production API or field is added. Co-authored-by: CommandCodeBot --- src/tools/os_sandbox.rs | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/src/tools/os_sandbox.rs b/src/tools/os_sandbox.rs index 97d14eef..71a2d98b 100644 --- a/src/tools/os_sandbox.rs +++ b/src/tools/os_sandbox.rs @@ -547,6 +547,44 @@ mod tests { assert_eq!(status.reason, None); } + /// A run's sandbox is torn down by taking its policy, which can leave the + /// backend available with no policy. `required` must refuse instead of + /// passing the command through unconfined. + #[test] + fn required_refuses_when_available_without_a_policy() { + let _serial = SANDBOX_TEST_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + let _guard = TestStateGuard; + *INSTALLED.lock().expect("sandbox state") = Some(Installed { + policy: None, + mode: SandboxMode::Required, + availability: SandboxAvailability::Available, + }); + + let error = wrap_command(Command::new("sh")).expect_err("required must refuse"); + assert_eq!( + error.summary(), + format!("{UNAVAILABLE_KIND}: {MISSING_POLICY_REASON}") + ); + } + + /// `auto` keeps its previous behavior for the same state. + #[test] + fn wrap_auto_continues_when_available_without_a_policy() { + let _serial = SANDBOX_TEST_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + let _guard = TestStateGuard; + *INSTALLED.lock().expect("sandbox state") = Some(Installed { + policy: None, + mode: SandboxMode::Auto, + availability: SandboxAvailability::Available, + }); + + assert!(wrap_command(Command::new("sh")).is_ok()); + } + struct TestStateGuard; impl Drop for TestStateGuard {