From 75dfb71e4e8b7c2feba5a8ca662f92ea840e0147 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 26 Feb 2026 03:47:52 +0100 Subject: [PATCH] fix(slack): gate pin/reaction system events by sender auth --- CHANGELOG.md | 3 +- src/slack/monitor/events/pins.test.ts | 170 +++++++++++++++++++++ src/slack/monitor/events/pins.ts | 24 +-- src/slack/monitor/events/reactions.test.ts | 30 ++++ src/slack/monitor/events/reactions.ts | 53 ++----- 5 files changed, 227 insertions(+), 53 deletions(-) create mode 100644 src/slack/monitor/events/pins.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index cba652419f4..6a46b277942 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,7 +22,8 @@ Docs: https://docs.openclaw.ai - Models/Auth probes: map permanent auth failover reasons (`auth_permanent`, for example revoked keys) into probe auth status instead of `unknown`, so `openclaw models status --probe` reports actionable auth failures. (#25754) thanks @rrenamed. - 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.25`). Thanks @tdjackey for reporting. - Security/Exec approvals: bind `system.run` approval matching to exact argv identity and preserve argv whitespace in rendered command text, preventing trailing-space executable path swaps from reusing a mismatched approval. This ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting. -- Security/Discord + Slack 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.25`). 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.25`). 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.25`). 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.25`). Thanks @tdjackey for reporting. - 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.25`). Thanks @tdjackey for reporting. - Security/macOS beta onboarding: remove Anthropic OAuth sign-in and the legacy `oauth.json` onboarding path that exposed the PKCE verifier via OAuth `state`; this impacted the macOS beta onboarding path only. Anthropic subscription auth is now setup-token-only and will ship in the next npm release (`2026.2.25`). Thanks @zdi-disclosures for reporting. diff --git a/src/slack/monitor/events/pins.test.ts b/src/slack/monitor/events/pins.test.ts new file mode 100644 index 00000000000..3bdae247613 --- /dev/null +++ b/src/slack/monitor/events/pins.test.ts @@ -0,0 +1,170 @@ +import { describe, expect, it, vi } from "vitest"; +import type { SlackMonitorContext } from "../context.js"; +import { registerSlackPinEvents } from "./pins.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 SlackPinHandler = (args: { event: Record; body: unknown }) => Promise; + +function createPinContext(overrides?: { + dmPolicy?: "open" | "pairing" | "allowlist" | "disabled"; + allowFrom?: string[]; + channelType?: "im" | "channel"; + channelUsers?: string[]; +}) { + let addedHandler: SlackPinHandler | null = null; + let removedHandler: SlackPinHandler | null = null; + const channelType = overrides?.channelType ?? "im"; + const app = { + event: vi.fn((name: string, handler: SlackPinHandler) => { + if (name === "pin_added") { + addedHandler = handler; + } else if (name === "pin_removed") { + removedHandler = handler; + } + }), + }; + const ctx = { + app, + runtime: { error: vi.fn() }, + dmEnabled: true, + dmPolicy: overrides?.dmPolicy ?? "open", + defaultRequireMention: true, + channelsConfig: overrides?.channelUsers + ? { + C1: { + users: overrides.channelUsers, + allow: true, + }, + } + : undefined, + groupPolicy: "open", + allowFrom: overrides?.allowFrom ?? [], + allowNameMatching: false, + shouldDropMismatchedSlackEvent: vi.fn().mockReturnValue(false), + isChannelAllowed: vi.fn().mockReturnValue(true), + resolveChannelName: vi.fn().mockResolvedValue({ + name: channelType === "im" ? "direct" : "general", + type: channelType, + }), + resolveUserName: vi.fn().mockResolvedValue({ name: "alice" }), + resolveSlackSystemEventSessionKey: vi.fn().mockReturnValue("agent:main:main"), + } as unknown as SlackMonitorContext; + registerSlackPinEvents({ ctx }); + return { + ctx, + getAddedHandler: () => addedHandler, + getRemovedHandler: () => removedHandler, + }; +} + +function makePinEvent(overrides?: { user?: string; channel?: string }) { + return { + type: "pin_added", + user: overrides?.user ?? "U1", + channel_id: overrides?.channel ?? "D1", + event_ts: "123.456", + item: { + type: "message", + message: { + ts: "123.456", + }, + }, + }; +} + +describe("registerSlackPinEvents", () => { + it("enqueues DM pin system events when dmPolicy is open", async () => { + enqueueSystemEventMock.mockClear(); + readAllowFromStoreMock.mockReset().mockResolvedValue([]); + const { getAddedHandler } = createPinContext({ dmPolicy: "open" }); + const addedHandler = getAddedHandler(); + expect(addedHandler).toBeTruthy(); + + await addedHandler!({ + event: makePinEvent(), + body: {}, + }); + + expect(enqueueSystemEventMock).toHaveBeenCalledTimes(1); + }); + + it("blocks DM pin system events when dmPolicy is disabled", async () => { + enqueueSystemEventMock.mockClear(); + readAllowFromStoreMock.mockReset().mockResolvedValue([]); + const { getAddedHandler } = createPinContext({ dmPolicy: "disabled" }); + const addedHandler = getAddedHandler(); + expect(addedHandler).toBeTruthy(); + + await addedHandler!({ + event: makePinEvent(), + body: {}, + }); + + expect(enqueueSystemEventMock).not.toHaveBeenCalled(); + }); + + it("blocks DM pin system events for unauthorized senders in allowlist mode", async () => { + enqueueSystemEventMock.mockClear(); + readAllowFromStoreMock.mockReset().mockResolvedValue([]); + const { getAddedHandler } = createPinContext({ + dmPolicy: "allowlist", + allowFrom: ["U2"], + }); + const addedHandler = getAddedHandler(); + expect(addedHandler).toBeTruthy(); + + await addedHandler!({ + event: makePinEvent({ user: "U1" }), + body: {}, + }); + + expect(enqueueSystemEventMock).not.toHaveBeenCalled(); + }); + + it("allows DM pin system events for authorized senders in allowlist mode", async () => { + enqueueSystemEventMock.mockClear(); + readAllowFromStoreMock.mockReset().mockResolvedValue([]); + const { getAddedHandler } = createPinContext({ + dmPolicy: "allowlist", + allowFrom: ["U1"], + }); + const addedHandler = getAddedHandler(); + expect(addedHandler).toBeTruthy(); + + await addedHandler!({ + event: makePinEvent({ user: "U1" }), + body: {}, + }); + + expect(enqueueSystemEventMock).toHaveBeenCalledTimes(1); + }); + + it("blocks channel pin events for users outside channel users allowlist", async () => { + enqueueSystemEventMock.mockClear(); + readAllowFromStoreMock.mockReset().mockResolvedValue([]); + const { getAddedHandler } = createPinContext({ + dmPolicy: "open", + channelType: "channel", + channelUsers: ["U_OWNER"], + }); + const addedHandler = getAddedHandler(); + expect(addedHandler).toBeTruthy(); + + await addedHandler!({ + event: makePinEvent({ channel: "C1", user: "U_ATTACKER" }), + body: {}, + }); + + expect(enqueueSystemEventMock).not.toHaveBeenCalled(); + }); +}); diff --git a/src/slack/monitor/events/pins.ts b/src/slack/monitor/events/pins.ts index 2613bc35e24..89d0e2264e8 100644 --- a/src/slack/monitor/events/pins.ts +++ b/src/slack/monitor/events/pins.ts @@ -1,6 +1,7 @@ import type { SlackEventMiddlewareArgs } from "@slack/bolt"; -import { danger } from "../../../globals.js"; +import { danger, logVerbose } from "../../../globals.js"; import { enqueueSystemEvent } from "../../../infra/system-events.js"; +import { authorizeSlackSystemEventSender } from "../auth.js"; import { resolveSlackChannelLabel } from "../channel-config.js"; import type { SlackMonitorContext } from "../context.js"; import type { SlackPinEvent } from "../types.js"; @@ -22,19 +23,20 @@ async function handleSlackPinEvent(params: { const payload = event as SlackPinEvent; const channelId = payload.channel_id; - const channelInfo = channelId ? await ctx.resolveChannelName(channelId) : {}; - if ( - !ctx.isChannelAllowed({ - channelId, - channelName: channelInfo?.name, - channelType: channelInfo?.type, - }) - ) { + const auth = await authorizeSlackSystemEventSender({ + ctx, + senderId: payload.user, + channelId, + }); + if (!auth.allowed) { + logVerbose( + `slack: drop pin sender ${payload.user ?? "unknown"} channel=${channelId ?? "unknown"} reason=${auth.reason ?? "unauthorized"}`, + ); return; } const label = resolveSlackChannelLabel({ channelId, - channelName: channelInfo?.name, + channelName: auth.channelName, }); const userInfo = payload.user ? await ctx.resolveUserName(payload.user) : {}; const userLabel = userInfo?.name ?? payload.user ?? "someone"; @@ -42,7 +44,7 @@ async function handleSlackPinEvent(params: { const messageId = payload.item?.message?.ts ?? payload.event_ts; const sessionKey = ctx.resolveSlackSystemEventSessionKey({ channelId, - channelType: channelInfo?.type ?? undefined, + channelType: auth.channelType, }); enqueueSystemEvent(`Slack: ${userLabel} ${action} a ${itemType} in ${label}.`, { sessionKey, diff --git a/src/slack/monitor/events/reactions.test.ts b/src/slack/monitor/events/reactions.test.ts index 815ca1c65b2..bb64fbb5b4a 100644 --- a/src/slack/monitor/events/reactions.test.ts +++ b/src/slack/monitor/events/reactions.test.ts @@ -22,6 +22,7 @@ function createReactionContext(overrides?: { dmPolicy?: "open" | "pairing" | "allowlist" | "disabled"; allowFrom?: string[]; channelType?: "im" | "channel"; + channelUsers?: string[]; }) { let addedHandler: SlackReactionHandler | null = null; let removedHandler: SlackReactionHandler | null = null; @@ -38,7 +39,17 @@ function createReactionContext(overrides?: { const ctx = { app, runtime: { error: vi.fn() }, + dmEnabled: true, dmPolicy: overrides?.dmPolicy ?? "open", + defaultRequireMention: true, + channelsConfig: overrides?.channelUsers + ? { + C1: { + users: overrides.channelUsers, + allow: true, + }, + } + : undefined, groupPolicy: "open", allowFrom: overrides?.allowFrom ?? [], allowNameMatching: false, @@ -160,4 +171,23 @@ describe("registerSlackReactionEvents", () => { expect(enqueueSystemEventMock).toHaveBeenCalledTimes(1); }); + + it("blocks channel reaction events for users outside channel users allowlist", async () => { + enqueueSystemEventMock.mockClear(); + readAllowFromStoreMock.mockReset().mockResolvedValue([]); + const { getAddedHandler } = createReactionContext({ + dmPolicy: "open", + channelType: "channel", + channelUsers: ["U_OWNER"], + }); + const addedHandler = getAddedHandler(); + expect(addedHandler).toBeTruthy(); + + await addedHandler!({ + event: makeReactionEvent({ channel: "C1", user: "U_ATTACKER" }), + body: {}, + }); + + expect(enqueueSystemEventMock).not.toHaveBeenCalled(); + }); }); diff --git a/src/slack/monitor/events/reactions.ts b/src/slack/monitor/events/reactions.ts index 5007c6aad93..844b6c94080 100644 --- a/src/slack/monitor/events/reactions.ts +++ b/src/slack/monitor/events/reactions.ts @@ -1,9 +1,7 @@ import type { SlackEventMiddlewareArgs } from "@slack/bolt"; import { danger, logVerbose } from "../../../globals.js"; import { enqueueSystemEvent } from "../../../infra/system-events.js"; -import { resolveDmGroupAccessWithLists } from "../../../security/dm-policy-shared.js"; -import { resolveSlackAllowListMatch } from "../allow-list.js"; -import { resolveSlackEffectiveAllowFrom } from "../auth.js"; +import { authorizeSlackSystemEventSender } from "../auth.js"; import { resolveSlackChannelLabel } from "../channel-config.js"; import type { SlackMonitorContext } from "../context.js"; import type { SlackReactionEvent } from "../types.js"; @@ -18,50 +16,23 @@ export function registerSlackReactionEvents(params: { ctx: SlackMonitorContext } return; } - const channelInfo = item.channel ? await ctx.resolveChannelName(item.channel) : {}; - const channelType = channelInfo?.type; - if ( - !ctx.isChannelAllowed({ - channelId: item.channel, - channelName: channelInfo?.name, - channelType, - }) - ) { + const auth = await authorizeSlackSystemEventSender({ + ctx, + senderId: event.user, + channelId: item.channel, + }); + if (!auth.allowed) { + logVerbose( + `slack: drop reaction sender ${event.user ?? "unknown"} channel=${item.channel ?? "unknown"} reason=${auth.reason ?? "unauthorized"}`, + ); return; } const channelLabel = resolveSlackChannelLabel({ channelId: item.channel, - channelName: channelInfo?.name, + channelName: auth.channelName, }); const actorInfo = event.user ? await ctx.resolveUserName(event.user) : undefined; - if (channelType === "im") { - if (!event.user) { - return; - } - const { allowFromLower } = await resolveSlackEffectiveAllowFrom(ctx); - const access = resolveDmGroupAccessWithLists({ - isGroup: false, - dmPolicy: ctx.dmPolicy, - groupPolicy: ctx.groupPolicy, - allowFrom: allowFromLower, - groupAllowFrom: [], - storeAllowFrom: [], - isSenderAllowed: (allowList) => - resolveSlackAllowListMatch({ - allowList, - id: event.user, - name: actorInfo?.name, - allowNameMatching: ctx.allowNameMatching, - }).allowed, - }); - if (access.decision !== "allow") { - logVerbose( - `slack: drop reaction sender ${event.user} (dmPolicy=${ctx.dmPolicy}, decision=${access.decision}, reason=${access.reason})`, - ); - return; - } - } const actorLabel = actorInfo?.name ?? event.user; const emojiLabel = event.reaction ?? "emoji"; const authorInfo = event.item_user ? await ctx.resolveUserName(event.item_user) : undefined; @@ -70,7 +41,7 @@ export function registerSlackReactionEvents(params: { ctx: SlackMonitorContext } const text = authorLabel ? `${baseText} from ${authorLabel}` : baseText; const sessionKey = ctx.resolveSlackSystemEventSessionKey({ channelId: item.channel, - channelType, + channelType: auth.channelType, }); enqueueSystemEvent(text, { sessionKey,