fix: harden sync safety — conflict backup, timestamp parsing, credential zeroization

Back up local files before overwriting during ConflictRemoteWins so data
is never silently lost. Fix false-positive change detection by parsing
timestamps before comparing (different formats like RFC3339 vs HTTP date
were never equal as strings). Add zeroize crate to zero WebDAV credentials
in memory on drop, preventing exposure in core dumps.
This commit is contained in:
Tristan Michael 2026-04-02 08:58:36 -07:00
parent fa1125bfeb
commit e0c7292a7e
4 changed files with 73 additions and 5 deletions

View file

@ -2394,6 +2394,7 @@ dependencies = [
"sha2", "sha2",
"tokio", "tokio",
"uuid", "uuid",
"zeroize",
] ]
[[package]] [[package]]

View file

@ -22,6 +22,7 @@ sha2 = { workspace = true }
quick-xml = { workspace = true } quick-xml = { workspace = true }
tokio = { workspace = true } tokio = { workspace = true }
keyring = { version = "3", features = ["apple-native", "windows-native", "sync-secret-service"], optional = true } keyring = { version = "3", features = ["apple-native", "windows-native", "sync-secret-service"], optional = true }
zeroize = "1"
[dev-dependencies] [dev-dependencies]
tempfile = "3.0" tempfile = "3.0"

View file

