diff --git a/crates/openshell-supervisor-process/src/child_env.rs b/crates/openshell-supervisor-process/src/child_env.rs index 32eecbee35..0d176f427f 100644 --- a/crates/openshell-supervisor-process/src/child_env.rs +++ b/crates/openshell-supervisor-process/src/child_env.rs @@ -1,9 +1,277 @@ // SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 +use std::io::{Read, Write}; use std::path::Path; +use openshell_core::policy::SandboxPolicy; + const LOCAL_NO_PROXY: &str = "127.0.0.1,localhost,::1"; +pub const DEFAULT_CHILD_PATH: &str = "/usr/local/bin:/usr/bin:/bin"; +const STANDARD_SBIN_PATHS: &[&str] = &["/usr/local/sbin", "/usr/sbin", "/sbin"]; +const ENSURE_STANDARD_SBIN_PATHS_SCRIPT: &str = "for dir in /usr/local/sbin /usr/sbin /sbin; do case \":${PATH:-}:\" in *:\"$dir\":*) ;; *) PATH=\"${PATH:+$PATH:}$dir\" ;; esac; done; export PATH"; +const STARTUP_SNIPPET_MARKER: &str = "# OpenShell standard sbin PATH"; +const PROFILE_D_SNIPPET_PATH: &str = "/etc/profile.d/openshell-standard-sbin-path.sh"; + +enum StartupFile { + Missing, + Regular(String), + Unsafe, +} + +pub fn standard_sbin_path_repair_enabled(policy: &SandboxPolicy) -> bool { + let cdi_context = std::env::var(openshell_core::sandbox_env::CDI_CONTEXT); + standard_sbin_path_repair_enabled_for_context(policy, cdi_context.as_deref().ok()) +} + +fn standard_sbin_path_repair_enabled_for_context( + policy: &SandboxPolicy, + cdi_context: Option<&str>, +) -> bool { + cdi_context + .map(str::trim) + .is_some_and(|path| path == openshell_core::cdi::CDI_CONTEXT_PATH) + && policy_has_standard_sbin_path(policy) +} + +fn policy_has_standard_sbin_path(policy: &SandboxPolicy) -> bool { + policy + .filesystem + .read_only + .iter() + .chain(policy.filesystem.read_write.iter()) + .any(|path| STANDARD_SBIN_PATHS.iter().any(|dir| path.starts_with(dir))) +} + +pub fn child_path_from_env(repair_standard_sbin: bool) -> String { + let path = std::env::var("PATH") + .ok() + .filter(|path| !path.trim().is_empty()) + .unwrap_or_else(|| DEFAULT_CHILD_PATH.to_string()); + + maybe_path_with_standard_sbin_paths(&path, repair_standard_sbin) +} + +pub fn maybe_path_with_standard_sbin_paths(path: &str, repair_standard_sbin: bool) -> String { + if repair_standard_sbin { + path_with_standard_sbin_paths(path) + } else { + path.to_string() + } +} + +pub fn path_with_standard_sbin_paths(path: &str) -> String { + let mut path = if path.trim().is_empty() { + DEFAULT_CHILD_PATH.to_string() + } else { + path.to_string() + }; + + for dir in STANDARD_SBIN_PATHS { + if !path.split(':').any(|entry| entry == *dir) { + if !path.is_empty() { + path.push(':'); + } + path.push_str(dir); + } + } + + path +} + +pub fn shell_command_with_standard_sbin_paths(command: &str) -> String { + format!("{ENSURE_STANDARD_SBIN_PATHS_SCRIPT}\n{command}") +} + +pub fn maybe_shell_command_with_standard_sbin_paths( + command: &str, + repair_standard_sbin: bool, +) -> String { + if repair_standard_sbin { + shell_command_with_standard_sbin_paths(command) + } else { + command.to_string() + } +} + +pub fn install_standard_sbin_path_startup_files(home: Option<&str>) { + let profile_path = Path::new(PROFILE_D_SNIPPET_PATH); + if let Err(error) = write_profile_snippet(profile_path) { + tracing::debug!( + path = %profile_path.display(), + error = %error, + "failed to install OpenShell PATH profile snippet" + ); + } + + if let Some(home) = home { + let bashrc_path = Path::new(home).join(".bashrc"); + if let Err(error) = append_startup_snippet(&bashrc_path) { + tracing::debug!( + path = %bashrc_path.display(), + error = %error, + "failed to install OpenShell PATH shell startup snippet" + ); + } + } +} + +fn startup_snippet() -> String { + format!("{STARTUP_SNIPPET_MARKER}\n{ENSURE_STANDARD_SBIN_PATHS_SCRIPT}\n") +} + +fn write_profile_snippet(path: &Path) -> std::io::Result<()> { + if let Some(parent) = path.parent() { + ensure_directory_without_symlink(parent)?; + } + + let snippet = startup_snippet(); + match read_startup_file(path)? { + StartupFile::Regular(content) if content == snippet => return Ok(()), + StartupFile::Regular(_) | StartupFile::Missing => {} + StartupFile::Unsafe => return Ok(()), + } + write_startup_file(path, &snippet) +} + +fn append_startup_snippet(path: &Path) -> std::io::Result<()> { + if let Some(parent) = path.parent() + && !existing_directory_without_symlink(parent)? + { + return Ok(()); + } + + let existing = match read_startup_file(path)? { + StartupFile::Regular(content) => content, + StartupFile::Missing | StartupFile::Unsafe => return Ok(()), + }; + if existing.contains(STARTUP_SNIPPET_MARKER) { + return Ok(()); + } + + let snippet = startup_snippet(); + let mut file = open_startup_file_for_append(path)?; + if !existing.is_empty() && !existing.ends_with('\n') { + file.write_all(b"\n")?; + } + file.write_all(snippet.as_bytes()) +} + +fn read_startup_file(path: &Path) -> std::io::Result { + let metadata = match std::fs::symlink_metadata(path) { + Ok(metadata) => metadata, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + return Ok(StartupFile::Missing); + } + Err(error) => return Err(error), + }; + let file_type = metadata.file_type(); + if file_type.is_symlink() || !file_type.is_file() { + tracing::debug!( + path = %path.display(), + "skipping OpenShell PATH startup repair for non-regular file" + ); + return Ok(StartupFile::Unsafe); + } + + let mut content = String::new(); + open_startup_file_for_read(path)?.read_to_string(&mut content)?; + Ok(StartupFile::Regular(content)) +} + +fn ensure_directory_without_symlink(path: &Path) -> std::io::Result<()> { + match std::fs::symlink_metadata(path) { + Ok(metadata) if metadata.file_type().is_symlink() => Err(std::io::Error::other(format!( + "directory '{}' is a symlink", + path.display() + ))), + Ok(metadata) if metadata.is_dir() => Ok(()), + Ok(_) => Err(std::io::Error::other(format!( + "'{}' is not a directory", + path.display() + ))), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + if let Some(parent) = path.parent() { + ensure_directory_without_symlink(parent)?; + } + std::fs::create_dir(path) + } + Err(error) => Err(error), + } +} + +fn existing_directory_without_symlink(path: &Path) -> std::io::Result { + if let Some(parent) = path.parent() + && parent != path + && !parent.as_os_str().is_empty() + && !existing_directory_without_symlink(parent)? + { + return Ok(false); + } + + match std::fs::symlink_metadata(path) { + Ok(metadata) if metadata.file_type().is_symlink() => { + tracing::debug!( + path = %path.display(), + "skipping OpenShell PATH startup repair through symlink directory" + ); + Ok(false) + } + Ok(metadata) if metadata.is_dir() => Ok(true), + Ok(_) => { + tracing::debug!( + path = %path.display(), + "skipping OpenShell PATH startup repair through non-directory path" + ); + Ok(false) + } + Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(false), + Err(error) => Err(error), + } +} + +fn write_startup_file(path: &Path, content: &str) -> std::io::Result<()> { + let mut file = no_follow_options() + .write(true) + .create(true) + .truncate(true) + .open(path)?; + ensure_opened_file_is_regular(&file, path)?; + file.write_all(content.as_bytes()) +} + +fn open_startup_file_for_read(path: &Path) -> std::io::Result { + let file = no_follow_options().read(true).open(path)?; + ensure_opened_file_is_regular(&file, path)?; + Ok(file) +} + +fn open_startup_file_for_append(path: &Path) -> std::io::Result { + let file = no_follow_options().append(true).open(path)?; + ensure_opened_file_is_regular(&file, path)?; + Ok(file) +} + +fn ensure_opened_file_is_regular(file: &std::fs::File, path: &Path) -> std::io::Result<()> { + let metadata = file.metadata()?; + if metadata.is_file() { + return Ok(()); + } + Err(std::io::Error::other(format!( + "'{}' is not a regular file", + path.display() + ))) +} + +fn no_follow_options() -> std::fs::OpenOptions { + let mut options = std::fs::OpenOptions::new(); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt as _; + options.custom_flags(libc::O_NOFOLLOW | libc::O_CLOEXEC); + } + options +} pub fn proxy_env_vars(proxy_url: &str) -> [(&'static str, String); 9] { [ @@ -45,6 +313,249 @@ mod tests { use std::process::Command; use std::process::Stdio; + #[test] + fn path_with_standard_sbin_paths_uses_default_for_empty_path() { + assert_eq!( + path_with_standard_sbin_paths(""), + "/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin" + ); + } + + #[test] + fn path_with_standard_sbin_paths_appends_missing_sbin_dirs() { + assert_eq!( + path_with_standard_sbin_paths("/sandbox/.venv/bin:/usr/local/bin:/usr/bin:/bin"), + "/sandbox/.venv/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin" + ); + } + + #[test] + fn path_with_standard_sbin_paths_is_idempotent() { + let path = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"; + assert_eq!(path_with_standard_sbin_paths(path), path); + } + + #[test] + fn maybe_path_with_standard_sbin_paths_respects_gate() { + let path = "/sandbox/.venv/bin:/usr/local/bin:/usr/bin:/bin"; + + assert_eq!(maybe_path_with_standard_sbin_paths(path, false), path); + assert_eq!( + maybe_path_with_standard_sbin_paths(path, true), + "/sandbox/.venv/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin" + ); + } + + #[test] + fn maybe_shell_command_with_standard_sbin_paths_respects_gate() { + assert_eq!( + maybe_shell_command_with_standard_sbin_paths("nvidia-smi -L", false), + "nvidia-smi -L" + ); + assert!( + maybe_shell_command_with_standard_sbin_paths("nvidia-smi -L", true) + .contains("/usr/sbin") + ); + } + + #[test] + fn standard_sbin_repair_requires_expected_cdi_context_and_policy_path() { + let policy = policy_with_read_only(["/usr/sbin/nvidia-smi"]); + + assert!(standard_sbin_path_repair_enabled_for_context( + &policy, + Some(openshell_core::cdi::CDI_CONTEXT_PATH) + )); + assert!(!standard_sbin_path_repair_enabled_for_context( + &policy, + Some("/tmp/cdi-context.json") + )); + assert!(!standard_sbin_path_repair_enabled_for_context( + &policy, None + )); + } + + #[test] + fn standard_sbin_repair_requires_standard_sbin_policy_path() { + let policy = policy_with_read_only(["/usr/local/bin/nvidia-smi"]); + + assert!(!standard_sbin_path_repair_enabled_for_context( + &policy, + Some(openshell_core::cdi::CDI_CONTEXT_PATH) + )); + } + + #[test] + fn standard_sbin_repair_accepts_read_write_standard_sbin_policy_path() { + let mut policy = policy_with_read_only(std::iter::empty::<&str>()); + policy + .filesystem + .read_write + .push("/sbin/vendor-tool".into()); + + assert!(standard_sbin_path_repair_enabled_for_context( + &policy, + Some(openshell_core::cdi::CDI_CONTEXT_PATH) + )); + } + + fn policy_with_read_only( + paths: impl IntoIterator>, + ) -> SandboxPolicy { + SandboxPolicy { + version: 1, + filesystem: openshell_core::policy::FilesystemPolicy { + read_only: paths.into_iter().map(Into::into).collect(), + read_write: Vec::new(), + include_workdir: false, + }, + network: openshell_core::policy::NetworkPolicy::default(), + landlock: openshell_core::policy::LandlockPolicy::default(), + process: openshell_core::policy::ProcessPolicy::default(), + } + } + + #[test] + fn shell_command_with_standard_sbin_paths_extends_runtime_path() { + let command = shell_command_with_standard_sbin_paths("printf '%s' \"$PATH\""); + let output = Command::new("/bin/sh") + .arg("-c") + .arg(format!( + "PATH=/sandbox/.venv/bin:/usr/local/bin:/usr/bin:/bin\n{command}" + )) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .output() + .expect("spawn shell"); + + assert!( + output.status.success(), + "shell command failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + assert_eq!( + String::from_utf8(output.stdout).expect("utf8"), + "/sandbox/.venv/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin" + ); + } + + #[test] + fn startup_snippets_are_idempotent() { + let dir = tempfile::tempdir().expect("tempdir"); + let profile_path = dir.path().join("etc/profile.d/openshell-path.sh"); + let home = dir.path().join("sandbox"); + std::fs::create_dir_all(&home).expect("home dir"); + let bashrc_path = home.join(".bashrc"); + std::fs::write( + &bashrc_path, + "export PATH=\"/sandbox/.venv/bin:/usr/local/bin:/usr/bin:/bin\"\n", + ) + .expect("write bashrc"); + + write_profile_snippet(&profile_path).expect("write profile snippet"); + append_startup_snippet(&bashrc_path).expect("append startup snippet"); + write_profile_snippet(&profile_path).expect("rewrite profile snippet"); + append_startup_snippet(&bashrc_path).expect("append startup snippet again"); + + let profile = std::fs::read_to_string(&profile_path).expect("read profile"); + let bashrc = std::fs::read_to_string(&bashrc_path).expect("read bashrc"); + + assert_eq!(profile.matches(STARTUP_SNIPPET_MARKER).count(), 1); + assert_eq!(bashrc.matches(STARTUP_SNIPPET_MARKER).count(), 1); + assert!(bashrc.contains("export PATH=\"/sandbox/.venv/bin:/usr/local/bin:/usr/bin:/bin\"")); + } + + #[cfg(unix)] + #[test] + fn profile_snippet_skips_symlink() { + use std::os::unix::fs::symlink; + + let dir = tempfile::tempdir().expect("tempdir"); + let target = dir.path().join("target"); + std::fs::write(&target, "keep me").expect("write target"); + let profile_path = dir.path().join("openshell-path.sh"); + symlink(&target, &profile_path).expect("symlink profile"); + + write_profile_snippet(&profile_path).expect("skip symlink profile"); + + assert_eq!( + std::fs::read_to_string(&target).expect("read target"), + "keep me" + ); + assert!( + std::fs::symlink_metadata(&profile_path) + .expect("profile metadata") + .file_type() + .is_symlink() + ); + } + + #[cfg(unix)] + #[test] + fn profile_snippet_rejects_symlink_parent() { + use std::os::unix::fs::symlink; + + let dir = tempfile::tempdir().expect("tempdir"); + let target_dir = dir.path().join("target-dir"); + std::fs::create_dir(&target_dir).expect("target dir"); + let parent = dir.path().join("profile.d"); + symlink(&target_dir, &parent).expect("symlink parent"); + let profile_path = parent.join("openshell-path.sh"); + + let error = write_profile_snippet(&profile_path).expect_err("reject symlink parent"); + + assert!(error.to_string().contains("symlink")); + assert!(!target_dir.join("openshell-path.sh").exists()); + } + + #[cfg(unix)] + #[test] + fn bashrc_snippet_skips_symlink() { + use std::os::unix::fs::symlink; + + let dir = tempfile::tempdir().expect("tempdir"); + let target = dir.path().join("target"); + std::fs::write(&target, "keep me").expect("write target"); + let bashrc_path = dir.path().join(".bashrc"); + symlink(&target, &bashrc_path).expect("symlink bashrc"); + + append_startup_snippet(&bashrc_path).expect("skip symlink bashrc"); + + assert_eq!( + std::fs::read_to_string(&target).expect("read target"), + "keep me" + ); + assert!( + std::fs::symlink_metadata(&bashrc_path) + .expect("bashrc metadata") + .file_type() + .is_symlink() + ); + } + + #[cfg(unix)] + #[test] + fn bashrc_snippet_skips_symlink_parent() { + use std::os::unix::fs::symlink; + + let dir = tempfile::tempdir().expect("tempdir"); + let target_dir = dir.path().join("target-dir"); + std::fs::create_dir(&target_dir).expect("target dir"); + std::fs::write( + target_dir.join(".bashrc"), + "export PATH=\"/sandbox/.venv/bin:/usr/local/bin:/usr/bin:/bin\"\n", + ) + .expect("target bashrc"); + let home = dir.path().join("home"); + symlink(&target_dir, &home).expect("symlink home"); + + append_startup_snippet(&home.join(".bashrc")).expect("skip symlink home"); + + let bashrc = std::fs::read_to_string(target_dir.join(".bashrc")).expect("read target"); + assert!(!bashrc.contains(STARTUP_SNIPPET_MARKER)); + } + #[test] fn apply_proxy_env_includes_node_proxy_opt_in_and_local_bypass() { let mut cmd = Command::new("/usr/bin/env"); diff --git a/crates/openshell-supervisor-process/src/process.rs b/crates/openshell-supervisor-process/src/process.rs index 4b5b10b454..d30e53244e 100644 --- a/crates/openshell-supervisor-process/src/process.rs +++ b/crates/openshell-supervisor-process/src/process.rs @@ -166,12 +166,23 @@ fn strip_supervisor_only_env(cmd: &mut Command) { } } -fn inject_provider_env(cmd: &mut Command, provider_env: &HashMap) { +fn inject_provider_env( + cmd: &mut Command, + provider_env: &HashMap, + repair_standard_sbin: bool, +) { for (key, value) in provider_env { if is_supervisor_only_env_var(key) { continue; } - cmd.env(key, value); + if key == "PATH" { + cmd.env( + key, + child_env::maybe_path_with_standard_sbin_paths(value, repair_standard_sbin), + ); + } else { + cmd.env(key, value); + } } } @@ -550,6 +561,35 @@ pub struct ProcessHandle { pid: u32, } +fn shell_command_arg_index(args: &[String]) -> Option { + for (index, arg) in args.iter().enumerate() { + if arg == "-c" || (arg.starts_with('-') && !arg.starts_with("--") && arg.contains('c')) { + return (index + 1 < args.len()).then_some(index + 1); + } + } + None +} + +fn process_args_with_standard_sbin_paths( + program: &str, + args: &[String], + repair_standard_sbin: bool, +) -> Vec { + let mut args = args.to_vec(); + if !repair_standard_sbin { + return args; + } + + let basename = program.rsplit('/').next().unwrap_or(program); + if matches!(basename, "bash" | "sh") + && let Some(command_index) = shell_command_arg_index(&args) + { + args[command_index] = + child_env::shell_command_with_standard_sbin_paths(&args[command_index]); + } + args +} + impl ProcessHandle { /// Spawn a new process. /// @@ -629,13 +669,16 @@ impl ProcessHandle { ca_paths: Option<&(PathBuf, PathBuf)>, provider_env: &HashMap, ) -> Result { + let repair_standard_sbin = child_env::standard_sbin_path_repair_enabled(policy); + let args = process_args_with_standard_sbin_paths(program, args, repair_standard_sbin); let mut cmd = Command::new(program); - cmd.args(args) + cmd.args(&args) .stdin(Stdio::inherit()) .stdout(Stdio::inherit()) .stderr(Stdio::inherit()) .kill_on_drop(true) - .env(openshell_core::sandbox_env::SANDBOX, "1"); + .env(openshell_core::sandbox_env::SANDBOX, "1") + .env("PATH", child_env::child_path_from_env(repair_standard_sbin)); // Strip supervisor-only identity material from the entrypoint's // inherited environment. The entrypoint drops to the sandbox user @@ -643,7 +686,7 @@ impl ProcessHandle { // supervisor credentials from its inherited environment. strip_supervisor_only_env(&mut cmd); - inject_provider_env(&mut cmd, provider_env); + inject_provider_env(&mut cmd, provider_env, repair_standard_sbin); if let Some(dir) = workspace.root() { cmd.current_dir(dir); @@ -786,19 +829,22 @@ impl ProcessHandle { ca_paths: Option<&(PathBuf, PathBuf)>, provider_env: &HashMap, ) -> Result { + let repair_standard_sbin = child_env::standard_sbin_path_repair_enabled(policy); + let args = process_args_with_standard_sbin_paths(program, args, repair_standard_sbin); let mut cmd = Command::new(program); - cmd.args(args) + cmd.args(&args) .stdin(Stdio::inherit()) .stdout(Stdio::inherit()) .stderr(Stdio::inherit()) .kill_on_drop(true) - .env(openshell_core::sandbox_env::SANDBOX, "1"); + .env(openshell_core::sandbox_env::SANDBOX, "1") + .env("PATH", child_env::child_path_from_env(repair_standard_sbin)); // Strip supervisor-only identity material from the entrypoint's // inherited environment. strip_supervisor_only_env(&mut cmd); - inject_provider_env(&mut cmd, provider_env); + inject_provider_env(&mut cmd, provider_env, repair_standard_sbin); if let Some(dir) = workspace.root() { cmd.current_dir(dir); @@ -2219,6 +2265,35 @@ mod tests { use std::mem::size_of; use std::process::Stdio as StdStdio; + #[test] + fn process_args_wrap_shell_c_command_with_standard_sbin_paths() { + let args = vec!["-lc".to_string(), "nvidia-smi -L".to_string()]; + + let wrapped = process_args_with_standard_sbin_paths("sh", &args, true); + + assert_eq!(wrapped[0], "-lc"); + assert!(wrapped[1].contains("/usr/sbin")); + assert!(wrapped[1].contains("nvidia-smi -L")); + } + + #[test] + fn process_args_leave_non_shell_commands_unchanged() { + let args = vec!["nvidia-smi -L".to_string()]; + + let wrapped = process_args_with_standard_sbin_paths("python", &args, true); + + assert_eq!(wrapped, args); + } + + #[test] + fn process_args_do_not_wrap_when_standard_sbin_repair_disabled() { + let args = vec!["-lc".to_string(), "nvidia-smi -L".to_string()]; + + let wrapped = process_args_with_standard_sbin_paths("sh", &args, false); + + assert_eq!(wrapped, args); + } + /// Helper to create a minimal `SandboxPolicy` with the given process policy. fn policy_with_process(process: ProcessPolicy) -> SandboxPolicy { SandboxPolicy { @@ -2737,13 +2812,61 @@ mod tests { )) .collect(); - inject_provider_env(&mut cmd, &provider_env); + inject_provider_env(&mut cmd, &provider_env, false); let output = cmd.output().await.expect("spawn env"); let stdout = String::from_utf8(output.stdout).expect("utf8"); assert!(stdout.contains("ANTHROPIC_API_KEY=openshell:resolve:env:ANTHROPIC_API_KEY")); } + #[tokio::test] + async fn inject_provider_env_appends_standard_sbin_to_path_when_enabled() { + let mut cmd = Command::new("/usr/bin/env"); + cmd.env_clear() + .stdin(StdStdio::null()) + .stdout(StdStdio::piped()) + .stderr(StdStdio::null()); + + let provider_env = HashMap::from([( + "PATH".to_string(), + "/sandbox/.venv/bin:/usr/local/bin:/usr/bin:/bin".to_string(), + )]); + + inject_provider_env(&mut cmd, &provider_env, true); + + let output = cmd.output().await.expect("spawn env"); + assert!(output.status.success()); + let stdout = String::from_utf8(output.stdout).expect("utf8"); + assert!(stdout.lines().any(|line| { + line == "PATH=/sandbox/.venv/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin" + })); + } + + #[tokio::test] + async fn inject_provider_env_leaves_path_unchanged_when_standard_sbin_repair_disabled() { + let mut cmd = Command::new("/usr/bin/env"); + cmd.env_clear() + .stdin(StdStdio::null()) + .stdout(StdStdio::piped()) + .stderr(StdStdio::null()); + + let provider_env = HashMap::from([( + "PATH".to_string(), + "/sandbox/.venv/bin:/usr/local/bin:/usr/bin:/bin".to_string(), + )]); + + inject_provider_env(&mut cmd, &provider_env, false); + + let output = cmd.output().await.expect("spawn env"); + assert!(output.status.success()); + let stdout = String::from_utf8(output.stdout).expect("utf8"); + assert!( + stdout + .lines() + .any(|line| { line == "PATH=/sandbox/.venv/bin:/usr/local/bin:/usr/bin:/bin" }) + ); + } + #[cfg(unix)] fn sandbox_policy_with_read_write( path: PathBuf, @@ -3621,7 +3744,7 @@ mod tests { ), ]); - inject_provider_env(&mut cmd, &provider_env); + inject_provider_env(&mut cmd, &provider_env, false); let output = cmd.output().await.expect("spawn env"); assert!(output.status.success()); diff --git a/crates/openshell-supervisor-process/src/run.rs b/crates/openshell-supervisor-process/src/run.rs index 91e56b7ec8..de89e8c351 100644 --- a/crates/openshell-supervisor-process/src/run.rs +++ b/crates/openshell-supervisor-process/src/run.rs @@ -104,6 +104,10 @@ pub async fn run_process( )?; } + if crate::child_env::standard_sbin_path_repair_enabled(policy) { + crate::child_env::install_standard_sbin_path_startup_files(workspace.home()); + } + // Eagerly fetch initial settings and install the agent skill if the // proposals flag is on at startup, rather than waiting for the policy // poll loop's first tick. In offline/file-mode there is no gateway, so diff --git a/crates/openshell-supervisor-process/src/ssh.rs b/crates/openshell-supervisor-process/src/ssh.rs index f19375f199..84bf56148d 100644 --- a/crates/openshell-supervisor-process/src/ssh.rs +++ b/crates/openshell-supervisor-process/src/ssh.rs @@ -743,6 +743,19 @@ fn session_user_and_home(policy: &SandboxPolicy, workdir_home: Option<&str>) -> (user, home) } +fn bash_login_command(command: String, repair_standard_sbin: bool) -> Command { + let mut cmd = Command::new("/bin/bash"); + // Login profiles can rewrite PATH after the supervisor-provided child + // environment is applied. Re-append standard sbin directories so + // CDI-injected tools such as /usr/sbin/nvidia-smi remain discoverable. + cmd.arg("-lc") + .arg(child_env::maybe_shell_command_with_standard_sbin_paths( + &command, + repair_standard_sbin, + )); + cmd +} + #[allow(clippy::too_many_arguments)] fn apply_child_env( cmd: &mut Command, @@ -753,8 +766,9 @@ fn apply_child_env( ca_file_paths: Option<&(PathBuf, PathBuf)>, provider_env: &HashMap, user_environment: &HashMap, + repair_standard_sbin: bool, ) { - let path = std::env::var("PATH").unwrap_or_else(|_| "/usr/local/bin:/usr/bin:/bin".into()); + let path = child_env::child_path_from_env(repair_standard_sbin); cmd.env_clear() .env(openshell_core::sandbox_env::SANDBOX, "1") @@ -766,7 +780,14 @@ fn apply_child_env( for (key, value) in user_environment { if !key.starts_with("OPENSHELL_") { - cmd.env(key, value); + if key == "PATH" { + cmd.env( + key, + child_env::maybe_path_with_standard_sbin_paths(value, repair_standard_sbin), + ); + } else { + cmd.env(key, value); + } } } @@ -786,7 +807,14 @@ fn apply_child_env( if is_supervisor_only_env_var(key) { continue; } - cmd.env(key, value); + if key == "PATH" { + cmd.env( + key, + child_env::maybe_path_with_standard_sbin_paths(value, repair_standard_sbin), + ); + } else { + cmd.env(key, value); + } } } @@ -823,17 +851,14 @@ fn spawn_pty_shell( let mut reader = master.try_clone()?; let mut writer = master.try_clone()?; + let repair_standard_sbin = child_env::standard_sbin_path_repair_enabled(policy); let mut cmd = command.map_or_else( || { let mut c = Command::new("/bin/bash"); c.arg("-i"); c }, - |command| { - let mut c = Command::new("/bin/bash"); - c.arg("-lc").arg(command); - c - }, + |command| bash_login_command(command, repair_standard_sbin), ); let term = if pty.term.is_empty() { @@ -854,6 +879,7 @@ fn spawn_pty_shell( ca_file_paths.as_deref(), provider_env, user_environment, + repair_standard_sbin, ); cmd.stdin(stdin).stdout(stdout).stderr(stderr); @@ -978,6 +1004,7 @@ fn spawn_pipe_exec( resolved_identity: ResolvedProcessIdentity, enforcement_mode: ProcessEnforcementMode, ) -> anyhow::Result>> { + let repair_standard_sbin = child_env::standard_sbin_path_repair_enabled(policy); let mut cmd = command.map_or_else( || { // No command — read from stdin. Do *not* pass `-i`; interactive @@ -989,12 +1016,10 @@ fn spawn_pipe_exec( Command::new("/bin/bash") }, |command| { - let mut c = Command::new("/bin/bash"); // Use login shell (-l) so that .profile/.bashrc are sourced and // tool-specific env vars (VIRTUAL_ENV, UV_PYTHON_INSTALL_DIR, etc.) // are available without hardcoding them here. - c.arg("-lc").arg(command); - c + bash_login_command(command, repair_standard_sbin) }, ); @@ -1008,6 +1033,7 @@ fn spawn_pipe_exec( ca_file_paths.as_deref(), provider_env, user_environment, + repair_standard_sbin, ); cmd.stdin(Stdio::piped()) .stdout(Stdio::piped()) @@ -1499,6 +1525,70 @@ mod tests { assert_eq!(output.stdout, b"hello"); } + #[test] + fn apply_child_env_appends_standard_sbin_to_user_path_when_enabled() { + let mut cmd = Command::new("/usr/bin/env"); + cmd.stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()); + + let user_environment = HashMap::from([( + "PATH".to_string(), + "/sandbox/.venv/bin:/usr/local/bin:/usr/bin:/bin".to_string(), + )]); + apply_child_env( + &mut cmd, + "/sandbox", + "sandbox", + "dumb", + None, + None, + &HashMap::new(), + &user_environment, + true, + ); + + let output = cmd.output().expect("spawn env"); + assert!(output.status.success()); + let stdout = String::from_utf8(output.stdout).expect("utf8"); + assert!(stdout.lines().any(|line| { + line == "PATH=/sandbox/.venv/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin" + })); + } + + #[test] + fn apply_child_env_leaves_user_path_unchanged_when_standard_sbin_repair_disabled() { + let mut cmd = Command::new("/usr/bin/env"); + cmd.stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()); + + let user_environment = HashMap::from([( + "PATH".to_string(), + "/sandbox/.venv/bin:/usr/local/bin:/usr/bin:/bin".to_string(), + )]); + apply_child_env( + &mut cmd, + "/sandbox", + "sandbox", + "dumb", + None, + None, + &HashMap::new(), + &user_environment, + false, + ); + + let output = cmd.output().expect("spawn env"); + assert!(output.status.success()); + let stdout = String::from_utf8(output.stdout).expect("utf8"); + assert!( + stdout + .lines() + .any(|line| line == "PATH=/sandbox/.venv/bin:/usr/local/bin:/usr/bin:/bin") + ); + } + /// Verify that the stdin writer delivers all buffered data before exiting /// when the sender is dropped. This ensures channel_eof doesn't cause /// data loss — only signals "no more data after this".