fix(slack): gate member and message subtype system events

2026-02-26 12:48:10 +01:00
parent da0ba1b73a
commit 3d30ba18a2
6 changed files with 381 additions and 58 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -91,6 +91,7 @@ Docs: https://docs.openclaw.ai
 - Security/Signal: enforce DM/group authorization before reaction-only notification enqueue so unauthorized senders can no longer inject Signal reaction system events under `dmPolicy`/`groupPolicy`; reaction notifications now require channel access checks first. This ships in the next npm release (`2026.2.26`). Thanks @tdjackey for reporting.
 - Security/Discord reactions: enforce DM policy/allowlist authorization before reaction-event system enqueue in direct messages; Discord reaction handling now also honors DM/group-DM enablement and guild `groupPolicy` channel gating to keep reaction ingress aligned with normal message preflight. This ships in the next npm release (`2026.2.26`). Thanks @tdjackey for reporting.
 - Security/Slack reactions + pins: gate `reaction_*` and `pin_*` system-event enqueue through shared sender authorization so DM `dmPolicy`/`allowFrom` and channel `users` allowlists are enforced consistently for non-message ingress, with regression coverage for denied/allowed sender paths. This ships in the next npm release (`2026.2.26`). Thanks @tdjackey for reporting.
+- Security/Slack member + message subtype events: gate `member_*` plus `message_changed`/`message_deleted`/`thread_broadcast` system-event enqueue through shared sender authorization so DM `dmPolicy`/`allowFrom` and channel `users` allowlists are enforced consistently for non-message ingress; message subtype system events now fail closed when sender identity is missing, with regression coverage. This ships in the next npm release (`2026.2.26`). Thanks @tdjackey for reporting.
 - Security/Telegram reactions: enforce `dmPolicy`/`allowFrom` and group allowlist authorization on `message_reaction` events before enqueueing reaction system events, preventing unauthorized reaction-triggered input in DMs and groups; ships in the next npm release (`2026.2.26`). Thanks @tdjackey for reporting.
 - Security/Telegram group allowlist: fail closed for group sender authorization by removing DM pairing-store fallback from group allowlist evaluation; group sender access now requires explicit `groupAllowFrom` or per-group/per-topic `allowFrom`. (#25988) Thanks @bmendonca3.
 - Security/Slack interactions: enforce channel/DM authorization and modal actor binding (`private_metadata.userId`) before enqueueing `block_action`/`view_submission`/`view_closed` system events, with regression coverage for unauthorized senders and missing/mismatched actor metadata. This ships in the next npm release (`2026.2.26`). Thanks @tdjackey for reporting.
--- a/src/slack/monitor/events/members.test.ts
+++ b/src/slack/monitor/events/members.test.ts
@@ -0,0 +1,131 @@
+import { describe, expect, it, vi } from "vitest";
+import { registerSlackMemberEvents } from "./members.js";
+import {
+  createSlackSystemEventTestHarness,
+  type SlackSystemEventTestOverrides,
+} from "./system-event-test-harness.js";
+
+const enqueueSystemEventMock = vi.fn();
+const readAllowFromStoreMock = vi.fn();
+
+vi.mock("../../../infra/system-events.js", () => ({
+  enqueueSystemEvent: (...args: unknown[]) => enqueueSystemEventMock(...args),
+}));
+
+vi.mock("../../../pairing/pairing-store.js", () => ({
+  readChannelAllowFromStore: (...args: unknown[]) => readAllowFromStoreMock(...args),
+}));
+
+type SlackMemberHandler = (args: {
+  event: Record<string, unknown>;
+  body: unknown;
+}) => Promise<void>;
+
+function createMembersContext(overrides?: SlackSystemEventTestOverrides) {
+  const harness = createSlackSystemEventTestHarness(overrides);
+  registerSlackMemberEvents({ ctx: harness.ctx });
+  return {
+    getJoinedHandler: () =>
+      harness.getHandler("member_joined_channel") as SlackMemberHandler | null,
+    getLeftHandler: () => harness.getHandler("member_left_channel") as SlackMemberHandler | null,
+  };
+}
+
+function makeMemberEvent(overrides?: { user?: string; channel?: string }) {
+  return {
+    type: "member_joined_channel",
+    user: overrides?.user ?? "U1",
+    channel: overrides?.channel ?? "D1",
+    event_ts: "123.456",
+  };
+}
+
+describe("registerSlackMemberEvents", () => {
+  it("enqueues DM member events when dmPolicy is open", async () => {
+    enqueueSystemEventMock.mockClear();
+    readAllowFromStoreMock.mockReset().mockResolvedValue([]);
+    const { getJoinedHandler } = createMembersContext({ dmPolicy: "open" });
+    const joinedHandler = getJoinedHandler();
+    expect(joinedHandler).toBeTruthy();
+
+    await joinedHandler!({
+      event: makeMemberEvent(),
+      body: {},
+    });
+
+    expect(enqueueSystemEventMock).toHaveBeenCalledTimes(1);
+  });
+
+  it("blocks DM member events when dmPolicy is disabled", async () => {
+    enqueueSystemEventMock.mockClear();
+    readAllowFromStoreMock.mockReset().mockResolvedValue([]);
+    const { getJoinedHandler } = createMembersContext({ dmPolicy: "disabled" });
+    const joinedHandler = getJoinedHandler();
+    expect(joinedHandler).toBeTruthy();
+
+    await joinedHandler!({
+      event: makeMemberEvent(),
+      body: {},
+    });
+
+    expect(enqueueSystemEventMock).not.toHaveBeenCalled();
+  });
+
+  it("blocks DM member events for unauthorized senders in allowlist mode", async () => {
+    enqueueSystemEventMock.mockClear();
+    readAllowFromStoreMock.mockReset().mockResolvedValue([]);
+    const { getJoinedHandler } = createMembersContext({
+      dmPolicy: "allowlist",
+      allowFrom: ["U2"],
+    });
+    const joinedHandler = getJoinedHandler();
+    expect(joinedHandler).toBeTruthy();
+
+    await joinedHandler!({
+      event: makeMemberEvent({ user: "U1" }),
+      body: {},
+    });
+
+    expect(enqueueSystemEventMock).not.toHaveBeenCalled();
+  });
+
+  it("allows DM member events for authorized senders in allowlist mode", async () => {
+    enqueueSystemEventMock.mockClear();
+    readAllowFromStoreMock.mockReset().mockResolvedValue([]);
+    const { getLeftHandler } = createMembersContext({
+      dmPolicy: "allowlist",
+      allowFrom: ["U1"],
+    });
+    const leftHandler = getLeftHandler();
+    expect(leftHandler).toBeTruthy();
+
+    await leftHandler!({
+      event: {
+        ...makeMemberEvent({ user: "U1" }),
+        type: "member_left_channel",
+      },
+      body: {},
+    });
+
+    expect(enqueueSystemEventMock).toHaveBeenCalledTimes(1);
+  });
+
+  it("blocks channel member events for users outside channel users allowlist", async () => {
+    enqueueSystemEventMock.mockClear();
+    readAllowFromStoreMock.mockReset().mockResolvedValue([]);
+    const { getJoinedHandler } = createMembersContext({
+      dmPolicy: "open",
+      channelType: "channel",
+      channelUsers: ["U_OWNER"],
+    });
+    const joinedHandler = getJoinedHandler();
+    expect(joinedHandler).toBeTruthy();
+
+    await joinedHandler!({
+      event: makeMemberEvent({ channel: "C1", user: "U_ATTACKER" }),
+      body: {},
+    });
+
+    expect(enqueueSystemEventMock).not.toHaveBeenCalled();
+  });
+});
--- a/src/slack/monitor/events/members.ts
+++ b/src/slack/monitor/events/members.ts
@@ -1,9 +1,9 @@
 import type { SlackEventMiddlewareArgs } from "@slack/bolt";
 import { danger } from "../../../globals.js";
 import { enqueueSystemEvent } from "../../../infra/system-events.js";
-import { resolveSlackChannelLabel } from "../channel-config.js";
 import type { SlackMonitorContext } from "../context.js";
 import type { SlackMemberChannelEvent } from "../types.js";
+import { authorizeAndResolveSlackSystemEventContext } from "./system-event-context.js";

 export function registerSlackMemberEvents(params: { ctx: SlackMonitorContext }) {
  const { ctx } = params;
@@ -21,27 +21,20 @@ export function registerSlackMemberEvents(params: { ctx: SlackMonitorContext })
      const channelId = payload.channel;
      const channelInfo = channelId ? await ctx.resolveChannelName(channelId) : {};
      const channelType = payload.channel_type ?? channelInfo?.type;
-      if (
-        !ctx.isChannelAllowed({
-          channelId,
-          channelName: channelInfo?.name,
-          channelType,
-        })
-      ) {
+      const ingressContext = await authorizeAndResolveSlackSystemEventContext({
+        ctx,
+        senderId: payload.user,
+        channelId,
+        channelType,
+        eventKind: `member-${params.verb}`,
+      });
+      if (!ingressContext) {
        return;
      }
      const userInfo = payload.user ? await ctx.resolveUserName(payload.user) : {};
      const userLabel = userInfo?.name ?? payload.user ?? "someone";
-      const label = resolveSlackChannelLabel({
-        channelId,
-        channelName: channelInfo?.name,
-      });
-      const sessionKey = ctx.resolveSlackSystemEventSessionKey({
-        channelId,
-        channelType,
-      });
-      enqueueSystemEvent(`Slack: ${userLabel} ${params.verb} ${label}.`, {
-        sessionKey,
+      enqueueSystemEvent(`Slack: ${userLabel} ${params.verb} ${ingressContext.channelLabel}.`, {
+        sessionKey: ingressContext.sessionKey,
        contextKey: `slack:member:${params.verb}:${channelId ?? "unknown"}:${payload.user ?? "unknown"}`,
      });
    } catch (err) {
--- a/src/slack/monitor/events/messages.test.ts
+++ b/src/slack/monitor/events/messages.test.ts
@@ -0,0 +1,196 @@
+import { describe, expect, it, vi } from "vitest";
+import { registerSlackMessageEvents } from "./messages.js";
+import {
+  createSlackSystemEventTestHarness,
+  type SlackSystemEventTestOverrides,
+} from "./system-event-test-harness.js";
+
+const enqueueSystemEventMock = vi.fn();
+const readAllowFromStoreMock = vi.fn();
+
+vi.mock("../../../infra/system-events.js", () => ({
+  enqueueSystemEvent: (...args: unknown[]) => enqueueSystemEventMock(...args),
+}));
+
+vi.mock("../../../pairing/pairing-store.js", () => ({
+  readChannelAllowFromStore: (...args: unknown[]) => readAllowFromStoreMock(...args),
+}));
+
+type SlackMessageHandler = (args: {
+  event: Record<string, unknown>;
+  body: unknown;
+}) => Promise<void>;
+
+function createMessagesContext(overrides?: SlackSystemEventTestOverrides) {
+  const harness = createSlackSystemEventTestHarness(overrides);
+  const handleSlackMessage = vi.fn(async () => {});
+  registerSlackMessageEvents({
+    ctx: harness.ctx,
+    handleSlackMessage,
+  });
+  return {
+    getMessageHandler: () => harness.getHandler("message") as SlackMessageHandler | null,
+    handleSlackMessage,
+  };
+}
+
+function makeChangedEvent(overrides?: { channel?: string; user?: string }) {
+  const user = overrides?.user ?? "U1";
+  return {
+    type: "message",
+    subtype: "message_changed",
+    channel: overrides?.channel ?? "D1",
+    message: {
+      ts: "123.456",
+      user,
+    },
+    previous_message: {
+      ts: "123.450",
+      user,
+    },
+    event_ts: "123.456",
+  };
+}
+
+function makeDeletedEvent(overrides?: { channel?: string; user?: string }) {
+  return {
+    type: "message",
+    subtype: "message_deleted",
+    channel: overrides?.channel ?? "D1",
+    deleted_ts: "123.456",
+    previous_message: {
+      ts: "123.450",
+      user: overrides?.user ?? "U1",
+    },
+    event_ts: "123.456",
+  };
+}
+
+function makeThreadBroadcastEvent(overrides?: { channel?: string; user?: string }) {
+  const user = overrides?.user ?? "U1";
+  return {
+    type: "message",
+    subtype: "thread_broadcast",
+    channel: overrides?.channel ?? "D1",
+    user,
+    message: {
+      ts: "123.456",
+      user,
+    },
+    event_ts: "123.456",
+  };
+}
+
+describe("registerSlackMessageEvents", () => {
+  it("enqueues message_changed system events when dmPolicy is open", async () => {
+    enqueueSystemEventMock.mockClear();
+    readAllowFromStoreMock.mockReset().mockResolvedValue([]);
+    const { getMessageHandler } = createMessagesContext({ dmPolicy: "open" });
+    const messageHandler = getMessageHandler();
+    expect(messageHandler).toBeTruthy();
+
+    await messageHandler!({
+      event: makeChangedEvent(),
+      body: {},
+    });
+
+    expect(enqueueSystemEventMock).toHaveBeenCalledTimes(1);
+  });
+
+  it("blocks message_changed system events when dmPolicy is disabled", async () => {
+    enqueueSystemEventMock.mockClear();
+    readAllowFromStoreMock.mockReset().mockResolvedValue([]);
+    const { getMessageHandler } = createMessagesContext({ dmPolicy: "disabled" });
+    const messageHandler = getMessageHandler();
+    expect(messageHandler).toBeTruthy();
+
+    await messageHandler!({
+      event: makeChangedEvent(),
+      body: {},
+    });
+
+    expect(enqueueSystemEventMock).not.toHaveBeenCalled();
+  });
+
+  it("blocks message_changed system events for unauthorized senders in allowlist mode", async () => {
+    enqueueSystemEventMock.mockClear();
+    readAllowFromStoreMock.mockReset().mockResolvedValue([]);
+    const { getMessageHandler } = createMessagesContext({
+      dmPolicy: "allowlist",
+      allowFrom: ["U2"],
+    });
+    const messageHandler = getMessageHandler();
+    expect(messageHandler).toBeTruthy();
+
+    await messageHandler!({
+      event: makeChangedEvent({ user: "U1" }),
+      body: {},
+    });
+
+    expect(enqueueSystemEventMock).not.toHaveBeenCalled();
+  });
+
+  it("blocks message_deleted system events for users outside channel users allowlist", async () => {
+    enqueueSystemEventMock.mockClear();
+    readAllowFromStoreMock.mockReset().mockResolvedValue([]);
+    const { getMessageHandler } = createMessagesContext({
+      dmPolicy: "open",
+      channelType: "channel",
+      channelUsers: ["U_OWNER"],
+    });
+    const messageHandler = getMessageHandler();
+    expect(messageHandler).toBeTruthy();
+
+    await messageHandler!({
+      event: makeDeletedEvent({ channel: "C1", user: "U_ATTACKER" }),
+      body: {},
+    });
+
+    expect(enqueueSystemEventMock).not.toHaveBeenCalled();
+  });
+
+  it("blocks thread_broadcast system events without an authenticated sender", async () => {
+    enqueueSystemEventMock.mockClear();
+    readAllowFromStoreMock.mockReset().mockResolvedValue([]);
+    const { getMessageHandler } = createMessagesContext({ dmPolicy: "open" });
+    const messageHandler = getMessageHandler();
+    expect(messageHandler).toBeTruthy();
+
+    await messageHandler!({
+      event: {
+        ...makeThreadBroadcastEvent(),
+        user: undefined,
+        message: {
+          ts: "123.456",
+        },
+      },
+      body: {},
+    });
+
+    expect(enqueueSystemEventMock).not.toHaveBeenCalled();
+  });
+
+  it("passes regular message events to the message handler", async () => {
+    enqueueSystemEventMock.mockClear();
+    readAllowFromStoreMock.mockReset().mockResolvedValue([]);
+    const { getMessageHandler, handleSlackMessage } = createMessagesContext({
+      dmPolicy: "open",
+    });
+    const messageHandler = getMessageHandler();
+    expect(messageHandler).toBeTruthy();
+
+    await messageHandler!({
+      event: {
+        type: "message",
+        channel: "D1",
+        user: "U1",
+        text: "hello",
+        ts: "123.456",
+      },
+      body: {},
+    });
+
+    expect(handleSlackMessage).toHaveBeenCalledTimes(1);
+    expect(enqueueSystemEventMock).not.toHaveBeenCalled();
+  });
+});
--- a/src/slack/monitor/events/messages.ts
+++ b/src/slack/monitor/events/messages.ts
@@ -2,7 +2,6 @@ import type { SlackEventMiddlewareArgs } from "@slack/bolt";
 import { danger } from "../../../globals.js";
 import { enqueueSystemEvent } from "../../../infra/system-events.js";
 import type { SlackAppMentionEvent, SlackMessageEvent } from "../../types.js";
-import { resolveSlackChannelLabel } from "../channel-config.js";
 import type { SlackMonitorContext } from "../context.js";
 import type { SlackMessageHandler } from "../message-handler.js";
 import type {
@@ -10,6 +9,7 @@ import type {
  SlackMessageDeletedEvent,
  SlackThreadBroadcastEvent,
 } from "../types.js";
+import { authorizeAndResolveSlackSystemEventContext } from "./system-event-context.js";

 export function registerSlackMessageEvents(params: {
  ctx: SlackMonitorContext;
@@ -17,30 +17,15 @@ export function registerSlackMessageEvents(params: {
 }) {
  const { ctx, handleSlackMessage } = params;

-  const resolveSlackChannelSystemEventTarget = async (channelId: string | undefined) => {
-    const channelInfo = channelId ? await ctx.resolveChannelName(channelId) : {};
-    const channelType = channelInfo?.type;
-    if (
-      !ctx.isChannelAllowed({
-        channelId,
-        channelName: channelInfo?.name,
-        channelType,
-      })
-    ) {
-      return null;
-    }
-
-    const label = resolveSlackChannelLabel({
-      channelId,
-      channelName: channelInfo?.name,
-    });
-    const sessionKey = ctx.resolveSlackSystemEventSessionKey({
-      channelId,
-      channelType,
-    });
-
-    return { channelInfo, channelType, label, sessionKey };
-  };
+  const resolveChangedSenderId = (changed: SlackMessageChangedEvent): string | undefined =>
+    changed.message?.user ??
+    changed.previous_message?.user ??
+    changed.message?.bot_id ??
+    changed.previous_message?.bot_id;
+  const resolveDeletedSenderId = (deleted: SlackMessageDeletedEvent): string | undefined =>
+    deleted.previous_message?.user ?? deleted.previous_message?.bot_id;
+  const resolveThreadBroadcastSenderId = (thread: SlackThreadBroadcastEvent): string | undefined =>
+    thread.user ?? thread.message?.user ?? thread.message?.bot_id;

  ctx.app.event("message", async ({ event, body }: SlackEventMiddlewareArgs<"message">) => {
    try {
@@ -52,13 +37,18 @@ export function registerSlackMessageEvents(params: {
      if (message.subtype === "message_changed") {
        const changed = event as SlackMessageChangedEvent;
        const channelId = changed.channel;
-        const target = await resolveSlackChannelSystemEventTarget(channelId);
-        if (!target) {
+        const ingressContext = await authorizeAndResolveSlackSystemEventContext({
+          ctx,
+          senderId: resolveChangedSenderId(changed),
+          channelId,
+          eventKind: "message_changed",
+        });
+        if (!ingressContext) {
          return;
        }
        const messageId = changed.message?.ts ?? changed.previous_message?.ts;
-        enqueueSystemEvent(`Slack message edited in ${target.label}.`, {
-          sessionKey: target.sessionKey,
+        enqueueSystemEvent(`Slack message edited in ${ingressContext.channelLabel}.`, {
+          sessionKey: ingressContext.sessionKey,
          contextKey: `slack:message:changed:${channelId ?? "unknown"}:${messageId ?? changed.event_ts ?? "unknown"}`,
        });
        return;
@@ -66,12 +56,17 @@ export function registerSlackMessageEvents(params: {
      if (message.subtype === "message_deleted") {
        const deleted = event as SlackMessageDeletedEvent;
        const channelId = deleted.channel;
-        const target = await resolveSlackChannelSystemEventTarget(channelId);
-        if (!target) {
+        const ingressContext = await authorizeAndResolveSlackSystemEventContext({
+          ctx,
+          senderId: resolveDeletedSenderId(deleted),
+          channelId,
+          eventKind: "message_deleted",
+        });
+        if (!ingressContext) {
          return;
        }
-        enqueueSystemEvent(`Slack message deleted in ${target.label}.`, {
-          sessionKey: target.sessionKey,
+        enqueueSystemEvent(`Slack message deleted in ${ingressContext.channelLabel}.`, {
+          sessionKey: ingressContext.sessionKey,
          contextKey: `slack:message:deleted:${channelId ?? "unknown"}:${deleted.deleted_ts ?? deleted.event_ts ?? "unknown"}`,
        });
        return;
@@ -79,13 +74,18 @@ export function registerSlackMessageEvents(params: {
      if (message.subtype === "thread_broadcast") {
        const thread = event as SlackThreadBroadcastEvent;
        const channelId = thread.channel;
-        const target = await resolveSlackChannelSystemEventTarget(channelId);
-        if (!target) {
+        const ingressContext = await authorizeAndResolveSlackSystemEventContext({
+          ctx,
+          senderId: resolveThreadBroadcastSenderId(thread),
+          channelId,
+          eventKind: "thread_broadcast",
+        });
+        if (!ingressContext) {
          return;
        }
        const messageId = thread.message?.ts ?? thread.event_ts;
-        enqueueSystemEvent(`Slack thread reply broadcast in ${target.label}.`, {
-          sessionKey: target.sessionKey,
+        enqueueSystemEvent(`Slack thread reply broadcast in ${ingressContext.channelLabel}.`, {
+          sessionKey: ingressContext.sessionKey,
          contextKey: `slack:thread:broadcast:${channelId ?? "unknown"}:${messageId ?? "unknown"}`,
        });
        return;
--- a/src/slack/monitor/types.ts
+++ b/src/slack/monitor/types.ts
@@ -66,8 +66,8 @@ export type SlackMessageChangedEvent = {
  type: "message";
  subtype: "message_changed";
  channel?: string;
-  message?: { ts?: string };
-  previous_message?: { ts?: string };
+  message?: { ts?: string; user?: string; bot_id?: string };
+  previous_message?: { ts?: string; user?: string; bot_id?: string };
  event_ts?: string;
 };

@@ -76,6 +76,7 @@ export type SlackMessageDeletedEvent = {
  subtype: "message_deleted";
  channel?: string;
  deleted_ts?: string;
+  previous_message?: { ts?: string; user?: string; bot_id?: string };
  event_ts?: string;
 };

@@ -83,7 +84,8 @@ export type SlackThreadBroadcastEvent = {
  type: "message";
  subtype: "thread_broadcast";
  channel?: string;
-  message?: { ts?: string };
+  user?: string;
+  message?: { ts?: string; user?: string; bot_id?: string };
  event_ts?: string;
 };