Skip to content

Commit b3aa616

Browse files
mmapsmmaps
committed
fix: address issue
1 parent 240362b commit b3aa616

4 files changed

Lines changed: 89 additions & 14 deletions

File tree

‎src/infra/dotenv.test.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -284,6 +284,23 @@ describe("loadDotEnv", () => {
284284
});
285285
});
286286

287+
it.each(["systemroot", "SystemRoot", "SYSTEMROOT", "windir", "WINDIR"])(
288+
"blocks %s from workspace .env",
289+
async (key) => {
290+
await withIsolatedEnvAndCwd(async () => {
291+
await withDotEnvFixture(async ({ cwdDir }) => {
292+
await writeEnvFile(path.join(cwdDir, ".env"), `${key}=.\\fake-root\n`);
293+
294+
delete process.env[key];
295+
296+
loadWorkspaceDotEnvFile(path.join(cwdDir, ".env"), { quiet: true });
297+
298+
expect(process.env[key]).toBeUndefined();
299+
});
300+
});
301+
},
302+
);
303+
287304
it("blocks path-override vars (OPENCLAW_AGENT_DIR, OPENCLAW_BUNDLED_PLUGINS_DIR, PI_CODING_AGENT_DIR, OPENCLAW_OAUTH_DIR) from workspace .env", async () => {
288305
await withIsolatedEnvAndCwd(async () => {
289306
await withDotEnvFixture(async ({ base, cwdDir }) => {

‎src/infra/dotenv.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,10 @@ const BLOCKED_WORKSPACE_DOTENV_KEYS = new Set([
7272
"PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH",
7373
"SYNOLOGY_CHAT_INCOMING_URL",
7474
"SYNOLOGY_NAS_HOST",
75+
// Windows helper roots affect command resolution for audit/repair flows.
76+
"SYSTEMROOT",
7577
"UV_PYTHON",
78+
"WINDIR",
7679
]);
7780

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

‎src/security/windows-acl.test.ts‎

Lines changed: 59 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ import type { WindowsAclEntry, WindowsAclSummary } from "./windows-acl.js";
44
const MOCK_USERNAME = "MockUser";
55
const mockUserInfo = () => ({ username: MOCK_USERNAME });
66
const emptyUserInfo = () => ({ username: "" });
7+
const DEFAULT_ICACLS = "C:\\Windows\\System32\\icacls.exe";
8+
const DEFAULT_WHOAMI = "C:\\Windows\\System32\\whoami.exe";
79

810
let createIcaclsResetCommand: typeof import("./windows-acl.js").createIcaclsResetCommand;
911
let formatIcaclsResetCommand: typeof import("./windows-acl.js").formatIcaclsResetCommand;
@@ -424,7 +426,7 @@ Successfully processed 1 files`;
424426
expectInspectSuccess(result, 2);
425427
// /sid is passed so that account names are printed as SIDs, making the
426428
// audit locale-independent (fixes #35834).
427-
expect(mockExec).toHaveBeenCalledWith("icacls.exe", ["C:\\test\\file.txt", "/sid"]);
429+
expect(mockExec).toHaveBeenCalledWith(DEFAULT_ICACLS, ["C:\\test\\file.txt", "/sid"]);
428430
});
429431

430432
it("classifies *S-1-5-18 (SID form of SYSTEM from /sid) as trusted", async () => {
@@ -469,8 +471,8 @@ Successfully processed 1 files`;
469471
expectInspectSuccess(result, 2);
470472
expect(result.trusted).toHaveLength(2);
471473
expect(result.untrustedGroup).toHaveLength(0);
472-
expect(mockExec).toHaveBeenNthCalledWith(1, "icacls.exe", ["C:\\test\\file.txt", "/sid"]);
473-
expect(mockExec).toHaveBeenNthCalledWith(2, "whoami.exe", ["/user", "/fo", "csv", "/nh"]);
474+
expect(mockExec).toHaveBeenNthCalledWith(1, DEFAULT_ICACLS, ["C:\\test\\file.txt", "/sid"]);
475+
expect(mockExec).toHaveBeenNthCalledWith(2, DEFAULT_WHOAMI, ["/user", "/fo", "csv", "/nh"]);
474476
});
475477

476478
it("returns error state on exec failure", async () => {
@@ -533,21 +535,61 @@ Successfully processed 1 files`;
533535

534536
const result = await inspectWindowsAcl("C:\\test\\file.txt", {
535537
exec: mockExec,
536-
env: { SystemRoot: "C:\\Windows" },
538+
env: { SystemRoot: "D:\\Windows" },
537539
});
538540

539541
expectInspectSuccess(result, 1);
540-
expect(mockExec).toHaveBeenNthCalledWith(1, "C:\\Windows\\System32\\icacls.exe", [
542+
expect(mockExec).toHaveBeenNthCalledWith(1, "D:\\Windows\\System32\\icacls.exe", [
541543
"C:\\test\\file.txt",
542544
"/sid",
543545
]);
544-
expect(mockExec).toHaveBeenNthCalledWith(2, "C:\\Windows\\System32\\whoami.exe", [
546+
expect(mockExec).toHaveBeenNthCalledWith(2, "D:\\Windows\\System32\\whoami.exe", [
545547
"/user",
546548
"/fo",
547549
"csv",
548550
"/nh",
549551
]);
550552
});
553+
554+
it("does not resolve Windows system commands through a relative SystemRoot", async () => {
555+
const mockExec = vi
556+
.fn()
557+
.mockResolvedValueOnce({
558+
stdout: "C:\\test\\file.txt *S-1-5-21-111-222-333-1001:(F)",
559+
stderr: "",
560+
})
561+
.mockResolvedValueOnce({
562+
stdout: '"mock-host\\\\MockUser","S-1-5-21-111-222-333-1001"\r\n',
563+
stderr: "",
564+
});
565+
566+
const result = await inspectWindowsAcl("C:\\test\\file.txt", {
567+
exec: mockExec,
568+
env: { SystemRoot: ".\\fake-root" },
569+
});
570+
571+
expectInspectSuccess(result, 1);
572+
expect(mockExec).toHaveBeenNthCalledWith(1, DEFAULT_ICACLS, ["C:\\test\\file.txt", "/sid"]);
573+
expect(mockExec).toHaveBeenNthCalledWith(2, DEFAULT_WHOAMI, ["/user", "/fo", "csv", "/nh"]);
574+
});
575+
576+
it("uses a valid WINDIR when SystemRoot is invalid", async () => {
577+
const mockExec = vi.fn().mockResolvedValueOnce({
578+
stdout: "C:\\test\\file.txt *S-1-5-18:(F)",
579+
stderr: "",
580+
});
581+
582+
const result = await inspectWindowsAcl("C:\\test\\file.txt", {
583+
exec: mockExec,
584+
env: { SystemRoot: ".\\fake-root", WINDIR: "E:\\Windows" },
585+
});
586+
587+
expectInspectSuccess(result, 1);
588+
expect(mockExec).toHaveBeenCalledWith("E:\\Windows\\System32\\icacls.exe", [
589+
"C:\\test\\file.txt",
590+
"/sid",
591+
]);
592+
});
551593
});
552594

553595
describe("formatWindowsAclSummary", () => {
@@ -653,11 +695,20 @@ Successfully processed 1 files`;
653695
env,
654696
});
655697
expect(result).not.toBeNull();
656-
expect(result?.command).toBe("icacls");
698+
expect(result?.command).toBe(DEFAULT_ICACLS);
657699
expect(result?.args).toContain("C:\\test\\file.txt");
658700
expect(result?.args).toContain("/inheritance:r");
659701
});
660702

703+
it("uses a validated SystemRoot for the structured command executable", () => {
704+
const result = createIcaclsResetCommand("C:\\test\\file.txt", {
705+
isDir: false,
706+
env: { SystemRoot: "D:\\Windows", USERNAME: "TestUser" },
707+
});
708+
709+
expect(result?.command).toBe("D:\\Windows\\System32\\icacls.exe");
710+
});
711+
661712
it("returns command with system username when env is empty (falls back to os.userInfo)", () => {
662713
// When env is empty, resolveWindowsUserPrincipal falls back to os.userInfo().username
663714
const result = createIcaclsResetCommand("C:\\test\\file.txt", {
@@ -667,7 +718,7 @@ Successfully processed 1 files`;
667718
});
668719
// Should return a valid command using the system username
669720
expect(result).not.toBeNull();
670-
expect(result?.command).toBe("icacls");
721+
expect(result?.command).toBe(DEFAULT_ICACLS);
671722
expect(result?.args).toContain(`${MOCK_USERNAME}:F`);
672723
});
673724

‎src/security/windows-acl.ts‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import os from "node:os";
22
import path from "node:path";
3+
import { normalizeWindowsInstallRoot } from "../infra/windows-install-roots.js";
34
import { runExec } from "../process/exec.js";
45
import { normalizeLowercaseStringOrEmpty } from "../shared/string-coerce.js";
56

@@ -51,6 +52,7 @@ const TRUSTED_BASE = new Set([
5152
]);
5253
const WORLD_SUFFIXES = ["\\users", "\\authenticated users"];
5354
const TRUSTED_SUFFIXES = ["\\administrators", "\\system", "\\système"];
55+
const DEFAULT_WINDOWS_SYSTEM_ROOT = "C:\\Windows";
5456

5557
// Accept an optional leading * which icacls prefixes to SIDs when invoked with /sid
5658
// (e.g. *S-1-5-18 instead of S-1-5-18).
@@ -115,11 +117,13 @@ function buildTrustedPrincipals(env?: NodeJS.ProcessEnv): Set<string> {
115117

116118
function resolveWindowsSystemCommand(command: string, env?: NodeJS.ProcessEnv): string {
117119
const root =
118-
env?.SystemRoot?.trim() ||
119-
env?.SYSTEMROOT?.trim() ||
120-
env?.windir?.trim() ||
121-
env?.WINDIR?.trim();
122-
return root ? path.win32.join(root, "System32", command) : command;
120+
[
121+
normalizeWindowsInstallRoot(env?.SystemRoot),
122+
normalizeWindowsInstallRoot(env?.SYSTEMROOT),
123+
normalizeWindowsInstallRoot(env?.windir),
124+
normalizeWindowsInstallRoot(env?.WINDIR),
125+
].find((candidate): candidate is string => candidate !== null) ?? DEFAULT_WINDOWS_SYSTEM_ROOT;
126+
return path.win32.join(root, "System32", command);
123127
}
124128

125129
function classifyPrincipal(
@@ -398,7 +402,7 @@ export function createIcaclsResetCommand(
398402
`*S-1-5-18:${grant}`,
399403
];
400404
return {
401-
command: "icacls",
405+
command: resolveWindowsSystemCommand("icacls.exe", opts.env),
402406
args,
403407
display: formatIcaclsResetCommand(targetPath, opts),
404408
};

0 commit comments

Comments
 (0)