refactor(backup): replace system zip/unzip with pure Rust implementation #773

NitronPlus · 2025-11-13T03:17:47Z

Replace system-dependent zip/unzip commands with the zip crate for cross-platform compatibility and better error handling.

Fix two bugs when restore backup files, see #771

I also write five unit test in backup.rs, it will import tempfile crates in dev-dependencies. And use walkdir crate to rewrite copy_dir_recursive function .If you prefer the previous version , I will change it and file a new PR

Adding zip and walkdir crates will increase the final binary size by approximately 1MB on Windows

Replace system-dependent zip/unzip commands with the `zip` crate for cross-platform compatibility and better error handling.

Copilot

Pull Request Overview

This PR refactors the backup/restore system by replacing platform-dependent system commands (zip/unzip) with a pure Rust implementation using the zip crate. This improves cross-platform compatibility, error handling, and maintainability while fixing two bugs in the restore functionality.

Key Changes:

Replaced system zip/unzip commands with the zip crate for compression and extraction operations
Added helper functions for file filtering and introduced a TempDirGuard for reliable cleanup
Implemented comprehensive unit tests covering roundtrip compression, system compatibility, and edge cases

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 10 comments.

File	Description
src-tauri/src/backup.rs	Complete rewrite of backup/restore logic using pure Rust zip operations with improved error handling, file filtering utilities, automatic cleanup via RAII guard, and extensive test coverage
src-tauri/Cargo.toml	Added `zip`, `walkdir` dependencies and `tempfile` dev-dependency to support the new Rust-based implementation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-13T03:21:49Z

src-tauri/src/backup.rs

+            Some(path) => target_dir.join(path),
+            None => continue,
+        };
+


[nitpick] The enclosed_name() method already provides protection against path traversal attacks by checking if the path is safe. However, there's no explicit validation that outpath stays within target_dir. While enclosed_name() should handle this, it would be more defensive to add an explicit check:

let outpath = match file.enclosed_name() { Some(path) => target_dir.join(path), None => continue, }; // Verify the output path is still within target_dir if !outpath.starts_with(target_dir) { return Err("Archive contains files outside target directory".to_string()); }

This provides defense in depth against potential security issues.

Suggested change

// Defense in depth: ensure outpath is within target_dir

if !outpath.starts_with(target_dir) {

return Err("Archive contains files outside target directory".to_string());

}

import_app_data function will explicit specify target dir , it will make sure outpath stays within target_dir

Copilot · 2025-11-13T03:21:49Z

src-tauri/Cargo.toml

 tauri-plugin-opener = "2.4.0"
 uuid = { version = "1.10", features = ["v4"] }
 dirs = "5.0"
+zip = "6"


The version specification for the zip crate is too loose. Using "6" will match any 6.x version, which could introduce breaking changes. Consider pinning to a more specific version:

zip = "6.2"

Or for more stability:

zip = "=6.2.0"

This ensures reproducible builds and prevents unexpected breaking changes.

Suggested change

zip = "6"

zip = "6.2"

src-tauri/src/backup.rs

Copilot · 2025-11-13T03:21:50Z

src-tauri/src/backup.rs

+fn should_skip_file(path: &Path) -> bool {
+    if let Some(file_name) = path.file_name() {
+        if let Some(file_str) = file_name.to_str() {
+            // Skip temp_import directory and SQLite temporary files
+            return file_str == TEMP_IMPORT_DIR
+                || path.to_string_lossy().ends_with(SQLITE_SHM_SUFFIX)
+                || path.to_string_lossy().ends_with(SQLITE_WAL_SUFFIX);
+        }
+    }
+    false
+}


The check for TEMP_IMPORT_DIR only checks the file name, but should check if the path starts with or contains the temp_import directory. This could fail to skip nested files within temp_import.

Consider using:

return file_str == TEMP_IMPORT_DIR || path.to_string_lossy().starts_with(TEMP_IMPORT_DIR) || path.to_string_lossy().ends_with(SQLITE_SHM_SUFFIX) || path.to_string_lossy().ends_with(SQLITE_WAL_SUFFIX);

Or better yet, check if any component of the path matches TEMP_IMPORT_DIR.

should explicit match the dir name

src-tauri/src/backup.rs

Copilot

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-13T06:55:50Z

src-tauri/src/backup.rs

+            let rel_path_display = rel_path.to_string_lossy();
+            fs::create_dir_all(&dest_path)
+                .map_err(|e| format!("Failed to create directory {}: {}", rel_path_display, e))?;


[nitpick] Same issue as above: rel_path_display is only used in error messages. Move its declaration into the error closure to avoid allocation in the success path.

