From 1dc6ca2ab27903e11bbfada1c8354693340f5058 Mon Sep 17 00:00:00 2001 From: Daniel Chalmers Date: Thu, 11 Jun 2026 11:56:27 -0500 Subject: [PATCH] Harden settings load and save safety This keeps the settings PR focused on direct data-loss prevention. Startup no longer saves the loaded settings object just to determine whether settings can be saved; it now loads settings and probes directory writability with a throwaway file. When an existing primary settings file is malformed, empty, or unreadable, the load path copies it to a timestamped .corrupt-* snapshot before recovered default settings are allowed to save over the primary path. If the unreadable primary cannot be copied aside, saving is disabled for the run so fallback defaults do not overwrite the only recoverable copy. Settings writes now go through a temporary file in the same directory and then replace or move into the primary path. That reduces the chance that a shutdown, installer interruption, or write failure leaves a truncated settings file behind. The tests cover malformed and empty primary files, locked unreadable files, startup probing without rewriting the real file, atomic save cleanup, and property-level deserialization errors preserving unrelated valid settings. The intentionally deferred pieces are installer behavior, graceful close/exit save, settings relocation, and last-known-good automatic restore. --- .../Settings/SettingsSerializationTests.cs | 115 +++++++ RadialActions/Properties/Settings.Engine.cs | 293 ++++++++++++++++-- 2 files changed, 383 insertions(+), 25 deletions(-) diff --git a/RadialActions.Tests/Settings/SettingsSerializationTests.cs b/RadialActions.Tests/Settings/SettingsSerializationTests.cs index bb552eb..29f73bb 100644 --- a/RadialActions.Tests/Settings/SettingsSerializationTests.cs +++ b/RadialActions.Tests/Settings/SettingsSerializationTests.cs @@ -109,4 +109,119 @@ public void DeserializeFromJson_MissingIsEnabled_DefaultsToTrue() Assert.Single(settings.Actions); Assert.True(settings.Actions[0].IsEnabled); } + + [Fact] + public void Load_MalformedPrimary_PreservesCorruptPrimaryBeforeDefaultsCanSave() + { + var directory = CreateTempDirectory(); + var settingsPath = Path.Combine(directory, "RadialActions.settings"); + File.WriteAllText(settingsPath, "{"); + + var settings = Settings.LoadAndInitializePersistenceState(settingsPath, out var canBeSaved); + + Assert.True(canBeSaved); + Assert.Equal(Settings.DefaultActivationHotkey, settings.ActivationHotkey); + Assert.Equal(Settings.DefaultSize, settings.Size); + Assert.Equal("{", File.ReadAllText(settingsPath)); + + var corruptPath = Assert.Single(Directory.GetFiles(directory, "RadialActions.settings.corrupt-*")); + Assert.Equal("{", File.ReadAllText(corruptPath)); + } + + [Fact] + public void Load_EmptyExistingFile_IsTreatedAsCorruptionNotFreshInstall() + { + var directory = CreateTempDirectory(); + var settingsPath = Path.Combine(directory, "RadialActions.settings"); + File.WriteAllText(settingsPath, string.Empty); + + var settings = Settings.LoadFromFileWithRecovery(settingsPath); + + Assert.Equal(Settings.DefaultActivationHotkey, settings.ActivationHotkey); + Assert.Equal(Settings.DefaultSize, settings.Size); + Assert.True(File.Exists(settingsPath)); + Assert.Single(Directory.GetFiles(directory, "RadialActions.settings.corrupt-*")); + } + + [Fact] + public void StartupWritableProbe_DoesNotRewriteSettingsFile() + { + var directory = CreateTempDirectory(); + var settingsPath = Path.Combine(directory, "RadialActions.settings"); + const string json = """ + { + "ActivationHotkey": "Ctrl+Alt+P", + "Size": 480 + } + """; + File.WriteAllText(settingsPath, json); + var lastWriteTime = new DateTime(2025, 1, 2, 3, 4, 5, DateTimeKind.Utc); + File.SetLastWriteTimeUtc(settingsPath, lastWriteTime); + + var settings = Settings.LoadAndInitializePersistenceState(settingsPath, out var canBeSaved); + + Assert.True(canBeSaved); + Assert.Equal("Ctrl+Alt+P", settings.ActivationHotkey); + Assert.Equal(json, File.ReadAllText(settingsPath)); + Assert.Equal(lastWriteTime, File.GetLastWriteTimeUtc(settingsPath)); + } + + [Fact] + public void Load_LockedUnreadablePrimary_DisablesSavingRecoveredDefaults() + { + var directory = CreateTempDirectory(); + var settingsPath = Path.Combine(directory, "RadialActions.settings"); + File.WriteAllText(settingsPath, "{"); + + using var lockedSettingsFile = new FileStream(settingsPath, FileMode.Open, FileAccess.ReadWrite, FileShare.None); + + var settings = Settings.LoadAndInitializePersistenceState(settingsPath, out var canBeSaved); + + Assert.False(canBeSaved); + Assert.Equal(Settings.DefaultActivationHotkey, settings.ActivationHotkey); + Assert.Empty(Directory.GetFiles(directory, "RadialActions.settings.corrupt-*")); + } + + [Fact] + public void Save_WritesAtomicallyAndLeavesNoTemporaryFileAfterSuccess() + { + var directory = CreateTempDirectory(); + var settingsPath = Path.Combine(directory, "RadialActions.settings"); + var settings = Settings.DeserializeFromJson("{}"); + settings.ActivationHotkey = "Ctrl+Shift+S"; + settings.Size = 640; + + var saved = settings.SaveToFile(settingsPath); + + Assert.True(saved); + Assert.Contains("Ctrl+Shift+S", File.ReadAllText(settingsPath)); + Assert.Empty(Directory.GetFiles(directory, "*.tmp")); + } + + [Fact] + public void Deserialize_PropertyLevelErrors_NormalizeWithoutDroppingUnrelatedValidSettings() + { + const string json = """ + { + "ActivationHotkey": "Ctrl+Alt+L", + "Size": 420, + "Actions": "not a collection", + "OpenMenuInScreenCenter": true + } + """; + + var settings = Settings.DeserializeFromJson(json); + + Assert.Equal("Ctrl+Alt+L", settings.ActivationHotkey); + Assert.Equal(420, settings.Size); + Assert.True(settings.OpenMenuInScreenCenter); + Assert.Equal(Settings.CreateDefaultActions().Count, settings.Actions.Count); + } + + private static string CreateTempDirectory() + { + var path = Path.Combine(Path.GetTempPath(), "RadialActions.Tests", Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(path); + return path; + } } diff --git a/RadialActions/Properties/Settings.Engine.cs b/RadialActions/Properties/Settings.Engine.cs index a7b67ce..4a0c314 100644 --- a/RadialActions/Properties/Settings.Engine.cs +++ b/RadialActions/Properties/Settings.Engine.cs @@ -1,4 +1,5 @@ -using System.IO; +using System.Globalization; +using System.IO; using CommunityToolkit.Mvvm.ComponentModel; using Newtonsoft.Json; @@ -6,7 +7,16 @@ namespace RadialActions.Properties; public sealed partial class Settings : ObservableObject { - private static readonly Lazy _default = new(LoadAndAttemptSave); + private static readonly Lazy _default = new(LoadAndInitializePersistenceState); + + /// + /// Carries loaded settings plus whether future saves are allowed to write back to the same path. + /// + /// The settings object to use for the current app session. + /// + /// false when the primary settings file existed but could not be preserved before recovery. + /// + private sealed record SettingsLoadResult(Settings Settings, bool CanSaveSettings); private static readonly JsonSerializerSettings _jsonSerializerSettings = new() { @@ -60,9 +70,20 @@ private Settings() { } /// /// Saves to the default path in JSON format. /// + /// true when the default settings file was written successfully; otherwise, false. public bool Save() { - Log.Information($"Saving to {FilePath}"); + return SaveToFile(FilePath); + } + + /// + /// Saves this settings object to a specific path using an atomic file replacement. + /// + /// The primary settings file path to write. + /// true when the primary file was written successfully; otherwise, false. + internal bool SaveToFile(string filePath) + { + Log.Information("Saving to {FilePath}", filePath); try { @@ -73,18 +94,17 @@ public bool Save() { try { - File.WriteAllText(FilePath, json); + WriteAllTextAtomically(filePath, json); return true; } - catch + catch (Exception ex) { - // Wait before next attempt to read. - Log.Debug("Couldn't save file; Waiting a bit"); + Log.Debug(ex, "Couldn't save file; waiting a bit"); Thread.Sleep(250); } } } - catch (JsonSerializationException ex) + catch (JsonException ex) { Log.Error(ex, "Failed to save settings"); } @@ -92,11 +112,20 @@ public bool Save() return false; } + /// + /// Serializes this settings object to the app's JSON settings format. + /// + /// The formatted JSON settings document. public string SerializeToJson() { return JsonConvert.SerializeObject(this, _jsonSerializerSettings); } + /// + /// Deserializes a settings document and normalizes missing or invalid values. + /// + /// The JSON settings content to deserialize. + /// A normalized settings object. public static Settings DeserializeFromJson(string json) { var settings = new Settings(); @@ -110,46 +139,260 @@ public static Settings DeserializeFromJson(string json) } /// - /// Reads settings from the default path. + /// Reads settings from the specified path. /// - private static string ReadJsonFromFile() + /// The settings file path to read. + /// The complete JSON file content. + private static string ReadJsonFromFile(string filePath) { - using var fileStream = new FileStream(FilePath, FileMode.Open, FileAccess.Read, FileShare.ReadWrite); + using var fileStream = new FileStream(filePath, FileMode.Open, FileAccess.Read, FileShare.ReadWrite); using var streamReader = new StreamReader(fileStream); return streamReader.ReadToEnd(); } /// - /// Loads from the default path in JSON format. + /// Loads settings from the specified path with recovery behavior for unreadable primary files. + /// + /// The primary settings file path. + /// The settings object to use for the current app session. + internal static Settings LoadFromFileWithRecovery(string filePath) + => LoadFromFileWithRecoveryResult(filePath).Settings; + + /// + /// Loads settings and records whether it is safe to save back to the primary path later. /// - private static Settings LoadFromFile() + /// The primary settings file path. + /// + /// A load result containing settings plus a save gate that protects unreadable files that could not be preserved. + /// + private static SettingsLoadResult LoadFromFileWithRecoveryResult(string filePath) { + if (!File.Exists(filePath)) + { + Log.Information("Creating new settings because {FilePath} does not exist", filePath); + return new SettingsLoadResult(CreateDefaultSettings(), CanSaveSettings: true); + } + try { - var json = ReadJsonFromFile(); - return DeserializeFromJson(json); + var settings = LoadSettingsFileOrThrow(filePath); + return new SettingsLoadResult(settings, CanSaveSettings: true); } catch (Exception ex) { - Log.Error(ex, $"Failed to load {FilePath}"); - Log.Information("Creating new settings"); - var settings = new Settings(); - settings.NormalizeAfterLoad(); - return settings; + return RecoverFromPrimaryLoadFailure(filePath, ex); } } /// - /// Loads from the default path in JSON format then attempts to save in order to check if it can be done. + /// Loads from the default path in JSON format and probes whether the directory is writable. + /// + /// The singleton settings object for the default settings file path. + private static Settings LoadAndInitializePersistenceState() + { + var settings = LoadAndInitializePersistenceState(FilePath, out var canBeSaved); + CanBeSaved = canBeSaved; + Log.Debug("Settings can be saved: {CanBeSaved}", CanBeSaved); + + return settings; + } + + /// + /// Loads settings from a specific path and determines whether later saves should be enabled. + /// + /// The primary settings file path. + /// + /// Set to true only when recovery did not need to suppress saves and the directory is writable. + /// + /// The settings object to use for the current app session. + internal static Settings LoadAndInitializePersistenceState(string filePath, out bool canBeSaved) + { + var loadResult = LoadFromFileWithRecoveryResult(filePath); + canBeSaved = loadResult.CanSaveSettings && ProbeSettingsDirectoryWritable(filePath); + return loadResult.Settings; + } + + /// + /// Checks whether the settings directory can be written without modifying the real settings file. /// - private static Settings LoadAndAttemptSave() + /// The primary settings file path whose directory should be probed. + /// true when a throwaway probe file can be created and deleted; otherwise, false. + internal static bool ProbeSettingsDirectoryWritable(string filePath) { - var settings = LoadFromFile(); + var directoryPath = Path.GetDirectoryName(filePath); + if (string.IsNullOrWhiteSpace(directoryPath)) + { + directoryPath = "."; + } - CanBeSaved = settings.Save(); - Log.Debug($"Settings can be saved: {CanBeSaved}"); + var probePath = Path.Combine(directoryPath, $".settings-write-probe-{Guid.NewGuid():N}.tmp"); + try + { + Directory.CreateDirectory(directoryPath); + File.WriteAllText(probePath, string.Empty); + File.Delete(probePath); + return true; + } + catch (Exception ex) + { + Log.Warning(ex, "Settings directory is not writable"); + return false; + } + finally + { + TryDeleteFile(probePath); + } + } + + /// + /// Creates a default settings object and runs the normal post-load normalization pass. + /// + /// A normalized default settings object. + private static Settings CreateDefaultSettings() + { + var settings = new Settings(); + settings.NormalizeAfterLoad(); return settings; } + /// + /// Reads and deserializes one settings file, treating an empty existing file as corruption. + /// + /// The settings file path to load. + /// A normalized settings object. + /// Thrown when the file exists but contains no JSON content. + private static Settings LoadSettingsFileOrThrow(string filePath) + { + var json = ReadJsonFromFile(filePath); + if (string.IsNullOrWhiteSpace(json)) + { + throw new InvalidDataException($"Settings file is empty: {filePath}"); + } + + return DeserializeFromJson(json); + } + + /// + /// Recovers from a failed primary settings load without silently overwriting recoverable user data. + /// + /// The primary settings file path that failed to load. + /// The exception raised while loading the primary file. + /// + /// Recovered settings and a save gate. Saves are disabled when the unreadable primary file could not be copied aside. + /// + private static SettingsLoadResult RecoverFromPrimaryLoadFailure(string filePath, Exception loadException) + { + Log.Error(loadException, "Failed to load {FilePath}", filePath); + + var canSaveDefaults = PreserveCorruptSettingsFile(filePath); + if (!canSaveDefaults) + { + Log.Warning("Default settings will not be saved automatically because the unreadable primary file could not be preserved"); + } + + Log.Information("Creating new settings in memory after unreadable settings file"); + return new SettingsLoadResult(CreateDefaultSettings(), canSaveDefaults); + } + + /// + /// Copies an unreadable primary settings file to a timestamped corrupt snapshot path. + /// + /// The primary settings file path to preserve. + /// + /// true when no primary file exists or the copy succeeds; false when recovery should avoid saving. + /// + private static bool PreserveCorruptSettingsFile(string filePath) + { + if (!File.Exists(filePath)) + return true; + + var corruptPath = CreateCorruptFilePath(filePath); + + try + { + using var source = new FileStream(filePath, FileMode.Open, FileAccess.Read, FileShare.ReadWrite | FileShare.Delete); + using var destination = new FileStream(corruptPath, FileMode.CreateNew, FileAccess.Write, FileShare.None); + source.CopyTo(destination); + Log.Warning("Preserved unreadable settings file at {CorruptPath}", corruptPath); + return true; + } + catch (Exception ex) + { + Log.Warning(ex, "Failed to preserve unreadable settings file {FilePath}", filePath); + return false; + } + } + + /// + /// Creates a unique timestamped corrupt snapshot path for a settings file. + /// + /// The primary settings file path. + /// A non-existing path using the `.corrupt-*` naming pattern. + private static string CreateCorruptFilePath(string filePath) + { + var timestamp = DateTimeOffset.UtcNow.ToString("yyyyMMddHHmmss", CultureInfo.InvariantCulture); + for (var attempt = 0; ; attempt++) + { + var suffix = attempt == 0 ? string.Empty : $"-{attempt}"; + var corruptPath = $"{filePath}.corrupt-{timestamp}{suffix}"; + if (!File.Exists(corruptPath)) + { + return corruptPath; + } + } + } + + /// + /// Writes text through a same-directory temporary file and atomically replaces the destination when possible. + /// + /// The destination file path. + /// The complete content to write. + private static void WriteAllTextAtomically(string filePath, string contents) + { + var directoryPath = Path.GetDirectoryName(filePath); + if (string.IsNullOrWhiteSpace(directoryPath)) + { + directoryPath = "."; + } + + Directory.CreateDirectory(directoryPath); + var tempFilePath = Path.Combine(directoryPath, $"{Path.GetFileName(filePath)}.{Guid.NewGuid():N}.tmp"); + + try + { + File.WriteAllText(tempFilePath, contents); + if (File.Exists(filePath)) + { + File.Replace(tempFilePath, filePath, null); + } + else + { + File.Move(tempFilePath, filePath); + } + } + finally + { + TryDeleteFile(tempFilePath); + } + } + + /// + /// Deletes a file when it exists, logging and ignoring cleanup failures. + /// + /// The file path to delete. + private static void TryDeleteFile(string filePath) + { + try + { + if (File.Exists(filePath)) + { + File.Delete(filePath); + } + } + catch (Exception ex) + { + Log.Debug(ex, "Failed to delete temporary file {FilePath}", filePath); + } + } }