🛡️ Sentinel: [CRITICAL] Fix path traversal in manual path normalization#217
🛡️ Sentinel: [CRITICAL] Fix path traversal in manual path normalization#217bashandbone wants to merge 1 commit intomainfrom
Conversation
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 critical path traversal vulnerability in the TypeScript dependency extractor’s manual path normalization by hardening handling of Flow diagram for secure ParentDir handling in manual path normalizationflowchart TD
Start[ParentDir component encountered] --> CheckEmpty[Is components list empty?]
CheckEmpty -->|Yes| PushParentEmpty[Push ParentDir onto components]
PushParentEmpty --> End[Continue to next component]
CheckEmpty -->|No| CheckLastParent[Does last component equal ParentDir?]
CheckLastParent -->|Yes| PushParentChain[Push ParentDir onto components]
PushParentChain --> End
CheckLastParent -->|No| CheckBoundary[Is last component RootDir or Prefix?]
CheckBoundary -->|Yes| KeepBoundary[Do not pop; keep boundary]
KeepBoundary --> End
CheckBoundary -->|No| PopComponent[Pop last component from components]
PopComponent --> End
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
ParentDirhandling block repeatedly callscomponents.last()and re-pattern-matches it; consider bindinglet last = components.last();once and derivingis_parent/is_rootfrom that to simplify the logic and reduce the chance of subtle discrepancies. - If the intent is to preserve leading
..segments only for relative paths, it might be clearer to explicitly distinguish absolute vs. relative paths (or current boundary state) in this normalization function, rather than relying solely on theis_empty/is_parentchecks when deciding whether to push or popParentDir.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `ParentDir` handling block repeatedly calls `components.last()` and re-pattern-matches it; consider binding `let last = components.last();` once and deriving `is_parent` / `is_root` from that to simplify the logic and reduce the chance of subtle discrepancies.
- If the intent is to preserve leading `..` segments only for relative paths, it might be clearer to explicitly distinguish absolute vs. relative paths (or current boundary state) in this normalization function, rather than relying solely on the `is_empty`/`is_parent` checks when deciding whether to push or pop `ParentDir`.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 addresses a path traversal vulnerability in the TypeScript dependency extractor’s manual path normalization fallback (used when canonicalize() fails), ensuring .. handling can’t pop past RootDir/Prefix or silently erase traversal at the start of relative paths.
Changes:
- Hardened manual
std::path::Componentnormalization to treatParentDirsafely around root/prefix boundaries. - Added a Sentinel note documenting the vulnerability, learning, 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 |
Updates manual path component reduction logic to prevent boundary-escaping behavior when resolving ... |
.jules/sentinel.md |
Adds a security “sentinel” write-up capturing the issue and mitigation guidance. |
Comments suppressed due to low confidence (1)
crates/flow/src/incremental/extractors/typescript.rs:831
- The new manual
ParentDirhandling is a security-critical normalization change but there’s no regression test covering the previously-vulnerable cases (e.g., canonicalize() failing + excessive..that would previously pop past RootDir/Prefix or incorrectly erase leading..on relative paths). Please add a test that exercisesresolve_module_pathwith such module specifiers and asserts the normalized path preserves boundaries (RootDir/Prefix) and/or retains leading..when appropriate.
// Normalize the path to resolve ../ components
if let Ok(canonical) = resolved.canonicalize() {
resolved = canonical;
} else {
// If canonicalize fails (file doesn't exist), manually resolve
let mut components = Vec::new();
for component in resolved.components() {
match component {
std::path::Component::ParentDir => {
let is_empty = components.is_empty();
let is_parent = matches!(
components.last(),
Some(std::path::Component::ParentDir)
);
let is_root = matches!(
components.last(),
Some(std::path::Component::RootDir | std::path::Component::Prefix(_))
);
if is_empty || is_parent {
components.push(component);
} else if !is_root {
components.pop();
}
}
std::path::Component::CurDir => {}
_ => components.push(component),
}
}
resolved = components.iter().collect();
}
đź’ˇ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ## 2026-05-09 - Prevent Path Traversal in Manual Path Resolution | ||
| **Vulnerability:** The TypeScript dependency extractor (`crates/flow/src/incremental/extractors/typescript.rs`) manually normalized paths without preventing `ParentDir` components from popping `RootDir` or `Prefix`, allowing paths to escape the intended directory. | ||
| **Learning:** Manual path normalization (e.g., when `canonicalize()` fails) must strictly handle standard library boundaries like `RootDir` and `Prefix` to prevent path traversal outside of absolute or project boundaries. | ||
| **Prevention:** Always check if the path component being popped represents a boundary, or use robust external path normalization libraries instead of rolling custom loops with `std::path::Component`. |
🚨 Severity: CRITICAL
đź’ˇ Vulnerability: Path traversal vulnerability in
TypeScriptDependencyExtractor. Whencanonicalize()fails, manual path normalization blindly popped components when encountering..(ParentDir), potentially poppingRootDirorPrefixand escaping boundary restrictions, or ignoring..entirely at the root of a relative path.🎯 Impact: An attacker could potentially extract dependencies or access files outside of the intended directory context via maliciously crafted import paths containing excessive
..components.đź”§ Fix: Updated the manual
std::path::Componentloop to securely handleParentDirby: (1) pushing it if the current component list is empty or already ends inParentDir, and (2) not popping if the current end isRootDirorPrefix.âś… Verification: Ran
cargo check -p thread-flowand verified functionality viacargo test -p thread-flow --test extractor_typescript_tests. Both compile and pass perfectly.PR created automatically by Jules for task 1072408770329239754 started by @bashandbone
Summary by Sourcery
Harden TypeScript dependency extraction path normalization to prevent directory traversal and document the incident and remediation in Sentinel notes.
Bug Fixes:
..components to escape root or prefix boundaries in the TypeScript dependency extractor.Documentation: