fix(config): enforce strict permission and ownership checks on system-wide configuration paths - #29115

Code Review

This pull request introduces synchronous security validation checks for system configuration files and their parent directories on both Windows and POSIX platforms, ensuring they are owned by trusted accounts and not writable by unprivileged users. The feedback highlights three critical issues: a fail-open vulnerability on Windows if the PowerShell execution fails, a POSIX bug where querying symlinks via lstat always fails due to standard 0777 permissions, and the use of an unbounded global Map for caching which should be replaced with a proper LRU cache.

Code Review

This pull request introduces synchronous security checks for configuration files and their parent directories, integrating them into the settings loading process to prevent loading insecure system settings or defaults. It also adds comprehensive unit tests for these security checks. The review feedback highlights two critical security issues on Windows: first, hardcoded English group names (like 'Users' and 'Everyone') will fail to detect insecure permissions on localized Windows installations, which can be resolved by using language-independent SIDs; second, spawning 'powershell' without an absolute path exposes the application to binary hijacking (CWE-426), which should be mitigated by resolving the absolute path to the system PowerShell executable.

Code Review

This pull request introduces synchronous security validation for system configuration files and their parent directories on both Windows and POSIX platforms, ensuring that insecure configurations are skipped and warnings are logged. A correctness bug was identified in the caching logic of isPathSecureSync, where the cache key only uses the file path and ignores the expectedType constraint, which can lead to incorrect security check results on subsequent calls.

Code Review

This pull request introduces synchronous security checks for system configuration files (system-defaults.json and settings.json) to ensure they and their parent directories are secure on both Windows and POSIX systems. The feedback recommends avoiding module-level global variables for caching, expanding the Windows PowerShell ACL permission checks to include granular rights like CreateFiles and Delete, adding -ExecutionPolicy Bypass to PowerShell invocations to prevent execution failures, and specifying a timeout for synchronous process spawning to avoid potential startup hangs.

Code Review

This pull request introduces synchronous security validation checks (isPathSecureSync and isFileAndDirectorySecureSync) to ensure system configuration files and their parent directories are secure before loading them, skipping insecure files with a warning. Feedback on these changes highlights two critical security issues: the PowerShell script on Windows uses unanchored substring matching (-match) for owner validation, which could allow unauthorized accounts with similar names to bypass the check, and the POSIX permission constants (S_IWGRP and S_IWOTH) may be undefined on non-POSIX or mocked environments, leading to a silent bypass of the permission checks.

Code Review

This pull request introduces synchronous security validation checks (isPathSecureSync and isFileAndDirectorySecureSync) to ensure that system-wide configuration files and their parent directories are secure before loading them, skipping insecure files and recording warnings. It also adds extensive unit tests for these checks. The review feedback highlights three key areas for improvement: refining the fragile PowerShell output parsing to avoid false positives from unexpected stdout lines, avoiding a module-level global cache to prevent race conditions in concurrent environments, and addressing the startup latency on Windows caused by spawning PowerShell synchronously.

Code Review

This pull request introduces robust, synchronous security validation for configuration files and directories across Windows and POSIX platforms. It implements isPathSecureSync and isFileAndDirectorySecureSync to ensure that system configuration files and their parent directories are owned by trusted accounts (root/administrator) and are not writable by unprivileged users, integrating these checks into the CLI settings loading process. The review feedback highlights a security improvement opportunity in isFileAndDirectorySecureSync to resolve symbolic links first using a robust function like resolveToRealPath to prevent potential path traversal vulnerabilities, and to explicitly verify the security of the resolved target file.

Code Review

This pull request introduces robust synchronous path security validation utilities in packages/core, including Windows ACL checking via PowerShell and POSIX ownership/permission checks, and integrates them into the CLI settings loading process to prevent loading insecure system configuration files. It also adds comprehensive unit tests for these security features and updates interactive file system integration tests to reduce flakiness. The code reviewer identified a potential security bypass on Windows where stripping only the '\\?\' prefix from UNC paths (which may be prefixed with '\\?\UNC\') can result in an invalid path format. To address this, the reviewer recommends using a unified, robust path resolution function like resolveToRealPath across all validations.

Code Review

This pull request introduces robust path security validation for system configuration files across Windows and POSIX platforms, implementing synchronous ownership and permission checks alongside caching and path normalization. The reviewer identified a critical security vulnerability in isFileAndDirectorySecureSync, which only checks the immediate parent directory. To prevent privilege escalation via grandparent directory manipulation, the reviewer recommended recursively validating all ancestor directories up to the root and provided a refactored implementation.

_{Note: Security Review did not run due to the size of the PR.}

Code Review

This pull request introduces robust, synchronous security validation for configuration files and their parent directories across Windows and POSIX platforms. It adds path normalization (including stripping Windows extended-length prefixes), batched PowerShell ACL checks on Windows, POSIX ownership/permission checks, and isolated caching. E2E tests and comprehensive unit tests are also added. The review feedback suggests a critical security and robustness improvement: using Base64-encoded UTF-16LE strings with the -EncodedCommand parameter instead of raw -Command in both PowerShell execution points to prevent command-line parsing, quoting, and length limitation issues on Windows.

Code Review

This pull request introduces robust file and directory security validation mechanisms across Windows and POSIX platforms, including synchronous ownership and permission checks, Windows extended-length path prefix stripping, and integration into system settings loading. Feedback from the review highlights a security concern regarding potential binary hijacking when spawning PowerShell with unvalidated environment variables, as well as test pollution issues in the security tests caused by directly mutating the shared node:fs constants.

Code Review

This pull request introduces robust, cross-platform path security validation to ensure that configuration files and their parent directories are secure before loading them. It implements synchronous security checks (isPathSecureSync and isFileAndDirectorySecureSync) for both POSIX and Windows, using PowerShell on Windows to securely batch ACL checks. Additionally, it handles Windows extended-length path prefixes and adds comprehensive unit tests. The reviewer identified a security vulnerability on Windows where grandparent and higher ancestor directories are skipped during validation, potentially allowing directory-replacement or symlink-redirection attacks, and suggested checking all ancestors up to the root (excluding the drive root itself).

Code Review

This pull request introduces synchronous security validation utilities and caching mechanisms to verify that configuration files and their parent directories are secure across Windows and POSIX environments. It updates the settings loading logic to skip insecure system defaults and system settings files, adds helper functions to strip Windows extended-length path prefixes, and includes comprehensive tests. Feedback on the changes highlights a high-severity issue in the PowerShell ACL script where the automatic variable $_ inside a catch block incorrectly references the thrown exception instead of the pipeline object, rendering the fallback check ineffective.

_{Note: Security Review did not run due to the size of the PR.}

Code Review

This pull request introduces robust security validation utilities to verify the ownership and permissions of configuration files and their ancestor directories on both Windows and POSIX systems, ensuring insecure files are skipped with warnings. It also handles Windows extended-length path prefixes and adds comprehensive unit tests. The review feedback highlights a security vulnerability in the SystemRoot validation regex, which is too permissive and could allow binary hijacking or path traversal; restricting it to standard Windows directories is recommended, along with updating the corresponding test cases.