fix: make sure to deal with malformed UTF-8 in file names (#374)

Author: palant

src/directory_listing.rs

@@ -13,8 +13,9 @@ use headers::{ContentLength, ContentType, HeaderMapExt};
 use humansize::FormatSize;
 use hyper::{Body, Method, Response, StatusCode};
 use mime_guess::mime;
-use percent_encoding::{percent_decode_str, utf8_percent_encode, AsciiSet, NON_ALPHANUMERIC};
+use percent_encoding::{percent_decode_str, percent_encode, AsciiSet, NON_ALPHANUMERIC};
 use serde::{Serialize, Serializer};
+use std::ffi::{OsStr, OsString};
 use std::future::Future;
 use std::io;
 use std::path::Path;
@@ -152,16 +153,15 @@ enum FileType {
 /// Defines a file entry and its properties.
 #[derive(Serialize)]
 struct FileEntry {
-    name: String,
-    #[serde(skip_serializing)]
-    name_encoded: String,
+    #[serde(serialize_with = "serialize_name")]
+    name: OsString,
     #[serde(serialize_with = "serialize_mtime")]
     mtime: Option<DateTime<Local>>,
     #[serde(skip_serializing_if = "Option::is_none")]
     size: Option<u64>,
     r#type: FileType,
     #[serde(skip_serializing)]
-    uri: Option<String>,
+    uri: String,
 }
 
 impl FileEntry {
@@ -205,36 +205,19 @@ fn read_dir_entries(
             }
         };
 
-        // FIXME: handle non-Unicode file names properly via OsString
-        let name = match dir_entry
-            .file_name()
-            .into_string()
-            .map_err(|err| anyhow::anyhow!(err.into_string().unwrap_or_default()))
-        {
-            Ok(s) => s,
-            Err(err) => {
-                tracing::error!(
-                    "unable to resolve name for file or directory entry (skipped): {:?}",
-                    err
-                );
-                continue;
-            }
-        };
+        let name = dir_entry.file_name();
 
         // Check and ignore the current hidden file/directory (dotfile) if feature enabled
-        if ignore_hidden_files && name.starts_with('.') {
+        if ignore_hidden_files && name.as_encoded_bytes().first().is_some_and(|c| *c == b'.') {
             continue;
         }
 
-        let mut name_encoded = utf8_percent_encode(&name, PERCENT_ENCODE_SET).to_string();
-        let mut size = None;
-
-        if meta.is_dir() {
-            name_encoded.push('/');
+        let (r#type, size) = if meta.is_dir() {
             dirs_count += 1;
+            (FileType::Directory, None)
         } else if meta.is_file() {
-            size = Some(meta.len());
             files_count += 1;
+            (FileType::File, Some(meta.len()))
         } else if meta.file_type().is_symlink() {
             // NOTE: we resolve the symlink path below to just know if is a directory or not.
             // Hwever, we are still showing the symlink name but not the resolved name.
@@ -246,7 +229,7 @@ fn read_dir_entries(
                     tracing::error!(
                         "unable to resolve `{}` symlink path (skipped): {:?}",
                         symlink.display(),
-                        err
+                        err,
                     );
                     continue;
                 }
@@ -258,59 +241,43 @@ fn read_dir_entries(
                     tracing::error!(
                         "unable to resolve metadata for `{}` symlink (skipped): {:?}",
                         symlink.display(),
-                        err
+                        err,
                     );
                     continue;
                 }
             };
             if symlink_meta.is_dir() {
-                name_encoded.push('/');
                 dirs_count += 1;
+                (FileType::Directory, None)
             } else {
-                size = Some(meta.len());
                 files_count += 1;
+                (FileType::File, Some(symlink_meta.len()))
             }
         } else {
             continue;
-        }
+        };
+
+        let name_encoded = percent_encode(name.as_encoded_bytes(), PERCENT_ENCODE_SET).to_string();
 
-        let mut uri = None;
         // NOTE: Use relative paths by default independently of
         // the "redirect trailing slash" feature.
         // However, when "redirect trailing slash" is disabled
         // and a request path doesn't contain a trailing slash then
         // entries should contain the "parent/entry-name" as a link format.
         // Otherwise, we just use the "entry-name" as a link (default behavior).
         // Note that in both cases, we add a trailing slash if the entry is a directory.
-        if !base_path.ends_with('/') {
-            let base_path = Path::new(base_path);
-            let parent_dir = base_path.parent().unwrap_or(base_path);
-            let mut base_dir = base_path;
-            if base_path != parent_dir {
-                base_dir = match base_path.strip_prefix(parent_dir) {
-                    Ok(v) => v,
-                    Err(err) => {
-                        tracing::error!(
-                            "unable to strip parent path prefix for `{}` (skipped): {:?}",
-                            base_path.display(),
-                            err
-                        );
-                        continue;
-                    }
-                };
-            }
-
-            let mut base_str = String::new();
-            if !base_dir.starts_with("/") {
-                let base_dir = base_dir.to_str().unwrap_or_default();
-                if !base_dir.is_empty() {
-                    base_str.push_str(base_dir);
-                }
-                base_str.push('/');
-            }
+        let mut uri = if !base_path.ends_with('/') && !base_path.is_empty() {
+            let parent = base_path
+                .rsplit_once('/')
+                .map(|(_, parent)| parent)
+                .unwrap_or(base_path);
+            format!("{parent}/{name_encoded}")
+        } else {
+            name_encoded
+        };
 
-            base_str.push_str(&name_encoded);
-            uri = Some(base_str);
+        if r#type == FileType::Directory {
+            uri.push('/');
         }
 
         let mtime = match parse_last_modified(meta.modified()?) {
@@ -320,15 +287,9 @@ fn read_dir_entries(
                 None
             }
         };
-        let r#type = if meta.is_dir() {
-            FileType::Directory
-        } else {
-            FileType::File
-        };
 
         file_entries.push(FileEntry {
             name,
-            name_encoded,
             mtime,
             size,
             r#type,
@@ -379,7 +340,7 @@ fn read_dir_entries(
                 files_count,
                 &mut file_entries,
                 order_code,
-            )?
+            )
         }
     };
 
