diff --git a/CHANGELOG.md b/CHANGELOG.md index 5fc9dd69c..f6d129905 100644 --- 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. diff --git a/src/slack/monitor/events/members.test.ts b/src/slack/monitor/events/members.test.ts new file mode 100644 index 000000000..bc9c6805a --- /dev/null +++ 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; + body: unknown; +}) => Promise; + +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(); + }); +}); diff --git a/src/slack/monitor/events/members.ts b/src/slack/monitor/events/members.ts index 652c75bb4..ca7907706 100644 --- 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) { diff --git a/src/slack/monitor/events/messages.test.ts b/src/slack/monitor/events/messages.test.ts new file mode 100644 index 000000000..0534cdcfa --- /dev/null +++ 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; + body: unknown; +}) => Promise; + +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(); + }); +}); diff --git a/src/slack/monitor/events/messages.ts b/src/slack/monitor/events/messages.ts index 0ccb8dc10..5d16bb967 100644 --- 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; diff --git a/src/slack/monitor/types.ts b/src/slack/monitor/types.ts index c77bd53f9..58c103e04 100644 --- 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; };