✨ Return non-zero exit code when entries are skipped for path security#2839
✨ Return non-zero exit code when entries are skipped for path security#2839ChanTsune wants to merge 1 commit into
Conversation
Add Outcome, FilterResult, and ExtractResult enums to the extract pipeline to distinguish security-related skips (path traversal, unsafe links) from normal filter skips (glob, exclude, overwrite). When any entry is skipped due to a security violation, the extract command now returns exit code 1 with a warning on stderr, matching bsdtar's behavior of setting return_value=1 on ARCHIVE_WARN. Update path_traversal integration tests to expect failure exit code when security skips occur.
📝 WalkthroughWalkthroughThis pull request introduces security-aware outcome tracking to the extraction process. The extraction functions now return an Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security posture of the archive extraction process by introducing a clear signal for security-related skips. By returning a non-zero exit code and providing more detailed categorization of skipped entries, it improves the robustness and user feedback of the extraction command, making it easier to identify and react to potential security vulnerabilities during archive processing. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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 introduces enhanced security checks for path traversal vulnerabilities during archive extraction. The extraction logic has been refactored to return an Outcome enum, which indicates whether the extraction was successful or if security-related warnings occurred. New FilterResult and ExtractResult enums are introduced to differentiate between various reasons for skipping entries, including path traversal or unsafe link targets. An atomic boolean had_security_skip tracks security-related skips, and if any occur, the bsdtar and extract commands now bail out with an error. Corresponding unit tests have been updated to reflect these new failure conditions for path traversal scenarios. I have no feedback to provide.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cli/src/command/extract.rs (2)
553-559:⚠️ Potential issue | 🟡 MinorDon't log success before handling
Outcome::Warn.A security-skip run now emits “Successfully extracted...” and then immediately returns an error, which makes the logs contradictory for the exact case this PR adds.
🐛 Proposed fix
- log::info!( - "Successfully extracted an archive in {}", - DurationDisplay(start.elapsed()) - ); if outcome == Outcome::Warn { anyhow::bail!("One or more entries were not extracted due to path security violations"); } + log::info!( + "Successfully extracted an archive in {}", + DurationDisplay(start.elapsed()) + );🤖 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 553 - 559, The success message is logged before checking for Outcome::Warn, causing contradictory logs when extraction is skipped for security reasons; modify the code around the log::info! call and the Outcome::Warn check in extract.rs so that you only emit the "Successfully extracted an archive in {}" message (using DurationDisplay(start.elapsed())) when outcome != Outcome::Warn (or move the log call to after the warn-check), and ensure that if outcome == Outcome::Warn you bail with the existing anyhow::bail! without logging success.
1414-1420:⚠️ Potential issue | 🟠 MajorVerify that
edit_hardlink()misses are always security-related.This branch unconditionally returns
ExtractResult::SecuritySkipped, butedit_hardlink()can returnNonefor benign cases. Thetransform_and_strip()function returnsNonewhen paths become empty after transformation or stripping (viais_effectively_empty_path()checks), which are non-security skips. Meanwhile, only thecheck_nodotdot()check (which guards against..components) is documented as security-related. Map empty-path cases toExtractResult::Skippedand reserveSecuritySkippedfor actual unsafe-target rejections like parent-directory traversal.🤖 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 1414 - 1420, The current branch treats every None from pathname_editor.edit_hardlink(...) as SecuritySkipped, but edit_hardlink can return None for benign empty-path cases (from transform_and_strip/is_effectively_empty_path()) as well as true security rejects (check_nodotdot()); update the logic to distinguish those: modify edit_hardlink (or add a new helper) to return a richer result (e.g., enum { Ok((PathBuf,bool)), SkippedEmpty, SecurityRejected }) or call transform_and_strip/check_nodotdot before deciding, and then map SkippedEmpty (or empty-path detection via is_effectively_empty_path()/transform_and_strip()) to ExtractResult::Skipped while mapping actual unsafe-target/nodotdot rejections to ExtractResult::SecuritySkipped; ensure references to edit_hardlink, transform_and_strip, is_effectively_empty_path, check_nodotdot, and the ExtractResult variants are used so reviewers can find the changes.
🧹 Nitpick comments (1)
cli/tests/cli/stdio/path_traversal.rs (1)
147-148: Assert the exact warning-exit contract here.
.failure()will also pass on unrelated non-zero exits or a panic. Since these cases are the regression coverage for the new security-skip behavior, please pin them to exit code1and stderr containing the path-security message as well.Also applies to: 339-340, 384-385, 565-566
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/tests/cli/stdio/path_traversal.rs` around lines 147 - 148, Replace the loose .assert().failure() checks in the path traversal tests with a strict assertion that the process exited with code 1 and stderr contains the path-security warning; specifically, change occurrences of the .assert().failure() call (e.g., the chain using .assert() in path_traversal.rs) to something like .assert().code(1).stderr(predicate::str::contains("path-security") /* or the exact warning text */) so the test pins both the exit code and the error message; apply the same replacement to the other occurrences flagged in the comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/command/extract.rs`:
- Around line 1105-1117: Don't re-infer skip reason from the raw archive path;
instead use the pathname editor's richer result. Replace the current let
Some(name) = args.pathname_editor.edit_entry_name(...) else { ... } block with a
call that captures the editor's full result (e.g., use the PathnameEditor
method/return type that includes a reason or status instead of just Option — the
existing edit_entry_name variant that yields a richer enum/struct in
cli/src/command/core/path.rs), then map its variants to
FilterResult::SecuritySkipped or FilterResult::Skipped based on the
editor-provided reason (preserve logging messages), and apply the same change
for the second occurrence around lines 1182-1193; reference symbols:
args.pathname_editor, edit_entry_name (or the richer edit result API), and
FilterResult::{SecuritySkipped, Skipped}.
---
Outside diff comments:
In `@cli/src/command/extract.rs`:
- Around line 553-559: The success message is logged before checking for
Outcome::Warn, causing contradictory logs when extraction is skipped for
security reasons; modify the code around the log::info! call and the
Outcome::Warn check in extract.rs so that you only emit the "Successfully
extracted an archive in {}" message (using DurationDisplay(start.elapsed()))
when outcome != Outcome::Warn (or move the log call to after the warn-check),
and ensure that if outcome == Outcome::Warn you bail with the existing
anyhow::bail! without logging success.
- Around line 1414-1420: The current branch treats every None from
pathname_editor.edit_hardlink(...) as SecuritySkipped, but edit_hardlink can
return None for benign empty-path cases (from
transform_and_strip/is_effectively_empty_path()) as well as true security
rejects (check_nodotdot()); update the logic to distinguish those: modify
edit_hardlink (or add a new helper) to return a richer result (e.g., enum {
Ok((PathBuf,bool)), SkippedEmpty, SecurityRejected }) or call
transform_and_strip/check_nodotdot before deciding, and then map SkippedEmpty
(or empty-path detection via is_effectively_empty_path()/transform_and_strip())
to ExtractResult::Skipped while mapping actual unsafe-target/nodotdot rejections
to ExtractResult::SecuritySkipped; ensure references to edit_hardlink,
transform_and_strip, is_effectively_empty_path, check_nodotdot, and the
ExtractResult variants are used so reviewers can find the changes.
---
Nitpick comments:
In `@cli/tests/cli/stdio/path_traversal.rs`:
- Around line 147-148: Replace the loose .assert().failure() checks in the path
traversal tests with a strict assertion that the process exited with code 1 and
stderr contains the path-security warning; specifically, change occurrences of
the .assert().failure() call (e.g., the chain using .assert() in
path_traversal.rs) to something like
.assert().code(1).stderr(predicate::str::contains("path-security") /* or the
exact warning text */) so the test pins both the exit code and the error
message; apply the same replacement to the other occurrences flagged in the
comment.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a54cf243-eda8-4058-9a5e-121a43f5c690
📒 Files selected for processing (3)
cli/src/command/bsdtar.rscli/src/command/extract.rscli/tests/cli/stdio/path_traversal.rs
| let Some(name) = args.pathname_editor.edit_entry_name(item_name.as_path()) else { | ||
| // Distinguish security-related skips (path traversal) from benign skips | ||
| // (path became empty after sanitization, e.g. "." entries). | ||
| if item_name | ||
| .as_path() | ||
| .components() | ||
| .any(|c| matches!(c, std::path::Component::ParentDir)) | ||
| { | ||
| log::warn!("Skipping entry with unsafe path: {item_name}"); | ||
| return FilterResult::SecuritySkipped; | ||
| } | ||
| log::debug!("Skip: {item_name}"); | ||
| } | ||
| name | ||
| return FilterResult::Skipped; |
There was a problem hiding this comment.
Don't infer the skip reason from the raw archive path.
Per cli/src/command/core/path.rs:43-55 and cli/src/command/core/path.rs:97-134, edit_entry_name() returns None for benign empty-after-strip / empty-after-sanitize cases as well as real .. rejections. Re-checking the pre-transform item_name here makes the new outcome unreliable: a/../b stripped away by --strip-components becomes a false SecuritySkipped, while a rename rule that introduces .. becomes a plain Skipped. This needs a richer reason from PathnameEditor instead of reconstructing it here.
Also applies to: 1182-1193
🤖 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 1105 - 1117, Don't re-infer skip
reason from the raw archive path; instead use the pathname editor's richer
result. Replace the current let Some(name) =
args.pathname_editor.edit_entry_name(...) else { ... } block with a call that
captures the editor's full result (e.g., use the PathnameEditor method/return
type that includes a reason or status instead of just Option — the existing
edit_entry_name variant that yields a richer enum/struct in
cli/src/command/core/path.rs), then map its variants to
FilterResult::SecuritySkipped or FilterResult::Skipped based on the
editor-provided reason (preserve logging messages), and apply the same change
for the second occurrence around lines 1182-1193; reference symbols:
args.pathname_editor, edit_entry_name (or the richer edit result API), and
FilterResult::{SecuritySkipped, Skipped}.
1 participant
Add Outcome, FilterResult, and ExtractResult enums to the extract pipeline to distinguish security-related skips (path traversal, unsafe links) from normal filter skips (glob, exclude, overwrite).
When any entry is skipped due to a security violation, the extract command now returns exit code 1 with a warning on stderr, matching bsdtar's behavior of setting return_value=1 on ARCHIVE_WARN.
Update path_traversal integration tests to expect failure exit code when security skips occur.
Summary by CodeRabbit
Release Notes