diff --git a/CHANGELOG.md b/CHANGELOG.md index fbade503071..52a805e3850 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ Docs: https://docs.openclaw.ai - Agents/subagents: bound automatic orphan recovery with persisted recovery attempts and a wedged-session tombstone, and teach task maintenance/doctor to reconcile those sessions so restart loops no longer require manual `sessions.json` surgery. Fixes #74864. Thanks @solosage1. - Gateway/startup: skip pre-bind web-fetch provider discovery for credential-free `tools.web.fetch` config, so Docker/Kubernetes gateways bind even when optional fetch limits are present. Fixes #74896. Thanks @KoykL. +- Slack: require bot-authored room messages with `allowBots=true` to come from an explicitly channel-allowlisted bot or from a room where an explicit Slack owner is present, so broad bot relays cannot run unattended. Fixes #59284. Thanks @andrewhong-translucent. - CLI/progress: suppress nested progress spinners and line clears while TUI input owns raw stdin, so Crestodian `/status` no longer disturbs the active input row. (#75003) Thanks @velvet-shark. - Models/OpenAI Codex: restore `openai-codex/gpt-5.4-mini` for ChatGPT/Codex OAuth PI runs after live OAuth proof, and align the manifest, forward-compat metadata, docs, and regression tests so stale cron and heartbeat configs resolve again. Fixes #74451. Thanks @0xCyda, @hclsys, and @Marvae. - Telegram: use durable message edits for streaming previews instead of native draft state, so generated replies no longer flicker through draft-to-message transitions that look like duplicates. (#75073) Thanks @obviyus. diff --git a/docs/channels/slack.md b/docs/channels/slack.md index 7ef96b71f1f..2b1c295b676 100644 --- a/docs/channels/slack.md +++ b/docs/channels/slack.md @@ -582,6 +582,8 @@ Current Slack message actions include `send`, `upload-file`, `download-file`, `r - `toolsBySender` key format: `id:`, `e164:`, `username:`, `name:`, or `"*"` wildcard (legacy unprefixed keys still map to `id:` only) + `allowBots` is conservative for channels and private channels: bot-authored room messages are accepted only when the sending bot is explicitly listed in that room's `users` allowlist, or when at least one explicit Slack owner ID from `channels.slack.allowFrom` is currently a room member. Wildcards and display-name owner entries do not satisfy owner presence. Owner presence uses Slack `conversations.members`; make sure the app has the matching read scope for the room type (`channels:read` for public channels, `groups:read` for private channels). If the member lookup fails, OpenClaw drops the bot-authored room message. + diff --git a/extensions/slack/src/monitor/auth.ts b/extensions/slack/src/monitor/auth.ts index 4bcf169af43..6e102615885 100644 --- a/extensions/slack/src/monitor/auth.ts +++ b/extensions/slack/src/monitor/auth.ts @@ -1,8 +1,11 @@ +import { formatErrorMessage } from "openclaw/plugin-sdk/error-runtime"; +import { logVerbose } from "openclaw/plugin-sdk/runtime-env"; import { readStoreAllowFromForDmPolicy } from "openclaw/plugin-sdk/security-runtime"; import { allowListMatches, normalizeAllowList, normalizeAllowListLower, + normalizeSlackAllowOwnerEntry, resolveSlackAllowListMatch, resolveSlackUserAllowed, } from "./allow-list.js"; @@ -24,8 +27,20 @@ type SlackAllowFromCacheState = { pairingPending?: Promise; }; +type SlackChannelMembersCacheEntry = { + expiresAtMs: number; + members?: Set; + pending?: Promise>; +}; + let slackAllowFromCache = new WeakMap(); +let slackChannelMembersCache = new WeakMap< + SlackMonitorContext, + Map +>(); const DEFAULT_PAIRING_ALLOW_FROM_CACHE_TTL_MS = 5000; +const DEFAULT_CHANNEL_MEMBERS_CACHE_TTL_MS = 60_000; +const CHANNEL_MEMBERS_CACHE_MAX = 512; function getPairingAllowFromCacheTtlMs(): number { const raw = process.env.OPENCLAW_SLACK_PAIRING_ALLOWFROM_CACHE_TTL_MS?.trim(); @@ -39,6 +54,18 @@ function getPairingAllowFromCacheTtlMs(): number { return Math.max(0, Math.floor(parsed)); } +function getChannelMembersCacheTtlMs(): number { + const raw = process.env.OPENCLAW_SLACK_CHANNEL_MEMBERS_CACHE_TTL_MS?.trim(); + if (!raw) { + return DEFAULT_CHANNEL_MEMBERS_CACHE_TTL_MS; + } + const parsed = Number(raw); + if (!Number.isFinite(parsed)) { + return DEFAULT_CHANNEL_MEMBERS_CACHE_TTL_MS; + } + return Math.max(0, Math.floor(parsed)); +} + function getAllowFromCacheState(ctx: SlackMonitorContext): SlackAllowFromCacheState { const existing = slackAllowFromCache.get(ctx); if (existing) { @@ -49,6 +76,28 @@ function getAllowFromCacheState(ctx: SlackMonitorContext): SlackAllowFromCacheSt return next; } +function getChannelMembersCache( + ctx: SlackMonitorContext, +): Map { + const existing = slackChannelMembersCache.get(ctx); + if (existing) { + return existing; + } + const next = new Map(); + slackChannelMembersCache.set(ctx, next); + return next; +} + +function pruneChannelMembersCache(cache: Map): void { + while (cache.size > CHANNEL_MEMBERS_CACHE_MAX) { + const oldest = cache.keys().next(); + if (oldest.done) { + return; + } + cache.delete(oldest.value); + } +} + function buildBaseAllowFrom(ctx: SlackMonitorContext): ResolvedAllowFromLists { const allowFrom = normalizeAllowList(ctx.allowFrom); return { @@ -131,6 +180,10 @@ export async function resolveSlackEffectiveAllowFrom( export function clearSlackAllowFromCacheForTest(): void { slackAllowFromCache = new WeakMap(); + slackChannelMembersCache = new WeakMap< + SlackMonitorContext, + Map + >(); } export function isSlackSenderAllowListed(params: { @@ -151,6 +204,128 @@ export function isSlackSenderAllowListed(params: { ); } +async function fetchSlackChannelMemberIds( + ctx: SlackMonitorContext, + channelId: string, +): Promise> { + const members = new Set(); + let cursor: string | undefined; + do { + const response = await ctx.app.client.conversations.members({ + token: ctx.botToken, + channel: channelId, + limit: 999, + ...(cursor ? { cursor } : {}), + }); + for (const member of normalizeAllowListLower(response.members)) { + members.add(member); + } + const nextCursor = response.response_metadata?.next_cursor?.trim(); + cursor = nextCursor ? nextCursor : undefined; + } while (cursor); + return members; +} + +async function resolveSlackChannelMemberIds( + ctx: SlackMonitorContext, + channelId: string, +): Promise> { + const cache = getChannelMembersCache(ctx); + const key = `${ctx.accountId}:${channelId}`; + const ttlMs = getChannelMembersCacheTtlMs(); + const nowMs = Date.now(); + const cached = cache.get(key); + if (ttlMs > 0 && cached?.members && cached.expiresAtMs >= nowMs) { + return cached.members; + } + if (cached?.pending) { + return await cached.pending; + } + + const pending = fetchSlackChannelMemberIds(ctx, channelId); + cache.set(key, { + expiresAtMs: ttlMs > 0 ? nowMs + ttlMs : 0, + pending, + }); + pruneChannelMembersCache(cache); + try { + const members = await pending; + if (ttlMs > 0) { + cache.set(key, { + expiresAtMs: Date.now() + ttlMs, + members, + }); + pruneChannelMembersCache(cache); + } else { + cache.delete(key); + } + return members; + } finally { + const latest = cache.get(key); + if (latest?.pending === pending) { + cache.delete(key); + } + } +} + +function resolveExplicitSlackOwnerIds(allowFromLower: string[]): string[] { + const ownerIds = new Set(); + for (const entry of allowFromLower) { + const ownerId = normalizeSlackAllowOwnerEntry(entry); + if (ownerId) { + ownerIds.add(ownerId); + } + } + return [...ownerIds]; +} + +export async function authorizeSlackBotRoomMessage(params: { + ctx: SlackMonitorContext; + channelId: string; + senderId: string; + senderName?: string; + channelUsers?: Array; + allowFromLower: string[]; +}): Promise { + const channelUserAllowList = normalizeAllowListLower(params.channelUsers).filter( + (entry) => entry !== "*", + ); + if ( + channelUserAllowList.length > 0 && + allowListMatches({ + allowList: channelUserAllowList, + id: params.senderId, + name: params.senderName, + allowNameMatching: params.ctx.allowNameMatching, + }) + ) { + return true; + } + + const explicitOwnerIds = resolveExplicitSlackOwnerIds(params.allowFromLower); + if (explicitOwnerIds.length === 0) { + logVerbose( + `slack: drop bot message ${params.senderId} in ${params.channelId} (no explicit owner id for presence check)`, + ); + return false; + } + + try { + const channelMemberIds = await resolveSlackChannelMemberIds(params.ctx, params.channelId); + if (explicitOwnerIds.some((ownerId) => channelMemberIds.has(ownerId))) { + return true; + } + logVerbose( + `slack: drop bot message ${params.senderId} in ${params.channelId} (no owner present)`, + ); + } catch (error) { + logVerbose( + `slack: drop bot message ${params.senderId} in ${params.channelId} (owner presence lookup failed: ${formatErrorMessage(error)})`, + ); + } + return false; +} + export type SlackSystemEventAuthResult = { allowed: boolean; reason?: diff --git a/extensions/slack/src/monitor/message-handler/prepare.test.ts b/extensions/slack/src/monitor/message-handler/prepare.test.ts index be910e77328..8e3b736072a 100644 --- a/extensions/slack/src/monitor/message-handler/prepare.test.ts +++ b/extensions/slack/src/monitor/message-handler/prepare.test.ts @@ -17,6 +17,7 @@ import { recordSlackThreadParticipation, } from "../../sent-thread-cache.js"; import type { SlackMessageEvent } from "../../types.js"; +import { clearSlackAllowFromCacheForTest } from "../auth.js"; import type { SlackMonitorContext } from "../context.js"; import { resetSlackThreadStarterCacheForTest } from "../thread.js"; import { resolveSlackMessageContent } from "./prepare-content.js"; @@ -37,6 +38,7 @@ describe("slack prepareSlackMessage inbound contract", () => { beforeEach(() => { resetSlackThreadStarterCacheForTest(); clearSlackThreadParticipationCache(); + clearSlackAllowFromCacheForTest(); }); afterAll(() => { @@ -86,6 +88,37 @@ describe("slack prepareSlackMessage inbound contract", () => { } as SlackMessageEvent; } + function createBotRoomMessage(overrides: Partial = {}): SlackMessageEvent { + return createSlackMessage({ + channel: "C123", + channel_type: "channel", + user: undefined, + bot_id: "B0AGV8EQYA3", + subtype: "bot_message", + username: "deploy-bot", + text: "Readiness probe failed", + ...overrides, + }); + } + + function createOwnerScopedBotRoomCtx(params: { members: string[] }) { + const members = vi.fn().mockResolvedValue({ + members: params.members, + response_metadata: { next_cursor: "" }, + }); + const slackCtx = createInboundSlackCtx({ + cfg: { + channels: { + slack: { enabled: true }, + }, + } as OpenClawConfig, + appClient: { conversations: { members } } as unknown as App["client"], + defaultRequireMention: false, + }); + slackCtx.allowFrom = ["UOWNER"]; + return { slackCtx, members }; + } + async function prepareMessageWith( ctx: SlackMonitorContext, account: ResolvedSlackAccount, @@ -424,6 +457,83 @@ describe("slack prepareSlackMessage inbound contract", () => { expect(prepared!.ctxPayload.RawBody).toContain("Readiness probe failed"); }); + it("drops bot-authored room messages when allowBots is true but no owner is present (#59284)", async () => { + const { slackCtx, members } = createOwnerScopedBotRoomCtx({ members: ["UOTHER"] }); + + const prepared = await prepareMessageWith( + slackCtx, + createSlackAccount({ allowBots: true }), + createBotRoomMessage(), + ); + + expect(prepared).toBeNull(); + expect(members).toHaveBeenCalledWith( + expect.objectContaining({ token: "token", channel: "C123", limit: 999 }), + ); + }); + + it("allows bot-authored room messages when an explicit owner is present (#59284)", async () => { + const { slackCtx, members } = createOwnerScopedBotRoomCtx({ members: ["UOWNER"] }); + + const prepared = await prepareMessageWith( + slackCtx, + createSlackAccount({ allowBots: true }), + createBotRoomMessage(), + ); + + expect(prepared).toBeTruthy(); + expect(prepared!.ctxPayload.RawBody).toContain("Readiness probe failed"); + expect(members).toHaveBeenCalledTimes(1); + }); + + it("allows bot-authored room messages when the bot is explicitly channel-allowlisted (#59284)", async () => { + const members = vi.fn(); + const slackCtx = createInboundSlackCtx({ + cfg: { + channels: { + slack: { enabled: true }, + }, + } as OpenClawConfig, + appClient: { conversations: { members } } as unknown as App["client"], + defaultRequireMention: false, + channelsConfig: { + C123: { users: ["B0AGV8EQYA3"] }, + }, + }); + + const prepared = await prepareMessageWith( + slackCtx, + createSlackAccount({ allowBots: true }), + createBotRoomMessage(), + ); + + expect(prepared).toBeTruthy(); + expect(prepared!.ctxPayload.RawBody).toContain("Readiness probe failed"); + expect(members).not.toHaveBeenCalled(); + }); + + it("drops bot-authored room messages when owner presence lookup fails (#59284)", async () => { + const members = vi.fn().mockRejectedValue(new Error("missing_scope")); + const slackCtx = createInboundSlackCtx({ + cfg: { + channels: { + slack: { enabled: true }, + }, + } as OpenClawConfig, + appClient: { conversations: { members } } as unknown as App["client"], + defaultRequireMention: false, + }); + slackCtx.allowFrom = ["UOWNER"]; + + const prepared = await prepareMessageWith( + slackCtx, + createSlackAccount({ allowBots: true }), + createBotRoomMessage(), + ); + + expect(prepared).toBeNull(); + }); + it("keeps channel metadata out of GroupSystemPrompt", async () => { const slackCtx = createInboundSlackCtx({ cfg: { diff --git a/extensions/slack/src/monitor/message-handler/prepare.ts b/extensions/slack/src/monitor/message-handler/prepare.ts index 2f2abef9611..77f0b9615ba 100644 --- a/extensions/slack/src/monitor/message-handler/prepare.ts +++ b/extensions/slack/src/monitor/message-handler/prepare.ts @@ -41,7 +41,7 @@ import { resolveSlackAllowListMatch, resolveSlackUserAllowed, } from "../allow-list.js"; -import { resolveSlackEffectiveAllowFrom } from "../auth.js"; +import { authorizeSlackBotRoomMessage, resolveSlackEffectiveAllowFrom } from "../auth.js"; import { resolveSlackChannelConfig } from "../channel-config.js"; import { stripSlackMentionsForCommandDetection } from "../commands.js"; import { @@ -271,6 +271,7 @@ export async function prepareSlackMessage(params: { isRoom, isRoomish, channelConfig, + allowBots, isBotMessage, } = conversation; const authorization = await authorizeSlackInboundMessage({ @@ -394,6 +395,21 @@ export async function prepareSlackMessage(params: { logVerbose(`Blocked unauthorized slack sender ${senderId} (not in channel users)`); return null; } + if ( + isRoom && + isBotMessage && + allowBots && + !(await authorizeSlackBotRoomMessage({ + ctx, + channelId: message.channel, + senderId, + senderName: senderNameForAuth, + channelUsers: channelConfig?.users, + allowFromLower, + })) + ) { + return null; + } const allowTextCommands = shouldHandleTextCommands({ cfg,