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/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 """ diff --git a/src/cli/args.rs b/src/cli/args.rs index b6ee904..db93b68 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 /// @@ -24,13 +15,9 @@ pub enum 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)] @@ -95,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 b3a99ec..80e688d 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(); @@ -355,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" @@ -411,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/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..70cd62d 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,42 +141,95 @@ 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 - warn and fallback to online + eprintln!( + "\x1b[33m[sx:warn]\x1b[0m Unknown profile '{}', falling back to 'online'", + name + ); + match BuiltinProfile::Online.load() { + Ok(profile) => Some(profile), + Err(e) => { + eprintln!( + "\x1b[31m[sx:error]\x1b[0m Failed to load fallback 'online' profile: {}", + e + ); + None } } - None }) .collect() } @@ -148,9 +266,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/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 31a381d..1f37e26 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); @@ -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) ); } 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"); +}