Stop telling clients where this machine keeps things

README.md

@@ -188,8 +188,8 @@ Requires **Rust 1.88+** (2021 edition).
 
 ```bash
 make build             # build the Rust crates
-make test              # run every suite (608 Rust tests + 116 Vitest cases)
-make test/rust         # only the Rust tests that need no Node toolchain (493)
+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/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))
 }

apps/server-backend/tests/api.rs

@@ -936,3 +936,107 @@ async fn a_job_this_build_cannot_read_answers_500_with_an_explanation() {
         "and not the database's own words: {detail}"
     );
 }
+
+// ---------------------------------------------------------------------------
+// What a failed job tells a client about this machine (issue #66)
+// ---------------------------------------------------------------------------
+
+/// A tar whose second entry has the first, a plain file, for a parent.
+///
+/// Built by hand because a real directory cannot hold this shape: the
+/// filesystem would refuse to create `a.txt/b.txt` under a file. It is what
+/// makes `extract_tar` fail at a write site, which is where the staging path
+/// used to end up in the client's error message.
+fn tar_with_a_file_for_a_parent() -> Vec<u8> {
+    let mut builder = tar::Builder::new(Vec::new());
+    for (name, content) in [
+        ("photos/a.txt", &b"a file"[..]),
+        ("photos/a.txt/b.txt", &b"a child of a file"[..]),
+    ] {
+        let mut header = tar::Header::new_gnu();
+        header.set_size(content.len() as u64);
+        header.set_mode(0o644);
+        header.set_entry_type(tar::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();
+    }
+    builder.into_inner().unwrap()
+}
+
+/// Issue #66. `error_message` is returned by `GET /jobs/{id}`, the server has no
+/// authentication, and it used to hand back the absolute path of the job's
+/// staging directory:
+///
+/// ```text
+/// Compression failed: failed to unpack `/…/jobs/<uuid>/tree/photos/a.txt/b.txt`
+/// ```
+///
+/// That is the server's storage layout, told to anyone who can reach the port.
+#[tokio::test]
+async fn a_failed_job_tells_the_client_nothing_about_where_the_server_keeps_things() {
+    let (router, storage) = app();
+    let accepted = post_compress(
+        &router,
+        "name=photos&envelope=tar",
+        &tar_with_a_file_for_a_parent(),
+    )
+    .await;
+    assert_eq!(accepted.status(), StatusCode::ACCEPTED);
+    let job_id = body_json(accepted).await["job_id"]
+        .as_str()
+        .unwrap()
+        .to_string();
+
+    let done = wait_for_job(&router, &job_id).await;
+    assert_eq!(done["status"], "failed");
+    let told = done["error_message"].as_str().unwrap();
+
+    assert!(
+        !told.is_empty(),
+        "a failure with nothing to say is no better"
+    );
+    // Nothing absolute, and nothing naming this machine.
+    assert!(
+        !told.contains('/')
+            || !told
+                .split_whitespace()
+                .any(|w| w.trim_matches('`').starts_with('/')),
+        "an absolute path reached the client: {told}"
+    );
+    let root = storage.path().to_string_lossy().to_string();
+    assert!(
+        !told.contains(&root),
+        "the staging directory reached the client: {told}"
+    );
+    assert!(
+        !told.contains(&job_id),
+        "the job's directory name reached the client: {told}"
+    );
+}
+
+/// The redaction must not swallow the failure itself. A client that is told
+/// nothing cannot tell a hostile upload from a broken server.
+#[tokio::test]
+async fn a_failed_job_still_says_what_went_wrong() {
+    let (router, _storage) = app();
+    let accepted = post_compress(
+        &router,
+        "name=photos&envelope=tar",
+        &tar_with_a_file_for_a_parent(),
+    )
+    .await;
+    let job_id = body_json(accepted).await["job_id"]
+        .as_str()
+        .unwrap()
+        .to_string();
+
+    let done = wait_for_job(&router, &job_id).await;
+    let told = done["error_message"].as_str().unwrap();
+
+    assert!(
+        told.contains("unpack") || told.contains("entry") || told.contains("<path>"),
+        "it has to describe the failure, not just refuse to: {told}"
+    );
+}