From 81fd4d560a9aaf8becd4770a79441839912482f6 Mon Sep 17 00:00:00 2001 From: Chris Zhang Date: Tue, 28 Apr 2026 15:37:58 +0800 Subject: [PATCH] fix(bluebubbles): address aisle review on routing-guard PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 `=` 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=` instead of raw values. --- .../bluebubbles/src/monitor-processing.ts | 13 ++++-- .../src/monitor-reply-cache.test.ts | 12 +++-- .../bluebubbles/src/monitor-reply-cache.ts | 44 +++++++++---------- 3 files changed, 37 insertions(+), 32 deletions(-) diff --git a/extensions/bluebubbles/src/monitor-processing.ts b/extensions/bluebubbles/src/monitor-processing.ts index 50c6cadeb81..61a2d303554 100644 --- a/extensions/bluebubbles/src/monitor-processing.ts +++ b/extensions/bluebubbles/src/monitor-processing.ts @@ -1359,7 +1359,7 @@ async function processMessageAfterDedupe( logVerbose( core, runtime, - `cannot resolve chatGuid for group inbound (chatGuid/chatId/chatIdentifier all missing); senderId=${message.senderId}`, + `cannot resolve chatGuid for group inbound (chatGuid/chatId/chatIdentifier all missing); senderId=${sanitizeForLog(message.senderId)}`, ); } } @@ -1915,16 +1915,21 @@ export async function processReaction( // 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. + // Treat whitespace-only chatGuid/chatIdentifier as missing — a webhook + // sender that supplies " " or "\t" must not be able to satisfy the guard + // and have peerId degrade to the literal "group" anyway. + const trimmedReactionChatGuid = reaction.chatGuid?.trim(); + const trimmedReactionChatIdentifier = reaction.chatIdentifier?.trim(); if ( reaction.isGroup && - !reaction.chatGuid && + !trimmedReactionChatGuid && reaction.chatId == null && - !reaction.chatIdentifier + !trimmedReactionChatIdentifier ) { logVerbose( core, runtime, - `dropping group reaction with no chat identifiers (senderId=${reaction.senderId} messageId=${reaction.messageId} action=${reaction.action})`, + `dropping group reaction with no chat identifiers (senderId=${sanitizeForLog(reaction.senderId)} messageId=${sanitizeForLog(reaction.messageId)} action=${sanitizeForLog(reaction.action)})`, ); return; } diff --git a/extensions/bluebubbles/src/monitor-reply-cache.test.ts b/extensions/bluebubbles/src/monitor-reply-cache.test.ts index 64c769b0aeb..b92d03986fb 100644 --- a/extensions/bluebubbles/src/monitor-reply-cache.test.ts +++ b/extensions/bluebubbles/src/monitor-reply-cache.test.ts @@ -223,8 +223,10 @@ describe("resolveBlueBubblesMessageId chat-scoped short-id guard", () => { expect.fail("expected cross-chat guard to throw"); } catch (err) { const message = err instanceof Error ? err.message : String(err); - expect(message).toContain("iMessage;+;chat240698944142298252"); - expect(message).toContain("iMessage;-;+8618621181874"); + // Chat identifiers redacted in error message (PII / log-stream hardening). + expect(message).toContain("chatGuid="); + expect(message).not.toContain("iMessage;+;chat240698944142298252"); + expect(message).not.toContain("iMessage;-;+8618621181874"); expect(message).toContain("correct chat target"); expect(message).not.toContain("Retry with the full message GUID"); } @@ -264,8 +266,10 @@ describe("resolveBlueBubblesMessageId chat-scoped short-id guard", () => { expect.fail("expected cross-chat guard to throw"); } catch (err) { const message = err instanceof Error ? err.message : String(err); - expect(message).toContain("iMessage;+;chat240698944142298252"); - expect(message).toContain("iMessage;-;+8618621181874"); + // Chat identifiers redacted in error message (PII / log-stream hardening). + expect(message).toContain("chatGuid="); + expect(message).not.toContain("iMessage;+;chat240698944142298252"); + expect(message).not.toContain("iMessage;-;+8618621181874"); expect(message).toContain("full message GUID"); } }); diff --git a/extensions/bluebubbles/src/monitor-reply-cache.ts b/extensions/bluebubbles/src/monitor-reply-cache.ts index 334b5985003..3b96c74e262 100644 --- a/extensions/bluebubbles/src/monitor-reply-cache.ts +++ b/extensions/bluebubbles/src/monitor-reply-cache.ts @@ -111,31 +111,25 @@ function isCrossChatMismatch( cached: BlueBubblesReplyCacheEntry, ctx: BlueBubblesChatContext, ): boolean { + // Compare each identifier independently based on availability on both sides. + // Earlier versions gated chatIdentifier/chatId 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. const cachedChatGuid = normalizeOptionalString(cached.chatGuid); const ctxChatGuid = normalizeOptionalString(ctx.chatGuid); - if (cachedChatGuid && ctxChatGuid && cachedChatGuid !== ctxChatGuid) { - return true; + if (cachedChatGuid && ctxChatGuid) { + return cachedChatGuid !== ctxChatGuid; } const cachedChatIdentifier = normalizeOptionalString(cached.chatIdentifier); const ctxChatIdentifier = normalizeOptionalString(ctx.chatIdentifier); - if ( - !ctxChatGuid && - cachedChatIdentifier && - ctxChatIdentifier && - cachedChatIdentifier !== ctxChatIdentifier - ) { - return true; + if (cachedChatIdentifier && ctxChatIdentifier) { + return cachedChatIdentifier !== ctxChatIdentifier; } const cachedChatId = typeof cached.chatId === "number" ? cached.chatId : undefined; const ctxChatId = typeof ctx.chatId === "number" ? ctx.chatId : undefined; - if ( - !ctxChatGuid && - !ctxChatIdentifier && - cachedChatId !== undefined && - ctxChatId !== undefined && - cachedChatId !== ctxChatId - ) { - return true; + if (cachedChatId !== undefined && ctxChatId !== undefined) { + return cachedChatId !== ctxChatId; } return false; } @@ -145,17 +139,19 @@ function describeChatForError(values: { chatIdentifier?: string; chatId?: number; }): string { + // Surface only the *shape* of the chat target, never the raw identifier, + // to avoid leaking phone numbers / email addresses / chat GUIDs into + // error messages that may end up in agent transcripts, tool results, + // remote channel deliveries, or third-party log aggregators. const parts: string[] = []; - const guid = normalizeOptionalString(values.chatGuid); - if (guid) { - parts.push(`chatGuid=${guid}`); + if (normalizeOptionalString(values.chatGuid)) { + parts.push("chatGuid="); } - const identifier = normalizeOptionalString(values.chatIdentifier); - if (identifier) { - parts.push(`chatIdentifier=${identifier}`); + if (normalizeOptionalString(values.chatIdentifier)) { + parts.push("chatIdentifier="); } if (typeof values.chatId === "number") { - parts.push(`chatId=${values.chatId}`); + parts.push("chatId="); } return parts.length === 0 ? "" : parts.join(", "); }