From 6fd1ef383aa27153f5789a80a8be67b7ccb17591 Mon Sep 17 00:00:00 2001 From: IntegriGit Date: Sat, 4 Jul 2026 00:13:09 -0400 Subject: [PATCH] fix(browse): grant Windows ACLs to token SID, not bare username restrictFilePermissions / restrictDirectoryPermissions passed os.userInfo().username straight to `icacls /grant`. On a domain-joined Windows host that also has a *local* account of the same login name, icacls resolves the bare name to the LOCAL account (e.g. HOST\user), not the domain account the process runs as (e.g. DOMAIN\user). Combined with `/inheritance:r`, the new single-entry ACL is granted to an account the process is NOT, locking it out of the .gstack state dir it just created. The browse daemon then can't create its lock/state file and startup fails with a misleading "Another instance is starting... Timed out." Fix: resolve the current process token SID via System32\whoami.exe (absolute path, so a shadowing Unix `whoami` on PATH can't hijack it) and grant `*`, which is immune to local-vs-domain name collisions. Fall back to USERDOMAIN\username, then bare username, if the SID lookup is unavailable. Cached per process. Adds a regression test asserting the resolved principal is a SID literal (*S-1-5-...) on Windows and a non-empty name-based fallback off-Windows. Co-Authored-By: Paperclip --- browse/src/file-permissions.ts | 61 +++++++++++++++++++++++++++- browse/test/file-permissions.test.ts | 27 ++++++++++++ 2 files changed, 86 insertions(+), 2 deletions(-) diff --git a/browse/src/file-permissions.ts b/browse/src/file-permissions.ts index d3d404acd..c718868e1 100644 --- a/browse/src/file-permissions.ts +++ b/browse/src/file-permissions.ts @@ -42,6 +42,51 @@ import * as os from 'os'; let warnedOnce = false; +/** + * Resolve an unambiguous icacls principal for the account this process runs as. + * + * `os.userInfo().username` returns the bare login name (e.g. "lmiller"). On a + * domain-joined Windows box that also has a *local* account of the same name, + * `icacls /grant lmiller:...` resolves the bare name to the LOCAL account + * (e.g. OFC01\lmiller), NOT the domain account the process actually runs as + * (e.g. INTEGRIBILT\lmiller). Combined with `/inheritance:r`, that hands the + * new ACL to an account this process is *not*, locking the process out of the + * directory it just created — the daemon then can't create its state/lock file + * and startup fails. See INT-50 / INT-48. + * + * Fix: grant to the current process token's SID, which is immune to + * local-vs-domain name collisions. Fall back to USERDOMAIN\username, then the + * bare username, if the SID lookup is unavailable. Cached per process. + */ +let cachedIcaclsPrincipal: string | undefined; +function currentUserIcaclsPrincipal(): string { + if (cachedIcaclsPrincipal !== undefined) return cachedIcaclsPrincipal; + try { + // Call System32's whoami.exe by absolute path. A bare 'whoami' can resolve + // to a shadowing binary earlier on PATH (e.g. Git-for-Windows' Unix + // `whoami`, which rejects `/user`), which would force the weaker name-based + // fallback below. The absolute path guarantees we get the real SID. + const systemRoot = process.env.SystemRoot || process.env.windir || 'C:\\Windows'; + const whoamiExe = `${systemRoot}\\System32\\whoami.exe`; + const out = execFileSync(whoamiExe, ['/user', '/fo', 'csv', '/nh'], { + encoding: 'utf8', + }).trim(); + // Format: "DOMAIN\user","S-1-5-21-..." + const m = out.match(/"[^"]*","(S-[0-9-]+)"/); + if (m) { + // icacls accepts a SID literal when prefixed with '*'. + cachedIcaclsPrincipal = `*${m[1]}`; + return cachedIcaclsPrincipal; + } + } catch { + /* fall through to name-based resolution */ + } + const username = os.userInfo().username; + const domain = process.env.USERDOMAIN; + cachedIcaclsPrincipal = domain ? `${domain}\\${username}` : username; + return cachedIcaclsPrincipal; +} + function warnIcaclsFailure(fsPath: string, err: unknown): void { if (warnedOnce) return; warnedOnce = true; @@ -67,7 +112,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 = currentUserIcaclsPrincipal(); execFileSync( 'icacls', [filePath, '/inheritance:r', '/grant:r', `${user}:(F)`], @@ -97,7 +142,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 = currentUserIcaclsPrincipal(); execFileSync( 'icacls', [dirPath, '/inheritance:r', '/grant:r', `${user}:(OI)(CI)(F)`], @@ -155,3 +200,15 @@ export function mkdirSecure(dirPath: string): void { export function __resetWarnedForTests(): void { warnedOnce = false; } + +/** + * Resolve the icacls principal for the current process token. Test-only + * accessor for the module-private `currentUserIcaclsPrincipal`; clears the + * per-process cache first so each call observes a fresh resolution. + */ +export function __currentUserIcaclsPrincipalForTests(): string { + cachedIcaclsPrincipal = undefined; + const principal = currentUserIcaclsPrincipal(); + cachedIcaclsPrincipal = undefined; + return principal; +} diff --git a/browse/test/file-permissions.test.ts b/browse/test/file-permissions.test.ts index e073b9945..6bd84aca0 100644 --- a/browse/test/file-permissions.test.ts +++ b/browse/test/file-permissions.test.ts @@ -23,6 +23,7 @@ import { appendSecureFile, mkdirSecure, __resetWarnedForTests, + __currentUserIcaclsPrincipalForTests, } from '../src/file-permissions'; let tmpDir: string; @@ -123,6 +124,32 @@ describe('appendSecureFile', () => { }); }); +describe('currentUserIcaclsPrincipal (regression: INT-50 domain SID collision)', () => { + // Background: on a domain-joined Windows box that also has a *local* account + // of the same login name, `icacls /grant lmiller:...` resolves the bare name + // to the LOCAL account, not the domain account the process runs as. Combined + // with `/inheritance:r`, that locks the process out of the .gstack state dir + // it just created and the browse daemon fails to start. The fix grants to the + // process token SID instead, which is collision-proof. + test('on Windows, resolves to a SID literal (*S-1-5-...), not a bare username', () => { + if (process.platform !== 'win32') return; + const principal = __currentUserIcaclsPrincipalForTests(); + // A bare `os.userInfo().username` (the pre-fix behavior) would NOT start + // with '*S-'. The '*'-prefixed SID is what makes the grant unambiguous. + expect(principal.startsWith('*S-')).toBe(true); + }); + + test('off Windows, falls back to a non-empty name-based principal without throwing', () => { + if (process.platform === 'win32') return; + // whoami.exe /user is unavailable off-Windows; the helper must swallow the + // spawn failure and fall back to a name-based principal rather than throw. + let principal = ''; + expect(() => { principal = __currentUserIcaclsPrincipalForTests(); }).not.toThrow(); + expect(principal.length).toBeGreaterThan(0); + expect(principal.startsWith('*S-')).toBe(false); + }); +}); + describe('mkdirSecure', () => { test('creates directory with owner-only mode (POSIX)', () => { if (process.platform === 'win32') return;