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: