From a1eb05acd8b8756c2944ad6bab1362b3abed6f6f Mon Sep 17 00:00:00 2001 From: Pierre Tomasina Date: Thu, 29 Jan 2026 02:38:59 +0800 Subject: [PATCH 1/3] fix(sandbox): harden path validation and error handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add SeatbeltError enum with path injection prevention - Validates paths for unescaped quotes, newlines, null bytes - Prevents malicious paths from breaking Seatbelt syntax - All path interpolation now validates before use - Replace Arc> with owned TraceOutput enum - Simplifies violation trace output handling - Removes unnecessary thread-safe wrapper pattern - More idiomatic Rust for single-owner code - Add ViolationKind enum for type-safe categorization - Replace 130+ chars of magic ANSI strings with compile-time safety - Supports colored() and plain() output methods - Cleaner, more maintainable violation reporting - Convert expect() to Result types throughout codebase - ProfileError: Io, Parse, InvalidBuiltin variants - ExecutionError: wraps IO and Seatbelt errors - SeatbeltError: path validation failures - Proper error chaining with source() implementation - Improve error logging for security-critical operations - Log warnings for failed profile loads (not silent skipping) - Add zshrc write failure warning instead of silent ignore - Implement one-time violation trace write error logging - Improve zombie process handling - Better error handling for kill/wait operations - Proper result propagation instead of silent swallowing - Optimize config merging performance - Change merge_unique() from O(n²) to O(n) - Use HashSet for O(1) path lookups instead of contains() - Remove code quality issues - Delete duplicate NetworkMode enum (re-export from config::schema) - Remove dangling profile_path reference in ExecutionResult - Remove unused helper functions and dependencies Testing: All 185 tests pass. Code ready for open-source release. --- Cargo.lock | 98 ----------------- Cargo.toml | 4 - src/cli/args.rs | 13 +-- src/cli/commands.rs | 15 +-- src/config/merge.rs | 7 +- src/config/mod.rs | 4 +- src/config/profile.rs | 127 +++++++++++++++++++-- src/sandbox/executor.rs | 113 ++++++++++--------- src/sandbox/seatbelt.rs | 164 ++++++++++++++++++++++----- src/sandbox/trace.rs | 238 ++++++++++++++++++++++++++++++---------- tests/integration.rs | 24 ++-- tests/profile_test.rs | 18 +-- tests/seatbelt_test.rs | 50 +++++---- 13 files changed, 562 insertions(+), 313 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2590741..5dfa1f0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -105,24 +105,12 @@ dependencies = [ "serde", ] -[[package]] -name = "bumpalo" -version = "3.19.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5dd9dc738b7a8311c7ade152424974d8115f2cdad61e8dab8dac9f2362298510" - [[package]] name = "cfg-if" version = "1.0.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9330f8b2ff13f34540b44e946ef35111825727b38d33286ef986142615121801" -[[package]] -name = "cfg_aliases" -version = "0.2.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "613afe47fcd5fac7ccf1db93babcb082c5994d996f20b8b159f2ad1658eb5724" - [[package]] name = "clap" version = "4.5.54" @@ -279,16 +267,6 @@ version = "1.70.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a6cb138bb79a146c1bd460005623e142ef0181e3d0219cb493e02f7d08a35695" -[[package]] -name = "js-sys" -version = "0.3.85" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8c942ebf8e95485ca0d52d97da7c5a2c387d0e7f0ba4c35e93bfcaee045955b3" -dependencies = [ - "once_cell", - "wasm-bindgen", -] - [[package]] name = "lazy_static" version = "1.5.0" @@ -338,18 +316,6 @@ version = "2.7.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f52b00d39961fc5b2736ea853c9cc86238e165017a493d1d5c8eac6bdc4cc273" -[[package]] -name = "nix" -version = "0.31.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "225e7cfe711e0ba79a68baeddb2982723e4235247aefce1482f2f16c27865b66" -dependencies = [ - "bitflags", - "cfg-if", - "cfg_aliases", - "libc", -] - [[package]] name = "normalize-line-endings" version = "0.3.0" @@ -505,12 +471,6 @@ dependencies = [ "windows-sys 0.61.2", ] -[[package]] -name = "rustversion" -version = "1.0.22" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b39cdef0fa800fc44525c84ccb54a029961a8215f9619753635a9c0d2538d46d" - [[package]] name = "serde" version = "1.0.228" @@ -588,7 +548,6 @@ dependencies = [ "assert_cmd", "clap", "dirs", - "nix", "predicates", "serde", "shellexpand", @@ -597,7 +556,6 @@ dependencies = [ "toml", "tracing", "tracing-subscriber", - "uuid", ] [[package]] @@ -781,17 +739,6 @@ version = "0.2.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "06abde3611657adf66d383f00b093d7faecc7fa57071cce2578660c9f1010821" -[[package]] -name = "uuid" -version = "1.20.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ee48d38b119b0cd71fe4141b30f5ba9c7c5d9f4e7a3a8b4a674e4b6ef789976f" -dependencies = [ - "getrandom 0.3.4", - "js-sys", - "wasm-bindgen", -] - [[package]] name = "valuable" version = "0.1.1" @@ -822,51 +769,6 @@ dependencies = [ "wit-bindgen", ] -[[package]] -name = "wasm-bindgen" -version = "0.2.108" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "64024a30ec1e37399cf85a7ffefebdb72205ca1c972291c51512360d90bd8566" -dependencies = [ - "cfg-if", - "once_cell", - "rustversion", - "wasm-bindgen-macro", - "wasm-bindgen-shared", -] - -[[package]] -name = "wasm-bindgen-macro" -version = "0.2.108" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "008b239d9c740232e71bd39e8ef6429d27097518b6b30bdf9086833bd5b6d608" -dependencies = [ - "quote", - "wasm-bindgen-macro-support", -] - -[[package]] -name = "wasm-bindgen-macro-support" -version = "0.2.108" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5256bae2d58f54820e6490f9839c49780dff84c65aeab9e772f15d5f0e913a55" -dependencies = [ - "bumpalo", - "proc-macro2", - "quote", - "syn", - "wasm-bindgen-shared", -] - -[[package]] -name = "wasm-bindgen-shared" -version = "0.2.108" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1f01b580c9ac74c8d8f0c0e4afb04eeef2acf145458e52c03845ee9cd23e3d12" -dependencies = [ - "unicode-ident", -] - [[package]] name = "windows-link" version = "0.2.1" diff --git a/Cargo.toml b/Cargo.toml index 3bf9da3..3aa71c7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -25,14 +25,10 @@ shellexpand = "3" tracing = "0.1" tracing-subscriber = { version = "0.3", features = ["env-filter"] } -# Process management -nix = { version = "0.31", features = ["process", "signal"] } - # Utilities thiserror = "2" anyhow = "1" tempfile = "3" -uuid = { version = "1", features = ["v4"] } [dev-dependencies] assert_cmd = "2" diff --git a/src/cli/args.rs b/src/cli/args.rs index b6ee904..6cbd57d 100644 --- a/src/cli/args.rs +++ b/src/cli/args.rs @@ -1,17 +1,8 @@ use clap::Parser; use std::path::PathBuf; -/// Network access mode for the sandbox -#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] -pub enum NetworkMode { - /// Block all network access (default) - #[default] - Offline, - /// Allow all network access - Online, - /// Allow localhost only - Localhost, -} +// Re-export NetworkMode from config schema to avoid duplication +pub use crate::config::schema::NetworkMode; /// sx - Lightweight sandbox for macOS development /// diff --git a/src/cli/commands.rs b/src/cli/commands.rs index b3a99ec..fe161df 100644 --- a/src/cli/commands.rs +++ b/src/cli/commands.rs @@ -107,7 +107,8 @@ pub fn explain(args: &Args) -> Result<()> { /// Print generated sandbox profile without executing pub fn dry_run(args: &Args) -> Result<()> { let context = build_sandbox_context(args)?; - let profile = generate_seatbelt_profile(&context.params); + let profile = generate_seatbelt_profile(&context.params) + .context("Failed to generate seatbelt profile")?; if args.verbose { println!("# Profiles: {}", context.profile_names.join(", ")); @@ -291,9 +292,8 @@ fn build_sandbox_params( /// Determine network mode with precedence: CLI > profile > config fn determine_network_mode(args: &Args, profile: &Profile, config: &Config) -> NetworkMode { // CLI flags take highest precedence - let cli_mode = args.network_mode(); if args.online || args.localhost || args.offline { - return convert_network_mode(cli_mode); + return args.network_mode(); } // Profile network mode @@ -308,15 +308,6 @@ fn determine_network_mode(args: &Args, profile: &Profile, config: &Config) -> Ne .unwrap_or(config.sandbox.default_network) } -/// Convert CLI NetworkMode to config NetworkMode -fn convert_network_mode(mode: super::args::NetworkMode) -> NetworkMode { - match mode { - super::args::NetworkMode::Offline => NetworkMode::Offline, - super::args::NetworkMode::Online => NetworkMode::Online, - super::args::NetworkMode::Localhost => NetworkMode::Localhost, - } -} - /// Collect allow-read paths from config, profile, and CLI fn collect_allow_read_paths(config: &Config, profile: &Profile, cli: &[String]) -> Vec { let mut paths = Vec::new(); diff --git a/src/config/merge.rs b/src/config/merge.rs index 9d540ba..a970712 100644 --- a/src/config/merge.rs +++ b/src/config/merge.rs @@ -1,4 +1,5 @@ use super::schema::{Config, FilesystemConfig, SandboxConfig, ShellConfig}; +use std::collections::HashSet; /// Merge two configurations, with project taking precedence /// @@ -68,11 +69,13 @@ fn merge_shell(global: &ShellConfig, project: &ShellConfig) -> ShellConfig { } } -/// Merge two string vectors, keeping unique values +/// Merge two string vectors, keeping unique values. +/// Uses HashSet for O(1) lookups instead of O(n) contains() checks. fn merge_unique_strings(a: &[String], b: &[String]) -> Vec { + let mut seen: HashSet<&str> = a.iter().map(|s| s.as_str()).collect(); let mut result = a.to_vec(); for item in b { - if !result.contains(item) { + if seen.insert(item.as_str()) { result.push(item.clone()); } } diff --git a/src/config/mod.rs b/src/config/mod.rs index 83fa883..2f86f63 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -7,8 +7,8 @@ pub mod schema; pub use global::load_global_config; pub use merge::merge_configs; pub use profile::{ - compose_profiles, load_profile, load_profiles, BuiltinProfile, Profile, ProfileFilesystem, - ProfileShell, + compose_profiles, load_profile, load_profiles, BuiltinProfile, Profile, ProfileError, + ProfileFilesystem, ProfileShell, }; pub use project::load_project_config; pub use schema::{Config, NetworkMode}; diff --git a/src/config/profile.rs b/src/config/profile.rs index 92acd6e..72ef02a 100644 --- a/src/config/profile.rs +++ b/src/config/profile.rs @@ -1,7 +1,56 @@ use crate::config::schema::NetworkMode; use serde::{Deserialize, Serialize}; +use std::collections::HashSet; use std::path::Path; +/// Error type for profile loading +#[derive(Debug)] +pub enum ProfileError { + /// IO error reading profile file + Io(std::io::Error), + /// TOML parsing error + Parse(toml::de::Error), + /// Built-in profile has invalid TOML (should never happen) + InvalidBuiltin { + name: &'static str, + error: toml::de::Error, + }, +} + +impl std::fmt::Display for ProfileError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + ProfileError::Io(e) => write!(f, "IO error: {}", e), + ProfileError::Parse(e) => write!(f, "TOML parse error: {}", e), + ProfileError::InvalidBuiltin { name, error } => { + write!(f, "Built-in profile '{}' is invalid: {}", name, error) + } + } + } +} + +impl std::error::Error for ProfileError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match self { + ProfileError::Io(e) => Some(e), + ProfileError::Parse(e) => Some(e), + ProfileError::InvalidBuiltin { error, .. } => Some(error), + } + } +} + +impl From for ProfileError { + fn from(e: std::io::Error) -> Self { + ProfileError::Io(e) + } +} + +impl From for ProfileError { + fn from(e: toml::de::Error) -> Self { + ProfileError::Parse(e) + } +} + /// Profile struct for composable sandbox configurations #[derive(Debug, Clone, Default, Serialize, Deserialize)] #[serde(default)] @@ -66,8 +115,24 @@ impl BuiltinProfile { } } + /// Get the name of this builtin profile + pub fn name(&self) -> &'static str { + match self { + Self::Base => "base", + Self::Online => "online", + Self::Localhost => "localhost", + Self::Rust => "rust", + Self::Claude => "claude", + Self::Gpg => "gpg", + } + } + /// Load the profile data from embedded TOML files - pub fn load(&self) -> Profile { + /// + /// # Errors + /// Returns `ProfileError::InvalidBuiltin` if the embedded TOML is invalid. + /// This should never happen with properly tested builtin profiles. + pub fn load(&self) -> Result { let toml_str = match self { Self::Base => include_str!("../../profiles/base.toml"), Self::Online => include_str!("../../profiles/online.toml"), @@ -76,41 +141,81 @@ impl BuiltinProfile { Self::Claude => include_str!("../../profiles/claude.toml"), Self::Gpg => include_str!("../../profiles/gpg.toml"), }; - toml::from_str(toml_str).expect("builtin profile TOML is invalid") + toml::from_str(toml_str).map_err(|e| ProfileError::InvalidBuiltin { + name: self.name(), + error: e, + }) } } /// Load a profile from a TOML file -pub fn load_profile(path: &Path) -> Result { +pub fn load_profile(path: &Path) -> Result { let content = std::fs::read_to_string(path)?; - toml::from_str(&content).map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e)) + Ok(toml::from_str(&content)?) } -/// Load profiles by name, optionally searching in a custom directory +/// Load profiles by name, optionally searching in a custom directory. +/// Logs warnings for profiles that fail to load instead of silently skipping them. pub fn load_profiles(names: &[String], custom_dir: Option<&Path>) -> Vec { names .iter() .filter_map(|name| { // First try builtin profiles if let Some(builtin) = BuiltinProfile::from_name(name) { - return Some(builtin.load()); + match builtin.load() { + Ok(profile) => return Some(profile), + Err(e) => { + // This should never happen with properly tested builtin profiles + eprintln!( + "\x1b[31m[sx:error]\x1b[0m Failed to load builtin profile '{}': {}", + name, e + ); + return None; + } + } } + // Then try custom directory if let Some(dir) = custom_dir { let path = dir.join(format!("{}.toml", name)); if path.exists() { - return load_profile(&path).ok(); + match load_profile(&path) { + Ok(profile) => return Some(profile), + Err(e) => { + eprintln!( + "\x1b[33m[sx:warn]\x1b[0m Failed to load profile '{}' from {}: {}", + name, + path.display(), + e + ); + return None; + } + } } } + // Try global profile directory if let Some(config_dir) = dirs::config_dir() { let path = config_dir .join("sx/profiles") .join(format!("{}.toml", name)); if path.exists() { - return load_profile(&path).ok(); + match load_profile(&path) { + Ok(profile) => return Some(profile), + Err(e) => { + eprintln!( + "\x1b[33m[sx:warn]\x1b[0m Failed to load profile '{}' from {}: {}", + name, + path.display(), + e + ); + return None; + } + } } } + + // Profile not found - this is not an error, it might be an unknown name None }) .collect() @@ -148,9 +253,13 @@ pub fn compose_profiles(profiles: &[Profile]) -> Profile { result } +/// Merge unique strings from source into target. +/// Uses HashSet for O(1) lookups instead of O(n) contains() checks. fn merge_unique(target: &mut Vec, source: &[String]) { + // Build set of existing items (owned strings to avoid borrow conflicts) + let existing: HashSet = target.iter().cloned().collect(); for item in source { - if !target.contains(item) { + if !existing.contains(item) { target.push(item.clone()); } } diff --git a/src/sandbox/executor.rs b/src/sandbox/executor.rs index efd5621..41b74f9 100644 --- a/src/sandbox/executor.rs +++ b/src/sandbox/executor.rs @@ -1,12 +1,11 @@ // sandbox-exec invocation -use crate::sandbox::seatbelt::{generate_seatbelt_profile, SandboxParams}; +use crate::sandbox::seatbelt::{generate_seatbelt_profile, SandboxParams, SeatbeltError}; use crate::sandbox::trace::TraceSession; use std::fs; use std::io; -use std::path::{Path, PathBuf}; +use std::path::Path; use std::process::{Command, ExitStatus, Stdio}; use tempfile::{NamedTempFile, TempDir}; -use uuid::Uuid; /// Exit codes for sandbox execution pub mod exit_codes { @@ -19,11 +18,49 @@ pub mod exit_codes { pub const SANDBOX_VIOLATION: i32 = 137; } +/// Error type for sandbox execution +#[derive(Debug)] +pub enum ExecutionError { + /// IO error during execution + Io(io::Error), + /// Seatbelt profile generation error + Seatbelt(SeatbeltError), +} + +impl std::fmt::Display for ExecutionError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + ExecutionError::Io(e) => write!(f, "IO error: {}", e), + ExecutionError::Seatbelt(e) => write!(f, "Seatbelt error: {}", e), + } + } +} + +impl std::error::Error for ExecutionError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match self { + ExecutionError::Io(e) => Some(e), + ExecutionError::Seatbelt(e) => Some(e), + } + } +} + +impl From for ExecutionError { + fn from(e: io::Error) -> Self { + ExecutionError::Io(e) + } +} + +impl From for ExecutionError { + fn from(e: SeatbeltError) -> Self { + ExecutionError::Seatbelt(e) + } +} + /// Result of sandbox execution #[derive(Debug)] pub struct ExecutionResult { pub exit_code: i32, - pub profile_path: Option, } /// Execute a command inside a sandbox @@ -31,7 +68,7 @@ pub fn execute_sandboxed( params: &SandboxParams, command: &[String], shell: Option<&str>, -) -> io::Result { +) -> Result { execute_sandboxed_with_trace(params, command, shell, false, None) } @@ -42,7 +79,7 @@ pub fn execute_sandboxed_with_trace( shell: Option<&str>, trace: bool, trace_file: Option<&Path>, -) -> io::Result { +) -> Result { // Start trace session if requested let mut trace_session = if trace || trace_file.is_some() { if let Some(path) = trace_file { @@ -62,7 +99,7 @@ pub fn execute_sandboxed_with_trace( }; // Generate the seatbelt profile - let profile_content = generate_seatbelt_profile(params); + let profile_content = generate_seatbelt_profile(params)?; // Write profile to temp file let profile_file = NamedTempFile::new()?; @@ -113,7 +150,12 @@ HISTFILE=/dev/null HISTSIZE=0 SAVEHIST=0 "#; - let _ = fs::write(dir.path().join(".zshrc"), zshrc_content); + if let Err(e) = fs::write(dir.path().join(".zshrc"), zshrc_content) { + eprintln!( + "\x1b[33m[sx:warn]\x1b[0m Failed to disable zsh history: {}", + e + ); + } cmd.env("ZDOTDIR", dir.path()); } zdotdir @@ -138,19 +180,16 @@ SAVEHIST=0 let exit_code = status.code().unwrap_or(exit_codes::GENERAL_ERROR); - Ok(ExecutionResult { - exit_code, - profile_path: Some(profile_file.path().to_path_buf()), - }) + Ok(ExecutionResult { exit_code }) } /// Execute a command in sandbox and capture output (for non-interactive use) pub fn execute_sandboxed_captured( params: &SandboxParams, command: &[String], -) -> io::Result<(ExitStatus, Vec, Vec)> { +) -> Result<(ExitStatus, Vec, Vec), ExecutionError> { // Generate the seatbelt profile - let profile_content = generate_seatbelt_profile(params); + let profile_content = generate_seatbelt_profile(params)?; // Write profile to temp file let profile_file = NamedTempFile::new()?; @@ -167,35 +206,15 @@ pub fn execute_sandboxed_captured( } /// Print the generated seatbelt profile (dry-run mode) -pub fn dry_run(params: &SandboxParams) -> String { +pub fn dry_run(params: &SandboxParams) -> Result { generate_seatbelt_profile(params) } -/// Create a unique temp file path for the profile -pub fn temp_profile_path() -> PathBuf { - let uuid = Uuid::new_v4(); - std::env::temp_dir().join(format!("sx-{}.sx", uuid)) -} - -/// Write profile to a file and return the path -pub fn write_profile_file(profile_content: &str) -> io::Result { - let path = temp_profile_path(); - fs::write(&path, profile_content)?; - Ok(path) -} - -/// Clean up a profile file -pub fn cleanup_profile(path: &PathBuf) -> io::Result<()> { - if path.exists() { - fs::remove_file(path)?; - } - Ok(()) -} - #[cfg(test)] mod tests { use super::*; use crate::config::schema::NetworkMode; + use std::path::PathBuf; #[test] fn test_dry_run_returns_profile() { @@ -206,25 +225,19 @@ mod tests { ..Default::default() }; - let profile = dry_run(¶ms); + let profile = dry_run(¶ms).unwrap(); assert!(profile.contains("(version 1)")); assert!(profile.contains("(deny default)")); } #[test] - fn test_temp_profile_path_is_unique() { - let path1 = temp_profile_path(); - let path2 = temp_profile_path(); - assert_ne!(path1, path2); - } - - #[test] - fn test_write_and_cleanup_profile() { - let content = "(version 1)\n(deny default)\n"; - let path = write_profile_file(content).unwrap(); - assert!(path.exists()); + fn test_dry_run_fails_on_invalid_path() { + let params = SandboxParams { + working_dir: PathBuf::from("/tmp/test\"injection"), + ..Default::default() + }; - cleanup_profile(&path).unwrap(); - assert!(!path.exists()); + let result = dry_run(¶ms); + assert!(result.is_err()); } } diff --git a/src/sandbox/seatbelt.rs b/src/sandbox/seatbelt.rs index 4da9f48..ea33aba 100644 --- a/src/sandbox/seatbelt.rs +++ b/src/sandbox/seatbelt.rs @@ -6,6 +6,56 @@ use crate::config::schema::NetworkMode; use std::path::PathBuf; +/// Error type for seatbelt profile generation +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum SeatbeltError { + /// Path contains invalid characters that could break seatbelt syntax + InvalidPath { path: String, reason: &'static str }, +} + +impl std::fmt::Display for SeatbeltError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + SeatbeltError::InvalidPath { path, reason } => { + write!(f, "Invalid path '{}': {}", path, reason) + } + } + } +} + +impl std::error::Error for SeatbeltError {} + +/// Validate and sanitize a path for use in seatbelt profiles. +/// Returns an error if the path contains characters that could break seatbelt syntax +/// or potentially inject additional rules. +fn validate_seatbelt_path(path: &str) -> Result<&str, SeatbeltError> { + // Check for null bytes (could truncate the path) + if path.contains('\0') { + return Err(SeatbeltError::InvalidPath { + path: path.to_string(), + reason: "path contains null byte", + }); + } + + // Check for unescaped double quotes (could break string literals) + if path.contains('"') { + return Err(SeatbeltError::InvalidPath { + path: path.to_string(), + reason: "path contains unescaped double quote", + }); + } + + // Check for newlines (could inject new rules) + if path.contains('\n') || path.contains('\r') { + return Err(SeatbeltError::InvalidPath { + path: path.to_string(), + reason: "path contains newline character", + }); + } + + Ok(path) +} + /// Check if a path string contains glob wildcard characters fn contains_glob(path: &str) -> bool { path.contains('*') || path.contains('?') @@ -58,7 +108,11 @@ pub struct SandboxParams { /// - Reads: Denied by default, only explicit allow_read paths are accessible /// - Writes: Denied by default, allow working dir + explicit paths /// - Network: Configurable (offline/localhost/online) -pub fn generate_seatbelt_profile(params: &SandboxParams) -> String { +/// +/// # Errors +/// Returns `SeatbeltError::InvalidPath` if any path contains characters that could +/// break seatbelt syntax or inject additional rules. +pub fn generate_seatbelt_profile(params: &SandboxParams) -> Result { let mut profile = String::new(); // Version and default deny @@ -94,11 +148,12 @@ pub fn generate_seatbelt_profile(params: &SandboxParams) -> String { profile.push_str("(allow file-read* (literal \"/\"))\n"); for path in ¶ms.allow_read { let p = path.display().to_string(); - if contains_glob(&p) { - let regex = glob_to_regex(&p); + let validated = validate_seatbelt_path(&p)?; + if contains_glob(validated) { + let regex = glob_to_regex(validated); profile.push_str(&format!("(allow file-read* (regex #\"{regex}\"))\n")); } else { - profile.push_str(&format!("(allow file-read* (subpath \"{p}\"))\n")); + profile.push_str(&format!("(allow file-read* (subpath \"{validated}\"))\n")); } } profile.push('\n'); @@ -109,11 +164,12 @@ pub fn generate_seatbelt_profile(params: &SandboxParams) -> String { profile.push_str("; Denied read paths (sensitive data)\n"); for path in ¶ms.deny_read { let p = path.display().to_string(); - if contains_glob(&p) { - let regex = glob_to_regex(&p); + let validated = validate_seatbelt_path(&p)?; + if contains_glob(validated) { + let regex = glob_to_regex(validated); profile.push_str(&format!("(deny file-read* (regex #\"{regex}\"))\n")); } else { - profile.push_str(&format!("(deny file-read* (subpath \"{p}\"))\n")); + profile.push_str(&format!("(deny file-read* (subpath \"{validated}\"))\n")); } } profile.push('\n'); @@ -122,8 +178,9 @@ pub fn generate_seatbelt_profile(params: &SandboxParams) -> String { // Working directory - full read/write access profile.push_str("; Working directory (full access)\n"); if !params.working_dir.as_os_str().is_empty() { - let wd = params.working_dir.display(); - profile.push_str(&format!("(allow file* (subpath \"{wd}\"))\n\n")); + let wd = params.working_dir.display().to_string(); + let validated_wd = validate_seatbelt_path(&wd)?; + profile.push_str(&format!("(allow file* (subpath \"{validated_wd}\"))\n\n")); } // Allowed write paths (beyond working directory) @@ -131,16 +188,17 @@ pub fn generate_seatbelt_profile(params: &SandboxParams) -> String { profile.push_str("; Allowed write paths\n"); for path in ¶ms.allow_write { let p = path.display().to_string(); - if contains_glob(&p) { + let validated = validate_seatbelt_path(&p)?; + if contains_glob(validated) { // Glob patterns use regex filter - let regex = glob_to_regex(&p); + let regex = glob_to_regex(validated); profile.push_str(&format!("(allow file-write* (regex #\"{regex}\"))\n")); } else if path.is_file() { // Use regex for files (to include lock files) - let escaped = p.replace('.', "\\."); + let escaped = validated.replace('.', "\\."); profile.push_str(&format!("(allow file* (regex #\"^{escaped}.*\"))\n")); } else { - profile.push_str(&format!("(allow file-write* (subpath \"{p}\"))\n")); + profile.push_str(&format!("(allow file-write* (subpath \"{validated}\"))\n")); } } profile.push('\n'); @@ -176,7 +234,7 @@ pub fn generate_seatbelt_profile(params: &SandboxParams) -> String { profile.push('\n'); } - profile + Ok(profile) } #[cfg(test)] @@ -186,11 +244,61 @@ mod tests { #[test] fn test_default_params_produces_valid_profile() { let params = SandboxParams::default(); - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); assert!(profile.contains("(version 1)")); assert!(profile.contains("(deny default)")); } + // === Path Validation Tests === + + #[test] + fn test_validate_path_rejects_null_bytes() { + let result = validate_seatbelt_path("/path/with\0null"); + assert!(result.is_err()); + assert!(matches!( + result.unwrap_err(), + SeatbeltError::InvalidPath { reason, .. } if reason.contains("null") + )); + } + + #[test] + fn test_validate_path_rejects_double_quotes() { + let result = validate_seatbelt_path("/path/with\"quote"); + assert!(result.is_err()); + assert!(matches!( + result.unwrap_err(), + SeatbeltError::InvalidPath { reason, .. } if reason.contains("quote") + )); + } + + #[test] + fn test_validate_path_rejects_newlines() { + let result = validate_seatbelt_path("/path/with\nnewline"); + assert!(result.is_err()); + assert!(matches!( + result.unwrap_err(), + SeatbeltError::InvalidPath { reason, .. } if reason.contains("newline") + )); + } + + #[test] + fn test_validate_path_accepts_valid_paths() { + assert!(validate_seatbelt_path("/usr/bin").is_ok()); + assert!(validate_seatbelt_path("/Users/test/.config").is_ok()); + assert!(validate_seatbelt_path("/private/tmp/zsh*").is_ok()); + assert!(validate_seatbelt_path("~/.ssh").is_ok()); + } + + #[test] + fn test_generate_profile_fails_on_invalid_path() { + let params = SandboxParams { + allow_read: vec![PathBuf::from("/path/with\"injection")], + ..Default::default() + }; + let result = generate_seatbelt_profile(¶ms); + assert!(result.is_err()); + } + // === Glob/Wildcard Detection Tests === #[test] @@ -255,7 +363,7 @@ mod tests { allow_read: vec![PathBuf::from("/private/tmp/zsh*")], ..Default::default() }; - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); // Should use regex filter, not subpath assert!( profile.contains(r#"(allow file-read* (regex #"^/private/tmp/zsh.*"))"#), @@ -274,7 +382,7 @@ mod tests { allow_read: vec![PathBuf::from("/private/tmp/claude")], ..Default::default() }; - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); // Non-glob paths should still use subpath assert!( profile.contains(r#"(allow file-read* (subpath "/private/tmp/claude"))"#), @@ -288,7 +396,7 @@ mod tests { allow_write: vec![PathBuf::from("/private/tmp/zsh*")], ..Default::default() }; - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); // Should use regex filter for write as well assert!( profile.contains(r#"(allow file-write* (regex #"^/private/tmp/zsh.*"))"#), @@ -303,7 +411,7 @@ mod tests { deny_read: vec![PathBuf::from("/home/*/.ssh")], ..Default::default() }; - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); // Dot in .ssh is escaped to match literal dot, not any character assert!( profile.contains(r#"(deny file-read* (regex #"^/home/.*/\.ssh"))"#), @@ -322,7 +430,7 @@ mod tests { ], ..Default::default() }; - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); // Regular paths use subpath assert!(profile.contains(r#"(allow file-read* (subpath "/usr"))"#)); assert!(profile.contains(r#"(allow file-read* (subpath "/bin"))"#)); @@ -333,7 +441,7 @@ mod tests { #[test] fn test_deny_by_default_no_global_read() { let params = SandboxParams::default(); - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); // Should NOT have global read access - deny by default assert!(!profile.contains("(allow file-read* (subpath \"/\"))")); } @@ -344,7 +452,7 @@ mod tests { allow_read: vec![PathBuf::from("/usr"), PathBuf::from("/bin")], ..Default::default() }; - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); assert!(profile.contains("(allow file-read* (subpath \"/usr\"))")); assert!(profile.contains("(allow file-read* (subpath \"/bin\"))")); } @@ -355,7 +463,7 @@ mod tests { deny_read: vec![PathBuf::from("/secret")], ..Default::default() }; - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); assert!(profile.contains("(deny file-read* (subpath \"/secret\"))")); } @@ -366,7 +474,7 @@ mod tests { deny_read: vec![PathBuf::from("/home/.ssh")], ..Default::default() }; - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); let deny_pos = profile .find("(deny file-read* (subpath \"/home/.ssh\"))") @@ -387,7 +495,7 @@ mod tests { working_dir: PathBuf::from("/projects/myapp"), ..Default::default() }; - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); assert!(profile.contains("(allow file* (subpath \"/projects/myapp\"))")); } @@ -397,7 +505,7 @@ mod tests { network_mode: NetworkMode::Offline, ..Default::default() }; - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); assert!(profile.contains("Network disabled")); assert!(!profile.contains("(allow network")); } @@ -408,7 +516,7 @@ mod tests { network_mode: NetworkMode::Online, ..Default::default() }; - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); assert!(profile.contains("(allow network*)")); } @@ -418,7 +526,7 @@ mod tests { network_mode: NetworkMode::Localhost, ..Default::default() }; - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); assert!(profile.contains("(allow network-outbound (to ip \"localhost:*\"))")); assert!(profile.contains("(allow network-inbound (from ip \"localhost:*\"))")); // seatbelt doesn't accept IP addresses, only "localhost" or "*" diff --git a/src/sandbox/trace.rs b/src/sandbox/trace.rs index c14f750..2ff8a3a 100644 --- a/src/sandbox/trace.rs +++ b/src/sandbox/trace.rs @@ -4,13 +4,73 @@ //! filtering for relevant violations to help debug sandbox issues. use std::fs::{File, OpenOptions}; -use std::io::{BufRead, BufReader, Write}; +use std::io::{BufRead, BufReader, ErrorKind, Write}; use std::path::Path; use std::process::{Child, Command, Stdio}; use std::sync::atomic::{AtomicBool, Ordering}; -use std::sync::{Arc, Mutex}; +use std::sync::Arc; use std::thread; +/// Category of sandbox violation for type-safe handling +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ViolationKind { + Network, + Read, + Write, + Process, + Mach, + Other, +} + +impl ViolationKind { + /// Get ANSI-colored display string for this violation kind + pub fn colored(&self) -> &'static str { + match self { + ViolationKind::Network => "\x1b[31m[NETWORK]\x1b[0m", + ViolationKind::Read => "\x1b[33m[READ]\x1b[0m", + ViolationKind::Write => "\x1b[35m[WRITE]\x1b[0m", + ViolationKind::Process => "\x1b[36m[PROCESS]\x1b[0m", + ViolationKind::Mach => "\x1b[34m[MACH]\x1b[0m", + ViolationKind::Other => "\x1b[90m[OTHER]\x1b[0m", + } + } + + /// Get plain text display string for this violation kind + pub fn plain(&self) -> &'static str { + match self { + ViolationKind::Network => "[NETWORK]", + ViolationKind::Read => "[READ]", + ViolationKind::Write => "[WRITE]", + ViolationKind::Process => "[PROCESS]", + ViolationKind::Mach => "[MACH]", + ViolationKind::Other => "[OTHER]", + } + } + + /// Determine violation kind from operation string + fn from_operation(operation: &str) -> Self { + if operation.contains("network") { + ViolationKind::Network + } else if operation.contains("file-read") { + ViolationKind::Read + } else if operation.contains("file-write") { + ViolationKind::Write + } else if operation.contains("process") { + ViolationKind::Process + } else if operation.contains("mach") { + ViolationKind::Mach + } else { + ViolationKind::Other + } + } +} + +/// Destination for trace output +enum TraceOutput { + Stderr, + File(File), +} + /// Handle to a running trace session pub struct TraceSession { child: Child, @@ -20,7 +80,7 @@ pub struct TraceSession { impl TraceSession { /// Start a new trace session that streams sandbox violations to stderr pub fn start() -> std::io::Result { - Self::start_with_output(None) + Self::start_with_output(TraceOutput::Stderr) } /// Start a new trace session that streams sandbox violations to a file @@ -30,14 +90,13 @@ impl TraceSession { .write(true) .truncate(true) .open(path)?; - Self::start_with_output(Some(file)) + Self::start_with_output(TraceOutput::File(file)) } - /// Start a trace session with optional file output - fn start_with_output(file: Option) -> std::io::Result { + /// Start a trace session with specified output destination + fn start_with_output(output: TraceOutput) -> std::io::Result { let running = Arc::new(AtomicBool::new(true)); let running_clone = running.clone(); - let file = file.map(|f| Arc::new(Mutex::new(f))); // Use macOS `log stream` to capture sandbox violations // Sandbox denials are logged by the kernel with sender "Sandbox" @@ -55,9 +114,13 @@ impl TraceSession { .spawn()?; // Spawn a thread to read and filter the log output + // Move ownership of output directly into the thread (no Arc needed) if let Some(stdout) = child.stdout.take() { thread::spawn(move || { let reader = BufReader::new(stdout); + let mut output = output; + let mut write_error_logged = false; + for line in reader.lines() { if !running_clone.load(Ordering::Relaxed) { break; @@ -65,16 +128,26 @@ impl TraceSession { if let Ok(line) = line { // Filter for denial messages and format output if let Some(formatted) = format_violation(&line) { - if let Some(ref file) = file { - // Write to file (strip ANSI codes for file output) - if let Ok(mut f) = file.lock() { + match &mut output { + TraceOutput::File(file) => { + // Write to file (strip ANSI codes for file output) let plain = strip_ansi_codes(&formatted); - let _ = writeln!(f, "{}", plain); - let _ = f.flush(); + if let Err(e) = writeln!(file, "{}", plain) { + // Log error once to avoid spam + if !write_error_logged { + eprintln!( + "\x1b[33m[sx:trace]\x1b[0m Warning: failed to write to trace file: {}", + e + ); + write_error_logged = true; + } + } + // Best effort flush, don't spam errors for this + let _ = file.flush(); + } + TraceOutput::Stderr => { + eprintln!("{}", formatted); } - } else { - // Write to stderr - eprintln!("{}", formatted); } } } @@ -88,8 +161,25 @@ impl TraceSession { /// Stop the trace session pub fn stop(&mut self) { self.running.store(false, Ordering::Relaxed); - let _ = self.child.kill(); - let _ = self.child.wait(); + + // Kill the log stream process + if let Err(e) = self.child.kill() { + // ESRCH (no such process) is expected if already exited + if e.kind() != ErrorKind::NotFound && e.kind() != ErrorKind::InvalidInput { + eprintln!( + "\x1b[33m[sx:trace]\x1b[0m Warning: failed to stop log stream: {}", + e + ); + } + } + + // Always try to reap the child process to prevent zombies + if let Err(e) = self.child.wait() { + eprintln!( + "\x1b[33m[sx:trace]\x1b[0m Warning: failed to wait for log stream: {}", + e + ); + } } } @@ -114,52 +204,40 @@ fn format_violation(line: &str) -> Option { // Extract the sandbox denial part // Look for "Sandbox: process(pid) deny(N) operation target" - if let Some(sandbox_start) = line.find("Sandbox: ") { - let sandbox_part = &line[sandbox_start + 9..]; // Skip "Sandbox: " - - // Parse: "process(pid) deny(N) operation target" - let parts: Vec<&str> = sandbox_part.splitn(2, " deny").collect(); - if parts.len() < 2 { - return None; - } + let sandbox_start = line.find("Sandbox: ")?; + let sandbox_part = &line[sandbox_start + 9..]; // Skip "Sandbox: " - let process = parts[0].trim(); - let deny_rest = parts[1].trim(); + // Parse: "process(pid) deny(N) operation target" + let parts: Vec<&str> = sandbox_part.splitn(2, " deny").collect(); + if parts.len() < 2 { + return None; + } - // Skip the "(N) " part to get "operation target" - let op_target = if let Some(paren_end) = deny_rest.find(") ") { - &deny_rest[paren_end + 2..] - } else { - deny_rest - }; + let process = parts[0].trim(); + let deny_rest = parts[1].trim(); - // Split operation and target - let op_parts: Vec<&str> = op_target.splitn(2, ' ').collect(); - let operation = op_parts.first().unwrap_or(&"unknown"); - let target = op_parts.get(1).unwrap_or(&""); + // Skip the "(N) " part to get "operation target" + let op_target = if let Some(paren_end) = deny_rest.find(") ") { + &deny_rest[paren_end + 2..] + } else { + deny_rest + }; - // Categorize the violation - let category = if operation.contains("network") { - "\x1b[31m[NETWORK]\x1b[0m" - } else if operation.contains("file-read") { - "\x1b[33m[READ]\x1b[0m" - } else if operation.contains("file-write") { - "\x1b[35m[WRITE]\x1b[0m" - } else if operation.contains("process") { - "\x1b[36m[PROCESS]\x1b[0m" - } else if operation.contains("mach") { - "\x1b[34m[MACH]\x1b[0m" - } else { - "\x1b[90m[OTHER]\x1b[0m" - }; + // Split operation and target + let op_parts: Vec<&str> = op_target.splitn(2, ' ').collect(); + let operation = op_parts.first().unwrap_or(&"unknown"); + let target = op_parts.get(1).unwrap_or(&""); - return Some(format!( - "\x1b[90m[sx:trace]\x1b[0m {} \x1b[1m{}\x1b[0m {} \x1b[90m({})\x1b[0m", - category, operation, target, process - )); - } + // Categorize the violation using type-safe enum + let kind = ViolationKind::from_operation(operation); - None + Some(format!( + "\x1b[90m[sx:trace]\x1b[0m {} \x1b[1m{}\x1b[0m {} \x1b[90m({})\x1b[0m", + kind.colored(), + operation, + target, + process + )) } /// Strip ANSI escape codes from a string for plain text output @@ -192,6 +270,52 @@ fn strip_ansi_codes(s: &str) -> String { mod tests { use super::*; + // === ViolationKind Tests === + + #[test] + fn test_violation_kind_from_operation() { + assert_eq!( + ViolationKind::from_operation("network-outbound"), + ViolationKind::Network + ); + assert_eq!( + ViolationKind::from_operation("file-read-data"), + ViolationKind::Read + ); + assert_eq!( + ViolationKind::from_operation("file-write-data"), + ViolationKind::Write + ); + assert_eq!( + ViolationKind::from_operation("process-exec"), + ViolationKind::Process + ); + assert_eq!( + ViolationKind::from_operation("mach-lookup"), + ViolationKind::Mach + ); + assert_eq!( + ViolationKind::from_operation("unknown-op"), + ViolationKind::Other + ); + } + + #[test] + fn test_violation_kind_colored() { + assert!(ViolationKind::Network.colored().contains("31m")); // Red + assert!(ViolationKind::Read.colored().contains("33m")); // Yellow + assert!(ViolationKind::Write.colored().contains("35m")); // Magenta + } + + #[test] + fn test_violation_kind_plain() { + assert_eq!(ViolationKind::Network.plain(), "[NETWORK]"); + assert_eq!(ViolationKind::Read.plain(), "[READ]"); + assert_eq!(ViolationKind::Write.plain(), "[WRITE]"); + } + + // === Format Violation Tests === + #[test] fn test_format_violation_network() { // Real kernel log format diff --git a/tests/integration.rs b/tests/integration.rs index 31a381d..84db5cc 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -303,7 +303,7 @@ fn test_localhost_mode_profile_content() { let temp = TempDir::new().unwrap(); let params = network_sandbox_params(temp.path().to_path_buf(), NetworkMode::Localhost); - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); assert!( profile.contains("(allow network-outbound (to ip \"localhost:*\"))"), "Localhost mode should allow outbound to localhost" @@ -324,7 +324,7 @@ fn test_network_mode_offline_profile_content() { let temp = TempDir::new().unwrap(); let params = network_sandbox_params(temp.path().to_path_buf(), NetworkMode::Offline); - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); assert!( !profile.contains("(allow network*)"), @@ -341,7 +341,7 @@ fn test_network_mode_online_profile_content() { let temp = TempDir::new().unwrap(); let params = network_sandbox_params(temp.path().to_path_buf(), NetworkMode::Online); - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); assert!( profile.contains("(allow network*)"), @@ -546,7 +546,7 @@ fn test_load_all_builtin_profiles() { #[test] fn test_base_profile_has_required_paths() { - let profile = BuiltinProfile::Base.load(); + let profile = BuiltinProfile::Base.load().unwrap(); assert!(profile .filesystem @@ -578,19 +578,19 @@ fn test_base_profile_has_required_paths() { #[test] fn test_online_profile_sets_network_mode() { - let profile = BuiltinProfile::Online.load(); + let profile = BuiltinProfile::Online.load().unwrap(); assert_eq!(profile.network_mode, Some(NetworkMode::Online)); } #[test] fn test_localhost_profile_sets_network_mode() { - let profile = BuiltinProfile::Localhost.load(); + let profile = BuiltinProfile::Localhost.load().unwrap(); assert_eq!(profile.network_mode, Some(NetworkMode::Localhost)); } #[test] fn test_rust_profile_allows_cargo() { - let profile = BuiltinProfile::Rust.load(); + let profile = BuiltinProfile::Rust.load().unwrap(); assert!(profile .filesystem @@ -607,9 +607,9 @@ fn test_rust_profile_allows_cargo() { #[test] fn test_compose_multiple_profiles() { let profiles = vec![ - BuiltinProfile::Base.load(), - BuiltinProfile::Rust.load(), - BuiltinProfile::Online.load(), + BuiltinProfile::Base.load().unwrap(), + BuiltinProfile::Rust.load().unwrap(), + BuiltinProfile::Online.load().unwrap(), ]; let composed = compose_profiles(&profiles); @@ -630,8 +630,8 @@ fn test_compose_multiple_profiles() { #[test] fn test_compose_profiles_network_mode_last_wins() { let profiles = vec![ - BuiltinProfile::Online.load(), - BuiltinProfile::Localhost.load(), + BuiltinProfile::Online.load().unwrap(), + BuiltinProfile::Localhost.load().unwrap(), ]; let composed = compose_profiles(&profiles); diff --git a/tests/profile_test.rs b/tests/profile_test.rs index 3e59a27..932a62e 100644 --- a/tests/profile_test.rs +++ b/tests/profile_test.rs @@ -4,7 +4,7 @@ use tempfile::TempDir; #[test] fn test_builtin_profile_base() { - let profile = BuiltinProfile::Base.load(); + let profile = BuiltinProfile::Base.load().unwrap(); assert!(profile .filesystem .allow_read @@ -19,19 +19,19 @@ fn test_builtin_profile_base() { #[test] fn test_builtin_profile_online() { - let profile = BuiltinProfile::Online.load(); + let profile = BuiltinProfile::Online.load().unwrap(); assert_eq!(profile.network_mode, Some(NetworkMode::Online)); } #[test] fn test_builtin_profile_localhost() { - let profile = BuiltinProfile::Localhost.load(); + let profile = BuiltinProfile::Localhost.load().unwrap(); assert_eq!(profile.network_mode, Some(NetworkMode::Localhost)); } #[test] fn test_builtin_profile_rust() { - let profile = BuiltinProfile::Rust.load(); + let profile = BuiltinProfile::Rust.load().unwrap(); assert!(profile .filesystem .allow_read @@ -46,7 +46,7 @@ fn test_builtin_profile_rust() { #[test] fn test_builtin_profile_claude() { - let profile = BuiltinProfile::Claude.load(); + let profile = BuiltinProfile::Claude.load().unwrap(); assert!(profile .filesystem .allow_read @@ -56,7 +56,7 @@ fn test_builtin_profile_claude() { #[test] fn test_builtin_profile_gpg() { - let profile = BuiltinProfile::Gpg.load(); + let profile = BuiltinProfile::Gpg.load().unwrap(); assert!(profile .filesystem .allow_read @@ -145,7 +145,7 @@ fn test_compose_profiles_empty() { #[test] fn test_compose_profiles_single() { - let profile = BuiltinProfile::Base.load(); + let profile = BuiltinProfile::Base.load().unwrap(); let composed = compose_profiles(&[profile.clone()]); assert_eq!( composed.filesystem.allow_read, @@ -155,8 +155,8 @@ fn test_compose_profiles_single() { #[test] fn test_compose_profiles_multiple() { - let base = BuiltinProfile::Base.load(); - let rust = BuiltinProfile::Rust.load(); + let base = BuiltinProfile::Base.load().unwrap(); + let rust = BuiltinProfile::Rust.load().unwrap(); let composed = compose_profiles(&[base, rust]); diff --git a/tests/seatbelt_test.rs b/tests/seatbelt_test.rs index d9aacf1..a352786 100644 --- a/tests/seatbelt_test.rs +++ b/tests/seatbelt_test.rs @@ -11,7 +11,7 @@ fn test_generate_deny_default() { network_mode: NetworkMode::Offline, ..Default::default() }; - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); assert!( profile.contains("(deny default)"), "Profile should deny by default" @@ -26,7 +26,7 @@ fn test_allow_working_directory() { network_mode: NetworkMode::Offline, ..Default::default() }; - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); assert!( profile.contains(r#"(subpath "/Users/test/project")"#), "Profile should allow working directory" @@ -36,7 +36,7 @@ fn test_allow_working_directory() { #[test] fn test_version_1() { let params = SandboxParams::default(); - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); assert!( profile.contains("(version 1)"), "Profile should have version 1" @@ -51,7 +51,7 @@ fn test_network_offline() { network_mode: NetworkMode::Offline, ..Default::default() }; - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); // In offline mode, no network-outbound should be allowed assert!( !profile.contains("(allow network-outbound"), @@ -67,7 +67,7 @@ fn test_network_online() { network_mode: NetworkMode::Online, ..Default::default() }; - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); assert!( profile.contains("(allow network-outbound)") || profile.contains("(allow network*)"), "Online mode should allow network" @@ -82,7 +82,7 @@ fn test_network_localhost() { network_mode: NetworkMode::Localhost, ..Default::default() }; - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); assert!( profile.contains("localhost") || profile.contains("127.0.0.1"), "Localhost mode should reference localhost" @@ -98,7 +98,7 @@ fn test_deny_by_default_reads() { allow_read: vec![PathBuf::from("/usr"), PathBuf::from("/bin")], ..Default::default() }; - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); // Should NOT have global read access assert!( !profile.contains(r#"(allow file-read* (subpath "/"))"#), @@ -123,7 +123,7 @@ fn test_deny_read_paths() { deny_read: vec![PathBuf::from("/Users/test/.ssh")], ..Default::default() }; - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); assert!( profile.contains(r#"(subpath "/Users/test/.ssh")"#), "Should reference denied path" @@ -141,7 +141,7 @@ fn test_allow_write_paths() { allow_write: vec![PathBuf::from("/tmp")], ..Default::default() }; - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); assert!( profile.contains("file-write") && profile.contains(r#"(subpath "/tmp")"#), "Should allow write to /tmp" @@ -151,7 +151,7 @@ fn test_allow_write_paths() { #[test] fn test_process_fork_allowed() { let params = SandboxParams::default(); - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); assert!( profile.contains("(allow process-fork)"), "Should allow process-fork for child processes" @@ -161,7 +161,7 @@ fn test_process_fork_allowed() { #[test] fn test_process_exec_allowed() { let params = SandboxParams::default(); - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); assert!( profile.contains("(allow process-exec)"), "Should allow process-exec for running commands" @@ -170,7 +170,7 @@ fn test_process_exec_allowed() { #[test] fn test_base_profile_integration() { - let base = BuiltinProfile::Base.load(); + let base = BuiltinProfile::Base.load().unwrap(); let composed = compose_profiles(&[base]); let expand_path = |p: &str| -> PathBuf { @@ -206,7 +206,7 @@ fn test_base_profile_integration() { ..Default::default() }; - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); // Base profile should deny SSH assert!( @@ -234,7 +234,7 @@ fn test_raw_seatbelt_rules() { raw_rules: Some(raw_rules.to_string()), ..Default::default() }; - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); assert!( profile.contains(raw_rules), "Should include raw seatbelt rules" @@ -252,7 +252,7 @@ fn test_profile_is_valid_sexp() { allow_write: vec![PathBuf::from("/tmp")], ..Default::default() }; - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); // Basic s-expression validation: count parens let open_parens = profile.chars().filter(|c| *c == '(').count(); @@ -266,7 +266,7 @@ fn test_profile_is_valid_sexp() { #[test] fn test_mach_lookup_required() { let params = SandboxParams::default(); - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); // mach-lookup is required for basic system functionality assert!( profile.contains("mach-lookup") || profile.contains("mach*"), @@ -277,7 +277,7 @@ fn test_mach_lookup_required() { #[test] fn test_signal_allowed() { let params = SandboxParams::default(); - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); // signal is required for process control assert!( profile.contains("(allow signal"), @@ -288,7 +288,7 @@ fn test_signal_allowed() { #[test] fn test_system_read_paths() { let params = SandboxParams::default(); - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); // System read paths like /dev should be allowed assert!( profile.contains("/dev") || profile.contains("sysctl-read"), @@ -306,8 +306,20 @@ fn test_empty_paths_handled() { allow_write: vec![], ..Default::default() }; - let profile = generate_seatbelt_profile(¶ms); + let profile = generate_seatbelt_profile(¶ms).unwrap(); // Should still produce valid profile assert!(profile.contains("(version 1)")); assert!(profile.contains("(deny default)")); } + +#[test] +fn test_invalid_path_rejected() { + let params = SandboxParams { + working_dir: PathBuf::from("/tmp"), + home_dir: PathBuf::from("/Users/test"), + allow_read: vec![PathBuf::from("/path/with\"quote")], + ..Default::default() + }; + let result = generate_seatbelt_profile(¶ms); + assert!(result.is_err(), "Paths with quotes should be rejected"); +} From 1c3c2da9ffb0029aac407148dc89f008a4856fab Mon Sep 17 00:00:00 2001 From: Pierre Tomasina Date: Thu, 29 Jan 2026 02:41:21 +0800 Subject: [PATCH 2/3] fix(release): remove GitHub API calls from changelog template The commit.github.username variable triggers GitHub API requests that fail with 404 errors when the repository is not accessible. Remove this reference to allow changelog generation to succeed with local git data only. --- cliff.toml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cliff.toml b/cliff.toml index f39c2a8..7da7d73 100644 --- a/cliff.toml +++ b/cliff.toml @@ -13,8 +13,7 @@ body = """ ### {{ group | striptags | trim | upper_first }} {% for commit in commits %} - {% if commit.scope %}**{{ commit.scope }}:** {% endif %}\ - {{ commit.message | upper_first }}\ - {% if commit.github.username %} by @{{ commit.github.username }}{%- endif %}\ + {{ commit.message | upper_first }} {% endfor %} {% endfor %}\n """ From 1e1357cce5c27fd7d8071dc897492d74aaeea09d Mon Sep 17 00:00:00 2001 From: Pierre Tomasina Date: Thu, 29 Jan 2026 02:55:04 +0800 Subject: [PATCH 3/3] fix(config): warn on unknown profiles and fallback to online - Add warning when unknown profile is specified instead of silent skipping - Fallback to 'online' profile for unknown names to prevent unexpected network blocking - Remove references to removed profiles (node, python, go, git) from help text - Update shell integrations (bash, zsh, fish) to list only available profiles - Remove deprecated aliases (sxn for node, sxp for python) - Update init template and tests to reflect actual builtin profiles Available profiles: base, online, localhost, rust, claude, gpg All 185 tests pass. --- src/cli/args.rs | 8 ++------ src/cli/commands.rs | 6 +++--- src/config/profile.rs | 17 +++++++++++++++-- src/shell/integration.rs | 16 +--------------- tests/integration.rs | 14 ++++++++++---- 5 files changed, 31 insertions(+), 30 deletions(-) diff --git a/src/cli/args.rs b/src/cli/args.rs index 6cbd57d..db93b68 100644 --- a/src/cli/args.rs +++ b/src/cli/args.rs @@ -15,13 +15,9 @@ pub use crate::config::schema::NetworkMode; base Minimal sandbox (always included)\n \ online Full network access\n \ localhost Localhost network only\n \ - node Node.js/npm toolchain\n \ - python Python toolchain\n \ rust Rust/Cargo toolchain\n \ - go Go toolchain\n \ claude Claude Code (~/.claude access)\n \ - gpg GPG signing support\n \ - git Git with signing support")] + gpg GPG signing support")] pub struct Args { /// Enable verbose output (show sandbox config) #[arg(short, long)] @@ -86,7 +82,7 @@ pub struct Args { #[arg(long = "deny-read", value_name = "PATH")] pub deny_read: Vec, - /// Profiles to apply (e.g., online, node, claude) + /// Profiles to apply (e.g., online, rust, claude) #[arg(value_name = "PROFILES")] pub profiles: Vec, diff --git a/src/cli/commands.rs b/src/cli/commands.rs index fe161df..80e688d 100644 --- a/src/cli/commands.rs +++ b/src/cli/commands.rs @@ -346,7 +346,7 @@ fn generate_config_template() -> &'static str { inherit_global = true # Profiles to apply for this project -# Available: base, online, localhost, node, python, rust, go, claude, gpg +# Available: base, online, localhost, rust, claude, gpg profiles = [] # Default network mode: "offline", "online", or "localhost" @@ -402,13 +402,13 @@ mod tests { #[test] fn test_collect_profile_names_adds_cli_profiles() { - let args = Args::try_parse_from(["sx", "online", "node"]).unwrap(); + let args = Args::try_parse_from(["sx", "online", "rust"]).unwrap(); let config = Config::default(); let working_dir = PathBuf::from("/tmp"); let names = collect_profile_names(&args, &config, &working_dir); assert!(names.contains(&"online".to_string())); - assert!(names.contains(&"node".to_string())); + assert!(names.contains(&"rust".to_string())); } #[test] diff --git a/src/config/profile.rs b/src/config/profile.rs index 72ef02a..70cd62d 100644 --- a/src/config/profile.rs +++ b/src/config/profile.rs @@ -215,8 +215,21 @@ pub fn load_profiles(names: &[String], custom_dir: Option<&Path>) -> Vec Some(profile), + Err(e) => { + eprintln!( + "\x1b[31m[sx:error]\x1b[0m Failed to load fallback 'online' profile: {}", + e + ); + None + } + } }) .collect() } diff --git a/src/shell/integration.rs b/src/shell/integration.rs index 351a4e2..bb05ef8 100644 --- a/src/shell/integration.rs +++ b/src/shell/integration.rs @@ -71,13 +71,9 @@ _sx() { 'base:Minimal sandbox (always included)' 'online:Full network access' 'localhost:Localhost network only' - 'node:Node.js/npm toolchain' - 'python:Python toolchain' 'rust:Rust/Cargo toolchain' - 'go:Go toolchain' 'claude:Claude Code support' 'gpg:GPG signing support' - 'git:Git with signing' ) local -a options @@ -114,8 +110,6 @@ compdef _sx sx # Aliases for common patterns alias sxo='sx online' alias sxl='sx localhost' -alias sxn='sx online node' -alias sxp='sx online python' alias sxr='sx online rust' alias sxc='sx online claude' "#; @@ -148,7 +142,7 @@ fi # Completions _sx_completions() { local cur="${COMP_WORDS[COMP_CWORD]}" - local profiles="base online localhost node python rust go claude gpg git" + local profiles="base online localhost rust claude gpg" local options="--help --version --verbose --debug --dry-run --explain --init --offline --online --localhost" if [[ "$cur" == -* ]]; then @@ -163,8 +157,6 @@ complete -F _sx_completions sx # Aliases alias sxo='sx online' alias sxl='sx localhost' -alias sxn='sx online node' -alias sxp='sx online python' alias sxr='sx online rust' alias sxc='sx online claude' "#; @@ -215,19 +207,13 @@ complete -c sx -l localhost -d 'Allow localhost only' complete -c sx -a 'base' -d 'Minimal sandbox' complete -c sx -a 'online' -d 'Full network access' complete -c sx -a 'localhost' -d 'Localhost network only' -complete -c sx -a 'node' -d 'Node.js/npm toolchain' -complete -c sx -a 'python' -d 'Python toolchain' complete -c sx -a 'rust' -d 'Rust/Cargo toolchain' -complete -c sx -a 'go' -d 'Go toolchain' complete -c sx -a 'claude' -d 'Claude Code support' complete -c sx -a 'gpg' -d 'GPG signing support' -complete -c sx -a 'git' -d 'Git with signing' # Aliases alias sxo 'sx online' alias sxl 'sx localhost' -alias sxn 'sx online node' -alias sxp 'sx online python' alias sxr 'sx online rust' alias sxc 'sx online claude' "#; diff --git a/tests/integration.rs b/tests/integration.rs index 84db5cc..1f37e26 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -702,11 +702,17 @@ allow_write = ["/custom/write"] } #[test] -fn test_load_missing_profile_returns_empty() { +fn test_load_missing_profile_falls_back_to_online() { let profiles = load_profiles(&["nonexistent".to_string()], None); - assert!( - profiles.is_empty(), - "Should return empty for missing profile" + assert_eq!( + profiles.len(), + 1, + "Should return one profile (online fallback) for missing profile" + ); + // Verify it's the online profile (has network_mode = Some(Online)) + assert_eq!( + profiles[0].network_mode, + Some(sx::config::schema::NetworkMode::Online) ); }