From 08a5c9378eecfda56e067c839f2918145fffd73d Mon Sep 17 00:00:00 2001
From: Eshan Thanvi <eshanthanvi@gmail.com>
Date: Sat, 4 Jul 2026 05:00:20 +0400
Subject: [PATCH] fix(browse): grant Windows ACLs by SID to avoid owner lockout
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

restrictFilePermissions/restrictDirectoryPermissions granted icacls
permissions by bare username (os.userInfo().username). On machines where
the username collides with another account object — most commonly when
the machine name equals the username — LookupAccountName resolves the
bare name to the machine's domain SID (S-1-5-21-... with no RID),
producing an ACL that denies even the owner. ~/.gstack becomes
inaccessible and browse fails with EPERM on mkdir.

Resolve the current user's SID via %SystemRoot%\System32\whoami.exe
(absolute path — a bare 'whoami' can hit GNU coreutils' whoami from the
Git for Windows PATH, which rejects /user) and grant *<SID>:(F), which
is unambiguous. Fall back to USERDOMAIN\USERNAME if SID resolution
fails.

Note: dist/browse.exe and dist/server-node.mjs embed this module, so a
dist rebuild is required for the fix to take effect in releases.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---
 browse/src/file-permissions.ts       | 47 ++++++++++++++++++++++++++--
 browse/test/file-permissions.test.ts | 15 +++++++++
 2 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/browse/src/file-permissions.ts b/browse/src/file-permissions.ts
index d3d404acd..27bc78208 100644
--- a/browse/src/file-permissions.ts
+++ b/browse/src/file-permissions.ts
@@ -42,6 +42,42 @@ import * as os from 'os';
 
 let warnedOnce = false;
 
+let cachedGrantee: string | undefined;
+
+/**
+ * Resolve the identity to use in icacls `/grant` arguments.
+ *
+ * Granting by bare username (`os.userInfo().username`) is ambiguous:
+ * LookupAccountName can resolve the name to the wrong principal when it
+ * collides with another account object — e.g. when the machine name equals
+ * the username (user "alice" on machine "ALICE"), the bare name can resolve
+ * to the machine's *domain* SID (S-1-5-21-… with no RID), producing an ACL
+ * that denies even the owner.
+ * A SID grant (`*S-1-5-21-…-1001`) is unambiguous, so we resolve the
+ * current user's SID via `whoami /user` and fall back to the fully
+ * qualified `DOMAIN\username` (still unambiguous) only if that fails.
+ */
+function getWindowsGrantee(): string {
+  if (cachedGrantee !== undefined) return cachedGrantee;
+  try {
+    // Absolute path: a bare `whoami` can resolve to GNU coreutils' whoami
+    // (Git for Windows / MSYS2 put usr/bin on PATH), which rejects /user.
+    const whoami = `${process.env.SystemRoot || 'C:\\Windows'}\\System32\\whoami.exe`;
+    const out = execFileSync(whoami, ['/user', '/fo', 'csv', '/nh'], {
+      encoding: 'utf8',
+      stdio: ['ignore', 'pipe', 'ignore'],
+    });
+    const sid = out.match(/"(S-1-[0-9-]+)"/)?.[1];
+    if (sid) {
+      cachedGrantee = `*${sid}`;
+      return cachedGrantee;
+    }
+  } catch { /* fall through to name-based grant */ }
+  const user = process.env.USERNAME || os.userInfo().username;
+  cachedGrantee = process.env.USERDOMAIN ? `${process.env.USERDOMAIN}\\${user}` : user;
+  return cachedGrantee;
+}
+
 function warnIcaclsFailure(fsPath: string, err: unknown): void {
   if (warnedOnce) return;
   warnedOnce = true;
@@ -67,7 +103,7 @@ function warnIcaclsFailure(fsPath: string, err: unknown): void {
 export function restrictFilePermissions(filePath: string): void {
   if (process.platform === 'win32') {
     try {
-      const user = os.userInfo().username;
+      const user = getWindowsGrantee();
       execFileSync(
         'icacls',
         [filePath, '/inheritance:r', '/grant:r', `${user}:(F)`],
@@ -97,7 +133,7 @@ export function restrictFilePermissions(filePath: string): void {
 export function restrictDirectoryPermissions(dirPath: string): void {
   if (process.platform === 'win32') {
     try {
-      const user = os.userInfo().username;
+      const user = getWindowsGrantee();
       execFileSync(
         'icacls',
         [dirPath, '/inheritance:r', '/grant:r', `${user}:(OI)(CI)(F)`],
@@ -155,3 +191,10 @@ export function mkdirSecure(dirPath: string): void {
 export function __resetWarnedForTests(): void {
   warnedOnce = false;
 }
+
+/**
+ * Expose the resolved icacls grantee. Test-only.
+ */
+export function __getWindowsGranteeForTests(): string {
+  return getWindowsGrantee();
+}
diff --git a/browse/test/file-permissions.test.ts b/browse/test/file-permissions.test.ts
index e073b9945..e1401a414 100644
--- a/browse/test/file-permissions.test.ts
+++ b/browse/test/file-permissions.test.ts
@@ -23,6 +23,7 @@ import {
   appendSecureFile,
   mkdirSecure,
   __resetWarnedForTests,
+  __getWindowsGranteeForTests,
 } from '../src/file-permissions';
 
 let tmpDir: string;
@@ -36,6 +37,20 @@ afterEach(() => {
   try { fs.rmSync(tmpDir, { recursive: true, force: true }); } catch { /* best-effort */ }
 });
 
+describe('getWindowsGrantee', () => {
+  test('on Windows, resolves an unambiguous grantee (SID or DOMAIN\\user)', () => {
+    if (process.platform !== 'win32') return;
+    // A bare-username grant is ambiguous when the username collides with
+    // another account object (e.g. machine name == username), which locks
+    // even the owner out. The grantee must be a SID (`*S-1-...`) or, as a
+    // fallback, a domain-qualified name — never a bare username.
+    const grantee = __getWindowsGranteeForTests();
+    const isSid = /^\*S-1-[0-9-]+$/.test(grantee);
+    const isQualified = grantee.includes('\\');
+    expect(isSid || isQualified).toBe(true);
+  });
+});
+
 describe('restrictFilePermissions', () => {
   test('on POSIX, sets file mode to 0o600', () => {
     if (process.platform === 'win32') return;