Dynamic import should respect permissions (#2764)

cli/ops.rs

@@ -497,7 +497,12 @@ fn op_fetch_source_file(
   let specifier = inner.specifier().unwrap();
   let referrer = inner.referrer().unwrap();
 
-  let resolved_specifier = state.resolve(specifier, referrer, false)?;
+  // TODO(ry) Maybe a security hole. Only the compiler worker should have access
+  // to this. Need a test to demonstrate the hole.
+  let is_dyn_import = false;
+
+  let resolved_specifier =
+    state.resolve(specifier, referrer, false, is_dyn_import)?;
 
   let fut = state
     .file_fetcher
@@ -750,7 +755,7 @@ fn op_fetch(
   let req = msg_util::deserialize_request(header, body)?;
 
   let url_ = url::Url::parse(url).map_err(ErrBox::from)?;
-  state.check_net_url(url_)?;
+  state.check_net_url(&url_)?;
 
   let client = http_util::get_client();
 

cli/permissions.rs

@@ -274,7 +274,7 @@ impl DenoPermissions {
     }
   }
 
-  pub fn check_net_url(&self, url: url::Url) -> Result<(), ErrBox> {
+  pub fn check_net_url(&self, url: &url::Url) -> Result<(), ErrBox> {
     let msg = &format!("network access to \"{}\"", url);
     match self.allow_net.get_state() {
       PermissionAccessorState::Allow => {
@@ -627,7 +627,7 @@ mod tests {
 
     for (url_str, is_ok) in url_tests.iter() {
       let u = url::Url::parse(url_str).unwrap();
-      assert_eq!(*is_ok, perms.check_net_url(u).is_ok());
+      assert_eq!(*is_ok, perms.check_net_url(&u).is_ok());
     }
 
     for (domain, is_ok) in domain_tests.iter() {

cli/state.rs

@@ -4,6 +4,7 @@ use crate::compilers::JsCompiler;
 use crate::compilers::JsonCompiler;
 use crate::compilers::TsCompiler;
 use crate::deno_dir;
+use crate::deno_error::permission_denied;
 use crate::file_fetcher::SourceFileFetcher;
 use crate::flags;
 use crate::global_timer::GlobalTimer;
@@ -119,6 +120,7 @@ impl Loader for ThreadSafeState {
     specifier: &str,
     referrer: &str,
     is_main: bool,
+    is_dyn_import: bool,
   ) -> Result<ModuleSpecifier, ErrBox> {
     if !is_main {
       if let Some(import_map) = &self.import_map {
@@ -128,8 +130,14 @@ impl Loader for ThreadSafeState {
         }
       }
     }
+    let module_specifier =
+      ModuleSpecifier::resolve_import(specifier, referrer)?;
 
-    ModuleSpecifier::resolve_import(specifier, referrer).map_err(ErrBox::from)
+    if is_dyn_import {
+      self.check_dyn_import(&module_specifier)?;
+    }
+
+    Ok(module_specifier)
   }
 
   /// Given an absolute url, load its source code.
@@ -294,7 +302,7 @@ impl ThreadSafeState {
   }
 
   #[inline]
-  pub fn check_net_url(&self, url: url::Url) -> Result<(), ErrBox> {
+  pub fn check_net_url(&self, url: &url::Url) -> Result<(), ErrBox> {
     self.permissions.check_net_url(url)
   }
 
@@ -303,6 +311,30 @@ impl ThreadSafeState {
     self.permissions.check_run()
   }
 
+  pub fn check_dyn_import(
+    self: &Self,
+    module_specifier: &ModuleSpecifier,
+  ) -> Result<(), ErrBox> {
+    let u = module_specifier.as_url();
+    match u.scheme() {
+      "http" | "https" => {
+        self.check_net_url(u)?;
+        Ok(())
+      }
+      "file" => {
+        let filename = u
+          .to_file_path()
+          .unwrap()
+          .into_os_string()
+          .into_string()
+          .unwrap();
+        self.check_read(&filename)?;
+        Ok(())
+      }
+      _ => Err(permission_denied()),
+    }
+  }
+
   #[cfg(test)]
   pub fn mock(argv: Vec<String>) -> ThreadSafeState {
     ThreadSafeState::new(

core/modules.rs

@@ -40,6 +40,7 @@ pub trait Loader: Send + Sync {
     specifier: &str,
     referrer: &str,
     is_main: bool,
+    is_dyn_import: bool,
   ) -> Result<ModuleSpecifier, ErrBox>;
 
   /// Given ModuleSpecifier, load its source code.
@@ -125,12 +126,15 @@ impl<L: Loader> RecursiveLoad<L> {
 
   fn add_root(&mut self) -> Result<(), ErrBox> {
     let module_specifier = match self.state {
-      State::ResolveMain(ref specifier) => {
-        self.loader.resolve(specifier, ".", true)?
-      }
-      State::ResolveImport(ref specifier, ref referrer) => {
-        self.loader.resolve(specifier, referrer, false)?
-      }
+      State::ResolveMain(ref specifier) => self.loader.resolve(
+        specifier,
+        ".",
+        true,
+        self.dyn_import_id().is_some(),
+      )?,
+      State::ResolveImport(ref specifier, ref referrer) => self
+        .loader
+        .resolve(specifier, referrer, false, self.dyn_import_id().is_some())?,
       _ => unreachable!(),
     };
 
@@ -160,7 +164,12 @@ impl<L: Loader> RecursiveLoad<L> {
     referrer: &str,
     parent_id: deno_mod,
   ) -> Result<(), ErrBox> {
-    let module_specifier = self.loader.resolve(specifier, referrer, false)?;
+    let module_specifier = self.loader.resolve(
+      specifier,
+      referrer,
+      false,
+      self.dyn_import_id().is_some(),
+    )?;
     let module_name = module_specifier.as_str();
 
     let mut modules = self.modules.lock().unwrap();
@@ -277,7 +286,12 @@ impl<L: Loader> ImportStream for RecursiveLoad<L> {
         |specifier: &str, referrer_id: deno_mod| -> deno_mod {
           let modules = self.modules.lock().unwrap();
           let referrer = modules.get_name(referrer_id).unwrap();
-          match self.loader.resolve(specifier, &referrer, is_main) {
+          match self.loader.resolve(
+            specifier,
+            &referrer,
+            is_main,
+            self.dyn_import_id().is_some(),
+          ) {
             Ok(specifier) => modules.get_id(specifier.as_str()).unwrap_or(0),
             // We should have already resolved and Ready this module, so
             // resolve() will not fail this time.
@@ -675,6 +689,7 @@ mod tests {
       specifier: &str,
       referrer: &str,
       _is_root: bool,
+      _is_dyn_import: bool,
     ) -> Result<ModuleSpecifier, ErrBox> {
       let referrer = if referrer == "." {
         "file:///"

tests/013_dynamic_import.test

@@ -1,2 +1,2 @@
-args: tests/013_dynamic_import.ts --reload
+args: tests/013_dynamic_import.ts --reload --allow-read
 output: tests/013_dynamic_import.ts.out

tests/014_duplicate_import.test

@@ -1,2 +1,2 @@
-args: tests/014_duplicate_import.ts --reload
+args: tests/014_duplicate_import.ts --reload --allow-read
 output: tests/014_duplicate_import.ts.out

tests/015_duplicate_parallel_import.test

@@ -1,2 +1,2 @@
-args: tests/015_duplicate_parallel_import.js --reload
+args: tests/015_duplicate_parallel_import.js --reload --allow-read
 output: tests/015_duplicate_parallel_import.js.out

tests/042_dyn_import_evalcontext.test

@@ -1,2 +1,2 @@
-args: run --reload tests/042_dyn_import_evalcontext.ts
+args: run --allow-read --reload tests/042_dyn_import_evalcontext.ts
 output: tests/042_dyn_import_evalcontext.ts.out

tests/error_014_catch_dynamic_import_error.test

@@ -1,2 +1,2 @@
-args: tests/error_014_catch_dynamic_import_error.js --reload
+args: tests/error_014_catch_dynamic_import_error.js --reload --allow-read
 output: tests/error_014_catch_dynamic_import_error.js.out

tests/error_015_dynamic_import_permissions.js

@@ -0,0 +1,3 @@
+(async () => {
+  await import("http://localhost:4545/tests/subdir/mod4.js");
+})();

tests/error_015_dynamic_import_permissions.out

@@ -0,0 +1 @@
+error: Uncaught TypeError: permission denied

tests/error_015_dynamic_import_permissions.test

@@ -0,0 +1,4 @@
+args: --reload --no-prompt tests/error_015_dynamic_import_permissions.js
+output: tests/error_015_dynamic_import_permissions.out
+check_stderr: true
+exit_code: 1

tests/error_016_dynamic_import_permissions2.js

@@ -0,0 +1,5 @@
+// If this is executed with --allow-net but not --allow-read the following
+// import should cause a permission denied error.
+(async () => {
+  await import("http://localhost:4545/tests/subdir/evil_remote_import.js");
+})();

tests/error_016_dynamic_import_permissions2.out

@@ -0,0 +1,2 @@
+[WILDCARD]
+error: Uncaught TypeError: permission denied

tests/error_016_dynamic_import_permissions2.test

@@ -0,0 +1,5 @@
+# We have an allow-net flag but not allow-read, it should still result in error.
+args: --no-prompt --reload --allow-net tests/error_016_dynamic_import_permissions2.js
+output: tests/error_016_dynamic_import_permissions2.out
+check_stderr: true
+exit_code: 1

tests/subdir/evil_remote_import.js

@@ -0,0 +1,4 @@
+// We expect to get a permission denied error if we dynamically
+// import this module without --allow-read.
+export * from "file:///c:/etc/passwd";
+console.log("Hello from evil_remote_import.js");