diff --git a/src/node-host/invoke-system-run-plan.test.ts b/src/node-host/invoke-system-run-plan.test.ts index b0904435dc6..3953c8f2d30 100644 --- a/src/node-host/invoke-system-run-plan.test.ts +++ b/src/node-host/invoke-system-run-plan.test.ts @@ -7,81 +7,105 @@ import { hardenApprovedExecutionPaths, } from "./invoke-system-run-plan.js"; +type PathTokenSetup = { + expected: string; +}; + +type HardeningCase = { + name: string; + mode: "build-plan" | "harden"; + argv: string[]; + shellCommand?: string | null; + withPathToken?: boolean; + expectedArgv: (ctx: { pathToken: PathTokenSetup | null }) => string[]; + expectedCmdText?: string; +}; + describe("hardenApprovedExecutionPaths", () => { - it.runIf(process.platform !== "win32")( - "preserves shell-wrapper argv during approval hardening", - () => { - const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-shell-wrapper-")); - try { - const prepared = buildSystemRunApprovalPlan({ - command: ["env", "sh", "-c", "echo SAFE"], - cwd: tmp, - }); - expect(prepared.ok).toBe(true); - if (!prepared.ok) { - throw new Error("unreachable"); - } - expect(prepared.plan.argv).toEqual(["env", "sh", "-c", "echo SAFE"]); - expect(prepared.cmdText).toBe("echo SAFE"); - } finally { - fs.rmSync(tmp, { recursive: true, force: true }); - } + const cases: HardeningCase[] = [ + { + name: "preserves shell-wrapper argv during approval hardening", + mode: "build-plan", + argv: ["env", "sh", "-c", "echo SAFE"], + expectedArgv: () => ["env", "sh", "-c", "echo SAFE"], + expectedCmdText: "echo SAFE", }, - ); - - it.runIf(process.platform !== "win32")( - "preserves dispatch-wrapper argv during approval hardening", - () => { - const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-dispatch-wrapper-")); - try { - const hardened = hardenApprovedExecutionPaths({ - approvedByAsk: true, - argv: ["env", "tr", "a", "b"], - shellCommand: null, - cwd: tmp, - }); - expect(hardened.ok).toBe(true); - if (!hardened.ok) { - throw new Error("unreachable"); - } - expect(hardened.argv).toEqual(["env", "tr", "a", "b"]); - } finally { - fs.rmSync(tmp, { recursive: true, force: true }); - } + { + name: "preserves dispatch-wrapper argv during approval hardening", + mode: "harden", + argv: ["env", "tr", "a", "b"], + shellCommand: null, + expectedArgv: () => ["env", "tr", "a", "b"], }, - ); + { + name: "pins direct PATH-token executable during approval hardening", + mode: "harden", + argv: ["poccmd", "SAFE"], + shellCommand: null, + withPathToken: true, + expectedArgv: ({ pathToken }) => [pathToken!.expected, "SAFE"], + }, + { + name: "preserves env-wrapper PATH-token argv during approval hardening", + mode: "harden", + argv: ["env", "poccmd", "SAFE"], + shellCommand: null, + withPathToken: true, + expectedArgv: () => ["env", "poccmd", "SAFE"], + }, + ]; - it.runIf(process.platform !== "win32")( - "pins direct PATH-token executable during approval hardening", - () => { - const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-direct-pin-")); - const binDir = path.join(tmp, "bin"); - fs.mkdirSync(binDir, { recursive: true }); - const link = path.join(binDir, "poccmd"); - fs.symlinkSync("/bin/echo", link); - const expected = fs.realpathSync(link); + for (const testCase of cases) { + it.runIf(process.platform !== "win32")(testCase.name, () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-approval-hardening-")); const oldPath = process.env.PATH; - process.env.PATH = `${binDir}${path.delimiter}${oldPath ?? ""}`; + let pathToken: PathTokenSetup | null = null; + if (testCase.withPathToken) { + const binDir = path.join(tmp, "bin"); + fs.mkdirSync(binDir, { recursive: true }); + const link = path.join(binDir, "poccmd"); + fs.symlinkSync("/bin/echo", link); + pathToken = { expected: fs.realpathSync(link) }; + process.env.PATH = `${binDir}${path.delimiter}${oldPath ?? ""}`; + } try { + if (testCase.mode === "build-plan") { + const prepared = buildSystemRunApprovalPlan({ + command: testCase.argv, + cwd: tmp, + }); + expect(prepared.ok).toBe(true); + if (!prepared.ok) { + throw new Error("unreachable"); + } + expect(prepared.plan.argv).toEqual(testCase.expectedArgv({ pathToken })); + if (testCase.expectedCmdText) { + expect(prepared.cmdText).toBe(testCase.expectedCmdText); + } + return; + } + const hardened = hardenApprovedExecutionPaths({ approvedByAsk: true, - argv: ["poccmd", "SAFE"], - shellCommand: null, + argv: testCase.argv, + shellCommand: testCase.shellCommand ?? null, cwd: tmp, }); expect(hardened.ok).toBe(true); if (!hardened.ok) { throw new Error("unreachable"); } - expect(hardened.argv).toEqual([expected, "SAFE"]); + expect(hardened.argv).toEqual(testCase.expectedArgv({ pathToken })); } finally { - if (oldPath === undefined) { - delete process.env.PATH; - } else { - process.env.PATH = oldPath; + if (testCase.withPathToken) { + if (oldPath === undefined) { + delete process.env.PATH; + } else { + process.env.PATH = oldPath; + } } fs.rmSync(tmp, { recursive: true, force: true }); } - }, - ); + }); + } }); diff --git a/src/node-host/invoke-system-run-plan.ts b/src/node-host/invoke-system-run-plan.ts index 15b8dad9f7e..9eca71c5adc 100644 --- a/src/node-host/invoke-system-run-plan.ts +++ b/src/node-host/invoke-system-run-plan.ts @@ -53,6 +53,16 @@ function hasMutableSymlinkPathComponentSync(targetPath: string): boolean { return false; } +function shouldPinExecutableForApproval(params: { + shellCommand: string | null; + wrapperChain: string[] | undefined; +}): boolean { + if (params.shellCommand !== null) { + return false; + } + return (params.wrapperChain?.length ?? 0) === 0; +} + export function hardenApprovedExecutionPaths(params: { approvedByAsk: boolean; argv: string[]; @@ -116,19 +126,19 @@ export function hardenApprovedExecutionPaths(params: { return { ok: true, argv: params.argv, cwd: hardenedCwd }; } - // Preserve shell-wrapper semantics. Rewriting argv[0] for wrappers can change - // runtime behavior (for example: env sh -c ... -> /bin/sh sh -c ...). - if (params.shellCommand !== null) { + const resolution = resolveCommandResolutionFromArgv(params.argv, hardenedCwd); + if ( + !shouldPinExecutableForApproval({ + shellCommand: params.shellCommand, + wrapperChain: resolution?.wrapperChain, + }) + ) { + // Preserve wrapper semantics for approval-based execution. Pinning the + // effective executable while keeping wrapper argv shape can shift positional + // arguments and execute a different command than approved. return { ok: true, argv: params.argv, cwd: hardenedCwd }; } - const resolution = resolveCommandResolutionFromArgv(params.argv, hardenedCwd); - // Preserve transparent wrapper semantics for approval-based execution. - // Pinning the effective executable while keeping wrapper argv shape can shift - // positional arguments and execute a different command than approved. - if ((resolution?.wrapperChain?.length ?? 0) > 0) { - return { ok: true, argv: params.argv, cwd: hardenedCwd }; - } const pinnedExecutable = resolution?.resolvedRealPath ?? resolution?.resolvedPath; if (!pinnedExecutable) { return { diff --git a/src/node-host/invoke-system-run.test.ts b/src/node-host/invoke-system-run.test.ts index 3bfd806e88f..6296ae6f101 100644 --- a/src/node-host/invoke-system-run.test.ts +++ b/src/node-host/invoke-system-run.test.ts @@ -300,6 +300,81 @@ describe("handleSystemRunInvoke mac app exec host routing", () => { }); }); + const approvedEnvShellWrapperCases = [ + { + name: "preserves wrapper argv for approved env shell commands in local execution", + preferMacAppExecHost: false, + }, + { + name: "preserves wrapper argv for approved env shell commands in mac app exec host forwarding", + preferMacAppExecHost: true, + }, + ] as const; + + for (const testCase of approvedEnvShellWrapperCases) { + it.runIf(process.platform !== "win32")(testCase.name, async () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-approved-wrapper-")); + const marker = path.join(tmp, "marker"); + const attackerScript = path.join(tmp, "sh"); + fs.writeFileSync(attackerScript, "#!/bin/sh\necho exploited > marker\n"); + fs.chmodSync(attackerScript, 0o755); + const runCommand = vi.fn(async (argv: string[]) => { + if (argv[0] === "/bin/sh" && argv[1] === "sh" && argv[2] === "-c") { + fs.writeFileSync(marker, "rewritten"); + } + return createLocalRunResult(); + }); + const sendInvokeResult = vi.fn(async () => {}); + try { + const invoke = await runSystemInvoke({ + preferMacAppExecHost: testCase.preferMacAppExecHost, + command: ["env", "sh", "-c", "echo SAFE"], + cwd: tmp, + approved: true, + security: "allowlist", + ask: "on-miss", + runCommand, + sendInvokeResult, + runViaResponse: testCase.preferMacAppExecHost + ? { + ok: true, + payload: { + success: true, + stdout: "app-ok", + stderr: "", + timedOut: false, + exitCode: 0, + error: null, + }, + } + : undefined, + }); + + if (testCase.preferMacAppExecHost) { + const canonicalCwd = fs.realpathSync(tmp); + expect(invoke.runCommand).not.toHaveBeenCalled(); + expect(invoke.runViaMacAppExecHost).toHaveBeenCalledWith({ + approvals: expect.anything(), + request: expect.objectContaining({ + command: ["env", "sh", "-c", "echo SAFE"], + rawCommand: "echo SAFE", + cwd: canonicalCwd, + }), + }); + expectInvokeOk(invoke.sendInvokeResult, { payloadContains: "app-ok" }); + return; + } + + const runArgs = vi.mocked(invoke.runCommand).mock.calls[0]?.[0] as string[] | undefined; + expect(runArgs).toEqual(["env", "sh", "-c", "echo SAFE"]); + expect(fs.existsSync(marker)).toBe(false); + expectInvokeOk(invoke.sendInvokeResult); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + } + it("handles transparent env wrappers in allowlist mode", async () => { const { runCommand, sendInvokeResult } = await runSystemInvoke({ preferMacAppExecHost: false, @@ -479,42 +554,6 @@ describe("handleSystemRunInvoke mac app exec host routing", () => { } }); - it.runIf(process.platform !== "win32")( - "preserves wrapper argv for approved env shell commands", - async () => { - const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-approved-wrapper-")); - const marker = path.join(tmp, "marker"); - const attackerScript = path.join(tmp, "sh"); - fs.writeFileSync(attackerScript, "#!/bin/sh\necho exploited > marker\n"); - fs.chmodSync(attackerScript, 0o755); - const runCommand = vi.fn(async (argv: string[]) => { - if (argv[0] === "/bin/sh" && argv[1] === "sh" && argv[2] === "-c") { - fs.writeFileSync(marker, "rewritten"); - } - return createLocalRunResult(); - }); - const sendInvokeResult = vi.fn(async () => {}); - try { - await runSystemInvoke({ - preferMacAppExecHost: false, - command: ["env", "sh", "-c", "echo SAFE"], - cwd: tmp, - approved: true, - security: "allowlist", - ask: "on-miss", - runCommand, - sendInvokeResult, - }); - const runArgs = vi.mocked(runCommand).mock.calls[0]?.[0] as string[] | undefined; - expect(runArgs).toEqual(["env", "sh", "-c", "echo SAFE"]); - expect(fs.existsSync(marker)).toBe(false); - expectInvokeOk(sendInvokeResult); - } finally { - fs.rmSync(tmp, { recursive: true, force: true }); - } - }, - ); - it("denies ./sh wrapper spoof in allowlist on-miss mode before execution", async () => { const marker = path.join(os.tmpdir(), `openclaw-wrapper-spoof-${process.pid}-${Date.now()}`); const runCommand = vi.fn(async () => {