From 90e72a67a328caeec4c1aacaa168f64617d61bd3 Mon Sep 17 00:00:00 2001 From: Alex Knight Date: Tue, 16 Jun 2026 07:30:02 +1000 Subject: [PATCH] fix(config): resolve DM allowFrom via canonical-or-legacy before warning The generic dmPolicy/allowFrom warning read only the canonical top-level allowFrom, so channels that keep their wildcard under the legacy dm.allowFrom alias (e.g. Discord/Slack, mode=topOnly/topOrNested) got a false 'all DMs dropped' warning even though runtime honors dm.allowFrom. Resolve policy and allowFrom through the shared resolveChannelDm* helpers with the channel's dmAllowFromMode (matching runtime and doctor), and skip nestedOnly channels whose canonical fields live under dm.* and do not match this warning's top-level paths. Adds a Discord legacy-alias regression test. Addresses ClawSweeper review finding P1 (false positives on legacy dm.allowFrom). --- .../validation.channel-metadata.test.ts | 21 +++++++++ src/config/validation.ts | 46 +++++++++++-------- 2 files changed, 47 insertions(+), 20 deletions(-) diff --git a/src/config/validation.channel-metadata.test.ts b/src/config/validation.channel-metadata.test.ts index 939188b8171..3da64392e68 100644 --- a/src/config/validation.channel-metadata.test.ts +++ b/src/config/validation.channel-metadata.test.ts @@ -393,6 +393,27 @@ describe("validateConfigObjectWithPlugins channel metadata (applyDefaults: true) false, ); }); + + it('does not warn when dmPolicy="open" is satisfied by the legacy dm.allowFrom alias', () => { + // Runtime resolves allowFrom as canonical `allowFrom` ?? legacy `dm.allowFrom`, so a + // top-level-canonical channel (Discord) that keeps its wildcard under `dm.allowFrom` + // is valid and must not produce a false "all DMs dropped" warning. + const result = validateConfigObjectWithPlugins({ + channels: { + discord: { + enabled: true, + token: "test-token", + dmPolicy: "open", + dm: { allowFrom: ["*"] }, + }, + }, + }); + + expect(result.ok).toBe(true); + expect(result.warnings.some((warning) => warning.path === "channels.discord.allowFrom")).toBe( + false, + ); + }); }); describe("validateConfigObjectRawWithPlugins channel metadata", () => { diff --git a/src/config/validation.ts b/src/config/validation.ts index a2ef60fe59b..15863e77c34 100644 --- a/src/config/validation.ts +++ b/src/config/validation.ts @@ -5,6 +5,11 @@ import { isCanonicalDottedDecimalIPv4, isLoopbackIpAddress } from "@openclaw/net import { normalizeLowercaseStringOrEmpty } from "@openclaw/normalization-core/string-coerce"; import { sanitizeForLog } from "../../packages/terminal-core/src/ansi.js"; import { resolveAgentWorkspaceDir, resolveDefaultAgentId } from "../agents/agent-scope.js"; +import { + resolveChannelDmAllowFrom, + resolveChannelDmPolicy, +} from "../channels/plugins/dm-access.js"; +import { getDoctorChannelCapabilities } from "../commands/doctor/channel-capabilities.js"; import { isPathInside } from "../infra/path-guards.js"; import { planManifestModelCatalogSuppressions } from "../model-catalog/index.js"; import { @@ -316,15 +321,6 @@ function formatRawChannelConfigIssueMessage(message: string): string { return `invalid config: ${message}`; } -function asAllowFromList(value: unknown): Array | undefined { - if (!Array.isArray(value)) { - return undefined; - } - return value.filter( - (entry): entry is string | number => typeof entry === "string" || typeof entry === "number", - ); -} - function buildDmPolicyDependencyWarning(params: { channelId: string; accountId?: string; @@ -344,11 +340,20 @@ function buildDmPolicyDependencyWarning(params: { return { path: allowFromPath, message }; } +// Channel map keys that are not channels and must be skipped while scanning DM policy. +const DM_POLICY_PSEUDO_CHANNEL_KEYS = new Set(["defaults", "modelByChannel", "tools"]); + /** * Surface dmPolicy/allowFrom dependency problems generically for every channel that - * uses the shared top-level dmPolicy/allowFrom contract. These configs parse fine but - * drop every DM at runtime, so we warn (rather than reject) to stay consistent with - * `security audit`/`doctor` and avoid breaking existing-but-usable configs on upgrade. + * exposes DM policy via the canonical top-level `dmPolicy`/`allowFrom` fields. These + * configs parse fine but drop every DM at runtime, so we warn (rather than reject) to + * stay consistent with `security audit`/`doctor` and avoid breaking existing-but-usable + * configs on upgrade. + * + * Resolution goes through the shared DM-access helpers so the warning matches the + * effective policy/allowFrom the runtime sees, including the legacy `dm.*` aliases and + * account->channel inheritance. `nestedOnly` channels (canonical fields under `dm.*`) + * are skipped because their config shape does not match this warning's top-level paths. */ function collectChannelDmPolicyDependencyWarnings(config: OpenClawConfig): ConfigValidationIssue[] { if (!config.channels || !isRecord(config.channels)) { @@ -356,15 +361,16 @@ function collectChannelDmPolicyDependencyWarnings(config: OpenClawConfig): Confi } const warnings: ConfigValidationIssue[] = []; for (const [channelId, channelValue] of Object.entries(config.channels)) { - if (channelId === "defaults" || channelId === "modelByChannel" || !isRecord(channelValue)) { + if (DM_POLICY_PSEUDO_CHANNEL_KEYS.has(channelId) || !isRecord(channelValue)) { + continue; + } + const mode = getDoctorChannelCapabilities(channelId).dmAllowFromMode; + if (mode === "nestedOnly") { continue; } - const channelPolicy = - typeof channelValue.dmPolicy === "string" ? channelValue.dmPolicy : undefined; - const channelAllowFrom = asAllowFromList(channelValue.allowFrom); const channelViolation = evaluateDmPolicyAllowFromDependency({ - policy: channelPolicy, - allowFrom: channelAllowFrom, + policy: resolveChannelDmPolicy({ account: channelValue, mode }), + allowFrom: resolveChannelDmAllowFrom({ account: channelValue, mode }), }); if (channelViolation) { warnings.push(buildDmPolicyDependencyWarning({ channelId, violation: channelViolation })); @@ -377,8 +383,8 @@ function collectChannelDmPolicyDependencyWarnings(config: OpenClawConfig): Confi continue; } const accountViolation = evaluateDmPolicyAllowFromDependency({ - policy: typeof accountValue.dmPolicy === "string" ? accountValue.dmPolicy : channelPolicy, - allowFrom: asAllowFromList(accountValue.allowFrom) ?? channelAllowFrom, + policy: resolveChannelDmPolicy({ account: accountValue, parent: channelValue, mode }), + allowFrom: resolveChannelDmAllowFrom({ account: accountValue, parent: channelValue, mode }), }); if (accountViolation) { warnings.push(