mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-06 05:50:43 +00:00
fix(bluebubbles): refuse sender-DM fallback when resolving group inbound chatGuid
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)
? <chat_id or chat_identifier>
: { 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.
This commit is contained in:
committed by
Peter Steinberger
parent
9f97e8c521
commit
4bd3d258cd
@@ -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();
|
||||
});
|
||||
});
|
||||
@@ -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}`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user