Skip to content

Commit cba0a34

Browse files
pgondhi987pgondhi987
authored
fix(infra): block Windows system path env vars from workspace .env injection (#74456)
* fix: address issue * fix: address PR review feedback * fix: address codex review feedback * fix: address codex review feedback * fix: address codex review feedback * docs: add changelog entry for PR merge * Update CHANGELOG.md
1 parent 3b75898 commit cba0a34

8 files changed

Lines changed: 112 additions & 6 deletions

‎CHANGELOG.md‎

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

2525
- fix(infra): block ambient Homebrew env vars from brew resolution. (#74463) Thanks @pgondhi987.
2626
- 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.
27+
- Security/Windows: ignore workspace `.env` system-path variables and resolve stale-process `taskkill.exe` from the validated Windows install root, preventing repository-local env files from redirecting cleanup helpers. Thanks @pgondhi987.
2728
- 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.
2829
- 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.
2930
- Voice Call/Twilio: send notify-mode initial TwiML directly in the outbound create-call request while keeping conversation and pre-connect DTMF calls webhook-driven, so one-shot notify calls do not depend on a first-answer webhook fetch. Supersedes #72758. Thanks @tyshepps.

‎src/agents/pi-project-settings.test.ts‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,13 @@
1+
import fs from "node:fs/promises";
2+
import os from "node:os";
3+
import path from "node:path";
14
import { describe, expect, it } from "vitest";
25
import {
36
buildEmbeddedPiSettingsSnapshot,
47
DEFAULT_EMBEDDED_PI_PROJECT_SETTINGS_POLICY,
58
resolveEmbeddedPiProjectSettingsPolicy,
69
} from "./pi-project-settings-snapshot.js";
10+
import { createPreparedEmbeddedPiSettingsManager } from "./pi-project-settings.js";
711

812
type EmbeddedPiSettingsArgs = Parameters<typeof buildEmbeddedPiSettingsSnapshot>[0];
913

@@ -126,3 +130,48 @@ describe("buildEmbeddedPiSettingsSnapshot", () => {
126130
});
127131
});
128132
});
133+
134+
describe("createPreparedEmbeddedPiSettingsManager", () => {
135+
it("keeps trusted file-backed settings runtime-scoped after preparation", async () => {
136+
const baseDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-pi-settings-"));
137+
try {
138+
const cwd = path.join(baseDir, "workspace");
139+
const agentDir = path.join(baseDir, "agent");
140+
const projectSettingsDir = path.join(cwd, ".pi");
141+
const agentSettingsPath = path.join(agentDir, "settings.json");
142+
await fs.mkdir(projectSettingsDir, { recursive: true });
143+
await fs.mkdir(agentDir, { recursive: true });
144+
await fs.writeFile(
145+
agentSettingsPath,
146+
JSON.stringify({ retry: { enabled: true } }, null, 2),
147+
"utf8",
148+
);
149+
await fs.writeFile(
150+
path.join(projectSettingsDir, "settings.json"),
151+
JSON.stringify({ shellCommandPrefix: "echo trusted &&" }, null, 2),
152+
"utf8",
153+
);
154+
155+
const settingsManager = createPreparedEmbeddedPiSettingsManager({
156+
cwd,
157+
agentDir,
158+
cfg: {
159+
agents: { defaults: { embeddedPi: { projectSettingsPolicy: "trusted" } } },
160+
},
161+
});
162+
163+
expect(settingsManager.getShellCommandPrefix()).toBe("echo trusted &&");
164+
expect(settingsManager.getRetryEnabled()).toBe(true);
165+
166+
settingsManager.setRetryEnabled(false);
167+
await settingsManager.flush();
168+
169+
const diskSettings = JSON.parse(await fs.readFile(agentSettingsPath, "utf8")) as {
170+
retry?: { enabled?: boolean };
171+
};
172+
expect(diskSettings.retry?.enabled).toBe(true);
173+
} finally {
174+
await fs.rm(baseDir, { recursive: true, force: true });
175+
}
176+
});
177+
});

