🛡️ Sentinel: [HIGH] Fix path traversal in manual path resolution#173
Open
bashandbone wants to merge 1 commit intomainfrom
Open
🛡️ Sentinel: [HIGH] Fix path traversal in manual path resolution#173bashandbone wants to merge 1 commit intomainfrom
bashandbone wants to merge 1 commit intomainfrom
Conversation
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 GuideAdjusts manual path normalization in the TypeScript dependency extractor to correctly handle parent directory components and prevent path traversal, and documents the vulnerability and fix in the Sentinel learning journal. Sequence diagram for safe manual path resolution in TypeScript dependency extractorsequenceDiagram
participant Extractor as TypeScriptDependencyExtractor
participant FS as FileSystem
participant Canon as Canonicalizer
participant Normalizer as ManualPathNormalizer
Extractor->>Extractor: extract_dependencies(source_file)
loop For each import specifier
Extractor->>Extractor: resolve_import_path(import_path)
Extractor->>Canon: canonicalize(resolved_path)
alt canonicalize succeeds
Canon-->>Extractor: Ok(canonical_path)
Extractor->>Extractor: use canonical_path
else canonicalize fails
Canon-->>Extractor: Err(io_error)
Extractor->>Normalizer: normalize_components(resolved_path)
Normalizer->>Normalizer: iterate resolved.components()
Normalizer-->>Extractor: normalized_relative_path
Extractor->>Extractor: use normalized_relative_path
end
end
Flow diagram for updated manual path normalization logicflowchart TD
A["Start normalization for resolved path"] --> B["Initialize empty components stack"]
B --> C["For each component in resolved.components()"]
C --> D{Is component ParentDir}
D -- Yes --> E{Is components stack empty}
E -- Yes --> F["Push ParentDir onto stack"]
F --> C
E -- No --> G{Last element is RootDir or Prefix}
G -- Yes --> H["Do nothing (do not pop RootDir or Prefix)"]
H --> C
G -- No --> I{Last element is ParentDir}
I -- Yes --> J["Push ParentDir onto stack"]
J --> C
I -- No --> K["Pop last component from stack"]
K --> C
D -- No --> L{Is component CurDir}
L -- Yes --> M["Skip component"]
M --> C
L -- No --> N["Push component onto stack"]
N --> C
C --> O["No more components"]
O --> P["Join components stack into normalized path"]
P --> Q["End"]
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
Component::ParentDirhandling block is getting a bit nested; consider extracting this normalization logic into a small helper function to clarify the intended semantics and make it easier to reason about future changes. - It might be worth adding a short comment in the
ParentDirmatch arm explaining the specific edge cases being handled (empty stack, previousParentDir, and blocking pops ofRootDir/Prefix) so future readers don’t inadvertently reintroduce the traversal bug. - Double-check that the
Prefixhandling here covers Windows UNC and drive prefixes as intended; if there are platform-specific assumptions (e.g., only used on Unix paths), it could help to enforce or assert that in this normalization path.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `Component::ParentDir` handling block is getting a bit nested; consider extracting this normalization logic into a small helper function to clarify the intended semantics and make it easier to reason about future changes.
- It might be worth adding a short comment in the `ParentDir` match arm explaining the specific edge cases being handled (empty stack, previous `ParentDir`, and blocking pops of `RootDir`/`Prefix`) so future readers don’t inadvertently reintroduce the traversal bug.
- Double-check that the `Prefix` handling here covers Windows UNC and drive prefixes as intended; if there are platform-specific assumptions (e.g., only used on Unix paths), it could help to enforce or assert that in this normalization path.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
Addresses a path traversal/incorrect path normalization issue in the TypeScript dependency extractor’s fallback path resolution when canonicalize() fails, and records the incident as a Sentinel learning entry.
Changes:
- Fixes manual
..component handling during TypeScript relative module resolution to avoid dropping leading parent traversals. - Adds a Sentinel journal entry documenting the vulnerability and the intended prevention pattern.
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 |
Hardens manual path normalization logic for Component::ParentDir when canonicalize() fails. |
.jules/sentinel.md |
Documents the vulnerability, learning, and prevention guidance. |
đź’ˇ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
810
to
+826
| std::path::Component::ParentDir => { | ||
| components.pop(); | ||
| if let Some(c) = components.last() { | ||
| match c { | ||
| std::path::Component::RootDir | ||
| | std::path::Component::Prefix(_) => { | ||
| // Do not pop RootDir or Prefix | ||
| } | ||
| std::path::Component::ParentDir => { | ||
| components.push(component); | ||
| } | ||
| _ => { | ||
| components.pop(); | ||
| } | ||
| } | ||
| } else { | ||
| 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: HIGH
đź’ˇ Vulnerability: A path traversal vulnerability existed in the manual path normalization logic (
resolved.components()) whencanonicalizefailed in the TypeScript extractor.Component::ParentDir(..) always popped a component from the stack, even when the stack was empty or contained another.., meaning a relative path like../../acould lose its initial..navigation parts and evaluate incorrectly. This could allow for path traversal when resolving TypeScript imports.🎯 Impact: This could be exploited to bypass the intended directory structure limit, leading to unauthorized file access or incorrect path resolution.
🔧 Fix: Updated the
Component::ParentDirmatching block to explicitly prevent poppingComponent::RootDirorComponent::Prefix. If the components list is empty or its last element isComponent::ParentDir, the newComponent::ParentDiris pushed to properly preserve relative navigation.âś… Verification: Ran the extractor typescript tests which passed successfully. Wrote a test script (deleted) to confirm Rust's path normalization correctly leaves
../../a/b/cinstead ofa/b/c. Added the.jules/sentinel.mdlearning journal.PR created automatically by Jules for task 6265645109127355951 started by @bashandbone
Summary by Sourcery
Fix unsafe manual path normalization in the TypeScript dependency extractor to correctly preserve relative parent directory components and prevent path traversal when canonicalization fails.
Bug Fixes:
..components cannot escape root or prefix and are preserved when there is no prior segment to pop.Documentation: