esm: do not interpret "main" as a URL

As documented, its value is a path. PR-URL: nodejs#55003 Reviewed-By: Benjamin Gruenbaum <[email protected]>
jasnell · Sep 27, 2024 · fb85279 · fb85279
1 parent fbc6fcb
commit fb85279
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 46 deletions.
diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js
@@ -22,6 +22,7 @@ const {
   StringPrototypeStartsWith,
   encodeURIComponent,
 } = primordials;
+const assert = require('internal/assert');
 const internalFS = require('internal/fs/utils');
 const { BuiltinModule } = require('internal/bootstrap/realm');
 const { realpathSync } = require('fs');
@@ -117,18 +118,17 @@ function emitInvalidSegmentDeprecation(target, request, match, pjsonUrl, interna
  * Emits a deprecation warning if the given URL is a module and
  * the package.json file does not define a "main" or "exports" field.
  * @param {URL} url - The URL of the module being resolved.
- * @param {URL} packageJSONUrl - The URL of the package.json file for the module.
+ * @param {string} path - The path of the module being resolved.
+ * @param {string} pkgPath - The path of the parent dir of the package.json file for the module.
  * @param {string | URL} [base] - The base URL for the module being resolved.
  * @param {string} [main] - The "main" field from the package.json file.
  */
-function emitLegacyIndexDeprecation(url, packageJSONUrl, base, main) {
+function emitLegacyIndexDeprecation(url, path, pkgPath, base, main) {
   if (process.noDeprecation) {
     return;
   }
   const format = defaultGetFormatWithoutErrors(url);
   if (format !== 'module') { return; }
-  const path = fileURLToPath(url);
-  const pkgPath = fileURLToPath(new URL('.', packageJSONUrl));
   const basePath = fileURLToPath(base);
   if (!main) {
     process.emitWarning(
@@ -196,20 +196,19 @@ const legacyMainResolveExtensionsIndexes = {
  * @returns {URL}
  */
 function legacyMainResolve(packageJSONUrl, packageConfig, base) {
-  const packageJsonUrlString = packageJSONUrl.href;
-
-  if (typeof packageJsonUrlString !== 'string') {
-    throw new ERR_INVALID_ARG_TYPE('packageJSONUrl', ['URL'], packageJSONUrl);
-  }
+  assert(isURL(packageJSONUrl));
+  const pkgPath = fileURLToPath(new URL('.', packageJSONUrl));
 
   const baseStringified = isURL(base) ? base.href : base;
 
-  const resolvedOption = FSLegacyMainResolve(packageJsonUrlString, packageConfig.main, baseStringified);
+  const resolvedOption = FSLegacyMainResolve(pkgPath, packageConfig.main, baseStringified);
 
-  const baseUrl = resolvedOption <= legacyMainResolveExtensionsIndexes.kResolvedByMainIndexNode ? `./${packageConfig.main}` : '';
-  const resolvedUrl = new URL(baseUrl + legacyMainResolveExtensions[resolvedOption], packageJSONUrl);
+  const maybeMain = resolvedOption <= legacyMainResolveExtensionsIndexes.kResolvedByMainIndexNode ?
+    packageConfig.main || './' : '';
+  const resolvedPath = resolve(pkgPath, maybeMain + legacyMainResolveExtensions[resolvedOption]);
+  const resolvedUrl = pathToFileURL(resolvedPath);
 
-  emitLegacyIndexDeprecation(resolvedUrl, packageJSONUrl, base, packageConfig.main);
+  emitLegacyIndexDeprecation(resolvedUrl, resolvedPath, pkgPath, base, packageConfig.main);
 
   return resolvedUrl;
 }
@@ -790,8 +789,8 @@ function packageResolve(specifier, base, conditions) {
   // ResolveSelf
   const packageConfig = packageJsonReader.getPackageScopeConfig(base);
   if (packageConfig.exists) {
-    const packageJSONUrl = pathToFileURL(packageConfig.pjsonPath);
     if (packageConfig.exports != null && packageConfig.name === packageName) {
+      const packageJSONUrl = pathToFileURL(packageConfig.pjsonPath);
       return packageExportsResolve(
         packageJSONUrl, packageSubpath, packageConfig, base, conditions);
     }

diff --git a/src/node_file.cc b/src/node_file.cc
@@ -3331,37 +3331,18 @@ void BindingData::LegacyMainResolve(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
   auto isolate = env->isolate();
 
-  Utf8Value utf8_package_json_url(isolate, args[0]);
-  auto package_json_url =
-      ada::parse<ada::url_aggregator>(utf8_package_json_url.ToStringView());
-
-  if (!package_json_url) {
-    THROW_ERR_INVALID_URL(isolate, "Invalid URL");
-    return;
-  }
+  auto utf8_package_path = Utf8Value(isolate, args[0]).ToString();
 
   std::string package_initial_file = "";
 
-  ada::result<ada::url_aggregator> file_path_url;
   std::optional<std::string> initial_file_path;
   std::string file_path;
 
   if (args.Length() >= 2 && args[1]->IsString()) {
     auto package_config_main = Utf8Value(isolate, args[1]).ToString();
 
-    file_path_url = ada::parse<ada::url_aggregator>(
-        std::string("./") + package_config_main, &package_json_url.value());
-
-    if (!file_path_url) {
-      THROW_ERR_INVALID_URL(isolate, "Invalid URL");
-      return;
-    }
-
-    initial_file_path = node::url::FileURLToPath(env, *file_path_url);
-    if (!initial_file_path.has_value()) {
-      return;
-    }
-
+    initial_file_path =
+        PathResolve(env, {utf8_package_path, package_config_main});
     FromNamespacedPath(&initial_file_path.value());
 
     package_initial_file = *initial_file_path;
@@ -3392,15 +3373,7 @@ void BindingData::LegacyMainResolve(const FunctionCallbackInfo<Value>& args) {
     }
   }
 
-  file_path_url =
-      ada::parse<ada::url_aggregator>("./index", &package_json_url.value());
-
-  if (!file_path_url) {
-    THROW_ERR_INVALID_URL(isolate, "Invalid URL");
-    return;
-  }
-
-  initial_file_path = node::url::FileURLToPath(env, *file_path_url);
+  initial_file_path = PathResolve(env, {utf8_package_path, "./index"});
   if (!initial_file_path.has_value()) {
     return;
   }

diff --git a/test/es-module/test-cjs-legacyMainResolve.js b/test/es-module/test-cjs-legacyMainResolve.js
@@ -82,7 +82,7 @@ describe('legacyMainResolve', () => {
           {},
           ''
         ),
-      { message: /instance of URL/, code: 'ERR_INVALID_ARG_TYPE' },
+      { code: 'ERR_INTERNAL_ASSERTION' },
     );
   });
 
@@ -166,4 +166,12 @@ describe('legacyMainResolve', () => {
       { message: /"base" argument must be/, code: 'ERR_INVALID_ARG_TYPE' },
     );
   });
+
+  it('should interpret main as a path, not a URL', () => {
+    const packageJsonUrl = fixtures.fileURL('/es-modules/legacy-main-resolver/package.json');
+    assert.deepStrictEqual(
+      legacyMainResolve(packageJsonUrl, { main: '../folder%25with percentage#/' }, packageJsonUrl),
+      fixtures.fileURL('/es-modules/folder%25with percentage#/index.js'),
+    );
+  });
 });