hugo/IronStorage
1
0
Fork 0
Code Issues 28 Pull Requests Actions Packages Projects Releases Wiki Activity

Implement audited findings
Some checks failed
Dependency security audit / rustsec (push) Failing after 3s
Details

Browse Source
This commit is contained in:
Chili Palmer
2026-08-27 15:34:31 +02:00
parent 2feecae70d
commit c2dd826920
11 changed files with 773 additions and 220 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
Cargo.lock generated
View File

@@ -5415,6 +5415,7 @@ dependencies = [
"reqwest 0.13.4",
"rqrr",
"russh",
"rustls",
"secret-service",
"security-framework 3.7.0",
"serde",

1
Cargo.toml
View File

@@ -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"

91
FINDINGS.md
View File

@@ -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.

54
apps/tui/src/runtime.rs
View File

@@ -210,6 +210,7 @@ impl AsyncExecutor {
where
F: FnOnce() -> Result<AsyncPayload, String> + 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<Item = AsyncResult> + '_ {
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);
}
}

2
crates/storage/Cargo.toml
View File

@@ -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 }

85
crates/storage/src/config.rs
View File

@@ -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<toml::Table, ConfigError> {
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<PathBuf> {
#[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");
}
}

200
crates/storage/src/git.rs
View File

@@ -1022,8 +1022,8 @@ impl ReqwestGitTransport {
fn response(response: reqwest::blocking::Response) -> Result<Vec<u8>, 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<GitError> {
matches!(
status,
reqwest::StatusCode::UNAUTHORIZED | reqwest::StatusCode::FORBIDDEN
)
.then_some(GitError::AuthenticationFailed)
}
fn classify_structured_https_error(error: &(dyn Error + 'static)) -> Option<GitError> {
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::<reqwest::Error>() {
if error.status().and_then(classify_http_status).is_some() {
return Some(GitError::AuthenticationFailed);
}
network |= error.is_timeout() || error.is_connect();
}
if error.is::<rustls::Error>() {
tls = true;
}
if let Some(error) = error.downcast_ref::<std::io::Error>() {
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")

329
crates/storage/src/mutation.rs
View File

@@ -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<Vec<EntryState>, MutationError> {
self.repository
.snapshot()?
fn source_entries(
&self,
snapshot: &RepositorySnapshot,
root: &DirectoryPath,
) -> Result<Vec<EntryState>, 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<Vec<PolicyState>, MutationError> {
self.repository
.snapshot()?
fn source_policies(
&self,
snapshot: &RepositorySnapshot,
root: &DirectoryPath,
) -> Result<Vec<PolicyState>, 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<Vec<DirectoryPath>, MutationError> {
Ok(self
.repository
.snapshot()?
) -> Vec<DirectoryPath> {
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<MutationError>,
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<MutationError>,
rollback: Box<MutationError>,
},
}
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<crate::crypto::CryptoError> 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 { .. }));
}
}

9
crates/storage/src/recipient.rs
View File

@@ -205,6 +205,15 @@ impl<'a> RecipientPolicyManager<'a> {
signing: Option<&SigningPolicy>,
) -> Result<EffectiveRecipients, RecipientPolicyError> {
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<EffectiveRecipients, RecipientPolicyError> {
let policy = snapshot.recipient_policy(entry).ok_or_else(|| {
RecipientPolicyError::MissingRecipientPolicy {
entry: entry.clone(),

121
crates/storage/src/repository.rs
View File

@@ -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<RepositorySnapshot, RepositoryError> {
#[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<EncryptedEntry, RepositoryError> {
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<T>(
&self,
created: Vec<PathBuf>,
original: RepositoryError,
) -> Result<T, RepositoryError> {
self.rollback_created_with_checkpoint(created, original, |_| Ok(()))
}
fn rollback_created_with_checkpoint<T>(
&self,
mut created: Vec<PathBuf>,
original: RepositoryError,
mut checkpoint: impl FnMut(RollbackStage) -> Result<(), RepositoryError>,
) -> Result<T, RepositoryError> {
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<RepositoryError>,
rollback: Box<RepositoryError>,
},
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<dyn Error>> {
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(())
}
}

100
crates/storage/tests/tree_mutations.rs
View File

@@ -60,6 +60,15 @@ impl TreeCommitter for Committer {
}
}
struct SabotagingCommitter(Box<dyn FnMut()>);
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(),