fix(allowlist): canonicalize Slack/Discord allowFrom

2026-04-18 11:17:27 +00:00 · 2026-02-15 03:46:11 +01:00
parent 3c3695d7c2
commit cf04208cb9
7 changed files with 153 additions and 25 deletions
--- 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) {
--- 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", () => {
--- a/src/channels/dock.ts
+++ b/src/channels/dock.ts
@@ -191,13 +191,16 @@ const DOCKS: Record<ChatChannelId, ChannelDock> = {
      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<ChatChannelId, ChannelDock> = {
      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: {
--- 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<string>();

-  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<string>();

-  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;
--- a/src/config/schema.help.ts
+++ b/src/config/schema.help.ts
@@ -364,7 +364,7 @@ export const FIELD_HELP: Record<string, string> = {
  "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<string, string> = {
    "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=["*"].',
 };
--- 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<string, DiscordGuildEntryResolved>;
-  /** DM allowlist (from dm.allowFrom config) */
+  /** DM allowlist (from allowFrom config; legacy: dm.allowFrom) */
  allowFrom?: Array<string | number>;
  /** DM policy (default: "pairing") */
  dmPolicy?: "open" | "pairing" | "allowlist" | "disabled";
--- 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.<id>.users.",
+              "Add your user id to channels.discord.allowFrom (or approve yourself via pairing), or configure channels.discord.guilds.<id>.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<string, unknown> | 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.<id>.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.<id>.users.",
+                "Approve yourself via pairing (recommended), or set channels.slack.allowFrom and/or channels.slack.channels.<id>.users.",
            });
          }
        }