fix(permissions): resolved allow run should be canonicalized

cli/args/flags.rs

@@ -699,7 +699,9 @@ impl PermissionFlags {
         if command_name.is_empty() {
           bail!("Empty command name not allowed in --allow-run=...")
         }
-        let command_path_result = which::which(command_name);
+        let command_path_result = which::which(command_name)
+          .map_err(AnyError::from)
+          .and_then(|path| canonicalize_path(&path).map_err(AnyError::from));
         match command_path_result {
           Ok(command_path) => new_allow_run.push(command_path),
           Err(err) => {

runtime/permissions/lib.rs

@@ -12,6 +12,7 @@ use deno_core::serde::Deserialize;
 use deno_core::serde::Deserializer;
 use deno_core::serde::Serialize;
 use deno_core::serde_json;
+use deno_core::strip_unc_prefix;
 use deno_core::unsync::sync::AtomicFlag;
 use deno_core::url;
 use deno_core::url::Url;
@@ -871,16 +872,7 @@ pub struct RunPathQuery<'a> {
 pub enum RunDescriptorArg {
   Name(String),
   Path(PathBuf),
-}
-
-#[derive(Clone, Eq, PartialEq, Hash, Debug)]
-pub enum RunDescriptor {
-  /// Warning: You may want to construct with `RunDescriptor::from()` for case
-  /// handling.
-  Name(String),
-  /// Warning: You may want to construct with `RunDescriptor::from()` for case
-  /// handling.
-  Path(PathBuf),
+  NoMatch,
 }
 
 impl From<String> for RunDescriptorArg {
@@ -891,16 +883,43 @@ impl From<String> for RunDescriptorArg {
     #[cfg(windows)]
     let is_path = is_path || s.contains('\\') || Path::new(&s).is_absolute();
     if is_path {
-      Self::Path(resolve_from_cwd(Path::new(&s)).unwrap())
+      PathBuf::from(&s).into()
     } else {
       match which::which(&s) {
-        Ok(path) => Self::Path(path),
-        Err(_) => Self::Name(s),
+        Ok(path) => path.into(),
+        Err(_) => Self::NoMatch,
       }
     }
   }
 }
 
+impl From<PathBuf> for RunDescriptorArg {
+  fn from(p: PathBuf) -> Self {
+    #[cfg(windows)]
+    let p = PathBuf::from(p.to_string_lossy().to_lowercase());
+    match resolve_from_cwd(&p) {
+      // ok, won't ever get here if not using a real file system
+      #[allow(clippy::disallowed_methods)]
+      Ok(p) => match p.canonicalize() {
+        Ok(path) => Self::Path(strip_unc_prefix(path)),
+        Err(_) => Self::Path(p),
+      },
+      Err(_) => Self::NoMatch,
+    }
+  }
+}
+
+#[derive(Clone, Eq, PartialEq, Hash, Debug)]
+pub enum RunDescriptor {
+  /// Warning: You may want to construct with `RunDescriptor::from()` for case
+  /// handling.
+  Name(String),
+  /// Warning: You may want to construct with `RunDescriptor::from()` for case
+  /// handling.
+  Path(PathBuf),
+  NoMatch,
+}
+
 impl Descriptor for RunDescriptor {
   type Arg = RunDescriptorArg;
 
@@ -928,44 +947,63 @@ impl Descriptor for RunDescriptor {
 
 impl From<String> for RunDescriptor {
   fn from(s: String) -> Self {
-    #[cfg(windows)]
-    let s = s.to_lowercase();
+    s.as_str().into()
+  }
+}
+
+impl<'a> From<&'a str> for RunDescriptor {
+  fn from(s: &'a str) -> Self {
+    let s = if cfg!(windows) {
+      Cow::Owned(s.to_lowercase())
+    } else {
+      Cow::Borrowed(s)
+    };
     let is_path = s.contains('/');
     #[cfg(windows)]
-    let is_path = is_path || s.contains('\\') || Path::new(&s).is_absolute();
+    let is_path =
+      is_path || s.contains('\\') || Path::new(s.as_ref()).is_absolute();
     if is_path {
-      Self::Path(resolve_from_cwd(Path::new(&s)).unwrap())
+      PathBuf::from(s.as_ref()).into()
     } else {
-      match which::which(&s) {
-        Ok(path) => Self::Path(path),
-        Err(_) => Self::Name(s),
+      match which::which(s.as_ref()) {
+        Ok(path) => path.into(),
+        Err(_) => Self::NoMatch,
       }
     }
   }
 }
 
 impl From<PathBuf> for RunDescriptor {
   fn from(p: PathBuf) -> Self {
-    #[cfg(windows)]
-    let p = PathBuf::from(p.to_string_lossy().to_string().to_lowercase());
-    Self::Path(resolve_from_cwd(&p).unwrap())
+    p.as_path().into()
   }
 }
 
-impl std::fmt::Display for RunDescriptor {
-  fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-    match self {
-      RunDescriptor::Name(s) => f.write_str(s),
-      RunDescriptor::Path(p) => f.write_str(&p.display().to_string()),
+impl<'a> From<&'a Path> for RunDescriptor {
+  fn from(p: &'a Path) -> Self {
+    let p = if cfg!(windows) {
+      Cow::Owned(PathBuf::from(p.to_string_lossy().to_lowercase()))
+    } else {
+      Cow::Borrowed(p)
+    };
+    match resolve_from_cwd(p.as_ref()) {
+      // ok, won't ever get here if not using a real file system
+      #[allow(clippy::disallowed_methods)]
+      Ok(p) => match p.canonicalize() {
+        Ok(path) => Self::Path(strip_unc_prefix(path)),
+        Err(_) => Self::Path(p),
+      },
+      Err(_) => Self::NoMatch,
     }
   }
 }
 
-impl AsRef<Path> for RunDescriptor {
-  fn as_ref(&self) -> &Path {
+impl std::fmt::Display for RunDescriptor {
+  fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
     match self {
-      RunDescriptor::Name(s) => s.as_ref(),
-      RunDescriptor::Path(s) => s.as_ref(),
+      RunDescriptor::Name(s) => f.write_str(s),
+      RunDescriptor::Path(p) => f.write_str(&p.display().to_string()),
+      RunDescriptor::NoMatch => f.write_str("<error>"),
     }
   }
 }
@@ -1309,20 +1347,19 @@ impl UnaryPermission<SysDescriptor> {
 impl UnaryPermission<RunDescriptor> {
   pub fn query(&self, cmd: Option<&str>) -> PermissionState {
     self.query_desc(
-      cmd.map(|c| RunDescriptor::from(c.to_string())).as_ref(),
+      cmd.map(RunDescriptor::from).as_ref(),
       AllowPartial::TreatAsPartialGranted,
     )
   }
 
   pub fn request(&mut self, cmd: Option<&str>) -> PermissionState {
-    self.request_desc(
-      cmd.map(|c| RunDescriptor::from(c.to_string())).as_ref(),
-      || Some(cmd?.to_string()),
-    )
+    self.request_desc(cmd.map(RunDescriptor::from).as_ref(), || {
+      Some(cmd?.to_string())
+    })
   }
 
   pub fn revoke(&mut self, cmd: Option<&str>) -> PermissionState {
-    self.revoke_desc(cmd.map(|c| RunDescriptor::from(c.to_string())).as_ref())
+    self.revoke_desc(cmd.map(RunDescriptor::from).as_ref())
   }
 
   pub fn check(
@@ -1332,12 +1369,9 @@ impl UnaryPermission<RunDescriptor> {
   ) -> Result<(), AnyError> {
     debug_assert!(cmd.resolved.is_absolute());
     skip_check_if_is_permission_fully_granted!(self);
-    self.check_desc(
-      Some(&RunDescriptor::Path(cmd.resolved.to_path_buf())),
-      false,
-      api_name,
-      || Some(format!("\"{}\"", cmd.requested)),
-    )
+    self.check_desc(Some(&cmd.resolved.into()), false, api_name, || {
+      Some(format!("\"{}\"", cmd.requested))
+    })
   }
 
   pub fn check_all(&mut self, api_name: Option<&str>) -> Result<(), AnyError> {
@@ -1987,6 +2021,7 @@ fn parse_run_list(
       .map(|arg| match arg {
         RunDescriptorArg::Name(s) => RunDescriptor::Name(s.clone()),
         RunDescriptorArg::Path(l) => RunDescriptor::Path(l.clone()),
+        RunDescriptorArg::NoMatch => RunDescriptor::NoMatch,
       })
       .collect(),
   )
@@ -3503,18 +3538,15 @@ mod tests {
       &mut main_perms,
       ChildPermissionsArg {
         read: ChildUnaryPermissionArg::Granted,
-        run: ChildUnaryPermissionArg::GrantedList(svec!["foo", "bar"]),
+        env: ChildUnaryPermissionArg::GrantedList(svec!["foo", "bar"]),
         ..ChildPermissionsArg::none()
       },
     )
     .unwrap();
     assert_eq!(main_perms, worker_perms);
     assert_eq!(
-      main_perms.run.granted_list,
-      HashSet::from([
-        RunDescriptor::Name("bar".to_owned()),
-        RunDescriptor::Name("foo".to_owned())
-      ])
+      main_perms.env.granted_list,
+      HashSet::from([EnvDescriptor::new("bar"), EnvDescriptor::new("foo")])
     );
   }
 

tests/specs/run/allow_run_allowlist_resolution/__test__.jsonc

@@ -1,4 +1,5 @@
 {
+  "tempDir": true,
   "args": "run --quiet -A main.ts",
   "output": "main.out",
   "envs": {

tests/specs/run/allow_run_allowlist_resolution/main.out

@@ -1,15 +1,37 @@
+--- name, on PATH ---
 PermissionStatus { state: "granted", onchange: null }
 PermissionStatus { state: "granted", onchange: null }
 PermissionStatus { state: "prompt", onchange: null }
 PermissionStatus { state: "granted", onchange: null }
----
-Info Failed to resolve 'deno' for allow-run: cannot find binary path
+PermissionStatus { state: "granted", onchange: null }
+--- name, no PATH ---
+Info Failed to resolve 'binary' for allow-run: cannot find binary path
+PermissionStatus { state: "prompt", onchange: null }
+PermissionStatus { state: "prompt", onchange: null }
 PermissionStatus { state: "prompt", onchange: null }
 PermissionStatus { state: "prompt", onchange: null }
 PermissionStatus { state: "prompt", onchange: null }
+--- path, on PATH ---
+PermissionStatus { state: "granted", onchange: null }
+PermissionStatus { state: "granted", onchange: null }
+PermissionStatus { state: "prompt", onchange: null }
+PermissionStatus { state: "granted", onchange: null }
+PermissionStatus { state: "granted", onchange: null }
+--- path, not on PATH (same as above) ---
+PermissionStatus { state: "granted", onchange: null }
+PermissionStatus { state: "granted", onchange: null }
+PermissionStatus { state: "prompt", onchange: null }
+PermissionStatus { state: "granted", onchange: null }
+PermissionStatus { state: "granted", onchange: null }
+--- path, dir symlink on PATH (same as above) ---
+PermissionStatus { state: "granted", onchange: null }
+PermissionStatus { state: "granted", onchange: null }
 PermissionStatus { state: "prompt", onchange: null }
----
+PermissionStatus { state: "granted", onchange: null }
+PermissionStatus { state: "granted", onchange: null }
+--- symlink path, dir symlink on PATH (same as above) ---
 PermissionStatus { state: "granted", onchange: null }
 PermissionStatus { state: "granted", onchange: null }
 PermissionStatus { state: "prompt", onchange: null }
 PermissionStatus { state: "granted", onchange: null }
+PermissionStatus { state: "granted", onchange: null }