mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-06 05:50:43 +00:00
fix(bluebubbles): address aisle re-review on routing-guard PR
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
(`<short:N-digit>` or `<uuid:prefix…>`) 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.
This commit is contained in:
committed by
Peter Steinberger
parent
81fd4d560a
commit
081e4be11e
@@ -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<redacted>",
|
||||
);
|
||||
cleaned = cleaned.replace(/(authorization\s*:\s*(?:bearer|basic)\s+)[^\s"]+/gi, "$1<redacted>");
|
||||
return cleaned.length > maxLen ? cleaned.slice(0, maxLen) + "..." : cleaned;
|
||||
}
|
||||
|
||||
|
||||
@@ -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", () => {
|
||||
|
||||
@@ -156,6 +156,20 @@ function describeChatForError(values: {
|
||||
return parts.length === 0 ? "<unknown chat>" : 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 `<short:${len}-digit>`;
|
||||
}
|
||||
// For UUID input, expose just an 8-char prefix; consumer can correlate
|
||||
// against full GUID via the trace if needed.
|
||||
return `<uuid:${inputId.slice(0, 8)}…>`;
|
||||
}
|
||||
|
||||
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;
|
||||
|
||||
Reference in New Issue
Block a user