mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-06 07:30:43 +00:00
fix(bluebubbles): drop group reactions that arrive without any chat identifier
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.
This commit is contained in:
committed by
Peter Steinberger
parent
6ade320421
commit
07089f11c7
@@ -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({
|
||||
|
||||
@@ -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();
|
||||
|
||||
|
||||
Reference in New Issue
Block a user