From 141f551a4ca0635e781a11c5aef09986e4c7f448 Mon Sep 17 00:00:00 2001 From: George Pickett Date: Thu, 5 Feb 2026 15:51:27 -0800 Subject: [PATCH] fix(exec-approvals): coerce bare string allowlist entries (#9903) (thanks @mcaxtr) --- CHANGELOG.md | 1 + src/agents/pi-tools.safe-bins.test.ts | 10 +++++++ src/agents/pi-tools.workspace-paths.test.ts | 16 ++++++++-- src/cli/gateway-cli.coverage.test.ts | 19 ++++++++---- src/cli/program.smoke.test.ts | 1 + src/infra/exec-approvals.test.ts | 33 +++++++++++++++++++++ src/infra/exec-approvals.ts | 24 +++++++-------- 7 files changed, 82 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e2e1e0f2..6715fdca6 100644 --- 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 index ecf976ef4..34f0176ac 100644 --- 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(); + return { ...mod, getShellPathFromLoginShell: () => null }; +}); + vi.mock("../infra/exec-approvals.js", async (importOriginal) => { const mod = await importOriginal(); const approvals: ExecApprovalsResolved = { diff --git a/src/agents/pi-tools.workspace-paths.test.ts b/src/agents/pi-tools.workspace-paths.test.ts index f6388c884..054f7bf12 100644 --- 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(); + return { ...mod, getShellPathFromLoginShell: () => null }; +}); + async function withTempDir(prefix: string, fn: (dir: string) => Promise) { 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 index f7adb3933..d70e4aa4d 100644 --- a/src/cli/gateway-cli.coverage.test.ts +++ b/src/cli/gateway-cli.coverage.test.ts @@ -53,10 +53,17 @@ async function withEnvOverride( } } -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 index edb684cfa..9827c0e27 100644 --- 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 index e1d196b48..6ccebc2e0 100644 --- 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 index 87b8c58c1..05787b1a3 100644 --- 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 };