From bed46474b2d59b229bd85dbdec5b3d943c32f60f Mon Sep 17 00:00:00 2001 From: Yazan AbdAl-Rahman Date: Wed, 18 Sep 2024 16:51:39 +0300 Subject: [PATCH] fix: do not panic running invalid file specifier (#25530) Co-authored-by: Bedis Nbiba --- Cargo.lock | 1 + cli/args/flags.rs | 3 +- cli/graph_util.rs | 2 +- cli/lsp/analysis.rs | 2 +- cli/lsp/cache.rs | 2 +- cli/lsp/completions.rs | 2 +- cli/lsp/documents.rs | 2 +- cli/lsp/language_server.rs | 2 +- cli/lsp/resolver.rs | 46 ++++++++--------- cli/lsp/tsc.rs | 2 +- cli/npm/byonm.rs | 2 +- runtime/fs_util.rs | 67 ++----------------------- runtime/permissions/Cargo.toml | 1 + runtime/permissions/lib.rs | 91 +++++++++++++++++++++++++++++++--- tests/integration/run_tests.rs | 24 +++++++++ 15 files changed, 146 insertions(+), 103 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 48b174c0f4..077d31f19a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1924,6 +1924,7 @@ dependencies = [ "libc", "log", "once_cell", + "percent-encoding", "serde", "which 4.4.2", "winapi", diff --git a/cli/args/flags.rs b/cli/args/flags.rs index 92336a0a16..da07c16d84 100644 --- a/cli/args/flags.rs +++ b/cli/args/flags.rs @@ -34,6 +34,7 @@ use deno_core::url::Url; use deno_graph::GraphKind; use deno_runtime::deno_permissions::parse_sys_kind; use deno_runtime::deno_permissions::PermissionsOptions; +use deno_runtime::fs_util::specifier_to_file_path; use log::debug; use log::Level; use serde::Deserialize; @@ -918,7 +919,7 @@ impl Flags { if module_specifier.scheme() == "file" || module_specifier.scheme() == "npm" { - if let Ok(p) = module_specifier.to_file_path() { + if let Ok(p) = specifier_to_file_path(&module_specifier) { Some(vec![p.parent().unwrap().to_path_buf()]) } else { Some(vec![current_dir.to_path_buf()]) diff --git a/cli/graph_util.rs b/cli/graph_util.rs index cb2e1bde87..cd98c3824d 100644 --- a/cli/graph_util.rs +++ b/cli/graph_util.rs @@ -25,7 +25,6 @@ use deno_graph::source::LoaderChecksum; use deno_graph::JsrLoadError; use deno_graph::ModuleLoadError; use deno_graph::WorkspaceFastCheckOption; -use deno_runtime::fs_util::specifier_to_file_path; use deno_core::error::custom_error; use deno_core::error::AnyError; @@ -42,6 +41,7 @@ use deno_graph::ResolutionError; use deno_graph::SpecifierError; use deno_runtime::deno_fs::FileSystem; use deno_runtime::deno_node; +use deno_runtime::fs_util::specifier_to_file_path; use deno_semver::jsr::JsrDepPackageReq; use deno_semver::package::PackageNv; use deno_semver::Version; diff --git a/cli/lsp/analysis.rs b/cli/lsp/analysis.rs index eeab796bca..fee9c86cbe 100644 --- a/cli/lsp/analysis.rs +++ b/cli/lsp/analysis.rs @@ -11,7 +11,6 @@ use super::urls::url_to_uri; use crate::args::jsr_url; use crate::tools::lint::CliLinter; use deno_lint::diagnostic::LintDiagnosticRange; -use deno_runtime::fs_util::specifier_to_file_path; use deno_ast::SourceRange; use deno_ast::SourceRangedForSpanned; @@ -25,6 +24,7 @@ use deno_core::serde_json; use deno_core::serde_json::json; use deno_core::ModuleSpecifier; use deno_runtime::deno_node::PathClean; +use deno_runtime::fs_util::specifier_to_file_path; use deno_semver::jsr::JsrPackageNvReference; use deno_semver::jsr::JsrPackageReqReference; use deno_semver::npm::NpmPackageReqReference; diff --git a/cli/lsp/cache.rs b/cli/lsp/cache.rs index d3c05ca91a..a32087842d 100644 --- a/cli/lsp/cache.rs +++ b/cli/lsp/cache.rs @@ -7,10 +7,10 @@ use crate::cache::LocalLspHttpCache; use crate::lsp::config::Config; use crate::lsp::logging::lsp_log; use crate::lsp::logging::lsp_warn; -use deno_runtime::fs_util::specifier_to_file_path; use deno_core::url::Url; use deno_core::ModuleSpecifier; +use deno_runtime::fs_util::specifier_to_file_path; use std::collections::BTreeMap; use std::fs; use std::path::Path; diff --git a/cli/lsp/completions.rs b/cli/lsp/completions.rs index 1e5504d75c..b3d6fbbd04 100644 --- a/cli/lsp/completions.rs +++ b/cli/lsp/completions.rs @@ -19,7 +19,6 @@ use crate::util::path::relative_specifier; use deno_graph::source::ResolutionMode; use deno_graph::Range; use deno_runtime::deno_node::SUPPORTED_BUILTIN_NODE_MODULES; -use deno_runtime::fs_util::specifier_to_file_path; use deno_ast::LineAndColumnIndex; use deno_ast::SourceTextInfo; @@ -30,6 +29,7 @@ use deno_core::serde::Serialize; use deno_core::serde_json::json; use deno_core::url::Position; use deno_core::ModuleSpecifier; +use deno_runtime::fs_util::specifier_to_file_path; use deno_semver::jsr::JsrPackageReqReference; use deno_semver::package::PackageNv; use import_map::ImportMap; diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs index d6af96b238..e96b8831bb 100644 --- a/cli/lsp/documents.rs +++ b/cli/lsp/documents.rs @@ -11,7 +11,6 @@ use super::tsc; use super::tsc::AssetDocument; use crate::graph_util::CliJsrUrlProvider; -use deno_runtime::fs_util::specifier_to_file_path; use dashmap::DashMap; use deno_ast::swc::visit::VisitWith; @@ -28,6 +27,7 @@ use deno_core::ModuleSpecifier; use deno_graph::source::ResolutionMode; use deno_graph::Resolution; use deno_runtime::deno_node; +use deno_runtime::fs_util::specifier_to_file_path; use deno_semver::jsr::JsrPackageReqReference; use deno_semver::npm::NpmPackageReqReference; use deno_semver::package::PackageReq; diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index c859abc024..5f6e380823 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -17,6 +17,7 @@ use deno_graph::GraphKind; use deno_graph::Resolution; use deno_runtime::deno_tls::rustls::RootCertStore; use deno_runtime::deno_tls::RootCertStoreProvider; +use deno_runtime::fs_util::specifier_to_file_path; use deno_semver::jsr::JsrPackageReqReference; use indexmap::Equivalent; use indexmap::IndexSet; @@ -112,7 +113,6 @@ use crate::util::fs::remove_dir_all_if_exists; use crate::util::path::is_importable_ext; use crate::util::path::to_percent_decoded_str; use crate::util::sync::AsyncFlag; -use deno_runtime::fs_util::specifier_to_file_path; struct LspRootCertStoreProvider(RootCertStore); diff --git a/cli/lsp/resolver.rs b/cli/lsp/resolver.rs index 1c3d5c88b2..2844eb6f98 100644 --- a/cli/lsp/resolver.rs +++ b/cli/lsp/resolver.rs @@ -1,28 +1,5 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. -use crate::args::create_default_npmrc; -use crate::args::CacheSetting; -use crate::args::CliLockfile; -use crate::args::NpmInstallDepsProvider; -use crate::graph_util::CliJsrUrlProvider; -use crate::http_util::HttpClientProvider; -use crate::lsp::config::Config; -use crate::lsp::config::ConfigData; -use crate::lsp::logging::lsp_warn; -use crate::npm::create_cli_npm_resolver_for_lsp; -use crate::npm::CliNpmResolver; -use crate::npm::CliNpmResolverByonmCreateOptions; -use crate::npm::CliNpmResolverCreateOptions; -use crate::npm::CliNpmResolverManagedCreateOptions; -use crate::npm::CliNpmResolverManagedSnapshotOption; -use crate::npm::ManagedCliNpmResolver; -use crate::resolver::CjsResolutionStore; -use crate::resolver::CliGraphResolver; -use crate::resolver::CliGraphResolverOptions; -use crate::resolver::CliNodeResolver; -use crate::resolver::WorkerCliNpmGraphResolver; -use crate::util::progress_bar::ProgressBar; -use crate::util::progress_bar::ProgressBarStyle; use dashmap::DashMap; use deno_ast::MediaType; use deno_cache_dir::HttpCache; @@ -55,6 +32,29 @@ use std::sync::Arc; use super::cache::LspCache; use super::jsr::JsrCacheResolver; +use crate::args::create_default_npmrc; +use crate::args::CacheSetting; +use crate::args::CliLockfile; +use crate::args::NpmInstallDepsProvider; +use crate::graph_util::CliJsrUrlProvider; +use crate::http_util::HttpClientProvider; +use crate::lsp::config::Config; +use crate::lsp::config::ConfigData; +use crate::lsp::logging::lsp_warn; +use crate::npm::create_cli_npm_resolver_for_lsp; +use crate::npm::CliNpmResolver; +use crate::npm::CliNpmResolverByonmCreateOptions; +use crate::npm::CliNpmResolverCreateOptions; +use crate::npm::CliNpmResolverManagedCreateOptions; +use crate::npm::CliNpmResolverManagedSnapshotOption; +use crate::npm::ManagedCliNpmResolver; +use crate::resolver::CjsResolutionStore; +use crate::resolver::CliGraphResolver; +use crate::resolver::CliGraphResolverOptions; +use crate::resolver::CliNodeResolver; +use crate::resolver::WorkerCliNpmGraphResolver; +use crate::util::progress_bar::ProgressBar; +use crate::util::progress_bar::ProgressBarStyle; #[derive(Debug, Clone)] struct LspScopeResolver { diff --git a/cli/lsp/tsc.rs b/cli/lsp/tsc.rs index 6d3e1317b1..fe3708d3cb 100644 --- a/cli/lsp/tsc.rs +++ b/cli/lsp/tsc.rs @@ -39,7 +39,6 @@ use deno_core::convert::ToV8; use deno_core::error::StdAnyError; use deno_core::futures::stream::FuturesOrdered; use deno_core::futures::StreamExt; -use deno_runtime::fs_util::specifier_to_file_path; use dashmap::DashMap; use deno_ast::MediaType; @@ -63,6 +62,7 @@ use deno_core::ModuleSpecifier; use deno_core::OpState; use deno_core::PollEventLoopOptions; use deno_core::RuntimeOptions; +use deno_runtime::fs_util::specifier_to_file_path; use deno_runtime::inspector_server::InspectorServer; use deno_runtime::tokio_util::create_basic_runtime; use indexmap::IndexMap; diff --git a/cli/npm/byonm.rs b/cli/npm/byonm.rs index 24645e4164..fc7069e1f8 100644 --- a/cli/npm/byonm.rs +++ b/cli/npm/byonm.rs @@ -16,6 +16,7 @@ use deno_runtime::deno_node::NodePermissions; use deno_runtime::deno_node::NodeRequireResolver; use deno_runtime::deno_node::NpmProcessStateProvider; use deno_runtime::deno_node::PackageJson; +use deno_runtime::fs_util::specifier_to_file_path; use deno_semver::package::PackageReq; use deno_semver::Version; use node_resolver::errors::PackageFolderResolveError; @@ -28,7 +29,6 @@ use node_resolver::NpmResolver; use crate::args::NpmProcessState; use crate::args::NpmProcessStateKind; use crate::util::fs::canonicalize_path_maybe_not_exists_with_fs; -use deno_runtime::fs_util::specifier_to_file_path; use super::managed::normalize_pkg_name_for_node_modules_deno_folder; use super::CliNpmResolver; diff --git a/runtime/fs_util.rs b/runtime/fs_util.rs index fe97360384..15ebafdf61 100644 --- a/runtime/fs_util.rs +++ b/runtime/fs_util.rs @@ -1,13 +1,13 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. -use deno_ast::ModuleSpecifier; use deno_core::anyhow::Context; -use deno_core::error::uri_error; use deno_core::error::AnyError; -pub use deno_core::normalize_path; use std::path::Path; use std::path::PathBuf; +pub use deno_core::normalize_path; +pub use deno_permissions::specifier_to_file_path; + #[inline] pub fn resolve_from_cwd(path: &Path) -> Result { if path.is_absolute() { @@ -20,49 +20,6 @@ pub fn resolve_from_cwd(path: &Path) -> Result { } } -/// Attempts to convert a specifier to a file path. By default, uses the Url -/// crate's `to_file_path()` method, but falls back to try and resolve unix-style -/// paths on Windows. -pub fn specifier_to_file_path( - specifier: &ModuleSpecifier, -) -> Result { - let result = if specifier.scheme() != "file" { - Err(()) - } else if cfg!(windows) { - match specifier.to_file_path() { - Ok(path) => Ok(path), - Err(()) => { - // This might be a unix-style path which is used in the tests even on Windows. - // Attempt to see if we can convert it to a `PathBuf`. This code should be removed - // once/if https://github.com/servo/rust-url/issues/730 is implemented. - if specifier.scheme() == "file" - && specifier.host().is_none() - && specifier.port().is_none() - && specifier.path_segments().is_some() - { - let path_str = specifier.path(); - match String::from_utf8( - percent_encoding::percent_decode(path_str.as_bytes()).collect(), - ) { - Ok(path_str) => Ok(PathBuf::from(path_str)), - Err(_) => Err(()), - } - } else { - Err(()) - } - } - } - } else { - specifier.to_file_path() - }; - match result { - Ok(path) => Ok(path), - Err(()) => Err(uri_error(format!( - "Invalid file path.\n Specifier: {specifier}" - ))), - } -} - #[cfg(test)] mod tests { use super::*; @@ -114,22 +71,4 @@ mod tests { let absolute_expected = cwd.join(expected); assert_eq!(resolve_from_cwd(expected).unwrap(), absolute_expected); } - - #[test] - fn test_specifier_to_file_path() { - run_success_test("file:///", "/"); - run_success_test("file:///test", "/test"); - run_success_test("file:///dir/test/test.txt", "/dir/test/test.txt"); - run_success_test( - "file:///dir/test%20test/test.txt", - "/dir/test test/test.txt", - ); - - fn run_success_test(specifier: &str, expected_path: &str) { - let result = - specifier_to_file_path(&ModuleSpecifier::parse(specifier).unwrap()) - .unwrap(); - assert_eq!(result, PathBuf::from(expected_path)); - } - } } diff --git a/runtime/permissions/Cargo.toml b/runtime/permissions/Cargo.toml index b435748965..9821b61484 100644 --- a/runtime/permissions/Cargo.toml +++ b/runtime/permissions/Cargo.toml @@ -20,6 +20,7 @@ fqdn = "0.3.4" libc.workspace = true log.workspace = true once_cell.workspace = true +percent-encoding = { version = "2.3.1", features = [] } serde.workspace = true which.workspace = true diff --git a/runtime/permissions/lib.rs b/runtime/permissions/lib.rs index ad84b9fc3e..b5c870a077 100644 --- a/runtime/permissions/lib.rs +++ b/runtime/permissions/lib.rs @@ -1932,7 +1932,7 @@ impl Permissions { specifier: &ModuleSpecifier, ) -> Result<(), AnyError> { match specifier.scheme() { - "file" => match specifier.to_file_path() { + "file" => match specifier_to_file_path(specifier) { Ok(path) => self.read.check( &PathQueryDescriptor { requested: path.to_string_lossy().into_owned(), @@ -1952,6 +1952,52 @@ impl Permissions { } } +/// Attempts to convert a specifier to a file path. By default, uses the Url +/// crate's `to_file_path()` method, but falls back to try and resolve unix-style +/// paths on Windows. +pub fn specifier_to_file_path( + specifier: &ModuleSpecifier, +) -> Result { + let result = if specifier.scheme() != "file" { + Err(()) + } else if cfg!(windows) { + if specifier.host().is_some() { + Err(()) + } else { + match specifier.to_file_path() { + Ok(path) => Ok(path), + Err(()) => { + // This might be a unix-style path which is used in the tests even on Windows. + // Attempt to see if we can convert it to a `PathBuf`. This code should be removed + // once/if https://github.com/servo/rust-url/issues/730 is implemented. + if specifier.scheme() == "file" + && specifier.port().is_none() + && specifier.path_segments().is_some() + { + let path_str = specifier.path(); + match String::from_utf8( + percent_encoding::percent_decode(path_str.as_bytes()).collect(), + ) { + Ok(path_str) => Ok(PathBuf::from(path_str)), + Err(_) => Err(()), + } + } else { + Err(()) + } + } + } + } + } else { + specifier.to_file_path() + }; + match result { + Ok(path) => Ok(path), + Err(()) => Err(uri_error(format!( + "Invalid file path.\n Specifier: {specifier}" + ))), + } +} + /// Wrapper struct for `Permissions` that can be shared across threads. /// /// We need a way to have internal mutability for permissions as they might get @@ -3228,12 +3274,9 @@ mod tests { let mut test_cases = vec![]; - if cfg!(target_os = "windows") { - test_cases.push("file://"); - test_cases.push("file:///"); - } else { - test_cases.push("file://remotehost/"); - } + test_cases.push("file://dir"); + test_cases.push("file://asdf/"); + test_cases.push("file://remotehost/"); for url in test_cases { assert!(perms @@ -4222,4 +4265,38 @@ mod tests { ); } } + + #[test] + fn test_specifier_to_file_path() { + run_success_test("file:///", "/"); + run_success_test("file:///test", "/test"); + run_success_test("file:///dir/test/test.txt", "/dir/test/test.txt"); + run_success_test( + "file:///dir/test%20test/test.txt", + "/dir/test test/test.txt", + ); + + assert_no_panic_specifier_to_file_path("file:/"); + assert_no_panic_specifier_to_file_path("file://"); + assert_no_panic_specifier_to_file_path("file://asdf/"); + assert_no_panic_specifier_to_file_path("file://asdf/66666/a.ts"); + + fn run_success_test(specifier: &str, expected_path: &str) { + let result = + specifier_to_file_path(&ModuleSpecifier::parse(specifier).unwrap()) + .unwrap(); + assert_eq!(result, PathBuf::from(expected_path)); + } + fn assert_no_panic_specifier_to_file_path(specifier: &str) { + let result = + specifier_to_file_path(&ModuleSpecifier::parse(specifier).unwrap()); + match result { + Ok(_) => (), + Err(err) => assert_eq!( + err.to_string(), + format!("Invalid file path.\n Specifier: {specifier}") + ), + } + } + } } diff --git a/tests/integration/run_tests.rs b/tests/integration/run_tests.rs index b47af238b7..50f0f58f88 100644 --- a/tests/integration/run_tests.rs +++ b/tests/integration/run_tests.rs @@ -5125,3 +5125,27 @@ fn emit_failed_readonly_file_system() { .run(); output.assert_matches_text("[WILDCARD]Error saving emit data ([WILDLINE]main.ts)[WILDCARD]Skipped emit cache save of [WILDLINE]other.ts[WILDCARD]hi[WILDCARD]"); } + +#[cfg(windows)] +#[test] +fn handle_invalid_path_error() { + let deno_cmd = util::deno_cmd_with_deno_dir(&util::new_deno_dir()); + let output = deno_cmd.arg("run").arg("file://asdf").output().unwrap(); + assert!( + String::from_utf8_lossy(&output.stderr).contains("Invalid file path.") + ); + + let deno_cmd = util::deno_cmd_with_deno_dir(&util::new_deno_dir()); + let output = deno_cmd.arg("run").arg("/a/b").output().unwrap(); + assert!(String::from_utf8_lossy(&output.stderr).contains("Module not found")); + + let deno_cmd = util::deno_cmd_with_deno_dir(&util::new_deno_dir()); + let output = deno_cmd.arg("run").arg("//a/b").output().unwrap(); + assert!( + String::from_utf8_lossy(&output.stderr).contains("Invalid file path.") + ); + + let deno_cmd = util::deno_cmd_with_deno_dir(&util::new_deno_dir()); + let output = deno_cmd.arg("run").arg("///a/b").output().unwrap(); + assert!(String::from_utf8_lossy(&output.stderr).contains("Module not found")); +}