From c9d76780f0edda88ebf88e877952dac491a9121c Mon Sep 17 00:00:00 2001 From: Chummy Date: Tue, 24 Feb 2026 14:52:30 +0800 Subject: [PATCH] fix(security): harden redirect/browser_open and restore masked secrets --- src/gateway/api.rs | 110 +++++++++++++++++++++++++++++ src/tools/browser_open.rs | 32 +++++---- src/tools/composio.rs | 18 ++++- src/tools/web_fetch.rs | 143 ++++++++++++++++++++++++++++---------- 4 files changed, 252 insertions(+), 51 deletions(-) diff --git a/src/gateway/api.rs b/src/gateway/api.rs index fe58acd8e..de570faa4 100644 --- a/src/gateway/api.rs +++ b/src/gateway/api.rs @@ -571,10 +571,17 @@ fn mask_sensitive_fields(config: &crate::config::Config) -> crate::config::Confi mask_optional_secret(&mut masked.api_key); mask_vec_secrets(&mut masked.reliability.api_keys); + mask_vec_secrets(&mut masked.gateway.paired_tokens); mask_optional_secret(&mut masked.composio.api_key); mask_optional_secret(&mut masked.browser.computer_use.api_key); mask_optional_secret(&mut masked.web_search.brave_api_key); mask_optional_secret(&mut masked.storage.provider.config.db_url); + if let Some(cloudflare) = masked.tunnel.cloudflare.as_mut() { + mask_required_secret(&mut cloudflare.token); + } + if let Some(ngrok) = masked.tunnel.ngrok.as_mut() { + mask_required_secret(&mut ngrok.auth_token); + } for agent in masked.agents.values_mut() { mask_optional_secret(&mut agent.api_key); @@ -612,6 +619,9 @@ fn mask_sensitive_fields(config: &crate::config::Config) -> crate::config::Confi mask_required_secret(&mut nextcloud.app_token); mask_optional_secret(&mut nextcloud.webhook_secret); } + if let Some(wati) = masked.channels_config.wati.as_mut() { + mask_required_secret(&mut wati.api_token); + } if let Some(irc) = masked.channels_config.irc.as_mut() { mask_optional_secret(&mut irc.server_password); mask_optional_secret(&mut irc.nickserv_password); @@ -643,6 +653,10 @@ fn restore_masked_sensitive_fields( current: &crate::config::Config, ) { restore_optional_secret(&mut incoming.api_key, ¤t.api_key); + restore_vec_secrets( + &mut incoming.gateway.paired_tokens, + ¤t.gateway.paired_tokens, + ); restore_vec_secrets( &mut incoming.reliability.api_keys, ¤t.reliability.api_keys, @@ -660,6 +674,18 @@ fn restore_masked_sensitive_fields( &mut incoming.storage.provider.config.db_url, ¤t.storage.provider.config.db_url, ); + if let (Some(incoming_tunnel), Some(current_tunnel)) = ( + incoming.tunnel.cloudflare.as_mut(), + current.tunnel.cloudflare.as_ref(), + ) { + restore_required_secret(&mut incoming_tunnel.token, ¤t_tunnel.token); + } + if let (Some(incoming_tunnel), Some(current_tunnel)) = ( + incoming.tunnel.ngrok.as_mut(), + current.tunnel.ngrok.as_ref(), + ) { + restore_required_secret(&mut incoming_tunnel.auth_token, ¤t_tunnel.auth_token); + } for (name, agent) in incoming.agents.iter_mut() { if let Some(current_agent) = current.agents.get(name) { @@ -726,6 +752,12 @@ fn restore_masked_sensitive_fields( restore_required_secret(&mut incoming_ch.app_token, ¤t_ch.app_token); restore_optional_secret(&mut incoming_ch.webhook_secret, ¤t_ch.webhook_secret); } + if let (Some(incoming_ch), Some(current_ch)) = ( + incoming.channels_config.wati.as_mut(), + current.channels_config.wati.as_ref(), + ) { + restore_required_secret(&mut incoming_ch.api_token, ¤t_ch.api_token); + } if let (Some(incoming_ch), Some(current_ch)) = ( incoming.channels_config.irc.as_mut(), current.channels_config.irc.as_ref(), @@ -798,6 +830,16 @@ mod tests { let mut cfg = crate::config::Config::default(); cfg.api_key = Some("sk-live-123".to_string()); cfg.reliability.api_keys = vec!["rk-1".to_string(), "rk-2".to_string()]; + cfg.gateway.paired_tokens = vec!["pair-token-1".to_string()]; + cfg.tunnel.cloudflare = Some(crate::config::CloudflareTunnelConfig { + token: "cf-token".to_string(), + }); + cfg.channels_config.wati = Some(crate::config::WatiConfig { + api_token: "wati-token".to_string(), + api_url: "https://live-mt-server.wati.io".to_string(), + tenant_id: None, + allowed_numbers: vec![], + }); let masked = mask_sensitive_fields(&cfg); let toml = toml::to_string_pretty(&masked).expect("masked config should serialize"); @@ -809,6 +851,22 @@ mod tests { parsed.reliability.api_keys, vec![MASKED_SECRET.to_string(), MASKED_SECRET.to_string()] ); + assert_eq!( + parsed.gateway.paired_tokens, + vec![MASKED_SECRET.to_string()] + ); + assert_eq!( + parsed.tunnel.cloudflare.as_ref().map(|v| v.token.as_str()), + Some(MASKED_SECRET) + ); + assert_eq!( + parsed + .channels_config + .wati + .as_ref() + .map(|v| v.api_token.as_str()), + Some(MASKED_SECRET) + ); } #[test] @@ -818,11 +876,35 @@ mod tests { current.workspace_dir = std::path::PathBuf::from("/tmp/current/workspace"); current.api_key = Some("real-key".to_string()); current.reliability.api_keys = vec!["r1".to_string(), "r2".to_string()]; + current.gateway.paired_tokens = vec!["pair-1".to_string(), "pair-2".to_string()]; + current.tunnel.cloudflare = Some(crate::config::CloudflareTunnelConfig { + token: "cf-token-real".to_string(), + }); + current.tunnel.ngrok = Some(crate::config::NgrokTunnelConfig { + auth_token: "ngrok-token-real".to_string(), + domain: None, + }); + current.channels_config.wati = Some(crate::config::WatiConfig { + api_token: "wati-real".to_string(), + api_url: "https://live-mt-server.wati.io".to_string(), + tenant_id: None, + allowed_numbers: vec![], + }); let mut incoming = mask_sensitive_fields(¤t); incoming.default_model = Some("gpt-4.1-mini".to_string()); // Simulate UI changing only one key and keeping the first masked. incoming.reliability.api_keys = vec![MASKED_SECRET.to_string(), "r2-new".to_string()]; + incoming.gateway.paired_tokens = vec![MASKED_SECRET.to_string(), "pair-2-new".to_string()]; + if let Some(cloudflare) = incoming.tunnel.cloudflare.as_mut() { + cloudflare.token = MASKED_SECRET.to_string(); + } + if let Some(ngrok) = incoming.tunnel.ngrok.as_mut() { + ngrok.auth_token = MASKED_SECRET.to_string(); + } + if let Some(wati) = incoming.channels_config.wati.as_mut() { + wati.api_token = MASKED_SECRET.to_string(); + } let hydrated = hydrate_config_for_save(incoming, ¤t); @@ -834,5 +916,33 @@ mod tests { hydrated.reliability.api_keys, vec!["r1".to_string(), "r2-new".to_string()] ); + assert_eq!( + hydrated.gateway.paired_tokens, + vec!["pair-1".to_string(), "pair-2-new".to_string()] + ); + assert_eq!( + hydrated + .tunnel + .cloudflare + .as_ref() + .map(|v| v.token.as_str()), + Some("cf-token-real") + ); + assert_eq!( + hydrated + .tunnel + .ngrok + .as_ref() + .map(|v| v.auth_token.as_str()), + Some("ngrok-token-real") + ); + assert_eq!( + hydrated + .channels_config + .wati + .as_ref() + .map(|v| v.api_token.as_str()), + Some("wati-real") + ); } } diff --git a/src/tools/browser_open.rs b/src/tools/browser_open.rs index 2c31fa1c6..7ac5013f7 100644 --- a/src/tools/browser_open.rs +++ b/src/tools/browser_open.rs @@ -192,26 +192,32 @@ async fn open_in_system_browser(url: &str) -> anyhow::Result<()> { #[cfg(target_os = "windows")] { - let primary_error = match tokio::process::Command::new("cmd") - .args(["/C", "start", "", url]) + // Use direct process invocation (not `cmd /C start`) to avoid shell + // metacharacter interpretation in URLs (e.g. `&` in query strings). + let primary_error = match tokio::process::Command::new("rundll32") + .arg("url.dll,FileProtocolHandler") + .arg(url) .status() .await { Ok(status) if status.success() => return Ok(()), - Ok(status) => format!("cmd start default-browser exited with status {status}"), - Err(error) => format!("cmd start default-browser not runnable: {error}"), + Ok(status) => format!("rundll32 default-browser launcher exited with status {status}"), + Err(error) => format!("rundll32 default-browser launcher not runnable: {error}"), }; // TODO(compat): remove Brave fallback after default-browser launch has been stable across Windows environments. - let brave_error = match tokio::process::Command::new("cmd") - .args(["/C", "start", "", "brave", url]) - .status() - .await - { - Ok(status) if status.success() => return Ok(()), - Ok(status) => format!("cmd start brave exited with status {status}"), - Err(error) => format!("cmd start brave not runnable: {error}"), - }; + let mut brave_error = String::new(); + for cmd in ["brave", "brave.exe"] { + match tokio::process::Command::new(cmd).arg(url).status().await { + Ok(status) if status.success() => return Ok(()), + Ok(status) => { + brave_error = format!("{cmd} exited with status {status}"); + } + Err(error) => { + brave_error = format!("{cmd} not runnable: {error}"); + } + } + } anyhow::bail!( "Failed to open URL with default browser launcher: {primary_error}. Brave compatibility fallback also failed: {brave_error}" diff --git a/src/tools/composio.rs b/src/tools/composio.rs index cb0f4fe32..d414d1649 100644 --- a/src/tools/composio.rs +++ b/src/tools/composio.rs @@ -1162,6 +1162,14 @@ fn format_input_params_hint(schema: Option<&serde_json::Value>) -> String { format!(" [params: {}]", keys.join(", ")) } +fn floor_char_boundary_compat(text: &str, index: usize) -> usize { + let mut end = index.min(text.len()); + while end > 0 && !text.is_char_boundary(end) { + end -= 1; + } + end +} + /// Build a human-readable schema hint from a full tool schema response. /// /// Used in execute error messages so the LLM can see the expected parameter @@ -1197,7 +1205,7 @@ fn format_schema_hint(schema: &serde_json::Value) -> Option { // Truncate long descriptions to keep the hint concise. // Use char boundary to avoid panic on multi-byte UTF-8. let short = if desc.len() > 80 { - let end = desc.floor_char_boundary(77); + let end = floor_char_boundary_compat(desc, 77); format!("{}...", &desc[..end]) } else { desc.to_string() @@ -1545,6 +1553,14 @@ mod tests { assert!(hyphen.contains(&"github_list_repos".to_string())); } + #[test] + fn floor_char_boundary_compat_handles_multibyte_offsets() { + let text = "abcπŸ˜€def"; + // Byte offset 5 is inside the 4-byte emoji, so boundary should floor to 3. + assert_eq!(floor_char_boundary_compat(text, 5), 3); + assert_eq!(floor_char_boundary_compat(text, usize::MAX), text.len()); + } + #[test] fn normalize_action_cache_key_merges_underscore_and_hyphen_variants() { assert_eq!( diff --git a/src/tools/web_fetch.rs b/src/tools/web_fetch.rs index e49eb57e0..6424ca30d 100644 --- a/src/tools/web_fetch.rs +++ b/src/tools/web_fetch.rs @@ -39,42 +39,12 @@ impl WebFetchTool { } fn validate_url(&self, raw_url: &str) -> anyhow::Result { - let url = raw_url.trim(); - - if url.is_empty() { - anyhow::bail!("URL cannot be empty"); - } - - if url.chars().any(char::is_whitespace) { - anyhow::bail!("URL cannot contain whitespace"); - } - - if !url.starts_with("http://") && !url.starts_with("https://") { - anyhow::bail!("Only http:// and https:// URLs are allowed"); - } - - if self.allowed_domains.is_empty() { - anyhow::bail!( - "web_fetch tool is enabled but no allowed_domains are configured. \ - Add [web_fetch].allowed_domains in config.toml" - ); - } - - let host = extract_host(url)?; - - if is_private_or_local_host(&host) { - anyhow::bail!("Blocked local/private host: {host}"); - } - - if host_matches_allowlist(&host, &self.blocked_domains) { - anyhow::bail!("Host '{host}' is in web_fetch.blocked_domains"); - } - - if !host_matches_allowlist(&host, &self.allowed_domains) { - anyhow::bail!("Host '{host}' is not in web_fetch.allowed_domains"); - } - - Ok(url.to_string()) + validate_target_url( + raw_url, + &self.allowed_domains, + &self.blocked_domains, + "web_fetch", + ) } fn truncate_response(&self, text: &str) -> String { @@ -159,10 +129,32 @@ impl Tool for WebFetchTool { self.timeout_secs }; + let allowed_domains = self.allowed_domains.clone(); + let blocked_domains = self.blocked_domains.clone(); + let redirect_policy = reqwest::redirect::Policy::custom(move |attempt| { + if attempt.previous().len() >= 10 { + return attempt.error(std::io::Error::other("Too many redirects (max 10)")); + } + + if let Err(err) = validate_target_url( + attempt.url().as_str(), + &allowed_domains, + &blocked_domains, + "web_fetch", + ) { + return attempt.error(std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + format!("Blocked redirect target: {err}"), + )); + } + + attempt.follow() + }); + let builder = reqwest::Client::builder() .timeout(Duration::from_secs(timeout_secs)) .connect_timeout(Duration::from_secs(10)) - .redirect(reqwest::redirect::Policy::limited(10)) + .redirect(redirect_policy) .user_agent("ZeroClaw/0.1 (web_fetch)"); let builder = crate::config::apply_runtime_proxy_to_builder(builder, "tool.web_fetch"); let client = match builder.build() { @@ -252,6 +244,50 @@ impl Tool for WebFetchTool { // ── Helper functions (independent from http_request.rs per DRY rule-of-three) ── +fn validate_target_url( + raw_url: &str, + allowed_domains: &[String], + blocked_domains: &[String], + tool_name: &str, +) -> anyhow::Result { + let url = raw_url.trim(); + + if url.is_empty() { + anyhow::bail!("URL cannot be empty"); + } + + if url.chars().any(char::is_whitespace) { + anyhow::bail!("URL cannot contain whitespace"); + } + + if !url.starts_with("http://") && !url.starts_with("https://") { + anyhow::bail!("Only http:// and https:// URLs are allowed"); + } + + if allowed_domains.is_empty() { + anyhow::bail!( + "{tool_name} tool is enabled but no allowed_domains are configured. \ + Add [{tool_name}].allowed_domains in config.toml" + ); + } + + let host = extract_host(url)?; + + if is_private_or_local_host(&host) { + anyhow::bail!("Blocked local/private host: {host}"); + } + + if host_matches_allowlist(&host, blocked_domains) { + anyhow::bail!("Host '{host}' is in {tool_name}.blocked_domains"); + } + + if !host_matches_allowlist(&host, allowed_domains) { + anyhow::bail!("Host '{host}' is not in {tool_name}.allowed_domains"); + } + + Ok(url.to_string()) +} + fn normalize_allowed_domains(domains: Vec) -> Vec { let mut normalized = domains .into_iter() @@ -561,6 +597,39 @@ mod tests { assert!(err.contains("local/private")); } + #[test] + fn redirect_target_validation_allows_permitted_host() { + let allowed = vec!["example.com".to_string()]; + let blocked = vec![]; + assert!(validate_target_url( + "https://docs.example.com/page", + &allowed, + &blocked, + "web_fetch" + ) + .is_ok()); + } + + #[test] + fn redirect_target_validation_blocks_private_host() { + let allowed = vec!["example.com".to_string()]; + let blocked = vec![]; + let err = validate_target_url("https://127.0.0.1/admin", &allowed, &blocked, "web_fetch") + .unwrap_err() + .to_string(); + assert!(err.contains("local/private")); + } + + #[test] + fn redirect_target_validation_blocks_blocklisted_host() { + let allowed = vec!["*".to_string()]; + let blocked = vec!["evil.com".to_string()]; + let err = validate_target_url("https://evil.com/phish", &allowed, &blocked, "web_fetch") + .unwrap_err() + .to_string(); + assert!(err.contains("blocked_domains")); + } + // ── Security policy ────────────────────────────────────────── #[tokio::test]