mirror of
https://github.com/garrytan/gstack.git
synced 2026-06-19 00:00:13 +02:00
dd8402c8b4
gstack's ~/.gstack/ state directory holds bearer tokens, canary tokens, agent
queue contents (with prompt history), session state, security-decision logs,
and saved cookie bundles — all written with { mode: 0o600 } / 0o700. On Windows,
those mode bits are a silent no-op: Node's fs module doesn't translate POSIX
modes to NTFS ACLs, and inherited ACLs leave every "restricted" file readable
by other principals on the machine (verified via icacls — six ACEs, the
intended user is the LAST of six).
Threat model is non-trivial on:
- Self-hosted CI runners (different service account on the same Windows box
can read developer tokens, canary tokens, prompt history)
- Shared development machines (agencies, studios, lab environments)
- Multi-tenant servers with shared home directories
Orthogonal to v1.24.0.0's binary-resolution work — complementary at the write
side. v1.24's bin/gstack-paths resolves ~/.gstack/ correctly across plugin /
global / local installs; this PR ensures files written into those resolved
paths actually get the POSIX 0o600 semantic translated to NTFS.
The fix:
- New browse/src/file-permissions.ts (158 LOC, 5 public + 1 test-reset).
restrictFilePermissions / restrictDirectoryPermissions wrap chmod (POSIX)
or icacls /inheritance:r /grant:r <user>:(F) (Windows). writeSecureFile /
appendSecureFile / mkdirSecure are drop-in wrappers for the common patterns.
- 19 call sites converted across 9 source files: browser-manager.ts,
browser-skill-write.ts, cli.ts, config.ts, meta-commands.ts,
security-classifier.ts, security.ts (4 sites), server.ts (5 sites),
terminal-agent.ts (8 sites), tunnel-denial-log.ts.
- (OI)(CI) inheritance flags on directories mean files created via fs.write*
*inside* an mkdirSecure-created dir inherit the owner-only ACL automatically
— important for tunnel-denial-log.ts where appends use async fsp.appendFile.
Error handling: icacls failures (nonexistent path, missing icacls.exe, hardened
environments) log a one-shot warning to stderr and proceed. Once-per-process
gating prevents log spam if the condition persists. Filesystem stays
functional; the file just ends up with inherited ACLs.
Test plan:
- bun test browse/test/file-permissions.test.ts — 13 pass, 0 fail (POSIX
mode-bit assertions, Windows no-throw, mkdir idempotence, recursive
creation, Buffer payloads, append-creates-then-reapplies-once semantics)
- bun test browse/test/security.test.ts — 38 pass, 0 fail (existing security
test suite plus the bash-binary resolution tests added in fix #1119; the
converted writeFileSync/appendFileSync/mkdirSync sites in security.ts
integrate cleanly)
- Empirical icacls before/after on a real file — 6 ACEs → 1 ACE
- bun build typecheck on all modified files — clean (server.ts has a
pre-existing playwright-core/electron resolution issue unrelated to this PR)
POSIX behavior is bit-identical to old code — fs.chmodSync(path, 0o6XX) on the
helper's POSIX branch matches the inline { mode: 0o6XX } it replaces. Linux
and macOS see no behavior change.
Inviting pushback on three judgment calls (in PR description):
1. icacls vs npm library
2. ACL scope — just user, or user + SYSTEM?
3. Graceful degradation — once-per-process warn, not silent, not hard-fail.
149 lines
4.9 KiB
TypeScript
149 lines
4.9 KiB
TypeScript
/**
|
|
* Unit tests for browse/src/file-permissions.ts
|
|
*
|
|
* Strategy:
|
|
* - POSIX assertions check fs.statSync.mode bits directly (cheap, reliable,
|
|
* runs on every CI config).
|
|
* - Windows assertions don't check ACLs (that'd require parsing icacls
|
|
* output, which is brittle across Windows versions / locales). Instead
|
|
* we verify the helper doesn't throw and the file ends up accessible
|
|
* to the current user — the "doesn't crash, file still usable"
|
|
* contract the callers rely on.
|
|
*/
|
|
|
|
import { afterEach, beforeEach, describe, expect, test } from 'bun:test';
|
|
import * as fs from 'fs';
|
|
import * as os from 'os';
|
|
import * as path from 'path';
|
|
|
|
import {
|
|
restrictFilePermissions,
|
|
restrictDirectoryPermissions,
|
|
writeSecureFile,
|
|
appendSecureFile,
|
|
mkdirSecure,
|
|
__resetWarnedForTests,
|
|
} from '../src/file-permissions';
|
|
|
|
let tmpDir: string;
|
|
|
|
beforeEach(() => {
|
|
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'file-perms-'));
|
|
__resetWarnedForTests();
|
|
});
|
|
|
|
afterEach(() => {
|
|
try { fs.rmSync(tmpDir, { recursive: true, force: true }); } catch { /* best-effort */ }
|
|
});
|
|
|
|
describe('restrictFilePermissions', () => {
|
|
test('on POSIX, sets file mode to 0o600', () => {
|
|
if (process.platform === 'win32') return;
|
|
const p = path.join(tmpDir, 'secret');
|
|
fs.writeFileSync(p, 'token');
|
|
fs.chmodSync(p, 0o644); // start world-readable to prove the call mutates it
|
|
restrictFilePermissions(p);
|
|
expect(fs.statSync(p).mode & 0o777).toBe(0o600);
|
|
});
|
|
|
|
test('on Windows, does not throw on an existing file', () => {
|
|
if (process.platform !== 'win32') return;
|
|
const p = path.join(tmpDir, 'secret');
|
|
fs.writeFileSync(p, 'token');
|
|
expect(() => restrictFilePermissions(p)).not.toThrow();
|
|
// File remains readable by the caller — core contract.
|
|
expect(fs.readFileSync(p, 'utf8')).toBe('token');
|
|
});
|
|
|
|
test('on Windows, does not throw when icacls fails (bad path)', () => {
|
|
if (process.platform !== 'win32') return;
|
|
// icacls emits an error for a nonexistent path; helper must swallow.
|
|
expect(() => restrictFilePermissions(path.join(tmpDir, 'nonexistent'))).not.toThrow();
|
|
});
|
|
});
|
|
|
|
describe('restrictDirectoryPermissions', () => {
|
|
test('on POSIX, sets directory mode to 0o700', () => {
|
|
if (process.platform === 'win32') return;
|
|
const d = path.join(tmpDir, 'subdir');
|
|
fs.mkdirSync(d, { mode: 0o755 });
|
|
restrictDirectoryPermissions(d);
|
|
expect(fs.statSync(d).mode & 0o777).toBe(0o700);
|
|
});
|
|
|
|
test('on Windows, does not throw on an existing directory', () => {
|
|
if (process.platform !== 'win32') return;
|
|
const d = path.join(tmpDir, 'subdir');
|
|
fs.mkdirSync(d);
|
|
expect(() => restrictDirectoryPermissions(d)).not.toThrow();
|
|
});
|
|
});
|
|
|
|
describe('writeSecureFile', () => {
|
|
test('writes the payload and restricts permissions atomically', () => {
|
|
const p = path.join(tmpDir, 'data');
|
|
writeSecureFile(p, 'hello');
|
|
expect(fs.readFileSync(p, 'utf8')).toBe('hello');
|
|
if (process.platform !== 'win32') {
|
|
expect(fs.statSync(p).mode & 0o777).toBe(0o600);
|
|
}
|
|
});
|
|
|
|
test('accepts Buffer payloads', () => {
|
|
const p = path.join(tmpDir, 'buffer');
|
|
writeSecureFile(p, Buffer.from([0xde, 0xad, 0xbe, 0xef]));
|
|
const out = fs.readFileSync(p);
|
|
expect(out.length).toBe(4);
|
|
expect(out[0]).toBe(0xde);
|
|
});
|
|
|
|
test('overwrites existing file', () => {
|
|
const p = path.join(tmpDir, 'existing');
|
|
fs.writeFileSync(p, 'old', { mode: 0o644 });
|
|
writeSecureFile(p, 'new');
|
|
expect(fs.readFileSync(p, 'utf8')).toBe('new');
|
|
});
|
|
});
|
|
|
|
describe('appendSecureFile', () => {
|
|
test('appends to a new file and sets owner-only permissions', () => {
|
|
const p = path.join(tmpDir, 'log');
|
|
appendSecureFile(p, 'line1\n');
|
|
expect(fs.readFileSync(p, 'utf8')).toBe('line1\n');
|
|
if (process.platform !== 'win32') {
|
|
expect(fs.statSync(p).mode & 0o777).toBe(0o600);
|
|
}
|
|
});
|
|
|
|
test('appends without re-applying ACL on subsequent writes', () => {
|
|
const p = path.join(tmpDir, 'log');
|
|
appendSecureFile(p, 'line1\n');
|
|
appendSecureFile(p, 'line2\n');
|
|
expect(fs.readFileSync(p, 'utf8')).toBe('line1\nline2\n');
|
|
});
|
|
});
|
|
|
|
describe('mkdirSecure', () => {
|
|
test('creates directory with owner-only mode (POSIX)', () => {
|
|
if (process.platform === 'win32') return;
|
|
const d = path.join(tmpDir, 'nested', 'deep');
|
|
mkdirSecure(d);
|
|
expect(fs.statSync(d).isDirectory()).toBe(true);
|
|
expect(fs.statSync(d).mode & 0o777).toBe(0o700);
|
|
});
|
|
|
|
test('is idempotent — safe to call on existing directory', () => {
|
|
const d = path.join(tmpDir, 'dir');
|
|
mkdirSecure(d);
|
|
expect(() => mkdirSecure(d)).not.toThrow();
|
|
});
|
|
|
|
test('recursive behavior: creates intermediate directories', () => {
|
|
const d = path.join(tmpDir, 'a', 'b', 'c');
|
|
mkdirSecure(d);
|
|
expect(fs.existsSync(path.join(tmpDir, 'a'))).toBe(true);
|
|
expect(fs.existsSync(path.join(tmpDir, 'a', 'b'))).toBe(true);
|
|
expect(fs.existsSync(d)).toBe(true);
|
|
});
|
|
});
|