fix(security): centralize WhatsApp outbound auth and return 403 tool auth errors

This commit is contained in:
Peter Steinberger
2026-02-21 14:30:53 +01:00
parent f64d5ddf60
commit 10b8839a82
6 changed files with 165 additions and 39 deletions

View File

@@ -24,7 +24,7 @@ export type ActionGate<T extends Record<string, boolean | undefined>> = (
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<T extends Record<string, boolean | undefined>>(
actions: T | undefined,
): ActionGate<T> {

View File

@@ -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",
});
});
});

View File

@@ -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<string, unknown>,
@@ -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 });

View File

@@ -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 };
}

View File

@@ -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" },

View File

@@ -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" },
});