From 6776373e8ef8db23eae68f779b6b6bac4b1888bf Mon Sep 17 00:00:00 2001 From: fettpl <38704082+fettpl@users.noreply.github.com> Date: Sun, 15 Feb 2026 00:01:23 +0100 Subject: [PATCH 1/5] fix: constant_time_eq no longer leaks secret length via early return Remove the early return on length mismatch that leaked length information via timing. Now iterates over max(a.len(), b.len()), padding the shorter input with zeros, and checks both byte-level differences and length equality at the end. Closes #57 Co-Authored-By: Claude Opus 4.6 --- src/security/pairing.rs | 26 +++++++++++++++++++------- src/security/secrets.rs | 2 +- src/tools/browser.rs | 1 + 3 files changed, 21 insertions(+), 8 deletions(-) diff --git a/src/security/pairing.rs b/src/security/pairing.rs index 5f55603..f9a9a05 100644 --- a/src/security/pairing.rs +++ b/src/security/pairing.rs @@ -174,15 +174,27 @@ fn generate_token() -> String { format!("zc_{}", uuid::Uuid::new_v4().as_simple()) } -/// Constant-time string comparison to prevent timing attacks on pairing code. +/// Constant-time string comparison to prevent timing attacks. +/// +/// Does not short-circuit on length mismatch — always iterates over the +/// longer input to avoid leaking length information via timing. pub fn constant_time_eq(a: &str, b: &str) -> bool { - if a.len() != b.len() { - return false; + let a = a.as_bytes(); + let b = b.as_bytes(); + + // Track length mismatch as a usize (non-zero = different lengths) + let len_diff = a.len() ^ b.len(); + + // XOR each byte, padding the shorter input with zeros. + // Iterates over max(a.len(), b.len()) to avoid timing differences. + let max_len = a.len().max(b.len()); + let mut byte_diff = 0u8; + for i in 0..max_len { + let x = if i < a.len() { a[i] } else { 0 }; + let y = if i < b.len() { b[i] } else { 0 }; + byte_diff |= x ^ y; } - a.bytes() - .zip(b.bytes()) - .fold(0u8, |acc, (x, y)| acc | (x ^ y)) - == 0 + len_diff == 0 && byte_diff == 0 } /// Check if a host string represents a non-localhost bind address. diff --git a/src/security/secrets.rs b/src/security/secrets.rs index bafad38..3940843 100644 --- a/src/security/secrets.rs +++ b/src/security/secrets.rs @@ -241,7 +241,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 2f2f56fc0c1c6540f79b895654ea6f6849423e39 Mon Sep 17 00:00:00 2001 From: fettpl <38704082+fettpl@users.noreply.github.com> Date: Sun, 15 Feb 2026 00:30:23 +0100 Subject: [PATCH 2/5] fix: use branchless operations in constant_time_eq - Use bitwise & instead of && to avoid short-circuit timing leak - Use get().unwrap_or(&0) instead of if/else for branchless byte access Co-Authored-By: Claude Opus 4.6 --- src/security/pairing.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/security/pairing.rs b/src/security/pairing.rs index f9a9a05..e176d38 100644 --- a/src/security/pairing.rs +++ b/src/security/pairing.rs @@ -190,11 +190,11 @@ pub fn constant_time_eq(a: &str, b: &str) -> bool { let max_len = a.len().max(b.len()); let mut byte_diff = 0u8; for i in 0..max_len { - let x = if i < a.len() { a[i] } else { 0 }; - let y = if i < b.len() { b[i] } else { 0 }; + let x = *a.get(i).unwrap_or(&0); + let y = *b.get(i).unwrap_or(&0); byte_diff |= x ^ y; } - len_diff == 0 && byte_diff == 0 + (len_diff == 0) & (byte_diff == 0) } /// Check if a host string represents a non-localhost bind address. From ac7c625368e6a89f9e8564b6820fcd06b4b095b8 Mon Sep 17 00:00:00 2001 From: fettpl <38704082+fettpl@users.noreply.github.com> Date: Sun, 15 Feb 2026 01:26:44 +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 3940843..bafad38 100644 --- a/src/security/secrets.rs +++ b/src/security/secrets.rs @@ -241,7 +241,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 e0341e599682ff6002e3d18954e78fa12e803b87 Mon Sep 17 00:00:00 2001 From: fettpl <38704082+fettpl@users.noreply.github.com> Date: Sun, 15 Feb 2026 02:15:24 +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 38a8d6a..8dea343 100644 --- a/src/security/secrets.rs +++ b/src/security/secrets.rs @@ -242,7 +242,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 33f64c714679429556032d56b886f62facb369d7 Mon Sep 17 00:00:00 2001 From: fettpl <38704082+fettpl@users.noreply.github.com> Date: Sun, 15 Feb 2026 02:27:13 +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();