♻️ Improve Windows path stripping: add tests, consolidate API, fix types

- Add boundary/adversarial tests for strip_absolute_path_bsdtar (empty,
  separators-only, drive-only, device prefix, terminal dotdot/dot)
- Add is_unsafe_link equivalent tests and bypass vector tests
- Consolidate pub(crate) strip_absolute_path_bsdtar + has_parent_dir_component
  into single pub(crate) is_unsafe_link_path, keeping primitives module-private
- Simplify RewrittenPath: remove unused lifetime, Cow<str> → String
- Add doc comments with security invariants to key functions
- Update stale comments on sanitize_preserve_curdir_str and cfg(windows) tests

Author: ChanTsune

cli/src/command/core/path.rs

@@ -122,18 +122,22 @@ impl PathnameEditor {
         Some(stripped.into_owned())
     }
 
+    /// Rewrite the path by stripping Windows-style absolute prefixes (drive letters,
+    /// UNC/API prefixes) and leading root separators, matching bsdtar's
+    /// `strip_absolute_path()` behavior. When `absolute_paths` is true, the path
+    /// is returned unchanged.
     #[inline]
-    fn rewrite_path_for_extraction<'a>(&self, path: &'a Path) -> RewrittenPath<'a> {
+    fn rewrite_path_for_extraction(&self, path: &Path) -> RewrittenPath {
         let raw = path.to_string_lossy().into_owned();
         if self.absolute_paths {
             RewrittenPath {
-                path: Cow::Owned(raw),
+                path: raw,
                 had_root: false,
             }
         } else {
-            let (path, had_root) = strip_absolute_path_bsdtar(&raw);
+            let (stripped, had_root) = strip_absolute_path_bsdtar(&raw);
             RewrittenPath {
-                path: Cow::Owned(path.into_owned()),
+                path: stripped.into_owned(),
                 had_root,
             }
         }
@@ -155,8 +159,12 @@ impl PathnameEditor {
     }
 }
 
-struct RewrittenPath<'a> {
-    path: Cow<'a, str>,
+/// Result of [`PathnameEditor::rewrite_path_for_extraction`].
+///
+/// `had_root` indicates whether any absolute prefix (root separator, drive
+/// letter, or Windows API prefix) was consumed during rewriting.
+struct RewrittenPath {
+    path: String,
     had_root: bool,
 }
 
@@ -174,11 +182,14 @@ fn strip_components(path: &Path, count: Option<usize>) -> Option<Cow<'_, Path>>
     Some(Cow::from(PathBuf::from_iter(components.skip(count))))
 }
 
-/// CLI-side sanitization that keeps `CurDir` (`.`) and `ParentDir` (`..`) components.
+/// CLI-side sanitization that retains `CurDir` (`.`) and `ParentDir` (`..`)
+/// while stripping `RootDir` and `Prefix` components via [`std::path::Path::components`].
 ///
