diff --git a/crates/agents/src/lazy_tools.rs b/crates/agents/src/lazy_tools.rs index 532a226d74..aa7f7a895b 100644 --- a/crates/agents/src/lazy_tools.rs +++ b/crates/agents/src/lazy_tools.rs @@ -12,7 +12,9 @@ use std::sync::Arc; use {anyhow::Result, async_trait::async_trait, tracing::debug}; -use crate::tool_registry::{ActivatedTools, AgentTool, ToolEntry, ToolRegistry, ToolSource}; +use crate::tool_registry::{ + ActivatedTools, AgentTool, ToolAudience, ToolEntry, ToolRegistry, ToolSource, +}; /// Maximum number of results returned by a keyword search. const MAX_SEARCH_RESULTS: usize = 15; @@ -73,13 +75,21 @@ impl ToolSearchTool { .full_registry .get_source(name) .unwrap_or(ToolSource::Builtin); + let audience = self + .full_registry + .get_audience(name) + .unwrap_or(ToolAudience::Trusted); let schema = tool.parameters_schema(); let description = tool.description().to_string(); // Insert into activated map, preserving the original source metadata. let mut activated = self.activated.lock().unwrap_or_else(|e| e.into_inner()); - activated.insert(name.to_string(), ToolEntry { tool, source }); + activated.insert(name.to_string(), ToolEntry { + tool, + source, + audience, + }); debug!(tool = name, "tool activated via tool_search"); @@ -192,9 +202,13 @@ impl AgentTool for ToolSearchTool { /// Wrap a full tool registry for lazy mode. /// /// Returns a new registry containing only `tool_search`. The model discovers -/// and activates tools from `full` via that meta-tool. Activated tools -/// appear in `list_schemas()` on the next runner iteration. +/// and activates tools from `full` via that meta-tool. Activated tools appear +/// in `list_schemas()` on the next runner iteration. An empty input stays empty +/// so a deny-all request cannot regain a meta-tool after policy filtering. pub fn wrap_registry_lazy(full: ToolRegistry) -> ToolRegistry { + if full.is_empty() { + return full; + } let full = Arc::new(full); let mut lazy_registry = ToolRegistry::new(); @@ -205,7 +219,9 @@ pub fn wrap_registry_lazy(full: ToolRegistry) -> ToolRegistry { full_registry: full, activated, }; - lazy_registry.register(Box::new(search_tool)); + // The full registry has already been filtered for this run. Searching it + // cannot discover or activate a tool above the caller's audience ceiling. + lazy_registry.register_public(Box::new(search_tool)); lazy_registry } @@ -300,6 +316,12 @@ mod tests { assert!(names.contains(&"tool_search".to_string())); } + #[test] + fn wrap_registry_lazy_keeps_deny_all_registry_empty() { + let lazy = wrap_registry_lazy(ToolRegistry::new()); + assert!(lazy.list_names().is_empty()); + } + #[tokio::test] async fn keyword_search_returns_matching_tools() { let full = build_full_registry(); diff --git a/crates/agents/src/runner/helpers.rs b/crates/agents/src/runner/helpers.rs index 4078116aa7..b1601d4648 100644 --- a/crates/agents/src/runner/helpers.rs +++ b/crates/agents/src/runner/helpers.rs @@ -426,9 +426,20 @@ pub(crate) fn explicit_shell_command_from_user_content( user_content: &UserContent, ) -> Option { let text = match user_content { - UserContent::Text(text) => text.trim(), + UserContent::Text(text) => text, UserContent::Multimodal(_) => return None, }; + explicit_shell_command(text) +} + +/// Detect an explicit `/sh ...` shell request in raw message text. +/// +/// Callers that gate shell access (e.g. the gateway's channel dispatch) must +/// use this exact predicate so authorization and execution agree on what +/// counts as a shell request. +#[must_use] +pub fn explicit_shell_command(text: &str) -> Option { + let text = text.trim(); if text.is_empty() || text.len() > 4096 || text.contains('\n') || text.contains('\r') { return None; diff --git a/crates/agents/src/runner/mod.rs b/crates/agents/src/runner/mod.rs index 31f47fb61e..b83472f9bd 100644 --- a/crates/agents/src/runner/mod.rs +++ b/crates/agents/src/runner/mod.rs @@ -16,7 +16,10 @@ mod tests_legacy; // ── Re-exports (preserve public API) ──────────────────────────────────── pub use { - helpers::{AgentLoopLimits, AgentRunError, AgentRunResult, OnEvent, RunnerEvent}, + helpers::{ + AgentLoopLimits, AgentRunError, AgentRunResult, OnEvent, RunnerEvent, + explicit_shell_command, + }, non_streaming::{ run_agent, run_agent_loop, run_agent_loop_with_context, run_agent_loop_with_context_and_limits, diff --git a/crates/agents/src/tool_registry.rs b/crates/agents/src/tool_registry.rs index a4464a9cdd..a0346ccaad 100644 --- a/crates/agents/src/tool_registry.rs +++ b/crates/agents/src/tool_registry.rs @@ -33,10 +33,39 @@ pub enum ToolSource { Wasm { component_hash: [u8; 32] }, } -/// Internal entry pairing a tool with its source metadata. +/// The least-trusted audience allowed to use a tool. +/// +/// Tools are trusted-only unless their registration site explicitly opts into +/// public use. Third-party metadata must not be able to widen this host-owned +/// security boundary. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub enum ToolAudience { + /// Safe for untrusted channel and webhook turns. + Public, + /// Available only to trusted owner/operator turns. + #[default] + Trusted, +} + +impl ToolAudience { + fn allows(self, required: Self) -> bool { + matches!(self, Self::Trusted) || matches!(required, Self::Public) + } + + fn as_str(self) -> &'static str { + match self { + Self::Public => "public", + Self::Trusted => "trusted", + } + } +} + +/// Internal entry pairing a tool with host-owned metadata. +#[derive(Clone)] pub(crate) struct ToolEntry { pub(crate) tool: Arc, pub(crate) source: ToolSource, + pub(crate) audience: ToolAudience, } /// Shared set of tools activated at runtime by [`ToolSearchTool`](crate::lazy_tools::ToolSearchTool). @@ -71,74 +100,81 @@ impl ToolRegistry { } } - /// Register a built-in tool. Warns (and overwrites) on name collision. + /// Whether the registry has no static or lazily activated tools. + #[must_use] + pub fn is_empty(&self) -> bool { + self.tools.is_empty() + && self + .activated + .lock() + .unwrap_or_else(|error| error.into_inner()) + .is_empty() + } + + /// Register a trusted-only built-in tool. + /// + /// Name collisions are rejected so an unrelated implementation cannot + /// inherit another tool's policy identity. pub fn register(&mut self, tool: Box) { - let name = tool.name().to_string(); - let new_source = ToolSource::Builtin; - if let Some(existing) = self.tools.get(&name) { - warn!( - tool = %name, - old_source = ?existing.source, - new_source = ?new_source, - "tool name collision — new registration overwrites existing entry" - ); - } - self.tools.insert(name, ToolEntry { - tool: Arc::from(tool), - source: new_source, - }); + self.register_builtin(tool, ToolAudience::Trusted); + } + + /// Register a built-in tool reviewed as safe for untrusted input. + pub fn register_public(&mut self, tool: Box) { + self.register_builtin(tool, ToolAudience::Public); + } + + fn register_builtin(&mut self, tool: Box, audience: ToolAudience) { + self.insert(tool, ToolSource::Builtin, audience); } - /// Register a tool from an MCP server. Warns (and overwrites) on name collision. + /// Register a trusted-only tool from an MCP server. pub fn register_mcp(&mut self, tool: Box, server: McpServerId) { - let name = tool.name().to_string(); - let new_source = ToolSource::Mcp { server }; - if let Some(existing) = self.tools.get(&name) { - warn!( - tool = %name, - old_source = ?existing.source, - new_source = ?new_source, - "tool name collision — new registration overwrites existing entry" - ); - } - self.tools.insert(name, ToolEntry { - tool: Arc::from(tool), - source: new_source, - }); + self.insert(tool, ToolSource::Mcp { server }, ToolAudience::Trusted); } - /// Register a tool from a WASM component. Warns (and overwrites) on name collision. + /// Register a trusted-only tool from a WASM component. pub fn register_wasm(&mut self, tool: Box, component_hash: [u8; 32]) { + self.insert( + tool, + ToolSource::Wasm { component_hash }, + ToolAudience::Trusted, + ); + } + + fn insert(&mut self, tool: Box, source: ToolSource, audience: ToolAudience) { let name = tool.name().to_string(); - let new_source = ToolSource::Wasm { component_hash }; if let Some(existing) = self.tools.get(&name) { warn!( tool = %name, old_source = ?existing.source, - new_source = ?new_source, - "tool name collision — new registration overwrites existing entry" + new_source = ?source, + "tool name collision; rejecting new registration" ); + return; } self.tools.insert(name, ToolEntry { tool: Arc::from(tool), - source: new_source, + source, + audience, }); } - /// Replace an existing tool by name, preserving its source metadata. + /// Replace an existing tool by name, preserving its host-owned metadata. /// /// Returns `true` if an existing tool was replaced, `false` if this was a new entry. pub fn replace(&mut self, tool: Box) -> bool { let name = tool.name().to_string(); - let source = self + let metadata = self .tools .get(&name) - .map(|entry| entry.source.clone()) - .unwrap_or(ToolSource::Builtin); + .map(|entry| (entry.source.clone(), entry.audience)) + .unwrap_or((ToolSource::Builtin, ToolAudience::Trusted)); self.tools .insert(name, ToolEntry { tool: Arc::from(tool), - source, + source: metadata.0, + audience: metadata.1, }) .is_some() } @@ -168,6 +204,11 @@ impl ToolRegistry { self.tools.get(name).map(|e| e.source.clone()) } + /// Return the [`ToolAudience`] for a tool by name. + pub(crate) fn get_audience(&self, name: &str) -> Option { + self.tools.get(name).map(|e| e.audience) + } + pub fn list_schemas(&self) -> Vec { let mut schemas: Vec = self .tools @@ -237,12 +278,7 @@ impl ToolRegistry { .tools .iter() .filter(|(name, _)| !name.starts_with(prefix)) - .map(|(name, entry)| { - (name.clone(), ToolEntry { - tool: Arc::clone(&entry.tool), - source: entry.source.clone(), - }) - }) + .map(|(name, entry)| (name.clone(), entry.clone())) .collect(); ToolRegistry { tools, @@ -256,12 +292,7 @@ impl ToolRegistry { .tools .iter() .filter(|(_, entry)| !matches!(entry.source, ToolSource::Mcp { .. })) - .map(|(name, entry)| { - (name.clone(), ToolEntry { - tool: Arc::clone(&entry.tool), - source: entry.source.clone(), - }) - }) + .map(|(name, entry)| (name.clone(), entry.clone())) .collect(); ToolRegistry { tools, @@ -275,12 +306,7 @@ impl ToolRegistry { .tools .iter() .filter(|(name, _)| !exclude.contains(&name.as_str())) - .map(|(name, entry)| { - (name.clone(), ToolEntry { - tool: Arc::clone(&entry.tool), - source: entry.source.clone(), - }) - }) + .map(|(name, entry)| (name.clone(), entry.clone())) .collect(); ToolRegistry { tools, @@ -305,12 +331,25 @@ impl ToolRegistry { .tools .iter() .filter(|(name, entry)| predicate(name, &entry.source)) - .map(|(name, entry)| { - (name.clone(), ToolEntry { - tool: Arc::clone(&entry.tool), - source: entry.source.clone(), - }) - }) + .map(|(name, entry)| (name.clone(), entry.clone())) + .collect(); + ToolRegistry { + tools, + activated: Arc::new(Mutex::new(HashMap::new())), + } + } + + /// Clone the registry at an audience ceiling. + /// + /// A public caller receives only explicitly reviewed public tools. Trusted + /// callers retain the complete registry. Name-based policy can narrow the + /// result later but cannot restore entries removed here. + pub fn clone_for_audience(&self, audience: ToolAudience) -> ToolRegistry { + let tools = self + .tools + .iter() + .filter(|(_, entry)| audience.allows(entry.audience)) + .map(|(name, entry)| (name.clone(), entry.clone())) .collect(); ToolRegistry { tools, @@ -328,6 +367,7 @@ fn entry_to_schema(e: &ToolEntry) -> serde_json::Value { "name": e.tool.name(), "description": e.tool.description(), "parameters": e.tool.parameters_schema(), + "audience": e.audience.as_str(), }); match &e.source { ToolSource::Builtin => { @@ -496,6 +536,7 @@ mod tests { .find(|s| s["name"] == "exec") .expect("exec should exist"); assert_eq!(builtin["source"], "builtin"); + assert_eq!(builtin["audience"], "trusted"); assert!(builtin.get("mcpServer").is_none() || builtin["mcpServer"].is_null()); let mcp = schemas @@ -504,6 +545,7 @@ mod tests { .expect("mcp tool should exist"); assert_eq!(mcp["source"], "mcp"); assert_eq!(mcp["mcpServer"], "github"); + assert_eq!(mcp["audience"], "trusted"); } #[test] @@ -582,22 +624,24 @@ mod tests { } #[test] - fn test_register_collision_overwrites_with_warning() { - // The warn! output is emitted via tracing; we assert the overwrite - // semantics and trust the log at runtime. + fn test_register_collision_keeps_original_metadata() { let mut registry = ToolRegistry::new(); - registry.register(Box::new(DummyTool { - name: "Read".to_string(), - })); - // Same name again — should overwrite, warn logged. - registry.register(Box::new(DummyTool { + registry.register_public(Box::new(DummyTool { name: "Read".to_string(), })); + registry.register_mcp( + Box::new(DummyTool { + name: "Read".to_string(), + }), + McpServerId::from("filesystem"), + ); assert_eq!(registry.list_names(), vec!["Read".to_string()]); + assert_eq!(registry.get_source("Read"), Some(ToolSource::Builtin)); + assert_eq!(registry.get_audience("Read"), Some(ToolAudience::Public)); } #[test] - fn test_register_mcp_overwriting_builtin_warns() { + fn test_register_mcp_cannot_overwrite_builtin() { let mut registry = ToolRegistry::new(); registry.register(Box::new(DummyTool { name: "Read".to_string(), @@ -608,9 +652,8 @@ mod tests { }), McpServerId::from("filesystem"), ); - // Source should now be Mcp even though the builtin was registered first. let src = registry.get_source("Read").unwrap(); - assert!(matches!(src, ToolSource::Mcp { .. })); + assert_eq!(src, ToolSource::Builtin); } #[test] @@ -630,4 +673,47 @@ mod tests { let names = filtered.list_names(); assert_eq!(names, vec!["exec".to_string(), "web_fetch".to_string()]); } + + #[test] + fn public_audience_excludes_new_and_external_tools_by_default() { + let mut registry = ToolRegistry::new(); + registry.register_public(Box::new(DummyTool { + name: "calc".to_string(), + })); + registry.register(Box::new(DummyTool { + name: "future_builtin".to_string(), + })); + registry.register_mcp( + Box::new(DummyTool { + name: "mcp__example__read".to_string(), + }), + McpServerId::from("example"), + ); + registry.register_wasm( + Box::new(DummyTool { + name: "future_wasm".to_string(), + }), + [0xCD; 32], + ); + + let public = registry.clone_for_audience(ToolAudience::Public); + assert_eq!(public.list_names(), vec!["calc".to_string()]); + assert!(public.get("future_builtin").is_none()); + assert!(public.get("mcp__example__read").is_none()); + assert!(public.get("future_wasm").is_none()); + } + + #[test] + fn trusted_audience_keeps_all_tools() { + let mut registry = ToolRegistry::new(); + registry.register_public(Box::new(DummyTool { + name: "calc".to_string(), + })); + registry.register(Box::new(DummyTool { + name: "exec".to_string(), + })); + + let trusted = registry.clone_for_audience(ToolAudience::Trusted); + assert_eq!(trusted.list_names(), vec!["calc", "exec"]); + } } diff --git a/crates/channels/src/chat_classification.rs b/crates/channels/src/chat_classification.rs new file mode 100644 index 0000000000..f7556ec7bf --- /dev/null +++ b/crates/channels/src/chat_classification.rs @@ -0,0 +1,148 @@ +use crate::plugin::ChannelType; + +pub(crate) fn classify_chat(channel_type: ChannelType, chat_id: &str) -> Option { + match channel_type { + ChannelType::Telegram => { + if chat_id.starts_with("-100") { + Some("channel_or_supergroup".to_string()) + } else if chat_id.starts_with('-') { + Some("group".to_string()) + } else { + Some("private".to_string()) + } + }, + ChannelType::Signal => Some( + if chat_id.starts_with("group:") { + "group" + } else { + "direct" + } + .to_string(), + ), + ChannelType::Slack => Some( + if chat_id.starts_with('D') { + "direct" + } else { + "channel" + } + .to_string(), + ), + ChannelType::Whatsapp => Some( + if chat_id.ends_with("@g.us") { + "group" + } else { + "direct" + } + .to_string(), + ), + ChannelType::Nostr => Some("dm".to_string()), + ChannelType::Telephony => Some("call".to_string()), + ChannelType::MsTeams | ChannelType::Discord | ChannelType::Matrix => None, + } +} + +/// WhatsApp JID suffixes that identify a one-to-one conversation. +/// +/// `@s.whatsapp.net` is the phone-number form and `@lid` the privacy-preserving +/// linked-ID form. Everything else — `@g.us` groups, `status@broadcast`, +/// `@newsletter` channels, and any suffix WhatsApp adds later — is treated as +/// shared. +const WHATSAPP_DIRECT_SUFFIXES: [&str; 2] = ["@s.whatsapp.net", "@lid"]; + +/// Whether a chat can carry messages from principals other than the sender. +/// +/// This is the gate for privileged access, so it is an **allowlist of shapes +/// known to be one-to-one**, not a denylist of known group shapes. A chat id +/// whose form we do not recognise is shared, which costs an operator a denial; +/// getting it backwards would hand a room full of strangers the host. +/// +/// Callers must not read this as "the platform proved the conversation is +/// direct" beyond what the id itself encodes. Telephony is deliberately shared: +/// its chat id is a caller number, and caller ID is trivially spoofable, so +/// there is nothing here to authenticate against. +pub(crate) fn is_shared_chat(channel_type: ChannelType, chat_id: &str) -> bool { + match channel_type { + // Negative ids are groups, supergroups, and channels; users are positive. + ChannelType::Telegram => !chat_id.chars().all(|c| c.is_ascii_digit()) || chat_id.is_empty(), + // Signal direct ids are an E.164 number or an ACI UUID; groups carry a + // `group:` prefix. Both forms are non-empty, so a blank id is unknown. + ChannelType::Signal => chat_id.is_empty() || chat_id.starts_with("group:"), + // Slack conversation ids are prefixed by kind: `D` is a 1:1 DM, `G` a + // multi-person DM, `C` a channel. + ChannelType::Slack => !chat_id.starts_with('D'), + ChannelType::Whatsapp => !WHATSAPP_DIRECT_SUFFIXES + .iter() + .any(|suffix| chat_id.ends_with(suffix)), + // Nostr DMs are addressed to a pubkey the sender must hold the key for. + ChannelType::Nostr => false, + // Telephony: caller-ID only, see above. Discord/Teams/Matrix ids do not + // encode conversation kind and the adapters do not yet forward it. + ChannelType::Telephony + | ChannelType::MsTeams + | ChannelType::Discord + | ChannelType::Matrix => true, + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn direct_and_shared_shapes_are_classified_fail_closed() { + assert!(!is_shared_chat(ChannelType::Telegram, "123")); + assert!(is_shared_chat(ChannelType::Telegram, "-123")); + assert!(!is_shared_chat(ChannelType::Slack, "D123")); + assert!(is_shared_chat(ChannelType::Slack, "C123")); + assert!(!is_shared_chat( + ChannelType::Whatsapp, + "15551234567@s.whatsapp.net" + )); + assert!(is_shared_chat(ChannelType::Whatsapp, "123@g.us")); + assert!(is_shared_chat(ChannelType::Discord, "123")); + assert!(is_shared_chat(ChannelType::Matrix, "!room:example.org")); + } + + /// Unrecognised id shapes must be shared, not direct. An empty or + /// unexpectedly-formatted id is exactly the case where we know least. + #[test] + fn unrecognised_id_shapes_are_shared() { + for channel_type in [ + ChannelType::Telegram, + ChannelType::Signal, + ChannelType::Slack, + ChannelType::Whatsapp, + ] { + assert!( + is_shared_chat(channel_type, ""), + "{channel_type:?} treated a blank chat id as direct" + ); + } + assert!(is_shared_chat(ChannelType::Telegram, "not-a-number")); + } + + /// WhatsApp gained group-adjacent address kinds over time (status + /// broadcasts, newsletter channels). Only the two one-to-one JID forms are + /// direct; a new suffix must not silently read as a DM. + #[test] + fn whatsapp_direct_is_an_allowlist_of_jid_suffixes() { + assert!(!is_shared_chat( + ChannelType::Whatsapp, + "15551234567@s.whatsapp.net" + )); + assert!(!is_shared_chat( + ChannelType::Whatsapp, + "259557842534599@lid" + )); + assert!(is_shared_chat(ChannelType::Whatsapp, "status@broadcast")); + assert!(is_shared_chat(ChannelType::Whatsapp, "123@newsletter")); + assert!(is_shared_chat(ChannelType::Whatsapp, "15551234567")); + } + + /// A phone number is a claim, not an authenticated identity: caller ID is + /// spoofable, so a call can never be a proven direct chat. + #[test] + fn telephony_is_never_a_proven_direct_chat() { + assert!(is_shared_chat(ChannelType::Telephony, "+15551234567")); + } +} diff --git a/crates/channels/src/commands.rs b/crates/channels/src/commands.rs index 5d7348dc38..4c3e2781ae 100644 --- a/crates/channels/src/commands.rs +++ b/crates/channels/src/commands.rs @@ -37,6 +37,50 @@ pub struct CommandDef { pub arg: Option, } +/// Authorization required before a channel command reaches its handler. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum CommandPrivilege { + Public, + OperatorDirect, +} + +impl CommandDef { + /// Commands default to operator direct-chat only. Adding a new command + /// therefore fails closed until it is deliberately reviewed and listed as + /// public here. + /// + /// A command is public only when the worst a hostile sender can do with it + /// is disrupt the conversation in the room they are already in — something + /// any member can do by talking. Anything that reaches the host, acts on + /// the owner's behalf, or reads state from outside this room is operator + /// direct-chat only: + /// + /// - `sh` runs host commands; `update` replaces the running binary. + /// - `approve`/`deny` act on the *owner's* pending exec requests, so a + /// guest approving one is arbitrary code execution by proxy. `approvals` + /// lists the command lines awaiting approval. + /// - `sandbox` can turn the sandbox off, moving execution onto the host. + /// - `attach` pulls an existing session into this chat, and + /// `sessions`/`context`/`insights`/`peek`/`btw` read session, prompt, or + /// cross-session state the room's participants are not entitled to. + /// - `rollback` restores a checkpoint; `agent` swaps the system prompt and + /// tool posture; `steer`/`queue` inject into a run someone else started. + #[must_use] + pub fn privilege(self) -> CommandPrivilege { + match self.name { + // Help lists the commands and marks which need an operator. + "help" + // Scoped to this chat's own session: start it over, clear it, + // summarize it, retitle it, branch it, or abort its current run. + | "new" | "clear" | "compact" | "title" | "fork" | "stop" + // Which model/mode answers in this chat. Reversible, and bounded by + // the account's own provider configuration. + | "model" | "mode" | "fast" => CommandPrivilege::Public, + _ => CommandPrivilege::OperatorDirect, + } + } +} + /// The single source of truth for all channel commands. /// /// Order determines display order in help text and platform menus. @@ -290,7 +334,10 @@ pub fn help_text() -> String { let mut lines = Vec::with_capacity(all_commands().len() + 1); lines.push("Available commands:".to_string()); for cmd in all_commands() { - lines.push(format!("/{} — {}", cmd.name, cmd.description)); + let operator = matches!(cmd.privilege(), CommandPrivilege::OperatorDirect) + .then_some(" (operator DM only)") + .unwrap_or_default(); + lines.push(format!("/{} — {}{operator}", cmd.name, cmd.description)); } lines.join("\n") } @@ -313,6 +360,65 @@ mod tests { assert_eq!(names.len(), deduped.len(), "duplicate command names found"); } + /// The public set is small and enumerated here on purpose. Adding a command + /// to it is a security decision, so it must be made in both places or this + /// fails — a new command is never public by accident. + #[test] + fn only_reviewed_commands_are_public() { + const PUBLIC: &[&str] = &[ + "help", "new", "clear", "compact", "title", "fork", "stop", "model", "mode", "fast", + ]; + + for command in all_commands() { + let expected = if PUBLIC.contains(&command.name) { + CommandPrivilege::Public + } else { + CommandPrivilege::OperatorDirect + }; + assert_eq!( + command.privilege(), + expected, + "unexpected privilege for /{}", + command.name + ); + } + } + + /// The commands that reach the host, act for the owner, or read state from + /// outside the current chat must never become public. + #[test] + fn host_and_owner_scoped_commands_stay_operator_only() { + for name in [ + "sh", + "update", + "approve", + "deny", + "approvals", + "sandbox", + "attach", + "sessions", + "context", + "insights", + "peek", + "btw", + "rollback", + "agent", + "steer", + "queue", + ] { + let command = all_commands() + .iter() + .find(|candidate| candidate.name == name) + .copied() + .unwrap_or_else(|| panic!("/{name} is missing from the command registry")); + assert_eq!( + command.privilege(), + CommandPrivilege::OperatorDirect, + "/{name} must stay restricted to operator direct chats" + ); + } + } + #[test] fn help_is_in_list() { assert!( diff --git a/crates/channels/src/config_view.rs b/crates/channels/src/config_view.rs index 43a1ae1678..1ae91d5388 100644 --- a/crates/channels/src/config_view.rs +++ b/crates/channels/src/config_view.rs @@ -15,6 +15,18 @@ pub trait ChannelConfigView: Send + Sync + std::fmt::Debug { /// Group/chat ID allowlist. fn group_allowlist(&self) -> &[String]; + /// Senders allowed to run privileged actions (`/sh`, shell command mode, + /// host-reaching tools). + /// + /// Separate from [`Self::allowlist`], which only decides who may talk to + /// the bot. In a guild or group chat every member can pass the access + /// gate, so privilege needs its own list. + /// + /// An empty list grants nobody privileged access. + fn operators(&self) -> &[String] { + &[] + } + /// DM access policy. fn dm_policy(&self) -> DmPolicy; diff --git a/crates/channels/src/gating.rs b/crates/channels/src/gating.rs index 7ace6ff44f..9df4749214 100644 --- a/crates/channels/src/gating.rs +++ b/crates/channels/src/gating.rs @@ -13,7 +13,7 @@ pub fn is_allowed(peer_id: &str, allowlist: &[String]) -> bool { allowlist.iter().any(|pattern| { let pat = pattern.to_lowercase(); if pat.contains('*') { - glob_match(&pat, &peer_lower) + glob_match_lower(&pat, &peer_lower) } else { pat == peer_lower } @@ -21,7 +21,9 @@ pub fn is_allowed(peer_id: &str, allowlist: &[String]) -> bool { } /// Simple glob matching supporting `*` as a wildcard for any sequence of chars. -fn glob_match(pattern: &str, text: &str) -> bool { +/// +/// Both arguments must already be lowercased by the caller. +fn glob_match_lower(pattern: &str, text: &str) -> bool { let parts: Vec<&str> = pattern.split('*').collect(); if parts.len() == 1 { return pattern == text; diff --git a/crates/channels/src/lib.rs b/crates/channels/src/lib.rs index 46f16ba8ef..45c20b6e59 100644 --- a/crates/channels/src/lib.rs +++ b/crates/channels/src/lib.rs @@ -4,6 +4,8 @@ //! ChannelPlugin trait with sub-traits for config, auth, inbound/outbound //! messaging, status, and gateway lifecycle. +mod chat_classification; + pub mod channel_webhook_middleware; pub mod commands; pub mod config_view; @@ -12,6 +14,7 @@ pub mod error; pub mod gating; pub mod media_download; pub mod message_log; +pub mod operators; pub mod otp; pub mod plugin; pub mod registry; diff --git a/crates/channels/src/operators.rs b/crates/channels/src/operators.rs new file mode 100644 index 0000000000..779b0b34e1 --- /dev/null +++ b/crates/channels/src/operators.rs @@ -0,0 +1,179 @@ +//! Operator (privileged sender) resolution for channel accounts. +//! +//! Channel access gating (`crate::gating`) answers "may this peer talk to the +//! bot at all". That is deliberately permissive: in a guild or group chat every +//! member who passes the group policy can talk to the bot. +//! +//! Operator resolution answers a narrower question: "is this sender eligible +//! for privileged actions". The gateway additionally requires a proven direct +//! conversation before exposing `/sh`, shell command mode, or tools that reach +//! the host (`exec`, file writes, …). Resolution is **fail-closed**: when +//! nothing is configured, nobody is an operator. + +/// Privilege level of a channel sender for a single inbound message. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub enum ChannelSenderRole { + /// Explicitly trusted; privileged access still requires a direct chat. + Operator, + /// Everyone else, including anonymous/unattributed senders. + #[default] + Guest, +} + +impl ChannelSenderRole { + /// Whether this role may perform privileged actions. + #[must_use] + pub fn is_operator(self) -> bool { + matches!(self, Self::Operator) + } +} + +/// Check whether a sender matches an entry in a privilege list. +/// +/// Unlike [`crate::gating::is_allowed`], an **empty list matches nobody** — +/// "no operators configured" must never mean "everyone is an operator". +/// +/// Privileged identities are exact, case-sensitive platform sender IDs. Globs, +/// usernames, and generic suffix stripping are intentionally unsupported: the +/// semantics differ between platforms and can conflate distinct principals. +#[must_use] +pub fn is_listed(sender_id: &str, list: &[String]) -> bool { + if list.is_empty() || sender_id.is_empty() { + return false; + } + list.iter().any(|listed| listed == sender_id) +} + +/// Resolve a sender's privilege level for an account. +/// +/// Only the explicit `operators` list grants privilege. The conversational +/// allowlist is intentionally not consulted: OTP approval mutates that list, +/// and access to the bot must not imply access to the host. +/// +/// An absent `sender_id` (e.g. an unattributed button callback) is always a +/// guest — privilege is never granted to an unidentified sender. +#[must_use] +pub fn resolve_sender_role(sender_id: Option<&str>, operators: &[String]) -> ChannelSenderRole { + let Some(sender_id) = sender_id.filter(|s| !s.is_empty()) else { + return ChannelSenderRole::Guest; + }; + + if is_listed(sender_id, operators) { + ChannelSenderRole::Operator + } else { + ChannelSenderRole::Guest + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn list(items: &[&str]) -> Vec { + items.iter().map(|s| (*s).to_string()).collect() + } + + #[test] + fn empty_list_matches_nobody() { + assert!(!is_listed("alice", &[])); + } + + #[test] + fn empty_sender_matches_nothing() { + assert!(!is_listed("", &list(&["alice"]))); + assert!(!is_listed("", &list(&["*"]))); + } + + #[test] + fn exact_platform_id_match() { + let ops = list(&["Alice", "400347514466992128"]); + assert!(is_listed("Alice", &ops)); + assert!(!is_listed("alice", &ops)); + assert!(!is_listed("ALICE", &ops)); + assert!(is_listed("400347514466992128", &ops)); + assert!(!is_listed("mallory", &ops)); + } + + #[test] + fn wildcard_is_not_a_privileged_identity() { + let ops = list(&["admin_*"]); + assert!(!is_listed("admin_alice", &ops)); + assert!(!is_listed("user_bob", &ops)); + } + + #[test] + fn suffixed_ids_require_the_full_platform_identity() { + assert!(!is_listed( + "15551234567@s.whatsapp.net", + &list(&["15551234567"]) + )); + assert!(is_listed( + "@alice:example.org", + &list(&["@alice:example.org"]) + )); + assert!(!is_listed( + "15559999999@s.whatsapp.net", + &list(&["15551234567"]) + )); + } + + #[test] + fn only_operators_grant_privilege() { + let operators = list(&["owner"]); + assert_eq!( + resolve_sender_role(Some("owner"), &operators), + ChannelSenderRole::Operator + ); + assert_eq!( + resolve_sender_role(Some("helper"), &operators), + ChannelSenderRole::Guest + ); + } + + #[test] + fn sender_ids_are_not_normalized_before_matching() { + let operators = list(&["owner"]); + assert_eq!( + resolve_sender_role(Some(" owner "), &operators), + ChannelSenderRole::Guest + ); + } + + #[test] + fn no_operators_fails_closed() { + assert_eq!( + resolve_sender_role(Some("owner"), &[]), + ChannelSenderRole::Guest + ); + } + + #[test] + fn fails_closed_with_no_lists() { + // Open account (no allowlist, no operators): nobody is privileged. + assert_eq!( + resolve_sender_role(Some("anyone"), &[]), + ChannelSenderRole::Guest + ); + } + + #[test] + fn missing_or_blank_sender_is_guest() { + let operators = list(&["owner"]); + assert_eq!( + resolve_sender_role(None, &operators), + ChannelSenderRole::Guest + ); + assert_eq!( + resolve_sender_role(Some(" "), &operators), + ChannelSenderRole::Guest + ); + } + + #[test] + fn wildcard_operator_entry_grants_nobody() { + assert_eq!( + resolve_sender_role(Some("anyone"), &list(&["*"])), + ChannelSenderRole::Guest + ); + } +} diff --git a/crates/channels/src/otp.rs b/crates/channels/src/otp.rs index 9a1ae0f880..72c8500be2 100644 --- a/crates/channels/src/otp.rs +++ b/crates/channels/src/otp.rs @@ -395,6 +395,7 @@ mod tests { async fn update_location( &self, _reply_to: &ChannelReplyTarget, + _sender_id: Option<&str>, _lat: f64, _lon: f64, ) -> bool { diff --git a/crates/channels/src/plugin.rs b/crates/channels/src/plugin.rs index e7212befcf..fdcb1151bf 100644 --- a/crates/channels/src/plugin.rs +++ b/crates/channels/src/plugin.rs @@ -61,27 +61,14 @@ impl ChannelType { /// Best-effort chat classification for hook and prompt context. #[must_use] pub fn classify_chat(&self, chat_id: &str) -> Option { - match self { - Self::Telegram => { - if chat_id.starts_with("-100") { - Some("channel_or_supergroup".to_string()) - } else if chat_id.starts_with('-') { - Some("group".to_string()) - } else { - Some("private".to_string()) - } - }, - Self::Signal => { - if chat_id.starts_with("group:") { - Some("group".to_string()) - } else { - Some("direct".to_string()) - } - }, - Self::Nostr => Some("dm".to_string()), - Self::Telephony => Some("call".to_string()), - _ => None, - } + crate::chat_classification::classify_chat(*self, chat_id) + } + + /// Whether a chat can contain messages from multiple principals. + /// Unknown platform chat kinds fail closed as shared. + #[must_use] + pub fn is_shared_chat(&self, chat_id: &str) -> bool { + crate::chat_classification::is_shared_chat(*self, chat_id) } /// Top-level config fields that must be treated as persisted secrets. @@ -429,10 +416,9 @@ pub trait ChannelEventSink: Send + Sync { /// Dispatch a slash command (e.g. "new", "clear", "compact", "context") /// and return a text result to send back to the channel. /// - /// `sender_id` identifies the message sender. Privileged commands - /// (`/approve`, `/deny`) are restricted to senders on the channel - /// account's allowlist — authorization is enforced centrally by the - /// gateway, so channel implementations do not need to handle it. + /// `sender_id` identifies the message sender. Commands other than `/help` + /// are restricted to exact IDs in the channel account's `operators` list. + /// Authorization is enforced centrally by the gateway. async fn dispatch_command( &self, command: &str, @@ -505,6 +491,7 @@ pub trait ChannelEventSink: Send + Sync { async fn update_location( &self, _reply_to: &ChannelReplyTarget, + _sender_id: Option<&str>, _latitude: f64, _longitude: f64, ) -> bool { @@ -519,6 +506,7 @@ pub trait ChannelEventSink: Send + Sync { async fn resolve_pending_location( &self, _reply_to: &ChannelReplyTarget, + _sender_id: Option<&str>, _latitude: f64, _longitude: f64, ) -> bool { @@ -532,6 +520,7 @@ pub trait ChannelEventSink: Send + Sync { &self, _callback_data: &str, _reply_to: ChannelReplyTarget, + _sender_id: Option<&str>, ) -> Result { Err(Error::unavailable("interactions not supported")) } @@ -658,6 +647,18 @@ pub struct ChannelReplyTarget { } impl ChannelReplyTarget { + /// Deterministic session key used when a channel has no explicit active + /// session override. + pub fn default_session_key(&self) -> String { + match &self.thread_id { + Some(thread_id) => format!( + "{}:{}:{}:{}", + self.channel_type, self.account_id, self.chat_id, thread_id + ), + None => format!("{}:{}:{}", self.channel_type, self.account_id, self.chat_id), + } + } + /// Returns the address string for outbound sends. /// /// For Telegram forum topics this encodes both chat and thread as @@ -1132,7 +1133,11 @@ mod tests { message_id: None, thread_id: None, }; - assert!(!sink.update_location(&target, 48.8566, 2.3522).await); + assert!( + !sink + .update_location(&target, Some("sender"), 48.8566, 2.3522) + .await + ); } #[test] diff --git a/crates/chat/src/agent_loop.rs b/crates/chat/src/agent_loop.rs index 0f62e43a2b..48f2abf579 100644 --- a/crates/chat/src/agent_loop.rs +++ b/crates/chat/src/agent_loop.rs @@ -399,13 +399,17 @@ pub(crate) async fn run_explicit_shell_command( client_seq, Some(run_id), ); - let tool_result_msg = PersistedMessage::tool_result( + // Both halves of the pair must carry the run id: run-scoped history + // filtering keeps or drops a run as a unit, and a result without one + // would be dropped on its own, orphaning the tool call above. + let tool_result_msg = PersistedMessage::tool_result_with_run_id( tool_call_id.clone(), "exec", Some(serde_json::json!({ "command": command })), true, Some(capped.clone()), None, + run_id, ); persist_tool_history_pair( session_store, @@ -452,13 +456,14 @@ pub(crate) async fn run_explicit_shell_command( client_seq, Some(run_id), ); - let tool_result_msg = PersistedMessage::tool_result( + let tool_result_msg = PersistedMessage::tool_result_with_run_id( tool_call_id.clone(), "exec", Some(serde_json::json!({ "command": command })), false, None, Some(error_text.clone()), + run_id, ); persist_tool_history_pair( session_store, diff --git a/crates/chat/src/lib.rs b/crates/chat/src/lib.rs index 63bcd570bd..a1855eb32a 100644 --- a/crates/chat/src/lib.rs +++ b/crates/chat/src/lib.rs @@ -13,6 +13,7 @@ mod types; pub mod chat_error; pub mod error; +pub mod request_params; pub mod runtime; pub use { diff --git a/crates/chat/src/request_params.rs b/crates/chat/src/request_params.rs new file mode 100644 index 0000000000..6058b3a0f9 --- /dev/null +++ b/crates/chat/src/request_params.rs @@ -0,0 +1,91 @@ +//! Host-owned `chat.send` parameters that untrusted callers must not supply. +//! +//! Most request-scoped security parameters (`_tool_policy`, `_tool_audience`, +//! `_private_context`) can only ever *narrow* what a turn may do, so a caller +//! passing one is harmless. The names here are different: they assert something +//! the caller is not entitled to assert — that the request came from the channel +//! gateway, and where its reply should be delivered. +//! +//! Any transport that accepts caller-supplied JSON and forwards it to +//! `ChatService::send`/`send_sync` must strip these first. The list lives here, +//! next to the code that reads them, so a new marker cannot be added on the +//! reading side without the stripping side picking it up. + +/// Parameters that only the channel gateway may set. +/// +/// - `channel` — inbound sender metadata; drives per-sender tool policy and the +/// `private_context` history marker. +/// - `_channel_reply_target` — where the turn's output is delivered. A forged +/// value would let a caller address any chat the bot can reach. +/// - `_native_channel_request` — asserts the gateway already authorized this +/// turn, which skips the channel-bound public ceiling in +/// [`crate::service`]. This is the only one of the three that *widens* +/// privilege, so it is the one that most needs stripping. +pub const GATEWAY_OWNED_REQUEST_PARAMS: [&str; 3] = [ + "channel", + "_channel_reply_target", + "_native_channel_request", +]; + +/// Remove every [`GATEWAY_OWNED_REQUEST_PARAMS`] entry from a request. +/// +/// A no-op for anything that is not a JSON object. +pub fn strip_gateway_owned_params(params: &mut serde_json::Value) { + if let Some(object) = params.as_object_mut() { + for name in GATEWAY_OWNED_REQUEST_PARAMS { + object.remove(name); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn strips_every_gateway_owned_param_and_keeps_the_rest() { + let mut params = serde_json::json!({ + "text": "hello", + "channel": {"sender_id": "forged"}, + "_channel_reply_target": {"chat_id": "forged"}, + "_native_channel_request": true, + "_tool_policy": {"allow": ["calc"]}, + }); + + strip_gateway_owned_params(&mut params); + + for name in GATEWAY_OWNED_REQUEST_PARAMS { + assert!(params.get(name).is_none(), "{name} survived stripping"); + } + // Restrictions a caller applies to itself are not privilege, so they stay. + assert_eq!(params["_tool_policy"]["allow"][0], "calc"); + assert_eq!(params["text"], "hello"); + } + + #[test] + fn non_object_params_are_left_alone() { + let mut params = serde_json::json!("not an object"); + strip_gateway_owned_params(&mut params); + assert_eq!(params, serde_json::json!("not an object")); + } + + /// The reason this module exists: every name the chat service reads as a + /// gateway assertion must be on the strip list. `_native_channel_request` is + /// checked here explicitly because it is the only one that grants rather + /// than restricts, so omitting it would be a privilege bypass rather than a + /// routing bug. + #[test] + fn every_gateway_trust_marker_read_by_the_service_is_listed() { + const CHANNEL_SECURITY: &str = include_str!("service/chat_impl/channel_security.rs"); + + assert!( + CHANNEL_SECURITY.contains("_native_channel_request"), + "channel_security.rs no longer reads `_native_channel_request`; if the \ + trust marker was renamed, update GATEWAY_OWNED_REQUEST_PARAMS to match" + ); + assert!( + GATEWAY_OWNED_REQUEST_PARAMS.contains(&"_native_channel_request"), + "the gateway trust marker must be stripped from caller-supplied params" + ); + } +} diff --git a/crates/chat/src/run_with_tools.rs b/crates/chat/src/run_with_tools.rs index 22be1d4b31..e559836151 100644 --- a/crates/chat/src/run_with_tools.rs +++ b/crates/chat/src/run_with_tools.rs @@ -89,8 +89,14 @@ pub(crate) async fn run_with_tools( terminal_runs: &Arc>>, sender_name: Option, tool_controls: Option, + private_context: bool, ) -> Option { let run_started = Instant::now(); + let skills = if private_context { + skills + } else { + &[] + }; let runtime_limits = persona.config.agent_runtime_limits(agent_id); info!( agent_id, @@ -120,7 +126,13 @@ pub(crate) async fn run_with_tools( registry_guard.clone_without(&[]) } }; - if tools_enabled && let Some(manager) = state.memory_manager() { + // Agent-scoped memory tools are owner-private, so a public turn never gets + // them. `install_agent_scoped_memory_tools` only re-registers names it just + // unregistered, so it cannot reintroduce a tool the filters above removed. + if private_context + && tools_enabled + && let Some(manager) = state.memory_manager() + { install_agent_scoped_memory_tools( &mut filtered_registry, manager, @@ -143,7 +155,7 @@ pub(crate) async fn run_with_tools( // Before building the system prompt, query long-term memory with the // user's message and inject relevant results as ``. let mut memory_text_with_prefetch: Option = None; - if persona.config.memory.enable_prefetch { + if private_context && persona.config.memory.enable_prefetch { let query_text = match user_content { UserContent::Text(t) => Some(t.as_str()), UserContent::Multimodal(parts) => parts.iter().find_map(|p| match p { @@ -193,9 +205,32 @@ pub(crate) async fn run_with_tools( } } } - let effective_memory_text = memory_text_with_prefetch - .as_deref() - .or(persona.memory_text.as_deref()); + let effective_memory_text = private_context + .then(|| { + memory_text_with_prefetch + .as_deref() + .or(persona.memory_text.as_deref()) + }) + .flatten(); + + let project_context = private_context.then_some(project_context).flatten(); + let user = private_context.then_some(&persona.user); + let soul_text = private_context + .then_some(persona.soul_text.as_deref()) + .flatten(); + let boot_text = private_context + .then_some(persona.boot_text.as_deref()) + .flatten(); + let agents_text = private_context + .then_some(persona.agents_text.as_deref()) + .flatten(); + let tools_text = private_context + .then_some(persona.tools_text.as_deref()) + .flatten(); + let guidelines_text = private_context + .then_some(persona.guidelines_text.as_deref()) + .flatten(); + let prompt_runtime_context = private_context.then_some(runtime_context).flatten(); // Build system prompt: // - Native tools: full prompt with tool schemas sent via API @@ -209,30 +244,30 @@ pub(crate) async fn run_with_tools( project_context, skills, Some(&persona.identity), - Some(&persona.user), - persona.soul_text.as_deref(), - persona.boot_text.as_deref(), - persona.agents_text.as_deref(), - persona.tools_text.as_deref(), - runtime_context, + user, + soul_text, + boot_text, + agents_text, + tools_text, + prompt_runtime_context, effective_memory_text, prompt_limits, - persona.guidelines_text.as_deref(), + guidelines_text, ) .prompt } else { build_system_prompt_minimal_runtime_details( project_context, Some(&persona.identity), - Some(&persona.user), - persona.soul_text.as_deref(), - persona.boot_text.as_deref(), - persona.agents_text.as_deref(), - persona.tools_text.as_deref(), - runtime_context, + user, + soul_text, + boot_text, + agents_text, + tools_text, + prompt_runtime_context, effective_memory_text, prompt_limits, - persona.guidelines_text.as_deref(), + guidelines_text, ) .prompt }; @@ -881,9 +916,11 @@ pub(crate) async fn run_with_tools( // it stays positionally stable, preserving KV cache prefix matching for // local LLMs (llama.cpp, Ollama, LM Studio) and prompt-cache hits for // cloud providers. - let effective_user_content = - moltis_agents::prompt::prepend_datetime_to_user_content(user_content, runtime_context) - .unwrap_or_else(|| user_content.clone()); + let effective_user_content = moltis_agents::prompt::prepend_datetime_to_user_content( + user_content, + prompt_runtime_context, + ) + .unwrap_or_else(|| user_content.clone()); // Inject session key and accept-language into tool call params so tools can // resolve per-session state and forward the user's locale to web requests. @@ -891,7 +928,7 @@ pub(crate) async fn run_with_tools( session_key, accept_language.as_deref(), conn_id.as_deref(), - runtime_context, + prompt_runtime_context, ); if let Some(controls) = tool_controls { if let Some(active_tools) = controls.active_tools { @@ -944,7 +981,9 @@ pub(crate) async fn run_with_tools( // On context-window overflow, compact the session and retry once. let result = match first_result { - Err(AgentRunError::ContextWindowExceeded(ref msg)) if session_store.is_some() => { + Err(AgentRunError::ContextWindowExceeded(ref msg)) + if private_context && session_store.is_some() => + { let store = session_store?; info!( run_id, diff --git a/crates/chat/src/service/chat_impl.rs b/crates/chat/src/service/chat_impl.rs index 72d7c0a5ac..fa9bc85ad0 100644 --- a/crates/chat/src/service/chat_impl.rs +++ b/crates/chat/src/service/chat_impl.rs @@ -1,6 +1,10 @@ //! `ChatService` trait implementation for `LiveChatService`. +mod channel_security; +mod public_context; +mod queue_drain; mod send; +mod tool_policy; use std::{ collections::{HashMap, HashSet}, @@ -27,7 +31,7 @@ use { moltis_config::ToolMode, moltis_service_traits::{ChatService, ServiceError, ServiceResult}, moltis_sessions::{ContentBlock, MessageContent, PersistedMessage}, - moltis_tools::policy::{PolicyContext, ToolPolicy}, + moltis_tools::policy::PolicyContext, }; use crate::{ @@ -59,7 +63,7 @@ impl ChatService for LiveChatService { self.send_impl(params).await } - async fn send_sync(&self, params: Value) -> ServiceResult { + async fn send_sync(&self, mut params: Value) -> ServiceResult { let text = params .get("text") .and_then(|v| v.as_str()) @@ -72,12 +76,17 @@ impl ChatService for LiveChatService { .map(str::trim) .filter(|value| !value.is_empty()) .map(str::to_string); - let request_tool_policy = params - .get("_tool_policy") - .cloned() - .map(serde_json::from_value::) - .transpose() - .map_err(|e| format!("invalid '_tool_policy' parameter: {e}"))?; + // Resolve the session before request-scoped security so API callers + // cannot bypass a channel binding with an explicit session key. + let session_key = self.resolve_session_key_from_params(¶ms).await; + self.apply_channel_bound_public_context(&mut params, &session_key) + .await?; + let request_tool_policy = tool_policy::parse_request_tool_policy(¶ms)?; + let request_tool_audience = tool_policy::parse_request_tool_audience(¶ms)?; + let private_context = tool_policy::allows_private_context(¶ms); + if !private_context { + public_context::mark_public_channel(&mut params); + } let ephemeral = params .get("_ephemeral") .and_then(|v| v.as_bool()) @@ -88,12 +97,6 @@ impl ChatService for LiveChatService { moltis_config::schema::AgentToolControls::from_tool_context(Some(¶ms)); let stream_only = !self.has_tools_sync(); - // Resolve session key from explicit override. - let session_key = match params.get("_session_key").and_then(|v| v.as_str()) { - Some(sk) => sk.to_string(), - None => "main".to_string(), - }; - // Resolve provider. let provider: Arc = { let reg = self.providers.read().await; @@ -112,6 +115,7 @@ impl ChatService for LiveChatService { let user_audio = user_audio_path_from_params(¶ms, &session_key); let user_documents = user_documents_from_params(¶ms, &session_key, self.session_store.as_ref()); + let run_id = uuid::Uuid::new_v4().to_string(); // Persist the user message. let user_msg = PersistedMessage::User { content: MessageContent::Text(text.clone()), @@ -120,9 +124,9 @@ impl ChatService for LiveChatService { documents: user_documents .as_deref() .and_then(user_documents_for_persistence), - channel: None, + channel: params.get("channel").cloned(), seq: None, - run_id: None, + run_id: Some(run_id.clone()), }; if !ephemeral { if let Err(e) = self @@ -181,22 +185,23 @@ impl ChatService for LiveChatService { if !ephemeral && !history.is_empty() { history.pop(); } + let persisted_history_len = history.len(); + if !private_context { + history = public_context::filter_public_history(history); + } - let run_id = uuid::Uuid::new_v4().to_string(); let state = Arc::clone(&self.state); - let tool_registry = if let Some(policy) = request_tool_policy.as_ref() { - let registry_guard = self.tool_registry.read().await; - Arc::new(RwLock::new( - registry_guard.clone_allowed_by(|name| policy.is_allowed(name)), - )) - } else { - Arc::clone(&self.tool_registry) - }; + let tool_registry = tool_policy::resolve_request_tool_registry( + &self.tool_registry, + request_tool_policy.as_ref(), + request_tool_audience, + ) + .await; let hook_registry = self.hook_registry.clone(); let provider_name = provider.name().to_string(); let model_id = provider.id().to_string(); let model_store = Arc::clone(&self.model_store); - let user_message_index = history.len(); + let user_message_index = persisted_history_len; info!( run_id = %run_id, @@ -249,6 +254,7 @@ impl ChatService for LiveChatService { None, // send_sync: no client seq None, // send_sync: no partial assistant tracking &terminal_runs, + private_context, ) .await } else { @@ -283,6 +289,7 @@ impl ChatService for LiveChatService { &terminal_runs, None, // send_sync: no sender name Some(tool_controls), + private_context, ) .await }; diff --git a/crates/chat/src/service/chat_impl/channel_security.rs b/crates/chat/src/service/chat_impl/channel_security.rs new file mode 100644 index 0000000000..dffd4343da --- /dev/null +++ b/crates/chat/src/service/chat_impl/channel_security.rs @@ -0,0 +1,128 @@ +use serde_json::Value; + +use super::*; + +fn apply_public_context_ceiling(params: &mut Value) -> Result<(), String> { + tool_policy::parse_request_tool_policy(params)?; + tool_policy::apply_untrusted_request_context(params); + params["channel"] = serde_json::json!({ "sender_id": "web" }); + Ok(()) +} + +impl LiveChatService { + /// Apply the public-channel ceiling to web/API turns whose session can + /// deliver its response to a channel. Native channel requests already + /// carry a `channel` object and are authorized by the gateway. + /// + /// This applies to *every* non-native caller, including cron jobs, webhooks, + /// and the `sessions_send` tool — a channel-bound session's history contains + /// messages the bot did not author, so a turn over it cannot be trusted with + /// tools or owner-private context no matter who scheduled it. The downgrade + /// is logged because it is otherwise invisible: an affected cron job simply + /// answers as if it had no tools. Clear the binding + /// (`sessions.patch { channelBinding: null }`) to restore a session that was + /// attached to a channel and no longer needs to be. + pub(super) async fn apply_channel_bound_public_context( + &self, + params: &mut Value, + session_key: &str, + ) -> Result { + if params + .get("_native_channel_request") + .and_then(Value::as_bool) + == Some(true) + { + return Ok(false); + } + let Some(entry) = self.session_metadata.get(session_key).await else { + return Ok(false); + }; + let Some(binding_json) = entry.channel_binding.as_deref() else { + return Ok(false); + }; + + apply_public_context_ceiling(params)?; + tracing::info!( + session = session_key, + "session is bound to a channel; running this turn without tools or \ + private context. Clear the binding to restore full access." + ); + + match serde_json::from_str::(binding_json) { + Ok(target) => { + params["channel"]["channel_type"] = + Value::String(target.channel_type.as_str().to_string()); + let active_session = self + .session_metadata + .get_active_session( + target.channel_type.as_str(), + &target.account_id, + &target.chat_id, + target.thread_id.as_deref(), + ) + .await; + let is_active = active_session.as_deref().map_or_else( + || target.default_session_key() == session_key, + |active| active == session_key, + ); + if is_active { + params["_channel_reply_target"] = + serde_json::to_value(&target).map_err(|error| { + format!("failed to serialize channel reply target: {error}") + })?; + } else if let Some(object) = params.as_object_mut() { + object.remove("_channel_reply_target"); + } + }, + Err(error) => { + if let Some(object) = params.as_object_mut() { + object.remove("_channel_reply_target"); + } + tracing::warn!( + session = session_key, + %error, + "channel-bound request has an invalid reply target; keeping public context" + ); + }, + } + Ok(true) + } +} + +#[cfg(test)] +mod tests { + use super::apply_public_context_ceiling; + + #[test] + fn public_ceiling_overrides_caller_supplied_private_access() { + let mut params = serde_json::json!({ + "_private_context": true, + "_tool_policy": {"allow": ["*"]}, + }); + + assert!(apply_public_context_ceiling(&mut params).is_ok()); + + assert_eq!(params["_private_context"], false); + assert_eq!(params["channel"]["sender_id"], "web"); + assert_eq!(params["_tool_audience"], "public"); + assert_eq!(params["_tool_policy"]["deny"], serde_json::json!(["*"])); + } + + #[test] + fn public_ceiling_cannot_be_widened_by_request_policy() { + let mut params = serde_json::json!({ + "_tool_policy": {"allow": ["calc"]}, + }); + + assert!(apply_public_context_ceiling(&mut params).is_ok()); + + assert_eq!(params["_tool_audience"], "public"); + assert_eq!(params["_tool_policy"]["deny"], serde_json::json!(["*"])); + + let mut deny_all = serde_json::json!({ + "_tool_policy": {"deny": ["*"]}, + }); + assert!(apply_public_context_ceiling(&mut deny_all).is_ok()); + assert_eq!(deny_all["_tool_policy"]["deny"], serde_json::json!(["*"])); + } +} diff --git a/crates/chat/src/service/chat_impl/public_context.rs b/crates/chat/src/service/chat_impl/public_context.rs new file mode 100644 index 0000000000..c0e734c39b --- /dev/null +++ b/crates/chat/src/service/chat_impl/public_context.rs @@ -0,0 +1,289 @@ +use std::collections::HashSet; + +use serde_json::Value; + +pub(crate) fn mark_public_channel(params: &mut Value) { + if !params.get("channel").is_some_and(Value::is_object) { + params["channel"] = serde_json::json!({}); + } + if let Some(channel) = params.get_mut("channel").and_then(Value::as_object_mut) { + channel.insert("private_context".to_string(), Value::Bool(false)); + } +} + +/// Tool call ids requested by an assistant message, if any. +fn requested_tool_call_ids(message: &Value) -> Vec<&str> { + message + .get("tool_calls") + .and_then(Value::as_array) + .map(|calls| { + calls + .iter() + .filter_map(|call| call.get("id").and_then(Value::as_str)) + .collect() + }) + .unwrap_or_default() +} + +/// Keep only history belonging to runs that were themselves public. +/// +/// Runs are the unit of visibility: a message is kept when its `run_id` matches +/// a run whose user message was marked `channel.private_context = false`. +/// Messages with no `run_id` are never public. +/// +/// Further passes then repair tool-call pairing, in both directions. Providers +/// reject a `tool_calls` entry with no matching result *and* a tool result with +/// no preceding call, so a filter that can orphan either turns a privacy +/// decision into a hard request failure. Doing it here keeps the output valid by +/// construction, rather than relying on every persistence site remembering to +/// stamp a `run_id`. +pub(crate) fn filter_public_history(history: Vec) -> Vec { + let public_run_ids: HashSet<&str> = history + .iter() + .filter(|message| { + message + .get("channel") + .and_then(|channel| channel.get("private_context")) + .and_then(Value::as_bool) + == Some(false) + }) + .filter_map(|message| message.get("run_id").and_then(Value::as_str)) + .collect(); + + let public: Vec<&Value> = history + .iter() + .filter(|message| { + message + .get("run_id") + .and_then(Value::as_str) + .is_some_and(|run_id| public_run_ids.contains(run_id)) + }) + .collect(); + + // Results that survived the run filter, so we can tell which calls are + // fully answered. A partially answered call is as invalid as an unanswered + // one, so it is dropped whole. + let answered: HashSet<&str> = public + .iter() + .filter_map(|message| message.get("tool_call_id").and_then(Value::as_str)) + .collect(); + let kept: Vec<&&Value> = public + .iter() + .filter(|message| { + requested_tool_call_ids(message) + .iter() + .all(|id| answered.contains(id)) + }) + .collect(); + + // Dropping an assistant message can strand the results it asked for, so the + // reverse direction needs the same treatment. This cannot cascade: every + // surviving call had all of its results in `answered`, and only results no + // surviving call requested are removed here. + let requested: HashSet<&str> = kept + .iter() + .flat_map(|message| requested_tool_call_ids(message)) + .collect(); + + kept.iter() + .filter(|message| { + message + .get("tool_call_id") + .and_then(Value::as_str) + .is_none_or(|id| requested.contains(id)) + }) + .map(|message| (**message).clone()) + .collect() +} + +#[cfg(test)] +mod tests { + use super::{filter_public_history, mark_public_channel}; + + #[test] + fn public_marker_is_created_when_channel_metadata_is_absent() { + let mut params = serde_json::json!({"text": "webhook"}); + mark_public_channel(&mut params); + assert_eq!(params["channel"]["private_context"], false); + } + + #[test] + fn keeps_only_explicitly_public_runs() { + let history = vec![ + serde_json::json!({ + "role": "user", + "content": "private request", + "run_id": "private-run", + "channel": {"sender_id": "owner"}, + }), + serde_json::json!({ + "role": "assistant", + "content": "private response", + "run_id": "private-run", + }), + serde_json::json!({ + "role": "user", + "content": "public request", + "run_id": "public-run", + "channel": {"sender_id": "guest", "private_context": false}, + }), + serde_json::json!({ + "role": "assistant", + "content": "public response", + "run_id": "public-run", + }), + ]; + + let filtered = filter_public_history(history); + assert_eq!(filtered.len(), 2); + assert!( + filtered + .iter() + .all(|message| message["run_id"] == "public-run") + ); + } + + #[test] + fn keeps_a_tool_call_whose_result_is_in_the_same_run() { + let history = vec![ + serde_json::json!({ + "role": "user", + "content": "search", + "run_id": "public-run", + "channel": {"sender_id": "guest", "private_context": false}, + }), + serde_json::json!({ + "role": "assistant", + "tool_calls": [{ + "id": "call-1", + "type": "function", + "function": {"name": "web_search", "arguments": "{}"}, + }], + "run_id": "public-run", + }), + serde_json::json!({ + "role": "tool_result", + "tool_call_id": "call-1", + "tool_name": "web_search", + "run_id": "public-run", + }), + ]; + + assert_eq!(filter_public_history(history).len(), 3); + } + + /// A tool result persisted without a `run_id` is dropped by the run filter. + /// The assistant message that requested it must go too — a `tool_calls` + /// entry with no matching result is rejected by most providers, so leaving + /// it behind would turn a filtered turn into a failed request. + #[test] + fn drops_a_tool_call_whose_result_did_not_survive() { + let history = vec![ + serde_json::json!({ + "role": "user", + "content": "search", + "run_id": "public-run", + "channel": {"sender_id": "guest", "private_context": false}, + }), + serde_json::json!({ + "role": "assistant", + "tool_calls": [{ + "id": "call-1", + "type": "function", + "function": {"name": "web_search", "arguments": "{}"}, + }], + "run_id": "public-run", + }), + serde_json::json!({ + "role": "tool_result", + "tool_call_id": "call-1", + "tool_name": "web_search", + }), + serde_json::json!({ + "role": "assistant", + "content": "here you go", + "run_id": "public-run", + }), + ]; + + let filtered = filter_public_history(history); + assert_eq!(filtered.len(), 2, "{filtered:?}"); + assert!( + filtered + .iter() + .all(|message| message.get("tool_calls").is_none()), + "an unanswered tool call must not survive: {filtered:?}" + ); + } + + /// The reverse orphan: dropping a partially answered assistant message + /// strands the result that *did* survive. A tool result with no preceding + /// call is rejected just like an unanswered call, so it has to go too. + #[test] + fn drops_a_result_whose_calling_message_was_dropped() { + let history = vec![ + serde_json::json!({ + "role": "user", + "content": "two things", + "run_id": "public-run", + "channel": {"sender_id": "guest", "private_context": false}, + }), + serde_json::json!({ + "role": "assistant", + "tool_calls": [{"id": "call-1"}, {"id": "call-2"}], + "run_id": "public-run", + }), + // Only one of the two results carries the run id. + serde_json::json!({ + "role": "tool_result", + "tool_call_id": "call-1", + "run_id": "public-run", + }), + serde_json::json!({ + "role": "tool_result", + "tool_call_id": "call-2", + }), + ]; + + let filtered = filter_public_history(history); + assert!( + filtered + .iter() + .all(|message| message.get("tool_calls").is_none() + && message.get("tool_call_id").is_none()), + "neither half of a broken pair may survive: {filtered:?}" + ); + assert_eq!(filtered.len(), 1, "only the user message remains"); + } + + /// Partially answered calls are just as invalid as fully unanswered ones. + #[test] + fn drops_a_multi_call_message_when_any_result_is_missing() { + let history = vec![ + serde_json::json!({ + "role": "user", + "content": "two things", + "run_id": "public-run", + "channel": {"sender_id": "guest", "private_context": false}, + }), + serde_json::json!({ + "role": "assistant", + "tool_calls": [{"id": "call-1"}, {"id": "call-2"}], + "run_id": "public-run", + }), + serde_json::json!({ + "role": "tool_result", + "tool_call_id": "call-1", + "run_id": "public-run", + }), + ]; + + let filtered = filter_public_history(history); + assert!( + filtered + .iter() + .all(|message| message.get("tool_calls").is_none()), + "{filtered:?}" + ); + } +} diff --git a/crates/chat/src/service/chat_impl/queue_drain.rs b/crates/chat/src/service/chat_impl/queue_drain.rs new file mode 100644 index 0000000000..f971505d40 --- /dev/null +++ b/crates/chat/src/service/chat_impl/queue_drain.rs @@ -0,0 +1,289 @@ +//! Replay of messages queued while a session already had a run in flight. +//! +//! Both async send paths (the explicit-shell fast path and the agent loop) end +//! by draining this queue, and they must drain it identically — the two used to +//! be copy-pasted blocks, which is how a privilege bug reached only one of them. + +use std::{collections::HashMap, sync::Arc}; + +use { + serde_json::json, + tokio::sync::RwLock, + tracing::{info, warn}, +}; + +use moltis_config::MessageQueueMode; + +use crate::service::types::QueuedMessage; + +use super::tool_policy::split_by_request_security_context; + +/// Anything that can start the replayed turn — in practice the live chat +/// service resolved from gateway state. +#[async_trait::async_trait] +pub(crate) trait ReplaySink: Send + Sync { + async fn replay(&self, params: serde_json::Value) -> Result; +} + +#[async_trait::async_trait] +impl ReplaySink for Arc { + async fn replay(&self, params: serde_json::Value) -> Result { + self.send(params).await.map_err(|e| e.to_string()) + } +} + +/// Drain and replay whatever queued up for `session_key` while its run was +/// active. +/// +/// `Followup` replays one message per turn, each under its own params. +/// `Collect` merges a group of messages into a single turn, which is only safe +/// for messages that share an authorization context — see +/// [`split_by_request_security_context`]. Anything not replayed now goes back on the +/// queue for the next drain. +pub(crate) async fn drain_queued_messages( + queue: &Arc>>>, + session_key: &str, + mode: MessageQueueMode, + sink: &dyn ReplaySink, +) { + let queued = queue.write().await.remove(session_key).unwrap_or_default(); + if queued.is_empty() { + return; + } + + let (group, rest) = match mode { + MessageQueueMode::Followup => { + let mut group = queued; + let rest = group.split_off(1.min(group.len())); + (group, rest) + }, + // Merge only messages that share an authorization context. + MessageQueueMode::Collect => split_by_request_security_context(queued), + }; + + let Some(first) = group.first() else { + return; + }; + let collect_as_text = matches!(mode, MessageQueueMode::Collect) + && group.iter().all(|message| { + message + .params + .get("text") + .and_then(|value| value.as_str()) + .is_some() + && message.params.get("content").is_none() + && message.params.get("_audio_filename").is_none() + && message.params.get("_document_files").is_none() + }); + + let mut replay = if collect_as_text { + group + .last() + .map(|message| message.params.clone()) + .unwrap_or_else(|| first.params.clone()) + } else { + first.params.clone() + }; + + let mut remaining = if collect_as_text { + rest + } else { + group.iter().skip(1).cloned().chain(rest).collect() + }; + if !remaining.is_empty() { + requeue(queue, session_key, std::mem::take(&mut remaining)).await; + } + + replay["_queued_replay"] = json!(true); + match mode { + MessageQueueMode::Followup => { + info!(session = %session_key, "replaying queued message (followup)"); + }, + MessageQueueMode::Collect => { + if !collect_as_text { + info!(session = %session_key, "replaying queued non-text message individually"); + if let Err(e) = sink.replay(replay).await { + warn!(session = %session_key, error = %e, "failed to replay queued message"); + } + return; + } + let combined: Vec<&str> = group + .iter() + .filter_map(|message| message.params.get("text").and_then(|value| value.as_str())) + .collect(); + info!( + session = %session_key, + count = combined.len(), + "replaying collected messages" + ); + replay["text"] = json!(combined.join("\n\n")); + }, + } + + if let Err(e) = sink.replay(replay).await { + warn!(session = %session_key, error = %e, "failed to replay queued messages"); + } +} + +async fn requeue( + queue: &Arc>>>, + session_key: &str, + rest: Vec, +) { + queue + .write() + .await + .entry(session_key.to_string()) + .or_default() + .extend(rest); +} + +#[allow(clippy::unwrap_used, clippy::expect_used)] +#[cfg(test)] +mod tests { + use super::*; + + use {serde_json::Value, tokio::sync::Mutex}; + + #[derive(Default)] + struct RecordingSink(Mutex>); + + #[async_trait::async_trait] + impl ReplaySink for RecordingSink { + async fn replay(&self, params: Value) -> Result { + self.0.lock().await.push(params); + Ok(Value::Null) + } + } + + fn queued(text: &str, policy: Option) -> QueuedMessage { + let mut params = json!({ "text": text }); + if let Some(policy) = policy { + params["_tool_policy"] = policy; + } + QueuedMessage { params } + } + + fn guest_policy() -> Value { + json!({ "deny": ["exec"] }) + } + + fn queue_with( + messages: Vec, + ) -> Arc>>> { + Arc::new(RwLock::new(HashMap::from([("s".to_string(), messages)]))) + } + + async fn remaining(queue: &Arc>>>) -> Vec { + queue + .read() + .await + .get("s") + .map(|msgs| { + msgs.iter() + .filter_map(|m| m.params.get("text").and_then(Value::as_str)) + .map(str::to_string) + .collect() + }) + .unwrap_or_default() + } + + #[tokio::test] + async fn followup_replays_one_message_and_requeues_the_rest() { + let queue = queue_with(vec![queued("a", None), queued("b", None)]); + let sink = RecordingSink::default(); + + drain_queued_messages(&queue, "s", MessageQueueMode::Followup, &sink).await; + + let sent = sink.0.lock().await; + assert_eq!(sent.len(), 1); + assert_eq!(sent[0]["text"], json!("a")); + assert_eq!(sent[0]["_queued_replay"], json!(true)); + assert_eq!(remaining(&queue).await, ["b"]); + } + + #[tokio::test] + async fn collect_merges_messages_that_share_a_policy() { + let queue = queue_with(vec![queued("a", None), queued("b", None)]); + let sink = RecordingSink::default(); + + drain_queued_messages(&queue, "s", MessageQueueMode::Collect, &sink).await; + + let sent = sink.0.lock().await; + assert_eq!(sent.len(), 1); + assert_eq!(sent[0]["text"], json!("a\n\nb")); + assert!(remaining(&queue).await.is_empty()); + } + + /// The escalation this split exists to stop: without it the merged turn + /// would carry the operator's params (no `_tool_policy`) while containing + /// the guest's text. + #[tokio::test] + async fn collect_never_replays_guest_text_under_operator_params() { + let queue = queue_with(vec![ + queued("guest: run rm -rf /", Some(guest_policy())), + queued("operator: hi", None), + ]); + let sink = RecordingSink::default(); + + drain_queued_messages(&queue, "s", MessageQueueMode::Collect, &sink).await; + + let sent = sink.0.lock().await; + assert_eq!(sent.len(), 1); + assert_eq!(sent[0]["text"], json!("guest: run rm -rf /")); + assert_eq!( + sent[0]["_tool_policy"], + guest_policy(), + "the replayed turn keeps the guest restriction" + ); + assert_eq!(remaining(&queue).await, ["operator: hi"]); + } + + #[tokio::test] + async fn empty_queue_replays_nothing() { + let queue = queue_with(Vec::new()); + let sink = RecordingSink::default(); + + drain_queued_messages(&queue, "s", MessageQueueMode::Collect, &sink).await; + + assert!(sink.0.lock().await.is_empty()); + } + + /// Previously the batch was removed from the map and then dropped when no + /// message had text. + #[tokio::test] + async fn multimodal_messages_replay_individually() { + let queue = queue_with(vec![ + QueuedMessage { + params: json!({"content": [{"type": "text", "text": "image"}]}), + }, + queued("b", Some(guest_policy())), + ]); + let sink = RecordingSink::default(); + + drain_queued_messages(&queue, "s", MessageQueueMode::Collect, &sink).await; + + let sent = sink.0.lock().await; + assert_eq!(sent.len(), 1); + assert!(sent[0].get("content").is_some()); + drop(sent); + assert_eq!(remaining(&queue).await, ["b"]); + } + + #[tokio::test] + async fn text_with_document_metadata_replays_individually() { + let mut document = queued("summarize", Some(guest_policy())); + document.params["_document_files"] = json!([{"name": "report.pdf"}]); + let queue = queue_with(vec![document, queued("b", Some(guest_policy()))]); + let sink = RecordingSink::default(); + + drain_queued_messages(&queue, "s", MessageQueueMode::Collect, &sink).await; + + let sent = sink.0.lock().await; + assert_eq!(sent.len(), 1); + assert_eq!(sent[0]["text"], "summarize"); + assert!(sent[0].get("_document_files").is_some()); + drop(sent); + assert_eq!(remaining(&queue).await, ["b"]); + } +} diff --git a/crates/chat/src/service/chat_impl/send.rs b/crates/chat/src/service/chat_impl/send.rs index b8d33a0e34..de2c65fe64 100644 --- a/crates/chat/src/service/chat_impl/send.rs +++ b/crates/chat/src/service/chat_impl/send.rs @@ -8,7 +8,7 @@ use { tracing::{debug, info, warn}, }; -use {moltis_config::MessageQueueMode, moltis_service_traits::ServiceResult}; +use moltis_service_traits::ServiceResult; #[cfg(feature = "local-llm")] use moltis_providers::model_id::raw_model_id; @@ -101,6 +101,41 @@ impl LiveChatService { .get("_conn_id") .and_then(|v| v.as_str()) .map(String::from); + // Resolve session key from explicit overrides, public request params, or connection context. + let session_key = self.resolve_session_key_from_params(¶ms).await; + // Must be the same predicate the gateway authorizes on and the runner + // executes on. A local variant would let a form one side treats as + // `/sh` (e.g. `/sh@bot ls`) skip the untrusted ceiling here while + // running as an ordinary agent turn with the full registry. + let explicit_shell_command = match &message_content { + MessageContent::Text(raw) => moltis_agents::runner::explicit_shell_command(raw), + MessageContent::Multimodal(_) => None, + }; + let channel_bound_web = self + .apply_channel_bound_public_context(&mut params, &session_key) + .await?; + if channel_bound_web && explicit_shell_command.is_some() { + return Err( + "shell commands cannot run in a channel-bound web session; switch sessions first" + .into(), + ); + } + + // Request-scoped restrictions must be resolved after channel binding + // so web and native channel turns share the same execution boundary. + let request_tool_policy = tool_policy::parse_request_tool_policy(¶ms)?; + let request_tool_audience = tool_policy::parse_request_tool_audience(¶ms)?; + let private_context = tool_policy::allows_private_context(¶ms); + if !private_context { + public_context::mark_public_channel(&mut params); + } + let request_tool_registry = tool_policy::resolve_request_tool_registry( + &self.tool_registry, + request_tool_policy.as_ref(), + request_tool_audience, + ) + .await; + let explicit_model = params.get("model").and_then(|v| v.as_str()); let tool_controls = moltis_config::schema::AgentToolControls::from_tool_context(Some(¶ms)); @@ -118,8 +153,6 @@ impl LiveChatService { "send() mode decision" ); - // Resolve session key from explicit overrides, public request params, or connection context. - let session_key = self.resolve_session_key_from_params(¶ms).await; let queued_replay = params .get("_queued_replay") .and_then(|v| v.as_bool()) @@ -199,6 +232,20 @@ impl LiveChatService { p }, Err(_) => { + // Privileged shell requests must be authorized immediately + // before execution. Do not retain them for delayed replay after + // an operator may have been revoked. + if explicit_shell_command.is_some() { + return Err( + "shell commands cannot be queued; retry when the active run finishes" + .into(), + ); + } + // Authorization can change before replay. Delayed channel + // turns always execute with no tools or private context. + if tool_policy::downgrade_queued_channel_request(&mut params, private_context) { + public_context::mark_public_channel(&mut params); + } let queue_mode = message_queue_mode; let position = { let mut q = self.message_queue.write().await; @@ -236,11 +283,6 @@ impl LiveChatService { }, }; - let explicit_shell_command = match &message_content { - MessageContent::Text(raw) => parse_explicit_shell_command(raw).map(str::to_string), - MessageContent::Multimodal(_) => None, - }; - if let Some(shell_command) = explicit_shell_command { // Generate run_id early so we can link the user message to this run. let run_id = uuid::Uuid::new_v4().to_string(); @@ -274,46 +316,11 @@ impl LiveChatService { .touch(&session_key, history.len() as u32) .await; - // If this is a web UI message on a channel-bound session, attach the - // channel reply target so /sh output can be delivered back to the channel. - let is_web_message = conn_id.is_some() - && params.get("_session_key").is_none() - && params.get("channel").is_none(); - - if is_web_message - && let Some(entry) = self.session_metadata.get(&session_key).await - && let Some(ref binding_json) = entry.channel_binding - && let Ok(target) = - serde_json::from_str::(binding_json) - { - let is_active = self - .session_metadata - .get_active_session( - target.channel_type.as_str(), - &target.account_id, - &target.chat_id, - target.thread_id.as_deref(), - ) - .await - .map(|k| k == session_key) - .unwrap_or(true); - - if is_active { - match serde_json::to_value(&target) { - Ok(target_val) => { - params["_channel_reply_target"] = target_val; - }, - Err(e) => { - warn!( - session = %session_key, - error = %e, - "failed to serialize channel reply target for /sh" - ); - }, - } - } - } - + // `/sh` reaching this point can only be a native channel turn the + // gateway authorized, so its reply target is already in `params`. + // Web turns on a channel-bound session were rejected above, and + // `apply_channel_bound_public_context` is what derives a target from + // the session's binding for every other request. let deferred_channel_target = params .get("_channel_reply_target") @@ -373,7 +380,7 @@ impl LiveChatService { let terminal_runs = Arc::clone(&self.terminal_runs); let session_store = Arc::clone(&self.session_store); let session_metadata = Arc::clone(&self.session_metadata); - let tool_registry = Arc::clone(&self.tool_registry); + let tool_registry = Arc::clone(&request_tool_registry); let session_key_clone = session_key.clone(); let message_queue = Arc::clone(&self.message_queue); let state_for_drain = Arc::clone(&self.state); @@ -446,60 +453,14 @@ impl LiveChatService { drop(permit); // Drain queued messages for this session. - let queued = message_queue - .write() - .await - .remove(&session_key_clone) - .unwrap_or_default(); - if !queued.is_empty() { - let queue_mode = message_queue_mode; - let chat = state_for_drain.chat_service().await; - match queue_mode { - MessageQueueMode::Followup => { - let mut iter = queued.into_iter(); - let Some(first) = iter.next() else { - return; - }; - let rest: Vec = iter.collect(); - if !rest.is_empty() { - message_queue - .write() - .await - .entry(session_key_clone.clone()) - .or_default() - .extend(rest); - } - info!(session = %session_key_clone, "replaying queued message (followup)"); - let mut replay_params = first.params; - replay_params["_queued_replay"] = serde_json::json!(true); - if let Err(e) = chat.send(replay_params).await { - warn!(session = %session_key_clone, error = %e, "failed to replay queued message"); - } - }, - MessageQueueMode::Collect => { - let combined: Vec<&str> = queued - .iter() - .filter_map(|m| m.params.get("text").and_then(|v| v.as_str())) - .collect(); - if !combined.is_empty() { - info!( - session = %session_key_clone, - count = combined.len(), - "replaying collected messages" - ); - let Some(last) = queued.last() else { - return; - }; - let mut merged = last.params.clone(); - merged["text"] = serde_json::json!(combined.join("\n\n")); - merged["_queued_replay"] = serde_json::json!(true); - if let Err(e) = chat.send(merged).await { - warn!(session = %session_key_clone, error = %e, "failed to replay collected messages"); - } - } - }, - } - } + let chat = state_for_drain.chat_service().await; + queue_drain::drain_queued_messages( + &message_queue, + &session_key_clone, + message_queue_mode, + &chat, + ) + .await; }); self.active_runs @@ -606,9 +567,12 @@ impl LiveChatService { } // Resolve project context plus optional command-generated context. - let project_context = self - .resolve_turn_context(&session_key, conn_id.as_deref()) - .await; + let project_context = if private_context { + self.resolve_turn_context(&session_key, conn_id.as_deref()) + .await + } else { + None + }; // Generate run_id early so we can link the user message to its agent run. let run_id = uuid::Uuid::new_v4().to_string(); @@ -620,6 +584,10 @@ impl LiveChatService { .read(&session_key) .await .unwrap_or_default(); + let persisted_history_len = history.len(); + if !private_context { + history = public_context::filter_public_history(history); + } info!( session = %session_key, history_len = history.len(), @@ -630,51 +598,9 @@ impl LiveChatService { // Update metadata. let _ = self.session_metadata.upsert(&session_key, None).await; self.session_metadata - .touch(&session_key, history.len() as u32) + .touch(&session_key, persisted_history_len as u32) .await; - // If this is a web UI message on a channel-bound session, attach the - // channel reply target so the run-start path can route the final - // response back to the channel. - let is_web_message = conn_id.is_some() - && params.get("_session_key").is_none() - && params.get("channel").is_none(); - - if is_web_message - && let Some(entry) = self.session_metadata.get(&session_key).await - && let Some(ref binding_json) = entry.channel_binding - && let Ok(target) = - serde_json::from_str::(binding_json) - { - // Only echo to channel if this is the active session for this chat. - let is_active = self - .session_metadata - .get_active_session( - target.channel_type.as_str(), - &target.account_id, - &target.chat_id, - target.thread_id.as_deref(), - ) - .await - .map(|k| k == session_key) - .unwrap_or(true); - - if is_active { - match serde_json::to_value(&target) { - Ok(target_val) => { - params["_channel_reply_target"] = target_val; - }, - Err(e) => { - warn!( - session = %session_key, - error = %e, - "failed to serialize channel reply target" - ); - }, - } - } - } - let deferred_channel_target = params .get("_channel_reply_target") @@ -921,7 +847,7 @@ impl LiveChatService { let active_partial_assistant = Arc::clone(&self.active_partial_assistant); let active_reply_medium = Arc::clone(&self.active_reply_medium); let run_id_clone = run_id.clone(); - let tool_registry = Arc::clone(&self.tool_registry); + let tool_registry = Arc::clone(&request_tool_registry); let hook_registry = self.hook_registry.clone(); // Log if tool mode is active but the provider doesn't support tools. @@ -948,7 +874,7 @@ impl LiveChatService { // Capture user message index (0-based) so we can include assistant // message index in the "final" broadcast for client-side deduplication. - let user_message_index = history.len(); // user msg is at this index in the JSONL + let user_message_index = persisted_history_len; // user msg is at this index in the JSONL let provider_name = provider.name().to_string(); let model_id = provider.id().to_string(); @@ -987,7 +913,7 @@ impl LiveChatService { let compact_threshold = compute_auto_compact_threshold(context_window, compaction_cfg.threshold_percent); - if estimated_next_input >= compact_threshold { + if private_context && estimated_next_input >= compact_threshold { let pre_compact_msg_count = history.len(); let pre_compact_total = token_usage .current_request_input_tokens @@ -1191,6 +1117,7 @@ impl LiveChatService { client_seq, Some(Arc::clone(&active_partial_assistant)), &terminal_runs, + private_context, ) .await } else { @@ -1225,6 +1152,7 @@ impl LiveChatService { &terminal_runs, sender_name, Some(tool_controls), + private_context, ) .await } @@ -1299,7 +1227,8 @@ impl LiveChatService { let max_tool_result_bytes = extraction_max_tool_result_bytes; // A "turn" = user + assistant = 2 messages. let turn_number = count / 2; - if interval > 0 + if private_context + && interval > 0 && turn_number > 0 && turn_number % interval == 0 && !stream_only @@ -1366,7 +1295,8 @@ impl LiveChatService { // generation. We check >= 2 (not == 2) because agentic turns // with tool calls produce more than 2 stored messages. // `generate_title_if_needed` guards against duplicate titles. - if auto_title_enabled + if private_context + && auto_title_enabled && let Ok(count) = session_store.count(&session_key_clone).await && count >= 2 && !queued_replay @@ -1404,63 +1334,14 @@ impl LiveChatService { drop(permit); // Drain queued messages for this session. - let queued = message_queue - .write() - .await - .remove(&session_key_clone) - .unwrap_or_default(); - if !queued.is_empty() { - let queue_mode = message_queue_mode; - let chat = state_for_drain.chat_service().await; - match queue_mode { - MessageQueueMode::Followup => { - let mut iter = queued.into_iter(); - let Some(first) = iter.next() else { - return; - }; - // Put remaining messages back so the replayed run's - // own drain loop picks them up after it completes. - let rest: Vec = iter.collect(); - if !rest.is_empty() { - message_queue - .write() - .await - .entry(session_key_clone.clone()) - .or_default() - .extend(rest); - } - info!(session = %session_key_clone, "replaying queued message (followup)"); - let mut replay_params = first.params; - replay_params["_queued_replay"] = serde_json::json!(true); - if let Err(e) = chat.send(replay_params).await { - warn!(session = %session_key_clone, error = %e, "failed to replay queued message"); - } - }, - MessageQueueMode::Collect => { - let combined: Vec<&str> = queued - .iter() - .filter_map(|m| m.params.get("text").and_then(|v| v.as_str())) - .collect(); - if !combined.is_empty() { - info!( - session = %session_key_clone, - count = combined.len(), - "replaying collected messages" - ); - // Use the last queued message as the base params, override text. - let Some(last) = queued.last() else { - return; - }; - let mut merged = last.params.clone(); - merged["text"] = serde_json::json!(combined.join("\n\n")); - merged["_queued_replay"] = serde_json::json!(true); - if let Err(e) = chat.send(merged).await { - warn!(session = %session_key_clone, error = %e, "failed to replay collected messages"); - } - } - }, - } - } + let chat = state_for_drain.chat_service().await; + queue_drain::drain_queued_messages( + &message_queue, + &session_key_clone, + message_queue_mode, + &chat, + ) + .await; }); self.active_runs diff --git a/crates/chat/src/service/chat_impl/tool_policy.rs b/crates/chat/src/service/chat_impl/tool_policy.rs new file mode 100644 index 0000000000..5e904523a2 --- /dev/null +++ b/crates/chat/src/service/chat_impl/tool_policy.rs @@ -0,0 +1,589 @@ +//! Request-scoped tool restriction for `chat.send` / `chat.send_sync`. +//! +//! Callers that cannot trust the requester — the channel gateway and webhooks +//! — pass `_tool_audience = "public"`. The registry filters by host-owned tool +//! metadata before applying an optional `_tool_policy` name filter. Both are +//! restrictions only and compose with configured policy layers applied later +//! in `apply_runtime_tool_filters`, which can remove more but never add back. +//! +//! Both send paths must apply it. `send()` (async, used by every channel turn) +//! previously ignored the parameter while `send_sync()` honoured it, which +//! silently disabled the restriction on exactly the untrusted path it exists +//! for. + +use std::sync::Arc; + +use {serde_json::Value, tokio::sync::RwLock}; + +use moltis_agents::tool_registry::{ToolAudience, ToolRegistry}; + +use moltis_tools::policy::ToolPolicy; + +use crate::service::types::QueuedMessage; + +/// Parse the request's `_tool_policy`, if present. +/// +/// Returns `Err` on a malformed policy rather than ignoring it — a caller that +/// meant to restrict tools must not silently get an unrestricted run. +pub(crate) fn parse_request_tool_policy(params: &Value) -> Result, String> { + params + .get("_tool_policy") + .cloned() + .map(serde_json::from_value::) + .transpose() + .map_err(|e| format!("invalid '_tool_policy' parameter: {e}")) +} + +/// Parse the host-owned audience ceiling for this request. +/// +/// Trusted requests omit the marker. Callers may only opt into the more +/// restrictive public audience; any other value is rejected rather than +/// risking a fail-open interpretation. +pub(crate) fn parse_request_tool_audience(params: &Value) -> Result { + match params.get("_tool_audience") { + None => Ok(ToolAudience::Trusted), + Some(Value::String(value)) if value == "public" => Ok(ToolAudience::Public), + Some(_) => Err("invalid '_tool_audience' parameter: expected 'public'".to_string()), + } +} + +/// Apply the fail-closed context used for delayed or channel-bound requests +/// that cannot retain caller authorization. +pub(crate) fn apply_untrusted_request_context(params: &mut Value) { + params["_tool_audience"] = Value::String("public".to_string()); + params["_tool_policy"] = serde_json::json!({ "deny": ["*"] }); + params["_private_context"] = Value::Bool(false); +} + +/// Downgrade a privileged channel turn before delayed replay, when its sender's +/// authorization may no longer be valid. +#[must_use] +pub(crate) fn downgrade_queued_channel_request(params: &mut Value, private_context: bool) -> bool { + if !private_context || !params.get("channel").is_some_and(Value::is_object) { + return false; + } + apply_untrusted_request_context(params); + true +} + +/// Resolve the tool registry for one run, applying its audience ceiling and +/// optional name policy. +/// +/// Without a policy this hands back the shared registry unchanged (no clone of +/// the tool set, no behaviour change for trusted callers such as the web UI). +pub(crate) async fn resolve_request_tool_registry( + base: &Arc>, + policy: Option<&ToolPolicy>, + audience: ToolAudience, +) -> Arc> { + if policy.is_none() && audience == ToolAudience::Trusted { + return Arc::clone(base); + } + let registry = base.read().await; + let audience_registry = registry.clone_for_audience(audience); + let filtered = match policy { + Some(policy) => audience_registry.clone_allowed_by(|name| policy.is_allowed(name)), + None => audience_registry, + }; + Arc::new(RwLock::new(filtered)) +} + +/// Whether this request may receive owner-private prompt and memory context. +pub(crate) fn allows_private_context(params: &Value) -> bool { + if params.get("_tool_audience").and_then(Value::as_str) == Some("public") { + return false; + } + match params.get("_private_context") { + None => true, + Some(Value::Bool(value)) => *value, + Some(_) => false, + } +} + +fn request_security_context( + params: &Value, +) -> ( + Option, + Option, + bool, + Option, + Option, +) { + let reply_scope = params.get("_channel_reply_target").map(|target| { + serde_json::json!({ + "channel_type": target.get("channel_type"), + "account_id": target.get("account_id"), + "chat_id": target.get("chat_id"), + "thread_id": target.get("thread_id"), + "message_id": target.get("message_id"), + }) + }); + ( + params.get("_tool_audience").cloned(), + params.get("_tool_policy").cloned(), + allows_private_context(params), + params + .get("channel") + .and_then(|channel| channel.get("sender_id")) + .cloned(), + reply_scope, + ) +} + +/// Split queued messages into the leading group that shares one authorization +/// context, plus the remainder. +/// +/// `MessageQueueMode::Collect` joins the text of every message in a group into +/// a single turn and runs it under one set of params. Messages from senders of +/// different principals or privilege must therefore never share a group: a +/// shared channel session queues multiple senders side by side, and merging +/// them would attribute all text to the final sender and its policy. +/// +/// The caller replays the returned group and puts the remainder back on the +/// queue, where the next drain replays it under its own policy. Splitting +/// rather than merging policies keeps the rule simple: a turn never runs with +/// more privilege than the sender of any line in it. +pub(crate) fn split_by_request_security_context( + mut queued: Vec, +) -> (Vec, Vec) { + let Some(head_context) = queued + .first() + .map(|message| request_security_context(&message.params)) + else { + return (queued, Vec::new()); + }; + let split = queued + .iter() + .position(|message| request_security_context(&message.params) != head_context) + .unwrap_or(queued.len()); + let rest = queued.split_off(split); + (queued, rest) +} + +#[allow(clippy::unwrap_used, clippy::expect_used)] +#[cfg(test)] +mod tests { + use super::*; + + use {async_trait::async_trait, moltis_agents::tool_registry::AgentTool}; + + struct StubTool(&'static str); + + #[async_trait] + impl AgentTool for StubTool { + fn name(&self) -> &str { + self.0 + } + + fn description(&self) -> &str { + "stub" + } + + fn parameters_schema(&self) -> Value { + serde_json::json!({"type": "object"}) + } + + async fn execute(&self, _args: Value) -> anyhow::Result { + Ok(Value::Null) + } + } + + fn registry_with(names: &[&'static str]) -> Arc> { + let mut registry = ToolRegistry::new(); + for name in names { + registry.register(Box::new(StubTool(name))); + } + Arc::new(RwLock::new(registry)) + } + + #[test] + fn absent_policy_parses_to_none() { + let params = serde_json::json!({ "text": "hi" }); + assert!( + parse_request_tool_policy(¶ms) + .expect("absent policy is not an error") + .is_none() + ); + } + + #[test] + fn deny_list_parses() { + let params = serde_json::json!({ "_tool_policy": { "deny": ["exec", "memory_*"] } }); + let policy = parse_request_tool_policy(¶ms) + .expect("valid policy") + .expect("policy present"); + assert!(!policy.is_allowed("exec")); + assert!(!policy.is_allowed("memory_save")); + assert!(policy.is_allowed("web_search")); + } + + #[test] + fn malformed_policy_is_rejected_not_ignored() { + let params = serde_json::json!({ "_tool_policy": "not-an-object" }); + assert!(parse_request_tool_policy(¶ms).is_err()); + } + + #[test] + fn audience_defaults_to_trusted_and_only_accepts_public_restriction() { + assert_eq!( + parse_request_tool_audience(&serde_json::json!({})), + Ok(ToolAudience::Trusted) + ); + assert_eq!( + parse_request_tool_audience(&serde_json::json!({"_tool_audience": "public"})), + Ok(ToolAudience::Public) + ); + assert!( + parse_request_tool_audience(&serde_json::json!({"_tool_audience": "trusted"})).is_err() + ); + assert!(parse_request_tool_audience(&serde_json::json!({"_tool_audience": true})).is_err()); + } + + #[test] + fn private_context_defaults_on_and_can_be_disabled() { + assert!(allows_private_context(&serde_json::json!({}))); + assert!(!allows_private_context( + &serde_json::json!({"_private_context": false}) + )); + assert!(!allows_private_context( + &serde_json::json!({"_private_context": "false"}) + )); + assert!(!allows_private_context( + &serde_json::json!({"_private_context": null}) + )); + assert!(!allows_private_context( + &serde_json::json!({"_tool_audience": "public", "_private_context": true}) + )); + } + + #[test] + fn untrusted_request_context_denies_all_tools() { + let mut params = serde_json::json!({ + "_tool_policy": {"allow": ["*"]}, + "_private_context": true, + }); + + apply_untrusted_request_context(&mut params); + + assert_eq!(params["_tool_audience"], "public"); + assert_eq!(params["_tool_policy"]["deny"], serde_json::json!(["*"])); + assert_eq!(params["_private_context"], false); + } + + #[test] + fn queued_channel_downgrade_removes_tools_and_private_context() { + let mut params = serde_json::json!({ + "channel": {"sender_id": "owner"}, + "_private_context": true, + }); + + assert!(downgrade_queued_channel_request(&mut params, true)); + assert_eq!(params["_tool_audience"], "public"); + assert_eq!(params["_tool_policy"]["deny"], serde_json::json!(["*"])); + assert_eq!(params["_private_context"], false); + + assert!(!downgrade_queued_channel_request( + &mut serde_json::json!({}), + true + )); + } + + #[tokio::test] + async fn no_policy_returns_the_shared_registry() { + let base = registry_with(&["exec", "web_search"]); + let resolved = resolve_request_tool_registry(&base, None, ToolAudience::Trusted).await; + assert!( + Arc::ptr_eq(&base, &resolved), + "trusted callers must keep the shared registry" + ); + } + + /// Regression guard for the bug this module was extracted to fix. + /// + /// `send_sync` honoured `_tool_policy` while `send` (the async path every + /// channel turn uses) ignored it and ran the agent with the shared + /// registry, so the guest restriction was a no-op on exactly the untrusted + /// path it exists for. A behavioural test would need a live provider and + /// database, so assert the structural invariant instead: neither send path + /// may hand a run the unfiltered `self.tool_registry`. + /// + /// If you are refactoring and this fails, keep the invariant rather than + /// deleting the test — every run started from a send path must receive the + /// registry returned by `resolve_request_tool_registry`. + #[test] + fn both_send_paths_apply_request_tool_security() { + const SEND_ASYNC: &str = include_str!("send.rs"); + const SEND_SYNC: &str = include_str!("../chat_impl.rs"); + + for (path, src) in [ + ("chat_impl/send.rs", SEND_ASYNC), + ("chat_impl.rs", SEND_SYNC), + ] { + assert!( + src.contains("resolve_request_tool_registry"), + "{path} must resolve the request-scoped tool registry" + ); + assert!( + src.contains("parse_request_tool_audience"), + "{path} must parse the request tool audience" + ); + assert!( + !src.contains("Arc::clone(&self.tool_registry)"), + "{path} passes the unfiltered shared registry to a run — a caller's \ + `_tool_policy` restriction would be silently ignored. Use the registry \ + from resolve_request_tool_registry instead." + ); + } + } + + /// The gateway skips the untrusted tool ceiling for an operator direct-chat + /// turn it recognizes as an explicit `/sh`, on the promise that the chat + /// layer will then run it through the deterministic shell path instead of + /// the agent loop. If the two disagree, forms only the gateway recognizes — + /// `/sh@bot ls`, `/SH ls` — arrive here as ordinary prose *and* keep the + /// unrestricted registry and private context, which is the opposite of what + /// the ceiling exists for. + /// + /// Keep one predicate. Do not reintroduce a local variant. + #[test] + fn send_path_detects_shell_commands_exactly_as_the_gateway_does() { + const SEND_ASYNC: &str = include_str!("send.rs"); + + assert!( + SEND_ASYNC.contains("moltis_agents::runner::explicit_shell_command"), + "send.rs must detect `/sh` with the same predicate the gateway \ + authorizes on and the runner executes on" + ); + + // Forms the gateway treats as `/sh`. Each must be recognized here too, + // or it bypasses the ceiling without taking the shell path. + for input in ["/sh ls", " /sh ls", "/SH ls", "/sh@mybot ls"] { + assert!( + moltis_agents::runner::explicit_shell_command(input).is_some(), + "{input} must be recognized as an explicit shell command" + ); + } + for input in ["explain what /sh does", "/shell ls", "/sh", "/sh "] { + assert!( + moltis_agents::runner::explicit_shell_command(input).is_none(), + "{input} must not be treated as an explicit shell command" + ); + } + } + + fn queued(text: &str, policy: Option) -> QueuedMessage { + let mut params = serde_json::json!({ "text": text }); + if let Some(policy) = policy { + params["_tool_policy"] = policy; + } + QueuedMessage { params } + } + + fn public_queued(text: &str, policy: Option) -> QueuedMessage { + let mut message = queued(text, policy); + message.params["_tool_audience"] = serde_json::json!("public"); + message + } + + fn guest_policy() -> Value { + serde_json::json!({ "deny": ["exec", "memory_*"] }) + } + + fn texts(messages: &[QueuedMessage]) -> Vec<&str> { + messages + .iter() + .filter_map(|m| m.params.get("text").and_then(Value::as_str)) + .collect() + } + + #[test] + fn same_policy_messages_stay_in_one_group() { + let (group, rest) = split_by_request_security_context(vec![ + queued("a", Some(guest_policy())), + queued("b", Some(guest_policy())), + ]); + assert_eq!(texts(&group), ["a", "b"]); + assert!(rest.is_empty()); + } + + #[test] + fn unrestricted_messages_stay_in_one_group() { + let (group, rest) = + split_by_request_security_context(vec![queued("a", None), queued("b", None)]); + assert_eq!(texts(&group), ["a", "b"]); + assert!(rest.is_empty()); + } + + /// The escalation this split exists to stop: Collect replay runs a merged + /// turn under the *last* message's params, so a guest message followed by + /// an operator message would execute the guest's text with no + /// `_tool_policy` at all. + #[test] + fn guest_text_is_never_merged_into_an_operator_turn() { + let (group, rest) = split_by_request_security_context(vec![ + queued("rm -rf /", Some(guest_policy())), + queued("hi", None), + ]); + assert_eq!(texts(&group), ["rm -rf /"], "guest message replays alone"); + assert_eq!(texts(&rest), ["hi"], "operator message is requeued"); + assert_eq!( + group.last().map(|m| m.params.get("_tool_policy").cloned()), + Some(Some(guest_policy())), + "the replayed group keeps the guest restriction" + ); + } + + #[test] + fn operator_group_splits_before_a_guest_message() { + let (group, rest) = split_by_request_security_context(vec![ + queued("a", None), + queued("b", None), + queued("c", Some(guest_policy())), + ]); + assert_eq!(texts(&group), ["a", "b"]); + assert_eq!(texts(&rest), ["c"]); + } + + #[test] + fn differing_policies_split_apart() { + let other = serde_json::json!({ "deny": ["exec"] }); + let (group, rest) = split_by_request_security_context(vec![ + queued("a", Some(guest_policy())), + queued("b", Some(other)), + ]); + assert_eq!(texts(&group), ["a"]); + assert_eq!(texts(&rest), ["b"]); + } + + #[test] + fn same_policy_from_different_senders_splits_apart() { + let mut alice = queued("a", Some(guest_policy())); + alice.params["channel"] = serde_json::json!({"sender_id": "alice"}); + let mut bob = queued("b", Some(guest_policy())); + bob.params["channel"] = serde_json::json!({"sender_id": "bob"}); + + let (group, rest) = split_by_request_security_context(vec![alice, bob]); + assert_eq!(texts(&group), ["a"]); + assert_eq!(texts(&rest), ["b"]); + } + + #[test] + fn private_and_public_contexts_split_apart() { + let private = queued("a", Some(guest_policy())); + let mut public = queued("b", Some(guest_policy())); + public.params["_private_context"] = serde_json::json!(false); + + let (group, rest) = split_by_request_security_context(vec![private, public]); + assert_eq!(texts(&group), ["a"]); + assert_eq!(texts(&rest), ["b"]); + } + + #[test] + fn public_and_trusted_audiences_split_apart() { + let (group, rest) = split_by_request_security_context(vec![ + public_queued("public", Some(guest_policy())), + queued("trusted", Some(guest_policy())), + ]); + assert_eq!(texts(&group), ["public"]); + assert_eq!(texts(&rest), ["trusted"]); + } + + #[test] + fn different_thread_roots_split_apart() { + let mut first = queued("a", Some(guest_policy())); + first.params["_channel_reply_target"] = serde_json::json!({ + "channel_type": "slack", + "account_id": "bot", + "chat_id": "C123", + "message_id": "thread-a", + }); + let mut second = queued("b", Some(guest_policy())); + second.params["_channel_reply_target"] = serde_json::json!({ + "channel_type": "slack", + "account_id": "bot", + "chat_id": "C123", + "message_id": "thread-b", + }); + + let (group, rest) = split_by_request_security_context(vec![first, second]); + assert_eq!(texts(&group), ["a"]); + assert_eq!(texts(&rest), ["b"]); + } + + #[test] + fn empty_queue_splits_into_nothing() { + let (group, rest) = split_by_request_security_context(Vec::new()); + assert!(group.is_empty()); + assert!(rest.is_empty()); + } + + #[tokio::test] + async fn policy_filters_denied_tools_out_of_the_registry() { + let base = registry_with(&["exec", "memory_save", "web_search"]); + let policy = ToolPolicy { + allow: Vec::new(), + deny: vec!["exec".into(), "memory_*".into()], + }; + + let resolved = + resolve_request_tool_registry(&base, Some(&policy), ToolAudience::Trusted).await; + let names = resolved.read().await.list_names(); + + assert!(!names.iter().any(|n| n == "exec")); + assert!(!names.iter().any(|n| n == "memory_save")); + assert!(names.iter().any(|n| n == "web_search")); + + // The shared registry must be untouched — the filter is per-run. + assert_eq!(base.read().await.list_names().len(), 3); + } + + #[tokio::test] + async fn public_audience_cannot_be_widened_by_name_policy() { + let mut registry = ToolRegistry::new(); + registry.register_public(Box::new(StubTool("calc"))); + registry.register(Box::new(StubTool("future_tool"))); + registry.register_mcp(Box::new(StubTool("mcp__example__read")), "example".into()); + let base = Arc::new(RwLock::new(registry)); + let permissive = ToolPolicy { + allow: vec!["*".into()], + deny: Vec::new(), + }; + + let resolved = + resolve_request_tool_registry(&base, Some(&permissive), ToolAudience::Public).await; + assert_eq!(resolved.read().await.list_names(), vec!["calc"]); + } + + #[tokio::test] + async fn deny_all_public_request_has_no_tools() { + let mut registry = ToolRegistry::new(); + registry.register_public(Box::new(StubTool("calc"))); + registry.register(Box::new(StubTool("exec"))); + let base = Arc::new(RwLock::new(registry)); + let deny_all = ToolPolicy { + allow: Vec::new(), + deny: vec!["*".into()], + }; + + let resolved = + resolve_request_tool_registry(&base, Some(&deny_all), ToolAudience::Public).await; + assert!(resolved.read().await.is_empty()); + } + + #[tokio::test] + async fn narrow_public_request_keeps_only_named_public_tool() { + let mut registry = ToolRegistry::new(); + registry.register_public(Box::new(StubTool("calc"))); + registry.register_public(Box::new(StubTool("web_search"))); + registry.register(Box::new(StubTool("exec"))); + let base = Arc::new(RwLock::new(registry)); + let calc_only = ToolPolicy { + allow: vec!["calc".into()], + deny: Vec::new(), + }; + + let resolved = + resolve_request_tool_registry(&base, Some(&calc_only), ToolAudience::Public).await; + assert_eq!(resolved.read().await.list_names(), vec!["calc"]); + } +} diff --git a/crates/chat/src/streaming.rs b/crates/chat/src/streaming.rs index 631954706e..026bb3329b 100644 --- a/crates/chat/src/streaming.rs +++ b/crates/chat/src/streaming.rs @@ -135,12 +135,13 @@ pub(crate) async fn run_streaming( client_seq: Option, active_partial_assistant: Option>>>, terminal_runs: &Arc>>, + private_context: bool, ) -> Option { let run_started = Instant::now(); // ── Memory prefetch (same logic as run_with_tools) ─────────── let mut memory_text_with_prefetch: Option = None; - if persona.config.memory.enable_prefetch { + if private_context && persona.config.memory.enable_prefetch { let query_text = match user_content { UserContent::Text(t) => Some(t.as_str()), UserContent::Multimodal(parts) => parts.iter().find_map(|p| match p { @@ -180,22 +181,37 @@ pub(crate) async fn run_streaming( } } } - let effective_memory_text = memory_text_with_prefetch - .as_deref() - .or(persona.memory_text.as_deref()); + let effective_memory_text = private_context + .then(|| { + memory_text_with_prefetch + .as_deref() + .or(persona.memory_text.as_deref()) + }) + .flatten(); + let prompt_runtime_context = private_context.then_some(runtime_context).flatten(); let system_prompt = build_system_prompt_minimal_runtime_details( - project_context, + private_context.then_some(project_context).flatten(), Some(&persona.identity), - Some(&persona.user), - persona.soul_text.as_deref(), - persona.boot_text.as_deref(), - persona.agents_text.as_deref(), - persona.tools_text.as_deref(), - runtime_context, + private_context.then_some(&persona.user), + private_context + .then_some(persona.soul_text.as_deref()) + .flatten(), + private_context + .then_some(persona.boot_text.as_deref()) + .flatten(), + private_context + .then_some(persona.agents_text.as_deref()) + .flatten(), + private_context + .then_some(persona.tools_text.as_deref()) + .flatten(), + prompt_runtime_context, effective_memory_text, prompt_build_limits_from_config(&persona.config), - persona.guidelines_text.as_deref(), + private_context + .then_some(persona.guidelines_text.as_deref()) + .flatten(), ) .prompt; @@ -206,9 +222,11 @@ pub(crate) async fn run_streaming( // it stays positionally stable, preserving KV cache prefix matching for // local LLMs (llama.cpp, Ollama, LM Studio) and prompt-cache hits for // cloud providers. - let effective_user_content = - moltis_agents::prompt::prepend_datetime_to_user_content(user_content, runtime_context) - .unwrap_or_else(|| user_content.clone()); + let effective_user_content = moltis_agents::prompt::prepend_datetime_to_user_content( + user_content, + prompt_runtime_context, + ) + .unwrap_or_else(|| user_content.clone()); let mut messages: Vec = Vec::new(); messages.push(ChatMessage::system(system_prompt)); diff --git a/crates/chat/src/types.rs b/crates/chat/src/types.rs index 38f163cf53..253682491c 100644 --- a/crates/chat/src/types.rs +++ b/crates/chat/src/types.rs @@ -966,21 +966,6 @@ pub(crate) fn sanitize_user_document_display_name(name: &str) -> Option } } -pub(crate) fn parse_explicit_shell_command(text: &str) -> Option<&str> { - let trimmed = text.trim_start(); - let rest = trimmed.strip_prefix("/sh")?; - let first = rest.chars().next()?; - if !first.is_whitespace() { - return None; - } - let command = &rest[first.len_utf8()..]; - if command.trim().is_empty() { - None - } else { - Some(command) - } -} - #[derive(Debug, Clone, Serialize)] #[serde(rename_all = "camelCase")] pub(crate) struct PromptMemoryStatus { diff --git a/crates/config/src/template.rs b/crates/config/src/template.rs index 93e3f46ec7..4ade5d7caf 100644 --- a/crates/config/src/template.rs +++ b/crates/config/src/template.rs @@ -815,6 +815,7 @@ port = {port} # Port number (auto-generated for this i # api_base_url = "https://slack.com/api" # dm_policy = "allowlist" # allowlist = [] +# operators = [] # Exact sender IDs allowed to run privileged commands; empty means nobody. # thread_replies = true # ack_reactions = true # 👀 on receipt, ✅/❌ on completion (DMs + @mentions) # reaction_triggers = false # route user reactions into the agent (react ✅ to approve) @@ -827,6 +828,7 @@ port = {port} # Port number (auto-generated for this i # app_password = "..." # dm_policy = "allowlist" # allowlist = [] +# operators = [] # Exact sender IDs; no allowlist fallback. # otp_self_approval = true # otp_cooldown_secs = 300 diff --git a/crates/discord/src/commands.rs b/crates/discord/src/commands.rs index f4cff340b3..64a3a21090 100644 --- a/crates/discord/src/commands.rs +++ b/crates/discord/src/commands.rs @@ -175,7 +175,11 @@ async fn handle_component_interaction( thread_id: None, }; - match sink.dispatch_interaction(callback_data, reply_to).await { + let sender_id = component.user.id.to_string(); + match sink + .dispatch_interaction(callback_data, reply_to, Some(&sender_id)) + .await + { Ok(_response) => { // Response already sent by the gateway. }, diff --git a/crates/discord/src/config.rs b/crates/discord/src/config.rs index d4c0197c83..2bdd521d20 100644 --- a/crates/discord/src/config.rs +++ b/crates/discord/src/config.rs @@ -100,6 +100,11 @@ pub struct DiscordAccountConfig { /// User allowlist (Discord user IDs or usernames). pub allowlist: Vec, + /// Exact sender IDs allowed to run privileged channel commands. + /// Empty grants nobody privileged access. + #[serde(default)] + pub operators: Vec, + /// Guild allowlist (Discord guild/server IDs). pub guild_allowlist: Vec, @@ -180,6 +185,7 @@ impl std::fmt::Debug for DiscordAccountConfig { .field("group_policy", &self.group_policy) .field("mention_mode", &self.mention_mode) .field("allowlist", &self.allowlist) + .field("operators", &self.operators) .field("guild_allowlist", &self.guild_allowlist) .field("model", &self.model) .field("model_provider", &self.model_provider) @@ -267,6 +273,10 @@ impl ChannelConfigView for DiscordAccountConfig { &self.allowlist } + fn operators(&self) -> &[String] { + &self.operators + } + fn group_allowlist(&self) -> &[String] { &self.guild_allowlist } @@ -336,6 +346,7 @@ impl Default for DiscordAccountConfig { group_policy: GroupPolicy::Open, mention_mode: MentionMode::Mention, allowlist: Vec::new(), + operators: Vec::new(), guild_allowlist: Vec::new(), model: None, model_provider: None, @@ -362,7 +373,7 @@ pub struct RedactedConfig<'a>(pub &'a DiscordAccountConfig); impl Serialize for RedactedConfig<'_> { fn serialize(&self, serializer: S) -> Result { let c = self.0; - let mut count = 9; // always-present fields + let mut count = 10; // always-present fields count += c.model.is_some() as usize; count += c.model_provider.is_some() as usize; count += c.agent_id.is_some() as usize; @@ -381,6 +392,7 @@ impl Serialize for RedactedConfig<'_> { s.serialize_field("group_policy", &c.group_policy)?; s.serialize_field("mention_mode", &c.mention_mode)?; s.serialize_field("allowlist", &c.allowlist)?; + s.serialize_field("operators", &c.operators)?; s.serialize_field("guild_allowlist", &c.guild_allowlist)?; if c.model.is_some() { s.serialize_field("model", &c.model)?; diff --git a/crates/discord/src/handler/implementation.rs b/crates/discord/src/handler/implementation.rs index 08f8759f3f..baa8029f2d 100644 --- a/crates/discord/src/handler/implementation.rs +++ b/crates/discord/src/handler/implementation.rs @@ -1019,7 +1019,7 @@ impl EventHandler for Handler { if let Some((latitude, longitude)) = extract_location_coordinates(&body) { let resolved = sink - .resolve_pending_location(&reply_to, latitude, longitude) + .resolve_pending_location(&reply_to, Some(&peer_id), latitude, longitude) .await; if resolved { info!( diff --git a/crates/gateway/src/channel.rs b/crates/gateway/src/channel.rs index f3d8df2973..73dd521a7e 100644 --- a/crates/gateway/src/channel.rs +++ b/crates/gateway/src/channel.rs @@ -71,6 +71,45 @@ fn sender_allowlist_key(channel_type: ChannelType) -> &'static str { } } +/// Role requested when approving a sender. +/// +/// Approving for access and granting host privilege are separate decisions: +/// the allowlist answers "may this peer talk to the bot", `operators` answers +/// "may this peer run commands on this machine". Absent or unrecognized values +/// mean guest, so a caller can never grant privilege by accident. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum ApprovalRole { + Guest, + Operator, +} + +fn parse_approval_role(params: &Value) -> ApprovalRole { + match params.get("role").and_then(Value::as_str) { + Some("operator") => ApprovalRole::Operator, + _ => ApprovalRole::Guest, + } +} + +/// Append `peer_id` to the account's `operators` list. +/// +/// Operator entries are matched exactly and case-sensitively, so this must be +/// the platform sender ID — never the username the allowlist may have been +/// given, which would silently never match. +fn grant_operator(config: &mut Value, peer_id: &str) -> Result<(), String> { + let operators = config + .as_object_mut() + .ok_or_else(|| "config is not an object".to_string())? + .entry("operators") + .or_insert_with(|| serde_json::json!([])); + let entries = operators + .as_array_mut() + .ok_or_else(|| "operators is not an array".to_string())?; + if !entries.iter().any(|entry| entry.as_str() == Some(peer_id)) { + entries.push(serde_json::json!(peer_id)); + } + Ok(()) +} + fn otp_pending_payload(code: &str, expires_at: i64) -> Value { serde_json::json!({ "code": code, @@ -682,6 +721,30 @@ impl ChannelService for LiveChannelService { obj.insert("dm_policy".into(), serde_json::json!("allowlist")); } + // Operator entries must be the exact platform sender ID. `identifier` + // may be a username (allowlists match those), which would be accepted + // here and then never match at authorization time — privilege that + // silently does nothing is worse than an error. + let role = parse_approval_role(¶ms); + if role == ApprovalRole::Operator { + let peer_id = params + .get("peer_id") + .and_then(Value::as_str) + .map(str::trim) + .filter(|peer_id| !peer_id.is_empty()) + .ok_or_else(|| { + "approving as operator requires 'peer_id' (the exact platform sender ID)" + .to_string() + })?; + grant_operator(&mut config, peer_id)?; + warn!( + account_id, + peer_id, + channel_type = channel_type.as_str(), + "sender granted operator privilege (host command access)" + ); + } + if let Err(e) = self .store .upsert(StoredChannel { @@ -712,6 +775,7 @@ impl ChannelService for LiveChannelService { ); Ok(serde_json::json!({ "approved": identifier, + "role": if role == ApprovalRole::Operator { "operator" } else { "guest" }, "type": channel_type.to_string() })) } @@ -914,10 +978,14 @@ impl ChannelService for LiveChannelService { } } +#[allow(clippy::expect_used)] #[cfg(test)] mod tests { use { - super::{merge_channel_config, otp_pending_payload, sender_allowlist_key}, + super::{ + ApprovalRole, grant_operator, merge_channel_config, otp_pending_payload, + parse_approval_role, sender_allowlist_key, + }, moltis_channels::ChannelType, serde_json::json, }; @@ -981,6 +1049,49 @@ mod tests { ); } + #[test] + fn approval_role_defaults_to_guest() { + // Anything that is not exactly "operator" must not grant privilege. + for params in [ + serde_json::json!({}), + serde_json::json!({"role": "guest"}), + serde_json::json!({"role": "Operator"}), + serde_json::json!({"role": true}), + serde_json::json!({"role": ["operator"]}), + ] { + assert_eq!( + parse_approval_role(¶ms), + ApprovalRole::Guest, + "unexpected privilege for {params}" + ); + } + assert_eq!( + parse_approval_role(&serde_json::json!({"role": "operator"})), + ApprovalRole::Operator + ); + } + + #[test] + fn grant_operator_appends_exact_id_once() { + let mut config = serde_json::json!({"allowlist": ["alice"]}); + + grant_operator(&mut config, "12345678").expect("grants"); + grant_operator(&mut config, "12345678").expect("is idempotent"); + + assert_eq!(config["operators"], serde_json::json!(["12345678"])); + // Approval must not disturb the access list. + assert_eq!(config["allowlist"], serde_json::json!(["alice"])); + } + + #[test] + fn grant_operator_keeps_existing_entries_and_is_case_sensitive() { + let mut config = serde_json::json!({"operators": ["Alice"]}); + + grant_operator(&mut config, "alice").expect("grants"); + + assert_eq!(config["operators"], serde_json::json!(["Alice", "alice"])); + } + #[test] fn sender_allowlist_key_uses_matrix_user_allowlist() { assert_eq!(sender_allowlist_key(ChannelType::Matrix), "user_allowlist"); diff --git a/crates/gateway/src/channel_events.rs b/crates/gateway/src/channel_events.rs index cbfcf9a0a8..9a0a5b6175 100644 --- a/crates/gateway/src/channel_events.rs +++ b/crates/gateway/src/channel_events.rs @@ -20,21 +20,14 @@ use crate::{ state::GatewayState, }; +pub use moltis_channels::operators::ChannelSenderRole; + /// Default (deterministic) session key for a channel chat. /// /// For Telegram forum topics the thread ID is appended so each topic gets its /// own session: `telegram:bot:chat:thread`. fn default_channel_session_key(target: &ChannelReplyTarget) -> String { - match &target.thread_id { - Some(tid) => format!( - "{}:{}:{}:{}", - target.channel_type, target.account_id, target.chat_id, tid - ), - None => format!( - "{}:{}:{}", - target.channel_type, target.account_id, target.chat_id - ), - } + target.default_session_key() } /// Resolve the active session key for a channel chat. @@ -110,39 +103,99 @@ fn parse_numbered_selection(arg: &str, command_name: &str) -> ChannelResult, +) -> String { + let mut message = format!( + "{action} is restricted to this bot's operators in direct chats. \ + Shared chats and chats that cannot be verified as direct are denied.\n\n\ + Open a direct chat first. If you own this moltis instance and are still denied, add yourself under \ + Settings → Channels → (your account) → Edit → Operators in the web UI, \ + or set `operators` for the account in moltis.toml. Entries must be exact \ + platform sender IDs." + ); + if let Some(sender_id) = sender_id.map(str::trim).filter(|id| !id.is_empty()) { + message.push_str(&format!("\n\nYour sender ID here is: {sender_id}")); + } + message +} + +/// Resolve a channel sender's privilege level for an account. /// -/// Returns `false` when the allowlist is empty (open DM policy) because an -/// open policy means no one has been explicitly authorized. -async fn is_sender_on_allowlist( +/// Privileged actions (`/sh`, shell command mode, `/approve`, `/update`, and +/// host-reaching tools) require the sender to be an **operator**. Passing the +/// channel access gate is not enough: in a guild or group chat every member +/// clears that gate, so privilege is decided by the account's explicit +/// `operators` list. +/// +/// Fail-closed at every step — an unknown account, a missing registry, or an +/// unidentified sender all resolve to [`ChannelSenderRole::Guest`]. +async fn resolve_sender_role( state: &Arc, account_id: &str, - sender_id: &str, -) -> bool { + sender_id: Option<&str>, +) -> ChannelSenderRole { let Some(ref registry) = state.services.channel_registry else { - return false; + return ChannelSenderRole::Guest; }; let Some(config) = registry.account_config(account_id).await else { - return false; + return ChannelSenderRole::Guest; }; - let allowlist = config.allowlist(); - // Empty allowlist = open policy → no explicit authorization. - if allowlist.is_empty() { - return false; + moltis_channels::operators::resolve_sender_role(sender_id, config.operators()) +} + +/// Apply the fail-closed context used for every untrusted channel turn. +/// +/// The audience ceiling excludes trusted tools, while the deny-all name policy +/// also removes explicitly public tools. Configured policies may narrow this +/// context further but cannot widen it. +fn apply_untrusted_channel_context(params: &mut serde_json::Value) { + params["_tool_audience"] = serde_json::json!("public"); + params["_tool_policy"] = serde_json::json!({ "deny": ["*"] }); + params["_private_context"] = serde_json::json!(false); +} + +/// Unknown chat kinds are treated as shared. Only channel types that can prove +/// a one-to-one conversation may expose private prompt context. +fn is_shared_channel_target(reply_to: &ChannelReplyTarget) -> bool { + reply_to.channel_type.is_shared_chat(&reply_to.chat_id) +} + +fn is_trusted_channel_turn(role: ChannelSenderRole, reply_to: &ChannelReplyTarget) -> bool { + role.is_operator() && !is_shared_channel_target(reply_to) +} + +fn is_channel_command_authorized( + privilege: moltis_channels::commands::CommandPrivilege, + role: ChannelSenderRole, + reply_to: &ChannelReplyTarget, +) -> bool { + match privilege { + moltis_channels::commands::CommandPrivilege::Public => true, + moltis_channels::commands::CommandPrivilege::OperatorDirect => { + is_trusted_channel_turn(role, reply_to) + }, } - // Check the full sender_id first, then try the user part before '@' - // (WhatsApp JIDs are e.g. "15551234567@s.whatsapp.net" but allowlists - // use plain phone numbers like "15551234567"). - moltis_channels::gating::is_allowed(sender_id, allowlist) - || sender_id - .split_once('@') - .is_some_and(|(user, _)| moltis_channels::gating::is_allowed(user, allowlist)) +} + +/// Whether `sender_id` may run privileged actions from this conversation. +async fn is_sender_authorized_for_target( + state: &Arc, + reply_to: &ChannelReplyTarget, + sender_id: Option<&str>, +) -> bool { + let role = resolve_sender_role(state, &reply_to.account_id, sender_id).await; + is_trusted_channel_turn(role, reply_to) } fn is_attachable_session(entry: &SessionEntry) -> bool { @@ -523,26 +576,30 @@ impl ChannelEventSink for GatewayChannelEventSink { &self, callback_data: &str, reply_to: ChannelReplyTarget, + sender_id: Option<&str>, ) -> ChannelResult { - commands::dispatch_interaction(&self.state, callback_data, reply_to).await + commands::dispatch_interaction(&self.state, callback_data, reply_to, sender_id).await } async fn update_location( &self, reply_to: &ChannelReplyTarget, + sender_id: Option<&str>, latitude: f64, longitude: f64, ) -> bool { - commands::update_location(&self.state, reply_to, latitude, longitude).await + commands::update_location(&self.state, reply_to, sender_id, latitude, longitude).await } async fn resolve_pending_location( &self, reply_to: &ChannelReplyTarget, + sender_id: Option<&str>, latitude: f64, longitude: f64, ) -> bool { - commands::resolve_pending_location(&self.state, reply_to, latitude, longitude).await + commands::resolve_pending_location(&self.state, reply_to, sender_id, latitude, longitude) + .await } async fn dispatch_to_chat_with_attachments( diff --git a/crates/gateway/src/channel_events/commands/attachments.rs b/crates/gateway/src/channel_events/commands/attachments.rs index 3a67b511dc..d09f5774de 100644 --- a/crates/gateway/src/channel_events/commands/attachments.rs +++ b/crates/gateway/src/channel_events/commands/attachments.rs @@ -10,7 +10,8 @@ use crate::{ }; use super::super::{ - default_channel_session_key, resolve_channel_agent_id, resolve_channel_session, + apply_untrusted_channel_context, default_channel_session_key, is_trusted_channel_turn, + resolve_channel_agent_id, resolve_channel_session, resolve_sender_role, start_channel_typing_loop, }; @@ -140,11 +141,20 @@ pub(in crate::channel_events) async fn dispatch_to_chat_with_attachments( // Defer reply-target registration until chat.send() actually // starts executing this message (after semaphore acquire). "_channel_reply_target": &reply_to, + "_native_channel_request": true, }); if let Some(ref documents) = meta.documents { params["_document_files"] = serde_json::json!(documents); } + // Same privilege gate as the text path: an image or voice note from a + // guest must not hand the agent host-reaching tools either. + let sender_role = + resolve_sender_role(state, &reply_to.account_id, meta.sender_id.as_deref()).await; + if !is_trusted_channel_turn(sender_role, &reply_to) { + apply_untrusted_channel_context(&mut params); + } + // Forward the channel's default model if configured if let Some(ref model) = meta.model { params["model"] = serde_json::json!(model); diff --git a/crates/gateway/src/channel_events/commands/control_handlers.rs b/crates/gateway/src/channel_events/commands/control_handlers.rs index bbb6005539..4dc8e4ab08 100644 --- a/crates/gateway/src/channel_events/commands/control_handlers.rs +++ b/crates/gateway/src/channel_events/commands/control_handlers.rs @@ -14,8 +14,8 @@ use crate::{ use super::{ super::{ - ApprovalListResponse, format_pending_approvals_list, is_sender_on_allowlist, - parse_numbered_selection, + ApprovalListResponse, format_pending_approvals_list, is_sender_authorized_for_target, + operator_denied_message, parse_numbered_selection, }, formatting::{format_model_list, unique_providers}, }; @@ -69,14 +69,11 @@ pub(in crate::channel_events) async fn handle_approve_deny( cmd: &str, args: &str, ) -> ChannelResult { - let authorized = match sender_id { - Some(sid) => is_sender_on_allowlist(state, &reply_to.account_id, sid).await, - None => false, - }; - if !authorized { - return Err(ChannelError::invalid_input( - "You are not authorized to manage approvals. Only users on this bot's allowlist can use /approve and /deny.", - )); + if !is_sender_authorized_for_target(state, reply_to, sender_id).await { + return Err(ChannelError::invalid_input(operator_denied_message( + "Managing exec approvals", + sender_id, + ))); } if args.is_empty() { return Err(ChannelError::invalid_input(format!( @@ -630,8 +627,20 @@ pub(in crate::channel_events) async fn handle_sandbox( pub(in crate::channel_events) async fn handle_sh( state: &Arc, session_key: &str, + reply_to: &ChannelReplyTarget, + sender_id: Option<&str>, args: &str, ) -> ChannelResult { + // `/sh` toggles a mode that turns every later message in this chat into a + // shell command. Only operators in proven direct chats may touch it, + // including the read-only `status` form. + if !is_sender_authorized_for_target(state, reply_to, sender_id).await { + return Err(ChannelError::invalid_input(operator_denied_message( + "Shell access", + sender_id, + ))); + } + let route = if let Some(ref router) = state.sandbox_router { if router.is_sandboxed(session_key).await { "sandboxed" @@ -699,15 +708,11 @@ pub(in crate::channel_events) async fn handle_update( sender_id: Option<&str>, args: &str, ) -> ChannelResult { - // Owner-only: same allowlist check as approve/deny. - let authorized = match sender_id { - Some(sid) => is_sender_on_allowlist(state, &reply_to.account_id, sid).await, - None => false, - }; - if !authorized { - return Err(ChannelError::invalid_input( - "You are not authorized to update moltis. Only users on this bot's allowlist can use /update.", - )); + // Operator direct-chat only: same check as approve/deny. + if !is_sender_authorized_for_target(state, reply_to, sender_id).await { + return Err(ChannelError::invalid_input(operator_denied_message( + "/update", sender_id, + ))); } let requested_version = if args.is_empty() { diff --git a/crates/gateway/src/channel_events/commands/dispatch.rs b/crates/gateway/src/channel_events/commands/dispatch.rs index 3b0e4ce4ce..a64fcf48a1 100644 --- a/crates/gateway/src/channel_events/commands/dispatch.rs +++ b/crates/gateway/src/channel_events/commands/dispatch.rs @@ -4,12 +4,19 @@ use moltis_channels::{ChannelReplyTarget, Error as ChannelError, Result as Chann use crate::state::GatewayState; -use super::{super::resolve_channel_session, control_handlers, quick_actions, session_handlers}; +use super::{ + super::{ + is_channel_command_authorized, operator_denied_message, resolve_channel_session, + resolve_sender_role, + }, + control_handlers, quick_actions, session_handlers, +}; pub(in crate::channel_events) async fn dispatch_interaction( state: &Arc>>, callback_data: &str, reply_to: ChannelReplyTarget, + sender_id: Option<&str>, ) -> ChannelResult { // Map callback_data prefixes to slash-command text, following the same // convention used by Telegram's handle_callback_query. @@ -31,7 +38,25 @@ pub(in crate::channel_events) async fn dispatch_interaction( ))); }; - dispatch_command(state, &cmd_text, reply_to, None).await + let result = dispatch_command(state, &cmd_text, reply_to.clone(), sender_id).await; + let response = match &result { + Ok(message) => message.clone(), + Err(error) => format!("Command failed: {error}"), + }; + if let Some(gateway) = state.get() + && let Some(outbound) = gateway.services.channel_outbound_arc() + && let Err(error) = outbound + .send_text( + &reply_to.account_id, + &reply_to.outbound_to(), + &response, + reply_to.message_id.as_deref(), + ) + .await + { + tracing::warn!(%error, "failed to send interaction response"); + } + result } pub(in crate::channel_events) async fn dispatch_command( @@ -58,7 +83,21 @@ pub(in crate::channel_events) async fn dispatch_command( let cmd = command.split_whitespace().next().unwrap_or(""); let args = command[cmd.len()..].trim(); + let privilege = moltis_channels::commands::all_commands() + .iter() + .find(|definition| definition.name == cmd) + .map(|definition| definition.privilege()) + .ok_or_else(|| ChannelError::invalid_input(format!("unknown command: /{cmd}")))?; + let sender_role = resolve_sender_role(state, &reply_to.account_id, sender_id).await; + if !is_channel_command_authorized(privilege, sender_role, &reply_to) { + return Err(ChannelError::invalid_input(operator_denied_message( + &format!("/{cmd}"), + sender_id, + ))); + } + match cmd { + "help" => Ok(moltis_channels::commands::help_text()), // Session management commands "new" => { session_handlers::handle_new( @@ -113,7 +152,7 @@ pub(in crate::channel_events) async fn dispatch_command( "sandbox" => { control_handlers::handle_sandbox(state, session_metadata, &session_key, args).await }, - "sh" => control_handlers::handle_sh(state, &session_key, args).await, + "sh" => control_handlers::handle_sh(state, &session_key, &reply_to, sender_id, args).await, "stop" => control_handlers::handle_stop(state, &session_key).await, "peek" => control_handlers::handle_peek(state, &session_key).await, "tts" => control_handlers::handle_tts(state, &session_key, args).await, @@ -126,7 +165,9 @@ pub(in crate::channel_events) async fn dispatch_command( "fast" => quick_actions::handle_fast(state, session_metadata, &session_key, args).await, "insights" => quick_actions::handle_insights(state, args).await, "steer" => quick_actions::handle_steer(state, &session_key, args).await, - "queue" => quick_actions::handle_queue(state, &session_key, args).await, + "queue" => { + quick_actions::handle_queue(state, &session_key, &reply_to, sender_id, args).await + }, _ => Err(ChannelError::invalid_input(format!( "unknown command: /{cmd}" ))), @@ -135,20 +176,17 @@ pub(in crate::channel_events) async fn dispatch_command( #[cfg(test)] mod tests { - /// Verify that every command in the centralized registry (except `"help"`, - /// which channels handle locally) has a matching arm in `dispatch_command`. + /// Verify that every command in the centralized registry has a matching arm + /// in `dispatch_command`. /// /// This is a compile/test-time safety net: if a new command is added to the /// registry but not wired into gateway dispatch, this test will fail. #[test] fn all_registered_commands_have_dispatch_arms() { - // The set of commands that channels handle locally and never reach - // gateway dispatch. - let locally_handled = ["help"]; - // The set of commands that dispatch_command handles. Keep this list // in sync with the match arms above. let dispatched = [ + "help", "new", "fork", "clear", @@ -177,9 +215,6 @@ mod tests { ]; for cmd in moltis_channels::commands::all_commands() { - if locally_handled.contains(&cmd.name) { - continue; - } assert!( dispatched.contains(&cmd.name), "command `/{name}` is registered in moltis_channels::commands but has no \ diff --git a/crates/gateway/src/channel_events/commands/location.rs b/crates/gateway/src/channel_events/commands/location.rs index 66188bd915..e866035387 100644 --- a/crates/gateway/src/channel_events/commands/location.rs +++ b/crates/gateway/src/channel_events/commands/location.rs @@ -6,11 +6,14 @@ use moltis_channels::ChannelReplyTarget; use crate::state::GatewayState; -use super::super::{default_channel_session_key, resolve_channel_session}; +use super::super::{ + default_channel_session_key, is_sender_authorized_for_target, resolve_channel_session, +}; pub(in crate::channel_events) async fn update_location( state: &Arc>>, reply_to: &ChannelReplyTarget, + sender_id: Option<&str>, latitude: f64, longitude: f64, ) -> bool { @@ -18,6 +21,14 @@ pub(in crate::channel_events) async fn update_location( warn!("update_location: gateway not ready"); return false; }; + if !is_sender_authorized_for_target(state, reply_to, sender_id).await { + warn!( + account_id = %reply_to.account_id, + sender_id, + "ignored location update outside an operator direct chat" + ); + return false; + } let session_key = if let Some(ref sm) = state.services.session_metadata { resolve_channel_session(reply_to, sm).await @@ -67,6 +78,7 @@ pub(in crate::channel_events) async fn update_location( pub(in crate::channel_events) async fn resolve_pending_location( state: &Arc>>, reply_to: &ChannelReplyTarget, + sender_id: Option<&str>, latitude: f64, longitude: f64, ) -> bool { @@ -74,6 +86,14 @@ pub(in crate::channel_events) async fn resolve_pending_location( warn!("resolve_pending_location: gateway not ready"); return false; }; + if !is_sender_authorized_for_target(state, reply_to, sender_id).await { + warn!( + account_id = %reply_to.account_id, + sender_id, + "ignored pending location response outside an operator direct chat" + ); + return false; + } let session_key = if let Some(ref sm) = state.services.session_metadata { resolve_channel_session(reply_to, sm).await diff --git a/crates/gateway/src/channel_events/commands/quick_actions.rs b/crates/gateway/src/channel_events/commands/quick_actions.rs index 15832d535f..8f3b759f7a 100644 --- a/crates/gateway/src/channel_events/commands/quick_actions.rs +++ b/crates/gateway/src/channel_events/commands/quick_actions.rs @@ -1,7 +1,7 @@ use std::sync::Arc; use { - moltis_channels::{Error as ChannelError, Result as ChannelResult}, + moltis_channels::{ChannelReplyTarget, Error as ChannelError, Result as ChannelResult}, moltis_sessions::metadata::SqliteSessionMetadata, }; @@ -12,6 +12,9 @@ use crate::{ // ── /btw — ephemeral side question ────────────────────────────────────────── +/// `/btw` reads recent session history, so it is `OperatorDirect` in the command +/// registry — `dispatch_command` has already rejected guests and shared chats +/// before this runs. No local scope check is needed here. pub(in crate::channel_events) async fn handle_btw( state: &Arc, session_key: &str, @@ -544,9 +547,20 @@ pub(in crate::channel_events) async fn handle_steer( // ── /queue — queue a message for the next agent turn ──────────────────────── +fn validate_queued_message(args: &str) -> ChannelResult<()> { + if moltis_agents::runner::explicit_shell_command(args).is_some() { + return Err(ChannelError::invalid_input( + "Shell commands cannot be submitted through /queue. Wait for the active run, then use /sh directly.", + )); + } + Ok(()) +} + pub(in crate::channel_events) async fn handle_queue( state: &Arc, session_key: &str, + reply_to: &ChannelReplyTarget, + sender_id: Option<&str>, args: &str, ) -> ChannelResult { if args.is_empty() { @@ -554,14 +568,24 @@ pub(in crate::channel_events) async fn handle_queue( "usage: /queue \nQueue a message for the next agent turn without interrupting the current one.", )); } + validate_queued_message(args)?; // Use the chat service's send method — when a run is active, it will // automatically queue the message according to MessageQueueMode. let chat = state.chat(); - let params = serde_json::json!({ + let mut params = serde_json::json!({ "text": args, "_session_key": session_key, + "_channel_reply_target": reply_to, + "_native_channel_request": true, + "channel": { + "channel_type": reply_to.channel_type, + "sender_id": sender_id, + }, }); + // Queued channel messages can execute after authorization changes. Keep + // them on the untrusted context regardless of the operator's current role. + super::super::apply_untrusted_channel_context(&mut params); match chat.send(params).await { Ok(res) => { @@ -575,3 +599,16 @@ pub(in crate::channel_events) async fn handle_queue( Err(e) => Err(ChannelError::external("queue", e)), } } + +#[cfg(test)] +mod tests { + use super::validate_queued_message; + + #[test] + fn queue_rejects_every_explicit_shell_form() { + for input in ["/sh id", " /SH whoami", "/sh@mybot uname -a"] { + assert!(validate_queued_message(input).is_err(), "accepted {input}"); + } + assert!(validate_queued_message("explain what /sh does").is_ok()); + } +} diff --git a/crates/gateway/src/channel_events/dispatch.rs b/crates/gateway/src/channel_events/dispatch.rs index a984ebc6a3..a1957f5e33 100644 --- a/crates/gateway/src/channel_events/dispatch.rs +++ b/crates/gateway/src/channel_events/dispatch.rs @@ -22,11 +22,48 @@ pub(in crate::channel_events) async fn dispatch_to_chat( } else { default_channel_session_key(&reply_to) }; - let effective_text = if state.is_channel_command_mode_enabled(&session_key).await { - rewrite_for_shell_mode(text).unwrap_or_else(|| text.to_string()) - } else { - text.to_string() - }; + // Privilege is resolved per inbound message, never per session: + // command mode is shared by everyone in the chat, so an operator + // enabling it must not hand the shell to the other participants. + let sender_role = + resolve_sender_role(state, &reply_to.account_id, meta.sender_id.as_deref()).await; + + // `/sh ` is deliberately not a registered channel command — it + // falls through to the agent, which force-executes it. Stop it here + // for guests, before it reaches the runner. + let trusted_channel_turn = is_trusted_channel_turn(sender_role, &reply_to); + if !trusted_channel_turn && moltis_agents::runner::explicit_shell_command(text).is_some() { + warn!( + account_id = %reply_to.account_id, + chat_id = %reply_to.chat_id, + sender_id = ?meta.sender_id, + "denied /sh outside an operator direct chat" + ); + if let Some(done_tx) = typing_done { + let _ = done_tx.send(()); + } + channel_ack_finish(state, &reply_to, false).await; + if let Some(outbound) = state.services.channel_outbound_arc() + && let Err(send_err) = outbound + .send_text( + &reply_to.account_id, + &reply_to.outbound_to(), + &operator_denied_message("/sh", meta.sender_id.as_deref()), + reply_to.message_id.as_deref(), + ) + .await + { + warn!("failed to send shell denial back to channel: {send_err}"); + } + return; + } + + let effective_text = + if trusted_channel_turn && state.is_channel_command_mode_enabled(&session_key).await { + rewrite_for_shell_mode(text).unwrap_or_else(|| text.to_string()) + } else { + text.to_string() + }; // Broadcast a "chat" event so the web UI shows the user message // in real-time (like typing from the UI). @@ -137,8 +174,19 @@ pub(in crate::channel_events) async fn dispatch_to_chat( // Defer reply-target registration until chat.send() actually // starts executing this message (after semaphore acquire). "_channel_reply_target": &reply_to, + "_native_channel_request": true, }); + // Only an operator in a proven direct chat receives tools and private + // context. This is the single condition on purpose: an earlier version + // also skipped the ceiling for anything that parsed as `/sh`, on the + // assumption that the guard above had already rejected every untrusted + // `/sh`. That made the ceiling depend on a rejection 60 lines away, so + // narrowing that guard would have silently opened this one. + if !trusted_channel_turn { + apply_untrusted_channel_context(&mut params); + } + // Attach thread context if available. if let Some(thread_history) = thread_context { params["_thread_context"] = serde_json::json!(thread_history); diff --git a/crates/gateway/src/channel_events/helpers.rs b/crates/gateway/src/channel_events/helpers.rs deleted file mode 100644 index 618bb0b611..0000000000 --- a/crates/gateway/src/channel_events/helpers.rs +++ /dev/null @@ -1,390 +0,0 @@ -use super::*; - -use std::{collections::BTreeSet, sync::Arc}; - -use { - async_trait::async_trait, - moltis_tools::image_cache::ImageBuilder, - serde::Deserialize, - tracing::{debug, error, info, warn}, -}; - -use { - moltis_channels::{ - ChannelAttachment, ChannelEvent, ChannelEventSink, ChannelMessageMeta, ChannelReplyTarget, - Error as ChannelError, Result as ChannelResult, SavedChannelFile, - }, - moltis_sessions::metadata::{SessionEntry, SqliteSessionMetadata}, - moltis_tools::approval::PendingApprovalView, -}; - -use crate::{ - broadcast::{BroadcastOpts, broadcast}, - state::GatewayState, -}; - -/// Default (deterministic) session key for a channel chat. -/// -/// For Telegram forum topics the thread ID is appended so each topic gets its -/// own session: `telegram:bot:chat:thread`. -fn default_channel_session_key(target: &ChannelReplyTarget) -> String { - match &target.thread_id { - Some(tid) => format!( - "{}:{}:{}:{}", - target.channel_type, target.account_id, target.chat_id, tid - ), - None => format!( - "{}:{}:{}", - target.channel_type, target.account_id, target.chat_id - ), - } -} - -/// Resolve the active session key for a channel chat. -/// Uses the forward mapping table if an override exists, otherwise falls back -/// to the deterministic key. -async fn resolve_channel_session( - target: &ChannelReplyTarget, - metadata: &SqliteSessionMetadata, -) -> String { - if let Some(key) = metadata - .get_active_session( - target.channel_type.as_str(), - &target.account_id, - &target.chat_id, - target.thread_id.as_deref(), - ) - .await - { - return key; - } - default_channel_session_key(target) -} - -fn slash_command_name(text: &str) -> Option<&str> { - let rest = text.trim_start().strip_prefix('/')?; - let cmd = rest.split_whitespace().next().unwrap_or(""); - if cmd.is_empty() { - None - } else { - Some(cmd) - } -} - -fn is_channel_control_command_name(cmd: &str) -> bool { - matches!( - cmd, - "new" - | "clear" - | "compact" - | "context" - | "model" - | "sandbox" - | "sessions" - | "attach" - | "approvals" - | "approve" - | "deny" - | "agent" - | "help" - | "sh" - | "peek" - | "stop" - ) -} - -fn rewrite_for_shell_mode(text: &str) -> Option { - let trimmed = text.trim(); - if trimmed.is_empty() { - return None; - } - - if let Some(cmd) = slash_command_name(trimmed) - && is_channel_control_command_name(cmd) - { - return None; - } - - Some(format!("/sh {trimmed}")) -} - -fn parse_numbered_selection(arg: &str, command_name: &str) -> ChannelResult { - arg.parse() - .map_err(|_| ChannelError::invalid_input(format!("usage: /{command_name} [number]"))) -} - -/// Check whether `sender_id` is on the channel account's DM allowlist. -/// -/// The DM allowlist is the source of truth for privileged command access: -/// anyone allowed to DM the bot is trusted to run commands like `/approve` -/// and `/deny` from any context (DM or group). Users not on the allowlist -/// can still chat (if group policy permits) but cannot run privileged -/// commands. -/// -/// Returns `false` when the allowlist is empty (open DM policy) because an -/// open policy means no one has been explicitly authorized. -async fn is_sender_on_allowlist( - state: &Arc, - account_id: &str, - sender_id: &str, -) -> bool { - let Some(ref registry) = state.services.channel_registry else { - return false; - }; - let Some(config) = registry.account_config(account_id).await else { - return false; - }; - let allowlist = config.allowlist(); - // Empty allowlist = open policy → no explicit authorization. - if allowlist.is_empty() { - return false; - } - // Check the full sender_id first, then try the user part before '@' - // (WhatsApp JIDs are e.g. "15551234567@s.whatsapp.net" but allowlists - // use plain phone numbers like "15551234567"). - moltis_channels::gating::is_allowed(sender_id, allowlist) - || sender_id - .split_once('@') - .is_some_and(|(user, _)| moltis_channels::gating::is_allowed(user, allowlist)) -} - -fn is_attachable_session(entry: &SessionEntry) -> bool { - !entry.archived && !entry.key.starts_with("cron:") -} - -fn session_list_label(entry: &SessionEntry) -> &str { - entry.label.as_deref().unwrap_or(&entry.key) -} - -fn format_channel_sessions_list(sessions: &[SessionEntry], current_session_key: &str) -> String { - let mut lines = Vec::new(); - for (i, session) in sessions.iter().enumerate() { - let marker = if session.key == current_session_key { - " *" - } else { - "" - }; - lines.push(format!( - "{}. {} ({} msgs){}", - i + 1, - session_list_label(session), - session.message_count, - marker, - )); - } - lines.push("\nUse /sessions N to switch.".to_string()); - lines.join("\n") -} - -fn format_attachable_sessions_list(sessions: &[SessionEntry], current_session_key: &str) -> String { - let mut lines = Vec::new(); - for (i, session) in sessions.iter().enumerate() { - let label = session_list_label(session); - let marker = if session.key == current_session_key { - " *" - } else { - "" - }; - let key_suffix = if label == session.key { - String::new() - } else { - format!(" [{}]", session.key) - }; - lines.push(format!( - "{}. {}{} ({} msgs){}", - i + 1, - label, - key_suffix, - session.message_count, - marker, - )); - } - lines.push( - "\nUse /attach N to move an existing session to this chat. This rebinds it from any previous channel chat." - .to_string(), - ); - lines.join("\n") -} - -fn format_pending_approvals_list(requests: &[PendingApprovalView]) -> String { - use crate::approval::{MAX_COMMAND_PREVIEW_LEN, truncate_command_preview}; - let mut lines = Vec::new(); - for (i, request) in requests.iter().enumerate() { - let preview = truncate_command_preview(&request.command, MAX_COMMAND_PREVIEW_LEN); - lines.push(format!("{}. `{}`", i + 1, preview)); - } - lines.push("\nUse /approve N or /deny N.".to_string()); - lines.join("\n") -} - -#[derive(Debug, Deserialize)] -struct ApprovalListResponse { - #[serde(default)] - requests: Vec, -} - -#[derive(Debug, Default)] -struct ChannelSessionDefaults { - model: Option, - agent_id: Option, -} - -fn config_string(value: Option<&serde_json::Value>) -> Option { - value - .and_then(serde_json::Value::as_str) - .map(str::trim) - .filter(|value| !value.is_empty()) - .map(str::to_string) -} - -fn override_map<'a>( - config: &'a serde_json::Value, - key: &str, - target_id: &str, -) -> Option<&'a serde_json::Map> { - config - .get(key) - .and_then(serde_json::Value::as_object) - .and_then(|overrides| overrides.get(target_id)) - .and_then(serde_json::Value::as_object) -} - -async fn resolve_channel_session_defaults( - state: &Arc, - reply_to: &ChannelReplyTarget, - sender_id: Option<&str>, -) -> ChannelSessionDefaults { - let Ok(status) = state.services.channel.status().await else { - return ChannelSessionDefaults::default(); - }; - let Some(channel) = status - .get("channels") - .and_then(serde_json::Value::as_array) - .and_then(|channels| { - channels.iter().find(|channel| { - channel - .get("account_id") - .and_then(serde_json::Value::as_str) - == Some(reply_to.account_id.as_str()) - && channel.get("type").and_then(serde_json::Value::as_str) - == Some(reply_to.channel_type.as_str()) - }) - }) - else { - return ChannelSessionDefaults::default(); - }; - let Some(config) = channel.get("config") else { - return ChannelSessionDefaults::default(); - }; - - resolve_channel_session_defaults_from_config(config, &reply_to.chat_id, sender_id) -} - -fn resolve_channel_session_defaults_from_config( - config: &serde_json::Value, - chat_id: &str, - sender_id: Option<&str>, -) -> ChannelSessionDefaults { - let user_override = override_map( - config, - "user_overrides", - sender_id - .filter(|sender_id| *sender_id != chat_id) - .unwrap_or(chat_id), - ); - let channel_override = override_map(config, "channel_overrides", chat_id); - - ChannelSessionDefaults { - model: user_override - .and_then(|override_value| config_string(override_value.get("model"))) - .or_else(|| { - channel_override - .and_then(|override_value| config_string(override_value.get("model"))) - }) - .or_else(|| config_string(config.get("model"))), - agent_id: user_override - .and_then(|override_value| config_string(override_value.get("agent_id"))) - .or_else(|| { - channel_override - .and_then(|override_value| config_string(override_value.get("agent_id"))) - }) - .or_else(|| config_string(config.get("agent_id"))), - } -} - -fn start_channel_typing_loop( - state: &Arc, - reply_to: &ChannelReplyTarget, -) -> Option> { - let outbound = state.services.channel_outbound_arc()?; - let (done_tx, mut done_rx) = tokio::sync::oneshot::channel::<()>(); - let account_id = reply_to.account_id.clone(); - let chat_id = reply_to.chat_id.clone(); - - tokio::spawn(async move { - loop { - if let Err(e) = outbound.send_typing(&account_id, &chat_id).await { - debug!(account_id, chat_id, "typing indicator failed: {e}"); - } - tokio::select! { - _ = tokio::time::sleep(std::time::Duration::from_secs(4)) => {}, - _ = &mut done_rx => break, - } - } - }); - - Some(done_tx) -} - -async fn resolve_channel_agent_id( - state: &Arc, - session_key: &str, - requested_agent_id: Option<&str>, -) -> String { - let fallback = if let Some(ref store) = state.services.agent_persona_store { - store - .default_id() - .await - .unwrap_or_else(|_| "main".to_string()) - } else { - "main".to_string() - }; - - let Some(agent_id) = requested_agent_id - .map(str::trim) - .filter(|value| !value.is_empty()) - else { - return fallback; - }; - - if agent_id == "main" { - return "main".to_string(); - } - - let Some(ref store) = state.services.agent_persona_store else { - return agent_id.to_string(); - }; - - match store.get(agent_id).await { - Ok(Some(_)) => agent_id.to_string(), - Ok(None) => { - warn!( - session = %session_key, - agent_id, - fallback = %fallback, - "channel requested unknown agent, falling back to default" - ); - fallback - }, - Err(error) => { - warn!( - session = %session_key, - agent_id, - fallback = %fallback, - %error, - "failed to resolve channel agent, falling back to default" - ); - fallback - }, - } -} diff --git a/crates/gateway/src/channel_events/tests.rs b/crates/gateway/src/channel_events/tests.rs index 860846639a..c14b252ed2 100644 --- a/crates/gateway/src/channel_events/tests.rs +++ b/crates/gateway/src/channel_events/tests.rs @@ -119,6 +119,179 @@ fn shell_mode_rewrite_skips_peek_and_stop() { assert!(rewrite_for_shell_mode("/stop").is_none()); } +// ── shell access gating ──────────────────────────────────────── + +/// `dispatch_to_chat` blocks `/sh ` from non-operators using +/// `explicit_shell_command`. It must recognise every form the agent runner +/// force-executes, or a guest could slip a command past the gate and still +/// have it run. +#[test] +fn explicit_shell_command_detects_forms_the_runner_executes() { + use moltis_agents::runner::explicit_shell_command; + + assert_eq!(explicit_shell_command("/sh pwd").as_deref(), Some("pwd")); + assert_eq!( + explicit_shell_command("/sh cat /etc/passwd").as_deref(), + Some("cat /etc/passwd") + ); + // Channel-mention form (Telegram/Discord append the bot name). + assert_eq!( + explicit_shell_command("/sh@mybot uname -a").as_deref(), + Some("uname -a") + ); + // Case-insensitive head. + assert_eq!( + explicit_shell_command("/SH whoami").as_deref(), + Some("whoami") + ); + // Leading whitespace must not hide the command from the gate. + assert_eq!(explicit_shell_command(" /sh id").as_deref(), Some("id")); +} + +#[test] +fn explicit_shell_command_ignores_ordinary_chat() { + use moltis_agents::runner::explicit_shell_command; + + assert!(explicit_shell_command("hello there").is_none()); + assert!(explicit_shell_command("/sh").is_none()); + assert!(explicit_shell_command("/shell pwd").is_none()); + assert!(explicit_shell_command("/context").is_none()); + // Talking *about* the command is not a request to run it. + assert!(explicit_shell_command("what does /sh do?").is_none()); +} + +#[test] +fn unknown_and_group_chat_types_are_shared() { + let discord = ChannelReplyTarget { + channel_type: ChannelType::Discord, + account_id: "bot".into(), + chat_id: "123".into(), + message_id: None, + thread_id: None, + ack_message_id: None, + }; + let telegram_group = ChannelReplyTarget { + channel_type: ChannelType::Telegram, + chat_id: "-123".into(), + ..discord.clone() + }; + assert!(is_shared_channel_target(&discord)); + assert!(is_shared_channel_target(&telegram_group)); +} + +#[test] +fn proven_direct_chat_is_not_shared() { + let target = ChannelReplyTarget { + channel_type: ChannelType::Telegram, + account_id: "bot".into(), + chat_id: "123".into(), + message_id: None, + thread_id: None, + ack_message_id: None, + }; + assert!(!is_shared_channel_target(&target)); +} + +#[test] +fn only_operator_direct_turns_are_trusted() { + let direct = ChannelReplyTarget { + channel_type: ChannelType::Telegram, + account_id: "bot".into(), + chat_id: "123".into(), + message_id: None, + thread_id: None, + ack_message_id: None, + }; + let shared = ChannelReplyTarget { + chat_id: "-123".into(), + ..direct.clone() + }; + + assert!(is_trusted_channel_turn( + ChannelSenderRole::Operator, + &direct + )); + assert!(!is_trusted_channel_turn(ChannelSenderRole::Guest, &direct)); + assert!(!is_trusted_channel_turn( + ChannelSenderRole::Operator, + &shared + )); +} + +#[test] +fn command_authorization_matches_privilege_and_conversation_scope() { + let direct = ChannelReplyTarget { + channel_type: ChannelType::Telegram, + account_id: "bot".into(), + chat_id: "123".into(), + message_id: None, + thread_id: None, + ack_message_id: None, + }; + let shared = ChannelReplyTarget { + chat_id: "-123".into(), + ..direct.clone() + }; + + for command in moltis_channels::commands::all_commands() { + let privilege = command.privilege(); + assert_eq!( + is_channel_command_authorized(privilege, ChannelSenderRole::Guest, &direct), + matches!( + privilege, + moltis_channels::commands::CommandPrivilege::Public + ), + "guest direct-chat authorization drifted for /{}", + command.name + ); + assert_eq!( + is_channel_command_authorized(privilege, ChannelSenderRole::Operator, &shared), + matches!( + privilege, + moltis_channels::commands::CommandPrivilege::Public + ), + "operator shared-chat authorization drifted for /{}", + command.name + ); + assert!( + is_channel_command_authorized(privilege, ChannelSenderRole::Operator, &direct), + "operator direct-chat authorization drifted for /{}", + command.name + ); + } +} + +#[test] +fn untrusted_channel_context_denies_every_tool_and_private_context() { + let mut params = serde_json::json!({ + "_tool_policy": {"allow": ["*"]}, + "_private_context": true, + }); + + apply_untrusted_channel_context(&mut params); + + assert_eq!(params["_tool_audience"], "public"); + assert_eq!(params["_tool_policy"]["deny"], serde_json::json!(["*"])); + assert_eq!(params["_private_context"], false); +} + +#[test] +fn public_audience_tools_require_explicit_registration() { + const REGISTRATION: &str = include_str!("../server/prepare_core/post_state.rs"); + + assert_eq!( + REGISTRATION.matches("register_public(").count(), + 3, + "only reviewed tools should enter the public audience" + ); + for tool in ["CalcTool", "WebSearchTool", "WebFetchTool"] { + assert!( + REGISTRATION.contains(tool), + "{tool} must have an explicit public registration" + ); + } +} + // ── unique_providers ─────────────────────────────────────────── /// Regression test for GitHub issue #637: providers must be deduplicated @@ -336,3 +509,36 @@ fn channel_session_defaults_use_chat_id_for_dm_commands() { assert_eq!(defaults.model.as_deref(), Some("dm-model")); assert_eq!(defaults.agent_id.as_deref(), Some("dm-agent")); } + +#[test] +fn operator_denial_tells_the_owner_how_to_unlock_the_command() { + let message = operator_denied_message("/sh", Some("400347514466992128")); + + assert!(message.starts_with("/sh is restricted to this bot's operators in direct chats.")); + assert!(message.contains("Shared chats")); + assert!( + message.contains("Settings → Channels"), + "must point at the web UI: {message}" + ); + assert!( + message.contains("operators"), + "must name the config field: {message}" + ); + // Operator entries are exact platform IDs, so echoing the sender's own ID + // back saves an owner from having to hunt for it. + assert!( + message.contains("Your sender ID here is: 400347514466992128"), + "must echo the sender id: {message}" + ); +} + +#[test] +fn operator_denial_omits_an_absent_or_blank_sender_id() { + for sender_id in [None, Some(""), Some(" ")] { + let message = operator_denied_message("/update", sender_id); + assert!( + !message.contains("Your sender ID"), + "unattributed senders have no id to show: {message}" + ); + } +} diff --git a/crates/gateway/src/external_agents.rs b/crates/gateway/src/external_agents.rs index 18915fe732..e6af59772f 100644 --- a/crates/gateway/src/external_agents.rs +++ b/crates/gateway/src/external_agents.rs @@ -6,6 +6,7 @@ use std::{ }; mod permissions; +mod security; use { async_trait::async_trait, @@ -424,9 +425,17 @@ impl ExternalAgentChatService { if !self.external_agents.config.enabled { return None; } + if security::is_explicit_shell_request(params) { + return None; + } let session_key = resolve_session_key(params, &self.state).await; let entry = self.session_metadata.get(&session_key).await?; let kind = entry.external_agent_kind?; + if !security::allows_external_agent_request(params, entry.channel_binding.is_some()) { + return Some(Err( + "external agents are unavailable for public or tool-restricted turns".into(), + )); + } Some(self.send_external(params.clone(), session_key, kind).await) } diff --git a/crates/gateway/src/external_agents/security.rs b/crates/gateway/src/external_agents/security.rs new file mode 100644 index 0000000000..40651c6d3f --- /dev/null +++ b/crates/gateway/src/external_agents/security.rs @@ -0,0 +1,71 @@ +use serde_json::Value; + +pub(super) fn is_explicit_shell_request(params: &Value) -> bool { + params + .get("text") + .or_else(|| params.get("message")) + .and_then(Value::as_str) + .is_some_and(|text| moltis_agents::runner::explicit_shell_command(text).is_some()) +} + +pub(super) fn allows_external_agent_request(params: &Value, channel_bound: bool) -> bool { + let private_context = match params.get("_private_context") { + None => true, + Some(Value::Bool(value)) => *value, + Some(_) => false, + }; + let native_channel = params + .get("_native_channel_request") + .and_then(Value::as_bool) + == Some(true); + + private_context + && params.get("_tool_policy").is_none() + && params.get("_tool_audience").is_none() + && (!channel_bound || native_channel) +} + +#[cfg(test)] +mod tests { + use super::{allows_external_agent_request, is_explicit_shell_request}; + + #[test] + fn explicit_shell_requests_use_the_deterministic_runner() { + assert!(is_explicit_shell_request( + &serde_json::json!({"text": "/sh echo safe"}) + )); + assert!(!is_explicit_shell_request( + &serde_json::json!({"text": "explain shell scripting"}) + )); + } + + #[test] + fn rejects_public_and_tool_restricted_requests() { + assert!(!allows_external_agent_request( + &serde_json::json!({"_private_context": false}), + false + )); + assert!(!allows_external_agent_request( + &serde_json::json!({"_private_context": "false"}), + false + )); + assert!(!allows_external_agent_request( + &serde_json::json!({"_tool_policy": {"allow": ["calc"]}}), + false + )); + assert!(!allows_external_agent_request( + &serde_json::json!({"_tool_audience": "public"}), + false + )); + } + + #[test] + fn only_native_requests_may_use_channel_bound_external_agents() { + assert!(!allows_external_agent_request(&serde_json::json!({}), true)); + assert!(allows_external_agent_request( + &serde_json::json!({"_native_channel_request": true}), + true + )); + assert!(allows_external_agent_request(&serde_json::json!({}), false)); + } +} diff --git a/crates/gateway/src/methods/services/core.rs b/crates/gateway/src/methods/services/core.rs index 95213dcf98..f3757ba500 100644 --- a/crates/gateway/src/methods/services/core.rs +++ b/crates/gateway/src/methods/services/core.rs @@ -1,5 +1,11 @@ use super::*; +/// Strip gateway-owned routing and trust markers from RPC-supplied params. +/// +/// The list is owned by `moltis-chat` (the crate that reads these) so it cannot +/// drift from what the chat service treats as a gateway assertion. +use moltis_chat::request_params::strip_gateway_owned_params as strip_internal_channel_fields; + pub(super) fn register(reg: &mut MethodRegistry) { // Config reg.register( @@ -541,6 +547,7 @@ pub(super) fn register(reg: &mut MethodRegistry) { Box::new(|ctx| { Box::pin(async move { let mut params = ctx.params.clone(); + strip_internal_channel_fields(&mut params); params["_conn_id"] = serde_json::json!(ctx.client_conn_id); // Forward client Accept-Language, public remote IP, and timezone. { @@ -570,6 +577,7 @@ pub(super) fn register(reg: &mut MethodRegistry) { Box::new(|ctx| { Box::pin(async move { let mut params = ctx.params.clone(); + strip_internal_channel_fields(&mut params); params["_conn_id"] = serde_json::json!(ctx.client_conn_id); { let registry = ctx.state.client_registry.read().await; @@ -1308,3 +1316,46 @@ pub(super) fn register(reg: &mut MethodRegistry) { ); } } + +#[cfg(test)] +mod tests { + use { + super::strip_internal_channel_fields, + moltis_chat::request_params::GATEWAY_OWNED_REQUEST_PARAMS, + }; + + #[test] + fn rpc_chat_params_cannot_inject_internal_channel_routing() { + let mut params = serde_json::json!({ + "text": "hello", + "channel": {"sender_id": "forged"}, + "_channel_reply_target": {"chat_id": "forged"}, + "_native_channel_request": true, + "_tool_policy": {"allow": ["calc"]}, + }); + + strip_internal_channel_fields(&mut params); + + for name in GATEWAY_OWNED_REQUEST_PARAMS { + assert!(params.get(name).is_none(), "{name} survived stripping"); + } + assert_eq!(params["_tool_policy"]["allow"][0], "calc"); + } + + /// Stripping only helps on the paths that call it. Both chat send methods + /// forward caller-supplied JSON straight to the service, so both must strip + /// first — a new send-shaped RPC that forgets is a forged-routing hole. + #[test] + fn both_chat_send_registrations_strip_gateway_owned_params() { + const SELF: &str = include_str!("core.rs"); + + let strip_calls = SELF + .matches("strip_internal_channel_fields(&mut params)") + .count(); + assert!( + strip_calls >= 2, + "expected `chat.send` and `chat.send_sync` to both strip \ + gateway-owned params, found {strip_calls} call site(s)" + ); + } +} diff --git a/crates/gateway/src/server/prepare_core/post_state.rs b/crates/gateway/src/server/prepare_core/post_state.rs index 28bf6d7fbb..2bb1e734d6 100644 --- a/crates/gateway/src/server/prepare_core/post_state.rs +++ b/crates/gateway/src/server/prepare_core/post_state.rs @@ -9,6 +9,7 @@ use { }; mod credential_env; +mod webhook_security; use credential_env::{CredentialEnvVarProvider, ensure_sandbox_api_key}; @@ -438,6 +439,8 @@ pub(super) async fn complete_startup( let mut params = serde_json::json!({ "text": req.message, "_session_key": req.session_key, + "_private_context": false, + "_tool_audience": "public", }); if let Some(ref model) = req.model { params["model"] = serde_json::Value::String(model.clone()); @@ -445,10 +448,8 @@ pub(super) async fn complete_startup( if let Some(ref agent_id) = req.agent_id { params["agent_id"] = serde_json::Value::String(agent_id.clone()); } - if let Some(ref tool_policy) = req.tool_policy { - params["_tool_policy"] = serde_json::to_value(tool_policy) - .map_err(|error| anyhow::anyhow!(error))?; - } + params["_tool_policy"] = + webhook_security::request_tool_policy(req.tool_policy.as_ref())?; let result = chat .send_sync(params) .await @@ -726,7 +727,7 @@ pub(super) async fn complete_startup( .with_sandbox_router(Arc::clone(&sandbox_router)); tool_registry.register(Box::new(exec_tool)); - tool_registry.register(Box::new(moltis_tools::calc::CalcTool::new())); + tool_registry.register_public(Box::new(moltis_tools::calc::CalcTool::new())); #[cfg(feature = "fs-tools")] { use moltis_config::schema::FsBinaryPolicy; @@ -901,13 +902,13 @@ pub(super) async fn complete_startup( ) { #[cfg(feature = "firecrawl")] let t = t.with_firecrawl_config(&config.tools.web.firecrawl); - tool_registry.register(Box::new(t.with_env_provider(Arc::clone(&env_provider)))); + tool_registry.register_public(Box::new(t.with_env_provider(Arc::clone(&env_provider)))); } if let Some(t) = moltis_tools::web_fetch::WebFetchTool::from_config(&config.tools.web.fetch) { #[cfg(feature = "firecrawl")] let t = t.with_firecrawl(&config.tools.web.firecrawl); - tool_registry.register(Box::new(t)); + tool_registry.register_public(Box::new(t)); } #[cfg(feature = "firecrawl")] if let Some(t) = diff --git a/crates/gateway/src/server/prepare_core/post_state/webhook_security.rs b/crates/gateway/src/server/prepare_core/post_state/webhook_security.rs new file mode 100644 index 0000000000..fb13d7880c --- /dev/null +++ b/crates/gateway/src/server/prepare_core/post_state/webhook_security.rs @@ -0,0 +1,38 @@ +use serde_json::Value; + +/// Resolve the request policy for an untrusted webhook turn. +/// +/// Webhooks receive no tools unless their stored configuration explicitly opts +/// into the host-owned public audience applied by the caller. +pub(super) fn request_tool_policy( + policy: Option<&moltis_webhooks::types::ToolPolicy>, +) -> anyhow::Result { + policy.map_or_else( + || Ok(serde_json::json!({ "deny": ["*"] })), + |policy| serde_json::to_value(policy).map_err(Into::into), + ) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn absent_policy_denies_every_tool() -> anyhow::Result<()> { + let policy = request_tool_policy(None)?; + assert_eq!(policy["deny"], serde_json::json!(["*"])); + Ok(()) + } + + #[test] + fn explicit_policy_is_preserved_for_public_audience_filtering() -> anyhow::Result<()> { + let configured = moltis_webhooks::types::ToolPolicy { + allow: vec!["web_search".to_string()], + deny: Vec::new(), + }; + let policy = request_tool_policy(Some(&configured))?; + assert_eq!(policy["allow"], serde_json::json!(["web_search"])); + assert!(policy.get("deny").is_none()); + Ok(()) + } +} diff --git a/crates/gateway/src/session/service.rs b/crates/gateway/src/session/service.rs index 81ec00a644..ce2343bd54 100644 --- a/crates/gateway/src/session/service.rs +++ b/crates/gateway/src/session/service.rs @@ -1,16 +1,7 @@ use super::*; fn default_channel_session_key(target: &moltis_channels::ChannelReplyTarget) -> String { - match &target.thread_id { - Some(thread_id) => format!( - "{}:{}:{}:{}", - target.channel_type, target.account_id, target.chat_id, thread_id - ), - None => format!( - "{}:{}:{}", - target.channel_type, target.account_id, target.chat_id - ), - } + target.default_session_key() } async fn is_current_channel_session( @@ -44,6 +35,31 @@ async fn is_archivable_entry( entry.key != "main" && !is_current_channel_session(metadata, entry).await } +/// Why a session's channel binding cannot be cleared, if it cannot. +/// +/// A session created *by* a channel is that chat's own conversation: its history +/// is the room's history. Clearing the binding there would let a web turn run +/// with full tool access and owner-private context over messages other people +/// wrote — exactly what the channel-bound ceiling exists to prevent — and the +/// next inbound message would re-create the binding anyway. +/// +/// A session that was merely *attached* to a channel (via `/attach`) has its own +/// key, so unbinding returns the chat to its default session and restores the +/// session to normal use. That is the case this exists to allow. +fn channel_binding_clear_refusal( + entry: &moltis_sessions::metadata::SessionEntry, +) -> Option { + let binding_json = entry.channel_binding.as_deref()?; + let target = serde_json::from_str::(binding_json).ok()?; + (default_channel_session_key(&target) == entry.key).then(|| { + format!( + "session '{}' is a channel's own conversation and cannot be unbound. \ + Attach a different session to that chat instead.", + entry.key + ) + }) +} + /// Live session service backed by JSONL store + SQLite metadata. pub struct LiveSessionService { pub(super) store: Arc, @@ -564,6 +580,27 @@ impl SessionService for LiveSessionService { if let Some(mcp_disabled) = p.mcp_disabled { self.metadata.set_mcp_disabled(key, mcp_disabled).await; } + if let Some(channel_binding) = p.channel_binding { + // Only clearing is supported. Creating a binding routes a chat's + // traffic into a session and is a privileged channel-side action + // (`/attach`), not something an API caller may assert. + if !channel_binding.as_ref().is_none_or(Value::is_null) { + return Err(ServiceError::message( + "'channelBinding' can only be set to null (to unbind); \ + bind a session to a chat with /attach from that chat", + )); + } + if let Some(refusal) = channel_binding_clear_refusal(&entry) { + return Err(ServiceError::message(refusal)); + } + // Drop the chat→session override first so the channel falls back to + // its own session. Leaving it would keep delivering channel traffic + // into a session that no longer declares a binding, which is exactly + // the combination the channel-bound ceiling cannot detect. + self.metadata.clear_active_session_mappings(key).await; + self.metadata.set_channel_binding(key, None).await; + info!(session = key, "cleared channel binding"); + } if let Some(sandbox_enabled_opt) = p.sandbox_enabled { let old_sandbox = entry.sandbox_enabled; self.metadata @@ -629,6 +666,7 @@ impl SessionService for LiveSessionService { "sandbox_backend": entry.sandbox_backend, "worktree_branch": entry.worktree_branch, "mcpDisabled": entry.mcp_disabled, + "channelBinding": entry.channel_binding, "agent_id": entry.agent_id, "agentId": entry.agent_id, "mode_id": entry.mode_id, diff --git a/crates/gateway/src/session/tests.rs b/crates/gateway/src/session/tests.rs index 78f497d691..203381f4be 100644 --- a/crates/gateway/src/session/tests.rs +++ b/crates/gateway/src/session/tests.rs @@ -1376,6 +1376,9 @@ mod tests { #[path = "archive_search_tests.rs"] mod archive_search_tests; + #[path = "channel_binding_tests.rs"] + mod channel_binding_tests; + #[cfg(feature = "fs-tools")] #[tokio::test] async fn delete_clears_fs_state_for_session() { diff --git a/crates/gateway/src/session/tests/tests/channel_binding_tests.rs b/crates/gateway/src/session/tests/tests/channel_binding_tests.rs new file mode 100644 index 0000000000..8b0f80407b --- /dev/null +++ b/crates/gateway/src/session/tests/tests/channel_binding_tests.rs @@ -0,0 +1,143 @@ +//! Tests for clearing a session's channel binding (`sessions.patch` with +//! `channelBinding: null`). +//! +//! A session attached to a chat runs every non-gateway turn without tools or +//! private context, so releasing it is the only way back. These pin both +//! directions: an attached session can be released, a chat's own session +//! cannot. + +use super::*; + +const ATTACH_BINDING: &str = r#"{"channel_type":"telegram","account_id":"bot1","chat_id":"123"}"#; + +async fn service_with_attached_session( + dir: &tempfile::TempDir, + key: &str, +) -> (LiveSessionService, Arc) { + let store = Arc::new(SessionStore::new(dir.path().to_path_buf())); + let metadata = Arc::new(SqliteSessionMetadata::new(sqlite_pool().await)); + metadata.upsert(key, None).await.unwrap(); + metadata + .set_channel_binding(key, Some(ATTACH_BINDING.to_string())) + .await; + metadata + .set_active_session("telegram", "bot1", "123", None, key) + .await; + let service = LiveSessionService::new(store, Arc::clone(&metadata)); + (service, metadata) +} + +/// A session attached to a chat runs every turn without tools or private +/// context, so there must be a way back. Unbinding must also drop the +/// chat→session override: leaving it would keep routing channel traffic +/// into a session that no longer advertises a binding, which is the one +/// combination the channel-bound ceiling cannot detect. +#[tokio::test] +async fn patch_clears_an_attached_channel_binding_and_its_active_mapping() { + let dir = tempfile::tempdir().unwrap(); + let (svc, metadata) = service_with_attached_session(&dir, "session:attached").await; + + let result = svc + .patch(serde_json::json!({ + "key": "session:attached", + "channelBinding": null, + })) + .await + .unwrap(); + + assert!(result["channelBinding"].is_null()); + assert!( + metadata + .get("session:attached") + .await + .unwrap() + .channel_binding + .is_none() + ); + assert_eq!( + metadata + .get_active_session("telegram", "bot1", "123", None) + .await, + None, + "the chat must fall back to its own default session" + ); +} + +/// A channel's own session *is* the room's conversation. Unbinding it would +/// hand a web turn full trust over other people's messages, and the next +/// inbound message would re-bind it anyway. +#[tokio::test] +async fn patch_refuses_to_unbind_a_channels_own_session() { + let dir = tempfile::tempdir().unwrap(); + let (svc, metadata) = service_with_attached_session(&dir, "telegram:bot1:123").await; + + let error = svc + .patch(serde_json::json!({ + "key": "telegram:bot1:123", + "channelBinding": null, + })) + .await + .unwrap_err(); + + assert!(error.to_string().contains("cannot be unbound"), "{error}"); + assert!( + metadata + .get("telegram:bot1:123") + .await + .unwrap() + .channel_binding + .is_some(), + "the binding must survive a refused unbind" + ); +} + +/// Creating a binding routes a chat's traffic into a session — a privileged +/// channel-side action. Rejecting the value outright is better than +/// ignoring it, which would let a caller believe it succeeded. +#[tokio::test] +async fn patch_rejects_setting_a_channel_binding() { + let dir = tempfile::tempdir().unwrap(); + let (svc, metadata) = service_with_attached_session(&dir, "session:attached").await; + + let error = svc + .patch(serde_json::json!({ + "key": "session:attached", + "channelBinding": {"channel_type": "telegram", "account_id": "bot1", "chat_id": "999"}, + })) + .await + .unwrap_err(); + + assert!(error.to_string().contains("only be set to null"), "{error}"); + assert_eq!( + metadata + .get("session:attached") + .await + .unwrap() + .channel_binding + .as_deref(), + Some(ATTACH_BINDING), + "a rejected patch must not alter the binding" + ); +} + +/// Absent field must stay a no-op — every other `patch` field behaves that +/// way, and an accidental unbind here silently widens a session's trust. +#[tokio::test] +async fn patch_without_channel_binding_leaves_it_untouched() { + let dir = tempfile::tempdir().unwrap(); + let (svc, metadata) = service_with_attached_session(&dir, "session:attached").await; + + svc.patch(serde_json::json!({ "key": "session:attached", "label": "renamed" })) + .await + .unwrap(); + + assert_eq!( + metadata + .get("session:attached") + .await + .unwrap() + .channel_binding + .as_deref(), + Some(ATTACH_BINDING) + ); +} diff --git a/crates/gateway/src/session_types.rs b/crates/gateway/src/session_types.rs index 434160eb17..535a124363 100644 --- a/crates/gateway/src/session_types.rs +++ b/crates/gateway/src/session_types.rs @@ -40,6 +40,11 @@ pub struct PatchParams { pub sandbox_enabled: Option>, #[serde(default, deserialize_with = "double_option", alias = "sandbox_backend")] pub sandbox_backend: Option>, + /// Channel binding. Only clearing (`null`) is accepted — see + /// `SessionService::patch`. A non-null value is rejected rather than + /// ignored, so a caller cannot believe it bound a session that it did not. + #[serde(default, deserialize_with = "double_option", alias = "channel_binding")] + pub channel_binding: Option>, } /// Deserialize a field as `Some(inner)` when present (even if null), diff --git a/crates/matrix/src/config.rs b/crates/matrix/src/config.rs index 5737298a93..230afd26d1 100644 --- a/crates/matrix/src/config.rs +++ b/crates/matrix/src/config.rs @@ -128,6 +128,11 @@ pub struct MatrixAccountConfig { /// User allowlist (Matrix user IDs). pub user_allowlist: Vec, + /// Exact sender IDs allowed to run privileged channel commands. + /// Empty grants nobody privileged access. + #[serde(default)] + pub operators: Vec, + /// Auto-join rooms on invite. pub auto_join: AutoJoinPolicy, @@ -191,6 +196,7 @@ impl Default for MatrixAccountConfig { mention_mode: MentionMode::Mention, room_allowlist: Vec::new(), user_allowlist: Vec::new(), + operators: Vec::new(), auto_join: AutoJoinPolicy::Always, model: None, model_provider: None, @@ -224,6 +230,7 @@ impl std::fmt::Debug for MatrixAccountConfig { .field("mention_mode", &self.mention_mode) .field("room_allowlist", &self.room_allowlist) .field("user_allowlist", &self.user_allowlist) + .field("operators", &self.operators) .field("auto_join", &self.auto_join) .field("model", &self.model) .field("model_provider", &self.model_provider) @@ -247,7 +254,7 @@ pub struct RedactedConfig<'a>(pub &'a MatrixAccountConfig); impl Serialize for RedactedConfig<'_> { fn serialize(&self, serializer: S) -> Result { let c = self.0; - let mut count = 16; + let mut count = 17; count += c.auth_mode.is_some() as usize; count += c.password.is_some() as usize; count += c.user_id.is_some() as usize; @@ -284,6 +291,7 @@ impl Serialize for RedactedConfig<'_> { s.serialize_field("mention_mode", &c.mention_mode)?; s.serialize_field("room_allowlist", &c.room_allowlist)?; s.serialize_field("user_allowlist", &c.user_allowlist)?; + s.serialize_field("operators", &c.operators)?; s.serialize_field("auto_join", &c.auto_join)?; if c.model.is_some() { s.serialize_field("model", &c.model)?; @@ -318,6 +326,10 @@ impl ChannelConfigView for MatrixAccountConfig { &self.user_allowlist } + fn operators(&self) -> &[String] { + &self.operators + } + fn group_allowlist(&self) -> &[String] { &self.room_allowlist } diff --git a/crates/matrix/src/handler/implementation.rs b/crates/matrix/src/handler/implementation.rs index 8be0a0fa91..a74e67e1ba 100644 --- a/crates/matrix/src/handler/implementation.rs +++ b/crates/matrix/src/handler/implementation.rs @@ -361,7 +361,7 @@ pub async fn handle_room_message( && let Some((latitude, longitude)) = extract_location_coordinates(&body) { let resolved = sink - .resolve_pending_location(&reply_to, latitude, longitude) + .resolve_pending_location(&reply_to, Some(&sender_id), latitude, longitude) .await; if resolved { info!( @@ -578,7 +578,10 @@ pub async fn handle_poll_response( message_id: None, }; - if let Err(error) = sink.dispatch_interaction(&callback_data, reply_to).await { + if let Err(error) = sink + .dispatch_interaction(&callback_data, reply_to, Some(&sender_id)) + .await + { debug!( account_id, callback_data, "matrix poll interaction dispatch failed: {error}" @@ -803,7 +806,9 @@ async fn handle_location_message( return; }; - let resolved = sink.update_location(&reply_to, latitude, longitude).await; + let resolved = sink + .update_location(&reply_to, meta.sender_id.as_deref(), latitude, longitude) + .await; info!( account_id, event_id, diff --git a/crates/msteams/src/config.rs b/crates/msteams/src/config.rs index ad420d208f..50a6fc4b96 100644 --- a/crates/msteams/src/config.rs +++ b/crates/msteams/src/config.rs @@ -102,6 +102,11 @@ pub struct MsTeamsAccountConfig { /// User allowlist (AAD object IDs or channel user IDs). pub allowlist: Vec, + /// Exact sender IDs allowed to run privileged channel commands. + /// Empty grants nobody privileged access. + #[serde(default)] + pub operators: Vec, + /// Group/team allowlist. pub group_allowlist: Vec, @@ -197,6 +202,7 @@ impl std::fmt::Debug for MsTeamsAccountConfig { .field("group_policy", &self.group_policy) .field("mention_mode", &self.mention_mode) .field("allowlist", &self.allowlist) + .field("operators", &self.operators) .field("group_allowlist", &self.group_allowlist) .field("otp_self_approval", &self.otp_self_approval) .field("otp_cooldown_secs", &self.otp_cooldown_secs) @@ -221,7 +227,7 @@ impl Serialize for RedactedConfig<'_> { fn serialize(&self, serializer: S) -> Result { let c = self.0; // Count: 16 always-present + conditional optional fields. - let mut count = 16; + let mut count = 17; count += c.webhook_secret.is_some() as usize; count += c.model.is_some() as usize; count += c.model_provider.is_some() as usize; @@ -240,6 +246,7 @@ impl Serialize for RedactedConfig<'_> { s.serialize_field("group_policy", &c.group_policy)?; s.serialize_field("mention_mode", &c.mention_mode)?; s.serialize_field("allowlist", &c.allowlist)?; + s.serialize_field("operators", &c.operators)?; s.serialize_field("group_allowlist", &c.group_allowlist)?; s.serialize_field("otp_self_approval", &c.otp_self_approval)?; s.serialize_field("otp_cooldown_secs", &c.otp_cooldown_secs)?; @@ -280,6 +287,10 @@ impl ChannelConfigView for MsTeamsAccountConfig { &self.allowlist } + fn operators(&self) -> &[String] { + &self.operators + } + fn group_allowlist(&self) -> &[String] { &self.group_allowlist } @@ -340,6 +351,7 @@ impl Default for MsTeamsAccountConfig { group_policy: GroupPolicy::Open, mention_mode: MentionMode::Mention, allowlist: Vec::new(), + operators: Vec::new(), group_allowlist: Vec::new(), otp_self_approval: true, otp_cooldown_secs: 300, diff --git a/crates/nostr/src/config.rs b/crates/nostr/src/config.rs index f26f9e62d2..8ee4ae4928 100644 --- a/crates/nostr/src/config.rs +++ b/crates/nostr/src/config.rs @@ -48,6 +48,11 @@ pub struct NostrAccountConfig { /// Public keys allowed to send DMs (npub1/hex). pub allowed_pubkeys: Vec, + /// Exact sender IDs allowed to run privileged channel commands. + /// Empty grants nobody privileged access. + #[serde(default)] + pub operators: Vec, + /// Whether this account is enabled. pub enabled: bool, @@ -81,6 +86,7 @@ impl Default for NostrAccountConfig { relays: default_relays(), dm_policy: DmPolicy::Allowlist, allowed_pubkeys: Vec::new(), + operators: Vec::new(), enabled: true, profile: None, model: None, @@ -107,6 +113,7 @@ impl std::fmt::Debug for NostrAccountConfig { .field("relays", &self.relays) .field("dm_policy", &self.dm_policy) .field("allowed_pubkeys", &self.allowed_pubkeys) + .field("operators", &self.operators) .field("enabled", &self.enabled) .field("profile", &self.profile) .field("model", &self.model) @@ -124,13 +131,14 @@ pub struct RedactedConfig<'a>(pub &'a NostrAccountConfig); impl Serialize for RedactedConfig<'_> { fn serialize(&self, serializer: S) -> Result { let c = self.0; - let mut count = 10; + let mut count = 11; count += c.agent_id.is_some() as usize; let mut s = serializer.serialize_struct("NostrAccountConfig", count)?; s.serialize_field("secret_key", "[REDACTED]")?; s.serialize_field("relays", &c.relays)?; s.serialize_field("dm_policy", &c.dm_policy)?; s.serialize_field("allowed_pubkeys", &c.allowed_pubkeys)?; + s.serialize_field("operators", &c.operators)?; s.serialize_field("enabled", &c.enabled)?; s.serialize_field("profile", &c.profile)?; s.serialize_field("model", &c.model)?; @@ -149,6 +157,10 @@ impl ChannelConfigView for NostrAccountConfig { &self.allowed_pubkeys } + fn operators(&self) -> &[String] { + &self.operators + } + fn group_allowlist(&self) -> &[String] { // Nostr DMs are always 1:1, no group concept &[] diff --git a/crates/sessions/src/message.rs b/crates/sessions/src/message.rs index 94d6e963e5..7170030dd4 100644 --- a/crates/sessions/src/message.rs +++ b/crates/sessions/src/message.rs @@ -305,7 +305,13 @@ impl PersistedMessage { } } - /// Create a tool execution result message. + /// Create a tool execution result message with no run ID. + /// + /// Prefer [`Self::tool_result_with_run_id`]. A message without a `run_id` + /// cannot be attributed to a run, so run-scoped history filtering (see + /// `filter_public_history` in `moltis-chat`) drops it — which would orphan + /// the paired assistant `tool_calls` entry and make the history invalid for + /// most providers. pub fn tool_result( tool_call_id: impl Into, tool_name: impl Into, @@ -327,7 +333,9 @@ impl PersistedMessage { } } - /// Create a tool execution result message with reasoning text. + /// Create a tool execution result message with reasoning text and no run ID. + /// + /// Carries the same caveat as [`Self::tool_result`]. pub fn tool_result_with_reasoning( tool_call_id: impl Into, tool_name: impl Into, diff --git a/crates/signal/src/config.rs b/crates/signal/src/config.rs index 6d0b504450..b9621e49d7 100644 --- a/crates/signal/src/config.rs +++ b/crates/signal/src/config.rs @@ -28,6 +28,11 @@ pub struct SignalAccountConfig { pub dm_policy: DmPolicy, /// Signal identifiers allowed to send DMs. pub allowlist: Vec, + + /// Exact sender IDs allowed to run privileged channel commands. + /// Empty grants nobody privileged access. + #[serde(default)] + pub operators: Vec, /// Group access policy. pub group_policy: GroupPolicy, /// Signal group IDs allowed when `group_policy = "allowlist"`. @@ -62,6 +67,7 @@ impl Default for SignalAccountConfig { http_url: DEFAULT_HTTP_URL.to_string(), dm_policy: DmPolicy::Allowlist, allowlist: Vec::new(), + operators: Vec::new(), group_policy: GroupPolicy::Disabled, group_allowlist: Vec::new(), mention_mode: MentionMode::Mention, @@ -113,6 +119,10 @@ impl ChannelConfigView for SignalAccountConfig { &self.allowlist } + fn operators(&self) -> &[String] { + &self.operators + } + fn group_allowlist(&self) -> &[String] { &self.group_allowlist } diff --git a/crates/slack/src/config.rs b/crates/slack/src/config.rs index 1f14be250d..9963a26807 100644 --- a/crates/slack/src/config.rs +++ b/crates/slack/src/config.rs @@ -95,6 +95,11 @@ pub struct SlackAccountConfig { #[serde(default)] pub allowlist: Vec, + /// Exact sender IDs allowed to run privileged channel commands. + /// Empty grants nobody privileged access. + #[serde(default)] + pub operators: Vec, + /// Channel allowlist (Slack channel IDs). #[serde(default)] pub channel_allowlist: Vec, @@ -166,6 +171,7 @@ impl std::fmt::Debug for SlackAccountConfig { .field("group_policy", &self.group_policy) .field("mention_mode", &self.mention_mode) .field("allowlist", &self.allowlist) + .field("operators", &self.operators) .field("channel_allowlist", &self.channel_allowlist) .field("model", &self.model) .field("model_provider", &self.model_provider) @@ -196,6 +202,7 @@ impl Default for SlackAccountConfig { group_policy: GroupPolicy::Open, mention_mode: MentionMode::Mention, allowlist: Vec::new(), + operators: Vec::new(), channel_allowlist: Vec::new(), model: None, model_provider: None, @@ -219,6 +226,10 @@ impl ChannelConfigView for SlackAccountConfig { &self.allowlist } + fn operators(&self) -> &[String] { + &self.operators + } + fn group_allowlist(&self) -> &[String] { &self.channel_allowlist } @@ -274,7 +285,7 @@ pub struct RedactedConfig<'a>(pub &'a SlackAccountConfig); impl Serialize for RedactedConfig<'_> { fn serialize(&self, serializer: S) -> Result { let c = self.0; - let mut count = 16; // always-present fields + let mut count = 17; // always-present fields count += c.signing_secret.is_some() as usize; count += !c.reaction_trigger_emojis.is_empty() as usize; count += c.model.is_some() as usize; @@ -294,6 +305,7 @@ impl Serialize for RedactedConfig<'_> { s.serialize_field("group_policy", &c.group_policy)?; s.serialize_field("mention_mode", &c.mention_mode)?; s.serialize_field("allowlist", &c.allowlist)?; + s.serialize_field("operators", &c.operators)?; s.serialize_field("channel_allowlist", &c.channel_allowlist)?; if c.model.is_some() { s.serialize_field("model", &c.model)?; diff --git a/crates/slack/src/socket.rs b/crates/slack/src/socket.rs index ba8b535679..a218e4fcc6 100644 --- a/crates/slack/src/socket.rs +++ b/crates/slack/src/socket.rs @@ -324,14 +324,15 @@ async fn interaction_events_callback( drop(guard); // Extract the action_id from block_actions interaction type. - let (action_id, channel_id) = match &event { + let (action_id, channel_id, sender_id) = match &event { SlackInteractionEvent::BlockActions(ba) => { let action = ba.actions.as_ref().and_then(|a| a.first()); let channel = ba.channel.as_ref().map(|c| c.id.to_string()); - match (action, channel) { - (Some(act), Some(ch)) => (act.action_id.to_string(), ch), + let user = ba.user.as_ref().map(|user| user.id.to_string()); + match (action, channel, user) { + (Some(act), Some(ch), Some(user)) => (act.action_id.to_string(), ch, user), _ => { - debug!("block_actions missing action or channel"); + debug!("block_actions missing action, channel, or user"); return Ok(()); }, } @@ -360,7 +361,10 @@ async fn interaction_events_callback( message_id: None, thread_id: None, }; - match sink.dispatch_interaction(&action_id, reply_to).await { + match sink + .dispatch_interaction(&action_id, reply_to, Some(&sender_id)) + .await + { Ok(_response) => { // Response already sent by the gateway. }, diff --git a/crates/slack/src/webhook.rs b/crates/slack/src/webhook.rs index d40af4c12f..d3f695a960 100644 --- a/crates/slack/src/webhook.rs +++ b/crates/slack/src/webhook.rs @@ -249,9 +249,17 @@ pub async fn handle_verified_interaction_webhook( .and_then(|c| c.get("id")) .and_then(|v| v.as_str()) .unwrap_or(""); + let sender_id = payload + .get("user") + .and_then(|user| user.get("id")) + .and_then(|value| value.as_str()) + .unwrap_or(""); - if action_id.is_empty() || channel_id.is_empty() { - debug!(account_id, "interaction missing action_id or channel"); + if action_id.is_empty() || channel_id.is_empty() || sender_id.is_empty() { + debug!( + account_id, + "interaction missing action_id, channel, or user" + ); return Ok(()); } @@ -269,7 +277,10 @@ pub async fn handle_verified_interaction_webhook( message_id: None, thread_id: None, }; - match sink.dispatch_interaction(action_id, reply_to).await { + match sink + .dispatch_interaction(action_id, reply_to, Some(sender_id)) + .await + { Ok(_) => {}, Err(e) => { debug!(account_id, action_id, "interaction dispatch failed: {e}"); @@ -475,9 +486,17 @@ pub async fn handle_interaction_webhook( .and_then(|c| c.get("id")) .and_then(|v| v.as_str()) .unwrap_or(""); + let sender_id = payload + .get("user") + .and_then(|user| user.get("id")) + .and_then(|value| value.as_str()) + .unwrap_or(""); - if action_id.is_empty() || channel_id.is_empty() { - debug!(account_id, "interaction missing action_id or channel"); + if action_id.is_empty() || channel_id.is_empty() || sender_id.is_empty() { + debug!( + account_id, + "interaction missing action_id, channel, or user" + ); return Ok(()); } @@ -495,7 +514,10 @@ pub async fn handle_interaction_webhook( message_id: None, thread_id: None, }; - match sink.dispatch_interaction(action_id, reply_to).await { + match sink + .dispatch_interaction(action_id, reply_to, Some(sender_id)) + .await + { Ok(_) => {}, Err(e) => { debug!(account_id, action_id, "interaction dispatch failed: {e}"); diff --git a/crates/telegram/src/config.rs b/crates/telegram/src/config.rs index 620d5b8c96..35ab56b77b 100644 --- a/crates/telegram/src/config.rs +++ b/crates/telegram/src/config.rs @@ -65,6 +65,11 @@ pub struct TelegramAccountConfig { /// User/peer allowlist for DMs. pub allowlist: Vec, + /// Exact sender IDs allowed to run privileged channel commands. + /// Empty grants nobody privileged access. + #[serde(default)] + pub operators: Vec, + /// Group/chat ID allowlist. pub group_allowlist: Vec, @@ -138,7 +143,7 @@ pub struct RedactedConfig<'a>(pub &'a TelegramAccountConfig); impl Serialize for RedactedConfig<'_> { fn serialize(&self, serializer: S) -> Result { let c = self.0; - let mut count = 14; // always-present fields + let mut count = 15; // always-present fields count += c.model.is_some() as usize; count += c.model_provider.is_some() as usize; count += c.agent_id.is_some() as usize; @@ -150,6 +155,7 @@ impl Serialize for RedactedConfig<'_> { s.serialize_field("group_policy", &c.group_policy)?; s.serialize_field("mention_mode", &c.mention_mode)?; s.serialize_field("allowlist", &c.allowlist)?; + s.serialize_field("operators", &c.operators)?; s.serialize_field("group_allowlist", &c.group_allowlist)?; s.serialize_field("stream_mode", &c.stream_mode)?; s.serialize_field("edit_throttle_ms", &c.edit_throttle_ms)?; @@ -183,6 +189,10 @@ impl ChannelConfigView for TelegramAccountConfig { &self.allowlist } + fn operators(&self) -> &[String] { + &self.operators + } + fn group_allowlist(&self) -> &[String] { &self.group_allowlist } @@ -252,6 +262,7 @@ impl Default for TelegramAccountConfig { group_policy: GroupPolicy::default(), mention_mode: MentionMode::default(), allowlist: Vec::new(), + operators: Vec::new(), group_allowlist: Vec::new(), stream_mode: StreamMode::default(), edit_throttle_ms: 2000, diff --git a/crates/telegram/src/handlers/implementation.rs b/crates/telegram/src/handlers/implementation.rs index a0ffa22263..a0cc6b23bb 100644 --- a/crates/telegram/src/handlers/implementation.rs +++ b/crates/telegram/src/handlers/implementation.rs @@ -461,7 +461,8 @@ pub async fn handle_message_direct( message_id: Some(msg.id.0.to_string()), thread_id: extract_thread_id(&msg), }; - sink.update_location(&reply_target, lat, lon).await + sink.update_location(&reply_target, Some(&peer_id), lat, lon) + .await } else { false }; @@ -843,7 +844,9 @@ pub async fn handle_edited_location( message_id: Some(msg.id.0.to_string()), thread_id: extract_thread_id(&msg), }; - sink.update_location(&reply_target, lat, lon).await; + let sender_id = msg.from.as_ref().map(|user| user.id.0.to_string()); + sink.update_location(&reply_target, sender_id.as_deref(), lat, lon) + .await; } Ok(()) diff --git a/crates/telephony/src/config.rs b/crates/telephony/src/config.rs index 9aef8e0df8..379c4b6ccc 100644 --- a/crates/telephony/src/config.rs +++ b/crates/telephony/src/config.rs @@ -81,6 +81,11 @@ pub struct TelephonyAccountConfig { #[serde(default)] pub allowlist: Vec, + /// Exact sender IDs allowed to run privileged channel commands. + /// Empty grants nobody privileged access. + #[serde(default)] + pub operators: Vec, + // ── Voice settings ── /// TTS voice ID to use for bot speech. #[serde(default, skip_serializing_if = "Option::is_none")] @@ -128,6 +133,7 @@ impl Default for TelephonyAccountConfig { notify_hangup_delay_secs: default_notify_hangup_delay(), inbound_policy: InboundPolicy::default(), allowlist: Vec::new(), + operators: Vec::new(), voice_id: None, tts_provider: None, model: None, @@ -142,6 +148,10 @@ impl ChannelConfigView for TelephonyAccountConfig { &self.allowlist } + fn operators(&self) -> &[String] { + &self.operators + } + fn group_allowlist(&self) -> &[String] { &[] } diff --git a/crates/web/ui/e2e/specs/channels-operators.spec.js b/crates/web/ui/e2e/specs/channels-operators.spec.js new file mode 100644 index 0000000000..85dd3b3a7f --- /dev/null +++ b/crates/web/ui/e2e/specs/channels-operators.spec.js @@ -0,0 +1,195 @@ +const { expect, test } = require("../base-test"); +const { navigateAndWait, waitForWsConnected, watchPageErrors } = require("../helpers"); + +// Inject a WebSocket stub serving a single configured channel and capturing +// the channels.update payload the edit modal sends on save. +async function installChannelMock(page, channel) { + await page.evaluate(async (mockChannel) => { + const appScript = document.querySelector('script[type="module"][src*="js/app.js"]'); + if (!appScript) throw new Error("app.js script not found"); + const appUrl = new URL(appScript.src, window.location.origin).href; + const marker = "js/app.js"; + const markerIdx = appUrl.indexOf(marker); + if (markerIdx < 0) throw new Error("app.js marker not found in script URL"); + const prefix = appUrl.slice(0, markerIdx); + const state = await import(`${prefix}js/state.js`); + const channelsPage = await import(`${prefix}js/page-channels.js`); + const wsOpen = typeof WebSocket === "undefined" ? 1 : WebSocket.OPEN; + window.__channelUpdateRequest = null; + state.setConnected(true); + const handlers = { + "channels.status": () => ({ channels: [mockChannel] }), + "channels.senders.list": () => ({ senders: mockChannel.__senders || [] }), + "agents.list": () => ({ agents: [] }), + "channels.senders.approve": (params) => { + window.__senderApproveRequest = params || null; + return {}; + }, + "channels.update": (params) => { + window.__channelUpdateRequest = params || null; + return {}; + }, + }; + state.setWs({ + readyState: wsOpen, + send(raw) { + const req = JSON.parse(raw || "{}"); + const resolver = state.pending[req.id]; + if (!resolver) return; + const handler = handlers[req.method]; + resolver({ ok: true, payload: handler ? handler(req.params) : {} }); + delete state.pending[req.id]; + }, + }); + if (typeof state.refreshChannelsPage === "function") { + state.refreshChannelsPage(); + } else { + await channelsPage.prefetchChannels(); + } + await new Promise((resolve) => setTimeout(resolve, 100)); + }, channel); +} + +function discordChannel(config) { + return { + type: "discord", + account_id: "test-bot", + name: "Test Discord", + status: "connected", + config, + }; +} + +async function openEditModal(page, channel) { + await installChannelMock(page, channel); + await expect(page.getByText("Test Discord", { exact: true })).toBeVisible({ timeout: 10_000 }); + await page.locator('button[title="Edit test-bot"]').click(); + const modal = page.locator(".modal-box"); + await expect(modal.getByText("Operators", { exact: true })).toBeVisible(); + return modal; +} + +// The tag list rendered by for the Operators field. Scoping to +// it keeps assertions off the Allowlist field above, which holds the same IDs. +function operatorsField(modal) { + return modal.locator("label", { hasText: "Operators" }).locator("xpath=following-sibling::div[1]"); +} + +// The Operators list decides who may use /sh and host-reaching tools, so the +// edit modal must load it, save it, and state which rule is currently in force. +test.describe("Channel operators", () => { + test("operators round-trip through the edit modal", async ({ page }) => { + const pageErrors = watchPageErrors(page); + await navigateAndWait(page, "/settings/channels"); + await waitForWsConnected(page); + + const modal = await openEditModal(page, discordChannel({ allowlist: ["owner-id"], operators: ["owner-id"] })); + + // Existing operator renders as a tag. Each tag is a holding the + // value plus a "×" remove button, so match on the tag rather than on + // text equal to the value alone. + const operators = operatorsField(modal); + await expect(operators.getByText("owner-id")).toBeVisible(); + await expect(modal.getByTestId("operators-hint")).toContainText("Only these exact sender IDs"); + + // Add a second operator via the tag input. + const operatorsInput = operators.locator("input"); + await operatorsInput.fill("trusted-admin"); + await operatorsInput.press("Enter"); + + await modal.getByRole("button", { name: "Save Changes", exact: true }).click(); + + await expect + .poll(() => page.evaluate(() => window.__channelUpdateRequest)) + .toMatchObject({ + config: { operators: ["owner-id", "trusted-admin"] }, + }); + + expect(pageErrors).toEqual([]); + }); + + test("hint explains that an empty operator list fails closed", async ({ page }) => { + const pageErrors = watchPageErrors(page); + await navigateAndWait(page, "/settings/channels"); + await waitForWsConnected(page); + + const modal = await openEditModal(page, discordChannel({ allowlist: ["owner-id"], operators: [] })); + + await expect(modal.getByTestId("operators-hint")).toContainText( + "privileged commands and private host access are disabled for every sender", + ); + + expect(pageErrors).toEqual([]); + }); + + test("hint warns that shell access is off when nothing is configured", async ({ page }) => { + const pageErrors = watchPageErrors(page); + await navigateAndWait(page, "/settings/channels"); + await waitForWsConnected(page); + + const modal = await openEditModal(page, discordChannel({ allowlist: [], operators: [] })); + + await expect(modal.getByTestId("operators-hint")).toContainText("disabled for every sender"); + + expect(pageErrors).toEqual([]); + }); +}); + +// Approving a sender for access and granting them the host are separate +// decisions, so "Approve" must always ask which one is meant. +test.describe("Sender approval role", () => { + async function openApprovalDialog(page, config) { + const channel = discordChannel(config); + channel.__senders = [ + { peer_id: "400347514466992128", sender_name: "Fabien", username: "fabien", message_count: 2, allowed: false }, + ]; + await installChannelMock(page, channel); + // The tab bar renders role="tab", and the badge puts the count in the + // accessible name, so match on the label rather than the exact string. + await page.getByRole("tab", { name: /Senders/ }).click(); + await page.getByRole("button", { name: "Approve", exact: true }).click(); + return page.locator(".modal-box"); + } + + test("operator is preselected while the channel has no operators", async ({ page }) => { + const pageErrors = watchPageErrors(page); + await navigateAndWait(page, "/settings/channels"); + await waitForWsConnected(page); + + const modal = await openApprovalDialog(page, { allowlist: [], operators: [] }); + + await expect(modal.getByTestId("approve-role-operator")).toHaveAttribute("data-selected", "true"); + await expect(modal.getByTestId("approve-role-guest")).toHaveAttribute("data-selected", "false"); + await expect(modal.getByTestId("approve-first-operator-hint")).toBeVisible(); + + // The operator grant must carry peer_id: operators match exact platform + // IDs, so approving on the username alone would never take effect. + await modal.getByRole("button", { name: "Approve as operator", exact: true }).click(); + await expect + .poll(() => page.evaluate(() => window.__senderApproveRequest)) + .toMatchObject({ + identifier: "fabien", + peer_id: "400347514466992128", + role: "operator", + }); + + expect(pageErrors).toEqual([]); + }); + + test("guest is the default once an operator exists", async ({ page }) => { + const pageErrors = watchPageErrors(page); + await navigateAndWait(page, "/settings/channels"); + await waitForWsConnected(page); + + const modal = await openApprovalDialog(page, { allowlist: [], operators: ["someone-else"] }); + + await expect(modal.getByTestId("approve-role-guest")).toHaveAttribute("data-selected", "true"); + await expect(modal.getByTestId("approve-role-operator")).toHaveAttribute("data-selected", "false"); + await expect(modal.getByTestId("approve-first-operator-hint")).toHaveCount(0); + + await modal.getByRole("button", { name: "Approve", exact: true }).click(); + await expect.poll(() => page.evaluate(() => window.__senderApproveRequest)).toMatchObject({ role: "guest" }); + + expect(pageErrors).toEqual([]); + }); +}); diff --git a/crates/web/ui/e2e/specs/session-channel-release.spec.js b/crates/web/ui/e2e/specs/session-channel-release.spec.js new file mode 100644 index 0000000000..5d447a5d2e --- /dev/null +++ b/crates/web/ui/e2e/specs/session-channel-release.spec.js @@ -0,0 +1,134 @@ +// A session attached to a chat (via `/attach`) runs every web turn as an +// untrusted public turn: no tools, no memory, no project context. Releasing the +// binding is the only way back, so the control has to be reachable and has to +// send the clearing patch. + +const { expect, test } = require("../base-test"); +const { createSession, navigateAndWait, waitForWsConnected } = require("../helpers"); + +// Capture outgoing RPC payloads without letting them reach the server: the +// server would (correctly) reject clearing a binding this test only faked +// client-side, and what we care about is the request the UI builds. +async function captureRpc(page, method) { + await page.evaluate(async (watchedMethod) => { + const appScript = document.querySelector('script[type="module"][src*="js/app.js"]'); + if (!appScript) throw new Error("app module script not found"); + const appUrl = new URL(appScript.src, window.location.origin).href; + const prefix = appUrl.slice(0, appUrl.length - "js/app.js".length); + const state = await import(`${prefix}js/state.js`); + const ws = state.ws; + if (!ws) throw new Error("websocket unavailable"); + + window.__capturedRpc = null; + const originalSend = ws.send.bind(ws); + ws.send = (payload) => { + let parsed = null; + try { + parsed = JSON.parse(payload); + } catch (_error) { + parsed = null; + } + if (parsed?.method === watchedMethod) { + window.__capturedRpc = parsed; + // Resolve the pending promise so the UI is not left spinning. + const resolver = state.pending[parsed.id]; + if (resolver) { + resolver({ ok: true, payload: { channelBinding: null } }); + delete state.pending[parsed.id]; + } + return; + } + originalSend(payload); + }; + }, method); +} + +// SessionHeader is mounted three times (name, toolbar, actions), so every one of +// its controls appears three times. Scope to the actions mount. +function releaseControl(page) { + return page.locator("#sessionActionsMount").getByTestId("session-unbind-channel"); +} + +// Mark the active session as attached to a Telegram chat whose own default +// session key differs from this session's key — the shape `/attach` produces. +async function markSessionAttached(page) { + await expect + .poll( + () => + page.evaluate(() => { + const store = window.__moltis_stores?.sessionStore; + const session = store?.activeSession?.value; + if (!session) return false; + session.channelBinding = { + channel_type: "telegram", + account_id: "bot1", + chat_id: "123", + }; + session.dataVersion.value++; + return true; + }), + { timeout: 10_000 }, + ) + .toBe(true); +} + +test.describe("Releasing a session from its channel", () => { + test("release control clears the binding via sessions.patch", async ({ page }) => { + const pageErrors = await navigateAndWait(page, "/"); + await waitForWsConnected(page); + await createSession(page); + const sessionKey = await page.evaluate(() => window.__moltis_stores?.sessionStore?.activeSessionKey?.value || ""); + expect(sessionKey).not.toBe(""); + + await markSessionAttached(page); + + const release = releaseControl(page); + await expect(release).toBeVisible({ timeout: 10_000 }); + // The label must say what it restores, not just "unbind". + await expect(release).toHaveAttribute("title", /without tools or private context/); + + await captureRpc(page, "sessions.patch"); + await release.click(); + + await expect + .poll(() => page.evaluate(() => window.__capturedRpc)) + .toMatchObject({ + method: "sessions.patch", + params: { key: sessionKey, channelBinding: null }, + }); + + expect(pageErrors).toEqual([]); + }); + + // A channel's own session cannot be released — the server refuses it and the + // next inbound message would re-bind it — so the control must not appear. + test("a channel's own session offers no release control", async ({ page }) => { + const pageErrors = await navigateAndWait(page, "/"); + await waitForWsConnected(page); + await createSession(page); + + await expect + .poll( + () => + page.evaluate(() => { + const store = window.__moltis_stores?.sessionStore; + const session = store?.activeSession?.value; + if (!session) return false; + // Binding whose default session key *is* this session's key. + session.channelBinding = { + channel_type: "telegram", + account_id: "bot1", + chat_id: "123", + }; + session.key = "telegram:bot1:123"; + session.dataVersion.value++; + return true; + }), + { timeout: 10_000 }, + ) + .toBe(true); + + await expect(releaseControl(page)).toHaveCount(0); + expect(pageErrors).toEqual([]); + }); +}); diff --git a/crates/web/ui/src/components/SessionHeader.tsx b/crates/web/ui/src/components/SessionHeader.tsx index 95413976a9..a90d250593 100644 --- a/crates/web/ui/src/components/SessionHeader.tsx +++ b/crates/web/ui/src/components/SessionHeader.tsx @@ -9,10 +9,12 @@ import { onEvent } from "../events"; import * as gon from "../gon"; import { parseAgentsListPayload, sendRpc } from "../helpers"; import { + channelBindingLabel, clearActiveSession, clearSessionHistoryCache, fetchSessions, isArchivableSession, + isChannelUnbindableSession, removeSessionFromClientState, setSessionActiveRunId, setSessionReplying, @@ -160,6 +162,8 @@ export function SessionHeader({ const canRename = !(isMain || isCron); const canStop = !isCron && replying; const canArchive = !!session && isArchivableSession(session.toMeta()); + const canUnbindChannel = !!session && isChannelUnbindableSession(session.toMeta()); + const boundChannelLabel = session ? channelBindingLabel(session.toMeta()) : ""; const showArchivedSessions = sessionStore.showArchivedSessions.value; const currentAgentId = session?.agent_id || defaultAgentId || "main"; const currentNodeId = session?.node_id || ""; @@ -396,6 +400,35 @@ export function SessionHeader({ }); }, [canArchive, currentKey, onBeforeArchive, session, showArchivedSessions]); + // A session attached to a chat runs every turn as an untrusted public turn: + // no tools, no memory, no project context, because the chat's history holds + // other people's messages. Releasing it is the only way back, so the control + // says what it restores rather than just "Unbind". + // + // No confirmation: this is reversible with `/attach` from the chat, and the + // shared confirm dialog labels its action button "Delete", which reads far + // more destructive than what happens. + const [unbinding, setUnbinding] = useState(false); + const onUnbindChannel = useCallback(() => { + if (!(session && canUnbindChannel)) return; + setUnbinding(true); + sendRpc("sessions.patch", { key: currentKey, channelBinding: null }) + .then((res) => { + if (!res?.ok) { + showToast((res?.error as { message?: string })?.message || "Failed to release channel", "error"); + return; + } + showToast( + boundChannelLabel + ? `Released from ${boundChannelLabel}. Tools and private context are available here again.` + : "Released from its channel. Tools and private context are available here again.", + "success", + ); + fetchSessions(); + }) + .finally(() => setUnbinding(false)); + }, [boundChannelLabel, canUnbindChannel, currentKey, session]); + const onAgentChange = useCallback( (nextAgentId: string) => { if (!nextAgentId || nextAgentId === currentAgentId || switchingAgent) { @@ -581,6 +614,21 @@ export function SessionHeader({ {session?.archived ? "Unarchive" : "Archive"} )} + {canUnbindChannel && ( + + )} {showFork && !isCron && ( ) : ( - ); @@ -789,13 +802,18 @@ function SendersTab(): VNode { return
No channels configured.
; } - function onAction(identifier: string, action: string): void { - const rpc = action === "approve" ? "channels.senders.approve" : "channels.senders.deny"; + // Approving asks for a role first: access and host privilege are separate + // grants, so "Approve" must never silently confer the latter. + function onAction(sender: SenderEntry, action: string): void { + if (action === "approve") { + pendingApproval.value = sender; + return; + } const parsed = parseSenderSelectionKey(sendersAccount.value); - sendRpc(rpc, { + sendRpc("channels.senders.deny", { type: parsed.type, account_id: parsed.account_id, - identifier, + identifier: sender.username || sender.peer_id, }).then(() => { loadSenders(); loadChannels(); @@ -939,6 +957,7 @@ function ChannelsPageComponent(): VNode { + ); diff --git a/crates/web/ui/src/pages/channels/modals/ApproveSenderModal.tsx b/crates/web/ui/src/pages/channels/modals/ApproveSenderModal.tsx new file mode 100644 index 0000000000..2394b0e65e --- /dev/null +++ b/crates/web/ui/src/pages/channels/modals/ApproveSenderModal.tsx @@ -0,0 +1,166 @@ +// ── Approve sender modal ───────────────────────────────────── +// +// Approving a sender for access and granting them host privilege are separate +// decisions, so the role is always an explicit choice here rather than a side +// effect of "Approve". Operator is pre-selected only while the account has no +// operators at all — on a fresh install the person holding the OTP code read it +// off this very UI, so they are almost certainly the owner, and making them +// hunt for their own platform ID in another settings page is pure friction. +// Once one operator exists, the next sender is a guest by default. + +import { useSignal } from "@preact/signals"; +import type { VNode } from "preact"; +import { useEffect } from "preact/hooks"; + +import { sendRpc } from "../../../helpers"; +import { Modal } from "../../../ui"; +import { + loadChannels, + loadSenders, + pendingApproval, + type SenderEntry, + selectedSenderChannel, +} from "../../ChannelsPage"; + +type ApprovalRole = "guest" | "operator"; + +// The prop is `value`, not `role`: a prop literally named `role` is read as the +// ARIA role attribute by the a11y lint, and "guest"/"operator" are not roles in +// that sense. +function RoleOption(props: { + value: ApprovalRole; + selected: ApprovalRole; + title: string; + detail: string; + onSelect: (role: ApprovalRole) => void; +}): VNode { + const isSelected = props.selected === props.value; + return ( + + ); +} + +export function ApproveSenderModal(): VNode | null { + const sender: SenderEntry | null = pendingApproval.value; + const channel = selectedSenderChannel(); + const hasOperators = (channel?.config?.operators?.length ?? 0) > 0; + const role = useSignal("guest"); + const error = useSignal(""); + const saving = useSignal(false); + + // This component stays mounted and renders null while closed, so the default + // must be chosen when the dialog opens — a mount-time initializer would be + // computed before any channel is loaded and then never revisited. + useEffect(() => { + if (!sender) return; + role.value = hasOperators ? "guest" : "operator"; + error.value = ""; + saving.value = false; + }, [sender, hasOperators, role, error, saving]); + + if (!(sender && channel)) return null; + + // Operators are matched as exact platform IDs, so the operator grant needs + // peer_id even when the allowlist entry is a username. + const identifier = sender.username || sender.peer_id; + + function close(): void { + pendingApproval.value = null; + error.value = ""; + saving.value = false; + } + + function approve(): void { + if (!(sender && channel)) return; + saving.value = true; + error.value = ""; + sendRpc("channels.senders.approve", { + type: channel.type, + account_id: channel.account_id, + identifier, + peer_id: sender.peer_id, + role: role.value, + }) + .then((res) => { + const r = res as { ok?: boolean; error?: { message?: string } } | undefined; + if (!r?.ok) { + error.value = r?.error?.message || "Approval failed."; + saving.value = false; + return; + } + close(); + loadSenders(); + loadChannels(); + }) + .catch((e: unknown) => { + error.value = e instanceof Error ? e.message : String(e); + saving.value = false; + }); + } + + return ( + +
+
+ {sender.username ? `@${String(sender.username).replace(/^@/, "")} · ` : ""} + {sender.peer_id} +
+ + { + role.value = next; + }} + /> + { + role.value = next; + }} + /> + + {!hasOperators && ( +

+ This channel has no operators yet, so nobody — including you — can run /sh or other privileged + commands on it. Approve yourself as an operator to enable them. +

+ )} + + {error.value &&
{error.value}
} + +
+ + +
+
+
+ ); +} diff --git a/crates/web/ui/src/pages/channels/modals/EditChannelModal.tsx b/crates/web/ui/src/pages/channels/modals/EditChannelModal.tsx index 76a21319dc..125c698424 100644 --- a/crates/web/ui/src/pages/channels/modals/EditChannelModal.tsx +++ b/crates/web/ui/src/pages/channels/modals/EditChannelModal.tsx @@ -32,6 +32,7 @@ export function EditChannelModal(): VNode | null { const editAgent = useSignal(""); const agentsList = useSignal>([]); const allowlistItems = useSignal([]); + const operatorItems = useSignal([]); const roomAllowlistItems = useSignal([]); const editCredential = useSignal(""); const editWebhookSecret = useSignal(""); @@ -60,6 +61,7 @@ export function EditChannelModal(): VNode | null { ch?.config?.user_allowlist || ch?.config?.allowed_pubkeys || []) as string[]; + operatorItems.value = (ch?.config?.operators || []) as string[]; roomAllowlistItems.value = (ch?.config?.room_allowlist || ch?.config?.group_allowlist || []) as string[]; editCredential.value = ""; editWebhookSecret.value = (ch?.config?.webhook_secret as string) || ""; @@ -161,6 +163,7 @@ export function EditChannelModal(): VNode | null { const dmFallback = isWhatsApp ? "open" : "allowlist"; updateConfig.dm_policy = (form.querySelector("[data-field=dmPolicy]") as HTMLSelectElement)?.value || dmFallback; updateConfig.allowlist = allowlistItems.value; + updateConfig.operators = operatorItems.value; if (isMatrix) { updateConfig.user_allowlist = allowlistItems.value; updateConfig.room_policy = @@ -726,6 +729,25 @@ export function EditChannelModal(): VNode | null { allowlistItems.value = v; }} /> + +

+ Senders who may run /sh and other commands on this machine, and read this + instance's sessions and memory. This is equivalent to giving someone your terminal — add yourself, or someone + you trust with the host. Entries are exact, case-sensitive platform sender IDs. +

+ { + operatorItems.value = v; + }} + /> +

+ {operatorItems.value.length > 0 + ? "Only these exact sender IDs can use privileged commands. Shared-room agent turns remain restricted for everyone." + : "No operators set — privileged commands and private host access are disabled for every sender on this channel."} +

{isMatrix && ( <> diff --git a/crates/web/ui/src/sessions.ts b/crates/web/ui/src/sessions.ts index 27e8bec776..97bac1a6fd 100644 --- a/crates/web/ui/src/sessions.ts +++ b/crates/web/ui/src/sessions.ts @@ -445,6 +445,50 @@ export function isArchivableSession(session: SessionMeta): boolean { ); } +/** The reply target stored in `SessionMeta.channelBinding`, JSON or already parsed. */ +interface ParsedChannelBinding { + channel_type?: string; + account_id?: string; + chat_id?: string; + thread_id?: string; +} + +export function parseChannelBinding(session: SessionMeta): ParsedChannelBinding | null { + const binding = session.channelBinding; + if (!binding) return null; + if (typeof binding !== "string") return binding as ParsedChannelBinding; + try { + return JSON.parse(binding) as ParsedChannelBinding; + } catch (_error) { + return null; + } +} + +/** Human-readable "telegram · bot1 · -100123" label for a bound session. */ +export function channelBindingLabel(session: SessionMeta): string { + const binding = parseChannelBinding(session); + if (!binding) return ""; + return [binding.channel_type, binding.account_id, binding.chat_id].filter(Boolean).join(" · "); +} + +/** + * Whether this session's channel binding can be cleared. + * + * A session created by a channel is that chat's own conversation — its history + * is the room's history, and the next inbound message would re-bind it, so the + * server refuses to unbind it. A session that was *attached* to a chat has its + * own key and can be released, which is what restores tools and private context + * to it in the web UI. Mirrors `channel_binding_clear_refusal` in + * `crates/gateway/src/session/service.rs`. + */ +export function isChannelUnbindableSession(session: SessionMeta): boolean { + const binding = parseChannelBinding(session); + if (!(binding?.channel_type && binding.account_id && binding.chat_id)) return false; + const parts = [binding.channel_type, binding.account_id, binding.chat_id]; + if (binding.thread_id) parts.push(binding.thread_id); + return session.key !== parts.join(":"); +} + function isClearableSession(session: SessionMeta): boolean { const isChannelSessionKey = session.key.startsWith("telegram:") || diff --git a/crates/whatsapp/src/config.rs b/crates/whatsapp/src/config.rs index a95b94b56a..0432b93d0c 100644 --- a/crates/whatsapp/src/config.rs +++ b/crates/whatsapp/src/config.rs @@ -66,6 +66,11 @@ pub struct WhatsAppAccountConfig { /// User/peer allowlist for DMs (JID user parts or phone numbers). pub allowlist: Vec, + /// Exact sender IDs allowed to run privileged channel commands. + /// Empty grants nobody privileged access. + #[serde(default)] + pub operators: Vec, + /// Group JID allowlist. pub group_allowlist: Vec, @@ -103,6 +108,10 @@ impl ChannelConfigView for WhatsAppAccountConfig { &self.allowlist } + fn operators(&self) -> &[String] { + &self.operators + } + fn group_allowlist(&self) -> &[String] { &self.group_allowlist } @@ -178,6 +187,7 @@ impl Default for WhatsAppAccountConfig { group_policy: GroupPolicy::default(), mention_mode: MentionMode::Always, allowlist: Vec::new(), + operators: Vec::new(), group_allowlist: Vec::new(), otp_self_approval: true, otp_cooldown_secs: 300, diff --git a/crates/whatsapp/src/handlers.rs b/crates/whatsapp/src/handlers.rs index 699985d0d7..9c9c0e4d6d 100644 --- a/crates/whatsapp/src/handlers.rs +++ b/crates/whatsapp/src/handlers.rs @@ -784,7 +784,8 @@ async fn handle_location( // Try to resolve a pending tool-triggered location request. let resolved = if let Some(ref sink) = state.event_sink { - sink.update_location(&reply_to, lat, lon).await + sink.update_location(&reply_to, meta.sender_id.as_deref(), lat, lon) + .await } else { false }; diff --git a/docs/src/channels.md b/docs/src/channels.md index 6b7003214b..51c57d85cd 100644 --- a/docs/src/channels.md +++ b/docs/src/channels.md @@ -225,6 +225,164 @@ All allowlist fields across all channels share the same matching behavior: - **Glob wildcards** — `"admin_*"`, `"*@example.com"`, `"user_*_vip"` - **Multiple identifiers** — both the user's numeric ID and username are checked (where applicable) +### Operators (Privileged Senders) + +Passing the access gate lets someone *talk* to the bot. It does not let them +run commands on your machine. That is a separate list: + +```toml +[channels.telegram.my-bot] +allowlist = ["owner-id"] # who may DM the bot +operators = ["owner-id"] # who may use privileged access in proven DMs +``` + +```admonish warning title="Upgrading: privileged commands are off until you set `operators`" +Earlier versions treated the DM `allowlist` as the privileged list, so anyone +allowed to DM the bot could run `/sh`, `/approve`, and `/update`. That fallback +is gone: **an empty `operators` list means nobody**, including you. + +After upgrading, add your own exact platform sender ID to `operators` for each +account, or those commands will refuse everyone. If you do not know your ID, +run `/sh` — the refusal message tells you what yours is on that channel. +``` + +```admonish warning title="Upgrading: shared chats start from an empty history once" +An untrusted turn only sees history from runs that were themselves untrusted, and +messages written before this change carry no such marker. The first turn after +upgrading in a group chat, guild, or channel-bound session therefore starts with +no prior context. This happens once per session; everything written from then on +accumulates normally. +``` + +A command requires an operator DM when the worst case reaches beyond the +current chat. Everything scoped to the conversation you are already in stays +open to any sender who clears the access gate: + +| | Commands | +|---|---| +| **Anyone who may chat** | `/help`, `/new`, `/clear`, `/compact`, `/title`, `/fork`, `/stop`, `/model`, `/mode`, `/fast` | +| **Operators in proven direct chats only** | `/sh`, `/update`, `/approve`, `/deny`, `/approvals`, `/sandbox`, `/attach`, `/sessions`, `/context`, `/insights`, `/peek`, `/btw`, `/rollback`, `/agent`, `/steer`, `/queue` | + +The public set can only disrupt the room's own conversation — something any +member can already do by talking. The operator set runs host commands (`/sh`, +`/update`), acts on the owner's behalf (`/approve` and `/deny` resolve the +*owner's* pending exec requests, which is code execution by proxy), weakens +isolation (`/sandbox` can turn the sandbox off), or reads state outside the +current chat (`/attach`, `/sessions`, `/context`, `/insights`, `/peek`, `/btw`). + +New commands default to operator direct-chat only, so adding one is safe until +it is deliberately reviewed. + +Guest, shared-room, and unknown-topology channel turns receive no tools. The +gateway applies both the registry's public audience ceiling and a deny-all name +policy, so account, group, and per-sender tool policies cannot restore even a +tool reviewed for other untrusted origins. + +Every normal turn in a shared room is untrusted, including turns sent by an +operator, because the shared history contains messages from other people. +`/sh`, shell command mode, and every privileged command are denied there. In an +operator's conversation that the adapter can prove is direct, normal agent +turns may use the full configured tool set. Unknown chat kinds fail closed as +shared. + +```admonish note title="Conservative DM detection" +Discord, Microsoft Teams, and Matrix chat IDs do not encode whether the +conversation is direct, and that topology is not yet carried into the gateway. +Those integrations currently fail closed as shared even for actual DMs, so use +the authenticated web UI or a supported proven-direct channel for privileged +work. Normal tool-free chat still works. + +Phone calls are also treated as shared, for a different reason: the only +identifier a call carries is the caller number, and caller ID is trivially +spoofable. There is nothing there to authenticate an operator against, so +telephony never grants privileged access no matter what `operators` contains. + +Chat kinds are matched as an **allowlist of shapes known to be one-to-one** +(Telegram positive chat IDs, Slack `D…` conversations, WhatsApp +`@s.whatsapp.net` and `@lid` JIDs, Signal non-`group:` identifiers, Nostr DMs). +An ID whose form is not recognised is shared. +``` + +Untrusted turns also omit owner-private prompt context: user profile, project +context, long-term memory, skills, and automatic memory extraction. + +### Channel-bound sessions + +A session is *bound* to a chat when the chat created it, or when an operator ran +`/attach` to move an existing session into that chat. Binding is what makes the +chat's replies land in the right place — and it also means the session's history +contains messages the bot did not author. + +Every request into a bound session that does not come from the channel gateway +is therefore treated as untrusted, whoever made it: web UI turns, cron jobs, +webhooks, and the `sessions_send` tool all run with no tools and no private +context. The downgrade is logged (`session is bound to a channel; running this +turn without tools or private context`), because otherwise an affected cron job +simply answers as though it had no tools. + +```admonish tip title="Getting a session back" +If you attached a working session to a chat and now want it back, open it in the +web UI and press **Release channel** in the session header (or call +`sessions.patch` with `channelBinding: null`). Messages from that chat return to +the chat's own session, and the released session regains tools, memory, and +project context. + +A chat's *own* session cannot be released — its history is the room's history, +and the next inbound message would re-create the binding. Attach a different +session to that chat instead. +``` + +### Granting operator + +Two places, and both state what it means: + +- **When approving a sender.** Approving asks for a role: *Guest* (may chat) or + *Operator* (may run commands on this machine). While an account has no + operators at all, Operator is pre-selected — on a fresh install the person + holding the OTP code read it off this very UI, so they are almost certainly + the owner. Once one operator exists, Guest is the default. +- **Settings → Channels → Edit → Operators**, to add or remove IDs directly. + +Approving as operator records the sender's exact platform ID, not their +username, because operator matching is exact. + +```admonish tip title="Locked out of your own bot?" +Send `/sh`. The refusal tells you your sender ID on that channel, which is what +`operators` needs. +``` + +```admonish warning title="This matters most in group and guild chats" +In a Discord guild or a group chat, every member who passes the group policy +clears the access gate. Without an `operators` list, anything gated only on +"can this person talk to the bot" is open to the whole room. +``` + +Resolution is **fail-closed**: + +| `operators` | `allowlist` | Who is an operator | +|-------------|-------------|--------------------| +| non-empty | anything | only exact sender IDs in `operators` | +| empty | anything | **nobody** — privileged commands are disabled | + +A sender with no identifier (for example an unattributed button callback) is +never an operator. + +`operators` contains exact, case-sensitive platform sender IDs. Globs, +usernames, and partial IDs do not grant privilege. For WhatsApp use the full +JID; for Matrix use the full Matrix user ID. + +```admonish note title="Interaction with OTP self-approval" +OTP self-approval appends the approved sender to `allowlist` only. It never +promotes the sender to operator; privileged access must be configured +separately. +``` + +Untrusted tool restrictions stack with the per-channel tool policy +(`channels...tools.groups.`). Those policies can +further restrict an operator DM, but cannot enable tools for a guest, shared +room, or unknown chat. Grant eligibility for privileged access by adding the +sender to `operators`; the sender must still use a proven direct chat. + ### OTP Self-Approval Channels that support OTP (Telegram, Microsoft Teams, Slack, Discord, Matrix, WhatsApp) allow non-allowlisted diff --git a/docs/src/commands.md b/docs/src/commands.md index 1746bc3406..4a4b164b15 100644 --- a/docs/src/commands.md +++ b/docs/src/commands.md @@ -13,9 +13,10 @@ Type `/` in the chat input to see the autocomplete popup. | `/new` | Start a new session | | `/clear` | Clear session history | | `/compact` | Summarize conversation to save tokens | -| `/context` | Show session context and project info | -| `/sessions` | List and switch sessions (channels only) | -| `/attach` | Attach an existing session to this channel (channels only) | +| `/title` | Generate a title from session history | +| `/context` | Show session context and project info — **operator DMs only on channels** | +| `/sessions` | List and switch sessions (channels only) — **operator DMs only** | +| `/attach` | Attach an existing session to this channel (channels only) — **operator DMs only** | | `/fork [label]` | Fork the current session into a new branch | ### /fork @@ -35,25 +36,40 @@ Available in web UI, all channels, and via the `sessions.fork` RPC. See | Command | Description | |---------|-------------| -| `/agent [N]` | Switch session agent | +| `/agent [N]` | Switch session agent — **operator DMs only on channels** | | `/mode [N\|name\|none]` | Switch session mode | | `/model [N]` | Switch provider/model | -| `/sandbox [on\|off\|image N]` | Toggle sandbox and choose image | -| `/sh [on\|off]` | Enter command mode (passthrough to shell) | +| `/sandbox [on\|off\|image N]` | Toggle sandbox and choose image — **operator DMs only on channels** | +| `/sh [on\|off]` | Enter command mode (passthrough to shell) — **operator DMs only on channels** | | `/stop` | Abort the current running agent | -| `/peek` | Show current thinking/tool status | -| `/update [version]` | Update moltis (owner-only) | +| `/peek` | Show current thinking/tool status — **operator DMs only on channels** | +| `/update [version]` | Update moltis — **operator DMs only on channels** | + +```admonish warning title="Some commands are restricted on channels" +Commands scoped to the current conversation — `/help`, `/new`, `/clear`, +`/compact`, `/title`, `/fork`, `/stop`, `/model`, `/mode`, `/fast` — are open to +any sender who clears the channel's access gate. + +Commands that reach the host, act on the owner's behalf, or read state outside +the current chat require the sender to be an **operator in a proven direct +chat**. An empty `operators` list means nobody, so these are disabled until you +configure it. Non-operators can still chat normally. + +Shared-room, unknown-topology, and guest turns receive no tools or owner-private +context. `/sh` and other privileged commands are also denied there. See +[Channels → Operators](channels.md#operators-privileged-senders). +``` ## Quick Actions | Command | Description | |---------|-------------| -| `/btw ` | Quick side question (no tools, not persisted) | +| `/btw ` | Quick side question (no tools, not persisted) — **operator DMs only on channels** | | `/fast [on\|off\|status]` | Toggle fast/priority mode | -| `/insights [days]` | Show usage analytics (tokens, providers) | -| `/steer ` | Inject guidance into the current agent run | -| `/queue ` | Queue a message for the next agent turn | -| `/rollback [N\|diff N]` | List or restore file checkpoints | +| `/insights [days]` | Show usage analytics (tokens, providers) — **operator DMs only on channels** | +| `/steer ` | Inject guidance into the current agent run — **operator DMs only on channels** | +| `/queue ` | Queue a message for the next agent turn — **operator DMs only on channels** | +| `/rollback [N\|diff N]` | List or restore file checkpoints — **operator DMs only on channels** | ### /btw @@ -143,12 +159,12 @@ system. | Command | Description | |---------|-------------| -| `/approvals` | List pending exec approvals | -| `/approve [N]` | Approve a pending exec request | -| `/deny [N]` | Deny a pending exec request | +| `/approvals` | List pending exec approvals — **operator DMs only on channels** | +| `/approve [N]` | Approve a pending exec request — **operator DMs only on channels** | +| `/deny [N]` | Deny a pending exec request — **operator DMs only on channels** | ## Help | Command | Description | |---------|-------------| -| `/help` | Show available commands (handled locally by each channel) | +| `/help` | Show available commands, marking which need an operator | diff --git a/docs/src/discord.md b/docs/src/discord.md index 9c3bad734a..8275eb2e4c 100644 --- a/docs/src/discord.md +++ b/docs/src/discord.md @@ -82,6 +82,7 @@ offered = ["telegram", "discord"] | `mention_mode` | no | `"mention"` | When the bot responds in guilds: `"always"`, `"mention"` (only when @mentioned), or `"none"` | | `allowlist` | no | `[]` | Discord usernames allowed to DM the bot (when `dm_policy = "allowlist"`) | | `guild_allowlist` | no | `[]` | Guild (server) IDs allowed to interact with the bot | +| `operators` | no | `[]` | Exact sender IDs eligible for privileged channel commands. Empty means nobody. Discord currently fails closed as shared even in DMs; see [Operators](./channels.md#operators-privileged-senders) | | `model` | no | — | Override the default model for this channel | | `model_provider` | no | — | Provider for the overridden model | | `agent_id` | no | — | Default agent ID for this Discord bot | @@ -104,8 +105,10 @@ token = "MTIzNDU2Nzg5.example.bot-token" dm_policy = "allowlist" group_policy = "open" mention_mode = "mention" -allowlist = ["alice", "bob"] +allowlist = ["111111111111111111", "222222222222222222"] guild_allowlist = ["123456789012345678"] +# Records operator identity for future proven-direct topology support. +operators = ["111111111111111111"] reply_to_message = true ack_reaction = "👀" model = "gpt-4o" @@ -213,6 +216,11 @@ Slash commands appear in Discord's command palette (type `/` in any channel wher the bot is present). Responses are ephemeral — only visible to the user who invoked the command. +Commands that require an operator direct chat, including `/context`, +`/sessions`, and `/agent`, currently fail closed on Discord because the gateway +cannot yet prove Discord conversation topology from its channel ID. Use the +authenticated web UI for those commands. + ```admonish note Text-based `/` commands (e.g. typing `/model` as a regular message) continue to work alongside native slash commands. The native commands provide autocomplete and @@ -271,6 +279,10 @@ username is listed in the `allowlist` array — otherwise the bot will ignore yo DMs. Set `dm_policy = "open"` to allow anyone to DM the bot. ``` +Discord DMs currently support normal tool-free chat only. Privileged commands, +agent tools, private owner context, and `/sh` fail closed until direct-chat +topology is carried into the gateway. + ### Without a Shared Server DMs work even if you and the bot don't share a server. Discord bots are diff --git a/docs/src/matrix.md b/docs/src/matrix.md index 124256a0f0..8e4182588a 100644 --- a/docs/src/matrix.md +++ b/docs/src/matrix.md @@ -46,7 +46,7 @@ password auth. | Voice and audio messages | Supported | Matrix audio is downloaded and sent through the normal transcription pipeline | | Interactive actions | Supported | Short action lists are sent as native Matrix polls | | Reactions | Supported | Ack reactions and normal reaction flows work | -| Location | Supported | Inbound location shares update user location and outbound location sends are supported | +| Location | Partial | Outbound location sends are supported. Inbound shares do not update owner location while Matrix topology fails closed as shared | | OTP sender approval | Supported | Unknown DM senders can self-approve through the shared OTP flow | | Model routing overrides | Supported | Per-room and per-user model/provider overrides | @@ -57,6 +57,9 @@ The main remaining Matrix-specific limitations are: - Matrix interactive actions are poll-based, not arbitrary button/select UIs - older encrypted history may remain unreadable until the missing room keys are shared with the Moltis device - arbitrary remote-media fetch and reupload for outbound URLs is still limited +- privileged commands, tools, private owner context, and inbound location + updates fail closed because Matrix room topology is not yet carried into the + gateway, including for actual DMs ## How It Works @@ -291,6 +294,7 @@ picker list from `moltis.toml`. | `mention_mode` | no | `"mention"` | When the bot responds in rooms: `"always"`, `"mention"`, or `"none"` | | `room_allowlist` | no | `[]` | Matrix room IDs or aliases allowed to interact with the bot | | `user_allowlist` | no | `[]` | Matrix user IDs allowed to DM the bot | +| `operators` | no | `[]` | Exact sender IDs eligible for privileged access. Matrix currently fails closed as shared even in DMs | | `auto_join` | no | `"always"` | Invite handling: `"always"`, `"allowlist"`, or `"off"`. With `"allowlist"`, invites are accepted only when the inviter is on `user_allowlist` or the room is on `room_allowlist`; empty allowlists deny all invites | | `model` | no | — | Override the default model for this account | | `model_provider` | no | — | Provider for the overridden model | diff --git a/docs/src/security.md b/docs/src/security.md index cb01883be9..ee52a572d9 100644 --- a/docs/src/security.md +++ b/docs/src/security.md @@ -122,13 +122,50 @@ interact with the agent. UI: Settings > Channels > Pending Senders ``` -### Per-Channel Permissions +### Operators vs. Allowed Senders + +Being allowed to message the bot is **not** permission to run commands on the +host. Shell access is gated separately by each account's `operators` list: + +| | Guest | Operator in proven DM | Operator in shared/unknown chat | +|---|---|---|---| +| Chat with the agent | yes | yes | yes | +| Room-local slash commands | yes | yes | yes | +| Privileged slash commands | no | yes | no | +| `/sh`, shell command mode | no | yes | no | +| Agent tools and external agents | no | yes | no | +| Owner memory, profile, project context | no | yes | no | + +Denial is enforced before command dispatch, before shell command-mode rewrite, +and with a deny-all request policy. Host-owned tool audience metadata provides +an additional ceiling, and name-based configuration cannot widen the deny-all +channel policy. Untrusted turns also omit private prompt and memory context. + +With no `operators` list, **no one** has privileged channel access. Configure it under +**Settings → Channels → Edit → Operators**, or see +[Channels → Operators](./channels.md#operators-privileged-senders). + +```admonish danger title="Public channels" +On a public Discord guild or any shared group chat, every member may clear the +access gate. Every turn therefore runs without tools or owner-private context, +even when an operator sends it. `/sh` and privileged commands are also denied; +move privileged work to an operator DM or the authenticated web UI. Adapters +that cannot prove a chat is direct treat it as shared. +``` + +Discord, Microsoft Teams, and Matrix currently fall into that conservative +category even for actual DMs. Their normal chat remains available, but tools, +private context, `/sh`, location updates, and privileged commands are denied. -Each channel can have different permission levels: +Telephony is in that category permanently: a call's only identifier is the +caller number, and caller ID is spoofable, so it can never authenticate an +operator. Adding a phone number to `operators` grants nothing. -- **Read-only**: Sender can ask questions, agent responds -- **Execute**: Sender can trigger actions (with approval still required) -- **Admin**: Full access including configuration changes +Sessions bound to a chat — including a working session an operator moved there +with `/attach` — are untrusted for *every* non-gateway caller, so cron jobs, +webhooks, and `sessions_send` also run tool-free against them. Release the +binding to restore full access; see +[Channels → Channel-bound sessions](./channels.md#channel-bound-sessions). ### Channel Isolation diff --git a/docs/src/teams.md b/docs/src/teams.md index ab1ca8dbf9..1dbbaf222c 100644 --- a/docs/src/teams.md +++ b/docs/src/teams.md @@ -127,6 +127,7 @@ app_password = "your-client-secret-here" | `mention_mode` | no | `"mention"` | When the bot responds in groups: `"always"`, `"mention"`, or `"none"` | | `allowlist` | no | `[]` | AAD object IDs or user IDs allowed to DM the bot | | `group_allowlist` | no | `[]` | Conversation/team IDs allowed for group messages | +| `operators` | no | `[]` | Exact sender IDs eligible for privileged access. Teams currently fails closed as shared even in personal chats | | `otp_self_approval` | no | `true` | Enable OTP self-approval for non-allowlisted DM users | | `otp_cooldown_secs` | no | `300` | Cooldown after failed OTP attempts | | `model` | no | — | Override the default model for this channel | @@ -206,6 +207,13 @@ receive a verification prompt. The PIN is visible to the bot owner in the web UI under **Channels → Senders**. After a correct PIN reply, Moltis adds the sender's Teams user ID to the account allowlist. +```admonish note title="Privileged access fails closed" +Teams personal chats currently support normal tool-free chat only. The gateway +cannot yet prove Teams topology from its conversation ID, so tools, private +owner context, `/sh`, location updates, and privileged commands are denied even +for configured operators. Use the authenticated web UI for privileged work. +``` + ### Group Policy Controls who can interact with the bot in group chats and team channels. diff --git a/docs/src/tool-policy.md b/docs/src/tool-policy.md index 202ed99c14..63b931dde3 100644 --- a/docs/src/tool-policy.md +++ b/docs/src/tool-policy.md @@ -1,9 +1,14 @@ # Tool Policy -Tool policies control which tools are available during a session. Policies -use a layered system where each layer can restrict or widen access, and -**deny always wins** — once a tool is denied at any layer, no later layer -can re-allow it. +Tool policies control which tools are available during a session. Before these +name-based layers run, Moltis applies the registry's host-owned audience +ceiling. Guest, shared-room, and unknown-topology channel turns receive no +tools. Webhooks receive no tools by default and can explicitly opt into tools +registered for public use. Policies may narrow but never widen these ceilings. + +Within the audience ceiling, policies use a layered system where each layer can +restrict or widen access, and **deny always wins** — once a tool is denied at +any layer, no later layer can re-allow it. ## Layers @@ -15,8 +20,8 @@ but deny entries accumulate across all of them. | 1 | Global | `[tools.policy]` | All sessions | | 2 | Per-provider | `[providers..policy]` | Requests routed through that provider | | 3 | Per-agent preset | `[agents.presets..tools]` | Sub-agents spawned with that preset | -| 4 | Per-channel group | `[channels...tools.groups.]` | Channel sessions matching that chat type | -| 5 | Per-sender | `...groups..by_sender.` | Messages from that sender in that group | +| 4 | Per-channel chat type | `[channels...tools.groups.]` | Channel sessions matching that chat type | +| 5 | Per-sender | `...groups..by_sender.` | Messages from that sender in that chat type | | 6 | Sandbox | `[tools.exec.sandbox.tools_policy]` | Commands running inside a sandbox container | **Web UI sessions** see layers 1-3 (no channel context), plus layer 6 if sandboxed. @@ -103,55 +108,39 @@ allowed, and `exec`/`write_file` are explicitly denied. See > `spawn_agent`. They do not affect the main agent session. Use the global > `[tools.policy]` for the main session. -## Layer 4 — Per-Channel Group +## Layer 4 — Per-Channel Chat Type Channel accounts can restrict tools by chat type (`private`, `group`, -`channel`, etc.). This is useful for hardening group chats where the bot -is exposed to untrusted users. +`channel`, etc.). Guest, shared-room, and unknown-topology turns already have a +non-widenable deny-all policy. This layer is primarily useful for narrowing the +tools available to an operator in a proven direct chat. ```toml -[channels.telegram.my-bot.tools.groups.group] +[channels.telegram.my-bot.tools.groups.private] deny = ["exec", "browser"] ``` -In this example, `exec` and `browser` are denied in Telegram group chats -handled by the `my-bot` account. Private chats and web UI sessions are -unaffected. +In this example, `exec` and `browser` are denied in Telegram direct chats +handled by the `my-bot` account. Web UI sessions are unaffected. ## Layer 5 — Per-Sender -Within a channel group, individual senders can receive overrides. This lets -you trust specific users in an otherwise restricted group. +Within a channel chat type, individual senders can receive name-policy +overrides. Overrides cannot cross the deny-all ceiling applied to guests, +shared rooms, and unknown chat kinds. ```toml -[channels.telegram.my-bot.tools.groups.group] +[channels.telegram.my-bot.tools.groups.private] deny = ["exec", "browser"] -[channels.telegram.my-bot.tools.groups.group.by_sender."123456"] -allow = ["*"] -``` - -Sender `123456` gets `allow = ["*"]`, which replaces the previous allow -list. However, because **deny always accumulates**, the `exec` and -`browser` denials from the group layer still apply. The sender override -is useful for widening the allow list (e.g., granting access to tools that -were not in the previous allow set) or for applying a different profile. - -If you need a trusted sender to have `exec` access in a group, avoid -denying `exec` at the group layer. Instead, use a restrictive allow list -at the group level and widen it per-sender: - -```toml -[channels.telegram.my-bot.tools.groups.group] -allow = ["web_search", "web_fetch", "memory_search"] - -[channels.telegram.my-bot.tools.groups.group.by_sender."123456"] +[channels.telegram.my-bot.tools.groups.private.by_sender."123456"] allow = ["*"] ``` -Here, untrusted group members can only use the three listed tools. Sender -`123456` gets full access because the group layer did not deny anything — -it only narrowed the allow list. +Sender `123456` gets `allow = ["*"]`, which replaces the previous allow list. +However, because **deny always accumulates**, the `exec` and `browser` denials +from the chat-type layer still apply. The sender must also be a configured +operator in a proven direct chat; a sender override alone never grants tools. ## Layer 6 — Sandbox @@ -179,28 +168,29 @@ policy.deny = ["exec"] When using OpenAI, the agent cannot run shell commands. All other providers retain their normal tool access. -### Restrict group chats on Telegram +### Restrict operator DMs on Telegram ```toml -[channels.telegram.my-bot.tools.groups.group] +[channels.telegram.my-bot.tools.groups.private] deny = ["exec", "browser*"] ``` -Group chats cannot use `exec` or any tool starting with `browser`. -Private chats are unaffected. +Operators in proven direct chats cannot use `exec` or any tool starting with +`browser`. Guest and shared-room turns already receive no tools. -### Trust a sender in a restricted group +### Narrow one operator in direct chats ```toml -[channels.telegram.my-bot.tools.groups.group] +[channels.telegram.my-bot.tools.groups.private] allow = ["web_search", "web_fetch"] -[channels.telegram.my-bot.tools.groups.group.by_sender."123456"] -allow = ["*"] +[channels.telegram.my-bot.tools.groups.private.by_sender."123456"] +allow = ["web_search"] ``` -Normal group members can only search and fetch. Sender `123456` can use -every tool (nothing was denied at the group layer, so nothing accumulates). +Operator `123456` can only search in a proven direct chat. Other operators in +direct chats can search and fetch. This configuration grants nothing to guests +or shared-room participants. ### Agent preset with limited tools @@ -227,19 +217,19 @@ Then `web_fetch` is denied. The effective policy allows `exec`, `browser`, and `memory`, and denies `web_fetch`. All other tools are not in the allow list and are therefore blocked. -### Widen a sender via profile +### Apply a profile to an operator DM ```toml -[channels.telegram.my-bot.tools.groups.group] +[channels.telegram.my-bot.tools.groups.private] allow = ["web_search"] -[channels.telegram.my-bot.tools.groups.group.by_sender."123456"] +[channels.telegram.my-bot.tools.groups.private.by_sender."123456"] profile = "full" ``` -Sender `123456` gets `allow = ["*"]` from the `full` profile, replacing -the group's narrow allow list. Since the group layer only set `allow` (no -`deny`), nothing is denied and the sender has full tool access. +Operator `123456` gets `allow = ["*"]` from the `full` profile, replacing the +chat-type allow list in a proven direct chat. The same sender still receives no +tools as a guest or in a shared or unknown chat. ## Debugging diff --git a/docs/src/tool-registry.md b/docs/src/tool-registry.md index f04056cc0a..d09093aedc 100644 --- a/docs/src/tool-registry.md +++ b/docs/src/tool-registry.md @@ -1,8 +1,8 @@ # Tool Registry The tool registry manages all tools available to the agent during a -conversation. It tracks where each tool comes from and supports filtering by -source. +conversation. It tracks where each tool comes from, which audience may use it, +and supports filtering by both properties. ## Tool Sources @@ -11,20 +11,42 @@ Every registered tool has a `ToolSource` that identifies its origin: - **`Builtin`** — tools shipped with the binary (exec, web_fetch, etc.) - **`Mcp { server }`** — tools provided by an MCP server, tagged with the server name +- **`Wasm { component_hash }`** — tools provided by a WASM component This replaces the previous convention of identifying MCP tools by their `mcp__` name prefix, providing type-safe filtering instead of string matching. ## Registration +Every entry also has a `ToolAudience`: + +- **`Trusted`** — authenticated owner and authorized operator turns +- **`Public`** — eligible for explicitly configured untrusted origins such as + webhook payloads + +Registration is fail-closed. Built-in tools are trusted-only by default, and +all MCP and WASM tools are trusted-only. A reviewed built-in must explicitly use +`register_public` to become public. Tool descriptions and third-party metadata +cannot grant public access. + +Public registration does not grant a tool to messaging-channel guests or +shared rooms. Those turns receive an additional deny-all request policy. A +webhook also receives no tools unless its configuration explicitly opts in. + ```rust -// Built-in tool +// Trusted-only built-in tool (the default) registry.register(Box::new(MyTool::new())); -// MCP tool — tagged with server name -registry.register_mcp(Box::new(adapter), "github".to_string()); +// Built-in reviewed as safe for untrusted input +registry.register_public(Box::new(PublicTool::new())); + +// Trusted-only MCP tool, tagged with server name +registry.register_mcp(Box::new(adapter), "github".into()); ``` +Duplicate names are rejected rather than overwritten. Intentional wrappers use +`replace`, which preserves the original source and audience metadata. + ## Filtering When MCP tools are disabled for a session, the registry can produce a filtered @@ -36,6 +58,9 @@ let no_mcp = registry.clone_without_mcp(); // Remove all MCP tools in-place (used during sync) let removed_count = registry.unregister_mcp(); + +// Keep only tools explicitly reviewed for public use +let public = registry.clone_for_audience(ToolAudience::Public); ``` ## Schema Output @@ -47,7 +72,8 @@ let removed_count = registry.unregister_mcp(); "name": "exec", "description": "Execute a command", "parameters": { ... }, - "source": "builtin" + "source": "builtin", + "audience": "trusted" } ``` @@ -57,12 +83,14 @@ let removed_count = registry.unregister_mcp(); "description": "Search GitHub", "parameters": { ... }, "source": "mcp", - "mcpServer": "github" + "mcpServer": "github", + "audience": "trusted" } ``` -The `source` and `mcpServer` fields are available to the UI for rendering -tools grouped by origin. +The source and audience fields are available to the UI for diagnostics. The +audience is enforced by registry filtering before model tool schemas are built; +it is not an instruction that the model is expected to follow. ## Lazy Registry Mode diff --git a/docs/src/webhooks.md b/docs/src/webhooks.md index b147b77f74..42e74c6d2a 100644 --- a/docs/src/webhooks.md +++ b/docs/src/webhooks.md @@ -278,7 +278,25 @@ Webhooks can override specific agent settings without changing the base preset: - **Model** — use a different LLM for webhook processing. - **System prompt suffix** — append extra instructions (e.g., "Focus on security issues" for a code review webhook). -- **Tool policy** — restrict which tools the agent can use. +- **Tool policy** — webhook turns have no tools by default. An explicit policy + may opt into tools registered for the public audience, currently `calc`, + `web_search`, and `web_fetch`. Even `allow = ["*"]` cannot widen beyond this + ceiling. Webhook payloads are untrusted input and never inherit trusted-only + tools. + +For example, the webhook create/update payload can opt into search only: + +```json +{ + "toolPolicy": { + "allow": ["web_search"] + } +} +``` + +Omitting `toolPolicy` denies every tool. Supplying an empty policy or only a +`deny` list permits every public tool not denied, so prefer a narrow, non-empty +`allow` list. ### Delivery Message Format diff --git a/scripts/local-validate.sh b/scripts/local-validate.sh index 103d924b46..d17aa6e978 100755 --- a/scripts/local-validate.sh +++ b/scripts/local-validate.sh @@ -231,20 +231,48 @@ changed_files() { fi } -package_name_for_path() { +crate_dir_for_path() { local path="$1" local dir dir="$(dirname "$path")" while [[ "$dir" != "." && "$dir" != "/" ]]; do if [[ -f "$dir/Cargo.toml" ]]; then - sed -nE 's/^name[[:space:]]*=[[:space:]]*"([^"]+)"/\1/p' "$dir/Cargo.toml" | head -n1 + printf '%s' "$dir" return fi dir="$(dirname "$dir")" done } +package_name_for_path() { + local dir + dir="$(crate_dir_for_path "$1")" + [[ -n "$dir" ]] || return + + sed -nE 's/^name[[:space:]]*=[[:space:]]*"([^"]+)"/\1/p' "$dir/Cargo.toml" \ + | tr -d '\r' \ + | head -n1 +} + +# Whether a path is a Cargo integration test target (`--test `). +# +# Only `.rs` files directly under a crate's own `tests/` directory are separate +# test binaries. A `tests/` directory nested inside `src/` — e.g. +# `src/session/tests/tests/channel_binding_tests.rs` — is an ordinary inline +# module compiled into the lib, and asking nextest for `--test` on it fails with +# "no test target named ...". +is_integration_test_target() { + local file="$1" + local crate_dir + crate_dir="$(crate_dir_for_path "$file")" + [[ -n "$crate_dir" ]] || return 1 + [[ "$file" == "$crate_dir"/tests/*.rs ]] || return 1 + # Reject anything deeper than `tests/.rs`. + local rest="${file#"$crate_dir"/tests/}" + [[ "$rest" != */* ]] +} + nextest_base_cmd_for_package() { local package="$1" if [[ "$(uname -s)" == "Darwin" ]]; then @@ -274,7 +302,7 @@ build_targeted_rust_test_cmd() { local base_cmd base_cmd="$(nextest_base_cmd_for_package "$package")" - if [[ "$file" == */tests/*.rs ]]; then + if is_integration_test_target "$file"; then local test_name test_name="$(basename "$file" .rs)" commands+=("$base_cmd --test $test_name")