🛡️ Sentinel: [improvement] Pre-canonicalize base path in FileSystemContext to avoid symlink escapes#245
Conversation
The `FileSystemContext` accepted an uncanonicalized `base_path` in its constructor. This could potentially allow symlink escapes during path validation inside `secure_path`. By changing the constructor to return a `ServiceResult<Self>` and canonicalizing the base path on creation, we ensure the base sandbox path is absolute and free of symlinks before any further validation takes place. Downstream consumers such as tests have been updated to handle the `Result`. Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
|
👋 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. |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideCanonicalizes the FileSystemContext base path at construction time, changing its constructor to return a ServiceResult and updating tests and sentinel documentation accordingly to harden against symlink-based path traversal. 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}
There was a problem hiding this comment.
Hey - I've found 2 issues
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="145-146" />
<code_context>
- 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()?;
+ Ok(Self {
+ base_path: canonical_base,
</code_context>
<issue_to_address>
**issue:** Canonicalizing the base path in `new` may reject otherwise valid configurations where the directory is created later.
This constructor now requires the directory to pre-exist, so callers can’t create a context for a path that will be created later. If that scenario should remain supported, consider either delaying canonicalization until first use, or eagerly creating the directory (e.g., via `create_dir_all`) before canonicalizing, depending on the intended invariants.
</issue_to_address>
### Comment 2
<location path="crates/services/src/lib.rs" line_range="146" />
<code_context>
- 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()?;
+ Ok(Self {
+ base_path: canonical_base,
</code_context>
<issue_to_address>
**suggestion:** Consider enriching the error from `canonicalize()` with path context for easier debugging and observability.
Using `?` here returns the bare `canonicalize` error, which can be hard to trace. Consider wrapping it (e.g. via a custom error variant or `map_err` that includes the `base_path`) so callers can see which base path failed when constructing different `FileSystemContext` roots.
Suggested implementation:
```rust
pub fn new<P: AsRef<Path>>(base_path: P) -> ServiceResult<Self> {
let canonical_base = base_path
.as_ref()
.canonicalize()
.map_err(|e| {
std::io::Error::new(
e.kind(),
format!(
"failed to canonicalize base path '{}': {}",
base_path.as_ref().display(),
e
),
)
})?;
Ok(Self {
base_path: canonical_base,
})
}
```
If `ServiceResult` does not currently accept `std::io::Error` via `From<std::io::Error>`, you will need to:
1. Ensure the error type used by `ServiceResult` implements `From<std::io::Error>` (or adjust the `map_err` to construct the appropriate service-specific error variant instead of `std::io::Error::new`).
2. Optionally, if you already use a custom error type with richer context (e.g. via `thiserror` or `anyhow::Context`), replace the `std::io::Error::new(...)` construction with your existing pattern while keeping the `base_path.as_ref().display()` information in the message.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| pub fn new<P: AsRef<Path>>(base_path: P) -> ServiceResult<Self> { | ||
| let canonical_base = base_path.as_ref().canonicalize()?; |
There was a problem hiding this comment.
issue: Canonicalizing the base path in new may reject otherwise valid configurations where the directory is created later.
This constructor now requires the directory to pre-exist, so callers can’t create a context for a path that will be created later. If that scenario should remain supported, consider either delaying canonicalization until first use, or eagerly creating the directory (e.g., via create_dir_all) before canonicalizing, depending on the intended invariants.
| 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()?; |
There was a problem hiding this comment.
suggestion: Consider enriching the error from canonicalize() with path context for easier debugging and observability.
Using ? here returns the bare canonicalize error, which can be hard to trace. Consider wrapping it (e.g. via a custom error variant or map_err that includes the base_path) so callers can see which base path failed when constructing different FileSystemContext roots.
Suggested implementation:
pub fn new<P: AsRef<Path>>(base_path: P) -> ServiceResult<Self> {
let canonical_base = base_path
.as_ref()
.canonicalize()
.map_err(|e| {
std::io::Error::new(
e.kind(),
format!(
"failed to canonicalize base path '{}': {}",
base_path.as_ref().display(),
e
),
)
})?;
Ok(Self {
base_path: canonical_base,
})
}If ServiceResult does not currently accept std::io::Error via From<std::io::Error>, you will need to:
- Ensure the error type used by
ServiceResultimplementsFrom<std::io::Error>(or adjust themap_errto construct the appropriate service-specific error variant instead ofstd::io::Error::new). - Optionally, if you already use a custom error type with richer context (e.g. via
thiserrororanyhow::Context), replace thestd::io::Error::new(...)construction with your existing pattern while keeping thebase_path.as_ref().display()information in the message.
There was a problem hiding this comment.
Pull request overview
This PR hardens the services-layer filesystem sandboxing by ensuring FileSystemContext is constructed with a canonicalized (absolute, symlink-resolved) base path, reducing the risk of symlink-based sandbox escapes during subsequent path validation.
Changes:
- Make
FileSystemContext::newfallible (ServiceResult<Self>) and canonicalizebase_pathat construction time. - Update affected tests to handle the new
Result-returning constructor. - Add a Sentinel learning note describing the vulnerability and prevention guidance.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
crates/services/src/lib.rs |
Canonicalizes filesystem sandbox base path during FileSystemContext construction and updates the related unit test call site. |
crates/thread/tests/integration.rs |
Updates integration test to unwrap the new fallible FileSystemContext::new API. |
.jules/sentinel.md |
Adds a short security learning note documenting the change and rationale. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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()?; |
| ## 2024-05-20 - Pre-canonicalize base path in FileSystemContext to avoid symlink escapes | ||
| **Vulnerability:** The `FileSystemContext::new` accepted an uncanonicalized base path, which could potentially lead to symlink escapes during path validation. | ||
| **Learning:** By canonicalizing the base path during construction, the check inside `secure_path` accurately determines if the target escapes the expected path, which wasn't fully guaranteed with an uncanonicalized base path. | ||
| **Prevention:** Always construct secure contexts with canonicalized paths when implementing file path validation checks, especially to prevent symlink traversal. |
🚨 Severity: MEDIUM
💡 Vulnerability:
FileSystemContext::newpreviously accepted an uncanonicalized base path. When validating paths against a sandbox, an uncanonicalized base path can lead to symlink traversal bypasses if the base path itself contains symlinks that resolve unexpectedly compared to the target path.🎯 Impact: Potential path traversal or symlink escapes during file system sandbox validation in the service layer, although practically limited by whether the base path is user-controlled.
🔧 Fix: Changed
FileSystemContext::newto return aServiceResult<Self>and explicitlycanonicalize()the base path at creation time, guaranteeing an absolute, symlink-free sandbox root for all subsequentsecure_pathchecks. Updated usage in tests to handle theResult.✅ Verification: Verified by running the existing
test_file_system_context_securitysuite and unit tests (cargo test -p thread-services), ensuring all security checks and standard usages still operate as intended.PR created automatically by Jules for task 17483386213109417647 started by @bashandbone
Summary by Sourcery
Canonicalize the base path used by FileSystemContext at construction time to harden sandboxed path validation against symlink-based escapes.
Bug Fixes:
Enhancements:
Documentation: