From 081e4be11e6382e4ed49b4753f9be137bece8fe7 Mon Sep 17 00:00:00 2001 From: Chris Zhang Date: Tue, 28 Apr 2026 17:06:38 +0800 Subject: [PATCH] fix(bluebubbles): address aisle re-review on routing-guard PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (`` or ``) 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. --- .../bluebubbles/src/monitor-processing.ts | 12 +++++- .../src/monitor-reply-cache.test.ts | 24 ++++++----- .../bluebubbles/src/monitor-reply-cache.ts | 40 ++++++++++++++++++- 3 files changed, 63 insertions(+), 13 deletions(-) diff --git a/extensions/bluebubbles/src/monitor-processing.ts b/extensions/bluebubbles/src/monitor-processing.ts index 61a2d303554..27f2cab92ed 100644 --- a/extensions/bluebubbles/src/monitor-processing.ts +++ b/extensions/bluebubbles/src/monitor-processing.ts @@ -629,7 +629,17 @@ function buildInboundHistorySnapshot(params: { } function sanitizeForLog(value: unknown, maxLen = 200): string { - const cleaned = String(value).replace(/[\r\n\t\p{C}]/gu, " "); + let cleaned = String(value).replace(/[\r\n\t\p{C}]/gu, " "); + // Redact common secret-bearing patterns before logging. BlueBubbles uses + // query-string auth (`?password=...`) by default, so attachment download + // failures and similar errors can carry the API password in the captured + // request URL; other libraries occasionally surface `Authorization: Bearer …` + // headers in error chains. Strip both before they reach the log sink (CWE-532). + cleaned = cleaned.replace( + /([?&](?:password|token|api[_-]?key|secret)=)[^&\s"]+/gi, + "$1", + ); + cleaned = cleaned.replace(/(authorization\s*:\s*(?:bearer|basic)\s+)[^\s"]+/gi, "$1"); return cleaned.length > maxLen ? cleaned.slice(0, maxLen) + "..." : cleaned; } diff --git a/extensions/bluebubbles/src/monitor-reply-cache.test.ts b/extensions/bluebubbles/src/monitor-reply-cache.test.ts index b92d03986fb..225691dd173 100644 --- a/extensions/bluebubbles/src/monitor-reply-cache.test.ts +++ b/extensions/bluebubbles/src/monitor-reply-cache.test.ts @@ -66,21 +66,25 @@ describe("resolveBlueBubblesMessageId chat-scoped short-id guard", () => { ).toThrow(/different chat/); }); - it("fails open when caller cannot supply any chat identifier", () => { - const entry = seedMessage({ + it("rejects empty chat context for privileged callers (fail-closed cross-chat scope)", () => { + seedMessage({ accountId: "default", messageId: "uuid-no-ctx", chatGuid: "iMessage;+;chat240698944142298252", }); - // Empty context means "caller could not derive any chat hint" (e.g. - // tool invocation with only messageId). Permit resolution; downstream - // API will still carry whatever chatGuid the call site provides. - const resolved = resolveBlueBubblesMessageId(entry.shortId, { - requireKnownShortId: true, - chatContext: {}, - }); - expect(resolved).toBe("uuid-no-ctx"); + // Empty context = caller could not derive any chat hint. The previous + // behavior (fail-open) let a short id resolve without a chat scope — + // but short ids are global across all chats, so an action call without + // chat context could silently apply to the wrong conversation. Now + // requireKnownShortId callers must pass at least one identifier + // (chatGuid / chatIdentifier / chatId). + expect(() => + resolveBlueBubblesMessageId("1", { + requireKnownShortId: true, + chatContext: {}, + }), + ).toThrow(/requires a chat scope/); }); it("falls back to chatIdentifier comparison when the caller has no chatGuid", () => { diff --git a/extensions/bluebubbles/src/monitor-reply-cache.ts b/extensions/bluebubbles/src/monitor-reply-cache.ts index 3b96c74e262..1acae61c31b 100644 --- a/extensions/bluebubbles/src/monitor-reply-cache.ts +++ b/extensions/bluebubbles/src/monitor-reply-cache.ts @@ -156,6 +156,20 @@ function describeChatForError(values: { return parts.length === 0 ? "" : parts.join(", "); } +function describeMessageIdForError(inputId: string, inputKind: "short" | "uuid"): string { + // Don't reflect the raw message id back into an error message that may end + // up in agent transcripts / tool results / log streams. Surface only the + // shape (numeric short id length range, or a UUID prefix) so callers can + // still tell which message id they typed (CWE-117 / CWE-200). + if (inputKind === "short") { + const len = inputId.length; + return ``; + } + // For UUID input, expose just an 8-char prefix; consumer can correlate + // against full GUID via the trace if needed. + return ``; +} + function buildCrossChatError( inputId: string, inputKind: "short" | "uuid", @@ -167,12 +181,23 @@ function buildCrossChatError( ? `Retry with the full message GUID to avoid cross-chat reactions/replies landing in the wrong conversation.` : `Retry with the correct chat target — even the full GUID cannot be reused across chats.`; return new Error( - `BlueBubbles message id "${inputId}" belongs to a different chat ` + + `BlueBubbles message id ${describeMessageIdForError(inputId, inputKind)} belongs to a different chat ` + `(${describeChatForError(cached)}) than the current call target ` + `(${describeChatForError(ctx)}). ${remediation}`, ); } +function hasChatScope(ctx?: BlueBubblesChatContext): boolean { + if (!ctx) { + return false; + } + return Boolean( + normalizeOptionalString(ctx.chatGuid) || + normalizeOptionalString(ctx.chatIdentifier) || + typeof ctx.chatId === "number", + ); +} + /** * Resolves a short message ID (e.g., "1", "2") to a full BlueBubbles GUID. * Returns the input unchanged if it's already a GUID or not found in the mapping. @@ -199,6 +224,17 @@ export function resolveBlueBubblesMessageId( // If it looks like a short ID (numeric), try to resolve it if (/^\d+$/.test(trimmed)) { + // Privileged callers (requireKnownShortId=true) MUST scope the resolution + // to a chat. Without a chat scope the cross-chat guard cannot detect when + // the short id belongs to a different chat than the action target — short + // ids are allocated from a single global counter across every account and + // chat, so an empty `chatContext={}` would otherwise let an action operate + // on a message in the wrong conversation (CWE-285). + if (opts?.requireKnownShortId && !hasChatScope(opts.chatContext)) { + throw new Error( + `BlueBubbles short message id "${describeMessageIdForError(trimmed, "short")}" requires a chat scope (chatGuid / chatIdentifier / chatId or a --to target).`, + ); + } const uuid = blueBubblesShortIdToUuid.get(trimmed); if (uuid) { if (opts?.chatContext) { @@ -211,7 +247,7 @@ export function resolveBlueBubblesMessageId( } if (opts?.requireKnownShortId) { throw new Error( - `BlueBubbles short message id "${trimmed}" is no longer available. Use MessageSidFull.`, + `BlueBubbles short message id ${describeMessageIdForError(trimmed, "short")} is no longer available. Use MessageSidFull.`, ); } return trimmed;