🛡️ Sentinel: [CRITICAL] Fix path traversal in typescript extractor#277
Open
bashandbone wants to merge 1 commit into
Open
🛡️ Sentinel: [CRITICAL] Fix path traversal in typescript extractor#277bashandbone wants to merge 1 commit into
bashandbone wants to merge 1 commit into
Conversation
This fixes a vulnerability where resolving module imports starting with `../` incorrectly dropped components when the component list was empty, allowing resolution outside the base path tree. 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 critical path traversal bug in the TypeScript dependency extractor’s manual path normalization logic and performs a small lifetime-simplification cleanup in the rule engine, plus adds a Sentinel write-up 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:
- The manual path-normalization logic in
TypeScriptDependencyExtractoris now fairly intricate; consider extracting it into a small, well-named helper function with a brief comment documenting its security invariants (e.g., whenParentDiris pushed vs. popped and whyRootDir/Prefixare protected) to make future maintenance and review easier. - In the
componentsmatch, you might want to handle allstd::path::Componentvariants explicitly (e.g., with a non-emptymatchoncomponentrather than using_) so that any new variants added by the standard library cause a compile-time error instead of being silently treated as a normal component.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The manual path-normalization logic in `TypeScriptDependencyExtractor` is now fairly intricate; consider extracting it into a small, well-named helper function with a brief comment documenting its security invariants (e.g., when `ParentDir` is pushed vs. popped and why `RootDir`/`Prefix` are protected) to make future maintenance and review easier.
- In the `components` match, you might want to handle all `std::path::Component` variants explicitly (e.g., with a non-empty `match` on `component` rather than using `_`) so that any new variants added by the standard library cause a compile-time error instead of being silently treated as a normal component.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 addresses a critical security issue in the TypeScript/JavaScript dependency extractor’s path normalization logic to reduce the risk of path traversal during module resolution, alongside minor Rust cleanup and an incident note.
Changes:
- Hardened manual
..component handling inTypeScriptDependencyExtractor::resolve_module_path. - Removed unnecessary lifetimes from rule-engine variable checking helpers (clippy cleanup).
- Added a Sentinel incident note documenting the vulnerability and prevention guidance.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
crates/rule-engine/src/check_var.rs |
Simplifies helper function signatures by removing unused lifetimes. |
crates/flow/src/incremental/extractors/typescript.rs |
Updates .. normalization logic during TypeScript module path resolution. |
.jules/sentinel.md |
Adds an incident write-up for the path traversal issue and mitigation guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1
to
+4
| ## 2024-05-15 - Path Traversal in TypeScript Module Resolution | ||
| **Vulnerability:** The manual path normalization logic for unresolved TypeScript modules blindly called `components.pop()` when encountering `..` (ParentDir), which would swallow components when the list was empty, allowing path traversal outside of intended boundaries. | ||
| **Learning:** Rust's `Path::components()` does not automatically normalize `.` and `..` away. Manual normalization must explicitly track when components is empty or ends in `..` and push `ParentDir` in those cases instead of ignoring/popping it, otherwise `../../../file` resolves as `file`. | ||
| **Prevention:** Whenever manually resolving paths with `std::path::Component`, block `ParentDir` from popping `RootDir` or `Prefix`, and explicitly push `ParentDir` if the current component list is empty or its last element is `ParentDir`. |
Comment on lines
+813
to
+816
| Some(std::path::Component::RootDir) | ||
| | Some(std::path::Component::Prefix(_)) => { | ||
| // Ignore: cannot pop root or prefix | ||
| } |
Comment on lines
810
to
+823
| std::path::Component::ParentDir => { | ||
| components.pop(); | ||
| let last = components.last().copied(); | ||
| match last { | ||
| Some(std::path::Component::RootDir) | ||
| | Some(std::path::Component::Prefix(_)) => { | ||
| // Ignore: cannot pop root or 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: CRITICAL
💡 Vulnerability: The
TypeScriptDependencyExtractormanually normalized paths and blindly popped thecomponentsvector when encountering..(std::path::Component::ParentDir). If the vector was empty (or navigating backwards multiple levels), it swallowed the.., which meant traversing../../../sensitivewould simply resolve tosensitive, opening up the risk for path traversal vulnerabilities in dependency extraction.🎯 Impact: An attacker controlling imports in a TypeScript file could force the extractor to resolve and process files outside of the intended source tree, potentially extracting contents or crashing the system depending on how the parsed results are handled.
🔧 Fix: Updated the normalization logic to securely inspect the last element in the components vector. It now correctly retains
..if the list is empty or the last element is also.., prevents poppingRootDirorPrefix, and only pops if the last component was a normal path directory. Also cleaned up some unused lifetimes incrates/rule-engine/src/check_var.rscaught by clippy.✅ Verification: Ran unit tests for the TypeScript extractor (
cargo test -p thread-flow --test extractor_typescript_tests) to ensure correct resolution behaviour and ensure no regressions were introduced. Also successfully rancargo clippy --workspace -- -D warnings.PR created automatically by Jules for task 12822901857360876647 started by @bashandbone
Summary by Sourcery
Fix insecure path normalization in the TypeScript dependency extractor to prevent path traversal and perform minor cleanup in the rule engine.
Bug Fixes:
..components without allowing traversal outside the intended root or prefix.Enhancements:
Chores: