From 3d8a4d3b81e107bbb152ad69047f64d16ca800f3 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Wed, 26 Apr 2023 21:23:28 +0100 Subject: [PATCH] feat(cli): don't check permissions for statically analyzable dynamic imports (#18713) Closes #17697 Closes #17658 --- cli/cache/mod.rs | 17 +++++------------ cli/graph_util.rs | 11 +++-------- cli/lsp/testing/execution.rs | 1 - cli/module_loader.rs | 17 +++-------------- cli/tests/integration/run_tests.rs | 5 +++++ cli/tests/testdata/dynamic_import/empty_1.ts | 0 cli/tests/testdata/dynamic_import/empty_2.ts | 0 .../dynamic_import/permissions_remote_remote.ts | 2 +- .../static_analysis_no_permissions.ts | 13 +++++++++++++ .../static_analysis_no_permissions.ts.out | 2 ++ .../run/error_015_dynamic_import_permissions.js | 2 +- .../error_015_dynamic_import_permissions.out | 2 +- cli/tests/testdata/workers/dynamic_remote.ts | 2 +- .../workers/permissions_dynamic_remote.ts.out | 2 +- cli/tools/bench.rs | 7 ++----- cli/tools/test.rs | 9 ++------- 16 files changed, 40 insertions(+), 52 deletions(-) create mode 100644 cli/tests/testdata/dynamic_import/empty_1.ts create mode 100644 cli/tests/testdata/dynamic_import/empty_2.ts create mode 100644 cli/tests/testdata/dynamic_import/static_analysis_no_permissions.ts create mode 100644 cli/tests/testdata/dynamic_import/static_analysis_no_permissions.ts.out diff --git a/cli/cache/mod.rs b/cli/cache/mod.rs index 24712d08ae..40d74ff66b 100644 --- a/cli/cache/mod.rs +++ b/cli/cache/mod.rs @@ -45,10 +45,9 @@ pub const CACHE_PERM: u32 = 0o644; /// a concise interface to the DENO_DIR when building module graphs. pub struct FetchCacher { emit_cache: EmitCache, - dynamic_permissions: PermissionsContainer, file_fetcher: Arc, file_header_overrides: HashMap>, - root_permissions: PermissionsContainer, + permissions: PermissionsContainer, cache_info_enabled: bool, maybe_local_node_modules_url: Option, } @@ -58,16 +57,14 @@ impl FetchCacher { emit_cache: EmitCache, file_fetcher: Arc, file_header_overrides: HashMap>, - root_permissions: PermissionsContainer, - dynamic_permissions: PermissionsContainer, + permissions: PermissionsContainer, maybe_local_node_modules_url: Option, ) -> Self { Self { emit_cache, - dynamic_permissions, file_fetcher, file_header_overrides, - root_permissions, + permissions, cache_info_enabled: false, maybe_local_node_modules_url, } @@ -105,7 +102,7 @@ impl Loader for FetchCacher { fn load( &mut self, specifier: &ModuleSpecifier, - is_dynamic: bool, + _is_dynamic: bool, ) -> LoadFuture { if let Some(node_modules_url) = self.maybe_local_node_modules_url.as_ref() { // The specifier might be in a completely different symlinked tree than @@ -124,11 +121,7 @@ impl Loader for FetchCacher { } } - let permissions = if is_dynamic { - self.dynamic_permissions.clone() - } else { - self.root_permissions.clone() - }; + let permissions = self.permissions.clone(); let file_fetcher = self.file_fetcher.clone(); let file_header_overrides = self.file_header_overrides.clone(); let specifier = specifier.clone(); diff --git a/cli/graph_util.rs b/cli/graph_util.rs index d5bc6ac0d5..f9dafbb573 100644 --- a/cli/graph_util.rs +++ b/cli/graph_util.rs @@ -314,23 +314,18 @@ impl ModuleGraphBuilder { /// Creates the default loader used for creating a graph. pub fn create_graph_loader(&self) -> cache::FetchCacher { - self.create_fetch_cacher( - PermissionsContainer::allow_all(), - PermissionsContainer::allow_all(), - ) + self.create_fetch_cacher(PermissionsContainer::allow_all()) } pub fn create_fetch_cacher( &self, - root_permissions: PermissionsContainer, - dynamic_permissions: PermissionsContainer, + permissions: PermissionsContainer, ) -> cache::FetchCacher { cache::FetchCacher::new( self.emit_cache.clone(), self.file_fetcher.clone(), self.options.resolve_file_header_overrides(), - root_permissions, - dynamic_permissions, + permissions, self.options.node_modules_dir_specifier(), ) } diff --git a/cli/lsp/testing/execution.rs b/cli/lsp/testing/execution.rs index 020dd5c08a..5e5a3788af 100644 --- a/cli/lsp/testing/execution.rs +++ b/cli/lsp/testing/execution.rs @@ -226,7 +226,6 @@ impl TestRun { Permissions::from_options(&ps.options.permissions_options())?; test::check_specifiers( &ps, - permissions.clone(), self .queue .iter() diff --git a/cli/module_loader.rs b/cli/module_loader.rs index 9798983748..e4b8b616d7 100644 --- a/cli/module_loader.rs +++ b/cli/module_loader.rs @@ -109,15 +109,12 @@ impl ModuleLoadPreparer { roots: Vec, is_dynamic: bool, lib: TsTypeLib, - root_permissions: PermissionsContainer, - dynamic_permissions: PermissionsContainer, + permissions: PermissionsContainer, ) -> Result<(), AnyError> { log::debug!("Preparing module load."); let _pb_clear_guard = self.progress_bar.clear_guard(); - let mut cache = self - .module_graph_builder - .create_fetch_cacher(root_permissions, dynamic_permissions); + let mut cache = self.module_graph_builder.create_fetch_cacher(permissions); let maybe_imports = self.options.to_maybe_imports()?; let graph_resolver = self.resolver.as_graph_resolver(); let graph_npm_resolver = self.resolver.as_graph_npm_resolver(); @@ -216,7 +213,6 @@ impl ModuleLoadPreparer { false, lib, PermissionsContainer::allow_all(), - PermissionsContainer::allow_all(), ) .await } @@ -537,7 +533,6 @@ impl ModuleLoader for CliModuleLoader { let specifier = specifier.clone(); let module_load_preparer = self.module_load_preparer.clone(); - let dynamic_permissions = self.dynamic_permissions.clone(); let root_permissions = if is_dynamic { self.dynamic_permissions.clone() } else { @@ -547,13 +542,7 @@ impl ModuleLoader for CliModuleLoader { async move { module_load_preparer - .prepare_module_load( - vec![specifier], - is_dynamic, - lib, - root_permissions, - dynamic_permissions, - ) + .prepare_module_load(vec![specifier], is_dynamic, lib, root_permissions) .await } .boxed_local() diff --git a/cli/tests/integration/run_tests.rs b/cli/tests/integration/run_tests.rs index cc37cf523a..d946b6a1c5 100644 --- a/cli/tests/integration/run_tests.rs +++ b/cli/tests/integration/run_tests.rs @@ -2661,6 +2661,11 @@ mod permissions { }); } + itest!(dynamic_import_static_analysis_no_permissions { + args: "run --quiet --reload --no-prompt dynamic_import/static_analysis_no_permissions.ts", + output: "dynamic_import/static_analysis_no_permissions.ts.out", + }); + itest!(dynamic_import_permissions_remote_remote { args: "run --quiet --reload --allow-net=localhost:4545 dynamic_import/permissions_remote_remote.ts", output: "dynamic_import/permissions_remote_remote.ts.out", diff --git a/cli/tests/testdata/dynamic_import/empty_1.ts b/cli/tests/testdata/dynamic_import/empty_1.ts new file mode 100644 index 0000000000..e69de29bb2 diff --git a/cli/tests/testdata/dynamic_import/empty_2.ts b/cli/tests/testdata/dynamic_import/empty_2.ts new file mode 100644 index 0000000000..e69de29bb2 diff --git a/cli/tests/testdata/dynamic_import/permissions_remote_remote.ts b/cli/tests/testdata/dynamic_import/permissions_remote_remote.ts index 0033bcccce..65a2541910 100644 --- a/cli/tests/testdata/dynamic_import/permissions_remote_remote.ts +++ b/cli/tests/testdata/dynamic_import/permissions_remote_remote.ts @@ -1,3 +1,3 @@ await import( - "http://localhost:4545/dynamic_import/static_remote.ts" + "" + "http://localhost:4545/dynamic_import/static_remote.ts" ); diff --git a/cli/tests/testdata/dynamic_import/static_analysis_no_permissions.ts b/cli/tests/testdata/dynamic_import/static_analysis_no_permissions.ts new file mode 100644 index 0000000000..de75ba87b6 --- /dev/null +++ b/cli/tests/testdata/dynamic_import/static_analysis_no_permissions.ts @@ -0,0 +1,13 @@ +try { + await import("./empty_1.ts"); + console.log("✅ Succeeded importing statically analyzable specifier"); +} catch { + console.log("❌ Failed importing statically analyzable specifier"); +} + +try { + await import("" + "./empty_2.ts"); + console.log("❌ Succeeded importing non-statically analyzable specifier"); +} catch { + console.log("✅ Failed importing non-statically analyzable specifier"); +} diff --git a/cli/tests/testdata/dynamic_import/static_analysis_no_permissions.ts.out b/cli/tests/testdata/dynamic_import/static_analysis_no_permissions.ts.out new file mode 100644 index 0000000000..ba9249ab0b --- /dev/null +++ b/cli/tests/testdata/dynamic_import/static_analysis_no_permissions.ts.out @@ -0,0 +1,2 @@ +✅ Succeeded importing statically analyzable specifier +✅ Failed importing non-statically analyzable specifier diff --git a/cli/tests/testdata/run/error_015_dynamic_import_permissions.js b/cli/tests/testdata/run/error_015_dynamic_import_permissions.js index 73da56fd89..47961cf63b 100644 --- a/cli/tests/testdata/run/error_015_dynamic_import_permissions.js +++ b/cli/tests/testdata/run/error_015_dynamic_import_permissions.js @@ -1,3 +1,3 @@ (async () => { - await import("http://localhost:4545/subdir/mod4.js"); + await import("" + "http://localhost:4545/subdir/mod4.js"); })(); diff --git a/cli/tests/testdata/run/error_015_dynamic_import_permissions.out b/cli/tests/testdata/run/error_015_dynamic_import_permissions.out index ef54f331b0..87ce43e9cd 100644 --- a/cli/tests/testdata/run/error_015_dynamic_import_permissions.out +++ b/cli/tests/testdata/run/error_015_dynamic_import_permissions.out @@ -1,4 +1,4 @@ error: Uncaught (in promise) TypeError: Requires net access to "localhost:4545", run again with the --allow-net flag - await import("http://localhost:4545/subdir/mod4.js"); + await import("" + "http://localhost:4545/subdir/mod4.js"); ^ at async file://[WILDCARD]/error_015_dynamic_import_permissions.js:2:3 diff --git a/cli/tests/testdata/workers/dynamic_remote.ts b/cli/tests/testdata/workers/dynamic_remote.ts index 381c7f374c..54e4a4714e 100644 --- a/cli/tests/testdata/workers/dynamic_remote.ts +++ b/cli/tests/testdata/workers/dynamic_remote.ts @@ -1,2 +1,2 @@ // This file doesn't really exist, but it doesn't matter, a "PermissionsDenied" error should be thrown. -await import("https://example.com/some/file.ts"); +await import("" + "https://example.com/some/file.ts"); diff --git a/cli/tests/testdata/workers/permissions_dynamic_remote.ts.out b/cli/tests/testdata/workers/permissions_dynamic_remote.ts.out index cd1884c7e6..91f3cc6d5b 100644 --- a/cli/tests/testdata/workers/permissions_dynamic_remote.ts.out +++ b/cli/tests/testdata/workers/permissions_dynamic_remote.ts.out @@ -1,5 +1,5 @@ error: Uncaught (in worker "") (in promise) TypeError: Requires net access to "example.com", run again with the --allow-net flag -await import("https://example.com/some/file.ts"); +await import("" + "https://example.com/some/file.ts"); ^ at async http://localhost:4545/workers/dynamic_remote.ts:2:1 [WILDCARD]error: Uncaught (in promise) Error: Unhandled error in child worker. diff --git a/cli/tools/bench.rs b/cli/tools/bench.rs index 9930bcc771..962b1ac174 100644 --- a/cli/tools/bench.rs +++ b/cli/tools/bench.rs @@ -418,7 +418,6 @@ impl BenchReporter for ConsoleReporter { /// Type check a collection of module and document specifiers. async fn check_specifiers( ps: &ProcState, - permissions: Permissions, specifiers: Vec, ) -> Result<(), AnyError> { let lib = ps.options.ts_type_lib_window(); @@ -428,10 +427,8 @@ async fn check_specifiers( false, lib, PermissionsContainer::allow_all(), - PermissionsContainer::new(permissions), ) .await?; - Ok(()) } @@ -654,7 +651,7 @@ pub async fn run_benchmarks( return Err(generic_error("No bench modules found")); } - check_specifiers(&ps, permissions.clone(), specifiers.clone()).await?; + check_specifiers(&ps, specifiers.clone()).await?; if bench_options.no_run { return Ok(()); @@ -813,7 +810,7 @@ pub async fn run_benchmarks_with_watch( .filter(|specifier| modules_to_reload.contains(specifier)) .collect::>(); - check_specifiers(&ps, permissions.clone(), specifiers.clone()).await?; + check_specifiers(&ps, specifiers.clone()).await?; if bench_options.no_run { return Ok(()); diff --git a/cli/tools/test.rs b/cli/tools/test.rs index 17d1cebf32..268f3b4b9e 100644 --- a/cli/tools/test.rs +++ b/cli/tools/test.rs @@ -1230,7 +1230,6 @@ async fn fetch_inline_files( /// Type check a collection of module and document specifiers. pub async fn check_specifiers( ps: &ProcState, - permissions: Permissions, specifiers: Vec<(ModuleSpecifier, TestMode)>, ) -> Result<(), AnyError> { let lib = ps.options.ts_type_lib_window(); @@ -1265,7 +1264,6 @@ pub async fn check_specifiers( false, lib, PermissionsContainer::new(Permissions::allow_all()), - PermissionsContainer::new(permissions.clone()), ) .await?; } @@ -1287,7 +1285,6 @@ pub async fn check_specifiers( false, lib, PermissionsContainer::allow_all(), - PermissionsContainer::new(permissions), ) .await?; @@ -1648,8 +1645,7 @@ pub async fn run_tests( return Err(generic_error("No test modules found")); } - check_specifiers(&ps, permissions.clone(), specifiers_with_mode.clone()) - .await?; + check_specifiers(&ps, specifiers_with_mode.clone()).await?; if test_options.no_run { return Ok(()); @@ -1821,8 +1817,7 @@ pub async fn run_tests_with_watch( .filter(|(specifier, _)| modules_to_reload.contains(specifier)) .collect::>(); - check_specifiers(&ps, permissions.clone(), specifiers_with_mode.clone()) - .await?; + check_specifiers(&ps, specifiers_with_mode.clone()).await?; if test_options.no_run { return Ok(());