From 62147d08e0deb5679000d3d492209534505d58b0 Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Sun, 13 Sep 2026 22:15:12 -0400 Subject: [PATCH 01/10] Fix CodeQL and SonarCloud findings; exclude generated files from CodeQL - Remove useless upcasts and simplify map/filter foreach loops flagged by CodeQL (cs/useless-upcast, cs/linq/missed-select, cs/linq/missed-where) - Guard expensive logging call arguments with ILogger.IsEnabled checks to address CA1873 across AgentToolLauncher, GitClient, LoggingSetup, and Program - Simplify Dictionary field initializers to collection expressions (IDE0028), extract repeated ".github" literal to a constant (S1192), and retype an internal-only logger factory parameter for CA1859 - Harden PackageZipExtractor's zip entry extraction with an inline GetFullPath/StartsWith re-verification against the repo root, in addition to the existing SafePathCombine guard, to address the S6096 zip-slip findings - Assert TryParse return values (CA1806), use Assert.Single's return value instead of re-indexing (xUnit2033), and cache JsonSerializerOptions instances (CA1869) in test code - Add paths-ignore for **/obj/** and **/bin/** to .github/codeql-config.yml (plus a matching query-filter exclude for cs/missed-ternary-operator) so CodeQL no longer scans generated build output, mirroring the ApiMark repository's configuration Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/codeql-config.yml | 10 +++++ .../AgentPackageManagement/PackageSource.cs | 6 +-- .../PackageVersionCache.cs | 6 +-- .../AgentToolLauncher/AgentToolLauncher.cs | 20 +++++---- .../CommittedAgentFilesCache.cs | 2 +- .../GitIntegration/GitClient.cs | 18 +++++--- .../LauncherUI/MainWindowViewModel.cs | 3 +- .../LauncherUI/SelectPackageWindow.axaml.cs | 4 +- .../Logging/LoggingSetup.cs | 13 +++--- src/DemaConsulting.AgentControl/Program.cs | 5 ++- .../RepoSync/PackageZipExtractor.cs | 42 +++++++++++-------- .../PackageVersionTests.cs | 14 +++---- .../LauncherUI/MainWindowViewModelTests.cs | 20 ++++----- .../LauncherUI/RepoCardViewModelTests.cs | 9 ++-- .../SelectPackageWindowViewModelTests.cs | 6 +-- .../Logging/LoggingSetupTests.cs | 5 ++- .../Utilities/UtilitiesSubsystemTests.cs | 7 +--- .../TestSettingsWriter.cs | 14 ++++++- .../TestSupportTypes.cs | 8 +++- 19 files changed, 131 insertions(+), 81 deletions(-) diff --git a/.github/codeql-config.yml b/.github/codeql-config.yml index c6f6375..e31bb2b 100644 --- a/.github/codeql-config.yml +++ b/.github/codeql-config.yml @@ -4,8 +4,18 @@ name: "Agent Control CodeQL Config" +# Globally ignore build output and generated files +paths-ignore: + - '**/obj/**' + - '**/bin/**' + # Query filters to disable specific queries for certain paths query-filters: + # Suppress warnings in all generated build output under obj/ (belt-and-suspenders + # alongside the global paths-ignore above) + - exclude: + id: cs/missed-ternary-operator + paths: ['**/obj/**/*.cs'] - exclude: id: cs/catch-of-all-exceptions paths: diff --git a/src/DemaConsulting.AgentControl/AgentPackageManagement/PackageSource.cs b/src/DemaConsulting.AgentControl/AgentPackageManagement/PackageSource.cs index 4ed368f..298c5a2 100644 --- a/src/DemaConsulting.AgentControl/AgentPackageManagement/PackageSource.cs +++ b/src/DemaConsulting.AgentControl/AgentPackageManagement/PackageSource.cs @@ -161,10 +161,10 @@ public static IReadOnlyList EnumeratePackageNames(string sourceDirectory var names = new HashSet(StringComparer.Ordinal); - foreach (var filePath in Directory.EnumerateFiles(sourceDirectory, "*" + ZipExtension)) + foreach (var fileName in Directory.EnumerateFiles(sourceDirectory, "*" + ZipExtension) + .Select(Path.GetFileNameWithoutExtension)) { - var fileName = Path.GetFileNameWithoutExtension(filePath); - var name = TrySplitNameAndVersion(fileName); + var name = TrySplitNameAndVersion(fileName!); if (name is not null) { names.Add(name); diff --git a/src/DemaConsulting.AgentControl/AgentPackageManagement/PackageVersionCache.cs b/src/DemaConsulting.AgentControl/AgentPackageManagement/PackageVersionCache.cs index 5773e33..453cad6 100644 --- a/src/DemaConsulting.AgentControl/AgentPackageManagement/PackageVersionCache.cs +++ b/src/DemaConsulting.AgentControl/AgentPackageManagement/PackageVersionCache.cs @@ -46,20 +46,20 @@ internal sealed class PackageVersionCache /// name. A cached value (no package found) is a valid, distinct /// cache entry from "not yet queried". /// - private readonly Dictionary<(string SourceDirectory, string PackageName), DiscoveredPackage?> _cache = new(); + private readonly Dictionary<(string SourceDirectory, string PackageName), DiscoveredPackage?> _cache = []; /// /// The cached distinct package base names discoverable at a source directory, keyed by /// source directory. /// - private readonly Dictionary> _packageNamesCache = new(); + private readonly Dictionary> _packageNamesCache = []; /// /// The cached descending-by-version package list for a given (sourceDirectory, /// packageName) pair. /// private readonly Dictionary<(string SourceDirectory, string PackageName), IReadOnlyList> - _versionsDescendingCache = new(); + _versionsDescendingCache = []; /// /// Enumerates the distinct package base names discoverable at a source directory, diff --git a/src/DemaConsulting.AgentControl/AgentToolLauncher/AgentToolLauncher.cs b/src/DemaConsulting.AgentControl/AgentToolLauncher/AgentToolLauncher.cs index 82da968..5745b61 100644 --- a/src/DemaConsulting.AgentControl/AgentToolLauncher/AgentToolLauncher.cs +++ b/src/DemaConsulting.AgentControl/AgentToolLauncher/AgentToolLauncher.cs @@ -139,11 +139,13 @@ public static Process Launch(ProcessStartInfo startInfo, ILogger? logger = null) ArgumentNullException.ThrowIfNull(startInfo); var effectiveLogger = logger ?? AppLogging.Factory.CreateLogger(LoggerCategoryName); - var argumentsText = string.Join(' ', startInfo.ArgumentList); - effectiveLogger.LogDebug( - "Starting shell process '{FileName}' with arguments '{Arguments}' in working directory '{WorkingDirectory}'", - startInfo.FileName, argumentsText, startInfo.WorkingDirectory); + if (effectiveLogger.IsEnabled(LogLevel.Debug)) + { + effectiveLogger.LogDebug( + "Starting shell process '{FileName}' with arguments '{Arguments}' in working directory '{WorkingDirectory}'", + startInfo.FileName, string.Join(' ', startInfo.ArgumentList), startInfo.WorkingDirectory); + } var stopwatch = Stopwatch.StartNew(); try @@ -153,9 +155,12 @@ public static Process Launch(ProcessStartInfo startInfo, ILogger? logger = null) $"Failed to start shell process '{startInfo.FileName}'."); stopwatch.Stop(); - effectiveLogger.LogInformation( - "Started shell process '{FileName} {Arguments}' as PID {ProcessId} after {ElapsedMilliseconds}ms", - startInfo.FileName, argumentsText, process.Id, stopwatch.ElapsedMilliseconds); + if (effectiveLogger.IsEnabled(LogLevel.Information)) + { + effectiveLogger.LogInformation( + "Started shell process '{FileName} {Arguments}' as PID {ProcessId} after {ElapsedMilliseconds}ms", + startInfo.FileName, string.Join(' ', startInfo.ArgumentList), process.Id, stopwatch.ElapsedMilliseconds); + } return process; } @@ -166,6 +171,7 @@ public static Process Launch(ProcessStartInfo startInfo, ILogger? logger = null) // NativeErrorCode is the actual OS error code behind a Win32Exception - the same // critical diagnostic data highlighted as missing for GitClient's intermittent // failures; captured here too since this is another real-process-spawning path. + var argumentsText = string.Join(' ', startInfo.ArgumentList); effectiveLogger.LogError( ex, "Failed to start shell process '{FileName} {Arguments}' after {ElapsedMilliseconds}ms (Win32 NativeErrorCode={NativeErrorCode})", diff --git a/src/DemaConsulting.AgentControl/GitIntegration/CommittedAgentFilesCache.cs b/src/DemaConsulting.AgentControl/GitIntegration/CommittedAgentFilesCache.cs index 4751e0e..3393853 100644 --- a/src/DemaConsulting.AgentControl/GitIntegration/CommittedAgentFilesCache.cs +++ b/src/DemaConsulting.AgentControl/GitIntegration/CommittedAgentFilesCache.cs @@ -40,7 +40,7 @@ internal sealed class CommittedAgentFilesCache /// The cached results, keyed by a normalized repo path and its HEAD commit hash at /// the time the result was recorded. /// - private readonly Dictionary<(string RepoPath, string HeadHash), bool> _cache = new(); + private readonly Dictionary<(string RepoPath, string HeadHash), bool> _cache = []; /// /// Attempts to retrieve a cached result for a repo at a specific HEAD commit hash. diff --git a/src/DemaConsulting.AgentControl/GitIntegration/GitClient.cs b/src/DemaConsulting.AgentControl/GitIntegration/GitClient.cs index 7d7caaa..b687293 100644 --- a/src/DemaConsulting.AgentControl/GitIntegration/GitClient.cs +++ b/src/DemaConsulting.AgentControl/GitIntegration/GitClient.cs @@ -364,9 +364,12 @@ private GitCommandResult RunGit(string repositoryPath, params string[] arguments // of exactly what was about to run and where - the key data point missing from prior // occurrences of the intermittent exit-code -1 failure. var argumentsText = string.Join(' ', arguments); - _logger.LogDebug( - "Starting git process '{GitExecutable}' with arguments '{Arguments}' in working directory '{WorkingDirectory}'", - _gitExecutablePath, argumentsText, repositoryPath); + if (_logger.IsEnabled(LogLevel.Debug)) + { + _logger.LogDebug( + "Starting git process '{GitExecutable}' with arguments '{Arguments}' in working directory '{WorkingDirectory}'", + _gitExecutablePath, argumentsText, repositoryPath); + } var stopwatch = Stopwatch.StartNew(); try @@ -393,9 +396,12 @@ private GitCommandResult RunGit(string repositoryPath, params string[] arguments var stdout = stdoutTask.GetAwaiter().GetResult(); var stderr = stderrTask.GetAwaiter().GetResult(); - _logger.LogInformation( - "git process '{GitExecutable} {Arguments}' exited with code {ExitCode} after {ElapsedMilliseconds}ms", - _gitExecutablePath, argumentsText, process.ExitCode, stopwatch.ElapsedMilliseconds); + if (_logger.IsEnabled(LogLevel.Information)) + { + _logger.LogInformation( + "git process '{GitExecutable} {Arguments}' exited with code {ExitCode} after {ElapsedMilliseconds}ms", + _gitExecutablePath, argumentsText, process.ExitCode, stopwatch.ElapsedMilliseconds); + } if (process.ExitCode != 0) { diff --git a/src/DemaConsulting.AgentControl/LauncherUI/MainWindowViewModel.cs b/src/DemaConsulting.AgentControl/LauncherUI/MainWindowViewModel.cs index 4047da8..6947f05 100644 --- a/src/DemaConsulting.AgentControl/LauncherUI/MainWindowViewModel.cs +++ b/src/DemaConsulting.AgentControl/LauncherUI/MainWindowViewModel.cs @@ -90,9 +90,8 @@ public MainWindowViewModel(AppSettings settings, StartupOptions? startupOptions RepoCards = []; DisplayedRepoCards = []; - foreach (var recentRepo in settings.RecentRepos) + foreach (var card in settings.RecentRepos.Select(CreateCard)) { - var card = CreateCard(recentRepo); AttachCardEvents(card); RepoCards.Add(card); } diff --git a/src/DemaConsulting.AgentControl/LauncherUI/SelectPackageWindow.axaml.cs b/src/DemaConsulting.AgentControl/LauncherUI/SelectPackageWindow.axaml.cs index c6602df..2278235 100644 --- a/src/DemaConsulting.AgentControl/LauncherUI/SelectPackageWindow.axaml.cs +++ b/src/DemaConsulting.AgentControl/LauncherUI/SelectPackageWindow.axaml.cs @@ -60,7 +60,7 @@ public SelectPackageWindow(SelectPackageWindowViewModel viewModel) : this() { ArgumentNullException.ThrowIfNull(viewModel); DataContext = viewModel; - viewModel.Confirmed += (_, result) => Close(((string Name, string Version)?)result); + viewModel.Confirmed += (_, result) => Close(result); } /// @@ -70,6 +70,6 @@ public SelectPackageWindow(SelectPackageWindowViewModel viewModel) : this() /// Routed event arguments (unused). private void CancelButton_Click(object? sender, RoutedEventArgs e) { - Close(((string Name, string Version)?)null); + Close(null); } } diff --git a/src/DemaConsulting.AgentControl/Logging/LoggingSetup.cs b/src/DemaConsulting.AgentControl/Logging/LoggingSetup.cs index 06daea4..a8d3335 100644 --- a/src/DemaConsulting.AgentControl/Logging/LoggingSetup.cs +++ b/src/DemaConsulting.AgentControl/Logging/LoggingSetup.cs @@ -137,16 +137,19 @@ public static ILoggerFactory Initialize(string? configDirectory) /// cannot be recovered, so the handler only logs - it never attempts to suppress /// termination. /// - private static void InstallUnhandledExceptionSafetyNet(ILoggerFactory factory) + private static void InstallUnhandledExceptionSafetyNet(SerilogLoggerFactory factory) { var logger = factory.CreateLogger("DemaConsulting.AgentControl.UnhandledException"); AppDomain.CurrentDomain.UnhandledException += (_, e) => { - logger.LogCritical( - e.ExceptionObject as Exception, - "Unhandled exception reached AppDomain.UnhandledException (IsTerminating={IsTerminating})", - e.IsTerminating); + if (logger.IsEnabled(LogLevel.Critical)) + { + logger.LogCritical( + e.ExceptionObject as Exception, + "Unhandled exception reached AppDomain.UnhandledException (IsTerminating={IsTerminating})", + e.IsTerminating); + } // The process is terminating regardless; flush now so the entry is not lost. Log.CloseAndFlush(); diff --git a/src/DemaConsulting.AgentControl/Program.cs b/src/DemaConsulting.AgentControl/Program.cs index 87a4690..90a2a02 100644 --- a/src/DemaConsulting.AgentControl/Program.cs +++ b/src/DemaConsulting.AgentControl/Program.cs @@ -111,7 +111,10 @@ public static int Main(string[] args) try { - logger.LogInformation("Starting AgentControl {Version}", Version); + if (logger.IsEnabled(LogLevel.Information)) + { + logger.LogInformation("Starting AgentControl {Version}", Version); + } // Attach the parsed options for App.OnFrameworkInitializationCompleted to consume, // then hand off to Avalonia's classic desktop lifetime for the remainder of the diff --git a/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs b/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs index 2bf78e4..8587cbd 100644 --- a/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs +++ b/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs @@ -40,16 +40,21 @@ namespace DemaConsulting.AgentControl.RepoSync; /// internal static class PackageZipExtractor { + /// + /// Name of the root .github folder under which every managed agent folder lives. + /// + private const string GitHubFolderName = ".github"; + /// /// The four agent folders (relative to a repo root) that are blind-deleted and replaced /// on every sync/upgrade, per architecture.md. /// private static readonly string[] ManagedFolders = [ - Path.Combine(".github", "agents"), - Path.Combine(".github", "standards"), - Path.Combine(".github", "templates"), - Path.Combine(".github", "skills") + Path.Combine(GitHubFolderName, "agents"), + Path.Combine(GitHubFolderName, "standards"), + Path.Combine(GitHubFolderName, "templates"), + Path.Combine(GitHubFolderName, "skills") ]; /// @@ -233,6 +238,19 @@ private static void ExtractEntryIfManaged(ZipArchiveEntry entry, string repoRoot } var destinationPath = PathHelpers.SafePathCombine(repoRoot, relativePath); + + // Zip-slip defense: re-verify (in addition to SafePathCombine's own check) that the + // entry's resolved destination is still contained within the repo root before any + // directory is created or file written, so this is visibly safe at the write site itself + // rather than relying solely on the called helper. + var resolvedRepoRoot = Path.GetFullPath(repoRoot); + var resolvedDestinationPath = Path.GetFullPath(destinationPath); + if (!resolvedDestinationPath.StartsWith(resolvedRepoRoot + Path.DirectorySeparatorChar, StringComparison.Ordinal)) + { + throw new InvalidOperationException( + $"Zip entry '{entry.FullName}' resolves outside the repository root."); + } + var destinationDirectory = Path.GetDirectoryName(destinationPath); if (!string.IsNullOrEmpty(destinationDirectory)) { @@ -249,17 +267,7 @@ private static void ExtractEntryIfManaged(ZipArchiveEntry entry, string repoRoot /// separators. /// if the path is inside a managed folder; otherwise /// . - private static bool IsInsideManagedFolder(string relativePath) - { - foreach (var folder in ManagedFolders) - { - var prefix = folder + Path.DirectorySeparatorChar; - if (relativePath.StartsWith(prefix, StringComparison.Ordinal)) - { - return true; - } - } - - return false; - } + private static bool IsInsideManagedFolder(string relativePath) => + ManagedFolders.Any(folder => + relativePath.StartsWith(folder + Path.DirectorySeparatorChar, StringComparison.Ordinal)); } diff --git a/test/DemaConsulting.AgentControl.Tests/AgentPackageManagement/PackageVersionTests.cs b/test/DemaConsulting.AgentControl.Tests/AgentPackageManagement/PackageVersionTests.cs index 6870098..369803f 100644 --- a/test/DemaConsulting.AgentControl.Tests/AgentPackageManagement/PackageVersionTests.cs +++ b/test/DemaConsulting.AgentControl.Tests/AgentPackageManagement/PackageVersionTests.cs @@ -87,8 +87,8 @@ public void PackageVersion_TryParse_MalformedInput_ReturnsFalse(string? input) public void PackageVersion_CompareTo_HigherNumericVersion_ComparesGreater() { // Arrange: two release versions - PackageVersion.TryParse("2.0.0", out var higher); - PackageVersion.TryParse("1.9.9", out var lower); + Assert.True(PackageVersion.TryParse("2.0.0", out var higher)); + Assert.True(PackageVersion.TryParse("1.9.9", out var lower)); Assert.NotNull(higher); Assert.NotNull(lower); @@ -105,8 +105,8 @@ public void PackageVersion_CompareTo_HigherNumericVersion_ComparesGreater() public void PackageVersion_CompareTo_ReleaseVsPrereleaseSameNumeric_ReleaseIsGreater() { // Arrange: a release and a prerelease sharing the same major.minor.patch - PackageVersion.TryParse("1.0.0", out var release); - PackageVersion.TryParse("1.0.0-beta", out var prerelease); + Assert.True(PackageVersion.TryParse("1.0.0", out var release)); + Assert.True(PackageVersion.TryParse("1.0.0-beta", out var prerelease)); Assert.NotNull(release); // Act / Assert: the release outranks the prerelease @@ -120,8 +120,8 @@ public void PackageVersion_CompareTo_ReleaseVsPrereleaseSameNumeric_ReleaseIsGre public void PackageVersion_Equals_SameVersionString_ReturnsTrue() { // Arrange: two independently parsed instances of the same version - PackageVersion.TryParse("3.4.5", out var first); - PackageVersion.TryParse("3.4.5", out var second); + Assert.True(PackageVersion.TryParse("3.4.5", out var first)); + Assert.True(PackageVersion.TryParse("3.4.5", out var second)); Assert.NotNull(first); // Act / Assert: they compare equal via both Equals and == @@ -139,7 +139,7 @@ public void PackageVersion_Equals_SameVersionString_ReturnsTrue() public void PackageVersion_ToString_ParsedVersion_RoundTripsThroughParse(string input) { // Arrange: parse the input - PackageVersion.TryParse(input, out var version); + Assert.True(PackageVersion.TryParse(input, out var version)); Assert.NotNull(version); // Act: format it back to a string diff --git a/test/DemaConsulting.AgentControl.Tests/LauncherUI/MainWindowViewModelTests.cs b/test/DemaConsulting.AgentControl.Tests/LauncherUI/MainWindowViewModelTests.cs index 5654605..63f196c 100644 --- a/test/DemaConsulting.AgentControl.Tests/LauncherUI/MainWindowViewModelTests.cs +++ b/test/DemaConsulting.AgentControl.Tests/LauncherUI/MainWindowViewModelTests.cs @@ -86,12 +86,12 @@ public void MainWindowViewModel_AddRepo_ValidNewPath_InsertsCardAndPersistsSetti // Assert: the card was added at the front, and settings were persisted Assert.True(added); - Assert.Single(viewModel.RepoCards); - Assert.Equal(newRepo, viewModel.RepoCards[0].RepoPath); + var card = Assert.Single(viewModel.RepoCards); + Assert.Equal(newRepo, card.RepoPath); var reloaded = SettingsStore.Load(configDir); - Assert.Single(reloaded.RecentRepos); - Assert.Equal(newRepo, reloaded.RecentRepos[0].Path); + var recentRepo = Assert.Single(reloaded.RecentRepos); + Assert.Equal(newRepo, recentRepo.Path); } /// @@ -151,14 +151,14 @@ public void MainWindowViewModel_ApplySettings_PreservesRecentReposAndPersistsUpd // Assert: the recent-repos list survived, the new field was adopted, and both were // persisted to disk - Assert.Single(viewModel.RepoCards); - Assert.Equal(repoPath, viewModel.RepoCards[0].RepoPath); + var card = Assert.Single(viewModel.RepoCards); + Assert.Equal(repoPath, card.RepoPath); Assert.Equal(@"\\share\packages", viewModel.Settings.PackageSourcePath); var reloaded = SettingsStore.Load(configDir); Assert.Equal(@"\\share\packages", reloaded.PackageSourcePath); - Assert.Single(reloaded.RecentRepos); - Assert.Equal(repoPath, reloaded.RecentRepos[0].Path); + var recentRepo = Assert.Single(reloaded.RecentRepos); + Assert.Equal(repoPath, recentRepo.Path); } /// @@ -178,8 +178,8 @@ public void MainWindowViewModel_FilterText_MatchesRepoNameCaseInsensitive_Filter viewModel.FilterText = "aLpHa"; // Assert: only the alpha repo remains displayed - Assert.Single(viewModel.DisplayedRepoCards); - Assert.Equal(alphaRepo, viewModel.DisplayedRepoCards[0].RepoPath); + var displayed = Assert.Single(viewModel.DisplayedRepoCards); + Assert.Equal(alphaRepo, displayed.RepoPath); } /// diff --git a/test/DemaConsulting.AgentControl.Tests/LauncherUI/RepoCardViewModelTests.cs b/test/DemaConsulting.AgentControl.Tests/LauncherUI/RepoCardViewModelTests.cs index 65b8777..a33ffb7 100644 --- a/test/DemaConsulting.AgentControl.Tests/LauncherUI/RepoCardViewModelTests.cs +++ b/test/DemaConsulting.AgentControl.Tests/LauncherUI/RepoCardViewModelTests.cs @@ -440,7 +440,7 @@ public void RepoCardViewModel_RefreshCheap_DoesNotRunWorkingTreeDirtyCheck() /// Reads the lines appended by a created with an invocation-log /// path, tolerating the log file not existing yet. /// - private static IReadOnlyList ReadInvocationLog(string path) => + private static string[] ReadInvocationLog(string path) => File.Exists(path) ? File.ReadAllLines(path) : []; /// @@ -569,8 +569,7 @@ public void RepoCardViewModel_RemoveCommand_RaisesRemoveRequestedWithoutMutating // Assert: the event fired exactly once, with this card as the sender, and nothing else // was mutated (there is no collection for this view model to mutate directly). - Assert.Single(raisedWith); - Assert.Same(card, raisedWith[0]); + Assert.Same(card, Assert.Single(raisedWith)); } /// @@ -671,8 +670,8 @@ public void RepoCardViewModel_SelectPackageCommand_SourceConfigured_RaisesSelect card.SelectPackageCommand.Execute(null); // Assert - Assert.Single(raisedWith); - Assert.Equal(sourceDir, raisedWith[0]); + var raisedSource = Assert.Single(raisedWith); + Assert.Equal(sourceDir, raisedSource); } /// diff --git a/test/DemaConsulting.AgentControl.Tests/LauncherUI/SelectPackageWindowViewModelTests.cs b/test/DemaConsulting.AgentControl.Tests/LauncherUI/SelectPackageWindowViewModelTests.cs index f4ebd77..9c25753 100644 --- a/test/DemaConsulting.AgentControl.Tests/LauncherUI/SelectPackageWindowViewModelTests.cs +++ b/test/DemaConsulting.AgentControl.Tests/LauncherUI/SelectPackageWindowViewModelTests.cs @@ -128,9 +128,9 @@ public void SelectPackageWindowViewModel_ConfirmCommand_RaisesConfirmedWithSelec viewModel.ConfirmCommand.Execute(null); // Assert - Assert.Single(raised); - Assert.Equal("contoso-agents", raised[0].Name); - Assert.Equal("1.0.0", raised[0].Version); + var confirmed = Assert.Single(raised); + Assert.Equal("contoso-agents", confirmed.Name); + Assert.Equal("1.0.0", confirmed.Version); } /// diff --git a/test/DemaConsulting.AgentControl.Tests/Logging/LoggingSetupTests.cs b/test/DemaConsulting.AgentControl.Tests/Logging/LoggingSetupTests.cs index c5141e4..e7ad72d 100644 --- a/test/DemaConsulting.AgentControl.Tests/Logging/LoggingSetupTests.cs +++ b/test/DemaConsulting.AgentControl.Tests/Logging/LoggingSetupTests.cs @@ -93,7 +93,10 @@ public void LoggingSetup_Initialize_LogEntryWritten_AppearsInLogFileUnderLogsSub // Act: write a log entry through the same Serilog pipeline the fixture initialized via // LoggingSetup.Initialize - logger.LogInformation("Test marker: {Marker}", marker); + if (logger.IsEnabled(LogLevel.Information)) + { + logger.LogInformation("Test marker: {Marker}", marker); + } // Assert: exactly one rolling log file exists under the documented "logs" subfolder // (Serilog's daily rolling has not yet rolled a second file within this test run), and diff --git a/test/DemaConsulting.AgentControl.Tests/Utilities/UtilitiesSubsystemTests.cs b/test/DemaConsulting.AgentControl.Tests/Utilities/UtilitiesSubsystemTests.cs index 83ab93d..c7310c0 100644 --- a/test/DemaConsulting.AgentControl.Tests/Utilities/UtilitiesSubsystemTests.cs +++ b/test/DemaConsulting.AgentControl.Tests/Utilities/UtilitiesSubsystemTests.cs @@ -137,12 +137,9 @@ public void UtilitiesSubsystem_DirectoryCreationWorkflow_ValidPaths_CreatesDirec finally { // Cleanup: delete only the root directories created by this test - foreach (var rootDir in new[] { rootDir1, rootDir2 }) + foreach (var rootDir in new[] { rootDir1, rootDir2 }.Where(Directory.Exists)) { - if (Directory.Exists(rootDir)) - { - Directory.Delete(rootDir, true); - } + Directory.Delete(rootDir, true); } } } diff --git a/test/DemaConsulting.AgentControl.UiTests/TestSettingsWriter.cs b/test/DemaConsulting.AgentControl.UiTests/TestSettingsWriter.cs index 3dd482f..e8b9f87 100644 --- a/test/DemaConsulting.AgentControl.UiTests/TestSettingsWriter.cs +++ b/test/DemaConsulting.AgentControl.UiTests/TestSettingsWriter.cs @@ -37,6 +37,12 @@ namespace DemaConsulting.AgentControl.UiTests; /// internal static class TestSettingsWriter { + /// + /// Shared options instance for the JSON serialization performed by this writer, avoiding + /// a fresh allocation on every call. + /// + private static readonly JsonSerializerOptions SerializerOptions = new() { WriteIndented = true }; + /// /// Writes a settings.json file under pointing /// the git executable and the (custom) agent-tool command at the arg-logger stub, and @@ -64,13 +70,17 @@ public static void Write( { Directory.CreateDirectory(configDirectory); + // Never set by this test helper; a typed local (rather than a cast null literal) gives + // the anonymous type property below a type without an unnecessary upcast. + string? shellPreference = null; + var settings = new { PackageSourcePath = packageSourcePath, GitExecutablePath = argLoggerStubExePath, AgentTool = 3, // AgentToolKind.Custom CustomAgentCommand = $"\"{argLoggerStubExePath}\"", - ShellPreference = (string?)null, + ShellPreference = shellPreference, RecentRepos = new[] { new @@ -82,7 +92,7 @@ public static void Write( } }; - var json = JsonSerializer.Serialize(settings, new JsonSerializerOptions { WriteIndented = true }); + var json = JsonSerializer.Serialize(settings, SerializerOptions); File.WriteAllText(Path.Combine(configDirectory, "settings.json"), json); } } diff --git a/test/DemaConsulting.AgentControl.UiTests/TestSupportTypes.cs b/test/DemaConsulting.AgentControl.UiTests/TestSupportTypes.cs index bd29371..83b9173 100644 --- a/test/DemaConsulting.AgentControl.UiTests/TestSupportTypes.cs +++ b/test/DemaConsulting.AgentControl.UiTests/TestSupportTypes.cs @@ -28,6 +28,12 @@ namespace DemaConsulting.AgentControl.UiTests; /// internal static class TestRepoPinWriter { + /// + /// Shared options instance for the JSON serialization performed by this writer, avoiding + /// a fresh allocation on every call. + /// + private static readonly JsonSerializerOptions SerializerOptions = new() { WriteIndented = true }; + /// /// Writes .agentcontrol.json at the root of . /// @@ -37,7 +43,7 @@ internal static class TestRepoPinWriter public static void Write(string repoPath, string packageName, string version) { var pin = new { PackageName = packageName, Version = version }; - var json = JsonSerializer.Serialize(pin, new JsonSerializerOptions { WriteIndented = true }); + var json = JsonSerializer.Serialize(pin, SerializerOptions); File.WriteAllText(Path.Combine(repoPath, ".agentcontrol.json"), json); } } From 386deaef332162ffa951a2ca43648ff07f6a5aa1 Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Sun, 13 Sep 2026 22:34:52 -0400 Subject: [PATCH 02/10] Address automated review feedback on zip-slip and logging fixes - PackageZipExtractor: rely on PathHelpers.SafePathCombine as the single source of truth for path-containment validation instead of re-deriving the same relative-path check by hand, which had a bug rejecting repo roots ending in a separator (e.g. a drive root). Also add EnsureNoSymlinkAncestors to reject extraction through an existing symlinked/junctioned ancestor directory, since Path.GetFullPath performs only lexical normalization and never resolves filesystem links. - GitClient.RunGit: make the joined arguments string lazily computed via Lazy so it is only evaluated once, and only when an enabled log statement (or the failure path) actually needs it - the prior IsEnabled(LogLevel.Debug) guard did not help because the string.Join was still being evaluated unconditionally beforehand. Verified with a local Sonnet 5 code-review pass plus a full build and test run (192/192 passing). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../GitIntegration/GitClient.cs | 17 ++--- .../RepoSync/PackageZipExtractor.cs | 65 ++++++++++++++++--- 2 files changed, 64 insertions(+), 18 deletions(-) diff --git a/src/DemaConsulting.AgentControl/GitIntegration/GitClient.cs b/src/DemaConsulting.AgentControl/GitIntegration/GitClient.cs index b687293..2f8a444 100644 --- a/src/DemaConsulting.AgentControl/GitIntegration/GitClient.cs +++ b/src/DemaConsulting.AgentControl/GitIntegration/GitClient.cs @@ -362,13 +362,14 @@ private GitCommandResult RunGit(string repositoryPath, params string[] arguments // Logged before the process starts so a crash/hang mid-invocation still leaves a record // of exactly what was about to run and where - the key data point missing from prior - // occurrences of the intermittent exit-code -1 failure. - var argumentsText = string.Join(' ', arguments); + // occurrences of the intermittent exit-code -1 failure. Lazily computed so the join is + // only ever performed once, and only if some enabled log statement actually needs it. + var argumentsText = new Lazy(() => string.Join(' ', arguments)); if (_logger.IsEnabled(LogLevel.Debug)) { _logger.LogDebug( "Starting git process '{GitExecutable}' with arguments '{Arguments}' in working directory '{WorkingDirectory}'", - _gitExecutablePath, argumentsText, repositoryPath); + _gitExecutablePath, argumentsText.Value, repositoryPath); } var stopwatch = Stopwatch.StartNew(); @@ -390,7 +391,7 @@ private GitCommandResult RunGit(string repositoryPath, params string[] arguments { _logger.LogWarning( "git process '{GitExecutable} {Arguments}' took an unexpectedly long {ElapsedMilliseconds}ms to exit", - _gitExecutablePath, argumentsText, stopwatch.ElapsedMilliseconds); + _gitExecutablePath, argumentsText.Value, stopwatch.ElapsedMilliseconds); } var stdout = stdoutTask.GetAwaiter().GetResult(); @@ -400,14 +401,14 @@ private GitCommandResult RunGit(string repositoryPath, params string[] arguments { _logger.LogInformation( "git process '{GitExecutable} {Arguments}' exited with code {ExitCode} after {ElapsedMilliseconds}ms", - _gitExecutablePath, argumentsText, process.ExitCode, stopwatch.ElapsedMilliseconds); + _gitExecutablePath, argumentsText.Value, process.ExitCode, stopwatch.ElapsedMilliseconds); } if (process.ExitCode != 0) { _logger.LogWarning( "git process '{GitExecutable} {Arguments}' failed with exit code {ExitCode}; stderr: {StandardError}", - _gitExecutablePath, argumentsText, process.ExitCode, stderr); + _gitExecutablePath, argumentsText.Value, process.ExitCode, stderr); } return new GitCommandResult(process.ExitCode, stdout, stderr); @@ -426,10 +427,10 @@ private GitCommandResult RunGit(string repositoryPath, params string[] arguments _logger.LogError( ex, "Failed to run '{GitExecutable} {Arguments}' after {ElapsedMilliseconds}ms (Win32 NativeErrorCode={NativeErrorCode})", - _gitExecutablePath, argumentsText, stopwatch.ElapsedMilliseconds, nativeErrorCode); + _gitExecutablePath, argumentsText.Value, stopwatch.ElapsedMilliseconds, nativeErrorCode); throw new InvalidOperationException( - $"Failed to run '{_gitExecutablePath} {argumentsText}': {ex.Message}", ex); + $"Failed to run '{_gitExecutablePath} {argumentsText.Value}': {ex.Message}", ex); } } } diff --git a/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs b/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs index 8587cbd..4e7aa49 100644 --- a/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs +++ b/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs @@ -237,29 +237,74 @@ private static void ExtractEntryIfManaged(ZipArchiveEntry entry, string repoRoot return; } - var destinationPath = PathHelpers.SafePathCombine(repoRoot, relativePath); - - // Zip-slip defense: re-verify (in addition to SafePathCombine's own check) that the - // entry's resolved destination is still contained within the repo root before any - // directory is created or file written, so this is visibly safe at the write site itself - // rather than relying solely on the called helper. - var resolvedRepoRoot = Path.GetFullPath(repoRoot); - var resolvedDestinationPath = Path.GetFullPath(destinationPath); - if (!resolvedDestinationPath.StartsWith(resolvedRepoRoot + Path.DirectorySeparatorChar, StringComparison.Ordinal)) + // PathHelpers.SafePathCombine is the single source of truth for containment validation + // (see its own doc remarks); it throws ArgumentException if the entry's path would + // resolve outside repoRoot, so it is not re-derived here by hand. + string destinationPath; + try + { + destinationPath = PathHelpers.SafePathCombine(repoRoot, relativePath); + } + catch (ArgumentException ex) { throw new InvalidOperationException( - $"Zip entry '{entry.FullName}' resolves outside the repository root."); + $"Zip entry '{entry.FullName}' resolves outside the repository root.", ex); } var destinationDirectory = Path.GetDirectoryName(destinationPath); if (!string.IsNullOrEmpty(destinationDirectory)) { + // Path.GetFullPath (used internally by SafePathCombine) performs lexical + // normalization only - it does not resolve filesystem links - so a symlinked + // ancestor directory could otherwise still cause extraction to escape the repo root + // despite the containment check above passing. Reject any ancestor between the repo + // root and the destination that is itself a reparse point (symlink/junction) before + // creating anything. + EnsureNoSymlinkAncestors(Path.GetFullPath(repoRoot), destinationDirectory, entry.FullName); + Directory.CreateDirectory(destinationDirectory); } entry.ExtractToFile(destinationPath, overwrite: true); } + /// + /// Walks upward from to , + /// rejecting extraction if any existing ancestor directory in between is itself a + /// reparse point (symlink/junction). + /// + /// The already-resolved () repo + /// root; the walk stops here. + /// The zip entry's destination directory. + /// The zip entry's name, for the exception message. + /// + /// is purely lexical - it never resolves + /// filesystem links - so the earlier relative-path containment check alone cannot detect + /// a symlinked ancestor redirecting a nominally-contained path outside the repo root. This + /// only guards against ancestors that already exist at the time of the check; it does not + /// eliminate a race where an ancestor is replaced with a symlink between this check and + /// /. + /// + /// Thrown when an ancestor directory is a + /// reparse point. + private static void EnsureNoSymlinkAncestors(string repoRoot, string destinationDirectory, string entryName) + { + var current = Path.TrimEndingDirectorySeparator(destinationDirectory); + var normalizedRoot = Path.TrimEndingDirectorySeparator(repoRoot); + + while (!string.IsNullOrEmpty(current) + && !string.Equals(current, normalizedRoot, StringComparison.OrdinalIgnoreCase)) + { + if (Directory.Exists(current) && File.GetAttributes(current).HasFlag(FileAttributes.ReparsePoint)) + { + throw new InvalidOperationException( + $"Zip entry '{entryName}' extracts through a symlinked directory '{current}'."); + } + + current = Path.GetDirectoryName(current); + } + } + /// /// Determines whether a zip-relative path falls within one of the four managed folders. /// From 507c12042a5be298b3b17e5fb12e4279e52ee480 Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Sun, 13 Sep 2026 22:55:06 -0400 Subject: [PATCH 03/10] Address third round of PR review feedback - PackageZipExtractor: derive the managed-folder membership check from the canonical (SafePathCombine-resolved) relative path instead of the raw zip-entry path, closing a bypass where an entry such as '.github/agents/../../outside.txt' textually matched a managed-folder prefix but resolved outside every managed folder. - AgentToolLauncher: share a single lazily-computed arguments string across the Debug/Information log guards and the failure path instead of recomputing string.Join separately in each branch (both are enabled by default, so the prior fix computed it twice on every launch). - GitClient: guard the slow-exit and nonzero-exit-code LogWarning calls with IsEnabled(LogLevel.Warning) so the lazily-computed arguments text is not forced when warning logging is disabled. - Add regression tests: a crafted-traversal-entry test for the managed-folder bypass, and a Windows-only junction-ancestor test (skipped on non-Windows runners) verifying extraction through a symlinked/junctioned managed folder is refused. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../AgentToolLauncher/AgentToolLauncher.cs | 14 ++- .../GitIntegration/GitClient.cs | 4 +- .../RepoSync/PackageZipExtractor.cs | 18 ++- .../RepoSync/PackageZipExtractorTests.cs | 106 ++++++++++++++++++ 4 files changed, 129 insertions(+), 13 deletions(-) diff --git a/src/DemaConsulting.AgentControl/AgentToolLauncher/AgentToolLauncher.cs b/src/DemaConsulting.AgentControl/AgentToolLauncher/AgentToolLauncher.cs index 5745b61..27ed563 100644 --- a/src/DemaConsulting.AgentControl/AgentToolLauncher/AgentToolLauncher.cs +++ b/src/DemaConsulting.AgentControl/AgentToolLauncher/AgentToolLauncher.cs @@ -140,11 +140,16 @@ public static Process Launch(ProcessStartInfo startInfo, ILogger? logger = null) var effectiveLogger = logger ?? AppLogging.Factory.CreateLogger(LoggerCategoryName); + // Lazily computed so the join is performed at most once per call, and only if some + // enabled log statement (or the failure path) actually needs it - avoids formatting the + // same argument list twice when both Debug and Information logging are enabled. + var argumentsText = new Lazy(() => string.Join(' ', startInfo.ArgumentList)); + if (effectiveLogger.IsEnabled(LogLevel.Debug)) { effectiveLogger.LogDebug( "Starting shell process '{FileName}' with arguments '{Arguments}' in working directory '{WorkingDirectory}'", - startInfo.FileName, string.Join(' ', startInfo.ArgumentList), startInfo.WorkingDirectory); + startInfo.FileName, argumentsText.Value, startInfo.WorkingDirectory); } var stopwatch = Stopwatch.StartNew(); @@ -159,7 +164,7 @@ public static Process Launch(ProcessStartInfo startInfo, ILogger? logger = null) { effectiveLogger.LogInformation( "Started shell process '{FileName} {Arguments}' as PID {ProcessId} after {ElapsedMilliseconds}ms", - startInfo.FileName, string.Join(' ', startInfo.ArgumentList), process.Id, stopwatch.ElapsedMilliseconds); + startInfo.FileName, argumentsText.Value, process.Id, stopwatch.ElapsedMilliseconds); } return process; @@ -171,14 +176,13 @@ public static Process Launch(ProcessStartInfo startInfo, ILogger? logger = null) // NativeErrorCode is the actual OS error code behind a Win32Exception - the same // critical diagnostic data highlighted as missing for GitClient's intermittent // failures; captured here too since this is another real-process-spawning path. - var argumentsText = string.Join(' ', startInfo.ArgumentList); effectiveLogger.LogError( ex, "Failed to start shell process '{FileName} {Arguments}' after {ElapsedMilliseconds}ms (Win32 NativeErrorCode={NativeErrorCode})", - startInfo.FileName, argumentsText, stopwatch.ElapsedMilliseconds, ex.NativeErrorCode); + startInfo.FileName, argumentsText.Value, stopwatch.ElapsedMilliseconds, ex.NativeErrorCode); throw new InvalidOperationException( - $"Failed to start shell process '{startInfo.FileName} {argumentsText}': {ex.Message}", ex); + $"Failed to start shell process '{startInfo.FileName} {argumentsText.Value}': {ex.Message}", ex); } } } diff --git a/src/DemaConsulting.AgentControl/GitIntegration/GitClient.cs b/src/DemaConsulting.AgentControl/GitIntegration/GitClient.cs index 2f8a444..91390a2 100644 --- a/src/DemaConsulting.AgentControl/GitIntegration/GitClient.cs +++ b/src/DemaConsulting.AgentControl/GitIntegration/GitClient.cs @@ -387,7 +387,7 @@ private GitCommandResult RunGit(string repositoryPath, params string[] arguments // A long WaitForExit points at a hung/slow process rather than a fast failure - // exactly the ambiguity the flaky-test investigation could not previously resolve. - if (stopwatch.Elapsed > SlowExitWarningThreshold) + if (stopwatch.Elapsed > SlowExitWarningThreshold && _logger.IsEnabled(LogLevel.Warning)) { _logger.LogWarning( "git process '{GitExecutable} {Arguments}' took an unexpectedly long {ElapsedMilliseconds}ms to exit", @@ -404,7 +404,7 @@ private GitCommandResult RunGit(string repositoryPath, params string[] arguments _gitExecutablePath, argumentsText.Value, process.ExitCode, stopwatch.ElapsedMilliseconds); } - if (process.ExitCode != 0) + if (process.ExitCode != 0 && _logger.IsEnabled(LogLevel.Warning)) { _logger.LogWarning( "git process '{GitExecutable} {Arguments}' failed with exit code {ExitCode}; stderr: {StandardError}", diff --git a/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs b/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs index 4e7aa49..6b622d1 100644 --- a/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs +++ b/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs @@ -229,13 +229,8 @@ private static void ExtractEntryIfManaged(ZipArchiveEntry entry, string repoRoot return; } - // Zip entries always use '/' regardless of platform; normalize before comparing against - // the OS-specific managed folder prefixes. + // Zip entries always use '/' regardless of platform; normalize before combining. var relativePath = entry.FullName.Replace('/', Path.DirectorySeparatorChar); - if (!IsInsideManagedFolder(relativePath)) - { - return; - } // PathHelpers.SafePathCombine is the single source of truth for containment validation // (see its own doc remarks); it throws ArgumentException if the entry's path would @@ -251,6 +246,17 @@ private static void ExtractEntryIfManaged(ZipArchiveEntry entry, string repoRoot $"Zip entry '{entry.FullName}' resolves outside the repository root.", ex); } + // The managed-folder membership check must run against the *canonical* (".."-resolved) + // relative path, not the raw entry-supplied one: a crafted entry such as + // ".github/agents/../../outside.txt" textually starts with a managed-folder prefix but + // resolves elsewhere. Deriving the relative path from the already-validated + // destinationPath closes that gap. + var canonicalRelativePath = Path.GetRelativePath(Path.GetFullPath(repoRoot), destinationPath); + if (!IsInsideManagedFolder(canonicalRelativePath)) + { + return; + } + var destinationDirectory = Path.GetDirectoryName(destinationPath); if (!string.IsNullOrEmpty(destinationDirectory)) { diff --git a/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs b/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs index e84e4ad..c0749d9 100644 --- a/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs +++ b/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs @@ -18,6 +18,7 @@ // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE // SOFTWARE. +using System.Diagnostics; using System.IO.Compression; using DemaConsulting.AgentControl.RepoSync; @@ -224,6 +225,111 @@ public void PackageZipExtractor_AllManagedFoldersExist_NoneExist_ReturnsFalse() } } + /// + /// Test that a zip entry whose name textually starts with a managed-folder prefix but + /// uses ".." components to resolve to a location outside every managed folder (while + /// still remaining under the repo root) is not extracted anywhere - closing the traversal + /// bypass where the managed-folder membership check ran against the raw, unnormalized + /// entry path instead of its canonical (".."-resolved) form. + /// + [Fact] + public void PackageZipExtractor_Extract_TraversalEntryWithinRepoRoot_DoesNotEscapeManagedFolders() + { + // Arrange: an entry that textually starts with ".github/agents/" but resolves (via "..") + // to a repo-root-level file outside every managed folder + var zipPath = Path.Combine(Path.GetTempPath(), "agentcontrol_package_" + Guid.NewGuid() + ".zip"); + using (var archive = ZipFile.Open(zipPath, ZipArchiveMode.Create)) + { + var entry = archive.CreateEntry(".github/agents/../../outside.txt"); + using var writer = new StreamWriter(entry.Open()); + writer.Write("should not be extracted"); + } + + var repoRoot = CreateTempDirectory(); + try + { + // Act: extract the crafted package + PackageZipExtractor.Extract(zipPath, repoRoot); + + // Assert: the traversal entry was skipped, not extracted to the repo root + Assert.False(File.Exists(Path.Combine(repoRoot, "outside.txt"))); + } + finally + { + File.Delete(zipPath); + Directory.Delete(repoRoot, recursive: true); + } + } + + /// + /// Test that extracting into a repo whose .github folder is a junction (reparse + /// point) pointing outside the repo root is refused, rather than silently writing through + /// the junction to the linked-to location. + /// + [Fact] + public void PackageZipExtractor_Extract_ManagedFolderAncestorIsJunction_ThrowsAndDoesNotWriteThroughLink() + { + // NTFS directory junctions (and the 'mklink /J' tool used to create them) are a + // Windows-only concept; this test project also runs on Linux/macOS CI runners, so skip + // there rather than shelling out to a nonexistent 'cmd.exe'. + if (!OperatingSystem.IsWindows()) + { + Assert.Skip("Directory junctions are a Windows-only filesystem feature."); + } + + // Arrange: a repo root whose ".github" entry is a junction to a separate, isolated + // directory standing in for a location outside the repo + var zipPath = CreatePackageZip(("agents/copilot.md", "should not be extracted")); + var repoRoot = CreateTempDirectory(); + var linkTarget = CreateTempDirectory(); + try + { + CreateJunction(Path.Combine(repoRoot, ".github"), linkTarget); + + // Act / Assert: extraction is refused rather than writing through the junction + Assert.Throws(() => PackageZipExtractor.Extract(zipPath, repoRoot)); + Assert.False(File.Exists(Path.Combine(linkTarget, "agents", "copilot.md"))); + } + finally + { + File.Delete(zipPath); + + // The ".github" junction entry itself must be removed (not recursively, since that + // would delete the link target's contents) before the repo root can be deleted. + Directory.Delete(Path.Combine(repoRoot, ".github")); + Directory.Delete(repoRoot, recursive: true); + Directory.Delete(linkTarget, recursive: true); + } + } + + /// + /// Creates an NTFS directory junction at pointing to + /// , using mklink /J since junctions (unlike symbolic + /// links) do not require elevated privileges or Developer Mode on Windows. + /// + /// The junction's path; its parent must exist and it must not already + /// exist. + /// The existing directory the junction points to. + private static void CreateJunction(string linkPath, string targetPath) + { + var startInfo = new ProcessStartInfo("cmd.exe", $"/c mklink /J \"{linkPath}\" \"{targetPath}\"") + { + RedirectStandardOutput = true, + RedirectStandardError = true, + UseShellExecute = false, + CreateNoWindow = true + }; + + using var process = Process.Start(startInfo) + ?? throw new InvalidOperationException("Failed to start 'cmd.exe' to create junction."); + process.WaitForExit(); + if (process.ExitCode != 0) + { + throw new InvalidOperationException( + $"Failed to create junction '{linkPath}' -> '{targetPath}': {process.StandardError.ReadToEnd()}"); + } + } + /// /// Creates a temporary package zip with the four managed folders populated from the /// given (relative-path-under-.github, content) pairs, plus a root-level file that must From d2348df2dfcabd3d14698dc9d491cff96f62438a Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Sun, 13 Sep 2026 23:17:10 -0400 Subject: [PATCH 04/10] Address fourth round of PR review feedback - PackageZipExtractor: apply the EnsureNoSymlinkAncestors reparse-point guard to the blind-delete step (DeleteManagedFolder) as well as extraction, since Directory.Delete(recursive: true) follows filesystem links and could otherwise remove content outside repoRoot through a junctioned/symlinked managed-folder ancestor before extraction's own guard is ever reached. Generalized EnsureNoSymlinkAncestors' signature (repoRoot, path, context) so both call sites share one implementation. - Add a regression test proving the delete-step guard actually fires: pre-populates content behind a junctioned ".github" target and asserts it survives a refused Extract call (the existing junction test alone only exercised the extraction-step guard, since its link target started empty). - Add "junctioned", "keepme", "mklink", "NTFS" to the cspell dictionary for the new test/doc content. - Update the PackageZipExtractor design doc to describe the symlink-ancestor guard and its new InvalidOperationException case. Caught via an independent local code-review + formal-review pass before pushing (the formal review additionally caught a stray duplicated code fragment left over from an earlier edit that broke compilation of the test file - fixed prior to this commit). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .cspell.yaml | 4 ++ .../repo-sync/package-zip-extractor.md | 12 +++-- .../RepoSync/PackageZipExtractor.cs | 31 ++++++++---- .../RepoSync/PackageZipExtractorTests.cs | 49 ++++++++++++++++++- 4 files changed, 81 insertions(+), 15 deletions(-) diff --git a/.cspell.yaml b/.cspell.yaml index bf49414..a1cebce 100644 --- a/.cspell.yaml +++ b/.cspell.yaml @@ -30,6 +30,10 @@ words: - lsfiles - Mvvm - MVVM + - junctioned + - keepme + - mklink + - NTFS - Postconditions - buildmark - buildtransitive diff --git a/docs/design/agent-control/repo-sync/package-zip-extractor.md b/docs/design/agent-control/repo-sync/package-zip-extractor.md index 5626139..9d53bf1 100644 --- a/docs/design/agent-control/repo-sync/package-zip-extractor.md +++ b/docs/design/agent-control/repo-sync/package-zip-extractor.md @@ -56,10 +56,14 @@ entry without extracting it to disk. `Extract` throws `ArgumentNullException` for a null `zipPath`/`repoRoot`, and `InvalidOperationException` when the zip cannot be opened/is not a valid archive, a managed -folder cannot be deleted, or extraction fails partway through — wrapping the underlying -`IOException`/`UnauthorizedAccessException`/`InvalidDataException` with a message naming the -zip path and repo root. `ReadReleaseNotes` throws the same `InvalidOperationException` pattern -for a zip that cannot be opened or whose release-notes entry cannot be read. +folder cannot be deleted, extraction fails partway through, a zip entry would resolve outside +`repoRoot`, or a managed-folder ancestor (e.g. `.github`) is itself a symlink/junction — wrapping +the underlying `IOException`/`UnauthorizedAccessException`/`InvalidDataException` with a +message naming the zip path and repo root. The symlink-ancestor check (`EnsureNoSymlinkAncestors`) +runs before both the blind-delete step and each entry's extraction, so a reparse-point ancestor +is rejected before either destructive operation can follow it outside `repoRoot`. `ReadReleaseNotes` +throws the same `InvalidOperationException` pattern for a zip that cannot be opened or whose +release-notes entry cannot be read. #### Dependencies diff --git a/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs b/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs index 6b622d1..0eeab20 100644 --- a/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs +++ b/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs @@ -194,10 +194,19 @@ private static ZipArchive OpenArchive(string zipPath) /// Absolute path to the repository root. /// The managed folder's path relative to the repo root. /// Thrown when the folder exists but cannot be - /// deleted. + /// deleted, or when an ancestor between and the folder is a + /// reparse point (symlink/junction). private static void DeleteManagedFolder(string repoRoot, string relativeFolder) { var folderPath = PathHelpers.SafePathCombine(repoRoot, relativeFolder); + + // Reject a reparse-point ancestor (e.g. a symlinked/junctioned '.github') before the + // blind delete below: Directory.Delete(recursive: true) follows filesystem links, so + // without this guard a crafted/pre-existing junction could cause content outside + // repoRoot to be deleted before extraction's own EnsureNoSymlinkAncestors check is ever + // reached. + EnsureNoSymlinkAncestors(Path.GetFullPath(repoRoot), folderPath, relativeFolder); + if (!Directory.Exists(folderPath)) { return; @@ -275,27 +284,29 @@ private static void ExtractEntryIfManaged(ZipArchiveEntry entry, string repoRoot } /// - /// Walks upward from to , - /// rejecting extraction if any existing ancestor directory in between is itself a - /// reparse point (symlink/junction). + /// Walks upward from to , rejecting the + /// operation if any existing ancestor directory in between (or + /// itself) is itself a reparse point (symlink/junction). /// /// The already-resolved () repo /// root; the walk stops here. - /// The zip entry's destination directory. - /// The zip entry's name, for the exception message. + /// The path whose ancestry is being validated - either a zip entry's + /// destination directory (before extraction) or a managed folder about to be blind-deleted. + /// A short description of the path/entry, for the exception message. /// /// is purely lexical - it never resolves /// filesystem links - so the earlier relative-path containment check alone cannot detect /// a symlinked ancestor redirecting a nominally-contained path outside the repo root. This /// only guards against ancestors that already exist at the time of the check; it does not /// eliminate a race where an ancestor is replaced with a symlink between this check and - /// /. + /// // + /// . /// /// Thrown when an ancestor directory is a /// reparse point. - private static void EnsureNoSymlinkAncestors(string repoRoot, string destinationDirectory, string entryName) + private static void EnsureNoSymlinkAncestors(string repoRoot, string path, string context) { - var current = Path.TrimEndingDirectorySeparator(destinationDirectory); + var current = Path.TrimEndingDirectorySeparator(path); var normalizedRoot = Path.TrimEndingDirectorySeparator(repoRoot); while (!string.IsNullOrEmpty(current) @@ -304,7 +315,7 @@ private static void EnsureNoSymlinkAncestors(string repoRoot, string destination if (Directory.Exists(current) && File.GetAttributes(current).HasFlag(FileAttributes.ReparsePoint)) { throw new InvalidOperationException( - $"Zip entry '{entryName}' extracts through a symlinked directory '{current}'."); + $"'{context}' resolves through a symlinked directory '{current}'."); } current = Path.GetDirectoryName(current); diff --git a/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs b/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs index c0749d9..9053eee 100644 --- a/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs +++ b/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs @@ -278,7 +278,9 @@ public void PackageZipExtractor_Extract_ManagedFolderAncestorIsJunction_ThrowsAn } // Arrange: a repo root whose ".github" entry is a junction to a separate, isolated - // directory standing in for a location outside the repo + // directory standing in for a location outside the repo. The link target starts empty so + // this test exercises the extraction-step guard specifically (DeleteManagedFolder is a + // no-op here since no managed folder exists through the junction yet). var zipPath = CreatePackageZip(("agents/copilot.md", "should not be extracted")); var repoRoot = CreateTempDirectory(); var linkTarget = CreateTempDirectory(); @@ -293,7 +295,52 @@ public void PackageZipExtractor_Extract_ManagedFolderAncestorIsJunction_ThrowsAn finally { File.Delete(zipPath); + // The ".github" junction entry itself must be removed (not recursively, since that + // would delete the link target's contents) before the repo root can be deleted. + Directory.Delete(Path.Combine(repoRoot, ".github")); + Directory.Delete(repoRoot, recursive: true); + Directory.Delete(linkTarget, recursive: true); + } + } + /// + /// Test that the blind-delete step itself refuses to recurse through a junctioned + /// .github ancestor, so pre-existing content at the link's target survives even + /// though it happens to be reachable at a path that lexically looks like a managed folder. + /// + [Fact] + public void PackageZipExtractor_Extract_ManagedFolderAncestorIsJunctionWithExistingContent_DoesNotBlindDeleteThroughLink() + { + // NTFS directory junctions are a Windows-only concept; skip on other CI runners. + if (!OperatingSystem.IsWindows()) + { + Assert.Skip("Directory junctions are a Windows-only filesystem feature."); + } + + // Arrange: a repo root whose ".github" entry is a junction to a separate, isolated + // directory that *already* has a real "agents" folder with content - so + // Directory.Exists(folderPath) is true through the junction, and without the + // DeleteManagedFolder symlink-ancestor guard, Extract's blind-delete step 2 would recurse + // through the junction and remove it before extraction's own guard is ever reached. + var zipPath = CreatePackageZip(("agents/copilot.md", "should not be extracted")); + var repoRoot = CreateTempDirectory(); + var linkTarget = CreateTempDirectory(); + var linkTargetAgentsDir = Path.Combine(linkTarget, "agents"); + Directory.CreateDirectory(linkTargetAgentsDir); + var keepFilePath = Path.Combine(linkTargetAgentsDir, "keepme.md"); + File.WriteAllText(keepFilePath, "must survive"); + try + { + CreateJunction(Path.Combine(repoRoot, ".github"), linkTarget); + + // Act / Assert: extraction is refused, and the pre-existing content behind the + // junction was never blind-deleted + Assert.Throws(() => PackageZipExtractor.Extract(zipPath, repoRoot)); + Assert.Equal("must survive", File.ReadAllText(keepFilePath)); + } + finally + { + File.Delete(zipPath); // The ".github" junction entry itself must be removed (not recursively, since that // would delete the link target's contents) before the repo root can be deleted. Directory.Delete(Path.Combine(repoRoot, ".github")); From 0b355794001348480c6f7feaf6f45ba022de4086 Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Sun, 13 Sep 2026 23:46:29 -0400 Subject: [PATCH 05/10] Harden symlink/reparse-point defenses in PackageZipExtractor - EnsureNoSymlinkAncestors now checks the repo root itself, not just ancestors between it and the target path. - Detect dangling symlinks/junctions via File.GetAttributes instead of Directory.Exists, which silently skips the check for broken links. - Add DeleteDirectoryRejectingReparsePoints: a recursive delete that fails closed if the managed folder or any nested descendant is a reparse point, replacing Directory.Delete's link-following recursive mode. - Widen DeleteManagedFolder's try/catch to also cover the ancestor symlink check, so ACL failures surface consistently as InvalidOperationException. - ExtractEntryIfManaged now also catches NotSupportedException from SafePathCombine and preserves the real failure reason in its message instead of always claiming a traversal outside the repo root. - Add XML doc tag and regression tests for repo-root-is- junction and nested-junction-inside-managed-folder scenarios. - Update design doc for the above. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../repo-sync/package-zip-extractor.md | 19 ++- .../RepoSync/PackageZipExtractor.cs | 134 ++++++++++++++---- .../RepoSync/PackageZipExtractorTests.cs | 83 +++++++++++ 3 files changed, 199 insertions(+), 37 deletions(-) diff --git a/docs/design/agent-control/repo-sync/package-zip-extractor.md b/docs/design/agent-control/repo-sync/package-zip-extractor.md index 9d53bf1..ce7605d 100644 --- a/docs/design/agent-control/repo-sync/package-zip-extractor.md +++ b/docs/design/agent-control/repo-sync/package-zip-extractor.md @@ -57,13 +57,18 @@ entry without extracting it to disk. `Extract` throws `ArgumentNullException` for a null `zipPath`/`repoRoot`, and `InvalidOperationException` when the zip cannot be opened/is not a valid archive, a managed folder cannot be deleted, extraction fails partway through, a zip entry would resolve outside -`repoRoot`, or a managed-folder ancestor (e.g. `.github`) is itself a symlink/junction — wrapping -the underlying `IOException`/`UnauthorizedAccessException`/`InvalidDataException` with a -message naming the zip path and repo root. The symlink-ancestor check (`EnsureNoSymlinkAncestors`) -runs before both the blind-delete step and each entry's extraction, so a reparse-point ancestor -is rejected before either destructive operation can follow it outside `repoRoot`. `ReadReleaseNotes` -throws the same `InvalidOperationException` pattern for a zip that cannot be opened or whose -release-notes entry cannot be read. +`repoRoot`, or the repo root itself, an ancestor, a managed folder, or any of its descendants is +a symlink/junction — wrapping the underlying `IOException`/`UnauthorizedAccessException`/ +`InvalidDataException` with a message naming the zip path and repo root. The symlink check +(`EnsureNoSymlinkAncestors`) walks from the affected path up to *and including* the repo root +itself (not just its ancestors) before both the blind-delete step and each entry's extraction, +using `File.GetAttributes` rather than `Directory.Exists` so a *dangling* symlink/junction (whose +target does not currently exist) is still detected. The blind-delete step additionally uses +`DeleteDirectoryRejectingReparsePoints`, a recursive delete that fails closed the moment it finds +a reparse point nested *inside* a managed folder, rather than a plain recursive +`Directory.Delete` that would otherwise follow such a link. `ReadReleaseNotes` throws the same +`InvalidOperationException` pattern for a zip that cannot be opened or whose release-notes entry +cannot be read. #### Dependencies diff --git a/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs b/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs index 0eeab20..7f571bc 100644 --- a/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs +++ b/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs @@ -194,32 +194,71 @@ private static ZipArchive OpenArchive(string zipPath) /// Absolute path to the repository root. /// The managed folder's path relative to the repo root. /// Thrown when the folder exists but cannot be - /// deleted, or when an ancestor between and the folder is a - /// reparse point (symlink/junction). + /// deleted, or when the repo root, an ancestor, the folder itself, or any descendant of + /// the folder is a reparse point (symlink/junction). private static void DeleteManagedFolder(string repoRoot, string relativeFolder) { var folderPath = PathHelpers.SafePathCombine(repoRoot, relativeFolder); - // Reject a reparse-point ancestor (e.g. a symlinked/junctioned '.github') before the - // blind delete below: Directory.Delete(recursive: true) follows filesystem links, so - // without this guard a crafted/pre-existing junction could cause content outside - // repoRoot to be deleted before extraction's own EnsureNoSymlinkAncestors check is ever - // reached. - EnsureNoSymlinkAncestors(Path.GetFullPath(repoRoot), folderPath, relativeFolder); + try + { + // Reject a reparse-point repo root/ancestor (e.g. a symlinked/junctioned '.github') + // before the blind delete below: Directory.Delete(recursive: true) follows filesystem + // links, so without this guard a crafted/pre-existing junction could cause content + // outside repoRoot to be deleted before extraction's own EnsureNoSymlinkAncestors check + // is ever reached. Wrapped alongside the delete itself so an UnauthorizedAccessException + // from an ACL-restricted ancestor surfaces as the same documented InvalidOperationException, + // consistent with ExtractEntryIfManaged's equivalent call (protected by Extract's step-3 + // try/catch). + EnsureNoSymlinkAncestors(Path.GetFullPath(repoRoot), folderPath, relativeFolder); + + if (!Directory.Exists(folderPath)) + { + return; + } - if (!Directory.Exists(folderPath)) + // A plain Directory.Delete(folderPath, recursive: true) would also follow any + // reparse point nested *inside* the managed folder (not just its ancestors), + // potentially deleting content outside repoRoot. DeleteDirectoryRejectingReparsePoints + // walks the tree itself and fails closed the moment it finds one. + DeleteDirectoryRejectingReparsePoints(folderPath, relativeFolder); + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { - return; + throw new InvalidOperationException($"Failed to delete folder '{folderPath}': {ex.Message}", ex); } + } - try + /// + /// Recursively deletes , rejecting the delete if it or any + /// descendant directory is itself a reparse point (symlink/junction) - unlike + /// 's recursive mode, which follows such links + /// and can delete content outside the directory being cleaned up. + /// + /// The directory to delete. + /// A short description of the folder being deleted, for the exception + /// message. + /// Thrown when a nested reparse point is + /// encountered. + private static void DeleteDirectoryRejectingReparsePoints(string directoryPath, string context) + { + if (File.GetAttributes(directoryPath).HasFlag(FileAttributes.ReparsePoint)) { - Directory.Delete(folderPath, recursive: true); + throw new InvalidOperationException( + $"'{context}' contains a symlinked directory '{directoryPath}'; refusing to delete through it."); } - catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + + foreach (var filePath in Directory.GetFiles(directoryPath)) { - throw new InvalidOperationException($"Failed to delete folder '{folderPath}': {ex.Message}", ex); + File.Delete(filePath); + } + + foreach (var subdirectoryPath in Directory.GetDirectories(directoryPath)) + { + DeleteDirectoryRejectingReparsePoints(subdirectoryPath, context); } + + Directory.Delete(directoryPath, recursive: false); } /// @@ -229,6 +268,9 @@ private static void DeleteManagedFolder(string repoRoot, string relativeFolder) /// /// The zip entry to consider. /// Absolute path to the repository root. + /// Thrown when the entry's path is invalid + /// (including resolving outside ), or when the repo root, an + /// ancestor, or the destination directory itself is a reparse point (symlink/junction). private static void ExtractEntryIfManaged(ZipArchiveEntry entry, string repoRoot) { // Directory entries have an empty Name (only FullName ends with '/'); skip them, as @@ -242,17 +284,19 @@ private static void ExtractEntryIfManaged(ZipArchiveEntry entry, string repoRoot var relativePath = entry.FullName.Replace('/', Path.DirectorySeparatorChar); // PathHelpers.SafePathCombine is the single source of truth for containment validation - // (see its own doc remarks); it throws ArgumentException if the entry's path would - // resolve outside repoRoot, so it is not re-derived here by hand. + // (see its own doc remarks); it throws ArgumentException/NotSupportedException both when + // the entry's path would resolve outside repoRoot and for other malformed-path failures, + // so the original message is preserved here rather than assuming every failure is a + // traversal attempt. string destinationPath; try { destinationPath = PathHelpers.SafePathCombine(repoRoot, relativePath); } - catch (ArgumentException ex) + catch (Exception ex) when (ex is ArgumentException or NotSupportedException) { throw new InvalidOperationException( - $"Zip entry '{entry.FullName}' resolves outside the repository root.", ex); + $"Zip entry '{entry.FullName}' has an invalid destination path: {ex.Message}", ex); } // The managed-folder membership check must run against the *canonical* (".."-resolved) @@ -284,40 +328,70 @@ private static void ExtractEntryIfManaged(ZipArchiveEntry entry, string repoRoot } /// - /// Walks upward from to , rejecting the - /// operation if any existing ancestor directory in between (or - /// itself) is itself a reparse point (symlink/junction). + /// Walks upward from to (and including) + /// itself, rejecting the operation if any existing path in the walk is itself a reparse + /// point (symlink/junction). /// /// The already-resolved () repo - /// root; the walk stops here. + /// root; also checked, since a symlinked/junctioned repo root would otherwise let every + /// managed-folder operation write through it undetected. /// The path whose ancestry is being validated - either a zip entry's /// destination directory (before extraction) or a managed folder about to be blind-deleted. /// A short description of the path/entry, for the exception message. /// /// is purely lexical - it never resolves /// filesystem links - so the earlier relative-path containment check alone cannot detect - /// a symlinked ancestor redirecting a nominally-contained path outside the repo root. This - /// only guards against ancestors that already exist at the time of the check; it does not - /// eliminate a race where an ancestor is replaced with a symlink between this check and + /// a symlinked ancestor redirecting a nominally-contained path outside the repo root. + /// Reparse-point status is queried via guarded by + /// a not-found catch, rather than : the latter + /// resolves the link's target to decide existence and so returns + /// (silently skipping the check) for a *dangling* symlink/junction, whereas + /// reports the reparse point's own attributes + /// without requiring its target to exist. This only guards against paths that already + /// exist at the time of the check; it does not eliminate a race where a path is replaced + /// with a symlink between this check and /// // /// . /// - /// Thrown when an ancestor directory is a - /// reparse point. + /// Thrown when a path in the walk is a reparse + /// point. private static void EnsureNoSymlinkAncestors(string repoRoot, string path, string context) { var current = Path.TrimEndingDirectorySeparator(path); var normalizedRoot = Path.TrimEndingDirectorySeparator(repoRoot); - while (!string.IsNullOrEmpty(current) - && !string.Equals(current, normalizedRoot, StringComparison.OrdinalIgnoreCase)) + while (!string.IsNullOrEmpty(current)) { - if (Directory.Exists(current) && File.GetAttributes(current).HasFlag(FileAttributes.ReparsePoint)) + FileAttributes attributes; + try + { + attributes = File.GetAttributes(current); + } + catch (Exception ex) when (ex is FileNotFoundException or DirectoryNotFoundException) + { + // This path segment does not exist yet (e.g. a destination directory that will + // be created by this same operation) - nothing to reject here, keep walking + // upward. + if (string.Equals(current, normalizedRoot, StringComparison.OrdinalIgnoreCase)) + { + break; + } + + current = Path.GetDirectoryName(current); + continue; + } + + if (attributes.HasFlag(FileAttributes.ReparsePoint)) { throw new InvalidOperationException( $"'{context}' resolves through a symlinked directory '{current}'."); } + if (string.Equals(current, normalizedRoot, StringComparison.OrdinalIgnoreCase)) + { + break; + } + current = Path.GetDirectoryName(current); } } diff --git a/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs b/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs index 9053eee..9903cfb 100644 --- a/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs +++ b/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs @@ -349,6 +349,89 @@ public void PackageZipExtractor_Extract_ManagedFolderAncestorIsJunctionWithExist } } + /// + /// Test that a repo root which is itself a junction to another location is refused, since + /// every managed-folder operation would otherwise silently write through it. + /// + [Fact] + public void PackageZipExtractor_Extract_RepoRootIsJunction_ThrowsAndDoesNotWriteThroughLink() + { + // NTFS directory junctions are a Windows-only concept; skip on other CI runners. + if (!OperatingSystem.IsWindows()) + { + Assert.Skip("Directory junctions are a Windows-only filesystem feature."); + } + + // Arrange: a "repo root" that is itself nothing but a junction to a separate, isolated + // directory - simulating a caller-supplied path that resolves through a link before any + // managed-folder segment is even appended. + var zipPath = CreatePackageZip(("agents/copilot.md", "should not be extracted")); + var linkTarget = CreateTempDirectory(); + var repoRootParent = CreateTempDirectory(); + var repoRoot = Path.Combine(repoRootParent, "repo-root-link"); + try + { + CreateJunction(repoRoot, linkTarget); + + // Act / Assert: extraction is refused, and nothing was written through the link + Assert.Throws(() => PackageZipExtractor.Extract(zipPath, repoRoot)); + Assert.False(File.Exists(Path.Combine(linkTarget, ".github", "agents", "copilot.md"))); + } + finally + { + File.Delete(zipPath); + // The repo-root junction entry itself must be removed (not recursively, since that + // would delete the link target's contents) before the parent can be deleted. + Directory.Delete(repoRoot); + Directory.Delete(repoRootParent, recursive: true); + Directory.Delete(linkTarget, recursive: true); + } + } + + /// + /// Test that a junction/symlink nested *inside* a managed folder (not just an ancestor of + /// it) is rejected during the blind-delete step, since a plain recursive + /// would otherwise follow it and delete + /// content outside the repo root. + /// + [Fact] + public void PackageZipExtractor_Extract_ManagedFolderContainsNestedJunction_ThrowsAndDoesNotDeleteThroughLink() + { + // NTFS directory junctions are a Windows-only concept; skip on other CI runners. + if (!OperatingSystem.IsWindows()) + { + Assert.Skip("Directory junctions are a Windows-only filesystem feature."); + } + + // Arrange: a normal (non-linked) ".github/agents" managed folder that itself contains a + // nested junction pointing to a separate, isolated directory with content that must + // survive. + var zipPath = CreatePackageZip(("agents/copilot.md", "should not be extracted")); + var repoRoot = CreateTempDirectory(); + var linkTarget = CreateTempDirectory(); + var keepFilePath = Path.Combine(linkTarget, "keepme.md"); + File.WriteAllText(keepFilePath, "must survive"); + var agentsDir = Path.Combine(repoRoot, ".github", "agents"); + Directory.CreateDirectory(agentsDir); + try + { + CreateJunction(Path.Combine(agentsDir, "linked"), linkTarget); + + // Act / Assert: the blind delete is refused, and the linked content was never deleted + Assert.Throws(() => PackageZipExtractor.Extract(zipPath, repoRoot)); + Assert.Equal("must survive", File.ReadAllText(keepFilePath)); + } + finally + { + File.Delete(zipPath); + // The nested junction entry itself must be removed (not recursively) before the repo + // root can be deleted. + Directory.Delete(Path.Combine(agentsDir, "linked")); + Directory.Delete(repoRoot, recursive: true); + Directory.Delete(linkTarget, recursive: true); + } + } + /// /// Creates an NTFS directory junction at pointing to /// , using mklink /J since junctions (unlike symbolic From 463b8e9a4ba788f4fdc9df1414f77993df34e4b2 Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Mon, 14 Sep 2026 00:06:57 -0400 Subject: [PATCH 06/10] Close AllManagedFoldersExist reparse-point bypass AllManagedFoldersExist previously used a plain Directory.Exists check, so a symlinked/junctioned repo root or ancestor (e.g. '.github') could make it report every managed folder as present, letting RepoCardViewModel.EnsureAgentFilesSyncedBeforeLaunch skip Extract entirely and launch using files reached through the link - bypassing all of Extract's reparse-point protections. - AllManagedFoldersExist now runs the same EnsureNoSymlinkAncestors check Extract relies on for each managed folder, treating a folder reached through a reparse-point root/ancestor/self as not present. - Also catches IOException/UnauthorizedAccessException from that check (e.g. an ACL-restricted ancestor) so the method keeps its documented ArgumentNullException-only contract instead of leaking an undocumented exception past RepoCardViewModel's catch clauses. - Add regression tests for an ancestor junction with real folders behind it, and for the managed folder itself being a junction. - Add a cross-platform dangling-symlink/junction regression test for Extract (real symlinks on Linux/macOS, a deleted-target junction on Windows), closing a formal-review-flagged test gap. - Update requirements, design, and verification documentation for the above. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../repo-sync/package-zip-extractor.md | 6 +- .../repo-sync/package-zip-extractor.yaml | 10 +- .../repo-sync/package-zip-extractor.md | 25 +++- .../RepoSync/PackageZipExtractor.cs | 56 +++++++- .../RepoSync/PackageZipExtractorTests.cs | 133 ++++++++++++++++++ 5 files changed, 221 insertions(+), 9 deletions(-) diff --git a/docs/design/agent-control/repo-sync/package-zip-extractor.md b/docs/design/agent-control/repo-sync/package-zip-extractor.md index ce7605d..4d8cf2c 100644 --- a/docs/design/agent-control/repo-sync/package-zip-extractor.md +++ b/docs/design/agent-control/repo-sync/package-zip-extractor.md @@ -41,7 +41,11 @@ a repo root. - *Returns*: `bool`. - *Postconditions*: Does not inspect folder contents — a managed folder that exists but is empty (or only partially populated) still counts as "existing" - (`AgentControl-PackageZipExtractor-AllManagedFoldersExist`). Consulted by + (`AgentControl-PackageZipExtractor-AllManagedFoldersExist`). A managed folder reached through + a reparse-point (symlink/junction) repo root, ancestor, or the folder itself is treated as + **not** existing, using the same `EnsureNoSymlinkAncestors` check `Extract` relies on — this + prevents a caller from trusting content reached through a link and skipping `Extract`'s own + reparse-point protections entirely. Consulted by `RepoCardViewModel.EnsureAgentFilesSyncedBeforeLaunch` to decide whether a silent re-extraction is needed before launch. diff --git a/docs/reqstream/agent-control/repo-sync/package-zip-extractor.yaml b/docs/reqstream/agent-control/repo-sync/package-zip-extractor.yaml index f1b9efb..c2e6c03 100644 --- a/docs/reqstream/agent-control/repo-sync/package-zip-extractor.yaml +++ b/docs/reqstream/agent-control/repo-sync/package-zip-extractor.yaml @@ -25,14 +25,20 @@ sections: - id: AgentControl-PackageZipExtractor-AllManagedFoldersExist title: >- The PackageZipExtractor class shall report whether all of a repo's managed agent-file - folders are present, returning false if any are missing. + folders are present, returning false if any are missing, and shall also treat a + managed folder as absent when it is only reachable through a reparse point + (symlink/junction) repo root, ancestor, or the folder itself. justification: | Used by the ensure-synced-before-launch check to decide whether a re-extraction is - needed without re-scanning file contents. + needed without re-scanning file contents. Trusting a reparse-point-reached folder as + "present" would let this check skip Extract entirely and let a launch proceed using + files outside the repo root, bypassing Extract's own symlink/junction protections. tests: - PackageZipExtractor_AllManagedFoldersExist_AllFourPresent_ReturnsTrue - PackageZipExtractor_AllManagedFoldersExist_SomeMissing_ReturnsFalse - PackageZipExtractor_AllManagedFoldersExist_NoneExist_ReturnsFalse + - PackageZipExtractor_AllManagedFoldersExist_AncestorIsJunctionWithRealFolders_ReturnsFalse + - PackageZipExtractor_AllManagedFoldersExist_ManagedFolderItselfIsJunction_ReturnsFalse - id: AgentControl-PackageZipExtractor-ReadReleaseNotes title: >- diff --git a/docs/verification/agent-control/repo-sync/package-zip-extractor.md b/docs/verification/agent-control/repo-sync/package-zip-extractor.md index 3445411..6e48335 100644 --- a/docs/verification/agent-control/repo-sync/package-zip-extractor.md +++ b/docs/verification/agent-control/repo-sync/package-zip-extractor.md @@ -30,11 +30,14 @@ throws `InvalidOperationException`. This scenario is tested by `AgentControl-PackageZipExtractor-Extract`. **PackageZipExtractor_AllManagedFoldersExist_ReflectsActualPresence**: The check returns true -when all four managed folders are present, and false when some or none are present. This -scenario is tested by +when all four managed folders are present, and false when some or none are present, or when a +managed folder is only reachable through a reparse-point (symlink/junction) repo root, +ancestor, or the folder itself. This scenario is tested by `PackageZipExtractor_AllManagedFoldersExist_AllFourPresent_ReturnsTrue`, -`PackageZipExtractor_AllManagedFoldersExist_SomeMissing_ReturnsFalse`, and -`PackageZipExtractor_AllManagedFoldersExist_NoneExist_ReturnsFalse`, covering +`PackageZipExtractor_AllManagedFoldersExist_SomeMissing_ReturnsFalse`, +`PackageZipExtractor_AllManagedFoldersExist_NoneExist_ReturnsFalse`, +`PackageZipExtractor_AllManagedFoldersExist_AncestorIsJunctionWithRealFolders_ReturnsFalse`, and +`PackageZipExtractor_AllManagedFoldersExist_ManagedFolderItselfIsJunction_ReturnsFalse`, covering `AgentControl-PackageZipExtractor-AllManagedFoldersExist`. **PackageZipExtractor_ReadReleaseNotes_ReturnsContentOrNullWithoutExtracting**: Reading @@ -43,3 +46,17 @@ the archive, and reading from a zip with no such entry returns null. This scenar by `PackageZipExtractor_ReadReleaseNotes_EntryPresent_ReturnsContentWithoutExtracting` and `PackageZipExtractor_ReadReleaseNotes_NoEntry_ReturnsNull`, covering `AgentControl-PackageZipExtractor-ReadReleaseNotes`. + +**PackageZipExtractor_Extract_RejectsReparsePointsAtEveryVulnerablePoint**: Extraction refuses +to operate through a reparse point (symlink/junction) wherever one could otherwise let content +be read from or written/deleted outside the repo root: a managed-folder ancestor (e.g. a +linked `.github`), whether empty or already containing real content that must survive; the +repo root itself; a reparse point nested *inside* a managed folder (not just above it); and a +*dangling* link (whose target no longer exists), which a naive `Directory.Exists`-based check +would silently miss. This scenario is tested by +`PackageZipExtractor_Extract_ManagedFolderAncestorIsJunction_ThrowsAndDoesNotWriteThroughLink`, +`PackageZipExtractor_Extract_ManagedFolderAncestorIsJunctionWithExistingContent_DoesNotBlindDeleteThroughLink`, +`PackageZipExtractor_Extract_RepoRootIsJunction_ThrowsAndDoesNotWriteThroughLink`, +`PackageZipExtractor_Extract_ManagedFolderContainsNestedJunction_ThrowsAndDoesNotDeleteThroughLink`, +and `PackageZipExtractor_Extract_ManagedFolderAncestorIsDanglingLink_ThrowsAndDoesNotBypassGuard`, +covering `AgentControl-PackageZipExtractor-Extract`. diff --git a/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs b/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs index 7f571bc..ba0e9a8 100644 --- a/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs +++ b/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs @@ -120,7 +120,12 @@ public static void Extract(string zipPath, string repoRoot) /// so the managed-folder list has a single source of truth - callers (e.g. /// RepoCardViewModel's ensure-synced-before-launch check) never need to duplicate /// it. Does not inspect folder contents; a managed folder that exists but is empty (or - /// only partially populated) still counts as "existing" here. + /// only partially populated) still counts as "existing" here. A managed folder that is + /// only reachable through a reparse-point (symlink/junction) repo root or ancestor - e.g. + /// a symlinked .github - is deliberately treated as not existing: trusting + /// it here would let a caller (such as RepoCardViewModel.EnsureAgentFilesSyncedBeforeLaunch) + /// skip entirely and launch using files outside + /// without any of 's reparse-point protections ever running. /// /// Thrown when is /// . @@ -128,7 +133,54 @@ public static bool AllManagedFoldersExist(string repoRoot) { ArgumentNullException.ThrowIfNull(repoRoot); - return ManagedFolders.All(folder => Directory.Exists(PathHelpers.SafePathCombine(repoRoot, folder))); + var normalizedRoot = Path.GetFullPath(repoRoot); + return ManagedFolders.All(folder => ManagedFolderGenuinelyExists(normalizedRoot, folder)); + } + + /// + /// Determines whether a single managed folder exists under a repo root without being + /// reached through a reparse point (symlink/junction) repo root, ancestor, or the folder + /// itself. + /// + /// The already-resolved () + /// repo root. + /// The managed folder's path relative to the repo root. + /// if the folder exists and no reparse point sits between it + /// and (inclusive); otherwise . + /// + /// Deliberately swallows (as "not genuinely present") both the documented reparse-point + /// rejection and any I/O failure while walking the ancestor chain - e.g. an + /// from an ACL-restricted ancestor, which + /// does not itself catch. This keeps + /// 's contract to only ever throw + /// (its callers, e.g. + /// RepoCardViewModel.EnsureAgentFilesSyncedBeforeLaunch, only guard against + /// 's own documented exceptions and do not expect this read-only + /// check to throw anything else). + /// + private static bool ManagedFolderGenuinelyExists(string normalizedRoot, string relativeFolder) + { + var folderPath = PathHelpers.SafePathCombine(normalizedRoot, relativeFolder); + if (!Directory.Exists(folderPath)) + { + return false; + } + + try + { + EnsureNoSymlinkAncestors(normalizedRoot, folderPath, relativeFolder); + } + catch (Exception ex) when (ex is InvalidOperationException or IOException or UnauthorizedAccessException) + { + // A reparse-point root/ancestor/self, or an inability to even inspect one (e.g. an + // ACL-restricted ancestor), means this folder cannot be confirmed as genuinely + // present under normalizedRoot; treat it the same as missing so callers fall back to + // Extract, which will itself surface the underlying failure as a hard error instead + // of silently trusting - or crashing on - content reached through a symlink/junction. + return false; + } + + return true; } /// diff --git a/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs b/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs index 9903cfb..d4af514 100644 --- a/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs +++ b/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs @@ -225,6 +225,43 @@ public void PackageZipExtractor_AllManagedFoldersExist_NoneExist_ReturnsFalse() } } + /// + /// Test that AllManagedFoldersExist returns false when a managed folder itself (not an + /// ancestor such as .github) is a junction, distinguishing this case from + /// . + /// + [Fact] + public void PackageZipExtractor_AllManagedFoldersExist_ManagedFolderItselfIsJunction_ReturnsFalse() + { + // NTFS directory junctions are a Windows-only concept; skip on other CI runners. + if (!OperatingSystem.IsWindows()) + { + Assert.Skip("Directory junctions are a Windows-only filesystem feature."); + } + + // Arrange: a repo root with three genuine managed folders, and a fourth + // (".github/agents") that is itself a junction to a separate, isolated directory with + // real content - so the naive Directory.Exists-based check alone would (incorrectly) + // report it as present. + var repoRoot = CreateTempDirectory(); + var linkTarget = CreateTempDirectory(); + Directory.CreateDirectory(Path.Combine(repoRoot, ".github", "standards")); + Directory.CreateDirectory(Path.Combine(repoRoot, ".github", "templates")); + Directory.CreateDirectory(Path.Combine(repoRoot, ".github", "skills")); + try + { + CreateJunction(Path.Combine(repoRoot, ".github", "agents"), linkTarget); + + Assert.False(PackageZipExtractor.AllManagedFoldersExist(repoRoot)); + } + finally + { + Directory.Delete(Path.Combine(repoRoot, ".github", "agents")); + Directory.Delete(repoRoot, recursive: true); + Directory.Delete(linkTarget, recursive: true); + } + } + /// /// Test that a zip entry whose name textually starts with a managed-folder prefix but /// uses ".." components to resolve to a location outside every managed folder (while @@ -432,6 +469,102 @@ public void PackageZipExtractor_Extract_ManagedFolderContainsNestedJunction_Thro } } + /// + /// Test that a *dangling* symlink/junction ancestor (one whose target no longer exists) is + /// still rejected, exercising the -based reparse + /// check regardless of whether the link's target exists - unlike the former + /// -based check, which follows the link to test for + /// the target's existence and would silently report "not a directory" (skipping the + /// guard entirely) for a dangling link. Runs on every platform: real symbolic links on + /// Linux/macOS via , and NTFS + /// junctions on Windows (created against a real target, then made dangling by deleting + /// that target, since mklink /J itself requires an existing target). + /// + [Fact] + public void PackageZipExtractor_Extract_ManagedFolderAncestorIsDanglingLink_ThrowsAndDoesNotBypassGuard() + { + var zipPath = CreatePackageZip(("agents/copilot.md", "should not be extracted")); + var repoRoot = CreateTempDirectory(); + var githubPath = Path.Combine(repoRoot, ".github"); + try + { + CreateDanglingLink(githubPath); + + // Act / Assert: extraction is refused, not silently allowed through the dangling link + Assert.Throws(() => PackageZipExtractor.Extract(zipPath, repoRoot)); + } + finally + { + File.Delete(zipPath); + // The dangling link entry itself must be removed (non-recursively - there is no + // real target content behind it) before the repo root can be deleted. + Directory.Delete(githubPath); + Directory.Delete(repoRoot, recursive: true); + } + } + + /// + /// Test that refuses to trust a + /// managed folder that is only reachable through a reparse-point ancestor, rather than + /// silently reporting it as present (which would let a caller such as + /// RepoCardViewModel.EnsureAgentFilesSyncedBeforeLaunch skip Extract + /// entirely and launch using files outside the repo root). + /// + [Fact] + public void PackageZipExtractor_AllManagedFoldersExist_AncestorIsJunctionWithRealFolders_ReturnsFalse() + { + // NTFS directory junctions are a Windows-only concept; skip on other CI runners. + if (!OperatingSystem.IsWindows()) + { + Assert.Skip("Directory junctions are a Windows-only filesystem feature."); + } + + // Arrange: a repo root whose ".github" entry is a junction to a separate, isolated + // directory that genuinely has all four managed folders - so the naive + // Directory.Exists-based check alone would (incorrectly) report every folder as present. + var repoRoot = CreateTempDirectory(); + var linkTarget = CreateTempDirectory(); + foreach (var folder in new[] { "agents", "standards", "templates", "skills" }) + { + Directory.CreateDirectory(Path.Combine(linkTarget, folder)); + } + + try + { + CreateJunction(Path.Combine(repoRoot, ".github"), linkTarget); + + Assert.False(PackageZipExtractor.AllManagedFoldersExist(repoRoot)); + } + finally + { + Directory.Delete(Path.Combine(repoRoot, ".github")); + Directory.Delete(repoRoot, recursive: true); + Directory.Delete(linkTarget, recursive: true); + } + } + + /// + /// Creates a dangling directory symlink/junction at - one + /// whose target does not exist - using a real symbolic link on Linux/macOS (created + /// without any target validation) or an NTFS junction on Windows (created against a real + /// temporary target that is deleted immediately afterward). + /// + /// The link's path; its parent must exist and it must not already + /// exist. + private static void CreateDanglingLink(string linkPath) + { + if (OperatingSystem.IsWindows()) + { + var target = CreateTempDirectory(); + CreateJunction(linkPath, target); + Directory.Delete(target); + } + else + { + Directory.CreateSymbolicLink(linkPath, Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N"))); + } + } + /// /// Creates an NTFS directory junction at pointing to /// , using mklink /J since junctions (unlike symbolic From af2c6825d7022536458e354a5f6fd9c9fa36a114 Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Mon, 14 Sep 2026 01:00:40 -0400 Subject: [PATCH 07/10] Centralize reparse-point detection into PathHelpers Every prior review round found one more symlink/junction edge case in PackageZipExtractor because the detection logic was duplicated and ad-hoc rather than being a single, well-tested primitive. This refactor centralizes it properly: - Add PathHelpers.FindReparsePointInAncestry and FindReparsePointInDescendants: pure, filesystem-aware query primitives that report the first reparse point found (or null), separating detection from policy. - PackageZipExtractor's EnsureNoSymlinkAncestors and DeleteDirectoryRejectingReparsePoints become thin wrappers that apply policy (throw UnsafeRepositoryStateException) on top of the new primitives; ManagedFolderGenuinelyExists uses a direct null check instead of exception-driven control flow. The old inline RejectNestedReparsePoints walker is removed. - Fix a catch-ordering bug in RepoCardViewModel: since UnsafeRepositoryStateException derives from InvalidOperationException, it must be caught before the general InvalidOperationException clause or Launch()'s dedicated handling is unreachable. - Fix remaining CA1873 findings (unguarded expensive LogError arguments) in AgentToolLauncher and GitClient. - Add 10 new PathHelpers unit tests, 1 new RepoCardViewModel regression test, and update 5 existing PackageZipExtractor tests to assert the more specific UnsafeRepositoryStateException. - Update design, reqstream, and verification docs accordingly, including adding windows@ test-evidence prefixes for junction-only tests that are skipped on non-Windows runners, and closing a traceability gap where 5 existing symlink/junction tests were never linked to any requirement. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .reviewmark.yaml | 1 + .../launcher-ui/repo-card-view-model.md | 39 ++- docs/design/agent-control/repo-sync.md | 17 +- .../repo-sync/package-zip-extractor.md | 57 ++-- docs/design/agent-control/utilities.md | 44 ++- .../agent-control/utilities/path-helpers.md | 72 ++++- .../launcher-ui/repo-card-view-model.yaml | 17 + .../repo-sync/package-zip-extractor.yaml | 29 +- .../agent-control/utilities/path-helpers.yaml | 46 ++- .../launcher-ui/repo-card-view-model.md | 12 +- .../repo-sync/package-zip-extractor.md | 6 +- .../agent-control/utilities/path-helpers.md | 49 ++- .../AgentToolLauncher/AgentToolLauncher.cs | 11 +- .../GitIntegration/GitClient.cs | 11 +- .../LauncherUI/RepoCardViewModel.cs | 55 +++- .../RepoSync/PackageZipExtractor.cs | 143 ++++---- .../UnsafeRepositoryStateException.cs | 48 +++ .../Utilities/PathHelpers.cs | 138 ++++++++ .../LauncherUI/RepoCardViewModelTests.cs | 85 +++++ .../RepoSync/PackageZipExtractorTests.cs | 29 +- .../Utilities/PathHelpersTests.cs | 304 ++++++++++++++++++ 21 files changed, 1060 insertions(+), 153 deletions(-) create mode 100644 src/DemaConsulting.AgentControl/RepoSync/UnsafeRepositoryStateException.cs diff --git a/.reviewmark.yaml b/.reviewmark.yaml index f5bc406..ad8b976 100644 --- a/.reviewmark.yaml +++ b/.reviewmark.yaml @@ -265,6 +265,7 @@ reviews: - "docs/design/agent-control/repo-sync/package-zip-extractor.md" - "docs/verification/agent-control/repo-sync/package-zip-extractor.md" - "src/**/RepoSync/PackageZipExtractor.cs" + - "src/**/RepoSync/UnsafeRepositoryStateException.cs" - "test/**/RepoSync/PackageZipExtractorTests.cs" - id: AgentControl-RepoSync-ReleaseNotesViewerViewModel diff --git a/docs/design/agent-control/launcher-ui/repo-card-view-model.md b/docs/design/agent-control/launcher-ui/repo-card-view-model.md index 40ced94..acf5738 100644 --- a/docs/design/agent-control/launcher-ui/repo-card-view-model.md +++ b/docs/design/agent-control/launcher-ui/repo-card-view-model.md @@ -68,15 +68,26 @@ refresh. Calls `EnsureAgentFilesSyncedBeforeLaunch` purely for its best-effort sync side effect, then always proceeds to resolve the shell/agent command and spawn the process regardless of that call's outcome — the agent-package sync state (missing pin, missing managed folders, failed -re-extraction) never gates the launch. +re-extraction) never gates the launch. The one deliberate exception: if +`EnsureAgentFilesSyncedBeforeLaunch` propagates `UnsafeRepositoryStateException` (a managed +folder is reachable only through a reparse point — symlink/junction), `Launch` catches it +specifically, raises `ErrorOccurred`, and returns without spawning the process — a narrow, +documented amendment to the "sync state never blocks launch" rule, justified because this +represents a security concern (launching against content reached through an unexpected link) +rather than a mere missing-content situation +(`AgentControl-RepoCardViewModel-Launch-UnsafeState`). **EnsureAgentFilesSyncedBeforeLaunch** (internal): Best-effort attempt to sync the repo's four managed agent folders with the pinned package version before `Launch` spawns the process. This -is an informational side action only — it never blocks `Launch`. +is an informational side action only for every failure mode except one — it never blocks +`Launch` for an ordinary sync failure, but deliberately lets `UnsafeRepositoryStateException` +propagate uncaught (see `Launch`, above). - *Parameters*: None. - *Returns*: `bool` — informational only; `true` if no sync action was needed/attempted or a - sync succeeded, `false` if a sync attempt was made and failed. Either way `Launch` proceeds. + sync succeeded, `false` if a sync attempt was made and failed for an ordinary reason. Either + way `Launch` proceeds, *except* when `UnsafeRepositoryStateException` propagates instead of a + `bool` being returned at all. - *Postconditions*: If `HasCommittedAgentFiles` is `true`, the managed folders are never touched (no delete, no extract) even if a pin exists and folders are missing; a non-blocking `StatusMessage` notes that sync was skipped. Otherwise, if `PinnedPackageName` is `null`, this @@ -85,8 +96,11 @@ is an informational side action only — it never blocks `Launch`. is already `true`, returns `true` immediately with no extra I/O. Otherwise resolves the *currently pinned* package at the configured source (never the latest) and re-extracts it directly via `PackageZipExtractor.Extract`, without displaying release notes; if this - re-extraction attempt fails for any reason, `ErrorOccurred` is raised as a non-blocking - warning (`AgentControl-RepoCardViewModel-EnsureSyncedBeforeLaunch`). + re-extraction attempt fails for an ordinary reason, `ErrorOccurred` is raised as a non-blocking + warning (`AgentControl-RepoCardViewModel-EnsureSyncedBeforeLaunch`); if it instead fails + because a managed folder is reachable only through a reparse point, the resulting + `UnsafeRepositoryStateException` is deliberately left uncaught rather than absorbed into this + best-effort return value. - Marked `internal` rather than `private` so it can be unit-tested directly, separated from `Launch`'s process-spawning side effect, mirroring `AgentToolLauncher.BuildProcessStartInfo`'s own precedent. @@ -128,10 +142,17 @@ non-blocking idiom already established by `EnsureAgentFilesSyncedBeforeLaunch`. #### Error Handling `Launch` catches `InvalidOperationException`/`ArgumentException` from -`AgentToolLauncher.BuildProcessStartInfo`/`Launch` and raises `ErrorOccurred`. +`AgentToolLauncher.BuildProcessStartInfo`/`Launch`, and `UnsafeRepositoryStateException` +(specifically, before the general `InvalidOperationException` case would otherwise apply) from +`EnsureAgentFilesSyncedBeforeLaunch`, raising `ErrorOccurred` in every case; only the +`UnsafeRepositoryStateException` case aborts before a process is spawned. `EnsureAgentFilesSyncedBeforeLaunch` catches `InvalidOperationException` (extraction failure) and `DirectoryNotFoundException` (unreachable source), raising `ErrorOccurred` as a -non-blocking warning for each rather than propagating or blocking the launch. `Pull` catches +non-blocking warning for each rather than propagating or blocking the launch — but re-throws +`UnsafeRepositoryStateException` unchanged (a `catch (UnsafeRepositoryStateException) { throw; }` +clause precedes the general `InvalidOperationException` catch, since it derives from that type +and C# evaluates catch clauses in source order) so `Launch`'s dedicated handling above actually +gets exercised. `Pull` catches `InvalidOperationException` from `GitClient.Pull` and reports it via `ErrorOccurred`. `RefreshGitStatus`/`RefreshBranchAndCommittedFiles` catch `InvalidOperationException` from `GitClient` and degrade to "unknown"/`false` rather than propagating, since these run as part @@ -150,7 +171,9 @@ throws `ArgumentNullException` for a null - **RepoPinStore** (`RepoConfig` subsystem) — reads/writes the repo's pin. - **PackageSource**, **PackageVersionCache** (`AgentPackageManagement` subsystem) — package discovery and upgrade-availability checks. -- **PackageZipExtractor** (`RepoSync` subsystem) — extracts a package into the repo. +- **PackageZipExtractor** (`RepoSync` subsystem) — extracts a package into the repo, and may + throw `UnsafeRepositoryStateException` if a managed folder is reachable only through a + reparse point, which `Launch` catches specifically to abort without spawning a process. - **GitIgnoreEnsurer** (`RepoSync` subsystem) — proactively ensures the repo's `.gitignore` covers the four managed agent folders after a successful extraction. - **GitClient**, **CommittedAgentFilesCache** (`GitIntegration` subsystem) — git status, diff --git a/docs/design/agent-control/repo-sync.md b/docs/design/agent-control/repo-sync.md index f202300..d8be375 100644 --- a/docs/design/agent-control/repo-sync.md +++ b/docs/design/agent-control/repo-sync.md @@ -25,7 +25,11 @@ zip's contents. and extracts the new files excluding root-level files such as `release-notes.md` (`AgentControl-PackageZipExtractor-Extract`). - *Constraints*: Throws `InvalidOperationException` for a file that is not a valid zip - archive; per architecture.md's "blind delete of the four known folders" decision, any local + archive; throws `UnsafeRepositoryStateException` (a dedicated `InvalidOperationException` + subtype) if the repo root, a managed folder, an ancestor of one, or anything nested inside + one is reachable only through a reparse point (symlink/junction), leaving the affected + content untouched (`AgentControl-PackageZipExtractor-RejectsReparsePoints`); per + architecture.md's "blind delete of the four known folders" decision, any local customizations a developer added inside those folders are silently removed — an accepted risk, not a defect (see architecture.md's Open Concerns #3). @@ -34,8 +38,9 @@ present on disk. - *Type*: In-process .NET static method. - *Role*: Provider. -- *Contract*: Returns `false` if any of the four folders is missing - (`AgentControl-PackageZipExtractor-AllManagedFoldersExist`); used by +- *Contract*: Returns `false` if any of the four folders is missing, and also treats a folder + as absent when it is only reachable through a reparse point (repo root, an ancestor, or the + folder itself) (`AgentControl-PackageZipExtractor-AllManagedFoldersExist`); used by `RepoCardViewModel`'s ensure-synced-before-launch check to decide whether a re-extraction is needed without re-scanning file contents. - *Constraints*: Performs only existence checks, not content verification. @@ -77,7 +82,11 @@ the new files, excluding root-level files like `release-notes.md`. It is a stati no persistent state; the pin-file rewrite and release-notes display happen in the calling `RepoCardViewModel`, deliberately *after* a successful extraction, so a failure partway through extraction never leaves a repo pinned to a version whose files were not actually -applied (`AgentControl-RepoSync-Sync`). +applied (`AgentControl-RepoSync-Sync`). `UnsafeRepositoryStateException` is a small, +purpose-built exception type owned by this subsystem with no dedicated unit-level design/ +reqstream/verification docs of its own; it is documented here, folded into +`PackageZipExtractor`'s description, because its only role is signaling the reparse-point +rejection behavior described above. `ReleaseNotesViewerViewModel` is a simple, stateless-beyond-construction display view model: it is constructed once per shown dialog with the repo name, package title, and release notes diff --git a/docs/design/agent-control/repo-sync/package-zip-extractor.md b/docs/design/agent-control/repo-sync/package-zip-extractor.md index 4d8cf2c..9d4da29 100644 --- a/docs/design/agent-control/repo-sync/package-zip-extractor.md +++ b/docs/design/agent-control/repo-sync/package-zip-extractor.md @@ -43,11 +43,11 @@ a repo root. empty (or only partially populated) still counts as "existing" (`AgentControl-PackageZipExtractor-AllManagedFoldersExist`). A managed folder reached through a reparse-point (symlink/junction) repo root, ancestor, or the folder itself is treated as - **not** existing, using the same `EnsureNoSymlinkAncestors` check `Extract` relies on — this - prevents a caller from trusting content reached through a link and skipping `Extract`'s own - reparse-point protections entirely. Consulted by - `RepoCardViewModel.EnsureAgentFilesSyncedBeforeLaunch` to decide whether a silent - re-extraction is needed before launch. + **not** existing, using `PathHelpers.FindReparsePointInAncestry` (the same filesystem-aware + primitive `Extract`'s own symlink guard is built on) — this prevents a caller from trusting + content reached through a link and skipping `Extract`'s own reparse-point protections + entirely. Consulted by `RepoCardViewModel.EnsureAgentFilesSyncedBeforeLaunch` to decide + whether a silent re-extraction is needed before launch. **ReadReleaseNotes**: Reads the content of the package zip's root-level `release-notes.md` entry without extracting it to disk. @@ -60,24 +60,38 @@ entry without extracting it to disk. `Extract` throws `ArgumentNullException` for a null `zipPath`/`repoRoot`, and `InvalidOperationException` when the zip cannot be opened/is not a valid archive, a managed -folder cannot be deleted, extraction fails partway through, a zip entry would resolve outside -`repoRoot`, or the repo root itself, an ancestor, a managed folder, or any of its descendants is -a symlink/junction — wrapping the underlying `IOException`/`UnauthorizedAccessException`/ -`InvalidDataException` with a message naming the zip path and repo root. The symlink check -(`EnsureNoSymlinkAncestors`) walks from the affected path up to *and including* the repo root -itself (not just its ancestors) before both the blind-delete step and each entry's extraction, -using `File.GetAttributes` rather than `Directory.Exists` so a *dangling* symlink/junction (whose -target does not currently exist) is still detected. The blind-delete step additionally uses -`DeleteDirectoryRejectingReparsePoints`, a recursive delete that fails closed the moment it finds -a reparse point nested *inside* a managed folder, rather than a plain recursive -`Directory.Delete` that would otherwise follow such a link. `ReadReleaseNotes` throws the same -`InvalidOperationException` pattern for a zip that cannot be opened or whose release-notes entry -cannot be read. +folder cannot be deleted, extraction fails partway through, or a zip entry would resolve outside +`repoRoot` — wrapping the underlying `IOException`/`UnauthorizedAccessException`/ +`InvalidDataException` with a message naming the zip path and repo root. When the repo root, an +ancestor, a managed folder, or any of its descendants is a symlink/junction, `Extract` instead +throws `UnsafeRepositoryStateException` (a dedicated `InvalidOperationException` subtype) — +this distinguishes a genuine security concern (content reached through an unexpected link) from +an ordinary extraction failure, letting callers that need to react differently (e.g. +`RepoCardViewModel.Launch`, which otherwise never lets sync state block a launch) catch it +specifically, while callers that only care about "did this fail" can still catch the base +`InvalidOperationException` type unchanged. The symlink checks +(`PathHelpers.FindReparsePointInAncestry` via the `EnsureNoSymlinkAncestors` wrapper) walk from +the affected path up to *and including* the repo root itself (not just its ancestors) before +both the blind-delete step and each entry's extraction, using `File.GetAttributes` rather than +`Directory.Exists` so a *dangling* symlink/junction (whose target does not currently exist) is +still detected. The blind-delete step additionally uses `DeleteDirectoryRejectingReparsePoints`, +which preflights the entire managed-folder tree with `PathHelpers.FindReparsePointInDescendants` +*before* deleting anything, then performs a plain recursive delete — this two-phase split avoids +a partial delete: interleaving the reparse-point check with the delete itself would still let an +ordinary sibling file be permanently removed before a reparse point discovered later in the same +tree aborts the operation. `ReadReleaseNotes` throws the same `InvalidOperationException` pattern +for a zip that cannot be opened or whose release-notes entry cannot be read. #### Dependencies - **PathHelpers** (`Utilities` subsystem) — `SafePathCombine` resolves every managed-folder - and zip-entry destination path safely relative to the repo root. + and zip-entry destination path safely relative to the repo root; `FindReparsePointInAncestry` + and `FindReparsePointInDescendants` detect reparse points that a lexical path check alone + cannot see, underpinning `EnsureNoSymlinkAncestors` and `DeleteDirectoryRejectingReparsePoints` + respectively. +- **UnsafeRepositoryStateException** (`RepoSync` subsystem) — the dedicated exception type + thrown when a reparse point is detected, letting callers distinguish this security concern + from ordinary `InvalidOperationException` failures. - **.NET BCL** — `System.IO.Compression.ZipFile`/`ZipArchive`. #### Callers @@ -85,4 +99,7 @@ cannot be read. - **RepoCardViewModel** — calls `Extract` (via `ApplyPackageAndShowReleaseNotes`) after a Select-Package or Upgrade action, calls `AllManagedFoldersExist` and, on a miss, `Extract` directly during `EnsureAgentFilesSyncedBeforeLaunch`, and calls `ReadReleaseNotes` to obtain - the text shown via `ReleaseNotesViewerViewModel`. + the text shown via `ReleaseNotesViewerViewModel`. `Launch` catches + `UnsafeRepositoryStateException` specifically (propagated up through + `EnsureAgentFilesSyncedBeforeLaunch`) to abort the launch entirely, rather than treating it as + a best-effort sync failure. diff --git a/docs/design/agent-control/utilities.md b/docs/design/agent-control/utilities.md index d794062..417c9d6 100644 --- a/docs/design/agent-control/utilities.md +++ b/docs/design/agent-control/utilities.md @@ -7,8 +7,9 @@ The `Utilities` subsystem provides shared utility functions for the Agent Control. It supplies reusable, independently testable helpers consumed by other subsystems. Its primary responsibility is safe file-path manipulation, protecting callers from path-traversal -vulnerabilities when constructing paths from caller-supplied inputs. The `Utilities` subsystem -contains one unit: `PathHelpers`. +vulnerabilities when constructing paths from caller-supplied inputs, and detecting filesystem +reparse points (symlinks/junctions) that could otherwise redirect a file operation outside an +intended directory tree. The `Utilities` subsystem contains one unit: `PathHelpers`. ### Interfaces @@ -24,14 +25,47 @@ that escapes the base directory. when the combined path escapes the base directory; may propagate `NotSupportedException` or `PathTooLongException` from underlying BCL path operations. +**PathHelpers.FindReparsePointInAncestry**: Searches upward from a path to a root (inclusive of +both ends) for the first directory entry that is a reparse point (symlink or junction). + +- *Type*: In-process .NET static method. +- *Role*: Provider. +- *Contract*: Accepts `string root` and `string path`, where `path` is expected to be at or + below `root`. Returns the first reparse-point path found while walking upward from `path` to + `root`, or `null` if none is found. Uses `File.GetAttributes` rather than `Directory.Exists` + so a dangling link (whose target no longer exists) is still detected. +- *Constraints*: Throws `ArgumentNullException` for null inputs; may propagate `IOException` or + `UnauthorizedAccessException` from underlying filesystem access. + +**PathHelpers.FindReparsePointInDescendants**: Recursively searches a directory tree +(inclusive of the directory itself) for the first reparse point. + +- *Type*: In-process .NET static method. +- *Role*: Provider. +- *Contract*: Accepts `string directory`. Returns the first reparse-point path found in the + directory itself or anywhere beneath it, or `null` if none is found. +- *Constraints*: Throws `ArgumentNullException` for null input; may propagate `IOException` or + `UnauthorizedAccessException` from underlying filesystem access. + ### Design The `Utilities` subsystem contains only the `PathHelpers` unit. It has no dependencies on other -tool units or subsystems; it uses only .NET BCL types (`Path`, `ArgumentNullException`). +tool units or subsystems; it uses only .NET BCL types (`Path`, `File`, `Directory`, +`ArgumentNullException`). -`PathHelpers.SafePathCombine` is a pure utility method: it performs no file-system I/O, holds -no state, and throws immediately on invalid input. All calls to `SafePathCombine` in the +`PathHelpers.SafePathCombine` is a pure, lexical utility method: it performs no file-system I/O, +holds no state, and throws immediately on invalid input. All calls to `SafePathCombine` in the codebase originate from the `RepoConfig` subsystem (`RepoPinStore`, resolving the per-repo `.agentcontrol.json` pin file path) and the `RepoSync` subsystem (`PackageZipExtractor`, resolving managed-folder and zip-entry destination paths), each using it to keep a caller-supplied repo root from being escaped by a malformed relative path. + +`FindReparsePointInAncestry` and `FindReparsePointInDescendants` are the filesystem-aware +counterpart to `SafePathCombine`: because a reparse point cannot be detected lexically, these +methods perform real filesystem queries (`File.GetAttributes`) to find one. Both are pure query +functions - they report what they find via a nullable return value and never throw for the +"reparse point found" case, deliberately separating *detection* from the *policy* of what to do +about a finding. The sole caller of both methods is the `RepoSync` subsystem +(`PackageZipExtractor`), which applies that policy by throwing +`UnsafeRepositoryStateException` when either method returns non-null, refusing to extract into +or delete through a path reachable only via a symlink/junction. diff --git a/docs/design/agent-control/utilities/path-helpers.md b/docs/design/agent-control/utilities/path-helpers.md index 26731c6..ddf0e70 100644 --- a/docs/design/agent-control/utilities/path-helpers.md +++ b/docs/design/agent-control/utilities/path-helpers.md @@ -4,9 +4,14 @@ #### Purpose -`PathHelpers` is a static utility class that provides a safe path-combination method. Its -single responsibility is to combine two path segments while verifying that the result does not -escape the base directory, protecting callers from string-level path-traversal attacks. +`PathHelpers` is a static utility class providing path-safety primitives with two +complementary halves: `SafePathCombine` performs purely lexical (string-level) containment +validation, while `FindReparsePointInAncestry` and `FindReparsePointInDescendants` perform +filesystem-aware detection of reparse points (symlinks/junctions) that a lexical check alone +cannot see. Callers that assemble a path and then act on the filesystem at it should normally +use both: `SafePathCombine` to reject a textually-escaping path, then one of the reparse-point +finders to reject a link that would otherwise redirect an apparently-safe path outside its +intended root. #### Data Model @@ -37,6 +42,48 @@ paths, platform case-sensitivity, and directory-separator normalization natively check treats a double-dot segment as escaping only when it is the entire relative result or is followed by a directory separator, avoiding false positives for valid names such as `"..data"`. +This method performs no file-system I/O; it is purely lexical and therefore cannot detect a +symlinked/junctioned directory silently redirecting a nominally-contained path outside its +intended root — see `FindReparsePointInAncestry` for that case. + +**FindReparsePointInAncestry**: Searches upward from a path to (and including) a root boundary +for a directory that is itself a reparse point (symlink/junction). + +- *Parameters*: `string root` — the already fully-resolved (`Path.GetFullPath`) boundary at + which the upward walk stops (inclusive); `string path` — the already fully-resolved path + whose ancestry is inspected. +- *Returns*: `string?` — the closest-to-`path` segment that is a reparse point, or `null` if no + segment between `path` and `root` (inclusive of both ends) is one. +- *Preconditions*: Both `root` and `path` are non-null. + +Reparse-point status is queried via `File.GetAttributes` rather than `Directory.Exists`: the +latter resolves (follows) a link to test whether its target exists, and so returns `false` +(silently missing the reparse point) for a *dangling* symlink/junction, whereas +`File.GetAttributes` reports the reparse point's own attributes without requiring its target to +exist. A path segment that does not exist yet (e.g. a destination directory a caller is about +to create) is not itself a finding; the walk simply continues upward past it. This method only +ever *reports* what it found — callers decide what policy to apply (e.g. throwing a +domain-specific exception, or treating a read-only existence check as "not present"). This only +reflects the state of the filesystem at the moment of the call; it does not eliminate a race +where a segment is replaced with a reparse point between this check and a caller's subsequent +file operation. + +**FindReparsePointInDescendants**: Recursively searches a directory (inclusive) for the first +nested directory that is a reparse point (symlink/junction). + +- *Parameters*: `string directory` — the already-existing directory (and its descendants) to + search. +- *Returns*: `string?` — the path of the first reparse point found (`directory` itself, or a + descendant), or `null` if none exists anywhere in the tree. +- *Preconditions*: `directory` is non-null. + +Exists for callers that need to recursively delete, copy, or otherwise walk a directory tree +without following filesystem links nested inside it — unlike `Directory.Delete(path, true)`'s +recursive mode, which follows such links and can affect content outside the tree being +processed. The whole tree is searched up front, rather than interleaving this check with +file-by-file processing, so a caller can preflight an entire operation and fail closed before +acting on any part of the tree if a reparse point exists anywhere within it. + #### Error Handling `SafePathCombine` throws `ArgumentNullException` for null inputs. It throws `ArgumentException` @@ -45,10 +92,18 @@ followed by a directory separator, avoiding false positives for valid names such operations (`Path.Combine`, `Path.GetFullPath`). No logging or error accumulation is performed; callers receive exceptions directly. +`FindReparsePointInAncestry` and `FindReparsePointInDescendants` throw `ArgumentNullException` +for null inputs, and may propagate `IOException`/`UnauthorizedAccessException` from +`File.GetAttributes`/`Directory.GetDirectories` when a path segment cannot be inspected for a +reason other than not existing (e.g. an ACL-restricted directory). Neither method throws a +domain-specific exception itself when a reparse point is found — that is a query result +(`string?`), not a failure; policy for what to do about a found reparse point belongs to the +caller. + #### Dependencies -- **.NET BCL** — `Path`, `ArgumentNullException`, and related types are the only dependencies. - No other tool units or subsystems are used. +- **.NET BCL** — `Path`, `File`, `Directory`, `ArgumentNullException`, and related types are the + only dependencies. No other tool units or subsystems are used. #### Callers @@ -57,4 +112,9 @@ callers receive exceptions directly. repo directory. - **PackageZipExtractor** — calls `SafePathCombine` to construct managed-folder paths for `AllManagedFoldersExist` and zip-entry destination paths during `Extract`, so a malicious or - malformed zip-entry name cannot write outside the target repo. + malformed zip-entry name cannot write outside the target repo. Calls + `FindReparsePointInAncestry` (via its own `EnsureNoSymlinkAncestors` wrapper, and directly from + `ManagedFolderGenuinelyExists`) to reject a repo root/ancestor/managed-folder that is a + reparse point, and `FindReparsePointInDescendants` (via `DeleteDirectoryRejectingReparsePoints`) + to reject a managed folder whose contents include a nested reparse point before blind-deleting + it. diff --git a/docs/reqstream/agent-control/launcher-ui/repo-card-view-model.yaml b/docs/reqstream/agent-control/launcher-ui/repo-card-view-model.yaml index 3aef231..8835a4d 100644 --- a/docs/reqstream/agent-control/launcher-ui/repo-card-view-model.yaml +++ b/docs/reqstream/agent-control/launcher-ui/repo-card-view-model.yaml @@ -92,6 +92,23 @@ sections: - RepoCardViewModel_EnsureAgentFilesSyncedBeforeLaunch_PinnedVersionMissingFromSource_ReturnsFalse - RepoCardViewModel_LaunchCommand_SyncFailsOrNoPin_StillLaunches + - id: AgentControl-RepoCardViewModel-Launch-UnsafeState + title: >- + The RepoCardViewModel class shall refuse to launch the configured agent tool, + raising an error instead, when the ensure-synced-before-launch sync attempt detects + that a managed agent-file folder is reachable only through a reparse point + (symlink/junction). + justification: | + Unlike an ordinary sync failure (missing pin, unreachable source, an I/O failure), + a managed folder reached only through an unexpected symlink or junction is a genuine + security concern: launching could run the agentic CLI tool against content outside + the repo root. This is a deliberate, narrow exception to the "sync state never + blocks launch" rule established by AgentControl-RepoCardViewModel-EnsureSyncedBeforeLaunch, + justified because it protects the developer from an attacker-controlled or + accidentally-misconfigured filesystem link rather than merely missing content. + tests: + - RepoCardViewModel_LaunchCommand_ManagedFolderAncestorIsJunction_DoesNotLaunch + - id: AgentControl-RepoCardViewModel-Upgrade title: >- The RepoCardViewModel class shall, when a newer package version is available, diff --git a/docs/reqstream/agent-control/repo-sync/package-zip-extractor.yaml b/docs/reqstream/agent-control/repo-sync/package-zip-extractor.yaml index c2e6c03..7f4f776 100644 --- a/docs/reqstream/agent-control/repo-sync/package-zip-extractor.yaml +++ b/docs/reqstream/agent-control/repo-sync/package-zip-extractor.yaml @@ -22,6 +22,31 @@ sections: - PackageZipExtractor_Extract_ExistingManagedFolder_ReplacesOldContents - PackageZipExtractor_Extract_InvalidZipFile_ThrowsInvalidOperationException + - id: AgentControl-PackageZipExtractor-RejectsReparsePoints + title: >- + The PackageZipExtractor class shall refuse to extract into, or delete through, a + managed folder that is reachable only through a reparse point (symlink/junction) - + whether the repo root itself, an ancestor of a managed folder, the managed folder + itself, or a directory nested inside it - throwing UnsafeRepositoryStateException + and leaving the affected content untouched. + justification: | + Extract's blind-delete-and-replace sequence uses recursive filesystem operations + that would otherwise follow a symlink/junction, letting a crafted or accidentally + misconfigured link redirect a delete or write outside the intended repo root. This + is treated as a distinct security concern (UnsafeRepositoryStateException, a + dedicated InvalidOperationException subtype) rather than an ordinary extraction + failure, so callers that must react differently to it can do so. Detection uses + File.GetAttributes rather than Directory.Exists so a dangling link (whose target no + longer exists) is still caught, and the managed-folder tree is fully preflighted for + nested reparse points before any deletion begins, so a partial delete cannot occur + before the guard fires. + tests: + - windows@PackageZipExtractor_Extract_ManagedFolderAncestorIsJunction_ThrowsAndDoesNotWriteThroughLink + - windows@PackageZipExtractor_Extract_ManagedFolderAncestorIsJunctionWithExistingContent_DoesNotBlindDeleteThroughLink + - windows@PackageZipExtractor_Extract_RepoRootIsJunction_ThrowsAndDoesNotWriteThroughLink + - windows@PackageZipExtractor_Extract_ManagedFolderContainsNestedJunction_ThrowsAndDoesNotDeleteThroughLink + - PackageZipExtractor_Extract_ManagedFolderAncestorIsDanglingLink_ThrowsAndDoesNotBypassGuard + - id: AgentControl-PackageZipExtractor-AllManagedFoldersExist title: >- The PackageZipExtractor class shall report whether all of a repo's managed agent-file @@ -37,8 +62,8 @@ sections: - PackageZipExtractor_AllManagedFoldersExist_AllFourPresent_ReturnsTrue - PackageZipExtractor_AllManagedFoldersExist_SomeMissing_ReturnsFalse - PackageZipExtractor_AllManagedFoldersExist_NoneExist_ReturnsFalse - - PackageZipExtractor_AllManagedFoldersExist_AncestorIsJunctionWithRealFolders_ReturnsFalse - - PackageZipExtractor_AllManagedFoldersExist_ManagedFolderItselfIsJunction_ReturnsFalse + - windows@PackageZipExtractor_AllManagedFoldersExist_AncestorIsJunctionWithRealFolders_ReturnsFalse + - windows@PackageZipExtractor_AllManagedFoldersExist_ManagedFolderItselfIsJunction_ReturnsFalse - id: AgentControl-PackageZipExtractor-ReadReleaseNotes title: >- diff --git a/docs/reqstream/agent-control/utilities/path-helpers.yaml b/docs/reqstream/agent-control/utilities/path-helpers.yaml index 805014c..0f3a60f 100644 --- a/docs/reqstream/agent-control/utilities/path-helpers.yaml +++ b/docs/reqstream/agent-control/utilities/path-helpers.yaml @@ -1,9 +1,11 @@ --- # Software Unit Requirements for the PathHelpers Class # -# The PathHelpers class provides safe path-combination utilities that protect -# against path-traversal attacks by rejecting relative paths containing ".." -# or absolute paths when a relative path is expected. +# The PathHelpers class provides path-safety utilities with two complementary halves: purely +# lexical safe path-combination (protecting against path-traversal attacks by rejecting +# relative paths containing ".." or absolute paths when a relative path is expected), and +# filesystem-aware reparse-point (symlink/junction) detection, which a lexical check alone +# cannot see. sections: - title: PathHelpers Unit Requirements @@ -25,3 +27,41 @@ sections: - PathHelpers_SafePathCombine_DotDotPrefixedName_CombinesCorrectly - PathHelpers_SafePathCombine_NullBasePath_ThrowsArgumentNullException - PathHelpers_SafePathCombine_NullRelativePath_ThrowsArgumentNullException + + - id: AgentControl-PathHelpers-FindReparsePointInAncestry + title: >- + The PathHelpers class shall report the closest reparse point (symlink/junction) + between a given path and a root boundary (inclusive of both), or report none if no + such reparse point exists, without following any link target to make that + determination. + justification: | + A purely lexical path-containment check (as performed by SafePathCombine) cannot + detect a symlinked or junctioned ancestor directory silently redirecting a + nominally-contained path outside its intended root. Detecting reparse points via + File.GetAttributes (rather than Directory.Exists, which follows a link to test its + target) also correctly detects a dangling link whose target no longer exists. + tests: + - PathHelpers_FindReparsePointInAncestry_NoReparsePoints_ReturnsNull + - PathHelpers_FindReparsePointInAncestry_AncestorIsJunction_ReturnsJunctionPath + - PathHelpers_FindReparsePointInAncestry_RootIsJunction_ReturnsRoot + - PathHelpers_FindReparsePointInAncestry_AncestorIsDanglingLink_ReturnsLinkPath + - PathHelpers_FindReparsePointInAncestry_NullRoot_ThrowsArgumentNullException + - PathHelpers_FindReparsePointInAncestry_NullPath_ThrowsArgumentNullException + + - id: AgentControl-PathHelpers-FindReparsePointInDescendants + title: >- + The PathHelpers class shall report the first reparse point (symlink/junction) found + anywhere within a directory tree (including the directory itself), or report none if + no such reparse point exists anywhere in the tree. + justification: | + A caller recursively deleting, copying, or otherwise walking a directory tree must + be able to detect a reparse point nested anywhere inside it before acting, since + Directory.Delete's recursive mode (and similar recursive BCL operations) follow such + links and can affect content outside the tree being processed. Searching the whole + tree up front (rather than interleaving detection with per-file processing) lets a + caller fail closed before touching any part of the tree. + tests: + - PathHelpers_FindReparsePointInDescendants_NoReparsePoints_ReturnsNull + - PathHelpers_FindReparsePointInDescendants_NestedJunction_ReturnsJunctionPath + - PathHelpers_FindReparsePointInDescendants_DirectoryItselfIsJunction_ReturnsDirectory + - PathHelpers_FindReparsePointInDescendants_NullDirectory_ThrowsArgumentNullException diff --git a/docs/verification/agent-control/launcher-ui/repo-card-view-model.md b/docs/verification/agent-control/launcher-ui/repo-card-view-model.md index 2a98671..65a5afa 100644 --- a/docs/verification/agent-control/launcher-ui/repo-card-view-model.md +++ b/docs/verification/agent-control/launcher-ui/repo-card-view-model.md @@ -19,7 +19,9 @@ invocations are not parallelized with other tests using real process launches). - All unit tests pass with zero failures. - Display fields, badges, and gating conditions reflect the correct underlying pin/git/source state for every tested input. -- Launch is never blocked by the outcome of the best-effort ensure-synced check. +- Launch is never blocked by the outcome of the best-effort ensure-synced check, except when + that check detects a managed folder reachable only through a reparse point (symlink/ + junction), which is a deliberate, narrow exception. - Upgrade and select-package flows never mutate the pin when their preconditions are not met. - Remove requests never mutate state on their own. @@ -81,6 +83,14 @@ outright. This scenario is tested by and `RepoCardViewModel_LaunchCommand_SyncFailsOrNoPin_StillLaunches`, covering `AgentControl-RepoCardViewModel-EnsureSyncedBeforeLaunch`. +**RepoCardViewModel_Launch_RefusesWhenManagedFolderIsUnsafe**: `LaunchCommand` does not spawn +the agent-tool process, and raises `ErrorOccurred` instead, when the ensure-synced-before-launch +sync attempt detects that a managed agent-file folder is reachable only through a reparse point +(symlink/junction) - the one deliberate exception to the "sync state never blocks launch" +policy verified by the previous scenario. This scenario is tested by +`RepoCardViewModel_LaunchCommand_ManagedFolderAncestorIsJunction_DoesNotLaunch`, covering +`AgentControl-RepoCardViewModel-Launch-UnsafeState`. + **RepoCardViewModel_Upgrade_UpdatesPinOnlyWhenNewerVersionExists**: `UpgradeCommand` updates the pin and raises `ReleaseNotesReady` when a newer version is available, and raises `ErrorOccurred` instead when no package source is configured. This scenario is tested by diff --git a/docs/verification/agent-control/repo-sync/package-zip-extractor.md b/docs/verification/agent-control/repo-sync/package-zip-extractor.md index 6e48335..58414c4 100644 --- a/docs/verification/agent-control/repo-sync/package-zip-extractor.md +++ b/docs/verification/agent-control/repo-sync/package-zip-extractor.md @@ -53,10 +53,12 @@ be read from or written/deleted outside the repo root: a managed-folder ancestor linked `.github`), whether empty or already containing real content that must survive; the repo root itself; a reparse point nested *inside* a managed folder (not just above it); and a *dangling* link (whose target no longer exists), which a naive `Directory.Exists`-based check -would silently miss. This scenario is tested by +would silently miss. Every case throws `UnsafeRepositoryStateException` (a dedicated +`InvalidOperationException` subtype distinguishing this security concern from an ordinary +extraction failure) and leaves the affected content untouched. This scenario is tested by `PackageZipExtractor_Extract_ManagedFolderAncestorIsJunction_ThrowsAndDoesNotWriteThroughLink`, `PackageZipExtractor_Extract_ManagedFolderAncestorIsJunctionWithExistingContent_DoesNotBlindDeleteThroughLink`, `PackageZipExtractor_Extract_RepoRootIsJunction_ThrowsAndDoesNotWriteThroughLink`, `PackageZipExtractor_Extract_ManagedFolderContainsNestedJunction_ThrowsAndDoesNotDeleteThroughLink`, and `PackageZipExtractor_Extract_ManagedFolderAncestorIsDanglingLink_ThrowsAndDoesNotBypassGuard`, -covering `AgentControl-PackageZipExtractor-Extract`. +covering `AgentControl-PackageZipExtractor-RejectsReparsePoints`. diff --git a/docs/verification/agent-control/utilities/path-helpers.md b/docs/verification/agent-control/utilities/path-helpers.md index 1a5360b..7928776 100644 --- a/docs/verification/agent-control/utilities/path-helpers.md +++ b/docs/verification/agent-control/utilities/path-helpers.md @@ -2,10 +2,15 @@ #### Verification Approach -`PathHelpers` is verified with unit tests defined in `PathHelpersTests.cs`. Because `PathHelpers` -performs pure path manipulation using only .NET BCL types, no mocking or test doubles are -required. Tests call `PathHelpers.SafePathCombine` directly with controlled base and relative -path arguments and assert on the returned string or the thrown exception type and message. +`PathHelpers` is verified with unit tests defined in `PathHelpersTests.cs`. The lexical +`SafePathCombine` method is verified using only .NET BCL types, with no mocking or test doubles +required - tests call it directly with controlled base and relative path arguments and assert +on the returned string or the thrown exception type and message. The filesystem-aware +`FindReparsePointInAncestry` and `FindReparsePointInDescendants` methods are verified against +real temporary directories, using real NTFS junctions on Windows (created via `mklink /J`, +which - unlike symbolic links - require neither elevated privileges nor Developer Mode) or real +directory symbolic links on Linux/macOS (which do not require an existing target at creation +time, letting a dangling-link scenario be constructed directly). #### Test Environment @@ -20,6 +25,10 @@ N/A - standard test environment. - Absolute paths supplied as the relative argument cause `ArgumentException`. - Null inputs cause `ArgumentNullException`. - A filename beginning with `".."` that is not a traversal sequence is accepted correctly. +- `FindReparsePointInAncestry` and `FindReparsePointInDescendants` return `null` when no + reparse point exists, and the offending path when one does - including when the root/ + directory itself is the reparse point, and including a dangling link whose target does not + currently exist. #### Test Scenarios @@ -74,3 +83,35 @@ the `basePath` argument; an `ArgumentNullException` is thrown, confirming the nu as the `relativePath` argument; an `ArgumentNullException` is thrown, confirming the null guard on `relativePath`. This scenario is tested by `PathHelpers_SafePathCombine_NullRelativePath_ThrowsArgumentNullException`. + +**PathHelpers_FindReparsePointInAncestry_ReportsClosestLinkOrNone**: An ordinary nested +directory tree with no links reports `null`; a junction/symbolic-link ancestor between the +root and the checked path reports that link's own path; a root that is itself a junction +reports the root; and a dangling junction/symbolic-link ancestor (whose target no longer +exists) is still reported, confirming detection uses `File.GetAttributes` rather than +`Directory.Exists`. This scenario is tested by +`PathHelpers_FindReparsePointInAncestry_NoReparsePoints_ReturnsNull`, +`PathHelpers_FindReparsePointInAncestry_AncestorIsJunction_ReturnsJunctionPath`, +`PathHelpers_FindReparsePointInAncestry_RootIsJunction_ReturnsRoot`, and +`PathHelpers_FindReparsePointInAncestry_AncestorIsDanglingLink_ReturnsLinkPath`, covering +`AgentControl-PathHelpers-FindReparsePointInAncestry`. + +**PathHelpers_FindReparsePointInAncestry_NullArguments_ThrowArgumentNullException**: A `null` +`root` or `null` `path` argument each throw `ArgumentNullException`. This scenario is tested by +`PathHelpers_FindReparsePointInAncestry_NullRoot_ThrowsArgumentNullException` and +`PathHelpers_FindReparsePointInAncestry_NullPath_ThrowsArgumentNullException`, covering +`AgentControl-PathHelpers-FindReparsePointInAncestry`. + +**PathHelpers_FindReparsePointInDescendants_ReportsNestedOrSelfLinkOrNone**: An ordinary nested +directory tree with no links reports `null`; a junction nested two levels deep anywhere in the +tree is found and its path reported; and a directory that is itself a junction reports that +same directory. This scenario is tested by +`PathHelpers_FindReparsePointInDescendants_NoReparsePoints_ReturnsNull`, +`PathHelpers_FindReparsePointInDescendants_NestedJunction_ReturnsJunctionPath`, and +`PathHelpers_FindReparsePointInDescendants_DirectoryItselfIsJunction_ReturnsDirectory`, covering +`AgentControl-PathHelpers-FindReparsePointInDescendants`. + +**PathHelpers_FindReparsePointInDescendants_NullDirectory_ThrowsArgumentNullException**: A +`null` `directory` argument throws `ArgumentNullException`. This scenario is tested by +`PathHelpers_FindReparsePointInDescendants_NullDirectory_ThrowsArgumentNullException`, covering +`AgentControl-PathHelpers-FindReparsePointInDescendants`. diff --git a/src/DemaConsulting.AgentControl/AgentToolLauncher/AgentToolLauncher.cs b/src/DemaConsulting.AgentControl/AgentToolLauncher/AgentToolLauncher.cs index 27ed563..8bef58e 100644 --- a/src/DemaConsulting.AgentControl/AgentToolLauncher/AgentToolLauncher.cs +++ b/src/DemaConsulting.AgentControl/AgentToolLauncher/AgentToolLauncher.cs @@ -176,10 +176,13 @@ public static Process Launch(ProcessStartInfo startInfo, ILogger? logger = null) // NativeErrorCode is the actual OS error code behind a Win32Exception - the same // critical diagnostic data highlighted as missing for GitClient's intermittent // failures; captured here too since this is another real-process-spawning path. - effectiveLogger.LogError( - ex, - "Failed to start shell process '{FileName} {Arguments}' after {ElapsedMilliseconds}ms (Win32 NativeErrorCode={NativeErrorCode})", - startInfo.FileName, argumentsText.Value, stopwatch.ElapsedMilliseconds, ex.NativeErrorCode); + if (effectiveLogger.IsEnabled(LogLevel.Error)) + { + effectiveLogger.LogError( + ex, + "Failed to start shell process '{FileName} {Arguments}' after {ElapsedMilliseconds}ms (Win32 NativeErrorCode={NativeErrorCode})", + startInfo.FileName, argumentsText.Value, stopwatch.ElapsedMilliseconds, ex.NativeErrorCode); + } throw new InvalidOperationException( $"Failed to start shell process '{startInfo.FileName} {argumentsText.Value}': {ex.Message}", ex); diff --git a/src/DemaConsulting.AgentControl/GitIntegration/GitClient.cs b/src/DemaConsulting.AgentControl/GitIntegration/GitClient.cs index 91390a2..b2be5eb 100644 --- a/src/DemaConsulting.AgentControl/GitIntegration/GitClient.cs +++ b/src/DemaConsulting.AgentControl/GitIntegration/GitClient.cs @@ -424,10 +424,13 @@ private GitCommandResult RunGit(string repositoryPath, params string[] arguments ? win32Exception.NativeErrorCode : (int?)null; - _logger.LogError( - ex, - "Failed to run '{GitExecutable} {Arguments}' after {ElapsedMilliseconds}ms (Win32 NativeErrorCode={NativeErrorCode})", - _gitExecutablePath, argumentsText.Value, stopwatch.ElapsedMilliseconds, nativeErrorCode); + if (_logger.IsEnabled(LogLevel.Error)) + { + _logger.LogError( + ex, + "Failed to run '{GitExecutable} {Arguments}' after {ElapsedMilliseconds}ms (Win32 NativeErrorCode={NativeErrorCode})", + _gitExecutablePath, argumentsText.Value, stopwatch.ElapsedMilliseconds, nativeErrorCode); + } throw new InvalidOperationException( $"Failed to run '{_gitExecutablePath} {argumentsText.Value}': {ex.Message}", ex); diff --git a/src/DemaConsulting.AgentControl/LauncherUI/RepoCardViewModel.cs b/src/DemaConsulting.AgentControl/LauncherUI/RepoCardViewModel.cs index 549446c..2c23b30 100644 --- a/src/DemaConsulting.AgentControl/LauncherUI/RepoCardViewModel.cs +++ b/src/DemaConsulting.AgentControl/LauncherUI/RepoCardViewModel.cs @@ -546,11 +546,26 @@ private void RecordLaunched() /// command and spawn the process regardless of that call's outcome. A user may want to /// launch their agentic CLI tool to help with agent-package migration, or simply because /// an agentic tool is useful even with zero agent files present - either way, sync state - /// must never stand in the way of launching. + /// must never stand in the way of launching. The sole exception is + /// , which + /// deliberately does not catch: unlike an + /// ordinary sync failure (missing source, missing pinned version, a locked file), it means + /// a managed folder is reachable only through a reparse point (symlink/junction), so + /// launching could run the agentic tool against content outside the repo root. That + /// specific failure mode blocks the launch instead - is still + /// raised explaining why, but no process is spawned. /// private void Launch() { - EnsureAgentFilesSyncedBeforeLaunch(); + try + { + EnsureAgentFilesSyncedBeforeLaunch(); + } + catch (UnsafeRepositoryStateException ex) + { + ErrorOccurred?.Invoke(this, $"Refusing to launch: {ex.Message}"); + return; + } try { @@ -586,10 +601,19 @@ private void Launch() /// if no sync action was needed or attempted (the repo has /// committed agent files, has no pin, or the managed folders were already present), or a /// missing set was silently re-extracted successfully; if a sync - /// attempt was made and failed, in which case has already been - /// raised as a non-blocking warning explaining why. Either way, - /// proceeds to spawn the agent tool regardless of this return value. + /// attempt was made and failed for an ordinary reason (source unreachable, pinned version + /// missing, an I/O failure), in which case has already been + /// raised as a non-blocking warning explaining why and proceeds to + /// spawn the agent tool regardless of this return value. /// + /// + /// Thrown (not caught here) when detects that a + /// managed folder is reachable only through a reparse point (symlink/junction). Unlike + /// every other failure this method absorbs, this represents a genuine security concern - + /// launching could run the agentic tool against content outside the repo root - so + /// deliberately does not proceed when this propagates, breaking the + /// "sync state never blocks launch" policy described above for this one case only. + /// /// /// /// If this repo has committed agent files ( is @@ -610,7 +634,9 @@ private void Launch() /// If a pin exists and is /// already , this returns immediately with no /// re-extraction - the common "already synced" case must not pay any extra I/O cost on - /// every launch. + /// every launch. itself treats a + /// folder reached through a reparse point as not present, so this case only applies to + /// genuinely present, unlinked managed folders. /// /// /// If a pin exists but one or more managed folders are missing (e.g. a freshly cloned @@ -621,10 +647,13 @@ private void Launch() /// through - architecture.md's /// ensure-synced-before-launch bullet never mentions showing release notes, unlike its /// Select-Package bullet, so a silent background repair must not pop a release-notes - /// window on every launch. If this re-extraction attempt fails for any reason (source - /// unreachable, pinned version missing, extraction I/O failure), - /// is raised as a non-blocking warning and this returns - but the - /// launch still proceeds regardless. + /// window on every launch. If this re-extraction attempt fails for an ordinary reason + /// (source unreachable, pinned version missing, extraction I/O failure), + /// is raised as a non-blocking warning and this returns + /// - but the launch still proceeds regardless. If it instead fails + /// because a managed folder is reachable only through a reparse point, the resulting + /// is deliberately left uncaught (see the + /// exception list above) rather than absorbed into this best-effort return value. /// /// /// Marked (not ) rather than tested @@ -674,6 +703,12 @@ internal bool EnsureAgentFilesSyncedBeforeLaunch() PackageZipExtractor.Extract(pinnedPackage.FilePath, RepoPath); return true; } + catch (UnsafeRepositoryStateException) + { + // Deliberately not absorbed into the best-effort false/ErrorOccurred pattern below - + // Launch must treat this as a hard stop, not a "launching anyway" warning. + throw; + } catch (InvalidOperationException ex) { ErrorOccurred?.Invoke(this, $"Failed to sync agent files: {ex.Message} Launching anyway."); diff --git a/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs b/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs index ba0e9a8..755ccc6 100644 --- a/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs +++ b/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs @@ -78,6 +78,13 @@ internal static class PackageZipExtractor /// there is no rollback: a partially-applied change is possible and must be resolved /// manually by the caller. /// + /// + /// Thrown (derives from ) when the repo root, a + /// managed folder's ancestor, the folder itself, or any descendant of it is a reparse + /// point (symlink/junction) - callers that must react differently to this specific + /// security concern (rather than an ordinary extraction failure) can catch it before the + /// general case. + /// public static void Extract(string zipPath, string repoRoot) { ArgumentNullException.ThrowIfNull(zipPath); @@ -148,10 +155,10 @@ public static bool AllManagedFoldersExist(string repoRoot) /// if the folder exists and no reparse point sits between it /// and (inclusive); otherwise . /// - /// Deliberately swallows (as "not genuinely present") both the documented reparse-point - /// rejection and any I/O failure while walking the ancestor chain - e.g. an + /// Deliberately swallows (as "not genuinely present") both a found reparse point and any + /// I/O failure while inspecting the ancestor chain - e.g. an /// from an ACL-restricted ancestor, which - /// does not itself catch. This keeps + /// does not itself catch. This keeps /// 's contract to only ever throw /// (its callers, e.g. /// RepoCardViewModel.EnsureAgentFilesSyncedBeforeLaunch, only guard against @@ -168,19 +175,17 @@ private static bool ManagedFolderGenuinelyExists(string normalizedRoot, string r try { - EnsureNoSymlinkAncestors(normalizedRoot, folderPath, relativeFolder); + return PathHelpers.FindReparsePointInAncestry(normalizedRoot, folderPath) is null; } - catch (Exception ex) when (ex is InvalidOperationException or IOException or UnauthorizedAccessException) + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { - // A reparse-point root/ancestor/self, or an inability to even inspect one (e.g. an - // ACL-restricted ancestor), means this folder cannot be confirmed as genuinely - // present under normalizedRoot; treat it the same as missing so callers fall back to - // Extract, which will itself surface the underlying failure as a hard error instead - // of silently trusting - or crashing on - content reached through a symlink/junction. + // An inability to even inspect an ancestor (e.g. an ACL-restricted directory) means + // this folder cannot be confirmed as genuinely present under normalizedRoot; treat it + // the same as missing so callers fall back to Extract, which will itself surface the + // underlying failure as a hard error instead of silently trusting - or crashing on - + // content reached through a symlink/junction. return false; } - - return true; } /// @@ -246,8 +251,10 @@ private static ZipArchive OpenArchive(string zipPath) /// Absolute path to the repository root. /// The managed folder's path relative to the repo root. /// Thrown when the folder exists but cannot be - /// deleted, or when the repo root, an ancestor, the folder itself, or any descendant of - /// the folder is a reparse point (symlink/junction). + /// deleted. + /// Thrown when the repo root, an ancestor, + /// the folder itself, or any descendant of the folder is a reparse point + /// (symlink/junction). private static void DeleteManagedFolder(string repoRoot, string relativeFolder) { var folderPath = PathHelpers.SafePathCombine(repoRoot, relativeFolder); @@ -290,16 +297,41 @@ private static void DeleteManagedFolder(string repoRoot, string relativeFolder) /// The directory to delete. /// A short description of the folder being deleted, for the exception /// message. - /// Thrown when a nested reparse point is - /// encountered. + /// + /// The entire tree is preflighted for reparse points () + /// before anything is deleted. Interleaving the reparse-point check with the actual + /// delete (checking each directory immediately before deleting its files) would still let + /// an ordinary sibling file be permanently deleted before a reparse point discovered + /// later in the same tree aborts the operation, leaving the managed folder partially + /// destroyed instead of untouched. + /// + /// Thrown when a nested reparse point is + /// encountered anywhere in the tree; nothing is deleted in this case. private static void DeleteDirectoryRejectingReparsePoints(string directoryPath, string context) { - if (File.GetAttributes(directoryPath).HasFlag(FileAttributes.ReparsePoint)) + var reparsePoint = PathHelpers.FindReparsePointInDescendants(directoryPath); + if (reparsePoint is not null) { - throw new InvalidOperationException( - $"'{context}' contains a symlinked directory '{directoryPath}'; refusing to delete through it."); + throw new UnsafeRepositoryStateException( + $"'{context}' contains a symlinked directory '{reparsePoint}'; refusing to delete through it."); } + DeleteDirectoryTree(directoryPath); + } + + /// + /// Recursively deletes every file and subdirectory under , + /// then the now-empty directory itself. + /// + /// The directory to delete. + /// + /// Assumes has already verified the + /// whole tree contains no reparse points; this method performs no such check itself, since + /// re-checking here would re-introduce the same interleaved check-then-delete race the + /// two-phase split in exists to avoid. + /// + private static void DeleteDirectoryTree(string directoryPath) + { foreach (var filePath in Directory.GetFiles(directoryPath)) { File.Delete(filePath); @@ -307,7 +339,7 @@ private static void DeleteDirectoryRejectingReparsePoints(string directoryPath, foreach (var subdirectoryPath in Directory.GetDirectories(directoryPath)) { - DeleteDirectoryRejectingReparsePoints(subdirectoryPath, context); + DeleteDirectoryTree(subdirectoryPath); } Directory.Delete(directoryPath, recursive: false); @@ -320,9 +352,10 @@ private static void DeleteDirectoryRejectingReparsePoints(string directoryPath, /// /// The zip entry to consider. /// Absolute path to the repository root. - /// Thrown when the entry's path is invalid - /// (including resolving outside ), or when the repo root, an - /// ancestor, or the destination directory itself is a reparse point (symlink/junction). + /// Thrown when the entry's path is invalid, + /// including resolving outside . + /// Thrown when the repo root, an ancestor, or + /// the destination directory itself is a reparse point (symlink/junction). private static void ExtractEntryIfManaged(ZipArchiveEntry entry, string repoRoot) { // Directory entries have an empty Name (only FullName ends with '/'); skip them, as @@ -380,9 +413,10 @@ private static void ExtractEntryIfManaged(ZipArchiveEntry entry, string repoRoot } /// - /// Walks upward from to (and including) - /// itself, rejecting the operation if any existing path in the walk is itself a reparse - /// point (symlink/junction). + /// Rejects the operation if or any path segment between it and + /// (inclusive of both ends) is itself a reparse point + /// (symlink/junction), throwing a domain-specific + /// with a message naming and the offending path. /// /// The already-resolved () repo /// root; also checked, since a symlinked/junctioned repo root would otherwise let every @@ -391,60 +425,25 @@ private static void ExtractEntryIfManaged(ZipArchiveEntry entry, string repoRoot /// destination directory (before extraction) or a managed folder about to be blind-deleted. /// A short description of the path/entry, for the exception message. /// - /// is purely lexical - it never resolves - /// filesystem links - so the earlier relative-path containment check alone cannot detect - /// a symlinked ancestor redirecting a nominally-contained path outside the repo root. - /// Reparse-point status is queried via guarded by - /// a not-found catch, rather than : the latter - /// resolves the link's target to decide existence and so returns - /// (silently skipping the check) for a *dangling* symlink/junction, whereas - /// reports the reparse point's own attributes - /// without requiring its target to exist. This only guards against paths that already + /// A thin, exception-throwing policy wrapper around + /// , which owns the actual + /// filesystem-aware detection logic (see its own doc remarks for why a lexical-only check + /// is insufficient and why is used over + /// ). This only guards against paths that already /// exist at the time of the check; it does not eliminate a race where a path is replaced /// with a symlink between this check and /// // /// . /// - /// Thrown when a path in the walk is a reparse - /// point. + /// Thrown when a path in the walk is a + /// reparse point. private static void EnsureNoSymlinkAncestors(string repoRoot, string path, string context) { - var current = Path.TrimEndingDirectorySeparator(path); - var normalizedRoot = Path.TrimEndingDirectorySeparator(repoRoot); - - while (!string.IsNullOrEmpty(current)) + var reparsePoint = PathHelpers.FindReparsePointInAncestry(repoRoot, path); + if (reparsePoint is not null) { - FileAttributes attributes; - try - { - attributes = File.GetAttributes(current); - } - catch (Exception ex) when (ex is FileNotFoundException or DirectoryNotFoundException) - { - // This path segment does not exist yet (e.g. a destination directory that will - // be created by this same operation) - nothing to reject here, keep walking - // upward. - if (string.Equals(current, normalizedRoot, StringComparison.OrdinalIgnoreCase)) - { - break; - } - - current = Path.GetDirectoryName(current); - continue; - } - - if (attributes.HasFlag(FileAttributes.ReparsePoint)) - { - throw new InvalidOperationException( - $"'{context}' resolves through a symlinked directory '{current}'."); - } - - if (string.Equals(current, normalizedRoot, StringComparison.OrdinalIgnoreCase)) - { - break; - } - - current = Path.GetDirectoryName(current); + throw new UnsafeRepositoryStateException( + $"'{context}' resolves through a symlinked directory '{reparsePoint}'."); } } diff --git a/src/DemaConsulting.AgentControl/RepoSync/UnsafeRepositoryStateException.cs b/src/DemaConsulting.AgentControl/RepoSync/UnsafeRepositoryStateException.cs new file mode 100644 index 0000000..481145a --- /dev/null +++ b/src/DemaConsulting.AgentControl/RepoSync/UnsafeRepositoryStateException.cs @@ -0,0 +1,48 @@ +// Copyright (c) DEMA Consulting +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. + +namespace DemaConsulting.AgentControl.RepoSync; + +/// +/// Thrown by when a reparse point (symlink/junction) is +/// found somewhere it would let a blind-delete-and-replace operation read from or write to a +/// location outside the intended repository directory. +/// +/// +/// Deliberately distinct from the plain thrown for +/// ordinary extraction failures (a corrupt zip, a locked file, an unreachable package +/// source), even though it derives from it for backward-compatible catch-by-base-type +/// behavior. Callers that need to react differently to a genuine security concern - e.g. +/// RepoCardViewModel.EnsureAgentFilesSyncedBeforeLaunch, which must not let its +/// "launch proceeds regardless of sync failure" best-effort policy also apply to a detected +/// symlink/junction attack - can catch this type specifically before the general +/// case. +/// +internal sealed class UnsafeRepositoryStateException : InvalidOperationException +{ + /// + /// Initializes a new instance of the class. + /// + /// A message describing the unsafe reparse point that was detected. + public UnsafeRepositoryStateException(string message) + : base(message) + { + } +} diff --git a/src/DemaConsulting.AgentControl/Utilities/PathHelpers.cs b/src/DemaConsulting.AgentControl/Utilities/PathHelpers.cs index 9197d55..e86a1d8 100644 --- a/src/DemaConsulting.AgentControl/Utilities/PathHelpers.cs +++ b/src/DemaConsulting.AgentControl/Utilities/PathHelpers.cs @@ -23,6 +23,16 @@ namespace DemaConsulting.AgentControl.Utilities; /// /// Helper utilities for safe path operations. /// +/// +/// Combines two complementary halves of path safety: is purely +/// lexical (string-level containment, no file-system I/O), while +/// / are +/// filesystem-aware (detecting symlinks/junctions that a lexical check alone cannot see). +/// Callers that assemble a path and then touch the filesystem at it should normally use both: +/// first to reject a textually-escaping path, then one of the +/// reparse-point finders to reject a link that would otherwise redirect an apparently-safe +/// path outside its intended root. +/// internal static class PathHelpers { /// @@ -67,4 +77,132 @@ internal static string SafePathCombine(string basePath, string relativePath) return combinedPath; } + + /// + /// Searches upward from to (and including) , + /// looking for a directory that is itself a reparse point (symlink/junction). + /// + /// The already fully-resolved () + /// boundary at which the upward walk stops; also checked, since a symlinked/junctioned + /// root would otherwise let every operation beneath it silently write through the link. + /// The already fully-resolved path whose ancestry (up to and including + /// ) is inspected. + /// + /// The closest-to- segment that is a reparse point, or + /// if no segment between and + /// (inclusive of both ends) is one. + /// + /// + /// This is the filesystem-aware counterpart to : that method + /// is purely lexical and never touches the filesystem, so it cannot detect a symlinked/ + /// junctioned ancestor silently redirecting a nominally-contained path outside its + /// intended root. Reparse-point status is queried via + /// rather than : + /// the latter resolves (follows) a link to test whether its target exists, and so returns + /// (silently missing the reparse point) for a *dangling* + /// symlink/junction, whereas reports the reparse + /// point's own attributes without requiring its target to exist. A path segment that does + /// not exist yet (e.g. a destination directory a caller is about to create) is not itself + /// a finding; the walk simply continues upward past it. Callers that need to react to a + /// found reparse point (e.g. by throwing, or by treating the path as unusable) decide that + /// policy themselves - this method only ever reports what it found. This only reflects the + /// state of the filesystem at the moment of the call; it does not eliminate a race where a + /// segment is replaced with a reparse point between this check and a caller's subsequent + /// file operation. Stateless and thread-safe, but - unlike - + /// performs real file-system I/O. + /// + /// Thrown when or + /// is . + /// Thrown when a path segment's attributes cannot be read for a + /// reason other than the segment not existing. + /// Thrown when the caller lacks permission to + /// read a path segment's attributes. + internal static string? FindReparsePointInAncestry(string root, string path) + { + ArgumentNullException.ThrowIfNull(root); + ArgumentNullException.ThrowIfNull(path); + + var current = Path.TrimEndingDirectorySeparator(path); + var normalizedRoot = Path.TrimEndingDirectorySeparator(root); + + while (!string.IsNullOrEmpty(current)) + { + FileAttributes attributes; + try + { + attributes = File.GetAttributes(current); + } + catch (Exception ex) when (ex is FileNotFoundException or DirectoryNotFoundException) + { + if (string.Equals(current, normalizedRoot, StringComparison.OrdinalIgnoreCase)) + { + break; + } + + current = Path.GetDirectoryName(current); + continue; + } + + if (attributes.HasFlag(FileAttributes.ReparsePoint)) + { + return current; + } + + if (string.Equals(current, normalizedRoot, StringComparison.OrdinalIgnoreCase)) + { + break; + } + + current = Path.GetDirectoryName(current); + } + + return null; + } + + /// + /// Recursively searches (inclusive) for the first nested + /// directory that is a reparse point (symlink/junction). + /// + /// The already-existing directory (and its descendants) to search. + /// + /// The path of the first reparse point found ( itself, or a + /// descendant), or if none exists anywhere in the tree. + /// + /// + /// Exists for callers that need to recursively delete, copy, or otherwise walk a directory + /// tree without following filesystem links nested inside it - unlike + /// 's recursive mode, which follows such links + /// and can affect content outside the tree being processed. The whole tree is searched up + /// front, rather than interleaving this check with file-by-file processing, so a caller + /// can preflight an entire operation and fail closed before acting on (e.g. deleting) any + /// part of the tree if a reparse point exists anywhere within it. As with + /// , this only reflects a single point in time and + /// performs real file-system I/O. + /// + /// Thrown when is + /// . + /// Thrown when a directory's attributes or contents cannot be + /// read for a reason other than a permission failure. + /// Thrown when the caller lacks permission to + /// read a directory's attributes or contents. + internal static string? FindReparsePointInDescendants(string directory) + { + ArgumentNullException.ThrowIfNull(directory); + + if (File.GetAttributes(directory).HasFlag(FileAttributes.ReparsePoint)) + { + return directory; + } + + foreach (var subdirectory in Directory.GetDirectories(directory)) + { + var found = FindReparsePointInDescendants(subdirectory); + if (found is not null) + { + return found; + } + } + + return null; + } } diff --git a/test/DemaConsulting.AgentControl.Tests/LauncherUI/RepoCardViewModelTests.cs b/test/DemaConsulting.AgentControl.Tests/LauncherUI/RepoCardViewModelTests.cs index a33ffb7..27126bf 100644 --- a/test/DemaConsulting.AgentControl.Tests/LauncherUI/RepoCardViewModelTests.cs +++ b/test/DemaConsulting.AgentControl.Tests/LauncherUI/RepoCardViewModelTests.cs @@ -974,6 +974,91 @@ public void RepoCardViewModel_LaunchCommand_SyncFailsOrNoPin_StillLaunches(bool Assert.NotNull(card.LastLaunchedUtc); } + /// + /// Test that LaunchCommand does not spawn the agent-tool process, and raises + /// instead, when the ensure-synced-before-launch + /// check detects that a managed folder is only reachable through a reparse point + /// (symlink/junction) - the one deliberate, narrow exception to the "sync state never + /// blocks launch" policy exercised by . + /// + [Fact] + public void RepoCardViewModel_LaunchCommand_ManagedFolderAncestorIsJunction_DoesNotLaunch() + { + // Arrange: a repo whose '.github' folder is a junction, so AllManagedFoldersExist treats + // the managed folders as missing and EnsureAgentFilesSyncedBeforeLaunch attempts to + // re-extract into it - which PackageZipExtractor refuses, since deleting/writing through + // the junction could affect content outside repoRoot. + var repoRoot = CreateTempDirectory(); + var linkTarget = CreateTempDirectory(); + var sourceDir = CreateTempDirectory(); + CreatePackageZip(sourceDir, "contoso-agents", "1.0.0"); + CreateJunction(Path.Combine(repoRoot, ".github"), linkTarget); + var settings = new AppSettings + { + AgentTool = AgentToolKind.Custom, + CustomAgentCommand = OperatingSystem.IsWindows() ? "cmd /c exit 0" : "true", + PackageSourcePath = sourceDir + }; + var card = CreateCard(repoRoot, "contoso-agents", "1.0.0", settings); + var raised = false; + card.LaunchRecorded += (_, _) => raised = true; + string? capturedError = null; + card.ErrorOccurred += (_, message) => capturedError = message; + + try + { + // Act + card.LaunchCommand.Execute(null); + + // Assert: no process was spawned, and the error explains why + Assert.False(raised); + Assert.Null(card.LastLaunchedUtc); + Assert.NotNull(capturedError); + Assert.Contains("Refusing to launch", capturedError, StringComparison.Ordinal); + } + finally + { + // Remove the junction entry itself (not its target's contents) before the temp + // directories tracked in _tempPaths are recursively deleted in Dispose - avoids + // Directory.Delete(recursive: true) following the still-live link during cleanup. + Directory.Delete(Path.Combine(repoRoot, ".github"), recursive: false); + } + } + + /// + /// Creates an NTFS directory junction at pointing to + /// (Windows), or a real directory symbolic link + /// (Linux/macOS) - both are reparse points for the purposes of this test, and symbolic + /// links require neither an existing target nor elevated privileges on non-Windows + /// platforms. + /// + private static void CreateJunction(string linkPath, string targetPath) + { + if (OperatingSystem.IsWindows()) + { + var startInfo = new System.Diagnostics.ProcessStartInfo("cmd.exe", $"/c mklink /J \"{linkPath}\" \"{targetPath}\"") + { + RedirectStandardOutput = true, + RedirectStandardError = true, + UseShellExecute = false, + CreateNoWindow = true + }; + + using var process = System.Diagnostics.Process.Start(startInfo) + ?? throw new InvalidOperationException("Failed to start 'cmd.exe' to create junction."); + process.WaitForExit(); + if (process.ExitCode != 0) + { + throw new InvalidOperationException( + $"Failed to create junction '{linkPath}' -> '{targetPath}': {process.StandardError.ReadToEnd()}"); + } + } + else + { + Directory.CreateSymbolicLink(linkPath, targetPath); + } + } + /// /// Creates a for the given repo path, cached pin fields, /// and settings snapshot. When a package name is supplied, the corresponding pin file is diff --git a/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs b/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs index d4af514..9069af5 100644 --- a/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs +++ b/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs @@ -326,7 +326,7 @@ public void PackageZipExtractor_Extract_ManagedFolderAncestorIsJunction_ThrowsAn CreateJunction(Path.Combine(repoRoot, ".github"), linkTarget); // Act / Assert: extraction is refused rather than writing through the junction - Assert.Throws(() => PackageZipExtractor.Extract(zipPath, repoRoot)); + Assert.Throws(() => PackageZipExtractor.Extract(zipPath, repoRoot)); Assert.False(File.Exists(Path.Combine(linkTarget, "agents", "copilot.md"))); } finally @@ -372,7 +372,7 @@ public void PackageZipExtractor_Extract_ManagedFolderAncestorIsJunctionWithExist // Act / Assert: extraction is refused, and the pre-existing content behind the // junction was never blind-deleted - Assert.Throws(() => PackageZipExtractor.Extract(zipPath, repoRoot)); + Assert.Throws(() => PackageZipExtractor.Extract(zipPath, repoRoot)); Assert.Equal("must survive", File.ReadAllText(keepFilePath)); } finally @@ -411,7 +411,7 @@ public void PackageZipExtractor_Extract_RepoRootIsJunction_ThrowsAndDoesNotWrite CreateJunction(repoRoot, linkTarget); // Act / Assert: extraction is refused, and nothing was written through the link - Assert.Throws(() => PackageZipExtractor.Extract(zipPath, repoRoot)); + Assert.Throws(() => PackageZipExtractor.Extract(zipPath, repoRoot)); Assert.False(File.Exists(Path.Combine(linkTarget, ".github", "agents", "copilot.md"))); } finally @@ -455,7 +455,7 @@ public void PackageZipExtractor_Extract_ManagedFolderContainsNestedJunction_Thro CreateJunction(Path.Combine(agentsDir, "linked"), linkTarget); // Act / Assert: the blind delete is refused, and the linked content was never deleted - Assert.Throws(() => PackageZipExtractor.Extract(zipPath, repoRoot)); + Assert.Throws(() => PackageZipExtractor.Extract(zipPath, repoRoot)); Assert.Equal("must survive", File.ReadAllText(keepFilePath)); } finally @@ -491,14 +491,27 @@ public void PackageZipExtractor_Extract_ManagedFolderAncestorIsDanglingLink_Thro CreateDanglingLink(githubPath); // Act / Assert: extraction is refused, not silently allowed through the dangling link - Assert.Throws(() => PackageZipExtractor.Extract(zipPath, repoRoot)); + Assert.Throws(() => PackageZipExtractor.Extract(zipPath, repoRoot)); } finally { File.Delete(zipPath); - // The dangling link entry itself must be removed (non-recursively - there is no - // real target content behind it) before the repo root can be deleted. - Directory.Delete(githubPath); + // The dangling link entry itself must be removed before the repo root can be + // deleted. Directory.Delete follows the link (via stat/lstat) to confirm it is a + // directory before removing it, which fails with DirectoryNotFoundException for a + // *dangling* link whose target no longer exists - this only works here because + // Windows junction metadata lives on the link entry itself, independent of target + // validity. On Linux/macOS, a dangling symlink must instead be removed with + // File.Delete, which unlinks the directory entry directly without following it. + if (OperatingSystem.IsWindows()) + { + Directory.Delete(githubPath); + } + else + { + File.Delete(githubPath); + } + Directory.Delete(repoRoot, recursive: true); } } diff --git a/test/DemaConsulting.AgentControl.Tests/Utilities/PathHelpersTests.cs b/test/DemaConsulting.AgentControl.Tests/Utilities/PathHelpersTests.cs index abdc74e..83991c6 100644 --- a/test/DemaConsulting.AgentControl.Tests/Utilities/PathHelpersTests.cs +++ b/test/DemaConsulting.AgentControl.Tests/Utilities/PathHelpersTests.cs @@ -190,6 +190,310 @@ public void PathHelpers_SafePathCombine_NullRelativePath_ThrowsArgumentNullExcep Assert.Throws(() => PathHelpers.SafePathCombine("/home/user", null!)); } + + /// + /// Test that FindReparsePointInAncestry returns null when no segment between path and + /// root is a reparse point. + /// + [Fact] + public void PathHelpers_FindReparsePointInAncestry_NoReparsePoints_ReturnsNull() + { + // Arrange: an ordinary nested directory with no symlinks/junctions anywhere + var root = CreateTempDirectory(); + try + { + var nested = Path.Combine(root, "a", "b"); + Directory.CreateDirectory(nested); + + // Act + var result = PathHelpers.FindReparsePointInAncestry(root, nested); + + // Assert + Assert.Null(result); + } + finally + { + Directory.Delete(root, recursive: true); + } + } + + /// + /// Test that FindReparsePointInAncestry finds a junction between root and path. + /// + [Fact] + public void PathHelpers_FindReparsePointInAncestry_AncestorIsJunction_ReturnsJunctionPath() + { + // Arrange: root/link is a junction, and the path being checked is a descendant of it + var root = CreateTempDirectory(); + var linkTarget = CreateTempDirectory(); + var linkPath = Path.Combine(root, "link"); + try + { + CreateJunction(linkPath, linkTarget); + var descendant = Path.Combine(linkPath, "nested"); + + // Act + var result = PathHelpers.FindReparsePointInAncestry(root, descendant); + + // Assert + Assert.Equal(linkPath, result); + } + finally + { + Directory.Delete(linkPath, recursive: false); + Directory.Delete(root, recursive: true); + Directory.Delete(linkTarget, recursive: true); + } + } + + /// + /// Test that FindReparsePointInAncestry finds root itself when it is a junction. + /// + [Fact] + public void PathHelpers_FindReparsePointInAncestry_RootIsJunction_ReturnsRoot() + { + // Arrange: root is itself a junction + var parent = CreateTempDirectory(); + var linkTarget = CreateTempDirectory(); + var root = Path.Combine(parent, "root-link"); + try + { + CreateJunction(root, linkTarget); + var descendant = Path.Combine(root, "nested"); + + // Act + var result = PathHelpers.FindReparsePointInAncestry(root, descendant); + + // Assert + Assert.Equal(root, result); + } + finally + { + Directory.Delete(root, recursive: false); + Directory.Delete(parent, recursive: true); + Directory.Delete(linkTarget, recursive: true); + } + } + + /// + /// Test that FindReparsePointInAncestry finds a dangling junction ancestor - one whose + /// target no longer exists - rather than silently missing it (as Directory.Exists would). + /// + [Fact] + public void PathHelpers_FindReparsePointInAncestry_AncestorIsDanglingLink_ReturnsLinkPath() + { + // Arrange + var root = CreateTempDirectory(); + var linkPath = Path.Combine(root, "dangling"); + try + { + CreateDanglingLink(linkPath); + var descendant = Path.Combine(linkPath, "nested", "deeper"); + + // Act + var result = PathHelpers.FindReparsePointInAncestry(root, descendant); + + // Assert + Assert.Equal(linkPath, result); + } + finally + { + if (OperatingSystem.IsWindows()) + { + Directory.Delete(linkPath, recursive: false); + } + else + { + File.Delete(linkPath); + } + + Directory.Delete(root, recursive: true); + } + } + + /// + /// Test that FindReparsePointInAncestry throws ArgumentNullException when root is null. + /// + [Fact] + public void PathHelpers_FindReparsePointInAncestry_NullRoot_ThrowsArgumentNullException() + { + Assert.Throws(() => + PathHelpers.FindReparsePointInAncestry(null!, "/some/path")); + } + + /// + /// Test that FindReparsePointInAncestry throws ArgumentNullException when path is null. + /// + [Fact] + public void PathHelpers_FindReparsePointInAncestry_NullPath_ThrowsArgumentNullException() + { + Assert.Throws(() => + PathHelpers.FindReparsePointInAncestry("/some/root", null!)); + } + + /// + /// Test that FindReparsePointInDescendants returns null for a tree with no reparse points. + /// + [Fact] + public void PathHelpers_FindReparsePointInDescendants_NoReparsePoints_ReturnsNull() + { + // Arrange: an ordinary nested directory tree with no symlinks/junctions anywhere + var root = CreateTempDirectory(); + try + { + Directory.CreateDirectory(Path.Combine(root, "a", "b")); + + // Act + var result = PathHelpers.FindReparsePointInDescendants(root); + + // Assert + Assert.Null(result); + } + finally + { + Directory.Delete(root, recursive: true); + } + } + + /// + /// Test that FindReparsePointInDescendants finds a nested junction anywhere in the tree. + /// + [Fact] + public void PathHelpers_FindReparsePointInDescendants_NestedJunction_ReturnsJunctionPath() + { + // Arrange: root/a/linked is a junction nested two levels deep + var root = CreateTempDirectory(); + var linkTarget = CreateTempDirectory(); + var linkPath = Path.Combine(root, "a", "linked"); + try + { + Directory.CreateDirectory(Path.Combine(root, "a")); + CreateJunction(linkPath, linkTarget); + + // Act + var result = PathHelpers.FindReparsePointInDescendants(root); + + // Assert + Assert.Equal(linkPath, result); + } + finally + { + Directory.Delete(linkPath, recursive: false); + Directory.Delete(root, recursive: true); + Directory.Delete(linkTarget, recursive: true); + } + } + + /// + /// Test that FindReparsePointInDescendants returns the directory itself when it is a + /// reparse point. + /// + [Fact] + public void PathHelpers_FindReparsePointInDescendants_DirectoryItselfIsJunction_ReturnsDirectory() + { + // Arrange + var parent = CreateTempDirectory(); + var linkTarget = CreateTempDirectory(); + var linkPath = Path.Combine(parent, "link"); + try + { + CreateJunction(linkPath, linkTarget); + + // Act + var result = PathHelpers.FindReparsePointInDescendants(linkPath); + + // Assert + Assert.Equal(linkPath, result); + } + finally + { + Directory.Delete(linkPath, recursive: false); + Directory.Delete(parent, recursive: true); + Directory.Delete(linkTarget, recursive: true); + } + } + + /// + /// Test that FindReparsePointInDescendants throws ArgumentNullException when directory is + /// null. + /// + [Fact] + public void PathHelpers_FindReparsePointInDescendants_NullDirectory_ThrowsArgumentNullException() + { + Assert.Throws(() => + PathHelpers.FindReparsePointInDescendants(null!)); + } + + /// + /// Creates an NTFS directory junction at pointing to + /// (Windows), or a real directory symbolic link + /// (Linux/macOS), since junctions and symbolic links behave identically for the purposes + /// of these tests - both are reparse points reported by + /// - and symbolic links do not require an + /// existing target or elevated privileges on non-Windows platforms. + /// + /// The link's path; its parent must exist and it must not already + /// exist. + /// The existing directory the link points to. + private static void CreateJunction(string linkPath, string targetPath) + { + if (OperatingSystem.IsWindows()) + { + var startInfo = new System.Diagnostics.ProcessStartInfo("cmd.exe", $"/c mklink /J \"{linkPath}\" \"{targetPath}\"") + { + RedirectStandardOutput = true, + RedirectStandardError = true, + UseShellExecute = false, + CreateNoWindow = true + }; + + using var process = System.Diagnostics.Process.Start(startInfo) + ?? throw new InvalidOperationException("Failed to start 'cmd.exe' to create junction."); + process.WaitForExit(); + if (process.ExitCode != 0) + { + throw new InvalidOperationException( + $"Failed to create junction '{linkPath}' -> '{targetPath}': {process.StandardError.ReadToEnd()}"); + } + } + else + { + Directory.CreateSymbolicLink(linkPath, targetPath); + } + } + + /// + /// Creates a dangling directory symlink/junction at - one + /// whose target does not exist - using a real symbolic link on Linux/macOS (created + /// without any target validation) or an NTFS junction on Windows (created against a real + /// temporary target that is deleted immediately afterward). + /// + /// The link's path; its parent must exist and it must not already + /// exist. + private static void CreateDanglingLink(string linkPath) + { + if (OperatingSystem.IsWindows()) + { + var target = CreateTempDirectory(); + CreateJunction(linkPath, target); + Directory.Delete(target); + } + else + { + Directory.CreateSymbolicLink(linkPath, Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N"))); + } + } + + /// + /// Creates a unique temporary directory for test isolation. + /// + /// The created directory's path. + private static string CreateTempDirectory() + { + var path = Path.Combine(Path.GetTempPath(), "agentcontrol_path_helpers_test_" + Guid.NewGuid()); + Directory.CreateDirectory(path); + return path; + } } From 27e8c36eb1432b93beeae601eb535af82a66a15e Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Mon, 14 Sep 2026 01:37:41 -0400 Subject: [PATCH 08/10] Add preflight/preflighted/preflights to cspell dictionary Fixes the actual CI Quality Checks failure: cspell flagged these as unknown words in doc comments describing the reparse-point preflight scan added by the PathHelpers centralization refactor. Not a dotnet-format issue. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .cspell.yaml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.cspell.yaml b/.cspell.yaml index a1cebce..77549b2 100644 --- a/.cspell.yaml +++ b/.cspell.yaml @@ -35,6 +35,9 @@ words: - mklink - NTFS - Postconditions + - preflight + - preflighted + - preflights - buildmark - buildtransitive - contentfiles From 063076bb849ddbd4eec413ea7bd8685a68577cca Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Mon, 14 Sep 2026 01:58:41 -0400 Subject: [PATCH 09/10] Remove reparse-point/symlink hardening from PackageZipExtractor Reverts the symlink/junction/reparse-point detection and rejection logic added across several PR review rounds (EnsureNoSymlinkAncestors, PathHelpers.FindReparsePointInAncestry/InDescendants, UnsafeRepositoryStateException, and the associated RepoCardViewModel launch-blocking behavior), along with their tests and documentation. This scope was never requested; it grew out of repeated PR review findings asking for 'one more edge case'. Per direction, filesystem- level symlink/junction defenses are out of scope for this tool - a user who mounts or links a managed folder is on their own. The legitimate, unrelated fixes are kept: - PathHelpers.SafePathCombine (lexical zip-slip containment, S6096) - The canonical ('..'-resolved) relative-path check in ExtractEntryIfManaged that closes a real managed-folder-membership traversal bypass Build and full test suite (202 tests) verified green after the revert; fix.ps1/lint.ps1 both clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .cspell.yaml | 7 - .reviewmark.yaml | 1 - .../launcher-ui/repo-card-view-model.md | 39 +- docs/design/agent-control/repo-sync.md | 17 +- .../repo-sync/package-zip-extractor.md | 46 +-- docs/design/agent-control/utilities.md | 44 +-- .../agent-control/utilities/path-helpers.md | 72 +--- .../launcher-ui/repo-card-view-model.yaml | 17 - .../repo-sync/package-zip-extractor.yaml | 35 +- .../agent-control/utilities/path-helpers.yaml | 46 +-- .../launcher-ui/repo-card-view-model.md | 12 +- .../repo-sync/package-zip-extractor.md | 27 +- .../agent-control/utilities/path-helpers.md | 49 +-- .../LauncherUI/RepoCardViewModel.cs | 47 +-- .../RepoSync/PackageZipExtractor.cs | 192 +--------- .../UnsafeRepositoryStateException.cs | 48 --- .../Utilities/PathHelpers.cs | 138 ------- .../LauncherUI/RepoCardViewModelTests.cs | 85 ----- .../RepoSync/PackageZipExtractorTests.cs | 346 ------------------ .../Utilities/PathHelpersTests.cs | 304 --------------- 20 files changed, 59 insertions(+), 1513 deletions(-) delete mode 100644 src/DemaConsulting.AgentControl/RepoSync/UnsafeRepositoryStateException.cs diff --git a/.cspell.yaml b/.cspell.yaml index 77549b2..bf49414 100644 --- a/.cspell.yaml +++ b/.cspell.yaml @@ -30,14 +30,7 @@ words: - lsfiles - Mvvm - MVVM - - junctioned - - keepme - - mklink - - NTFS - Postconditions - - preflight - - preflighted - - preflights - buildmark - buildtransitive - contentfiles diff --git a/.reviewmark.yaml b/.reviewmark.yaml index ad8b976..f5bc406 100644 --- a/.reviewmark.yaml +++ b/.reviewmark.yaml @@ -265,7 +265,6 @@ reviews: - "docs/design/agent-control/repo-sync/package-zip-extractor.md" - "docs/verification/agent-control/repo-sync/package-zip-extractor.md" - "src/**/RepoSync/PackageZipExtractor.cs" - - "src/**/RepoSync/UnsafeRepositoryStateException.cs" - "test/**/RepoSync/PackageZipExtractorTests.cs" - id: AgentControl-RepoSync-ReleaseNotesViewerViewModel diff --git a/docs/design/agent-control/launcher-ui/repo-card-view-model.md b/docs/design/agent-control/launcher-ui/repo-card-view-model.md index acf5738..40ced94 100644 --- a/docs/design/agent-control/launcher-ui/repo-card-view-model.md +++ b/docs/design/agent-control/launcher-ui/repo-card-view-model.md @@ -68,26 +68,15 @@ refresh. Calls `EnsureAgentFilesSyncedBeforeLaunch` purely for its best-effort sync side effect, then always proceeds to resolve the shell/agent command and spawn the process regardless of that call's outcome — the agent-package sync state (missing pin, missing managed folders, failed -re-extraction) never gates the launch. The one deliberate exception: if -`EnsureAgentFilesSyncedBeforeLaunch` propagates `UnsafeRepositoryStateException` (a managed -folder is reachable only through a reparse point — symlink/junction), `Launch` catches it -specifically, raises `ErrorOccurred`, and returns without spawning the process — a narrow, -documented amendment to the "sync state never blocks launch" rule, justified because this -represents a security concern (launching against content reached through an unexpected link) -rather than a mere missing-content situation -(`AgentControl-RepoCardViewModel-Launch-UnsafeState`). +re-extraction) never gates the launch. **EnsureAgentFilesSyncedBeforeLaunch** (internal): Best-effort attempt to sync the repo's four managed agent folders with the pinned package version before `Launch` spawns the process. This -is an informational side action only for every failure mode except one — it never blocks -`Launch` for an ordinary sync failure, but deliberately lets `UnsafeRepositoryStateException` -propagate uncaught (see `Launch`, above). +is an informational side action only — it never blocks `Launch`. - *Parameters*: None. - *Returns*: `bool` — informational only; `true` if no sync action was needed/attempted or a - sync succeeded, `false` if a sync attempt was made and failed for an ordinary reason. Either - way `Launch` proceeds, *except* when `UnsafeRepositoryStateException` propagates instead of a - `bool` being returned at all. + sync succeeded, `false` if a sync attempt was made and failed. Either way `Launch` proceeds. - *Postconditions*: If `HasCommittedAgentFiles` is `true`, the managed folders are never touched (no delete, no extract) even if a pin exists and folders are missing; a non-blocking `StatusMessage` notes that sync was skipped. Otherwise, if `PinnedPackageName` is `null`, this @@ -96,11 +85,8 @@ propagate uncaught (see `Launch`, above). is already `true`, returns `true` immediately with no extra I/O. Otherwise resolves the *currently pinned* package at the configured source (never the latest) and re-extracts it directly via `PackageZipExtractor.Extract`, without displaying release notes; if this - re-extraction attempt fails for an ordinary reason, `ErrorOccurred` is raised as a non-blocking - warning (`AgentControl-RepoCardViewModel-EnsureSyncedBeforeLaunch`); if it instead fails - because a managed folder is reachable only through a reparse point, the resulting - `UnsafeRepositoryStateException` is deliberately left uncaught rather than absorbed into this - best-effort return value. + re-extraction attempt fails for any reason, `ErrorOccurred` is raised as a non-blocking + warning (`AgentControl-RepoCardViewModel-EnsureSyncedBeforeLaunch`). - Marked `internal` rather than `private` so it can be unit-tested directly, separated from `Launch`'s process-spawning side effect, mirroring `AgentToolLauncher.BuildProcessStartInfo`'s own precedent. @@ -142,17 +128,10 @@ non-blocking idiom already established by `EnsureAgentFilesSyncedBeforeLaunch`. #### Error Handling `Launch` catches `InvalidOperationException`/`ArgumentException` from -`AgentToolLauncher.BuildProcessStartInfo`/`Launch`, and `UnsafeRepositoryStateException` -(specifically, before the general `InvalidOperationException` case would otherwise apply) from -`EnsureAgentFilesSyncedBeforeLaunch`, raising `ErrorOccurred` in every case; only the -`UnsafeRepositoryStateException` case aborts before a process is spawned. +`AgentToolLauncher.BuildProcessStartInfo`/`Launch` and raises `ErrorOccurred`. `EnsureAgentFilesSyncedBeforeLaunch` catches `InvalidOperationException` (extraction failure) and `DirectoryNotFoundException` (unreachable source), raising `ErrorOccurred` as a -non-blocking warning for each rather than propagating or blocking the launch — but re-throws -`UnsafeRepositoryStateException` unchanged (a `catch (UnsafeRepositoryStateException) { throw; }` -clause precedes the general `InvalidOperationException` catch, since it derives from that type -and C# evaluates catch clauses in source order) so `Launch`'s dedicated handling above actually -gets exercised. `Pull` catches +non-blocking warning for each rather than propagating or blocking the launch. `Pull` catches `InvalidOperationException` from `GitClient.Pull` and reports it via `ErrorOccurred`. `RefreshGitStatus`/`RefreshBranchAndCommittedFiles` catch `InvalidOperationException` from `GitClient` and degrade to "unknown"/`false` rather than propagating, since these run as part @@ -171,9 +150,7 @@ throws `ArgumentNullException` for a null - **RepoPinStore** (`RepoConfig` subsystem) — reads/writes the repo's pin. - **PackageSource**, **PackageVersionCache** (`AgentPackageManagement` subsystem) — package discovery and upgrade-availability checks. -- **PackageZipExtractor** (`RepoSync` subsystem) — extracts a package into the repo, and may - throw `UnsafeRepositoryStateException` if a managed folder is reachable only through a - reparse point, which `Launch` catches specifically to abort without spawning a process. +- **PackageZipExtractor** (`RepoSync` subsystem) — extracts a package into the repo. - **GitIgnoreEnsurer** (`RepoSync` subsystem) — proactively ensures the repo's `.gitignore` covers the four managed agent folders after a successful extraction. - **GitClient**, **CommittedAgentFilesCache** (`GitIntegration` subsystem) — git status, diff --git a/docs/design/agent-control/repo-sync.md b/docs/design/agent-control/repo-sync.md index d8be375..f202300 100644 --- a/docs/design/agent-control/repo-sync.md +++ b/docs/design/agent-control/repo-sync.md @@ -25,11 +25,7 @@ zip's contents. and extracts the new files excluding root-level files such as `release-notes.md` (`AgentControl-PackageZipExtractor-Extract`). - *Constraints*: Throws `InvalidOperationException` for a file that is not a valid zip - archive; throws `UnsafeRepositoryStateException` (a dedicated `InvalidOperationException` - subtype) if the repo root, a managed folder, an ancestor of one, or anything nested inside - one is reachable only through a reparse point (symlink/junction), leaving the affected - content untouched (`AgentControl-PackageZipExtractor-RejectsReparsePoints`); per - architecture.md's "blind delete of the four known folders" decision, any local + archive; per architecture.md's "blind delete of the four known folders" decision, any local customizations a developer added inside those folders are silently removed — an accepted risk, not a defect (see architecture.md's Open Concerns #3). @@ -38,9 +34,8 @@ present on disk. - *Type*: In-process .NET static method. - *Role*: Provider. -- *Contract*: Returns `false` if any of the four folders is missing, and also treats a folder - as absent when it is only reachable through a reparse point (repo root, an ancestor, or the - folder itself) (`AgentControl-PackageZipExtractor-AllManagedFoldersExist`); used by +- *Contract*: Returns `false` if any of the four folders is missing + (`AgentControl-PackageZipExtractor-AllManagedFoldersExist`); used by `RepoCardViewModel`'s ensure-synced-before-launch check to decide whether a re-extraction is needed without re-scanning file contents. - *Constraints*: Performs only existence checks, not content verification. @@ -82,11 +77,7 @@ the new files, excluding root-level files like `release-notes.md`. It is a stati no persistent state; the pin-file rewrite and release-notes display happen in the calling `RepoCardViewModel`, deliberately *after* a successful extraction, so a failure partway through extraction never leaves a repo pinned to a version whose files were not actually -applied (`AgentControl-RepoSync-Sync`). `UnsafeRepositoryStateException` is a small, -purpose-built exception type owned by this subsystem with no dedicated unit-level design/ -reqstream/verification docs of its own; it is documented here, folded into -`PackageZipExtractor`'s description, because its only role is signaling the reparse-point -rejection behavior described above. +applied (`AgentControl-RepoSync-Sync`). `ReleaseNotesViewerViewModel` is a simple, stateless-beyond-construction display view model: it is constructed once per shown dialog with the repo name, package title, and release notes diff --git a/docs/design/agent-control/repo-sync/package-zip-extractor.md b/docs/design/agent-control/repo-sync/package-zip-extractor.md index 9d4da29..5626139 100644 --- a/docs/design/agent-control/repo-sync/package-zip-extractor.md +++ b/docs/design/agent-control/repo-sync/package-zip-extractor.md @@ -41,13 +41,9 @@ a repo root. - *Returns*: `bool`. - *Postconditions*: Does not inspect folder contents — a managed folder that exists but is empty (or only partially populated) still counts as "existing" - (`AgentControl-PackageZipExtractor-AllManagedFoldersExist`). A managed folder reached through - a reparse-point (symlink/junction) repo root, ancestor, or the folder itself is treated as - **not** existing, using `PathHelpers.FindReparsePointInAncestry` (the same filesystem-aware - primitive `Extract`'s own symlink guard is built on) — this prevents a caller from trusting - content reached through a link and skipping `Extract`'s own reparse-point protections - entirely. Consulted by `RepoCardViewModel.EnsureAgentFilesSyncedBeforeLaunch` to decide - whether a silent re-extraction is needed before launch. + (`AgentControl-PackageZipExtractor-AllManagedFoldersExist`). Consulted by + `RepoCardViewModel.EnsureAgentFilesSyncedBeforeLaunch` to decide whether a silent + re-extraction is needed before launch. **ReadReleaseNotes**: Reads the content of the package zip's root-level `release-notes.md` entry without extracting it to disk. @@ -60,38 +56,15 @@ entry without extracting it to disk. `Extract` throws `ArgumentNullException` for a null `zipPath`/`repoRoot`, and `InvalidOperationException` when the zip cannot be opened/is not a valid archive, a managed -folder cannot be deleted, extraction fails partway through, or a zip entry would resolve outside -`repoRoot` — wrapping the underlying `IOException`/`UnauthorizedAccessException`/ -`InvalidDataException` with a message naming the zip path and repo root. When the repo root, an -ancestor, a managed folder, or any of its descendants is a symlink/junction, `Extract` instead -throws `UnsafeRepositoryStateException` (a dedicated `InvalidOperationException` subtype) — -this distinguishes a genuine security concern (content reached through an unexpected link) from -an ordinary extraction failure, letting callers that need to react differently (e.g. -`RepoCardViewModel.Launch`, which otherwise never lets sync state block a launch) catch it -specifically, while callers that only care about "did this fail" can still catch the base -`InvalidOperationException` type unchanged. The symlink checks -(`PathHelpers.FindReparsePointInAncestry` via the `EnsureNoSymlinkAncestors` wrapper) walk from -the affected path up to *and including* the repo root itself (not just its ancestors) before -both the blind-delete step and each entry's extraction, using `File.GetAttributes` rather than -`Directory.Exists` so a *dangling* symlink/junction (whose target does not currently exist) is -still detected. The blind-delete step additionally uses `DeleteDirectoryRejectingReparsePoints`, -which preflights the entire managed-folder tree with `PathHelpers.FindReparsePointInDescendants` -*before* deleting anything, then performs a plain recursive delete — this two-phase split avoids -a partial delete: interleaving the reparse-point check with the delete itself would still let an -ordinary sibling file be permanently removed before a reparse point discovered later in the same -tree aborts the operation. `ReadReleaseNotes` throws the same `InvalidOperationException` pattern +folder cannot be deleted, or extraction fails partway through — wrapping the underlying +`IOException`/`UnauthorizedAccessException`/`InvalidDataException` with a message naming the +zip path and repo root. `ReadReleaseNotes` throws the same `InvalidOperationException` pattern for a zip that cannot be opened or whose release-notes entry cannot be read. #### Dependencies - **PathHelpers** (`Utilities` subsystem) — `SafePathCombine` resolves every managed-folder - and zip-entry destination path safely relative to the repo root; `FindReparsePointInAncestry` - and `FindReparsePointInDescendants` detect reparse points that a lexical path check alone - cannot see, underpinning `EnsureNoSymlinkAncestors` and `DeleteDirectoryRejectingReparsePoints` - respectively. -- **UnsafeRepositoryStateException** (`RepoSync` subsystem) — the dedicated exception type - thrown when a reparse point is detected, letting callers distinguish this security concern - from ordinary `InvalidOperationException` failures. + and zip-entry destination path safely relative to the repo root. - **.NET BCL** — `System.IO.Compression.ZipFile`/`ZipArchive`. #### Callers @@ -99,7 +72,4 @@ for a zip that cannot be opened or whose release-notes entry cannot be read. - **RepoCardViewModel** — calls `Extract` (via `ApplyPackageAndShowReleaseNotes`) after a Select-Package or Upgrade action, calls `AllManagedFoldersExist` and, on a miss, `Extract` directly during `EnsureAgentFilesSyncedBeforeLaunch`, and calls `ReadReleaseNotes` to obtain - the text shown via `ReleaseNotesViewerViewModel`. `Launch` catches - `UnsafeRepositoryStateException` specifically (propagated up through - `EnsureAgentFilesSyncedBeforeLaunch`) to abort the launch entirely, rather than treating it as - a best-effort sync failure. + the text shown via `ReleaseNotesViewerViewModel`. diff --git a/docs/design/agent-control/utilities.md b/docs/design/agent-control/utilities.md index 417c9d6..d794062 100644 --- a/docs/design/agent-control/utilities.md +++ b/docs/design/agent-control/utilities.md @@ -7,9 +7,8 @@ The `Utilities` subsystem provides shared utility functions for the Agent Control. It supplies reusable, independently testable helpers consumed by other subsystems. Its primary responsibility is safe file-path manipulation, protecting callers from path-traversal -vulnerabilities when constructing paths from caller-supplied inputs, and detecting filesystem -reparse points (symlinks/junctions) that could otherwise redirect a file operation outside an -intended directory tree. The `Utilities` subsystem contains one unit: `PathHelpers`. +vulnerabilities when constructing paths from caller-supplied inputs. The `Utilities` subsystem +contains one unit: `PathHelpers`. ### Interfaces @@ -25,47 +24,14 @@ that escapes the base directory. when the combined path escapes the base directory; may propagate `NotSupportedException` or `PathTooLongException` from underlying BCL path operations. -**PathHelpers.FindReparsePointInAncestry**: Searches upward from a path to a root (inclusive of -both ends) for the first directory entry that is a reparse point (symlink or junction). - -- *Type*: In-process .NET static method. -- *Role*: Provider. -- *Contract*: Accepts `string root` and `string path`, where `path` is expected to be at or - below `root`. Returns the first reparse-point path found while walking upward from `path` to - `root`, or `null` if none is found. Uses `File.GetAttributes` rather than `Directory.Exists` - so a dangling link (whose target no longer exists) is still detected. -- *Constraints*: Throws `ArgumentNullException` for null inputs; may propagate `IOException` or - `UnauthorizedAccessException` from underlying filesystem access. - -**PathHelpers.FindReparsePointInDescendants**: Recursively searches a directory tree -(inclusive of the directory itself) for the first reparse point. - -- *Type*: In-process .NET static method. -- *Role*: Provider. -- *Contract*: Accepts `string directory`. Returns the first reparse-point path found in the - directory itself or anywhere beneath it, or `null` if none is found. -- *Constraints*: Throws `ArgumentNullException` for null input; may propagate `IOException` or - `UnauthorizedAccessException` from underlying filesystem access. - ### Design The `Utilities` subsystem contains only the `PathHelpers` unit. It has no dependencies on other -tool units or subsystems; it uses only .NET BCL types (`Path`, `File`, `Directory`, -`ArgumentNullException`). +tool units or subsystems; it uses only .NET BCL types (`Path`, `ArgumentNullException`). -`PathHelpers.SafePathCombine` is a pure, lexical utility method: it performs no file-system I/O, -holds no state, and throws immediately on invalid input. All calls to `SafePathCombine` in the +`PathHelpers.SafePathCombine` is a pure utility method: it performs no file-system I/O, holds +no state, and throws immediately on invalid input. All calls to `SafePathCombine` in the codebase originate from the `RepoConfig` subsystem (`RepoPinStore`, resolving the per-repo `.agentcontrol.json` pin file path) and the `RepoSync` subsystem (`PackageZipExtractor`, resolving managed-folder and zip-entry destination paths), each using it to keep a caller-supplied repo root from being escaped by a malformed relative path. - -`FindReparsePointInAncestry` and `FindReparsePointInDescendants` are the filesystem-aware -counterpart to `SafePathCombine`: because a reparse point cannot be detected lexically, these -methods perform real filesystem queries (`File.GetAttributes`) to find one. Both are pure query -functions - they report what they find via a nullable return value and never throw for the -"reparse point found" case, deliberately separating *detection* from the *policy* of what to do -about a finding. The sole caller of both methods is the `RepoSync` subsystem -(`PackageZipExtractor`), which applies that policy by throwing -`UnsafeRepositoryStateException` when either method returns non-null, refusing to extract into -or delete through a path reachable only via a symlink/junction. diff --git a/docs/design/agent-control/utilities/path-helpers.md b/docs/design/agent-control/utilities/path-helpers.md index ddf0e70..26731c6 100644 --- a/docs/design/agent-control/utilities/path-helpers.md +++ b/docs/design/agent-control/utilities/path-helpers.md @@ -4,14 +4,9 @@ #### Purpose -`PathHelpers` is a static utility class providing path-safety primitives with two -complementary halves: `SafePathCombine` performs purely lexical (string-level) containment -validation, while `FindReparsePointInAncestry` and `FindReparsePointInDescendants` perform -filesystem-aware detection of reparse points (symlinks/junctions) that a lexical check alone -cannot see. Callers that assemble a path and then act on the filesystem at it should normally -use both: `SafePathCombine` to reject a textually-escaping path, then one of the reparse-point -finders to reject a link that would otherwise redirect an apparently-safe path outside its -intended root. +`PathHelpers` is a static utility class that provides a safe path-combination method. Its +single responsibility is to combine two path segments while verifying that the result does not +escape the base directory, protecting callers from string-level path-traversal attacks. #### Data Model @@ -42,48 +37,6 @@ paths, platform case-sensitivity, and directory-separator normalization natively check treats a double-dot segment as escaping only when it is the entire relative result or is followed by a directory separator, avoiding false positives for valid names such as `"..data"`. -This method performs no file-system I/O; it is purely lexical and therefore cannot detect a -symlinked/junctioned directory silently redirecting a nominally-contained path outside its -intended root — see `FindReparsePointInAncestry` for that case. - -**FindReparsePointInAncestry**: Searches upward from a path to (and including) a root boundary -for a directory that is itself a reparse point (symlink/junction). - -- *Parameters*: `string root` — the already fully-resolved (`Path.GetFullPath`) boundary at - which the upward walk stops (inclusive); `string path` — the already fully-resolved path - whose ancestry is inspected. -- *Returns*: `string?` — the closest-to-`path` segment that is a reparse point, or `null` if no - segment between `path` and `root` (inclusive of both ends) is one. -- *Preconditions*: Both `root` and `path` are non-null. - -Reparse-point status is queried via `File.GetAttributes` rather than `Directory.Exists`: the -latter resolves (follows) a link to test whether its target exists, and so returns `false` -(silently missing the reparse point) for a *dangling* symlink/junction, whereas -`File.GetAttributes` reports the reparse point's own attributes without requiring its target to -exist. A path segment that does not exist yet (e.g. a destination directory a caller is about -to create) is not itself a finding; the walk simply continues upward past it. This method only -ever *reports* what it found — callers decide what policy to apply (e.g. throwing a -domain-specific exception, or treating a read-only existence check as "not present"). This only -reflects the state of the filesystem at the moment of the call; it does not eliminate a race -where a segment is replaced with a reparse point between this check and a caller's subsequent -file operation. - -**FindReparsePointInDescendants**: Recursively searches a directory (inclusive) for the first -nested directory that is a reparse point (symlink/junction). - -- *Parameters*: `string directory` — the already-existing directory (and its descendants) to - search. -- *Returns*: `string?` — the path of the first reparse point found (`directory` itself, or a - descendant), or `null` if none exists anywhere in the tree. -- *Preconditions*: `directory` is non-null. - -Exists for callers that need to recursively delete, copy, or otherwise walk a directory tree -without following filesystem links nested inside it — unlike `Directory.Delete(path, true)`'s -recursive mode, which follows such links and can affect content outside the tree being -processed. The whole tree is searched up front, rather than interleaving this check with -file-by-file processing, so a caller can preflight an entire operation and fail closed before -acting on any part of the tree if a reparse point exists anywhere within it. - #### Error Handling `SafePathCombine` throws `ArgumentNullException` for null inputs. It throws `ArgumentException` @@ -92,18 +45,10 @@ acting on any part of the tree if a reparse point exists anywhere within it. operations (`Path.Combine`, `Path.GetFullPath`). No logging or error accumulation is performed; callers receive exceptions directly. -`FindReparsePointInAncestry` and `FindReparsePointInDescendants` throw `ArgumentNullException` -for null inputs, and may propagate `IOException`/`UnauthorizedAccessException` from -`File.GetAttributes`/`Directory.GetDirectories` when a path segment cannot be inspected for a -reason other than not existing (e.g. an ACL-restricted directory). Neither method throws a -domain-specific exception itself when a reparse point is found — that is a query result -(`string?`), not a failure; policy for what to do about a found reparse point belongs to the -caller. - #### Dependencies -- **.NET BCL** — `Path`, `File`, `Directory`, `ArgumentNullException`, and related types are the - only dependencies. No other tool units or subsystems are used. +- **.NET BCL** — `Path`, `ArgumentNullException`, and related types are the only dependencies. + No other tool units or subsystems are used. #### Callers @@ -112,9 +57,4 @@ caller. repo directory. - **PackageZipExtractor** — calls `SafePathCombine` to construct managed-folder paths for `AllManagedFoldersExist` and zip-entry destination paths during `Extract`, so a malicious or - malformed zip-entry name cannot write outside the target repo. Calls - `FindReparsePointInAncestry` (via its own `EnsureNoSymlinkAncestors` wrapper, and directly from - `ManagedFolderGenuinelyExists`) to reject a repo root/ancestor/managed-folder that is a - reparse point, and `FindReparsePointInDescendants` (via `DeleteDirectoryRejectingReparsePoints`) - to reject a managed folder whose contents include a nested reparse point before blind-deleting - it. + malformed zip-entry name cannot write outside the target repo. diff --git a/docs/reqstream/agent-control/launcher-ui/repo-card-view-model.yaml b/docs/reqstream/agent-control/launcher-ui/repo-card-view-model.yaml index 8835a4d..3aef231 100644 --- a/docs/reqstream/agent-control/launcher-ui/repo-card-view-model.yaml +++ b/docs/reqstream/agent-control/launcher-ui/repo-card-view-model.yaml @@ -92,23 +92,6 @@ sections: - RepoCardViewModel_EnsureAgentFilesSyncedBeforeLaunch_PinnedVersionMissingFromSource_ReturnsFalse - RepoCardViewModel_LaunchCommand_SyncFailsOrNoPin_StillLaunches - - id: AgentControl-RepoCardViewModel-Launch-UnsafeState - title: >- - The RepoCardViewModel class shall refuse to launch the configured agent tool, - raising an error instead, when the ensure-synced-before-launch sync attempt detects - that a managed agent-file folder is reachable only through a reparse point - (symlink/junction). - justification: | - Unlike an ordinary sync failure (missing pin, unreachable source, an I/O failure), - a managed folder reached only through an unexpected symlink or junction is a genuine - security concern: launching could run the agentic CLI tool against content outside - the repo root. This is a deliberate, narrow exception to the "sync state never - blocks launch" rule established by AgentControl-RepoCardViewModel-EnsureSyncedBeforeLaunch, - justified because it protects the developer from an attacker-controlled or - accidentally-misconfigured filesystem link rather than merely missing content. - tests: - - RepoCardViewModel_LaunchCommand_ManagedFolderAncestorIsJunction_DoesNotLaunch - - id: AgentControl-RepoCardViewModel-Upgrade title: >- The RepoCardViewModel class shall, when a newer package version is available, diff --git a/docs/reqstream/agent-control/repo-sync/package-zip-extractor.yaml b/docs/reqstream/agent-control/repo-sync/package-zip-extractor.yaml index 7f4f776..f1b9efb 100644 --- a/docs/reqstream/agent-control/repo-sync/package-zip-extractor.yaml +++ b/docs/reqstream/agent-control/repo-sync/package-zip-extractor.yaml @@ -22,48 +22,17 @@ sections: - PackageZipExtractor_Extract_ExistingManagedFolder_ReplacesOldContents - PackageZipExtractor_Extract_InvalidZipFile_ThrowsInvalidOperationException - - id: AgentControl-PackageZipExtractor-RejectsReparsePoints - title: >- - The PackageZipExtractor class shall refuse to extract into, or delete through, a - managed folder that is reachable only through a reparse point (symlink/junction) - - whether the repo root itself, an ancestor of a managed folder, the managed folder - itself, or a directory nested inside it - throwing UnsafeRepositoryStateException - and leaving the affected content untouched. - justification: | - Extract's blind-delete-and-replace sequence uses recursive filesystem operations - that would otherwise follow a symlink/junction, letting a crafted or accidentally - misconfigured link redirect a delete or write outside the intended repo root. This - is treated as a distinct security concern (UnsafeRepositoryStateException, a - dedicated InvalidOperationException subtype) rather than an ordinary extraction - failure, so callers that must react differently to it can do so. Detection uses - File.GetAttributes rather than Directory.Exists so a dangling link (whose target no - longer exists) is still caught, and the managed-folder tree is fully preflighted for - nested reparse points before any deletion begins, so a partial delete cannot occur - before the guard fires. - tests: - - windows@PackageZipExtractor_Extract_ManagedFolderAncestorIsJunction_ThrowsAndDoesNotWriteThroughLink - - windows@PackageZipExtractor_Extract_ManagedFolderAncestorIsJunctionWithExistingContent_DoesNotBlindDeleteThroughLink - - windows@PackageZipExtractor_Extract_RepoRootIsJunction_ThrowsAndDoesNotWriteThroughLink - - windows@PackageZipExtractor_Extract_ManagedFolderContainsNestedJunction_ThrowsAndDoesNotDeleteThroughLink - - PackageZipExtractor_Extract_ManagedFolderAncestorIsDanglingLink_ThrowsAndDoesNotBypassGuard - - id: AgentControl-PackageZipExtractor-AllManagedFoldersExist title: >- The PackageZipExtractor class shall report whether all of a repo's managed agent-file - folders are present, returning false if any are missing, and shall also treat a - managed folder as absent when it is only reachable through a reparse point - (symlink/junction) repo root, ancestor, or the folder itself. + folders are present, returning false if any are missing. justification: | Used by the ensure-synced-before-launch check to decide whether a re-extraction is - needed without re-scanning file contents. Trusting a reparse-point-reached folder as - "present" would let this check skip Extract entirely and let a launch proceed using - files outside the repo root, bypassing Extract's own symlink/junction protections. + needed without re-scanning file contents. tests: - PackageZipExtractor_AllManagedFoldersExist_AllFourPresent_ReturnsTrue - PackageZipExtractor_AllManagedFoldersExist_SomeMissing_ReturnsFalse - PackageZipExtractor_AllManagedFoldersExist_NoneExist_ReturnsFalse - - windows@PackageZipExtractor_AllManagedFoldersExist_AncestorIsJunctionWithRealFolders_ReturnsFalse - - windows@PackageZipExtractor_AllManagedFoldersExist_ManagedFolderItselfIsJunction_ReturnsFalse - id: AgentControl-PackageZipExtractor-ReadReleaseNotes title: >- diff --git a/docs/reqstream/agent-control/utilities/path-helpers.yaml b/docs/reqstream/agent-control/utilities/path-helpers.yaml index 0f3a60f..805014c 100644 --- a/docs/reqstream/agent-control/utilities/path-helpers.yaml +++ b/docs/reqstream/agent-control/utilities/path-helpers.yaml @@ -1,11 +1,9 @@ --- # Software Unit Requirements for the PathHelpers Class # -# The PathHelpers class provides path-safety utilities with two complementary halves: purely -# lexical safe path-combination (protecting against path-traversal attacks by rejecting -# relative paths containing ".." or absolute paths when a relative path is expected), and -# filesystem-aware reparse-point (symlink/junction) detection, which a lexical check alone -# cannot see. +# The PathHelpers class provides safe path-combination utilities that protect +# against path-traversal attacks by rejecting relative paths containing ".." +# or absolute paths when a relative path is expected. sections: - title: PathHelpers Unit Requirements @@ -27,41 +25,3 @@ sections: - PathHelpers_SafePathCombine_DotDotPrefixedName_CombinesCorrectly - PathHelpers_SafePathCombine_NullBasePath_ThrowsArgumentNullException - PathHelpers_SafePathCombine_NullRelativePath_ThrowsArgumentNullException - - - id: AgentControl-PathHelpers-FindReparsePointInAncestry - title: >- - The PathHelpers class shall report the closest reparse point (symlink/junction) - between a given path and a root boundary (inclusive of both), or report none if no - such reparse point exists, without following any link target to make that - determination. - justification: | - A purely lexical path-containment check (as performed by SafePathCombine) cannot - detect a symlinked or junctioned ancestor directory silently redirecting a - nominally-contained path outside its intended root. Detecting reparse points via - File.GetAttributes (rather than Directory.Exists, which follows a link to test its - target) also correctly detects a dangling link whose target no longer exists. - tests: - - PathHelpers_FindReparsePointInAncestry_NoReparsePoints_ReturnsNull - - PathHelpers_FindReparsePointInAncestry_AncestorIsJunction_ReturnsJunctionPath - - PathHelpers_FindReparsePointInAncestry_RootIsJunction_ReturnsRoot - - PathHelpers_FindReparsePointInAncestry_AncestorIsDanglingLink_ReturnsLinkPath - - PathHelpers_FindReparsePointInAncestry_NullRoot_ThrowsArgumentNullException - - PathHelpers_FindReparsePointInAncestry_NullPath_ThrowsArgumentNullException - - - id: AgentControl-PathHelpers-FindReparsePointInDescendants - title: >- - The PathHelpers class shall report the first reparse point (symlink/junction) found - anywhere within a directory tree (including the directory itself), or report none if - no such reparse point exists anywhere in the tree. - justification: | - A caller recursively deleting, copying, or otherwise walking a directory tree must - be able to detect a reparse point nested anywhere inside it before acting, since - Directory.Delete's recursive mode (and similar recursive BCL operations) follow such - links and can affect content outside the tree being processed. Searching the whole - tree up front (rather than interleaving detection with per-file processing) lets a - caller fail closed before touching any part of the tree. - tests: - - PathHelpers_FindReparsePointInDescendants_NoReparsePoints_ReturnsNull - - PathHelpers_FindReparsePointInDescendants_NestedJunction_ReturnsJunctionPath - - PathHelpers_FindReparsePointInDescendants_DirectoryItselfIsJunction_ReturnsDirectory - - PathHelpers_FindReparsePointInDescendants_NullDirectory_ThrowsArgumentNullException diff --git a/docs/verification/agent-control/launcher-ui/repo-card-view-model.md b/docs/verification/agent-control/launcher-ui/repo-card-view-model.md index 65a5afa..2a98671 100644 --- a/docs/verification/agent-control/launcher-ui/repo-card-view-model.md +++ b/docs/verification/agent-control/launcher-ui/repo-card-view-model.md @@ -19,9 +19,7 @@ invocations are not parallelized with other tests using real process launches). - All unit tests pass with zero failures. - Display fields, badges, and gating conditions reflect the correct underlying pin/git/source state for every tested input. -- Launch is never blocked by the outcome of the best-effort ensure-synced check, except when - that check detects a managed folder reachable only through a reparse point (symlink/ - junction), which is a deliberate, narrow exception. +- Launch is never blocked by the outcome of the best-effort ensure-synced check. - Upgrade and select-package flows never mutate the pin when their preconditions are not met. - Remove requests never mutate state on their own. @@ -83,14 +81,6 @@ outright. This scenario is tested by and `RepoCardViewModel_LaunchCommand_SyncFailsOrNoPin_StillLaunches`, covering `AgentControl-RepoCardViewModel-EnsureSyncedBeforeLaunch`. -**RepoCardViewModel_Launch_RefusesWhenManagedFolderIsUnsafe**: `LaunchCommand` does not spawn -the agent-tool process, and raises `ErrorOccurred` instead, when the ensure-synced-before-launch -sync attempt detects that a managed agent-file folder is reachable only through a reparse point -(symlink/junction) - the one deliberate exception to the "sync state never blocks launch" -policy verified by the previous scenario. This scenario is tested by -`RepoCardViewModel_LaunchCommand_ManagedFolderAncestorIsJunction_DoesNotLaunch`, covering -`AgentControl-RepoCardViewModel-Launch-UnsafeState`. - **RepoCardViewModel_Upgrade_UpdatesPinOnlyWhenNewerVersionExists**: `UpgradeCommand` updates the pin and raises `ReleaseNotesReady` when a newer version is available, and raises `ErrorOccurred` instead when no package source is configured. This scenario is tested by diff --git a/docs/verification/agent-control/repo-sync/package-zip-extractor.md b/docs/verification/agent-control/repo-sync/package-zip-extractor.md index 58414c4..3445411 100644 --- a/docs/verification/agent-control/repo-sync/package-zip-extractor.md +++ b/docs/verification/agent-control/repo-sync/package-zip-extractor.md @@ -30,14 +30,11 @@ throws `InvalidOperationException`. This scenario is tested by `AgentControl-PackageZipExtractor-Extract`. **PackageZipExtractor_AllManagedFoldersExist_ReflectsActualPresence**: The check returns true -when all four managed folders are present, and false when some or none are present, or when a -managed folder is only reachable through a reparse-point (symlink/junction) repo root, -ancestor, or the folder itself. This scenario is tested by +when all four managed folders are present, and false when some or none are present. This +scenario is tested by `PackageZipExtractor_AllManagedFoldersExist_AllFourPresent_ReturnsTrue`, -`PackageZipExtractor_AllManagedFoldersExist_SomeMissing_ReturnsFalse`, -`PackageZipExtractor_AllManagedFoldersExist_NoneExist_ReturnsFalse`, -`PackageZipExtractor_AllManagedFoldersExist_AncestorIsJunctionWithRealFolders_ReturnsFalse`, and -`PackageZipExtractor_AllManagedFoldersExist_ManagedFolderItselfIsJunction_ReturnsFalse`, covering +`PackageZipExtractor_AllManagedFoldersExist_SomeMissing_ReturnsFalse`, and +`PackageZipExtractor_AllManagedFoldersExist_NoneExist_ReturnsFalse`, covering `AgentControl-PackageZipExtractor-AllManagedFoldersExist`. **PackageZipExtractor_ReadReleaseNotes_ReturnsContentOrNullWithoutExtracting**: Reading @@ -46,19 +43,3 @@ the archive, and reading from a zip with no such entry returns null. This scenar by `PackageZipExtractor_ReadReleaseNotes_EntryPresent_ReturnsContentWithoutExtracting` and `PackageZipExtractor_ReadReleaseNotes_NoEntry_ReturnsNull`, covering `AgentControl-PackageZipExtractor-ReadReleaseNotes`. - -**PackageZipExtractor_Extract_RejectsReparsePointsAtEveryVulnerablePoint**: Extraction refuses -to operate through a reparse point (symlink/junction) wherever one could otherwise let content -be read from or written/deleted outside the repo root: a managed-folder ancestor (e.g. a -linked `.github`), whether empty or already containing real content that must survive; the -repo root itself; a reparse point nested *inside* a managed folder (not just above it); and a -*dangling* link (whose target no longer exists), which a naive `Directory.Exists`-based check -would silently miss. Every case throws `UnsafeRepositoryStateException` (a dedicated -`InvalidOperationException` subtype distinguishing this security concern from an ordinary -extraction failure) and leaves the affected content untouched. This scenario is tested by -`PackageZipExtractor_Extract_ManagedFolderAncestorIsJunction_ThrowsAndDoesNotWriteThroughLink`, -`PackageZipExtractor_Extract_ManagedFolderAncestorIsJunctionWithExistingContent_DoesNotBlindDeleteThroughLink`, -`PackageZipExtractor_Extract_RepoRootIsJunction_ThrowsAndDoesNotWriteThroughLink`, -`PackageZipExtractor_Extract_ManagedFolderContainsNestedJunction_ThrowsAndDoesNotDeleteThroughLink`, -and `PackageZipExtractor_Extract_ManagedFolderAncestorIsDanglingLink_ThrowsAndDoesNotBypassGuard`, -covering `AgentControl-PackageZipExtractor-RejectsReparsePoints`. diff --git a/docs/verification/agent-control/utilities/path-helpers.md b/docs/verification/agent-control/utilities/path-helpers.md index 7928776..1a5360b 100644 --- a/docs/verification/agent-control/utilities/path-helpers.md +++ b/docs/verification/agent-control/utilities/path-helpers.md @@ -2,15 +2,10 @@ #### Verification Approach -`PathHelpers` is verified with unit tests defined in `PathHelpersTests.cs`. The lexical -`SafePathCombine` method is verified using only .NET BCL types, with no mocking or test doubles -required - tests call it directly with controlled base and relative path arguments and assert -on the returned string or the thrown exception type and message. The filesystem-aware -`FindReparsePointInAncestry` and `FindReparsePointInDescendants` methods are verified against -real temporary directories, using real NTFS junctions on Windows (created via `mklink /J`, -which - unlike symbolic links - require neither elevated privileges nor Developer Mode) or real -directory symbolic links on Linux/macOS (which do not require an existing target at creation -time, letting a dangling-link scenario be constructed directly). +`PathHelpers` is verified with unit tests defined in `PathHelpersTests.cs`. Because `PathHelpers` +performs pure path manipulation using only .NET BCL types, no mocking or test doubles are +required. Tests call `PathHelpers.SafePathCombine` directly with controlled base and relative +path arguments and assert on the returned string or the thrown exception type and message. #### Test Environment @@ -25,10 +20,6 @@ N/A - standard test environment. - Absolute paths supplied as the relative argument cause `ArgumentException`. - Null inputs cause `ArgumentNullException`. - A filename beginning with `".."` that is not a traversal sequence is accepted correctly. -- `FindReparsePointInAncestry` and `FindReparsePointInDescendants` return `null` when no - reparse point exists, and the offending path when one does - including when the root/ - directory itself is the reparse point, and including a dangling link whose target does not - currently exist. #### Test Scenarios @@ -83,35 +74,3 @@ the `basePath` argument; an `ArgumentNullException` is thrown, confirming the nu as the `relativePath` argument; an `ArgumentNullException` is thrown, confirming the null guard on `relativePath`. This scenario is tested by `PathHelpers_SafePathCombine_NullRelativePath_ThrowsArgumentNullException`. - -**PathHelpers_FindReparsePointInAncestry_ReportsClosestLinkOrNone**: An ordinary nested -directory tree with no links reports `null`; a junction/symbolic-link ancestor between the -root and the checked path reports that link's own path; a root that is itself a junction -reports the root; and a dangling junction/symbolic-link ancestor (whose target no longer -exists) is still reported, confirming detection uses `File.GetAttributes` rather than -`Directory.Exists`. This scenario is tested by -`PathHelpers_FindReparsePointInAncestry_NoReparsePoints_ReturnsNull`, -`PathHelpers_FindReparsePointInAncestry_AncestorIsJunction_ReturnsJunctionPath`, -`PathHelpers_FindReparsePointInAncestry_RootIsJunction_ReturnsRoot`, and -`PathHelpers_FindReparsePointInAncestry_AncestorIsDanglingLink_ReturnsLinkPath`, covering -`AgentControl-PathHelpers-FindReparsePointInAncestry`. - -**PathHelpers_FindReparsePointInAncestry_NullArguments_ThrowArgumentNullException**: A `null` -`root` or `null` `path` argument each throw `ArgumentNullException`. This scenario is tested by -`PathHelpers_FindReparsePointInAncestry_NullRoot_ThrowsArgumentNullException` and -`PathHelpers_FindReparsePointInAncestry_NullPath_ThrowsArgumentNullException`, covering -`AgentControl-PathHelpers-FindReparsePointInAncestry`. - -**PathHelpers_FindReparsePointInDescendants_ReportsNestedOrSelfLinkOrNone**: An ordinary nested -directory tree with no links reports `null`; a junction nested two levels deep anywhere in the -tree is found and its path reported; and a directory that is itself a junction reports that -same directory. This scenario is tested by -`PathHelpers_FindReparsePointInDescendants_NoReparsePoints_ReturnsNull`, -`PathHelpers_FindReparsePointInDescendants_NestedJunction_ReturnsJunctionPath`, and -`PathHelpers_FindReparsePointInDescendants_DirectoryItselfIsJunction_ReturnsDirectory`, covering -`AgentControl-PathHelpers-FindReparsePointInDescendants`. - -**PathHelpers_FindReparsePointInDescendants_NullDirectory_ThrowsArgumentNullException**: A -`null` `directory` argument throws `ArgumentNullException`. This scenario is tested by -`PathHelpers_FindReparsePointInDescendants_NullDirectory_ThrowsArgumentNullException`, covering -`AgentControl-PathHelpers-FindReparsePointInDescendants`. diff --git a/src/DemaConsulting.AgentControl/LauncherUI/RepoCardViewModel.cs b/src/DemaConsulting.AgentControl/LauncherUI/RepoCardViewModel.cs index 2c23b30..139d5bf 100644 --- a/src/DemaConsulting.AgentControl/LauncherUI/RepoCardViewModel.cs +++ b/src/DemaConsulting.AgentControl/LauncherUI/RepoCardViewModel.cs @@ -546,26 +546,11 @@ private void RecordLaunched() /// command and spawn the process regardless of that call's outcome. A user may want to /// launch their agentic CLI tool to help with agent-package migration, or simply because /// an agentic tool is useful even with zero agent files present - either way, sync state - /// must never stand in the way of launching. The sole exception is - /// , which - /// deliberately does not catch: unlike an - /// ordinary sync failure (missing source, missing pinned version, a locked file), it means - /// a managed folder is reachable only through a reparse point (symlink/junction), so - /// launching could run the agentic tool against content outside the repo root. That - /// specific failure mode blocks the launch instead - is still - /// raised explaining why, but no process is spawned. + /// must never stand in the way of launching. /// private void Launch() { - try - { - EnsureAgentFilesSyncedBeforeLaunch(); - } - catch (UnsafeRepositoryStateException ex) - { - ErrorOccurred?.Invoke(this, $"Refusing to launch: {ex.Message}"); - return; - } + EnsureAgentFilesSyncedBeforeLaunch(); try { @@ -606,14 +591,6 @@ private void Launch() /// raised as a non-blocking warning explaining why and proceeds to /// spawn the agent tool regardless of this return value. /// - /// - /// Thrown (not caught here) when detects that a - /// managed folder is reachable only through a reparse point (symlink/junction). Unlike - /// every other failure this method absorbs, this represents a genuine security concern - - /// launching could run the agentic tool against content outside the repo root - so - /// deliberately does not proceed when this propagates, breaking the - /// "sync state never blocks launch" policy described above for this one case only. - /// /// /// /// If this repo has committed agent files ( is @@ -634,9 +611,7 @@ private void Launch() /// If a pin exists and is /// already , this returns immediately with no /// re-extraction - the common "already synced" case must not pay any extra I/O cost on - /// every launch. itself treats a - /// folder reached through a reparse point as not present, so this case only applies to - /// genuinely present, unlinked managed folders. + /// every launch. /// /// /// If a pin exists but one or more managed folders are missing (e.g. a freshly cloned @@ -647,13 +622,9 @@ private void Launch() /// through - architecture.md's /// ensure-synced-before-launch bullet never mentions showing release notes, unlike its /// Select-Package bullet, so a silent background repair must not pop a release-notes - /// window on every launch. If this re-extraction attempt fails for an ordinary reason - /// (source unreachable, pinned version missing, extraction I/O failure), - /// is raised as a non-blocking warning and this returns - /// - but the launch still proceeds regardless. If it instead fails - /// because a managed folder is reachable only through a reparse point, the resulting - /// is deliberately left uncaught (see the - /// exception list above) rather than absorbed into this best-effort return value. + /// window on every launch. If this re-extraction attempt fails, + /// is raised as a non-blocking warning and this returns - but the + /// launch still proceeds regardless. /// /// /// Marked (not ) rather than tested @@ -703,12 +674,6 @@ internal bool EnsureAgentFilesSyncedBeforeLaunch() PackageZipExtractor.Extract(pinnedPackage.FilePath, RepoPath); return true; } - catch (UnsafeRepositoryStateException) - { - // Deliberately not absorbed into the best-effort false/ErrorOccurred pattern below - - // Launch must treat this as a hard stop, not a "launching anyway" warning. - throw; - } catch (InvalidOperationException ex) { ErrorOccurred?.Invoke(this, $"Failed to sync agent files: {ex.Message} Launching anyway."); diff --git a/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs b/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs index 755ccc6..e0a6815 100644 --- a/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs +++ b/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs @@ -78,13 +78,6 @@ internal static class PackageZipExtractor /// there is no rollback: a partially-applied change is possible and must be resolved /// manually by the caller. /// - /// - /// Thrown (derives from ) when the repo root, a - /// managed folder's ancestor, the folder itself, or any descendant of it is a reparse - /// point (symlink/junction) - callers that must react differently to this specific - /// security concern (rather than an ordinary extraction failure) can catch it before the - /// general case. - /// public static void Extract(string zipPath, string repoRoot) { ArgumentNullException.ThrowIfNull(zipPath); @@ -127,12 +120,7 @@ public static void Extract(string zipPath, string repoRoot) /// so the managed-folder list has a single source of truth - callers (e.g. /// RepoCardViewModel's ensure-synced-before-launch check) never need to duplicate /// it. Does not inspect folder contents; a managed folder that exists but is empty (or - /// only partially populated) still counts as "existing" here. A managed folder that is - /// only reachable through a reparse-point (symlink/junction) repo root or ancestor - e.g. - /// a symlinked .github - is deliberately treated as not existing: trusting - /// it here would let a caller (such as RepoCardViewModel.EnsureAgentFilesSyncedBeforeLaunch) - /// skip entirely and launch using files outside - /// without any of 's reparse-point protections ever running. + /// only partially populated) still counts as "existing" here. /// /// Thrown when is /// . @@ -140,52 +128,7 @@ public static bool AllManagedFoldersExist(string repoRoot) { ArgumentNullException.ThrowIfNull(repoRoot); - var normalizedRoot = Path.GetFullPath(repoRoot); - return ManagedFolders.All(folder => ManagedFolderGenuinelyExists(normalizedRoot, folder)); - } - - /// - /// Determines whether a single managed folder exists under a repo root without being - /// reached through a reparse point (symlink/junction) repo root, ancestor, or the folder - /// itself. - /// - /// The already-resolved () - /// repo root. - /// The managed folder's path relative to the repo root. - /// if the folder exists and no reparse point sits between it - /// and (inclusive); otherwise . - /// - /// Deliberately swallows (as "not genuinely present") both a found reparse point and any - /// I/O failure while inspecting the ancestor chain - e.g. an - /// from an ACL-restricted ancestor, which - /// does not itself catch. This keeps - /// 's contract to only ever throw - /// (its callers, e.g. - /// RepoCardViewModel.EnsureAgentFilesSyncedBeforeLaunch, only guard against - /// 's own documented exceptions and do not expect this read-only - /// check to throw anything else). - /// - private static bool ManagedFolderGenuinelyExists(string normalizedRoot, string relativeFolder) - { - var folderPath = PathHelpers.SafePathCombine(normalizedRoot, relativeFolder); - if (!Directory.Exists(folderPath)) - { - return false; - } - - try - { - return PathHelpers.FindReparsePointInAncestry(normalizedRoot, folderPath) is null; - } - catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) - { - // An inability to even inspect an ancestor (e.g. an ACL-restricted directory) means - // this folder cannot be confirmed as genuinely present under normalizedRoot; treat it - // the same as missing so callers fall back to Extract, which will itself surface the - // underlying failure as a hard error instead of silently trusting - or crashing on - - // content reached through a symlink/junction. - return false; - } + return ManagedFolders.All(folder => Directory.Exists(PathHelpers.SafePathCombine(repoRoot, folder))); } /// @@ -252,97 +195,23 @@ private static ZipArchive OpenArchive(string zipPath) /// The managed folder's path relative to the repo root. /// Thrown when the folder exists but cannot be /// deleted. - /// Thrown when the repo root, an ancestor, - /// the folder itself, or any descendant of the folder is a reparse point - /// (symlink/junction). private static void DeleteManagedFolder(string repoRoot, string relativeFolder) { var folderPath = PathHelpers.SafePathCombine(repoRoot, relativeFolder); - try - { - // Reject a reparse-point repo root/ancestor (e.g. a symlinked/junctioned '.github') - // before the blind delete below: Directory.Delete(recursive: true) follows filesystem - // links, so without this guard a crafted/pre-existing junction could cause content - // outside repoRoot to be deleted before extraction's own EnsureNoSymlinkAncestors check - // is ever reached. Wrapped alongside the delete itself so an UnauthorizedAccessException - // from an ACL-restricted ancestor surfaces as the same documented InvalidOperationException, - // consistent with ExtractEntryIfManaged's equivalent call (protected by Extract's step-3 - // try/catch). - EnsureNoSymlinkAncestors(Path.GetFullPath(repoRoot), folderPath, relativeFolder); - - if (!Directory.Exists(folderPath)) - { - return; - } - - // A plain Directory.Delete(folderPath, recursive: true) would also follow any - // reparse point nested *inside* the managed folder (not just its ancestors), - // potentially deleting content outside repoRoot. DeleteDirectoryRejectingReparsePoints - // walks the tree itself and fails closed the moment it finds one. - DeleteDirectoryRejectingReparsePoints(folderPath, relativeFolder); - } - catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) - { - throw new InvalidOperationException($"Failed to delete folder '{folderPath}': {ex.Message}", ex); - } - } - - /// - /// Recursively deletes , rejecting the delete if it or any - /// descendant directory is itself a reparse point (symlink/junction) - unlike - /// 's recursive mode, which follows such links - /// and can delete content outside the directory being cleaned up. - /// - /// The directory to delete. - /// A short description of the folder being deleted, for the exception - /// message. - /// - /// The entire tree is preflighted for reparse points () - /// before anything is deleted. Interleaving the reparse-point check with the actual - /// delete (checking each directory immediately before deleting its files) would still let - /// an ordinary sibling file be permanently deleted before a reparse point discovered - /// later in the same tree aborts the operation, leaving the managed folder partially - /// destroyed instead of untouched. - /// - /// Thrown when a nested reparse point is - /// encountered anywhere in the tree; nothing is deleted in this case. - private static void DeleteDirectoryRejectingReparsePoints(string directoryPath, string context) - { - var reparsePoint = PathHelpers.FindReparsePointInDescendants(directoryPath); - if (reparsePoint is not null) + if (!Directory.Exists(folderPath)) { - throw new UnsafeRepositoryStateException( - $"'{context}' contains a symlinked directory '{reparsePoint}'; refusing to delete through it."); + return; } - DeleteDirectoryTree(directoryPath); - } - - /// - /// Recursively deletes every file and subdirectory under , - /// then the now-empty directory itself. - /// - /// The directory to delete. - /// - /// Assumes has already verified the - /// whole tree contains no reparse points; this method performs no such check itself, since - /// re-checking here would re-introduce the same interleaved check-then-delete race the - /// two-phase split in exists to avoid. - /// - private static void DeleteDirectoryTree(string directoryPath) - { - foreach (var filePath in Directory.GetFiles(directoryPath)) + try { - File.Delete(filePath); + Directory.Delete(folderPath, recursive: true); } - - foreach (var subdirectoryPath in Directory.GetDirectories(directoryPath)) + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { - DeleteDirectoryTree(subdirectoryPath); + throw new InvalidOperationException($"Failed to delete folder '{folderPath}': {ex.Message}", ex); } - - Directory.Delete(directoryPath, recursive: false); } /// @@ -354,8 +223,6 @@ private static void DeleteDirectoryTree(string directoryPath) /// Absolute path to the repository root. /// Thrown when the entry's path is invalid, /// including resolving outside . - /// Thrown when the repo root, an ancestor, or - /// the destination directory itself is a reparse point (symlink/junction). private static void ExtractEntryIfManaged(ZipArchiveEntry entry, string repoRoot) { // Directory entries have an empty Name (only FullName ends with '/'); skip them, as @@ -398,55 +265,12 @@ private static void ExtractEntryIfManaged(ZipArchiveEntry entry, string repoRoot var destinationDirectory = Path.GetDirectoryName(destinationPath); if (!string.IsNullOrEmpty(destinationDirectory)) { - // Path.GetFullPath (used internally by SafePathCombine) performs lexical - // normalization only - it does not resolve filesystem links - so a symlinked - // ancestor directory could otherwise still cause extraction to escape the repo root - // despite the containment check above passing. Reject any ancestor between the repo - // root and the destination that is itself a reparse point (symlink/junction) before - // creating anything. - EnsureNoSymlinkAncestors(Path.GetFullPath(repoRoot), destinationDirectory, entry.FullName); - Directory.CreateDirectory(destinationDirectory); } entry.ExtractToFile(destinationPath, overwrite: true); } - /// - /// Rejects the operation if or any path segment between it and - /// (inclusive of both ends) is itself a reparse point - /// (symlink/junction), throwing a domain-specific - /// with a message naming and the offending path. - /// - /// The already-resolved () repo - /// root; also checked, since a symlinked/junctioned repo root would otherwise let every - /// managed-folder operation write through it undetected. - /// The path whose ancestry is being validated - either a zip entry's - /// destination directory (before extraction) or a managed folder about to be blind-deleted. - /// A short description of the path/entry, for the exception message. - /// - /// A thin, exception-throwing policy wrapper around - /// , which owns the actual - /// filesystem-aware detection logic (see its own doc remarks for why a lexical-only check - /// is insufficient and why is used over - /// ). This only guards against paths that already - /// exist at the time of the check; it does not eliminate a race where a path is replaced - /// with a symlink between this check and - /// // - /// . - /// - /// Thrown when a path in the walk is a - /// reparse point. - private static void EnsureNoSymlinkAncestors(string repoRoot, string path, string context) - { - var reparsePoint = PathHelpers.FindReparsePointInAncestry(repoRoot, path); - if (reparsePoint is not null) - { - throw new UnsafeRepositoryStateException( - $"'{context}' resolves through a symlinked directory '{reparsePoint}'."); - } - } - /// /// Determines whether a zip-relative path falls within one of the four managed folders. /// diff --git a/src/DemaConsulting.AgentControl/RepoSync/UnsafeRepositoryStateException.cs b/src/DemaConsulting.AgentControl/RepoSync/UnsafeRepositoryStateException.cs deleted file mode 100644 index 481145a..0000000 --- a/src/DemaConsulting.AgentControl/RepoSync/UnsafeRepositoryStateException.cs +++ /dev/null @@ -1,48 +0,0 @@ -// Copyright (c) DEMA Consulting -// -// Permission is hereby granted, free of charge, to any person obtaining a copy -// of this software and associated documentation files (the "Software"), to deal -// in the Software without restriction, including without limitation the rights -// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell -// copies of the Software, and to permit persons to whom the Software is -// furnished to do so, subject to the following conditions: -// -// The above copyright notice and this permission notice shall be included in all -// copies or substantial portions of the Software. -// -// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE -// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE -// SOFTWARE. - -namespace DemaConsulting.AgentControl.RepoSync; - -/// -/// Thrown by when a reparse point (symlink/junction) is -/// found somewhere it would let a blind-delete-and-replace operation read from or write to a -/// location outside the intended repository directory. -/// -/// -/// Deliberately distinct from the plain thrown for -/// ordinary extraction failures (a corrupt zip, a locked file, an unreachable package -/// source), even though it derives from it for backward-compatible catch-by-base-type -/// behavior. Callers that need to react differently to a genuine security concern - e.g. -/// RepoCardViewModel.EnsureAgentFilesSyncedBeforeLaunch, which must not let its -/// "launch proceeds regardless of sync failure" best-effort policy also apply to a detected -/// symlink/junction attack - can catch this type specifically before the general -/// case. -/// -internal sealed class UnsafeRepositoryStateException : InvalidOperationException -{ - /// - /// Initializes a new instance of the class. - /// - /// A message describing the unsafe reparse point that was detected. - public UnsafeRepositoryStateException(string message) - : base(message) - { - } -} diff --git a/src/DemaConsulting.AgentControl/Utilities/PathHelpers.cs b/src/DemaConsulting.AgentControl/Utilities/PathHelpers.cs index e86a1d8..9197d55 100644 --- a/src/DemaConsulting.AgentControl/Utilities/PathHelpers.cs +++ b/src/DemaConsulting.AgentControl/Utilities/PathHelpers.cs @@ -23,16 +23,6 @@ namespace DemaConsulting.AgentControl.Utilities; /// /// Helper utilities for safe path operations. /// -/// -/// Combines two complementary halves of path safety: is purely -/// lexical (string-level containment, no file-system I/O), while -/// / are -/// filesystem-aware (detecting symlinks/junctions that a lexical check alone cannot see). -/// Callers that assemble a path and then touch the filesystem at it should normally use both: -/// first to reject a textually-escaping path, then one of the -/// reparse-point finders to reject a link that would otherwise redirect an apparently-safe -/// path outside its intended root. -/// internal static class PathHelpers { /// @@ -77,132 +67,4 @@ internal static string SafePathCombine(string basePath, string relativePath) return combinedPath; } - - /// - /// Searches upward from to (and including) , - /// looking for a directory that is itself a reparse point (symlink/junction). - /// - /// The already fully-resolved () - /// boundary at which the upward walk stops; also checked, since a symlinked/junctioned - /// root would otherwise let every operation beneath it silently write through the link. - /// The already fully-resolved path whose ancestry (up to and including - /// ) is inspected. - /// - /// The closest-to- segment that is a reparse point, or - /// if no segment between and - /// (inclusive of both ends) is one. - /// - /// - /// This is the filesystem-aware counterpart to : that method - /// is purely lexical and never touches the filesystem, so it cannot detect a symlinked/ - /// junctioned ancestor silently redirecting a nominally-contained path outside its - /// intended root. Reparse-point status is queried via - /// rather than : - /// the latter resolves (follows) a link to test whether its target exists, and so returns - /// (silently missing the reparse point) for a *dangling* - /// symlink/junction, whereas reports the reparse - /// point's own attributes without requiring its target to exist. A path segment that does - /// not exist yet (e.g. a destination directory a caller is about to create) is not itself - /// a finding; the walk simply continues upward past it. Callers that need to react to a - /// found reparse point (e.g. by throwing, or by treating the path as unusable) decide that - /// policy themselves - this method only ever reports what it found. This only reflects the - /// state of the filesystem at the moment of the call; it does not eliminate a race where a - /// segment is replaced with a reparse point between this check and a caller's subsequent - /// file operation. Stateless and thread-safe, but - unlike - - /// performs real file-system I/O. - /// - /// Thrown when or - /// is . - /// Thrown when a path segment's attributes cannot be read for a - /// reason other than the segment not existing. - /// Thrown when the caller lacks permission to - /// read a path segment's attributes. - internal static string? FindReparsePointInAncestry(string root, string path) - { - ArgumentNullException.ThrowIfNull(root); - ArgumentNullException.ThrowIfNull(path); - - var current = Path.TrimEndingDirectorySeparator(path); - var normalizedRoot = Path.TrimEndingDirectorySeparator(root); - - while (!string.IsNullOrEmpty(current)) - { - FileAttributes attributes; - try - { - attributes = File.GetAttributes(current); - } - catch (Exception ex) when (ex is FileNotFoundException or DirectoryNotFoundException) - { - if (string.Equals(current, normalizedRoot, StringComparison.OrdinalIgnoreCase)) - { - break; - } - - current = Path.GetDirectoryName(current); - continue; - } - - if (attributes.HasFlag(FileAttributes.ReparsePoint)) - { - return current; - } - - if (string.Equals(current, normalizedRoot, StringComparison.OrdinalIgnoreCase)) - { - break; - } - - current = Path.GetDirectoryName(current); - } - - return null; - } - - /// - /// Recursively searches (inclusive) for the first nested - /// directory that is a reparse point (symlink/junction). - /// - /// The already-existing directory (and its descendants) to search. - /// - /// The path of the first reparse point found ( itself, or a - /// descendant), or if none exists anywhere in the tree. - /// - /// - /// Exists for callers that need to recursively delete, copy, or otherwise walk a directory - /// tree without following filesystem links nested inside it - unlike - /// 's recursive mode, which follows such links - /// and can affect content outside the tree being processed. The whole tree is searched up - /// front, rather than interleaving this check with file-by-file processing, so a caller - /// can preflight an entire operation and fail closed before acting on (e.g. deleting) any - /// part of the tree if a reparse point exists anywhere within it. As with - /// , this only reflects a single point in time and - /// performs real file-system I/O. - /// - /// Thrown when is - /// . - /// Thrown when a directory's attributes or contents cannot be - /// read for a reason other than a permission failure. - /// Thrown when the caller lacks permission to - /// read a directory's attributes or contents. - internal static string? FindReparsePointInDescendants(string directory) - { - ArgumentNullException.ThrowIfNull(directory); - - if (File.GetAttributes(directory).HasFlag(FileAttributes.ReparsePoint)) - { - return directory; - } - - foreach (var subdirectory in Directory.GetDirectories(directory)) - { - var found = FindReparsePointInDescendants(subdirectory); - if (found is not null) - { - return found; - } - } - - return null; - } } diff --git a/test/DemaConsulting.AgentControl.Tests/LauncherUI/RepoCardViewModelTests.cs b/test/DemaConsulting.AgentControl.Tests/LauncherUI/RepoCardViewModelTests.cs index 27126bf..a33ffb7 100644 --- a/test/DemaConsulting.AgentControl.Tests/LauncherUI/RepoCardViewModelTests.cs +++ b/test/DemaConsulting.AgentControl.Tests/LauncherUI/RepoCardViewModelTests.cs @@ -974,91 +974,6 @@ public void RepoCardViewModel_LaunchCommand_SyncFailsOrNoPin_StillLaunches(bool Assert.NotNull(card.LastLaunchedUtc); } - /// - /// Test that LaunchCommand does not spawn the agent-tool process, and raises - /// instead, when the ensure-synced-before-launch - /// check detects that a managed folder is only reachable through a reparse point - /// (symlink/junction) - the one deliberate, narrow exception to the "sync state never - /// blocks launch" policy exercised by . - /// - [Fact] - public void RepoCardViewModel_LaunchCommand_ManagedFolderAncestorIsJunction_DoesNotLaunch() - { - // Arrange: a repo whose '.github' folder is a junction, so AllManagedFoldersExist treats - // the managed folders as missing and EnsureAgentFilesSyncedBeforeLaunch attempts to - // re-extract into it - which PackageZipExtractor refuses, since deleting/writing through - // the junction could affect content outside repoRoot. - var repoRoot = CreateTempDirectory(); - var linkTarget = CreateTempDirectory(); - var sourceDir = CreateTempDirectory(); - CreatePackageZip(sourceDir, "contoso-agents", "1.0.0"); - CreateJunction(Path.Combine(repoRoot, ".github"), linkTarget); - var settings = new AppSettings - { - AgentTool = AgentToolKind.Custom, - CustomAgentCommand = OperatingSystem.IsWindows() ? "cmd /c exit 0" : "true", - PackageSourcePath = sourceDir - }; - var card = CreateCard(repoRoot, "contoso-agents", "1.0.0", settings); - var raised = false; - card.LaunchRecorded += (_, _) => raised = true; - string? capturedError = null; - card.ErrorOccurred += (_, message) => capturedError = message; - - try - { - // Act - card.LaunchCommand.Execute(null); - - // Assert: no process was spawned, and the error explains why - Assert.False(raised); - Assert.Null(card.LastLaunchedUtc); - Assert.NotNull(capturedError); - Assert.Contains("Refusing to launch", capturedError, StringComparison.Ordinal); - } - finally - { - // Remove the junction entry itself (not its target's contents) before the temp - // directories tracked in _tempPaths are recursively deleted in Dispose - avoids - // Directory.Delete(recursive: true) following the still-live link during cleanup. - Directory.Delete(Path.Combine(repoRoot, ".github"), recursive: false); - } - } - - /// - /// Creates an NTFS directory junction at pointing to - /// (Windows), or a real directory symbolic link - /// (Linux/macOS) - both are reparse points for the purposes of this test, and symbolic - /// links require neither an existing target nor elevated privileges on non-Windows - /// platforms. - /// - private static void CreateJunction(string linkPath, string targetPath) - { - if (OperatingSystem.IsWindows()) - { - var startInfo = new System.Diagnostics.ProcessStartInfo("cmd.exe", $"/c mklink /J \"{linkPath}\" \"{targetPath}\"") - { - RedirectStandardOutput = true, - RedirectStandardError = true, - UseShellExecute = false, - CreateNoWindow = true - }; - - using var process = System.Diagnostics.Process.Start(startInfo) - ?? throw new InvalidOperationException("Failed to start 'cmd.exe' to create junction."); - process.WaitForExit(); - if (process.ExitCode != 0) - { - throw new InvalidOperationException( - $"Failed to create junction '{linkPath}' -> '{targetPath}': {process.StandardError.ReadToEnd()}"); - } - } - else - { - Directory.CreateSymbolicLink(linkPath, targetPath); - } - } - /// /// Creates a for the given repo path, cached pin fields, /// and settings snapshot. When a package name is supplied, the corresponding pin file is diff --git a/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs b/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs index 9069af5..0eae1fd 100644 --- a/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs +++ b/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs @@ -18,7 +18,6 @@ // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE // SOFTWARE. -using System.Diagnostics; using System.IO.Compression; using DemaConsulting.AgentControl.RepoSync; @@ -225,43 +224,6 @@ public void PackageZipExtractor_AllManagedFoldersExist_NoneExist_ReturnsFalse() } } - /// - /// Test that AllManagedFoldersExist returns false when a managed folder itself (not an - /// ancestor such as .github) is a junction, distinguishing this case from - /// . - /// - [Fact] - public void PackageZipExtractor_AllManagedFoldersExist_ManagedFolderItselfIsJunction_ReturnsFalse() - { - // NTFS directory junctions are a Windows-only concept; skip on other CI runners. - if (!OperatingSystem.IsWindows()) - { - Assert.Skip("Directory junctions are a Windows-only filesystem feature."); - } - - // Arrange: a repo root with three genuine managed folders, and a fourth - // (".github/agents") that is itself a junction to a separate, isolated directory with - // real content - so the naive Directory.Exists-based check alone would (incorrectly) - // report it as present. - var repoRoot = CreateTempDirectory(); - var linkTarget = CreateTempDirectory(); - Directory.CreateDirectory(Path.Combine(repoRoot, ".github", "standards")); - Directory.CreateDirectory(Path.Combine(repoRoot, ".github", "templates")); - Directory.CreateDirectory(Path.Combine(repoRoot, ".github", "skills")); - try - { - CreateJunction(Path.Combine(repoRoot, ".github", "agents"), linkTarget); - - Assert.False(PackageZipExtractor.AllManagedFoldersExist(repoRoot)); - } - finally - { - Directory.Delete(Path.Combine(repoRoot, ".github", "agents")); - Directory.Delete(repoRoot, recursive: true); - Directory.Delete(linkTarget, recursive: true); - } - } - /// /// Test that a zip entry whose name textually starts with a managed-folder prefix but /// uses ".." components to resolve to a location outside every managed folder (while @@ -298,314 +260,6 @@ public void PackageZipExtractor_Extract_TraversalEntryWithinRepoRoot_DoesNotEsca } } - /// - /// Test that extracting into a repo whose .github folder is a junction (reparse - /// point) pointing outside the repo root is refused, rather than silently writing through - /// the junction to the linked-to location. - /// - [Fact] - public void PackageZipExtractor_Extract_ManagedFolderAncestorIsJunction_ThrowsAndDoesNotWriteThroughLink() - { - // NTFS directory junctions (and the 'mklink /J' tool used to create them) are a - // Windows-only concept; this test project also runs on Linux/macOS CI runners, so skip - // there rather than shelling out to a nonexistent 'cmd.exe'. - if (!OperatingSystem.IsWindows()) - { - Assert.Skip("Directory junctions are a Windows-only filesystem feature."); - } - - // Arrange: a repo root whose ".github" entry is a junction to a separate, isolated - // directory standing in for a location outside the repo. The link target starts empty so - // this test exercises the extraction-step guard specifically (DeleteManagedFolder is a - // no-op here since no managed folder exists through the junction yet). - var zipPath = CreatePackageZip(("agents/copilot.md", "should not be extracted")); - var repoRoot = CreateTempDirectory(); - var linkTarget = CreateTempDirectory(); - try - { - CreateJunction(Path.Combine(repoRoot, ".github"), linkTarget); - - // Act / Assert: extraction is refused rather than writing through the junction - Assert.Throws(() => PackageZipExtractor.Extract(zipPath, repoRoot)); - Assert.False(File.Exists(Path.Combine(linkTarget, "agents", "copilot.md"))); - } - finally - { - File.Delete(zipPath); - // The ".github" junction entry itself must be removed (not recursively, since that - // would delete the link target's contents) before the repo root can be deleted. - Directory.Delete(Path.Combine(repoRoot, ".github")); - Directory.Delete(repoRoot, recursive: true); - Directory.Delete(linkTarget, recursive: true); - } - } - - /// - /// Test that the blind-delete step itself refuses to recurse through a junctioned - /// .github ancestor, so pre-existing content at the link's target survives even - /// though it happens to be reachable at a path that lexically looks like a managed folder. - /// - [Fact] - public void PackageZipExtractor_Extract_ManagedFolderAncestorIsJunctionWithExistingContent_DoesNotBlindDeleteThroughLink() - { - // NTFS directory junctions are a Windows-only concept; skip on other CI runners. - if (!OperatingSystem.IsWindows()) - { - Assert.Skip("Directory junctions are a Windows-only filesystem feature."); - } - - // Arrange: a repo root whose ".github" entry is a junction to a separate, isolated - // directory that *already* has a real "agents" folder with content - so - // Directory.Exists(folderPath) is true through the junction, and without the - // DeleteManagedFolder symlink-ancestor guard, Extract's blind-delete step 2 would recurse - // through the junction and remove it before extraction's own guard is ever reached. - var zipPath = CreatePackageZip(("agents/copilot.md", "should not be extracted")); - var repoRoot = CreateTempDirectory(); - var linkTarget = CreateTempDirectory(); - var linkTargetAgentsDir = Path.Combine(linkTarget, "agents"); - Directory.CreateDirectory(linkTargetAgentsDir); - var keepFilePath = Path.Combine(linkTargetAgentsDir, "keepme.md"); - File.WriteAllText(keepFilePath, "must survive"); - try - { - CreateJunction(Path.Combine(repoRoot, ".github"), linkTarget); - - // Act / Assert: extraction is refused, and the pre-existing content behind the - // junction was never blind-deleted - Assert.Throws(() => PackageZipExtractor.Extract(zipPath, repoRoot)); - Assert.Equal("must survive", File.ReadAllText(keepFilePath)); - } - finally - { - File.Delete(zipPath); - // The ".github" junction entry itself must be removed (not recursively, since that - // would delete the link target's contents) before the repo root can be deleted. - Directory.Delete(Path.Combine(repoRoot, ".github")); - Directory.Delete(repoRoot, recursive: true); - Directory.Delete(linkTarget, recursive: true); - } - } - - /// - /// Test that a repo root which is itself a junction to another location is refused, since - /// every managed-folder operation would otherwise silently write through it. - /// - [Fact] - public void PackageZipExtractor_Extract_RepoRootIsJunction_ThrowsAndDoesNotWriteThroughLink() - { - // NTFS directory junctions are a Windows-only concept; skip on other CI runners. - if (!OperatingSystem.IsWindows()) - { - Assert.Skip("Directory junctions are a Windows-only filesystem feature."); - } - - // Arrange: a "repo root" that is itself nothing but a junction to a separate, isolated - // directory - simulating a caller-supplied path that resolves through a link before any - // managed-folder segment is even appended. - var zipPath = CreatePackageZip(("agents/copilot.md", "should not be extracted")); - var linkTarget = CreateTempDirectory(); - var repoRootParent = CreateTempDirectory(); - var repoRoot = Path.Combine(repoRootParent, "repo-root-link"); - try - { - CreateJunction(repoRoot, linkTarget); - - // Act / Assert: extraction is refused, and nothing was written through the link - Assert.Throws(() => PackageZipExtractor.Extract(zipPath, repoRoot)); - Assert.False(File.Exists(Path.Combine(linkTarget, ".github", "agents", "copilot.md"))); - } - finally - { - File.Delete(zipPath); - // The repo-root junction entry itself must be removed (not recursively, since that - // would delete the link target's contents) before the parent can be deleted. - Directory.Delete(repoRoot); - Directory.Delete(repoRootParent, recursive: true); - Directory.Delete(linkTarget, recursive: true); - } - } - - /// - /// Test that a junction/symlink nested *inside* a managed folder (not just an ancestor of - /// it) is rejected during the blind-delete step, since a plain recursive - /// would otherwise follow it and delete - /// content outside the repo root. - /// - [Fact] - public void PackageZipExtractor_Extract_ManagedFolderContainsNestedJunction_ThrowsAndDoesNotDeleteThroughLink() - { - // NTFS directory junctions are a Windows-only concept; skip on other CI runners. - if (!OperatingSystem.IsWindows()) - { - Assert.Skip("Directory junctions are a Windows-only filesystem feature."); - } - - // Arrange: a normal (non-linked) ".github/agents" managed folder that itself contains a - // nested junction pointing to a separate, isolated directory with content that must - // survive. - var zipPath = CreatePackageZip(("agents/copilot.md", "should not be extracted")); - var repoRoot = CreateTempDirectory(); - var linkTarget = CreateTempDirectory(); - var keepFilePath = Path.Combine(linkTarget, "keepme.md"); - File.WriteAllText(keepFilePath, "must survive"); - var agentsDir = Path.Combine(repoRoot, ".github", "agents"); - Directory.CreateDirectory(agentsDir); - try - { - CreateJunction(Path.Combine(agentsDir, "linked"), linkTarget); - - // Act / Assert: the blind delete is refused, and the linked content was never deleted - Assert.Throws(() => PackageZipExtractor.Extract(zipPath, repoRoot)); - Assert.Equal("must survive", File.ReadAllText(keepFilePath)); - } - finally - { - File.Delete(zipPath); - // The nested junction entry itself must be removed (not recursively) before the repo - // root can be deleted. - Directory.Delete(Path.Combine(agentsDir, "linked")); - Directory.Delete(repoRoot, recursive: true); - Directory.Delete(linkTarget, recursive: true); - } - } - - /// - /// Test that a *dangling* symlink/junction ancestor (one whose target no longer exists) is - /// still rejected, exercising the -based reparse - /// check regardless of whether the link's target exists - unlike the former - /// -based check, which follows the link to test for - /// the target's existence and would silently report "not a directory" (skipping the - /// guard entirely) for a dangling link. Runs on every platform: real symbolic links on - /// Linux/macOS via , and NTFS - /// junctions on Windows (created against a real target, then made dangling by deleting - /// that target, since mklink /J itself requires an existing target). - /// - [Fact] - public void PackageZipExtractor_Extract_ManagedFolderAncestorIsDanglingLink_ThrowsAndDoesNotBypassGuard() - { - var zipPath = CreatePackageZip(("agents/copilot.md", "should not be extracted")); - var repoRoot = CreateTempDirectory(); - var githubPath = Path.Combine(repoRoot, ".github"); - try - { - CreateDanglingLink(githubPath); - - // Act / Assert: extraction is refused, not silently allowed through the dangling link - Assert.Throws(() => PackageZipExtractor.Extract(zipPath, repoRoot)); - } - finally - { - File.Delete(zipPath); - // The dangling link entry itself must be removed before the repo root can be - // deleted. Directory.Delete follows the link (via stat/lstat) to confirm it is a - // directory before removing it, which fails with DirectoryNotFoundException for a - // *dangling* link whose target no longer exists - this only works here because - // Windows junction metadata lives on the link entry itself, independent of target - // validity. On Linux/macOS, a dangling symlink must instead be removed with - // File.Delete, which unlinks the directory entry directly without following it. - if (OperatingSystem.IsWindows()) - { - Directory.Delete(githubPath); - } - else - { - File.Delete(githubPath); - } - - Directory.Delete(repoRoot, recursive: true); - } - } - - /// - /// Test that refuses to trust a - /// managed folder that is only reachable through a reparse-point ancestor, rather than - /// silently reporting it as present (which would let a caller such as - /// RepoCardViewModel.EnsureAgentFilesSyncedBeforeLaunch skip Extract - /// entirely and launch using files outside the repo root). - /// - [Fact] - public void PackageZipExtractor_AllManagedFoldersExist_AncestorIsJunctionWithRealFolders_ReturnsFalse() - { - // NTFS directory junctions are a Windows-only concept; skip on other CI runners. - if (!OperatingSystem.IsWindows()) - { - Assert.Skip("Directory junctions are a Windows-only filesystem feature."); - } - - // Arrange: a repo root whose ".github" entry is a junction to a separate, isolated - // directory that genuinely has all four managed folders - so the naive - // Directory.Exists-based check alone would (incorrectly) report every folder as present. - var repoRoot = CreateTempDirectory(); - var linkTarget = CreateTempDirectory(); - foreach (var folder in new[] { "agents", "standards", "templates", "skills" }) - { - Directory.CreateDirectory(Path.Combine(linkTarget, folder)); - } - - try - { - CreateJunction(Path.Combine(repoRoot, ".github"), linkTarget); - - Assert.False(PackageZipExtractor.AllManagedFoldersExist(repoRoot)); - } - finally - { - Directory.Delete(Path.Combine(repoRoot, ".github")); - Directory.Delete(repoRoot, recursive: true); - Directory.Delete(linkTarget, recursive: true); - } - } - - /// - /// Creates a dangling directory symlink/junction at - one - /// whose target does not exist - using a real symbolic link on Linux/macOS (created - /// without any target validation) or an NTFS junction on Windows (created against a real - /// temporary target that is deleted immediately afterward). - /// - /// The link's path; its parent must exist and it must not already - /// exist. - private static void CreateDanglingLink(string linkPath) - { - if (OperatingSystem.IsWindows()) - { - var target = CreateTempDirectory(); - CreateJunction(linkPath, target); - Directory.Delete(target); - } - else - { - Directory.CreateSymbolicLink(linkPath, Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N"))); - } - } - - /// - /// Creates an NTFS directory junction at pointing to - /// , using mklink /J since junctions (unlike symbolic - /// links) do not require elevated privileges or Developer Mode on Windows. - /// - /// The junction's path; its parent must exist and it must not already - /// exist. - /// The existing directory the junction points to. - private static void CreateJunction(string linkPath, string targetPath) - { - var startInfo = new ProcessStartInfo("cmd.exe", $"/c mklink /J \"{linkPath}\" \"{targetPath}\"") - { - RedirectStandardOutput = true, - RedirectStandardError = true, - UseShellExecute = false, - CreateNoWindow = true - }; - - using var process = Process.Start(startInfo) - ?? throw new InvalidOperationException("Failed to start 'cmd.exe' to create junction."); - process.WaitForExit(); - if (process.ExitCode != 0) - { - throw new InvalidOperationException( - $"Failed to create junction '{linkPath}' -> '{targetPath}': {process.StandardError.ReadToEnd()}"); - } - } - /// /// Creates a temporary package zip with the four managed folders populated from the /// given (relative-path-under-.github, content) pairs, plus a root-level file that must diff --git a/test/DemaConsulting.AgentControl.Tests/Utilities/PathHelpersTests.cs b/test/DemaConsulting.AgentControl.Tests/Utilities/PathHelpersTests.cs index 83991c6..abdc74e 100644 --- a/test/DemaConsulting.AgentControl.Tests/Utilities/PathHelpersTests.cs +++ b/test/DemaConsulting.AgentControl.Tests/Utilities/PathHelpersTests.cs @@ -190,310 +190,6 @@ public void PathHelpers_SafePathCombine_NullRelativePath_ThrowsArgumentNullExcep Assert.Throws(() => PathHelpers.SafePathCombine("/home/user", null!)); } - - /// - /// Test that FindReparsePointInAncestry returns null when no segment between path and - /// root is a reparse point. - /// - [Fact] - public void PathHelpers_FindReparsePointInAncestry_NoReparsePoints_ReturnsNull() - { - // Arrange: an ordinary nested directory with no symlinks/junctions anywhere - var root = CreateTempDirectory(); - try - { - var nested = Path.Combine(root, "a", "b"); - Directory.CreateDirectory(nested); - - // Act - var result = PathHelpers.FindReparsePointInAncestry(root, nested); - - // Assert - Assert.Null(result); - } - finally - { - Directory.Delete(root, recursive: true); - } - } - - /// - /// Test that FindReparsePointInAncestry finds a junction between root and path. - /// - [Fact] - public void PathHelpers_FindReparsePointInAncestry_AncestorIsJunction_ReturnsJunctionPath() - { - // Arrange: root/link is a junction, and the path being checked is a descendant of it - var root = CreateTempDirectory(); - var linkTarget = CreateTempDirectory(); - var linkPath = Path.Combine(root, "link"); - try - { - CreateJunction(linkPath, linkTarget); - var descendant = Path.Combine(linkPath, "nested"); - - // Act - var result = PathHelpers.FindReparsePointInAncestry(root, descendant); - - // Assert - Assert.Equal(linkPath, result); - } - finally - { - Directory.Delete(linkPath, recursive: false); - Directory.Delete(root, recursive: true); - Directory.Delete(linkTarget, recursive: true); - } - } - - /// - /// Test that FindReparsePointInAncestry finds root itself when it is a junction. - /// - [Fact] - public void PathHelpers_FindReparsePointInAncestry_RootIsJunction_ReturnsRoot() - { - // Arrange: root is itself a junction - var parent = CreateTempDirectory(); - var linkTarget = CreateTempDirectory(); - var root = Path.Combine(parent, "root-link"); - try - { - CreateJunction(root, linkTarget); - var descendant = Path.Combine(root, "nested"); - - // Act - var result = PathHelpers.FindReparsePointInAncestry(root, descendant); - - // Assert - Assert.Equal(root, result); - } - finally - { - Directory.Delete(root, recursive: false); - Directory.Delete(parent, recursive: true); - Directory.Delete(linkTarget, recursive: true); - } - } - - /// - /// Test that FindReparsePointInAncestry finds a dangling junction ancestor - one whose - /// target no longer exists - rather than silently missing it (as Directory.Exists would). - /// - [Fact] - public void PathHelpers_FindReparsePointInAncestry_AncestorIsDanglingLink_ReturnsLinkPath() - { - // Arrange - var root = CreateTempDirectory(); - var linkPath = Path.Combine(root, "dangling"); - try - { - CreateDanglingLink(linkPath); - var descendant = Path.Combine(linkPath, "nested", "deeper"); - - // Act - var result = PathHelpers.FindReparsePointInAncestry(root, descendant); - - // Assert - Assert.Equal(linkPath, result); - } - finally - { - if (OperatingSystem.IsWindows()) - { - Directory.Delete(linkPath, recursive: false); - } - else - { - File.Delete(linkPath); - } - - Directory.Delete(root, recursive: true); - } - } - - /// - /// Test that FindReparsePointInAncestry throws ArgumentNullException when root is null. - /// - [Fact] - public void PathHelpers_FindReparsePointInAncestry_NullRoot_ThrowsArgumentNullException() - { - Assert.Throws(() => - PathHelpers.FindReparsePointInAncestry(null!, "/some/path")); - } - - /// - /// Test that FindReparsePointInAncestry throws ArgumentNullException when path is null. - /// - [Fact] - public void PathHelpers_FindReparsePointInAncestry_NullPath_ThrowsArgumentNullException() - { - Assert.Throws(() => - PathHelpers.FindReparsePointInAncestry("/some/root", null!)); - } - - /// - /// Test that FindReparsePointInDescendants returns null for a tree with no reparse points. - /// - [Fact] - public void PathHelpers_FindReparsePointInDescendants_NoReparsePoints_ReturnsNull() - { - // Arrange: an ordinary nested directory tree with no symlinks/junctions anywhere - var root = CreateTempDirectory(); - try - { - Directory.CreateDirectory(Path.Combine(root, "a", "b")); - - // Act - var result = PathHelpers.FindReparsePointInDescendants(root); - - // Assert - Assert.Null(result); - } - finally - { - Directory.Delete(root, recursive: true); - } - } - - /// - /// Test that FindReparsePointInDescendants finds a nested junction anywhere in the tree. - /// - [Fact] - public void PathHelpers_FindReparsePointInDescendants_NestedJunction_ReturnsJunctionPath() - { - // Arrange: root/a/linked is a junction nested two levels deep - var root = CreateTempDirectory(); - var linkTarget = CreateTempDirectory(); - var linkPath = Path.Combine(root, "a", "linked"); - try - { - Directory.CreateDirectory(Path.Combine(root, "a")); - CreateJunction(linkPath, linkTarget); - - // Act - var result = PathHelpers.FindReparsePointInDescendants(root); - - // Assert - Assert.Equal(linkPath, result); - } - finally - { - Directory.Delete(linkPath, recursive: false); - Directory.Delete(root, recursive: true); - Directory.Delete(linkTarget, recursive: true); - } - } - - /// - /// Test that FindReparsePointInDescendants returns the directory itself when it is a - /// reparse point. - /// - [Fact] - public void PathHelpers_FindReparsePointInDescendants_DirectoryItselfIsJunction_ReturnsDirectory() - { - // Arrange - var parent = CreateTempDirectory(); - var linkTarget = CreateTempDirectory(); - var linkPath = Path.Combine(parent, "link"); - try - { - CreateJunction(linkPath, linkTarget); - - // Act - var result = PathHelpers.FindReparsePointInDescendants(linkPath); - - // Assert - Assert.Equal(linkPath, result); - } - finally - { - Directory.Delete(linkPath, recursive: false); - Directory.Delete(parent, recursive: true); - Directory.Delete(linkTarget, recursive: true); - } - } - - /// - /// Test that FindReparsePointInDescendants throws ArgumentNullException when directory is - /// null. - /// - [Fact] - public void PathHelpers_FindReparsePointInDescendants_NullDirectory_ThrowsArgumentNullException() - { - Assert.Throws(() => - PathHelpers.FindReparsePointInDescendants(null!)); - } - - /// - /// Creates an NTFS directory junction at pointing to - /// (Windows), or a real directory symbolic link - /// (Linux/macOS), since junctions and symbolic links behave identically for the purposes - /// of these tests - both are reparse points reported by - /// - and symbolic links do not require an - /// existing target or elevated privileges on non-Windows platforms. - /// - /// The link's path; its parent must exist and it must not already - /// exist. - /// The existing directory the link points to. - private static void CreateJunction(string linkPath, string targetPath) - { - if (OperatingSystem.IsWindows()) - { - var startInfo = new System.Diagnostics.ProcessStartInfo("cmd.exe", $"/c mklink /J \"{linkPath}\" \"{targetPath}\"") - { - RedirectStandardOutput = true, - RedirectStandardError = true, - UseShellExecute = false, - CreateNoWindow = true - }; - - using var process = System.Diagnostics.Process.Start(startInfo) - ?? throw new InvalidOperationException("Failed to start 'cmd.exe' to create junction."); - process.WaitForExit(); - if (process.ExitCode != 0) - { - throw new InvalidOperationException( - $"Failed to create junction '{linkPath}' -> '{targetPath}': {process.StandardError.ReadToEnd()}"); - } - } - else - { - Directory.CreateSymbolicLink(linkPath, targetPath); - } - } - - /// - /// Creates a dangling directory symlink/junction at - one - /// whose target does not exist - using a real symbolic link on Linux/macOS (created - /// without any target validation) or an NTFS junction on Windows (created against a real - /// temporary target that is deleted immediately afterward). - /// - /// The link's path; its parent must exist and it must not already - /// exist. - private static void CreateDanglingLink(string linkPath) - { - if (OperatingSystem.IsWindows()) - { - var target = CreateTempDirectory(); - CreateJunction(linkPath, target); - Directory.Delete(target); - } - else - { - Directory.CreateSymbolicLink(linkPath, Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N"))); - } - } - - /// - /// Creates a unique temporary directory for test isolation. - /// - /// The created directory's path. - private static string CreateTempDirectory() - { - var path = Path.Combine(Path.GetTempPath(), "agentcontrol_path_helpers_test_" + Guid.NewGuid()); - Directory.CreateDirectory(path); - return path; - } } From ba54427e5bc4b3c76522d430048baf8684599142 Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Mon, 14 Sep 2026 02:20:00 -0400 Subject: [PATCH 10/10] Reject invalid zip entries before deleting managed folders Addresses PR review feedback that a malicious/malformed zip entry converts a SafePathCombine containment violation into a plain InvalidOperationException, which RepoCardViewModel's best-effort sync path treats as an ordinary failure and launches anyway - after the managed folders had already been blind-deleted. PackageZipExtractor.Extract now resolves and validates every entry's destination path in an up-front pass, before any managed folder is deleted. An invalid or path-escaping entry rejects the whole upgrade immediately, so the repo's existing managed folders are never blind-deleted ahead of a rejected extraction. Extraction itself is unchanged (still no rollback for a genuine I/O failure mid-extract, per architecture.md). Adds a regression test verifying a repo-root-escaping entry is rejected before deletion and pre-existing managed-folder content survives, and updates the design/verification docs accordingly. Build and full test suite (203 tests) verified green; fix.ps1/lint.ps1 both clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../repo-sync/package-zip-extractor.md | 33 ++++---- .../repo-sync/package-zip-extractor.md | 13 ++++ .../RepoSync/PackageZipExtractor.cs | 77 +++++++++++++------ .../RepoSync/PackageZipExtractorTests.cs | 42 ++++++++++ 4 files changed, 130 insertions(+), 35 deletions(-) diff --git a/docs/design/agent-control/repo-sync/package-zip-extractor.md b/docs/design/agent-control/repo-sync/package-zip-extractor.md index 5626139..ed39119 100644 --- a/docs/design/agent-control/repo-sync/package-zip-extractor.md +++ b/docs/design/agent-control/repo-sync/package-zip-extractor.md @@ -8,10 +8,13 @@ four managed agent folders (`.github/agents`, `.github/standards`, `.github/templates`, `.github/skills`) from a package zip file. Per architecture.md's "Blind delete of the four known folders" decision (see `docs/design/agent-control/repo-sync.md`), it performs: (1) open/ -validate the zip; (2) delete the four known folders if present; (3) extract only those same -four folders from the zip — root-level files such as `release-notes.md` are never extracted to -disk. Rewriting the `.agentcontrol.json` pin is the caller's responsibility (the `RepoConfig` -subsystem), so this class stays focused on file operations alone. +validate the zip; (2) validate every entry's destination path up front, before anything is +deleted; (3) delete the four known folders if present; (4) extract only the already-validated +entries that fall within those same four folders — root-level files such as `release-notes.md` +are never extracted to disk. Validating every entry before the delete step means an invalid or +path-traversing entry rejects the whole upgrade without ever touching the existing managed +folders. Rewriting the `.agentcontrol.json` pin is the caller's responsibility (the +`RepoConfig` subsystem), so this class stays focused on file operations alone. #### Data Model @@ -23,15 +26,17 @@ consulted by both `Extract` and `AllManagedFoldersExist`. #### Key Methods -**Extract**: Opens/validates the zip, deletes the four managed folders if present, and -extracts the same four folders from the zip into the repo. +**Extract**: Opens/validates the zip, validates every entry's destination path, deletes the +four managed folders if present, and extracts the already-validated entries into the repo. - *Parameters*: `string zipPath`, `string repoRoot`. - *Returns*: `void`. - *Preconditions*: Neither parameter `null`. - *Postconditions*: The four managed folders under `repoRoot` match the zip's contents exactly - (`AgentControl-PackageZipExtractor-Extract`). There is intentionally no rollback on failure — - a partially-applied change is possible and must be resolved manually by the caller, per + (`AgentControl-PackageZipExtractor-Extract`). An entry with an invalid destination path is + rejected before any managed folder is deleted, so that failure mode never leaves the repo + partially upgraded. A delete or extraction I/O failure can still do so — there is + intentionally no rollback for that case, and it must be resolved manually by the caller, per architecture.md's "Upgrade sequence and failure handling" decision. **AllManagedFoldersExist**: Determines whether all four managed folders currently exist under @@ -55,11 +60,13 @@ entry without extracting it to disk. #### Error Handling `Extract` throws `ArgumentNullException` for a null `zipPath`/`repoRoot`, and -`InvalidOperationException` when the zip cannot be opened/is not a valid archive, a managed -folder cannot be deleted, or extraction fails partway through — wrapping the underlying -`IOException`/`UnauthorizedAccessException`/`InvalidDataException` with a message naming the -zip path and repo root. `ReadReleaseNotes` throws the same `InvalidOperationException` pattern -for a zip that cannot be opened or whose release-notes entry cannot be read. +`InvalidOperationException` when the zip cannot be opened/is not a valid archive, an entry's +destination path is invalid (including resolving outside `repoRoot`), a managed folder cannot +be deleted, or extraction fails partway through — wrapping the underlying +`IOException`/`UnauthorizedAccessException`/`InvalidDataException`/`ArgumentException`/ +`NotSupportedException` with a message naming the zip path and repo root (or the offending +entry). `ReadReleaseNotes` throws the same `InvalidOperationException` pattern for a zip that +cannot be opened or whose release-notes entry cannot be read. #### Dependencies diff --git a/docs/verification/agent-control/repo-sync/package-zip-extractor.md b/docs/verification/agent-control/repo-sync/package-zip-extractor.md index 3445411..4bb9fd8 100644 --- a/docs/verification/agent-control/repo-sync/package-zip-extractor.md +++ b/docs/verification/agent-control/repo-sync/package-zip-extractor.md @@ -16,6 +16,9 @@ N/A - standard test environment. - All unit tests pass with zero failures. - Extraction fully replaces managed folder contents and creates them when absent. - An invalid zip file is rejected with a clear exception rather than a partial extraction. +- A zip entry that escapes the repo root, or resolves inside the repo root but outside every + managed folder, is rejected/skipped without ever writing outside a managed folder, and + without leaving pre-existing managed folders blind-deleted ahead of a rejected extraction. - Folder-presence and release-notes checks reflect genuine filesystem/archive state. #### Test Scenarios @@ -29,6 +32,16 @@ throws `InvalidOperationException`. This scenario is tested by `PackageZipExtractor_Extract_InvalidZipFile_ThrowsInvalidOperationException`, covering `AgentControl-PackageZipExtractor-Extract`. +**PackageZipExtractor_Extract_RejectsPathTraversalWithoutPartialUpgrade**: A zip entry that +textually starts with a managed-folder prefix but uses ".." to resolve outside every managed +folder (while staying under the repo root) is silently skipped rather than extracted. A zip +entry that escapes the repo root entirely is rejected with `InvalidOperationException` before +any managed folder is deleted, leaving a pre-existing managed folder's contents untouched. +This scenario is tested by +`PackageZipExtractor_Extract_TraversalEntryWithinRepoRoot_DoesNotEscapeManagedFolders` and +`PackageZipExtractor_Extract_EntryEscapesRepoRoot_ThrowsBeforeDeletingManagedFolders`, covering +`AgentControl-PackageZipExtractor-Extract`. + **PackageZipExtractor_AllManagedFoldersExist_ReflectsActualPresence**: The check returns true when all four managed folders are present, and false when some or none are present. This scenario is tested by diff --git a/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs b/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs index e0a6815..1808daa 100644 --- a/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs +++ b/src/DemaConsulting.AgentControl/RepoSync/PackageZipExtractor.cs @@ -64,19 +64,22 @@ internal static class PackageZipExtractor private const string ReleaseNotesEntryName = "release-notes.md"; /// - /// Opens/validates the package zip, deletes the four managed folders under - /// if present, and extracts the same four folders from the - /// zip into the repo. + /// Opens/validates the package zip, validates every entry's destination path, deletes the + /// four managed folders under if present, and extracts the + /// same four folders from the zip into the repo. /// /// Path to the agent package zip file. /// Absolute path to the repository root to sync. /// Thrown when or /// is . /// - /// Thrown when the zip cannot be opened/is not a valid zip archive, when a managed folder - /// cannot be deleted, or when extraction fails partway through. Per the class remarks, - /// there is no rollback: a partially-applied change is possible and must be resolved - /// manually by the caller. + /// Thrown when the zip cannot be opened/is not a valid zip archive, when an entry's + /// destination path is invalid (including resolving outside ), + /// when a managed folder cannot be deleted, or when extraction fails partway through. An + /// invalid entry path is rejected before any managed folder is deleted, so that failure + /// mode never leaves the repo partially upgraded; a delete or extraction failure can + /// still do so, and per the class remarks there is no rollback for that case — it must be + /// resolved manually by the caller. /// public static void Extract(string zipPath, string repoRoot) { @@ -87,19 +90,36 @@ public static void Extract(string zipPath, string repoRoot) // good per architecture.md — no checksum/signature verification is performed. using var archive = OpenArchive(zipPath); - // Step 2: blind-delete the four known folders (if present) + // Step 2: resolve and validate every entry's destination path up front, before anything + // is deleted. A malformed or path-traversing entry (one that fails SafePathCombine's + // containment check) must reject the whole upgrade before any managed folder is + // touched — otherwise a malicious/corrupt zip could blind-delete the existing managed + // folders and then fail partway through extraction, leaving the repo unsynced while + // still reporting only an "ordinary" failure to callers such as RepoCardViewModel's + // best-effort, never-block-launch sync path. + var managedEntries = new List<(ZipArchiveEntry Entry, string DestinationPath)>(); + foreach (var entry in archive.Entries) + { + var destinationPath = ResolveManagedDestination(entry, repoRoot); + if (destinationPath is not null) + { + managedEntries.Add((entry, destinationPath)); + } + } + + // Step 3: blind-delete the four known folders (if present) foreach (var folder in ManagedFolders) { DeleteManagedFolder(repoRoot, folder); } - // Step 3: extract only the same four folders from the zip, skipping root-level files - // such as release-notes.md + // Step 4: extract the already-validated managed entries, skipping any non-managed/root- + // level files such as release-notes.md (those were never added to managedEntries above) try { - foreach (var entry in archive.Entries) + foreach (var (entry, destinationPath) in managedEntries) { - ExtractEntryIfManaged(entry, repoRoot); + ExtractEntry(entry, destinationPath); } } catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) @@ -215,21 +235,25 @@ private static void DeleteManagedFolder(string repoRoot, string relativeFolder) } /// - /// Extracts a single zip entry into the repo root, but only if it falls within one of the - /// four managed folders; entries elsewhere (including root-level files like - /// release-notes.md) are skipped. + /// Resolves a single zip entry's destination path if it falls within one of the four + /// managed folders, validating that path but not yet writing anything to disk. /// /// The zip entry to consider. /// Absolute path to the repository root. + /// + /// The entry's absolute destination path if it falls within a managed folder; otherwise + /// (a directory entry, or a file entry that does not resolve + /// inside any managed folder, including root-level files like release-notes.md). + /// /// Thrown when the entry's path is invalid, /// including resolving outside . - private static void ExtractEntryIfManaged(ZipArchiveEntry entry, string repoRoot) + private static string? ResolveManagedDestination(ZipArchiveEntry entry, string repoRoot) { // Directory entries have an empty Name (only FullName ends with '/'); skip them, as - // CreateDirectory below (driven by file entries) recreates any needed structure. + // CreateDirectory in ExtractEntry (driven by file entries) recreates any needed structure. if (string.IsNullOrEmpty(entry.Name)) { - return; + return null; } // Zip entries always use '/' regardless of platform; normalize before combining. @@ -257,11 +281,20 @@ private static void ExtractEntryIfManaged(ZipArchiveEntry entry, string repoRoot // resolves elsewhere. Deriving the relative path from the already-validated // destinationPath closes that gap. var canonicalRelativePath = Path.GetRelativePath(Path.GetFullPath(repoRoot), destinationPath); - if (!IsInsideManagedFolder(canonicalRelativePath)) - { - return; - } + return IsInsideManagedFolder(canonicalRelativePath) ? destinationPath : null; + } + /// + /// Extracts a single zip entry to an already-validated destination path, creating any + /// missing parent directories first. + /// + /// The zip entry to extract. + /// + /// The entry's destination path, as previously resolved and validated by + /// . + /// + private static void ExtractEntry(ZipArchiveEntry entry, string destinationPath) + { var destinationDirectory = Path.GetDirectoryName(destinationPath); if (!string.IsNullOrEmpty(destinationDirectory)) { diff --git a/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs b/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs index 0eae1fd..938980d 100644 --- a/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs +++ b/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs @@ -260,6 +260,48 @@ public void PackageZipExtractor_Extract_TraversalEntryWithinRepoRoot_DoesNotEsca } } + /// + /// Test that a zip entry whose ".."-resolved path escapes the repo root entirely (the + /// classic zip-slip payload) is rejected before any managed folder is deleted, leaving a + /// pre-existing managed folder's contents untouched rather than blind-deleted ahead of a + /// failed extraction. + /// + [Fact] + public void PackageZipExtractor_Extract_EntryEscapesRepoRoot_ThrowsBeforeDeletingManagedFolders() + { + // Arrange: a repo with a pre-existing, populated managed folder, and a package whose + // first entry resolves outside the repo root entirely + var zipPath = Path.Combine(Path.GetTempPath(), "agentcontrol_package_" + Guid.NewGuid() + ".zip"); + using (var archive = ZipFile.Open(zipPath, ZipArchiveMode.Create)) + { + var entry = archive.CreateEntry(".github/agents/../../../outside.txt"); + using var writer = new StreamWriter(entry.Open()); + writer.Write("must not be extracted"); + } + + var repoRoot = CreateTempDirectory(); + var agentsDir = Path.Combine(repoRoot, ".github", "agents"); + Directory.CreateDirectory(agentsDir); + File.WriteAllText(Path.Combine(agentsDir, "existing.md"), "pre-existing content"); + try + { + // Act / Assert: extraction is refused + Assert.Throws(() => PackageZipExtractor.Extract(zipPath, repoRoot)); + + // Assert: the pre-existing managed folder was never blind-deleted, since the bad + // entry is rejected during up-front validation, before any deletion occurs + Assert.Equal("pre-existing content", File.ReadAllText(Path.Combine(agentsDir, "existing.md"))); + + // Assert: nothing was written outside the repo root + Assert.False(File.Exists(Path.Combine(Path.GetDirectoryName(repoRoot)!, "outside.txt"))); + } + finally + { + File.Delete(zipPath); + Directory.Delete(repoRoot, recursive: true); + } + } + /// /// Creates a temporary package zip with the four managed folders populated from the /// given (relative-path-under-.github, content) pairs, plus a root-level file that must