-/// Absolute path prefix handling is performed before this stage; this function
-/// only normalizes `.` / `..` under the host path semantics while preserving
-/// backslashes as literal bytes on Unix to match bsdtar's POSIX behavior.
+/// Absolute path prefix stripping (Windows drive letters, UNC prefixes,
+/// leading `/../` sequences) is handled upstream by [`strip_absolute_path_bsdtar`];
+/// this function handles residual host-path normalization. On Unix,
+/// backslash-containing segments are treated as literal `Normal` components by
+/// the standard library, which preserves them as-is.
 fn sanitize_preserve_curdir_str(s: &str) -> String {
     let path = Path::new(s);
     join_components_forward_slash(path.components().filter(|c| {
@@ -201,7 +212,23 @@ fn sanitize_preserve_curdir_reference(reference: EntryReference) -> EntryReferen
     EntryReference::from_utf8_preserve_root(&sanitize_preserve_curdir_str(reference.as_str()))
 }
 
-pub(crate) fn has_parent_dir_component(s: &str) -> bool {
+/// Returns `true` if the path is unsafe as a link reference.
+///
+/// A link is unsafe if it contains an absolute path component (root separator,
+/// drive letter, or Windows API prefix) or a parent directory (`..`) component
+/// under either host or Windows path semantics.
+pub(crate) fn is_unsafe_link_path(s: &str) -> bool {
+    let (rewritten, had_root) = strip_absolute_path_bsdtar(s);
+    had_root || has_parent_dir_component(rewritten.as_ref())
+}
+
+/// Returns `true` if the path contains a `..` (parent directory) component
+/// under either host path semantics or Windows path semantics.
+///
+/// The dual check ensures that Windows-style `..` preceded by backslash
+/// separators (e.g., `..\\file`) is detected even on non-Windows hosts where
+/// `std::path::Path` treats backslashes as literal characters.
+fn has_parent_dir_component(s: &str) -> bool {
     Path::new(s)
         .components()
         .any(|c| matches!(c, Component::ParentDir))
@@ -210,7 +237,21 @@ pub(crate) fn has_parent_dir_component(s: &str) -> bool {
             .any(|c| matches!(c, Utf8WindowsComponent::ParentDir))
 }
 
-pub(crate) fn strip_absolute_path_bsdtar(path: &str) -> (Cow<'_, str>, bool) {
+/// bsdtar-compatible stripping of absolute path prefixes.
+///
+/// Strips Windows API prefixes (`\\?\`, `\\.\`), UNC prefixes (`\\?\UNC\`),
+/// drive letters (`C:`), and leading separators (including consuming `/../`
+/// and `/./` sequences that follow a root).
+///
+/// Returns the remaining path after stripping and a flag indicating whether
+/// any prefix or root separator was consumed.
+///
+/// # Security invariant
+///
+/// This function may leave `..` components in the output (e.g., from `D:..`).
+/// Callers **must** check the result with [`has_parent_dir_component`] to
+/// detect path traversal, or use [`is_unsafe_link_path`] which combines both.
+fn strip_absolute_path_bsdtar(path: &str) -> (Cow<'_, str>, bool) {
     let mut rest = path;
     let mut had_root = false;
 
@@ -646,8 +687,10 @@ mod tests {
     //
     // bsdtar's strip_absolute_path() strips Windows API prefixes, drive letters,
     // and leading separators. On Windows, Rust's Path::components() correctly
-    // parses these as Prefix/RootDir components, which sanitize_preserve_curdir
-    // filters out — producing the same result as bsdtar.
+    // parses these as Prefix/RootDir components. The `rewrite_path_for_extraction`
+    // method calls `strip_absolute_path_bsdtar` to handle this at the string
+    // level before host-path normalization. The cross-platform tests below this
+    // section cover the same scenarios without #[cfg(windows)].
     //
     // These 8 types correspond to bsdtar's test_windows.c mkfullpath() types.
 
@@ -856,4 +899,214 @@ mod tests {
         assert_eq!(reference.as_str(), "etc/hosts");
         assert!(had_root);
     }
+
+    // --- B1: strip_absolute_path_bsdtar boundary values ---
+
+    #[test]
+    fn strip_absolute_path_bsdtar_empty_string() {
+        let (path, had_root) = strip_absolute_path_bsdtar("");
+        assert_eq!("", path);
+        assert!(!had_root);
+    }
+
+    #[test]
+    fn strip_absolute_path_bsdtar_single_forward_slash() {
+        let (path, had_root) = strip_absolute_path_bsdtar("/");
+        assert_eq!("", path);
+        assert!(had_root);
+    }
+
+    #[test]
+    fn strip_absolute_path_bsdtar_single_backslash() {
+        let (path, had_root) = strip_absolute_path_bsdtar("\\");
+        assert_eq!("", path);
+        assert!(had_root);
+    }
+
+    #[test]
+    fn strip_absolute_path_bsdtar_multiple_separators() {
+        let (path, had_root) = strip_absolute_path_bsdtar("///");
+        assert_eq!("", path);
+        assert!(had_root);
+
+        let (path, had_root) = strip_absolute_path_bsdtar("\\\\");
+        assert_eq!("", path);
+        assert!(had_root);
+    }
+
+    #[test]
+    fn strip_absolute_path_bsdtar_drive_letter_only() {
+        let (path, had_root) = strip_absolute_path_bsdtar("c:");
+        assert_eq!("", path);
+        assert!(had_root);
+    }
+
+    #[test]
+    fn strip_absolute_path_bsdtar_terminal_dotdot() {
+        // bsdtar: "/.."-at-end -- p[3]=='\0' is not a separator, so only "/"
+        // is stripped, leaving "..". The ".." is caught by has_parent_dir_component.
+        let (path, had_root) = strip_absolute_path_bsdtar("/..");
+        assert_eq!("..", path);
+        assert!(had_root);
+
+        let (path, had_root) = strip_absolute_path_bsdtar("\\..");
+        assert_eq!("..", path);
+        assert!(had_root);
+    }
+
+    #[test]
+    fn strip_absolute_path_bsdtar_terminal_dot() {
+        // bsdtar: "/."-at-end -- p[2]=='\0' is not a separator, so only "/"
+        // is stripped, leaving ".".
+        let (path, had_root) = strip_absolute_path_bsdtar("/.");
+        assert_eq!(".", path);
+        assert!(had_root);
+
+        let (path, had_root) = strip_absolute_path_bsdtar("\\.");
+        assert_eq!(".", path);
+        assert!(had_root);
+    }
+
+    #[test]
+    fn strip_absolute_path_bsdtar_prefix_exactly_4_bytes() {
+        let (path, had_root) = strip_absolute_path_bsdtar("//?/");
+        assert_eq!("", path);
+        assert!(had_root);
+    }
+
+    #[test]
+    fn strip_absolute_path_bsdtar_unc_prefix_exactly_8_bytes() {
+        let (path, had_root) = strip_absolute_path_bsdtar("//?/UNC/");
+        assert_eq!("", path);
+        assert!(had_root);
+    }
+
+    #[test]
+    fn strip_absolute_path_bsdtar_safe_relative_paths() {
+        let (path, had_root) = strip_absolute_path_bsdtar("file.txt");
+        assert_eq!("file.txt", path);
+        assert!(!had_root);
+
+        let (path, had_root) = strip_absolute_path_bsdtar("a/b/c");
+        assert_eq!("a/b/c", path);
+        assert!(!had_root);
+
+        let (path, had_root) = strip_absolute_path_bsdtar("./a/b");
+        assert_eq!("./a/b", path);
+        assert!(!had_root);
+    }
+
+    // --- B2: device prefix (\\.\) tests ---
+
+    #[test]
+    fn strip_absolute_path_bsdtar_device_prefix_with_drive() {
+        // \\.\C:\file -- device prefix (4 bytes stripped), then drive letter
+        let (path, had_root) = strip_absolute_path_bsdtar("\\\\.\\C:\\file");
+        assert_eq!("file", path);
+        assert!(had_root);
+
+        // //./C:/file -- forward-slash variant
+        let (path, had_root) = strip_absolute_path_bsdtar("//./C:/file");
+        assert_eq!("file", path);
+        assert!(had_root);
+    }
+
+    #[test]
+    fn strip_absolute_path_bsdtar_device_prefix_only() {
+        // \\.\ alone (4 bytes + trailing separator)
+        let (path, had_root) = strip_absolute_path_bsdtar("\\\\.\\");
+        assert_eq!("", path);
+        assert!(had_root);
+    }
+
+    #[test]
+    fn strip_absolute_path_bsdtar_device_prefix_with_traversal() {
+        // \\.\..\secret -- device prefix (4 bytes) stripped, "..\secret" remains.
+        // The ".." is NOT consumed because it doesn't follow a separator;
+        // has_parent_dir_component catches it downstream.
+        let (path, had_root) = strip_absolute_path_bsdtar("\\\\.\\..\\secret");
+        assert_eq!("..\\secret", path);
+        assert!(had_root);
+        assert!(has_parent_dir_component(path.as_ref()));
+    }
+
+    // --- B3+B4: is_unsafe_link equivalent (strip + has_parent_dir combined) ---
+    // is_unsafe_link is: had_root || has_parent_dir_component(rewritten)
+    // We test the same logic directly here.
+
+    #[test]
+    fn strip_then_parent_dir_detects_drive_dotdot_backslash() {
+        let (rewritten, had_root) = strip_absolute_path_bsdtar("c:..\\file");
+        assert!(had_root || has_parent_dir_component(rewritten.as_ref()));
+    }
+
+    #[test]
+    fn strip_then_parent_dir_detects_slash_dotdot_backslash() {
+        let (rewritten, had_root) = strip_absolute_path_bsdtar("/..\\file");
+        assert!(had_root || has_parent_dir_component(rewritten.as_ref()));
+    }
+
+    #[test]
+    fn strip_then_parent_dir_detects_drive_slash_dotdot_backslash() {
+        let (rewritten, had_root) = strip_absolute_path_bsdtar("c:/..\\file");
+        assert!(had_root || has_parent_dir_component(rewritten.as_ref()));
+    }
+
+    #[test]
+    fn strip_then_parent_dir_detects_device_prefix_dotdot() {
+        let (rewritten, had_root) = strip_absolute_path_bsdtar("\\\\?\\..\\file");
+        assert!(had_root || has_parent_dir_component(rewritten.as_ref()));
+    }
+
+    #[test]
+    fn strip_then_parent_dir_allows_safe_path() {
+        let (rewritten, had_root) = strip_absolute_path_bsdtar("a/b/c");
+        assert!(!had_root);
+        assert!(!has_parent_dir_component(rewritten.as_ref()));
+    }
+
+    /// Equivalent to is_unsafe_link logic for various link reference patterns.
+    fn is_unsafe_link_equivalent(s: &str) -> bool {
+        let (rewritten, had_root) = strip_absolute_path_bsdtar(s);
+        had_root || has_parent_dir_component(rewritten.as_ref())
+    }
+
+    #[test]
+    fn unsafe_link_equivalent_detects_all_unsafe_patterns() {
+        // Windows drive prefix
+        assert!(is_unsafe_link_equivalent("C:/file"));
+        // POSIX absolute
+        assert!(is_unsafe_link_equivalent("/etc/passwd"));
+        // Backslash parent traversal
+        assert!(is_unsafe_link_equivalent("..\\file"));
+        // Forward slash parent traversal
+        assert!(is_unsafe_link_equivalent("../file"));
+        // Windows UNC with embedded dotdot
+        assert!(is_unsafe_link_equivalent("\\\\?\\UNC\\..\\file"));
+        // Drive-relative with dotdot
+        assert!(is_unsafe_link_equivalent("D:../file"));
+    }
+
+    #[test]
+    fn unsafe_link_equivalent_allows_safe_patterns() {
+        assert!(!is_unsafe_link_equivalent("a/b/c"));
+        assert!(!is_unsafe_link_equivalent("file.txt"));
+        assert!(!is_unsafe_link_equivalent("./a/b"));
+    }
+
+    #[test]
+    fn has_parent_dir_component_adversarial_cases() {
+        // mid-path backslash dotdot
+        assert!(has_parent_dir_component("foo\\..\\bar"));
+        // standalone dotdot
+        assert!(has_parent_dir_component(".."));
+        // dotdot with mixed separators
+        assert!(has_parent_dir_component("a/b\\../c"));
+        // trailing separator after dotdot
+        assert!(has_parent_dir_component("../"));
+        // NOT parent dir: "..name" is a normal component
+        assert!(!has_parent_dir_component("a/..name"));
+        // NOT parent dir: "..." is a normal component
+        assert!(!has_parent_dir_component("a/.../b"));
+    }
 }

cli/src/command/extract.rs

@@ -1732,7 +1732,5 @@ fn search_group(name: &str, id: u64) -> io::Result<Group> {
 
 #[inline]
 fn is_unsafe_link(reference: &EntryReference) -> bool {
-    let (rewritten, had_root) =
-        crate::command::core::path::strip_absolute_path_bsdtar(reference.as_str());
-    had_root || crate::command::core::path::has_parent_dir_component(rewritten.as_ref())
+    crate::command::core::path::is_unsafe_link_path(reference.as_str())
 }