Repository navigation
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughExpand-ZipArchive now attempts extraction through .NET ZIP APIs. It rejects archive entries that escape the destination and falls back to Expand-Archive for other extraction errors. A test checks that a traversal entry does not create a file outside the destination. ChangesZIP extraction
Priority: ⬇️ Low Merge Risk: 🟡 Moderate · up to Faster ZIP extraction is a good change. However, on modern PowerShell a malicious archive's path-traversal error currently sends it to the older extractor instead of rejecting it. Relative paths may also point to the wrong archive or destination. Both should be fixed before merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The compatibility extraction path does not explicitly confine archive entries to the destination directory. Recovery also retries against potentially partial output without undoing earlier writes. The impact could extend beyond the application directory, but exposure depends on runtime compatibility and attacker control of an accepted archive. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
9030e76 to
5f13b64
Compare
5f13b64 to
2a5f719
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
cdb1e833-ce91-477e-8268-8ca96b45e384
📒 Files selected for processing (2)
lib/decompress.ps1test/Scoop-Decompress.Tests.ps1
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.
| if (!(Test-Path $DestinationPath)) { | ||
| [System.IO.Directory]::CreateDirectory($DestinationPath) | Out-Null | ||
| } | ||
| $destFull = [System.IO.Path]::GetFullPath($DestinationPath) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Resolve both paths using PowerShell’s location before calling .NET.
If a caller supplies relative paths after changing PowerShell location, .NET can resolve $Path and $DestinationPath against a different process directory. The new extraction path can then extract the wrong archive or write to the wrong destination; -Removal still acts on the PowerShell-resolved $Path. Resolve both paths to absolute filesystem paths before directory creation or either .NET extraction branch. (learn.microsoft.com)
| if ($_.Exception.Message -like "*attempts path traversal*") { | ||
| throw | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Rethrow native ZIP path-traversal failures.
On a runtime with the three-argument ExtractToDirectory overload, .NET rejects an escaping entry with its own IOException, not the custom message checked here. This catch then sends the rejected archive to Expand-Archive. The fallback’s handling determines whether the entry is skipped or extracted, so this function no longer reliably rejects the archive. Detect traversal independently of exception text, or validate entries before allowing either extraction path to fall back. Cover the native branch in the traversal test. (learn.microsoft.com)
…versal with tests
026d80d to
7008e09
Compare
| $legacyDest = "$working_dir\legacy_extract" | ||
| New-Item -ItemType Directory -Path $legacyDest -Force | Out-Null | ||
| try { | ||
| Expand-ZipArchive -Path $test -DestinationPath $legacyDest -ForceLegacyExtract |
There was a problem hiding this comment.
Legacy test uses deleted archive
The earlier ZIP removal test deletes ZipTest.zip, but this test then passes the same path to Expand-ZipArchive. Because the archive is created in BeforeAll and is not restored between tests, this test fails on a missing archive instead of exercising legacy extraction. Run it before the removal test or restore the archive.
📋 Problème Traité
Dans
lib/decompress.ps1, la fonctionExpand-ZipArchiveutiliseMicrosoft.PowerShell.Archive\Expand-Archivepour extraire les archives ZIP lorsque 7-Zip n'est pas utilisé :Conséquences :
Expand-Archiveest un script PowerShell procédural qui itère sur chaque entrée de l'archive, effectuant des résolutions de chemins, des contrôles de sécurité et des créations de répertoires individuels via des cmdlets PowerShell lentes.💡 Solution Apportée
Utilisation directe de la BCL .NET (
System.IO.Compression.FileSystem), intégrée nativement dans Windows depuis .NET Framework 4.5 et dans .NET Core / PowerShell 7+ :[System.IO.Compression.ZipFile]::ExtractToDirectory($Path, $DestinationPath, $true).ZipArchiveEntrysans passer par la tuyauterie PowerShell.Expand-Archiveen cas d'anomalie imprévue.🔗 Issue Associée
Amélioration des performances d'extraction des archives ZIP sans dépendance 7-Zip obligatoire.
✅ Validation & Actions Réalisées
Note
extract_dir, suppression des archives temporaires et compatibilité multi-architectures.perf(decompress): accelerate zip extraction using native .NET ZipFile BCL🧪 Reproduction de l'Erreur
Script de mesure avant optimisation avec
Expand-Archive:🔬 Test de la Correction
Script de test de la correction avec
[System.IO.Compression.ZipFile]:The PR is not ready to merge because the new legacy extraction test runs against a deleted fixture.
Findings
Summary
The PR replaces the default PowerShell ZIP extraction path with .NET extraction, adds upfront entry-path validation, and introduces tests for traversal rejection and the legacy per-entry path.
Reviews (7) · Last reviewed commit: "fix(decompress): validate zip slip path ..."