policy: repair required deny tool findings

giodl73-repo · giodl73-repo · commit c7ab05e372db · 2026-07-06T08:59:51.000-07:00
diff --git a/docs/cli/policy.md b/docs/cli/policy.md
@@ -943,6 +943,8 @@ only after the policy file has been reviewed, because a valid rule can change
 workspace config:
 
 - set `tools.elevated.enabled=false` when a global policy forbids elevated tools
+- add missing required-deny tool ids to `tools.deny` or
+  `agents.list[].tools.deny` when policy requires those tools to be denied
 - set insecure `gateway.controlUi.*` toggles to `false`
 - set `gateway.mode=local` when policy denies remote gateway mode
 - set `logging.redactSensitive=tools` when policy requires sensitive logging
@@ -956,6 +958,11 @@ also skipped when the finding reports shared logging or telemetry config,
 because changing the shared setting would affect more than the scoped policy
 target.
 
+Scoped required-deny repairs are skipped when the finding reports inherited
+root `tools.deny`, because adding the required tool to root config would affect
+more than the scoped policy target. Agent-local required-deny repairs can update
+the reported `agents.list[].tools.deny` path.
+
 ```jsonc
 {
   "plugins": {
diff --git a/extensions/policy/src/doctor/automatic-repairs.ts b/extensions/policy/src/doctor/automatic-repairs.ts
@@ -17,7 +17,9 @@ type RepairPatch = {
 };
 
 const AUTOMATIC_REPAIR_CHECK_IDS = new Set<PolicyCheckId>([
+  CHECK_IDS.policyAgentsToolNotDenied,
   CHECK_IDS.policyToolsElevatedEnabled,
+  CHECK_IDS.policyToolsRequiredDenyMissing,
   CHECK_IDS.policyGatewayControlUiInsecure,
   CHECK_IDS.policyGatewayRemoteEnabled,
   CHECK_IDS.policyDataHandlingRedactionDisabled,
@@ -77,6 +79,8 @@ function applyAutomaticPatch(
   checkId: PolicyCheckId,
 ): RepairPatch {
   switch (checkId) {
+    case CHECK_IDS.policyAgentsToolNotDenied:
+      return mergeRequiredDenyTools(cfg, findings);
     case CHECK_IDS.policyToolsElevatedEnabled:
       if (hasScopedPolicyRequirement(findings)) {
         return skippedUnsafeScopedRepair(
@@ -85,6 +89,8 @@ function applyAutomaticPatch(
         );
       }
       return disableElevatedTools(cfg, findings);
+    case CHECK_IDS.policyToolsRequiredDenyMissing:
+      return mergeRequiredDenyTools(cfg, findings);
     case CHECK_IDS.policyGatewayControlUiInsecure:
       return disableInsecureControlUi(cfg, findings);
     case CHECK_IDS.policyGatewayRemoteEnabled:
@@ -110,6 +116,36 @@ function applyAutomaticPatch(
   }
 }
 
+function mergeRequiredDenyTools(
+  cfg: OpenClawConfig,
+  findings: readonly HealthFinding[],
+): RepairPatch {
+  const next = cloneConfig(cfg);
+  const changes: string[] = [];
+  const warnings: string[] = [];
+  for (const finding of findings) {
+    const tool = missingRequiredTool(finding);
+    if (tool === undefined || finding.ocPath === undefined) {
+      continue;
+    }
+    if (
+      hasScopedPolicyRequirement([finding]) &&
+      finding.ocPath === "oc://openclaw.config/tools/deny"
+    ) {
+      warnings.push(
+        `Skipped scoped deny repair for ${tool}. The finding reports inherited root tools.deny, so changing it would affect more than the scoped policy target.`,
+      );
+      continue;
+    }
+    if (mergeStringArrayAtOcPath(next, finding.ocPath, tool)) {
+      changes.push(`Added ${tool} to ${configPathLabel(finding.ocPath)} for policy conformance.`);
+    }
+  }
+  return changes.length > 0
+    ? { config: next as OpenClawConfig, changes: uniqueStrings(changes), warnings }
+    : { config: cfg, changes, warnings: uniqueStrings(warnings) };
+}
+
 function disableElevatedTools(
   cfg: OpenClawConfig,
   findings: readonly HealthFinding[],
@@ -213,6 +249,74 @@ function cloneConfig(cfg: OpenClawConfig): ConfigRecord {
   return structuredClone(cfg) as ConfigRecord;
 }
 
+function mergeStringArrayAtOcPath(cfg: ConfigRecord, ocPath: string, entry: string): boolean {
+  const segments = configPathSegments(ocPath);
+  if (segments.length === 0 || segments.at(-1) !== "deny") {
+    return false;
+  }
+  let current: unknown = cfg;
+  for (let index = 0; index < segments.length - 1; index += 1) {
+    const segment = segments[index];
+    if (segment === undefined) {
+      return false;
+    }
+    if (segment.startsWith("#")) {
+      const arrayIndex = Number.parseInt(segment.slice(1), 10);
+      if (!Array.isArray(current) || !Number.isInteger(arrayIndex) || arrayIndex < 0) {
+        return false;
+      }
+      current = current[arrayIndex];
+      continue;
+    }
+    if (!isRecord(current)) {
+      return false;
+    }
+    const nextSegment = segments[index + 1];
+    const existing = current[segment];
+    if (existing === undefined) {
+      current[segment] = nextSegment?.startsWith("#") ? [] : {};
+    }
+    current = current[segment];
+  }
+  if (!isRecord(current)) {
+    return false;
+  }
+  const existing = current.deny;
+  if (existing !== undefined && !Array.isArray(existing)) {
+    return false;
+  }
+  const deny = existing ?? [];
+  if (deny.some((value) => typeof value === "string" && value === entry)) {
+    return false;
+  }
+  current.deny = [...deny, entry];
+  return true;
+}
+
+function configPathSegments(ocPath: string): readonly string[] {
+  const prefix = "oc://openclaw.config/";
+  if (!ocPath.startsWith(prefix)) {
+    return [];
+  }
+  return ocPath.slice(prefix.length).split("/").filter(Boolean);
+}
+
+function configPathLabel(ocPath: string): string {
+  let label = "";
+  for (const segment of configPathSegments(ocPath)) {
+    if (segment.startsWith("#")) {
+      label += `[${segment.slice(1)}]`;
+    } else {
+      label += label === "" ? segment : `.${segment}`;
+    }
+  }
+  return label;
+}
+
+function missingRequiredTool(finding: HealthFinding): string | undefined {
+  return finding.message.match(/required tool '([^']+)'/)?.[1]?.trim();
+}
+
 function workspaceRepairsEnabled(ctx: HealthRepairContext): boolean {
   const plugins = isRecord(ctx.cfg.plugins) ? ctx.cfg.plugins : {};
   const entries = isRecord(plugins.entries) ? plugins.entries : {};
@@ -255,3 +359,7 @@ function ensureRecord(parent: ConfigRecord, key: string): ConfigRecord {
 function isRecord(value: unknown): value is ConfigRecord {
   return typeof value === "object" && value !== null && !Array.isArray(value);
 }
+
+function uniqueStrings(values: readonly string[]): readonly string[] {
+  return [...new Set(values)];
+}
diff --git a/extensions/policy/src/doctor/fix-metadata.ts b/extensions/policy/src/doctor/fix-metadata.ts
@@ -229,7 +229,10 @@ export const POLICY_FIX_METADATA = [
     CHECK_IDS.policyToolsRequiredDenyMissing,
     "automatic",
     "Merge required built-in deny tool classes.",
-    { policyPath: ["tools", "denyTools"], configTargets: ["tools.denyTools"] },
+    {
+      policyPath: ["tools", "denyTools"],
+      configTargets: ["tools.deny", "agents.list[].tools.deny"],
+    },
   ),
   m(
     CHECK_IDS.policySandboxModeUnapproved,
diff --git a/extensions/policy/src/doctor/metadata.test.ts b/extensions/policy/src/doctor/metadata.test.ts
@@ -5,7 +5,12 @@ import {
   POLICY_FIX_METADATA_BY_CHECK_ID,
   type PolicyFixMetadata,
 } from "./fix-metadata.js";
-import { POLICY_CHECK_IDS, POLICY_RULE_METADATA, type PolicyRuleMetadata } from "./metadata.js";
+import {
+  CHECK_IDS,
+  POLICY_CHECK_IDS,
+  POLICY_RULE_METADATA,
+  type PolicyRuleMetadata,
+} from "./metadata.js";
 
 describe("policy doctor metadata", () => {
   it("describes strictness for agent-scoped policy fields", () => {
@@ -173,6 +178,12 @@ describe("policy doctor metadata", () => {
     );
   });
 
+  it("points required-deny repair metadata at OpenClaw deny config paths", () => {
+    expect(
+      POLICY_FIX_METADATA_BY_CHECK_ID.get(CHECK_IDS.policyToolsRequiredDenyMissing)?.configTargets,
+    ).toEqual(["tools.deny", "agents.list[].tools.deny"]);
+  });
+
   it("keeps policy fix class assignments explicit", () => {
     const grouped = new Map<PolicyFixMetadata["fixClass"], PolicyFixMetadata[]>();
     for (const rule of POLICY_FIX_METADATA) {
diff --git a/extensions/policy/src/doctor/register.test.ts b/extensions/policy/src/doctor/register.test.ts
@@ -1739,6 +1739,136 @@ describe("registerPolicyDoctorChecks", () => {
     });
   });
 
+  it("dry-runs required tool deny repairs without mutating config", async () => {
+    const configPath = join(workspaceDir, "openclaw.jsonc");
+    const cfg = {
+      ...cfgWithPolicy({ workspaceRepairs: true }),
+      tools: { deny: ["read"] },
+    } as unknown as OpenClawConfig;
+    await fs.writeFile(configPath, "{}", "utf-8");
+    await fs.writeFile(
+      join(workspaceDir, "policy.jsonc"),
+      JSON.stringify({ tools: { denyTools: ["exec", "write"] } }),
+      "utf-8",
+    );
+
+    const result = await runPolicyRepairCheck("policy/tools-required-deny-missing", {
+      ...repairCtx(configPath, cfg),
+      dryRun: true,
+    });
+
+    expect(result.findings).toEqual(
+      expect.arrayContaining([
+        expect.objectContaining({
+          checkId: "policy/tools-required-deny-missing",
+          message: "global tools config does not deny required tool 'exec'.",
+          ocPath: "oc://openclaw.config/tools/deny",
+        }),
+        expect.objectContaining({
+          checkId: "policy/tools-required-deny-missing",
+          message: "global tools config does not deny required tool 'write'.",
+          ocPath: "oc://openclaw.config/tools/deny",
+        }),
+      ]),
+    );
+    expect(result.status).toBe("repaired");
+    expect(result.changes).toEqual([
+      "Added exec to tools.deny for policy conformance.",
+      "Added write to tools.deny for policy conformance.",
+    ]);
+    expect(result.config.tools?.deny).toEqual(["read", "exec", "write"]);
+    expect(cfg.tools?.deny).toEqual(["read"]);
+  });
+
+  it("repairs required agent workspace deny tool findings", async () => {
+    const configPath = join(workspaceDir, "openclaw.jsonc");
+    const cfg = {
+      ...cfgWithPolicy({ workspaceRepairs: true }),
+      agents: {
+        list: [
+          {
+            id: "reviewer",
+            tools: { deny: ["exec"] },
+          },
+        ],
+      },
+    } as unknown as OpenClawConfig;
+    await fs.writeFile(configPath, "{}", "utf-8");
+    await fs.writeFile(
+      join(workspaceDir, "policy.jsonc"),
+      JSON.stringify({
+        scopes: {
+          reviewer: {
+            agentIds: ["reviewer"],
+            agents: { workspace: { denyTools: ["exec", "write", "edit"] } },
+          },
+        },
+      }),
+      "utf-8",
+    );
+
+    const result = await runPolicyRepairCheck(
+      "policy/agents-tool-not-denied",
+      repairCtx(configPath, cfg),
+    );
+
+    expect(result.status).toBe("repaired");
+    expect(result.changes).toEqual([
+      "Added edit to agents.list[0].tools.deny for policy conformance.",
+      "Added write to agents.list[0].tools.deny for policy conformance.",
+    ]);
+    expect(result.remainingFindings).toEqual([]);
+    expect(result.config.agents?.list?.[0]).toMatchObject({
+      id: "reviewer",
+      tools: { deny: ["exec", "edit", "write"] },
+    });
+  });
+
+  it("skips scoped required deny repairs that would mutate root tools deny", async () => {
+    const configPath = join(workspaceDir, "openclaw.jsonc");
+    const cfg = {
+      ...cfgWithPolicy({ workspaceRepairs: true }),
+      tools: { deny: ["exec"] },
+      agents: {
+        list: [{ id: "reviewer" }],
+      },
+    } as unknown as OpenClawConfig;
+    await fs.writeFile(configPath, "{}", "utf-8");
+    await fs.writeFile(
+      join(workspaceDir, "policy.jsonc"),
+      JSON.stringify({
+        scopes: {
+          reviewer: {
+            agentIds: ["reviewer"],
+            tools: { denyTools: ["exec", "write"] },
+          },
+        },
+      }),
+      "utf-8",
+    );
+
+    const result = await runPolicyRepairCheck(
+      "policy/tools-required-deny-missing",
+      repairCtx(configPath, cfg),
+    );
+
+    expect(result.status).toBe("skipped");
+    expect(result.reason).toBe("policy automatic repair had no config changes to apply");
+    expect(result.changes).toEqual([]);
+    expect(result.warnings).toEqual([
+      "Skipped scoped deny repair for write. The finding reports inherited root tools.deny, so changing it would affect more than the scoped policy target.",
+    ]);
+    expect(result.config.tools?.deny).toEqual(["exec"]);
+    expect(result.config.agents?.list?.[0]).toEqual({ id: "reviewer" });
+    expect(result.remainingFindings).toEqual([
+      expect.objectContaining({
+        checkId: "policy/tools-required-deny-missing",
+        ocPath: "oc://openclaw.config/tools/deny",
+        requirement: "oc://policy.jsonc/scopes/reviewer/tools/denyTools",
+      }),
+    ]);
+  });
+
   it("skips scoped data-handling repairs that would mutate shared config", async () => {
     const configPath = join(workspaceDir, "openclaw.jsonc");
     const cfg = {
diff --git a/extensions/policy/src/doctor/scopes/tools.ts b/extensions/policy/src/doctor/scopes/tools.ts
@@ -27,6 +27,9 @@ export function createPolicyAgentToolChecks(deps: PolicyDoctorCheckDeps): readon
     async detect(ctx) {
       return findingsForCheck(await evaluatePolicy(ctx), CHECK_IDS.policyAgentsToolNotDenied);
     },
+    repair(ctx, findings) {
+      return repairPolicyAutomaticNarrower(ctx, findings, CHECK_IDS.policyAgentsToolNotDenied);
+    },
   };
   const policyToolsProfileUnapprovedCheck: HealthCheck = {
     id: CHECK_IDS.policyToolsProfileUnapproved,
@@ -117,6 +120,9 @@ export function createPolicyAgentToolChecks(deps: PolicyDoctorCheckDeps): readon
     async detect(ctx) {
       return findingsForCheck(await evaluatePolicy(ctx), CHECK_IDS.policyToolsRequiredDenyMissing);
     },
+    repair(ctx, findings) {
+      return repairPolicyAutomaticNarrower(ctx, findings, CHECK_IDS.policyToolsRequiredDenyMissing);
+    },
   };
 
   return [
