diff --git a/browse/src/file-permissions.ts b/browse/src/file-permissions.ts index d3d404acd..ebd56a20e 100644 --- a/browse/src/file-permissions.ts +++ b/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. + * + * `*` 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)`], diff --git a/browse/test/file-permissions.test.ts b/browse/test/file-permissions.test.ts index e073b9945..4a548650c 100644 --- a/browse/test/file-permissions.test.ts +++ b/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);