🛡️ Sentinel: [HIGH] Fix Path Traversal in TypeScript Extractor#223
Open
bashandbone wants to merge 1 commit into
Open
🛡️ Sentinel: [HIGH] Fix Path Traversal in TypeScript Extractor#223bashandbone wants to merge 1 commit into
bashandbone wants to merge 1 commit into
Conversation
The manual path normalization logic for `ParentDir` used `components.pop()`, which incorrectly simplified `../../a` to `a` by popping the first `..`, and could traverse past the root directory. This logic has been rewritten to handle `RootDir`/`Prefix` barriers and correctly preserve preceding `ParentDir` components when resolving relative paths. 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 custom path normalization by tightening ParentDir handling; also includes minor API and formatting cleanups plus adds a Sentinel security note. 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 updated
ParentDirhandling logic intypescript.rsis security-sensitive; consider extracting it into a small, well-named helper (e.g.,normalize_components(...)) so the intended invariants (no poppingRootDir/Prefix, preserving leading..) are encapsulated and reused consistently wherever similar normalization is needed. - Given this is fixing a path traversal bug, it may be worth adding a brief comment or type alias around the
resolved.components()iteration to clarify whetherresolvedis guaranteed to be an untrusted/relative path or may be absolute, so future changes don’t accidentally relax the newRootDir/Prefixguards.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The updated `ParentDir` handling logic in `typescript.rs` is security-sensitive; consider extracting it into a small, well-named helper (e.g., `normalize_components(...)`) so the intended invariants (no popping `RootDir`/`Prefix`, preserving leading `..`) are encapsulated and reused consistently wherever similar normalization is needed.
- Given this is fixing a path traversal bug, it may be worth adding a brief comment or type alias around the `resolved.components()` iteration to clarify whether `resolved` is guaranteed to be an untrusted/relative path or may be absolute, so future changes don’t accidentally relax the new `RootDir`/`Prefix` guards.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 high-severity path traversal bypass in the TypeScript dependency extractor’s manual path normalization, and includes a handful of formatting/cleanup changes in nearby Rust modules plus a new incident log entry.
Changes:
- Harden
Component::ParentDirhandling during manual normalization to avoid collapsing excess..segments and prevent popping pastRootDir/Prefix. - Minor refactors/formatting in rule-engine and ast-engine code (no functional change intended).
- Add a
.juleswrite-up documenting the vulnerability and prevention approach.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/rule-engine/src/rule/referent_rule.rs | Formatting-only refactor of a RwLock read/clone chain. |
| crates/rule-engine/src/rule/mod.rs | Formatting-only expansion of defined_vars collection pipeline. |
| crates/rule-engine/src/check_var.rs | Removes unnecessary lifetime parameters from helper function signatures. |
| crates/flow/src/incremental/extractors/typescript.rs | Security fix: correct ParentDir stack handling during manual path normalization. |
| crates/ast-engine/src/tree_sitter/mod.rs | Formatting-only changes (unwrap formatting and assert formatting). |
| .jules/sentinel.md | Adds a new security incident/learning note (currently missing SPDX header, and uses a literal $(date ...) placeholder). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
810
to
+814
| std::path::Component::ParentDir => { | ||
| components.pop(); | ||
| // SECURITY: Prevent path traversal bypassing by correctly handling ParentDir. | ||
| // If the list is empty or ends in ParentDir, push it to preserve relative paths like `../../a`. | ||
| if let Some(c) = components.last() { | ||
| match c { |
Comment on lines
+1
to
+3
|
|
||
| ## $(date +%Y-%m-%d) - Path Traversal Bypass in Manual Normalization | ||
| **Vulnerability:** Path traversal (`../../`) bypass during custom path normalization using `std::path::Component::ParentDir`. |
| @@ -0,0 +1,5 @@ | |||
|
|
|||
| ## $(date +%Y-%m-%d) - Path Traversal Bypass in Manual Normalization | |||
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: The custom path normalization algorithm incorrectly handled
ParentDir(..). It allowed trailing..references to bypass directory restrictions by popping past root directories, and malformed relative references like../../xintox.🎯 Impact: This allowed crafted imports to traverse outside of intended source directories and potentially expose internal configuration or restricted code segments.
🔧 Fix: Updated the
ParentDirhandling logic intypescript.rs. It now strictly prevents poppingRootDirorPrefixand ensuresParentDircomponents are properly preserved (pushed) if the component stack is empty or already ends inParentDir.✅ Verification: Tested via unit tests and manually confirmed the correct normalization of paths like
../../aand/a/../../etc/passwd.PR created automatically by Jules for task 553011325921943399 started by @bashandbone
Summary by Sourcery
Fix path traversal handling in the TypeScript dependency extractor and make minor refactors and documentation updates.
Bug Fixes:
Enhancements:
Documentation: