From 29372576a7a98d2ab1dd8ff20d787c57cebf29e6 Mon Sep 17 00:00:00 2001 From: chaodufashi Date: Wed, 13 May 2026 21:57:57 +0000 Subject: [PATCH] fix(slack): accept raw bot IDs in trustedBotIds --- docs/config-reference.md | 2 +- docs/messaging.md | 2 +- src/slack.rs | 86 +++++++++++++++++++++++++++++++++++----- 3 files changed, 79 insertions(+), 11 deletions(-) diff --git a/docs/config-reference.md b/docs/config-reference.md index 9ddaf40f..a0eef687 100644 --- a/docs/config-reference.md +++ b/docs/config-reference.md @@ -57,7 +57,7 @@ Slack adapter using Socket Mode. Requires both a Bot User OAuth Token and an App | `allow_all_users` | bool \| omit | auto-detect | Same behavior as Discord. | | `allowed_users` | string[] | `[]` | Slack user IDs (e.g. `U0123456789`). | | `allow_bot_messages` | string | `"off"` | Same as Discord. | -| `trusted_bot_ids` | string[] | `[]` | Slack Bot User IDs (`U...`). Find via: click bot profile → Copy member ID. | +| `trusted_bot_ids` | string[] | `[]` | Slack Bot User IDs (`U...`) or Bot IDs (`B...`). `U...` matching resolves event Bot IDs via Slack `bots.info`, so the bot token needs `users:read`. | | `allow_user_messages` | string | `"involved"` | Same as Discord. | | `max_bot_turns` | u32 | `100` | Same as Discord. | diff --git a/docs/messaging.md b/docs/messaging.md index 9172bca0..40b3273d 100644 --- a/docs/messaging.md +++ b/docs/messaging.md @@ -187,7 +187,7 @@ BotA in thread: here's my analysis | Key | Type | Default | Description | |-----|------|---------|-------------| | `allow_bot_messages` | string | `"off"` | `"off"` — ignore bot messages. `"mentions"` — only process bot messages that @mention this bot. `"all"` — process all bot messages (capped by `max_bot_turns`). | -| `trusted_bot_ids` | string[] | `[]` | Whitelist of bot IDs. When non-empty, only these bots pass the bot gate. Empty = any bot (mode permitting). Ignored when `allow_bot_messages = "off"`. | +| `trusted_bot_ids` | string[] | `[]` | Whitelist of bot IDs. For Slack, entries may be Bot User IDs (`U...`) or Bot IDs (`B...`); `U...` matching requires `users:read` so OpenAB can call `bots.info`. Empty = any bot (mode permitting). Ignored when `allow_bot_messages = "off"`. | | `max_bot_turns` | u32 | `20` | Max consecutive bot turns per thread before throttling. A human message resets the counter. | > **Safety:** When `allow_bot_messages = "all"`, a separate hardcoded cap of 10 consecutive bot turns applies regardless of `max_bot_turns`. diff --git a/src/slack.rs b/src/slack.rs index cbe101f2..b1e0b534 100644 --- a/src/slack.rs +++ b/src/slack.rs @@ -200,6 +200,10 @@ impl SlackAdapter { /// Resolve a Bot ID (B...) to Bot User ID (U...) via bots.info API. /// Cached permanently (bot IDs don't change). async fn resolve_bot_user_id(&self, bot_id: &str) -> Option { + if bot_id.is_empty() { + return None; + } + { let cache = self.bot_id_cache.lock().await; if let Some(user_id) = cache.get(bot_id) { @@ -210,6 +214,13 @@ impl SlackAdapter { let resp = self .api_post("bots.info", serde_json::json!({ "bot": bot_id })) .await + .inspect_err(|e| { + warn!( + bot_id, + error = %e, + "failed to resolve Slack bot ID via bots.info" + ) + }) .ok()?; let user_id = resp.get("bot")?.get("user_id")?.as_str()?.to_string(); @@ -221,6 +232,21 @@ impl SlackAdapter { Some(user_id) } + async fn trusted_bot_ids_contains( + &self, + trusted_bot_ids: &HashSet, + event_bot_id: &str, + ) -> bool { + if trusted_bot_ids.is_empty() { + return true; + } + if bot_id_matches_trusted(trusted_bot_ids, event_bot_id, None) { + return true; + } + let resolved = self.resolve_bot_user_id(event_bot_id).await; + bot_id_matches_trusted(trusted_bot_ids, event_bot_id, resolved.as_deref()) + } + /// Check whether the bot has participated in a Slack thread and whether /// other bots have also posted in it. /// Returns `(involved, other_bot_present)`. @@ -538,11 +564,11 @@ pub async fn run_slack_adapter( AllowBots::Mentions | AllowBots::All => { if !trusted_bot_ids.is_empty() { let event_bot_id = event["bot_id"].as_str().unwrap_or(""); - let resolved = adapter.resolve_bot_user_id(event_bot_id).await; - let is_trusted = resolved.as_ref() - .is_some_and(|uid| trusted_bot_ids.contains(uid.as_str())); + let is_trusted = adapter + .trusted_bot_ids_contains(&trusted_bot_ids, event_bot_id) + .await; if !is_trusted { - debug!(event_bot_id, resolved = ?resolved, "bot not in trusted_bot_ids, ignoring app_mention"); + debug!(event_bot_id, "bot not in trusted_bot_ids, ignoring app_mention"); continue; } } @@ -713,12 +739,11 @@ pub async fn run_slack_adapter( } // Check trusted_bot_ids if !trusted_bot_ids.is_empty() { - let resolved = adapter.resolve_bot_user_id(event_bot_id).await; - let is_trusted = resolved - .as_ref() - .is_some_and(|uid| trusted_bot_ids.contains(uid.as_str())); + let is_trusted = adapter + .trusted_bot_ids_contains(&trusted_bot_ids, event_bot_id) + .await; if !is_trusted { - debug!(event_bot_id, resolved = ?resolved, "bot not in trusted_bot_ids, ignoring"); + debug!(event_bot_id, "bot not in trusted_bot_ids, ignoring"); continue; } } @@ -1171,6 +1196,19 @@ fn strip_mime_params(mimetype: &str) -> &str { mimetype.split(';').next().unwrap_or(mimetype).trim() } +fn bot_id_matches_trusted( + trusted_bot_ids: &HashSet, + event_bot_id: &str, + resolved_user_id: Option<&str>, +) -> bool { + if event_bot_id.is_empty() { + return false; + } + + trusted_bot_ids.contains(event_bot_id) + || resolved_user_id.is_some_and(|uid| trusted_bot_ids.contains(uid)) +} + /// True only when a Slack non-bot event represents a real user message /// that should reset the bot-turn counter. /// @@ -1364,6 +1402,36 @@ mod tests { assert_eq!(strip_mime_params(" text/plain "), "text/plain"); } + // --- bot_id_matches_trusted tests --- + + #[test] + fn trusted_bot_ids_accepts_raw_slack_bot_id() { + let trusted = HashSet::from(["B123BOT".to_string()]); + assert!(bot_id_matches_trusted(&trusted, "B123BOT", None)); + } + + #[test] + fn trusted_bot_ids_accepts_resolved_bot_user_id() { + let trusted = HashSet::from(["U123BOT".to_string()]); + assert!(bot_id_matches_trusted( + &trusted, + "B123BOT", + Some("U123BOT") + )); + } + + #[test] + fn trusted_bot_ids_rejects_unknown_bot_when_resolution_fails() { + let trusted = HashSet::from(["U123BOT".to_string()]); + assert!(!bot_id_matches_trusted(&trusted, "B999BOT", None)); + } + + #[test] + fn trusted_bot_ids_rejects_empty_event_bot_id() { + let trusted = HashSet::from(["".to_string()]); + assert!(!bot_id_matches_trusted(&trusted, "", None)); + } + /// Per-thread streaming: ON by default, OFF when another bot is present (#534). #[test] fn streaming_per_thread() {