Skip to content

Commit f86953f

Browse files
pgondhi987pgondhi987
authored
fix(infra): block ambient Homebrew env vars from brew resolution (#74463)
* fix: address issue * fix: address issue * fix: address PR review feedback * fix: address PR review feedback * fix: address codex review feedback * docs: add changelog entry for PR merge
1 parent 94b4b3c commit f86953f

8 files changed

Lines changed: 224 additions & 102 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ Docs: https://docs.openclaw.ai
2222

2323
### Fixes
2424

25+
- fix(infra): block ambient Homebrew env vars from brew resolution. (#74463) Thanks @pgondhi987.
2526
- Thinking/providers: resolve bundled provider thinking profiles through lightweight provider policy artifacts when startup-lazy providers are not active, so OpenAI Codex GPT-5.x keeps xhigh available in Gateway session validation. Fixes #74796. Thanks @maxschachere.
2627
- Plugins/TTS: keep bundled speech-provider discovery available on cold package Gateway paths and add bundled plugin matrix runtime probes for health, readiness, RPC, TTS discovery, and post-ready runtime-deps watchdog coverage. Refs #75283. Thanks @vincentkoc.
2728
- Google Meet/Twilio: show delegated voice call ID, DTMF, and intro-greeting state in `googlemeet doctor`, and avoid claiming DTMF was sent when no Meet PIN sequence was configured. Refs #72478. Thanks @DougButdorf.

‎src/agents/skills-install-fallback.test.ts‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,65 @@ describe("skills-install fallback edge cases", () => {
165165
expect(runCommandWithTimeoutMock).not.toHaveBeenCalled();
166166
});
167167

168+
it("does not use HOMEBREW_PREFIX as a brew bin fallback for go installs", async () => {
169+
const envSnapshot = captureEnv(["HOMEBREW_PREFIX"]);
170+
try {
171+
const maliciousPrefix = path.join(workspaceDir, "evil-brew");
172+
process.env.HOMEBREW_PREFIX = maliciousPrefix;
173+
mockAvailableBinaries([]);
174+
skillsInstallTesting.setDepsForTest({
175+
hasBinary: (bin: string) => hasBinaryMock(bin),
176+
resolveBrewExecutable: () => "/safe/homebrew/bin/brew",
177+
});
178+
runCommandWithTimeoutMock.mockResolvedValue({
179+
code: 0,
180+
stdout: "ok",
181+
stderr: "",
182+
signal: null,
183+
killed: false,
184+
});
185+
runCommandWithTimeoutMock.mockResolvedValueOnce({
186+
code: 0,
187+
stdout: "installed go",
188+
stderr: "",
189+
signal: null,
190+
killed: false,
191+
});
192+
runCommandWithTimeoutMock.mockResolvedValueOnce({
193+
code: 1,
194+
stdout: "",
195+
stderr: "prefix unavailable",
196+
signal: null,
197+
killed: false,
198+
});
199+
200+
const result = await installSkill({
201+
workspaceDir,
202+
skillName: "go-tool-single",
203+
installId: "deps",
204+
});
205+
206+
expect(result.ok).toBe(true);
207+
expect(runCommandWithTimeoutMock).toHaveBeenNthCalledWith(
208+
1,
209+
["/safe/homebrew/bin/brew", "install", "go"],
210+
expect.objectContaining({ timeoutMs: 300_000 }),
211+
);
212+
expect(runCommandWithTimeoutMock).toHaveBeenNthCalledWith(
213+
2,
214+
["/safe/homebrew/bin/brew", "--prefix"],
215+
expect.objectContaining({ timeoutMs: 30_000 }),
216+
);
217+
const finalCall = runCommandWithTimeoutMock.mock.calls.at(-1) as
218+
| [string[], { env?: NodeJS.ProcessEnv }]
219+
| undefined;
220+
expect(finalCall?.[0]).toEqual(["go", "install", "example.com/tool@latest"]);
221+
expect(finalCall?.[1]?.env?.GOBIN).not.toBe(path.join(maliciousPrefix, "bin"));
222+
} finally {
223+
envSnapshot.restore();
224+
}
225+
});
226+
168227
it("preserves system uv/python env vars when running uv installs", async () => {
169228
mockAvailableBinaries(["uv"]);
170229
runCommandWithTimeoutMock.mockResolvedValueOnce({

‎src/agents/skills-install.ts‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -234,11 +234,6 @@ async function resolveBrewBinDir(timeoutMs: number, brewExe?: string): Promise<s
234234
}
235235
}
236236

237-
const envPrefix = process.env.HOMEBREW_PREFIX?.trim();
238-
if (envPrefix) {
239-
return path.join(envPrefix, "bin");
240-
}
241-
242237
for (const candidate of ["/opt/homebrew/bin", "/usr/local/bin"]) {
243238
try {
244239
if (fs.existsSync(candidate)) {

‎src/infra/brew.test.ts‎

Lines changed: 107 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -4,78 +4,114 @@ import { describe, expect, it } from "vitest";
44
import { withTempDir } from "../test-helpers/temp-dir.js";
55
import { resolveBrewExecutable, resolveBrewPathDirs } from "./brew.js";
66

7+
const HOMEBREW_ENV_KEYS = ["HOMEBREW_BREW_FILE", "HOMEBREW_PREFIX"] as const;
8+
79
describe("brew helpers", () => {
810
async function writeExecutable(filePath: string) {
911
await fs.mkdir(path.dirname(filePath), { recursive: true });
1012
await fs.writeFile(filePath, "#!/bin/sh\necho ok\n", "utf-8");
1113
await fs.chmod(filePath, 0o755);
1214
}
1315

16+
async function withHomebrewEnv(
17+
values: Partial<Record<(typeof HOMEBREW_ENV_KEYS)[number], string>>,
18+
run: () => Promise<void>,
19+
) {
20+
const previous = Object.fromEntries(
21+
HOMEBREW_ENV_KEYS.map((key) => [key, process.env[key]]),
22+
) as Record<(typeof HOMEBREW_ENV_KEYS)[number], string | undefined>;
23+
try {
24+
for (const key of HOMEBREW_ENV_KEYS) {
25+
const value = values[key];
26+
if (value === undefined) {
27+
delete process.env[key];
28+
} else {
29+
process.env[key] = value;
30+
}
31+
}
32+
await run();
33+
} finally {
34+
for (const key of HOMEBREW_ENV_KEYS) {
35+
const value = previous[key];
36+
if (value === undefined) {
37+
delete process.env[key];
38+
} else {
39+
process.env[key] = value;
40+
}
41+
}
42+
}
43+
}
44+
45+
async function withPathEnv(value: string | undefined, run: () => Promise<void>) {
46+
const previous = process.env.PATH;
47+
try {
48+
if (value === undefined) {
49+
delete process.env.PATH;
50+
} else {
51+
process.env.PATH = value;
52+
}
53+
await run();
54+
} finally {
55+
if (previous === undefined) {
56+
delete process.env.PATH;
57+
} else {
58+
process.env.PATH = previous;
59+
}
60+
}
61+
}
62+
1463
it("resolves brew from ~/.linuxbrew/bin when executable exists", async () => {
1564
await withTempDir({ prefix: "openclaw-brew-" }, async (tmp) => {
1665
const homebrewBin = path.join(tmp, ".linuxbrew", "bin");
1766
const brewPath = path.join(homebrewBin, "brew");
1867
await writeExecutable(brewPath);
1968

20-
const env: NodeJS.ProcessEnv = {};
21-
expect(resolveBrewExecutable({ homeDir: tmp, env })).toBe(brewPath);
69+
await withPathEnv("", async () => {
70+
expect(resolveBrewExecutable({ homeDir: tmp })).toBe(brewPath);
71+
});
2272
});
2373
});
2474

25-
it("prefers HOMEBREW_PREFIX/bin/brew when present", async () => {
75+
it("resolves brew from absolute PATH entries for non-standard installs", async () => {
2676
await withTempDir({ prefix: "openclaw-brew-" }, async (tmp) => {
27-
const prefix = path.join(tmp, "prefix");
28-
const prefixBin = path.join(prefix, "bin");
29-
const prefixBrew = path.join(prefixBin, "brew");
30-
await writeExecutable(prefixBrew);
31-
32-
const homebrewBin = path.join(tmp, ".linuxbrew", "bin");
33-
const homebrewBrew = path.join(homebrewBin, "brew");
34-
await writeExecutable(homebrewBrew);
77+
const customBin = path.join(tmp, "custom-homebrew", "bin");
78+
const customBrew = path.join(customBin, "brew");
79+
await writeExecutable(customBrew);
3580

36-
const env: NodeJS.ProcessEnv = { HOMEBREW_PREFIX: prefix };
37-
expect(resolveBrewExecutable({ homeDir: tmp, env })).toBe(prefixBrew);
81+
await withPathEnv(customBin, async () => {
82+
expect(resolveBrewExecutable({ homeDir: path.join(tmp, "home") })).toBe(customBrew);
83+
});
3884
});
3985
});
4086

41-
it("prefers HOMEBREW_BREW_FILE over prefix and trims value", async () => {
87+
it("ignores HOMEBREW_BREW_FILE and HOMEBREW_PREFIX by default", async () => {
4288
await withTempDir({ prefix: "openclaw-brew-" }, async (tmp) => {
4389
const explicit = path.join(tmp, "custom", "brew");
4490
const prefix = path.join(tmp, "prefix");
45-
const prefixBrew = path.join(prefix, "bin", "brew");
91+
const prefixBin = path.join(prefix, "bin");
92+
const prefixBrew = path.join(prefixBin, "brew");
93+
const homebrewBin = path.join(tmp, ".linuxbrew", "bin");
94+
const homebrewBrew = path.join(homebrewBin, "brew");
4695
await writeExecutable(explicit);
4796
await writeExecutable(prefixBrew);
97+
await writeExecutable(homebrewBrew);
4898

49-
const env: NodeJS.ProcessEnv = {
50-
HOMEBREW_BREW_FILE: ` ${explicit} `,
51-
HOMEBREW_PREFIX: prefix,
52-
};
53-
expect(resolveBrewExecutable({ homeDir: tmp, env })).toBe(explicit);
54-
});
55-
});
56-
57-
it("falls back to prefix when HOMEBREW_BREW_FILE is missing or not executable", async () => {
58-
await withTempDir({ prefix: "openclaw-brew-" }, async (tmp) => {
59-
const explicit = path.join(tmp, "custom", "brew");
60-
const prefix = path.join(tmp, "prefix");
61-
const prefixBrew = path.join(prefix, "bin", "brew");
62-
let brewFile = explicit;
63-
if (process.platform === "win32") {
64-
// Windows doesn't enforce POSIX executable bits, so use a missing path
65-
// to verify fallback behavior deterministically.
66-
brewFile = path.join(tmp, "custom", "missing-brew");
67-
} else {
68-
await fs.mkdir(path.dirname(explicit), { recursive: true });
69-
await fs.writeFile(explicit, "#!/bin/sh\necho no\n", "utf-8");
70-
await fs.chmod(explicit, 0o644);
71-
}
72-
await writeExecutable(prefixBrew);
73-
74-
const env: NodeJS.ProcessEnv = {
75-
HOMEBREW_BREW_FILE: brewFile,
76-
HOMEBREW_PREFIX: prefix,
77-
};
78-
expect(resolveBrewExecutable({ homeDir: tmp, env })).toBe(prefixBrew);
99+
await withHomebrewEnv(
100+
{
101+
HOMEBREW_BREW_FILE: explicit,
102+
HOMEBREW_PREFIX: prefix,
103+
},
104+
async () => {
105+
const env: NodeJS.ProcessEnv = {
106+
HOMEBREW_BREW_FILE: explicit,
107+
HOMEBREW_PREFIX: prefix,
108+
};
109+
await withPathEnv("", async () => {
110+
expect(resolveBrewExecutable({ homeDir: tmp, env })).toBe(homebrewBrew);
111+
});
112+
expect(resolveBrewPathDirs({ homeDir: tmp, env })).not.toContain(prefixBin);
113+
},
114+
);
79115
});
80116
});
81117

@@ -85,37 +121,39 @@ describe("brew helpers", () => {
85121
const brewPath = path.join(homebrewBin, "brew");
86122
await writeExecutable(brewPath);
87123

88-
const env: NodeJS.ProcessEnv = {
89-
HOMEBREW_BREW_FILE: " ",
90-
HOMEBREW_PREFIX: "\t",
91-
};
92-
93-
expect(resolveBrewExecutable({ homeDir: tmp, env })).toBe(brewPath);
94-
95-
const dirs = resolveBrewPathDirs({ homeDir: tmp, env });
96-
expect(dirs).not.toContain(path.join("", "bin"));
97-
expect(dirs).not.toContain(path.join("", "sbin"));
124+
await withHomebrewEnv(
125+
{
126+
HOMEBREW_BREW_FILE: " ",
127+
HOMEBREW_PREFIX: "\t",
128+
},
129+
async () => {
130+
await withPathEnv("", async () => {
131+
expect(resolveBrewExecutable({ homeDir: tmp })).toBe(brewPath);
132+
});
133+
134+
const dirs = resolveBrewPathDirs({ homeDir: tmp });
135+
expect(dirs).not.toContain(path.join("", "bin"));
136+
expect(dirs).not.toContain(path.join("", "sbin"));
137+
},
138+
);
98139
});
99140
});
100141

101142
it("always includes the standard macOS brew dirs after linuxbrew candidates", () => {
102-
const env: NodeJS.ProcessEnv = {
103-
HOMEBREW_BREW_FILE: " ",
104-
HOMEBREW_PREFIX: " ",
105-
};
106-
const dirs = resolveBrewPathDirs({ homeDir: "/home/test", env });
143+
const dirs = resolveBrewPathDirs({ homeDir: "/home/test" });
107144

108145
expect(dirs.slice(-2)).toEqual(["/opt/homebrew/bin", "/usr/local/bin"]);
109146
});
110147

111-
it("includes Linuxbrew bin/sbin in path candidates", () => {
112-
const env: NodeJS.ProcessEnv = { HOMEBREW_PREFIX: "/custom/prefix" };
113-
const dirs = resolveBrewPathDirs({ homeDir: "/home/test", env });
114-
expect(dirs).toContain(path.join("/custom/prefix", "bin"));
115-
expect(dirs).toContain(path.join("/custom/prefix", "sbin"));
116-
expect(dirs).toContain("/home/linuxbrew/.linuxbrew/bin");
117-
expect(dirs).toContain("/home/linuxbrew/.linuxbrew/sbin");
118-
expect(dirs).toContain(path.join("/home/test", ".linuxbrew", "bin"));
119-
expect(dirs).toContain(path.join("/home/test", ".linuxbrew", "sbin"));
148+
it("includes Linuxbrew bin/sbin in path candidates without env prefixes", async () => {
149+
await withHomebrewEnv({ HOMEBREW_PREFIX: "/custom/prefix" }, async () => {
150+
const dirs = resolveBrewPathDirs({ homeDir: "/home/test" });
151+
expect(dirs).not.toContain(path.join("/custom/prefix", "bin"));
152+
expect(dirs).not.toContain(path.join("/custom/prefix", "sbin"));
153+
expect(dirs).toContain("/home/linuxbrew/.linuxbrew/bin");
154+
expect(dirs).toContain("/home/linuxbrew/.linuxbrew/sbin");
155+
expect(dirs).toContain(path.join("/home/test", ".linuxbrew", "bin"));
156+
expect(dirs).toContain(path.join("/home/test", ".linuxbrew", "sbin"));
157+
});
120158
});
121159
});

‎src/infra/brew.ts‎

Lines changed: 26 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -11,26 +11,33 @@ function isExecutable(filePath: string): boolean {
1111
}
1212
}
1313

14-
function normalizePathValue(value: unknown): string | undefined {
15-
if (typeof value !== "string") {
16-
return undefined;
14+
type BrewResolutionOptions = {
15+
homeDir?: string;
16+
/**
17+
* @deprecated No-op compatibility field for plugin SDK callers. Homebrew
18+
* env vars are ignored for resolution because workspace env can be untrusted.
19+
*/
20+
env?: NodeJS.ProcessEnv;
21+
};
22+
23+
function resolveBrewFromPath(pathEnv = process.env.PATH): string | undefined {
24+
for (const dir of (pathEnv ?? "").split(path.delimiter)) {
25+
const trimmed = dir.trim();
26+
if (!trimmed || !path.isAbsolute(trimmed)) {
27+
continue;
28+
}
29+
const candidate = path.join(trimmed, "brew");
30+
if (isExecutable(candidate)) {
31+
return candidate;
32+
}
1733
}
18-
const trimmed = value.trim();
19-
return trimmed ? trimmed : undefined;
34+
return undefined;
2035
}
2136

22-
export function resolveBrewPathDirs(opts?: {
23-
homeDir?: string;
24-
env?: NodeJS.ProcessEnv;
25-
}): string[] {
37+
export function resolveBrewPathDirs(opts?: BrewResolutionOptions): string[] {
2638
const homeDir = opts?.homeDir ?? os.homedir();
27-
const env = opts?.env ?? process.env;
2839

2940
const dirs: string[] = [];
30-
const prefix = normalizePathValue(env.HOMEBREW_PREFIX);
31-
if (prefix) {
32-
dirs.push(path.join(prefix, "bin"), path.join(prefix, "sbin"));
33-
}
3441

3542
// Linuxbrew defaults.
3643
dirs.push(path.join(homeDir, ".linuxbrew", "bin"));
@@ -43,24 +50,15 @@ export function resolveBrewPathDirs(opts?: {
4350
return dirs;
4451
}
4552

46-
export function resolveBrewExecutable(opts?: {
47-
homeDir?: string;
48-
env?: NodeJS.ProcessEnv;
49-
}): string | undefined {
53+
export function resolveBrewExecutable(opts?: BrewResolutionOptions): string | undefined {
5054
const homeDir = opts?.homeDir ?? os.homedir();
51-
const env = opts?.env ?? process.env;
5255

53-
const candidates: string[] = [];
54-
55-
const brewFile = normalizePathValue(env.HOMEBREW_BREW_FILE);
56-
if (brewFile) {
57-
candidates.push(brewFile);
56+
const pathBrew = resolveBrewFromPath();
57+
if (pathBrew) {
58+
return pathBrew;
5859
}
5960

60-
const prefix = normalizePathValue(env.HOMEBREW_PREFIX);
61-
if (prefix) {
62-
candidates.push(path.join(prefix, "bin", "brew"));
63-
}
61+
const candidates: string[] = [];
6462

6563
// Linuxbrew defaults.
6664
candidates.push(path.join(homeDir, ".linuxbrew", "bin", "brew"));

0 commit comments

Comments
 (0)