feat: verify release asset integrity with SHA256 checksums#38
Conversation
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
✨ Finishing Touches🧪 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 |
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| using (FileStream stream = File.OpenRead(filePath)) | ||
| using (SHA256 sha256 = SHA256.Create()) | ||
| { | ||
| byte[] hash = sha256.ComputeHash(stream); | ||
| return BytesToHex(hash); | ||
| } |
There was a problem hiding this comment.
The ComputeSha256 method does not handle IO-related exceptions (e.g., file not found, access denied) that may occur when opening or reading the file. This can cause the application to crash if the caller does not catch these exceptions.
Recommendation:
Wrap the file operations in a try-catch block and return a meaningful error or propagate a custom exception, depending on the intended usage. For example:
try {
using (FileStream stream = File.OpenRead(filePath))
using (SHA256 sha256 = SHA256.Create())
{
byte[] hash = sha256.ComputeHash(stream);
return BytesToHex(hash);
}
} catch (IOException ex) {
// Handle or log the exception as appropriate
throw new InvalidOperationException($"Failed to compute hash for '{filePath}'", ex);
}| int separatorIndex = trimmed.IndexOf(' '); | ||
| if (separatorIndex <= 0) continue; | ||
|
|
||
| string hash = trimmed.Substring(0, separatorIndex).Trim(); | ||
| string fileName = trimmed.Substring(separatorIndex + 1).Trim(); |
There was a problem hiding this comment.
The parsing logic in ParseChecksums splits each line on the first space character, which assumes that file names do not contain spaces. If a file name contains spaces, this will result in incorrect parsing and potentially invalid hash-to-file mappings.
Recommendation:
Consider using a more robust parsing strategy, such as splitting on the last space or using a delimiter that cannot appear in file names. Alternatively, document the expected format and enforce it during checksum file generation.
| { | ||
| string targetPath = GetPluginPackagePath(); | ||
| string tempPath = targetPath + ".download"; | ||
| string checksumPath = targetPath + ".sha256"; | ||
|
|
||
| using (HttpUpdateClient client = new HttpUpdateClient()) | ||
| { | ||
| await client.DownloadFileAsync(info.AssetUrl, tempPath); | ||
| if (!string.IsNullOrWhiteSpace(info.ChecksumUrl)) | ||
| { | ||
| try | ||
| { | ||
| await client.DownloadFileAsync(info.ChecksumUrl, checksumPath); | ||
| } | ||
| catch (Exception checksumEx) | ||
| { | ||
| File.Delete(tempPath); | ||
| throw new InvalidOperationException("Failed to download release checksums: " + checksumEx.Message, checksumEx); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (File.Exists(checksumPath) && !VerifyDownloadedAsset(tempPath, checksumPath, Path.GetFileName(info.AssetUrl))) | ||
| { | ||
| File.Delete(tempPath); | ||
| File.Delete(checksumPath); | ||
| throw new InvalidOperationException("Downloaded asset failed hash verification."); | ||
| } | ||
|
|
||
| try | ||
| { | ||
| File.Copy(tempPath, targetPath, true); | ||
| File.Delete(tempPath); | ||
| if (File.Exists(checksumPath)) File.Delete(checksumPath); | ||
|
|
||
| ShowOnUi(delegate | ||
| { |
There was a problem hiding this comment.
The update installation logic performs file copy and delete operations that are not atomic, which may result in a partially updated or corrupted plugin if the process is interrupted or if there is concurrent access. To improve reliability and safety, consider using atomic file replacement techniques (such as writing to a temporary file and then using File.Replace or equivalent) and ensure all file operations are robust against interruptions. Additionally, validate all file paths before performing operations to prevent potential path traversal or unintended file overwrites.
| } | ||
| } | ||
|
|
||
| private static bool VerifyDownloadedAsset(string assetPath, string checksumPath, string assetFileName) | ||
| { | ||
| if (!File.Exists(checksumPath)) return true; | ||
|
|
||
| string checksumsText = File.ReadAllText(checksumPath); | ||
| Dictionary<string, string> checksums = AssetVerifier.ParseChecksums(checksumsText); | ||
| if (!checksums.ContainsKey(assetFileName)) return true; | ||
|
|
||
| return AssetVerifier.VerifyFile(assetPath, checksums[assetFileName]); | ||
| } | ||
|
|
||
| private static string GetPluginPackagePath() | ||
| { | ||
| return PluginPathResolver.ResolvePluginPackagePath( |
There was a problem hiding this comment.
The method GetPluginPackagePath() does not validate the returned path, which could lead to security issues if the path is outside the intended plugin directory. Ensure that the resolved path is within the expected directory and does not allow for path traversal or writing to unintended locations. This is especially important when performing update operations that overwrite executable code.
| info.LatestVersion = tagName; | ||
| info.ReleaseUrl = BuildReleaseUrl(tagName); | ||
| info.AssetUrl = BuildAssetUrl(tagName, format); | ||
| info.ChecksumUrl = BuildChecksumUrl(tagName); | ||
| info.IsUpdateAvailable = IsNewerVersion(GetCurrentVersion(), tagName); | ||
| return info; | ||
| } |
There was a problem hiding this comment.
The code constructs the UpdateInfo object without validating whether tagName is null or empty. If the GitHub API response is malformed or contains no valid tags, tagName will be empty, resulting in URLs that point to the generic releases page and potentially setting IsUpdateAvailable incorrectly. This could mislead users or cause downstream errors.
Recommended solution:
Add a check after extracting tagName to ensure it is not null or empty. If it is, either throw an exception or return an UpdateInfo object indicating that update information could not be determined.
if (string.IsNullOrEmpty(tagName))
throw new InvalidOperationException("No valid release tag found in the API response.");- Add AssetVerifier helper to compute and verify SHA256 hashes. - Add ChecksumUrl to UpdateInfo and download SHA256SUMS.txt during updates. - Verify downloaded asset hash before replacing the active plugin file. - Update CI to generate SHA256SUMS.txt and attach it to releases. - Add 7 unit tests for AssetVerifier; dotnet test passes 60/60. Closes #34.
hieuck
force-pushed
the
feature/verify-asset-hash
branch
from
8b7ee9b to
c436e5f
Compare
| { | ||
| List<string> values = new List<string>(); |
There was a problem hiding this comment.
Fragile JSON Parsing with Regex
The ExtractJsonStrings method uses a regular expression to extract values from JSON. Parsing JSON with regex is error-prone and fragile, especially if the JSON structure changes, contains nested objects, or includes escaped characters. This can lead to incorrect extraction or missed values, impacting the reliability of update checks.
Recommended solution:
Use a proper JSON parser such as System.Text.Json or Newtonsoft.Json to extract values robustly:
using System.Text.Json;
...
var doc = JsonDocument.Parse(json);
// Extract values using JsonElement traversalThis approach is more maintainable and less error-prone.
1 participant
Summary\r\nAdd SHA256 checksum verification for downloaded update assets to mitigate supply-chain risks.\r\n\r\n## Changes\r\n- Add helper to compute/parse/verify SHA256 hashes.\r\n- now includes a pointing to .\r\n- downloads and verifies the asset hash before installation.\r\n- If verification fails, the update is aborted and the user is notified.\r\n- CI generates for each release and uploads it as a release asset.\r\n\r\n## Tests\r\n- Added 7 unit tests for .\r\n- MSBUILD : error MSB1003: Specify a project or solution file. The current working directory does not contain a project or solution file. passes: 60/60.\r\n\r\nCloses #34.