From 3a39dc4e18841d2a3986928fbeabcd36ae93b694 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 11 Mar 2026 01:34:53 +0000 Subject: [PATCH] refactor(security): unify config write target policy --- src/auto-reply/reply/commands-allowlist.ts | 67 +++++++++------- src/auto-reply/reply/commands-config.ts | 33 +++----- src/auto-reply/reply/commands.test.ts | 4 + src/channels/plugins/config-writes.ts | 91 +++++++++++++++++----- src/channels/plugins/plugins-core.test.ts | 84 ++++++++++++++++++-- 5 files changed, 203 insertions(+), 76 deletions(-) diff --git a/src/auto-reply/reply/commands-allowlist.ts b/src/auto-reply/reply/commands-allowlist.ts index 643d20143c6..ffba3bf2505 100644 --- 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; 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; 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") diff --git a/src/auto-reply/reply/commands-config.ts b/src/auto-reply/reply/commands-config.ts index edc0c145526..0d00358e582 100644 --- 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..configWrites=true"; return { shouldContinue: false, reply: { - text: `⚠️ Config writes are disabled for ${channelLabel}. Set ${hint} to enable.`, + text: formatConfigWriteDeniedMessage({ result: writeAuth, fallbackChannelId: channelId }), }, }; } diff --git a/src/auto-reply/reply/commands.test.ts b/src/auto-reply/reply/commands.test.ts index 01e2c2238dd..073cc36488c 100644 --- 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", diff --git a/src/channels/plugins/config-writes.ts b/src/channels/plugins/config-writes.ts index 440e95dcfe9..3e3ef36ed04 100644 --- 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(); - 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; + 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..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]; } diff --git a/src/channels/plugins/plugins-core.test.ts b/src/channels/plugins/plugins-core.test.ts index 8d574e472b4..4e346f465bd 100644 --- 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)", () => {