mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-17 18:32:19 +02:00
fix(windows): grant icacls ACEs by *SID, not unqualified username
An unqualified username handed to icacls is ambiguous: on a machine whose hostname equals the username (a common Windows setup), it resolves to the MACHINE account instead of the user. Combined with /inheritance:r, that leaves ~/.gstack with a single ACE matching nobody — the process that just "secured" the directory locks itself out, and icacls still reports success. Both icacls sites in the repo (restrictFilePermissions and restrictDirectoryPermissions in browse/src/file-permissions.ts — the only icacls call sites; setup has none) now grant via icacls' literal-SID form `*<SID>`, resolved once per process from System32\whoami.exe (pinned to System32 because a bare `whoami` under a bash-flavoured PATH picks up the MSYS build, which rejects /user). Fallback when the SID can't be resolved is the domain-qualified `USERDOMAIN\username` name, which is unambiguous where the bare username was not. Windows-only regression tests assert the hardened directory stays usable by the calling process (readdir + write), which is exactly the check that a not-toThrow assertion sailed past before. Contributed by @asizux2 (PR #2479); the same defect was independently fixed by @Icandi40, @chiragborse1, @IntegriGit and @voltapix26. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
81659f9456
commit
f96fd46b1c
@@ -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);
|
||||
|
||||
Reference in New Issue
Block a user