Skip to content

Commit d6dfbe8

Browse files
JusterZhuclaude
andcommitted
fix: harden config module against corruption, I/O failures, and silent data loss
Risks discovered in security review and their mitigations: 1. [HIGH] Invalid enum deserialization from manual JSON edits - Add AppConfig.Sanitize() + AuthCredential.Sanitize() to repair invalid AuthScheme values and null nested objects on load 2. [HIGH] Narrow exception catch in Load() — only caught JsonException - Expanded to catch IOException + UnauthorizedAccessException to handle disk-full and permission-denied scenarios gracefully 3. [HIGH] Fire-and-forget SaveAsync exceptions silently lost - Added ConfigService.SafeFireAndForgetSave() that logs failures to Trace - Updated all 12 fire-and-forget call sites in App + 4 ViewModels 4. [MEDIUM] Unnecessary disk write on every startup - OnAutoUploadEnabledChanged in PatchViewModel constructor triggered SaveAsync during init. Added _initialized guard to skip during construction 5. [LOW] Corrupted config silently overwrites backup before recovery attempt - Sanitize() is called on recovered configs before re-saving to disk Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent af494b4 commit d6dfbe8

9 files changed

Lines changed: 86 additions & 23 deletions

File tree

src/App.axaml.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,8 +69,8 @@ public override void OnFrameworkInitializationCompleted()
6969
config.WindowHeight = mainWindow.Height;
7070
}
7171

72-
// Fire-and-forget save
73-
_ = configService.SaveAsync();
72+
// Fire-and-forget save (exceptions logged to Trace, not lost)
73+
ConfigService.SafeFireAndForgetSave(configService);
7474
};
7575

7676
desktop.MainWindow = mainWindow;

src/Configuration/AppConfig.cs

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,4 +112,28 @@ public class AppConfig
112112

113113
[JsonProperty("showJsonPreview")]
114114
public bool ShowJsonPreview { get; set; } = true;
115+
116+
// ── Sanitization ───────────────────────────────────────────
117+
118+
/// <summary>
119+
/// Repair invalid values that may have been introduced by manual JSON editing
120+
/// or deserialization of an unknown/invalid enum value.
121+
/// Called automatically by <see cref="ConfigService.Load"/> after deserialization.
122+
/// </summary>
123+
internal void Sanitize()
124+
{
125+
// Repair null nested objects (should never be null, but guard against manual JSON edits)
126+
UploadAuth ??= new AuthCredential();
127+
SimulationAuth ??= new AuthCredential();
128+
129+
// Validate numeric ranges
130+
if (UploadTimeoutSeconds < 10)
131+
UploadTimeoutSeconds = 300;
132+
if (UploadRetryCount is < 0 or > 10)
133+
UploadRetryCount = 3;
134+
135+
// Repair invalid enum values in auth credentials
136+
UploadAuth.Sanitize();
137+
SimulationAuth.Sanitize();
138+
}
115139
}

src/Configuration/AuthCredential.cs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
using System;
2+
13
namespace GeneralUpdate.Tools.Configuration;
24

35
/// <summary>
@@ -35,4 +37,14 @@ public class AuthCredential
3537

3638
/// <summary>DPAPI-encrypted API key value.</summary>
3739
public string EncryptedApiKey { get; set; } = string.Empty;
40+
41+
/// <summary>
42+
/// Repair invalid enum values that may result from manual JSON editing
43+
/// or unknown future scheme values.
44+
/// </summary>
45+
internal void Sanitize()
46+
{
47+
if (!Enum.IsDefined(typeof(AuthScheme), Scheme))
48+
Scheme = AuthScheme.None;
49+
}
3850
}

src/Configuration/ConfigService.cs

Lines changed: 29 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,8 @@ public void Load()
5050
{
5151
var backupJson = File.ReadAllText(_backupPath);
5252
Config = JsonConvert.DeserializeObject<AppConfig>(backupJson, JsonSettings) ?? new AppConfig();
53-
Save(); // Restore main file from backup
53+
Config.Sanitize(); // repair invalid enum values etc.
54+
Save();
5455
return;
5556
}
5657
catch
@@ -59,7 +60,6 @@ public void Load()
5960
}
6061
}
6162

