From 109ecd455bcf87a6fa54c8ddcb35e5a65c487b68 Mon Sep 17 00:00:00 2001 From: Erick Guan <297343+erickguan@users.noreply.github.com> Date: Tue, 9 Jun 2026 10:52:58 +0800 Subject: [PATCH 1/3] Use confined_join and root_join across services --- core/core/src/raw/path.rs | 66 +++++++++++++++++++++++++++ core/services/compfs/src/backend.rs | 18 ++++---- core/services/compfs/src/core.rs | 36 +++++---------- core/services/compfs/src/deleter.rs | 4 +- core/services/fs/src/core.rs | 31 ++----------- core/services/monoiofs/src/backend.rs | 10 ++-- core/services/monoiofs/src/core.rs | 38 +++++---------- core/services/monoiofs/src/deleter.rs | 2 +- 8 files changed, 109 insertions(+), 96 deletions(-) diff --git a/core/core/src/raw/path.rs b/core/core/src/raw/path.rs index 55733f47cec9..dfe433d1277d 100644 --- a/core/core/src/raw/path.rs +++ b/core/core/src/raw/path.rs @@ -17,6 +17,7 @@ use crate::*; use std::hash::{BuildHasher, Hasher}; +use std::path::{Component, Path, PathBuf}; /// build_abs_path will build an absolute path with root. /// @@ -147,6 +148,35 @@ pub fn normalize_root(v: &str) -> String { v } +/// Join a path to a service's root. Useful for local filesystem services to +/// reject `..` traversal in a key so path joins cannot escape `root`. +/// +/// Services must confine operations within configured root. Local filesystem services +/// could use this function to confine paths within configured root. +/// +/// Expect `path` comes from `normalize_path` before calling this function. +/// OpenDAL dispatches path to services after `normalize_path`, so services can +/// safely assume that `path` is normalized. +pub fn confined_join(root: &Path, path: &str) -> Result { + let trimmed = path.trim_end_matches('/'); + + // `normalize_path` strips leading `/` and empty segments but + // intentionally does NOT resolve `.`/`..`, and `PathBuf::join` is purely + // lexical, so a key such as `../../etc/passwd` would otherwise escape the + // configured `root` at syscall time. + if Path::new(trimmed) + .components() + .any(|c| matches!(c, Component::ParentDir)) + { + return Err(Error::new( + ErrorKind::NotFound, + "path escapes the configured root via `..`", + ) + .with_context("path", path)); + } + Ok(root.join(trimmed)) +} + /// Get basename from path. pub fn get_basename(path: &str) -> &str { // Handle root case @@ -380,6 +410,42 @@ mod tests { } } + #[test] + fn test_confined_join() { + let root = Path::new("/root"); + let cases = vec![ + ("root path", "/", root.to_path_buf()), + ("empty path", "", root.to_path_buf()), + ("current path", ".", root.to_path_buf()), + ("input file", "def", root.join("def")), + ("input nested file", "abc/def", root.join("abc/def")), + ("input dir", "def/", root.join("def")), + ("input nested dir", "abc/def/", root.join("abc/def")), + ]; + + for (name, input, expect) in cases { + let actual = confined_join(root, input).expect(name); + assert_eq!(actual, expect, "{name}"); + } + } + + #[test] + fn test_confined_join_rejects_parent_dir() { + let root = Path::new("/root"); + let cases = vec![ + ("parent dir", ".."), + ("parent dir file", "../def"), + ("nested parent dir", "abc/../def"), + ("trailing parent dir", "abc/.."), + ("trailing parent dir slash", "abc/../"), + ]; + + for (name, input) in cases { + let err = confined_join(root, input).expect_err(name); + assert_eq!(err.kind(), ErrorKind::NotFound, "{name}"); + } + } + #[test] fn test_build_rel_path() { let cases = vec![ diff --git a/core/services/compfs/src/backend.rs b/core/services/compfs/src/backend.rs index 7b92946891e4..97f8d770d659 100644 --- a/core/services/compfs/src/backend.rs +++ b/core/services/compfs/src/backend.rs @@ -136,7 +136,7 @@ impl Service for CompfsBackend { path: &str, _: OpCreateDir, ) -> Result { - let path = self.core.prepare_path(path)?; + let path = self.core.root_join(path)?; self.core .exec(move || async move { compio::fs::create_dir_all(path).await }) @@ -146,7 +146,7 @@ impl Service for CompfsBackend { } async fn stat(&self, _ctx: &OperationContext, path: &str, _: OpStat) -> Result { - let path = self.core.prepare_path(path)?; + let path = self.core.root_join(path)?; let meta = self .core .exec(move || async move { compio::fs::metadata(path).await }) @@ -185,8 +185,8 @@ impl Service for CompfsBackend { _opts: OpCopier, ) -> Result { let core = self.core.clone(); - let from = self.core.prepare_path(from)?; - let to = self.core.prepare_path(to)?; + let from = self.core.root_join(from)?; + let to = self.core.root_join(to)?; Ok(oio::OneShotCopier::new(async move { core.exec(move || async move { @@ -217,8 +217,8 @@ impl Service for CompfsBackend { to: &str, _: OpRename, ) -> Result { - let from = self.core.prepare_path(from)?; - let to = self.core.prepare_path(to)?; + let from = self.core.root_join(from)?; + let to = self.core.root_join(to)?; self.core .exec(move || async move { @@ -234,14 +234,14 @@ impl Service for CompfsBackend { fn read(&self, _ctx: &OperationContext, path: &str, _: OpRead) -> Result { Ok(oio::PositionReader::new(CompfsReader::new( self.core.clone(), - self.core.prepare_path(path)?, + self.core.root_join(path)?, ))) } fn write(&self, _ctx: &OperationContext, path: &str, args: OpWrite) -> Result { Ok(CompfsLazyWriter::new( self.core.clone(), - self.core.prepare_path(path)?, + self.core.root_join(path)?, args, )) } @@ -249,7 +249,7 @@ impl Service for CompfsBackend { fn list(&self, _ctx: &OperationContext, path: &str, _: OpList) -> Result { Ok(CompfsLazyLister::new( self.core.clone(), - self.core.prepare_path(path)?, + self.core.root_join(path)?, )) } diff --git a/core/services/compfs/src/core.rs b/core/services/compfs/src/core.rs index acd7239e287d..431db094df4e 100644 --- a/core/services/compfs/src/core.rs +++ b/core/services/compfs/src/core.rs @@ -16,7 +16,6 @@ // under the License. use std::future::Future; -use std::path::Path; use std::path::PathBuf; use compio::buf::{IoBuf, IoVectoredBuf}; @@ -64,23 +63,10 @@ pub(super) struct CompfsCore { } impl CompfsCore { - /// Reject `..` traversal in a key so it cannot escape `root`, matching the - /// `fs` backend (#7684). Confinement lives here because `normalize_path` - /// deliberately leaves `.`/`..` unresolved (RFC 0112). - pub fn prepare_path(&self, path: &str) -> Result { - use std::path::Component; - let trimmed = path.trim_end_matches('/'); - if Path::new(trimmed) - .components() - .any(|c| matches!(c, Component::ParentDir)) - { - return Err(Error::new( - ErrorKind::NotFound, - "path escapes the configured root via `..`", - ) - .with_context("path", path)); - } - Ok(self.root.join(trimmed)) + /// Join a path to root safely. Rejects `..` traversal beyond root. + #[inline] + pub fn root_join(&self, path: &str) -> Result { + confined_join(&self.root, path) } pub async fn exec(&self, f: Fn) -> opendal_core::Result @@ -164,10 +150,10 @@ mod tests { } #[test] - fn test_prepare_path_rejects_parent_dir() { + fn test_root_join_rejects_parent_dir() { let core = new_test_core(); for key in ["../etc/passwd", "../../etc/passwd", "a/../../b", "a/.."] { - let err = core.prepare_path(key).unwrap_err(); + let err = core.root_join(key).unwrap_err(); assert_eq!( err.kind(), ErrorKind::NotFound, @@ -177,24 +163,24 @@ mod tests { } #[test] - fn test_prepare_path_allows_normal_keys() { + fn test_root_join_allows_normal_keys() { let core = new_test_core(); // Normal keys, `.` (CurDir), and trailing slashes resolve unchanged. assert_eq!( - core.prepare_path("a/b.txt").unwrap(), + core.root_join("a/b.txt").unwrap(), PathBuf::from("/data/root/a/b.txt") ); assert_eq!( - core.prepare_path("a/b/").unwrap(), + core.root_join("a/b/").unwrap(), PathBuf::from("/data/root/a/b") ); assert_eq!( - core.prepare_path("./a/b").unwrap(), + core.root_join("./a/b").unwrap(), PathBuf::from("/data/root/a/b") ); // A key containing `..` only as a substring of a name is not a traversal. assert_eq!( - core.prepare_path("a..b").unwrap(), + core.root_join("a..b").unwrap(), PathBuf::from("/data/root/a..b") ); } diff --git a/core/services/compfs/src/deleter.rs b/core/services/compfs/src/deleter.rs index 90c0c716d108..88eae250a0a9 100644 --- a/core/services/compfs/src/deleter.rs +++ b/core/services/compfs/src/deleter.rs @@ -34,12 +34,12 @@ impl CompfsDeleter { impl oio::OneShotDelete for CompfsDeleter { async fn delete_once(&self, path: String, _: OpDelete) -> Result<()> { let res = if path.ends_with('/') { - let path = self.core.prepare_path(&path)?; + let path = self.core.root_join(&path)?; self.core .exec(move || async move { compio::fs::remove_dir(path).await }) .await } else { - let path = self.core.prepare_path(&path)?; + let path = self.core.root_join(&path)?; self.core .exec(move || async move { compio::fs::remove_file(path).await }) .await diff --git a/core/services/fs/src/core.rs b/core/services/fs/src/core.rs index e6b45f579f12..f8593eed52f1 100644 --- a/core/services/fs/src/core.rs +++ b/core/services/fs/src/core.rs @@ -33,40 +33,15 @@ pub struct FsCore { } impl FsCore { - /// Join a caller-supplied key onto a base directory while keeping the result - /// confined to that base. - /// - /// `normalize_path` (opendal-core) strips leading `/` and empty segments but - /// intentionally does NOT resolve `.`/`..`, and `PathBuf::join` is purely - /// lexical, so a key such as `../../etc/passwd` would otherwise escape the - /// configured `root` at syscall time. The fs backend documents that "all - /// operations will happen under this root", so we reject any key whose - /// components include a `..` (parent-dir) traversal. - pub fn confined_join(base: &Path, path: &str) -> Result { - use std::path::Component; - let trimmed = path.trim_end_matches('/'); - if Path::new(trimmed) - .components() - .any(|c| matches!(c, Component::ParentDir)) - { - return Err(Error::new( - ErrorKind::NotFound, - "path escapes the configured root via `..`", - ) - .with_context("path", path)); - } - Ok(base.join(trimmed)) - } - - /// Join a caller-supplied key onto `self.root`, rejecting `..` traversal. + /// Join a path to root safely. Rejects `..` traversal beyond root. #[inline] pub fn root_join(&self, path: &str) -> Result { - Self::confined_join(&self.root, path) + confined_join(&self.root, path) } // Build write path and ensure the parent dirs created pub async fn ensure_write_abs_path(&self, parent: &Path, path: &str) -> Result { - let p = Self::confined_join(parent, path)?; + let p = confined_join(parent, path)?; // Create dir before write path. // diff --git a/core/services/monoiofs/src/backend.rs b/core/services/monoiofs/src/backend.rs index 5bf9cab4d845..5da2e7f53be5 100644 --- a/core/services/monoiofs/src/backend.rs +++ b/core/services/monoiofs/src/backend.rs @@ -107,7 +107,7 @@ impl Service for MonoiofsBackend { } async fn stat(&self, _ctx: &OperationContext, path: &str, _args: OpStat) -> Result { - let path = self.core.prepare_path(path)?; + let path = self.core.root_join(path)?; let meta = self .core .dispatch(move || monoio::fs::metadata(path)) @@ -128,7 +128,7 @@ impl Service for MonoiofsBackend { Ok(RpStat::new(m)) } fn read(&self, _ctx: &OperationContext, path: &str, _args: OpRead) -> Result { - let path = self.core.prepare_path(path)?; + let path = self.core.root_join(path)?; Ok(oio::PositionReader::new(MonoiofsPositionReader::new( self.core.clone(), path, @@ -163,7 +163,7 @@ impl Service for MonoiofsBackend { to: &str, _args: OpRename, ) -> Result { - let from = self.core.prepare_path(from)?; + let from = self.core.root_join(from)?; // ensure file exists self.core .dispatch({ @@ -186,7 +186,7 @@ impl Service for MonoiofsBackend { path: &str, _args: OpCreateDir, ) -> Result { - let path = self.core.prepare_path(path)?; + let path = self.core.root_join(path)?; self.core .dispatch(move || monoio::fs::create_dir_all(path)) .await @@ -203,7 +203,7 @@ impl Service for MonoiofsBackend { _opts: OpCopier, ) -> Result { let core = self.core.clone(); - let from = self.core.prepare_path(from)?; + let from = self.core.root_join(from)?; let to = to.to_string(); let copier = oio::OneShotCopier::new(async move { diff --git a/core/services/monoiofs/src/core.rs b/core/services/monoiofs/src/core.rs index 98932fc1d1dc..f5652a64228f 100644 --- a/core/services/monoiofs/src/core.rs +++ b/core/services/monoiofs/src/core.rs @@ -16,7 +16,6 @@ // under the License. use std::mem; -use std::path::Path; use std::path::PathBuf; use std::sync::Mutex; @@ -91,28 +90,15 @@ impl MonoiofsCore { } } - /// Reject `..` traversal in a key so it cannot escape `root`, matching the - /// `fs` backend (#7684). Confinement lives here because `normalize_path` - /// deliberately leaves `.`/`..` unresolved (RFC 0112). - pub fn prepare_path(&self, path: &str) -> Result { - use std::path::Component; - let trimmed = path.trim_end_matches('/'); - if Path::new(trimmed) - .components() - .any(|c| matches!(c, Component::ParentDir)) - { - return Err(Error::new( - ErrorKind::NotFound, - "path escapes the configured root via `..`", - ) - .with_context("path", path)); - } - Ok(self.root.join(trimmed)) + /// Join a path to root safely. Rejects `..` traversal beyond root. + #[inline] + pub fn root_join(&self, path: &str) -> Result { + confined_join(&self.root, path) } /// join root and path, create parent dirs pub async fn prepare_write_path(&self, path: &str) -> Result { - let path = self.prepare_path(path)?; + let path = self.root_join(path)?; let parent = path .parent() .ok_or_else(|| { @@ -321,10 +307,10 @@ mod tests { } #[tokio::test] - async fn test_prepare_path_rejects_parent_dir() { + async fn test_root_join_rejects_parent_dir() { let core = Arc::new(MonoiofsCore::new(PathBuf::from("/data/root"), 1, 1024)); for key in ["../etc/passwd", "../../etc/passwd", "a/../../b", "a/.."] { - let err = core.prepare_path(key).unwrap_err(); + let err = core.root_join(key).unwrap_err(); assert_eq!( err.kind(), ErrorKind::NotFound, @@ -334,24 +320,24 @@ mod tests { } #[tokio::test] - async fn test_prepare_path_allows_normal_keys() { + async fn test_root_join_allows_normal_keys() { let core = Arc::new(MonoiofsCore::new(PathBuf::from("/data/root"), 1, 1024)); // Normal keys, `.` (CurDir), and trailing slashes resolve unchanged. assert_eq!( - core.prepare_path("a/b.txt").unwrap(), + core.root_join("a/b.txt").unwrap(), PathBuf::from("/data/root/a/b.txt") ); assert_eq!( - core.prepare_path("a/b/").unwrap(), + core.root_join("a/b/").unwrap(), PathBuf::from("/data/root/a/b") ); assert_eq!( - core.prepare_path("./a/b").unwrap(), + core.root_join("./a/b").unwrap(), PathBuf::from("/data/root/a/b") ); // A key containing `..` only as a substring of a name is not a traversal. assert_eq!( - core.prepare_path("a..b").unwrap(), + core.root_join("a..b").unwrap(), PathBuf::from("/data/root/a..b") ); } diff --git a/core/services/monoiofs/src/deleter.rs b/core/services/monoiofs/src/deleter.rs index a040219087ab..23f5bd6c954c 100644 --- a/core/services/monoiofs/src/deleter.rs +++ b/core/services/monoiofs/src/deleter.rs @@ -34,7 +34,7 @@ impl MonoiofsDeleter { impl oio::OneShotDelete for MonoiofsDeleter { async fn delete_once(&self, path: String, _: OpDelete) -> Result<()> { - let path = self.core.prepare_path(&path)?; + let path = self.core.root_join(&path)?; let meta = self .core .dispatch({ From a1741c625b675c04e40f1a6ea3b9079242ed2c2a Mon Sep 17 00:00:00 2001 From: Erick Guan <297343+erickguan@users.noreply.github.com> Date: Sat, 20 Jun 2026 14:14:45 +0800 Subject: [PATCH 2/3] merge tests --- core/core/src/raw/path.rs | 4 ++++ core/services/compfs/src/core.rs | 36 ------------------------------ core/services/monoiofs/src/core.rs | 36 ------------------------------ 3 files changed, 4 insertions(+), 72 deletions(-) diff --git a/core/core/src/raw/path.rs b/core/core/src/raw/path.rs index dfe433d1277d..dc7fed56e18d 100644 --- a/core/core/src/raw/path.rs +++ b/core/core/src/raw/path.rs @@ -421,6 +421,10 @@ mod tests { ("input nested file", "abc/def", root.join("abc/def")), ("input dir", "def/", root.join("def")), ("input nested dir", "abc/def/", root.join("abc/def")), + + (". in path", "dir/file.txt", root.join("dir/file.txt")), + (". in path with dir", "dir/./file.txt", root.join("dir/file.txt")), + (".. in path", "a..b", root.join("a..b")), ]; for (name, input, expect) in cases { diff --git a/core/services/compfs/src/core.rs b/core/services/compfs/src/core.rs index 431db094df4e..b2d8323409c7 100644 --- a/core/services/compfs/src/core.rs +++ b/core/services/compfs/src/core.rs @@ -148,40 +148,4 @@ mod tests { buf_pool: oio::PooledBuf::new(16), } } - - #[test] - fn test_root_join_rejects_parent_dir() { - let core = new_test_core(); - for key in ["../etc/passwd", "../../etc/passwd", "a/../../b", "a/.."] { - let err = core.root_join(key).unwrap_err(); - assert_eq!( - err.kind(), - ErrorKind::NotFound, - "key should be rejected: {key}" - ); - } - } - - #[test] - fn test_root_join_allows_normal_keys() { - let core = new_test_core(); - // Normal keys, `.` (CurDir), and trailing slashes resolve unchanged. - assert_eq!( - core.root_join("a/b.txt").unwrap(), - PathBuf::from("/data/root/a/b.txt") - ); - assert_eq!( - core.root_join("a/b/").unwrap(), - PathBuf::from("/data/root/a/b") - ); - assert_eq!( - core.root_join("./a/b").unwrap(), - PathBuf::from("/data/root/a/b") - ); - // A key containing `..` only as a substring of a name is not a traversal. - assert_eq!( - core.root_join("a..b").unwrap(), - PathBuf::from("/data/root/a..b") - ); - } } diff --git a/core/services/monoiofs/src/core.rs b/core/services/monoiofs/src/core.rs index f5652a64228f..3f2f73507b67 100644 --- a/core/services/monoiofs/src/core.rs +++ b/core/services/monoiofs/src/core.rs @@ -305,40 +305,4 @@ mod tests { let core = new_core(4); dispatch_panic(core).await; } - - #[tokio::test] - async fn test_root_join_rejects_parent_dir() { - let core = Arc::new(MonoiofsCore::new(PathBuf::from("/data/root"), 1, 1024)); - for key in ["../etc/passwd", "../../etc/passwd", "a/../../b", "a/.."] { - let err = core.root_join(key).unwrap_err(); - assert_eq!( - err.kind(), - ErrorKind::NotFound, - "key should be rejected: {key}" - ); - } - } - - #[tokio::test] - async fn test_root_join_allows_normal_keys() { - let core = Arc::new(MonoiofsCore::new(PathBuf::from("/data/root"), 1, 1024)); - // Normal keys, `.` (CurDir), and trailing slashes resolve unchanged. - assert_eq!( - core.root_join("a/b.txt").unwrap(), - PathBuf::from("/data/root/a/b.txt") - ); - assert_eq!( - core.root_join("a/b/").unwrap(), - PathBuf::from("/data/root/a/b") - ); - assert_eq!( - core.root_join("./a/b").unwrap(), - PathBuf::from("/data/root/a/b") - ); - // A key containing `..` only as a substring of a name is not a traversal. - assert_eq!( - core.root_join("a..b").unwrap(), - PathBuf::from("/data/root/a..b") - ); - } } From b3373b3f2bcaa49f633ba397859bddb5b534711f Mon Sep 17 00:00:00 2001 From: Erick Guan <297343+erickguan@users.noreply.github.com> Date: Sat, 20 Jun 2026 14:16:07 +0800 Subject: [PATCH 3/3] fix style --- core/core/src/raw/path.rs | 7 +++++-- core/services/compfs/src/core.rs | 10 ---------- 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/core/core/src/raw/path.rs b/core/core/src/raw/path.rs index dc7fed56e18d..1481550d48fa 100644 --- a/core/core/src/raw/path.rs +++ b/core/core/src/raw/path.rs @@ -421,9 +421,12 @@ mod tests { ("input nested file", "abc/def", root.join("abc/def")), ("input dir", "def/", root.join("def")), ("input nested dir", "abc/def/", root.join("abc/def")), - (". in path", "dir/file.txt", root.join("dir/file.txt")), - (". in path with dir", "dir/./file.txt", root.join("dir/file.txt")), + ( + ". in path with dir", + "dir/./file.txt", + root.join("dir/file.txt"), + ), (".. in path", "a..b", root.join("a..b")), ]; diff --git a/core/services/compfs/src/core.rs b/core/services/compfs/src/core.rs index b2d8323409c7..01ed8053c5ba 100644 --- a/core/services/compfs/src/core.rs +++ b/core/services/compfs/src/core.rs @@ -138,14 +138,4 @@ mod tests { assert_eq!(buf.total_len(), len); assert_eq!(collected, bytes); } - - fn new_test_core() -> CompfsCore { - CompfsCore { - info: ServiceInfo::new("compfs", "", ""), - capability: Capability::default(), - root: PathBuf::from("/data/root"), - dispatcher: Dispatcher::new().unwrap(), - buf_pool: oio::PooledBuf::new(16), - } - } }