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/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/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..8bef58e 100644 --- a/src/DemaConsulting.AgentControl/AgentToolLauncher/AgentToolLauncher.cs +++ b/src/DemaConsulting.AgentControl/AgentToolLauncher/AgentToolLauncher.cs @@ -139,11 +139,18 @@ 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); + // 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, argumentsText.Value, startInfo.WorkingDirectory); + } var stopwatch = Stopwatch.StartNew(); try @@ -153,9 +160,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, argumentsText.Value, process.Id, stopwatch.ElapsedMilliseconds); + } return process; } @@ -166,13 +176,16 @@ 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, 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}': {ex.Message}", ex); + $"Failed to start shell process '{startInfo.FileName} {argumentsText.Value}': {ex.Message}", ex); } } } 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..b2be5eb 100644 --- a/src/DemaConsulting.AgentControl/GitIntegration/GitClient.cs +++ b/src/DemaConsulting.AgentControl/GitIntegration/GitClient.cs @@ -362,11 +362,15 @@ 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); - _logger.LogDebug( - "Starting git process '{GitExecutable}' with arguments '{Arguments}' in working directory '{WorkingDirectory}'", - _gitExecutablePath, argumentsText, repositoryPath); + // 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.Value, repositoryPath); + } var stopwatch = Stopwatch.StartNew(); try @@ -383,25 +387,28 @@ 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", - _gitExecutablePath, argumentsText, stopwatch.ElapsedMilliseconds); + _gitExecutablePath, argumentsText.Value, stopwatch.ElapsedMilliseconds); } 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.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}", - _gitExecutablePath, argumentsText, process.ExitCode, stderr); + _gitExecutablePath, argumentsText.Value, process.ExitCode, stderr); } return new GitCommandResult(process.ExitCode, stdout, stderr); @@ -417,13 +424,16 @@ 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, 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}': {ex.Message}", ex); + $"Failed to run '{_gitExecutablePath} {argumentsText.Value}': {ex.Message}", ex); } } } 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/RepoCardViewModel.cs b/src/DemaConsulting.AgentControl/LauncherUI/RepoCardViewModel.cs index 549446c..139d5bf 100644 --- a/src/DemaConsulting.AgentControl/LauncherUI/RepoCardViewModel.cs +++ b/src/DemaConsulting.AgentControl/LauncherUI/RepoCardViewModel.cs @@ -586,9 +586,10 @@ 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. /// /// /// @@ -621,8 +622,7 @@ 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), + /// 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. /// 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..1808daa 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") ]; /// @@ -59,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) { @@ -82,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) @@ -193,6 +218,7 @@ private static ZipArchive OpenArchive(string zipPath) private static void DeleteManagedFolder(string repoRoot, string relativeFolder) { var folderPath = PathHelpers.SafePathCombine(repoRoot, relativeFolder); + if (!Directory.Exists(folderPath)) { return; @@ -209,30 +235,66 @@ 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. - private static void ExtractEntryIfManaged(ZipArchiveEntry entry, string repoRoot) + /// + /// 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 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 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)) + + // PathHelpers.SafePathCombine is the single source of truth for containment validation + // (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 { - return; + destinationPath = PathHelpers.SafePathCombine(repoRoot, relativePath); } + catch (Exception ex) when (ex is ArgumentException or NotSupportedException) + { + throw new InvalidOperationException( + $"Zip entry '{entry.FullName}' has an invalid destination path: {ex.Message}", 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); + return IsInsideManagedFolder(canonicalRelativePath) ? destinationPath : null; + } - var destinationPath = PathHelpers.SafePathCombine(repoRoot, relativePath); + /// + /// 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)) { @@ -249,17 +311,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/RepoSync/PackageZipExtractorTests.cs b/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs index e84e4ad..938980d 100644 --- a/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs +++ b/test/DemaConsulting.AgentControl.Tests/RepoSync/PackageZipExtractorTests.cs @@ -224,6 +224,84 @@ 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 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 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); } }