From 9632b9bcf032c5f2280c3103961fde912ab1f920 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 21 Feb 2026 19:51:07 +0100 Subject: [PATCH] fix(security): fail closed parsed chat allowlist --- CHANGELOG.md | 1 + extensions/bluebubbles/src/monitor.test.ts | 120 ++++++++++++++++++++- src/imessage/targets.test.ts | 8 ++ src/plugin-sdk/allow-from.test.ts | 73 +++++++++++++ src/plugin-sdk/allow-from.ts | 2 +- 5 files changed, 199 insertions(+), 5 deletions(-) create mode 100644 src/plugin-sdk/allow-from.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index b8fa5ddc439..5965599babb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,7 @@ Docs: https://docs.openclaw.ai - Chat/Usage/TUI: strip synthetic inbound metadata blocks (including `Conversation info` and trailing `Untrusted context` channel metadata wrappers) from displayed conversation history so internal prompt context no longer leaks into user-visible logs. - Security/Exec: in non-default setups that manually add `sort` to `tools.exec.safeBins`, block `sort --compress-program` so allowlist-mode safe-bin checks cannot bypass approval. Thanks @tdjackey for reporting. - Security/Discord: add `openclaw security audit` warnings for name/tag-based Discord allowlist entries (DM allowlists, guild/channel `users`, and pairing-store entries), highlighting slug-collision risk while keeping name-based matching supported. Thanks @tdjackey for reporting. +- Security/BlueBubbles: make parsed chat allowlist checks fail closed when `allowFrom` is empty, restoring expected `pairing`/`allowlist` DM gating for BlueBubbles and blocking unauthorized DM/reaction processing when no allowlist entries are configured. This ships in the next npm release. Thanks @tdjackey for reporting. - Doctor/State integrity: only require/create the OAuth credentials directory when WhatsApp or pairing-backed channels are configured, and downgrade fresh-install missing-dir noise to an informational warning. - Agents/Sanitization: stop rewriting billing-shaped assistant text outside explicit error context so normal replies about billing/credits/payment are preserved across messaging channels. (#17834, fixes #11359) - Security/Agents: cap embedded Pi runner outer retry loop with a higher profile-aware dynamic limit (32-160 attempts) and return an explicit `retry_limit` error payload when retries never converge, preventing unbounded internal retry cycles (`GHSA-76m6-pj3w-v7mf`). diff --git a/extensions/bluebubbles/src/monitor.test.ts b/extensions/bluebubbles/src/monitor.test.ts index 1ebd9455830..69f416b8265 100644 --- a/extensions/bluebubbles/src/monitor.test.ts +++ b/extensions/bluebubbles/src/monitor.test.ts @@ -1017,9 +1017,86 @@ describe("BlueBubbles webhook monitor", () => { expect(mockDispatchReplyWithBufferedBlockDispatcher).not.toHaveBeenCalled(); }); + it("blocks DM when dmPolicy=allowlist and allowFrom is empty", async () => { + const account = createMockAccount({ + dmPolicy: "allowlist", + allowFrom: [], + }); + const config: OpenClawConfig = {}; + const core = createMockRuntime(); + setBlueBubblesRuntime(core); + + unregister = registerBlueBubblesWebhookTarget({ + account, + config, + runtime: { log: vi.fn(), error: vi.fn() }, + core, + path: "/bluebubbles-webhook", + }); + + const payload = { + type: "new-message", + data: { + text: "hello from blocked sender", + handle: { address: "+15551234567" }, + isGroup: false, + isFromMe: false, + guid: "msg-1", + date: Date.now(), + }, + }; + + const req = createMockRequest("POST", "/bluebubbles-webhook", payload); + const res = createMockResponse(); + + await handleBlueBubblesWebhookRequest(req, res); + await flushAsync(); + + expect(res.statusCode).toBe(200); + expect(mockDispatchReplyWithBufferedBlockDispatcher).not.toHaveBeenCalled(); + expect(mockUpsertPairingRequest).not.toHaveBeenCalled(); + }); + + it("triggers pairing flow for unknown sender when dmPolicy=pairing and allowFrom is empty", async () => { + const account = createMockAccount({ + dmPolicy: "pairing", + allowFrom: [], + }); + const config: OpenClawConfig = {}; + const core = createMockRuntime(); + setBlueBubblesRuntime(core); + + unregister = registerBlueBubblesWebhookTarget({ + account, + config, + runtime: { log: vi.fn(), error: vi.fn() }, + core, + path: "/bluebubbles-webhook", + }); + + const payload = { + type: "new-message", + data: { + text: "hello", + handle: { address: "+15551234567" }, + isGroup: false, + isFromMe: false, + guid: "msg-1", + date: Date.now(), + }, + }; + + const req = createMockRequest("POST", "/bluebubbles-webhook", payload); + const res = createMockResponse(); + + await handleBlueBubblesWebhookRequest(req, res); + await flushAsync(); + + expect(mockUpsertPairingRequest).toHaveBeenCalled(); + expect(mockDispatchReplyWithBufferedBlockDispatcher).not.toHaveBeenCalled(); + }); + it("triggers pairing flow for unknown sender when dmPolicy=pairing", async () => { - // Note: empty allowFrom = allow all. To trigger pairing, we need a non-empty - // allowlist that doesn't include the sender const account = createMockAccount({ dmPolicy: "pairing", allowFrom: ["+15559999999"], // Different number than sender @@ -1061,8 +1138,6 @@ describe("BlueBubbles webhook monitor", () => { it("does not resend pairing reply when request already exists", async () => { mockUpsertPairingRequest.mockResolvedValue({ code: "TESTCODE", created: false }); - // Note: empty allowFrom = allow all. To trigger pairing, we need a non-empty - // allowlist that doesn't include the sender const account = createMockAccount({ dmPolicy: "pairing", allowFrom: ["+15559999999"], // Different number than sender @@ -2627,6 +2702,43 @@ describe("BlueBubbles webhook monitor", () => { }); describe("reaction events", () => { + it("drops DM reactions when dmPolicy=pairing and allowFrom is empty", async () => { + mockEnqueueSystemEvent.mockClear(); + + const account = createMockAccount({ dmPolicy: "pairing", allowFrom: [] }); + const config: OpenClawConfig = {}; + const core = createMockRuntime(); + setBlueBubblesRuntime(core); + + unregister = registerBlueBubblesWebhookTarget({ + account, + config, + runtime: { log: vi.fn(), error: vi.fn() }, + core, + path: "/bluebubbles-webhook", + }); + + const payload = { + type: "message-reaction", + data: { + handle: { address: "+15551234567" }, + isGroup: false, + isFromMe: false, + associatedMessageGuid: "msg-original-123", + associatedMessageType: 2000, + date: Date.now(), + }, + }; + + const req = createMockRequest("POST", "/bluebubbles-webhook", payload); + const res = createMockResponse(); + + await handleBlueBubblesWebhookRequest(req, res); + await flushAsync(); + + expect(mockEnqueueSystemEvent).not.toHaveBeenCalled(); + }); + it("enqueues system event for reaction added", async () => { mockEnqueueSystemEvent.mockClear(); diff --git a/src/imessage/targets.test.ts b/src/imessage/targets.test.ts index 217b0ea6732..afafb6d8260 100644 --- a/src/imessage/targets.test.ts +++ b/src/imessage/targets.test.ts @@ -71,6 +71,14 @@ describe("imessage targets", () => { expect(ok).toBe(true); }); + it("denies when allowFrom is empty", () => { + const ok = isAllowedIMessageSender({ + allowFrom: [], + sender: "+1555", + }); + expect(ok).toBe(false); + }); + it("formats chat targets", () => { expect(formatIMessageChatTarget(42)).toBe("chat_id:42"); expect(formatIMessageChatTarget(undefined)).toBe(""); diff --git a/src/plugin-sdk/allow-from.test.ts b/src/plugin-sdk/allow-from.test.ts new file mode 100644 index 00000000000..cc69376c5fe --- /dev/null +++ b/src/plugin-sdk/allow-from.test.ts @@ -0,0 +1,73 @@ +import { describe, expect, it } from "vitest"; +import { isAllowedParsedChatSender } from "./allow-from.js"; + +function parseAllowTarget( + entry: string, +): + | { kind: "chat_id"; chatId: number } + | { kind: "chat_guid"; chatGuid: string } + | { kind: "chat_identifier"; chatIdentifier: string } + | { kind: "handle"; handle: string } { + const trimmed = entry.trim(); + const lower = trimmed.toLowerCase(); + if (lower.startsWith("chat_id:")) { + return { kind: "chat_id", chatId: Number.parseInt(trimmed.slice("chat_id:".length), 10) }; + } + if (lower.startsWith("chat_guid:")) { + return { kind: "chat_guid", chatGuid: trimmed.slice("chat_guid:".length) }; + } + if (lower.startsWith("chat_identifier:")) { + return { + kind: "chat_identifier", + chatIdentifier: trimmed.slice("chat_identifier:".length), + }; + } + return { kind: "handle", handle: lower }; +} + +describe("isAllowedParsedChatSender", () => { + it("denies when allowFrom is empty", () => { + const allowed = isAllowedParsedChatSender({ + allowFrom: [], + sender: "+15551234567", + normalizeSender: (sender) => sender, + parseAllowTarget, + }); + + expect(allowed).toBe(false); + }); + + it("allows wildcard entries", () => { + const allowed = isAllowedParsedChatSender({ + allowFrom: ["*"], + sender: "user@example.com", + normalizeSender: (sender) => sender.toLowerCase(), + parseAllowTarget, + }); + + expect(allowed).toBe(true); + }); + + it("matches normalized handles", () => { + const allowed = isAllowedParsedChatSender({ + allowFrom: ["User@Example.com"], + sender: "user@example.com", + normalizeSender: (sender) => sender.toLowerCase(), + parseAllowTarget, + }); + + expect(allowed).toBe(true); + }); + + it("matches chat IDs when provided", () => { + const allowed = isAllowedParsedChatSender({ + allowFrom: ["chat_id:42"], + sender: "+15551234567", + chatId: 42, + normalizeSender: (sender) => sender, + parseAllowTarget, + }); + + expect(allowed).toBe(true); + }); +}); diff --git a/src/plugin-sdk/allow-from.ts b/src/plugin-sdk/allow-from.ts index c349caa017e..39ef277876a 100644 --- a/src/plugin-sdk/allow-from.ts +++ b/src/plugin-sdk/allow-from.ts @@ -26,7 +26,7 @@ export function isAllowedParsedChatSender }): boolean { const allowFrom = params.allowFrom.map((entry) => String(entry).trim()); if (allowFrom.length === 0) { - return true; + return false; } if (allowFrom.includes("*")) { return true;