fix: auto-update now performs in-place self-replace with SHA256 verification#274
Merged
Conversation
…ication Previously, Install only launched the new exe from a temp folder without replacing the running executable. Now it: 1. Verifies the SHA256 hash of the downloaded build against the release asset 2. Writes an updater script that waits for the current process to exit 3. Copies the new exe over the original 4. Restarts SysManager from the original path Also handles access-denied gracefully (suggests running as admin). Closes #240
📝 WalkthroughWalkthroughThis PR replaces the app's self-update mechanism from launching a downloaded executable from a temporary location to performing SHA256 hash verification, writing an updater script that waits for process exit, overwriting the current executable in-place, and restarting. Test coverage is expanded with three async test cases, and user-facing documentation is updated to describe the new behavior. ChangesIn-Place Update with SHA256 Verification
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/ViewModels/AboutViewModel.cs`:
- Around line 379-388: The current VerifyHashAsync call (via
_updates.VerifyHashAsync with _latest and DownloadedPath) is vulnerable to a
TOCTOU race because the downloaded file can be swapped before the later
replace/copy step; fix by adding a re-check and atomic replace: have the updater
component that performs the actual file replace/copy re-run the SHA256
verification against the same expected hash (pass expected from VerifyHashAsync
or compute expected from metadata) immediately before performing the
copy/replace, and perform the replacement atomically (e.g., replace temp file to
final location or use OS atomic rename) and/or hold an exclusive handle while
copying to prevent a swap. Ensure the verifier and the replacer share the same
expected hash value (from _updates/_latest) and that the final copy occurs only
if the re-verification succeeds.
- Around line 419-423: The updater batch's error path is hidden because the
process is started with CreateNoWindow = true and the app exits before the user
can see failures; update the code that generates/runs the batch (look for the
batch text containing the echo "Update failed — could not copy file." and the
launcher that sets CreateNoWindow) so that on copy failure the script writes the
error to a visible log file or to stderr (e.g., append error details and
%ERRORLEVEL% to a well-known log path) and/or launches the batch without
CreateNoWindow or with a visible MsgBox/pause that will actually be shown;
ensure the error branch includes the actual error info and that the process
launching the batch (the code that sets CreateNoWindow = true) either waits or
allows the window to be visible so failures aren’t silent.
🪄 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: 704ea789-62bb-4b01-95b2-4a63e30ffc36
📒 Files selected for processing (4)
CHANGELOG.mdREADME.mdSysManager/SysManager.Tests/AboutViewModelTests.csSysManager/SysManager/ViewModels/AboutViewModel.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 (3)
README.md (1)
270-272: Update documentation now accurately describes the in-place installer flow.Nice clarification here; this aligns the user-facing docs with the new verify-and-replace behavior.
CHANGELOG.md (1)
47-51: Changelog entry is clear and implementation-consistent.The fix note captures both the old behavior and the new in-place update path well.
SysManager/SysManager.Tests/AboutViewModelTests.cs (1)
144-174: Good edge-case coverage for the new async install guard clauses.These tests validate the new early-return states (
no file/no release info) and match the updated command behavior.
Comment on lines
+379
to
+388
| // Step 1: Verify SHA256 hash before installing. | ||
| DownloadStatus = "Verifying file integrity..."; | ||
| var (verified, expected, actual) = await _updates.VerifyHashAsync(_latest, DownloadedPath); | ||
| if (!verified) | ||
| { | ||
| DownloadStatus = expected != null && actual != null | ||
| ? $"SHA256 mismatch — file may be corrupted. Expected: {expected[..12]}… Got: {actual[..12]}…" | ||
| : "Hash verification failed — file may be corrupted. Try downloading again."; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Hash verification is vulnerable to TOCTOU between validation and replace.
Line 381 verifies the file, but Line 418 copies it later from disk via another process. A local file swap in-between bypasses the integrity check.
🔐 Suggested hardening (re-verify in updater script right before copy)
- var (verified, expected, actual) = await _updates.VerifyHashAsync(_latest, DownloadedPath);
+ var (verified, expected, actual) = await _updates.VerifyHashAsync(_latest, DownloadedPath);
if (!verified)
{
DownloadStatus = expected != null && actual != null
? $"SHA256 mismatch — file may be corrupted. Expected: {expected[..12]}… Got: {actual[..12]}…"
: "Hash verification failed — file may be corrupted. Try downloading again.";
return;
}
+ var expectedHash = (expected ?? actual ?? string.Empty).ToUpperInvariant();
...
- echo Applying update...
+ echo Verifying hash again...
+ for /f "delims=" %%H in ('powershell -NoProfile -Command "(Get-FileHash -Path ''{DownloadedPath}'' -Algorithm SHA256).Hash"') do set "SRC_HASH=%%H"
+ if /I not "%SRC_HASH%"=="{expectedHash}" (
+ echo Update failed — hash changed after verification.
+ pause >NUL
+ exit /b 1
+ )
+ echo Applying update...
copy /Y "{DownloadedPath}" "{currentExe}" >NULAlso applies to: 418-419
🤖 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/AboutViewModel.cs` around lines 379 - 388,
The current VerifyHashAsync call (via _updates.VerifyHashAsync with _latest and
DownloadedPath) is vulnerable to a TOCTOU race because the downloaded file can
be swapped before the later replace/copy step; fix by adding a re-check and
atomic replace: have the updater component that performs the actual file
replace/copy re-run the SHA256 verification against the same expected hash (pass
expected from VerifyHashAsync or compute expected from metadata) immediately
before performing the copy/replace, and perform the replacement atomically
(e.g., replace temp file to final location or use OS atomic rename) and/or hold
an exclusive handle while copying to prevent a swap. Ensure the verifier and the
replacer share the same expected hash value (from _updates/_latest) and that the
final copy occurs only if the re-verification succeeds.
Comment on lines
+419
to
+423
| if errorlevel 1 ( | ||
| echo Update failed — could not copy file. Press any key to exit. | ||
| pause >NUL | ||
| exit /b 1 | ||
| ) |
There was a problem hiding this comment.
Updater copy failures become invisible because the batch runs hidden before shutdown.
The error path at Line 420-Line 422 is unreachable to users in practice: the script runs with CreateNoWindow = true/hidden and the app exits at Line 444. A failed copy can look like “app closed and never restarted.”
🛠️ Minimal fix to avoid silent failures
Process.Start(new ProcessStartInfo
{
FileName = "cmd.exe",
Arguments = $"/C \"{scriptPath}\"",
- UseShellExecute = false,
- CreateNoWindow = true,
- WindowStyle = ProcessWindowStyle.Hidden
+ UseShellExecute = true,
+ CreateNoWindow = false,
+ WindowStyle = ProcessWindowStyle.Normal
});Also applies to: 433-444
🤖 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/AboutViewModel.cs` around lines 419 - 423,
The updater batch's error path is hidden because the process is started with
CreateNoWindow = true and the app exits before the user can see failures; update
the code that generates/runs the batch (look for the batch text containing the
echo "Update failed — could not copy file." and the launcher that sets
CreateNoWindow) so that on copy failure the script writes the error to a visible
log file or to stderr (e.g., append error details and %ERRORLEVEL% to a
well-known log path) and/or launches the batch without CreateNoWindow or with a
visible MsgBox/pause that will actually be shown; ensure the error branch
includes the actual error info and that the process launching the batch (the
code that sets CreateNoWindow = true) either waits or allows the window to be
visible so failures aren’t silent.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+403
to
+405
| var scriptPath = Path.Combine( | ||
| Path.GetDirectoryName(DownloadedPath)!, | ||
| "update.cmd"); |
This was referenced May 13, 2026
laurentiu021
added a commit
that referenced
this pull request
May 22, 2026
Test PR to verify branch protection rules work correctly. Changes: minor punctuation fix (period to exclamation mark). Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
laurentiu021
added a commit
that referenced
this pull request
May 22, 2026
…ication (#274) Previously, Install only launched the new exe from a temp folder without replacing the running executable. Now it: 1. Verifies the SHA256 hash of the downloaded build against the release asset 2. Writes an updater script that waits for the current process to exit 3. Copies the new exe over the original 4. Restarts SysManager from the original path Also handles access-denied gracefully (suggests running as admin). Closes #240 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
Auto-update previously only launched the new exe from a temp folder without replacing the running executable. Users ended up with the old version on next restart.
Changes
Files changed
Testing
Closes #240
Summary by CodeRabbit
Bug Fixes
Tests