From 6ab2cfeb7c1c757012c2a3a771a681aae8490927 Mon Sep 17 00:00:00 2001 From: louisdevzz Date: Thu, 5 Mar 2026 12:30:17 +0700 Subject: [PATCH 1/3] fix: create sandbox in user workspace instead of temp directory Fixes #24 - Sandbox was being created in system temp directory (/var/folders/.../T/ on macOS, /tmp/ on Linux) making it difficult to monitor, debug, and clean up. Changes: - Sandbox now created in ~/.zerobuild/workspace/sandbox/ by default - Support ZEROBUILD_SANDBOX_PATH environment variable for custom location - Updated module documentation to reflect new behavior - Improved error messages with full path information Benefits: - Easy to find: Consistent location under user's home directory - Simple monitoring: Users can check ~/.zerobuild/workspace/sandbox/ - Controlled cleanup: Users decide when to clean up, not the OS - Better debugging: Easy to inspect sandbox contents --- src/sandbox/local.rs | 40 ++++++++++++++++++++++++++++++---------- 1 file changed, 30 insertions(+), 10 deletions(-) diff --git a/src/sandbox/local.rs b/src/sandbox/local.rs index 6a5872e..38d53dd 100644 --- a/src/sandbox/local.rs +++ b/src/sandbox/local.rs @@ -1,9 +1,10 @@ -//! Local process sandbox provider — runs commands in an isolated temp directory. +//! Local process sandbox provider — runs commands in an isolated directory. //! -//! No external API key or Docker daemon required. Creates a temporary directory -//! under `$TMPDIR/zerobuild-sandbox-{uuid}/`, runs commands via -//! `tokio::process::Command` with a restricted environment, and constrains all -//! file operations to the sandbox directory (rejects `..` path components). +//! No external API key or Docker daemon required. Creates a sandbox directory +//! under `~/.zerobuild/workspace/sandbox/zerobuild-sandbox-{uuid}/` (or custom +//! path via `$ZEROBUILD_SANDBOX_PATH`), runs commands via `tokio::process::Command` +//! with a restricted environment, and constrains all file operations to the +//! sandbox directory (rejects `..` path components). //! //! **Isolation model:** //! - Filesystem: path-constrained to sandbox dir; `..` components are rejected. @@ -124,11 +125,30 @@ impl SandboxClient for LocalProcessSandboxClient { } *self.sandbox_id.lock() = None; - // Create new sandbox dir: $TMPDIR/zerobuild-sandbox-{uuid}/ - let tmp_base = std::env::temp_dir(); - let sandbox_dir = tmp_base.join(format!("zerobuild-sandbox-{}", Uuid::new_v4())); - std::fs::create_dir_all(&sandbox_dir) - .map_err(|e| anyhow::anyhow!("Failed to create sandbox dir: {e}"))?; + // Determine sandbox base directory + // Priority: $ZEROBUILD_SANDBOX_PATH > ~/.zerobuild/workspace/sandbox/ + let sandbox_base = if let Ok(custom_path) = std::env::var("ZEROBUILD_SANDBOX_PATH") { + PathBuf::from(custom_path) + } else { + // Default to ~/.zerobuild/workspace/sandbox/ + let home = std::env::var("HOME") + .or_else(|_| std::env::var("USERPROFILE")) + .map_err(|_| anyhow::anyhow!("Unable to determine home directory"))?; + PathBuf::from(home) + .join(".zerobuild") + .join("workspace") + .join("sandbox") + }; + + // Create new sandbox dir: {sandbox_base}/zerobuild-sandbox-{uuid}/ + let sandbox_dir = sandbox_base.join(format!("zerobuild-sandbox-{}", Uuid::new_v4())); + std::fs::create_dir_all(&sandbox_dir).map_err(|e| { + anyhow::anyhow!( + "Failed to create sandbox dir at {}: {}", + sandbox_dir.display(), + e + ) + })?; // Pre-create sub-directories used for npm cache redirection for sub in &[".npm-cache", ".npm-global", "tmp"] { From 9078c025890fd989d581d23de2ed59f30d05f2e9 Mon Sep 17 00:00:00 2001 From: louisdevzz Date: Thu, 5 Mar 2026 13:40:22 +0700 Subject: [PATCH 2/3] fix(sandbox): add validation for ZEROBUILD_SANDBOX_PATH and unit tests Address code review comments on PR #35: Validation improvements: - Check that ZEROBUILD_SANDBOX_PATH is non-empty - Reject relative paths (require absolute paths) - Reject paths containing parent directory traversal (..) - Return descriptive errors when validation fails Unit tests added: - sandbox_path_uses_custom_absolute_path_from_env - sandbox_path_falls_back_to_default_when_env_not_set - sandbox_path_rejects_empty_env_var - sandbox_path_rejects_relative_path - sandbox_path_rejects_parent_traversal All 21 sandbox tests pass, plus 3139 total tests pass. --- src/sandbox/local.rs | 163 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 162 insertions(+), 1 deletion(-) diff --git a/src/sandbox/local.rs b/src/sandbox/local.rs index 38d53dd..3da71fb 100644 --- a/src/sandbox/local.rs +++ b/src/sandbox/local.rs @@ -128,7 +128,26 @@ impl SandboxClient for LocalProcessSandboxClient { // Determine sandbox base directory // Priority: $ZEROBUILD_SANDBOX_PATH > ~/.zerobuild/workspace/sandbox/ let sandbox_base = if let Ok(custom_path) = std::env::var("ZEROBUILD_SANDBOX_PATH") { - PathBuf::from(custom_path) + // Validate custom path + if custom_path.is_empty() { + anyhow::bail!("ZEROBUILD_SANDBOX_PATH environment variable is set but empty"); + } + let path = PathBuf::from(&custom_path); + // Reject relative paths - require absolute paths + if !path.is_absolute() { + anyhow::bail!( + "ZEROBUILD_SANDBOX_PATH must be an absolute path, got: {}", + custom_path + ); + } + // Reject paths containing parent directory traversal (..) + if path.components().any(|c| matches!(c, Component::ParentDir)) { + anyhow::bail!( + "ZEROBUILD_SANDBOX_PATH contains parent directory traversal (..): {}", + custom_path + ); + } + path } else { // Default to ~/.zerobuild/workspace/sandbox/ let home = std::env::var("HOME") @@ -637,4 +656,146 @@ mod tests { let url = client.get_preview_url(3000).await.unwrap(); assert_eq!(url, "http://localhost:3000"); } + + // Tests for sandbox path selection logic (ZEROBUILD_SANDBOX_PATH validation) + + #[tokio::test] + async fn sandbox_path_uses_custom_absolute_path_from_env() { + // Create a temporary directory to use as custom sandbox base + let temp_dir = tempfile::tempdir().unwrap(); + let custom_path = temp_dir.path().to_path_buf(); + + // Set the environment variable + std::env::set_var("ZEROBUILD_SANDBOX_PATH", custom_path.as_os_str()); + + let client = LocalProcessSandboxClient::new(); + let id = client.create_sandbox(false, "", 30_000).await.unwrap(); + + // Verify sandbox was created under the custom path + let custom_path_str = custom_path + .to_str() + .expect("custom path should be valid UTF-8"); + assert!( + id.starts_with(custom_path_str), + "Sandbox should be under custom path: {}", + id + ); + assert!( + id.contains("zerobuild-sandbox-"), + "Sandbox dir should contain 'zerobuild-sandbox-': {}", + id + ); + assert!( + std::path::Path::new(&id).exists(), + "Sandbox directory should exist" + ); + + // Clean up + client.kill_sandbox().await.unwrap(); + std::env::remove_var("ZEROBUILD_SANDBOX_PATH"); + } + + #[tokio::test] + async fn sandbox_path_falls_back_to_default_when_env_not_set() { + // Ensure env var is not set + std::env::remove_var("ZEROBUILD_SANDBOX_PATH"); + + let client = LocalProcessSandboxClient::new(); + let id = client.create_sandbox(false, "", 30_000).await.unwrap(); + + // Verify sandbox was created under default location (~/.zerobuild/workspace/sandbox/) + let home = std::env::var("HOME") + .or_else(|_| std::env::var("USERPROFILE")) + .expect("HOME or USERPROFILE should be set"); + let expected_base = std::path::PathBuf::from(home) + .join(".zerobuild") + .join("workspace") + .join("sandbox"); + + let expected_base_str = expected_base + .to_str() + .expect("expected base should be valid UTF-8"); + assert!( + id.starts_with(expected_base_str), + "Sandbox should be under default path {} but got: {}", + expected_base.display(), + id + ); + assert!( + std::path::Path::new(&id).exists(), + "Sandbox directory should exist" + ); + + // Clean up + client.kill_sandbox().await.unwrap(); + } + + #[tokio::test] + async fn sandbox_path_rejects_empty_env_var() { + // Set empty environment variable + std::env::set_var("ZEROBUILD_SANDBOX_PATH", ""); + + let client = LocalProcessSandboxClient::new(); + let result = client.create_sandbox(false, "", 30_000).await; + + // Should fail with error about empty path + assert!( + result.is_err(), + "Should fail when ZEROBUILD_SANDBOX_PATH is empty" + ); + let err = result.unwrap_err().to_string(); + assert!( + err.contains("empty"), + "Error should mention empty variable: {}", + err + ); + + std::env::remove_var("ZEROBUILD_SANDBOX_PATH"); + } + + #[tokio::test] + async fn sandbox_path_rejects_relative_path() { + // Set a relative path + std::env::set_var("ZEROBUILD_SANDBOX_PATH", "relative/path/to/sandbox"); + + let client = LocalProcessSandboxClient::new(); + let result = client.create_sandbox(false, "", 30_000).await; + + // Should fail with error about relative path + assert!( + result.is_err(), + "Should fail when ZEROBUILD_SANDBOX_PATH is relative" + ); + let err = result.unwrap_err().to_string(); + assert!( + err.contains("absolute"), + "Error should mention absolute path requirement: {}", + err + ); + + std::env::remove_var("ZEROBUILD_SANDBOX_PATH"); + } + + #[tokio::test] + async fn sandbox_path_rejects_parent_traversal() { + // Set a path with parent directory traversal + std::env::set_var("ZEROBUILD_SANDBOX_PATH", "/tmp/../etc/sandbox"); + + let client = LocalProcessSandboxClient::new(); + let result = client.create_sandbox(false, "", 30_000).await; + + // Should fail with error about parent traversal + assert!( + result.is_err(), + "Should fail when ZEROBUILD_SANDBOX_PATH contains .." + ); + let err = result.unwrap_err().to_string(); + assert!( + err.contains("parent directory traversal") || err.contains(".."), + "Error should mention parent directory traversal: {}", + err + ); + + std::env::remove_var("ZEROBUILD_SANDBOX_PATH"); + } } From aaca62164144ff8822a8e8666d4488cbd7c14fc9 Mon Sep 17 00:00:00 2001 From: louisdevzz Date: Thu, 5 Mar 2026 13:59:39 +0700 Subject: [PATCH 3/3] fix(sandbox): serialize env var tests to prevent flakiness Address code review feedback on PR #35: Test improvements: - Add shared test mutex (ENV_MUTEX) to serialize tests that mutate ZEROBUILD_SANDBOX_PATH environment variable - Add EnvGuard struct with Drop impl to ensure env var is always restored to original value, even on test failure or panic - All 5 env var tests now use consistent pattern: 1. Lock mutex 2. Save original env state 3. Create EnvGuard for cleanup 4. Set test env var 5. Run test 6. Guard restores env on drop This prevents test flakiness from concurrent test execution and ensures env var state is always cleaned up properly. Also added OsString import for env var handling. --- src/sandbox/local.rs | 113 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 106 insertions(+), 7 deletions(-) diff --git a/src/sandbox/local.rs b/src/sandbox/local.rs index 3da71fb..893b5d1 100644 --- a/src/sandbox/local.rs +++ b/src/sandbox/local.rs @@ -22,7 +22,9 @@ use super::{CommandOutput, PackageManager, SandboxClient}; use anyhow::Context as _; use async_trait::async_trait; use parking_lot::Mutex; + use std::collections::HashMap; +use std::ffi::OsString; use std::path::{Component, Path, PathBuf}; use std::sync::Arc; use uuid::Uuid; @@ -514,6 +516,11 @@ fn collect_files_recursive(base: &Path, dir: &Path, out: &mut HashMap = std::sync::Mutex::new(()); #[test] fn safe_join_normal_path() { @@ -598,6 +605,9 @@ mod tests { #[tokio::test] async fn write_and_read_file() { + // Serialize with env var tests to avoid interference + let _guard = ENV_MUTEX.lock().unwrap(); + let client = LocalProcessSandboxClient::new(); client.create_sandbox(false, "", 30_000).await.unwrap(); client @@ -661,6 +671,24 @@ mod tests { #[tokio::test] async fn sandbox_path_uses_custom_absolute_path_from_env() { + // Serialize tests that mutate environment variables + let _guard = ENV_MUTEX.lock().unwrap(); + + // Save original env state + let original_env = std::env::var_os("ZEROBUILD_SANDBOX_PATH"); + + // Cleanup guard - ensures env is restored even on panic + struct EnvGuard(Option); + impl Drop for EnvGuard { + fn drop(&mut self) { + match &self.0 { + Some(val) => std::env::set_var("ZEROBUILD_SANDBOX_PATH", val), + None => std::env::remove_var("ZEROBUILD_SANDBOX_PATH"), + } + } + } + let _env_guard = EnvGuard(original_env); + // Create a temporary directory to use as custom sandbox base let temp_dir = tempfile::tempdir().unwrap(); let custom_path = temp_dir.path().to_path_buf(); @@ -690,14 +718,31 @@ mod tests { "Sandbox directory should exist" ); - // Clean up + // Clean up sandbox (env will be restored by guard) client.kill_sandbox().await.unwrap(); - std::env::remove_var("ZEROBUILD_SANDBOX_PATH"); } #[tokio::test] async fn sandbox_path_falls_back_to_default_when_env_not_set() { - // Ensure env var is not set + // Serialize tests that mutate environment variables + let _guard = ENV_MUTEX.lock().unwrap(); + + // Save original env state + let original_env = std::env::var_os("ZEROBUILD_SANDBOX_PATH"); + + // Cleanup guard - ensures env is restored even on panic + struct EnvGuard(Option); + impl Drop for EnvGuard { + fn drop(&mut self) { + match &self.0 { + Some(val) => std::env::set_var("ZEROBUILD_SANDBOX_PATH", val), + None => std::env::remove_var("ZEROBUILD_SANDBOX_PATH"), + } + } + } + let _env_guard = EnvGuard(original_env); + + // Ensure env var is not set for this test std::env::remove_var("ZEROBUILD_SANDBOX_PATH"); let client = LocalProcessSandboxClient::new(); @@ -726,12 +771,30 @@ mod tests { "Sandbox directory should exist" ); - // Clean up + // Clean up sandbox (env will be restored by guard) client.kill_sandbox().await.unwrap(); } #[tokio::test] async fn sandbox_path_rejects_empty_env_var() { + // Serialize tests that mutate environment variables + let _guard = ENV_MUTEX.lock().unwrap(); + + // Save original env state + let original_env = std::env::var_os("ZEROBUILD_SANDBOX_PATH"); + + // Cleanup guard - ensures env is restored even on panic + struct EnvGuard(Option); + impl Drop for EnvGuard { + fn drop(&mut self) { + match &self.0 { + Some(val) => std::env::set_var("ZEROBUILD_SANDBOX_PATH", val), + None => std::env::remove_var("ZEROBUILD_SANDBOX_PATH"), + } + } + } + let _env_guard = EnvGuard(original_env); + // Set empty environment variable std::env::set_var("ZEROBUILD_SANDBOX_PATH", ""); @@ -750,11 +813,29 @@ mod tests { err ); - std::env::remove_var("ZEROBUILD_SANDBOX_PATH"); + // Env will be restored by guard } #[tokio::test] async fn sandbox_path_rejects_relative_path() { + // Serialize tests that mutate environment variables + let _guard = ENV_MUTEX.lock().unwrap(); + + // Save original env state + let original_env = std::env::var_os("ZEROBUILD_SANDBOX_PATH"); + + // Cleanup guard - ensures env is restored even on panic + struct EnvGuard(Option); + impl Drop for EnvGuard { + fn drop(&mut self) { + match &self.0 { + Some(val) => std::env::set_var("ZEROBUILD_SANDBOX_PATH", val), + None => std::env::remove_var("ZEROBUILD_SANDBOX_PATH"), + } + } + } + let _env_guard = EnvGuard(original_env); + // Set a relative path std::env::set_var("ZEROBUILD_SANDBOX_PATH", "relative/path/to/sandbox"); @@ -773,11 +854,29 @@ mod tests { err ); - std::env::remove_var("ZEROBUILD_SANDBOX_PATH"); + // Env will be restored by guard } #[tokio::test] async fn sandbox_path_rejects_parent_traversal() { + // Serialize tests that mutate environment variables + let _guard = ENV_MUTEX.lock().unwrap(); + + // Save original env state + let original_env = std::env::var_os("ZEROBUILD_SANDBOX_PATH"); + + // Cleanup guard - ensures env is restored even on panic + struct EnvGuard(Option); + impl Drop for EnvGuard { + fn drop(&mut self) { + match &self.0 { + Some(val) => std::env::set_var("ZEROBUILD_SANDBOX_PATH", val), + None => std::env::remove_var("ZEROBUILD_SANDBOX_PATH"), + } + } + } + let _env_guard = EnvGuard(original_env); + // Set a path with parent directory traversal std::env::set_var("ZEROBUILD_SANDBOX_PATH", "/tmp/../etc/sandbox"); @@ -796,6 +895,6 @@ mod tests { err ); - std::env::remove_var("ZEROBUILD_SANDBOX_PATH"); + // Env will be restored by guard } }