Description

createPersistentDedupe() introduces an in-process per-file write queue (fileWriteQueues) that serializes all read-modify-write cycles to the same JSON file.

Because the queue is implemented as an unbounded promise chain, an attacker (or simply high-throughput workloads) can trigger arbitrarily many concurrent checkAndRecord() calls that target the same namespace (hence the same file path), causing:

• Unbounded memory growth: each queued operation captures closures/stack/promise bookkeeping until it reaches the head of the chain.
• Event-loop / throughput collapse: the system becomes effectively single-threaded on that file; if the head operation slows, all subsequent work backs up.
• Global stall amplification: if any queued operation hangs (e.g., stuck filesystem I/O, pathological lock contention), all subsequent enqueued operations for that file will stall indefinitely.

This is reachable in typical usage patterns where many different keys map to the same namespace file (e.g., deduping many unique message IDs under a single namespace). The existing inflight map only dedupes the exact same namespace:key and does not limit concurrency for different keys within the same file.

Vulnerable code:

const fileWriteQueues = new Map<string, Promise<unknown>>();

function enqueueFileWrite<T>(filePath: string, fn: () => Promise<T>): Promise<T> {
  const prev = fileWriteQueues.get(filePath) ?? Promise.resolve();
  const next = prev.then(fn, fn);
  fileWriteQueues.set(filePath, next);
  ​// ...
  return next;
}

Recommendation

Add explicit backpressure and bounded queueing per file, and ensure stalled operations cannot block the queue indefinitely.

Suggested mitigations (combine as appropriate):

1. Cap queue length per file path; reject or shed load when exceeded.
2. Coalesce writes (batch multiple keys into a single write) rather than queueing one write per call.
3. Add timeouts around the queued work (including file-lock acquisition and disk I/O), failing fast and allowing subsequent work to proceed.
4. Use a real mutex/semaphore with a bounded wait queue rather than chaining promises.

Example: bounded semaphore (pseudo-code):

import { Semaphore } from "async-mutex";

const semaphores = new Map<string, { sem: Semaphore; queued: number }>();
const MAX_WAITERS = 1000;

async function withBoundedFileMutex<T>(filePath: string, fn: () => Promise<T>): Promise<T> {
  const entry = semaphores.get(filePath) ?? { sem: new Semaphore(1), queued: 0 };
  semaphores.set(filePath, entry);
  if (entry.queued >= MAX_WAITERS) {
    throw new Error(`dedupe backlog exceeded for ${filePath}`);
  }
  entry.queued++;
  const [value] = await entry.sem.acquire();
  try {
    return await fn();
  } finally {
    entry.queued--;
    value();
    if (entry.queued === 0) semaphores.delete(filePath);
  }
}

Description

migrateLegacyDedupeFile() performs a legacy→new dedupe file rename/removal using existsSync checks followed by renameSync/unlinkSync with no validation that the involved paths are regular files inside the expected directory.

If an attacker can write into the OpenClaw state directory (or can influence OPENCLAW_STATE_DIR to point at attacker-controlled storage), they can exploit this best-effort migration to affect which filesystem object is renamed or deleted:

• TOCTOU race: existsSync(legacyPath)/existsSync(newPath) checks are separated from the subsequent renameSync/unlinkSync operations.
• No type checks: the code never lstats to ensure legacyPath is a regular file (not a symlink) and not a hardlink to another file.
• Potential arbitrary file deletion: if legacyPath is (or is swapped to) a hardlink to another file that the process can delete, unlinkSync(legacyPath) decrements the link count and may delete the underlying target when it reaches zero.

Vulnerable code:

if (!fs.existsSync(newPath)) {
  fs.renameSync(legacyPath, newPath);
} else {
  fs.unlinkSync(legacyPath);
}

Recommendation

Harden the migration so it only operates on expected regular files within the dedupe directory, and avoid TOCTOU patterns.

Recommended changes:

1. Resolve and validate the dedupe directory and both paths are within it.
2. Use lstat (not stat) to reject symlinks.
3. Only migrate regular files (optionally also reject files with nlink > 1 to reduce hardlink risk).
4. Prefer a single-step operation where possible and handle errors explicitly.

Example (sketch):

const dedupeDir = path.join(resolveStateDirFromEnv(), "bluebubbles", "inbound-dedupe");
const legacyPath = resolveLegacyNamespaceFilePath(namespace);

for (const p of [legacyPath, newPath]) {
  const realParent = await fs.promises.realpath(path.dirname(p));
  const realDedupe = await fs.promises.realpath(dedupeDir);
  if (!realParent.startsWith(realDedupe + path.sep)) return; // refuse
}

let st: fs.Stats;
try {
  st = await fs.promises.lstat(legacyPath);
} catch { return; }
if (!st.isFile()) return;           // rejects symlink and non-file
if (st.nlink > 1) return;           // optional hardlink defense