62-
// First run: save defaults so the file exists
6363
Config = new AppConfig();
6464
Save();
6565
return;
@@ -69,19 +69,21 @@ public void Load()
6969
{
7070
var json = File.ReadAllText(_configPath);
7171
Config = JsonConvert.DeserializeObject<AppConfig>(json, JsonSettings) ?? new AppConfig();
72+
Config.Sanitize();
7273

7374
// Run schema migrations
7475
Migrate();
7576
}
76-
catch (JsonException)
77+
catch (Exception ex) when (ex is JsonException or IOException or UnauthorizedAccessException)
7778
{
78-
// Config file is corrupted; try backup
79+
// Config file is corrupted or inaccessible; try backup
7980
if (File.Exists(_backupPath))
8081
{
8182
try
8283
{
8384
var backupJson = File.ReadAllText(_backupPath);
8485
Config = JsonConvert.DeserializeObject<AppConfig>(backupJson, JsonSettings) ?? new AppConfig();
86+
Config.Sanitize();
8587
Save();
8688
return;
8789
}
@@ -158,15 +160,16 @@ public async Task LoadAsync()
158160
// Run schema migrations
159161
Migrate();
160162
}
161-
catch (JsonException)
163+
catch (Exception ex) when (ex is JsonException or IOException or UnauthorizedAccessException)
162164
{
163-
// Config file is corrupted; try backup
165+
// Config file is corrupted or inaccessible; try backup
164166
if (File.Exists(_backupPath))
165167
{
166168
try
167169
{
168170
var backupJson = await File.ReadAllTextAsync(_backupPath);
169171
Config = JsonConvert.DeserializeObject<AppConfig>(backupJson, JsonSettings) ?? new AppConfig();
172+
Config.Sanitize();
170173
await SaveAsync();
171174
return;
172175
}
@@ -181,6 +184,26 @@ public async Task LoadAsync()
181184
}
182185
}
183186

187+
/// <summary>
188+
/// Fire-and-forget safe save. Logs exceptions via System.Diagnostics.Trace
189+
/// rather than losing them silently — no crash, no dialog, just a trace log.
190+
/// </summary>
191+
public static void SafeFireAndForgetSave(ConfigService service)
192+
{
193+
_ = Task.Run(async () =>
194+
{
195+
try
196+
{
197+
await service.SaveAsync();
198+
}
199+
catch (Exception ex)
200+
{
201+
System.Diagnostics.Trace.WriteLine(
202+
$"[GeneralUpdate.Tools] Config save failed: {ex.Message}");
203+
}
204+
});
205+
}
206+
184207
/// <inheritdoc />
185208
public async Task SaveAsync()
186209
{

src/ViewModels/ConfigViewModel.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ private async Task BrowseClient()
6565
{
6666
Model.ClientPath = files[0].Path.LocalPath;
6767
_config.LastConfigClientPath = files[0].Path.LocalPath;
68-
_ = ConfigServiceSingleton.Instance.SaveAsync();
68+
ConfigService.SafeFireAndForgetSave(ConfigServiceSingleton.Instance);
6969
}
7070
}
7171

@@ -88,7 +88,7 @@ private async Task BrowseUpgrade()
8888
{
8989
Model.UpgradePath = files[0].Path.LocalPath;
9090
_config.LastConfigUpgradePath = files[0].Path.LocalPath;
91-
_ = ConfigServiceSingleton.Instance.SaveAsync();
91+
ConfigService.SafeFireAndForgetSave(ConfigServiceSingleton.Instance);
9292
}
9393
}
9494

src/ViewModels/ExtensionViewModel.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ async Task SelectExt()
6464
{
6565
Config.ExtensionDirectory = p;
6666
_config.LastExtensionDir = p;
67-
_ = ConfigServiceSingleton.Instance.SaveAsync();
67+
ConfigService.SafeFireAndForgetSave(ConfigServiceSingleton.Instance);
6868
}
6969
}
7070

@@ -76,7 +76,7 @@ async Task SelectExport()
7676
{
7777
Config.ExportPath = p;
7878
_config.LastExtensionOutputDir = p;
79-
_ = ConfigServiceSingleton.Instance.SaveAsync();
79+
ConfigService.SafeFireAndForgetSave(ConfigServiceSingleton.Instance);
8080
}
8181
}
8282

src/ViewModels/MainWindowViewModel.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ private void ToggleTheme()
8080

8181
// Persist
8282
_config.Theme = IsDarkTheme ? "Dark" : "Light";
83-
_ = ConfigServiceSingleton.Instance.SaveAsync();
83+
ConfigService.SafeFireAndForgetSave(ConfigServiceSingleton.Instance);
8484
}
8585

