refactor: Use PathBuf for paths in flag parsing and whitelists (#3955)

* Use PathBuf for DenoSubcommand::Bundle's out_file
* Use PathBuf for DenoSubcommand::Format's files
* Use PathBuf for DenoSubcommand::Install's dir
* Use PathBuf for read/write whitelists

cli/compilers/ts.rs

@@ -170,7 +170,7 @@ fn req(
   request_type: msg::CompilerRequestType,
   root_names: Vec<String>,
   compiler_config: CompilerConfig,
-  out_file: Option<String>,
+  out_file: Option<PathBuf>,
   target: &str,
   bundle: bool,
 ) -> Buf {
@@ -275,7 +275,7 @@ impl TsCompiler {
     &self,
     global_state: GlobalState,
     module_name: String,
-    out_file: Option<String>,
+    out_file: Option<PathBuf>,
   ) -> Result<(), ErrBox> {
     debug!(
       "Invoking the compiler to bundle. module_name: {}",
@@ -743,7 +743,7 @@ mod tests {
       .bundle_async(
         state.clone(),
         module_name,
-        Some(String::from("$deno$/bundle.js")),
+        Some(PathBuf::from("$deno$/bundle.js")),
       )
       .await;
     assert!(result.is_ok());

cli/flags.rs

@@ -7,7 +7,7 @@ use clap::ArgMatches;
 use clap::SubCommand;
 use log::Level;
 use std::collections::HashSet;
-use std::path::Path;
+use std::path::{Path, PathBuf};
 
 /// Creates vector of strings, Vec<String>
 macro_rules! svec {
@@ -35,7 +35,7 @@ const TEST_RUNNER_URL: &str = std_url!("testing/runner.ts");
 pub enum DenoSubcommand {
   Bundle {
     source_file: String,
-    out_file: Option<String>,
+    out_file: Option<PathBuf>,
   },
   Completions {
     buf: Box<[u8]>,
@@ -48,14 +48,14 @@ pub enum DenoSubcommand {
   },
   Format {
     check: bool,
-    files: Option<Vec<String>>,
+    files: Option<Vec<PathBuf>>,
   },
   Help,
   Info {
     file: Option<String>,
   },
   Install {
-    dir: Option<String>,
+    dir: Option<PathBuf>,
     exe_name: String,
     module_url: String,
     args: Vec<String>,
@@ -87,10 +87,10 @@ pub struct DenoFlags {
   pub config_path: Option<String>,
   pub import_map_path: Option<String>,
   pub allow_read: bool,
-  pub read_whitelist: Vec<String>,
+  pub read_whitelist: Vec<PathBuf>,
   pub cache_blacklist: Vec<String>,
   pub allow_write: bool,
-  pub write_whitelist: Vec<String>,
+  pub write_whitelist: Vec<PathBuf>,
   pub allow_net: bool,
   pub net_whitelist: Vec<String>,
   pub allow_env: bool,
@@ -107,6 +107,14 @@ pub struct DenoFlags {
   pub lock_write: bool,
 }
 
+fn join_paths(whitelist: &[PathBuf], d: &str) -> String {
+  whitelist
+    .iter()
+    .map(|path| path.to_str().unwrap().to_string())
+    .collect::<Vec<String>>()
+    .join(d)
+}
+
 impl DenoFlags {
   /// Return list of permission arguments that are equivalent
   /// to the ones used to create `self`.
@@ -114,7 +122,7 @@ impl DenoFlags {
     let mut args = vec![];
 
     if !self.read_whitelist.is_empty() {
-      let s = format!("--allow-read={}", self.read_whitelist.join(","));
+      let s = format!("--allow-read={}", join_paths(&self.read_whitelist, ","));
       args.push(s);
     }
 
@@ -123,7 +131,8 @@ impl DenoFlags {
     }
 
     if !self.write_whitelist.is_empty() {
-      let s = format!("--allow-write={}", self.write_whitelist.join(","));
+      let s =
+        format!("--allow-write={}", join_paths(&self.write_whitelist, ","));
       args.push(s);
     }
 
@@ -297,7 +306,7 @@ fn types_parse(flags: &mut DenoFlags, _matches: &clap::ArgMatches) {
 fn fmt_parse(flags: &mut DenoFlags, matches: &clap::ArgMatches) {
   let maybe_files = match matches.values_of("files") {
     Some(f) => {
-      let files: Vec<String> = f.map(String::from).collect();
+      let files: Vec<PathBuf> = f.map(PathBuf::from).collect();
       Some(files)
     }
     None => None,
@@ -316,7 +325,7 @@ fn install_parse(flags: &mut DenoFlags, matches: &clap::ArgMatches) {
 
   let dir = if matches.is_present("dir") {
     let install_dir = matches.value_of("dir").unwrap();
-    Some(install_dir.to_string())
+    Some(PathBuf::from(install_dir))
   } else {
     None
   };
@@ -347,7 +356,7 @@ fn bundle_parse(flags: &mut DenoFlags, matches: &clap::ArgMatches) {
 
   let out_file = if let Some(out_file) = matches.value_of("out_file") {
     flags.allow_write = true;
-    Some(out_file.to_string())
+    Some(PathBuf::from(out_file))
   } else {
     None
   };
@@ -428,16 +437,10 @@ fn lock_args_parse(flags: &mut DenoFlags, matches: &clap::ArgMatches) {
   }
 }
 
-fn resolve_fs_whitelist(whitelist: &[String]) -> Vec<String> {
+fn resolve_fs_whitelist(whitelist: &[PathBuf]) -> Vec<PathBuf> {
   whitelist
     .iter()
-    .map(|raw_path| {
+    .map(|raw_path| resolve_from_cwd(Path::new(&raw_path)).unwrap())
-      resolve_from_cwd(Path::new(&raw_path))
-        .unwrap()
-        .to_str()
-        .unwrap()
-        .to_owned()
-    })
     .collect()
 }
 
@@ -998,8 +1001,8 @@ fn permission_args_parse(flags: &mut DenoFlags, matches: &clap::ArgMatches) {
   if matches.is_present("allow-read") {
     if matches.value_of("allow-read").is_some() {
       let read_wl = matches.values_of("allow-read").unwrap();
-      let raw_read_whitelist: Vec<String> =
+      let raw_read_whitelist: Vec<PathBuf> =
-        read_wl.map(std::string::ToString::to_string).collect();
+        read_wl.map(PathBuf::from).collect();
       flags.read_whitelist = resolve_fs_whitelist(&raw_read_whitelist);
       debug!("read whitelist: {:#?}", &flags.read_whitelist);
     } else {
@@ -1009,10 +1012,9 @@ fn permission_args_parse(flags: &mut DenoFlags, matches: &clap::ArgMatches) {
   if matches.is_present("allow-write") {
     if matches.value_of("allow-write").is_some() {
       let write_wl = matches.values_of("allow-write").unwrap();
-      let raw_write_whitelist: Vec<String> =
+      let raw_write_whitelist: Vec<PathBuf> =
-        write_wl.map(std::string::ToString::to_string).collect();
+        write_wl.map(PathBuf::from).collect();
-      flags.write_whitelist =
+      flags.write_whitelist = resolve_fs_whitelist(&raw_write_whitelist);
-        resolve_fs_whitelist(raw_write_whitelist.as_slice());
       debug!("write whitelist: {:#?}", &flags.write_whitelist);
     } else {
       flags.allow_write = true;
@@ -1376,7 +1378,10 @@ mod tests {
       DenoFlags {
         subcommand: DenoSubcommand::Format {
           check: false,
-          files: Some(svec!["script_1.ts", "script_2.ts"])
+          files: Some(vec![
+            PathBuf::from("script_1.ts"),
+            PathBuf::from("script_2.ts")
+          ])
         },
         ..DenoFlags::default()
       }
@@ -1544,23 +1549,19 @@ mod tests {
   #[test]
   fn allow_read_whitelist() {
     use tempfile::TempDir;
-    let temp_dir = TempDir::new().expect("tempdir fail");
+    let temp_dir = TempDir::new().expect("tempdir fail").path().to_path_buf();
-    let temp_dir_path = temp_dir.path().to_str().unwrap();
 
     let r = flags_from_vec_safe(svec![
       "deno",
       "run",
-      format!("--allow-read=.,{}", &temp_dir_path),
+      format!("--allow-read=.,{}", temp_dir.to_str().unwrap()),
       "script.ts"
     ]);
     assert_eq!(
       r.unwrap(),
       DenoFlags {
         allow_read: false,
-        read_whitelist: svec![
+        read_whitelist: vec![current_dir().unwrap(), temp_dir],
-          current_dir().unwrap().to_str().unwrap().to_owned(),
-          &temp_dir_path
-        ],
         subcommand: DenoSubcommand::Run {
           script: "script.ts".to_string(),
         },
@@ -1572,23 +1573,19 @@ mod tests {
   #[test]
   fn allow_write_whitelist() {
     use tempfile::TempDir;
-    let temp_dir = TempDir::new().expect("tempdir fail");
+    let temp_dir = TempDir::new().expect("tempdir fail").path().to_path_buf();
-    let temp_dir_path = temp_dir.path().to_str().unwrap();
 
     let r = flags_from_vec_safe(svec![
       "deno",
       "run",
-      format!("--allow-write=.,{}", &temp_dir_path),
+      format!("--allow-write=.,{}", temp_dir.to_str().unwrap()),
       "script.ts"
     ]);
     assert_eq!(
       r.unwrap(),
       DenoFlags {
         allow_write: false,
-        write_whitelist: svec![
+        write_whitelist: vec![current_dir().unwrap(), temp_dir],
-          current_dir().unwrap().to_str().unwrap().to_owned(),
-          &temp_dir_path
-        ],
         subcommand: DenoSubcommand::Run {
           script: "script.ts".to_string(),
         },
@@ -1677,7 +1674,7 @@ mod tests {
       DenoFlags {
         subcommand: DenoSubcommand::Bundle {
           source_file: "source.ts".to_string(),
-          out_file: Some("bundle.js".to_string()),
+          out_file: Some(PathBuf::from("bundle.js")),
         },
         allow_write: true,
         ..DenoFlags::default()
@@ -1868,7 +1865,7 @@ mod tests {
       r.unwrap(),
       DenoFlags {
         subcommand: DenoSubcommand::Install {
-          dir: Some("/usr/local/bin".to_string()),
+          dir: Some(PathBuf::from("/usr/local/bin")),
           exe_name: "file_server".to_string(),
           module_url: "https://deno.land/std/http/file_server.ts".to_string(),
           args: svec!["arg1", "arg2"],

cli/fmt.rs

@@ -147,11 +147,11 @@ fn format_source_files(
   );
 }
 
-fn get_matching_files(glob_paths: Vec<String>) -> Vec<PathBuf> {
+fn get_matching_files(glob_paths: Vec<PathBuf>) -> Vec<PathBuf> {
   let mut target_files = Vec::with_capacity(128);
 
   for path in glob_paths {
-    let files = glob::glob(&path)
+    let files = glob::glob(&path.to_str().unwrap())
       .expect("Failed to execute glob.")
       .filter_map(Result::ok);
     target_files.extend(files);
@@ -165,14 +165,14 @@ fn get_matching_files(glob_paths: Vec<String>) -> Vec<PathBuf> {
 /// First argument supports globs, and if it is `None`
 /// then the current directory is recursively walked.
 pub fn format_files(
-  maybe_files: Option<Vec<String>>,
+  maybe_files: Option<Vec<PathBuf>>,
   check: bool,
 ) -> Result<(), ErrBox> {
   // TODO: improve glob to look for tsx?/jsx? files only
-  let glob_paths = maybe_files.unwrap_or_else(|| vec!["**/*".to_string()]);
+  let glob_paths = maybe_files.unwrap_or_else(|| vec![PathBuf::from("**/*")]);
 
   for glob_path in glob_paths.iter() {
-    if glob_path == "-" {
+    if glob_path.to_str().unwrap() == "-" {
       // If the only given path is '-', format stdin.
       if glob_paths.len() == 1 {
         format_stdin(check);

cli/installer.rs

@@ -101,14 +101,14 @@ fn get_installer_dir() -> Result<PathBuf, Error> {
 
 pub fn install(
   flags: DenoFlags,
-  installation_dir: Option<String>,
+  installation_dir: Option<PathBuf>,
   exec_name: &str,
   module_url: &str,
   args: Vec<String>,
   force: bool,
 ) -> Result<(), Error> {
   let installation_dir = if let Some(dir) = installation_dir {
-    PathBuf::from(dir).canonicalize()?
+    dir.canonicalize()?
   } else {
     get_installer_dir()?
   };
@@ -245,7 +245,7 @@ mod tests {
     let temp_dir = TempDir::new().expect("tempdir fail");
     install(
       DenoFlags::default(),
-      Some(temp_dir.path().to_string_lossy().to_string()),
+      Some(temp_dir.path().to_path_buf()),
       "echo_test",
       "http://localhost:4545/cli/tests/echo_server.ts",
       vec![],
@@ -274,7 +274,7 @@ mod tests {
         allow_read: true,
         ..DenoFlags::default()
       },
-      Some(temp_dir.path().to_string_lossy().to_string()),
+      Some(temp_dir.path().to_path_buf()),
       "echo_test",
       "http://localhost:4545/cli/tests/echo_server.ts",
       vec!["--foobar".to_string()],
@@ -301,7 +301,7 @@ mod tests {
 
     install(
       DenoFlags::default(),
-      Some(temp_dir.path().to_string_lossy().to_string()),
+      Some(temp_dir.path().to_path_buf()),
       "echo_test",
       &local_module_str,
       vec![],

cli/lib.rs

@@ -72,6 +72,7 @@ use log::Level;
 use log::Metadata;
 use log::Record;
 use std::env;
+use std::path::PathBuf;
 
 static LOGGER: Logger = Logger;
 
@@ -258,7 +259,7 @@ async fn info_command(flags: DenoFlags, file: Option<String>) {
 
 async fn install_command(
   flags: DenoFlags,
-  dir: Option<String>,
+  dir: Option<PathBuf>,
   exe_name: String,
   module_url: String,
   args: Vec<String>,
@@ -331,7 +332,7 @@ async fn eval_command(flags: DenoFlags, code: String) {
 async fn bundle_command(
   flags: DenoFlags,
   source_file: String,
-  out_file: Option<String>,
+  out_file: Option<PathBuf>,
 ) {
   debug!(">>>>> bundle_async START");
   let source_file_specifier =
@@ -404,7 +405,7 @@ async fn run_script(flags: DenoFlags, script: String) {
   js_check(worker.execute("window.dispatchEvent(new Event('unload'))"));
 }
 
-async fn fmt_command(files: Option<Vec<String>>, check: bool) {
+async fn fmt_command(files: Option<Vec<PathBuf>>, check: bool) {
   if let Err(err) = fmt::format_files(files, check) {
     print_err_and_exit(err);
   }

cli/permissions.rs

@@ -102,9 +102,9 @@ impl Default for PermissionState {
 pub struct DenoPermissions {
   // Keep in sync with cli/js/permissions.ts
   pub allow_read: PermissionState,
-  pub read_whitelist: HashSet<String>,
+  pub read_whitelist: HashSet<PathBuf>,
   pub allow_write: PermissionState,
-  pub write_whitelist: HashSet<String>,
+  pub write_whitelist: HashSet<PathBuf>,
   pub allow_net: PermissionState,
   pub net_whitelist: HashSet<String>,
   pub allow_env: PermissionState,
@@ -349,10 +349,10 @@ fn log_perm_access(message: &str) {
   }
 }
 
-fn check_path_white_list(path: &Path, white_list: &HashSet<String>) -> bool {
+fn check_path_white_list(path: &Path, white_list: &HashSet<PathBuf>) -> bool {
   let mut path_buf = PathBuf::from(path);
   loop {
-    if white_list.contains(path_buf.to_str().unwrap()) {
+    if white_list.contains(&path_buf) {
       return true;
     }
     if !path_buf.pop() {
@@ -383,7 +383,11 @@ mod tests {
 
   #[test]
   fn check_paths() {
-    let whitelist = svec!["/a/specific/dir/name", "/a/specific", "/b/c"];
+    let whitelist = vec![
+      PathBuf::from("/a/specific/dir/name"),
+      PathBuf::from("/a/specific"),
+      PathBuf::from("/b/c"),
+    ];
 
     let perms = DenoPermissions::from_flags(&DenoFlags {
       read_whitelist: whitelist.clone(),
@@ -530,7 +534,7 @@ mod tests {
 
   #[test]
   fn test_permissions_request_read() {
-    let whitelist = svec!["/foo/bar"];
+    let whitelist = vec![PathBuf::from("/foo/bar")];
     let mut perms0 = DenoPermissions::from_flags(&DenoFlags {
       read_whitelist: whitelist.clone(),
       ..Default::default()
@@ -566,7 +570,7 @@ mod tests {
 
   #[test]
   fn test_permissions_request_write() {
-    let whitelist = svec!["/foo/bar"];
+    let whitelist = vec![PathBuf::from("/foo/bar")];
     let mut perms0 = DenoPermissions::from_flags(&DenoFlags {
       write_whitelist: whitelist.clone(),
       ..Default::default()

cli/tests/integration_tests.rs

@@ -108,7 +108,7 @@ fn installer_test_local_module_run() {
   let local_module_str = local_module.to_string_lossy();
   installer::install(
     DenoFlags::default(),
-    Some(temp_dir.path().to_string_lossy().to_string()),
+    Some(temp_dir.path().to_path_buf()),
     "echo_test",
     &local_module_str,
     vec!["hello".to_string()],
@@ -156,7 +156,7 @@ fn installer_test_remote_module_run() {
   let temp_dir = TempDir::new().expect("tempdir fail");
   installer::install(
     DenoFlags::default(),
-    Some(temp_dir.path().to_string_lossy().to_string()),
+    Some(temp_dir.path().to_path_buf()),
     "echo_test",
     "http://localhost:4545/cli/tests/echo.ts",
     vec!["hello".to_string()],