fix: tighten BlueBubbles route identity hardening (#73235) (thanks @zqchris)

2026-05-06 05:30:42 +00:00 · 2026-04-28 19:11:39 +01:00
parent 081e4be11e
commit 5b1202e11e
4 changed files with 80 additions and 31 deletions
--- a/extensions/bluebubbles/src/monitor-processing-chat-resolve.test.ts
+++ b/extensions/bluebubbles/src/monitor-processing-chat-resolve.test.ts
@@ -1,5 +1,8 @@
 import { describe, expect, it } from "vitest";
-import { buildBlueBubblesInboundChatResolveTarget } from "./monitor-processing.js";
+import {
+  _sanitizeBlueBubblesLogValueForTest,
+  buildBlueBubblesInboundChatResolveTarget,
+} from "./monitor-processing.js";

 describe("buildBlueBubblesInboundChatResolveTarget", () => {
  it("uses chat_id for group inbound when chatId is present", () => {
@@ -111,3 +114,24 @@ describe("buildBlueBubblesInboundChatResolveTarget", () => {
    expect(target).toBeNull();
  });
 });
+
+describe("BlueBubbles monitor log sanitization", () => {
+  it("redacts BlueBubbles query auth and Authorization headers", () => {
+    const input =
+      "GET /api/v1/attachment?password=secret&guid=socket-secret&token=api-token Authorization: Bearer abc123";
+
+    const sanitized = _sanitizeBlueBubblesLogValueForTest(input);
+
+    expect(sanitized).toContain("password=<redacted>");
+    expect(sanitized).toContain("guid=<redacted>");
+    expect(sanitized).toContain("token=<redacted>");
+    expect(sanitized).toContain("Authorization: Bearer <redacted>");
+    expect(sanitized).not.toContain("secret");
+    expect(sanitized).not.toContain("api-token");
+    expect(sanitized).not.toContain("abc123");
+  });
+
+  it("strips control characters before logging", () => {
+    expect(_sanitizeBlueBubblesLogValueForTest("one\ntwo\tt\u0000hree")).toBe("one two t hree");
+  });
+});
--- a/extensions/bluebubbles/src/monitor-processing.ts
+++ b/extensions/bluebubbles/src/monitor-processing.ts
@@ -631,18 +631,21 @@ function buildInboundHistorySnapshot(params: {
 function sanitizeForLog(value: unknown, maxLen = 200): string {
  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).
+  // query-string auth (`?password=...`, `?guid=...`, or `?token=...`) 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,
+    /([?&](?:password|guid|token|api[_-]?key|secret)=)[^&\s"]+/gi,
    "$1<redacted>",
  );
  cleaned = cleaned.replace(/(authorization\s*:\s*(?:bearer|basic)\s+)[^\s"]+/gi, "$1<redacted>");
  return cleaned.length > maxLen ? cleaned.slice(0, maxLen) + "..." : cleaned;
 }

+export const _sanitizeBlueBubblesLogValueForTest = sanitizeForLog;
+
 /**
 * Signal object threaded through `processMessageAfterDedupe` so the outer
 * wrapper can distinguish "reply delivery failed silently" from "returned
@@ -810,7 +813,7 @@ async function processMessageAfterDedupe(
      logVerbose(
        core,
        runtime,
-        `attachment retry failed for msgId=${message.messageId}: ${String(err)}`,
+        `attachment retry failed for msgId=${sanitizeForLog(message.messageId)}: ${sanitizeForLog(err)}`,
      );
    }
  }
@@ -904,18 +907,22 @@ async function processMessageAfterDedupe(
  }

  if (isSelfChatMessage && hasBlueBubblesSelfChatCopy(selfChatLookup)) {
-    logVerbose(core, runtime, `drop: reflected self-chat duplicate sender=${message.senderId}`);
+    logVerbose(
+      core,
+      runtime,
+      `drop: reflected self-chat duplicate sender=${sanitizeForLog(message.senderId)}`,
+    );
    return;
  }

  if (!rawBody) {
-    logVerbose(core, runtime, `drop: empty text sender=${message.senderId}`);
+    logVerbose(core, runtime, `drop: empty text sender=${sanitizeForLog(message.senderId)}`);
    return;
  }
  logVerbose(
    core,
    runtime,
-    `msg sender=${message.senderId} group=${isGroup} textLen=${text.length} attachments=${attachments.length} chatGuid=${message.chatGuid ?? ""} chatId=${message.chatId ?? ""}`,
+    `msg sender=${sanitizeForLog(message.senderId)} group=${isGroup} textLen=${text.length} attachments=${attachments.length} chatGuid=${sanitizeForLog(message.chatGuid ?? "")} chatId=${sanitizeForLog(message.chatId ?? "")}`,
  );

  const dmPolicy = account.config.dmPolicy ?? "pairing";
@@ -1011,8 +1018,14 @@ async function processMessageAfterDedupe(
        senderIdLine: `Your BlueBubbles sender id: ${message.senderId}`,
        meta: { name: message.senderName },
        onCreated: () => {
-          runtime.log?.(`[bluebubbles] pairing request sender=${message.senderId} created=true`);
-          logVerbose(core, runtime, `bluebubbles pairing request sender=${message.senderId}`);
+          runtime.log?.(
+            `[bluebubbles] pairing request sender=${sanitizeForLog(message.senderId)} created=true`,
+          );
+          logVerbose(
+            core,
+            runtime,
+            `bluebubbles pairing request sender=${sanitizeForLog(message.senderId)}`,
+          );
        },
        sendPairingReply: async (text) => {
          await sendMessageBlueBubbles(message.senderId, text, {
@@ -1025,10 +1038,10 @@ async function processMessageAfterDedupe(
          logVerbose(
            core,
            runtime,
-            `bluebubbles pairing reply failed for ${message.senderId}: ${String(err)}`,
+            `bluebubbles pairing reply failed for ${sanitizeForLog(message.senderId)}: ${sanitizeForLog(err)}`,
          );
          runtime.error?.(
-            `[bluebubbles] pairing reply failed sender=${message.senderId}: ${String(err)}`,
+            `[bluebubbles] pairing reply failed sender=${sanitizeForLog(message.senderId)}: ${sanitizeForLog(err)}`,
          );
        },
      });
@@ -1159,7 +1172,7 @@ async function processMessageAfterDedupe(
      logVerbose(
        core,
        runtime,
-        `bluebubbles: participant fallback lookup failed chat=${peerId}: ${String(err)}`,
+        `bluebubbles: participant fallback lookup failed chat=${sanitizeForLog(peerId)}: ${sanitizeForLog(err)}`,
      );
    }
  }
@@ -1225,7 +1238,7 @@ async function processMessageAfterDedupe(
          logVerbose(
            core,
            runtime,
-            `attachment download failed guid=${attachment.guid} err=${String(err)}`,
+            `attachment download failed guid=${sanitizeForLog(attachment.guid)} err=${sanitizeForLog(err)}`,
          );
        }
      }
@@ -1410,7 +1423,7 @@ async function processMessageAfterDedupe(
            logVerbose(
              core,
              runtime,
-              `ack reaction failed chatGuid=${chatGuidForActions} msg=${ackMessageId}: ${String(err)}`,
+              `ack reaction failed chatGuid=${sanitizeForLog(chatGuidForActions)} msg=${sanitizeForLog(ackMessageId)}: ${sanitizeForLog(err)}`,
            );
            return false;
          },
@@ -1425,9 +1438,9 @@ async function processMessageAfterDedupe(
        cfg: config,
        accountId: account.accountId,
      });
-      logVerbose(core, runtime, `marked read chatGuid=${chatGuidForActions}`);
+      logVerbose(core, runtime, `marked read chatGuid=${sanitizeForLog(chatGuidForActions)}`);
    } catch (err) {
-      runtime.error?.(`[bluebubbles] mark read failed: ${String(err)}`);
+      runtime.error?.(`[bluebubbles] mark read failed: ${sanitizeForLog(err)}`);
    }
  } else if (!sendReadReceipts) {
    logVerbose(core, runtime, "mark read skipped (sendReadReceipts=false)");
@@ -1569,7 +1582,7 @@ async function processMessageAfterDedupe(
        logVerbose(
          core,
          runtime,
-          `history backfill failed for ${historyIdentifier}: ${String(err)} (retries left=${Math.max(remainingAttempts, 0)} next_in_ms=${nextBackoffMs})`,
+          `history backfill failed for ${sanitizeForLog(historyIdentifier)}: ${sanitizeForLog(err)} (retries left=${Math.max(remainingAttempts, 0)} next_in_ms=${nextBackoffMs})`,
        );
      }
    }
@@ -1660,7 +1673,7 @@ async function processMessageAfterDedupe(
        cfg: config,
        accountId: account.accountId,
      }).catch((err) => {
-        runtime.error?.(`[bluebubbles] typing restart failed: ${String(err)}`);
+        runtime.error?.(`[bluebubbles] typing restart failed: ${sanitizeForLog(err)}`);
      });
    }, typingRestartDelayMs);
  };
@@ -1686,7 +1699,7 @@ async function processMessageAfterDedupe(
              accountId: account.accountId,
            });
          } catch (err) {
-            runtime.error?.(`[bluebubbles] typing start failed: ${String(err)}`);
+            runtime.error?.(`[bluebubbles] typing start failed: ${sanitizeForLog(err)}`);
          }
        },
        onIdle: () => {
@@ -1848,7 +1861,7 @@ async function processMessageAfterDedupe(
          if (info.kind === "final") {
            dedupeSignal.deliveryFailed = true;
          }
-          runtime.error?.(`BlueBubbles ${info.kind} reply failed: ${String(err)}`);
+          runtime.error?.(`BlueBubbles ${info.kind} reply failed: ${sanitizeForLog(err)}`);
        },
      },
      replyOptions: {
--- a/extensions/bluebubbles/src/session-route.test.ts
+++ b/extensions/bluebubbles/src/session-route.test.ts
@@ -3,10 +3,13 @@ import type { OpenClawConfig } from "./runtime-api.js";
 import { resolveBlueBubblesOutboundSessionRoute } from "./session-route.js";

 const EMPTY_CFG = {} as OpenClawConfig;
+const PER_PEER_CFG = {
+  session: { dmScope: "per-peer" },
+} as OpenClawConfig;

-function call(target: string) {
+function call(target: string, cfg = EMPTY_CFG) {
  return resolveBlueBubblesOutboundSessionRoute({
-    cfg: EMPTY_CFG,
+    cfg,
    agentId: "agent-1",
    accountId: "default",
    target,
@@ -25,8 +28,10 @@ describe("resolveBlueBubblesOutboundSessionRoute DM/group disambiguation", () =>
    const route = call("bluebubbles:chat_guid:iMessage;-;+15551234567");
    expect(route).not.toBeNull();
    expect(route?.peer.kind).toBe("direct");
+    expect(route?.peer.id).toBe("+15551234567");
    expect(route?.chatType).toBe("direct");
-    expect(route?.from).toMatch(/^bluebubbles:/);
+    expect(route?.from).toBe("bluebubbles:+15551234567");
+    expect(route?.to).toBe("bluebubbles:chat_guid:iMessage;-;+15551234567");
    expect(route?.from).not.toMatch(/^group:/);
  });

@@ -71,11 +76,14 @@ describe("resolveBlueBubblesOutboundSessionRoute DM/group disambiguation", () =>
  it("DM via chat_guid and DM via handle land on the same session key", () => {
    // The point of disambiguation: a DM addressed two different ways must
    // converge on the same sessionKey so existing bindings keep matching.
-    const handleRoute = call("bluebubbles:imessage:+15551234567");
-    const chatGuidRoute = call("bluebubbles:chat_guid:iMessage;-;+15551234567");
+    const handleRoute = call("bluebubbles:imessage:+15551234567", PER_PEER_CFG);
+    const chatGuidRoute = call("bluebubbles:chat_guid:iMessage;-;+15551234567", PER_PEER_CFG);
    expect(handleRoute?.sessionKey).toBeDefined();
    expect(chatGuidRoute?.sessionKey).toBeDefined();
-    // Both are direct now; sessionKey base derives from peer.id.
    expect(handleRoute?.peer.kind).toBe(chatGuidRoute?.peer.kind);
+    expect(handleRoute?.peer.id).toBe(chatGuidRoute?.peer.id);
+    expect(handleRoute?.from).toBe(chatGuidRoute?.from);
+    expect(handleRoute?.sessionKey).toBe(chatGuidRoute?.sessionKey);
+    expect(chatGuidRoute?.to).toBe("bluebubbles:chat_guid:iMessage;-;+15551234567");
  });
 });
--- a/extensions/bluebubbles/src/session-route.ts
+++ b/extensions/bluebubbles/src/session-route.ts
@@ -4,7 +4,7 @@ import {
  type ChannelOutboundSessionRouteParams,
 } from "openclaw/plugin-sdk/channel-core";
 import { resolveGroupFlagFromChatGuid } from "./monitor-normalize.js";
-import { parseBlueBubblesTarget } from "./targets.js";
+import { extractHandleFromChatGuid, parseBlueBubblesTarget } from "./targets.js";

 export function resolveBlueBubblesOutboundSessionRoute(params: ChannelOutboundSessionRouteParams) {
  const stripped = stripChannelTargetPrefix(params.target, "bluebubbles");
@@ -27,11 +27,15 @@ export function resolveBlueBubblesOutboundSessionRoute(params: ChannelOutboundSe
      : parsed.kind === "chat_guid"
        ? (groupFromChatGuid ?? true)
        : false;
+  const dmHandleFromChatGuid =
+    parsed.kind === "chat_guid" && groupFromChatGuid === false
+      ? extractHandleFromChatGuid(parsed.chatGuid)
+      : null;
  const peerId =
    parsed.kind === "chat_id"
      ? String(parsed.chatId)
      : parsed.kind === "chat_guid"
-        ? parsed.chatGuid
+        ? (dmHandleFromChatGuid ?? parsed.chatGuid)
        : parsed.kind === "chat_identifier"
          ? parsed.chatIdentifier
          : parsed.to;