fix(linter/plugins): handle non-UTF8 file paths (#16157)

Fix an edge case bug.

Gracefully handle plugins or files being linted that have file paths which are not valid UTF-8. Previously these would cause panics.

* Load plugin: Convert plugin path to a `file://...` URL on Rust side, instead of on JS side. File URLs can encode any path via URL escaping.
* Lint file: Convert path to `String` with `to_string_lossy`.

Avoiding calling `pathToFileURL` on JS side should also be more performant, but that's a very marginal benefit, as loading plugins is a one-time cost in each lint run.

This adds a dependency on `url` crate. It's a very popular and battle-tested crate. 430m downloads. https://crates.io/crates/url

I could not work out how to add tests for this, as it seems it's not possible to create a file on Mac whose path is not valid UTF-8. It *is* possible on Linux and Windows, but no way to check such a file into Git without it malfunctioning on Mac. We could create test files when tests run, dependent on platform, but I didn't want to create complication in the test infra just for this edge case. It *should* work!

Author: overlookmotel

Cargo.lock

@@ -230,6 +230,7 @@ toml = { version = "0.9.8" }
 tower-lsp-server = "0.22.1" # LSP server framework
 tracing-subscriber = "0.3.20" # Tracing implementation
 ureq = { version = "3.1.4", default-features = false } # HTTP client
+url = { version = "2.5.7" } # URL parsing
 walkdir = "2.5.0" # Directory traversal
 
 [workspace.metadata.cargo-shear]

Cargo.toml

@@ -1,5 +1,3 @@
-import { pathToFileURL } from "node:url";
-
 import { createContext } from "./context.js";
 import { getErrorMessage } from "../utils/utils.js";
 
@@ -77,7 +75,7 @@ interface CreateOnceRuleDetails extends RuleDetailsBase {
 }
 
 // Absolute paths of plugins which have been loaded
-const registeredPluginPaths = new Set<string>();
+const registeredPluginUrls = new Set<string>();
 
 // Rule objects for loaded rules.
 // Indexed by `ruleId`, which is passed to `lintFile`.
@@ -101,13 +99,13 @@ interface PluginDetails {
  *
  * Main logic is in separate function `loadPluginImpl`, because V8 cannot optimize functions containing try/catch.
  *
- * @param path - Absolute path of plugin file
+ * @param url - Absolute path of plugin file as a `file://...` URL
  * @param packageName - Optional package name from `package.json` (fallback if `plugin.meta.name` is not defined)
  * @returns Plugin details or error serialized to JSON string
  */
-export async function loadPlugin(path: string, packageName: string | null): Promise<string> {
+export async function loadPlugin(url: string, packageName: string | null): Promise<string> {
   try {
-    const res = await loadPluginImpl(path, packageName);
+    const res = await loadPluginImpl(url, packageName);
     return JSON.stringify({ Success: res });
   } catch (err) {
     return JSON.stringify({ Failure: getErrorMessage(err) });
@@ -117,7 +115,7 @@ export async function loadPlugin(path: string, packageName: string | null): Prom
 /**
  * Load a plugin.
  *
- * @param path - Absolute path of plugin file
+ * @param url - Absolute path of plugin file as a `file://...` URL
  * @param packageName - Optional package name from `package.json` (fallback if `plugin.meta.name` is not defined)
  * @returns - Plugin details
  * @throws {Error} If plugin has already been registered
@@ -126,13 +124,13 @@ export async function loadPlugin(path: string, packageName: string | null): Prom
  * @throws {TypeError} if `plugin.meta.name` is not a string
  * @throws {*} If plugin throws an error during import
  */
-async function loadPluginImpl(path: string, packageName: string | null): Promise<PluginDetails> {
+async function loadPluginImpl(url: string, packageName: string | null): Promise<PluginDetails> {
   if (DEBUG) {
-    if (registeredPluginPaths.has(path)) throw new Error("This plugin has already been registered");
-    registeredPluginPaths.add(path);
+    if (registeredPluginUrls.has(url)) throw new Error("This plugin has already been registered");
+    registeredPluginUrls.add(url);
   }
 
-  const { default: plugin } = (await import(pathToFileURL(path).href)) as { default: Plugin };
+  const { default: plugin } = (await import(url)) as { default: Plugin };
 
   // TODO: Use a validation library to assert the shape of the plugin, and of rules
 

apps/oxlint/src-js/plugins/load.ts

@@ -36,12 +36,12 @@ pub fn create_external_linter(
 ///
 /// The returned function will panic if called outside of a Tokio runtime.
 fn wrap_load_plugin(cb: JsLoadPluginCb) -> ExternalLinterLoadPluginCb {
-    Box::new(move |plugin_path, package_name| {
+    Box::new(move |plugin_url, package_name| {
         let cb = &cb;
         tokio::task::block_in_place(|| {
             tokio::runtime::Handle::current().block_on(async move {
                 let result = cb
-                    .call_async(FnArgs::from((plugin_path, package_name)))
+                    .call_async(FnArgs::from((plugin_url, package_name)))
                     .await?
                     .into_future()
                     .await?;

apps/oxlint/src/js_plugins/external_linter.rs

@@ -69,6 +69,7 @@ serde = { workspace = true, features = ["derive"] }
 serde_json = { workspace = true, features = ["preserve_order"] } # preserve_order: print config with ordered keys.
 simdutf8 = { workspace = true }
 smallvec = { workspace = true }
+url = { workspace = true }
 
 [dev-dependencies]
 insta = { workspace = true }

crates/oxc_linter/Cargo.toml

@@ -6,6 +6,7 @@ use std::{
 use itertools::Itertools;
 use oxc_resolver::{ResolveOptions, Resolver};
 use rustc_hash::{FxHashMap, FxHashSet};
+use url::Url;
 
 use oxc_span::{CompactStr, format_compact_str};
 
@@ -530,25 +531,25 @@ impl ConfigStoreBuilder {
                 error: e.to_string(),
             }
         })?;
-        // TODO: We should support paths which are not valid UTF-8. How?
-        let plugin_path = resolved.full_path().to_str().unwrap().to_string();
+        let plugin_path = resolved.full_path();
 
         if external_plugin_store.is_plugin_registered(&plugin_path) {
             return Ok(());
         }
 
-        // Extract package name from package.json if available
+        // Extract package name from `package.json` if available
         let package_name = resolved.package_json().and_then(|pkg| pkg.name().map(String::from));
 
-        let result = {
-            let plugin_path = plugin_path.clone();
-            (external_linter.load_plugin)(plugin_path, package_name).map_err(|e| {
-                ConfigBuilderError::PluginLoadFailed {
-                    plugin_specifier: plugin_specifier.to_string(),
-                    error: e.to_string(),
-                }
-            })
-        }?;
+        // Convert path to a `file://...` URL, as required by `import(...)` on JS side.
+        // Note: `unwrap()` here is infallible as `plugin_path` is an absolute path.
+        let plugin_url = Url::from_file_path(&plugin_path).unwrap().as_str().to_string();
+
+        let result = (external_linter.load_plugin)(plugin_url, package_name).map_err(|e| {
+            ConfigBuilderError::PluginLoadFailed {
+                plugin_specifier: plugin_specifier.to_string(),
+                error: e.to_string(),
+            }
+        })?;
 
         match result {
             PluginLoadResult::Success { name, offset, rule_names } => {

crates/oxc_linter/src/config/config_builder.rs

@@ -1,4 +1,7 @@
-use std::fmt;
+use std::{
+    fmt,
+    path::{Path, PathBuf},
+};
 
 use rustc_hash::{FxHashMap, FxHashSet};
 
@@ -14,7 +17,7 @@ define_index_type! {
 
 #[derive(Debug)]
 pub struct ExternalPluginStore {
-    registered_plugin_paths: FxHashSet<String>,
+    registered_plugin_paths: FxHashSet<PathBuf>,
 
     plugins: IndexVec<ExternalPluginId, ExternalPlugin>,
     plugin_names: FxHashMap<String, ExternalPluginId>,
@@ -51,7 +54,7 @@ impl ExternalPluginStore {
         self.plugins.is_empty()
     }
 
-    pub fn is_plugin_registered(&self, plugin_path: &str) -> bool {
+    pub fn is_plugin_registered(&self, plugin_path: &Path) -> bool {
         self.registered_plugin_paths.contains(plugin_path)
     }
 
@@ -63,7 +66,7 @@ impl ExternalPluginStore {
     /// - `offset` does not equal the number of registered rules.
     pub fn register_plugin(
         &mut self,
-        plugin_path: String,
+        plugin_path: PathBuf,
         plugin_name: String,
         offset: usize,
         rule_names: Vec<String>,

crates/oxc_linter/src/external_plugin_store.rs

@@ -462,7 +462,7 @@ impl Linter {
         };
 
         let result = (external_linter.lint_file)(
-            path.to_str().unwrap().to_string(),
+            path.to_string_lossy().into_owned(),
             external_rules.iter().map(|(rule_id, _)| rule_id.raw()).collect(),
             settings_json,
             allocator,