Adress review comments

2 files changed, 47 insertions, 6 deletions

diff --git a/src/file_upload.rs b/src/file_upload.rs
index 2319b6a..5009f36 100644
--- a/src/file_upload.rs
+++ b/src/file_upload.rs
@@ -8,7 +8,10 @@ use std::{
 use crate::errors::ContextualError;
 use crate::listing::{self};
 
-/// Create future to save file.
+/// Saves file data from a multipart form field (`field`) to `file_path`, optionally overwriting
+/// existing file.
+///
+/// Returns total bytes written to file.
 async fn save_file(
     field: actix_multipart::Field,
     file_path: PathBuf,
@@ -34,7 +37,7 @@ async fn save_file(
     Ok(written_len)
 }
 
-/// Create new future to handle file as multipart data.
+/// Handles a single field in a multipart form
 async fn handle_multipart(
     field: actix_multipart::Field,
     path: PathBuf,
@@ -121,6 +124,8 @@ pub async fn upload_file(
 
 /// Guarantee that the path is relative and cannot traverse back to parent directories
 /// and optionally prevent traversing hidden directories.
+///
+/// See the unit tests tests::test_sanitize_path* for examples
 fn sanitize_path(path: &Path, traverse_hidden: bool) -> Option<PathBuf> {
     let mut buf = PathBuf::new();
 
@@ -145,3 +150,35 @@ fn sanitize_path(path: &Path, traverse_hidden: bool) -> Option<PathBuf> {
 
     Some(buf)
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use pretty_assertions::assert_eq;
+    use rstest::rstest;
+
+    #[rstest]
+    #[case("/foo", "foo")]
+    #[case("////foo", "foo")]
+    #[case("C:/foo", if cfg!(windows) { "foo" } else { "C:/foo" })]
+    #[case("../foo", "foo")]
+    #[case("../foo/../bar/abc", "bar/abc")]
+    fn test_sanitize_path(#[case] input: &str, #[case] output: &str) {
+        assert_eq!(
+            sanitize_path(Path::new(input), true).unwrap(),
+            Path::new(output)
+        );
+        assert_eq!(
+            sanitize_path(Path::new(input), false).unwrap(),
+            Path::new(output)
+        );
+    }
+
+    #[rstest]
+    #[case(".foo")]
+    #[case("/.foo")]
+    #[case("foo/.bar/foo")]
+    fn test_sanitize_path_no_hidden_files(#[case] input: &str) {
+        assert_eq!(sanitize_path(Path::new(input), false), None);
+    }
+}
diff --git a/tests/upload_files.rs b/tests/upload_files.rs
index 26750d9..331db1c 100644
--- a/tests/upload_files.rs
+++ b/tests/upload_files.rs
@@ -80,6 +80,10 @@ fn uploading_files_is_prevented(server: TestServer) -> Result<(), Error> {
     Ok(())
 }
 
+/// Test for path traversal vulnerability (CWE-22) in both path parameter of query string and in
+/// file name (Content-Disposition)
+///
+/// see: https://github.com/svenstaro/miniserve/issues/518
 #[rstest]
 #[case("foo", "bar", "foo/bar")]
 #[case("/../foo", "bar", "foo/bar")]
@@ -87,13 +91,13 @@ fn uploading_files_is_prevented(server: TestServer) -> Result<(), Error> {
 #[case("C:/foo", "C:/bar", if cfg!(windows) { "foo/bar" } else { "C:/foo/C:/bar" })]
 #[case(r"C:\foo", r"C:\bar", if cfg!(windows) { "foo/bar" } else { r"C:\foo/C:\bar" })]
 #[case(r"\foo", r"\..\bar", if cfg!(windows) { "foo/bar" } else { r"\foo/\..\bar" })]
-fn path_traversal(
+fn prevent_path_traversal_attacks(
     #[with(&["-u"])] server: TestServer,
     #[case] path: &str,
     #[case] filename: &'static str,
     #[case] expected: &str,
 ) -> Result<(), Error> {
-    // create test directories
+    // Create test directories
     use std::fs::create_dir_all;
     create_dir_all(server.path().join("foo")).unwrap();
     if !cfg!(windows) {
@@ -132,14 +136,14 @@ fn symlink(#[case] server: TestServer, #[case] ok: bool, tmpdir: TempDir) -> Res
     #[cfg(windows)]
     use std::os::windows::fs::symlink_dir;
 
-    // create symlink directory "foo" to point outside the root
+    // Create symlink directory "foo" to point outside the root
     let (dir, filename) = ("foo", "bar");
     symlink_dir(tmpdir.path(), server.path().join(dir)).unwrap();
 
     let full_path = server.path().join(dir).join(filename);
     assert!(!full_path.exists());
 
-    // try to upload
+    // Try to upload
     let part = multipart::Part::text("this should be uploaded")
         .file_name(filename)
         .mime_str("text/plain")?;