From 4e979ea6ca0d4ffb0b4eba8957a5cd18c9b1df36 Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Sat, 21 Mar 2026 10:09:38 -0700 Subject: [PATCH] refactor(doctor): extract provider and shared config helpers (#51753) * refactor(doctor): add shared doctor types * refactor(doctor): add shared allowlist helpers * refactor(doctor): extract empty allowlist warnings * refactor(doctor): extract telegram allowfrom scanning * refactor(doctor): extract telegram allowfrom repair * refactor(doctor): extract discord id repair * refactor(doctor): add shared object helpers * refactor(doctor): extract mutable allowlist scanning * refactor(doctor): extract open-policy allowfrom repair * refactor(doctor): extract allowlist policy repair * fix(doctor): unblock discord provider refactor checks * refactor(doctor): fix provider layering in shared warnings --- src/commands/doctor-config-flow.ts | 1176 +---------------- src/commands/doctor/providers/discord.test.ts | 69 + src/commands/doctor/providers/discord.ts | 161 +++ .../doctor/providers/telegram.test.ts | 41 +- src/commands/doctor/providers/telegram.ts | 298 ++++- src/commands/doctor/shared/allow-from-mode.ts | 11 + .../doctor/shared/allowlist-policy-repair.ts | 148 +++ src/commands/doctor/shared/allowlist.test.ts | 15 + src/commands/doctor/shared/allowlist.ts | 5 + .../shared/empty-allowlist-policy.test.ts | 41 + .../doctor/shared/empty-allowlist-policy.ts | 107 ++ .../doctor/shared/mutable-allowlist.test.ts | 77 ++ .../doctor/shared/mutable-allowlist.ts | 305 +++++ src/commands/doctor/shared/object.ts | 6 + .../shared/open-policy-allowfrom.test.ts | 54 + .../doctor/shared/open-policy-allowfrom.ts | 114 ++ src/commands/doctor/types.ts | 3 + 17 files changed, 1493 insertions(+), 1138 deletions(-) create mode 100644 src/commands/doctor/providers/discord.test.ts create mode 100644 src/commands/doctor/providers/discord.ts create mode 100644 src/commands/doctor/shared/allow-from-mode.ts create mode 100644 src/commands/doctor/shared/allowlist-policy-repair.ts create mode 100644 src/commands/doctor/shared/allowlist.test.ts create mode 100644 src/commands/doctor/shared/allowlist.ts create mode 100644 src/commands/doctor/shared/empty-allowlist-policy.test.ts create mode 100644 src/commands/doctor/shared/empty-allowlist-policy.ts create mode 100644 src/commands/doctor/shared/mutable-allowlist.test.ts create mode 100644 src/commands/doctor/shared/mutable-allowlist.ts create mode 100644 src/commands/doctor/shared/object.ts create mode 100644 src/commands/doctor/shared/open-policy-allowfrom.test.ts create mode 100644 src/commands/doctor/shared/open-policy-allowfrom.ts create mode 100644 src/commands/doctor/types.ts diff --git a/src/commands/doctor-config-flow.ts b/src/commands/doctor-config-flow.ts index f11b810e963..07243095f0d 100644 --- a/src/commands/doctor-config-flow.ts +++ b/src/commands/doctor-config-flow.ts @@ -1,21 +1,10 @@ -import { - inspectTelegramAccount, - isNumericTelegramUserId, - listTelegramAccountIds, - lookupTelegramChatId, - normalizeTelegramAllowFromEntry, -} from "../../extensions/telegram/api.js"; import { normalizeChatChannelId } from "../channels/registry.js"; import { formatCliCommand } from "../cli/command-format.js"; -import { resolveCommandSecretRefsViaGateway } from "../cli/command-secret-gateway.js"; -import { getChannelsCommandSecretTargetIds } from "../cli/command-secret-targets.js"; import { listRouteBindings } from "../config/bindings.js"; import type { OpenClawConfig } from "../config/config.js"; import { CONFIG_PATH, migrateLegacyConfig } from "../config/config.js"; -import { collectProviderDangerousNameMatchingScopes } from "../config/dangerous-name-matching.js"; import { formatConfigIssueLines } from "../config/issue-format.js"; import { applyPluginAutoEnable } from "../config/plugin-auto-enable.js"; -import type { TelegramNetworkConfig } from "../config/types.telegram.js"; import { parseToolsBySenderTypedKey } from "../config/types.tools.js"; import { resolveCommandResolutionFromArgv } from "../infra/exec-command-resolution.js"; import { @@ -44,8 +33,6 @@ import { detectPluginInstallPathIssue, formatPluginInstallPathIssue, } from "../infra/plugin-install-path-warnings.js"; -import { readChannelAllowFromStore } from "../pairing/pairing-store.js"; -import { resolveTelegramAccount } from "../plugin-sdk/account-resolution.js"; import { formatChannelAccountsDefaultPath, formatSetExplicitDefaultInstruction, @@ -56,16 +43,6 @@ import { normalizeAccountId, normalizeOptionalAccountId, } from "../routing/session-key.js"; -import { describeUnknownError } from "../secrets/shared.js"; -import { - isDiscordMutableAllowEntry, - isGoogleChatMutableAllowEntry, - isIrcMutableAllowEntry, - isMSTeamsMutableAllowEntry, - isMattermostMutableAllowEntry, - isSlackMutableAllowEntry, - isZalouserMutableGroupEntry, -} from "../security/mutable-allowlist-detectors.js"; import { note } from "../terminal/note.js"; import { formatConfigPath, @@ -76,29 +53,20 @@ import { import { runDoctorConfigPreflight } from "./doctor-config-preflight.js"; import { normalizeCompatibilityConfigValues } from "./doctor-legacy-config.js"; import type { DoctorOptions } from "./doctor-prompter.js"; -import { collectTelegramGroupPolicyWarnings } from "./doctor/providers/telegram.js"; - -type TelegramAllowFromUsernameHit = { path: string; entry: string }; - -type TelegramAllowFromListRef = { - pathLabel: string; - holder: Record; - key: "allowFrom" | "groupAllowFrom"; -}; - -type ResolvedTelegramLookupAccount = { - token: string; - apiRoot?: string; - proxyUrl?: string; - network?: TelegramNetworkConfig; -}; - -function asObjectRecord(value: unknown): Record | null { - if (!value || typeof value !== "object" || Array.isArray(value)) { - return null; - } - return value as Record; -} +import { + maybeRepairDiscordNumericIds, + scanDiscordNumericIdEntries, +} from "./doctor/providers/discord.js"; +import { + collectTelegramGroupPolicyWarnings, + maybeRepairTelegramAllowFromUsernames, + scanTelegramAllowFromUsernameEntries, +} from "./doctor/providers/telegram.js"; +import { maybeRepairAllowlistPolicyAllowFrom } from "./doctor/shared/allowlist-policy-repair.js"; +import { collectEmptyAllowlistPolicyWarningsForAccount } from "./doctor/shared/empty-allowlist-policy.js"; +import { scanMutableAllowlistEntries } from "./doctor/shared/mutable-allowlist.js"; +import { asObjectRecord } from "./doctor/shared/object.js"; +import { maybeRepairOpenPolicyAllowFrom } from "./doctor/shared/open-policy-allowfrom.js"; function normalizeBindingChannelKey(raw?: string | null): string { const normalized = normalizeChatChannelId(raw); @@ -243,101 +211,6 @@ export function collectMissingExplicitDefaultAccountWarnings(cfg: OpenClawConfig return warnings; } -function collectTelegramAccountScopes( - cfg: OpenClawConfig, -): Array<{ prefix: string; account: Record }> { - const scopes: Array<{ prefix: string; account: Record }> = []; - const telegram = asObjectRecord(cfg.channels?.telegram); - if (!telegram) { - return scopes; - } - - scopes.push({ prefix: "channels.telegram", account: telegram }); - const accounts = asObjectRecord(telegram.accounts); - if (!accounts) { - return scopes; - } - for (const key of Object.keys(accounts)) { - const account = asObjectRecord(accounts[key]); - if (!account) { - continue; - } - scopes.push({ prefix: `channels.telegram.accounts.${key}`, account }); - } - - return scopes; -} - -function collectTelegramAllowFromLists( - prefix: string, - account: Record, -): TelegramAllowFromListRef[] { - const refs: TelegramAllowFromListRef[] = [ - { pathLabel: `${prefix}.allowFrom`, holder: account, key: "allowFrom" }, - { pathLabel: `${prefix}.groupAllowFrom`, holder: account, key: "groupAllowFrom" }, - ]; - const groups = asObjectRecord(account.groups); - if (!groups) { - return refs; - } - - for (const groupId of Object.keys(groups)) { - const group = asObjectRecord(groups[groupId]); - if (!group) { - continue; - } - refs.push({ - pathLabel: `${prefix}.groups.${groupId}.allowFrom`, - holder: group, - key: "allowFrom", - }); - const topics = asObjectRecord(group.topics); - if (!topics) { - continue; - } - for (const topicId of Object.keys(topics)) { - const topic = asObjectRecord(topics[topicId]); - if (!topic) { - continue; - } - refs.push({ - pathLabel: `${prefix}.groups.${groupId}.topics.${topicId}.allowFrom`, - holder: topic, - key: "allowFrom", - }); - } - } - return refs; -} - -function scanTelegramAllowFromUsernameEntries(cfg: OpenClawConfig): TelegramAllowFromUsernameHit[] { - const hits: TelegramAllowFromUsernameHit[] = []; - - const scanList = (pathLabel: string, list: unknown) => { - if (!Array.isArray(list)) { - return; - } - for (const entry of list) { - const normalized = normalizeTelegramAllowFromEntry(entry); - if (!normalized || normalized === "*") { - continue; - } - if (isNumericTelegramUserId(normalized)) { - continue; - } - hits.push({ path: pathLabel, entry: String(entry).trim() }); - } - }; - - for (const scope of collectTelegramAccountScopes(cfg)) { - for (const ref of collectTelegramAllowFromLists(scope.prefix, scope.account)) { - scanList(ref.pathLabel, ref.holder[ref.key]); - } - } - - return hits; -} - function formatMatrixLegacyStatePreview( detection: Exclude, null | { warning: string }>, ): string { @@ -388,912 +261,6 @@ async function collectMatrixInstallPathWarnings(cfg: OpenClawConfig): Promise `- ${entry}`); } -async function maybeRepairTelegramAllowFromUsernames(cfg: OpenClawConfig): Promise<{ - config: OpenClawConfig; - changes: string[]; -}> { - const hits = scanTelegramAllowFromUsernameEntries(cfg); - if (hits.length === 0) { - return { config: cfg, changes: [] }; - } - - const { resolvedConfig } = await resolveCommandSecretRefsViaGateway({ - config: cfg, - commandName: "doctor --fix", - targetIds: getChannelsCommandSecretTargetIds(), - mode: "read_only_status", - }); - const hasConfiguredUnavailableToken = listTelegramAccountIds(cfg).some((accountId) => { - const inspected = inspectTelegramAccount({ cfg, accountId }); - return inspected.enabled && inspected.tokenStatus === "configured_unavailable"; - }); - const tokenResolutionWarnings: string[] = []; - const lookupAccounts: ResolvedTelegramLookupAccount[] = []; - const seenLookupAccounts = new Set(); - for (const accountId of listTelegramAccountIds(resolvedConfig)) { - let account: NonNullable>; - try { - account = resolveTelegramAccount({ cfg: resolvedConfig, accountId }); - } catch (error) { - tokenResolutionWarnings.push( - `- Telegram account ${accountId}: failed to inspect bot token (${describeUnknownError(error)}).`, - ); - continue; - } - const token = account.tokenSource === "none" ? "" : account.token.trim(); - if (!token) { - continue; - } - const apiRoot = account.config.apiRoot?.trim() || undefined; - const proxyUrl = account.config.proxy?.trim() || undefined; - const network = account.config.network; - const cacheKey = `${token}::${apiRoot ?? ""}::${proxyUrl ?? ""}::${JSON.stringify(network ?? {})}`; - if (seenLookupAccounts.has(cacheKey)) { - continue; - } - seenLookupAccounts.add(cacheKey); - lookupAccounts.push({ token, apiRoot, proxyUrl, network }); - } - - if (lookupAccounts.length === 0) { - return { - config: cfg, - changes: [ - ...tokenResolutionWarnings, - hasConfiguredUnavailableToken - ? `- Telegram allowFrom contains @username entries, but configured Telegram bot credentials are unavailable in this command path; cannot auto-resolve (start the gateway or make the secret source available, then rerun doctor --fix).` - : `- Telegram allowFrom contains @username entries, but no Telegram bot token is configured; cannot auto-resolve (run setup or replace with numeric sender IDs).`, - ], - }; - } - - const resolveUserId = async (raw: string): Promise => { - const trimmed = raw.trim(); - if (!trimmed) { - return null; - } - const stripped = normalizeTelegramAllowFromEntry(trimmed); - if (!stripped || stripped === "*") { - return null; - } - if (isNumericTelegramUserId(stripped)) { - return stripped; - } - if (/\s/.test(stripped)) { - return null; - } - const username = stripped.startsWith("@") ? stripped : `@${stripped}`; - for (const account of lookupAccounts) { - const controller = new AbortController(); - const timeout = setTimeout(() => controller.abort(), 4000); - try { - const id = await lookupTelegramChatId({ - token: account.token, - chatId: username, - signal: controller.signal, - apiRoot: account.apiRoot, - proxyUrl: account.proxyUrl, - network: account.network, - }); - if (id) { - return id; - } - } catch { - // ignore and try next token - } finally { - clearTimeout(timeout); - } - } - return null; - }; - - const changes: string[] = []; - const next = structuredClone(cfg); - - const repairList = async (pathLabel: string, holder: Record, key: string) => { - const raw = holder[key]; - if (!Array.isArray(raw)) { - return; - } - const out: Array = []; - const replaced: Array<{ from: string; to: string }> = []; - for (const entry of raw) { - const normalized = normalizeTelegramAllowFromEntry(entry); - if (!normalized) { - continue; - } - if (normalized === "*") { - out.push("*"); - continue; - } - if (isNumericTelegramUserId(normalized)) { - out.push(normalized); - continue; - } - const resolved = await resolveUserId(String(entry)); - if (resolved) { - out.push(resolved); - replaced.push({ from: String(entry).trim(), to: resolved }); - } else { - out.push(String(entry).trim()); - } - } - const deduped: Array = []; - const seen = new Set(); - for (const entry of out) { - const k = String(entry).trim(); - if (!k || seen.has(k)) { - continue; - } - seen.add(k); - deduped.push(entry); - } - holder[key] = deduped; - if (replaced.length > 0) { - for (const rep of replaced.slice(0, 5)) { - changes.push(`- ${pathLabel}: resolved ${rep.from} -> ${rep.to}`); - } - if (replaced.length > 5) { - changes.push(`- ${pathLabel}: resolved ${replaced.length - 5} more @username entries`); - } - } - }; - - const repairAccount = async (prefix: string, account: Record) => { - for (const ref of collectTelegramAllowFromLists(prefix, account)) { - await repairList(ref.pathLabel, ref.holder, ref.key); - } - }; - - for (const scope of collectTelegramAccountScopes(next)) { - await repairAccount(scope.prefix, scope.account); - } - - if (changes.length === 0) { - return { config: cfg, changes: [] }; - } - return { config: next, changes }; -} - -type DiscordNumericIdHit = { path: string; entry: number }; - -type DiscordIdListRef = { - pathLabel: string; - holder: Record; - key: string; -}; - -function collectDiscordAccountScopes( - cfg: OpenClawConfig, -): Array<{ prefix: string; account: Record }> { - const scopes: Array<{ prefix: string; account: Record }> = []; - const discord = asObjectRecord(cfg.channels?.discord); - if (!discord) { - return scopes; - } - - scopes.push({ prefix: "channels.discord", account: discord }); - const accounts = asObjectRecord(discord.accounts); - if (!accounts) { - return scopes; - } - for (const key of Object.keys(accounts)) { - const account = asObjectRecord(accounts[key]); - if (!account) { - continue; - } - scopes.push({ prefix: `channels.discord.accounts.${key}`, account }); - } - - return scopes; -} - -function collectDiscordIdLists( - prefix: string, - account: Record, -): DiscordIdListRef[] { - const refs: DiscordIdListRef[] = [ - { pathLabel: `${prefix}.allowFrom`, holder: account, key: "allowFrom" }, - ]; - const dm = asObjectRecord(account.dm); - if (dm) { - refs.push({ pathLabel: `${prefix}.dm.allowFrom`, holder: dm, key: "allowFrom" }); - refs.push({ pathLabel: `${prefix}.dm.groupChannels`, holder: dm, key: "groupChannels" }); - } - const execApprovals = asObjectRecord(account.execApprovals); - if (execApprovals) { - refs.push({ - pathLabel: `${prefix}.execApprovals.approvers`, - holder: execApprovals, - key: "approvers", - }); - } - const guilds = asObjectRecord(account.guilds); - if (!guilds) { - return refs; - } - - for (const guildId of Object.keys(guilds)) { - const guild = asObjectRecord(guilds[guildId]); - if (!guild) { - continue; - } - refs.push({ pathLabel: `${prefix}.guilds.${guildId}.users`, holder: guild, key: "users" }); - refs.push({ pathLabel: `${prefix}.guilds.${guildId}.roles`, holder: guild, key: "roles" }); - const channels = asObjectRecord(guild.channels); - if (!channels) { - continue; - } - for (const channelId of Object.keys(channels)) { - const channel = asObjectRecord(channels[channelId]); - if (!channel) { - continue; - } - refs.push({ - pathLabel: `${prefix}.guilds.${guildId}.channels.${channelId}.users`, - holder: channel, - key: "users", - }); - refs.push({ - pathLabel: `${prefix}.guilds.${guildId}.channels.${channelId}.roles`, - holder: channel, - key: "roles", - }); - } - } - return refs; -} - -function scanDiscordNumericIdEntries(cfg: OpenClawConfig): DiscordNumericIdHit[] { - const hits: DiscordNumericIdHit[] = []; - const scanList = (pathLabel: string, list: unknown) => { - if (!Array.isArray(list)) { - return; - } - for (const [index, entry] of list.entries()) { - if (typeof entry !== "number") { - continue; - } - hits.push({ path: `${pathLabel}[${index}]`, entry }); - } - }; - - for (const scope of collectDiscordAccountScopes(cfg)) { - for (const ref of collectDiscordIdLists(scope.prefix, scope.account)) { - scanList(ref.pathLabel, ref.holder[ref.key]); - } - } - - return hits; -} - -function maybeRepairDiscordNumericIds(cfg: OpenClawConfig): { - config: OpenClawConfig; - changes: string[]; -} { - const hits = scanDiscordNumericIdEntries(cfg); - if (hits.length === 0) { - return { config: cfg, changes: [] }; - } - - const next = structuredClone(cfg); - const changes: string[] = []; - - const repairList = (pathLabel: string, holder: Record, key: string) => { - const raw = holder[key]; - if (!Array.isArray(raw)) { - return; - } - let converted = 0; - const updated = raw.map((entry) => { - if (typeof entry === "number") { - converted += 1; - return String(entry); - } - return entry; - }); - if (converted === 0) { - return; - } - holder[key] = updated; - changes.push( - `- ${pathLabel}: converted ${converted} numeric ${converted === 1 ? "entry" : "entries"} to strings`, - ); - }; - - for (const scope of collectDiscordAccountScopes(next)) { - for (const ref of collectDiscordIdLists(scope.prefix, scope.account)) { - repairList(ref.pathLabel, ref.holder, ref.key); - } - } - - if (changes.length === 0) { - return { config: cfg, changes: [] }; - } - return { config: next, changes }; -} - -type MutableAllowlistHit = { - channel: string; - path: string; - entry: string; - dangerousFlagPath: string; -}; - -function addMutableAllowlistHits(params: { - hits: MutableAllowlistHit[]; - pathLabel: string; - list: unknown; - detector: (entry: string) => boolean; - channel: string; - dangerousFlagPath: string; -}) { - if (!Array.isArray(params.list)) { - return; - } - for (const entry of params.list) { - const text = String(entry).trim(); - if (!text || text === "*") { - continue; - } - if (!params.detector(text)) { - continue; - } - params.hits.push({ - channel: params.channel, - path: params.pathLabel, - entry: text, - dangerousFlagPath: params.dangerousFlagPath, - }); - } -} - -function scanMutableAllowlistEntries(cfg: OpenClawConfig): MutableAllowlistHit[] { - const hits: MutableAllowlistHit[] = []; - - for (const scope of collectProviderDangerousNameMatchingScopes(cfg, "discord")) { - if (scope.dangerousNameMatchingEnabled) { - continue; - } - addMutableAllowlistHits({ - hits, - pathLabel: `${scope.prefix}.allowFrom`, - list: scope.account.allowFrom, - detector: isDiscordMutableAllowEntry, - channel: "discord", - dangerousFlagPath: scope.dangerousFlagPath, - }); - const dm = asObjectRecord(scope.account.dm); - if (dm) { - addMutableAllowlistHits({ - hits, - pathLabel: `${scope.prefix}.dm.allowFrom`, - list: dm.allowFrom, - detector: isDiscordMutableAllowEntry, - channel: "discord", - dangerousFlagPath: scope.dangerousFlagPath, - }); - } - const guilds = asObjectRecord(scope.account.guilds); - if (!guilds) { - continue; - } - for (const [guildId, guildRaw] of Object.entries(guilds)) { - const guild = asObjectRecord(guildRaw); - if (!guild) { - continue; - } - addMutableAllowlistHits({ - hits, - pathLabel: `${scope.prefix}.guilds.${guildId}.users`, - list: guild.users, - detector: isDiscordMutableAllowEntry, - channel: "discord", - dangerousFlagPath: scope.dangerousFlagPath, - }); - const channels = asObjectRecord(guild.channels); - if (!channels) { - continue; - } - for (const [channelId, channelRaw] of Object.entries(channels)) { - const channel = asObjectRecord(channelRaw); - if (!channel) { - continue; - } - addMutableAllowlistHits({ - hits, - pathLabel: `${scope.prefix}.guilds.${guildId}.channels.${channelId}.users`, - list: channel.users, - detector: isDiscordMutableAllowEntry, - channel: "discord", - dangerousFlagPath: scope.dangerousFlagPath, - }); - } - } - } - - for (const scope of collectProviderDangerousNameMatchingScopes(cfg, "slack")) { - if (scope.dangerousNameMatchingEnabled) { - continue; - } - addMutableAllowlistHits({ - hits, - pathLabel: `${scope.prefix}.allowFrom`, - list: scope.account.allowFrom, - detector: isSlackMutableAllowEntry, - channel: "slack", - dangerousFlagPath: scope.dangerousFlagPath, - }); - const dm = asObjectRecord(scope.account.dm); - if (dm) { - addMutableAllowlistHits({ - hits, - pathLabel: `${scope.prefix}.dm.allowFrom`, - list: dm.allowFrom, - detector: isSlackMutableAllowEntry, - channel: "slack", - dangerousFlagPath: scope.dangerousFlagPath, - }); - } - const channels = asObjectRecord(scope.account.channels); - if (!channels) { - continue; - } - for (const [channelKey, channelRaw] of Object.entries(channels)) { - const channel = asObjectRecord(channelRaw); - if (!channel) { - continue; - } - addMutableAllowlistHits({ - hits, - pathLabel: `${scope.prefix}.channels.${channelKey}.users`, - list: channel.users, - detector: isSlackMutableAllowEntry, - channel: "slack", - dangerousFlagPath: scope.dangerousFlagPath, - }); - } - } - - for (const scope of collectProviderDangerousNameMatchingScopes(cfg, "googlechat")) { - if (scope.dangerousNameMatchingEnabled) { - continue; - } - addMutableAllowlistHits({ - hits, - pathLabel: `${scope.prefix}.groupAllowFrom`, - list: scope.account.groupAllowFrom, - detector: isGoogleChatMutableAllowEntry, - channel: "googlechat", - dangerousFlagPath: scope.dangerousFlagPath, - }); - const dm = asObjectRecord(scope.account.dm); - if (dm) { - addMutableAllowlistHits({ - hits, - pathLabel: `${scope.prefix}.dm.allowFrom`, - list: dm.allowFrom, - detector: isGoogleChatMutableAllowEntry, - channel: "googlechat", - dangerousFlagPath: scope.dangerousFlagPath, - }); - } - const groups = asObjectRecord(scope.account.groups); - if (!groups) { - continue; - } - for (const [groupKey, groupRaw] of Object.entries(groups)) { - const group = asObjectRecord(groupRaw); - if (!group) { - continue; - } - addMutableAllowlistHits({ - hits, - pathLabel: `${scope.prefix}.groups.${groupKey}.users`, - list: group.users, - detector: isGoogleChatMutableAllowEntry, - channel: "googlechat", - dangerousFlagPath: scope.dangerousFlagPath, - }); - } - } - - for (const scope of collectProviderDangerousNameMatchingScopes(cfg, "msteams")) { - if (scope.dangerousNameMatchingEnabled) { - continue; - } - addMutableAllowlistHits({ - hits, - pathLabel: `${scope.prefix}.allowFrom`, - list: scope.account.allowFrom, - detector: isMSTeamsMutableAllowEntry, - channel: "msteams", - dangerousFlagPath: scope.dangerousFlagPath, - }); - addMutableAllowlistHits({ - hits, - pathLabel: `${scope.prefix}.groupAllowFrom`, - list: scope.account.groupAllowFrom, - detector: isMSTeamsMutableAllowEntry, - channel: "msteams", - dangerousFlagPath: scope.dangerousFlagPath, - }); - } - - for (const scope of collectProviderDangerousNameMatchingScopes(cfg, "mattermost")) { - if (scope.dangerousNameMatchingEnabled) { - continue; - } - addMutableAllowlistHits({ - hits, - pathLabel: `${scope.prefix}.allowFrom`, - list: scope.account.allowFrom, - detector: isMattermostMutableAllowEntry, - channel: "mattermost", - dangerousFlagPath: scope.dangerousFlagPath, - }); - addMutableAllowlistHits({ - hits, - pathLabel: `${scope.prefix}.groupAllowFrom`, - list: scope.account.groupAllowFrom, - detector: isMattermostMutableAllowEntry, - channel: "mattermost", - dangerousFlagPath: scope.dangerousFlagPath, - }); - } - - for (const scope of collectProviderDangerousNameMatchingScopes(cfg, "irc")) { - if (scope.dangerousNameMatchingEnabled) { - continue; - } - addMutableAllowlistHits({ - hits, - pathLabel: `${scope.prefix}.allowFrom`, - list: scope.account.allowFrom, - detector: isIrcMutableAllowEntry, - channel: "irc", - dangerousFlagPath: scope.dangerousFlagPath, - }); - addMutableAllowlistHits({ - hits, - pathLabel: `${scope.prefix}.groupAllowFrom`, - list: scope.account.groupAllowFrom, - detector: isIrcMutableAllowEntry, - channel: "irc", - dangerousFlagPath: scope.dangerousFlagPath, - }); - const groups = asObjectRecord(scope.account.groups); - if (!groups) { - continue; - } - for (const [groupKey, groupRaw] of Object.entries(groups)) { - const group = asObjectRecord(groupRaw); - if (!group) { - continue; - } - addMutableAllowlistHits({ - hits, - pathLabel: `${scope.prefix}.groups.${groupKey}.allowFrom`, - list: group.allowFrom, - detector: isIrcMutableAllowEntry, - channel: "irc", - dangerousFlagPath: scope.dangerousFlagPath, - }); - } - } - - 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)) { - continue; - } - hits.push({ - channel: "zalouser", - path: `${scope.prefix}.groups`, - entry, - dangerousFlagPath: scope.dangerousFlagPath, - }); - } - } - - return hits; -} - -/** - * Scan all channel configs for dmPolicy="open" without allowFrom including "*". - * This configuration is rejected by the schema validator but can easily occur when - * users (or integrations) set dmPolicy to "open" without realising that an explicit - * allowFrom wildcard is also required. - */ -function maybeRepairOpenPolicyAllowFrom(cfg: OpenClawConfig): { - config: OpenClawConfig; - changes: string[]; -} { - const channels = cfg.channels; - if (!channels || typeof channels !== "object") { - return { config: cfg, changes: [] }; - } - - const next = structuredClone(cfg); - const changes: string[] = []; - - type OpenPolicyAllowFromMode = "topOnly" | "topOrNested" | "nestedOnly"; - - const resolveAllowFromMode = (channelName: string): OpenPolicyAllowFromMode => { - if (channelName === "googlechat") { - return "nestedOnly"; - } - if (channelName === "discord" || channelName === "slack") { - return "topOrNested"; - } - return "topOnly"; - }; - - const hasWildcard = (list?: Array) => - list?.some((v) => String(v).trim() === "*") ?? false; - - const ensureWildcard = ( - account: Record, - prefix: string, - mode: OpenPolicyAllowFromMode, - ) => { - const dmEntry = account.dm; - const dm = - dmEntry && typeof dmEntry === "object" && !Array.isArray(dmEntry) - ? (dmEntry as Record) - : undefined; - const dmPolicy = - (account.dmPolicy as string | undefined) ?? (dm?.policy as string | undefined) ?? undefined; - - if (dmPolicy !== "open") { - return; - } - - const topAllowFrom = account.allowFrom as Array | undefined; - const nestedAllowFrom = dm?.allowFrom as Array | undefined; - - if (mode === "nestedOnly") { - if (hasWildcard(nestedAllowFrom)) { - return; - } - if (Array.isArray(nestedAllowFrom)) { - nestedAllowFrom.push("*"); - changes.push(`- ${prefix}.dm.allowFrom: added "*" (required by dmPolicy="open")`); - return; - } - const nextDm = dm ?? {}; - nextDm.allowFrom = ["*"]; - account.dm = nextDm; - changes.push(`- ${prefix}.dm.allowFrom: set to ["*"] (required by dmPolicy="open")`); - return; - } - - if (mode === "topOrNested") { - if (hasWildcard(topAllowFrom) || hasWildcard(nestedAllowFrom)) { - return; - } - - if (Array.isArray(topAllowFrom)) { - topAllowFrom.push("*"); - changes.push(`- ${prefix}.allowFrom: added "*" (required by dmPolicy="open")`); - } else if (Array.isArray(nestedAllowFrom)) { - nestedAllowFrom.push("*"); - changes.push(`- ${prefix}.dm.allowFrom: added "*" (required by dmPolicy="open")`); - } else { - account.allowFrom = ["*"]; - changes.push(`- ${prefix}.allowFrom: set to ["*"] (required by dmPolicy="open")`); - } - return; - } - - if (hasWildcard(topAllowFrom)) { - return; - } - if (Array.isArray(topAllowFrom)) { - topAllowFrom.push("*"); - changes.push(`- ${prefix}.allowFrom: added "*" (required by dmPolicy="open")`); - } else { - account.allowFrom = ["*"]; - changes.push(`- ${prefix}.allowFrom: set to ["*"] (required by dmPolicy="open")`); - } - }; - - const nextChannels = next.channels as Record>; - for (const [channelName, channelConfig] of Object.entries(nextChannels)) { - if (!channelConfig || typeof channelConfig !== "object") { - continue; - } - - const allowFromMode = resolveAllowFromMode(channelName); - - // Check the top-level channel config - ensureWildcard(channelConfig, `channels.${channelName}`, allowFromMode); - - // Check per-account configs (e.g. channels.discord.accounts.mybot) - const accounts = channelConfig.accounts as Record> | undefined; - if (accounts && typeof accounts === "object") { - for (const [accountName, accountConfig] of Object.entries(accounts)) { - if (accountConfig && typeof accountConfig === "object") { - ensureWildcard( - accountConfig, - `channels.${channelName}.accounts.${accountName}`, - allowFromMode, - ); - } - } - } - } - - if (changes.length === 0) { - return { config: cfg, changes: [] }; - } - return { config: next, changes }; -} - -function hasAllowFromEntries(list?: Array) { - return Array.isArray(list) && list.map((v) => String(v).trim()).filter(Boolean).length > 0; -} - -async function maybeRepairAllowlistPolicyAllowFrom(cfg: OpenClawConfig): Promise<{ - config: OpenClawConfig; - changes: string[]; -}> { - const channels = cfg.channels; - if (!channels || typeof channels !== "object") { - return { config: cfg, changes: [] }; - } - - type AllowFromMode = "topOnly" | "topOrNested" | "nestedOnly"; - - const resolveAllowFromMode = (channelName: string): AllowFromMode => { - if (channelName === "googlechat") { - return "nestedOnly"; - } - if (channelName === "discord" || channelName === "slack") { - return "topOrNested"; - } - return "topOnly"; - }; - - const next = structuredClone(cfg); - const changes: string[] = []; - - const applyRecoveredAllowFrom = (params: { - account: Record; - allowFrom: string[]; - mode: AllowFromMode; - prefix: string; - }) => { - const count = params.allowFrom.length; - const noun = count === 1 ? "entry" : "entries"; - - if (params.mode === "nestedOnly") { - const dmEntry = params.account.dm; - const dm = - dmEntry && typeof dmEntry === "object" && !Array.isArray(dmEntry) - ? (dmEntry as Record) - : {}; - dm.allowFrom = params.allowFrom; - params.account.dm = dm; - changes.push( - `- ${params.prefix}.dm.allowFrom: restored ${count} sender ${noun} from pairing store (dmPolicy="allowlist").`, - ); - return; - } - - if (params.mode === "topOrNested") { - const dmEntry = params.account.dm; - const dm = - dmEntry && typeof dmEntry === "object" && !Array.isArray(dmEntry) - ? (dmEntry as Record) - : undefined; - const nestedAllowFrom = dm?.allowFrom as Array | undefined; - if (dm && !Array.isArray(params.account.allowFrom) && Array.isArray(nestedAllowFrom)) { - dm.allowFrom = params.allowFrom; - changes.push( - `- ${params.prefix}.dm.allowFrom: restored ${count} sender ${noun} from pairing store (dmPolicy="allowlist").`, - ); - return; - } - } - - params.account.allowFrom = params.allowFrom; - changes.push( - `- ${params.prefix}.allowFrom: restored ${count} sender ${noun} from pairing store (dmPolicy="allowlist").`, - ); - }; - - const recoverAllowFromForAccount = async (params: { - channelName: string; - account: Record; - accountId?: string; - prefix: string; - }) => { - const dmEntry = params.account.dm; - const dm = - dmEntry && typeof dmEntry === "object" && !Array.isArray(dmEntry) - ? (dmEntry as Record) - : undefined; - const dmPolicy = - (params.account.dmPolicy as string | undefined) ?? (dm?.policy as string | undefined); - if (dmPolicy !== "allowlist") { - return; - } - - const topAllowFrom = params.account.allowFrom as Array | undefined; - const nestedAllowFrom = dm?.allowFrom as Array | undefined; - if (hasAllowFromEntries(topAllowFrom) || hasAllowFromEntries(nestedAllowFrom)) { - return; - } - - const normalizedChannelId = (normalizeChatChannelId(params.channelName) ?? params.channelName) - .trim() - .toLowerCase(); - if (!normalizedChannelId) { - return; - } - const normalizedAccountId = normalizeAccountId(params.accountId) || DEFAULT_ACCOUNT_ID; - const fromStore = await readChannelAllowFromStore( - normalizedChannelId, - process.env, - normalizedAccountId, - ).catch(() => []); - const recovered = Array.from(new Set(fromStore.map((entry) => String(entry).trim()))).filter( - Boolean, - ); - if (recovered.length === 0) { - return; - } - - applyRecoveredAllowFrom({ - account: params.account, - allowFrom: recovered, - mode: resolveAllowFromMode(params.channelName), - prefix: params.prefix, - }); - }; - - const nextChannels = next.channels as Record>; - for (const [channelName, channelConfig] of Object.entries(nextChannels)) { - if (!channelConfig || typeof channelConfig !== "object") { - continue; - } - await recoverAllowFromForAccount({ - channelName, - account: channelConfig, - prefix: `channels.${channelName}`, - }); - - const accounts = channelConfig.accounts as Record> | undefined; - if (!accounts || typeof accounts !== "object") { - continue; - } - for (const [accountId, accountConfig] of Object.entries(accounts)) { - if (!accountConfig || typeof accountConfig !== "object") { - continue; - } - await recoverAllowFromForAccount({ - channelName, - account: accountConfig, - accountId, - prefix: `channels.${channelName}.accounts.${accountId}`, - }); - } - } - - if (changes.length === 0) { - return { config: cfg, changes: [] }; - } - return { config: next, changes }; -} - /** * Scan all channel configs for dmPolicy="allowlist" without any allowFrom entries. * This configuration blocks all DMs because no sender can match the empty @@ -1308,104 +275,51 @@ function detectEmptyAllowlistPolicy(cfg: OpenClawConfig): string[] { const warnings: string[] = []; - const usesSenderBasedGroupAllowlist = (channelName?: string): boolean => { - if (!channelName) { - return true; - } - // These channels enforce group access via channel/space config, not sender-based - // groupAllowFrom lists. - return !(channelName === "discord" || channelName === "slack" || channelName === "googlechat"); - }; - - const allowsGroupAllowFromFallback = (channelName?: string): boolean => { - if (!channelName) { - return true; - } - // Keep doctor warnings aligned with runtime access semantics. - return !( - channelName === "googlechat" || - channelName === "imessage" || - channelName === "matrix" || - channelName === "msteams" || - channelName === "irc" - ); - }; - const checkAccount = ( account: Record, prefix: string, parent?: Record, channelName?: string, ) => { - const dmEntry = account.dm; - const dm = - dmEntry && typeof dmEntry === "object" && !Array.isArray(dmEntry) - ? (dmEntry as Record) - : undefined; - const parentDmEntry = parent?.dm; - const parentDm = - parentDmEntry && typeof parentDmEntry === "object" && !Array.isArray(parentDmEntry) - ? (parentDmEntry as Record) - : undefined; + const accountDm = asObjectRecord(account.dm); + const parentDm = asObjectRecord(parent?.dm); const dmPolicy = (account.dmPolicy as string | undefined) ?? - (dm?.policy as string | undefined) ?? + (accountDm?.policy as string | undefined) ?? (parent?.dmPolicy as string | undefined) ?? (parentDm?.policy as string | undefined) ?? undefined; - - const topAllowFrom = + const effectiveAllowFrom = (account.allowFrom as Array | undefined) ?? - (parent?.allowFrom as Array | undefined); - const nestedAllowFrom = dm?.allowFrom as Array | undefined; - const parentNestedAllowFrom = parentDm?.allowFrom as Array | undefined; - const effectiveAllowFrom = topAllowFrom ?? nestedAllowFrom ?? parentNestedAllowFrom; - - if (dmPolicy === "allowlist" && !hasAllowFromEntries(effectiveAllowFrom)) { - warnings.push( - `- ${prefix}.dmPolicy is "allowlist" but allowFrom is empty — all DMs will be blocked. Add sender IDs to ${prefix}.allowFrom, or run "${formatCliCommand("openclaw doctor --fix")}" to auto-migrate from pairing store when entries exist.`, - ); - } - - const groupPolicy = - (account.groupPolicy as string | undefined) ?? - (parent?.groupPolicy as string | undefined) ?? + (parent?.allowFrom as Array | undefined) ?? + (accountDm?.allowFrom as Array | undefined) ?? + (parentDm?.allowFrom as Array | undefined) ?? undefined; - if (groupPolicy === "allowlist" && usesSenderBasedGroupAllowlist(channelName)) { - if (channelName === "telegram") { - warnings.push( - ...collectTelegramGroupPolicyWarnings({ - account, - prefix, - effectiveAllowFrom, - dmPolicy, - parent, - }), - ); - return; - } - const rawGroupAllowFrom = - (account.groupAllowFrom as Array | undefined) ?? - (parent?.groupAllowFrom as Array | undefined); - // Match runtime semantics: resolveGroupAllowFromSources treats - // empty arrays as unset and falls back to allowFrom. - const groupAllowFrom = hasAllowFromEntries(rawGroupAllowFrom) ? rawGroupAllowFrom : undefined; - const fallbackToAllowFrom = allowsGroupAllowFromFallback(channelName); - const effectiveGroupAllowFrom = - groupAllowFrom ?? (fallbackToAllowFrom ? effectiveAllowFrom : undefined); - - if (!hasAllowFromEntries(effectiveGroupAllowFrom)) { - if (fallbackToAllowFrom) { - warnings.push( - `- ${prefix}.groupPolicy is "allowlist" but groupAllowFrom (and allowFrom) is empty — all group messages will be silently dropped. Add sender IDs to ${prefix}.groupAllowFrom or ${prefix}.allowFrom, or set groupPolicy to "open".`, - ); - } else { - warnings.push( - `- ${prefix}.groupPolicy is "allowlist" but groupAllowFrom is empty — this channel does not fall back to allowFrom, so all group messages will be silently dropped. Add sender IDs to ${prefix}.groupAllowFrom, or set groupPolicy to "open".`, - ); - } - } + warnings.push( + ...collectEmptyAllowlistPolicyWarningsForAccount({ + account, + channelName, + doctorFixCommand: formatCliCommand("openclaw doctor --fix"), + parent, + prefix, + }), + ); + if ( + channelName === "telegram" && + ((account.groupPolicy as string | undefined) ?? + (parent?.groupPolicy as string | undefined) ?? + undefined) === "allowlist" + ) { + warnings.push( + ...collectTelegramGroupPolicyWarnings({ + account, + dmPolicy, + effectiveAllowFrom, + parent, + prefix, + }), + ); } }; diff --git a/src/commands/doctor/providers/discord.test.ts b/src/commands/doctor/providers/discord.test.ts new file mode 100644 index 00000000000..152a5968411 --- /dev/null +++ b/src/commands/doctor/providers/discord.test.ts @@ -0,0 +1,69 @@ +import { describe, expect, it } from "vitest"; +import type { OpenClawConfig } from "../../../config/config.js"; +import { maybeRepairDiscordNumericIds, scanDiscordNumericIdEntries } from "./discord.js"; + +describe("doctor discord provider repairs", () => { + it("finds numeric id entries across discord scopes", () => { + const cfg = { + channels: { + discord: { + allowFrom: [123], + dm: { allowFrom: ["ok"], groupChannels: [456] }, + execApprovals: { approvers: [789] }, + guilds: { + main: { + users: [111], + roles: [222], + channels: { + general: { + users: [333], + roles: [444], + }, + }, + }, + }, + }, + }, + } as unknown as OpenClawConfig; + + const hits = scanDiscordNumericIdEntries(cfg); + + expect(hits.map((hit) => hit.path)).toEqual([ + "channels.discord.allowFrom[0]", + "channels.discord.dm.groupChannels[0]", + "channels.discord.execApprovals.approvers[0]", + "channels.discord.guilds.main.users[0]", + "channels.discord.guilds.main.roles[0]", + "channels.discord.guilds.main.channels.general.users[0]", + "channels.discord.guilds.main.channels.general.roles[0]", + ]); + }); + + it("repairs numeric discord ids into strings", () => { + const cfg = { + channels: { + discord: { + allowFrom: [123], + accounts: { + work: { + execApprovals: { approvers: [456] }, + }, + }, + }, + }, + } as unknown as OpenClawConfig; + + const result = maybeRepairDiscordNumericIds(cfg); + + expect(result.changes).toEqual([ + expect.stringContaining("channels.discord.allowFrom: converted 1 numeric entry to strings"), + expect.stringContaining( + "channels.discord.accounts.work.execApprovals.approvers: converted 1 numeric entry to strings", + ), + ]); + expect(result.config.channels?.discord?.allowFrom).toEqual(["123"]); + expect(result.config.channels?.discord?.accounts?.work?.execApprovals?.approvers).toEqual([ + "456", + ]); + }); +}); diff --git a/src/commands/doctor/providers/discord.ts b/src/commands/doctor/providers/discord.ts new file mode 100644 index 00000000000..480da9edd83 --- /dev/null +++ b/src/commands/doctor/providers/discord.ts @@ -0,0 +1,161 @@ +import type { OpenClawConfig } from "../../../config/config.js"; +import { asObjectRecord } from "../shared/object.js"; +import type { DoctorAccountRecord } from "../types.js"; + +type DiscordNumericIdHit = { path: string; entry: number }; + +type DiscordIdListRef = { + pathLabel: string; + holder: Record; + key: string; +}; + +export function collectDiscordAccountScopes( + cfg: OpenClawConfig, +): Array<{ prefix: string; account: DoctorAccountRecord }> { + const scopes: Array<{ prefix: string; account: DoctorAccountRecord }> = []; + const discord = asObjectRecord(cfg.channels?.discord); + if (!discord) { + return scopes; + } + + scopes.push({ prefix: "channels.discord", account: discord }); + const accounts = asObjectRecord(discord.accounts); + if (!accounts) { + return scopes; + } + for (const key of Object.keys(accounts)) { + const account = asObjectRecord(accounts[key]); + if (!account) { + continue; + } + scopes.push({ prefix: `channels.discord.accounts.${key}`, account }); + } + + return scopes; +} + +export function collectDiscordIdLists( + prefix: string, + account: DoctorAccountRecord, +): DiscordIdListRef[] { + const refs: DiscordIdListRef[] = [ + { pathLabel: `${prefix}.allowFrom`, holder: account, key: "allowFrom" }, + ]; + const dm = asObjectRecord(account.dm); + if (dm) { + refs.push({ pathLabel: `${prefix}.dm.allowFrom`, holder: dm, key: "allowFrom" }); + refs.push({ pathLabel: `${prefix}.dm.groupChannels`, holder: dm, key: "groupChannels" }); + } + const execApprovals = asObjectRecord(account.execApprovals); + if (execApprovals) { + refs.push({ + pathLabel: `${prefix}.execApprovals.approvers`, + holder: execApprovals, + key: "approvers", + }); + } + const guilds = asObjectRecord(account.guilds); + if (!guilds) { + return refs; + } + + for (const guildId of Object.keys(guilds)) { + const guild = asObjectRecord(guilds[guildId]); + if (!guild) { + continue; + } + refs.push({ pathLabel: `${prefix}.guilds.${guildId}.users`, holder: guild, key: "users" }); + refs.push({ pathLabel: `${prefix}.guilds.${guildId}.roles`, holder: guild, key: "roles" }); + const channels = asObjectRecord(guild.channels); + if (!channels) { + continue; + } + for (const channelId of Object.keys(channels)) { + const channel = asObjectRecord(channels[channelId]); + if (!channel) { + continue; + } + refs.push({ + pathLabel: `${prefix}.guilds.${guildId}.channels.${channelId}.users`, + holder: channel, + key: "users", + }); + refs.push({ + pathLabel: `${prefix}.guilds.${guildId}.channels.${channelId}.roles`, + holder: channel, + key: "roles", + }); + } + } + return refs; +} + +export function scanDiscordNumericIdEntries(cfg: OpenClawConfig): DiscordNumericIdHit[] { + const hits: DiscordNumericIdHit[] = []; + const scanList = (pathLabel: string, list: unknown) => { + if (!Array.isArray(list)) { + return; + } + for (const [index, entry] of list.entries()) { + if (typeof entry !== "number") { + continue; + } + hits.push({ path: `${pathLabel}[${index}]`, entry }); + } + }; + + for (const scope of collectDiscordAccountScopes(cfg)) { + for (const ref of collectDiscordIdLists(scope.prefix, scope.account)) { + scanList(ref.pathLabel, ref.holder[ref.key]); + } + } + + return hits; +} + +export function maybeRepairDiscordNumericIds(cfg: OpenClawConfig): { + config: OpenClawConfig; + changes: string[]; +} { + const hits = scanDiscordNumericIdEntries(cfg); + if (hits.length === 0) { + return { config: cfg, changes: [] }; + } + + const next = structuredClone(cfg); + const changes: string[] = []; + + const repairList = (pathLabel: string, holder: Record, key: string) => { + const raw = holder[key]; + if (!Array.isArray(raw)) { + return; + } + let converted = 0; + const updated = raw.map((entry) => { + if (typeof entry === "number") { + converted += 1; + return String(entry); + } + return entry; + }); + if (converted === 0) { + return; + } + holder[key] = updated; + changes.push( + `- ${pathLabel}: converted ${converted} numeric ${converted === 1 ? "entry" : "entries"} to strings`, + ); + }; + + for (const scope of collectDiscordAccountScopes(next)) { + for (const ref of collectDiscordIdLists(scope.prefix, scope.account)) { + repairList(ref.pathLabel, ref.holder, ref.key); + } + } + + if (changes.length === 0) { + return { config: cfg, changes: [] }; + } + return { config: next, changes }; +} diff --git a/src/commands/doctor/providers/telegram.test.ts b/src/commands/doctor/providers/telegram.test.ts index 9ea9d39ea61..6d9295dc334 100644 --- a/src/commands/doctor/providers/telegram.test.ts +++ b/src/commands/doctor/providers/telegram.test.ts @@ -1,5 +1,8 @@ import { describe, expect, it } from "vitest"; -import { collectTelegramGroupPolicyWarnings } from "./telegram.js"; +import { + collectTelegramGroupPolicyWarnings, + scanTelegramAllowFromUsernameEntries, +} from "./telegram.js"; describe("doctor telegram provider warnings", () => { it("shows first-run guidance when groups are not configured yet", () => { @@ -53,4 +56,40 @@ describe("doctor telegram provider warnings", () => { expect(warnings).toEqual([]); }); + + it("finds non-numeric telegram allowFrom username entries across account scopes", () => { + const hits = scanTelegramAllowFromUsernameEntries({ + channels: { + telegram: { + allowFrom: ["@top"], + groupAllowFrom: ["12345"], + accounts: { + work: { + allowFrom: ["tg:@work"], + groups: { + "-100123": { + allowFrom: ["topic-user"], + topics: { + "99": { + allowFrom: ["777", "@topic-user"], + }, + }, + }, + }, + }, + }, + }, + }, + }); + + expect(hits).toEqual([ + { path: "channels.telegram.allowFrom", entry: "@top" }, + { path: "channels.telegram.accounts.work.allowFrom", entry: "tg:@work" }, + { path: "channels.telegram.accounts.work.groups.-100123.allowFrom", entry: "topic-user" }, + { + path: "channels.telegram.accounts.work.groups.-100123.topics.99.allowFrom", + entry: "@topic-user", + }, + ]); + }); }); diff --git a/src/commands/doctor/providers/telegram.ts b/src/commands/doctor/providers/telegram.ts index 74398d79023..f2a326e8fa3 100644 --- a/src/commands/doctor/providers/telegram.ts +++ b/src/commands/doctor/providers/telegram.ts @@ -1,7 +1,293 @@ -type DoctorAccountRecord = Record; +import { + inspectTelegramAccount, + isNumericTelegramUserId, + listTelegramAccountIds, + lookupTelegramChatId, + normalizeTelegramAllowFromEntry, +} from "../../../../extensions/telegram/api.js"; +import { resolveCommandSecretRefsViaGateway } from "../../../cli/command-secret-gateway.js"; +import { getChannelsCommandSecretTargetIds } from "../../../cli/command-secret-targets.js"; +import type { OpenClawConfig } from "../../../config/config.js"; +import type { TelegramNetworkConfig } from "../../../config/types.telegram.js"; +import { resolveTelegramAccount } from "../../../plugin-sdk/account-resolution.js"; +import { describeUnknownError } from "../../../secrets/shared.js"; +import { hasAllowFromEntries } from "../shared/allowlist.js"; +import { asObjectRecord } from "../shared/object.js"; +import type { DoctorAccountRecord, DoctorAllowFromList } from "../types.js"; -function hasAllowFromEntries(list?: Array) { - return Array.isArray(list) && list.map((v) => String(v).trim()).filter(Boolean).length > 0; +type TelegramAllowFromUsernameHit = { path: string; entry: string }; + +type TelegramAllowFromListRef = { + pathLabel: string; + holder: Record; + key: "allowFrom" | "groupAllowFrom"; +}; + +type ResolvedTelegramLookupAccount = { + token: string; + apiRoot?: string; + proxyUrl?: string; + network?: TelegramNetworkConfig; +}; + +export function collectTelegramAccountScopes( + cfg: OpenClawConfig, +): Array<{ prefix: string; account: Record }> { + const scopes: Array<{ prefix: string; account: Record }> = []; + const telegram = asObjectRecord(cfg.channels?.telegram); + if (!telegram) { + return scopes; + } + + scopes.push({ prefix: "channels.telegram", account: telegram }); + const accounts = asObjectRecord(telegram.accounts); + if (!accounts) { + return scopes; + } + for (const key of Object.keys(accounts)) { + const account = asObjectRecord(accounts[key]); + if (!account) { + continue; + } + scopes.push({ prefix: `channels.telegram.accounts.${key}`, account }); + } + + return scopes; +} + +export function collectTelegramAllowFromLists( + prefix: string, + account: Record, +): TelegramAllowFromListRef[] { + const refs: TelegramAllowFromListRef[] = [ + { pathLabel: `${prefix}.allowFrom`, holder: account, key: "allowFrom" }, + { pathLabel: `${prefix}.groupAllowFrom`, holder: account, key: "groupAllowFrom" }, + ]; + const groups = asObjectRecord(account.groups); + if (!groups) { + return refs; + } + + for (const groupId of Object.keys(groups)) { + const group = asObjectRecord(groups[groupId]); + if (!group) { + continue; + } + refs.push({ + pathLabel: `${prefix}.groups.${groupId}.allowFrom`, + holder: group, + key: "allowFrom", + }); + const topics = asObjectRecord(group.topics); + if (!topics) { + continue; + } + for (const topicId of Object.keys(topics)) { + const topic = asObjectRecord(topics[topicId]); + if (!topic) { + continue; + } + refs.push({ + pathLabel: `${prefix}.groups.${groupId}.topics.${topicId}.allowFrom`, + holder: topic, + key: "allowFrom", + }); + } + } + return refs; +} + +export function scanTelegramAllowFromUsernameEntries( + cfg: OpenClawConfig, +): TelegramAllowFromUsernameHit[] { + const hits: TelegramAllowFromUsernameHit[] = []; + + const scanList = (pathLabel: string, list: unknown) => { + if (!Array.isArray(list)) { + return; + } + for (const entry of list) { + const normalized = normalizeTelegramAllowFromEntry(entry); + if (!normalized || normalized === "*") { + continue; + } + if (isNumericTelegramUserId(normalized)) { + continue; + } + hits.push({ path: pathLabel, entry: String(entry).trim() }); + } + }; + + for (const scope of collectTelegramAccountScopes(cfg)) { + for (const ref of collectTelegramAllowFromLists(scope.prefix, scope.account)) { + scanList(ref.pathLabel, ref.holder[ref.key]); + } + } + + return hits; +} + +export async function maybeRepairTelegramAllowFromUsernames(cfg: OpenClawConfig): Promise<{ + config: OpenClawConfig; + changes: string[]; +}> { + const hits = scanTelegramAllowFromUsernameEntries(cfg); + if (hits.length === 0) { + return { config: cfg, changes: [] }; + } + + const { resolvedConfig } = await resolveCommandSecretRefsViaGateway({ + config: cfg, + commandName: "doctor --fix", + targetIds: getChannelsCommandSecretTargetIds(), + mode: "read_only_status", + }); + const hasConfiguredUnavailableToken = listTelegramAccountIds(cfg).some((accountId) => { + const inspected = inspectTelegramAccount({ cfg, accountId }); + return inspected.enabled && inspected.tokenStatus === "configured_unavailable"; + }); + const tokenResolutionWarnings: string[] = []; + const lookupAccounts: ResolvedTelegramLookupAccount[] = []; + const seenLookupAccounts = new Set(); + for (const accountId of listTelegramAccountIds(resolvedConfig)) { + let account: NonNullable>; + try { + account = resolveTelegramAccount({ cfg: resolvedConfig, accountId }); + } catch (error) { + tokenResolutionWarnings.push( + `- Telegram account ${accountId}: failed to inspect bot token (${describeUnknownError(error)}).`, + ); + continue; + } + const token = account.tokenSource === "none" ? "" : account.token.trim(); + if (!token) { + continue; + } + const apiRoot = account.config.apiRoot?.trim() || undefined; + const proxyUrl = account.config.proxy?.trim() || undefined; + const network = account.config.network; + const cacheKey = `${token}::${apiRoot ?? ""}::${proxyUrl ?? ""}::${JSON.stringify(network ?? {})}`; + if (seenLookupAccounts.has(cacheKey)) { + continue; + } + seenLookupAccounts.add(cacheKey); + lookupAccounts.push({ token, apiRoot, proxyUrl, network }); + } + + if (lookupAccounts.length === 0) { + return { + config: cfg, + changes: [ + ...tokenResolutionWarnings, + hasConfiguredUnavailableToken + ? `- Telegram allowFrom contains @username entries, but configured Telegram bot credentials are unavailable in this command path; cannot auto-resolve (start the gateway or make the secret source available, then rerun doctor --fix).` + : `- Telegram allowFrom contains @username entries, but no Telegram bot token is configured; cannot auto-resolve (run setup or replace with numeric sender IDs).`, + ], + }; + } + + const resolveUserId = async (raw: string): Promise => { + const trimmed = raw.trim(); + if (!trimmed) { + return null; + } + const stripped = normalizeTelegramAllowFromEntry(trimmed); + if (!stripped || stripped === "*") { + return null; + } + if (isNumericTelegramUserId(stripped)) { + return stripped; + } + if (/\s/.test(stripped)) { + return null; + } + const username = stripped.startsWith("@") ? stripped : `@${stripped}`; + for (const account of lookupAccounts) { + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 4000); + try { + const id = await lookupTelegramChatId({ + token: account.token, + chatId: username, + signal: controller.signal, + apiRoot: account.apiRoot, + proxyUrl: account.proxyUrl, + network: account.network, + }); + if (id) { + return id; + } + } catch { + // ignore and try next token + } finally { + clearTimeout(timeout); + } + } + return null; + }; + + const changes: string[] = []; + const next = structuredClone(cfg); + + const repairList = async (pathLabel: string, holder: Record, key: string) => { + const raw = holder[key]; + if (!Array.isArray(raw)) { + return; + } + const out: DoctorAllowFromList = []; + const replaced: Array<{ from: string; to: string }> = []; + for (const entry of raw) { + const normalized = normalizeTelegramAllowFromEntry(entry); + if (!normalized) { + continue; + } + if (normalized === "*") { + out.push("*"); + continue; + } + if (isNumericTelegramUserId(normalized)) { + out.push(normalized); + continue; + } + const resolved = await resolveUserId(String(entry)); + if (resolved) { + out.push(resolved); + replaced.push({ from: String(entry).trim(), to: resolved }); + } else { + out.push(String(entry).trim()); + } + } + const deduped: DoctorAllowFromList = []; + const seen = new Set(); + for (const entry of out) { + const keyValue = String(entry).trim(); + if (!keyValue || seen.has(keyValue)) { + continue; + } + seen.add(keyValue); + deduped.push(entry); + } + holder[key] = deduped; + if (replaced.length > 0) { + for (const rep of replaced.slice(0, 5)) { + changes.push(`- ${pathLabel}: resolved ${rep.from} -> ${rep.to}`); + } + if (replaced.length > 5) { + changes.push(`- ${pathLabel}: resolved ${replaced.length - 5} more @username entries`); + } + } + }; + + for (const scope of collectTelegramAccountScopes(next)) { + for (const ref of collectTelegramAllowFromLists(scope.prefix, scope.account)) { + await repairList(ref.pathLabel, ref.holder, ref.key); + } + } + + if (changes.length === 0) { + return { config: cfg, changes: [] }; + } + return { config: next, changes }; } function hasConfiguredGroups(account: DoctorAccountRecord, parent?: DoctorAccountRecord): boolean { @@ -14,7 +300,7 @@ function hasConfiguredGroups(account: DoctorAccountRecord, parent?: DoctorAccoun type CollectTelegramGroupPolicyWarningsParams = { account: DoctorAccountRecord; prefix: string; - effectiveAllowFrom?: Array; + effectiveAllowFrom?: DoctorAllowFromList; dmPolicy?: string; parent?: DoctorAccountRecord; }; @@ -38,8 +324,8 @@ export function collectTelegramGroupPolicyWarnings( } const rawGroupAllowFrom = - (params.account.groupAllowFrom as Array | undefined) ?? - (params.parent?.groupAllowFrom as Array | undefined); + (params.account.groupAllowFrom as DoctorAllowFromList | undefined) ?? + (params.parent?.groupAllowFrom as DoctorAllowFromList | undefined); // Match runtime semantics: resolveGroupAllowFromSources treats empty arrays as // unset and falls back to allowFrom. const groupAllowFrom = hasAllowFromEntries(rawGroupAllowFrom) ? rawGroupAllowFrom : undefined; diff --git a/src/commands/doctor/shared/allow-from-mode.ts b/src/commands/doctor/shared/allow-from-mode.ts new file mode 100644 index 00000000000..e865f790b16 --- /dev/null +++ b/src/commands/doctor/shared/allow-from-mode.ts @@ -0,0 +1,11 @@ +export type AllowFromMode = "topOnly" | "topOrNested" | "nestedOnly"; + +export function resolveAllowFromMode(channelName: string): AllowFromMode { + if (channelName === "googlechat") { + return "nestedOnly"; + } + if (channelName === "discord" || channelName === "slack") { + return "topOrNested"; + } + return "topOnly"; +} diff --git a/src/commands/doctor/shared/allowlist-policy-repair.ts b/src/commands/doctor/shared/allowlist-policy-repair.ts new file mode 100644 index 00000000000..b4655b4d3cc --- /dev/null +++ b/src/commands/doctor/shared/allowlist-policy-repair.ts @@ -0,0 +1,148 @@ +import { normalizeChatChannelId } from "../../../channels/registry.js"; +import type { OpenClawConfig } from "../../../config/config.js"; +import { readChannelAllowFromStore } from "../../../pairing/pairing-store.js"; +import { DEFAULT_ACCOUNT_ID, normalizeAccountId } from "../../../routing/session-key.js"; +import { resolveAllowFromMode, type AllowFromMode } from "./allow-from-mode.js"; +import { hasAllowFromEntries } from "./allowlist.js"; +import { asObjectRecord } from "./object.js"; + +export async function maybeRepairAllowlistPolicyAllowFrom(cfg: OpenClawConfig): Promise<{ + config: OpenClawConfig; + changes: string[]; +}> { + const channels = cfg.channels; + if (!channels || typeof channels !== "object") { + return { config: cfg, changes: [] }; + } + + const next = structuredClone(cfg); + const changes: string[] = []; + + const applyRecoveredAllowFrom = (params: { + account: Record; + allowFrom: string[]; + mode: AllowFromMode; + prefix: string; + }) => { + const count = params.allowFrom.length; + const noun = count === 1 ? "entry" : "entries"; + + if (params.mode === "nestedOnly") { + const dmEntry = params.account.dm; + const dm = + dmEntry && typeof dmEntry === "object" && !Array.isArray(dmEntry) + ? (dmEntry as Record) + : {}; + dm.allowFrom = params.allowFrom; + params.account.dm = dm; + changes.push( + `- ${params.prefix}.dm.allowFrom: restored ${count} sender ${noun} from pairing store (dmPolicy="allowlist").`, + ); + return; + } + + if (params.mode === "topOrNested") { + const dmEntry = params.account.dm; + const dm = + dmEntry && typeof dmEntry === "object" && !Array.isArray(dmEntry) + ? (dmEntry as Record) + : undefined; + const nestedAllowFrom = dm?.allowFrom as Array | undefined; + if (dm && !Array.isArray(params.account.allowFrom) && Array.isArray(nestedAllowFrom)) { + dm.allowFrom = params.allowFrom; + changes.push( + `- ${params.prefix}.dm.allowFrom: restored ${count} sender ${noun} from pairing store (dmPolicy="allowlist").`, + ); + return; + } + } + + params.account.allowFrom = params.allowFrom; + changes.push( + `- ${params.prefix}.allowFrom: restored ${count} sender ${noun} from pairing store (dmPolicy="allowlist").`, + ); + }; + + const recoverAllowFromForAccount = async (params: { + channelName: string; + account: Record; + accountId?: string; + prefix: string; + }) => { + const dmEntry = params.account.dm; + const dm = + dmEntry && typeof dmEntry === "object" && !Array.isArray(dmEntry) + ? (dmEntry as Record) + : undefined; + const dmPolicy = + (params.account.dmPolicy as string | undefined) ?? (dm?.policy as string | undefined); + if (dmPolicy !== "allowlist") { + return; + } + + const topAllowFrom = params.account.allowFrom as Array | undefined; + const nestedAllowFrom = dm?.allowFrom as Array | undefined; + if (hasAllowFromEntries(topAllowFrom) || hasAllowFromEntries(nestedAllowFrom)) { + return; + } + + const normalizedChannelId = (normalizeChatChannelId(params.channelName) ?? params.channelName) + .trim() + .toLowerCase(); + if (!normalizedChannelId) { + return; + } + const normalizedAccountId = normalizeAccountId(params.accountId) || DEFAULT_ACCOUNT_ID; + const fromStore = await readChannelAllowFromStore( + normalizedChannelId, + process.env, + normalizedAccountId, + ).catch(() => []); + const recovered = Array.from(new Set(fromStore.map((entry) => String(entry).trim()))).filter( + Boolean, + ); + if (recovered.length === 0) { + return; + } + + applyRecoveredAllowFrom({ + account: params.account, + allowFrom: recovered, + mode: resolveAllowFromMode(params.channelName), + prefix: params.prefix, + }); + }; + + const nextChannels = next.channels as Record>; + for (const [channelName, channelConfig] of Object.entries(nextChannels)) { + if (!channelConfig || typeof channelConfig !== "object") { + continue; + } + await recoverAllowFromForAccount({ + channelName, + account: channelConfig, + prefix: `channels.${channelName}`, + }); + + const accounts = asObjectRecord(channelConfig.accounts); + if (!accounts) { + continue; + } + for (const [accountId, accountConfig] of Object.entries(accounts)) { + if (!accountConfig || typeof accountConfig !== "object") { + continue; + } + await recoverAllowFromForAccount({ + channelName, + account: accountConfig as Record, + accountId, + prefix: `channels.${channelName}.accounts.${accountId}`, + }); + } + } + + if (changes.length === 0) { + return { config: cfg, changes: [] }; + } + return { config: next, changes }; +} diff --git a/src/commands/doctor/shared/allowlist.test.ts b/src/commands/doctor/shared/allowlist.test.ts new file mode 100644 index 00000000000..832bcdce426 --- /dev/null +++ b/src/commands/doctor/shared/allowlist.test.ts @@ -0,0 +1,15 @@ +import { describe, expect, it } from "vitest"; +import { hasAllowFromEntries } from "./allowlist.js"; + +describe("doctor allowlist helpers", () => { + it("returns false for missing and blank entries", () => { + expect(hasAllowFromEntries()).toBe(false); + expect(hasAllowFromEntries([])).toBe(false); + expect(hasAllowFromEntries(["", " "])).toBe(false); + }); + + it("returns true when at least one trimmed entry is present", () => { + expect(hasAllowFromEntries([" ", "12345"])).toBe(true); + expect(hasAllowFromEntries([0, " "])).toBe(true); + }); +}); diff --git a/src/commands/doctor/shared/allowlist.ts b/src/commands/doctor/shared/allowlist.ts new file mode 100644 index 00000000000..fbe1b448295 --- /dev/null +++ b/src/commands/doctor/shared/allowlist.ts @@ -0,0 +1,5 @@ +import type { DoctorAllowFromList } from "../types.js"; + +export function hasAllowFromEntries(list?: DoctorAllowFromList) { + return Array.isArray(list) && list.map((v) => String(v).trim()).filter(Boolean).length > 0; +} diff --git a/src/commands/doctor/shared/empty-allowlist-policy.test.ts b/src/commands/doctor/shared/empty-allowlist-policy.test.ts new file mode 100644 index 00000000000..b9f469d8264 --- /dev/null +++ b/src/commands/doctor/shared/empty-allowlist-policy.test.ts @@ -0,0 +1,41 @@ +import { describe, expect, it } from "vitest"; +import { collectEmptyAllowlistPolicyWarningsForAccount } from "./empty-allowlist-policy.js"; + +describe("doctor empty allowlist policy warnings", () => { + it("warns when dm allowlist mode has no allowFrom entries", () => { + const warnings = collectEmptyAllowlistPolicyWarningsForAccount({ + account: { dmPolicy: "allowlist" }, + channelName: "signal", + doctorFixCommand: "openclaw doctor --fix", + prefix: "channels.signal", + }); + + expect(warnings).toEqual([ + expect.stringContaining('channels.signal.dmPolicy is "allowlist" but allowFrom is empty'), + ]); + }); + + it("warns when non-telegram group allowlist mode does not fall back to allowFrom", () => { + const warnings = collectEmptyAllowlistPolicyWarningsForAccount({ + account: { groupPolicy: "allowlist" }, + channelName: "imessage", + doctorFixCommand: "openclaw doctor --fix", + prefix: "channels.imessage", + }); + + expect(warnings).toEqual([ + expect.stringContaining("this channel does not fall back to allowFrom"), + ]); + }); + + it("stays quiet for channels that do not use sender-based group allowlists", () => { + const warnings = collectEmptyAllowlistPolicyWarningsForAccount({ + account: { groupPolicy: "allowlist" }, + channelName: "discord", + doctorFixCommand: "openclaw doctor --fix", + prefix: "channels.discord", + }); + + expect(warnings).toEqual([]); + }); +}); diff --git a/src/commands/doctor/shared/empty-allowlist-policy.ts b/src/commands/doctor/shared/empty-allowlist-policy.ts new file mode 100644 index 00000000000..a18e3de975b --- /dev/null +++ b/src/commands/doctor/shared/empty-allowlist-policy.ts @@ -0,0 +1,107 @@ +import type { DoctorAccountRecord, DoctorAllowFromList } from "../types.js"; +import { hasAllowFromEntries } from "./allowlist.js"; + +type CollectEmptyAllowlistPolicyWarningsParams = { + account: DoctorAccountRecord; + channelName?: string; + doctorFixCommand: string; + parent?: DoctorAccountRecord; + prefix: string; +}; + +function usesSenderBasedGroupAllowlist(channelName?: string): boolean { + if (!channelName) { + return true; + } + // These channels enforce group access via channel/space config, not sender-based + // groupAllowFrom lists. + return !(channelName === "discord" || channelName === "slack" || channelName === "googlechat"); +} + +function allowsGroupAllowFromFallback(channelName?: string): boolean { + if (!channelName) { + return true; + } + // Keep doctor warnings aligned with runtime access semantics. + return !( + channelName === "googlechat" || + channelName === "imessage" || + channelName === "matrix" || + channelName === "msteams" || + channelName === "irc" + ); +} + +export function collectEmptyAllowlistPolicyWarningsForAccount( + params: CollectEmptyAllowlistPolicyWarningsParams, +): string[] { + const warnings: string[] = []; + const dmEntry = params.account.dm; + const dm = + dmEntry && typeof dmEntry === "object" && !Array.isArray(dmEntry) + ? (dmEntry as DoctorAccountRecord) + : undefined; + const parentDmEntry = params.parent?.dm; + const parentDm = + parentDmEntry && typeof parentDmEntry === "object" && !Array.isArray(parentDmEntry) + ? (parentDmEntry as DoctorAccountRecord) + : undefined; + const dmPolicy = + (params.account.dmPolicy as string | undefined) ?? + (dm?.policy as string | undefined) ?? + (params.parent?.dmPolicy as string | undefined) ?? + (parentDm?.policy as string | undefined) ?? + undefined; + + const topAllowFrom = + (params.account.allowFrom as DoctorAllowFromList | undefined) ?? + (params.parent?.allowFrom as DoctorAllowFromList | undefined); + const nestedAllowFrom = dm?.allowFrom as DoctorAllowFromList | undefined; + const parentNestedAllowFrom = parentDm?.allowFrom as DoctorAllowFromList | undefined; + const effectiveAllowFrom = topAllowFrom ?? nestedAllowFrom ?? parentNestedAllowFrom; + + if (dmPolicy === "allowlist" && !hasAllowFromEntries(effectiveAllowFrom)) { + warnings.push( + `- ${params.prefix}.dmPolicy is "allowlist" but allowFrom is empty — all DMs will be blocked. Add sender IDs to ${params.prefix}.allowFrom, or run "${params.doctorFixCommand}" to auto-migrate from pairing store when entries exist.`, + ); + } + + const groupPolicy = + (params.account.groupPolicy as string | undefined) ?? + (params.parent?.groupPolicy as string | undefined) ?? + undefined; + + if (groupPolicy !== "allowlist" || !usesSenderBasedGroupAllowlist(params.channelName)) { + return warnings; + } + + if (params.channelName === "telegram") { + return warnings; + } + + const rawGroupAllowFrom = + (params.account.groupAllowFrom as DoctorAllowFromList | undefined) ?? + (params.parent?.groupAllowFrom as DoctorAllowFromList | undefined); + // Match runtime semantics: resolveGroupAllowFromSources treats empty arrays as + // unset and falls back to allowFrom. + const groupAllowFrom = hasAllowFromEntries(rawGroupAllowFrom) ? rawGroupAllowFrom : undefined; + const fallbackToAllowFrom = allowsGroupAllowFromFallback(params.channelName); + const effectiveGroupAllowFrom = + groupAllowFrom ?? (fallbackToAllowFrom ? effectiveAllowFrom : undefined); + + if (hasAllowFromEntries(effectiveGroupAllowFrom)) { + return warnings; + } + + if (fallbackToAllowFrom) { + warnings.push( + `- ${params.prefix}.groupPolicy is "allowlist" but groupAllowFrom (and allowFrom) is empty — all group messages will be silently dropped. Add sender IDs to ${params.prefix}.groupAllowFrom or ${params.prefix}.allowFrom, or set groupPolicy to "open".`, + ); + } else { + warnings.push( + `- ${params.prefix}.groupPolicy is "allowlist" but groupAllowFrom is empty — this channel does not fall back to allowFrom, so all group messages will be silently dropped. Add sender IDs to ${params.prefix}.groupAllowFrom, or set groupPolicy to "open".`, + ); + } + + return warnings; +} diff --git a/src/commands/doctor/shared/mutable-allowlist.test.ts b/src/commands/doctor/shared/mutable-allowlist.test.ts new file mode 100644 index 00000000000..4de7be56416 --- /dev/null +++ b/src/commands/doctor/shared/mutable-allowlist.test.ts @@ -0,0 +1,77 @@ +import { describe, expect, it } from "vitest"; +import { scanMutableAllowlistEntries } from "./mutable-allowlist.js"; + +describe("doctor mutable allowlist scanner", () => { + it("finds mutable discord, irc, and zalouser entries when dangerous matching is disabled", () => { + const hits = scanMutableAllowlistEntries({ + channels: { + discord: { + allowFrom: ["alice"], + guilds: { + ops: { + users: ["bob"], + roles: [], + channels: {}, + }, + }, + }, + irc: { + allowFrom: ["charlie"], + groups: { + "#ops": { + allowFrom: ["dana"], + }, + }, + }, + zalouser: { + groups: { + "Ops Room": { allow: true }, + }, + }, + }, + }); + + expect(hits).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + channel: "discord", + path: "channels.discord.allowFrom", + entry: "alice", + }), + expect.objectContaining({ + channel: "discord", + path: "channels.discord.guilds.ops.users", + entry: "bob", + }), + expect.objectContaining({ + channel: "irc", + path: "channels.irc.allowFrom", + entry: "charlie", + }), + expect.objectContaining({ + channel: "irc", + path: "channels.irc.groups.#ops.allowFrom", + entry: "dana", + }), + expect.objectContaining({ + channel: "zalouser", + path: "channels.zalouser.groups", + entry: "Ops Room", + }), + ]), + ); + }); + + it("skips scopes that explicitly allow dangerous name matching", () => { + const hits = scanMutableAllowlistEntries({ + channels: { + slack: { + dangerouslyAllowNameMatching: true, + allowFrom: ["alice"], + }, + }, + }); + + expect(hits).toEqual([]); + }); +}); diff --git a/src/commands/doctor/shared/mutable-allowlist.ts b/src/commands/doctor/shared/mutable-allowlist.ts new file mode 100644 index 00000000000..b5840b2783a --- /dev/null +++ b/src/commands/doctor/shared/mutable-allowlist.ts @@ -0,0 +1,305 @@ +import type { OpenClawConfig } from "../../../config/config.js"; +import { collectProviderDangerousNameMatchingScopes } from "../../../config/dangerous-name-matching.js"; +import { + isDiscordMutableAllowEntry, + isGoogleChatMutableAllowEntry, + isIrcMutableAllowEntry, + isMSTeamsMutableAllowEntry, + isMattermostMutableAllowEntry, + isSlackMutableAllowEntry, + isZalouserMutableGroupEntry, +} from "../../../security/mutable-allowlist-detectors.js"; +import { asObjectRecord } from "./object.js"; + +export type MutableAllowlistHit = { + channel: string; + path: string; + entry: string; + dangerousFlagPath: string; +}; + +function addMutableAllowlistHits(params: { + hits: MutableAllowlistHit[]; + pathLabel: string; + list: unknown; + detector: (entry: string) => boolean; + channel: string; + dangerousFlagPath: string; +}) { + if (!Array.isArray(params.list)) { + return; + } + for (const entry of params.list) { + const text = String(entry).trim(); + if (!text || text === "*") { + continue; + } + if (!params.detector(text)) { + continue; + } + params.hits.push({ + channel: params.channel, + path: params.pathLabel, + entry: text, + dangerousFlagPath: params.dangerousFlagPath, + }); + } +} + +export function scanMutableAllowlistEntries(cfg: OpenClawConfig): MutableAllowlistHit[] { + const hits: MutableAllowlistHit[] = []; + + for (const scope of collectProviderDangerousNameMatchingScopes(cfg, "discord")) { + if (scope.dangerousNameMatchingEnabled) { + continue; + } + addMutableAllowlistHits({ + hits, + pathLabel: `${scope.prefix}.allowFrom`, + list: scope.account.allowFrom, + detector: isDiscordMutableAllowEntry, + channel: "discord", + dangerousFlagPath: scope.dangerousFlagPath, + }); + const dm = asObjectRecord(scope.account.dm); + if (dm) { + addMutableAllowlistHits({ + hits, + pathLabel: `${scope.prefix}.dm.allowFrom`, + list: dm.allowFrom, + detector: isDiscordMutableAllowEntry, + channel: "discord", + dangerousFlagPath: scope.dangerousFlagPath, + }); + } + const guilds = asObjectRecord(scope.account.guilds); + if (!guilds) { + continue; + } + for (const [guildId, guildRaw] of Object.entries(guilds)) { + const guild = asObjectRecord(guildRaw); + if (!guild) { + continue; + } + addMutableAllowlistHits({ + hits, + pathLabel: `${scope.prefix}.guilds.${guildId}.users`, + list: guild.users, + detector: isDiscordMutableAllowEntry, + channel: "discord", + dangerousFlagPath: scope.dangerousFlagPath, + }); + const channels = asObjectRecord(guild.channels); + if (!channels) { + continue; + } + for (const [channelId, channelRaw] of Object.entries(channels)) { + const channel = asObjectRecord(channelRaw); + if (!channel) { + continue; + } + addMutableAllowlistHits({ + hits, + pathLabel: `${scope.prefix}.guilds.${guildId}.channels.${channelId}.users`, + list: channel.users, + detector: isDiscordMutableAllowEntry, + channel: "discord", + dangerousFlagPath: scope.dangerousFlagPath, + }); + } + } + } + + for (const scope of collectProviderDangerousNameMatchingScopes(cfg, "slack")) { + if (scope.dangerousNameMatchingEnabled) { + continue; + } + addMutableAllowlistHits({ + hits, + pathLabel: `${scope.prefix}.allowFrom`, + list: scope.account.allowFrom, + detector: isSlackMutableAllowEntry, + channel: "slack", + dangerousFlagPath: scope.dangerousFlagPath, + }); + const dm = asObjectRecord(scope.account.dm); + if (dm) { + addMutableAllowlistHits({ + hits, + pathLabel: `${scope.prefix}.dm.allowFrom`, + list: dm.allowFrom, + detector: isSlackMutableAllowEntry, + channel: "slack", + dangerousFlagPath: scope.dangerousFlagPath, + }); + } + const channels = asObjectRecord(scope.account.channels); + if (!channels) { + continue; + } + for (const [channelKey, channelRaw] of Object.entries(channels)) { + const channel = asObjectRecord(channelRaw); + if (!channel) { + continue; + } + addMutableAllowlistHits({ + hits, + pathLabel: `${scope.prefix}.channels.${channelKey}.users`, + list: channel.users, + detector: isSlackMutableAllowEntry, + channel: "slack", + dangerousFlagPath: scope.dangerousFlagPath, + }); + } + } + + for (const scope of collectProviderDangerousNameMatchingScopes(cfg, "googlechat")) { + if (scope.dangerousNameMatchingEnabled) { + continue; + } + addMutableAllowlistHits({ + hits, + pathLabel: `${scope.prefix}.groupAllowFrom`, + list: scope.account.groupAllowFrom, + detector: isGoogleChatMutableAllowEntry, + channel: "googlechat", + dangerousFlagPath: scope.dangerousFlagPath, + }); + const dm = asObjectRecord(scope.account.dm); + if (dm) { + addMutableAllowlistHits({ + hits, + pathLabel: `${scope.prefix}.dm.allowFrom`, + list: dm.allowFrom, + detector: isGoogleChatMutableAllowEntry, + channel: "googlechat", + dangerousFlagPath: scope.dangerousFlagPath, + }); + } + const groups = asObjectRecord(scope.account.groups); + if (!groups) { + continue; + } + for (const [groupKey, groupRaw] of Object.entries(groups)) { + const group = asObjectRecord(groupRaw); + if (!group) { + continue; + } + addMutableAllowlistHits({ + hits, + pathLabel: `${scope.prefix}.groups.${groupKey}.users`, + list: group.users, + detector: isGoogleChatMutableAllowEntry, + channel: "googlechat", + dangerousFlagPath: scope.dangerousFlagPath, + }); + } + } + + for (const scope of collectProviderDangerousNameMatchingScopes(cfg, "msteams")) { + if (scope.dangerousNameMatchingEnabled) { + continue; + } + addMutableAllowlistHits({ + hits, + pathLabel: `${scope.prefix}.allowFrom`, + list: scope.account.allowFrom, + detector: isMSTeamsMutableAllowEntry, + channel: "msteams", + dangerousFlagPath: scope.dangerousFlagPath, + }); + addMutableAllowlistHits({ + hits, + pathLabel: `${scope.prefix}.groupAllowFrom`, + list: scope.account.groupAllowFrom, + detector: isMSTeamsMutableAllowEntry, + channel: "msteams", + dangerousFlagPath: scope.dangerousFlagPath, + }); + } + + for (const scope of collectProviderDangerousNameMatchingScopes(cfg, "mattermost")) { + if (scope.dangerousNameMatchingEnabled) { + continue; + } + addMutableAllowlistHits({ + hits, + pathLabel: `${scope.prefix}.allowFrom`, + list: scope.account.allowFrom, + detector: isMattermostMutableAllowEntry, + channel: "mattermost", + dangerousFlagPath: scope.dangerousFlagPath, + }); + addMutableAllowlistHits({ + hits, + pathLabel: `${scope.prefix}.groupAllowFrom`, + list: scope.account.groupAllowFrom, + detector: isMattermostMutableAllowEntry, + channel: "mattermost", + dangerousFlagPath: scope.dangerousFlagPath, + }); + } + + for (const scope of collectProviderDangerousNameMatchingScopes(cfg, "irc")) { + if (scope.dangerousNameMatchingEnabled) { + continue; + } + addMutableAllowlistHits({ + hits, + pathLabel: `${scope.prefix}.allowFrom`, + list: scope.account.allowFrom, + detector: isIrcMutableAllowEntry, + channel: "irc", + dangerousFlagPath: scope.dangerousFlagPath, + }); + addMutableAllowlistHits({ + hits, + pathLabel: `${scope.prefix}.groupAllowFrom`, + list: scope.account.groupAllowFrom, + detector: isIrcMutableAllowEntry, + channel: "irc", + dangerousFlagPath: scope.dangerousFlagPath, + }); + const groups = asObjectRecord(scope.account.groups); + if (!groups) { + continue; + } + for (const [groupKey, groupRaw] of Object.entries(groups)) { + const group = asObjectRecord(groupRaw); + if (!group) { + continue; + } + addMutableAllowlistHits({ + hits, + pathLabel: `${scope.prefix}.groups.${groupKey}.allowFrom`, + list: group.allowFrom, + detector: isIrcMutableAllowEntry, + channel: "irc", + dangerousFlagPath: scope.dangerousFlagPath, + }); + } + } + + 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)) { + continue; + } + hits.push({ + channel: "zalouser", + path: `${scope.prefix}.groups`, + entry, + dangerousFlagPath: scope.dangerousFlagPath, + }); + } + } + + return hits; +} diff --git a/src/commands/doctor/shared/object.ts b/src/commands/doctor/shared/object.ts new file mode 100644 index 00000000000..502399ed22e --- /dev/null +++ b/src/commands/doctor/shared/object.ts @@ -0,0 +1,6 @@ +export function asObjectRecord(value: unknown): Record | null { + if (!value || typeof value !== "object" || Array.isArray(value)) { + return null; + } + return value as Record; +} diff --git a/src/commands/doctor/shared/open-policy-allowfrom.test.ts b/src/commands/doctor/shared/open-policy-allowfrom.test.ts new file mode 100644 index 00000000000..4d9813dbf87 --- /dev/null +++ b/src/commands/doctor/shared/open-policy-allowfrom.test.ts @@ -0,0 +1,54 @@ +import { describe, expect, it } from "vitest"; +import { maybeRepairOpenPolicyAllowFrom } from "./open-policy-allowfrom.js"; + +describe("doctor open-policy allowFrom repair", () => { + it('adds top-level wildcard when dmPolicy="open" has no allowFrom', () => { + const result = maybeRepairOpenPolicyAllowFrom({ + channels: { + signal: { + dmPolicy: "open", + }, + }, + }); + + expect(result.changes).toEqual([ + '- channels.signal.allowFrom: set to ["*"] (required by dmPolicy="open")', + ]); + expect(result.config.channels?.signal?.allowFrom).toEqual(["*"]); + }); + + it("repairs nested-only googlechat dm allowFrom", () => { + const result = maybeRepairOpenPolicyAllowFrom({ + channels: { + googlechat: { + dm: { + policy: "open", + }, + }, + }, + }); + + expect(result.changes).toEqual([ + '- channels.googlechat.dm.allowFrom: set to ["*"] (required by dmPolicy="open")', + ]); + expect(result.config.channels?.googlechat?.dm?.allowFrom).toEqual(["*"]); + }); + + it("appends wildcard to discord nested dm allowFrom when top-level is absent", () => { + const result = maybeRepairOpenPolicyAllowFrom({ + channels: { + discord: { + dm: { + policy: "open", + allowFrom: ["123"], + }, + }, + }, + }); + + expect(result.changes).toEqual([ + '- channels.discord.dm.allowFrom: added "*" (required by dmPolicy="open")', + ]); + expect(result.config.channels?.discord?.dm?.allowFrom).toEqual(["123", "*"]); + }); +}); diff --git a/src/commands/doctor/shared/open-policy-allowfrom.ts b/src/commands/doctor/shared/open-policy-allowfrom.ts new file mode 100644 index 00000000000..0f405dfd2a2 --- /dev/null +++ b/src/commands/doctor/shared/open-policy-allowfrom.ts @@ -0,0 +1,114 @@ +import type { OpenClawConfig } from "../../../config/config.js"; +import { resolveAllowFromMode, type AllowFromMode } from "./allow-from-mode.js"; +import { asObjectRecord } from "./object.js"; + +function hasWildcard(list?: Array) { + return list?.some((v) => String(v).trim() === "*") ?? false; +} + +export function maybeRepairOpenPolicyAllowFrom(cfg: OpenClawConfig): { + config: OpenClawConfig; + changes: string[]; +} { + const channels = cfg.channels; + if (!channels || typeof channels !== "object") { + return { config: cfg, changes: [] }; + } + + const next = structuredClone(cfg); + const changes: string[] = []; + + const ensureWildcard = ( + account: Record, + prefix: string, + mode: AllowFromMode, + ) => { + const dmEntry = account.dm; + const dm = + dmEntry && typeof dmEntry === "object" && !Array.isArray(dmEntry) + ? (dmEntry as Record) + : undefined; + const dmPolicy = + (account.dmPolicy as string | undefined) ?? (dm?.policy as string | undefined) ?? undefined; + + if (dmPolicy !== "open") { + return; + } + + const topAllowFrom = account.allowFrom as Array | undefined; + const nestedAllowFrom = dm?.allowFrom as Array | undefined; + + if (mode === "nestedOnly") { + if (hasWildcard(nestedAllowFrom)) { + return; + } + if (dm && Array.isArray(nestedAllowFrom)) { + dm.allowFrom = [...nestedAllowFrom, "*"]; + changes.push(`- ${prefix}.dm.allowFrom: added "*" (required by dmPolicy="open")`); + } else { + const nextDm = dm ?? {}; + nextDm.allowFrom = ["*"]; + account.dm = nextDm; + changes.push(`- ${prefix}.dm.allowFrom: set to ["*"] (required by dmPolicy="open")`); + } + return; + } + + if (mode === "topOrNested") { + if (hasWildcard(topAllowFrom) || hasWildcard(nestedAllowFrom)) { + return; + } + if (Array.isArray(topAllowFrom)) { + account.allowFrom = [...topAllowFrom, "*"]; + changes.push(`- ${prefix}.allowFrom: added "*" (required by dmPolicy="open")`); + } else if (dm && Array.isArray(nestedAllowFrom)) { + dm.allowFrom = [...nestedAllowFrom, "*"]; + changes.push(`- ${prefix}.dm.allowFrom: added "*" (required by dmPolicy="open")`); + } else { + account.allowFrom = ["*"]; + changes.push(`- ${prefix}.allowFrom: set to ["*"] (required by dmPolicy="open")`); + } + return; + } + + if (hasWildcard(topAllowFrom)) { + return; + } + if (Array.isArray(topAllowFrom)) { + account.allowFrom = [...topAllowFrom, "*"]; + changes.push(`- ${prefix}.allowFrom: added "*" (required by dmPolicy="open")`); + } else { + account.allowFrom = ["*"]; + changes.push(`- ${prefix}.allowFrom: set to ["*"] (required by dmPolicy="open")`); + } + }; + + const nextChannels = next.channels as Record>; + for (const [channelName, channelConfig] of Object.entries(nextChannels)) { + if (!channelConfig || typeof channelConfig !== "object") { + continue; + } + + const allowFromMode = resolveAllowFromMode(channelName); + ensureWildcard(channelConfig, `channels.${channelName}`, allowFromMode); + + const accounts = asObjectRecord(channelConfig.accounts); + if (!accounts) { + continue; + } + for (const [accountName, accountConfig] of Object.entries(accounts)) { + if (accountConfig && typeof accountConfig === "object") { + ensureWildcard( + accountConfig as Record, + `channels.${channelName}.accounts.${accountName}`, + allowFromMode, + ); + } + } + } + + if (changes.length === 0) { + return { config: cfg, changes: [] }; + } + return { config: next, changes }; +} diff --git a/src/commands/doctor/types.ts b/src/commands/doctor/types.ts new file mode 100644 index 00000000000..b6619112dd6 --- /dev/null +++ b/src/commands/doctor/types.ts @@ -0,0 +1,3 @@ +export type DoctorAccountRecord = Record; +export type DoctorAllowFromEntry = string | number; +export type DoctorAllowFromList = DoctorAllowFromEntry[];