fix(security): enforce target account configWrites

2026-05-05 07:20:22 +00:00 · 2026-03-11 01:24:17 +00:00
parent 11924a7026
commit 8eac939417
8 changed files with 303 additions and 33 deletions
--- a/src/auto-reply/reply/commands-allowlist.ts
+++ b/src/auto-reply/reply/commands-allowlist.ts
@@ -1,5 +1,5 @@
 import { getChannelDock } from "../../channels/dock.js";
-import { resolveChannelConfigWrites } from "../../channels/plugins/config-writes.js";
+import { authorizeConfigWrite } from "../../channels/plugins/config-writes.js";
 import { listPairingChannels } from "../../channels/plugins/pairing.js";
 import type { ChannelId } from "../../channels/plugins/types.js";
 import { normalizeChannelId } from "../../channels/registry.js";
@@ -28,6 +28,7 @@ import { resolveSignalAccount } from "../../signal/accounts.js";
 import { resolveSlackAccount } from "../../slack/accounts.js";
 import { resolveSlackUserAllowlist } from "../../slack/resolve-users.js";
 import { resolveTelegramAccount } from "../../telegram/accounts.js";
+import { isInternalMessageChannel } from "../../utils/message-channel.js";
 import { resolveWhatsAppAccount } from "../../web/accounts.js";
 import { rejectUnauthorizedCommand, requireCommandFlagEnabled } from "./command-gates.js";
 import type { CommandHandler } from "./commands-types.js";
