mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-06 09:30:43 +00:00
fix(bluebubbles): resolve PR review findings (#67510)
- monitor-processing: move attachment retry into the !rawBody guard so image-only new-message events that arrive with empty attachments and empty text are recovered via a BB API refetch before being dropped. The existing retry block at the end of processMessageAfterDedupe was unreachable for this case because the !rawBody early-return fired first. (Greptile) - monitor: derive isAttachmentUpdate from the normalized message shape instead of raw payload.data.attachments so updated-message webhooks with attachments under wrapper formats (payload.message, JSON-string payloads) are correctly routed through for processing instead of silently filtered. (Codex) - types: use bundled-undici fetch when init.dispatcher is present so the SSRF guard's DNS-pinning dispatcher is preserved when this function is called as fetchImpl from guarded callers (e.g. the attachment download path via fetchRemoteMedia). Falls back to globalThis.fetch when no dispatcher is present so tests that stub globalThis.fetch keep working. (Codex) - attachments: blueBubblesPolicy returns undefined for the non-private case (matching monitor-processing's helper) so sendBlueBubblesAttachment stops routing localhost BB through the SSRF guard. (Greptile) - scripts/check-no-raw-channel-fetch: bump the types.ts allowlist line to match the restructured non-SSRF branch.
This commit is contained in:
@@ -27,8 +27,12 @@ import {
|
||||
type SsrFPolicy,
|
||||
} from "./types.js";
|
||||
|
||||
function blueBubblesPolicy(allowPrivateNetwork: boolean | undefined): SsrFPolicy {
|
||||
return allowPrivateNetwork ? { allowPrivateNetwork: true } : {};
|
||||
function blueBubblesPolicy(allowPrivateNetwork: boolean | undefined): SsrFPolicy | undefined {
|
||||
// Pass `undefined` (not `{}`) for the non-private case so the non-SSRF fallback path
|
||||
// is used. An empty `{}` policy routes through the SSRF guard, which blocks the
|
||||
// localhost BB deployments that are the most common self-hosted setup. The opt-in
|
||||
// private-network branch keeps the explicit policy. (#64105, #67510)
|
||||
return allowPrivateNetwork ? { allowPrivateNetwork: true } : undefined;
|
||||
}
|
||||
|
||||
export type BlueBubblesAttachmentOpts = {
|
||||
|
||||
@@ -695,8 +695,8 @@ async function processMessageAfterDedupe(
|
||||
const isGroup = typeof groupFlag === "boolean" ? groupFlag : message.isGroup;
|
||||
|
||||
const text = message.text.trim();
|
||||
const attachments = message.attachments ?? [];
|
||||
const placeholder = buildMessagePlaceholder(message);
|
||||
let attachments = message.attachments ?? [];
|
||||
let placeholder = buildMessagePlaceholder(message);
|
||||
// Check if text is a tapback pattern (e.g., 'Loved "hello"') and transform to emoji format
|
||||
// For tapbacks, we'll append [[reply_to:N]] at the end; for regular messages, prepend it
|
||||
const tapbackContext = resolveTapbackContext(message);
|
||||
@@ -707,7 +707,7 @@ async function processMessageAfterDedupe(
|
||||
requireQuoted: !tapbackContext,
|
||||
});
|
||||
const isTapbackMessage = Boolean(tapbackParsed);
|
||||
const rawBody = tapbackParsed
|
||||
let rawBody = tapbackParsed
|
||||
? tapbackParsed.action === "removed"
|
||||
? `removed ${tapbackParsed.emoji} reaction`
|
||||
: `reacted with ${tapbackParsed.emoji}`
|
||||
@@ -789,8 +789,60 @@ async function processMessageAfterDedupe(
|
||||
}
|
||||
|
||||
if (!rawBody) {
|
||||
logVerbose(core, runtime, `drop: empty text sender=${message.senderId}`);
|
||||
return;
|
||||
// Image-only `new-message` events can arrive with text="" and attachments=[]
|
||||
// before BB finishes attachment indexing. The retry block lower in this function
|
||||
// is unreachable in that case (the early return above would fire), so attempt one
|
||||
// recovery fetch from the BB API here. Without this, deployments where BB never
|
||||
// sends a follow-up `updated-message` would silently drop image-only messages.
|
||||
// (#65430, #67437, #67510)
|
||||
const retryBaseUrl = normalizeSecretInputString(account.config.serverUrl);
|
||||
const retryPassword = normalizeSecretInputString(account.config.password);
|
||||
const retryMessageIdEarly = message.messageId?.trim();
|
||||
const canRecoverEmpty =
|
||||
attachments.length === 0 &&
|
||||
retryMessageIdEarly &&
|
||||
retryBaseUrl &&
|
||||
retryPassword &&
|
||||
(text.length === 0 || message.eventType === "updated-message");
|
||||
let recoveredEmpty = false;
|
||||
if (canRecoverEmpty) {
|
||||
try {
|
||||
await new Promise<void>((resolve) => setTimeout(resolve, 2_000));
|
||||
const fetched = await fetchBlueBubblesMessageAttachments(retryMessageIdEarly, {
|
||||
baseUrl: retryBaseUrl,
|
||||
password: retryPassword,
|
||||
timeoutMs: 10_000,
|
||||
allowPrivateNetwork: isPrivateNetworkOptInEnabled(account.config),
|
||||
});
|
||||
if (fetched.length > 0) {
|
||||
message.attachments = fetched;
|
||||
attachments = fetched;
|
||||
const recoveredPlaceholder = buildMessagePlaceholder(message);
|
||||
if (recoveredPlaceholder) {
|
||||
placeholder = recoveredPlaceholder;
|
||||
rawBody = text || placeholder;
|
||||
recoveredEmpty = rawBody.length > 0;
|
||||
if (recoveredEmpty) {
|
||||
logVerbose(
|
||||
core,
|
||||
runtime,
|
||||
`attachment retry recovered ${fetched.length} attachment(s) for empty msgId=${message.messageId}`,
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
} catch (err) {
|
||||
logVerbose(
|
||||
core,
|
||||
runtime,
|
||||
`attachment retry (empty body) failed for msgId=${message.messageId}: ${String(err)}`,
|
||||
);
|
||||
}
|
||||
}
|
||||
if (!recoveredEmpty) {
|
||||
logVerbose(core, runtime, `drop: empty text sender=${message.senderId}`);
|
||||
return;
|
||||
}
|
||||
}
|
||||
logVerbose(
|
||||
core,
|
||||
|
||||
@@ -249,15 +249,16 @@ export async function handleBlueBubblesWebhookRequest(
|
||||
return true;
|
||||
}
|
||||
const reaction = normalizeWebhookReaction(payload);
|
||||
// Normalize the webhook message early so the attachment-update detection
|
||||
// below sees attachments under any supported wrapper format (`payload.data`,
|
||||
// `payload.message`, `payload.data.message`, JSON-string payloads), not just
|
||||
// raw `payload.data.attachments`. (#65430, #67510)
|
||||
const message = reaction ? null : normalizeWebhookMessage(payload, { eventType });
|
||||
// BlueBubbles fires `updated-message` when attachments are indexed after the
|
||||
// initial `new-message` (which may arrive with attachments: []). Let those
|
||||
// through so the agent can ingest the image. (#65430)
|
||||
const dataRecord = asRecord(payload.data);
|
||||
const dataAttachments = dataRecord?.attachments;
|
||||
const isAttachmentUpdate =
|
||||
eventType === "updated-message" &&
|
||||
Array.isArray(dataAttachments) &&
|
||||
dataAttachments.length > 0;
|
||||
eventType === "updated-message" && (message?.attachments?.length ?? 0) > 0;
|
||||
if (
|
||||
(eventType === "updated-message" ||
|
||||
eventType === "message-reaction" ||
|
||||
@@ -271,12 +272,11 @@ export async function handleBlueBubblesWebhookRequest(
|
||||
logVerbose(
|
||||
firstTarget.core,
|
||||
firstTarget.runtime,
|
||||
`webhook ignored ${eventType || "event"} without reaction`,
|
||||
`webhook ignored ${eventType || "event"} (no reaction or attachment update)`,
|
||||
);
|
||||
}
|
||||
return true;
|
||||
}
|
||||
const message = reaction ? null : normalizeWebhookMessage(payload, { eventType });
|
||||
if (!message && !reaction) {
|
||||
res.statusCode = 400;
|
||||
res.end("invalid payload");
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import type { DmPolicy, GroupPolicy } from "openclaw/plugin-sdk/setup";
|
||||
import { fetchWithSsrFGuard, type SsrFPolicy } from "openclaw/plugin-sdk/ssrf-runtime";
|
||||
import { fetch as undiciFetch } from "undici";
|
||||
|
||||
export type { SsrFPolicy } from "openclaw/plugin-sdk/ssrf-runtime";
|
||||
export type { DmPolicy, GroupPolicy } from "openclaw/plugin-sdk/setup";
|
||||
@@ -175,16 +176,26 @@ export async function blueBubblesFetchWithTimeout(
|
||||
await release();
|
||||
}
|
||||
}
|
||||
// Strip `dispatcher` from init — the SSRF guard may have attached a bundled-undici
|
||||
// dispatcher that is incompatible with Node 22+'s built-in undici backing globalThis.fetch().
|
||||
// Passing it through causes a silent TypeError (invalid onRequestStart method). (#64105)
|
||||
const { dispatcher: _dispatcher, ...safeInit } = (init ?? {}) as RequestInit & {
|
||||
dispatcher?: unknown;
|
||||
};
|
||||
// The SSRF guard (and other guarded callers using this function as their
|
||||
// `fetchImpl`) may attach a bundled-undici `dispatcher` to `init` to enforce DNS
|
||||
// pinning per request. That dispatcher is incompatible with Node 22+'s built-in
|
||||
// undici backing globalThis.fetch and causes a silent TypeError (invalid
|
||||
// onRequestStart method) when forwarded — but it works correctly with the
|
||||
// bundled-undici `fetch`. When a dispatcher is present, route through bundled
|
||||
// undici so the DNS-pinning contract is preserved; otherwise stay on
|
||||
// globalThis.fetch. (#64105, #67510)
|
||||
const initWithDispatcher = (init ?? {}) as RequestInit & { dispatcher?: unknown };
|
||||
const hasDispatcher = initWithDispatcher.dispatcher !== undefined;
|
||||
const controller = new AbortController();
|
||||
const timer = setTimeout(() => controller.abort(), timeoutMs);
|
||||
try {
|
||||
return await fetch(url, { ...safeInit, signal: controller.signal });
|
||||
if (hasDispatcher) {
|
||||
return (await undiciFetch(url, {
|
||||
...initWithDispatcher,
|
||||
signal: controller.signal,
|
||||
} as Parameters<typeof undiciFetch>[1])) as unknown as Response;
|
||||
}
|
||||
return await fetch(url, { ...initWithDispatcher, signal: controller.signal });
|
||||
} finally {
|
||||
clearTimeout(timer);
|
||||
}
|
||||
|
||||
@@ -15,7 +15,7 @@ const sourceRoots = ["src/channels", "src/routing", "src/line", "extensions"];
|
||||
// code should be rejected and migrated to fetchWithSsrFGuard/shared channel helpers.
|
||||
const allowedRawFetchCallsites = new Set([
|
||||
bundledPluginCallsite("bluebubbles", "src/test-harness.ts", 130),
|
||||
bundledPluginCallsite("bluebubbles", "src/types.ts", 187),
|
||||
bundledPluginCallsite("bluebubbles", "src/types.ts", 198),
|
||||
bundledPluginCallsite("browser", "src/browser/cdp.helpers.ts", 268),
|
||||
bundledPluginCallsite("browser", "src/browser/client-fetch.ts", 192),
|
||||
bundledPluginCallsite("browser", "src/browser/test-fetch.ts", 24),
|
||||
|
||||
Reference in New Issue
Block a user