From f19eabee54c49e9a2e264b4965edf28a2f92e657 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 14 Feb 2026 19:07:44 +0100 Subject: [PATCH] fix(slack): gate DM slash command authorization --- CHANGELOG.md | 1 + src/slack/monitor/slash.policy.test.ts | 39 ++++++++++++++++++++++++++ src/slack/monitor/slash.ts | 13 +++++++-- 3 files changed, 51 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 99a7d3cf19..0959984824 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ Docs: https://docs.openclaw.ai - Security/Google Chat: deprecate `users/` allowlists (treat `users/...` as immutable user id only); keep raw email allowlists for usability. Thanks @vincentkoc. - Security/Google Chat: reject ambiguous shared-path webhook routing when multiple webhook targets verify successfully (prevents cross-account policy-context misrouting). Thanks @vincentkoc. - Security/Browser: block cross-origin mutating requests to loopback browser control routes (CSRF hardening). Thanks @vincentkoc. +- Security/Slack: compute command authorization for DM slash commands even when `dmPolicy=open`, preventing unauthorized users from running privileged commands via DM. Thanks @christos-eth. - Security/Nostr: require loopback source and block cross-origin profile mutation/import attempts. Thanks @vincentkoc. - Security/Archive: enforce archive extraction entry/size limits to prevent resource exhaustion from high-expansion ZIP/TAR archives. Thanks @vincentkoc. - Security/Media: reject oversized base64-backed input media before decoding to avoid large allocations. Thanks @vincentkoc. diff --git a/src/slack/monitor/slash.policy.test.ts b/src/slack/monitor/slash.policy.test.ts index 3b03b27ce9..24ba9358b8 100644 --- a/src/slack/monitor/slash.policy.test.ts +++ b/src/slack/monitor/slash.policy.test.ts @@ -267,6 +267,45 @@ describe("slack slash commands access groups", () => { expect(respond).not.toHaveBeenCalledWith( expect.objectContaining({ text: "You are not authorized to use this command." }), ); + const dispatchArg = dispatchMock.mock.calls[0]?.[0] as { + ctx?: { CommandAuthorized?: boolean }; + }; + expect(dispatchArg?.ctx?.CommandAuthorized).toBe(false); + }); + + it("computes CommandAuthorized for DM slash commands when dmPolicy is open", async () => { + const { commands, ctx, account } = createHarness({ + allowFrom: ["U_OWNER"], + channelId: "D999", + channelName: "directmessage", + resolveChannelName: async () => ({ name: "directmessage", type: "im" }), + }); + registerSlackMonitorSlashCommands({ ctx: ctx as never, account: account as never }); + + const handler = [...commands.values()][0]; + if (!handler) { + throw new Error("Missing slash handler"); + } + + const respond = vi.fn().mockResolvedValue(undefined); + await handler({ + command: { + user_id: "U_ATTACKER", + user_name: "Mallory", + channel_id: "D999", + channel_name: "directmessage", + text: "hello", + trigger_id: "t1", + }, + ack: vi.fn().mockResolvedValue(undefined), + respond, + }); + + expect(dispatchMock).toHaveBeenCalledTimes(1); + const dispatchArg = dispatchMock.mock.calls[0]?.[0] as { + ctx?: { CommandAuthorized?: boolean }; + }; + expect(dispatchArg?.ctx?.CommandAuthorized).toBe(false); }); it("enforces access-group gating when lookup fails for private channels", async () => { diff --git a/src/slack/monitor/slash.ts b/src/slack/monitor/slash.ts index 2eca0f9c07..3ee2b30833 100644 --- a/src/slack/monitor/slash.ts +++ b/src/slack/monitor/slash.ts @@ -204,7 +204,9 @@ export function registerSlackMonitorSlashCommands(params: { const effectiveAllowFrom = normalizeAllowList([...ctx.allowFrom, ...storeAllowFrom]); const effectiveAllowFromLower = normalizeAllowListLower(effectiveAllowFrom); - let commandAuthorized = true; + // Privileged command surface: compute CommandAuthorized, don't assume true. + // Keep this aligned with the Slack message path (message-handler/prepare.ts). + let commandAuthorized = false; let channelConfig: SlackChannelConfigResolved | null = null; if (isDirectMessage) { if (!ctx.dmEnabled || ctx.dmPolicy === "disabled") { @@ -256,7 +258,6 @@ export function registerSlackMonitorSlashCommands(params: { } return; } - commandAuthorized = true; } } @@ -322,6 +323,13 @@ export function registerSlackMonitorSlashCommands(params: { id: command.user_id, name: senderName, }).allowed; + // DMs: allow chatting in dmPolicy=open, but keep privileged command gating intact by setting + // CommandAuthorized based on allowlists/access-groups (downstream decides which commands need it). + commandAuthorized = resolveCommandAuthorizedFromAuthorizers({ + useAccessGroups: ctx.useAccessGroups, + authorizers: [{ configured: effectiveAllowFromLower.length > 0, allowed: ownerAllowed }], + modeWhenAccessGroupsOff: "configured", + }); if (isRoomish) { commandAuthorized = resolveCommandAuthorizedFromAuthorizers({ useAccessGroups: ctx.useAccessGroups, @@ -329,6 +337,7 @@ export function registerSlackMonitorSlashCommands(params: { { configured: effectiveAllowFromLower.length > 0, allowed: ownerAllowed }, { configured: channelUsersAllowlistConfigured, allowed: channelUserAllowed }, ], + modeWhenAccessGroupsOff: "configured", }); if (ctx.useAccessGroups && !commandAuthorized) { await respond({