From af2bf8488fea3b6a0482974a9936b535add76fb4 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 22 Aug 2026 17:24:34 +1000 Subject: [PATCH] Do not let a hand edited key name take the tray down KeyRegister parsed the key half of a hot key with Enum.Parse, unguarded, and SettingsValidator only checked that it was non-empty. So "Key": "Ctrl+A" in settings.json passed the "Cannot start" guard, which only covers reading the file, and then threw ArgumentException out of ReBindKeys during startup - at every login, until the file was deleted by hand. Enum.Parse also reads the underlying number, so "Key": "1" silently bound the left mouse button rather than the digit. KeyName.TryParse is the one reader now. It rejects numbers and flag lists before parsing and accepts aliases, which is why it is not a round trip through ToString: Keys gives several values two names and prints the other one, so "Enter" would have been rejected. A key it cannot read leaves that hot key unbound and the tray running, and the tray says so with a balloon tip rather than only a log line - the same message covers a key already registered by another application, which was silent before. The Options form cannot produce either, offering nothing but letters. --- src/DiffEngineTray.Tests/KeyNameTests.cs | 116 ++++++++++++++++++ .../SettingsValidatorTests.cs | 22 ++++ src/DiffEngineTray/HotKey/KeyName.cs | 28 +++++ src/DiffEngineTray/HotKey/KeyRegister.cs | 10 +- src/DiffEngineTray/Program.cs | 27 ++-- .../Settings/SettingsValidator.cs | 6 + 6 files changed, 199 insertions(+), 10 deletions(-) create mode 100644 src/DiffEngineTray.Tests/KeyNameTests.cs create mode 100644 src/DiffEngineTray/HotKey/KeyName.cs diff --git a/src/DiffEngineTray.Tests/KeyNameTests.cs b/src/DiffEngineTray.Tests/KeyNameTests.cs new file mode 100644 index 00000000..0193e078 --- /dev/null +++ b/src/DiffEngineTray.Tests/KeyNameTests.cs @@ -0,0 +1,116 @@ +using Keys = System.Windows.Forms.Keys; + +/// +/// The key half of a hot key, which reaches the tray as whatever text settings.json holds. The +/// Options form only ever writes a letter, so everything rejected here arrives by hand edit. +/// +[TUnit.Core.Executors.STAThreadExecutor] +public class KeyNameTests +{ + [Test] + public async Task Reads_a_key_name() + { + var parsed = KeyName.TryParse("A", out var key); + + await Assert.That(parsed).IsTrue(); + await Assert.That(key).IsEqualTo(Keys.A); + } + + [Test] + public async Task Reads_a_key_name_in_any_case() + { + var parsed = KeyName.TryParse("f12", out var key); + + await Assert.That(parsed).IsTrue(); + await Assert.That(key).IsEqualTo(Keys.F12); + } + + /// + /// Which is why this is not a round trip through ToString: Keys gives several values + /// two names, and prints the other one. + /// + [Test] + public async Task Reads_an_alias() + { + var parsed = KeyName.TryParse("Enter", out var key); + + await Assert.That(parsed).IsTrue(); + await Assert.That(key).IsEqualTo(Keys.Return); + } + + /// + /// Enum.Parse reads the underlying number as readily as the name, so "1" used to bind the + /// left mouse button to a hot key that looked like it was for the digit. + /// + [Test] + public async Task Rejects_a_number() + { + var parsed = KeyName.TryParse("1", out var key); + + await Assert.That(parsed).IsFalse(); + await Assert.That(key).IsNotEqualTo(Keys.LButton); + } + + /// + /// The modifiers are checkboxes of their own, so a key holding them is a misunderstanding of + /// the file rather than a key. + /// + [Test] + public async Task Rejects_a_modifier_combination() => + await Assert.That(KeyName.TryParse("Ctrl+A", out _)).IsFalse(); + + [Test] + public async Task Rejects_a_flag_list() => + await Assert.That(KeyName.TryParse("A,B", out _)).IsFalse(); + + [Test] + public async Task Rejects_nothing() + { + await Assert.That(KeyName.TryParse(null, out _)).IsFalse(); + await Assert.That(KeyName.TryParse("", out _)).IsFalse(); + await Assert.That(KeyName.TryParse(" ", out _)).IsFalse(); + } + + [Test] + public async Task A_bad_key_leaves_the_hot_key_unbound() + { + using var register = new KeyRegister(0); + + // Nothing is registered with the OS for a key that is not one, so the handle above is + // never used and no hot key is taken from the machine running this + var bound = register.TryAddBinding( + KeyBindingIds.AcceptAll, + shift: true, + control: false, + alt: false, + "Ctrl+A", + () => throw new("Not bound, so never invoked")); + + await Assert.That(bound).IsFalse(); + } + + /// + /// The tray binds its hot keys at startup, so a key name it cannot read used to throw out of + /// startup and take the tray down at every login until settings.json was deleted by hand. + /// + [Test] + public async Task A_bad_key_does_not_stop_the_tray_starting() + { + var settings = new Settings + { + AcceptAllHotKey = new() + { + Control = true, + Key = "Ctrl+A" + } + }; + await using var tracker = new RecordingTracker(); + using var register = new KeyRegister(0); + var warnings = new List(); + + Program.ReBindKeys(settings, register, tracker, warnings.Add); + + await Assert.That(warnings).HasSingleItem(); + await Assert.That(warnings[0]).Contains("Ctrl+A"); + } +} diff --git a/src/DiffEngineTray.Tests/SettingsValidatorTests.cs b/src/DiffEngineTray.Tests/SettingsValidatorTests.cs index ef92f875..544040ca 100644 --- a/src/DiffEngineTray.Tests/SettingsValidatorTests.cs +++ b/src/DiffEngineTray.Tests/SettingsValidatorTests.cs @@ -45,6 +45,28 @@ public async Task Hotkey_without_key_is_invalid() await Assert.That(errors.Contains("HotKey: key is required")).IsTrue(); } + /// + /// Only a hand edit produces one, the form offering nothing but letters, and it used to pass + /// validation and then throw out of the hot key registration at every startup. + /// + [Test] + public async Task Hotkey_with_a_key_that_is_not_a_key_name_is_invalid() + { + var settings = new Settings + { + AcceptAllHotKey = new() + { + Shift = true, + Key = "Ctrl+A" + } + }; + + var valid = settings.IsValidate(out var errors); + + await Assert.That(valid).IsFalse(); + await Assert.That(errors.Contains("HotKey: 'Ctrl+A' is not a key name")).IsTrue(); + } + [Test] public async Task Valid_hotkey_passes() { diff --git a/src/DiffEngineTray/HotKey/KeyName.cs b/src/DiffEngineTray/HotKey/KeyName.cs new file mode 100644 index 00000000..a1c0b183 --- /dev/null +++ b/src/DiffEngineTray/HotKey/KeyName.cs @@ -0,0 +1,28 @@ +/// +/// Reads the key half of a . It is a name in settings.json, so it is whatever +/// a hand edit left there rather than one of the twenty six letters the Options form offers. +/// +/// Numbers and lists are rejected before parsing, because +/// reads both: "1" is , silently binding the left mouse button, and +/// "A,B" is a flag combination rather than a key. Aliases are accepted, which is why this is not +/// a round trip through - Enum.Parse takes "Enter" and +/// prints "Return". +/// +/// +static class KeyName +{ + public static bool TryParse([NotNullWhen(true)] string? name, out Keys key) + { + key = Keys.None; + + if (string.IsNullOrWhiteSpace(name) || + name.Contains(',') || + long.TryParse(name, out _)) + { + return false; + } + + return Enum.TryParse(name, true, out key) && + key != Keys.None; + } +} diff --git a/src/DiffEngineTray/HotKey/KeyRegister.cs b/src/DiffEngineTray/HotKey/KeyRegister.cs index 97a49615..553e3236 100644 --- a/src/DiffEngineTray/HotKey/KeyRegister.cs +++ b/src/DiffEngineTray/HotKey/KeyRegister.cs @@ -37,7 +37,15 @@ public bool TryAddBinding(int id, bool shift, bool control, bool alt, string key modifiers |= KeyModifiers.Alt; } - return TryAddBinding(id, modifiers, Enum.Parse(key, true), action); + if (!KeyName.TryParse(key, out var keys)) + { + // Unbound rather than thrown. This runs at startup for every configured hot key, and + // a hand edited settings.json used to take the tray down at every login + Log.Error("'{Key}' is not a key name. The hot key was not bound.", key); + return false; + } + + return TryAddBinding(id, modifiers, keys, action); } public bool TryAddBinding(int id, KeyModifiers modifiers, Keys keys, Action action) diff --git a/src/DiffEngineTray/Program.cs b/src/DiffEngineTray/Program.cs index c5684f14..3cdd7717 100644 --- a/src/DiffEngineTray/Program.cs +++ b/src/DiffEngineTray/Program.cs @@ -97,7 +97,7 @@ void Warn(string message) => var task = StartServer(tracker, cancel); using var keyRegister = new KeyRegister(icon.Handle()); - ReBindKeys(settings, keyRegister, tracker); + ReBindKeys(settings, keyRegister, tracker, Warn); var menuStrip = MenuBuilder.Build( Application.Exit, @@ -133,18 +133,27 @@ void Warn(string message) => [UnsafeAccessor(UnsafeAccessorKind.Method, Name = "ShowContextMenu")] static extern void ShowContextMenu(NotifyIcon icon); - internal static void ReBindKeys(Settings settings, KeyRegister keyRegister, Tracker tracker) + internal static void ReBindKeys(Settings settings, KeyRegister keyRegister, Tracker tracker, Action? warn = null) { foreach (var binding in BuildKeyBindings(settings, tracker)) { var hotKey = binding.HotKey; - keyRegister.TryAddBinding( - binding.Id, - hotKey.Shift, - hotKey.Control, - hotKey.Alt, - hotKey.Key, - binding.Action); + if (keyRegister.TryAddBinding( + binding.Id, + hotKey.Shift, + hotKey.Control, + hotKey.Alt, + hotKey.Key, + binding.Action)) + { + continue; + } + + // Said out loud, because the alternative is a hot key that quietly does nothing. Only + // settings.json can produce a key name the Options form cannot, so only a hand edit + // reaches the first half of this + warn?.Invoke( + $"Could not bind the hot key '{hotKey.Key}'. It is either not a key name, or already registered by another application."); } } diff --git a/src/DiffEngineTray/Settings/SettingsValidator.cs b/src/DiffEngineTray/Settings/SettingsValidator.cs index 447bc023..322ab1d9 100644 --- a/src/DiffEngineTray/Settings/SettingsValidator.cs +++ b/src/DiffEngineTray/Settings/SettingsValidator.cs @@ -26,6 +26,12 @@ static void ValidateHotKey(List errors, HotKey? hotKey) if (string.IsNullOrWhiteSpace(hotKey.Key)) { errors.Add("HotKey: key is required"); + return; + } + + if (!KeyName.TryParse(hotKey.Key, out _)) + { + errors.Add($"HotKey: '{hotKey.Key}' is not a key name"); } } } \ No newline at end of file