From 03e02591ec99d14810334a07c5f5825e5c576507 Mon Sep 17 00:00:00 2001
From: Brian Donovan <1938+eventualbuddha@users.noreply.github.com>
Date: Fri, 4 Aug 2023 21:28:09 -0700
Subject: [PATCH] fix(cli): allows extracting files without separate directory
 entries

Some zip files do not list directories separately from files. For example, a zip might contain `dir/file.txt` without a corresponding entry for just `dir/`. This should be okay, so we just create the leading paths for the files we extract as necessary.

Also adds tests for the expected and dangerous path cases of `extract_zip`.
---
 packages/cli/Cargo.toml                   |   3 ++
 packages/cli/src/tools.rs                 |  55 +++++++++++++++++++++-
 packages/cli/tests/fixtures/dangerous.zip | Bin 0 -> 127 bytes
 packages/cli/tests/fixtures/test.zip      | Bin 0 -> 315 bytes
 4 files changed, 56 insertions(+), 2 deletions(-)
 create mode 100644 packages/cli/tests/fixtures/dangerous.zip
 create mode 100644 packages/cli/tests/fixtures/test.zip

diff --git a/packages/cli/Cargo.toml b/packages/cli/Cargo.toml
index c5168a4db..892399a9d 100644
--- a/packages/cli/Cargo.toml
+++ b/packages/cli/Cargo.toml
@@ -97,3 +97,6 @@ plugin = ["mlua"]
 [[bin]]
 path = "src/main.rs"
 name = "dx"
+
+[dev-dependencies]
+tempfile = "3.3"
diff --git a/packages/cli/src/tools.rs b/packages/cli/src/tools.rs
index b1fcdc72d..4045b236e 100644
--- a/packages/cli/src/tools.rs
+++ b/packages/cli/src/tools.rs
@@ -329,6 +329,9 @@ pub fn extract_zip(file: &Path, target: &Path) -> anyhow::Result<()> {
 
     for i in 0..zip.len() {
         let mut zip_entry = zip.by_index(i)?;
+
+        // check for dangerous paths
+        // see https://docs.rs/zip/latest/zip/read/struct.ZipFile.html#warnings
         let Some(enclosed_name) = zip_entry.enclosed_name() else {
             return Err(anyhow::anyhow!(
                 "Refusing to unpack zip entry with potentially dangerous path: zip={} entry={:?}",
@@ -336,12 +339,17 @@ pub fn extract_zip(file: &Path, target: &Path) -> anyhow::Result<()> {
                 zip_entry.name()
             ));
         };
+
         let output_path = target.join(enclosed_name);
         if zip_entry.is_dir() {
-            // dir
             std::fs::create_dir_all(output_path)?;
         } else {
-            // file
+            // create parent dirs if needed
+            if let Some(parent) = output_path.parent() {
+                std::fs::create_dir_all(parent)?;
+            }
+
+            // extract file
             let mut target_file = if !output_path.exists() {
                 std::fs::File::create(output_path)?
             } else {
@@ -353,3 +361,46 @@ pub fn extract_zip(file: &Path, target: &Path) -> anyhow::Result<()> {
 
     Ok(())
 }
+
+#[cfg(test)]
+mod test {
+    use super::*;
+    use std::path::PathBuf;
+    use tempfile::tempdir;
+
+    #[test]
+    fn test_extract_zip() -> anyhow::Result<()> {
+        let path = PathBuf::from(std::env::var("CARGO_MANIFEST_DIR").unwrap())
+            .join("tests/fixtures/test.zip");
+        let temp_dir = tempdir()?;
+        let temp_path = temp_dir.path();
+
+        extract_zip(path.as_path(), temp_path)?;
+
+        let expected_files = vec!["file1.txt", "file2.txt", "dir/file3.txt"];
+        for file in expected_files {
+            let path = temp_path.join(file);
+            assert!(path.exists(), "File not found: {:?}", path);
+        }
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_extract_zip_dangerous_path() -> anyhow::Result<()> {
+        let path = PathBuf::from(std::env::var("CARGO_MANIFEST_DIR").unwrap())
+            .join("tests/fixtures/dangerous.zip");
+        let temp_dir = tempdir()?;
+        let temp_path = temp_dir.path();
+
+        let result = extract_zip(path.as_path(), temp_path);
+
+        let err = result.unwrap_err();
+        assert!(err
+            .to_string()
+            .contains("Refusing to unpack zip entry with potentially dangerous path: zip="));
+        assert!(err.to_string().contains("entry=\"/etc/passwd\""));
+
+        Ok(())
+    }
+}
diff --git a/packages/cli/tests/fixtures/dangerous.zip b/packages/cli/tests/fixtures/dangerous.zip
new file mode 100644
index 0000000000000000000000000000000000000000..09ff13d6357a4fc55c07488c5c5ed11c7d7bbc16
GIT binary patch
literal 127
zcmWIWW@Zs#0D-cVEa43KoA$8-*&xgf#QLcv$@&F}#l__*DT#UMsYQwb-i%Bl%(!&`
cHGx3`BZxxMAK=Z(1`=WfLQ5cR0OBwJ0N|Sx-~a#s

literal 0
HcmV?d00001

diff --git a/packages/cli/tests/fixtures/test.zip b/packages/cli/tests/fixtures/test.zip
new file mode 100644
index 0000000000000000000000000000000000000000..bb90b81addaf5ca48b42ee9f7cbdfae67fddbfcc
GIT binary patch
literal 315
zcmWIWW@Zs#0D<ULEaBVV-)Uw9vO$;=h|@B2QVsP=DoP9k;OY{A>Uj5El|ol%1X5>&
zP?ri+H@ojrE=(OS(7cq)B7KmV#vrxE0p5&EBFwmL2WkL=29Oa5w;*f7<`{@328ISk
k4X8G-(~z}ca~nci0Fa5|(g1H(Hjp$k5UvE$H6RWH07iW_^Z)<=

literal 0
HcmV?d00001