Skip to content

Commit b1369b5

Browse files
authored
fix: normalize paths in native resolver for .js → .ts remap (#600)
* fix: normalize paths in native resolver for .js → .ts extension remap Adds clean_path() to properly resolve .. components before extension probing. Also updates normalize_path() to use clean_path() internally. The JS-side defense-in-depth (path.normalize + remapJsToTs) was already in place; this fixes the root cause in the Rust native resolver. Fixes #592 Impact: 3 functions changed, 6 affected * fix: add clean_path tests, doc comment, and un-skip parity test (#600) - Add #[cfg(test)] module with unit tests for clean_path covering parent dir collapse, cur dir skip, absolute root, mixed segments, and the known excess-parent-dir limitation. - Document the silent-drop behavior of leading .. on empty base in the clean_path doc comment. - Un-skip the 'resolves parent directory traversal' parity test now that the native engine properly normalizes paths. Impact: 5 functions changed, 0 affected * fix: re-skip parity test until native binary is published (#600) The parity test requires the native binary to include the clean_path fix, but CI tests run against the pre-built binary. Re-skip with an updated comment explaining when to un-skip.
1 parent 5660f75 commit b1369b5

File tree

2 files changed

+72
-3
lines changed

2 files changed

+72
-3
lines changed

crates/codegraph-core/src/import_resolution.rs

Lines changed: 71 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,33 @@ fn file_exists(path: &str, known: Option<&HashSet<String>>) -> bool {
1010
known.map_or_else(|| Path::new(path).exists(), |set| set.contains(path))
1111
}
1212

13+
/// Resolve `.` and `..` components in a path without touching the filesystem.
14+
/// Unlike `PathBuf::components().collect()`, this properly collapses `..` by
15+
/// popping the previous component from the result.
16+
///
17+
/// NOTE: if the path begins with more `..` components than there are preceding
18+
/// components to pop (e.g. a purely relative `../../foo`), the excess `..`
19+
/// components are silently dropped. This function is therefore only correct
20+
/// when called on paths that have already been joined to a base directory with
21+
/// sufficient depth.
22+
fn clean_path(p: &Path) -> PathBuf {
23+
let mut result = PathBuf::new();
24+
for c in p.components() {
25+
match c {
26+
std::path::Component::ParentDir => {
27+
result.pop();
28+
}
29+
std::path::Component::CurDir => {}
30+
_ => result.push(c),
31+
}
32+
}
33+
result
34+
}
35+
1336
/// Normalize a path to use forward slashes and clean `.` / `..` segments
1437
/// (cross-platform consistency).
1538
fn normalize_path(p: &str) -> String {
16-
let cleaned: PathBuf = Path::new(p).components().collect();
39+
let cleaned = clean_path(Path::new(p));
1740
cleaned.display().to_string().replace('\\', "/")
1841
}
1942

@@ -111,7 +134,7 @@ fn resolve_import_path_inner(
111134

112135
// Relative import — normalize immediately to remove `.` / `..` segments
113136
let dir = Path::new(from_file).parent().unwrap_or(Path::new(""));
114-
let resolved: PathBuf = dir.join(import_source).components().collect();
137+
let resolved = clean_path(&dir.join(import_source));
115138
let resolved_str = resolved.display().to_string().replace('\\', "/");
116139

117140
// .js → .ts remap
@@ -246,3 +269,49 @@ pub fn resolve_imports_batch(
246269
})
247270
.collect()
248271
}
272+
273+
#[cfg(test)]
274+
mod tests {
275+
use super::*;
276+
277+
#[test]
278+
fn clean_path_collapses_parent_dirs() {
279+
assert_eq!(
280+
clean_path(Path::new("src/cli/commands/../../domain/graph/builder.js")),
281+
PathBuf::from("src/domain/graph/builder.js")
282+
);
283+
}
284+
285+
#[test]
286+
fn clean_path_skips_cur_dir() {
287+
assert_eq!(
288+
clean_path(Path::new("src/./foo.ts")),
289+
PathBuf::from("src/foo.ts")
290+
);
291+
}
292+
293+
#[test]
294+
fn clean_path_handles_absolute_root() {
295+
assert_eq!(
296+
clean_path(Path::new("/src/../foo.ts")),
297+
PathBuf::from("/foo.ts")
298+
);
299+
}
300+
301+
#[test]
302+
fn clean_path_mixed_segments() {
303+
assert_eq!(
304+
clean_path(Path::new("a/b/../c/./d/../e.js")),
305+
PathBuf::from("a/c/e.js")
306+
);
307+
}
308+
309+
#[test]
310+
fn clean_path_excess_parent_dirs_silently_dropped() {
311+
// Documents the known limitation: excess leading `..` are dropped
312+
assert_eq!(
313+
clean_path(Path::new("../../foo")),
314+
PathBuf::from("foo")
315+
);
316+
}
317+
}

tests/resolution/parity.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ describe.skipIf(!hasNative)('Import resolution parity', () => {
9090
assertParity(path.join(rootDir, 'index.js'), 'lodash', noAliases);
9191
});
9292

93-
// Known native engine limitation: does not normalize ../ or append extensions
93+
// Un-skip once the native binary is published with the clean_path fix (#600)
9494
it.skip('resolves parent directory traversal', () => {
9595
// Create a temporary nested structure
9696
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-parity-'));

0 commit comments

Comments
 (0)