From f97c45c5b5e0698b6667bb5f6badc0cac7dabd12 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 21 Feb 2026 19:44:58 +0100 Subject: [PATCH] fix(security): warn on Discord name-based allowlists in audit --- CHANGELOG.md | 1 + docs/channels/discord.md | 1 + docs/cli/security.md | 1 + src/security/audit-channel.ts | 107 +++++++++++++++++++++++++++++++++- src/security/audit.test.ts | 93 +++++++++++++++++++++++++++++ 5 files changed, 201 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 93cfcbf73..a9ed3b02b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +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. - 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) - Security/Agents: cap embedded Pi runner outer retry loop with a higher profile-aware dynamic limit (32-160 attempts) and return an explicit `retry_limit` error payload when retries never converge, preventing unbounded internal retry cycles (`GHSA-76m6-pj3w-v7mf`). diff --git a/docs/channels/discord.md b/docs/channels/discord.md index 044e87840..adafd6042 100644 --- a/docs/channels/discord.md +++ b/docs/channels/discord.md @@ -398,6 +398,7 @@ Example: - guild must match `channels.discord.guilds` (`id` preferred, slug accepted) - optional sender allowlists: `users` (IDs or names) and `roles` (role IDs only); if either is configured, senders are allowed when they match `users` OR `roles` + - names/tags are supported for `users`, but IDs are safer; `openclaw security audit` warns when name/tag entries are used - if a guild has `channels` configured, non-listed channels are denied - if a guild has no `channels` block, all channels in that allowlisted guild are allowed diff --git a/docs/cli/security.md b/docs/cli/security.md index 9bfa39b13..84f8c4080 100644 --- a/docs/cli/security.md +++ b/docs/cli/security.md @@ -31,6 +31,7 @@ It also warns when sandbox Docker settings are configured while sandbox mode is It also warns when sandbox browser uses Docker `bridge` network without `sandbox.browser.cdpSourceRange`. It also warns when existing sandbox browser Docker containers have missing/stale hash labels (for example pre-migration containers missing `openclaw.browserConfigEpoch`) and recommends `openclaw sandbox recreate --browser --all`. It also warns when npm-based plugin/hook install records are unpinned, missing integrity metadata, or drift from currently installed package versions. +It warns when Discord allowlists (`channels.discord.allowFrom`, `channels.discord.guilds.*.users`, pairing store) use name or tag entries instead of stable IDs. It warns when `gateway.auth.mode="none"` leaves Gateway HTTP APIs reachable without a shared secret (`/tools/invoke` plus any enabled `/v1/*` endpoint). ## JSON output diff --git a/src/security/audit-channel.ts b/src/security/audit-channel.ts index be70bb00b..05ff4616b 100644 --- a/src/security/audit-channel.ts +++ b/src/security/audit-channel.ts @@ -17,6 +17,47 @@ function normalizeAllowFromList(list: Array | undefined | null) return normalizeStringEntries(Array.isArray(list) ? list : undefined); } +const DISCORD_ALLOWLIST_ID_PREFIXES = ["discord:", "user:", "pk:"] as const; + +function isDiscordNameBasedAllowEntry(raw: string | number): boolean { + const text = String(raw).trim(); + if (!text || text === "*") { + return false; + } + const maybeId = text.replace(/^<@!?/, "").replace(/>$/, ""); + if (/^\d+$/.test(maybeId)) { + return false; + } + const prefixed = DISCORD_ALLOWLIST_ID_PREFIXES.find((prefix) => text.startsWith(prefix)); + if (prefixed) { + const candidate = text.slice(prefixed.length); + if (candidate) { + return false; + } + } + return true; +} + +function addDiscordNameBasedEntries(params: { + target: Set; + values: unknown; + source: string; +}): void { + if (!Array.isArray(params.values)) { + return; + } + for (const value of params.values) { + if (!isDiscordNameBasedAllowEntry(value as string | number)) { + continue; + } + const text = String(value).trim(); + if (!text) { + continue; + } + params.target.add(`${params.source}:${text}`); + } +} + function classifyChannelWarningSeverity(message: string): SecurityAuditSeverity { const s = message.toLowerCase(); if ( @@ -141,6 +182,69 @@ export async function collectChannelSecurityFindings(params: { const discordCfg = (account as { config?: Record } | null)?.config ?? ({} as Record); + const storeAllowFrom = await readChannelAllowFromStore("discord").catch(() => []); + const discordNameBasedAllowEntries = new Set(); + addDiscordNameBasedEntries({ + target: discordNameBasedAllowEntries, + values: discordCfg.allowFrom, + source: "channels.discord.allowFrom", + }); + addDiscordNameBasedEntries({ + target: discordNameBasedAllowEntries, + values: (discordCfg.dm as { allowFrom?: unknown } | undefined)?.allowFrom, + source: "channels.discord.dm.allowFrom", + }); + addDiscordNameBasedEntries({ + target: discordNameBasedAllowEntries, + values: storeAllowFrom, + source: "~/.openclaw/credentials/discord-allowFrom.json", + }); + const discordGuildEntries = (discordCfg.guilds as Record | undefined) ?? {}; + for (const [guildKey, guildValue] of Object.entries(discordGuildEntries)) { + if (!guildValue || typeof guildValue !== "object") { + continue; + } + const guild = guildValue as Record; + addDiscordNameBasedEntries({ + target: discordNameBasedAllowEntries, + values: guild.users, + source: `channels.discord.guilds.${guildKey}.users`, + }); + const channels = guild.channels; + if (!channels || typeof channels !== "object") { + continue; + } + for (const [channelKey, channelValue] of Object.entries( + channels as Record, + )) { + if (!channelValue || typeof channelValue !== "object") { + continue; + } + const channel = channelValue as Record; + addDiscordNameBasedEntries({ + target: discordNameBasedAllowEntries, + values: channel.users, + source: `channels.discord.guilds.${guildKey}.channels.${channelKey}.users`, + }); + } + } + if (discordNameBasedAllowEntries.size > 0) { + const examples = Array.from(discordNameBasedAllowEntries).slice(0, 5); + const more = + discordNameBasedAllowEntries.size > examples.length + ? ` (+${discordNameBasedAllowEntries.size - examples.length} more)` + : ""; + findings.push({ + checkId: "channels.discord.allowFrom.name_based_entries", + severity: "warn", + title: "Discord allowlist contains name or tag entries", + detail: + "Discord name/tag allowlist matching uses normalized slugs and can collide across users. " + + `Found: ${examples.join(", ")}${more}.`, + remediation: + "Prefer stable Discord IDs (or <@id>/user:/pk:) in channels.discord.allowFrom and channels.discord.guilds.*.users.", + }); + } const nativeEnabled = resolveNativeCommandsEnabled({ providerId: "discord", providerSetting: coerceNativeSetting( @@ -160,7 +264,7 @@ export async function collectChannelSecurityFindings(params: { const defaultGroupPolicy = params.cfg.channels?.defaults?.groupPolicy; const groupPolicy = (discordCfg.groupPolicy as string | undefined) ?? defaultGroupPolicy ?? "allowlist"; - const guildEntries = (discordCfg.guilds as Record | undefined) ?? {}; + const guildEntries = discordGuildEntries; const guildsConfigured = Object.keys(guildEntries).length > 0; const hasAnyUserAllowlist = Object.values(guildEntries).some((guild) => { if (!guild || typeof guild !== "object") { @@ -184,7 +288,6 @@ export async function collectChannelSecurityFindings(params: { }); const dmAllowFromRaw = (discordCfg.dm as { allowFrom?: unknown } | undefined)?.allowFrom; const dmAllowFrom = Array.isArray(dmAllowFromRaw) ? dmAllowFromRaw : []; - const storeAllowFrom = await readChannelAllowFromStore("discord").catch(() => []); const ownerAllowFromConfigured = normalizeAllowFromList([...dmAllowFrom, ...storeAllowFrom]).length > 0; diff --git a/src/security/audit.test.ts b/src/security/audit.test.ts index b4b905df4..6d7b155d6 100644 --- a/src/security/audit.test.ts +++ b/src/security/audit.test.ts @@ -1166,6 +1166,99 @@ describe("security audit", () => { }); }); + it("warns when Discord allowlists contain name-based entries", async () => { + await withStateDir("discord-name-based-allowlist", async (tmp) => { + await fs.writeFile( + path.join(tmp, "credentials", "discord-allowFrom.json"), + JSON.stringify({ version: 1, allowFrom: ["team.owner"] }), + ); + const cfg: OpenClawConfig = { + channels: { + discord: { + enabled: true, + token: "t", + allowFrom: ["Alice#1234", "<@123456789012345678>"], + guilds: { + "123": { + users: ["trusted.operator"], + channels: { + general: { + users: ["987654321098765432", "security-team"], + }, + }, + }, + }, + }, + }, + }; + + const res = await runSecurityAudit({ + config: cfg, + includeFilesystem: false, + includeChannelSecurity: true, + plugins: [discordPlugin], + }); + + const finding = res.findings.find( + (entry) => entry.checkId === "channels.discord.allowFrom.name_based_entries", + ); + expect(finding).toBeDefined(); + expect(finding?.severity).toBe("warn"); + expect(finding?.detail).toContain("channels.discord.allowFrom:Alice#1234"); + expect(finding?.detail).toContain("channels.discord.guilds.123.users:trusted.operator"); + expect(finding?.detail).toContain( + "channels.discord.guilds.123.channels.general.users:security-team", + ); + expect(finding?.detail).toContain( + "~/.openclaw/credentials/discord-allowFrom.json:team.owner", + ); + expect(finding?.detail).not.toContain("<@123456789012345678>"); + }); + }); + + it("does not warn when Discord allowlists use ID-style entries only", async () => { + await withStateDir("discord-id-only-allowlist", async () => { + const cfg: OpenClawConfig = { + channels: { + discord: { + enabled: true, + token: "t", + allowFrom: [ + "123456789012345678", + "<@223456789012345678>", + "user:323456789012345678", + "discord:423456789012345678", + "pk:member-123", + ], + guilds: { + "123": { + users: ["523456789012345678", "<@623456789012345678>", "pk:member-456"], + channels: { + general: { + users: ["723456789012345678", "user:823456789012345678"], + }, + }, + }, + }, + }, + }, + }; + + const res = await runSecurityAudit({ + config: cfg, + includeFilesystem: false, + includeChannelSecurity: true, + plugins: [discordPlugin], + }); + + expect(res.findings).not.toEqual( + expect.arrayContaining([ + expect.objectContaining({ checkId: "channels.discord.allowFrom.name_based_entries" }), + ]), + ); + }); + }); + it("flags Discord slash commands when access-group enforcement is disabled and no users allowlist exists", async () => { await withStateDir("discord-open", async () => { const cfg: OpenClawConfig = {