π‘οΈ Sentinel: [CRITICAL/HIGH] Fix path traversal normalization bug#302
π‘οΈ Sentinel: [CRITICAL/HIGH] Fix path traversal normalization bug#302bashandbone wants to merge 1 commit into
Conversation
Fixes a path normalization issue where popping components naively allowed relative traversals (`../`) to be silently ignored when reaching the root or when stacking multiple traversals, potentially bypassing bounds checks. 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 HIGH-severity path traversal normalization bug in the TypeScript dependency extractor by making ParentDir handling safe when the path buffer is empty or contains prior ParentDir components, and includes several small refactors and formatting cleanups in the rule engine, AST engine, and tests. Flow diagram for secure ParentDir path normalization in TypeScript dependency extractorflowchart TD
A[Start normalization over resolved.components] --> B[Iterate component]
B --> C{component kind}
C -->|ParentDir| D{last in components}
D -->|RootDir or Prefix| E[Leave components unchanged]
D -->|ParentDir or None| F[Push ParentDir into components]
D -->|Other| G[Pop last component]
C -->|CurDir| H[Skip component]
C -->|Other segment| I[Push component into components]
E --> J[Next component]
F --> J
G --> J
H --> J
I --> J
J --> B
B -->|no more components| K[Build normalized path from components]
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 found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="crates/flow/tests/d1_cache_integration.rs" line_range="171-174" />
<code_context>
.expect("Failed to create context without caching");
// Should compile and work without cache field
- assert!(
- true,
- "D1ExportContext created successfully without caching feature"
- );
}
}
</code_context>
<issue_to_address>
**issue (testing):** Add regression tests for the TypeScript path normalization `ParentDir` handling fix.
This change alters security-relevant `ParentDir` handling in `TypeScriptDependencyExtractor`, but there are no tests covering the new behavior. Please extend the `thread-flow` TypeScript extractor tests (e.g. `extractor_typescript_tests`) to cover:
- Attempts to traverse above root/prefix (e.g. `/../foo`, `C:\..\bar`), ensuring we donβt pop past the root/prefix.
- Multiple/leading `ParentDir` components (`../foo`, `../../bar`, `foo/../../baz`).
- Mixed `CurDir`/`ParentDir` sequences (`./../foo`, `foo/./../bar`).
- Cases where `ParentDir` appears before vs. after a pushed `RootDir`/`Prefix`.
These should fail with the old implementation and pass with the new one to lock in the fix.
</issue_to_address>Help me be more useful! Please click π or π on each comment and I'll use the feedback to improve your reviews.
| assert!( | ||
| true, | ||
| "D1ExportContext created successfully without caching feature" | ||
| ); |
There was a problem hiding this comment.
issue (testing): Add regression tests for the TypeScript path normalization ParentDir handling fix.
This change alters security-relevant ParentDir handling in TypeScriptDependencyExtractor, but there are no tests covering the new behavior. Please extend the thread-flow TypeScript extractor tests (e.g. extractor_typescript_tests) to cover:
- Attempts to traverse above root/prefix (e.g.
/../foo,C:\..\bar), ensuring we donβt pop past the root/prefix. - Multiple/leading
ParentDircomponents (../foo,../../bar,foo/../../baz). - Mixed
CurDir/ParentDirsequences (./../foo,foo/./../bar). - Cases where
ParentDirappears before vs. after a pushedRootDir/Prefix.
These should fail with the old implementation and pass with the new one to lock in the fix.
Component::ParentDirignored when popping from an empty path buffer.ParentDirhandling.cargo test -p thread-flow --test extractor_typescript_tests.PR created automatically by Jules for task 4834465002024166172 started by @bashandbone
Summary by Sourcery
Harden TypeScript path normalization to correctly handle parent directory components and prevent traversal beyond expected bounds.
Bug Fixes:
Enhancements:
Documentation:
Tests: