From 26d4c14f2c60be699cf257b308fc90bf781c0e02 Mon Sep 17 00:00:00 2001
From: Wibias <37517432+Wibias@users.noreply.github.com>
Date: Tue, 28 Jul 2026 07:53:44 +0200
Subject: [PATCH] fix(windows): grant owner ACE before ACL inheritance removal

/inheritance:r drops inherited ACEs immediately. Doing that before an explicit current-user Full Control grant left atomic-write temps with a protected zero-ACE DACL when a later icacls step timed out (#596).
---
 src/lib/windows-secret-acl.ts    | 30 +++++++++++------
 tests/windows-secret-acl.test.ts | 57 ++++++++++++++++++++++++++++++--
 2 files changed, 74 insertions(+), 13 deletions(-)

diff --git a/src/lib/windows-secret-acl.ts b/src/lib/windows-secret-acl.ts
index 4aae1da16..7cdc48a85 100644
--- a/src/lib/windows-secret-acl.ts
+++ b/src/lib/windows-secret-acl.ts
@@ -3,9 +3,14 @@
  *
  * On Windows, `chmod` only controls POSIX-style bits in the ACE list and does NOT remove
  * inherited permissions from other users. Real per-user isolation requires icacls to:
- *   1. Disable inheritance   (icacls path /inheritance:r)
- *   2. Strip broad explicit grants by SID (Everyone, Users, Authenticated Users)
- *   3. Grant the current user full control (icacls path /grant:r "CURRENTUSER:(F)")
+ *   1. Grant the current user full control (icacls path /grant:r "CURRENTUSER:(F)")
+ *   2. Disable inheritance               (icacls path /inheritance:r)
+ *   3. Strip broad explicit grants by SID (Everyone, Users, Authenticated Users)
+ *
+ * Owner grant MUST precede `/inheritance:r`. That flag is destructive: it drops inherited
+ * ACEs immediately. If a later step times out after inheritance is removed but before an
+ * explicit owner ACE exists, the temp is left with a protected empty DACL — owned by the
+ * current user yet unreadable/ununlinkable until Full Control is restored (issue #596).
  *
  * On non-Windows platforms the helpers fall through to the caller's existing chmod-based
  * behaviour: they return ok:true without invoking any external process.
@@ -141,8 +146,8 @@ function currentWindowsUser(): string | undefined {
 
 /**
  * Run icacls to harden a single file system entry.
- * - Disables inheritance (keeps nothing: /inheritance:r)
- * - Grants the current user Full Control
+ * Order is intentional (issue #596): grant the owner ACE first so a later
+ * `/inheritance:r` or `/remove:g` timeout cannot strand a protected zero-ACE DACL.
  *
  * We do NOT use a shell string; all arguments are passed as an array so no
  * shell injection is possible even for paths with unusual characters.
@@ -170,13 +175,20 @@ function runIcacls(targetPath: string, directory: boolean, deadline: number): vo
     if (!result.success) throw icaclsError(step, result);
   };
 
-  // Step 1: disable inheritance and remove inherited ACEs
+  // Step 1: grant current user full control BEFORE any destructive ACL change.
+  // If this fails, inheritance is untouched and the writer keeps inherited access.
+  const grant = directory ? `${user}:(OI)(CI)(F)` : `${user}:(F)`;
+  runOrThrow("/grant:r", [targetPath, "/grant:r", grant]);
+
+  // Step 2: disable inheritance and remove inherited ACEs. The explicit owner ACE
+  // from step 1 survives this transition, so a later failure still leaves cleanup access.
   runOrThrow("/inheritance:r", [targetPath, "/inheritance:r"]);
 
-  // Step 2: remove broad explicit grants using stable SIDs (not localized names).
+  // Step 3: remove broad explicit grants using stable SIDs (not localized names).
   // Missing ACEs can yield a non-zero exit; verify with locale-independent /findsid
   // before accepting the failure as harmless — a swallowed real failure would leave
   // Everyone/Users/Authenticated Users grants while reporting hardened.
+  // `/remove:g` cannot remove the explicit current-user ACE installed in step 1.
   const removal = run("/remove:g", [targetPath, "/remove:g", ...BROAD_SIDS]);
   if (!removal.success) {
     if (removal.timedOut) throw icaclsError("/remove:g", removal);
@@ -191,10 +203,6 @@ function runIcacls(targetPath: string, directory: boolean, deadline: number): vo
       }
     }
   }
-
-  // Step 3: grant current user full control.
-  const grant = directory ? `${user}:(OI)(CI)(F)` : `${user}:(F)`;
-  runOrThrow("/grant:r", [targetPath, "/grant:r", grant]);
 }
 
 /**
diff --git a/tests/windows-secret-acl.test.ts b/tests/windows-secret-acl.test.ts
index 0b124eb32..e3a19b1d8 100644
--- a/tests/windows-secret-acl.test.ts
+++ b/tests/windows-secret-acl.test.ts
@@ -411,14 +411,67 @@ describe("icacls failure paths (injected seams)", () => {
 
   test("a thrown EPERM error on a required path still fails closed (no retry)", () => {
     let calls = 0;
-    setIcaclsRunnerForTests(() => {
+    const steps: string[] = [];
+    setIcaclsRunnerForTests(args => {
       calls += 1;
+      if (args.includes("/grant:r")) steps.push("grant-owner");
+      else if (args.includes("/inheritance:r")) steps.push("remove-inheritance");
+      else if (args.includes("/remove:g")) steps.push("remove-broad");
+      else if (args.includes("/findsid")) steps.push("findsid");
+      else steps.push("other");
       const err = new Error("icacls denied") as NodeJS.ErrnoException;
       err.code = "EPERM";
       throw err;
     });
 
     expect(() => hardenSecretPath(secretFile(), { required: true })).toThrow(/permission denied/);
-    expect(calls).toBe(1); // real failures do not consume the timeout retry
+    // Grant runs first: a grant failure must not have already mutated inheritance (#596).
+    expect(calls).toBe(1);
+    expect(steps).toEqual(["grant-owner"]);
+  });
+
+  test("successful harden runs grant-owner before inheritance removal (#596)", () => {
+    const steps: string[] = [];
+    setIcaclsRunnerForTests(args => {
+      if (args.includes("/grant:r")) steps.push("grant-owner");
+      else if (args.includes("/inheritance:r")) steps.push("remove-inheritance");
+      else if (args.includes("/remove:g")) steps.push("remove-broad");
+      else if (args.includes("/findsid")) steps.push("findsid");
+      return ok;
+    });
+
+    expect(hardenSecretPath(secretFile(), { required: true })).toEqual({ ok: true });
+    expect(steps).toEqual(["grant-owner", "remove-inheritance", "remove-broad"]);
+  });
+
+  test("remove:g timeout after owner grant leaves explicit Full Control (#596)", () => {
+    // Models the production strand: inheritance already removed, then a later step
+    // times out. With owner-first ordering the writer still has an explicit ACE.
+    let ownerHasExplicitAce = false;
+    let inheritanceRemoved = false;
+    const timeoutOnRemove: IcaclsResult = {
+      success: false,
+      exitCode: null,
+      timedOut: true,
+      stdout: "",
+    };
+    setIcaclsRunnerForTests(args => {
+      if (args.includes("/grant:r")) {
+        ownerHasExplicitAce = true;
+        return ok;
+      }
+      if (args.includes("/inheritance:r")) {
+        inheritanceRemoved = true;
+        return ok;
+      }
+      if (args.includes("/remove:g")) return timeoutOnRemove;
+      return ok;
+    });
+
+    const result = hardenSecretPath(secretFile(), { required: true });
+    expect(result.ok).toBe(false);
+    expect(result.diagnostics).toContain("ETIMEDOUT");
+    expect(inheritanceRemoved).toBe(true);
+    expect(ownerHasExplicitAce).toBe(true);
   });
 });