From 0506ed172275d88020538c89d2cecdb1c0b95bd2 Mon Sep 17 00:00:00 2001 From: dailytrap <6246506+dailytrap@users.noreply.github.com> Date: Mon, 31 Aug 2026 06:01:00 +0200 Subject: [PATCH 1/3] fix(doctor): allow legacy exec approvals migration --- src/commands/doctor-security.test.ts | 25 +++++++++++++++++ src/commands/doctor-security.ts | 41 ++++++++++++++++++++++++---- 2 files changed, 60 insertions(+), 6 deletions(-) diff --git a/src/commands/doctor-security.test.ts b/src/commands/doctor-security.test.ts index acad373bb8606..3f114247225f2 100644 --- a/src/commands/doctor-security.test.ts +++ b/src/commands/doctor-security.test.ts @@ -1,4 +1,5 @@ // Doctor security tests cover security audit checks, config findings, and repair output. +import fs from "node:fs/promises"; import path from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { OpenClawConfig } from "../config/config.js"; @@ -88,6 +89,30 @@ describe("noteSecurityWarnings gateway exposure", () => { const lastMessage = () => String(note.mock.calls[note.mock.calls.length - 1]?.[0] ?? ""); + it("does not let pending legacy exec approvals abort Doctor security checks", async () => { + await withTestDir({ prefix: "openclaw-doctor-security-legacy-" }, async (home) => { + const stateDir = path.join(home, ".openclaw"); + process.env.HOME = home; + process.env.OPENCLAW_STATE_DIR = stateDir; + await fs.mkdir(stateDir, { recursive: true }); + await fs.writeFile( + path.join(stateDir, "exec-approvals.json"), + `${JSON.stringify({ version: 1 })}\n`, + "utf8", + ); + closeOpenClawStateDatabaseForTest(); + execApprovalsStoreTesting.reset(); + + const findings = await collectSecurityWarnings({}); + + expect(findings).toEqual( + expect.arrayContaining([ + expect.objectContaining({ checkId: "doctor.exec_approvals_migration_pending" }), + ]), + ); + }); + }); + async function withExecApprovalsFile( file: Record, run: () => Promise, diff --git a/src/commands/doctor-security.ts b/src/commands/doctor-security.ts index 22b6bf3c8d59c..f2fa8971fadc5 100644 --- a/src/commands/doctor-security.ts +++ b/src/commands/doctor-security.ts @@ -11,9 +11,11 @@ import { resolveGatewayAuth } from "../gateway/auth.js"; import { isLoopbackHost, resolveGatewayBindHost } from "../gateway/net.js"; import { resolveExecPolicyScopeSnapshot } from "../infra/exec-approvals-effective.js"; import { countObsoleteGeneratedExecApprovals } from "../infra/exec-approvals-generated-migration.js"; +import { ExecApprovalsMigrationRequiredError } from "../infra/exec-approvals-migration-gate.js"; import { loadExecApprovalsReadOnly, resolveExecApprovalsDisplayPath, + type ExecApprovalsFile, type ExecAsk, type ExecMode, type ExecSecurity, @@ -92,9 +94,11 @@ function execAskRank(value: ExecAsk): number { throw new Error("Unsupported exec ask value"); } -function collectExecPolicyConflictWarnings(cfg: OpenClawConfig): SecurityAuditFinding[] { +function collectExecPolicyConflictWarnings( + cfg: OpenClawConfig, + approvals: ExecApprovalsFile, +): SecurityAuditFinding[] { const findings: SecurityAuditFinding[] = []; - const approvals = loadExecApprovalsReadOnly(); const defaultRequestedSecuritySource = "OpenClaw default (full)"; const defaultRequestedAskSource = "OpenClaw default (off)"; @@ -194,9 +198,12 @@ function collectExecPolicyConflictWarnings(cfg: OpenClawConfig): SecurityAuditFi return findings; } -function collectDurableExecApprovalWarnings(cfg: OpenClawConfig): SecurityAuditFinding[] { +function collectDurableExecApprovalWarnings( + cfg: OpenClawConfig, + approvals: ExecApprovalsFile, +): SecurityAuditFinding[] { void cfg; - const count = countObsoleteGeneratedExecApprovals(loadExecApprovalsReadOnly()); + const count = countObsoleteGeneratedExecApprovals(approvals); if (count === 0) { return []; } @@ -303,10 +310,32 @@ export async function collectSecurityWarnings( } findings.push(...collectImplicitHeartbeatDirectPolicyWarnings(cfg)); - findings.push(...collectExecPolicyConflictWarnings(cfg)); + let approvals: ExecApprovalsFile | undefined; + try { + approvals = loadExecApprovalsReadOnly(); + } catch (error) { + if (!(error instanceof ExecApprovalsMigrationRequiredError)) { + throw error; + } + // Doctor owns this migration later in the same repair flow. Security + // diagnostics must not invoke the runtime gate first and make + // `doctor --fix` abort with an instruction to run itself. + findings.push({ + checkId: "doctor.exec_approvals_migration_pending", + severity: "warn", + title: "Exec approvals migration pending", + detail: error.message, + remediation: "Continue this Doctor repair to migrate the retired approvals store.", + }); + } + if (approvals) { + findings.push(...collectExecPolicyConflictWarnings(cfg, approvals)); + } findings.push(...collectExecFilesystemPolicyWarnings(cfg)); findings.push(...collectPlaintextConfigSecretWarnings(cfg)); - findings.push(...collectDurableExecApprovalWarnings(cfg)); + if (approvals) { + findings.push(...collectDurableExecApprovalWarnings(cfg, approvals)); + } // Network exposure needs auth proof before doctor can treat non-loopback bind as intentional. const tailscaleMode = cfg.gateway?.tailscale?.mode ?? "off"; From b3a32523ec3ddc5a29256b102876aa8d4028049d Mon Sep 17 00:00:00 2001 From: dailytrap <6246506+dailytrap@users.noreply.github.com> Date: Mon, 31 Aug 2026 06:47:16 +0200 Subject: [PATCH 2/3] fix(doctor): order exec approval renewal after import --- .../doctor-config-preflight.process.test.ts | 114 ++++++++++++++++++ src/commands/doctor/repair-sequencing.ts | 8 -- src/flows/doctor-health-contributions.test.ts | 6 + src/flows/doctor-health-contributions.ts | 74 ++++++++---- 4 files changed, 168 insertions(+), 34 deletions(-) diff --git a/src/commands/doctor-config-preflight.process.test.ts b/src/commands/doctor-config-preflight.process.test.ts index 2c64ed9c59853..f62305cd3b655 100644 --- a/src/commands/doctor-config-preflight.process.test.ts +++ b/src/commands/doctor-config-preflight.process.test.ts @@ -283,6 +283,120 @@ describe("doctor invalid config process exit", () => { }, 75_000); }); +describe("doctor legacy exec approvals process repair", () => { + it("imports legacy approvals before renewing obsolete generated grants", async () => { + const root = fs.realpathSync(tempDirs.make("openclaw-doctor-exec-approvals-order-")); + const stateDir = path.join(root, "state"); + const configPath = path.join(stateDir, "openclaw.json"); + const sourcePath = path.join(stateDir, "exec-approvals.json"); + const databasePath = path.join(stateDir, "state", "openclaw.sqlite"); + const env: NodeJS.ProcessEnv = { + ...process.env, + HOME: root, + USERPROFILE: root, + OPENCLAW_CONFIG_PATH: configPath, + OPENCLAW_DISABLE_BUNDLED_PLUGINS: "1", + OPENCLAW_NO_RESPAWN: "1", + OPENCLAW_SKIP_CHANNELS: "1", + OPENCLAW_STATE_DIR: stateDir, + OPENCLAW_TEST_FAST: "1", + NO_COLOR: "1", + }; + delete env.NODE_ENV; + delete env.NODE_OPTIONS; + delete env.OPENCLAW_GATEWAY_PASSWORD; + delete env.OPENCLAW_GATEWAY_TOKEN; + delete env.OPENCLAW_GATEWAY_URL; + delete env.OPENCLAW_HOME; + delete env.VITEST; + delete env.VITEST_POOL_ID; + delete env.VITEST_WORKER_ID; + + fs.mkdirSync(stateDir, { recursive: true }); + fs.writeFileSync( + configPath, + JSON.stringify({ gateway: { mode: "local", auth: { mode: "none" } } }), + ); + fs.writeFileSync( + sourcePath, + JSON.stringify({ + version: 1, + agents: { + main: { + allowlist: [ + { pattern: "/usr/bin/rg", source: "allow-always" }, + { pattern: "=command:durable", source: "allow-always" }, + ], + }, + }, + }), + ); + const preflightUrl = new URL("./doctor-config-preflight.ts", import.meta.url).href; + await runIsolatedModuleScript( + env, + ` + const { runDoctorConfigPreflight } = await import(${JSON.stringify(preflightUrl)}); + await runDoctorConfigPreflight({ + migrateLegacyConfig: false, + invalidConfigNote: false, + observe: false, + requireStartupMigrationCheckpoint: true, + beforeStateMigrations: async () => true, + }); + `, + ); + expect(fs.existsSync(sourcePath)).toBe(true); + + const result = spawnSync( + process.execPath, + [ + "--import", + "tsx", + path.resolve("src/entry.ts"), + "doctor", + "--fix", + "--non-interactive", + "--no-workspace-suggestions", + ], + { + cwd: path.resolve("."), + encoding: "utf8", + env, + timeout: 60_000, + }, + ); + const output = `${result.stderr}\n${result.stdout}`; + + expect(result.error, output).toBeUndefined(); + expect(result.status, output).toBe(0); + expect(result.signal, output).toBeNull(); + expect(output).toContain("Doctor complete."); + expect(fs.existsSync(sourcePath)).toBe(false); + + const database = new DatabaseSync(databasePath, { readOnly: true }); + try { + const row = database + .prepare("SELECT raw_json FROM exec_approvals_config WHERE config_key = 'current'") + .get() as { raw_json: string } | undefined; + const approvals = JSON.parse(row?.raw_json ?? "null") as { + agents?: { main?: { allowlist?: Array<{ pattern?: string }> } }; + } | null; + expect(approvals?.agents?.main?.allowlist).toEqual([ + expect.objectContaining({ pattern: "=command:durable" }), + ]); + expect( + database + .prepare( + "SELECT status, removed_source FROM migration_sources WHERE migration_kind = 'legacy-exec-approvals-json'", + ) + .get(), + ).toMatchObject({ status: "completed", removed_source: 1 }); + } finally { + database.close(); + } + }, 75_000); +}); + // Synchronous CLI probes must not consume neighboring cases' timeout budgets. describe("gateway startup-migration refusal", () => { it("repairs the stable upgrade config and additive state schema before readiness", async () => { diff --git a/src/commands/doctor/repair-sequencing.ts b/src/commands/doctor/repair-sequencing.ts index 612e869834a66..095f4752d9db6 100644 --- a/src/commands/doctor/repair-sequencing.ts +++ b/src/commands/doctor/repair-sequencing.ts @@ -5,7 +5,6 @@ import { applyPluginAutoEnable, materializePluginAutoEnableCandidates, } from "../../config/plugin-auto-enable.js"; -import { repairObsoleteGeneratedExecApprovals } from "../../infra/exec-approvals-generated-migration.js"; import type { PluginCapabilityConsentHandler } from "../../plugins/capability-consent.js"; import type { PluginMetadataSnapshotScopeRunner } from "../../plugins/current-plugin-metadata-snapshot.js"; import { @@ -112,13 +111,6 @@ export async function runDoctorRepairSequence(params: { return params.runWithPluginMetadataSnapshot(resolveCurrentPluginMetadataScope(), run); }; - const removedExecApprovals = repairObsoleteGeneratedExecApprovals(); - if (removedExecApprovals > 0) { - changeNotes.push( - `Exec approvals updated: removed ${removedExecApprovals} older generated ${removedExecApprovals === 1 ? "approval" : "approvals"} that were not tied to a working directory. Manual allowlist rules were not changed. Rerun affected workflows and choose "Always allow here" when prompted.`, - ); - } - const applyMutation = (mutation: { config: DoctorConfigMutationState["candidate"]; changes: string[]; diff --git a/src/flows/doctor-health-contributions.test.ts b/src/flows/doctor-health-contributions.test.ts index 6fe661784de05..4aecabb64b1d9 100644 --- a/src/flows/doctor-health-contributions.test.ts +++ b/src/flows/doctor-health-contributions.test.ts @@ -107,6 +107,7 @@ const mocks = vi.hoisted(() => ({ noteChromeMcpBrowserReadiness: vi.fn(), detectLegacyStateMigrations: vi.fn(), runLegacyStateMigrations: vi.fn(), + repairObsoleteGeneratedExecApprovals: vi.fn(() => 0), collectLegacyPluginManifestContractMigrations: vi.fn(() => [] as unknown[]), legacyPluginManifestContractMigrationToHealthFinding: vi.fn( (migration: { pluginId: string }) => ({ @@ -339,6 +340,10 @@ vi.mock("../infra/state-migrations.doctor.js", () => ({ runLegacyStateMigrations: mocks.runLegacyStateMigrations, })); +vi.mock("../infra/exec-approvals-generated-migration.js", () => ({ + repairObsoleteGeneratedExecApprovals: mocks.repairObsoleteGeneratedExecApprovals, +})); + vi.mock("../commands/doctor-plugin-manifests.js", () => ({ collectLegacyPluginManifestContractMigrations: mocks.collectLegacyPluginManifestContractMigrations, @@ -757,6 +762,7 @@ describe("doctor health contributions", () => { .mockReset() .mockResolvedValue({ preview: [], warnings: [], notices: [] }); mocks.runLegacyStateMigrations.mockReset().mockResolvedValue({ changes: [], warnings: [] }); + mocks.repairObsoleteGeneratedExecApprovals.mockReset().mockReturnValue(0); mocks.detectLegacyClawdBrowserProfileResidue.mockReset().mockReturnValue(null); mocks.maybeArchiveLegacyClawdBrowserProfileResidue.mockReset().mockResolvedValue({ changes: [], diff --git a/src/flows/doctor-health-contributions.ts b/src/flows/doctor-health-contributions.ts index cc821c4221167..678b0d5bf7351 100644 --- a/src/flows/doctor-health-contributions.ts +++ b/src/flows/doctor-health-contributions.ts @@ -284,36 +284,58 @@ async function runLegacyStateHealth(ctx: DoctorHealthFlowContext): Promise if (legacyState.notices.length > 0) { note(legacyState.notices.join("\n"), "Doctor notices"); } - if (legacyState.preview.length === 0) { - return; + if (legacyState.preview.length > 0) { + note(legacyState.preview.join("\n"), "Legacy state detected"); + const migrate = + ctx.options.nonInteractive === true + ? true + : await ctx.prompter.confirm({ + message: "Migrate detected legacy state now?", + initialValue: true, + }); + if (!migrate) { + return; + } + const migrated = await runLegacyStateMigrations({ + detected: legacyState, + config: ctx.cfg, + ...(doctorOnlyStateMigrations ? { doctorOnlyStateMigrations: true } : {}), + recoverCorruptTargetStore: ctx.options.repair === true || ctx.options.yes === true, + legacySessionSurfaces, + }); + if (migrated.changes.length > 0) { + note(migrated.changes.join("\n"), "Doctor changes"); + } + const notices = migrated.notices ?? []; + if (notices.length > 0) { + note(notices.join("\n"), "Doctor notices"); + } + if (migrated.warnings.length > 0) { + note(migrated.warnings.join("\n"), "Doctor warnings"); + } } - note(legacyState.preview.join("\n"), "Legacy state detected"); - const migrate = - ctx.options.nonInteractive === true - ? true - : await ctx.prompter.confirm({ - message: "Migrate detected legacy state now?", - initialValue: true, - }); - if (!migrate) { + if (!doctorOnlyStateMigrations) { return; } - const migrated = await runLegacyStateMigrations({ - detected: legacyState, - config: ctx.cfg, - ...(doctorOnlyStateMigrations ? { doctorOnlyStateMigrations: true } : {}), - recoverCorruptTargetStore: ctx.options.repair === true || ctx.options.yes === true, - legacySessionSurfaces, - }); - if (migrated.changes.length > 0) { - note(migrated.changes.join("\n"), "Doctor changes"); - } - const notices = migrated.notices ?? []; - if (notices.length > 0) { - note(notices.join("\n"), "Doctor notices"); + const { repairObsoleteGeneratedExecApprovals } = + await import("../infra/exec-approvals-generated-migration.js"); + const { ExecApprovalsMigrationRequiredError } = + await import("../infra/exec-approvals-migration-gate.js"); + let removedExecApprovals: number; + try { + // The legacy-state owner must import retired JSON before this gated SQLite update. + removedExecApprovals = repairObsoleteGeneratedExecApprovals(); + } catch (error) { + if (error instanceof ExecApprovalsMigrationRequiredError) { + return; + } + throw error; } - if (migrated.warnings.length > 0) { - note(migrated.warnings.join("\n"), "Doctor warnings"); + if (removedExecApprovals > 0) { + note( + `Exec approvals updated: removed ${removedExecApprovals} older generated ${removedExecApprovals === 1 ? "approval" : "approvals"} that were not tied to a working directory. Manual allowlist rules were not changed. Rerun affected workflows and choose "Always allow here" when prompted.`, + "Doctor changes", + ); } } From a100cd20c0c0054e41b50b9176e6037035f097a9 Mon Sep 17 00:00:00 2001 From: PollyBot13 Date: Mon, 31 Aug 2026 08:48:55 +0200 Subject: [PATCH 3/3] fix(doctor): preserve exec approval renewal after decline --- src/flows/doctor-health-contributions.test.ts | 20 ++++++++++ src/flows/doctor-health-contributions.ts | 37 +++++++++---------- 2 files changed, 38 insertions(+), 19 deletions(-) diff --git a/src/flows/doctor-health-contributions.test.ts b/src/flows/doctor-health-contributions.test.ts index 4aecabb64b1d9..bb9ae7f76adbf 100644 --- a/src/flows/doctor-health-contributions.test.ts +++ b/src/flows/doctor-health-contributions.test.ts @@ -2203,6 +2203,26 @@ describe("doctor health contributions", () => { }); }); + it("still renews generated exec approvals after declining an unrelated migration", async () => { + const contribution = requireDoctorContribution("doctor:legacy-state"); + mocks.detectLegacyStateMigrations.mockResolvedValue({ + preview: ["legacy sessions"], + warnings: [], + notices: [], + }); + const ctx = createDoctorContext({ + cfg: {}, + configResult: {}, + shouldRepair: false, + options: { repair: true }, + }); + + await contribution.run(ctx); + + expect(mocks.runLegacyStateMigrations).not.toHaveBeenCalled(); + expect(mocks.repairObsoleteGeneratedExecApprovals).toHaveBeenCalledOnce(); + }); + it("prints legacy state migration notices during manual doctor runs", async () => { const contribution = requireDoctorContribution("doctor:legacy-state"); const detected = { preview: ["legacy sessions"], warnings: [], notices: [] }; diff --git a/src/flows/doctor-health-contributions.ts b/src/flows/doctor-health-contributions.ts index 678b0d5bf7351..74267c6f596fb 100644 --- a/src/flows/doctor-health-contributions.ts +++ b/src/flows/doctor-health-contributions.ts @@ -293,25 +293,24 @@ async function runLegacyStateHealth(ctx: DoctorHealthFlowContext): Promise message: "Migrate detected legacy state now?", initialValue: true, }); - if (!migrate) { - return; - } - const migrated = await runLegacyStateMigrations({ - detected: legacyState, - config: ctx.cfg, - ...(doctorOnlyStateMigrations ? { doctorOnlyStateMigrations: true } : {}), - recoverCorruptTargetStore: ctx.options.repair === true || ctx.options.yes === true, - legacySessionSurfaces, - }); - if (migrated.changes.length > 0) { - note(migrated.changes.join("\n"), "Doctor changes"); - } - const notices = migrated.notices ?? []; - if (notices.length > 0) { - note(notices.join("\n"), "Doctor notices"); - } - if (migrated.warnings.length > 0) { - note(migrated.warnings.join("\n"), "Doctor warnings"); + if (migrate) { + const migrated = await runLegacyStateMigrations({ + detected: legacyState, + config: ctx.cfg, + ...(doctorOnlyStateMigrations ? { doctorOnlyStateMigrations: true } : {}), + recoverCorruptTargetStore: ctx.options.repair === true || ctx.options.yes === true, + legacySessionSurfaces, + }); + if (migrated.changes.length > 0) { + note(migrated.changes.join("\n"), "Doctor changes"); + } + const notices = migrated.notices ?? []; + if (notices.length > 0) { + note(notices.join("\n"), "Doctor notices"); + } + if (migrated.warnings.length > 0) { + note(migrated.warnings.join("\n"), "Doctor warnings"); + } } } if (!doctorOnlyStateMigrations) {