Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .jules/sentinel.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
## 2025-02-14 - Fix Path Traversal Normalization Bug in File System Resolution
**Vulnerability:** In `crates/flow/src/incremental/extractors/typescript.rs` and potentially elsewhere, path normalization logic used `components.pop()` when encountering `Component::ParentDir`. When `components` was empty or ended with another `Component::ParentDir`, this ignored `../` components rather than accumulating them, leading to incorrect path resolution or potential path traversal issues outside expected bounds.
**Learning:** `Vec::pop()` silently returns `None` on an empty vector. Using it without checking the preceding components in a path navigation allows relative paths like `../../` to be collapsed incorrectly.
**Prevention:** Explicitly check the last component. If it's empty or also a `ParentDir`, append the `ParentDir` to accurately represent `../` navigation, or return an error if such paths are not allowed in the context.
10 changes: 6 additions & 4 deletions crates/ast-engine/src/tree_sitter/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -553,9 +553,8 @@ impl ContentExt for String {
let mut bytes = std::mem::take(self).into_bytes();
let original_len = bytes.len();
bytes.splice(safe_start..safe_end, full_inserted);
*self = Self::from_utf8(bytes).unwrap_or_else(|e| {
Self::from_utf8_lossy(&e.into_bytes()).into_owned()
});
*self = Self::from_utf8(bytes)
.unwrap_or_else(|e| Self::from_utf8_lossy(&e.into_bytes()).into_owned());

// We calculate new_end_byte using the difference in the new overall string length
// to correctly align the end offset, taking any potential replacement bytes from
Expand Down Expand Up @@ -791,7 +790,10 @@ mod test {

let tree2 = parse_lang(|p| p.parse(&src, Some(&tree)), &Tsx.get_ts_language())?;
let fresh_tree = parse(&src)?;
assert_eq!(tree2.root_node().to_sexp(), fresh_tree.root_node().to_sexp());
assert_eq!(
tree2.root_node().to_sexp(),
fresh_tree.root_node().to_sexp()
);
Ok(())
}
}
13 changes: 10 additions & 3 deletions crates/flow/src/incremental/extractors/typescript.rs
Original file line number Diff line number Diff line change
Expand Up @@ -807,9 +807,16 @@ impl TypeScriptDependencyExtractor {
let mut components = Vec::new();
for component in resolved.components() {
match component {
std::path::Component::ParentDir => {
components.pop();
}
std::path::Component::ParentDir => match components.last() {
Some(std::path::Component::RootDir)
| Some(std::path::Component::Prefix(_)) => {}
Some(std::path::Component::ParentDir) | None => {
components.push(component);
}
_ => {
components.pop();
}
},
std::path::Component::CurDir => {}
_ => components.push(component),
}
Expand Down
4 changes: 0 additions & 4 deletions crates/flow/tests/d1_cache_integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -168,9 +168,5 @@ mod d1_no_cache_tests {
.expect("Failed to create context without caching");

// Should compile and work without cache field
assert!(
true,
"D1ExportContext created successfully without caching feature"
);
Comment on lines -171 to -174

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
20 changes: 10 additions & 10 deletions crates/rule-engine/src/check_var.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ pub enum CheckHint<'r> {
pub fn check_rule_with_hint<'r>(
rule: &'r Rule,
utils: &'r RuleRegistration,
constraints: &'r RapidMap<thread_ast_engine::meta_var::MetaVariableID, Rule>,
transform: &'r Option<Transform>,
constraints: &RapidMap<thread_ast_engine::meta_var::MetaVariableID, Rule>,
transform: &Option<Transform>,
fixer: &Vec<Fixer>,
hint: CheckHint<'r>,
) -> RResult<()> {
Expand Down Expand Up @@ -56,8 +56,8 @@ pub fn check_rule_with_hint<'r>(
fn check_vars_in_rewriter<'r>(
rule: &'r Rule,
utils: &'r RuleRegistration,
constraints: &'r RapidMap<thread_ast_engine::meta_var::MetaVariableID, Rule>,
transform: &'r Option<Transform>,
constraints: &RapidMap<thread_ast_engine::meta_var::MetaVariableID, Rule>,
transform: &Option<Transform>,
fixer: &Vec<Fixer>,
upper_var: &RapidSet<String>,
) -> RResult<()> {
Expand Down Expand Up @@ -85,8 +85,8 @@ fn check_utils_defined(
fn check_vars<'r>(
rule: &'r Rule,
utils: &'r RuleRegistration,
constraints: &'r RapidMap<thread_ast_engine::meta_var::MetaVariableID, Rule>,
transform: &'r Option<Transform>,
constraints: &RapidMap<thread_ast_engine::meta_var::MetaVariableID, Rule>,
transform: &Option<Transform>,
fixer: &Vec<Fixer>,
) -> RResult<()> {
let vars = get_vars_from_rules(rule, utils);
Expand All @@ -104,9 +104,9 @@ fn get_vars_from_rules<'r>(rule: &'r Rule, utils: &'r RuleRegistration) -> Rapid
vars
}

fn check_var_in_constraints<'r>(
fn check_var_in_constraints(
mut vars: RapidSet<String>,
constraints: &'r RapidMap<thread_ast_engine::meta_var::MetaVariableID, Rule>,
constraints: &RapidMap<thread_ast_engine::meta_var::MetaVariableID, Rule>,
) -> RResult<RapidSet<String>> {
for rule in constraints.values() {
for var in rule.defined_vars() {
Expand All @@ -125,9 +125,9 @@ fn check_var_in_constraints<'r>(
Ok(vars)
}

fn check_var_in_transform<'r>(
fn check_var_in_transform(
mut vars: RapidSet<String>,
transform: &'r Option<Transform>,
transform: &Option<Transform>,
) -> RResult<RapidSet<String>> {
let Some(transform) = transform else {
return Ok(vars);
Expand Down
6 changes: 5 additions & 1 deletion crates/rule-engine/src/rule/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,11 @@ impl Rule {

pub fn defined_vars(&self) -> RapidSet<String> {
match self {
Rule::Pattern(p) => p.defined_vars().into_iter().map(|s| s.to_string()).collect(),
Rule::Pattern(p) => p
.defined_vars()
.into_iter()
.map(|s| s.to_string())
.collect(),
Rule::Kind(_) => RapidSet::default(),
Rule::Regex(_) => RapidSet::default(),
Rule::NthChild(n) => n.defined_vars(),
Expand Down
5 changes: 1 addition & 4 deletions crates/rule-engine/src/rule/referent_rule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,7 @@ impl<R> Clone for Registration<R> {

impl<R> Registration<R> {
fn read(&self) -> Arc<RapidMap<String, R>> {
self.0
.read()
.unwrap_or_else(|e| e.into_inner())
.clone()
self.0.read().unwrap_or_else(|e| e.into_inner()).clone()
}
pub(crate) fn contains_key(&self, key: &str) -> bool {
self.read().contains_key(key)
Expand Down