From ce64d74e5a0f36ac3d4338fe89ed37a4ec42d4ea Mon Sep 17 00:00:00 2001 From: Coy Geek <65363919+coygeek@users.noreply.github.com> Date: Tue, 26 May 2026 23:10:50 -0700 Subject: [PATCH] fix(commands): enforce /allowlist configWrites origin policy Summary: - Enforces /allowlist config and pairing-store writes against the real command origin plus the selected target. - Adds regressions for disabled Telegram-origin commands targeting an enabled Discord allowlist. Verification: - node scripts/run-vitest.mjs src/auto-reply/reply/commands-allowlist.test.ts - pnpm check:changed via Blacksmith Testbox tbx_01ksm06e82dnpxmnj00hrt6xzd - autoreview --mode local clean, no accepted/actionable findings - GitHub PR checks green on 42a38d2b007f55e182dabc0a2fa462441f3de112 Closes #72360. Thanks @coygeek. Co-authored-by: Coy Geek <65363919+coygeek@users.noreply.github.com> Co-authored-by: opencode --- .../reply/commands-allowlist.test.ts | 125 +++++++++++++++++- src/auto-reply/reply/commands-allowlist.ts | 30 ++++- src/auto-reply/reply/commands-config.ts | 4 +- .../reply/config-write-authorization.ts | 9 +- 4 files changed, 156 insertions(+), 12 deletions(-) diff --git a/src/auto-reply/reply/commands-allowlist.test.ts b/src/auto-reply/reply/commands-allowlist.test.ts index f4c203875a3..550c3346673 100644 --- a/src/auto-reply/reply/commands-allowlist.test.ts +++ b/src/auto-reply/reply/commands-allowlist.test.ts @@ -37,10 +37,12 @@ vi.mock("../../config/config.js", () => ({ transform: ( currentConfig: OpenClawConfig, context: { snapshot: ConfigSnapshotMock; previousHash: string | null; attempt: number }, - ) => Promise<{ nextConfig: OpenClawConfig; result?: unknown }> | { - nextConfig: OpenClawConfig; - result?: unknown; - }; + ) => + | Promise<{ nextConfig: OpenClawConfig; result?: unknown }> + | { + nextConfig: OpenClawConfig; + result?: unknown; + }; }) => { const snapshot = (await readConfigFileSnapshotMock()) as ConfigSnapshotMock; const previousHash = snapshot.hash ?? null; @@ -586,6 +588,121 @@ describe("handleAllowlistCommand", () => { expect(addChannelAllowFromStoreEntryMock).not.toHaveBeenCalled(); }); + it("blocks cross-channel config edits when the command origin disables writes", async () => { + const cfg = { + commands: { text: true, config: true }, + channels: { + telegram: { allowFrom: ["123"], configWrites: false }, + discord: { allowFrom: ["456"], configWrites: true }, + }, + } as OpenClawConfig; + readConfigFileSnapshotMock.mockResolvedValueOnce({ + valid: true, + parsed: structuredClone(cfg), + }); + const params = buildAllowlistParams("/allowlist add dm --channel discord --config 789", cfg, { + Provider: "telegram", + Surface: "telegram", + }); + params.command.senderIsOwner = true; + const result = await handleAllowlistCommand(params, true); + + expect(result?.shouldContinue).toBe(false); + expect(result?.reply?.text).toContain("channels.telegram.accounts.default.configWrites=true"); + expect(replaceConfigFileMock).not.toHaveBeenCalled(); + expect(addChannelAllowFromStoreEntryMock).not.toHaveBeenCalled(); + }); + + it("blocks store-targeted allowlist edits when channel configWrites is false", async () => { + const cfg = { + commands: { text: true, config: true }, + channels: { + telegram: { + allowFrom: ["123"], + configWrites: false, + }, + }, + } as OpenClawConfig; + const params = buildAllowlistParams("/allowlist add dm --store 789", cfg); + params.command.senderIsOwner = true; + const result = await handleAllowlistCommand(params, true); + + expect(result?.shouldContinue).toBe(false); + expect(result?.reply?.text).toContain("channels.telegram.accounts.default.configWrites=true"); + expect(addChannelAllowFromStoreEntryMock).not.toHaveBeenCalled(); + }); + + it("blocks store-targeted allowlist edits when account configWrites is false", async () => { + const cfg = { + commands: { text: true, config: true }, + channels: { + telegram: { + configWrites: true, + accounts: { + work: { configWrites: false, allowFrom: ["123"] }, + }, + }, + }, + } as OpenClawConfig; + const params = buildAllowlistParams("/allowlist add dm --store --account work 789", cfg, { + AccountId: "work", + }); + params.command.senderIsOwner = true; + const result = await handleAllowlistCommand(params, true); + + expect(result?.shouldContinue).toBe(false); + expect(result?.reply?.text).toContain("channels.telegram.accounts.work.configWrites=true"); + expect(addChannelAllowFromStoreEntryMock).not.toHaveBeenCalled(); + }); + + it("blocks cross-channel store edits when the command origin disables writes", async () => { + const cfg = { + commands: { text: true, config: true }, + channels: { + telegram: { allowFrom: ["123"], configWrites: false }, + discord: { allowFrom: ["456"], configWrites: true }, + }, + } as OpenClawConfig; + const params = buildAllowlistParams("/allowlist add dm --channel discord --store 789", cfg, { + Provider: "telegram", + Surface: "telegram", + }); + params.command.senderIsOwner = true; + const result = await handleAllowlistCommand(params, true); + + expect(result?.shouldContinue).toBe(false); + expect(result?.reply?.text).toContain("channels.telegram.accounts.default.configWrites=true"); + expect(addChannelAllowFromStoreEntryMock).not.toHaveBeenCalled(); + }); + + it("allows store-targeted allowlist edits when configWrites is true", async () => { + addChannelAllowFromStoreEntryMock.mockResolvedValueOnce({ + changed: true, + allowFrom: ["789"], + }); + + const cfg = { + commands: { text: true, config: true }, + channels: { + telegram: { + allowFrom: ["123"], + configWrites: true, + }, + }, + } as OpenClawConfig; + const params = buildAllowlistParams("/allowlist add dm --store 789", cfg); + params.command.senderIsOwner = true; + const result = await handleAllowlistCommand(params, true); + + expect(result?.shouldContinue).toBe(false); + expect(result?.reply?.text).toContain("DM allowlist added in pairing store"); + expect(addChannelAllowFromStoreEntryMock).toHaveBeenCalledWith({ + channel: "telegram", + entry: "789", + accountId: "default", + }); + }); + it("removes default-account entries from scoped and legacy pairing stores", async () => { removeChannelAllowFromStoreEntryMock .mockResolvedValueOnce({ diff --git a/src/auto-reply/reply/commands-allowlist.ts b/src/auto-reply/reply/commands-allowlist.ts index 4d4520e4cc5..e005049a77a 100644 --- a/src/auto-reply/reply/commands-allowlist.ts +++ b/src/auto-reply/reply/commands-allowlist.ts @@ -1,3 +1,4 @@ +import { resolveExplicitConfigWriteTarget } from "../../channels/plugins/config-writes.js"; import { getChannelPlugin } from "../../channels/plugins/index.js"; import type { ChannelId } from "../../channels/plugins/types.public.js"; import { normalizeChannelId } from "../../channels/registry.js"; @@ -14,6 +15,7 @@ import { normalizeOptionalString, } from "../../shared/string-coerce.js"; import { normalizeStringEntries } from "../../shared/string-normalization.js"; +import { resolveChannelAccountId, resolveCommandSurfaceChannel } from "./channel-context.js"; import { rejectNonOwnerCommand, rejectUnauthorizedCommand, @@ -307,6 +309,13 @@ export const handleAllowlistCommand: CommandHandler = async (params, allowTextCo parsedAccount: parsed.account, ctxAccountId: params.ctx.AccountId, }); + const originChannelId = + params.command.channelId ?? normalizeChannelId(resolveCommandSurfaceChannel(params)); + const originAccountId = resolveChannelAccountId({ + cfg: params.cfg, + ctx: params.ctx, + command: params.command, + }); const plugin = getChannelPlugin(channelId); if (parsed.action === "list") { @@ -490,10 +499,11 @@ export const handleAllowlistCommand: CommandHandler = async (params, allowTextCo const deniedText = resolveConfigWriteDeniedText({ cfg: params.cfg, channel: params.command.channel, - channelId, - accountId, + originChannelId, + originAccountId, gatewayClientScopes: params.ctx.GatewayClientScopes, target: editResult.writeTarget, + fallbackChannelId: channelId, }); if (deniedText) { return { @@ -562,6 +572,22 @@ export const handleAllowlistCommand: CommandHandler = async (params, allowTextCo }; } + const storeDeniedText = resolveConfigWriteDeniedText({ + cfg: params.cfg, + channel: params.command.channel, + originChannelId, + originAccountId, + gatewayClientScopes: params.ctx.GatewayClientScopes, + target: resolveExplicitConfigWriteTarget({ channelId, accountId }), + fallbackChannelId: channelId, + }); + if (storeDeniedText) { + return { + shouldContinue: false, + reply: { text: storeDeniedText }, + }; + } + await updatePairingStoreAllowlist({ action: parsed.action, channelId, diff --git a/src/auto-reply/reply/commands-config.ts b/src/auto-reply/reply/commands-config.ts index 006dc3f1bc7..8bb8ba0900d 100644 --- a/src/auto-reply/reply/commands-config.ts +++ b/src/auto-reply/reply/commands-config.ts @@ -81,8 +81,8 @@ export const handleConfigCommand: CommandHandler = async (params, allowTextComma const deniedText = resolveConfigWriteDeniedText({ cfg: params.cfg, channel: params.command.channel, - channelId, - accountId: resolveChannelAccountId({ + originChannelId: channelId, + originAccountId: resolveChannelAccountId({ cfg: params.cfg, ctx: params.ctx, command: params.command, diff --git a/src/auto-reply/reply/config-write-authorization.ts b/src/auto-reply/reply/config-write-authorization.ts index be31230bf01..5ccec6582e9 100644 --- a/src/auto-reply/reply/config-write-authorization.ts +++ b/src/auto-reply/reply/config-write-authorization.ts @@ -9,14 +9,15 @@ import type { OpenClawConfig } from "../../config/types.openclaw.js"; export function resolveConfigWriteDeniedText(params: { cfg: OpenClawConfig; channel?: string | null; - channelId: ChannelId | null; - accountId?: string; + originChannelId: ChannelId | null; + originAccountId?: string; gatewayClientScopes?: string[]; target: Parameters[0]["target"]; + fallbackChannelId?: ChannelId | null; }): string | null { const writeAuth = authorizeConfigWrite({ cfg: params.cfg, - origin: { channelId: params.channelId, accountId: params.accountId }, + origin: { channelId: params.originChannelId, accountId: params.originAccountId }, target: params.target, allowBypass: canBypassConfigWritePolicy({ channel: params.channel ?? "", @@ -28,6 +29,6 @@ export function resolveConfigWriteDeniedText(params: { } return formatConfigWriteDeniedMessage({ result: writeAuth, - fallbackChannelId: params.channelId, + fallbackChannelId: params.fallbackChannelId ?? params.originChannelId, }); }