Add explicit symlink handling in tarball creation #36
+221
−0
Conversation
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
Test Coverage Reporttotal: (statements) 77.0% Coverage by function |
Skip symlinks during tarball walk to avoid security risks (symlinks pointing outside repo) and issues with broken symlinks or loops. Log a warning to stderr for each skipped symlink.
yiftach-armis
force-pushed
the
ci/check-checkout-update
branch
from
ea6d163 to
03449a7
Compare
Add comprehensive test coverage for symlink scenarios: - Symlinks pointing to directories - Broken symlinks pointing to non-existent targets These tests ensure the symlink skip logic handles all edge cases correctly.
There was a problem hiding this comment.
Pull request overview
This pull request adds explicit symlink detection and skipping during tarball creation to prevent security risks from symlinks pointing outside the repository, avoid errors from broken symlinks, and prevent potential infinite loops from circular symlinks. Symlinks are now logged with a warning to stderr when encountered.
Changes:
- Added symlink detection check in
tarGzDirectoryto skip symlinks during tarball creation - Added warning message to stderr when symlinks are skipped
- Added comprehensive test coverage for file symlinks, directory symlinks, and broken symlinks
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/scan/repo/repo.go | Added symlink detection logic to skip symlinks during tarball creation with warning message |
| internal/scan/repo/repo_test.go | Added three comprehensive test cases covering file symlinks, directory symlinks, and broken symlinks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+184
to
+189
| // Skip symlinks to avoid security risks (symlinks pointing outside repo) | ||
| // and potential issues (broken symlinks, loops) | ||
| if info.Mode()&os.ModeSymlink != 0 { | ||
| fmt.Fprintf(os.Stderr, "Warning: skipping symlink %s\n", relPath) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The calculateDirSize function should also skip symlinks for consistency with tarGzDirectory. Currently, tarGzDirectory explicitly skips symlinks (lines 184-189), but calculateDirSize does not. While filepath.Walk doesn't follow symlinks by default, calculateDirSize may still count the symlink's metadata size, which could lead to a mismatch between the calculated size and the actual tarball size. Add the same symlink check after line 248 in calculateDirSize to ensure consistent behavior.
The calculateDirSize function now skips symlinks to match the behavior of tarGzDirectory. This ensures the calculated size matches the actual tarball size and maintains consistent symlink handling throughout the codebase. Addresses review comment from @copilot-pull-request-reviewer.
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.
Description
Add explicit symlink detection and skipping in the tarball creation process to prevent security risks and extraction errors. Symlinks are now logged with a warning to stderr when encountered.
Type of Change
Testing
Checklist
Additional Notes
Symlinks are now skipped during tarball creation to avoid: