From 4bd3d258cdc09b46c84d4dd559661c3266cbd348 Mon Sep 17 00:00:00 2001 From: Chris Zhang Date: Sat, 25 Apr 2026 12:14:53 +0800 Subject: [PATCH] fix(bluebubbles): refuse sender-DM fallback when resolving group inbound chatGuid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a BlueBubbles inbound webhook arrives without `chatGuid`, processMessage falls back to `resolveChatGuidForTarget` to look it up. The previous fallback target was: isGroup && (chatId || chatIdentifier) ? : { kind: "handle", address: message.senderId } That `else` branch quietly covered two very different cases: 1. DM with no chatGuid — resolving via sender handle is correct, the chat IS the conversation with that handle. 2. **Group with no chatGuid AND no chatId AND no chatIdentifier** — resolving via sender handle yields *that sender's DM chatGuid*, then the rest of processMessage uses it for ack reactions, mark-read, outbound reply cache, typing indicators, and outboundTarget. Case 2 is reachable: `monitor.webhook.test-helpers.ts` ships a default `createMessageReactionPayloadForTest` payload with no chatGuid/chatId/ chatIdentifier and `isGroup` defaulted to `false`, mirroring real BlueBubbles reaction/tapback webhooks. When a group reaction or tapback arrives in that shape and isGroup is later corrected to true (or the message takes the same poisoned path), `chatGuidForActions` becomes the sender's DM chatGuid. The poisoned chatGuid then writes the outbound reply cache (line ~1395) with the wrong chat, defeating the cross-chat short-id guard added in 9912472289 — a later short id resolved against that cache cannot detect the mismatch and the agent's reaction/reply silently lands in the DM. Symptom Chris observed (recurring after 9912472289 baked): group messages getting reacted to from the agent's side show up in a DM transcript with that sender, attached to a message GUID the user can no longer locate in the DM. Extract the fallback target construction into `buildBlueBubblesInboundChatResolveTarget` so the rule is testable in isolation and the wrong fallback can never be reached again: - Group inbound + chatId present → `chat_id` - Group inbound + chatIdentifier present → `chat_identifier` - **Group inbound + neither → return null (caller skips chatGuid-dependent actions)** - DM inbound → `handle` (unchanged: the conversation IS that sender) processMessage now logs at verbose when the group case returns null instead of silently degrading to the sender's DM. Tests: extensions/bluebubbles/src/monitor-processing-chat-resolve.test.ts covers the eight branches (group with id, group with identifier, group preferring id, group with neither, blank/non-finite/null variants, DM, DM with chat_id present, DM with empty sender). Local patch for upstream consideration — pairs with the short-id chat guard landed in the previous commit. --- .../monitor-processing-chat-resolve.test.ts | 113 ++++++++++++++++++ .../bluebubbles/src/monitor-processing.ts | 66 ++++++++-- 2 files changed, 172 insertions(+), 7 deletions(-) create mode 100644 extensions/bluebubbles/src/monitor-processing-chat-resolve.test.ts diff --git a/extensions/bluebubbles/src/monitor-processing-chat-resolve.test.ts b/extensions/bluebubbles/src/monitor-processing-chat-resolve.test.ts new file mode 100644 index 00000000000..ecd5e57283c --- /dev/null +++ b/extensions/bluebubbles/src/monitor-processing-chat-resolve.test.ts @@ -0,0 +1,113 @@ +import { describe, expect, it } from "vitest"; +import { buildBlueBubblesInboundChatResolveTarget } from "./monitor-processing.js"; + +describe("buildBlueBubblesInboundChatResolveTarget", () => { + it("uses chat_id for group inbound when chatId is present", () => { + const target = buildBlueBubblesInboundChatResolveTarget({ + isGroup: true, + chatId: 42, + chatIdentifier: undefined, + senderId: "+15551234567", + }); + expect(target).toEqual({ kind: "chat_id", chatId: 42 }); + }); + + it("uses chat_identifier for group inbound when chatId missing but identifier present", () => { + const target = buildBlueBubblesInboundChatResolveTarget({ + isGroup: true, + chatId: undefined, + chatIdentifier: "iMessage;+;chat-abc", + senderId: "+15551234567", + }); + expect(target).toEqual({ + kind: "chat_identifier", + chatIdentifier: "iMessage;+;chat-abc", + }); + }); + + it("prefers chat_id over chat_identifier when both are present for a group", () => { + const target = buildBlueBubblesInboundChatResolveTarget({ + isGroup: true, + chatId: 7, + chatIdentifier: "iMessage;+;chat-abc", + senderId: "+15551234567", + }); + expect(target).toEqual({ kind: "chat_id", chatId: 7 }); + }); + + it("REFUSES sender-handle fallback for group inbound with no chat identifiers", () => { + // This is the candidate-4 regression: BlueBubbles webhooks for tapbacks + // and certain reaction/updated-message events arrive without chatGuid/ + // chatId/chatIdentifier. Falling through to { kind: "handle", + // address: senderId } would resolve the sender's DM chatGuid and + // poison every action keyed off it (ack reaction, mark-read, outbound + // reply cache), making group reactions land in DMs. + const target = buildBlueBubblesInboundChatResolveTarget({ + isGroup: true, + chatId: undefined, + chatIdentifier: undefined, + senderId: "+15551234567", + }); + expect(target).toBeNull(); + }); + + it("treats blank chatIdentifier as missing for group inbound", () => { + const target = buildBlueBubblesInboundChatResolveTarget({ + isGroup: true, + chatId: undefined, + chatIdentifier: " ", + senderId: "+15551234567", + }); + expect(target).toBeNull(); + }); + + it("treats non-finite chatId as missing for group inbound", () => { + const target = buildBlueBubblesInboundChatResolveTarget({ + isGroup: true, + chatId: Number.NaN, + chatIdentifier: undefined, + senderId: "+15551234567", + }); + expect(target).toBeNull(); + }); + + it("treats null chatId/chatIdentifier as missing for group inbound", () => { + const target = buildBlueBubblesInboundChatResolveTarget({ + isGroup: true, + chatId: null, + chatIdentifier: null, + senderId: "+15551234567", + }); + expect(target).toBeNull(); + }); + + it("uses sender handle for DM inbound (the chat IS the conversation with that sender)", () => { + const target = buildBlueBubblesInboundChatResolveTarget({ + isGroup: false, + chatId: undefined, + chatIdentifier: undefined, + senderId: "+15551234567", + }); + expect(target).toEqual({ kind: "handle", address: "+15551234567" }); + }); + + it("uses sender handle for DM inbound even when chatId is present (preserves prior behavior)", () => { + const target = buildBlueBubblesInboundChatResolveTarget({ + isGroup: false, + chatId: 99, + chatIdentifier: "iMessage;-;+15551234567", + senderId: "+15551234567", + }); + expect(target).toEqual({ kind: "handle", address: "+15551234567" }); + }); + + it("returns null for DM inbound with empty senderId", () => { + const target = buildBlueBubblesInboundChatResolveTarget({ + isGroup: false, + chatId: undefined, + chatIdentifier: undefined, + senderId: " ", + }); + expect(target).toBeNull(); + }); +}); diff --git a/extensions/bluebubbles/src/monitor-processing.ts b/extensions/bluebubbles/src/monitor-processing.ts index c36dcf3851b..d2b98befdaa 100644 --- a/extensions/bluebubbles/src/monitor-processing.ts +++ b/extensions/bluebubbles/src/monitor-processing.ts @@ -354,6 +354,52 @@ export function logVerbose( } } +export type BlueBubblesInboundChatResolveTarget = + | { readonly kind: "chat_id"; readonly chatId: number } + | { readonly kind: "chat_identifier"; readonly chatIdentifier: string } + | { readonly kind: "handle"; readonly address: string }; + +/** + * Builds the fallback target used to look up a chatGuid when an inbound + * webhook arrives without one. + * + * Critically, group inbounds that lack every chat identifier (chatGuid / + * chatId / chatIdentifier all missing) MUST NOT fall through to the + * sender's handle. Resolving a group via the sender handle yields that + * sender's DM chatGuid, which then poisons every downstream action keyed + * off it: ack reactions land in the DM, the read receipt marks the DM, + * and the outbound reply cache stores the wrong chat — so a later short + * id resolved against that cache cannot detect the cross-chat reuse and + * the agent's react/reply silently target the DM instead of the group. + * + * Returns null in that unresolvable group case so the caller can skip + * actions that need a chatGuid rather than acting on a wrong one. DMs + * always resolve via the sender handle (the chat is, by definition, the + * conversation with that handle). + */ +export function buildBlueBubblesInboundChatResolveTarget(params: { + isGroup: boolean; + chatId?: number | null; + chatIdentifier?: string | null; + senderId: string; +}): BlueBubblesInboundChatResolveTarget | null { + if (params.isGroup) { + if (typeof params.chatId === "number" && Number.isFinite(params.chatId)) { + return { kind: "chat_id", chatId: params.chatId }; + } + const trimmedIdentifier = params.chatIdentifier?.trim(); + if (trimmedIdentifier) { + return { kind: "chat_identifier", chatIdentifier: trimmedIdentifier }; + } + return null; + } + const trimmedSender = params.senderId.trim(); + if (!trimmedSender) { + return null; + } + return { kind: "handle", address: trimmedSender }; +} + function logGroupAllowlistHint(params: { runtime: BlueBubblesRuntimeEnv; reason: string; @@ -1295,13 +1341,13 @@ async function processMessageAfterDedupe( }); let chatGuidForActions = chatGuid; if (!chatGuidForActions && baseUrl && password) { - const resolveTarget = - isGroup && (chatId || chatIdentifier) - ? chatId - ? ({ kind: "chat_id", chatId } as const) - : ({ kind: "chat_identifier", chatIdentifier: chatIdentifier ?? "" } as const) - : ({ kind: "handle", address: message.senderId } as const); - if (resolveTarget.kind !== "chat_identifier" || resolveTarget.chatIdentifier) { + const resolveTarget = buildBlueBubblesInboundChatResolveTarget({ + isGroup, + chatId, + chatIdentifier, + senderId: message.senderId, + }); + if (resolveTarget) { chatGuidForActions = (await resolveChatGuidForTarget({ baseUrl, @@ -1309,6 +1355,12 @@ async function processMessageAfterDedupe( target: resolveTarget, allowPrivateNetwork: isPrivateNetworkOptInEnabled(account.config), })) ?? undefined; + } else { + logVerbose( + core, + runtime, + `cannot resolve chatGuid for group inbound (chatGuid/chatId/chatIdentifier all missing); senderId=${message.senderId}`, + ); } }