Release v0.9.0 (final)

README.md

@@ -188,8 +188,8 @@ Requires **Rust 1.88+** (2021 edition).
 
 ```bash
 make build             # build the Rust crates
-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              # run every suite (614 Rust tests + 116 Vitest cases)
+make test/rust         # only the Rust tests that need no Node toolchain (499)
 ```
 
 `make test` includes the desktop app's own Rust suite, which compiles Tauri, so

apps/core/src/compression.rs

@@ -103,6 +103,24 @@ pub enum CompressionError {
         source: std::io::Error,
     },
 
+    /// One of the archive's own entries would be written over the archive.
+    ///
+    /// Refused outright rather than made overridable. Writing an entry onto the
+    /// archive being read truncates it mid-read: the archive is gone and what
+    /// replaces it is whatever fraction the extractor had reached, so the
+    /// contents are lost from the output as much as from disk. Nobody agrees to
+    /// that by asking to extract something.
+    ///
+    /// Compression has always refused the mirror image of this, and by file
+    /// identity rather than by path, so that a hardlink cannot slip past
+    /// (`paths::same_file`). Extraction had no equivalent (issue #96).
+    #[error(
+        "the entry {entry:?} would be written over the archive itself ({}), so nothing was \
+         extracted. Extract into a different directory.",
+        archive.display()
+    )]
+    WouldOverwriteArchive { archive: PathBuf, entry: String },
+
     /// An entry name this filesystem cannot hold, or an answer that does not
     /// resolve one.
     #[error(transparent)]
@@ -502,7 +520,8 @@ pub fn extract_with(
     // 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 plan = plan_for(archive, algorithm, options)?;
+    let (names, plan) = plan_for(archive, algorithm, options)?;
+    refuse_overwriting_the_archive(archive, output_dir, &names, &plan)?;
 
     match algorithm {
         Algorithm::SevenZ => self::sevenz::extract_7z_planned(archive, output_dir, &plan),
@@ -547,7 +566,42 @@ fn plan_for(
     archive: &Path,
     algorithm: Algorithm,
     options: &ExtractOptions,
-) -> Result<NamePlan, CompressionError> {
+) -> Result<(Vec<String>, NamePlan), CompressionError> {
     let names = list_entries(archive, algorithm).map_err(unreadable_archive)?;
-    Ok(plan_names(&names, options.rules(), options.replacements())?)
+    let plan = plan_names(&names, options.rules(), options.replacements())?;
+    Ok((names, plan))
+}
+
+/// 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
+/// per entry and no extra pass over the file.
+///
+/// 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
+/// was fixed. Repeating the path comparison here would repeat the bug.
+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 {
+            // Not containable at all. The backend rejects it as traversal, and
+            // 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) {
+            return Err(CompressionError::WouldOverwriteArchive {
+                archive: archive.to_path_buf(),
+                entry: name.clone(),
+            });
+        }
+    }
+    Ok(())
 }

apps/core/tests/security.rs

@@ -799,3 +799,113 @@ fn a_colon_in_a_later_component_cannot_clear_the_path_being_built() {
         }
     }
 }
+
+// -------------------------------------- writing over the archive being read --
+
+/// Issue #96. An archive holding an entry with its own name, extracted into its
+/// own directory, used to overwrite itself and report success.
+///
+/// Measured before this guard, on all three formats: `Ok`, "Extracted 1
+/// file(s)", and the archive replaced by the 12 bytes it contained.
+///
+/// The asymmetry is what made it indefensible rather than merely unfortunate:
+/// compression has always refused to write an archive over its own source, and
+/// refuses it even with `--force`, while extraction had no equivalent at all.
+#[test]
+fn no_format_writes_an_entry_over_the_archive_it_is_reading() {
+    for ext in ["zip", "7z", "tar"] {
+        let dir = tempfile::TempDir::new().unwrap();
+        let archive = dir.path().join(format!("victim.{ext}"));
+
+        // An archive whose single entry is named after the archive itself.
+        let entry = format!("victim.{ext}");
+        match ext {
+            "zip" => malicious_zip(&archive, &entry),
+            "7z" => malicious_7z(&archive, &entry),
+            _ => malicious_tar(&archive, &entry),
+        }
+        let before = std::fs::read(&archive).unwrap();
+
+        let err = extract(&archive, dir.path())
+            .expect_err("extracting into its own directory must be refused");
+
+        assert!(
+            err.to_string().contains("over the archive itself"),
+            "{ext}: {err}"
+        );
+        assert_eq!(
+            std::fs::read(&archive).unwrap(),
+            before,
+            "{ext}: the archive was modified"
+        );
+    }
+}
+
+/// The same guard, reached through a second name for the same file.
+///
+/// A hardlink never resolves to the same path, so a string comparison would
+/// wave this through. That is not hypothetical: it is exactly how `--force`
+/// used to be able to overwrite its own source on the compression side, which
+/// is why `paths::same_file` exists and why this uses it.
+#[cfg(unix)]
+#[test]
+fn a_hardlink_to_the_archive_is_not_a_way_around_it() {
+    let dir = tempfile::TempDir::new().unwrap();
+    let archive = dir.path().join("real.zip");
+    malicious_zip(&archive, "alias.zip");
+
+    std::fs::hard_link(&archive, dir.path().join("alias.zip")).unwrap();
+    let before = std::fs::read(&archive).unwrap();
+
+    let err = extract(&archive, dir.path()).expect_err("a second name is still the same file");
+    assert!(err.to_string().contains("over the archive itself"), "{err}");
+    assert_eq!(std::fs::read(&archive).unwrap(), before);
+}
+
+/// The guard must not cost anyone an extraction that was never dangerous.
+///
+/// Same archive, same entry name, a different output directory: nothing to
+/// refuse. Without this the fix could be "refuse everything that looks vaguely
+/// like the archive" and still pass the two tests above.
+#[test]
+fn the_same_archive_extracts_normally_somewhere_else() {
+    for ext in ["zip", "7z", "tar"] {
+        let dir = tempfile::TempDir::new().unwrap();
+        let archive = dir.path().join(format!("victim.{ext}"));
+        let entry = format!("victim.{ext}");
+        match ext {
+            "zip" => malicious_zip(&archive, &entry),
+            "7z" => malicious_7z(&archive, &entry),
+            _ => malicious_tar(&archive, &entry),
+        }
+
+        let out = dir.path().join("elsewhere");
+        let files = extract(&archive, &out).unwrap_or_else(|e| panic!("{ext}: {e}"));
+        assert_eq!(listing(files), vec![entry.clone()], "{ext}");
+        assert!(out.join(&entry).exists(), "{ext}");
+    }
+}
+
+/// A renamed entry must be checked at the name it will actually be written
+/// under, not the one the archive spells.
+///
+/// The archive is `v_.zip` and its entry is `v?.zip`, which is nothing special
+/// on Unix; under Windows rules the `?` is answered with `_`, so the entry
+/// 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() {
+    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 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}");
+    assert_eq!(std::fs::read(&archive).unwrap(), before);
+}

