From 325b382cb6c2d86935cdd7a143e2beeeb9c2c913 Mon Sep 17 00:00:00 2001 From: Tig Date: Mon, 6 Jul 2026 21:21:35 -0600 Subject: [PATCH] Fixes IsReparsePoint fail-open and PopoverMenu.DefaultKey binding collision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fixes flagged on release PR #5582: 1. Codex CR feedback: FileSystemTreeBuilder.IsReparsePoint returned false when reading Attributes throws, so an unreadable symlink/junction was treated as safe to traverse — recreating the directory-cycle risk the guard exists to prevent. Now fails closed (treats unreadable status as a reparse point), so CanExpand/GetChildren and FileDialog.SearchState do not traverse it. 2. CI failure (Parallel Unit Tests, windows): FileDialog's ctor crashed with "A binding for Ctrl+P exists" when PopoverMenu.DefaultKey read Ctrl+P — a key TableView already binds to Command.Up. Root cause was MecDottedKeyTests mutating the process-wide PopoverMenuSettings.Defaults while running in parallel with view-constructing tests; but the crash is also reachable by real users who configure PopoverMenu.DefaultKey to an already-bound key. - FileDialog/CharMap now use KeyBindings.ReplaceCommands instead of Add for the context-menu key (the user's configured key wins). - The "StaticSettingsTests" xUnit collection now has a CollectionDefinition with DisableParallelization = true, so the five Mec* test classes that swap static settings facades no longer race the rest of the suite. Tests: fail-closed IsReparsePoint regression test; FileDialog/CharMap ctor no-throw tests under a colliding DefaultKey. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../FileServices/FileSystemTreeBuilder.cs | 5 +- Terminal.Gui/Views/CharMap/CharMap.cs | 4 +- Terminal.Gui/Views/FileDialogs/FileDialog.cs | 5 +- .../PopoverMenuDefaultKeyCollisionTests.cs | 58 +++++++++++++++++++ .../StaticSettingsTestCollection.cs | 11 ++++ .../FileSystemTreeBuilderTests.cs | 12 ++++ 6 files changed, 92 insertions(+), 3 deletions(-) create mode 100644 Tests/UnitTestsParallelizable/Configuration/PopoverMenuDefaultKeyCollisionTests.cs create mode 100644 Tests/UnitTestsParallelizable/Configuration/StaticSettingsTestCollection.cs diff --git a/Terminal.Gui/FileServices/FileSystemTreeBuilder.cs b/Terminal.Gui/FileServices/FileSystemTreeBuilder.cs index 4b616a59e1..2121c0e9d2 100644 --- a/Terminal.Gui/FileServices/FileSystemTreeBuilder.cs +++ b/Terminal.Gui/FileServices/FileSystemTreeBuilder.cs @@ -80,7 +80,10 @@ internal static bool IsReparsePoint (IFileSystemInfo entry) } catch (Exception) { - return false; // treat an unreadable entry as not a reparse point / not expandable + // Fail closed: if the reparse status cannot be read, treat the entry as a reparse point so + // callers do not traverse it — an unreadable symlink/junction could otherwise recreate the + // directory-cycle risk this guard exists to prevent. + return true; } } } diff --git a/Terminal.Gui/Views/CharMap/CharMap.cs b/Terminal.Gui/Views/CharMap/CharMap.cs index 69133b56bc..86c3f8cab3 100644 --- a/Terminal.Gui/Views/CharMap/CharMap.cs +++ b/Terminal.Gui/Views/CharMap/CharMap.cs @@ -103,7 +103,9 @@ public CharMap () KeyBindings.Add (Key.PageDown, Command.PageDown); KeyBindings.Add (Key.Home, Command.Start); KeyBindings.Add (Key.End, Command.End); - KeyBindings.Add (PopoverMenu.DefaultKey, Command.Context); + // ReplaceCommands, not Add: PopoverMenu.DefaultKey is configurable and may collide with a key + // that is already bound (e.g. Ctrl+P); the user's context-menu key wins. + KeyBindings.ReplaceCommands (PopoverMenu.DefaultKey, Command.Context); MouseBindings.ReplaceCommands (MouseFlags.LeftButtonClicked, Command.Activate); MouseBindings.Add (MouseFlags.LeftButtonDoubleClicked, Command.Accept); diff --git a/Terminal.Gui/Views/FileDialogs/FileDialog.cs b/Terminal.Gui/Views/FileDialogs/FileDialog.cs index d4425cfe3f..0529ac71d6 100644 --- a/Terminal.Gui/Views/FileDialogs/FileDialog.cs +++ b/Terminal.Gui/Views/FileDialogs/FileDialog.cs @@ -231,7 +231,10 @@ internal FileDialog (IFileSystem? fileSystem) // by default, Runnable doesn't bind to Command.Context, so // we can take advantage of the CommandNotBound event to handle it _tableView.CommandNotBound += TableViewHandleCommandNotBound; - _tableView.KeyBindings.Add (PopoverMenu.DefaultKey, Command.Context); + + // ReplaceCommands, not Add: PopoverMenu.DefaultKey is configurable and may collide with a key + // TableView already binds (e.g. Ctrl+P -> Command.Up); the user's context-menu key wins. + _tableView.KeyBindings.ReplaceCommands (PopoverMenu.DefaultKey, Command.Context); _tableView.MouseBindings.Add (MouseFlags.RightButtonClicked, Command.Context); _tbPath.TextChanged += (_, _) => PathChanged (); diff --git a/Tests/UnitTestsParallelizable/Configuration/PopoverMenuDefaultKeyCollisionTests.cs b/Tests/UnitTestsParallelizable/Configuration/PopoverMenuDefaultKeyCollisionTests.cs new file mode 100644 index 0000000000..ff9277fe24 --- /dev/null +++ b/Tests/UnitTestsParallelizable/Configuration/PopoverMenuDefaultKeyCollisionTests.cs @@ -0,0 +1,58 @@ +// Claude - Opus 4.8 + +using System.IO.Abstractions.TestingHelpers; +using Terminal.Gui.Configuration; + +namespace ConfigurationTests; + +/// +/// Verifies that views binding to +/// do not throw when the configured key collides with a key the view already binds +/// (e.g. Ctrl+P, which binds to by default). +/// +[Collection ("StaticSettingsTests")] +public class PopoverMenuDefaultKeyCollisionTests +{ + [Fact] + public void FileDialog_Ctor_DoesNotThrow_WhenDefaultKeyCollidesWithTableViewBinding () + { + PopoverMenuSettings original = PopoverMenuSettings.Defaults; + + try + { + // TableView binds Ctrl+P -> Command.Up by default; the context-menu key must win without throwing. + PopoverMenuSettings.Defaults = new () { DefaultKey = Key.P.WithCtrl }; + + MockFileSystem fs = new (); + fs.AddDirectory ("/testdir"); + + using FileDialog fd = new (fs); + + Assert.NotNull (fd); + } + finally + { + PopoverMenuSettings.Defaults = original; + } + } + + [Fact] + public void CharMap_Ctor_DoesNotThrow_WhenDefaultKeyCollidesWithExistingBinding () + { + PopoverMenuSettings original = PopoverMenuSettings.Defaults; + + try + { + // CharMap binds Key.End -> Command.End before binding the context-menu key. + PopoverMenuSettings.Defaults = new () { DefaultKey = Key.End }; + + using CharMap charMap = new (); + + Assert.NotNull (charMap); + } + finally + { + PopoverMenuSettings.Defaults = original; + } + } +} diff --git a/Tests/UnitTestsParallelizable/Configuration/StaticSettingsTestCollection.cs b/Tests/UnitTestsParallelizable/Configuration/StaticSettingsTestCollection.cs new file mode 100644 index 0000000000..7bc66418f5 --- /dev/null +++ b/Tests/UnitTestsParallelizable/Configuration/StaticSettingsTestCollection.cs @@ -0,0 +1,11 @@ +namespace ConfigurationTests; + +[CollectionDefinition ("StaticSettingsTests", DisableParallelization = true)] +public class StaticSettingsTestCollection +{ + // Marker collection for tests that mutate process-wide static settings facades + // (e.g. DriverSettings.Defaults, PopoverMenuSettings.Defaults). Without + // DisableParallelization, these tests race views constructed in parallel tests — + // e.g. temporarily making PopoverMenu.DefaultKey read Ctrl+P while a FileDialog + // is being constructed elsewhere. +} diff --git a/Tests/UnitTestsParallelizable/FileServices/FileSystemTreeBuilderTests.cs b/Tests/UnitTestsParallelizable/FileServices/FileSystemTreeBuilderTests.cs index 3e916a86f4..fc0e24df17 100644 --- a/Tests/UnitTestsParallelizable/FileServices/FileSystemTreeBuilderTests.cs +++ b/Tests/UnitTestsParallelizable/FileServices/FileSystemTreeBuilderTests.cs @@ -34,4 +34,16 @@ public void GetChildren_DirectoryWithUnreadableAttributes_DoesNotThrowAndReturns Assert.Empty (children); } + + // Claude - Opus 4.8 + [Fact] + public void IsReparsePoint_WhenAttributesUnreadable_FailsClosed () + { + Mock directory = new (); + directory.SetupGet (d => d.Attributes).Throws (new UnauthorizedAccessException ("Access denied")); + + // Fail closed: an entry whose reparse status cannot be read must be treated as a reparse + // point so callers (CanExpand, GetChildren, FileDialog search) do not traverse it. + Assert.True (FileSystemTreeBuilder.IsReparsePoint (directory.Object)); + } }