From aa27ae9d9f37ee1127f89952e1dfeb9fb8577b99 Mon Sep 17 00:00:00 2001 From: Gio Della-Libera Date: Wed, 8 Jul 2026 10:44:56 -0700 Subject: [PATCH] policy: preview review-required gateway repairs (#99776) * policy: preview review-required gateway repairs * docs(policy): document gateway review previews * docs(policy): document gateway review previews * docs(policy): document gateway review previews * docs(policy): document gateway review previews --------- Co-authored-by: Gio Della-Libera --- docs/cli/policy.md | 7 + extensions/policy/src/doctor/register.test.ts | 123 +++++++++++++++- .../src/doctor/review-required-repairs.ts | 138 ++++++++++++++++++ .../policy/src/doctor/scopes/gateway.ts | 15 ++ src/flows/doctor-repair-flow.test.ts | 3 +- src/flows/doctor-repair-flow.ts | 2 +- 6 files changed, 285 insertions(+), 3 deletions(-) create mode 100644 extensions/policy/src/doctor/review-required-repairs.ts diff --git a/docs/cli/policy.md b/docs/cli/policy.md index 6ae9c00079a7..b9c538ebbca1 100644 --- a/docs/cli/policy.md +++ b/docs/cli/policy.md @@ -975,6 +975,13 @@ more than the scoped policy target. Gateway HTTP URL-fetch allowlist findings remain manual because automatic repair cannot choose the correct endpoint URL allowlist values. +Gateway bind and node-command findings stay review-required. When +`policy/gateway-non-loopback-bind` or `policy/gateway-node-command-denied` +can be mapped to a config path, `doctor --fix` reports the proposed +`gateway.bind` or `gateway.nodes.denyCommands` change as skipped preview +guidance. It does not apply the change, and the finding does not count as +repaired until an operator reviews and updates config or policy. + ```jsonc { "plugins": { diff --git a/extensions/policy/src/doctor/register.test.ts b/extensions/policy/src/doctor/register.test.ts index 0d24b82cac5c..5be6e0e808f5 100644 --- a/extensions/policy/src/doctor/register.test.ts +++ b/extensions/policy/src/doctor/register.test.ts @@ -1797,6 +1797,127 @@ describe("registerPolicyDoctorChecks", () => { }); }); + it("previews review-required gateway bind repair without mutating config", async () => { + const configPath = join(workspaceDir, "openclaw.jsonc"); + const cfg = { + ...cfgWithPolicy({ workspaceRepairs: true }), + gateway: { bind: "lan" }, + } as unknown as OpenClawConfig; + await fs.writeFile(configPath, "{}", "utf-8"); + await fs.writeFile( + join(workspaceDir, "policy.jsonc"), + JSON.stringify({ gateway: { exposure: { allowNonLoopbackBind: false } } }), + "utf-8", + ); + + const result = await runPolicyRepairCheck( + "policy/gateway-non-loopback-bind", + repairCtx(configPath, cfg), + ); + + expect(result.status).toBe("skipped"); + expect(result.reason).toBe("policy repair requires review before changing config"); + expect(result.changes).toEqual([ + "Review required: set gateway.bind=loopback for policy conformance.", + ]); + expect(result.warnings).toEqual([ + "Review required: set gateway.bind=loopback for policy conformance.", + ]); + expect(result.effects).toEqual([ + { + kind: "config", + action: "would-set-after-review", + target: "gateway.bind=loopback", + dryRunSafe: true, + }, + ]); + expect(result.config.gateway?.bind).toBe("lan"); + expect(result.remainingFindings).toHaveLength(1); + }); + + it("previews review-required custom gateway bind repair without mutating config", async () => { + const configPath = join(workspaceDir, "openclaw.jsonc"); + const cfg = { + ...cfgWithPolicy({ workspaceRepairs: true }), + gateway: { bind: "custom", customBindHost: "10.0.0.4" }, + } as unknown as OpenClawConfig; + await fs.writeFile(configPath, "{}", "utf-8"); + await fs.writeFile( + join(workspaceDir, "policy.jsonc"), + JSON.stringify({ gateway: { exposure: { allowNonLoopbackBind: false } } }), + "utf-8", + ); + + const result = await runPolicyRepairCheck( + "policy/gateway-non-loopback-bind", + repairCtx(configPath, cfg), + ); + + expect(result.findings).toEqual([ + expect.objectContaining({ + ocPath: "oc://openclaw.config/gateway/customBindHost", + }), + ]); + expect(result.status).toBe("skipped"); + expect(result.changes).toEqual([ + "Review required: set gateway.bind=loopback for policy conformance.", + ]); + expect(result.warnings).toEqual([ + "Review required: set gateway.bind=loopback for policy conformance.", + ]); + expect(result.effects).toEqual([ + { + kind: "config", + action: "would-set-after-review", + target: "gateway.bind=loopback", + dryRunSafe: true, + }, + ]); + expect(result.config.gateway).toMatchObject({ + bind: "custom", + customBindHost: "10.0.0.4", + }); + expect(result.remainingFindings).toHaveLength(1); + }); + + it("previews review-required gateway node command repairs without mutating config", async () => { + const configPath = join(workspaceDir, "openclaw.jsonc"); + const cfg = { + ...cfgWithPolicy({ workspaceRepairs: true }), + gateway: { nodes: { denyCommands: ["mcp.help"] } }, + } as unknown as OpenClawConfig; + await fs.writeFile(configPath, "{}", "utf-8"); + await fs.writeFile( + join(workspaceDir, "policy.jsonc"), + JSON.stringify({ gateway: { nodes: { denyCommands: ["mcp.help", "system.run"] } } }), + "utf-8", + ); + + const result = await runPolicyRepairCheck( + "policy/gateway-node-command-denied", + repairCtx(configPath, cfg), + ); + + expect(result.status).toBe("skipped"); + expect(result.reason).toBe("policy repair requires review before changing config"); + expect(result.changes).toEqual([ + "Review required: add system.run to gateway.nodes.denyCommands for policy conformance.", + ]); + expect(result.warnings).toEqual([ + "Review required: add system.run to gateway.nodes.denyCommands for policy conformance.", + ]); + expect(result.effects).toEqual([ + { + kind: "config", + action: "would-append-after-review", + target: "gateway.nodes.denyCommands += system.run", + dryRunSafe: true, + }, + ]); + expect(result.config.gateway?.nodes?.denyCommands).toEqual(["mcp.help"]); + expect(result.remainingFindings).toHaveLength(1); + }); + it("repairs automatic channel ingress narrowing findings", async () => { const configPath = join(workspaceDir, "openclaw.jsonc"); const cfg = { @@ -2161,7 +2282,7 @@ describe("registerPolicyDoctorChecks", () => { ]); }); - it("does not register repair for review-required policy findings", () => { + it("does not register repair for non-previewable policy findings", () => { const check = registerChecks().find( (entry) => entry.id === "policy/gateway-http-url-fetch-unrestricted", ); diff --git a/extensions/policy/src/doctor/review-required-repairs.ts b/extensions/policy/src/doctor/review-required-repairs.ts new file mode 100644 index 000000000000..2832f83b703a --- /dev/null +++ b/extensions/policy/src/doctor/review-required-repairs.ts @@ -0,0 +1,138 @@ +// Policy review-required repairs surface proposed config changes without applying them. +import type { + HealthFinding, + HealthRepairContext, + HealthRepairEffect, + HealthRepairResult, +} from "openclaw/plugin-sdk/health"; +import { POLICY_FIX_METADATA_BY_CHECK_ID } from "./fix-metadata.js"; +import { CHECK_IDS, type POLICY_CHECK_IDS } from "./metadata.js"; + +type PolicyCheckId = (typeof POLICY_CHECK_IDS)[number]; + +const REVIEW_REQUIRED_REPAIR_CHECK_IDS = new Set([ + CHECK_IDS.policyGatewayNonLoopbackBind, + CHECK_IDS.policyGatewayNodeCommandDenied, +]); + +export function previewPolicyReviewRequiredRepair( + _ctx: HealthRepairContext, + findings: readonly HealthFinding[], + checkId: PolicyCheckId, +): Promise { + const metadata = POLICY_FIX_METADATA_BY_CHECK_ID.get(checkId); + if (!REVIEW_REQUIRED_REPAIR_CHECK_IDS.has(checkId) || metadata?.fixClass !== "reviewRequired") { + return Promise.resolve({ + status: "skipped", + reason: "policy finding does not have a review-required repair preview", + changes: [], + }); + } + if ( + findings.length === 0 || + findings.some( + (finding) => + finding.checkId !== checkId || + POLICY_FIX_METADATA_BY_CHECK_ID.get(finding.checkId)?.fixClass !== "reviewRequired", + ) + ) { + return Promise.resolve({ + status: "skipped", + reason: "policy finding is not classified as review-required", + changes: [], + }); + } + + const previews = findings.flatMap((finding) => previewForFinding(finding, checkId)); + if (previews.length === 0) { + return Promise.resolve({ + status: "skipped", + reason: "policy review-required repair had no previewable config changes", + changes: [], + }); + } + + return Promise.resolve({ + status: "skipped", + reason: "policy repair requires review before changing config", + changes: uniqueStrings(previews.map((preview) => preview.change)), + warnings: uniqueStrings(previews.map((preview) => preview.change)), + effects: uniqueEffects(previews.map((preview) => preview.effect)), + }); +} + +function previewForFinding( + finding: HealthFinding, + checkId: PolicyCheckId, +): readonly { readonly change: string; readonly effect: HealthRepairEffect }[] { + switch (checkId) { + case CHECK_IDS.policyGatewayNonLoopbackBind: + return previewGatewayLoopbackBind(finding); + case CHECK_IDS.policyGatewayNodeCommandDenied: + return previewGatewayNodeDenyCommand(finding); + default: + return []; + } +} + +function previewGatewayLoopbackBind( + finding: HealthFinding, +): readonly { readonly change: string; readonly effect: HealthRepairEffect }[] { + if ( + finding.ocPath !== "oc://openclaw.config/gateway/bind" && + finding.ocPath !== "oc://openclaw.config/gateway/customBindHost" + ) { + return []; + } + return [ + { + change: "Review required: set gateway.bind=loopback for policy conformance.", + effect: { + kind: "config", + action: "would-set-after-review", + target: "gateway.bind=loopback", + dryRunSafe: true, + }, + }, + ]; +} + +function previewGatewayNodeDenyCommand( + finding: HealthFinding, +): readonly { readonly change: string; readonly effect: HealthRepairEffect }[] { + const command = finding.message.match(/Gateway node command '([^']+)'/)?.[1]?.trim(); + if ( + command === undefined || + command === "" || + finding.ocPath !== "oc://openclaw.config/gateway/nodes/denyCommands" + ) { + return []; + } + return [ + { + change: `Review required: add ${command} to gateway.nodes.denyCommands for policy conformance.`, + effect: { + kind: "config", + action: "would-append-after-review", + target: `gateway.nodes.denyCommands += ${command}`, + dryRunSafe: true, + }, + }, + ]; +} + +function uniqueStrings(values: readonly string[]): readonly string[] { + return [...new Set(values)]; +} + +function uniqueEffects(values: readonly HealthRepairEffect[]): readonly HealthRepairEffect[] { + const seen = new Set(); + return values.filter((value) => { + const key = JSON.stringify(value); + if (seen.has(key)) { + return false; + } + seen.add(key); + return true; + }); +} diff --git a/extensions/policy/src/doctor/scopes/gateway.ts b/extensions/policy/src/doctor/scopes/gateway.ts index a5aed8df7955..57f00f1fa43e 100644 --- a/extensions/policy/src/doctor/scopes/gateway.ts +++ b/extensions/policy/src/doctor/scopes/gateway.ts @@ -3,6 +3,7 @@ import type { HealthCheck, HealthFinding } from "openclaw/plugin-sdk/health"; import type { PolicyEvidence } from "../../policy-state.js"; import { repairPolicyAutomaticNarrower } from "../automatic-repairs.js"; import { CHECK_IDS } from "../metadata.js"; +import { previewPolicyReviewRequiredRepair } from "../review-required-repairs.js"; import type { PolicyDoctorCheckDeps } from "../types.js"; import { readPolicyBoolean, readStringList } from "../utils.js"; @@ -17,6 +18,13 @@ export function createPolicyGatewayChecks(deps: PolicyDoctorCheckDeps): readonly async detect(ctx) { return findingsForCheck(await evaluatePolicy(ctx), CHECK_IDS.policyGatewayNonLoopbackBind); }, + repair(ctx, findings) { + return previewPolicyReviewRequiredRepair( + ctx, + findings, + CHECK_IDS.policyGatewayNonLoopbackBind, + ); + }, }; const policyGatewayAuthDisabledCheck: HealthCheck = { id: CHECK_IDS.policyGatewayAuthDisabled, @@ -108,6 +116,13 @@ export function createPolicyGatewayChecks(deps: PolicyDoctorCheckDeps): readonly async detect(ctx) { return findingsForCheck(await evaluatePolicy(ctx), CHECK_IDS.policyGatewayNodeCommandDenied); }, + repair(ctx, findings) { + return previewPolicyReviewRequiredRepair( + ctx, + findings, + CHECK_IDS.policyGatewayNodeCommandDenied, + ); + }, }; return [ diff --git a/src/flows/doctor-repair-flow.test.ts b/src/flows/doctor-repair-flow.test.ts index e118a40158d9..2de0cc2d73af 100644 --- a/src/flows/doctor-repair-flow.test.ts +++ b/src/flows/doctor-repair-flow.test.ts @@ -292,7 +292,7 @@ describe("runDoctorHealthRepairs", () => { return { status: "skipped", reason: "manual confirmation required", - changes: [], + changes: ["Review required before changing gateway.mode."], }; }, }), @@ -304,6 +304,7 @@ describe("runDoctorHealthRepairs", () => { expect(result.checksRepaired).toBe(0); expect(result.checksValidated).toBe(0); expect(result.remainingFindings).toEqual([]); + expect(result.changes).toEqual(["Review required before changing gateway.mode."]); expect(result.warnings).toEqual(["test/skipped repair skipped: manual confirmation required"]); }); diff --git a/src/flows/doctor-repair-flow.ts b/src/flows/doctor-repair-flow.ts index 3cbeb257ecec..b09b473da269 100644 --- a/src/flows/doctor-repair-flow.ts +++ b/src/flows/doctor-repair-flow.ts @@ -130,6 +130,7 @@ async function runSplitHealthCheck( warnings.push(...(result.warnings ?? [])); diffs.push(...(result.diffs ?? [])); effects.push(...(result.effects ?? [])); + changes.push(...result.changes); const status = result.status ?? "repaired"; if (status !== "repaired") { warnings.push(`${check.id} repair ${status}${result.reason ? `: ${result.reason}` : ""}`); @@ -138,7 +139,6 @@ async function runSplitHealthCheck( if (result.config !== undefined && opts.dryRun !== true) { cfg = result.config; } - changes.push(...result.changes); checksRepaired++; if (opts.dryRun === true) { return repairRunResult(cfg, findings, remainingFindings, changes, warnings, diffs, effects, {