From 10b8839a8240a23d8828efcf72d8cbf070bf180d Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 21 Feb 2026 14:30:53 +0100 Subject: [PATCH] fix(security): centralize WhatsApp outbound auth and return 403 tool auth errors --- src/agents/tools/common.ts | 11 ++- src/agents/tools/whatsapp-actions.e2e.test.ts | 90 ++++++++++++++++--- src/agents/tools/whatsapp-actions.ts | 26 ++---- src/agents/tools/whatsapp-target-auth.ts | 27 ++++++ src/gateway/tools-invoke-http.test.ts | 22 ++++- src/gateway/tools-invoke-http.ts | 28 +++--- 6 files changed, 165 insertions(+), 39 deletions(-) create mode 100644 src/agents/tools/whatsapp-target-auth.ts diff --git a/src/agents/tools/common.ts b/src/agents/tools/common.ts index b5f88c2fe9d..1aea6dd3cfa 100644 --- a/src/agents/tools/common.ts +++ b/src/agents/tools/common.ts @@ -24,7 +24,7 @@ export type ActionGate> = ( export const OWNER_ONLY_TOOL_ERROR = "Tool restricted to owner senders."; export class ToolInputError extends Error { - readonly status = 400; + readonly status: number = 400; constructor(message: string) { super(message); @@ -32,6 +32,15 @@ export class ToolInputError extends Error { } } +export class ToolAuthorizationError extends ToolInputError { + override readonly status = 403; + + constructor(message: string) { + super(message); + this.name = "ToolAuthorizationError"; + } +} + export function createActionGate>( actions: T | undefined, ): ActionGate { diff --git a/src/agents/tools/whatsapp-actions.e2e.test.ts b/src/agents/tools/whatsapp-actions.e2e.test.ts index 0cc2a544a12..bb0941dbb42 100644 --- a/src/agents/tools/whatsapp-actions.e2e.test.ts +++ b/src/agents/tools/whatsapp-actions.e2e.test.ts @@ -1,9 +1,12 @@ -import { describe, expect, it, vi } from "vitest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; import type { OpenClawConfig } from "../../config/config.js"; +import { DEFAULT_ACCOUNT_ID } from "../../routing/session-key.js"; import { handleWhatsAppAction } from "./whatsapp-actions.js"; -const sendReactionWhatsApp = vi.fn(async () => undefined); -const sendPollWhatsApp = vi.fn(async () => ({ messageId: "poll-1", toJid: "jid-1" })); +const { sendReactionWhatsApp, sendPollWhatsApp } = vi.hoisted(() => ({ + sendReactionWhatsApp: vi.fn(async () => undefined), + sendPollWhatsApp: vi.fn(async () => ({ messageId: "poll-1", toJid: "jid-1" })), +})); vi.mock("../../web/outbound.js", () => ({ sendReactionWhatsApp, @@ -15,6 +18,10 @@ const enabledConfig = { } as OpenClawConfig; describe("handleWhatsAppAction", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + it("adds reactions", async () => { await handleWhatsAppAction( { @@ -25,11 +32,11 @@ describe("handleWhatsAppAction", () => { }, enabledConfig, ); - expect(sendReactionWhatsApp).toHaveBeenCalledWith("123@s.whatsapp.net", "msg1", "✅", { + expect(sendReactionWhatsApp).toHaveBeenLastCalledWith("+123", "msg1", "✅", { verbose: false, fromMe: undefined, participant: undefined, - accountId: undefined, + accountId: DEFAULT_ACCOUNT_ID, }); }); @@ -43,11 +50,11 @@ describe("handleWhatsAppAction", () => { }, enabledConfig, ); - expect(sendReactionWhatsApp).toHaveBeenCalledWith("123@s.whatsapp.net", "msg1", "", { + expect(sendReactionWhatsApp).toHaveBeenLastCalledWith("+123", "msg1", "", { verbose: false, fromMe: undefined, participant: undefined, - accountId: undefined, + accountId: DEFAULT_ACCOUNT_ID, }); }); @@ -62,11 +69,11 @@ describe("handleWhatsAppAction", () => { }, enabledConfig, ); - expect(sendReactionWhatsApp).toHaveBeenCalledWith("123@s.whatsapp.net", "msg1", "", { + expect(sendReactionWhatsApp).toHaveBeenLastCalledWith("+123", "msg1", "", { verbose: false, fromMe: undefined, participant: undefined, - accountId: undefined, + accountId: DEFAULT_ACCOUNT_ID, }); }); @@ -83,7 +90,7 @@ describe("handleWhatsAppAction", () => { }, enabledConfig, ); - expect(sendReactionWhatsApp).toHaveBeenCalledWith("123@s.whatsapp.net", "msg1", "🎉", { + expect(sendReactionWhatsApp).toHaveBeenLastCalledWith("+123", "msg1", "🎉", { verbose: false, fromMe: true, participant: "999@s.whatsapp.net", @@ -107,4 +114,67 @@ describe("handleWhatsAppAction", () => { ), ).rejects.toThrow(/WhatsApp reactions are disabled/); }); + + it("applies default account allowFrom when accountId is omitted", async () => { + const cfg = { + channels: { + whatsapp: { + actions: { reactions: true }, + allowFrom: ["111@s.whatsapp.net"], + accounts: { + [DEFAULT_ACCOUNT_ID]: { + allowFrom: ["222@s.whatsapp.net"], + }, + }, + }, + }, + } as OpenClawConfig; + + await expect( + handleWhatsAppAction( + { + action: "react", + chatJid: "111@s.whatsapp.net", + messageId: "msg1", + emoji: "✅", + }, + cfg, + ), + ).rejects.toMatchObject({ + name: "ToolAuthorizationError", + status: 403, + }); + }); + + it("routes to resolved default account when no accountId is provided", async () => { + const cfg = { + channels: { + whatsapp: { + actions: { reactions: true }, + accounts: { + work: { + allowFrom: ["123@s.whatsapp.net"], + }, + }, + }, + }, + } as OpenClawConfig; + + await handleWhatsAppAction( + { + action: "react", + chatJid: "123@s.whatsapp.net", + messageId: "msg1", + emoji: "✅", + }, + cfg, + ); + + expect(sendReactionWhatsApp).toHaveBeenLastCalledWith("+123", "msg1", "✅", { + verbose: false, + fromMe: undefined, + participant: undefined, + accountId: "work", + }); + }); }); diff --git a/src/agents/tools/whatsapp-actions.ts b/src/agents/tools/whatsapp-actions.ts index 780ea85b29a..b2da3820797 100644 --- a/src/agents/tools/whatsapp-actions.ts +++ b/src/agents/tools/whatsapp-actions.ts @@ -1,8 +1,8 @@ import type { AgentToolResult } from "@mariozechner/pi-agent-core"; import type { OpenClawConfig } from "../../config/config.js"; import { sendReactionWhatsApp } from "../../web/outbound.js"; -import { resolveWhatsAppOutboundTarget } from "../../whatsapp/resolve-outbound-target.js"; import { createActionGate, jsonResult, readReactionParams, readStringParam } from "./common.js"; +import { resolveAuthorizedWhatsAppOutboundTarget } from "./whatsapp-target-auth.js"; export async function handleWhatsAppAction( params: Record, @@ -25,28 +25,20 @@ export async function handleWhatsAppAction( const fromMeRaw = params.fromMe; const fromMe = typeof fromMeRaw === "boolean" ? fromMeRaw : undefined; - // Validate chatJid against the configured allowFrom list before sending. - // Per-account allowFrom takes precedence over the channel-level allowFrom. - const whatsappCfg = cfg.channels?.whatsapp; - const accountCfg = accountId ? whatsappCfg?.accounts?.[accountId] : undefined; - const allowFrom = accountCfg?.allowFrom ?? whatsappCfg?.allowFrom; - const resolution = resolveWhatsAppOutboundTarget({ - to: chatJid, - allowFrom: allowFrom ?? [], - mode: "implicit", + // Resolve account + allowFrom via shared account logic so auth and routing stay aligned. + const resolved = resolveAuthorizedWhatsAppOutboundTarget({ + cfg, + chatJid, + accountId, + actionLabel: "reaction", }); - if (!resolution.ok) { - throw new Error( - `WhatsApp reaction blocked: chatJid "${chatJid}" is not in the configured allowFrom list.`, - ); - } const resolvedEmoji = remove ? "" : emoji; - await sendReactionWhatsApp(resolution.to, messageId, resolvedEmoji, { + await sendReactionWhatsApp(resolved.to, messageId, resolvedEmoji, { verbose: false, fromMe, participant: participant ?? undefined, - accountId: accountId ?? undefined, + accountId: resolved.accountId, }); if (!remove && !isEmpty) { return jsonResult({ ok: true, added: emoji }); diff --git a/src/agents/tools/whatsapp-target-auth.ts b/src/agents/tools/whatsapp-target-auth.ts new file mode 100644 index 00000000000..b6f4da57ccf --- /dev/null +++ b/src/agents/tools/whatsapp-target-auth.ts @@ -0,0 +1,27 @@ +import type { OpenClawConfig } from "../../config/config.js"; +import { resolveWhatsAppAccount } from "../../web/accounts.js"; +import { resolveWhatsAppOutboundTarget } from "../../whatsapp/resolve-outbound-target.js"; +import { ToolAuthorizationError } from "./common.js"; + +export function resolveAuthorizedWhatsAppOutboundTarget(params: { + cfg: OpenClawConfig; + chatJid: string; + accountId?: string; + actionLabel: string; +}): { to: string; accountId: string } { + const account = resolveWhatsAppAccount({ + cfg: params.cfg, + accountId: params.accountId, + }); + const resolution = resolveWhatsAppOutboundTarget({ + to: params.chatJid, + allowFrom: account.allowFrom ?? [], + mode: "implicit", + }); + if (!resolution.ok) { + throw new ToolAuthorizationError( + `WhatsApp ${params.actionLabel} blocked: chatJid "${params.chatJid}" is not in the configured allowFrom list for account "${account.accountId}".`, + ); + } + return { to: resolution.to, accountId: account.accountId }; +} diff --git a/src/gateway/tools-invoke-http.test.ts b/src/gateway/tools-invoke-http.test.ts index 2a631f9c330..648b80a1a17 100644 --- a/src/gateway/tools-invoke-http.test.ts +++ b/src/gateway/tools-invoke-http.test.ts @@ -57,6 +57,12 @@ vi.mock("../agents/openclaw-tools.js", () => { err.name = "ToolInputError"; return err; }; + const toolAuthorizationError = (message: string) => { + const err = new Error(message) as Error & { status?: number }; + err.name = "ToolAuthorizationError"; + err.status = 403; + return err; + }; const tools = [ { @@ -101,6 +107,9 @@ vi.mock("../agents/openclaw-tools.js", () => { if (mode === "input") { throw toolInputError("mode invalid"); } + if (mode === "auth") { + throw toolAuthorizationError("mode forbidden"); + } if (mode === "crash") { throw new Error("boom"); } @@ -453,7 +462,7 @@ describe("POST /tools/invoke", () => { expect(resMain.status).toBe(200); }); - it("maps tool input errors to 400 and unexpected execution errors to 500", async () => { + it("maps tool input/auth errors to 400/403 and unexpected execution errors to 500", async () => { cfg = { ...cfg, agents: { @@ -472,6 +481,17 @@ describe("POST /tools/invoke", () => { expect(inputBody.error?.type).toBe("tool_error"); expect(inputBody.error?.message).toBe("mode invalid"); + const authRes = await invokeToolAuthed({ + tool: "tools_invoke_test", + args: { mode: "auth" }, + sessionKey: "main", + }); + expect(authRes.status).toBe(403); + const authBody = await authRes.json(); + expect(authBody.ok).toBe(false); + expect(authBody.error?.type).toBe("tool_error"); + expect(authBody.error?.message).toBe("mode forbidden"); + const crashRes = await invokeToolAuthed({ tool: "tools_invoke_test", args: { mode: "crash" }, diff --git a/src/gateway/tools-invoke-http.ts b/src/gateway/tools-invoke-http.ts index 5e5c6db2c34..0be53d5fc4e 100644 --- a/src/gateway/tools-invoke-http.ts +++ b/src/gateway/tools-invoke-http.ts @@ -112,16 +112,23 @@ function getErrorMessage(err: unknown): string { return String(err); } -function isToolInputError(err: unknown): boolean { +function resolveToolInputErrorStatus(err: unknown): number | null { if (err instanceof ToolInputError) { - return true; + const status = (err as { status?: unknown }).status; + return typeof status === "number" ? status : 400; } - return ( - typeof err === "object" && - err !== null && - "name" in err && - (err as { name?: unknown }).name === "ToolInputError" - ); + if (typeof err !== "object" || err === null || !("name" in err)) { + return null; + } + const name = (err as { name?: unknown }).name; + if (name !== "ToolInputError" && name !== "ToolAuthorizationError") { + return null; + } + const status = (err as { status?: unknown }).status; + if (typeof status === "number") { + return status; + } + return name === "ToolAuthorizationError" ? 403 : 400; } export async function handleToolsInvokeHttpRequest( @@ -308,8 +315,9 @@ export async function handleToolsInvokeHttpRequest( const result = await (tool as any).execute?.(`http-${Date.now()}`, toolArgs); sendJson(res, 200, { ok: true, result }); } catch (err) { - if (isToolInputError(err)) { - sendJson(res, 400, { + const inputStatus = resolveToolInputErrorStatus(err); + if (inputStatus !== null) { + sendJson(res, inputStatus, { ok: false, error: { type: "tool_error", message: getErrorMessage(err) || "invalid tool arguments" }, });