🛡️ Sentinel: [HIGH] Fix path traversal in manual TS module path resolution#161
🛡️ Sentinel: [HIGH] Fix path traversal in manual TS module path resolution#161bashandbone wants to merge 1 commit intomainfrom
Conversation
The `TypeScriptDependencyExtractor::resolve_module_path` manual fallback path normalization previously failed to correctly handle `ParentDir` (i.e. `..`) traversal limits, potentially allowing a path to erroneously escape its virtual root context or discard absolute prefixes incorrectly if it encountered consecutive `..` components. This update implements safer boundary checks. Co-authored-by: bashandbone <89049923+bashandbone@users.noreply.github.com>
|
👋 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. |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts manual TypeScript module path normalization to safely handle ParentDir components and prevent path traversal, and documents the vulnerability and fix in a Sentinel note. Flow diagram for updated ParentDir handling in resolve_module_path normalizationflowchart TD
A[Start processing component] --> B{component is ParentDir?}
B -->|No| C[If CurDir: do nothing<br/>Else: push component onto components]
C --> Z[Next path component]
B -->|Yes| D[Compute flags<br/>is_empty = components.is_empty<br/>last_is_parent = last == ParentDir<br/>last_is_root_or_prefix = last == RootDir or Prefix]
D --> E{is_empty or last_is_parent?}
E -->|Yes| F[Push ParentDir onto components]
F --> Z
E -->|No| G{last_is_root_or_prefix?}
G -->|Yes| H[Do not pop components<br/>Leave components unchanged]
H --> Z
G -->|No| I[Pop last component from components]
I --> Z
Z --> J{More components?}
J -->|Yes| A
J -->|No| K[Finish normalization]
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}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new
ParentDirhandling logic is subtle enough that it would benefit from a short comment explaining the intended invariants (e.g., when..should be preserved vs. collapsed and whyRootDir/Prefixare never popped) to aid future maintainers. - Consider factoring the
ParentDirnormalization into a small helper function (e.g.,fn push_parent_dir(components: &mut Vec<Component>)) to encapsulate the rules around empty vectors, consecutiveParentDir, andRootDir/Prefix, which will make the main loop easier to read and reason about.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `ParentDir` handling logic is subtle enough that it would benefit from a short comment explaining the intended invariants (e.g., when `..` should be preserved vs. collapsed and why `RootDir`/`Prefix` are never popped) to aid future maintainers.
- Consider factoring the `ParentDir` normalization into a small helper function (e.g., `fn push_parent_dir(components: &mut Vec<Component>)`) to encapsulate the rules around empty vectors, consecutive `ParentDir`, and `RootDir`/`Prefix`, which will make the main loop easier to read and reason about.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
Fixes a high-severity path traversal/canonicalization bug in the TypeScript extractor’s manual module path normalization fallback, and documents the vulnerability/learning in the Jules Sentinel log.
Changes:
- Harden manual
..component reduction inTypeScriptDependencyExtractor::resolve_module_pathto preserve leading/consecutive..and avoid poppingRootDir/Prefix. - Add a Sentinel entry describing the vulnerability and prevention guidance.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/flow/src/incremental/extractors/typescript.rs | Updates manual path component reduction logic for .. during TS module resolution fallback. |
| .jules/sentinel.md | Adds a security note documenting the issue and mitigation guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let last_is_parent = matches!(components.last(), Some(std::path::Component::ParentDir)); | ||
| let last_is_root_or_prefix = matches!( | ||
| components.last(), | ||
| Some(std::path::Component::RootDir) | Some(std::path::Component::Prefix(_)) |
There was a problem hiding this comment.
components.last() returns Option<&std::path::Component>, but the matches! patterns here are written as if they were matching Option<std::path::Component>. This is likely a compile error (and will also be caught by clippy). Update the patterns to match references (e.g., Some(&Component::ParentDir) / Some(&Component::RootDir) / Some(&Component::Prefix(_))) or bind components.last() to a variable and match on *last/last appropriately.
| let last_is_parent = matches!(components.last(), Some(std::path::Component::ParentDir)); | |
| let last_is_root_or_prefix = matches!( | |
| components.last(), | |
| Some(std::path::Component::RootDir) | Some(std::path::Component::Prefix(_)) | |
| let last_is_parent = | |
| matches!(components.last(), Some(&std::path::Component::ParentDir)); | |
| let last_is_root_or_prefix = matches!( | |
| components.last(), | |
| Some(&std::path::Component::RootDir) | |
| | Some(&std::path::Component::Prefix(_)) |
| if is_empty || last_is_parent { | ||
| components.push(component); | ||
| } else if !last_is_root_or_prefix { | ||
| components.pop(); | ||
| } |
There was a problem hiding this comment.
This change fixes tricky .. normalization edge cases, but there are no tests covering the newly handled scenarios (leading ../../... that should remain relative, and .. following RootDir/Prefix that must not pop them). Please add unit tests exercising these cases so the security fix can’t regress silently; the existing resolve_module_path tests in crates/flow/tests/extractor_typescript_tests.rs can be extended with a couple of dedicated cases.
| ## 2025-05-15 - [Path Traversal in Manual Path Normalization] | ||
| **Vulnerability:** Manual path resolution using `components.pop()` on `std::path::Component::ParentDir` allowed path traversal. If a path like `../../a` was parsed, `components.pop()` on an empty `Vec` did nothing, turning the path into `a` instead of preserving the parent traversal. It could also pop `RootDir` or `Prefix` components, changing absolute paths to relative ones or traversing beyond intended roots. | ||
| **Learning:** `std::path::Component` normalization must handle empty lists and consecutive `ParentDir` components by pushing them instead of ignoring them. It must also explicitly avoid popping `RootDir` or `Prefix` components to prevent escaping virtual file systems or simulated root directories. | ||
| **Prevention:** Explicitly check if the `components` list is empty, if the last component is `ParentDir`, or if the last component is `RootDir` / `Prefix` before calling `pop()`. |
There was a problem hiding this comment.
New markdown files in this repo typically include an SPDX header (often via an HTML comment block) or are covered by a REUSE.toml annotation. This file currently has no SPDX metadata and appears not to be covered by the existing REUSE.toml annotations, which can cause the fsfe/reuse-action CI job to fail. Add an SPDX header to this file or extend REUSE.toml to cover .jules/**.
🚨 Severity: HIGH
💡 Vulnerability: A path traversal vulnerability existed in the manual path normalization fallback logic within
TypeScriptDependencyExtractor::resolve_module_pathincrates/flow/src/incremental/extractors/typescript.rs. The logic usedcomponents.pop()incorrectly when encounteringstd::path::Component::ParentDir. If it was passed consecutive..directories, or paths leading back up out of the virtual resolution root, it could either mistakenly ignore them entirely, or it could popRootDirandPrefixcomponents, effectively breaking out of intended boundaries.🎯 Impact: It could cause incorrect relative module resolution, either masking dependencies, creating erroneous dependency edges outside of the project folder structure, or in specific cases exposing the internal path structure.
🔧 Fix: Updated the component reduction logic to push
ParentDironto the vector if the vector is empty, if the previous component was aParentDir, and prevented popping if the previous component was aRootDirorPrefix.✅ Verification: Ran
cargo test -p thread-flowunit tests and standard incremental tests successfully without regression.PR created automatically by Jules for task 16283416882480453920 started by @bashandbone
Summary by Sourcery
Fix TypeScript dependency extraction path normalization to prevent path traversal and document the vulnerability in Sentinel notes.
Bug Fixes:
Documentation: