Repository navigation
fix: resolve 38 CodeQL alerts and exclude obj/ from analysis - #413
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR is a comprehensive CodeQL alert remediation that introduces a dedicated analysis configuration, adds explicit exception handling across services, standardizes path construction to use ChangesCodeQL Alert Remediation
Possibly related PRs
Poem
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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 |
![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/SpeedTestService.cs`:
- Around line 356-360: The current Zip Slip check in the archive extraction loop
(archive.Entries, destinationPath, fullToolsDir, toolsDir) uses a raw prefix
check that can be bypassed; replace it by computing the canonical full path and
then a relative path (e.g., Path.GetRelativePath(fullToolsDir, destinationPath))
and reject any entry where the relative path starts with ".." or is empty, or
otherwise contains path traversal, before extracting; ensure you use
Path.GetFullPath and OrdinalIgnoreCase comparisons and skip/throw for invalid
entries so extraction cannot escape the intended toolsDir boundary.
In `@SysManager/SysManager/Services/UninstallerService.cs`:
- Around line 405-408: The trusted-directory check in UninstallerService
currently uses StartsWith on fullPath which allows prefix bypass (e.g.,
"C:\Program Files Evil"). Normalize both paths (use Path.GetFullPath) and
compare directories safely: for each trustedDirs entry, get its full path,
ensure it ends with a directory separator (or otherwise treat it as a
directory), then verify fullPath starts with that trustedDirFullPath + separator
OR compute Path.GetRelativePath(trustedDirFullPath, fullPath) and ensure the
relative path does not begin with ".." (use OrdinalIgnoreCase); update the loop
that iterates trustedDirs and the check that returns true to use this
directory-aware comparison instead of naive StartsWith.
In `@SysManager/SysManager/ViewModels/NetworkSharedState.cs`:
- Around line 353-356: The loop over touched uses Buffers.ContainsKey(host) then
indexing Buffers[host], creating a TOCTOU race; replace that pattern with a
single thread-safe retrieval using Buffers.TryGetValue(host, out var buffer) and
only call TrimBuffer(buffer) when TryGetValue returns true. Update the foreach
in the method that contains the current loop (the one that calls TrimBuffer and
iterates touched) to use TryGetValue to avoid the race and eliminate separate
ContainsKey + index access.
🪄 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: f45d368e-118c-478b-80f0-4b6bc6e742d6
📒 Files selected for processing (19)
.github/codeql-config.yml.github/workflows/codeql.ymlCHANGELOG.mdSysManager/SysManager/Helpers/OutputKindToBrushConverter.csSysManager/SysManager/Services/AppAlertService.csSysManager/SysManager/Services/HealthScoreService.csSysManager/SysManager/Services/SpeedTestHistoryService.csSysManager/SysManager/Services/SpeedTestService.csSysManager/SysManager/Services/SystemInfoService.csSysManager/SysManager/Services/TrayIconService.csSysManager/SysManager/Services/TuneUpService.csSysManager/SysManager/Services/UninstallerService.csSysManager/SysManager/Services/UpdateService.csSysManager/SysManager/Services/WindowsFeaturesService.csSysManager/SysManager/ViewModels/AboutViewModel.csSysManager/SysManager/ViewModels/DashboardViewModel.csSysManager/SysManager/ViewModels/NetworkSharedState.csSysManager/SysManager/ViewModels/ProcessManagerViewModel.csSysManager/SysManager/ViewModels/UninstallerViewModel.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 (16)
.github/codeql-config.yml (1)
1-5: LGTM!.github/workflows/codeql.yml (1)
35-35: LGTM!CHANGELOG.md (1)
9-36: LGTM!SysManager/SysManager/Services/SystemInfoService.cs (1)
68-70: LGTM!Also applies to: 90-91
SysManager/SysManager/Services/TrayIconService.cs (1)
132-140: LGTM!Also applies to: 204-209
SysManager/SysManager/Services/TuneUpService.cs (1)
161-161: LGTM!Also applies to: 197-198
SysManager/SysManager/Services/SpeedTestHistoryService.cs (1)
21-23: LGTM!SysManager/SysManager/Services/UpdateService.cs (1)
140-143: LGTM!SysManager/SysManager/ViewModels/AboutViewModel.cs (1)
429-431: LGTM!Also applies to: 517-522
SysManager/SysManager/Services/AppAlertService.cs (1)
184-186: LGTM!SysManager/SysManager/Services/HealthScoreService.cs (1)
70-76: LGTM!Also applies to: 106-112
SysManager/SysManager/Services/WindowsFeaturesService.cs (1)
119-119: LGTM!SysManager/SysManager/ViewModels/ProcessManagerViewModel.cs (1)
146-146: LGTM!Also applies to: 168-173
SysManager/SysManager/ViewModels/UninstallerViewModel.cs (1)
160-166: LGTM!SysManager/SysManager/ViewModels/DashboardViewModel.cs (1)
148-148: LGTM!SysManager/SysManager/Helpers/OutputKindToBrushConverter.cs (1)
107-107: LGTM!
Comment on lines
+356
to
360
| foreach (var entry in archive.Entries.Where(e => !string.IsNullOrEmpty(e.Name))) | ||
| { | ||
| // Skip directory entries | ||
| if (string.IsNullOrEmpty(entry.Name)) continue; | ||
|
|
||
| var destinationPath = Path.GetFullPath(Path.Combine(toolsDir, entry.FullName)); | ||
| var destinationPath = Path.GetFullPath(Path.Join(toolsDir, entry.FullName)); | ||
| if (!destinationPath.StartsWith(fullToolsDir, StringComparison.OrdinalIgnoreCase)) | ||
| { |
There was a problem hiding this comment.
Harden Zip Slip boundary check to enforce a path-segment boundary.
Line 359 uses a raw prefix check, which can be bypassed by sibling paths like ...\tools_evil\... that still start with ...\tools. This allows writes outside the intended extraction root.
🔒 Proposed fix
- var fullToolsDir = Path.GetFullPath(toolsDir);
+ var fullToolsDir = Path.GetFullPath(toolsDir)
+ .TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar)
+ + Path.DirectorySeparatorChar;
foreach (var entry in archive.Entries.Where(e => !string.IsNullOrEmpty(e.Name)))
{
var destinationPath = Path.GetFullPath(Path.Join(toolsDir, entry.FullName));
- if (!destinationPath.StartsWith(fullToolsDir, StringComparison.OrdinalIgnoreCase))
+ if (!destinationPath.StartsWith(fullToolsDir, StringComparison.OrdinalIgnoreCase))
{
Log.Warning("Zip Slip attempt blocked: {Entry} resolves outside target dir", entry.FullName);
continue;
}🤖 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 356 - 360,
The current Zip Slip check in the archive extraction loop (archive.Entries,
destinationPath, fullToolsDir, toolsDir) uses a raw prefix check that can be
bypassed; replace it by computing the canonical full path and then a relative
path (e.g., Path.GetRelativePath(fullToolsDir, destinationPath)) and reject any
entry where the relative path starts with ".." or is empty, or otherwise
contains path traversal, before extracting; ensure you use Path.GetFullPath and
OrdinalIgnoreCase comparisons and skip/throw for invalid entries so extraction
cannot escape the intended toolsDir boundary.
Comment on lines
+405
to
+408
| foreach (var dir in trustedDirs) | ||
| { | ||
| if (!string.IsNullOrEmpty(dir) && fullPath.StartsWith(dir, StringComparison.OrdinalIgnoreCase)) | ||
| return true; |
There was a problem hiding this comment.
Fix trusted-directory validation to avoid prefix bypass.
Line 407 accepts any path starting with the trusted string, so paths like C:\Program Files Evil\malware.exe can pass. This weakens the uninstall execution trust boundary.
🛡️ Proposed fix
foreach (var dir in trustedDirs)
{
- if (!string.IsNullOrEmpty(dir) && fullPath.StartsWith(dir, StringComparison.OrdinalIgnoreCase))
- return true;
+ if (string.IsNullOrEmpty(dir)) continue;
+ var trusted = System.IO.Path.GetFullPath(dir)
+ .TrimEnd(System.IO.Path.DirectorySeparatorChar, System.IO.Path.AltDirectorySeparatorChar);
+ var candidate = System.IO.Path.GetFullPath(fullPath)
+ .TrimEnd(System.IO.Path.DirectorySeparatorChar, System.IO.Path.AltDirectorySeparatorChar);
+
+ if (candidate.Equals(trusted, StringComparison.OrdinalIgnoreCase) ||
+ candidate.StartsWith(trusted + System.IO.Path.DirectorySeparatorChar, StringComparison.OrdinalIgnoreCase) ||
+ candidate.StartsWith(trusted + System.IO.Path.AltDirectorySeparatorChar, StringComparison.OrdinalIgnoreCase))
+ return true;
}📝 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
| foreach (var dir in trustedDirs) | |
| { | |
| if (!string.IsNullOrEmpty(dir) && fullPath.StartsWith(dir, StringComparison.OrdinalIgnoreCase)) | |
| return true; | |
| foreach (var dir in trustedDirs) | |
| { | |
| if (string.IsNullOrEmpty(dir)) continue; | |
| var trusted = System.IO.Path.GetFullPath(dir) | |
| .TrimEnd(System.IO.Path.DirectorySeparatorChar, System.IO.Path.AltDirectorySeparatorChar); | |
| var candidate = System.IO.Path.GetFullPath(fullPath) | |
| .TrimEnd(System.IO.Path.DirectorySeparatorChar, System.IO.Path.AltDirectorySeparatorChar); | |
| if (candidate.Equals(trusted, StringComparison.OrdinalIgnoreCase) || | |
| candidate.StartsWith(trusted + System.IO.Path.DirectorySeparatorChar, StringComparison.OrdinalIgnoreCase) || | |
| candidate.StartsWith(trusted + System.IO.Path.AltDirectorySeparatorChar, StringComparison.OrdinalIgnoreCase)) | |
| return true; | |
| } |
🤖 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/UninstallerService.cs` around lines 405 - 408,
The trusted-directory check in UninstallerService currently uses StartsWith on
fullPath which allows prefix bypass (e.g., "C:\Program Files Evil"). Normalize
both paths (use Path.GetFullPath) and compare directories safely: for each
trustedDirs entry, get its full path, ensure it ends with a directory separator
(or otherwise treat it as a directory), then verify fullPath starts with that
trustedDirFullPath + separator OR compute
Path.GetRelativePath(trustedDirFullPath, fullPath) and ensure the relative path
does not begin with ".." (use OrdinalIgnoreCase); update the loop that iterates
trustedDirs and the check that returns true to use this directory-aware
comparison instead of naive StartsWith.
Comment on lines
+353
to
356
| foreach (var host in touched.Where(h => Buffers.ContainsKey(h))) | ||
| { | ||
| if (!Buffers.TryGetValue(host, out var buffer)) continue; | ||
| var buffer = Buffers[host]; | ||
| TrimBuffer(buffer); |
There was a problem hiding this comment.
Use TryGetValue to avoid a race between key check and index access.
Line 353 + Line 355 introduce a TOCTOU window on Buffers: another thread can remove host after ContainsKey and before Buffers[host], which can throw. Use a single TryGetValue read instead.
Suggested fix
- foreach (var host in touched.Where(h => Buffers.ContainsKey(h)))
- {
- var buffer = Buffers[host];
+ foreach (var host in touched)
+ {
+ if (!Buffers.TryGetValue(host, out var buffer)) continue;
TrimBuffer(buffer);
var target = Targets.FirstOrDefault(t => t.Host == host);
if (target == null) continue;
RecomputeStats(target, buffer);
}📝 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
| foreach (var host in touched.Where(h => Buffers.ContainsKey(h))) | |
| { | |
| if (!Buffers.TryGetValue(host, out var buffer)) continue; | |
| var buffer = Buffers[host]; | |
| TrimBuffer(buffer); | |
| foreach (var host in touched) | |
| { | |
| if (!Buffers.TryGetValue(host, out var buffer)) continue; | |
| TrimBuffer(buffer); |
🤖 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/NetworkSharedState.cs` around lines 353 -
356, The loop over touched uses Buffers.ContainsKey(host) then indexing
Buffers[host], creating a TOCTOU race; replace that pattern with a single
thread-safe retrieval using Buffers.TryGetValue(host, out var buffer) and only
call TrimBuffer(buffer) when TryGetValue returns true. Update the foreach in the
method that contains the current loop (the one that calls TrimBuffer and
iterates touched) to use TryGetValue to avoid the race and eliminate separate
ContainsKey + index access.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+405
to
+409
| foreach (var dir in trustedDirs) | ||
| { | ||
| if (!string.IsNullOrEmpty(dir) && fullPath.StartsWith(dir, StringComparison.OrdinalIgnoreCase)) | ||
| return true; | ||
| } |
Comment on lines
+169
to
+173
| p.Name.Contains(filter, StringComparison.OrdinalIgnoreCase) || | ||
| (p.Description?.Contains(filter, StringComparison.OrdinalIgnoreCase) ?? false) || | ||
| (p.PlainDescription?.Contains(filter, StringComparison.OrdinalIgnoreCase) ?? false) || | ||
| (p.Category?.Contains(filter, StringComparison.OrdinalIgnoreCase) ?? false) || | ||
| p.Pid.ToString().Contains(filter); |
laurentiu021
added a commit
that referenced
this pull request
May 22, 2026
…vice (#419) ## Summary **Batch 2** of the QA bug fix series. ### Issue #396 — CancellationTokenSource not disposed Added Dispose(bool) override to **8 ViewModels** that had CTS fields but no cleanup on dispose: - AppUpdatesViewModel - DiskAnalyzerViewModel - DriversViewModel - DuplicateFileViewModel - LogsViewModel - SpeedTestViewModel - TracerouteViewModel - UninstallerViewModel Each override disposes the CTS field and calls �ase.Dispose(disposing). The existing VMs that already had Dispose (CleanupVM, DeepCleanupVM, NetworkVM, SystemHealthVM, WindowsUpdateVM) were verified and left unchanged. ### Issue #413 — Bare catch in UpdateService Replaced bare \catch\ blocks with specific exception types + Serilog logging: - **GetRecentAsync**: catches \OperationCanceledException\, \HttpRequestException\, \JsonException\ - **DownloadAsync**: catches \OperationCanceledException\, \HttpRequestException\, \IOException\ - Cleanup catch blocks in DownloadAsync now catch \IOException\ and \UnauthorizedAccessException\ instead of bare catch ### Testing - Main project builds with 0 errors - No behavior changes — only proper resource cleanup and specific exception handling - CI will validate full test suite Closes #396, Closes #413 Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
laurentiu021
added a commit
that referenced
this pull request
May 22, 2026
Adds CHANGELOG entries for all 9 releases from the QA bug fix session: - **v0.28.16** — Dispose lifecycle (#395, #410) - **v0.28.17** — CTS disposal + bare catch (#396, #413) - **v0.28.18** — Input validation + null checks (#397, #398) - **v0.28.19** — JSON error handling (#400) - **v0.28.20** — Drive scanning + cache eviction + ConfigureAwait (#401, #402, #403) - **v0.28.21** — Audit logging + error messages (#405, #407) - **v0.28.22** — SHA256 verification (#408, #409) - **v0.28.23** — Service timeout + snapshot persist + traceroute DNS (#414, #415, #416) - **v0.28.24** — Accessibility (#411) 18 bugs fixed in total. Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
laurentiu021
added a commit
that referenced
this pull request
May 22, 2026
Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
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
3 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
Resolves 38 open CodeQL code scanning alerts across 16 source files and configures CodeQL to exclude auto-generated obj/ and �in/ directories (36 additional alerts in generated code).
Changes
Code fixes (38 alerts resolved)
Configuration
Remaining alerts (not fixable)
Testing