refactor(security): dedupe shell env probe and add path regression test

src/agents/bash-tools.exec.path.test.ts

@@ -1,3 +1,6 @@
+import fs from "node:fs";
+import os from "node:os";
+import path from "node:path";
 import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
 import type { ExecApprovalsResolved } from "../infra/exec-approvals.js";
 import { captureEnv } from "../test-utils/env.js";
@@ -67,7 +70,7 @@ describe("exec PATH login shell merge", () => {
   let envSnapshot: ReturnType<typeof captureEnv>;
 
   beforeEach(() => {
-    envSnapshot = captureEnv(["PATH"]);
+    envSnapshot = captureEnv(["PATH", "SHELL"]);
   });
 
   afterEach(() => {
@@ -112,6 +115,43 @@ describe("exec PATH login shell merge", () => {
 
     expect(shellPathMock).not.toHaveBeenCalled();
   });
+
+  it("does not apply login-shell PATH when probe rejects unregistered absolute SHELL", async () => {
+    if (isWin) {
+      return;
+    }
+    process.env.PATH = "/usr/bin";
+    const shellDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-shell-env-"));
+    const unregisteredShellPath = path.join(shellDir, "unregistered-shell");
+    fs.writeFileSync(unregisteredShellPath, '#!/bin/sh\nexec /bin/sh "$@"\n', {
+      encoding: "utf8",
+      mode: 0o755,
+    });
+    process.env.SHELL = unregisteredShellPath;
+
+    try {
+      const shellPathMock = vi.mocked(getShellPathFromLoginShell);
+      shellPathMock.mockClear();
+      shellPathMock.mockImplementation((opts) =>
+        opts.env.SHELL?.trim() === unregisteredShellPath ? null : "/custom/bin:/opt/bin",
+      );
+
+      const tool = createExecTool({ host: "gateway", security: "full", ask: "off" });
+      const result = await tool.execute("call1", { command: "echo $PATH" });
+      const entries = normalizePathEntries(result.content.find((c) => c.type === "text")?.text);
+
+      expect(entries).toEqual(["/usr/bin"]);
+      expect(shellPathMock).toHaveBeenCalledTimes(1);
+      expect(shellPathMock).toHaveBeenCalledWith(
+        expect.objectContaining({
+          env: process.env,
+          timeoutMs: 1234,
+        }),
+      );
+    } finally {
+      fs.rmSync(shellDir, { recursive: true, force: true });
+    }
+  });
 });
 
 describe("exec host env validation", () => {

src/infra/shell-env.test.ts

@@ -59,6 +59,23 @@ describe("shell env fallback", () => {
     expect(receivedEnv?.HOME).toBe(os.homedir());
   }
 
+  function withEtcShells(shells: string[], fn: () => void) {
+    const etcShellsContent = `${shells.join("\n")}\n`;
+    const readFileSyncSpy = vi
+      .spyOn(fs, "readFileSync")
+      .mockImplementation((filePath, encoding) => {
+        if (filePath === "/etc/shells" && encoding === "utf8") {
+          return etcShellsContent;
+        }
+        throw new Error(`Unexpected readFileSync(${String(filePath)}) in test`);
+      });
+    try {
+      fn();
+    } finally {
+      readFileSyncSpy.mockRestore();
+    }
+  }
+
   it("is disabled by default", () => {
     expect(shouldEnableShellEnvFallback({} as NodeJS.ProcessEnv)).toBe(false);
     expect(shouldEnableShellEnvFallback({ OPENCLAW_LOAD_SHELL_ENV: "0" })).toBe(false);
@@ -172,44 +189,24 @@ describe("shell env fallback", () => {
   });
 
   it("falls back to /bin/sh when SHELL is absolute but not registered in /etc/shells", () => {
-    const readFileSyncSpy = vi
-      .spyOn(fs, "readFileSync")
-      .mockImplementation((filePath, encoding) => {
-        if (filePath === "/etc/shells" && encoding === "utf8") {
-          return "/bin/sh\n/bin/bash\n/bin/zsh\n";
-        }
-        throw new Error(`Unexpected readFileSync(${String(filePath)}) in test`);
-      });
-    try {
+    withEtcShells(["/bin/sh", "/bin/bash", "/bin/zsh"], () => {
       const { res, exec } = runShellEnvFallbackForShell("/opt/homebrew/bin/evil-shell");
 
       expect(res.ok).toBe(true);
       expect(exec).toHaveBeenCalledTimes(1);
       expect(exec).toHaveBeenCalledWith("/bin/sh", ["-l", "-c", "env -0"], expect.any(Object));
-    } finally {
-      readFileSyncSpy.mockRestore();
-    }
+    });
   });
 
   it("uses SHELL when it is explicitly registered in /etc/shells", () => {
-    const readFileSyncSpy = vi
-      .spyOn(fs, "readFileSync")
-      .mockImplementation((filePath, encoding) => {
-        if (filePath === "/etc/shells" && encoding === "utf8") {
-          return "/bin/sh\n/usr/bin/zsh-trusted\n";
-        }
-        throw new Error(`Unexpected readFileSync(${String(filePath)}) in test`);
-      });
-    try {
+    withEtcShells(["/bin/sh", "/usr/bin/zsh-trusted"], () => {
       const trustedShell = "/usr/bin/zsh-trusted";
       const { res, exec } = runShellEnvFallbackForShell(trustedShell);
 
       expect(res.ok).toBe(true);
       expect(exec).toHaveBeenCalledTimes(1);
       expect(exec).toHaveBeenCalledWith(trustedShell, ["-l", "-c", "env -0"], expect.any(Object));
-    } finally {
-      readFileSyncSpy.mockRestore();
-    }
+    });
   });
 
   it("sanitizes startup-related env vars before shell fallback exec", () => {

src/infra/shell-env.ts

@@ -110,6 +110,28 @@ function parseShellEnv(stdout: Buffer): Map<string, string> {
   return shellEnv;
 }
 
+type LoginShellEnvProbeResult =
+  | { ok: true; shellEnv: Map<string, string> }
+  | { ok: false; error: string };
+
+function probeLoginShellEnv(params: {
+  env: NodeJS.ProcessEnv;
+  timeoutMs?: number;
+  exec?: typeof execFileSync;
+}): LoginShellEnvProbeResult {
+  const exec = params.exec ?? execFileSync;
+  const timeoutMs = resolveTimeoutMs(params.timeoutMs);
+  const shell = resolveShell(params.env);
+  const execEnv = resolveShellExecEnv(params.env);
+
+  try {
+    const stdout = execLoginShellEnvZero({ shell, env: execEnv, exec, timeoutMs });
+    return { ok: true, shellEnv: parseShellEnv(stdout) };
+  } catch (err) {
+    return { ok: false, error: err instanceof Error ? err.message : String(err) };
+  }
+}
+
 export type ShellEnvFallbackResult =
   | { ok: true; applied: string[]; skippedReason?: never }
   | { ok: true; applied: []; skippedReason: "already-has-keys" | "disabled" }
@@ -126,7 +148,6 @@ export type ShellEnvFallbackOptions = {
 
 export function loadShellEnvFallback(opts: ShellEnvFallbackOptions): ShellEnvFallbackResult {
   const logger = opts.logger ?? console;
-  const exec = opts.exec ?? execFileSync;
 
   if (!opts.enabled) {
     lastAppliedKeys = [];
@@ -139,29 +160,23 @@ export function loadShellEnvFallback(opts: ShellEnvFallbackOptions): ShellEnvFal
     return { ok: true, applied: [], skippedReason: "already-has-keys" };
   }
 
-  const timeoutMs = resolveTimeoutMs(opts.timeoutMs);
-
-  const shell = resolveShell(opts.env);
-  const execEnv = resolveShellExecEnv(opts.env);
-
-  let stdout: Buffer;
-  try {
-    stdout = execLoginShellEnvZero({ shell, env: execEnv, exec, timeoutMs });
-  } catch (err) {
-    const msg = err instanceof Error ? err.message : String(err);
-    logger.warn(`[openclaw] shell env fallback failed: ${msg}`);
+  const probe = probeLoginShellEnv({
+    env: opts.env,
+    timeoutMs: opts.timeoutMs,
+    exec: opts.exec,
+  });
+  if (!probe.ok) {
+    logger.warn(`[openclaw] shell env fallback failed: ${probe.error}`);
     lastAppliedKeys = [];
-    return { ok: false, error: msg, applied: [] };
+    return { ok: false, error: probe.error, applied: [] };
   }
 
-  const shellEnv = parseShellEnv(stdout);
-
   const applied: string[] = [];
   for (const key of opts.expectedKeys) {
     if (opts.env[key]?.trim()) {
       continue;
     }
-    const value = shellEnv.get(key);
+    const value = probe.shellEnv.get(key);
     if (!value?.trim()) {
       continue;
     }
@@ -208,21 +223,17 @@ export function getShellPathFromLoginShell(opts: {
     return cachedShellPath;
   }
 
-  const exec = opts.exec ?? execFileSync;
-  const timeoutMs = resolveTimeoutMs(opts.timeoutMs);
-  const shell = resolveShell(opts.env);
-  const execEnv = resolveShellExecEnv(opts.env);
-
-  let stdout: Buffer;
-  try {
-    stdout = execLoginShellEnvZero({ shell, env: execEnv, exec, timeoutMs });
-  } catch {
+  const probe = probeLoginShellEnv({
+    env: opts.env,
+    timeoutMs: opts.timeoutMs,
+    exec: opts.exec,
+  });
+  if (!probe.ok) {
     cachedShellPath = null;
     return cachedShellPath;
   }
 
-  const shellEnv = parseShellEnv(stdout);
-  const shellPath = shellEnv.get("PATH")?.trim();
+  const shellPath = probe.shellEnv.get("PATH")?.trim();
   cachedShellPath = shellPath && shellPath.length > 0 ? shellPath : null;
   return cachedShellPath;
 }