From 2aa7842adeedef423be7ce283a9144b9f1a0a669 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 26 Feb 2026 00:44:36 +0100 Subject: [PATCH] fix(signal): enforce auth before reaction notification enqueue --- CHANGELOG.md | 1 + ...ends-tool-summaries-responseprefix.test.ts | 59 ++++++++++++++ src/signal/monitor/event-handler.ts | 78 ++++++++++++------- 3 files changed, 110 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c8d520827bf..d5cbd2384e8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- Security/Signal: enforce DM/group authorization before reaction-only notification enqueue so unauthorized senders can no longer inject Signal reaction system events under `dmPolicy`/`groupPolicy`; reaction notifications now require channel access checks first. This ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting. - Security/macOS beta onboarding: remove Anthropic OAuth sign-in and the legacy `oauth.json` onboarding path that exposed the PKCE verifier via OAuth `state`; this impacted the macOS beta onboarding path only. Anthropic subscription auth is now setup-token-only and will ship in the next npm release (`2026.2.25`). Thanks @zdi-disclosures for reporting. - Security/Nextcloud Talk: drop replayed signed webhook events with persistent per-account replay dedupe across restarts, and reject unexpected webhook backend origins when account base URL is configured. Thanks @aristorechina for reporting. - Security/Gateway: harden `agents.files` path handling to block out-of-workspace symlink targets for `agents.files.get`/`agents.files.set`, keep in-workspace symlink targets supported, and add gateway regression coverage for both blocked escapes and allowed in-workspace symlinks. Thanks @tdjackey for reporting. diff --git a/src/signal/monitor.tool-result.sends-tool-summaries-responseprefix.test.ts b/src/signal/monitor.tool-result.sends-tool-summaries-responseprefix.test.ts index 429f9e3896c..cc927fe2b36 100644 --- a/src/signal/monitor.tool-result.sends-tool-summaries-responseprefix.test.ts +++ b/src/signal/monitor.tool-result.sends-tool-summaries-responseprefix.test.ts @@ -378,6 +378,65 @@ describe("monitorSignalProvider tool results", () => { expect(events.some((text) => text.includes("Signal reaction added"))).toBe(true); }); + it("blocks reaction notifications from unauthorized senders when dmPolicy is allowlist", async () => { + setReactionNotificationConfig("all", { + dmPolicy: "allowlist", + allowFrom: ["+15550007777"], + }); + await receiveSingleEnvelope({ + ...makeBaseEnvelope(), + reactionMessage: { + emoji: "✅", + targetAuthor: "+15550002222", + targetSentTimestamp: 2, + }, + }); + + const events = getDirectSignalEventsFor("+15550001111"); + expect(events.some((text) => text.includes("Signal reaction added"))).toBe(false); + expect(sendMock).not.toHaveBeenCalled(); + expect(upsertPairingRequestMock).not.toHaveBeenCalled(); + }); + + it("blocks reaction notifications from unauthorized senders when dmPolicy is pairing", async () => { + setReactionNotificationConfig("own", { + dmPolicy: "pairing", + allowFrom: [], + account: "+15550009999", + }); + await receiveSingleEnvelope({ + ...makeBaseEnvelope(), + reactionMessage: { + emoji: "✅", + targetAuthor: "+15550009999", + targetSentTimestamp: 2, + }, + }); + + const events = getDirectSignalEventsFor("+15550001111"); + expect(events.some((text) => text.includes("Signal reaction added"))).toBe(false); + expect(sendMock).not.toHaveBeenCalled(); + expect(upsertPairingRequestMock).not.toHaveBeenCalled(); + }); + + it("allows reaction notifications for allowlisted senders when dmPolicy is allowlist", async () => { + setReactionNotificationConfig("all", { + dmPolicy: "allowlist", + allowFrom: ["+15550001111"], + }); + await receiveSingleEnvelope({ + ...makeBaseEnvelope(), + reactionMessage: { + emoji: "✅", + targetAuthor: "+15550002222", + targetSentTimestamp: 2, + }, + }); + + const events = getDirectSignalEventsFor("+15550001111"); + expect(events.some((text) => text.includes("Signal reaction added"))).toBe(true); + }); + it("notifies on own reactions when target includes uuid + phone", async () => { setReactionNotificationConfig("own", { account: "+15550002222" }); await receiveSingleEnvelope({ diff --git a/src/signal/monitor/event-handler.ts b/src/signal/monitor/event-handler.ts index b095626ab46..3cdd8cf5e9d 100644 --- a/src/signal/monitor/event-handler.ts +++ b/src/signal/monitor/event-handler.ts @@ -36,6 +36,10 @@ import { upsertChannelPairingRequest, } from "../../pairing/pairing-store.js"; import { resolveAgentRoute } from "../../routing/resolve-route.js"; +import { + resolveDmGroupAccessDecision, + resolveEffectiveAllowFromLists, +} from "../../security/dm-policy-shared.js"; import { normalizeE164 } from "../../utils.js"; import { formatSignalPairingIdLine, @@ -366,15 +370,45 @@ export function createSignalEventHandler(deps: SignalEventHandlerDeps) { const quoteText = dataMessage?.quote?.text?.trim() ?? ""; const hasBodyContent = Boolean(messageText || quoteText) || Boolean(!reaction && dataMessage?.attachments?.length); + const senderDisplay = formatSignalSenderDisplay(sender); + const storeAllowFrom = + deps.dmPolicy === "allowlist" + ? [] + : await readChannelAllowFromStore("signal").catch(() => []); + const { effectiveAllowFrom: effectiveDmAllow } = resolveEffectiveAllowFromLists({ + allowFrom: deps.allowFrom, + groupAllowFrom: deps.groupAllowFrom, + storeAllowFrom, + dmPolicy: deps.dmPolicy, + }); + const effectiveGroupAllow = [...deps.groupAllowFrom, ...storeAllowFrom]; + const resolveAccessDecision = (isGroup: boolean) => + resolveDmGroupAccessDecision({ + isGroup, + dmPolicy: deps.dmPolicy, + groupPolicy: deps.groupPolicy, + effectiveAllowFrom: effectiveDmAllow, + effectiveGroupAllowFrom: effectiveGroupAllow, + isSenderAllowed: (allowFrom) => isSignalSenderAllowed(sender, allowFrom), + }); + const dmAccess = resolveAccessDecision(false); + const dmAllowed = dmAccess.decision === "allow"; if (reaction && !hasBodyContent) { if (reaction.isRemove) { return; } // Ignore reaction removals const emojiLabel = reaction.emoji?.trim() || "emoji"; - const senderDisplay = formatSignalSenderDisplay(sender); const senderName = envelope.sourceName ?? senderDisplay; logVerbose(`signal reaction: ${emojiLabel} from ${senderName}`); + const groupId = reaction.groupInfo?.groupId ?? undefined; + const groupName = reaction.groupInfo?.groupName ?? undefined; + const isGroup = Boolean(groupId); + const reactionAccess = resolveAccessDecision(isGroup); + if (reactionAccess.decision !== "allow") { + logVerbose(`Blocked signal reaction sender ${senderDisplay} (${reactionAccess.reason})`); + return; + } const targets = deps.resolveSignalReactionTargets(reaction); const shouldNotify = deps.shouldEmitSignalReactionNotification({ mode: deps.reactionMode, @@ -387,9 +421,6 @@ export function createSignalEventHandler(deps: SignalEventHandlerDeps) { return; } - const groupId = reaction.groupInfo?.groupId ?? undefined; - const groupName = reaction.groupInfo?.groupName ?? undefined; - const isGroup = Boolean(groupId); const senderPeerId = resolveSignalPeerId(sender); const route = resolveAgentRoute({ cfg: deps.cfg, @@ -430,7 +461,6 @@ export function createSignalEventHandler(deps: SignalEventHandlerDeps) { return; } - const senderDisplay = formatSignalSenderDisplay(sender); const senderRecipient = resolveSignalRecipient(sender); const senderPeerId = resolveSignalPeerId(sender); const senderAllowId = formatSignalSenderId(sender); @@ -441,20 +471,15 @@ export function createSignalEventHandler(deps: SignalEventHandlerDeps) { const groupId = dataMessage.groupInfo?.groupId ?? undefined; const groupName = dataMessage.groupInfo?.groupName ?? undefined; const isGroup = Boolean(groupId); - const storeAllowFrom = - deps.dmPolicy === "allowlist" - ? [] - : await readChannelAllowFromStore("signal").catch(() => []); - const effectiveDmAllow = [...deps.allowFrom, ...storeAllowFrom]; - const effectiveGroupAllow = [...deps.groupAllowFrom, ...storeAllowFrom]; - const dmAllowed = - deps.dmPolicy === "open" ? true : isSignalSenderAllowed(sender, effectiveDmAllow); if (!isGroup) { - if (deps.dmPolicy === "disabled") { + if (dmAccess.decision === "block") { + if (deps.dmPolicy !== "disabled") { + logVerbose(`Blocked signal sender ${senderDisplay} (dmPolicy=${deps.dmPolicy})`); + } return; } - if (!dmAllowed) { + if (dmAccess.decision === "pairing") { if (deps.dmPolicy === "pairing") { const senderId = senderAllowId; const { code, created } = await upsertChannelPairingRequest({ @@ -483,23 +508,20 @@ export function createSignalEventHandler(deps: SignalEventHandlerDeps) { logVerbose(`signal pairing reply failed for ${senderId}: ${String(err)}`); } } - } else { - logVerbose(`Blocked signal sender ${senderDisplay} (dmPolicy=${deps.dmPolicy})`); } return; } } - if (isGroup && deps.groupPolicy === "disabled") { - logVerbose("Blocked signal group message (groupPolicy: disabled)"); - return; - } - if (isGroup && deps.groupPolicy === "allowlist") { - if (effectiveGroupAllow.length === 0) { - logVerbose("Blocked signal group message (groupPolicy: allowlist, no groupAllowFrom)"); - return; - } - if (!isSignalSenderAllowed(sender, effectiveGroupAllow)) { - logVerbose(`Blocked signal group sender ${senderDisplay} (not in groupAllowFrom)`); + if (isGroup) { + const groupAccess = resolveAccessDecision(true); + if (groupAccess.decision !== "allow") { + if (groupAccess.reason === "groupPolicy=disabled") { + logVerbose("Blocked signal group message (groupPolicy: disabled)"); + } else if (groupAccess.reason === "groupPolicy=allowlist (empty allowlist)") { + logVerbose("Blocked signal group message (groupPolicy: allowlist, no groupAllowFrom)"); + } else { + logVerbose(`Blocked signal group sender ${senderDisplay} (not in groupAllowFrom)`); + } return; } }