fix(security): apply the settings ACL for real and assert it

`restrictWindowsAcl` passed the program name as the first argument, so icacls
received two operands ("icacls" and the target) and exited with
ERROR_INVALID_PARAMETER (87).  The failure was swallowed as best-effort, so the
ACL was never touched while the caller kept going: on Windows the settings file
(which stores the API key) stayed readable by whatever the parent directory
grants, and the 0600 intent was silently unmet.

- drop the duplicated "icacls" operand
- resolve the principal from USERDOMAIN/USERNAME before falling back to whoami,
  which under MSYS/Git-Bash answers with a bare, ambiguous name
- return whether the ACL is now user-only instead of failing invisibly
- tests: assert that no inherited ACE survives (this is the assertion that
  catches the no-op), read the exact principal back instead of matching a loose
  pattern, and skip loudly when icacls cannot run

Verified on Windows: the file and its directory now carry a single explicit
ACE for the current user, with no Authenticated Users/Everyone entry.

Author: raymondginger2018-sudo

packages/core/src/common/private-storage.ts

@@ -21,12 +21,21 @@ export const PRIVATE_DIRECTORY_MODE = 0o700;
 /**
  * Resolve the current Windows user as a fully-qualified principal
  * (``DOMAIN\user``) so ACL grants are unambiguous across machines/domains.
+ *
+ * ``USERDOMAIN``/``USERNAME`` win when present: they are set by Windows for
+ * every process, whereas under MSYS/Git-Bash a POSIX ``whoami`` earlier in
+ * PATH answers with a bare, ambiguous name.
+ *
  * Returns null when the identity cannot be resolved (callers no-op).
  */
 export function windowsIdentity(): string | null {
   if (process.platform !== "win32") {
     return null;
   }
+  const { USERDOMAIN, USERNAME } = process.env;
+  if (USERDOMAIN && USERNAME) {
+    return `${USERDOMAIN}\\${USERNAME}`;
+  }
   try {
     const stdout = execFileSync("whoami", {
       encoding: "utf8",
@@ -51,20 +60,24 @@ export function windowsIdentity(): string | null {
  *  2. ``icacls /grant:r <user>:F`` grants the current user exclusive full
  *     control (``:r`` replaces, does not append).
  *
- * Failures are swallowed (best-effort, like POSIX chmod) — the file is still
- * created; only its ACL may be more permissive than intended.
+ * Returns whether the ACL now grants the current user alone.  Failures are
+ * reported, never thrown: callers decide whether an unprotected path is
+ * acceptable.  Note the argv must not repeat the program name — passing
+ * ``icacls icacls <path>`` makes icacls exit with ERROR_INVALID_PARAMETER (87)
+ * and leave the ACL untouched, which is how this helper silently no-opped
+ * before.
  */
-export function restrictWindowsAcl(targetPath: string): void {
+export function restrictWindowsAcl(targetPath: string): boolean {
   if (process.platform !== "win32") {
-    return;
+    return true;
   }
   const identity = windowsIdentity();
   if (!identity) {
-    return;
+    return false;
   }
   for (const args of [
-    ["icacls", targetPath, "/inheritance:r"],
-    ["icacls", targetPath, "/grant:r", `${identity}:F`],
+    [targetPath, "/inheritance:r"],
+    [targetPath, "/grant:r", `${identity}:F`],
   ]) {
     try {
       execFileSync("icacls", args, {
@@ -74,34 +87,33 @@ export function restrictWindowsAcl(targetPath: string): void {
         stdio: ["ignore", "pipe", "ignore"],
       });
     } catch {
-      return; // best-effort; keep the caller moving
+      return false; // best-effort: keep the caller moving, but do not pretend
     }
   }
+  return true;
 }
 
 /**
  * Write a private file with user-only permissions on every platform.
  *
- * - POSIX: mode 0600 (subject to umask, which typically keeps it at 0600).
+ * - POSIX: mode 0600 (applied by the write itself, subject to umask).
  * - Windows: mode bits are ignored by the OS, so we remove inherited ACEs
  *   and grant the current user exclusive full control.
+ *
+ * Returns true when the platform's permission model was applied as requested.
  */
-export function writePrivateFile(targetPath: string, contents: string): void {
+export function writePrivateFile(targetPath: string, contents: string): boolean {
   fs.writeFileSync(targetPath, contents, { encoding: "utf8", mode: PRIVATE_FILE_MODE });
-  if (process.platform === "win32") {
-    restrictWindowsAcl(targetPath);
-  }
+  return process.platform === "win32" ? restrictWindowsAcl(targetPath) : true;
 }
 
 /**
  * Ensure a directory exists with user-only permissions (0700 on POSIX;
- * current-user-only ACL on Windows).
+ * current-user-only ACL on Windows).  Returns true when applied as requested.
  */
-export function ensurePrivateDirectory(dirPath: string): void {
+export function ensurePrivateDirectory(dirPath: string): boolean {
   fs.mkdirSync(dirPath, { recursive: true, mode: PRIVATE_DIRECTORY_MODE });
-  if (process.platform === "win32") {
-    restrictWindowsAcl(dirPath);
-  }
+  return process.platform === "win32" ? restrictWindowsAcl(dirPath) : true;
 }
 
 /** Home directory used for DeepCode user state. */

packages/core/src/tests/private-storage.test.ts

@@ -6,9 +6,10 @@ import * as os from "os";
 import * as path from "path";
 import {
   PRIVATE_FILE_MODE,
+  PRIVATE_DIRECTORY_MODE,
   ensurePrivateDirectory,
+  windowsIdentity,
   writePrivateFile,
-  restrictWindowsAcl,
 } from "../common/private-storage";
 
 const tempDirs: string[] = [];
@@ -32,7 +33,9 @@ describe("private-storage", () => {
     }
     const dir = tempDir("dc-priv-posix-");
     const file = path.join(dir, "secret.json");
-    writePrivateFile(file, "{}");
+    // The strict return value is part of the contract: true means the caller
+    // has a private file, false must never masquerade as success.
+    assert.equal(writePrivateFile(file, "{}"), true, "writePrivateFile must report success");
     const mode = fs.statSync(file).mode & 0o777;
     assert.equal(mode, PRIVATE_FILE_MODE, "file must be 0600");
   });
@@ -43,41 +46,55 @@ describe("private-storage", () => {
     }
     const base = tempDir("dc-priv-dir-");
     const dir = path.join(base, ".deepcode", "nested");
-    ensurePrivateDirectory(dir);
+    assert.equal(ensurePrivateDirectory(dir), true, "ensurePrivateDirectory must report success");
     const mode = fs.statSync(dir).mode & 0o777;
-    assert.equal(mode, 0o700, "directory must be 0700");
+    assert.equal(mode, PRIVATE_DIRECTORY_MODE, "directory must be 0700");
   });
 
-  it("restricts Windows ACL to the current user (Windows only)", () => {
+  it("restricts the Windows ACL to the current user (Windows only)", (t) => {
     if (process.platform !== "win32") {
       return;
     }
     const dir = tempDir("dc-priv-acl-");
     const file = path.join(dir, "credentials.json");
-    writePrivateFile(file, "{}");
+    const applied = writePrivateFile(file, "{}");
+    if (!applied) {
+      // Say "not verified" out loud instead of passing vacuously: a file that
+      // icacls could not touch must never look like a green ACL test.
+      t.skip("icacls could not restrict the ACL in this environment");
+      return;
+    }
 
     const out = execFileSync("icacls", [file], {
       encoding: "utf8",
       windowsHide: true,
     });
-    // Dangerous inherited ACEs must be gone.
-    for (const dangerous of ["Authenticated Users", "Everyone"]) {
-      assert.ok(!out.includes(dangerous), `must not contain ${dangerous}`);
+
+    // 1. /inheritance:r really ran: not one inherited ACE may survive.  This is
+    //    the assertion that catches a silent no-op, where the file keeps the
+    //    permissive ACEs it inherited from its parent directory.
+    assert.ok(!out.includes("(I)"), `inherited ACEs still apply:\n${out}`);
+
+    // 2. The current user keeps full control.  icacls prints `DOMAIN\user:(F)`
+    //    for an explicit ACE and `DOMAIN\user:(I)(F)` when it is inherited, so
+    //    read the exact principal back instead of matching a loose pattern.
+    const identity = windowsIdentity();
+    assert.ok(identity, "windowsIdentity() must resolve the current user");
+    assert.ok(out.includes(`${identity}:(F)`), `${identity} must keep (F):\n${out}`);
+
+    // 3. No broad principal keeps access.  These are the English Windows names;
+    //    a localized build passes this trivially, which assertion 1 covers.
+    for (const broad of ["Authenticated Users", "Everyone"]) {
+      assert.ok(!out.includes(broad), `must not grant ${broad}:\n${out}`);
     }
-    // The current user keeps full control (with or without inherited flag).
-    // icacls prints either `user:(F)` or `user:(I)(F)` depending on the ACE.
-    assert.match(out, /:\(I?\)?\(F\)/, "current user must retain full control");
   });
 
   it("is idempotent when called repeatedly", () => {
     const dir = tempDir("dc-priv-again-");
     const file = path.join(dir, "x.json");
-    writePrivateFile(file, "1");
-    writePrivateFile(file, "2");
-    assert.equal(fs.readFileSync(file, "utf8"), "2");
-    if (process.platform === "win32") {
-      // Second call must not throw and must keep the ACL restricted.
-      restrictWindowsAcl(file);
-    }
+    const first = writePrivateFile(file, "1");
+    const second = writePrivateFile(file, "2");
+    assert.equal(fs.readFileSync(file, "utf8"), "2", "last write must win");
+    assert.equal(second, first, "repeated writes must report the same outcome");
   });
 });