From d2d77054d201012ba418c0008839dcd87c31f6ae Mon Sep 17 00:00:00 2001 From: valoq Date: Thu, 23 Jul 2026 20:14:30 +0200 Subject: [PATCH 1/2] fix: do not overwrite during merge --- src/archive/sevenz.rs | 43 ++++++++++++++++++++++------- src/archive/tar.rs | 39 +++++++++++++++++++-------- src/archive/zip.rs | 23 +++++++++++++--- src/commands/decompress.rs | 7 +++-- src/utils/fs.rs | 15 +++++++++++ tests/integration.rs | 55 ++++++++++++++++++++++++++++++++++---- 6 files changed, 150 insertions(+), 32 deletions(-) diff --git a/src/archive/sevenz.rs b/src/archive/sevenz.rs index 5e1b33b40..796af204f 100644 --- a/src/archive/sevenz.rs +++ b/src/archive/sevenz.rs @@ -12,22 +12,30 @@ use same_file::Handle; use sevenz_rust2::ArchiveEntry; use crate::{ - Result, + QuestionPolicy, Result, error::{Error, FinalError}, info, list::{FileInArchive, ListFileType}, utils::{ BytesFmt, FileVisibilityPolicy, PathFmt, cd_into_same_dir_as, copy_limited_decompression, - ensure_parent_dir_exists, is_same_file_as_output, validate_dest_inside_root, validate_entry_path, + ensure_parent_dir_exists, is_same_file_as_output, resolve_extraction_conflict, validate_dest_inside_root, + validate_entry_path, }, warning, }; -pub fn unpack_archive(reader: R, output_path: &Path, password: Option<&[u8]>) -> Result +pub fn unpack_archive( + reader: R, + output_path: &Path, + password: Option<&[u8]>, + question_policy: QuestionPolicy, +) -> Result where R: Read + Seek, { let mut files_unpacked = 0; + // The closure cannot return an ouch error so it is carried out here. + let mut conflict_error = None; let entry_extract_fn = |entry: &ArchiveEntry, reader: &mut dyn Read, path: &PathBuf| -> Result { @@ -54,11 +62,20 @@ where fs::create_dir_all(path)?; } } else { - info!("extracted ({}) {}", BytesFmt(entry.size()), PathFmt(&file_path)); + let dest = match resolve_extraction_conflict(path, question_policy) { + Ok(Some(dest)) => dest, + Ok(None) => return Ok(true), + Err(err) => { + conflict_error = Some(err); + return Ok(false); + } + }; + + info!("extracted ({}) {}", BytesFmt(entry.size()), PathFmt(&dest)); - ensure_parent_dir_exists(path)?; + ensure_parent_dir_exists(&dest)?; - let file = fs::File::create(path)?; + let file = fs::File::create(&dest)?; let mut writer = BufWriter::new(file); copy_limited_decompression(reader, &mut writer)?; @@ -70,7 +87,7 @@ where Some(ft::FileTime::from_system_time(entry.last_modified_date().into())), Some(ft::FileTime::from_system_time(entry.creation_date().into())), ) { - warning!("could not set timestamps on {}: {e}", PathFmt(&file_path)); + warning!("could not set timestamps on {}: {e}", PathFmt(&dest)); } } @@ -78,7 +95,7 @@ where Ok(true) // Always proceed }; - match password { + let result = match password { Some(password) => sevenz_rust2::decompress_with_extract_fn_and_password( reader, output_path, @@ -86,9 +103,15 @@ where reason: err.to_string(), })?), entry_extract_fn, - )?, - None => sevenz_rust2::decompress_with_extract_fn(reader, output_path, entry_extract_fn)?, + ), + None => sevenz_rust2::decompress_with_extract_fn(reader, output_path, entry_extract_fn), + }; + + // Report the prompt failure instead of the library error it caused. + if let Some(err) = conflict_error { + return Err(err); } + result?; Ok(files_unpacked) } diff --git a/src/archive/tar.rs b/src/archive/tar.rs index 1fda12309..71d85c305 100644 --- a/src/archive/tar.rs +++ b/src/archive/tar.rs @@ -13,21 +13,21 @@ use fs_err as fs; use same_file::Handle; use crate::{ - Result, + QuestionPolicy, Result, error::FinalError, info, list::{FileInArchive, ListFileType}, utils::{ self, BytesFmt, FileType, FileVisibilityPolicy, PathFmt, canonicalize, create_symlink, is_same_file_as_output, - read_file_type, sanitize_archive_mode, set_permission_mode, validate_dest_inside_root, validate_entry_path, - validate_symlink_target, + read_file_type, resolve_extraction_conflict, sanitize_archive_mode, set_permission_mode, + validate_dest_inside_root, validate_entry_path, validate_symlink_target, }, warning, }; /// Unpacks the archive given by `archive` into the folder given by `into`. /// Assumes that output_folder is empty -pub fn unpack_archive(reader: impl Read, output_folder: &Path) -> Result { +pub fn unpack_archive(reader: impl Read, output_folder: &Path, question_policy: QuestionPolicy) -> Result { let mut archive = tar::Archive::new(reader); let mut files_unpacked = 0; @@ -36,6 +36,9 @@ pub fn unpack_archive(reader: impl Read, output_folder: &Path) -> Result { for entry in archive.entries()? { let mut entry = entry?; + // Set when the user renamed a file so the log can show the real path. + let mut written = None; + match entry.header().entry_type() { tar::EntryType::Symlink => { let raw_path = entry.path()?.into_owned(); @@ -66,7 +69,20 @@ pub fn unpack_archive(reader: impl Read, output_folder: &Path) -> Result { fs::hard_link(&full_target_path, &full_link_path)?; } tar::EntryType::Regular | tar::EntryType::GNUSparse => { - entry.unpack_in(output_folder)?; + let raw_path = entry.path()?.into_owned(); + let safe_relpath = validate_entry_path(&raw_path)?; + let full_path = output_folder.join(&safe_relpath); + + let Some(dest) = resolve_extraction_conflict(&full_path, question_policy)? else { + continue; + }; + + if dest == full_path { + entry.unpack_in(output_folder)?; + } else { + entry.unpack(&dest)?; + } + written = Some(dest); } tar::EntryType::Directory => { let original_mode = entry.header().mode()?; @@ -90,14 +106,15 @@ pub fn unpack_archive(reader: impl Read, output_folder: &Path) -> Result { _ => continue, } + let unpacked_path = match written { + Some(path) => path, + None => output_folder.join(entry.path()?), + }; + if entry.header().entry_type().is_dir() { - info!("Directory {} created", PathFmt(&output_folder.join(entry.path()?))); + info!("Directory {} created", PathFmt(&unpacked_path)); } else { - info!( - "extracted ({}) {}", - BytesFmt(entry.size()), - PathFmt(&output_folder.join(entry.path()?)), - ); + info!("extracted ({}) {}", BytesFmt(entry.size()), PathFmt(&unpacked_path)); } files_unpacked += 1; } diff --git a/src/archive/zip.rs b/src/archive/zip.rs index c1d3cdf5f..82c46d57d 100644 --- a/src/archive/zip.rs +++ b/src/archive/zip.rs @@ -17,22 +17,27 @@ use zip::{self, DateTime, ZipArchive, read::ZipFile}; #[cfg(unix)] use crate::utils::sanitize_archive_mode; use crate::{ - Result, + QuestionPolicy, Result, error::FinalError, info, info_accessible, list::{FileInArchive, ListFileType}, utils::{ BytesFmt, FileType, FileVisibilityPolicy, PathFmt, canonicalize, cd_into_same_dir_as, copy_limited_decompression, create_symlink, ensure_parent_dir_exists, get_invalid_utf8_paths, - is_same_file_as_output, pretty_format_list_of_paths, read_file_type, strip_cur_dir, validate_dest_inside_root, - validate_symlink_target, + is_same_file_as_output, pretty_format_list_of_paths, read_file_type, resolve_extraction_conflict, + strip_cur_dir, validate_dest_inside_root, validate_symlink_target, }, warning, }; /// Unpacks the archive given by `archive` into the folder given by `output_folder`. /// Assumes that output_folder is empty -pub fn unpack_archive(reader: R, output_folder: &Path, password: Option<&[u8]>) -> Result +pub fn unpack_archive( + reader: R, + output_folder: &Path, + password: Option<&[u8]>, + question_policy: QuestionPolicy, +) -> Result where R: Read + Seek, { @@ -87,6 +92,16 @@ where let mode = file.unix_mode(); let is_symlink = mode.is_some_and(|mode| mode & 0o170000 == 0o120000); + // Symlink creation fails on its own when the path is taken. + let mut resolved = None; + if !is_symlink { + let Some(path) = resolve_extraction_conflict(file_path, question_policy)? else { + continue; + }; + resolved = Some(path); + } + let file_path = resolved.as_deref().unwrap_or(file_path); + if is_symlink { // Symlink targets are arbitrary bytes on Unix, not guaranteed UTF-8; read as bytes. let mut target_bytes = Vec::new(); diff --git a/src/commands/decompress.rs b/src/commands/decompress.rs index 7f467c146..8e1e87e32 100644 --- a/src/commands/decompress.rs +++ b/src/commands/decompress.rs @@ -206,7 +206,7 @@ pub fn decompress_file(options: DecompressOptions) -> Result<()> { Tar => unpack_archive( |output_dir| { let reader = LimitedReader::new(create_decoder_up_to_first_extension()?); - crate::archive::tar::unpack_archive(reader, output_dir) + crate::archive::tar::unpack_archive(reader, output_dir, options.question_policy) }, dir, )?, @@ -251,7 +251,10 @@ pub fn decompress_file(options: DecompressOptions) -> Result<()> { )) }; - unpack_archive(|output_dir| unpack_fn(reader, output_dir, options.password), dir)? + unpack_archive( + |output_dir| unpack_fn(reader, output_dir, options.password, options.question_policy), + dir, + )? } #[cfg(feature = "unrar")] Rar => { diff --git a/src/utils/fs.rs b/src/utils/fs.rs index 4a931172d..86a4cc7eb 100644 --- a/src/utils/fs.rs +++ b/src/utils/fs.rs @@ -50,6 +50,21 @@ pub fn resolve_path_conflict( } } +/// Decide where to extract a file when the path is taken. None means skip it. +pub fn resolve_extraction_conflict(path: &Path, question_policy: QuestionPolicy) -> Result> { + // Only an existing file clashes. Directories merge and other kinds fail on write. + if !path.is_file() { + return Ok(Some(path.to_path_buf())); + } + + // These choices fit a single file. They are rename or overwrite or skip. + match user_wants_to_overwrite(path, question_policy, QuestionAction::Compression)? { + FileConflitOperation::Cancel => Ok(None), + FileConflitOperation::Rename => Ok(Some(find_available_filename_by_renaming(path)?)), + FileConflitOperation::Overwrite | FileConflitOperation::Merge => Ok(Some(path.to_path_buf())), + } +} + pub fn remove_file_or_dir(path: &Path) -> Result<()> { if path.is_dir() { if let Ok(cwd) = env::current_dir() diff --git a/tests/integration.rs b/tests/integration.rs index 2a6ff20fe..c6beeb768 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -835,7 +835,7 @@ fn unpack_multiple_sources_into_the_same_destination_with_merge( .arg(archive1) .arg("-d") .arg(&out_path) - .write_stdin("m") + .write_stdin("m\no\n") .assert() .success(); @@ -1606,12 +1606,13 @@ fn decompress_dir_flag_current_dir_and_overwrite() { .assert() .success(); - // First decompress with `--dir .` — should succeed + // First decompress with `--dir .` asks before replacing the file crate::utils::cargo_bin() .arg("decompress") .arg(&archive) .args(["--dir", "."]) .current_dir(dir) + .write_stdin("o") .assert() .success(); @@ -1630,14 +1631,14 @@ fn decompress_dir_flag_current_dir_and_overwrite() { .assert() .success(); - // Decompressing again with `--dir .` — this succeeds for now, and it's - // inconsistent with other `--dir PATH` usages, this test ensure this isn't - // changed by accident. + // Decompressing again with `--dir .` asks before it replaces the file. + // Answering overwrite keeps the file replaced. crate::utils::cargo_bin() .arg("decompress") .arg(&archive) .args(["--dir", "."]) .current_dir(dir) + .write_stdin("o") .assert() .success(); @@ -1974,3 +1975,47 @@ fn decompress_conflict_with_dev_null_stdin_exits_nonzero() { "decompress with an unresolvable conflict on /dev/null stdin must exit non-zero" ); } + +// Merging into a folder must ask per file instead of replacing files silently. +#[test] +fn merging_into_a_folder_asks_before_replacing_each_file() { + for ext in MainDirectoryExtension::iter() { + let (_tempdir, dir) = testdir().unwrap(); + let archive = dir.join(format!("archive.{ext}")); + let source = dir.join("src"); + fs::create_dir(&source).unwrap(); + fs::write(source.join("data.txt"), "from archive").unwrap(); + + crate::utils::cargo_bin() + .args(["compress", source.join("data.txt").to_str().unwrap()]) + .arg(&archive) + .assert() + .success(); + + let out = dir.join("out"); + fs::create_dir(&out).unwrap(); + fs::write(out.join("data.txt"), "original").unwrap(); + + // Merge the folder and then skip the file that is already there. + crate::utils::cargo_bin() + .arg("decompress") + .arg(&archive) + .arg("-d") + .arg(&out) + .write_stdin("m\ns\n") + .assert() + .success(); + assert_eq!("original", fs::read_to_string(out.join("data.txt")).unwrap()); + + // Answering overwrite replaces it. + crate::utils::cargo_bin() + .arg("decompress") + .arg(&archive) + .arg("-d") + .arg(&out) + .write_stdin("m\no\n") + .assert() + .success(); + assert_eq!("from archive", fs::read_to_string(out.join("data.txt")).unwrap()); + } +} From 20b27db2cdaace0d60c8726b33b98fdc92ceb15e Mon Sep 17 00:00:00 2001 From: valoq Date: Wed, 29 Jul 2026 00:35:16 +0200 Subject: [PATCH 2/2] add fix for rar --- src/archive/rar.rs | 59 +++++++++++++++++++++++++++++++------- src/commands/decompress.rs | 14 +++++++-- tests/integration.rs | 35 ++++++++++++++++++++++ 3 files changed, 95 insertions(+), 13 deletions(-) diff --git a/src/archive/rar.rs b/src/archive/rar.rs index d8e2c1c3a..8206b4504 100644 --- a/src/archive/rar.rs +++ b/src/archive/rar.rs @@ -2,22 +2,64 @@ use std::path::{Path, PathBuf}; +use fs_err as fs; use unrar::{ Archive, ExtractEvent, error::{Code, UnrarError, When}, }; use crate::{ + QuestionPolicy, error::{Error, FinalError, Result}, info, list::{FileInArchive, ListFileType}, - utils::{BytesFmt, PathFmt, validate_entry_path}, + utils::{BytesFmt, PathFmt, resolve_extraction_conflict, validate_entry_path}, warning, }; -/// Unpacks the archive given by `archive_path` into the folder given by `output_folder`. -/// Assumes that output_folder is empty -pub fn unpack_archive(archive_path: &Path, output_folder: &Path, password: Option<&[u8]>) -> Result { +/// Unpacks the archive into `output_folder` and asks before replacing files. +pub fn unpack_archive( + archive_path: &Path, + output_folder: &Path, + password: Option<&[u8]>, + question_policy: QuestionPolicy, +) -> Result { + // Rar reference records need a full extraction pass to resolve. + fs::create_dir_all(output_folder)?; + let staging = tempfile::Builder::new() + .prefix(".ouch-rar-") + .tempdir_in(output_folder)?; + extract_all(archive_path, staging.path(), password)?; + move_into_place(staging.path(), staging.path(), output_folder, question_policy) +} + +/// Move each staged entry into `output_folder` at the same relative path. +fn move_into_place(root: &Path, dir: &Path, output_folder: &Path, question_policy: QuestionPolicy) -> Result { + let mut files_unpacked = 0; + for entry in fs::read_dir(dir)? { + let source = entry?.path(); + let dest = output_folder.join(source.strip_prefix(root).expect("child of staging root")); + + if fs::symlink_metadata(&source)?.is_dir() { + std::fs::create_dir_all(&dest).map_err(|err| Error::Custom { + reason: FinalError::with_title(format!("failed to create {}", PathFmt(&dest))).detail(err.to_string()), + })?; + files_unpacked += move_into_place(root, &source, output_folder, question_policy)?; + } else if let Some(target) = resolve_extraction_conflict(&dest, question_policy)? { + let size = fs::symlink_metadata(&source)?.len(); + std::fs::rename(&source, &target).map_err(|err| Error::Custom { + reason: FinalError::with_title(format!("failed to extract {}", PathFmt(&target))) + .detail(err.to_string()), + })?; + info!("extracted ({}) {}", BytesFmt(size), PathFmt(&target)); + files_unpacked += 1; + } + } + Ok(files_unpacked) +} + +/// Extract the whole archive into a staging folder in one pass. +fn extract_all(archive_path: &Path, output_folder: &Path, password: Option<&[u8]>) -> Result<()> { let archive = match password { Some(password) => Archive::with_password(archive_path, password), None => Archive::new(archive_path), @@ -25,7 +67,6 @@ pub fn unpack_archive(archive_path: &Path, output_folder: &Path, password: Optio let archive = archive.open_for_processing()?; - let mut files_unpacked: u64 = 0; let mut first_err: Option<(PathBuf, i32)> = None; let mut unsafe_path: Option<(PathBuf, String)> = None; @@ -39,11 +80,7 @@ pub fn unpack_archive(archive_path: &Path, output_folder: &Path, password: Optio true } } - ExtractEvent::Ok { filename, size } => { - info!("extracted ({}) {}", BytesFmt(size), PathFmt(&filename)); - files_unpacked += 1; - true - } + ExtractEvent::Ok { .. } => true, ExtractEvent::Err { filename, error_code } => { first_err = Some((filename, error_code)); // Returning false cancels the rest of the extraction so any @@ -80,7 +117,7 @@ pub fn unpack_archive(archive_path: &Path, output_folder: &Path, password: Optio }); } let _status = cb_result?; - Ok(files_unpacked) + Ok(()) } /// List contents of `archive_path`, returning a vector of archive entries diff --git a/src/commands/decompress.rs b/src/commands/decompress.rs index 8e1e87e32..d7ed63f99 100644 --- a/src/commands/decompress.rs +++ b/src/commands/decompress.rs @@ -263,11 +263,21 @@ pub fn decompress_file(options: DecompressOptions) -> Result<()> { let mut temp_file = tempfile::Builder::new().prefix(".ouch-rar-").tempfile_in(&dir)?; copy_limited_decompression(create_decoder_up_to_first_extension()?, &mut temp_file)?; Box::new(move |output_dir| { - crate::archive::rar::unpack_archive(temp_file.path(), output_dir, options.password) + crate::archive::rar::unpack_archive( + temp_file.path(), + output_dir, + options.password, + options.question_policy, + ) }) } else { Box::new(|output_dir| { - crate::archive::rar::unpack_archive(options.input_file_path, output_dir, options.password) + crate::archive::rar::unpack_archive( + options.input_file_path, + output_dir, + options.password, + options.question_policy, + ) }) }; diff --git a/tests/integration.rs b/tests/integration.rs index c6beeb768..9797e925a 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -2019,3 +2019,38 @@ fn merging_into_a_folder_asks_before_replacing_each_file() { assert_eq!("from archive", fs::read_to_string(out.join("data.txt")).unwrap()); } } + +// Merging a rar into a folder must ask before it replaces a file. +#[cfg(feature = "unrar")] +#[test] +fn merging_a_rar_asks_before_replacing_each_file() { + let (_tempdir, dir) = testdir().unwrap(); + let mut archive = PathBuf::from(std::env::var("CARGO_MANIFEST_DIR").unwrap()); + archive.push("tests/data/testfile.rar5.rar"); + + let out = dir.join("out"); + fs::create_dir(&out).unwrap(); + fs::write(out.join("testfile.txt"), "original").unwrap(); + + // Skip the file that is already there. + crate::utils::cargo_bin() + .arg("decompress") + .arg(&archive) + .arg("-d") + .arg(&out) + .write_stdin("s\n") + .assert() + .success(); + assert_eq!("original", fs::read_to_string(out.join("testfile.txt")).unwrap()); + + // Answering overwrite replaces it. + crate::utils::cargo_bin() + .arg("decompress") + .arg(&archive) + .arg("-d") + .arg(&out) + .write_stdin("o\n") + .assert() + .success(); + assert_eq!("Testing 123\n", fs::read_to_string(out.join("testfile.txt")).unwrap()); +}