Skip to content

Commit c40884d

Browse files
optimoljacobtomlinson
optimol
and
jacobtomlinson
authored
Prefer non-user writeable paths (#54346)
* infra: trust system binary roots * infra: isolate windows install root overrides * infra: narrow windows reg lookup * browser: restore windows executable comments --------- Co-authored-by: Jacob Tomlinson <[email protected]>
1 parent 9d58f9e commit c40884d

12 files changed

Lines changed: 1121 additions & 36 deletions

‎extensions/browser/src/browser/chrome.executables.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -574,9 +574,8 @@ export function findGoogleChromeExecutableLinux(): BrowserExecutable | null {
574574
export function findChromeExecutableWindows(): BrowserExecutable | null {
575575
const localAppData = process.env.LOCALAPPDATA ?? "";
576576
const programFiles = process.env.ProgramFiles ?? "C:\\Program Files";
577-
// Must use bracket notation: variable name contains parentheses
577+
// Must use bracket notation: variable name contains parentheses.
578578
const programFilesX86 = process.env["ProgramFiles(x86)"] ?? "C:\\Program Files (x86)";
579-
580579
const joinWin = path.win32.join;
581580
const candidates: Array<BrowserExecutable> = [];
582581

‎src/daemon/runtime-paths.test.ts‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -254,4 +254,40 @@ describe("resolveSystemNodeInfo", () => {
254254
expect(warning).toContain("below the required Node 22.14+");
255255
expect(warning).toContain(darwinNode);
256256
});
257+
258+
it("uses validated custom Program Files roots on Windows", async () => {
259+
const customNode = "D:\\Programs\\nodejs\\node.exe";
260+
mockNodePathPresent(customNode);
261+
262+
const execFile = vi.fn().mockResolvedValue({ stdout: "24.11.1\n", stderr: "" });
263+
const result = await resolveSystemNodeInfo({
264+
env: {
265+
ProgramFiles: "D:\\Programs",
266+
"ProgramFiles(x86)": "E:\\Programs (x86)",
267+
},
268+
platform: "win32",
269+
execFile,
270+
});
271+
272+
expect(result?.path).toBe(customNode);
273+
});
274+
275+
it("prefers ProgramW6432 over ProgramFiles on Windows", async () => {
276+
const preferredNode = "D:\\Programs\\nodejs\\node.exe";
277+
const x86Node = "E:\\Programs (x86)\\nodejs\\node.exe";
278+
mockNodePathPresent(preferredNode, x86Node);
279+
280+
const execFile = vi.fn().mockResolvedValue({ stdout: "24.11.1\n", stderr: "" });
281+
const result = await resolveSystemNodeInfo({
282+
env: {
283+
ProgramFiles: "E:\\Programs (x86)",
284+
"ProgramFiles(x86)": "E:\\Programs (x86)",
285+
ProgramW6432: "D:\\Programs",
286+
},
287+
platform: "win32",
288+
execFile,
289+
});
290+
291+
expect(result?.path).toBe(preferredNode);
292+
});
257293
});

‎src/daemon/runtime-paths.ts‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import path from "node:path";
44
import { promisify } from "node:util";
55
import { isSupportedNodeVersion } from "../infra/runtime-guard.js";
66
import { resolveStableNodePath } from "../infra/stable-node-path.js";
7+
import { getWindowsProgramFilesRoots } from "../infra/windows-install-roots.js";
78

89
const VERSION_MANAGER_MARKERS = [
910
"/.nvm/",
@@ -47,12 +48,9 @@ function buildSystemNodeCandidates(
4748
}
4849
if (platform === "win32") {
4950
const pathModule = getPathModule(platform);
50-
const programFiles = env.ProgramFiles ?? "C:\\Program Files";
51-
const programFilesX86 = env["ProgramFiles(x86)"] ?? "C:\\Program Files (x86)";
52-
return [
53-
pathModule.join(programFiles, "nodejs", "node.exe"),
54-
pathModule.join(programFilesX86, "nodejs", "node.exe"),
55-
];
51+
return getWindowsProgramFilesRoots(env).map((root) =>
52+
pathModule.join(root, "nodejs", "node.exe"),
53+
);
5654
}
5755
return [];
5856
}

‎src/daemon/schtasks.ts‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import path from "node:path";
44
import { isGatewayArgv } from "../infra/gateway-process-argv.js";
55
import { findVerifiedGatewayListenerPidsOnPortSync } from "../infra/gateway-processes.js";
66
import { inspectPortUsage } from "../infra/ports.js";
7+
import { getWindowsInstallRoots } from "../infra/windows-install-roots.js";
78
import { killProcessTree } from "../process/kill-tree.js";
89
import { sleep } from "../utils.js";
910
import { parseCmdScriptCommandLine, quoteCmdScriptArg } from "./cmd-argv.js";
@@ -438,11 +439,7 @@ async function terminateGatewayProcessTree(pid: number, graceMs: number): Promis
438439
killProcessTree(pid, { graceMs });
439440
return;
440441
}
441-
const taskkillPath = path.join(
442-
process.env.SystemRoot ?? "C:\\Windows",
443-
"System32",
444-
"taskkill.exe",
445-
);
442+
const taskkillPath = path.join(getWindowsInstallRoots().systemRoot, "System32", "taskkill.exe");
446443
spawnSync(taskkillPath, ["/T", "/PID", String(pid)], {
447444
stdio: "ignore",
448445
timeout: 5_000,

‎src/infra/path-env.test.ts‎

Lines changed: 88 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -123,8 +123,8 @@ describe("ensureOpenClawCliOnPath", () => {
123123
expect(process.env.PATH).toBe("/bin");
124124
});
125125

126-
it("prepends mise shims when available", () => {
127-
const { tmp, appBinDir, appCli } = setupAppCliRoot("case-mise");
126+
it("appends mise shims after system dirs", () => {
127+
const { tmp, appCli } = setupAppCliRoot("case-mise");
128128
const miseDataDir = path.join(tmp, "mise");
129129
const shimsDir = path.join(miseDataDir, "shims");
130130
setDir(miseDataDir);
@@ -140,10 +140,10 @@ describe("ensureOpenClawCliOnPath", () => {
140140
homeDir: tmp,
141141
platform: "darwin",
142142
});
143-
const appBinIndex = updated.indexOf(appBinDir);
143+
const usrBinIndex = updated.indexOf("/usr/bin");
144144
const shimsIndex = updated.indexOf(shimsDir);
145-
expect(appBinIndex).toBeGreaterThanOrEqual(0);
146-
expect(shimsIndex).toBeGreaterThan(appBinIndex);
145+
expect(usrBinIndex).toBeGreaterThanOrEqual(0);
146+
expect(shimsIndex).toBeGreaterThan(usrBinIndex);
147147
});
148148

149149
it.each([
@@ -222,7 +222,85 @@ describe("ensureOpenClawCliOnPath", () => {
222222
expect(updated.indexOf(xdgBinHome)).toBeLessThan(updated.indexOf(localBin));
223223
});
224224

225-
it("prepends Linuxbrew dirs when present", () => {
225+
it("places ~/.local/bin AFTER /usr/bin to prevent PATH hijack", () => {
226+
const { tmp, appCli } = setupAppCliRoot("case-path-hijack");
227+
const localBin = path.join(tmp, ".local", "bin");
228+
setDir(path.join(tmp, ".local"));
229+
setDir(localBin);
230+
231+
process.env.PATH = "/usr/bin:/bin";
232+
delete process.env.OPENCLAW_PATH_BOOTSTRAPPED;
233+
delete process.env.XDG_BIN_HOME;
234+
235+
const updated = bootstrapPath({
236+
execPath: appCli,
237+
cwd: tmp,
238+
homeDir: tmp,
239+
platform: "linux",
240+
});
241+
const usrBinIndex = updated.indexOf("/usr/bin");
242+
const localBinIndex = updated.indexOf(localBin);
243+
expect(usrBinIndex).toBeGreaterThanOrEqual(0);
244+
expect(localBinIndex).toBeGreaterThanOrEqual(0);
245+
expect(localBinIndex).toBeGreaterThan(usrBinIndex);
246+
});
247+
248+
it("places all user-writable home dirs after system dirs", () => {
249+
const { tmp, appCli } = setupAppCliRoot("case-user-writable-after-system");
250+
const localBin = path.join(tmp, ".local", "bin");
251+
const pnpmBin = path.join(tmp, ".local", "share", "pnpm");
252+
const bunBin = path.join(tmp, ".bun", "bin");
253+
const yarnBin = path.join(tmp, ".yarn", "bin");
254+
setDir(path.join(tmp, ".local"));
255+
setDir(localBin);
256+
setDir(path.join(tmp, ".local", "share"));
257+
setDir(pnpmBin);
258+
setDir(path.join(tmp, ".bun"));
259+
setDir(bunBin);
260+
setDir(path.join(tmp, ".yarn"));
261+
setDir(yarnBin);
262+
263+
process.env.PATH = "/usr/bin:/bin";
264+
delete process.env.OPENCLAW_PATH_BOOTSTRAPPED;
265+
delete process.env.XDG_BIN_HOME;
266+
267+
const updated = bootstrapPath({
268+
execPath: appCli,
269+
cwd: tmp,
270+
homeDir: tmp,
271+
platform: "linux",
272+
});
273+
const usrBinIndex = updated.indexOf("/usr/bin");
274+
for (const userDir of [localBin, pnpmBin, bunBin, yarnBin]) {
275+
const idx = updated.indexOf(userDir);
276+
expect(idx, `${userDir} should come after /usr/bin`).toBeGreaterThan(usrBinIndex);
277+
}
278+
});
279+
280+
it("appends Homebrew dirs after immutable OS dirs", () => {
281+
const { tmp, appCli } = setupAppCliRoot("case-homebrew-after-system");
282+
setDir("/opt/homebrew/bin");
283+
setDir("/usr/local/bin");
284+
285+
process.env.PATH = "/usr/bin:/bin";
286+
delete process.env.OPENCLAW_PATH_BOOTSTRAPPED;
287+
delete process.env.HOMEBREW_PREFIX;
288+
delete process.env.HOMEBREW_BREW_FILE;
289+
delete process.env.XDG_BIN_HOME;
290+
291+
const updated = bootstrapPath({
292+
execPath: appCli,
293+
cwd: tmp,
294+
homeDir: tmp,
295+
platform: "darwin",
296+
});
297+
const usrBinIndex = updated.indexOf("/usr/bin");
298+
expect(usrBinIndex).toBeGreaterThanOrEqual(0);
299+
expect(updated.indexOf("/opt/homebrew/bin")).toBeGreaterThan(usrBinIndex);
300+
expect(updated.indexOf("/usr/local/bin")).toBeGreaterThan(usrBinIndex);
301+
});
302+
303+
it("appends Linuxbrew dirs after system dirs", () => {
226304
const tmp = abs("/tmp/openclaw-path/case-linuxbrew");
227305
const execDir = path.join(tmp, "exec");
228306
setDir(tmp);
@@ -247,7 +325,9 @@ describe("ensureOpenClawCliOnPath", () => {
247325
homeDir: tmp,
248326
platform: "linux",
249327
});
250-
expect(parts[0]).toBe(linuxbrewBin);
251-
expect(parts[1]).toBe(linuxbrewSbin);
328+
const usrBinIndex = parts.indexOf("/usr/bin");
329+
expect(usrBinIndex).toBeGreaterThanOrEqual(0);
330+
expect(parts.indexOf(linuxbrewBin)).toBeGreaterThan(usrBinIndex);
331+
expect(parts.indexOf(linuxbrewSbin)).toBeGreaterThan(usrBinIndex);
252332
});
253333
});

‎src/infra/path-env.ts‎

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -81,26 +81,30 @@ function candidateBinDirs(opts: EnsureOpenClawPathOpts): { prepend: string[]; ap
8181
}
8282
}
8383

84+
// Only immutable OS directories go in prepend so they take priority over
85+
// user-writable locations, preventing PATH hijack of system binaries.
86+
prepend.push("/usr/bin", "/bin");
87+
88+
// User-writable / package-manager directories are appended so they never
89+
// shadow trusted OS binaries.
90+
// This includes Brew/Homebrew dirs, which are useful for finding `openclaw`
91+
// in launchd/minimal environments but must not be treated as trusted.
92+
append.push(...resolveBrewPathDirs({ homeDir }));
8493
const miseDataDir = process.env.MISE_DATA_DIR ?? path.join(homeDir, ".local", "share", "mise");
8594
const miseShims = path.join(miseDataDir, "shims");
8695
if (isDirectory(miseShims)) {
87-
prepend.push(miseShims);
96+
append.push(miseShims);
8897
}
89-
90-
prepend.push(...resolveBrewPathDirs({ homeDir }));
91-
92-
// Common global install locations (macOS first).
9398
if (platform === "darwin") {
94-
prepend.push(path.join(homeDir, "Library", "pnpm"));
99+
append.push(path.join(homeDir, "Library", "pnpm"));
95100
}
96101
if (process.env.XDG_BIN_HOME) {
97-
prepend.push(process.env.XDG_BIN_HOME);
102+
append.push(process.env.XDG_BIN_HOME);
98103
}
99-
prepend.push(path.join(homeDir, ".local", "bin"));
100-
prepend.push(path.join(homeDir, ".local", "share", "pnpm"));
101-
prepend.push(path.join(homeDir, ".bun", "bin"));
102-
prepend.push(path.join(homeDir, ".yarn", "bin"));
103-
prepend.push("/opt/homebrew/bin", "/usr/local/bin", "/usr/bin", "/bin");
104+
append.push(path.join(homeDir, ".local", "bin"));
105+
append.push(path.join(homeDir, ".local", "share", "pnpm"));
106+
append.push(path.join(homeDir, ".bun", "bin"));
107+
append.push(path.join(homeDir, ".yarn", "bin"));
104108

105109
return { prepend: prepend.filter(isDirectory), append: append.filter(isDirectory) };
106110
}

0 commit comments

Comments
 (0)