From b507bf0af59dc10962d8420ba91edc7b6f08c431 Mon Sep 17 00:00:00 2001 From: zhom <2717306+zhom@users.noreply.github.com> Date: Tue, 7 Jul 2026 22:44:43 +0400 Subject: [PATCH] fix: sync engine correctness and manifest traversal guard --- src-tauri/src/sync/engine.rs | 201 +++++++++++++++++++++++++++++++++-- 1 file changed, 192 insertions(+), 9 deletions(-) diff --git a/src-tauri/src/sync/engine.rs b/src-tauri/src/sync/engine.rs index 899d9c3..0b9df3f 100644 --- a/src-tauri/src/sync/engine.rs +++ b/src-tauri/src/sync/engine.rs @@ -91,6 +91,24 @@ fn is_critical_file(path: &str) -> bool { .any(|pattern| path.contains(pattern)) } +/// Validate that a manifest-supplied relative file path is safe to join onto a +/// profile directory before writing/deleting. The manifest is remote-controlled +/// (a self-hosted or compromised sync server, a MITM on a plaintext Regular-mode +/// manifest, or a malicious team member who shares the E2E key), so an +/// unvalidated `path` is an arbitrary file write/delete primitive: +/// `profile_dir.join(path)` escapes the profile dir when `path` is absolute +/// (`join` replaces the base) or contains `..`. Accept only plain relative paths +/// built from normal components; reject absolute paths, `..`, and root/prefix. +fn is_safe_manifest_path(path: &str) -> bool { + use std::path::{Component, Path}; + let p = Path::new(path); + if path.is_empty() || p.is_absolute() { + return false; + } + p.components() + .all(|c| matches!(c, Component::Normal(_) | Component::CurDir)) +} + /// Checkpoint all SQLite WAL files in a profile directory. /// /// When a browser crashes or is killed, SQLite WAL files may contain @@ -656,6 +674,15 @@ impl SyncEngine { // Delete local files that don't exist remotely (when remote is newer) for path in &diff.files_to_delete_local { + // The delete list comes from the remote-controlled manifest; guard against + // traversal/absolute paths so it can't delete files outside the profile. + if !is_safe_manifest_path(path) { + log::warn!( + "Skipping local delete with unsafe relative path: {:?}", + path + ); + continue; + } let file_path = profile_dir.join(path); if file_path.exists() { let _ = fs::remove_file(&file_path); @@ -675,10 +702,16 @@ impl SyncEngine { .upload_profile_metadata(&profile_id, profile, &key_prefix) .await?; - // If we recovered from an empty local state (downloaded everything from remote), - // regenerate the manifest from the actual files now on disk so we don't - // overwrite the remote manifest with an empty one. - let final_manifest = if local_manifest.files.is_empty() && !diff.files_to_download.is_empty() { + // If this sync changed the local profile directory (downloaded files and/or + // deleted local files), the manifest generated at the START of the sync is + // now stale. Uploading it would advertise wrong hashes/mtimes for the files + // we just pulled, so the peer keeps computing a non-empty diff and the two + // devices ping-pong re-uploads forever (issue #470). Regenerate from the + // actual on-disk files before uploading. When only uploads happened, the + // on-disk state is unchanged and the original manifest is still accurate. + let local_changed = + !diff.files_to_download.is_empty() || !diff.files_to_delete_local.is_empty(); + let final_manifest = if local_changed { let mut new_cache = HashCache::load(&cache_path); let mut regenerated = generate_manifest(&profile_id, &profile_dir, &mut new_cache)?; new_cache.save(&cache_path)?; @@ -725,6 +758,8 @@ impl SyncEngine { updated_profile.proxy_id = remote_meta.proxy_id; updated_profile.vpn_id = remote_meta.vpn_id; updated_profile.group_id = remote_meta.group_id; + updated_profile.extension_group_id = remote_meta.extension_group_id; + updated_profile.window_color = remote_meta.window_color; updated_profile.last_sync = Some( std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) @@ -872,6 +907,8 @@ impl SyncEngine { updated.proxy_id = remote_meta.proxy_id; updated.vpn_id = remote_meta.vpn_id; updated.group_id = remote_meta.group_id; + updated.extension_group_id = remote_meta.extension_group_id; + updated.window_color = remote_meta.window_color; updated.last_sync = Some( std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) @@ -1059,6 +1096,15 @@ impl SyncEngine { ); break; } + // Reject paths that would escape the profile dir (path traversal / + // absolute path). On download the manifest is remote-controlled, so this + // is the load-bearing containment check; on upload it is defense-in-depth. + // Legitimate profile files are always plain relative paths, so a real file + // is never skipped. + if !is_safe_manifest_path(&file.path) { + log::warn!("Skipping file with unsafe relative path: {:?}", file.path); + continue; + } let sem = semaphore.clone(); let file_path = profile_dir.join(&file.path); let relative_path = file.path.clone(); @@ -1336,6 +1382,15 @@ impl SyncEngine { ); break; } + // Reject paths that would escape the profile dir (path traversal / + // absolute path). On download the manifest is remote-controlled, so this + // is the load-bearing containment check; on upload it is defense-in-depth. + // Legitimate profile files are always plain relative paths, so a real file + // is never skipped. + if !is_safe_manifest_path(&file.path) { + log::warn!("Skipping file with unsafe relative path: {:?}", file.path); + continue; + } let sem = semaphore.clone(); let file_path = profile_dir.join(&file.path); let relative_path = file.path.clone(); @@ -2341,7 +2396,16 @@ impl SyncEngine { } let metadata_presign = self.client.presign_download(&metadata_key).await?; - let metadata_data = self.client.download_bytes(&metadata_presign.url).await?; + let metadata_raw = self.client.download_bytes(&metadata_presign.url).await?; + // Unseal E2E-encrypted metadata before parsing — otherwise an encrypted + // envelope fails to deserialize and the missing profile never downloads to + // this device (issue #466). Plaintext (legacy) metadata passes through. + let metadata_data = encryption::maybe_unseal_after_download(&metadata_raw).map_err(|e| { + if e.contains("ENCRYPTION_PASSWORD_REQUIRED") { + let _ = events::emit("profile-sync-e2e-password-required", ()); + } + SyncError::InvalidData(format!("Failed to unseal profile metadata: {e}")) + })?; let mut profile: BrowserProfile = serde_json::from_slice(&metadata_data) .map_err(|e| SyncError::SerializationError(format!("Failed to parse metadata: {e}")))?; @@ -2756,7 +2820,13 @@ impl SyncEngine { Ok(stat) if stat.exists => match self.client.presign_download(&metadata_key).await { Ok(presign) => match self.client.download_bytes(&presign.url).await { Ok(data) => { - if let Ok(mut remote_profile) = serde_json::from_slice::(&data) { + // Unseal E2E metadata before parsing; keep the tolerant skip so a + // profile without a loaded password (or bad data) is left as-is. + if let Ok(mut remote_profile) = encryption::maybe_unseal_after_download(&data) + .and_then(|d| { + serde_json::from_slice::(&d).map_err(|e| e.to_string()) + }) + { remote_profile.sync_mode = *sync_mode; remote_profile.last_sync = Some( std::time::SystemTime::now() @@ -2893,6 +2963,85 @@ impl SyncEngine { } } + // Check for remote extensions not present locally. The extensions/ prefix + // also holds binary files under extensions/{id}/file/..., so the `/` filter + // keeps only the top-level extensions/{id}.json entries. + let remote_extensions = self.client.list("extensions/").await?; + for obj in &remote_extensions.objects { + if let Some(ext_id) = obj + .key + .strip_prefix("extensions/") + .and_then(|s| s.strip_suffix(".json")) + .filter(|s| !s.contains('/')) + { + let exists_locally = { + let manager = crate::extension_manager::EXTENSION_MANAGER.lock().unwrap(); + manager + .list_extensions() + .unwrap_or_default() + .iter() + .any(|e| e.id == ext_id) + }; + if !exists_locally { + let tombstone_key = format!("tombstones/extensions/{}.json", ext_id); + if let Ok(stat) = self.client.stat(&tombstone_key).await { + if stat.exists { + continue; + } + } + log::info!( + "Extension {} exists remotely but not locally, downloading...", + ext_id + ); + if let Err(e) = self.download_extension(ext_id, Some(app_handle)).await { + log::warn!("Failed to download missing extension {}: {}", ext_id, e); + } + } + } + } + + // Check for remote extension groups not present locally + let remote_ext_groups = self.client.list("extension_groups/").await?; + for obj in &remote_ext_groups.objects { + if let Some(group_id) = obj + .key + .strip_prefix("extension_groups/") + .and_then(|s| s.strip_suffix(".json")) + .filter(|s| !s.contains('/')) + { + let exists_locally = { + let manager = crate::extension_manager::EXTENSION_MANAGER.lock().unwrap(); + manager + .list_groups() + .unwrap_or_default() + .iter() + .any(|g| g.id == group_id) + }; + if !exists_locally { + let tombstone_key = format!("tombstones/extension_groups/{}.json", group_id); + if let Ok(stat) = self.client.stat(&tombstone_key).await { + if stat.exists { + continue; + } + } + log::info!( + "Extension group {} exists remotely but not locally, downloading...", + group_id + ); + if let Err(e) = self + .download_extension_group(group_id, Some(app_handle)) + .await + { + log::warn!( + "Failed to download missing extension group {}: {}", + group_id, + e + ); + } + } + } + } + log::info!("Missing synced entities check complete"); Ok(()) } @@ -3039,18 +3188,18 @@ pub async fn enable_extension_group_sync_if_needed(extension_group_id: &str) -> // Cascade to every extension referenced by the group so the other device // has the actual extension binaries when it pulls the group. - for ext_id in extension_ids { + for ext_id in &extension_ids { let already_synced = { let manager = crate::extension_manager::EXTENSION_MANAGER.lock().unwrap(); manager - .get_extension(&ext_id) + .get_extension(ext_id) .ok() .map(|e| e.sync_enabled) .unwrap_or(true) }; if !already_synced { let manager = crate::extension_manager::EXTENSION_MANAGER.lock().unwrap(); - if let Ok(mut ext) = manager.get_extension(&ext_id) { + if let Ok(mut ext) = manager.get_extension(ext_id) { ext.sync_enabled = true; if let Err(e) = manager.update_extension_internal(&ext) { log::warn!("Failed to auto-enable sync for extension {}: {e}", ext_id); @@ -3061,6 +3210,18 @@ pub async fn enable_extension_group_sync_if_needed(extension_group_id: &str) -> } } + // Enabling sync only flips a local flag; without queueing a sync run the + // group and its extensions are never uploaded, so the other device never + // receives them (issue #477). Queue them now. + if let Some(scheduler) = crate::sync::get_global_scheduler() { + scheduler + .queue_extension_group_sync(extension_group_id.to_string()) + .await; + for ext_id in &extension_ids { + scheduler.queue_extension_sync(ext_id.clone()).await; + } + } + Ok(()) } @@ -4031,6 +4192,28 @@ pub async fn rollover_encryption_for_all_entities( mod tests { use super::*; + #[test] + fn test_is_safe_manifest_path() { + // Legitimate profile-relative paths are accepted. + assert!(is_safe_manifest_path("Local State")); + assert!(is_safe_manifest_path("Default/Cookies")); + assert!(is_safe_manifest_path("Default/Secure Preferences")); + assert!(is_safe_manifest_path( + "Default/Extensions/abc/1.0_0/manifest.json" + )); + assert!(is_safe_manifest_path("./Default/Preferences")); + + // Traversal / absolute / escaping paths are rejected. + assert!(!is_safe_manifest_path("")); + assert!(!is_safe_manifest_path("../../../.zshrc")); + assert!(!is_safe_manifest_path("Default/../../evil")); + assert!(!is_safe_manifest_path( + "/Users/victim/Library/LaunchAgents/evil.plist" + )); + #[cfg(windows)] + assert!(!is_safe_manifest_path(r"C:\Windows\System32\evil.dll")); + } + #[test] fn test_checkpoint_sqlite_wal_files() { let temp_dir = tempfile::TempDir::new().unwrap();