diff --git a/Cargo.lock b/Cargo.lock index 7e3c2d7..161f7a4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -309,7 +309,7 @@ checksum = "c8d4a3bb8b1e0c1050499d1815f5ab16d04f0959b233085fb31653fbfc9d98f9" [[package]] name = "collapse-cli" -version = "0.7.0" +version = "0.8.0" dependencies = [ "axum", "clap", @@ -319,11 +319,12 @@ dependencies = [ "tempfile", "thiserror", "tokio", + "zip", ] [[package]] name = "collapse-core" -version = "0.7.0" +version = "0.8.0" dependencies = [ "same-file", "serde", @@ -337,7 +338,7 @@ dependencies = [ [[package]] name = "collapse-remote" -version = "0.7.0" +version = "0.8.0" dependencies = [ "axum", "collapse-core", @@ -352,7 +353,7 @@ dependencies = [ [[package]] name = "collapse-server-backend" -version = "0.7.0" +version = "0.8.0" dependencies = [ "axum", "clap", diff --git a/README.md b/README.md index 5252b64..0b5f28d 100644 --- a/README.md +++ b/README.md @@ -188,8 +188,8 @@ Requires **Rust 1.88+** (2021 edition). ```bash make build # build the Rust crates -make test # run every suite (417 Rust tests + 76 Vitest cases) -make test/rust # only the Rust tests that need no Node toolchain (327) +make test # run every suite (585 Rust tests + 113 Vitest cases) +make test/rust # only the Rust tests that need no Node toolchain (470) ``` `make test` includes the desktop app's own Rust suite, which compiles Tauri, so diff --git a/apps/cli/Cargo.toml b/apps/cli/Cargo.toml index 1de9bb1..991ed17 100644 --- a/apps/cli/Cargo.toml +++ b/apps/cli/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "collapse-cli" -version = "0.7.0" +version = "0.8.0" edition = "2021" license = "GPL-3.0-only" @@ -28,5 +28,10 @@ collapse-core = { path = "../core" } collapse-server-backend = { path = "../server-backend" } clap = { version = "4", features = ["derive"] } tempfile = "3" +# tests/names.rs writes entry names no filesystem here would accept, which is +# the only way to reach the refusal path on a machine that is not Windows. +# Nothing that goes through core can spell such a name, since core takes its +# names from files that exist. +zip = "2" tokio = { version = "1", features = ["rt", "net"] } axum = "0.8" diff --git a/apps/cli/src/lib.rs b/apps/cli/src/lib.rs index bd690a7..7948f65 100644 --- a/apps/cli/src/lib.rs +++ b/apps/cli/src/lib.rs @@ -2,11 +2,15 @@ //! an archive, on top of `collapse-core` — or, with `--server`, through a //! remote collapse-server-backend instance. +use std::fmt::Write as _; use std::path::{Path, PathBuf}; use clap::{Parser, Subcommand, ValueEnum}; use collapse_core::paths::{inside, same_file}; -use collapse_core::{compress, compress_dir, extract, Algorithm}; +use collapse_core::{ + compress, compress_dir, extract, unwritable_names_with, Algorithm, CharacterFault, NameProblem, + NameReport, NameRules, Substitutions, Verify, +}; use thiserror::Error; /// Compress and extract files and folders. @@ -41,6 +45,12 @@ pub enum Command { #[arg(long)] force: bool, + /// Read every entry back, not just the archive's listing: about twice + /// the work, and it checks the per-entry checksums zip and 7z store + /// (tar stores none). + #[arg(long)] + verify: bool, + /// Compress on a remote Collapse server instead of locally /// (e.g. http://localhost:8000). #[arg(long, value_name = "URL")] @@ -83,21 +93,67 @@ impl From for Algorithm { pub enum Outcome { Compressed { output: PathBuf, + /// How thoroughly the archive was checked before it landed at + /// `output`, or `None` when this side checked nothing at all. + /// + /// `None` is the remote path: the archive arrives already finished and + /// the list of entries to hold it against is the server's, not ours. + /// Reporting a depth there would claim a check that never ran, and the + /// whole point of the check is that a user can trust it. + checked: Option, }, Extracted { output_dir: PathBuf, + /// The names as written, which is what the engine returns. files: Vec, + /// Entries whose name this machine could not hold as the archive + /// spells it, and the name they were written under instead. + /// + /// Only the adjustments that need no answer land here (a trailing dot + /// or space to drop, a device name to suffix); anything needing a + /// replacement stopped the run before it started. Reported rather than + /// left to be noticed, because a rename the user did not ask for is + /// the kind of thing they should hear about from us and not from a + /// missing file later. + /// + /// Always empty on Unix, where the one character no name can hold is a + /// NUL, and that is a question rather than an adjustment. + adjusted: Vec, }, } +/// An entry the host could not name as the archive spells it, and what it is +/// called on disk. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Adjustment { + /// The name the archive spells. + pub entry: String, + /// The name on disk, relative to the output directory. + pub written: String, +} + impl Outcome { /// Print a human-readable summary to stdout. pub fn report(&self) { match self { - Outcome::Compressed { output } => { + // Only the deeper check is mentioned, because it is the only one + // the user asked for and paid for. The listing check runs on every + // local compression, so announcing it would be noise on every + // single run. + Outcome::Compressed { + output, + checked: Some(Verify::Contents), + } => { + println!("Created {} (contents verified)", output.display()); + } + Outcome::Compressed { output, .. } => { println!("Created {}", output.display()); } - Outcome::Extracted { output_dir, files } => { + Outcome::Extracted { + output_dir, + files, + adjusted, + } => { println!( "Extracted {} file(s) into {}", files.len(), @@ -106,6 +162,17 @@ impl Outcome { for file in files { println!(" {file}"); } + if !adjusted.is_empty() { + println!( + "{} name(s) this system cannot write were adjusted:", + adjusted.len() + ); + for change in adjusted { + // Quoted, because the difference between the two names + // can be a trailing space, which is invisible unquoted. + println!(" {:?} was written as {:?}", change.entry, change.written); + } + } } } } @@ -131,6 +198,27 @@ pub enum CliError { #[error("invalid path: {}", .0.display())] InvalidPath(PathBuf), + /// `--verify` asks for work that happens where the archive is built, and + /// with `--server` that is the other end of the wire. + /// + /// Refused rather than ignored: a flag that asks for a stronger guarantee + /// is the last one to silently do nothing. It is also not something this + /// side can make up for by checking the download, because for a directory + /// it has no list of entries to hold the archive against. + #[error("--verify cannot be used with --server: the archive is built on the server, which this build has no way to ask for that check (compress locally to use --verify)")] + RemoteVerifyUnsupported, + + /// The archive holds entry names this machine cannot write, and at least + /// one of them needs an answer nobody can be asked for here. + /// + /// The whole message is built by [`unwritable_entries_message`]: it is the + /// only thing a user gets, so it is worth more than a sentence. + #[error("{}", unwritable_entries_message(.archive, .report))] + UnwritableEntries { + archive: PathBuf, + report: NameReport, + }, + #[error(transparent)] Remote(#[from] collapse_remote::RemoteError), @@ -150,8 +238,9 @@ pub fn run(cli: Cli) -> Result { level, output, force, + verify, server, - } => run_compress(path, format, level, output, force, server), + } => run_compress(path, format, level, output, force, verify, server), Command::Extract { archive, output } => run_extract(archive, output), } } @@ -162,8 +251,17 @@ fn run_compress( level: u32, output: Option, force: bool, + verify: bool, server: Option, ) -> Result { + // First, ahead of the filesystem: this one is a mistake in the command + // itself, decidable from the arguments alone. Reporting "output already + // exists" first would send the user off to add --force and meet this on + // the next run. + if verify && server.is_some() { + return Err(CliError::RemoteVerifyUnsupported); + } + // Canonicalize so `.`/`..`/trailing slashes resolve to a real path with a // usable file name (and to detect an output that aliases the source). let source = source @@ -208,25 +306,39 @@ fn run_compress( return Err(CliError::UnsupportedSource(source)); } - match server.as_deref() { + // Every local compression is checked; --verify only says how deeply. The + // depth is bound once and then both handed to the engine and reported, so + // the Outcome cannot end up naming a check that did not happen. + let depth = if verify { + Verify::Contents + } else { + Verify::Index + }; + + let checked = match server.as_deref() { // Remote handles both shapes: a file goes as-is, a directory travels // as a tar envelope the server unwraps. Some(server) => { let archive = collapse_remote::compress_path(server, &source, algorithm, level)?; std::fs::write(&output, archive)?; + None + } + None if source.is_dir() => { + compress_dir(&source, &output, algorithm, level, depth)?; + Some(depth) } - None if source.is_dir() => compress_dir(&source, &output, algorithm, level)?, None => { let arcname = source .file_name() .ok_or_else(|| CliError::InvalidPath(source.clone()))? .to_string_lossy() .into_owned(); - compress(&source, &output, &arcname, algorithm, level)?; + compress(&source, &output, &arcname, algorithm, level, depth)?; + Some(depth) } - } + }; - Ok(Outcome::Compressed { output }) + Ok(Outcome::Compressed { output, checked }) } /// Resolve the archive format: explicit `--format`, else the output file's @@ -246,8 +358,173 @@ fn run_extract(archive: PathBuf, output_dir: PathBuf) -> Result Vec { + let no_answers = Substitutions::new(); + report + .entries + .iter() + .filter_map(|unwritable| { + let written = rules.rewrite_entry(&unwritable.entry, &no_answers).ok()?; + Some(Adjustment { + entry: unwritable.entry.clone(), + written: written.to_string_lossy().into_owned(), + }) + }) + .collect() +} + +/// Why an archive was refused, which entries are at fault, what is wrong with +/// each, and what would let the user get their files. +/// +/// Public and pure so it can be read back on any machine: a [`NameReport`] is +/// data, and `NameReport::of(names, NameRules::windows())` builds the Windows +/// one from a Mac. This message is the entire feature on the command line, and +/// a message only Windows can produce is a message nobody here would ever read +/// before a user does. +pub fn unwritable_entries_message(archive: &Path, report: &NameReport) -> String { + let mut message = String::new(); + let count = report.entries.len(); + let plural = if count == 1 { "name" } else { "names" }; + // `write!` into a String cannot fail, so the results are dropped rather + // than dressed up as an error this function has no way to return. + let _ = write!( + message, + "cannot extract {}: {count} entry {plural} cannot be written on this system", + archive.display() + ); + + for unwritable in &report.entries { + // Quoted, because a name whose fault is a trailing space says nothing + // at all unquoted. + let _ = write!(message, "\n {:?}", unwritable.entry); + for problem in &unwritable.problems { + let _ = write!(message, "\n {}", explain(problem)); + } + } + + let _ = write!(message, "\nNothing was extracted."); + if !report.characters.is_empty() { + let (needed, them) = if report.characters.len() == 1 { + ("a replacement for", "it") + } else { + ("replacements for", "them") + }; + let _ = write!( + message, + " Going ahead needs {needed} {}, and this command cannot ask for {them} mid-run \ + without becoming interactive: the Collapse desktop app asks once per character, \ + checks the answer is writable too, and extracts with it.", + listed(report) + ); + } + if report + .entries + .iter() + .any(|e| e.problems.iter().any(|p| p.replaceable().is_none())) + { + let _ = write!( + message, + " The names above with nothing to replace are adjusted for you once it can go ahead." + ); + } + message +} + +/// One line for one problem, about the machine in front of the user ("here") +/// rather than about filesystems in general, because that is what the rules +/// are: what this host can hold, not what is portable. +fn explain(problem: &NameProblem) -> String { + match problem { + NameProblem::Character { + character, + fault: CharacterFault::Rejected, + } => format!("{character:?} cannot appear in a file name here"), + // The loud/silent split is the whole difference between issues #64 and + // #63, and it is what the user needs to hear: one is a name the system + // refuses, the other is a name it accepts and reads as something else. + NameProblem::Character { + character, + fault: CharacterFault::Reinterpreted, + } => format!( + "{character:?} is not read as part of a name here: the entry would be attached to \ + another file as hidden data instead of becoming a file, with no error" + ), + NameProblem::TrailingCharacters { removed } => format!( + "the name ends in {removed:?}, which this system does not keep, so it would be dropped" + ), + NameProblem::ReservedDevice { device } => format!( + "{device:?} names a device rather than a file, in every directory, so it would be \ + written under an adjusted name" + ), + } +} + +/// The characters holding the archive up, with how much of it each holds up: +/// `'?' (2 entries) and ':' (1 entry)`. +fn listed(report: &NameReport) -> String { + let parts: Vec = report + .characters + .iter() + .map(|offender| { + let unit = if offender.entries == 1 { + "entry" + } else { + "entries" + }; + format!("{:?} ({} {unit})", offender.character, offender.entries) + }) + .collect(); + match parts.split_last() { + None => String::new(), + Some((last, [])) => last.clone(), + Some((last, rest)) => format!("{} and {last}", rest.join(", ")), + } } /// Derive the default archive path: `.` next to the source. diff --git a/apps/cli/tests/cli.rs b/apps/cli/tests/cli.rs index 03615c5..54c5d13 100644 --- a/apps/cli/tests/cli.rs +++ b/apps/cli/tests/cli.rs @@ -3,6 +3,8 @@ use clap::Parser; use collapse_cli::{run, Cli, CliError, Command, Outcome}; +use collapse_core::compression::verify_archive; +use collapse_core::{Algorithm, Verify}; /// Parse an argv-style slice through the real CLI definition. fn parse(args: &[&str]) -> Result { @@ -19,7 +21,19 @@ fn run_err(args: &[&str]) -> CliError { fn compressed_output(outcome: Outcome) -> std::path::PathBuf { match outcome { - Outcome::Compressed { output } => output, + Outcome::Compressed { output, .. } => output, + other => panic!("expected compressed, got {other:?}"), + } +} + +/// The depth `run` says it checked the archive at. +/// +/// `run_compress` binds that depth once and uses the same binding for the call +/// into the engine and for the `Outcome`, so this is what the engine was +/// given, not a second opinion about it. +fn checked_depth(outcome: Outcome) -> Option { + match outcome { + Outcome::Compressed { checked, .. } => checked, other => panic!("expected compressed, got {other:?}"), } } @@ -52,6 +66,7 @@ fn compress_parses_defaults() { level, output, force, + verify, format, server, } => { @@ -60,6 +75,9 @@ fn compress_parses_defaults() { assert!(output.is_none()); assert!(format.is_none()); assert!(!force); + // Off unless asked for: the deeper check costs about twice the + // work, so it can only ever be opt-in. + assert!(!verify); assert!(server.is_none()); } _ => panic!("expected compress"), @@ -287,6 +305,160 @@ fn tar_output_is_independent_of_level() { assert_eq!(std::fs::read(&a1).unwrap(), std::fs::read(&a5).unwrap()); } +// -------------------------------------------------------------------- verify -- + +/// Bytes deflate and LZMA2 cannot shrink, so the archive's data region is about +/// as long as the input and a byte flipped in the middle of the file is +/// certainly inside it rather than in a header or the listing. Same +/// construction, and same reason, as `incompressible` in core's `verify.rs`. +fn incompressible(len: usize) -> Vec { + (0..len) + .map(|i| ((i as u64).wrapping_mul(2_654_435_761) >> 13) as u8) + .collect() +} + +fn flip_byte(path: &std::path::Path, offset: usize) { + let mut bytes = std::fs::read(path).unwrap(); + bytes[offset] ^= 0xFF; + std::fs::write(path, &bytes).unwrap(); +} + +#[test] +fn verify_flag_parses() { + let cli = parse(&["collapse", "compress", "f.txt", "--verify"]).unwrap(); + match cli.command { + Command::Compress { verify, .. } => assert!(verify), + _ => panic!("expected compress"), + } +} + +/// Which check `--verify` asks the engine for, established by running that +/// check rather than by reading the CLI's source: the depth `run` reports is +/// the one it handed the engine, so pointing it at a damaged archive says what +/// the flag bought. +/// +/// The CLI cannot be made to write a corrupt archive (its compressors checksum +/// exactly the bytes they wrote), so the damage is done afterwards, to the +/// archive the CLI itself produced. +/// +/// Falsifiable in both directions: map `--verify` to the listing check and the +/// second half stops failing; make the listing check read entry data too and +/// the first half stops passing. It leans on `run_compress` binding the depth +/// once for both the call and the report, which is why that binding is single +/// and says so. +#[test] +fn verify_asks_for_the_check_that_catches_a_corrupt_entry() { + let dir = tempfile::TempDir::new().unwrap(); + let src = dir.path().join("notes.bin"); + std::fs::write(&src, incompressible(8192)).unwrap(); + + let compress_to = |archive: &std::path::Path, flag: Option<&str>| -> Verify { + let mut args = vec![ + "collapse", + "compress", + src.to_str().unwrap(), + "-o", + archive.to_str().unwrap(), + ]; + args.extend(flag); + checked_depth(run_ok(&args)).expect("a local compression checks the archive it wrote") + }; + + let plain = dir.path().join("plain.zip"); + let deep = dir.path().join("deep.zip"); + let shallow_depth = compress_to(&plain, None); + let deep_depth = compress_to(&deep, Some("--verify")); + + // Both archives are sound as written, so the difference between the two + // depths is invisible until one of them is damaged. Halfway through the + // file: zip puts the entry's data first and its listing at the end, and + // this data does not compress, so the flip lands in the payload. + for archive in [&plain, &deep] { + let midpoint = std::fs::metadata(archive).unwrap().len() as usize / 2; + flip_byte(archive, midpoint); + } + + let expected = ["notes.bin".to_string()]; + assert!( + verify_archive(&plain, Algorithm::Zip, &expected, shallow_depth).is_ok(), + "the default depth reads the listing, which a flipped data byte leaves intact" + ); + let caught = verify_archive(&deep, Algorithm::Zip, &expected, deep_depth) + .expect_err("--verify must ask for the depth that reads every entry back"); + assert!( + caught.to_string().contains("notes.bin"), + "the failure names the entry that went bad: {caught}" + ); + + // And the same two answers in the engine's own words. + assert_eq!(shallow_depth, Verify::Index); + assert_eq!(deep_depth, Verify::Contents); +} + +/// The deeper check must pass on a healthy archive, for every format and both +/// shapes. It runs on the archive's way to the destination, so a false +/// positive would not merely be noise: nothing would land at all, and +/// `--verify` would be unusable. The tree carries the two entries most likely +/// to trip a reader that assumes every entry has data, an empty file and an +/// empty directory. +/// +/// Falsifiable: have the contents check read a directory entry as a stream, or +/// treat a zero-length entry as a short read, and this goes red. +#[test] +fn verify_still_lands_a_correct_archive_for_every_format() { + for (fmt, ext) in [("zip", "zip"), ("7z", "7z"), ("tar", "tar")] { + let dir = tempfile::TempDir::new().unwrap(); + + let file = dir.path().join("notes.txt"); + std::fs::write(&file, b"hello verify").unwrap(); + let file_archive = dir.path().join(format!("file.{ext}")); + let outcome = run_ok(&[ + "collapse", + "compress", + file.to_str().unwrap(), + "-f", + fmt, + "-o", + file_archive.to_str().unwrap(), + "--verify", + ]); + assert_eq!(checked_depth(outcome), Some(Verify::Contents), "{fmt}"); + let out = dir.path().join("file-out"); + assert_eq!( + listing(collapse_core::extract(&file_archive, &out).unwrap()), + vec!["notes.txt"], + "{fmt}" + ); + + let root = dir.path().join("photos"); + std::fs::create_dir_all(root.join("empty_dir")).unwrap(); + std::fs::write(root.join("a.txt"), b"alpha").unwrap(); + std::fs::write(root.join("empty.txt"), b"").unwrap(); + let dir_archive = dir.path().join(format!("tree.{ext}")); + let outcome = run_ok(&[ + "collapse", + "compress", + root.to_str().unwrap(), + "-f", + fmt, + "-o", + dir_archive.to_str().unwrap(), + "--verify", + ]); + assert_eq!(checked_depth(outcome), Some(Verify::Contents), "{fmt}"); + let out = dir.path().join("tree-out"); + assert_eq!( + listing(collapse_core::extract(&dir_archive, &out).unwrap()), + vec!["photos/a.txt", "photos/empty.txt"], + "{fmt}" + ); + assert!( + out.join("photos").join("empty_dir").is_dir(), + "{fmt}: the empty directory survived the round trip" + ); + } +} + // ----------------------------------------------------------------- extraction -- #[test] @@ -295,7 +467,7 @@ fn extract_lists_and_writes_files() { let src = dir.path().join("data.bin"); std::fs::write(&src, b"payload").unwrap(); let archive = dir.path().join("data.zip"); - collapse_core::compress(&src, &archive, "data.bin", collapse_core::Algorithm::Zip, 1).unwrap(); + collapse_core::compress(&src, &archive, "data.bin", Algorithm::Zip, 1, Verify::Index).unwrap(); let out = dir.path().join("out"); let outcome = run_ok(&[ @@ -306,9 +478,18 @@ fn extract_lists_and_writes_files() { out.to_str().unwrap(), ]); match outcome { - Outcome::Extracted { output_dir, files } => { + Outcome::Extracted { + output_dir, + files, + adjusted, + } => { assert_eq!(output_dir, out); assert_eq!(listing(files), vec!["data.bin"]); + // A name this machine can write is written as it is spelled. The + // adjustment list exists for the Windows cases in tests/names.rs, + // and an ordinary archive must never populate it, or every run + // would end with a paragraph about renames that did not happen. + assert!(adjusted.is_empty()); } _ => panic!("expected extracted"), } @@ -321,7 +502,7 @@ fn extract_creates_deep_nested_output_dir() { let src = dir.path().join("data.bin"); std::fs::write(&src, b"deep").unwrap(); let archive = dir.path().join("data.zip"); - collapse_core::compress(&src, &archive, "data.bin", collapse_core::Algorithm::Zip, 1).unwrap(); + collapse_core::compress(&src, &archive, "data.bin", Algorithm::Zip, 1, Verify::Index).unwrap(); let out = dir.path().join("a/b/c"); assert!(!out.exists()); @@ -346,6 +527,44 @@ fn extract_unknown_extension_errors() { )); } +/// The same case-insensitive match, reached by the other road: with no +/// `--format`, the CLI infers the format from the output's extension, so +/// `-o backup.7Z` used to fall through to the zip default and write a zip +/// under a name promising a 7z. That archive was then refused by this same +/// CLI, since extraction dispatched on the extension too. +#[test] +fn compress_infers_the_format_from_an_uppercase_output_extension() { + for (shouted, magic) in [("BACKUP.7Z", &b"7z"[..]), ("BACKUP.ZIP", &b"PK"[..])] { + let dir = tempfile::TempDir::new().unwrap(); + let src = dir.path().join("notes.txt"); + std::fs::write(&src, b"body").unwrap(); + let archive = dir.path().join(shouted); + + run_ok(&[ + "collapse", + "compress", + src.to_str().unwrap(), + "-o", + archive.to_str().unwrap(), + ]); + + let written = std::fs::read(&archive).unwrap(); + assert_eq!( + &written[..magic.len()], + magic, + "{shouted} should hold what its name promises" + ); + + // And the round trip closes: the archive this CLI wrote is one it reads. + let out = dir.path().join("out"); + assert_eq!( + collapse_core::extract(&archive, &out).unwrap(), + vec!["notes.txt"], + "{shouted}" + ); + } +} + // ------------------------------------------------------- safety / data loss -- #[test] diff --git a/apps/cli/tests/names.rs b/apps/cli/tests/names.rs new file mode 100644 index 0000000..f12fb9a --- /dev/null +++ b/apps/cli/tests/names.rs @@ -0,0 +1,275 @@ +//! What the CLI does with an archive holding entry names this machine cannot +//! write as files (issues #63 and #64). +//! +//! A command line cannot stop and ask, so it refuses instead, and the refusal +//! is the whole feature here: it has to name the entries, say what is wrong +//! with each one, and point at something that would get the user their files. +//! +//! Two kinds of test, and the split is deliberate. The ones that judge the +//! *message* build a `NameReport` against `NameRules::windows()`, so they read +//! the Windows refusal on the Mac this is written on and on the Linux CI leg; +//! the report is data, and asking for another platform's rules is a function +//! call. The ones that drive `run` end to end use the host's rules and a NUL +//! byte, which is the one character no filesystem anywhere will take, so they +//! assert the same thing on every platform. A test reachable only under +//! `#[cfg(windows)]` is a test this repository never runs. + +use std::io::Write as _; +use std::path::{Path, PathBuf}; + +use clap::Parser; +use collapse_cli::{adjustments, run, Adjustment, Cli, CliError}; +use collapse_core::{NameReport, NameRules}; + +fn run_err(args: &[&str]) -> CliError { + run(Cli::try_parse_from(args).expect("args should parse")).expect_err("command should fail") +} + +/// Write a zip holding exactly these entries, names included, with no opinion +/// about them. +/// +/// Everything in core takes its entry names from files that exist, so nothing +/// there can spell a name the local filesystem refuses. Reaching the refusal +/// path at all means writing the archive by hand. +fn zip_named(path: &Path, entries: &[(&str, &[u8])]) { + let options = + zip::write::SimpleFileOptions::default().compression_method(zip::CompressionMethod::Stored); + let mut writer = zip::ZipWriter::new(std::fs::File::create(path).unwrap()); + for (name, body) in entries { + writer.start_file(*name, options).unwrap(); + writer.write_all(body).unwrap(); + } + writer.finish().unwrap(); +} + +/// The refusal a Windows machine would print for this listing, read from a +/// machine that is not one. +fn windows_refusal(archive: &str, names: &[&str]) -> String { + CliError::UnwritableEntries { + archive: PathBuf::from(archive), + report: NameReport::of(names, NameRules::windows()), + } + .to_string() +} + +// ------------------------------------------------------------- the refusal -- + +/// Every offending entry is named, and every entry is told what is wrong with +/// it. Break any arm of the explanation and one of these disappears. +#[test] +fn the_refusal_names_every_bad_entry_and_says_what_is_wrong_with_each() { + let message = windows_refusal( + "report.zip", + &[ + "summary.txt", + "what?.txt", + "notes.txt.", + "CON", + "logs/a:b.txt", + ], + ); + + assert!( + message.starts_with("cannot extract report.zip: 4 entry names cannot be written"), + "the archive and the count come first: {message}" + ); + for named in ["what?.txt", "notes.txt.", "CON", "logs/a:b.txt"] { + assert!( + message.contains(named), + "{named:?} is unwritable and must be named: {message}" + ); + } + assert!( + !message.contains("summary.txt"), + "an entry that is perfectly writable is not the user's problem: {message}" + ); + + // One reason per kind of fault, each phrased for the person reading it. + assert!( + message.contains("'?' cannot appear in a file name here"), + "the rejected character says so: {message}" + ); + assert!( + message.contains("':' is not read as part of a name here"), + "the reinterpreted character is a different fault and reads differently: {message}" + ); + assert!( + message.contains("attached to another file as hidden data"), + "and the difference that matters is that it fails silently: {message}" + ); + assert!( + message.contains(r#"the name ends in ".", which this system does not keep"#), + "the trailing dot says which characters go: {message}" + ); + assert!( + message.contains(r#""CON" names a device"#), + "the device name says it is a device: {message}" + ); +} + +/// Nothing is half-written, and the user is told where to go. Drop the pointer +/// and the message becomes a dead end. +#[test] +fn the_refusal_says_nothing_was_written_and_where_to_go_next() { + let message = windows_refusal("report.zip", &["what?.txt"]); + + assert!( + message.contains("Nothing was extracted."), + "the state of the output directory is the first thing a user wonders about: {message}" + ); + assert!( + message.contains("a replacement for '?' (1 entry)"), + "what would unblock it, in the singular: {message}" + ); + assert!( + message.contains("desktop app"), + "the pointer at the one front end that can ask: {message}" + ); +} + +/// A UI puts one field per character, not one per file, and the message counts +/// the same way: the user needs to know how much of the archive one answer +/// buys. +#[test] +fn the_refusal_counts_the_entries_each_character_holds_up() { + let message = windows_refusal( + "report.zip", + &["what?.txt", "why?.txt", "logs/a:b.txt", "clean.txt"], + ); + + assert!( + message.contains("replacements for '?' (2 entries) and ':' (1 entry)"), + "both characters, both counts, and a plural that matches: {message}" + ); +} + +/// A trailing dot and a device name have one correct answer and nobody to ask, +/// so they are not what the refusal is about. Refusing them too would leave a +/// Windows user unable to extract an archive whose only fault is a file called +/// `aux.log`, which the desktop app would open without a word. +#[test] +fn a_name_that_needs_no_answer_is_adjusted_rather_than_refused() { + let windows = NameRules::windows(); + let report = NameReport::of(&["notes.txt.", "CON.txt", "aux.log", "fine.txt"], windows); + + assert_eq!( + adjustments(&report, windows), + vec![ + Adjustment { + entry: "notes.txt.".to_string(), + written: "notes.txt".to_string(), + }, + Adjustment { + entry: "CON.txt".to_string(), + written: "CON_.txt".to_string(), + }, + Adjustment { + entry: "aux.log".to_string(), + written: "aux_.log".to_string(), + }, + ], + "the device keeps the extension that says what the file is, and the writable name is absent" + ); +} + +/// An entry needing an answer has no adjustment to report, because there is no +/// answer to apply. That is what keeps the two lists disjoint: what `run` +/// refuses is exactly what this cannot rewrite. +#[test] +fn an_entry_needing_a_replacement_has_no_adjustment() { + let windows = NameRules::windows(); + let report = NameReport::of(&["what?.txt", "notes.txt."], windows); + + assert_eq!( + adjustments(&report, windows), + vec![Adjustment { + entry: "notes.txt.".to_string(), + written: "notes.txt".to_string(), + }], + ); +} + +// ------------------------------------------------------ end to end, on any host -- + +/// The refusal happens before anything is written, so the entries an archive +/// could have delivered are not left sitting in the output directory next to a +/// failure. +/// +/// A NUL byte is the character every filesystem here refuses (`NameRules::unix` +/// lists it and nothing else, and Windows refuses every control character), so +/// this exercises the host path wherever it runs. +#[test] +fn extract_refuses_an_archive_this_host_cannot_name_and_writes_nothing() { + let dir = tempfile::TempDir::new().unwrap(); + let archive = dir.path().join("mixed.zip"); + zip_named( + &archive, + &[("keep.txt", b"kept"), ("no\u{0}pe.txt", b"impossible")], + ); + let out = dir.path().join("out"); + + let error = run_err(&[ + "collapse", + "extract", + archive.to_str().unwrap(), + "-o", + out.to_str().unwrap(), + ]); + + // Not the engine's own refusal (which names one entry and one character): + // the CLI's, which surveys the whole listing first. + assert!( + matches!(&error, CliError::UnwritableEntries { report, .. } if report.entries.len() == 1), + "expected the CLI's up-front refusal, got {error:?}" + ); + let message = error.to_string(); + assert!( + message.contains(r"no\0pe.txt"), + "the entry is named, escaped so an unprintable character is visible: {message}" + ); + + assert!( + !out.join("keep.txt").exists(), + "the writable entry must not have been extracted: refusing halfway would leave the user \ + with a directory nobody can tell apart from a complete one" + ); +} + +// ------------------------------------------------- an ordinary write failure -- + +/// Piece 1 of #64 at the surface a user sees: when a write fails for a reason +/// that has nothing to do with names (here a parent that is already a file, +/// which is how a read-only directory or a full disk arrives too), the message +/// says which entry it was. +/// +/// Before this, everything above was `error: IO error: File exists (os error +/// 17)`: no entry, no archive, nothing to act on. +#[test] +fn a_failing_entry_names_itself_in_the_error() { + let dir = tempfile::TempDir::new().unwrap(); + let archive = dir.path().join("conflict.zip"); + // `x` is written as a file, and then `x/y.txt` needs `x` to be a directory. + zip_named(&archive, &[("x", b"a file"), ("x/y.txt", b"under it")]); + let out = dir.path().join("out"); + + let message = run_err(&[ + "collapse", + "extract", + archive.to_str().unwrap(), + "-o", + out.to_str().unwrap(), + ]) + .to_string(); + + assert!( + message.contains("x/y.txt"), + "the entry that could not be written is named: {message}" + ); + // See the twin in apps/core/tests/names.rs: on Windows the message is built + // from a canonicalized root, so compare against what was actually resolved. + let resolved = out.canonicalize().unwrap_or_else(|_| out.clone()); + assert!( + message.contains(&resolved.display().to_string()), + "and where it was going: {message}" + ); +} diff --git a/apps/cli/tests/remote.rs b/apps/cli/tests/remote.rs index 1e68bd9..861cab6 100644 --- a/apps/cli/tests/remote.rs +++ b/apps/cli/tests/remote.rs @@ -18,7 +18,16 @@ fn run_err(args: &[&str]) -> CliError { fn compressed_output(outcome: Outcome) -> std::path::PathBuf { match outcome { - Outcome::Compressed { output } => output, + Outcome::Compressed { output, .. } => output, + other => panic!("expected compressed, got {other:?}"), + } +} + +/// The depth `run` says it checked the archive at, `None` when it checked +/// nothing. Same helper as in `tests/cli.rs`. +fn checked_depth(outcome: Outcome) -> Option { + match outcome { + Outcome::Compressed { checked, .. } => checked, other => panic!("expected compressed, got {other:?}"), } } @@ -71,6 +80,19 @@ fn start_server() -> (String, std::path::PathBuf) { // guard-order tests point --server here to prove no request is ever made. const UNREACHABLE: &str = "http://127.0.0.1:9"; +/// `RemoteError::BlankServer` rendered, which is what a user of the CLI reads +/// verbatim. Spelled out here rather than matched in fragments so a front-end +/// that started decorating the message would fail: the point of moving this +/// answer into `collapse-remote` was that both apps say the same sentence. +const BLANK_ADDRESS: &str = + "the server address is blank: it needs a URL, for example http://localhost:8000"; + +/// `CliError::RemoteVerifyUnsupported` rendered. Spelled out in full because +/// this sentence is the entire answer the user gets: it has to name the flag +/// that cannot be honoured, why, and what to do instead, and a `contains` +/// check on a fragment would not notice any of the three going missing. +const VERIFY_NOT_REMOTE: &str = "--verify cannot be used with --server: the archive is built on the server, which this build has no way to ask for that check (compress locally to use --verify)"; + // ------------------------------------------------------------------ parsing -- #[test] @@ -290,6 +312,181 @@ fn remote_compress_unreachable_server_errors() { assert!(!dir.path().join("notes.txt.zip").exists()); } +// ---------------------------------------------------------- verify vs server -- + +/// `--verify` asks for a check that happens where the archive is built, and +/// with `--server` that is the other machine. This build cannot ask the server +/// for it, so the combination is refused: a flag whose whole purpose is a +/// stronger guarantee is the last one that may quietly do nothing. +/// +/// Two things are pinned besides the message. The refusal comes ahead of the +/// filesystem guards, so the output that already exists here is reported as +/// the flag mistake it is rather than sending the user off to add `--force` +/// and meet this on the next run. And it is a refusal, not a fallback: no +/// archive appears anywhere, least of all one compressed locally that the user +/// would take for the server's work. +/// +/// The address points at a port nothing listens on, so nothing here depends on +/// a server existing; if the guard were ever moved after the dispatch, this +/// would fail as a connection error instead. +#[test] +fn verify_is_refused_with_server_rather_than_ignored() { + let dir = tempfile::TempDir::new().unwrap(); + let src = dir.path().join("notes.txt"); + std::fs::write(&src, b"stay home").unwrap(); + let archive = dir.path().join("out.zip"); + std::fs::write(&archive, b"pre-existing").unwrap(); + + let err = run_err(&[ + "collapse", + "compress", + src.to_str().unwrap(), + "-o", + archive.to_str().unwrap(), + "--verify", + "--server", + UNREACHABLE, + ]); + + assert!( + matches!(err, CliError::RemoteVerifyUnsupported), + "got {err:?}" + ); + assert_eq!(err.to_string(), VERIFY_NOT_REMOTE); + assert_eq!( + std::fs::read(&archive).unwrap(), + b"pre-existing", + "the refusal must not have touched the file it was aimed at" + ); + assert!( + !dir.path().join("notes.txt.zip").exists(), + "and it must not have compressed locally instead" + ); +} + +/// Without `--verify` the two flags never meet, so `--server` keeps working +/// exactly as before. +#[test] +fn server_without_verify_is_unaffected() { + let (server, _storage) = start_server(); + let dir = tempfile::TempDir::new().unwrap(); + let src = dir.path().join("notes.txt"); + std::fs::write(&src, b"compressed far away").unwrap(); + + let output = compressed_output(run_ok(&[ + "collapse", + "compress", + src.to_str().unwrap(), + "--server", + &server, + ])); + + let out = dir.path().join("out"); + assert_eq!( + listing(collapse_core::extract(&output, &out).unwrap()), + vec!["notes.txt"] + ); +} + +/// The remote path reports that it checked nothing, because it checked +/// nothing: the archive arrives finished, and the list of entries to hold it +/// against belongs to the server. Naming a depth here would have the CLI claim +/// a guarantee only the local path can give, and the claim would be invisible +/// to every other test in this file, which look at the archive rather than at +/// what was promised about it. +#[test] +fn remote_compress_claims_no_check_of_its_own() { + let (server, _storage) = start_server(); + let dir = tempfile::TempDir::new().unwrap(); + let src = dir.path().join("notes.txt"); + std::fs::write(&src, b"nobody checked this here").unwrap(); + + let outcome = run_ok(&[ + "collapse", + "compress", + src.to_str().unwrap(), + "--server", + &server, + ]); + assert_eq!(checked_depth(outcome), None); +} + +// ------------------------------------------------------------ blank address -- + +/// `--server ""` and `--server " "` are a flag typed wrong, and the +/// realistic way to get one is a wrapper script running +/// `--server "$COLLAPSE_SERVER"` with the variable unset. Both name the +/// address as the mistake instead of failing against a server with no name, +/// and neither quietly compresses locally: the message and the empty +/// directory are what tell the two apart. +/// +/// `collapse-remote` owns that answer, so the desktop's +/// `an_empty_server_string_is_refused_not_compressed_locally` and +/// `a_whitespace_only_server_string_is_refused_not_sent` assert the very same +/// message. The two front-ends used to disagree here (issue #65). +#[test] +fn remote_compress_rejects_a_blank_server() { + for blank in ["", " ", "\t"] { + let dir = tempfile::TempDir::new().unwrap(); + let src = dir.path().join("notes.txt"); + std::fs::write(&src, b"stay home").unwrap(); + + let err = run_err(&[ + "collapse", + "compress", + src.to_str().unwrap(), + "--server", + blank, + ]); + + assert!(matches!(err, CliError::Remote(_)), "{blank:?}: {err:?}"); + // The whole message, not a fragment of it. `CliError::Remote` is + // `#[error(transparent)]`, which is what makes the CLI's wording and + // the desktop's the same string; giving the variant a format of its + // own ("remote error: {0}") would still contain every fragment a + // `contains` check looks for, so only equality can see that happen. + // The old wording, "cannot reach the server at : ...", sent the + // user hunting for a network problem that was never there. + assert_eq!(err.to_string(), BLANK_ADDRESS, "{blank:?}"); + + assert!( + !dir.path().join("notes.txt.zip").exists(), + "{blank:?} must not fall back to compressing locally" + ); + assert_eq!(std::fs::read(&src).unwrap(), b"stay home"); + } +} + +/// The dispatch's other arm. `--server` sends a directory as a tar envelope, +/// so a blank address there is refused before a whole tree is walked and +/// copied into a temporary tar (the ordering is pinned in +/// `apps/remote/tests/client.rs`). Covered separately because the file case +/// above cannot see it: a directory reaches the guard by a different route +/// and, if the refusal were ever softened into a local fallback, this is the +/// call that would quietly produce a full archive of the tree. +#[test] +fn remote_compress_rejects_a_blank_server_for_a_directory_too() { + let dir = tempfile::TempDir::new().unwrap(); + let root = dir.path().join("photos"); + std::fs::create_dir(&root).unwrap(); + std::fs::write(root.join("a.txt"), b"first").unwrap(); + + let err = run_err(&[ + "collapse", + "compress", + root.to_str().unwrap(), + "--server", + " ", + ]); + + assert_eq!(err.to_string(), BLANK_ADDRESS); + assert!( + !dir.path().join("photos.zip").exists(), + "the tree was archived locally instead of being reported" + ); + assert_eq!(std::fs::read(root.join("a.txt")).unwrap(), b"first"); +} + // ------------------------------------------------------- safety guard order -- #[test] diff --git a/apps/core/Cargo.toml b/apps/core/Cargo.toml index a5af77e..6463d9d 100644 --- a/apps/core/Cargo.toml +++ b/apps/core/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "collapse-core" -version = "0.7.0" +version = "0.8.0" edition = "2021" license = "GPL-3.0-only" diff --git a/apps/core/src/compression.rs b/apps/core/src/compression.rs index d1a122f..8d829a1 100644 --- a/apps/core/src/compression.rs +++ b/apps/core/src/compression.rs @@ -1,17 +1,30 @@ mod algorithm; +mod names; mod sevenz; mod tar; +mod verify; mod walk; mod zip; pub use self::algorithm::Algorithm; +pub use self::names::{ + CharacterFault, NameError, NameProblem, NameReport, NameRules, OffendingCharacter, + Substitutions, UnwritableEntry, +}; pub use self::sevenz::{compress_7z, compress_7z_dir, extract_7z}; pub use self::tar::{compress_tar, compress_tar_dir, extract_tar}; +pub use self::verify::{verify_archive, Verify}; pub use self::zip::{compress_zip, compress_zip_dir, extract_zip}; +pub(crate) use self::names::{plan_names, NamePlan}; +pub(crate) use self::sevenz::{list_7z_entries, read_7z_entries}; +pub(crate) use self::tar::{list_tar_entries, read_tar_entries}; pub(crate) use self::walk::walk_tree; +pub(crate) use self::zip::{list_zip_entries, read_zip_entries}; +use std::fs; use std::path::{Component, Path, PathBuf}; +use std::sync::atomic::{AtomicU64, Ordering}; use thiserror::Error; @@ -24,7 +37,24 @@ pub(crate) fn sanitize_entry_path(name: &str) -> Option { let mut safe = PathBuf::new(); for component in Path::new(name).components() { match component { - Component::Normal(part) => safe.push(part), + Component::Normal(part) => { + // `PathBuf::push` **replaces** what it holds when handed a path + // carrying a prefix, and Windows reads any `x:` at the start of + // a component as a drive. A prefix is only parsed at the head of + // a whole path, so `docs/c:evil.txt` offers no `Prefix` component + // for the arm below to reject, yet pushing its second component + // discards `docs` and leaves `c:evil.txt`: a drive-relative path + // that resolves against the current directory of C:, not the + // output directory. Re-parsing each part and demanding it still + // be exactly one `Normal` component is what closes that, and it + // leaves Unix (where the same string is an ordinary file name) + // untouched. + let mut parts = Path::new(part).components(); + match (parts.next(), parts.next()) { + (Some(Component::Normal(only)), None) if only == part => safe.push(only), + _ => return None, + } + } Component::CurDir => {} Component::ParentDir | Component::RootDir | Component::Prefix(_) => return None, } @@ -46,6 +76,201 @@ pub enum CompressionError { #[error("Invalid compression level: {0}. Must be between 1 and 5.")] InvalidLevel(u32), + + /// The compressor reported success, but reading the archive back showed it + /// was not what it was meant to be, so it was discarded and never reached + /// `archive`. + /// + /// Deliberately not a [`Self::Failed`] with a string: "the compressor + /// errored" and "the archive I just wrote is wrong" call for different + /// answers from a caller, and only this one means a bug or a corruption + /// rather than a file it could not read. + #[error("Verification of {} failed: {reason}", archive.display())] + VerificationFailed { archive: PathBuf, reason: String }, + + /// One entry could not be written, naming which one and where it was + /// going. + /// + /// [`Self::Io`] carries the operating system's sentence and nothing else, + /// so a read-only output directory, a full disk and a name whose parent is + /// already a file were all reported as the same blank message with no clue + /// which of an archive's entries was at fault (issue #64). + #[error("cannot write entry {entry:?} to {}: {source}", destination.display())] + Entry { + entry: String, + destination: PathBuf, + #[source] + source: std::io::Error, + }, + + /// An entry name this filesystem cannot hold, or an answer that does not + /// resolve one. + #[error(transparent)] + Name(#[from] NameError), +} + +/// Refuse a destination that resolved to somewhere outside the output directory. +/// +/// A lexical check on an entry name cannot see two things: a component the +/// host's path parser reads differently from the way the name was judged, and a +/// symlink that was already sitting in the output directory before extraction +/// began. Resolving the directory that was just created and requiring it to +/// still be inside covers both, and it is what tar's `unpack_in` has always +/// done through `validate_inside_dst`. zip and 7z had no equivalent at all: they +/// joined a sanitized path and wrote. +/// +/// `output` must already be canonical, and `resolved` must exist, so call this +/// after the parent directory has been created and before anything is written +/// into it. +pub(crate) fn ensure_inside( + output: &Path, + resolved: &Path, + entry: &str, +) -> Result<(), CompressionError> { + let real = resolved + .canonicalize() + .map_err(|e| entry_error(entry, resolved, e))?; + if !real.starts_with(output) { + return Err(CompressionError::Failed(format!( + "Path traversal detected in archive entry: {entry}" + ))); + } + Ok(()) +} + +/// Attach the entry and the destination to an IO failure at a write site. +pub(crate) fn entry_error( + entry: &str, + destination: &Path, + source: std::io::Error, +) -> CompressionError { + CompressionError::Entry { + entry: entry.to_string(), + destination: destination.to_path_buf(), + source, + } +} + +/// Serial number for staged file names, so two compressions running inside one +/// process cannot pick the same temporary. +static STAGED_SERIAL: AtomicU64 = AtomicU64::new(0); + +/// Bytes of the output's own name kept in the temporary's name. +/// +/// A file name has a length limit of its own (255 bytes on Linux and macOS), so +/// a long but perfectly legal output name would otherwise fail to stage. The +/// suffix below is at most 46 bytes, and 200 leaves room to spare. +const STAGED_NAME_BUDGET: usize = 200; + +/// An archive being written next to where it belongs, not at it. +/// +/// zip and tar finalise on drop, so a compression that died halfway used to +/// leave a *valid* archive at the destination silently missing entries: the +/// user saw an error, opened the archive, found it opened fine, and could +/// delete the originals. The bytes therefore go to a temporary file which is +/// renamed into place only once the archive checks out, so the destination only +/// ever holds a finished archive, and a failed run leaves whatever was already +/// there untouched. +/// +/// The temporary is a sibling of the output because a rename is only atomic, +/// and on most platforms only possible at all, within one filesystem. +/// +/// It is a guard rather than a pair of calls so that an early return through +/// `?`, a panic, or a future edit that adds a step between writing and renaming +/// cannot leak it. +struct StagedOutput { + path: PathBuf, + committed: bool, +} + +impl StagedOutput { + /// Pick a staging path beside `output`. Nothing is created here; the + /// backend creates the file when it writes to [`Self::path`]. + fn beside(output: &Path) -> Result { + let file_name = output.file_name().ok_or_else(|| { + CompressionError::Failed(format!( + "Not a path an archive can be written to: {}", + output.display() + )) + })?; + // `parent()` answers `Some("")` for a bare relative name like `out.zip`, + // and joining onto that would produce a path starting with a separator. + let dir = match output.parent() { + Some(parent) if !parent.as_os_str().is_empty() => parent.to_path_buf(), + _ => PathBuf::from("."), + }; + // Named after the output, marked as ours, and unique: two live + // processes cannot share a pid, and two compressions in one process + // cannot share a serial. A leftover after a crash says what it is and + // which file it was on its way to becoming. + let file_name = file_name.to_string_lossy(); + let stem = keep_bytes(&file_name, STAGED_NAME_BUDGET); + let path = dir.join(format!( + "{stem}.collapse-part-{}-{}", + std::process::id(), + STAGED_SERIAL.fetch_add(1, Ordering::Relaxed) + )); + Ok(Self { + path, + committed: false, + }) + } + + fn path(&self) -> &Path { + &self.path + } + + /// Move the finished archive to where the caller asked for it. + /// + /// A rename replaces the destination name rather than writing through it, + /// which is also what stops a hardlinked output from being written into the + /// file it shares an inode with. + fn commit(mut self, output: &Path) -> Result<(), CompressionError> { + fs::rename(&self.path, output)?; + self.committed = true; + Ok(()) + } +} + +impl Drop for StagedOutput { + fn drop(&mut self) { + if !self.committed { + // Best effort on purpose: whatever failure brought us here is what + // the caller needs to hear about, and there is nothing useful to do + // with a second one raised while cleaning up. + let _ = fs::remove_file(&self.path); + } + } +} + +/// The longest prefix of `name` that fits in `max` bytes, cut on a character +/// boundary so the result is still a string. +fn keep_bytes(name: &str, max: usize) -> &str { + if name.len() <= max { + return name; + } + let mut end = max; + while end > 0 && !name.is_char_boundary(end) { + end -= 1; + } + &name[..end] +} + +/// Re-point a verification failure at the destination the caller asked for. +/// +/// Verification runs on the staged temporary, which is deleted before the error +/// is returned; naming it would send the reader looking for a file that is +/// gone, and for a name they never chose. +fn reported_at(error: CompressionError, output: &Path) -> CompressionError { + match error { + CompressionError::VerificationFailed { reason, .. } => { + CompressionError::VerificationFailed { + archive: output.to_path_buf(), + reason, + } + } + other => other, + } } /// Compress a file using the given algorithm and level (1–5). @@ -53,57 +278,213 @@ pub enum CompressionError { /// The file is stored inside the archive under `arcname`. /// `tar` archives without compressing, so it ignores the level /// (which must still be in range). +/// +/// Nothing appears at `output` until the archive is written *and* checked at +/// `verify`'s depth: the bytes go to a temporary file beside it and are renamed +/// into place at the end. A failure therefore leaves the destination exactly as +/// it was, whether that is an older archive or nothing at all, and leaves no +/// temporary behind either. +/// +/// The backend functions ([`compress_zip`] and friends) write straight to the +/// path they are given and do none of that; this dispatcher is the safe path. pub fn compress( source: &Path, output: &Path, arcname: &str, algorithm: Algorithm, level: u32, + verify: Verify, ) -> Result<(), CompressionError> { if !(1..=5).contains(&level) { return Err(CompressionError::InvalidLevel(level)); } + let staged = StagedOutput::beside(output)?; match algorithm { - Algorithm::SevenZ => compress_7z(source, output, arcname, level), - Algorithm::Tar => compress_tar(source, output, arcname), - Algorithm::Zip => compress_zip(source, output, arcname, level), - } + Algorithm::SevenZ => compress_7z(source, staged.path(), arcname, level), + Algorithm::Tar => compress_tar(source, staged.path(), arcname), + Algorithm::Zip => compress_zip(source, staged.path(), arcname, level), + }?; + // One file in, one entry out, under the name the caller chose. Anything + // else in there, or anything else missing, is not what was asked for. + let expected = [arcname.to_string()]; + verify_archive(staged.path(), algorithm, &expected, verify) + .map_err(|e| reported_at(e, output))?; + staged.commit(output) } /// Compress a whole directory tree into an archive. /// /// Entries keep their paths relative to (and prefixed with) the directory's /// own name, producing a standard archive other tools can read. As with -/// [`compress`], `level` must be 1–5; `tar` ignores it. +/// [`compress`], `level` must be 1–5; `tar` ignores it, the archive is staged +/// beside `output` and renamed in only once it passes at `verify`'s depth, and +/// the backend functions do neither. pub fn compress_dir( source_dir: &Path, output: &Path, algorithm: Algorithm, level: u32, + verify: Verify, ) -> Result<(), CompressionError> { if !(1..=5).contains(&level) { return Err(CompressionError::InvalidLevel(level)); } + // The tree is walked here as well as inside the backend. The backends take + // a path and are called directly from elsewhere in the workspace, so their + // signatures are not ours alone to change, and the dispatcher needs the + // same list to know what the archive was meant to hold. + let expected: Vec = walk_tree(source_dir)? + .into_iter() + .map(|entry| entry.archive_name) + .collect(); + + let staged = StagedOutput::beside(output)?; + match algorithm { + Algorithm::SevenZ => compress_7z_dir(source_dir, staged.path(), level), + Algorithm::Tar => compress_tar_dir(source_dir, staged.path()), + Algorithm::Zip => compress_zip_dir(source_dir, staged.path(), level), + }?; + verify_archive(staged.path(), algorithm, &expected, verify) + .map_err(|e| reported_at(e, output))?; + staged.commit(output) +} + +/// How to extract, beyond where to put it. +/// +/// A separate type rather than more arguments on [`extract`], and +/// [`extract_with`] rather than a replacement for it: every existing caller +/// (the CLI, the server, the desktop, this crate's own tests) has no +/// substitutions to offer and should not have to say so, and the next knob +/// extraction grows should not add a third function. +#[derive(Debug, Clone, Default)] +#[non_exhaustive] +pub struct ExtractOptions { + rules: NameRules, + replacements: Substitutions, +} + +impl ExtractOptions { + pub fn new() -> Self { + Self::default() + } + + /// Judge entry names by these rules instead of the host's. Mostly for + /// tests, which is the whole reason the rules are data: this is what lets a + /// Mac extract an archive the way Windows would. + pub fn with_rules(mut self, rules: NameRules) -> Self { + self.rules = rules; + self + } + + /// The caller's answers for the characters the host cannot write. + pub fn with_replacements(mut self, replacements: Substitutions) -> Self { + self.replacements = replacements; + self + } + + pub fn rules(&self) -> NameRules { + self.rules + } + + pub fn replacements(&self) -> &Substitutions { + &self.replacements + } +} + +/// Which algorithm reads this archive, by file extension (not by magic bytes). +fn algorithm_of(archive: &Path) -> Result { + let ext = archive.extension().and_then(|e| e.to_str()).unwrap_or(""); + Algorithm::from_extension(ext) + .ok_or_else(|| CompressionError::Failed(format!("Unknown archive extension: .{ext}"))) +} + +/// The entry names an archive holds, without extracting anything. +fn list_entries(archive: &Path, algorithm: Algorithm) -> Result, CompressionError> { match algorithm { - Algorithm::SevenZ => compress_7z_dir(source_dir, output, level), - Algorithm::Tar => compress_tar_dir(source_dir, output), - Algorithm::Zip => compress_zip_dir(source_dir, output, level), + Algorithm::SevenZ => list_7z_entries(archive), + Algorithm::Tar => list_tar_entries(archive), + Algorithm::Zip => list_zip_entries(archive), } } +/// What an archive holds that this machine cannot write as ordinary files. +/// +/// Reads the listing and nothing else: no entry is decompressed and nothing is +/// created, so a front end can ask this before it asks the user anything. Feed +/// the answers back through [`ExtractOptions::with_replacements`]. +/// +/// An empty report ([`NameReport::is_empty`]) means extraction has no naming +/// question to ask, which on Unix is nearly always the case. +pub fn unwritable_names(archive: &Path) -> Result { + unwritable_names_with(archive, NameRules::host()) +} + +/// [`unwritable_names`] against a chosen set of rules, so a Mac can be asked +/// what a Windows machine would refuse. +pub fn unwritable_names_with( + archive: &Path, + rules: NameRules, +) -> Result { + let algorithm = algorithm_of(archive)?; + let names = list_entries(archive, algorithm)?; + Ok(NameReport::of(&names, rules)) +} + /// Extract an archive into `output_dir`. /// -/// Returns the list of extracted file paths (relative to `output_dir`). +/// Returns the list of extracted file paths (relative to `output_dir`), which +/// are the names **as written**: an entry the host had to be given a different +/// name for is reported under the name that is on disk, never under the +/// archive's, or a front end would list files nobody can find. +/// /// The algorithm is detected from the archive file extension. pub fn extract(archive: &Path, output_dir: &Path) -> Result, CompressionError> { - let ext = archive.extension().and_then(|e| e.to_str()).unwrap_or(""); + extract_with(archive, output_dir, &ExtractOptions::default()) +} - let algorithm = Algorithm::from_extension(ext) - .ok_or_else(|| CompressionError::Failed(format!("Unknown archive extension: .{ext}")))?; +/// [`extract`], with the caller's answers for the entry names this machine +/// cannot write. +/// +/// Naming is settled over the whole listing before the first byte is written, +/// so the two answers nothing can recover from (a character with no +/// replacement, and two entries that would land on one name) leave the output +/// directory as they found it. +pub fn extract_with( + archive: &Path, + output_dir: &Path, + options: &ExtractOptions, +) -> Result, CompressionError> { + let algorithm = algorithm_of(archive)?; + // Before the archive is even opened: an answer that is itself unwritable is + // wrong whether or not any entry needs it. + options.rules().check_replacements(options.replacements())?; + let plan = plan_for(archive, algorithm, options)?; match algorithm { - Algorithm::SevenZ => extract_7z(archive, output_dir), - Algorithm::Tar => extract_tar(archive, output_dir), - Algorithm::Zip => extract_zip(archive, output_dir), + Algorithm::SevenZ => self::sevenz::extract_7z_planned(archive, output_dir, &plan), + Algorithm::Tar => self::tar::extract_tar_planned(archive, output_dir, &plan), + Algorithm::Zip => self::zip::extract_zip_planned(archive, output_dir, &plan), } } + +/// Work out what every entry will be called, from the listing. +/// +/// A listing that cannot be read is deliberately **not** an error here: the +/// extractor is about to open the same archive and fail on it in its own +/// vocabulary, which is the message this layer would otherwise replace with a +/// worse one. Only a readable listing produces a plan. +/// +/// It costs one listing per extraction, paid even when nothing needs renaming, +/// because the only way to know that is to read the names. For zip and 7z that +/// is a header read; for tar it is a second walk over the headers, seeking past +/// each member rather than reading it. +fn plan_for( + archive: &Path, + algorithm: Algorithm, + options: &ExtractOptions, +) -> Result { + let Ok(names) = list_entries(archive, algorithm) else { + return Ok(NamePlan::identity()); + }; + Ok(plan_names(&names, options.rules(), options.replacements())?) +} diff --git a/apps/core/src/compression/algorithm.rs b/apps/core/src/compression/algorithm.rs index 45cb856..d8d9a22 100644 --- a/apps/core/src/compression/algorithm.rs +++ b/apps/core/src/compression/algorithm.rs @@ -34,8 +34,23 @@ impl Algorithm { } /// Try to detect the algorithm from a file extension. + /// + /// Case insensitive, because a file name is not a wire value: Windows and + /// macOS fold case in the filesystem, plenty of tools write `.ZIP`, and a + /// perfectly good archive was being refused as an unknown format for the + /// spelling of its name alone. + /// + /// Deliberately NOT the same rule as [`FromStr`], which parses the + /// `algorithm=` query parameter of `POST /compress` and the CLI's + /// `--format`. Those are wire values with a documented enum, and they stay + /// strict. [`Algorithm::extension`] likewise keeps returning lowercase, + /// since it names the files this toolkit writes. + /// + /// ASCII folding rather than [`str::to_lowercase`]: the three extensions + /// are ASCII, and Unicode case folding has surprises (Turkish dotless i + /// among them) that have no business deciding an archive format. pub fn from_extension(ext: &str) -> Option { - match ext { + match ext.to_ascii_lowercase().as_str() { "7z" => Some(Algorithm::SevenZ), "tar" => Some(Algorithm::Tar), "zip" => Some(Algorithm::Zip), diff --git a/apps/core/src/compression/names.rs b/apps/core/src/compression/names.rs new file mode 100644 index 0000000..551e225 --- /dev/null +++ b/apps/core/src/compression/names.rs @@ -0,0 +1,769 @@ +//! Which entry names the host can write, and what to do with the ones it +//! cannot. +//! +//! Extraction takes names from an archive, which is to say from another +//! machine. Containment ([`sanitize_entry_path`](super::sanitize_entry_path)) +//! asks whether a name would escape the output directory; this module asks the +//! different question of whether the host can hold the name at all. On Windows +//! an ordinary Unix name often cannot be held: `what?.txt` is refused outright, +//! `notes.txt.` is not preserved, `CON` resolves to a device in every +//! directory, and `notes.txt:hidden` is accepted as the `hidden` stream of +//! `notes.txt` rather than as a file, with no error at all. +//! +//! The rules are **data** ([`NameRules`]) rather than `#[cfg]`, and that is the +//! point: [`NameRules::windows`] can be asked for from any machine, so every +//! rule here is exercised on a Mac and on Linux CI as well as on the Windows +//! leg. A rule reachable only under `#[cfg(windows)]` is a rule this repository +//! cannot test, and that habit is what left a data-loss guard broken on Windows +//! for months. +//! +//! Nothing here touches the filesystem: these are string rules, and the reading +//! and writing all happen in [`super`]. +//! +//! The Windows rules follow "Naming Files, Paths, and Namespaces" +//! (learn.microsoft.com/windows/win32/fileio/naming-a-file), read rather than +//! remembered, which is where the two easily-missed details come from: a +//! reserved device name is reserved *with* an extension too (`NUL.tar.gz` is +//! `NUL`), and the superscript digits `¹²³` count as digits in `COM#`/`LPT#`. + +use std::collections::{BTreeMap, HashMap}; +use std::path::{Path, PathBuf}; + +use serde::Serialize; +use thiserror::Error; + +/// Characters Win32 refuses in a file name outright, minus the one separator. +/// +/// `/` is on the documented list as well and is deliberately absent here: it is +/// the separator **the archive formats define** (ZIP APPNOTE 4.4.17.1, and tar +/// by convention), so [`entry_components`] has already split on it and no +/// component reaching a [`NameRules`] can contain one. Reporting it would ask +/// the user to replace something that is not in any name we ever check. +/// +/// `\` is a different case and belongs here. It is *not* an archive separator, +/// so it arrives as an ordinary character inside a component, and Windows +/// genuinely cannot hold it: there it is a path separator, which is precisely +/// why the component cannot carry one. Unix can, and does (see [`UNIX_REJECTED`]), +/// which is the whole reason this is a rule and not a constant. +const WINDOWS_REJECTED: &[char] = &['<', '>', '"', '|', '?', '*', '\\']; + +/// The colon is not refused by Win32, it is *honoured*: `notes.txt:hidden` names +/// the `hidden` alternate data stream of `notes.txt`, so the write succeeds, the +/// bytes go somewhere invisible, and the listing names a file that exists +/// nowhere. That silence is why it is a fault of its own rather than one more +/// rejected character (issue #63). +const WINDOWS_REINTERPRETED: &[char] = &[':']; + +/// "Do not end a file or directory name with a space or a period." +const WINDOWS_TRAILING: &[char] = &['.', ' ']; + +/// A NUL byte cannot cross the libc boundary, so no Unix filesystem can hold +/// one; `std` answers `InvalidInput` before the kernel is ever asked. It is the +/// only character that fails here, which is why this whole feature is a Windows +/// feature in practice. +const UNIX_REJECTED: &[char] = &['\0']; + +const NO_CHARS: &[char] = &[]; + +/// Device names that resolve in every directory, whatever the extension. +const DEVICE_NAMES: &[&str] = &["CON", "PRN", "AUX", "NUL"]; + +/// `COM#` and `LPT#` are devices too, for a single digit. +const DEVICE_PREFIXES: &[&str] = &["COM", "LPT"]; + +/// The digits `COM#`/`LPT#` accept. The superscripts are not decoration: +/// Windows reads the ISO 8859-1 `¹`, `²` and `³` as digits, so `COM¹` is as +/// reserved as `COM1`. +const DEVICE_DIGITS: &[char] = &['1', '2', '3', '4', '5', '6', '7', '8', '9', '¹', '²', '³']; + +/// What a reserved device name gains so it stops being one. +/// +/// Appended to the part before the first dot, so `CON.txt` becomes `CON_.txt` +/// and keeps the extension that tells a person what the file is. +const DEVICE_SUFFIX: char = '_'; + +/// What a filesystem will accept as the name of an ordinary file. +/// +/// Copy this rather than reaching for `#[cfg]`: [`Self::host`] is what +/// extraction uses, and [`Self::windows`] is what makes the Windows behaviour +/// testable from a machine that is not Windows. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct NameRules { + rejected: &'static [char], + reinterpreted: &'static [char], + /// Whether U+0000 to U+001F are refused (Win32 refuses all of them in a + /// file name, and allows them only inside a stream name). + rejects_control_characters: bool, + trailing: &'static [char], + devices: bool, +} + +impl NameRules { + /// The rules of the machine this build runs on. + pub const fn host() -> Self { + #[cfg(windows)] + { + Self::windows() + } + #[cfg(not(windows))] + { + Self::unix() + } + } + + /// What Windows can write, askable from anywhere. + pub const fn windows() -> Self { + Self { + rejected: WINDOWS_REJECTED, + reinterpreted: WINDOWS_REINTERPRETED, + rejects_control_characters: true, + trailing: WINDOWS_TRAILING, + devices: true, + } + } + + /// What Unix can write, which is very nearly everything. + pub const fn unix() -> Self { + Self { + rejected: UNIX_REJECTED, + reinterpreted: NO_CHARS, + rejects_control_characters: false, + trailing: NO_CHARS, + devices: false, + } + } + + /// Characters that need a replacement before a name carrying them can be + /// written, in no particular order. Offered so a front end can explain the + /// rules before it has an archive to complain about. + pub fn offending_characters(&self) -> impl Iterator + '_ { + let rejected = self + .rejected + .iter() + .map(|c| (*c, CharacterFault::Rejected)) + .chain( + self.reinterpreted + .iter() + .map(|c| (*c, CharacterFault::Reinterpreted)), + ); + let controls = self + .rejects_control_characters + .then_some('\u{0}'..='\u{1f}') + .into_iter() + .flatten() + .map(|c| (c, CharacterFault::Rejected)); + rejected.chain(controls) + } + + /// Everything about **one component** of a name that this filesystem cannot + /// hold. An empty answer means the component is writable as it stands. + /// + /// One problem per distinct offending character, in the order they first + /// appear, so a caller can ask about each exactly once. + pub fn problems(&self, component: &str) -> Vec { + let mut problems: Vec = Vec::new(); + for character in component.chars() { + let Some(fault) = self.fault_of(character) else { + continue; + }; + let problem = NameProblem::Character { character, fault }; + if !problems.contains(&problem) { + problems.push(problem); + } + } + let trailing = self.trailing_run(component); + if !trailing.is_empty() { + problems.push(NameProblem::TrailingCharacters { + removed: trailing.to_string(), + }); + } + if let Some(device) = self.reserved_device(component) { + problems.push(NameProblem::ReservedDevice { + device: device.to_string(), + }); + } + problems + } + + /// [`Self::problems`] over every component of an entry name, deduplicated: + /// a `?` in two components is one question, not two. + /// + /// Split on `/` by [`entry_components`], so the answer does not depend on + /// which machine is asking. Empty components, `.` and `..` are skipped: + /// they are containment's business, not this module's, and extraction + /// settles them before it gets here. + pub fn entry_problems(&self, name: &str) -> Vec { + let mut problems: Vec = Vec::new(); + for component in entry_components(name) { + for problem in self.problems(component) { + if !problems.contains(&problem) { + problems.push(problem); + } + } + } + problems + } + + /// True when this filesystem can hold `component` exactly as spelled. + pub fn can_write(&self, component: &str) -> bool { + self.problems(component).is_empty() + } + + /// The name this filesystem would be given for **one component**, applying + /// the caller's replacements and the two adjustments that need no answer. + /// + /// The order matters and is not arbitrary: + /// + /// 1. every offending character is replaced, because a replacement can + /// create or remove either of the problems below (`CO?1` answered with + /// `M` is `COM1`, a device that was not there before); + /// 2. trailing dots and spaces go, which is what the host would silently do + /// to the name anyway; + /// 3. a reserved device name gains [`DEVICE_SUFFIX`]. + pub fn rewrite( + &self, + component: &str, + replacements: &Substitutions, + ) -> Result { + let mut written = String::with_capacity(component.len()); + for character in component.chars() { + if self.fault_of(character).is_none() { + written.push(character); + continue; + } + let replacement = + replacements + .get(character) + .ok_or_else(|| NameError::NoReplacement { + entry: component.to_string(), + character, + })?; + self.check_replacement(character, replacement)?; + written.push_str(replacement); + } + + let trailing = self.trailing_run(&written).len(); + written.truncate(written.len() - trailing); + + // The offset of the first dot, which is where the device name ends. + let device_ends = self.reserved_device(&written).map(str::len); + if let Some(at) = device_ends { + written.insert(at, DEVICE_SUFFIX); + } + + // Everything below is defence in depth against a replacement that turns + // a name into something that is not a name: `??` answered with `.` is + // `..`, which would climb out of the output directory, and an empty + // answer can leave nothing at all. A caller cannot reach the write path + // without coming through here. + // + // `/` is checked structurally because it is the archive separator and so + // is in no ruleset; a component holding one would silently become two. + // A backslash is deliberately **not** checked here any more. It used to + // be, on the premise that a separator could only appear because a + // replacement put it there, and that premise was wrong twice over: + // `check_replacement` already refuses both separators before either is + // pushed, so the test could not fire for its stated reason, and on Unix + // a backslash is an ordinary, legal character, so the only thing it ever + // caught was a name the host could hold perfectly well. Windows cannot, + // and says so through `can_write` below, because the backslash is in + // WINDOWS_REJECTED where it belongs. + let unnameable = written.is_empty() + || written == "." + || written == ".." + || written.contains('/') + || !self.can_write(&written); + if unnameable { + return Err(NameError::Unnameable { + entry: component.to_string(), + component: component.to_string(), + result: written, + }); + } + Ok(written) + } + + /// [`Self::rewrite`] over a whole entry name, rebuilt as a relative path. + /// + /// **Not a traversal guard**: `.`, `..` and empty components are dropped, + /// exactly as `unpack_in` drops a root and as `sanitize_entry_path` reduces + /// a name to what is left. Callers check containment first; all three + /// extractors in this crate do. + pub fn rewrite_entry( + &self, + name: &str, + replacements: &Substitutions, + ) -> Result { + let mut written = PathBuf::new(); + for component in entry_components(name) { + written.push( + self.rewrite(component, replacements) + .map_err(|e| e.in_entry(name))?, + ); + } + Ok(written) + } + + /// Refuse a replacement this filesystem could not write either, before an + /// archive is opened and before anything is on disk. + /// + /// An empty replacement is fine, and means "drop the character". + pub fn check_replacements(&self, replacements: &Substitutions) -> Result<(), NameError> { + for (character, replacement) in replacements.pairs() { + self.check_replacement(character, replacement)?; + } + Ok(()) + } + + fn check_replacement(&self, character: char, replacement: &str) -> Result<(), NameError> { + for candidate in replacement.chars() { + // Checked before the ruleset, because neither ruleset lists the + // separators (see WINDOWS_REJECTED) and this is the one that would + // be a traversal rather than an unreadable name: `?` answered with + // `../` moves the entry to another directory entirely. + if candidate == '/' || candidate == '\\' { + return Err(NameError::SeparatorInReplacement { + character, + replacement: replacement.to_string(), + }); + } + if self.fault_of(candidate).is_some() { + return Err(NameError::UnwritableReplacement { + character, + replacement: replacement.to_string(), + offending: candidate, + }); + } + } + Ok(()) + } + + fn fault_of(&self, character: char) -> Option { + if self.reinterpreted.contains(&character) { + return Some(CharacterFault::Reinterpreted); + } + if self.rejected.contains(&character) { + return Some(CharacterFault::Rejected); + } + if self.rejects_control_characters && character <= '\u{1f}' { + return Some(CharacterFault::Rejected); + } + None + } + + /// The run of characters at the end of `component` that this filesystem + /// would not keep, as a slice of it. + fn trailing_run<'a>(&self, component: &'a str) -> &'a str { + let kept = component.trim_end_matches(|c| self.trailing.contains(&c)); + &component[kept.len()..] + } + + /// The device this component resolves to, if it resolves to one: the part + /// before the **first** dot, since the docs make `NUL.tar.gz` equivalent to + /// `NUL`, and matched case insensitively. + /// + /// Returned as a slice of `component` so a caller knows where the name ends + /// and the extension begins. + fn reserved_device<'a>(&self, component: &'a str) -> Option<&'a str> { + if !self.devices { + return None; + } + let stem = component.split('.').next().unwrap_or(component); + // Win32 drops trailing spaces before it resolves a name, so `CON ` is + // the console as much as `CON` is. + let named = stem.trim_end_matches(' '); + let upper = named.to_ascii_uppercase(); + if DEVICE_NAMES.contains(&upper.as_str()) { + return Some(stem); + } + for prefix in DEVICE_PREFIXES { + let Some(rest) = upper.strip_prefix(prefix) else { + continue; + }; + let mut digits = rest.chars(); + match (digits.next(), digits.next()) { + (Some(digit), None) if DEVICE_DIGITS.contains(&digit) => return Some(stem), + _ => {} + } + } + None + } +} + +impl Default for NameRules { + fn default() -> Self { + Self::host() + } +} + +/// Why a filesystem cannot hold a name, as data a UI can render. +/// +/// The split that matters to a front end is [`Self::replaceable`]: a character +/// is a question for the user, while the other two are adjustments that need no +/// answer and only need explaining. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +#[serde(tag = "kind", rename_all = "camelCase")] +pub enum NameProblem { + /// A character the filesystem will not take, or will take and read as + /// something other than part of the name. + Character { + character: char, + fault: CharacterFault, + }, + /// The name ends in characters the filesystem does not preserve. Removed + /// automatically: they are what the host would have dropped anyway. + TrailingCharacters { removed: String }, + /// The name resolves to a device rather than to a file, in any directory. + /// A `_` is appended to it automatically, before the extension. + ReservedDevice { device: String }, +} + +impl NameProblem { + /// The character a caller has to supply a replacement for, or `None` when + /// the problem is adjusted automatically. + pub fn replaceable(&self) -> Option { + match self { + Self::Character { character, .. } => Some(*character), + Self::TrailingCharacters { .. } | Self::ReservedDevice { .. } => None, + } + } +} + +/// How a filesystem gets a character in a name wrong. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize)] +#[serde(rename_all = "camelCase")] +pub enum CharacterFault { + /// The write fails. Loud, and the easy case. + Rejected, + /// The write succeeds and does something else: on Windows `:` opens an + /// alternate data stream, so the bytes attach to another file and the name + /// exists as no file at all. + Reinterpreted, +} + +/// One entry an archive holds that the filesystem cannot write. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct UnwritableEntry { + /// The name exactly as the archive spells it. + pub entry: String, + /// Every reason it cannot be written, deduplicated across its components. + pub problems: Vec, +} + +/// One character to ask the user about, and how much of the archive it holds up. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct OffendingCharacter { + pub character: char, + pub fault: CharacterFault, + /// How many entries carry it. A UI can say "3 files" beside the field. + pub entries: usize, +} + +/// What an archive holds that this filesystem cannot write, gathered from its +/// listing alone. +/// +/// Two views of the same thing, because a UI needs both: [`Self::entries`] to +/// show what is affected, and [`Self::characters`] to put one text field per +/// character on screen rather than one per file. +#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct NameReport { + pub entries: Vec, + pub characters: Vec, +} + +impl NameReport { + /// Judge a listing. Pure: no archive is opened here. + pub fn of>(names: &[S], rules: NameRules) -> Self { + let mut entries = Vec::new(); + let mut characters: Vec = Vec::new(); + for name in names { + let name = name.as_ref(); + let problems = rules.entry_problems(name); + if problems.is_empty() { + continue; + } + for problem in &problems { + let NameProblem::Character { character, fault } = problem else { + continue; + }; + match characters.iter_mut().find(|c| c.character == *character) { + Some(known) => known.entries += 1, + None => characters.push(OffendingCharacter { + character: *character, + fault: *fault, + entries: 1, + }), + } + } + entries.push(UnwritableEntry { + entry: name.to_string(), + problems, + }); + } + Self { + entries, + characters, + } + } + + /// True when every name in the archive can be written as it stands. + pub fn is_empty(&self) -> bool { + self.entries.is_empty() + } +} + +/// The caller's answers: what to write in place of each character the host +/// refuses. +/// +/// A replacement may be empty, which drops the character. It is validated +/// against the same rules ([`NameRules::check_replacements`]), because "replace +/// `?` with `*`" is not an answer. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct Substitutions { + /// Ordered so that a caller who gives two bad answers is told about the + /// same one every run. + by_character: BTreeMap, +} + +impl Substitutions { + pub fn new() -> Self { + Self::default() + } + + pub fn is_empty(&self) -> bool { + self.by_character.is_empty() + } + + /// Answer for one character. A later answer replaces an earlier one. + pub fn set(&mut self, character: char, replacement: impl Into) { + self.by_character.insert(character, replacement.into()); + } + + /// [`Self::set`] for a key that arrived as a string, which is how it + /// crosses a UI boundary (a JSON object has no char keys). Both front ends + /// need this, so it lives here rather than twice. + pub fn set_str(&mut self, key: &str, replacement: impl Into) -> Result<(), NameError> { + let mut characters = key.chars(); + match (characters.next(), characters.next()) { + (Some(character), None) => { + self.set(character, replacement); + Ok(()) + } + _ => Err(NameError::NotOneCharacter { + key: key.to_string(), + }), + } + } + + /// Builder form, for a caller that has its answers to hand. + pub fn with(mut self, character: char, replacement: impl Into) -> Self { + self.set(character, replacement); + self + } + + pub fn get(&self, character: char) -> Option<&str> { + self.by_character.get(&character).map(String::as_str) + } + + pub fn pairs(&self) -> impl Iterator { + self.by_character.iter().map(|(c, r)| (*c, r.as_str())) + } +} + +impl FromIterator<(char, String)> for Substitutions { + fn from_iter>(pairs: T) -> Self { + Self { + by_character: pairs.into_iter().collect(), + } + } +} + +/// A name that cannot be written, or an answer that does not help. +/// +/// Separate from the IO and format errors because every one of these is +/// actionable by the person in front of the screen: each names the entry, and +/// says what would fix it. +#[derive(Debug, Error, PartialEq, Eq)] +pub enum NameError { + #[error( + "the archive entry {entry:?} contains {character:?}, which this system cannot write in a \ + file name, and no replacement for it was given" + )] + NoReplacement { entry: String, character: char }, + + #[error( + "{replacement:?} cannot replace {character:?}: this system cannot write {offending:?} in a \ + file name either" + )] + UnwritableReplacement { + character: char, + replacement: String, + offending: char, + }, + + #[error( + "{replacement:?} cannot replace {character:?}: a replacement may not contain a path \ + separator, which would move the entry to another directory" + )] + SeparatorInReplacement { + character: char, + replacement: String, + }, + + #[error( + "the archive entries {first:?} and {second:?} would both be written as {name:?}; choose a \ + replacement that keeps them apart" + )] + Collision { + first: String, + second: String, + name: String, + }, + + #[error( + "the archive entry {entry:?} cannot be written: {component:?} becomes {result:?}, which is \ + not a name this system can hold" + )] + Unnameable { + entry: String, + component: String, + result: String, + }, + + #[error("{key:?} is not a single character, so there is nothing to replace")] + NotOneCharacter { key: String }, +} + +impl NameError { + /// Re-point an error raised over one component at the whole entry, which is + /// the only name the person reading it has ever seen. + fn in_entry(self, entry: &str) -> Self { + match self { + Self::NoReplacement { character, .. } => Self::NoReplacement { + entry: entry.to_string(), + character, + }, + Self::Unnameable { + component, result, .. + } => Self::Unnameable { + entry: entry.to_string(), + component, + result, + }, + other => other, + } + } +} + +/// The name every entry will be written under, for the entries whose name +/// changes. +/// +/// Built from the whole listing before anything is written, because two of the +/// three answers it can give ("no replacement for this" and "these two entries +/// collide") must stop the extraction while the output directory is still +/// empty, and a collision cannot be seen one entry at a time. +#[derive(Debug, Clone, Default)] +pub(crate) struct NamePlan { + /// Only the entries whose name changed. An entry that is absent is written + /// under the name the extractor derived for it, which is what this would + /// have stored anyway. + rewritten: HashMap, +} + +impl NamePlan { + /// The plan that changes nothing, for [`super::extract_zip`] and the other + /// backends called directly with no options. + pub(crate) fn identity() -> Self { + Self::default() + } + + pub(crate) fn written_as(&self, entry: &str) -> Option<&Path> { + self.rewritten.get(entry).map(PathBuf::as_path) + } +} + +/// Work out what each name in `names` becomes, and refuse the two situations +/// nothing downstream could recover from. +pub(crate) fn plan_names>( + names: &[S], + rules: NameRules, + replacements: &Substitutions, +) -> Result { + let mut rewritten = HashMap::new(); + // planned name -> the first entry that claimed it, and whether that entry's + // name had to change to claim it. + let mut claimed: HashMap = HashMap::new(); + + for name in names { + let name = name.as_ref(); + let planned = rules.rewrite_entry(name, replacements)?; + if planned.as_os_str().is_empty() { + // Nothing normal in it (`.`, a bare root). Containment decides what + // becomes of those, per format, and it is not this pass's business. + continue; + } + let changed = planned != natural_path(name); + + if let Some((first, first_changed)) = claimed.get(&planned) { + // Two entries spelled the same way is an archive that was already + // like that, and extraction has always let the second win. Refusing + // it here would start rejecting archives that have nothing to do + // with this feature; a collision is only ours when a rewrite caused + // it. + if *first != name && (changed || *first_changed) { + return Err(NameError::Collision { + first: (*first).to_string(), + second: name.to_string(), + name: planned.to_string_lossy().into_owned(), + }); + } + } + claimed.insert(planned.clone(), (name, changed)); + if changed { + rewritten.insert(name.to_string(), planned); + } + } + + Ok(NamePlan { rewritten }) +} + +/// The relative path an extractor derives from an entry name with no rules +/// applied: its `Normal` components and nothing else. +fn natural_path(name: &str) -> PathBuf { + entry_components(name).collect() +} + +/// Split an archive entry name into its components, the same way on every host. +/// +/// **The separator is `/`, always.** An archive entry name is not a host path: +/// ZIP mandates the forward slash (APPNOTE 4.4.17.1) and tar has used it since +/// v7, so an entry means the same thing whatever machine reads it, and this +/// module has to agree with that rather than with the local convention. +/// +/// It used to be `Path::new(name).components()`, and that was wrong in both +/// directions at once, because `std::path` is `#[cfg]`-dependent while +/// [`NameRules`] is data: +/// +/// * on Windows, `\` is a separator, so `dir\file.txt` split into two +/// components; on Unix it is an ordinary character, so the same entry was one +/// component that [`NameRules::rewrite`] then refused, and a legal Unix file +/// name became an archive this crate could build but not extract; +/// * on Windows, a leading `a:` parses as a drive prefix, and a prefix is not a +/// `Normal` component, so it was silently *discarded*: `a:b/c.txt` was judged, +/// reported and written as `b/c.txt`. That hole was in the colon handling that +/// issue #63 exists for, on the only platform issue #63 is about. +/// +/// Splitting here instead means `NameRules::windows()` answers the same question +/// on a Mac as on Windows, which is the property the whole design rests on. +/// +/// Empty components, `.` and `..` are skipped, exactly as the `Component` filter +/// skipped everything that was not `Normal`. **This is not a traversal guard**: +/// containment is [`super::sanitize_entry_path`]'s job and it rejects, rather +/// than skips, the same input. +fn entry_components(name: &str) -> impl Iterator { + name.split('/') + .filter(|part| !part.is_empty() && *part != "." && *part != "..") +} diff --git a/apps/core/src/compression/sevenz.rs b/apps/core/src/compression/sevenz.rs index 6a53ada..662e62d 100644 --- a/apps/core/src/compression/sevenz.rs +++ b/apps/core/src/compression/sevenz.rs @@ -1,14 +1,174 @@ use std::fs; +use std::io; use std::path::Path; use sevenz_rust2::lzma::LZMA2Options; use sevenz_rust2::{SevenZArchiveEntry, SevenZMethod, SevenZMethodConfiguration, SevenZWriter}; -use super::CompressionError; +use super::{CompressionError, NamePlan, Verify}; /// API level (1–5) → LZMA2 preset (1–9). const SEVENZ_PRESETS: [u32; 5] = [1, 3, 5, 7, 9]; +/// One sentence for a failed CRC, wherever it is raised. +/// +/// The dependency reports the same condition two ways (a variant of its own for +/// the header, an `io::Error` wrapping that variant for a decoded stream), and +/// a user has no use for the difference. +const CHECKSUM_MISMATCH: &str = "the 7z archive is corrupt: a checksum did not match"; + +/// Translate a `sevenz_rust2::Error` into this crate's error type. +/// +/// Never call `to_string()` on one of those: the dependency implements +/// `Display` as `Debug` (sevenz-rust2 0.13.2, `src/error.rs`), so stringifying +/// it hands the user a struct dump such as +/// `Io(Os { code: 2, kind: NotFound, ... }, "/var/lib/collapse/jobs//a.7z")`. +/// That is unreadable, and the absolute path in it is a disclosure once the +/// server forwards a job's error message to its clients. +/// +/// So the two variants that carry an `io::Error` unwrap to +/// [`CompressionError::Io`], which is exactly what zip and tar already produce +/// for the same failure (they reach it through `std`), and every other variant +/// gets a sentence written here. +/// +/// The match is deliberately exhaustive: when the dependency grows a variant +/// this stops compiling, instead of quietly reintroducing a dump through a +/// catch-all arm. +fn from_sevenz(e: sevenz_rust2::Error) -> CompressionError { + use sevenz_rust2::Error as SevenZError; + + let message = match e { + // The second field is the file name the dependency was working on; + // dropping it is the point. `io::Error`'s own Display already says + // what went wrong ("No such file or directory (os error 2)"), and the + // caller knows which path it asked for. + SevenZError::Io(io, _) | SevenZError::FileOpen(io, _) => return from_sevenz_io(io), + // `Other` is prose, and it is also the variant `extract_7z` builds for + // a rejected entry name, so passing it through unchanged is what keeps + // the traversal message identical across the three formats. + SevenZError::Other(reason) => reason.into_owned(), + SevenZError::BadSignature(_) => { + "not a 7z archive: the file does not start with a 7z signature".to_string() + } + SevenZError::UnsupportedVersion { major, minor } => { + format!("unsupported 7z format version {major}.{minor}") + } + // Raised for the start header and for an entry alike, so the sentence + // must not claim which one. + SevenZError::ChecksumVerificationFailed => CHECKSUM_MISMATCH.to_string(), + SevenZError::NextHeaderCrcMismatch => { + "the 7z archive is corrupt: its header failed the checksum".to_string() + } + // The five "bad terminated" variants each name an internal header + // section, and the byte they carry is the property id the parser did + // not expect. Neither tells a user anything the sentence does not. + SevenZError::BadTerminatedStreamsInfo(_) + | SevenZError::BadTerminatedUnpackInfo + | SevenZError::BadTerminatedPackInfo(_) + | SevenZError::BadTerminatedSubStreamsInfo + | SevenZError::BadTerminatedheader(_) => { + "the 7z archive is corrupt: its header is malformed".to_string() + } + SevenZError::ExternalUnsupported => { + "this 7z archive keeps its file list in an external stream, which is not supported" + .to_string() + } + // The payload is quoted nowhere on purpose: two of the construction + // sites pass a method name, a third passes `format!("{:?}", id)` over + // the raw method id bytes, and there is no way to tell them apart. + SevenZError::UnsupportedCompressionMethod(_) => { + "the 7z archive uses a compression method this build cannot decode".to_string() + } + SevenZError::MaxMemLimited { max_kb, actaul_kb } => format!( + "decoding the 7z archive needs {actaul_kb} KB of memory, over the {max_kb} KB limit" + ), + // Nothing here ever supplies a password, so an encrypted archive is + // simply out of reach; both variants mean the same thing to a user. + SevenZError::PasswordRequired | SevenZError::MaybeBadPassword(_) => { + "the 7z archive is encrypted, and passwords are not supported".to_string() + } + // Same reasoning as UnsupportedCompressionMethod: one construction + // site formats a method id with `{:?}`. + SevenZError::Unsupported(_) => { + "the 7z archive uses a feature this build does not support".to_string() + } + SevenZError::FileNotFound => "the entry was not found in the 7z archive".to_string(), + }; + + CompressionError::Failed(message) +} + +/// Unwrap an `io::Error` the dependency handed back. +/// +/// Almost always it is a real IO failure and belongs in +/// [`CompressionError::Io`] untouched. The exception is the CRC guard on a +/// decoded stream: it reports a mismatch as an `io::Error` *wrapping* +/// `sevenz_rust2::Error::ChecksumVerificationFailed`, and since that type's +/// `Display` is its `Debug`, passing it through prints the bare variant name at +/// the user. It is not an IO problem either, so it gets the same sentence the +/// header-side mismatch already gets. +fn from_sevenz_io(io: std::io::Error) -> CompressionError { + let checksum = io + .get_ref() + .and_then(|inner| inner.downcast_ref::()) + .is_some_and(|inner| matches!(inner, sevenz_rust2::Error::ChecksumVerificationFailed)); + if checksum { + CompressionError::Failed(CHECKSUM_MISMATCH.to_string()) + } else { + CompressionError::Io(io) + } +} + +/// Read a 7z back for [`verify_archive`](super::verify_archive), returning the +/// names it holds. +/// +/// [`Verify::Index`] decodes the archive header (which is itself compressed) +/// and reads no entry. [`Verify::Contents`] decodes every entry into a sink, +/// which is what makes the dependency compare each one against the CRC it +/// stores per file. +pub(crate) fn read_7z_entries( + archive: &Path, + depth: Verify, +) -> Result, CompressionError> { + if depth == Verify::Index { + let listing = sevenz_rust2::Archive::open(archive).map_err(from_sevenz)?; + return Ok(listing.files.iter().map(|f| f.name.clone()).collect()); + } + + let mut reader = sevenz_rust2::SevenZReader::open(archive, sevenz_rust2::Password::empty()) + .map_err(from_sevenz)?; + let mut names = Vec::new(); + reader + .for_each_entries(|entry, stream| { + names.push(entry.name.clone()); + // Reading to the end is what makes the CRC guard fire; the bytes + // themselves are not wanted anywhere. + match io::copy(stream, &mut io::sink()) { + Ok(_) => Ok(true), + // `Other` is the one variant `from_sevenz` passes through + // unchanged, which is how a sentence written here survives the + // mapping and keeps the entry's name attached to it. + Err(e) => Err(sevenz_rust2::Error::other(format!( + "entry {:?} could not be read back: {}", + entry.name, + describe_stream_failure(e) + ))), + } + }) + .map_err(from_sevenz)?; + Ok(names) +} + +/// Say in words why a decoded stream stopped, with no prefix: the caller is +/// building a longer sentence around it. +fn describe_stream_failure(e: std::io::Error) -> String { + match from_sevenz_io(e) { + CompressionError::Failed(message) => message, + CompressionError::Io(io) => io.to_string(), + other => other.to_string(), + } +} + pub fn compress_7z( source: &Path, output: &Path, @@ -19,8 +179,7 @@ pub fn compress_7z( let content = fs::read(source)?; - let mut writer = - SevenZWriter::create(output).map_err(|e| CompressionError::Failed(e.to_string()))?; + let mut writer = SevenZWriter::create(output).map_err(from_sevenz)?; let lzma2_opts = LZMA2Options::with_preset(preset); writer.set_content_methods(vec![ @@ -32,11 +191,11 @@ pub fn compress_7z( writer .push_archive_entry(entry, Some(content.as_slice())) - .map_err(|e| CompressionError::Failed(e.to_string()))?; + .map_err(from_sevenz)?; - writer - .finish() - .map_err(|e| CompressionError::Failed(e.to_string()))?; + // `finish` is the one call here that answers with a plain `io::Error`, so + // `?` alone already lands it in `CompressionError::Io`. + writer.finish()?; Ok(()) } @@ -54,8 +213,7 @@ pub fn compress_7z_dir( let entries = super::walk_tree(source_dir)?; let preset = SEVENZ_PRESETS[(level - 1) as usize]; - let mut writer = - SevenZWriter::create(output).map_err(|e| CompressionError::Failed(e.to_string()))?; + let mut writer = SevenZWriter::create(output).map_err(from_sevenz)?; let lzma2_opts = LZMA2Options::with_preset(preset); writer.set_content_methods(vec![ SevenZMethodConfiguration::new(SevenZMethod::LZMA2).with_options(lzma2_opts.into()) @@ -67,55 +225,108 @@ pub fn compress_7z_dir( if entry.is_dir { writer .push_archive_entry::<&[u8]>(sz_entry, None) - .map_err(|e| CompressionError::Failed(e.to_string()))?; + .map_err(from_sevenz)?; } else { let content = fs::read(&entry.disk_path)?; writer .push_archive_entry(sz_entry, Some(content.as_slice())) - .map_err(|e| CompressionError::Failed(e.to_string()))?; + .map_err(from_sevenz)?; } } - writer - .finish() - .map_err(|e| CompressionError::Failed(e.to_string()))?; + // See `compress_7z`: `finish` fails with a plain `io::Error`. + writer.finish()?; Ok(()) } +/// The names a 7z holds, decoding its header and no entry. +/// +/// The one format where reading a listing to plan names and reading one to +/// verify an archive are the same operation with the same error vocabulary +/// (zip and tar each need their own; see their `list_*` functions), so this is +/// [`read_7z_entries`] at [`Verify::Index`] rather than a second copy of it. +pub(crate) fn list_7z_entries(archive: &Path) -> Result, CompressionError> { + read_7z_entries(archive, Verify::Index) +} + pub fn extract_7z(archive: &Path, output_dir: &Path) -> Result, CompressionError> { + extract_7z_planned(archive, output_dir, &NamePlan::identity()) +} + +/// [`extract_7z`], writing each entry under the name `plan` gives it. +pub(crate) fn extract_7z_planned( + archive: &Path, + output_dir: &Path, + plan: &NamePlan, +) -> Result, CompressionError> { fs::create_dir_all(output_dir)?; let canonical_output = output_dir.canonicalize()?; let file = std::fs::File::open(archive)?; + // A write failure has to come back with the entry that caused it, and the + // callback can only fail with the dependency's own error type, whose + // variants have no room for that. So the real error is set aside here and + // the callback returns a placeholder that never reaches a user: it is + // replaced below, before the dependency's own error is even looked at. + let mut write_failure: Option = None; + // Validate each entry name and write it ourselves, so a malicious name is // rejected *before* any bytes reach disk. (`decompress` writes first and // asks questions later, which lets `..` entries escape output_dir.) let mut extracted = Vec::new(); - sevenz_rust2::decompress_with_extract_fn(file, &canonical_output, |entry, reader, _dest| { - let name = entry.name().to_string(); - let rel = super::sanitize_entry_path(&name).ok_or_else(|| { - sevenz_rust2::Error::other(format!("Path traversal detected in archive entry: {name}")) - })?; - let dest = canonical_output.join(&rel); - - if entry.is_directory() { - fs::create_dir_all(&dest).map_err(sevenz_rust2::Error::io)?; - } else { - if let Some(parent) = dest.parent() { - fs::create_dir_all(parent).map_err(sevenz_rust2::Error::io)?; + let outcome = sevenz_rust2::decompress_with_extract_fn( + file, + &canonical_output, + |entry, reader, _dest| { + let name = entry.name().to_string(); + let rel = super::sanitize_entry_path(&name).ok_or_else(|| { + sevenz_rust2::Error::other(format!( + "Path traversal detected in archive entry: {name}" + )) + })?; + // See `extract_zip_planned`: the plan renames inside the output, + // and `ensure_inside` below is the backstop. + let rel = plan.written_as(&name).map_or(rel, Path::to_path_buf); + let dest = canonical_output.join(&rel); + + // The callback can only fail with sevenz's own error type, so the + // real one is stashed for the caller and a placeholder returned. + let mut stash = |e: CompressionError| { + write_failure = Some(e); + sevenz_rust2::Error::other("the entry could not be written") + }; + + if entry.is_directory() { + fs::create_dir_all(&dest) + .map_err(|e| super::entry_error(&name, &dest, e)) + .and_then(|()| super::ensure_inside(&canonical_output, &dest, &name)) + .map_err(&mut stash)?; + } else { + if let Some(parent) = dest.parent() { + fs::create_dir_all(parent) + .map_err(|e| super::entry_error(&name, &dest, e)) + .and_then(|()| super::ensure_inside(&canonical_output, parent, &name)) + .map_err(&mut stash)?; + } + let mut buf = Vec::new(); + reader + .read_to_end(&mut buf) + .map_err(sevenz_rust2::Error::io)?; + fs::write(&dest, &buf) + .map_err(|e| super::entry_error(&name, &dest, e)) + .map_err(&mut stash)?; + extracted.push(rel.to_string_lossy().to_string()); } - let mut buf = Vec::new(); - reader - .read_to_end(&mut buf) - .map_err(sevenz_rust2::Error::io)?; - fs::write(&dest, &buf).map_err(sevenz_rust2::Error::io)?; - extracted.push(rel.to_string_lossy().to_string()); - } - Ok(true) - }) - .map_err(|e| CompressionError::Failed(e.to_string()))?; + Ok(true) + }, + ); + + if let Some(failure) = write_failure { + return Err(failure); + } + outcome.map_err(from_sevenz)?; Ok(extracted) } diff --git a/apps/core/src/compression/tar.rs b/apps/core/src/compression/tar.rs index 4e4cceb..ff8666e 100644 --- a/apps/core/src/compression/tar.rs +++ b/apps/core/src/compression/tar.rs @@ -1,9 +1,10 @@ use std::fs::{self, File}; +use std::io; use std::path::{Component, Path, PathBuf}; use tar::{Archive, Builder, EntryType}; -use super::CompressionError; +use super::{CompressionError, NamePlan, Verify}; /// tar is an archive container without compression, so there is no level. pub fn compress_tar(source: &Path, output: &Path, arcname: &str) -> Result<(), CompressionError> { @@ -60,7 +61,101 @@ pub fn compress_tar_dir(source_dir: &Path, output: &Path) -> Result<(), Compress Ok(()) } +/// Read a tar back for [`verify_archive`](super::verify_archive), returning the +/// names it holds. +/// +/// Both depths walk every header, and the `tar` crate checks each header's +/// `cksum` field as it goes, so a bent header is caught either way; both also +/// have to move over the data to reach the next header, which is what catches a +/// member cut short. +/// +/// [`Verify::Contents`] additionally reads each entry's bytes into a sink, and +/// it is worth being honest about what that adds: **tar stores no checksum over +/// an entry's data**, only over the 512 byte header, so this confirms the data +/// is all there and readable and cannot confirm it is unchanged. zip and 7z get +/// a real per-entry CRC here; tar cannot, from any reader. +pub(crate) fn read_tar_entries( + archive: &Path, + depth: Verify, +) -> Result, CompressionError> { + let file = File::open(archive)?; + let mut ar = Archive::new(file); + let entries = ar.entries().map_err(|e| { + CompressionError::Failed(format!("the archive could not be read back: {e}")) + })?; + + let mut names = Vec::new(); + for entry in entries { + let mut entry = entry.map_err(|e| { + CompressionError::Failed(format!("the archive could not be read back: {e}")) + })?; + let name = entry + .path() + .map_err(|e| CompressionError::Failed(format!("an entry has an unreadable name: {e}")))? + .to_string_lossy() + .to_string(); + if depth == Verify::Contents { + io::copy(&mut entry, &mut io::sink()).map_err(|e| { + CompressionError::Failed(format!("entry {name:?} could not be read back: {e}")) + })?; + } + names.push(name); + } + Ok(names) +} + +/// The names a tar holds that extraction would write, walking the headers and +/// skipping the data. +/// +/// Filtered the same way [`extract_tar`] filters, so a caller asking what an +/// archive contains is never told about an entry that would be skipped. It +/// seeks past each member rather than reading it, which is what keeps a listing +/// cheap on a large archive; [`read_tar_entries`] deliberately does not, since +/// reading every byte is half of what verification is for. +pub(crate) fn list_tar_entries(archive: &Path) -> Result, CompressionError> { + let file = File::open(archive)?; + let mut ar = Archive::new(file); + let entries = ar + .entries_with_seek() + .map_err(|e| CompressionError::Failed(e.to_string()))?; + + let mut names = Vec::new(); + for entry in entries { + let entry = entry.map_err(|e| CompressionError::Failed(e.to_string()))?; + let entry_type = entry.header().entry_type(); + if entry_type != EntryType::Regular && entry_type != EntryType::Directory { + continue; + } + names.push( + entry + .path() + .map_err(|e| CompressionError::Failed(e.to_string()))? + .to_string_lossy() + .to_string(), + ); + } + Ok(names) +} + pub fn extract_tar(archive: &Path, output_dir: &Path) -> Result, CompressionError> { + extract_tar_planned(archive, output_dir, &NamePlan::identity()) +} + +/// [`extract_tar`], writing each entry under the name `plan` gives it. +/// +/// Two write paths, and the split is deliberate. `unpack_in` derives the +/// destination from the entry's own name, so it cannot write a renamed entry at +/// all; but it is also the traversal guard tar has always used, and the +/// canonicalizing containment check inside it is what stops a write from +/// following a symlink that was already sitting in the output directory. So an +/// entry whose name is unchanged still goes through it, exactly as before, and +/// only a renamed one is unpacked to an explicit destination, with the same +/// containment check made here. +pub(crate) fn extract_tar_planned( + archive: &Path, + output_dir: &Path, + plan: &NamePlan, +) -> Result, CompressionError> { fs::create_dir_all(output_dir)?; let canonical_output = output_dir.canonicalize()?; @@ -81,35 +176,81 @@ pub fn extract_tar(archive: &Path, output_dir: &Path) -> Result, Com // Only ever materialize regular files and directories. Symlinks, // hardlinks and special nodes are skipped so extraction never plants - // an outbound link in the output tree — the same "no links" guarantee - // zip/7z give (they write link entries as regular files). + // an outbound link in the output tree, the same "no links" guarantee + // zip/7z give (they write link entries as regular files). Checked + // before anything else about the name, so an entry that is not going to + // be written is not judged either. let entry_type = entry.header().entry_type(); if entry_type != EntryType::Regular && entry_type != EntryType::Directory { continue; } - // unpack_in refuses entries whose path would escape the output dir. - let unpacked = entry - .unpack_in(&canonical_output) - .map_err(|e| CompressionError::Failed(e.to_string()))?; - if !unpacked { - return Err(CompressionError::Failed(format!( - "Path traversal detected in archive entry: {name}" - ))); + // The path unpack_in would write to: its `Normal` components, with a + // root or a drive stripped. `..` is refused here rather than left to + // unpack_in's `Ok(false)`, because the renamed branch below never calls + // unpack_in and would otherwise have no guard at all. + let natural = normal_path(&name).ok_or_else(|| { + CompressionError::Failed(format!("Path traversal detected in archive entry: {name}")) + })?; + if natural.as_os_str().is_empty() { + // Nothing but `.` or a root: unpack_in treats it as an empty name + // and writes nothing, and neither do we. + continue; } - if entry_type == EntryType::Regular { - // The traversal guard is unpack_in itself (it refuses `..` and - // strips root/cur-dir before writing). Report the path it actually - // wrote by keeping only the normal components, relative to output_dir. - let written: PathBuf = Path::new(&name) - .components() - .filter(|c| matches!(c, Component::Normal(_))) - .collect(); - if !written.as_os_str().is_empty() { - extracted.push(written.to_string_lossy().to_string()); + match plan.written_as(&name) { + None => { + let unpacked = entry + .unpack_in(&canonical_output) + .map_err(|e| CompressionError::Failed(e.to_string()))?; + if !unpacked { + return Err(CompressionError::Failed(format!( + "Path traversal detected in archive entry: {name}" + ))); + } + if entry_type == EntryType::Regular { + extracted.push(natural.to_string_lossy().to_string()); + } + } + Some(rel) => { + let dest = canonical_output.join(rel); + if let Some(parent) = dest.parent() { + fs::create_dir_all(parent).map_err(|e| super::entry_error(&name, &dest, e))?; + // What unpack_in's `validate_inside_dst` does: resolve the + // directory being written into and refuse one that turned + // out to be somewhere else. + let resolved = parent + .canonicalize() + .map_err(|e| super::entry_error(&name, &dest, e))?; + if !resolved.starts_with(&canonical_output) { + return Err(CompressionError::Failed(format!( + "Path traversal detected in archive entry: {name}" + ))); + } + } + entry + .unpack(&dest) + .map_err(|e| super::entry_error(&name, &dest, e))?; + if entry_type == EntryType::Regular { + extracted.push(rel.to_string_lossy().to_string()); + } } } } Ok(extracted) } + +/// An entry's path reduced to the components tar would write: `Normal` ones +/// only, a root or drive prefix dropped, `.` dropped, and `None` for any `..`, +/// which is the traversal `unpack_in` refuses. +fn normal_path(name: &str) -> Option { + let mut safe = PathBuf::new(); + for component in Path::new(name).components() { + match component { + Component::Normal(part) => safe.push(part), + Component::ParentDir => return None, + Component::CurDir | Component::RootDir | Component::Prefix(_) => {} + } + } + Some(safe) +} diff --git a/apps/core/src/compression/verify.rs b/apps/core/src/compression/verify.rs new file mode 100644 index 0000000..c12015c --- /dev/null +++ b/apps/core/src/compression/verify.rs @@ -0,0 +1,150 @@ +//! Reading an archive back to check it says what it was meant to say. +//! +//! Compressors here finalise on drop, so a run that dies partway through still +//! closes out a *structurally valid* archive: zip writes its central directory, +//! tar writes its end-of-archive blocks, and what is left opens cleanly while +//! silently missing whatever had not been written yet. Nothing about the +//! archive itself says so, which is why the check has to compare it against +//! what the caller asked for rather than merely asking whether it parses. + +use std::collections::BTreeSet; +use std::path::Path; + +use super::{Algorithm, CompressionError}; + +/// How thoroughly to check an archive once it is written. +/// +/// [`compress`](super::compress) and [`compress_dir`](super::compress_dir) both +/// check before the archive reaches the destination, so a failure leaves +/// nothing there rather than an archive that looks fine and is not. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Verify { + /// Read the archive's own listing back and confirm it names exactly the + /// entries that were meant to go in. Nothing is decompressed: for zip and + /// 7z this reads a header, for tar it walks the headers skipping the data. + /// + /// This is the depth that catches the failure this exists for, a + /// compression that stopped early and finalised anyway. + Index, + /// The listing, and then every entry decompressed into a sink. Roughly + /// doubles the work, which is why it is the caller's choice. + /// + /// What that buys differs by format, and the difference is worth stating + /// exactly rather than implying all three get the same thing: + /// + /// - **zip** stores a CRC32 per entry, and the `zip` crate compares it as + /// the entry is read to its end. A flipped bit in the data is caught. + /// - **7z** stores a CRC per file (and per pack stream), verified the same + /// way while the entry is decoded. A flipped bit in the data is caught. + /// - **tar** stores *no* checksum over an entry's data at all: its `cksum` + /// header field covers the 512 byte header and nothing else. So here this + /// reads every entry through and confirms the archive is well formed and + /// complete (headers intact, no member cut short, the listing as + /// expected), and that is the whole of it. A flipped bit inside a tar + /// member's data is not detectable from the archive, by this or by any + /// other reader. + /// + /// Nothing is ever written to disk, so this needs no space of its own. + Contents, +} + +/// At most this many entry names are spelled out before a message says "and N +/// more": a tree with ten thousand files must not produce a ten thousand name +/// error. +const NAMES_SHOWN: usize = 5; + +/// Check that `archive` holds exactly the entries in `expected`, at `depth`. +/// +/// `algorithm` says how to read it rather than the file extension, because the +/// dispatchers verify a temporary file whose name is not an archive name. +/// +/// Entry names are compared as sets and with any trailing `/` removed: a +/// directory is spelled `photos/` by zip and tar and `photos` by 7z, and the +/// order entries come back in is the writer's business, not the caller's. +pub fn verify_archive( + archive: &Path, + algorithm: Algorithm, + expected: &[String], + depth: Verify, +) -> Result<(), CompressionError> { + let found_names = match algorithm { + Algorithm::SevenZ => super::read_7z_entries(archive, depth), + Algorithm::Tar => super::read_tar_entries(archive, depth), + Algorithm::Zip => super::read_zip_entries(archive, depth), + } + .map_err(|e| failed(archive, reason_of(e)))?; + + let expected: BTreeSet<&str> = expected.iter().map(|n| normalize(n)).collect(); + let found: BTreeSet<&str> = found_names.iter().map(|n| normalize(n)).collect(); + + let missing: Vec<&str> = expected.difference(&found).copied().collect(); + let unexpected: Vec<&str> = found.difference(&expected).copied().collect(); + if missing.is_empty() && unexpected.is_empty() { + return Ok(()); + } + + // Both halves are reported, and by name. "the archive has 3 entries, not 4" + // tells whoever reads the log nothing about which file went missing. + let mut problems = Vec::new(); + if !missing.is_empty() { + problems.push(format!( + "{} missing: {}", + counted(&missing), + listed(&missing) + )); + } + if !unexpected.is_empty() { + problems.push(format!( + "{} unexpected: {}", + counted(&unexpected), + listed(&unexpected) + )); + } + Err(failed(archive, problems.join("; "))) +} + +/// Directory entries carry a trailing separator in zip and tar and none in 7z, +/// so it cannot be part of an entry's identity here. +fn normalize(name: &str) -> &str { + name.trim_end_matches('/') +} + +fn failed(archive: &Path, reason: String) -> CompressionError { + CompressionError::VerificationFailed { + archive: archive.to_path_buf(), + reason, + } +} + +/// Render an error raised while reading the archive back as the `reason` half +/// of a verification failure. +/// +/// The variant's own prefix ("Compression failed: ", "IO error: ") is dropped +/// because it would read as if compressing had failed, which is precisely the +/// distinction [`CompressionError::VerificationFailed`] exists to draw. +fn reason_of(error: CompressionError) -> String { + match error { + CompressionError::Io(io) => io.to_string(), + CompressionError::Failed(message) => message, + other => other.to_string(), + } +} + +fn counted(names: &[&str]) -> String { + match names.len() { + 1 => "1 entry is".to_string(), + n => format!("{n} entries are"), + } +} + +fn listed(names: &[&str]) -> String { + let shown: Vec = names + .iter() + .take(NAMES_SHOWN) + .map(|n| format!("{n:?}")) + .collect(); + match names.len().checked_sub(NAMES_SHOWN) { + Some(rest) if rest > 0 => format!("{} and {rest} more", shown.join(", ")), + _ => shown.join(", "), + } +} diff --git a/apps/core/src/compression/zip.rs b/apps/core/src/compression/zip.rs index e76369d..a13283f 100644 --- a/apps/core/src/compression/zip.rs +++ b/apps/core/src/compression/zip.rs @@ -1,11 +1,11 @@ use std::fs::{self, File}; -use std::io::{Read, Write}; +use std::io::{self, Read, Write}; use std::path::Path; use zip::write::SimpleFileOptions; use zip::{CompressionMethod, ZipWriter}; -use super::CompressionError; +use super::{CompressionError, NamePlan, Verify}; /// API level (1–5) → Deflate compresslevel (1–9). const ZIP_LEVELS: [i64; 5] = [1, 3, 5, 7, 9]; @@ -18,6 +18,13 @@ pub fn compress_zip( ) -> Result<(), CompressionError> { let compress_level = ZIP_LEVELS[(level - 1) as usize]; + // Read the source before creating the output, the way `compress_tar` and + // `compress_7z` already do: the other order left a zero-byte `.zip` behind + // whenever the source could not be opened. + let mut source_file = File::open(source)?; + let mut buffer = Vec::new(); + source_file.read_to_end(&mut buffer)?; + let output_file = File::create(output)?; let mut writer = ZipWriter::new(output_file); @@ -28,10 +35,6 @@ pub fn compress_zip( writer .start_file(arcname, options) .map_err(|e| CompressionError::Failed(e.to_string()))?; - - let mut source_file = File::open(source)?; - let mut buffer = Vec::new(); - source_file.read_to_end(&mut buffer)?; writer.write_all(&buffer)?; writer @@ -68,10 +71,16 @@ pub fn compress_zip_dir( .add_directory(&entry.archive_name, dir_options) .map_err(|e| CompressionError::Failed(e.to_string()))?; } else { + // Read the member before naming it in the archive. The other order + // put the name in first, so a member that could not be read still + // appeared in the archive with nothing behind it, and the CRC + // written for it was the CRC of nothing: an archive no reader could + // fault, holding an empty file where a real one belonged. Whole + // files are buffered here either way, so this costs nothing. + let bytes = fs::read(&entry.disk_path)?; writer .start_file(&entry.archive_name, file_options) .map_err(|e| CompressionError::Failed(e.to_string()))?; - let bytes = fs::read(&entry.disk_path)?; writer.write_all(&bytes)?; } } @@ -83,7 +92,71 @@ pub fn compress_zip_dir( Ok(()) } +/// Read a ZIP back for [`verify_archive`](super::verify_archive), returning the +/// names it holds. +/// +/// At [`Verify::Index`] this reads the central directory and stops there. At +/// [`Verify::Contents`] every file entry is decompressed into a sink, which is +/// what makes the `zip` crate compare it against the CRC32 stored beside it: +/// the check fires on the read that reports end of file, so an entry has to be +/// read all the way through for it to happen at all. +pub(crate) fn read_zip_entries( + archive: &Path, + depth: Verify, +) -> Result, CompressionError> { + let file = File::open(archive)?; + let mut zip = zip::ZipArchive::new(file).map_err(|e| { + CompressionError::Failed(format!("the archive could not be read back: {e}")) + })?; + + if depth == Verify::Index { + return Ok(zip.file_names().map(|name| name.to_string()).collect()); + } + + let mut names = Vec::with_capacity(zip.len()); + for i in 0..zip.len() { + let mut entry = zip.by_index(i).map_err(|e| { + CompressionError::Failed(format!("entry {i} could not be read back: {e}")) + })?; + let name = entry.name().to_string(); + if !entry.is_dir() { + io::copy(&mut entry, &mut io::sink()).map_err(|e| { + CompressionError::Failed(format!("entry {name:?} could not be read back: {e}")) + })?; + } + names.push(name); + } + Ok(names) +} + +/// The names a ZIP holds, in the order its entries are stored, reading the +/// central directory and nothing else. +/// +/// Kept apart from [`read_zip_entries`] because the two answer different +/// questions and their failures read differently: that one is verifying an +/// archive this process just wrote, this one is looking at a file someone +/// handed us, so its error is the one extraction would give. +pub(crate) fn list_zip_entries(archive: &Path) -> Result, CompressionError> { + let file = File::open(archive)?; + let zip = zip::ZipArchive::new(file).map_err(|e| CompressionError::Failed(e.to_string()))?; + Ok((0..zip.len()) + .filter_map(|i| zip.name_for_index(i).map(str::to_string)) + .collect()) +} + pub fn extract_zip(archive: &Path, output_dir: &Path) -> Result, CompressionError> { + extract_zip_planned(archive, output_dir, &NamePlan::identity()) +} + +/// [`extract_zip`], writing each entry under the name `plan` gives it. +/// +/// An entry the plan says nothing about keeps the name the archive spells, +/// which is what makes the plain [`extract_zip`] the same function. +pub(crate) fn extract_zip_planned( + archive: &Path, + output_dir: &Path, + plan: &NamePlan, +) -> Result, CompressionError> { let file = File::open(archive)?; let mut zip = zip::ZipArchive::new(file).map_err(|e| CompressionError::Failed(e.to_string()))?; @@ -108,17 +181,25 @@ pub fn extract_zip(archive: &Path, output_dir: &Path) -> Result, Com let rel = super::sanitize_entry_path(&name).ok_or_else(|| { CompressionError::Failed(format!("Path traversal detected in archive entry: {name}")) })?; + // The plan is built from the same name, one component at a time, so it + // can only rename inside the output directory. `ensure_inside` below is + // the backstop for the cases a lexical rule cannot reach: a caller that + // judged the name under another host's rules, and a symlink already + // sitting in the output. + let rel = plan.written_as(&name).map_or(rel, Path::to_path_buf); let dest = canonical_output.join(&rel); if entry.is_dir() { - fs::create_dir_all(&dest)?; + fs::create_dir_all(&dest).map_err(|e| super::entry_error(&name, &dest, e))?; + super::ensure_inside(&canonical_output, &dest, &name)?; } else { if let Some(parent) = dest.parent() { - fs::create_dir_all(parent)?; + fs::create_dir_all(parent).map_err(|e| super::entry_error(&name, &dest, e))?; + super::ensure_inside(&canonical_output, parent, &name)?; } let mut buf = Vec::new(); entry.read_to_end(&mut buf)?; - fs::write(&dest, &buf)?; + fs::write(&dest, &buf).map_err(|e| super::entry_error(&name, &dest, e))?; extracted.push(rel.to_string_lossy().to_string()); } } diff --git a/apps/core/src/lib.rs b/apps/core/src/lib.rs index 2688e1d..cb8f713 100644 --- a/apps/core/src/lib.rs +++ b/apps/core/src/lib.rs @@ -4,4 +4,8 @@ pub mod compression; // what let them drift apart, and the drift cost the CLI its data-loss guard. pub mod paths; -pub use compression::{compress, compress_dir, extract, Algorithm, CompressionError}; +pub use compression::{ + compress, compress_dir, extract, extract_with, unwritable_names, unwritable_names_with, + Algorithm, CharacterFault, CompressionError, ExtractOptions, NameError, NameProblem, + NameReport, NameRules, OffendingCharacter, Substitutions, UnwritableEntry, Verify, +}; diff --git a/apps/core/tests/compression.rs b/apps/core/tests/compression.rs index cc8b23d..532493c 100644 --- a/apps/core/tests/compression.rs +++ b/apps/core/tests/compression.rs @@ -3,7 +3,7 @@ use std::path::Path; -use collapse_core::{compress, compress_dir, extract, Algorithm, CompressionError}; +use collapse_core::{compress, compress_dir, extract, Algorithm, CompressionError, Verify}; /// Normalize and sort an extracted listing so the expectations read the same /// on a platform whose path separator is not `/`. @@ -72,13 +72,27 @@ fn algorithm_serde_roundtrip() { #[test] fn compress_invalid_level_zero() { - let result = compress(Path::new("/x"), Path::new("/y"), "f", Algorithm::Zip, 0); + let result = compress( + Path::new("/x"), + Path::new("/y"), + "f", + Algorithm::Zip, + 0, + Verify::Index, + ); assert!(matches!(result, Err(CompressionError::InvalidLevel(0)))); } #[test] fn compress_invalid_level_six() { - let result = compress(Path::new("/x"), Path::new("/y"), "f", Algorithm::Zip, 6); + let result = compress( + Path::new("/x"), + Path::new("/y"), + "f", + Algorithm::Zip, + 6, + Verify::Index, + ); assert!(matches!(result, Err(CompressionError::InvalidLevel(6)))); } @@ -106,8 +120,98 @@ fn from_extension_unknown() { assert_eq!(Algorithm::from_extension(""), None); } +/// An extension is a file name, not a wire value. Windows and macOS fold case +/// in the filesystem and plenty of tools write `.ZIP`, so a valid archive was +/// being refused as an unknown format for the spelling of its name alone. +#[test] +fn from_extension_is_case_insensitive() { + for (spelling, expected) in [ + ("ZIP", Algorithm::Zip), + ("Zip", Algorithm::Zip), + ("zIp", Algorithm::Zip), + ("7Z", Algorithm::SevenZ), + ("TAR", Algorithm::Tar), + ("Tar", Algorithm::Tar), + ] { + assert_eq!( + Algorithm::from_extension(spelling), + Some(expected), + "{spelling} names the same format as its lowercase spelling" + ); + } +} + +/// Lenient about spelling, not about formats: a case-insensitive match must not +/// become a match that accepts anything. +#[test] +fn from_extension_still_refuses_a_format_it_does_not_have() { + for unknown in ["RAR", "Gz", "TAR.GZ", "ZIPX", " zip", "zip "] { + assert_eq!( + Algorithm::from_extension(unknown), + None, + "{unknown:?} is not one of the three" + ); + } +} + +/// The two parsers are deliberately different rules, and this is the one that +/// must NOT follow: `FromStr` reads the `algorithm=` query parameter of +/// `POST /compress` and the CLI's `--format`, both wire values with a +/// documented enum. Loosening it here would silently widen the API. +#[test] +fn from_str_stays_strict_about_case() { + for shouted in ["ZIP", "Zip", "7Z", "TAR"] { + assert!( + shouted.parse::().is_err(), + "{shouted} is not a wire value, whatever from_extension says about file names" + ); + } +} + +/// What names the files this toolkit writes stays lowercase, so making the +/// reader case insensitive cannot start producing `photos.ZIP`. +#[test] +fn extension_is_always_written_lowercase() { + for algorithm in [Algorithm::Zip, Algorithm::SevenZ, Algorithm::Tar] { + let ext = algorithm.extension(); + assert_eq!( + ext, + ext.to_ascii_lowercase(), + "{algorithm} names output files" + ); + } +} + // -- extract dispatcher tests -- +/// The dispatcher end to end, not just the parser: a real archive written by +/// this toolkit, renamed the way a user or another tool would, still opens. +/// The parser test above would keep passing if `extract` stopped calling it. +#[test] +fn extract_opens_an_archive_whatever_the_case_of_its_name() { + for (algorithm, shouted) in [ + (Algorithm::Zip, "OUT.ZIP"), + (Algorithm::SevenZ, "Out.7Z"), + (Algorithm::Tar, "OUT.Tar"), + ] { + let dir = tempfile::TempDir::new().unwrap(); + let src = dir.path().join("input.txt"); + std::fs::write(&src, b"shouted name").unwrap(); + + let archive = dir.path().join(shouted); + compress(&src, &archive, "input.txt", algorithm, 1, Verify::Index).unwrap(); + + let out = dir.path().join("extracted"); + let files = extract(&archive, &out).expect("the name is understood"); + assert_eq!(listing(files), vec!["input.txt"], "{shouted}"); + assert_eq!( + std::fs::read(out.join("input.txt")).unwrap(), + b"shouted name", + "{shouted}" + ); + } +} + #[test] fn extract_dispatches_zip() { let dir = tempfile::TempDir::new().unwrap(); @@ -115,7 +219,15 @@ fn extract_dispatches_zip() { std::fs::write(&src, b"dispatch zip").unwrap(); let archive = dir.path().join("out.zip"); - compress(&src, &archive, "input.txt", Algorithm::Zip, 1).unwrap(); + compress( + &src, + &archive, + "input.txt", + Algorithm::Zip, + 1, + Verify::Index, + ) + .unwrap(); let out = dir.path().join("extracted"); let files = extract(&archive, &out).unwrap(); @@ -133,7 +245,15 @@ fn extract_dispatches_7z() { std::fs::write(&src, b"dispatch 7z").unwrap(); let archive = dir.path().join("out.7z"); - compress(&src, &archive, "input.txt", Algorithm::SevenZ, 1).unwrap(); + compress( + &src, + &archive, + "input.txt", + Algorithm::SevenZ, + 1, + Verify::Index, + ) + .unwrap(); let out = dir.path().join("extracted"); let files = extract(&archive, &out).unwrap(); @@ -151,7 +271,15 @@ fn extract_dispatches_tar() { std::fs::write(&src, b"dispatch tar").unwrap(); let archive = dir.path().join("out.tar"); - compress(&src, &archive, "input.txt", Algorithm::Tar, 1).unwrap(); + compress( + &src, + &archive, + "input.txt", + Algorithm::Tar, + 1, + Verify::Index, + ) + .unwrap(); let out = dir.path().join("extracted"); let files = extract(&archive, &out).unwrap(); @@ -170,12 +298,28 @@ fn tar_level_is_ignored() { // All valid levels must produce byte-identical tar archives. let reference = dir.path().join("out_l1.tar"); - compress(&src, &reference, "input.txt", Algorithm::Tar, 1).unwrap(); + compress( + &src, + &reference, + "input.txt", + Algorithm::Tar, + 1, + Verify::Index, + ) + .unwrap(); let reference_bytes = std::fs::read(&reference).unwrap(); for level in 2..=5 { let archive = dir.path().join(format!("out_l{level}.tar")); - compress(&src, &archive, "input.txt", Algorithm::Tar, level).unwrap(); + compress( + &src, + &archive, + "input.txt", + Algorithm::Tar, + level, + Verify::Index, + ) + .unwrap(); assert_eq!( std::fs::read(&archive).unwrap(), reference_bytes, @@ -186,9 +330,23 @@ fn tar_level_is_ignored() { #[test] fn tar_out_of_range_level_is_still_rejected() { - let result = compress(Path::new("/x"), Path::new("/y"), "f", Algorithm::Tar, 0); + let result = compress( + Path::new("/x"), + Path::new("/y"), + "f", + Algorithm::Tar, + 0, + Verify::Index, + ); assert!(matches!(result, Err(CompressionError::InvalidLevel(0)))); - let result = compress(Path::new("/x"), Path::new("/y"), "f", Algorithm::Tar, 6); + let result = compress( + Path::new("/x"), + Path::new("/y"), + "f", + Algorithm::Tar, + 6, + Verify::Index, + ); assert!(matches!(result, Err(CompressionError::InvalidLevel(6)))); } @@ -213,6 +371,7 @@ fn compress_nonexistent_source_errors() { "ghost.txt", Algorithm::Zip, 1, + Verify::Index, ); assert!(result.is_err()); } @@ -247,7 +406,7 @@ fn compress_dir_dispatches_tar() { std::fs::write(root.join("a.txt"), b"alpha").unwrap(); let archive = dir.path().join("data.tar"); - compress_dir(&root, &archive, Algorithm::Tar, 1).unwrap(); + compress_dir(&root, &archive, Algorithm::Tar, 1, Verify::Index).unwrap(); let out = dir.path().join("out"); let files = extract(&archive, &out).unwrap(); @@ -263,7 +422,7 @@ fn compress_dir_dispatches_zip() { std::fs::write(root.join("a.txt"), b"alpha").unwrap(); let archive = dir.path().join("data.zip"); - compress_dir(&root, &archive, Algorithm::Zip, 3).unwrap(); + compress_dir(&root, &archive, Algorithm::Zip, 3, Verify::Index).unwrap(); let out = dir.path().join("out"); let files = extract(&archive, &out).unwrap(); @@ -279,7 +438,7 @@ fn compress_dir_dispatches_7z() { std::fs::write(root.join("a.txt"), b"alpha").unwrap(); let archive = dir.path().join("data.7z"); - compress_dir(&root, &archive, Algorithm::SevenZ, 3).unwrap(); + compress_dir(&root, &archive, Algorithm::SevenZ, 3, Verify::Index).unwrap(); let out = dir.path().join("out"); let files = extract(&archive, &out).unwrap(); @@ -295,11 +454,11 @@ fn compress_dir_invalid_level_is_rejected() { let archive = dir.path().join("data.tar"); assert!(matches!( - compress_dir(&root, &archive, Algorithm::Tar, 0), + compress_dir(&root, &archive, Algorithm::Tar, 0, Verify::Index), Err(CompressionError::InvalidLevel(0)) )); assert!(matches!( - compress_dir(&root, &archive, Algorithm::Tar, 6), + compress_dir(&root, &archive, Algorithm::Tar, 6, Verify::Index), Err(CompressionError::InvalidLevel(6)) )); } diff --git a/apps/core/tests/names.rs b/apps/core/tests/names.rs new file mode 100644 index 0000000..7bfe740 --- /dev/null +++ b/apps/core/tests/names.rs @@ -0,0 +1,975 @@ +//! Entry names the host cannot write: the rules, the report a front end asks +//! for, and extraction with the answers a user gave (issues #63 and #64). +//! +//! Covers `src/compression/names.rs` plus the two dispatchers that exist for +//! it, `unwritable_names` and `extract_with`. +//! +//! **Almost every test here runs the Windows rules on whatever machine is +//! running the suite.** That is the point of `NameRules::windows()`: nobody +//! working on this repository has Windows, and the CI leg that does runs on the +//! release path only, so a rule reachable only under `#[cfg(windows)]` is a rule +//! nobody would find out was broken. The handful of tests that must know where +//! they are say so in their name. + +use std::fs; +use std::io::Write; +use std::path::{Path, PathBuf}; + +use collapse_core::compression::{ + extract_7z, extract_tar, extract_zip, CharacterFault, NameError, NameProblem, NameReport, + NameRules, Substitutions, +}; +use collapse_core::{extract, extract_with, unwritable_names_with, ExtractOptions}; +use sevenz_rust2::{SevenZArchiveEntry, SevenZWriter}; +use tar::{Builder, EntryType, Header}; +use zip::write::SimpleFileOptions; +use zip::{CompressionMethod, ZipWriter}; + +// ---------------------------------------------------------------- fixtures -- + +const FORMATS: [&str; 3] = ["zip", "7z", "tar"]; + +/// Build an archive of `format` whose entries are named exactly as given. +/// +/// The writers are driven at a low enough level that a name Windows hates +/// survives into the file: that is the whole fixture. tar goes through a raw +/// header for the same reason `security.rs` does, since `Builder::append_data` +/// has opinions of its own about names. +fn archive_with(dir: &Path, format: &str, entries: &[(&str, &[u8])]) -> PathBuf { + let archive = dir.join(format!("input.{format}")); + match format { + "zip" => { + let file = fs::File::create(&archive).unwrap(); + let mut writer = ZipWriter::new(file); + let options = + SimpleFileOptions::default().compression_method(CompressionMethod::Stored); + for (name, content) in entries { + writer.start_file(*name, options).unwrap(); + writer.write_all(content).unwrap(); + } + writer.finish().unwrap(); + } + "7z" => { + let mut writer = SevenZWriter::create(&archive).unwrap(); + for (name, content) in entries { + let entry = SevenZArchiveEntry { + name: (*name).to_string(), + ..Default::default() + }; + writer.push_archive_entry(entry, Some(*content)).unwrap(); + } + writer.finish().unwrap(); + } + "tar" => { + let file = fs::File::create(&archive).unwrap(); + let mut builder = Builder::new(file); + for (name, content) in entries { + let mut header = Header::new_gnu(); + header.set_size(content.len() as u64); + header.set_mode(0o644); + header.set_entry_type(EntryType::Regular); + let bytes = name.as_bytes(); + header.as_old_mut().name[..bytes.len()].copy_from_slice(bytes); + header.set_cksum(); + builder.append(&header, *content).unwrap(); + } + builder.finish().unwrap(); + } + other => panic!("no fixture builder for {other}"), + } + archive +} + +/// Every file under `dir`, relative and sorted, with `/` separators so the +/// expectations read the same on Windows. +fn files_under(dir: &Path) -> Vec { + let mut found = Vec::new(); + let mut pending = vec![dir.to_path_buf()]; + while let Some(current) = pending.pop() { + let Ok(children) = fs::read_dir(¤t) else { + continue; + }; + for child in children.flatten() { + let path = child.path(); + if path.is_dir() { + pending.push(path); + } else { + found.push( + path.strip_prefix(dir) + .unwrap() + .to_string_lossy() + .replace('\\', "/"), + ); + } + } + } + found.sort(); + found +} + +fn sorted(mut names: Vec) -> Vec { + for name in &mut names { + *name = name.replace('\\', "/"); + } + names.sort(); + names +} + +/// Extract the way a Windows machine would, wherever this runs. +fn as_windows(replacements: Substitutions) -> ExtractOptions { + ExtractOptions::new() + .with_rules(NameRules::windows()) + .with_replacements(replacements) +} + +// ------------------------------------------------------------- the ruleset -- + +#[test] +fn windows_refuses_every_character_win32_reserves() { + // Straight from "Naming Files, Paths, and Namespaces". Dropping any one of + // these from the ruleset makes this fail, which is the only thing standing + // between the list and somebody's memory of it. The two separators are + // deliberately not here: the rules judge one component, and splitting is + // the caller's job. + for character in ['<', '>', '"', '|', '?', '*'] { + let name = format!("a{character}b.txt"); + assert_eq!( + NameRules::windows().problems(&name), + vec![NameProblem::Character { + character, + fault: CharacterFault::Rejected, + }], + "{name}" + ); + } +} + +#[test] +fn a_colon_is_reinterpreted_rather_than_rejected() { + // Issue #63, and the reason `CharacterFault` has two variants at all: + // Windows *accepts* `notes.txt:hidden` and writes the bytes into an + // alternate data stream of `notes.txt`. Calling it `Rejected` would let a + // front end tell the user the write fails, when the danger is that it does + // not. + assert_eq!( + NameRules::windows().problems("notes.txt:hidden"), + vec![NameProblem::Character { + character: ':', + fault: CharacterFault::Reinterpreted, + }] + ); +} + +#[test] +fn windows_refuses_control_characters_and_unix_refuses_only_the_nul() { + // The documented range is 0 through 31 for Windows. On Unix the only byte a + // file name cannot hold is the NUL, and this pins the asymmetry so nobody + // "simplifies" the two rulesets into one. + for character in ['\u{0}', '\u{1}', '\t', '\n', '\u{1f}'] { + let name = format!("a{character}b"); + assert!( + !NameRules::windows().can_write(&name), + "windows should refuse {:?}", + character + ); + } + assert!(!NameRules::unix().can_write("a\u{0}b")); + for character in ['\u{1}', '\t', '\n', '\u{1f}'] { + let name = format!("a{character}b"); + assert!( + NameRules::unix().can_write(&name), + "unix holds {:?} perfectly well", + character + ); + } +} + +#[test] +fn a_trailing_dot_or_space_is_a_problem_and_the_whole_run_is_reported() { + assert_eq!( + NameRules::windows().problems("notes.txt."), + vec![NameProblem::TrailingCharacters { + removed: ".".to_string() + }] + ); + assert_eq!( + NameRules::windows().problems("notes.txt . "), + vec![NameProblem::TrailingCharacters { + removed: " . ".to_string() + }], + "the run is everything the host would drop, not just the last character" + ); + // The counterweight: dots and spaces are ordinary in the middle of a name, + // and a rule that flagged them would make this feature fire on nearly every + // archive. + assert!(NameRules::windows().can_write("my notes.v2.txt")); + assert!(NameRules::windows().can_write(".hidden")); +} + +#[test] +fn reserved_device_names_are_matched_without_their_extension_and_case_insensitively() { + for name in [ + "CON", + "con", + "Con.txt", + "PRN", + "AUX", + "NUL", + "NUL.tar.gz", + "COM1", + "com9", + "LPT1", + "LPT9", + "CON ", + ] { + assert!( + !NameRules::windows().can_write(name), + "{name} resolves to a device" + ); + } + // The superscripts are not a typo: Windows reads ISO 8859-1 `¹ ² ³` as + // digits, so these are devices too. Nothing but reading the documentation + // would put them in the ruleset, and nothing but this test would keep them. + for name in ["COM¹", "com²", "LPT³"] { + assert!( + !NameRules::windows().can_write(name), + "{name} resolves to a device" + ); + } + // And the neighbours that are perfectly ordinary files. A rule written as + // "starts with CON" or "contains COM" fails here. + for name in [ + "COM0", + "COM10", + "CONS", + "CONSOLE.txt", + "my CON", + "a.CON", + "LPT", + "NULL", + ] { + assert!( + NameRules::windows().can_write(name), + "{name} is an ordinary file name" + ); + } +} + +#[test] +fn unix_writes_what_windows_will_not() { + // The honest framing of the whole feature: on Unix nearly nothing here is a + // problem, so extraction has no question to ask. If this ever fails, the + // host rules have picked up portability rules, and every Mac and Linux user + // is being asked about names their machine can hold. + for name in ["what?.txt", "notes.txt.", "CON", "a:b", "trailing "] { + assert!( + NameRules::unix().can_write(name), + "{name} is writable on Unix" + ); + assert!( + !NameRules::windows().can_write(name), + "{name} is not writable on Windows" + ); + } +} + +#[test] +fn the_host_rules_are_this_platform_s_rules() { + let expected = if cfg!(windows) { + NameRules::windows() + } else { + NameRules::unix() + }; + assert_eq!(NameRules::host(), expected); + assert_eq!(NameRules::default(), NameRules::host()); +} + +// ------------------------------------------------------------- rewriting ---- + +#[test] +fn a_replacement_is_applied_to_every_occurrence_and_may_be_empty() { + let rules = NameRules::windows(); + let answers = Substitutions::new().with('?', "_"); + assert_eq!(rules.rewrite("a?b?c", &answers).unwrap(), "a_b_c"); + let dropped = Substitutions::new().with('?', ""); + assert_eq!(rules.rewrite("a?b", &dropped).unwrap(), "ab"); +} + +#[test] +fn the_structural_problems_are_adjusted_without_being_asked() { + // A trailing dot and a device name have no offending character, so there is + // nothing to put a text field beside; they are stated, not asked. If this + // ever needs an answer, the UI has a field with no question. + let rules = NameRules::windows(); + let nothing = Substitutions::new(); + assert_eq!(rules.rewrite("notes.txt.", ¬hing).unwrap(), "notes.txt"); + assert_eq!(rules.rewrite("CON.txt", ¬hing).unwrap(), "CON_.txt"); + assert_eq!( + rules.rewrite("con", ¬hing).unwrap(), + "con_", + "the adjustment keeps the spelling the archive used" + ); +} + +#[test] +fn the_adjustments_run_after_the_replacements_not_before() { + // Both of these come out wrong if the order in `rewrite` is reversed, and + // both are reachable from an ordinary answer: + let rules = NameRules::windows(); + // `M` for `?` spells a device that was not in the archive. + assert_eq!( + rules + .rewrite("CO?1", &Substitutions::new().with('?', "M")) + .unwrap(), + "COM1_" + ); + // `.` for `?` puts a dot at the end, which Windows would drop silently. + assert_eq!( + rules + .rewrite("notes?", &Substitutions::new().with('?', ".")) + .unwrap(), + "notes" + ); +} + +#[test] +fn a_replacement_the_host_cannot_write_either_is_refused() { + let err = NameRules::windows() + .rewrite("a?b", &Substitutions::new().with('?', "*")) + .unwrap_err(); + assert_eq!( + err, + NameError::UnwritableReplacement { + character: '?', + replacement: "*".to_string(), + offending: '*', + } + ); +} + +#[test] +fn a_replacement_may_not_contain_a_path_separator() { + // Not a usability rule: `../` in an answer is a traversal, since the + // replacement lands inside a component that has already been cleared by the + // containment guard. + for replacement in ["../", "a/b", r"a\b"] { + let err = NameRules::windows() + .rewrite("a?b", &Substitutions::new().with('?', replacement)) + .unwrap_err(); + assert!( + matches!(err, NameError::SeparatorInReplacement { .. }), + "{replacement}: {err}" + ); + } +} + +#[test] +fn an_answer_that_leaves_no_name_or_spells_a_parent_directory_is_refused() { + let rules = NameRules::windows(); + // An answer of `.` for a two-character name spells the directory above, + // which would climb out of the output directory. Under these rules the + // trailing-dot adjustment gets there first and leaves nothing at all, so + // the refusal says `""` rather than `".."`; either way it is refused, and + // the second assertion is the one that matters if a future ruleset stops + // trimming trailing dots. + let outcome = rules.rewrite("??", &Substitutions::new().with('?', ".")); + assert!( + matches!(&outcome, Err(NameError::Unnameable { .. })), + "{outcome:?}" + ); + assert_ne!(outcome.unwrap_or_default(), ".."); + // An empty answer that empties the whole name. + let err = rules + .rewrite("??", &Substitutions::new().with('?', "")) + .unwrap_err(); + assert!(matches!(err, NameError::Unnameable { result, .. } if result.is_empty())); + // And a name that is nothing but what the host drops, which no answer can + // help with because there is no character to answer for. + let err = rules.rewrite("...", &Substitutions::new()).unwrap_err(); + assert!(matches!(err, NameError::Unnameable { .. }), "{err}"); +} + +#[test] +fn an_unanswered_character_is_reported_against_the_whole_entry() { + // The user sees entry names, not components. Dropping `in_entry` would name + // `a?b.txt` here, which is not a line they can find in any listing. + let err = NameRules::windows() + .rewrite_entry("photos/2026/a?b.txt", &Substitutions::new()) + .unwrap_err(); + assert_eq!( + err, + NameError::NoReplacement { + entry: "photos/2026/a?b.txt".to_string(), + character: '?', + } + ); + assert!(err.to_string().contains("photos/2026/a?b.txt"), "{err}"); +} + +#[test] +fn every_component_of_an_entry_is_rewritten() { + let answers = Substitutions::new().with(':', "-").with('?', "_"); + let written = NameRules::windows() + .rewrite_entry("a:b/CON/c?.txt.", &answers) + .unwrap(); + assert_eq!(written, PathBuf::from("a-b").join("CON_").join("c_.txt")); +} + +#[test] +fn a_key_that_is_not_a_single_character_is_refused() { + // The front ends receive their answers as strings (a JSON object has no + // char keys), so this is where "??" or "" is caught, once, instead of in + // each of them. + let mut answers = Substitutions::new(); + assert!(answers.set_str("?", "_").is_ok()); + assert_eq!(answers.get('?'), Some("_")); + assert!(matches!( + answers.set_str("??", "_"), + Err(NameError::NotOneCharacter { .. }) + )); + assert!(matches!( + answers.set_str("", "_"), + Err(NameError::NotOneCharacter { .. }) + )); +} + +// ---------------------------------------------------------------- reports --- + +#[test] +fn the_report_asks_about_each_character_once_and_says_how_many_entries_carry_it() { + // One text field per character is the whole shape of the UI: a report that + // listed the characters per entry would put three fields on screen for the + // same question. + let names = [ + "ok.txt", + "a?b.txt", + "c?d.txt", + "notes.txt:hidden", + "photos/e?f.txt", + ]; + let report = NameReport::of(&names, NameRules::windows()); + assert_eq!(report.entries.len(), 4); + assert_eq!( + report + .characters + .iter() + .map(|c| (c.character, c.fault, c.entries)) + .collect::>(), + vec![ + ('?', CharacterFault::Rejected, 3), + (':', CharacterFault::Reinterpreted, 1), + ] + ); +} + +#[test] +fn the_report_separates_the_questions_from_the_stated_adjustments() { + let names = ["what?.txt", "notes.txt.", "CON.log"]; + let report = NameReport::of(&names, NameRules::windows()); + let asked: Vec> = report + .entries + .iter() + .map(|e| e.problems[0].replaceable()) + .collect(); + assert_eq!( + asked, + vec![Some('?'), None, None], + "only a character is a question; the other two are announcements" + ); + assert_eq!( + report.characters.len(), + 1, + "a trailing dot and a device name must not produce a text field" + ); + assert_eq!( + report.entries[2].problems, + vec![NameProblem::ReservedDevice { + device: "CON".to_string() + }] + ); +} + +#[test] +fn a_listing_the_host_can_write_reports_nothing() { + let names = ["a.txt", "photos/b.jpg", "photos/sub/"]; + assert!(NameReport::of(&names, NameRules::windows()).is_empty()); + assert!(NameReport::of(&names, NameRules::unix()).is_empty()); +} + +// --------------------------------------------------- inspecting an archive -- + +#[test] +fn inspecting_an_archive_finds_the_unwritable_names_without_extracting_anything() { + for format in FORMATS { + let dir = tempfile::TempDir::new().unwrap(); + let archive = archive_with( + dir.path(), + format, + &[ + ("summary.txt", b"fine"), + ("what?.txt", b"question"), + ("notes.txt.", b"trailing"), + ("CON.txt", b"device"), + ], + ); + + let report = unwritable_names_with(&archive, NameRules::windows()).unwrap(); + + assert_eq!( + report + .entries + .iter() + .map(|e| e.entry.as_str()) + .collect::>(), + vec!["what?.txt", "notes.txt.", "CON.txt"], + "{format}: the writable entry must not be reported" + ); + assert_eq!( + report.characters.len(), + 1, + "{format}: one question, for `?`" + ); + // Nothing was written: this is what a front end calls before it has + // even asked the user where the files should go. + assert_eq!( + files_under(dir.path()), + vec![format!("input.{format}")], + "{format}: inspection created something" + ); + } +} + +#[test] +fn inspecting_this_machine_s_own_archives_asks_nothing() { + // The regression that would make the feature intolerable: a report that + // fires on ordinary names would put a dialog in front of every extraction. + for format in FORMATS { + let dir = tempfile::TempDir::new().unwrap(); + let archive = archive_with( + dir.path(), + format, + &[("summary.txt", b"fine"), ("photos/a.jpg", b"also fine")], + ); + assert!( + collapse_core::unwritable_names(&archive) + .unwrap() + .is_empty(), + "{format}" + ); + } +} + +// -------------------------------------------- extracting with the answers --- + +#[test] +fn the_answers_are_written_and_the_listing_names_what_is_on_disk() { + // Issue #64 end to end, for all three formats. The listing is the half that + // matters most: returning the archive's names would have a front end show + // `what?.txt` next to a file called `what_.txt`. + for format in FORMATS { + let dir = tempfile::TempDir::new().unwrap(); + let archive = archive_with( + dir.path(), + format, + &[ + ("summary.txt", b"fine"), + ("what?.txt", b"question"), + ("notes.txt.", b"trailing"), + ("CON.txt", b"device"), + ], + ); + let out = dir.path().join("out"); + + let written = extract_with( + &archive, + &out, + &as_windows(Substitutions::new().with('?', "_")), + ) + .unwrap(); + + let expected = vec![ + "CON_.txt".to_string(), + "notes.txt".to_string(), + "summary.txt".to_string(), + "what_.txt".to_string(), + ]; + assert_eq!(sorted(written), expected, "{format}: the returned listing"); + assert_eq!(files_under(&out), expected, "{format}: what is on disk"); + assert_eq!( + fs::read(out.join("what_.txt")).unwrap(), + b"question", + "{format}: the renamed entry kept its content" + ); + } +} + +#[test] +fn a_colon_entry_becomes_a_file_of_its_own_and_leaves_its_neighbour_alone() { + // Issue #63. On Windows the unfixed path writes these bytes into the + // `hidden` stream of `notes.txt`, which changes nothing about `notes.txt` + // that `dir` can see and leaves the listing naming a file that exists + // nowhere. Here the answer turns it into a file, and the assertion that + // `notes.txt` still holds its own bytes is what would fail if a future + // "simplification" let the colon through. + for format in FORMATS { + let dir = tempfile::TempDir::new().unwrap(); + let archive = archive_with( + dir.path(), + format, + &[ + ("notes.txt", b"the real file"), + ("notes.txt:hidden", b"the payload"), + ], + ); + let out = dir.path().join("out"); + + let written = extract_with( + &archive, + &out, + &as_windows(Substitutions::new().with(':', "-")), + ) + .unwrap(); + + assert_eq!( + sorted(written), + vec!["notes.txt".to_string(), "notes.txt-hidden".to_string()], + "{format}" + ); + assert_eq!(fs::read(out.join("notes.txt")).unwrap(), b"the real file"); + assert_eq!( + fs::read(out.join("notes.txt-hidden")).unwrap(), + b"the payload" + ); + } +} + +#[test] +fn an_entry_with_no_answer_stops_before_anything_is_written() { + // The pre-pass earning its keep: judged one entry at a time, `summary.txt` + // would already be on disk when `what?.txt` was refused, and the user would + // be left with half a directory and no list of what is in it (issue #64's + // other complaint). + for format in FORMATS { + let dir = tempfile::TempDir::new().unwrap(); + let archive = archive_with( + dir.path(), + format, + &[("summary.txt", b"fine"), ("what?.txt", b"question")], + ); + let out = dir.path().join("out"); + + let err = extract_with(&archive, &out, &as_windows(Substitutions::new())).unwrap_err(); + + let message = err.to_string(); + assert!(message.contains("what?.txt"), "{format}: {message}"); + assert!(message.contains('?'), "{format}: {message}"); + assert!( + files_under(&out).is_empty(), + "{format}: {:?} was written before the refusal", + files_under(&out) + ); + } +} + +#[test] +fn two_entries_that_would_land_on_one_name_are_refused_by_name() { + // Deliberately not disambiguated: renaming one of them to `a_b (2).txt` is + // how a user ends up with a file they never look at again. Both names are + // in the message so the answer can be changed. + for format in FORMATS { + let dir = tempfile::TempDir::new().unwrap(); + let archive = archive_with( + dir.path(), + format, + &[("a?b.txt", b"first"), ("a*b.txt", b"second")], + ); + let out = dir.path().join("out"); + + let answers = Substitutions::new().with('?', "_").with('*', "_"); + let err = extract_with(&archive, &out, &as_windows(answers)).unwrap_err(); + + let message = err.to_string(); + assert!(message.contains("a?b.txt"), "{format}: {message}"); + assert!(message.contains("a*b.txt"), "{format}: {message}"); + assert!(message.contains("a_b.txt"), "{format}: {message}"); + assert!( + files_under(&out).is_empty(), + "{format}: a collision must leave the output alone" + ); + } +} + +#[test] +fn a_renamed_entry_colliding_with_an_untouched_one_is_refused_too() { + // The case a "compare the rewritten names to each other" check would miss: + // only one of these two changes, and it lands on a name the archive already + // uses. + let dir = tempfile::TempDir::new().unwrap(); + let archive = archive_with( + dir.path(), + "zip", + &[("notes.txt", b"first"), ("notes.txt.", b"second")], + ); + let out = dir.path().join("out"); + + let err = extract_with(&archive, &out, &as_windows(Substitutions::new())).unwrap_err(); + + let message = err.to_string(); + assert!(message.contains("notes.txt."), "{message}"); + assert!(files_under(&out).is_empty()); +} + +#[test] +fn an_archive_that_already_names_one_entry_twice_still_extracts() { + // The counterweight to the collision rule. Duplicate entries are an old + // property of tar (and of extraction here: the last one wins), and this + // feature must not turn them into a refusal, because no answer the user + // gives could fix an archive that was already like that. + let dir = tempfile::TempDir::new().unwrap(); + let archive = archive_with( + dir.path(), + "tar", + &[("notes.txt", b"first"), ("notes.txt", b"second")], + ); + let out = dir.path().join("out"); + + let written = extract_with(&archive, &out, &as_windows(Substitutions::new())).unwrap(); + + assert_eq!( + written, + vec!["notes.txt".to_string(), "notes.txt".to_string()] + ); + assert_eq!(fs::read(out.join("notes.txt")).unwrap(), b"second"); +} + +#[test] +fn an_answer_the_host_cannot_write_is_refused_before_the_archive_is_opened() { + // Checked against a path that does not exist: if the answer were validated + // per entry instead of up front, this would fail with "no such file" + // instead, and a user would only learn their replacement was no good after + // choosing an archive. + let dir = tempfile::TempDir::new().unwrap(); + let missing = dir.path().join("nowhere.zip"); + let err = extract_with( + &missing, + &dir.path().join("out"), + &as_windows(Substitutions::new().with('?', "<")), + ) + .unwrap_err(); + assert!( + matches!( + err, + collapse_core::CompressionError::Name(NameError::UnwritableReplacement { .. }) + ), + "{err}" + ); +} + +#[test] +fn extraction_with_no_options_leaves_ordinary_names_alone() { + // `extract` is `extract_with` with the host's rules and no answers, and on + // this machine that has to be exactly what it always was. + for format in FORMATS { + let dir = tempfile::TempDir::new().unwrap(); + let archive = archive_with( + dir.path(), + format, + &[("summary.txt", b"fine"), ("photos/a.jpg", b"also fine")], + ); + let out = dir.path().join("out"); + let written = extract(&archive, &out).unwrap(); + assert_eq!( + sorted(written), + vec!["photos/a.jpg".to_string(), "summary.txt".to_string()], + "{format}" + ); + assert_eq!(files_under(&out), vec!["photos/a.jpg", "summary.txt"]); + } +} + +#[test] +fn the_backends_called_directly_still_write_the_archive_s_own_names() { + // The plan is the dispatcher's business. `extract_zip` and friends are + // public and are called directly (the server unpacks a tar envelope that + // way), so they must keep behaving as they did: no listing pass, no + // renaming, on any platform. + let dir = tempfile::TempDir::new().unwrap(); + for (format, extractor) in [ + ("zip", extract_zip as fn(&Path, &Path) -> _), + ("7z", extract_7z), + ("tar", extract_tar), + ] { + let archive = archive_with(dir.path(), format, &[("plain.txt", b"content")]); + let out = dir.path().join(format!("out-{format}")); + assert_eq!( + extractor(&archive, &out).unwrap(), + vec!["plain.txt".to_string()], + "{format}" + ); + } +} + +#[test] +fn an_archive_that_cannot_be_listed_still_fails_in_the_extractor_s_words() { + // The listing pass is advisory on purpose. A corrupt archive must produce + // the message extraction has always produced, not a second-hand one from a + // pass that only exists to plan names. + let dir = tempfile::TempDir::new().unwrap(); + let archive = dir.path().join("truncated.zip"); + fs::write(&archive, b"PK\x03\x04 and then nothing").unwrap(); + + let err = extract(&archive, &dir.path().join("out")).unwrap_err(); + let message = err.to_string(); + assert!(message.starts_with("Compression failed:"), "{message}"); + assert!(message.contains("Zip"), "{message}"); +} + +// ----------------------------------------------- naming the failing entry --- + +#[test] +fn a_failing_entry_names_itself_and_its_destination() { + // Issue #64, piece one, and it helps on every platform: this exact archive + // (an entry whose parent is another entry, a plain file) used to produce + // `IO error: File exists (os error 17)` with no clue which of four entries + // was at fault. A read-only output directory and a full disk were just as + // blank. + for format in FORMATS { + let dir = tempfile::TempDir::new().unwrap(); + let archive = archive_with( + dir.path(), + format, + &[("a.txt", b"a file"), ("a.txt/b.txt", b"a child of a file")], + ); + let out = dir.path().join("out"); + + let err = extract(&archive, &out).unwrap_err(); + + let message = err.to_string(); + assert!( + message.contains("a.txt/b.txt") || message.contains(r"a.txt\b.txt"), + "{format}: the message must name the entry: {message}" + ); + // Windows renders this from a canonicalized root, which carries a `\\?\` + // verbatim prefix and expands any 8.3 short name on the way, so the + // message legitimately does not contain the path this test built. What + // it must contain is where extraction actually resolved to. + let resolved = out.canonicalize().unwrap_or_else(|_| out.clone()); + assert!( + message.contains(&resolved.display().to_string()), + "{format}: the message must say where it was going: {message}" + ); + } +} + +// ------------------------------------- the seam: splitting is not the host's -- + +/// An archive entry name is not a host path, and this is the test that says so. +/// +/// It used to be split with `Path::new(name).components()`, and `std::path` is +/// `#[cfg]`-dependent while `NameRules` is data, so the rules were portable and +/// the splitting they ran over was not. Both directions were wrong at once, and +/// neither was visible from a Mac: +/// +/// * Windows parses a leading `a:` as a drive prefix, which is not a `Normal` +/// component, so it was silently dropped and `a:b/c.txt` was judged, reported +/// and written as `b/c.txt`. The colon that issue #63 is entirely about went +/// unasked on the only platform issue #63 concerns. +/// * Windows treats `\` as a separator and Unix does not, so one name split +/// into a different number of components depending on who was reading. +/// +/// ZIP mandates `/` (APPNOTE 4.4.17.1) and tar has used it since v7, so the +/// component count is a property of the archive and must not move. +#[test] +fn an_entry_splits_the_same_way_on_every_host() { + // One component, whatever std would make of it. `\` is not a separator in + // an archive, and `a:` is not a drive. + for name in ["a:b", "C:x", r"dir\file.txt", r"\\server\share"] { + assert_eq!( + NameRules::unix() + .rewrite_entry(name, &Substitutions::new()) + .ok(), + Some(PathBuf::from(name)), + "{name} must stay one component: Unix can hold every character in it" + ); + } + // And the split happens exactly where the archive says it does. + assert_eq!( + NameRules::unix() + .rewrite_entry("a:b/c.txt", &Substitutions::new()) + .unwrap(), + PathBuf::from("a:b").join("c.txt") + ); +} + +/// The Windows half of the same seam: the report must ask about a colon +/// wherever it sits, including the leading component that used to vanish. +/// +/// Note this one passed on Unix before the fix and failed only on Windows, +/// since `a:b` was already a single component here. It is a pin, not a +/// reproduction: what it stops is the answer diverging by host again. +#[test] +fn the_report_sees_a_colon_in_the_first_component_too() { + for name in ["a:b/c.txt", "C:/x/y", "deep/a:b.txt"] { + let problems = NameRules::windows().entry_problems(name); + assert_eq!( + problems, + vec![NameProblem::Character { + character: ':', + fault: CharacterFault::Reinterpreted, + }], + "{name}: the colon must be a question on every host" + ); + } +} + +/// A backslash is an ordinary character in an archive entry, so the two rulesets +/// must disagree about it, and each must be right about its own filesystem. +#[test] +fn only_windows_refuses_a_backslash_inside_a_component() { + assert_eq!( + NameRules::windows().entry_problems(r"dir\file.txt"), + vec![NameProblem::Character { + character: '\\', + fault: CharacterFault::Rejected, + }] + ); + assert!(NameRules::unix().entry_problems(r"dir\file.txt").is_empty()); +} + +/// Issue caught by nothing until Windows CI ran: collapse could build an archive +/// it then refused to extract. +/// +/// A backslash is a legal character in a Unix file name. The old splitter made +/// it one component on Unix, and `rewrite` refused any rewritten component +/// holding a separator, so `extract` failed the **whole** archive and wrote +/// nothing, after `compress_dir` had happily archived it and the verification +/// pass had signed it off. The user could have deleted the originals by then. +#[cfg(unix)] +#[test] +fn a_unix_name_holding_a_backslash_survives_the_round_trip() { + use collapse_core::{compress_dir, Algorithm, Verify}; + + for algorithm in [Algorithm::Zip, Algorithm::Tar, Algorithm::SevenZ] { + let dir = tempfile::TempDir::new().unwrap(); + let tree = dir.path().join("tree"); + fs::create_dir_all(&tree).unwrap(); + fs::write(tree.join(r"a\b.txt"), b"payload").unwrap(); + fs::write(tree.join("ok.txt"), b"fine").unwrap(); + + let archive = dir.path().join(format!("t.{}", algorithm.extension())); + compress_dir(&tree, &archive, algorithm, 3, Verify::Index).unwrap(); + + let out = dir.path().join("back"); + let mut files = extract(&archive, &out).unwrap(); + files.sort(); + assert_eq!( + files, + vec![r"tree/a\b.txt".to_string(), "tree/ok.txt".to_string()], + "{algorithm}: the archive this crate just built must extract" + ); + assert_eq!( + fs::read(out.join("tree").join(r"a\b.txt")).unwrap(), + b"payload" + ); + } +} diff --git a/apps/core/tests/security.rs b/apps/core/tests/security.rs index f78b145..63851ff 100644 --- a/apps/core/tests/security.rs +++ b/apps/core/tests/security.rs @@ -9,8 +9,9 @@ use std::path::Path; use collapse_core::compression::{ compress_7z_dir, compress_tar_dir, compress_zip_dir, extract_7z, extract_tar, extract_zip, + NameRules, Substitutions, }; -use collapse_core::{extract, Algorithm}; +use collapse_core::{extract, extract_with, Algorithm, ExtractOptions, Verify}; use sevenz_rust2::{SevenZArchiveEntry, SevenZWriter}; use tar::{Builder, EntryType, Header}; use zip::write::SimpleFileOptions; @@ -364,7 +365,15 @@ fn benign_nested_names_still_extract() { for algo in [Algorithm::Zip, Algorithm::SevenZ, Algorithm::Tar] { let archive = dir.path().join(format!("ok.{}", algo.extension())); - collapse_core::compress(&src, &archive, "nested/dir/input.txt", algo, 1).unwrap(); + collapse_core::compress( + &src, + &archive, + "nested/dir/input.txt", + algo, + 1, + Verify::Index, + ) + .unwrap(); let out = dir.path().join(format!("out_{}", algo.extension())); let files = extract(&archive, &out).unwrap(); @@ -559,6 +568,99 @@ fn sevenz_rejects_malicious_entry_after_benign_ones() { assert_contained(extract_7z(&archive, &out), &dir.path().join("escape.txt")); } +// -- entry names Windows accepts and reads as something other than a file -- + +/// Names whose colon Win32 reads as alternate data stream syntax rather than as +/// part of a file name: the first attaches a payload to a file an archive can +/// extract legitimately alongside it (`Zone.Identifier` among the streams it +/// could overwrite), the second attaches one to the output directory itself. +/// +/// Both are ordinary, if odd, file names on Unix, so the assertions split by +/// platform: refusing them there would be a bug of its own. +const STREAM_SHAPED_NAMES: [&str; 2] = ["notes.txt:hidden", ":hidden"]; + +#[test] +fn no_format_writes_an_entry_name_with_a_colon_as_a_stream() { + for (ext, build) in [ + ("zip", malicious_zip as fn(&Path, &str)), + ("7z", malicious_7z), + ("tar", malicious_tar), + ] { + for name in STREAM_SHAPED_NAMES { + let dir = tempfile::TempDir::new().unwrap(); + let archive = dir.path().join(format!("stream.{ext}")); + build(&archive, name); + let out = dir.path().join("out"); + + let result = extract(&archive, &out); + + if cfg!(windows) { + // The host would take this name and write the bytes somewhere + // no listing shows, so extraction stops and says which entry + // and which character (issue #63). Nothing is written at all: + // the naming pass runs before the output directory exists. + let message = match result { + Err(err) => err.to_string(), + Ok(files) => panic!("{ext}/{name}: must be refused, wrote {files:?}"), + }; + assert!(message.contains(name), "{ext}/{name}: {message}"); + assert!(message.contains(':'), "{ext}/{name}: {message}"); + assert!( + !out.exists() || std::fs::read_dir(&out).unwrap().next().is_none(), + "{ext}/{name}: something was written before the refusal" + ); + } else { + // One contained file, named exactly as the archive spells it. + let files = + result.unwrap_or_else(|e| panic!("{ext}/{name}: a legal Unix name, got {e}")); + assert_eq!(listing(files), vec![name.to_string()], "{ext}/{name}"); + assert!(out.join(name).is_file(), "{ext}/{name}"); + } + // Whatever the platform made of it, nothing landed beside `out`, + // and in particular no carrier file appeared next to the archive. + assert!( + !dir.path().join("notes.txt").exists(), + "{ext}/{name}: a file was written outside the output directory" + ); + } + } +} + +#[test] +fn a_replacement_cannot_carry_an_entry_out_of_the_output_directory() { + // The answer the user gives is put inside a name that containment has + // already cleared, so an unchecked replacement is a traversal by the back + // door: `?` answered with `../..` would write above the output directory + // with nothing left to notice it. + for (ext, build) in [ + ("zip", malicious_zip as fn(&Path, &str)), + ("7z", malicious_7z), + ("tar", malicious_tar), + ] { + for replacement in ["../../escape", "/escape", r"..\..\escape"] { + let dir = tempfile::TempDir::new().unwrap(); + let archive = dir.path().join(format!("q.{ext}")); + build(&archive, "sub/a?b.txt"); + let out = dir.path().join("out"); + + let options = ExtractOptions::new() + .with_rules(NameRules::windows()) + .with_replacements(Substitutions::new().with('?', replacement)); + let result = extract_with(&archive, &out, &options); + + assert!( + result.is_err(), + "{ext}: {replacement:?} was accepted as a replacement" + ); + assert!( + !dir.path().join("escape").exists() + && !dir.path().join("..").join("escape").exists(), + "{ext}: {replacement:?} wrote outside the output directory" + ); + } + } +} + // -- compression: archiving a directory must never follow a symlink out of // the tree (all three formats skip symlinks) -- @@ -596,3 +698,104 @@ fn compress_dir_skips_symlinks_for_every_format() { ); } } + +// -- writing through something already in the output directory -- + +/// A symlink the extractor did not create, sitting in the output directory +/// before extraction begins, is the one traversal a name-only guard cannot see. +/// +/// It was reachable and silent: with `link` a symlink in the output directory, +/// an archive holding `link/evil.txt` wrote straight through it and returned +/// `Ok`. tar was immune, because `unpack_in` resolves the directory it is about +/// to write into; zip and 7z joined a sanitized name and wrote. Extracting into +/// a directory that already holds a symlink is ordinary, and this predates the +/// naming work rather than arriving with it. +#[cfg(unix)] +#[test] +fn no_format_writes_through_a_symlink_already_in_the_output() { + for ext in ["zip", "7z", "tar"] { + let dir = tempfile::TempDir::new().unwrap(); + let outside = dir.path().join("outside"); + std::fs::create_dir_all(&outside).unwrap(); + let out = dir.path().join("out"); + std::fs::create_dir_all(&out).unwrap(); + std::os::unix::fs::symlink(&outside, out.join("link")).unwrap(); + + let archive = dir.path().join(format!("a.{ext}")); + match ext { + "zip" => malicious_zip(&archive, "link/evil.txt"), + "7z" => malicious_7z(&archive, "link/evil.txt"), + _ => malicious_tar(&archive, "link/evil.txt"), + } + + let escaped = outside.join("evil.txt"); + assert_contained(extract(&archive, &out), &escaped); + } +} + +/// The same guard, reached through the naming layer rather than around it: a +/// planned rename must not be able to land outside either. +#[cfg(unix)] +#[test] +fn a_renamed_entry_cannot_be_written_through_such_a_symlink() { + for ext in ["zip", "7z", "tar"] { + let dir = tempfile::TempDir::new().unwrap(); + let outside = dir.path().join("outside"); + std::fs::create_dir_all(&outside).unwrap(); + let out = dir.path().join("out"); + std::fs::create_dir_all(&out).unwrap(); + std::os::unix::fs::symlink(&outside, out.join("link")).unwrap(); + + let archive = dir.path().join(format!("a.{ext}")); + match ext { + "zip" => malicious_zip(&archive, "link/ev?l.txt"), + "7z" => malicious_7z(&archive, "link/ev?l.txt"), + _ => malicious_tar(&archive, "link/ev?l.txt"), + } + + // Windows rules make the `?` a question, so this entry is renamed and + // takes the planned write path rather than the untouched one. + let options = ExtractOptions::new() + .with_rules(NameRules::windows()) + .with_replacements(Substitutions::new().with('?', "i")); + let escaped = outside.join("evil.txt"); + assert_contained(extract_with(&archive, &out, &options), &escaped); + } +} + +/// `PathBuf::push` replaces what it holds when handed a path carrying a prefix, +/// and Windows reads `c:` at the head of a component as a drive. Prefixes are +/// parsed only at the head of a whole path, so a colon in a *later* component +/// gives `sanitize_entry_path` nothing to reject while still clearing the +/// buffer it is building, which would drop `docs` and leave a drive-relative +/// path resolving against the current directory of C:. +/// +/// On Unix a colon is an ordinary character and this is simply a nested file, +/// which is the half this can assert here. Either way the bytes must land under +/// the output directory and nowhere else. +#[test] +fn a_colon_in_a_later_component_cannot_clear_the_path_being_built() { + for ext in ["zip", "7z", "tar"] { + let dir = tempfile::TempDir::new().unwrap(); + let out = dir.path().join("out"); + let archive = dir.path().join(format!("a.{ext}")); + match ext { + "zip" => malicious_zip(&archive, "docs/c:evil.txt"), + "7z" => malicious_7z(&archive, "docs/c:evil.txt"), + _ => malicious_tar(&archive, "docs/c:evil.txt"), + } + + match extract(&archive, &out) { + // Unix: written, and written inside. + Ok(files) => { + assert_eq!(listing(files), vec!["docs/c:evil.txt"], "{ext}"); + assert!(out.join("docs").join("c:evil.txt").exists(), "{ext}"); + } + // Windows: refused, and nothing written anywhere. + Err(_) => assert!( + !out.join("c:evil.txt").exists(), + "{ext}: a drive-relative path was written" + ), + } + } +} diff --git a/apps/core/tests/sevenz.rs b/apps/core/tests/sevenz.rs index 98cab81..174905c 100644 --- a/apps/core/tests/sevenz.rs +++ b/apps/core/tests/sevenz.rs @@ -1,7 +1,15 @@ //! Tests for the 7z backend (`compress_7z` / `extract_7z`). -use collapse_core::compression::{compress_7z, compress_7z_dir, extract_7z}; +use std::io::Write; +use std::path::Path; + +use collapse_core::compression::{ + compress_7z, compress_7z_dir, compress_zip, extract_7z, extract_zip, +}; +use collapse_core::CompressionError; use sevenz_rust2::{SevenZArchiveEntry, SevenZWriter}; +use zip::write::SimpleFileOptions; +use zip::{CompressionMethod, ZipWriter}; const SAMPLE: &[u8] = b"Hello, Collapse! Hello, Collapse! Hello, Collapse! "; @@ -130,8 +138,26 @@ fn extract_7z_creates_output_dir() { #[test] fn extract_7z_nonexistent_archive_errors() { let dir = tempfile::TempDir::new().unwrap(); - let result = extract_7z(&dir.path().join("nope.7z"), &dir.path().join("out")); - assert!(result.is_err()); + let out = dir.path().join("out"); + let err = extract_7z(&dir.path().join("nope.7z"), &out).unwrap_err(); + + // `is_err()` alone passed whatever the message was, including the struct + // dump 7z used to produce. Pin the variant and the sentence: this failure + // never reaches the dependency at all (`File::open` refuses first), so it + // must read like every other missing file in the crate. + assert!( + matches!(err, CompressionError::Io(ref io) if io.kind() == std::io::ErrorKind::NotFound), + "expected a NotFound Io, got {err:?}" + ); + #[cfg(unix)] + assert_eq!( + err.to_string(), + "IO error: No such file or directory (os error 2)" + ); + // `extract_7z` creates the output directory before it opens the archive, + // so a missing archive still leaves the directory behind; the point here + // is only that it stays empty. + assert_eq!(std::fs::read_dir(&out).unwrap().count(), 0); } #[test] @@ -172,13 +198,26 @@ fn extract_7z_empty_archive_returns_empty_list() { #[test] fn compress_nonexistent_source_errors() { let dir = tempfile::TempDir::new().unwrap(); - let result = compress_7z( - &dir.path().join("nope.txt"), - &dir.path().join("out.7z"), - "nope.txt", - 1, + let output = dir.path().join("out.7z"); + let err = compress_7z(&dir.path().join("nope.txt"), &output, "nope.txt", 1).unwrap_err(); + + // `is_err()` alone could not tell this apart from a struct dump. The read + // of the source happens before the writer exists, so this must be a plain + // `Io` with std's own wording. + assert!( + matches!(err, CompressionError::Io(ref io) if io.kind() == std::io::ErrorKind::NotFound), + "expected a NotFound Io, got {err:?}" + ); + #[cfg(unix)] + assert_eq!( + err.to_string(), + "IO error: No such file or directory (os error 2)" ); - assert!(result.is_err()); + // And the observable effect: the source is read before the writer exists, + // so a missing source leaves no stub archive behind. All three backends + // agree on this now; `compress_zip` used to create its output first and + // leave a zero-byte `.zip`, and `zip.rs` pins that it no longer does. + assert!(!output.exists(), "a stub archive was left on disk"); } #[test] @@ -252,6 +291,773 @@ fn compress_7z_dir_rejects_non_directory() { let file = source_file(dir.path()); let archive = dir.path().join("out.7z"); - let result = compress_7z_dir(&file, &archive, 1); - assert!(result.is_err()); + let err = compress_7z_dir(&file, &archive, 1).unwrap_err(); + + // This one never reaches the dependency: `walk_tree` refuses first, so the + // message is core's own and it does name the path (the caller passed it). + // Pinned so the check cannot be silently downgraded to "some error". + assert_eq!( + err.to_string(), + format!("Compression failed: Not a directory: {}", file.display()) + ); + assert!(!archive.exists(), "a stub archive was left on disk"); +} + +// -- error reporting -- +// +// `sevenz_rust2::Error` implements `Display` as `Debug`, so the backend must +// translate it rather than stringify it. Every test below fails the moment a +// `map_err(from_sevenz)` in `compression/sevenz.rs` goes back to +// `CompressionError::Failed(e.to_string())`: they pin the whole message, and +// the Debug spelling is never the same string. + +/// The six bytes every 7z file starts with, so a header can be hand-built far +/// enough to reach the check under test. +const SEVENZ_SIGNATURE: [u8; 6] = [b'7', b'z', 0xBC, 0xAF, 0x27, 0x1C]; + +#[test] +fn a_missing_output_directory_reads_exactly_like_zip() { + let dir = tempfile::TempDir::new().unwrap(); + let src = source_file(dir.path()); + let missing = dir.path().join("does_not_exist"); + + let sevenz_err = compress_7z(&src, &missing.join("out.7z"), "sample.txt", 3).unwrap_err(); + let zip_err = compress_zip(&src, &missing.join("out.zip"), "sample.txt", 3).unwrap_err(); + + // The point of the mapping: the same mistake now reaches the same variant + // through all three backends, not just the same prose. + assert!( + matches!(sevenz_err, CompressionError::Io(_)), + "7z should report ENOENT as Io, got {sevenz_err:?}" + ); + assert_eq!(sevenz_err.to_string(), zip_err.to_string()); + #[cfg(unix)] + assert_eq!( + sevenz_err.to_string(), + "IO error: No such file or directory (os error 2)" + ); + // The dependency puts the absolute output path in the error it hands back; + // dropping it is half of why the mapping exists (the server forwards this + // string to unauthenticated clients). + assert!( + !sevenz_err.to_string().contains("out.7z"), + "the message leaks the output path: {sevenz_err}" + ); +} + +#[test] +fn a_file_that_is_not_an_archive_is_described_in_words() { + let dir = tempfile::TempDir::new().unwrap(); + let archive = dir.path().join("corrupt.7z"); + std::fs::write(&archive, b"this is plain text, not an archive").unwrap(); + + let err = extract_7z(&archive, &dir.path().join("out")).unwrap_err(); + + assert_eq!( + err.to_string(), + "Compression failed: not a 7z archive: the file does not start with a 7z signature" + ); + // The old spelling was `BadSignature([116, 104, 105, 115, 32, 105])`: the + // first six bytes of the user's file, printed as a byte array. + assert!( + !err.to_string().contains("116"), + "the message still echoes the file's bytes: {err}" + ); +} + +#[test] +fn an_unreadable_format_version_is_described_in_words() { + let dir = tempfile::TempDir::new().unwrap(); + let archive = dir.path().join("future.7z"); + let mut bytes = SEVENZ_SIGNATURE.to_vec(); + // Major/minor, read straight after the signature. Major 0 is the only one + // the format defines, so anything else stops the reader right here. + bytes.extend_from_slice(&[9, 4]); + std::fs::write(&archive, &bytes).unwrap(); + + let err = extract_7z(&archive, &dir.path().join("out")).unwrap_err(); + + assert_eq!( + err.to_string(), + "Compression failed: unsupported 7z format version 9.4" + ); +} + +#[test] +fn a_corrupt_header_is_described_in_words() { + let dir = tempfile::TempDir::new().unwrap(); + let archive = dir.path().join("bent.7z"); + let mut bytes = SEVENZ_SIGNATURE.to_vec(); + bytes.extend_from_slice(&[0, 0]); // version 0.0 + bytes.extend_from_slice(&0xDEAD_BEEFu32.to_le_bytes()); // claimed header CRC + bytes.extend_from_slice(&[7u8; 20]); // 20 header bytes that do not hash to it + std::fs::write(&archive, &bytes).unwrap(); + + let err = extract_7z(&archive, &dir.path().join("out")).unwrap_err(); + + assert_eq!( + err.to_string(), + "Compression failed: the 7z archive is corrupt: a checksum did not match" + ); +} + +/// A one entry archive with a byte flipped inside the entry's own data. +/// +/// Written with the COPY method on purpose: with LZMA2 a flipped byte usually +/// breaks the decoder before the checksum is ever compared, which reaches a +/// different arm of the mapping. +fn bitrotted_archive(dir: &Path) -> std::path::PathBuf { + let archive = dir.join("bitrot.7z"); + { + let mut writer = SevenZWriter::create(&archive).unwrap(); + writer.set_content_methods(vec![sevenz_rust2::SevenZMethodConfiguration::new( + sevenz_rust2::SevenZMethod::COPY, + )]); + let entry = SevenZArchiveEntry { + name: "sample.txt".to_string(), + ..Default::default() + }; + writer.push_archive_entry(entry, Some(SAMPLE)).unwrap(); + writer.finish().unwrap(); + } + + // The packed streams start straight after the 32 byte signature header, so + // this lands in the entry's stored bytes and nowhere near a header. + let mut bytes = std::fs::read(&archive).unwrap(); + bytes[40] ^= 0xFF; + std::fs::write(&archive, &bytes).unwrap(); + archive +} + +/// The dependency reports a header CRC mismatch as a variant of its own, but a +/// *data* CRC mismatch as an `io::Error` wrapping that variant, and its +/// `Display` is its `Debug`, so unwrapped it reached the user as +/// `IO error: ChecksumVerificationFailed`. Both spellings must arrive as the +/// same sentence, since the difference means nothing to whoever reads it. +#[test] +fn a_flipped_bit_in_the_data_is_described_in_words() { + let dir = tempfile::TempDir::new().unwrap(); + let archive = bitrotted_archive(dir.path()); + + let err = extract_7z(&archive, &dir.path().join("out")).unwrap_err(); + + assert_eq!( + err.to_string(), + "Compression failed: the 7z archive is corrupt: a checksum did not match" + ); +} + +#[test] +fn a_truncated_archive_is_reported_as_an_io_error() { + let dir = tempfile::TempDir::new().unwrap(); + let src = source_file(dir.path()); + let archive = dir.path().join("half.7z"); + compress_7z(&src, &archive, "sample.txt", 3).unwrap(); + let whole = std::fs::read(&archive).unwrap(); + std::fs::write(&archive, &whole[..whole.len() / 2]).unwrap(); + + let err = extract_7z(&archive, &dir.path().join("out")).unwrap_err(); + + // The dependency reports the short read as its `Io` variant, so the + // mapping hands back the `io::Error` verbatim rather than inventing a + // "truncated" sentence it cannot actually distinguish from a bad disk. + assert!( + matches!(err, CompressionError::Io(ref io) if io.kind() == std::io::ErrorKind::UnexpectedEof), + "expected an UnexpectedEof Io, got {err:?}" + ); + assert_eq!(err.to_string(), "IO error: failed to fill whole buffer"); +} + +#[test] +fn a_rejected_entry_name_reads_exactly_like_zip() { + let dir = tempfile::TempDir::new().unwrap(); + let archive = dir.path().join("evil.7z"); + { + let mut writer = SevenZWriter::create(&archive).unwrap(); + let mut entry = SevenZArchiveEntry::default(); + entry.name = "../escape.txt".to_string(); + writer + .push_archive_entry(entry, Some(b"pwned".as_slice())) + .unwrap(); + writer.finish().unwrap(); + } + + let err = extract_7z(&archive, &dir.path().join("out")).unwrap_err(); + + // `extract_7z` builds this one itself, as a `sevenz_rust2::Error::Other`, + // so it has to survive the round trip through the dependency unchanged. + assert_eq!( + err.to_string(), + "Compression failed: Path traversal detected in archive entry: ../escape.txt" + ); + + // The test's name was a claim nothing checked: it pinned a literal and + // never looked at zip, so zip could drift and only this comment would + // notice. Build the same hostile entry as a zip and compare the two + // messages, which is the property the `Other` passthrough exists for. + let zip_archive = dir.path().join("evil.zip"); + { + let f = std::fs::File::create(&zip_archive).unwrap(); + let mut w = ZipWriter::new(f); + let opts = SimpleFileOptions::default().compression_method(CompressionMethod::Stored); + // The zip crate writes the name verbatim, so a traversal name survives. + w.start_file("../escape.txt", opts).unwrap(); + w.write_all(b"pwned").unwrap(); + w.finish().unwrap(); + } + let zip_err = extract_zip(&zip_archive, &dir.path().join("out_zip")).unwrap_err(); + assert_eq!(err.to_string(), zip_err.to_string()); +} + +#[test] +fn compressing_a_tree_into_a_missing_directory_reads_like_the_single_file_path() { + let dir = tempfile::TempDir::new().unwrap(); + let root = sample_tree(dir.path()); + let output = dir.path().join("does_not_exist").join("photos.7z"); + + let err = compress_7z_dir(&root, &output, 3).unwrap_err(); + + // `compress_7z_dir` has its own `SevenZWriter::create` call site, and no + // test reached it: the whole-directory half of the backend could go back + // to `e.to_string()` on its own and every other test here would still + // pass. The message must match the single-file path exactly. + assert!( + matches!(err, CompressionError::Io(ref io) if io.kind() == std::io::ErrorKind::NotFound), + "expected a NotFound Io, got {err:?}" + ); + #[cfg(unix)] + assert_eq!( + err.to_string(), + "IO error: No such file or directory (os error 2)" + ); + assert!( + !err.to_string().contains("photos.7z"), + "the message leaks the output path: {err}" + ); +} + +// -- crafted archives -- +// +// The 7z writer can only produce well-formed archives, so the header-parsing +// failures below are unreachable without building the bytes by hand. Each one +// is gated behind a checksum, hence the CRC helper. + +/// CRC-32 (IEEE), the checksum 7z uses for both of its headers. +/// +/// Hand-rolled rather than pulled in as a dev-dependency: it is a dozen lines, +/// and the archives it seals are the only thing in the suite that needs it. +fn crc32(bytes: &[u8]) -> u32 { + let mut crc = 0xFFFF_FFFFu32; + for &byte in bytes { + crc ^= byte as u32; + for _ in 0..8 { + let mask = (crc & 1).wrapping_neg(); + crc = (crc >> 1) ^ (0xEDB8_8320 & mask); + } + } + !crc +} + +/// A 7z file whose 32-byte signature header is valid and points at `header`. +/// +/// Layout: signature (6) + format version (2) + CRC of the start header (4) + +/// start header (20: next-header offset, size and CRC) = 32 bytes, then the +/// next header itself. `declared_crc` is what the file *claims* the next +/// header hashes to, so passing a wrong value is how the mismatch case is +/// reached; `None` seals it correctly and lets the reader go on to parse it. +fn archive_bytes(header: &[u8], declared_crc: Option) -> Vec { + let mut start_header = Vec::with_capacity(20); + start_header.extend_from_slice(&0u64.to_le_bytes()); // next header offset + start_header.extend_from_slice(&(header.len() as u64).to_le_bytes()); + start_header.extend_from_slice(&declared_crc.unwrap_or_else(|| crc32(header)).to_le_bytes()); + + let mut bytes = SEVENZ_SIGNATURE.to_vec(); + bytes.extend_from_slice(&[0, 4]); // format version 0.4 + bytes.extend_from_slice(&crc32(&start_header).to_le_bytes()); + bytes.extend_from_slice(&start_header); + assert_eq!(bytes.len(), 32, "signature header must be 32 bytes"); + bytes.extend_from_slice(header); + bytes +} + +/// Write a crafted archive and hand back the failure extracting it produces. +fn extract_crafted( + dir: &Path, + name: &str, + header: &[u8], + declared_crc: Option, +) -> CompressionError { + let archive = dir.join(name); + std::fs::write(&archive, archive_bytes(header, declared_crc)).unwrap(); + extract_7z(&archive, &dir.join(format!("{name}.out"))).unwrap_err() +} + +// Property ids from the 7z header grammar, named as the format spec names them. +const K_END: u8 = 0x00; +const K_HEADER: u8 = 0x01; +const K_ADDITIONAL_STREAMS_INFO: u8 = 0x03; +const K_MAIN_STREAMS_INFO: u8 = 0x04; +const K_UNPACK_INFO: u8 = 0x07; +const K_FOLDER: u8 = 0x0B; + +#[test] +fn a_header_that_fails_its_own_checksum_is_described_in_words() { + let dir = tempfile::TempDir::new().unwrap(); + + // Two different checksums guard a 7z: the one over the start header (the + // `a_corrupt_header_...` test above trips that one) and the one the start + // header declares for the next header. They map to two different + // sentences, so collapsing the arms into one would go unnoticed without + // this: here the start header is valid and only its claim about the next + // header is wrong. + let err = extract_crafted( + dir.path(), + "lying.7z", + &[K_HEADER, K_END], + Some(0xDEAD_BEEF), + ); + + assert_eq!( + err.to_string(), + "Compression failed: the 7z archive is corrupt: its header failed the checksum" + ); +} + +#[test] +fn a_malformed_header_section_is_described_in_words() { + let dir = tempfile::TempDir::new().unwrap(); + + // The five `BadTerminated*` variants share one arm and one sentence, and + // nothing reached any of them. `0x42` is not a property id the parser + // expects at either position, which is what the variant carries; the + // sentence must not repeat it, because a raw property id tells a user + // nothing. + for (name, header) in [ + ("bent_header.7z", vec![K_HEADER, 0x42]), + ("bent_streams.7z", vec![K_HEADER, K_MAIN_STREAMS_INFO, 0x42]), + ] { + let message = extract_crafted(dir.path(), name, &header, None).to_string(); + assert_eq!( + message, "Compression failed: the 7z archive is corrupt: its header is malformed", + "{name}" + ); + assert!( + !message.contains("66") && !message.contains("0x42"), + "{name}: the message repeats the raw property id: {message}" + ); + } +} + +#[test] +fn an_archive_with_an_external_file_list_is_described_in_words() { + let dir = tempfile::TempDir::new().unwrap(); + + // Header, main streams info, unpack info, folder, zero folders, and then a + // non-zero "external" flag: the byte that means the file list lives in a + // stream of its own. It has its own hand-written sentence and its own arm, + // neither of which anything exercised. + let header = [ + K_HEADER, + K_MAIN_STREAMS_INFO, + K_UNPACK_INFO, + K_FOLDER, + 0x00, // number of folders + 0x01, // external != 0 + ]; + let err = extract_crafted(dir.path(), "external.7z", &header, None); + + assert_eq!( + err.to_string(), + "Compression failed: this 7z archive keeps its file list in an external stream, \ + which is not supported" + ); +} + +#[test] +fn a_sentence_the_dependency_wrote_travels_through_unchanged() { + let dir = tempfile::TempDir::new().unwrap(); + + // The `Other` arm passes its payload through, which is what keeps our own + // traversal message intact. The other half of that arm is the dependency's + // own prose, and this pins it: it must arrive as a sentence, not wrapped + // in `Other(...)` the way `to_string()` used to render it. + let err = extract_crafted( + dir.path(), + "extra_streams.7z", + &[K_HEADER, K_ADDITIONAL_STREAMS_INFO], + None, + ); + + assert_eq!( + err.to_string(), + "Compression failed: Additional streams unsupported" + ); +} + +// -- the property, across every failure the suite can provoke -- + +/// Every 7z failure these tests know how to cause, paired with a description +/// of what a user did to cause it. +/// +/// A per-variant test proves the mapping for the variant it names. This table +/// is the other half: the change's real promise is that *no* 7z failure can +/// show a `Debug` form or a path, and a promise about all of them needs a test +/// over all of them, so a newly mapped arm that stringifies is caught even +/// before anyone writes its own test. +/// +/// Only failures that go through the 7z backend belong here. `walk_tree`'s +/// "Not a directory: " is deliberately excluded: it never reaches the +/// dependency, and it names the path on purpose (it is echoing the argument +/// the caller just passed). Its own test pins it. +fn every_provokable_failure(dir: &Path) -> Vec<(&'static str, CompressionError)> { + let src = source_file(dir); + let tree = sample_tree(dir); + let missing = dir.join("does_not_exist"); + let occupied = dir.join("already_a_file"); + std::fs::write(&occupied, b"in the way").unwrap(); + + let not_an_archive = dir.join("plain.7z"); + std::fs::write(¬_an_archive, b"this is plain text, not an archive").unwrap(); + + let from_the_future = dir.join("future.7z"); + let mut future_bytes = SEVENZ_SIGNATURE.to_vec(); + future_bytes.extend_from_slice(&[9, 4]); + std::fs::write(&from_the_future, &future_bytes).unwrap(); + + let bad_start_header = dir.join("bent_start.7z"); + let mut bent = SEVENZ_SIGNATURE.to_vec(); + bent.extend_from_slice(&[0, 0]); + bent.extend_from_slice(&0xDEAD_BEEFu32.to_le_bytes()); + bent.extend_from_slice(&[7u8; 20]); + std::fs::write(&bad_start_header, &bent).unwrap(); + + let half = dir.join("half.7z"); + compress_7z(&src, &half, "sample.txt", 3).unwrap(); + let whole = std::fs::read(&half).unwrap(); + std::fs::write(&half, &whole[..whole.len() / 2]).unwrap(); + + let hostile = dir.join("hostile.7z"); + { + let mut writer = SevenZWriter::create(&hostile).unwrap(); + let entry = SevenZArchiveEntry { + name: "../escape.txt".to_string(), + ..Default::default() + }; + writer + .push_archive_entry(entry, Some(b"pwned".as_slice())) + .unwrap(); + writer.finish().unwrap(); + } + + // Not `mut` on a platform without /dev/full; see the push below. + #[allow(unused_mut)] + let mut failures = vec![ + ( + "compressing a file into a directory that is not there", + compress_7z(&src, &missing.join("out.7z"), "sample.txt", 3).unwrap_err(), + ), + ( + "compressing a tree into a directory that is not there", + compress_7z_dir(&tree, &missing.join("photos.7z"), 3).unwrap_err(), + ), + ( + "compressing a file that is not there", + compress_7z(&missing, &dir.join("out.7z"), "sample.txt", 3).unwrap_err(), + ), + ( + "compressing onto a path that is already a directory", + compress_7z(&src, &tree, "sample.txt", 3).unwrap_err(), + ), + ( + "extracting an archive that is not there", + extract_7z(&missing.join("nope.7z"), &dir.join("out_missing")).unwrap_err(), + ), + ( + "extracting into a path that is already a file", + extract_7z(&half, &occupied).unwrap_err(), + ), + ( + "extracting a file that is not an archive", + extract_7z(¬_an_archive, &dir.join("out_plain")).unwrap_err(), + ), + ( + "extracting an archive from a newer format version", + extract_7z(&from_the_future, &dir.join("out_future")).unwrap_err(), + ), + ( + "extracting an archive whose start header is corrupt", + extract_7z(&bad_start_header, &dir.join("out_bent")).unwrap_err(), + ), + ( + "extracting an archive that lies about its header checksum", + extract_crafted(dir, "lying.7z", &[K_HEADER, K_END], Some(0xDEAD_BEEF)), + ), + ( + "extracting an archive whose header section is malformed", + extract_crafted(dir, "bent_header.7z", &[K_HEADER, 0x42], None), + ), + ( + "extracting an archive whose streams section is malformed", + extract_crafted( + dir, + "bent_streams.7z", + &[K_HEADER, K_MAIN_STREAMS_INFO, 0x42], + None, + ), + ), + ( + "extracting an archive with no header at all", + extract_crafted(dir, "headerless.7z", &[0x42], None), + ), + ( + "extracting an archive with additional streams", + extract_crafted( + dir, + "extra_streams.7z", + &[K_HEADER, K_ADDITIONAL_STREAMS_INFO], + None, + ), + ), + ( + "extracting an archive with an external file list", + extract_crafted( + dir, + "external.7z", + &[ + K_HEADER, + K_MAIN_STREAMS_INFO, + K_UNPACK_INFO, + K_FOLDER, + 0x00, + 0x01, + ], + None, + ), + ), + ( + "extracting an archive that stops halfway", + extract_7z(&half, &dir.join("out_half")).unwrap_err(), + ), + ( + "extracting an archive whose entry name escapes the output", + extract_7z(&hostile, &dir.join("out_hostile")).unwrap_err(), + ), + ( + "extracting an archive with a flipped bit in an entry's data", + extract_7z(&bitrotted_archive(dir), &dir.join("out_bitrot")).unwrap_err(), + ), + ]; + + // The only failure that reaches `push_archive_entry`'s mapping, where the + // packed stream fails to write. Provoking it needs a device that is + // permanently out of space, and only Linux has one, so this entry is + // skipped elsewhere rather than faked; CI runs on Linux, so the call site + // is still covered there. + #[cfg(target_os = "linux")] + { + let always_full = Path::new("/dev/full"); + if always_full.exists() { + failures.push(( + "compressing onto a device with no space left", + compress_7z(&src, always_full, "sample.txt", 3).unwrap_err(), + )); + } + } + + failures +} + +#[test] +fn no_seven_z_failure_shows_a_debug_dump_or_a_path() { + let dir = tempfile::TempDir::new().unwrap(); + let failures = every_provokable_failure(dir.path()); + assert!( + failures.len() >= 17, + "the table was gutted; this test only means something if it is broad" + ); + + // Everything the tests touch lives under the temp dir, so its own path is + // the exact string a leaked path would contain. On macOS `TempDir` hands + // back `/var/...` while the dependency would report the resolved + // `/private/var/...`, so both spellings are checked. + let here = dir.path().to_string_lossy().to_string(); + let resolved = dir + .path() + .canonicalize() + .unwrap() + .to_string_lossy() + .to_string(); + + for (what, err) in failures { + let message = err.to_string(); + + // `sevenz_rust2::Error` implements `Display` as `Debug`, so these are + // the fingerprints of the dump the mapping replaced. `Io(` alone + // catches the old `Failed("Io(Os { .. }, \"/path\")")` spelling. + for fingerprint in [ + "Os {", "Error {", "Custom {", "kind:", "code:", "Error::", "Io(", "Other(", "\"", + ] { + assert!( + !message.contains(fingerprint), + "{what}: the message contains `{fingerprint}`, so it is a struct dump: {message}" + ); + } + + assert!( + !message.contains(&here) && !message.contains(&resolved), + "{what}: the message names a path on this machine: {message}" + ); + assert!( + !message.contains(".7z"), + "{what}: the message names a file, and the only file names here are ours: {message}" + ); + assert!( + !message.contains('\n'), + "{what}: the message spans several lines: {message}" + ); + + // Only two renderings are acceptable, and neither may be a bare + // prefix with nothing after it. + let body = message + .strip_prefix("IO error: ") + .or_else(|| message.strip_prefix("Compression failed: ")) + .unwrap_or_else(|| panic!("{what}: unexpected rendering: {message}")); + assert!(!body.trim().is_empty(), "{what}: empty message"); + } +} + +#[test] +fn the_mapping_keeps_the_two_kinds_apart() { + let dir = tempfile::TempDir::new().unwrap(); + let failures = every_provokable_failure(dir.path()); + + // A mapping that answered `Failed` to everything would still satisfy the + // "no dump" property above while throwing away the distinction the commit + // introduced, so check both kinds actually occur, and that the ones that + // land in `Io` really are IO problems rather than parse problems dressed + // up as one. + let (io, failed): (Vec<_>, Vec<_>) = failures + .iter() + .partition(|(_, err)| matches!(err, CompressionError::Io(_))); + assert!(io.len() >= 7, "failures stopped reaching Io: {io:?}"); + assert!( + failed.len() >= 10, + "failures stopped reaching Failed: {failed:?}" + ); + + for (what, err) in &io { + let CompressionError::Io(inner) = err else { + unreachable!() + }; + assert!( + inner.raw_os_error().is_some() || inner.kind() == std::io::ErrorKind::UnexpectedEof, + "{what}: `Io` should carry a real OS failure, got {inner:?}" + ); + } + + // Known defect, pinned rather than fixed: a truncated archive is a corrupt + // archive, but the dependency reports the short read as its `Io` variant + // and the mapping cannot tell that apart from a failing disk, so the user + // is told "IO error: failed to fill whole buffer". Layer 2 of issue #66 + // (a `#[source]` on `CompressionError`) is where that gets resolved. + let truncated = io + .iter() + .find(|(what, _)| what.contains("stops halfway")) + .expect("the truncated archive must still be in the table"); + assert_eq!( + truncated.1.to_string(), + "IO error: failed to fill whole buffer" + ); +} + +#[test] +fn a_compression_failure_reads_the_same_whether_it_is_a_file_or_a_tree() { + let dir = tempfile::TempDir::new().unwrap(); + let src = source_file(dir.path()); + let tree = sample_tree(dir.path()); + let occupied = dir.path().join("occupied"); + std::fs::create_dir(&occupied).unwrap(); + + // Writing onto a directory is the compression-side failure that reaches + // the dependency with a path in hand (`SevenZWriter::create` puts the + // whole output path in its error). Both entry points must drop it, and + // both must agree with each other. + let file_err = compress_7z(&src, &occupied, "sample.txt", 3).unwrap_err(); + let tree_err = compress_7z_dir(&tree, &occupied, 3).unwrap_err(); + + assert!(matches!(file_err, CompressionError::Io(_)), "{file_err:?}"); + assert!(matches!(tree_err, CompressionError::Io(_)), "{tree_err:?}"); + assert_eq!(file_err.to_string(), tree_err.to_string()); + for message in [file_err.to_string(), tree_err.to_string()] { + assert!( + !message.contains("occupied"), + "the message names the path the user picked: {message}" + ); + } + #[cfg(unix)] + assert_eq!( + file_err.to_string(), + "IO error: Is a directory (os error 21)" + ); +} + +/// The `finish()` calls that lost their `map_err` are the one part of the +/// change no test here reaches, and this records why rather than pretending. +/// +/// `SevenZWriter::finish` only fails when a write or a seek on the output +/// fails, and by then the file has been created, seeked and (for any entry +/// with content) written to successfully. Filling a real disk is the only way +/// in, and the obvious stand-in does not work: on `/dev/full` the dependency +/// panics inside `finish` (`writer.rs:388`, `position - SIGNATURE_HEADER_SIZE` +/// underflows because that device's `lseek` always answers 0) before it ever +/// reaches a failing write. So this test pins the successful path instead: an +/// archive of nothing but directories writes no entry bytes at all, which +/// makes `finish` the only thing that touches the output. +#[test] +fn finishing_an_archive_of_only_directories_writes_the_whole_file() { + let dir = tempfile::TempDir::new().unwrap(); + let root = dir.path().join("photos"); + std::fs::create_dir_all(root.join("a").join("b")).unwrap(); + let archive = dir.path().join("photos.7z"); + + compress_7z_dir(&root, &archive, 3).unwrap(); + + let written = std::fs::read(&archive).unwrap(); + assert!( + written.starts_with(&SEVENZ_SIGNATURE), + "finish never rewrote the signature header" + ); + let out = dir.path().join("out"); + assert!(extract_7z(&archive, &out).unwrap().is_empty()); + assert!(out.join("photos/a/b").is_dir()); +} + +/// Writing an entry's packed stream is the second compression-side call into +/// the mapping, and a device that is permanently out of space is the only +/// portable way to make it fail. Linux only: macOS has no `/dev/full`, so this +/// runs in CI (Linux runners) and is skipped on a developer's Mac. +#[cfg(target_os = "linux")] +#[test] +fn a_write_that_fails_mid_entry_is_reported_as_an_io_error() { + let always_full = Path::new("/dev/full"); + if !always_full.exists() { + return; + } + let dir = tempfile::TempDir::new().unwrap(); + let src = dir.path().join("big.txt"); + std::fs::write(&src, vec![b'x'; 100_000]).unwrap(); + + let err = compress_7z(&src, always_full, "big.txt", 3).unwrap_err(); + + // The dependency wraps this one as `Io(err, "Encode entry:big.txt")`, so + // `to_string()` used to render the whole struct, entry name included. + assert!( + matches!(err, CompressionError::Io(ref io) if io.raw_os_error() == Some(28)), + "expected an ENOSPC Io, got {err:?}" + ); + assert_eq!( + err.to_string(), + "IO error: No space left on device (os error 28)" + ); + assert!(!err.to_string().contains("big.txt"), "{err}"); } diff --git a/apps/core/tests/verify.rs b/apps/core/tests/verify.rs new file mode 100644 index 0000000..3ea277b --- /dev/null +++ b/apps/core/tests/verify.rs @@ -0,0 +1,837 @@ +//! Tests for what happens around a compression: the archive is staged beside +//! its destination and checked before it is allowed to land there. +//! +//! The failure this exists for (issue #70) is not that a compression can fail, +//! it is what a failed compression used to leave behind. zip and tar finalise +//! on drop, so a run that died partway through still closed out a *valid* +//! archive at the destination, silently missing entries: the user saw an error, +//! opened the archive, found it opened fine, and could then delete the +//! originals. `the_old_way_left_a_tar_missing_an_entry` and +//! `the_old_way_left_a_zip_no_check_can_fault` below are that bug, reproduced +//! against the backends, which still write straight to the path they are given. + +use std::fs; +use std::path::{Path, PathBuf}; + +use collapse_core::compression::{ + compress_7z_dir, compress_tar_dir, compress_zip_dir, extract_tar, extract_zip, verify_archive, +}; +use collapse_core::{compress, compress_dir, Algorithm, CompressionError, Verify}; + +const FORMATS: [Algorithm; 3] = [Algorithm::SevenZ, Algorithm::Tar, Algorithm::Zip]; + +/// Eight kilobytes deflate and LZMA2 cannot shrink, so the archive's data +/// region is about as long as the input and a byte flipped in the middle of the +/// file is certainly inside it rather than in a header. +fn incompressible(len: usize) -> Vec { + (0..len) + .map(|i| ((i as u64).wrapping_mul(2_654_435_761) >> 13) as u8) + .collect() +} + +/// `/data` holding `a.txt` and `b.txt`, plus an empty subdirectory and +/// an empty file: the two shapes an entry listing is most likely to lose. +fn sample_tree(parent: &Path) -> PathBuf { + let root = parent.join("data"); + fs::create_dir_all(root.join("empty_dir")).unwrap(); + fs::write(root.join("a.txt"), b"alpha").unwrap(); + fs::write(root.join("b.txt"), b"beta").unwrap(); + fs::write(root.join("empty.txt"), b"").unwrap(); + root +} + +/// Every entry `compress_dir` is meant to put in an archive of `sample_tree`. +fn sample_tree_entries() -> Vec { + [ + "data", + "data/a.txt", + "data/b.txt", + "data/empty.txt", + "data/empty_dir", + ] + .iter() + .map(|s| s.to_string()) + .collect() +} + +fn names_in(dir: &Path) -> Vec { + let mut names: Vec = fs::read_dir(dir) + .unwrap() + .map(|e| e.unwrap().file_name().to_string_lossy().to_string()) + .collect(); + names.sort(); + names +} + +/// Whatever the staging file is called, it says `collapse-part` so a leftover +/// is identifiable; that is the string this asserts on. +fn staging_leftovers(dir: &Path) -> Vec { + names_in(dir) + .into_iter() + .filter(|n| n.contains("collapse-part")) + .collect() +} + +fn flip_byte(path: &Path, offset: usize) { + let mut bytes = fs::read(path).unwrap(); + bytes[offset] ^= 0xFF; + fs::write(path, &bytes).unwrap(); +} + +// -- the reproduction from issue #70 -- + +/// A tree with one member nobody can read: the compressor gets partway through +/// and fails. This is the case the whole change is about. +#[cfg(unix)] +fn tree_with_an_unreadable_member(parent: &Path) -> Option { + use std::os::unix::fs::PermissionsExt; + + let root = parent.join("data"); + fs::create_dir_all(&root).unwrap(); + // Sorted first, so it is archived before the failure: what makes the + // leftover a *plausible* archive rather than an empty one. + fs::write(root.join("a.txt"), b"alpha").unwrap(); + let locked = root.join("b.txt"); + fs::write(&locked, b"beta").unwrap(); + fs::set_permissions(&locked, fs::Permissions::from_mode(0o000)).unwrap(); + + // Root reads a 0o000 file happily, so in a container this provokes nothing + // at all. Say so rather than assert something untrue. + if fs::File::open(&locked).is_ok() { + eprintln!("skipped: this user can read a 0o000 file, so nothing fails"); + return None; + } + Some(root) +} + +/// The bug itself, against the two backends that finalise on drop. They still +/// write straight to the path they are handed, so what is left at the +/// destination opens cleanly and is short by the entry that never made it. Only +/// comparing it against what was asked for can tell. +/// +/// Delete `verify_archive`'s set comparison and the last assertion fails. +#[cfg(unix)] +#[test] +fn the_old_way_left_a_valid_archive_missing_an_entry() { + let expected = vec![ + "data".to_string(), + "data/a.txt".to_string(), + "data/b.txt".to_string(), + ]; + + for (algorithm, archive_name) in [(Algorithm::Tar, "raw.tar"), (Algorithm::Zip, "raw.zip")] { + let dir = tempfile::TempDir::new().unwrap(); + let Some(root) = tree_with_an_unreadable_member(dir.path()) else { + return; + }; + let archive = dir.path().join(archive_name); + + let err = match algorithm { + Algorithm::Tar => compress_tar_dir(&root, &archive), + _ => compress_zip_dir(&root, &archive, 3), + } + .unwrap_err(); + assert!( + matches!(err, CompressionError::Io(_)), + "{algorithm}: expected the unreadable member to surface as IO, got {err:?}" + ); + + // The archive is there, and it opens. That is the whole problem. + assert!(archive.exists(), "{algorithm}: nothing was left behind"); + let out = dir.path().join("out"); + let listed = match algorithm { + Algorithm::Tar => extract_tar(&archive, &out), + _ => extract_zip(&archive, &out), + } + .unwrap_or_else(|e| panic!("{algorithm}: the leftover did not even open: {e}")); + assert_eq!( + listed, + vec!["data/a.txt".to_string()], + "{algorithm}: the leftover holds only the member read before the failure" + ); + + // And this is what catches it. + let err = verify_archive(&archive, algorithm, &expected, Verify::Index).unwrap_err(); + assert!( + err.to_string().contains("data/b.txt"), + "{algorithm}: the failure must name the entry that is gone: {err}" + ); + } +} + +/// `compress_zip_dir` used to start the entry and *then* read the file, so a +/// member it could not read still got its name into the archive with nothing +/// behind it, and the CRC written for it was the CRC of nothing. That leftover +/// named every entry that was asked for and was entirely self-consistent, so no +/// depth of checking could fault it: an empty file is a legitimate thing for an +/// archive to hold. +/// +/// Put the read back after `start_file` and this fails, on an archive that +/// claims to hold `data/b.txt` and holds nothing under that name. +#[cfg(unix)] +#[test] +fn a_member_that_cannot_be_read_is_not_named_in_the_archive_anyway() { + let dir = tempfile::TempDir::new().unwrap(); + let Some(root) = tree_with_an_unreadable_member(dir.path()) else { + return; + }; + let archive = dir.path().join("raw.zip"); + + compress_zip_dir(&root, &archive, 3).unwrap_err(); + + let out = dir.path().join("out"); + let listed = extract_zip(&archive, &out).expect("the leftover opens like any other zip"); + assert_eq!( + listed, + vec!["data/a.txt".to_string()], + "the member it failed on must not appear at all" + ); + assert!(!out.join("data/b.txt").exists()); +} + +/// The same failure through the dispatchers: nothing at all at the output path, +/// for every format. +/// +/// Write the archive straight to `output` again and this fails on `exists()`. +#[cfg(unix)] +#[test] +fn a_failed_compression_leaves_nothing_at_the_output_path() { + for algorithm in FORMATS { + let dir = tempfile::TempDir::new().unwrap(); + let Some(root) = tree_with_an_unreadable_member(dir.path()) else { + return; + }; + let output = dir.path().join(format!("data.{}", algorithm.extension())); + + let err = compress_dir(&root, &output, algorithm, 3, Verify::Index).unwrap_err(); + + assert!( + matches!(err, CompressionError::Io(_)), + "{algorithm}: expected an IO failure, got {err:?}" + ); + assert!( + !output.exists(), + "{algorithm}: a partial archive was left at {}", + output.display() + ); + } +} + +/// Nor anything half-written anywhere near it. `StagedOutput` is a guard rather +/// than a pair of calls precisely so the `?` above cannot skip the cleanup. +#[cfg(unix)] +#[test] +fn a_failed_compression_leaves_no_staging_file_behind() { + for algorithm in FORMATS { + let dir = tempfile::TempDir::new().unwrap(); + let Some(root) = tree_with_an_unreadable_member(dir.path()) else { + return; + }; + let output = dir.path().join(format!("data.{}", algorithm.extension())); + + compress_dir(&root, &output, algorithm, 3, Verify::Index).unwrap_err(); + + assert_eq!( + staging_leftovers(dir.path()), + Vec::::new(), + "{algorithm}: a staging file survived the failure" + ); + } +} + +/// The other half of "nothing bad is ever visible at the destination": an +/// archive already sitting there is still the archive that was there. +/// +/// This is what a local run could not promise and a remote one always could, +/// since a remote failure never got as far as writing the file. +#[cfg(unix)] +#[test] +fn a_failed_compression_leaves_the_previous_archive_untouched() { + for algorithm in FORMATS { + let dir = tempfile::TempDir::new().unwrap(); + let Some(root) = tree_with_an_unreadable_member(dir.path()) else { + return; + }; + let output = dir.path().join(format!("data.{}", algorithm.extension())); + let previous = b"last week's archive, such as it is"; + fs::write(&output, previous).unwrap(); + + compress_dir(&root, &output, algorithm, 3, Verify::Index).unwrap_err(); + + assert_eq!( + fs::read(&output).unwrap(), + previous, + "{algorithm}: the archive that was already there was destroyed" + ); + } +} + +/// The single-file entry point stages too. A source it cannot open is the +/// easiest way to fail it, and `compress_zip` in particular used to create the +/// output first and leave a zero-byte `.zip` sitting there. +#[test] +fn a_failed_single_file_compression_leaves_nothing_behind() { + for algorithm in FORMATS { + let dir = tempfile::TempDir::new().unwrap(); + let output = dir.path().join(format!("out.{}", algorithm.extension())); + + compress( + &dir.path().join("ghost.txt"), + &output, + "ghost.txt", + algorithm, + 3, + Verify::Index, + ) + .unwrap_err(); + + assert!( + !output.exists(), + "{algorithm}: a stub archive was published" + ); + assert_eq!( + staging_leftovers(dir.path()), + Vec::::new(), + "{algorithm}: a staging file survived the failure" + ); + } +} + +/// A rename replaces a *name*. An output that happens to be a hardlink to +/// something else therefore stops being one, instead of the compressor writing +/// through the shared inode and taking the other name's content with it. +/// +/// This was pinned as a KNOWN LIMITATION in the desktop crate's +/// `replacing_an_output_writes_through_a_hardlink_to_it`, whose own comment +/// named staging and renaming as the fix. Write to `output` directly again and +/// this fails. +/// +/// Not `cfg(unix)`: NTFS hardlinks share their data the same way. +#[test] +fn replacing_an_output_no_longer_writes_through_a_hardlink_to_it() { + let dir = tempfile::TempDir::new().unwrap(); + let src = dir.path().join("notes.txt"); + fs::write(&src, b"hello").unwrap(); + let output = dir.path().join("out.zip"); + let older = b"an older archive the user agreed to replace"; + fs::write(&output, older).unwrap(); + let bystander = dir.path().join("someone-elses-copy.zip"); + fs::hard_link(&output, &bystander).unwrap(); + + compress(&src, &output, "notes.txt", Algorithm::Zip, 3, Verify::Index).unwrap(); + + assert_eq!( + fs::read(&bystander).unwrap(), + older, + "the other name for that file was overwritten" + ); + // And the archive the caller asked for is really there. + let out = dir.path().join("extracted"); + assert_eq!(extract_zip(&output, &out).unwrap(), vec!["notes.txt"]); +} + +/// A run that works leaves the archive and nothing else. Forget to set +/// `committed`, or rename by copying, and this finds the leftover. +#[test] +fn a_successful_compression_leaves_only_the_archive() { + for algorithm in FORMATS { + let dir = tempfile::TempDir::new().unwrap(); + let root = sample_tree(dir.path()); + let output = dir.path().join(format!("data.{}", algorithm.extension())); + + compress_dir(&root, &output, algorithm, 3, Verify::Index).unwrap(); + + assert!(output.exists(), "{algorithm}: no archive was produced"); + assert_eq!( + staging_leftovers(dir.path()), + Vec::::new(), + "{algorithm}: a staging file survived a successful run" + ); + } +} + +/// The staging name is the output's name plus a suffix, and a file name has a +/// length limit of its own (255 bytes here). Drop the truncation in +/// `keep_bytes` and this fails with `File name too long` on an output name that +/// is perfectly legal. +/// +/// Unix only: Windows limits the whole path rather than one component, so the +/// same test there would be measuring the temporary directory's depth. +#[cfg(unix)] +#[test] +fn a_long_output_name_still_compresses() { + let dir = tempfile::TempDir::new().unwrap(); + let root = sample_tree(dir.path()); + let output = dir.path().join(format!("{}.zip", "n".repeat(250))); + + compress_dir(&root, &output, Algorithm::Zip, 1, Verify::Index) + .unwrap_or_else(|e| panic!("a 254 byte output name should be fine: {e}")); + + assert!(output.exists()); +} + +// -- Verify::Index -- + +/// Every format the crate writes, read back and compared against what +/// `compress_dir` was asked to put in. Directory entries are spelled `data/` by +/// zip and tar and `data` by 7z, so a comparison that did not normalize that +/// away would fail here for two formats out of three. +#[test] +fn index_accepts_what_the_dispatcher_just_wrote() { + for algorithm in FORMATS { + let dir = tempfile::TempDir::new().unwrap(); + let root = sample_tree(dir.path()); + let archive = dir.path().join(format!("data.{}", algorithm.extension())); + compress_dir(&root, &archive, algorithm, 3, Verify::Index).unwrap(); + + verify_archive(&archive, algorithm, &sample_tree_entries(), Verify::Index) + .unwrap_or_else(|e| panic!("{algorithm}: a freshly written archive was refused: {e}")); + } +} + +/// The single-file entry point puts exactly one named entry in the archive. +#[test] +fn index_accepts_a_single_file_archive() { + for algorithm in FORMATS { + let dir = tempfile::TempDir::new().unwrap(); + let src = dir.path().join("input.txt"); + fs::write(&src, b"just the one").unwrap(); + let archive = dir.path().join(format!("out.{}", algorithm.extension())); + + compress(&src, &archive, "renamed.dat", algorithm, 3, Verify::Index).unwrap(); + + verify_archive( + &archive, + algorithm, + &["renamed.dat".to_string()], + Verify::Index, + ) + .unwrap_or_else(|e| panic!("{algorithm}: {e}")); + } +} + +/// A count is not an answer: the message has to say which entry is gone, or +/// nobody can tell whether the archive is worth keeping. +#[test] +fn index_names_the_entries_that_are_missing() { + for algorithm in FORMATS { + let dir = tempfile::TempDir::new().unwrap(); + let root = sample_tree(dir.path()); + let archive = dir.path().join(format!("data.{}", algorithm.extension())); + compress_dir(&root, &archive, algorithm, 3, Verify::Index).unwrap(); + + let mut expected = sample_tree_entries(); + expected.push("data/holidays.jpg".to_string()); + + let err = verify_archive(&archive, algorithm, &expected, Verify::Index).unwrap_err(); + + assert_eq!( + err.to_string(), + format!( + "Verification of {} failed: 1 entry is missing: \"data/holidays.jpg\"", + archive.display() + ), + "{algorithm}" + ); + } +} + +/// The other direction, which is a different bug: an archive holding something +/// nobody asked for. +#[test] +fn index_names_the_entries_that_should_not_be_there() { + let dir = tempfile::TempDir::new().unwrap(); + let root = sample_tree(dir.path()); + let archive = dir.path().join("data.zip"); + compress_dir(&root, &archive, Algorithm::Zip, 3, Verify::Index).unwrap(); + + let expected: Vec = sample_tree_entries() + .into_iter() + .filter(|n| n != "data/b.txt" && n != "data/empty.txt") + .collect(); + + let err = verify_archive(&archive, Algorithm::Zip, &expected, Verify::Index).unwrap_err(); + + assert_eq!( + err.to_string(), + format!( + "Verification of {} failed: 2 entries are unexpected: \ + \"data/b.txt\", \"data/empty.txt\"", + archive.display() + ) + ); +} + +/// A tree of ten thousand files must not produce a ten thousand name message. +#[test] +fn a_long_list_of_missing_entries_is_cut_short() { + let dir = tempfile::TempDir::new().unwrap(); + let root = sample_tree(dir.path()); + let archive = dir.path().join("data.tar"); + compress_dir(&root, &archive, Algorithm::Tar, 1, Verify::Index).unwrap(); + + let mut expected = sample_tree_entries(); + for i in 0..8 { + expected.push(format!("data/ghost{i}.txt")); + } + + let err = verify_archive(&archive, Algorithm::Tar, &expected, Verify::Index).unwrap_err(); + + assert_eq!( + err.to_string(), + format!( + "Verification of {} failed: 8 entries are missing: \"data/ghost0.txt\", \ + \"data/ghost1.txt\", \"data/ghost2.txt\", \"data/ghost3.txt\", \ + \"data/ghost4.txt\" and 3 more", + archive.display() + ) + ); +} + +/// A caller has to be able to tell "the archive I just wrote is not right" from +/// "the compressor errored", and a `Failed(String)` cannot be told apart from +/// any other `Failed(String)`. +#[test] +fn a_verification_failure_is_its_own_kind_of_error() { + let dir = tempfile::TempDir::new().unwrap(); + let root = sample_tree(dir.path()); + let archive = dir.path().join("data.zip"); + compress_dir(&root, &archive, Algorithm::Zip, 3, Verify::Index).unwrap(); + + let err = verify_archive( + &archive, + Algorithm::Zip, + &["nothing/like/it".to_string()], + Verify::Index, + ) + .unwrap_err(); + + let CompressionError::VerificationFailed { + archive: named, + reason, + } = &err + else { + panic!("expected VerificationFailed, got {err:?}"); + }; + assert_eq!(named, &archive, "the error must name the archive"); + assert!( + reason.contains("nothing/like/it") && reason.contains("unexpected"), + "the error must say what was wrong: {reason}" + ); +} + +/// Reading a file that is not an archive at all is a verification failure too, +/// not an IO error and not a compression error: the compressor was fine, what +/// it produced is not readable. +#[test] +fn an_unreadable_archive_is_a_verification_failure() { + let dir = tempfile::TempDir::new().unwrap(); + let junk = dir.path().join("junk.zip"); + fs::write(&junk, b"this was never an archive").unwrap(); + + let err = + verify_archive(&junk, Algorithm::Zip, &["a.txt".to_string()], Verify::Index).unwrap_err(); + + assert!( + matches!(err, CompressionError::VerificationFailed { .. }), + "got {err:?}" + ); + assert!( + err.to_string() + .contains("the archive could not be read back"), + "{err}" + ); + // Not "Compression failed: ..." with the reader's words pasted in: that + // prefix would blame the compressor for a file it wrote correctly. + assert!(err.to_string().starts_with("Verification of "), "{err}"); +} + +// -- Verify::Contents -- + +#[test] +fn contents_accepts_what_the_dispatcher_just_wrote() { + for algorithm in FORMATS { + let dir = tempfile::TempDir::new().unwrap(); + let root = sample_tree(dir.path()); + let archive = dir.path().join(format!("data.{}", algorithm.extension())); + + compress_dir(&root, &archive, algorithm, 3, Verify::Contents) + .unwrap_or_else(|e| panic!("{algorithm}: a good tree was refused: {e}")); + + verify_archive( + &archive, + algorithm, + &sample_tree_entries(), + Verify::Contents, + ) + .unwrap_or_else(|e| panic!("{algorithm}: {e}")); + } +} + +/// Decompressing to a sink means to a sink: the check needs no space of its own +/// and cannot itself put anything on disk. Extract to a scratch directory +/// instead and this finds it. +#[test] +fn contents_writes_nothing_to_disk() { + for algorithm in FORMATS { + let dir = tempfile::TempDir::new().unwrap(); + let root = sample_tree(dir.path()); + let archive = dir.path().join(format!("data.{}", algorithm.extension())); + compress_dir(&root, &archive, algorithm, 3, Verify::Index).unwrap(); + + let before = names_in(dir.path()); + verify_archive( + &archive, + algorithm, + &sample_tree_entries(), + Verify::Contents, + ) + .unwrap(); + + assert_eq!( + names_in(dir.path()), + before, + "{algorithm}: verification put something on disk" + ); + } +} + +/// The point of the deeper depth: zip and 7z both store a checksum per entry, +/// and only reading the entry back compares it. The index still lists every +/// name, so `Index` cannot see this and is not supposed to. +/// +/// Make `Contents` behave like `Index` and the second half fails. +#[test] +fn contents_catches_a_flipped_bit_that_index_cannot_see() { + for algorithm in [Algorithm::SevenZ, Algorithm::Zip] { + let dir = tempfile::TempDir::new().unwrap(); + let src = dir.path().join("input.bin"); + fs::write(&src, incompressible(8192)).unwrap(); + let archive = dir.path().join(format!("out.{}", algorithm.extension())); + compress(&src, &archive, "input.bin", algorithm, 1, Verify::Contents).unwrap(); + + // Halfway through the file. Both formats put the entry data first and + // their listing at the end, and the data does not compress, so this + // lands in the payload and leaves the listing intact. + let midpoint = fs::metadata(&archive).unwrap().len() as usize / 2; + flip_byte(&archive, midpoint); + + let expected = ["input.bin".to_string()]; + verify_archive(&archive, algorithm, &expected, Verify::Index).unwrap_or_else(|e| { + panic!("{algorithm}: the listing should be untouched, so Index should pass: {e}") + }); + let err = verify_archive(&archive, algorithm, &expected, Verify::Contents).unwrap_err(); + assert!( + matches!(err, CompressionError::VerificationFailed { .. }), + "{algorithm}: got {err:?}" + ); + assert!( + err.to_string().contains("input.bin"), + "{algorithm}: the failure should name the entry: {err}" + ); + } +} + +/// What tar can and cannot promise, written down as a test rather than as a +/// hopeful sentence in a doc comment. +/// +/// tar's only checksum is `cksum`, over the 512 byte header. There is nothing +/// covering an entry's data, so a bit flipped inside a member is invisible to +/// every reader there is, including this one. Saying `Contents` protects a tar +/// the way it protects a zip would be a lie, and this is the test that would +/// have to be deleted to tell it. +#[test] +fn contents_on_a_tar_cannot_see_a_flipped_bit_in_the_data() { + let dir = tempfile::TempDir::new().unwrap(); + let src = dir.path().join("input.bin"); + fs::write(&src, incompressible(8192)).unwrap(); + let archive = dir.path().join("out.tar"); + compress( + &src, + &archive, + "input.bin", + Algorithm::Tar, + 1, + Verify::Contents, + ) + .unwrap(); + + // Well past the 512 byte header, so this is the member's own data. + flip_byte(&archive, 512 + 4096); + + let expected = ["input.bin".to_string()]; + verify_archive(&archive, Algorithm::Tar, &expected, Verify::Contents) + .expect("tar stores no checksum over an entry's data, so this cannot be caught"); + + // And the damage is real: the extracted file is not the file that went in. + let out = dir.path().join("out"); + extract_tar(&archive, &out).unwrap(); + assert_ne!( + fs::read(out.join("input.bin")).unwrap(), + fs::read(&src).unwrap(), + "the flip did not change anything, so this test proves nothing" + ); +} + +/// The half tar does cover. Its header carries a checksum, so a bent header is +/// caught at both depths. +#[test] +fn a_bent_tar_header_is_caught() { + let dir = tempfile::TempDir::new().unwrap(); + let src = dir.path().join("input.bin"); + fs::write(&src, b"small enough").unwrap(); + let archive = dir.path().join("out.tar"); + compress( + &src, + &archive, + "input.bin", + Algorithm::Tar, + 1, + Verify::Index, + ) + .unwrap(); + + // Byte 4 is inside the name field, which the header checksum covers. + flip_byte(&archive, 4); + + for depth in [Verify::Index, Verify::Contents] { + let err = verify_archive(&archive, Algorithm::Tar, &["input.bin".to_string()], depth) + .unwrap_err(); + assert!( + matches!(err, CompressionError::VerificationFailed { .. }), + "{depth:?}: got {err:?}" + ); + } +} + +/// The other half tar does cover, and the one that matters for a compression +/// cut short: a member whose data stops before the header said it would. +#[test] +fn a_tar_member_cut_short_is_caught() { + let dir = tempfile::TempDir::new().unwrap(); + let src = dir.path().join("input.bin"); + fs::write(&src, incompressible(8192)).unwrap(); + let archive = dir.path().join("out.tar"); + compress( + &src, + &archive, + "input.bin", + Algorithm::Tar, + 1, + Verify::Index, + ) + .unwrap(); + + let whole = fs::read(&archive).unwrap(); + fs::write(&archive, &whole[..512 + 4096]).unwrap(); + + for depth in [Verify::Index, Verify::Contents] { + let err = verify_archive(&archive, Algorithm::Tar, &["input.bin".to_string()], depth) + .unwrap_err(); + assert!( + matches!(err, CompressionError::VerificationFailed { .. }), + "{depth:?}: got {err:?}" + ); + } +} + +// -- the dispatchers really do run the check -- + +/// tar drops a `.` component from an entry name, so asking for +/// `notes/./x.txt` produces an archive holding `notes/x.txt`. The compressor +/// reports success; the archive does not hold what was asked for; the +/// dispatcher must not pretend otherwise. +/// +/// This is the one failure that reaches the discard path through verification +/// rather than through the compressor, so it is what proves the dispatcher +/// calls the check at all, and cleans up after it when it fails. +#[test] +fn a_name_the_format_will_not_store_is_refused_and_discarded() { + let dir = tempfile::TempDir::new().unwrap(); + let src = dir.path().join("input.txt"); + fs::write(&src, b"whatever").unwrap(); + let output = dir.path().join("out.tar"); + + let err = compress( + &src, + &output, + "notes/./x.txt", + Algorithm::Tar, + 1, + Verify::Index, + ) + .unwrap_err(); + + assert!( + matches!(err, CompressionError::VerificationFailed { .. }), + "got {err:?}" + ); + // The error names the destination, not the temporary it was checked on: + // that file is deleted before this returns, and the caller never chose it. + assert_eq!( + err.to_string(), + format!( + "Verification of {} failed: 1 entry is missing: \"notes/./x.txt\"; \ + 1 entry is unexpected: \"notes/x.txt\"", + output.display() + ) + ); + assert!( + !output.exists(), + "the rejected archive was published anyway" + ); + assert_eq!( + staging_leftovers(dir.path()), + Vec::::new(), + "the rejected archive was left staged" + ); +} + +/// The same, one level up: a rejected `compress_dir` leaves the archive that +/// was already there alone. +#[test] +fn a_rejected_archive_does_not_replace_an_older_one() { + let dir = tempfile::TempDir::new().unwrap(); + let src = dir.path().join("input.txt"); + fs::write(&src, b"whatever").unwrap(); + let output = dir.path().join("out.tar"); + let previous = b"the archive from before"; + fs::write(&output, previous).unwrap(); + + compress( + &src, + &output, + "notes/./x.txt", + Algorithm::Tar, + 1, + Verify::Index, + ) + .unwrap_err(); + + assert_eq!(fs::read(&output).unwrap(), previous); +} + +/// 7z has no `Drop` that finalises, so unlike zip and tar it never left a +/// readable-but-short archive; what it left was an unreadable stub. Recorded +/// here because it is the reason the reproduction above only covers two +/// formats, and because the dispatcher now removes it for all three either way. +#[cfg(unix)] +#[test] +fn a_failed_7z_leaves_a_stub_that_is_not_an_archive() { + let dir = tempfile::TempDir::new().unwrap(); + let Some(root) = tree_with_an_unreadable_member(dir.path()) else { + return; + }; + let archive = dir.path().join("raw.7z"); + + compress_7z_dir(&root, &archive, 3).unwrap_err(); + + assert!( + archive.exists(), + "the backend still writes where it is told" + ); + assert!( + verify_archive(&archive, Algorithm::SevenZ, &[], Verify::Index).is_err(), + "a 7z stub should not parse as an archive" + ); +} diff --git a/apps/core/tests/zip.rs b/apps/core/tests/zip.rs index 33a11c4..9d9ce7c 100644 --- a/apps/core/tests/zip.rs +++ b/apps/core/tests/zip.rs @@ -82,6 +82,30 @@ fn all_levels_produce_valid_zip() { } } +/// `compress_zip` used to call `File::create(output)` before opening the +/// source, so a source that could not be read left a zero-byte `.zip` sitting +/// where the archive should have been: a file the same CLI then refused to +/// extract, and one that a `--force` guard would later see as an archive worth +/// asking about. Reordering the two is what removed it. +/// +/// Reinstate the old order and this fails on `!archive.exists()`. +#[test] +fn a_source_that_cannot_be_read_leaves_no_stub_archive() { + let dir = tempfile::TempDir::new().unwrap(); + let archive = dir.path().join("out.zip"); + + let err = compress_zip(&dir.path().join("ghost.txt"), &archive, "ghost.txt", 1).unwrap_err(); + + assert!( + matches!(err, collapse_core::CompressionError::Io(ref io) if io.kind() == std::io::ErrorKind::NotFound), + "expected a NotFound Io, got {err:?}" + ); + assert!( + !archive.exists(), + "a zero-byte archive was left at the output path" + ); +} + // -- extract_zip tests -- #[test] diff --git a/apps/desktop/package-lock.json b/apps/desktop/package-lock.json index 2f83ca2..55bee7b 100644 --- a/apps/desktop/package-lock.json +++ b/apps/desktop/package-lock.json @@ -1,12 +1,12 @@ { "name": "collapse-desktop", - "version": "0.7.0", + "version": "0.8.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "collapse-desktop", - "version": "0.7.0", + "version": "0.8.0", "dependencies": { "@tauri-apps/api": "^2", "@tauri-apps/plugin-dialog": "^2", diff --git a/apps/desktop/package.json b/apps/desktop/package.json index 72162c6..0deb5d8 100644 --- a/apps/desktop/package.json +++ b/apps/desktop/package.json @@ -1,7 +1,7 @@ { "name": "collapse-desktop", "private": true, - "version": "0.7.0", + "version": "0.8.0", "type": "module", "scripts": { "dev": "vite", diff --git a/apps/desktop/src-tauri/Cargo.lock b/apps/desktop/src-tauri/Cargo.lock index b6c1a2c..273e5c1 100644 --- a/apps/desktop/src-tauri/Cargo.lock +++ b/apps/desktop/src-tauri/Cargo.lock @@ -516,7 +516,7 @@ checksum = "c8d4a3bb8b1e0c1050499d1815f5ab16d04f0959b233085fb31653fbfc9d98f9" [[package]] name = "collapse-core" -version = "0.7.0" +version = "0.8.0" dependencies = [ "same-file", "serde", @@ -528,7 +528,7 @@ dependencies = [ [[package]] name = "collapse-desktop" -version = "0.7.0" +version = "0.8.0" dependencies = [ "axum", "collapse-core", @@ -541,11 +541,12 @@ dependencies = [ "tauri-plugin-dialog", "tempfile", "tokio", + "zip", ] [[package]] name = "collapse-remote" -version = "0.7.0" +version = "0.8.0" dependencies = [ "collapse-core", "serde_json", @@ -556,7 +557,7 @@ dependencies = [ [[package]] name = "collapse-server-backend" -version = "0.7.0" +version = "0.8.0" dependencies = [ "axum", "clap", diff --git a/apps/desktop/src-tauri/Cargo.toml b/apps/desktop/src-tauri/Cargo.toml index 17b0c77..2a17caf 100644 --- a/apps/desktop/src-tauri/Cargo.toml +++ b/apps/desktop/src-tauri/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "collapse-desktop" -version = "0.7.0" +version = "0.8.0" edition = "2021" description = "Collapse — a small, fast file compressor" authors = ["cervantic"] @@ -38,6 +38,11 @@ collapse-server-backend = { path = "../../server-backend" } tempfile = "3" tokio = { version = "1", features = ["rt", "net"] } axum = "0.8" +# tests/names.rs crafts an archive holding an entry name this host cannot +# write, which no compression of a real directory could ever produce: the +# filesystem would have refused the file first. Same version core already +# builds with, so this adds nothing to the dependency tree. +zip = "2" [profile.release] codegen-units = 1 diff --git a/apps/desktop/src-tauri/src/commands.rs b/apps/desktop/src-tauri/src/commands.rs index fa86261..70f8d13 100644 --- a/apps/desktop/src-tauri/src/commands.rs +++ b/apps/desktop/src-tauri/src/commands.rs @@ -8,15 +8,40 @@ //! macro, and the integration tests in `tests/` drive these functions //! directly (a command is an ordinary function, and this crate carries no //! inline `mod tests`, like the rest of the workspace). +//! +//! **Anything that can take longer than an instant carries +//! `#[tauri::command(async)]`.** A bare `#[tauri::command]` on a synchronous +//! function compiles to what tauri-macros calls the `sync` path, which runs +//! the body inline on the thread handling the IPC message: the window stops +//! repainting for the duration, and the system eventually offers to force +//! quit. The attribute argument moves it to `sync_threadpool`, which hands the +//! call to `respond_async_serialized` and so to `async_runtime::spawn`. +//! +//! Worth knowing what that is and is not: it is the async runtime, a +//! multi-thread tokio with one worker per core, not the blocking pool, so the +//! body occupies a worker for its whole duration. That is fine here because +//! the UI runs one operation at a time and disables itself while it does, but +//! a caller that wanted several at once should move the work to +//! `spawn_blocking` rather than add more of these. +use std::collections::BTreeMap; use std::path::PathBuf; -use collapse_core::{compress, compress_dir, extract, Algorithm}; +use collapse_core::{ + compress, compress_dir, extract_with, Algorithm, CompressionError, ExtractOptions, NameRules, + Verify, +}; +use serde::Serialize; +use crate::names::{substitutions_from, NameInspection}; use crate::paths::{inside, same_file}; /// Whether a path points at a directory (used by the UI to pick the icon and /// the default archive name). +/// +/// The one command with no `async`: it is a single `stat`, and the UI calls it +/// while the user is still choosing, so an IPC round trip through the runtime +/// would cost more than the call. #[tauri::command] pub fn is_directory(path: String) -> bool { std::path::Path::new(&path).is_dir() @@ -26,16 +51,36 @@ pub fn is_directory(path: String) -> bool { /// /// With `server` set, the work happens on a remote Collapse instance instead: /// the bytes go out (a folder as a tar envelope), the archive comes back and -/// is written to the same `output` the local path would use. This command is -/// deliberately synchronous, so Tauri runs it on its blocking pool and the -/// window stays responsive while the server works. +/// is written to the same `output` the local path would use. That exchange has +/// no read timeout, so it can outlast any patience: all the more reason for +/// the `async` on the attribute below (see the module header). Only `None` +/// means "this computer"; a `Some` that holds a blank string is an error from +/// `collapse-remote`, not a quiet fallback to local. /// /// `overwrite` is the caller saying the user already agreed to replace what is /// at `output`, which is what the native save dialog asks on every platform. /// It is the `--force` of the CLI, and like it, it cannot buy past the two /// guards below: agreeing to replace a file is not agreeing to destroy the /// source, nor to destroy a file that is part of what is being archived. -#[tauri::command] +/// +/// `verify` picks between the two depths core checks a local archive at before +/// it is allowed to reach `output`. It is never "check or do not check": with +/// `false` the archive's own listing is read back and compared against the +/// entries it was meant to hold, which decompresses nothing and is what catches +/// the failure this exists for, a compression that died half way through and +/// finalised a valid-looking archive anyway. With `true` every entry is +/// decompressed as well, so zip's and 7z's per-entry checksums are checked; tar +/// stores no checksum over an entry's data at all, so there the deeper pass can +/// only confirm the archive is complete and well formed. It roughly doubles the +/// work, which is why it is the user's call and not the default. +/// +/// It says nothing about a run with a `server`. The archive is built over there +/// and arrives as bytes this app never described, so there is no list of +/// expected entries here to check it against. The UI disables the checkbox in +/// that case, and a caller that asks anyway gets the archive rather than a +/// refusal: nothing about the request is harmful, it is just not something this +/// side can do. +#[tauri::command(async)] pub fn compress_path( path: String, output: String, @@ -43,6 +88,7 @@ pub fn compress_path( level: u32, server: Option, overwrite: bool, + verify: bool, ) -> Result { let source = PathBuf::from(&path); if !source.exists() { @@ -82,28 +128,43 @@ pub fn compress_path( "The output already exists: {output}. Delete it first, or choose another name." )); } - // Deliberately NOT unlinked here. The write happens only once the - // archive is fully in hand (the remote path downloads it all before - // touching disk), so a failed run leaves the previous archive exactly - // as it was. Removing it up front would trade that away for nothing. + // Deliberately NOT unlinked here. Neither branch touches this path + // until the archive is whole: core writes a local archive to a + // temporary beside it and renames it in only once it passes its check, + // and the remote branch downloads every byte before it writes. So a + // failed run leaves the previous archive exactly as it was, and + // removing it up front would trade that away for nothing. } - match server.as_deref().filter(|s| !s.is_empty()) { + // Two depths, no "off": see the note on `verify` above. Unused by the + // remote arm below, which has nothing of its own to check. + let depth = if verify { + Verify::Contents + } else { + Verify::Index + }; + + // `Some(_)` is the caller asking for a server, whatever it put in the + // string: whether that string is usable is `collapse-remote`'s answer, + // not this app's. Filtering here is what let the two front-ends disagree + // (this one read `""` as "compress locally" and `" "` as a real + // destination, the CLI read both as a destination). "This computer" is + // `null` from the UI, which arrives as `None`. + match server.as_deref() { Some(server) => { let archive = collapse_remote::compress_path(server, &source, algorithm, level) .map_err(|e| e.to_string())?; std::fs::write(&output_path, archive).map_err(|e| e.to_string())?; } - None if source.is_dir() => { - compress_dir(&source, &output_path, algorithm, level).map_err(|e| e.to_string())? - } + None if source.is_dir() => compress_dir(&source, &output_path, algorithm, level, depth) + .map_err(|e| e.to_string())?, None => { let arcname = source .file_name() .ok_or_else(|| "Invalid source path.".to_string())? .to_string_lossy() .into_owned(); - compress(&source, &output_path, &arcname, algorithm, level) + compress(&source, &output_path, &arcname, algorithm, level, depth) .map_err(|e| e.to_string())?; } } @@ -113,18 +174,115 @@ pub fn compress_path( /// Check that a remote Collapse server is reachable, so a typo surfaces in /// the settings panel rather than at the end of an upload. -#[tauri::command] +#[tauri::command(async)] pub fn check_server(url: String) -> Result<(), String> { collapse_remote::check_health(&url).map_err(|e| e.to_string()) } -/// Extract an archive into `output_dir`, returning the extracted file paths. -#[tauri::command] -pub fn extract_archive(archive: String, output_dir: String) -> Result, String> { +/// What an archive holds that this machine cannot write as ordinary files, so +/// the UI can ask the user about it **before** anything is extracted. +/// +/// Reads the archive's listing and nothing else: no entry is decompressed and +/// nothing is created, which is what makes it safe to call the moment a +/// destination has been chosen. An empty answer (`entries` empty) means there +/// is no question to ask, which on macOS and Linux is nearly always the case: +/// the rules are **this host's**, and Unix refuses only the NUL byte, while +/// Windows refuses `? * < > | "`, control characters, a trailing dot or space +/// and the device names, and silently reinterprets a colon. +/// +/// An archive whose listing cannot be read reports nothing rather than +/// failing, and that is deliberate: [`extract_archive`] is about to open the +/// same file and fail on it in the extractor's own vocabulary ("Could not find +/// EOCD", "failed to unpack `x`"), which is the message the user needs. This +/// command answering first would replace it with a worse one. Core's +/// `extract_with` makes the same choice for the same reason. +#[tauri::command(async)] +pub fn unwritable_names(archive: String) -> Result { + let archive_path = PathBuf::from(&archive); + if !archive_path.exists() { + return Err(format!("Not found: {archive}")); + } + // Named rather than left to `ExtractOptions`' default so that the rules the + // dialog is built from are visibly the same ones `extract_archive` will + // judge the answers by. They must agree: a dialog that asked about a + // different alphabet than the extractor enforces would ask the wrong + // questions and then fail anyway. + let rules = NameRules::host(); + let report = collapse_core::unwritable_names_with(&archive_path, rules).unwrap_or_default(); + Ok(NameInspection::new(report, rules)) +} + +/// What came of an extraction attempt. +/// +/// Two arms because two things can come back that are not failures of the +/// machine: the archive was extracted, or **nothing was written** and the user +/// has another naming question to answer. An `Err` from [`extract_archive`] +/// stays what it always was, something the user cannot fix by typing (a +/// missing file, a corrupt archive, a full disk). +/// +/// Keeping the second case out of `Err` is what lets the dialog stay open on +/// the answer that needs changing while the error banner keeps meaning "this +/// did not work". The distinction is sound rather than hopeful: core raises +/// `CompressionError::Name` only while validating the answers and while +/// planning every entry's name from the listing, both of which happen before +/// the first byte is written. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +#[serde(tag = "status", rename_all = "camelCase")] +pub enum Extraction { + /// Written, under these names. Never the archive's names: an entry this + /// host had to be given a different name for is reported as it is on disk, + /// or the UI would list files nobody can find. + Extracted { files: Vec }, + /// Nothing was written, and here is what has to be answered differently. + NameProblem { message: String }, +} + +/// Extract an archive into `output_dir` with the user's answers for the names +/// this host cannot write. +/// +/// `replacements` maps one character to whatever should stand in for it, and +/// is empty for the ordinary archive that needs nothing. An empty *value* is a +/// real answer meaning "drop the character". The two adjustments no one can be +/// asked about (a trailing dot or space, a reserved device name) are applied by +/// core without appearing here; [`unwritable_names`] is what tells the user +/// they are coming. +/// +/// A `BTreeMap` rather than a `HashMap`: with two bad keys in one payload, the +/// message has to name the same one on every run. +#[tauri::command(async)] +pub fn extract_archive( + archive: String, + output_dir: String, + replacements: BTreeMap, +) -> Result { let archive_path = PathBuf::from(&archive); if !archive_path.exists() { return Err(format!("Not found: {archive}")); } let output = PathBuf::from(&output_dir); - extract(&archive_path, &output).map_err(|e| e.to_string()) + + let answers = match substitutions_from(&replacements) { + Ok(answers) => answers, + // A key that is not a single character is the webview's mistake rather + // than the user's, but it is still a naming question that wrote + // nothing, so it travels the same way and the dialog stays open. + Err(problem) => { + return Ok(Extraction::NameProblem { + message: problem.to_string(), + }) + } + }; + // The host's rules, spelled out rather than left to the default, because + // they have to be the same ones `unwritable_names` built the dialog from. + let options = ExtractOptions::new() + .with_rules(NameRules::host()) + .with_replacements(answers); + + match extract_with(&archive_path, &output, &options) { + Ok(files) => Ok(Extraction::Extracted { files }), + Err(CompressionError::Name(problem)) => Ok(Extraction::NameProblem { + message: problem.to_string(), + }), + Err(other) => Err(other.to_string()), + } } diff --git a/apps/desktop/src-tauri/src/lib.rs b/apps/desktop/src-tauri/src/lib.rs index 59ccc71..74b9c0d 100644 --- a/apps/desktop/src-tauri/src/lib.rs +++ b/apps/desktop/src-tauri/src/lib.rs @@ -1,8 +1,10 @@ //! Tauri backend for the Collapse desktop app: the app wiring only. The -//! commands themselves live in [`commands`] and the path predicates they rely -//! on in [`paths`], both public so `tests/` can drive them directly. +//! commands themselves live in [`commands`], the path predicates they rely on +//! in [`paths`] and the naming exchange the extract dialog needs in [`names`], +//! all public so `tests/` can drive them directly. pub mod commands; +pub mod names; pub mod paths; #[cfg_attr(mobile, tauri::mobile_entry_point)] @@ -14,6 +16,7 @@ pub fn run() { .invoke_handler(tauri::generate_handler![ commands::is_directory, commands::compress_path, + commands::unwritable_names, commands::extract_archive, commands::check_server ]) diff --git a/apps/desktop/src-tauri/src/names.rs b/apps/desktop/src-tauri/src/names.rs new file mode 100644 index 0000000..59f7ab4 --- /dev/null +++ b/apps/desktop/src-tauri/src/names.rs @@ -0,0 +1,94 @@ +//! The naming question, on its way to the webview and back. +//! +//! Extraction can no longer be a single call. An archive built on Linux may +//! hold names this machine cannot save as they are spelled, and issues #63 and +//! #64 settled what to do about it: ask. So the UI first asks what is wrong +//! ([`NameInspection`], from `collapse_core::unwritable_names_with`), puts one +//! text field on screen per offending character, and only then extracts with +//! the answers. +//! +//! This module is the shape of that exchange plus the two conversions it needs. +//! It deliberately holds **no rules of its own**: every judgement about what a +//! name may contain belongs to `collapse_core::NameRules`, and duplicating any +//! part of it here is how the webview and the extractor would come to disagree +//! about the same name. What crosses to the webview is the *ruleset's own +//! answer*, as data ([`NameInspection::rejected_in_replacement`]), so the +//! dialog can refuse a bad answer as it is typed without knowing why it is bad. + +use std::collections::BTreeMap; + +use collapse_core::{NameError, NameReport, NameRules, Substitutions}; +use serde::Serialize; + +/// Path separators, which no replacement may contain. +/// +/// Neither ruleset lists them (a `NameRules` judges one component of a path, so +/// a separator is never *in* a name it is asked about), yet +/// `NameRules::check_replacements` refuses them, because answering `?` with +/// `../` would move the entry to another directory rather than rename it. They +/// are therefore added to the set the dialog checks against, or the dialog +/// would accept an answer the extractor is about to reject. +const SEPARATORS: [char; 2] = ['/', '\\']; + +/// What an archive holds that this machine cannot write, and what the dialog +/// needs to ask about it. +/// +/// The report is flattened, so the webview sees one flat object +/// (`{ entries, characters, rejectedInReplacement }`) rather than a report +/// nested inside a wrapper: the JSON is the dialog's data model, and it has no +/// use for the seam between the two halves. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct NameInspection { + #[serde(flatten)] + pub report: NameReport, + /// Every character a replacement may not contain: everything this machine + /// cannot write, plus the two path separators. + /// + /// Sent as data rather than reimplemented in JavaScript. The dialog only + /// picks the wording; whether a character is acceptable is answered here by + /// the same ruleset the extraction will use, and the extraction checks + /// again regardless (`extract_with` validates the answers before it opens + /// the archive). + pub rejected_in_replacement: String, +} + +impl NameInspection { + pub fn new(report: NameReport, rules: NameRules) -> Self { + let mut rejected: Vec = rules + .offending_characters() + .map(|(character, _)| character) + .chain(SEPARATORS) + .collect(); + // Sorted and deduplicated so the string is the same on every run: it is + // pinned by a test, and a set that arrived in a different order each + // time would be untestable as well as unreadable. + rejected.sort_unstable(); + rejected.dedup(); + Self { + report, + rejected_in_replacement: rejected.into_iter().collect(), + } + } + + /// True when every name in the archive can be written as it stands, which + /// is when the dialog has nothing to ask. + pub fn is_empty(&self) -> bool { + self.report.is_empty() + } +} + +/// The webview's answers, turned into what core takes. +/// +/// A JSON object has no `char` keys, so each one arrives as a string and is +/// checked for being exactly one character. That check is core's +/// ([`Substitutions::set_str`]), not a second copy of it here. +pub fn substitutions_from( + replacements: &BTreeMap, +) -> Result { + let mut answers = Substitutions::new(); + for (key, replacement) in replacements { + answers.set_str(key, replacement.as_str())?; + } + Ok(answers) +} diff --git a/apps/desktop/src-tauri/tauri.conf.json b/apps/desktop/src-tauri/tauri.conf.json index 2fc8bfc..d264ccb 100644 --- a/apps/desktop/src-tauri/tauri.conf.json +++ b/apps/desktop/src-tauri/tauri.conf.json @@ -1,7 +1,7 @@ { "$schema": "https://schema.tauri.app/config/2", "productName": "Collapse", - "version": "0.7.0", + "version": "0.8.0", "identifier": "com.cervantic.collapse", "build": { "frontendDist": "../dist", @@ -15,7 +15,7 @@ "title": "Collapse", "label": "main", "width": 480, - "height": 520, + "height": 600, "minWidth": 440, "minHeight": 460, "resizable": true, diff --git a/apps/desktop/src-tauri/tests/commands.rs b/apps/desktop/src-tauri/tests/commands.rs index e1d8212..217dcda 100644 --- a/apps/desktop/src-tauri/tests/commands.rs +++ b/apps/desktop/src-tauri/tests/commands.rs @@ -3,22 +3,33 @@ //! //! A `#[tauri::command]` is an ordinary function, so these drive the real //! commands in-process and assert what lands on disk. Nothing here starts a -//! server; the remote branch of `compress_path` is `tests/remote.rs`'s job. +//! server; the remote branch of `compress_path` is `tests/remote.rs`'s job, +//! and `extract_archive`'s other half, the archive holding a name this host +//! cannot write, is `tests/names.rs`'s. //! //! Every command reports failure as a plain `String` (the backend errors are //! stringified with `.to_string()`, and `Algorithm`'s own `FromStr` already //! yields one), so the assertions match on the message, not on a variant. +use std::collections::BTreeMap; use std::fs; use std::path::{Path, PathBuf}; -use collapse_desktop::commands::{compress_path, extract_archive, is_directory}; +use collapse_desktop::commands::{compress_path, extract_archive, is_directory, Extraction}; use tempfile::TempDir; /// The three formats the UI offers. The wire spelling doubles as the archive /// extension for all of them, which is why one array serves both roles. const FORMATS: [&str; 3] = ["zip", "7z", "tar"]; +/// `RemoteError::BlankServer` rendered, which is the string `App.vue` puts in +/// its error banner. Spelled out whole so the tests below compare the sentence +/// a user reads, not a fragment of it: `apps/cli/tests/remote.rs` spells out +/// the same literal, and the two saying different things is the drift the +/// shared crate exists to prevent (issue #65). +const BLANK_ADDRESS: &str = + "the server address is blank: it needs a URL, for example http://localhost:8000"; + /// Names a real user produces and a tidy-ASCII test suite never would: a /// space, an accent, a `#`, a `%`, a leading dash (which a shell would read as /// a flag) and a non-Latin script. They cross three boundaries here: the @@ -39,7 +50,7 @@ fn compress_local( format: &str, level: u32, ) -> Result { - compress_local_with(source, output, format, level, false) + compress_local_with(source, output, format, level, false, false) } /// The same call with the caller reporting that the user agreed to replace @@ -51,7 +62,18 @@ fn compress_local_overwriting( format: &str, level: u32, ) -> Result { - compress_local_with(source, output, format, level, true) + compress_local_with(source, output, format, level, true, false) +} + +/// The same call with the Verify box ticked, which asks for the deeper of the +/// two checks: every entry decompressed rather than the listing alone. +fn compress_local_checking_contents( + source: &Path, + output: &Path, + format: &str, + level: u32, +) -> Result { + compress_local_with(source, output, format, level, false, true) } fn compress_local_with( @@ -60,6 +82,7 @@ fn compress_local_with( format: &str, level: u32, overwrite: bool, + verify: bool, ) -> Result { compress_path( source.to_string_lossy().into_owned(), @@ -68,14 +91,25 @@ fn compress_local_with( level, None, overwrite, + verify, ) } +/// Extract with no answers, which is every archive in this file: none of them +/// carries a name this host cannot write, so `Extraction` can only be the +/// `Extracted` arm and the naming question is `tests/names.rs`'s subject. fn extract_to(archive: &Path, output_dir: &Path) -> Result, String> { - extract_archive( + match extract_archive( archive.to_string_lossy().into_owned(), output_dir.to_string_lossy().into_owned(), - ) + BTreeMap::new(), + )? { + Extraction::Extracted { files } => Ok(files), + Extraction::NameProblem { message } => panic!( + "{} holds a name this host cannot write, which no fixture here intends: {message}", + archive.display() + ), + } } /// Normalize and sort an extracted listing so the expectations read the same @@ -484,6 +518,131 @@ fn the_level_reaches_the_backend_because_one_and_five_compress_differently() { ); } +// ------------------------------------------------------------- verification -- + +/// Every file an archive holds, with its bytes, so two runs can be compared by +/// what they really contain rather than by the size of the file they produced. +fn contents_of(archive: &Path, into: &Path) -> Vec<(String, Vec)> { + listing(extract_to(archive, into).expect("the archive extracts cleanly")) + .into_iter() + .map(|name| { + let bytes = fs::read(into.join(&name)).expect("an extracted file is readable"); + (name, bytes) + }) + .collect() +} + +/// The Verify checkbox, both ways, for both source shapes and every format. +/// +/// What this can prove from out here: ticking it is accepted rather than +/// refused, the archive it produces holds exactly what the unticked run's does, +/// and the staging the check runs on top of leaves nothing behind. What it +/// cannot prove is that `true` reaches core as the deeper depth: a compression +/// that succeeds looks identical at both depths by construction, since the +/// check only reads, and telling them apart needs an archive whose listing is +/// right and whose entry data is corrupt, which this command cannot be made to +/// produce. Core's own suite owns that half; here the parameter is held in +/// place by these three claims plus the frozen signature in tests/ipc.rs. +#[test] +fn checking_contents_is_accepted_and_yields_the_same_archive() { + for format in FORMATS { + let dir = TempDir::new().unwrap(); + let file = dir.path().join("notes.txt"); + fs::write(&file, prose(20_000)).unwrap(); + let tree = make_tree(dir.path()); + + for (shape, source) in [("file", &file), ("tree", &tree)] { + let mut by_depth = Vec::new(); + for (depth, checked) in [("listing", false), ("contents", true)] { + // A folder of its own for the archive, so "what is beside it" + // has exactly one right answer: nothing else writes here. + let destination = dir.path().join(format!("{format}-{shape}-{depth}")); + fs::create_dir(&destination).unwrap(); + let archive = destination.join(format!("out.{format}")); + + let call = if checked { + compress_local_checking_contents(source, &archive, format, 3) + } else { + compress_local(source, &archive, format, 3) + }; + call.unwrap_or_else(|e| panic!("{format} {shape}, {depth} check: {e}")); + + // The archive is built in a temporary beside the destination + // and renamed in, so one that outlived the run shows up here. + let mut beside: Vec = fs::read_dir(&destination) + .unwrap() + .map(|entry| entry.unwrap().file_name().to_string_lossy().into_owned()) + .collect(); + beside.sort(); + assert_eq!( + beside, + vec![format!("out.{format}")], + "{format} {shape}, {depth} check: something was left beside the archive" + ); + + by_depth.push(contents_of( + &archive, + &dir.path().join(format!("{format}-{shape}-{depth}-out")), + )); + } + + assert_eq!( + by_depth[0], by_depth[1], + "{format} {shape}: checking the contents changed what the archive holds" + ); + assert!( + !by_depth[0].is_empty(), + "{format} {shape}: the comparison above passed on two empty archives" + ); + } + } +} + +#[test] +fn the_data_loss_guards_hold_whatever_the_verify_box_says() { + // Checking happens after an archive exists; the guards happen before + // anything is read or written. Ticking the box must not reorder that, or + // the app would destroy a source and then carefully verify the archive it + // made out of it. + for format in FORMATS { + let dir = TempDir::new().unwrap(); + let source = dir.path().join("notes.txt"); + fs::write(&source, b"irreplaceable").unwrap(); + + let err = compress_local_checking_contents(&source, &source, format, 3).unwrap_err(); + + assert_eq!( + err, "The output is the same file as the source.", + "{format}" + ); + assert_eq!( + fs::read(&source).unwrap(), + b"irreplaceable", + "{format}: the source was modified" + ); + } + + // The other irreversible one, with consent given as well, since that is + // the combination the containment guard exists to refuse. + let dir = TempDir::new().unwrap(); + let root = dir.path().join("photos"); + fs::create_dir_all(&root).unwrap(); + let victim = root.join("a.txt"); + fs::write(&victim, b"irreplaceable member").unwrap(); + + let err = compress_local_with(&root, &victim, "zip", 3, true, true).unwrap_err(); + + assert_eq!( + err, + format!( + "The output is inside the folder being compressed: {}. \ + It would be destroyed instead of archived. Choose a location outside it.", + victim.display() + ) + ); + assert_eq!(fs::read(&victim).unwrap(), b"irreplaceable member"); +} + #[test] #[cfg(unix)] fn a_symlinked_directory_is_archived_under_the_links_own_name() { @@ -516,76 +675,103 @@ fn a_symlinked_directory_is_archived_under_the_links_own_name() { ); } -#[test] -fn an_empty_server_string_is_treated_as_local() { - // The dispatcher filters the empty string out before choosing the remote - // branch. If that filter went missing this would try to reach a server at - // "" and fail, with nothing listening anywhere. +/// Drive `compress_path` with a blank `server` and assert the single answer +/// both spellings of blank now get: the address is named as the mistake, +/// nothing is written, and the source is untouched. +/// +/// The wording belongs to `collapse-remote`, which is where the decision now +/// lives, and `apps/cli/tests/remote.rs` asserts the very same message. That +/// is the point of the fix: the two front-ends used to answer this +/// differently (issue #65), and the shared crate is what stops them drifting +/// again. "This computer" is `None`, covered by every local test above. +fn expect_blank_server_refusal(blank: &str) { let dir = TempDir::new().unwrap(); let source = dir.path().join("notes.txt"); - fs::write(&source, b"local please").unwrap(); + fs::write(&source, b"should have stayed here").unwrap(); let output = dir.path().join("out.zip"); - let returned = compress_path( + let error = compress_path( source.to_string_lossy().into_owned(), output.to_string_lossy().into_owned(), "zip".to_string(), 3, - Some(String::new()), + Some(blank.to_string()), + false, false, ) - .expect("an empty server string must compress locally"); + .expect_err("a blank address is not a server"); + + // The whole message rather than fragments of it. The command stringifies + // whatever `collapse-remote` returned, so equality is what pins that this + // app adds nothing of its own to the sentence the CLI also prints; three + // `contains` checks would all still pass if it started prefixing them. + // ("cannot reach the server at " named a server with no name and blamed + // the network for what is a bad setting.) + assert_eq!(error, BLANK_ADDRESS, "{blank:?}"); + + // An archive on disk is what a silent local fallback looks like, and its + // absence is the only thing that tells the two readings apart. + assert!( + !output.exists(), + "{blank:?} was compressed locally instead of being reported" + ); + assert_eq!(fs::read(&source).unwrap(), b"should have stayed here"); +} - assert_eq!(returned, output.to_string_lossy()); +/// Was `an_empty_server_string_is_treated_as_local`, which pinned the +/// dispatcher's `!s.is_empty()` filter turning `""` into a local compression. +/// Nothing sends `""`: `App.vue` sends `null` for this computer, so an empty +/// string means a stale stored value or a caller's bug, and compressing +/// locally hid it. It is now refused, the same as on the CLI. +#[test] +fn an_empty_server_string_is_refused_not_compressed_locally() { + expect_blank_server_refusal(""); +} - // The same archive the `None` path produces, proven by opening it: "did not - // error" would also be satisfied by an empty archive of the wrong source. - let out_dir = dir.path().join("extracted"); - assert_eq!( - extract_to(&output, &out_dir).unwrap(), - vec!["notes.txt".to_string()] - ); - assert_eq!( - fs::read(out_dir.join("notes.txt")).unwrap(), - b"local please" - ); +/// Was `a_whitespace_only_server_string_takes_the_remote_branch`, the KNOWN +/// DEFECT this replaces: the old filter tested emptiness rather than trimming, +/// so a run of blanks was a real destination and the compression tried to +/// leave the machine. It is refused before any request now, with the same +/// message `""` gets, so the two spellings can no longer mean two things. +#[test] +fn a_whitespace_only_server_string_is_refused_not_sent() { + for blank in [" ", "\t", "\n"] { + expect_blank_server_refusal(blank); + } } +/// The dispatch's other arm, and the one the old filter cost the most. With +/// `Some("")` read as "compress locally", pointing the app at a folder built +/// a complete archive of the tree on the very machine the user had told it +/// not to use, and nothing said so. A file cannot show that: it takes a +/// different route into `collapse-remote` (its own bytes, where a folder +/// travels as a tar envelope), so both arms have to be pinned. #[test] -fn a_whitespace_only_server_string_takes_the_remote_branch() { - // KNOWN DEFECT, pinned rather than endorsed. The dispatcher filters on - // `!s.is_empty()`, not on a trim, so a server string of blanks is treated - // as a real destination: the compression leaves the machine (or tries to) - // instead of running locally. A stale or half-cleared localStorage entry in - // the settings sheet is enough to produce one. If the filter ever learns to - // trim, this test should be rewritten to assert the local result, not - // deleted. - let dir = TempDir::new().unwrap(); - let source = dir.path().join("notes.txt"); - fs::write(&source, b"should have stayed here").unwrap(); - let output = dir.path().join("out.zip"); +fn a_blank_server_string_is_refused_for_a_directory_too() { + for blank in ["", " "] { + let dir = TempDir::new().unwrap(); + let tree = make_tree(dir.path()); + let output = dir.path().join("out.zip"); - let error = compress_path( - source.to_string_lossy().into_owned(), - output.to_string_lossy().into_owned(), - "zip".to_string(), - 3, - Some(" ".to_string()), - false, - ) - .expect_err("blanks are not a reachable server"); + let error = compress_path( + tree.to_string_lossy().into_owned(), + output.to_string_lossy().into_owned(), + "zip".to_string(), + 3, + Some(blank.to_string()), + false, + false, + ) + .expect_err("a blank address is not a server"); - // The wording is the remote client's, which is the proof the branch was - // taken: no local error can mention a server. - assert!( - error.starts_with("cannot reach the server at"), - "the remote branch must be what failed: {error}" - ); - assert!( - !output.exists(), - "nothing was compressed locally, so no archive may exist" - ); - assert_eq!(fs::read(&source).unwrap(), b"should have stayed here"); + assert_eq!(error, BLANK_ADDRESS, "{blank:?}"); + assert!( + !output.exists(), + "{blank:?} archived the whole tree locally instead of reporting the address" + ); + // And the tree it would have archived is still there to archive. + assert_eq!(fs::read(tree.join("a.txt")).unwrap(), b"top level"); + } } // ----------------------------------------------------------- compress guards -- @@ -702,10 +888,10 @@ fn a_level_out_of_range_is_refused_for_a_directory_too() { #[test] fn an_output_inside_a_missing_directory_fails_instead_of_panicking() { // The command does not create the parent directory of its output, so the - // failure comes from the backend. The two spellings below are the real - // messages a user would see; they differ because zip and tar reach it - // through `File::create` (surfacing as `CompressionError::Io`) while the - // 7z writer stringifies its own error into `Failed`. + // failure comes from the backend, and the message below is the real one a + // user would see. All three formats now spell it the same way: 7z used to + // answer with a `Debug` dump of its dependency's error, absolute output + // path included, until core learned to map that error (issue #66). for format in FORMATS { let dir = TempDir::new().unwrap(); let source = dir.path().join("notes.txt"); @@ -715,20 +901,16 @@ fn an_output_inside_a_missing_directory_fails_instead_of_panicking() { let err = compress_local(&source, &output, format, 3).unwrap_err(); - if format == "7z" { - assert!(err.starts_with("Compression failed:"), "{format}: {err}"); - assert!( - err.contains("NotFound") && err.contains("out.7z"), - "the message must name the path it could not write: {err}" - ); - } else { - assert!(err.starts_with("IO error:"), "{format}: {err}"); - #[cfg(unix)] - assert_eq!( - err, "IO error: No such file or directory (os error 2)", - "{format}" - ); - } + assert!(err.starts_with("IO error:"), "{format}: {err}"); + #[cfg(unix)] + assert_eq!( + err, "IO error: No such file or directory (os error 2)", + "{format}" + ); + assert!( + !err.contains(&format!("out.{format}")), + "{format}: the message names the path the user picked: {err}" + ); assert!(!output.exists(), "{format}: an archive appeared anyway"); } } @@ -974,35 +1156,58 @@ fn an_existing_output_is_replaced_when_the_caller_says_the_user_agreed() { } #[test] -fn replacing_an_output_writes_through_a_hardlink_to_it() { - // KNOWN LIMITATION, pinned rather than endorsed. The archive is not written - // to a temporary file and renamed into place, so replacing an output that - // happens to be a hardlink writes through the shared inode and takes the - // other name down with it. Unlinking first would fix this and cost more - // than it is worth: nothing is written until the archive is fully in hand, - // which is what lets a failed run leave the previous archive untouched (see - // the truncated-download tests in tests/remote.rs). Writing to a temporary - // file and renaming would buy both, and is the real fix if this ever bites. +fn replacing_an_output_no_longer_writes_through_a_hardlink_to_it() { + // This was a KNOWN LIMITATION and is now the opposite assertion. The + // archive used to be written straight to the output path, so replacing an + // output that happened to be a hardlink wrote through the shared inode and + // took the other name down with it: someone else's copy of an old archive + // silently became this one. Core now writes to a temporary beside the + // destination and renames it in, and a rename replaces the *name*, so the + // other name keeps the file it always had. // - // Not `cfg(unix)`: nothing here is Unix-only, and NTFS hardlinks share - // their data the same way, so the limitation is shipped on every platform - // and is pinned on every platform. + // Not `cfg(unix)`: NTFS hardlinks share their data the same way, so this is + // a property of every platform the app ships to. let dir = TempDir::new().unwrap(); let source = dir.path().join("notes.txt"); fs::write(&source, b"hello").unwrap(); let output = dir.path().join("out.zip"); - fs::write(&output, b"an older archive the user agreed to replace").unwrap(); + let previous = b"an older archive the user agreed to replace"; + fs::write(&output, previous).unwrap(); let bystander = dir.path().join("someone-elses-copy.zip"); fs::hard_link(&output, &bystander).unwrap(); compress_local_overwriting(&source, &output, "zip", 3).expect("the user agreed"); - assert_ne!( + assert_eq!( fs::read(&bystander).unwrap(), - b"an older archive the user agreed to replace", - "the hardlink kept its content, so the write stopped going through the \ - inode: update this test" + previous, + "the write went through the shared inode and destroyed a file nobody \ + named in the dialog" + ); + // And the file the user did name really was replaced, so this is not a + // refusal dressed up as a fix. + let out_dir = dir.path().join("extracted"); + assert_eq!( + listing(extract_to(&output, &out_dir).unwrap()), + vec!["notes.txt".to_string()] ); + assert_eq!(fs::read(out_dir.join("notes.txt")).unwrap(), b"hello"); + // The two names are now two files, which is what a rename into place means + // and what a write-through would have avoided. + #[cfg(unix)] + { + use std::os::unix::fs::MetadataExt; + assert_eq!( + fs::metadata(&bystander).unwrap().nlink(), + 1, + "the archive still shares an inode with the bystander" + ); + assert_ne!( + fs::metadata(&output).unwrap().ino(), + fs::metadata(&bystander).unwrap().ino(), + "both names still point at one file" + ); + } } #[test] @@ -1032,7 +1237,7 @@ fn an_output_landing_on_a_member_of_the_source_tree_is_refused_even_with_consent for format in FORMATS { for consented in [false, true] { - let err = compress_local_with(&root, &victim, format, 3, consented).unwrap_err(); + let err = compress_local_with(&root, &victim, format, 3, consented, false).unwrap_err(); assert_eq!(err, expected, "{format}, overwrite={consented}"); assert_eq!( fs::read(&victim).unwrap(), @@ -1124,7 +1329,7 @@ fn an_output_hardlinked_to_a_member_of_the_source_tree_is_refused_even_with_cons ); for consented in [false, true] { - let err = compress_local_with(&root, &alias, "zip", 3, consented).unwrap_err(); + let err = compress_local_with(&root, &alias, "zip", 3, consented, false).unwrap_err(); assert_eq!(err, expected, "overwrite={consented}"); assert_eq!( fs::read(&member).unwrap(), @@ -1245,11 +1450,11 @@ fn extracting_an_unknown_extension_is_refused() { } #[test] -fn an_uppercase_extension_is_rejected_even_for_a_valid_archive() { - // KNOWN LIMITATION, pinned rather than endorsed: collapse-core matches the - // extension against literal lowercase strings without lowercasing the - // input, so a perfectly good zip named `.ZIP` is unreadable. If the match - // is ever made case-insensitive, this test should be updated, not deleted. +fn an_uppercase_extension_extracts_like_any_other() { + // The extension is a file name, not a wire value, and Windows and macOS + // fold case in the filesystem, so a perfectly good zip called `.ZIP` used + // to be refused as an unknown format for the spelling of its name alone. + // This is where a user met that: whatever the open dialog handed back. let dir = TempDir::new().unwrap(); let source = dir.path().join("notes.txt"); fs::write(&source, b"hello").unwrap(); @@ -1257,17 +1462,40 @@ fn an_uppercase_extension_is_rejected_even_for_a_valid_archive() { compress_local(&source, &lower, "zip", 3).unwrap(); // A distinct base name, so this still works on a case-insensitive volume. - let upper = dir.path().join("LOUD.ZIP"); - fs::copy(&lower, &upper).unwrap(); - - let err = extract_to(&upper, &dir.path().join("out")).unwrap_err(); + for shouted in ["LOUD.ZIP", "Mixed.Zip", "SEVEN.7Z", "BALL.TAR"] { + let renamed = dir.path().join(shouted); + fs::copy(&lower, &renamed).unwrap(); + let out = dir.path().join(format!("out_{shouted}")); + + // .7Z and .TAR are the wrong format for these bytes, so they must fail + // on the CONTENT, not on the name: the point is that the extension was + // understood and the archive opened, which is the opposite of what a + // rejected name looks like. + let outcome = extract_to(&renamed, &out); + match shouted { + "LOUD.ZIP" | "Mixed.Zip" => assert_eq!( + outcome.unwrap(), + vec!["notes.txt".to_string()], + "{shouted} is a zip and should read as one" + ), + _ => { + let err = outcome.expect_err("zip bytes are not a 7z or a tar"); + assert!( + !err.contains("Unknown archive extension"), + "{shouted}: the name was understood, so the complaint must be \ + about the bytes, got {err}" + ); + } + } + } - assert_eq!(err, "Compression failed: Unknown archive extension: .ZIP"); - // The same bytes under a lowercase name extract fine, which is what makes - // the failure above a naming quirk rather than a corrupt archive. + // An extension that is not one of ours is still refused, whatever its case: + // this made the match lenient about spelling, not about formats. + let foreign = dir.path().join("photos.RAR"); + fs::copy(&lower, &foreign).unwrap(); assert_eq!( - extract_to(&lower, &dir.path().join("out_lower")).unwrap(), - vec!["notes.txt".to_string()] + extract_to(&foreign, &dir.path().join("out_rar")).unwrap_err(), + "Compression failed: Unknown archive extension: .RAR" ); } @@ -1277,8 +1505,8 @@ fn a_truncated_archive_is_reported_legibly_instead_of_panicking() { // show something a person can act on, and the process must survive. The // three messages below are the real ones, and they are pinned by their // recognizable half so a zip/7z/tar version bump does not rewrite the test - // while a change of *variant* (an `IO error:` prefix, or the extension - // error, meaning the dispatch went wrong) still fails it. + // while the extension error (meaning the dispatch went wrong before the + // archive was ever read) still fails it. for format in FORMATS { let dir = TempDir::new().unwrap(); let source = dir.path().join("notes.txt"); @@ -1292,24 +1520,31 @@ fn a_truncated_archive_is_reported_legibly_instead_of_panicking() { let out_dir = dir.path().join("extracted"); let err = extract_to(&archive, &out_dir).unwrap_err(); - assert!(err.starts_with("Compression failed:"), "{format}: {err}"); assert!( !err.contains("Unknown archive extension"), "{format}: the extension dispatch failed before the archive was even read: {err}" ); + // No backend may answer with a struct dump, whatever its variant. + assert!( + !err.contains("Error {") && !err.contains("kind:"), + "{format}: this is a Debug dump, not a sentence: {err}" + ); match format { // "Compression failed: invalid Zip archive: Could not find EOCD" "zip" => { + assert!(err.starts_with("Compression failed:"), "{format}: {err}"); assert!(err.contains("Zip"), "{err}"); assert!( !out_dir.exists(), "zip refuses the archive before creating the output directory" ); } - // "Compression failed: Io(Error { kind: UnexpectedEof, message: - // \"failed to fill whole buffer\" }, \"\")" + // The short read reaches core as its dependency's `Io` variant, so + // core unwraps it to `CompressionError::Io` (issue #66). It used to + // read `Compression failed: Io(Error { kind: UnexpectedEof, + // message: "failed to fill whole buffer" }, "")`. "7z" => { - assert!(err.contains("UnexpectedEof"), "{err}"); + assert_eq!(err, "IO error: failed to fill whole buffer"); let leftovers: Vec = fs::read_dir(&out_dir) .map(|entries| entries.map(|e| e.unwrap().path()).collect()) .unwrap_or_default(); @@ -1317,6 +1552,7 @@ fn a_truncated_archive_is_reported_legibly_instead_of_panicking() { } // "Compression failed: failed to unpack `/notes.txt`" _ => { + assert!(err.starts_with("Compression failed:"), "{format}: {err}"); assert!(err.contains("failed to unpack"), "{err}"); assert!( err.contains("notes.txt"), diff --git a/apps/desktop/src-tauri/tests/ipc.rs b/apps/desktop/src-tauri/tests/ipc.rs index 335456a..ea82fdc 100644 --- a/apps/desktop/src-tauri/tests/ipc.rs +++ b/apps/desktop/src-tauri/tests/ipc.rs @@ -63,11 +63,12 @@ use std::path::{Path, PathBuf}; /// containment, so adding a fifth command is also a deliberate edit here. /// That matters: a command missing from this list would get none of the /// anti-vacuity protection the canary exists to provide. -const BASELINE: [&str; 4] = [ +const BASELINE: [&str; 5] = [ "check_server", "compress_path", "extract_archive", "is_directory", + "unwritable_names", ]; /// Quote characters that open a string literal, per language. Needed by every @@ -938,6 +939,88 @@ fn no_command_attribute_renames_the_wire_contract() { } } +#[test] +fn every_command_that_can_block_is_marked_async() { + // The only place this can be pinned. A `#[tauri::command]` is an ordinary + // function, so every other test in this crate calls these directly and gets + // the identical result whatever the attribute says: the argument changes + // how TAURI invokes them, and nothing else. + // + // What it changes is which thread runs the body. Bare, tauri-macros 2.6.3 + // compiles a synchronous command to its `sync` path, which runs inline on + // the thread handling the IPC message, so the window stops repainting until + // the call returns. `async` moves it to `sync_threadpool`, off that thread. + // + // Measured before this was fixed: `check_server` against an unroutable + // address froze the window for the whole of ureq's 30 second connect + // timeout, and a compression froze it for as long as the compression took. + const MUST_NOT_BLOCK: [(&str, &str); 4] = [ + ( + "compress_path", + "compresses a whole tree, or waits on a server with no read timeout", + ), + ("extract_archive", "unpacks a whole archive"), + ( + "unwritable_names", + "reads the listing of a whole archive, which for a tar means walking every header", + ), + ( + "check_server", + "waits out a connect timeout when the address is wrong", + ), + ]; + // The exception, listed rather than merely absent so that adding a command + // here is a decision someone made on purpose. + const MAY_BLOCK: [(&str, &str); 1] = [( + "is_directory", + "one stat, called while the user is still choosing", + )]; + + let commands = parse_rust_commands(&commands_rs()); + for (name, why) in MUST_NOT_BLOCK { + let command = commands + .get(name) + .unwrap_or_else(|| panic!("`{name}` is gone from src/commands.rs: update this list")); + let args: Vec = command.attribute_args.chars().collect(); + assert!( + find_word(&args, "async", 0).is_some(), + "`{name}` (src/commands.rs line {}) has no `async` in its attribute, and it {why}.\n\ + Without it Tauri runs the body on the thread handling the IPC message and the \ + window freezes for the whole call. Write `#[tauri::command(async)]`, or, if this \ + command genuinely cannot block any more, move it to MAY_BLOCK with the reason.", + command.line + ); + } + for (name, why) in MAY_BLOCK { + let command = commands + .get(name) + .unwrap_or_else(|| panic!("`{name}` is gone from src/commands.rs: update this list")); + let args: Vec = command.attribute_args.chars().collect(); + assert!( + find_word(&args, "async", 0).is_none(), + "`{name}` (src/commands.rs line {}) is marked `async`, but it is listed here as \ + cheap enough not to need it ({why}).\n\ + Marking it is not harmful, it just costs a round trip through the runtime for a \ + call the UI makes while the user is still choosing. If it now does real work, \ + move it to MUST_NOT_BLOCK.", + command.line + ); + } + // Nothing may sit in neither list: a new command has to be classified. + let listed: BTreeSet<&str> = MUST_NOT_BLOCK + .iter() + .chain(MAY_BLOCK.iter()) + .map(|(name, _)| *name) + .collect(); + let found: BTreeSet<&str> = commands.keys().map(|k| k.as_str()).collect(); + assert_eq!( + found, listed, + "src/commands.rs and this test disagree about which commands exist. Every command has \ + to be in MUST_NOT_BLOCK or in MAY_BLOCK, so that whether it can freeze the window is \ + something someone decided rather than something nobody noticed." + ); +} + // -------------------------------------------------------------- registration -- #[test] @@ -1020,12 +1103,14 @@ fn every_command_is_covered_by_the_vitest_stub_switch() { // command name that ends in a bare `return null`, so an unstubbed command // does not fail the JS run: it resolves to null. What that costs depends on // the command, and it was measured by deleting each branch and re-running - // the Vitest suite. Two branches are load-bearing: dropping `compress_path` - // or `extract_archive` turns a case red, because both cases assert on what - // the component renders from the returned value. Two are cosmetic: - // `is_directory` returns false and `check_server` returns null, and with - // either branch gone the suite still passes 7 of 7, since no assertion can - // tell those values from the fallthrough null. + // the Vitest suite. Three branches are load-bearing: dropping + // `compress_path`, `extract_archive` or `unwritable_names` turns a case + // red, because each is read for something the component then renders (the + // last one is read for `entries.length`, so a null throws where a stub + // would have answered). Two are cosmetic: `is_directory` returns false and + // `check_server` returns null, and with either branch gone the suite still + // passes, since no assertion can tell those values from the fallthrough + // null. // // So this test does not claim every branch is load-bearing. It keeps the // switch in lockstep with the handler list, so a command added on the Rust @@ -1129,7 +1214,7 @@ fn command_signatures_are_pinned_with_their_types() { // // Sets, not sequences: Tauri binds arguments by key, so reordering two // parameters changes nothing on the wire and must not fail here. - let expected: [(&str, &[(&str, &str)]); 4] = [ + let expected: [(&str, &[(&str, &str)]); 5] = [ ("check_server", &[("url", "String")]), ( "compress_path", @@ -1140,13 +1225,23 @@ fn command_signatures_are_pinned_with_their_types() { ("level", "u32"), ("server", "Option"), ("overwrite", "bool"), + ("verify", "bool"), ], ), ( "extract_archive", - &[("archive", "String"), ("output_dir", "String")], + &[ + ("archive", "String"), + ("output_dir", "String"), + // The user's answers for the entry names this host cannot + // write, one character to what it becomes. A `HashMap` here + // would deserialize identically and report two bad keys in a + // different order on every run. + ("replacements", "BTreeMap"), + ], ), ("is_directory", &[("path", "String")]), + ("unwritable_names", &[("archive", "String")]), ]; // One list of commands, not two: BASELINE decides what ships, this table diff --git a/apps/desktop/src-tauri/tests/names.rs b/apps/desktop/src-tauri/tests/names.rs new file mode 100644 index 0000000..4941c3c --- /dev/null +++ b/apps/desktop/src-tauri/tests/names.rs @@ -0,0 +1,476 @@ +//! The naming exchange behind the extract dialog: `unwritable_names`, the +//! answers `extract_archive` takes, and the JSON both of them cross the IPC +//! boundary as. +//! +//! Two halves, and the split is deliberate. +//! +//! The **pure** half runs the Windows ruleset from wherever the suite runs, so +//! the dialog's data (and the exact JSON the webview parses) is verified on a +//! Mac and on the Linux CI leg, not only on the Windows leg that runs on the +//! release path. That is the whole reason `NameRules` is data rather than +//! `#[cfg]`, and this file would be worth very little without it. +//! +//! The **host** half needs an entry name this machine genuinely refuses, which +//! on Unix means exactly one character: the NUL byte, which cannot cross the +//! libc boundary. Those cases are `#[cfg(unix)]` because their fixture is, not +//! because the behaviour is: the same code path runs on Windows for a much +//! larger alphabet, and core's `tests/names.rs` covers that alphabet with the +//! Windows rules from anywhere. +//! +//! The fixtures are crafted with the `zip` crate directly. Nothing that +//! compresses a real directory could produce them, because the filesystem +//! would have refused to hold the file in the first place. + +use std::collections::BTreeMap; +use std::fs; +use std::io::Write; +use std::path::{Path, PathBuf}; + +use collapse_core::{NameReport, NameRules}; +use collapse_desktop::commands::{extract_archive, unwritable_names, Extraction}; +use collapse_desktop::names::{substitutions_from, NameInspection}; +use serde_json::json; +use tempfile::TempDir; +use zip::write::SimpleFileOptions; +use zip::{CompressionMethod, ZipWriter}; + +// ---------------------------------------------------------------- fixtures -- + +/// A zip whose entries are named exactly as given, however hostile the name. +fn zip_with(dir: &Path, entries: &[(&str, &[u8])]) -> PathBuf { + let archive = dir.join("input.zip"); + let file = fs::File::create(&archive).unwrap(); + let mut writer = ZipWriter::new(file); + let options = SimpleFileOptions::default().compression_method(CompressionMethod::Stored); + for (name, content) in entries { + writer.start_file(*name, options).unwrap(); + writer.write_all(content).unwrap(); + } + writer.finish().unwrap(); + archive +} + +fn text(path: &Path) -> String { + path.to_string_lossy().into_owned() +} + +fn inspect(archive: &Path) -> Result { + unwritable_names(text(archive)) +} + +/// Extract with the answers a user would have typed into the dialog. +fn extract_answering( + archive: &Path, + into: &Path, + answers: &[(&str, &str)], +) -> Result { + let replacements: BTreeMap = answers + .iter() + .map(|(character, replacement)| (character.to_string(), replacement.to_string())) + .collect(); + extract_archive(text(archive), text(into), replacements) +} + +/// Every file under `dir`, relative, sorted and forward-slashed. +fn files_under(dir: &Path) -> Vec { + let mut found = Vec::new(); + let mut pending = vec![dir.to_path_buf()]; + while let Some(current) = pending.pop() { + let Ok(children) = fs::read_dir(¤t) else { + continue; + }; + for child in children.flatten() { + let path = child.path(); + if path.is_dir() { + pending.push(path); + } else { + found.push( + path.strip_prefix(dir) + .unwrap() + .to_string_lossy() + .replace('\\', "/"), + ); + } + } + } + found.sort(); + found +} + +/// The message of an outcome that refused to write anything, or a panic naming +/// what came back instead. +fn refusal(outcome: Extraction) -> String { + match outcome { + Extraction::NameProblem { message } => message, + Extraction::Extracted { files } => { + panic!("expected a naming question, but the archive extracted {files:?}") + } + } +} + +fn written(outcome: Extraction) -> Vec { + match outcome { + Extraction::Extracted { mut files } => { + for name in &mut files { + *name = name.replace('\\', "/"); + } + files.sort(); + files + } + Extraction::NameProblem { message } => { + panic!("expected an extraction, but it asked: {message}") + } + } +} + +// ------------------------------------------------- the shape on the wire -- + +#[test] +fn the_dialog_is_handed_exactly_the_json_it_reads() { + // The webview builds the whole dialog out of this object and nothing type + // checks the crossing, so the shape is pinned here, from the Windows rules, + // on whatever machine runs the suite. `apps/desktop/tests/App.test.js` + // stubs this same shape by hand; if serde's tags move (the `kind` tag, the + // camelCase variant names, the lowercase faults) this fails here and the + // Vitest suite goes on passing against a shape that no longer exists. + let names = ["logs/what?.txt", "when?.txt", "notes.txt.", "CON.txt"]; + let rules = NameRules::windows(); + let inspection = NameInspection::new(NameReport::of(&names, rules), rules); + + let wire = serde_json::to_value(&inspection).unwrap(); + assert_eq!( + wire["entries"], + json!([ + { + "entry": "logs/what?.txt", + "problems": [{ "kind": "character", "character": "?", "fault": "rejected" }], + }, + { + "entry": "when?.txt", + "problems": [{ "kind": "character", "character": "?", "fault": "rejected" }], + }, + { + "entry": "notes.txt.", + "problems": [{ "kind": "trailingCharacters", "removed": "." }], + }, + { + "entry": "CON.txt", + "problems": [{ "kind": "reservedDevice", "device": "CON" }], + }, + ]) + ); + // One question per character, not per entry: two files carry the `?`, and + // the dialog puts one text field on screen for both. + assert_eq!( + wire["characters"], + json!([{ "character": "?", "fault": "rejected", "entries": 2 }]) + ); + // Flattened, so the webview reads one object rather than reaching through + // a wrapper for the half it needs. + assert!( + wire.get("report").is_none(), + "the report must be flattened into the inspection: {wire}" + ); +} + +#[test] +fn a_colon_is_offered_as_the_fault_that_it_is() { + // The colon is the one Windows ACCEPTS: `notes.txt:hidden` is the `hidden` + // stream of `notes.txt`, the write succeeds and the file exists under no + // name (issue #63). The dialog says something quite different for it than + // for a `?`, which it can only do if the fault survives serialization. + let rules = NameRules::windows(); + let inspection = NameInspection::new(NameReport::of(&["notes.txt:hidden"], rules), rules); + + let wire = serde_json::to_value(&inspection).unwrap(); + assert_eq!( + wire["characters"], + json!([{ "character": ":", "fault": "reinterpreted", "entries": 1 }]) + ); +} + +#[test] +fn the_dialog_is_told_every_character_an_answer_may_not_contain() { + // Sent as data so the dialog can refuse a bad answer as it is typed without + // holding a copy of the rules in JavaScript. Two rulesets, both asked for + // by name, so this runs everywhere. + let windows: String = (0u8..=0x1f) + .map(char::from) + .chain("\"*/:<>?\\|".chars()) + .collect(); + assert_eq!( + NameInspection::new(NameReport::default(), NameRules::windows()).rejected_in_replacement, + windows + ); + // Unix refuses one character, and the two separators are added to both: + // answering `?` with `../` would move the entry to another directory rather + // than rename it, which is why core refuses it whatever the ruleset. + assert_eq!( + NameInspection::new(NameReport::default(), NameRules::unix()).rejected_in_replacement, + "\u{0}/\\" + ); +} + +#[test] +fn an_archive_with_nothing_wrong_still_says_what_an_answer_may_not_contain() { + // The empty case is not the null case: `rejectedInReplacement` describes + // the host, not the archive, and a UI that only received it alongside a + // complaint could not validate anything. + let inspection = NameInspection::new(NameReport::default(), NameRules::windows()); + assert!(inspection.is_empty()); + assert!(inspection.rejected_in_replacement.contains('?')); +} + +// ------------------------------------------------------------- the answers -- + +#[test] +fn the_answers_arrive_as_strings_and_become_characters() { + let answers = BTreeMap::from([("?".to_string(), "-".to_string())]); + let substitutions = substitutions_from(&answers).unwrap(); + assert_eq!(substitutions.get('?'), Some("-")); +} + +#[test] +fn an_answer_keyed_by_more_than_one_character_is_refused_by_name() { + // A JSON object has no `char` keys, so this is the one thing that can go + // wrong in the translation, and it has to name the key it choked on. + let answers = BTreeMap::from([("??".to_string(), "-".to_string())]); + let problem = substitutions_from(&answers).unwrap_err().to_string(); + assert!(problem.contains("\"??\""), "{problem}"); + assert!(problem.contains("not a single character"), "{problem}"); +} + +// -------------------------------------------------------------- inspecting -- + +#[test] +fn inspecting_a_missing_archive_reports_it_by_path() { + let dir = TempDir::new().unwrap(); + let missing = dir.path().join("nope.zip"); + + assert_eq!( + inspect(&missing).unwrap_err(), + format!("Not found: {}", missing.to_string_lossy()) + ); +} + +#[test] +fn an_archive_this_computer_can_write_asks_nothing() { + let dir = TempDir::new().unwrap(); + let archive = zip_with( + dir.path(), + &[("notes.txt", b"hello"), ("sub/deep.txt", b"hi")], + ); + + let inspection = inspect(&archive).unwrap(); + + assert!(inspection.is_empty()); + assert!(inspection.report.entries.is_empty()); + assert!(inspection.report.characters.is_empty()); +} + +#[test] +fn an_archive_that_cannot_be_read_asks_nothing_and_leaves_the_complaining_to_the_extractor() { + // Deliberate: the extractor is about to open the same file and fail on it + // in its own words ("Could not find EOCD"), which is the message a user can + // act on. Answering first would replace it with a worse one, and would put + // a dialog in the way of an archive that has no naming question at all. + let dir = TempDir::new().unwrap(); + let archive = zip_with(dir.path(), &[("notes.txt", b"hello")]); + let whole = fs::read(&archive).unwrap(); + fs::write(&archive, &whole[..whole.len() / 2]).unwrap(); + + assert!(inspect(&archive).unwrap().is_empty()); + + let out = dir.path().join("out"); + let complaint = extract_answering(&archive, &out, &[]).unwrap_err(); + assert!(complaint.contains("Zip"), "{complaint}"); + + // Same for a name no backend claims: refused by the extractor, not here. + let foreign = dir.path().join("photos.rar"); + fs::write(&foreign, b"not an archive").unwrap(); + assert!(inspect(&foreign).unwrap().is_empty()); + assert_eq!( + extract_answering(&foreign, &out, &[]).unwrap_err(), + "Compression failed: Unknown archive extension: .rar" + ); +} + +// -------------------------------------------------------------- extracting -- + +#[test] +fn an_ordinary_archive_extracts_with_no_answers_at_all() { + let dir = TempDir::new().unwrap(); + let archive = zip_with( + dir.path(), + &[("notes.txt", b"hello"), ("sub/deep.txt", b"hi")], + ); + let out = dir.path().join("out"); + + let outcome = extract_answering(&archive, &out, &[]).unwrap(); + + assert_eq!(written(outcome), ["notes.txt", "sub/deep.txt"]); + // What was reported is what is there: the listing is not a promise made + // from the archive's own names. + assert_eq!(files_under(&out), ["notes.txt", "sub/deep.txt"]); + assert_eq!(fs::read(out.join("notes.txt")).unwrap(), b"hello"); +} + +#[test] +fn an_answer_containing_a_separator_is_refused_before_anything_is_written() { + // `?` is writable on this host, so nothing in this archive needs answering: + // the answer is refused on its own account, by the ruleset, before the + // archive is opened. That is what keeps a bad answer from being discovered + // half way through an extraction. + let dir = TempDir::new().unwrap(); + let archive = zip_with(dir.path(), &[("notes.txt", b"hello")]); + let out = dir.path().join("out"); + + let problem = refusal(extract_answering(&archive, &out, &[("?", "../escaped")]).unwrap()); + + assert!(problem.contains("path separator"), "{problem}"); + assert!( + !out.exists(), + "the output directory was created before the answer was judged" + ); +} + +#[test] +fn an_answer_keyed_by_a_whole_word_is_a_question_rather_than_a_failure() { + // The webview's mistake, not the user's, but it still wrote nothing, so it + // comes back the same way and the dialog stays open on it. + let dir = TempDir::new().unwrap(); + let archive = zip_with(dir.path(), &[("notes.txt", b"hello")]); + let out = dir.path().join("out"); + + let problem = refusal(extract_answering(&archive, &out, &[("colon", "-")]).unwrap()); + + assert!(problem.contains("not a single character"), "{problem}"); + assert!(!out.exists(), "nothing may be written on a bad answer"); +} + +// --------------------------------------------- what this host really refuses -- + +/// An entry name this machine cannot write, and the character behind it. +/// +/// The NUL byte cannot cross the libc boundary, so no Unix filesystem can hold +/// it: `std` answers `InvalidInput` before the kernel is ever asked. It is the +/// only character in that position on Unix, which is why the dialog is a +/// Windows feature in practice, and why it is also the only fixture that can +/// drive the real host rules from a Mac. +#[cfg(unix)] +const UNWRITABLE_HERE: &str = "bad\u{0}name.txt"; + +#[cfg(unix)] +#[test] +fn a_name_this_computer_cannot_write_is_reported_with_the_character_to_ask_about() { + let dir = TempDir::new().unwrap(); + let archive = zip_with(dir.path(), &[(UNWRITABLE_HERE, b"hello")]); + + let inspection = inspect(&archive).unwrap(); + + assert!(!inspection.is_empty()); + assert_eq!(inspection.report.entries.len(), 1); + assert_eq!(inspection.report.entries[0].entry, UNWRITABLE_HERE); + assert_eq!(inspection.report.characters.len(), 1); + assert_eq!(inspection.report.characters[0].character, '\u{0}'); + assert_eq!(inspection.report.characters[0].entries, 1); + // Nothing is created by asking: the dialog goes up before any destination + // has been touched. + assert_eq!(files_under(dir.path()), ["input.zip"]); +} + +#[cfg(unix)] +#[test] +fn the_answer_is_written_and_the_listing_names_what_is_on_disk() { + let dir = TempDir::new().unwrap(); + let archive = zip_with(dir.path(), &[(UNWRITABLE_HERE, b"hello")]); + let out = dir.path().join("out"); + + let outcome = extract_answering(&archive, &out, &[("\u{0}", "_")]).unwrap(); + + // The name on disk, never the archive's: reporting `bad\0name.txt` would + // name a file that exists nowhere and send the user looking for it. + assert_eq!(written(outcome), ["bad_name.txt"]); + assert_eq!(files_under(&out), ["bad_name.txt"]); + assert_eq!(fs::read(out.join("bad_name.txt")).unwrap(), b"hello"); +} + +#[cfg(unix)] +#[test] +fn an_empty_answer_removes_the_character() { + let dir = TempDir::new().unwrap(); + let archive = zip_with(dir.path(), &[(UNWRITABLE_HERE, b"hello")]); + let out = dir.path().join("out"); + + let outcome = extract_answering(&archive, &out, &[("\u{0}", "")]).unwrap(); + + assert_eq!(written(outcome), ["badname.txt"]); +} + +#[cfg(unix)] +#[test] +fn a_name_left_unanswered_stops_before_anything_is_written() { + // The other entry is perfectly writable and still does not get written: + // extraction settles every name from the listing before the first byte, so + // a user who dismissed the dialog is not left with half an archive. + let dir = TempDir::new().unwrap(); + let archive = zip_with( + dir.path(), + &[("fine.txt", b"hello"), (UNWRITABLE_HERE, b"hello")], + ); + let out = dir.path().join("out"); + + let problem = refusal(extract_answering(&archive, &out, &[]).unwrap()); + + assert!( + problem.contains("no replacement for it was given"), + "{problem}" + ); + assert!(problem.contains("bad"), "the entry names itself: {problem}"); + assert!( + !out.exists(), + "an unanswered name wrote {:?}", + files_under(&out) + ); +} + +#[cfg(unix)] +#[test] +fn an_answer_this_computer_cannot_write_either_is_refused_with_the_reason() { + let dir = TempDir::new().unwrap(); + let archive = zip_with(dir.path(), &[(UNWRITABLE_HERE, b"hello")]); + let out = dir.path().join("out"); + + // Replacing the NUL with a NUL is not an answer, and it is caught before + // the archive is opened rather than by the write failing. + let problem = refusal(extract_answering(&archive, &out, &[("\u{0}", "a\u{0}b")]).unwrap()); + + assert!(problem.contains("cannot write"), "{problem}"); + assert!(!out.exists()); +} + +#[cfg(unix)] +#[test] +fn two_entries_that_would_land_on_one_name_are_refused_naming_both() { + // Renaming one of them behind the user's back is how a file disappears + // without anyone noticing, so the answer is refused and both names are + // said out loud. The second entry here is one the host could write + // perfectly well: the collision is created by the answer, not found in the + // archive. + let dir = TempDir::new().unwrap(); + let archive = zip_with( + dir.path(), + &[(UNWRITABLE_HERE, b"first"), ("bad_name.txt", b"second")], + ); + let out = dir.path().join("out"); + + let problem = refusal(extract_answering(&archive, &out, &[("\u{0}", "_")]).unwrap()); + + assert!(problem.contains("bad_name.txt"), "{problem}"); + assert!(problem.contains("both be written as"), "{problem}"); + assert!(!out.exists(), "a collision wrote {:?}", files_under(&out)); + + // And an answer that keeps them apart goes through. + let outcome = extract_answering(&archive, &out, &[("\u{0}", "-")]).unwrap(); + assert_eq!(written(outcome), ["bad-name.txt", "bad_name.txt"]); +} diff --git a/apps/desktop/src-tauri/tests/remote.rs b/apps/desktop/src-tauri/tests/remote.rs index 96d464d..4cf8c60 100644 --- a/apps/desktop/src-tauri/tests/remote.rs +++ b/apps/desktop/src-tauri/tests/remote.rs @@ -16,11 +16,12 @@ //! tests that have to rule out a local fallback assert on that log. It is the //! one piece of evidence such a fallback cannot fake. +use std::collections::BTreeMap; use std::path::{Path, PathBuf}; use std::sync::{Arc, Mutex, OnceLock}; use collapse_core::compression::compress_tar_dir; -use collapse_desktop::commands::{check_server, compress_path, extract_archive}; +use collapse_desktop::commands::{check_server, compress_path, extract_archive, Extraction}; // ------------------------------------------------------------------ harness -- @@ -160,11 +161,21 @@ fn compress_request(server: &Server) -> String { /// without any HTTP exchange. const UNREACHABLE: &str = "http://127.0.0.1:9"; +/// `RemoteError::BlankServer` rendered, which is what `App.vue` shows. The +/// whole sentence, because the value of moving this answer into +/// `collapse-remote` is that the CLI prints the identical one +/// (`apps/cli/tests/remote.rs` and `tests/commands.rs` spell out the same +/// literal): a front-end that started decorating it would drift again, and no +/// `contains` check could tell. +const BLANK_ADDRESS: &str = + "the server address is blank: it needs a URL, for example http://localhost:8000"; + fn text(path: &Path) -> String { path.to_string_lossy().into_owned() } -/// `compress_path` with a server, spelled the way the webview spells it. +/// `compress_path` with a server, spelled the way the webview spells it: the +/// Verify box is disabled for a remote destination, so `verify` arrives false. fn compress_remotely( server: &str, source: &Path, @@ -179,6 +190,7 @@ fn compress_remotely( level, Some(server.to_string()), false, + false, ) } @@ -192,6 +204,7 @@ fn compress_locally(source: &Path, output: &Path, format: &str, level: u32) -> S level, None, false, + false, ) .expect("the local compression succeeds") } @@ -207,8 +220,14 @@ fn compress_locally(source: &Path, output: &Path, format: &str, level: u32) -> S /// The normalized name still reads the file, because `Path::join` accepts a /// forward slash on Windows too. fn extracted(archive: &Path, into: &Path) -> Vec<(String, Vec)> { - let mut files: Vec = extract_archive(text(archive), text(into)) - .expect("the archive extracts cleanly") + // No answers to give: these archives are built from real files on this + // machine, so every name in them is one this machine can write. + let outcome = extract_archive(text(archive), text(into), BTreeMap::new()) + .expect("the archive extracts cleanly"); + let Extraction::Extracted { files } = outcome else { + panic!("a locally built archive asked a naming question: {outcome:?}"); + }; + let mut files: Vec = files .into_iter() .map(|name| name.replace('\\', "/")) .collect(); @@ -417,6 +436,109 @@ fn a_file_named_like_a_tarball_is_compressed_as_a_file() { ); } +// ------------------------------------------------------- where the work goes -- + +/// The dispatch itself, all three readings of `server` in one place against +/// one server. Since the blank-address fix (issue #65) `Some(_)` means remote +/// whatever the string holds, where the app used to filter `""` out and +/// compress locally, and dropping that filter could just as easily have +/// broken the two cases nobody was looking at: a real address must still +/// cross the wire, and `None` must still not. +/// +/// The request log is the only referee available. A remote archive is +/// indistinguishable from a local one by design (this file's header says why), +/// so no file on disk can say where it was built; what a misrouted call +/// changes is how many uploads the server saw. `compress_request` fails on +/// none (a real address gone local) and on two (a `None`, or a blank, gone +/// remote). +#[test] +fn the_server_argument_alone_decides_where_the_work_happens() { + let server = start_server(); + let dir = tempfile::TempDir::new().unwrap(); + let source = dir.path().join("notes.txt"); + std::fs::write(&source, b"one source, three destinations").unwrap(); + + let remote_archive = dir.path().join("remote.zip"); + compress_remotely(&server.url, &source, &remote_archive, "zip", 3) + .expect("a real address compresses on the server"); + + let local_archive = dir.path().join("local.zip"); + compress_locally(&source, &local_archive, "zip", 3); + + let blank_archive = dir.path().join("blank.zip"); + let error = compress_path( + text(&source), + text(&blank_archive), + "zip".to_string(), + 3, + Some(" ".to_string()), + false, + false, + ) + .expect_err("a blank address is not a server"); + assert_eq!(error, BLANK_ADDRESS); + assert!( + !blank_archive.exists(), + "the blank address fell back to compressing locally" + ); + + let request = compress_request(&server); + assert!( + request.contains("name=notes.txt") && request.contains("envelope=none"), + "the one upload is the one the real address asked for: {request}" + ); + + // Both archives that were produced hold the same thing, which is what + // makes "where" the only difference between the two calls. + assert_eq!( + extracted(&remote_archive, &dir.path().join("r")), + extracted(&local_archive, &dir.path().join("l")) + ); +} + +/// Asking for the contents check while the work goes to a server is answered +/// with the archive, not with a refusal. +/// +/// The UI cannot produce this (the box is disabled and sends false whenever a +/// server is chosen), so this pins the decision behind that: the flag describes +/// a check on an archive this app built, and the archive arrives from elsewhere +/// with no list of expected entries to check it against. Nothing about the +/// request is harmful, so refusing it would cost the user their compression for +/// a box the app itself let them tick. +#[test] +fn asking_to_check_contents_is_ignored_rather_than_refused_by_a_server_run() { + let server = shared_server(); + let dir = tempfile::TempDir::new().unwrap(); + let source = dir.path().join("notes.txt"); + let body = b"the checking happens over there, if at all".to_vec(); + std::fs::write(&source, &body).unwrap(); + + let asked = dir.path().join("asked.zip"); + compress_path( + text(&source), + text(&asked), + "zip".to_string(), + 3, + Some(server.to_string()), + false, + true, + ) + .expect("a check this side cannot make is not a reason to refuse the compression"); + + // And it holds what the same call without the tick holds: the flag reaches + // no part of the remote path, so it can change nothing about the result. + // (Contents rather than bytes: zip stamps a modification time, so two runs + // a second apart differ as files while holding the same archive.) + let plain = dir.path().join("plain.zip"); + compress_remotely(server, &source, &plain, "zip", 3).expect("the server compresses"); + let asked_entries = extracted(&asked, &dir.path().join("asked-out")); + assert_eq!( + asked_entries, + extracted(&plain, &dir.path().join("plain-out")) + ); + assert_eq!(asked_entries, vec![("notes.txt".to_string(), body)]); +} + // ---------------------------------------------------------------- failures -- /// A server that is not there has to say so, by name, and must not leave a @@ -731,6 +853,23 @@ fn check_server_reports_an_unreachable_address() { ); } +/// A blank address never leaves the machine: the probe says the address is +/// the problem instead of reporting a server with no name as unreachable. +/// `sources.js` refuses a blank before the sheet can send one, so reaching +/// this means a stale stored value, and "cannot reach" would point the user +/// at the network for it (issue #65). +#[test] +fn check_server_rejects_a_blank_address() { + for blank in ["", " ", "\t"] { + let error = check_server(blank.to_string()).expect_err("a blank address is not a server"); + // The whole message: the probe hands the settings sheet whatever + // `collapse-remote` said, and equality is what keeps this app from + // wrapping it into a sentence of its own. (A fragment check would + // still pass if it did.) + assert_eq!(error, BLANK_ADDRESS, "{blank:?}"); + } +} + /// A URL no HTTP client can even parse is still just a failed probe: the /// settings sheet gets a message, and the app does not come down with it. The /// message has to say more than "unreachable", or a user typing a broken diff --git a/apps/desktop/src/App.vue b/apps/desktop/src/App.vue index 4ea2122..13ba606 100644 --- a/apps/desktop/src/App.vue +++ b/apps/desktop/src/App.vue @@ -3,7 +3,21 @@ import { ref, computed, onMounted, onUnmounted } from 'vue' import { invoke } from '@tauri-apps/api/core' import { open, save } from '@tauri-apps/plugin-dialog' import { getCurrentWebview } from '@tauri-apps/api/webview' -import { baseName, dirOf, isArchive, levelHint as levelHintFor } from './paths.js' +import { + baseName, + dirOf, + isArchive, + levelHint as levelHintFor, + verifyNote as verifyNoteFor, +} from './paths.js' +import { + adjustmentNote, + characterLabel, + faultNote, + initialAnswers, + replacementError, + substitutions, +} from './names.js' import { LOCAL, labelFor, @@ -28,6 +42,10 @@ const error = ref(null) const format = ref('zip') const level = ref(3) +// The deeper of the two checks the archive gets before it is saved, off by +// default because it costs about as much again as the compression did. The +// cheap one (reading the listing back) is not optional and runs regardless. +const verifyContents = ref(false) // Where compression runs. Remotes are remembered between launches; the // dropdown is always on screen in compress mode, so the active one is never @@ -41,6 +59,19 @@ const checking = ref(null) // source id being tested const checkResults = ref({}) // id -> { ok, message } const addError = ref(null) +// An extraction held up on a naming question. `naming` is what the backend +// found in the archive (entries, one question per offending character, and the +// characters an answer may not contain), `namingArchive` and `namingInto` the +// archive and destination the question is about, `answers` what the user has +// typed so far, and `nameProblem` what came back from an attempt that was +// refused. Nothing is on disk while these are set: the backend settles every +// name before it writes the first byte. +const naming = ref(null) +const namingArchive = ref(null) +const namingInto = ref(null) +const answers = ref({}) +const nameProblem = ref(null) + const destinationLabel = computed(() => labelFor(sources.value, destination.value)) const serverUrl = computed(() => urlFor(sources.value, destination.value)) const isRemote = computed(() => serverUrl.value !== null) @@ -48,6 +79,58 @@ const isRemote = computed(() => serverUrl.value !== null) const levelHint = computed(() => levelHintFor(level.value)) const levelDisabled = computed(() => format.value === 'tar') +// A remote run is compressed on the server and comes back as finished bytes +// this app never described, so there is no list of expected entries here to +// check them against. The row stays on screen, dimmed, rather than +// disappearing the way the destination picker does in extract mode: the picker +// goes because the whole feature does, while this one is the direct +// consequence of the choice the user just made one row above, and a control +// that vanishes under the pointer explains nothing. +const verifyDisabled = computed(() => isRemote.value) +// What is really asked for, and what the box shows. Keeping the preference in +// its own ref means picking a server does not silently forget it, and reading +// the effective value here means a disabled box can never send `true`. +const verifyRequested = computed(() => verifyContents.value && !verifyDisabled.value) +const verifyNote = computed(() => + verifyNoteFor({ + checked: verifyRequested.value, + format: format.value, + remote: isRemote.value, + }) +) + +// Checked as it is typed, from the set the backend sent, so an answer that +// cannot be written is refused before the user commits to it rather than after +// a round trip. The backend refuses it again on its own account. +const answerErrors = computed(() => { + if (!naming.value) return {} + const errors = {} + for (const { character } of naming.value.characters) { + const problem = replacementError( + answers.value[character] ?? '', + naming.value.rejectedInReplacement + ) + if (problem) errors[character] = problem + } + return errors +}) +const answersOk = computed(() => Object.keys(answerErrors.value).length === 0) + +// The problems with no character to replace: one line each saying what will be +// done about them, deduplicated because the same sentence for the same entry is +// one thing to read, not two. +const adjustments = computed(() => { + if (!naming.value) return [] + const notes = [] + for (const entry of naming.value.entries) { + for (const problem of entry.problems) { + const note = adjustmentNote(problem, entry.entry) + if (note && !notes.includes(note)) notes.push(note) + } + } + return notes +}) + function selectDestination(value) { destination.value = value saveDestination(value) @@ -89,6 +172,10 @@ async function checkSource(source) { async function pick(path) { error.value = null result.value = null + // A question asked about the previous archive is not a question about this + // one. A drop lands even while the sheet is up, and answering it afterwards + // would apply one archive's replacements to another's names. + closeNaming() inputPath.value = path inputName.value = baseName(path) try { @@ -110,6 +197,7 @@ function reset() { isDir.value = false result.value = null error.value = null + closeNaming() } async function browse() { @@ -143,6 +231,10 @@ async function compress() { format: format.value, level: level.value, server: serverUrl.value, + // Always the effective value, never the raw preference: the checkbox is + // disabled for a remote run, so asking for a check the server side + // cannot make would be this app lying to itself. + verify: verifyRequested.value, // The save dialog asks before handing back a path that is already // taken, on every platform, so reaching here means the user has // already agreed to replace it. The backend still refuses the cases @@ -162,7 +254,11 @@ async function extract() { if (!inputPath.value || processing.value) return error.value = null - const { dir } = dirOf(inputPath.value) + // Held from here on, rather than read back from `inputPath` at each step: a + // drop can change the selection while a dialog is open, and an extraction + // that started on one archive must not finish on another. + const archive = inputPath.value + const { dir } = dirOf(archive) const outputDir = await open({ directory: true, multiple: false, @@ -173,18 +269,97 @@ async function extract() { processing.value = true try { - const files = await invoke('extract_archive', { - archive: inputPath.value, - outputDir, - }) - result.value = { files, dir: outputDir } + // Ask the archive what it holds before writing any of it. A tarball built + // on Linux can carry names this computer cannot save, and the user is the + // only one who can say what they should become; this reads the listing + // only, so nothing is created if the answer is "ask them". + const inspection = await invoke('unwritable_names', { archive }) + if (inspection.entries.length > 0) { + answers.value = initialAnswers(inspection.characters) + nameProblem.value = null + namingArchive.value = archive + namingInto.value = outputDir + naming.value = inspection + return + } + await runExtraction(archive, outputDir, {}) + } catch (e) { + error.value = String(e) + } finally { + processing.value = false + } +} + +/** + * Extract with these answers, and put the result on screen. + * + * A refusal about a name is not a failure: nothing was written, and the sheet + * stays open on the question so the user can answer it differently. Anything + * else throws and is caught by the caller, which is the error banner's job. + */ +async function runExtraction(archive, outputDir, replacements) { + const outcome = await invoke('extract_archive', { + archive, + outputDir, + replacements, + }) + if (outcome.status === 'nameProblem') { + // The sheet is where a naming question belongs, but it is only open when + // the report found something to ask about. A refusal can still arrive + // without one, because the report and the extractor read the archive in two + // separate passes and can disagree: a listing the first pass could not read + // is reported as "nothing to ask", and then extraction refuses a name. With + // `nameProblem` rendered only inside the sheet, that combination made the + // Extract button do nothing at all: no files, no question, no banner. Send + // it to the banner when there is no sheet to hold it. + if (naming.value) { + nameProblem.value = outcome.message + } else { + error.value = outcome.message + } + return + } + closeNaming() + // The names as written, which is what the backend returns: an entry that had + // to be renamed is listed under the name that is on disk. + result.value = { files: outcome.files, dir: outputDir } +} + +async function confirmNames() { + if (processing.value || !answersOk.value) return + nameProblem.value = null + processing.value = true + try { + await runExtraction( + namingArchive.value, + namingInto.value, + substitutions(naming.value.characters, answers.value) + ) } catch (e) { + // Not a naming question, so the sheet has nothing left to ask: it gets out + // of the way of the error banner. + closeNaming() error.value = String(e) } finally { processing.value = false } } +function cancelNaming() { + // Not while the extraction is in flight: there would be nothing to cancel, + // and closing the sheet would hide the answer it is about to come back with. + if (processing.value) return + closeNaming() +} + +function closeNaming() { + naming.value = null + namingArchive.value = null + namingInto.value = null + answers.value = {} + nameProblem.value = null +} + const canProceed = computed(() => !!inputPath.value && !processing.value) let unlisten = null @@ -276,6 +451,67 @@ onUnmounted(() => { + + +
+
+
+

Names this computer cannot write

+ +
+

+ {{ naming.entries.length }} + {{ naming.entries.length === 1 ? 'entry is named' : 'entries are named' }} + in a way this computer cannot save. Nothing has been extracted yet. + Say what each character should become; every other name is written exactly as the + archive spells it. +

+ +
    +
  • {{ e.entry }}
    • +
  • + +{{ naming.entries.length - 6 }} more +
    • +
+ +
+
+ {{ characterLabel(c.character) }} + + in {{ c.entries }} {{ c.entries === 1 ? 'entry' : 'entries' }} + +
+

{{ faultNote(c.fault) }}

+ +

+ {{ answerErrors[c.character] }} +

+
+ + +
    +
  • {{ note }}
    • +
+ +

{{ nameProblem }}

+ + +
+
+ +
@@ -369,6 +605,20 @@ onUnmounted(() => { +
+ Verifyn/a + +
+

{{ verifyNote }}

+ @@ -1680,21 +1684,30 @@

Run the whole flow

// instead of keeping a second hardcoded copy of the contract. var algorithmParam = null; var levelParam = null; + var verifyParam = null; compressOp.params.forEach(function (param) { if (param.name === 'algorithm') algorithmParam = param; if (param.name === 'level') levelParam = param; + if (param.name === 'verify') verifyParam = param; }); - var algorithmSchema = (algorithmParam && algorithmParam.schema) || {}; - var choices = Array.isArray(algorithmSchema.enum) ? algorithmSchema.enum : ['zip']; - var algorithmSelect = clear(byId('flow-algorithm')); - choices.forEach(function (value) { - algorithmSelect.appendChild(el('option', { value: String(value), text: String(value) })); - }); - if (algorithmSchema.default !== undefined) { - algorithmSelect.value = String(algorithmSchema.default); + /** Fill a