@@ -403,6 +364,11 @@ fn json_auto_index(entries: &mut [FileEntry], order_code: u8) -> Result<String>
     Ok(serde_json::to_string(entries)?)
 }
 
+/// Serialize FileEntry::name
+fn serialize_name<S: Serializer>(name: &OsStr, serializer: S) -> Result<S::Ok, S::Error> {
+    serializer.serialize_str(&name.to_string_lossy())
+}
+
 /// Serialize FileEntry::mtime field
 fn serialize_mtime<S: Serializer>(
     mtime: &Option<DateTime<Local>>,
@@ -425,13 +391,13 @@ fn html_auto_index<'a>(
     files_count: usize,
     entries: &'a mut [FileEntry],
     order_code: u8,
-) -> Result<String> {
+) -> String {
     use maud::{html, DOCTYPE};
 
     let sort_attrs = sort_file_entries(entries, order_code);
-    let current_path = percent_decode_str(base_path).decode_utf8()?.to_string();
+    let current_path = percent_decode_str(base_path).decode_utf8_lossy();
 
-    Ok(html! {
+    html! {
         (DOCTYPE)
         html {
             head {
@@ -490,8 +456,8 @@ fn html_auto_index<'a>(
                     @for entry in entries {
                         tr {
                             td {
-                                a href=(entry.uri.as_ref().unwrap_or(&entry.name_encoded)) {
-                                    (entry.name)
+                                a href=(entry.uri) {
+                                    (entry.name.to_string_lossy())
                                     @if entry.is_dir() {
                                         "/"
                                     }
@@ -519,7 +485,7 @@ fn html_auto_index<'a>(
                 }
             }
         }
-    }.into())
+    }.into()
 }
 
 /// Sort a list of file entries by a specific order code.
@@ -532,7 +498,7 @@ fn sort_file_entries(files: &mut [FileEntry], order_code: u8) -> SortingAttr<'_>
     match order_code {
         0 | 1 => {
             // Name (asc, desc)
-            files.sort_by_cached_key(|f| f.name.to_lowercase());
+            files.sort_by_cached_key(|f| f.name.to_string_lossy().to_lowercase());
             if order_code == 1 {
                 files.reverse();
             } else {

src/file_path.rs

@@ -7,10 +7,7 @@
 
 use hyper::StatusCode;
 use percent_encoding::percent_decode_str;
-use std::{
-    borrow::Cow,
-    path::{Component, Path, PathBuf},
-};
+use std::path::{Component, Path, PathBuf};
 
 /// SWS Path extensions trait.
 pub(crate) trait PathExt {
@@ -30,20 +27,29 @@ impl PathExt for Path {
     }
 }
 
-fn decode_tail_path(tail: &str) -> Result<Cow<'_, str>, StatusCode> {
-    match percent_decode_str(tail.trim_start_matches('/')).decode_utf8() {
-        Ok(p) => Ok(p),
-        Err(err) => {
-            tracing::debug!("dir: failed to decode route={:?}: {:?}", tail, err);
-            Err(StatusCode::UNSUPPORTED_MEDIA_TYPE)
-        }
-    }
+#[cfg(unix)]
+fn path_from_bytes(bytes: &[u8]) -> PathBuf {
+    use std::ffi::OsStr;
+    use std::os::unix::ffi::OsStrExt;
+
+    OsStr::from_bytes(bytes).into()
+}
+
+#[cfg(windows)]
+fn path_from_bytes(bytes: &[u8]) -> PathBuf {
+    // This should really be OsStr::from_encoded_bytes_unchecked() but it’s
+    // unsafe. With this fallback non-Unicode file names will result in 404.
+    String::from_utf8_lossy(bytes).into_owned().into()
+}
+
+fn decode_tail_path(tail: &str) -> PathBuf {
+    let bytes = percent_decode_str(tail.trim_start_matches('/')).collect::<Vec<_>>();
+    path_from_bytes(&bytes)
 }
 
 /// Sanitizes a base/tail paths and then it returns an unified one.
 pub(crate) fn sanitize_path(base: &Path, tail: &str) -> Result<PathBuf, StatusCode> {
-    let path_decoded = decode_tail_path(tail)?;
-    let path_decoded = Path::new(&*path_decoded);
+    let path_decoded = decode_tail_path(tail);
     let mut full_path = base.to_path_buf();
     tracing::trace!("dir: base={:?}, route={:?}", full_path, path_decoded);