fix(core): don't allow to import internal code is snapshot is loaded (#17694)

Addressing Luca concerns from
https://github.com/denoland/deno/pull/17648#discussion_r1099352286

core/bindings.rs

@@ -10,6 +10,7 @@ use v8::MapFnTo;
 use crate::error::is_instance_of_error;
 use crate::modules::get_asserted_module_type_from_assertions;
 use crate::modules::parse_import_assertions;
+use crate::modules::resolve_helper;
 use crate::modules::validate_import_assertions;
 use crate::modules::ImportAssertionsKind;
 use crate::modules::ModuleMap;
@@ -379,9 +380,12 @@ fn import_meta_resolve(
     url_prop.to_rust_string_lossy(scope)
   };
   let module_map_rc = JsRuntime::module_map(scope);
-  let loader = {
+  let (loader, snapshot_loaded_and_not_snapshotting) = {
     let module_map = module_map_rc.borrow();
-    module_map.loader.clone()
+    (
+      module_map.loader.clone(),
+      module_map.snapshot_loaded_and_not_snapshotting,
+    )
   };
   let specifier_str = specifier.to_rust_string_lossy(scope);
 
@@ -390,8 +394,13 @@ fn import_meta_resolve(
     return;
   }
 
-  match loader.resolve(&specifier_str, &referrer, ResolutionKind::DynamicImport)
-  {
+  match resolve_helper(
+    snapshot_loaded_and_not_snapshotting,
+    loader,
+    &specifier_str,
+    &referrer,
+    ResolutionKind::DynamicImport,
+  ) {
     Ok(resolved) => {
       let resolved_val = serde_v8::to_v8(scope, resolved.as_str()).unwrap();
       rv.set(resolved_val);

core/modules.rs

@@ -292,6 +292,26 @@ impl ModuleLoader for NoopModuleLoader {
   }
 }
 
+/// Helper function, that calls into `loader.resolve()`, but denies resolution
+/// of `internal` scheme if we are running with a snapshot loaded and not
+/// creating a snapshot
+pub(crate) fn resolve_helper(
+  snapshot_loaded_and_not_snapshotting: bool,
+  loader: Rc<dyn ModuleLoader>,
+  specifier: &str,
+  referrer: &str,
+  kind: ResolutionKind,
+) -> Result<ModuleSpecifier, Error> {
+  if snapshot_loaded_and_not_snapshotting && specifier.starts_with("internal:")
+  {
+    return Err(generic_error(
+      "Cannot load internal module from external code",
+    ));
+  }
+
+  loader.resolve(specifier, referrer, kind)
+}
+
 pub struct InternalModuleLoader(Rc<dyn ModuleLoader>);
 
 impl InternalModuleLoader {
@@ -440,10 +460,11 @@ pub(crate) struct RecursiveModuleLoad {
   module_map_rc: Rc<RefCell<ModuleMap>>,
   pending: FuturesUnordered<Pin<Box<ModuleLoadFuture>>>,
   visited: HashSet<ModuleRequest>,
-  // These two fields are copied from `module_map_rc`, but they are cloned ahead
-  // of time to avoid already-borrowed errors.
+  // These three fields are copied from `module_map_rc`, but they are cloned
+  // ahead of time to avoid already-borrowed errors.
   op_state: Rc<RefCell<OpState>>,
   loader: Rc<dyn ModuleLoader>,
+  snapshot_loaded_and_not_snapshotting: bool,
 }
 
 impl RecursiveModuleLoad {
@@ -499,6 +520,9 @@ impl RecursiveModuleLoad {
       init,
       state: LoadState::Init,
       module_map_rc: module_map_rc.clone(),
+      snapshot_loaded_and_not_snapshotting: module_map_rc
+        .borrow()
+        .snapshot_loaded_and_not_snapshotting,
       op_state,
       loader,
       pending: FuturesUnordered::new(),
@@ -527,39 +551,60 @@ impl RecursiveModuleLoad {
 
   fn resolve_root(&self) -> Result<ModuleSpecifier, Error> {
     match self.init {
-      LoadInit::Main(ref specifier) => {
-        self
-          .loader
-          .resolve(specifier, ".", ResolutionKind::MainModule)
+      LoadInit::Main(ref specifier) => resolve_helper(
+        self.snapshot_loaded_and_not_snapshotting,
+        self.loader.clone(),
+        specifier,
+        ".",
+        ResolutionKind::MainModule,
+      ),
+      LoadInit::Side(ref specifier) => resolve_helper(
+        self.snapshot_loaded_and_not_snapshotting,
+        self.loader.clone(),
+        specifier,
+        ".",
+        ResolutionKind::Import,
+      ),
+      LoadInit::DynamicImport(ref specifier, ref referrer, _) => {
+        resolve_helper(
+          self.snapshot_loaded_and_not_snapshotting,
+          self.loader.clone(),
+          specifier,
+          referrer,
+          ResolutionKind::DynamicImport,
+        )
       }
-      LoadInit::Side(ref specifier) => {
-        self.loader.resolve(specifier, ".", ResolutionKind::Import)
-      }
-      LoadInit::DynamicImport(ref specifier, ref referrer, _) => self
-        .loader
-        .resolve(specifier, referrer, ResolutionKind::DynamicImport),
     }
   }
 
   async fn prepare(&self) -> Result<(), Error> {
     let op_state = self.op_state.clone();
+
     let (module_specifier, maybe_referrer) = match self.init {
       LoadInit::Main(ref specifier) => {
-        let spec =
-          self
-            .loader
-            .resolve(specifier, ".", ResolutionKind::MainModule)?;
+        let spec = resolve_helper(
+          self.snapshot_loaded_and_not_snapshotting,
+          self.loader.clone(),
+          specifier,
+          ".",
+          ResolutionKind::MainModule,
+        )?;
         (spec, None)
       }
       LoadInit::Side(ref specifier) => {
-        let spec =
-          self
-            .loader
-            .resolve(specifier, ".", ResolutionKind::Import)?;
+        let spec = resolve_helper(
+          self.snapshot_loaded_and_not_snapshotting,
+          self.loader.clone(),
+          specifier,
+          ".",
+          ResolutionKind::Import,
+        )?;
         (spec, None)
       }
       LoadInit::DynamicImport(ref specifier, ref referrer, _) => {
-        let spec = self.loader.resolve(
+        let spec = resolve_helper(
+          self.snapshot_loaded_and_not_snapshotting,
+          self.loader.clone(),
           specifier,
           referrer,
           ResolutionKind::DynamicImport,
@@ -868,6 +913,8 @@ pub(crate) struct ModuleMap {
   // This store is used temporarly, to forward parsed JSON
   // value from `new_json_module` to `json_module_evaluation_steps`
   json_value_store: HashMap<v8::Global<v8::Module>, v8::Global<v8::Value>>,
+
+  pub(crate) snapshot_loaded_and_not_snapshotting: bool,
 }
 
 impl ModuleMap {
@@ -945,6 +992,7 @@ impl ModuleMap {
   pub(crate) fn new(
     loader: Rc<dyn ModuleLoader>,
     op_state: Rc<RefCell<OpState>>,
+    snapshot_loaded_and_not_snapshotting: bool,
   ) -> ModuleMap {
     Self {
       handles: vec![],
@@ -957,6 +1005,7 @@ impl ModuleMap {
       preparing_dynamic_imports: FuturesUnordered::new(),
       pending_dynamic_imports: FuturesUnordered::new(),
       json_value_store: HashMap::new(),
+      snapshot_loaded_and_not_snapshotting,
     }
   }
 
@@ -1083,7 +1132,9 @@ impl ModuleMap {
         return Err(ModuleError::Exception(exception));
       }
 
-      let module_specifier = match self.loader.resolve(
+      let module_specifier = match resolve_helper(
+        self.snapshot_loaded_and_not_snapshotting,
+        self.loader.clone(),
         &import_specifier,
         name,
         if is_dynamic_import {
@@ -1249,7 +1300,17 @@ impl ModuleMap {
       .borrow_mut()
       .dynamic_import_map
       .insert(load.id, resolver_handle);
-    let resolve_result = module_map_rc.borrow().loader.resolve(
+
+    let (loader, snapshot_loaded_and_not_snapshotting) = {
+      let module_map = module_map_rc.borrow();
+      (
+        module_map.loader.clone(),
+        module_map.snapshot_loaded_and_not_snapshotting,
+      )
+    };
+    let resolve_result = resolve_helper(
+      snapshot_loaded_and_not_snapshotting,
+      loader,
       specifier,
       referrer,
       ResolutionKind::DynamicImport,
@@ -1287,10 +1348,14 @@ impl ModuleMap {
     referrer: &str,
     import_assertions: HashMap<String, String>,
   ) -> Option<v8::Local<'s, v8::Module>> {
-    let resolved_specifier = self
-      .loader
-      .resolve(specifier, referrer, ResolutionKind::Import)
-      .expect("Module should have been already resolved");
+    let resolved_specifier = resolve_helper(
+      self.snapshot_loaded_and_not_snapshotting,
+      self.loader.clone(),
+      specifier,
+      referrer,
+      ResolutionKind::Import,
+    )
+    .expect("Module should have been already resolved");
 
     let module_type =
       get_asserted_module_type_from_assertions(&import_assertions);
@@ -2599,5 +2664,17 @@ if (import.meta.url != 'file:///main_with_code.js') throw Error();
         .map(|e| e.to_string()),
       Some("Module loading is not supported".to_string())
     );
+    assert_eq!(
+      resolve_helper(
+        true,
+        Rc::new(loader),
+        "internal:core.js",
+        "file://bar",
+        ResolutionKind::Import,
+      )
+      .err()
+      .map(|e| e.to_string()),
+      Some("Cannot load internal module from external code".to_string())
+    );
   }
 }

core/runtime.rs

@@ -629,7 +629,11 @@ impl JsRuntime {
       Rc::into_raw(state_rc.clone()) as *mut c_void,
     );
 
-    let module_map_rc = Rc::new(RefCell::new(ModuleMap::new(loader, op_state)));
+    let module_map_rc = Rc::new(RefCell::new(ModuleMap::new(
+      loader,
+      op_state,
+      snapshot_options == SnapshotOptions::Load,
+    )));
     if let Some(module_map_data) = module_map_data {
       let scope =
         &mut v8::HandleScope::with_context(&mut isolate, global_context);
@@ -4928,4 +4932,72 @@ Deno.core.ops.op_async_serialize_object_with_numbers_as_keys({
       )
       .is_ok());
   }
+
+  #[tokio::test]
+  async fn cant_load_internal_module_when_snapshot_is_loaded_and_not_snapshotting(
+  ) {
+    #[derive(Default)]
+    struct ModsLoader;
+
+    impl ModuleLoader for ModsLoader {
+      fn resolve(
+        &self,
+        specifier: &str,
+        referrer: &str,
+        _kind: ResolutionKind,
+      ) -> Result<ModuleSpecifier, Error> {
+        assert_eq!(specifier, "file:///main.js");
+        assert_eq!(referrer, ".");
+        let s = crate::resolve_import(specifier, referrer).unwrap();
+        Ok(s)
+      }
+
+      fn load(
+        &self,
+        _module_specifier: &ModuleSpecifier,
+        _maybe_referrer: Option<ModuleSpecifier>,
+        _is_dyn_import: bool,
+      ) -> Pin<Box<ModuleSourceFuture>> {
+        let source = r#"
+        // This module doesn't really exist, just verifying that we'll get 
+        // an error when specifier starts with "internal:".
+        import { core } from "internal:core.js";
+        "#;
+
+        async move {
+          Ok(ModuleSource {
+            code: source.as_bytes().to_vec().into_boxed_slice(),
+            module_url_specified: "file:///main.js".to_string(),
+            module_url_found: "file:///main.js".to_string(),
+            module_type: ModuleType::JavaScript,
+          })
+        }
+        .boxed_local()
+      }
+    }
+
+    let snapshot = {
+      let runtime = JsRuntime::new(RuntimeOptions {
+        will_snapshot: true,
+        ..Default::default()
+      });
+      let snap: &[u8] = &runtime.snapshot();
+      Vec::from(snap).into_boxed_slice()
+    };
+
+    let mut runtime2 = JsRuntime::new(RuntimeOptions {
+      module_loader: Some(Rc::new(ModsLoader)),
+      startup_snapshot: Some(Snapshot::Boxed(snapshot)),
+      ..Default::default()
+    });
+
+    let err = runtime2
+      .load_main_module(&crate::resolve_url("file:///main.js").unwrap(), None)
+      .await
+      .unwrap_err();
+    assert_eq!(
+      err.to_string(),
+      "Cannot load internal module from external code"
+    );
+  }
 }