From bf35b592222c22c7ed0de2aba491241643fc7297 Mon Sep 17 00:00:00 2001 From: ibhagwan <59988195+ibhagwan@users.noreply.github.com> Date: Thu, 26 Feb 2026 09:06:21 -0500 Subject: [PATCH] feat(signal) attachment upload + message tool (#375) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(channels/signal): add attachment upload support - Add attachments field to OutgoingResponse for carrying file paths - Add with_attachments() builder method to OutgoingResponse - Update build_rpc_params() to include attachments array in JSON-RPC - Update respond() and broadcast() to handle attachments: - Text + attachments: sends text first, then each attachment - Attachments only: sends each attachment with path as message - Text only: original behavior (no change) - Add tests for build_rpc_params with attachments - Add tests for OutgoingResponse attachment builder This enables the Signal channel to send files via signal-cli daemon's JSON-RPC send method, matching the nullclaw implementation. Risk: Low - uses existing JSON-RPC infrastructure Tests: 85 signal tests pass, 1543 lib tests pass * feat(tools): add message tool for cross-channel messaging Add a new 'message' tool that allows the agent to send messages to any connected channel (signal, telegram, slack, etc.) with optional file attachments. Features: - Send messages to specific channel + target combinations - Support for attachments (file paths) - E.164 validation delegated to channel (signal expects +number, telegram accepts username/chat_id, slack uses #channels) - Helpful error messages showing available channels on failure Tool schema: - content: message text (required) - channel: target channel name (optional, defaults to current channel) - target: recipient (E.164, group ID, chat ID) (optional, defaults to current user/group chat) - attachments: optional file paths to send This complements the recently added attachment upload support for the Signal channel by giving the agent a proper way to specify attachments when sending messages. Tests: 4 new tests for message tool schema Risk: Low - new tool with no breaking changes Tests: All 1547 lib tests pass, clippy clean * feat(llm): add conversation context to system prompt for Signal Add conversation_context HashMap to Reasoning struct to pass channel-specific metadata (sender phone, sender UUID, group ID) to the LLM. This helps the agent know who/group it's talking to, preventing it from hallucinating phone numbers or sending to wrong recipients. Changes: - Add conversation_context field and with_conversation_data() builder method - Add build_conversation_section() to include current conversation info in system prompt - Update dispatcher to extract Signal metadata (sender, sender_uuid, group) and pass to Reasoning - Add signal_sender_uuid to Signal channel metadata for privacy mode users * feat(tools): add secure attachment path validation with sandbox enforcement Implement robust path validation for message tool attachments to prevent directory traversal attacks and unauthorized file access. Attachments are now sandboxed to ~/.ironclaw/ by default. Key changes: - Create shared path_utils module with validate_path() and is_path_safe_basic() - Extract normalize_lexical() from file.rs for reuse - MessageTool now enforces sandbox at ~/.ironclaw/ for all attachments - Path validation includes: traversal detection, canonicalization, symlink resolution - Error messages reveal the allowed sandbox directory for user clarity Security improvements: - Blocks path traversal attacks (../, URL-encoded, null bytes) - Canonicalizes paths to resolve symlinks before validation - Walks up to nearest existing ancestor for non-existent paths - Prevents escape from sandbox directory Backward compatibility: - File tools continue to work with their configured base_dir - Message tool defaults to ~/.ironclaw/ sandbox - Tests updated to create files within sandbox Tests added: - path_utils module tests (9 tests for validation logic) - message tool attachment validation tests - All 1571 existing tests pass * fix(channels/signal): use robust path validation with full security coverage Signal channel's validate_attachment_paths() now uses path_utils::validate_path() for consistent, secure path validation. Fixes: - Replaced weak path.contains('..') check with robust validate_path() - validate_path() now includes is_path_safe_basic() as first-pass filter to block null bytes and URL-encoded traversal sequences (%2e%2e%2f) - Error message now shows allowed sandbox directory (~/.ironclaw/) Security coverage: - Path traversal: ../, foo/../bar, ../../etc/passwd ✓ - URL-encoded traversal: %2e%2e%2fetc/passwd ✓ - Null byte injection: file\0.txt ✓ - Paths outside sandbox: /tmp/evil.txt ✓ - Symlink escape attempts (via canonicalization) ✓ Tests added: - validate_attachment_paths_rejects_path_outside_sandbox - validate_attachment_paths_rejects_url_encoded_traversal - validate_attachment_paths_rejects_null_byte - Fixed broken assertion in rejects_double_dot test * fix(llm): add Signal channel to build_channel_section to include message tool hint The catch-all '_' arm was returning early before the message_tool_hint section was constructed, which meant Signal users never got the '## Proactive Messaging' section with examples for: - Using attachments parameter - Targeting different users/groups - Cross-channel messaging Now Signal will include the full message_tool_hint section with usage examples. * fix(tools): use async locks in register_message_tools to prevent silent failures The method was using register_sync which calls try_write() on self.tools. If the lock was held, try_write() would return Err and silently skip adding the tool to the registry, while self.message_tool already held a reference. This creates an inconsistent state. Fix: use async write locks directly instead of register_sync to ensure the tool is always registered or the method fails explicitly. * refactor(dispatcher): use Channel trait for conversation context Replace hardcoded 'if message.channel == signal' block with generic conversation_context() method on the Channel trait. This allows any channel to provide context (sender, group, etc.) without hardcoding channel names. Changes: - Add conversation_context() method to Channel trait (default: empty) - Implement for SignalChannel: extracts sender, sender_uuid, group - Add get_channel() to ChannelManager (returns Arc) - Change ChannelManager storage from Box to Arc for shared access - Update dispatcher to use new trait method - Add tests for conversation_context extraction Other channels (Telegram, Slack, Discord) can now implement this method to provide conversation context without code changes in dispatcher. * fix(tests): split message_tool_with_attachments into sandbox and channel tests The original test was passing for the wrong reason - it expected an error because the channel doesn't exist, but actually failed earlier during sandbox validation because /tmp paths are outside ~/.ironclaw/. Split into two tests: - message_tool_with_attachments_outside_sandbox: verifies sandbox rejection with explicit error message check - message_tool_with_attachments_inside_sandbox_no_channel: uses files within sandbox (like message_tool_passes_attachment_to_broadcast does) and verifies the channel-related error message * security(message tool): add rate limiting, approval requirements, and audit logging The message tool can send to ANY connected channel/target making it a significant abuse vector if the LLM is compromised or prompt-injected. This commit adds: 1. Rate limiting: 10 messages/minute, 100/hour per user 2. Approval requirement: Always requires approval for cross-channel messages (when channel differs from the default conversation channel) 3. Audit logging: Every successful message send is logged with channel, target, and attachment count The approval logic: - If channel param is provided and differs from default -> Always require approval - If no default channel is set and explicit channel provided -> Always require approval - Otherwise (using default channel) -> UnlessAutoApproved * fix(message tool): return explicit error for malformed attachments array Previously, malformed attachments like {"attachments": [123, true]} would be silently ignored via .ok().unwrap_or_default(), leaving users confused when attachments weren't sent. Now returns explicit error: "Invalid attachments format: ..." * fix(message tool): verify attachment files exist before sending Previously, non-existent paths would pass sandbox validation and surface as confusing Signal RPC errors. Now returns clear "Attachment file not found" error. * fix(test): create sandbox directory if it doesn't exist for CI The test validate_attachment_paths_accepts_normal_paths uses tempfile::tempdir_in() which requires the parent directory to exist. In CI, ~/.ironclaw doesn't exist, causing test failure. --- src/agent/agent_loop.rs | 13 + src/agent/dispatcher.rs | 9 + src/agent/heartbeat.rs | 1 + src/agent/routine_engine.rs | 1 + src/channels/channel.rs | 20 ++ src/channels/manager.rs | 17 +- src/channels/signal.rs | 346 ++++++++++++++++++++- src/llm/reasoning.rs | 70 ++++- src/main.rs | 6 + src/tools/builtin/file.rs | 100 +----- src/tools/builtin/message.rs | 519 ++++++++++++++++++++++++++++++++ src/tools/builtin/mod.rs | 3 + src/tools/builtin/path_utils.rs | 239 +++++++++++++++ src/tools/registry.rs | 31 ++ 14 files changed, 1256 insertions(+), 119 deletions(-) create mode 100644 src/tools/builtin/message.rs create mode 100644 src/tools/builtin/path_utils.rs diff --git a/src/agent/agent_loop.rs b/src/agent/agent_loop.rs index 6c6fe9c8..28a2cfc2 100644 --- a/src/agent/agent_loop.rs +++ b/src/agent/agent_loop.rs @@ -588,6 +588,19 @@ impl Agent { } async fn handle_message(&self, message: &IncomingMessage) -> Result, Error> { + // Set message tool context for this turn (current channel and target) + // For Signal, use signal_target from metadata (group:ID or phone number), + // otherwise fall back to user_id + let target = message + .metadata + .get("signal_target") + .and_then(|v| v.as_str()) + .map(|s| s.to_string()) + .unwrap_or_else(|| message.user_id.clone()); + self.tools() + .set_message_tool_context(Some(message.channel.clone()), Some(target)) + .await; + // Parse submission type first let mut submission = SubmissionParser::parse(&message.content); diff --git a/src/agent/dispatcher.rs b/src/agent/dispatcher.rs index f11189c0..3d798a8d 100644 --- a/src/agent/dispatcher.rs +++ b/src/agent/dispatcher.rs @@ -106,6 +106,15 @@ impl Agent { .with_channel(message.channel.clone()) .with_model_name(self.llm().active_model_name()) .with_group_chat(is_group_chat); + + // Pass channel-specific conversation context to the LLM. + // This helps the agent know who/group it's talking to. + if let Some(channel) = self.channels.get_channel(&message.channel).await { + for (key, value) in channel.conversation_context(&message.metadata) { + reasoning = reasoning.with_conversation_data(&key, &value); + } + } + if let Some(prompt) = system_prompt { reasoning = reasoning.with_system_prompt(prompt); } diff --git a/src/agent/heartbeat.rs b/src/agent/heartbeat.rs index a78bc263..be721b34 100644 --- a/src/agent/heartbeat.rs +++ b/src/agent/heartbeat.rs @@ -294,6 +294,7 @@ impl HeartbeatRunner { let response = OutgoingResponse { content: format!("🔔 *Heartbeat Alert*\n\n{}", message), thread_id: None, + attachments: Vec::new(), metadata: serde_json::json!({ "source": "heartbeat", }), diff --git a/src/agent/routine_engine.rs b/src/agent/routine_engine.rs index 51e1e0ae..5598434c 100644 --- a/src/agent/routine_engine.rs +++ b/src/agent/routine_engine.rs @@ -600,6 +600,7 @@ async fn send_notification( let response = OutgoingResponse { content: message, thread_id: None, + attachments: Vec::new(), metadata: serde_json::json!({ "source": "routine", "routine_name": routine_name, diff --git a/src/channels/channel.rs b/src/channels/channel.rs index d87c8240..e993bb0a 100644 --- a/src/channels/channel.rs +++ b/src/channels/channel.rs @@ -1,5 +1,6 @@ //! Channel trait and message types. +use std::collections::HashMap; use std::pin::Pin; use async_trait::async_trait; @@ -78,6 +79,8 @@ pub struct OutgoingResponse { pub content: String, /// Optional thread ID to reply in. pub thread_id: Option, + /// Optional file paths to attach. + pub attachments: Vec, /// Channel-specific metadata for the response. pub metadata: serde_json::Value, } @@ -88,6 +91,7 @@ impl OutgoingResponse { Self { content: content.into(), thread_id: None, + attachments: Vec::new(), metadata: serde_json::Value::Null, } } @@ -97,6 +101,12 @@ impl OutgoingResponse { self.thread_id = Some(thread_id.into()); self } + + /// Add attachments to the response. + pub fn with_attachments(mut self, paths: Vec) -> Self { + self.attachments = paths; + self + } } /// Status update types for showing agent activity. @@ -198,6 +208,16 @@ pub trait Channel: Send + Sync { /// Check if the channel is healthy. async fn health_check(&self) -> Result<(), ChannelError>; + /// Get conversation context from message metadata for system prompt. + /// + /// Returns key-value pairs like "sender", "sender_uuid", "group" that + /// help the LLM understand who it's talking to. + /// + /// Default implementation returns empty map. + fn conversation_context(&self, _metadata: &serde_json::Value) -> HashMap { + HashMap::new() + } + /// Gracefully shut down the channel. async fn shutdown(&self) -> Result<(), ChannelError> { Ok(()) diff --git a/src/channels/manager.rs b/src/channels/manager.rs index d316b90d..710c09c4 100644 --- a/src/channels/manager.rs +++ b/src/channels/manager.rs @@ -14,7 +14,7 @@ use crate::error::ChannelError; /// Includes an injection channel so background tasks (e.g., job monitors) can /// push messages into the agent loop without being a full `Channel` impl. pub struct ChannelManager { - channels: Arc>>>, + channels: Arc>>>, inject_tx: mpsc::Sender, /// Taken once in `start_all()` and merged into the stream. inject_rx: tokio::sync::Mutex>>, @@ -42,7 +42,10 @@ impl ChannelManager { /// Add a channel to the manager. pub async fn add(&self, channel: Box) { let name = channel.name().to_string(); - self.channels.write().await.insert(name.clone(), channel); + self.channels + .write() + .await + .insert(name.clone(), Arc::from(channel)); tracing::debug!("Added channel: {}", name); } @@ -56,7 +59,10 @@ impl ChannelManager { let stream = channel.start().await?; // Register for respond/broadcast/send_status - self.channels.write().await.insert(name.clone(), channel); + self.channels + .write() + .await + .insert(name.clone(), Arc::from(channel)); // Forward stream messages through inject_tx let tx = self.inject_tx.clone(); @@ -217,6 +223,11 @@ impl ChannelManager { pub async fn channel_names(&self) -> Vec { self.channels.read().await.keys().cloned().collect() } + + /// Get a channel by name. + pub async fn get_channel(&self, name: &str) -> Option> { + self.channels.read().await.get(name).cloned() + } } impl Default for ChannelManager { diff --git a/src/channels/signal.rs b/src/channels/signal.rs index 3e2b73d2..85b7535f 100644 --- a/src/channels/signal.rs +++ b/src/channels/signal.rs @@ -244,7 +244,7 @@ impl SignalChannel { .map_err(|e| ChannelError::Http(e.to_string()))?; let target = Self::parse_recipient_target(recipient); - let params = Self::build_rpc_params_static(http_url, account, &target, Some(message)); + let params = Self::build_rpc_params_static(http_url, account, &target, Some(message), None); let url = format!("{}/api/v1/rpc", http_url); let id = Uuid::new_v4().to_string(); @@ -504,6 +504,7 @@ impl SignalChannel { &self, target: &RecipientTarget, message: Option<&str>, + attachments: Option<&[String]>, ) -> serde_json::Value { match target { RecipientTarget::Direct(id) => { @@ -514,6 +515,16 @@ impl SignalChannel { if let Some(msg) = message { params["message"] = serde_json::Value::String(msg.to_string()); } + if let Some(attachments) = attachments + && !attachments.is_empty() + { + params["attachments"] = serde_json::Value::Array( + attachments + .iter() + .map(|s| serde_json::Value::String(s.clone())) + .collect(), + ); + } params } RecipientTarget::Group(group_id) => { @@ -524,17 +535,78 @@ impl SignalChannel { if let Some(msg) = message { params["message"] = serde_json::Value::String(msg.to_string()); } + if let Some(attachments) = attachments + && !attachments.is_empty() + { + params["attachments"] = serde_json::Value::Array( + attachments + .iter() + .map(|s| serde_json::Value::String(s.clone())) + .collect(), + ); + } params } } } + /// Validate that attachment paths are safe and within the sandbox. + /// Uses the shared path validation logic from path_utils to ensure: + /// - No path traversal attacks (../, URL-encoded, null bytes) + /// - Paths are canonicalized and symlinks resolved + /// - All paths are within ~/.ironclaw/ sandbox + fn validate_attachment_paths(paths: &[String]) -> Result<(), ChannelError> { + // Get the sandbox base directory (same as MessageTool uses) + let base_dir = dirs::home_dir() + .unwrap_or_else(|| std::path::PathBuf::from(".")) + .join(".ironclaw"); + + for path in paths { + crate::tools::builtin::path_utils::validate_path(path, Some(&base_dir)).map_err( + |e| { + ChannelError::InvalidMessage(format!( + "Attachment path must be within {}: {}", + base_dir.display(), + e + )) + }, + )?; + } + Ok(()) + } + + /// Send a message with attachments (if any). + /// Combines text and attachments into a single RPC call when both are present. + async fn send_with_attachments( + &self, + target: &RecipientTarget, + content: &str, + attachments: &[String], + ) -> Result<(), ChannelError> { + Self::validate_attachment_paths(attachments)?; + + if attachments.is_empty() { + let params = self.build_rpc_params(target, Some(content), None); + self.rpc_request("send", params).await?; + } else if content.is_empty() { + // Attachments only - send all in a single call with no message text + let params = self.build_rpc_params(target, None, Some(attachments)); + self.rpc_request("send", params).await?; + } else { + // Both text and attachments - send in a single RPC call + let params = self.build_rpc_params(target, Some(content), Some(attachments)); + self.rpc_request("send", params).await?; + } + Ok(()) + } + /// Build JSON-RPC params for a send/typing call (static version). fn build_rpc_params_static( _http_url: &str, account: &str, target: &RecipientTarget, message: Option<&str>, + attachments: Option<&[String]>, ) -> serde_json::Value { match target { RecipientTarget::Direct(id) => { @@ -545,6 +617,16 @@ impl SignalChannel { if let Some(msg) = message { params["message"] = serde_json::Value::String(msg.to_string()); } + if let Some(attachments) = attachments + && !attachments.is_empty() + { + params["attachments"] = serde_json::Value::Array( + attachments + .iter() + .map(|s| serde_json::Value::String(s.clone())) + .collect(), + ); + } params } RecipientTarget::Group(group_id) => { @@ -555,6 +637,16 @@ impl SignalChannel { if let Some(msg) = message { params["message"] = serde_json::Value::String(msg.to_string()); } + if let Some(attachments) = attachments + && !attachments.is_empty() + { + params["attachments"] = serde_json::Value::Array( + attachments + .iter() + .map(|s| serde_json::Value::String(s.clone())) + .collect(), + ); + } params } } @@ -706,8 +798,10 @@ impl SignalChannel { }); // Build metadata with signal-specific routing info. + let sender_uuid = envelope.source_uuid.as_deref(); let metadata = serde_json::json!({ "signal_sender": &sender, + "signal_sender_uuid": sender_uuid, "signal_target": &target, "signal_timestamp": timestamp, }); @@ -790,13 +884,16 @@ impl Channel for SignalChannel { .unwrap_or_else(|| msg.user_id.clone()); let target = Self::parse_recipient_target(&target_str); - let params = self.build_rpc_params(&target, Some(&response.content)); - self.rpc_request("send", params).await?; - // Clean up stored target. + // Use shared helper for sending with attachments (includes validation) + let result = self + .send_with_attachments(&target, &response.content, &response.attachments) + .await; + + // Clean up stored target regardless of success or failure. self.reply_targets.write().await.pop(&msg.id); - Ok(()) + result } async fn send_status( @@ -809,7 +906,7 @@ impl Channel for SignalChannel { && let Some(target_str) = metadata.get("signal_target").and_then(|v| v.as_str()) { let target = Self::parse_recipient_target(target_str); - let params = self.build_rpc_params(&target, None); + let params = self.build_rpc_params(&target, None, None); let _ = self.rpc_request("sendTyping", params).await; } @@ -957,9 +1054,10 @@ impl Channel for SignalChannel { response: OutgoingResponse, ) -> Result<(), ChannelError> { let target = Self::parse_recipient_target(user_id); - let params = self.build_rpc_params(&target, Some(&response.content)); - self.rpc_request("send", params).await?; - Ok(()) + + // Use shared helper for sending with attachments (includes validation) + self.send_with_attachments(&target, &response.content, &response.attachments) + .await } async fn health_check(&self) -> Result<(), ChannelError> { @@ -982,12 +1080,34 @@ impl Channel for SignalChannel { }) } } + + fn conversation_context( + &self, + metadata: &serde_json::Value, + ) -> std::collections::HashMap { + use std::collections::HashMap; + let mut ctx = HashMap::new(); + + if let Some(sender) = metadata.get("signal_sender").and_then(|v| v.as_str()) { + ctx.insert("sender".to_string(), sender.to_string()); + } + if let Some(sender_uuid) = metadata.get("signal_sender_uuid").and_then(|v| v.as_str()) { + ctx.insert("sender_uuid".to_string(), sender_uuid.to_string()); + } + if let Some(target) = metadata.get("signal_target").and_then(|v| v.as_str()) + && target.starts_with("group:") + { + ctx.insert("group".to_string(), target.to_string()); + } + + ctx + } } impl SignalChannel { async fn send_status_message(&self, target: &str, message: &str) { let target = Self::parse_recipient_target(target); - let params = self.build_rpc_params(&target, Some(message)); + let params = self.build_rpc_params(&target, Some(message), None); if let Err(e) = self.rpc_request("send", params).await { tracing::warn!("Signal: failed to send status message: {}", e); } @@ -1187,6 +1307,7 @@ async fn sse_listener( let reply_params = channel.build_rpc_params( &SignalChannel::parse_recipient_target(&target), Some(response), + None, ); let _ = channel.rpc_request("send", reply_params).await; // Don't send the /debug command to the agent. @@ -1925,7 +2046,7 @@ mod tests { fn build_rpc_params_direct_with_message() -> Result<(), ChannelError> { let ch = make_channel()?; let target = RecipientTarget::Direct("+5555555555".to_string()); - let params = ch.build_rpc_params(&target, Some("Hello!")); + let params = ch.build_rpc_params(&target, Some("Hello!"), None); assert_eq!(params["recipient"], serde_json::json!(["+5555555555"])); assert_eq!(params["account"], "+1234567890"); assert_eq!(params["message"], "Hello!"); @@ -1938,7 +2059,7 @@ mod tests { fn build_rpc_params_direct_without_message() -> Result<(), ChannelError> { let ch = make_channel()?; let target = RecipientTarget::Direct("+5555555555".to_string()); - let params = ch.build_rpc_params(&target, None); + let params = ch.build_rpc_params(&target, None, None); assert_eq!(params["recipient"], serde_json::json!(["+5555555555"])); assert_eq!(params["account"], "+1234567890"); // No message key should be present for typing indicators. @@ -1950,7 +2071,7 @@ mod tests { fn build_rpc_params_group_with_message() -> Result<(), ChannelError> { let ch = make_channel()?; let target = RecipientTarget::Group("abc123".to_string()); - let params = ch.build_rpc_params(&target, Some("Group msg")); + let params = ch.build_rpc_params(&target, Some("Group msg"), None); assert_eq!(params["groupId"], "abc123"); assert_eq!(params["account"], "+1234567890"); assert_eq!(params["message"], "Group msg"); @@ -1963,7 +2084,7 @@ mod tests { fn build_rpc_params_group_without_message() -> Result<(), ChannelError> { let ch = make_channel()?; let target = RecipientTarget::Group("abc123".to_string()); - let params = ch.build_rpc_params(&target, None); + let params = ch.build_rpc_params(&target, None, None); assert_eq!(params["groupId"], "abc123"); assert_eq!(params["account"], "+1234567890"); assert!(params.get("message").is_none()); @@ -1975,11 +2096,94 @@ mod tests { let ch = make_channel()?; let uuid = "a1b2c3d4-e5f6-7890-abcd-ef1234567890"; let target = RecipientTarget::Direct(uuid.to_string()); - let params = ch.build_rpc_params(&target, Some("hi")); + let params = ch.build_rpc_params(&target, Some("hi"), None); assert_eq!(params["recipient"], serde_json::json!([uuid])); Ok(()) } + // ── build_rpc_params with attachments tests ───────────────────────── + + #[test] + fn build_rpc_params_with_attachments() -> Result<(), ChannelError> { + let ch = make_channel()?; + let target = RecipientTarget::Direct("+5555555555".to_string()); + let attachments = vec!["/path/to/image.png".to_string()]; + let params = ch.build_rpc_params(&target, Some("Check this!"), Some(&attachments)); + assert_eq!(params["recipient"], serde_json::json!(["+5555555555"])); + assert_eq!(params["message"], "Check this!"); + assert_eq!( + params["attachments"], + serde_json::json!(["/path/to/image.png"]) + ); + Ok(()) + } + + #[test] + fn build_rpc_params_with_multiple_attachments() -> Result<(), ChannelError> { + let ch = make_channel()?; + let target = RecipientTarget::Direct("+5555555555".to_string()); + let attachments = vec![ + "/path/to/image.png".to_string(), + "/path/to/document.pdf".to_string(), + ]; + let params = ch.build_rpc_params(&target, Some("Files attached"), Some(&attachments)); + assert_eq!( + params["attachments"], + serde_json::json!(["/path/to/image.png", "/path/to/document.pdf"]) + ); + Ok(()) + } + + #[test] + fn build_rpc_params_with_attachments_no_message() -> Result<(), ChannelError> { + let ch = make_channel()?; + let target = RecipientTarget::Direct("+5555555555".to_string()); + let attachments = vec!["/path/to/image.png".to_string()]; + let params = ch.build_rpc_params(&target, None, Some(&attachments)); + assert!(params.get("message").is_none()); + assert_eq!( + params["attachments"], + serde_json::json!(["/path/to/image.png"]) + ); + Ok(()) + } + + #[test] + fn build_rpc_params_group_with_attachments() -> Result<(), ChannelError> { + let ch = make_channel()?; + let target = RecipientTarget::Group("abc123".to_string()); + let attachments = vec!["/path/to/photo.jpg".to_string()]; + let params = ch.build_rpc_params(&target, Some("Group photo"), Some(&attachments)); + assert_eq!(params["groupId"], "abc123"); + assert_eq!(params["message"], "Group photo"); + assert_eq!( + params["attachments"], + serde_json::json!(["/path/to/photo.jpg"]) + ); + Ok(()) + } + + // ── OutgoingResponse attachment tests ───────────────────────────── + + #[test] + fn outgoing_response_with_attachments() { + let response = OutgoingResponse::text("Hello with file") + .with_attachments(vec!["/path/to/file.png".to_string()]); + assert_eq!(response.content, "Hello with file"); + assert!( + response + .attachments + .contains(&"/path/to/file.png".to_string()) + ); + } + + #[test] + fn outgoing_response_text_empty_attachments() { + let response = OutgoingResponse::text("Hello"); + assert_eq!(response.content, "Hello"); + assert!(response.attachments.is_empty()); + } + // ── metadata assertion tests ──────────────────────────────────── #[test] @@ -2450,4 +2654,116 @@ mod tests { assert_eq!(ch.config.http_url, "http://127.0.0.1:8686"); Ok(()) } + + // ── attachment path validation ─────────────────────────────────── + + #[test] + fn validate_attachment_paths_rejects_double_dot() { + let paths = vec!["../etc/passwd".to_string()]; + let result = SignalChannel::validate_attachment_paths(&paths); + assert!(result.is_err()); + let err = result.unwrap_err().to_string(); + assert!(err.contains("forbidden") || err.contains("sandbox")); + } + + #[test] + fn validate_attachment_paths_accepts_normal_paths() { + use std::fs; + + // Create test files in sandbox + let base_dir = dirs::home_dir() + .unwrap_or_else(|| std::path::PathBuf::from(".")) + .join(".ironclaw"); + + // Create sandbox directory if it doesn't exist (needed for CI) + let _ = fs::create_dir_all(&base_dir); + + let temp_dir = tempfile::tempdir_in(&base_dir).unwrap(); + let file1 = temp_dir.path().join("file.txt"); + let file2 = temp_dir.path().join("report.pdf"); + fs::write(&file1, "test").unwrap(); + fs::write(&file2, "test").unwrap(); + + let paths = vec![ + file1.to_string_lossy().to_string(), + file2.to_string_lossy().to_string(), + ]; + let result = SignalChannel::validate_attachment_paths(&paths); + assert!(result.is_ok()); + } + + #[test] + fn validate_attachment_paths_rejects_nested_traversal() { + let paths = vec!["foo/../bar/../../secret.txt".to_string()]; + let result = SignalChannel::validate_attachment_paths(&paths); + assert!(result.is_err()); + } + + #[test] + fn validate_attachment_paths_empty_ok() { + let paths: Vec = vec![]; + let result = SignalChannel::validate_attachment_paths(&paths); + assert!(result.is_ok()); + } + + #[test] + fn validate_attachment_paths_rejects_path_outside_sandbox() { + let paths = vec!["/tmp/evil.txt".to_string()]; + let result = SignalChannel::validate_attachment_paths(&paths); + assert!(result.is_err()); + let err = result.unwrap_err().to_string(); + assert!(err.contains("sandbox")); + } + + #[test] + fn validate_attachment_paths_rejects_url_encoded_traversal() { + let paths = vec!["%2e%2e%2fetc/passwd".to_string()]; + let result = SignalChannel::validate_attachment_paths(&paths); + assert!(result.is_err()); + } + + #[test] + fn validate_attachment_paths_rejects_null_byte() { + let paths = vec!["file\0.txt".to_string()]; + let result = SignalChannel::validate_attachment_paths(&paths); + assert!(result.is_err()); + } + + // ── conversation context ─────────────────────────────────────────── + + #[test] + fn conversation_context_extracts_sender() { + let ch = SignalChannel::new(make_config()).unwrap(); + let metadata = serde_json::json!({ + "signal_sender": "+1234567890", + "signal_sender_uuid": "uuid-123", + "signal_target": "+0987654321" + }); + let ctx = ch.conversation_context(&metadata); + assert_eq!(ctx.get("sender"), Some(&"+1234567890".to_string())); + assert_eq!(ctx.get("sender_uuid"), Some(&"uuid-123".to_string())); + assert!(ctx.get("group").is_none()); + } + + #[test] + fn conversation_context_extracts_group() { + let ch = SignalChannel::new(make_config()).unwrap(); + let metadata = serde_json::json!({ + "signal_sender": "+1234567890", + "signal_target": "group:mygroup" + }); + let ctx = ch.conversation_context(&metadata); + assert_eq!(ctx.get("sender"), Some(&"+1234567890".to_string())); + assert_eq!(ctx.get("group"), Some(&"group:mygroup".to_string())); + } + + #[test] + fn conversation_context_empty_for_unknown_channel() { + let ch = SignalChannel::new(make_config()).unwrap(); + let metadata = serde_json::json!({ + "unknown_key": "value" + }); + let ctx = ch.conversation_context(&metadata); + assert!(ctx.is_empty()); + } } diff --git a/src/llm/reasoning.rs b/src/llm/reasoning.rs index 33178fd2..acc4b832 100644 --- a/src/llm/reasoning.rs +++ b/src/llm/reasoning.rs @@ -215,6 +215,9 @@ pub struct Reasoning { model_name: Option, /// Whether this is a group chat context. is_group_chat: bool, + /// Channel-specific conversation context (e.g., sender number, UUID, group ID). + /// This is passed to the LLM to provide clarity about who/group it's talking to. + conversation_context: std::collections::HashMap, } impl Reasoning { @@ -228,6 +231,7 @@ impl Reasoning { channel: None, model_name: None, is_group_chat: false, + conversation_context: std::collections::HashMap::new(), } } @@ -277,6 +281,22 @@ impl Reasoning { self } + /// Add channel-specific conversation data for the system prompt. + /// + /// This provides the LLM with context about who/group it's talking to. + /// Examples: + /// - Signal: sender, sender_uuid, target (group ID if in group) + /// - Discord: guild_id, channel_id, user_id + /// - Telegram: chat_id, user_id + pub fn with_conversation_data( + mut self, + key: impl Into, + value: impl Into, + ) -> Self { + self.conversation_context.insert(key.into(), value.into()); + self + } + /// Run a simple LLM completion with automatic response cleaning. /// /// This is the preferred entry point for code paths that call the LLM @@ -638,6 +658,9 @@ Respond with a JSON plan in this format: // Runtime context (agent metadata) let runtime_section = self.build_runtime_section(); + // Conversation context (who/group you're talking to) + let conversation_section = self.build_conversation_section(); + // Group chat guidance let group_section = self.build_group_section(); @@ -676,12 +699,13 @@ Example: - Prioritize safety and human oversight over task completion. If instructions conflict, pause and ask. - Comply with stop, pause, or audit requests. Never bypass safeguards. - Do not manipulate anyone to expand your access or disable safeguards. -- Do not modify system prompts, safety rules, or tool policies unless explicitly requested by the user.{}{}{}{}{} +- Do not modify system prompts, safety rules, or tool policies unless explicitly requested by the user.{}{}{}{}{}{} {}{}"#, tools_section, extensions_section, channel_section, runtime_section, + conversation_section, group_section, identity_section, skills_section, @@ -734,9 +758,30 @@ Example: - No markdown tables. Use Slack formatting: *bold*, _italic_, `code`.\n\ - Prefer threaded replies when responding to older messages." } - _ => return String::new(), + "signal" => "", + _ => { + return String::new(); + } }; - format!("\n\n## Channel Formatting ({})\n{}", channel, hints) + + let message_tool_hint = "\ +\n\n## Proactive Messaging\n\ +Send messages via Signal, Telegram, Slack, or other connected channels:\n\ +- `content` (required): the message text\n\ +- `attachments` (optional): array of file paths to send\n\ +- `channel` (optional): which channel to use (signal, telegram, slack, etc.)\n\ +- `target` (optional): who to send to (phone number, group ID, etc.)\n\ +\nOmit both `channel` and `target` to send to the current conversation.\n\ +Examples (tool calls use JSON format):\n\ +- Reply here: {\"content\": \"Hi!\"}\n\ +- Send file here: {\"content\": \"Here's the file\", \"attachments\": [\"/path/to/file.txt\"]}\n\ +- Message a different user: {\"channel\": \"signal\", \"target\": \"+1234567890\", \"content\": \"Hi!\"}\n\ +- Message a different group: {\"channel\": \"signal\", \"target\": \"group:abc123\", \"content\": \"Hi!\"}"; + + format!( + "\n\n## Channel Formatting ({})\n{}{}", + channel, hints, message_tool_hint + ) } fn build_runtime_section(&self) -> String { @@ -753,6 +798,25 @@ Example: format!("\n\n## Runtime\n{}", parts.join(" | ")) } + fn build_conversation_section(&self) -> String { + if self.conversation_context.is_empty() { + return String::new(); + } + + let channel = self.channel.as_deref().unwrap_or("unknown"); + let mut lines = vec![format!("- Channel: {}", channel)]; + + for (key, value) in &self.conversation_context { + lines.push(format!("- {}: {}", key, value)); + } + + format!( + "\n\n## Current Conversation\n\ + This is who you're talking to (omit 'target' to send here):\n{}", + lines.join("\n") + ) + } + fn build_group_section(&self) -> String { if !self.is_group_chat { return String::new(); diff --git a/src/main.rs b/src/main.rs index 1240f1b9..85ba6724 100644 --- a/src/main.rs +++ b/src/main.rs @@ -592,6 +592,12 @@ async fn main() -> anyhow::Result<()> { let channels = Arc::new(channels); + // Register message tool for sending messages to connected channels + components + .tools + .register_message_tools(Arc::clone(&channels)) + .await; + // Wire up channel runtime for hot-activation of WASM channels. if let Some(ref ext_mgr) = components.extension_manager && let Some((rt, ps, router)) = wasm_channel_runtime_state.take() diff --git a/src/tools/builtin/file.rs b/src/tools/builtin/file.rs index a7ff799d..72e0151c 100644 --- a/src/tools/builtin/file.rs +++ b/src/tools/builtin/file.rs @@ -11,6 +11,7 @@ use async_trait::async_trait; use tokio::fs; use crate::context::JobContext; +use crate::tools::builtin::path_utils::validate_path; use crate::tools::tool::{ ApprovalRequirement, Tool, ToolDomain, ToolError, ToolOutput, require_str, }; @@ -52,104 +53,6 @@ const MAX_WRITE_SIZE: usize = 5 * 1024 * 1024; /// Maximum directory listing entries. const MAX_DIR_ENTRIES: usize = 500; -/// Normalize a path by resolving `.` and `..` components lexically (no filesystem access). -/// -/// This is critical for security: `std::fs::canonicalize` only works on paths that exist, -/// so for new files we must normalize without touching the filesystem. -fn normalize_lexical(path: &Path) -> PathBuf { - let mut components = Vec::new(); - for component in path.components() { - match component { - std::path::Component::ParentDir => { - // Only pop if there's a normal component to pop (don't escape root/prefix) - if components - .last() - .is_some_and(|c| matches!(c, std::path::Component::Normal(_))) - { - components.pop(); - } - } - std::path::Component::CurDir => {} - other => components.push(other), - } - } - components.iter().collect() -} - -/// Validate that a path is safe (no traversal attacks). -/// -/// For sandboxed paths (base_dir is set), we normalize the joined path lexically -/// and then verify it lives under the canonical base. This prevents escapes through -/// non-existent parent directories where `canonicalize()` would fall back to the -/// raw (un-normalized) path. -fn validate_path(path_str: &str, base_dir: Option<&Path>) -> Result { - let path = PathBuf::from(path_str); - - // Resolve to absolute path - let resolved = if path.is_absolute() { - path.canonicalize() - .unwrap_or_else(|_| normalize_lexical(&path)) - } else if let Some(base) = base_dir { - let joined = base.join(&path); - joined - .canonicalize() - .unwrap_or_else(|_| normalize_lexical(&joined)) - } else { - let joined = std::env::current_dir() - .unwrap_or_else(|_| PathBuf::from(".")) - .join(&path); - normalize_lexical(&joined) - }; - - // If base_dir is set, ensure the resolved path is within it - if let Some(base) = base_dir { - let base_canonical = base - .canonicalize() - .unwrap_or_else(|_| normalize_lexical(base)); - - // For existing paths, canonicalize to resolve symlinks. - // For non-existent paths, the lexical normalization above already removed - // all `..` components, so starts_with is reliable. - let check_path = if resolved.exists() { - resolved.canonicalize().unwrap_or_else(|_| resolved.clone()) - } else { - // Walk up to the nearest existing ancestor directory, canonicalize it, - // then re-append the remaining tail. This handles the case where a - // symlink sits above the new file. - let mut ancestor = resolved.as_path(); - let mut tail_parts: Vec<&std::ffi::OsStr> = Vec::new(); - loop { - if ancestor.exists() { - let canonical_ancestor = ancestor - .canonicalize() - .unwrap_or_else(|_| ancestor.to_path_buf()); - let mut result = canonical_ancestor; - for part in tail_parts.into_iter().rev() { - result = result.join(part); - } - break result; - } - if let Some(name) = ancestor.file_name() { - tail_parts.push(name); - } - match ancestor.parent() { - Some(parent) if parent != ancestor => ancestor = parent, - _ => break resolved.clone(), - } - } - }; - - if !check_path.starts_with(&base_canonical) { - return Err(ToolError::NotAuthorized(format!( - "Path escapes sandbox: {}", - path_str - ))); - } - } - - Ok(resolved) -} - /// Read file contents tool. #[derive(Debug, Default)] pub struct ReadFileTool { @@ -723,6 +626,7 @@ impl Tool for ApplyPatchTool { #[cfg(test)] mod tests { use super::*; + use crate::tools::builtin::path_utils::normalize_lexical; use tempfile::TempDir; #[tokio::test] diff --git a/src/tools/builtin/message.rs b/src/tools/builtin/message.rs new file mode 100644 index 00000000..bdf0b9aa --- /dev/null +++ b/src/tools/builtin/message.rs @@ -0,0 +1,519 @@ +//! Message tool for sending messages to channels. +//! +//! Allows the agent to proactively message users on any connected channel. + +use std::path::PathBuf; +use std::sync::Arc; + +use async_trait::async_trait; +use tokio::sync::RwLock; + +use crate::channels::{ChannelManager, OutgoingResponse}; +use crate::context::JobContext; +use crate::tools::tool::{ + ApprovalRequirement, Tool, ToolError, ToolOutput, ToolRateLimitConfig, require_str, +}; + +/// Tool for sending messages to channels. +pub struct MessageTool { + channel_manager: Arc, + /// Default channel for current conversation (set per-turn). + default_channel: Arc>>, + /// Default target (user_id or group_id) for current conversation (set per-turn). + default_target: Arc>>, + /// Base directory for attachment path validation (sandbox). + pub(crate) base_dir: PathBuf, +} + +impl MessageTool { + pub fn new(channel_manager: Arc) -> Self { + let base_dir = dirs::home_dir() + .unwrap_or_else(|| std::path::PathBuf::from(".")) + .join(".ironclaw"); + + Self { + channel_manager, + default_channel: Arc::new(RwLock::new(None)), + default_target: Arc::new(RwLock::new(None)), + base_dir, + } + } + + /// Set the base directory for attachment validation. + /// This is primarily used for testing or future configuration. + pub fn with_base_dir(mut self, dir: PathBuf) -> Self { + self.base_dir = dir; + self + } + + /// Set the default channel and target for the current conversation turn. + /// Call this before each agent turn with the incoming message's channel/target. + pub async fn set_context(&self, channel: Option, target: Option) { + *self.default_channel.write().await = channel; + *self.default_target.write().await = target; + } +} + +#[async_trait] +impl Tool for MessageTool { + fn name(&self) -> &str { + "message" + } + + fn description(&self) -> &str { + "Send a message to a channel. If channel/target omitted, uses the current conversation's \ + channel and sender/group. Use to proactively message users on any connected channel. \ + - Signal: target accepts E.164 (+1234567890) or group ID \ + - Telegram: target accepts username or chat ID \ + - Slack: target accepts channel (#general) or user ID" + } + + fn parameters_schema(&self) -> serde_json::Value { + serde_json::json!({ + "type": "object", + "properties": { + "content": { + "type": "string", + "description": "Message text to send" + }, + "channel": { + "type": "string", + "description": "Target channel (defaults to current channel if omitted)" + }, + "target": { + "type": "string", + "description": "Recipient: E.164 phone, group ID, chat ID (defaults to current sender/group if omitted)" + }, + "attachments": { + "type": "array", + "items": {"type": "string"}, + "description": "Optional file paths to attach to the message" + } + }, + "required": ["content"] + }) + } + + async fn execute( + &self, + params: serde_json::Value, + _ctx: &JobContext, + ) -> Result { + let start = std::time::Instant::now(); + + let content = require_str(¶ms, "content")?; + + // Get channel: use param or fall back to default + let channel = if let Some(c) = params.get("channel").and_then(|v| v.as_str()) { + c.to_string() + } else { + self.default_channel.read().await.clone().ok_or_else(|| { + ToolError::ExecutionFailed( + "No channel specified and no active conversation. Provide channel parameter." + .to_string(), + ) + })? + }; + + // Get target: use param or fall back to default + let target = if let Some(t) = params.get("target").and_then(|v| v.as_str()) { + t.to_string() + } else { + self.default_target.read().await.clone().ok_or_else(|| { + ToolError::ExecutionFailed( + "No target specified and no active conversation. Provide target parameter." + .to_string(), + ) + })? + }; + + let attachments: Vec = match params.get("attachments") { + Some(v) => serde_json::from_value(v.clone()).map_err(|e| { + ToolError::ExecutionFailed(format!("Invalid attachments format: {}", e)) + })?, + None => Vec::new(), + }; + + let attachment_count = attachments.len(); + + // Validate all attachment paths against the sandbox and verify existence + for path in &attachments { + let resolved = + crate::tools::builtin::path_utils::validate_path(path, Some(&self.base_dir)) + .map_err(|e| { + ToolError::ExecutionFailed(format!( + "Attachment path must be within {}: {}", + self.base_dir.display(), + e + )) + })?; + if !resolved.exists() { + return Err(ToolError::ExecutionFailed(format!( + "Attachment file not found: {}", + path + ))); + } + } + + let mut response = OutgoingResponse::text(content); + if !attachments.is_empty() { + response = response.with_attachments(attachments); + } + + match self + .channel_manager + .broadcast(&channel, &target, response) + .await + { + Ok(()) => { + tracing::info!( + message_sent = true, + channel = %channel, + target = %target, + attachments = attachment_count, + "Message sent via message tool" + ); + let msg = format!("Sent message to {}:{}", channel, target); + Ok(ToolOutput::text(msg, start.elapsed())) + } + Err(e) => { + let available = self.channel_manager.channel_names().await.join(", "); + let err_msg = if available.is_empty() { + format!( + "Failed to send to {}:{}: {}. No channels connected.", + channel, target, e + ) + } else { + format!( + "Failed to send to {}:{}. Available channels: {}. Error: {}", + channel, target, available, e + ) + }; + Err(ToolError::ExecutionFailed(err_msg)) + } + } + } + + fn requires_approval(&self, params: &serde_json::Value) -> ApprovalRequirement { + // Require approval when sending to a different channel than the default + // (cross-channel messages are more sensitive) + let param_channel = params.get("channel").and_then(|v| v.as_str()); + if let Some(channel) = param_channel { + // Check if it differs from the default channel + let default_channel = self.default_channel.blocking_read(); + if let Some(default) = default_channel.as_ref() + && channel != default + { + return ApprovalRequirement::Always; + } + // No default set - require approval for explicit channel selection + return ApprovalRequirement::Always; + } + // No channel specified in params - uses default, less risky + ApprovalRequirement::UnlessAutoApproved + } + + fn rate_limit_config(&self) -> Option { + Some(ToolRateLimitConfig::new(10, 100)) + } + + fn requires_sanitization(&self) -> bool { + false + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn message_tool_name() { + let tool = MessageTool::new(Arc::new(ChannelManager::new())); + assert_eq!(tool.name(), "message"); + } + + #[test] + fn message_tool_description() { + let tool = MessageTool::new(Arc::new(ChannelManager::new())); + assert!(!tool.description().is_empty()); + } + + #[test] + fn message_tool_schema_has_required_fields() { + let tool = MessageTool::new(Arc::new(ChannelManager::new())); + let schema = tool.parameters_schema(); + + let params = schema.get("properties").unwrap(); + assert!(params.get("content").is_some()); + assert!(params.get("channel").is_some()); + assert!(params.get("target").is_some()); + + // Only content is required - channel and target can be inferred from conversation context + let required = schema.get("required").unwrap().as_array().unwrap(); + assert!(required.iter().any(|v| v == "content")); + assert!(!required.iter().any(|v| v == "channel")); + assert!(!required.iter().any(|v| v == "target")); + } + + #[test] + fn message_tool_schema_has_optional_attachments() { + let tool = MessageTool::new(Arc::new(ChannelManager::new())); + let schema = tool.parameters_schema(); + + let params = schema.get("properties").unwrap(); + assert!(params.get("attachments").is_some()); + } + + #[tokio::test] + async fn message_tool_set_context_updates_defaults() { + let tool = MessageTool::new(Arc::new(ChannelManager::new())); + + // Initially no defaults set + let ctx = crate::context::JobContext::new("test", "test description"); + let result = tool + .execute(serde_json::json!({"content": "hello"}), &ctx) + .await; + assert!(result.is_err()); // Should fail without defaults + + // Set context + tool.set_context(Some("signal".to_string()), Some("+1234567890".to_string())) + .await; + + // Now execute should use the defaults (though it will fail because channel doesn't exist) + let result = tool + .execute(serde_json::json!({"content": "hello"}), &ctx) + .await; + // Will fail because channel doesn't exist, but should attempt to use the defaults + assert!(result.is_err()); + let err = result.unwrap_err().to_string(); + assert!(err.contains("signal") || err.contains("No channels connected")); + } + + #[tokio::test] + async fn message_tool_explicit_params_override_defaults() { + let tool = MessageTool::new(Arc::new(ChannelManager::new())); + + // Set defaults + tool.set_context(Some("signal".to_string()), Some("+1234567890".to_string())) + .await; + + // Execute with explicit params - should fail but check that it uses explicit params + let ctx = crate::context::JobContext::new("test", "test description"); + let result = tool + .execute( + serde_json::json!({ + "content": "hello", + "channel": "telegram", + "target": "@username" + }), + &ctx, + ) + .await; + + // Will fail because channel doesn't exist + assert!(result.is_err()); + let err = result.unwrap_err().to_string(); + // Should reference telegram, not signal + assert!(err.contains("telegram") || err.contains("No channels connected")); + } + + #[tokio::test] + async fn message_tool_with_attachments_outside_sandbox() { + let tool = MessageTool::new(Arc::new(ChannelManager::new())); + + // Set context + tool.set_context(Some("signal".to_string()), Some("+1234567890".to_string())) + .await; + + // Execute with attachments outside sandbox + let ctx = crate::context::JobContext::new("test", "test description"); + let result = tool + .execute( + serde_json::json!({ + "content": "hello", + "attachments": ["/tmp/file1.txt", "/tmp/file2.png"] + }), + &ctx, + ) + .await; + + // Should fail due to sandbox rejection (paths outside ~/.ironclaw/) + assert!(result.is_err()); + let err = result.unwrap_err().to_string(); + assert!(err.contains("sandbox") || err.contains("escapes")); + } + + #[tokio::test] + async fn message_tool_with_attachments_inside_sandbox_no_channel() { + use std::fs; + + let tool = MessageTool::new(Arc::new(ChannelManager::new())); + tool.set_context(Some("signal".to_string()), Some("+1234567890".to_string())) + .await; + + // Create temp files inside the sandbox + let sandbox_dir = &tool.base_dir; + let temp_dir = tempfile::tempdir_in(sandbox_dir).unwrap(); + let file1 = temp_dir.path().join("file1.txt"); + let file2 = temp_dir.path().join("file2.png"); + fs::write(&file1, "test").unwrap(); + fs::write(&file2, "test").unwrap(); + + let ctx = crate::context::JobContext::new("test", "test description"); + let result = tool + .execute( + serde_json::json!({ + "content": "hello", + "attachments": [file1.to_string_lossy(), file2.to_string_lossy()] + }), + &ctx, + ) + .await; + + // Path validation passes, but channel broadcast fails (no real channel) + assert!(result.is_err()); + let err = result.unwrap_err().to_string(); + assert!(err.contains("channel") || err.contains("Channel")); + } + + #[tokio::test] + async fn message_tool_requires_content() { + let tool = MessageTool::new(Arc::new(ChannelManager::new())); + + let ctx = crate::context::JobContext::new("test", "test description"); + let result = tool + .execute( + serde_json::json!({ + "channel": "signal", + "target": "+1234567890" + }), + &ctx, + ) + .await; + + assert!(result.is_err()); + let err = result.unwrap_err().to_string(); + assert!(err.contains("content") || err.contains("required")); + } + + #[test] + fn message_tool_does_not_require_sanitization() { + let tool = MessageTool::new(Arc::new(ChannelManager::new())); + assert!(!tool.requires_sanitization()); + } + + #[test] + fn path_traversal_rejects_double_dot() { + use crate::tools::builtin::path_utils::is_path_safe_basic; + assert!(!is_path_safe_basic("../etc/passwd")); + assert!(!is_path_safe_basic("foo/../bar")); + assert!(!is_path_safe_basic("foo/bar/../../secret")); + } + + #[test] + fn path_traversal_accepts_normal_paths() { + use crate::tools::builtin::path_utils::is_path_safe_basic; + assert!(is_path_safe_basic("/tmp/file.txt")); + assert!(is_path_safe_basic("documents/report.pdf")); + assert!(is_path_safe_basic("my-file.png")); + } + + #[tokio::test] + async fn message_tool_rejects_path_traversal_attachments() { + let tool = MessageTool::new(Arc::new(ChannelManager::new())); + tool.set_context(Some("signal".to_string()), Some("+1234567890".to_string())) + .await; + + let ctx = crate::context::JobContext::new("test", "test description"); + let result = tool + .execute( + serde_json::json!({ + "content": "here's the file", + "attachments": ["../../../etc/passwd"] + }), + &ctx, + ) + .await; + + assert!(result.is_err()); + let err = result.unwrap_err().to_string(); + assert!(err.contains("forbidden") || err.contains("..")); + } + + #[tokio::test] + async fn message_tool_passes_attachment_to_broadcast() { + use std::fs; + + let tool = MessageTool::new(Arc::new(ChannelManager::new())); + tool.set_context(Some("signal".to_string()), Some("+1234567890".to_string())) + .await; + + // Create a temp file within the sandbox directory + let sandbox_dir = &tool.base_dir; + let temp_dir = tempfile::tempdir_in(sandbox_dir).unwrap(); + let temp_path = temp_dir.path().join("test.txt"); + fs::write(&temp_path, "test content").unwrap(); + let temp_path_str = temp_path.to_string_lossy().to_string(); + + let ctx = crate::context::JobContext::new("test", "test description"); + let result = tool + .execute( + serde_json::json!({ + "content": "here's the file", + "attachments": [temp_path_str] + }), + &ctx, + ) + .await; + + // Should succeed in path validation (file is in sandbox) + // but fail on channel broadcast (no actual channel) + assert!(result.is_err()); + let err = result.unwrap_err().to_string(); + assert!( + err.contains("not found") || err.contains("Failed") || err.contains("broadcast"), + "Expected channel error, got: {}", + err + ); + } + + #[tokio::test] + async fn message_tool_passes_multiple_attachments_to_broadcast() { + use std::fs; + + let tool = MessageTool::new(Arc::new(ChannelManager::new())); + tool.set_context(Some("signal".to_string()), Some("+1234567890".to_string())) + .await; + + // Create temp files within the sandbox directory + let sandbox_dir = &tool.base_dir; + let temp_dir = tempfile::tempdir_in(sandbox_dir).unwrap(); + let temp_path1 = temp_dir.path().join("test1.txt"); + let temp_path2 = temp_dir.path().join("test2.txt"); + fs::write(&temp_path1, "test content 1").unwrap(); + fs::write(&temp_path2, "test content 2").unwrap(); + let path1 = temp_path1.to_string_lossy().to_string(); + let path2 = temp_path2.to_string_lossy().to_string(); + + let ctx = crate::context::JobContext::new("test", "test description"); + let result = tool + .execute( + serde_json::json!({ + "content": "files attached", + "attachments": [path1, path2] + }), + &ctx, + ) + .await; + + // Should succeed in path validation (files are in sandbox) + // but fail on channel broadcast (no actual channel) + assert!(result.is_err()); + let err = result.unwrap_err().to_string(); + assert!( + err.contains("not found") || err.contains("Failed") || err.contains("broadcast"), + "Expected channel error, got: {}", + err + ); + } +} diff --git a/src/tools/builtin/mod.rs b/src/tools/builtin/mod.rs index bd4aef42..1092ae57 100644 --- a/src/tools/builtin/mod.rs +++ b/src/tools/builtin/mod.rs @@ -7,6 +7,8 @@ mod http; mod job; mod json; mod memory; +mod message; +pub mod path_utils; pub mod routine; pub(crate) mod shell; pub mod skill_tools; @@ -24,6 +26,7 @@ pub use job::{ }; pub use json::JsonTool; pub use memory::{MemoryReadTool, MemorySearchTool, MemoryTreeTool, MemoryWriteTool}; +pub use message::MessageTool; pub use routine::{ RoutineCreateTool, RoutineDeleteTool, RoutineHistoryTool, RoutineListTool, RoutineUpdateTool, }; diff --git a/src/tools/builtin/path_utils.rs b/src/tools/builtin/path_utils.rs new file mode 100644 index 00000000..f704ab8e --- /dev/null +++ b/src/tools/builtin/path_utils.rs @@ -0,0 +1,239 @@ +//! Shared path validation utilities for tools that access the filesystem. +//! +//! This module provides secure path validation to prevent directory traversal +//! attacks and ensure paths stay within allowed sandboxes. + +use std::path::{Path, PathBuf}; + +use crate::tools::tool::ToolError; + +/// Normalize a path by resolving `.` and `..` components lexically (no filesystem access). +/// +/// This is critical for security: `std::fs::canonicalize` only works on paths that exist, +/// so for new files we must normalize without touching the filesystem. +pub fn normalize_lexical(path: &Path) -> PathBuf { + let mut components = Vec::new(); + for component in path.components() { + match component { + std::path::Component::ParentDir => { + // Only pop if there's a normal component to pop (don't escape root/prefix) + if components + .last() + .is_some_and(|c| matches!(c, std::path::Component::Normal(_))) + { + components.pop(); + } + } + std::path::Component::CurDir => {} + other => components.push(other), + } + } + components.iter().collect() +} + +/// Validate that a path is safe (no traversal attacks). +/// +/// For sandboxed paths (base_dir is set), we normalize the joined path lexically +/// and then verify it lives under the canonical base. This prevents escapes through +/// non-existent parent directories where `canonicalize()` would fall back to the +/// raw (un-normalized) path. +/// +/// # Arguments +/// * `path_str` - The path to validate +/// * `base_dir` - Optional base directory for sandboxing +/// +/// # Returns +/// * `Ok(resolved_path)` - The canonicalized, validated path +/// * `Err(ToolError)` - If path escapes sandbox or is invalid +pub fn validate_path(path_str: &str, base_dir: Option<&Path>) -> Result { + // First pass: reject null bytes and URL-encoded traversal + // Note: We don't block `..` here because validate_path handles it by + // normalizing lexically and checking sandbox containment + if !is_path_safe_minimal(path_str) { + return Err(ToolError::NotAuthorized(format!( + "Path contains forbidden characters or sequences: {}", + path_str + ))); + } + + let path = PathBuf::from(path_str); + + // Resolve to absolute path + let resolved = if path.is_absolute() { + path.canonicalize() + .unwrap_or_else(|_| normalize_lexical(&path)) + } else if let Some(base) = base_dir { + let joined = base.join(&path); + joined + .canonicalize() + .unwrap_or_else(|_| normalize_lexical(&joined)) + } else { + let joined = std::env::current_dir() + .unwrap_or_else(|_| PathBuf::from(".")) + .join(&path); + normalize_lexical(&joined) + }; + + // If base_dir is set, ensure the resolved path is within it + if let Some(base) = base_dir { + let base_canonical = base + .canonicalize() + .unwrap_or_else(|_| normalize_lexical(base)); + + // For existing paths, canonicalize to resolve symlinks. + // For non-existent paths, the lexical normalization above already removed + // all `..` components, so starts_with is reliable. + let check_path = if resolved.exists() { + resolved.canonicalize().unwrap_or_else(|_| resolved.clone()) + } else { + // Walk up to the nearest existing ancestor directory, canonicalize it, + // then re-append the remaining tail. This handles the case where a + // symlink sits above the new file. + let mut ancestor = resolved.as_path(); + let mut tail_parts: Vec<&std::ffi::OsStr> = Vec::new(); + loop { + if ancestor.exists() { + let canonical_ancestor = ancestor + .canonicalize() + .unwrap_or_else(|_| ancestor.to_path_buf()); + let mut result = canonical_ancestor; + for part in tail_parts.into_iter().rev() { + result = result.join(part); + } + break result; + } + if let Some(name) = ancestor.file_name() { + tail_parts.push(name); + } + match ancestor.parent() { + Some(parent) if parent != ancestor => ancestor = parent, + _ => break resolved.clone(), + } + } + }; + + if !check_path.starts_with(&base_canonical) { + return Err(ToolError::NotAuthorized(format!( + "Path escapes sandbox: {}", + path_str + ))); + } + } + + Ok(resolved) +} + +/// Basic path safety check without requiring a base directory. +/// +/// This is a fallback check that blocks obvious traversal attempts: +/// - Contains `..` components +/// - Contains null bytes +/// - Uses URL encoding to hide traversal +/// +/// For stronger security, use validate_path() with a base_dir. +pub fn is_path_safe_basic(path: &str) -> bool { + // Block path traversal + if path.contains("..") { + return false; + } + + // Block null bytes (would panic in Path) + if path.contains('\0') { + return false; + } + + // Block URL-encoded traversal attempts + let lower = path.to_lowercase(); + if lower.contains("%2e") || lower.contains("%2f") || lower.contains("%5c") { + return false; + } + + true +} + +/// Check for null bytes and URL-encoded traversal only. +/// Unlike is_path_safe_basic, this allows `..` in paths since validate_path +/// handles that by normalizing lexically and checking sandbox containment. +fn is_path_safe_minimal(path: &str) -> bool { + if path.contains('\0') { + return false; + } + + let lower = path.to_lowercase(); + if lower.contains("%2e") || lower.contains("%2f") || lower.contains("%5c") { + return false; + } + + true +} + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::tempdir; + + #[test] + fn test_is_path_safe_basic_allows_normal_paths() { + assert!(is_path_safe_basic("/tmp/file.txt")); + assert!(is_path_safe_basic("documents/report.pdf")); + assert!(is_path_safe_basic("my-file.png")); + } + + #[test] + fn test_is_path_safe_basic_rejects_traversal() { + assert!(!is_path_safe_basic("../etc/passwd")); + assert!(!is_path_safe_basic("foo/../bar")); + assert!(!is_path_safe_basic("foo/bar/../../secret")); + } + + #[test] + fn test_is_path_safe_basic_rejects_null_bytes() { + assert!(!is_path_safe_basic("file\0.txt")); + assert!(!is_path_safe_basic("/tmp/test\0.txt")); + } + + #[test] + fn test_is_path_safe_basic_rejects_url_encoding() { + assert!(!is_path_safe_basic("%2e%2e%2fetc/passwd")); + assert!(!is_path_safe_basic("foo%2fbar")); + assert!(!is_path_safe_basic("test%5cpath")); + } + + #[test] + fn test_validate_path_allows_within_sandbox() { + let dir = tempdir().unwrap(); + let result = validate_path("subdir/file.txt", Some(dir.path())); + assert!(result.is_ok()); + } + + #[test] + fn test_validate_path_rejects_traversal_nonexistent_parent() { + let dir = tempdir().unwrap(); + // Create a sibling directory structure to test escape + // Try to escape to parent and access /etc/passwd + let result = validate_path("../etc/passwd", Some(dir.path())); + assert!(result.is_err()); + } + + #[test] + fn test_validate_path_rejects_relative_traversal() { + let dir = tempdir().unwrap(); + let result = validate_path("../../etc/passwd", Some(dir.path())); + assert!(result.is_err()); + } + + #[test] + fn test_validate_path_allows_valid_nested_write() { + let dir = tempdir().unwrap(); + let result = validate_path("subdir/newfile.txt", Some(dir.path())); + assert!(result.is_ok()); + } + + #[test] + fn test_validate_path_allows_dot_dot_within_sandbox() { + let dir = tempdir().unwrap(); + // This should be allowed as it stays within the sandbox + let result = validate_path("a/b/../c.txt", Some(dir.path())); + assert!(result.is_ok()); + } +} diff --git a/src/tools/registry.rs b/src/tools/registry.rs index f17a73a0..2ed639db 100644 --- a/src/tools/registry.rs +++ b/src/tools/registry.rs @@ -67,6 +67,7 @@ const PROTECTED_TOOL_NAMES: &[&str] = &[ "skill_search", "skill_install", "skill_remove", + "message", ]; /// Registry of available tools. @@ -80,6 +81,8 @@ pub struct ToolRegistry { secrets_store: Option>, /// Shared rate limiter for built-in tool invocations. rate_limiter: RateLimiter, + /// Reference to the message tool for setting context per-turn. + message_tool: RwLock>>, } impl ToolRegistry { @@ -91,6 +94,7 @@ impl ToolRegistry { credential_registry: None, secrets_store: None, rate_limiter: RateLimiter::new(), + message_tool: RwLock::new(None), } } @@ -399,6 +403,33 @@ impl ToolRegistry { tracing::info!("Registered 5 routine management tools"); } + /// Register message tool for sending messages to channels. + pub async fn register_message_tools( + &self, + channel_manager: Arc, + ) { + use crate::tools::builtin::MessageTool; + let tool = Arc::new(MessageTool::new(channel_manager)); + *self.message_tool.write().await = Some(Arc::clone(&tool)); + self.tools + .write() + .await + .insert(tool.name().to_string(), tool as Arc); + self.builtin_names + .write() + .await + .insert("message".to_string()); + tracing::info!("Registered message tool"); + } + + /// Set the default channel and target for the message tool. + /// Call this before each agent turn with the current conversation's context. + pub async fn set_message_tool_context(&self, channel: Option, target: Option) { + if let Some(tool) = self.message_tool.read().await.as_ref() { + tool.set_context(channel, target).await; + } + } + /// Register the software builder tool. /// /// The builder tool allows the agent to create new software including WASM tools,