diff --git a/src/openhuman/tools/impl/system/node_exec.rs b/src/openhuman/tools/impl/system/node_exec.rs index eb30dce6b..7646cce56 100644 --- a/src/openhuman/tools/impl/system/node_exec.rs +++ b/src/openhuman/tools/impl/system/node_exec.rs @@ -407,4 +407,48 @@ mod tests { ); } } + + /// Regression guard for #3238. + /// + /// `node_exec` resolves caller-supplied `script_path` values against + /// `security.action_dir` (the agent's writable sandbox), never + /// `security.workspace_dir` (internal product state). If a future + /// refactor changes `NodeExecTool::execute` to pass + /// `&self.security.workspace_dir` to `resolve_script_path`, scripts + /// would resolve into the internal denylist instead of the action + /// sandbox, which is exactly the action/internal split that + /// PR #3074 prevents. + /// + /// The behavioural end-to-end test for the CWD plumbing lives in + /// `shell.rs` (`shell_pwd_returns_action_dir_not_workspace_dir`) — + /// `node_exec` shares the same `runtime.build_shell_command(&command, + /// &self.security.action_dir)` call site, and the source-grep guard + /// in `shell.rs` (`shell_family_tools_route_cwd_through_action_dir`) + /// covers all three system tools. This test pins the script-resolution + /// contract specifically for `node_exec` by exercising + /// `resolve_script_path` against an `action_dir` distinct from + /// `workspace_dir`. + #[test] + fn resolve_script_path_targets_action_dir_not_workspace_dir() { + let action_dir = std::path::Path::new("/tmp/action-sandbox-3238"); + let workspace_dir = std::path::Path::new("/tmp/internal-workspace-3238"); + + let resolved = resolve_script_path(action_dir, "scripts/run.js") + .expect("relative script under action_dir must resolve"); + assert_eq!( + resolved, + action_dir.join("scripts/run.js"), + "script_path must resolve under action_dir, not workspace_dir (see #3238)" + ); + assert!( + resolved.starts_with(action_dir), + "resolved path must be under action_dir; got {}", + resolved.display() + ); + assert!( + !resolved.starts_with(workspace_dir), + "resolved path leaked into workspace_dir; got {}", + resolved.display() + ); + } } diff --git a/src/openhuman/tools/impl/system/shell.rs b/src/openhuman/tools/impl/system/shell.rs index 51cb9ba58..09f8e6b6c 100644 --- a/src/openhuman/tools/impl/system/shell.rs +++ b/src/openhuman/tools/impl/system/shell.rs @@ -479,6 +479,119 @@ mod tests { assert!(tool.external_effect_with_args(&json!({"command": "rm -rf /"}))); } + /// End-to-end regression guard for #3238. + /// + /// PR #3074 split `Config.action_dir` (the agent's read/write root) + /// from `Config.workspace_dir` (internal product state). `ShellTool` + /// is contractually obligated to spawn its child process with + /// `current_dir = security.action_dir` so `pwd` inside the shell + /// reports the action sandbox path, never `workspace_dir` and never + /// the cargo-test caller's CWD. + /// + /// This test constructs a `SecurityPolicy` whose `action_dir` is a + /// fresh tempdir (distinct from `workspace_dir` and from `cwd`), + /// runs `pwd`, and asserts the captured stdout canonicalises to the + /// same path as `action_dir`. If `ShellTool::run_with_security` + /// stops passing `&security.action_dir` to `build_shell_command` + /// (or `build_shell_command` stops calling `current_dir`), this + /// test fails before the regression ships. + #[cfg(not(windows))] + #[tokio::test] + async fn shell_pwd_returns_action_dir_not_workspace_dir() { + let action_tmp = tempfile::tempdir().expect("create action tempdir"); + let workspace_tmp = tempfile::tempdir().expect("create workspace tempdir"); + let security = Arc::new(SecurityPolicy { + autonomy: AutonomyLevel::Supervised, + workspace_dir: workspace_tmp.path().to_path_buf(), + action_dir: action_tmp.path().to_path_buf(), + ..SecurityPolicy::default() + }); + let tool = ShellTool::new(security.clone(), test_runtime(), test_audit()); + + let result = tool + .execute(json!({"command": "pwd"})) + .await + .expect("pwd should execute without harness error"); + assert!( + !result.is_error, + "pwd unexpectedly errored: {}", + result.output() + ); + + // Canonicalise both sides — on macOS `/tmp` is a symlink to + // `/private/tmp`, so the raw strings won't match even when the + // paths are the same. + let reported = std::path::PathBuf::from(result.output().trim()); + let actual = reported.canonicalize().unwrap_or_else(|_| reported.clone()); + let expected = security + .action_dir + .canonicalize() + .unwrap_or_else(|_| security.action_dir.clone()); + let workspace_canon = security + .workspace_dir + .canonicalize() + .unwrap_or_else(|_| security.workspace_dir.clone()); + + assert_eq!( + actual, + expected, + "pwd must report `action_dir`. got `{}`, expected `{}`. \ + If this fails, `ShellTool::run_with_security` likely stopped \ + passing `&security.action_dir` to `runtime.build_shell_command`, \ + or `build_shell_command` stopped calling `current_dir(...)`. See #3238.", + actual.display(), + expected.display(), + ); + assert_ne!( + actual, workspace_canon, + "pwd reported `workspace_dir` instead of `action_dir` — the \ + action/internal split is broken. See #3074, #3238." + ); + } + + /// Source-level regression guard for #3238. + /// + /// Locks in the contract that the three shell-family acting tools + /// (`shell`, `node_exec`, `npm_exec`) resolve their CWD against + /// `security.action_dir`, never `security.workspace_dir`. The + /// behavioural assertion above covers `shell`; this guard catches + /// regressions in `node_exec` / `npm_exec` without requiring a real + /// Node.js install in CI (their `execute()` path runs + /// `NodeBootstrap::resolve()` first, which is brittle to mock). + /// + /// If a future refactor accidentally switches any of these tools + /// back to `workspace_dir`, this assertion fires at compile-time + /// string-match level. + #[test] + fn shell_family_tools_route_cwd_through_action_dir() { + 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"); + + // Compose forbidden patterns at runtime so this test's own source + // doesn't trigger the contains() check on itself. + let bad_field = format!("self.security.{}_dir", "workspace"); + let bad_call_1 = format!("build_shell_command(&command, &{bad_field})"); + let bad_call_2 = format!("build_shell_command(command, &{bad_field})"); + + for (name, src) in [ + ("shell.rs", SHELL_SRC), + ("node_exec.rs", NODE_EXEC_SRC), + ("npm_exec.rs", NPM_EXEC_SRC), + ] { + assert!( + src.contains("self.security.action_dir"), + "{name} must reference `self.security.action_dir` for tool CWD \ + (see #3074, #3238)" + ); + assert!( + !src.contains(&bad_call_1) && !src.contains(&bad_call_2), + "{name} must not pass `workspace_dir` to `build_shell_command` — \ + acting tools spawn into `action_dir`. See #3074, #3238." + ); + } + } + #[tokio::test] async fn shell_readonly_allows_reads_blocks_writes() { let security = test_security(AutonomyLevel::ReadOnly);