From d776fe18f30cf09f1e0bec703b0e6637198b1d07 Mon Sep 17 00:00:00 2001 From: Agustin Rivera <31522568+eleqtrizit@users.noreply.github.com> Date: Mon, 13 Apr 2026 19:38:11 -0700 Subject: [PATCH] fix(slack): align interaction auth with allowlists (#66028) * fix(slack): align interaction auth with allowlists * fix(slack): address review followups * fix(slack): preserve explicit owners with wildcard * chore: append Claude comments resolution worklog * fix(slack): harden interaction auth with default-deny, mandatory actor binding, and channel type validation - Add interactiveEvent flag to authorizeSlackSystemEventSender for stricter interactive control authorization - Default-deny when no allowFrom or channel users are configured for interactive events (block actions, modals) - Require expectedSenderId for all interactive event types; block actions pass Slack-verified userId, modals pass metadata-embedded userId - Reject ambiguous channel types for interactive events to prevent DM authorization bypass via channel-type fallback - Add comprehensive test coverage for all new behaviors * fix(slack): scope interactive owner/allowFrom enforcement to interactive paths only * fix(slack): preserve no-channel interactive default * Update context-engine-maintenance test * chore: remove USER.md worklog artifact Co-Authored-By: Claude Opus 4.6 (1M context) * changelog: note Slack interactive auth allowlist alignment (#66028) --------- Co-authored-by: Claude Opus 4.6 (1M context) Co-authored-by: Devin Robison --- CHANGELOG.md | 1 + extensions/slack/src/monitor/auth.test.ts | 462 +++++++++++++++++- extensions/slack/src/monitor/auth.ts | 97 +++- .../events/interactions.block-actions.ts | 5 + .../src/monitor/events/interactions.modal.ts | 1 + .../src/monitor/events/interactions.test.ts | 180 ++++++- .../context-engine-maintenance.test.ts | 63 +-- 7 files changed, 770 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 889a7fedbc5..9c92e685eba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ Docs: https://docs.openclaw.ai ### Fixes - Models/Codex: include `apiKey` in the codex provider catalog output so the Pi ModelRegistry validator no longer rejects the entry and silently drops all custom models from every provider in `models.json`. (#66180) Thanks @hoyyeva. +- Slack/interactions: apply the configured global `allowFrom` owner allowlist to channel block-action and modal interactive events, require an expected sender id for cross-verification, and reject ambiguous channel types so interactive triggers can no longer bypass the documented allowlist intent in channels without a `users` list. Open-by-default behavior is preserved when no allowlists are configured. (#66028) Thanks @eleqtrizit. ## 2026.4.14-beta.1 diff --git a/extensions/slack/src/monitor/auth.test.ts b/extensions/slack/src/monitor/auth.test.ts index 128ea75aef5..db554cb401b 100644 --- a/extensions/slack/src/monitor/auth.test.ts +++ b/extensions/slack/src/monitor/auth.test.ts @@ -2,6 +2,7 @@ import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; import type { SlackMonitorContext } from "./context.js"; const readStoreAllowFromForDmPolicyMock = vi.hoisted(() => vi.fn()); +let authorizeSlackSystemEventSender: typeof import("./auth.js").authorizeSlackSystemEventSender; let clearSlackAllowFromCacheForTest: typeof import("./auth.js").clearSlackAllowFromCacheForTest; let resolveSlackEffectiveAllowFrom: typeof import("./auth.js").resolveSlackEffectiveAllowFrom; @@ -24,12 +25,42 @@ function makeSlackCtx(allowFrom: string[]): SlackMonitorContext { } as unknown as SlackMonitorContext; } +function makeAuthorizeCtx(params?: { + allowFrom?: string[]; + channelsConfig?: Record; + resolveUserName?: (userId: string) => Promise<{ name?: string }>; + resolveChannelName?: ( + channelId: string, + ) => Promise<{ name?: string; type?: "im" | "mpim" | "channel" | "group" }>; +}) { + return { + allowFrom: params?.allowFrom ?? [], + accountId: "main", + dmPolicy: "open", + dmEnabled: true, + allowNameMatching: false, + channelsConfig: params?.channelsConfig ?? {}, + channelsConfigKeys: Object.keys(params?.channelsConfig ?? {}), + defaultRequireMention: true, + isChannelAllowed: vi.fn(() => true), + resolveUserName: vi.fn( + params?.resolveUserName ?? ((_) => Promise.resolve({ name: undefined })), + ), + resolveChannelName: vi.fn( + params?.resolveChannelName ?? ((_) => Promise.resolve({ name: "general", type: "channel" })), + ), + } as unknown as SlackMonitorContext; +} + describe("resolveSlackEffectiveAllowFrom", () => { const prevTtl = process.env.OPENCLAW_SLACK_PAIRING_ALLOWFROM_CACHE_TTL_MS; beforeAll(async () => { - ({ clearSlackAllowFromCacheForTest, resolveSlackEffectiveAllowFrom } = - await import("./auth.js")); + ({ + authorizeSlackSystemEventSender, + clearSlackAllowFromCacheForTest, + resolveSlackEffectiveAllowFrom, + } = await import("./auth.js")); }); beforeEach(() => { @@ -83,3 +114,430 @@ describe("resolveSlackEffectiveAllowFrom", () => { expect(readStoreAllowFromForDmPolicyMock).toHaveBeenCalledTimes(2); }); }); + +describe("authorizeSlackSystemEventSender", () => { + beforeAll(async () => { + ({ authorizeSlackSystemEventSender, clearSlackAllowFromCacheForTest } = + await import("./auth.js")); + }); + + beforeEach(() => { + clearSlackAllowFromCacheForTest(); + }); + + it("keeps non-interactive channel senders open when only global allowFrom is configured", async () => { + const result = await authorizeSlackSystemEventSender({ + ctx: makeAuthorizeCtx({ allowFrom: ["U_OWNER"] }), + senderId: "U_ATTACKER", + channelId: "C1", + }); + + expect(result).toEqual({ + allowed: true, + channelType: "channel", + channelName: "general", + }); + }); + + it("keeps channel users as the non-interactive gate even when global allowFrom is configured", async () => { + const result = await authorizeSlackSystemEventSender({ + ctx: makeAuthorizeCtx({ + allowFrom: ["U_OWNER"], + channelsConfig: { + C1: { users: ["U_ALLOWED"] }, + }, + }), + senderId: "U_OWNER", + channelId: "C1", + }); + + expect(result).toEqual({ + allowed: false, + reason: "sender-not-channel-allowed", + channelType: "channel", + channelName: "general", + }); + }); + + it("uses the channel denial reason for non-interactive senders who miss a channel users allowlist", async () => { + const result = await authorizeSlackSystemEventSender({ + ctx: makeAuthorizeCtx({ + allowFrom: ["U_OWNER"], + channelsConfig: { + C1: { users: ["U_ALLOWED"] }, + }, + }), + senderId: "U_ATTACKER", + channelId: "C1", + }); + + expect(result).toEqual({ + allowed: false, + reason: "sender-not-channel-allowed", + channelType: "channel", + channelName: "general", + }); + }); + + it("allows channel senders authorized by channel users even when not in global allowFrom", async () => { + const result = await authorizeSlackSystemEventSender({ + ctx: makeAuthorizeCtx({ + allowFrom: ["U_OWNER"], + channelsConfig: { + C1: { users: ["U_ALLOWED"] }, + }, + }), + senderId: "U_ALLOWED", + channelId: "C1", + }); + + expect(result).toEqual({ + allowed: true, + channelType: "channel", + channelName: "general", + }); + }); + + it("keeps channel interactions open when no global or channel allowlists are configured", async () => { + const result = await authorizeSlackSystemEventSender({ + ctx: makeAuthorizeCtx(), + senderId: "U_ANYONE", + channelId: "C1", + }); + + expect(result).toEqual({ + allowed: true, + channelType: "channel", + channelName: "general", + }); + }); + + it("does not let a wildcard global allowFrom bypass non-interactive channel users restrictions", async () => { + const result = await authorizeSlackSystemEventSender({ + ctx: makeAuthorizeCtx({ + allowFrom: ["*"], + channelsConfig: { + C1: { users: ["U_ALLOWED"] }, + }, + }), + senderId: "U_ATTACKER", + channelId: "C1", + }); + + expect(result).toEqual({ + allowed: false, + reason: "sender-not-channel-allowed", + channelType: "channel", + channelName: "general", + }); + }); + + it("still allows a channel user when the global allowFrom is wildcard", async () => { + const result = await authorizeSlackSystemEventSender({ + ctx: makeAuthorizeCtx({ + allowFrom: ["*"], + channelsConfig: { + C1: { users: ["U_ALLOWED"] }, + }, + }), + senderId: "U_ALLOWED", + channelId: "C1", + }); + + expect(result).toEqual({ + allowed: true, + channelType: "channel", + channelName: "general", + }); + }); + + it("does not give non-interactive owner bypass when channel users are configured, even if explicit owners are also listed", async () => { + const result = await authorizeSlackSystemEventSender({ + ctx: makeAuthorizeCtx({ + allowFrom: ["U_OWNER", "*"], + channelsConfig: { + C1: { users: ["U_ALLOWED"] }, + }, + }), + senderId: "U_ATTACKER", + channelId: "C1", + }); + + expect(result).toEqual({ + allowed: false, + reason: "sender-not-channel-allowed", + channelType: "channel", + channelName: "general", + }); + }); + + it("keeps explicit owners behind the non-interactive channel users gate when allowFrom also contains wildcard", async () => { + const result = await authorizeSlackSystemEventSender({ + ctx: makeAuthorizeCtx({ + allowFrom: ["U_OWNER", "*"], + channelsConfig: { + C1: { users: ["U_ALLOWED"] }, + }, + }), + senderId: "U_OWNER", + channelId: "C1", + }); + + expect(result).toEqual({ + allowed: false, + reason: "sender-not-channel-allowed", + channelType: "channel", + channelName: "general", + }); + }); + + it("allows senders without channel context when no allowFrom is configured", async () => { + const result = await authorizeSlackSystemEventSender({ + ctx: makeAuthorizeCtx(), + senderId: "U_ANYONE", + }); + + expect(result).toEqual({ + allowed: true, + channelType: "channel", + channelName: undefined, + }); + }); +}); + +describe("authorizeSlackSystemEventSender interactiveEvent", () => { + beforeAll(async () => { + ({ authorizeSlackSystemEventSender, clearSlackAllowFromCacheForTest } = + await import("./auth.js")); + }); + + beforeEach(() => { + clearSlackAllowFromCacheForTest(); + }); + + it("rejects interactive events when expectedSenderId is not provided", async () => { + const result = await authorizeSlackSystemEventSender({ + ctx: makeAuthorizeCtx({ allowFrom: ["U_OWNER"] }), + senderId: "U_OWNER", + channelId: "C1", + interactiveEvent: true, + }); + + expect(result).toEqual({ + allowed: false, + reason: "missing-expected-sender", + }); + }); + + it("allows interactive events when expectedSenderId matches senderId", async () => { + const result = await authorizeSlackSystemEventSender({ + ctx: makeAuthorizeCtx({ allowFrom: ["U_OWNER"] }), + senderId: "U_OWNER", + channelId: "C1", + expectedSenderId: "U_OWNER", + interactiveEvent: true, + }); + + expect(result).toEqual({ + allowed: true, + channelType: "channel", + channelName: "general", + }); + }); + + it("allows interactive channel senders who match the global allowFrom even when channel users are configured", async () => { + const result = await authorizeSlackSystemEventSender({ + ctx: makeAuthorizeCtx({ + allowFrom: ["U_OWNER"], + channelsConfig: { + C1: { users: ["U_ALLOWED"] }, + }, + }), + senderId: "U_OWNER", + channelId: "C1", + expectedSenderId: "U_OWNER", + interactiveEvent: true, + }); + + expect(result).toEqual({ + allowed: true, + channelType: "channel", + channelName: "general", + }); + }); + + it("uses a combined denial reason when an interactive sender matches neither global nor channel allowlists", async () => { + const result = await authorizeSlackSystemEventSender({ + ctx: makeAuthorizeCtx({ + allowFrom: ["U_OWNER"], + channelsConfig: { + C1: { users: ["U_ALLOWED"] }, + }, + }), + senderId: "U_ATTACKER", + channelId: "C1", + expectedSenderId: "U_ATTACKER", + interactiveEvent: true, + }); + + expect(result).toEqual({ + allowed: false, + reason: "sender-not-authorized", + channelType: "channel", + channelName: "general", + }); + }); + + it("keeps interactive channel events open when no allowlists are configured", async () => { + const result = await authorizeSlackSystemEventSender({ + ctx: makeAuthorizeCtx(), + senderId: "U_ANYONE", + channelId: "C1", + expectedSenderId: "U_ANYONE", + interactiveEvent: true, + }); + + expect(result).toEqual({ + allowed: true, + channelType: "channel", + channelName: "general", + }); + }); + + it("preserves explicit owner access for interactive events when allowFrom also contains wildcard", async () => { + const result = await authorizeSlackSystemEventSender({ + ctx: makeAuthorizeCtx({ + allowFrom: ["U_OWNER", "*"], + channelsConfig: { + C1: { users: ["U_ALLOWED"] }, + }, + }), + senderId: "U_OWNER", + channelId: "C1", + expectedSenderId: "U_OWNER", + interactiveEvent: true, + }); + + expect(result).toEqual({ + allowed: true, + channelType: "channel", + channelName: "general", + }); + }); + + it("keeps interactive no-channel events open when no allowFrom is configured", async () => { + const result = await authorizeSlackSystemEventSender({ + ctx: makeAuthorizeCtx(), + senderId: "U_ANYONE", + expectedSenderId: "U_ANYONE", + interactiveEvent: true, + }); + + expect(result).toEqual({ + allowed: true, + channelType: "channel", + channelName: undefined, + }); + }); + + it("denies interactive no-channel events when sender is not in allowFrom", async () => { + const result = await authorizeSlackSystemEventSender({ + ctx: makeAuthorizeCtx({ allowFrom: ["U_OWNER"] }), + senderId: "U_ATTACKER", + expectedSenderId: "U_ATTACKER", + interactiveEvent: true, + }); + + expect(result).toEqual({ + allowed: false, + reason: "sender-not-allowlisted", + }); + }); + + it("allows interactive no-channel events when sender is in allowFrom", async () => { + const result = await authorizeSlackSystemEventSender({ + ctx: makeAuthorizeCtx({ allowFrom: ["U_OWNER"] }), + senderId: "U_OWNER", + expectedSenderId: "U_OWNER", + interactiveEvent: true, + }); + + expect(result).toEqual({ + allowed: true, + channelType: "channel", + channelName: undefined, + }); + }); + + it("rejects interactive events with ambiguous channel type", async () => { + // Channel ID "X1" has no recognized prefix (D, C, G) so the type is ambiguous + const result = await authorizeSlackSystemEventSender({ + ctx: makeAuthorizeCtx({ + allowFrom: ["U_OWNER"], + resolveChannelName: () => Promise.resolve({ name: "mystery" }), + }), + senderId: "U_OWNER", + channelId: "X1", + expectedSenderId: "U_OWNER", + interactiveEvent: true, + }); + + expect(result).toEqual({ + allowed: false, + reason: "ambiguous-channel-type", + channelType: "channel", + channelName: "mystery", + }); + }); + + it("allows interactive events when channel type is known from ID prefix", async () => { + const result = await authorizeSlackSystemEventSender({ + ctx: makeAuthorizeCtx({ allowFrom: ["U_OWNER"] }), + senderId: "U_OWNER", + channelId: "C1", + expectedSenderId: "U_OWNER", + interactiveEvent: true, + }); + + expect(result).toEqual({ + allowed: true, + channelType: "channel", + channelName: "general", + }); + }); + + it("allows interactive events when channel type is known from explicit type", async () => { + const result = await authorizeSlackSystemEventSender({ + ctx: makeAuthorizeCtx({ + allowFrom: ["U_OWNER"], + resolveChannelName: () => Promise.resolve({ name: "mystery", type: "group" }), + }), + senderId: "U_OWNER", + channelId: "X1", + channelType: "group", + expectedSenderId: "U_OWNER", + interactiveEvent: true, + }); + + expect(result).toEqual({ + allowed: true, + channelType: "group", + channelName: "mystery", + }); + }); + + it("does not apply interactiveEvent restrictions to non-interactive events", async () => { + // Same scenario as the denying test above, but without interactiveEvent flag + const result = await authorizeSlackSystemEventSender({ + ctx: makeAuthorizeCtx(), + senderId: "U_ANYONE", + channelId: "C1", + }); + + expect(result).toEqual({ + allowed: true, + channelType: "channel", + channelName: "general", + }); + }); +}); diff --git a/extensions/slack/src/monitor/auth.ts b/extensions/slack/src/monitor/auth.ts index df8946a01c0..5040e2e7057 100644 --- a/extensions/slack/src/monitor/auth.ts +++ b/extensions/slack/src/monitor/auth.ts @@ -3,9 +3,11 @@ import { allowListMatches, normalizeAllowList, normalizeAllowListLower, + resolveSlackAllowListMatch, resolveSlackUserAllowed, } from "./allow-list.js"; import { resolveSlackChannelConfig } from "./channel-config.js"; +import { inferSlackChannelType } from "./channel-type.js"; import { normalizeSlackChannelType, type SlackMonitorContext } from "./context.js"; type ResolvedAllowFromLists = { @@ -153,11 +155,14 @@ export type SlackSystemEventAuthResult = { allowed: boolean; reason?: | "missing-sender" + | "missing-expected-sender" | "sender-mismatch" | "channel-not-allowed" + | "ambiguous-channel-type" | "dm-disabled" | "sender-not-allowlisted" - | "sender-not-channel-allowed"; + | "sender-not-channel-allowed" + | "sender-not-authorized"; channelType?: "im" | "mpim" | "channel" | "group"; channelName?: string; }; @@ -168,6 +173,10 @@ export async function authorizeSlackSystemEventSender(params: { channelId?: string; channelType?: string | null; expectedSenderId?: string; + /** When true, requires expectedSenderId, rejects ambiguous channel types, + * and applies interactive-only owner allowFrom checks without changing the + * open-by-default channel behavior when no allowlists are configured. */ + interactiveEvent?: boolean; }): Promise { const senderId = params.senderId?.trim(); if (!senderId) { @@ -179,6 +188,11 @@ export async function authorizeSlackSystemEventSender(params: { return { allowed: false, reason: "sender-mismatch" }; } + // Interactive events require an expected sender to cross-verify the actor. + if (params.interactiveEvent && !expectedSenderId) { + return { allowed: false, reason: "missing-expected-sender" }; + } + const channelId = params.channelId?.trim(); let channelType = normalizeSlackChannelType(params.channelType, channelId); let channelName: string | undefined; @@ -188,7 +202,8 @@ export async function authorizeSlackSystemEventSender(params: { type?: "im" | "mpim" | "channel" | "group"; } = await params.ctx.resolveChannelName(channelId).catch(() => ({})); channelName = info.name; - channelType = normalizeSlackChannelType(params.channelType ?? info.type, channelId); + const resolvedTypeSource = params.channelType ?? info.type; + channelType = normalizeSlackChannelType(resolvedTypeSource, channelId); if ( !params.ctx.isChannelAllowed({ channelId, @@ -203,6 +218,31 @@ export async function authorizeSlackSystemEventSender(params: { channelName, }; } + + // For interactive events, reject when channel type could not be positively + // determined from either the explicit type or the channel ID prefix. This + // prevents a DM from being misclassified as "channel" and skipping + // DM-specific authorization. + if (params.interactiveEvent) { + const inferredFromId = inferSlackChannelType(channelId); + const sourceNormalized = + typeof resolvedTypeSource === "string" + ? resolvedTypeSource.toLowerCase().trim() + : undefined; + const sourceIsKnownType = + sourceNormalized === "im" || + sourceNormalized === "mpim" || + sourceNormalized === "channel" || + sourceNormalized === "group"; + if (inferredFromId === undefined && !sourceIsKnownType) { + return { + allowed: false, + reason: "ambiguous-channel-type", + channelType, + channelName, + }; + } + } } const senderInfo: { name?: string } = await params.ctx @@ -235,8 +275,8 @@ export async function authorizeSlackSystemEventSender(params: { } } } else if (!channelId) { - // No channel context. Apply allowFrom if configured so we fail closed - // for privileged interactive events when owner allowlist is present. + // No channel context. Preserve the existing open default unless a global + // allowFrom list is configured. const allowFromLower = await resolveAllowFromLower(false); if (allowFromLower.length > 0) { const senderAllowListed = isSlackSenderAllowListed({ @@ -250,6 +290,9 @@ export async function authorizeSlackSystemEventSender(params: { } } } else { + const allowFromLower = await resolveAllowFromLower(false); + const ownerAllowlistConfigured = allowFromLower.length > 0; + const allowFromLowerWithoutWildcard = allowFromLower.filter((entry) => entry !== "*"); const channelConfig = resolveSlackChannelConfig({ channelId, channelName, @@ -260,6 +303,23 @@ export async function authorizeSlackSystemEventSender(params: { }); const channelUsersAllowlistConfigured = Array.isArray(channelConfig?.users) && channelConfig.users.length > 0; + const ownerMatch = ownerAllowlistConfigured + ? resolveSlackAllowListMatch({ + allowList: allowFromLower, + id: senderId, + name: senderName, + allowNameMatching: params.ctx.allowNameMatching, + }) + : { allowed: false }; + const ownerAllowed = ownerMatch.allowed; + const ownerExplicitlyAllowed = + allowFromLowerWithoutWildcard.length > 0 && + resolveSlackAllowListMatch({ + allowList: allowFromLowerWithoutWildcard, + id: senderId, + name: senderName, + allowNameMatching: params.ctx.allowNameMatching, + }).allowed; if (channelUsersAllowlistConfigured) { const channelUserAllowed = resolveSlackUserAllowed({ allowList: channelConfig?.users, @@ -267,14 +327,37 @@ export async function authorizeSlackSystemEventSender(params: { userName: senderName, allowNameMatching: params.ctx.allowNameMatching, }); - if (!channelUserAllowed) { + if (channelUserAllowed || (params.interactiveEvent && ownerExplicitlyAllowed)) { return { - allowed: false, - reason: "sender-not-channel-allowed", + allowed: true, channelType, channelName, }; } + return { + allowed: false, + reason: + params.interactiveEvent && ownerAllowlistConfigured + ? "sender-not-authorized" + : "sender-not-channel-allowed", + channelType, + channelName, + }; + } + if (params.interactiveEvent && ownerAllowed) { + return { + allowed: true, + channelType, + channelName, + }; + } + if (params.interactiveEvent && ownerAllowlistConfigured) { + return { + allowed: false, + reason: "sender-not-allowlisted", + channelType, + channelName, + }; } } diff --git a/extensions/slack/src/monitor/events/interactions.block-actions.ts b/extensions/slack/src/monitor/events/interactions.block-actions.ts index c272ba666f8..c337c42ffaf 100644 --- a/extensions/slack/src/monitor/events/interactions.block-actions.ts +++ b/extensions/slack/src/monitor/events/interactions.block-actions.ts @@ -472,6 +472,11 @@ async function authorizeSlackBlockAction(params: { ctx: params.ctx, senderId: params.parsed.userId, channelId: params.parsed.channelId, + // Block action sender identity is verified by Slack's request signing. + // Pass the Slack-verified userId as expectedSenderId to satisfy the + // mandatory actor-binding requirement for interactive events. + expectedSenderId: params.parsed.userId, + interactiveEvent: true, }); if (auth.allowed) { return auth; diff --git a/extensions/slack/src/monitor/events/interactions.modal.ts b/extensions/slack/src/monitor/events/interactions.modal.ts index 14f7a0af0cd..9e579be94b9 100644 --- a/extensions/slack/src/monitor/events/interactions.modal.ts +++ b/extensions/slack/src/monitor/events/interactions.modal.ts @@ -219,6 +219,7 @@ export async function emitSlackModalLifecycleEvent(params: { channelId: sessionRouting.channelId, channelType: sessionRouting.channelType, expectedSenderId: expectedUserId, + interactiveEvent: true, }); if (!auth.allowed) { params.ctx.runtime.log?.( diff --git a/extensions/slack/src/monitor/events/interactions.test.ts b/extensions/slack/src/monitor/events/interactions.test.ts index 10ad11f0316..add1c41fa81 100644 --- a/extensions/slack/src/monitor/events/interactions.test.ts +++ b/extensions/slack/src/monitor/events/interactions.test.ts @@ -138,9 +138,10 @@ function createContext(overrides?: { runtime: { log: runtimeLog }, dmEnabled: overrides?.dmEnabled ?? true, dmPolicy: overrides?.dmPolicy ?? ("open" as const), - allowFrom: overrides?.allowFrom ?? [], + allowFrom: overrides?.allowFrom ?? ["*"], allowNameMatching: overrides?.allowNameMatching ?? false, channelsConfig: overrides?.channelsConfig ?? {}, + channelsConfigKeys: Object.keys(overrides?.channelsConfig ?? {}), defaultRequireMention: true, shouldDropMismatchedSlackEvent: (body: unknown) => overrides?.shouldDropMismatchedSlackEvent?.(body) ?? false, @@ -771,6 +772,156 @@ describe("registerSlackInteractionEvents", () => { }); }); + it("blocks channel block actions when sender is outside configured global allowFrom", async () => { + enqueueSystemEventMock.mockClear(); + const { ctx, app, getHandler } = createContext({ + allowFrom: ["U_OWNER"], + }); + 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: "U_ATTACKER" }, + channel: { id: "C1" }, + message: { + ts: "250.251", + blocks: [{ type: "actions", block_id: "verify_block", elements: [] }], + }, + }, + action: { + type: "button", + action_id: "openclaw:verify", + block_id: "verify_block", + }, + }); + + expect(ack).toHaveBeenCalled(); + expect(enqueueSystemEventMock).not.toHaveBeenCalled(); + expect(app.client.chat.update).not.toHaveBeenCalled(); + expect(respond).toHaveBeenCalledWith({ + text: "You are not authorized to use this control.", + response_type: "ephemeral", + }); + }); + + it("allows channel block actions when channel users allowlist authorizes the sender", async () => { + enqueueSystemEventMock.mockClear(); + const { ctx, app, getHandler } = createContext({ + allowFrom: ["U_OWNER"], + channelsConfig: { + C1: { users: ["U_ALLOWED"] }, + }, + }); + 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: "U_ALLOWED" }, + channel: { id: "C1" }, + message: { + ts: "260.261", + blocks: [{ type: "actions", block_id: "verify_block", elements: [] }], + }, + }, + action: { + type: "button", + action_id: "openclaw:verify", + block_id: "verify_block", + }, + }); + + expect(ack).toHaveBeenCalled(); + expect(enqueueSystemEventMock).toHaveBeenCalledTimes(1); + expect(app.client.chat.update).toHaveBeenCalledTimes(1); + expect(respond).not.toHaveBeenCalled(); + }); + + it("blocks wildcard global allowFrom from bypassing configured channel users", async () => { + enqueueSystemEventMock.mockClear(); + const { ctx, app, getHandler } = createContext({ + allowFrom: ["*"], + channelsConfig: { + C1: { users: ["U_ALLOWED"] }, + }, + }); + 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: "U_ATTACKER" }, + channel: { id: "C1" }, + message: { + ts: "270.271", + blocks: [{ type: "actions", block_id: "verify_block", elements: [] }], + }, + }, + action: { + type: "button", + action_id: "openclaw:verify", + block_id: "verify_block", + }, + }); + + expect(ack).toHaveBeenCalled(); + expect(enqueueSystemEventMock).not.toHaveBeenCalled(); + expect(app.client.chat.update).not.toHaveBeenCalled(); + expect(respond).toHaveBeenCalledWith({ + text: "You are not authorized to use this control.", + response_type: "ephemeral", + }); + }); + + it("keeps channel block actions open when no allowlists are configured", async () => { + enqueueSystemEventMock.mockClear(); + const { ctx, app, getHandler } = createContext({ allowFrom: [] }); + 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: "U_ANYONE" }, + channel: { id: "C1" }, + message: { + ts: "305.306", + blocks: [{ type: "actions", block_id: "verify_block", elements: [] }], + }, + }, + action: { + type: "button", + action_id: "openclaw:verify", + block_id: "verify_block", + }, + }); + + expect(ack).toHaveBeenCalled(); + expect(enqueueSystemEventMock).toHaveBeenCalledTimes(1); + expect(app.client.chat.update).toHaveBeenCalledTimes(1); + expect(respond).not.toHaveBeenCalled(); + }); + it("blocks DM block actions when sender is not in allowFrom", async () => { enqueueSystemEventMock.mockClear(); const { ctx, app, getHandler } = createContext({ @@ -1412,6 +1563,33 @@ describe("registerSlackInteractionEvents", () => { expect(enqueueSystemEventMock).not.toHaveBeenCalled(); }); + it("keeps no-channel modal events open when allowFrom is unset", async () => { + enqueueSystemEventMock.mockClear(); + const { ctx, getViewHandler } = createContext({ allowFrom: [] }); + registerSlackInteractionEvents({ ctx: ctx as never }); + const viewHandler = getViewHandler(); + expect(viewHandler).toBeTruthy(); + + const ack = vi.fn().mockResolvedValue(undefined); + await viewHandler!({ + ack, + body: { + user: { id: "U444" }, + view: { + id: "V444", + callback_id: "openclaw:routing_form", + private_metadata: JSON.stringify({ userId: "U444" }), + state: { + values: {}, + }, + }, + }, + } as never); + + expect(ack).toHaveBeenCalled(); + expect(enqueueSystemEventMock).toHaveBeenCalledTimes(1); + }); + it("captures modal input labels and picker values across block types", async () => { enqueueSystemEventMock.mockClear(); const { ctx, getViewHandler } = createContext(); diff --git a/src/agents/pi-embedded-runner/context-engine-maintenance.test.ts b/src/agents/pi-embedded-runner/context-engine-maintenance.test.ts index addd583e7ef..b396ca8f51d 100644 --- a/src/agents/pi-embedded-runner/context-engine-maintenance.test.ts +++ b/src/agents/pi-embedded-runner/context-engine-maintenance.test.ts @@ -171,14 +171,16 @@ describe("buildContextEngineMaintenanceRuntimeContext", () => { }); await Promise.resolve(); - rewriteTranscriptEntriesInSessionFileMock.mockImplementationOnce(async (_params?: unknown) => { - events.push("rewrite"); - return { - changed: true, - bytesFreed: 123, - rewrittenEntries: 2, - }; - }); + rewriteTranscriptEntriesInSessionFileMock.mockImplementationOnce( + async (_params?: unknown) => { + events.push("rewrite"); + return { + changed: true, + bytesFreed: 123, + rewrittenEntries: 2, + }; + }, + ); const runtimeContext = buildContextEngineMaintenanceRuntimeContext({ sessionId: "session-rewrite-handoff", @@ -836,19 +838,20 @@ describe("runContextEngineMaintenance", () => { allowRewrite = resolve; }); events.push("maintenance-before-rewrite"); - await (params as { runtimeContext?: ContextEngineRuntimeContext }).runtimeContext - ?.rewriteTranscriptEntries?.({ - replacements: [ - { - entryId: "entry-1", - message: castAgentMessage({ - role: "assistant", - content: [{ type: "text", text: "done" }], - timestamp: 2, - }), - }, - ], - }); + await ( + params as { runtimeContext?: ContextEngineRuntimeContext } + ).runtimeContext?.rewriteTranscriptEntries?.({ + replacements: [ + { + entryId: "entry-1", + message: castAgentMessage({ + role: "assistant", + content: [{ type: "text", text: "done" }], + timestamp: 2, + }), + }, + ], + }); events.push("maintenance-after-rewrite"); return { changed: false, @@ -857,14 +860,16 @@ describe("runContextEngineMaintenance", () => { }; }); - rewriteTranscriptEntriesInSessionFileMock.mockImplementationOnce(async (_params?: unknown) => { - events.push("rewrite"); - return { - changed: true, - bytesFreed: 123, - rewrittenEntries: 2, - }; - }); + rewriteTranscriptEntriesInSessionFileMock.mockImplementationOnce( + async (_params?: unknown) => { + events.push("rewrite"); + return { + changed: true, + bytesFreed: 123, + rewrittenEntries: 2, + }; + }, + ); const backgroundEngine = { info: {