🛡️ Sentinel: [HIGH] Fix Path Traversal in TypeScript Module Resolution#157
🛡️ Sentinel: [HIGH] Fix Path Traversal in TypeScript Module Resolution#157bashandbone wants to merge 1 commit intomainfrom
Conversation
…ution 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 GuideFixes a path traversal vulnerability in TypeScript module resolution by making manual path normalization robust against Class diagram for TypeScriptDependencyExtractor path resolution changesclassDiagram
class TypeScriptDependencyExtractor {
+resolve_module_path(resolved_path) PathBuf
}
class PathNormalization {
+components Vec_Component
+normalize(resolved_path) PathBuf
+handle_parent_dir(components, component) void
+handle_cur_dir(components, component) void
+handle_other(components, component) void
}
class Component {
<<enum>>
RootDir
Prefix
Normal
CurDir
ParentDir
}
TypeScriptDependencyExtractor --> PathNormalization : uses
PathNormalization --> Component : iterates_over
class Vec_Component {
+push(component) void
+pop() Option_Component
+last() Option_Component
}
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
ParentDirhandling logic would be clearer and less error-prone if the_arm were split into explicit cases (e.g.,NonevsSome(ParentDir)), so future readers don’t have to infer the intended behavior for empty stacks versus nested..segments.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `ParentDir` handling logic would be clearer and less error-prone if the `_` arm were split into explicit cases (e.g., `None` vs `Some(ParentDir)`), so future readers don’t have to infer the intended behavior for empty stacks versus nested `..` segments.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
Addresses a high-severity path traversal risk in TypeScript module resolution by fixing incorrect manual path normalization when handling .. (Component::ParentDir) components.
Changes:
- Updates
resolve_module_pathmanual normalization to prevent poppingRootDir/Prefixcomponents when processingParentDir. - Preserves leading
..segments for relative paths (e.g.,../../a) instead of incorrectly discarding them. - Adds a Sentinel write-up documenting the vulnerability 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 |
Fixes manual normalization logic for .. to avoid root/prefix loss and incorrect path rewriting. |
.jules/sentinel.md |
Documents the incident/learning around safe manual normalization with std::path::Component. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| std::path::Component::ParentDir => match components.last() { | ||
| Some(std::path::Component::RootDir) | ||
| | Some(std::path::Component::Prefix(_)) => { | ||
| // Cannot go above root | ||
| } | ||
| Some(std::path::Component::Normal(_)) => { | ||
| components.pop(); | ||
| } | ||
| _ => { | ||
| components.push(component); | ||
| } | ||
| }, |
There was a problem hiding this comment.
The new ParentDir normalization logic is security-sensitive but currently isn’t covered by an automated regression test. Please add a test case that forces the manual normalization path (canonicalize fails) with an absolute source_file and an import containing enough .. segments to hit the root, then assert the result remains absolute (i.e., root/prefix is preserved and not dropped). This would prevent future regressions back to the prior components.pop() behavior.
🚨 Severity: HIGH
💡 Vulnerability: The
resolve_module_pathincrates/flow/src/incremental/extractors/typescript.rsused a blindcomponents.pop()to handle..(ParentDir) components in manual path normalization. This was flawed because if the path started with.., or if..was evaluated against the root directory (/) or a drive prefix (C:\), it would improperly discard these important root or prefix components, potentially changing absolute paths to relative ones, escaping directories, or causing resolution mismatches.🎯 Impact: Attackers could potentially exploit this manual path normalization behavior when parsing adversarial imports or files, allowing resolution outside of the intended extraction boundary, which is a classic path traversal risk.
🔧 Fix: Replaced the blind
.pop()with a match statement oncomponents.last(). It explicitly checks forRootDirandPrefixto prevent escaping above the root. It correctly popsNormalcomponents, and it safely retainsParentDir(pushes it back onto the stack) when resolving at the root level of a relative path (e.g.../../a).✅ Verification: Ran
cargo test -p thread-flow --test extractor_typescript_testswhich confirmed 25 module resolution tests passed with no regressions.PR created automatically by Jules for task 817992255317682365 started by @bashandbone
Summary by Sourcery
Fix path traversal handling in TypeScript module resolution by hardening manual path normalization and document the vulnerability and prevention guidance in Sentinel notes.
Bug Fixes:
Documentation: