refactor: lock file (#6569)

- refactor lock file creation
- provide deterministic output in lock file (alphabetically sorted)
- dynamic imports are checked against lock file

cli/flags.rs

@@ -1119,6 +1119,7 @@ fn lock_arg<'a, 'b>() -> Arg<'a, 'b> {
 fn lock_write_arg<'a, 'b>() -> Arg<'a, 'b> {
   Arg::with_name("lock-write")
     .long("lock-write")
+    .requires("lock")
     .help("Write lock file. Use with --lock.")
 }
 

cli/global_state.rs

@@ -74,9 +74,9 @@ impl GlobalState {
       dir.gen_cache.clone(),
     )?;
 
-    // Note: reads lazily from disk on first call to lockfile.check()
     let lockfile = if let Some(filename) = &flags.lock {
-      Some(Mutex::new(Lockfile::new(filename.to_string())))
+      let lockfile = Lockfile::new(filename.to_string(), flags.lock_write)?;
+      Some(Mutex::new(lockfile))
     } else {
       None
     };
@@ -142,8 +142,26 @@ impl GlobalState {
       .fetch_cached_source_file(&module_specifier, permissions.clone())
       .expect("Source file not found");
 
-    // Check if we need to compile files.
     let module_graph_files = module_graph.values().collect::<Vec<_>>();
+    // Check integrity of every file in module graph
+    if let Some(ref lockfile) = self.lockfile {
+      let mut g = lockfile.lock().unwrap();
+
+      for graph_file in &module_graph_files {
+        let check_passed =
+          g.check_or_insert(&graph_file.url, &graph_file.source_code);
+
+        if !check_passed {
+          eprintln!(
+            "Subresource integrity check failed --lock={}\n{}",
+            g.filename, graph_file.url
+          );
+          std::process::exit(10);
+        }
+      }
+    }
+
+    // Check if we need to compile files.
     let should_compile = needs_compilation(
       self.ts_compiler.compile_js,
       out.media_type,
@@ -165,6 +183,11 @@ impl GlobalState {
         .await?;
     }
 
+    if let Some(ref lockfile) = self.lockfile {
+      let g = lockfile.lock().unwrap();
+      g.write()?;
+    }
+
     drop(compile_lock);
 
     Ok(())
@@ -181,7 +204,6 @@ impl GlobalState {
     _maybe_referrer: Option<ModuleSpecifier>,
   ) -> Result<CompiledModule, ErrBox> {
     let state1 = self.clone();
-    let state2 = self.clone();
     let module_specifier = module_specifier.clone();
 
     let out = self
@@ -222,24 +244,6 @@ impl GlobalState {
 
     drop(compile_lock);
 
-    if let Some(ref lockfile) = state2.lockfile {
-      let mut g = lockfile.lock().unwrap();
-      if state2.flags.lock_write {
-        g.insert(&out.url, out.source_code);
-      } else {
-        let check = match g.check(&out.url, out.source_code) {
-          Err(e) => return Err(ErrBox::from(e)),
-          Ok(v) => v,
-        };
-        if !check {
-          eprintln!(
-            "Subresource integrity check failed --lock={}\n{}",
-            g.filename, compiled_module.name
-          );
-          std::process::exit(10);
-        }
-      }
-    }
     Ok(compiled_module)
   }
 

cli/lockfile.rs

@@ -1,26 +1,40 @@
 use serde_json::json;
 pub use serde_json::Value;
-use std::collections::HashMap;
+use std::collections::BTreeMap;
 use std::io::Result;
-use url::Url;
 
 pub struct Lockfile {
-  need_read: bool,
+  write: bool,
-  map: HashMap<String, String>,
+  map: BTreeMap<String, String>,
   pub filename: String,
 }
 
 impl Lockfile {
-  pub fn new(filename: String) -> Lockfile {
+  pub fn new(filename: String, write: bool) -> Result<Lockfile> {
-    Lockfile {
+    debug!("lockfile \"{}\", write: {}", filename, write);
-      map: HashMap::new(),
+
+    let map = if write {
+      BTreeMap::new()
+    } else {
+      let s = std::fs::read_to_string(&filename)?;
+      serde_json::from_str(&s)?
+    };
+
+    Ok(Lockfile {
+      write,
+      map,
       filename,
-      need_read: true,
+    })
-    }
   }
 
+  // Synchronize lock file to disk - noop if --lock-write file is not specified.
   pub fn write(&self) -> Result<()> {
-    let j = json!(self.map);
+    if !self.write {
+      return Ok(());
+    }
+    // Will perform sort so output is deterministic
+    let map: BTreeMap<_, _> = self.map.iter().collect();
+    let j = json!(map);
     let s = serde_json::to_string_pretty(&j).unwrap();
     let mut f = std::fs::OpenOptions::new()
       .write(true)
@@ -33,40 +47,35 @@ impl Lockfile {
     Ok(())
   }
 
-  pub fn read(&mut self) -> Result<()> {
+  pub fn check_or_insert(&mut self, specifier: &str, code: &str) -> bool {
-    debug!("lockfile read {}", self.filename);
+    if self.write {
-    let s = std::fs::read_to_string(&self.filename)?;
+      // In case --lock-write is specified check always passes
-    self.map = serde_json::from_str(&s)?;
+      self.insert(specifier, code);
-    self.need_read = false;
+      true
-    Ok(())
+    } else {
+      self.check(specifier, code)
+    }
   }
 
-  /// Lazily reads the filename, checks the given module is included.
+  /// Checks the given module is included.
-  /// Returns Ok(true) if check passed
+  /// Returns Ok(true) if check passed.
-  pub fn check(&mut self, url: &Url, code: Vec<u8>) -> Result<bool> {
+  fn check(&mut self, specifier: &str, code: &str) -> bool {
-    let url_str = url.to_string();
+    if specifier.starts_with("file:") {
-    if url_str.starts_with("file:") {
+      return true;
-      return Ok(true);
     }
-    if self.need_read {
+    if let Some(lockfile_checksum) = self.map.get(specifier) {
-      self.read()?;
+      let compiled_checksum = crate::checksum::gen(&[code.as_bytes()]);
-    }
-    assert!(!self.need_read);
-    Ok(if let Some(lockfile_checksum) = self.map.get(&url_str) {
-      let compiled_checksum = crate::checksum::gen(&[&code]);
       lockfile_checksum == &compiled_checksum
     } else {
       false
-    })
+    }
   }
 
-  // Returns true if module was not already inserted.
+  fn insert(&mut self, specifier: &str, code: &str) {
-  pub fn insert(&mut self, url: &Url, code: Vec<u8>) -> bool {
+    if specifier.starts_with("file:") {
-    let url_str = url.to_string();
+      return;
-    if url_str.starts_with("file:") {
-      return false;
     }
-    let checksum = crate::checksum::gen(&[&code]);
+    let checksum = crate::checksum::gen(&[code.as_bytes()]);
-    self.map.insert(url_str, checksum).is_none()
+    self.map.insert(specifier.to_string(), checksum);
   }
 }

cli/main.rs

@@ -140,19 +140,6 @@ fn write_to_stdout_ignore_sigpipe(bytes: &[u8]) -> Result<(), std::io::Error> {
   }
 }
 
-fn write_lockfile(global_state: GlobalState) -> Result<(), std::io::Error> {
-  if global_state.flags.lock_write {
-    if let Some(ref lockfile) = global_state.lockfile {
-      let g = lockfile.lock().unwrap();
-      g.write()?;
-    } else {
-      eprintln!("--lock flag must be specified when using --lock-write");
-      std::process::exit(11);
-    }
-  }
-  Ok(())
-}
-
 fn print_cache_info(state: &GlobalState) {
   println!(
     "{} {:?}",
@@ -356,8 +343,6 @@ async fn cache_command(flags: Flags, files: Vec<String>) -> Result<(), ErrBox> {
     worker.preload_module(&specifier).await.map(|_| ())?;
   }
 
-  write_lockfile(global_state)?;
-
   Ok(())
 }
 
@@ -606,7 +591,6 @@ async fn run_command(flags: Flags, script: String) -> Result<(), ErrBox> {
 
   debug!("main_module {}", main_module);
   worker.execute_module(&main_module).await?;
-  write_lockfile(global_state)?;
   worker.execute("window.dispatchEvent(new Event('load'))")?;
   (&mut *worker).await?;
   worker.execute("window.dispatchEvent(new Event('unload'))")?;

cli/tests/integration_tests.rs

@@ -1562,6 +1562,12 @@ itest!(js_import_detect {
   exit_code: 0,
 });
 
+itest!(lock_write_requires_lock {
+  args: "run --lock-write some_file.ts",
+  output: "lock_write_requires_lock.out",
+  exit_code: 1,
+});
+
 itest!(lock_write_fetch {
   args:
     "run --quiet --allow-read --allow-write --allow-env --allow-run lock_write_fetch.ts",
@@ -1582,6 +1588,13 @@ itest_ignore!(lock_check_ok2 {
   http_server: true,
 });
 
+itest!(lock_dynamic_imports {
+  args: "run --lock=lock_dynamic_imports.json --allow-read --allow-net http://127.0.0.1:4545/cli/tests/013_dynamic_import.ts",
+  output: "lock_dynamic_imports.out",
+  exit_code: 10,
+  http_server: true,
+});
+
 itest!(lock_check_err {
   args: "run --lock=lock_check_err.json http://127.0.0.1:4545/cli/tests/003_relative_import.ts",
   output: "lock_check_err.out",

cli/tests/lock_check_err.json

@@ -1,4 +1,4 @@
 {
-  "http://127.0.0.1:4545/cli/tests/subdir/print_hello.ts": "5c93c66125878389f47f4abcac003f4be1276c5223612c26302460d71841e287",
+  "http://127.0.0.1:4545/cli/tests/subdir/print_hello.ts": "fe7bbccaedb6579200a8b582f905139296402d06b1b91109d6e12c41a23125da",
   "http://127.0.0.1:4545/cli/tests/003_relative_import.ts": "bad"
 }

cli/tests/lock_dynamic_imports.json

@@ -0,0 +1,6 @@
+{
+  "http://127.0.0.1:4545/cli/tests/013_dynamic_import.ts": "c875f10de49bded1ad76f1709d68e6cf2c0cfb8e8e862542a3fcb4ab09257b99",
+  "http://127.0.0.1:4545/cli/tests/subdir/mod1.ts": "f627f1649f9853adfa096241ae2defa75e4e327cbeb6af0e82a11304b3e5c8be",
+  "http://127.0.0.1:4545/cli/tests/subdir/print_hello.ts": "fe7bbccaedb6579200a8b582f905139296402d06b1b91109d6e12c41a23125da",
+  "http://127.0.0.1:4545/cli/tests/subdir/subdir2/mod2.ts": "bad"
+}

cli/tests/lock_dynamic_imports.out

@@ -0,0 +1,3 @@
+[WILDCARD]
+Subresource integrity check failed --lock=lock_dynamic_imports.json
+http://127.0.0.1:4545/cli/tests/subdir/subdir2/mod2.ts

cli/tests/lock_write_requires_lock.out

@@ -0,0 +1,3 @@
+error: The following required arguments were not provided:
+    --lock <FILE>
+[WILDCARD]