8686
[RelayCommand]
@@ -97,7 +97,7 @@ private void ToggleLocale()
9797

9898
// Persist
9999
_config.Language = newLocale;
100-
_ = ConfigServiceSingleton.Instance.SaveAsync();
100+
ConfigService.SafeFireAndForgetSave(ConfigServiceSingleton.Instance);
101101
}
102102

103103
private static void ApplyTheme(bool isDark)

src/ViewModels/PatchViewModel.cs

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -43,13 +43,17 @@ public PatchViewModel(AppConfig config)
4343
AutoUploadEnabled = config.AutoUploadEnabled;
4444

4545
_status = _loc["Patch.Ready"];
46+
_initialized = true;
4647
}
4748

48-
/// <summary>Persist auto-upload toggle changes immediately.</summary>
49+
private bool _initialized;
50+
51+
/// <summary>Persist auto-upload toggle changes immediately (skip during construction).</summary>
4952
partial void OnAutoUploadEnabledChanged(bool value)
5053
{
54+
if (!_initialized) return;
5155
_config.AutoUploadEnabled = value;
52-
_ = ConfigServiceSingleton.Instance.SaveAsync();
56+
ConfigService.SafeFireAndForgetSave(ConfigServiceSingleton.Instance);
5357
}
5458

5559
async Task<string?> Pick()
@@ -71,7 +75,7 @@ async Task SelectOld()
7175
{
7276
Config.OldDirectory = p;
7377
_config.LastPatchOldDir = p;
74-
_ = ConfigServiceSingleton.Instance.SaveAsync();
78+
ConfigService.SafeFireAndForgetSave(ConfigServiceSingleton.Instance);
7579
L(_loc.T("Patch.OldSelected", p));
7680
}
7781
}
@@ -84,7 +88,7 @@ async Task SelectNew()
8488
{
8589
Config.NewDirectory = p;
8690
_config.LastPatchNewDir = p;
87-
_ = ConfigServiceSingleton.Instance.SaveAsync();
91+
ConfigService.SafeFireAndForgetSave(ConfigServiceSingleton.Instance);
8892
L(_loc.T("Patch.NewSelected", p));
8993
}
9094
}
@@ -97,7 +101,7 @@ async Task SelectOut()
97101
{
98102
Config.OutputPath = p;
99103
_config.LastPatchOutputDir = p;
100-
_ = ConfigServiceSingleton.Instance.SaveAsync();
104+
ConfigService.SafeFireAndForgetSave(ConfigServiceSingleton.Instance);
101105
}
102106
}
103107

@@ -118,7 +122,7 @@ async Task Build()
118122

119123
// Persist encryption scan preference
120124
_config.EncryptionScanEnabled = Config.EnableEncryptionCheck;
121-
_ = ConfigServiceSingleton.Instance.SaveAsync();
125+
ConfigService.SafeFireAndForgetSave(ConfigServiceSingleton.Instance);
122126

123127
IsBuilding = true;
124128
Log.Clear();

src/ViewModels/SimulateViewModel.cs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ async Task SelectAppDir()
9494
{
9595
Config.AppDirectory = p;
9696
_config.LastSimulateAppDir = p;
97-
_ = ConfigServiceSingleton.Instance.SaveAsync();
97+
ConfigService.SafeFireAndForgetSave(ConfigServiceSingleton.Instance);
9898
}
9999
}
100100

@@ -106,7 +106,7 @@ async Task SelectPatch()
106106
{
107107
Config.PatchFilePath = p;
108108
_config.LastSimulatePatchFile = p;
109-
_ = ConfigServiceSingleton.Instance.SaveAsync();
109+
ConfigService.SafeFireAndForgetSave(ConfigServiceSingleton.Instance);
110110
}
111111
}
112112

@@ -130,7 +130,7 @@ async Task StartSimulation()
130130
_config.SimulationServerPort = Config.ServerPort.ToString();
131131
_config.SimulationPlatformType = Config.Platform == 2 ? "Linux" : "Windows";
132132
_config.SimulationAppType = Config.AppType == 2 ? "UpgradeApp" : "ClientApp";
133-
_ = ConfigServiceSingleton.Instance.SaveAsync();
133+
ConfigService.SafeFireAndForgetSave(ConfigServiceSingleton.Instance);
134134

135135
IsRunning = true; StartButtonText = "⏳ Running..."; Log.Clear(); Status = _loc["Sim.Starting"];
136136
try

0 commit comments

Comments
 (0)