refactor: improve node permission checks (denoland#26028)

Does less work when requesting permissions with `-A`

Author: dsherret

cli/npm/byonm.rs

@@ -1,5 +1,6 @@
 // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.
 
+use std::borrow::Cow;
 use std::path::Path;
 use std::path::PathBuf;
 use std::sync::Arc;
@@ -32,18 +33,19 @@ pub type CliByonmNpmResolver = ByonmNpmResolver<CliDenoResolverFs>;
 struct CliByonmWrapper(Arc<CliByonmNpmResolver>);
 
 impl NodeRequireResolver for CliByonmWrapper {
-  fn ensure_read_permission(
+  fn ensure_read_permission<'a>(
     &self,
     permissions: &mut dyn NodePermissions,
-    path: &Path,
-  ) -> Result<(), AnyError> {
+    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)?;
+      permissions.check_read_path(path)
+    } else {
+      Ok(Cow::Borrowed(path))
     }
-    Ok(())
   }
 }
 

cli/npm/managed/mod.rs

@@ -1,5 +1,6 @@
 // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.
 
+use std::borrow::Cow;
 use std::path::Path;
 use std::path::PathBuf;
 use std::sync::Arc;
@@ -593,11 +594,11 @@ impl NpmResolver for ManagedCliNpmResolver {
 }
 
 impl NodeRequireResolver for ManagedCliNpmResolver {
-  fn ensure_read_permission(
+  fn ensure_read_permission<'a>(
     &self,
     permissions: &mut dyn NodePermissions,
-    path: &Path,
-  ) -> Result<(), AnyError> {
+    path: &'a Path,
+  ) -> Result<Cow<'a, Path>, AnyError> {
     self.fs_resolver.ensure_read_permission(permissions, path)
   }
 }

cli/npm/managed/resolvers/common.rs

@@ -3,6 +3,7 @@
 pub mod bin_entries;
 pub mod lifecycle_scripts;
 
+use std::borrow::Cow;
 use std::collections::HashMap;
 use std::io::ErrorKind;
 use std::path::Path;
@@ -62,11 +63,12 @@ pub trait NpmPackageFsResolver: Send + Sync {
 
   async fn cache_packages(&self) -> Result<(), AnyError>;
 
-  fn ensure_read_permission(
+  #[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: &Path,
-  ) -> Result<(), AnyError>;
+    path: &'a Path,
+  ) -> Result<Cow<'a, Path>, AnyError>;
 }
 
 #[derive(Debug)]
@@ -85,11 +87,15 @@ impl RegistryReadPermissionChecker {
     }
   }
 
-  pub fn ensure_registry_read_permission(
+  pub fn ensure_registry_read_permission<'a>(
     &self,
     permissions: &mut dyn NodePermissions,
-    path: &Path,
-  ) -> Result<(), AnyError> {
+    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
@@ -118,20 +124,20 @@ impl RegistryReadPermissionChecker {
             },
           }
         };
-      let Some(registry_path_canon) = canonicalize(&self.registry_path)? else {
-        return Ok(()); // not exists, allow reading
-      };
-      let Some(path_canon) = canonicalize(path)? else {
-        return Ok(()); // not exists, allow reading
-      };
-
-      if path_canon.starts_with(registry_path_canon) {
-        return Ok(());
+      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)?;
-    Ok(())
+    permissions.check_read_path(path)
   }
 }
 

cli/npm/managed/resolvers/global.rs

@@ -183,11 +183,11 @@ impl NpmPackageFsResolver for GlobalNpmPackageResolver {
     Ok(())
   }
 
-  fn ensure_read_permission(
+  fn ensure_read_permission<'a>(
     &self,
     permissions: &mut dyn NodePermissions,
-    path: &Path,
-  ) -> Result<(), AnyError> {
+    path: &'a Path,
+  ) -> Result<Cow<'a, Path>, AnyError> {
     self
       .registry_read_permission_checker
       .ensure_registry_read_permission(permissions, path)

cli/npm/managed/resolvers/local.rs

