fix: critical and high-priority audit fixes (P0+P1) for 1.18.1#614
Merged
Conversation
- PrivacyView: remove auto-apply on toggle flip; require explicit Apply/Discard with pending-change counter - DashboardViewModel: replace sync-over-async LoadStaticInfo with async path so first paint never blocks UI - DnsHostsViewModel + HostsFileService: switch to ReadHostsAsync + File.ReadAllLinesAsync - IconExtractorService: replace random-eviction (Take on ConcurrentDictionary.Keys) with insertion-order Queue for true FIFO - SpeedTestService: drop Task.Result after WhenAll in favor of awaited reads - PerformanceService.TakeSnapshotAsync: document caller contract (must run before any Apply) - README + CHANGELOG: reflect new Apply/Discard UX, version 1.18.1
📝 WalkthroughWalkthroughVersion 1.18.1 introduces a pending-changes workflow for privacy toggles (staging updates before registry write), upgrades UI-blocking I/O in Dashboard and DNS/Hosts tabs to asynchronous operations, improves icon cache eviction to FIFO ordering, fixes unsafe async task result access in SpeedTest, and documents PerformanceService snapshot ordering requirements. Changesv1.18.1 Release
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| HostEntries.ReplaceWith(entries); | ||
| HostsStatus = $"Loaded {entries.Count} entries."; | ||
| } | ||
| catch (OperationCanceledException) { } |
Comment on lines
+94
to
+96
| foreach (var t in Toggles) | ||
| if (_baselineStates.TryGetValue(t, out var baseline) && baseline != t.IsEnabled) | ||
| pending++; |
Comment on lines
+134
to
+136
| foreach (var t in Toggles) | ||
| if (_baselineStates.TryGetValue(t, out var baseline)) | ||
| t.IsEnabled = baseline; |
- ThemePopup, TemperatureService, WindowsUpdateService (ExtractKbIds + ClassifyCategory): empty catch{} replaced with typed catch + Debug log
- DeepCleanupService: log file/directory delete failures at Debug level in addition to error list
- AdminHelper.RelaunchAsAdmin: distinguish UAC decline (Win32 1223) from real Win32 errors; log InvalidOperationException instead of swallowing
- IconExtractorService.SHGetFileInfo: add SetLastError = true so callers can inspect Win32 error
- PerformanceService.CreateRestorePointAsync: rename misleading BUG-003 marker to a NOTE explaining the PowerShell AddParameter limitation
- App.xaml.cs OnDispatcherUnhandled: document why MessageBox is used instead of DialogService
- .gitignore: add .kiro/, .session-notes/, notes-local.md, scratch.md to prevent accidental tracking of developer notes
laurentiu021
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@SysManager/SysManager/Services/HostsFileService.cs`:
- Around line 30-35: The current ReadHostsAsync method uses
File.Exists(HostsPath) which can hide real IO/permission errors; instead remove
the preflight check and wrap the File.ReadAllLinesAsync(HostsPath, ct) call in a
try/catch that only catches FileNotFoundException and DirectoryNotFoundException
and returns an empty List<HostsEntry> for those cases, while allowing other
exceptions (e.g., UnauthorizedAccessException/IOException) to propagate so
DnsHostsViewModel.LoadHostsAsync can handle them; keep using the existing
HostsPath and ct parameters and ConfigureAwait(false) as before.
In `@SysManager/SysManager/Services/IconExtractorService.cs`:
- Around line 26-27: ClearCache() currently only clears the _cache but leaves
the FIFO _insertionOrder queue populated, causing stale entries and growth;
update ClearCache() to also clear _insertionOrder in the same critical section
(use the existing _evictionLock) so both _cache.Clear() and
_insertionOrder.Clear() are done under lock to maintain thread-safety and
consistent state for future evictions.
In `@SysManager/SysManager/ViewModels/PrivacyViewModel.cs`:
- Around line 116-128: The code currently updates _baselineStates and clears
pending changes immediately after calling _service.ApplyAll(changed); instead,
after calling PrivacyService.ApplyAll(changed) (or ApplyToggle), reload the
persisted toggle states from the service/registry and rebuild _baselineStates
from those reloaded values before updating _baselineStates and calling
RecomputePendingChanges(); this ensures toggles that failed to persist (since
ApplyAll is void and only logs failures) remain as pending and the
StatusMessage/Log reflect the actual persisted count.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d7871cc4-2822-4320-8a6d-f944b10a7e00
📒 Files selected for processing (12)
CHANGELOG.mdREADME.mdSysManager/SysManager.Tests/PrivacyViewModelTests.csSysManager/SysManager/Services/HostsFileService.csSysManager/SysManager/Services/IconExtractorService.csSysManager/SysManager/Services/PerformanceService.csSysManager/SysManager/Services/SpeedTestService.csSysManager/SysManager/SysManager.csprojSysManager/SysManager/ViewModels/DashboardViewModel.csSysManager/SysManager/ViewModels/DnsHostsViewModel.csSysManager/SysManager/ViewModels/PrivacyViewModel.csSysManager/SysManager/Views/PrivacyView.xaml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build & unit tests
- GitHub Check: Analyze (csharp)
🔇 Additional comments (10)
SysManager/SysManager/SysManager.csproj (1)
13-15: LGTM!CHANGELOG.md (1)
9-20: LGTM!README.md (1)
238-240: LGTM!SysManager/SysManager/Views/PrivacyView.xaml (1)
62-72: LGTM!SysManager/SysManager.Tests/PrivacyViewModelTests.cs (1)
66-77: LGTM!Also applies to: 81-89, 92-118, 122-136
SysManager/SysManager/Services/SpeedTestService.cs (1)
222-223: LGTM!SysManager/SysManager/Services/PerformanceService.cs (1)
80-87: LGTM!SysManager/SysManager/Services/IconExtractorService.cs (1)
50-64: LGTM!SysManager/SysManager/ViewModels/DashboardViewModel.cs (2)
91-99: LGTM!Also applies to: 675-689
186-205: LGTM!
Comment on lines
+30
to
+35
| public async Task<List<HostsEntry>> ReadHostsAsync(CancellationToken ct = default) | ||
| { | ||
| List<HostsEntry> entries = []; | ||
| if (!File.Exists(HostsPath)) return entries; | ||
|
|
||
| foreach (string rawLine in File.ReadAllLines(HostsPath)) | ||
| var lines = await File.ReadAllLinesAsync(HostsPath, ct).ConfigureAwait(false); |
There was a problem hiding this comment.
Don’t preflight with File.Exists here.
Line 33 can silently collapse read failures into “missing file.” DnsHostsViewModel.LoadHostsAsync explicitly handles UnauthorizedAccessException/IOException, but File.Exists(HostsPath) returns false for some access/path errors, so the UI can incorrectly report Loaded 0 entries. instead of surfacing the failure.
Treat only FileNotFoundException/DirectoryNotFoundException as the empty-list case and let real read errors propagate.
Suggested fix
public async Task<List<HostsEntry>> ReadHostsAsync(CancellationToken ct = default)
{
List<HostsEntry> entries = [];
- if (!File.Exists(HostsPath)) return entries;
-
- var lines = await File.ReadAllLinesAsync(HostsPath, ct).ConfigureAwait(false);
+ string[] lines;
+ try
+ {
+ lines = await File.ReadAllLinesAsync(HostsPath, ct).ConfigureAwait(false);
+ }
+ catch (FileNotFoundException)
+ {
+ return entries;
+ }
+ catch (DirectoryNotFoundException)
+ {
+ return entries;
+ }
+
foreach (string rawLine in lines)
{
string line = rawLine.Trim();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public async Task<List<HostsEntry>> ReadHostsAsync(CancellationToken ct = default) | |
| { | |
| List<HostsEntry> entries = []; | |
| if (!File.Exists(HostsPath)) return entries; | |
| foreach (string rawLine in File.ReadAllLines(HostsPath)) | |
| var lines = await File.ReadAllLinesAsync(HostsPath, ct).ConfigureAwait(false); | |
| public async Task<List<HostsEntry>> ReadHostsAsync(CancellationToken ct = default) | |
| { | |
| List<HostsEntry> entries = []; | |
| string[] lines; | |
| try | |
| { | |
| lines = await File.ReadAllLinesAsync(HostsPath, ct).ConfigureAwait(false); | |
| } | |
| catch (FileNotFoundException) | |
| { | |
| return entries; | |
| } | |
| catch (DirectoryNotFoundException) | |
| { | |
| return entries; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SysManager/SysManager/Services/HostsFileService.cs` around lines 30 - 35, The
current ReadHostsAsync method uses File.Exists(HostsPath) which can hide real
IO/permission errors; instead remove the preflight check and wrap the
File.ReadAllLinesAsync(HostsPath, ct) call in a try/catch that only catches
FileNotFoundException and DirectoryNotFoundException and returns an empty
List<HostsEntry> for those cases, while allowing other exceptions (e.g.,
UnauthorizedAccessException/IOException) to propagate so
DnsHostsViewModel.LoadHostsAsync can handle them; keep using the existing
HostsPath and ct parameters and ConfigureAwait(false) as before.
Comment on lines
+26
to
27
| private static readonly Queue<string> _insertionOrder = new(); | ||
| private static readonly object _evictionLock = new(); |
There was a problem hiding this comment.
Clear cache must also reset FIFO queue state.
After adding _insertionOrder, ClearCache() (Line 157) only clears _cache. That leaves stale keys in the queue, which can grow unbounded across clears and force extra stale dequeues during later evictions.
🔧 Proposed fix
- public static void ClearCache() => _cache.Clear();
+ public static void ClearCache()
+ {
+ lock (_evictionLock)
+ {
+ _cache.Clear();
+ _insertionOrder.Clear();
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SysManager/SysManager/Services/IconExtractorService.cs` around lines 26 - 27,
ClearCache() currently only clears the _cache but leaves the FIFO
_insertionOrder queue populated, causing stale entries and growth; update
ClearCache() to also clear _insertionOrder in the same critical section (use the
existing _evictionLock) so both _cache.Clear() and _insertionOrder.Clear() are
done under lock to maintain thread-safety and consistent state for future
evictions.
Comment on lines
+116
to
+128
| var changed = Toggles | ||
| .Where(t => _baselineStates.TryGetValue(t, out var baseline) && baseline != t.IsEnabled) | ||
| .ToList(); | ||
|
|
||
| _service.ApplyAll(changed); | ||
|
|
||
| // Refresh baseline to the just-applied state. | ||
| foreach (var t in changed) | ||
| _baselineStates[t] = t.IsEnabled; | ||
| RecomputePendingChanges(); | ||
|
|
||
| StatusMessage = $"Applied {changed.Count} change{(changed.Count == 1 ? "" : "s")}."; | ||
| Log.Information("Privacy: applied {Count} pending changes", changed.Count); |
There was a problem hiding this comment.
Don’t clear pending state unless the registry write actually succeeded.
ApplyChanges() currently marks every changed toggle as the new baseline immediately after _service.ApplyAll(changed), but PrivacyService.ApplyAll()/ApplyToggle() never reports failures back to the caller. In the current service contract, unauthorized/security failures are only logged, so Line 123-Line 125 can silently clear pending changes and show a false “Applied N changes” status even when some writes never reached the registry. This breaks the new staged workflow by losing the user’s unsaved diffs after a partial failure.
Suggested direction
- _service.ApplyAll(changed);
-
- // Refresh baseline to the just-applied state.
- foreach (var t in changed)
- _baselineStates[t] = t.IsEnabled;
- RecomputePendingChanges();
-
- StatusMessage = $"Applied {changed.Count} change{(changed.Count == 1 ? "" : "s")}.";
- Log.Information("Privacy: applied {Count} pending changes", changed.Count);
+ // Prefer a service contract that returns the toggles that were
+ // actually persisted, or reload from the registry before advancing
+ // the baseline.
+ var applied = _service.ApplyAll(changed); // e.g. IReadOnlyList<PrivacyToggle>
+
+ foreach (var t in applied)
+ _baselineStates[t] = t.IsEnabled;
+
+ RecomputePendingChanges();
+ StatusMessage = applied.Count == changed.Count
+ ? $"Applied {applied.Count} change{(applied.Count == 1 ? "" : "s")}."
+ : $"Applied {applied.Count} of {changed.Count} pending changes.";If you want to keep PrivacyService.ApplyAll() void, the safer fallback is to reload the toggles from the registry after the write attempt and rebuild _baselineStates from the persisted values before clearing the pending count.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SysManager/SysManager/ViewModels/PrivacyViewModel.cs` around lines 116 - 128,
The code currently updates _baselineStates and clears pending changes
immediately after calling _service.ApplyAll(changed); instead, after calling
PrivacyService.ApplyAll(changed) (or ApplyToggle), reload the persisted toggle
states from the service/registry and rebuild _baselineStates from those reloaded
values before updating _baselineStates and calling RecomputePendingChanges();
this ensures toggles that failed to persist (since ApplyAll is void and only
logs failures) remain as pending and the StatusMessage/Log reflect the actual
persisted count.
Comment on lines
+356
to
+360
| catch (Exception ex) | ||
| { | ||
| errors.Add($"{file}: {ex.Message}"); | ||
| Log.Debug(ex, "Deep cleanup: failed to delete file {File}", file); | ||
| } |
Comment on lines
+367
to
+371
| catch (Exception ex) | ||
| { | ||
| errors.Add($"{path}: {ex.Message}"); | ||
| Log.Debug(ex, "Deep cleanup: failed to enumerate path {Path}", path); | ||
| } |
| { | ||
| try { computer?.Close(); } | ||
| catch { /* dispose errors — ignore */ } | ||
| catch (Exception ex) { Log.Debug(ex, "LibreHardwareMonitor close failed"); } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Round 1 of the post-1.18.0 audit fixes. Six P0/critical issues identified by the multi-agent audit are addressed here in a single bundled fix PR. All changes are bug fixes — no new features and no behavior change beyond the Privacy toggle flow (documented below).
What's fixed
Privacy — no more silent registry writes (CRITICAL)
Before: every flip of a privacy toggle wrote to the registry instantly via a hidden
PropertyChangedhandler. Users could change system-wide privacy settings just by scrolling and accidentally clicking a switch.After: flipping a toggle stages the change locally. The toolbar now exposes:
N pending changes — press Apply).Both buttons disable when no changes are pending. The Apply button uses the primary style.
Dashboard — first load no longer freezes the UI (CRITICAL)
LoadStaticInfocalled_sys.CaptureAsync().GetAwaiter().GetResult()on the UI thread inside the constructor'sInitAsync. With WMI/CIM queries running synchronously, the Dashboard tab visibly stalled on open. Replaced with a proper async path (LoadStaticInfoAsync→await _sys.CaptureAsync()).DNS / Hosts — async file I/O (CRITICAL)
HostsFileService.ReadHostsusedFile.ReadAllLinessynchronously. Renamed toReadHostsAsyncusingFile.ReadAllLinesAsync; the VM'sLoadHostsAsyncand the newRefreshHostsAsynccommand flow through awaited calls.Icon cache — proper FIFO eviction (CRITICAL)
IconExtractorServiceevicted entries via_cache.Keys.Take(_cache.Count / 2)— butConcurrentDictionary.Keysdoes not preserve insertion order, so frequently-used icons could be dropped while stale ones survived. Eviction now uses an explicitQueue<string>to track insertion order; the oldest entries are removed first when the cache exceedsMaxCacheSize.SpeedTest — clean await pattern (CRITICAL)
After
Task.WhenAll(stdoutTask, stderrTask), the service then read.Resulton both. The awaited tasks were already complete here so it was technically safe, but.Resultis the wrong pattern to ship. Replaced withawait stdoutTask/await stderrTask.Performance snapshot — documented contract (CRITICAL)
TakeSnapshotAsyncis callable at any time. If a caller mutated state first and snapshotted second, Restore would revert to the modified state instead of the original. The lazy-init pattern inPerformanceViewModel.EnsureSnapshotAsyncalready prevents that, but the contract was not documented on the service. Added an XML doc warning describing the required call order and pointing at the recommended pattern.Files
ViewModels/PrivacyViewModel.cs— pending-change tracking, Apply/Discard commands, baseline dictionaryViews/PrivacyView.xaml— toolbar reworked for the new commandsSysManager.Tests/PrivacyViewModelTests.cs— replaced reset-flow tests with pending-change tests covering: counter increment, baseline restore via Discard, "no changes" message, status text containing "pending"ViewModels/DashboardViewModel.cs— async LoadStaticInfo pathViewModels/DnsHostsViewModel.cs— async load + refreshServices/HostsFileService.cs—ReadHostsAsyncServices/IconExtractorService.cs— FIFO Queue evictionServices/SpeedTestService.cs— drop.ResultServices/PerformanceService.cs— XML doc contractREADME.md— Privacy section reflects the new flowCHANGELOG.md—[1.18.1]entrySysManager.csproj— bump to 1.18.1Test plan
dotnet buildRelease: 0 warnings, 0 errors (main project)dotnet buildRelease: 0 warnings, 0 errors (SysManager.Tests)dotnet buildRelease: 0 warnings, 0 errors (SysManager.UITests)Notes
SysManager.IntegrationTestswas already broken onmain(unrelatedLogsViewModel.SearchTextreferences) and is not run in CI; not addressed in this PR.Summary by CodeRabbit
Bug Fixes
Changed
Documentation