Conversation
Contributor
📝 WalkthroughWalkthroughAdds safe tar extraction helpers that compute safe member paths, normalize extracted file/directory permissions, and retry extraction on PermissionError; updates all tar extraction sites in the module to use the new safe extractor. ChangesSafer tar extraction with permission handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/fosslight_util/download.py (1)
1116-1118:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
.crateextraction still bypasses the safe tar wrapper.This branch still uses direct
tarfile.extractall(), so it misses the new permission-retry and post-fix behavior. It should route through_tar_extractall_safe()like the other tar paths.🔧 Suggested patch
elif fname.endswith(".crate"): - with contextlib.closing(tarfile.open(fname, "r:gz")) as t: - t.extractall(path=extract_path) + _tar_extractall_safe(fname, "r:gz", extract_path)🤖 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 `@src/fosslight_util/download.py` around lines 1116 - 1118, The .crate branch currently calls tarfile.extractall directly and should instead use the safe extractor; replace the with contextlib.closing(tarfile.open(fname, "r:gz")) as t: t.extractall(path=extract_path call with a call to _tar_extractall_safe(t, extract_path) so the fname.endswith(".crate") branch reuses the existing _tar_extractall_safe function (keep the tarfile.open(...) context wrapper and variables fname and extract_path).
🤖 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 `@src/fosslight_util/download.py`:
- Around line 1016-1034: _fix_extracted_permissions currently chmods every file
under the extraction root; change it to accept (or compute) the set of paths
extracted by the current archive and only normalize permissions for those files
and their ancestor directories. Specifically: update
_fix_extracted_permissions(path: str) to take an additional parameter like
extracted_paths: Iterable[str] (or compute a set inside the caller), build a
canonical set of absolute file paths and their parent directories under the
given extract_path, then when walking (or when handling the initial path) only
call os.chmod for entries whose absolute path is in that allowed set; apply the
same change to the duplicate logic around lines 1047-1060 so pre-existing files
outside the current archive are not modified. Ensure you reference and update
callers that invoke _fix_extracted_permissions to provide the extracted paths
set.
- Around line 1039-1040: Replace all raw tarfile.extractall(...) calls with the
safe extractor _tar_extractall_safe() (including the .crate extraction branch
currently calling t.extractall(path=extract_path)) so every extraction path
enforces path traversal checks; implement in _tar_extractall_safe() a
Python-3.10-safe fallback that, when tarfile.extractall(filter=...) is
unavailable, iterates members, computes target =
os.path.normpath(os.path.join(extract_path, member.name)), verifies
target.startswith(os.path.abspath(extract_path) + os.sep) before extracting that
member (and skips/raises on violations); and modify _fix_extracted_permissions()
to only chmod existing files/directories and surface or log permission errors
instead of silently swallowing exceptions. Ensure all references to
tarfile.extractall in this module are routed through the updated
_tar_extractall_safe().
---
Outside diff comments:
In `@src/fosslight_util/download.py`:
- Around line 1116-1118: The .crate branch currently calls tarfile.extractall
directly and should instead use the safe extractor; replace the with
contextlib.closing(tarfile.open(fname, "r:gz")) as t:
t.extractall(path=extract_path call with a call to _tar_extractall_safe(t,
extract_path) so the fname.endswith(".crate") branch reuses the existing
_tar_extractall_safe function (keep the tarfile.open(...) context wrapper and
variables fname and extract_path).
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 89d6b2b8-0192-40ae-9671-7ed631ff1783
📒 Files selected for processing (1)
src/fosslight_util/download.py
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/fosslight_util/download.py (2)
1039-1060:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude ancestor directories in the permission-retry set.
_fix_extracted_permissions()only receives explicit directory members plus file paths. If an archive containspkg/file.txtwithout a separatepkg/entry andextract_path/pkgis what caused the originalPermissionError, this retry skips that parent directory, skips the still-missing file, and then fails again on the same member. Please add each file member's parent chain underextract_pathtomember_dirsbefore retrying.🛠️ Minimal fix
def _member_extracted_paths(members, extract_path: str) -> tuple[set, set]: @@ if m.isdir(): dir_paths.add(abs_path) elif m.isfile(): file_paths.add(abs_path) + parent = os.path.dirname(abs_path) + while os.path.commonpath([parent, abs_extract]) == abs_extract: + dir_paths.add(parent) + if parent == abs_extract: + break + parent = os.path.dirname(parent) except ValueError: passAlso applies to: 1108-1109, 1137-1138
🤖 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 `@src/fosslight_util/download.py` around lines 1039 - 1060, The retry logic in _fix_extracted_permissions only iterates explicit member_dirs and skips parent directories that may not be listed (e.g., archive has "pkg/file.txt" but no "pkg/" entry), so before fixing permissions add each file member's parent chain (up to the extraction root) into member_dirs: for every path in member_files, walk its parents and insert them into the member_dirs set (ensuring you only add paths under the intended extract root if available), then proceed with the existing directory-first chmod loop; apply the same parent-chain addition to the two other similar permission-fix blocks noted in the file.
1072-1097:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winManual fallback still permits unsafe tar member types.
The fallback only bounds-checks
member.nameand then callstf.extract(member, ...). That still leaves the older-runtime branch accepting symlink/hardlink members whose link targets escape the destination, and special members like device/FIFO entries, so it is not equivalent tofilter='data'. The stdlibdatafilter explicitly rejects links to absolute/out-of-destination targets and refuses special files. (docs.python.org)Also applies to: 1119-1131
🤖 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 `@src/fosslight_util/download.py` around lines 1072 - 1097, The manual fallback currently only bounds-checks member.name then calls tf.extract(member,...), which still allows symlink/hardlink and special device/FIFO entries to escape or be created; update the members loop (the branch where has_filter is False) to reject non-regular file members: call TarInfo.issym()/islnk()/isdev()/isfifo()/ischr()/isblk() on each member and raise a ValueError for any non-regular type, and for symlink/hardlink also validate member.linkname by resolving os.path.normpath(os.path.join(extract_path, member.linkname)) to an absolute path and ensuring it is inside abs_extract (or simply reject any link entries if you prefer stricter behavior); only allow extraction (tf.extract or tf.extractfile->write) for regular files and directories after these checks to match the stdlib 'data' filter behavior (refer to variables: has_filter, tf, members, member, extract_path, abs_extract, member.name, member.linkname).
🤖 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.
Duplicate comments:
In `@src/fosslight_util/download.py`:
- Around line 1039-1060: The retry logic in _fix_extracted_permissions only
iterates explicit member_dirs and skips parent directories that may not be
listed (e.g., archive has "pkg/file.txt" but no "pkg/" entry), so before fixing
permissions add each file member's parent chain (up to the extraction root) into
member_dirs: for every path in member_files, walk its parents and insert them
into the member_dirs set (ensuring you only add paths under the intended extract
root if available), then proceed with the existing directory-first chmod loop;
apply the same parent-chain addition to the two other similar permission-fix
blocks noted in the file.
- Around line 1072-1097: The manual fallback currently only bounds-checks
member.name then calls tf.extract(member,...), which still allows
symlink/hardlink and special device/FIFO entries to escape or be created; update
the members loop (the branch where has_filter is False) to reject non-regular
file members: call TarInfo.issym()/islnk()/isdev()/isfifo()/ischr()/isblk() on
each member and raise a ValueError for any non-regular type, and for
symlink/hardlink also validate member.linkname by resolving
os.path.normpath(os.path.join(extract_path, member.linkname)) to an absolute
path and ensuring it is inside abs_extract (or simply reject any link entries if
you prefer stricter behavior); only allow extraction (tf.extract or
tf.extractfile->write) for regular files and directories after these checks to
match the stdlib 'data' filter behavior (refer to variables: has_filter, tf,
members, member, extract_path, abs_extract, member.name, member.linkname).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9c91b8ff-c231-4284-be0c-edd9241cd7e2
📒 Files selected for processing (1)
src/fosslight_util/download.py
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by CodeRabbit