Fix Tmds.DBus CVE and Zip Slip archive extraction - #38
Conversation
Bumps Tmds.DBus.Protocol from 0.20.0 to 0.21.3 --- updated-dependencies: - dependency-name: Tmds.DBus.Protocol dependency-version: 0.21.3 dependency-type: direct:production dependency-group: nuget ... Signed-off-by: dependabot[bot] <support@github.com>
dependabot
Bot
added
dependencies
Add ArchivePath.GetSafeExtractPath and use it in ExtractAllContents. Strip archive path components when unzipping updates to temp. Reference Tmds.DBus.Protocol 0.21.3 directly from the Shell project. Co-authored-by: Michael <Narehood@users.noreply.github.com>
cursor
Bot
changed the title
ZipArchive lives in the System.IO.Compression framework assembly on net481; reference it from the test project like XenCenterLib does. Co-authored-by: Michael <Narehood@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Warning Review limit reached
Next review available in: 38 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (2)
📝 WalkthroughWalkthroughThe change adds safe archive path resolution, applies it during extraction, tests traversal handling, restricts update extraction to leaf filenames, and pins ChangesArchive extraction safety
Package version pinning
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
XenCenterLib.Tests/ArchivePathTests.cs (1)
1-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the null/empty argument guard clauses.
The XML doc for
GetSafeExtractPathdocumentsArgumentNullExceptionfor a null/emptydestinationDirectoryandInvalidDataExceptionfor a null/emptyentryName. No test in this file exercises these guard clauses. Add two smallFacttests to lock in this documented contract.🤖 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 `@XenCenterLib.Tests/ArchivePathTests.cs` around lines 1 - 114, Add two Fact tests for GetSafeExtractPath: verify null or empty destinationDirectory throws ArgumentNullException, and null or empty entryName throws InvalidDataException. Keep the tests focused on the documented guard-clause contract without requiring filesystem setup.
🤖 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 `@XenModel/Actions/Updates/DownloadUnzipUpdateAction.cs`:
- Around line 94-99: Update the extraction path construction in ExtractFile to
create and use a unique temporary subdirectory, using the existing
Path.GetRandomFileName() pattern from the downloaded archive flow, before
combining it with leafName. Ensure each extraction gets its own directory so
identical archive entries and concurrent updates cannot share output paths.
---
Nitpick comments:
In `@XenCenterLib.Tests/ArchivePathTests.cs`:
- Around line 1-114: Add two Fact tests for GetSafeExtractPath: verify null or
empty destinationDirectory throws ArgumentNullException, and null or empty
entryName throws InvalidDataException. Keep the tests focused on the documented
guard-clause contract without requiring filesystem setup.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d7827394-752d-43e4-89d3-c0d013a650bf
📒 Files selected for processing (8)
Directory.Packages.propsXcpNgCenter.Shell/XcpNgCenter.Shell.csprojXcpNgCenter.Shell/packages.lock.jsonXenCenterLib.Tests/ArchivePathTests.csXenCenterLib.Tests/XenCenterLib.Tests.csprojXenCenterLib/Archive/ArchiveIterator.csXenCenterLib/Archive/ArchivePath.csXenModel/Actions/Updates/DownloadUnzipUpdateAction.cs
Use GetRandomFileName for each update extract directory so identical leaf names and concurrent extractions do not collide. Add guard-clause tests for null/empty destination and entryName. Co-authored-by: Michael <Narehood@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
Pinned Tmds.DBus.Protocol at 0.21.3.
Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting
@dependabot rebase.Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR:
@dependabot rebasewill rebase this PR@dependabot recreatewill recreate this PR, overwriting any edits that have been made to it@dependabot show <dependency name> ignore conditionswill show all of the ignore conditions of the specified dependency@dependabot ignore <dependency name> major versionwill close this group update PR and stop Dependabot creating any more for the specific dependency's major version (unless you unignore this specific dependency's major version or upgrade to it yourself)@dependabot ignore <dependency name> minor versionwill close this group update PR and stop Dependabot creating any more for the specific dependency's minor version (unless you unignore this specific dependency's minor version or upgrade to it yourself)@dependabot ignore <dependency name>will close this group update PR and stop Dependabot creating any more for the specific dependency (unless you unignore this specific dependency or upgrade to it yourself)@dependabot unignore <dependency name>will remove all of the ignore conditions of the specified dependency@dependabot unignore <dependency name> <ignore condition>will remove the ignore condition of the specified dependency and ignore conditionsYou can disable automated security fix PRs for this repo from the Security Alerts page.
Summary by CodeRabbit
Security
Bug Fixes
Tests