fix(doctor): harden follow-up repair paths (#51888)

* fix(doctor): harden follow-up repair paths * fix(doctor): sanitize remaining warning paths
2026-04-26 16:41:49 +00:00 · 2026-03-21 15:11:01 -07:00
parent 825d82b5c9
commit 21544f9e53
4 changed files with 125 additions and 31 deletions
--- a/src/commands/doctor-config-flow.test.ts
+++ b/src/commands/doctor-config-flow.test.ts
@@ -599,12 +599,10 @@ describe("doctor config flow", () => {
      } as unknown as Response;
    });
    vi.stubGlobal("fetch", globalFetch);
-    const proxyFetch = vi.fn();
    const telegramFetchModule = await import("../../extensions/telegram/src/fetch.js");
    const telegramProxyModule = await import("../../extensions/telegram/src/proxy.js");
    const resolveTelegramFetch = vi.spyOn(telegramFetchModule, "resolveTelegramFetch");
    const makeProxyFetch = vi.spyOn(telegramProxyModule, "makeProxyFetch");
-    makeProxyFetch.mockReturnValue(proxyFetch as unknown as typeof fetch);
    resolveTelegramFetch.mockReturnValue(fetchSpy as unknown as typeof fetch);
    try {
      const result = await runDoctorConfigWithInput({
@@ -705,13 +703,13 @@ describe("doctor config flow", () => {
    }
  });

-  it("uses account apiRoot when repairing Telegram allowFrom usernames", async () => {
+  it("ignores custom Telegram apiRoot and proxy when repairing allowFrom usernames", async () => {
    const globalFetch = vi.fn(async () => {
      throw new Error("global fetch should not be called");
    });
    const fetchSpy = vi.fn(async (input: RequestInfo | URL) => {
      const url = input instanceof URL ? input.href : typeof input === "string" ? input : input.url;
-      expect(url).toBe("https://custom.telegram.test/root/bottok/getChat?chat_id=%40testuser");
+      expect(url).toBe("https://api.telegram.org/bottok/getChat?chat_id=%40testuser");
      return {
        ok: true,
        json: async () => ({ ok: true, result: { id: 12345 } }),
@@ -774,8 +772,8 @@ describe("doctor config flow", () => {
        };
      };
      expect(cfg.channels?.telegram?.accounts?.work?.allowFrom).toEqual(["12345"]);
-      expect(makeProxyFetch).toHaveBeenCalledWith("http://127.0.0.1:8888");
-      expect(resolveTelegramFetch).toHaveBeenCalledWith(proxyFetch, {
+      expect(makeProxyFetch).not.toHaveBeenCalled();
+      expect(resolveTelegramFetch).toHaveBeenCalledWith(undefined, {
        network: { autoSelectFamily: false, dnsResultOrder: "ipv4first" },
      });
      expect(fetchSpy).toHaveBeenCalledTimes(1);
@@ -787,6 +785,88 @@ describe("doctor config flow", () => {
    }
  });

+  it("sanitizes config-derived doctor warnings and changes before logging", async () => {
+    const noteSpy = vi.spyOn(noteModule, "note").mockImplementation(() => {});
+    const globalFetch = vi.fn(async () => {
+      throw new Error("global fetch should not be called");
+    });
+    const fetchSpy = vi.fn(async () => ({
+      ok: true,
+      json: async () => ({ ok: true, result: { id: 12345 } }),
+    }));
+    vi.stubGlobal("fetch", globalFetch);
+    const telegramFetchModule = await import("../../extensions/telegram/src/fetch.js");
+    const resolveTelegramFetch = vi.spyOn(telegramFetchModule, "resolveTelegramFetch");
+    resolveTelegramFetch.mockReturnValue(fetchSpy as unknown as typeof fetch);
+    try {
+      await runDoctorConfigWithInput({
+        repair: true,
+        config: {
+          channels: {
+            telegram: {
+              accounts: {
+                work: {
+                  botToken: "tok",
+                  allowFrom: ["@\u001b[31mtestuser"],
+                },
+              },
+            },
+            slack: {
+              accounts: {
+                work: {
+                  allowFrom: ["alice\u001b[31m\nforged"],
+                },
+                "ops\u001b[31m\nopen": {
+                  dmPolicy: "open",
+                },
+              },
+            },
+            whatsapp: {
+              accounts: {
+                "ops\u001b[31m\nempty": {
+                  groupPolicy: "allowlist",
+                },
+              },
+            },
+          },
+        },
+        run: loadAndMaybeMigrateDoctorConfig,
+      });
+
+      const outputs = noteSpy.mock.calls
+        .filter((call) => call[1] === "Doctor warnings" || call[1] === "Doctor changes")
+        .map((call) => String(call[0]));
+      expect(outputs.filter((line) => line.includes("\u001b"))).toEqual([]);
+      expect(outputs.filter((line) => line.includes("\nforged"))).toEqual([]);
+      expect(outputs.some((line) => line.includes("resolved @testuser -> 12345"))).toBe(true);
+      expect(
+        outputs.some(
+          (line) =>
+            line.includes("channels.slack.accounts.work.allowFrom: aliceforged") &&
+            line.includes("mutable allowlist"),
+        ),
+      ).toBe(true);
+      expect(
+        outputs.some(
+          (line) =>
+            line.includes('channels.slack.accounts.opsopen.allowFrom: set to ["*"]') &&
+            line.includes('required by dmPolicy="open"'),
+        ),
+      ).toBe(true);
+      expect(
+        outputs.some(
+          (line) =>
+            line.includes('channels.whatsapp.accounts.opsempty.groupPolicy is "allowlist"') &&
+            line.includes("groupAllowFrom"),
+        ),
+      ).toBe(true);
+    } finally {
+      resolveTelegramFetch.mockRestore();
+      noteSpy.mockRestore();
+      vi.unstubAllGlobals();
+    }
+  });
+
  it("warns and continues when Telegram account inspection hits inactive SecretRef surfaces", async () => {
    const noteSpy = vi.spyOn(noteModule, "note").mockImplementation(() => {});
    const fetchSpy = vi.fn();
--- a/src/commands/doctor-config-flow.ts
+++ b/src/commands/doctor-config-flow.ts
@@ -20,6 +20,7 @@ import {
  detectPluginInstallPathIssue,
  formatPluginInstallPathIssue,
 } from "../infra/plugin-install-path-warnings.js";
+import { sanitizeForLog } from "../terminal/ansi.js";
 import { note } from "../terminal/note.js";
 import { noteOpencodeProviderOverrides, stripUnknownConfigKeys } from "./doctor-config-analysis.js";
 import { runDoctorConfigPreflight } from "./doctor-config-preflight.js";
@@ -390,7 +391,10 @@ export async function loadAndMaybeMigrateDoctorConfig(params: {

    const allowFromRepair = maybeRepairOpenPolicyAllowFrom(candidate);
    if (allowFromRepair.changes.length > 0) {
-      note(allowFromRepair.changes.join("\n"), "Doctor changes");
+      note(
+        allowFromRepair.changes.map((line) => sanitizeForLog(line)).join("\n"),
+        "Doctor changes",
+      );
      candidate = allowFromRepair.config;
      pendingChanges = true;
      cfg = allowFromRepair.config;
@@ -406,7 +410,10 @@ export async function loadAndMaybeMigrateDoctorConfig(params: {

    const emptyAllowlistWarnings = detectEmptyAllowlistPolicy(candidate);
    if (emptyAllowlistWarnings.length > 0) {
-      note(emptyAllowlistWarnings.join("\n"), "Doctor warnings");
+      note(
+        emptyAllowlistWarnings.map((line) => sanitizeForLog(line)).join("\n"),
+        "Doctor warnings",
+      );
    }

    const toolsBySenderRepair = maybeRepairLegacyToolsBySenderKeys(candidate);
@@ -430,9 +437,10 @@ 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. ${hits[0]?.entry ?? "@"}); Telegram authorization requires numeric sender IDs.`,
+          `- 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"),
        "Doctor warnings",
@@ -441,9 +449,11 @@ export async function loadAndMaybeMigrateDoctorConfig(params: {

    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. ${discordHits[0]?.path}=${discordHits[0]?.entry}).`,
+          `- 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"),
        "Doctor warnings",
@@ -454,7 +464,7 @@ export async function loadAndMaybeMigrateDoctorConfig(params: {
    if (allowFromScan.changes.length > 0) {
      note(
        [
-          ...allowFromScan.changes,
+          ...allowFromScan.changes.map((line) => sanitizeForLog(line)),
          `- Run "${formatCliCommand("openclaw doctor --fix")}" to add missing allowFrom wildcards.`,
        ].join("\n"),
        "Doctor warnings",
@@ -463,13 +473,18 @@ export async function loadAndMaybeMigrateDoctorConfig(params: {

    const emptyAllowlistWarnings = detectEmptyAllowlistPolicy(candidate);
    if (emptyAllowlistWarnings.length > 0) {
-      note(emptyAllowlistWarnings.join("\n"), "Doctor warnings");
+      note(
+        emptyAllowlistWarnings.map((line) => sanitizeForLog(line)).join("\n"),
+        "Doctor warnings",
+      );
    }

    const toolsBySenderHits = scanLegacyToolsBySenderKeys(candidate);
    if (toolsBySenderHits.length > 0) {
      const sample = toolsBySenderHits[0];
-      const sampleLabel = sample ? `${sample.pathLabel}.${sample.key}` : "toolsBySender";
+      const sampleLabel = sanitizeForLog(
+        sample ? `${sample.pathLabel}.${sample.key}` : "toolsBySender",
+      );
      note(
        [
          `- Found ${toolsBySenderHits.length} legacy untyped toolsBySender key${toolsBySenderHits.length === 1 ? "" : "s"} (for example ${sampleLabel}).`,
@@ -488,7 +503,7 @@ export async function loadAndMaybeMigrateDoctorConfig(params: {
      if (interpreterHits.length > 0) {
        for (const hit of interpreterHits.slice(0, 5)) {
          lines.push(
-            `- ${hit.scopePath}.safeBins includes interpreter/runtime '${hit.bin}' without profile.`,
+            `- ${sanitizeForLog(hit.scopePath)}.safeBins includes interpreter/runtime '${sanitizeForLog(hit.bin)}' without profile.`,
          );
        }
        if (interpreterHits.length > 5) {
@@ -500,7 +515,7 @@ export async function loadAndMaybeMigrateDoctorConfig(params: {
      if (customHits.length > 0) {
        for (const hit of customHits.slice(0, 5)) {
          lines.push(
-            `- ${hit.scopePath}.safeBins entry '${hit.bin}' is missing safeBinProfiles.${hit.bin}.`,
+            `- ${sanitizeForLog(hit.scopePath)}.safeBins entry '${sanitizeForLog(hit.bin)}' is missing safeBinProfiles.${sanitizeForLog(hit.bin)}.`,
          );
        }
        if (customHits.length > 5) {
@@ -521,7 +536,7 @@ export async function loadAndMaybeMigrateDoctorConfig(params: {
        .slice(0, 5)
        .map(
          (hit) =>
-            `- ${hit.scopePath}.safeBins entry '${hit.bin}' resolves to '${hit.resolvedPath}' outside trusted safe-bin dirs.`,
+            `- ${sanitizeForLog(hit.scopePath)}.safeBins entry '${sanitizeForLog(hit.bin)}' resolves to '${sanitizeForLog(hit.resolvedPath)}' outside trusted safe-bin dirs.`,
        );
      if (safeBinTrustedDirHints.length > 5) {
        lines.push(
@@ -540,7 +555,7 @@ export async function loadAndMaybeMigrateDoctorConfig(params: {
    const channels = Array.from(new Set(mutableAllowlistHits.map((hit) => hit.channel))).toSorted();
    const exampleLines = mutableAllowlistHits
      .slice(0, 8)
-      .map((hit) => `- ${hit.path}: ${hit.entry}`)
+      .map((hit) => `- ${sanitizeForLog(hit.path)}: ${sanitizeForLog(hit.entry)}`)
      .join("\n");
    const remaining =
      mutableAllowlistHits.length > 8
@@ -549,8 +564,8 @@ export async function loadAndMaybeMigrateDoctorConfig(params: {
    const flagPaths = Array.from(new Set(mutableAllowlistHits.map((hit) => hit.dangerousFlagPath)));
    const flagHint =
      flagPaths.length === 1
-        ? flagPaths[0]
-        : `${flagPaths[0]} (and ${flagPaths.length - 1} other scope flags)`;
+        ? 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.`,
--- a/src/commands/doctor/providers/telegram.ts
+++ b/src/commands/doctor/providers/telegram.ts
@@ -11,6 +11,7 @@ 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 { sanitizeForLog } from "../../../terminal/ansi.js";
 import { hasAllowFromEntries } from "../shared/allowlist.js";
 import { asObjectRecord } from "../shared/object.js";
 import type { DoctorAccountRecord, DoctorAllowFromList } from "../types.js";
@@ -25,8 +26,6 @@ type TelegramAllowFromListRef = {

 type ResolvedTelegramLookupAccount = {
  token: string;
-  apiRoot?: string;
-  proxyUrl?: string;
  network?: TelegramNetworkConfig;
 };

@@ -163,15 +162,13 @@ export async function maybeRepairTelegramAllowFromUsernames(cfg: OpenClawConfig)
    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 ?? {})}`;
+    const cacheKey = `${token}::${JSON.stringify(network ?? {})}`;
    if (seenLookupAccounts.has(cacheKey)) {
      continue;
    }
    seenLookupAccounts.add(cacheKey);
-    lookupAccounts.push({ token, apiRoot, proxyUrl, network });
+    lookupAccounts.push({ token, network });
  }

  if (lookupAccounts.length === 0) {
@@ -210,8 +207,6 @@ export async function maybeRepairTelegramAllowFromUsernames(cfg: OpenClawConfig)
          token: account.token,
          chatId: username,
          signal: controller.signal,
-          apiRoot: account.apiRoot,
-          proxyUrl: account.proxyUrl,
          network: account.network,
        });
        if (id) {
@@ -270,10 +265,14 @@ export async function maybeRepairTelegramAllowFromUsernames(cfg: OpenClawConfig)
    holder[key] = deduped;
    if (replaced.length > 0) {
      for (const rep of replaced.slice(0, 5)) {
-        changes.push(`- ${pathLabel}: resolved ${rep.from} -> ${rep.to}`);
+        changes.push(
+          `- ${sanitizeForLog(pathLabel)}: resolved ${sanitizeForLog(rep.from)} -> ${sanitizeForLog(rep.to)}`,
+        );
      }
      if (replaced.length > 5) {
-        changes.push(`- ${pathLabel}: resolved ${replaced.length - 5} more @username entries`);
+        changes.push(
+          `- ${sanitizeForLog(pathLabel)}: resolved ${replaced.length - 5} more @username entries`,
+        );
      }
    }
  };
--- a/src/commands/doctor/shared/exec-safe-bins.test.ts
+++ b/src/commands/doctor/shared/exec-safe-bins.test.ts
@@ -1,6 +1,6 @@
 import { chmodSync, mkdtempSync, rmSync, writeFileSync } from "node:fs";
 import { tmpdir } from "node:os";
-import { join } from "node:path";
+import { delimiter, join } from "node:path";
 import { afterEach, describe, expect, it } from "vitest";
 import type { OpenClawConfig } from "../../../config/config.js";
 import {
@@ -52,7 +52,7 @@ describe("doctor exec safe bin helpers", () => {
    const binPath = join(tempDir, "custom-safe-bin");
    writeFileSync(binPath, "#!/bin/sh\nexit 0\n");
    chmodSync(binPath, 0o755);
-    process.env.PATH = `${tempDir}:${originalPath}`;
+    process.env.PATH = [tempDir, originalPath].filter((entry) => entry.length > 0).join(delimiter);

    const hits = scanExecSafeBinTrustedDirHints({
      tools: {