🔒 Fix path traversal in FileSystemContext::write_content#130
Open
bashandbone wants to merge 1 commit intomainfrom
Open
🔒 Fix path traversal in FileSystemContext::write_content#130bashandbone wants to merge 1 commit intomainfrom
bashandbone wants to merge 1 commit intomainfrom
Conversation
Implement lexical path validation in `FileSystemContext` to prevent unauthorized file access and overwrites outside the base directory. Add unit tests to verify the fix against absolute paths and traversal attacks. Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
Contributor
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Contributor
Reviewer's GuideAdds lexical path validation to FileSystemContext to prevent path traversal and updates read/write operations and tests accordingly. Sequence diagram for write_content with secure_path validationsequenceDiagram
participant Caller
participant FileSystemContext
participant secure_path
participant FileSystem
Caller->>FileSystemContext: write_content(destination, content)
FileSystemContext->>secure_path: secure_path(destination)
alt path is invalid
secure_path-->>FileSystemContext: Err(ServiceError)
FileSystemContext-->>Caller: Err(ServiceError)
else path is valid
secure_path-->>FileSystemContext: Ok(valid_path)
FileSystemContext->>FileSystem: create_dir_all(parent_of(valid_path))
FileSystem-->>FileSystemContext: Result<(), io::Error>
FileSystemContext->>FileSystem: write(valid_path, content)
FileSystem-->>FileSystemContext: Result<(), io::Error>
FileSystemContext-->>Caller: Ok(())
end
Class diagram for updated FileSystemContext with secure path validationclassDiagram
class FileSystemContext {
- base_path : PathBuf
+ new(base_path : &Path) FileSystemContext
- secure_path(source : &str) Result<PathBuf, ServiceError>
+ read_content(source : &str) Result<String, ServiceError>
+ write_content(destination : &str, content : &str) Result<(), ServiceError>
}
class ExecutionContext {
<<interface>> ExecutionContext
+ read_content(source : &str) Result<String, ServiceError>
+ write_content(destination : &str, content : &str) Result<(), ServiceError>
}
FileSystemContext ..|> ExecutionContext
class ServiceError {
+ execution_dynamic(message : String) ServiceError
}
Flow diagram for secure_path lexical validation logicflowchart TD
A["start secure_path(source)"] --> B["parse source into path components"]
B --> C["set depth = 0"]
C --> D{"next component?"}
D -->|no| Z["return base_path.join(source)"]
D -->|yes| E{"component type"}
E -->|Prefix or RootDir| F["return Err(ServiceError: absolute paths not allowed)"]
E -->|CurDir| D
E -->|ParentDir| G{"depth == 0?"}
G -->|yes| H["return Err(ServiceError: traversal outside base path)"]
G -->|no| I["depth = depth - 1"]
I --> D
E -->|Normal| J["depth = depth + 1"]
J --> D
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
secure_path, you validate components but then join the originalsourcestring instead of a normalized/validatedPathBuf; consider reconstructing the path from the iterated components to avoid any divergence between what was validated and what is ultimately used. - The current implementation allows an empty string as
source/destination(resulting inbase_pathitself); if this is not intended, add an explicit check insecure_pathto reject or handle empty paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `secure_path`, you validate components but then join the original `source` string instead of a normalized/validated `PathBuf`; consider reconstructing the path from the iterated components to avoid any divergence between what was validated and what is ultimately used.
- The current implementation allows an empty string as `source`/`destination` (resulting in `base_path` itself); if this is not intended, add an explicit check in `secure_path` to reject or handle empty paths.
## Individual Comments
### Comment 1
<location path="crates/services/src/lib.rs" line_range="151-160" />
<code_context>
+ /// Lexically validate path to prevent traversal attacks
</code_context>
<issue_to_address>
**🚨 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.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+151
to
+160
| /// 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 => { |
Contributor
There was a problem hiding this comment.
🚨 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.
Contributor
There was a problem hiding this comment.
Pull request overview
Mitigates a reported path traversal issue in the FileSystemContext execution context by adding path validation before reading/writing files under a configured base directory.
Changes:
- Added
FileSystemContext::secure_pathto reject absolute paths and lexical..traversal. - Refactored
read_content/write_contentto usesecure_path. - Added unit tests for valid paths, absolute paths, and common traversal patterns.
Comments suppressed due to low confidence (1)
crates/services/src/lib.rs:195
write_contentcallscreate_dir_all(parent)on the path returned bysecure_path. If any path component underbase_pathis a symlink, this can still create directories and write files outsidebase_path(symlink traversal). If you keepsecure_pathlexical, add a filesystem-level guard here (e.g., validate the parent path is within canonicalized base and that no intermediate component is a symlink) before creating dirs/writing.
let path = self.secure_path(destination)?;
if let Some(parent) = path.parent() {
std::fs::create_dir_all(parent)?;
}
Ok(std::fs::write(path, content)?)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+151
to
+180
| /// 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 => { | ||
| 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)) |
There was a problem hiding this comment.
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.
| 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()); | ||
| } |
There was a problem hiding this comment.
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" | |
| ); | |
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎯 What: A path traversal vulnerability was identified in
FileSystemContext::write_contentandFileSystemContext::read_content. Thedestination/sourceparameters were joined tobase_pathwithout validation, allowing access to arbitrary files on the system using ".." or absolute paths.🛡️ Solution: Implemented a
secure_pathhelper method that lexically validates input paths by:read_contentandwrite_contentto use this validation.PR created automatically by Jules for task 11773844134454313898 started by @bashandbone
Summary by Sourcery
Harden file reading and writing in FileSystemContext to prevent path traversal outside the configured base directory.
Bug Fixes:
Enhancements:
Tests: