diff --git a/README.md b/README.md index 81b82d1..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 (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 diff --git a/apps/core/src/compression.rs b/apps/core/src/compression.rs index 13da057..4137783 100644 --- a/apps/core/src/compression.rs +++ b/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 { +) -> Result<(Vec, 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(()) } diff --git a/apps/core/tests/security.rs b/apps/core/tests/security.rs index 63851ff..28c433c 100644 --- a/apps/core/tests/security.rs +++ b/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); +} 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/apps/server-frontend/src/api.js b/apps/server-frontend/src/api.js index 738b14f..a5a0e28 100644 --- a/apps/server-frontend/src/api.js +++ b/apps/server-frontend/src/api.js @@ -4,8 +4,43 @@ // backend, and in development Vite does. That is deliberate, because the // backend ships no CORS layer and should not need one. -/** How often a running job is polled. */ -export const POLL_INTERVAL = 400 +/** + * How long to wait before the FIRST re-poll of a job the server has not + * finished yet. + * + * Short on purpose. Nearly every archive is done in less time than a person + * notices, and this loop used to sleep a flat 400 ms before asking a second + * time, so a tiny file spent almost all of its wall clock waiting on the + * client rather than on the server (issue #48). + * + * Not zero: the point is to stop making a finished job wait, not to spin on a + * server that is genuinely busy. These mirror `apps/remote/src/waiting.rs`, + * which fixed the same shape on the Rust side; keep them in step. + */ +export const FIRST_POLL_DELAY = 10 + +/** + * The ceiling the wait grows to, and the interval a long job settles into. + * + * Deliberately below the old flat 400 ms, and equal to the Rust client's + * ceiling, so the browser no longer waits longer than the CLI for the same + * job. + */ +export const MAX_POLL_DELAY = 200 + +/** + * The wait before the next poll, given the wait before the last one. + * + * Doubles until it reaches the ceiling: 10, 20, 40, 80, 160, 200, 200, ... + * + * It is not uniformly faster, and that is worth knowing rather than glossing: + * a job that finishes just after the ramp is asked again a whole ceiling + * later, where the flat schedule might have caught it sooner. The band is + * narrow and bounded by one ceiling. + */ +export function nextDelay(previous) { + return Math.min(previous * 2, MAX_POLL_DELAY) +} /** Read the backend's error shape, falling back to the status line. */ async function failure(response) { @@ -75,6 +110,7 @@ export async function compress( if (!job?.job_id) throw new Error('malformed server response: no job_id') let last = null + let delay = FIRST_POLL_DELAY for (;;) { const polled = await fetcher(`/jobs/${job.job_id}`) if (!polled.ok) throw await failure(polled) @@ -85,7 +121,8 @@ export async function compress( onStatus(current.status) } if (progressOf(current) === 'ready') break - await sleep(POLL_INTERVAL) + await sleep(delay) + delay = nextDelay(delay) } onStatus('downloading') diff --git a/apps/server-frontend/tests/api.test.js b/apps/server-frontend/tests/api.test.js index d209765..05982c0 100644 --- a/apps/server-frontend/tests/api.test.js +++ b/apps/server-frontend/tests/api.test.js @@ -1,5 +1,12 @@ import { describe, it, expect, vi } from 'vitest' -import { compress, health, progressOf } from '../src/api.js' +import { + compress, + health, + progressOf, + nextDelay, + FIRST_POLL_DELAY, + MAX_POLL_DELAY, +} from '../src/api.js' /** A fetch stub driven by a list of canned responses, in order. */ function fetcherFrom(responses) { @@ -184,3 +191,75 @@ describe('compress', () => { ).rejects.toThrow(/no job_id/) }) }) + +describe('the poll schedule', () => { + /** + * Record every delay the loop asks for and fire it at once, so a schedule + * spanning seconds of nominal waiting is tested in no time at all and with + * no wall clock in the assertions. + */ + function captureDelays() { + const waits = [] + const real = globalThis.setTimeout + vi.spyOn(globalThis, 'setTimeout').mockImplementation((fn, ms) => { + waits.push(ms) + return real(fn, 0) + }) + return waits + } + + /** A job that answers `compressing` `n` times before it completes. */ + function jobTaking(n) { + const responses = [json({ job_id: 'j' })] + for (let i = 0; i < n; i += 1) responses.push(json({ status: 'compressing' })) + responses.push(json({ status: 'completed' })) + responses.push(json({})) // download + responses.push(json({})) // delete + return fetcherFrom(responses) + } + + /** + * Issue #48, the half this file owns. The loop slept a flat 400 ms before + * asking a second time, so a job the server had already finished cost the + * browser that much, and twice what the CLI cost after the Rust half was + * fixed. + */ + it('does not make a job that finishes at once wait out the ceiling', async () => { + const waits = captureDelays() + const { fetcher } = jobTaking(1) + + await compress( + { body: new Blob(['x']), name: 'a.txt', algorithm: 'zip', level: 3 }, + { fetcher }, + ) + + expect(waits).toEqual([FIRST_POLL_DELAY]) + expect(FIRST_POLL_DELAY).toBeLessThan(400) + }) + + it('doubles to the ceiling and then holds', async () => { + const waits = captureDelays() + const { fetcher } = jobTaking(8) + + await compress( + { body: new Blob(['x']), name: 'a.txt', algorithm: 'zip', level: 3 }, + { fetcher }, + ) + + expect(waits).toEqual([10, 20, 40, 80, 160, 200, 200, 200]) + expect(Math.max(...waits)).toBe(MAX_POLL_DELAY) + }) + + /** + * The two clients must not drift. The browser waiting longer than the CLI + * for the same job is exactly what this issue was about. + */ + it('keeps the same schedule the Rust client uses', () => { + expect(FIRST_POLL_DELAY).toBe(10) + expect(MAX_POLL_DELAY).toBe(200) + expect(nextDelay(FIRST_POLL_DELAY)).toBe(20) + expect(nextDelay(160)).toBe(MAX_POLL_DELAY) + expect(nextDelay(MAX_POLL_DELAY)).toBe(MAX_POLL_DELAY) + expect(nextDelay(10_000)).toBe(MAX_POLL_DELAY) + }) +}) diff --git a/docs/threat_model.md b/docs/threat_model.md index 230e645..709b6b7 100644 --- a/docs/threat_model.md +++ b/docs/threat_model.md @@ -115,6 +115,41 @@ materializes a symlink, so nothing is planted. --- +### 4a. An entry written over the archive being read + +**Attack.** Not an attack so much as a foot-gun the product armed itself with: +an archive holding an entry named after the archive, extracted into the +archive's own directory. The entry is written onto the file still being read, so +the archive is truncated mid-read and what replaces it is whatever fraction the +extractor had reached. The contents are lost from the output as much as from +disk. + +Measured before the guard, on all three formats: `Ok`, "Extracted 1 file(s)", +and a 132 byte archive replaced by the 12 bytes it contained. Two of the three +reported success while doing it. + +**Prevention.** The planning pass already reads the whole listing before a byte +is written, so it now also resolves each entry's destination and refuses one +that turns out to be the archive itself. By **file identity**, not by path: a +hardlink is a second name for one file and never resolves to the same string, +which is precisely how `--force` was once able to overwrite its own source on +the compression side. + +The check follows the **planned** name rather than the archive's spelling, since +a rename can land an entry on the archive that the archive's own name does not +match. + +This is the mirror of a guard compression has always had (`OutputIsSource`, and +an output inside the folder being archived, neither of which `--force` unlocks). +Extraction simply had no equivalent, so the same product held two opposite +positions on the same question (issue #96). + +**Covered by** `no_format_writes_an_entry_over_the_archive_it_is_reading`, +`a_hardlink_to_the_archive_is_not_a_way_around_it`, +`the_check_follows_the_renamed_name_not_the_archive_s`, and +`the_same_archive_extracts_normally_somewhere_else`, which is the one that stops +the guard from being fixed by refusing too much. + ### 4b. Entry names this host cannot write **Attack.** An entry name that a filesystem does not reject but *reinterprets*. @@ -249,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: