#17 Replacing DotNetZip with System.IO.Compression#18
Conversation
Updated zip file handling in ShuffleDeployer and ShuffleSolutionImport to use .NET's built-in System.IO.Compression library. Removed DotNetZip references from project and package configurations. Improved file path handling with Path.Combine and added error handling for missing .cdpkg files.
The changes streamline the `Ionic.Zip.xml` documentation by removing detailed descriptions, remarks, examples, and parameter explanations for various classes and members in the Ionic.Zip and Ionic.Zlib libraries. This includes properties and methods in classes such as `ZipEntry`, `ZipFile`, `ParallelDeflateOutputStream`, `ZlibStream`, and `CRC32`. Additionally, the `Ionic.Zip.xml` file has been removed from the project file (`Rappen.XTB.Shuffle.csproj`), indicating that the project may no longer require this documentation or has shifted its resource management. These modifications may impact the clarity and usability of the documentation for developers relying on comprehensive guidance for implementation and troubleshooting.
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the legacy DotNetZip library with the built-in System.IO.Compression APIs to address high-severity vulnerabilities and updates related package references across projects.
- Swaps out all
Ionic.Zipusage forSystem.IO.Compressionin import and deploy code. - Removes the DotNetZip package and adds
System.IO.Compressionpackages inpackages.config. - Updates nuspec metadata, config binding redirects, and project references to use the new assemblies and bump dependency versions.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| shared/Xrm.Shuffle.Core/ShuffleSolutionImport.cs | Replaced ZipFile.Read with ZipFile.OpenRead and ExtractToFile |
| XTB/packages.config | Removed DotNetZip, added System.IO.Compression packages, updated versions |
| XTB/app.config | Updated bindingRedirects for bumped dependency versions |
| XTB/ShuffleRunner.nuspec | Bumped copyright, replaced DotNetZip.dll entry |
| XTB/ShuffleDeployer.nuspec | Bumped copyright, replaced DotNetZip.dll entry |
| XTB/ShuffleBuilder.nuspec | Bumped copyright, replaced DotNetZip.dll entry |
| XTB/Rappen.XTB.Shuffle.csproj | Removed DotNetZip references, added System.IO.Compression and updated other refs |
| XTB/Deployer/ShuffleDeployer.cs | Replaced ZipFile usage with ZipArchive, added extraction logic |
Comments suppressed due to low confidence (2)
XTB/ShuffleRunner.nuspec:22
- Typo in ‘officiall’; it should be spelled ‘official’.
<releaseNotes>First officiall release!</releaseNotes>
shared/Xrm.Shuffle.Core/ShuffleSolutionImport.cs:284
- [nitpick] The variable ‘definitionpath’ uses non-standard casing; consider renaming to ‘definitionPath’ to follow C# camelCase conventions.
string solutionFilePath = Path.Combine(definitionpath, "solution.xml");
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| { | ||
| container.StartSection("ExtractVersionFromSolutionZip"); | ||
| using (var zip = ZipFile.Read(filename)) | ||
| string solutionFilePath = Path.Combine(definitionPath, "solution.xml"); |
There was a problem hiding this comment.
The variable definitionPath is used but not declared in this method. This should likely use the class field, which appears to be a private member.
| string solutionFilePath = Path.Combine(definitionPath, "solution.xml"); | |
| string solutionFilePath = Path.Combine(this.definitionPath, "solution.xml"); |
| zip.GetEntry("solution.xml").ExtractToFile(solutionFilePath, true); | ||
| } | ||
| if (!System.IO.File.Exists(definitionpath + "\\solution.xml")) | ||
| if (!File.Exists(solutionFilePath)) | ||
| { | ||
| throw new Exception("Unable to unzip solution.xml from file: " + filename); | ||
| throw new FileNotFoundException($"Unable to unzip solution.xml from file: {filename}, invalid solution file."); |
There was a problem hiding this comment.
The error message is misleading. A FileNotFoundException suggests the input file doesn't exist, but the actual issue is that solution.xml wasn't found inside the zip archive. Consider using a more appropriate exception type like InvalidDataException.
| var normalizedPath = Path.GetFullPath(fullPath); | ||
|
|
||
| // Validate that the normalized path is within the target folder | ||
| if (!normalizedPath.StartsWith(folder, StringComparison.Ordinal)) |
There was a problem hiding this comment.
The path traversal validation should use StringComparison.OrdinalIgnoreCase instead of StringComparison.Ordinal to handle case-insensitive file systems like Windows, which could allow bypassing the security check with different casing.
| if (!normalizedPath.StartsWith(folder, StringComparison.Ordinal)) | |
| if (!normalizedPath.StartsWith(folder, StringComparison.OrdinalIgnoreCase)) |
| } | ||
|
|
||
| // Ensure directory exists | ||
| Directory.CreateDirectory(Path.GetDirectoryName(normalizedPath)); |
There was a problem hiding this comment.
Potential null reference exception if Path.GetDirectoryName(normalizedPath) returns null, which can happen for root paths. This should be null-checked before calling Directory.CreateDirectory.
| Directory.CreateDirectory(Path.GetDirectoryName(normalizedPath)); | |
| var dirName = Path.GetDirectoryName(normalizedPath); | |
| if (dirName != null) | |
| { | |
| Directory.CreateDirectory(dirName); | |
| } |
Upgraded Microsoft.CrmSdk.Workflow to 9.0.2.60 and NuGet.Protocol to 7.3.0 to ensure compatibility and access to the latest features and fixes.
This is due to high severity vulnerabilities.