From 04e011cfc40adc95d5ebf994e628e766a5ca714a Mon Sep 17 00:00:00 2001 From: valoq Date: Sun, 20 Sep 2026 12:47:46 +0200 Subject: [PATCH 1/5] fix: prevent writing through a symlink in the output directory --- src/archive/rar.rs | 4 +++- src/archive/sevenz.rs | 8 ++++---- src/archive/tar.rs | 2 ++ src/commands/decompress.rs | 9 ++++++--- src/utils/fs.rs | 12 ++++++++++++ 5 files changed, 27 insertions(+), 8 deletions(-) diff --git a/src/archive/rar.rs b/src/archive/rar.rs index 7c04e69c4..af9c39aff 100644 --- a/src/archive/rar.rs +++ b/src/archive/rar.rs @@ -13,7 +13,7 @@ use crate::{ error::{Error, FinalError, Result}, info, list::{FileInArchive, ListFileType}, - utils::{BytesFmt, PathFmt, resolve_extraction_conflict, validate_entry_path}, + utils::{BytesFmt, PathFmt, resolve_extraction_conflict, validate_dest_inside_root, validate_entry_path}, warning, }; @@ -40,6 +40,8 @@ fn move_into_place(root: &Path, dir: &Path, output_folder: &Path, question_polic let source = entry?.path(); let dest = output_folder.join(source.strip_prefix(root).expect("child of staging root")); + validate_dest_inside_root(output_folder, &dest)?; + 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()), diff --git a/src/archive/sevenz.rs b/src/archive/sevenz.rs index 518440abc..c4eadc9a9 100644 --- a/src/archive/sevenz.rs +++ b/src/archive/sevenz.rs @@ -38,7 +38,7 @@ where let mut conflict_error = None; let entry_extract_fn = - |entry: &ArchiveEntry, reader: &mut dyn Read, path: &PathBuf| -> Result { + |entry: &ArchiveEntry, reader: &mut dyn Read, _path: &PathBuf| -> Result { // Manually handle writing all files from 7z archive (the library defaults ignore empty files) let name_as_path = Path::new(entry.name()); @@ -58,11 +58,11 @@ where if entry.is_directory() { info!("File {} extracted to {}", entry.name(), PathFmt(&file_path)); - if !path.fs_err_try_exists()? { - fs::create_dir_all(path)?; + if !file_path.fs_err_try_exists()? { + fs::create_dir_all(&file_path)?; } } else { - let dest = match resolve_extraction_conflict(path, question_policy) { + let dest = match resolve_extraction_conflict(&file_path, question_policy) { Ok(Some(dest)) => dest, Ok(None) => return Ok(true), Err(err) => { diff --git a/src/archive/tar.rs b/src/archive/tar.rs index b9268c2f5..8518dacbb 100644 --- a/src/archive/tar.rs +++ b/src/archive/tar.rs @@ -73,6 +73,8 @@ pub fn unpack_archive(reader: impl Read, output_folder: &Path, question_policy: let safe_relpath = validate_entry_path(&raw_path)?; let full_path = output_folder.join(&safe_relpath); + validate_dest_inside_root(output_folder, &full_path)?; + let Some(dest) = resolve_extraction_conflict(&full_path, question_policy)? else { continue; }; diff --git a/src/commands/decompress.rs b/src/commands/decompress.rs index d7ed63f99..8a05c3733 100644 --- a/src/commands/decompress.rs +++ b/src/commands/decompress.rs @@ -69,7 +69,8 @@ pub fn prepare_decompress_target( let name = output_file_path.file_name().unwrap_or_default().to_os_string(); let file_target = output_dir.join(&name); let claimed = claimed_targets.contains(&file_target); - let resolved = if !claimed && !file_target.fs_err_try_exists()? { + // symlink_metadata makes a symlink in the way a conflict, not a path to write through + let resolved = if !claimed && fs::symlink_metadata(&file_target).is_err() { file_target } else if let Some(p) = resolve_path_conflict(&file_target, question_policy, QuestionAction::Decompression)? { p @@ -100,8 +101,10 @@ pub fn prepare_decompress_target( let is_cwd = target == *INITIAL_CURRENT_DIR; // merging several inputs into the CWD is intentional so the claimed check skips it let claimed_by_earlier_input = !is_cwd && claimed_targets.contains(&target); - let is_valid = !claimed_by_earlier_input - && (is_cwd || !target.fs_err_try_exists()? || (target.is_dir() && target.read_dir()?.next().is_none())); + let existing = fs::symlink_metadata(&target).ok(); + // a symlink is never a valid target, writing into it would leave the output directory + let is_empty_dir = existing.as_ref().is_some_and(|md| md.is_dir()) && target.read_dir()?.next().is_none(); + let is_valid = !claimed_by_earlier_input && (is_cwd || existing.is_none() || is_empty_dir); let resolved = if is_valid { target diff --git a/src/utils/fs.rs b/src/utils/fs.rs index 7e9f78165..899a31bdc 100644 --- a/src/utils/fs.rs +++ b/src/utils/fs.rs @@ -35,6 +35,18 @@ pub fn resolve_path_conflict( question_policy: QuestionPolicy, question_action: QuestionAction, ) -> Result> { + // a symlink here would send every write behind it out of the output directory + if fs::symlink_metadata(path).is_ok_and(|md| md.file_type().is_symlink()) { + return match user_wants_to_overwrite(path, question_policy, question_action)? { + FileConflictOperation::Cancel => Ok(None), + FileConflictOperation::Rename => Ok(Some(find_available_filename_by_renaming(path)?)), + FileConflictOperation::Overwrite | FileConflictOperation::Merge => { + fs::remove_file(path)?; + Ok(Some(path.to_path_buf())) + } + }; + } + if path.fs_err_try_exists()? { match user_wants_to_overwrite(path, question_policy, question_action)? { FileConflictOperation::Cancel => Ok(None), From 5eacb0fa1ddbaa5d8f3c98eb16c1df550661fd5b Mon Sep 17 00:00:00 2001 From: valoq Date: Sun, 20 Sep 2026 14:40:09 +0200 Subject: [PATCH 2/5] fix additional gaps --- src/archive/tar.rs | 14 ++++++++++++-- src/archive/zip.rs | 13 ++++--------- src/utils/fs.rs | 18 ++++++++++++------ 3 files changed, 28 insertions(+), 17 deletions(-) diff --git a/src/archive/tar.rs b/src/archive/tar.rs index 8518dacbb..3c3e17abe 100644 --- a/src/archive/tar.rs +++ b/src/archive/tar.rs @@ -50,7 +50,12 @@ pub fn unpack_archive(reader: impl Read, output_folder: &Path, question_policy: validate_symlink_target(&safe_relpath, &target)?; validate_dest_inside_root(output_folder, &full_path)?; - create_symlink(&target, &full_path)?; + + let Some(dest) = resolve_extraction_conflict(&full_path, question_policy)? else { + continue; + }; + create_symlink(&target, &dest)?; + written = Some(dest); } tar::EntryType::Link => { let raw_link = entry.path()?.into_owned(); @@ -66,7 +71,12 @@ pub fn unpack_archive(reader: impl Read, output_folder: &Path, question_policy: validate_dest_inside_root(output_folder, &full_link_path)?; validate_dest_inside_root(output_folder, &full_target_path)?; - fs::hard_link(&full_target_path, &full_link_path)?; + + let Some(dest) = resolve_extraction_conflict(&full_link_path, question_policy)? else { + continue; + }; + fs::hard_link(&full_target_path, &dest)?; + written = Some(dest); } tar::EntryType::Regular | tar::EntryType::GNUSparse => { let raw_path = entry.path()?.into_owned(); diff --git a/src/archive/zip.rs b/src/archive/zip.rs index 1a27665dd..e482d3357 100644 --- a/src/archive/zip.rs +++ b/src/archive/zip.rs @@ -92,15 +92,10 @@ 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); + let Some(resolved) = resolve_extraction_conflict(file_path, question_policy)? else { + continue; + }; + let file_path = resolved.as_path(); if is_symlink { // Symlink targets are arbitrary bytes on Unix, not guaranteed UTF-8; read as bytes. diff --git a/src/utils/fs.rs b/src/utils/fs.rs index 899a31bdc..29cd755e2 100644 --- a/src/utils/fs.rs +++ b/src/utils/fs.rs @@ -64,16 +64,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())); + // a directory is merged into, anything else in the way is a conflict + match fs::symlink_metadata(path) { + Ok(metadata) if metadata.is_dir() => return Ok(Some(path.to_path_buf())), + Err(_) => return Ok(Some(path.to_path_buf())), + Ok(_) => {} } - // These choices fit a single file. They are rename or overwrite or skip. match user_wants_to_overwrite(path, question_policy, QuestionAction::Compression)? { FileConflictOperation::Cancel => Ok(None), FileConflictOperation::Rename => Ok(Some(find_available_filename_by_renaming(path)?)), - FileConflictOperation::Overwrite | FileConflictOperation::Merge => Ok(Some(path.to_path_buf())), + FileConflictOperation::Overwrite | FileConflictOperation::Merge => { + // unlink instead of writing through it, the old entry may be a link or a pipe + fs::remove_file(path)?; + Ok(Some(path.to_path_buf())) + } } } @@ -194,8 +199,9 @@ pub fn validate_dest_inside_root(root: &Path, dest: &Path) -> Result<()> { let rel = dest.strip_prefix(root).map_err(|_| { FinalError::with_title("refusing to write outside extraction root").detail(format!("dest: {}", PathFmt(dest))) })?; + let parent_count = rel.components().count().saturating_sub(1); let mut probe = root.to_path_buf(); - for comp in rel.components() { + for comp in rel.components().take(parent_count) { probe.push(comp); match fs::symlink_metadata(&probe) { Ok(md) if md.file_type().is_symlink() => { From 5f3c9af8e29829114bb6a993b7ebbb02ff855058 Mon Sep 17 00:00:00 2001 From: valoq Date: Sun, 20 Sep 2026 15:51:49 +0200 Subject: [PATCH 3/5] fix additional minor issues --- src/archive/rar.rs | 2 +- src/utils/fs.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/archive/rar.rs b/src/archive/rar.rs index af9c39aff..2c4a269af 100644 --- a/src/archive/rar.rs +++ b/src/archive/rar.rs @@ -46,7 +46,7 @@ fn move_into_place(root: &Path, dir: &Path, output_folder: &Path, question_polic 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)?; + files_unpacked += 1 + 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 { diff --git a/src/utils/fs.rs b/src/utils/fs.rs index 29cd755e2..e9b1d4a0b 100644 --- a/src/utils/fs.rs +++ b/src/utils/fs.rs @@ -71,7 +71,7 @@ pub fn resolve_extraction_conflict(path: &Path, question_policy: QuestionPolicy) Ok(_) => {} } - match user_wants_to_overwrite(path, question_policy, QuestionAction::Compression)? { + match user_wants_to_overwrite(path, question_policy, QuestionAction::Decompression)? { FileConflictOperation::Cancel => Ok(None), FileConflictOperation::Rename => Ok(Some(find_available_filename_by_renaming(path)?)), FileConflictOperation::Overwrite | FileConflictOperation::Merge => { From fba03da2830f2b56da7084d78c4b3139dd04f6a5 Mon Sep 17 00:00:00 2001 From: valoq Date: Sun, 11 Oct 2026 01:51:50 +0200 Subject: [PATCH 4/5] address additional gaps --- src/archive/rar.rs | 10 +++++---- src/archive/sevenz.rs | 27 ++++++++++++++++------- src/archive/tar.rs | 15 ++++++++----- src/archive/zip.rs | 24 ++++++++++----------- src/commands/decompress.rs | 11 ++++++++++ src/utils/fs.rs | 44 +++++++++++++++++++++++++++++++++----- 6 files changed, 97 insertions(+), 34 deletions(-) diff --git a/src/archive/rar.rs b/src/archive/rar.rs index 2c4a269af..cbaf043d5 100644 --- a/src/archive/rar.rs +++ b/src/archive/rar.rs @@ -13,7 +13,10 @@ use crate::{ error::{Error, FinalError, Result}, info, list::{FileInArchive, ListFileType}, - utils::{BytesFmt, PathFmt, resolve_extraction_conflict, validate_dest_inside_root, validate_entry_path}, + utils::{ + BytesFmt, PathFmt, canonical_dest_inside_root, create_dir_inside_root, resolve_extraction_conflict, + validate_dest_inside_root, validate_entry_path, + }, warning, }; @@ -43,11 +46,10 @@ fn move_into_place(root: &Path, dir: &Path, output_folder: &Path, question_polic validate_dest_inside_root(output_folder, &dest)?; 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()), - })?; + create_dir_inside_root(output_folder, &dest)?; files_unpacked += 1 + move_into_place(root, &source, output_folder, question_policy)?; } else if let Some(target) = resolve_extraction_conflict(&dest, question_policy)? { + let target = canonical_dest_inside_root(output_folder, &target)?; 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))) diff --git a/src/archive/sevenz.rs b/src/archive/sevenz.rs index c4eadc9a9..c7e13fbc5 100644 --- a/src/archive/sevenz.rs +++ b/src/archive/sevenz.rs @@ -7,7 +7,6 @@ use std::{ use bstr::ByteSlice; use fs_err as fs; -use fs_err::PathExt; use same_file::Handle; use sevenz_rust2::ArchiveEntry; @@ -17,9 +16,9 @@ use crate::{ 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, resolve_extraction_conflict, validate_dest_inside_root, - validate_entry_path, + BytesFmt, FileVisibilityPolicy, PathFmt, canonical_dest_inside_root, cd_into_same_dir_as, + copy_limited_decompression, create_dir_inside_root, is_same_file_as_output, resolve_extraction_conflict, + validate_dest_inside_root, validate_entry_path, }, warning, }; @@ -58,8 +57,9 @@ where if entry.is_directory() { info!("File {} extracted to {}", entry.name(), PathFmt(&file_path)); - if !file_path.fs_err_try_exists()? { - fs::create_dir_all(&file_path)?; + if let Err(err) = create_dir_inside_root(output_path, &file_path) { + conflict_error = Some(err); + return Ok(false); } } else { let dest = match resolve_extraction_conflict(&file_path, question_policy) { @@ -73,9 +73,20 @@ where info!("extracted ({}) {}", BytesFmt(entry.size()), PathFmt(&dest)); - ensure_parent_dir_exists(&dest)?; + if let Err(err) = create_dir_inside_root(output_path, dest.parent().unwrap_or(output_path)) { + conflict_error = Some(err); + return Ok(false); + } + + let dest = match canonical_dest_inside_root(output_path, &dest) { + Ok(dest) => dest, + Err(err) => { + conflict_error = Some(err); + return Ok(false); + } + }; - let file = fs::File::create(&dest)?; + let file = fs::OpenOptions::new().write(true).create_new(true).open(&dest)?; let mut writer = BufWriter::new(file); copy_limited_decompression(reader, &mut writer)?; diff --git a/src/archive/tar.rs b/src/archive/tar.rs index 3c3e17abe..ce1775a38 100644 --- a/src/archive/tar.rs +++ b/src/archive/tar.rs @@ -18,9 +18,9 @@ use crate::{ info, list::{FileInArchive, ListFileType}, utils::{ - self, BytesFmt, FileType, FileVisibilityPolicy, PathFmt, canonicalize, create_symlink, is_same_file_as_output, - read_file_type, resolve_extraction_conflict, sanitize_archive_mode, set_permission_mode, - validate_dest_inside_root, validate_entry_path, validate_symlink_target, + self, BytesFmt, FileType, FileVisibilityPolicy, PathFmt, canonical_dest_inside_root, canonicalize, + create_symlink, is_same_file_as_output, read_file_type, resolve_extraction_conflict, sanitize_archive_mode, + set_permission_mode, validate_dest_inside_root, validate_entry_path, validate_symlink_target, }, warning, }; @@ -54,6 +54,7 @@ pub fn unpack_archive(reader: impl Read, output_folder: &Path, question_policy: let Some(dest) = resolve_extraction_conflict(&full_path, question_policy)? else { continue; }; + let dest = canonical_dest_inside_root(output_folder, &dest)?; create_symlink(&target, &dest)?; written = Some(dest); } @@ -75,6 +76,7 @@ pub fn unpack_archive(reader: impl Read, output_folder: &Path, question_policy: let Some(dest) = resolve_extraction_conflict(&full_link_path, question_policy)? else { continue; }; + let dest = canonical_dest_inside_root(output_folder, &dest)?; fs::hard_link(&full_target_path, &dest)?; written = Some(dest); } @@ -89,11 +91,14 @@ pub fn unpack_archive(reader: impl Read, output_folder: &Path, question_policy: continue; }; - if dest == full_path { + let dest = if dest == full_path { entry.unpack_in(output_folder)?; + dest } else { + let dest = canonical_dest_inside_root(output_folder, &dest)?; entry.unpack(&dest)?; - } + dest + }; written = Some(dest); } tar::EntryType::Directory => { diff --git a/src/archive/zip.rs b/src/archive/zip.rs index e482d3357..5da04cd8c 100644 --- a/src/archive/zip.rs +++ b/src/archive/zip.rs @@ -22,10 +22,10 @@ use crate::{ 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, resolve_extraction_conflict, - strip_cur_dir, validate_dest_inside_root, validate_symlink_target, + BytesFmt, FileType, FileVisibilityPolicy, PathFmt, canonical_dest_inside_root, canonicalize, + cd_into_same_dir_as, copy_limited_decompression, create_dir_inside_root, create_symlink, + get_invalid_utf8_paths, is_same_file_as_output, pretty_format_list_of_paths, read_file_type, + resolve_extraction_conflict, validate_dest_inside_root, validate_symlink_target, }, warning, }; @@ -77,24 +77,25 @@ where let target = symlink_target_from_bytes(&target_bytes); validate_symlink_target(&relpath, &target)?; + let file_path = canonical_dest_inside_root(output_folder, &file_path)?; #[cfg(unix)] std::os::unix::fs::symlink(&target, &file_path)?; #[cfg(windows)] - std::os::windows::fs::symlink_dir(&target, file_path)?; + std::os::windows::fs::symlink_dir(&target, &file_path)?; } else { - fs::create_dir_all(&file_path)?; + create_dir_inside_root(output_folder, &file_path)?; } } _is_file @ false => { - ensure_parent_dir_exists(&file_path)?; - let file_path = strip_cur_dir(file_path.as_path()); + create_dir_inside_root(output_folder, file_path.parent().unwrap_or(output_folder))?; let mode = file.unix_mode(); let is_symlink = mode.is_some_and(|mode| mode & 0o170000 == 0o120000); - let Some(resolved) = resolve_extraction_conflict(file_path, question_policy)? else { + let Some(resolved) = resolve_extraction_conflict(&file_path, question_policy)? else { continue; }; + let resolved = canonical_dest_inside_root(output_folder, &resolved)?; let file_path = resolved.as_path(); if is_symlink { @@ -114,13 +115,12 @@ where let mode = file.unix_mode().and_then(valid_unix_permissions).unwrap_or(0o644); fs::OpenOptions::new() .write(true) - .create(true) - .truncate(true) + .create_new(true) .mode(mode) .open(file_path)? }; #[cfg(not(unix))] - let mut output_file = fs::File::create(file_path)?; + let mut output_file = fs::OpenOptions::new().write(true).create_new(true).open(file_path)?; { copy_limited_decompression(&mut file, &mut output_file)?; } diff --git a/src/commands/decompress.rs b/src/commands/decompress.rs index 8a05c3733..7f893a675 100644 --- a/src/commands/decompress.rs +++ b/src/commands/decompress.rs @@ -200,6 +200,17 @@ pub fn decompress_file(options: DecompressOptions) -> Result<()> { }; let final_output_path = dir.join(file_name); + #[cfg(unix)] + let mut writer = { + use fs_err::os::unix::fs::OpenOptionsExt; + fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .custom_flags(libc::O_NOFOLLOW) + .open(&final_output_path)? + }; + #[cfg(not(unix))] let mut writer = fs::File::create(&final_output_path)?; io::copy(&mut reader, &mut writer)?; ControlFlow::Continue(DecompressionSummary::NonArchive { diff --git a/src/utils/fs.rs b/src/utils/fs.rs index e9b1d4a0b..ad64120ba 100644 --- a/src/utils/fs.rs +++ b/src/utils/fs.rs @@ -217,6 +217,25 @@ pub fn validate_dest_inside_root(root: &Path, dest: &Path) -> Result<()> { Ok(()) } +/// Return `dest` under its canonical parent and fail if that parent is outside `root`. +pub fn canonical_dest_inside_root(root: &Path, dest: &Path) -> Result { + let file_name = dest.file_name().ok_or_else(|| { + FinalError::with_title("refusing to extract entry without a final path component") + .detail(format!("dest: {}", PathFmt(dest))) + })?; + let parent = dest.parent().unwrap_or(root); + let canon_parent = canonicalize(parent)?; + let canon_root = canonicalize(root)?; + if !canon_parent.starts_with(&canon_root) { + return Err( + FinalError::with_title("refusing to traverse on-disk symlink during extraction") + .detail(format!("path: {}", PathFmt(&canon_parent))) + .into(), + ); + } + Ok(canon_parent.join(file_name)) +} + /// LZMA/XZ dictionary memory cap (256 MiB). Bounds malformed-stream allocations pub const LZMA_MEMLIMIT_BYTES: u32 = 256 * 1024 * 1024; @@ -294,11 +313,26 @@ impl Drop for CwdGuard { } } -pub fn ensure_parent_dir_exists(file_path: &Path) -> io::Result<()> { - if let Some(parent) = file_path.parent() - && !parent.fs_err_try_exists()? - { - fs::create_dir_all(parent)?; +/// Create each missing directory from `root` down to `dir` and fail if a component is a symlink. +pub fn create_dir_inside_root(root: &Path, dir: &Path) -> Result<()> { + let rel = dir.strip_prefix(root).map_err(|_| { + FinalError::with_title("refusing to create a directory outside extraction root") + .detail(format!("dir: {}", PathFmt(dir))) + })?; + let mut probe = root.to_path_buf(); + for comp in rel.components() { + probe.push(comp); + match fs::symlink_metadata(&probe) { + Ok(md) if md.is_dir() => {} + Ok(_) => { + return Err( + FinalError::with_title("refusing to traverse on-disk symlink during extraction") + .detail(format!("path: {}", PathFmt(&probe))) + .into(), + ); + } + Err(_) => fs::create_dir(&probe)?, + } } Ok(()) } From 14b443b198b8346b7ba329b8466c9e0b6fe29139 Mon Sep 17 00:00:00 2001 From: valoq Date: Sun, 11 Oct 2026 16:34:07 +0200 Subject: [PATCH 5/5] add unit tests --- src/utils/fs.rs | 66 ++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 60 insertions(+), 6 deletions(-) diff --git a/src/utils/fs.rs b/src/utils/fs.rs index ad64120ba..adb3bfee1 100644 --- a/src/utils/fs.rs +++ b/src/utils/fs.rs @@ -324,12 +324,15 @@ pub fn create_dir_inside_root(root: &Path, dir: &Path) -> Result<()> { probe.push(comp); match fs::symlink_metadata(&probe) { Ok(md) if md.is_dir() => {} - Ok(_) => { - return Err( - FinalError::with_title("refusing to traverse on-disk symlink during extraction") - .detail(format!("path: {}", PathFmt(&probe))) - .into(), - ); + Ok(md) => { + let title = if md.file_type().is_symlink() { + "refusing to traverse on-disk symlink during extraction" + } else { + "refusing to extract into a path that is not a directory" + }; + return Err(FinalError::with_title(title) + .detail(format!("path: {}", PathFmt(&probe))) + .into()); } Err(_) => fs::create_dir(&probe)?, } @@ -567,4 +570,55 @@ mod tests { fn try_infer_format_returns_none_on_missing_file() { assert_eq!(try_infer_format(Path::new("/nonexistent/path/to/nothing")), None); } + + #[cfg(unix)] + #[test] + fn create_dir_inside_root_refuses_symlinked_dir() { + let root = tempfile::tempdir().unwrap(); + let outside = tempfile::tempdir().unwrap(); + std::os::unix::fs::symlink(outside.path(), root.path().join("sub")).unwrap(); + let err = create_dir_inside_root(root.path(), &root.path().join("sub/inner")).unwrap_err(); + assert!(err.to_string().contains("symlink"), "{err}"); + assert!(!outside.path().join("inner").exists()); + } + + #[cfg(unix)] + #[test] + fn canonical_dest_inside_root_refuses_symlinked_parent() { + let root = tempfile::tempdir().unwrap(); + let outside = tempfile::tempdir().unwrap(); + std::os::unix::fs::symlink(outside.path(), root.path().join("sub")).unwrap(); + assert!(canonical_dest_inside_root(root.path(), &root.path().join("sub/file")).is_err()); + } + + #[cfg(unix)] + #[test] + fn resolve_extraction_conflict_unlinks_symlink_instead_of_following_it() { + let root = tempfile::tempdir().unwrap(); + let outside = tempfile::tempdir().unwrap(); + let secret = outside.path().join("secret"); + std::fs::write(&secret, "keep").unwrap(); + let link = root.path().join("file"); + std::os::unix::fs::symlink(&secret, &link).unwrap(); + + let dest = resolve_extraction_conflict(&link, QuestionPolicy::AlwaysYes).unwrap(); + assert_eq!(dest.as_deref(), Some(link.as_path())); + assert!(std::fs::symlink_metadata(&link).is_err()); + assert_eq!(std::fs::read_to_string(&secret).unwrap(), "keep"); + } + + #[cfg(unix)] + #[test] + fn resolve_path_conflict_unlinks_symlinked_dir_and_keeps_its_target() { + let root = tempfile::tempdir().unwrap(); + let outside = tempfile::tempdir().unwrap(); + std::fs::write(outside.path().join("old"), "keep").unwrap(); + let link = root.path().join("out"); + std::os::unix::fs::symlink(outside.path(), &link).unwrap(); + + let dest = resolve_path_conflict(&link, QuestionPolicy::AlwaysYes, QuestionAction::Decompression).unwrap(); + assert_eq!(dest.as_deref(), Some(link.as_path())); + assert!(std::fs::symlink_metadata(&link).is_err()); + assert_eq!(std::fs::read_to_string(outside.path().join("old")).unwrap(), "keep"); + } }