Three findings from the second pass:
1. **MEDIUM — Cross-chat short message ID guard bypassed on empty chat
context (CWE-285).** When `requireKnownShortId=true` and `chatContext`
was missing or `{}`, `resolveBlueBubblesMessageId` would still resolve
the short id. Short ids are allocated from a single global counter
across every account and chat, so an action call without a chat
scope could silently apply to the wrong conversation. Throw "requires
a chat scope" instead. The previous behavior was an explicit
"fail-open" choice with a comment acknowledging the risk; the
underlying assumption (downstream call carries chatGuid) does not
hold for every action handler. Test rewritten to expect fail-closed.
2. **LOW — Unsanitized messageId reflected in cross-chat guard error
(CWE-117 / CWE-200).** The thrown error embedded the raw inputId
(and the raw chatGuid / chatIdentifier from the cached entry until
the previous pass). Replace the inputId with a shape descriptor
(`<short:N-digit>` or `<uuid:prefix…>`) so cross-chat errors no
longer leak any concrete identifier. Combined with the chat
identifier redaction in describeChatForError (already in place),
the error is fully redacted.
3. **LOW — PII exposure via verbose logs (CWE-532).** Untrusted webhook
identifiers (senderId / messageId / action) were already passed
through `sanitizeForLog`, but the helper only stripped control
characters — it did not redact secrets such as `?password=` query
strings or `Authorization: Bearer …` headers that occasionally
bleed into error chains. Extend `sanitizeForLog` to redact those
patterns. All call sites benefit immediately.
Four findings on this PR, all addressed in this commit:
1. **Cross-chat guard bypass when ctx.chatGuid present but cached lacks chatGuid**
(CWE-697). Earlier `isCrossChatMismatch` gated chatIdentifier and chatId
fallback comparisons on `!ctxChatGuid`, which let any non-empty
ctx.chatGuid suppress the fallback checks when the cached entry happened
to lack chatGuid — letting a short id from chat A be reused while acting
in chat B. Rewrite the function so chatIdentifier/chatId comparisons
run independently based on availability on each side, not on whether
ctx.chatGuid happens to be present.
2. **Sensitive chat identifiers exposed via thrown cross-chat error**
(CWE-200). `describeChatForError` interpolated raw chatGuid /
chatIdentifier / chatId into the error message — these can leak phone
numbers / email addresses / chat GUIDs into agent transcripts, tool
results, remote channel deliveries, or third-party log aggregators.
Surface only the *shape* of the chat target with `=<redacted>` values.
3. **Group reaction drop-guard bypass via whitespace chatIdentifier**.
Earlier guard treated "" as missing but accepted " " / "\t". Trim
chatGuid/chatIdentifier before the missing-check so a webhook sender
supplying whitespace cannot satisfy the guard and have peerId degrade
to the literal "group".
4. **Log injection via webhook senderId/messageId in verbose log lines**
(CWE-117). Untrusted webhook fields were interpolated directly into
`logVerbose` calls without sanitization, allowing log forging if a
sender carried CR/LF/control bytes. Wrap with the existing
`sanitizeForLog()` helper at all such sites.
Test updates: monitor-reply-cache.test.ts cross-chat error assertions
now expect `chatGuid=<redacted>` instead of raw values.
resolveBlueBubblesOutboundSessionRoute classified all `chat_guid:`
prefixed targets as groups:
const isGroup =
parsed.kind === "chat_id" ||
parsed.kind === "chat_guid" ||
parsed.kind === "chat_identifier";
But BlueBubbles also encodes DM chatGuids in the same `chat_guid:`
form — they look like `iMessage;-;+15551234567` (the `;-;` separator
is the DM marker; groups use `;+;`). Treating those as groups gave
the same DM two different sessionKeys depending on how the caller
addressed it:
- handle form (`bluebubbles:imessage:+15551234567`)
→ peer.kind = "direct", from = `bluebubbles:+15551234567`
- chat_guid form (`bluebubbles:chat_guid:iMessage;-;+15551234567`)
→ peer.kind = "group", from = `group:iMessage;-;+15551234567`
When a bound DM session was looked up against the second form, no
binding matched and the outbound landed in a freshly-synthesized
"group" sessionKey — a degenerate session that the next inbound
message also failed to find, surfacing the conversation in the
wrong place.
Use resolveGroupFlagFromChatGuid (already used by monitor-normalize
to read the same marker for inbound webhooks) so both directions
agree on what counts as a group. Unknown chatGuid shapes still
fall back to "group" to preserve prior behavior — we never
silently downgrade a real group to direct.
Tests: extensions/bluebubbles/src/session-route.test.ts (new)
- chat_guid `;-;` → direct
- chat_guid `;+;` → group
- chat_guid with no recognizable marker → group (back-compat)
- handle target → direct
- chat_id / chat_identifier → group (unchanged)
- DM addressed two ways converges on the same peer kind
Local patch for upstream consideration. Latent bug introduced by
0f7cd59824 (BlueBubbles: move outbound session routing behind plugin
boundary), not commonly hit because most outbound DM call sites use
the handle form, but a real foot-gun for callers that pass the
chat_guid form.
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.
The cross-chat guard added in the prior commit (resolveBlueBubblesMessageId
with chatContext) only ran on numeric short ids — `if (/^\d+$/.test(trimmed))`.
Full GUID input fell through to `return trimmed` with no chat check.
Once the short-id guard started rejecting cross-chat reuses, agents would
retry the same call with the full GUID copied from history or a previous
tool result. That second attempt bypassed the guard entirely and the
group reaction landed in the DM anyway — exactly the symptom the prior
commit was meant to close.
Apply the same `isCrossChatMismatch` check to full GUID input. Cache miss
still falls through (callers may legitimately supply a fresh-from-the-wire
GUID the cache hasn't observed yet), but cache hits with a chat mismatch
throw with a remediation hint pointed at the chat target rather than at
the id format — telling an agent to "retry with the full GUID" makes no
sense when it already supplied one.
Tests (extensions/bluebubbles/src/monitor-reply-cache.test.ts):
- UUID + same chat → resolves
- UUID + different chat → throws (this is the regression)
- UUID + cache miss → passes through (preserves behavior for fresh GUIDs)
- UUID + empty chatContext → passes through (preserves prior behavior)
- UUID error message hints at the chat target, not the id format
- chatIdentifier fallback applies to UUID input too
Local patch for upstream consideration — completes the cross-chat guard
started in the prior commit so both id forms are protected symmetrically.
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.
BlueBubbles short message ids (numeric aliases like "1", "5" that agents
use instead of full GUIDs to save tokens) are allocated from a single
global counter across every account and every chat. Nothing in
resolveBlueBubblesMessageId verified that the resolved GUID was actually
in the chat the caller was acting on, so any time an agent reused or
mis-remembered a short id — especially common after a long group
conversation — the id could silently point at a different chat entirely.
Symptom Chris observed: reactions/tapbacks and quoted replies authored
inside a group would intermittently land in a DM, targeting an old
message the user could no longer see. Tool call looks successful, chat
archive shows a group reaction appearing in the DM transcript.
Add an optional chatContext parameter to resolveBlueBubblesMessageId
(chatGuid / chatIdentifier / chatId). When provided, look up the
cached reply entry for the resolved GUID and compare. A clear mismatch
(same identifier present on both sides, different values) throws with a
message that lists both chats and points at "use the full GUID", so the
agent fails fast and retries with a disambiguated id. Ambiguous cases
(either side missing all identifiers) pass through to preserve existing
behavior for callers that cannot supply chat hints. The comparison
mirrors resolveReplyContextFromCache so outbound and inbound paths agree
on scope.
Update every call site that resolves a short id for outbound BB traffic
to pass chatContext:
- extensions/bluebubbles/src/actions.ts: react, edit, unsend, reply
(build context from chat* params, then to/target, then the tool's
currentChannelId)
- extensions/bluebubbles/src/channel.ts sendText: derive context from
the `to` target
- extensions/bluebubbles/src/media-send.ts: same
- extensions/bluebubbles/src/monitor-processing.ts deliver path: pass
the chat already resolved for routing
Add buildBlueBubblesChatContextFromTarget to targets.ts so callers can
project a raw target string (`chat_guid:...`, `chat_id:42`,
`imessage:+1...`, bare handle) into the context shape.
Tests:
- extensions/bluebubbles/src/monitor-reply-cache.test.ts (new, 8 cases):
same-chat resolves, cross-chatGuid throws, ambiguous passes,
chatIdentifier fallback, chatId fallback, full GUID input bypasses,
error message identifies both chats, unknown short id still errors.
- extensions/bluebubbles/src/actions.test.ts: update the react short-id
assertion to verify chatContext now flows through.
Local patch for upstream consideration — same root cause affects every
BB user; plan is to open a separate upstream PR once this bakes locally.
