fix(security): fail closed parsed chat allowlist

This commit is contained in:
Peter Steinberger
2026-02-21 19:51:07 +01:00
parent 09d5f508b1
commit 9632b9bcf0
5 changed files with 199 additions and 5 deletions

View File

@@ -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`).

View File

@@ -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();

View File

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

View File

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

View File

@@ -26,7 +26,7 @@ export function isAllowedParsedChatSender<TParsed extends ParsedChatAllowTarget>
}): boolean {
const allowFrom = params.allowFrom.map((entry) => String(entry).trim());
if (allowFrom.length === 0) {
return true;
return false;
}
if (allowFrom.includes("*")) {
return true;