fix: resolve 5 CodeQL source alerts and suppress 1 intentional obsolete call#428
Conversation
📝 WalkthroughWalkthroughThis PR resolves CodeQL alerts and improves code maintainability through targeted refactoring. Configuration suppresses an intentional obsolete-method call, while six files are updated: loops converted to LINQ, filter logic extracted into helpers, exception handling made specific, and Path.Combine standardized to Path.Join. ChangesCodeQL Cleanup and Refactoring
🎯 2 (Simple) | ⏱️ ~12 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 |
![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 @.github/codeql-config.yml:
- Around line 11-13: Remove the unsupported "path" field from the
query-filters.exclude entry so the exclusion is not applied globally: delete the
entire exclude block that contains id: cs/call-to-obsolete-method (the one
attempting to target SysManager/SysManager/Services/UpdateService.cs), leaving
CodeQL to rely on the local pragmas already present (the existing "`#pragma`
warning disable SYSLIB0057" usages in UpdateService.cs and SpeedTestService.cs)
instead of a repository-wide suppression.
In `@SysManager/SysManager/Services/UninstallerService.cs`:
- Around line 405-407: The current trusted-directory check in UninstallerService
(the lambda returning trustedDirs.Any(...)) uses StartsWith and can be bypassed
by sibling paths; change it to compare normalized full paths with
directory-boundary checks by normalizing both fullPath and each trusted dir via
Path.GetFullPath, ensuring each trusted dir has a trailing directory separator
(e.g., append Path.DirectorySeparatorChar if missing) and then use
fullPathNormalized.StartsWith(trustedDirNormalized,
StringComparison.OrdinalIgnoreCase) so only paths under that directory (or the
directory itself) match; update the Any(...) predicate to perform these
normalizations and the separator-aware comparison.
In `@SysManager/SysManager/ViewModels/ViewModelBase.cs`:
- Around line 34-53: The narrow exception handlers in
ViewModelBase.InitializeAsync leave unhandled exceptions from its async void
path; add a final catch (Exception ex) at the end of the try/catch chain inside
InitializeAsync to log the unexpected exception (using Log.Error with context
including callerName) and swallow or handle it to preserve the documented safety
guarantee for async void initialization; ensure the new catch comes after the
existing specific catches in the InitializeAsync method of ViewModelBase so no
unexpected exception can escape.
🪄 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: d0707758-a032-42ad-ba8a-ca9b7093c3a8
📒 Files selected for processing (7)
.github/codeql-config.ymlCHANGELOG.mdSysManager/SysManager/Services/UninstallerService.csSysManager/SysManager/Services/UpdateService.csSysManager/SysManager/Services/WindowsFeaturesService.csSysManager/SysManager/ViewModels/ProcessManagerViewModel.csSysManager/SysManager/ViewModels/ViewModelBase.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 (4)
SysManager/SysManager/ViewModels/ProcessManagerViewModel.cs (1)
162-174: LGTM!SysManager/SysManager/Services/UpdateService.cs (1)
144-144: LGTM!SysManager/SysManager/Services/WindowsFeaturesService.cs (1)
117-142: LGTM!CHANGELOG.md (1)
11-31: LGTM!Also applies to: 39-39
| - exclude: | ||
| id: cs/call-to-obsolete-method | ||
| path: SysManager/SysManager/Services/UpdateService.cs |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In GitHub CodeQL codeql-config.yml, does query-filters.excludesupport apathfield? Please list the officially supported fields forquery-filters and whether unsupported keys are ignored or rejected.
💡 Result:
In GitHub CodeQL configuration files (codeql-config.yml), the query-filters.exclude block does not support a path field [1][2]. The query-filters mechanism is designed to filter queries based on their metadata properties (such as id, tags, or severity) rather than file paths [1][2]. To exclude specific files or directories from CodeQL analysis, you should use the top-level paths-ignore field in your codeql-config.yml file instead [3][2][4]. Officially supported fields for query-filters The query-filters section supports include and exclude blocks [2]. Within these blocks, you can filter queries using any valid CodeQL query metadata property [1]. Common examples include: - id: The unique identifier of the query [5][1][2]. - tags: The tags associated with the query [2]. - kind: The kind of query (e.g., problem, path-problem, table) [5][2]. - problem.severity: The severity level of the query (e.g., warning, recommendation) [2]. Handling of unsupported keys In GitHub Actions, providing unsupported keys in the codeql-config.yml file typically results in a warning or the key being ignored by the CodeQL action [6]. If you provide an invalid property at the top level of the configuration file or within specific blocks, the action may log a warning indicating that the property is not a valid input or is being ignored [6]. It is recommended to strictly follow the documented schema to ensure your configuration is applied correctly [2].
Citations:
- 1: https://github.blog/changelog/2022-08-30-code-scanning-customize-your-codeql-analysis-using-query-filters/
- 2: https://docs.github.com/en/code-security/reference/code-scanning/workflow-configuration-options
- 3: https://github.com/github/codeql/blob/main/.github/codeql/codeql-config.yml
- 4: https://stackoverflow.com/questions/74030852/is-there-a-way-to-exclude-files-from-codeql-scanning-on-github
- 5: Can't setup query-filters: exclude by query id github/codeql#16106
- 6: Trying to configure my codeql.yml to ignore paths in the codeql scan. github/codeql#13977
🏁 Script executed:
#!/bin/bash
# Verify whether obsolete-call usage is truly isolated to the intended call site.
rg -n --type=cs 'CreateFromSignedFile|SYSLIB0057|`#pragma` warning disable SYSLIB0057'Repository: laurentiu021/SystemManager
Length of output: 822
Remove path from query-filters.exclude—it's unsupported and causes the suppression to be global
query-filters.exclude does not support a path field. Unsupported fields are ignored, so the id: cs/call-to-obsolete-method exclusion will suppress this rule across the entire repository, not just UpdateService.cs. Since both UpdateService.cs and SpeedTestService.cs already use local #pragma warning disable SYSLIB0057 pragmas at the call sites (which is the correct approach), remove the ineffective CodeQL config line entirely.
🤖 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 @.github/codeql-config.yml around lines 11 - 13, Remove the unsupported
"path" field from the query-filters.exclude entry so the exclusion is not
applied globally: delete the entire exclude block that contains id:
cs/call-to-obsolete-method (the one attempting to target
SysManager/SysManager/Services/UpdateService.cs), leaving CodeQL to rely on the
local pragmas already present (the existing "`#pragma` warning disable SYSLIB0057"
usages in UpdateService.cs and SpeedTestService.cs) instead of a repository-wide
suppression.
| return trustedDirs.Any(dir => | ||
| !string.IsNullOrEmpty(dir) && | ||
| fullPath.StartsWith(dir, StringComparison.OrdinalIgnoreCase)); |
There was a problem hiding this comment.
Harden trusted-directory matching to avoid prefix bypass.
This check trusts paths that only start with a trusted prefix, which can be bypassed by sibling paths (for example, C:\Program Files Evil\...). Since this guards execution of registry-sourced uninstall commands, it should enforce a directory-boundary match.
Proposed fix
- return trustedDirs.Any(dir =>
- !string.IsNullOrEmpty(dir) &&
- fullPath.StartsWith(dir, StringComparison.OrdinalIgnoreCase));
+ var normalizedPath = System.IO.Path.GetFullPath(fullPath)
+ .TrimEnd(System.IO.Path.DirectorySeparatorChar, System.IO.Path.AltDirectorySeparatorChar);
+
+ return trustedDirs.Any(dir =>
+ {
+ if (string.IsNullOrWhiteSpace(dir)) return false;
+ var normalizedDir = System.IO.Path.GetFullPath(dir)
+ .TrimEnd(System.IO.Path.DirectorySeparatorChar, System.IO.Path.AltDirectorySeparatorChar);
+
+ return normalizedPath.Equals(normalizedDir, StringComparison.OrdinalIgnoreCase)
+ || normalizedPath.StartsWith(
+ normalizedDir + System.IO.Path.DirectorySeparatorChar,
+ StringComparison.OrdinalIgnoreCase)
+ || normalizedPath.StartsWith(
+ normalizedDir + System.IO.Path.AltDirectorySeparatorChar,
+ StringComparison.OrdinalIgnoreCase);
+ });🤖 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 - 407,
The current trusted-directory check in UninstallerService (the lambda returning
trustedDirs.Any(...)) uses StartsWith and can be bypassed by sibling paths;
change it to compare normalized full paths with directory-boundary checks by
normalizing both fullPath and each trusted dir via Path.GetFullPath, ensuring
each trusted dir has a trailing directory separator (e.g., append
Path.DirectorySeparatorChar if missing) and then use
fullPathNormalized.StartsWith(trustedDirNormalized,
StringComparison.OrdinalIgnoreCase) so only paths under that directory (or the
directory itself) match; update the Any(...) predicate to perform these
normalizations and the separator-aware comparison.
| catch (InvalidOperationException ex) | ||
| { | ||
| Log.Error(ex, "Unhandled exception in async initialization of {Caller}", callerName); | ||
| Log.Error(ex, "Invalid operation in async initialization of {Caller}", callerName); | ||
| } | ||
| catch (UnauthorizedAccessException ex) | ||
| { | ||
| Log.Error(ex, "Access denied in async initialization of {Caller}", callerName); | ||
| } | ||
| catch (System.IO.IOException ex) | ||
| { | ||
| Log.Error(ex, "I/O error in async initialization of {Caller}", callerName); | ||
| } | ||
| catch (System.Net.Http.HttpRequestException ex) | ||
| { | ||
| Log.Error(ex, "Network error in async initialization of {Caller}", callerName); | ||
| } | ||
| catch (TimeoutException ex) | ||
| { | ||
| Log.Error(ex, "Timeout in async initialization of {Caller}", callerName); | ||
| } |
There was a problem hiding this comment.
Unhandled exception gap reintroduced in InitializeAsync
Line 34 onward narrows catches, but without a final fallback, async void can crash the app for unexpected exceptions. This breaks the safety guarantee documented for InitializeAsync.
Suggested patch
catch (TimeoutException ex)
{
Log.Error(ex, "Timeout in async initialization of {Caller}", callerName);
}
+ catch (Exception ex)
+ {
+ Log.Error(ex, "Unexpected error in async initialization of {Caller}", callerName);
+ }📝 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.
| catch (InvalidOperationException ex) | |
| { | |
| Log.Error(ex, "Unhandled exception in async initialization of {Caller}", callerName); | |
| Log.Error(ex, "Invalid operation in async initialization of {Caller}", callerName); | |
| } | |
| catch (UnauthorizedAccessException ex) | |
| { | |
| Log.Error(ex, "Access denied in async initialization of {Caller}", callerName); | |
| } | |
| catch (System.IO.IOException ex) | |
| { | |
| Log.Error(ex, "I/O error in async initialization of {Caller}", callerName); | |
| } | |
| catch (System.Net.Http.HttpRequestException ex) | |
| { | |
| Log.Error(ex, "Network error in async initialization of {Caller}", callerName); | |
| } | |
| catch (TimeoutException ex) | |
| { | |
| Log.Error(ex, "Timeout in async initialization of {Caller}", callerName); | |
| } | |
| catch (InvalidOperationException ex) | |
| { | |
| Log.Error(ex, "Invalid operation in async initialization of {Caller}", callerName); | |
| } | |
| catch (UnauthorizedAccessException ex) | |
| { | |
| Log.Error(ex, "Access denied in async initialization of {Caller}", callerName); | |
| } | |
| catch (System.IO.IOException ex) | |
| { | |
| Log.Error(ex, "I/O error in async initialization of {Caller}", callerName); | |
| } | |
| catch (System.Net.Http.HttpRequestException ex) | |
| { | |
| Log.Error(ex, "Network error in async initialization of {Caller}", callerName); | |
| } | |
| catch (TimeoutException ex) | |
| { | |
| Log.Error(ex, "Timeout in async initialization of {Caller}", callerName); | |
| } | |
| catch (Exception ex) | |
| { | |
| Log.Error(ex, "Unexpected error in async initialization of {Caller}", callerName); | |
| } |
🤖 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/ViewModelBase.cs` around lines 34 - 53, The
narrow exception handlers in ViewModelBase.InitializeAsync leave unhandled
exceptions from its async void path; add a final catch (Exception ex) at the end
of the try/catch chain inside InitializeAsync to log the unexpected exception
(using Log.Error with context including callerName) and swallow or handle it to
preserve the documented safety guarantee for async void initialization; ensure
the new catch comes after the existing specific catches in the InitializeAsync
method of ViewModelBase so no unexpected exception can escape.
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
## Summary **Batch 8** of the QA bug fix series. ### Issue #408 — Update download no SHA256 verification Added \VerifyHashAsync\ method to \UpdateService\: - Downloads the \.sha256\ file from the same GitHub release - Computes SHA256 of the local downloaded file - Compares and returns \(Verified, ExpectedHash, ActualHash)\ - Best-effort: if \.sha256\ file is unavailable (network error), returns verified=true to not block install - Logs hash match/mismatch via Serilog ### Issue #409 — Ookla CLI no hash verification Added integrity verification to \SpeedTestService.EnsureOoklaAsync\: - Computes SHA256 of downloaded zip and logs it for audit trail - Validates the zip is not corrupt (\ZipFile.OpenRead\) - Verifies the zip contains \speedtest.exe\ - Rejects corrupt/tampered downloads with descriptive error ### Testing - Build: 0 errors Closes #408, Closes #409 Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
2 participants
Summary
Resolves all 6 remaining CodeQL source code alerts (24 obj/ alerts pending auto-dismiss from codeql-config exclusion).
Changes
catch (Exception)with specific types (InvalidOperationException,UnauthorizedAccessException,IOException,HttpRequestException,TimeoutException).Any()(cs/linq/missed-where).Select().Where()pipeline (cs/linq/missed-select)Path.CombinewithPath.Join(cs/path-combine)CreateFromSignedFileusage (no modern .NET replacement)CodeQL Alerts Addressed
Also in this PR
Summary by CodeRabbit
Bug Fixes
Chores