diff --git a/src/agents/bash-tools.exec.path.test.ts b/src/agents/bash-tools.exec.path.test.ts index 9bdbe07524c..5481ec9668d 100644 --- a/src/agents/bash-tools.exec.path.test.ts +++ b/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; 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", () => { diff --git a/src/infra/shell-env.test.ts b/src/infra/shell-env.test.ts index ab06202dc20..644948b03c9 100644 --- a/src/infra/shell-env.test.ts +++ b/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", () => { diff --git a/src/infra/shell-env.ts b/src/infra/shell-env.ts index ac1369c48be..796c19b2666 100644 --- a/src/infra/shell-env.ts +++ b/src/infra/shell-env.ts @@ -110,6 +110,28 @@ function parseShellEnv(stdout: Buffer): Map { return shellEnv; } +type LoginShellEnvProbeResult = + | { ok: true; shellEnv: Map } + | { 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; }