fix(compile/npm): ignore symlinks to non-existent paths in node_modul…

…es directory (denoland#21479)

Part of denoland#21476

cli/Cargo.toml

@@ -65,7 +65,7 @@ deno_emit = "=0.31.5"
 deno_graph = "=0.61.5"
 deno_lint = { version = "=0.52.2", features = ["docs"] }
 deno_lockfile.workspace = true
-deno_npm = "0.15.2"
+deno_npm = "0.15.3"
 deno_runtime = { workspace = true, features = ["dont_create_runtime_snapshot", "include_js_files_for_snapshotting"] }
 deno_semver = "0.5.1"
 deno_task_shell = "=0.14.0"

cli/standalone/binary.rs

@@ -568,9 +568,16 @@ impl<'a> DenoCompileBinaryWriter<'a> {
  }
 
  fn build_vfs(&self) -> Result<VfsBuilder, AnyError> {
+ fn maybe_warn_different_system(system_info: &NpmSystemInfo) {
+ if system_info != &NpmSystemInfo::default() {
+ log::warn!("{} The node_modules directory may be incompatible with the target system.", crate::colors::yellow("Warning"));
+ }
+ }
+
  match self.npm_resolver.as_inner() {
  InnerCliNpmResolverRef::Managed(npm_resolver) => {
  if let Some(node_modules_path) = npm_resolver.root_node_modules_path() {
+ maybe_warn_different_system(&self.npm_system_info);
  let mut builder = VfsBuilder::new(node_modules_path.clone())?;
  builder.add_dir_recursive(node_modules_path)?;
  Ok(builder)
@@ -593,6 +600,7 @@ impl<'a> DenoCompileBinaryWriter<'a> {
  }
  }
  InnerCliNpmResolverRef::Byonm(npm_resolver) => {
+ maybe_warn_different_system(&self.npm_system_info);
  // the root_node_modules directory will always exist for byonm
  let node_modules_path = npm_resolver.root_node_modules_path().unwrap();
  let parent_path = node_modules_path.parent().unwrap();

cli/standalone/virtual_fs.rs

@@ -94,27 +94,40 @@ impl VfsBuilder {
  } else if file_type.is_file() {
  self.add_file_at_path(&path)?;
  } else if file_type.is_symlink() {
- let target = util::fs::canonicalize_path(&path)
- .with_context(|| format!("Reading symlink {}", path.display()))?;
- if let Err(StripRootError { .. }) = self.add_symlink(&path, &target) {
- if target.is_file() {
- // this may change behavior, so warn the user about it
- log::warn!(
- "Symlink target is outside '{}'. Inlining symlink at '{}' to '{}' as file.",
- self.root_path.display(),
- path.display(),
- target.display(),
- );
- // inline the symlink and make the target file
- let file_bytes = std::fs::read(&target)
- .with_context(|| format!("Reading {}", path.display()))?;
- self.add_file(&path, file_bytes)?;
- } else {
+ match util::fs::canonicalize_path(&path) {
+ Ok(target) => {
+ if let Err(StripRootError { .. }) = self.add_symlink(&path, &target)
+ {
+ if target.is_file() {
+ // this may change behavior, so warn the user about it
+ log::warn!(
+ "{} Symlink target is outside '{}'. Inlining symlink at '{}' to '{}' as file.",
+ crate::colors::yellow("Warning"),
+ self.root_path.display(),
+ path.display(),
+ target.display(),
+ );
+ // inline the symlink and make the target file
+ let file_bytes = std::fs::read(&target)
+ .with_context(|| format!("Reading {}", path.display()))?;
+ self.add_file(&path, file_bytes)?;
+ } else {
+ log::warn!(
+ "{} Symlink target is outside '{}'. Excluding symlink at '{}' with target '{}'.",
+ crate::colors::yellow("Warning"),
+ self.root_path.display(),
+ path.display(),
+ target.display(),
+ );
+ }
+ }
+ }
+ Err(err) => {
  log::warn!(
- "Symlink target is outside '{}'. Excluding symlink at '{}' with target '{}'.",
- self.root_path.display(),
+ "{} Failed resolving symlink. Ignoring.\n Path: {}\n Message: {:#}",
+ crate::colors::yellow("Warning"),
  path.display(),
- target.display(),
+ err
  );
  }
  }

cli/tests/integration/compile_tests.rs

@@ -1062,6 +1062,44 @@ fn compile_node_modules_symlink_outside() {
  output.assert_matches_file("compile/node_modules_symlink_outside/main.out");
 }
 
+#[test]
+fn compile_node_modules_symlink_non_existent() {
+ let context = TestContextBuilder::for_npm().use_temp_cwd().build();
+ let temp_dir = context.temp_dir().path();
+ temp_dir.join("main.ts").write(
+ r#"import { getValue, setValue } from "npm:@denotest/esm-basic";
+setValue(4);
+console.log(getValue());"#,
+ );
+ let node_modules_dir = temp_dir.join("node_modules");
+ node_modules_dir.create_dir_all();
+ // create a symlink that points to a non_existent file
+ node_modules_dir.symlink_dir("non_existent", "folder");
+ // compile folder
+ let output = context
+ .new_command()
+ .args("compile --allow-read --node-modules-dir --output bin main.ts")
+ .run();
+ output.assert_exit_code(0);
+ output.assert_matches_text(
+ r#"Download http:https://localhost:4545/npm/registry/@denotest/esm-basic
+Download http:https://localhost:4545/npm/registry/@denotest/esm-basic/1.0.0.tgz
+Initialize @denotest/[email protected]
+Check file:https:///[WILDCARD]/main.ts
+Compile file:https:///[WILDCARD]/main.ts to [WILDCARD]
+Warning Failed resolving symlink. Ignoring.
+ Path: [WILDCARD]
+ Message: [WILDCARD])
+"#,
+ );
+
+ // run
+ let binary_path =
+ temp_dir.join(if cfg!(windows) { "bin.exe" } else { "bin" });
+ let output = context.new_command().name(binary_path).run();
+ output.assert_matches_text("4\n");
+}
+
 #[test]
 fn dynamic_imports_tmp_lit() {
  let context = TestContextBuilder::new().build();

cli/tests/testdata/compile/node_modules_symlink_outside/main_compile_file.out

@@ -1,2 +1,2 @@
 Compile file:https:///[WILDCARD]/node_modules_symlink_outside/main.ts to [WILDCARD]
-Symlink target is outside '[WILDCARD]node_modules_symlink_outside[WILDCARD]node_modules'. Inlining symlink at '[WILDCARD]node_modules_symlink_outside[WILDCARD]node_modules[WILDCARD]test.txt' to '[WILDCARD]node_modules_symlink_outside[WILDCARD]test.txt' as file.
+Warning Symlink target is outside '[WILDCARD]node_modules_symlink_outside[WILDCARD]node_modules'. Inlining symlink at '[WILDCARD]node_modules_symlink_outside[WILDCARD]node_modules[WILDCARD]test.txt' to '[WILDCARD]node_modules_symlink_outside[WILDCARD]test.txt' as file.

cli/tests/testdata/compile/node_modules_symlink_outside/main_compile_folder.out

@@ -3,4 +3,4 @@ Download http:https://localhost:4545/npm/registry/@denotest/esm-basic/1.0.0.tgz
 Initialize @denotest/[email protected]
 Check file:https:///[WILDCARD]/node_modules_symlink_outside/main.ts
 Compile file:https:///[WILDCARD]/node_modules_symlink_outside/main.ts to [WILDCARD]
-Symlink target is outside '[WILDCARD]node_modules_symlink_outside[WILDCARD]node_modules'. Excluding symlink at '[WILDCARD]node_modules_symlink_outside[WILDCARD]node_modules[WILDCARD]some_folder' with target '[WILDCARD]node_modules_symlink_outside[WILDCARD]some_folder'.
+Warning Symlink target is outside '[WILDCARD]node_modules_symlink_outside[WILDCARD]node_modules'. Excluding symlink at '[WILDCARD]node_modules_symlink_outside[WILDCARD]node_modules[WILDCARD]some_folder' with target '[WILDCARD]node_modules_symlink_outside[WILDCARD]some_folder'.

cli/tools/compile.rs

@@ -71,8 +71,9 @@ pub async fn compile(
  );
  validate_output_path(&output_path)?;
 
- let mut file = std::fs::File::create(&output_path)?;
- binary_writer
+ let mut file = std::fs::File::create(&output_path)
+ .with_context(|| format!("Opening file '{}'", output_path.display()))?;
+ let write_result = binary_writer
  .write_bin(
  &mut file,
  eszip,
@@ -81,8 +82,13 @@ pub async fn compile(
  cli_options,
  )
  .await
- .with_context(|| format!("Writing {}", output_path.display()))?;
+ .with_context(|| format!("Writing {}", output_path.display()));
  drop(file);
+ if let Err(err) = write_result {
+ // errored, so attempt to remove the output path
+ let _ = std::fs::remove_file(output_path);
+ return Err(err);
+ }
 
  // set it as executable
  #[cfg(unix)]