fix(ext/fs): make errors in tempfile creation clearer (#22498)

When using a prefix or suffix containing an invalid filename character,
it's not entirely clear where the errors come from. We make these errors
more consistent across platforms.

In addition, all permission prompts for tempfile and tempdir were
printing the same API name.

We also take the opportunity to make the tempfile random space larger by
2x (using a base32-encoded u64 rather than a hex-encoded u32).

Cargo.lock

@@ -1402,6 +1402,7 @@ name = "deno_fs"
 version = "0.48.0"
 dependencies = [
  "async-trait",
+ "base32",
  "deno_core",
  "deno_io",
  "filetime",

Cargo.toml

@@ -85,6 +85,7 @@ deno_webstorage = { version = "0.133.0", path = "./ext/webstorage" }
 aes = "=0.8.3"
 anyhow = "1.0.57"
 async-trait = "0.1.73"
+base32 = "=0.4.0"
 base64 = "0.21.4"
 bencher = "0.1"
 brotli = "3.3.4"

cli/Cargo.toml

@@ -80,7 +80,7 @@ eszip = "=0.64.0"
 napi_sym.workspace = true
 
 async-trait.workspace = true
-base32 = "=0.4.0"
+base32.workspace = true
 base64.workspace = true
 bincode = "=1.3.3"
 bytes.workspace = true

ext/fs/Cargo.toml

@@ -18,6 +18,7 @@ sync_fs = []
 
 [dependencies]
 async-trait.workspace = true
+base32.workspace = true
 deno_core.workspace = true
 deno_io.workspace = true
 filetime.workspace = true

ext/fs/ops.rs

@@ -8,6 +8,7 @@ use std::path::Path;
 use std::path::PathBuf;
 use std::rc::Rc;
 
+use deno_core::anyhow::bail;
 use deno_core::error::custom_error;
 use deno_core::error::type_error;
 use deno_core::error::AnyError;
@@ -880,7 +881,8 @@ pub fn op_fs_make_temp_dir_sync<P>(
 where
   P: FsPermissions + 'static,
 {
-  let (dir, fs) = make_temp_check_sync::<P>(state, dir)?;
+  let (dir, fs) =
+    make_temp_check_sync::<P>(state, dir, "Deno.makeTempDirSync()")?;
 
   let mut rng = thread_rng();
 
@@ -914,7 +916,7 @@ pub async fn op_fs_make_temp_dir_async<P>(
 where
   P: FsPermissions + 'static,
 {
-  let (dir, fs) = make_temp_check_async::<P>(state, dir)?;
+  let (dir, fs) = make_temp_check_async::<P>(state, dir, "Deno.makeTempDir()")?;
 
   let mut rng = thread_rng();
 
@@ -948,7 +950,8 @@ pub fn op_fs_make_temp_file_sync<P>(
 where
   P: FsPermissions + 'static,
 {
-  let (dir, fs) = make_temp_check_sync::<P>(state, dir)?;
+  let (dir, fs) =
+    make_temp_check_sync::<P>(state, dir, "Deno.makeTempFileSync()")?;
 
   let open_opts = OpenOptions {
     write: true,
@@ -989,7 +992,8 @@ pub async fn op_fs_make_temp_file_async<P>(
 where
   P: FsPermissions + 'static,
 {
-  let (dir, fs) = make_temp_check_async::<P>(state, dir)?;
+  let (dir, fs) =
+    make_temp_check_async::<P>(state, dir, "Deno.makeTempFile()")?;
 
   let open_opts = OpenOptions {
     write: true,
@@ -1021,6 +1025,7 @@ where
 fn make_temp_check_sync<P>(
   state: &mut OpState,
   dir: Option<String>,
+  api_name: &str,
 ) -> Result<(PathBuf, FileSystemRc), AnyError>
 where
   P: FsPermissions + 'static,
@@ -1029,18 +1034,14 @@ where
   let dir = match dir {
     Some(dir) => {
       let dir = PathBuf::from(dir);
-      state
+      state.borrow_mut::<P>().check_write(&dir, api_name)?;
-        .borrow_mut::<P>()
-        .check_write(&dir, "Deno.makeTempFile()")?;
       dir
     }
     None => {
       let dir = fs.tmp_dir().context("tmpdir")?;
-      state.borrow_mut::<P>().check_write_blind(
+      state
-        &dir,
+        .borrow_mut::<P>()
-        "TMP",
+        .check_write_blind(&dir, "TMP", api_name)?;
-        "Deno.makeTempFile()",
-      )?;
       dir
     }
   };
@@ -1050,6 +1051,7 @@ where
 fn make_temp_check_async<P>(
   state: Rc<RefCell<OpState>>,
   dir: Option<String>,
+  api_name: &str,
 ) -> Result<(PathBuf, FileSystemRc), AnyError>
 where
   P: FsPermissions + 'static,
@@ -1059,24 +1061,57 @@ where
   let dir = match dir {
     Some(dir) => {
       let dir = PathBuf::from(dir);
-      state
+      state.borrow_mut::<P>().check_write(&dir, api_name)?;
-        .borrow_mut::<P>()
-        .check_write(&dir, "Deno.makeTempFile()")?;
       dir
     }
     None => {
       let dir = fs.tmp_dir().context("tmpdir")?;
-      state.borrow_mut::<P>().check_write_blind(
+      state
-        &dir,
+        .borrow_mut::<P>()
-        "TMP",
+        .check_write_blind(&dir, "TMP", api_name)?;
-        "Deno.makeTempFile()",
-      )?;
       dir
     }
   };
   Ok((dir, fs))
 }
 
+/// Identify illegal filename characters before attempting to use them in a filesystem
+/// operation. We're a bit stricter with tempfile and tempdir names than with regular
+/// files.
+fn validate_temporary_filename_component(
+  component: &str,
+  #[allow(unused_variables)] suffix: bool,
+) -> Result<(), AnyError> {
+  // Ban ASCII and Unicode control characters: these will often fail
+  if let Some(c) = component.matches(|c: char| c.is_control()).next() {
+    bail!("Invalid control character in prefix or suffix: {:?}", c);
+  }
+  // Windows has the most restrictive filenames. As temp files aren't normal files, we just
+  // use this set of banned characters for all platforms because wildcard-like files can also
+  // be problematic in unix-like shells.
+
+  // The list of banned characters in Windows:
+  // https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file#naming-conventions
+
+  // You might ask why <, >, and " are included in the Windows list? It's because they are
+  // special wildcard implemented in the filesystem driver!
+  // https://learn.microsoft.com/en-ca/archive/blogs/jeremykuhne/wildcards-in-windows
+  if let Some(c) = component
+    .matches(|c: char| "<>:\"/\\|?*".contains(c))
+    .next()
+  {
+    bail!("Invalid character in prefix or suffix: {:?}", c);
+  }
+
+  // This check is only for Windows
+  #[cfg(windows)]
+  if suffix && component.ends_with(|c: char| ". ".contains(c)) {
+    bail!("Invalid trailing character in suffix");
+  }
+
+  Ok(())
+}
+
 fn tmp_name(
   rng: &mut ThreadRng,
   dir: &Path,
@@ -1084,12 +1119,18 @@ fn tmp_name(
   suffix: Option<&str>,
 ) -> Result<PathBuf, AnyError> {
   let prefix = prefix.unwrap_or("");
+  validate_temporary_filename_component(prefix, false)?;
   let suffix = suffix.unwrap_or("");
+  validate_temporary_filename_component(suffix, true)?;
 
-  let mut path = dir.join("_");
+  // If we use a 32-bit number, we only need ~70k temp files before we have a 50%
-
+  // chance of collision. By bumping this up to 64-bits, we require ~5 billion
-  let unique = rng.gen::<u32>();
+  // before hitting a 50% chance. We also base32-encode this value so the entire
-  path.set_file_name(format!("{prefix}{unique:08x}{suffix}"));
+  // thing is 1) case insensitive and 2) slightly shorter than the equivalent hex
+  // value.
+  let unique = rng.gen::<u64>();
+  base32::encode(base32::Alphabet::Crockford, &unique.to_le_bytes());
+  let path = dir.join(format!("{prefix}{unique:08x}{suffix}"));
 
   Ok(path)
 }

tests/unit/make_temp_test.ts

@@ -155,3 +155,25 @@ Deno.test(
     }
   },
 );
+
+Deno.test(
+  { permissions: { read: true, write: true } },
+  function makeTempInvalidCharacter() {
+    // Various control and ASCII characters that are disallowed on all platforms
+    for (const invalid of ["\0", "*", "\x9f"]) {
+      assertThrows(() => Deno.makeTempFileSync({ prefix: invalid }));
+      assertThrows(() => Deno.makeTempDirSync({ prefix: invalid }));
+      assertThrows(() => Deno.makeTempFileSync({ suffix: invalid }));
+      assertThrows(() => Deno.makeTempDirSync({ suffix: invalid }));
+    }
+
+    // On Windows, files can't end with space or period
+    if (Deno.build.os === "windows") {
+      assertThrows(() => Deno.makeTempFileSync({ suffix: "." }));
+      assertThrows(() => Deno.makeTempFileSync({ suffix: " " }));
+    } else {
+      Deno.makeTempFileSync({ suffix: "." });
+      Deno.makeTempFileSync({ suffix: " " });
+    }
+  },
+);