diff --git a/browse/src/file-permissions.ts b/browse/src/file-permissions.ts index d3d404acd..e52c1966a 100644 --- a/browse/src/file-permissions.ts +++ b/browse/src/file-permissions.ts @@ -71,7 +71,7 @@ export function restrictFilePermissions(filePath: string): void { execFileSync( 'icacls', [filePath, '/inheritance:r', '/grant:r', `${user}:(F)`], - { stdio: 'ignore' }, + { stdio: 'ignore', windowsHide: true }, ); } catch (err) { warnIcaclsFailure(filePath, err); @@ -101,7 +101,7 @@ export function restrictDirectoryPermissions(dirPath: string): void { execFileSync( 'icacls', [dirPath, '/inheritance:r', '/grant:r', `${user}:(OI)(CI)(F)`], - { stdio: 'ignore' }, + { stdio: 'ignore', windowsHide: true }, ); } catch (err) { warnIcaclsFailure(dirPath, err); @@ -139,14 +139,54 @@ export function appendSecureFile( if (!existed) restrictFilePermissions(filePath); } +/** + * Windows only: probe whether the current process can actually list the + * directory. `fs.accessSync` doesn't consult NTFS ACLs on Windows, so a + * real readdir is the only honest check. + */ +function canListDir(dirPath: string): boolean { + try { fs.readdirSync(dirPath); return true; } catch { return false; } +} + +/** + * Windows only: repair a broken DACL on a state directory (#1605). + * + * `icacls /inheritance:r /grant:r :(F)` is a single command, but the + * two halves can partially fail: inheritance gets stripped while the user + * grant doesn't resolve (localized account names, domain accounts, roaming + * profiles). The result is a DACL with no usable ACE — often just a machine + * SID — and the client can't read its own state files. `/reset` restores + * inherited ACLs from the parent, making the directory functional again. + * Functional-but-unhardened beats hardened-but-unusable. + */ +export function repairBrokenDacl(dirPath: string): void { + if (process.platform !== 'win32') return; + try { + execFileSync('icacls', [dirPath, '/reset', '/T', '/C', '/Q'], { stdio: 'ignore', windowsHide: true }); + } catch (err) { + warnIcaclsFailure(dirPath, err); + } +} + /** * `mkdir -p` with owner-only directory permissions, cross-platform. * Replaces `fs.mkdirSync(path, { recursive: true, mode: 0o700 })` + Windows ACL. * Safe to call on an existing directory — re-applies the ACL idempotently. + * + * Windows: after applying the restricted ACL, verifies the directory is + * still listable by this process and repairs a broken DACL (#1605) if not. */ export function mkdirSecure(dirPath: string): void { fs.mkdirSync(dirPath, { recursive: true, mode: 0o700 }); restrictDirectoryPermissions(dirPath); + if (process.platform === 'win32' && !canListDir(dirPath)) { + repairBrokenDacl(dirPath); + restrictDirectoryPermissions(dirPath); + // If re-hardening broke access again, reset once more and leave the + // directory with inherited ACLs — the client must be able to read + // its own state. + if (!canListDir(dirPath)) repairBrokenDacl(dirPath); + } } /** diff --git a/browse/test/file-permissions.test.ts b/browse/test/file-permissions.test.ts index e073b9945..1a892bfc0 100644 --- a/browse/test/file-permissions.test.ts +++ b/browse/test/file-permissions.test.ts @@ -22,6 +22,7 @@ import { writeSecureFile, appendSecureFile, mkdirSecure, + repairBrokenDacl, __resetWarnedForTests, } from '../src/file-permissions'; @@ -145,4 +146,35 @@ describe('mkdirSecure', () => { expect(fs.existsSync(path.join(tmpDir, 'a', 'b'))).toBe(true); expect(fs.existsSync(d)).toBe(true); }); + + test('created directory is listable by the creating process', () => { + // #1605 contract: whatever ACL hardening happens, the client must be + // able to read its own state dir immediately after creation. + const d = path.join(tmpDir, 'state'); + mkdirSecure(d); + fs.writeFileSync(path.join(d, 'browse.json'), '{}'); + expect(fs.readdirSync(d)).toContain('browse.json'); + }); +}); + +describe('repairBrokenDacl', () => { + test('is a no-op on non-Windows platforms', () => { + if (process.platform === 'win32') return; + const d = path.join(tmpDir, 'dir'); + fs.mkdirSync(d); + expect(() => repairBrokenDacl(d)).not.toThrow(); + }); + + test('on Windows, does not throw and directory stays listable', () => { + if (process.platform !== 'win32') return; + const d = path.join(tmpDir, 'dir'); + fs.mkdirSync(d); + expect(() => repairBrokenDacl(d)).not.toThrow(); + expect(() => fs.readdirSync(d)).not.toThrow(); + }); + + test('on Windows, swallows icacls failure on a nonexistent path', () => { + if (process.platform !== 'win32') return; + expect(() => repairBrokenDacl(path.join(tmpDir, 'nonexistent'))).not.toThrow(); + }); }); diff --git a/browse/test/windows-spawn-hide.test.ts b/browse/test/windows-spawn-hide.test.ts new file mode 100644 index 000000000..fb4de5b95 --- /dev/null +++ b/browse/test/windows-spawn-hide.test.ts @@ -0,0 +1,55 @@ +/** + * Static tripwire for #1835: child spawns reachable on Windows must pass + * windowsHide, or every daemon relaunch / taskkill / icacls / powershell + * invocation flashes a black console window (and can steal focus). + * + * Source-level, same style as server-auth.test.ts / cdp-session-cleanup.test.ts: + * cheap, deterministic, runs on every platform. + */ + +import { describe, expect, test } from 'bun:test'; +import * as fs from 'fs'; +import * as path from 'path'; + +const SRC = (f: string) => fs.readFileSync(path.join(import.meta.dir, '../src', f), 'utf-8'); + +/** Every occurrence of `needle` in `src` must have `windowsHide` within the + * next `window` chars (the spawn's options object). */ +function expectHideNearEvery(src: string, needle: string, window = 400): void { + let idx = src.indexOf(needle); + expect(idx).toBeGreaterThanOrEqual(0); + while (idx !== -1) { + const slice = src.slice(idx, idx + window); + expect(slice).toMatch(/windowsHide:\s*true/); + idx = src.indexOf(needle, idx + needle.length); + } +} + +describe('windowsHide on Windows-reachable spawns (#1835)', () => { + test('daemon launch paths in cli.ts pass windowsHide', () => { + const cli = SRC('cli.ts'); + // Installed path: node -e launcher — both the outer spawnSync and the + // inner detached daemon spawn (inside the launcher code string). + expect(cli).toContain('detached:true,windowsHide:true'); + expectHideNearEvery(cli, "'-e', launcherCode]"); + // Dev fallback: detached bun spawn. + expectHideNearEvery(cli, "nodeSpawn('bun'"); + // taskkill (killServer). + expectHideNearEvery(cli, "'taskkill'"); + }); + + test('Windows-only process probes pass windowsHide', () => { + // tasklist in isProcessAlive — runs in polling loops. + expectHideNearEvery(SRC('error-handling.ts'), "'tasklist'"); + // powershell DPAPI + tasklist in cookie import. + const cookie = SRC('cookie-import-browser.ts'); + expectHideNearEvery(cookie, "'powershell'"); + expectHideNearEvery(cookie, "'tasklist'"); + }); + + test('icacls calls in file-permissions.ts pass windowsHide', () => { + const perms = SRC('file-permissions.ts'); + expect((perms.match(/'icacls'/g) || []).length).toBeGreaterThanOrEqual(3); + expectHideNearEvery(perms, "'icacls'"); + }); +});