From 91f404dc7e599555e930af36ec000052976bb5c6 Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Sat, 21 Mar 2026 16:32:11 -0700 Subject: [PATCH] refactor(doctor): continue doctor flow extraction (#51920) * refactor(doctor): extract shared warning formatters * refactor(doctor): extract provider warning previews * style(doctor): sort telegram imports --- src/commands/doctor-config-flow.ts | 133 +++++------------- src/commands/doctor/providers/discord.test.ts | 18 ++- src/commands/doctor/providers/discord.ts | 16 +++ .../doctor/providers/telegram.test.ts | 13 ++ src/commands/doctor/providers/telegram.ts | 14 ++ .../doctor/shared/exec-safe-bins.test.ts | 25 ++++ src/commands/doctor/shared/exec-safe-bins.ts | 60 ++++++++ .../shared/legacy-tools-by-sender.test.ts | 21 +++ .../doctor/shared/legacy-tools-by-sender.ts | 19 +++ .../doctor/shared/mutable-allowlist.test.ts | 32 ++++- .../doctor/shared/mutable-allowlist.ts | 25 ++++ .../shared/open-policy-allowfrom.test.ts | 17 ++- .../doctor/shared/open-policy-allowfrom.ts | 14 ++ 13 files changed, 310 insertions(+), 97 deletions(-) diff --git a/src/commands/doctor-config-flow.ts b/src/commands/doctor-config-flow.ts index fb1c343d6a8..46cc7c9b8d1 100644 --- a/src/commands/doctor-config-flow.ts +++ b/src/commands/doctor-config-flow.ts @@ -12,6 +12,7 @@ import { runDoctorConfigPreflight } from "./doctor-config-preflight.js"; import { normalizeCompatibilityConfigValues } from "./doctor-legacy-config.js"; import type { DoctorOptions } from "./doctor-prompter.js"; import { + collectDiscordNumericIdWarnings, maybeRepairDiscordNumericIds, scanDiscordNumericIdEntries, } from "./doctor/providers/discord.js"; @@ -22,6 +23,7 @@ import { formatMatrixLegacyStatePreview, } from "./doctor/providers/matrix.js"; import { + collectTelegramAllowFromUsernameWarnings, collectTelegramEmptyAllowlistExtraWarnings, maybeRepairTelegramAllowFromUsernames, scanTelegramAllowFromUsernameEntries, @@ -33,16 +35,25 @@ import { } from "./doctor/shared/default-account-warnings.js"; import { scanEmptyAllowlistPolicyWarnings } from "./doctor/shared/empty-allowlist-scan.js"; import { + collectExecSafeBinCoverageWarnings, + collectExecSafeBinTrustedDirHintWarnings, maybeRepairExecSafeBinProfiles, scanExecSafeBinCoverage, scanExecSafeBinTrustedDirHints, } from "./doctor/shared/exec-safe-bins.js"; import { + collectLegacyToolsBySenderWarnings, maybeRepairLegacyToolsBySenderKeys, scanLegacyToolsBySenderKeys, } from "./doctor/shared/legacy-tools-by-sender.js"; -import { scanMutableAllowlistEntries } from "./doctor/shared/mutable-allowlist.js"; -import { maybeRepairOpenPolicyAllowFrom } from "./doctor/shared/open-policy-allowfrom.js"; +import { + collectMutableAllowlistWarnings, + scanMutableAllowlistEntries, +} from "./doctor/shared/mutable-allowlist.js"; +import { + collectOpenPolicyAllowFromWarnings, + maybeRepairOpenPolicyAllowFrom, +} from "./doctor/shared/open-policy-allowfrom.js"; export async function loadAndMaybeMigrateDoctorConfig(params: { options: DoctorOptions; @@ -225,25 +236,22 @@ export async function loadAndMaybeMigrateDoctorConfig(params: { } else { const hits = scanTelegramAllowFromUsernameEntries(candidate); if (hits.length > 0) { - const sampleEntry = sanitizeForLog(hits[0]?.entry ?? "@"); note( - [ - `- Telegram allowFrom contains ${hits.length} non-numeric entries (e.g. ${sampleEntry}); Telegram authorization requires numeric sender IDs.`, - `- Run "${formatCliCommand("openclaw doctor --fix")}" to auto-resolve @username entries to numeric IDs (requires a Telegram bot token).`, - ].join("\n"), + collectTelegramAllowFromUsernameWarnings({ + hits, + doctorFixCommand: formatCliCommand("openclaw doctor --fix"), + }).join("\n"), "Doctor warnings", ); } const discordHits = scanDiscordNumericIdEntries(candidate); if (discordHits.length > 0) { - const samplePath = sanitizeForLog(discordHits[0]?.path ?? "channels.discord.allowFrom"); - const sampleEntry = sanitizeForLog(String(discordHits[0]?.entry ?? "")); note( - [ - `- Discord allowlists contain ${discordHits.length} numeric entries (e.g. ${samplePath}=${sampleEntry}).`, - `- Discord IDs must be strings; run "${formatCliCommand("openclaw doctor --fix")}" to convert numeric IDs to quoted strings.`, - ].join("\n"), + collectDiscordNumericIdWarnings({ + hits: discordHits, + doctorFixCommand: formatCliCommand("openclaw doctor --fix"), + }).join("\n"), "Doctor warnings", ); } @@ -251,10 +259,10 @@ export async function loadAndMaybeMigrateDoctorConfig(params: { const allowFromScan = maybeRepairOpenPolicyAllowFrom(candidate); if (allowFromScan.changes.length > 0) { note( - [ - ...allowFromScan.changes.map((line) => sanitizeForLog(line)), - `- Run "${formatCliCommand("openclaw doctor --fix")}" to add missing allowFrom wildcards.`, - ].join("\n"), + collectOpenPolicyAllowFromWarnings({ + changes: allowFromScan.changes, + doctorFixCommand: formatCliCommand("openclaw doctor --fix"), + }).join("\n"), "Doctor warnings", ); } @@ -272,101 +280,38 @@ export async function loadAndMaybeMigrateDoctorConfig(params: { const toolsBySenderHits = scanLegacyToolsBySenderKeys(candidate); if (toolsBySenderHits.length > 0) { - const sample = toolsBySenderHits[0]; - const sampleLabel = sanitizeForLog( - sample ? `${sample.pathLabel}.${sample.key}` : "toolsBySender", - ); note( - [ - `- Found ${toolsBySenderHits.length} legacy untyped toolsBySender key${toolsBySenderHits.length === 1 ? "" : "s"} (for example ${sampleLabel}).`, - "- Untyped sender keys are deprecated; use explicit prefixes (id:, e164:, username:, name:).", - `- Run "${formatCliCommand("openclaw doctor --fix")}" to migrate legacy keys to typed id: entries.`, - ].join("\n"), + collectLegacyToolsBySenderWarnings({ + hits: toolsBySenderHits, + doctorFixCommand: formatCliCommand("openclaw doctor --fix"), + }).join("\n"), "Doctor warnings", ); } const safeBinCoverage = scanExecSafeBinCoverage(candidate); if (safeBinCoverage.length > 0) { - const interpreterHits = safeBinCoverage.filter((hit) => hit.isInterpreter); - const customHits = safeBinCoverage.filter((hit) => !hit.isInterpreter); - const lines: string[] = []; - if (interpreterHits.length > 0) { - for (const hit of interpreterHits.slice(0, 5)) { - lines.push( - `- ${sanitizeForLog(hit.scopePath)}.safeBins includes interpreter/runtime '${sanitizeForLog(hit.bin)}' without profile.`, - ); - } - if (interpreterHits.length > 5) { - lines.push( - `- ${interpreterHits.length - 5} more interpreter/runtime safeBins entries are missing profiles.`, - ); - } - } - if (customHits.length > 0) { - for (const hit of customHits.slice(0, 5)) { - lines.push( - `- ${sanitizeForLog(hit.scopePath)}.safeBins entry '${sanitizeForLog(hit.bin)}' is missing safeBinProfiles.${sanitizeForLog(hit.bin)}.`, - ); - } - if (customHits.length > 5) { - lines.push( - `- ${customHits.length - 5} more custom safeBins entries are missing profiles.`, - ); - } - } - lines.push( - `- Run "${formatCliCommand("openclaw doctor --fix")}" to scaffold missing custom safeBinProfiles entries.`, + note( + collectExecSafeBinCoverageWarnings({ + hits: safeBinCoverage, + doctorFixCommand: formatCliCommand("openclaw doctor --fix"), + }).join("\n"), + "Doctor warnings", ); - note(lines.join("\n"), "Doctor warnings"); } const safeBinTrustedDirHints = scanExecSafeBinTrustedDirHints(candidate); if (safeBinTrustedDirHints.length > 0) { - const lines = safeBinTrustedDirHints - .slice(0, 5) - .map( - (hit) => - `- ${sanitizeForLog(hit.scopePath)}.safeBins entry '${sanitizeForLog(hit.bin)}' resolves to '${sanitizeForLog(hit.resolvedPath)}' outside trusted safe-bin dirs.`, - ); - if (safeBinTrustedDirHints.length > 5) { - lines.push( - `- ${safeBinTrustedDirHints.length - 5} more safeBins entries resolve outside trusted safe-bin dirs.`, - ); - } - lines.push( - "- If intentional, add the binary directory to tools.exec.safeBinTrustedDirs (global or agent scope).", + note( + collectExecSafeBinTrustedDirHintWarnings(safeBinTrustedDirHints).join("\n"), + "Doctor warnings", ); - note(lines.join("\n"), "Doctor warnings"); } } const mutableAllowlistHits = scanMutableAllowlistEntries(candidate); if (mutableAllowlistHits.length > 0) { - const channels = Array.from(new Set(mutableAllowlistHits.map((hit) => hit.channel))).toSorted(); - const exampleLines = mutableAllowlistHits - .slice(0, 8) - .map((hit) => `- ${sanitizeForLog(hit.path)}: ${sanitizeForLog(hit.entry)}`) - .join("\n"); - const remaining = - mutableAllowlistHits.length > 8 - ? `- +${mutableAllowlistHits.length - 8} more mutable allowlist entries.` - : null; - const flagPaths = Array.from(new Set(mutableAllowlistHits.map((hit) => hit.dangerousFlagPath))); - const flagHint = - flagPaths.length === 1 - ? sanitizeForLog(flagPaths[0] ?? "") - : `${sanitizeForLog(flagPaths[0] ?? "")} (and ${flagPaths.length - 1} other scope flags)`; - note( - [ - `- Found ${mutableAllowlistHits.length} mutable allowlist ${mutableAllowlistHits.length === 1 ? "entry" : "entries"} across ${channels.join(", ")} while name matching is disabled by default.`, - exampleLines, - ...(remaining ? [remaining] : []), - `- Option A (break-glass): enable ${flagHint}=true to keep name/email/nick matching.`, - "- Option B (recommended): resolve names/emails/nicks to stable sender IDs and rewrite the allowlist entries.", - ].join("\n"), - "Doctor warnings", - ); + note(collectMutableAllowlistWarnings(mutableAllowlistHits).join("\n"), "Doctor warnings"); } const unknown = stripUnknownConfigKeys(candidate); diff --git a/src/commands/doctor/providers/discord.test.ts b/src/commands/doctor/providers/discord.test.ts index 152a5968411..c2d14be9888 100644 --- a/src/commands/doctor/providers/discord.test.ts +++ b/src/commands/doctor/providers/discord.test.ts @@ -1,6 +1,10 @@ import { describe, expect, it } from "vitest"; import type { OpenClawConfig } from "../../../config/config.js"; -import { maybeRepairDiscordNumericIds, scanDiscordNumericIdEntries } from "./discord.js"; +import { + collectDiscordNumericIdWarnings, + maybeRepairDiscordNumericIds, + scanDiscordNumericIdEntries, +} from "./discord.js"; describe("doctor discord provider repairs", () => { it("finds numeric id entries across discord scopes", () => { @@ -66,4 +70,16 @@ describe("doctor discord provider repairs", () => { "456", ]); }); + + it("formats numeric id warnings", () => { + const warnings = collectDiscordNumericIdWarnings({ + hits: [{ path: "channels.discord.allowFrom[0]", entry: 123 }], + doctorFixCommand: "openclaw doctor --fix", + }); + + expect(warnings).toEqual([ + expect.stringContaining("Discord allowlists contain 1 numeric entries"), + expect.stringContaining('run "openclaw doctor --fix"'), + ]); + }); }); diff --git a/src/commands/doctor/providers/discord.ts b/src/commands/doctor/providers/discord.ts index 480da9edd83..2ecc35b556e 100644 --- a/src/commands/doctor/providers/discord.ts +++ b/src/commands/doctor/providers/discord.ts @@ -1,4 +1,5 @@ import type { OpenClawConfig } from "../../../config/config.js"; +import { sanitizeForLog } from "../../../terminal/ansi.js"; import { asObjectRecord } from "../shared/object.js"; import type { DoctorAccountRecord } from "../types.js"; @@ -114,6 +115,21 @@ export function scanDiscordNumericIdEntries(cfg: OpenClawConfig): DiscordNumeric return hits; } +export function collectDiscordNumericIdWarnings(params: { + hits: DiscordNumericIdHit[]; + doctorFixCommand: string; +}): string[] { + if (params.hits.length === 0) { + return []; + } + const samplePath = sanitizeForLog(params.hits[0]?.path ?? "channels.discord.allowFrom"); + const sampleEntry = sanitizeForLog(String(params.hits[0]?.entry ?? "")); + return [ + `- Discord allowlists contain ${params.hits.length} numeric entries (e.g. ${samplePath}=${sampleEntry}).`, + `- Discord IDs must be strings; run "${params.doctorFixCommand}" to convert numeric IDs to quoted strings.`, + ]; +} + export function maybeRepairDiscordNumericIds(cfg: OpenClawConfig): { config: OpenClawConfig; changes: string[]; diff --git a/src/commands/doctor/providers/telegram.test.ts b/src/commands/doctor/providers/telegram.test.ts index e10f517938e..e67a81f6968 100644 --- a/src/commands/doctor/providers/telegram.test.ts +++ b/src/commands/doctor/providers/telegram.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from "vitest"; import { + collectTelegramAllowFromUsernameWarnings, collectTelegramEmptyAllowlistExtraWarnings, collectTelegramGroupPolicyWarnings, scanTelegramAllowFromUsernameEntries, @@ -120,4 +121,16 @@ describe("doctor telegram provider warnings", () => { }, ]); }); + + it("formats allowFrom username warnings", () => { + const warnings = collectTelegramAllowFromUsernameWarnings({ + hits: [{ path: "channels.telegram.allowFrom", entry: "@top" }], + doctorFixCommand: "openclaw doctor --fix", + }); + + expect(warnings).toEqual([ + expect.stringContaining("Telegram allowFrom contains 1 non-numeric entries"), + expect.stringContaining('Run "openclaw doctor --fix"'), + ]); + }); }); diff --git a/src/commands/doctor/providers/telegram.ts b/src/commands/doctor/providers/telegram.ts index 01fe1222b9a..03bc1416348 100644 --- a/src/commands/doctor/providers/telegram.ts +++ b/src/commands/doctor/providers/telegram.ts @@ -127,6 +127,20 @@ export function scanTelegramAllowFromUsernameEntries( return hits; } +export function collectTelegramAllowFromUsernameWarnings(params: { + hits: TelegramAllowFromUsernameHit[]; + doctorFixCommand: string; +}): string[] { + if (params.hits.length === 0) { + return []; + } + const sampleEntry = sanitizeForLog(params.hits[0]?.entry ?? "@"); + return [ + `- Telegram allowFrom contains ${params.hits.length} non-numeric entries (e.g. ${sampleEntry}); Telegram authorization requires numeric sender IDs.`, + `- Run "${params.doctorFixCommand}" to auto-resolve @username entries to numeric IDs (requires a Telegram bot token).`, + ]; +} + export async function maybeRepairTelegramAllowFromUsernames(cfg: OpenClawConfig): Promise<{ config: OpenClawConfig; changes: string[]; diff --git a/src/commands/doctor/shared/exec-safe-bins.test.ts b/src/commands/doctor/shared/exec-safe-bins.test.ts index 8c803a71d03..74894506246 100644 --- a/src/commands/doctor/shared/exec-safe-bins.test.ts +++ b/src/commands/doctor/shared/exec-safe-bins.test.ts @@ -4,6 +4,8 @@ import { delimiter, join } from "node:path"; import { afterEach, describe, expect, it } from "vitest"; import type { OpenClawConfig } from "../../../config/config.js"; import { + collectExecSafeBinCoverageWarnings, + collectExecSafeBinTrustedDirHintWarnings, maybeRepairExecSafeBinProfiles, scanExecSafeBinCoverage, scanExecSafeBinTrustedDirHints, @@ -29,6 +31,22 @@ describe("doctor exec safe bin helpers", () => { expect(hits).toEqual([{ scopePath: "tools.exec", bin: "node", isInterpreter: true }]); }); + it("formats coverage warnings", () => { + const warnings = collectExecSafeBinCoverageWarnings({ + hits: [ + { scopePath: "tools.exec", bin: "node", isInterpreter: true }, + { scopePath: "agents.list.runner.tools.exec", bin: "jq", isInterpreter: false }, + ], + doctorFixCommand: "openclaw doctor --fix", + }); + + expect(warnings).toEqual([ + expect.stringContaining("tools.exec.safeBins includes interpreter/runtime 'node'"), + expect.stringContaining("agents.list.runner.tools.exec.safeBins entry 'jq'"), + expect.stringContaining('Run "openclaw doctor --fix"'), + ]); + }); + it("scaffolds custom safeBin profiles but warns on interpreters", () => { const result = maybeRepairExecSafeBinProfiles({ tools: { @@ -70,6 +88,13 @@ describe("doctor exec safe bin helpers", () => { resolvedPath: binPath, }); + expect(collectExecSafeBinTrustedDirHintWarnings(hits)).toEqual( + expect.arrayContaining([ + expect.stringContaining("tools.exec.safeBins entry 'custom-safe-bin'"), + expect.stringContaining("tools.exec.safeBinTrustedDirs"), + ]), + ); + rmSync(tempDir, { recursive: true, force: true }); }); }); diff --git a/src/commands/doctor/shared/exec-safe-bins.ts b/src/commands/doctor/shared/exec-safe-bins.ts index c59a940918d..a3e695f180b 100644 --- a/src/commands/doctor/shared/exec-safe-bins.ts +++ b/src/commands/doctor/shared/exec-safe-bins.ts @@ -9,6 +9,7 @@ import { isTrustedSafeBinPath, normalizeTrustedSafeBinDirs, } from "../../../infra/exec-safe-bin-trust.js"; +import { sanitizeForLog } from "../../../terminal/ansi.js"; import { asObjectRecord } from "./object.js"; export type ExecSafeBinCoverageHit = { @@ -153,6 +154,65 @@ export function scanExecSafeBinTrustedDirHints( return hits; } +export function collectExecSafeBinCoverageWarnings(params: { + hits: ExecSafeBinCoverageHit[]; + doctorFixCommand: string; +}): string[] { + if (params.hits.length === 0) { + return []; + } + const interpreterHits = params.hits.filter((hit) => hit.isInterpreter); + const customHits = params.hits.filter((hit) => !hit.isInterpreter); + const lines: string[] = []; + if (interpreterHits.length > 0) { + for (const hit of interpreterHits.slice(0, 5)) { + lines.push( + `- ${sanitizeForLog(hit.scopePath)}.safeBins includes interpreter/runtime '${sanitizeForLog(hit.bin)}' without profile.`, + ); + } + if (interpreterHits.length > 5) { + lines.push( + `- ${interpreterHits.length - 5} more interpreter/runtime safeBins entries are missing profiles.`, + ); + } + } + if (customHits.length > 0) { + for (const hit of customHits.slice(0, 5)) { + lines.push( + `- ${sanitizeForLog(hit.scopePath)}.safeBins entry '${sanitizeForLog(hit.bin)}' is missing safeBinProfiles.${sanitizeForLog(hit.bin)}.`, + ); + } + if (customHits.length > 5) { + lines.push(`- ${customHits.length - 5} more custom safeBins entries are missing profiles.`); + } + } + lines.push( + `- Run "${params.doctorFixCommand}" to scaffold missing custom safeBinProfiles entries.`, + ); + return lines; +} + +export function collectExecSafeBinTrustedDirHintWarnings( + hits: ExecSafeBinTrustedDirHintHit[], +): string[] { + if (hits.length === 0) { + return []; + } + const lines = hits + .slice(0, 5) + .map( + (hit) => + `- ${sanitizeForLog(hit.scopePath)}.safeBins entry '${sanitizeForLog(hit.bin)}' resolves to '${sanitizeForLog(hit.resolvedPath)}' outside trusted safe-bin dirs.`, + ); + if (hits.length > 5) { + lines.push(`- ${hits.length - 5} more safeBins entries resolve outside trusted safe-bin dirs.`); + } + lines.push( + "- If intentional, add the binary directory to tools.exec.safeBinTrustedDirs (global or agent scope).", + ); + return lines; +} + export function maybeRepairExecSafeBinProfiles(cfg: OpenClawConfig): { config: OpenClawConfig; changes: string[]; diff --git a/src/commands/doctor/shared/legacy-tools-by-sender.test.ts b/src/commands/doctor/shared/legacy-tools-by-sender.test.ts index b3a152c3d3a..d809f9fbae0 100644 --- a/src/commands/doctor/shared/legacy-tools-by-sender.test.ts +++ b/src/commands/doctor/shared/legacy-tools-by-sender.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from "vitest"; import type { OpenClawConfig } from "../../../config/config.js"; import { + collectLegacyToolsBySenderWarnings, maybeRepairLegacyToolsBySenderKeys, scanLegacyToolsBySenderKeys, } from "./legacy-tools-by-sender.js"; @@ -59,4 +60,24 @@ describe("doctor legacy toolsBySender helpers", () => { "id:alice": { deny: ["exec"] }, }); }); + + it("formats legacy sender key warnings", () => { + const warnings = collectLegacyToolsBySenderWarnings({ + hits: [ + { + toolsBySenderPath: ["channels", "whatsapp", "groups", "123@g.us", "toolsBySender"], + pathLabel: "channels.whatsapp.groups.123@g.us.toolsBySender", + key: "owner", + targetKey: "id:owner", + }, + ], + doctorFixCommand: "openclaw doctor --fix", + }); + + expect(warnings).toEqual([ + expect.stringContaining("legacy untyped toolsBySender key"), + expect.stringContaining("explicit prefixes"), + expect.stringContaining('Run "openclaw doctor --fix"'), + ]); + }); }); diff --git a/src/commands/doctor/shared/legacy-tools-by-sender.ts b/src/commands/doctor/shared/legacy-tools-by-sender.ts index 4f3e4b51195..cea7b446342 100644 --- a/src/commands/doctor/shared/legacy-tools-by-sender.ts +++ b/src/commands/doctor/shared/legacy-tools-by-sender.ts @@ -1,5 +1,6 @@ import type { OpenClawConfig } from "../../../config/config.js"; import { parseToolsBySenderTypedKey } from "../../../config/types.tools.js"; +import { sanitizeForLog } from "../../../terminal/ansi.js"; import { formatConfigPath, resolveConfigPathTarget } from "../../doctor-config-analysis.js"; import { asObjectRecord } from "./object.js"; @@ -58,6 +59,24 @@ export function scanLegacyToolsBySenderKeys(cfg: OpenClawConfig): LegacyToolsByS return hits; } +export function collectLegacyToolsBySenderWarnings(params: { + hits: LegacyToolsBySenderKeyHit[]; + doctorFixCommand: string; +}): string[] { + if (params.hits.length === 0) { + return []; + } + const sample = params.hits[0]; + const sampleLabel = sanitizeForLog( + sample ? `${sample.pathLabel}.${sample.key}` : "toolsBySender", + ); + return [ + `- Found ${params.hits.length} legacy untyped toolsBySender key${params.hits.length === 1 ? "" : "s"} (for example ${sampleLabel}).`, + "- Untyped sender keys are deprecated; use explicit prefixes (id:, e164:, username:, name:).", + `- Run "${params.doctorFixCommand}" to migrate legacy keys to typed id: entries.`, + ]; +} + export function maybeRepairLegacyToolsBySenderKeys(cfg: OpenClawConfig): { config: OpenClawConfig; changes: string[]; diff --git a/src/commands/doctor/shared/mutable-allowlist.test.ts b/src/commands/doctor/shared/mutable-allowlist.test.ts index 4de7be56416..3c5f2b1bb8a 100644 --- a/src/commands/doctor/shared/mutable-allowlist.test.ts +++ b/src/commands/doctor/shared/mutable-allowlist.test.ts @@ -1,5 +1,8 @@ import { describe, expect, it } from "vitest"; -import { scanMutableAllowlistEntries } from "./mutable-allowlist.js"; +import { + collectMutableAllowlistWarnings, + scanMutableAllowlistEntries, +} from "./mutable-allowlist.js"; describe("doctor mutable allowlist scanner", () => { it("finds mutable discord, irc, and zalouser entries when dangerous matching is disabled", () => { @@ -74,4 +77,31 @@ describe("doctor mutable allowlist scanner", () => { expect(hits).toEqual([]); }); + + it("formats mutable allowlist warnings", () => { + const warnings = collectMutableAllowlistWarnings([ + { + channel: "discord", + path: "channels.discord.allowFrom", + entry: "alice", + dangerousFlagPath: "channels.discord.dangerouslyAllowNameMatching", + }, + { + channel: "irc", + path: "channels.irc.allowFrom", + entry: "bob", + dangerousFlagPath: "channels.irc.dangerouslyAllowNameMatching", + }, + ]); + + expect(warnings).toEqual( + expect.arrayContaining([ + expect.stringContaining("mutable allowlist entries across discord, irc"), + expect.stringContaining("channels.discord.allowFrom: alice"), + expect.stringContaining("channels.irc.allowFrom: bob"), + expect.stringContaining("Option A"), + expect.stringContaining("Option B"), + ]), + ); + }); }); diff --git a/src/commands/doctor/shared/mutable-allowlist.ts b/src/commands/doctor/shared/mutable-allowlist.ts index b5840b2783a..74c4808e1dc 100644 --- a/src/commands/doctor/shared/mutable-allowlist.ts +++ b/src/commands/doctor/shared/mutable-allowlist.ts @@ -9,6 +9,7 @@ import { isSlackMutableAllowEntry, isZalouserMutableGroupEntry, } from "../../../security/mutable-allowlist-detectors.js"; +import { sanitizeForLog } from "../../../terminal/ansi.js"; import { asObjectRecord } from "./object.js"; export type MutableAllowlistHit = { @@ -303,3 +304,27 @@ export function scanMutableAllowlistEntries(cfg: OpenClawConfig): MutableAllowli return hits; } + +export function collectMutableAllowlistWarnings(hits: MutableAllowlistHit[]): string[] { + if (hits.length === 0) { + return []; + } + const channels = Array.from(new Set(hits.map((hit) => hit.channel))).toSorted(); + 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; + const flagPaths = Array.from(new Set(hits.map((hit) => hit.dangerousFlagPath))); + const flagHint = + flagPaths.length === 1 + ? sanitizeForLog(flagPaths[0] ?? "") + : `${sanitizeForLog(flagPaths[0] ?? "")} (and ${flagPaths.length - 1} other scope flags)`; + return [ + `- Found ${hits.length} mutable allowlist ${hits.length === 1 ? "entry" : "entries"} across ${channels.join(", ")} while name matching is disabled by default.`, + ...exampleLines, + ...(remaining ? [remaining] : []), + `- Option A (break-glass): enable ${flagHint}=true to keep name/email/nick matching.`, + "- Option B (recommended): resolve names/emails/nicks to stable sender IDs and rewrite the allowlist entries.", + ]; +} diff --git a/src/commands/doctor/shared/open-policy-allowfrom.test.ts b/src/commands/doctor/shared/open-policy-allowfrom.test.ts index 4d9813dbf87..b7f75dc8b7a 100644 --- a/src/commands/doctor/shared/open-policy-allowfrom.test.ts +++ b/src/commands/doctor/shared/open-policy-allowfrom.test.ts @@ -1,5 +1,8 @@ import { describe, expect, it } from "vitest"; -import { maybeRepairOpenPolicyAllowFrom } from "./open-policy-allowfrom.js"; +import { + collectOpenPolicyAllowFromWarnings, + maybeRepairOpenPolicyAllowFrom, +} from "./open-policy-allowfrom.js"; describe("doctor open-policy allowFrom repair", () => { it('adds top-level wildcard when dmPolicy="open" has no allowFrom', () => { @@ -51,4 +54,16 @@ describe("doctor open-policy allowFrom repair", () => { ]); expect(result.config.channels?.discord?.dm?.allowFrom).toEqual(["123", "*"]); }); + + it("formats open-policy wildcard warnings", () => { + const warnings = collectOpenPolicyAllowFromWarnings({ + changes: ['- channels.signal.allowFrom: set to ["*"] (required by dmPolicy="open")'], + doctorFixCommand: "openclaw doctor --fix", + }); + + expect(warnings).toEqual([ + expect.stringContaining('channels.signal.allowFrom: set to ["*"]'), + expect.stringContaining('Run "openclaw doctor --fix"'), + ]); + }); }); diff --git a/src/commands/doctor/shared/open-policy-allowfrom.ts b/src/commands/doctor/shared/open-policy-allowfrom.ts index 0f405dfd2a2..66ec37ff904 100644 --- a/src/commands/doctor/shared/open-policy-allowfrom.ts +++ b/src/commands/doctor/shared/open-policy-allowfrom.ts @@ -1,4 +1,5 @@ import type { OpenClawConfig } from "../../../config/config.js"; +import { sanitizeForLog } from "../../../terminal/ansi.js"; import { resolveAllowFromMode, type AllowFromMode } from "./allow-from-mode.js"; import { asObjectRecord } from "./object.js"; @@ -6,6 +7,19 @@ function hasWildcard(list?: Array) { return list?.some((v) => String(v).trim() === "*") ?? false; } +export function collectOpenPolicyAllowFromWarnings(params: { + changes: string[]; + doctorFixCommand: string; +}): string[] { + if (params.changes.length === 0) { + return []; + } + return [ + ...params.changes.map((line) => sanitizeForLog(line)), + `- Run "${params.doctorFixCommand}" to add missing allowFrom wildcards.`, + ]; +} + export function maybeRepairOpenPolicyAllowFrom(cfg: OpenClawConfig): { config: OpenClawConfig; changes: string[];