From 161d9841dc6aae925c7e3d4994f7df1c5030c335 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Tue, 24 Feb 2026 01:32:23 +0000 Subject: [PATCH] refactor(security): unify dangerous name matching handling --- extensions/googlechat/src/monitor.ts | 3 +- extensions/irc/src/inbound.ts | 3 +- .../mattermost/src/mattermost/monitor.ts | 3 +- .../src/monitor-handler/message-handler.ts | 9 +- src/commands/doctor-config-flow.ts | 167 +---- src/config/dangerous-name-matching.ts | 84 +++ src/discord/monitor/agent-components.ts | 17 +- .../monitor/message-handler.preflight.ts | 7 +- .../monitor/message-handler.process.ts | 3 +- src/discord/monitor/native-command.ts | 9 +- src/discord/monitor/provider.ts | 5 +- src/discord/voice/command.ts | 5 +- src/plugin-sdk/index.ts | 1 + src/security/audit-channel.ts | 599 +++++++++--------- src/security/audit.test.ts | 123 +++- src/security/mutable-allowlist-detectors.ts | 101 +++ src/slack/monitor/provider.ts | 3 +- 17 files changed, 671 insertions(+), 471 deletions(-) create mode 100644 src/config/dangerous-name-matching.ts create mode 100644 src/security/mutable-allowlist-detectors.ts diff --git a/extensions/googlechat/src/monitor.ts b/extensions/googlechat/src/monitor.ts index 19bd2f6421a..c7529489695 100644 --- a/extensions/googlechat/src/monitor.ts +++ b/extensions/googlechat/src/monitor.ts @@ -6,6 +6,7 @@ import { readJsonBodyWithLimit, registerWebhookTarget, rejectNonPostWebhookRequest, + isDangerousNameMatchingEnabled, resolveAllowlistProviderRuntimeGroupPolicy, resolveDefaultGroupPolicy, resolveSingleWebhookTargetAsync, @@ -410,7 +411,7 @@ async function processMessageWithPipeline(params: { const senderId = sender?.name ?? ""; const senderName = sender?.displayName ?? ""; const senderEmail = sender?.email ?? undefined; - const allowNameMatching = account.config.dangerouslyAllowNameMatching === true; + const allowNameMatching = isDangerousNameMatchingEnabled(account.config); const allowBots = account.config.allowBots === true; if (!allowBots) { diff --git a/extensions/irc/src/inbound.ts b/extensions/irc/src/inbound.ts index db2bd584f14..26d0aa85927 100644 --- a/extensions/irc/src/inbound.ts +++ b/extensions/irc/src/inbound.ts @@ -4,6 +4,7 @@ import { createReplyPrefixOptions, formatTextWithAttachmentLinks, logInboundDrop, + isDangerousNameMatchingEnabled, resolveControlCommandGate, resolveOutboundMediaUrls, resolveAllowlistProviderRuntimeGroupPolicy, @@ -78,7 +79,7 @@ export async function handleIrcInbound(params: { const senderDisplay = message.senderHost ? `${message.senderNick}!${message.senderUser ?? "?"}@${message.senderHost}` : message.senderNick; - const allowNameMatching = account.config.dangerouslyAllowNameMatching === true; + const allowNameMatching = isDangerousNameMatchingEnabled(account.config); const dmPolicy = account.config.dmPolicy ?? "pairing"; const defaultGroupPolicy = resolveDefaultGroupPolicy(config); diff --git a/extensions/mattermost/src/mattermost/monitor.ts b/extensions/mattermost/src/mattermost/monitor.ts index 3baa129d6fb..fe799a295c9 100644 --- a/extensions/mattermost/src/mattermost/monitor.ts +++ b/extensions/mattermost/src/mattermost/monitor.ts @@ -15,6 +15,7 @@ import { clearHistoryEntriesIfEnabled, DEFAULT_GROUP_HISTORY_LIMIT, recordPendingHistoryEntryIfEnabled, + isDangerousNameMatchingEnabled, resolveControlCommandGate, resolveAllowlistProviderRuntimeGroupPolicy, resolveDefaultGroupPolicy, @@ -212,7 +213,7 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {} cfg, accountId: opts.accountId, }); - const allowNameMatching = account.config.dangerouslyAllowNameMatching === true; + const allowNameMatching = isDangerousNameMatchingEnabled(account.config); const botToken = opts.botToken?.trim() || account.botToken?.trim(); if (!botToken) { throw new Error( diff --git a/extensions/msteams/src/monitor-handler/message-handler.ts b/extensions/msteams/src/monitor-handler/message-handler.ts index 332a354a2fc..085efeeb0a8 100644 --- a/extensions/msteams/src/monitor-handler/message-handler.ts +++ b/extensions/msteams/src/monitor-handler/message-handler.ts @@ -6,6 +6,7 @@ import { recordPendingHistoryEntryIfEnabled, resolveControlCommandGate, resolveDefaultGroupPolicy, + isDangerousNameMatchingEnabled, resolveMentionGating, formatAllowlistMatchMeta, type HistoryEntry, @@ -145,7 +146,7 @@ export function createMSTeamsMessageHandler(deps: MSTeamsMessageHandlerDeps) { if (dmPolicy !== "open") { const effectiveAllowFrom = [...allowFrom.map((v) => String(v)), ...storedAllowFrom]; - const allowNameMatching = msteamsCfg.dangerouslyAllowNameMatching === true; + const allowNameMatching = isDangerousNameMatchingEnabled(msteamsCfg); const allowMatch = resolveMSTeamsAllowlistMatch({ allowFrom: effectiveAllowFrom, senderId, @@ -228,7 +229,7 @@ export function createMSTeamsMessageHandler(deps: MSTeamsMessageHandlerDeps) { return; } if (effectiveGroupAllowFrom.length > 0) { - const allowNameMatching = msteamsCfg.dangerouslyAllowNameMatching === true; + const allowNameMatching = isDangerousNameMatchingEnabled(msteamsCfg); const allowMatch = resolveMSTeamsAllowlistMatch({ allowFrom: effectiveGroupAllowFrom, senderId, @@ -252,14 +253,14 @@ export function createMSTeamsMessageHandler(deps: MSTeamsMessageHandlerDeps) { allowFrom: effectiveDmAllowFrom, senderId, senderName, - allowNameMatching: msteamsCfg?.dangerouslyAllowNameMatching === true, + allowNameMatching: isDangerousNameMatchingEnabled(msteamsCfg), }); const groupAllowedForCommands = isMSTeamsGroupAllowed({ groupPolicy: "allowlist", allowFrom: effectiveGroupAllowFrom, senderId, senderName, - allowNameMatching: msteamsCfg?.dangerouslyAllowNameMatching === true, + allowNameMatching: isDangerousNameMatchingEnabled(msteamsCfg), }); const hasControlCommandInMessage = core.channel.text.hasControlCommand(text, cfg); const commandGate = resolveControlCommandGate({ diff --git a/src/commands/doctor-config-flow.ts b/src/commands/doctor-config-flow.ts index b3df903e081..e86dec9e819 100644 --- a/src/commands/doctor-config-flow.ts +++ b/src/commands/doctor-config-flow.ts @@ -14,12 +14,21 @@ import { migrateLegacyConfig, readConfigFileSnapshot, } from "../config/config.js"; +import { collectProviderDangerousNameMatchingScopes } from "../config/dangerous-name-matching.js"; import { applyPluginAutoEnable } from "../config/plugin-auto-enable.js"; import { parseToolsBySenderTypedKey } from "../config/types.tools.js"; import { listInterpreterLikeSafeBins, resolveMergedSafeBinProfileFixtures, } from "../infra/exec-safe-bin-runtime-policy.js"; +import { + isDiscordMutableAllowEntry, + isGoogleChatMutableAllowEntry, + isIrcMutableAllowEntry, + isMSTeamsMutableAllowEntry, + isMattermostMutableAllowEntry, + isSlackMutableAllowEntry, +} from "../security/mutable-allowlist-detectors.js"; import { listTelegramAccountIds, resolveTelegramAccount } from "../telegram/accounts.js"; import { note } from "../terminal/note.js"; import { isRecord, resolveHomeDir } from "../utils.js"; @@ -192,10 +201,6 @@ function asObjectRecord(value: unknown): Record | null { return value as Record; } -function asOptionalBoolean(value: unknown): boolean | undefined { - return typeof value === "boolean" ? value : undefined; -} - function collectTelegramAccountScopes( cfg: OpenClawConfig, ): Array<{ prefix: string; account: Record }> { @@ -589,148 +594,6 @@ type MutableAllowlistHit = { dangerousFlagPath: string; }; -type ProviderAccountScope = { - prefix: string; - account: Record; - dangerousNameMatchingEnabled: boolean; - dangerousFlagPath: string; -}; - -function collectProviderAccountScopes( - cfg: OpenClawConfig, - provider: string, -): ProviderAccountScope[] { - const scopes: ProviderAccountScope[] = []; - const channels = asObjectRecord(cfg.channels); - if (!channels) { - return scopes; - } - const providerCfg = asObjectRecord(channels[provider]); - if (!providerCfg) { - return scopes; - } - const providerPrefix = `channels.${provider}`; - const providerDangerousFlagPath = `${providerPrefix}.dangerouslyAllowNameMatching`; - const providerDangerousNameMatchingEnabled = providerCfg.dangerouslyAllowNameMatching === true; - scopes.push({ - prefix: providerPrefix, - account: providerCfg, - dangerousNameMatchingEnabled: providerDangerousNameMatchingEnabled, - dangerousFlagPath: providerDangerousFlagPath, - }); - const accounts = asObjectRecord(providerCfg.accounts); - if (!accounts) { - return scopes; - } - for (const key of Object.keys(accounts)) { - const account = asObjectRecord(accounts[key]); - if (!account) { - continue; - } - const accountPrefix = `${providerPrefix}.accounts.${key}`; - const accountDangerousNameMatching = asOptionalBoolean(account.dangerouslyAllowNameMatching); - scopes.push({ - prefix: accountPrefix, - account, - dangerousNameMatchingEnabled: - accountDangerousNameMatching ?? providerDangerousNameMatchingEnabled, - dangerousFlagPath: - accountDangerousNameMatching == null - ? providerDangerousFlagPath - : `${accountPrefix}.dangerouslyAllowNameMatching`, - }); - } - return scopes; -} - -function isDiscordMutableAllowEntry(raw: string): boolean { - const text = raw.trim(); - if (!text || text === "*") { - return false; - } - const maybeMentionId = text.replace(/^<@!?/, "").replace(/>$/, ""); - if (/^\d+$/.test(maybeMentionId)) { - return false; - } - for (const prefix of ["discord:", "user:", "pk:"]) { - if (!text.startsWith(prefix)) { - continue; - } - return text.slice(prefix.length).trim().length === 0; - } - return true; -} - -function isSlackMutableAllowEntry(raw: string): boolean { - const text = raw.trim(); - if (!text || text === "*") { - return false; - } - const mentionMatch = text.match(/^<@([A-Z0-9]+)>$/i); - if (mentionMatch && /^[A-Z0-9]{8,}$/i.test(mentionMatch[1] ?? "")) { - return false; - } - const withoutPrefix = text.replace(/^(slack|user):/i, "").trim(); - if (/^[UWBCGDT][A-Z0-9]{2,}$/.test(withoutPrefix)) { - return false; - } - if (/^[A-Z0-9]{8,}$/i.test(withoutPrefix)) { - return false; - } - return true; -} - -function isGoogleChatMutableAllowEntry(raw: string): boolean { - const text = raw.trim(); - if (!text || text === "*") { - return false; - } - const withoutPrefix = text.replace(/^(googlechat|google-chat|gchat):/i, "").trim(); - if (!withoutPrefix) { - return false; - } - const withoutUsers = withoutPrefix.replace(/^users\//i, ""); - return withoutUsers.includes("@"); -} - -function isMSTeamsMutableAllowEntry(raw: string): boolean { - const text = raw.trim(); - if (!text || text === "*") { - return false; - } - const withoutPrefix = text.replace(/^(msteams|user):/i, "").trim(); - return /\s/.test(withoutPrefix) || withoutPrefix.includes("@"); -} - -function isMattermostMutableAllowEntry(raw: string): boolean { - const text = raw.trim(); - if (!text || text === "*") { - return false; - } - const normalized = text - .replace(/^(mattermost|user):/i, "") - .replace(/^@/, "") - .trim() - .toLowerCase(); - // Mattermost user IDs are stable 26-char lowercase/number tokens. - if (/^[a-z0-9]{26}$/.test(normalized)) { - return false; - } - return true; -} - -function isIrcMutableAllowEntry(raw: string): boolean { - const text = raw.trim().toLowerCase(); - if (!text || text === "*") { - return false; - } - const normalized = text - .replace(/^irc:/, "") - .replace(/^user:/, "") - .trim(); - return !normalized.includes("!") && !normalized.includes("@"); -} - function addMutableAllowlistHits(params: { hits: MutableAllowlistHit[]; pathLabel: string; @@ -762,7 +625,7 @@ function addMutableAllowlistHits(params: { function scanMutableAllowlistEntries(cfg: OpenClawConfig): MutableAllowlistHit[] { const hits: MutableAllowlistHit[] = []; - for (const scope of collectProviderAccountScopes(cfg, "discord")) { + for (const scope of collectProviderDangerousNameMatchingScopes(cfg, "discord")) { if (scope.dangerousNameMatchingEnabled) { continue; } @@ -823,7 +686,7 @@ function scanMutableAllowlistEntries(cfg: OpenClawConfig): MutableAllowlistHit[] } } - for (const scope of collectProviderAccountScopes(cfg, "slack")) { + for (const scope of collectProviderDangerousNameMatchingScopes(cfg, "slack")) { if (scope.dangerousNameMatchingEnabled) { continue; } @@ -866,7 +729,7 @@ function scanMutableAllowlistEntries(cfg: OpenClawConfig): MutableAllowlistHit[] } } - for (const scope of collectProviderAccountScopes(cfg, "googlechat")) { + for (const scope of collectProviderDangerousNameMatchingScopes(cfg, "googlechat")) { if (scope.dangerousNameMatchingEnabled) { continue; } @@ -909,7 +772,7 @@ function scanMutableAllowlistEntries(cfg: OpenClawConfig): MutableAllowlistHit[] } } - for (const scope of collectProviderAccountScopes(cfg, "msteams")) { + for (const scope of collectProviderDangerousNameMatchingScopes(cfg, "msteams")) { if (scope.dangerousNameMatchingEnabled) { continue; } @@ -931,7 +794,7 @@ function scanMutableAllowlistEntries(cfg: OpenClawConfig): MutableAllowlistHit[] }); } - for (const scope of collectProviderAccountScopes(cfg, "mattermost")) { + for (const scope of collectProviderDangerousNameMatchingScopes(cfg, "mattermost")) { if (scope.dangerousNameMatchingEnabled) { continue; } @@ -953,7 +816,7 @@ function scanMutableAllowlistEntries(cfg: OpenClawConfig): MutableAllowlistHit[] }); } - for (const scope of collectProviderAccountScopes(cfg, "irc")) { + for (const scope of collectProviderDangerousNameMatchingScopes(cfg, "irc")) { if (scope.dangerousNameMatchingEnabled) { continue; } diff --git a/src/config/dangerous-name-matching.ts b/src/config/dangerous-name-matching.ts new file mode 100644 index 00000000000..c911d3f2361 --- /dev/null +++ b/src/config/dangerous-name-matching.ts @@ -0,0 +1,84 @@ +import type { OpenClawConfig } from "./config.js"; + +export type DangerousNameMatchingConfig = { + dangerouslyAllowNameMatching?: boolean; +}; + +export type ProviderDangerousNameMatchingScope = { + prefix: string; + account: Record; + dangerousNameMatchingEnabled: boolean; + dangerousFlagPath: string; +}; + +function asObjectRecord(value: unknown): Record | null { + if (!value || typeof value !== "object" || Array.isArray(value)) { + return null; + } + return value as Record; +} + +function asOptionalBoolean(value: unknown): boolean | undefined { + return typeof value === "boolean" ? value : undefined; +} + +export function isDangerousNameMatchingEnabled( + config: DangerousNameMatchingConfig | null | undefined, +): boolean { + return config?.dangerouslyAllowNameMatching === true; +} + +export function collectProviderDangerousNameMatchingScopes( + cfg: OpenClawConfig, + provider: string, +): ProviderDangerousNameMatchingScope[] { + const scopes: ProviderDangerousNameMatchingScope[] = []; + const channels = asObjectRecord(cfg.channels); + if (!channels) { + return scopes; + } + + const providerCfg = asObjectRecord(channels[provider]); + if (!providerCfg) { + return scopes; + } + + const providerPrefix = `channels.${provider}`; + const providerDangerousFlagPath = `${providerPrefix}.dangerouslyAllowNameMatching`; + const providerDangerousNameMatchingEnabled = isDangerousNameMatchingEnabled(providerCfg); + + scopes.push({ + prefix: providerPrefix, + account: providerCfg, + dangerousNameMatchingEnabled: providerDangerousNameMatchingEnabled, + dangerousFlagPath: providerDangerousFlagPath, + }); + + const accounts = asObjectRecord(providerCfg.accounts); + if (!accounts) { + return scopes; + } + + for (const key of Object.keys(accounts)) { + const account = asObjectRecord(accounts[key]); + if (!account) { + continue; + } + + const accountPrefix = `${providerPrefix}.accounts.${key}`; + const accountDangerousNameMatching = asOptionalBoolean(account.dangerouslyAllowNameMatching); + + scopes.push({ + prefix: accountPrefix, + account, + dangerousNameMatchingEnabled: + accountDangerousNameMatching ?? providerDangerousNameMatchingEnabled, + dangerousFlagPath: + accountDangerousNameMatching == null + ? providerDangerousFlagPath + : `${accountPrefix}.dangerouslyAllowNameMatching`, + }); + } + + return scopes; +} diff --git a/src/discord/monitor/agent-components.ts b/src/discord/monitor/agent-components.ts index 112ab78f9ac..c4d31780311 100644 --- a/src/discord/monitor/agent-components.ts +++ b/src/discord/monitor/agent-components.ts @@ -26,6 +26,7 @@ import { createReplyReferencePlanner } from "../../auto-reply/reply/reply-refere import { createReplyPrefixOptions } from "../../channels/reply-prefix.js"; import { recordInboundSession } from "../../channels/session.js"; import type { OpenClawConfig } from "../../config/config.js"; +import { isDangerousNameMatchingEnabled } from "../../config/dangerous-name-matching.js"; import { resolveMarkdownTableMode } from "../../config/markdown-tables.js"; import { readSessionUpdatedAt, resolveStorePath } from "../../config/sessions.js"; import type { DiscordAccountConfig } from "../../config/types.discord.js"; @@ -365,7 +366,7 @@ async function ensureAgentComponentInteractionAllowed(params: { replyOpts: params.replyOpts, componentLabel: params.componentLabel, unauthorizedReply: params.unauthorizedReply, - allowNameMatching: params.ctx.discordConfig?.dangerouslyAllowNameMatching === true, + allowNameMatching: isDangerousNameMatchingEnabled(params.ctx.discordConfig), }); if (!memberAllowed) { return null; @@ -481,7 +482,7 @@ async function ensureDmComponentAuthorized(params: { name: user.username, tag: formatDiscordUserTag(user), }, - allowNameMatching: ctx.discordConfig?.dangerouslyAllowNameMatching === true, + allowNameMatching: isDangerousNameMatchingEnabled(ctx.discordConfig), }) : { allowed: false }; if (allowMatch.allowed) { @@ -784,7 +785,7 @@ async function dispatchDiscordComponentEvent(params: { channelConfig, guildInfo, sender: { id: interactionCtx.user.id, name: interactionCtx.user.username, tag: senderTag }, - allowNameMatching: ctx.discordConfig?.dangerouslyAllowNameMatching === true, + allowNameMatching: isDangerousNameMatchingEnabled(ctx.discordConfig), }); const storePath = resolveStorePath(ctx.cfg.session?.store, { agentId }); const envelopeOptions = resolveEnvelopeFormatOptions(ctx.cfg); @@ -982,7 +983,7 @@ async function handleDiscordComponentEvent(params: { replyOpts, componentLabel: params.componentLabel, unauthorizedReply, - allowNameMatching: params.ctx.discordConfig?.dangerouslyAllowNameMatching === true, + allowNameMatching: isDangerousNameMatchingEnabled(params.ctx.discordConfig), }); if (!memberAllowed) { return; @@ -995,7 +996,7 @@ async function handleDiscordComponentEvent(params: { replyOpts, componentLabel: params.componentLabel, unauthorizedReply, - allowNameMatching: params.ctx.discordConfig?.dangerouslyAllowNameMatching === true, + allowNameMatching: isDangerousNameMatchingEnabled(params.ctx.discordConfig), }); if (!componentAllowed) { return; @@ -1134,7 +1135,7 @@ async function handleDiscordModalTrigger(params: { replyOpts, componentLabel: "form", unauthorizedReply, - allowNameMatching: params.ctx.discordConfig?.dangerouslyAllowNameMatching === true, + allowNameMatching: isDangerousNameMatchingEnabled(params.ctx.discordConfig), }); if (!memberAllowed) { return; @@ -1147,7 +1148,7 @@ async function handleDiscordModalTrigger(params: { replyOpts, componentLabel: "form", unauthorizedReply, - allowNameMatching: params.ctx.discordConfig?.dangerouslyAllowNameMatching === true, + allowNameMatching: isDangerousNameMatchingEnabled(params.ctx.discordConfig), }); if (!componentAllowed) { return; @@ -1583,7 +1584,7 @@ class DiscordComponentModal extends Modal { replyOpts, componentLabel: "form", unauthorizedReply: "You are not authorized to use this form.", - allowNameMatching: this.ctx.discordConfig?.dangerouslyAllowNameMatching === true, + allowNameMatching: isDangerousNameMatchingEnabled(this.ctx.discordConfig), }); if (!memberAllowed) { return; diff --git a/src/discord/monitor/message-handler.preflight.ts b/src/discord/monitor/message-handler.preflight.ts index 67dfc58e1c1..e321c8ef86f 100644 --- a/src/discord/monitor/message-handler.preflight.ts +++ b/src/discord/monitor/message-handler.preflight.ts @@ -14,6 +14,7 @@ import { resolveControlCommandGate } from "../../channels/command-gating.js"; import { logInboundDrop } from "../../channels/logging.js"; import { resolveMentionGatingWithBypass } from "../../channels/mention-gating.js"; import { loadConfig } from "../../config/config.js"; +import { isDangerousNameMatchingEnabled } from "../../config/dangerous-name-matching.js"; import { logVerbose, shouldLogVerbose } from "../../globals.js"; import { recordChannelActivity } from "../../infra/channel-activity.js"; import { enqueueSystemEvent } from "../../infra/system-events.js"; @@ -190,7 +191,7 @@ export async function preflightDiscordMessage( name: sender.name, tag: sender.tag, }, - allowNameMatching: params.discordConfig?.dangerouslyAllowNameMatching === true, + allowNameMatching: isDangerousNameMatchingEnabled(params.discordConfig), }) : { allowed: false }; const allowMatchMeta = formatAllowlistMatchMeta(allowMatch); @@ -564,7 +565,7 @@ export async function preflightDiscordMessage( guildInfo, memberRoleIds, sender, - allowNameMatching: params.discordConfig?.dangerouslyAllowNameMatching === true, + allowNameMatching: isDangerousNameMatchingEnabled(params.discordConfig), }); if (!isDirectMessage) { @@ -581,7 +582,7 @@ export async function preflightDiscordMessage( name: sender.name, tag: sender.tag, }, - { allowNameMatching: params.discordConfig?.dangerouslyAllowNameMatching === true }, + { allowNameMatching: isDangerousNameMatchingEnabled(params.discordConfig) }, ) : false; const useAccessGroups = params.cfg.commands?.useAccessGroups !== false; diff --git a/src/discord/monitor/message-handler.process.ts b/src/discord/monitor/message-handler.process.ts index 8c0530b8135..2d5b4058f6e 100644 --- a/src/discord/monitor/message-handler.process.ts +++ b/src/discord/monitor/message-handler.process.ts @@ -21,6 +21,7 @@ import { type StatusReactionAdapter, } from "../../channels/status-reactions.js"; import { createTypingCallbacks } from "../../channels/typing.js"; +import { isDangerousNameMatchingEnabled } from "../../config/dangerous-name-matching.js"; import { resolveDiscordPreviewStreamMode } from "../../config/discord-preview-streaming.js"; import { resolveMarkdownTableMode } from "../../config/markdown-tables.js"; import { readSessionUpdatedAt, resolveStorePath } from "../../config/sessions.js"; @@ -199,7 +200,7 @@ export async function processDiscordMessage(ctx: DiscordMessagePreflightContext) channelConfig, guildInfo, sender: { id: sender.id, name: sender.name, tag: sender.tag }, - allowNameMatching: discordConfig?.dangerouslyAllowNameMatching === true, + allowNameMatching: isDangerousNameMatchingEnabled(discordConfig), }); const storePath = resolveStorePath(cfg.session?.store, { agentId: route.agentId, diff --git a/src/discord/monitor/native-command.ts b/src/discord/monitor/native-command.ts index fc2fdee2fb6..1629f03fba1 100644 --- a/src/discord/monitor/native-command.ts +++ b/src/discord/monitor/native-command.ts @@ -39,6 +39,7 @@ import type { ReplyPayload } from "../../auto-reply/types.js"; import { resolveCommandAuthorizedFromAuthorizers } from "../../channels/command-gating.js"; import { createReplyPrefixOptions } from "../../channels/reply-prefix.js"; import type { OpenClawConfig, loadConfig } from "../../config/config.js"; +import { isDangerousNameMatchingEnabled } from "../../config/dangerous-name-matching.js"; import { resolveOpenProviderRuntimeGroupPolicy } from "../../config/runtime-group-policy.js"; import { loadSessionStore, resolveStorePath } from "../../config/sessions.js"; import { logVerbose } from "../../globals.js"; @@ -1283,7 +1284,7 @@ async function dispatchDiscordCommandInteraction(params: { name: sender.name, tag: sender.tag, }, - { allowNameMatching: discordConfig?.dangerouslyAllowNameMatching === true }, + { allowNameMatching: isDangerousNameMatchingEnabled(discordConfig) }, ) : false; const guildInfo = resolveDiscordGuildEntry({ @@ -1374,7 +1375,7 @@ async function dispatchDiscordCommandInteraction(params: { name: sender.name, tag: sender.tag, }, - { allowNameMatching: discordConfig?.dangerouslyAllowNameMatching === true }, + { allowNameMatching: isDangerousNameMatchingEnabled(discordConfig) }, ) : false; if (!permitted) { @@ -1412,7 +1413,7 @@ async function dispatchDiscordCommandInteraction(params: { guildInfo, memberRoleIds, sender, - allowNameMatching: discordConfig?.dangerouslyAllowNameMatching === true, + allowNameMatching: isDangerousNameMatchingEnabled(discordConfig), }); const authorizers = useAccessGroups ? [ @@ -1518,7 +1519,7 @@ async function dispatchDiscordCommandInteraction(params: { channelConfig, guildInfo, sender: { id: sender.id, name: sender.name, tag: sender.tag }, - allowNameMatching: discordConfig?.dangerouslyAllowNameMatching === true, + allowNameMatching: isDangerousNameMatchingEnabled(discordConfig), }); const ctxPayload = finalizeInboundContext({ Body: prompt, diff --git a/src/discord/monitor/provider.ts b/src/discord/monitor/provider.ts index 3d72677b465..b31697189de 100644 --- a/src/discord/monitor/provider.ts +++ b/src/discord/monitor/provider.ts @@ -21,6 +21,7 @@ import { } from "../../config/commands.js"; import type { OpenClawConfig, ReplyToMode } from "../../config/config.js"; import { loadConfig } from "../../config/config.js"; +import { isDangerousNameMatchingEnabled } from "../../config/dangerous-name-matching.js"; import { GROUP_POLICY_BLOCKED_LABEL, resolveOpenProviderRuntimeGroupPolicy, @@ -559,7 +560,7 @@ export async function monitorDiscordProvider(opts: MonitorDiscordOpts = {}) { accountId: account.accountId, runtime, botUserId, - allowNameMatching: discordCfg.dangerouslyAllowNameMatching === true, + allowNameMatching: isDangerousNameMatchingEnabled(discordCfg), guildEntries, logger, }), @@ -571,7 +572,7 @@ export async function monitorDiscordProvider(opts: MonitorDiscordOpts = {}) { accountId: account.accountId, runtime, botUserId, - allowNameMatching: discordCfg.dangerouslyAllowNameMatching === true, + allowNameMatching: isDangerousNameMatchingEnabled(discordCfg), guildEntries, logger, }), diff --git a/src/discord/voice/command.ts b/src/discord/voice/command.ts index 831d7e42e91..adb3e6ca879 100644 --- a/src/discord/voice/command.ts +++ b/src/discord/voice/command.ts @@ -12,6 +12,7 @@ import { } from "discord-api-types/v10"; import { resolveCommandAuthorizedFromAuthorizers } from "../../channels/command-gating.js"; import type { OpenClawConfig } from "../../config/config.js"; +import { isDangerousNameMatchingEnabled } from "../../config/dangerous-name-matching.js"; import type { DiscordAccountConfig } from "../../config/types.js"; import { allowListMatches, @@ -156,7 +157,7 @@ async function authorizeVoiceCommand( guildInfo, memberRoleIds, sender, - allowNameMatching: params.discordConfig?.dangerouslyAllowNameMatching === true, + allowNameMatching: isDangerousNameMatchingEnabled(params.discordConfig), }); const ownerAllowList = normalizeDiscordAllowList( @@ -171,7 +172,7 @@ async function authorizeVoiceCommand( name: sender.name, tag: sender.tag, }, - { allowNameMatching: params.discordConfig?.dangerouslyAllowNameMatching === true }, + { allowNameMatching: isDangerousNameMatchingEnabled(params.discordConfig) }, ) : false; diff --git a/src/plugin-sdk/index.ts b/src/plugin-sdk/index.ts index bc675fb36b6..461370054b0 100644 --- a/src/plugin-sdk/index.ts +++ b/src/plugin-sdk/index.ts @@ -91,6 +91,7 @@ export { emptyPluginConfigSchema } from "../plugins/config-schema.js"; export type { OpenClawConfig } from "../config/config.js"; /** @deprecated Use OpenClawConfig instead */ export type { OpenClawConfig as ClawdbotConfig } from "../config/config.js"; +export { isDangerousNameMatchingEnabled } from "../config/dangerous-name-matching.js"; export type { FileLockHandle, FileLockOptions } from "./file-lock.js"; export { acquireFileLock, withFileLock } from "./file-lock.js"; diff --git a/src/security/audit-channel.ts b/src/security/audit-channel.ts index f403f059ec1..dcf344891cf 100644 --- a/src/security/audit-channel.ts +++ b/src/security/audit-channel.ts @@ -8,36 +8,17 @@ import { import { formatCliCommand } from "../cli/command-format.js"; import { resolveNativeCommandsEnabled, resolveNativeSkillsEnabled } from "../config/commands.js"; import type { OpenClawConfig } from "../config/config.js"; +import { isDangerousNameMatchingEnabled } from "../config/dangerous-name-matching.js"; import { readChannelAllowFromStore } from "../pairing/pairing-store.js"; import { normalizeStringEntries } from "../shared/string-normalization.js"; import type { SecurityAuditFinding, SecurityAuditSeverity } from "./audit.js"; import { resolveDmAllowState } from "./dm-policy-shared.js"; +import { isDiscordMutableAllowEntry } from "./mutable-allowlist-detectors.js"; function normalizeAllowFromList(list: Array | undefined | null): string[] { 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; @@ -47,7 +28,7 @@ function addDiscordNameBasedEntries(params: { return; } for (const value of params.values) { - if (!isDiscordNameBasedAllowEntry(value as string | number)) { + if (!isDiscordMutableAllowEntry(String(value))) { continue; } const text = String(value).trim(); @@ -76,6 +57,42 @@ function classifyChannelWarningSeverity(message: string): SecurityAuditSeverity return "warn"; } +function dedupeFindings(findings: SecurityAuditFinding[]): SecurityAuditFinding[] { + const seen = new Set(); + const out: SecurityAuditFinding[] = []; + for (const finding of findings) { + const key = [ + finding.checkId, + finding.severity, + finding.title, + finding.detail ?? "", + finding.remediation ?? "", + ].join("\n"); + if (seen.has(key)) { + continue; + } + seen.add(key); + out.push(finding); + } + return out; +} + +function hasExplicitProviderAccountConfig( + cfg: OpenClawConfig, + provider: string, + accountId: string, +): boolean { + const channel = cfg.channels?.[provider]; + if (!channel || typeof channel !== "object") { + return false; + } + const accounts = (channel as { accounts?: Record }).accounts; + if (!accounts || typeof accounts !== "object") { + return false; + } + return accountId in accounts; +} + export async function collectChannelSecurityFindings(params: { cfg: OpenClawConfig; plugins: ReturnType; @@ -166,299 +183,317 @@ export async function collectChannelSecurityFindings(params: { cfg: params.cfg, accountIds, }); - const account = plugin.config.resolveAccount(params.cfg, defaultAccountId); - const enabled = plugin.config.isEnabled ? plugin.config.isEnabled(account, params.cfg) : true; - if (!enabled) { - continue; - } - const configured = plugin.config.isConfigured - ? await plugin.config.isConfigured(account, params.cfg) - : true; - if (!configured) { - continue; - } + const orderedAccountIds = Array.from(new Set([defaultAccountId, ...accountIds])); - const accountConfig = (account as { config?: Record } | null | undefined) - ?.config; - if (accountConfig?.dangerouslyAllowNameMatching === true) { - findings.push({ - checkId: `channels.${plugin.id}.allowFrom.dangerous_name_matching_enabled`, - severity: "info", - title: `${plugin.meta.label ?? plugin.id} dangerous name matching is enabled`, - detail: - "dangerouslyAllowNameMatching=true re-enables mutable name/email/tag matching for sender authorization. This is a break-glass compatibility mode, not a hardened default.", - remediation: - "Prefer stable sender IDs in allowlists, then disable dangerouslyAllowNameMatching.", - }); - } + for (const accountId of orderedAccountIds) { + const hasExplicitAccountPath = hasExplicitProviderAccountConfig( + params.cfg, + plugin.id, + accountId, + ); + const account = plugin.config.resolveAccount(params.cfg, accountId); + const enabled = plugin.config.isEnabled ? plugin.config.isEnabled(account, params.cfg) : true; + if (!enabled) { + continue; + } + const configured = plugin.config.isConfigured + ? await plugin.config.isConfigured(account, params.cfg) + : true; + if (!configured) { + continue; + } - if (plugin.id === "discord") { - const discordCfg = - (account as { config?: Record } | null)?.config ?? - ({} as Record); - const dangerousNameMatchingEnabled = discordCfg.dangerouslyAllowNameMatching === true; - 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; + const accountConfig = (account as { config?: Record } | null | undefined) + ?.config; + if (isDangerousNameMatchingEnabled(accountConfig)) { + const accountNote = + orderedAccountIds.length > 1 || hasExplicitAccountPath ? ` (account: ${accountId})` : ""; + findings.push({ + checkId: `channels.${plugin.id}.allowFrom.dangerous_name_matching_enabled`, + severity: "info", + title: `${plugin.meta.label ?? plugin.id} dangerous name matching is enabled${accountNote}`, + detail: + "dangerouslyAllowNameMatching=true re-enables mutable name/email/tag matching for sender authorization. This is a break-glass compatibility mode, not a hardened default.", + remediation: + "Prefer stable sender IDs in allowlists, then disable dangerouslyAllowNameMatching.", + }); + } + + if (plugin.id === "discord") { + const discordCfg = + (account as { config?: Record } | null)?.config ?? + ({} as Record); + const dangerousNameMatchingEnabled = isDangerousNameMatchingEnabled(discordCfg); + const storeAllowFrom = await readChannelAllowFromStore("discord").catch(() => []); + const discordNameBasedAllowEntries = new Set(); + const discordPathPrefix = + orderedAccountIds.length > 1 || hasExplicitAccountPath + ? `channels.discord.accounts.${accountId}` + : "channels.discord"; addDiscordNameBasedEntries({ target: discordNameBasedAllowEntries, - values: guild.users, - source: `channels.discord.guilds.${guildKey}.users`, + values: discordCfg.allowFrom, + source: `${discordPathPrefix}.allowFrom`, }); - 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") { + addDiscordNameBasedEntries({ + target: discordNameBasedAllowEntries, + values: (discordCfg.dm as { allowFrom?: unknown } | undefined)?.allowFrom, + source: `${discordPathPrefix}.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 channel = channelValue as Record; + const guild = guildValue as Record; addDiscordNameBasedEntries({ target: discordNameBasedAllowEntries, - values: channel.users, - source: `channels.discord.guilds.${guildKey}.channels.${channelKey}.users`, + values: guild.users, + source: `${discordPathPrefix}.guilds.${guildKey}.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: dangerousNameMatchingEnabled ? "info" : "warn", - title: dangerousNameMatchingEnabled - ? "Discord allowlist uses break-glass name/tag matching" - : "Discord allowlist contains name or tag entries", - detail: dangerousNameMatchingEnabled - ? "Discord name/tag allowlist matching is explicitly enabled via dangerouslyAllowNameMatching. This mutable-identity mode is operator-selected break-glass behavior and out-of-scope for vulnerability reports by itself. " + - `Found: ${examples.join(", ")}${more}.` - : "Discord name/tag allowlist matching uses normalized slugs and can collide across users. " + - `Found: ${examples.join(", ")}${more}.`, - remediation: dangerousNameMatchingEnabled - ? "Prefer stable Discord IDs (or <@id>/user:/pk:), then disable dangerouslyAllowNameMatching." - : "Prefer stable Discord IDs (or <@id>/user:/pk:) in channels.discord.allowFrom and channels.discord.guilds.*.users, or explicitly opt in with dangerouslyAllowNameMatching=true if you accept the risk.", - }); - } - const nativeEnabled = resolveNativeCommandsEnabled({ - providerId: "discord", - providerSetting: coerceNativeSetting( - (discordCfg.commands as { native?: unknown } | undefined)?.native, - ), - globalSetting: params.cfg.commands?.native, - }); - const nativeSkillsEnabled = resolveNativeSkillsEnabled({ - providerId: "discord", - providerSetting: coerceNativeSetting( - (discordCfg.commands as { nativeSkills?: unknown } | undefined)?.nativeSkills, - ), - globalSetting: params.cfg.commands?.nativeSkills, - }); - const slashEnabled = nativeEnabled || nativeSkillsEnabled; - if (slashEnabled) { - const defaultGroupPolicy = params.cfg.channels?.defaults?.groupPolicy; - const groupPolicy = - (discordCfg.groupPolicy as string | undefined) ?? defaultGroupPolicy ?? "allowlist"; - const guildEntries = discordGuildEntries; - const guildsConfigured = Object.keys(guildEntries).length > 0; - const hasAnyUserAllowlist = Object.values(guildEntries).some((guild) => { - if (!guild || typeof guild !== "object") { - return false; - } - const g = guild as Record; - if (Array.isArray(g.users) && g.users.length > 0) { - return true; - } - const channels = g.channels; + const channels = guild.channels; if (!channels || typeof channels !== "object") { - return false; + continue; } - return Object.values(channels as Record).some((channel) => { - if (!channel || typeof channel !== "object") { - return false; + for (const [channelKey, channelValue] of Object.entries( + channels as Record, + )) { + if (!channelValue || typeof channelValue !== "object") { + continue; } - const c = channel as Record; - return Array.isArray(c.users) && c.users.length > 0; - }); - }); - const dmAllowFromRaw = (discordCfg.dm as { allowFrom?: unknown } | undefined)?.allowFrom; - const dmAllowFrom = Array.isArray(dmAllowFromRaw) ? dmAllowFromRaw : []; - const ownerAllowFromConfigured = - normalizeAllowFromList([...dmAllowFrom, ...storeAllowFrom]).length > 0; - - const useAccessGroups = params.cfg.commands?.useAccessGroups !== false; - if ( - !useAccessGroups && - groupPolicy !== "disabled" && - guildsConfigured && - !hasAnyUserAllowlist - ) { + const channel = channelValue as Record; + addDiscordNameBasedEntries({ + target: discordNameBasedAllowEntries, + values: channel.users, + source: `${discordPathPrefix}.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.commands.native.unrestricted", - severity: "critical", - title: "Discord slash commands are unrestricted", - detail: - "commands.useAccessGroups=false disables sender allowlists for Discord slash commands unless a per-guild/channel users allowlist is configured; with no users allowlist, any user in allowed guild channels can invoke /… commands.", - remediation: - "Set commands.useAccessGroups=true (recommended), or configure channels.discord.guilds..users (or channels.discord.guilds..channels..users).", - }); - } else if ( - useAccessGroups && - groupPolicy !== "disabled" && - guildsConfigured && - !ownerAllowFromConfigured && - !hasAnyUserAllowlist - ) { - findings.push({ - checkId: "channels.discord.commands.native.no_allowlists", - severity: "warn", - title: "Discord slash commands have no allowlists", - detail: - "Discord slash commands are enabled, but neither an owner allowFrom list nor any per-guild/channel users allowlist is configured; /… commands will be rejected for everyone.", - remediation: - "Add your user id to channels.discord.allowFrom (or approve yourself via pairing), or configure channels.discord.guilds..users.", + checkId: "channels.discord.allowFrom.name_based_entries", + severity: dangerousNameMatchingEnabled ? "info" : "warn", + title: dangerousNameMatchingEnabled + ? "Discord allowlist uses break-glass name/tag matching" + : "Discord allowlist contains name or tag entries", + detail: dangerousNameMatchingEnabled + ? "Discord name/tag allowlist matching is explicitly enabled via dangerouslyAllowNameMatching. This mutable-identity mode is operator-selected break-glass behavior and out-of-scope for vulnerability reports by itself. " + + `Found: ${examples.join(", ")}${more}.` + : "Discord name/tag allowlist matching uses normalized slugs and can collide across users. " + + `Found: ${examples.join(", ")}${more}.`, + remediation: dangerousNameMatchingEnabled + ? "Prefer stable Discord IDs (or <@id>/user:/pk:), then disable dangerouslyAllowNameMatching." + : "Prefer stable Discord IDs (or <@id>/user:/pk:) in channels.discord.allowFrom and channels.discord.guilds.*.users, or explicitly opt in with dangerouslyAllowNameMatching=true if you accept the risk.", }); } - } - } - - if (plugin.id === "slack") { - const slackCfg = - (account as { config?: Record; dm?: Record } | null) - ?.config ?? ({} as Record); - const nativeEnabled = resolveNativeCommandsEnabled({ - providerId: "slack", - providerSetting: coerceNativeSetting( - (slackCfg.commands as { native?: unknown } | undefined)?.native, - ), - globalSetting: params.cfg.commands?.native, - }); - const nativeSkillsEnabled = resolveNativeSkillsEnabled({ - providerId: "slack", - providerSetting: coerceNativeSetting( - (slackCfg.commands as { nativeSkills?: unknown } | undefined)?.nativeSkills, - ), - globalSetting: params.cfg.commands?.nativeSkills, - }); - const slashCommandEnabled = - nativeEnabled || - nativeSkillsEnabled || - (slackCfg.slashCommand as { enabled?: unknown } | undefined)?.enabled === true; - if (slashCommandEnabled) { - const useAccessGroups = params.cfg.commands?.useAccessGroups !== false; - if (!useAccessGroups) { - findings.push({ - checkId: "channels.slack.commands.slash.useAccessGroups_off", - severity: "critical", - title: "Slack slash commands bypass access groups", - detail: - "Slack slash/native commands are enabled while commands.useAccessGroups=false; this can allow unrestricted /… command execution from channels/users you didn't explicitly authorize.", - remediation: "Set commands.useAccessGroups=true (recommended).", - }); - } else { - const allowFromRaw = ( - account as - | { config?: { allowFrom?: unknown }; dm?: { allowFrom?: unknown } } - | null - | undefined - )?.config?.allowFrom; - const legacyAllowFromRaw = ( - account as { dm?: { allowFrom?: unknown } } | null | undefined - )?.dm?.allowFrom; - const allowFrom = Array.isArray(allowFromRaw) - ? allowFromRaw - : Array.isArray(legacyAllowFromRaw) - ? legacyAllowFromRaw - : []; - const storeAllowFrom = await readChannelAllowFromStore("slack").catch(() => []); - const ownerAllowFromConfigured = - normalizeAllowFromList([...allowFrom, ...storeAllowFrom]).length > 0; - const channels = (slackCfg.channels as Record | undefined) ?? {}; - const hasAnyChannelUsersAllowlist = Object.values(channels).some((value) => { - if (!value || typeof value !== "object") { + const nativeEnabled = resolveNativeCommandsEnabled({ + providerId: "discord", + providerSetting: coerceNativeSetting( + (discordCfg.commands as { native?: unknown } | undefined)?.native, + ), + globalSetting: params.cfg.commands?.native, + }); + const nativeSkillsEnabled = resolveNativeSkillsEnabled({ + providerId: "discord", + providerSetting: coerceNativeSetting( + (discordCfg.commands as { nativeSkills?: unknown } | undefined)?.nativeSkills, + ), + globalSetting: params.cfg.commands?.nativeSkills, + }); + const slashEnabled = nativeEnabled || nativeSkillsEnabled; + if (slashEnabled) { + const defaultGroupPolicy = params.cfg.channels?.defaults?.groupPolicy; + const groupPolicy = + (discordCfg.groupPolicy as string | undefined) ?? defaultGroupPolicy ?? "allowlist"; + const guildEntries = discordGuildEntries; + const guildsConfigured = Object.keys(guildEntries).length > 0; + const hasAnyUserAllowlist = Object.values(guildEntries).some((guild) => { + if (!guild || typeof guild !== "object") { return false; } - const channel = value as Record; - return Array.isArray(channel.users) && channel.users.length > 0; + const g = guild as Record; + if (Array.isArray(g.users) && g.users.length > 0) { + return true; + } + const channels = g.channels; + if (!channels || typeof channels !== "object") { + return false; + } + return Object.values(channels as Record).some((channel) => { + if (!channel || typeof channel !== "object") { + return false; + } + const c = channel as Record; + return Array.isArray(c.users) && c.users.length > 0; + }); }); - if (!ownerAllowFromConfigured && !hasAnyChannelUsersAllowlist) { + const dmAllowFromRaw = (discordCfg.dm as { allowFrom?: unknown } | undefined)?.allowFrom; + const dmAllowFrom = Array.isArray(dmAllowFromRaw) ? dmAllowFromRaw : []; + const ownerAllowFromConfigured = + normalizeAllowFromList([...dmAllowFrom, ...storeAllowFrom]).length > 0; + + const useAccessGroups = params.cfg.commands?.useAccessGroups !== false; + if ( + !useAccessGroups && + groupPolicy !== "disabled" && + guildsConfigured && + !hasAnyUserAllowlist + ) { findings.push({ - checkId: "channels.slack.commands.slash.no_allowlists", - severity: "warn", - title: "Slack slash commands have no allowlists", + checkId: "channels.discord.commands.native.unrestricted", + severity: "critical", + title: "Discord slash commands are unrestricted", detail: - "Slack slash/native commands are enabled, but neither an owner allowFrom list nor any channels..users allowlist is configured; /… commands will be rejected for everyone.", + "commands.useAccessGroups=false disables sender allowlists for Discord slash commands unless a per-guild/channel users allowlist is configured; with no users allowlist, any user in allowed guild channels can invoke /… commands.", remediation: - "Approve yourself via pairing (recommended), or set channels.slack.allowFrom and/or channels.slack.channels..users.", + "Set commands.useAccessGroups=true (recommended), or configure channels.discord.guilds..users (or channels.discord.guilds..channels..users).", + }); + } else if ( + useAccessGroups && + groupPolicy !== "disabled" && + guildsConfigured && + !ownerAllowFromConfigured && + !hasAnyUserAllowlist + ) { + findings.push({ + checkId: "channels.discord.commands.native.no_allowlists", + severity: "warn", + title: "Discord slash commands have no allowlists", + detail: + "Discord slash commands are enabled, but neither an owner allowFrom list nor any per-guild/channel users allowlist is configured; /… commands will be rejected for everyone.", + remediation: + "Add your user id to channels.discord.allowFrom (or approve yourself via pairing), or configure channels.discord.guilds..users.", }); } } } - } - const dmPolicy = plugin.security.resolveDmPolicy?.({ - cfg: params.cfg, - accountId: defaultAccountId, - account, - }); - if (dmPolicy) { - await warnDmPolicy({ - label: plugin.meta.label ?? plugin.id, - provider: plugin.id, - dmPolicy: dmPolicy.policy, - allowFrom: dmPolicy.allowFrom, - policyPath: dmPolicy.policyPath, - allowFromPath: dmPolicy.allowFromPath, - normalizeEntry: dmPolicy.normalizeEntry, - }); - } + if (plugin.id === "slack") { + const slackCfg = + (account as { config?: Record; dm?: Record } | null) + ?.config ?? ({} as Record); + const nativeEnabled = resolveNativeCommandsEnabled({ + providerId: "slack", + providerSetting: coerceNativeSetting( + (slackCfg.commands as { native?: unknown } | undefined)?.native, + ), + globalSetting: params.cfg.commands?.native, + }); + const nativeSkillsEnabled = resolveNativeSkillsEnabled({ + providerId: "slack", + providerSetting: coerceNativeSetting( + (slackCfg.commands as { nativeSkills?: unknown } | undefined)?.nativeSkills, + ), + globalSetting: params.cfg.commands?.nativeSkills, + }); + const slashCommandEnabled = + nativeEnabled || + nativeSkillsEnabled || + (slackCfg.slashCommand as { enabled?: unknown } | undefined)?.enabled === true; + if (slashCommandEnabled) { + const useAccessGroups = params.cfg.commands?.useAccessGroups !== false; + if (!useAccessGroups) { + findings.push({ + checkId: "channels.slack.commands.slash.useAccessGroups_off", + severity: "critical", + title: "Slack slash commands bypass access groups", + detail: + "Slack slash/native commands are enabled while commands.useAccessGroups=false; this can allow unrestricted /… command execution from channels/users you didn't explicitly authorize.", + remediation: "Set commands.useAccessGroups=true (recommended).", + }); + } else { + const allowFromRaw = ( + account as + | { config?: { allowFrom?: unknown }; dm?: { allowFrom?: unknown } } + | null + | undefined + )?.config?.allowFrom; + const legacyAllowFromRaw = ( + account as { dm?: { allowFrom?: unknown } } | null | undefined + )?.dm?.allowFrom; + const allowFrom = Array.isArray(allowFromRaw) + ? allowFromRaw + : Array.isArray(legacyAllowFromRaw) + ? legacyAllowFromRaw + : []; + const storeAllowFrom = await readChannelAllowFromStore("slack").catch(() => []); + const ownerAllowFromConfigured = + normalizeAllowFromList([...allowFrom, ...storeAllowFrom]).length > 0; + const channels = (slackCfg.channels as Record | undefined) ?? {}; + const hasAnyChannelUsersAllowlist = Object.values(channels).some((value) => { + if (!value || typeof value !== "object") { + return false; + } + const channel = value as Record; + return Array.isArray(channel.users) && channel.users.length > 0; + }); + if (!ownerAllowFromConfigured && !hasAnyChannelUsersAllowlist) { + findings.push({ + checkId: "channels.slack.commands.slash.no_allowlists", + severity: "warn", + title: "Slack slash commands have no allowlists", + detail: + "Slack slash/native commands are enabled, but neither an owner allowFrom list nor any channels..users allowlist is configured; /… commands will be rejected for everyone.", + remediation: + "Approve yourself via pairing (recommended), or set channels.slack.allowFrom and/or channels.slack.channels..users.", + }); + } + } + } + } - if (plugin.security.collectWarnings) { - const warnings = await plugin.security.collectWarnings({ + const dmPolicy = plugin.security.resolveDmPolicy?.({ cfg: params.cfg, - accountId: defaultAccountId, + accountId, account, }); - for (const message of warnings ?? []) { - const trimmed = String(message).trim(); - if (!trimmed) { - continue; - } - findings.push({ - checkId: `channels.${plugin.id}.warning.${findings.length + 1}`, - severity: classifyChannelWarningSeverity(trimmed), - title: `${plugin.meta.label ?? plugin.id} security warning`, - detail: trimmed.replace(/^-\s*/, ""), + if (dmPolicy) { + await warnDmPolicy({ + label: plugin.meta.label ?? plugin.id, + provider: plugin.id, + dmPolicy: dmPolicy.policy, + allowFrom: dmPolicy.allowFrom, + policyPath: dmPolicy.policyPath, + allowFromPath: dmPolicy.allowFromPath, + normalizeEntry: dmPolicy.normalizeEntry, }); } - } - if (plugin.id === "telegram") { + if (plugin.security.collectWarnings) { + const warnings = await plugin.security.collectWarnings({ + cfg: params.cfg, + accountId, + account, + }); + for (const message of warnings ?? []) { + const trimmed = String(message).trim(); + if (!trimmed) { + continue; + } + findings.push({ + checkId: `channels.${plugin.id}.warning.${findings.length + 1}`, + severity: classifyChannelWarningSeverity(trimmed), + title: `${plugin.meta.label ?? plugin.id} security warning`, + detail: trimmed.replace(/^-\s*/, ""), + }); + } + } + + if (plugin.id !== "telegram") { + continue; + } + const allowTextCommands = params.cfg.commands?.text !== false; if (!allowTextCommands) { continue; @@ -614,5 +649,5 @@ export async function collectChannelSecurityFindings(params: { } } - return findings; + return dedupeFindings(findings); } diff --git a/src/security/audit.test.ts b/src/security/audit.test.ts index d5ae8a97762..6915855b1e8 100644 --- a/src/security/audit.test.ts +++ b/src/security/audit.test.ts @@ -15,7 +15,8 @@ const isWindows = process.platform === "win32"; function stubChannelPlugin(params: { id: "discord" | "slack" | "telegram"; label: string; - resolveAccount: (cfg: OpenClawConfig) => unknown; + resolveAccount: (cfg: OpenClawConfig, accountId: string | null | undefined) => unknown; + listAccountIds?: (cfg: OpenClawConfig) => string[]; }): ChannelPlugin { return { id: params.id, @@ -31,11 +32,15 @@ function stubChannelPlugin(params: { }, security: {}, config: { - listAccountIds: (cfg) => { - const enabled = Boolean((cfg.channels as Record | undefined)?.[params.id]); - return enabled ? ["default"] : []; - }, - resolveAccount: (cfg) => params.resolveAccount(cfg), + listAccountIds: + params.listAccountIds ?? + ((cfg) => { + const enabled = Boolean( + (cfg.channels as Record | undefined)?.[params.id], + ); + return enabled ? ["default"] : []; + }), + resolveAccount: (cfg, accountId) => params.resolveAccount(cfg, accountId), isEnabled: () => true, isConfigured: () => true, }, @@ -45,19 +50,46 @@ function stubChannelPlugin(params: { const discordPlugin = stubChannelPlugin({ id: "discord", label: "Discord", - resolveAccount: (cfg) => ({ config: cfg.channels?.discord ?? {} }), + listAccountIds: (cfg) => { + const ids = Object.keys(cfg.channels?.discord?.accounts ?? {}); + return ids.length > 0 ? ids : ["default"]; + }, + resolveAccount: (cfg, accountId) => { + const resolvedAccountId = typeof accountId === "string" && accountId ? accountId : "default"; + const base = cfg.channels?.discord ?? {}; + const account = cfg.channels?.discord?.accounts?.[resolvedAccountId] ?? {}; + return { config: { ...base, ...account } }; + }, }); const slackPlugin = stubChannelPlugin({ id: "slack", label: "Slack", - resolveAccount: (cfg) => ({ config: cfg.channels?.slack ?? {} }), + listAccountIds: (cfg) => { + const ids = Object.keys(cfg.channels?.slack?.accounts ?? {}); + return ids.length > 0 ? ids : ["default"]; + }, + resolveAccount: (cfg, accountId) => { + const resolvedAccountId = typeof accountId === "string" && accountId ? accountId : "default"; + const base = cfg.channels?.slack ?? {}; + const account = cfg.channels?.slack?.accounts?.[resolvedAccountId] ?? {}; + return { config: { ...base, ...account } }; + }, }); const telegramPlugin = stubChannelPlugin({ id: "telegram", label: "Telegram", - resolveAccount: (cfg) => ({ config: cfg.channels?.telegram ?? {} }), + listAccountIds: (cfg) => { + const ids = Object.keys(cfg.channels?.telegram?.accounts ?? {}); + return ids.length > 0 ? ids : ["default"]; + }, + resolveAccount: (cfg, accountId) => { + const resolvedAccountId = typeof accountId === "string" && accountId ? accountId : "default"; + const base = cfg.channels?.telegram ?? {}; + const account = cfg.channels?.telegram?.accounts?.[resolvedAccountId] ?? {}; + return { config: { ...base, ...account } }; + }, }); function successfulProbeResult(url: string) { @@ -1537,6 +1569,79 @@ describe("security audit", () => { }); }); + it("audits non-default Discord accounts for dangerous name matching", async () => { + await withChannelSecurityStateDir(async () => { + const cfg: OpenClawConfig = { + channels: { + discord: { + enabled: true, + token: "t", + accounts: { + alpha: { token: "a" }, + beta: { + token: "b", + dangerouslyAllowNameMatching: true, + }, + }, + }, + }, + }; + + const res = await runSecurityAudit({ + config: cfg, + includeFilesystem: false, + includeChannelSecurity: true, + plugins: [discordPlugin], + }); + + expect(res.findings).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + checkId: "channels.discord.allowFrom.dangerous_name_matching_enabled", + title: expect.stringContaining("(account: beta)"), + severity: "info", + }), + ]), + ); + }); + }); + + it("audits name-based allowlists on non-default Discord accounts", async () => { + await withChannelSecurityStateDir(async () => { + const cfg: OpenClawConfig = { + channels: { + discord: { + enabled: true, + token: "t", + accounts: { + alpha: { + token: "a", + allowFrom: ["123456789012345678"], + }, + beta: { + token: "b", + allowFrom: ["Alice#1234"], + }, + }, + }, + }, + }; + + 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?.detail).toContain("channels.discord.accounts.beta.allowFrom:Alice#1234"); + }); + }); + it("does not warn when Discord allowlists use ID-style entries only", async () => { await withChannelSecurityStateDir(async () => { const cfg: OpenClawConfig = { diff --git a/src/security/mutable-allowlist-detectors.ts b/src/security/mutable-allowlist-detectors.ts new file mode 100644 index 00000000000..af3a8f81eaa --- /dev/null +++ b/src/security/mutable-allowlist-detectors.ts @@ -0,0 +1,101 @@ +export function isDiscordMutableAllowEntry(raw: string): boolean { + const text = raw.trim(); + if (!text || text === "*") { + return false; + } + + const maybeMentionId = text.replace(/^<@!?/, "").replace(/>$/, ""); + if (/^\d+$/.test(maybeMentionId)) { + return false; + } + + for (const prefix of ["discord:", "user:", "pk:"]) { + if (!text.startsWith(prefix)) { + continue; + } + return text.slice(prefix.length).trim().length === 0; + } + + return true; +} + +export function isSlackMutableAllowEntry(raw: string): boolean { + const text = raw.trim(); + if (!text || text === "*") { + return false; + } + + const mentionMatch = text.match(/^<@([A-Z0-9]+)>$/i); + if (mentionMatch && /^[A-Z0-9]{8,}$/i.test(mentionMatch[1] ?? "")) { + return false; + } + + const withoutPrefix = text.replace(/^(slack|user):/i, "").trim(); + if (/^[UWBCGDT][A-Z0-9]{2,}$/.test(withoutPrefix)) { + return false; + } + if (/^[A-Z0-9]{8,}$/i.test(withoutPrefix)) { + return false; + } + + return true; +} + +export function isGoogleChatMutableAllowEntry(raw: string): boolean { + const text = raw.trim(); + if (!text || text === "*") { + return false; + } + + const withoutPrefix = text.replace(/^(googlechat|google-chat|gchat):/i, "").trim(); + if (!withoutPrefix) { + return false; + } + + const withoutUsers = withoutPrefix.replace(/^users\//i, ""); + return withoutUsers.includes("@"); +} + +export function isMSTeamsMutableAllowEntry(raw: string): boolean { + const text = raw.trim(); + if (!text || text === "*") { + return false; + } + + const withoutPrefix = text.replace(/^(msteams|user):/i, "").trim(); + return /\s/.test(withoutPrefix) || withoutPrefix.includes("@"); +} + +export function isMattermostMutableAllowEntry(raw: string): boolean { + const text = raw.trim(); + if (!text || text === "*") { + return false; + } + + const normalized = text + .replace(/^(mattermost|user):/i, "") + .replace(/^@/, "") + .trim() + .toLowerCase(); + + // Mattermost user IDs are stable 26-char lowercase/number tokens. + if (/^[a-z0-9]{26}$/.test(normalized)) { + return false; + } + + return true; +} + +export function isIrcMutableAllowEntry(raw: string): boolean { + const text = raw.trim().toLowerCase(); + if (!text || text === "*") { + return false; + } + + const normalized = text + .replace(/^irc:/, "") + .replace(/^user:/, "") + .trim(); + + return !normalized.includes("!") && !normalized.includes("@"); +} diff --git a/src/slack/monitor/provider.ts b/src/slack/monitor/provider.ts index dcec6074dea..827784723a4 100644 --- a/src/slack/monitor/provider.ts +++ b/src/slack/monitor/provider.ts @@ -10,6 +10,7 @@ import { summarizeMapping, } from "../../channels/allowlists/resolve-utils.js"; import { loadConfig } from "../../config/config.js"; +import { isDangerousNameMatchingEnabled } from "../../config/dangerous-name-matching.js"; import { resolveOpenProviderRuntimeGroupPolicy, resolveDefaultGroupPolicy, @@ -210,7 +211,7 @@ export async function monitorSlackProvider(opts: MonitorSlackOpts = {}) { dmEnabled, dmPolicy, allowFrom, - allowNameMatching: slackCfg.dangerouslyAllowNameMatching === true, + allowNameMatching: isDangerousNameMatchingEnabled(slackCfg), groupDmEnabled, groupDmChannels, defaultRequireMention: slackCfg.requireMention,