refactor(node/npm): separate out permission check from npm resolvers (#27511)

Decouples permissions from the npm resolvers (towards moving the
resolvers out of the cli crate)

Author: dsherret

cli/factory.rs

@@ -71,6 +71,8 @@ use crate::npm::CliNpmResolver;
 use crate::npm::CliNpmResolverCreateOptions;
 use crate::npm::CliNpmResolverManagedSnapshotOption;
 use crate::npm::CreateInNpmPkgCheckerOptions;
+use crate::npm::NpmRegistryReadPermissionChecker;
+use crate::npm::NpmRegistryReadPermissionCheckerMode;
 use crate::resolver::CjsTracker;
 use crate::resolver::CliDenoResolver;
 use crate::resolver::CliNpmReqResolver;
@@ -941,6 +943,19 @@ impl CliFactory {
     let cjs_tracker = self.cjs_tracker()?.clone();
     let pkg_json_resolver = self.pkg_json_resolver().clone();
     let npm_req_resolver = self.npm_req_resolver().await?;
+    let npm_registry_permission_checker = {
+      let mode = if cli_options.use_byonm() {
+        NpmRegistryReadPermissionCheckerMode::Byonm
+      } else if let Some(node_modules_dir) = cli_options.node_modules_dir_path()
+      {
+        NpmRegistryReadPermissionCheckerMode::Local(node_modules_dir.clone())
+      } else {
+        NpmRegistryReadPermissionCheckerMode::Global(
+          self.npm_cache_dir()?.root_dir().to_path_buf(),
+        )
+      };
+      Arc::new(NpmRegistryReadPermissionChecker::new(self.sys(), mode))
+    };
 
     Ok(CliMainWorkerFactory::new(
       self.blob_store().clone(),
@@ -968,13 +983,14 @@ impl CliFactory {
         self.module_load_preparer().await?.clone(),
         node_code_translator.clone(),
         node_resolver.clone(),
-        npm_req_resolver.clone(),
-        cli_npm_resolver.clone(),
         NpmModuleLoader::new(
           self.cjs_tracker()?.clone(),
           fs.clone(),
           node_code_translator.clone(),
         ),
+        npm_registry_permission_checker,
+        npm_req_resolver.clone(),
+        cli_npm_resolver.clone(),
         self.parsed_source_cache().clone(),
         self.resolver().await?.clone(),
         self.sys(),

cli/module_loader.rs

@@ -66,6 +66,7 @@ use crate::graph_util::ModuleGraphBuilder;
 use crate::node::CliNodeCodeTranslator;
 use crate::node::CliNodeResolver;
 use crate::npm::CliNpmResolver;
+use crate::npm::NpmRegistryReadPermissionChecker;
 use crate::resolver::CjsTracker;
 use crate::resolver::CliNpmReqResolver;
 use crate::resolver::CliResolver;
@@ -221,9 +222,10 @@ struct SharedCliModuleLoaderState {
   module_load_preparer: Arc<ModuleLoadPreparer>,
   node_code_translator: Arc<CliNodeCodeTranslator>,
   node_resolver: Arc<CliNodeResolver>,
+  npm_module_loader: NpmModuleLoader,
+  npm_registry_permission_checker: Arc<NpmRegistryReadPermissionChecker>,
   npm_req_resolver: Arc<CliNpmReqResolver>,
   npm_resolver: Arc<dyn CliNpmResolver>,
-  npm_module_loader: NpmModuleLoader,
   parsed_source_cache: Arc<ParsedSourceCache>,
   resolver: Arc<CliResolver>,
   sys: CliSys,
@@ -281,9 +283,10 @@ impl CliModuleLoaderFactory {
     module_load_preparer: Arc<ModuleLoadPreparer>,
     node_code_translator: Arc<CliNodeCodeTranslator>,
     node_resolver: Arc<CliNodeResolver>,
+    npm_module_loader: NpmModuleLoader,
+    npm_registry_permission_checker: Arc<NpmRegistryReadPermissionChecker>,
     npm_req_resolver: Arc<CliNpmReqResolver>,
     npm_resolver: Arc<dyn CliNpmResolver>,
-    npm_module_loader: NpmModuleLoader,
     parsed_source_cache: Arc<ParsedSourceCache>,
     resolver: Arc<CliResolver>,
     sys: CliSys,
@@ -307,9 +310,10 @@ impl CliModuleLoaderFactory {
         module_load_preparer,
         node_code_translator,
         node_resolver,
+        npm_module_loader,
+        npm_registry_permission_checker,
         npm_req_resolver,
         npm_resolver,
-        npm_module_loader,
         parsed_source_cache,
         resolver,
         sys,
@@ -348,7 +352,10 @@ impl CliModuleLoaderFactory {
       sys: self.shared.sys.clone(),
       graph_container,
       in_npm_pkg_checker: self.shared.in_npm_pkg_checker.clone(),
-      npm_resolver: self.shared.npm_resolver.clone(),
+      npm_registry_permission_checker: self
+        .shared
+        .npm_registry_permission_checker
+        .clone(),
     });
     CreateModuleLoaderResult {
       module_loader,
@@ -1095,7 +1102,7 @@ struct CliNodeRequireLoader<TGraphContainer: ModuleGraphContainer> {
   sys: CliSys,
   graph_container: TGraphContainer,
   in_npm_pkg_checker: Arc<dyn InNpmPackageChecker>,
-  npm_resolver: Arc<dyn CliNpmResolver>,
+  npm_registry_permission_checker: Arc<NpmRegistryReadPermissionChecker>,
 }
 
 impl<TGraphContainer: ModuleGraphContainer> NodeRequireLoader
@@ -1112,7 +1119,9 @@ impl<TGraphContainer: ModuleGraphContainer> NodeRequireLoader
         return Ok(std::borrow::Cow::Borrowed(path));
       }
     }
-    self.npm_resolver.ensure_read_permission(permissions, path)
+    self
+      .npm_registry_permission_checker
+      .ensure_read_permission(permissions, path)
   }
 
   fn load_text_file_lossy(

cli/npm/byonm.rs

@@ -1,15 +1,12 @@
 // Copyright 2018-2025 the Deno authors. MIT license.
 
-use std::borrow::Cow;
 use std::path::Path;
 use std::sync::Arc;
 
-use deno_core::error::AnyError;
 use deno_core::serde_json;
 use deno_resolver::npm::ByonmNpmResolver;
 use deno_resolver::npm::ByonmNpmResolverCreateOptions;
 use deno_resolver::npm::CliNpmReqResolver;
-use deno_runtime::deno_node::NodePermissions;
 use deno_runtime::ops::process::NpmProcessStateProvider;
 use node_resolver::NpmPackageFolderResolver;
 
@@ -73,21 +70,6 @@ impl CliNpmResolver for CliByonmNpmResolver {
     self.root_node_modules_dir()
   }
 
-  fn ensure_read_permission<'a>(
-    &self,
-    permissions: &mut dyn NodePermissions,
-    path: &'a Path,
-  ) -> Result<Cow<'a, Path>, AnyError> {
-    if !path
-      .components()
-      .any(|c| c.as_os_str().to_ascii_lowercase() == "node_modules")
-    {
-      permissions.check_read_path(path).map_err(Into::into)
-    } else {
-      Ok(Cow::Borrowed(path))
-    }
-  }
-
   fn check_state_hash(&self) -> Option<u64> {
     // it is very difficult to determine the check state hash for byonm
     // so we just return None to signify check caching is not supported

cli/npm/managed/mod.rs

@@ -24,7 +24,6 @@ use deno_npm_cache::NpmCacheSetting;
 use deno_path_util::fs::canonicalize_path_maybe_not_exists;
 use deno_resolver::npm::CliNpmReqResolver;
 use deno_runtime::colors;
-use deno_runtime::deno_node::NodePermissions;
 use deno_runtime::ops::process::NpmProcessStateProvider;
 use deno_semver::package::PackageNv;
 use deno_semver::package::PackageReq;
@@ -167,6 +166,7 @@ fn create_inner(
     sys.clone(),
     npm_rc.clone(),
   ));
+
   let fs_resolver = create_npm_fs_resolver(
     npm_cache.clone(),
     &npm_install_deps_provider,
@@ -754,14 +754,6 @@ impl CliNpmResolver for ManagedCliNpmResolver {
     self.fs_resolver.node_modules_path()
   }
 
-  fn ensure_read_permission<'a>(
-    &self,
-    permissions: &mut dyn NodePermissions,
-    path: &'a Path,
-  ) -> Result<Cow<'a, Path>, AnyError> {
-    self.fs_resolver.ensure_read_permission(permissions, path)
-  }
-
   fn check_state_hash(&self) -> Option<u64> {
     // We could go further and check all the individual
     // npm packages, but that's probably overkill.

cli/npm/managed/resolvers/common.rs

@@ -3,30 +3,17 @@
 pub mod bin_entries;
 pub mod lifecycle_scripts;
 
-use std::borrow::Cow;
-use std::collections::HashMap;
-use std::io::ErrorKind;
 use std::path::Path;
 use std::path::PathBuf;
-use std::sync::Arc;
-use std::sync::Mutex;
 
 use async_trait::async_trait;
 use deno_ast::ModuleSpecifier;
-use deno_core::anyhow::Context;
 use deno_core::error::AnyError;
-use deno_core::futures;
-use deno_core::futures::StreamExt;
 use deno_npm::NpmPackageCacheFolderId;
 use deno_npm::NpmPackageId;
-use deno_npm::NpmResolutionPackage;
-use deno_runtime::deno_node::NodePermissions;
 use node_resolver::errors::PackageFolderResolveError;
-use sys_traits::FsCanonicalize;
 
 use super::super::PackageCaching;
-use crate::npm::CliNpmTarballCache;
-use crate::sys::CliSys;
 
 /// Part of the resolution that interacts with the file system.
 #[async_trait(?Send)]
@@ -63,101 +50,4 @@ pub trait NpmPackageFsResolver: Send + Sync {
     &self,
     caching: PackageCaching<'a>,
   ) -> Result<(), AnyError>;
-
-  #[must_use = "the resolved return value to mitigate time-of-check to time-of-use issues"]
-  fn ensure_read_permission<'a>(
-    &self,
-    permissions: &mut dyn NodePermissions,
-    path: &'a Path,
-  ) -> Result<Cow<'a, Path>, AnyError>;
-}
-
-#[derive(Debug)]
-pub struct RegistryReadPermissionChecker {
-  sys: CliSys,
-  cache: Mutex<HashMap<PathBuf, PathBuf>>,
-  registry_path: PathBuf,
-}
-
-impl RegistryReadPermissionChecker {
-  pub fn new(sys: CliSys, registry_path: PathBuf) -> Self {
-    Self {
-      sys,
-      registry_path,
-      cache: Default::default(),
-    }
-  }
-
-  pub fn ensure_registry_read_permission<'a>(
-    &self,
-    permissions: &mut dyn NodePermissions,
-    path: &'a Path,
-  ) -> Result<Cow<'a, Path>, AnyError> {
-    if permissions.query_read_all() {
-      return Ok(Cow::Borrowed(path)); // skip permissions checks below
-    }
-
-    // allow reading if it's in the node_modules
-    let is_path_in_node_modules = path.starts_with(&self.registry_path)
-      && path
-        .components()
-        .all(|c| !matches!(c, std::path::Component::ParentDir));
-
-    if is_path_in_node_modules {
-      let mut cache = self.cache.lock().unwrap();
-      let mut canonicalize =
-        |path: &Path| -> Result<Option<PathBuf>, AnyError> {
-          match cache.get(path) {
-            Some(canon) => Ok(Some(canon.clone())),
-            None => match self.sys.fs_canonicalize(path) {
-              Ok(canon) => {
-                cache.insert(path.to_path_buf(), canon.clone());
-                Ok(Some(canon))
-              }
-              Err(e) => {
-                if e.kind() == ErrorKind::NotFound {
-                  return Ok(None);
-                }
-                Err(AnyError::from(e)).with_context(|| {
-                  format!("failed canonicalizing '{}'", path.display())
-                })
-              }
-            },
-          }
-        };
-      if let Some(registry_path_canon) = canonicalize(&self.registry_path)? {
-        if let Some(path_canon) = canonicalize(path)? {
-          if path_canon.starts_with(registry_path_canon) {
-            return Ok(Cow::Owned(path_canon));
-          }
-        } else if path.starts_with(registry_path_canon)
-          || path.starts_with(&self.registry_path)
-        {
-          return Ok(Cow::Borrowed(path));
-        }
-      }
-    }
-
-    permissions.check_read_path(path).map_err(Into::into)
-  }
-}
-
-/// Caches all the packages in parallel.
-pub async fn cache_packages(
-  packages: &[NpmResolutionPackage],
-  tarball_cache: &Arc<CliNpmTarballCache>,
-) -> Result<(), AnyError> {
-  let mut futures_unordered = futures::stream::FuturesUnordered::new();
-  for package in packages {
-    futures_unordered.push(async move {
-      tarball_cache
-        .ensure_package(&package.id.nv, &package.dist)
-        .await
-    });
-  }
-  while let Some(result) = futures_unordered.next().await {
-    // surface the first error
-    result?;
-  }
-  Ok(())
 }