diff --git a/README.md b/README.md index 5ad1d0d..81b82d1 100644 --- a/README.md +++ b/README.md @@ -188,8 +188,8 @@ Requires **Rust 1.88+** (2021 edition). ```bash make build # build the Rust crates -make test # run every suite (600 Rust tests + 113 Vitest cases) -make test/rust # only the Rust tests that need no Node toolchain (485) +make test # run every suite (604 Rust tests + 113 Vitest cases) +make test/rust # only the Rust tests that need no Node toolchain (489) ``` `make test` includes the desktop app's own Rust suite, which compiles Tauri, so diff --git a/apps/core/src/compression.rs b/apps/core/src/compression.rs index 8d829a1..13da057 100644 --- a/apps/core/src/compression.rs +++ b/apps/core/src/compression.rs @@ -398,6 +398,49 @@ fn algorithm_of(archive: &Path) -> Result { .ok_or_else(|| CompressionError::Failed(format!("Unknown archive extension: .{ext}"))) } +/// Say that the archive could not be read, in words, keeping the parser's +/// detail but not its debris. +/// +/// This mattered less while a failed listing was advisory. Now that it stops +/// the extraction, the message is the whole of what the user gets, and the tar +/// crate embeds the bytes it choked on, so a damaged header arrived as a wall +/// of replacement characters ("numeric field did not have utf-8 text: ???? +/// when getting cksum for ????...", with every ? an unprintable byte). +/// +/// Narrower than issue #66, which is about callers being able to tell causes +/// apart. This is only about what is fit to show a person. +fn unreadable_archive(error: CompressionError) -> CompressionError { + let detail = match &error { + // Unwrap rather than nest, or the message repeats its own prefix: + // "Compression failed: ... : Compression failed: ...". + CompressionError::Failed(message) => legible(message), + CompressionError::Io(io) => legible(&io.to_string()), + other => legible(&other.to_string()), + }; + CompressionError::Failed(format!( + "this archive could not be read, so nothing was extracted: {detail}" + )) +} + +/// Strip what a person cannot read out of a dependency's message, and cap it. +fn legible(detail: &str) -> String { + let cleaned: String = detail + .chars() + .map(|c| { + if c.is_control() || c == char::REPLACEMENT_CHARACTER { + ' ' + } else { + c + } + }) + .collect(); + let collapsed = cleaned.split_whitespace().collect::>().join(" "); + match collapsed.char_indices().nth(140) { + Some((cut, _)) => format!("{}...", &collapsed[..cut]), + None => collapsed, + } +} + /// The entry names an archive holds, without extracting anything. fn list_entries(archive: &Path, algorithm: Algorithm) -> Result, CompressionError> { match algorithm { @@ -446,8 +489,9 @@ pub fn extract(archive: &Path, output_dir: &Path) -> Result, Compres /// cannot write. /// /// Naming is settled over the whole listing before the first byte is written, -/// so the two answers nothing can recover from (a character with no -/// replacement, and two entries that would land on one name) leave the output +/// 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. pub fn extract_with( archive: &Path, @@ -469,10 +513,31 @@ pub fn extract_with( /// Work out what every entry will be called, from the listing. /// -/// A listing that cannot be read is deliberately **not** an error here: the -/// extractor is about to open the same archive and fail on it in its own -/// vocabulary, which is the message this layer would otherwise replace with a -/// worse one. Only a readable listing produces a plan. +/// **A listing that cannot be read stops the extraction here**, before anything +/// is written. +/// +/// It used to be swallowed, on the reasoning that the extractor was about to +/// open the same archive and fail on it in its own vocabulary, and that this +/// 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 +/// 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 +/// header appended to a tar was enough to arrange it. +/// +/// The message turned out to be the weaker half of the argument anyway. Both +/// passes go through the same parser, so in practice they say close to the same +/// thing; the reproduction had them differing only in where they were cut off. +/// +/// What this does cost is partial recovery: a truncated tar used to hand back +/// whatever preceded the damage and now hands back nothing. That was a +/// deliberate call. It is still reachable on purpose through the backends +/// themselves ([`self::tar::extract_tar`] and friends), which take no options +/// 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, /// because the only way to know that is to read the names. For zip and 7z that @@ -483,8 +548,6 @@ fn plan_for( algorithm: Algorithm, options: &ExtractOptions, ) -> Result { - let Ok(names) = list_entries(archive, algorithm) else { - return Ok(NamePlan::identity()); - }; + let names = list_entries(archive, algorithm).map_err(unreadable_archive)?; Ok(plan_names(&names, options.rules(), options.replacements())?) } diff --git a/apps/core/tests/names.rs b/apps/core/tests/names.rs index 7bfe740..48a9c1f 100644 --- a/apps/core/tests/names.rs +++ b/apps/core/tests/names.rs @@ -19,7 +19,9 @@ use collapse_core::compression::{ extract_7z, extract_tar, extract_zip, CharacterFault, NameError, NameProblem, NameReport, NameRules, Substitutions, }; -use collapse_core::{extract, extract_with, unwritable_names_with, ExtractOptions}; +use collapse_core::{ + extract, extract_with, unwritable_names_with, CompressionError, ExtractOptions, +}; use sevenz_rust2::{SevenZArchiveEntry, SevenZWriter}; use tar::{Builder, EntryType, Header}; use zip::write::SimpleFileOptions; @@ -810,10 +812,11 @@ fn the_backends_called_directly_still_write_the_archive_s_own_names() { } #[test] -fn an_archive_that_cannot_be_listed_still_fails_in_the_extractor_s_words() { - // The listing pass is advisory on purpose. A corrupt archive must produce - // the message extraction has always produced, not a second-hand one from a - // pass that only exists to plan names. +fn an_archive_that_cannot_be_listed_still_fails_in_the_parser_s_words() { + // The listing pass is no longer advisory (issue #89): it stops the + // extraction. The message is unaffected, because both passes go through the + // same parser, which is what made the old "keep going so the extractor can + // phrase it better" argument weaker than it looked. let dir = tempfile::TempDir::new().unwrap(); let archive = dir.path().join("truncated.zip"); fs::write(&archive, b"PK\x03\x04 and then nothing").unwrap(); @@ -973,3 +976,160 @@ fn a_unix_name_holding_a_backslash_survives_the_round_trip() { ); } } + +// ------------------------------------------ a listing that cannot be read --- + +/// A tar whose entries are sound and whose end-of-archive marker is not. +/// +/// This shape is the whole of issue #89: `list_tar_entries` walks every header +/// before extraction starts, while extraction writes as it walks, so the +/// listing dies on the damage after the extractor would already have written +/// everything before it. +fn tar_with_a_broken_tail(dir: &Path, entries: &[(&str, &[u8])]) -> PathBuf { + let mut builder = Builder::new(Vec::new()); + for (name, content) in entries { + let mut header = Header::new_gnu(); + header.set_size(content.len() as u64); + header.set_mode(0o644); + header.set_entry_type(EntryType::Regular); + let raw = name.as_bytes(); + header.as_old_mut().name[..raw.len()].copy_from_slice(raw); + header.set_cksum(); + builder.append(&header, *content).unwrap(); + } + let mut bytes = builder.into_inner().unwrap(); + // Replace the two trailing zero blocks with something that is not a header. + let tail = bytes.len() - 1024; + for byte in bytes[tail..].iter_mut() { + *byte = 0xAA; + } + let archive = dir.join("broken-tail.tar"); + fs::write(&archive, &bytes).unwrap(); + archive +} + +/// Issue #89. An unreadable listing used to turn the entire naming layer off +/// and let every entry before the damage be written under its raw name. +/// +/// On Windows `notes.txt:hidden` is not a file name: it is the `hidden` +/// alternate data stream of `notes.txt`, so the write succeeds, the bytes land +/// where no listing shows them, and the user was told there was nothing to +/// answer for. That is issue #63's harm performed without consent, and one bad +/// 512 byte header was enough to arrange it. +#[test] +fn a_damaged_archive_writes_nothing_rather_than_writing_raw_names() { + let dir = tempfile::TempDir::new().unwrap(); + let archive = tar_with_a_broken_tail( + dir.path(), + &[("notes.txt:hidden", b"payload"), ("second.txt", b"more")], + ); + let out = dir.path().join("out"); + + let options = ExtractOptions::new().with_rules(NameRules::windows()); + let err = extract_with(&archive, &out, &options).unwrap_err(); + + // The parser's own words, not a second-hand summary. + assert!(err.to_string().starts_with("Compression failed:"), "{err}"); + // And nothing on disk. Before the fix both entries were here, the first as + // an NTFS stream on a Windows host. + let written: Vec<_> = fs::read_dir(&out) + .map(|entries| { + entries + .filter_map(Result::ok) + .map(|e| e.file_name()) + .collect() + }) + .unwrap_or_default(); + assert!( + written.is_empty(), + "a damaged archive wrote {written:?} before failing" + ); +} + +/// The same archive with an intact tail is refused too, but for the reason the +/// user can act on. The pair is the point: an archive must not become *more* +/// permissive by being damaged. +#[test] +fn the_same_names_are_refused_whether_or_not_the_archive_is_damaged() { + let dir = tempfile::TempDir::new().unwrap(); + let entries: &[(&str, &[u8])] = &[("notes.txt:hidden", b"payload"), ("second.txt", b"more")]; + let options = ExtractOptions::new().with_rules(NameRules::windows()); + + let intact = archive_with(dir.path(), "tar", entries); + let out = dir.path().join("intact"); + let err = extract_with(&intact, &out, &options).unwrap_err(); + assert!( + matches!(err, CompressionError::Name(_)), + "an intact archive must name the character to answer for: {err}" + ); + assert!(!out.exists() || fs::read_dir(&out).unwrap().count() == 0); + + let damaged = tar_with_a_broken_tail(dir.path(), entries); + let out = dir.path().join("damaged"); + assert!(extract_with(&damaged, &out, &options).is_err()); + assert!(!out.exists() || fs::read_dir(&out).unwrap().count() == 0); +} + +/// Recovering what a damaged archive still holds is deliberately not gone, it +/// is just no longer what `extract` does by default. +/// +/// The backends take no options, so they never go through the planning pass. +/// That is the escape hatch, and it is worth knowing it exists: without this +/// test the capability would look like it had been deleted. +#[test] +fn recovering_from_a_damaged_archive_is_still_possible_through_the_backend() { + let dir = tempfile::TempDir::new().unwrap(); + let archive = + tar_with_a_broken_tail(dir.path(), &[("first.txt", b"one"), ("second.txt", b"two")]); + let out = dir.path().join("salvage"); + + // It still fails, on the damage, but only after handing back what preceded + // it. `extract` writes nothing at all for the same archive. + let _ = extract_tar(&archive, &out); + let salvaged: Vec<_> = fs::read_dir(&out) + .map(|entries| { + entries + .filter_map(Result::ok) + .map(|e| e.file_name()) + .collect() + }) + .unwrap_or_default(); + assert_eq!(salvaged.len(), 2, "the backend salvaged {salvaged:?}"); +} + +/// A refusal is now the whole of what the user gets for a damaged archive, so +/// the message has to be fit to read. +/// +/// The tar crate embeds the bytes it choked on, so this one used to arrive as +/// "numeric field did not have utf-8 text: when +/// getting cksum for ". That was survivable while the listing +/// pass was advisory and the files came out anyway; it is not now. +#[test] +fn a_damaged_archive_says_so_in_words_a_person_can_read() { + let dir = tempfile::TempDir::new().unwrap(); + let archive = tar_with_a_broken_tail(dir.path(), &[("a.txt", b"one")]); + + let err = extract(&archive, &dir.path().join("out")).unwrap_err(); + let message = err.to_string(); + + assert!( + message.contains("could not be read, so nothing was extracted"), + "it must lead with what happened: {message}" + ); + assert!( + !message.contains(char::REPLACEMENT_CHARACTER), + "the dependency's debris reached the user: {message}" + ); + assert!( + !message.chars().any(char::is_control), + "control characters reached the user: {message}" + ); + // The parser's own detail is kept, just cleaned up. + assert!(message.contains("cksum"), "the detail was lost: {message}"); + // And it stays short enough to read in a terminal. + assert!( + message.chars().count() < 240, + "{} chars", + message.chars().count() + ); +} diff --git a/apps/desktop/src-tauri/tests/commands.rs b/apps/desktop/src-tauri/tests/commands.rs index 217dcda..a423f78 100644 --- a/apps/desktop/src-tauri/tests/commands.rs +++ b/apps/desktop/src-tauri/tests/commands.rs @@ -1540,11 +1540,21 @@ fn a_truncated_archive_is_reported_legibly_instead_of_panicking() { ); } // The short read reaches core as its dependency's `Io` variant, so - // core unwraps it to `CompressionError::Io` (issue #66). It used to - // read `Compression failed: Io(Error { kind: UnexpectedEof, - // message: "failed to fill whole buffer" }, "")`. + // core unwraps it (issue #66). It used to read `Compression failed: + // Io(Error { kind: UnexpectedEof, message: "failed to fill whole + // buffer" }, "")`, and then plain `IO error: failed to fill whole + // buffer` once that was mapped properly. + // + // It now names the consequence first: a listing that cannot be read + // stops the extraction rather than letting it write raw names + // (issue #89), and that refusal is the whole of what the user gets, + // so it has to say what happened before it says why. "7z" => { - assert_eq!(err, "IO error: failed to fill whole buffer"); + assert_eq!( + err, + "Compression failed: this archive could not be read, so nothing was \ + extracted: failed to fill whole buffer" + ); let leftovers: Vec = fs::read_dir(&out_dir) .map(|entries| entries.map(|e| e.unwrap().path()).collect()) .unwrap_or_default(); diff --git a/bad.zip b/bad.zip new file mode 100644 index 0000000..00208e4 --- /dev/null +++ b/bad.zip @@ -0,0 +1 @@ +PK and then nothing \ No newline at end of file diff --git a/docs/threat_model.md b/docs/threat_model.md index e4d009d..230e645 100644 --- a/docs/threat_model.md +++ b/docs/threat_model.md @@ -149,11 +149,25 @@ tar has used it since v7), so the split is now on `/` on every machine, and a backslash is judged as what it is: an ordinary character, legal on Unix, refused by Windows. +An archive whose **listing cannot be read** stops extraction before anything is +written. It used to be waved through: the pre-flight pass gave up, extraction +carried on with no plan, and every entry before the damage was written under its +raw name, unrewritten and unrefused. One bad 512 byte header appended to a tar +was enough, because tar lists all its headers up front and writes as it walks. +On Windows that turned `notes.txt:hidden` into an invisible NTFS stream without +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. + **Covered by** `apps/core/tests/names.rs`, in particular `an_entry_splits_the_same_way_on_every_host`, `the_report_sees_a_colon_in_the_first_component_too`, `only_windows_refuses_a_backslash_inside_a_component`, and -`a_unix_name_holding_a_backslash_survives_the_round_trip`. +`a_unix_name_holding_a_backslash_survives_the_round_trip`, +`a_damaged_archive_writes_nothing_rather_than_writing_raw_names` and +`the_same_names_are_refused_whether_or_not_the_archive_is_damaged`. ## Compression measures