diff --git a/Cargo.lock b/Cargo.lock index 01f2ce4a75..20613ec66c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1402,6 +1402,7 @@ name = "deno_fs" version = "0.48.0" dependencies = [ "async-trait", + "base32", "deno_core", "deno_io", "filetime", diff --git a/Cargo.toml b/Cargo.toml index 6bcf3a2229..5ea993f053 100644 --- a/Cargo.toml +++ b/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" diff --git a/cli/Cargo.toml b/cli/Cargo.toml index a8b7a0b258..67795b3c21 100644 --- a/cli/Cargo.toml +++ b/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 diff --git a/ext/fs/Cargo.toml b/ext/fs/Cargo.toml index 81dab267f3..8bdc5cf38c 100644 --- a/ext/fs/Cargo.toml +++ b/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 diff --git a/ext/fs/ops.rs b/ext/fs/ops.rs index bc9f75a3ca..d688f619ee 100644 --- a/ext/fs/ops.rs +++ b/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

( where P: FsPermissions + 'static, { - let (dir, fs) = make_temp_check_sync::

(state, dir)?; + let (dir, fs) = + make_temp_check_sync::

(state, dir, "Deno.makeTempDirSync()")?; let mut rng = thread_rng(); @@ -914,7 +916,7 @@ pub async fn op_fs_make_temp_dir_async

( where P: FsPermissions + 'static, { - let (dir, fs) = make_temp_check_async::

(state, dir)?; + let (dir, fs) = make_temp_check_async::

(state, dir, "Deno.makeTempDir()")?; let mut rng = thread_rng(); @@ -948,7 +950,8 @@ pub fn op_fs_make_temp_file_sync

( where P: FsPermissions + 'static, { - let (dir, fs) = make_temp_check_sync::

(state, dir)?; + let (dir, fs) = + make_temp_check_sync::

(state, dir, "Deno.makeTempFileSync()")?; let open_opts = OpenOptions { write: true, @@ -989,7 +992,8 @@ pub async fn op_fs_make_temp_file_async

( where P: FsPermissions + 'static, { - let (dir, fs) = make_temp_check_async::

(state, dir)?; + let (dir, fs) = + make_temp_check_async::

(state, dir, "Deno.makeTempFile()")?; let open_opts = OpenOptions { write: true, @@ -1021,6 +1025,7 @@ where fn make_temp_check_sync

( state: &mut OpState, dir: Option, + 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 - .borrow_mut::

() - .check_write(&dir, "Deno.makeTempFile()")?; + state.borrow_mut::

().check_write(&dir, api_name)?; dir } None => { let dir = fs.tmp_dir().context("tmpdir")?; - state.borrow_mut::

().check_write_blind( - &dir, - "TMP", - "Deno.makeTempFile()", - )?; + state + .borrow_mut::

() + .check_write_blind(&dir, "TMP", api_name)?; dir } }; @@ -1050,6 +1051,7 @@ where fn make_temp_check_async

( state: Rc>, dir: Option, + 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 - .borrow_mut::

() - .check_write(&dir, "Deno.makeTempFile()")?; + state.borrow_mut::

().check_write(&dir, api_name)?; dir } None => { let dir = fs.tmp_dir().context("tmpdir")?; - state.borrow_mut::

().check_write_blind( - &dir, - "TMP", - "Deno.makeTempFile()", - )?; + state + .borrow_mut::

() + .check_write_blind(&dir, "TMP", api_name)?; 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 { 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("_"); - - let unique = rng.gen::(); - path.set_file_name(format!("{prefix}{unique:08x}{suffix}")); + // 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 + // before hitting a 50% chance. We also base32-encode this value so the entire + // thing is 1) case insensitive and 2) slightly shorter than the equivalent hex + // value. + let unique = rng.gen::(); + base32::encode(base32::Alphabet::Crockford, &unique.to_le_bytes()); + let path = dir.join(format!("{prefix}{unique:08x}{suffix}")); Ok(path) } diff --git a/tests/unit/make_temp_test.ts b/tests/unit/make_temp_test.ts index cbbae8dfe8..2c771177bc 100644 --- a/tests/unit/make_temp_test.ts +++ b/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: " " }); + } + }, +);