From b3bfbaff4a5221986bb6b3fbb78f456f6dd5d29e Mon Sep 17 00:00:00 2001 From: fettpl <38704082+fettpl@users.noreply.github.com> Date: Sat, 14 Feb 2026 23:58:09 +0100 Subject: [PATCH 1/5] fix: store bearer tokens as SHA-256 hashes instead of plaintext Hash paired bearer tokens with SHA-256 before storing in config and in-memory. When authenticating, hash the incoming token and compare against stored hashes. Backward compatible: existing plaintext tokens (zc_ prefix) are detected and hashed on load; already-hashed tokens (64-char hex) are stored as-is. Closes #58 Co-Authored-By: Claude Opus 4.6 --- Cargo.toml | 3 ++ src/security/pairing.rs | 99 ++++++++++++++++++++++++++++++++++++----- src/security/secrets.rs | 2 +- src/tools/browser.rs | 1 + 4 files changed, 93 insertions(+), 12 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index eebcbc9..fbdb65c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -43,6 +43,9 @@ uuid = { version = "1.11", default-features = false, features = ["v4", "std"] } # Authenticated encryption (AEAD) for secret store chacha20poly1305 = "0.10" +# SHA-256 for bearer token hashing +sha2 = "0.10" + # Async traits async-trait = "0.1" diff --git a/src/security/pairing.rs b/src/security/pairing.rs index 5f55603..e8d946c 100644 --- a/src/security/pairing.rs +++ b/src/security/pairing.rs @@ -8,6 +8,7 @@ // Already-paired tokens are persisted in config so restarts don't require // re-pairing. +use sha2::{Digest, Sha256}; use std::collections::HashSet; use std::sync::Mutex; use std::time::Instant; @@ -18,13 +19,17 @@ const MAX_PAIR_ATTEMPTS: u32 = 5; const PAIR_LOCKOUT_SECS: u64 = 300; // 5 minutes /// Manages pairing state for the gateway. +/// +/// Bearer tokens are stored as SHA-256 hashes to prevent plaintext exposure +/// in config files. When a new token is generated, the plaintext is returned +/// to the client once, and only the hash is retained. #[derive(Debug)] pub struct PairingGuard { /// Whether pairing is required at all. require_pairing: bool, /// One-time pairing code (generated on startup, consumed on first pair). pairing_code: Option, - /// Set of valid bearer tokens (persisted across restarts). + /// Set of SHA-256 hashed bearer tokens (persisted across restarts). paired_tokens: Mutex>, /// Brute-force protection: failed attempt counter + lockout time. failed_attempts: Mutex<(u32, Option)>, @@ -35,8 +40,21 @@ impl PairingGuard { /// /// If `require_pairing` is true and no tokens exist yet, a fresh /// pairing code is generated and returned via `pairing_code()`. + /// + /// Existing tokens are accepted in both forms: + /// - Plaintext (`zc_...`): hashed on load for backward compatibility + /// - Already hashed (64-char hex): stored as-is pub fn new(require_pairing: bool, existing_tokens: &[String]) -> Self { - let tokens: HashSet = existing_tokens.iter().cloned().collect(); + let tokens: HashSet = existing_tokens + .iter() + .map(|t| { + if is_token_hash(t) { + t.clone() + } else { + hash_token(t) + } + }) + .collect(); let code = if require_pairing && tokens.is_empty() { Some(generate_code()) } else { @@ -94,7 +112,7 @@ impl PairingGuard { .paired_tokens .lock() .unwrap_or_else(std::sync::PoisonError::into_inner); - tokens.insert(token.clone()); + tokens.insert(hash_token(&token)); return Ok(Some(token)); } } @@ -114,16 +132,17 @@ impl PairingGuard { Ok(None) } - /// Check if a bearer token is valid. + /// Check if a bearer token is valid (compares against stored hashes). pub fn is_authenticated(&self, token: &str) -> bool { if !self.require_pairing { return true; } + let hashed = hash_token(token); let tokens = self .paired_tokens .lock() .unwrap_or_else(std::sync::PoisonError::into_inner); - tokens.contains(token) + tokens.contains(&hashed) } /// Returns true if the gateway is already paired (has at least one token). @@ -135,7 +154,7 @@ impl PairingGuard { !tokens.is_empty() } - /// Get all paired tokens (for persisting to config). + /// Get all paired token hashes (for persisting to config). pub fn tokens(&self) -> Vec { let tokens = self .paired_tokens @@ -174,6 +193,23 @@ fn generate_token() -> String { format!("zc_{}", uuid::Uuid::new_v4().as_simple()) } +/// SHA-256 hash a bearer token for storage. Returns lowercase hex. +fn hash_token(token: &str) -> String { + let digest = Sha256::digest(token.as_bytes()); + let mut hex = String::with_capacity(64); + for b in digest { + use std::fmt::Write; + let _ = write!(hex, "{b:02x}"); + } + hex +} + +/// Check if a stored value looks like a SHA-256 hash (64 hex chars) +/// rather than a plaintext token. +fn is_token_hash(value: &str) -> bool { + value.len() == 64 && value.chars().all(|c| c.is_ascii_hexdigit()) +} + /// Constant-time string comparison to prevent timing attacks on pairing code. pub fn constant_time_eq(a: &str, b: &str) -> bool { if a.len() != b.len() { @@ -246,10 +282,19 @@ mod tests { #[test] fn is_authenticated_with_valid_token() { + // Pass plaintext token — PairingGuard hashes it on load let guard = PairingGuard::new(true, &["zc_valid".into()]); assert!(guard.is_authenticated("zc_valid")); } + #[test] + fn is_authenticated_with_prehashed_token() { + // Pass an already-hashed token (64 hex chars) + let hashed = hash_token("zc_valid"); + let guard = PairingGuard::new(true, &[hashed]); + assert!(guard.is_authenticated("zc_valid")); + } + #[test] fn is_authenticated_with_invalid_token() { let guard = PairingGuard::new(true, &["zc_valid".into()]); @@ -264,11 +309,16 @@ mod tests { } #[test] - fn tokens_returns_all_paired() { - let guard = PairingGuard::new(true, &["a".into(), "b".into()]); - let mut tokens = guard.tokens(); - tokens.sort(); - assert_eq!(tokens, vec!["a", "b"]); + fn tokens_returns_hashes() { + let guard = PairingGuard::new(true, &["zc_a".into(), "zc_b".into()]); + let tokens = guard.tokens(); + assert_eq!(tokens.len(), 2); + // Tokens should be stored as 64-char hex hashes, not plaintext + for t in &tokens { + assert_eq!(t.len(), 64, "Token should be a SHA-256 hash"); + assert!(t.chars().all(|c| c.is_ascii_hexdigit())); + assert!(!t.starts_with("zc_"), "Token should not be plaintext"); + } } #[test] @@ -280,6 +330,33 @@ mod tests { assert!(!guard.is_authenticated("wrong")); } + // ── Token hashing ──────────────────────────────────────── + + #[test] + fn hash_token_produces_64_hex_chars() { + let hash = hash_token("zc_test_token"); + assert_eq!(hash.len(), 64); + assert!(hash.chars().all(|c| c.is_ascii_hexdigit())); + } + + #[test] + fn hash_token_is_deterministic() { + assert_eq!(hash_token("zc_abc"), hash_token("zc_abc")); + } + + #[test] + fn hash_token_differs_for_different_inputs() { + assert_ne!(hash_token("zc_a"), hash_token("zc_b")); + } + + #[test] + fn is_token_hash_detects_hash_vs_plaintext() { + assert!(is_token_hash(&hash_token("zc_test"))); + assert!(!is_token_hash("zc_test_token")); + assert!(!is_token_hash("too_short")); + assert!(!is_token_hash("")); + } + // ── is_public_bind ─────────────────────────────────────── #[test] 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 23048d10ac07fbc0b84603a6c1ce13396ad7c280 Mon Sep 17 00:00:00 2001 From: fettpl <38704082+fettpl@users.noreply.github.com> Date: Sun, 15 Feb 2026 00:28:04 +0100 Subject: [PATCH 2/5] refactor: simplify hash_token using format macro Replace manual hex encoding loop with `format!("{:x}", Sha256::digest(...))`, which is more idiomatic and concise. Co-Authored-By: Claude Opus 4.6 --- src/security/pairing.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/security/pairing.rs b/src/security/pairing.rs index e8d946c..dd5f2eb 100644 --- a/src/security/pairing.rs +++ b/src/security/pairing.rs @@ -195,13 +195,7 @@ fn generate_token() -> String { /// SHA-256 hash a bearer token for storage. Returns lowercase hex. fn hash_token(token: &str) -> String { - let digest = Sha256::digest(token.as_bytes()); - let mut hex = String::with_capacity(64); - for b in digest { - use std::fmt::Write; - let _ = write!(hex, "{b:02x}"); - } - hex + format!("{:x}", Sha256::digest(token.as_bytes())) } /// Check if a stored value looks like a SHA-256 hash (64 hex chars) From 671c3b2a554f83b39406691541d85c018cab96f9 Mon Sep 17 00:00:00 2001 From: fettpl <38704082+fettpl@users.noreply.github.com> Date: Sun, 15 Feb 2026 01:26:24 +0100 Subject: [PATCH 3/5] fix: replace unstable is_multiple_of and update Cargo.lock for sha2 The Docker image uses rust:1.83-slim where is_multiple_of is unstable. Also regenerates Cargo.lock to include the sha2 dependency. Co-Authored-By: Claude Opus 4.6 --- Cargo.lock | 12 ++++++++++++ src/security/secrets.rs | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index 5a5debc..6e29ff6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1510,6 +1510,17 @@ dependencies = [ "digest", ] +[[package]] +name = "sha2" +version = "0.10.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a7507d819769d01a365ab707794a4084392c824f54a7a6a7862f8c3d0892b283" +dependencies = [ + "cfg-if", + "cpufeatures", + "digest", +] + [[package]] name = "sharded-slab" version = "0.1.7" @@ -2491,6 +2502,7 @@ dependencies = [ "rusqlite", "serde", "serde_json", + "sha2", "shellexpand", "tempfile", "thiserror 2.0.18", 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 0603bed8431ca7eb0f68ddc768964839ca9050a1 Mon Sep 17 00:00:00 2001 From: fettpl <38704082+fettpl@users.noreply.github.com> Date: Sun, 15 Feb 2026 02:15:08 +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 e62b7c9153c85afdbc93c16c0051309b15715bc2 Mon Sep 17 00:00:00 2001 From: fettpl <38704082+fettpl@users.noreply.github.com> Date: Sun, 15 Feb 2026 02:27:08 +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();