🛡️ Sentinel: [CRITICAL] Fix Path Traversal Vulnerability in TypeScript Path Resolution#151
Open
bashandbone wants to merge 1 commit intomainfrom
Conversation
This fixes a critical path traversal bypass in `TypeScriptDependencyExtractor` caused by incorrectly popping empty/`ParentDir` path components and improperly deleting absolute path boundaries (`RootDir` and `Prefix`). This enforces standard lexical normalization semantics on the component vector stack. 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 guide (collapsed on small PRs)Reviewer's GuideFixes a critical path traversal vulnerability in the TypeScript dependency extractor by making ParentDir handling in manual path normalization respect path roots/prefixes and preserve unresolvable parent components in relative paths, and documents the issue and lessons learned in Sentinel notes. Flow diagram for updated ParentDir normalization logicflowchart TD
A[Handle Component ParentDir] --> B[Get last component from stack]
B -->|Last is RootDir or Prefix| C[Do nothing
Preserve absolute path boundary]
B -->|Last is None| D[Push ParentDir onto stack
Preserve leading .. in relative path]
B -->|Last is ParentDir| E[Push ParentDir onto stack
Accumulate unresolved ..]
B -->|Last is normal component| F[Pop last component
Resolve ParentDir against previous segment]
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 found 1 issue, and left some high level feedback:
- The
ParentDirhandling branch has become fairly complex; consider extracting this logic into a small helper function (e.g.,fn apply_parent_dir(components: &mut Vec<Component>)) to make the control flow clearer and easier to reason about. - The conditions around when to push vs. skip
ParentDir(especially the secondmatches!oncomponents.last()) could benefit from a brief comment explaining the intended invariants for absolute vs. relative paths to future readers. - Double-check whether Windows-specific
Prefixvariants (UNC, Verbatim paths, etc.) need any special-case handling beyond being treated as non-poppable, and if so, consider making that intent explicit in the code structure or naming.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `ParentDir` handling branch has become fairly complex; consider extracting this logic into a small helper function (e.g., `fn apply_parent_dir(components: &mut Vec<Component>)`) to make the control flow clearer and easier to reason about.
- The conditions around when to push vs. skip `ParentDir` (especially the second `matches!` on `components.last()`) could benefit from a brief comment explaining the intended invariants for absolute vs. relative paths to future readers.
- Double-check whether Windows-specific `Prefix` variants (UNC, Verbatim paths, etc.) need any special-case handling beyond being treated as non-poppable, and if so, consider making that intent explicit in the code structure or naming.
## Individual Comments
### Comment 1
<location path=".jules/sentinel.md" line_range="4" />
<code_context>
+**Prevention:** Whenever manual path resolution loop involves popping on `Component::ParentDir`, check the top of the component stack first to prevent popping prefixes/roots or ignoring `ParentDir` when the stack is empty/only contains `ParentDir`.
</code_context>
<issue_to_address>
**nitpick (typo):** Consider adding an article in "Whenever manual path resolution loop" for grammatical correctness.
Consider rephrasing the beginning to: "Whenever a manual path resolution loop involves popping on `Component::ParentDir`, ..." to improve grammar and readability.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ## 2025-02-28 - [Path Traversal bypass in PathBuf resolution] | ||
| **Vulnerability:** Found a vulnerable path component normalization in `crates/flow/src/incremental/extractors/typescript.rs`. Manual handling of `..` (`ParentDir`) blindly called `components.pop()`. This fails to handle root directories or prefixes correctly (converting absolute paths to relative, allowing traversal escapes), and incorrectly ignores `..` at the beginning of relative paths (e.g., resolving `../../a` to `a`). | ||
| **Learning:** `std::path::PathBuf::components()` correctly yields `Component::RootDir` and `Component::ParentDir`. Normalization algorithms MUST explicitly check the previous component before popping. Popping `RootDir` is inherently unsafe as it changes path type. Empty states or states ending in `ParentDir` must push `ParentDir` instead of doing nothing or blindly popping. | ||
| **Prevention:** Whenever manual path resolution loop involves popping on `Component::ParentDir`, check the top of the component stack first to prevent popping prefixes/roots or ignoring `ParentDir` when the stack is empty/only contains `ParentDir`. |
Contributor
There was a problem hiding this comment.
nitpick (typo): Consider adding an article in "Whenever manual path resolution loop" for grammatical correctness.
Consider rephrasing the beginning to: "Whenever a manual path resolution loop involves popping on Component::ParentDir, ..." to improve grammar and readability.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes unsafe manual path normalization in the TypeScript/JavaScript dependency extractor to avoid incorrectly collapsing .. segments and to prevent popping path boundaries (root/prefix) during module path resolution.
Changes:
- Harden manual
..(ParentDir) handling during path normalization whencanonicalize()fails. - Preserve unresolvable leading
..in relative paths instead of silently dropping them. - Add a Sentinel learning note documenting the vulnerability and prevention guidance.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/flow/src/incremental/extractors/typescript.rs | Updates manual path normalization logic to avoid unsafe ParentDir popping and preserve unresolved traversal components. |
| .jules/sentinel.md | Documents the identified path normalization vulnerability and prevention guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+811
to
+814
| let pop = match components.last() { | ||
| Some(std::path::Component::RootDir) | ||
| | Some(std::path::Component::Prefix(_)) => false, | ||
| None | Some(std::path::Component::ParentDir) => false, |
Comment on lines
810
to
+815
| std::path::Component::ParentDir => { | ||
| components.pop(); | ||
| let pop = match components.last() { | ||
| Some(std::path::Component::RootDir) | ||
| | Some(std::path::Component::Prefix(_)) => false, | ||
| None | Some(std::path::Component::ParentDir) => false, | ||
| _ => true, |
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: When manually resolving paths (such as
import ... from '../../a'), encountering..(ParentDir) blindly triggeredcomponents.pop(). This fails because popping an absolute path prefix (/orC:\) strips boundaries, converting absolute to relative and allowing traversal escape. Conversely, popping on an empty stack does nothing, incorrectly normalizing paths like../../aintoa.🎯 Impact: Exploitable path traversal during AST dependency extraction leading to arbitrary file read/resolution context escapes.
🔧 Fix: Added lexical stack validation before popping components.
RootDirandPrefixare protected, and un-resolvableParentDirs are preserved in relative paths.✅ Verification: Ran
cargo test -p thread-flow --test extractor_typescript_tests. Also cleaned up scratchpad scripts and recorded learning in.jules/sentinel.md.PR created automatically by Jules for task 16673417475008707265 started by @bashandbone
Summary by Sourcery
Fix unsafe handling of parent directory components during TypeScript dependency path normalization to prevent traversal escapes.
Bug Fixes:
Chores: