From eb53d122ccad73f44681903a5d0a40f48b2d5f6b Mon Sep 17 00:00:00 2001 From: Nils Pukropp Date: Fri, 2 Oct 2026 19:54:37 +0200 Subject: [PATCH] fix(fuse,cli): let rm, rename and mkdir take paths in on-demand folders `pdfs rm ~/Downloads/x` answered "is not under the mountpoint" while `pdfs move` took the same path: the Delete, Rename and CreateFolder handlers still read paths against the primary mount only (B88). They now go through `route_change`, the router ResolveConflict already used, renamed from `route_conflict`. It answers the request in the inode space of the mount that owns the path. An on-demand folder's root has a parent on Drive, so `pdfs rm ~/Downloads` would have trashed the whole synced folder from under its mount. rm and rename refuse a path that names a mount's root. --- crates/pdfs-cli/src/main.rs | 9 ++-- crates/pdfs-fuse/src/control.rs | 92 +++++++++++++++++++++++---------- docs/BUGS.md | 19 ++++++- 3 files changed, 89 insertions(+), 31 deletions(-) diff --git a/crates/pdfs-cli/src/main.rs b/crates/pdfs-cli/src/main.rs index 7becb94..7dfb5ad 100644 --- a/crates/pdfs-cli/src/main.rs +++ b/crates/pdfs-cli/src/main.rs @@ -285,7 +285,8 @@ enum Command { }, /// Rename a file or folder via the running daemon. Rename { - /// File/folder path, inside the mountpoint or relative to it. + /// File/folder path, inside My files or an on-demand synced folder, or + /// relative to the mountpoint. path: PathBuf, /// New name (a single path component). new_name: String, @@ -304,12 +305,14 @@ enum Command { }, /// Trash a file or folder via the running daemon. Rm { - /// File/folder path, inside the mountpoint or relative to it. + /// File/folder path, inside My files or an on-demand synced folder, or + /// relative to the mountpoint. path: PathBuf, }, /// Create a folder via the running daemon. Mkdir { - /// Parent folder path, inside the mountpoint or relative to it. + /// Parent folder path, inside My files or an on-demand synced folder, + /// or relative to the mountpoint. parent: PathBuf, /// New folder name (a single path component). name: String, diff --git a/crates/pdfs-fuse/src/control.rs b/crates/pdfs-fuse/src/control.rs index f7d94ef..e7beaac 100644 --- a/crates/pdfs-fuse/src/control.rs +++ b/crates/pdfs-fuse/src/control.rs @@ -163,8 +163,8 @@ fn read_request_line(reader: &mut impl BufRead) -> std::io::Result CoreResult<(Cor rel_to_mount(mountpoint, path).map(|rel| (core.clone(), rel)) } -/// [`route_to_mount`] for a conflict copy. The listing names a copy inside a -/// synced folder by its absolute local path (`docs/BUGS.md` B99), so the -/// resolve has to run against the on-demand mount serving it. A mirror folder -/// has no mount to run against: its copy is an ordinary local file. -fn route_conflict(core: &Core, mountpoint: &Path, path: &str) -> CoreResult<(Core, PathBuf)> { +/// [`route_to_mount`] for a request that changes what it names: trash, rename, +/// create a folder, resolve a conflict. Each has to run against the on-demand +/// mount serving the path, and `pdfs rm ~/Downloads/x` answered "is not under +/// the mountpoint" while `pdfs move` took the same path. The conflict listing +/// names a copy in a synced folder by its absolute local path (`docs/BUGS.md` +/// B99), so resolving one needs this too. A mirror folder has no mount to run +/// against: a file there is an ordinary local file. +fn route_change(core: &Core, mountpoint: &Path, path: &str) -> CoreResult<(Core, PathBuf)> { let routed = route_to_mount(core, mountpoint, path).map_err(|e| { let p = Path::new(path); let mirrored = core @@ -200,7 +203,7 @@ fn route_conflict(core: &Core, mountpoint: &Path, path: &str) -> CoreResult<(Cor .any(|folder| folder.mode == "mirror" && p.starts_with(&folder.local_path)); if mirrored { CoreError::invalid(format!( - "{path} is in a mirrored folder; rename or delete the local file there" + "{path} is in a mirrored folder; change the local file there instead" )) } else { e @@ -217,6 +220,27 @@ fn route_conflict(core: &Core, mountpoint: &Path, path: &str) -> CoreResult<(Cor } } +/// [`route_change`] for a request that trashes or renames the node it names. +/// A mount's root is My files or a whole synced folder. An on-demand folder's +/// root has a parent on Drive, so nothing further down would stop `pdfs rm +/// ~/Downloads` from trashing the folder out from under its own mount. +fn route_node(core: &Core, mountpoint: &Path, path: &str) -> CoreResult<(Core, PathBuf)> { + let routed = route_change(core, mountpoint, path)?; + if below_root(&routed.1) { + Ok(routed) + } else { + Err(CoreError::invalid(format!( + "{path} is the root of a synced location and cannot be changed" + ))) + } +} + +/// Whether a mount-relative path names something below the mount's root. +fn below_root(rel: &Path) -> bool { + rel.components() + .any(|component| matches!(component, Component::Normal(_))) +} + /// Turn a CLI-supplied path into a mountpoint-relative path. An absolute path /// must live under `mountpoint`; a relative path is taken as already relative to /// the mount root. @@ -959,8 +983,8 @@ fn handle_control_conn(core: &Core, username: &str, mountpoint: &Path, stream: U }, Err(e) => CtlResponse::error(e), }, - Ok(CtlRequest::Rename { path, new_name }) => match rel_to_mount(mountpoint, &path) { - Ok(rel) => match core.rename(&rel, &new_name) { + Ok(CtlRequest::Rename { path, new_name }) => match route_node(core, mountpoint, &path) { + Ok((core, rel)) => match core.rename(&rel, &new_name) { Ok(name) => { core.log_activity(ActivityKind::Rename, &name, format!("was {path}"), true); CtlResponse::Ok { @@ -1017,8 +1041,8 @@ fn handle_control_conn(core: &Core, username: &str, mountpoint: &Path, stream: U (Err(e), _) | (_, Err(e)) => CtlResponse::error(e), } } - Ok(CtlRequest::Delete { path }) => match rel_to_mount(mountpoint, &path) { - Ok(rel) => match core.delete(&rel) { + Ok(CtlRequest::Delete { path }) => match route_node(core, mountpoint, &path) { + Ok((core, rel)) => match core.delete(&rel) { Ok(name) => { core.log_activity(ActivityKind::Trash, &name, "", true); CtlResponse::Ok { @@ -1032,21 +1056,23 @@ fn handle_control_conn(core: &Core, username: &str, mountpoint: &Path, stream: U }, Err(e) => CtlResponse::error(e), }, - Ok(CtlRequest::CreateFolder { parent, name }) => match rel_to_mount(mountpoint, &parent) { - Ok(parent_rel) => match core.create_folder(&parent_rel, &name) { - Ok(name) => { - core.log_activity(ActivityKind::CreateFolder, &name, "", true); - CtlResponse::Ok { - message: format!("created folder {name}"), + Ok(CtlRequest::CreateFolder { parent, name }) => { + match route_change(core, mountpoint, &parent) { + Ok((core, parent_rel)) => match core.create_folder(&parent_rel, &name) { + Ok(name) => { + core.log_activity(ActivityKind::CreateFolder, &name, "", true); + CtlResponse::Ok { + message: format!("created folder {name}"), + } } - } - Err(e) => { - core.log_activity(ActivityKind::CreateFolder, &name, &e, false); - CtlResponse::error(e) - } - }, - Err(e) => CtlResponse::error(e), - }, + Err(e) => { + core.log_activity(ActivityKind::CreateFolder, &name, &e, false); + CtlResponse::error(e) + } + }, + Err(e) => CtlResponse::error(e), + } + } Ok(CtlRequest::UploadPaths { parent, sources }) => { match rel_to_mount(mountpoint, &parent) { // Ack immediately and upload on a background thread: a directory tree @@ -1363,7 +1389,7 @@ fn handle_control_conn(core: &Core, username: &str, mountpoint: &Path, stream: U Err(e) => CtlResponse::error(e), }, Ok(CtlRequest::ResolveConflict { path, keep }) => { - match route_conflict(core, mountpoint, &path) { + match route_change(core, mountpoint, &path) { Ok((core, rel)) => match core.resolve_conflict(&rel, &keep) { Ok(message) => CtlResponse::Ok { message }, Err(e) => CtlResponse::error(e), @@ -1864,6 +1890,18 @@ mod request_limit_tests { assert!(rel_to_mount(mountpoint, "Photos/../../escape").is_err()); assert!(rel_to_mount(mountpoint, "/mnt/elsewhere").is_err()); } + + /// `pdfs rm` and `pdfs rename` reach on-demand mounts, whose root is a whole + /// synced folder with a parent on Drive. Only a path below a root may get + /// through to the trash or rename call. + #[test] + fn only_a_path_below_a_mount_root_names_a_node() { + assert!(below_root(Path::new("Photos"))); + assert!(below_root(Path::new("./Photos/2026"))); + assert!(!below_root(Path::new(""))); + assert!(!below_root(Path::new("."))); + assert!(!below_root(Path::new("./."))); + } } #[cfg(test)] diff --git a/docs/BUGS.md b/docs/BUGS.md index be8d1df..4093118 100644 --- a/docs/BUGS.md +++ b/docs/BUGS.md @@ -804,7 +804,9 @@ in every version — but that was luck, and the next question of this shape may ## B88 — `pdfs pin`, `rename`, `move`, `delete` and sharing still cannot name a path under a secondary mount -**Status:** Open — B86's fix covers `ls` and `refresh` only. +**Status:** Partly fixed. B86's fix covers `ls` and `refresh`. `move` takes paths in any location +through `Core::move_between`. `rm`, `rename` and `mkdir` were fixed 2026-10-02 (unverified live). +`pin`, `unpin`, opening a file, thumbnails, uploads, sharing and versions are still open. **Found:** 2026-08-17, while fixing B86. **Where:** `crates/pdfs-fuse/src/control.rs`, every handler still calling `rel_to_mount`. @@ -830,6 +832,21 @@ per-mount state assumptions as it goes; the `for_each_state`/uid-keyed helpers a the ones that touch more than their own inode space. Test: a `pdfs pin` and a `pdfs rename` by absolute path inside a secondary on-demand mount, plus a path under no mount still erroring. +**Fix so far (`rm`, `rename`, `mkdir`):** the three handlers go through `control.rs::route_change`, +which is `route_to_mount` plus the mirror-folder hint and escape check that `ResolveConflict` +already had. The audit found nothing primary-only in `Core::delete`, `Core::rename` or +`Core::create_folder`. `Core::rooted_at` swaps in the fork's state, notifier, session flag and size +upgrades, the same per-mount fields `Core::fork_state` makes fresh. So `self.state()` in a routed +call already is the fork's inode space. My files and the on-demand folders do not overlap, so +`delete` forgetting the node in its own space alone is enough. `rename` already forgets it +everywhere (B74). + +Routing raised a hazard the primary mount never had. An on-demand folder's root has a parent on +Drive, so `pdfs rm ~/Downloads` would have passed `source_parent_uid` and trashed the whole synced +folder from under its mount. `rm` and `rename` therefore go through `route_node`, which refuses a +path naming a mount's root. Test: `only_a_path_below_a_mount_root_names_a_node`. There is still no +handler-level test, because a `Core` can only be built against a live client. + --- ## B87 — A revision's block geometry is learned a read too late