Cli/extract/fix symlink extract#2947
Conversation
Split the monolithic extract_entry function into four helper functions organized by DataKind: - prepare_extraction: shared preamble (path build + overwrite check) - extract_file_entry: regular file extraction (safe writes, timestamps) - extract_directory_entry: directory structure + metadata restoration - extract_link_entry: symbolic and hard link extraction All dispatch sites in run_extract_archive_reader (streaming non-WASM, WASM fast_read/normal) and run_extract_archive (memmap) now call the type-specific handler directly. WASM closures gained a new Directory branch that previously fell through to extract_entry. Pure refactoring — no behavior change. The deferred directory metadata fix is introduced in a follow-up commit.
Replace debug_assert with unreachable! for DataKind validation in extract_link_entry and add equivalent checks to extract_file_entry and extract_directory_entry to restore compile-time safety lost during the extract_entry decomposition.
📝 WalkthroughWalkthroughThe PR refactors extraction logic in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the archive extraction logic by decomposing the extract_entry function into specialized handlers for files, directories, and links, supported by a new prepare_extraction helper. The review feedback recommends replacing unreachable! macros with explicit io::Error returns in the file and directory extraction functions to enhance error handling and prevent potential panics if new data types are introduced.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/src/command/extract.rs (1)
1364-1420:⚠️ Potential issue | 🔴 CriticalValidate the link target before mutating the destination.
prepare_extraction()can remove an existingpathor create parent directories before the link payload is even read. If the symlink is rejected at Lines 1381-1386, or the hardlink is skipped at Lines 1395-1402 / 1406-1410, extraction returns successfully after already changing the filesystem. In overwrite flows that can silently delete the previous entry atpathwithout replacing it.Please resolve and validate the link target first, then run the mutating prepare/remove step.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/command/extract.rs` around lines 1364 - 1420, prepare_extraction currently mutates the filesystem before the link target is validated, which can delete the existing entry at path even if the symlink/hardlink is later rejected; to fix, defer or avoid the mutating steps until after validating the link target: first open the payload with item.reader(ReadOptions::with_password(password)) and run pathname_editor.edit_symlink(...) or pathname_editor.edit_hardlink(...) and is_unsafe_link checks (and handle the None case for edit_hardlink and warned_lead_slash logic) to decide whether to proceed, and only after those validations call the mutating prepare/remove logic (the code that uses remove_existing, utils::fs::remove_path(...) and safe_writes) and finally create the link with utils::fs::symlink(...) or fs::hard_link(...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cli/src/command/extract.rs`:
- Around line 1364-1420: prepare_extraction currently mutates the filesystem
before the link target is validated, which can delete the existing entry at path
even if the symlink/hardlink is later rejected; to fix, defer or avoid the
mutating steps until after validating the link target: first open the payload
with item.reader(ReadOptions::with_password(password)) and run
pathname_editor.edit_symlink(...) or pathname_editor.edit_hardlink(...) and
is_unsafe_link checks (and handle the None case for edit_hardlink and
warned_lead_slash logic) to decide whether to proceed, and only after those
validations call the mutating prepare/remove logic (the code that uses
remove_existing, utils::fs::remove_path(...) and safe_writes) and finally create
the link with utils::fs::symlink(...) or fs::hard_link(...).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a45dc65-2a5c-486b-aeb7-7f3634616a30
📒 Files selected for processing (1)
cli/src/command/extract.rs
1 participant
Summary by CodeRabbit