From fd7a1fd3b2ee42662f6d43e6ecf3532b0077c69e Mon Sep 17 00:00:00 2001 From: Javier Parada Date: Wed, 26 Aug 2026 15:39:31 +0200 Subject: [PATCH] core: stop extracting an archive whose listing cannot be read `plan_for` swallowed a listing failure and carried on with the identity plan. The comment justifying that was right about the message and wrong about the timing: the extractor fails **while streaming**, so by the time it notices it has written every entry before the fault, and written them with no plan at all. No rewriting, no refusal, no collision check. Reproduced, same two entries, twice: intact listing report Ok(1) extract Err ON DISK [] unreadable report Err extract Err ON DISK [notes.txt:hidden, second.txt] On Windows `notes.txt:hidden` is not a file name, it is the `hidden` alternate data stream of `notes.txt`: the write succeeds, the bytes land where no listing shows them, and the user was told there was nothing to answer for. Issue #63's harm, performed without consent, arranged by appending one bad 512 byte header to a tar. The message half of the old argument turned out to be the weak half. Both passes go through the same parser, so they say close to the same thing; in the reproduction they differed only in where they were cut off. What this costs is partial recovery: a truncated tar used to hand back whatever preceded the damage and now hands back nothing. Deliberate, and not actually gone, since the backends take no options and never come through the planning pass. `recovering_from_a_damaged_archive_is_still_possible_through_ the_backend` pins that so the capability does not look deleted. Checked before committing to it that the listing parser is not stricter than the extractor for archives that are merely unusual: a tar with no end-of-archive marker and a zip with junk appended both list fine, so this does not start refusing good archives. Also makes the refusal readable, which is not scope creep but the consequence of the change: it is now the whole of what the user gets for a damaged archive, and the tar crate embeds the bytes it choked on. before: Compression failed: numeric field did not have utf-8 text: <4 bad bytes> when getting cksum for <100 more> after: Compression failed: this archive could not be read, so nothing was extracted: numeric field did not have utf-8 text: when getting cksum for Narrower than issue #66, which is about callers telling causes apart. This is only about what is fit to show a person. One desktop assertion moves with it, since the 7z message is now prefixed with what happened before why. Closes #89 --- README.md | 4 +- apps/core/src/compression.rs | 81 +++++++++-- apps/core/tests/names.rs | 170 ++++++++++++++++++++++- apps/desktop/src-tauri/tests/commands.rs | 18 ++- bad.zip | 1 + docs/threat_model.md | 16 ++- 6 files changed, 269 insertions(+), 21 deletions(-) create mode 100644 bad.zip 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