From 9f0e02e5765915983e890076fa3a62f6257abe30 Mon Sep 17 00:00:00 2001 From: Javier Parada Date: Thu, 27 Aug 2026 12:08:07 +0200 Subject: [PATCH] server-backend: stop telling clients where this machine keeps things MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `GET /jobs/{id}` returns a failed job's `error_message`, and that message came from the engine, which names the file it was working on. For a job that file is inside the staging directory, so an ordinary upload learned the server's storage layout: Compression failed: failed to unpack `/var/lib/collapse/jobs//tree/…` Reachable with a tar whose second entry has the first, a plain file, for a parent. The server has no authentication (issue #72, untouched here), so "a client" is anyone who can reach the port. Two things were wrong and only one was obvious. The obvious one: unpacking a tar envelope did not go through `failure_message` at all. The other: the curation it skipped would not have helped much, because it rewrote one variant and passed the rest through, on the stated reasoning that they "already read as a sentence about something the client did". So the rule is inverted rather than extended. Redact unless there is a curated sentence. Enumerating the leaky variants would have been the smaller change and the wrong one: `CompressionError` gains variants, and the next one would have leaked until somebody noticed. `From for Failure` redacts too, so a new error path is safe by default rather than safe only if someone remembers. Redaction is blunt on purpose. 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 is mounted. Relative paths survive: those are the client's own entry names. `Failure { client, log }` splits the two audiences. An operator wants the whole truth including the path; a client must not have it. Verified end to end against a running server: client: Compression failed: failed to unpack `` log: Compression failed: failed to unpack `/private/tmp/.../tree/…` One existing test asserted the old policy by name, `other_engine_errors_are_passed_through_word_for_word`. Its assertions still passed, since none of its fixtures held a path, but its name and comment claimed something no longer true. Rewritten rather than left to mislead. Both mutations are caught: making the redaction a no-op, and letting `extract_tar` bypass the curation again. Closes #66 --- README.md | 4 +- apps/server-backend/src/error.rs | 121 +++++++++++++++++++++++++---- apps/server-backend/src/queue.rs | 25 ++++-- apps/server-backend/tests/api.rs | 104 +++++++++++++++++++++++++ apps/server-backend/tests/error.rs | 97 +++++++++++++++++++++-- docs/threat_model.md | 36 +++++++++ 6 files changed, 357 insertions(+), 30 deletions(-) diff --git a/README.md b/README.md index fff003b..32000d0 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 (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 diff --git a/apps/server-backend/src/error.rs b/apps/server-backend/src/error.rs index be2ecec..8b54a61 100644 --- a/apps/server-backend/src/error.rs +++ b/apps/server-backend/src/error.rs @@ -53,24 +53,119 @@ impl From 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 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, "") + } else { + word.to_string() + } + }) + .collect::>() + .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 diff --git a/apps/server-backend/src/queue.rs b/apps/server-backend/src/queue.rs index 2f3a14a..5c68b09 100644 --- a/apps/server-backend/src/queue.rs +++ b/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 \`\`` (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)) } diff --git a/apps/server-backend/tests/api.rs b/apps/server-backend/tests/api.rs index 1cf8271..59b6f06 100644 --- a/apps/server-backend/tests/api.rs +++ b/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 { + 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//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(""), + "it has to describe the failure, not just refuse to: {told}" + ); +} diff --git a/apps/server-backend/tests/error.rs b/apps/server-backend/tests/error.rs index ce88e33..4878375 100644 --- a/apps/server-backend/tests/error.rs +++ b/apps/server-backend/tests/error.rs @@ -5,7 +5,7 @@ use axum::response::{IntoResponse, Response}; use http_body_util::BodyExt; use collapse_core::CompressionError; -use collapse_server_backend::error::{failure_message, ApiError}; +use collapse_server_backend::error::{failure, ApiError}; async fn detail_of(response: Response) -> String { let bytes = response.into_body().collect().await.unwrap().to_bytes(); @@ -69,7 +69,7 @@ fn a_verification_failure_names_the_archive_not_the_servers_own_path() { reason: "1 entry is missing: \"photos/b.jpg\"".to_string(), }; - let message = failure_message("photos.zip", &error); + let message = failure("photos.zip", &error).client; assert!( message.contains("photos.zip"), @@ -95,16 +95,17 @@ fn a_verification_failure_reads_as_a_sentence_and_keeps_the_reason() { }; assert_eq!( - failure_message("photos.zip", &error), + failure("photos.zip", &error).client, "photos.zip was compressed but did not check out, so it was discarded: \ 2 entries are missing: \"a.txt\", \"b.txt\"" ); } -/// Every other engine error already says something the client can act on, and -/// rewriting those would throw away the only description of what went wrong. +/// An engine error that says something the client can act on, and names no +/// location, still reaches it word for word. Redacting those would throw away +/// the only description of what went wrong. #[test] -fn other_engine_errors_are_passed_through_word_for_word() { +fn an_engine_error_that_names_no_location_is_passed_through_word_for_word() { for error in [ CompressionError::Failed("unexpected end of file".to_string()), CompressionError::InvalidLevel(9), @@ -113,6 +114,88 @@ fn other_engine_errors_are_passed_through_word_for_word() { "denied", )), ] { - assert_eq!(failure_message("photos.zip", &error), error.to_string()); + assert_eq!(failure("photos.zip", &error).client, error.to_string()); } } + +/// The half this used to get wrong. +/// +/// The old rule was "rewrite a verification failure, pass everything else +/// through", justified on the reasoning that every other variant "already reads +/// as a sentence about something the client did". It does not. Unpacking a +/// client's tar envelope reaches `extract_tar`, and its failure names a path +/// inside the staging directory (issue #66). +/// +/// The rule is now the other way round: redact unless there is a curated +/// sentence, so a variant added later is safe by default rather than safe only +/// if somebody remembers. +#[test] +fn an_engine_error_that_names_a_location_does_not_reach_the_client() { + let leaky = [ + // Exactly what the tar envelope path produced. + CompressionError::Failed( + "failed to unpack `/var/lib/collapse/jobs/abc123/tree/root/a/b`".to_string(), + ), + // A Windows host, including the verbatim prefix `canonicalize` adds. + CompressionError::Failed( + "cannot write to \\\\?\\C:\\ProgramData\\collapse\\jobs\\abc\\out".to_string(), + ), + CompressionError::Failed("cannot read C:\\jobs\\abc\\input".to_string()), + ]; + + for error in leaky { + let told = failure("photos.zip", &error).client; + assert!( + !told.contains("/var/lib") && !told.contains("C:\\") && !told.contains("ProgramData"), + "a path reached the client: {told}" + ); + assert!( + told.contains(""), + "and it says something was removed: {told}" + ); + } +} + +/// The operator loses nothing. The log half is the failure whole, path and all, +/// because the person reading it is the one who can act on the path. +#[test] +fn the_log_half_keeps_what_the_client_half_drops() { + let error = CompressionError::Failed( + "failed to unpack `/var/lib/collapse/jobs/abc123/tree/x`".to_string(), + ); + let both = failure("photos.zip", &error); + + assert!( + both.log.contains("/var/lib/collapse/jobs/abc123"), + "{}", + both.log + ); + assert!(!both.client.contains("/var/lib"), "{}", both.client); +} + +/// A per-entry failure keeps the entry, which is the client's own content and +/// the useful half, and drops the destination, which is ours. +#[test] +fn a_failing_entry_names_the_entry_but_not_where_it_was_going() { + let error = CompressionError::Entry { + entry: "photos/a.jpg".to_string(), + destination: std::path::PathBuf::from("/var/lib/collapse/jobs/abc123/tree/photos/a.jpg"), + source: std::io::Error::new(std::io::ErrorKind::PermissionDenied, "denied"), + }; + + let told = failure("photos.zip", &error).client; + assert!(told.contains("photos/a.jpg"), "{told}"); + assert!(!told.contains("/var/lib"), "{told}"); +} + +/// A relative path is the client's own entry name, not a location on this +/// machine, and must survive. +#[test] +fn a_relative_path_is_not_mistaken_for_a_location() { + let error = CompressionError::Failed("cannot read photos/2026/a.jpg".to_string()); + assert_eq!( + failure("photos.zip", &error).client, + error.to_string(), + "an entry name was redacted as if it were a location" + ); +} diff --git a/docs/threat_model.md b/docs/threat_model.md index 0b81fbb..709b6b7 100644 --- a/docs/threat_model.md +++ b/docs/threat_model.md @@ -284,6 +284,42 @@ so the existing `--max-upload-mb` cap also bounds what reaches the disk. A zip or 7z envelope would have introduced a decompression bomb where there is none today, which is why it is not offered. +### 8b. What a failed job tells the client + +**Attack.** Not an attack, an oversight in a message. `GET /jobs/{id}` returns a +failed job's `error_message`, and that message came from the engine, which names +the file it was working on. For a job, that file lives inside the staging +directory, so a client learned where the server keeps things: + +```text +Compression failed: failed to unpack `/var/lib/collapse/jobs//tree/photos/a.txt/b.txt` +``` + +Reachable with an ordinary upload: a tar whose second entry has the first, a +plain file, for a parent. Since the server has no authentication (see below), +"a client" is anyone who can reach the port. + +**Prevention.** A failure now has two halves. The client is told a message with +every absolute path removed; the log keeps the failure whole, because the person +reading it is the one who can act on the path. + +The rule is **redact unless there is a curated sentence**, not the other way +round. It used to be "rewrite a verification failure, pass everything else +through", justified on the reasoning that every other variant already read as a +sentence about something the client did. It did not, and enumerating the leaky +variants would have left the next one leaking until somebody noticed. Redaction +is blunt on purpose: 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. + +Relative paths survive, because those are the client's own entry names and +exactly what it needs to see. A per-entry failure keeps the entry and drops the +destination. + +**Covered by** `a_failed_job_tells_the_client_nothing_about_where_the_server_keeps_things` +and `a_failed_job_still_says_what_went_wrong` in `apps/server-backend/tests/api.rs`, +which drive a real job end to end, plus the unit cases in `tests/error.rs`. + ### 9. What the server does not defend against Stated plainly, because deploying it assumes these: