fix: Support for non-UTF8 paths on HFS+ on MacOS

Now illegal UTF8 is percent-encoded. Previously this code would have panicked.

Author: Byron

Cargo.toml

@@ -1,5 +1,3 @@
-
-
 [package]
 name = "trash"
 version = "5.2.0"
@@ -29,6 +27,7 @@ rand = "0.8.5"
 once_cell = "1.18.0"
 env_logger = "0.10.0"
 tempfile = "3.8.0"
+defer = "0.2.1"
 
 
 [target.'cfg(target_os = "macos")'.dependencies]
@@ -39,6 +38,7 @@ objc2-foundation = { version = "0.2.0", features = [
     "NSString",
     "NSURL",
 ] }
+percent-encoding = "2.3.1"
 
 [target.'cfg(all(unix, not(target_os = "macos"), not(target_os = "ios"), not(target_os = "android")))'.dependencies]
 chrono = { version = "0.4.31", optional = true, default-features = false, features = [

src/macos.rs

@@ -1,4 +1,8 @@
-use std::{ffi::OsString, path::PathBuf, process::Command};
+use std::{
+    ffi::OsString,
+    path::{Path, PathBuf},
+    process::Command,
+};
 
 use log::trace;
 use objc2_foundation::{NSFileManager, NSString, NSURL};
@@ -71,22 +75,25 @@ impl TrashContextExtMacos for TrashContext {
 }
 impl TrashContext {
     pub(crate) fn delete_all_canonicalized(&self, full_paths: Vec<PathBuf>) -> Result<(), Error> {
-        let full_paths = full_paths.into_iter().map(to_string).collect::<Result<Vec<_>, _>>()?;
         match self.platform_specific.delete_method {
-            DeleteMethod::Finder => delete_using_finder(full_paths),
-            DeleteMethod::NsFileManager => delete_using_file_mgr(full_paths),
+            DeleteMethod::Finder => delete_using_finder(&full_paths),
+            DeleteMethod::NsFileManager => delete_using_file_mgr(&full_paths),
         }
     }
 }
 
-fn delete_using_file_mgr(full_paths: Vec<String>) -> Result<(), Error> {
+fn delete_using_file_mgr<P: AsRef<Path>>(full_paths: &[P]) -> Result<(), Error> {
     trace!("Starting delete_using_file_mgr");
     let file_mgr = unsafe { NSFileManager::defaultManager() };
     for path in full_paths {
-        let string = NSString::from_str(&path);
+        let path = path.as_ref().as_os_str().as_encoded_bytes();
+        let path = match std::str::from_utf8(path) {
+            Ok(path_utf8) => NSString::from_str(path_utf8), // utf-8 path, use as is
+            Err(_) => NSString::from_str(&percent_encode(path)), // binary path, %-encode it
+        };
 
         trace!("Starting fileURLWithPath");
-        let url = unsafe { NSURL::fileURLWithPath(&string) };
+        let url = unsafe { NSURL::fileURLWithPath(&path) };
         trace!("Finished fileURLWithPath");
 
         trace!("Calling trashItemAtURL");
@@ -95,19 +102,29 @@ fn delete_using_file_mgr(full_paths: Vec<String>) -> Result<(), Error> {
 
         if let Err(err) = res {
             return Err(Error::Unknown {
-                description: format!("While deleting '{path}', `trashItemAtURL` failed: {err}"),
+                description: format!("While deleting '{:?}', `trashItemAtURL` failed: {err}", path.as_ref()),
             });
         }
     }
     Ok(())
 }
 
-fn delete_using_finder(full_paths: Vec<String>) -> Result<(), Error> {
+fn delete_using_finder<P: AsRef<Path>>(full_paths: &[P]) -> Result<(), Error> {
     // AppleScript command to move files (or directories) to Trash looks like
     //   osascript -e 'tell application "Finder" to delete { POSIX file "file1", POSIX "file2" }'
     // The `-e` flag is used to execute only one line of AppleScript.
     let mut command = Command::new("osascript");
-    let posix_files = full_paths.into_iter().map(|p| format!("POSIX file \"{p}\"")).collect::<Vec<String>>().join(", ");
+    let posix_files = full_paths
+        .iter()
+        .map(|p| {
+            let path_b = p.as_ref().as_os_str().as_encoded_bytes();
+            match std::str::from_utf8(path_b) {
+                Ok(path_utf8) => format!("POSIX file \"{path_utf8}\""), // utf-8 path, use as is
+                Err(_) => format!("POSIX file \"{}\"", &percent_encode(path_b)), // binary path, %-encode it
+            }
+        })
+        .collect::<Vec<String>>()
+        .join(", ");
     let script = format!("tell application \"Finder\" to delete {{ {posix_files} }}");
 
     let argv: Vec<OsString> = vec!["-e".into(), script.into()];
@@ -135,24 +152,47 @@ fn delete_using_finder(full_paths: Vec<String>) -> Result<(), Error> {
     Ok(())
 }
 
-fn to_string<T: Into<OsString>>(str_in: T) -> Result<String, Error> {
-    let os_string = str_in.into();
-    let s = os_string.to_str();
-    match s {
-        Some(s) => Ok(s.to_owned()),
-        None => Err(Error::ConvertOsString { original: os_string }),
+/// std's from_utf8_lossy, but non-utf8 byte sequences are %-encoded instead of being replaced by a special symbol.
+/// Valid utf8, including `%`, are not escaped.
+use std::borrow::Cow;
+fn percent_encode(input: &[u8]) -> Cow<'_, str> {
+    use percent_encoding::percent_encode_byte as b2pc;
+
+    let mut iter = input.utf8_chunks().peekable();
+    if let Some(chunk) = iter.peek() {
+        if chunk.invalid().is_empty() {
+            return Cow::Borrowed(chunk.valid());
+        }
+    } else {
+        return Cow::Borrowed("");
+    };
+
+    let mut res = String::with_capacity(input.len());
+    for chunk in iter {
+        res.push_str(chunk.valid());
+        let invalid = chunk.invalid();
+        if !invalid.is_empty() {
+            for byte in invalid {
+                res.push_str(b2pc(*byte));
+            }
+        }
     }
+    Cow::Owned(res)
 }
 
 #[cfg(test)]
 mod tests {
     use crate::{
-        macos::{DeleteMethod, TrashContextExtMacos},
+        macos::{percent_encode, DeleteMethod, TrashContextExtMacos},
         tests::{get_unique_name, init_logging},
         TrashContext,
     };
     use serial_test::serial;
+    use std::ffi::OsStr;
     use std::fs::File;
+    use std::os::unix::ffi::OsStrExt;
+    use std::path::PathBuf;
+    use std::process::Command;
 
     #[test]
     #[serial]
@@ -166,4 +206,69 @@ mod tests {
         trash_ctx.delete(&path).unwrap();
         assert!(File::open(&path).is_err());
     }
+
+    #[test]
+    #[serial]
+    fn test_delete_binary_path_with_ns_file_manager() {
+        let (_cleanup, tmp) = create_hfs_volume().unwrap();
+        let parent_fs_supports_binary = tmp.path();
+
+        init_logging();
+        let mut trash_ctx = TrashContext::default();
+        trash_ctx.set_delete_method(DeleteMethod::NsFileManager);
+
+        let invalid_utf8 = b"\x80"; // lone continuation byte (128) (invalid utf8)
+        let mut path_invalid = parent_fs_supports_binary.join(get_unique_name());
+        path_invalid.set_extension(OsStr::from_bytes(invalid_utf8)); //...trash-test-111-0.\x80 (not push to avoid fail unexisting dir)
+
+        File::create_new(&path_invalid).unwrap();
+
+        assert!(path_invalid.exists());
+        trash_ctx.delete(&path_invalid).unwrap();
+        assert!(!path_invalid.exists());
+    }
+
+    #[test]
+    fn test_path_byte() {
+        let invalid_utf8 = b"\x80"; // lone continuation byte (128) (invalid utf8)
+        let percent_encoded = "%80"; // valid macOS path in a %-escaped encoding
+
+        let mut expected_path = PathBuf::from(get_unique_name());
+        let mut path_with_invalid_utf8 = expected_path.clone();
+
+        path_with_invalid_utf8.push(OsStr::from_bytes(invalid_utf8)); //      trash-test-111-0/\x80
+        expected_path.push(percent_encoded); //                    trash-test-111-0/%80
+
+        let actual = percent_encode(&path_with_invalid_utf8.as_os_str().as_encoded_bytes()); // trash-test-111-0/%80
+        assert_eq!(std::path::Path::new(actual.as_ref()), expected_path);
+    }
+
+    fn create_hfs_volume() -> std::io::Result<(impl Drop, tempfile::TempDir)> {
+        let tmp = tempfile::tempdir()?;
+        let dmg_file = tmp.path().join("fs.dmg");
+        let cleanup = {
+            // Create dmg file
+            Command::new("hdiutil").args(["create", "-size", "1m", "-fs", "HFS+"]).arg(&dmg_file).status()?;
+
+            // Mount dmg file into temporary location
+            Command::new("hdiutil")
+                .args(["attach", "-nobrowse", "-mountpoint"])
+                .arg(tmp.path())
+                .arg(&dmg_file)
+                .status()?;
+
+            // Ensure that the mount point is always cleaned up
+            defer::defer({
+                let mount_point = tmp.path().to_owned();
+                move || {
+                    Command::new("hdiutil")
+                        .arg("detach")
+                        .arg(&mount_point)
+                        .status()
+                        .expect("detach temporary test dmg filesystem successfully");
+                }
+            })
+        };
+        Ok((cleanup, tmp))
+    }
 }

tests/trash.rs

@@ -1,4 +1,3 @@
-use std::ffi::OsStr;
 use std::fs::{create_dir, File};
 use std::path::{Path, PathBuf};
 
@@ -143,6 +142,7 @@ fn create_remove_single_file() {
 #[test]
 #[serial]
 fn create_remove_single_file_invalid_utf8() {
+    use std::ffi::OsStr;
     let name = unsafe { OsStr::from_encoded_bytes_unchecked(&[168]) };
     File::create_new(name).unwrap();
     trash::delete(name).unwrap();