refactor: dedupe mutable allowlist doctor warnings

This commit is contained in:
Peter Steinberger
2026-04-06 18:47:35 +01:00
parent 2d0618f8b5
commit dab4a4790d
4 changed files with 107 additions and 96 deletions

View File

@@ -2,6 +2,38 @@ import { describe, expect, it } from "vitest";
import { slackDoctor } from "./doctor.js"; import { slackDoctor } from "./doctor.js";
describe("slack doctor", () => { 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", () => { it("normalizes legacy slack streaming aliases into the nested streaming shape", () => {
const normalize = slackDoctor.normalizeCompatibilityConfig; const normalize = slackDoctor.normalizeCompatibilityConfig;
expect(normalize).toBeDefined(); expect(normalize).toBeDefined();

View File

@@ -1,6 +1,5 @@
import { type ChannelDoctorAdapter } from "openclaw/plugin-sdk/channel-contract"; import { type ChannelDoctorAdapter } from "openclaw/plugin-sdk/channel-contract";
import { type OpenClawConfig } from "openclaw/plugin-sdk/config-runtime"; import { createDangerousNameMatchingMutableAllowlistWarningCollector } from "openclaw/plugin-sdk/channel-policy";
import { collectProviderDangerousNameMatchingScopes } from "openclaw/plugin-sdk/runtime-doctor";
import { import {
legacyConfigRules as SLACK_LEGACY_CONFIG_RULES, legacyConfigRules as SLACK_LEGACY_CONFIG_RULES,
normalizeCompatibilityConfig as normalizeSlackCompatibilityConfig, normalizeCompatibilityConfig as normalizeSlackCompatibilityConfig,
@@ -13,61 +12,40 @@ function asObjectRecord(value: unknown): Record<string, unknown> | null {
: null; : null;
} }
function sanitizeForLog(value: string): string { export const collectSlackMutableAllowlistWarnings =
return value.replace(/\p{Cc}+/gu, " ").trim(); createDangerousNameMatchingMutableAllowlistWarningCollector({
} channel: "slack",
export function collectSlackMutableAllowlistWarnings(cfg: OpenClawConfig): string[] { detector: isSlackMutableAllowEntry,
const hits: Array<{ path: string; entry: string }> = []; collectLists: (scope) => {
const addHits = (pathLabel: string, list: unknown) => { const lists = [
if (!Array.isArray(list)) { {
return; pathLabel: `${scope.prefix}.allowFrom`,
} list: scope.account.allowFrom,
for (const entry of list) { },
const text = String(entry).trim(); ];
if (!text || text === "*" || !isSlackMutableAllowEntry(text)) { const dm = asObjectRecord(scope.account.dm);
continue; if (dm) {
lists.push({
pathLabel: `${scope.prefix}.dm.allowFrom`,
list: dm.allowFrom,
});
} }
hits.push({ path: pathLabel, entry: text }); const channels = asObjectRecord(scope.account.channels);
} if (channels) {
}; for (const [channelKey, channelRaw] of Object.entries(channels)) {
const channel = asObjectRecord(channelRaw);
for (const scope of collectProviderDangerousNameMatchingScopes(cfg, "slack")) { if (!channel) {
if (scope.dangerousNameMatchingEnabled) { continue;
continue; }
} lists.push({
addHits(`${scope.prefix}.allowFrom`, scope.account.allowFrom); pathLabel: `${scope.prefix}.channels.${channelKey}.users`,
const dm = asObjectRecord(scope.account.dm); list: channel.users,
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);
} }
} return lists;
} },
});
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.",
];
}
export const slackDoctor: ChannelDoctorAdapter = { export const slackDoctor: ChannelDoctorAdapter = {
dmAllowFromMode: "topOrNested", dmAllowFromMode: "topOrNested",
@@ -76,5 +54,5 @@ export const slackDoctor: ChannelDoctorAdapter = {
warnOnEmptyGroupSenderAllowlist: false, warnOnEmptyGroupSenderAllowlist: false,
legacyConfigRules: SLACK_LEGACY_CONFIG_RULES, legacyConfigRules: SLACK_LEGACY_CONFIG_RULES,
normalizeCompatibilityConfig: normalizeSlackCompatibilityConfig, normalizeCompatibilityConfig: normalizeSlackCompatibilityConfig,
collectMutableAllowlistWarnings: ({ cfg }) => collectSlackMutableAllowlistWarnings(cfg), collectMutableAllowlistWarnings: collectSlackMutableAllowlistWarnings,
}; };

View File

@@ -2,6 +2,29 @@ import { describe, expect, it } from "vitest";
import { zalouserDoctor } from "./doctor.js"; import { zalouserDoctor } from "./doctor.js";
describe("zalouser doctor", () => { 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", () => { it("normalizes legacy group allow aliases to enabled", () => {
const normalize = zalouserDoctor.normalizeCompatibilityConfig; const normalize = zalouserDoctor.normalizeCompatibilityConfig;
expect(normalize).toBeDefined(); expect(normalize).toBeDefined();

View File

@@ -3,8 +3,8 @@ import type {
ChannelDoctorConfigMutation, ChannelDoctorConfigMutation,
ChannelDoctorLegacyConfigRule, ChannelDoctorLegacyConfigRule,
} from "openclaw/plugin-sdk/channel-contract"; } from "openclaw/plugin-sdk/channel-contract";
import { createDangerousNameMatchingMutableAllowlistWarningCollector } from "openclaw/plugin-sdk/channel-policy";
import type { OpenClawConfig } from "openclaw/plugin-sdk/config-runtime"; import type { OpenClawConfig } from "openclaw/plugin-sdk/config-runtime";
import { collectProviderDangerousNameMatchingScopes } from "openclaw/plugin-sdk/runtime-doctor";
import { isZalouserMutableGroupEntry } from "./security-audit.js"; import { isZalouserMutableGroupEntry } from "./security-audit.js";
type ZalouserChannelsConfig = NonNullable<OpenClawConfig["channels"]>; type ZalouserChannelsConfig = NonNullable<OpenClawConfig["channels"]>;
@@ -15,10 +15,6 @@ function asObjectRecord(value: unknown): Record<string, unknown> | null {
: null; : null;
} }
function sanitizeForLog(value: string): string {
return value.replace(/\p{Cc}+/gu, " ").trim();
}
function hasLegacyZalouserGroupAllowAlias(value: unknown): boolean { function hasLegacyZalouserGroupAllowAlias(value: unknown): boolean {
const group = asObjectRecord(value); const group = asObjectRecord(value);
return Boolean(group && typeof group.allow === "boolean"); return Boolean(group && typeof group.allow === "boolean");
@@ -156,40 +152,22 @@ const ZALOUSER_LEGACY_CONFIG_RULES: ChannelDoctorLegacyConfigRule[] = [
}, },
]; ];
export function collectZalouserMutableAllowlistWarnings(cfg: OpenClawConfig): string[] { export const collectZalouserMutableAllowlistWarnings =
const hits: Array<{ path: string; entry: string }> = []; createDangerousNameMatchingMutableAllowlistWarningCollector({
channel: "zalouser",
for (const scope of collectProviderDangerousNameMatchingScopes(cfg, "zalouser")) { detector: isZalouserMutableGroupEntry,
if (scope.dangerousNameMatchingEnabled) { collectLists: (scope) => {
continue; const groups = asObjectRecord(scope.account.groups);
} return groups
const groups = asObjectRecord(scope.account.groups); ? [
if (!groups) { {
continue; pathLabel: `${scope.prefix}.groups`,
} list: Object.keys(groups),
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 zalouserDoctor: ChannelDoctorAdapter = { export const zalouserDoctor: ChannelDoctorAdapter = {
dmAllowFromMode: "topOnly", dmAllowFromMode: "topOnly",
@@ -198,5 +176,5 @@ export const zalouserDoctor: ChannelDoctorAdapter = {
warnOnEmptyGroupSenderAllowlist: false, warnOnEmptyGroupSenderAllowlist: false,
legacyConfigRules: ZALOUSER_LEGACY_CONFIG_RULES, legacyConfigRules: ZALOUSER_LEGACY_CONFIG_RULES,
normalizeCompatibilityConfig: ({ cfg }) => normalizeZalouserCompatibilityConfig(cfg), normalizeCompatibilityConfig: ({ cfg }) => normalizeZalouserCompatibilityConfig(cfg),
collectMutableAllowlistWarnings: ({ cfg }) => collectZalouserMutableAllowlistWarnings(cfg), collectMutableAllowlistWarnings: collectZalouserMutableAllowlistWarnings,
}; };