refactor: simplify windows ACL parsing and expand coverage

src/security/windows-acl.test.ts

@@ -18,6 +18,22 @@ const {
   summarizeWindowsAcl,
 } = await import("./windows-acl.js");
 
+function aclEntry(params: {
+  principal: string;
+  rights?: string[];
+  rawRights?: string;
+  canRead?: boolean;
+  canWrite?: boolean;
+}): WindowsAclEntry {
+  return {
+    principal: params.principal,
+    rights: params.rights ?? ["F"],
+    rawRights: params.rawRights ?? "(F)",
+    canRead: params.canRead ?? true,
+    canWrite: params.canWrite ?? true,
+  };
+}
+
 describe("windows-acl", () => {
   describe("resolveWindowsUserPrincipal", () => {
     it("returns DOMAIN\\USERNAME when both are present", () => {
@@ -81,6 +97,7 @@ Successfully processed 1 files`;
 
     it("skips status messages", () => {
       const output = `Successfully processed 1 files
+                     Processed file: C:\\test\\file.txt
                      Failed processing 0 files
                      No mapping between account names`;
       const entries = parseIcaclsOutput(output, "C:\\test\\file.txt");
@@ -107,6 +124,14 @@ Successfully processed 1 files`;
       expect(entries[1].principal).toBe("S-1-5-21-1824257776-4070701511-781240313-1001");
     });
 
+    it("ignores malformed ACL lines that contain ':' but no rights tokens", () => {
+      const output = `C:\\test\\file.txt random:message
+                     C:\\test\\file.txt BUILTIN\\Administrators:(F)`;
+      const entries = parseIcaclsOutput(output, "C:\\test\\file.txt");
+      expect(entries).toHaveLength(1);
+      expect(entries[0].principal).toBe("BUILTIN\\Administrators");
+    });
+
     it("handles quoted target paths", () => {
       const output = `"C:\\path with spaces\\file.txt" BUILTIN\\Administrators:(F)`;
       const entries = parseIcaclsOutput(output, "C:\\path with spaces\\file.txt");
@@ -140,20 +165,8 @@ Successfully processed 1 files`;
   describe("summarizeWindowsAcl", () => {
     it("classifies trusted principals", () => {
       const entries: WindowsAclEntry[] = [
-        {
+        aclEntry({ principal: "NT AUTHORITY\\SYSTEM" }),
-          principal: "NT AUTHORITY\\SYSTEM",
+        aclEntry({ principal: "BUILTIN\\Administrators" }),
-          rights: ["F"],
-          rawRights: "(F)",
-          canRead: true,
-          canWrite: true,
-        },
-        {
-          principal: "BUILTIN\\Administrators",
-          rights: ["F"],
-          rawRights: "(F)",
-          canRead: true,
-          canWrite: true,
-        },
       ];
       const summary = summarizeWindowsAcl(entries);
       expect(summary.trusted).toHaveLength(2);
@@ -163,20 +176,8 @@ Successfully processed 1 files`;
 
     it("classifies world principals", () => {
       const entries: WindowsAclEntry[] = [
-        {
+        aclEntry({ principal: "Everyone", rights: ["R"], rawRights: "(R)", canWrite: false }),
-          principal: "Everyone",
+        aclEntry({ principal: "BUILTIN\\Users", rights: ["R"], rawRights: "(R)", canWrite: false }),
-          rights: ["R"],
-          rawRights: "(R)",
-          canRead: true,
-          canWrite: false,
-        },
-        {
-          principal: "BUILTIN\\Users",
-          rights: ["R"],
-          rawRights: "(R)",
-          canRead: true,
-          canWrite: false,
-        },
       ];
       const summary = summarizeWindowsAcl(entries);
       expect(summary.trusted).toHaveLength(0);
@@ -185,15 +186,7 @@ Successfully processed 1 files`;
     });
 
     it("classifies current user as trusted", () => {
-      const entries: WindowsAclEntry[] = [
+      const entries: WindowsAclEntry[] = [aclEntry({ principal: "WORKGROUP\\TestUser" })];
-        {
-          principal: "WORKGROUP\\TestUser",
-          rights: ["F"],
-          rawRights: "(F)",
-          canRead: true,
-          canWrite: true,
-        },
-      ];
       const env = { USERNAME: "TestUser", USERDOMAIN: "WORKGROUP" };
       const summary = summarizeWindowsAcl(entries, env);
       expect(summary.trusted).toHaveLength(1);
@@ -217,15 +210,7 @@ Successfully processed 1 files`;
 
   describe("summarizeWindowsAcl — SID-based classification", () => {
     it("classifies SYSTEM SID (S-1-5-18) as trusted", () => {
-      const entries: WindowsAclEntry[] = [
+      const entries: WindowsAclEntry[] = [aclEntry({ principal: "S-1-5-18" })];
-        {
-          principal: "S-1-5-18",
-          rights: ["F"],
-          rawRights: "(F)",
-          canRead: true,
-          canWrite: true,
-        },
-      ];
       const summary = summarizeWindowsAcl(entries);
       expect(summary.trusted).toHaveLength(1);
       expect(summary.untrustedWorld).toHaveLength(0);
@@ -233,15 +218,7 @@ Successfully processed 1 files`;
     });
 
     it("classifies BUILTIN\\Administrators SID (S-1-5-32-544) as trusted", () => {
-      const entries: WindowsAclEntry[] = [
+      const entries: WindowsAclEntry[] = [aclEntry({ principal: "S-1-5-32-544" })];
-        {
-          principal: "S-1-5-32-544",
-          rights: ["F"],
-          rawRights: "(F)",
-          canRead: true,
-          canWrite: true,
-        },
-      ];
       const summary = summarizeWindowsAcl(entries);
       expect(summary.trusted).toHaveLength(1);
       expect(summary.untrustedGroup).toHaveLength(0);
@@ -249,21 +226,23 @@ Successfully processed 1 files`;
 
     it("classifies caller SID from USERSID env var as trusted", () => {
       const callerSid = "S-1-5-21-1824257776-4070701511-781240313-1001";
-      const entries: WindowsAclEntry[] = [
+      const entries: WindowsAclEntry[] = [aclEntry({ principal: callerSid })];
-        {
-          principal: callerSid,
-          rights: ["F"],
-          rawRights: "(F)",
-          canRead: true,
-          canWrite: true,
-        },
-      ];
       const env = { USERSID: callerSid };
       const summary = summarizeWindowsAcl(entries, env);
       expect(summary.trusted).toHaveLength(1);
       expect(summary.untrustedGroup).toHaveLength(0);
     });
 
+    it("matches SIDs case-insensitively and trims USERSID", () => {
+      const entries: WindowsAclEntry[] = [
+        aclEntry({ principal: "s-1-5-21-1824257776-4070701511-781240313-1001" }),
+      ];
+      const env = { USERSID: "  S-1-5-21-1824257776-4070701511-781240313-1001  " };
+      const summary = summarizeWindowsAcl(entries, env);
+      expect(summary.trusted).toHaveLength(1);
+      expect(summary.untrustedGroup).toHaveLength(0);
+    });
+
     it("classifies unknown SID as group (not world)", () => {
       const entries: WindowsAclEntry[] = [
         {

src/security/windows-acl.ts

@@ -43,6 +43,12 @@ const TRUSTED_SIDS = new Set([
   "s-1-5-32-544",
   "s-1-5-80-956008885-3418522649-1831038044-1853292631-2271478464",
 ]);
+const STATUS_PREFIXES = [
+  "successfully processed",
+  "processed",
+  "failed processing",
+  "no mapping between account names",
+];
 
 const normalize = (value: string) => value.trim().toLowerCase();
 
@@ -66,7 +72,7 @@ function buildTrustedPrincipals(env?: NodeJS.ProcessEnv): Set<string> {
       trusted.add(normalize(userOnly));
     }
   }
-  const userSid = env?.USERSID?.trim().toLowerCase();
+  const userSid = normalize(env?.USERSID ?? "");
   if (userSid && SID_RE.test(userSid)) {
     trusted.add(userSid);
   }
@@ -75,19 +81,24 @@ function buildTrustedPrincipals(env?: NodeJS.ProcessEnv): Set<string> {
 
 function classifyPrincipal(
   principal: string,
-  env?: NodeJS.ProcessEnv,
+  trustedPrincipals: Set<string>,
 ): "trusted" | "world" | "group" {
   const normalized = normalize(principal);
-  const trusted = buildTrustedPrincipals(env);
 
   if (SID_RE.test(normalized)) {
-    return TRUSTED_SIDS.has(normalized) || trusted.has(normalized) ? "trusted" : "group";
+    return TRUSTED_SIDS.has(normalized) || trustedPrincipals.has(normalized) ? "trusted" : "group";
   }
 
-  if (trusted.has(normalized) || TRUSTED_SUFFIXES.some((s) => normalized.endsWith(s))) {
+  if (
+    trustedPrincipals.has(normalized) ||
+    TRUSTED_SUFFIXES.some((suffix) => normalized.endsWith(suffix))
+  ) {
     return "trusted";
   }
-  if (WORLD_PRINCIPALS.has(normalized) || WORLD_SUFFIXES.some((s) => normalized.endsWith(s))) {
+  if (
+    WORLD_PRINCIPALS.has(normalized) ||
+    WORLD_SUFFIXES.some((suffix) => normalized.endsWith(suffix))
+  ) {
     return "world";
   }
   return "group";
@@ -101,6 +112,58 @@ function rightsFromTokens(tokens: string[]): { canRead: boolean; canWrite: boole
   return { canRead, canWrite };
 }
 
+function isStatusLine(lowerLine: string): boolean {
+  return STATUS_PREFIXES.some((prefix) => lowerLine.startsWith(prefix));
+}
+
+function stripTargetPrefix(params: {
+  trimmedLine: string;
+  lowerLine: string;
+  normalizedTarget: string;
+  lowerTarget: string;
+  quotedTarget: string;
+  quotedLower: string;
+}): string {
+  if (params.lowerLine.startsWith(params.lowerTarget)) {
+    return params.trimmedLine.slice(params.normalizedTarget.length).trim();
+  }
+  if (params.lowerLine.startsWith(params.quotedLower)) {
+    return params.trimmedLine.slice(params.quotedTarget.length).trim();
+  }
+  return params.trimmedLine;
+}
+
+function parseAceEntry(entry: string): WindowsAclEntry | null {
+  if (!entry || !entry.includes("(")) {
+    return null;
+  }
+
+  const idx = entry.indexOf(":");
+  if (idx === -1) {
+    return null;
+  }
+
+  const principal = entry.slice(0, idx).trim();
+  const rawRights = entry.slice(idx + 1).trim();
+  const tokens =
+    rawRights
+      .match(/\(([^)]+)\)/g)
+      ?.map((token) => token.slice(1, -1).trim())
+      .filter(Boolean) ?? [];
+
+  if (tokens.some((token) => token.toUpperCase() === "DENY")) {
+    return null;
+  }
+
+  const rights = tokens.filter((token) => !INHERIT_FLAGS.has(token.toUpperCase()));
+  if (rights.length === 0) {
+    return null;
+  }
+
+  const { canRead, canWrite } = rightsFromTokens(rights);
+  return { principal, rights, rawRights, canRead, canWrite };
+}
+
 export function parseIcaclsOutput(output: string, targetPath: string): WindowsAclEntry[] {
   const entries: WindowsAclEntry[] = [];
   const normalizedTarget = targetPath.trim();
@@ -115,50 +178,23 @@ export function parseIcaclsOutput(output: string, targetPath: string): WindowsAc
     }
     const trimmed = line.trim();
     const lower = trimmed.toLowerCase();
-    if (
+    if (isStatusLine(lower)) {
-      lower.startsWith("successfully processed") ||
-      lower.startsWith("processed") ||
-      lower.startsWith("failed processing") ||
-      lower.startsWith("no mapping between account names")
-    ) {
       continue;
     }
 
-    let entry = trimmed;
+    const entry = stripTargetPrefix({
-    if (lower.startsWith(lowerTarget)) {
+      trimmedLine: trimmed,
-      entry = trimmed.slice(normalizedTarget.length).trim();
+      lowerLine: lower,
-    } else if (lower.startsWith(quotedLower)) {
+      normalizedTarget,
-      entry = trimmed.slice(quotedTarget.length).trim();
+      lowerTarget,
-    }
+      quotedTarget,
-    if (!entry) {
+      quotedLower,
+    });
+    const parsed = parseAceEntry(entry);
+    if (!parsed) {
       continue;
     }
-
+    entries.push(parsed);
-    if (!entry.includes("(")) {
-      continue;
-    }
-
-    const idx = entry.indexOf(":");
-    if (idx === -1) {
-      continue;
-    }
-
-    const principal = entry.slice(0, idx).trim();
-    const rawRights = entry.slice(idx + 1).trim();
-    const tokens =
-      rawRights
-        .match(/\(([^)]+)\)/g)
-        ?.map((token) => token.slice(1, -1).trim())
-        .filter(Boolean) ?? [];
-    if (tokens.some((token) => token.toUpperCase() === "DENY")) {
-      continue;
-    }
-    const rights = tokens.filter((token) => !INHERIT_FLAGS.has(token.toUpperCase()));
-    if (rights.length === 0) {
-      continue;
-    }
-    const { canRead, canWrite } = rightsFromTokens(rights);
-    entries.push({ principal, rights, rawRights, canRead, canWrite });
   }
 
   return entries;
@@ -168,11 +204,12 @@ export function summarizeWindowsAcl(
   entries: WindowsAclEntry[],
   env?: NodeJS.ProcessEnv,
 ): Pick<WindowsAclSummary, "trusted" | "untrustedWorld" | "untrustedGroup"> {
+  const trustedPrincipals = buildTrustedPrincipals(env);
   const trusted: WindowsAclEntry[] = [];
   const untrustedWorld: WindowsAclEntry[] = [];
   const untrustedGroup: WindowsAclEntry[] = [];
   for (const entry of entries) {
-    const classification = classifyPrincipal(entry.principal, env);
+    const classification = classifyPrincipal(entry.principal, trustedPrincipals);
     if (classification === "trusted") {
       trusted.push(entry);
     } else if (classification === "world") {