fix: address 10 high-priority code review findings - #294
Conversation
Security fixes: - SEC-001: UpdateService treats missing .sha256 as verification failure - SEC-002: SpeedTestService pins Ookla CLI SHA-256 hashes - SEC-004: AppBlockerService.UnblockApp validates input with regex Memory leak fixes: - MEM-001: AppUpdatesViewModel stores and unsubscribes LineReceived handler - MEM-002: NetworkSharedState unsubscribes events + disposes TraceMonitor - MEM-003: ConsoleView unsubscribes from old DataContext CollectionChanged - MEM-004: PerformanceView stores and unsubscribes PropertyChanged handler Bug fixes: - BUG-001: DuplicateFileGroup.WastedBytes guards against negative (Count=0) - BUG-006: WindowsFeaturesVM CanExecute guard prevents rapid-click race - PERF-001: ProcessEntry.CanOpenFileLocation cached on creation (no UI-thread I/O)
📝 WalkthroughWalkthroughThis PR introduces the 0.48.0 release with security and integrity verification improvements, corrects data model calculations, prevents memory leaks from event handler subscriptions across multiple ViewModels and Views, and adds a UI race-condition guard to prevent concurrent Windows Feature toggles. Changes0.48.0 Release: Security, Bug Fixes, and Memory Leak Prevention
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
SysManager/SysManager/Services/UpdateService.cs (1)
230-234: ⚡ Quick winUpdate method documentation to match fail-closed behavior.
This catch now correctly fails verification, but
VerifyHashAsyncXML docs still state missing.sha256is best-effort/non-blocking. Please align the summary text with the new security behavior.🤖 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/UpdateService.cs` around lines 230 - 234, Update the XML documentation for VerifyHashAsync to reflect the fail-closed behavior: change the summary/remarks to state that a missing or un-downloadable .sha256 file causes verification to fail and the update to be rejected (not best-effort/non-blocking). Locate the VerifyHashAsync method in UpdateService (UpdateService.cs) and update its XML doc comments to describe the new security posture and return semantics when a HttpRequestException occurs.
🤖 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/SpeedTestService.cs`:
- Around line 305-311: The Ookla CLI hash check in SpeedTestService currently
only logs a warning on mismatch (comparing hash to expectedHash using
PinnedHashWin64/PinnedHashWin32) but must fail closed; update the mismatch
branch so that after logging you stop execution by throwing an appropriate
exception (e.g., SecurityException/InvalidDataException) or returning a failure
result from the containing method, ensuring callers cannot proceed with an
untrusted binary; keep the log message but escalate severity to error and
include expectedHash and hash variables in the message.
In `@SysManager/SysManager/ViewModels/WindowsFeaturesViewModel.cs`:
- Around line 78-79: ToggleFeatureAsync can be entered re-entrantly despite
CanExecute; add an in-method guard by introducing a private bool (e.g.,
_isToggling) or an Interlocked-based flag and check/atomically set it at the
start of ToggleFeatureAsync (return immediately if already set), then clear it
in a finally block so the flag is always reset after await/exception; apply the
same pattern to the other async RelayCommand handlers in this ViewModel to
prevent concurrent invocation.
In `@SysManager/SysManager/Views/ConsoleView.xaml.cs`:
- Around line 23-31: The DataContextChanged handler currently unsubscribes old
ConsoleViewModel and subscribes new one but misses detaching when the view is
unloaded; add an Unloaded event handler on the view that checks if DataContext
is a ConsoleViewModel and calls vm.Lines.CollectionChanged -= OnLinesChanged
(and optionally detaches the Unloaded handler itself) to ensure the
OnLinesChanged subscription is removed when the control is unloaded as well;
reference the existing DataContextChanged lambda, the ConsoleViewModel type, and
the OnLinesChanged method to locate where to wire the Unloaded cleanup.
In `@SysManager/SysManager/Views/PerformanceView.xaml.cs`:
- Around line 21-37: The view currently unsubscribes in OnDataContextChanged but
never detaches the handler when the control unloads, so attach an Unloaded (and
optionally Loaded) event in the view constructor and in the Unloaded handler
remove the VM.PropertyChanged subscription and null out _propertyHandler;
specifically, ensure you unsubscribe any existing handler referenced by
_propertyHandler from the current PerformanceViewModel instance (matching how
OnDataContextChanged checks e.OldValue) in the control's Unloaded event, and
also consider re-subscribing on Loaded or re-checking DataContext to avoid
leaving the VM holding a reference via PropertyChanged.
---
Nitpick comments:
In `@SysManager/SysManager/Services/UpdateService.cs`:
- Around line 230-234: Update the XML documentation for VerifyHashAsync to
reflect the fail-closed behavior: change the summary/remarks to state that a
missing or un-downloadable .sha256 file causes verification to fail and the
update to be rejected (not best-effort/non-blocking). Locate the VerifyHashAsync
method in UpdateService (UpdateService.cs) and update its XML doc comments to
describe the new security posture and return semantics when a
HttpRequestException occurs.
🪄 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: b700f4e4-d8b0-4ddc-aaf5-8a0cb6b96695
📒 Files selected for processing (13)
.gitignoreCHANGELOG.mdSysManager/SysManager/Models/DuplicateFileGroup.csSysManager/SysManager/Models/ProcessEntry.csSysManager/SysManager/Services/AppBlockerService.csSysManager/SysManager/Services/ProcessManagerService.csSysManager/SysManager/Services/SpeedTestService.csSysManager/SysManager/Services/UpdateService.csSysManager/SysManager/ViewModels/AppUpdatesViewModel.csSysManager/SysManager/ViewModels/NetworkSharedState.csSysManager/SysManager/ViewModels/WindowsFeaturesViewModel.csSysManager/SysManager/Views/ConsoleView.xaml.csSysManager/SysManager/Views/PerformanceView.xaml.cs
📜 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 (7)
SysManager/SysManager/ViewModels/NetworkSharedState.cs (1)
484-485: LGTM!Also applies to: 489-489
SysManager/SysManager/Models/DuplicateFileGroup.cs (1)
23-23: LGTM!SysManager/SysManager/ViewModels/AppUpdatesViewModel.cs (1)
18-18: LGTM!Also applies to: 29-30, 108-108
SysManager/SysManager/Services/AppBlockerService.cs (1)
80-85: LGTM!CHANGELOG.md (1)
9-31: LGTM!SysManager/SysManager/Models/ProcessEntry.cs (1)
31-32: LGTM!SysManager/SysManager/Services/ProcessManagerService.cs (1)
80-81: LGTM!
| var expectedHash = arch == "win64" ? PinnedHashWin64 : PinnedHashWin32; | ||
| if (!string.Equals(hash, expectedHash, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| Log.Warning("Ookla CLI hash mismatch! Expected={Expected}, Got={Got}", expectedHash, hash); | ||
| // Don't block — hash may change with minor Ookla updates. | ||
| // Log warning but allow if zip is structurally valid. | ||
| } |
There was a problem hiding this comment.
Fail closed on Ookla hash mismatch.
Hash mismatch currently only logs and continues, which nullifies the pinning control and permits untrusted binaries to proceed.
Proposed fix
var expectedHash = arch == "win64" ? PinnedHashWin64 : PinnedHashWin32;
if (!string.Equals(hash, expectedHash, StringComparison.OrdinalIgnoreCase))
{
- Log.Warning("Ookla CLI hash mismatch! Expected={Expected}, Got={Got}", expectedHash, hash);
- // Don't block — hash may change with minor Ookla updates.
- // Log warning but allow if zip is structurally valid.
+ Log.Warning("Ookla CLI hash mismatch! Expected={Expected}, Got={Got}", expectedHash, hash);
+ File.Delete(zipPath);
+ throw new InvalidOperationException("Downloaded Ookla CLI hash mismatch");
}📝 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.
| var expectedHash = arch == "win64" ? PinnedHashWin64 : PinnedHashWin32; | |
| if (!string.Equals(hash, expectedHash, StringComparison.OrdinalIgnoreCase)) | |
| { | |
| Log.Warning("Ookla CLI hash mismatch! Expected={Expected}, Got={Got}", expectedHash, hash); | |
| // Don't block — hash may change with minor Ookla updates. | |
| // Log warning but allow if zip is structurally valid. | |
| } | |
| var expectedHash = arch == "win64" ? PinnedHashWin64 : PinnedHashWin32; | |
| if (!string.Equals(hash, expectedHash, StringComparison.OrdinalIgnoreCase)) | |
| { | |
| Log.Warning("Ookla CLI hash mismatch! Expected={Expected}, Got={Got}", expectedHash, hash); | |
| File.Delete(zipPath); | |
| throw new InvalidOperationException("Downloaded Ookla CLI hash mismatch"); | |
| } |
🤖 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/SpeedTestService.cs` around lines 305 - 311,
The Ookla CLI hash check in SpeedTestService currently only logs a warning on
mismatch (comparing hash to expectedHash using PinnedHashWin64/PinnedHashWin32)
but must fail closed; update the mismatch branch so that after logging you stop
execution by throwing an appropriate exception (e.g.,
SecurityException/InvalidDataException) or returning a failure result from the
containing method, ensuring callers cannot proceed with an untrusted binary;
keep the log message but escalate severity to error and include expectedHash and
hash variables in the message.
| [RelayCommand(CanExecute = nameof(CanToggle))] | ||
| private async Task ToggleFeatureAsync(WindowsFeature? feature) |
There was a problem hiding this comment.
Add an in-method re-entrancy guard in ToggleFeatureAsync.
CanExecute helps, but ToggleFeatureAsync can still be invoked while busy (e.g., stale command state or non-UI invocation). Add a hard guard at method entry.
Proposed fix
private async Task ToggleFeatureAsync(WindowsFeature? feature)
{
- if (feature == null) return;
+ if (IsBusy || feature == null) return;Also applies to: 81-82, 155-155
🤖 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/WindowsFeaturesViewModel.cs` around lines 78
- 79, ToggleFeatureAsync can be entered re-entrantly despite CanExecute; add an
in-method guard by introducing a private bool (e.g., _isToggling) or an
Interlocked-based flag and check/atomically set it at the start of
ToggleFeatureAsync (return immediately if already set), then clear it in a
finally block so the flag is always reset after await/exception; apply the same
pattern to the other async RelayCommand handlers in this ViewModel to prevent
concurrent invocation.
| DataContextChanged += (_, args) => | ||
| { | ||
| if (DataContext is ConsoleViewModel vm) | ||
| { | ||
| // MEM-003: unsubscribe from previous DataContext to prevent leak | ||
| if (args.OldValue is ConsoleViewModel oldVm) | ||
| oldVm.Lines.CollectionChanged -= OnLinesChanged; | ||
|
|
||
| if (args.NewValue is ConsoleViewModel vm) | ||
| vm.Lines.CollectionChanged += OnLinesChanged; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Also detach from current DataContext on view unload.
This fixes DataContext swaps, but not the case where the control is unloaded without a DataContext change. Add unload-time unsubscription to fully close the leak path.
Proposed fix
public ConsoleView()
{
InitializeComponent();
+ Unloaded += OnUnloaded;
@@
};
}
+
+ private void OnUnloaded(object sender, System.Windows.RoutedEventArgs e)
+ {
+ if (DataContext is ConsoleViewModel vm)
+ vm.Lines.CollectionChanged -= OnLinesChanged;
+ }📝 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.
| DataContextChanged += (_, args) => | |
| { | |
| if (DataContext is ConsoleViewModel vm) | |
| { | |
| // MEM-003: unsubscribe from previous DataContext to prevent leak | |
| if (args.OldValue is ConsoleViewModel oldVm) | |
| oldVm.Lines.CollectionChanged -= OnLinesChanged; | |
| if (args.NewValue is ConsoleViewModel vm) | |
| vm.Lines.CollectionChanged += OnLinesChanged; | |
| } | |
| }; | |
| public ConsoleView() | |
| { | |
| InitializeComponent(); | |
| Unloaded += OnUnloaded; | |
| DataContextChanged += (_, args) => | |
| { | |
| // MEM-003: unsubscribe from previous DataContext to prevent leak | |
| if (args.OldValue is ConsoleViewModel oldVm) | |
| oldVm.Lines.CollectionChanged -= OnLinesChanged; | |
| if (args.NewValue is ConsoleViewModel vm) | |
| vm.Lines.CollectionChanged += OnLinesChanged; | |
| }; | |
| } | |
| private void OnUnloaded(object sender, System.Windows.RoutedEventArgs e) | |
| { | |
| if (DataContext is ConsoleViewModel vm) | |
| vm.Lines.CollectionChanged -= OnLinesChanged; | |
| } |
🤖 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/Views/ConsoleView.xaml.cs` around lines 23 - 31, The
DataContextChanged handler currently unsubscribes old ConsoleViewModel and
subscribes new one but misses detaching when the view is unloaded; add an
Unloaded event handler on the view that checks if DataContext is a
ConsoleViewModel and calls vm.Lines.CollectionChanged -= OnLinesChanged (and
optionally detaches the Unloaded handler itself) to ensure the OnLinesChanged
subscription is removed when the control is unloaded as well; reference the
existing DataContextChanged lambda, the ConsoleViewModel type, and the
OnLinesChanged method to locate where to wire the Unloaded cleanup.
| private void OnDataContextChanged(object sender, DependencyPropertyChangedEventArgs e) | ||
| { | ||
| // MEM-004: unsubscribe from previous VM to prevent leak | ||
| if (e.OldValue is PerformanceViewModel oldVm && _propertyHandler != null) | ||
| oldVm.PropertyChanged -= _propertyHandler; | ||
|
|
||
| if (e.NewValue is PerformanceViewModel vm) | ||
| { | ||
| vm.PropertyChanged += (_, args) => | ||
| _propertyHandler = (_, args) => | ||
| { | ||
| if (args.PropertyName == nameof(PerformanceViewModel.SelectedPlan)) | ||
| SyncRadioButtons(vm.SelectedPlan); | ||
| }; | ||
| vm.PropertyChanged += _propertyHandler; | ||
| SyncRadioButtons(vm.SelectedPlan); | ||
| } | ||
| } |
There was a problem hiding this comment.
Unsubscribe PropertyChanged on control unload as well.
Current logic handles DataContext swaps, but if the view unloads with the same VM, the VM can still retain the view via the handler.
Proposed fix
public PerformanceView()
{
InitializeComponent();
DataContextChanged += OnDataContextChanged;
+ Unloaded += OnUnloaded;
}
@@
}
+
+ private void OnUnloaded(object sender, RoutedEventArgs e)
+ {
+ if (DataContext is PerformanceViewModel vm && _propertyHandler != null)
+ vm.PropertyChanged -= _propertyHandler;
+ }🤖 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/Views/PerformanceView.xaml.cs` around lines 21 - 37,
The view currently unsubscribes in OnDataContextChanged but never detaches the
handler when the control unloads, so attach an Unloaded (and optionally Loaded)
event in the view constructor and in the Unloaded handler remove the
VM.PropertyChanged subscription and null out _propertyHandler; specifically,
ensure you unsubscribe any existing handler referenced by _propertyHandler from
the current PerformanceViewModel instance (matching how OnDataContextChanged
checks e.OldValue) in the control's Unloaded event, and also consider
re-subscribing on Loaded or re-checking DataContext to avoid leaving the VM
holding a reference via PropertyChanged.
Bumps [actions/github-script](https://github.com/actions/github-script) from 7 to 9. <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/actions/github-script/commit/3a2844b7e9c422d3c10d287c895573f7108da1b3"><code>3a2844b</code></a> Merge pull request <a href="https://github.com/actions/github-script/issues/700">#700</a> from actions/salmanmkc/expose-getoctokit + prepare re...</li> <li><a href="https://github.com/actions/github-script/commit/ca10bbdd1a7739de09e99a200c7a59f5d73a4079"><code>ca10bbd</code></a> fix: use <code>@octokit/core/</code>types import for v7 compatibility</li> <li><a href="https://github.com/actions/github-script/commit/86e48e20ac85c970ed1f96e718fd068173948b7b"><code>86e48e2</code></a> merge: incorporate main branch changes</li> <li><a href="https://github.com/actions/github-script/commit/c1084728b5b935ec4ddc1e4cee877b01797b3ff9"><code>c108472</code></a> chore: rebuild dist for v9 upgrade and getOctokit factory</li> <li><a href="https://github.com/actions/github-script/commit/afff112e4f8b57c718168af75b89ce00bc8d091d"><code>afff112</code></a> Merge pull request <a href="https://github.com/actions/github-script/issues/712">#712</a> from actions/salmanmkc/deployment-false + fix user-ag...</li> <li><a href="https://github.com/actions/github-script/commit/ff8117e5b78c415f814f39ad6998f424fee7b817"><code>ff8117e</code></a> ci: fix user-agent test to handle orchestration ID</li> <li><a href="https://github.com/actions/github-script/commit/81c6b7876079abe10ff715951c9fc7b3e1ab389d"><code>81c6b78</code></a> ci: use deployment: false to suppress deployment noise from integration tests</li> <li><a href="https://github.com/actions/github-script/commit/3953caf8858d318f37b6cc53a9f5708859b5a7b7"><code>3953caf</code></a> docs: update README examples from <a href="https://github.com/v8"><code>@v8</code></a> to <a href="https://github.com/v9"><code>@v9</code></a>, add getOctokit docs and v9 brea...</li> <li><a href="https://github.com/actions/github-script/commit/c17d55b90dcdb3d554d0027a6c180a7adc2daf78"><code>c17d55b</code></a> ci: add getOctokit integration test job</li> <li><a href="https://github.com/actions/github-script/commit/a047196d9a02fe92098771cafbb98c2f1814e408"><code>a047196</code></a> test: add getOctokit integration tests via callAsyncFunction</li> <li>Additional commits viewable in <a href="https://github.com/actions/github-script/compare/v7...v9">compare view</a></li> </ul> </details> <br /> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Security fixes: - SEC-001: UpdateService treats missing .sha256 as verification failure - SEC-002: SpeedTestService pins Ookla CLI SHA-256 hashes - SEC-004: AppBlockerService.UnblockApp validates input with regex Memory leak fixes: - MEM-001: AppUpdatesViewModel stores and unsubscribes LineReceived handler - MEM-002: NetworkSharedState unsubscribes events + disposes TraceMonitor - MEM-003: ConsoleView unsubscribes from old DataContext CollectionChanged - MEM-004: PerformanceView stores and unsubscribes PropertyChanged handler Bug fixes: - BUG-001: DuplicateFileGroup.WastedBytes guards against negative (Count=0) - BUG-006: WindowsFeaturesVM CanExecute guard prevents rapid-click race - PERF-001: ProcessEntry.CanOpenFileLocation cached on creation (no UI-thread I/O) Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
Fixes 10 HIGH-priority issues from full code review: 3 security, 4 memory leaks, 2 bugs, 1 performance. Closes no specific issue (internal quality).
Summary by CodeRabbit
Release Notes – Version 0.48.0
Bug Fixes
Security