@@ -585,16 +586,25 @@ export const handleAllowlistCommand: CommandHandler = async (params, allowTextCo
  const shouldTouchStore = parsed.target !== "config" && listPairingChannels().includes(channelId);

  if (shouldUpdateConfig) {
-    const allowWrites = resolveChannelConfigWrites({
+    const writeAuth = authorizeConfigWrite({
      cfg: params.cfg,
-      channelId,
-      accountId: params.ctx.AccountId,
+      origin: { channelId, accountId: params.ctx.AccountId },
+      targets: [{ channelId, accountId }],
+      allowBypass:
+        isInternalMessageChannel(params.command.channel) &&
+        params.ctx.GatewayClientScopes?.includes("operator.admin") === true,
    });
-    if (!allowWrites) {
-      const hint = `channels.${channelId}.configWrites=true`;
+    if (!writeAuth.allowed) {
+      const blocked = writeAuth.blockedScope?.scope;
+      const hint =
+        blocked?.channelId && blocked.accountId
+          ? `channels.${blocked.channelId}.accounts.${blocked.accountId}.configWrites=true`
+          : `channels.${blocked?.channelId ?? channelId}.configWrites=true`;
      return {
        shouldContinue: false,
-        reply: { text: `⚠️ Config writes are disabled for ${channelId}. Set ${hint} to enable.` },
+        reply: {
+          text: `⚠️ Config writes are disabled for ${blocked?.channelId ?? channelId}. Set ${hint} to enable.`,
+        },
      };
    }

--- a/src/auto-reply/reply/commands-config.ts
+++ b/src/auto-reply/reply/commands-config.ts
@@ -1,4 +1,7 @@
-import { resolveChannelConfigWrites } from "../../channels/plugins/config-writes.js";
+import {
+  authorizeConfigWrite,
+  resolveConfigWriteScopesFromPath,
+} from "../../channels/plugins/config-writes.js";
 import { normalizeChannelId } from "../../channels/registry.js";
 import {
  getConfigValueAtPath,
@@ -17,6 +20,7 @@ import {
  setConfigOverride,
  unsetConfigOverride,
 } from "../../config/runtime-overrides.js";
+import { isInternalMessageChannel } from "../../utils/message-channel.js";
 import {
  rejectUnauthorizedCommand,
  requireCommandFlagEnabled,
@@ -52,6 +56,7 @@ export const handleConfigCommand: CommandHandler = async (params, allowTextComma
    };
  }

+  let parsedWritePath: string[] | undefined;
  if (configCommand.action === "set" || configCommand.action === "unset") {
    const missingAdminScope = requireGatewayClientScopeForInternalChannel(params, {
      label: "/config write",
@@ -61,17 +66,41 @@ export const handleConfigCommand: CommandHandler = async (params, allowTextComma
    if (missingAdminScope) {
      return missingAdminScope;
    }
+    const parsedPath = parseConfigPath(configCommand.path);
+    if (!parsedPath.ok || !parsedPath.path) {
+      return {
+        shouldContinue: false,
+        reply: { text: `⚠️ ${parsedPath.error ?? "Invalid path."}` },
+      };
+    }
+    parsedWritePath = parsedPath.path;
    const channelId = params.command.channelId ?? normalizeChannelId(params.command.channel);
-    const allowWrites = resolveChannelConfigWrites({
+    const writeAuth = authorizeConfigWrite({
      cfg: params.cfg,
-      channelId,
-      accountId: params.ctx.AccountId,
+      origin: { channelId, accountId: params.ctx.AccountId },
+      ...resolveConfigWriteScopesFromPath(parsedWritePath),
+      allowBypass:
+        isInternalMessageChannel(params.command.channel) &&
+        params.ctx.GatewayClientScopes?.includes("operator.admin") === true,
    });
-    if (!allowWrites) {
-      const channelLabel = channelId ?? "this channel";
-      const hint = channelId
-        ? `channels.${channelId}.configWrites=true`
-        : "channels.<channel>.configWrites=true";
+    if (!writeAuth.allowed) {
+      if (writeAuth.reason === "ambiguous-target") {
+        return {
+          shouldContinue: false,
+          reply: {
+            text: "⚠️ Channel-initiated /config writes cannot replace channels, channel roots, or accounts collections. Use a more specific path or gateway operator.admin.",
+          },
+        };
+      }
+      const blocked = writeAuth.blockedScope?.scope;
+      const channelLabel = blocked?.channelId ?? channelId ?? "this channel";
+      const hint = blocked?.channelId
+        ? blocked?.accountId
+          ? `channels.${blocked.channelId}.accounts.${blocked.accountId}.configWrites=true`
+          : `channels.${blocked.channelId}.configWrites=true`
+        : channelId
+          ? `channels.${channelId}.configWrites=true`
+          : "channels.<channel>.configWrites=true";
      return {
        shouldContinue: false,
        reply: {
@@ -119,14 +148,7 @@ export const handleConfigCommand: CommandHandler = async (params, allowTextComma
  }

  if (configCommand.action === "unset") {
-    const parsedPath = parseConfigPath(configCommand.path);
-    if (!parsedPath.ok || !parsedPath.path) {
-      return {
-        shouldContinue: false,
-        reply: { text: `⚠️ ${parsedPath.error ?? "Invalid path."}` },
-      };
-    }
-    const removed = unsetConfigValueAtPath(parsedBase, parsedPath.path);
+    const removed = unsetConfigValueAtPath(parsedBase, parsedWritePath ?? []);
    if (!removed) {
      return {
        shouldContinue: false,
@@ -151,14 +173,7 @@ export const handleConfigCommand: CommandHandler = async (params, allowTextComma
  }

  if (configCommand.action === "set") {
-    const parsedPath = parseConfigPath(configCommand.path);
-    if (!parsedPath.ok || !parsedPath.path) {
-      return {
-        shouldContinue: false,
-        reply: { text: `⚠️ ${parsedPath.error ?? "Invalid path."}` },
-      };
-    }
-    setConfigValueAtPath(parsedBase, parsedPath.path, configCommand.value);
+    setConfigValueAtPath(parsedBase, parsedWritePath ?? [], configCommand.value);
    const validated = validateConfigObjectWithPlugins(parsedBase);
    if (!validated.ok) {
      const issue = validated.issues[0];
--- a/src/auto-reply/reply/commands.test.ts
+++ b/src/auto-reply/reply/commands.test.ts
@@ -682,6 +682,52 @@ describe("handleCommands /config configWrites gating", () => {
    expect(result.reply?.text).toContain("Config writes are disabled");
  });

+  it("blocks /config set when the target account disables writes", async () => {
+    const previousWriteCount = writeConfigFileMock.mock.calls.length;
+    const cfg = {
+      commands: { config: true, text: true },
+      channels: {
+        telegram: {
+          configWrites: true,
+          accounts: {
+            work: { configWrites: false, enabled: true },
+          },
+        },
+      },
+    } as OpenClawConfig;
+    const params = buildPolicyParams(
+      "/config set channels.telegram.accounts.work.enabled=false",
+      cfg,
+      {
+        AccountId: "default",
+        Provider: "telegram",
+        Surface: "telegram",
+      },
+    );
+    const result = await handleCommands(params);
+    expect(result.shouldContinue).toBe(false);
+    expect(result.reply?.text).toContain("channels.telegram.accounts.work.configWrites=true");
+    expect(writeConfigFileMock.mock.calls.length).toBe(previousWriteCount);
+  });
+
+  it("blocks ambiguous channel-root /config writes from channel commands", async () => {
+    const previousWriteCount = writeConfigFileMock.mock.calls.length;
+    const cfg = {
+      commands: { config: true, text: true },
+      channels: { telegram: { configWrites: true } },
+    } as OpenClawConfig;
+    const params = buildPolicyParams('/config set channels.telegram={"enabled":false}', cfg, {
+      Provider: "telegram",
+      Surface: "telegram",
+    });
+    const result = await handleCommands(params);
+    expect(result.shouldContinue).toBe(false);
+    expect(result.reply?.text).toContain(
+      "cannot replace channels, channel roots, or accounts collections",
+    );
+    expect(writeConfigFileMock.mock.calls.length).toBe(previousWriteCount);
+  });
+
  it("blocks /config set from gateway clients without operator.admin", async () => {
    const cfg = {
      commands: { config: true, text: true },
@@ -739,6 +785,49 @@ describe("handleCommands /config configWrites gating", () => {
    expect(writeConfigFileMock).toHaveBeenCalledOnce();
    expect(result.reply?.text).toContain("Config updated");
  });
+
+  it("keeps /config set working for gateway operator.admin on protected account paths", async () => {
+    readConfigFileSnapshotMock.mockResolvedValueOnce({
+      valid: true,
+      parsed: {
+        channels: {
+          telegram: {
+            accounts: {
+              work: { enabled: true, configWrites: false },
+            },
+          },
+        },
+      },
+    });
+    validateConfigObjectWithPluginsMock.mockImplementation((config: unknown) => ({
+      ok: true,
+      config,
+    }));
+    const params = buildParams(
+      "/config set channels.telegram.accounts.work.enabled=false",
+      {
+        commands: { config: true, text: true },
+        channels: {
+          telegram: {
+            accounts: {
+              work: { enabled: true, configWrites: false },
+            },
+          },
+        },
+      } as OpenClawConfig,
+      {
+        Provider: INTERNAL_MESSAGE_CHANNEL,
+        Surface: INTERNAL_MESSAGE_CHANNEL,
+        GatewayClientScopes: ["operator.write", "operator.admin"],
+      },
+    );
+    params.command.channel = INTERNAL_MESSAGE_CHANNEL;
+    const result = await handleCommands(params);
+    expect(result.shouldContinue).toBe(false);
+    expect(result.reply?.text).toContain("Config updated");
+    const written = writeConfigFileMock.mock.calls.at(-1)?.[0] as OpenClawConfig;
+    expect(written.channels?.telegram?.accounts?.work?.enabled).toBe(false);
+  });
 });

 describe("handleCommands bash alias", () => {
@@ -891,6 +980,31 @@ describe("handleCommands /allowlist", () => {
    });
  });

+  it("blocks config-targeted /allowlist edits when the target account disables writes", async () => {
+    const previousWriteCount = writeConfigFileMock.mock.calls.length;
+    const cfg = {
+      commands: { text: true, config: true },
+      channels: {
+        telegram: {
+          configWrites: true,
+          accounts: {
+            work: { configWrites: false, allowFrom: ["123"] },
+          },
+        },
+      },
+    } as OpenClawConfig;
+    const params = buildPolicyParams("/allowlist add dm --account work --config 789", cfg, {
+      AccountId: "default",
+      Provider: "telegram",
+      Surface: "telegram",
+    });
+    const result = await handleCommands(params);
+
+    expect(result.shouldContinue).toBe(false);
+    expect(result.reply?.text).toContain("channels.telegram.accounts.work.configWrites=true");
+    expect(writeConfigFileMock.mock.calls.length).toBe(previousWriteCount);
+  });
+
  it("removes default-account entries from scoped and legacy pairing stores", async () => {
    removeChannelAllowFromStoreEntryMock
      .mockResolvedValueOnce({
--- a/src/channels/plugins/config-writes.ts
+++ b/src/channels/plugins/config-writes.ts
@@ -12,6 +12,19 @@ function resolveAccountConfig(accounts: ChannelConfigWithAccounts["accounts"], a
  return resolveAccountEntry(accounts, accountId);
 }

+export type ConfigWriteScope = {
+  channelId?: ChannelId | null;
+  accountId?: string | null;
+};
+
+export type ConfigWriteAuthorizationResult =
+  | { allowed: true }
+  | {
+      allowed: false;
+      reason: "ambiguous-target" | "origin-disabled" | "target-disabled";
+      blockedScope?: { kind: "origin" | "target"; scope: ConfigWriteScope };
+    };
+
 export function resolveChannelConfigWrites(params: {
  cfg: OpenClawConfig;
  channelId?: ChannelId | null;
@@ -30,3 +43,86 @@ export function resolveChannelConfigWrites(params: {
  const value = accountConfig?.configWrites ?? channelConfig.configWrites;
  return value !== false;
 }
+
+export function authorizeConfigWrite(params: {
+  cfg: OpenClawConfig;
+  origin?: ConfigWriteScope;
+  targets?: ConfigWriteScope[];
+  allowBypass?: boolean;
+  hasAmbiguousTarget?: boolean;
+}): ConfigWriteAuthorizationResult {
+  if (params.allowBypass) {
+    return { allowed: true };
+  }
+  if (params.hasAmbiguousTarget) {
+    return { allowed: false, reason: "ambiguous-target" };
+  }
+  if (
+    params.origin?.channelId &&
+    !resolveChannelConfigWrites({
+      cfg: params.cfg,
+      channelId: params.origin.channelId,
+      accountId: params.origin.accountId,
+    })
+  ) {
+    return {
+      allowed: false,
+      reason: "origin-disabled",
+      blockedScope: { kind: "origin", scope: params.origin },
+    };
+  }
+  const seen = new Set<string>();
+  for (const target of params.targets ?? []) {
+    if (!target.channelId) {
+      continue;
+    }
+    const key = `${target.channelId}:${normalizeAccountId(target.accountId)}`;
+    if (seen.has(key)) {
+      continue;
+    }
+    seen.add(key);
+    if (
+      !resolveChannelConfigWrites({
+        cfg: params.cfg,
+        channelId: target.channelId,
+        accountId: target.accountId,
+      })
+    ) {
+      return {
+        allowed: false,
+        reason: "target-disabled",
+        blockedScope: { kind: "target", scope: target },
+      };
+    }
+  }
+  return { allowed: true };
+}
+
+export function resolveConfigWriteScopesFromPath(path: string[]): {
+  targets: ConfigWriteScope[];
+  hasAmbiguousTarget: boolean;
+} {
+  if (path[0] !== "channels") {
+    return { targets: [], hasAmbiguousTarget: false };
+  }
+  if (path.length < 2) {
+    return { targets: [], hasAmbiguousTarget: true };
+  }
+  const channelId = path[1].trim().toLowerCase() as ChannelId;
+  if (!channelId) {
+    return { targets: [], hasAmbiguousTarget: true };
+  }
+  if (path.length === 2) {
+    return { targets: [{ channelId }], hasAmbiguousTarget: true };
+  }
+  if (path[2] !== "accounts") {
+    return { targets: [{ channelId }], hasAmbiguousTarget: false };
+  }
+  if (path.length < 4) {
+    return { targets: [{ channelId }], hasAmbiguousTarget: true };
+  }
+  return {
+    targets: [{ channelId, accountId: normalizeAccountId(path[3]) }],
+    hasAmbiguousTarget: false,
+  };
+}
--- a/src/channels/plugins/plugins-core.test.ts
+++ b/src/channels/plugins/plugins-core.test.ts
@@ -20,7 +20,11 @@ import {
 } from "../../test-utils/channel-plugins.js";
 import { withEnvAsync } from "../../test-utils/env.js";
 import { getChannelPluginCatalogEntry, listChannelPluginCatalogEntries } from "./catalog.js";
-import { resolveChannelConfigWrites } from "./config-writes.js";
+import {
+  authorizeConfigWrite,
+  resolveChannelConfigWrites,
+  resolveConfigWriteScopesFromPath,
+} from "./config-writes.js";
 import {
  listDiscordDirectoryGroupsFromConfig,
  listDiscordDirectoryPeersFromConfig,
@@ -325,6 +329,34 @@ describe("resolveChannelConfigWrites", () => {
  });
 });

+describe("authorizeConfigWrite", () => {
+  it("blocks when a target account disables writes", () => {
+    const cfg = makeSlackConfigWritesCfg("work");
+    expect(
+      authorizeConfigWrite({
+        cfg,
+        origin: { channelId: "slack", accountId: "default" },
+        targets: [{ channelId: "slack", accountId: "work" }],
+      }),
+    ).toEqual({
+      allowed: false,
+      reason: "target-disabled",
+      blockedScope: { kind: "target", scope: { channelId: "slack", accountId: "work" } },
+    });
+  });
+
+  it("rejects ambiguous channel collection writes", () => {
+    expect(resolveConfigWriteScopesFromPath(["channels", "telegram"])).toEqual({
+      targets: [{ channelId: "telegram" }],
+      hasAmbiguousTarget: true,
+    });
+    expect(resolveConfigWriteScopesFromPath(["channels", "telegram", "accounts"])).toEqual({
+      targets: [{ channelId: "telegram" }],
+      hasAmbiguousTarget: true,
+    });
+  });
+});
+
 describe("directory (config-backed)", () => {
  it("lists Slack peers/groups from config", async () => {
    const cfg = {