‎src/agents/pi-project-settings.ts‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,14 +37,27 @@ export function createEmbeddedPiSettingsManager(params: {
3737
return SettingsManager.inMemory(settings);
3838
}
3939

40+
function createRuntimeEmbeddedPiSettingsManager(settingsManager: SettingsManager): SettingsManager {
41+
return SettingsManager.inMemory(
42+
buildEmbeddedPiSettingsSnapshot({
43+
globalSettings: settingsManager.getGlobalSettings(),
44+
pluginSettings: {},
45+
projectSettings: settingsManager.getProjectSettings(),
46+
policy: "trusted",
47+
}),
48+
);
49+
}
50+
4051
export function createPreparedEmbeddedPiSettingsManager(params: {
4152
cwd: string;
4253
agentDir: string;
4354
cfg?: OpenClawConfig;
4455
/** Resolved context window budget so reserve-token floor can be capped for small models. */
4556
contextTokenBudget?: number;
4657
}): SettingsManager {
47-
const settingsManager = createEmbeddedPiSettingsManager(params);
58+
const settingsManager = createRuntimeEmbeddedPiSettingsManager(
59+
createEmbeddedPiSettingsManager(params),
60+
);
4861
applyPiCompactionSettingsFromConfig({
4962
settingsManager,
5063
cfg: params.cfg,

‎src/infra/brew.test.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,18 @@ describe("brew helpers", () => {
139139
});
140140
});
141141

142+
it("does not resolve brew from PATH entries", async () => {
143+
await withTempDir({ prefix: "openclaw-brew-" }, async (tmp) => {
144+
const pathBin = path.join(tmp, "path-bin");
145+
const pathBrew = path.join(pathBin, "brew");
146+
await writeExecutable(pathBrew);
147+
148+
const env: NodeJS.ProcessEnv = { PATH: pathBin };
149+
150+
expect(resolveBrewExecutable({ homeDir: path.join(tmp, "home"), env })).not.toBe(pathBrew);
151+
});
152+
});
153+
142154
it("always includes the standard macOS brew dirs after linuxbrew candidates", () => {
143155
const dirs = resolveBrewPathDirs({ homeDir: "/home/test" });
144156

‎src/infra/dotenv.test.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -680,6 +680,14 @@ describe("workspace .env blocklist completeness", () => {
680680
"OPENCLAW_NODE_EXEC_HOST",
681681
"OPENCLAW_NODE_EXEC_FALLBACK",
682682
"OPENCLAW_ALLOW_PROJECT_LOCAL_BIN",
683+
"PATH",
684+
"HOMEBREW_BREW_FILE",
685+
"HOMEBREW_PREFIX",
686+
"SystemRoot",
687+
"WINDIR",
688+
"ProgramFiles",
689+
"ProgramFiles(x86)",
690+
"ProgramW6432",
683691
"SYNOLOGY_CHAT_INCOMING_URL",
684692
"SYNOLOGY_NAS_HOST",
685693
];

‎src/infra/dotenv.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,10 +74,16 @@ const BLOCKED_WORKSPACE_DOTENV_KEYS = new Set([
7474
"OPENCLAW_STATE_DIR",
7575
"OPENCLAW_TEST_TAILSCALE_BINARY",
7676
"PI_CODING_AGENT_DIR",
77+
"PATH",
7778
"PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH",
79+
"PROGRAMFILES",
80+
"PROGRAMFILES(X86)",
81+
"PROGRAMW6432",
7882
"SYNOLOGY_CHAT_INCOMING_URL",
7983
"SYNOLOGY_NAS_HOST",
84+
"SYSTEMROOT",
8085
"UV_PYTHON",
86+
"WINDIR",
8187
]);
8288

8389
// Block endpoint redirection for any service without overfitting per-provider names.

‎src/infra/restart-stale-pids.test.ts‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,15 @@ vi.mock("./windows-port-pids.js", () => ({
9090
mockReadWindowsProcessArgsResult(pid, timeoutMs),
9191
}));
9292

93+
vi.mock("./windows-install-roots.js", () => ({
94+
getWindowsInstallRoots: () => ({
95+
systemRoot: "C:\\Windows",
96+
programFiles: "C:\\Program Files",
97+
programFilesX86: "C:\\Program Files (x86)",
98+
programW6432: null,
99+
}),
100+
}));
101+
93102
import { resolveLsofCommandSync } from "./ports-lsof.js";
94103
let __testing: typeof import("./restart-stale-pids.js").__testing;
95104
let cleanStaleGatewayProcessesSync: typeof import("./restart-stale-pids.js").cleanStaleGatewayProcessesSync;
@@ -980,8 +989,10 @@ describe.skipIf(isWindows)("restart-stale-pids", () => {
980989

981990
it("does not report Windows pids as killed when taskkill fails", () => {
982991
const origDescriptor = Object.getOwnPropertyDescriptor(process, "platform");
992+
const originalSystemRoot = process.env.SystemRoot;
983993
const stalePid = process.pid + 911;
984994
Object.defineProperty(process, "platform", { value: "win32", configurable: true });
995+
process.env.SystemRoot = "C:\\PoisonedWindows";
985996
try {
986997
let fakeNow = 0;
987998
__testing.setDateNowOverride(() => fakeNow);
@@ -1012,12 +1023,17 @@ describe.skipIf(isWindows)("restart-stale-pids", () => {
10121023

10131024
expect(cleanStaleGatewayProcessesSync()).toEqual([]);
10141025
expect(mockSpawnSync).toHaveBeenCalledWith(
1015-
expect.stringContaining("taskkill.exe"),
1026+
"C:\\Windows\\System32\\taskkill.exe",
10161027
["/T", "/PID", String(stalePid)],
10171028
expect.objectContaining({ timeout: 5000 }),
10181029
);
10191030
} finally {
10201031
__testing.setDateNowOverride(null);
1032+
if (originalSystemRoot === undefined) {
1033+
delete process.env.SystemRoot;
1034+
} else {
1035+
process.env.SystemRoot = originalSystemRoot;
1036+
}
10211037
if (origDescriptor) {
10221038
Object.defineProperty(process, "platform", origDescriptor);
10231039
}
@@ -1063,13 +1079,13 @@ describe.skipIf(isWindows)("restart-stale-pids", () => {
10631079
expect(cleanStaleGatewayProcessesSync()).toEqual([]);
10641080
expect(mockSpawnSync).toHaveBeenNthCalledWith(
10651081
1,
1066-
expect.stringContaining("taskkill.exe"),
1082+
"C:\\Windows\\System32\\taskkill.exe",
10671083
["/T", "/PID", String(stalePid)],
10681084
expect.objectContaining({ timeout: 5000 }),
10691085
);
10701086
expect(mockSpawnSync).toHaveBeenNthCalledWith(
10711087
2,
1072-
expect.stringContaining("taskkill.exe"),
1088+
"C:\\Windows\\System32\\taskkill.exe",
10731089
["/F", "/T", "/PID", String(stalePid)],
10741090
expect.objectContaining({ timeout: 5000 }),
10751091
);

‎src/infra/restart-stale-pids.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { createSubsystemLogger } from "../logging/subsystem.js";
66
import { normalizeLowercaseStringOrEmpty } from "../shared/string-coerce.js";
77
import { isGatewayArgv } from "./gateway-process-argv.js";
88
import { resolveLsofCommandSync } from "./ports-lsof.js";
9+
import { getWindowsInstallRoots } from "./windows-install-roots.js";
910
import {
1011
readWindowsListeningPidsOnPortSync,
1112
readWindowsListeningPidsResultSync,
@@ -424,8 +425,8 @@ function terminateStaleProcessesSync(pids: number[]): number[] {
424425
* Sends a graceful taskkill first (/T for tree), waits, then escalates to /F.
425426
*/
426427
function terminateStaleProcessesWindows(pids: number[]): number[] {
427-
const taskkillPath = path.join(
428-
process.env.SystemRoot ?? "C:\\Windows",
428+
const taskkillPath = path.win32.join(
429+
getWindowsInstallRoots().systemRoot,
429430
"System32",
430431
"taskkill.exe",
431432
);

0 commit comments

Comments
 (0)