🛡️ Sentinel: [CRITICAL/HIGH] Fix path traversal in file resolution#187
🛡️ Sentinel: [CRITICAL/HIGH] Fix path traversal in file resolution#187bashandbone wants to merge 1 commit intomainfrom
Conversation
- Updated path component popping logic during `..` (ParentDir) handling in `typescript.rs` to ensure relative paths aren't dangerously truncated to arbitrary system paths. - Added Sentinel journal entry about path normalization vulnerability and pattern prevention. - Ran formatting and clippy rules to bring files into compliance. 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 GuideFixes a critical path traversal vulnerability in manual path normalization for TypeScript dependency resolution by preventing Sequence diagram for TypeScript dependency path resolution on canonicalization failuresequenceDiagram
participant Extractor as TypeScriptDependencyExtractor
participant FS as FileSystem
participant Resolver as ManualPathNormalizer
Extractor->>FS: canonicalize(resolved_path)
FS-->>Extractor: Error (path does not exist)
Extractor->>Resolver: normalize(resolved_path.components())
loop for each component in components
Resolver->>Resolver: match component
alt Component::ParentDir
Resolver->>Resolver: inspect components.last()
alt last is Prefix or RootDir
Resolver->>Resolver: ignore ParentDir
else last is ParentDir or None
Resolver->>Resolver: push ParentDir
else last is normal component
Resolver->>Resolver: pop last component
end
else Component::CurDir
Resolver->>Resolver: skip component
else other component
Resolver->>Resolver: push component
end
end
Resolver-->>Extractor: normalized_path
Extractor->>Extractor: resolved = normalized_path
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:
- The manual
Component::ParentDirhandling in the TypeScript path resolution is subtle; consider extracting this logic into a small helper (e.g.,normalize_components(...)) so the prefix/root/parent behavior is clearly documented in one place and can be reused consistently if other code paths need similar normalization.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The manual `Component::ParentDir` handling in the TypeScript path resolution is subtle; consider extracting this logic into a small helper (e.g., `normalize_components(...)`) so the prefix/root/parent behavior is clearly documented in one place and can be reused consistently if other code paths need similar normalization.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
Updates the incremental TypeScript dependency resolver to harden manual path normalization against path traversal edge cases (specifically preventing .. from popping RootDir/Windows Prefix), with a small set of formatting and signature cleanups across the rule engine and AST engine.
Changes:
- Hardened manual
Component::ParentDirhandling in TypeScript module path resolution to avoid poppingRootDir/Prefix. - Simplified/cleaned up some Rust formatting and removed unused lifetime parameters.
- Added a Sentinel write-up documenting the traversal issue and the prevention pattern.
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/flow/src/incremental/extractors/typescript.rs | Fixes manual path normalization so .. can’t remove root/prefix boundaries. |
| crates/rule-engine/src/rule/referent_rule.rs | Formatting simplification in Registration::read(). |
| crates/rule-engine/src/rule/mod.rs | Formatting-only change in Rule::defined_vars(). |
| crates/rule-engine/src/check_var.rs | Removes unused lifetimes from helper function signatures. |
| crates/ast-engine/src/tree_sitter/mod.rs | Formatting-only changes (string conversion and test assert formatting). |
| .jules/sentinel.md | Adds documentation describing the vulnerability and prevention guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ## 2025-02-18 - Prevent Path Traversal by blocking parent popping of prefix/root in path component manipulation | ||
| **Vulnerability:** In `crates/flow/src/incremental/extractors/typescript.rs` and potentially elsewhere, path normalization popped standard components blindly when encountering `Component::ParentDir`. This could allow relative paths to evaluate to dangerous arbitrary paths. | ||
| **Learning:** Path normalization routines built on top of `std::path::Component` iterations must verify the previous component before popping it for a parent directory escape. `Component::ParentDir` should push to the component stack if the stack is empty, only has another `ParentDir`, or if the last element is `Prefix` or `RootDir` it should be ignored. | ||
| **Prevention:** In loops managing components, match on `components.last()` during a `ParentDir` to ensure we do not `pop` root directories or prefixes. |
🚨 Severity: CRITICAL
💡 Vulnerability: When manually resolving file paths with
std::path::Component,Component::ParentDirwas unconditionally popping the last component of the vector. If a malicious or malformed string evaluates with parent navigations that hitPrefix(C:\) orRootDir(/), it incorrectly popped those security boundaries off the stack, enabling potential path traversal.🎯 Impact: Allowed maliciously crafted input paths to escape bounds, converting safe absolute paths into relative arbitrary accesses.
🔧 Fix: Changed path component traversal logic to match the end of the
componentsvector; explicitly ignoring pops if the top of stack is a root or prefix, and safely accumulating parent traversal for relative structures.✅ Verification: Validated that test cases passing malformed absolute traversal inputs are resolved safely without directory escape. Ran both
cargo fmtandcargo clippyand ran the relevant TypeScript extraction tests and library tests locally.PR created automatically by Jules for task 16129905665493744072 started by @bashandbone
Summary by Sourcery
Fix unsafe path normalization in TypeScript dependency extraction to prevent path traversal when resolving non-canonicalized file paths.
Bug Fixes:
Enhancements:
Documentation: