From 50ed55972a140809bb755d5ab727e05433631a11 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 21 Sep 2026 21:25:26 +0000 Subject: [PATCH] fix: survive an unreadable directory while scanning the dev directory [patch] ScanDevDirectoryForOwnersAndRepos handed the whole tree to the recursive form of Directory.EnumerateDirectories, which leaves IgnoreInaccessible off and enumerates lazily. A single permission-denied folder anywhere under the dev directory therefore threw part way through the walk, on the ImGui render thread, and took the application with it -- and a dev directory holds exactly the package caches, build output and IDE metadata that produce such a folder. Replace it with EnumerateGitDirectories, which lists one level at a time and skips a directory that refuses to be read, so a refusal costs only that subtree rather than every repository that would have been found after it. The walk also stops at a .git directory, whose contents are git's own storage and hold no further working trees. The lister is injectable because a test process running as root -- which CI containers routinely do -- bypasses the permission bits, so chmod alone would pass against the unfixed code. The on-disk test is kept as well and goes inconclusive where the host cannot deny a read. Fixes #425 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01EdV5iCFkUxFqkLZQAGLAVT --- ProjectDirector.Test/DevDirectoryScanTests.cs | 207 ++++++++++++++++++ ProjectDirector/ProjectDirector.cs | 66 +++++- 2 files changed, 272 insertions(+), 1 deletion(-) create mode 100644 ProjectDirector.Test/DevDirectoryScanTests.cs diff --git a/ProjectDirector.Test/DevDirectoryScanTests.cs b/ProjectDirector.Test/DevDirectoryScanTests.cs new file mode 100644 index 0000000..356a796 --- /dev/null +++ b/ProjectDirector.Test/DevDirectoryScanTests.cs @@ -0,0 +1,207 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.ProjectDirector.Test; + +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +/// +/// Tests the walk that finds working trees under the configured dev directory. +/// +/// +/// The scan used to hand the whole tree to the recursive form of +/// , which leaves +/// off and enumerates lazily. One unreadable +/// folder therefore threw part way through, on the render thread, and took the whole application +/// with it -- and a dev directory holds exactly the package caches, build output and IDE metadata +/// that produce such a folder. exists +/// separately so that rule can be driven without a live ImGui context, the way +/// drives the pull rule. +/// +[TestClass] +public sealed class DevDirectoryScanTests +{ + private static string CreateTree() + { + string root = Path.Join(Path.GetTempPath(), $"ktsu_pd_scan_{Guid.NewGuid():N}"); + _ = Directory.CreateDirectory(Path.Join(root, "alpha", ".git")); + _ = Directory.CreateDirectory(Path.Join(root, "nested", "beta", ".git")); + _ = Directory.CreateDirectory(Path.Join(root, "nested", "beta", "src")); + _ = Directory.CreateDirectory(Path.Join(root, "notarepo")); + return root; + } + + private static string[] Walk(string root, Func? listDirectories = null) => + [.. ProjectDirector.EnumerateGitDirectories(root, listDirectories) + .Select(Path.GetFullPath) + .Order(StringComparer.Ordinal)]; + + [TestMethod] + public void EveryWorkingTreeUnderTheRootIsFound() + { + string root = CreateTree(); + try + { + string[] found = Walk(root); + + CollectionAssert.AreEqual( + new[] + { + Path.GetFullPath(Path.Join(root, "alpha", ".git")), + Path.GetFullPath(Path.Join(root, "nested", "beta", ".git")), + }.Order(StringComparer.Ordinal).ToArray(), + found, + "The walk should report every .git directory at any depth and nothing else."); + } + finally + { + Directory.Delete(root, recursive: true); + } + } + + /// + /// The regression this file exists for: a directory that refuses to be listed must cost only + /// itself, not the repositories that sort after it. + /// + /// + /// The refusal is injected rather than arranged with file permissions because a test process + /// running as root -- which CI containers routinely do -- bypasses the permission bits + /// entirely, so chmod would quietly produce a readable directory and the test would pass + /// against the unfixed code. covers the real + /// file system wherever the host can actually deny a read. + /// + [TestMethod] + public void ADeniedDirectoryCostsOnlyItsOwnSubtree() + { + string root = CreateTree(); + try + { + string denied = Path.Join(root, "nested"); + List refused = []; + + string[] found = Walk(root, directory => + { + if (string.Equals(directory, denied, StringComparison.Ordinal)) + { + refused.Add(directory); + throw new UnauthorizedAccessException($"Access to the path '{directory}' is denied."); + } + + return Directory.GetDirectories(directory); + }); + + Assert.AreEqual(1, refused.Count, "The walk should have reached the denied directory exactly once."); + CollectionAssert.AreEqual( + new[] { Path.GetFullPath(Path.Join(root, "alpha", ".git")) }, + found, + "The readable half of the tree should survive a directory that refuses to be listed."); + } + finally + { + Directory.Delete(root, recursive: true); + } + } + + [TestMethod] + public void ADirectoryThatVanishesMidWalkIsSkipped() + { + string root = CreateTree(); + try + { + string vanished = Path.Join(root, "nested"); + + string[] found = Walk(root, directory => string.Equals(directory, vanished, StringComparison.Ordinal) + ? throw new DirectoryNotFoundException($"Could not find a part of the path '{directory}'.") + : Directory.GetDirectories(directory)); + + CollectionAssert.AreEqual( + new[] { Path.GetFullPath(Path.Join(root, "alpha", ".git")) }, + found, + "A directory removed while the scan is running should not end the scan."); + } + finally + { + Directory.Delete(root, recursive: true); + } + } + + [TestMethod] + public void AMissingRootYieldsNothingRatherThanThrowing() + { + string missing = Path.Join(Path.GetTempPath(), $"ktsu_pd_scan_{Guid.NewGuid():N}"); + + CollectionAssert.AreEqual(Array.Empty(), Walk(missing), "A dev directory that does not exist is not a crash."); + } + + [TestMethod] + public void TheContentsOfAGitDirectoryAreNotWalked() + { + string root = CreateTree(); + try + { + // A working tree checked out inside another repository's storage is not a second + // repository, and git's own object store is large enough to be worth not descending into. + _ = Directory.CreateDirectory(Path.Join(root, "alpha", ".git", "modules", "sub", ".git")); + + string[] found = Walk(root); + + CollectionAssert.AreEqual( + new[] + { + Path.GetFullPath(Path.Join(root, "alpha", ".git")), + Path.GetFullPath(Path.Join(root, "nested", "beta", ".git")), + }.Order(StringComparer.Ordinal).ToArray(), + found, + "The walk should stop at a .git directory rather than descend into it."); + } + finally + { + Directory.Delete(root, recursive: true); + } + } + + /// + /// The same regression against the real file system, for hosts where a read can actually be + /// denied. Goes inconclusive rather than passing vacuously where it cannot be. + /// + [TestMethod] + public void ADeniedDirectoryOnDiskIsSkipped() + { + if (OperatingSystem.IsWindows()) + { + Assert.Inconclusive("Denying a directory read on Windows needs an ACL edit rather than a mode change."); + return; + } + + string root = CreateTree(); + string denied = Path.Join(root, "nested"); + try + { + File.SetUnixFileMode(denied, UnixFileMode.None); + + try + { + _ = Directory.GetDirectories(denied); + Assert.Inconclusive("This process reads a mode-000 directory anyway, most likely because it is root."); + } + catch (UnauthorizedAccessException) + { + // The mode took effect, so the walk is about to meet a genuinely unreadable directory. + } + + CollectionAssert.AreEqual( + new[] { Path.GetFullPath(Path.Join(root, "alpha", ".git")) }, + Walk(root), + "The readable half of the tree should survive an unreadable directory on disk."); + } + finally + { + File.SetUnixFileMode(denied, UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute); + Directory.Delete(root, recursive: true); + } + } +} diff --git a/ProjectDirector/ProjectDirector.cs b/ProjectDirector/ProjectDirector.cs index 79d2af9..875e686 100644 --- a/ProjectDirector/ProjectDirector.cs +++ b/ProjectDirector/ProjectDirector.cs @@ -823,11 +823,75 @@ private bool UpdateClonedStatus(GitRepository repo) private static FullyQualifiedGitHubRepoName GetFullyQualifiedRepoName(Repository repo) => FullyQualifiedGitHubRepoName.Create(repo.FullName.Replace('/', '.')); private static FullyQualifiedGitHubRepoName GetFullyQualifiedRepoName(GitHubOwnerName ownerName, GitHubRepoName repoName) => FullyQualifiedGitHubRepoName.Create($"{ownerName}.{repoName}"); + /// + /// Walks for .git directories, skipping any directory that cannot + /// be read rather than abandoning the rest of the tree. + /// + /// The directory to walk. + /// + /// Lists the immediate subdirectories of one directory. Defaults to ; + /// the tests substitute a lister that denies a chosen directory, which is the one thing a test cannot + /// arrange through the file system itself when it runs as a user that bypasses permission checks. + /// + /// + /// The recursive form of + /// leaves off, and it is lazy, so one + /// permission-denied folder anywhere under the dev directory throws part way through the walk and + /// takes every repository that would have been found after it along with it. A dev directory + /// realistically holds package caches, build output and IDE metadata, so such a folder is ordinary + /// rather than exotic. Listing a level at a time keeps a refusal local to the directory that + /// raised it. + /// Descending stops at a .git directory, whose contents are git's own storage and hold no + /// further working trees. + /// + internal static IEnumerable EnumerateGitDirectories(string root, Func? listDirectories = null) + { + listDirectories ??= Directory.GetDirectories; + + Stack pending = new(); + pending.Push(root); + + while (pending.Count > 0) + { + string current = pending.Pop(); + string[] subdirectories; + + try + { + subdirectories = listDirectories(current); + } + catch (UnauthorizedAccessException) + { + // The process cannot read this directory; the rest of the tree is still worth walking. + continue; + } + catch (IOException) + { + // Covers a directory removed mid-walk, a dead junction, and an unreadable volume. + continue; + } + + foreach (string subdirectory in subdirectories) + { + // Matched without regard to case because that is what the previous pattern match did + // on Windows, which is where this application primarily runs. + if (string.Equals(Path.GetFileName(subdirectory), ".git", StringComparison.OrdinalIgnoreCase)) + { + yield return subdirectory; + } + else + { + pending.Push(subdirectory); + } + } + } + } + [System.Diagnostics.CodeAnalysis.SuppressMessage("Design", "CA1031:Do not catch general exception types", Justification = "")] private void ScanDevDirectoryForOwnersAndRepos() { // scan the dev directory for git repos and when we find one we add the owner to the list of owners and the repo to the list of repos - IEnumerable gitDirs = Directory.EnumerateDirectories(Options.DevDirectory, ".git", SearchOption.AllDirectories); + IEnumerable gitDirs = EnumerateGitDirectories(Options.DevDirectory); foreach (string gitDir in gitDirs) { // The working tree is the parent of the .git directory, so there is nothing to ask git