fix(slack): explain allowlist-denied mentions (#116618)

* fix(slack): explain allowlist-denied mentions * fix(slack): deduplicate denial notices
2026-08-27 21:07:01 -06:00 · 2026-07-30 21:11:06 -07:00
parent 9906f810f6
commit 5e10af048d
5 changed files with 267 additions and 4 deletions
@@ -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<string, unknown> } | null> => ({ ctxPayload: {} }),
+);
 const dispatchPreparedSlackMessageMock = vi.fn(async (_prepared: unknown) => {});
 const resolveThreadTsMock = vi.fn(async ({ message }: { message: Record<string, unknown> }) => ({
  ...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<Record<string, unknown>>;
+    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<string, unknown>;
+    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<string, unknown>;
+    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 = [
@@ -271,6 +271,7 @@ export function createSlackMessageHandler(params: {
                ...lastOpts
              } = last.opts;
              let prepared: Awaited<ReturnType<typeof prepareSlackMessage>>;
+              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();
@@ -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: [],
@@ -169,6 +169,135 @@ describe("slack prepareSlackMessage inbound contract", () => {
    });
  }

+  function createAllowlistDeniedRoomCtx(params: {
+    postEphemeral: ReturnType<typeof vi.fn>;
+  }): 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. <https://docs.openclaw.ai/channels/slack#access-control-and-routing|Learn how to configure Slack channel access.>",
+      });
+      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,
@@ -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<string, unknown> | undefined,
@@ -555,7 +558,9 @@ async function authorizeSlackInboundMessage(params: {
  account: ResolvedSlackAccount;
  message: SlackMessageEvent;
  conversation: SlackConversationContext;
+  explicitBotMention: boolean;
  eventScope?: SlackEventScope;
+  onVisibleDrop?: () => void;
 }): Promise<SlackAuthorizationContext | null> {
  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<PreparedSlackMessage | null> {
  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<string> => {
    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 =