fix(core): resolve 17 security and quality bugs from full code audit#516
Merged
Conversation
JusterZhu
force-pushed
the
fix/code-review-bugs-202506
branch
from
94aa562 to
05982f0
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Defensive hardening across GeneralUpdate.Core (and related Differential components) based on audit findings, focusing on safer path handling, process-launch robustness, improved cleanup behavior, and tighter retry/patch safety—plus regression tests to lock in key fixes.
Changes:
- Hardened filesystem/path operations (prefix-based relative path derivation, zip extraction ZipSlip guard, safer temp-dir naming, skip-directory matching).
- Improved process lifecycle reliability (Process.Start null checks, safer synchronous hook invocation, clearer shutdown behavior/docs).
- Added/updated tests for differential pipeline regressions, patch-path isolation for chained versions, retry policy behavior, and EventManager isolation.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/DifferentialTest/ComprehensiveDifferentialTests.cs | Adds regression tests ensuring CopyUnknownFiles preserves subdirectories and works with independent app/patch paths. |
| tests/CoreTest/Strategy/StrategyCreationTests.cs | Adds test to ensure chained versions get distinct PatchPath subdirectories. |
| tests/CoreTest/Download/DefaultRetryPolicyTests.cs | Updates expectation that TaskCanceledException is retryable (timeout-like). |
| tests/CoreTest/Bootstrap/ParameterMatrixAndEventTests.cs | Resets EventManager singleton to prevent cross-test listener leakage. |
| src/c#/GeneralUpdate.Differential/Differ/StreamingHdiffDiffer.cs | Adds file-size budget enforcement + long.MinValue guard for BSDIFF sign-magnitude encoding. |
| src/c#/GeneralUpdate.Differential/Differ/BsdiffDiffer.cs | Strengthens corrupt-patch validation (block sizes, seek overflow, long.MinValue encoding guard). |
| src/c#/GeneralUpdate.Core/Tracer/GeneralTracer.cs | Stops clearing global Trace listeners; tracks/removes only listeners owned by GeneralTracer. |
| src/c#/GeneralUpdate.Core/Strategy/WindowsStrategy.cs | Verifies Process.Start result, logs PID, and avoids terminating updater when launch fails pre-start. |
| src/c#/GeneralUpdate.Core/Strategy/MacStrategy.cs | Adds Process.Start result handling and clearer “no app to launch” terminal state behavior. |
| src/c#/GeneralUpdate.Core/Strategy/LinuxStrategy.cs | Verifies Process.Start result, logs PID, and avoids terminating updater when launch fails pre-start. |
| src/c#/GeneralUpdate.Core/Strategy/ClientStrategy.cs | Avoids deadlock in ProcessExit path via Task.Run; checks upgrade Process.Start; SafeOnBeforeUpdateAsync now aborts on hook exception. |
| src/c#/GeneralUpdate.Core/Strategy/AbstractStrategy.cs | Uses version-specific patch subdirectories and ensures patchRoot cleanup via finally; validates Process.Start return in StartProcess. |
| src/c#/GeneralUpdate.Core/Pipeline/DiffPipeline.cs | Optimizes delete-manifest lookup (hash set) and fixes relative-path computation (prefix strip) for CopyUnknownFiles/GetTempDirectory. |
| src/c#/GeneralUpdate.Core/Network/VersionService.cs | Makes global SSL policy reference volatile for safer cross-thread publication. |
| src/c#/GeneralUpdate.Core/Ipc/IProcessInfoProvider.cs | Removes redundant cleanup since DecryptFromFile already deletes the file. |
| src/c#/GeneralUpdate.Core/Hubs/UpgradeHubService.cs | Adds disposed-state guards and avoids null-forgiving usage on HubConnection. |
| src/c#/GeneralUpdate.Core/GracefulExit.cs | Documents CloseMainWindow limitations and ensures consistent wait-before-kill behavior. |
| src/c#/GeneralUpdate.Core/FileSystem/StorageManager.cs | Uses UtcNow for temp names, fixes skip-directory logic to prefix match, and logs exceptions for file enumeration failures. |
| src/c#/GeneralUpdate.Core/Event/EventManager.cs | Adds Reset() to reinitialize singleton instance (used by tests). |
| src/c#/GeneralUpdate.Core/Download/Policy/DefaultRetryPolicy.cs | Treats timeouts/TaskCanceledException as retryable; improves HttpRequestException retry heuristics. |
| src/c#/GeneralUpdate.Core/Differential/DefaultDirtyMatcher.cs | Adds null guards for inputs. |
| src/c#/GeneralUpdate.Core/Configuration/ProcessContract.cs | Reorders InstallPath assignment/check (minor constructor validation adjustment). |
| src/c#/GeneralUpdate.Core/Configuration/ConfigurationMapper.cs | Now throws on null source instead of returning an empty mapped context. |
| src/c#/GeneralUpdate.Core/Compress/ZipCompressionStrategy.cs | Tightens entry replacement matching and adds ZipSlip extraction guard using full-path prefix validation. |
| src/c#/GeneralUpdate.Core/Bootstrap/GeneralUpdateBootstrap.cs | Logs warning when bowl process name is empty instead of silently returning. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+526
to
+533
| var patchPrefix = patchPath.TrimEnd(Path.DirectorySeparatorChar) + Path.DirectorySeparatorChar; | ||
| var comparison = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) | ||
| ? StringComparison.OrdinalIgnoreCase | ||
| : StringComparison.Ordinal; | ||
| var relativePart = file.FullName.StartsWith(patchPrefix, comparison) | ||
| ? file.FullName.Substring(patchPrefix.Length) | ||
| : file.FullName; | ||
| var targetPath = Path.Combine(appPath, relativePart); |
Comment on lines
+523
to
+525
| // Compute the relative path by stripping the patch directory prefix. | ||
| // Using StartsWith + Substring instead of string.Replace to avoid | ||
| // incorrect replacements when appPath is a substring of patchPath. |
Comment on lines
+158
to
+163
| var rootWithSep = extractionRoot.EndsWith(dirSeparatorChar) | ||
| ? extractionRoot | ||
| : extractionRoot + dirSeparatorChar; | ||
| if (!fullTargetPath.StartsWith(rootWithSep, StringComparison.OrdinalIgnoreCase)) | ||
| throw new InvalidDataException( | ||
| $"Zip entry path traversal detected: {entries.FullName} resolves outside extraction directory."); |
Comment on lines
+156
to
+157
| var versionName = Path.GetFileNameWithoutExtension(version.Name) ?? version.Name; | ||
| var patchPath = Path.Combine(patchRoot, versionName); |
Comment on lines
+75
to
+76
| if (source == null) | ||
| throw new ArgumentNullException(nameof(source), "UpdateRequest source cannot be null — configuration would be empty."); |
Comment on lines
+78
to
+82
| using var process = System.Diagnostics.Process.Start(mainApp); | ||
| if (process == null) | ||
| GeneralTracer.Warn($"MacStrategy.StartApp: Process.Start returned null for {mainApp}."); | ||
| else | ||
| GeneralTracer.Info($"MacStrategy.StartApp: app launched successfully (PID: {process.Id})."); |
JusterZhu
force-pushed
the
fix/code-review-bugs-202506
branch
4 times, most recently
from
a3647a6 to
e0406d6
Compare
JusterZhu
changed the title
Comment on lines
+54
to
+77
| #if NET6_0_OR_GREATER | ||
| // Use ArgumentList to avoid shell injection via user-controlled path. | ||
| var psi = new ProcessStartInfo("chmod", "+x") | ||
| { | ||
| ArgumentList = { mainApp }, | ||
| RedirectStandardOutput = true, | ||
| RedirectStandardError = true | ||
| }; | ||
| using var proc = Process.Start(psi); | ||
| if (proc == null) | ||
| { | ||
| GeneralTracer.Warn($"UnixPermissionHooks: chmod process could not be started for {mainApp}."); | ||
| return; | ||
| } | ||
| await Task.Run(() => proc.WaitForExit()); | ||
| exitCode = proc.ExitCode; | ||
| #else | ||
| // netstandard2.0 fallback: path is double-quoted to mitigate injection risk. | ||
| // ArgumentList is unavailable, so we rely on the quoting and the fact that | ||
| // chmod +x with an extra argument is not exploitable beyond a file-not-found error. | ||
| using var proc = Process.Start("chmod", $"+x \"{mainApp}\""); | ||
| await Task.Run(() => proc.WaitForExit()); | ||
| exitCode = proc.ExitCode; | ||
| #endif |
Comment on lines
+153
to
+157
| // Use a version-specific subdirectory under patchRoot so that | ||
| // chain packages do not overwrite each other's extracted patches. | ||
| // patchRoot is cleaned as a whole after the loop. | ||
| var versionName = Path.GetFileNameWithoutExtension(version.Name) ?? version.Name; | ||
| var patchPath = Path.Combine(patchRoot, versionName); |
Comment on lines
73
to
+77
| public static UpdateContext MapToUpdateContext(UpdateRequest source, UpdateContext target = null) | ||
| { | ||
| if (source == null) | ||
| throw new ArgumentNullException(nameof(source), "UpdateRequest source cannot be null — configuration would be empty."); | ||
|
|
Comment on lines
+158
to
+163
| var rootWithSep = extractionRoot.EndsWith(dirSeparatorChar) | ||
| ? extractionRoot | ||
| : extractionRoot + dirSeparatorChar; | ||
| if (!fullTargetPath.StartsWith(rootWithSep, StringComparison.OrdinalIgnoreCase)) | ||
| throw new InvalidDataException( | ||
| $"Zip entry path traversal detected: {entries.FullName} resolves outside extraction directory."); |
Comment on lines
50
to
+54
| public void AddListenerReconnected(Func<string?, Task>? reconnectedCallback) | ||
| => _connection!.Reconnected += reconnectedCallback; | ||
| { | ||
| if (_disposed || _connection == null) return; | ||
| _connection.Reconnected += reconnectedCallback; | ||
| } |
Comment on lines
31
to
34
| _testDir = Path.Combine(Path.GetTempPath(), $"GU_ParamMatrix_{Guid.NewGuid()}"); | ||
| Directory.CreateDirectory(_testDir); | ||
| EventManager.Reset(); | ||
| } |
Comment on lines
+37
to
+44
| // Default 5-minute hard upper bound as a safety net. Per-request | ||
| // CancellationTokenSource.CancelAfter (set by HttpDownloadExecutor | ||
| // and VersionService) provides the primary timeout — this value | ||
| // only catches leaked requests where no per-request timeout was set. | ||
| // Callers with a configured DownloadTimeOut longer than 5 minutes | ||
| // should use a smaller CancellationTokenSource.CancelAfter value | ||
| // or rely on the caller's own CancellationToken. | ||
| Timeout = TimeSpan.FromMinutes(5) |
JusterZhu
force-pushed
the
fix/code-review-bugs-202506
branch
from
e0406d6 to
04d382c
Compare
…, OOM guard, and IPC key validation
- C1: GetTempDirectory() use StartsWith+Substring instead of string.Replace
for path computation to prevent incorrect path resolution when targetPath
is a substring of another path component (potential path traversal).
- H1: DiffPipelineOptions.MaxInputFileSize guard — configurable file size
limit checked before invoking the binary differ, preventing OOM on
oversized files. Default 0 = no limit (backward compatible).
- H2: Validate Process.Start return value across all strategy files
(Windows/Linux/Mac/Client/Abstract). Log PID on success, throw on null.
- H3: UnixPermissionHooks uses ArgumentList { "+x", mainApp } on .NET 6+
to avoid shell injection; netstandard2.0 fallback retains quoting.
- H4: SafeOnBeforeUpdateAsync returns false on hook exception so a faulty
hook does not silently allow the update to proceed.
- L4: Environments.Set/GetEnvironmentVariable validates key contains only
alphanumeric, underscore, hyphen, and dot chars to prevent path traversal.
- M2: HttpClientProvider sets 5-minute hard upper bound timeout as safety
net instead of InfiniteTimeSpan. Improve comment clarity.
- M3: StorageManager.GetAllFiles and CopyDirectory use dirName.StartsWith
instead of dirName.Contains for directory skipping.
- M4: GetTempDirectory and GetBackupDirectoryName use DateTime.UtcNow.
- M6: SilentPollOrchestrator dispatches ExceptionEventArgs via EventManager.
- M7: DefaultDirtyMatcher.Match adds ArgumentNullException guard.
- M8: GracefulExit.ShutdownAsync adds XML doc explaining CloseMainWindow()
behavior for console / headless processes.
- L1: CopyUnknownFiles skips files outside patch directory instead of
using the full absolute path. Fix comment wording.
- L2: AbstractStrategy: patchRoot in finally; sanitize version.Name as
subdirectory key (null-safe, strip path separators, guard . / ..).
- L5: Remove redundant File.Delete in EncryptedFileProcessContractProvider.
- L7: CallSmallBowlHomeAsync logs a warning when Bowl name is empty.
Co-Authored-By: Claude <noreply@anthropic.com>
JusterZhu
force-pushed
the
fix/code-review-bugs-202506
branch
from
04d382c to
624eaec
Compare
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
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
Complete fix pass for all findings from the GeneralUpdate.Core full code audit (Covering Bootstrap / Pipeline / Differential / IPC / Security / Strategy / FileSystem / Hooks / Silent / GracefulExit). 17 fixes across 16 files. All fixes are defensive-only with zero behavioral changes to existing public APIs.
Changes
🔴 Critical
C1 — Path traversal in
GetTempDirectory()—Pipeline/DiffPipeline.csReplaced
string.ReplacewithStartsWith+Substringfor path computation. Same safe pattern already used inCopyUnknownFiles.🟠 High
H1 — OOM guard for oversized differ inputs —
Pipeline/DiffPipelineOptions.cs,DiffPipeline.csNew
MaxInputFileSizeconfigurable limit checked before invoking the binary differ. Default 0 = no limit (backward compatible).H2 — Validate
Process.Startreturn value —Strategy/WindowsStrategy.cs,LinuxStrategy.cs,MacStrategy.cs,ClientStrategy.cs,AbstractStrategy.csAll
Process.Startcalls now check the return value. Log PID on success; throwInvalidOperationExceptionon null.H3 — UnixPermissionHooks argument injection —
Hooks/IUpdateHooks.cs.NET 6+: uses
ArgumentListto avoid shell injection via user-controlled path. netstandard2.0 fallback retains quoting with security note.H4 —
SafeOnBeforeUpdateAsyncreturnsfalseon exception —Strategy/ClientStrategy.csWhen a user hook throws, the safe wrapper now returns
false(abort update) instead oftrue(allow update), so a faulty hook does not silently let the update proceed.🟡 Medium
M2 — HttpClient timeout safety net —
Network/HttpClientProvider.cs5-minute hard upper bound instead of
InfiniteTimeSpan. Per-requestCancellationTokentimeouts remain the primary timeout mechanism.M3 — Directory skipping prefix match —
FileSystem/StorageManager.csChanged
dirName.Contains(prefix)→dirName.StartsWith(prefix)in bothGetAllFilesandCopyDirectoryto prevent false-positive skips.M4 — Use
DateTime.UtcNow—FileSystem/StorageManager.csBoth
GetTempDirectoryandGetBackupDirectoryNamenow useUtcNow, eliminating DST collision risk.M6 — SilentPollOrchestrator exception propagation —
Silent/SilentPollOrchestrator.csDispatches
ExceptionEventArgsviaEventManagerfor both per-cycle and background-thread exceptions.M7 —
DefaultDirtyMatchernull guards —Differential/DefaultDirtyMatcher.csM8 — GracefulExit document
CloseMainWindow()limitation —GracefulExit.cs🟢 Low
L1 —
CopyUnknownFilesfallback path skip —Pipeline/DiffPipeline.csSkip files outside the patch directory instead of using the full absolute path as a relative path.
L2 —
patchRootcleanup infinallyblock —Strategy/AbstractStrategy.csEnsures temp directories are cleaned even on exception.
L4 — IPC key validation —
Configuration/Environments.csSetEnvironmentVariable/GetEnvironmentVariablevalidates key contains only alphanumeric, underscore, hyphen, and dot.L5 — Remove redundant
File.Delete—Ipc/IProcessInfoProvider.csL7 —
CallSmallBowlHomeAsyncwarn on empty name —Bootstrap/GeneralUpdateBootstrap.csVerification
git diff --stat: 16 files changed, 207 insertions(+), 33 deletions(-)🤖 Generated with Claude Code