Repository navigation
fix(npm): reject symlinked package materialization dirs - #36191
Conversation
bartlomieju
left a comment
bartlomieju
left a comment
There was a problem hiding this comment.
Nice fix — the ensure_not_symlink before/after create_dir_all approach is right, and covering all three helpers with in-memory-fs regressions is great. Two things worth considering:
1. clone_dir_recursive_except_node_modules_child (local.rs) has the same vector but isn't hardened. It's used for patch packages and workspace members (local.rs:582, hoisted.rs:536) and still does let _ = fs_remove_dir_all(to) then fs_create_dir_all(to) then hard_link_file/fs_copy with no symlink check. On Linux, remove_dir_all on a symlinked to fails with NotADirectory and the error is discarded, so the symlink stays, create_dir_all succeeds (target is a dir), and package files get written through the redirected path — the exact write-through this PR closes in the other three helpers. Worth routing this destination through create_dir_all_no_symlink/ensure_not_symlink too so the fix is complete.
2. Minor: ensure_not_symlink/create_dir_all_no_symlink are duplicated verbatim in both libs/npm_cache/fs_util.rs and libs/npm_installer/fs.rs. Since npm_installer already depends on npm_cache (it calls deno_npm_cache::hard_link_dir_recursive), these could be exported once from npm_cache and reused rather than kept in two places where they can drift. Non-blocking.
## Summary - reject symlinked package materialization destinations - apply the check to both hard-link and copy paths - cover both helpers with in-memory filesystem regressions ## Details Package initialization previously treated an existing symlinked destination as a directory. Creating the destination and then hard-linking or copying package files could therefore write through that redirected path. Materialization now checks each destination directory before and after creation using symlink-aware metadata. Existing ordinary directories continue to work as before, while a symlinked destination returns an ordinary filesystem error. ## Validation - `./tools/format.js --check` - `cargo test -p deno_npm_cache hard_link_dir_recursive -- --nocapture` - `cargo test -p deno_npm_installer copy_dir_recursive -- --nocapture` AI assistance was used to help implement and test this change.
Summary
Details
Package initialization previously treated an existing symlinked destination
as a directory. Creating the destination and then hard-linking or copying
package files could therefore write through that redirected path.
Materialization now checks each destination directory before and after
creation using symlink-aware metadata. Existing ordinary directories continue
to work as before, while a symlinked destination returns an ordinary
filesystem error.
Validation
./tools/format.js --checkcargo test -p deno_npm_cache hard_link_dir_recursive -- --nocapturecargo test -p deno_npm_installer copy_dir_recursive -- --nocaptureAI assistance was used to help implement and test this change.