Will be modified in the next commit

Copilot · 2025-11-13T06:55:50Z

src-tauri/src/backup.rs

 use tauri_plugin_store::StoreExt;
-use serde_json::Value;
+use walkdir::WalkDir;
+use zip::write::SimpleFileOptions;


The SimpleFileOptions type does not exist in the zip crate (version 2.x uses FileOptions). This code will not compile with the correct version of the zip crate. Please update to use FileOptions::default() instead of SimpleFileOptions::default().

zip crate version is 6.0 , use SImpleFileOption is ok

Copilot · 2025-11-13T06:55:50Z

src-tauri/src/backup.rs

+        let outpath = match file.enclosed_name() {
+            Some(path) => target_dir.join(path),
+            None => continue,
+        };


Potential path traversal vulnerability: While enclosed_name() provides some protection, it's recommended to additionally verify that the outpath stays within target_dir after joining. Consider adding: if !outpath.starts_with(target_dir) { return Err("Path traversal detected".to_string()); } after line 96.

Suggested change

};

};

// Additional path traversal protection

if !outpath.starts_with(target_dir) {

return Err("Path traversal detected".to_string());

}

app will make sure the outpath stays within target_dir

Copilot · 2025-11-13T06:55:51Z

src-tauri/src/backup.rs

+    let file =
+        File::create(&output_path).map_err(|e| format!("Failed to create zip file: {}", e))?;


[nitpick] Race condition potential: If the zip file already exists at output_path, File::create will truncate it. However, if the subsequent compression fails, the original backup file will be lost. Consider writing to a temporary file first and then renaming it to the final destination only after successful compression.

app is running on singletion, would not be happen

src-tauri/src/backup.rs

Copilot · 2025-11-13T06:55:51Z

src-tauri/src/backup.rs

+            let mut outfile =
+                File::create(&outpath).map_err(|e| format!("Failed to create file: {}", e))?;
+            std::io::copy(&mut file, &mut outfile)
+                .map_err(|e| format!("Failed to extract file: {}", e))?;
+        }


[nitpick] Missing file permissions preservation: The extraction process doesn't preserve Unix file permissions or attributes from the original files. For cross-platform compatibility, consider using set_permissions() with the permissions from the zip entry if available.

Will be modified in the next commit

Copilot · 2025-11-13T06:55:51Z

src-tauri/src/backup.rs

+        .unwrap_or(false)
+}
+
+/// Compress a directory to a zip file


The function documentation says "Compress a directory to a zip file" but the function signature shows it takes a mutable ZipWriter reference, not creating the zip file itself. Consider updating the documentation to: "Add directory contents to an existing zip archive".

Suggested change

/// Compress a directory to a zip file

/// Add directory contents to an existing zip archive

Will be modified in the next commit

refactor(backup): replace system zip/unzip with pure Rust implementation

6d7ce44

Replace system-dependent zip/unzip commands with the `zip` crate for cross-platform compatibility and better error handling.

Copilot AI review requested due to automatic review settings November 13, 2025 03:17

Copilot started reviewing on behalf of NitronPlus November 13, 2025 03:18 View session

Copilot finished reviewing on behalf of NitronPlus November 13, 2025 03:20

Copilot AI reviewed Nov 13, 2025

View reviewed changes

unit test for the zip/unzip function

a9f5685

NitronPlus requested a review from Copilot November 13, 2025 06:52

Copilot started reviewing on behalf of NitronPlus November 13, 2025 06:52 View session

Copilot finished reviewing on behalf of NitronPlus November 13, 2025 06:54

Copilot AI reviewed Nov 13, 2025

View reviewed changes

handle file permission for unix

9a3ede0

+        // Defense in depth: ensure outpath is within target_dir
+        if !outpath.starts_with(target_dir) {
+            return Err("Archive contains files outside target directory".to_string());
+        }

-        };
+        };
+        // Additional path traversal protection
+        if !outpath.starts_with(target_dir) {
+            return Err("Path traversal detected".to_string());
+        }

		let file =
		File::create(&output_path).map_err(\|e\| format!("Failed to create zip file: {}", e))?;

	/// Compress a directory to a zip file
	/// Add directory contents to an existing zip archive

refactor(backup): replace system zip/unzip with pure Rust implementation #773

Are you sure you want to change the base?

refactor(backup): replace system zip/unzip with pure Rust implementation #773

Uh oh!

Conversation

NitronPlus commented Nov 13, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

NitronPlus Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

NitronPlus Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

NitronPlus Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

NitronPlus Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

NitronPlus Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

NitronPlus Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

NitronPlus Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

NitronPlus Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

NitronPlus Nov 13, 2025 •

edited

Loading