diff --git a/browse/src/file-permissions.ts b/browse/src/file-permissions.ts index 078fcac89..8e04b3222 100644 --- a/browse/src/file-permissions.ts +++ b/browse/src/file-permissions.ts @@ -140,8 +140,29 @@ export function restrictFilePermissions(filePath: string): void { * (CI = container inherit) inherit the single-user-full ACL — important * because child creations in `fs.writeFileSync(...)` without explicit * `restrictFilePermissions` still end up owner-only. + * + * Symlinked dirs are warned about and SKIPPED, never followed: both + * `chmod` and `icacls` dereference the link, so restricting through a + * symlink hardens whatever the link points at — a target the caller never + * vetted (and, with `/inheritance:r`, one we could lock its real owner out + * of). Skipping is best-effort-consistent with the rest of this module: + * the filesystem stays functional, we just don't hit the hardening target. */ export function restrictDirectoryPermissions(dirPath: string): void { + try { + if (fs.lstatSync(dirPath).isSymbolicLink()) { + // biome-ignore lint/suspicious/noConsole: intentional user-facing warning + console.warn( + `[gstack] Refusing to restrict permissions through symlink ${dirPath} — skipping.\n` + + ` Restricting through a symlink would alter the link target instead. ` + + `Harden the real directory directly.` + ); + return; + } + } catch { + // Path doesn't exist (or lstat failed) — fall through; both platform + // branches below already swallow failures on missing paths. + } if (process.platform === 'win32') { try { const user = currentUserPrincipal(); diff --git a/browse/test/file-permissions.test.ts b/browse/test/file-permissions.test.ts index 1d772948b..f6a71e59a 100644 --- a/browse/test/file-permissions.test.ts +++ b/browse/test/file-permissions.test.ts @@ -9,6 +9,11 @@ * 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. + * - Every `mode & 0o777` bitmask assertion is platform-guarded: Windows + * fakes POSIX mode bits (chmod is ~a no-op; dirs stat as 0o777), so a + * bitmask expectation on win32 tests the runner, not our code. Symlink + * fixtures are created in try/catch — Windows runners without Developer + * Mode / admin can't create symlinks, and the test skips gracefully. */ import { afterEach, beforeEach, describe, expect, test } from 'bun:test'; @@ -79,6 +84,47 @@ describe('restrictDirectoryPermissions', () => { expect(() => restrictDirectoryPermissions(d)).not.toThrow(); }); + test('warns and skips a symlinked dir without throwing', () => { + const real = path.join(tmpDir, 'real-target'); + fs.mkdirSync(real); + if (process.platform !== 'win32') { + // chmod, not mkdir({ mode }), so a restrictive umask can't skew the + // starting bits we later assert were left untouched. + fs.chmodSync(real, 0o755); + } + const link = path.join(tmpDir, 'linked'); + try { + fs.symlinkSync(real, link, 'dir'); + } catch { + // Windows runners without Developer Mode / admin can't create + // symlinks (house pattern: security-audit-r2.test.ts skips the same + // way). Nothing to test without the link. + // biome-ignore lint/suspicious/noConsole: test-skip diagnostics + console.warn('Skipping: symlink creation failed (no symlink privilege)'); + return; + } + + const warnings: string[] = []; + const originalWarn = console.warn; + console.warn = (...args: unknown[]) => { warnings.push(args.map(String).join(' ')); }; + try { + expect(() => restrictDirectoryPermissions(link)).not.toThrow(); + } finally { + console.warn = originalWarn; + } + expect(warnings.some((w) => w.includes('symlink'))).toBe(true); + + // The skip must leave the link target untouched. Mode bits are only + // meaningful on POSIX — Windows fakes stat().mode (dirs report 0o777 + // no matter what), so asserting 0o755 there fails on runner semantics, + // not on our behavior. The no-throw + warn + still-usable checks are + // the meaningful win32 contract. + if (process.platform !== 'win32') { + expect(fs.statSync(real).mode & 0o777).toBe(0o755); + } + expect(() => fs.readdirSync(real)).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'); diff --git a/scripts/test-free-shards.ts b/scripts/test-free-shards.ts index 140eac361..83a176cda 100755 --- a/scripts/test-free-shards.ts +++ b/scripts/test-free-shards.ts @@ -282,12 +282,18 @@ const KNOWN_WINDOWS_SAFE: Array<{ file: string; reason: string }> = [ { file: 'browse/test/file-permissions.test.ts', // Trips the POSIX-mode-bitmask pattern, but every `mode & 0o777` assertion - // is platform-guarded (win32 returns early / takes the icacls branch). + // is platform-guarded: win32-only tests return early, POSIX-only tests + // guard the bitmask behind `process.platform !== 'win32'`, and the + // symlink-skip regression test both wraps symlinkSync in try/catch + // (runners without Developer Mode can't create symlinks) and guards its + // bitmask — on win32 it asserts behavior (warns, skips, doesn't throw, + // target stays usable), never fake Windows mode bits (dirs stat 0o777 + // there, so a 0o755 expectation fails on runner semantics, not our code). // This file carries the win32-only icacls-by-SID regression tests, which // can ONLY execute on windows-latest — excluding it here means the // machine-account ACL lockout regression is never exercised on the one // platform it bricks. - reason: 'mode-bitmask hits are POSIX-branch only; win32-only ACL regression tests must run on windows-latest', + reason: 'every mode-bitmask assertion is guarded off win32 (behavior asserted instead); win32-only ACL regression tests must run on windows-latest', }, { file: 'browse/test/terminal-agent-owner-watchdog.test.ts',