From b65d739e7a4accc05dff916576ee62ee422d2000 Mon Sep 17 00:00:00 2001 From: Javier Parada Date: Fri, 28 Aug 2026 20:41:54 +0200 Subject: [PATCH 1/5] core: collapse the three extractors back to one write path each MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `NamePlan` has been the identity since extraction stopped renaming entries, so every `extract_*_planned` was its plain counterpart with an extra argument and a lookup that always answered `None`. The type, the three variants and the wrappers that called them are gone; `extract_zip`, `extract_7z` and `extract_tar` are now the functions themselves. The one that mattered is tar. It carried **two** write paths, because `unpack_in` derives the destination from the entry's own name and so cannot write a renamed entry at all — and `unpack_in` is also the traversal guard tar has always used, the canonicalizing containment check that stops a write from following a symlink already sitting in the output directory. The renamed branch therefore had to repeat that check by hand. With nothing renamed, that branch and its hand-rolled containment check are both gone, and every entry goes through `unpack_in` again. No behaviour changes and no test changes: 506 passing before, 506 passing after. That is the point of doing this separately from anything that alters what the code does. The explicit `..` refusal before `unpack_in` stays. It existed because the renamed branch had no other guard, and it is now belt and braces — but it is also what names the offending entry, which `unpack_in`'s own `Ok(false)` does not. --- apps/core/src/compression.rs | 2 +- apps/core/src/compression/names.rs | 35 +--------- apps/core/src/compression/sevenz.rs | 17 ++--- apps/core/src/compression/tar.rs | 104 +++++++++------------------- apps/core/src/compression/zip.rs | 24 ++----- 5 files changed, 46 insertions(+), 136 deletions(-) diff --git a/apps/core/src/compression.rs b/apps/core/src/compression.rs index 9ef50fc..01a8bca 100644 --- a/apps/core/src/compression.rs +++ b/apps/core/src/compression.rs @@ -16,7 +16,7 @@ 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; diff --git a/apps/core/src/compression/names.rs b/apps/core/src/compression/names.rs index 80dbdc0..07fa6dc 100644 --- a/apps/core/src/compression/names.rs +++ b/apps/core/src/compression/names.rs @@ -37,8 +37,8 @@ //! 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 std::collections::BTreeMap; +use std::path::PathBuf; use serde::Serialize; use thiserror::Error; @@ -718,37 +718,6 @@ impl NameError { } } -/// 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() { From 12df37b02a23af2f4c00b6fbcf557350b6cc60aa Mon Sep 17 00:00:00 2001 From: Javier Parada Date: Fri, 28 Aug 2026 20:45:45 +0200 Subject: [PATCH 2/5] cli: stop promising an answer nobody can give any more MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `adjustments`, `Adjustment` and `Outcome::Extracted { adjusted }` are gone. They reported the names extraction had rewritten to fit the host, and extraction stopped rewriting names in v0.10.0, so the list was provably empty at the only place it was built. **More seriously, the refusal message was lying, and shipped that way.** Two of its sentences described the old scheme: Going ahead needs a replacement for '?' (1 entry), 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. The names above with nothing to replace are adjusted for you once it can go ahead. Neither is true. There is no replacement to give, and the desktop dialog it points at collects answers that reach nothing. A message that sends someone to a feature that no longer works is worse than one that says less, so it now says what is actually the case: nothing was extracted, no option would change that, extract on a system that can hold the names. `explain` had the same problem in two lines — a trailing run "would be dropped" and a device name "would be written under an adjusted name". Both described adjustments that no longer happen. The characters are still listed with their counts, and deliberately: there is no answer to give, but one character across forty entries is an archive that is awkward here, while forty characters is one that does not belong on this machine at all, and that is the user's call. The two tests that pinned the old sentences are rewritten rather than deleted — one of them now asserts the dead pointer is *absent*, so it cannot come back. 506 passing, unchanged. --- apps/cli/src/lib.rs | 126 ++++++++-------------------------------- apps/cli/tests/cli.rs | 14 ++--- apps/cli/tests/names.rs | 32 ++++++---- 3 files changed, 48 insertions(+), 124 deletions(-) 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}" ); } From 4ddbbe4ee8b9065e0fdc891a5930caacdf5bcc25 Mon Sep 17 00:00:00 2001 From: Javier Parada Date: Fri, 28 Aug 2026 20:55:07 +0200 Subject: [PATCH 3/5] desktop: take out the naming dialog and the exchange behind it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The whole conversation is gone: `unwritable_names`, the `names` module that shaped its answer for the webview, the sheet in `App.vue` with its fields, validation and adjustment notes, `src/names.js`, and the `replacements` argument `extract_archive` took. It had stopped being a conversation. Extraction refuses a name it cannot write rather than asking what to call it, so the dialog collected answers that reached nothing and then reported a refusal the user could not act on — the worst shape a question can have. `Extraction` goes with it. A naming refusal had a variant of its own so the sheet could stay open on it while everything else went to the error banner; with no sheet there is one destination for both, so `extract_archive` returns `Result, String>` like every other command. **Four places had to move together**, which is what `tests/ipc.rs` exists to police: the `generate_handler!` list, the command itself, the `invoke` call in `App.vue`, and the stub switch in `tests/App.test.js`. Its `BASELINE` and signature table are updated too, so the five-command surface is now four and nothing can quietly become untyped. Tests: `tests/names.rs` is rewritten rather than deleted, down from eighteen cases to four, because one guarantee in it survives and is worth more now that it is the only one — a refusal must reach the user **with its reason attached**. A NUL prints as nothing, so an entry named without its fault looks perfectly fine, and a user told "extraction failed" would think the archive is broken when it is merely foreign. Ten dialog cases went from `App.test.js`, and the two that remain assert a refusal and an ordinary failure both land in the banner. 101 Rust tests and 43 Vitest cases pass. --- apps/desktop/src-tauri/src/commands.rs | 117 +----- apps/desktop/src-tauri/src/lib.rs | 7 +- apps/desktop/src-tauri/src/names.rs | 96 ----- apps/desktop/src-tauri/tests/commands.rs | 20 +- apps/desktop/src-tauri/tests/ipc.rs | 22 +- apps/desktop/src-tauri/tests/names.rs | 473 +++-------------------- apps/desktop/src-tauri/tests/remote.rs | 13 +- apps/desktop/src/App.vue | 283 +------------- apps/desktop/src/names.js | 117 ------ apps/desktop/tests/App.test.js | 267 ++----------- apps/desktop/tests/names.test.js | 162 -------- 11 files changed, 120 insertions(+), 1457 deletions(-) delete mode 100644 apps/desktop/src-tauri/src/names.rs delete mode 100644 apps/desktop/src/names.js delete mode 100644 apps/desktop/tests/names.test.js 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({}) - }) -}) From fbc77e449c4092a8550d184c183c0acd798de507 Mon Sep 17 00:00:00 2001 From: Javier Parada Date: Fri, 28 Aug 2026 21:00:03 +0200 Subject: [PATCH 4/5] core: delete the substitution machinery now that nothing can reach it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With both front ends off it, the rest goes: `Substitutions` and its `FromIterator`, `ExtractOptions::with_replacements` and `replacements`, `NameRules::rewrite`, `rewrite_entry`, `check_replacements` and `check_replacement`, `DEVICE_SUFFIX`, `NameProblem::replaceable`, `NameRules::offending_characters`, and six of `NameError`'s seven variants with the `in_entry` helper that re-pointed them. `NameError` is one variant now, `Unwritable`, which is the only one anything constructed. The other six described a negotiation that no longer happens: no replacement to be missing, unwritable or separator-bearing, no rewrite to make two entries collide, no adjustment to empty a component, no answer key to be more than one character. `NameReport` stays, and so does its `characters` half. It is no longer a questionnaire — nothing can be answered — but it is what the CLI groups its refusal by, and one character across forty entries is a different problem from forty characters. Tests: eleven cases in `apps/core/tests/names.rs` exercised the removed machinery and go with it. `an_entry_splits_the_same_way_on_every_host` goes too, for a different reason — it used `rewrite_entry` only as a way to observe where a name splits, and that guarantee is already held by `the_report_sees_a_colon_in_the_first_component_too` and `only_windows_refuses_a_backslash_inside_a_component`, both of which survive. `the_report_separates_the_questions_from_the_stated_adjustments` is rewritten around what `characters` still means. In `security.rs`, `a_replacement_cannot_carry_an_entry_out_of_the_output_ directory` is replaced by a comment in its place rather than removed silently. It proved a hostile answer like `../../escape` could not move an entry out of the output directory. That hole is now closed by construction instead of by a check — there is no way to supply a replacement, so the code that would fail the test does not compile — and the guarantee is still one this crate makes, so it is worth someone finding the reasoning where the test used to be. 494 passing across the root workspace, down from 506 by exactly the twelve that tested machinery that no longer exists. --- apps/core/src/compression.rs | 21 +- apps/core/src/compression/names.rs | 306 +---------------------------- apps/core/src/lib.rs | 2 +- apps/core/tests/names.rs | 261 ++---------------------- apps/core/tests/security.rs | 56 ++---- 5 files changed, 32 insertions(+), 614 deletions(-) diff --git a/apps/core/src/compression.rs b/apps/core/src/compression.rs index 01a8bca..485001e 100644 --- a/apps/core/src/compression.rs +++ b/apps/core/src/compression.rs @@ -9,7 +9,7 @@ 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}; @@ -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 07fa6dc..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; -use std::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,27 +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, - } - } -} - /// Refuse a listing holding a name this filesystem cannot write exactly as the /// archive spells it. /// 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) -- From 9c7c1ce52634149d3137eed10e1ec5101e47a205 Mon Sep 17 00:00:00 2001 From: Javier Parada Date: Fri, 28 Aug 2026 21:02:20 +0200 Subject: [PATCH 5/5] docs: the naming machinery is gone, not merely unreachable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `architecture.md` and `desktop.md` described `Substitutions`, `NamePlan`, the `extract_*_planned` variants and the desktop's `unwritable_names` command as inert surface awaiting removal. They have been removed, so the documents now describe what is there. Two facts worth keeping rather than just deleting: `NameError` is down to one variant because the other six described a negotiation that no longer happens, and tar went from two write paths to one — the renamed branch had to repeat by hand the containment check `unpack_in` performs, and both went together. The desktop's command surface is four, not five. --- docs/architecture.md | 32 +++++++++++++++++++------------- docs/desktop.md | 7 +------ 2 files changed, 20 insertions(+), 19 deletions(-) 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)