From 701ce9b3342647cf01cb23c4fc28bc99ce0aa8c1 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Tue, 11 Feb 2020 09:29:36 +0000 Subject: [PATCH] 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 | 6 +-- cli/flags.rs | 81 ++++++++++++++++------------------ cli/fmt.rs | 10 ++--- cli/installer.rs | 10 ++--- cli/lib.rs | 7 +-- cli/permissions.rs | 18 +++++--- cli/tests/integration_tests.rs | 4 +- 7 files changed, 69 insertions(+), 67 deletions(-) diff --git a/cli/compilers/ts.rs b/cli/compilers/ts.rs index d1fb3a6ace..d0c0d87c0c 100644 --- a/cli/compilers/ts.rs +++ b/cli/compilers/ts.rs @@ -170,7 +170,7 @@ fn req( request_type: msg::CompilerRequestType, root_names: Vec, compiler_config: CompilerConfig, - out_file: Option, + out_file: Option, target: &str, bundle: bool, ) -> Buf { @@ -275,7 +275,7 @@ impl TsCompiler { &self, global_state: GlobalState, module_name: String, - out_file: Option, + out_file: Option, ) -> 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()); diff --git a/cli/flags.rs b/cli/flags.rs index a8c51093e5..7f02d70661 100644 --- a/cli/flags.rs +++ b/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 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, + out_file: Option, }, Completions { buf: Box<[u8]>, @@ -48,14 +48,14 @@ pub enum DenoSubcommand { }, Format { check: bool, - files: Option>, + files: Option>, }, Help, Info { file: Option, }, Install { - dir: Option, + dir: Option, exe_name: String, module_url: String, args: Vec, @@ -87,10 +87,10 @@ pub struct DenoFlags { pub config_path: Option, pub import_map_path: Option, pub allow_read: bool, - pub read_whitelist: Vec, + pub read_whitelist: Vec, pub cache_blacklist: Vec, pub allow_write: bool, - pub write_whitelist: Vec, + pub write_whitelist: Vec, pub allow_net: bool, pub net_whitelist: Vec, 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::>() + .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 = f.map(String::from).collect(); + let files: Vec = 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 { +fn resolve_fs_whitelist(whitelist: &[PathBuf]) -> Vec { whitelist .iter() - .map(|raw_path| { - resolve_from_cwd(Path::new(&raw_path)) - .unwrap() - .to_str() - .unwrap() - .to_owned() - }) + .map(|raw_path| resolve_from_cwd(Path::new(&raw_path)).unwrap()) .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 = - read_wl.map(std::string::ToString::to_string).collect(); + let raw_read_whitelist: Vec = + 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 = - write_wl.map(std::string::ToString::to_string).collect(); - flags.write_whitelist = - resolve_fs_whitelist(raw_write_whitelist.as_slice()); + let raw_write_whitelist: Vec = + write_wl.map(PathBuf::from).collect(); + flags.write_whitelist = resolve_fs_whitelist(&raw_write_whitelist); 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_path = temp_dir.path().to_str().unwrap(); + let temp_dir = TempDir::new().expect("tempdir fail").path().to_path_buf(); 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![ - current_dir().unwrap().to_str().unwrap().to_owned(), - &temp_dir_path - ], + read_whitelist: vec![current_dir().unwrap(), temp_dir], 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_path = temp_dir.path().to_str().unwrap(); + let temp_dir = TempDir::new().expect("tempdir fail").path().to_path_buf(); 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![ - current_dir().unwrap().to_str().unwrap().to_owned(), - &temp_dir_path - ], + write_whitelist: vec![current_dir().unwrap(), temp_dir], 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"], diff --git a/cli/fmt.rs b/cli/fmt.rs index 390203f14a..f02df560a4 100644 --- a/cli/fmt.rs +++ b/cli/fmt.rs @@ -147,11 +147,11 @@ fn format_source_files( ); } -fn get_matching_files(glob_paths: Vec) -> Vec { +fn get_matching_files(glob_paths: Vec) -> Vec { 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) -> Vec { /// First argument supports globs, and if it is `None` /// then the current directory is recursively walked. pub fn format_files( - maybe_files: Option>, + maybe_files: Option>, 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); diff --git a/cli/installer.rs b/cli/installer.rs index 64fe2b4161..b1a795f99c 100644 --- a/cli/installer.rs +++ b/cli/installer.rs @@ -101,14 +101,14 @@ fn get_installer_dir() -> Result { pub fn install( flags: DenoFlags, - installation_dir: Option, + installation_dir: Option, exec_name: &str, module_url: &str, args: Vec, 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![], diff --git a/cli/lib.rs b/cli/lib.rs index 4e100127b0..f6d12d21d6 100644 --- a/cli/lib.rs +++ b/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) { async fn install_command( flags: DenoFlags, - dir: Option, + dir: Option, exe_name: String, module_url: String, args: Vec, @@ -331,7 +332,7 @@ async fn eval_command(flags: DenoFlags, code: String) { async fn bundle_command( flags: DenoFlags, source_file: String, - out_file: Option, + out_file: Option, ) { 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>, check: bool) { +async fn fmt_command(files: Option>, check: bool) { if let Err(err) = fmt::format_files(files, check) { print_err_and_exit(err); } diff --git a/cli/permissions.rs b/cli/permissions.rs index 98b7e7a33a..868f0e0090 100644 --- a/cli/permissions.rs +++ b/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, + pub read_whitelist: HashSet, pub allow_write: PermissionState, - pub write_whitelist: HashSet, + pub write_whitelist: HashSet, pub allow_net: PermissionState, pub net_whitelist: HashSet, pub allow_env: PermissionState, @@ -349,10 +349,10 @@ fn log_perm_access(message: &str) { } } -fn check_path_white_list(path: &Path, white_list: &HashSet) -> bool { +fn check_path_white_list(path: &Path, white_list: &HashSet) -> 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() diff --git a/cli/tests/integration_tests.rs b/cli/tests/integration_tests.rs index a42dd439e3..013e4c41b5 100644 --- a/cli/tests/integration_tests.rs +++ b/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()],