From b03f4a4a1c252d808b72fc462ea783362f810c75 Mon Sep 17 00:00:00 2001 From: Kitson Kelly Date: Tue, 27 Oct 2020 06:56:00 +1100 Subject: [PATCH] fix(cli): restore permission check on workers (#8123) Fixes #8120 --- cli/permissions.rs | 67 +++++++++++++++++++ cli/program_state.rs | 10 ++- cli/specifier_handler.rs | 13 ++-- cli/tests/error_worker_permissions_local.ts | 4 ++ .../error_worker_permissions_local.ts.out | 3 + cli/tests/error_worker_permissions_remote.ts | 4 ++ .../error_worker_permissions_remote.ts.out | 3 + cli/tests/integration_tests.rs | 13 ++++ cli/tsc.rs | 2 +- 9 files changed, 110 insertions(+), 9 deletions(-) create mode 100644 cli/tests/error_worker_permissions_local.ts create mode 100644 cli/tests/error_worker_permissions_local.ts.out create mode 100644 cli/tests/error_worker_permissions_remote.ts create mode 100644 cli/tests/error_worker_permissions_remote.ts.out diff --git a/cli/permissions.rs b/cli/permissions.rs index 2eebdba9a2..477f58c8f1 100644 --- a/cli/permissions.rs +++ b/cli/permissions.rs @@ -7,6 +7,7 @@ use deno_core::error::custom_error; use deno_core::error::uri_error; use deno_core::error::AnyError; use deno_core::url; +use deno_core::ModuleSpecifier; use serde::Deserialize; use std::collections::HashSet; use std::env::current_dir; @@ -548,6 +549,21 @@ impl Permissions { .check(&format!("network access to \"{}\"", url), "--allow-net") } + /// A helper function that determines if the module specifier is a local or + /// remote, and performs a read or net check for the specifier. + pub fn check_specifier( + &self, + specifier: &ModuleSpecifier, + ) -> Result<(), AnyError> { + let url = specifier.as_url(); + if url.scheme() == "file" { + let path = url.to_file_path().unwrap(); + self.check_read(&path) + } else { + self.check_net_url(url) + } + } + pub fn check_env(&self) -> Result<(), AnyError> { self .env @@ -830,6 +846,57 @@ mod tests { } } + #[test] + fn check_specifiers() { + let read_allowlist = if cfg!(target_os = "windows") { + vec![PathBuf::from("C:\\a")] + } else { + vec![PathBuf::from("/a")] + }; + let perms = Permissions::from_flags(&Flags { + read_allowlist, + net_allowlist: svec!["localhost"], + ..Default::default() + }); + + let mut fixtures = vec![ + ( + ModuleSpecifier::resolve_url_or_path("http://localhost:4545/mod.ts") + .unwrap(), + true, + ), + ( + ModuleSpecifier::resolve_url_or_path("http://deno.land/x/mod.ts") + .unwrap(), + false, + ), + ]; + + if cfg!(target_os = "windows") { + fixtures.push(( + ModuleSpecifier::resolve_url_or_path("file:///C:/a/mod.ts").unwrap(), + true, + )); + fixtures.push(( + ModuleSpecifier::resolve_url_or_path("file:///C:/b/mod.ts").unwrap(), + false, + )); + } else { + fixtures.push(( + ModuleSpecifier::resolve_url_or_path("file:///a/mod.ts").unwrap(), + true, + )); + fixtures.push(( + ModuleSpecifier::resolve_url_or_path("file:///b/mod.ts").unwrap(), + false, + )); + } + + for (specifier, expected) in fixtures { + assert_eq!(perms.check_specifier(&specifier).is_ok(), expected); + } + } + #[test] fn test_deserialize_perms() { let json_perms = r#" diff --git a/cli/program_state.rs b/cli/program_state.rs index a3dd65dc19..f8af957b69 100644 --- a/cli/program_state.rs +++ b/cli/program_state.rs @@ -121,13 +121,19 @@ impl ProgramState { self: &Arc, specifier: ModuleSpecifier, target_lib: TargetLib, - dynamic_permissions: Permissions, + runtime_permissions: Permissions, is_dynamic: bool, maybe_import_map: Option, ) -> Result<(), AnyError> { let specifier = specifier.clone(); + // Workers are subject to the current runtime permissions. We do the + // permission check here early to avoid "wasting" time building a module + // graph for a module that cannot be loaded. + if target_lib == TargetLib::Worker { + runtime_permissions.check_specifier(&specifier)?; + } let handler = - Rc::new(RefCell::new(FetchHandler::new(self, dynamic_permissions)?)); + Rc::new(RefCell::new(FetchHandler::new(self, runtime_permissions)?)); let mut builder = GraphBuilder2::new(handler, maybe_import_map, self.lockfile.clone()); builder.add(&specifier, is_dynamic).await?; diff --git a/cli/specifier_handler.rs b/cli/specifier_handler.rs index 016ad04689..988cad72be 100644 --- a/cli/specifier_handler.rs +++ b/cli/specifier_handler.rs @@ -215,8 +215,9 @@ impl CompiledFileMetadata { pub struct FetchHandler { /// An instance of disk where generated (emitted) files are stored. disk_cache: DiskCache, - /// A set of permissions to apply to dynamic imports. - dynamic_permissions: Permissions, + /// The set of current runtime permissions which need to be applied to + /// dynamic imports. + runtime_permissions: Permissions, /// A clone of the `program_state` file fetcher. file_fetcher: SourceFileFetcher, } @@ -224,7 +225,7 @@ pub struct FetchHandler { impl FetchHandler { pub fn new( program_state: &Arc, - dynamic_permissions: Permissions, + runtime_permissions: Permissions, ) -> Result { let custom_root = env::var("DENO_DIR").map(String::into).ok(); let deno_dir = DenoDir::new(custom_root)?; @@ -233,7 +234,7 @@ impl FetchHandler { Ok(FetchHandler { disk_cache, - dynamic_permissions, + runtime_permissions, file_fetcher, }) } @@ -250,7 +251,7 @@ impl SpecifierHandler for FetchHandler { // permissions need to be applied. Other static imports have all // permissions. let permissions = if is_dynamic { - self.dynamic_permissions.clone() + self.runtime_permissions.clone() } else { Permissions::allow_all() }; @@ -445,7 +446,7 @@ pub mod tests { let fetch_handler = FetchHandler { disk_cache, - dynamic_permissions: Permissions::default(), + runtime_permissions: Permissions::default(), file_fetcher, }; diff --git a/cli/tests/error_worker_permissions_local.ts b/cli/tests/error_worker_permissions_local.ts new file mode 100644 index 0000000000..b43c8fe949 --- /dev/null +++ b/cli/tests/error_worker_permissions_local.ts @@ -0,0 +1,4 @@ +new Worker( + new URL("./subdeb/worker_types.ts", import.meta.url).toString(), + { type: "module" }, +); diff --git a/cli/tests/error_worker_permissions_local.ts.out b/cli/tests/error_worker_permissions_local.ts.out new file mode 100644 index 0000000000..e53accbab7 --- /dev/null +++ b/cli/tests/error_worker_permissions_local.ts.out @@ -0,0 +1,3 @@ +[WILDCARD] +error: Uncaught (in worker "") read access to "[WILDCARD]worker_types.ts", run again with the --allow-read flag +[WILDCARD] diff --git a/cli/tests/error_worker_permissions_remote.ts b/cli/tests/error_worker_permissions_remote.ts new file mode 100644 index 0000000000..9fd1ba5a82 --- /dev/null +++ b/cli/tests/error_worker_permissions_remote.ts @@ -0,0 +1,4 @@ +new Worker( + "http://localhost:4545/cli/tests/subdir/worker_types.ts", + { type: "module" }, +); diff --git a/cli/tests/error_worker_permissions_remote.ts.out b/cli/tests/error_worker_permissions_remote.ts.out new file mode 100644 index 0000000000..6cf63fa22e --- /dev/null +++ b/cli/tests/error_worker_permissions_remote.ts.out @@ -0,0 +1,3 @@ +[WILDCARD] +error: Uncaught (in worker "") network access to "http://localhost:4545/cli/tests/subdir/worker_types.ts", run again with the --allow-net flag +[WILDCARD] diff --git a/cli/tests/integration_tests.rs b/cli/tests/integration_tests.rs index e637db07ab..5a6bdae14f 100644 --- a/cli/tests/integration_tests.rs +++ b/cli/tests/integration_tests.rs @@ -2467,6 +2467,19 @@ itest!(error_local_static_import_from_remote_js { output: "error_local_static_import_from_remote.js.out", }); +itest!(error_worker_permissions_local { + args: "run --reload error_worker_permissions_local.ts", + output: "error_worker_permissions_local.ts.out", + exit_code: 1, +}); + +itest!(error_worker_permissions_remote { + args: "run --reload error_worker_permissions_remote.ts", + http_server: true, + output: "error_worker_permissions_remote.ts.out", + exit_code: 1, +}); + itest!(exit_error42 { exit_code: 42, args: "run --quiet --reload exit_error42.ts", diff --git a/cli/tsc.rs b/cli/tsc.rs index 7b72e8d365..2597789def 100644 --- a/cli/tsc.rs +++ b/cli/tsc.rs @@ -135,7 +135,7 @@ lazy_static! { Regex::new(r#"(?i)\slib\s*=\s*["']([^"']*)["']"#).unwrap(); } -#[derive(Clone)] +#[derive(Clone, Eq, PartialEq)] pub enum TargetLib { Main, Worker,