@ -129,7 +129,8 @@ pub fn compute_sync_actions(
// Both present, base known: check for changes // Both present, base known: check for changes
(Some(l), Some(r), Some(b)) => { (Some(l), Some(r), Some(b)) => {
let local_changed = l.checksum != b.checksum; let local_changed = l.checksum != b.checksum;
let remote_changed = r.size != b.size || r.last_modified.as_deref() != b.modified_at.as_deref(); // Compare remote vs base using parsed timestamps to avoid format mismatches
let remote_changed = r.size != b.size || !timestamps_equal(r.last_modified.as_deref(), b.modified_at.as_deref());
match (local_changed, remote_changed) { match (local_changed, remote_changed) {
(false, false) => {} // Skip, unchanged (false, false) => {} // Skip, unchanged
@ -173,7 +174,7 @@ pub fn compute_sync_actions(
// Remote present, local gone, base known: local was deleted // Remote present, local gone, base known: local was deleted
(None, Some(_), Some(b)) => { (None, Some(_), Some(b)) => {
let remote_changed = remote.is_some_and(|r| r.size != b.size || r.last_modified.as_deref() != b.modified_at.as_deref()); let remote_changed = remote.is_some_and(|r| r.size != b.size || !timestamps_equal(r.last_modified.as_deref(), b.modified_at.as_deref()));
if remote_changed { if remote_changed {
// deleted locally + modified remotely -> download (remote wins) // deleted locally + modified remotely -> download (remote wins)
actions.push(SyncAction::Download { path: path.to_string() }); actions.push(SyncAction::Download { path: path.to_string() });
@ -197,6 +198,23 @@ pub fn compute_sync_actions(
actions actions
} }
/// Compare two timestamps for equality by parsing both, tolerating format differences.
fn timestamps_equal(a: Option<&str>, b: Option<&str>) -> bool {
match (a, b) {
(None, None) => true,
(Some(a), Some(b)) => {
// Try string equality first (fast path)
if a == b { return true; }
// Parse both and compare as DateTime
match (parse_timestamp(a), parse_timestamp(b)) {
(Some(ta), Some(tb)) => ta == tb,
_ => false,
}
}
_ => false,
}
}
/// Determine if local wins based on timestamps. True means local wins. /// Determine if local wins based on timestamps. True means local wins.
fn local_wins(local_modified: Option<&str>, remote_modified: Option<&str>) -> bool { fn local_wins(local_modified: Option<&str>, remote_modified: Option<&str>) -> bool {
// Try parsing both; if we can't parse, local wins by default // Try parsing both; if we can't parse, local wins by default
@ -582,12 +600,11 @@ async fn execute_action(
report: &(dyn Fn(&str) + Send + Sync), report: &(dyn Fn(&str) + Send + Sync),
) -> Result<()> { ) -> Result<()> {
match action { match action {
SyncAction::Upload { path } | SyncAction::ConflictLocalWins { path } => { SyncAction::Upload { path } => {
let local_path = workspace_path.join(path.replace('/', std::path::MAIN_SEPARATOR_STR)); let local_path = workspace_path.join(path.replace('/', std::path::MAIN_SEPARATOR_STR));
let data = std::fs::read(&local_path)?; let data = std::fs::read(&local_path)?;
let checksum = compute_checksum(&data); let checksum = compute_checksum(&data);
// Ensure remote parent directory exists
if let Some(parent) = path_parent(path) { if let Some(parent) = path_parent(path) {
client.ensure_dir(parent).await?; client.ensure_dir(parent).await?;
} }
@ -602,7 +619,25 @@ async fn execute_action(
sync_state.record_file(path, &checksum, modified.as_deref(), data.len() as u64); sync_state.record_file(path, &checksum, modified.as_deref(), data.len() as u64);
} }
SyncAction::Download { path } | SyncAction::ConflictRemoteWins { path } => { SyncAction::ConflictLocalWins { path } => {
let local_path = workspace_path.join(path.replace('/', std::path::MAIN_SEPARATOR_STR));
let data = std::fs::read(&local_path)?;
let checksum = compute_checksum(&data);
if let Some(parent) = path_parent(path) {
client.ensure_dir(parent).await?;
}
report(&format!(" ^ Conflict: uploading local version of {}", path));
client.put_file(path, data.clone()).await?;
let modified = std::fs::metadata(&local_path).ok()
.and_then(|m| m.modified().ok())
.map(|t| { let dt: DateTime<Utc> = t.into(); dt.to_rfc3339() });
sync_state.record_file(path, &checksum, modified.as_deref(), data.len() as u64);
}
SyncAction::Download { path } => {
report(&format!(" v Downloading {}", path)); report(&format!(" v Downloading {}", path));
let data = client.get_file(path).await?; let data = client.get_file(path).await?;
let checksum = compute_checksum(&data); let checksum = compute_checksum(&data);
@ -620,6 +655,29 @@ async fn execute_action(
sync_state.record_file(path, &checksum, modified.as_deref(), data.len() as u64); sync_state.record_file(path, &checksum, modified.as_deref(), data.len() as u64);
} }
SyncAction::ConflictRemoteWins { path } => {
let local_path = workspace_path.join(path.replace('/', std::path::MAIN_SEPARATOR_STR));
// Back up local version before overwriting with remote
if local_path.exists() {
let backup_path = local_path.with_extension("conflict-backup");
let _ = std::fs::copy(&local_path, &backup_path);
report(&format!(" ! Backed up local version to {}", backup_path.display()));
}
report(&format!(" v Conflict: downloading remote version of {}", path));
let data = client.get_file(path).await?;
let checksum = compute_checksum(&data);
if let Some(parent) = local_path.parent() {
std::fs::create_dir_all(parent)?;
}
std::fs::write(&local_path, &data)?;
let modified = std::fs::metadata(&local_path).ok()
.and_then(|m| m.modified().ok())
.map(|t| { let dt: DateTime<Utc> = t.into(); dt.to_rfc3339() });
sync_state.record_file(path, &checksum, modified.as_deref(), data.len() as u64);
}
SyncAction::DeleteLocal { path } => { SyncAction::DeleteLocal { path } => {
report(&format!(" x Deleting local {}", path)); report(&format!(" x Deleting local {}", path));
let local_path = workspace_path.join(path.replace('/', std::path::MAIN_SEPARATOR_STR)); let local_path = workspace_path.join(path.replace('/', std::path::MAIN_SEPARATOR_STR));

View file

@ -1,4 +1,5 @@
use reqwest::Client; use reqwest::Client;
use zeroize::Zeroize;
use crate::error::{Error, Result}; use crate::error::{Error, Result};
/// Information about a file on the remote WebDAV server. /// Information about a file on the remote WebDAV server.
@ -18,6 +19,13 @@ pub struct WebDavClient {
_password: String, _password: String,
} }
impl Drop for WebDavClient {
fn drop(&mut self) {
self._password.zeroize();
self._username.zeroize();
}
}
impl WebDavClient { impl WebDavClient {
pub fn new(base_url: &str, username: &str, password: &str) -> Self { pub fn new(base_url: &str, username: &str, password: &str) -> Self {
let base_url = base_url.trim_end_matches('/').to_string(); let base_url = base_url.trim_end_matches('/').to_string();