From cf04208cb9d02f7f17373f1f1365b0f6cca04b73 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 15 Feb 2026 03:46:11 +0100 Subject: [PATCH] fix(allowlist): canonicalize Slack/Discord allowFrom --- src/auto-reply/reply/commands-allowlist.ts | 34 ++++++-- src/auto-reply/reply/commands-policy.test.ts | 92 +++++++++++++++++++- src/channels/dock.ts | 19 ++-- src/channels/plugins/directory-config.ts | 4 +- src/config/schema.help.ts | 4 +- src/discord/monitor/agent-components.ts | 2 +- src/security/audit-channel.ts | 23 +++-- 7 files changed, 153 insertions(+), 25 deletions(-) diff --git a/src/auto-reply/reply/commands-allowlist.ts b/src/auto-reply/reply/commands-allowlist.ts index a57c739f45d..09a626d9e64 100644 --- a/src/auto-reply/reply/commands-allowlist.ts +++ b/src/auto-reply/reply/commands-allowlist.ts @@ -254,7 +254,8 @@ function resolveChannelAllowFromPaths( } if (scope === "dm") { if (channelId === "slack" || channelId === "discord") { - return ["dm", "allowFrom"]; + // Canonical DM allowlist location for Slack/Discord. Legacy: dm.allowFrom. + return ["allowFrom"]; } if ( channelId === "telegram" || @@ -404,7 +405,7 @@ export const handleAllowlistCommand: CommandHandler = async (params, allowTextCo groupPolicy = account.config.groupPolicy; } else if (channelId === "slack") { const account = resolveSlackAccount({ cfg: params.cfg, accountId }); - dmAllowFrom = (account.dm?.allowFrom ?? []).map(String); + dmAllowFrom = (account.config.allowFrom ?? account.config.dm?.allowFrom ?? []).map(String); groupPolicy = account.groupPolicy; const channels = account.channels ?? {}; groupOverrides = Object.entries(channels) @@ -415,7 +416,7 @@ export const handleAllowlistCommand: CommandHandler = async (params, allowTextCo .filter(Boolean) as Array<{ label: string; entries: string[] }>; } else if (channelId === "discord") { const account = resolveDiscordAccount({ cfg: params.cfg, accountId }); - dmAllowFrom = (account.config.dm?.allowFrom ?? []).map(String); + dmAllowFrom = (account.config.allowFrom ?? account.config.dm?.allowFrom ?? []).map(String); groupPolicy = account.config.groupPolicy; const guilds = account.config.guilds ?? {}; for (const [guildKey, guildCfg] of Object.entries(guilds)) { @@ -567,10 +568,25 @@ export const handleAllowlistCommand: CommandHandler = async (params, allowTextCo pathPrefix, accountId: normalizedAccountId, } = resolveAccountTarget(parsedConfig, channelId, accountId); - const existingRaw = getNestedValue(target, allowlistPath); - const existing = Array.isArray(existingRaw) - ? existingRaw.map((entry) => String(entry).trim()).filter(Boolean) - : []; + const existing: string[] = []; + const existingPaths = + scope === "dm" && (channelId === "slack" || channelId === "discord") + ? // Read both while legacy alias may still exist; write canonical below. + [allowlistPath, ["dm", "allowFrom"]] + : [allowlistPath]; + for (const path of existingPaths) { + const existingRaw = getNestedValue(target, path); + if (!Array.isArray(existingRaw)) { + continue; + } + for (const entry of existingRaw) { + const value = String(entry).trim(); + if (!value || existing.includes(value)) { + continue; + } + existing.push(value); + } + } const normalizedEntry = normalizeAllowFrom({ cfg: params.cfg, @@ -628,6 +644,10 @@ export const handleAllowlistCommand: CommandHandler = async (params, allowTextCo } else { setNestedValue(target, allowlistPath, next); } + if (scope === "dm" && (channelId === "slack" || channelId === "discord")) { + // Remove legacy DM allowlist alias to prevent drift. + deleteNestedValue(target, ["dm", "allowFrom"]); + } } if (configChanged) { diff --git a/src/auto-reply/reply/commands-policy.test.ts b/src/auto-reply/reply/commands-policy.test.ts index aa747b24cc3..c93b818e25f 100644 --- a/src/auto-reply/reply/commands-policy.test.ts +++ b/src/auto-reply/reply/commands-policy.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, vi } from "vitest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; import type { OpenClawConfig } from "../../config/config.js"; import type { MsgContext } from "../templating.js"; import { buildCommandContext, handleCommands } from "./commands.js"; @@ -94,6 +94,10 @@ function buildParams(commandBody: string, cfg: OpenClawConfig, ctxOverrides?: Pa } describe("handleCommands /allowlist", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + it("lists config + store allowFrom entries", async () => { readChannelAllowFromStoreMock.mockResolvedValueOnce(["456"]); @@ -145,6 +149,92 @@ describe("handleCommands /allowlist", () => { }); expect(result.reply?.text).toContain("DM allowlist added"); }); + + it("removes Slack DM allowlist entries from canonical allowFrom and deletes legacy dm.allowFrom", async () => { + readConfigFileSnapshotMock.mockResolvedValueOnce({ + valid: true, + parsed: { + channels: { + slack: { + allowFrom: ["U111", "U222"], + dm: { allowFrom: ["U111", "U222"] }, + configWrites: true, + }, + }, + }, + }); + validateConfigObjectWithPluginsMock.mockImplementation((config: unknown) => ({ + ok: true, + config, + })); + + const cfg = { + commands: { text: true, config: true }, + channels: { + slack: { + allowFrom: ["U111", "U222"], + dm: { allowFrom: ["U111", "U222"] }, + configWrites: true, + }, + }, + } as OpenClawConfig; + + const params = buildParams("/allowlist remove dm U111", cfg, { + Provider: "slack", + Surface: "slack", + }); + const result = await handleCommands(params); + + expect(result.shouldContinue).toBe(false); + expect(writeConfigFileMock).toHaveBeenCalledTimes(1); + const written = writeConfigFileMock.mock.calls[0]?.[0] as OpenClawConfig; + expect(written.channels?.slack?.allowFrom).toEqual(["U222"]); + expect(written.channels?.slack?.dm?.allowFrom).toBeUndefined(); + expect(result.reply?.text).toContain("channels.slack.allowFrom"); + }); + + it("removes Discord DM allowlist entries from canonical allowFrom and deletes legacy dm.allowFrom", async () => { + readConfigFileSnapshotMock.mockResolvedValueOnce({ + valid: true, + parsed: { + channels: { + discord: { + allowFrom: ["111", "222"], + dm: { allowFrom: ["111", "222"] }, + configWrites: true, + }, + }, + }, + }); + validateConfigObjectWithPluginsMock.mockImplementation((config: unknown) => ({ + ok: true, + config, + })); + + const cfg = { + commands: { text: true, config: true }, + channels: { + discord: { + allowFrom: ["111", "222"], + dm: { allowFrom: ["111", "222"] }, + configWrites: true, + }, + }, + } as OpenClawConfig; + + const params = buildParams("/allowlist remove dm 111", cfg, { + Provider: "discord", + Surface: "discord", + }); + const result = await handleCommands(params); + + expect(result.shouldContinue).toBe(false); + expect(writeConfigFileMock).toHaveBeenCalledTimes(1); + const written = writeConfigFileMock.mock.calls[0]?.[0] as OpenClawConfig; + expect(written.channels?.discord?.allowFrom).toEqual(["222"]); + expect(written.channels?.discord?.dm?.allowFrom).toBeUndefined(); + expect(result.reply?.text).toContain("channels.discord.allowFrom"); + }); }); describe("/models command", () => { diff --git a/src/channels/dock.ts b/src/channels/dock.ts index e7a5b7e0f13..e0aec226964 100644 --- a/src/channels/dock.ts +++ b/src/channels/dock.ts @@ -191,13 +191,16 @@ const DOCKS: Record = { blockStreamingCoalesceDefaults: { minChars: 1500, idleMs: 1000 }, }, elevated: { - allowFromFallback: ({ cfg }) => cfg.channels?.discord?.dm?.allowFrom, + allowFromFallback: ({ cfg }) => + cfg.channels?.discord?.allowFrom ?? cfg.channels?.discord?.dm?.allowFrom, }, config: { - resolveAllowFrom: ({ cfg, accountId }) => - (resolveDiscordAccount({ cfg, accountId }).config.dm?.allowFrom ?? []).map((entry) => + resolveAllowFrom: ({ cfg, accountId }) => { + const account = resolveDiscordAccount({ cfg, accountId }); + return (account.config.allowFrom ?? account.config.dm?.allowFrom ?? []).map((entry) => String(entry), - ), + ); + }, formatAllowFrom: ({ allowFrom }) => formatLower(allowFrom), }, groups: { @@ -355,8 +358,12 @@ const DOCKS: Record = { blockStreamingCoalesceDefaults: { minChars: 1500, idleMs: 1000 }, }, config: { - resolveAllowFrom: ({ cfg, accountId }) => - (resolveSlackAccount({ cfg, accountId }).dm?.allowFrom ?? []).map((entry) => String(entry)), + resolveAllowFrom: ({ cfg, accountId }) => { + const account = resolveSlackAccount({ cfg, accountId }); + return (account.config.allowFrom ?? account.dm?.allowFrom ?? []).map((entry) => + String(entry), + ); + }, formatAllowFrom: ({ allowFrom }) => formatLower(allowFrom), }, groups: { diff --git a/src/channels/plugins/directory-config.ts b/src/channels/plugins/directory-config.ts index 5c25993a50b..3b57bd082c9 100644 --- a/src/channels/plugins/directory-config.ts +++ b/src/channels/plugins/directory-config.ts @@ -21,7 +21,7 @@ export async function listSlackDirectoryPeersFromConfig( const q = params.query?.trim().toLowerCase() || ""; const ids = new Set(); - for (const entry of account.dm?.allowFrom ?? []) { + for (const entry of account.config.allowFrom ?? account.dm?.allowFrom ?? []) { const raw = String(entry).trim(); if (!raw || raw === "*") { continue; @@ -84,7 +84,7 @@ export async function listDiscordDirectoryPeersFromConfig( const q = params.query?.trim().toLowerCase() || ""; const ids = new Set(); - for (const entry of account.config.dm?.allowFrom ?? []) { + for (const entry of account.config.allowFrom ?? account.config.dm?.allowFrom ?? []) { const raw = String(entry).trim(); if (!raw || raw === "*") { continue; diff --git a/src/config/schema.help.ts b/src/config/schema.help.ts index 145c805e79a..6bab5950a32 100644 --- a/src/config/schema.help.ts +++ b/src/config/schema.help.ts @@ -364,7 +364,7 @@ export const FIELD_HELP: Record = { "channels.discord.dmPolicy": 'Direct message access control ("pairing" recommended). "open" requires channels.discord.allowFrom=["*"].', "channels.discord.dm.policy": - 'Direct message access control ("pairing" recommended). "open" requires channels.discord.dm.allowFrom=["*"].', + 'Direct message access control ("pairing" recommended). "open" requires channels.discord.allowFrom=["*"] (legacy: channels.discord.dm.allowFrom).', "channels.discord.retry.attempts": "Max retry attempts for outbound Discord API calls (default: 3).", "channels.discord.retry.minDelayMs": "Minimum retry delay in ms for Discord outbound calls.", @@ -385,7 +385,7 @@ export const FIELD_HELP: Record = { "Discord presence activity type (0=Playing,1=Streaming,2=Listening,3=Watching,4=Custom,5=Competing).", "channels.discord.activityUrl": "Discord presence streaming URL (required for activityType=1).", "channels.slack.dm.policy": - 'Direct message access control ("pairing" recommended). "open" requires channels.slack.dm.allowFrom=["*"].', + 'Direct message access control ("pairing" recommended). "open" requires channels.slack.allowFrom=["*"] (legacy: channels.slack.dm.allowFrom).', "channels.slack.dmPolicy": 'Direct message access control ("pairing" recommended). "open" requires channels.slack.allowFrom=["*"].', }; diff --git a/src/discord/monitor/agent-components.ts b/src/discord/monitor/agent-components.ts index 697bbcbda8a..a99e8118665 100644 --- a/src/discord/monitor/agent-components.ts +++ b/src/discord/monitor/agent-components.ts @@ -141,7 +141,7 @@ export type AgentComponentContext = { cfg: OpenClawConfig; accountId: string; guildEntries?: Record; - /** DM allowlist (from dm.allowFrom config) */ + /** DM allowlist (from allowFrom config; legacy: dm.allowFrom) */ allowFrom?: Array; /** DM policy (default: "pairing") */ dmPolicy?: "open" | "pairing" | "allowlist" | "disabled"; diff --git a/src/security/audit-channel.ts b/src/security/audit-channel.ts index d62346787e7..ee55f7b23a2 100644 --- a/src/security/audit-channel.ts +++ b/src/security/audit-channel.ts @@ -237,7 +237,7 @@ export async function collectChannelSecurityFindings(params: { detail: "Discord slash commands are enabled, but neither an owner allowFrom list nor any per-guild/channel users allowlist is configured; /… commands will be rejected for everyone.", remediation: - "Add your user id to channels.discord.dm.allowFrom (or approve yourself via pairing), or configure channels.discord.guilds..users.", + "Add your user id to channels.discord.allowFrom (or approve yourself via pairing), or configure channels.discord.guilds..users.", }); } } @@ -277,12 +277,23 @@ export async function collectChannelSecurityFindings(params: { remediation: "Set commands.useAccessGroups=true (recommended).", }); } else { - const dmAllowFromRaw = (account as { dm?: { allowFrom?: unknown } } | null)?.dm - ?.allowFrom; - const dmAllowFrom = Array.isArray(dmAllowFromRaw) ? dmAllowFromRaw : []; + const allowFromRaw = ( + account as + | { config?: { allowFrom?: unknown }; dm?: { allowFrom?: unknown } } + | null + | undefined + )?.config?.allowFrom; + const legacyAllowFromRaw = ( + account as { dm?: { allowFrom?: unknown } } | null | undefined + )?.dm?.allowFrom; + const allowFrom = Array.isArray(allowFromRaw) + ? allowFromRaw + : Array.isArray(legacyAllowFromRaw) + ? legacyAllowFromRaw + : []; const storeAllowFrom = await readChannelAllowFromStore("slack").catch(() => []); const ownerAllowFromConfigured = - normalizeAllowFromList([...dmAllowFrom, ...storeAllowFrom]).length > 0; + normalizeAllowFromList([...allowFrom, ...storeAllowFrom]).length > 0; const channels = (slackCfg.channels as Record | undefined) ?? {}; const hasAnyChannelUsersAllowlist = Object.values(channels).some((value) => { if (!value || typeof value !== "object") { @@ -299,7 +310,7 @@ export async function collectChannelSecurityFindings(params: { detail: "Slack slash/native commands are enabled, but neither an owner allowFrom list nor any channels..users allowlist is configured; /… commands will be rejected for everyone.", remediation: - "Approve yourself via pairing (recommended), or set channels.slack.dm.allowFrom and/or channels.slack.channels..users.", + "Approve yourself via pairing (recommended), or set channels.slack.allowFrom and/or channels.slack.channels..users.", }); } }