From c6dfa26f037977b82e91648bee47d99360d604d4 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 26 Feb 2026 01:01:58 +0100 Subject: [PATCH] refactor(signal): unify reaction auth flow and table-drive tests --- ...ends-tool-summaries-responseprefix.test.ts | 76 +++----- src/signal/monitor/event-handler.ts | 171 +++++++++++------- 2 files changed, 133 insertions(+), 114 deletions(-) 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 cc927fe2b36..a06d17d61d9 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,65 +378,49 @@ 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"], - }); + it.each([ + { + name: "blocks reaction notifications from unauthorized senders when dmPolicy is allowlist", + mode: "all" as const, + extra: { dmPolicy: "allowlist", allowFrom: ["+15550007777"] } as Record, + targetAuthor: "+15550002222", + shouldEnqueue: false, + }, + { + name: "blocks reaction notifications from unauthorized senders when dmPolicy is pairing", + mode: "own" as const, + extra: { + dmPolicy: "pairing", + allowFrom: [], + account: "+15550009999", + } as Record, + targetAuthor: "+15550009999", + shouldEnqueue: false, + }, + { + name: "allows reaction notifications for allowlisted senders when dmPolicy is allowlist", + mode: "all" as const, + extra: { dmPolicy: "allowlist", allowFrom: ["+15550001111"] } as Record, + targetAuthor: "+15550002222", + shouldEnqueue: true, + }, + ])("$name", async ({ mode, extra, targetAuthor, shouldEnqueue }) => { + setReactionNotificationConfig(mode, extra); await receiveSingleEnvelope({ ...makeBaseEnvelope(), reactionMessage: { emoji: "✅", - targetAuthor: "+15550002222", + targetAuthor, targetSentTimestamp: 2, }, }); const events = getDirectSignalEventsFor("+15550001111"); - expect(events.some((text) => text.includes("Signal reaction added"))).toBe(false); + expect(events.some((text) => text.includes("Signal reaction added"))).toBe(shouldEnqueue); 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 3cdd8cf5e9d..e87158d8d8d 100644 --- a/src/signal/monitor/event-handler.ts +++ b/src/signal/monitor/event-handler.ts @@ -49,9 +49,15 @@ import { resolveSignalPeerId, resolveSignalRecipient, resolveSignalSender, + type SignalSender, } from "../identity.js"; import { sendMessageSignal, sendReadReceiptSignal, sendTypingSignal } from "../send.js"; -import type { SignalEventHandlerDeps, SignalReceivePayload } from "./event-handler.types.js"; +import type { + SignalEnvelope, + SignalEventHandlerDeps, + SignalReactionMessage, + SignalReceivePayload, +} from "./event-handler.types.js"; import { renderSignalMentions } from "./mentions.js"; export function createSignalEventHandler(deps: SignalEventHandlerDeps) { const inboundDebounceMs = resolveInboundDebounceMs({ cfg: deps.cfg, channel: "signal" }); @@ -321,6 +327,85 @@ export function createSignalEventHandler(deps: SignalEventHandlerDeps) { }, }); + function handleReactionOnlyInbound(params: { + envelope: SignalEnvelope; + sender: SignalSender; + senderDisplay: string; + reaction: SignalReactionMessage; + hasBodyContent: boolean; + resolveAccessDecision: (isGroup: boolean) => { + decision: "allow" | "block" | "pairing"; + reason: string; + }; + }): boolean { + if (params.hasBodyContent) { + return false; + } + if (params.reaction.isRemove) { + return true; // Ignore reaction removals + } + const emojiLabel = params.reaction.emoji?.trim() || "emoji"; + const senderName = params.envelope.sourceName ?? params.senderDisplay; + logVerbose(`signal reaction: ${emojiLabel} from ${senderName}`); + const groupId = params.reaction.groupInfo?.groupId ?? undefined; + const groupName = params.reaction.groupInfo?.groupName ?? undefined; + const isGroup = Boolean(groupId); + const reactionAccess = params.resolveAccessDecision(isGroup); + if (reactionAccess.decision !== "allow") { + logVerbose( + `Blocked signal reaction sender ${params.senderDisplay} (${reactionAccess.reason})`, + ); + return true; + } + const targets = deps.resolveSignalReactionTargets(params.reaction); + const shouldNotify = deps.shouldEmitSignalReactionNotification({ + mode: deps.reactionMode, + account: deps.account, + targets, + sender: params.sender, + allowlist: deps.reactionAllowlist, + }); + if (!shouldNotify) { + return true; + } + + const senderPeerId = resolveSignalPeerId(params.sender); + const route = resolveAgentRoute({ + cfg: deps.cfg, + channel: "signal", + accountId: deps.accountId, + peer: { + kind: isGroup ? "group" : "direct", + id: isGroup ? (groupId ?? "unknown") : senderPeerId, + }, + }); + const groupLabel = isGroup ? `${groupName ?? "Signal Group"} id:${groupId}` : undefined; + const messageId = params.reaction.targetSentTimestamp + ? String(params.reaction.targetSentTimestamp) + : "unknown"; + const text = deps.buildSignalReactionSystemEventText({ + emojiLabel, + actorLabel: senderName, + messageId, + targetLabel: targets[0]?.display, + groupLabel, + }); + const senderId = formatSignalSenderId(params.sender); + const contextKey = [ + "signal", + "reaction", + "added", + messageId, + senderId, + emojiLabel, + groupId ?? "", + ] + .filter(Boolean) + .join(":"); + enqueueSystemEvent(text, { sessionKey: route.sessionKey, contextKey }); + return true; + } + return async (event: { event?: string; data?: string }) => { if (event.event !== "receive" || !event.data) { return; @@ -375,13 +460,13 @@ export function createSignalEventHandler(deps: SignalEventHandlerDeps) { 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 { effectiveAllowFrom: effectiveDmAllow, effectiveGroupAllowFrom: effectiveGroupAllow } = + resolveEffectiveAllowFromLists({ + allowFrom: deps.allowFrom, + groupAllowFrom: deps.groupAllowFrom, + storeAllowFrom, + dmPolicy: deps.dmPolicy, + }); const resolveAccessDecision = (isGroup: boolean) => resolveDmGroupAccessDecision({ isGroup, @@ -394,67 +479,17 @@ export function createSignalEventHandler(deps: SignalEventHandlerDeps) { 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 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, - account: deps.account, - targets, + if ( + reaction && + handleReactionOnlyInbound({ + envelope, sender, - allowlist: deps.reactionAllowlist, - }); - if (!shouldNotify) { - return; - } - - const senderPeerId = resolveSignalPeerId(sender); - const route = resolveAgentRoute({ - cfg: deps.cfg, - channel: "signal", - accountId: deps.accountId, - peer: { - kind: isGroup ? "group" : "direct", - id: isGroup ? (groupId ?? "unknown") : senderPeerId, - }, - }); - const groupLabel = isGroup ? `${groupName ?? "Signal Group"} id:${groupId}` : undefined; - const messageId = reaction.targetSentTimestamp - ? String(reaction.targetSentTimestamp) - : "unknown"; - const text = deps.buildSignalReactionSystemEventText({ - emojiLabel, - actorLabel: senderName, - messageId, - targetLabel: targets[0]?.display, - groupLabel, - }); - const senderId = formatSignalSenderId(sender); - const contextKey = [ - "signal", - "reaction", - "added", - messageId, - senderId, - emojiLabel, - groupId ?? "", - ] - .filter(Boolean) - .join(":"); - enqueueSystemEvent(text, { sessionKey: route.sessionKey, contextKey }); + senderDisplay, + reaction, + hasBodyContent, + resolveAccessDecision, + }) + ) { return; } if (!dataMessage) {