From a96844118c24d870abfe5332547dab99dc53d09c Mon Sep 17 00:00:00 2001 From: David Sherret Date: Sat, 27 May 2023 10:33:15 -0400 Subject: [PATCH] fix(compile): inline symlinks as files outside node_modules dir and warn for directories (#19285) If a symlink within the `node_modules` directory lies outside that directory, it will now warn and inline the file. For directories, it will just warn for now. Probably fixes #19251 (I'm still unable to reproduce). --- cli/standalone/virtual_fs.rs | 151 +++++++++++++----- cli/tests/integration/compile_tests.rs | 65 ++++++++ .../node_modules_symlink_outside/main.out | 2 + .../node_modules_symlink_outside/main.ts | 6 + .../main_compile_file.out | 2 + .../main_compile_folder.out | 6 + test_util/src/temp_dir.rs | 36 +++++ 7 files changed, 229 insertions(+), 39 deletions(-) create mode 100644 cli/tests/testdata/compile/node_modules_symlink_outside/main.out create mode 100644 cli/tests/testdata/compile/node_modules_symlink_outside/main.ts create mode 100644 cli/tests/testdata/compile/node_modules_symlink_outside/main_compile_file.out create mode 100644 cli/tests/testdata/compile/node_modules_symlink_outside/main_compile_folder.out diff --git a/cli/standalone/virtual_fs.rs b/cli/standalone/virtual_fs.rs index e94758bd90..42416554da 100644 --- a/cli/standalone/virtual_fs.rs +++ b/cli/standalone/virtual_fs.rs @@ -24,9 +24,19 @@ use deno_runtime::deno_io::fs::FsResult; use deno_runtime::deno_io::fs::FsStat; use serde::Deserialize; use serde::Serialize; +use thiserror::Error; use crate::util; +#[derive(Error, Debug)] +#[error( + "Failed to strip prefix '{}' from '{}'", root_path.display(), target.display() +)] +pub struct StripRootError { + root_path: PathBuf, + target: PathBuf, +} + pub struct VfsBuilder { root_path: PathBuf, root_dir: VirtualDirectory, @@ -37,6 +47,7 @@ pub struct VfsBuilder { impl VfsBuilder { pub fn new(root_path: PathBuf) -> Self { + log::debug!("Building vfs with root '{}'", root_path.display()); Self { root_dir: VirtualDirectory { name: root_path @@ -58,7 +69,7 @@ impl VfsBuilder { } pub fn add_dir_recursive(&mut self, path: &Path) -> Result<(), AnyError> { - self.add_dir(path); + self.add_dir(path)?; let read_dir = std::fs::read_dir(path) .with_context(|| format!("Reading {}", path.display()))?; @@ -72,19 +83,44 @@ impl VfsBuilder { } else if file_type.is_file() { let file_bytes = std::fs::read(&path) .with_context(|| format!("Reading {}", path.display()))?; - self.add_file(&path, file_bytes); + self.add_file(&path, file_bytes)?; } else if file_type.is_symlink() { - let target = std::fs::read_link(&path) + let target = util::fs::canonicalize_path(&path) .with_context(|| format!("Reading symlink {}", path.display()))?; - self.add_symlink(&path, &target); + if let Err(StripRootError { .. }) = self.add_symlink(&path, &target) { + if target.is_file() { + // this may change behavior, so warn the user about it + log::warn!( + "Symlink target is outside '{}'. Inlining symlink at '{}' to '{}' as file.", + self.root_path.display(), + path.display(), + target.display(), + ); + // inline the symlink and make the target file + let file_bytes = std::fs::read(&target) + .with_context(|| format!("Reading {}", path.display()))?; + self.add_file(&path, file_bytes)?; + } else { + log::warn!( + "Symlink target is outside '{}'. Excluding symlink at '{}' with target '{}'.", + self.root_path.display(), + path.display(), + target.display(), + ); + } + } } } Ok(()) } - pub fn add_dir(&mut self, path: &Path) -> &mut VirtualDirectory { - let path = self.path_relative_root(path); + pub fn add_dir( + &mut self, + path: &Path, + ) -> Result<&mut VirtualDirectory, StripRootError> { + log::debug!("Ensuring directory '{}'", path.display()); + let path = self.path_relative_root(path)?; let mut current_dir = &mut self.root_dir; for component in path.components() { @@ -113,10 +149,15 @@ impl VfsBuilder { }; } - current_dir + Ok(current_dir) } - pub fn add_file(&mut self, path: &Path, data: Vec) { + pub fn add_file( + &mut self, + path: &Path, + data: Vec, + ) -> Result<(), AnyError> { + log::debug!("Adding file '{}'", path.display()); let checksum = util::checksum::gen(&[&data]); let offset = if let Some(offset) = self.file_offsets.get(&checksum) { // duplicate file, reuse an old offset @@ -126,7 +167,7 @@ impl VfsBuilder { self.current_offset }; - let dir = self.add_dir(path.parent().unwrap()); + let dir = self.add_dir(path.parent().unwrap())?; let name = path.file_name().unwrap().to_string_lossy(); let data_len = data.len(); match dir.entries.binary_search_by(|e| e.name().cmp(&name)) { @@ -148,11 +189,22 @@ impl VfsBuilder { self.files.push(data); self.current_offset += data_len as u64; } + + Ok(()) } - pub fn add_symlink(&mut self, path: &Path, target: &Path) { - let dest = self.path_relative_root(target); - let dir = self.add_dir(path.parent().unwrap()); + pub fn add_symlink( + &mut self, + path: &Path, + target: &Path, + ) -> Result<(), StripRootError> { + log::debug!( + "Adding symlink '{}' to '{}'", + path.display(), + target.display() + ); + let dest = self.path_relative_root(target)?; + let dir = self.add_dir(path.parent().unwrap())?; let name = path.file_name().unwrap().to_string_lossy(); match dir.entries.binary_search_by(|e| e.name().cmp(&name)) { Ok(_) => unreachable!(), @@ -169,23 +221,20 @@ impl VfsBuilder { ); } } + Ok(()) } pub fn into_dir_and_files(self) -> (VirtualDirectory, Vec>) { (self.root_dir, self.files) } - fn path_relative_root(&self, path: &Path) -> PathBuf { + fn path_relative_root(&self, path: &Path) -> Result { match path.strip_prefix(&self.root_path) { - Ok(p) => p.to_path_buf(), - Err(err) => { - panic!( - "Failed to strip prefix '{}' from '{}': {:#}", - self.root_path.display(), - path.display(), - err - ) - } + Ok(p) => Ok(p.to_path_buf()), + Err(_) => Err(StripRootError { + root_path: self.root_path.clone(), + target: path.to_path_buf(), + }), } } } @@ -457,7 +506,11 @@ impl FileBackedVfsFile { } SeekFrom::End(offset) => { if offset < 0 && -offset as u64 > self.file.len { - Err(std::io::Error::new(std::io::ErrorKind::PermissionDenied, "An attempt was made to move the file pointer before the beginning of the file.").into()) + let msg = "An attempt was made to move the file pointer before the beginning of the file."; + Err( + std::io::Error::new(std::io::ErrorKind::PermissionDenied, msg) + .into(), + ) } else { let mut current_pos = self.pos.lock(); *current_pos = if offset >= 0 { @@ -790,16 +843,28 @@ mod test { let temp_dir = TempDir::new(); let src_path = temp_dir.path().join("src"); let mut builder = VfsBuilder::new(src_path.clone()); - builder.add_file(&src_path.join("a.txt"), "data".into()); - builder.add_file(&src_path.join("b.txt"), "data".into()); + builder + .add_file(&src_path.join("a.txt"), "data".into()) + .unwrap(); + builder + .add_file(&src_path.join("b.txt"), "data".into()) + .unwrap(); assert_eq!(builder.files.len(), 1); // because duplicate data - builder.add_file(&src_path.join("c.txt"), "c".into()); - builder.add_file(&src_path.join("sub_dir").join("d.txt"), "d".into()); - builder.add_file(&src_path.join("e.txt"), "e".into()); - builder.add_symlink( - &src_path.join("sub_dir").join("e.txt"), - &src_path.join("e.txt"), - ); + builder + .add_file(&src_path.join("c.txt"), "c".into()) + .unwrap(); + builder + .add_file(&src_path.join("sub_dir").join("d.txt"), "d".into()) + .unwrap(); + builder + .add_file(&src_path.join("e.txt"), "e".into()) + .unwrap(); + builder + .add_symlink( + &src_path.join("sub_dir").join("e.txt"), + &src_path.join("e.txt"), + ) + .unwrap(); // get the virtual fs let (dest_path, virtual_fs) = into_virtual_fs(builder, &temp_dir); @@ -923,9 +988,15 @@ mod test { let temp_dir = TempDir::new(); let src_path = temp_dir.path().join("src"); let mut builder = VfsBuilder::new(src_path.clone()); - builder.add_symlink(&src_path.join("a.txt"), &src_path.join("b.txt")); - builder.add_symlink(&src_path.join("b.txt"), &src_path.join("c.txt")); - builder.add_symlink(&src_path.join("c.txt"), &src_path.join("a.txt")); + builder + .add_symlink(&src_path.join("a.txt"), &src_path.join("b.txt")) + .unwrap(); + builder + .add_symlink(&src_path.join("b.txt"), &src_path.join("c.txt")) + .unwrap(); + builder + .add_symlink(&src_path.join("c.txt"), &src_path.join("a.txt")) + .unwrap(); let (dest_path, virtual_fs) = into_virtual_fs(builder, &temp_dir); assert_eq!( virtual_fs @@ -950,10 +1021,12 @@ mod test { let temp_dir = TempDir::new(); let temp_path = temp_dir.path(); let mut builder = VfsBuilder::new(temp_path.to_path_buf()); - builder.add_file( - &temp_path.join("a.txt"), - "0123456789".to_string().into_bytes(), - ); + builder + .add_file( + &temp_path.join("a.txt"), + "0123456789".to_string().into_bytes(), + ) + .unwrap(); let (dest_path, virtual_fs) = into_virtual_fs(builder, &temp_dir); let virtual_fs = Arc::new(virtual_fs); let file = virtual_fs.open_file(&dest_path.join("a.txt")).unwrap(); diff --git a/cli/tests/integration/compile_tests.rs b/cli/tests/integration/compile_tests.rs index 4938b36cd2..f202fc87e1 100644 --- a/cli/tests/integration/compile_tests.rs +++ b/cli/tests/integration/compile_tests.rs @@ -1082,3 +1082,68 @@ fn run_npm_bin_compile_test(opts: RunNpmBinCompileOptions) { output.assert_matches_file(opts.output_file); output.assert_exit_code(opts.exit_code); } + +#[test] +fn compile_node_modules_symlink_outside() { + let context = TestContextBuilder::for_npm() + .use_sync_npm_download() + .use_copy_temp_dir("compile/node_modules_symlink_outside") + .cwd("compile/node_modules_symlink_outside") + .build(); + + let temp_dir = context.temp_dir(); + let project_dir = temp_dir + .path() + .join("compile") + .join("node_modules_symlink_outside"); + temp_dir.create_dir_all(project_dir.join("node_modules")); + temp_dir.create_dir_all(project_dir.join("some_folder")); + temp_dir.write(project_dir.join("test.txt"), "5"); + + // create a symlink in the node_modules directory that points to a folder in the cwd + temp_dir.symlink_dir( + project_dir.join("some_folder"), + project_dir.join("node_modules").join("some_folder"), + ); + // compile folder + let output = context + .new_command() + .args("compile --allow-read --node-modules-dir --output bin main.ts") + .run(); + output.assert_exit_code(0); + output.assert_matches_file( + "compile/node_modules_symlink_outside/main_compile_folder.out", + ); + assert!(project_dir.join("node_modules/some_folder").exists()); + + // Cleanup and remove the folder. The folder test is done separately from + // the file symlink test because different systems would traverse + // the directory items in different order. + temp_dir.remove_dir_all(project_dir.join("node_modules/some_folder")); + + // create a symlink in the node_modules directory that points to a file in the cwd + temp_dir.symlink_file( + project_dir.join("test.txt"), + project_dir.join("node_modules").join("test.txt"), + ); + assert!(project_dir.join("node_modules/test.txt").exists()); + + // compile + let output = context + .new_command() + .args("compile --allow-read --node-modules-dir --output bin main.ts") + .run(); + output.assert_exit_code(0); + output.assert_matches_file( + "compile/node_modules_symlink_outside/main_compile_file.out", + ); + + // run + let binary_path = + project_dir.join(if cfg!(windows) { "bin.exe" } else { "bin" }); + let output = context + .new_command() + .command_name(binary_path.to_string_lossy()) + .run(); + output.assert_matches_file("compile/node_modules_symlink_outside/main.out"); +} diff --git a/cli/tests/testdata/compile/node_modules_symlink_outside/main.out b/cli/tests/testdata/compile/node_modules_symlink_outside/main.out new file mode 100644 index 0000000000..61c83cba41 --- /dev/null +++ b/cli/tests/testdata/compile/node_modules_symlink_outside/main.out @@ -0,0 +1,2 @@ +4 +5 diff --git a/cli/tests/testdata/compile/node_modules_symlink_outside/main.ts b/cli/tests/testdata/compile/node_modules_symlink_outside/main.ts new file mode 100644 index 0000000000..45f681f7ca --- /dev/null +++ b/cli/tests/testdata/compile/node_modules_symlink_outside/main.ts @@ -0,0 +1,6 @@ +import { getValue, setValue } from "npm:@denotest/esm-basic"; + +setValue(4); + +console.log(getValue()); +console.log(Deno.readTextFileSync("./node_modules/test.txt")); diff --git a/cli/tests/testdata/compile/node_modules_symlink_outside/main_compile_file.out b/cli/tests/testdata/compile/node_modules_symlink_outside/main_compile_file.out new file mode 100644 index 0000000000..7602a40028 --- /dev/null +++ b/cli/tests/testdata/compile/node_modules_symlink_outside/main_compile_file.out @@ -0,0 +1,2 @@ +Compile file:///[WILDCARD]/node_modules_symlink_outside/main.ts to [WILDCARD] +Symlink target is outside '[WILDCARD]node_modules_symlink_outside[WILDCARD]node_modules'. Inlining symlink at '[WILDCARD]node_modules_symlink_outside[WILDCARD]node_modules[WILDCARD]test.txt' to '[WILDCARD]node_modules_symlink_outside[WILDCARD]test.txt' as file. diff --git a/cli/tests/testdata/compile/node_modules_symlink_outside/main_compile_folder.out b/cli/tests/testdata/compile/node_modules_symlink_outside/main_compile_folder.out new file mode 100644 index 0000000000..883a3f262f --- /dev/null +++ b/cli/tests/testdata/compile/node_modules_symlink_outside/main_compile_folder.out @@ -0,0 +1,6 @@ +Download http://localhost:4545/npm/registry/@denotest/esm-basic +Download http://localhost:4545/npm/registry/@denotest/esm-basic/1.0.0.tgz +Initialize @denotest/esm-basic@1.0.0 +Check file:///[WILDCARD]/node_modules_symlink_outside/main.ts +Compile file:///[WILDCARD]/node_modules_symlink_outside/main.ts to [WILDCARD] +Symlink target is outside '[WILDCARD]node_modules_symlink_outside[WILDCARD]node_modules'. Excluding symlink at '[WILDCARD]node_modules_symlink_outside[WILDCARD]node_modules[WILDCARD]some_folder' with target '[WILDCARD]node_modules_symlink_outside[WILDCARD]some_folder'. diff --git a/test_util/src/temp_dir.rs b/test_util/src/temp_dir.rs index f0f866d886..f66bf1398b 100644 --- a/test_util/src/temp_dir.rs +++ b/test_util/src/temp_dir.rs @@ -80,4 +80,40 @@ impl TempDir { pub fn write(&self, path: impl AsRef, text: impl AsRef) { fs::write(self.path().join(path), text.as_ref()).unwrap(); } + + pub fn symlink_dir( + &self, + oldpath: impl AsRef, + newpath: impl AsRef, + ) { + #[cfg(unix)] + { + use std::os::unix::fs::symlink; + symlink(self.path().join(oldpath), self.path().join(newpath)).unwrap(); + } + #[cfg(not(unix))] + { + use std::os::windows::fs::symlink_dir; + symlink_dir(self.path().join(oldpath), self.path().join(newpath)) + .unwrap(); + } + } + + pub fn symlink_file( + &self, + oldpath: impl AsRef, + newpath: impl AsRef, + ) { + #[cfg(unix)] + { + use std::os::unix::fs::symlink; + symlink(self.path().join(oldpath), self.path().join(newpath)).unwrap(); + } + #[cfg(not(unix))] + { + use std::os::windows::fs::symlink_file; + symlink_file(self.path().join(oldpath), self.path().join(newpath)) + .unwrap(); + } + } }