mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-01 10:50:39 +02:00
fix(browse): restrictDirectoryPermissions warns and skips symlinked dirs
Closes the Windows Free Tests red: recent lane failures showed a
platform-unguarded POSIX mode-bit assertion ('Expected: 493' — a
symlink-skip test) from PR-branch variants; the KNOWN_WINDOWS_SAFE
force-include reason ('mode-bitmask hits are POSIX-branch only') did
not hold for that shape, and main had neither the guard nor the
behavior.
- product: lstat first; a symlinked dir gets a warning and a skip on
both platforms — chmod AND icacls dereference the link, so
restricting through a symlink hardens an unvetted target (and
/inheritance:r could lock out its real owner). All callers already
treat hardening as best-effort (try/catch).
- test: the symlink regression test, platform-aware — symlinkSync in
the house try/catch skip pattern (Windows runners without Developer
Mode can't create symlinks), mode-bit assertion guarded off win32,
behavior assertions (no throw, warning text, target readable)
everywhere; POSIX still proves the skip (0o755 unchanged, not 0o700)
- KNOWN_WINDOWS_SAFE reason updated to the now-true premise
20/20 pass on Linux.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
72a5246aae
commit
0d2f703f28
@@ -140,8 +140,29 @@ export function restrictFilePermissions(filePath: string): void {
|
|||||||
* (CI = container inherit) inherit the single-user-full ACL — important
|
* (CI = container inherit) inherit the single-user-full ACL — important
|
||||||
* because child creations in `fs.writeFileSync(...)` without explicit
|
* because child creations in `fs.writeFileSync(...)` without explicit
|
||||||
* `restrictFilePermissions` still end up owner-only.
|
* `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 {
|
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') {
|
if (process.platform === 'win32') {
|
||||||
try {
|
try {
|
||||||
const user = currentUserPrincipal();
|
const user = currentUserPrincipal();
|
||||||
|
|||||||
@@ -9,6 +9,11 @@
|
|||||||
* we verify the helper doesn't throw and the file ends up accessible
|
* we verify the helper doesn't throw and the file ends up accessible
|
||||||
* to the current user — the "doesn't crash, file still usable"
|
* to the current user — the "doesn't crash, file still usable"
|
||||||
* contract the callers rely on.
|
* 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';
|
import { afterEach, beforeEach, describe, expect, test } from 'bun:test';
|
||||||
@@ -79,6 +84,47 @@ describe('restrictDirectoryPermissions', () => {
|
|||||||
expect(() => restrictDirectoryPermissions(d)).not.toThrow();
|
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', () => {
|
test('on Windows, the directory stays usable by the calling process', () => {
|
||||||
if (process.platform !== 'win32') return;
|
if (process.platform !== 'win32') return;
|
||||||
const d = path.join(tmpDir, 'still-usable');
|
const d = path.join(tmpDir, 'still-usable');
|
||||||
|
|||||||
@@ -282,12 +282,18 @@ const KNOWN_WINDOWS_SAFE: Array<{ file: string; reason: string }> = [
|
|||||||
{
|
{
|
||||||
file: 'browse/test/file-permissions.test.ts',
|
file: 'browse/test/file-permissions.test.ts',
|
||||||
// Trips the POSIX-mode-bitmask pattern, but every `mode & 0o777` assertion
|
// 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
|
// This file carries the win32-only icacls-by-SID regression tests, which
|
||||||
// can ONLY execute on windows-latest — excluding it here means the
|
// can ONLY execute on windows-latest — excluding it here means the
|
||||||
// machine-account ACL lockout regression is never exercised on the one
|
// machine-account ACL lockout regression is never exercised on the one
|
||||||
// platform it bricks.
|
// 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',
|
file: 'browse/test/terminal-agent-owner-watchdog.test.ts',
|
||||||
|
|||||||
Reference in New Issue
Block a user