diff --git a/README.md b/README.md index 5638742..999fccf 100644 --- a/README.md +++ b/README.md @@ -60,8 +60,18 @@ Formats: `zip` (default, or inferred from `-o`'s extension), `7z`, `tar`. Level next to the source; override with `-o`. It **won't overwrite** an existing archive unless you pass `--force`, and `--force` still refuses to write onto its own source or onto a file inside the folder being compressed, because either -would destroy the data instead of archiving it. Run `collapse --help` for the -full surface. +would destroy the data instead of archiving it. + +**Every local compression is read back before it is reported.** A compressor +here finalises on drop, so a run that dies partway through still closes out an +archive that opens cleanly and is quietly missing whatever had not been written +yet — nothing about the file itself says so. By default the check is the +archive's own listing, which is what catches that. `--verify` upgrades it to +reading every entry back, roughly twice the work, which also checks the +per-entry checksums zip and 7z store (tar stores none). It cannot be combined +with `--server`, since the archive is built on the far side. + +Run `collapse --help` for the full surface. Prebuilt macOS binaries (Apple Silicon and Intel tarballs, with sha256 checksums) are attached to every @@ -188,8 +198,8 @@ Requires **Rust 1.88+** (2021 edition). ```bash make build # build the Rust crates -make test # run every suite (620 Rust tests + 116 Vitest cases) -make test/rust # only the Rust tests that need no Node toolchain (505) +make test # run every suite (621 Rust tests + 116 Vitest cases) +make test/rust # only the Rust tests that need no Node toolchain (506) ``` `make test` includes the desktop app's own Rust suite, which compiles Tauri, so diff --git a/apps/cli/src/lib.rs b/apps/cli/src/lib.rs index 4df923d..945b564 100644 --- a/apps/cli/src/lib.rs +++ b/apps/cli/src/lib.rs @@ -437,17 +437,18 @@ fn run_extract(archive: PathBuf, output_dir: PathBuf) -> Result CliError { @@ -143,50 +143,50 @@ fn the_refusal_counts_the_entries_each_character_holds_up() { ); } -/// A trailing dot and a device name have one correct answer and nobody to ask, -/// so they are not what the refusal is about. Refusing them too would leave a -/// Windows user unable to extract an archive whose only fault is a file called -/// `aux.log`, which the desktop app would open without a word. +/// A trailing dot and a device name used to be adjusted rather than refused, on +/// the reasoning that they had one correct answer and nobody to ask. Core +/// adjusts nothing now, so `run_extract` refuses on the whole report instead of +/// on its `characters` half, and these three entries are exactly what that +/// widening is about: an archive whose only fault is a file called `aux.log` is +/// refused on Windows rather than arriving as `aux_.log`. +/// +/// Judged through `NameReport` rather than by running the command, because the +/// faults are Windows-only and this suite runs on Linux. #[test] -fn a_name_that_needs_no_answer_is_adjusted_rather_than_refused() { +fn a_name_that_needs_no_answer_is_refused_like_any_other() { let windows = NameRules::windows(); let report = NameReport::of(&["notes.txt.", "CON.txt", "aux.log", "fine.txt"], windows); + // The half the CLI used to consult is empty: not one of these is a question + // anybody could be asked. Under the old rule the archive extracted. + assert!(report.characters.is_empty(), "none of these is a question"); + // The half it consults now is not, which is the whole of the change. + assert!(!report.is_empty(), "the archive is refused all the same"); + + let named: Vec<&str> = report.entries.iter().map(|e| e.entry.as_str()).collect(); assert_eq!( - adjustments(&report, windows), - vec![ - Adjustment { - entry: "notes.txt.".to_string(), - written: "notes.txt".to_string(), - }, - Adjustment { - entry: "CON.txt".to_string(), - written: "CON_.txt".to_string(), - }, - Adjustment { - entry: "aux.log".to_string(), - written: "aux_.log".to_string(), - }, - ], - "the device keeps the extension that says what the file is, and the writable name is absent" + named, + ["notes.txt.", "CON.txt", "aux.log"], + "every offending entry, and the writable one absent" ); } -/// An entry needing an answer has no adjustment to report, because there is no -/// answer to apply. That is what keeps the two lists disjoint: what `run` -/// refuses is exactly what this cannot rewrite. +/// The two kinds of fault now arrive at the same place, which is what stops the +/// refusal being half a list. A report mixing one of each names both, so a +/// user is not refused for `what?.txt`, fixed nothing, and then refused again +/// for `notes.txt.`. #[test] -fn an_entry_needing_a_replacement_has_no_adjustment() { +fn a_report_mixing_both_kinds_of_fault_names_all_of_them() { let windows = NameRules::windows(); let report = NameReport::of(&["what?.txt", "notes.txt."], windows); - assert_eq!( - adjustments(&report, windows), - vec![Adjustment { - entry: "notes.txt.".to_string(), - written: "notes.txt".to_string(), - }], - ); + assert!(!report.is_empty()); + let named: Vec<&str> = report.entries.iter().map(|e| e.entry.as_str()).collect(); + assert_eq!(named, ["what?.txt", "notes.txt."]); + + let message = windows_refusal("mixed.zip", &["what?.txt", "notes.txt."]); + assert!(message.contains("what?.txt"), "{message}"); + assert!(message.contains("notes.txt."), "{message}"); } // ------------------------------------------------------ end to end, on any host -- diff --git a/apps/core/src/compression.rs b/apps/core/src/compression.rs index 4137783..9ef50fc 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::{plan_names, NamePlan}; +pub(crate) use self::names::{refuse_unwritable_names, NamePlan}; pub(crate) use self::sevenz::{list_7z_entries, read_7z_entries}; pub(crate) use self::tar::{list_tar_entries, read_tar_entries}; pub(crate) use self::walk::walk_tree; @@ -371,9 +371,9 @@ pub fn compress_dir( /// /// A separate type rather than more arguments on [`extract`], and /// [`extract_with`] rather than a replacement for it: every existing caller -/// (the CLI, the server, the desktop, this crate's own tests) has no -/// substitutions to offer and should not have to say so, and the next knob -/// extraction grows should not add a third function. +/// (the CLI, the server, the desktop, this crate's own tests) has nothing to +/// say here and should not have to say so, and the next knob extraction grows +/// should not add a third function. #[derive(Debug, Clone, Default)] #[non_exhaustive] pub struct ExtractOptions { @@ -394,7 +394,13 @@ impl ExtractOptions { self } - /// The caller's answers for the characters the host cannot write. + /// **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 @@ -404,6 +410,8 @@ impl ExtractOptions { self.rules } + /// The answers this was handed. Read by nobody; see + /// [`Self::with_replacements`]. pub fn replacements(&self) -> &Substitutions { &self.replacements } @@ -471,11 +479,14 @@ fn list_entries(archive: &Path, algorithm: Algorithm) -> Result, Com /// What an archive holds that this machine cannot write as ordinary files. /// /// Reads the listing and nothing else: no entry is decompressed and nothing is -/// created, so a front end can ask this before it asks the user anything. Feed -/// the answers back through [`ExtractOptions::with_replacements`]. +/// created, so a front end can ask this before it makes anyone wait. /// -/// An empty report ([`NameReport::is_empty`]) means extraction has no naming -/// question to ask, which on Unix is nearly always the case. +/// **This is now a prediction, not a questionnaire.** A non-empty report +/// ([`NameReport::is_empty`]) means [`extract`] will refuse this archive on +/// this machine, and the report says which entries and why so a front end can +/// explain it. There is nothing to answer: the entries it names are the reason +/// the extraction will not happen, not a form to fill in. On Unix the report is +/// nearly always empty. pub fn unwritable_names(archive: &Path) -> Result { unwritable_names_with(archive, NameRules::host()) } @@ -493,44 +504,55 @@ pub fn unwritable_names_with( /// Extract an archive into `output_dir`. /// -/// Returns the list of extracted file paths (relative to `output_dir`), which -/// are the names **as written**: an entry the host had to be given a different -/// name for is reported under the name that is on disk, never under the -/// archive's, or a front end would list files nobody can find. +/// Returns the list of extracted file paths, relative to `output_dir` and +/// spelled exactly as the archive spells them. +/// +/// **All of the archive, under its own names, or none of it.** An entry this +/// filesystem cannot hold as spelled fails the whole extraction +/// ([`CompressionError::Name`]) before anything is written, rather than being +/// adjusted to fit: a renamed file is not the file the archive named, and +/// nothing downstream can tell the two apart afterwards. Ask +/// [`unwritable_names`] first if the answer is worth showing someone before +/// they wait for it. /// /// The algorithm is detected from the archive file extension. pub fn extract(archive: &Path, output_dir: &Path) -> Result, CompressionError> { extract_with(archive, output_dir, &ExtractOptions::default()) } -/// [`extract`], with the caller's answers for the entry names this machine -/// cannot write. +/// [`extract`], against a chosen set of [`NameRules`] rather than the host's. /// -/// Naming is settled over the whole listing before the first byte is written, -/// and a listing that cannot be read stops it there (issue #89), so the answers -/// nothing can recover from (a character with no replacement, two entries that -/// would land on one name, an archive too damaged to read) leave the output -/// directory as they found it. +/// Same policy as [`extract`], which is the point of it: this exists so a test +/// on one machine can ask what another machine would do, not so a caller can +/// soften the answer. +/// +/// Every name is judged against the whole listing before the first byte is +/// written, and a listing that cannot be read stops it there (issue #89), so +/// an archive this system cannot hold leaves the output directory exactly as +/// it found it. pub fn extract_with( archive: &Path, output_dir: &Path, options: &ExtractOptions, ) -> Result, CompressionError> { let algorithm = algorithm_of(archive)?; - // Before the archive is even opened: an answer that is itself unwritable is - // wrong whether or not any entry needs it. - options.rules().check_replacements(options.replacements())?; - let (names, plan) = plan_for(archive, algorithm, options)?; - refuse_overwriting_the_archive(archive, output_dir, &names, &plan)?; - + let names = listing_for(archive, algorithm)?; + // Judged against the whole listing before a byte is written, so an archive + // this host cannot hold leaves the output directory as it found it. + refuse_unwritable_names(&names, options.rules())?; + refuse_overwriting_the_archive(archive, output_dir, &names)?; + + // The plain backends, because there is no longer any plan to hand them: + // every entry is written under the name the archive spells or the run + // stopped above. match algorithm { - Algorithm::SevenZ => self::sevenz::extract_7z_planned(archive, output_dir, &plan), - Algorithm::Tar => self::tar::extract_tar_planned(archive, output_dir, &plan), - Algorithm::Zip => self::zip::extract_zip_planned(archive, output_dir, &plan), + Algorithm::SevenZ => self::sevenz::extract_7z(archive, output_dir), + Algorithm::Tar => self::tar::extract_tar(archive, output_dir), + Algorithm::Zip => self::zip::extract_zip(archive, output_dir), } } -/// Work out what every entry will be called, from the listing. +/// The archive's listing, read before anything is written. /// /// **A listing that cannot be read stops the extraction here**, before anything /// is written. @@ -540,8 +562,8 @@ pub fn extract_with( /// layer would only replace that message with a worse one. The reasoning was /// right about the message and wrong about the timing (issue #89): the /// extractor fails **while streaming**, so by the time it notices it has -/// already written every entry before the fault, and written them with no plan -/// at all, which means no rewriting, no refusal and no collision check. An +/// already written every entry before the fault, and written them with nothing +/// judged at all, so no name was refused however badly it fitted the host. An /// archive holding `notes.txt:hidden` was refused outright when its listing was /// intact and written as an invisible NTFS stream when it was not: the harm of /// issue #63, performed without the user ever being asked. One bad 512 byte @@ -558,27 +580,26 @@ pub fn extract_with( /// and so never come through here; `recovering_from_a_damaged_archive_is_still_ /// possible_through_the_backend` pins that. /// -/// It costs one listing per extraction, paid even when nothing needs renaming, +/// It costs one listing per extraction, paid even when every name is ordinary, /// because the only way to know that is to read the names. For zip and 7z that /// is a header read; for tar it is a second walk over the headers, seeking past /// each member rather than reading it. -fn plan_for( - archive: &Path, - algorithm: Algorithm, - options: &ExtractOptions, -) -> Result<(Vec, NamePlan), CompressionError> { - let names = list_entries(archive, algorithm).map_err(unreadable_archive)?; - let plan = plan_names(&names, options.rules(), options.replacements())?; - Ok((names, plan)) +fn listing_for(archive: &Path, algorithm: Algorithm) -> Result, CompressionError> { + list_entries(archive, algorithm).map_err(unreadable_archive) } /// Refuse an extraction that would write one of the archive's own entries over /// the archive. /// /// Free to run, in the sense that matters: the listing it needs has already -/// been read and paid for by the planning pass, so this adds one identity check +/// been read and paid for by [`listing_for`], so this adds one identity check /// per entry and no extra pass over the file. /// +/// It compares the name the archive spells, because that is now the only name +/// an entry can be written under. It used to have to follow a rewritten name +/// as well, since a substitution could land an entry on the archive that the +/// archive's own name did not match; nothing rewrites a name any more. +/// /// By **file identity**, not by path. A hardlink is a second name for one file /// and never resolves to the same string, which is exactly how `--force` used /// to be able to overwrite its own source on the compression side before that @@ -587,7 +608,6 @@ fn refuse_overwriting_the_archive( archive: &Path, output_dir: &Path, names: &[String], - plan: &NamePlan, ) -> Result<(), CompressionError> { for name in names { let Some(natural) = sanitize_entry_path(name) else { @@ -595,8 +615,7 @@ fn refuse_overwriting_the_archive( // that is the message worth keeping. continue; }; - let rel = plan.written_as(name).map_or(natural, Path::to_path_buf); - if crate::paths::same_file(&output_dir.join(&rel), archive) { + if crate::paths::same_file(&output_dir.join(&natural), archive) { return Err(CompressionError::WouldOverwriteArchive { archive: archive.to_path_buf(), entry: name.clone(), diff --git a/apps/core/src/compression/names.rs b/apps/core/src/compression/names.rs index 551e225..80dbdc0 100644 --- a/apps/core/src/compression/names.rs +++ b/apps/core/src/compression/names.rs @@ -1,5 +1,4 @@ -//! Which entry names the host can write, and what to do with the ones it -//! cannot. +//! Which entry names the host can write, and which ones stop an extraction. //! //! Extraction takes names from an archive, which is to say from another //! machine. Containment ([`sanitize_entry_path`](super::sanitize_entry_path)) @@ -10,6 +9,18 @@ //! directory, and `notes.txt:hidden` is accepted as the `hidden` stream of //! `notes.txt` rather than as a file, with no error at all. //! +//! **The answer to all four is the same: refuse the archive.** This module used +//! to negotiate instead. It asked the user to supply a character in place of +//! the `?`, dropped the trailing dot on its own and suffixed the device name, +//! then extracted under the names it had arrived at. The output was then a tree +//! whose names were this crate's invention rather than the archive's, with no +//! way for anything downstream to tell which files those were. An extraction +//! that cannot reproduce what the archive says is one that should not happen, +//! so [`refuse_unwritable_names`] stops it before the first byte and says which +//! entry and why. The machinery for the old answer is still here and unused, +//! because the front ends still hand it answers; see +//! [`super::ExtractOptions::with_replacements`]. +//! //! The rules are **data** ([`NameRules`]) rather than `#[cfg]`, and that is the //! point: [`NameRules::windows`] can be asked for from any machine, so every //! rule here is exercised on a Mac and on Linux CI as well as on the Windows @@ -633,10 +644,59 @@ pub enum NameError { result: String, }, + /// The host cannot hold this name exactly as the archive spells it. + /// + /// The whole extraction stops here. Extraction writes what the archive + /// says or it writes nothing: a name this filesystem would refuse, or + /// silently store under a different name, is reported rather than adjusted, + /// because the adjusted file is not the file the archive named and nothing + /// downstream can tell the difference afterwards. + #[error( + "the archive entry {entry:?} cannot be written on this system: {component:?} {}. \ + Nothing was extracted; extract it on a system that can hold the name.", + describe(.problems) + )] + Unwritable { + entry: String, + 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. +/// +/// Rendered here rather than on [`NameProblem`] itself: this is the sentence a +/// refusal reads in, and the same data has to render differently in the desktop +/// dialog, which shows it as structure rather than as prose. +fn describe(problems: &[NameProblem]) -> String { + problems + .iter() + .map(|problem| match problem { + NameProblem::Character { + character, + fault: CharacterFault::Rejected, + } => format!("contains {character:?}, which this system refuses in a file name"), + NameProblem::Character { + character, + fault: CharacterFault::Reinterpreted, + } => format!( + "contains {character:?}, which this system reads as something other than part \ + of the name" + ), + NameProblem::TrailingCharacters { removed } => { + format!("ends in {removed:?}, which this system does not preserve") + } + NameProblem::ReservedDevice { device } => { + format!("is the reserved device name {device:?}") + } + }) + .collect::>() + .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. @@ -661,10 +721,14 @@ impl NameError { /// The name every entry will be written under, for the entries whose name /// changes. /// -/// Built from the whole listing before anything is written, because two of the -/// three answers it can give ("no replacement for this" and "these two entries -/// collide") must stop the extraction while the output directory is still -/// empty, and a collision cannot be seen one entry at a time. +/// **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 @@ -685,55 +749,55 @@ impl NamePlan { } } -/// Work out what each name in `names` becomes, and refuse the two situations -/// nothing downstream could recover from. -pub(crate) fn plan_names>( +/// Refuse a listing holding a name this filesystem cannot write exactly as the +/// archive spells it. +/// +/// **Nothing is renamed.** This used to be a planning pass: it applied the +/// caller's replacements, dropped a trailing dot or space and suffixed a +/// reserved device, so an archive Windows could not hold was extracted anyway +/// under names it could. That traded one problem for a worse one. The file on +/// disk was then not the file the archive named, nothing downstream could tell +/// the two apart, and the listing handed back was the only record that a +/// substitution had happened at all. An extraction either reproduces what the +/// archive says or it does not happen. +/// +/// So the question is now the narrow one [`NameRules::can_write`] answers, and +/// the answer is yes or no rather than a rewrite: can this host hold this +/// component, spelled this way. A component it would refuse (`what?.txt` on +/// Windows), silently store elsewhere (`notes.txt:hidden`, an NTFS stream), +/// silently rename (`notes.txt.`, whose trailing dot is dropped) or resolve to +/// a device (`CON`) all fail the same way, because from the caller's side they +/// are the same failure: the name they asked for is not the name they would +/// get. +/// +/// The whole listing is judged before a byte is written, so a refusal leaves +/// the output directory exactly as it found it. +/// +/// The first offending component stops it. Reporting every one at once is a +/// front end's job, and both have the listing to do it from +/// ([`NameReport::of`]), which is also what lets them ask before extracting +/// rather than after failing. +pub(crate) fn refuse_unwritable_names>( names: &[S], rules: NameRules, - replacements: &Substitutions, -) -> Result { - let mut rewritten = HashMap::new(); - // planned name -> the first entry that claimed it, and whether that entry's - // name had to change to claim it. - let mut claimed: HashMap = HashMap::new(); - +) -> Result<(), NameError> { for name in names { let name = name.as_ref(); - let planned = rules.rewrite_entry(name, replacements)?; - if planned.as_os_str().is_empty() { - // Nothing normal in it (`.`, a bare root). Containment decides what - // becomes of those, per format, and it is not this pass's business. - continue; - } - let changed = planned != natural_path(name); - - if let Some((first, first_changed)) = claimed.get(&planned) { - // Two entries spelled the same way is an archive that was already - // like that, and extraction has always let the second win. Refusing - // it here would start rejecting archives that have nothing to do - // with this feature; a collision is only ours when a rewrite caused - // it. - if *first != name && (changed || *first_changed) { - return Err(NameError::Collision { - first: (*first).to_string(), - second: name.to_string(), - name: planned.to_string_lossy().into_owned(), + // Component by component, and split on `/` rather than by the host's + // rules: an archive entry name means the same thing on every machine, + // which is the property `entry_components` exists to preserve. + for component in entry_components(name) { + let problems = rules.problems(component); + if !problems.is_empty() { + return Err(NameError::Unwritable { + entry: name.to_string(), + component: component.to_string(), + problems, }); } } - claimed.insert(planned.clone(), (name, changed)); - if changed { - rewritten.insert(name.to_string(), planned); - } } - - Ok(NamePlan { rewritten }) -} - -/// The relative path an extractor derives from an entry name with no rules -/// applied: its `Normal` components and nothing else. -fn natural_path(name: &str) -> PathBuf { - entry_components(name).collect() + Ok(()) } /// Split an archive entry name into its components, the same way on every host. diff --git a/apps/core/tests/names.rs b/apps/core/tests/names.rs index 73619f4..304adbe 100644 --- a/apps/core/tests/names.rs +++ b/apps/core/tests/names.rs @@ -124,6 +124,19 @@ fn as_windows(replacements: Substitutions) -> ExtractOptions { .with_replacements(replacements) } +/// The message of the refusal an unwritable entry must produce. +/// +/// Panics naming what happened instead, because the two ways this goes wrong +/// need telling apart: extracting anything at all is the policy being broken, +/// while a different error is usually the fixture failing to build the name. +fn refusal(result: Result, CompressionError>, context: &str) -> String { + match result { + Err(CompressionError::Name(problem @ NameError::Unwritable { .. })) => problem.to_string(), + Err(other) => panic!("{context}: expected a naming refusal, got {other}"), + Ok(files) => panic!("{context}: expected a refusal, extracted {files:?}"), + } +} + // ------------------------------------------------------------- the ruleset -- #[test] @@ -564,55 +577,62 @@ fn inspecting_this_machine_s_own_archives_asks_nothing() { // -------------------------------------------- extracting with the answers --- #[test] -fn the_answers_are_written_and_the_listing_names_what_is_on_disk() { - // Issue #64 end to end, for all three formats. The listing is the half that - // matters most: returning the archive's names would have a front end show - // `what?.txt` next to a file called `what_.txt`. +fn every_fault_stops_the_whole_archive_the_same_way() { + // Issue #64 end to end, for all three formats, under the policy that + // replaced the answers. The four faults used to have four different + // endings: a question put to the user, a trailing run truncated in + // silence, a `_` appended to a device name. They have one ending now, and + // that is the whole of what this pins. + // + // `summary.txt` rides along in every archive and is the assertion that + // matters most. It is perfectly writable on any host, and it must still not + // be on disk: the refusal is judged over the listing before a byte is + // written, so a good entry beside a bad one goes nowhere either. for format in FORMATS { - let dir = tempfile::TempDir::new().unwrap(); - let archive = archive_with( - dir.path(), - format, - &[ - ("summary.txt", b"fine"), - ("what?.txt", b"question"), - ("notes.txt.", b"trailing"), - ("CON.txt", b"device"), - ], - ); - let out = dir.path().join("out"); - - let written = extract_with( - &archive, - &out, - &as_windows(Substitutions::new().with('?', "_")), - ) - .unwrap(); - - let expected = vec![ - "CON_.txt".to_string(), - "notes.txt".to_string(), - "summary.txt".to_string(), - "what_.txt".to_string(), - ]; - assert_eq!(sorted(written), expected, "{format}: the returned listing"); - assert_eq!(files_under(&out), expected, "{format}: what is on disk"); - assert_eq!( - fs::read(out.join("what_.txt")).unwrap(), - b"question", - "{format}: the renamed entry kept its content" - ); + for (bad, fault) in [ + ("what?.txt", "a character the host refuses"), + ("notes.txt:hidden", "a character the host reinterprets"), + ("notes.txt.", "a trailing dot"), + ("CON.txt", "a reserved device"), + ] { + let dir = tempfile::TempDir::new().unwrap(); + let archive = archive_with( + dir.path(), + format, + &[("summary.txt", b"fine"), (bad, b"trouble")], + ); + let out = dir.path().join("out"); + let context = format!("{format}, {fault}"); + + let message = refusal( + extract_with(&archive, &out, &as_windows(Substitutions::new())), + &context, + ); + + assert!(message.contains(bad), "{context}: {message}"); + assert!( + files_under(&out).is_empty(), + "{context}: {:?} was written before the refusal", + files_under(&out) + ); + } } } #[test] -fn a_colon_entry_becomes_a_file_of_its_own_and_leaves_its_neighbour_alone() { +fn a_colon_entry_is_refused_and_its_neighbour_is_never_written() { // Issue #63. On Windows the unfixed path writes these bytes into the // `hidden` stream of `notes.txt`, which changes nothing about `notes.txt` // that `dir` can see and leaves the listing naming a file that exists - // nowhere. Here the answer turns it into a file, and the assertion that - // `notes.txt` still holds its own bytes is what would fail if a future - // "simplification" let the colon through. + // nowhere. + // + // The answer used to be a substitution that turned it into a file of its + // own. That is gone: `notes.txt-hidden` is not the file the archive named + // either, and inventing it hid the problem rather than reporting it. + // + // `notes.txt` is what would catch a future "simplification" letting the + // colon through. It is the innocent half of the archive, and it must not + // exist: on a host that refuses the other entry, nothing is written. for format in FORMATS { let dir = tempfile::TempDir::new().unwrap(); let archive = archive_with( @@ -625,59 +645,34 @@ fn a_colon_entry_becomes_a_file_of_its_own_and_leaves_its_neighbour_alone() { ); let out = dir.path().join("out"); - let written = extract_with( - &archive, - &out, - &as_windows(Substitutions::new().with(':', "-")), - ) - .unwrap(); - - assert_eq!( - sorted(written), - vec!["notes.txt".to_string(), "notes.txt-hidden".to_string()], - "{format}" - ); - assert_eq!(fs::read(out.join("notes.txt")).unwrap(), b"the real file"); - assert_eq!( - fs::read(out.join("notes.txt-hidden")).unwrap(), - b"the payload" - ); - } -} - -#[test] -fn an_entry_with_no_answer_stops_before_anything_is_written() { - // The pre-pass earning its keep: judged one entry at a time, `summary.txt` - // would already be on disk when `what?.txt` was refused, and the user would - // be left with half a directory and no list of what is in it (issue #64's - // other complaint). - for format in FORMATS { - let dir = tempfile::TempDir::new().unwrap(); - let archive = archive_with( - dir.path(), + let message = refusal( + extract_with(&archive, &out, &as_windows(Substitutions::new())), format, - &[("summary.txt", b"fine"), ("what?.txt", b"question")], ); - let out = dir.path().join("out"); - let err = extract_with(&archive, &out, &as_windows(Substitutions::new())).unwrap_err(); - - let message = err.to_string(); - assert!(message.contains("what?.txt"), "{format}: {message}"); - assert!(message.contains('?'), "{format}: {message}"); + assert!(message.contains("notes.txt:hidden"), "{format}: {message}"); + assert!(message.contains(':'), "{format}: {message}"); assert!( - files_under(&out).is_empty(), - "{format}: {:?} was written before the refusal", - files_under(&out) + !out.join("notes.txt").exists(), + "{format}: the neighbour was written despite the refusal" ); + assert!(files_under(&out).is_empty(), "{format}"); } } #[test] -fn two_entries_that_would_land_on_one_name_are_refused_by_name() { - // Deliberately not disambiguated: renaming one of them to `a_b (2).txt` is - // how a user ends up with a file they never look at again. Both names are - // in the message so the answer can be changed. +fn two_entries_that_used_to_collide_are_refused_for_their_own_names() { + // These two were the collision case: `?` and `*` both answered with `_` + // made one name out of two, and the refusal named all three spellings so + // the answer could be changed. + // + // A collision can no longer be manufactured, because nothing is renamed, so + // the guard that looked for one has gone with it. The archive is still + // refused, for the plainer reason that this host cannot write either name. + // + // Only the first offending entry is reported, and the assertion that + // `a_b.txt` is absent is the one that matters: a planned name appearing in + // a message would mean something is still working one out. for format in FORMATS { let dir = tempfile::TempDir::new().unwrap(); let archive = archive_with( @@ -688,24 +683,26 @@ fn two_entries_that_would_land_on_one_name_are_refused_by_name() { let out = dir.path().join("out"); let answers = Substitutions::new().with('?', "_").with('*', "_"); - let err = extract_with(&archive, &out, &as_windows(answers)).unwrap_err(); + let message = refusal(extract_with(&archive, &out, &as_windows(answers)), format); - let message = err.to_string(); assert!(message.contains("a?b.txt"), "{format}: {message}"); - assert!(message.contains("a*b.txt"), "{format}: {message}"); - assert!(message.contains("a_b.txt"), "{format}: {message}"); assert!( - files_under(&out).is_empty(), - "{format}: a collision must leave the output alone" + !message.contains("a_b.txt"), + "{format}: nothing is renamed, so no planned name should appear: {message}" ); + assert!(files_under(&out).is_empty(), "{format}"); } } #[test] -fn a_renamed_entry_colliding_with_an_untouched_one_is_refused_too() { - // The case a "compare the rewritten names to each other" check would miss: - // only one of these two changes, and it lands on a name the archive already - // uses. +fn a_trailing_dot_is_refused_rather_than_folded_onto_the_name_beside_it() { + // `notes.txt.` used to lose its dot and land on `notes.txt`, which the + // archive already holds, and the collision guard existed to catch exactly + // that. Now the dot is never dropped, so the two names stay two names and + // the archive is refused for the one the host will not preserve. + // + // The neighbour is ordinary and still goes nowhere, which is the all-or- + // nothing half of the policy. let dir = tempfile::TempDir::new().unwrap(); let archive = archive_with( dir.path(), @@ -714,9 +711,11 @@ fn a_renamed_entry_colliding_with_an_untouched_one_is_refused_too() { ); let out = dir.path().join("out"); - let err = extract_with(&archive, &out, &as_windows(Substitutions::new())).unwrap_err(); + let message = refusal( + extract_with(&archive, &out, &as_windows(Substitutions::new())), + "zip", + ); - let message = err.to_string(); assert!(message.contains("notes.txt."), "{message}"); assert!(files_under(&out).is_empty()); } @@ -745,26 +744,32 @@ fn an_archive_that_already_names_one_entry_twice_still_extracts() { } #[test] -fn an_answer_the_host_cannot_write_is_refused_before_the_archive_is_opened() { - // Checked against a path that does not exist: if the answer were validated - // per entry instead of up front, this would fail with "no such file" - // instead, and a user would only learn their replacement was no good after - // choosing an archive. - let dir = tempfile::TempDir::new().unwrap(); - let missing = dir.path().join("nowhere.zip"); - let err = extract_with( - &missing, - &dir.path().join("out"), - &as_windows(Substitutions::new().with('?', "<")), - ) - .unwrap_err(); - assert!( - matches!( - err, - collapse_core::CompressionError::Name(NameError::UnwritableReplacement { .. }) - ), - "{err}" - ); +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] diff --git a/apps/core/tests/security.rs b/apps/core/tests/security.rs index 28c433c..f84ca8f 100644 --- a/apps/core/tests/security.rs +++ b/apps/core/tests/security.rs @@ -11,7 +11,7 @@ use collapse_core::compression::{ compress_7z_dir, compress_tar_dir, compress_zip_dir, extract_7z, extract_tar, extract_zip, NameRules, Substitutions, }; -use collapse_core::{extract, extract_with, Algorithm, ExtractOptions, Verify}; +use collapse_core::{extract, extract_with, Algorithm, CompressionError, ExtractOptions, Verify}; use sevenz_rust2::{SevenZArchiveEntry, SevenZWriter}; use tar::{Builder, EntryType, Header}; use zip::write::SimpleFileOptions; @@ -628,10 +628,17 @@ 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 gives is put inside a name that containment has - // already cleared, so an unchecked replacement is a traversal by the back - // door: `?` answered with `../..` would write above the output directory - // with nothing left to notice it. + // 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), @@ -737,7 +744,17 @@ fn no_format_writes_through_a_symlink_already_in_the_output() { /// planned rename must not be able to land outside either. #[cfg(unix)] #[test] -fn a_renamed_entry_cannot_be_written_through_such_a_symlink() { +fn an_entry_that_cannot_be_named_is_refused_before_any_symlink_is_followed() { + // This entry used to take a second write path. Windows rules made the `?` a + // question, the answer renamed it, and the renamed branch wrote through a + // route that `unpack_in`'s own containment check never saw — so it needed + // its own proof that a symlink already sitting in the output could not be + // followed. + // + // That branch is unreachable now: the name is refused and nothing is + // written by any path. The test stays because the thing it guards is the + // file outside the output directory, and that assertion does not care which + // of the two reasons kept it safe. for ext in ["zip", "7z", "tar"] { let dir = tempfile::TempDir::new().unwrap(); let outside = dir.path().join("outside"); @@ -753,12 +770,8 @@ fn a_renamed_entry_cannot_be_written_through_such_a_symlink() { _ => malicious_tar(&archive, "link/ev?l.txt"), } - // Windows rules make the `?` a question, so this entry is renamed and - // takes the planned write path rather than the untouched one. - let options = ExtractOptions::new() - .with_rules(NameRules::windows()) - .with_replacements(Substitutions::new().with('?', "i")); - let escaped = outside.join("evil.txt"); + let options = ExtractOptions::new().with_rules(NameRules::windows()); + let escaped = outside.join("ev?l.txt"); assert_contained(extract_with(&archive, &out, &options), &escaped); } } @@ -894,18 +907,29 @@ fn the_same_archive_extracts_normally_somewhere_else() { /// lands exactly on the archive. Checking the archive's own spelling would /// miss it. #[test] -fn the_check_follows_the_renamed_name_not_the_archive_s() { +fn an_entry_that_cannot_be_named_never_reaches_the_archive_it_would_overwrite() { + // This used to be the case for following the *planned* name: `v?.zip` + // answered with `_` became `v_.zip`, which is the archive being read, and + // the overwrite guard had to see that even though the archive's own name + // matched no entry. + // + // Nothing is renamed any more, so that arrangement cannot be built. What + // is left is the ordering, and it is worth pinning: the naming refusal + // comes first, so the archive is never opened for writing at all. The + // guarantee a person cares about is unchanged and is the last assertion — + // the archive is still there, byte for byte. let dir = tempfile::TempDir::new().unwrap(); let archive = dir.path().join("v_.zip"); malicious_zip(&archive, "v?.zip"); let before = std::fs::read(&archive).unwrap(); - let options = ExtractOptions::new() - .with_rules(NameRules::windows()) - .with_replacements(Substitutions::new().with('?', "_")); + let options = ExtractOptions::new().with_rules(NameRules::windows()); let err = extract_with(&archive, dir.path(), &options) - .expect_err("the planned name lands on the archive"); - assert!(err.to_string().contains("over the archive itself"), "{err}"); + .expect_err("a name this host cannot write is refused"); + assert!( + matches!(err, CompressionError::Name(_)), + "refused for the naming reason, before the overwrite check: {err}" + ); assert_eq!(std::fs::read(&archive).unwrap(), before); } diff --git a/apps/desktop/src-tauri/src/names.rs b/apps/desktop/src-tauri/src/names.rs index 59f7ab4..d66d887 100644 --- a/apps/desktop/src-tauri/src/names.rs +++ b/apps/desktop/src-tauri/src/names.rs @@ -47,9 +47,11 @@ pub struct NameInspection { /// /// Sent as data rather than reimplemented in JavaScript. The dialog only /// picks the wording; whether a character is acceptable is answered here by - /// the same ruleset the extraction will use, and the extraction checks - /// again regardless (`extract_with` validates the answers before it opens - /// the archive). + /// 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, } diff --git a/apps/desktop/src-tauri/tests/names.rs b/apps/desktop/src-tauri/tests/names.rs index 4941c3c..8540d25 100644 --- a/apps/desktop/src-tauri/tests/names.rs +++ b/apps/desktop/src-tauri/tests/names.rs @@ -316,21 +316,27 @@ fn an_ordinary_archive_extracts_with_no_answers_at_all() { } #[test] -fn an_answer_containing_a_separator_is_refused_before_anything_is_written() { - // `?` is writable on this host, so nothing in this archive needs answering: - // the answer is refused on its own account, by the ruleset, before the - // archive is opened. That is what keeps a bad answer from being discovered - // half way through an extraction. +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 problem = refusal(extract_answering(&archive, &out, &[("?", "../escaped")]).unwrap()); + let outcome = extract_answering(&archive, &out, &[("?", "../escaped")]).unwrap(); - assert!(problem.contains("path separator"), "{problem}"); + assert_eq!(written(outcome), ["notes.txt"]); + assert_eq!(files_under(&out), ["notes.txt"]); assert!( - !out.exists(), - "the output directory was created before the answer was judged" + !dir.path().join("escaped").exists() && !out.join("escaped").exists(), + "the answer reached a path: {:?}", + files_under(dir.path()) ); } @@ -381,38 +387,40 @@ fn a_name_this_computer_cannot_write_is_reported_with_the_character_to_ask_about #[cfg(unix)] #[test] -fn the_answer_is_written_and_the_listing_names_what_is_on_disk() { - let dir = TempDir::new().unwrap(); - let archive = zip_with(dir.path(), &[(UNWRITABLE_HERE, b"hello")]); - let out = dir.path().join("out"); - - let outcome = extract_answering(&archive, &out, &[("\u{0}", "_")]).unwrap(); - - // The name on disk, never the archive's: reporting `bad\0name.txt` would - // name a file that exists nowhere and send the user looking for it. - assert_eq!(written(outcome), ["bad_name.txt"]); - assert_eq!(files_under(&out), ["bad_name.txt"]); - assert_eq!(fs::read(out.join("bad_name.txt")).unwrap(), b"hello"); -} - -#[cfg(unix)] -#[test] -fn an_empty_answer_removes_the_character() { - let dir = TempDir::new().unwrap(); - let archive = zip_with(dir.path(), &[(UNWRITABLE_HERE, b"hello")]); - let out = dir.path().join("out"); - - let outcome = extract_answering(&archive, &out, &[("\u{0}", "")]).unwrap(); - - assert_eq!(written(outcome), ["badname.txt"]); +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_left_unanswered_stops_before_anything_is_written() { +fn a_name_this_computer_cannot_write_stops_before_anything_is_written() { // The other entry is perfectly writable and still does not get written: - // extraction settles every name from the listing before the first byte, so - // a user who dismissed the dialog is not left with half an archive. + // 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(), @@ -423,40 +431,49 @@ fn a_name_left_unanswered_stops_before_anything_is_written() { let problem = refusal(extract_answering(&archive, &out, &[]).unwrap()); assert!( - problem.contains("no replacement for it was given"), + problem.contains("cannot be written on this system"), "{problem}" ); assert!(problem.contains("bad"), "the entry names itself: {problem}"); assert!( !out.exists(), - "an unanswered name wrote {:?}", + "an unwritable name wrote {:?}", files_under(&out) ); } #[cfg(unix)] #[test] -fn an_answer_this_computer_cannot_write_either_is_refused_with_the_reason() { +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. let dir = TempDir::new().unwrap(); let archive = zip_with(dir.path(), &[(UNWRITABLE_HERE, b"hello")]); let out = dir.path().join("out"); - // Replacing the NUL with a NUL is not an answer, and it is caught before - // the archive is opened rather than by the write failing. - let problem = refusal(extract_answering(&archive, &out, &[("\u{0}", "a\u{0}b")]).unwrap()); + let problem = refusal(extract_answering(&archive, &out, &[]).unwrap()); - assert!(problem.contains("cannot write"), "{problem}"); + assert!(problem.contains("refuses in a file name"), "{problem}"); + assert!( + problem.contains("\\0"), + "the character is shown escaped: {problem}" + ); assert!(!out.exists()); } #[cfg(unix)] #[test] -fn two_entries_that_would_land_on_one_name_are_refused_naming_both() { - // Renaming one of them behind the user's back is how a file disappears - // without anyone noticing, so the answer is refused and both names are - // said out loud. The second entry here is one the host could write - // perfectly well: the collision is created by the answer, not found in the - // archive. +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. let dir = TempDir::new().unwrap(); let archive = zip_with( dir.path(), @@ -466,11 +483,9 @@ fn two_entries_that_would_land_on_one_name_are_refused_naming_both() { let problem = refusal(extract_answering(&archive, &out, &[("\u{0}", "_")]).unwrap()); - assert!(problem.contains("bad_name.txt"), "{problem}"); - assert!(problem.contains("both be written as"), "{problem}"); - assert!(!out.exists(), "a collision wrote {:?}", files_under(&out)); - - // And an answer that keeps them apart goes through. - let outcome = extract_answering(&archive, &out, &[("\u{0}", "-")]).unwrap(); - assert_eq!(written(outcome), ["bad-name.txt", "bad_name.txt"]); + assert!( + problem.contains("cannot be written on this system"), + "{problem}" + ); + assert!(!out.exists(), "wrote {:?}", files_under(&out)); } diff --git a/docs/architecture.md b/docs/architecture.md index 438eb23..363f6b1 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -82,19 +82,35 @@ any platform, so every Windows rule is tested from a Mac, and only `NameRules::host()` is chosen by the compiler. A rule reachable solely under `#[cfg(windows)]` is a rule this repository cannot test. -It answers with structure rather than a string, because a front end has to -render it: each offending character (and whether the host **rejects** it or -**reinterprets** it, which is the difference between a colon failing and a -colon quietly becoming an NTFS stream), a trailing dot or space, and a reserved -device name. Only the first is a question for the user; the other two are -adjustments that need explaining, not answering. - -`unwritable_names` inspects a listing without extracting, and `extract_with` -takes the answers. Replacements are applied **before** the structural -adjustments, or `CO?1` answered with `M` would be left as the device `COM1`. -An answer that is itself unwritable, carries a path separator, or empties a -component is refused, and a collision is refused naming both entries rather -than silently renaming one. +**A name that does not fit stops the extraction.** Nothing is renamed to make it +fit, and nothing is put to the user as a question: an archive arrives under the +names it carries or it does not arrive. `refuse_unwritable_names` judges the +whole listing before the first byte and fails on the first component +`NameRules::can_write` rejects, so the output directory is left as it was found, +the writable entries included. + +The four faults it knows about are one fault as far as a caller is concerned — +a character the host **rejects** (`?`), one it **reinterprets** (`:`, which is +the difference between a write failing and a write quietly becoming an NTFS +stream), a trailing dot or space, and a reserved device name. Each would leave a +file under a name the archive did not ask for, and that is the whole of what is +being prevented. +[threat_model.md](threat_model.md#4b-entry-names-this-host-cannot-write) records +why this replaced a scheme that adjusted and asked instead. + +It still answers with **structure** rather than a string, because the refusal has +to be rendered: `unwritable_names` inspects a listing without extracting +anything and reports every entry this host would refuse and why, so a front end +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. ### `src/paths.rs` — the guards both front ends share @@ -160,7 +176,8 @@ effects — no subprocess needed. ### Command surface ``` -collapse compress [-f 7z|zip|tar] [-l 1-5] [-o ] [--force] [--server ] +collapse compress [-f 7z|zip|tar] [-l 1-5] [-o ] [--force] + [--verify] [--server ] collapse extract [-o ] ``` @@ -202,11 +219,29 @@ Aliases `c` / `e`. The CLI-local `Format` enum (`clap::ValueEnum`) converts to handles files and directories alike; the archive lands at the same output path local mode would use. The safety guards in step 4 run before any network I/O. Extraction has no remote mode. +6. **Read the archive back.** Every local compression is checked, and `--verify` + only chooses how deeply: `Verify::Index` by default, `Verify::Contents` with + the flag. The depth is bound once and then both handed to the engine and + reported, so an `Outcome` cannot name a check that did not happen. The flag + is refused alongside `--server`, because the archive is built on the far side + and the protocol has no way to ask for the deeper check; the server takes its + own `verify=` parameter instead. What each depth buys per format is in + `compression/verify.rs`, and the difference is not cosmetic: tar stores no + checksum over an entry's data at all. Extraction (`run_extract`) resolves the output directory (default the current directory) and calls `collapse_core::extract`, which creates the directory tree as needed. +Ahead of that it reads the listing once through `unwritable_names_with` and +refuses an archive holding any name this host cannot write, which core would +refuse anyway. The duplication is deliberate and is the reason the CLI's message +is worth having: core stops at the first offending component, while this has the +whole listing and names every entry at fault in one go, so a user is not refused +four times over four runs. A listing this build cannot read is deliberately not +this check's business — extraction is about to open the same archive and fail on +it in the extractor's own vocabulary, which is the message worth keeping. + ## collapse-remote — the client for a remote server A small crate holding the client side of the server's job flow: @@ -438,15 +473,21 @@ 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 four Tauri commands: `is_directory` (UI icon/name hint), +The backend exposes five 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`, and `check_server` (a health probe for the settings panel). +`extract_archive`, `check_server` (a health probe for the settings panel), and +`unwritable_names`, which reports the entry names this machine cannot write. + +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. -All three of those carry `#[tauri::command(async)]`. A bare `#[tauri::command]` -on a synchronous function runs the body on the thread handling the IPC message, +All four 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 whole duration, and a mistyped server address froze it for the 30 seconds of ureq's connect timeout. `is_directory` is the exception, being a single `stat`. @@ -521,7 +562,10 @@ for four minutes. It is there because merging two individually well formatted branches can still produce an unformatted tree, and only a check on the merged result sees that. Per app, tests gate the build: `test (core)` (`make core/test`) gates `test (remote)`, `test (cli)`, `test (server-backend)` and -`test (desktop)` (the Tauri IPC is mocked, so that one needs Node only), while +`test (desktop)` (the Tauri IPC is mocked, so that one needs Node only) and +`test (desktop, rust)` (the `src-tauri` suite, which needs the Node toolchain to +build the bundle `generate_context!()` embeds, plus the webkit system libraries, +and is the only Linux job that compiles the Tauri crate at all), while `test (server-frontend)` is independent of the Rust engine and waits only on `fmt`. **Linux runs everything, on every push and every pull request.** macOS and Windows run the whole Rust suite too, as `test (rust, macos)` and diff --git a/docs/desktop.md b/docs/desktop.md index bde473b..4a04ff0 100644 --- a/docs/desktop.md +++ b/docs/desktop.md @@ -25,9 +25,14 @@ 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 + compress_path, extract_archive, check_server, + unwritable_names 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) diff --git a/docs/threat_model.md b/docs/threat_model.md index 709b6b7..d79d56e 100644 --- a/docs/threat_model.md +++ b/docs/threat_model.md @@ -91,7 +91,7 @@ still be exactly one `Normal` component, which closes that at the source. `zip_symlink_entry_is_not_materialized_as_symlink`, `tar_symlink_entry_is_not_materialized`, `no_format_writes_through_a_symlink_already_in_the_output`, -`a_renamed_entry_cannot_be_written_through_such_a_symlink`, +`an_entry_that_cannot_be_named_is_refused_before_any_symlink_is_followed`, `a_colon_in_a_later_component_cannot_clear_the_path_being_built`. ### 3. Hardlink escape (tar) @@ -135,9 +135,12 @@ hardlink is a second name for one file and never resolves to the same string, which is precisely how `--force` was once able to overwrite its own source on the compression side. -The check follows the **planned** name rather than the archive's spelling, since -a rename can land an entry on the archive that the archive's own name does not -match. +The check compares the name the archive spells, because that is the only name an +entry can be written under. It used to have to follow a *planned* name as well: +while extraction still renamed entries to fit the host, a substitution could +land one on the archive that the archive's own name did not match. Nothing +renames an entry any more (see [4b](#4b-entry-names-this-host-cannot-write)), so +the two names are one and the guard got simpler rather than weaker. This is the mirror of a guard compression has always had (`OutputIsSource`, and an output inside the folder being archived, neither of which `--force` unlocks). @@ -146,7 +149,7 @@ positions on the same question (issue #96). **Covered by** `no_format_writes_an_entry_over_the_archive_it_is_reading`, `a_hardlink_to_the_archive_is_not_a_way_around_it`, -`the_check_follows_the_renamed_name_not_the_archive_s`, and +`an_entry_that_cannot_be_named_never_reaches_the_archive_it_would_overwrite`, and `the_same_archive_extracts_normally_somewhere_else`, which is the one that stops the guard from being fixed by refusing too much. @@ -162,12 +165,34 @@ same family: the host silently gives the file a different name than the archive asked for. **Prevention.** Extraction judges every entry name against the host's rules -before anything is written, and refuses rather than guessing. A character the -host cannot hold is a question for the user, who supplies a replacement; the two -adjustments nobody can be asked about (a trailing run, a device name) are -applied and reported. The whole listing is planned before the first byte, so an -unanswerable name or two entries that would collide leave the output directory -as they were found. +before anything is written, and **refuses**. Not adjusts, and not asks: an +archive arrives under the names it carries or it does not arrive. + +That is a narrowing of what shipped first, and the reason is worth recording, +because the original answer looked like the kinder one. A character the host +could not hold was put to the user as a question and replaced with their answer; +a trailing run and a reserved device name were adjusted without asking, since +neither has anything anyone could be asked about. It worked. What it produced +was a directory tree whose names this program had invented: `what?.txt` arrived +as `what_.txt` and `CON.txt` as `CON_.txt`, and nothing afterwards — not the +returned listing, not a later `collapse compress` of the same folder, not the +person reading it a month later — could tell which names came from the archive +and which from us. An extraction that cannot reproduce what the archive says is +one that should not happen, and saying so is cheaper to live with than a rename +nobody remembers agreeing to. + +So the four faults have one ending now. `NameRules::can_write` is the whole +question — can this host hold this component, spelled exactly this way — and the +first component that fails it stops the run with `NameError::Unwritable`, naming +the entry, the component and the reason. The listing is judged in full before +the first byte, so a refusal leaves the output directory exactly as it found it, +the entries that were perfectly writable included. + +What replaces the question is a prediction. `unwritable_names` reads a listing +and reports every entry this host would refuse, decompressing nothing and +creating nothing, so a front end can say what will not work before anybody waits +for it. The CLI surveys the whole listing and names every offending entry at +once; core stops at the first, since by then the run is over either way. The rules are **data**, not `#[cfg]`, so a Mac can be asked what a Windows host would refuse, which is what makes them testable at all: nobody working on this @@ -194,7 +219,7 @@ the user ever seeing the question (issue #89). Recovering what a damaged archive still holds is not gone, it is simply no longer the default: the backends (`extract_tar` and friends) take no options and -never come through the planning pass. +never come through this pass. **Covered by** `apps/core/tests/names.rs`, in particular `an_entry_splits_the_same_way_on_every_host`, @@ -204,6 +229,12 @@ never come through the planning pass. `a_damaged_archive_writes_nothing_rather_than_writing_raw_names` and `the_same_names_are_refused_whether_or_not_the_archive_is_damaged`. +The policy itself is pinned by `every_fault_stops_the_whole_archive_the_same_way`, +which drives all four faults through all three formats and checks that a +writable entry sitting beside a bad one is not written either, and by +`an_answer_no_longer_rescues_an_entry_the_host_cannot_write`, which is the guard +against the substitutions being quietly reintroduced. + ## Compression measures ### 5. Symlinks are never followed out of the tree