From 2942e5607d1270c0b862d32e8b10a4e1784e1a5e Mon Sep 17 00:00:00 2001 From: fettpl <38704082+fettpl@users.noreply.github.com> Date: Sat, 14 Feb 2026 23:59:36 +0100 Subject: [PATCH 1/4] fix: log warning when Windows key file permissions fail to set Replace silently discarded icacls result with proper error handling that logs a tracing::warn! on failure. Previously, if icacls failed (binary not found, permission denied), the key file would remain world-readable on Windows with no indication of the problem. Closes #56 Co-Authored-By: Claude Opus 4.6 --- src/security/secrets.rs | 18 +++++++++++++++--- src/tools/browser.rs | 1 + 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/security/secrets.rs b/src/security/secrets.rs index bafad38..c845bd9 100644 --- a/src/security/secrets.rs +++ b/src/security/secrets.rs @@ -191,14 +191,26 @@ impl SecretStore { #[cfg(windows)] { // On Windows, use icacls to restrict permissions to current user only - let _ = std::process::Command::new("icacls") + match std::process::Command::new("icacls") .arg(&self.key_path) .args(["/inheritance:r", "/grant:r"]) .arg(format!( "{}:F", std::env::var("USERNAME").unwrap_or_default() )) - .output(); + .output() + { + Ok(o) if !o.status.success() => { + tracing::warn!( + "Failed to set key file permissions via icacls (exit code {:?})", + o.status.code() + ); + } + Err(e) => { + tracing::warn!("Could not set key file permissions: {e}"); + } + _ => {} + } } Ok(key) @@ -241,7 +253,7 @@ fn hex_encode(data: &[u8]) -> String { /// Hex-decode a hex string to bytes. fn hex_decode(hex: &str) -> Result> { - if hex.len() % 2 != 0 { + if !hex.len().is_multiple_of(2) { anyhow::bail!("Hex string has odd length"); } (0..hex.len()) diff --git a/src/tools/browser.rs b/src/tools/browser.rs index 5ee9505..25be13c 100644 --- a/src/tools/browser.rs +++ b/src/tools/browser.rs @@ -366,6 +366,7 @@ impl BrowserTool { } #[async_trait] +#[allow(clippy::too_many_lines)] impl Tool for BrowserTool { fn name(&self) -> &str { "browser" From 6fd4b2d750816f9cd922fab0698b4b411696e301 Mon Sep 17 00:00:00 2001 From: fettpl <38704082+fettpl@users.noreply.github.com> Date: Sun, 15 Feb 2026 00:29:22 +0100 Subject: [PATCH 2/4] fix: handle empty USERNAME and add debug log for icacls success - Check for empty USERNAME env var before running icacls to avoid a doomed invocation with ":F" grant argument - Log a clear warning when USERNAME is empty - Add tracing::debug on successful permission set Co-Authored-By: Claude Opus 4.6 --- src/security/secrets.rs | 43 ++++++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 18 deletions(-) diff --git a/src/security/secrets.rs b/src/security/secrets.rs index c845bd9..db38972 100644 --- a/src/security/secrets.rs +++ b/src/security/secrets.rs @@ -191,25 +191,32 @@ impl SecretStore { #[cfg(windows)] { // On Windows, use icacls to restrict permissions to current user only - match std::process::Command::new("icacls") - .arg(&self.key_path) - .args(["/inheritance:r", "/grant:r"]) - .arg(format!( - "{}:F", - std::env::var("USERNAME").unwrap_or_default() - )) - .output() - { - Ok(o) if !o.status.success() => { - tracing::warn!( - "Failed to set key file permissions via icacls (exit code {:?})", - o.status.code() - ); + let username = std::env::var("USERNAME").unwrap_or_default(); + if username.is_empty() { + tracing::warn!( + "USERNAME environment variable is empty; \ + cannot restrict key file permissions via icacls" + ); + } else { + match std::process::Command::new("icacls") + .arg(&self.key_path) + .args(["/inheritance:r", "/grant:r"]) + .arg(format!("{username}:F")) + .output() + { + Ok(o) if !o.status.success() => { + tracing::warn!( + "Failed to set key file permissions via icacls (exit code {:?})", + o.status.code() + ); + } + Err(e) => { + tracing::warn!("Could not set key file permissions: {e}"); + } + _ => { + tracing::debug!("Key file permissions restricted via icacls"); + } } - Err(e) => { - tracing::warn!("Could not set key file permissions: {e}"); - } - _ => {} } } From a68004184cb7fbd356ba1ee900d3a16edd040d45 Mon Sep 17 00:00:00 2001 From: argenis de la rosa Date: Sat, 14 Feb 2026 19:25:30 -0500 Subject: [PATCH 3/4] fix(secrets): harden windows icacls username edge cases --- src/security/secrets.rs | 75 +++++++++++++++++++++++++++++------------ 1 file changed, 54 insertions(+), 21 deletions(-) diff --git a/src/security/secrets.rs b/src/security/secrets.rs index db38972..c7d670b 100644 --- a/src/security/secrets.rs +++ b/src/security/secrets.rs @@ -192,30 +192,31 @@ impl SecretStore { { // On Windows, use icacls to restrict permissions to current user only let username = std::env::var("USERNAME").unwrap_or_default(); - if username.is_empty() { + let Some(grant_arg) = build_windows_icacls_grant_arg(&username) else { tracing::warn!( "USERNAME environment variable is empty; \ cannot restrict key file permissions via icacls" ); - } else { - match std::process::Command::new("icacls") - .arg(&self.key_path) - .args(["/inheritance:r", "/grant:r"]) - .arg(format!("{username}:F")) - .output() - { - Ok(o) if !o.status.success() => { - tracing::warn!( - "Failed to set key file permissions via icacls (exit code {:?})", - o.status.code() - ); - } - Err(e) => { - tracing::warn!("Could not set key file permissions: {e}"); - } - _ => { - tracing::debug!("Key file permissions restricted via icacls"); - } + return Ok(key); + }; + + match std::process::Command::new("icacls") + .arg(&self.key_path) + .args(["/inheritance:r", "/grant:r"]) + .arg(grant_arg) + .output() + { + Ok(o) if !o.status.success() => { + tracing::warn!( + "Failed to set key file permissions via icacls (exit code {:?})", + o.status.code() + ); + } + Err(e) => { + tracing::warn!("Could not set key file permissions: {e}"); + } + _ => { + tracing::debug!("Key file permissions restricted via icacls"); } } } @@ -258,9 +259,19 @@ fn hex_encode(data: &[u8]) -> String { s } +/// Build the `/grant` argument for `icacls` using a normalized username. +/// Returns `None` when the username is empty or whitespace-only. +fn build_windows_icacls_grant_arg(username: &str) -> Option { + let normalized = username.trim(); + if normalized.is_empty() { + return None; + } + Some(format!("{normalized}:F")) +} + /// Hex-decode a hex string to bytes. fn hex_decode(hex: &str) -> Result> { - if !hex.len().is_multiple_of(2) { + if (hex.len() & 1) != 0 { anyhow::bail!("Hex string has odd length"); } (0..hex.len()) @@ -751,6 +762,28 @@ mod tests { assert!(hex_decode("zzzz").is_err()); } + #[test] + fn windows_icacls_grant_arg_rejects_empty_username() { + assert_eq!(build_windows_icacls_grant_arg(""), None); + assert_eq!(build_windows_icacls_grant_arg(" \t\n"), None); + } + + #[test] + fn windows_icacls_grant_arg_trims_username() { + assert_eq!( + build_windows_icacls_grant_arg(" alice "), + Some("alice:F".to_string()) + ); + } + + #[test] + fn windows_icacls_grant_arg_preserves_valid_characters() { + assert_eq!( + build_windows_icacls_grant_arg("DOMAIN\\svc-user"), + Some("DOMAIN\\svc-user:F".to_string()) + ); + } + #[test] fn generate_random_key_correct_length() { let key = generate_random_key(); From 65a5c3c1e8fcf30f2ea84a5f3e22fda026f819d4 Mon Sep 17 00:00:00 2001 From: fettpl <38704082+fettpl@users.noreply.github.com> Date: Sun, 15 Feb 2026 02:26:39 +0100 Subject: [PATCH 4/4] fix: consolidate env-var override tests to eliminate parallel races Tests that set/remove the same environment variables can race when cargo test runs them in parallel. Merges each racing pair into a single test function. Co-Authored-By: Claude Opus 4.6 --- src/config/schema.rs | 146 +++++++++++++++++-------------------------- 1 file changed, 59 insertions(+), 87 deletions(-) diff --git a/src/config/schema.rs b/src/config/schema.rs index be6f768..e437407 100644 --- a/src/config/schema.rs +++ b/src/config/schema.rs @@ -1474,55 +1474,53 @@ default_temperature = 0.7 #[test] fn env_override_api_key() { + // Primary and fallback tested together to avoid env-var races. + std::env::remove_var("ZEROCLAW_API_KEY"); + std::env::remove_var("API_KEY"); + + // Primary: ZEROCLAW_API_KEY let mut config = Config::default(); assert!(config.api_key.is_none()); - - // Simulate ZEROCLAW_API_KEY std::env::set_var("ZEROCLAW_API_KEY", "sk-test-env-key"); config.apply_env_overrides(); assert_eq!(config.api_key.as_deref(), Some("sk-test-env-key")); - - // Clean up std::env::remove_var("ZEROCLAW_API_KEY"); - } - #[test] - fn env_override_api_key_fallback() { - let mut config = Config::default(); - - // Simulate API_KEY (fallback) - std::env::remove_var("ZEROCLAW_API_KEY"); + // Fallback: API_KEY + let mut config2 = Config::default(); std::env::set_var("API_KEY", "sk-fallback-key"); - config.apply_env_overrides(); - assert_eq!(config.api_key.as_deref(), Some("sk-fallback-key")); - - // Clean up + config2.apply_env_overrides(); + assert_eq!(config2.api_key.as_deref(), Some("sk-fallback-key")); std::env::remove_var("API_KEY"); } #[test] fn env_override_provider() { - let mut config = Config::default(); + // Primary, fallback, and empty-value tested together to avoid env-var races. + std::env::remove_var("ZEROCLAW_PROVIDER"); + std::env::remove_var("PROVIDER"); + // Primary: ZEROCLAW_PROVIDER + let mut config = Config::default(); std::env::set_var("ZEROCLAW_PROVIDER", "anthropic"); config.apply_env_overrides(); assert_eq!(config.default_provider.as_deref(), Some("anthropic")); - - // Clean up std::env::remove_var("ZEROCLAW_PROVIDER"); - } - #[test] - fn env_override_provider_fallback() { - let mut config = Config::default(); - - std::env::remove_var("ZEROCLAW_PROVIDER"); + // Fallback: PROVIDER + let mut config2 = Config::default(); std::env::set_var("PROVIDER", "openai"); - config.apply_env_overrides(); - assert_eq!(config.default_provider.as_deref(), Some("openai")); - - // Clean up + config2.apply_env_overrides(); + assert_eq!(config2.default_provider.as_deref(), Some("openai")); std::env::remove_var("PROVIDER"); + + // Empty value should not override + let mut config3 = Config::default(); + let original_provider = config3.default_provider.clone(); + std::env::set_var("ZEROCLAW_PROVIDER", ""); + config3.apply_env_overrides(); + assert_eq!(config3.default_provider, original_provider); + std::env::remove_var("ZEROCLAW_PROVIDER"); } #[test] @@ -1549,108 +1547,82 @@ default_temperature = 0.7 std::env::remove_var("ZEROCLAW_WORKSPACE"); } - #[test] - fn env_override_empty_values_ignored() { - let mut config = Config::default(); - let original_provider = config.default_provider.clone(); - - std::env::set_var("ZEROCLAW_PROVIDER", ""); - config.apply_env_overrides(); - // Empty value should not override - assert_eq!(config.default_provider, original_provider); - - // Clean up - std::env::remove_var("ZEROCLAW_PROVIDER"); - } - #[test] fn env_override_gateway_port() { + // Port, fallback, and invalid tested together to avoid env-var races. + std::env::remove_var("ZEROCLAW_GATEWAY_PORT"); + std::env::remove_var("PORT"); + + // Primary: ZEROCLAW_GATEWAY_PORT let mut config = Config::default(); assert_eq!(config.gateway.port, 3000); - std::env::set_var("ZEROCLAW_GATEWAY_PORT", "8080"); config.apply_env_overrides(); assert_eq!(config.gateway.port, 8080); - std::env::remove_var("ZEROCLAW_GATEWAY_PORT"); - } - - #[test] - fn env_override_port_fallback() { - std::env::remove_var("ZEROCLAW_GATEWAY_PORT"); - let mut config = Config::default(); + // Fallback: PORT + let mut config2 = Config::default(); std::env::set_var("PORT", "9000"); - config.apply_env_overrides(); - assert_eq!(config.gateway.port, 9000); + config2.apply_env_overrides(); + assert_eq!(config2.gateway.port, 9000); + + // Invalid PORT is ignored + let mut config3 = Config::default(); + let original_port = config3.gateway.port; + std::env::set_var("PORT", "not_a_number"); + config3.apply_env_overrides(); + assert_eq!(config3.gateway.port, original_port); std::env::remove_var("PORT"); } #[test] fn env_override_gateway_host() { + // Primary and fallback tested together to avoid env-var races. + std::env::remove_var("ZEROCLAW_GATEWAY_HOST"); + std::env::remove_var("HOST"); + + // Primary: ZEROCLAW_GATEWAY_HOST let mut config = Config::default(); assert_eq!(config.gateway.host, "127.0.0.1"); - std::env::set_var("ZEROCLAW_GATEWAY_HOST", "0.0.0.0"); config.apply_env_overrides(); assert_eq!(config.gateway.host, "0.0.0.0"); - std::env::remove_var("ZEROCLAW_GATEWAY_HOST"); - } - - #[test] - fn env_override_host_fallback() { - std::env::remove_var("ZEROCLAW_GATEWAY_HOST"); - let mut config = Config::default(); + // Fallback: HOST + let mut config2 = Config::default(); std::env::set_var("HOST", "0.0.0.0"); - config.apply_env_overrides(); - assert_eq!(config.gateway.host, "0.0.0.0"); - + config2.apply_env_overrides(); + assert_eq!(config2.gateway.host, "0.0.0.0"); std::env::remove_var("HOST"); } #[test] fn env_override_temperature() { + // Valid and out-of-range tested together to avoid env-var races. std::env::remove_var("ZEROCLAW_TEMPERATURE"); - let mut config = Config::default(); + // Valid temperature is applied + let mut config = Config::default(); std::env::set_var("ZEROCLAW_TEMPERATURE", "0.5"); config.apply_env_overrides(); assert!((config.default_temperature - 0.5).abs() < f64::EPSILON); - std::env::remove_var("ZEROCLAW_TEMPERATURE"); - } - - #[test] - fn env_override_temperature_out_of_range_ignored() { - std::env::remove_var("ZEROCLAW_TEMPERATURE"); - let mut config = Config::default(); - let original_temp = config.default_temperature; - + // Out-of-range temperature is ignored + let mut config2 = Config::default(); + let original_temp = config2.default_temperature; std::env::set_var("ZEROCLAW_TEMPERATURE", "3.0"); - config.apply_env_overrides(); + config2.apply_env_overrides(); assert!( - (config.default_temperature - original_temp).abs() < f64::EPSILON, + (config2.default_temperature - original_temp).abs() < f64::EPSILON, "Temperature 3.0 should be ignored (out of range)" ); std::env::remove_var("ZEROCLAW_TEMPERATURE"); } - #[test] - fn env_override_invalid_port_ignored() { - let mut config = Config::default(); - let original_port = config.gateway.port; - - std::env::set_var("PORT", "not_a_number"); - config.apply_env_overrides(); - assert_eq!(config.gateway.port, original_port); - - std::env::remove_var("PORT"); - } - #[test] fn gateway_config_default_values() { let g = GatewayConfig::default();