🛡️ Sentinel: [CRITICAL/HIGH] Fix path traversal in manual path resolution#202
🛡️ Sentinel: [CRITICAL/HIGH] Fix path traversal in manual path resolution#202bashandbone wants to merge 1 commit intomainfrom
Conversation
…tion Co-authored-by: bashandbone <[email protected]>
|
đź‘‹ 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 GuideFixes a high-severity path traversal in TypeScript import resolution by making ParentDir handling safe and adds a small set of style/ergonomic cleanups elsewhere, plus a Sentinel learning note. Flow diagram for safe ParentDir handling in TypeScriptDependencyExtractorflowchart TD
A[Encounter Component::ParentDir] --> B{components.last exists?}
B -- No --> C[Push Component::ParentDir onto components]
B -- Yes --> D{last is RootDir or Prefix?}
D -- Yes --> E[Do not pop
Keep components unchanged]
D -- No --> F{last is ParentDir?}
F -- Yes --> G[Push Component::ParentDir
Preserve leading ParentDirs]
F -- No --> H[Pop last component
Navigate up one directory]
style A fill:#fdf5e6,stroke:#333
style C fill:#e6f7ff,stroke:#333
style E fill:#ffe6e6,stroke:#333
style G fill:#e6ffe6,stroke:#333
style H fill:#e6ffe6,stroke:#333
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In the new
ParentDirhandling logic, consider extracting the component-manipulation into a small helper function with a clear name (e.g.apply_parent_dir) so the control flow aroundRootDir,Prefix, and leadingParentDircases is easier to understand and reuse if needed elsewhere. - The manual component handling currently distinguishes
RootDirandPrefixbut does not explicitly document how Windows-specific prefixes (e.g. UNC paths or drive letters) should behave; adding a short code comment describing the intended behavior for these cases will make future changes to this logic safer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the new `ParentDir` handling logic, consider extracting the component-manipulation into a small helper function with a clear name (e.g. `apply_parent_dir`) so the control flow around `RootDir`, `Prefix`, and leading `ParentDir` cases is easier to understand and reuse if needed elsewhere.
- The manual component handling currently distinguishes `RootDir` and `Prefix` but does not explicitly document how Windows-specific prefixes (e.g. UNC paths or drive letters) should behave; adding a short code comment describing the intended behavior for these cases will make future changes to this logic safer.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
This PR hardens the TypeScript/JavaScript dependency extractor’s fallback path normalization logic to avoid unsafe handling of .. (ParentDir) components during manual resolution, reducing the risk of path traversal via crafted import specifiers.
Changes:
- Update manual
..resolution inTypeScriptDependencyExtractor::resolve_module_pathto avoid poppingRootDir/Prefixand to preserve consecutive/leading..correctly. - Minor refactors/formatting adjustments in rule-engine and ast-engine modules (no intended behavior change).
- Add a
.jules/sentinel.mdnote documenting the issue and prevention guidance.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/rule-engine/src/rule/referent_rule.rs | Minor formatting change in Registration::read (no logic change). |
| crates/rule-engine/src/rule/mod.rs | Reformat defined_vars collection for readability. |
| crates/rule-engine/src/check_var.rs | Simplify function signatures by removing unnecessary lifetimes. |
| crates/flow/src/incremental/extractors/typescript.rs | Security fix: safer manual handling of ParentDir components during path normalization. |
| crates/ast-engine/src/tree_sitter/mod.rs | Small formatting/refactor in UTF-8 conversion and a test assertion. |
| .jules/sentinel.md | Add internal documentation describing the path traversal issue and mitigation. |
đź’ˇ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 🛡️ Sentinel: Fix path traversal in manual path resolution | ||
| // Prevent popping RootDir or Prefix, and preserve leading ParentDirs | ||
| if let Some(last) = components.last() { | ||
| match last { | ||
| std::path::Component::RootDir |
🚨 Severity: HIGH
đź’ˇ Vulnerability: In
crates/flow/src/incremental/extractors/typescript.rs, manual path resolution for imports poppingstd::path::Component::ParentDir(..) simply usedcomponents.pop(). This allows path traversal attacks, as it could popRootDirorPrefixcomponents, or discard existing consecutive..components, bypassing the base directory and potentially allowing arbitrary file access on the host system.🎯 Impact: An attacker could supply crafted import paths (e.g.,
../../../etc/passwd) that escape the intended base directory and access sensitive files on the system or server.đź”§ Fix: Explicitly check the last component before popping. If it's a
RootDirorPrefix, do not pop. If the components list is empty or already ends in aParentDir, correctly push the newParentDirto preserve relative traversal properly.âś… Verification: Ran
cargo test -p thread-flow --test extractor_typescript_testsandcargo test --workspaceto ensure no regressions and verify the path components logic handles standard operations safely.PR created automatically by Jules for task 11042287032616640001 started by @bashandbone
Summary by Sourcery
Harden TypeScript import path resolution against path traversal while performing minor API and documentation cleanups.
Bug Fixes:
Enhancements:
Documentation: