From c2dd8269205d976adde07dff5dc58f69b99e08ca Mon Sep 17 00:00:00 2001 From: Chili Palmer Date: Thu, 27 Aug 2026 15:34:31 +0200 Subject: [PATCH] Implement audited findings --- Cargo.lock | 1 + Cargo.toml | 1 + FINDINGS.md | 91 ------- apps/tui/src/runtime.rs | 54 ++++ crates/storage/Cargo.toml | 2 + crates/storage/src/config.rs | 85 ++++++- crates/storage/src/git.rs | 200 ++++++++++++--- crates/storage/src/mutation.rs | 329 +++++++++++++++++++------ crates/storage/src/recipient.rs | 9 + crates/storage/src/repository.rs | 121 +++++++-- crates/storage/tests/tree_mutations.rs | 100 ++++++++ 11 files changed, 773 insertions(+), 220 deletions(-) delete mode 100644 FINDINGS.md diff --git a/Cargo.lock b/Cargo.lock index c8b438d..d55a921 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5415,6 +5415,7 @@ dependencies = [ "reqwest 0.13.4", "rqrr", "russh", + "rustls", "secret-service", "security-framework 3.7.0", "serde", diff --git a/Cargo.toml b/Cargo.toml index e02c602..82d8e5c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -50,6 +50,7 @@ qrcode = { version = "0.14", default-features = false } rand = "0.8" regex = "1.13" reqwest = { version = "0.13", default-features = false, features = ["blocking", "rustls"] } +rustls = { version = "0.23", default-features = false } rfd = { version = "0.17", default-features = false } rqrr = { version = "0.10", default-features = false } rpassword = "7.5" diff --git a/FINDINGS.md b/FINDINGS.md deleted file mode 100644 index fe8ac1d..0000000 --- a/FINDINGS.md +++ /dev/null @@ -1,91 +0,0 @@ -# Implementation Findings - -Confirmed, high-impact work only. Keep changes inside `crates/storage` unless a -finding explicitly names another crate. Prefer the smallest shared fix and add -one focused regression test per behavior change. - -## 1. Preserve operation and rollback failures - -Rollback code can replace the original failure with a restoration failure: - -- `Repository::rollback_created` in `crates/storage/src/repository.rs` -- `TreeMutator::remove` and `TreeMutator::transfer` in - `crates/storage/src/mutation.rs` - -Return an error containing both failures, following the existing -`WriteError::RollbackFailed { operation, rollback }` pattern. Cover failures -from reopening/syncing directories and from all source/destination restoration -steps. Never report successful rollback if any restoration step failed. - -Acceptance: focused tests prove that the original operation error and rollback -error are both retained. - -## 2. Use one snapshot per tree mutation - -Directory remove/move/copy currently scan the repository up to five times via -`snapshot`, `reject_unmanaged`, `source_entries`, `source_policies`, and -`source_directories` in `crates/storage/src/mutation.rs`. - -Take one snapshot at operation start and pass it to those helpers. All -resolution, validation, and source selection for the transaction must use that -same snapshot. - -Acceptance: existing mutation tests pass and a focused test or instrumentation -proves one snapshot is taken for a directory operation. - -## 3. Remove string-based HTTPS Git error classification - -`map_reqwest_error` and `fetch_embedded_https` in -`crates/storage/src/git.rs` classify authentication, TLS, and network failures -using substrings from third-party error messages. - -Use structured `reqwest` status/connect/timeout information, error sources, and -matchable `gix` variants wherever available. Classify TLS only when a structured -source identifies it. When `gix` has erased the underlying status or cause, -return a generic error with a bounded diagnostic instead of guessing a specific -class from rendered text. - -Acceptance: tests classify direct HTTP 401/403 and structured connection, -timeout, and TLS failures without depending on English error text. A `gix` -failure without a structured cause remains generic and its diagnostic is -bounded. - -## 4. Make config replacement durable - -`Config::persist` and `persist_new` in `crates/storage/src/config.rs` sync the -file but not the parent directory after replacement/creation. - -Sync the parent directory after the atomic rename, reusing the repository's -existing durability pattern where practical. Return a distinct durability error -if the rename succeeds but the directory sync fails. - -Acceptance: focused tests cover the post-rename sync failure boundary. Do not -add config locking unless concurrent writers are an explicit supported use case. - -## 5. Reap completed TUI task handles - -`AsyncExecutor::submit` in `apps/tui/src/runtime.rs` retains every `JoinHandle` -until executor drop. - -Remove and join finished handles during normal executor activity. Keep the -current thread-per-task design; do not add a thread pool without measured need. - -Acceptance: a focused test submits and completes repeated tasks and verifies -the retained handle count does not grow without bound. - -## Required checks - -```sh -cargo fmt --all -- --check -RUSTFLAGS="-D warnings" cargo check --workspace --all-targets -cargo clippy --workspace --all-targets -- -D warnings -cargo test --workspace -``` - -## Explicitly deferred - -Do not spend this pass on `thiserror`, broad error-enum redesign, Git options -structs, frontend file splitting, generic generation tracking, public struct -fields, UniFFI mirror removal, Git performance caching, config locking, or a -thread pool. Revisit only with a concrete bug, compatibility need, or measured -performance problem. diff --git a/apps/tui/src/runtime.rs b/apps/tui/src/runtime.rs index 4c56e9b..c57e11e 100644 --- a/apps/tui/src/runtime.rs +++ b/apps/tui/src/runtime.rs @@ -210,6 +210,7 @@ impl AsyncExecutor { where F: FnOnce() -> Result + Send + 'static, { + self.reap_finished(); let sender = self.sender.clone(); let task = thread::spawn(move || { let _ignored = sender.send(AsyncResult { @@ -223,6 +224,34 @@ impl AsyncExecutor { .push(task); } + fn reap_finished(&self) { + let mut tasks = self + .tasks + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let mut completed = Vec::new(); + let mut index = 0; + while index < tasks.len() { + if tasks[index].is_finished() { + completed.push(tasks.swap_remove(index)); + } else { + index += 1; + } + } + drop(tasks); + for task in completed { + let _ignored = task.join(); + } + } + + #[cfg(test)] + fn retained_task_count(&self) -> usize { + self.tasks + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .len() + } + pub fn progress_reporter( &self, token: RequestToken, @@ -253,6 +282,7 @@ impl AsyncExecutor { } pub fn drain(&self) -> impl Iterator + '_ { + self.reap_finished(); self.receiver.try_iter() } } @@ -313,4 +343,28 @@ mod tests { .expect("worker result"); assert_eq!(app.apply_result(result), ResultDisposition::Applied); } + + #[test] + fn completed_task_handles_are_reaped_during_normal_activity() { + let executor = AsyncExecutor::new(); + let mut app = App::new(); + let submitted = 256; + for _ in 0..submitted { + let token = app.begin_request(); + executor.submit(token, || Err("complete".to_owned())); + } + for _ in 0..submitted { + executor + .receiver + .recv_timeout(Duration::from_secs(2)) + .expect("worker result"); + } + + let deadline = std::time::Instant::now() + Duration::from_secs(2); + while executor.retained_task_count() != 0 && std::time::Instant::now() < deadline { + let _ = executor.drain().count(); + thread::yield_now(); + } + assert_eq!(executor.retained_task_count(), 0); + } } diff --git a/crates/storage/Cargo.toml b/crates/storage/Cargo.toml index dbf13a3..cd8763a 100644 --- a/crates/storage/Cargo.toml +++ b/crates/storage/Cargo.toml @@ -29,6 +29,7 @@ full = [ "dep:rand", "dep:regex", "dep:reqwest", + "dep:rustls", "dep:rqrr", "dep:secret-service", "dep:security-framework", @@ -61,6 +62,7 @@ qrcode = { workspace = true, optional = true } rand = { workspace = true, optional = true } regex = { workspace = true, optional = true } reqwest = { workspace = true, optional = true } +rustls = { workspace = true, optional = true } rqrr = { workspace = true, optional = true } russh = { workspace = true, optional = true } serde = { workspace = true, optional = true } diff --git a/crates/storage/src/config.rs b/crates/storage/src/config.rs index 77a1995..6551e14 100644 --- a/crates/storage/src/config.rs +++ b/crates/storage/src/config.rs @@ -6,7 +6,7 @@ use std::{ error::Error, fmt, fs, fs::OpenOptions, - io::Write, + io::{self, Write}, path::{Component, Path, PathBuf}, time::Duration, }; @@ -638,6 +638,13 @@ impl Config { } pub(crate) fn persist(&self) -> Result<(), ConfigError> { + self.persist_with_directory_sync(sync_config_directory) + } + + fn persist_with_directory_sync( + &self, + sync_parent: impl FnOnce(&Dir) -> io::Result<()>, + ) -> Result<(), ConfigError> { let parent = self.source.parent().ok_or_else(|| ConfigError::Write { path: self.source.clone(), })?; @@ -668,10 +675,20 @@ impl Config { .and_then(|()| temporary.replace(name)) .map_err(|_| ConfigError::Write { path: self.source.clone(), - }) + })?; + sync_parent(&directory).map_err(|_| ConfigError::DurabilityUncertain { + path: self.source.clone(), + }) } fn persist_new(&self) -> Result<(), ConfigError> { + self.persist_new_with_directory_sync(sync_config_directory) + } + + fn persist_new_with_directory_sync( + &self, + sync_parent: impl FnOnce(&Dir) -> io::Result<()>, + ) -> Result<(), ConfigError> { let parent = self.source.parent().ok_or_else(|| ConfigError::Write { path: self.source.clone(), })?; @@ -689,6 +706,10 @@ impl Config { set_private_directory(parent).map_err(|_| ConfigError::Write { path: self.source.clone(), })?; + let directory = + Dir::open_ambient_dir(parent, ambient_authority()).map_err(|_| ConfigError::Write { + path: self.source.clone(), + })?; let contents = toml::to_string_pretty(&self.document).map_err(|_| ConfigError::Write { path: self.source.clone(), })?; @@ -725,7 +746,9 @@ impl Config { drop(temporary_file); let _ = fs::remove_file(&temporary_path); match installed { - Ok(()) => Ok(()), + Ok(()) => sync_parent(&directory).map_err(|_| ConfigError::DurabilityUncertain { + path: self.source.clone(), + }), Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => { Err(ConfigError::AlreadyConfigured { path: self.source.clone(), @@ -738,6 +761,10 @@ impl Config { } } +fn sync_config_directory(directory: &Dir) -> io::Result<()> { + directory.open(".").and_then(|file| file.sync_all()) +} + fn git_remote_document(remote: &GitRemote) -> Result { let mut configured = toml::Table::new(); configured.insert( @@ -1412,6 +1439,7 @@ pub enum ConfigError { VaultUnavailable { path: PathBuf }, VaultIsNotDirectory { path: PathBuf }, Write { path: PathBuf }, + DurabilityUncertain { path: PathBuf }, AlreadyConfigured { path: PathBuf }, KeyMaterialNotFound { path: PathBuf }, InvalidKeyMaterial { path: PathBuf }, @@ -1483,6 +1511,11 @@ impl fmt::Display for ConfigError { path.display() ) } + Self::DurabilityUncertain { path } => write!( + formatter, + "configuration replacement completed but its directory sync failed: {}", + path.display() + ), Self::AlreadyConfigured { path } => write!( formatter, "configuration already exists and was not replaced: {}", @@ -2315,7 +2348,7 @@ fn native_config_directory() -> Option { #[cfg(test)] mod tests { - use std::fs; + use std::{fs, io}; use crate::mobile::MobileTab; @@ -2401,4 +2434,48 @@ key_material = "keys" assert!(contents.contains("vault = \"vault\"")); assert!(!contents.contains("OLD-CONTAINER")); } + + #[test] + fn post_install_directory_sync_failure_is_distinct_and_keeps_complete_config() { + let temporary = tempfile::tempdir().expect("temporary directory"); + let vault = temporary.path().join("vault"); + let keys = temporary.path().join("keys"); + let source = temporary.path().join("config.toml"); + fs::create_dir(&vault).expect("vault"); + fs::create_dir(&keys).expect("keys"); + let remote = GitRemote::https( + "origin", + "https://example.test/team/passwords.git", + "server-example", + "repository-example", + ) + .expect("remote"); + let config = Config::create_mobile_clone(source.clone(), &vault, &keys, "ALICE", &remote) + .expect("initial config"); + + let error = config + .persist_with_directory_sync(|_| Err(io::Error::other("simulated sync failure"))) + .expect_err("replacement directory sync must fail"); + assert_eq!( + error, + ConfigError::DurabilityUncertain { + path: source.clone() + } + ); + Config::load(Some(&source)).expect("replacement is complete"); + + let created = temporary.path().join("created.toml"); + let mut new_config = config; + new_config.source = created.clone(); + let error = new_config + .persist_new_with_directory_sync(|_| Err(io::Error::other("simulated sync failure"))) + .expect_err("creation directory sync must fail"); + assert_eq!( + error, + ConfigError::DurabilityUncertain { + path: created.clone() + } + ); + Config::load(Some(&created)).expect("creation is complete"); + } } diff --git a/crates/storage/src/git.rs b/crates/storage/src/git.rs index 6513650..0c22384 100644 --- a/crates/storage/src/git.rs +++ b/crates/storage/src/git.rs @@ -1022,8 +1022,8 @@ impl ReqwestGitTransport { fn response(response: reqwest::blocking::Response) -> Result, GitError> { let status = response.status(); - if status == reqwest::StatusCode::UNAUTHORIZED || status == reqwest::StatusCode::FORBIDDEN { - return Err(GitError::AuthenticationFailed); + if let Some(error) = classify_http_status(status) { + return Err(error); } if !status.is_success() { return Err(GitError::InvalidRepository(format!( @@ -1273,13 +1273,103 @@ fn map_ambiguous_push_error(error: GitError) -> GitError { fn map_reqwest_error(error: reqwest::Error) -> GitError { if error.is_builder() { - return invalid(error); + return generic_https_error(&error); } - let message = error.to_string().to_ascii_lowercase(); - if message.contains("certificate") || message.contains("tls") { - GitError::TlsFailed + classify_structured_https_error(&error).unwrap_or_else(|| generic_https_error(&error)) +} + +fn classify_http_status(status: reqwest::StatusCode) -> Option { + matches!( + status, + reqwest::StatusCode::UNAUTHORIZED | reqwest::StatusCode::FORBIDDEN + ) + .then_some(GitError::AuthenticationFailed) +} + +fn classify_structured_https_error(error: &(dyn Error + 'static)) -> Option { + let mut current = Some(error); + let mut tls = false; + let mut network = false; + while let Some(error) = current { + if let Some(error) = error.downcast_ref::() { + if error.status().and_then(classify_http_status).is_some() { + return Some(GitError::AuthenticationFailed); + } + network |= error.is_timeout() || error.is_connect(); + } + if error.is::() { + tls = true; + } + if let Some(error) = error.downcast_ref::() { + if error.kind() == std::io::ErrorKind::PermissionDenied { + return Some(GitError::AuthenticationFailed); + } + network |= matches!( + error.kind(), + std::io::ErrorKind::ConnectionRefused + | std::io::ErrorKind::ConnectionReset + | std::io::ErrorKind::ConnectionAborted + | std::io::ErrorKind::NotConnected + | std::io::ErrorKind::TimedOut + | std::io::ErrorKind::AddrNotAvailable + | std::io::ErrorKind::NetworkUnreachable + | std::io::ErrorKind::HostUnreachable + ); + } + current = error.source(); + } + if tls { + Some(GitError::TlsFailed) + } else if network { + Some(GitError::NetworkUnavailable) } else { - GitError::NetworkUnavailable + None + } +} + +fn generic_https_error(error: &dyn fmt::Display) -> GitError { + GitError::InvalidRepository(protocol_diagnostic(error.to_string().as_bytes())) +} + +fn map_https_connect_error(error: gix::remote::connect::Error) -> GitError { + match error { + error @ gix::remote::connect::Error::Connect(_) => { + classify_structured_https_error(&error).unwrap_or_else(|| generic_https_error(&error)) + } + error => generic_https_error(&error), + } +} + +fn map_https_prepare_error(error: gix::remote::fetch::prepare::Error) -> GitError { + match error { + error @ gix::remote::fetch::prepare::Error::RefMap( + gix::remote::ref_map::Error::Transport(_) | gix::remote::ref_map::Error::Handshake(_), + ) => classify_structured_https_error(&error).unwrap_or_else(|| generic_https_error(&error)), + error => generic_https_error(&error), + } +} + +fn map_https_fetch_error( + error: gix::remote::fetch::Error, + control: &GitOperationControl, +) -> GitError { + if control.is_cancelled() { + return GitError::Cancelled; + } + match error { + gix::remote::fetch::Error::Fetch(gix::protocol::fetch::Error::ConsumePack(_)) => { + GitError::MalformedGitPack + } + gix::remote::fetch::Error::Fetch( + gix::protocol::fetch::Error::FetchResponse(_) + | gix::protocol::fetch::Error::Negotiate(_) + | gix::protocol::fetch::Error::MissingServerFeature { .. }, + ) => GitError::GitProtocolFailed, + error @ (gix::remote::fetch::Error::Fetch(gix::protocol::fetch::Error::Client(_)) + | gix::remote::fetch::Error::Client(_)) => { + classify_structured_https_error(&error).unwrap_or_else(|| generic_https_error(&error)) + } + error => generic_https_error(&error), } } @@ -1905,7 +1995,7 @@ impl GitRepository { .map_err(invalid)?; let mut connection = remote .connect(gix::remote::Direction::Fetch) - .map_err(invalid)?; + .map_err(map_https_connect_error)?; connection.set_credentials(move |action| match action { gix::credentials::helper::Action::Get(context) => { let same_origin = context @@ -1931,32 +2021,10 @@ impl GitRepository { }); let prepared = connection .prepare_fetch(gix::progress::Discard, Default::default()) - .map_err(invalid)?; + .map_err(map_https_prepare_error)?; let outcome = prepared .receive(gix::progress::Discard, control.cancelled.as_ref()) - .map_err(|error| { - if control.is_cancelled() { - return GitError::Cancelled; - } - let text = error.to_string(); - let lower = text.to_ascii_lowercase(); - if text.contains("401") - || text.contains("403") - || lower.contains("authentication") - || (lower.contains("credential") && lower.contains("not accepted")) - { - GitError::AuthenticationFailed - } else if lower.contains("certificate") || lower.contains("tls") { - GitError::TlsFailed - } else if lower.contains("network") - || lower.contains("connect") - || lower.contains("dns") - { - GitError::NetworkUnavailable - } else { - invalid(error) - } - })?; + .map_err(|error| map_https_fetch_error(error, control))?; Ok(matches!( outcome.status, gix::remote::fetch::Status::Change { .. } @@ -4331,8 +4399,12 @@ mod ssh_push_tests; #[cfg(test)] mod tests { + use std::{error::Error, fmt, io}; + use super::{ - GitError, GitIdentity, GitRemoteCredentialOverride, GitRepository, SshPassphraseProvider, + GitError, GitIdentity, GitOperationControl, GitRemoteCredentialOverride, GitRepository, + SshPassphraseProvider, classify_http_status, classify_structured_https_error, + generic_https_error, map_https_fetch_error, }; use crate::{ config::SshFingerprint, @@ -4341,6 +4413,17 @@ mod tests { struct Passphrases; + #[derive(Debug)] + struct Diagnostic(String); + + impl fmt::Display for Diagnostic { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str(&self.0) + } + } + + impl Error for Diagnostic {} + impl SshPassphraseProvider for Passphrases { fn ssh_key_passphrase( &self, @@ -4362,6 +4445,57 @@ mod tests { assert!(repository.repository.committer().is_some()); } + #[test] + fn https_failures_use_only_structured_status_and_sources() { + assert_eq!( + classify_http_status(reqwest::StatusCode::UNAUTHORIZED), + Some(GitError::AuthenticationFailed) + ); + assert_eq!( + classify_http_status(reqwest::StatusCode::FORBIDDEN), + Some(GitError::AuthenticationFailed) + ); + assert_eq!( + classify_structured_https_error(&io::Error::from(io::ErrorKind::ConnectionRefused)), + Some(GitError::NetworkUnavailable) + ); + assert_eq!( + classify_structured_https_error(&io::Error::from(io::ErrorKind::TimedOut)), + Some(GitError::NetworkUnavailable) + ); + assert_eq!( + classify_structured_https_error(&rustls::Error::General("opaque".to_owned())), + Some(GitError::TlsFailed) + ); + + let rendered_only = Diagnostic( + "401 403 authentication credential certificate tls network connect dns".to_owned(), + ); + assert_eq!(classify_structured_https_error(&rendered_only), None); + assert!(matches!( + generic_https_error(&rendered_only), + GitError::InvalidRepository(_) + )); + + let local_permission = gix::remote::fetch::Error::RemovePackKeepFile { + path: "pack.keep".into(), + source: io::Error::from(io::ErrorKind::PermissionDenied), + }; + assert!(matches!( + map_https_fetch_error(local_permission, &GitOperationControl::default()), + GitError::InvalidRepository(_) + )); + } + + #[test] + fn generic_https_diagnostic_is_bounded() { + let error = generic_https_error(&Diagnostic("x".repeat(32 * 1024))); + let GitError::InvalidRepository(diagnostic) = error else { + panic!("unstructured HTTPS error must stay generic"); + }; + assert!(diagnostic.len() <= 8 * 1024); + } + #[test] fn one_ssh_passphrase_override_is_bound_to_its_fingerprint() { let selected = SshFingerprint::parse("SHA256:47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU") diff --git a/crates/storage/src/mutation.rs b/crates/storage/src/mutation.rs index df8b388..11c6f72 100644 --- a/crates/storage/src/mutation.rs +++ b/crates/storage/src/mutation.rs @@ -7,7 +7,8 @@ use crate::{ crypto::{KeyStore, SecretProvider}, recipient::{RecipientPolicyManager, SigningPolicy}, repository::{ - DirectoryPath, EncryptedEntry, EntryPath, Repository, RepositoryError, ResolvedObject, + DirectoryPath, EncryptedEntry, EntryPath, Repository, RepositoryError, RepositorySnapshot, + ResolvedObject, }, write::OverwriteDecision, }; @@ -164,19 +165,21 @@ impl<'a> TreeMutator<'a> { if !request.recursive { return Err(MutationError::RecursiveRequired); } - self.reject_unmanaged(record.path())?; + self.reject_unmanaged(&snapshot, record.path())?; ( - self.source_entries(record.path())?, - self.source_policies(record.path())?, - self.source_directories(record.path())?, + self.source_entries(&snapshot, record.path())?, + self.source_policies(&snapshot, record.path())?, + self.source_directories(&snapshot, record.path()), record.path().clone(), format!("{}/", record.path()), ) } }; if let Err(operation) = self.remove_sources(&entries, &policies, &directories) { - self.restore_sources(&entries, &policies, &directories)?; - return Err(operation); + return Err(with_rollback( + operation, + self.restore_sources(&entries, &policies, &directories), + )); } let change = TreeCommit { action: MutationAction::Remove, @@ -195,8 +198,10 @@ impl<'a> TreeMutator<'a> { message: format!("Remove {display} from store."), }; if let Err(error) = committer.commit(&change) { - self.restore_sources(&entries, &policies, &directories)?; - return Err(MutationError::Commit(error)); + return Err(with_rollback( + MutationError::Commit(error), + self.restore_sources(&entries, &policies, &directories), + )); } self.repository.cleanup_empty_directories(&cleanup)?; Ok(MutationOutcome { @@ -331,11 +336,11 @@ impl<'a> TreeMutator<'a> { path: destination.as_path().to_owned(), }); } - self.reject_unmanaged(record.path())?; + self.reject_unmanaged(&snapshot, record.path())?; ( - self.source_entries(record.path())?, - self.source_policies(record.path())?, - self.source_directories(record.path())?, + self.source_entries(&snapshot, record.path())?, + self.source_policies(&snapshot, record.path())?, + self.source_directories(&snapshot, record.path()), record.path().clone(), destination, None, @@ -386,10 +391,10 @@ impl<'a> TreeMutator<'a> { .starts_with(source_root.as_path()) }) { RecipientPolicyManager::new(self.repository, self.keys) - .resolve_for_entry(&source_entry.source, signing)? + .resolve_for_entry_in_snapshot(&snapshot, &source_entry.source, signing)? } else { RecipientPolicyManager::new(self.repository, self.keys) - .resolve_for_entry(&destination_path, signing)? + .resolve_for_entry_in_snapshot(&snapshot, &destination_path, signing)? }; let replacement = if self .keys @@ -484,12 +489,18 @@ impl<'a> TreeMutator<'a> { Ok(()) })(); if let Err(operation) = applied { - if moving { - self.restore_sources(&source_entries, &source_policies, &source_directories)?; - } - self.restore_destinations(&entries, &policies)?; - self.remove_created_directories(&created_directories)?; - return Err(operation); + return Err(with_rollback( + operation, + self.rollback_transfer( + moving, + &source_entries, + &source_policies, + &source_directories, + &entries, + &policies, + &created_directories, + ), + )); } let action = if moving { MutationAction::Move @@ -528,12 +539,18 @@ impl<'a> TreeMutator<'a> { message: format!("{verb} {source} to {destination}."), }; if let Err(error) = committer.commit(&change) { - if moving { - self.restore_sources(&source_entries, &source_policies, &source_directories)?; - } - self.restore_destinations(&entries, &policies)?; - self.remove_created_directories(&created_directories)?; - return Err(MutationError::Commit(error)); + return Err(with_rollback( + MutationError::Commit(error), + self.rollback_transfer( + moving, + &source_entries, + &source_policies, + &source_directories, + &entries, + &policies, + &created_directories, + ), + )); } if moving { self.repository.cleanup_empty_directories(&source_root)?; @@ -549,9 +566,12 @@ impl<'a> TreeMutator<'a> { }) } - fn source_entries(&self, root: &DirectoryPath) -> Result, MutationError> { - self.repository - .snapshot()? + fn source_entries( + &self, + snapshot: &RepositorySnapshot, + root: &DirectoryPath, + ) -> Result, MutationError> { + snapshot .entries() .filter(|entry| entry.path().as_path().starts_with(root.as_path())) .map(|entry| { @@ -563,9 +583,12 @@ impl<'a> TreeMutator<'a> { .collect() } - fn source_policies(&self, root: &DirectoryPath) -> Result, MutationError> { - self.repository - .snapshot()? + fn source_policies( + &self, + snapshot: &RepositorySnapshot, + root: &DirectoryPath, + ) -> Result, MutationError> { + snapshot .recipient_policies() .filter(|policy| policy.directory().as_path().starts_with(root.as_path())) .map(|policy| { @@ -583,19 +606,21 @@ impl<'a> TreeMutator<'a> { fn source_directories( &self, + snapshot: &RepositorySnapshot, root: &DirectoryPath, - ) -> Result, MutationError> { - Ok(self - .repository - .snapshot()? + ) -> Vec { + snapshot .directories() .filter(|directory| directory.path().as_path().starts_with(root.as_path())) .map(|directory| directory.path().clone()) - .collect()) + .collect() } - fn reject_unmanaged(&self, root: &DirectoryPath) -> Result<(), MutationError> { - let snapshot = self.repository.snapshot()?; + fn reject_unmanaged( + &self, + snapshot: &RepositorySnapshot, + root: &DirectoryPath, + ) -> Result<(), MutationError> { if let Some(file) = snapshot .auxiliary_files() .find(|file| file.path().starts_with(root.as_path())) @@ -648,39 +673,65 @@ impl<'a> TreeMutator<'a> { policies: &[PolicyState], directories: &[DirectoryPath], ) -> Result<(), MutationError> { + let mut failure = None; for directory in directories { - self.repository.ensure_directory(directory)?; + remember_rollback_failure( + &mut failure, + self.repository + .ensure_directory(directory) + .map(|_| ()) + .map_err(MutationError::from), + ); } for policy in policies { - self.repository.replace_policy_file( - &policy.directory, - false, - Some(&policy.recipients), - )?; - if let Some(signature) = &policy.signature { + remember_rollback_failure( + &mut failure, self.repository - .replace_policy_file(&policy.directory, true, Some(signature))?; + .replace_policy_file(&policy.directory, false, Some(&policy.recipients)) + .map_err(MutationError::from), + ); + if let Some(signature) = &policy.signature { + remember_rollback_failure( + &mut failure, + self.repository + .replace_policy_file(&policy.directory, true, Some(signature)) + .map_err(MutationError::from), + ); } } for entry in entries { - self.repository - .write_entry(&entry.source, &entry.ciphertext)?; + remember_rollback_failure( + &mut failure, + self.repository + .write_entry(&entry.source, &entry.ciphertext) + .map_err(MutationError::from), + ); } - Ok(()) + failure.map_or(Ok(()), Err) } fn remove_created_directories( &self, directories: &[DirectoryPath], ) -> Result<(), MutationError> { + let mut failure = None; for directory in directories.iter().rev() { - if !self.repository.remove_empty_directory(directory)? { - return Err(MutationError::TreeChanged { - path: directory.as_path().to_owned(), + let result = self + .repository + .remove_empty_directory(directory) + .map_err(MutationError::from) + .and_then(|removed| { + if removed { + Ok(()) + } else { + Err(MutationError::TreeChanged { + path: directory.as_path().to_owned(), + }) + } }); - } + remember_rollback_failure(&mut failure, result); } - Ok(()) + failure.map_or(Ok(()), Err) } fn restore_destinations( @@ -688,25 +739,63 @@ impl<'a> TreeMutator<'a> { entries: &[TransferEntry], policies: &[TransferPolicy], ) -> Result<(), MutationError> { + let mut failure = None; for policy in policies.iter().rev() { if policy.source.signature.is_some() { - self.repository - .replace_policy_file(&policy.destination, true, None)?; + remember_rollback_failure( + &mut failure, + self.repository + .replace_policy_file(&policy.destination, true, None) + .map_err(MutationError::from), + ); } - self.repository - .replace_policy_file(&policy.destination, false, None)?; + remember_rollback_failure( + &mut failure, + self.repository + .replace_policy_file(&policy.destination, false, None) + .map_err(MutationError::from), + ); } for entry in entries.iter().rev() { - if let Some(original) = &entry.old_destination { - self.repository.write_entry(&entry.destination, original)?; + let result = if let Some(original) = &entry.old_destination { + self.repository + .write_entry(&entry.destination, original) + .map_err(MutationError::from) } else { match self.repository.remove_entry(&entry.destination) { - Ok(_) | Err(RepositoryError::NotFound { .. }) => {} - Err(error) => return Err(error.into()), + Ok(_) | Err(RepositoryError::NotFound { .. }) => Ok(()), + Err(error) => Err(error.into()), } - } + }; + remember_rollback_failure(&mut failure, result); } - Ok(()) + failure.map_or(Ok(()), Err) + } + + #[allow(clippy::too_many_arguments)] + fn rollback_transfer( + &self, + moving: bool, + source_entries: &[EntryState], + source_policies: &[PolicyState], + source_directories: &[DirectoryPath], + entries: &[TransferEntry], + policies: &[TransferPolicy], + created_directories: &[DirectoryPath], + ) -> Result<(), MutationError> { + let mut failure = None; + if moving { + remember_rollback_failure( + &mut failure, + self.restore_sources(source_entries, source_policies, source_directories), + ); + } + remember_rollback_failure(&mut failure, self.restore_destinations(entries, policies)); + remember_rollback_failure( + &mut failure, + self.remove_created_directories(created_directories), + ); + failure.map_or(Ok(()), Err) } } @@ -741,6 +830,25 @@ struct TransferPolicy { destination: DirectoryPath, } +fn remember_rollback_failure( + failure: &mut Option, + result: Result<(), MutationError>, +) { + if failure.is_none() { + *failure = result.err(); + } +} + +fn with_rollback(operation: MutationError, rollback: Result<(), MutationError>) -> MutationError { + match rollback { + Ok(()) => operation, + Err(rollback) => MutationError::RollbackFailed { + operation: Box::new(operation), + rollback: Box::new(rollback), + }, + } +} + #[derive(Debug)] pub enum MutationError { Repository(RepositoryError), @@ -751,13 +859,29 @@ pub enum MutationError { RootMutation, SameObject, DestinationInsideSource, - DestinationDirectoryMissing { directory: DirectoryPath }, - DestinationTypeCollision { path: std::path::PathBuf }, - UnsafePolicyOverwrite { directory: DirectoryPath }, - UnsupportedAuxiliary { path: std::path::PathBuf }, - UnsupportedGitRepository { path: std::path::PathBuf }, - TreeChanged { path: std::path::PathBuf }, + DestinationDirectoryMissing { + directory: DirectoryPath, + }, + DestinationTypeCollision { + path: std::path::PathBuf, + }, + UnsafePolicyOverwrite { + directory: DirectoryPath, + }, + UnsupportedAuxiliary { + path: std::path::PathBuf, + }, + UnsupportedGitRepository { + path: std::path::PathBuf, + }, + TreeChanged { + path: std::path::PathBuf, + }, Commit(TreeCommitError), + RollbackFailed { + operation: Box, + rollback: Box, + }, } impl fmt::Display for MutationError { @@ -804,6 +928,13 @@ impl fmt::Display for MutationError { path.display() ), Self::Commit(error) => write!(formatter, "cannot commit tree mutation: {error}"), + Self::RollbackFailed { + operation, + rollback, + } => write!( + formatter, + "tree mutation failed ({operation}) and rollback failed ({rollback})" + ), } } } @@ -829,3 +960,57 @@ impl From for MutationError { Self::Crypto(error) } } + +#[cfg(test)] +mod tests { + use std::fs; + + use super::*; + + #[test] + fn directory_mutation_uses_one_repository_snapshot() { + let temporary = tempfile::tempdir().expect("temporary store"); + fs::create_dir_all(temporary.path().join("folder/nested")).expect("tree"); + fs::write( + temporary.path().join("folder/nested/entry.gpg"), + b"ciphertext", + ) + .expect("entry"); + let repository = Repository::open(temporary.path()).expect("repository"); + let keys = KeyStore::new(); + let mut committer = NoGitTreeCommitter; + + TreeMutator::new(&repository, &keys) + .remove( + &RemoveRequest { + entry: "folder/".to_owned(), + recursive: true, + force: true, + }, + OverwriteDecision::Decline, + &mut committer, + ) + .expect("remove directory"); + + assert_eq!(repository.snapshot_count(), 1); + } + + #[test] + fn rollback_error_retains_both_failures() { + let operation = MutationError::Commit(TreeCommitError::new("operation")); + let rollback = MutationError::TreeChanged { + path: "rollback".into(), + }; + let combined = with_rollback(operation, Err(rollback)); + + let MutationError::RollbackFailed { + operation, + rollback, + } = combined + else { + panic!("both failures must be retained"); + }; + assert!(matches!(*operation, MutationError::Commit(_))); + assert!(matches!(*rollback, MutationError::TreeChanged { .. })); + } +} diff --git a/crates/storage/src/recipient.rs b/crates/storage/src/recipient.rs index 51c56a8..e758572 100644 --- a/crates/storage/src/recipient.rs +++ b/crates/storage/src/recipient.rs @@ -205,6 +205,15 @@ impl<'a> RecipientPolicyManager<'a> { signing: Option<&SigningPolicy>, ) -> Result { let snapshot = self.repository.snapshot()?; + self.resolve_for_entry_in_snapshot(&snapshot, entry, signing) + } + + pub(crate) fn resolve_for_entry_in_snapshot( + &self, + snapshot: &RepositorySnapshot, + entry: &EntryPath, + signing: Option<&SigningPolicy>, + ) -> Result { let policy = snapshot.recipient_policy(entry).ok_or_else(|| { RecipientPolicyError::MissingRecipientPolicy { entry: entry.clone(), diff --git a/crates/storage/src/repository.rs b/crates/storage/src/repository.rs index b12a569..2fadc45 100644 --- a/crates/storage/src/repository.rs +++ b/crates/storage/src/repository.rs @@ -11,6 +11,8 @@ use std::{ #[cfg(test)] use std::collections::BTreeSet; +#[cfg(test)] +use std::sync::atomic::{AtomicUsize, Ordering}; pub use crate::secret::SecretBytes; use cap_std::{ambient_authority, fs::Dir}; @@ -316,6 +318,8 @@ impl RepositorySnapshot { pub struct Repository { root_path: PathBuf, root: Dir, + #[cfg(test)] + snapshot_count: AtomicUsize, } impl fmt::Debug for Repository { @@ -346,7 +350,12 @@ impl Repository { .map_err(|error| io_error("canonicalize repository root", requested, error))?; let root = Dir::open_ambient_dir(&root_path, ambient_authority()) .map_err(|error| io_error("open repository root", &root_path, error))?; - Ok(Self { root_path, root }) + Ok(Self { + root_path, + root, + #[cfg(test)] + snapshot_count: AtomicUsize::new(0), + }) } pub fn root_path(&self) -> &Path { @@ -354,11 +363,18 @@ impl Repository { } pub fn snapshot(&self) -> Result { + #[cfg(test)] + self.snapshot_count.fetch_add(1, Ordering::Relaxed); let mut snapshot = RepositorySnapshot::default(); scan_directory(&self.root, &DirectoryPath::root(), &mut snapshot)?; Ok(snapshot) } + #[cfg(test)] + pub(crate) fn snapshot_count(&self) -> usize { + self.snapshot_count.load(Ordering::Relaxed) + } + pub fn read_entry(&self, path: &EntryPath) -> Result { let (parent, file_name) = self.open_entry_parent(path)?; let metadata = child_metadata(&parent, &file_name, &path.encrypted_relative_path())? @@ -757,27 +773,41 @@ impl Repository { } fn rollback_created( + &self, + created: Vec, + original: RepositoryError, + ) -> Result { + self.rollback_created_with_checkpoint(created, original, |_| Ok(())) + } + + fn rollback_created_with_checkpoint( &self, mut created: Vec, original: RepositoryError, + mut checkpoint: impl FnMut(RollbackStage) -> Result<(), RepositoryError>, ) -> Result { while let Some(path) = created.pop() { let parent = path.parent().unwrap_or_else(|| Path::new("")); - let parent_dir = self.open_directory(parent)?; - let name = path - .file_name() - .expect("created directory path has a file name"); - match parent_dir.remove_dir(name) { - Ok(()) => { - sync_directory(&parent_dir, parent)?; - } - Err(error) if error.kind() == io::ErrorKind::NotFound => {} - Err(error) => { - return Err(RepositoryError::RollbackFailed { - path, - source: error.kind(), - }); + let rollback = (|| { + checkpoint(RollbackStage::BeforeReopen)?; + let parent_dir = self.open_directory(parent)?; + let name = path + .file_name() + .expect("created directory path has a file name"); + match parent_dir.remove_dir(name) { + Ok(()) => { + checkpoint(RollbackStage::BeforeSync)?; + sync_directory(&parent_dir, parent) + } + Err(error) if error.kind() == io::ErrorKind::NotFound => Ok(()), + Err(error) => Err(io_error("remove rollback directory", &path, error)), } + })(); + if let Err(rollback) = rollback { + return Err(RepositoryError::RollbackFailed { + operation: Box::new(original), + rollback: Box::new(rollback), + }); } } Err(original) @@ -816,6 +846,12 @@ enum WriteStage { AfterRename, } +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum RollbackStage { + BeforeReopen, + BeforeSync, +} + #[cfg(test)] impl WriteStage { fn fixture_id(self) -> &'static str { @@ -856,8 +892,8 @@ pub enum RepositoryError { source: io::ErrorKind, }, RollbackFailed { - path: PathBuf, - source: io::ErrorKind, + operation: Box, + rollback: Box, }, DurabilityUncertain { path: PathBuf, @@ -919,10 +955,12 @@ impl fmt::Display for RepositoryError { path, source, } => write!(formatter, "cannot {operation} {}: {source}", path.display()), - Self::RollbackFailed { path, source } => write!( + Self::RollbackFailed { + operation, + rollback, + } => write!( formatter, - "failed to roll back empty password-store directory {}: {source}", - path.display() + "repository operation failed ({operation}) and directory rollback failed ({rollback})" ), Self::DurabilityUncertain { path } => write!( formatter, @@ -1330,4 +1368,47 @@ mod tests { assert!(!temporary.path().join("vault/one").exists()); Ok(()) } + + #[test] + fn rollback_retains_operation_and_directory_failure() -> Result<(), Box> { + for failed_stage in [RollbackStage::BeforeReopen, RollbackStage::BeforeSync] { + let temporary = tempfile::tempdir()?; + let vault = temporary.path().join("vault"); + fs::create_dir_all(vault.join("created"))?; + let repository = Repository::open(&vault)?; + let operation = RepositoryError::Io { + operation: "simulated operation", + path: PathBuf::from("entry"), + source: io::ErrorKind::Interrupted, + }; + let rollback = RepositoryError::Io { + operation: "simulated rollback", + path: PathBuf::from("created"), + source: io::ErrorKind::Other, + }; + + let error = repository + .rollback_created_with_checkpoint::<()>( + vec![PathBuf::from("created")], + operation.clone(), + |stage| { + if stage == failed_stage { + Err(rollback.clone()) + } else { + Ok(()) + } + }, + ) + .expect_err("rollback checkpoint must fail"); + + assert_eq!( + error, + RepositoryError::RollbackFailed { + operation: Box::new(operation), + rollback: Box::new(rollback), + } + ); + } + Ok(()) + } } diff --git a/crates/storage/tests/tree_mutations.rs b/crates/storage/tests/tree_mutations.rs index 6a79700..d6b3f8e 100644 --- a/crates/storage/tests/tree_mutations.rs +++ b/crates/storage/tests/tree_mutations.rs @@ -60,6 +60,15 @@ impl TreeCommitter for Committer { } } +struct SabotagingCommitter(Box); + +impl TreeCommitter for SabotagingCommitter { + fn commit(&mut self, _change: &TreeCommit) -> Result<(), TreeCommitError> { + (self.0)(); + Err(TreeCommitError::new("simulated operation failure")) + } +} + #[test] fn remove_entry_supports_confirmation_force_and_empty_cleanup() -> TestResult { let fixture = FixtureSet::load()?; @@ -514,6 +523,97 @@ fn collisions_auxiliary_files_and_commit_failure_never_leave_partial_trees() -> Ok(()) } +#[test] +fn rollback_failures_from_sources_destinations_and_cleanup_retain_the_operation() -> TestResult { + let fixture = FixtureSet::load()?; + + let store = fixture.materialize_store("basic")?; + let repository = Repository::open(store.path())?; + let keys = KeyStore::load(fixture.path("keys"))?; + let mut provider = Secrets::all(&fixture); + let source_collision = store.path().join("email/personal.gpg"); + let mut committer = SabotagingCommitter(Box::new(move || { + fs::create_dir(&source_collision).expect("block source restoration"); + })); + let error = TreeMutator::new(&repository, &keys) + .move_tree( + &MoveRequest { + source: "email/personal".into(), + destination: "archive/personal".into(), + force: false, + }, + OverwriteDecision::Allow, + None, + &mut provider, + &mut committer, + ) + .expect_err("source restoration must fail"); + assert_operation_and_rollback(error); + + let store = fixture.materialize_store("basic")?; + let repository = Repository::open(store.path())?; + let keys = KeyStore::load(fixture.path("keys"))?; + let mut provider = Secrets::all(&fixture); + let destination = store.path().join("archive/personal.gpg"); + let mut committer = SabotagingCommitter(Box::new(move || { + fs::remove_file(&destination).expect("remove destination entry"); + fs::create_dir(&destination).expect("block destination restoration"); + })); + let error = TreeMutator::new(&repository, &keys) + .copy( + &CopyRequest { + source: "email/personal".into(), + destination: "archive/personal".into(), + force: false, + }, + OverwriteDecision::Allow, + None, + &mut provider, + &mut committer, + ) + .expect_err("destination restoration must fail"); + assert_operation_and_rollback(error); + + let store = fixture.materialize_store("basic")?; + let repository = Repository::open(store.path())?; + let keys = KeyStore::load(fixture.path("keys"))?; + let mut provider = Secrets::all(&fixture); + let blocker = store.path().join("archive/blocker"); + let mut committer = SabotagingCommitter(Box::new(move || { + fs::write(&blocker, b"concurrent file").expect("block directory cleanup"); + })); + let error = TreeMutator::new(&repository, &keys) + .copy( + &CopyRequest { + source: "email/personal".into(), + destination: "archive/personal".into(), + force: false, + }, + OverwriteDecision::Allow, + None, + &mut provider, + &mut committer, + ) + .expect_err("created-directory cleanup must fail"); + assert_operation_and_rollback(error); + Ok(()) +} + +fn assert_operation_and_rollback(error: MutationError) { + let MutationError::RollbackFailed { + operation, + rollback, + } = error + else { + panic!("operation and rollback errors must both be retained"); + }; + assert!(matches!(*operation, MutationError::Commit(_))); + assert!(matches!( + *rollback, + MutationError::Repository(_) | MutationError::TreeChanged { .. } + )); +} + fn remove(entry: &str, recursive: bool, force: bool) -> RemoveRequest { RemoveRequest { entry: entry.into(),