refactor: dedupe exec wrapper denial plan and test setup

2026-03-07 22:44:16 +00:00 · 2026-02-25 00:43:19 +00:00
parent 943b8f171a
commit a9ce6bd79b
3 changed files with 140 additions and 125 deletions
--- a/src/infra/exec-approvals.test.ts
+++ b/src/infra/exec-approvals.test.ts
@@ -24,6 +24,14 @@ import {
  type ExecAllowlistEntry,
 } from "./exec-approvals.js";

+function buildNestedEnvShellCommand(params: {
+  envExecutable: string;
+  depth: number;
+  payload: string;
+}): string[] {
+  return [...Array(params.depth).fill(params.envExecutable), "/bin/sh", "-c", params.payload];
+}
+
 describe("exec approvals allowlist matching", () => {
  const baseResolution = {
    rawExecutable: "rg",
@@ -311,7 +319,11 @@ describe("exec approvals command resolution", () => {
    fs.chmodSync(envPath, 0o755);

    const analysis = analyzeArgvCommand({
-      argv: [envPath, envPath, envPath, envPath, envPath, "/bin/sh", "-c", "echo pwned"],
+      argv: buildNestedEnvShellCommand({
+        envExecutable: envPath,
+        depth: 5,
+        payload: "echo pwned",
+      }),
      cwd: dir,
      env: makePathEnv(binDir),
    });
--- a/src/infra/exec-wrapper-resolution.ts
+++ b/src/infra/exec-wrapper-resolution.ts
@@ -448,6 +448,19 @@ function isSemanticDispatchWrapperUsage(wrapper: string, argv: string[]): boolea
  return !TRANSPARENT_DISPATCH_WRAPPERS.has(wrapper);
 }

+function blockedDispatchWrapperPlan(params: {
+  argv: string[];
+  wrappers: string[];
+  blockedWrapper: string;
+}): DispatchWrapperExecutionPlan {
+  return {
+    argv: params.argv,
+    wrappers: params.wrappers,
+    policyBlocked: true,
+    blockedWrapper: params.blockedWrapper,
+  };
+}
+
 export function resolveDispatchWrapperExecutionPlan(
  argv: string[],
  maxDepth = MAX_DISPATCH_WRAPPER_DEPTH,
@@ -457,36 +470,33 @@ export function resolveDispatchWrapperExecutionPlan(
  for (let depth = 0; depth < maxDepth; depth += 1) {
    const unwrap = unwrapKnownDispatchWrapperInvocation(current);
    if (unwrap.kind === "blocked") {
-      return {
+      return blockedDispatchWrapperPlan({
        argv: current,
        wrappers,
-        policyBlocked: true,
        blockedWrapper: unwrap.wrapper,
-      };
+      });
    }
    if (unwrap.kind !== "unwrapped" || unwrap.argv.length === 0) {
      break;
    }
    wrappers.push(unwrap.wrapper);
    if (isSemanticDispatchWrapperUsage(unwrap.wrapper, current)) {
-      return {
+      return blockedDispatchWrapperPlan({
        argv: current,
        wrappers,
-        policyBlocked: true,
        blockedWrapper: unwrap.wrapper,
-      };
+      });
    }
    current = unwrap.argv;
  }
  if (wrappers.length >= maxDepth) {
    const overflow = unwrapKnownDispatchWrapperInvocation(current);
    if (overflow.kind === "blocked" || overflow.kind === "unwrapped") {
-      return {
+      return blockedDispatchWrapperPlan({
        argv: current,
        wrappers,
-        policyBlocked: true,
        blockedWrapper: overflow.wrapper,
-      };
+      });
    }
  }
  return { argv: current, wrappers, policyBlocked: false };
--- a/src/node-host/invoke-system-run.test.ts
+++ b/src/node-host/invoke-system-run.test.ts
@@ -21,6 +21,30 @@ describe("formatSystemRunAllowlistMissMessage", () => {
 });

 describe("handleSystemRunInvoke mac app exec host routing", () => {
+  function buildNestedEnvShellCommand(params: { depth: number; payload: string }): string[] {
+    return [...Array(params.depth).fill("/usr/bin/env"), "/bin/sh", "-c", params.payload];
+  }
+
+  async function withTempApprovalsHome<T>(params: {
+    approvals: Parameters<typeof saveExecApprovals>[0];
+    run: (ctx: { tempHome: string }) => Promise<T>;
+  }): Promise<T> {
+    const tempHome = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-exec-approvals-"));
+    const previousOpenClawHome = process.env.OPENCLAW_HOME;
+    process.env.OPENCLAW_HOME = tempHome;
+    saveExecApprovals(params.approvals);
+    try {
+      return await params.run({ tempHome });
+    } finally {
+      if (previousOpenClawHome === undefined) {
+        delete process.env.OPENCLAW_HOME;
+      } else {
+        process.env.OPENCLAW_HOME = previousOpenClawHome;
+      }
+      fs.rmSync(tempHome, { recursive: true, force: true });
+    }
+  }
+
  async function runSystemInvoke(params: {
    preferMacAppExecHost: boolean;
    runViaResponse?: ExecHostResponse | null;
@@ -254,22 +278,6 @@ describe("handleSystemRunInvoke mac app exec host routing", () => {
  });

  it("denies ./skill-bin even when autoAllowSkills trust entry exists", async () => {
-    const tempHome = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-skill-path-spoof-"));
-    const previousOpenClawHome = process.env.OPENCLAW_HOME;
-    const skillBinPath = path.join(tempHome, "skill-bin");
-    fs.writeFileSync(skillBinPath, "#!/bin/sh\necho should-not-run\n", { mode: 0o755 });
-    fs.chmodSync(skillBinPath, 0o755);
-    process.env.OPENCLAW_HOME = tempHome;
-    saveExecApprovals({
-      version: 1,
-      defaults: {
-        security: "allowlist",
-        ask: "on-miss",
-        askFallback: "deny",
-        autoAllowSkills: true,
-      },
-      agents: {},
-    });
    const runCommand = vi.fn(async () => ({
      success: true,
      stdout: "local-ok",
@@ -282,39 +290,47 @@ describe("handleSystemRunInvoke mac app exec host routing", () => {
    const sendInvokeResult = vi.fn(async () => {});
    const sendNodeEvent = vi.fn(async () => {});

-    try {
-      await handleSystemRunInvoke({
-        client: {} as never,
-        params: {
-          command: ["./skill-bin", "--help"],
-          cwd: tempHome,
-          sessionKey: "agent:main:main",
+    await withTempApprovalsHome({
+      approvals: {
+        version: 1,
+        defaults: {
+          security: "allowlist",
+          ask: "on-miss",
+          askFallback: "deny",
+          autoAllowSkills: true,
        },
-        skillBins: {
-          current: async () => [{ name: "skill-bin", resolvedPath: skillBinPath }],
-        },
-        execHostEnforced: false,
-        execHostFallbackAllowed: true,
-        resolveExecSecurity: () => "allowlist",
-        resolveExecAsk: () => "on-miss",
-        isCmdExeInvocation: () => false,
-        sanitizeEnv: () => undefined,
-        runCommand,
-        runViaMacAppExecHost: vi.fn(async () => null),
-        sendNodeEvent,
-        buildExecEventPayload: (payload) => payload,
-        sendInvokeResult,
-        sendExecFinishedEvent: vi.fn(async () => {}),
-        preferMacAppExecHost: false,
-      });
-    } finally {
-      if (previousOpenClawHome === undefined) {
-        delete process.env.OPENCLAW_HOME;
-      } else {
-        process.env.OPENCLAW_HOME = previousOpenClawHome;
-      }
-      fs.rmSync(tempHome, { recursive: true, force: true });
-    }
+        agents: {},
+      },
+      run: async ({ tempHome }) => {
+        const skillBinPath = path.join(tempHome, "skill-bin");
+        fs.writeFileSync(skillBinPath, "#!/bin/sh\necho should-not-run\n", { mode: 0o755 });
+        fs.chmodSync(skillBinPath, 0o755);
+        await handleSystemRunInvoke({
+          client: {} as never,
+          params: {
+            command: ["./skill-bin", "--help"],
+            cwd: tempHome,
+            sessionKey: "agent:main:main",
+          },
+          skillBins: {
+            current: async () => [{ name: "skill-bin", resolvedPath: skillBinPath }],
+          },
+          execHostEnforced: false,
+          execHostFallbackAllowed: true,
+          resolveExecSecurity: () => "allowlist",
+          resolveExecAsk: () => "on-miss",
+          isCmdExeInvocation: () => false,
+          sanitizeEnv: () => undefined,
+          runCommand,
+          runViaMacAppExecHost: vi.fn(async () => null),
+          sendNodeEvent,
+          buildExecEventPayload: (payload) => payload,
+          sendInvokeResult,
+          sendExecFinishedEvent: vi.fn(async () => {}),
+          preferMacAppExecHost: false,
+        });
+      },
+    });

    expect(runCommand).not.toHaveBeenCalled();
    expect(sendNodeEvent).toHaveBeenCalledWith(
@@ -353,82 +369,59 @@ describe("handleSystemRunInvoke mac app exec host routing", () => {
    if (process.platform === "win32") {
      return;
    }
-    const tempHome = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-env-depth-overflow-"));
-    const previousOpenClawHome = process.env.OPENCLAW_HOME;
-    const marker = path.join(tempHome, "pwned.txt");
-    process.env.OPENCLAW_HOME = tempHome;
-    saveExecApprovals({
-      version: 1,
-      defaults: {
-        security: "allowlist",
-        ask: "on-miss",
-        askFallback: "deny",
-      },
-      agents: {
-        main: {
-          allowlist: [{ pattern: "/usr/bin/env" }],
-        },
-      },
-    });
    const runCommand = vi.fn(async () => {
-      fs.writeFileSync(marker, "executed");
-      return {
-        success: true,
-        stdout: "local-ok",
-        stderr: "",
-        timedOut: false,
-        truncated: false,
-        exitCode: 0,
-        error: null,
-      };
+      throw new Error("runCommand should not be called for nested env depth overflow");
    });
    const sendInvokeResult = vi.fn(async () => {});
    const sendNodeEvent = vi.fn(async () => {});

-    try {
-      await handleSystemRunInvoke({
-        client: {} as never,
-        params: {
-          command: [
-            "/usr/bin/env",
-            "/usr/bin/env",
-            "/usr/bin/env",
-            "/usr/bin/env",
-            "/usr/bin/env",
-            "/bin/sh",
-            "-c",
-            `echo PWNED > ${marker}`,
-          ],
-          sessionKey: "agent:main:main",
+    await withTempApprovalsHome({
+      approvals: {
+        version: 1,
+        defaults: {
+          security: "allowlist",
+          ask: "on-miss",
+          askFallback: "deny",
        },
-        skillBins: {
-          current: async () => [],
+        agents: {
+          main: {
+            allowlist: [{ pattern: "/usr/bin/env" }],
+          },
        },
-        execHostEnforced: false,
-        execHostFallbackAllowed: true,
-        resolveExecSecurity: () => "allowlist",
-        resolveExecAsk: () => "on-miss",
-        isCmdExeInvocation: () => false,
-        sanitizeEnv: () => undefined,
-        runCommand,
-        runViaMacAppExecHost: vi.fn(async () => null),
-        sendNodeEvent,
-        buildExecEventPayload: (payload) => payload,
-        sendInvokeResult,
-        sendExecFinishedEvent: vi.fn(async () => {}),
-        preferMacAppExecHost: false,
-      });
-    } finally {
-      if (previousOpenClawHome === undefined) {
-        delete process.env.OPENCLAW_HOME;
-      } else {
-        process.env.OPENCLAW_HOME = previousOpenClawHome;
-      }
-      fs.rmSync(tempHome, { recursive: true, force: true });
-    }
+      },
+      run: async ({ tempHome }) => {
+        const marker = path.join(tempHome, "pwned.txt");
+        await handleSystemRunInvoke({
+          client: {} as never,
+          params: {
+            command: buildNestedEnvShellCommand({
+              depth: 5,
+              payload: `echo PWNED > ${marker}`,
+            }),
+            sessionKey: "agent:main:main",
+          },
+          skillBins: {
+            current: async () => [],
+          },
+          execHostEnforced: false,
+          execHostFallbackAllowed: true,
+          resolveExecSecurity: () => "allowlist",
+          resolveExecAsk: () => "on-miss",
+          isCmdExeInvocation: () => false,
+          sanitizeEnv: () => undefined,
+          runCommand,
+          runViaMacAppExecHost: vi.fn(async () => null),
+          sendNodeEvent,
+          buildExecEventPayload: (payload) => payload,
+          sendInvokeResult,
+          sendExecFinishedEvent: vi.fn(async () => {}),
+          preferMacAppExecHost: false,
+        });
+        expect(fs.existsSync(marker)).toBe(false);
+      },
+    });

    expect(runCommand).not.toHaveBeenCalled();
-    expect(fs.existsSync(marker)).toBe(false);
    expect(sendNodeEvent).toHaveBeenCalledWith(
      expect.anything(),
      "exec.denied",