🛡️ Sentinel: [CRITICAL] Fix path traversal risk in TypeScript module resolution#237
Open
bashandbone wants to merge 1 commit into
Open
🛡️ Sentinel: [CRITICAL] Fix path traversal risk in TypeScript module resolution#237bashandbone wants to merge 1 commit into
bashandbone wants to merge 1 commit into
Conversation
…resolution This commit fixes a vulnerability in `crates/flow/src/incremental/extractors/typescript.rs` where manual path component resolution could unconditionally pop the `RootDir` or improperly handle multiple consecutive `ParentDir`s (`..`). By properly verifying the previous path component type, `..` correctly handles boundaries, preventing potential sandboxing escapes and module graph corruption. 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 GuideAdjusts TypeScript module path normalization to prevent escaping the root via Flow diagram for updated ParentDir handling in TypeScript path normalizationflowchart TD
A[Component is ParentDir] --> B{Last component on stack}
B -->|Normal| C[Pop last component]
B -->|RootDir or Prefix| D[Do nothing]
B -->|None or ParentDir or other| E[Push ParentDir onto stack]
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 new ParentDir handling logic is embedded inline in the loop; consider extracting it into a small helper function (e.g.
normalize_parent_dir_component(&mut components)) so any other manual path normalization can reuse the same hardened behavior and stay consistent. - It may be worth double-checking how this logic behaves with Windows-specific prefixes (e.g. UNC paths or drive letters) and documenting any assumptions in code comments if certain
Component::Prefixvariants are not expected to appear inresolvedpaths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new ParentDir handling logic is embedded inline in the loop; consider extracting it into a small helper function (e.g. `normalize_parent_dir_component(&mut components)`) so any other manual path normalization can reuse the same hardened behavior and stay consistent.
- It may be worth double-checking how this logic behaves with Windows-specific prefixes (e.g. UNC paths or drive letters) and documenting any assumptions in code comments if certain `Component::Prefix` variants are not expected to appear in `resolved` paths.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
Hardens the manual path-component normalization used when canonicalize() fails inside TypeScriptDependencyExtractor::resolve_module_path, so that a ParentDir (..) only pops a real directory name. Encountering .. above a RootDir/Prefix is now ignored, and .. against an empty stack or another ParentDir is preserved. A short retrospective note is also added under .jules/.
Changes:
- Replace the unconditional
components.pop()onParentDirwith a check on the previous component to prevent escaping above/and to preserve consecutive../segments in relative paths. - Add
.jules/sentinel.mddescribing the issue, learning, and prevention guidance.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/flow/src/incremental/extractors/typescript.rs | Guards ParentDir handling against popping RootDir/Prefix and preserves consecutive .. components. |
| .jules/sentinel.md | Adds a security retrospective entry describing the traversal fix. |
đź’ˇ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+811
to
+819
| // Prevent path traversal escaping root, and handle consecutive ParentDirs | ||
| let last = components.last().copied(); | ||
| match last { | ||
| Some(std::path::Component::Normal(_)) => { | ||
| components.pop(); | ||
| } | ||
| Some(std::path::Component::RootDir) | Some(std::path::Component::Prefix(_)) => {} | ||
| _ => components.push(component), | ||
| } |
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 path resolution logic for TypeScript imports (
crates/flow/src/incremental/extractors/typescript.rs) usedstd::path::Componentsequentially. When hitting aParentDir(..), it unconditionally removed the last element from the path stack. This allowed malicious payloads with../../structures to bypassRootDirlimitations or incorrectly flatten valid complex paths, leading to path traversal outside expected boundaries.🎯 Impact: Exploitation of this flaw could allow attackers parsing malicious or crafted TypeScript code bases to cause the module graph resolver to inspect arbitrary files on the local filesystem outside of the provided module tree (depending on sandbox strictness), acting as a blind path-traversal primitive during module extraction.
🔧 Fix: We updated the resolution logic to check the
last()component on the stack. Now,ParentDirwill onlypop()if the top of the stack is aNormalcomponent. If it's aRootDirorPrefix, theParentDiris safely ignored (cannot traverse above root). If the stack is empty or ends with anotherParentDir, theParentDiris pushed, correctly preserving paths like../../a.âś… Verification: We ran the
thread-flowunit tests (specificallyextractor_typescript_tests) and verifiedcargo clippy -p thread-flowpasses correctly. I also performed manual tests with isolated payloads to verify the root escape path behavior.PR created automatically by Jules for task 2592889757875481194 started by @bashandbone
Summary by Sourcery
Harden TypeScript dependency extractor path normalization to prevent path traversal above the configured root and document the incident and lessons learned.
Bug Fixes:
Documentation: