diff --git a/CHANGELOG.md b/CHANGELOG.md index 36bbb271a0d..bd90d3b7bc3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,7 +34,7 @@ Docs: https://docs.openclaw.ai - Chat/Usage/TUI: strip synthetic inbound metadata blocks (including `Conversation info` and trailing `Untrusted context` channel metadata wrappers) from displayed conversation history so internal prompt context no longer leaks into user-visible logs. - Security/Exec: in non-default setups that manually add `sort` to `tools.exec.safeBins`, block `sort --compress-program` so allowlist-mode safe-bin checks cannot bypass approval. Thanks @tdjackey for reporting. -- Security/Discord: add `openclaw security audit` warnings for name/tag-based Discord allowlist entries (DM allowlists, guild/channel `users`, and pairing-store entries), highlighting slug-collision risk while keeping name-based matching supported. Thanks @tdjackey for reporting. +- Security/Discord: add `openclaw security audit` warnings for name/tag-based Discord allowlist entries (DM allowlists, guild/channel `users`, and pairing-store entries), highlighting slug-collision risk while keeping name-based matching supported, and canonicalize resolved Discord allowlist names to IDs at runtime without rewriting config files. Thanks @tdjackey for reporting. - Security/BlueBubbles: make parsed chat allowlist checks fail closed when `allowFrom` is empty, restoring expected `pairing`/`allowlist` DM gating for BlueBubbles and blocking unauthorized DM/reaction processing when no allowlist entries are configured. This ships in the next npm release. Thanks @tdjackey for reporting. - Doctor/State integrity: only require/create the OAuth credentials directory when WhatsApp or pairing-backed channels are configured, and downgrade fresh-install missing-dir noise to an informational warning. - Agents/Sanitization: stop rewriting billing-shaped assistant text outside explicit error context so normal replies about billing/credits/payment are preserved across messaging channels. (#17834, fixes #11359) diff --git a/src/channels/allowlists/resolve-utils.test.ts b/src/channels/allowlists/resolve-utils.test.ts index 7d8cc212345..807e7c06877 100644 --- a/src/channels/allowlists/resolve-utils.test.ts +++ b/src/channels/allowlists/resolve-utils.test.ts @@ -2,6 +2,8 @@ import { describe, expect, it } from "vitest"; import { addAllowlistUserEntriesFromConfigEntry, buildAllowlistResolutionSummary, + canonicalizeAllowlistWithResolvedIds, + patchAllowlistUsersInConfigEntries, } from "./resolve-utils.js"; describe("buildAllowlistResolutionSummary", () => { @@ -40,3 +42,46 @@ describe("addAllowlistUserEntriesFromConfigEntry", () => { expect(Array.from(target)).toEqual(["a"]); }); }); + +describe("canonicalizeAllowlistWithResolvedIds", () => { + it("replaces resolved names with ids and keeps unresolved entries", () => { + const resolvedMap = new Map([ + ["Alice#1234", { input: "Alice#1234", resolved: true, id: "111" }], + ["bob", { input: "bob", resolved: false }], + ]); + const result = canonicalizeAllowlistWithResolvedIds({ + existing: ["Alice#1234", "bob", "222", "*"], + resolvedMap, + }); + expect(result).toEqual(["111", "bob", "222", "*"]); + }); + + it("deduplicates ids after canonicalization", () => { + const resolvedMap = new Map([["alice", { input: "alice", resolved: true, id: "111" }]]); + const result = canonicalizeAllowlistWithResolvedIds({ + existing: ["alice", "111", "alice"], + resolvedMap, + }); + expect(result).toEqual(["111"]); + }); +}); + +describe("patchAllowlistUsersInConfigEntries", () => { + it("supports canonicalization strategy for nested users", () => { + const entries = { + alpha: { users: ["Alice", "111", "Bob"] }, + beta: { users: ["*"] }, + }; + const resolvedMap = new Map([ + ["Alice", { input: "Alice", resolved: true, id: "111" }], + ["Bob", { input: "Bob", resolved: false }], + ]); + const patched = patchAllowlistUsersInConfigEntries({ + entries, + resolvedMap, + strategy: "canonicalize", + }); + expect((patched.alpha as { users: string[] }).users).toEqual(["111", "Bob"]); + expect((patched.beta as { users: string[] }).users).toEqual(["*"]); + }); +}); diff --git a/src/channels/allowlists/resolve-utils.ts b/src/channels/allowlists/resolve-utils.ts index 46b439093c9..183571ea420 100644 --- a/src/channels/allowlists/resolve-utils.ts +++ b/src/channels/allowlists/resolve-utils.ts @@ -6,31 +6,32 @@ export type AllowlistUserResolutionLike = { id?: string; }; +function dedupeAllowlistEntries(entries: string[]): string[] { + const seen = new Set(); + const deduped: string[] = []; + for (const entry of entries) { + const normalized = entry.trim(); + if (!normalized) { + continue; + } + const key = normalized.toLowerCase(); + if (seen.has(key)) { + continue; + } + seen.add(key); + deduped.push(normalized); + } + return deduped; +} + export function mergeAllowlist(params: { existing?: Array; additions: string[]; }): string[] { - const seen = new Set(); - const merged: string[] = []; - const push = (value: string) => { - const normalized = value.trim(); - if (!normalized) { - return; - } - const key = normalized.toLowerCase(); - if (seen.has(key)) { - return; - } - seen.add(key); - merged.push(normalized); - }; - for (const entry of params.existing ?? []) { - push(String(entry)); - } - for (const entry of params.additions) { - push(entry); - } - return merged; + return dedupeAllowlistEntries([ + ...(params.existing ?? []).map((entry) => String(entry)), + ...params.additions, + ]); } export function buildAllowlistResolutionSummary( @@ -71,10 +72,33 @@ export function resolveAllowlistIdAdditions(params: { existing?: Array; resolvedMap: Map }): string[] { + const canonicalized: string[] = []; + for (const entry of params.existing ?? []) { + const trimmed = String(entry).trim(); + if (!trimmed) { + continue; + } + if (trimmed === "*") { + canonicalized.push(trimmed); + continue; + } + const resolved = params.resolvedMap.get(trimmed); + canonicalized.push(resolved?.resolved && resolved.id ? resolved.id : trimmed); + } + return dedupeAllowlistEntries(canonicalized); +} + export function patchAllowlistUsersInConfigEntries< T extends AllowlistUserResolutionLike, TEntries extends Record, ->(params: { entries: TEntries; resolvedMap: Map }): TEntries { +>(params: { + entries: TEntries; + resolvedMap: Map; + strategy?: "merge" | "canonicalize"; +}): TEntries { const nextEntries: Record = { ...params.entries }; for (const [entryKey, entryConfig] of Object.entries(params.entries)) { if (!entryConfig || typeof entryConfig !== "object") { @@ -88,9 +112,16 @@ export function patchAllowlistUsersInConfigEntries< existing: users, resolvedMap: params.resolvedMap, }); + const resolvedUsers = + params.strategy === "canonicalize" + ? canonicalizeAllowlistWithResolvedIds({ + existing: users, + resolvedMap: params.resolvedMap, + }) + : mergeAllowlist({ existing: users, additions }); nextEntries[entryKey] = { ...entryConfig, - users: mergeAllowlist({ existing: users, additions }), + users: resolvedUsers, }; } return nextEntries as TEntries; diff --git a/src/discord/monitor/provider.allowlist.test.ts b/src/discord/monitor/provider.allowlist.test.ts new file mode 100644 index 00000000000..63b4b01708d --- /dev/null +++ b/src/discord/monitor/provider.allowlist.test.ts @@ -0,0 +1,57 @@ +import { describe, expect, it, vi } from "vitest"; +import type { RuntimeEnv } from "../../runtime.js"; + +const { resolveDiscordChannelAllowlistMock, resolveDiscordUserAllowlistMock } = vi.hoisted(() => ({ + resolveDiscordChannelAllowlistMock: vi.fn(async () => []), + resolveDiscordUserAllowlistMock: vi.fn(async (params: { entries: string[] }) => + params.entries.map((entry) => { + switch (entry) { + case "Alice": + return { input: entry, resolved: true, id: "111" }; + case "Bob": + return { input: entry, resolved: true, id: "222" }; + case "Carol": + return { input: entry, resolved: false }; + default: + return { input: entry, resolved: true, id: entry }; + } + }), + ), +})); + +vi.mock("../resolve-channels.js", () => ({ + resolveDiscordChannelAllowlist: resolveDiscordChannelAllowlistMock, +})); + +vi.mock("../resolve-users.js", () => ({ + resolveDiscordUserAllowlist: resolveDiscordUserAllowlistMock, +})); + +import { resolveDiscordAllowlistConfig } from "./provider.allowlist.js"; + +describe("resolveDiscordAllowlistConfig", () => { + it("canonicalizes resolved user names to ids in runtime config", async () => { + const runtime = { log: vi.fn(), error: vi.fn(), exit: vi.fn() } as unknown as RuntimeEnv; + const result = await resolveDiscordAllowlistConfig({ + token: "token", + allowFrom: ["Alice", "111", "*"], + guildEntries: { + "*": { + users: ["Bob", "999"], + channels: { + "*": { + users: ["Carol", "888"], + }, + }, + }, + }, + fetcher: vi.fn() as unknown as typeof fetch, + runtime, + }); + + expect(result.allowFrom).toEqual(["111", "*"]); + expect(result.guildEntries?.["*"]?.users).toEqual(["222", "999"]); + expect(result.guildEntries?.["*"]?.channels?.["*"]?.users).toEqual(["Carol", "888"]); + expect(resolveDiscordUserAllowlistMock).toHaveBeenCalledTimes(2); + }); +}); diff --git a/src/discord/monitor/provider.allowlist.ts b/src/discord/monitor/provider.allowlist.ts index 1d64d022f5c..4bc6cc3a6d8 100644 --- a/src/discord/monitor/provider.allowlist.ts +++ b/src/discord/monitor/provider.allowlist.ts @@ -1,9 +1,8 @@ import { addAllowlistUserEntriesFromConfigEntry, buildAllowlistResolutionSummary, - mergeAllowlist, + canonicalizeAllowlistWithResolvedIds, patchAllowlistUsersInConfigEntries, - resolveAllowlistIdAdditions, summarizeMapping, } from "../../channels/allowlists/resolve-utils.js"; import type { DiscordGuildEntry } from "../../config/types.discord.js"; @@ -138,8 +137,11 @@ export async function resolveDiscordAllowlistConfig(params: { entries: allowEntries.map((entry) => String(entry)), fetcher: params.fetcher, }); - const { mapping, unresolved, additions } = buildAllowlistResolutionSummary(resolvedUsers); - allowFrom = mergeAllowlist({ existing: allowFrom, additions }); + const { resolvedMap, mapping, unresolved } = buildAllowlistResolutionSummary(resolvedUsers); + allowFrom = canonicalizeAllowlistWithResolvedIds({ + existing: allowFrom, + resolvedMap, + }); summarizeMapping("discord users", mapping, unresolved, params.runtime); } catch (err) { params.runtime.log?.( @@ -178,14 +180,17 @@ export async function resolveDiscordAllowlistConfig(params: { const nextGuild = { ...guildConfig } as Record; const users = (guildConfig as { users?: string[] }).users; if (Array.isArray(users) && users.length > 0) { - const additions = resolveAllowlistIdAdditions({ existing: users, resolvedMap }); - nextGuild.users = mergeAllowlist({ existing: users, additions }); + nextGuild.users = canonicalizeAllowlistWithResolvedIds({ + existing: users, + resolvedMap, + }); } const channels = (guildConfig as { channels?: Record }).channels ?? {}; if (channels && typeof channels === "object") { nextGuild.channels = patchAllowlistUsersInConfigEntries({ entries: channels, resolvedMap, + strategy: "canonicalize", }); } nextGuilds[guildKey] = nextGuild as DiscordGuildEntry;