From 1f9247092c39b391b3201a666df444ba949ce447 Mon Sep 17 00:00:00 2001 From: fettpl <38704082+fettpl@users.noreply.github.com> Date: Sat, 14 Feb 2026 23:51:57 +0100 Subject: [PATCH 1/5] fix: replace UUID v4 key generation with direct CSPRNG Use ChaCha20Poly1305::generate_key(&mut OsRng) to generate encryption keys directly from the OS CSPRNG, providing full 256-bit entropy without the fixed version/variant bits that UUID v4 introduces (6 fixed bits per 128-bit UUID = only 244 effective bits from two UUIDs). Closes #54 Co-Authored-By: Claude Opus 4.6 --- src/security/secrets.rs | 44 +++++++++++++++++++++++++++++++---------- src/tools/browser.rs | 1 + 2 files changed, 35 insertions(+), 10 deletions(-) diff --git a/src/security/secrets.rs b/src/security/secrets.rs index bafad38..cd93ede 100644 --- a/src/security/secrets.rs +++ b/src/security/secrets.rs @@ -217,16 +217,12 @@ fn xor_cipher(data: &[u8], key: &[u8]) -> Vec { .collect() } -/// Generate a random key using system entropy (UUID v4 + process ID + timestamp). +/// Generate a random 256-bit key using the OS CSPRNG. +/// +/// Uses `OsRng` (via `getrandom`) directly, providing full 256-bit entropy +/// without the fixed version/variant bits that UUID v4 introduces. fn generate_random_key() -> Vec { - // Use two UUIDs (32 random bytes) as our key material - let u1 = uuid::Uuid::new_v4(); - let u2 = uuid::Uuid::new_v4(); - let mut key = Vec::with_capacity(KEY_LEN); - key.extend_from_slice(u1.as_bytes()); - key.extend_from_slice(u2.as_bytes()); - key.truncate(KEY_LEN); - key + ChaCha20Poly1305::generate_key(&mut OsRng).to_vec() } /// Hex-encode bytes to a lowercase hex string. @@ -241,7 +237,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()) @@ -751,6 +747,34 @@ mod tests { assert_ne!(k1, k2, "Two random keys should differ"); } + #[test] + fn generate_random_key_has_no_uuid_fixed_bits() { + // UUID v4 has fixed bits at positions 6 (version = 0b0100xxxx) and + // 8 (variant = 0b10xxxxxx). A direct CSPRNG key should not consistently + // have these patterns across multiple samples. + let mut version_match = 0; + let mut variant_match = 0; + let samples = 100; + for _ in 0..samples { + let key = generate_random_key(); + // In UUID v4, byte 6 always has top nibble = 0x4 + if key[6] & 0xf0 == 0x40 { + version_match += 1; + } + // In UUID v4, byte 8 always has top 2 bits = 0b10 + if key[8] & 0xc0 == 0x80 { + variant_match += 1; + } + } + // With true randomness, each pattern should appear ~1/16 and ~1/4 of + // the time. UUID would hit 100/100 on both. Allow generous margin. + assert!( + version_match < 30, + "byte[6] matched UUID v4 version nibble {version_match}/100 times — \ + likely still using UUID-based key generation" + ); + } + #[cfg(unix)] #[test] fn key_file_has_restricted_permissions() { 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 a7ed2329d1c32dd5e83dcfa4ac8d1ff78b663d9b Mon Sep 17 00:00:00 2001 From: fettpl <38704082+fettpl@users.noreply.github.com> Date: Sun, 15 Feb 2026 00:25:20 +0100 Subject: [PATCH 2/5] fix: assert variant_match in CSPRNG key entropy test Add missing assertion for variant_match (byte[8] UUID v4 variant bits) which was computed but never checked. Co-Authored-By: Claude Opus 4.6 --- src/security/secrets.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/security/secrets.rs b/src/security/secrets.rs index cd93ede..f78d055 100644 --- a/src/security/secrets.rs +++ b/src/security/secrets.rs @@ -773,6 +773,11 @@ mod tests { "byte[6] matched UUID v4 version nibble {version_match}/100 times — \ likely still using UUID-based key generation" ); + assert!( + variant_match < 50, + "byte[8] matched UUID v4 variant bits {variant_match}/100 times — \ + likely still using UUID-based key generation" + ); } #[cfg(unix)] From 41ba251686661c349c52394fa885f407b488faa8 Mon Sep 17 00:00:00 2001 From: fettpl <38704082+fettpl@users.noreply.github.com> Date: Sun, 15 Feb 2026 01:24:24 +0100 Subject: [PATCH 3/5] fix: replace unstable is_multiple_of with modulo for Rust 1.83 compat The Docker image uses rust:1.83-slim where is_multiple_of is unstable. Co-Authored-By: Claude Opus 4.6 --- src/security/secrets.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/security/secrets.rs b/src/security/secrets.rs index f78d055..dbfc1e0 100644 --- a/src/security/secrets.rs +++ b/src/security/secrets.rs @@ -237,7 +237,7 @@ fn hex_encode(data: &[u8]) -> String { /// Hex-decode a hex string to bytes. fn hex_decode(hex: &str) -> Result> { - if !hex.len().is_multiple_of(2) { + if hex.len() % 2 != 0 { anyhow::bail!("Hex string has odd length"); } (0..hex.len()) From dc0d6b6ca9484587497c6660d5534b2c7a4dbeed Mon Sep 17 00:00:00 2001 From: fettpl <38704082+fettpl@users.noreply.github.com> Date: Sun, 15 Feb 2026 02:14:25 +0100 Subject: [PATCH 4/5] fix: replace unstable is_multiple_of with modulo for Rust 1.83 compat The Docker image uses rust:1.83-slim where is_multiple_of is unstable. Co-Authored-By: Claude Opus 4.6 --- src/security/secrets.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/security/secrets.rs b/src/security/secrets.rs index c3d012a..4d00384 100644 --- a/src/security/secrets.rs +++ b/src/security/secrets.rs @@ -238,7 +238,7 @@ fn hex_encode(data: &[u8]) -> String { /// Hex-decode a hex string to bytes. #[allow(clippy::manual_is_multiple_of)] fn hex_decode(hex: &str) -> Result> { - if !hex.len().is_multiple_of(2) { + if hex.len() % 2 != 0 { anyhow::bail!("Hex string has odd length"); } (0..hex.len()) From f87cbb28f2a138232755e3841df80411b2ce64ea Mon Sep 17 00:00:00 2001 From: fettpl <38704082+fettpl@users.noreply.github.com> Date: Sun, 15 Feb 2026 02:27:02 +0100 Subject: [PATCH 5/5] 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();