diff --git a/crates/pdfs-core/src/lib.rs b/crates/pdfs-core/src/lib.rs index 5338996..f429310 100644 --- a/crates/pdfs-core/src/lib.rs +++ b/crates/pdfs-core/src/lib.rs @@ -23,6 +23,7 @@ pub mod service; pub mod shell; pub mod similar; pub mod syncignore; +pub mod syncroot; pub mod takeout; pub mod tray; diff --git a/crates/pdfs-core/src/syncroot.rs b/crates/pdfs-core/src/syncroot.rs new file mode 100644 index 0000000..2fffafa --- /dev/null +++ b/crates/pdfs-core/src/syncroot.rs @@ -0,0 +1,209 @@ +//! Which local folders may become a sync root. +//! +//! A mirror uploads everything under its root and deletes what Drive deleted, +//! so a root that holds more than the user meant is dangerous. `$HOME` takes +//! the decrypted content cache, `cache.db` and `~/.ssh` up to Drive, and walks +//! into the mount, which downloads the whole Drive only to upload it again. +//! Every place a root is added or restored goes through [`problem`]. + +use std::path::{Component, Path, PathBuf}; + +use crate::config::{AppConfig, AppDirs}; + +/// The paths a sync root must stay clear of. +#[derive(Debug, Clone, Default)] +pub struct Reserved { + /// The user's home folder. A root may sit inside it, not be it or above it. + pub home: Option, + /// The Proton Drive mount. + pub mountpoint: Option, + /// This app's own configuration, state and cache folders. + pub app_dirs: Vec, +} + +impl Reserved { + /// The reserved paths of this user and `config`, resolved like a root. + pub fn current(config: &AppConfig) -> Self { + let Ok(dirs) = AppDirs::new() else { + return Self::default(); + }; + let resolved = |path: PathBuf| resolve(&path).unwrap_or(path); + let mut app_dirs = vec![dirs.state_dir(), dirs.cache_dir()]; + if let Some(config_dir) = dirs.config_path().parent() { + app_dirs.push(config_dir.to_path_buf()); + } + Self { + home: dirs.home_dir().map(resolved), + mountpoint: Some(resolved(dirs.resolved_mountpoint(config))), + app_dirs: app_dirs.into_iter().map(resolved).collect(), + } + } +} + +/// `path` as a root is compared: absolute, with symlinks resolved as far as the +/// path exists. A folder a restore is about to create does not exist yet, so +/// the part past the deepest existing folder is kept as written. +/// +/// A `..` is refused rather than resolved: before a symlink it means something +/// else than after it, and a path recorded on another machine has no business +/// holding one. +pub fn resolve(path: &Path) -> Result { + if !path.is_absolute() { + return Err(format!("{} is not an absolute path", path.display())); + } + if path.components().any(|c| c == Component::ParentDir) { + return Err(format!("{} goes up with `..`", path.display())); + } + let mut existing = path.to_path_buf(); + let mut rest = Vec::new(); + loop { + match existing.canonicalize() { + Ok(canonical) => { + return Ok(rest + .into_iter() + .rev() + .fold(canonical, |path: PathBuf, part| path.join(part))); + } + Err(_) => { + let Some(name) = existing.file_name().map(|n| n.to_os_string()) else { + return Ok(path.to_path_buf()); + }; + rest.push(name); + if !existing.pop() { + return Ok(path.to_path_buf()); + } + } + } + } +} + +/// Why `root` cannot be synced, or `None` when it can. `root` is resolved +/// ([`resolve`]); `others` are the roots already synced. +pub fn problem(root: &Path, others: &[PathBuf], reserved: &Reserved) -> Option { + let shown = root.display(); + if root.parent().is_none() { + return Some(format!("{shown} is the root of the file system")); + } + if let Some(home) = &reserved.home { + if root == home { + return Some(format!( + "{shown} is your home folder; pick a folder inside it" + )); + } + if home.starts_with(root) { + return Some(format!("{shown} holds your home folder")); + } + } + if let Some(mountpoint) = &reserved.mountpoint + && overlaps(root, mountpoint) + { + return Some(format!( + "{shown} overlaps the Proton Drive folder {}", + mountpoint.display() + )); + } + if let Some(dir) = reserved.app_dirs.iter().find(|dir| overlaps(root, dir)) { + return Some(format!( + "{shown} overlaps this app's own data in {}", + dir.display() + )); + } + if others.iter().any(|other| other == root) { + return Some(format!("{shown} is already synced")); + } + if let Some(other) = others.iter().find(|other| overlaps(root, other)) { + return Some(format!( + "{shown} overlaps the synced folder {}", + other.display() + )); + } + None +} + +/// Whether one of `a` and `b` is the other or inside it. +fn overlaps(a: &Path, b: &Path) -> bool { + a.starts_with(b) || b.starts_with(a) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn reserved() -> Reserved { + Reserved { + home: Some("/home/u".into()), + mountpoint: Some("/home/u/ProtonDrive".into()), + app_dirs: vec![ + "/home/u/.local/state/proton-drive-linux".into(), + "/home/u/.cache/proton-drive-linux".into(), + "/home/u/.config/proton-drive-linux".into(), + ], + } + } + + fn refused(root: &str, others: &[&str]) -> bool { + let others: Vec = others.iter().map(PathBuf::from).collect(); + problem(Path::new(root), &others, &reserved()).is_some() + } + + #[test] + fn a_folder_inside_home_can_be_synced() { + assert!(!refused("/home/u/Documents", &[])); + assert!(!refused("/home/u/Pictures", &["/home/u/Documents"])); + assert!(!refused("/mnt/data/photos", &[])); + } + + #[test] + fn the_file_system_root_home_and_its_parents_are_refused() { + assert!(refused("/", &[])); + assert!(refused("/home/u", &[])); + assert!(refused("/home", &[])); + } + + #[test] + fn the_mount_and_the_apps_own_data_are_refused_either_way_round() { + assert!(refused("/home/u/ProtonDrive", &[])); + assert!(refused("/home/u/ProtonDrive/Documents", &[])); + assert!(refused("/home/u/.cache", &[])); + assert!(refused( + "/home/u/.local/state/proton-drive-linux/staging", + &[] + )); + assert!(refused("/home/u/.config", &[])); + } + + #[test] + fn a_root_inside_or_around_another_root_is_refused() { + assert!(refused("/home/u/Documents", &["/home/u/Documents"])); + assert!(refused("/home/u/Documents/Work", &["/home/u/Documents"])); + assert!(refused("/home/u/stuff", &["/home/u/stuff/Documents"])); + // A shared prefix of characters is not a shared folder. + assert!(!refused("/home/u/Documents2", &["/home/u/Documents"])); + } + + #[test] + fn a_path_that_goes_up_or_is_relative_does_not_resolve() { + assert!(resolve(Path::new("/home/u/../../etc")).is_err()); + assert!(resolve(Path::new("Documents")).is_err()); + } + + #[test] + fn a_folder_still_to_be_created_resolves_under_its_existing_parent() { + let base = std::env::temp_dir().join(format!("pdfs-syncroot-{}", std::process::id())); + std::fs::create_dir_all(&base).unwrap(); + let canonical = base.canonicalize().unwrap(); + assert_eq!( + resolve(&base.join("new/inner")).unwrap(), + canonical.join("new/inner") + ); + let link = base.join("link"); + let _ = std::fs::remove_file(&link); + std::os::unix::fs::symlink(&canonical, &link).unwrap(); + assert_eq!( + resolve(&link.join("later")).unwrap(), + canonical.join("later"), + "a symlink is followed to where it points" + ); + std::fs::remove_dir_all(&base).unwrap(); + } +} diff --git a/crates/pdfs-fuse/src/devices.rs b/crates/pdfs-fuse/src/devices.rs index 3f660c0..72560b7 100644 --- a/crates/pdfs-fuse/src/devices.rs +++ b/crates/pdfs-fuse/src/devices.rs @@ -105,6 +105,15 @@ pub(crate) fn adopted_device_uid() -> Option { (!uid.is_empty()).then_some(uid) } +/// The paths no sync root may overlap, with the mountpoint as the config says +/// now. +pub(crate) fn sync_root_reserved() -> pdfs_core::syncroot::Reserved { + let config = pdfs_core::config::AppDirs::new() + .map(|dirs| dirs.load_config()) + .unwrap_or_default(); + pdfs_core::syncroot::Reserved::current(&config) +} + /// Pin (or, with `None`, unpin) the adopted device uid in `config.json`. pub(crate) fn set_adopted_device_uid(uid: Option<&str>) -> CoreResult<()> { let dirs = pdfs_core::config::AppDirs::new() @@ -517,18 +526,20 @@ impl Core { .map_err(|e| CoreError::internal(format!("canonicalize {}: {e}", local.display())))?; let local_str = local.to_string_lossy().to_string(); - // Reject duplicates up front for a clear error (UNIQUE would also catch it). - if self + // `/`, the home folder, the mount, this app's own data, and a folder + // inside or around another root are refused (`syncroot::problem`). A + // duplicate is one of those, refused here for a clear error before + // the UNIQUE constraint would catch it. + let others = self .db .sync_folder_list() .map_err(|e| CoreError::internal(format!("db: {e:?}")))? .iter() - .any(|f| f.local_path == local_str) - { - return Err(CoreError::invalid(format!( - "{} is already synced", - local.display() - ))); + .map(|f| PathBuf::from(&f.local_path)) + .collect::>(); + let reserved = sync_root_reserved(); + if let Some(problem) = pdfs_core::syncroot::problem(&local, &others, &reserved) { + return Err(CoreError::invalid(problem)); } let name = local diff --git a/crates/pdfs-fuse/src/profile.rs b/crates/pdfs-fuse/src/profile.rs index 348e80f..0d3ad47 100644 --- a/crates/pdfs-fuse/src/profile.rs +++ b/crates/pdfs-fuse/src/profile.rs @@ -304,6 +304,9 @@ impl Core { // offering it as a restorable folder would sync the bookkeeping // back onto the machine it describes. .filter(|n| n.is_folder() && !n.trashed && n.name != PROFILE_DIR_NAME) + // The name becomes a folder under the home folder: one with a `/` + // or a `..` in it would put the restore somewhere else. + .filter(|n| crate::remote_name_is_safe(&n.name)) .map(|n| { let uid = n.uid.to_string(); let saved = profile @@ -349,23 +352,29 @@ impl Core { .sync_folder_list() .map_err(|e| CoreError::internal(format!("db: {e:?}")))?; + let reserved = crate::devices::sync_root_reserved(); + let mut roots: Vec = existing + .iter() + .map(|f| PathBuf::from(&f.local_path)) + .collect(); let mut restored = 0usize; let mut skipped: Vec = Vec::new(); for item in items { - let local = PathBuf::from(&item.local_path); - if !local.is_absolute() { - skipped.push(format!("{}: local path must be absolute", item.local_path)); - continue; - } + // The path is proposed from the profile another machine wrote, and + // may hold a folder name from Drive: checked like any new root. + let local = match pdfs_core::syncroot::resolve(Path::new(&item.local_path)) { + Ok(local) => local, + Err(e) => { + skipped.push(format!("{}: {e}", item.local_path)); + continue; + } + }; if existing.iter().any(|f| f.remote_uid == item.remote_uid) { skipped.push(format!("{}: already synced", item.local_path)); continue; } - if existing - .iter() - .any(|f| Path::new(&f.local_path) == local.as_path()) - { - skipped.push(format!("{}: path already in use", item.local_path)); + if let Some(problem) = pdfs_core::syncroot::problem(&local, &roots, &reserved) { + skipped.push(format!("{}: {problem}", item.local_path)); continue; } if let Err(e) = std::fs::create_dir_all(&local) { @@ -390,6 +399,7 @@ impl Core { let _ = self.request_sync_folder_mode(id, "ondemand"); } let _ = self.sync_tx.send(sync::SyncMsg::Reconcile(id)); + roots.push(local); restored += 1; } let _ = self.sync_tx.send(sync::SyncMsg::Rewatch); diff --git a/docs/BUGS.md b/docs/BUGS.md index 9998ea2..e2c42cf 100644 --- a/docs/BUGS.md +++ b/docs/BUGS.md @@ -100,6 +100,7 @@ a live account since. | Entry | Title | |---|---| +| [B234](#b234--any-folder-can-become-a-sync-root-the-home-folder-included) | Any folder can become a sync root, the home folder included | | [B233](#b233--signing-out-leaves-the-session-valid-on-proton) | Signing out leaves the session valid on Proton | | [B232](#b232--names-with--or--show-as-empty-rows-or-are-drawn-as-markup) | Names with & or < show as empty rows or are drawn as markup | | [B231](#b231--a-large-upload-under-a-low-upload-limit-never-finishes) | A large upload under a low upload limit never finishes | @@ -336,6 +337,39 @@ a live account since. Newest first. +### B234 — Any folder can become a sync root, the home folder included + +**Status:** Fixed, not verified live +**Found:** 2026-10-16, architecture audit (`FINDINGS.md` 1.12). +**Where:** `crates/pdfs-core/src/syncroot.rs`; `Core::add_sync_folder` and `sync_root_reserved` in +`crates/pdfs-fuse/src/devices.rs`; `Core::restore_sync_folders` and the restore proposals in +`crates/pdfs-fuse/src/profile.rs` + +**Repro:** `pdfs sync add ~`, or `pdfs sync add /`. Or restore a profile whose recorded path is +`/home/u/../..`, or whose folder on Drive has a name with a `/`. + +**Cause.** A new sync root was only checked for being a directory and not already synced. A +restore checked only that the path was absolute. Nothing refused `/`, the home folder, a parent +of either, the Proton Drive mount, the app's own state, cache and config folders, or a root +inside or around another root. Adding the home folder uploads the decrypted content cache, +`cache.db` and `~/.ssh`, and walks into the mount, which downloads the Drive only to upload it +again. A restore proposal joined a folder name from Drive onto the home folder as it was. + +**Fix.** One check, `syncroot::problem`, runs wherever a root is added or restored. It refuses +the cases above. Paths are compared after `syncroot::resolve`, which follows symlinks as far as +the path exists and refuses `..`. Restore proposals leave out folders whose name could not be a +local file name. + +The audit also suggested not crossing into other file systems during the local walk. That was +left out on purpose: a btrfs subvolume inside a sync root has a device id of its own, and would +silently stop syncing. + +**Test:** `syncroot::tests` (six cases, among them +`the_file_system_root_home_and_its_parents_are_refused` and +`a_root_inside_or_around_another_root_is_refused`). + +--- + ### B233 — Signing out leaves the session valid on Proton **Status:** Fixed, not verified live