From 0f36ee5a2ed5fecdc1763b0b1166f62c0a0bb5e8 Mon Sep 17 00:00:00 2001 From: Colin Johnson Date: Sun, 1 Mar 2026 10:40:57 -0500 Subject: [PATCH] Slack: harden slash and interactions ingress checks (openclaw#29091) thanks @Solvely-Colin Verified: - pnpm build - pnpm check - pnpm test:macmini Co-authored-by: Solvely-Colin <211764741+Solvely-Colin@users.noreply.github.com> Co-authored-by: Tak Hoffman <781889+Takhoffman@users.noreply.github.com> --- CHANGELOG.md | 1 + src/slack/monitor/context.test.ts | 82 ++++++++++++++++++ src/slack/monitor/context.ts | 13 ++- src/slack/monitor/events/interactions.test.ts | 85 +++++++++++++++++++ src/slack/monitor/events/interactions.ts | 10 +++ src/slack/monitor/slash.test.ts | 24 ++++++ src/slack/monitor/slash.ts | 26 +++++- 7 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 src/slack/monitor/context.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d6961e7b..7171cd864 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -81,6 +81,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- Slack/Security ingress mismatch guard: drop slash-command and interaction payloads when app/team identifiers do not match the active Slack account context (including nested `team.id` interaction payloads), preventing cross-app or cross-workspace payload injection into system-event handling. (#29091) Thanks @Solvely-Colin. - Cron/Failure alerts: add configurable repeated-failure alerting with per-job overrides and Web UI cron editor support (`inherit|disabled|custom` with threshold/cooldown/channel/target fields). (#24789) Thanks xbrak. - Cron/Isolated model defaults: resolve isolated cron `subagents.model` (including object-form `primary`) through allowlist-aware model selection so isolated cron runs honor subagent model defaults unless explicitly overridden by job payload model. (#11474) Thanks @AnonO6. - Cron/Isolated sessions list: persist the intended pre-run model/provider on isolated cron session entries so `sessions_list` reflects payload/session model overrides even when runs fail before post-run telemetry persistence. (#21279) Thanks @altaywtf. diff --git a/src/slack/monitor/context.test.ts b/src/slack/monitor/context.test.ts new file mode 100644 index 000000000..73b37e272 --- /dev/null +++ b/src/slack/monitor/context.test.ts @@ -0,0 +1,82 @@ +import type { App } from "@slack/bolt"; +import { describe, expect, it } from "vitest"; +import type { OpenClawConfig } from "../../config/config.js"; +import type { RuntimeEnv } from "../../runtime.js"; +import { createSlackMonitorContext } from "./context.js"; + +function createTestContext() { + return createSlackMonitorContext({ + cfg: { + channels: { slack: { enabled: true } }, + session: { dmScope: "main" }, + } as OpenClawConfig, + accountId: "default", + botToken: "xoxb-test", + app: { client: {} } as App, + runtime: {} as RuntimeEnv, + botUserId: "U_BOT", + teamId: "T_EXPECTED", + apiAppId: "A_EXPECTED", + historyLimit: 0, + sessionScope: "per-sender", + mainKey: "main", + dmEnabled: true, + dmPolicy: "open", + allowFrom: [], + allowNameMatching: false, + groupDmEnabled: false, + groupDmChannels: [], + defaultRequireMention: true, + groupPolicy: "allowlist", + useAccessGroups: true, + reactionMode: "off", + reactionAllowlist: [], + replyToMode: "off", + threadHistoryScope: "thread", + threadInheritParent: false, + slashCommand: { + enabled: true, + name: "openclaw", + ephemeral: true, + sessionPrefix: "slack:slash", + }, + textLimit: 4000, + ackReactionScope: "group-mentions", + mediaMaxBytes: 20 * 1024 * 1024, + removeAckAfterReply: false, + }); +} + +describe("createSlackMonitorContext shouldDropMismatchedSlackEvent", () => { + it("drops mismatched top-level app/team identifiers", () => { + const ctx = createTestContext(); + expect( + ctx.shouldDropMismatchedSlackEvent({ + api_app_id: "A_WRONG", + team_id: "T_EXPECTED", + }), + ).toBe(true); + expect( + ctx.shouldDropMismatchedSlackEvent({ + api_app_id: "A_EXPECTED", + team_id: "T_WRONG", + }), + ).toBe(true); + }); + + it("drops mismatched nested team.id payloads used by interaction bodies", () => { + const ctx = createTestContext(); + expect( + ctx.shouldDropMismatchedSlackEvent({ + api_app_id: "A_EXPECTED", + team: { id: "T_WRONG" }, + }), + ).toBe(true); + expect( + ctx.shouldDropMismatchedSlackEvent({ + api_app_id: "A_EXPECTED", + team: { id: "T_EXPECTED" }, + }), + ).toBe(false); + }); +}); diff --git a/src/slack/monitor/context.ts b/src/slack/monitor/context.ts index f43f77a0d..63fa3907f 100644 --- a/src/slack/monitor/context.ts +++ b/src/slack/monitor/context.ts @@ -365,9 +365,18 @@ export function createSlackMonitorContext(params: { if (!body || typeof body !== "object") { return false; } - const raw = body as { api_app_id?: unknown; team_id?: unknown }; + const raw = body as { + api_app_id?: unknown; + team_id?: unknown; + team?: { id?: unknown }; + }; const incomingApiAppId = typeof raw.api_app_id === "string" ? raw.api_app_id : ""; - const incomingTeamId = typeof raw.team_id === "string" ? raw.team_id : ""; + const incomingTeamId = + typeof raw.team_id === "string" + ? raw.team_id + : typeof raw.team?.id === "string" + ? raw.team.id + : ""; if (params.apiAppId && incomingApiAppId && incomingApiAppId !== params.apiAppId) { logVerbose( diff --git a/src/slack/monitor/events/interactions.test.ts b/src/slack/monitor/events/interactions.test.ts index cfd535063..7b5f0d24f 100644 --- a/src/slack/monitor/events/interactions.test.ts +++ b/src/slack/monitor/events/interactions.test.ts @@ -65,6 +65,7 @@ function createContext(overrides?: { allowFrom?: string[]; allowNameMatching?: boolean; channelsConfig?: Record; + shouldDropMismatchedSlackEvent?: (body: unknown) => boolean; isChannelAllowed?: (params: { channelId?: string; channelName?: string; @@ -128,6 +129,8 @@ function createContext(overrides?: { allowNameMatching: overrides?.allowNameMatching ?? false, channelsConfig: overrides?.channelsConfig ?? {}, defaultRequireMention: true, + shouldDropMismatchedSlackEvent: (body: unknown) => + overrides?.shouldDropMismatchedSlackEvent?.(body) ?? false, isChannelAllowed, resolveUserName, resolveChannelName, @@ -224,6 +227,88 @@ describe("registerSlackInteractionEvents", () => { expect(app.client.chat.update).toHaveBeenCalledTimes(1); }); + it("drops block actions when mismatch guard triggers", async () => { + enqueueSystemEventMock.mockClear(); + const { ctx, app, getHandler } = createContext({ + shouldDropMismatchedSlackEvent: () => true, + }); + registerSlackInteractionEvents({ ctx: ctx as never }); + + const handler = getHandler(); + expect(handler).toBeTruthy(); + + const ack = vi.fn().mockResolvedValue(undefined); + const respond = vi.fn().mockResolvedValue(undefined); + await handler!({ + ack, + respond, + body: { + user: { id: "U123" }, + team: { id: "T9" }, + channel: { id: "C1" }, + container: { channel_id: "C1", message_ts: "100.200" }, + message: { + ts: "100.200", + text: "fallback", + blocks: [], + }, + }, + action: { + type: "button", + action_id: "openclaw:verify", + }, + }); + + expect(ack).toHaveBeenCalledTimes(1); + expect(enqueueSystemEventMock).not.toHaveBeenCalled(); + expect(app.client.chat.update).not.toHaveBeenCalled(); + expect(respond).not.toHaveBeenCalled(); + }); + + it("drops modal lifecycle payloads when mismatch guard triggers", async () => { + enqueueSystemEventMock.mockClear(); + const { ctx, getViewHandler, getViewClosedHandler } = createContext({ + shouldDropMismatchedSlackEvent: () => true, + }); + registerSlackInteractionEvents({ ctx: ctx as never }); + + const viewHandler = getViewHandler(); + const viewClosedHandler = getViewClosedHandler(); + expect(viewHandler).toBeTruthy(); + expect(viewClosedHandler).toBeTruthy(); + + const ackSubmit = vi.fn().mockResolvedValue(undefined); + await viewHandler!({ + ack: ackSubmit, + body: { + user: { id: "U123" }, + team: { id: "T9" }, + view: { + id: "V123", + callback_id: "openclaw:deploy_form", + private_metadata: JSON.stringify({ userId: "U123" }), + }, + }, + }); + expect(ackSubmit).toHaveBeenCalledTimes(1); + + const ackClosed = vi.fn().mockResolvedValue(undefined); + await viewClosedHandler!({ + ack: ackClosed, + body: { + user: { id: "U123" }, + team: { id: "T9" }, + view: { + id: "V123", + callback_id: "openclaw:deploy_form", + private_metadata: JSON.stringify({ userId: "U123" }), + }, + }, + }); + expect(ackClosed).toHaveBeenCalledTimes(1); + expect(enqueueSystemEventMock).not.toHaveBeenCalled(); + }); + it("captures select values and updates action rows for non-button actions", async () => { enqueueSystemEventMock.mockClear(); const { ctx, app, getHandler } = createContext(); diff --git a/src/slack/monitor/events/interactions.ts b/src/slack/monitor/events/interactions.ts index 40a06ad9f..54a9b0c14 100644 --- a/src/slack/monitor/events/interactions.ts +++ b/src/slack/monitor/events/interactions.ts @@ -527,6 +527,12 @@ function registerModalLifecycleHandler(params: { }) { params.register(params.matcher, async ({ ack, body }: SlackModalEventHandlerArgs) => { await ack(); + if (params.ctx.shouldDropMismatchedSlackEvent?.(body)) { + params.ctx.runtime.log?.( + `slack:interaction drop ${params.interactionType} payload (mismatched app/team)`, + ); + return; + } await emitSlackModalLifecycleEvent({ ctx: params.ctx, body: body as SlackModalBody, @@ -561,6 +567,10 @@ export function registerSlackInteractionEvents(params: { ctx: SlackMonitorContex // Acknowledge the action immediately to prevent the warning icon await ack(); + if (ctx.shouldDropMismatchedSlackEvent?.(body)) { + ctx.runtime.log?.("slack:interaction drop block action payload (mismatched app/team)"); + return; + } // Extract action details using proper Bolt types const typedAction = readInteractionAction(action); diff --git a/src/slack/monitor/slash.test.ts b/src/slack/monitor/slash.test.ts index f6bd45673..527bd2eac 100644 --- a/src/slack/monitor/slash.test.ts +++ b/src/slack/monitor/slash.test.ts @@ -700,6 +700,7 @@ function createPolicyHarness(overrides?: { channelName?: string; allowFrom?: string[]; useAccessGroups?: boolean; + shouldDropMismatchedSlackEvent?: (body: unknown) => boolean; resolveChannelName?: () => Promise<{ name?: string; type?: string }>; }) { const commands = new Map Promise>(); @@ -738,6 +739,8 @@ function createPolicyHarness(overrides?: { textLimit: 4000, app, isChannelAllowed: () => true, + shouldDropMismatchedSlackEvent: (body: unknown) => + overrides?.shouldDropMismatchedSlackEvent?.(body) ?? false, resolveChannelName: overrides?.resolveChannelName ?? (async () => ({ name: channelName, type: "channel" })), resolveUserName: async () => ({ name: "Ada" }), @@ -750,6 +753,7 @@ function createPolicyHarness(overrides?: { async function runSlashHandler(params: { commands: Map Promise>; + body?: unknown; command: Partial<{ user_id: string; user_name: string; @@ -769,6 +773,7 @@ async function runSlashHandler(params: { const ack = vi.fn().mockResolvedValue(undefined); await handler({ + body: params.body, command: { user_id: "U1", user_name: "Ada", @@ -785,6 +790,7 @@ async function runSlashHandler(params: { async function registerAndRunPolicySlash(params: { harness: ReturnType; + body?: unknown; command?: Partial<{ user_id: string; user_name: string; @@ -797,6 +803,7 @@ async function registerAndRunPolicySlash(params: { await registerCommands(params.harness.ctx, params.harness.account); return await runSlashHandler({ commands: params.harness.commands, + body: params.body, command: { channel_id: params.command?.channel_id ?? params.harness.channelId, channel_name: params.command?.channel_name ?? params.harness.channelName, @@ -822,6 +829,23 @@ function expectUnauthorizedResponse(respond: ReturnType) { } describe("slack slash commands channel policy", () => { + it("drops mismatched slash payloads before dispatch", async () => { + const harness = createPolicyHarness({ + shouldDropMismatchedSlackEvent: () => true, + }); + const { respond, ack } = await registerAndRunPolicySlash({ + harness, + body: { + api_app_id: "A_MISMATCH", + team_id: "T_MISMATCH", + }, + }); + + expect(ack).toHaveBeenCalledTimes(1); + expect(dispatchMock).not.toHaveBeenCalled(); + expect(respond).not.toHaveBeenCalled(); + }); + it("allows unlisted channels when groupPolicy is open", async () => { const harness = createPolicyHarness({ groupPolicy: "open", diff --git a/src/slack/monitor/slash.ts b/src/slack/monitor/slash.ts index 292e608d4..104db52ec 100644 --- a/src/slack/monitor/slash.ts +++ b/src/slack/monitor/slash.ts @@ -284,12 +284,20 @@ export async function registerSlackMonitorSlashCommands(params: { command: SlackCommandMiddlewareArgs["command"]; ack: SlackCommandMiddlewareArgs["ack"]; respond: SlackCommandMiddlewareArgs["respond"]; + body?: unknown; prompt: string; commandArgs?: CommandArgs; commandDefinition?: ChatCommandDefinition; }) => { - const { command, ack, respond, prompt, commandArgs, commandDefinition } = p; + const { command, ack, respond, body, prompt, commandArgs, commandDefinition } = p; try { + if (ctx.shouldDropMismatchedSlackEvent?.(body)) { + await ack(); + runtime.log?.( + `slack: drop slash command from user=${command.user_id ?? "unknown"} channel=${command.channel_id ?? "unknown"} (mismatched app/team)`, + ); + return; + } if (!prompt.trim()) { await ack({ text: "Message required.", @@ -667,7 +675,7 @@ export async function registerSlackMonitorSlashCommands(params: { for (const command of nativeCommands) { ctx.app.command( `/${command.name}`, - async ({ command: cmd, ack, respond }: SlackCommandMiddlewareArgs) => { + async ({ command: cmd, ack, respond, body }: SlackCommandMiddlewareArgs) => { const commandDefinition = registry.findCommandByNativeName(command.name, "slack"); const rawText = cmd.text?.trim() ?? ""; const commandArgs = commandDefinition @@ -684,6 +692,7 @@ export async function registerSlackMonitorSlashCommands(params: { command: cmd, ack, respond, + body, prompt, commandArgs, commandDefinition: commandDefinition ?? undefined, @@ -694,11 +703,12 @@ export async function registerSlackMonitorSlashCommands(params: { } else if (slashCommand.enabled) { ctx.app.command( buildSlackSlashCommandMatcher(slashCommand.name), - async ({ command, ack, respond }: SlackCommandMiddlewareArgs) => { + async ({ command, ack, respond, body }: SlackCommandMiddlewareArgs) => { await handleSlashCommand({ command, ack, respond, + body, prompt: command.text?.trim() ?? "", }); }, @@ -725,6 +735,11 @@ export async function registerSlackMonitorSlashCommands(params: { return; } appWithOptions.options(SLACK_COMMAND_ARG_ACTION_ID, async ({ ack, body }) => { + if (ctx.shouldDropMismatchedSlackEvent?.(body)) { + await ack({ options: [] }); + runtime.log?.("slack: drop slash arg options payload (mismatched app/team)"); + return; + } const typedBody = body as { value?: string; user?: { id?: string }; @@ -779,6 +794,10 @@ export async function registerSlackMonitorSlashCommands(params: { const { ack, body, respond } = args; const action = args.action as { value?: string; selected_option?: { value?: string } }; await ack(); + if (ctx.shouldDropMismatchedSlackEvent?.(body)) { + runtime.log?.("slack: drop slash arg action payload (mismatched app/team)"); + return; + } const respondFn = respond ?? (async (payload: { text: string; blocks?: SlackBlock[]; response_type?: string }) => { @@ -836,6 +855,7 @@ export async function registerSlackMonitorSlashCommands(params: { command: commandPayload, ack: async () => {}, respond: respondFn, + body, prompt, commandArgs, commandDefinition: commandDefinition ?? undefined,