From 07089f11c73cd70e72b8a65a27f8b76f47775246 Mon Sep 17 00:00:00 2001 From: Chris Zhang Date: Sat, 25 Apr 2026 12:19:16 +0800 Subject: [PATCH] fix(bluebubbles): drop group reactions that arrive without any chat identifier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit processReaction's peerId calculation: const peerId = reaction.isGroup ? (chatGuid ?? chatIdentifier ?? (chatId ? String(chatId) : "group")) : reaction.senderId; reads as "if it's a group with at least one chat hint, use that hint; otherwise fall through to either the literal string 'group' (group case) or the sender id (DM case)". Two failure modes hide here: 1. BlueBubbles fires a `message-reaction` event with `isGroup: true` but omits chatGuid AND chatId AND chatIdentifier — peerId becomes the literal "group" and resolveBlueBubblesConversationRoute synthesizes a session key unrelated to any real binding. The reaction surfaces in whatever session the binding fallback picks, never the right one. 2. The same payload arrives with isGroup misclassified as false (BB's group-flag inference relies on chatGuid, explicit isGroup, or participants > 2 — none of which are guaranteed for reaction events; monitor.webhook.test-helpers.ts even ships a default reaction fixture with no chatGuid and isGroup defaulted to false). peerId then becomes reaction.senderId and the event is enqueued into the sender's DM session — the group tapback shows up inside an unrelated 1:1 transcript Chris was looking at. Neither outcome is recoverable without a chat hint — without chatGuid, chatId, or chatIdentifier we cannot identify which group the reaction belongs to. Drop the event with a verbose-log and let the agent miss that reaction rather than route it incorrectly. DM reactions (which legitimately may arrive with no chat hint and only a sender) keep working because the guard is gated on `reaction.isGroup === true`. A latent risk remains: if BB ever sends an isGroup-misclassified-as-false payload, this guard does not catch it. That would require teaching normalize to surface group-flag confidence, which is a larger change left for follow-up. Tests (extensions/bluebubbles/src/monitor.test.ts): - Group reaction with no chat identifiers → not enqueued - Group reaction with at least one chat identifier → still enqueued (regression sentinel for the new guard) Local patch for upstream consideration. --- .../bluebubbles/src/monitor-processing.ts | 20 ++++++++ extensions/bluebubbles/src/monitor.test.ts | 50 +++++++++++++++++++ 2 files changed, 70 insertions(+) diff --git a/extensions/bluebubbles/src/monitor-processing.ts b/extensions/bluebubbles/src/monitor-processing.ts index d2b98befdaa..50c6cadeb81 100644 --- a/extensions/bluebubbles/src/monitor-processing.ts +++ b/extensions/bluebubbles/src/monitor-processing.ts @@ -1909,6 +1909,26 @@ export async function processReaction( return; } + // Group reaction with no chat identifiers cannot be routed safely. The + // peerId fallback below would degrade to the literal string "group", and + // resolveBlueBubblesConversationRoute would then synthesize a session key + // unrelated to any real binding — worse, an isGroup=false misclassification + // upstream would have routed this to the sender's DM session, surfacing + // a group tapback inside an unrelated 1:1 transcript. Drop+log instead. + if ( + reaction.isGroup && + !reaction.chatGuid && + reaction.chatId == null && + !reaction.chatIdentifier + ) { + logVerbose( + core, + runtime, + `dropping group reaction with no chat identifiers (senderId=${reaction.senderId} messageId=${reaction.messageId} action=${reaction.action})`, + ); + return; + } + const dmPolicy = account.config.dmPolicy ?? "pairing"; const groupPolicy = account.config.groupPolicy ?? "allowlist"; const storeAllowFrom = await readStoreAllowFromForDmPolicy({ diff --git a/extensions/bluebubbles/src/monitor.test.ts b/extensions/bluebubbles/src/monitor.test.ts index bb6164302f8..3c57352ddad 100644 --- a/extensions/bluebubbles/src/monitor.test.ts +++ b/extensions/bluebubbles/src/monitor.test.ts @@ -2233,6 +2233,56 @@ describe("BlueBubbles webhook monitor", () => { expect(mockEnqueueSystemEvent).not.toHaveBeenCalled(); }); + it("drops group reactions that arrive with no chat identifiers", async () => { + // Real-world failure mode: BlueBubbles fires a reaction webhook with + // isGroup=true but omits chatGuid AND chatId AND chatIdentifier. The + // legacy code falls peerId back to the literal string "group" and + // resolves a session key unrelated to any real binding; if isGroup + // had been misclassified as false the same payload would have been + // routed to the sender's DM session instead — surfacing a group + // tapback inside an unrelated 1:1 transcript. Either way the event + // cannot be routed correctly, so drop it. + mockEnqueueSystemEvent.mockClear(); + mockResolveRequireMention.mockReturnValue(false); + + setupWebhookTarget({ + account: createMockAccount({ groupPolicy: "open" }), + }); + + const payload = createTimestampedMessageReactionPayloadForTest({ + isGroup: true, + // chatGuid / chatId / chatIdentifier intentionally omitted + associatedMessageType: 2000, + handle: { address: "+15559999999" }, + }); + + await dispatchWebhookPayload(payload); + + expect(mockEnqueueSystemEvent).not.toHaveBeenCalled(); + }); + + it("still enqueues group reactions when at least one chat identifier is present", async () => { + // Sanity check: the drop guard must not fire when the webhook does + // include a chatGuid. + mockEnqueueSystemEvent.mockClear(); + mockResolveRequireMention.mockReturnValue(false); + + setupWebhookTarget({ + account: createMockAccount({ groupPolicy: "open" }), + }); + + const payload = createTimestampedMessageReactionPayloadForTest({ + isGroup: true, + chatGuid: "iMessage;+;chat-known-123", + associatedMessageType: 2000, + handle: { address: "+15559999999" }, + }); + + await dispatchWebhookPayload(payload); + + expect(mockEnqueueSystemEvent).toHaveBeenCalled(); + }); + it("maps reaction types to correct emojis", async () => { mockEnqueueSystemEvent.mockClear();