From 8dacafce7f48e9a3abc805437910a3ba85657ea9 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 15 Jan 2026 07:53:12 +0000 Subject: [PATCH] fix: harden whatsapp command auth --- CHANGELOG.md | 1 + docs/tools/slash-commands.md | 1 + src/auto-reply/command-auth.test.ts | 23 +++ src/auto-reply/command-auth.ts | 55 ++++--- .../reply/session-reset-group.test.ts | 83 +++++++++++ ...oup-chats-injects-history-replying.test.ts | 138 ++++++++++++++++++ src/web/auto-reply/monitor/group-gating.ts | 6 +- 7 files changed, 286 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6be86789a..c405788ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,7 @@ - Telegram: let control commands bypass per-chat sequentialization; always allow abort triggers. - Auto-reply: treat trailing `NO_REPLY` tokens as silent replies. - Config: prevent partial config writes from clobbering unrelated settings (base hash guard + merge patch for connection saves). +- WhatsApp: honor SenderE164 for owner command auth when SenderId is LID; let owner control commands bypass group mention gating. - Sessions: return deep clones (`structuredClone`) so cached session entries can't be mutated. (#934) — thanks @ronak-guliani. - Heartbeat: keep `updatedAt` monotonic when restoring heartbeat sessions. (#934) — thanks @ronak-guliani. - Agent: clear run context after CLI runs (`clearAgentRunContext`) to avoid runaway contexts. (#934) — thanks @ronak-guliani. diff --git a/docs/tools/slash-commands.md b/docs/tools/slash-commands.md index beeecf917..3157bdea7 100644 --- a/docs/tools/slash-commands.md +++ b/docs/tools/slash-commands.md @@ -91,6 +91,7 @@ Notes: - `/verbose` is meant for debugging and extra visibility; keep it **off** in normal use. - `/reasoning` (and `/verbose`) are risky in group settings: they may reveal internal reasoning or tool output you did not intend to expose. Prefer leaving them off, especially in group chats. - **Fast path:** command-only messages from allowlisted senders are handled immediately (bypass queue + model). +- **Group mention gating:** command-only messages from allowlisted senders bypass mention requirements. - **Inline shortcuts (allowlisted senders only):** certain commands also work when embedded in a normal message and are stripped before the model sees the remaining text. - Example: `hey /status` triggers a status reply, and the remaining text continues through the normal flow. - Currently: `/help`, `/commands`, `/status` (`/usage`), `/whoami` (`/id`). diff --git a/src/auto-reply/command-auth.test.ts b/src/auto-reply/command-auth.test.ts index abc770eb2..0b6cf0826 100644 --- a/src/auto-reply/command-auth.test.ts +++ b/src/auto-reply/command-auth.test.ts @@ -96,4 +96,27 @@ describe("resolveCommandAuthorization", () => { expect(auth.senderId).toBe("+123"); expect(auth.isAuthorizedSender).toBe(true); }); + + it("prefers SenderE164 when SenderId does not match allowFrom", () => { + const cfg = { + channels: { whatsapp: { allowFrom: ["+41796666864"] } }, + } as ClawdbotConfig; + + const ctx = { + Provider: "whatsapp", + Surface: "whatsapp", + From: "whatsapp:120363401234567890@g.us", + SenderId: "123@lid", + SenderE164: "+41796666864", + } as MsgContext; + + const auth = resolveCommandAuthorization({ + ctx, + cfg, + commandAuthorized: true, + }); + + expect(auth.senderId).toBe("+41796666864"); + expect(auth.isAuthorizedSender).toBe(true); + }); }); diff --git a/src/auto-reply/command-auth.ts b/src/auto-reply/command-auth.ts index e3509a465..e4ddd99fb 100644 --- a/src/auto-reply/command-auth.ts +++ b/src/auto-reply/command-auth.ts @@ -61,35 +61,49 @@ function normalizeAllowFromEntry(params: { cfg: ClawdbotConfig; accountId?: string | null; value: string; -}): string | undefined { - return formatAllowFromList({ +}): string[] { + const normalized = formatAllowFromList({ dock: params.dock, cfg: params.cfg, accountId: params.accountId, allowFrom: [params.value], - })[0]; + }); + return normalized.filter((entry) => entry.trim().length > 0); } -function resolveSenderId(params: { +function resolveSenderCandidates(params: { dock?: ChannelDock; + providerId?: ChannelId; cfg: ClawdbotConfig; accountId?: string | null; senderId?: string | null; senderE164?: string | null; from?: string | null; -}): string | undefined { +}): string[] { const { dock, cfg, accountId } = params; - - const senderCandidates = [params.senderId, params.senderE164, params.from] - .map((value) => (value ?? "").trim()) - .filter(Boolean); - - for (const sender of senderCandidates) { - const normalized = normalizeAllowFromEntry({ dock, cfg, accountId, value: sender }); - if (normalized) return normalized; + const candidates: string[] = []; + const pushCandidate = (value?: string | null) => { + const trimmed = (value ?? "").trim(); + if (!trimmed) return; + candidates.push(trimmed); + }; + if (params.providerId === "whatsapp") { + pushCandidate(params.senderE164); + pushCandidate(params.senderId); + } else { + pushCandidate(params.senderId); + pushCandidate(params.senderE164); } + pushCandidate(params.from); - return undefined; + const normalized: string[] = []; + for (const sender of candidates) { + const entries = normalizeAllowFromEntry({ dock, cfg, accountId, value: sender }); + for (const entry of entries) { + if (!normalized.includes(entry)) normalized.push(entry); + } + } + return normalized; } export function resolveCommandAuthorization(params: { @@ -122,25 +136,30 @@ export function resolveCommandAuthorization(params: { accountId: ctx.AccountId, value: to, }); - if (normalizedTo) ownerCandidates.push(normalizedTo); + if (normalizedTo.length > 0) ownerCandidates.push(...normalizedTo); } - const ownerList = ownerCandidates; + const ownerList = Array.from(new Set(ownerCandidates)); - const senderId = resolveSenderId({ + const senderCandidates = resolveSenderCandidates({ dock, + providerId, cfg, accountId: ctx.AccountId, senderId: ctx.SenderId, senderE164: ctx.SenderE164, from, }); + const matchedSender = ownerList.length + ? senderCandidates.find((candidate) => ownerList.includes(candidate)) + : undefined; + const senderId = matchedSender ?? senderCandidates[0]; const enforceOwner = Boolean(dock?.commands?.enforceOwnerForCommands); const isOwner = !enforceOwner || allowAll || ownerList.length === 0 || - (senderId ? ownerList.includes(senderId) : false); + Boolean(matchedSender); const isAuthorizedSender = commandAuthorized && isOwner; return { diff --git a/src/auto-reply/reply/session-reset-group.test.ts b/src/auto-reply/reply/session-reset-group.test.ts index 7cb89b1b1..ed08bd5a1 100644 --- a/src/auto-reply/reply/session-reset-group.test.ts +++ b/src/auto-reply/reply/session-reset-group.test.ts @@ -167,4 +167,87 @@ describe("initSessionState reset triggers in WhatsApp groups", () => { expect(result.sessionId).not.toBe(existingSessionId); expect(result.bodyStripped).toBe(""); }); + + it("Reset trigger /new works when SenderId is LID but SenderE164 is authorized", async () => { + const storePath = await createStorePath("clawdbot-group-reset-lid-"); + const sessionKey = "agent:main:whatsapp:group:120363406150318674@g.us"; + const existingSessionId = "existing-session-123"; + await seedSessionStore({ + storePath, + sessionKey, + sessionId: existingSessionId, + }); + + const cfg = makeCfg({ + storePath, + allowFrom: ["+41796666864"], + }); + + const groupMessageCtx = { + Body: `[WhatsApp 120363406150318674@g.us 2026-01-13T07:45Z] Owner: /new\n[from: Owner (+41796666864)]`, + RawBody: "/new", + CommandBody: "/new", + From: "120363406150318674@g.us", + To: "+41779241027", + ChatType: "group", + SessionKey: sessionKey, + Provider: "whatsapp", + Surface: "whatsapp", + SenderName: "Owner", + SenderE164: "+41796666864", + SenderId: "123@lid", + }; + + const result = await initSessionState({ + ctx: groupMessageCtx, + cfg, + commandAuthorized: true, + }); + + expect(result.triggerBodyNormalized).toBe("/new"); + expect(result.isNewSession).toBe(true); + expect(result.sessionId).not.toBe(existingSessionId); + expect(result.bodyStripped).toBe(""); + }); + + it("Reset trigger /new blocked when SenderId is LID but SenderE164 is unauthorized", async () => { + const storePath = await createStorePath("clawdbot-group-reset-lid-unauth-"); + const sessionKey = "agent:main:whatsapp:group:120363406150318674@g.us"; + const existingSessionId = "existing-session-123"; + await seedSessionStore({ + storePath, + sessionKey, + sessionId: existingSessionId, + }); + + const cfg = makeCfg({ + storePath, + allowFrom: ["+41796666864"], + }); + + const groupMessageCtx = { + Body: `[WhatsApp 120363406150318674@g.us 2026-01-13T07:45Z] Other: /new\n[from: Other (+1555123456)]`, + RawBody: "/new", + CommandBody: "/new", + From: "120363406150318674@g.us", + To: "+41779241027", + ChatType: "group", + SessionKey: sessionKey, + Provider: "whatsapp", + Surface: "whatsapp", + SenderName: "Other", + SenderE164: "+1555123456", + SenderId: "123@lid", + }; + + const result = await initSessionState({ + ctx: groupMessageCtx, + cfg, + commandAuthorized: true, + }); + + expect(result.triggerBodyNormalized).toBe("/new"); + expect(result.sessionId).toBe(existingSessionId); + expect(result.isNewSession).toBe(false); + }); }); diff --git a/src/web/auto-reply.web-auto-reply.requires-mention-group-chats-injects-history-replying.test.ts b/src/web/auto-reply.web-auto-reply.requires-mention-group-chats-injects-history-replying.test.ts index 8b184f5a0..9965ca0b5 100644 --- a/src/web/auto-reply.web-auto-reply.requires-mention-group-chats-injects-history-replying.test.ts +++ b/src/web/auto-reply.web-auto-reply.requires-mention-group-chats-injects-history-replying.test.ts @@ -169,6 +169,144 @@ describe("web auto-reply", () => { expect(payload.Body).toContain("[from: Bob (+222)]"); }); + it("bypasses mention gating for owner /new in group chats", async () => { + const sendMedia = vi.fn(); + const reply = vi.fn().mockResolvedValue(undefined); + const sendComposing = vi.fn(); + const resolver = vi.fn().mockResolvedValue({ text: "ok" }); + + let capturedOnMessage: + | ((msg: import("./inbound.js").WebInboundMessage) => Promise) + | undefined; + const listenerFactory = async (opts: { + onMessage: (msg: import("./inbound.js").WebInboundMessage) => Promise; + }) => { + capturedOnMessage = opts.onMessage; + return { close: vi.fn() }; + }; + + setLoadConfigMock(() => ({ + channels: { + whatsapp: { + allowFrom: ["+111"], + }, + }, + })); + + await monitorWebChannel(false, listenerFactory, false, resolver); + expect(capturedOnMessage).toBeDefined(); + + await capturedOnMessage?.({ + body: "/new", + from: "123@g.us", + conversationId: "123@g.us", + chatId: "123@g.us", + chatType: "group", + to: "+2", + id: "g-new", + senderE164: "+111", + senderName: "Owner", + selfE164: "+999", + sendComposing, + reply, + sendMedia, + }); + + expect(resolver).toHaveBeenCalledTimes(1); + }); + + it("does not bypass mention gating for non-owner /new in group chats", async () => { + const sendMedia = vi.fn(); + const reply = vi.fn().mockResolvedValue(undefined); + const sendComposing = vi.fn(); + const resolver = vi.fn().mockResolvedValue({ text: "ok" }); + + let capturedOnMessage: + | ((msg: import("./inbound.js").WebInboundMessage) => Promise) + | undefined; + const listenerFactory = async (opts: { + onMessage: (msg: import("./inbound.js").WebInboundMessage) => Promise; + }) => { + capturedOnMessage = opts.onMessage; + return { close: vi.fn() }; + }; + + setLoadConfigMock(() => ({ + channels: { + whatsapp: { + allowFrom: ["+999"], + }, + }, + })); + + await monitorWebChannel(false, listenerFactory, false, resolver); + expect(capturedOnMessage).toBeDefined(); + + await capturedOnMessage?.({ + body: "/new", + from: "123@g.us", + conversationId: "123@g.us", + chatId: "123@g.us", + chatType: "group", + to: "+2", + id: "g-new-unauth", + senderE164: "+111", + senderName: "NotOwner", + selfE164: "+999", + sendComposing, + reply, + sendMedia, + }); + + expect(resolver).not.toHaveBeenCalled(); + }); + + it("bypasses mention gating for owner /status in group chats", async () => { + const sendMedia = vi.fn(); + const reply = vi.fn().mockResolvedValue(undefined); + const sendComposing = vi.fn(); + const resolver = vi.fn().mockResolvedValue({ text: "ok" }); + + let capturedOnMessage: + | ((msg: import("./inbound.js").WebInboundMessage) => Promise) + | undefined; + const listenerFactory = async (opts: { + onMessage: (msg: import("./inbound.js").WebInboundMessage) => Promise; + }) => { + capturedOnMessage = opts.onMessage; + return { close: vi.fn() }; + }; + + setLoadConfigMock(() => ({ + channels: { + whatsapp: { + allowFrom: ["+111"], + }, + }, + })); + + await monitorWebChannel(false, listenerFactory, false, resolver); + expect(capturedOnMessage).toBeDefined(); + + await capturedOnMessage?.({ + body: "/status", + from: "123@g.us", + conversationId: "123@g.us", + chatId: "123@g.us", + chatType: "group", + to: "+2", + id: "g-status", + senderE164: "+111", + senderName: "Owner", + selfE164: "+999", + sendComposing, + reply, + sendMedia, + }); + + expect(resolver).toHaveBeenCalledTimes(1); + }); + it("passes conversation id through as From for group replies", async () => { const sendMedia = vi.fn(); const reply = vi.fn().mockResolvedValue(undefined); diff --git a/src/web/auto-reply/monitor/group-gating.ts b/src/web/auto-reply/monitor/group-gating.ts index 7581afd1f..735ca76d6 100644 --- a/src/web/auto-reply/monitor/group-gating.ts +++ b/src/web/auto-reply/monitor/group-gating.ts @@ -1,10 +1,11 @@ +import { hasControlCommand } from "../../../auto-reply/command-detection.js"; import { parseActivationCommand } from "../../../auto-reply/group-activation.js"; import type { loadConfig } from "../../../config/config.js"; import { normalizeE164 } from "../../../utils.js"; import type { MentionConfig } from "../mentions.js"; import { buildMentionConfig, debugMention, resolveOwnerList } from "../mentions.js"; import type { WebInboundMsg } from "../types.js"; -import { isStatusCommand, stripMentionsForCommand } from "./commands.js"; +import { stripMentionsForCommand } from "./commands.js"; import { resolveGroupActivationFor, resolveGroupPolicyFor } from "./group-activation.js"; import { noteGroupMember } from "./group-members.js"; @@ -59,8 +60,7 @@ export function applyGroupGating(params: { ); const activationCommand = parseActivationCommand(commandBody); const owner = isOwnerSender(params.baseMentionConfig, params.msg); - const statusCommand = isStatusCommand(commandBody); - const shouldBypassMention = owner && (activationCommand.hasCommand || statusCommand); + const shouldBypassMention = owner && hasControlCommand(commandBody, params.cfg); if (activationCommand.hasCommand && !owner) { params.logVerbose(`Ignoring /activation from non-owner in group ${params.conversationId}`);