fix: do not panic running invalid file specifier (#25530)

Co-authored-by: Bedis Nbiba <bedisnbiba@gmail.com>

Cargo.lock

@@ -1924,6 +1924,7 @@ dependencies = [
  "libc",
  "log",
  "once_cell",
+ "percent-encoding",
  "serde",
  "which 4.4.2",
  "winapi",

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()])

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;

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;

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;

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;

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;

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);
 

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 {

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;

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;

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<PathBuf, AnyError> {
   if path.is_absolute() {
@@ -20,49 +20,6 @@ pub fn resolve_from_cwd(path: &Path) -> Result<PathBuf, AnyError> {
   }
 }
 
-/// 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<PathBuf, AnyError> {
-  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));
-    }
-  }
 }

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
 

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<PathBuf, AnyError> {
+  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://dir");
-      test_cases.push("file://");
+    test_cases.push("file://asdf/");
-      test_cases.push("file:///");
-    } else {
     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}")
+        ),
+      }
+    }
+  }
 }

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"));
+}