mirror of
https://fastgit.cc/github.com/openclaw/openclaw
synced 2026-04-30 22:12:32 +08:00
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) <noreply@anthropic.com> * changelog: note Slack interactive auth allowlist alignment (#66028) --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Devin Robison <drobison@nvidia.com>
This commit is contained in:
Agustin Rivera
committed by
Peter Steinberger
Peter Steinberger
parent
4dcfcbceee
commit
d776fe18f3
@@ -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
|
||||
|
||||
|
||||
@@ -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<string, { users?: string[] }>;
|
||||
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",
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<SlackSystemEventAuthResult> {
|
||||
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,
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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?.(
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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: {
|
||||
|
||||
Reference in New Issue
Block a user