Potential fix for code scanning alert no. 4: Arbitrary file access during archive extraction ("Zip Slip") - #184
Conversation
…ring archive extraction ("Zip Slip")
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
🚥 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)
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 |
There was a problem hiding this comment.
Pull request overview
This PR addresses code scanning alert #4 (Zip Slip / arbitrary file access during archive extraction) by adding explicit validation of tar header names before extracting files in the test helper archive extractor.
Changes:
- Reject empty tar entry names.
- Reject absolute paths.
- Add a path traversal (
..segment) validation step before the existing “flat archive only” check.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
copilot suggestion Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@stembuild/test/helpers/helpers.go`:
- Around line 116-125: The path validation logic applies checks inconsistently
by normalizing backslashes to forward slashes only after performing
absolute-path and subdirectory checks on the raw name. Move the normalization
step (strings.ReplaceAll call that converts backslashes to forward slashes) to
the beginning of the validation logic, before the filepath.IsAbs check, and
apply all subsequent validations including the filepath.Base comparison to the
normalized path instead of the raw name. This ensures that backslash separators
are handled consistently across all checks, preventing entries like dir\file
from bypassing the subdirectory detection on Unix systems.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: d327fb12-2fbc-4214-be52-6308616a44f1
📒 Files selected for processing (1)
stembuild/test/helpers/helpers.go
Copilot suggestion Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Note
Copilot generated fix
Potential fix for https://github.com/cloudfoundry/bosh-windows-stemcell-builder/security/code-scanning/4
To fix this safely, validate each archive entry name with explicit deny rules before using it in any filesystem operation. Keep existing behavior (flat archive only) but add strict checks for:
..path segment (after normalization and slash unification)Best targeted fix in
stembuild/test/helpers/helpers.goinsideextractArchive:name := h.Name, reject empty names.filepath.IsAbs(name).\to/) and reject any path containing..segment.filepath.Base(name) != name) to preserve intended functionality.No new imports are needed;
stringsandfilepathare already imported.Suggested fixes powered by Copilot Autofix. Review carefully before merging.