From dd726fda14ba371cafec0d73cc3753b7b545a1d5 Mon Sep 17 00:00:00 2001 From: CodeGhost21 <164498022+CodeGhost21@users.noreply.github.com> Date: Thu, 4 Jun 2026 10:13:30 +0530 Subject: [PATCH] feat(tools/system): route node_exec and npm_exec through sandbox backend like shell (#3309) --- CLAUDE.md | 8 ++ src/openhuman/cwd_jail/noop.rs | 57 +++++++++ src/openhuman/tools/impl/system/node_exec.rs | 117 +++++++++++++++++++ src/openhuman/tools/impl/system/npm_exec.rs | 117 +++++++++++++++++++ src/openhuman/tools/impl/system/shell.rs | 46 ++++++++ 5 files changed, 345 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index aa253b6b3..8691f5131 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -130,6 +130,14 @@ PRs must meet **≥ 80% coverage on changed lines**. Enforced by [`.github/workf Overriding `OPENHUMAN_ACTION_DIR` does **not** weaken the internal denylist — `workspace_dir` is always blocked even if `action_dir` is relocated to overlap it. Adding new internal-state subdirectories under `workspace_dir` requires extending `WORKSPACE_INTERNAL_DIRS` (or `WORKSPACE_INTERNAL_FILES`) in the same change; otherwise an agent tool with a `trusted_root` grant could write into the new subtree. +**Sandbox execution backends** — when an agent's `agent.toml` declares `sandbox_mode = "sandboxed"`, the three shell-family tools (`shell`, `node_exec`, `npm_exec`) route execution through `src/openhuman/sandbox/` instead of the plain `RuntimeAdapter`. The sandbox backend is picked by `sandbox::resolve_sandbox_policy` (`src/openhuman/sandbox/ops.rs`) based on the session origin + `RuntimeConfig`: + +- **Docker** — ephemeral `docker run --rm` containers with `--cap-drop ALL`, `--security-opt no-new-privileges`, `--network none` (remote sessions), read-only rootfs + tmpfs, the action sandbox mounted at the working directory, and labels for orphan cleanup. Preferred for remote / channel / cron / background sessions. +- **Local OS jail (`cwd_jail`)** — Landlock on Linux, Seatbelt on macOS, AppContainer on Windows. Rooted at `Config.action_dir`, with output capture via temp files inside the jail (works around Seatbelt's stdio-forwarding limitation). Preferred for interactive desktop sessions. +- **Noop** — on platforms where `cwd_jail` has no real backend (or when Docker is unavailable and the local backend is unsupported), the sandbox falls back to a documented passthrough that runs the command with `current_dir = action_dir` and `env_clear()` + the `SANDBOX_ENV_PASSTHROUGH` allow-list. The in-Rust path hardening (`is_path_string_allowed`, `is_workspace_internal_path`, structural `check_gated_command` guards, the unconditional `is_always_forbidden` system/credential block) **still applies** regardless of which sandbox backend is selected — defense-in-depth, not "noop = no defense." + +Default `sandbox_mode = "none"` agents keep the historical behavior (plain `runtime.build_shell_command` path), so the sandbox layer is strictly additive and opt-in per agent. `ShellTool::run_with_security`, `NodeExecTool::execute`, and `NpmExecTool::execute` all check `current_sandbox_mode()` (a `tokio` task-local set by the agent harness) and delegate to their `run_sandboxed()` helper when `SandboxMode::Sandboxed` is active. The security/rate-limit/audit checks run *before* the sandbox routing decision, so a denied command is still denied identically in both paths. + **Command permission model (deterministic, fail-closed):** `classify_command` buckets a command into `CommandClass` (`Read` / `Write` / `Network` / `Install` / `Destructive`); an unrecognized command is **`Write`**, never `Read`. `gate_decision(class, tier)` → `Allow` / `Prompt` / `Block`: read-only allows only reads; ask-before-edit prompts every act (file *create* is free, *edit-existing* prompts); full runs read+write but **always-asks** Network/Install/Destructive. Acting tools (`shell`/`node_exec`/`npm_exec`/`file_write`/`edit_file`/`apply_patch`/`git_operations`/`curl`) return `external_effect_with_args() == true` for `Prompt` classes so the harness routes them through the `ApprovalGate` *before* `execute()`; read-only `Block` + structural guards (`check_gated_command`) are enforced in-tool. The LLM may pass a `category` (escalate-only: `max(rust_floor, declared)`). System/credential dirs are an **unconditional** cross-platform block (`is_always_forbidden`, trusted-root-proof). Enforcement is in Rust (`classify_command`/`gate_decision`/`check_gated_command`/`is_path_string_allowed`/`validate_path`), never the system prompt. > ⚠️ **The approval prompt is ON by default** (opt out with `OPENHUMAN_APPROVAL_GATE=0`/`false`, `jsonrpc.rs`). `ApprovalGate::init_global` installs unless disabled, so `try_global()` is `Some` and the prompt is wired end-to-end; with `OPENHUMAN_APPROVAL_GATE=0` the harness skips the intercept and `Prompt`-class calls **run unprompted**. The gate parks only for **interactive chat turns** (a `tokio` task-local chat context is set in `channels/providers/web.rs`; background triage/cron turns carry no context and are allowed through, not gated). It publishes `DomainEvent::ApprovalRequested`, which `ApprovalSurfaceSubscriber` bridges to the `approval_request` web-channel socket event; the frontend (`ChatApprovalRequestEvent` → `chatRuntime.pendingApprovalByThread` → `ApprovalRequestCard` above the composer) surfaces Approve/Deny, routing to the `openhuman.approval_decide` RPC. A typed `yes`/`no` chat reply is also honoured server-side (web.rs ingress router runs before the "newer request aborts the in-flight turn" path); any other text cancels the parked turn and is taken as a fresh message. Unanswered prompts still park to the 10-min TTL → Deny. Read-only blocking, path hardening, structural guards, and classification **are** live regardless of the flag. Full access ships as documented full-trust (not sandboxed). diff --git a/src/openhuman/cwd_jail/noop.rs b/src/openhuman/cwd_jail/noop.rs index cb9901ee3..5276835af 100644 --- a/src/openhuman/cwd_jail/noop.rs +++ b/src/openhuman/cwd_jail/noop.rs @@ -24,3 +24,60 @@ impl JailBackend for NoopBackend { cmd.spawn() } } + +#[cfg(test)] +mod tests { + use super::*; + + /// Regression guard for #3235. + /// + /// On platforms where no OS-level jail backend is available + /// (Landlock / Seatbelt / AppContainer absent or unsupported), + /// `cwd_jail::detect::pick_backend()` returns `NoopBackend`. This + /// test pins the NoopBackend contract — it must always report + /// `is_available() == true` so it's a usable fallback, must + /// identify as `"noop"` so operators can see in logs which backend + /// is active, and `spawn` must be a passthrough that runs the + /// command without enforcement. The in-Rust path hardening in + /// `SecurityPolicy::is_path_string_allowed` / + /// `is_workspace_internal_path` still applies; the noop name + /// reflects "no OS-level isolation," not "no defense at all." + #[test] + fn noop_backend_identifies_as_noop_and_is_always_available() { + let backend = NoopBackend; + assert_eq!(backend.name(), "noop"); + assert!( + backend.is_available(), + "NoopBackend must always be available so it can serve as the \ + documented passthrough on unsupported platforms (see CLAUDE.md \ + 'Sandbox execution backends')." + ); + } + + #[test] + fn noop_backend_spawn_runs_command_passthrough() { + // On a POSIX host, `true` exits 0 immediately. The NoopBackend + // must spawn it successfully without injecting any jail flags, + // proving the passthrough contract. + let backend = NoopBackend; + let jail = Jail::new("/tmp", "noop-test-3235"); + let cmd = if cfg!(windows) { + let mut c = Command::new("cmd"); + c.args(["/C", "exit 0"]); + c + } else { + Command::new("true") + }; + let child = backend + .spawn(&jail, cmd) + .expect("NoopBackend::spawn must passthrough-spawn the child"); + let output = child + .wait_with_output() + .expect("child must run to completion"); + assert!( + output.status.success(), + "passthrough child should succeed; got {:?}", + output.status + ); + } +} diff --git a/src/openhuman/tools/impl/system/node_exec.rs b/src/openhuman/tools/impl/system/node_exec.rs index 7646cce56..fb3811f98 100644 --- a/src/openhuman/tools/impl/system/node_exec.rs +++ b/src/openhuman/tools/impl/system/node_exec.rs @@ -228,6 +228,21 @@ impl Tool for NodeExecTool { unreachable!("guarded above") }; + // When the agent's sandbox mode is `Sandboxed`, route execution + // through the sandbox backend (Docker / OS-level `cwd_jail` / + // documented noop) instead of the native runtime path. Mirrors + // the wiring in `ShellTool::run_with_security` (PR #3261) so + // node_exec gets the same isolation guarantees as shell. The + // security/rate-limit checks above still apply. + if matches!( + crate::openhuman::agent::harness::current_sandbox_mode(), + Some(crate::openhuman::agent::harness::definition::SandboxMode::Sandboxed) + ) { + return Ok(self + .run_sandboxed(&command, &resolved.bin_dir, timeout_secs) + .await); + } + let mut cmd = match self .runtime .build_shell_command(&command, &self.security.action_dir) @@ -298,6 +313,108 @@ impl Tool for NodeExecTool { } } +impl NodeExecTool { + /// Execute a node command through the sandbox backend. Called from + /// `execute()` when the agent's `SandboxMode` is `Sandboxed`. + /// + /// Mirrors `ShellTool::run_sandboxed`. The sandbox policy is resolved + /// from the current `RuntimeConfig` and rooted at + /// `security.action_dir`; on platforms without a real `cwd_jail` + /// backend the local backend falls back to a documented noop with + /// the in-Rust path-hardening guards from `SecurityPolicy` still + /// applying (see CLAUDE.md "Action sandbox vs internal workspace"). + async fn run_sandboxed( + &self, + command: &str, + bin_dir: &std::path::Path, + timeout_secs: u64, + ) -> ToolResult { + use crate::openhuman::sandbox; + + // Load the live `RuntimeConfig` so `resolve_sandbox_policy` derives + // the right backend (Docker / local / noop) from the operator's + // configuration instead of the unconfigured `RuntimeConfig::default()`. + // Falls back to defaults with a warning if the config load fails — + // a failed config read shouldn't block tool execution. (CodeRabbit + // finding on PR #3309.) + let runtime_cfg = match crate::openhuman::config::ops::load_config_with_timeout().await { + Ok(cfg) => cfg.runtime, + Err(err) => { + tracing::warn!( + error = %err, + "[node_exec] failed to load live RuntimeConfig — falling back to defaults" + ); + crate::openhuman::config::RuntimeConfig::default() + } + }; + // `is_remote_session = false` matches `ShellTool::run_sandboxed`'s + // current behavior (PR #3261). Threading the real session origin + // through requires a new `tokio::task_local!` next to + // `CURRENT_AGENT_SANDBOX_MODE` and is the same gap across all three + // shell-family tools; tracked separately so it can be fixed uniformly. + let policy = sandbox::resolve_sandbox_policy( + crate::openhuman::agent::harness::definition::SandboxMode::Sandboxed, + &self.security.action_dir, + &runtime_cfg, + false, + ); + + tracing::debug!( + backend = ?policy.backend, + runtime_kind = ?runtime_cfg.kind, + "[node_exec] routing to sandbox backend" + ); + + // Forward the managed Node.js bin dir on PATH so the child node + // process can resolve `node`, `npm`, `npx`, `corepack` consistently + // with the unsandboxed path. + let mut extra_env = std::collections::HashMap::new(); + let host_path = std::env::var("PATH").unwrap_or_default(); + let sep = if cfg!(windows) { ";" } else { ":" }; + let prepended = if host_path.is_empty() { + bin_dir.to_string_lossy().into_owned() + } else { + format!("{}{}{}", bin_dir.display(), sep, host_path) + }; + extra_env.insert("PATH".to_string(), prepended); + + match sandbox::execute_in_sandbox( + &policy, + command, + &self.security.action_dir, + extra_env, + Duration::from_secs(timeout_secs), + ) + .await + { + Ok(result) => { + if result.timed_out { + ToolResult::error(format!( + "node_exec timed out after {timeout_secs}s and was killed" + )) + } else if result.success() { + if result.stderr.is_empty() { + ToolResult::success(result.stdout) + } else { + ToolResult::success(format!( + "{}\n[stderr]\n{}", + result.stdout, result.stderr + )) + } + } else { + let err_msg = if result.stderr.is_empty() { + result.stdout + } else { + result.stderr + }; + ToolResult::error(err_msg) + } + } + Err(e) => ToolResult::error(format!("Sandbox execution failed: {e}")), + } + } +} + /// POSIX-safe single-quote escaping. Wraps `s` in `'…'`, turning any embedded /// single-quote into the four-char sequence `'\''`. Node bin paths and user /// code pass through untouched semantically, but no shell metacharacter can diff --git a/src/openhuman/tools/impl/system/npm_exec.rs b/src/openhuman/tools/impl/system/npm_exec.rs index f4fce74c5..675b18ed6 100644 --- a/src/openhuman/tools/impl/system/npm_exec.rs +++ b/src/openhuman/tools/impl/system/npm_exec.rs @@ -241,6 +241,21 @@ impl Tool for NpmExecTool { } let command = parts.join(" "); + // When the agent's sandbox mode is `Sandboxed`, route execution + // through the sandbox backend (Docker / OS-level `cwd_jail` / + // documented noop) instead of the native runtime path. Mirrors + // the wiring in `ShellTool::run_with_security` (PR #3261) so + // npm_exec gets the same isolation guarantees as shell. The + // security/rate-limit checks above still apply. + if matches!( + crate::openhuman::agent::harness::current_sandbox_mode(), + Some(crate::openhuman::agent::harness::definition::SandboxMode::Sandboxed) + ) { + return Ok(self + .run_sandboxed(&command, &cwd, &resolved.bin_dir, timeout_secs) + .await); + } + let mut cmd = match self.runtime.build_shell_command(&command, &cwd) { Ok(cmd) => cmd, Err(e) => { @@ -308,6 +323,108 @@ impl Tool for NpmExecTool { } } +impl NpmExecTool { + /// Execute an npm command through the sandbox backend. Called from + /// `execute()` when the agent's `SandboxMode` is `Sandboxed`. + /// + /// Mirrors `ShellTool::run_sandboxed` and `NodeExecTool::run_sandboxed`. + /// The sandbox policy is resolved from the current `RuntimeConfig` and + /// rooted at `security.action_dir` — note that the actual child-process + /// `working_dir` may be a sub-path of `action_dir` (the resolved `cwd` + /// from `cwd_override`), kept consistent with the unsandboxed path. + async fn run_sandboxed( + &self, + command: &str, + cwd: &std::path::Path, + bin_dir: &std::path::Path, + timeout_secs: u64, + ) -> ToolResult { + use crate::openhuman::sandbox; + + // Load the live `RuntimeConfig` so `resolve_sandbox_policy` derives + // the right backend (Docker / local / noop) from the operator's + // configuration instead of the unconfigured `RuntimeConfig::default()`. + // Falls back to defaults with a warning if the config load fails — + // a failed config read shouldn't block tool execution. (CodeRabbit + // finding on PR #3309.) + let runtime_cfg = match crate::openhuman::config::ops::load_config_with_timeout().await { + Ok(cfg) => cfg.runtime, + Err(err) => { + tracing::warn!( + error = %err, + "[npm_exec] failed to load live RuntimeConfig — falling back to defaults" + ); + crate::openhuman::config::RuntimeConfig::default() + } + }; + // `is_remote_session = false` matches `ShellTool::run_sandboxed`'s + // current behavior (PR #3261). Threading the real session origin + // through requires a new `tokio::task_local!` next to + // `CURRENT_AGENT_SANDBOX_MODE` and is the same gap across all three + // shell-family tools; tracked separately so it can be fixed uniformly. + let policy = sandbox::resolve_sandbox_policy( + crate::openhuman::agent::harness::definition::SandboxMode::Sandboxed, + &self.security.action_dir, + &runtime_cfg, + false, + ); + + tracing::debug!( + backend = ?policy.backend, + runtime_kind = ?runtime_cfg.kind, + "[npm_exec] routing to sandbox backend" + ); + + // Forward the managed Node.js bin dir on PATH so npm child invocations + // (e.g. `npm run` spawning user scripts) resolve `node`/`npx` + // consistently with the unsandboxed path. + let mut extra_env = std::collections::HashMap::new(); + let host_path = std::env::var("PATH").unwrap_or_default(); + let sep = if cfg!(windows) { ";" } else { ":" }; + let prepended = if host_path.is_empty() { + bin_dir.to_string_lossy().into_owned() + } else { + format!("{}{}{}", bin_dir.display(), sep, host_path) + }; + extra_env.insert("PATH".to_string(), prepended); + + match sandbox::execute_in_sandbox( + &policy, + command, + cwd, + extra_env, + Duration::from_secs(timeout_secs), + ) + .await + { + Ok(result) => { + if result.timed_out { + ToolResult::error(format!( + "npm_exec timed out after {timeout_secs}s and was killed" + )) + } else if result.success() { + if result.stderr.is_empty() { + ToolResult::success(result.stdout) + } else { + ToolResult::success(format!( + "{}\n[stderr]\n{}", + result.stdout, result.stderr + )) + } + } else { + let err_msg = if result.stderr.is_empty() { + result.stdout + } else { + result.stderr + }; + ToolResult::error(err_msg) + } + } + Err(e) => ToolResult::error(format!("Sandbox execution failed: {e}")), + } + } +} + /// POSIX-safe single-quote escaping (mirrors the helper in `node_exec`). /// Wraps `s` in `'…'`, turning any embedded single-quote into `'\''` so no /// shell metacharacter can escape the quoted string. diff --git a/src/openhuman/tools/impl/system/shell.rs b/src/openhuman/tools/impl/system/shell.rs index 2a2ca53f7..a883b46d5 100644 --- a/src/openhuman/tools/impl/system/shell.rs +++ b/src/openhuman/tools/impl/system/shell.rs @@ -937,4 +937,50 @@ mod tests { result.output() ); } + + /// Regression guard for #3235 (cwd_jail wiring for shell-family tools). + /// + /// PR #3261 wired `ShellTool` to route through `sandbox::execute_in_sandbox` + /// (which uses `cwd_jail` for the local-OS-jail backend) when the + /// active agent's `SandboxMode::Sandboxed` is set. This PR extends the + /// same wiring to `NodeExecTool` and `NpmExecTool`. The behavioural + /// `shell_sandboxed_mode_routes_through_sandbox_backend` test above + /// proves the contract end-to-end for `shell` (no managed-Node + /// dependency); `node_exec` and `npm_exec` cannot run end-to-end in + /// unit tests without a resolved `NodeBootstrap`, so this source-grep + /// guard catches refactors that drop the sandbox check from either + /// tool's `execute()` body. + #[test] + fn shell_family_tools_route_to_sandbox_when_sandboxed_mode_active() { + const SHELL_SRC: &str = include_str!("shell.rs"); + const NODE_EXEC_SRC: &str = include_str!("node_exec.rs"); + const NPM_EXEC_SRC: &str = include_str!("npm_exec.rs"); + + for (name, src) in [ + ("shell.rs", SHELL_SRC), + ("node_exec.rs", NODE_EXEC_SRC), + ("npm_exec.rs", NPM_EXEC_SRC), + ] { + assert!( + src.contains("current_sandbox_mode()"), + "{name} must check `current_sandbox_mode()` to detect SandboxMode::Sandboxed \ + sessions and route through the sandbox backend (see #3235)" + ); + assert!( + src.contains("SandboxMode::Sandboxed"), + "{name} must compare against `SandboxMode::Sandboxed` to opt in to the \ + sandbox routing path (see #3235)" + ); + // Use the call-site pattern `.run_sandboxed(` so the assertion + // doesn't trivially pass on the helper definition itself + // (`fn run_sandboxed(...)`). If `execute()` / `run_with_security()` + // stop delegating, this fires even though the helper still exists. + assert!( + src.contains(".run_sandboxed("), + "{name} must delegate to a `run_sandboxed` helper when the sandbox mode is \ + active (see #3235). Whitespace before `.run_sandboxed` is tolerated; the \ + helper call must appear in the source — *not* just the helper definition." + ); + } + } }