diff --git a/crates/pdfs-fuse/src/lib.rs b/crates/pdfs-fuse/src/lib.rs index 30d5aa3..a6935d5 100644 --- a/crates/pdfs-fuse/src/lib.rs +++ b/crates/pdfs-fuse/src/lib.rs @@ -6743,7 +6743,27 @@ fn fuse_name(name: &OsStr) -> Result { name.to_str().map(str::to_owned).ok_or(Errno::EILSEQ) } +/// Whether a decrypted remote name can stand as one local path component. +/// +/// Drive checks names only when a client creates or renames a node, and only +/// for length, so a name made by another client, or by a server that should +/// not be trusted, can hold anything. Joined onto a local path, a `/` or a `..` +/// in it walks out of the folder it belongs to, and the mirror engine would +/// then write or delete outside the sync root. In the mount, the kernel fails +/// the whole listing with EIO on a single such name (`docs/BUGS.md` B215). +pub(crate) fn remote_name_is_safe(name: &str) -> bool { + !name.is_empty() + && name != "." + && name != ".." + && name.len() <= 255 + && !name.contains(['/', '\0']) +} + fn node_visible(node: &Node, folder_uid: &NodeUid, hidden: &HashSet) -> bool { + if !remote_name_is_safe(&node.name) { + warn!(uid = %node.uid, "a remote name cannot be a local file name; leaving it out"); + return false; + } !node.trashed && node.uid != *folder_uid && !hidden.contains(&node.uid) } @@ -8654,6 +8674,28 @@ mod tests { )); } + #[test] + fn a_remote_name_that_is_not_one_path_component_is_left_out() { + let folder_uid = NodeUid::new(VolumeId::from("vol"), LinkId::from("folder")); + let none = std::collections::HashSet::new(); + for name in [ + "a/b", + "../../.bashrc", + "..", + ".", + "", + "a\0b", + &"x".repeat(256), + ] { + let node = node_helper("child", "folder", name, false); + assert!(!node_visible(&node, &folder_uid, &none), "{name:?}"); + } + for name in [".hidden", "a..b", "unicodé-文件", &"x".repeat(255)] { + let node = node_helper("child", "folder", name, false); + assert!(node_visible(&node, &folder_uid, &none), "{name:?}"); + } + } + #[test] fn denied_queue_guard_preserves_accepted_bytes_and_pending_count() { use std::cell::Cell; diff --git a/crates/pdfs-fuse/src/sync.rs b/crates/pdfs-fuse/src/sync.rs index abe74ba..8a2246b 100644 --- a/crates/pdfs-fuse/src/sync.rs +++ b/crates/pdfs-fuse/src/sync.rs @@ -59,9 +59,10 @@ pub(crate) use planner::base_name; #[cfg(test)] use planner::conflict_path; use planner::{ - FilePlan, Unscanned, classification_order, conflict_path_with_suffix, filter_baseline, - guard_local_wipe, join_rel, local_deletions, parent_rel, plan_file, rel_to_path, remote_sig, - tracked_baseline, unchanged_remote_size, + FilePlan, RemoteGaps, Unscanned, classification_order, conflict_path_with_suffix, + filter_baseline, guard_local_wipe, guard_remote_wipe, join_rel, local_deletions, parent_rel, + plan_file, rel_to_path, remote_sig, remote_subtree_changed, tracked_baseline, + unchanged_remote_size, }; /// What a [`Pending`] op did, so the reconcile can update shared state on the /// engine thread (never inside a task). @@ -638,23 +639,32 @@ impl Core { let mut remote: HashMap = HashMap::new(); let mut remote_dirs: HashMap = HashMap::new(); + let mut gaps = RemoteGaps::default(); remote_dirs.insert(String::new(), remote_root.clone()); self.walk_remote( folder_id, remote_root, "", - &mut remote, - &mut remote_dirs, + &mut RemoteWalk { + out: &mut remote, + dirs: &mut remote_dirs, + gaps: &mut gaps, + }, &baseline, )?; + guard_remote_wipe(&tracked_baseline(&baseline, rules, &unscanned), &remote)?; let mut order = classification_order(&local, &remote, &baseline, rules); // A path the scan could not read is no more a local deletion than an // ignored one, and a remote path under it has nowhere to be written. - order.retain(|rel| !unscanned.covers(rel)); + // The same holds for a path the remote walk could not read: missing + // from it, it would read as deleted on Drive (B212). + order.retain(|rel| !unscanned.covers(rel) && !gaps.hides(rel, &remote, &remote_dirs)); + let mut unscanned_report = unscanned.report(); + unscanned_report.extend(gaps.report()); let mut outcome = Outcome { - unscanned: unscanned.report(), + unscanned: unscanned_report, ..Default::default() }; // Folders to delete, collected here and removed deepest-first at the end @@ -814,9 +824,17 @@ impl Core { } }, FilePlan::DeleteLocal => { - if let Err(e) = - removed_locally(std::fs::remove_file(local_root.join(rel_to_path(rel)))) - { + let path = local_root.join(rel_to_path(rel)); + // The plan comes from a scan taken before the remote walk, + // which can take minutes. An edit saved since then is not + // something Drive deleted: keep it, and the next pass + // uploads it as a new file (B213). + if l.is_some_and(|planned| changed_since_scan(&path, planned)) { + info!(rel, "sync: changed since the scan; not deleting it"); + outcome.deferred += 1; + continue; + } + if let Err(e) = removed_locally(std::fs::remove_file(&path)) { warn!(rel, error = %e, "sync: remove local file failed"); self.log_activity( ActivityKind::Trash, @@ -874,28 +892,70 @@ impl Core { &mut outcome, )?; - // Deferred folder deletions, deepest first. - delete_local_dirs.sort_by_key(|p| std::cmp::Reverse(p.matches('/').count())); - for rel in delete_local_dirs { - if let Err(e) = - removed_locally(std::fs::remove_dir_all(local_root.join(rel_to_path(&rel)))) - { - warn!(rel, error = %e, "sync: remove local folder failed"); - self.log_activity(ActivityKind::Trash, base_name(&rel), e.to_string(), false); - outcome.errors += 1; - continue; + // Deferred folder deletions, deepest first. A folder that holds what + // Drive never had stays, and its baseline row goes, so the next pass + // creates it on Drive again and uploads what it holds (B211). + if !delete_local_dirs.is_empty() { + let mut current = self + .db + .sync_entries(folder_id) + .map_err(|e| format!("reload baseline: {e:?}"))?; + for removal in remove_local_dirs(local_root, delete_local_dirs, &mut current) { + match removal { + DirRemoval::Removed(rel) => { + let _ = self.db.sync_entry_remove(folder_id, &rel); + self.log_activity( + ActivityKind::Trash, + base_name(&rel), + "removed locally", + true, + ); + outcome.deleted += 1; + } + DirRemoval::Kept(rel, reason) => { + warn!( + rel, + reason, "sync: deleted on Drive, but holds unsynced data; keeping it" + ); + let _ = self.db.sync_entry_remove(folder_id, &rel); + self.log_activity( + ActivityKind::Trash, + base_name(&rel), + format!("deleted on Proton Drive, but kept here: {reason}"), + false, + ); + } + DirRemoval::Failed(rel, e) => { + warn!(rel, error = %e, "sync: remove local folder failed"); + self.log_activity( + ActivityKind::Trash, + base_name(&rel), + e.to_string(), + false, + ); + outcome.errors += 1; + } + } } - let _ = self.db.sync_entry_remove(folder_id, &rel); - self.log_activity( - ActivityKind::Trash, - base_name(&rel), - "removed locally", - true, - ); - outcome.deleted += 1; } delete_remote_dirs.sort_by_key(|(p, _)| std::cmp::Reverse(p.matches('/').count())); for (rel, uid) in delete_remote_dirs { + // Deleted here, but another device has put something new in it on + // Drive. This pass downloaded that, so the folder is back here + // with it; trashing the folder now would take it away again (B214). + if let Some(reason) = remote_subtree_changed(&rel, &remote, &baseline) { + warn!( + rel, + reason, "sync: deleted here, but changed on Drive; not trashing it" + ); + self.log_activity( + ActivityKind::Trash, + base_name(&rel), + format!("deleted here, but kept on Proton Drive: {reason}"), + false, + ); + continue; + } if let Err(e) = self .rt .block_on(self.drive.trash_nodes(&[uid])) @@ -963,7 +1023,7 @@ impl Core { .block_on(self.drive.enumerate_nodes_light(&uids)) .map_err(|e| format!("resolve nodes: {e}"))?; for node in nodes { - if node.trashed || !node.is_folder() { + if node.trashed || !node.is_folder() || !crate::remote_name_is_safe(&node.name) { continue; } let rel = join_rel(prefix, &node.name); @@ -985,13 +1045,15 @@ impl Core { /// unchanged mtime means the recorded size still stands. /// /// [`enumerate_nodes_light`]: proton_drive_rs::ProtonDriveClient::enumerate_nodes_light + /// + /// What the walk could not read goes to `walk.gaps`, never silently out of + /// the map: a missing path reads as deleted on Drive. fn walk_remote( &self, folder_id: i64, folder: &NodeUid, prefix: &str, - out: &mut HashMap, - dirs: &mut HashMap, + walk: &mut RemoteWalk<'_>, baseline: &HashMap, ) -> Result<(), PassAbort> { // Checked per folder, not per pass: this walk is the long pole on a large @@ -1017,6 +1079,27 @@ impl Core { .rt .block_on(self.drive.enumerate_nodes_light(&uids)) .map_err(|e| format!("resolve nodes: {e}"))?; + // The SDK leaves out a node it cannot decrypt, and says so only in its + // log. Its name went with it, so all this can record is the folder. + if nodes.len() < uids.len() { + warn!( + folder = prefix, + listed = uids.len(), + read = nodes.len(), + "sync: some nodes on Drive cannot be read; leaving them out" + ); + walk.gaps.short_listing(prefix); + } + let nodes: Vec = nodes + .into_iter() + .filter(|node| { + let safe = crate::remote_name_is_safe(&node.name); + if !safe { + warn!(folder = prefix, uid = %node.uid, "sync: a name on Drive cannot be a local file name; leaving it out"); + } + safe + }) + .collect(); // Files whose recorded mtime no longer matches: their size has to be // read for real, so these are the only node keys worth unlocking. @@ -1048,8 +1131,8 @@ impl Core { let rel = join_rel(prefix, &node.name); self.progress_scanned(folder_id, base_name(&rel)); if node.is_folder() { - dirs.insert(rel.clone(), node.uid.clone()); - out.insert( + walk.dirs.insert(rel.clone(), node.uid.clone()); + walk.out.insert( rel.clone(), RemoteItem { uid: node.uid.clone(), @@ -1058,16 +1141,19 @@ impl Core { size: 0, }, ); - self.walk_remote(folder_id, &node.uid, &rel, out, dirs, baseline)?; + self.walk_remote(folder_id, &node.uid, &rel, walk, baseline)?; } else { let size = if stale.contains(&node.uid.to_string()) { match sized.get(&node.uid.to_string()) { Some(size) => *size, // Its size was needed but could not be read (the full // enumeration skipped it as undecryptable). Leave it out - // rather than record a stale size, exactly as a full - // walk would have. - None => continue, + // rather than record a stale size, and say so: missing, + // it would read as deleted on Drive. + None => { + walk.gaps.unread(rel); + continue; + } } } else { match unchanged_remote_size(baseline, &rel, node.modification_time) { @@ -1075,7 +1161,7 @@ impl Core { None => continue, } }; - out.insert( + walk.out.insert( rel, RemoteItem { uid: node.uid, @@ -1613,6 +1699,62 @@ fn keep_racing_local_edit(path: &Path, expected: Option) -> std::io::R preserve_conflict_copy(path, now_secs()).map(|_| ()) } +/// What became of one folder [`remove_local_dirs`] was asked to remove. +enum DirRemoval { + Removed(String), + /// Left in place, with why: it holds something Drive does not have. + Kept(String, String), + Failed(String, std::io::Error), +} + +/// Remove the mirror folders in `dirs`, which Drive deleted, deepest first, +/// except those that hold something Drive never had. +/// +/// The uploads of a new file or an edit in such a folder failed earlier in the +/// pass for want of a parent, and an ignored `.git` is never uploaded at all, +/// so removing the folder would lose them for good (B211). `current` is the +/// baseline as the pass has left it: a row the pass already removed with its +/// file must not read as a deletion not carried up yet, and a subfolder removed +/// here takes its rows with it for the same reason. A kept folder loses its own +/// row, so its parent sees it as new and is kept too. +fn remove_local_dirs( + root: &Path, + mut dirs: Vec, + current: &mut HashMap, +) -> Vec { + dirs.sort_by_key(|p| std::cmp::Reverse(p.matches('/').count())); + dirs.into_iter() + .map(|rel| { + if let Some(reason) = mirror_subtree_unsynced(root, &rel, current) { + current.remove(&rel); + return DirRemoval::Kept(rel, reason); + } + let prefix = format!("{rel}/"); + current.retain(|path, _| *path != rel && !path.starts_with(&prefix)); + match removed_locally(std::fs::remove_dir_all(root.join(rel_to_path(&rel)))) { + Ok(()) => DirRemoval::Removed(rel), + Err(e) => DirRemoval::Failed(rel, e), + } + }) + .collect() +} + +/// What a remote walk fills in as it goes down the tree. +struct RemoteWalk<'a> { + /// Every path it read, by rel path. + out: &'a mut HashMap, + /// Every folder it read, by rel path, to its uid. + dirs: &'a mut HashMap, + /// What it saw but could not read. + gaps: &'a mut RemoteGaps, +} + +/// Whether the file at `path` is no longer what the scan saw as `planned`. +/// A file that is gone has nothing left to lose, so it has not changed. +fn changed_since_scan(path: &Path, planned: &LocalItem) -> bool { + local_sig(path).is_some_and(|now| !now.same_content(&LocalSig::from(planned))) +} + /// Read a local removal as the sync engine has to read it: the *state* it was /// asking for is "this path is gone", so a path that was already gone is a /// success, not a failure. @@ -2892,6 +3034,83 @@ mod tests { (root, baseline) } + /// Drive deleted `dir`. Everything it held that was synced goes with it; + /// a file Drive never had keeps the folder, and the parent with it + /// (`docs/BUGS.md` B211). + #[test] + fn a_folder_deleted_on_drive_keeps_what_drive_never_had() { + let (root, mut baseline) = synced_subtree("b211-keep"); + // The pass already removed the synced files with their rows. + std::fs::remove_file(root.join("dir/a.txt")).unwrap(); + baseline.remove("dir/a.txt"); + std::fs::remove_file(root.join("dir/sub/b.txt")).unwrap(); + baseline.remove("dir/sub/b.txt"); + std::fs::write(root.join("dir/new.txt"), b"never uploaded").unwrap(); + + let removals = remove_local_dirs( + &root, + vec!["dir".to_string(), "dir/sub".to_string()], + &mut baseline, + ); + let shape: Vec<(&str, &str)> = removals + .iter() + .map(|r| match r { + DirRemoval::Removed(rel) => ("removed", rel.as_str()), + DirRemoval::Kept(rel, _) => ("kept", rel.as_str()), + DirRemoval::Failed(rel, _) => ("failed", rel.as_str()), + }) + .collect(); + assert_eq!(shape, [("removed", "dir/sub"), ("kept", "dir")]); + assert_eq!( + std::fs::read(root.join("dir/new.txt")).unwrap(), + b"never uploaded" + ); + assert!(!root.join("dir/sub").exists()); + // Its row is gone, so the next pass sees a new local folder. + assert!(!baseline.contains_key("dir")); + let _ = std::fs::remove_dir_all(&root); + } + + #[test] + fn a_folder_deleted_on_drive_with_only_synced_content_is_removed() { + let (root, mut baseline) = synced_subtree("b211-remove"); + for file in ["dir/a.txt", "dir/sub/b.txt"] { + std::fs::remove_file(root.join(file)).unwrap(); + baseline.remove(file); + } + let removals = remove_local_dirs( + &root, + vec!["dir/sub".to_string(), "dir".to_string()], + &mut baseline, + ); + assert!(removals.iter().all(|r| matches!(r, DirRemoval::Removed(_)))); + assert!(!root.join("dir").exists()); + let _ = std::fs::remove_dir_all(&root); + } + + /// An edit to a file saved after the scan is not a deletion on Drive + /// (`docs/BUGS.md` B213). + #[test] + fn a_file_changed_since_the_scan_is_not_deleted() { + let root = sync_test_dir("b213"); + let path = root.join("notes.txt"); + std::fs::write(&path, b"as scanned").unwrap(); + let sig = local_sig(&path).unwrap(); + let planned = LocalItem { + is_dir: false, + mtime: sig.mtime, + mtime_ns: sig.mtime_ns, + size: sig.size, + open_for_write: false, + }; + assert!(!changed_since_scan(&path, &planned)); + std::fs::write(&path, b"edited after the scan").unwrap(); + assert!(changed_since_scan(&path, &planned)); + std::fs::remove_file(&path).unwrap(); + assert!(!changed_since_scan(&path, &planned)); + let _ = std::fs::remove_dir_all(&root); + } + #[test] fn a_fully_synced_mirror_subtree_may_leave() { let (root, baseline) = synced_subtree("move-synced"); diff --git a/crates/pdfs-fuse/src/sync/planner.rs b/crates/pdfs-fuse/src/sync/planner.rs index ce51994..3b7a881 100644 --- a/crates/pdfs-fuse/src/sync/planner.rs +++ b/crates/pdfs-fuse/src/sync/planner.rs @@ -199,6 +199,135 @@ pub(super) fn guard_local_wipe( Ok(()) } +/// Refuse to run a pass whose remote side has vanished in its entirety: the +/// remote twin of [`guard_local_wipe`]. +/// +/// An empty or truncated remote walk reads to the planner as "Drive deleted +/// everything", which a pass answers by deleting the local copies. When not +/// one synced path is left on Drive, a failed listing is far likelier than a +/// deliberate wipe of the whole folder, so the pass stops instead. +pub(super) fn guard_remote_wipe( + baseline: &HashMap, + remote: &HashMap, +) -> Result<(), String> { + if !baseline.is_empty() && baseline.keys().all(|rel| !remote.contains_key(rel)) { + return Err(format!( + "every one of the {} synced paths is missing on Proton Drive; refusing to \ + delete them here. If they were deleted on purpose, remove the folder from sync.", + baseline.len() + )); + } + Ok(()) +} + +/// What a remote walk saw but could not read: a node the SDK left out because +/// it would not decrypt, or a file whose size could not be read. +/// +/// A path missing from the walk is read as deleted on Drive, so a pass must +/// not classify one it only failed to read. Since a node that failed to decrypt +/// has no name, all a walk can say is that a folder's listing came back short; +/// every path under that folder that the walk did not return is then left out, +/// as [`Unscanned`] leaves out what the local scan could not read. +#[derive(Default)] +pub(super) struct RemoteGaps { + /// Folders whose listing came back with fewer nodes than it named. + short: HashSet, + /// Paths seen, but whose entry could not be read. + unread: HashSet, +} + +impl RemoteGaps { + /// The listing of `folder` named nodes that did not come back. + pub(super) fn short_listing(&mut self, folder: &str) { + self.short.insert(folder.to_string()); + } + + /// `rel` is on Drive, but the walk could not read what it needs of it. + pub(super) fn unread(&mut self, rel: String) { + self.unread.insert(rel); + } + + pub(super) fn is_empty(&self) -> bool { + self.short.is_empty() && self.unread.is_empty() + } + + /// Whether `rel` may be missing from `remote` only because the walk could + /// not read it. A path the walk returned is never hidden. + pub(super) fn hides( + &self, + rel: &str, + remote: &HashMap, + remote_dirs: &HashMap, + ) -> bool { + if self.is_empty() || remote.contains_key(rel) { + return false; + } + if self.unread.contains(rel) { + return true; + } + // The deepest folder the walk did list decides: under a short listing, + // anything not returned may be one of the nodes it could not read. + let mut at = parent_rel(rel); + loop { + if remote_dirs.contains_key(at) { + return self.short.contains(at); + } + if at.is_empty() { + return false; + } + at = parent_rel(at); + } + } + + /// The folders to name to the user, sorted. + pub(super) fn report(&self) -> Vec { + let mut lines: Vec = self + .short + .iter() + .map(|rel| { + let shown = if rel.is_empty() { "the folder" } else { rel }; + format!("{shown} (items on Proton Drive that cannot be read)") + }) + .chain( + self.unread + .iter() + .map(|rel| format!("{rel} (cannot be read on Proton Drive)")), + ) + .collect(); + lines.sort_unstable(); + lines + } +} + +/// Why the remote folder at `rel` must not be trashed although it was deleted +/// here: something under it is new or changed on Drive since the last sync, +/// so it is another device's work that this computer has never had. +/// +/// Trashing the folder would take that work with it, and the next pass would +/// then delete the copy this pass just downloaded. `None` when everything under +/// it is exactly what the baseline recorded. +pub(super) fn remote_subtree_changed( + rel: &str, + remote: &HashMap, + baseline: &HashMap, +) -> Option { + let prefix = format!("{rel}/"); + let mut changed: Vec<&String> = remote + .iter() + .filter(|(path, _)| path.starts_with(&prefix)) + .filter(|(path, item)| match baseline.get(*path) { + None => true, + Some(_) if item.is_dir => false, + Some(base) => remote_sig(base) != Some((item.mtime, item.size)), + }) + .map(|(path, _)| path) + .collect(); + changed.sort_unstable(); + changed + .first() + .map(|path| format!("{path} is new or changed on Proton Drive")) +} + /// The size the baseline recorded for `rel`, if its remote signature's mtime /// still matches `mtime`. pub(super) fn unchanged_remote_size( @@ -530,4 +659,96 @@ mod file_plan_tests { FilePlan::Download ); } + + fn uid(id: &str) -> NodeUid { + NodeUid::new(VolumeId::from("v"), LinkId::from(id)) + } + + fn folder() -> RemoteItem { + RemoteItem { + is_dir: true, + mtime: 0, + size: 0, + ..remote(0, 0) + } + } + + /// An empty or failed remote listing must not read as "Drive deleted + /// everything" and delete the local copies. + #[test] + fn the_remote_wipe_guard_blocks_only_a_total_disappearance() { + let base = HashMap::from([("a".to_string(), ()), ("b".to_string(), ())]); + let none: HashMap = HashMap::new(); + assert!(guard_remote_wipe(&base, &none).is_err()); + let one = HashMap::from([("b".to_string(), remote(1, 1))]); + assert!(guard_remote_wipe(&base, &one).is_ok()); + let empty: HashMap = HashMap::new(); + assert!(guard_remote_wipe(&empty, &none).is_ok()); + } + + /// A node that failed to decrypt has no name, so everything a short + /// listing did not return is left out, and nothing else. + #[test] + fn a_short_remote_listing_hides_what_it_did_not_return() { + let remote = HashMap::from([ + ("docs".to_string(), folder()), + ("docs/a.txt".to_string(), remote(1, 1)), + ("pics".to_string(), folder()), + ]); + let dirs = HashMap::from([ + (String::new(), uid("root")), + ("docs".to_string(), uid("docs")), + ("pics".to_string(), uid("pics")), + ]); + let mut gaps = RemoteGaps::default(); + assert!(!gaps.hides("docs/b.txt", &remote, &dirs)); + gaps.short_listing("docs"); + // Returned paths stand; unreturned ones under the short folder hide, + // and so does anything under a subfolder it failed to return. + assert!(!gaps.hides("docs/a.txt", &remote, &dirs)); + assert!(gaps.hides("docs/b.txt", &remote, &dirs)); + assert!(gaps.hides("docs/sub/deep.txt", &remote, &dirs)); + // A complete sibling listing still reads deletions as deletions. + assert!(!gaps.hides("pics/gone.jpg", &remote, &dirs)); + assert!(!gaps.hides("top.txt", &remote, &dirs)); + gaps.short_listing(""); + assert!(gaps.hides("top.txt", &remote, &dirs)); + } + + #[test] + fn a_file_whose_size_could_not_be_read_is_hidden() { + let remote: HashMap = HashMap::new(); + let dirs = HashMap::from([(String::new(), uid("root"))]); + let mut gaps = RemoteGaps::default(); + gaps.unread("a.txt".to_string()); + assert!(gaps.hides("a.txt", &remote, &dirs)); + assert!(!gaps.hides("b.txt", &remote, &dirs)); + assert_eq!(gaps.report(), ["a.txt (cannot be read on Proton Drive)"]); + } + + /// A folder deleted here holds another device's new work on Drive, so + /// trashing it would lose that work. + #[test] + fn a_remote_folder_with_new_or_changed_files_is_not_trashed() { + let mut base = HashMap::from([ + ("x".to_string(), baseline(0, 0, 0, 0)), + ("x/old.txt".to_string(), baseline(1, 1, 5, 6)), + ]); + let mut remote_tree = HashMap::from([ + ("x".to_string(), folder()), + ("x/old.txt".to_string(), remote(5, 6)), + ("xy/new.txt".to_string(), remote(1, 1)), + ]); + // Unchanged under x; a name-prefixed sibling does not count. + assert_eq!(remote_subtree_changed("x", &remote_tree, &base), None); + + remote_tree.insert("x/old.txt".to_string(), remote(9, 6)); + assert!(remote_subtree_changed("x", &remote_tree, &base).is_some()); + + remote_tree.insert("x/old.txt".to_string(), remote(5, 6)); + remote_tree.insert("x/sub".to_string(), folder()); + assert!(remote_subtree_changed("x", &remote_tree, &base).is_some()); + base.insert("x/sub".to_string(), baseline(0, 0, 0, 0)); + assert_eq!(remote_subtree_changed("x", &remote_tree, &base), None); + } } diff --git a/docs/BUGS.md b/docs/BUGS.md index 7af894c..3794d18 100644 --- a/docs/BUGS.md +++ b/docs/BUGS.md @@ -100,6 +100,11 @@ a live account since. | Entry | Title | |---|---| +| [B215](#b215--a-remote-name-holding--or--becomes-a-path-outside-the-sync-folder) | A remote name holding `/` or `..` becomes a path outside the sync folder | +| [B214](#b214--deleting-a-mirror-folder-here-trashes-another-devices-new-files-in-it) | Deleting a mirror folder here trashes another device's new files in it | +| [B213](#b213--a-file-edited-during-a-mirror-pass-is-deleted-because-drive-deleted-it-earlier) | A file edited during a mirror pass is deleted because Drive deleted it earlier | +| [B212](#b212--a-node-on-drive-that-cannot-be-decrypted-is-deleted-from-the-mirror) | A node on Drive that cannot be decrypted is deleted from the mirror | +| [B211](#b211--a-folder-deleted-on-drive-takes-unsynced-local-files-with-it) | A folder deleted on Drive takes unsynced local files with it | | [B210](#b210--after-a-password-reset-the-mount-retries-missing-key-forever) | After a password reset, the mount retries "missing key" forever | | [B209](#b209--a-blocked-sign-in-says-the-app-version-is-no-longer-supported) | A blocked sign-in says the app version is no longer supported | | [B208](#b208--a-file-landing-while-its-folder-is-pinned-is-kept-but-not-available-offline) | A file landing while its folder is pinned is kept but not available offline | @@ -313,6 +318,129 @@ a live account since. Newest first. +### B215 — A remote name holding `/` or `..` becomes a path outside the sync folder + +**Status:** Fixed, not verified live +**Found:** 2026-10-16, architecture audit (`FINDINGS.md` 1.2). +**Where:** `walk_remote` and `walk_remote_dirs` in `crates/pdfs-fuse/src/sync.rs`; `node_visible` +in `crates/pdfs-fuse/src/lib.rs` + +**Repro:** Needs a node whose decrypted name holds `/`, `..` or NUL. Drive checks names only when +a client creates or renames a node, and only for length, so another client or a server that +should not be trusted can make one. Then let a mirror pass or a listing of its folder run. + +**Cause.** The mirror engine joined each decrypted name onto its parent's relative path and +turned that into a local path, so `x/../../.config/autostart/a.desktop` named a file outside the +sync root. The pass then created, replaced or removed it there. In the mount, the kernel refuses a +whole `readdir` with EIO when one name in it is not a single path component. + +**Fix.** `remote_name_is_safe` accepts a name only if it is one path component: not empty, not +`.` or `..`, no `/` or NUL, at most 255 bytes. Both remote walks of the mirror engine and the +listing filter of the mount leave out any other name, with a warning. A name left out cannot be a +local path at all, so leaving it out cannot read as a deletion of anything local. + +**Test:** `tests::a_remote_name_that_is_not_one_path_component_is_left_out`. + +--- + +### B214 — Deleting a mirror folder here trashes another device's new files in it + +**Status:** Fixed, not verified live +**Found:** 2026-10-16, architecture audit (`FINDINGS.md` 1.5). +**Where:** `Core::do_reconcile` in `crates/pdfs-fuse/src/sync.rs` + +**Repro:** Delete a synced folder locally while another device adds a file to it on Drive, before +the next pass. + +**Cause.** The pass planned the new file as a download, which recreated the folder locally. At +the end of the same pass it trashed the remote folder, because the folder was in the baseline and +gone locally, and took the new file with it. The next pass then saw the downloaded copy unchanged +and its remote gone, so it deleted that too. The other device's work survived only in the Drive +trash, and nothing said so. + +**Fix.** Before a pass trashes a remote folder, `remote_subtree_changed` looks for anything under +it on Drive that is not in the baseline or whose signature moved. If there is, the folder stays on +Drive with its row, the activity feed says why, and only its unchanged files are trashed, one by +one, as local deletions. + +**Test:** `planner::file_plan_tests::a_remote_folder_with_new_or_changed_files_is_not_trashed`. + +--- + +### B213 — A file edited during a mirror pass is deleted because Drive deleted it earlier + +**Status:** Fixed, not verified live +**Found:** 2026-10-16, architecture audit (`FINDINGS.md` 1.6). +**Where:** `Core::do_reconcile`, `FilePlan::DeleteLocal`, in `crates/pdfs-fuse/src/sync.rs` + +**Repro:** Delete a synced file on another device. On this one, edit the file while a pass over a +large folder walks Drive. + +**Cause.** A pass takes its local signatures before the remote walk, which takes minutes on a +large tree, and deleted a file that Drive had deleted without looking at it again. Downloads got +that second look in B39 (`keep_racing_local_edit`); deletions did not, so the edit was lost with +no conflict copy. + +**Fix.** `changed_since_scan` compares the file's signature with the one the scan planned on, right +before the deletion. A changed file stays, and the next pass uploads it as a new file. + +**Test:** `tests::a_file_changed_since_the_scan_is_not_deleted`. + +--- + +### B212 — A node on Drive that cannot be decrypted is deleted from the mirror + +**Status:** Fixed, not verified live +**Found:** 2026-10-16, architecture audit (`FINDINGS.md` 1.4). The SDK leaves out nodes it cannot +decrypt, which B112 showed on real accounts and B97 during cancellation. +**Where:** `Core::walk_remote` and `Core::do_reconcile` in `crates/pdfs-fuse/src/sync.rs` + +**Repro:** Make one synced file or folder fail to decrypt, for example with a key the session +cannot unlock, and let a pass run. + +**Cause.** The remote walk kept what the SDK returned and nothing else. A node the SDK left out +was then missing from the remote map, and the planner reads "in the baseline and here, but not on +Drive" as a deletion on Drive. A file was deleted locally; a folder was removed with +`remove_dir_all`. The local side had a guard against a scan that came back empty +(`guard_local_wipe`), the remote side had none. + +**Fix.** +- A listing that returns fewer nodes than it named marks its folder as short. A file whose size + could not be read is marked unread. +- `RemoteGaps::hides` leaves out of the pass every path such a gap could explain, as `Unscanned` + does for the local scan. The folder is reported with the paths that cannot be synced. +- `guard_remote_wipe` refuses a pass in which none of the synced paths is left on Drive. + +**Test:** `planner::file_plan_tests::a_short_remote_listing_hides_what_it_did_not_return`, +`planner::file_plan_tests::a_file_whose_size_could_not_be_read_is_hidden`, +`planner::file_plan_tests::the_remote_wipe_guard_blocks_only_a_total_disappearance`. + +--- + +### B211 — A folder deleted on Drive takes unsynced local files with it + +**Status:** Fixed, not verified live +**Found:** 2026-10-16, architecture audit (`FINDINGS.md` 1.1). +**Where:** `Core::do_reconcile` and `remove_local_dirs` in `crates/pdfs-fuse/src/sync.rs` + +**Repro:** In a mirror folder, add a file to a synced subfolder. Before the next pass, delete the +subfolder on another device. + +**Cause.** The pass put the folder on its list of local deletions. The upload of the new file +failed for want of a remote parent and was counted as an error. At the end of the pass, +`remove_dir_all` removed the folder with everything in it: new files, edits, files open for +writing, and ignored content such as `.git`, none of which had ever been on Drive. + +**Fix.** `remove_local_dirs` asks `mirror_subtree_unsynced` about each folder before it removes +it, against the baseline as the pass has left it. A folder that holds anything Drive does not +have stays, and loses its baseline row, so the next pass creates it on Drive again and uploads +what it holds. What was synced in it is still deleted. + +**Test:** `tests::a_folder_deleted_on_drive_keeps_what_drive_never_had`, +`tests::a_folder_deleted_on_drive_with_only_synced_content_is_removed`. + +--- + ### B210 — After a password reset, the mount retries "missing key" forever **Status:** Fixed, not verified live