🛡️ Sentinel: [CRITICAL] Fix Path Traversal in FileSystemContext::new#193
Open
bashandbone wants to merge 1 commit intomainfrom
Open
🛡️ Sentinel: [CRITICAL] Fix Path Traversal in FileSystemContext::new#193bashandbone wants to merge 1 commit intomainfrom
bashandbone wants to merge 1 commit intomainfrom
Conversation
🚨 Severity: CRITICAL 💡 Vulnerability: `FileSystemContext::new` previously stored `base_path` without canonicalizing it. Path traversal validation logic (`FileSystemContext::secure_path`) assumes that the base_path is absolute and fully resolved, relying on a naive `starts_with` bounds check. This allows path traversal attacks if symlinks and `..` references are cleverly manipulated against the uncanonicalized base path, bypassing the lexical checks before symlink validation can catch them. 🎯 Impact: Attackers could potentially read or overwrite sensitive files outside of the intended sandbox directory, compromising the system or leaking secrets. 🔧 Fix: Updated `FileSystemContext::new` to eagerly resolve and canonicalize the `base_path` upon construction, returning a `ServiceResult`. ✅ Verification: Tested the new logic via `cargo test -p thread-services` and ensured the `secure_path` checks correctly block traversal with fully canonicalized base bounds. 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 guide (collapsed on small PRs)Reviewer's GuideCanonicalizes FileSystemContext base paths at construction time and propagates errors, tightening path traversal protections and updating tests and internal docs accordingly. Sequence diagram for canonicalized FileSystemContext::new and secure_pathsequenceDiagram
actor Caller
participant FileSystemContext
participant OS
Caller->>FileSystemContext: new(base_path)
FileSystemContext->>OS: canonicalize(base_path)
alt canonicalization_success
OS-->>FileSystemContext: canonical_base_path
FileSystemContext-->>Caller: Ok(FileSystemContext{ base_path: canonical_base_path })
Caller->>FileSystemContext: secure_path(relative_path)
FileSystemContext->>FileSystemContext: join(base_path, relative_path)
FileSystemContext->>FileSystemContext: validate_starts_with(base_path)
FileSystemContext->>OS: canonicalize(joined_path)
OS-->>FileSystemContext: resolved_path
FileSystemContext-->>Caller: Ok(resolved_path)
else canonicalization_failure
OS-->>FileSystemContext: error
FileSystemContext->>FileSystemContext: ServiceError::execution_dynamic(message)
FileSystemContext-->>Caller: Err(ServiceError)
end
Class diagram for updated FileSystemContext constructionclassDiagram
class FileSystemContext {
+PathBuf base_path
+new(base_path: P) ServiceResult_FileSystemContext
+secure_path(relative_path: &str) ServiceResult_PathBuf
}
class ServiceResult_FileSystemContext {
+ok(value: FileSystemContext)
+err(error: ServiceError)
}
class ServiceResult_PathBuf {
+ok(value: PathBuf)
+err(error: ServiceError)
}
class ServiceError {
+execution_dynamic(message: String) ServiceError
}
class PathBuf
class Path
FileSystemContext --> PathBuf : base_path
FileSystemContext ..> ServiceResult_FileSystemContext : returns
FileSystemContext ..> ServiceResult_PathBuf : returns
ServiceResult_FileSystemContext --> ServiceError : error
ServiceResult_PathBuf --> ServiceError : error
FileSystemContext ..> Path : uses
FileSystemContext ..> ServiceError : on_invalid_base_path
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
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="crates/services/src/lib.rs" line_range="146-149" />
<code_context>
+ let canonical_base = base_path
+ .as_ref()
+ .canonicalize()
+ .map_err(|e| ServiceError::execution_dynamic(format!("Invalid base path: {}", e)))?;
+
+ Ok(Self {
</code_context>
<issue_to_address>
**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.
```suggestion
let canonical_base = base_path
.as_ref()
.canonicalize()
.map_err(|e| {
ServiceError::execution_dynamic(format!(
"Invalid base path '{}': {}",
base_path.as_ref().display(),
e
))
})?;
```
</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
+146
to
+149
| let canonical_base = base_path | ||
| .as_ref() | ||
| .canonicalize() | ||
| .map_err(|e| ServiceError::execution_dynamic(format!("Invalid base path: {}", e)))?; |
Contributor
There was a problem hiding this comment.
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 | |
| )) | |
| })?; |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the thread-services filesystem execution context by ensuring FileSystemContext is constructed with a fully resolved, canonical base directory, preventing path traversal/symlink-escape bypasses that rely on an uncanonicalized base.
Changes:
- Change
FileSystemContext::newto canonicalizebase_pathat construction time and return aServiceResult<Self>. - Update call sites/tests to handle the new fallible constructor (
.unwrap()in tests). - Add a Sentinel note documenting the vulnerability and the prevention strategy.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| crates/services/src/lib.rs | Make FileSystemContext::new canonicalize the base path and return ServiceResult; adjust unit test call site. |
| crates/thread/tests/integration.rs | Update integration test to unwrap the now-fallible constructor. |
| .jules/sentinel.md | Document the path traversal issue and mitigation as an internal security learning note. |
đź’ˇ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .as_ref() | ||
| .canonicalize() | ||
| .map_err(|e| ServiceError::execution_dynamic(format!("Invalid base path: {}", e)))?; | ||
|
|
| Self { | ||
| base_path: base_path.as_ref().to_path_buf(), | ||
| } | ||
| pub fn new<P: AsRef<Path>>(base_path: P) -> ServiceResult<Self> { |
Comment on lines
+145
to
+149
| 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
| let canonical_base = base_path | ||
| .as_ref() | ||
| .canonicalize() | ||
| .map_err(|e| ServiceError::execution_dynamic(format!("Invalid base path: {}", e)))?; |
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.
🚨 Severity: CRITICAL
đź’ˇ Vulnerability:
FileSystemContext::newpreviously storedbase_pathwithout canonicalizing it. Path traversal validation logic (FileSystemContext::secure_path) assumes that the base_path is absolute and fully resolved, relying on a naivestarts_withbounds check. This allows path traversal attacks if symlinks and..references are cleverly manipulated against the uncanonicalized base path, bypassing the lexical checks before symlink validation can catch them.🎯 Impact: Attackers could potentially read or overwrite sensitive files outside of the intended sandbox directory, compromising the system or leaking secrets.
🔧 Fix: Updated
FileSystemContext::newto eagerly resolve and canonicalize thebase_pathupon construction, returning aServiceResult.âś… Verification: Tested the new logic via
cargo test -p thread-servicesand ensured thesecure_pathchecks correctly block traversal with fully canonicalized base bounds.PR created automatically by Jules for task 4933271394501377337 started by @bashandbone
Summary by Sourcery
Canonicalize FileSystemContext base paths to close a path traversal vulnerability and update callers and security notes accordingly.
Bug Fixes:
Documentation:
Tests: