From e0c7292a7ebbc46416dc26b8fdc69777c432c50c Mon Sep 17 00:00:00 2001 From: Tristan Michael Date: Thu, 2 Apr 2026 08:58:36 -0700 Subject: [PATCH] =?UTF-8?q?fix:=20harden=20sync=20safety=20=E2=80=94=20con?= =?UTF-8?q?flict=20backup,=20timestamp=20parsing,=20credential=20zeroizati?= =?UTF-8?q?on?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- apps/tauri/src-tauri/Cargo.lock | 1 + crates/onyx-core/Cargo.toml | 1 + crates/onyx-core/src/sync.rs | 68 ++++++++++++++++++++++++++++++--- crates/onyx-core/src/webdav.rs | 8 ++++ 4 files changed, 73 insertions(+), 5 deletions(-) diff --git a/apps/tauri/src-tauri/Cargo.lock b/apps/tauri/src-tauri/Cargo.lock index 0b0814b..364c90c 100644 --- a/apps/tauri/src-tauri/Cargo.lock +++ b/apps/tauri/src-tauri/Cargo.lock @@ -2394,6 +2394,7 @@ dependencies = [ "sha2", "tokio", "uuid", + "zeroize", ] [[package]] diff --git a/crates/onyx-core/Cargo.toml b/crates/onyx-core/Cargo.toml index 4949aec..3b5f2a8 100644 --- a/crates/onyx-core/Cargo.toml +++ b/crates/onyx-core/Cargo.toml @@ -22,6 +22,7 @@ sha2 = { workspace = true } quick-xml = { workspace = true } tokio = { workspace = true } keyring = { version = "3", features = ["apple-native", "windows-native", "sync-secret-service"], optional = true } +zeroize = "1" [dev-dependencies] tempfile = "3.0" diff --git a/crates/onyx-core/src/sync.rs b/crates/onyx-core/src/sync.rs index 167854c..debb46a 100644 --- a/crates/onyx-core/src/sync.rs +++ b/crates/onyx-core/src/sync.rs @@ -129,7 +129,8 @@ pub fn compute_sync_actions( // Both present, base known: check for changes (Some(l), Some(r), Some(b)) => { 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) { (false, false) => {} // Skip, unchanged @@ -173,7 +174,7 @@ pub fn compute_sync_actions( // Remote present, local gone, base known: local was deleted (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 { // deleted locally + modified remotely -> download (remote wins) actions.push(SyncAction::Download { path: path.to_string() }); @@ -197,6 +198,23 @@ pub fn compute_sync_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. fn local_wins(local_modified: Option<&str>, remote_modified: Option<&str>) -> bool { // 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), ) -> Result<()> { 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 data = std::fs::read(&local_path)?; let checksum = compute_checksum(&data); - // Ensure remote parent directory exists if let Some(parent) = path_parent(path) { 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); } - 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 = 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)); let data = client.get_file(path).await?; 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); } + 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 = t.into(); dt.to_rfc3339() }); + sync_state.record_file(path, &checksum, modified.as_deref(), data.len() as u64); + } + SyncAction::DeleteLocal { path } => { report(&format!(" x Deleting local {}", path)); let local_path = workspace_path.join(path.replace('/', std::path::MAIN_SEPARATOR_STR)); diff --git a/crates/onyx-core/src/webdav.rs b/crates/onyx-core/src/webdav.rs index 3a3d532..00df9d0 100644 --- a/crates/onyx-core/src/webdav.rs +++ b/crates/onyx-core/src/webdav.rs @@ -1,4 +1,5 @@ use reqwest::Client; +use zeroize::Zeroize; use crate::error::{Error, Result}; /// Information about a file on the remote WebDAV server. @@ -18,6 +19,13 @@ pub struct WebDavClient { _password: String, } +impl Drop for WebDavClient { + fn drop(&mut self) { + self._password.zeroize(); + self._username.zeroize(); + } +} + impl WebDavClient { pub fn new(base_url: &str, username: &str, password: &str) -> Self { let base_url = base_url.trim_end_matches('/').to_string();