🛡️ Sentinel: [CRITICAL] Fix path traversal#257
Open
bashandbone wants to merge 1 commit into
Open
Conversation
…ution 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 manual path normalization in the TypeScript dependency extractor to safely handle 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:
- Consider extracting the
ParentDirhandling logic into a small helper function (e.g.,normalize_parent_dir_component) so the control flow is easier to read and can be reused if other parts of the code need the same normalization semantics. - The nested
matchoncomponents.last()could be simplified withif let/matches!to make the three branches (“ignore”, “push”, “pop”) more visually distinct and reduce indentation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the `ParentDir` handling logic into a small helper function (e.g., `normalize_parent_dir_component`) so the control flow is easier to read and can be reused if other parts of the code need the same normalization semantics.
- The nested `match` on `components.last()` could be simplified with `if let`/`matches!` to make the three branches (“ignore”, “push”, “pop”) more visually distinct and reduce indentation.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 manual path normalization in the TypeScript dependency extractor to correctly handle std::path::Component::ParentDir, reducing the risk of path traversal caused by incorrect .. processing when canonicalization fails.
Changes:
- Update
resolve_module_pathmanual normalization to preserve/ignore..appropriately based on the last component (instead of unconditionalpop()). - Add a Sentinel note documenting the vulnerability and recommended handling pattern.
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 |
Reworks ParentDir normalization logic to avoid unsafe/incorrect component popping. |
.jules/sentinel.md |
Adds security documentation for the path traversal fix (needs REUSE/SPDX header). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1
to
+4
| ## 2025-05-23 - Prevent Path Traversal in Component Normalization | ||
| **Vulnerability:** Path traversal during module resolution (`crates/flow/src/incremental/extractors/typescript.rs`). When manually resolving paths with `std::path::Component`, encountering `Component::ParentDir` was handled by unconditionally calling `components.pop()`. This allowed paths like `dir/../../passwd` to strip the relative path and silently traverse directories without being correctly preserved. | ||
| **Learning:** `std::path::Component::ParentDir` resolution isn't as simple as just popping the previous element. You cannot pop `Component::RootDir` or `Component::Prefix`, and if the previous element is already `ParentDir` (or the list is empty for a relative path), the new `ParentDir` must be explicitly pushed to preserve upward traversal securely. | ||
| **Prevention:** Always use a safe path normalization pattern when handling `ParentDir` manually that matches on the last pushed component type before determining whether to push or pop. |
Comment on lines
+811
to
+822
| // SECURITY: Prevent path traversal by correctly handling ParentDir. | ||
| // Do not pop RootDir or Prefix. Push ParentDir if empty or last is ParentDir | ||
| // to preserve relative paths like `../../a`. | ||
| match components.last() { | ||
| Some(std::path::Component::RootDir) | Some(std::path::Component::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 manual path resolution logic for module specifiers in
crates/flow/src/incremental/extractors/typescript.rsfailed to properly processstd::path::Component::ParentDir. By unconditionally using.pop(), it could incorrectly modify paths and allow potential path traversal escapes (e.g.../../being flattened incorrectly).🎯 Impact: If module extraction processed untrusted input (or crafted dependencies), it could potentially cause the system to resolve paths outside intended directory boundaries.
🔧 Fix: Introduced secure handling for
Component::ParentDir. It now prevents poppingRootDirorPrefix, and preservesParentDirif it's pushed against an empty list or anotherParentDir.✅ Verification: Verified by checking manual path constructions and running
cargo test -p thread-flow --test extractor_typescript_tests. No regressions introduced and the logic mirrors safe standard library behaviors.PR created automatically by Jules for task 16679692601869506387 started by @bashandbone
Summary by Sourcery
Harden TypeScript dependency extraction path normalization to correctly handle parent directory components and document the security fix.
Bug Fixes:
Documentation: