fix(npm): canonicalize filename before returning (#18948)

This commit changes how paths for npm packages are handled,
by canonicalizing them when resolving. This is done so that instead
of returning
"node_modules/<package_name>@<version>/node_modules/<dep>/index.js"
(which is a symlink) we "node_modules/<dep>@<dep_version>/index.js.

Fixes https://github.com/denoland/deno/issues/18924
Fixes https://github.com/bluwy/create-vite-extra/issues/31

---------

Co-authored-by: David Sherret <dsherret@gmail.com>

cli/npm/resolvers/local.rs

@@ -91,7 +91,11 @@ impl LocalNpmPackageResolver {
     specifier: &ModuleSpecifier,
   ) -> Result<PathBuf, AnyError> {
     match self.maybe_resolve_folder_for_specifier(specifier) {
-      Some(path) => Ok(path),
+      // Canonicalize the path so it's not pointing to the symlinked directory
+      // in `node_modules` directory of the referrer.
+      Some(path) => {
+        Ok(deno_core::strip_unc_prefix(self.fs.canonicalize(&path)?))
+      }
       None => bail!("could not find npm package for '{}'", specifier),
     }
   }

cli/tests/integration/npm_tests.rs

@@ -156,6 +156,16 @@ itest!(mixed_case_package_name_local_dir {
   temp_cwd: true,
 });
 
+itest!(local_dir_resolves_symlinks {
+  args: "run -A index.js",
+  output: "npm/local_dir_resolves_symlinks/index.out",
+  exit_code: 0,
+  envs: env_vars_for_npm_tests(),
+  cwd: Some("npm/local_dir_resolves_symlinks/"),
+  copy_temp_dir: Some("npm/local_dir_resolves_symlinks/"),
+  http_server: true,
+});
+
 // FIXME(bartlomieju): npm: specifiers are not handled in dynamic imports
 // at the moment
 // itest!(dynamic_import {

cli/tests/testdata/npm/local_dir_resolves_symlinks/index.js

@@ -0,0 +1,3 @@
+import * as d from "define-properties";
+
+console.log(typeof d.default === "function", "it works");

cli/tests/testdata/npm/local_dir_resolves_symlinks/index.out

@@ -0,0 +1,2 @@
+Download [WILDCARD]
+true it works

cli/tests/testdata/npm/local_dir_resolves_symlinks/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "foo",
+  "type": "module",
+  "dependencies": {
+    "define-properties": "^1.2.0"
+  }
+}

cli/util/fs.rs

@@ -81,11 +81,7 @@ pub fn write_file_2<T: AsRef<[u8]>>(
 
 /// Similar to `std::fs::canonicalize()` but strips UNC prefixes on Windows.
 pub fn canonicalize_path(path: &Path) -> Result<PathBuf, Error> {
-  let path = path.canonicalize()?;
+  Ok(deno_core::strip_unc_prefix(path.canonicalize()?))
-  #[cfg(windows)]
-  return Ok(strip_unc_prefix(path));
-  #[cfg(not(windows))]
-  return Ok(path);
 }
 
 /// Canonicalizes a path which might be non-existent by going up the
@@ -117,47 +113,6 @@ pub fn canonicalize_path_maybe_not_exists(
   }
 }
 
-#[cfg(windows)]
-fn strip_unc_prefix(path: PathBuf) -> PathBuf {
-  use std::path::Component;
-  use std::path::Prefix;
-
-  let mut components = path.components();
-  match components.next() {
-    Some(Component::Prefix(prefix)) => {
-      match prefix.kind() {
-        // \\?\device
-        Prefix::Verbatim(device) => {
-          let mut path = PathBuf::new();
-          path.push(format!(r"\\{}\", device.to_string_lossy()));
-          path.extend(components.filter(|c| !matches!(c, Component::RootDir)));
-          path
-        }
-        // \\?\c:\path
-        Prefix::VerbatimDisk(_) => {
-          let mut path = PathBuf::new();
-          path.push(prefix.as_os_str().to_string_lossy().replace(r"\\?\", ""));
-          path.extend(components);
-          path
-        }
-        // \\?\UNC\hostname\share_name\path
-        Prefix::VerbatimUNC(hostname, share_name) => {
-          let mut path = PathBuf::new();
-          path.push(format!(
-            r"\\{}\{}\",
-            hostname.to_string_lossy(),
-            share_name.to_string_lossy()
-          ));
-          path.extend(components.filter(|c| !matches!(c, Component::RootDir)));
-          path
-        }
-        _ => path,
-      }
-    }
-    _ => path,
-  }
-}
-
 pub fn resolve_from_cwd(path: &Path) -> Result<PathBuf, AnyError> {
   let resolved_path = if path.is_absolute() {
     path.to_owned()
@@ -921,41 +876,6 @@ mod tests {
     assert_eq!(result, expected);
   }
 
-  #[cfg(windows)]
-  #[test]
-  fn test_strip_unc_prefix() {
-    run_test(r"C:\", r"C:\");
-    run_test(r"C:\test\file.txt", r"C:\test\file.txt");
-
-    run_test(r"\\?\C:\", r"C:\");
-    run_test(r"\\?\C:\test\file.txt", r"C:\test\file.txt");
-
-    run_test(r"\\.\C:\", r"\\.\C:\");
-    run_test(r"\\.\C:\Test\file.txt", r"\\.\C:\Test\file.txt");
-
-    run_test(r"\\?\UNC\localhost\", r"\\localhost");
-    run_test(r"\\?\UNC\localhost\c$\", r"\\localhost\c$");
-    run_test(
-      r"\\?\UNC\localhost\c$\Windows\file.txt",
-      r"\\localhost\c$\Windows\file.txt",
-    );
-    run_test(r"\\?\UNC\wsl$\deno.json", r"\\wsl$\deno.json");
-
-    run_test(r"\\?\server1", r"\\server1");
-    run_test(r"\\?\server1\e$\", r"\\server1\e$\");
-    run_test(
-      r"\\?\server1\e$\test\file.txt",
-      r"\\server1\e$\test\file.txt",
-    );
-
-    fn run_test(input: &str, expected: &str) {
-      assert_eq!(
-        strip_unc_prefix(PathBuf::from(input)),
-        PathBuf::from(expected)
-      );
-    }
-  }
-
   #[tokio::test]
   async fn lax_fs_lock() {
     let temp_dir = TempDir::new();

core/lib.rs

@@ -17,6 +17,7 @@ mod ops;
 mod ops_builtin;
 mod ops_builtin_v8;
 mod ops_metrics;
+mod path;
 mod realm;
 mod resources;
 mod runtime;
@@ -101,6 +102,7 @@ pub use crate::ops_builtin::op_resources;
 pub use crate::ops_builtin::op_void_async;
 pub use crate::ops_builtin::op_void_sync;
 pub use crate::ops_metrics::OpsTracker;
+pub use crate::path::strip_unc_prefix;
 pub use crate::realm::JsRealm;
 pub use crate::resources::AsyncResult;
 pub use crate::resources::Resource;

core/path.rs

@@ -0,0 +1,91 @@
+// Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
+
+use std::path::PathBuf;
+
+#[cfg(not(windows))]
+#[inline]
+pub fn strip_unc_prefix(path: PathBuf) -> PathBuf {
+  path
+}
+
+/// Strips the unc prefix (ex. \\?\) from Windows paths.
+#[cfg(windows)]
+pub fn strip_unc_prefix(path: PathBuf) -> PathBuf {
+  use std::path::Component;
+  use std::path::Prefix;
+
+  let mut components = path.components();
+  match components.next() {
+    Some(Component::Prefix(prefix)) => {
+      match prefix.kind() {
+        // \\?\device
+        Prefix::Verbatim(device) => {
+          let mut path = PathBuf::new();
+          path.push(format!(r"\\{}\", device.to_string_lossy()));
+          path.extend(components.filter(|c| !matches!(c, Component::RootDir)));
+          path
+        }
+        // \\?\c:\path
+        Prefix::VerbatimDisk(_) => {
+          let mut path = PathBuf::new();
+          path.push(prefix.as_os_str().to_string_lossy().replace(r"\\?\", ""));
+          path.extend(components);
+          path
+        }
+        // \\?\UNC\hostname\share_name\path
+        Prefix::VerbatimUNC(hostname, share_name) => {
+          let mut path = PathBuf::new();
+          path.push(format!(
+            r"\\{}\{}\",
+            hostname.to_string_lossy(),
+            share_name.to_string_lossy()
+          ));
+          path.extend(components.filter(|c| !matches!(c, Component::RootDir)));
+          path
+        }
+        _ => path,
+      }
+    }
+    _ => path,
+  }
+}
+
+#[cfg(test)]
+mod test {
+  #[cfg(windows)]
+  #[test]
+  fn test_strip_unc_prefix() {
+    use std::path::PathBuf;
+
+    run_test(r"C:\", r"C:\");
+    run_test(r"C:\test\file.txt", r"C:\test\file.txt");
+
+    run_test(r"\\?\C:\", r"C:\");
+    run_test(r"\\?\C:\test\file.txt", r"C:\test\file.txt");
+
+    run_test(r"\\.\C:\", r"\\.\C:\");
+    run_test(r"\\.\C:\Test\file.txt", r"\\.\C:\Test\file.txt");
+
+    run_test(r"\\?\UNC\localhost\", r"\\localhost");
+    run_test(r"\\?\UNC\localhost\c$\", r"\\localhost\c$");
+    run_test(
+      r"\\?\UNC\localhost\c$\Windows\file.txt",
+      r"\\localhost\c$\Windows\file.txt",
+    );
+    run_test(r"\\?\UNC\wsl$\deno.json", r"\\wsl$\deno.json");
+
+    run_test(r"\\?\server1", r"\\server1");
+    run_test(r"\\?\server1\e$\", r"\\server1\e$\");
+    run_test(
+      r"\\?\server1\e$\test\file.txt",
+      r"\\server1\e$\test\file.txt",
+    );
+
+    fn run_test(input: &str, expected: &str) {
+      assert_eq!(
+        super::strip_unc_prefix(PathBuf::from(input)),
+        PathBuf::from(expected)
+      );
+    }
+  }
+}

ext/fs/std_fs.rs

@@ -647,15 +647,7 @@ fn metadata_to_fsstat(metadata: fs::Metadata) -> FsStat {
 }
 
 fn realpath(path: impl AsRef<Path>) -> FsResult<PathBuf> {
-  let canonicalized_path = path.as_ref().canonicalize()?;
+  Ok(deno_core::strip_unc_prefix(path.as_ref().canonicalize()?))
-  #[cfg(windows)]
-  let canonicalized_path = PathBuf::from(
-    canonicalized_path
-      .display()
-      .to_string()
-      .trim_start_matches("\\\\?\\"),
-  );
-  Ok(canonicalized_path)
 }
 
 fn read_dir(path: impl AsRef<Path>) -> FsResult<Vec<FsDirEntry>> {

ext/node/ops/require.rs

@@ -287,15 +287,7 @@ where
   let path = PathBuf::from(request);
   ensure_read_permission::<Env::P>(state, &path)?;
   let fs = state.borrow::<Arc<dyn NodeFs>>();
-  let mut canonicalized_path = fs.canonicalize(&path)?;
+  let canonicalized_path = deno_core::strip_unc_prefix(fs.canonicalize(&path)?);
-  if cfg!(windows) {
-    canonicalized_path = PathBuf::from(
-      canonicalized_path
-        .display()
-        .to_string()
-        .trim_start_matches("\\\\?\\"),
-    );
-  }
   Ok(canonicalized_path.to_string_lossy().to_string())
 }
 

ext/node/polyfills/01_require.js

@@ -861,9 +861,11 @@ Module.prototype.load = function (filename) {
     throw Error("Module already loaded");
   }
 
-  this.filename = filename;
+  // Canonicalize the path so it's not pointing to the symlinked directory
+  // in `node_modules` directory of the referrer.
+  this.filename = ops.op_require_real_path(filename);
   this.paths = Module._nodeModulePaths(
-    pathDirname(filename),
+    pathDirname(this.filename),
   );
   const extension = findLongestRegisteredExtension(filename);
   // allow .mjs to be overriden

runtime/fs_util.rs

@@ -10,16 +10,7 @@ use std::path::PathBuf;
 
 /// Similar to `std::fs::canonicalize()` but strips UNC prefixes on Windows.
 pub fn canonicalize_path(path: &Path) -> Result<PathBuf, Error> {
-  let mut canonicalized_path = path.canonicalize()?;
+  Ok(deno_core::strip_unc_prefix(path.canonicalize()?))
-  if cfg!(windows) {
-    canonicalized_path = PathBuf::from(
-      canonicalized_path
-        .display()
-        .to_string()
-        .trim_start_matches("\\\\?\\"),
-    );
-  }
-  Ok(canonicalized_path)
 }
 
 #[inline]