fix(security): validate remote result archives - #620
Open
njzjz-bot wants to merge 1 commit into
Open
Conversation
Validate tar and zip manifests before extraction, reject unsafe paths and file types, and write regular files through atomic replacements across all remote result backends. Closes deepmodeling#604 Coding-Agent: Codex Codex-Version: codex-cli 0.144.4 Model: gpt-5.6-sol Reasoning-Effort: xhigh
dosubot
Bot
added
the
size:L
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #620 +/- ##
==========================================
+ Coverage 48.38% 50.39% +2.01%
==========================================
Files 40 41 +1
Lines 3960 4143 +183
==========================================
+ Hits 1916 2088 +172
- Misses 2044 2055 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
📝 WalkthroughWalkthroughAdds centralized safe tar and zip extraction with path, member, symlink, special-file, and atomic-write validation. HDFS, SSH, OpenAPI, and cloud-server result downloads now use these helpers, with comprehensive security tests. ChangesArchive extraction safety
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 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 |
![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: 3
🧹 Nitpick comments (2)
tests/test_archive.py (2)
82-86: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCover additional portable path collisions.
Add case-folding and trailing-dot aliases such as
Result.txt/result.txtandresult.txt/result.txt.. The current manifests only exercise./and Unicode-normalization aliases.Also applies to: 255-259
🤖 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 `@tests/test_archive.py` around lines 82 - 86, Add case-folding and trailing-dot collision pairs to the unsafe_manifests test data in the archive safety tests, including Result.txt/result.txt and result.txt/result.txt. Preserve the existing dot-slash, Unicode-normalization, and parent-child collision cases.
76-78: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that validation does not create the output root.
These assertions allow an empty
output_dirto be created before validation, contrary tosafe_extract_tarandsafe_extract_zip’s documented contract.Proposed assertion
- self.assertFalse( - os.path.exists(os.path.join(output_dir, "valid.txt")) - ) + self.assertFalse(os.path.exists(output_dir))Also applies to: 249-251
🤖 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 `@tests/test_archive.py` around lines 76 - 78, Update the validation assertions in the archive extraction tests around the valid.txt checks to also assert that the output_dir itself does not exist after validation rejects the archive. Apply the same assertion to both the tar and zip cases, preserving the existing checks that no output file is created.
🤖 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 `@dpdispatcher/utils/archive.py`:
- Around line 155-163: Update _prepare_root to reject a pre-existing symlink or
Windows junction at destination before calling realpath or validating the
resolved directory. Raise UnsafeArchiveError for these link-based roots, while
preserving normal directory creation and resolution for genuine directories.
- Around line 141-147: Update the manifest validation around the non-directory
branch to track encountered directory prefixes in a dedicated set rather than
scanning all entries in path_types for each file. Use that set to detect
file-versus-directory conflicts while preserving the existing UnsafeArchiveError
and canonical_path behavior.
In `@tests/test_archive.py`:
- Around line 102-124: Add root-destination tests alongside
test_rejects_existing_symlink_components and the related cases, covering both
public extraction entry points. Make output_dir itself a symlink or Windows
junction to an outside directory, invoke each extractor with an archive, and
assert UnsafeArchiveError is raised without creating files outside the trusted
destination. Preserve platform skips where symlinks or junctions are
unavailable.
---
Nitpick comments:
In `@tests/test_archive.py`:
- Around line 82-86: Add case-folding and trailing-dot collision pairs to the
unsafe_manifests test data in the archive safety tests, including
Result.txt/result.txt and result.txt/result.txt. Preserve the existing
dot-slash, Unicode-normalization, and parent-child collision cases.
- Around line 76-78: Update the validation assertions in the archive extraction
tests around the valid.txt checks to also assert that the output_dir itself does
not exist after validation rejects the archive. Apply the same assertion to both
the tar and zip cases, preserving the existing checks that no output file is
created.
🪄 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: CHILL
Plan: Pro
Run ID: 447a8546-443d-42be-b674-2f42c3a22cbc
📒 Files selected for processing (7)
dpdispatcher/contexts/hdfs_context.pydpdispatcher/contexts/openapi_context.pydpdispatcher/contexts/ssh_context.pydpdispatcher/machines/openapi.pydpdispatcher/utils/archive.pydpdispatcher/utils/dpcloudserver/zip_file.pytests/test_archive.py
Comment on lines
+141
to
+147
| if not is_directory: | ||
| descendant_prefix = path_key + "/" | ||
| if any(key.startswith(descendant_prefix) for key in path_types): | ||
| raise UnsafeArchiveError( | ||
| "Archive file conflicts with an existing directory member: " | ||
| f"{canonical_path!r}" | ||
| ) |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Avoid quadratic manifest validation on remote archives.
Each file scans every previously seen path, so an archive with many sibling files causes O(n²) validation and can stall result downloads. Track previously encountered parent paths in a set instead.
Proposed fix
validated: List[_ValidatedMember] = []
path_types: Dict[str, bool] = {}
+ parent_paths = set()
for source, name, is_directory, mode in candidates:
parts = _canonical_member_parts(name, is_directory)
canonical_path = "/".join(parts)
path_key = unicodedata.normalize("NFC", canonical_path).casefold()
+ member_parent_paths = []
for index in range(1, len(parts)):
parent_key = unicodedata.normalize(
"NFC", "/".join(parts[:index])
).casefold()
+ member_parent_paths.append(parent_key)
if path_types.get(parent_key) is False:
raise UnsafeArchiveError(...)
if not is_directory:
- descendant_prefix = path_key + "/"
- if any(key.startswith(descendant_prefix) for key in path_types):
+ if path_key in parent_paths:
raise UnsafeArchiveError(...)
path_types[path_key] = is_directory
+ parent_paths.update(member_parent_paths)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if not is_directory: | |
| descendant_prefix = path_key + "/" | |
| if any(key.startswith(descendant_prefix) for key in path_types): | |
| raise UnsafeArchiveError( | |
| "Archive file conflicts with an existing directory member: " | |
| f"{canonical_path!r}" | |
| ) | |
| validated: List[_ValidatedMember] = [] | |
| path_types: Dict[str, bool] = {} | |
| parent_paths = set() | |
| for source, name, is_directory, mode in candidates: | |
| parts = _canonical_member_parts(name, is_directory) | |
| canonical_path = "/".join(parts) | |
| path_key = unicodedata.normalize("NFC", canonical_path).casefold() | |
| member_parent_paths = [] | |
| for index in range(1, len(parts)): | |
| parent_key = unicodedata.normalize( | |
| "NFC", "/".join(parts[:index]) | |
| ).casefold() | |
| member_parent_paths.append(parent_key) | |
| if path_types.get(parent_key) is False: | |
| raise UnsafeArchiveError( | |
| "Archive file conflicts with an existing directory member: " | |
| f"{canonical_path!r}" | |
| ) | |
| if not is_directory: | |
| if path_key in parent_paths: | |
| raise UnsafeArchiveError( | |
| "Archive file conflicts with an existing directory member: " | |
| f"{canonical_path!r}" | |
| ) | |
| path_types[path_key] = is_directory | |
| parent_paths.update(member_parent_paths) |
🤖 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 `@dpdispatcher/utils/archive.py` around lines 141 - 147, Update the manifest
validation around the non-directory branch to track encountered directory
prefixes in a dedicated set rather than scanning all entries in path_types for
each file. Use that set to detect file-versus-directory conflicts while
preserving the existing UnsafeArchiveError and canonical_path behavior.
Comment on lines
+155
to
+163
| def _prepare_root(destination: str) -> str: | ||
| """Create and resolve the trusted extraction root.""" | ||
| os.makedirs(destination, exist_ok=True) | ||
| root = os.path.realpath(os.path.abspath(destination)) | ||
| if not os.path.isdir(root): | ||
| raise UnsafeArchiveError( | ||
| f"Archive extraction destination is not a directory: {destination!r}" | ||
| ) | ||
| return root |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject symlink and junction extraction roots.
realpath() currently converts a destination link into its target before validation. A pre-existing symlink or Windows junction can therefore redirect every validated write outside the intended destination.
Proposed fix
def _prepare_root(destination: str) -> str:
"""Create and resolve the trusted extraction root."""
- os.makedirs(destination, exist_ok=True)
- root = os.path.realpath(os.path.abspath(destination))
- if not os.path.isdir(root):
+ destination_path = os.path.abspath(destination)
+ os.makedirs(destination_path, exist_ok=True)
+ destination_stat = os.lstat(destination_path)
+ if _is_unsafe_link(destination_path, destination_stat):
+ raise UnsafeArchiveError(
+ f"Archive extraction destination is a link: {destination!r}"
+ )
+ if not stat.S_ISDIR(destination_stat.st_mode):
raise UnsafeArchiveError(
f"Archive extraction destination is not a directory: {destination!r}"
)
- return root
+ return os.path.realpath(destination_path)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _prepare_root(destination: str) -> str: | |
| """Create and resolve the trusted extraction root.""" | |
| os.makedirs(destination, exist_ok=True) | |
| root = os.path.realpath(os.path.abspath(destination)) | |
| if not os.path.isdir(root): | |
| raise UnsafeArchiveError( | |
| f"Archive extraction destination is not a directory: {destination!r}" | |
| ) | |
| return root | |
| def _prepare_root(destination: str) -> str: | |
| """Create and resolve the trusted extraction root.""" | |
| destination_path = os.path.abspath(destination) | |
| os.makedirs(destination_path, exist_ok=True) | |
| destination_stat = os.lstat(destination_path) | |
| if _is_unsafe_link(destination_path, destination_stat): | |
| raise UnsafeArchiveError( | |
| f"Archive extraction destination is a link: {destination!r}" | |
| ) | |
| if not stat.S_ISDIR(destination_stat.st_mode): | |
| raise UnsafeArchiveError( | |
| f"Archive extraction destination is not a directory: {destination!r}" | |
| ) | |
| return os.path.realpath(destination_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 `@dpdispatcher/utils/archive.py` around lines 155 - 163, Update _prepare_root
to reject a pre-existing symlink or Windows junction at destination before
calling realpath or validating the resolved directory. Raise UnsafeArchiveError
for these link-based roots, while preserving normal directory creation and
resolution for genuine directories.
Comment on lines
+102
to
+124
| def test_rejects_existing_symlink_components(self) -> None: | ||
| """Extraction never follows a symlink below the trusted output root.""" | ||
| with tempfile.TemporaryDirectory() as temp_dir: | ||
| archive_path = os.path.join(temp_dir, "result.tar") | ||
| output_dir = os.path.join(temp_dir, "output") | ||
| outside_dir = os.path.join(temp_dir, "outside") | ||
| os.mkdir(output_dir) | ||
| os.mkdir(outside_dir) | ||
| try: | ||
| os.symlink(outside_dir, os.path.join(output_dir, "task")) | ||
| except (NotImplementedError, OSError) as error: | ||
| self.skipTest(f"symlinks are unavailable: {error}") | ||
|
|
||
| with tarfile.open(archive_path, "w") as archive: | ||
| self._add_file(archive, "valid.txt", b"valid") | ||
| self._add_file(archive, "task/result.txt", b"result") | ||
|
|
||
| with tarfile.open(archive_path, "r") as archive: | ||
| with self.assertRaisesRegex(UnsafeArchiveError, "symlink"): | ||
| safe_extract_tar(archive, output_dir) | ||
|
|
||
| self.assertFalse(os.path.exists(os.path.join(outside_dir, "result.txt"))) | ||
| self.assertFalse(os.path.exists(os.path.join(output_dir, "valid.txt"))) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Test redirects at the destination root itself.
These cases only redirect output/task; they do not pass a symlink or Windows junction as output_dir. Add root-destination cases for both public extractors to enforce the PR’s symlink/junction destination guarantee.
Also applies to: 151-176, 277-299
🤖 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 `@tests/test_archive.py` around lines 102 - 124, Add root-destination tests
alongside test_rejects_existing_symlink_components and the related cases,
covering both public extraction entry points. Make output_dir itself a symlink
or Windows junction to an outside directory, invoke each extractor with an
archive, and assert UnsafeArchiveError is raised without creating files outside
the trusted destination. Preserve platform skips where symlinks or junctions are
unavailable.
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
Validation
python -m coverage run -p --source=./dpdispatcher -m unittest: 176 passed, 43 skippedruff check .: passedruff format --check .: passedty check: reports four pre-existingallow_ref/dargsdiagnostics in unchanged filesCloses #604
Coding agent: Codex
Codex version: codex-cli 0.144.4
Model: gpt-5.6-sol
Reasoning effort: xhigh
Summary by CodeRabbit
Bug Fixes
Tests