fix(tools): harden pushover security and validation

.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

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]

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<SecurityPolicy>,
     workspace_dir: PathBuf,
 }
 
 impl PushoverTool {
-    pub fn new(workspace_dir: PathBuf) -> Self {
+    pub fn new(security: Arc<SecurityPolicy>, 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<ToolResult> {
+        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,10 +175,8 @@ impl Tool for PushoverTool {
         }
 
         if let Some(priority) = priority {
-            if priority >= -2 && priority <= 2 {
             form = form.text("priority", priority.to_string());
         }
-        }
 
         if let Some(sound) = sound {
             form = form.text("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::<serde_json::Value>(&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<SecurityPolicy> {
+        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"));
+    }
 }