diff --git a/apps/cli/src/lib.rs b/apps/cli/src/lib.rs index 945b564..9a2e38a 100644 --- a/apps/cli/src/lib.rs +++ b/apps/cli/src/lib.rs @@ -9,7 +9,7 @@ use clap::{Parser, Subcommand, ValueEnum}; use collapse_core::paths::{inside, same_file}; use collapse_core::{ compress, compress_dir, extract, unwritable_names_with, Algorithm, CharacterFault, NameProblem, - NameReport, NameRules, Substitutions, Verify, + NameReport, NameRules, Verify, }; use thiserror::Error; @@ -105,33 +105,15 @@ pub enum Outcome { 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, + /// "As written" is no longer a distinction: extraction refuses a name + /// it cannot write rather than adjusting it, so these are the names the + /// archive spells. The wording stays because the guarantee is worth + /// stating either way. + files: 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) { @@ -149,11 +131,7 @@ impl Outcome { Outcome::Compressed { output, .. } => { println!("Created {}", output.display()); } - Outcome::Extracted { - output_dir, - files, - adjusted, - } => { + Outcome::Extracted { output_dir, files } => { println!( "Extracted {} file(s) into {}", files.len(), @@ -162,17 +140,6 @@ 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); - } - } } } } @@ -446,41 +413,8 @@ 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() + Ok(Outcome::Extracted { output_dir, files }) } /// Why an archive was refused, which entries are at fault, what is wrong with @@ -512,30 +446,18 @@ pub fn unwritable_entries_message(archive: &Path, report: &NameReport) -> String } } - let _ = write!(message, "\nNothing was extracted."); + let _ = write!( + message, + "\nNothing was extracted, and nothing this command could be told would change that: \ + extraction writes every entry under the name the archive spells or it writes none of \ + them. Extract on a system that can hold these names." + ); 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." - ); + // Named even though there is no answer to give, because it is what + // tells a user whether the archive is unusable here or merely awkward: + // one character across forty entries is a different problem from forty + // characters. + let _ = write!(message, " The characters at fault are {}.", listed(report)); } message } @@ -560,12 +482,12 @@ fn explain(problem: &NameProblem) -> String { 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 name ends in {removed:?}, which this system does not keep, so the file would not \ + be the one the archive names" ), + NameProblem::ReservedDevice { device } => { + format!("{device:?} names a device rather than a file, in every directory") + } } } diff --git a/apps/cli/tests/cli.rs b/apps/cli/tests/cli.rs index 71faacd..b4235e9 100644 --- a/apps/cli/tests/cli.rs +++ b/apps/cli/tests/cli.rs @@ -478,18 +478,12 @@ fn extract_lists_and_writes_files() { out.to_str().unwrap(), ]); match outcome { - Outcome::Extracted { - output_dir, - files, - adjusted, - } => { + Outcome::Extracted { output_dir, files } => { assert_eq!(output_dir, out); + // A name this machine can write is written as it is spelled, which + // is now the only thing extraction does: there is no adjusted-name + // list beside the listing any more, because nothing is adjusted. 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"), } diff --git a/apps/cli/tests/names.rs b/apps/cli/tests/names.rs index c8e27d1..03fe1d1 100644 --- a/apps/cli/tests/names.rs +++ b/apps/cli/tests/names.rs @@ -107,29 +107,37 @@ fn the_refusal_names_every_bad_entry_and_says_what_is_wrong_with_each() { ); } -/// Nothing is half-written, and the user is told where to go. Drop the pointer -/// and the message becomes a dead end. +/// Nothing is half-written, and the user is told the only thing that helps. +/// +/// This used to promise a replacement would unblock it and point at the desktop +/// app as the front end that could ask for one. Both stopped being true when +/// extraction stopped substituting, and a message that sends someone to a +/// dialog that no longer exists is worse than one that says nothing. #[test] -fn the_refusal_says_nothing_was_written_and_where_to_go_next() { +fn the_refusal_says_nothing_was_written_and_what_actually_helps() { let message = windows_refusal("report.zip", &["what?.txt"]); assert!( - message.contains("Nothing was extracted."), + 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}" + message.contains("Extract on a system that can hold these names"), + "the only thing that helps, and it is not something this command can do: {message}" ); assert!( - message.contains("desktop app"), - "the pointer at the one front end that can ask: {message}" + !message.contains("desktop app") && !message.contains("replacement for"), + "nothing may point at an answer nobody can give any more: {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. +/// The characters are still counted, though nothing can be done about them. +/// +/// The count used to say how much of the archive one answer would buy. There +/// are no answers now, and it is still worth saying: one character across forty +/// entries is an archive that is awkward here, forty characters is one that +/// does not belong on this machine at all, and the user decides what to do with +/// that. #[test] fn the_refusal_counts_the_entries_each_character_holds_up() { let message = windows_refusal( @@ -138,7 +146,7 @@ fn the_refusal_counts_the_entries_each_character_holds_up() { ); assert!( - message.contains("replacements for '?' (2 entries) and ':' (1 entry)"), + message.contains("The characters at fault are '?' (2 entries) and ':' (1 entry)"), "both characters, both counts, and a plural that matches: {message}" ); } diff --git a/apps/core/src/compression.rs b/apps/core/src/compression.rs index 9ef50fc..485001e 100644 --- a/apps/core/src/compression.rs +++ b/apps/core/src/compression.rs @@ -9,14 +9,14 @@ mod zip; pub use self::algorithm::Algorithm; pub use self::names::{ CharacterFault, NameError, NameProblem, NameReport, NameRules, OffendingCharacter, - Substitutions, UnwritableEntry, + 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::{refuse_unwritable_names, NamePlan}; +pub(crate) use self::names::refuse_unwritable_names; 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; @@ -378,7 +378,6 @@ pub fn compress_dir( #[non_exhaustive] pub struct ExtractOptions { rules: NameRules, - replacements: Substitutions, } impl ExtractOptions { @@ -394,27 +393,9 @@ impl ExtractOptions { self } - /// **Inert.** Kept so the callers that pass answers still compile while - /// their own naming dialogs are being taken out; nothing reads it. - /// - /// Extraction no longer substitutes anything: a character this filesystem - /// cannot write fails the extraction rather than being replaced, so there - /// is no answer for a caller to supply. Removing this, [`Substitutions`] - /// and the two front-end dialogs that fill it is follow-up work. - pub fn with_replacements(mut self, replacements: Substitutions) -> Self { - self.replacements = replacements; - self - } - pub fn rules(&self) -> NameRules { self.rules } - - /// The answers this was handed. Read by nobody; see - /// [`Self::with_replacements`]. - pub fn replacements(&self) -> &Substitutions { - &self.replacements - } } /// Which algorithm reads this archive, by file extension (not by magic bytes). diff --git a/apps/core/src/compression/names.rs b/apps/core/src/compression/names.rs index 80dbdc0..188d115 100644 --- a/apps/core/src/compression/names.rs +++ b/apps/core/src/compression/names.rs @@ -37,9 +37,6 @@ //! 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; @@ -87,12 +84,6 @@ const DEVICE_PREFIXES: &[&str] = &["COM", "LPT"]; /// 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 @@ -144,28 +135,6 @@ impl NameRules { } } - /// 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. /// @@ -220,135 +189,6 @@ impl NameRules { 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); @@ -429,16 +269,7 @@ pub enum NameProblem { 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, - } - } -} +impl NameProblem {} /// How a filesystem gets a character in a name wrong. #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize)] @@ -526,72 +357,6 @@ impl NameReport { } } -/// 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 @@ -599,51 +364,6 @@ impl FromIterator<(char, String)> for Substitutions { /// 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, - }, - /// The host cannot hold this name exactly as the archive spells it. /// /// The whole extraction stops here. Extraction writes what the archive @@ -661,9 +381,6 @@ pub enum NameError { component: String, problems: Vec, }, - - #[error("{key:?} is not a single character, so there is nothing to replace")] - NotOneCharacter { key: String }, } /// Why one component cannot be written, as a phrase that follows its name. @@ -697,58 +414,6 @@ fn describe(problems: &[NameProblem]) -> String { .join("; and ") } -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. -/// -/// **Always the identity now, and kept only so the write paths do not have to -/// change in the same commit as the policy.** Extraction no longer renames an -/// entry ([`refuse_unwritable_names`]), so nothing constructs a plan that -/// says anything: every backend receives [`Self::identity`] and every -/// [`Self::written_as`] answers `None`. Removing this type, the -/// `extract_*_planned` variants that take it and tar's second write path is -/// follow-up work, deliberately separate because that path carries the -/// containment guard and should not be moved by a commit about naming. -#[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) - } -} - /// Refuse a listing holding a name this filesystem cannot write exactly as the /// archive spells it. /// diff --git a/apps/core/src/compression/sevenz.rs b/apps/core/src/compression/sevenz.rs index 662e62d..b256faa 100644 --- a/apps/core/src/compression/sevenz.rs +++ b/apps/core/src/compression/sevenz.rs @@ -5,7 +5,7 @@ use std::path::Path; use sevenz_rust2::lzma::LZMA2Options; use sevenz_rust2::{SevenZArchiveEntry, SevenZMethod, SevenZMethodConfiguration, SevenZWriter}; -use super::{CompressionError, NamePlan, Verify}; +use super::{CompressionError, Verify}; /// API level (1–5) → LZMA2 preset (1–9). const SEVENZ_PRESETS: [u32; 5] = [1, 3, 5, 7, 9]; @@ -250,16 +250,8 @@ pub(crate) fn list_7z_entries(archive: &Path) -> Result, Compression read_7z_entries(archive, Verify::Index) } +/// Extract every entry under the name the archive spells for it. 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()?; @@ -286,9 +278,8 @@ pub(crate) fn extract_7z_planned( "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); + // See `extract_zip`: `ensure_inside` below is the backstop for + // what the lexical rule cannot reach. let dest = canonical_output.join(&rel); // The callback can only fail with sevenz's own error type, so the diff --git a/apps/core/src/compression/tar.rs b/apps/core/src/compression/tar.rs index 8f1fb06..ea85fef 100644 --- a/apps/core/src/compression/tar.rs +++ b/apps/core/src/compression/tar.rs @@ -4,7 +4,7 @@ use std::path::{Component, Path, PathBuf}; use tar::{Archive, Builder, EntryType}; -use super::{CompressionError, NamePlan, Verify}; +use super::{CompressionError, Verify}; /// The sentence at the bottom of tar's error chain. /// @@ -163,25 +163,16 @@ pub(crate) fn list_tar_entries(archive: &Path) -> Result, Compressio 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. +/// Extract every entry under the name the archive spells for 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> { +/// One write path, and it is `unpack_in`: the traversal guard tar has always +/// used, whose canonicalizing containment check is what stops a write from +/// following a symlink already sitting in the output directory. There used to +/// be a second, for entries extraction had renamed to fit the host, which +/// could not go through `unpack_in` because that derives the destination from +/// the entry's own name. Nothing is renamed any more, so that branch is gone +/// and with it the containment check it had to repeat by hand. +pub fn extract_tar(archive: &Path, output_dir: &Path) -> Result, CompressionError> { fs::create_dir_all(output_dir)?; let canonical_output = output_dir.canonicalize()?; @@ -212,9 +203,9 @@ pub(crate) fn extract_tar_planned( } // 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. + // root or a drive stripped. `..` is refused here as well as by + // unpack_in's `Ok(false)`, which is belt and braces now that the branch + // needing its own guard is gone, and is what names the entry. let natural = normal_path(&name).ok_or_else(|| { CompressionError::Failed(format!("Path traversal detected in archive entry: {name}")) })?; @@ -224,54 +215,27 @@ pub(crate) fn extract_tar_planned( continue; } - match plan.written_as(&name) { - None => { - // `unpack_in` derives the destination itself, so the failure it - // reports names a path and nothing else: `failed to unpack - // \`/…/out/a.txt/b.txt\``, with no clue which of an archive's - // entries was at fault. That is what issue #64 was about, and - // the fix reached zip and 7z but not this branch, which is the - // one nearly every archive takes (issue #93). - // - // The call itself is untouched. It is the traversal guard tar - // has always used, and its canonicalizing containment check is - // what stops a write from following a symlink already sitting - // in the output directory. Only its error is dressed. - let unpacked = entry.unpack_in(&canonical_output).map_err(|e| { - super::entry_error(&name, &canonical_output.join(&natural), root_cause(e)) - })?; - 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()); - } - } + // `unpack_in` derives the destination itself, so the failure it + // reports names a path and nothing else: `failed to unpack + // \`/…/out/a.txt/b.txt\``, with no clue which of an archive's + // entries was at fault. That is what issue #64 was about, and + // the fix reached zip and 7z but not this branch, which is the + // one nearly every archive takes (issue #93). + // + // The call itself is untouched. It is the traversal guard tar + // has always used, and its canonicalizing containment check is + // what stops a write from following a symlink already sitting + // in the output directory. Only its error is dressed. + let unpacked = entry.unpack_in(&canonical_output).map_err(|e| { + super::entry_error(&name, &canonical_output.join(&natural), root_cause(e)) + })?; + 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()); } } Ok(extracted) diff --git a/apps/core/src/compression/zip.rs b/apps/core/src/compression/zip.rs index a13283f..f60f8a8 100644 --- a/apps/core/src/compression/zip.rs +++ b/apps/core/src/compression/zip.rs @@ -5,7 +5,7 @@ use std::path::Path; use zip::write::SimpleFileOptions; use zip::{CompressionMethod, ZipWriter}; -use super::{CompressionError, NamePlan, Verify}; +use super::{CompressionError, Verify}; /// API level (1–5) → Deflate compresslevel (1–9). const ZIP_LEVELS: [i64; 5] = [1, 3, 5, 7, 9]; @@ -144,19 +144,8 @@ pub(crate) fn list_zip_entries(archive: &Path) -> Result, Compressio .collect()) } +/// Extract every entry under the name the archive spells for it. 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()))?; @@ -181,12 +170,9 @@ pub(crate) fn extract_zip_planned( 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); + // `sanitize_entry_path` is a lexical rule, so `ensure_inside` below is + // the backstop for what it cannot reach: a symlink already sitting in + // the output directory. let dest = canonical_output.join(&rel); if entry.is_dir() { diff --git a/apps/core/src/lib.rs b/apps/core/src/lib.rs index cb8f713..48ddc77 100644 --- a/apps/core/src/lib.rs +++ b/apps/core/src/lib.rs @@ -7,5 +7,5 @@ pub mod paths; 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, + NameReport, NameRules, OffendingCharacter, UnwritableEntry, Verify, }; diff --git a/apps/core/tests/names.rs b/apps/core/tests/names.rs index 304adbe..1d5cb70 100644 --- a/apps/core/tests/names.rs +++ b/apps/core/tests/names.rs @@ -17,7 +17,7 @@ use std::path::{Path, PathBuf}; use collapse_core::compression::{ extract_7z, extract_tar, extract_zip, CharacterFault, NameError, NameProblem, NameReport, - NameRules, Substitutions, + NameRules, }; use collapse_core::{ extract, extract_with, unwritable_names_with, CompressionError, ExtractOptions, @@ -118,10 +118,8 @@ fn sorted(mut names: Vec) -> Vec { } /// 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) +fn as_windows() -> ExtractOptions { + ExtractOptions::new().with_rules(NameRules::windows()) } /// The message of the refusal an unwritable entry must produce. @@ -301,153 +299,6 @@ fn the_host_rules_are_this_platform_s_rules() { // ------------------------------------------------------------- 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] @@ -477,24 +328,19 @@ fn the_report_asks_about_each_character_once_and_says_how_many_entries_carry_it( ); } +/// `characters` counts only the faults that are about a character, which is +/// what the CLI's refusal message groups by. A trailing dot and a device name +/// are entries with a problem and nothing to group. #[test] -fn the_report_separates_the_questions_from_the_stated_adjustments() { +fn only_a_character_fault_reaches_the_characters_half_of_the_report() { 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.entries.len(), 3, "all three are refused"); assert_eq!( report.characters.len(), 1, - "a trailing dot and a device name must not produce a text field" + "but only the `?` is a character anyone could name" ); assert_eq!( report.entries[2].problems, @@ -604,10 +450,7 @@ fn every_fault_stops_the_whole_archive_the_same_way() { let out = dir.path().join("out"); let context = format!("{format}, {fault}"); - let message = refusal( - extract_with(&archive, &out, &as_windows(Substitutions::new())), - &context, - ); + let message = refusal(extract_with(&archive, &out, &as_windows()), &context); assert!(message.contains(bad), "{context}: {message}"); assert!( @@ -645,10 +488,7 @@ fn a_colon_entry_is_refused_and_its_neighbour_is_never_written() { ); let out = dir.path().join("out"); - let message = refusal( - extract_with(&archive, &out, &as_windows(Substitutions::new())), - format, - ); + let message = refusal(extract_with(&archive, &out, &as_windows()), format); assert!(message.contains("notes.txt:hidden"), "{format}: {message}"); assert!(message.contains(':'), "{format}: {message}"); @@ -682,8 +522,7 @@ fn two_entries_that_used_to_collide_are_refused_for_their_own_names() { ); let out = dir.path().join("out"); - let answers = Substitutions::new().with('?', "_").with('*', "_"); - let message = refusal(extract_with(&archive, &out, &as_windows(answers)), format); + let message = refusal(extract_with(&archive, &out, &as_windows()), format); assert!(message.contains("a?b.txt"), "{format}: {message}"); assert!( @@ -711,10 +550,7 @@ fn a_trailing_dot_is_refused_rather_than_folded_onto_the_name_beside_it() { ); let out = dir.path().join("out"); - let message = refusal( - extract_with(&archive, &out, &as_windows(Substitutions::new())), - "zip", - ); + let message = refusal(extract_with(&archive, &out, &as_windows()), "zip"); assert!(message.contains("notes.txt."), "{message}"); assert!(files_under(&out).is_empty()); @@ -734,7 +570,7 @@ fn an_archive_that_already_names_one_entry_twice_still_extracts() { ); let out = dir.path().join("out"); - let written = extract_with(&archive, &out, &as_windows(Substitutions::new())).unwrap(); + let written = extract_with(&archive, &out, &as_windows()).unwrap(); assert_eq!( written, @@ -743,35 +579,6 @@ fn an_archive_that_already_names_one_entry_twice_still_extracts() { assert_eq!(fs::read(out.join("notes.txt")).unwrap(), b"second"); } -#[test] -fn an_answer_no_longer_rescues_an_entry_the_host_cannot_write() { - // What this test used to guarantee — that a replacement the host could not - // write was refused up front, before the archive was even opened — has no - // subject left: nothing validates an answer, because nothing applies one. - // - // It is kept, turned around to face the policy itself, because that is the - // part most likely to be quietly undone. `?` answered with `_` is the most - // reasonable answer anyone could give to the most ordinary question this - // ever asked, and it must still extract nothing at all. - for format in FORMATS { - let dir = tempfile::TempDir::new().unwrap(); - let archive = archive_with(dir.path(), format, &[("what?.txt", b"question")]); - let out = dir.path().join("out"); - - let message = refusal( - extract_with( - &archive, - &out, - &as_windows(Substitutions::new().with('?', "_")), - ), - format, - ); - - assert!(message.contains("what?.txt"), "{format}: {message}"); - assert!(files_under(&out).is_empty(), "{format}"); - } -} - #[test] fn extraction_with_no_options_leaves_ordinary_names_alone() { // `extract` is `extract_with` with the host's rules and no answers, and on @@ -884,44 +691,6 @@ fn a_failing_entry_names_itself_and_its_destination() { // ------------------------------------- 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. /// diff --git a/apps/core/tests/security.rs b/apps/core/tests/security.rs index f84ca8f..57b8156 100644 --- a/apps/core/tests/security.rs +++ b/apps/core/tests/security.rs @@ -9,7 +9,7 @@ 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, + NameRules, }; use collapse_core::{extract, extract_with, Algorithm, CompressionError, ExtractOptions, Verify}; use sevenz_rust2::{SevenZArchiveEntry, SevenZWriter}; @@ -626,47 +626,19 @@ fn no_format_writes_an_entry_name_with_a_colon_as_a_stream() { } } -#[test] -fn a_replacement_cannot_carry_an_entry_out_of_the_output_directory() { - // The answer the user gave used to be pushed inside a name that containment - // had already cleared, so an unchecked replacement was a traversal by the - // back door: `?` answered with `../..` would write above the output - // directory with nothing left to notice it. `check_replacements` stood in - // front of that. - // - // The hole is now closed a layer earlier and by construction rather than by - // a check: an answer is never applied to anything, so it cannot reach a - // path at all, and the entry that would have carried it is refused for its - // own name. The hostile answers are still offered here, and must still - // change nothing. - 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" - ); - } - } -} +// `a_replacement_cannot_carry_an_entry_out_of_the_output_directory` stood here. +// +// It offered `?` answers like `../../escape` and proved none of them moved an +// entry out of the output directory, because the answer was pushed inside a +// name containment had already cleared — a traversal by the back door that +// `check_replacements` existed to stop. +// +// The hole is now closed by construction rather than by a check: there is no +// way to supply a replacement, so nothing can be pushed into a name at all. +// `ExtractOptions` has no such method and `Substitutions` no longer exists, so +// this cannot be tested at runtime — the code that would fail it does not +// compile. Recorded here rather than deleted silently, because the guarantee is +// still one this crate makes. // -- compression: archiving a directory must never follow a symlink out of // the tree (all three formats skip symlinks) -- diff --git a/apps/desktop/src-tauri/src/commands.rs b/apps/desktop/src-tauri/src/commands.rs index 70f8d13..53c1eec 100644 --- a/apps/desktop/src-tauri/src/commands.rs +++ b/apps/desktop/src-tauri/src/commands.rs @@ -24,16 +24,12 @@ //! 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_with, Algorithm, CompressionError, ExtractOptions, NameRules, - Verify, + compress, compress_dir, extract_with, Algorithm, 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 @@ -179,110 +175,27 @@ pub fn check_server(url: String) -> Result<(), String> { collapse_remote::check_health(&url).map_err(|e| e.to_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. +/// Extract an archive into `output_dir`. /// -/// 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. +/// Returns the names as written, which are the names the archive spells: a name +/// this host cannot hold fails the whole extraction rather than being adjusted +/// to fit, so there is nothing for the user to answer and nothing for this +/// command to take besides where the archive is and where it should go. /// -/// 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. +/// A naming refusal comes back as an ordinary `Err`. It used to have a variant +/// of its own so the dialog could stay open on it; there is no dialog now, and +/// what is left is a message the user reads in the same place as any other +/// failure. #[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 { +pub fn extract_archive(archive: String, output_dir: String) -> Result, String> { let archive_path = PathBuf::from(&archive); if !archive_path.exists() { return Err(format!("Not found: {archive}")); } let output = PathBuf::from(&output_dir); - 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()), - } + // The host's rules, spelled out rather than left to the default, so the + // machine being judged is visibly this one. + let options = ExtractOptions::new().with_rules(NameRules::host()); + extract_with(&archive_path, &output, &options).map_err(|e| e.to_string()) } diff --git a/apps/desktop/src-tauri/src/lib.rs b/apps/desktop/src-tauri/src/lib.rs index 74b9c0d..59ccc71 100644 --- a/apps/desktop/src-tauri/src/lib.rs +++ b/apps/desktop/src-tauri/src/lib.rs @@ -1,10 +1,8 @@ //! Tauri backend for the Collapse desktop app: the app wiring only. The -//! 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. +//! commands themselves live in [`commands`] and the path predicates they rely +//! on in [`paths`], both public so `tests/` can drive them directly. pub mod commands; -pub mod names; pub mod paths; #[cfg_attr(mobile, tauri::mobile_entry_point)] @@ -16,7 +14,6 @@ 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 deleted file mode 100644 index d66d887..0000000 --- a/apps/desktop/src-tauri/src/names.rs +++ /dev/null @@ -1,96 +0,0 @@ -//! 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. - /// - /// Nothing downstream re-checks it any more, and nothing needs to: core no - /// longer substitutes anything, so an answer cannot reach a file name. This - /// whole exchange is inert until the dialog is taken out. - 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/tests/commands.rs b/apps/desktop/src-tauri/tests/commands.rs index d7187de..68d0e19 100644 --- a/apps/desktop/src-tauri/tests/commands.rs +++ b/apps/desktop/src-tauri/tests/commands.rs @@ -11,11 +11,10 @@ //! 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, Extraction}; +use collapse_desktop::commands::{compress_path, extract_archive, is_directory}; use tempfile::TempDir; /// The three formats the UI offers. The wire spelling doubles as the archive @@ -95,21 +94,14 @@ fn compress_local_with( ) } -/// 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. +/// Extract, which for every archive in this file simply works: none of them +/// carries a name this host cannot write, and such a name is a plain `Err` +/// now rather than an outcome of its own. fn extract_to(archive: &Path, output_dir: &Path) -> Result, String> { - match extract_archive( + 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 diff --git a/apps/desktop/src-tauri/tests/ipc.rs b/apps/desktop/src-tauri/tests/ipc.rs index ea82fdc..f45c3ed 100644 --- a/apps/desktop/src-tauri/tests/ipc.rs +++ b/apps/desktop/src-tauri/tests/ipc.rs @@ -63,12 +63,11 @@ 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; 5] = [ +const BASELINE: [&str; 4] = [ "check_server", "compress_path", "extract_archive", "is_directory", - "unwritable_names", ]; /// Quote characters that open a string literal, per language. Needed by every @@ -954,16 +953,12 @@ fn every_command_that_can_block_is_marked_async() { // 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] = [ + const MUST_NOT_BLOCK: [(&str, &str); 3] = [ ( "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", @@ -1214,7 +1209,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)]); 5] = [ + let expected: [(&str, &[(&str, &str)]); 4] = [ ("check_server", &[("url", "String")]), ( "compress_path", @@ -1230,18 +1225,9 @@ fn command_signatures_are_pinned_with_their_types() { ), ( "extract_archive", - &[ - ("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"), - ], + &[("archive", "String"), ("output_dir", "String")], ), ("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 index 8540d25..6f105e0 100644 --- a/apps/desktop/src-tauri/tests/names.rs +++ b/apps/desktop/src-tauri/tests/names.rs @@ -1,42 +1,30 @@ -//! 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. +//! Entry names this computer cannot write, at the desktop's own boundary. //! -//! Two halves, and the split is deliberate. +//! This file used to drive a conversation: `unwritable_names` reported what an +//! archive held, a dialog asked the user for a character to put in its place, +//! and `extract_archive` took the answers. None of that exists any more — +//! extraction refuses a name it cannot write rather than negotiating one — so +//! what is left to guard is narrower and, being narrower, worth stating +//! exactly. //! -//! 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. +//! **The refusal has to reach the user with its reason attached.** The command +//! surface is the last place that can be lost: core builds a message naming the +//! entry, the component and the fault, and if this layer flattened it to +//! "extraction failed" the user would be told an archive is broken when it is +//! merely foreign. On Unix the whole alphabet of such names is one character, +//! the NUL byte, which is why the cases below are `#[cfg(unix)]` — their +//! fixture is Unix-specific, not their subject. `apps/core/tests/names.rs` +//! covers the Windows rules from any machine, since the rules are data. -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 collapse_desktop::commands::extract_archive; 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(); @@ -54,438 +42,69 @@ 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()); -} +/// An entry name this machine genuinely refuses. The NUL byte cannot cross the +/// libc boundary, so no Unix filesystem can hold one. +#[cfg(unix)] +const UNWRITABLE_HERE: &str = "bad\u{0}name.txt"; #[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. +fn an_ordinary_archive_extracts_and_lists_what_it_wrote() { 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(); + let files = extract_archive(text(&archive), text(&out)).expect("an ordinary archive extracts"); - 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!(files, ["notes.txt"]); assert_eq!(fs::read(out.join("notes.txt")).unwrap(), b"hello"); } -#[test] -fn a_hostile_answer_changes_nothing_because_no_answer_is_applied() { - // This used to be the guard on the answer itself: `?` replaced with - // `../escaped` would have carried an entry out of the output directory, so - // the ruleset refused the answer before the archive was opened. - // - // There is no answer to refuse any more. Nothing in this archive is - // unwritable, so it extracts, and the point is what does *not* happen: the - // replacement reaches no name, creates no directory and moves nothing. A - // regression here would show up as `escaped` existing somewhere. - let dir = TempDir::new().unwrap(); - let archive = zip_with(dir.path(), &[("notes.txt", b"hello")]); - let out = dir.path().join("out"); - - let outcome = extract_answering(&archive, &out, &[("?", "../escaped")]).unwrap(); - - assert_eq!(written(outcome), ["notes.txt"]); - assert_eq!(files_under(&out), ["notes.txt"]); - assert!( - !dir.path().join("escaped").exists() && !out.join("escaped").exists(), - "the answer reached a path: {:?}", - files_under(dir.path()) - ); -} - -#[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() { +fn a_name_this_computer_cannot_write_is_refused_with_its_reason() { 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 no_answer_rescues_a_name_this_computer_cannot_write() { - // Both halves of what the dialog used to offer, and neither works now: a - // character to put in its place, and an empty answer meaning "just drop - // it". The name the archive gave is the only name that may be written, so - // an entry holding a NUL does not arrive under some other spelling — it - // does not arrive. - // - // This is the test to look at if the dialog is ever wired back up by - // accident: it is the one that says the answers are inert. - for answer in ["_", ""] { - let dir = TempDir::new().unwrap(); - let archive = zip_with(dir.path(), &[(UNWRITABLE_HERE, b"hello")]); - let out = dir.path().join("out"); - - let problem = refusal(extract_answering(&archive, &out, &[("\u{0}", answer)]).unwrap()); - - assert!( - problem.contains("cannot be written on this system"), - "answer {answer:?}: {problem}" - ); - assert!( - !out.exists(), - "answer {answer:?} wrote {:?}", - files_under(&out) - ); - } -} - -#[cfg(unix)] -#[test] -fn a_name_this_computer_cannot_write_stops_before_anything_is_written() { - // The other entry is perfectly writable and still does not get written: - // every name is judged from the listing before the first byte, so a user is - // never left with half an archive and no way to tell which half. - 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()); + let message = extract_archive(text(&archive), text(&out)).expect_err("the name is refused"); assert!( - problem.contains("cannot be written on this system"), - "{problem}" + message.contains("cannot be written on this system"), + "{message}" ); - assert!(problem.contains("bad"), "the entry names itself: {problem}"); + assert!(message.contains("bad"), "the entry names itself: {message}"); assert!( - !out.exists(), - "an unwritable name wrote {:?}", - files_under(&out) + message.contains("refuses in a file name"), + "and the reason travels, which matters most here: a NUL prints as \ + nothing, so an entry named without its fault looks fine: {message}" ); } #[cfg(unix)] #[test] -fn the_refusal_says_which_character_is_the_problem() { - // The reason has to travel, because the dialog renders it and it is all the - // user gets: naming the entry without naming the character leaves them - // staring at a file name that looks fine, since a NUL prints as nothing. +fn the_writable_entry_beside_it_is_not_written_either() { + // All of the archive or none of it, seen from the surface the app calls. + // `fine.txt` is ordinary and must still not appear: a user who is told an + // extraction failed should not find half of one on disk. let dir = TempDir::new().unwrap(); - let archive = zip_with(dir.path(), &[(UNWRITABLE_HERE, b"hello")]); + 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()); + extract_archive(text(&archive), text(&out)).expect_err("the archive is refused"); - assert!(problem.contains("refuses in a file name"), "{problem}"); - assert!( - problem.contains("\\0"), - "the character is shown escaped: {problem}" - ); - assert!(!out.exists()); + assert!(!out.exists(), "nothing may be written before the refusal"); } -#[cfg(unix)] #[test] -fn an_entry_beside_the_name_it_would_have_taken_is_still_just_refused() { - // This was the collision case: answering the NUL with `_` made this entry - // into `bad_name.txt`, which the archive already holds, and one of the two - // would have been written over the other. Both names went into the refusal - // so the answer could be changed. - // - // Nothing is renamed now, so the two names stay two names and there is no - // collision to find. The archive is refused for the NUL alone, and - // `bad_name.txt` — an entry this host writes perfectly well — is not - // written either, which is the part worth keeping. +fn a_missing_archive_is_reported_by_path_rather_than_by_the_extractor() { 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 missing = dir.path().join("nowhere.zip"); - let problem = refusal(extract_answering(&archive, &out, &[("\u{0}", "_")]).unwrap()); + let message = extract_archive(text(&missing), text(&dir.path().join("out"))) + .expect_err("a missing archive fails"); - assert!( - problem.contains("cannot be written on this system"), - "{problem}" - ); - assert!(!out.exists(), "wrote {:?}", files_under(&out)); + assert!(message.contains("Not found"), "{message}"); } diff --git a/apps/desktop/src-tauri/tests/remote.rs b/apps/desktop/src-tauri/tests/remote.rs index 4cf8c60..20dfd2e 100644 --- a/apps/desktop/src-tauri/tests/remote.rs +++ b/apps/desktop/src-tauri/tests/remote.rs @@ -16,12 +16,11 @@ //! 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, Extraction}; +use collapse_desktop::commands::{check_server, compress_path, extract_archive}; // ------------------------------------------------------------------ harness -- @@ -220,13 +219,9 @@ 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)> { - // 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:?}"); - }; + // These archives are built from real files on this machine, so every name + // in them is one this machine can write. + let files = extract_archive(text(archive), text(into)).expect("the archive extracts cleanly"); let mut files: Vec = files .into_iter() .map(|name| name.replace('\\', "/")) diff --git a/apps/desktop/src/App.vue b/apps/desktop/src/App.vue index 13ba606..fe2bf19 100644 --- a/apps/desktop/src/App.vue +++ b/apps/desktop/src/App.vue @@ -10,14 +10,6 @@ import { levelHint as levelHintFor, verifyNote as verifyNoteFor, } from './paths.js' -import { - adjustmentNote, - characterLabel, - faultNote, - initialAnswers, - replacementError, - substitutions, -} from './names.js' import { LOCAL, labelFor, @@ -59,19 +51,6 @@ 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) @@ -99,37 +78,7 @@ const verifyNote = computed(() => }) ) -// 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 @@ -172,10 +121,6 @@ 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 { @@ -197,7 +142,6 @@ function reset() { isDir.value = false result.value = null error.value = null - closeNaming() } async function browse() { @@ -269,20 +213,7 @@ async function extract() { processing.value = true try { - // 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, {}) + await runExtraction(archive, outputDir) } catch (e) { error.value = String(e) } finally { @@ -291,73 +222,15 @@ async function extract() { } /** - * Extract with these answers, and put the result on screen. + * Extract, 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. + * A name this computer cannot write fails the whole extraction and arrives as + * an ordinary error: nothing is on disk, and there is nothing to ask, so it + * belongs in the banner with every other failure. */ -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 +async function runExtraction(archive, outputDir) { + const files = await invoke('extract_archive', { archive, outputDir }) + result.value = { files, dir: outputDir } } const canProceed = computed(() => !!inputPath.value && !processing.value) @@ -451,66 +324,6 @@ 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 }}

- - -
-
-
@@ -865,84 +678,6 @@ main { .add-source input:focus { outline: none; border-color: var(--accent); } .add-error { font-size: 0.74rem; color: var(--danger); } -/* ---- unwritable names sheet ---- */ -/* Same sheet as the servers panel, so the two modals read as one idea. */ - -.unwritable { - list-style: none; - display: flex; - flex-direction: column; - gap: 2px; - padding: 9px 11px; - border: 1px solid var(--border); - border-radius: var(--r-sm); - background: var(--surface-2); - font-size: 0.74rem; - color: var(--muted); -} -.unwritable li { word-break: break-all; } -.unwritable .more { color: var(--faint); } - -.answer { - display: flex; - flex-direction: column; - gap: 5px; - padding: 10px 11px; - border: 1px solid var(--border); - border-radius: var(--r-sm); - background: var(--surface-2); -} -.answer-head { display: flex; align-items: baseline; gap: 8px; } - -/* The character itself, big enough to be unmistakable: the whole question is - "what should THIS become", and a `?` lost in a sentence does not ask it. */ -.offender { - font-family: var(--font); - font-size: 0.95rem; - font-weight: 700; - color: var(--accent); - background: var(--accent-dim); - border-radius: 6px; - padding: 1px 8px; -} -.answer-count { font-size: 0.71rem; color: var(--faint); } -.answer-why { font-size: 0.73rem; line-height: 1.45; color: var(--muted); } - -.answer-field { - font-family: var(--font); - font-size: 0.82rem; - color: var(--text); - background: var(--cream); - border: 1px solid var(--border-2); - border-radius: var(--r-sm); - padding: 8px 10px; -} -.answer-field:focus { outline: none; border-color: var(--accent); } -.answer-error { font-size: 0.72rem; line-height: 1.4; color: var(--danger); } - -.adjustments { - list-style: none; - display: flex; - flex-direction: column; - gap: 6px; - font-size: 0.73rem; - line-height: 1.45; - color: var(--muted); -} - -/* A refusal that arrived from the backend (two entries landing on one name, an - answer that leaves no name at all). Louder than a per-field message: it is - about the set of answers rather than one of them. */ -.name-problem { - padding: 9px 11px; - border-radius: var(--r-sm); - background: var(--danger-dim); - border: 1px solid rgba(158, 59, 42, 0.18); - color: var(--danger); - font-size: 0.76rem; - line-height: 1.45; -} - .work { display: flex; flex-direction: column; gap: 14px; flex: 1; } .drop { diff --git a/apps/desktop/src/names.js b/apps/desktop/src/names.js deleted file mode 100644 index d38b6e0..0000000 --- a/apps/desktop/src/names.js +++ /dev/null @@ -1,117 +0,0 @@ -// The wording and the small pure decisions behind the "names this computer -// cannot write" dialog, split out of App.vue so they can be unit-tested. The -// dialog itself cannot be seen on a Mac (macOS writes every one of these names -// happily), so this file plus tests/names.test.js is where its behaviour is -// actually verified. -// -// There are NO naming rules here. What a file name may contain is -// `collapse_core::NameRules`' answer and nobody else's: the `rejected` string -// this file checks against arrives from the Rust side, produced by the very -// ruleset the extraction will be judged by, and the extraction validates the -// answers again before it opens the archive. A second copy of the rules in -// JavaScript would be a copy that drifts. - -/** - * What a character becomes unless the user says otherwise. - * - * One character for every case, deliberately: `_` is what every tool that has - * ever had to do this picks, it is writable everywhere, and it keeps the name - * the same length so nothing looks like it went missing. Clearing the field is - * how a user asks for the character to be dropped instead. - */ -export const DEFAULT_REPLACEMENT = '_' - -/** - * A character as it can be shown on screen. - * - * A control character has no glyph: rendering it raw would put an invisible - * label on a text field and leave the user with nothing to read. Its code - * point is the only honest thing to show. - */ -export function characterLabel(character) { - const code = character.codePointAt(0) - if (code < 0x20 || code === 0x7f) { - return `U+${code.toString(16).toUpperCase().padStart(4, '0')}` - } - return character -} - -/** How a character reads in prose: a space has to say so, or it is invisible. */ -function named(character) { - return character === ' ' ? 'a space' : `"${characterLabel(character)}"` -} - -/** - * Why this computer cannot keep a character, in one sentence. - * - * The two faults are genuinely different, and saying so is the point of the - * split: a rejected character makes the write fail, while a reinterpreted one - * makes it SUCCEED and put the bytes somewhere the user will never find them. - * The second is the more alarming of the two and must not be described as if - * it were a refusal. - */ -export function faultNote(fault) { - if (fault === 'reinterpreted') { - return 'This computer accepts it and then reads it as the start of a hidden stream, so the file would exist under no name at all.' - } - return 'This computer cannot write it in a file name.' -} - -/** - * The sentence for a problem that has no character to replace, or null when - * the problem is a character (which gets a field instead of a sentence). - * - * These two are applied without asking, because there is nothing to ask: a - * name the host would silently mangle is adjusted the way the host would have - * adjusted it. Saying so is all the dialog can usefully do. - */ -export function adjustmentNote(problem, entry) { - if (problem.kind === 'trailingCharacters') { - const characters = [...new Set(problem.removed)].map(named).join(' and ') - const them = problem.removed.length === 1 ? 'it' : 'them' - return `"${entry}" ends in ${characters}, which this computer does not keep. The name is saved without ${them}.` - } - if (problem.kind === 'reservedDevice') { - return `"${entry}" is the "${problem.device}" device in every folder, whatever follows the dot, so a "_" is added to it.` - } - return null -} - -/** - * Why a replacement will not do, or null when it is fine. - * - * `rejected` is the set the Rust side sent: every character this host cannot - * write, plus the path separators. Membership is its answer; this function - * only chooses the wording, so a rule that changes on the Rust side changes - * here with it. An empty replacement is valid and means "drop the character". - */ -export function replacementError(replacement, rejected) { - for (const character of replacement) { - if (!rejected.includes(character)) continue - if (character === '/' || character === '\\') { - return `A replacement cannot contain ${named(character)}: it would move the file into another folder rather than rename it.` - } - return `${named(character)} is a character this computer cannot write in a file name either.` - } - return null -} - -/** The prefilled answers for the characters an archive asks about. */ -export function initialAnswers(characters) { - const answers = {} - for (const { character } of characters) answers[character] = DEFAULT_REPLACEMENT - return answers -} - -/** - * The payload for `extract_archive`, built from the characters this archive - * actually asks about. - * - * Driven by the character list rather than by the answers object, so an answer - * left over from a previous archive cannot ride along into this extraction. - */ -export function substitutions(characters, answers) { - const payload = {} - for (const { character } of characters) payload[character] = answers[character] ?? '' - return payload -} diff --git a/apps/desktop/tests/App.test.js b/apps/desktop/tests/App.test.js index 99b34e8..3f308e6 100644 --- a/apps/desktop/tests/App.test.js +++ b/apps/desktop/tests/App.test.js @@ -28,78 +28,13 @@ const modeButtons = (w) => w.findAll('.modes button') const formatButtons = (w) => w.findAll('.segmented:not(.levels) button') const verifyBox = (w) => w.find('input[type="checkbox"]') -/// The naming sheet: the second modal, so every selector inside it is scoped -/// to `.naming` rather than fighting the servers sheet for `.sheet`. -const namingSheet = (w) => w.find('.naming') -const answerFields = (w) => w.findAll('.naming .answer-field') -const confirmNames = (w) => w.find('.naming .cta') - -/** - * What `unwritable_names` answers when the host cannot write something. - * - * Every case below builds one of these by hand, because the machine running - * this suite is a Mac or a Linux CI runner and both write `what?.txt` without - * complaint: the dialog literally cannot be produced here by extracting a real - * archive. The shape is not invented, it is `NameInspection`'s serialization, - * pinned on the Rust side by `src-tauri/tests/names.rs`. - */ -function inspection({ entries = [], characters = [], rejected = '<>"|?*:/\\' } = {}) { - return { entries, characters, rejectedInReplacement: rejected } -} - -const NOTHING_TO_ASK = inspection() - -/** - * An archive holding two entries a Windows host cannot write, both for the - * same reason: one question, two files behind it. - */ -const A_QUESTION = inspection({ - entries: [ - { - entry: 'logs/what?.txt', - problems: [{ kind: 'character', character: '?', fault: 'rejected' }], - }, - { entry: 'when?.txt', problems: [{ kind: 'character', character: '?', fault: 'rejected' }] }, - ], - characters: [{ character: '?', fault: 'rejected', entries: 2 }], -}) - -/** - * Pick an archive, choose a destination, and get as far as the naming sheet. - * - * `extraction` is what `extract_archive` will answer once the user confirms; - * everything else falls through to the stub switch in `beforeEach`. - */ -async function ask({ report = A_QUESTION, extraction } = {}) { - open - .mockResolvedValueOnce('/Users/me/logs.tar') // browse: pick the archive - .mockResolvedValueOnce('/Users/me/out') // extract: pick the destination - const stub = invoke.getMockImplementation() - invoke.mockImplementation(async (cmd, args) => { - if (cmd === 'unwritable_names') return report - if (cmd === 'extract_archive' && extraction) return extraction - return stub(cmd, args) - }) - - const w = await mountApp() - await modeButtons(w)[1].trigger('click') // Extract mode - await w.find('.drop').trigger('click') // browse - await flushPromises() - await w.find('.work .cta').trigger('click') // "Extract to…" - await flushPromises() - return w -} - beforeEach(() => { vi.clearAllMocks() onDragDropEvent.mockResolvedValue(() => {}) invoke.mockImplementation(async (cmd) => { if (cmd === 'is_directory') return false if (cmd === 'compress_path') return '/Users/me/report.pdf.7z' - if (cmd === 'unwritable_names') return NOTHING_TO_ASK - if (cmd === 'extract_archive') { - return { status: 'extracted', files: ['notes.txt', 'sub/data.bin'] } - } + if (cmd === 'extract_archive') return ['notes.txt', 'sub/data.bin'] if (cmd === 'check_server') return null return null }) @@ -162,14 +97,12 @@ describe('App', () => { await w.find('.cta').trigger('click') // "Extract to…" await flushPromises() + // Where the archive is and where it goes, and nothing else: there is no + // answer to carry any more. expect(invoke).toHaveBeenCalledWith('extract_archive', { archive: '/Users/me/photos.zip', outputDir: '/Users/me/out', - // Nothing to answer, so nothing is substituted. An archive the host can - // write goes straight through, without a dialog in the way. - replacements: {}, }) - expect(namingSheet(w).exists()).toBe(false) expect(w.text()).toContain('Extracted 2 files') }) @@ -337,181 +270,49 @@ describe('App', () => { expect(w.find('.picker').exists()).toBe(false) }) - it('asks before extracting a name this computer cannot write', async () => { - const w = await ask() - - expect(namingSheet(w).exists()).toBe(true) - expect(w.text()).toContain('Names this computer cannot write') - expect(w.text()).toContain('2 entries are named') - expect(w.text()).toContain('logs/what?.txt') - // The whole point of the feature: the archive is not touched until the - // question is answered. A component that extracted first and asked - // afterwards would pass every other assertion here. - expect(invoke).not.toHaveBeenCalledWith('extract_archive', expect.anything()) - // One field for the one character, whatever the number of entries, and - // prefilled so the common case is one click. - expect(answerFields(w)).toHaveLength(1) - expect(answerFields(w)[0].element.value).toBe('_') - }) - - it('extracts with the answers the user gave, and lists what is on disk', async () => { - const w = await ask({ - extraction: { status: 'extracted', files: ['logs/what-.txt', 'when-.txt'] }, - }) - - await answerFields(w)[0].setValue('-') - await confirmNames(w).trigger('click') - await flushPromises() - - expect(invoke).toHaveBeenCalledWith('extract_archive', { - archive: '/Users/me/logs.tar', - outputDir: '/Users/me/out', - replacements: { '?': '-' }, + it('shows a naming refusal in the banner', async () => { + // The dialog that used to hold these is gone: extraction refuses a name it + // cannot write instead of asking what to call it, so the refusal is an + // ordinary failure and the banner is the only place it can land. If it + // stopped arriving, the Extract button would appear to do nothing at all. + open + .mockResolvedValueOnce('/Users/me/logs.tar') + .mockResolvedValueOnce('/Users/me/out') + const stub = invoke.getMockImplementation() + invoke.mockImplementation(async (cmd, args) => { + if (cmd === 'extract_archive') { + throw 'the archive entry "what?.txt" cannot be written on this system' + } + return stub(cmd, args) }) - expect(namingSheet(w).exists()).toBe(false) - // The names as written, never the archive's: `what?.txt` is on no disk - // here, and showing it would send the user looking for a file that is not - // there. - expect(w.text()).toContain('when-.txt') - expect(w.text()).not.toContain('when?.txt') - }) - - it('reads an empty answer as "remove the character"', async () => { - const w = await ask() - - await answerFields(w)[0].setValue('') - await confirmNames(w).trigger('click') - await flushPromises() - - // Empty is an answer, not a missing one: a component that treated a blank - // field as "unanswered" and refused to send it would fail here. - expect(invoke).toHaveBeenCalledWith( - 'extract_archive', - expect.objectContaining({ replacements: { '?': '' } }) - ) - }) - - it('extracts nothing when the question is cancelled', async () => { - const w = await ask() - - await w.find('.naming .ghost').trigger('click') // Cancel - - expect(namingSheet(w).exists()).toBe(false) - expect(invoke).not.toHaveBeenCalledWith('extract_archive', expect.anything()) - expect(w.text()).not.toContain('Extracted') - }) - - it('refuses a replacement this computer cannot write either', async () => { - const w = await ask() - await answerFields(w)[0].setValue('*') - expect(w.find('.answer-error').text()).toContain('cannot write in a file name either') - expect(confirmNames(w).attributes('disabled')).toBeDefined() - - // A separator is refused for its own reason: it would not rename the entry, - // it would move it somewhere else entirely. - await answerFields(w)[0].setValue('../') - expect(w.find('.answer-error').text()).toContain('move the file into another folder') - - await confirmNames(w).trigger('click') + const w = await mountApp() + await modeButtons(w)[1].trigger('click') + await w.find('.drop').trigger('click') await flushPromises() - expect(invoke).not.toHaveBeenCalledWith('extract_archive', expect.anything()) - - // And a good answer clears the way again. - await answerFields(w)[0].setValue('-') - expect(w.find('.answer-error').exists()).toBe(false) - expect(confirmNames(w).attributes('disabled')).toBeUndefined() - }) - - it('brings a collision back into the dialog naming both entries', async () => { - const message = - 'the archive entries "what?.txt" and "what_.txt" would both be written as "what_.txt"; ' + - 'choose a replacement that keeps them apart' - const w = await ask({ extraction: { status: 'nameProblem', message } }) - - await confirmNames(w).trigger('click') + await w.find('.work .cta').trigger('click') await flushPromises() - // Nothing was written, so this is a question and not a failure: the sheet - // stays open on it, the success screen never appears, and the error banner - // (which means "this did not work") is not the thing that says so. - expect(namingSheet(w).exists()).toBe(true) - expect(w.find('.name-problem').text()).toBe(message) - expect(w.text()).not.toContain('Extracted') - expect(w.find('.error').exists()).toBe(false) + expect(w.text()).toContain('cannot be written on this system') }) - it('shows a naming refusal in the banner when no dialog is open to hold it', async () => { - // 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. `nameProblem` is - // rendered only inside the sheet, so this combination used to make the - // Extract button do nothing at all: no files, no question, no banner. - const message = 'the archive entry "x:y.txt" contains \':\' and no replacement for it was given' - const w = await ask({ - report: NOTHING_TO_ASK, - extraction: { status: 'nameProblem', message }, - }) - - expect(namingSheet(w).exists()).toBe(false) - expect(w.find('.error').text()).toContain(message) - expect(w.text()).not.toContain('Extracted') - }) - - it('states the adjustment for a problem with no character to replace', async () => { - // A trailing dot and a device name have nothing to substitute: the host - // would mangle them whatever anyone typed. So they are explained, not - // asked about, and a text field beside them would be a lie. - const w = await ask({ - report: inspection({ - entries: [ - { entry: 'notes.txt.', problems: [{ kind: 'trailingCharacters', removed: '.' }] }, - { entry: 'CON.txt', problems: [{ kind: 'reservedDevice', device: 'CON' }] }, - ], - }), + it('shows a failure that is not about names in the banner too', async () => { + open + .mockResolvedValueOnce('/Users/me/logs.tar') + .mockResolvedValueOnce('/Users/me/out') + const stub = invoke.getMockImplementation() + invoke.mockImplementation(async (cmd, args) => { + if (cmd === 'extract_archive') throw 'IO error: No space left on device' + return stub(cmd, args) }) - expect(answerFields(w)).toHaveLength(0) - expect(w.text()).toContain('"notes.txt." ends in "."') - expect(w.text()).toContain('is the "CON" device in every folder') - - await confirmNames(w).trigger('click') - await flushPromises() - - expect(invoke).toHaveBeenCalledWith( - 'extract_archive', - expect.objectContaining({ replacements: {} }) - ) - }) - - it('abandons the question when another archive is dropped on it', async () => { - const w = await ask() - expect(namingSheet(w).exists()).toBe(true) - - // A drop reaches the webview even with the sheet on screen. Keeping the - // question up would let one archive's answers be applied to another - // archive's names, which is a rename nobody asked for. - const onDrop = onDragDropEvent.mock.calls[0][0] - onDrop({ payload: { type: 'drop', paths: ['/Users/me/other.zip'] } }) + const w = await mountApp() + await modeButtons(w)[1].trigger('click') + await w.find('.drop').trigger('click') await flushPromises() - - expect(namingSheet(w).exists()).toBe(false) - expect(invoke).not.toHaveBeenCalledWith('extract_archive', expect.anything()) - }) - - it('gives way to the error banner when the failure is not about names', async () => { - const w = await ask() - - invoke.mockImplementation(async (cmd) => { - if (cmd === 'extract_archive') throw 'IO error: No space left on device' - return null - }) - await confirmNames(w).trigger('click') + await w.find('.work .cta').trigger('click') await flushPromises() - // A full disk is nothing the dialog can help with, so it gets out of the - // way instead of holding a question the user cannot answer. - expect(namingSheet(w).exists()).toBe(false) expect(w.find('.error').text()).toContain('No space left on device') }) diff --git a/apps/desktop/tests/names.test.js b/apps/desktop/tests/names.test.js deleted file mode 100644 index 669bc4b..0000000 --- a/apps/desktop/tests/names.test.js +++ /dev/null @@ -1,162 +0,0 @@ -import { describe, it, expect } from 'vitest' -import { - DEFAULT_REPLACEMENT, - adjustmentNote, - characterLabel, - faultNote, - initialAnswers, - replacementError, - substitutions, -} from '../src/names.js' - -// The Windows set, which is the only one that makes this dialog appear in -// practice, plus the two separators. It arrives from the Rust side as -// `NameInspection.rejectedInReplacement`; spelled out here so these cases read -// on their own. -const WINDOWS_REJECTED = '<>"|?*:/\\' - -describe('characterLabel', () => { - it('shows an ordinary character as itself', () => { - for (const character of ['?', '*', ':', '<', '|', ' ']) { - expect(characterLabel(character)).toBe(character) - } - }) - - it('names a control character by its code point', () => { - // A raw control character renders as nothing at all, so a field labelled - // with one asks the user about a blank. They do reach this dialog: the NUL - // is the single character Unix itself refuses, and Windows refuses every - // one of U+0000 to U+001F. (U+007F is labelled too, for the same reason, - // though no ruleset here refuses it today.) - expect(characterLabel('\u0000')).toBe('U+0000') - expect(characterLabel('\u0007')).toBe('U+0007') - expect(characterLabel('\u001F')).toBe('U+001F') - expect(characterLabel('\u007F')).toBe('U+007F') - }) -}) - -describe('faultNote', () => { - it('says a rejected character makes the write fail', () => { - expect(faultNote('rejected')).toContain('cannot write it') - }) - - it('says a reinterpreted character makes the file disappear instead', () => { - // The colon is the dangerous one precisely because Windows ACCEPTS it: the - // write succeeds and the bytes land in an alternate data stream. Wording - // this as another refusal would describe the wrong thing entirely. - const note = faultNote('reinterpreted') - expect(note).toContain('accepts it') - expect(note).toContain('hidden stream') - expect(note).not.toContain('cannot write') - }) -}) - -describe('adjustmentNote', () => { - it('has nothing to say about a character, which gets a field instead', () => { - expect( - adjustmentNote({ kind: 'character', character: '?', fault: 'rejected' }, 'what?.txt') - ).toBeNull() - }) - - it('names the entry and the trailing characters that will go', () => { - const note = adjustmentNote({ kind: 'trailingCharacters', removed: '.' }, 'notes.txt.') - expect(note).toBe( - '"notes.txt." ends in ".", which this computer does not keep. The name is saved without it.' - ) - }) - - it('calls a trailing space a space, since it cannot be seen', () => { - // `"x " ends in " "` is a sentence with a hole in it: the quotes are all - // the reader gets. - const note = adjustmentNote({ kind: 'trailingCharacters', removed: ' ' }, 'draft ') - expect(note).toContain('ends in a space') - expect(note).toContain('without it') - }) - - it('describes a run of several trailing characters once each', () => { - const note = adjustmentNote({ kind: 'trailingCharacters', removed: '. ..' }, 'odd. ..') - expect(note).toContain('ends in "." and a space') - expect(note).toContain('without them') - }) - - it('explains a device name without promising a name it has not computed', () => { - const note = adjustmentNote({ kind: 'reservedDevice', device: 'CON' }, 'CON.txt') - expect(note).toContain('"CON.txt" is the "CON" device in every folder') - expect(note).toContain('whatever follows the dot') - }) -}) - -describe('replacementError', () => { - it('accepts an ordinary replacement', () => { - for (const replacement of ['_', '-', 'at', '·', '']) { - expect(replacementError(replacement, WINDOWS_REJECTED)).toBeNull() - } - }) - - it('accepts an empty replacement, which drops the character', () => { - expect(replacementError('', WINDOWS_REJECTED)).toBeNull() - }) - - it('refuses a replacement the host cannot write either', () => { - // "Replace ? with *" is not an answer. Caught here so the user is told - // while typing rather than after a round trip. - const problem = replacementError('*', WINDOWS_REJECTED) - expect(problem).toContain('"*"') - expect(problem).toContain('cannot write in a file name either') - }) - - it('refuses a separator for the reason that makes it worse than unwritable', () => { - for (const replacement of ['../', 'a\\b']) { - const problem = replacementError(replacement, WINDOWS_REJECTED) - expect(problem).toContain('move the file into another folder') - } - }) - - it('checks every character of a longer replacement, not only the first', () => { - expect(replacementError('ok?', WINDOWS_REJECTED)).not.toBeNull() - }) - - it('judges only what the host sent, so Unix accepts what Windows refuses', () => { - // The rules are the host's, and this is the shape that proves this file - // holds none of its own: with the Unix set, `*` is a perfectly good - // replacement. A hardcoded list of "bad characters" here would fail. - const unixRejected = '\u0000/\\' - expect(replacementError('*', unixRejected)).toBeNull() - expect(replacementError('\u0000', unixRejected)).toContain('U+0000') - }) -}) - -describe('initialAnswers', () => { - it('prefills every character with the default stand-in', () => { - const answers = initialAnswers([ - { character: '?', fault: 'rejected', entries: 2 }, - { character: ':', fault: 'reinterpreted', entries: 1 }, - ]) - expect(answers).toEqual({ '?': DEFAULT_REPLACEMENT, ':': DEFAULT_REPLACEMENT }) - }) - - it('is writable everywhere, or the default would be refused on sight', () => { - expect(replacementError(DEFAULT_REPLACEMENT, WINDOWS_REJECTED)).toBeNull() - }) -}) - -describe('substitutions', () => { - it('sends what the user typed for each character asked about', () => { - const characters = [ - { character: '?', fault: 'rejected', entries: 1 }, - { character: ':', fault: 'reinterpreted', entries: 1 }, - ] - expect(substitutions(characters, { '?': '-', ':': '' })).toEqual({ '?': '-', ':': '' }) - }) - - it('leaves out an answer for a character this archive never asked about', () => { - // Driven by the archive's questions, so an answer left over from a - // previous one cannot ride along into this extraction. - const characters = [{ character: '?', fault: 'rejected', entries: 1 }] - expect(substitutions(characters, { '?': '-', '*': 'stale' })).toEqual({ '?': '-' }) - }) - - it('sends nothing when there is nothing to answer', () => { - expect(substitutions([], { '?': '-' })).toEqual({}) - }) -}) diff --git a/docs/architecture.md b/docs/architecture.md index d15f712..cf481c3 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -105,12 +105,17 @@ can say what will not work before anyone waits for it. The CLI names every offending entry at once from that report; core stops at the first, the run being over either way. -`Substitutions` and `ExtractOptions::with_replacements` survive as inert -surface, so the two front ends still compile while their naming dialogs are -taken out. `NamePlan` and the `extract_*_planned` backend variants are likewise -vestigial: nothing constructs a plan that says anything. Removing all of it, -tar's second write path included, is follow-up work kept out of the commit that -changed the policy, because that path carries the containment guard. +The machinery of the old answer is gone: `Substitutions`, +`ExtractOptions::with_replacements`, `NameRules::rewrite`, `NamePlan` and the +`extract_*_planned` backend variants. `NameError` is one variant, `Unwritable`, +because the other six described a negotiation that no longer happens. + +Tar went from two write paths to one with it. `unpack_in` derives the +destination from the entry's own name and so could not write a renamed entry, +which meant the renamed branch had to repeat by hand the canonicalizing +containment check `unpack_in` does — the one that stops a write following a +symlink already in the output. Nothing is renamed, so that branch and its +duplicate check are both gone. ### `src/paths.rs` — the guards both front ends share @@ -473,19 +478,20 @@ HTTP, same engine as the CLI. It compresses files and folders and extracts archives, in the cervantic visual style (warm cream + terracotta, monospace), and targets macOS, Windows and Linux from one codebase. -The backend exposes five Tauri commands: `is_directory` (UI icon/name hint), +The backend exposes four Tauri commands: `is_directory` (UI icon/name hint), `compress_path` (dispatches file vs. folder, refuses to overwrite its own source or a file inside the folder being compressed, replaces an existing output only when `overwrite` says the user agreed to it in the save dialog, and hands the work to a remote server when one is chosen), -`extract_archive`, `check_server` (a health probe for the settings panel), and -`unwritable_names`, which reports the entry names this machine cannot write. +`extract_archive` (which takes where the archive is and where it goes, and +nothing else: a name this host cannot write fails the extraction rather than +becoming a question) and `check_server` (a health probe for the settings panel). -That last one, and the dialog it feeds, are **inert**: extraction refuses such -an archive outright rather than taking answers, so the replacements the dialog -collects reach nothing. Both go when the dialog does. +There used to be a fifth, `unwritable_names`, feeding a dialog that asked the +user for a character to put in place of one the host refuses. It went when the +answers stopped reaching anything. -All four of the blocking ones carry `#[tauri::command(async)]`. A bare +All three of the blocking ones carry `#[tauri::command(async)]`. A bare `#[tauri::command]` on a synchronous function runs the body on the thread handling the IPC message, so the window stops repainting until it returns: a compression froze it for its diff --git a/docs/desktop.md b/docs/desktop.md index 4a04ff0..bde473b 100644 --- a/docs/desktop.md +++ b/docs/desktop.md @@ -25,14 +25,9 @@ apps/desktop/ src-tauri/ Rust backend src/lib.rs app wiring: the plugin, the command registry, run() src/commands.rs the commands the webview invokes: is_directory, - compress_path, extract_archive, check_server, - unwritable_names + compress_path, extract_archive, check_server src/paths.rs same_file and inside, the guards that stop an archive overwriting its own source or a file within it - src/names.rs the entry-name exchange the extract dialog was built on. - Inert: extraction refuses a name this host cannot write - rather than taking a replacement for it, so the answers - the dialog collects reach nothing. Goes when it does. tests/ Cargo integration tests (see below) tauri.conf.json window, bundle, identifier (com.cervantic.collapse) capabilities/ window permissions (core + dialog)