From d00c1140d9baf03aca55f2ec492f00e86111d590 Mon Sep 17 00:00:00 2001 From: Chummy Date: Tue, 17 Feb 2026 18:25:40 +0800 Subject: [PATCH] fix(tools): harden pushover security and validation --- .env.example | 5 + src/tools/mod.rs | 7 +- src/tools/pushover.rs | 225 +++++++++++++++++++++++++++++++++++++----- 3 files changed, 212 insertions(+), 25 deletions(-) diff --git a/.env.example b/.env.example index 6fd6fc6..7a2c253 100644 --- a/.env.example +++ b/.env.example @@ -60,6 +60,11 @@ PROVIDER=openrouter # ZEROCLAW_GATEWAY_HOST=127.0.0.1 # ZEROCLAW_ALLOW_PUBLIC_BIND=false +# ── Optional Integrations ──────────────────────────────────── +# Pushover notifications (`pushover` tool) +# PUSHOVER_TOKEN=your-pushover-app-token +# PUSHOVER_USER_KEY=your-pushover-user-key + # ── Docker Compose ─────────────────────────────────────────── # Host port mapping (used by docker-compose.yml) # HOST_PORT=3000 diff --git a/src/tools/mod.rs b/src/tools/mod.rs index 1c8547e..7c4a8fc 100644 --- a/src/tools/mod.rs +++ b/src/tools/mod.rs @@ -143,7 +143,10 @@ pub fn all_tools_with_runtime( security.clone(), workspace_dir.to_path_buf(), )), - Box::new(PushoverTool::new(workspace_dir.to_path_buf())), + Box::new(PushoverTool::new( + security.clone(), + workspace_dir.to_path_buf(), + )), ]; if browser_config.enabled { @@ -264,6 +267,7 @@ mod tests { let names: Vec<&str> = tools.iter().map(|t| t.name()).collect(); assert!(!names.contains(&"browser_open")); assert!(names.contains(&"schedule")); + assert!(names.contains(&"pushover")); } #[test] @@ -301,6 +305,7 @@ mod tests { ); let names: Vec<&str> = tools.iter().map(|t| t.name()).collect(); assert!(names.contains(&"browser_open")); + assert!(names.contains(&"pushover")); } #[test] diff --git a/src/tools/pushover.rs b/src/tools/pushover.rs index 39f7699..ad1d385 100644 --- a/src/tools/pushover.rs +++ b/src/tools/pushover.rs @@ -1,26 +1,59 @@ use super::traits::{Tool, ToolResult}; +use crate::security::SecurityPolicy; use async_trait::async_trait; use reqwest::Client; use serde_json::json; use std::path::PathBuf; +use std::sync::Arc; +use std::time::Duration; + +const PUSHOVER_API_URL: &str = "https://api.pushover.net/1/messages.json"; +const PUSHOVER_REQUEST_TIMEOUT_SECS: u64 = 15; pub struct PushoverTool { client: Client, + security: Arc, workspace_dir: PathBuf, } impl PushoverTool { - pub fn new(workspace_dir: PathBuf) -> Self { + pub fn new(security: Arc, workspace_dir: PathBuf) -> Self { + let client = Client::builder() + .timeout(Duration::from_secs(PUSHOVER_REQUEST_TIMEOUT_SECS)) + .build() + .unwrap_or_else(|_| Client::new()); + Self { - client: Client::new(), + client, + security, workspace_dir, } } + fn parse_env_value(raw: &str) -> String { + let raw = raw.trim(); + + let unquoted = if raw.len() >= 2 + && ((raw.starts_with('"') && raw.ends_with('"')) + || (raw.starts_with('\'') && raw.ends_with('\''))) + { + &raw[1..raw.len() - 1] + } else { + raw + }; + + // Keep support for inline comments in unquoted values: + // KEY=value # comment + unquoted.split_once(" #").map_or_else( + || unquoted.trim().to_string(), + |(value, _)| value.trim().to_string(), + ) + } + fn get_credentials(&self) -> anyhow::Result<(String, String)> { let env_path = self.workspace_dir.join(".env"); let content = std::fs::read_to_string(&env_path) - .map_err(|e| anyhow::anyhow!("Failed to read .env: {}", e))?; + .map_err(|e| anyhow::anyhow!("Failed to read {}: {}", env_path.display(), e))?; let mut token = None; let mut user_key = None; @@ -30,13 +63,15 @@ impl PushoverTool { if line.starts_with('#') || line.is_empty() { continue; } + let line = line.strip_prefix("export ").map(str::trim).unwrap_or(line); if let Some((key, value)) = line.split_once('=') { let key = key.trim(); - let value = value.trim(); + let value = Self::parse_env_value(value); + if key.eq_ignore_ascii_case("PUSHOVER_TOKEN") { - token = Some(value.to_string()); + token = Some(value); } else if key.eq_ignore_ascii_case("PUSHOVER_USER_KEY") { - user_key = Some(value.to_string()); + user_key = Some(value); } } } @@ -86,15 +121,45 @@ impl Tool for PushoverTool { } async fn execute(&self, args: serde_json::Value) -> anyhow::Result { + if !self.security.can_act() { + return Ok(ToolResult { + success: false, + output: String::new(), + error: Some("Action blocked: autonomy is read-only".into()), + }); + } + + if !self.security.record_action() { + return Ok(ToolResult { + success: false, + output: String::new(), + error: Some("Action blocked: rate limit exceeded".into()), + }); + } + let message = args .get("message") .and_then(|v| v.as_str()) + .map(str::trim) + .filter(|v| !v.is_empty()) .ok_or_else(|| anyhow::anyhow!("Missing 'message' parameter"))? .to_string(); let title = args.get("title").and_then(|v| v.as_str()).map(String::from); - let priority = args.get("priority").and_then(|v| v.as_i64()); + let priority = match args.get("priority").and_then(|v| v.as_i64()) { + Some(value) if (-2..=2).contains(&value) => Some(value), + Some(value) => { + return Ok(ToolResult { + success: false, + output: String::new(), + error: Some(format!( + "Invalid 'priority': {value}. Expected integer in range -2..=2" + )), + }) + } + None => None, + }; let sound = args.get("sound").and_then(|v| v.as_str()).map(String::from); @@ -110,9 +175,7 @@ impl Tool for PushoverTool { } if let Some(priority) = priority { - if priority >= -2 && priority <= 2 { - form = form.text("priority", priority.to_string()); - } + form = form.text("priority", priority.to_string()); } if let Some(sound) = sound { @@ -121,7 +184,7 @@ impl Tool for PushoverTool { let response = self .client - .post("https://api.pushover.net/1/messages.json") + .post(PUSHOVER_API_URL) .multipart(form) .send() .await?; @@ -129,7 +192,19 @@ impl Tool for PushoverTool { let status = response.status(); let body = response.text().await.unwrap_or_default(); - if status.is_success() { + if !status.is_success() { + return Ok(ToolResult { + success: false, + output: body, + error: Some(format!("Pushover API returned status {}", status)), + }); + } + + let api_status = serde_json::from_str::(&body) + .ok() + .and_then(|json| json.get("status").and_then(|value| value.as_i64())); + + if api_status == Some(1) { Ok(ToolResult { success: true, output: format!( @@ -142,7 +217,7 @@ impl Tool for PushoverTool { Ok(ToolResult { success: false, output: body, - error: Some(format!("Pushover API returned status {}", status)), + error: Some("Pushover API returned an application-level error".into()), }) } } @@ -151,24 +226,43 @@ impl Tool for PushoverTool { #[cfg(test)] mod tests { use super::*; + use crate::security::AutonomyLevel; use std::fs; use tempfile::TempDir; + fn test_security(level: AutonomyLevel, max_actions_per_hour: u32) -> Arc { + Arc::new(SecurityPolicy { + autonomy: level, + max_actions_per_hour, + workspace_dir: std::env::temp_dir(), + ..SecurityPolicy::default() + }) + } + #[test] fn pushover_tool_name() { - let tool = PushoverTool::new(PathBuf::from("/tmp")); + let tool = PushoverTool::new( + test_security(AutonomyLevel::Full, 100), + PathBuf::from("/tmp"), + ); assert_eq!(tool.name(), "pushover"); } #[test] fn pushover_tool_description() { - let tool = PushoverTool::new(PathBuf::from("/tmp")); + let tool = PushoverTool::new( + test_security(AutonomyLevel::Full, 100), + PathBuf::from("/tmp"), + ); assert!(!tool.description().is_empty()); } #[test] fn pushover_tool_has_parameters_schema() { - let tool = PushoverTool::new(PathBuf::from("/tmp")); + let tool = PushoverTool::new( + test_security(AutonomyLevel::Full, 100), + PathBuf::from("/tmp"), + ); let schema = tool.parameters_schema(); assert_eq!(schema["type"], "object"); assert!(schema["properties"].get("message").is_some()); @@ -176,7 +270,10 @@ mod tests { #[test] fn pushover_tool_requires_message() { - let tool = PushoverTool::new(PathBuf::from("/tmp")); + let tool = PushoverTool::new( + test_security(AutonomyLevel::Full, 100), + PathBuf::from("/tmp"), + ); let schema = tool.parameters_schema(); let required = schema["required"].as_array().unwrap(); assert!(required.contains(&serde_json::Value::String("message".to_string()))); @@ -192,7 +289,10 @@ mod tests { ) .unwrap(); - let tool = PushoverTool::new(tmp.path().to_path_buf()); + let tool = PushoverTool::new( + test_security(AutonomyLevel::Full, 100), + tmp.path().to_path_buf(), + ); let result = tool.get_credentials(); assert!(result.is_ok()); @@ -204,7 +304,10 @@ mod tests { #[test] fn credentials_fail_without_env_file() { let tmp = TempDir::new().unwrap(); - let tool = PushoverTool::new(tmp.path().to_path_buf()); + let tool = PushoverTool::new( + test_security(AutonomyLevel::Full, 100), + tmp.path().to_path_buf(), + ); let result = tool.get_credentials(); assert!(result.is_err()); @@ -216,7 +319,10 @@ mod tests { let env_path = tmp.path().join(".env"); fs::write(&env_path, "PUSHOVER_USER_KEY=userkey456\n").unwrap(); - let tool = PushoverTool::new(tmp.path().to_path_buf()); + let tool = PushoverTool::new( + test_security(AutonomyLevel::Full, 100), + tmp.path().to_path_buf(), + ); let result = tool.get_credentials(); assert!(result.is_err()); @@ -228,7 +334,10 @@ mod tests { let env_path = tmp.path().join(".env"); fs::write(&env_path, "PUSHOVER_TOKEN=testtoken123\n").unwrap(); - let tool = PushoverTool::new(tmp.path().to_path_buf()); + let tool = PushoverTool::new( + test_security(AutonomyLevel::Full, 100), + tmp.path().to_path_buf(), + ); let result = tool.get_credentials(); assert!(result.is_err()); @@ -240,7 +349,10 @@ mod tests { let env_path = tmp.path().join(".env"); fs::write(&env_path, "# This is a comment\nPUSHOVER_TOKEN=realtoken\n# Another comment\nPUSHOVER_USER_KEY=realuser\n").unwrap(); - let tool = PushoverTool::new(tmp.path().to_path_buf()); + let tool = PushoverTool::new( + test_security(AutonomyLevel::Full, 100), + tmp.path().to_path_buf(), + ); let result = tool.get_credentials(); assert!(result.is_ok()); @@ -251,15 +363,80 @@ mod tests { #[test] fn pushover_tool_supports_priority() { - let tool = PushoverTool::new(PathBuf::from("/tmp")); + let tool = PushoverTool::new( + test_security(AutonomyLevel::Full, 100), + PathBuf::from("/tmp"), + ); let schema = tool.parameters_schema(); assert!(schema["properties"].get("priority").is_some()); } #[test] fn pushover_tool_supports_sound() { - let tool = PushoverTool::new(PathBuf::from("/tmp")); + let tool = PushoverTool::new( + test_security(AutonomyLevel::Full, 100), + PathBuf::from("/tmp"), + ); let schema = tool.parameters_schema(); assert!(schema["properties"].get("sound").is_some()); } + + #[test] + fn credentials_support_export_and_quoted_values() { + let tmp = TempDir::new().unwrap(); + let env_path = tmp.path().join(".env"); + fs::write( + &env_path, + "export PUSHOVER_TOKEN=\"quotedtoken\"\nPUSHOVER_USER_KEY='quoteduser'\n", + ) + .unwrap(); + + let tool = PushoverTool::new( + test_security(AutonomyLevel::Full, 100), + tmp.path().to_path_buf(), + ); + let result = tool.get_credentials(); + + assert!(result.is_ok()); + let (token, user_key) = result.unwrap(); + assert_eq!(token, "quotedtoken"); + assert_eq!(user_key, "quoteduser"); + } + + #[tokio::test] + async fn execute_blocks_readonly_mode() { + let tool = PushoverTool::new( + test_security(AutonomyLevel::ReadOnly, 100), + PathBuf::from("/tmp"), + ); + + let result = tool.execute(json!({"message": "hello"})).await.unwrap(); + assert!(!result.success); + assert!(result.error.unwrap().contains("read-only")); + } + + #[tokio::test] + async fn execute_blocks_rate_limit() { + let tool = PushoverTool::new(test_security(AutonomyLevel::Full, 0), PathBuf::from("/tmp")); + + let result = tool.execute(json!({"message": "hello"})).await.unwrap(); + assert!(!result.success); + assert!(result.error.unwrap().contains("rate limit")); + } + + #[tokio::test] + async fn execute_rejects_priority_out_of_range() { + let tool = PushoverTool::new( + test_security(AutonomyLevel::Full, 100), + PathBuf::from("/tmp"), + ); + + let result = tool + .execute(json!({"message": "hello", "priority": 5})) + .await + .unwrap(); + + assert!(!result.success); + assert!(result.error.unwrap().contains("-2..=2")); + } }