@@ -257,11 +257,11 @@ impl NpmPackageFsResolver for LocalNpmPackageResolver {
     .await
   }
 
-  fn ensure_read_permission(
+  fn ensure_read_permission<'a>(
     &self,
     permissions: &mut dyn NodePermissions,
-    path: &Path,
-  ) -> Result<(), AnyError> {
+    path: &'a Path,
+  ) -> Result<Cow<'a, Path>, AnyError> {
     self
       .registry_read_permission_checker
       .ensure_registry_read_permission(permissions, path)

ext/node/lib.rs

@@ -66,6 +66,7 @@ pub trait NodePermissions {
     &mut self,
     path: &'a Path,
   ) -> Result<Cow<'a, Path>, AnyError>;
+  fn query_read_all(&mut self) -> bool;
   fn check_sys(&mut self, kind: &str, api_name: &str) -> Result<(), AnyError>;
   #[must_use = "the resolved return value to mitigate time-of-check to time-of-use issues"]
   fn check_write_with_api_name(
@@ -103,6 +104,10 @@ impl NodePermissions for deno_permissions::PermissionsContainer {
     deno_permissions::PermissionsContainer::check_read_path(self, path, None)
   }
 
+  fn query_read_all(&mut self) -> bool {
+    deno_permissions::PermissionsContainer::query_read_all(self)
+  }
+
   #[inline(always)]
   fn check_write_with_api_name(
     &mut self,
@@ -124,11 +129,12 @@ pub type NodeRequireResolverRc =
   deno_fs::sync::MaybeArc<dyn NodeRequireResolver>;
 
 pub trait NodeRequireResolver: std::fmt::Debug + MaybeSend + MaybeSync {
-  fn ensure_read_permission(
+  #[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: &Path,
-  ) -> Result<(), AnyError>;
+    path: &'a Path,
+  ) -> Result<Cow<'a, Path>, AnyError>;
 }
 
 pub static NODE_ENV_VAR_ALLOWLIST: Lazy<HashSet<String>> = Lazy::new(|| {

ext/node/ops/require.rs

@@ -15,6 +15,7 @@ use deno_path_util::normalize_path;
 use node_resolver::NodeModuleKind;
 use node_resolver::NodeResolutionMode;
 use node_resolver::REQUIRE_CONDITIONS;
+use std::borrow::Cow;
 use std::cell::RefCell;
 use std::path::Path;
 use std::path::PathBuf;
@@ -25,10 +26,11 @@ use crate::NodeRequireResolverRc;
 use crate::NodeResolverRc;
 use crate::NpmResolverRc;
 
-fn ensure_read_permission<P>(
+#[must_use = "the resolved return value to mitigate time-of-check to time-of-use issues"]
+fn ensure_read_permission<'a, P>(
   state: &mut OpState,
-  file_path: &Path,
-) -> Result<(), AnyError>
+  file_path: &'a Path,
+) -> Result<Cow<'a, Path>, AnyError>
 where
   P: NodePermissions + 'static,
 {
@@ -107,7 +109,7 @@ where
     deno_path_util::normalize_path(current_dir.join(from))
   };
 
-  ensure_read_permission::<P>(state, &from)?;
+  let from = ensure_read_permission::<P>(state, &from)?;
 
   if cfg!(windows) {
     // return root node_modules when path is 'D:\\'.
@@ -129,7 +131,7 @@ where
   }
 
   let mut paths = Vec::with_capacity(from.components().count());
-  let mut current_path = from.as_path();
+  let mut current_path = from.as_ref();
   let mut maybe_parent = Some(current_path);
   while let Some(parent) = maybe_parent {
     if !parent.ends_with("node_modules") {
@@ -267,7 +269,7 @@ where
   P: NodePermissions + 'static,
 {
   let path = PathBuf::from(path);
-  ensure_read_permission::<P>(state, &path)?;
+  let path = ensure_read_permission::<P>(state, &path)?;
   let fs = state.borrow::<FileSystemRc>();
   if let Ok(metadata) = fs.stat_sync(&path) {
     if metadata.is_file {
@@ -290,7 +292,7 @@ where
   P: NodePermissions + 'static,
 {
   let path = PathBuf::from(request);
-  ensure_read_permission::<P>(state, &path)?;
+  let path = ensure_read_permission::<P>(state, &path)?;
   let fs = state.borrow::<FileSystemRc>();
   let canonicalized_path =
     deno_core::strip_unc_prefix(fs.realpath_sync(&path)?);
@@ -362,7 +364,7 @@ where
     if parent_id == "<repl>" || parent_id == "internal/preload" {
       let fs = state.borrow::<FileSystemRc>();
       if let Ok(cwd) = fs.cwd() {
-        ensure_read_permission::<P>(state, &cwd)?;
+        let cwd = ensure_read_permission::<P>(state, &cwd)?;
         return Ok(Some(cwd.to_string_lossy().into_owned()));
       }
     }
@@ -443,7 +445,7 @@ where
   P: NodePermissions + 'static,
 {
   let file_path = PathBuf::from(file_path);
-  ensure_read_permission::<P>(state, &file_path)?;
+  let file_path = ensure_read_permission::<P>(state, &file_path)?;
   let fs = state.borrow::<FileSystemRc>();
   Ok(fs.read_text_file_lossy_sync(&file_path, None)?)
 }
@@ -528,7 +530,7 @@ where
   P: NodePermissions + 'static,
 {
   let filename = PathBuf::from(filename);
-  ensure_read_permission::<P>(state, filename.parent().unwrap())?;
+  // permissions: allow reading the closest package.json files
   let node_resolver = state.borrow::<NodeResolverRc>().clone();
   node_resolver
     .get_closest_package_json_from_path(&filename)
@@ -567,7 +569,7 @@ where
   P: NodePermissions + 'static,
 {
   let referrer_path = PathBuf::from(&referrer_filename);
-  ensure_read_permission::<P>(state, &referrer_path)?;
+  let referrer_path = ensure_read_permission::<P>(state, &referrer_path)?;
   let node_resolver = state.borrow::<NodeResolverRc>();
   let Some(pkg) =
     node_resolver.get_closest_package_json_from_path(&referrer_path)?

ext/node/ops/worker_threads.rs

@@ -7,17 +7,19 @@ use deno_core::url::Url;
 use deno_core::OpState;
 use deno_fs::FileSystemRc;
 use node_resolver::NodeResolution;
+use std::borrow::Cow;
 use std::path::Path;
 use std::path::PathBuf;
 
 use crate::NodePermissions;
 use crate::NodeRequireResolverRc;
 use crate::NodeResolverRc;
 
-fn ensure_read_permission<P>(
+#[must_use = "the resolved return value to mitigate time-of-check to time-of-use issues"]
+fn ensure_read_permission<'a, P>(
   state: &mut OpState,
-  file_path: &Path,
-) -> Result<(), AnyError>
+  file_path: &'a Path,
+) -> Result<Cow<'a, Path>, AnyError>
 where
   P: NodePermissions + 'static,
 {
@@ -47,7 +49,7 @@ where
         "Relative path entries must start with '.' or '..'",
       ));
     }
-    ensure_read_permission::<P>(state, &path)?;
+    let path = ensure_read_permission::<P>(state, &path)?;
     let fs = state.borrow::<FileSystemRc>();
     let canonicalized_path =
       deno_core::strip_unc_prefix(fs.realpath_sync(&path)?);
@@ -57,7 +59,7 @@ where
   let url_path = url
     .to_file_path()
     .map_err(|e| generic_error(format!("URL to Path-String: {:#?}", e)))?;
-  ensure_read_permission::<P>(state, &url_path)?;
+  let url_path = ensure_read_permission::<P>(state, &url_path)?;
   let fs = state.borrow::<FileSystemRc>();
   if !fs.exists_sync(&url_path) {
     return Err(generic_error(format!("File not found [{:?}]", url_path)));