apps/server-backend/src/error.rs

@@ -53,24 +53,119 @@ impl From<crate::registry::RegistryError> for ApiError {
 
 /// The `error_message` a failed job carries, given what the engine returned.
 ///
-/// Clients (the CLI, the web app, `curl`) print this verbatim, so it is written
-/// for a person, and it is a `Display` rather than a `Debug` dump for the same
-/// reason.
+/// Clients (the CLI, the web app, `curl`) print the client half verbatim, so it
+/// is written for a person, and it is a `Display` rather than a `Debug` dump
+/// for the same reason.
 ///
-/// Only a verification failure is rewritten, and only because the engine's own
-/// message names the file it read back, which here is a path inside the job's
-/// staging directory: a location the client has never heard of, cannot reach,
-/// and should not be told about. The archive's own name is the same fact said
-/// in the client's vocabulary. Every other error already reads as a sentence
-/// about something the client did (an unreadable upload, a tar that is not a
-/// tar), so it is passed through untouched.
-pub fn failure_message(archive_name: &str, error: &CompressionError) -> String {
-    match error {
+/// **The two halves exist because the audiences differ.** An operator reading
+/// the log wants the whole truth, including which file on their disk was at
+/// fault. A client wants to know what *it* did wrong, and must not be told
+/// where anything lives on a machine it has never heard of and cannot reach.
+/// The server has no authentication (issue #72), so "a client" is anyone who
+/// can reach the port.
+pub struct Failure {
+    /// What the job's `error_message` becomes, and so what `GET /jobs/{id}`
+    /// hands back. Never names a location on this machine.
+    pub client: String,
+    /// The same failure, whole, for the log.
+    pub log: String,
+}
+
+impl Failure {
+    /// Both halves from one message.
+    pub fn from_message(message: String) -> Self {
+        Self {
+            client: without_locations(&message),
+            log: message,
+        }
+    }
+}
+
+/// So a `?` on any of the plain-`String` failures in this crate still works,
+/// and still gets redacted. Fail closed: a new error path is safe by default
+/// rather than safe only if someone remembers.
+impl From<String> for Failure {
+    fn from(message: String) -> Self {
+        Self::from_message(message)
+    }
+}
+
+/// Turn a core error into what the client is told and what the log records.
+///
+/// Two variants are rewritten rather than redacted, because a curated sentence
+/// is more useful than a redacted one:
+///
+/// * a verification failure names the file it read back, which is a path inside
+///   the job's staging directory; the archive's own name is the same fact in the
+///   client's vocabulary;
+/// * a per-entry write failure names the entry, which is the client's own
+///   content and worth keeping, and its destination, which is not.
+///
+/// **Everything else is redacted rather than passed through.** That is the
+/// change: the old code passed every other variant through untouched, on the
+/// stated reasoning that they "already read as a sentence about something the
+/// client did". That was not true. Unpacking a client's tar envelope reaches
+/// `extract_tar`, whose failure reads ``failed to unpack `/…/out/root/a/b` ``,
+/// and it did not even go through here (issue #66).
+///
+/// Enumerating the leaky variants would have been the smaller change and the
+/// wrong one: `CompressionError` gains variants, and the next one would leak
+/// until somebody noticed.
+pub fn failure(archive_name: &str, error: &CompressionError) -> Failure {
+    let client = match error {
         CompressionError::VerificationFailed { reason, .. } => format!(
             "{archive_name} was compressed but did not check out, so it was discarded: {reason}"
         ),
-        other => other.to_string(),
+        CompressionError::Entry { entry, source, .. } => {
+            format!("the entry {entry:?} could not be written: {source}")
+        }
+        other => without_locations(&other.to_string()),
+    };
+    Failure {
+        client,
+        log: error.to_string(),
+    }
+}
+
+/// Remove anything shaped like a path on this machine.
+///
+/// Deliberately blunt. The server has no reason to tell a client where anything
+/// lives, so removing every absolute path is correct rather than merely
+/// convenient, and it does not depend on knowing which variant produced the
+/// message or where the staging directory happens to be mounted.
+///
+/// Relative paths are left alone: those are the client's own entry names, which
+/// are exactly what it needs to see.
+fn without_locations(message: &str) -> String {
+    message
+        .split_whitespace()
+        .map(|word| {
+            // A path is usually wrapped in the punctuation of the sentence
+            // around it: backticks, quotes, a trailing comma or colon.
+            let trimmed = word.trim_matches(|c: char| {
+                c == '`' || c == '"' || c == '\'' || c == ',' || c == ':' || c == '.'
+            });
+            if looks_like_a_location(trimmed) {
+                word.replace(trimmed, "<path>")
+            } else {
+                word.to_string()
+            }
+        })
+        .collect::<Vec<_>>()
+        .join(" ")
+}
+
+/// Absolute on Unix, or carrying a Windows drive or UNC prefix.
+fn looks_like_a_location(word: &str) -> bool {
+    if word.starts_with('/') || word.starts_with("\\\\") {
+        return true;
     }
+    // `C:\...` or the verbatim `\\?\C:\...` that canonicalize returns.
+    let mut chars = word.chars();
+    matches!(
+        (chars.next(), chars.next(), chars.next()),
+        (Some(letter), Some(':'), Some('\\' | '/')) if letter.is_ascii_alphabetic()
+    )
 }
 
 /// What can stop the server from coming up: the two things it owns are the

apps/server-backend/src/queue.rs

@@ -8,7 +8,7 @@ use std::path::Path;
 use collapse_core::compression::extract_tar;
 use collapse_core::{compress, compress_dir};
 
-use crate::error::failure_message;
+use crate::error::{failure, Failure};
 use crate::models::{Envelope, Job, JobStatus};
 use crate::registry::Registry;
 use crate::storage::{single_root_dir, Storage};
@@ -79,7 +79,7 @@ async fn process_job(registry: &Registry, storage: &Storage, job_id: &str) {
             job.level,
             job.verify.into(),
         )
-        .map_err(|e| failure_message(&job.archive_name, &e)),
+        .map_err(|e| failure(&job.archive_name, &e)),
         Envelope::Tar => unwrap_and_compress(&input, &tree, &output, &job),
     })
     .await;
@@ -96,9 +96,16 @@ async fn process_job(registry: &Registry, storage: &Storage, job_id: &str) {
         // A rejected upload (a hostile tar, an unreadable source) is the
         // client's problem, not the server's, so it is a warning; a worker
         // that dies mid-job is ours.
-        Ok(Err(message)) => {
-            set_status(registry, job_id, JobStatus::Failed, Some(message.clone()));
-            tracing::warn!(job = %job_id, elapsed_ms, error = %message, "failed");
+        Ok(Err(failure)) => {
+            // The client is told the redacted half; the log keeps the whole
+            // truth, including the path an operator needs to find the file.
+            set_status(
+                registry,
+                job_id,
+                JobStatus::Failed,
+                Some(failure.client.clone()),
+            );
+            tracing::warn!(job = %job_id, elapsed_ms, error = %failure.log, "failed");
         }
         Err(e) => {
             set_status(registry, job_id, JobStatus::Failed, Some(e.to_string()));
@@ -112,9 +119,11 @@ async fn process_job(registry: &Registry, storage: &Storage, job_id: &str) {
 /// Extraction goes through the engine's tar backend, which refuses entries
 /// that would escape the output directory and materializes no links, so a
 /// hostile tar cannot reach outside the job's own staging area.
-fn unwrap_and_compress(input: &Path, tree: &Path, output: &Path, job: &Job) -> Result<(), String> {
-    extract_tar(input, tree).map_err(|e| e.to_string())?;
+fn unwrap_and_compress(input: &Path, tree: &Path, output: &Path, job: &Job) -> Result<(), Failure> {
+    // This one used to bypass the curation entirely and hand the client
+    // `failed to unpack \`<staging path>\`` (issue #66).
+    extract_tar(input, tree).map_err(|e| failure(&job.archive_name, &e))?;
     let root = single_root_dir(tree, &job.name)?;
     compress_dir(&root, output, job.algorithm, job.level, job.verify.into())
-        .map_err(|e| failure_message(&job.archive_name, &e))
+        .map_err(|e| failure(&job.archive_name, &e))
 }