Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .jules/sentinel.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
## 2026-05-04 - Path Traversal bypass via uncanonicalized base path in FileSystemContext::new
**Vulnerability:** The `FileSystemContext::new` method previously accepted a `base_path` and stored it as-is without canonicalization. Later on, `FileSystemContext::secure_path` validated requested paths by joining them with the `base_path` and checking that it `starts_with` the `base_path`, then canonicalizing the base path _just_ to check symlink escapes. If the `base_path` wasn't fully resolved initially, clever manipulation of paths with `..` relative to uncanonicalized base paths combined with symlinks could slip past the starts_with lexical check before the physical check triggered correctly.
**Learning:** An uncanonicalized base directory breaks lexical boundary checks. When checking bounds (`starts_with`) on a potentially dangerous path relative to a base path, the base path itself must be canonicalized _before_ the context is ever used to ensure uniform comparison.
**Prevention:** Canonicalize the absolute base directory upfront during object construction and fail immediately if it cannot be resolved.
15 changes: 10 additions & 5 deletions crates/services/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,10 +142,15 @@ pub struct FileSystemContext {
}

impl FileSystemContext {
pub fn new<P: AsRef<Path>>(base_path: P) -> Self {
Self {
base_path: base_path.as_ref().to_path_buf(),
}
pub fn new<P: AsRef<Path>>(base_path: P) -> ServiceResult<Self> {
let canonical_base = base_path
.as_ref()
.canonicalize()
.map_err(|e| ServiceError::execution_dynamic(format!("Invalid base path: {}", e)))?;
Comment on lines +146 to +149
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Including the offending path in the error message would make debugging invalid base paths easier.

Right now the message only includes the IO error (e) and not which base_path failed. Including the path itself (e.g. format!("Invalid base path '{}': {}", base_path.as_ref().display(), e)) will make logs much clearer, especially when multiple contexts are constructed.

Suggested change
let canonical_base = base_path
.as_ref()
.canonicalize()
.map_err(|e| ServiceError::execution_dynamic(format!("Invalid base path: {}", e)))?;
let canonical_base = base_path
.as_ref()
.canonicalize()
.map_err(|e| {
ServiceError::execution_dynamic(format!(
"Invalid base path '{}': {}",
base_path.as_ref().display(),
e
))
})?;

Comment on lines +145 to +149
Comment on lines +146 to +149

Ok(Self {
base_path: canonical_base,
})
}

/// Lexically validate path to prevent traversal attacks and symlink escapes
Expand Down Expand Up @@ -310,7 +315,7 @@ mod tests {
#[test]
fn test_file_system_context_security() {
let temp = std::env::temp_dir();
let ctx = FileSystemContext::new(&temp);
let ctx = FileSystemContext::new(&temp).unwrap();

// Valid paths
assert!(ctx.secure_path("test.txt").is_ok());
Expand Down
2 changes: 1 addition & 1 deletion crates/thread/tests/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,5 @@ fn test_service_reexports_work() {
use thread::services::FileSystemContext;

// Just check if we can use the types
let _ctx = FileSystemContext::new(".");
let _ctx = FileSystemContext::new(".").unwrap();
}
Loading