From 1140cd500b48033e1bc789820b41942f48a11888 Mon Sep 17 00:00:00 2001 From: yyc12345 Date: Sun, 4 Oct 2026 16:15:01 +0800 Subject: [PATCH] fix(config): never let app.json I/O stop startup and reject numeric enum values --- .../Storage/AppData/AppConfigStoreTests.cs | 33 ++++++++++++++++ YKanBan/Storage/AppData/AppConfigStore.cs | 38 +++++++++++++------ 2 files changed, 60 insertions(+), 11 deletions(-) diff --git a/YKanBan.Tests/Storage/AppData/AppConfigStoreTests.cs b/YKanBan.Tests/Storage/AppData/AppConfigStoreTests.cs index c1a3b1f..6b02c12 100644 --- a/YKanBan.Tests/Storage/AppData/AppConfigStoreTests.cs +++ b/YKanBan.Tests/Storage/AppData/AppConfigStoreTests.cs @@ -53,6 +53,39 @@ public class AppConfigStoreTests { Assert.AreEqual(AppConfig.CurrentFormatVersion, store.Load().Version); } + [TestMethod] + public void LoadNumericEnumValueIsRejectedAndBackedUp() { + using var directory = new TempDirectory(); + string path = Path.Combine(directory.FullPath, "app.json"); + + // Integer enum values are not accepted; the file is treated like corrupt. + File.WriteAllText(path, """{ "theme": 1 }"""); + var store = new AppConfigStore(path); + + AppConfig config = store.Load(); + + Assert.AreEqual(ThemeOption.FollowSystem, config.Theme); + Assert.IsTrue(File.Exists(path + ".bak")); + } + + [TestMethod] + public void LoadUnreadableFileReturnsDefaults() { + // File sharing is only reliably enforced on Windows, so this branch is exercised there. + if (!OperatingSystem.IsWindows()) { + return; + } + + using var directory = new TempDirectory(); + string path = Path.Combine(directory.FullPath, "app.json"); + File.WriteAllText(path, """{ "version": 1 }"""); + + // Hold the file exclusively so the store's read fails with an I/O error. + using FileStream hold = File.Open(path, FileMode.Open, FileAccess.Read, FileShare.None); + var store = new AppConfigStore(path); + + Assert.AreEqual(AppConfig.CurrentFormatVersion, store.Load().Version); + } + [TestMethod] public void LoadReadsKebabCaseKeysAndEnumValues() { using var directory = new TempDirectory(); diff --git a/YKanBan/Storage/AppData/AppConfigStore.cs b/YKanBan/Storage/AppData/AppConfigStore.cs index bc7e521..b398326 100644 --- a/YKanBan/Storage/AppData/AppConfigStore.cs +++ b/YKanBan/Storage/AppData/AppConfigStore.cs @@ -4,18 +4,23 @@ using Newtonsoft.Json.Converters; namespace YKanBan.Storage.AppData; /// -/// Loads and saves app.json with graceful degradation: a missing file yields -/// defaults; a corrupt file is backed up to app.json.bak and then defaults are -/// used. Saving writes the file directly (no atomic replace) — losing the last -/// session's settings to a crash is an accepted trade-off. +/// Loads and saves app.json with graceful degradation: a missing or unreadable +/// file yields defaults; a corrupt file is backed up to app.json.bak and then +/// defaults are used. Saving writes the file directly (no atomic replace) — +/// losing the last session's settings to a crash is an accepted trade-off. /// public sealed class AppConfigStore { private static readonly JsonSerializerSettings SerializerSettings = new() { Formatting = Formatting.Indented, NullValueHandling = NullValueHandling.Ignore, - // Enums round-trip through the kebab-case strings declared via [EnumMember]. - Converters = { new StringEnumConverter() }, + // Enums round-trip through the kebab-case strings declared via [EnumMember]; + // numeric enum values are rejected like any other unknown value. + // YYC MARK: non-enum fields (bool/int/string) are still coerced by + // Newtonsoft (for example 0 or "true" to bool, "500" or 500.0 to int); + // a wrongly typed non-enum value is accepted and converted rather than + // treated as invalid. That leniency is accepted here. + Converters = { new StringEnumConverter { AllowIntegerValues = false } }, }; /// @@ -31,17 +36,24 @@ public sealed class AppConfigStore { /// /// Loads the configuration. A missing file yields defaults; a corrupt file - /// is backed up to app.json.bak and then defaults are used. + /// is backed up to app.json.bak and then defaults are used; a file that + /// exists but cannot be read (I/O or permission error) also yields defaults. /// /// The loaded configuration, or defaults when none could be read. - /// The file exists but cannot be read. public AppConfig Load() { // Missing file is a normal first-run state: run with defaults. if (!File.Exists(FilePath)) { return new AppConfig(); } - string json = File.ReadAllText(FilePath); + string json; + try { + json = File.ReadAllText(FilePath); + } catch (Exception exception) when (exception is IOException or UnauthorizedAccessException) { + // A bad app.json must not stop startup. + return new AppConfig(); + } + try { // A JSON null literal deserializes to null; treat it like defaults. AppConfig? config = JsonConvert.DeserializeObject(json, SerializerSettings); @@ -54,8 +66,12 @@ public sealed class AppConfigStore { config.Confirmations ??= new ConfirmationSettings(); return config; } catch (JsonException) { - // Corrupt: keep a copy for inspection, then continue with defaults. - File.Copy(FilePath, FilePath + ".bak", overwrite: true); + // Corrupt: keep a copy for inspection (best effort), then continue with defaults. + try { + File.Copy(FilePath, FilePath + ".bak", overwrite: true); + } catch (Exception exception) when (exception is IOException or UnauthorizedAccessException) { + // The backup is only a diagnostic aid; starting with defaults matters more. + } return new AppConfig(); } }