refactor(security): unify config write target policy

2026-03-12 07:20:45 +00:00 · 2026-03-11 01:34:53 +00:00
parent 7289c19f1a
commit 3a39dc4e18
5 changed files with 203 additions and 76 deletions
--- a/src/auto-reply/reply/commands-allowlist.ts
+++ b/src/auto-reply/reply/commands-allowlist.ts
@@ -1,5 +1,10 @@
 import { getChannelDock } from "../../channels/dock.js";
-import { authorizeConfigWrite } from "../../channels/plugins/config-writes.js";
+import {
+  authorizeConfigWrite,
+  canBypassConfigWritePolicy,
+  formatConfigWriteDeniedMessage,
+  resolveExplicitConfigWriteTarget,
+} 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,7 +33,6 @@ 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";
@@ -232,12 +236,22 @@ function resolveAccountTarget(
  const channel = (channels[channelId] ??= {}) as Record<string, unknown>;
  const normalizedAccountId = normalizeAccountId(accountId);
  if (isBlockedObjectKey(normalizedAccountId)) {
-    return { target: channel, pathPrefix: `channels.${channelId}`, accountId: DEFAULT_ACCOUNT_ID };
+    return {
+      target: channel,
+      pathPrefix: `channels.${channelId}`,
+      accountId: DEFAULT_ACCOUNT_ID,
+      writeTarget: resolveExplicitConfigWriteTarget({ channelId }),
+    };
  }
  const hasAccounts = Boolean(channel.accounts && typeof channel.accounts === "object");
  const useAccount = normalizedAccountId !== DEFAULT_ACCOUNT_ID || hasAccounts;
  if (!useAccount) {
-    return { target: channel, pathPrefix: `channels.${channelId}`, accountId: normalizedAccountId };
+    return {
+      target: channel,
+      pathPrefix: `channels.${channelId}`,
+      accountId: normalizedAccountId,
+      writeTarget: resolveExplicitConfigWriteTarget({ channelId }),
+    };
  }
  const accounts = (channel.accounts ??= {}) as Record<string, unknown>;
  const existingAccount = Object.hasOwn(accounts, normalizedAccountId)
@@ -251,6 +265,10 @@ function resolveAccountTarget(
    target: account,
    pathPrefix: `channels.${channelId}.accounts.${normalizedAccountId}`,
    accountId: normalizedAccountId,
+    writeTarget: resolveExplicitConfigWriteTarget({
+      channelId,
+      accountId: normalizedAccountId,
+    }),
  };
 }

