From 5e10af048dec01497ce0aeda850fb7015e16e433 Mon Sep 17 00:00:00 2001 From: Patrick Erichsen Date: Thu, 30 Jul 2026 21:11:06 -0700 Subject: [PATCH] fix(slack): explain allowlist-denied mentions (#116618) * fix(slack): explain allowlist-denied mentions * fix(slack): deduplicate denial notices --- .../slack/src/monitor/message-handler.test.ts | 79 ++++++++++- .../slack/src/monitor/message-handler.ts | 10 ++ .../message-handler/prepare.test-helpers.ts | 3 +- .../monitor/message-handler/prepare.test.ts | 129 ++++++++++++++++++ .../src/monitor/message-handler/prepare.ts | 50 ++++++- 5 files changed, 267 insertions(+), 4 deletions(-) diff --git a/extensions/slack/src/monitor/message-handler.test.ts b/extensions/slack/src/monitor/message-handler.test.ts index 7d7ed0cb3a15..10f8a6a37766 100644 --- a/extensions/slack/src/monitor/message-handler.test.ts +++ b/extensions/slack/src/monitor/message-handler.test.ts @@ -12,7 +12,11 @@ const onFlushCallbacks: Array< createFlush: typeof createTestInboundDebounceFlush, ) => InboundDebounceFlush > = []; -const prepareSlackMessageMock = vi.fn(async () => ({ ctxPayload: {} })); +const prepareSlackMessageMock = vi.fn( + async (_params?: { + opts: { onVisibleDrop?: () => void }; + }): Promise<{ ctxPayload: Record } | null> => ({ ctxPayload: {} }), +); const dispatchPreparedSlackMessageMock = vi.fn(async (_prepared: unknown) => {}); const resolveThreadTsMock = vi.fn(async ({ message }: { message: Record }) => ({ ...message, @@ -457,6 +461,79 @@ describe("createSlackMessageHandler", () => { }, ); + it("prepares a denied message/app_mention twin pair once without dispatching", async () => { + prepareSlackMessageMock.mockImplementationOnce(async (params) => { + params?.opts.onVisibleDrop?.(); + return null; + }); + const { handler } = createHandlerWithTracker(); + const message = { + type: "message" as const, + channel: "C111", + user: "U111", + ts: "1709000000.001881", + text: "<@UBOT> hello", + }; + const asMessage = handler(message as never, { + source: "message", + awaitDispatch: true, + }); + const asMention = handler(message as never, { + source: "app_mention", + wasMentioned: true, + awaitDispatch: true, + }); + await vi.waitFor(() => expect(enqueueMock).toHaveBeenCalledTimes(2)); + + const entries = enqueueMock.mock.calls.map((call) => call[0]) as Array>; + await runOnFlush(entries); + await expect(Promise.all([asMessage, asMention])).resolves.toEqual([undefined, undefined]); + + expect(prepareSlackMessageMock).toHaveBeenCalledExactlyOnceWith( + expect.objectContaining({ + opts: expect.objectContaining({ source: "app_mention", wasMentioned: true }), + }), + ); + expect(dispatchPreparedSlackMessageMock).not.toHaveBeenCalled(); + }); + + it("does not repeat a visible denial for a later message/app_mention twin", async () => { + prepareSlackMessageMock.mockImplementationOnce(async (params) => { + params?.opts.onVisibleDrop?.(); + return null; + }); + const { handler } = createHandlerWithTracker(); + const message = { + type: "message" as const, + channel: "C111", + user: "U111", + ts: "1709000000.001882", + text: "<@UBOT> hello", + }; + + const asMessage = handler(message as never, { + source: "message", + awaitDispatch: true, + }); + await vi.waitFor(() => expect(enqueueMock).toHaveBeenCalledTimes(1)); + const first = enqueueMock.mock.calls[0]?.[0] as Record; + await runOnFlush([first]); + await asMessage; + + const asMention = handler(message as never, { + source: "app_mention", + wasMentioned: true, + awaitDispatch: true, + }); + await vi.waitFor(() => expect(enqueueMock).toHaveBeenCalledTimes(2)); + const second = enqueueMock.mock.calls[1]?.[0] as Record; + await runOnFlush([second]); + await asMention; + + expect(prepareSlackMessageMock).toHaveBeenCalledTimes(1); + expect(dispatchPreparedSlackMessageMock).not.toHaveBeenCalled(); + }); + it("preserves distinct messages and identities in the same debounced flush", async () => { const { handler } = createHandlerWithTracker(); const messages = [ diff --git a/extensions/slack/src/monitor/message-handler.ts b/extensions/slack/src/monitor/message-handler.ts index 33fc4e315438..5505dfb8ecf6 100644 --- a/extensions/slack/src/monitor/message-handler.ts +++ b/extensions/slack/src/monitor/message-handler.ts @@ -271,6 +271,7 @@ export function createSlackMessageHandler(params: { ...lastOpts } = last.opts; let prepared: Awaited>; + let visibleDrop = false; let settlementHandedOff = false; try { prepared = await prepareSlackMessage({ @@ -280,9 +281,18 @@ export function createSlackMessageHandler(params: { opts: { ...lastOpts, wasMentioned: combinedMentioned || last.opts.wasMentioned, + onVisibleDrop: () => { + visibleDrop = true; + }, }, }); if (!prepared) { + if (visibleDrop) { + // The gate already produced a sender-visible notice. Commit the + // logical claim so a later message/app_mention twin cannot repeat it. + await commitClaims(); + return; + } // Gated before dispatch: release so the surviving twin can run the // same gate; nothing visible was produced, so no duplicate risk. releaseClaims(); diff --git a/extensions/slack/src/monitor/message-handler/prepare.test-helpers.ts b/extensions/slack/src/monitor/message-handler/prepare.test-helpers.ts index aaf77cfcbcc1..1fd7a69966ad 100644 --- a/extensions/slack/src/monitor/message-handler/prepare.test-helpers.ts +++ b/extensions/slack/src/monitor/message-handler/prepare.test-helpers.ts @@ -20,6 +20,7 @@ export function createInboundSlackTestContext(params: { channelsConfig?: SlackChannelConfigEntries; dmHistoryLimit?: number; groupDmEnabled?: boolean; + groupPolicy?: "open" | "disabled" | "allowlist"; channelRuntime?: ChannelRuntimeSurface; }) { return createSlackMonitorContext({ @@ -45,7 +46,7 @@ export function createInboundSlackTestContext(params: { groupDmChannels: [], defaultRequireMention: params.defaultRequireMention ?? true, channelsConfig: params.channelsConfig, - groupPolicy: "open", + groupPolicy: params.groupPolicy ?? "open", useAccessGroups: true, reactionMode: "off", reactionAllowlist: [], diff --git a/extensions/slack/src/monitor/message-handler/prepare.test.ts b/extensions/slack/src/monitor/message-handler/prepare.test.ts index 470241410068..3be445fdb6a4 100644 --- a/extensions/slack/src/monitor/message-handler/prepare.test.ts +++ b/extensions/slack/src/monitor/message-handler/prepare.test.ts @@ -169,6 +169,135 @@ describe("slack prepareSlackMessage inbound contract", () => { }); } + function createAllowlistDeniedRoomCtx(params: { + postEphemeral: ReturnType; + }): SlackMonitorContext { + const ctx = createInboundSlackCtx({ + cfg: { + channels: { + slack: { + enabled: true, + groupPolicy: "allowlist", + channels: { C_ALLOWED: { enabled: true } }, + }, + }, + } as OpenClawConfig, + appClient: { + chat: { postEphemeral: params.postEphemeral }, + } as unknown as App["client"], + channelsConfig: { C_ALLOWED: { enabled: true } }, + groupPolicy: "allowlist", + }); + ctx.resolveChannelName = async () => ({ name: "blocked-room", type: "channel" }); + ctx.resolveUserName = async (userId) => ({ + name: userId === ctx.botUserId ? "Personal Claw" : "Alice", + }); + return ctx; + } + + it.each([ + ["message", undefined], + ["app_mention", true], + ] as const)( + "sends an ephemeral allowlist notice for an explicit bot mention from %s", + async (source, wasMentioned) => { + const postEphemeral = vi.fn().mockResolvedValue({ ok: true }); + const ctx = createAllowlistDeniedRoomCtx({ postEphemeral }); + + const prepared = await prepareSlackMessage({ + ctx, + account: defaultAccount, + message: createSlackMessage({ + channel: "C_DENIED", + channel_type: "channel", + user: "U1", + text: "<@B1> hello", + }), + opts: { source, ...(wasMentioned ? { wasMentioned } : {}) }, + }); + + expect(prepared).toBeNull(); + expect(postEphemeral).toHaveBeenCalledExactlyOnceWith({ + token: "token", + channel: "C_DENIED", + user: "U1", + text: "Personal Claw can’t reply here because this channel isn’t in its OpenClaw channel allowlist. Ask the OpenClaw owner to allow this channel. ", + }); + expect(enqueueSystemEventMock).not.toHaveBeenCalled(); + }, + ); + + it("does not send an allowlist notice for an unmentioned denied room message", async () => { + const postEphemeral = vi.fn().mockResolvedValue({ ok: true }); + const ctx = createAllowlistDeniedRoomCtx({ postEphemeral }); + + const prepared = await prepareSlackMessage({ + ctx, + account: defaultAccount, + message: createSlackMessage({ + channel: "C_DENIED", + channel_type: "channel", + user: "U1", + text: "hello", + }), + opts: { source: "message" }, + }); + + expect(prepared).toBeNull(); + expect(postEphemeral).not.toHaveBeenCalled(); + }); + + it("uses generic copy when the bot display-name lookup fails", async () => { + const postEphemeral = vi.fn().mockResolvedValue({ ok: true }); + const ctx = createAllowlistDeniedRoomCtx({ postEphemeral }); + ctx.resolveUserName = vi.fn().mockRejectedValue(new Error("users.info failed")); + + await expect( + prepareSlackMessage({ + ctx, + account: defaultAccount, + message: createSlackMessage({ + channel: "C_DENIED", + channel_type: "channel", + user: "U1", + text: "<@B1> hello", + }), + opts: { source: "app_mention", wasMentioned: true }, + }), + ).resolves.toBeNull(); + + expect(postEphemeral).toHaveBeenCalledExactlyOnceWith( + expect.objectContaining({ + text: expect.stringMatching(/^This OpenClaw bot can’t reply here/), + }), + ); + }); + + it("keeps the original deny behavior when the ephemeral notice fails", async () => { + const postEphemeral = vi.fn().mockRejectedValue(new Error("invalid_auth xoxb-secret-value")); + const ctx = createAllowlistDeniedRoomCtx({ postEphemeral }); + const error = vi.fn(); + ctx.runtime.error = error; + + await expect( + prepareSlackMessage({ + ctx, + account: defaultAccount, + message: createSlackMessage({ + channel: "C_DENIED", + channel_type: "group", + user: "U1", + text: "<@B1> hello", + }), + opts: { source: "app_mention", wasMentioned: true }, + }), + ).resolves.toBeNull(); + + expect(error).toHaveBeenCalledOnce(); + expect(error.mock.calls[0]?.[0]).toContain("slack allowlist denial notice failed"); + expect(error.mock.calls[0]?.[0]).not.toContain("xoxb-secret-value"); + }); + function createOwnerScopedBotRoomCtx(params: { members: string[] }) { const members = vi.fn().mockResolvedValue({ members: params.members, diff --git a/extensions/slack/src/monitor/message-handler/prepare.ts b/extensions/slack/src/monitor/message-handler/prepare.ts index 434ff1f3d04a..f147bd94790f 100644 --- a/extensions/slack/src/monitor/message-handler/prepare.ts +++ b/extensions/slack/src/monitor/message-handler/prepare.ts @@ -72,6 +72,7 @@ import { resolveConversationLabel } from "../conversation.runtime.js"; import { authorizeSlackDirectMessage } from "../dm-auth.js"; import type { SlackEventScope } from "../event-scope.js"; import type { SlackMediaResult } from "../media-types.js"; +import { escapeSlackMrkdwn } from "../mrkdwn.js"; import { resolveSlackRoomContextHints } from "../room-context.js"; import { sendMessageSlack } from "../send.runtime.js"; import { resolveSlackThreadStarter, type SlackThreadStarter } from "../thread.js"; @@ -99,6 +100,8 @@ const SLACK_HISTORY_MEDIA_MAX_ATTACHMENTS = 4; const SLACK_HISTORY_MEDIA_MAX_BYTES = 10 * 1024 * 1024; const SLACK_HISTORY_MEDIA_IDLE_TIMEOUT_MS = 1_000; const SLACK_HISTORY_MEDIA_TOTAL_TIMEOUT_MS = 3_000; +const SLACK_CHANNEL_ACCESS_DOCS_URL = + "https://docs.openclaw.ai/channels/slack#access-control-and-routing"; function recordString( record: Record | undefined, @@ -555,7 +558,9 @@ async function authorizeSlackInboundMessage(params: { account: ResolvedSlackAccount; message: SlackMessageEvent; conversation: SlackConversationContext; + explicitBotMention: boolean; eventScope?: SlackEventScope; + onVisibleDrop?: () => void; }): Promise { const { ctx, account, message, conversation } = params; const { isDirectMessage, channelName, resolvedChannelType, isBotMessage, allowBotsMode } = @@ -589,6 +594,39 @@ async function authorizeSlackInboundMessage(params: { channelType: resolvedChannelType, }) ) { + if ( + conversation.isRoom && + ctx.groupPolicy === "allowlist" && + params.explicitBotMention && + !isBotMessage && + message.user + ) { + let subject = "This OpenClaw bot"; + if (ctx.botUserId) { + try { + const botIdentity = await ctx.resolveUserName(ctx.botUserId, params.eventScope); + const botName = normalizeOptionalString(botIdentity?.name); + if (botName) { + subject = escapeSlackMrkdwn(botName); + } + } catch (error) { + logVerbose(`slack allowlist denial bot-name lookup failed: ${formatSlackError(error)}`); + } + } + try { + await (params.eventScope?.client ?? ctx.app.client).chat.postEphemeral({ + token: ctx.botToken, + channel: message.channel, + user: message.user, + text: `${subject} can’t reply here because this channel isn’t in its OpenClaw channel allowlist. Ask the OpenClaw owner to allow this channel. <${SLACK_CHANNEL_ACCESS_DOCS_URL}|Learn how to configure Slack channel access.>`, + }); + params.onVisibleDrop?.(); + } catch (error) { + ctx.runtime.error?.( + `slack allowlist denial notice failed for channel ${message.channel}: ${formatSlackError(error)}`, + ); + } + } logVerbose("slack: drop message (channel not allowed)"); return null; } @@ -649,6 +687,8 @@ export async function prepareSlackMessage(params: { eventScope?: SlackEventScope; /** Handler-owned race check for suppressing a duplicate dropped-history record. */ shouldRecordDroppedHistory?: () => boolean; + /** Handler-owned signal that a gate produced a user-visible terminal outcome. */ + onVisibleDrop?: () => void; }; }): Promise { const { ctx, account, message, opts } = params; @@ -674,18 +714,25 @@ export async function prepareSlackMessage(params: { allowBotsMode, isBotMessage, } = conversation; + const messageText = message.text ?? ""; + const mentionMetadata = collectSlackMentionMetadata(messageText); + const normalizedBotUserId = normalizeSlackId(ctx.botUserId); + const explicitBotMention = + opts.source === "app_mention" || + Boolean(normalizedBotUserId && mentionMetadata.mentionedUserIds.includes(normalizedBotUserId)); const authorization = await authorizeSlackInboundMessage({ ctx, account, message, conversation, + explicitBotMention, eventScope: opts.eventScope, + onVisibleDrop: opts.onVisibleDrop, }); if (!authorization) { return null; } const { senderId, allowFromLower } = authorization; - const messageText = message.text ?? ""; let resolvedSenderName = normalizeOptionalString(message.username); const resolveSenderName = async (): Promise => { if (resolvedSenderName) { @@ -702,7 +749,6 @@ export async function prepareSlackMessage(params: { resolvedSenderName = message.user ?? message.bot_id ?? "unknown"; return resolvedSenderName; }; - const mentionMetadata = collectSlackMentionMetadata(messageText); const { mentionedUserIds, mentionedSubteamIds, hasAnyMention } = mentionMetadata; const messageAssistantThreadContext = resolveSlackMessageAssistantThreadContext(message); const assistantContextLookupChannelId =