Skip to content
Open
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
58 changes: 56 additions & 2 deletions crates/services/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -147,16 +147,48 @@ impl FileSystemContext {
base_path: base_path.as_ref().to_path_buf(),
}
}

/// Lexically validate path to prevent traversal attacks
fn secure_path(&self, source: &str) -> Result<std::path::PathBuf, ServiceError> {
use std::path::Component;

let path = Path::new(source);
let mut depth = 0;

for component in path.components() {
match component {
Component::Prefix(_) | Component::RootDir => {
Comment on lines +151 to +160
Copy link
Contributor

Choose a reason for hiding this comment

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

🚨 issue (security): Lexical validation still allows escapes via symlinks inside the base directory.

The lexical depth check only blocks .. segments; it doesn’t stop escapes via symlinks under base_path. For instance, if base_path/sub points to /etc, then source = "sub/passwd" passes but resolves to /etc/passwd.

If untrusted content can exist under base_path, you should resolve the final path (e.g. with canonicalize) and confirm it still resides under a canonicalized base_path before using it. That closes the symlink escape while retaining the lexical checks.

return Err(ServiceError::execution_dynamic(format!(
"Absolute paths are not allowed: {source}"
)));
}
Component::CurDir => {}
Component::ParentDir => {
if depth == 0 {
return Err(ServiceError::execution_dynamic(format!(
"Path traversal outside of base path is not allowed: {source}"
)));
}
depth -= 1;
}
Component::Normal(_) => {
depth += 1;
}
}
}

Ok(self.base_path.join(source))
Comment on lines +151 to +180
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

secure_path only performs lexical ../absolute checks. It still allows escaping base_path via symlinks inside the base (e.g., base/link -> / then secure_path("link/etc/passwd") passes and read_to_string reads /etc/passwd). Consider resolving/validating against base_path on the filesystem (canonicalize + starts_with for reads, and for writes ensure each existing path segment isn’t a symlink / use no-follow semantics) before returning the final path.

Copilot uses AI. Check for mistakes.
}
}

impl ExecutionContext for FileSystemContext {
fn read_content(&self, source: &str) -> Result<String, ServiceError> {
let path = self.base_path.join(source);
let path = self.secure_path(source)?;
Ok(std::fs::read_to_string(path)?)
}

fn write_content(&self, destination: &str, content: &str) -> Result<(), ServiceError> {
let path = self.base_path.join(destination);
let path = self.secure_path(destination)?;
if let Some(parent) = path.parent() {
std::fs::create_dir_all(parent)?;
}
Expand Down Expand Up @@ -235,4 +267,26 @@ mod tests {
let sources = ctx.list_sources().unwrap();
assert_eq!(sources, vec!["test.rs"]);
}

#[test]
fn test_file_system_context_security() {
let temp = std::env::temp_dir();
let ctx = FileSystemContext::new(&temp);

// Valid paths
assert!(ctx.secure_path("test.txt").is_ok());
assert!(ctx.secure_path("dir/test.txt").is_ok());
assert!(ctx.secure_path("./test.txt").is_ok());
assert!(ctx.secure_path("dir/../test.txt").is_ok());

// Absolute paths
assert!(ctx.secure_path("/etc/passwd").is_err());
#[cfg(windows)]
assert!(ctx.secure_path("C:\\Windows\\System32\\cmd.exe").is_err());

// Traversal attacks
assert!(ctx.secure_path("../test.txt").is_err());
assert!(ctx.secure_path("dir/../../test.txt").is_err());
assert!(ctx.secure_path("dir/../inc/../../test.txt").is_err());
}
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

The new tests cover lexical .. and absolute paths, but don’t cover symlink-based traversal (e.g., link -> /) or assert the higher-level read_content/write_content behavior. Adding a #[cfg(unix)] test that creates a temp dir, a symlink inside it, and verifies reads/writes via that symlink are rejected would prevent regressions of the remaining traversal vector.

Suggested change
}
}
#[cfg(unix)]
#[test]
fn test_file_system_context_symlink_traversal() {
use std::fs;
use std::os::unix::fs as unix_fs;
// Create a temporary root directory for the FileSystemContext.
let root_dir = tempfile::tempdir().expect("failed to create temp root dir");
// Create a separate directory outside the root with a file in it.
let external_dir = tempfile::tempdir().expect("failed to create external temp dir");
let external_file_path = external_dir.path().join("secret.txt");
fs::write(&external_file_path, "top secret").expect("failed to write external file");
// Inside the root, create a symlink pointing to the external directory.
let link_path = root_dir.path().join("link");
unix_fs::symlink(external_dir.path(), &link_path)
.expect("failed to create symlink to external dir");
// Initialize the context rooted at the temporary directory.
let ctx = FileSystemContext::new(root_dir.path());
// Attempt to read a file via the symlink; this should be rejected.
let read_result = ctx.read_content("link/secret.txt");
assert!(
read_result.is_err(),
"reading via symlink that escapes root should be rejected"
);
// Attempt to write a file via the symlink; this should also be rejected.
let write_result = ctx.write_content("link/secret.txt", "hacked");
assert!(
write_result.is_err(),
"writing via symlink that escapes root should be rejected"
);
}

Copilot uses AI. Check for mistakes.
}
Loading