fix(security): prevent path traversal in NestedGlob search root#407
Conversation
Hardens the NestedGlob sync type in `src/linker.rs` by validating the
search root (`source` field) against the project root. This prevents a
malicious configuration from triggering a recursive directory walk
outside the intended project boundaries.
Validation is implemented using `revalidate_destination_path` which
correctly handles current directory ('.') search roots while rejecting
traversal components or absolute paths that escape the project root.
Security regression tests are added in `tests/test_security.rs` to
verify the fix.
Under 50 lines changed.
|
👋 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. |
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change revalidates NestedGlob computed search_root paths with the destination safety check during target processing and cleanup, and adds a test that verifies relative and absolute outside-root search_roots are rejected and do not create or remove files outside the project root. ChangesPath Traversal Prevention for NestedGlob Targets
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/linker.rs (1)
384-401: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueNestedGlob search_root validation looks correct.
revalidate_destination_pathresolves symlinks and..via canonicalization on the first existing ancestor and asserts the result is undercanonical_project_root, which correctly rejects:
- relative traversal (e.g.
source = "../outside"),- absolute escapes (e.g.
source = "/etc", sincePath::joinof an absolute replaces the base), and- symlinked roots that resolve outside the project.
It still allows
source = "."because canonicalizingproject_root/.yields the project root itself. The error path here propagates via?intoLinker::sync, which incrementsresult.errors— exactly what the new regression test intests/test_security.rsasserts.One minor wart: the error surfaced will read
Destination path resolves outside project root: …even though the offending value is the NestedGlobsource(not a destination). Consider wrapping the error with context that names the field, e.g.:♻️ Optional clarity tweak
- let search_root = self.project_root.join(&target.source); - // SECURITY: Validate search root to prevent traversal/absolute escapes. - self.revalidate_destination_path(&search_root)?; + let search_root = self.project_root.join(&target.source); + // SECURITY: Validate search root to prevent traversal/absolute escapes. + self.revalidate_destination_path(&search_root) + .with_context(|| { + format!( + "NestedGlob source resolves outside project root: {}", + target.source + ) + })?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/linker.rs` around lines 384 - 401, The validation call for NestedGlob uses self.revalidate_destination_path(&search_root)? which returns an error message mentioning "Destination path..." even though the offending input is the NestedGlob `source`; update the call to attach contextual information naming the field so the propagated error clearly indicates the NestedGlob source is invalid (e.g., replace the bare ?-propagation with adding context via .with_context or map_err to include target.source/display), keeping the same failure semantics and still returning the original error wrapped with a message like "NestedGlob source '<…>' failed validation".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_security.rs`:
- Around line 1-47: Add two extra assertions/cases to the existing
test_nested_glob_search_root_traversal: (1) add a run that uses an absolute
source (use outside_dir.display() value in the TOML) to ensure absolute-path
escapes are blocked when creating the config and calling linker.sync(&options);
(2) call linker.clean(&SyncOptions::default()) and assert result.removed == 0 to
ensure malicious config cannot drive clean to touch outside files. Also
strengthen the final filesystem assertion by asserting the entire leaked
directory was not created (e.g., assert !project_root.join("leaked").exists())
in addition to checking the specific leaked AGENTS.md; reference the test
function test_nested_glob_search_root_traversal, the Linker struct and its
methods sync and clean, and SyncOptions to locate where to add these checks.
---
Outside diff comments:
In `@src/linker.rs`:
- Around line 384-401: The validation call for NestedGlob uses
self.revalidate_destination_path(&search_root)? which returns an error message
mentioning "Destination path..." even though the offending input is the
NestedGlob `source`; update the call to attach contextual information naming the
field so the propagated error clearly indicates the NestedGlob source is invalid
(e.g., replace the bare ?-propagation with adding context via .with_context or
map_err to include target.source/display), keeping the same failure semantics
and still returning the original error wrapped with a message like "NestedGlob
source '<…>' failed validation".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 62f65b33-c3d1-4a27-9f71-015181ee51e7
📒 Files selected for processing (2)
src/linker.rstests/test_security.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_security.rs`:
- Around line 64-80: The test builds a TOML string using outside_dir.display()
which on Windows produces backslashes that become invalid TOML escape sequences
when placed in a basic string; update the test in tests/test_security.rs so the
absolute-path case uses a TOML literal string or properly escapes backslashes
before calling Config::load: modify the absolute_toml construction (referenced
by absolute_toml, Config::load, Linker::new, absolute_linker, absolute_result)
to wrap the interpolated path in single quotes (TOML literal) or replace
backslashes with doubled backslashes (e.g. path_str.replace("\\","\\\\")).
Alternatively, if the test must remain Unix-only, gate the whole absolute-path
block with #[cfg(unix)] to avoid running on Windows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 86fae0a1-c7a2-40e7-ba76-8c1d8144a1a7
📒 Files selected for processing (2)
src/linker.rstests/test_security.rs
|
@copilot Verify each finding against current code. Fix only still-valid issues, skip the Inline comments:
|
…s compatibility Agent-Logs-Url: https://github.com/dallay/agentsync/sessions/a8b3f8b6-f5cd-4ac2-9373-a959808eb383 Co-authored-by: yacosta738 <33158051+yacosta738@users.noreply.github.com>
... Fixed in b20a90f. Applied |
|
yacosta738
deleted the
fix/security-nested-glob-traversal-10326664153959606349
branch
Identified and fixed a path traversal vulnerability in AgentSync's
NestedGlobsynchronization logic.Severity: HIGH
Vulnerability: Path traversal in symlink discovery (search root).
Impact: A malicious
agentsync.tomlcould specify a search root outside the project (e.g.,source = "../../"), causing AgentSync to recursively walk sensitive host directories.Fix: Added validation for the
sourcefield inNestedGlobtargets usingrevalidate_destination_pathto ensure it resolves within the project root.Source:
src/linker.rsinprocess_targetandclean.Verification: Added
tests/test_security.rswith a reproduction case that verifies the traversal is now rejected with an error. All existing tests pass.PR created automatically by Jules for task 10326664153959606349 started by @yacosta738