🛡️ Sentinel: [CRITICAL/HIGH] Fix path traversal in module resolution#293
Open
bashandbone wants to merge 1 commit into
Open
🛡️ Sentinel: [CRITICAL/HIGH] Fix path traversal in module resolution#293bashandbone wants to merge 1 commit into
bashandbone wants to merge 1 commit into
Conversation
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 GuideFixes a HIGH-severity path traversal bug in the TypeScript dependency extractor’s manual path normalization, and includes a few minor refactors/formatting adjustments plus a Sentinel journal entry documenting the vulnerability and mitigation pattern. 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 left some high level feedback:
- In the TypeScript extractor path normalization, consider explicitly commenting what should happen for Windows-specific prefixes (e.g., UNC paths or verbatim prefixes) so future changes don’t accidentally alter the
Prefix/ParentDirinteraction you’re securing here. - The new
ParentDirhandling silently ignores upward traversal at a root/prefix boundary; if this represents an invalid or attack-like path, it might be safer to surface an error or signal (rather than silently normalizing) so callers can distinguish benign relative paths from blocked traversal attempts.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the TypeScript extractor path normalization, consider explicitly commenting what should happen for Windows-specific prefixes (e.g., UNC paths or verbatim prefixes) so future changes don’t accidentally alter the `Prefix`/`ParentDir` interaction you’re securing here.
- The new `ParentDir` handling silently ignores upward traversal at a root/prefix boundary; if this represents an invalid or attack-like path, it might be safer to surface an error or signal (rather than silently normalizing) so callers can distinguish benign relative paths from blocked traversal attempts.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the TypeScript/JavaScript dependency extractor’s manual path normalization to avoid unsafe .. handling when canonicalize() fails, addressing a potential path traversal vector during module resolution. It also includes small readability refactors in the rule engine and tree-sitter utilities, plus a Sentinel journal entry documenting the security issue.
Changes:
- Harden
Component::ParentDirnormalization inTypeScriptDependencyExtractor::resolve_module_pathto avoid popping pastRootDir/Prefixand to preserve leading..when appropriate. - Minor refactors for readability (method chaining formatting, lifetime elision, assertion formatting) across rule-engine and ast-engine.
- Add a Sentinel journal entry describing the vulnerability and mitigation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/rule-engine/src/rule/referent_rule.rs | Minor formatting/refactor of lock-read + clone chain. |
| crates/rule-engine/src/rule/mod.rs | Readability formatting for defined_vars() collection. |
| crates/rule-engine/src/check_var.rs | Removes unnecessary explicit lifetimes in helper fns. |
| crates/flow/src/incremental/extractors/typescript.rs | Security hardening for manual .. path normalization when canonicalization fails. |
| crates/ast-engine/src/tree_sitter/mod.rs | Minor readability formatting for UTF-8 fallback and test assertion. |
| .jules/sentinel.md | Adds Sentinel journal entry documenting the TS extractor path traversal issue and mitigation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,4 @@ | |||
| ## 2024-06-06 - Path Traversal in TS Extractor | |||
Comment on lines
808
to
+824
| for component in resolved.components() { | ||
| match component { | ||
| std::path::Component::ParentDir => { | ||
| components.pop(); | ||
| // SECURITY: Prevent path traversal vulnerabilities by checking the last component | ||
| // before popping. We shouldn't pop RootDir or Prefix components. | ||
| // If the component list is empty or the last component is already ParentDir, | ||
| // we must push ParentDir to preserve relative paths like "../../a". | ||
| match components.last() { | ||
| Some(std::path::Component::RootDir) | ||
| | Some(std::path::Component::Prefix(_)) => {} | ||
| Some(std::path::Component::ParentDir) | None => { | ||
| components.push(component) | ||
| } | ||
| _ => { | ||
| components.pop(); | ||
| } | ||
| } |
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: HIGH
💡 Vulnerability: During manual canonicalization in the TypeScript dependency extractor (
crates/flow/src/incremental/extractors/typescript.rs), poppingstd::path::Component::ParentDircould incorrectly traverse beyond restricted directory boundaries (likeRootDirorPrefix) when canonicalize failed, or misinterpret valid relative paths like../../a.🎯 Impact: Attackers might be able to exploit module specifiers to escape project roots or load malicious local modules, bypassing module resolution constraints.
🔧 Fix: Explicitly track bounds during path normalizations. Added safeguards so that
ParentDirpushes instead of drops when the stack is empty or contains anotherParentDir, preserving relative patterns, and ignores root/prefix overrides.✅ Verification: Tested against standard TS resolution behavior using
cargo test -p thread-flow --test extractor_typescript_testsand verified the logic handles complex paths. Documented the pattern strictly inside the Sentinel journal at.jules/sentinel.md.PR created automatically by Jules for task 6371263555837495802 started by @bashandbone
Summary by Sourcery
Harden TypeScript module path normalization to prevent path traversal and document the vulnerability in the Sentinel journal.
Bug Fixes:
Enhancements:
Documentation: