polymech/zeroclaw
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(security): harden redirect/browser_open and restore masked secrets

Browse Source
This commit is contained in:
Chummy 2026-02-24 14:52:30 +08:00
parent 16c509f939
commit c9d76780f0
No known key found for this signature in database
4 changed files with 252 additions and 51 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

110
src/gateway/api.rs
View File

@ -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, &current.api_key);
restore_vec_secrets(
&mut incoming.gateway.paired_tokens,
&current.gateway.paired_tokens,
);
restore_vec_secrets(
&mut incoming.reliability.api_keys,
&current.reliability.api_keys,
@ -660,6 +674,18 @@ fn restore_masked_sensitive_fields(
&mut incoming.storage.provider.config.db_url,
&current.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, &current_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, &current_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, &current_ch.app_token);
restore_optional_secret(&mut incoming_ch.webhook_secret, &current_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, &current_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(&current);
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, &current);
@ -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")
);
}
}

32
src/tools/browser_open.rs
View File

@ -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}"

18
src/tools/composio.rs
View File

@ -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<String> {
// 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!(

143
src/tools/web_fetch.rs
View File

@ -39,42 +39,12 @@ impl WebFetchTool {
}
fn validate_url(&self, raw_url: &str) -> anyhow::Result<String> {
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<String> {
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<String>) -> Vec<String> {
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]