From dab4a4790d320acf211ade5ce9994e970b02c5b5 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 6 Apr 2026 18:47:35 +0100 Subject: [PATCH] refactor: dedupe mutable allowlist doctor warnings --- extensions/slack/src/doctor.test.ts | 32 +++++++++ extensions/slack/src/doctor.ts | 90 ++++++++++---------------- extensions/zalouser/src/doctor.test.ts | 23 +++++++ extensions/zalouser/src/doctor.ts | 58 ++++++----------- 4 files changed, 107 insertions(+), 96 deletions(-) diff --git a/extensions/slack/src/doctor.test.ts b/extensions/slack/src/doctor.test.ts index 1d3445f93f0..02f1869c6a6 100644 --- a/extensions/slack/src/doctor.test.ts +++ b/extensions/slack/src/doctor.test.ts @@ -2,6 +2,38 @@ import { describe, expect, it } from "vitest"; import { slackDoctor } from "./doctor.js"; describe("slack doctor", () => { + it("warns when mutable allowlist entries rely on disabled name matching", () => { + expect( + slackDoctor.collectMutableAllowlistWarnings?.({ + cfg: { + channels: { + slack: { + allowFrom: ["alice"], + accounts: { + work: { + dm: { + allowFrom: ["U12345678"], + }, + channels: { + general: { + users: ["bob"], + }, + }, + }, + }, + }, + }, + } as never, + }), + ).toEqual( + expect.arrayContaining([ + expect.stringContaining("mutable allowlist entries across slack"), + expect.stringContaining("channels.slack.allowFrom: alice"), + expect.stringContaining("channels.slack.accounts.work.channels.general.users: bob"), + ]), + ); + }); + it("normalizes legacy slack streaming aliases into the nested streaming shape", () => { const normalize = slackDoctor.normalizeCompatibilityConfig; expect(normalize).toBeDefined(); diff --git a/extensions/slack/src/doctor.ts b/extensions/slack/src/doctor.ts index ab0d0ce0a95..ffafc9c6356 100644 --- a/extensions/slack/src/doctor.ts +++ b/extensions/slack/src/doctor.ts @@ -1,6 +1,5 @@ import { type ChannelDoctorAdapter } from "openclaw/plugin-sdk/channel-contract"; -import { type OpenClawConfig } from "openclaw/plugin-sdk/config-runtime"; -import { collectProviderDangerousNameMatchingScopes } from "openclaw/plugin-sdk/runtime-doctor"; +import { createDangerousNameMatchingMutableAllowlistWarningCollector } from "openclaw/plugin-sdk/channel-policy"; import { legacyConfigRules as SLACK_LEGACY_CONFIG_RULES, normalizeCompatibilityConfig as normalizeSlackCompatibilityConfig, @@ -13,61 +12,40 @@ function asObjectRecord(value: unknown): Record | null { : null; } -function sanitizeForLog(value: string): string { - return value.replace(/\p{Cc}+/gu, " ").trim(); -} -export function collectSlackMutableAllowlistWarnings(cfg: OpenClawConfig): string[] { - const hits: Array<{ path: string; entry: string }> = []; - const addHits = (pathLabel: string, list: unknown) => { - if (!Array.isArray(list)) { - return; - } - for (const entry of list) { - const text = String(entry).trim(); - if (!text || text === "*" || !isSlackMutableAllowEntry(text)) { - continue; +export const collectSlackMutableAllowlistWarnings = + createDangerousNameMatchingMutableAllowlistWarningCollector({ + channel: "slack", + detector: isSlackMutableAllowEntry, + collectLists: (scope) => { + const lists = [ + { + pathLabel: `${scope.prefix}.allowFrom`, + list: scope.account.allowFrom, + }, + ]; + const dm = asObjectRecord(scope.account.dm); + if (dm) { + lists.push({ + pathLabel: `${scope.prefix}.dm.allowFrom`, + list: dm.allowFrom, + }); } - hits.push({ path: pathLabel, entry: text }); - } - }; - - for (const scope of collectProviderDangerousNameMatchingScopes(cfg, "slack")) { - if (scope.dangerousNameMatchingEnabled) { - continue; - } - addHits(`${scope.prefix}.allowFrom`, scope.account.allowFrom); - const dm = asObjectRecord(scope.account.dm); - if (dm) { - addHits(`${scope.prefix}.dm.allowFrom`, dm.allowFrom); - } - const channels = asObjectRecord(scope.account.channels); - if (!channels) { - continue; - } - for (const [channelKey, channelRaw] of Object.entries(channels)) { - const channel = asObjectRecord(channelRaw); - if (channel) { - addHits(`${scope.prefix}.channels.${channelKey}.users`, channel.users); + const channels = asObjectRecord(scope.account.channels); + if (channels) { + for (const [channelKey, channelRaw] of Object.entries(channels)) { + const channel = asObjectRecord(channelRaw); + if (!channel) { + continue; + } + lists.push({ + pathLabel: `${scope.prefix}.channels.${channelKey}.users`, + list: channel.users, + }); + } } - } - } - - if (hits.length === 0) { - return []; - } - const exampleLines = hits - .slice(0, 8) - .map((hit) => `- ${sanitizeForLog(hit.path)}: ${sanitizeForLog(hit.entry)}`); - const remaining = - hits.length > 8 ? `- +${hits.length - 8} more mutable allowlist entries.` : null; - return [ - `- Found ${hits.length} mutable allowlist ${hits.length === 1 ? "entry" : "entries"} across slack while name matching is disabled by default.`, - ...exampleLines, - ...(remaining ? [remaining] : []), - "- Option A (break-glass): enable channels.slack.dangerousNameMatching=true for the affected scope.", - "- Option B (recommended): resolve names to stable Slack IDs and rewrite the allowlist entries.", - ]; -} + return lists; + }, + }); export const slackDoctor: ChannelDoctorAdapter = { dmAllowFromMode: "topOrNested", @@ -76,5 +54,5 @@ export const slackDoctor: ChannelDoctorAdapter = { warnOnEmptyGroupSenderAllowlist: false, legacyConfigRules: SLACK_LEGACY_CONFIG_RULES, normalizeCompatibilityConfig: normalizeSlackCompatibilityConfig, - collectMutableAllowlistWarnings: ({ cfg }) => collectSlackMutableAllowlistWarnings(cfg), + collectMutableAllowlistWarnings: collectSlackMutableAllowlistWarnings, }; diff --git a/extensions/zalouser/src/doctor.test.ts b/extensions/zalouser/src/doctor.test.ts index 04fa42376e9..6e5f98ce9f9 100644 --- a/extensions/zalouser/src/doctor.test.ts +++ b/extensions/zalouser/src/doctor.test.ts @@ -2,6 +2,29 @@ import { describe, expect, it } from "vitest"; import { zalouserDoctor } from "./doctor.js"; describe("zalouser doctor", () => { + it("warns when mutable group names rely on disabled name matching", () => { + expect( + zalouserDoctor.collectMutableAllowlistWarnings?.({ + cfg: { + channels: { + zalouser: { + groups: { + "group:trusted": { + enabled: true, + }, + }, + }, + }, + } as never, + }), + ).toEqual( + expect.arrayContaining([ + expect.stringContaining("mutable allowlist entry across zalouser"), + expect.stringContaining("channels.zalouser.groups: group:trusted"), + ]), + ); + }); + it("normalizes legacy group allow aliases to enabled", () => { const normalize = zalouserDoctor.normalizeCompatibilityConfig; expect(normalize).toBeDefined(); diff --git a/extensions/zalouser/src/doctor.ts b/extensions/zalouser/src/doctor.ts index 7f032c33361..cfa3fe642da 100644 --- a/extensions/zalouser/src/doctor.ts +++ b/extensions/zalouser/src/doctor.ts @@ -3,8 +3,8 @@ import type { ChannelDoctorConfigMutation, ChannelDoctorLegacyConfigRule, } from "openclaw/plugin-sdk/channel-contract"; +import { createDangerousNameMatchingMutableAllowlistWarningCollector } from "openclaw/plugin-sdk/channel-policy"; import type { OpenClawConfig } from "openclaw/plugin-sdk/config-runtime"; -import { collectProviderDangerousNameMatchingScopes } from "openclaw/plugin-sdk/runtime-doctor"; import { isZalouserMutableGroupEntry } from "./security-audit.js"; type ZalouserChannelsConfig = NonNullable; @@ -15,10 +15,6 @@ function asObjectRecord(value: unknown): Record | null { : null; } -function sanitizeForLog(value: string): string { - return value.replace(/\p{Cc}+/gu, " ").trim(); -} - function hasLegacyZalouserGroupAllowAlias(value: unknown): boolean { const group = asObjectRecord(value); return Boolean(group && typeof group.allow === "boolean"); @@ -156,40 +152,22 @@ const ZALOUSER_LEGACY_CONFIG_RULES: ChannelDoctorLegacyConfigRule[] = [ }, ]; -export function collectZalouserMutableAllowlistWarnings(cfg: OpenClawConfig): string[] { - const hits: Array<{ path: string; entry: string }> = []; - - for (const scope of collectProviderDangerousNameMatchingScopes(cfg, "zalouser")) { - if (scope.dangerousNameMatchingEnabled) { - continue; - } - const groups = asObjectRecord(scope.account.groups); - if (!groups) { - continue; - } - for (const entry of Object.keys(groups)) { - if (isZalouserMutableGroupEntry(entry)) { - hits.push({ path: `${scope.prefix}.groups`, entry }); - } - } - } - - if (hits.length === 0) { - return []; - } - const exampleLines = hits - .slice(0, 8) - .map((hit) => `- ${sanitizeForLog(hit.path)}: ${sanitizeForLog(hit.entry)}`); - const remaining = - hits.length > 8 ? `- +${hits.length - 8} more mutable allowlist entries.` : null; - return [ - `- Found ${hits.length} mutable allowlist ${hits.length === 1 ? "entry" : "entries"} across zalouser while name matching is disabled by default.`, - ...exampleLines, - ...(remaining ? [remaining] : []), - "- Option A (break-glass): enable channels.zalouser.dangerousNameMatching=true for the affected scope.", - "- Option B (recommended): resolve mutable group names to stable IDs and rewrite the allowlist entries.", - ]; -} +export const collectZalouserMutableAllowlistWarnings = + createDangerousNameMatchingMutableAllowlistWarningCollector({ + channel: "zalouser", + detector: isZalouserMutableGroupEntry, + collectLists: (scope) => { + const groups = asObjectRecord(scope.account.groups); + return groups + ? [ + { + pathLabel: `${scope.prefix}.groups`, + list: Object.keys(groups), + }, + ] + : []; + }, + }); export const zalouserDoctor: ChannelDoctorAdapter = { dmAllowFromMode: "topOnly", @@ -198,5 +176,5 @@ export const zalouserDoctor: ChannelDoctorAdapter = { warnOnEmptyGroupSenderAllowlist: false, legacyConfigRules: ZALOUSER_LEGACY_CONFIG_RULES, normalizeCompatibilityConfig: ({ cfg }) => normalizeZalouserCompatibilityConfig(cfg), - collectMutableAllowlistWarnings: ({ cfg }) => collectZalouserMutableAllowlistWarnings(cfg), + collectMutableAllowlistWarnings: collectZalouserMutableAllowlistWarnings, };