mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-30 12:11:04 +00:00
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 42a38d2b00
Closes #72360.
Thanks @coygeek.
Co-authored-by: Coy Geek <65363919+coygeek@users.noreply.github.com>
Co-authored-by: opencode <opencode@users.noreply.github.com>
This commit is contained in:
@@ -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({
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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<typeof authorizeConfigWrite>[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,
|
||||
});
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user