fix(exec-approvals): coerce bare string allowlist entries (#9903) (thanks @mcaxtr)

grp06 · grp06 · commit 141f551a4ca0 · 2026-02-05T15:52:51.000-08:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -37,6 +37,7 @@ Docs: https://docs.openclaw.ai
 - CLI: pass `--disable-warning=ExperimentalWarning` as a Node CLI option when respawning (avoid disallowed `NODE_OPTIONS` usage; fixes npm pack). (#9691) Thanks @18-RAJAT.
 - CLI: resolve bundled Chrome extension assets by walking up to the nearest assets directory; add resolver and clipboard tests. (#8914) Thanks @kelvinCB.
 - Tests: stabilize Windows ACL coverage with deterministic os.userInfo mocking. (#9335) Thanks @M00N7682.
+- Exec approvals: coerce bare string allowlist entries to objects to prevent allowlist corruption. (#9903, fixes #9790) Thanks @mcaxtr.
 - Heartbeat: allow explicit accountId routing for multi-account channels. (#8702) Thanks @lsh411.
 - TUI/Gateway: handle non-streaming finals, refresh history for non-local chat runs, and avoid event gap warnings for targeted tool streams. (#8432) Thanks @gumadeiras.
 - Shell completion: auto-detect and migrate slow dynamic patterns to cached files for faster terminal startup; add completion health checks to doctor/update/onboard.
diff --git a/src/agents/pi-tools.safe-bins.test.ts b/src/agents/pi-tools.safe-bins.test.ts
@@ -6,6 +6,16 @@ import type { OpenClawConfig } from "../config/config.js";
 import type { ExecApprovalsResolved } from "../infra/exec-approvals.js";
 import { createOpenClawCodingTools } from "./pi-tools.js";
 
+vi.mock("../plugins/tools.js", () => ({
+  getPluginToolMeta: () => undefined,
+  resolvePluginTools: () => [],
+}));
+
+vi.mock("../infra/shell-env.js", async (importOriginal) => {
+  const mod = await importOriginal<typeof import("../infra/shell-env.js")>();
+  return { ...mod, getShellPathFromLoginShell: () => null };
+});
+
 vi.mock("../infra/exec-approvals.js", async (importOriginal) => {
   const mod = await importOriginal<typeof import("../infra/exec-approvals.js")>();
   const approvals: ExecApprovalsResolved = {
diff --git a/src/agents/pi-tools.workspace-paths.test.ts b/src/agents/pi-tools.workspace-paths.test.ts
@@ -1,9 +1,19 @@
 import fs from "node:fs/promises";
 import os from "node:os";
 import path from "node:path";
-import { describe, expect, it } from "vitest";
+import { describe, expect, it, vi } from "vitest";
 import { createOpenClawCodingTools } from "./pi-tools.js";
 
+vi.mock("../plugins/tools.js", () => ({
+  getPluginToolMeta: () => undefined,
+  resolvePluginTools: () => [],
+}));
+
+vi.mock("../infra/shell-env.js", async (importOriginal) => {
+  const mod = await importOriginal<typeof import("../infra/shell-env.js")>();
+  return { ...mod, getShellPathFromLoginShell: () => null };
+});
+
 async function withTempDir<T>(prefix: string, fn: (dir: string) => Promise<T>) {
   const dir = await fs.mkdtemp(path.join(os.tmpdir(), prefix));
   try {
@@ -99,7 +109,7 @@ describe("workspace path resolution", () => {
 
   it("defaults exec cwd to workspaceDir when workdir is omitted", async () => {
     await withTempDir("openclaw-ws-", async (workspaceDir) => {
-      const tools = createOpenClawCodingTools({ workspaceDir });
+      const tools = createOpenClawCodingTools({ workspaceDir, exec: { host: "gateway" } });
       const execTool = tools.find((tool) => tool.name === "exec");
       expect(execTool).toBeDefined();
 
@@ -122,7 +132,7 @@ describe("workspace path resolution", () => {
   it("lets exec workdir override the workspace default", async () => {
     await withTempDir("openclaw-ws-", async (workspaceDir) => {
       await withTempDir("openclaw-override-", async (overrideDir) => {
-        const tools = createOpenClawCodingTools({ workspaceDir });
+        const tools = createOpenClawCodingTools({ workspaceDir, exec: { host: "gateway" } });
         const execTool = tools.find((tool) => tool.name === "exec");
         expect(execTool).toBeDefined();
 
diff --git a/src/cli/gateway-cli.coverage.test.ts b/src/cli/gateway-cli.coverage.test.ts
@@ -53,10 +53,17 @@ async function withEnvOverride<T>(
   }
 }
 
-vi.mock("../gateway/call.js", () => ({
-  callGateway: (opts: unknown) => callGateway(opts),
-  randomIdempotencyKey: () => "rk_test",
-}));
+vi.mock(
+  new URL("../../gateway/call.ts", new URL("./gateway-cli/call.ts", import.meta.url)).href,
+  async (importOriginal) => {
+    const mod = await importOriginal();
+    return {
+      ...mod,
+      callGateway: (opts: unknown) => callGateway(opts),
+      randomIdempotencyKey: () => "rk_test",
+    };
+  },
+);
 
 vi.mock("../gateway/server.js", () => ({
   startGatewayServer: (port: number, opts?: unknown) => startGatewayServer(port, opts),
@@ -122,7 +129,7 @@ describe("gateway-cli coverage", () => {
 
     expect(callGateway).toHaveBeenCalledTimes(1);
     expect(runtimeLogs.join("\n")).toContain('"ok": true');
-  }, 30_000);
+  }, 60_000);
 
   it("registers gateway probe and routes to gatewayStatusCommand", async () => {
     runtimeLogs.length = 0;
@@ -137,7 +144,7 @@ describe("gateway-cli coverage", () => {
     await program.parseAsync(["gateway", "probe", "--json"], { from: "user" });
 
     expect(gatewayStatusCommand).toHaveBeenCalledTimes(1);
-  }, 30_000);
+  }, 60_000);
 
   it("registers gateway discover and prints JSON", async () => {
     runtimeLogs.length = 0;
diff --git a/src/cli/program.smoke.test.ts b/src/cli/program.smoke.test.ts
@@ -50,6 +50,7 @@ vi.mock("../gateway/call.js", () => ({
   }),
 }));
 vi.mock("./deps.js", () => ({ createDefaultDeps: () => ({}) }));
+vi.mock("./preaction.js", () => ({ registerPreActionHooks: () => {} }));
 
 const { buildProgram } = await import("./program.js");
 
diff --git a/src/infra/exec-approvals.test.ts b/src/infra/exec-approvals.test.ts
@@ -680,4 +680,37 @@ describe("normalizeExecApprovals handles string allowlist entries (#9790)", () =
     // Only "ls" should survive; empty/whitespace strings should be dropped
     expect(entries.map((e) => e.pattern)).toEqual(["ls"]);
   });
+
+  it("drops malformed object entries with missing/non-string patterns", () => {
+    const file = {
+      version: 1,
+      agents: {
+        main: {
+          allowlist: [{ pattern: "/usr/bin/ls" }, {}, { pattern: 123 }, { pattern: "   " }, "echo"],
+        },
+      },
+    } as unknown as ExecApprovalsFile;
+
+    const normalized = normalizeExecApprovals(file);
+    const entries = normalized.agents?.main?.allowlist ?? [];
+
+    expect(entries.map((e) => e.pattern)).toEqual(["/usr/bin/ls", "echo"]);
+    for (const entry of entries) {
+      expect(entry).not.toHaveProperty("0");
+    }
+  });
+
+  it("drops non-array allowlist values", () => {
+    const file = {
+      version: 1,
+      agents: {
+        main: {
+          allowlist: "ls",
+        },
+      },
+    } as unknown as ExecApprovalsFile;
+
+    const normalized = normalizeExecApprovals(file);
+    expect(normalized.agents?.main?.allowlist).toBeUndefined();
+  });
 });
diff --git a/src/infra/exec-approvals.ts b/src/infra/exec-approvals.ts
@@ -132,18 +132,11 @@ function ensureDir(filePath: string) {
   fs.mkdirSync(dir, { recursive: true });
 }
 
-/**
- * Coerce each allowlist item into a proper {@link ExecAllowlistEntry}.
- * Older config formats or manual edits may store bare strings (e.g.
- * `["ls", "cat"]`).  Spreading a string (`{ ..."ls" }`) produces
- * `{"0":"l","1":"s"}`, so we must detect and convert strings first.
- * Non-object, non-string entries and blank strings are dropped.
- */
-function coerceAllowlistEntries(
-  allowlist: unknown[] | undefined,
-): ExecAllowlistEntry[] | undefined {
+// Coerce legacy/corrupted allowlists into `ExecAllowlistEntry[]` before we spread
+// entries to add ids (spreading strings creates {"0":"l","1":"s",...}).
+function coerceAllowlistEntries(allowlist: unknown): ExecAllowlistEntry[] | undefined {
   if (!Array.isArray(allowlist) || allowlist.length === 0) {
-    return allowlist as ExecAllowlistEntry[] | undefined;
+    return Array.isArray(allowlist) ? (allowlist as ExecAllowlistEntry[]) : undefined;
   }
   let changed = false;
   const result: ExecAllowlistEntry[] = [];
@@ -157,7 +150,12 @@ function coerceAllowlistEntries(
         changed = true; // dropped empty string
       }
     } else if (item && typeof item === "object" && !Array.isArray(item)) {
-      result.push(item as ExecAllowlistEntry);
+      const pattern = (item as { pattern?: unknown }).pattern;
+      if (typeof pattern === "string" && pattern.trim().length > 0) {
+        result.push(item as ExecAllowlistEntry);
+      } else {
+        changed = true; // dropped invalid entry
+      }
     } else {
       changed = true; // dropped invalid entry
     }
@@ -193,7 +191,7 @@ export function normalizeExecApprovals(file: ExecApprovalsFile): ExecApprovalsFi
     delete agents.default;
   }
   for (const [key, agent] of Object.entries(agents)) {
-    const coerced = coerceAllowlistEntries(agent.allowlist as unknown[]);
+    const coerced = coerceAllowlistEntries(agent.allowlist);
     const allowlist = ensureAllowlistIds(coerced);
     if (allowlist !== agent.allowlist) {
       agents[key] = { ...agent, allowlist };
