Stop extracting an archive whose listing cannot be read

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

apps/core/src/compression.rs

@@ -398,6 +398,49 @@ fn algorithm_of(archive: &Path) -> Result<Algorithm, CompressionError> {
         .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::<Vec<_>>().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<Vec<String>, CompressionError> {
     match algorithm {
@@ -446,8 +489,9 @@ pub fn extract(archive: &Path, output_dir: &Path) -> Result<Vec<String>, 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<NamePlan, CompressionError> {
-    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())?)
 }

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: <four unprintable bytes> when
+/// getting cksum for <a hundred more>". 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()
+    );
+}

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<PathBuf> = fs::read_dir(&out_dir)
                     .map(|entries| entries.map(|e| e.unwrap().path()).collect())
                     .unwrap_or_default();

bad.zip

@@ -0,0 +1 @@
+PK and then nothing

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