refactor: improve node permission checks (#26028)

Does less work when requesting permissions with `-A`

5 files changed, 40 insertions, 31 deletions

diff --git a/cli/npm/byonm.rs b/cli/npm/byonm.rs
index 02c2e6da8..fc095ab16 100644
--- a/cli/npm/byonm.rs
+++ b/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(())
   }
 }
 
diff --git a/cli/npm/managed/mod.rs b/cli/npm/managed/mod.rs
index 225cd6c29..ec50a9c65 100644
--- a/cli/npm/managed/mod.rs
+++ b/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)
   }
 }
diff --git a/cli/npm/managed/resolvers/common.rs b/cli/npm/managed/resolvers/common.rs
index 8df4debc5..867bb4168 100644
--- a/cli/npm/managed/resolvers/common.rs
+++ b/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)
   }
 }
 
diff --git a/cli/npm/managed/resolvers/global.rs b/cli/npm/managed/resolvers/global.rs
index ca5722c86..5be315e99 100644
--- a/cli/npm/managed/resolvers/global.rs
+++ b/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)
diff --git a/cli/npm/managed/resolvers/local.rs b/cli/npm/managed/resolvers/local.rs
index 258c9bf3d..63a972a43 100644
--- a/cli/npm/managed/resolvers/local.rs
+++ b/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)