fix(browse): NTFS ACL hardening for Windows state files via icacls

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.
This commit is contained in:
Samuel Carson
2026-05-03 16:01:07 -05:00
parent bf65487162
commit dd8402c8b4
12 changed files with 355 additions and 37 deletions
+9 -8
View File
@@ -24,6 +24,7 @@ import { spawn } from 'child_process';
import * as fs from 'fs';
import * as path from 'path';
import * as os from 'os';
import { writeSecureFile, appendSecureFile, mkdirSecure } from './file-permissions';
// ─── Thresholds + verdict types ──────────────────────────────
@@ -344,11 +345,11 @@ function getDeviceSalt(): string {
// fall through to generate
}
try {
fs.mkdirSync(SECURITY_DIR, { recursive: true, mode: 0o700 });
mkdirSecure(SECURITY_DIR);
} catch {}
cachedSalt = randomBytes(16).toString('hex');
try {
fs.writeFileSync(SALT_FILE, cachedSalt, { mode: 0o600 });
writeSecureFile(SALT_FILE, cachedSalt);
} catch {
// Can't persist (read-only fs, disk full). Keep the in-memory salt
// for this process so cross-log correlation still works within a
@@ -456,10 +457,10 @@ export function logAttempt(record: AttemptRecord): boolean {
// the event reported (it goes to a different directory anyway).
reportAttemptTelemetry(record);
try {
fs.mkdirSync(SECURITY_DIR, { recursive: true, mode: 0o700 });
mkdirSecure(SECURITY_DIR);
rotateIfNeeded();
const line = JSON.stringify(record) + '\n';
fs.appendFileSync(ATTEMPTS_LOG, line, { mode: 0o600 });
appendSecureFile(ATTEMPTS_LOG, line);
return true;
} catch (err) {
// Non-fatal. Log to stderr for debugging but don't block.
@@ -489,9 +490,9 @@ export interface SessionState {
*/
export function writeSessionState(state: SessionState): void {
try {
fs.mkdirSync(SECURITY_DIR, { recursive: true, mode: 0o700 });
mkdirSecure(SECURITY_DIR);
const tmp = `${STATE_FILE}.tmp.${process.pid}`;
fs.writeFileSync(tmp, JSON.stringify(state, null, 2), { mode: 0o600 });
writeSecureFile(tmp, JSON.stringify(state, null, 2));
fs.renameSync(tmp, STATE_FILE);
} catch (err) {
console.error('[security] writeSessionState failed:', (err as Error).message);
@@ -532,10 +533,10 @@ export interface DecisionRecord {
export function writeDecision(record: DecisionRecord): void {
try {
fs.mkdirSync(DECISIONS_DIR, { recursive: true, mode: 0o700 });
mkdirSecure(DECISIONS_DIR);
const file = decisionFileForTab(record.tabId);
const tmp = `${file}.tmp.${process.pid}`;
fs.writeFileSync(tmp, JSON.stringify(record), { mode: 0o600 });
writeSecureFile(tmp, JSON.stringify(record));
fs.renameSync(tmp, file);
} catch (err) {
console.error('[security] writeDecision failed:', (err as Error).message);