@@ -586,28 +604,6 @@ export const handleAllowlistCommand: CommandHandler = async (params, allowTextCo
  const shouldTouchStore = parsed.target !== "config" && listPairingChannels().includes(channelId);

  if (shouldUpdateConfig) {
-    const writeAuth = authorizeConfigWrite({
-      cfg: params.cfg,
-      origin: { channelId, accountId: params.ctx.AccountId },
-      targets: [{ channelId, accountId }],
-      allowBypass:
-        isInternalMessageChannel(params.command.channel) &&
-        params.ctx.GatewayClientScopes?.includes("operator.admin") === 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 ${blocked?.channelId ?? channelId}. Set ${hint} to enable.`,
-        },
-      };
-    }
-
    const allowlistPath = resolveChannelAllowFromPaths(channelId, scope);
    if (!allowlistPath) {
      return {
@@ -630,7 +626,26 @@ export const handleAllowlistCommand: CommandHandler = async (params, allowTextCo
      target,
      pathPrefix,
      accountId: normalizedAccountId,
+      writeTarget,
    } = resolveAccountTarget(parsedConfig, channelId, accountId);
+    const writeAuth = authorizeConfigWrite({
+      cfg: params.cfg,
+      origin: { channelId, accountId: params.ctx.AccountId },
+      target: writeTarget,
+      allowBypass: canBypassConfigWritePolicy({
+        channel: params.command.channel,
+        gatewayClientScopes: params.ctx.GatewayClientScopes,
+      }),
+    });
+    if (!writeAuth.allowed) {
+      return {
+        shouldContinue: false,
+        reply: {
+          text: formatConfigWriteDeniedMessage({ result: writeAuth, fallbackChannelId: channelId }),
+        },
+      };
+    }
+
    const existing: string[] = [];
    const existingPaths =
      scope === "dm" && (channelId === "slack" || channelId === "discord")
--- a/src/auto-reply/reply/commands-config.ts
+++ b/src/auto-reply/reply/commands-config.ts
@@ -1,6 +1,8 @@
 import {
  authorizeConfigWrite,
-  resolveConfigWriteScopesFromPath,
+  canBypassConfigWritePolicy,
+  formatConfigWriteDeniedMessage,
+  resolveConfigWriteTargetFromPath,
 } from "../../channels/plugins/config-writes.js";
 import { normalizeChannelId } from "../../channels/registry.js";
 import {
@@ -20,7 +22,6 @@ import {
  setConfigOverride,
  unsetConfigOverride,
 } from "../../config/runtime-overrides.js";
-import { isInternalMessageChannel } from "../../utils/message-channel.js";
 import {
  rejectUnauthorizedCommand,
  requireCommandFlagEnabled,
@@ -78,33 +79,17 @@ export const handleConfigCommand: CommandHandler = async (params, allowTextComma
    const writeAuth = authorizeConfigWrite({
      cfg: params.cfg,
      origin: { channelId, accountId: params.ctx.AccountId },
-      ...resolveConfigWriteScopesFromPath(parsedWritePath),
-      allowBypass:
-        isInternalMessageChannel(params.command.channel) &&
-        params.ctx.GatewayClientScopes?.includes("operator.admin") === true,
+      target: resolveConfigWriteTargetFromPath(parsedWritePath),
+      allowBypass: canBypassConfigWritePolicy({
+        channel: params.command.channel,
+        gatewayClientScopes: params.ctx.GatewayClientScopes,
+      }),
    });
    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: {
-          text: `⚠️ Config writes are disabled for ${channelLabel}. Set ${hint} to enable.`,
+          text: formatConfigWriteDeniedMessage({ result: writeAuth, fallbackChannelId: channelId }),
        },
      };
    }
--- a/src/auto-reply/reply/commands.test.ts
+++ b/src/auto-reply/reply/commands.test.ts
@@ -993,6 +993,10 @@ describe("handleCommands /allowlist", () => {
        },
      },
    } as OpenClawConfig;
+    readConfigFileSnapshotMock.mockResolvedValueOnce({
+      valid: true,
+      parsed: structuredClone(cfg),
+    });
    const params = buildPolicyParams("/allowlist add dm --account work --config 789", cfg, {
      AccountId: "default",
      Provider: "telegram",
--- a/src/channels/plugins/config-writes.ts
+++ b/src/channels/plugins/config-writes.ts
@@ -1,6 +1,8 @@
 import type { OpenClawConfig } from "../../config/config.js";
 import { resolveAccountEntry } from "../../routing/account-lookup.js";
+import { DEFAULT_ACCOUNT_ID } from "../../routing/session-key.js";
 import { normalizeAccountId } from "../../routing/session-key.js";
+import { isInternalMessageChannel } from "../../utils/message-channel.js";
 import type { ChannelId } from "./types.js";

 type ChannelConfigWithAccounts = {
@@ -17,6 +19,12 @@ export type ConfigWriteScope = {
  accountId?: string | null;
 };

+export type ConfigWriteTarget =
+  | { kind: "global" }
+  | { kind: "channel"; scope: { channelId: ChannelId } }
+  | { kind: "account"; scope: { channelId: ChannelId; accountId: string } }
+  | { kind: "ambiguous"; scopes: ConfigWriteScope[] };
+
 export type ConfigWriteAuthorizationResult =
  | { allowed: true }
  | {
@@ -47,14 +55,13 @@ export function resolveChannelConfigWrites(params: {
 export function authorizeConfigWrite(params: {
  cfg: OpenClawConfig;
  origin?: ConfigWriteScope;
-  targets?: ConfigWriteScope[];
+  target?: ConfigWriteTarget;
  allowBypass?: boolean;
-  hasAmbiguousTarget?: boolean;
 }): ConfigWriteAuthorizationResult {
  if (params.allowBypass) {
    return { allowed: true };
  }
-  if (params.hasAmbiguousTarget) {
+  if (params.target?.kind === "ambiguous") {
    return { allowed: false, reason: "ambiguous-target" };
  }
  if (
@@ -72,7 +79,7 @@ export function authorizeConfigWrite(params: {
    };
  }
  const seen = new Set<string>();
-  for (const target of params.targets ?? []) {
+  for (const target of listConfigWriteTargetScopes(params.target)) {
    if (!target.channelId) {
      continue;
    }
@@ -98,31 +105,79 @@ export function authorizeConfigWrite(params: {
  return { allowed: true };
 }

-export function resolveConfigWriteScopesFromPath(path: string[]): {
-  targets: ConfigWriteScope[];
-  hasAmbiguousTarget: boolean;
-} {
+export function resolveExplicitConfigWriteTarget(scope: ConfigWriteScope): ConfigWriteTarget {
+  if (!scope.channelId) {
+    return { kind: "global" };
+  }
+  const accountId = normalizeAccountId(scope.accountId);
+  if (!accountId || accountId === DEFAULT_ACCOUNT_ID) {
+    return { kind: "channel", scope: { channelId: scope.channelId } };
+  }
+  return { kind: "account", scope: { channelId: scope.channelId, accountId } };
+}
+
+export function resolveConfigWriteTargetFromPath(path: string[]): ConfigWriteTarget {
  if (path[0] !== "channels") {
-    return { targets: [], hasAmbiguousTarget: false };
+    return { kind: "global" };
  }
  if (path.length < 2) {
-    return { targets: [], hasAmbiguousTarget: true };
+    return { kind: "ambiguous", scopes: [] };
  }
  const channelId = path[1].trim().toLowerCase() as ChannelId;
  if (!channelId) {
-    return { targets: [], hasAmbiguousTarget: true };
+    return { kind: "ambiguous", scopes: [] };
  }
  if (path.length === 2) {
-    return { targets: [{ channelId }], hasAmbiguousTarget: true };
+    return { kind: "ambiguous", scopes: [{ channelId }] };
  }
  if (path[2] !== "accounts") {
-    return { targets: [{ channelId }], hasAmbiguousTarget: false };
+    return { kind: "channel", scope: { channelId } };
  }
  if (path.length < 4) {
-    return { targets: [{ channelId }], hasAmbiguousTarget: true };
+    return { kind: "ambiguous", scopes: [{ channelId }] };
  }
-  return {
-    targets: [{ channelId, accountId: normalizeAccountId(path[3]) }],
-    hasAmbiguousTarget: false,
-  };
+  return resolveExplicitConfigWriteTarget({
+    channelId,
+    accountId: normalizeAccountId(path[3]),
+  });
+}
+
+export function canBypassConfigWritePolicy(params: {
+  channel?: string | null;
+  gatewayClientScopes?: string[] | null;
+}): boolean {
+  return (
+    isInternalMessageChannel(params.channel) &&
+    params.gatewayClientScopes?.includes("operator.admin") === true
+  );
+}
+
+export function formatConfigWriteDeniedMessage(params: {
+  result: Exclude<ConfigWriteAuthorizationResult, { allowed: true }>;
+  fallbackChannelId?: ChannelId | null;
+}): string {
+  if (params.result.reason === "ambiguous-target") {
+    return "⚠️ Channel-initiated /config writes cannot replace channels, channel roots, or accounts collections. Use a more specific path or gateway operator.admin.";
+  }
+
+  const blocked = params.result.blockedScope?.scope;
+  const channelLabel = blocked?.channelId ?? params.fallbackChannelId ?? "this channel";
+  const hint = blocked?.channelId
+    ? blocked.accountId
+      ? `channels.${blocked.channelId}.accounts.${blocked.accountId}.configWrites=true`
+      : `channels.${blocked.channelId}.configWrites=true`
+    : params.fallbackChannelId
+      ? `channels.${params.fallbackChannelId}.configWrites=true`
+      : "channels.<channel>.configWrites=true";
+  return `⚠️ Config writes are disabled for ${channelLabel}. Set ${hint} to enable.`;
+}
+
+function listConfigWriteTargetScopes(target?: ConfigWriteTarget): ConfigWriteScope[] {
+  if (!target || target.kind === "global") {
+    return [];
+  }
+  if (target.kind === "ambiguous") {
+    return target.scopes;
+  }
+  return [target.scope];
 }
--- a/src/channels/plugins/plugins-core.test.ts
+++ b/src/channels/plugins/plugins-core.test.ts
@@ -19,11 +19,15 @@ import {
  createTestRegistry,
 } from "../../test-utils/channel-plugins.js";
 import { withEnvAsync } from "../../test-utils/env.js";
+import { INTERNAL_MESSAGE_CHANNEL } from "../../utils/message-channel.js";
 import { getChannelPluginCatalogEntry, listChannelPluginCatalogEntries } from "./catalog.js";
 import {
  authorizeConfigWrite,
+  canBypassConfigWritePolicy,
+  formatConfigWriteDeniedMessage,
+  resolveExplicitConfigWriteTarget,
  resolveChannelConfigWrites,
-  resolveConfigWriteScopesFromPath,
+  resolveConfigWriteTargetFromPath,
 } from "./config-writes.js";
 import {
  listDiscordDirectoryGroupsFromConfig,
@@ -336,7 +340,7 @@ describe("authorizeConfigWrite", () => {
      authorizeConfigWrite({
        cfg,
        origin: { channelId: "slack", accountId: "default" },
-        targets: [{ channelId: "slack", accountId: "work" }],
+        target: resolveExplicitConfigWriteTarget({ channelId: "slack", accountId: "work" }),
      }),
    ).toEqual({
      allowed: false,
@@ -345,16 +349,80 @@ describe("authorizeConfigWrite", () => {
    });
  });

+  it("blocks when the origin account disables writes", () => {
+    const cfg = makeSlackConfigWritesCfg("default");
+    expect(
+      authorizeConfigWrite({
+        cfg,
+        origin: { channelId: "slack", accountId: "default" },
+        target: resolveExplicitConfigWriteTarget({ channelId: "slack", accountId: "work" }),
+      }),
+    ).toEqual({
+      allowed: false,
+      reason: "origin-disabled",
+      blockedScope: { kind: "origin", scope: { channelId: "slack", accountId: "default" } },
+    });
+  });
+
+  it("allows bypass for internal operator.admin writes", () => {
+    const cfg = makeSlackConfigWritesCfg("work");
+    expect(
+      authorizeConfigWrite({
+        cfg,
+        origin: { channelId: "slack", accountId: "default" },
+        target: resolveExplicitConfigWriteTarget({ channelId: "slack", accountId: "work" }),
+        allowBypass: canBypassConfigWritePolicy({
+          channel: INTERNAL_MESSAGE_CHANNEL,
+          gatewayClientScopes: ["operator.admin"],
+        }),
+      }),
+    ).toEqual({ allowed: true });
+  });
+
+  it("treats non-channel config paths as global writes", () => {
+    const cfg = makeSlackConfigWritesCfg("work");
+    expect(
+      authorizeConfigWrite({
+        cfg,
+        origin: { channelId: "slack", accountId: "default" },
+        target: resolveConfigWriteTargetFromPath(["messages", "ackReaction"]),
+      }),
+    ).toEqual({ allowed: true });
+  });
+
  it("rejects ambiguous channel collection writes", () => {
-    expect(resolveConfigWriteScopesFromPath(["channels", "telegram"])).toEqual({
-      targets: [{ channelId: "telegram" }],
-      hasAmbiguousTarget: true,
+    expect(resolveConfigWriteTargetFromPath(["channels", "telegram"])).toEqual({
+      kind: "ambiguous",
+      scopes: [{ channelId: "telegram" }],
    });
-    expect(resolveConfigWriteScopesFromPath(["channels", "telegram", "accounts"])).toEqual({
-      targets: [{ channelId: "telegram" }],
-      hasAmbiguousTarget: true,
+    expect(resolveConfigWriteTargetFromPath(["channels", "telegram", "accounts"])).toEqual({
+      kind: "ambiguous",
+      scopes: [{ channelId: "telegram" }],
    });
  });
+
+  it("resolves explicit channel and account targets", () => {
+    expect(resolveExplicitConfigWriteTarget({ channelId: "slack" })).toEqual({
+      kind: "channel",
+      scope: { channelId: "slack" },
+    });
+    expect(resolveExplicitConfigWriteTarget({ channelId: "slack", accountId: "work" })).toEqual({
+      kind: "account",
+      scope: { channelId: "slack", accountId: "work" },
+    });
+  });
+
+  it("formats denied messages consistently", () => {
+    expect(
+      formatConfigWriteDeniedMessage({
+        result: {
+          allowed: false,
+          reason: "target-disabled",
+          blockedScope: { kind: "target", scope: { channelId: "slack", accountId: "work" } },
+        },
+      }),
+    ).toContain("channels.slack.accounts.work.configWrites=true");
+  });
 });

 describe("directory (config-backed)", () => {