try {
  await fs.promises.rename(legacyPath, newPath);
} catch (e: any) {
  if (e.code === "EEXIST") {
    await fs.promises.unlink(legacyPath);
  }
}

Also ensure the state directory is created with restrictive permissions (e.g., 0700) and is not writable by untrusted users.

Description

In runBlueBubblesCatchupInner, catchup now skips processing entirely for any fetched message record that has a non-empty associatedMessageGuid and either associatedMessageType is non-null or balloonBundleId is set.

• These fields come directly from the BlueBubbles API response (rec) and are not validated against a known set of reaction/tapback types.
• When the condition matches, the loop continues before normalization, dedupe, or any processing, but latestFetchedTs has already been advanced.
• As a result, the cursor can advance past the skipped record, causing permanent message loss in catchup replay.

If a malicious/compromised BlueBubbles server (or a buggy upstream) marks a normal text message with these fields, the gateway will silently drop it during catchup, potentially bypassing downstream automations/moderation that rely on replay integrity.

Vulnerable code:

const assocType = rec.associatedMessageType ?? rec.associated_message_type;
const balloonId = typeof rec.balloonBundleId === "string" ? rec.balloonBundleId.trim() : "";
if (assocGuid && (assocType != null || balloonId)) {
  continue;
}

Recommendation

Harden the skip logic so only known non-message events are skipped, and avoid permanently advancing the cursor past potentially-real messages.

Suggested approaches (pick one):

1. Strictly recognize reactions/tapbacks:

   • Only skip when associatedMessageType is a number and is in a known allowlist of tapback/reaction types.
   • Consider also requiring isTapback === true (if available) for reaction skips.

2. Treat balloon-only events separately:

   • Only skip balloon events when there is strong evidence it is a preview/sticker-only payload (e.g., empty text AND no meaningful attachments).
   • Otherwise, normalize and process.

3. Fail safe on ambiguous cases:

   • If assocGuid is set but the type/bundle is unrecognized, log a warning and still pass the message through normal processing.

Example hardening:

const assocType = rec.associatedMessageType ?? rec.associated_message_type;
const balloonId = typeof rec.balloonBundleId === "string" ? rec.balloonBundleId.trim() : "";
const isKnownReaction = typeof assocType === "number" && KNOWN_REACTION_TYPES.has(assocType);
const isBalloon = Boolean(balloonId);

if (assocGuid && (isKnownReaction || isBalloon)) {
  ​// optionally: increment a skipped counter + debug log
  continue;
}

Additionally, consider tracking skipped items in the summary/logs (and/or not advancing the cursor past them) to avoid silent data loss if upstream mislabels events.

Description

The catchup retry/give-up logic logs retryKey (derived from normalized.messageId or raw rec.guid) and String(err) directly into error logs.

• normalized.messageId comes from webhook payload fields guid/id/messageId without any sanitization (see normalizeWebhookMessage).
• String(err) may include attacker-influenced content (e.g., values derived from webhook payloads, remote responses, or other untrusted strings).
• Because these values are interpolated verbatim, an attacker who can influence them can inject control characters (CR/LF, tabs, other Unicode control chars) to forge/split log lines, confuse structured log parsers, or cause sensitive content to be persisted in operator logs.

Vulnerable code:

error?.(
  `[${accountId}] BlueBubbles catchup: giving up on guid=${retryKey} ` +
    `after ${nextCount} consecutive failures; future sweeps will skip ` +
    `this message. timestamp=${ts}: ${String(err)}`,
);
...
error?.(
  `[${accountId}] BlueBubbles catchup: processMessage failed (retry ` +
    `${nextCount}/${maxFailureRetries}): ${String(err)}`,
);

Note: elsewhere in the codebase there is already a sanitizeForLog() helper that strips control characters before logging, but this file does not use it.

Recommendation

Sanitize/neutralize untrusted strings before writing to logs, and bound their length.

• Reuse the existing sanitizeForLog() logic (or introduce a shared utility) to strip CR/LF/other control characters and truncate.
• Consider logging structured error fields (e.g., err.name, err.code) and only a sanitized message.

Example:

function sanitizeForLog(value: unknown, maxLen = 200): string {
  const cleaned = String(value).replace(/[\r\n\t\p{C}]/gu, " ");
  return cleaned.length > maxLen ? cleaned.slice(0, maxLen) + "..." : cleaned;
}

const safeKey = sanitizeForLog(retryKey, 120);
const safeErr = sanitizeForLog(err, 200);

error?.(
  `[${accountId}] BlueBubbles catchup: giving up on guid=${safeKey} ` +
  `after ${nextCount} consecutive failures; timestamp=${ts}: ${safeErr}`,
);

This prevents log forging/splitting and reduces the chance of persisting sensitive payload fragments in logs.