fix(windows): grant icacls by SID, not unqualified username

An unqualified username is ambiguous. On a machine whose hostname equals
the username, it does not resolve to the user account; icacls silently
writes an ACE for the machine SID instead. Combined with /inheritance:r,
the directory is left with a single ACE that matches nobody, so the
process that just created it can no longer enumerate or write to it.

In practice this means browse can never start on an affected machine: it
bricks .gstack/ on first run, then reports "Another instance is starting
the server" because acquireServerLock() returns null on the resulting
EACCES. The real cause is invisible, since icacls reports success and
stdio is ignored.

Resolve the current user's SID and pass icacls the literal *<SID> form,
which is immune to name-resolution ambiguity. Fall back to the
domain-qualified name if the lookup fails.

The SID lookup pins %SystemRoot%\System32\whoami.exe rather than a bare
`whoami`, which under a bash-flavoured PATH resolves to the MSYS build
and rejects /user. Without the pin the primary path would silently fail
on Git Bash, one of the most common Windows setups for this tool.

Tests assert the directory stays usable by the calling process after
hardening. The existing Windows cases only asserted "does not throw",
which this bug sails straight past. On the pre-fix code path 8 of 15
tests fail, several of them pre-existing.

Refs #2478

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

browse/src/file-permissions.ts

@@ -42,6 +42,52 @@ import * as os from 'os';
 
 let warnedOnce = false;
 
+let cachedSid: string | null | undefined;
+
+/**
+ * Resolve the current user's SID, cached for the process lifetime.
+ *
+ * Returns null if `whoami` is unavailable or its output cannot be parsed,
+ * in which case callers fall back to a domain-qualified account name.
+ */
+function currentUserSid(): string | null {
+  if (cachedSid !== undefined) return cachedSid;
+  try {
+    // Pin to the System32 binary. A bare `whoami` resolves to the MSYS/Git
+    // Bash build under a bash-flavoured PATH, which rejects `/user` — the
+    // lookup would then silently fail on one of the most common Windows
+    // setups for this tool.
+    const systemRoot = process.env.SystemRoot || process.env.windir || 'C:\\Windows';
+    const out = execFileSync(`${systemRoot}\\System32\\whoami.exe`, ['/user', '/fo', 'csv', '/nh'], {
+      encoding: 'utf8',
+    });
+    const match = out.match(/S-1-[\d-]+/);
+    cachedSid = match ? match[0] : null;
+  } catch {
+    cachedSid = null;
+  }
+  return cachedSid;
+}
+
+/**
+ * The principal to hand icacls for "the current user".
+ *
+ * An unqualified username is ambiguous: on a machine whose hostname equals
+ * the username, it fails to resolve to the user account and icacls silently
+ * writes an ACE for the machine SID instead. Combined with `/inheritance:r`
+ * that leaves a directory whose only ACE matches nobody — locking out the
+ * process that just created it.
+ *
+ * `*<SID>` is icacls' literal-SID form and is immune to that ambiguity.
+ * The domain-qualified name is the fallback.
+ */
+function currentUserPrincipal(): string {
+  const sid = currentUserSid();
+  if (sid) return `*${sid}`;
+  const domain = process.env.USERDOMAIN || os.hostname();
+  return `${domain}\\${os.userInfo().username}`;
+}
+
 function warnIcaclsFailure(fsPath: string, err: unknown): void {
   if (warnedOnce) return;
   warnedOnce = true;
@@ -67,7 +113,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 = currentUserPrincipal();
       execFileSync(
         'icacls',
         [filePath, '/inheritance:r', '/grant:r', `${user}:(F)`],
@@ -97,7 +143,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 = currentUserPrincipal();
       execFileSync(
         'icacls',
         [dirPath, '/inheritance:r', '/grant:r', `${user}:(OI)(CI)(F)`],

browse/test/file-permissions.test.ts

@@ -77,6 +77,26 @@ describe('restrictDirectoryPermissions', () => {
     fs.mkdirSync(d);
     expect(() => restrictDirectoryPermissions(d)).not.toThrow();
   });
+
+  test('on Windows, the directory stays usable by the calling process', () => {
+    if (process.platform !== 'win32') return;
+    const d = path.join(tmpDir, 'still-usable');
+    fs.mkdirSync(d);
+    fs.writeFileSync(path.join(d, 'before'), 'x');
+
+    restrictDirectoryPermissions(d);
+
+    // Regression: an unqualified username passed to icacls can resolve to
+    // the machine SID rather than the user account. Combined with
+    // /inheritance:r that leaves a directory whose only ACE matches nobody,
+    // so the process that just "secured" it can no longer enumerate or
+    // write to it. icacls still reports success, so a not-toThrow assertion
+    // sails straight past it — hence these access checks.
+    expect(() => fs.readdirSync(d)).not.toThrow();
+    expect(fs.readdirSync(d)).toContain('before');
+    expect(() => fs.writeFileSync(path.join(d, 'after'), 'y')).not.toThrow();
+    expect(fs.readFileSync(path.join(d, 'after'), 'utf8')).toBe('y');
+  });
 });
 
 describe('writeSecureFile', () => {
@@ -138,6 +158,16 @@ describe('mkdirSecure', () => {
     expect(() => mkdirSecure(d)).not.toThrow();
   });
 
+  test('on Windows, the created directory stays usable by the caller', () => {
+    if (process.platform !== 'win32') return;
+    // The state-dir path that broke: mkdirSecure() creates .gstack/, hardens
+    // it, and the very next thing the daemon does is write a lockfile inside.
+    const d = path.join(tmpDir, 'state', '.gstack');
+    mkdirSecure(d);
+    expect(() => fs.writeFileSync(path.join(d, 'browse.json.lock'), '1')).not.toThrow();
+    expect(fs.readdirSync(d)).toContain('browse.json.lock');
+  });
+
   test('recursive behavior: creates intermediate directories', () => {
     const d = path.join(tmpDir, 'a', 'b', 'c');
     mkdirSecure(d);