From f260c35652f376e6e1785aee1e09eabeef144fa1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vin=C3=ADcius=20Louren=C3=A7o?= Date: Fri, 1 Dec 2023 22:49:04 -0300 Subject: [PATCH 1/5] src,lib: reducing C++ calls of esm legacy main resolve Instead of many C++ calls, now we make only one C++ call to return a enum number that represents the selected state. Backport-PR-URL: https://github.com/nodejs/node/pull/48325 --- benchmark/esm/esm-legacyMainResolve.js | 53 ++++ lib/internal/modules/esm/resolve.js | 87 ++--- src/node_errors.h | 6 + src/node_file.cc | 297 +++++++++++++++++- src/node_file.h | 25 ++ .../test-cjs-legacyMainResolve-permission.js | 132 ++++++++ test/es-module/test-cjs-legacyMainResolve.js | 148 +++++++++ .../legacy-main-resolver/index-js/index.js | 1 + .../index-json/index.json | 1 + .../index-node/index.node | 0 10 files changed, 707 insertions(+), 43 deletions(-) create mode 100644 benchmark/esm/esm-legacyMainResolve.js create mode 100644 test/es-module/test-cjs-legacyMainResolve-permission.js create mode 100644 test/es-module/test-cjs-legacyMainResolve.js create mode 100644 test/fixtures/es-modules/legacy-main-resolver/index-js/index.js create mode 100644 test/fixtures/es-modules/legacy-main-resolver/index-json/index.json create mode 100644 test/fixtures/es-modules/legacy-main-resolver/index-node/index.node diff --git a/benchmark/esm/esm-legacyMainResolve.js b/benchmark/esm/esm-legacyMainResolve.js new file mode 100644 index 00000000000000..f5751e6840ff9a --- /dev/null +++ b/benchmark/esm/esm-legacyMainResolve.js @@ -0,0 +1,53 @@ +// Tests the impact on eager operations required for policies affecting +// general startup, does not test lazy operations +'use strict'; +const fs = require('node:fs'); +const path = require('node:path'); +const common = require('../common.js'); + +const tmpdir = require('../../test/common/tmpdir.js'); +const { pathToFileURL } = require('node:url'); + +const benchmarkDirectory = + path.resolve(tmpdir.path, 'benchmark-import-meta-resolve'); + +const configs = { + n: [1e4], + packageJsonUrl: [ + 'node_modules/test/package.json', + ], + packageConfigMain: ['', './index.js'], + resolvedFile: [ + 'node_modules/test/index.js', + 'node_modules/test/index.json', + 'node_modules/test/index.node', + 'node_modules/non-exist', + ], +}; + +const options = { + flags: ['--expose-internals'], +}; + +const bench = common.createBenchmark(main, configs, options); + +function main(conf) { + const { legacyMainResolve } = require('internal/modules/esm/resolve'); + tmpdir.refresh(); + + fs.mkdirSync(path.join(benchmarkDirectory, 'node_modules', 'test'), { recursive: true }); + fs.writeFileSync(path.join(benchmarkDirectory, conf.resolvedFile), '\n'); + + const packageJsonUrl = pathToFileURL(conf.packageJsonUrl); + const packageConfigMain = { main: conf.packageConfigMain }; + + bench.start(); + + for (let i = 0; i < conf.n; i++) { + try { + legacyMainResolve(packageJsonUrl, packageConfigMain, undefined); + } catch { /* empty */ } + } + + bench.end(conf.n); +} diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index 96dd20a86b076e..b44610a3b4aae4 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -40,6 +40,7 @@ const inputTypeFlag = getOptionValue('--input-type'); const { URL, pathToFileURL, fileURLToPath, isURL, toPathIfFileURL } = require('internal/url'); const { getCWDURL } = require('internal/util'); const { canParse: URLCanParse } = internalBinding('url'); +const { legacyMainResolve: FSLegacyMainResolve } = internalBinding('fs'); const { ERR_INPUT_TYPE_NOT_ALLOWED, ERR_INVALID_ARG_TYPE, @@ -161,6 +162,35 @@ function fileExists(url) { return internalModuleStat(toNamespacedPath(toPathIfFileURL(url))) === 0; } +const legacyMainResolveExtensions = [ + '', + '.js', + '.json', + '.node', + '/index.js', + '/index.json', + '/index.node', + './index.js', + './index.json', + './index.node', +]; + +const legacyMainResolveExtensionsIndexes = { + // 0-6: when packageConfig.main is defined + kResolvedByMain: 0, + kResolvedByMainJs: 1, + kResolvedByMainJson: 2, + kResolvedByMainNode: 3, + kResolvedByMainIndexJs: 4, + kResolvedByMainIndexJson: 5, + kResolvedByMainIndexNode: 6, + // 7-9: when packageConfig.main is NOT defined, + // or when the previous case didn't found the file + kResolvedByPackageAndJs: 7, + kResolvedByPackageAndJson: 8, + kResolvedByPackageAndNode: 9, +}; + /** * Legacy CommonJS main resolution: * 1. let M = pkg_url + (json main field) @@ -174,49 +204,22 @@ function fileExists(url) { * @returns {URL} */ function legacyMainResolve(packageJSONUrl, packageConfig, base) { - let guess; - if (packageConfig.main !== undefined) { - // Note: fs check redundances will be handled by Descriptor cache here. - if (fileExists(guess = new URL(`./${packageConfig.main}`, packageJSONUrl))) { - return guess; - } else if (fileExists(guess = new URL(`./${packageConfig.main}.js`, packageJSONUrl))) { - // Handled below. - } else if (fileExists(guess = new URL(`./${packageConfig.main}.json`, packageJSONUrl))) { - // Handled below. - } else if (fileExists(guess = new URL(`./${packageConfig.main}.node`, packageJSONUrl))) { - // Handled below. - } else if (fileExists(guess = new URL(`./${packageConfig.main}/index.js`, packageJSONUrl))) { - // Handled below. - } else if (fileExists(guess = new URL(`./${packageConfig.main}/index.json`, packageJSONUrl))) { - // Handled below. - } else if (fileExists(guess = new URL(`./${packageConfig.main}/index.node`, packageJSONUrl))) { - // Handled below. - } else { - guess = undefined; - } - if (guess) { - emitLegacyIndexDeprecation(guess, packageJSONUrl, base, - packageConfig.main); - return guess; - } - // Fallthrough. - } - if (fileExists(guess = new URL('./index.js', packageJSONUrl))) { - // Handled below. - } else if (fileExists(guess = new URL('./index.json', packageJSONUrl))) { - // Handled below. - } else if (fileExists(guess = new URL('./index.node', packageJSONUrl))) { - // Handled below. - } else { - guess = undefined; - } - if (guess) { - emitLegacyIndexDeprecation(guess, packageJSONUrl, base, packageConfig.main); - return guess; + const packageJsonUrlString = packageJSONUrl.href; + + if (typeof packageJsonUrlString !== 'string') { + throw new ERR_INVALID_ARG_TYPE('packageJSONUrl', ['URL'], packageJSONUrl); } - // Not found. - throw new ERR_MODULE_NOT_FOUND( - fileURLToPath(new URL('.', packageJSONUrl)), fileURLToPath(base)); + + const baseStringified = isURL(base) ? base.href : base; + + const resolvedOption = FSLegacyMainResolve(packageJsonUrlString, packageConfig.main, baseStringified); + + const baseUrl = resolvedOption <= legacyMainResolveExtensionsIndexes.kResolvedByMainIndexNode ? `./${packageConfig.main}` : ''; + const resolvedUrl = new URL(baseUrl + legacyMainResolveExtensions[resolvedOption], packageJSONUrl); + + emitLegacyIndexDeprecation(resolvedUrl, packageJSONUrl, base, packageConfig.main); + + return resolvedUrl; } /** diff --git a/src/node_errors.h b/src/node_errors.h index cc336536af0170..41f437a62ce328 100644 --- a/src/node_errors.h +++ b/src/node_errors.h @@ -67,17 +67,22 @@ void AppendExceptionLine(Environment* env, V(ERR_INVALID_ARG_VALUE, TypeError) \ V(ERR_OSSL_EVP_INVALID_DIGEST, Error) \ V(ERR_INVALID_ARG_TYPE, TypeError) \ + V(ERR_INVALID_FILE_URL_HOST, TypeError) \ + V(ERR_INVALID_FILE_URL_PATH, TypeError) \ V(ERR_INVALID_OBJECT_DEFINE_PROPERTY, TypeError) \ V(ERR_INVALID_MODULE, Error) \ V(ERR_INVALID_STATE, Error) \ V(ERR_INVALID_THIS, TypeError) \ V(ERR_INVALID_TRANSFER_OBJECT, TypeError) \ + V(ERR_INVALID_URL, TypeError) \ + V(ERR_INVALID_URL_SCHEME, TypeError) \ V(ERR_MEMORY_ALLOCATION_FAILED, Error) \ V(ERR_MESSAGE_TARGET_CONTEXT_UNAVAILABLE, Error) \ V(ERR_MISSING_ARGS, TypeError) \ V(ERR_MISSING_TRANSFERABLE_IN_TRANSFER_LIST, TypeError) \ V(ERR_MISSING_PASSPHRASE, TypeError) \ V(ERR_MISSING_PLATFORM_FOR_WORKER, Error) \ + V(ERR_MODULE_NOT_FOUND, Error) \ V(ERR_NON_CONTEXT_AWARE_DISABLED, Error) \ V(ERR_OUT_OF_RANGE, RangeError) \ V(ERR_SCRIPT_EXECUTION_INTERRUPTED, Error) \ @@ -160,6 +165,7 @@ ERRORS_WITH_CODE(V) V(ERR_INVALID_MODULE, "No such module") \ V(ERR_INVALID_THIS, "Value of \"this\" is the wrong type") \ V(ERR_INVALID_TRANSFER_OBJECT, "Found invalid object in transferList") \ + V(ERR_INVALID_URL_SCHEME, "The URL must be of scheme file:") \ V(ERR_MEMORY_ALLOCATION_FAILED, "Failed to allocate memory") \ V(ERR_OSSL_EVP_INVALID_DIGEST, "Invalid digest used") \ V(ERR_MESSAGE_TARGET_CONTEXT_UNAVAILABLE, \ diff --git a/src/node_file.cc b/src/node_file.cc index 7f627ac458492c..73c5c3368a3818 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -20,10 +20,13 @@ // USE OR OTHER DEALINGS IN THE SOFTWARE. #include "node_file.h" // NOLINT(build/include_inline) #include "node_file-inl.h" -#include "aliased_buffer-inl.h" +#include "ada.h" +#include "aliased_buffer.h" #include "memory_tracker-inl.h" #include "node_buffer.h" +#include "node_errors.h" #include "node_external_reference.h" +#include "node_metadata.h" #include "node_process-inl.h" #include "node_stat_watcher.h" #include "util-inl.h" @@ -2608,6 +2611,285 @@ static void Mkdtemp(const FunctionCallbackInfo& args) { } } +static bool FileURLToPath( + Environment* env, + const ada::url_aggregator& file_url, + /* The linter can't detect the assign for result_file_path + So we need to ignore since it suggest to put const */ + // NOLINTNEXTLINE(runtime/references) + std::string& result_file_path) { + if (file_url.type != ada::scheme::FILE) { + env->isolate()->ThrowException(ERR_INVALID_URL_SCHEME(env->isolate())); + + return false; + } + + std::string_view pathname = file_url.get_pathname(); +#ifdef _WIN32 + size_t first_percent = std::string::npos; + size_t pathname_size = pathname.size(); + std::string pathname_escaped_slash; + + for (size_t i = 0; i < pathname_size; i++) { + if (pathname[i] == '/') { + pathname_escaped_slash += '\\'; + } else + pathname_escaped_slash += pathname[i]; + + if (pathname[i] != '%') continue; + + if (first_percent == std::string::npos) { + first_percent = i; + } + + // just safe-guard against access the pathname + // outside the bounds + if ((i + 2) >= pathname_size) continue; + + char third = pathname[i + 2] | 0x20; + + bool is_slash = pathname[i + 1] == '2' && third == 102; + bool is_forward_slash = pathname[i + 1] == '5' && third == 99; + + if (!is_slash && !is_forward_slash) continue; + + env->isolate()->ThrowException(ERR_INVALID_FILE_URL_PATH( + env->isolate(), + "File URL path must not include encoded \\ or / characters")); + + return false; + } + + std::string_view hostname = file_url.get_hostname(); + std::string decoded_pathname = ada::unicode::percent_decode( + std::string_view(pathname_escaped_slash), first_percent); + + if (hostname.size() > 0) { + // If hostname is set, then we have a UNC path + // Pass the hostname through domainToUnicode just in case + // it is an IDN using punycode encoding. We do not need to worry + // about percent encoding because the URL parser will have + // already taken care of that for us. Note that this only + // causes IDNs with an appropriate `xn--` prefix to be decoded. + result_file_path = + "\\\\" + ada::unicode::to_unicode(hostname) + decoded_pathname; + + return true; + } + + char letter = decoded_pathname[1] | 0x20; + char sep = decoded_pathname[2]; + + // a..z A..Z + if (letter < 'a' || letter > 'z' || sep != ':') { + env->isolate()->ThrowException(ERR_INVALID_FILE_URL_PATH( + env->isolate(), "File URL path must be absolute")); + + return false; + } + + result_file_path = decoded_pathname.substr(1); + + return true; +#else // _WIN32 + std::string_view hostname = file_url.get_hostname(); + + if (hostname.size() > 0) { + std::string error_message = + std::string("File URL host must be \"localhost\" or empty on ") + + std::string(per_process::metadata.platform); + env->isolate()->ThrowException( + ERR_INVALID_FILE_URL_HOST(env->isolate(), error_message.c_str())); + + return false; + } + + size_t first_percent = std::string::npos; + for (size_t i = 0; (i + 2) < pathname.size(); i++) { + if (pathname[i] != '%') continue; + + if (first_percent == std::string::npos) { + first_percent = i; + } + + if (pathname[i + 1] == '2' && (pathname[i + 2] | 0x20) == 102) { + env->isolate()->ThrowException(ERR_INVALID_FILE_URL_PATH( + env->isolate(), + "File URL path must not include encoded / characters")); + + return false; + } + } + + result_file_path = ada::unicode::percent_decode(pathname, first_percent); + + return true; +#endif // _WIN32 +} + +BindingData::FilePathIsFileReturnType BindingData::FilePathIsFile( + Environment* env, const std::string& file_path) { + uv_fs_t req; + + int rc = uv_fs_stat(env->event_loop(), &req, file_path.c_str(), nullptr); + + if (rc == 0) { + const uv_stat_t* const s = static_cast(req.ptr); + rc = !!(s->st_mode & S_IFDIR); + } + + uv_fs_req_cleanup(&req); + + // rc is 0 if the path refers to a file + if (rc == 0) return BindingData::FilePathIsFileReturnType::kIsFile; + + return BindingData::FilePathIsFileReturnType::kIsNotFile; +} + +// the possible file extensions that should be tested +// 0-6: when packageConfig.main is defined +// 7-9: when packageConfig.main is NOT defined, +// or when the previous case didn't found the file +const std::array BindingData::legacy_main_extensions = { + "", + ".js", + ".json", + ".node", + "/index.js", + "/index.json", + "/index.node", + ".js", + ".json", + ".node"}; + +void BindingData::LegacyMainResolve(const FunctionCallbackInfo& args) { + CHECK_GE(args.Length(), 1); + CHECK(args[0]->IsString()); + + Environment* env = Environment::GetCurrent(args); + + Utf8Value utf8_package_json_url(env->isolate(), args[0].As()); + auto package_json_url = + ada::parse(utf8_package_json_url.ToStringView()); + + if (!package_json_url) { + env->isolate()->ThrowException( + ERR_INVALID_URL(env->isolate(), "Invalid URL")); + + return; + } + + ada::result file_path_url; + std::string initial_file_path; + std::string file_path; + + if (args.Length() >= 2 && !args[1]->IsNullOrUndefined() && + args[1]->IsString()) { + std::string package_config_main = + Utf8Value(env->isolate(), args[1].As()).ToString(); + + file_path_url = ada::parse( + std::string("./") + package_config_main, &package_json_url.value()); + + if (!file_path_url) { + env->isolate()->ThrowException( + ERR_INVALID_URL(env->isolate(), "Invalid URL")); + + return; + } + + if (!FileURLToPath(env, file_path_url.value(), initial_file_path)) return; + + FromNamespacedPath(&initial_file_path); + + for (int i = 0; i < BindingData::legacy_main_extensions_with_main_end; + i++) { + file_path = initial_file_path + BindingData::legacy_main_extensions[i]; + + switch (FilePathIsFile(env, file_path)) { + case BindingData::FilePathIsFileReturnType::kIsFile: + return args.GetReturnValue().Set(i); + case BindingData::FilePathIsFileReturnType::kIsNotFile: + continue; + case BindingData::FilePathIsFileReturnType:: + kThrowInsufficientPermissions: + // the default behavior when do not have permission is to return + // and exit the execution of the method as soon as possible + // the internal function will throw the exception + return; + default: + UNREACHABLE(); + } + } + } + + file_path_url = + ada::parse("./index", &package_json_url.value()); + + if (!file_path_url) { + env->isolate()->ThrowException( + ERR_INVALID_URL(env->isolate(), "Invalid URL")); + + return; + } + + if (!FileURLToPath(env, file_path_url.value(), initial_file_path)) return; + + FromNamespacedPath(&initial_file_path); + + for (int i = BindingData::legacy_main_extensions_with_main_end; + i < BindingData::legacy_main_extensions_package_fallback_end; + i++) { + file_path = initial_file_path + BindingData::legacy_main_extensions[i]; + + switch (FilePathIsFile(env, file_path)) { + case BindingData::FilePathIsFileReturnType::kIsFile: + return args.GetReturnValue().Set(i); + case BindingData::FilePathIsFileReturnType::kIsNotFile: + continue; + case BindingData::FilePathIsFileReturnType::kThrowInsufficientPermissions: + // the default behavior when do not have permission is to return + // and exit the execution of the method as soon as possible + // the internal function will throw the exception + return; + default: + UNREACHABLE(); + } + } + + std::string module_path; + std::string module_base; + + if (!FileURLToPath(env, package_json_url.value(), module_path)) return; + + if (args.Length() >= 3 && !args[2]->IsNullOrUndefined() && + args[2]->IsString()) { + Utf8Value utf8_base_path(env->isolate(), args[2].As()); + auto base_url = + ada::parse(utf8_base_path.ToStringView()); + + if (!base_url) { + env->isolate()->ThrowException( + ERR_INVALID_URL(env->isolate(), "Invalid URL")); + + return; + } + + if (!FileURLToPath(env, base_url.value(), module_base)) return; + } else { + std::string err_arg_message = + "The \"base\" argument must be of type string or an instance of URL."; + env->isolate()->ThrowException( + ERR_INVALID_ARG_TYPE(env->isolate(), err_arg_message.c_str())); + return; + } + + std::string err_module_message = + "Cannot find package '" + module_path + "' imported from " + module_base; + env->isolate()->ThrowException( + ERR_MODULE_NOT_FOUND(env->isolate(), err_module_message.c_str())); +} + void BindingData::MemoryInfo(MemoryTracker* tracker) const { tracker->TrackField("stats_field_array", stats_field_array); tracker->TrackField("stats_field_bigint_array", stats_field_bigint_array); @@ -2678,6 +2960,17 @@ InternalFieldInfoBase* BindingData::Serialize(int index) { return info; } +void BindingData::CreatePerIsolateProperties(Local context, + Local target) { + SetMethod( + context, target, "legacyMainResolve", BindingData::LegacyMainResolve); +} + +void BindingData::RegisterExternalReferences( + ExternalReferenceRegistry* registry) { + registry->Register(BindingData::LegacyMainResolve); +} + void Initialize(Local target, Local unused, Local context, @@ -2740,6 +3033,7 @@ void Initialize(Local target, .Check(); StatWatcher::Initialize(env, target); + BindingData::CreatePerIsolateProperties(context, target); // Create FunctionTemplate for FSReqCallback Local fst = NewFunctionTemplate(isolate, NewFSReqCallback); @@ -2802,6 +3096,7 @@ BindingData* FSReqBase::binding_data() { void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(Access); StatWatcher::RegisterExternalReferences(registry); + BindingData::RegisterExternalReferences(registry); registry->Register(Close); registry->Register(Open); diff --git a/src/node_file.h b/src/node_file.h index 472b45f4611042..6e7450c6566706 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -56,6 +56,13 @@ constexpr size_t kFsStatFsBufferLength = class BindingData : public SnapshotableObject { public: + + enum class FilePathIsFileReturnType { + kIsFile = 0, + kIsNotFile, + kThrowInsufficientPermissions + }; + explicit BindingData(Realm* realm, v8::Local wrap); AliasedFloat64Array stats_field_array; @@ -71,9 +78,27 @@ class BindingData : public SnapshotableObject { SERIALIZABLE_OBJECT_METHODS() SET_BINDING_ID(fs_binding_data) + static void LegacyMainResolve( + const v8::FunctionCallbackInfo& args); + + static void CreatePerIsolateProperties(v8::Local context, + v8::Local ctor); + static void RegisterExternalReferences(ExternalReferenceRegistry* registry); + void MemoryInfo(MemoryTracker* tracker) const override; SET_SELF_SIZE(BindingData) SET_MEMORY_INFO_NAME(BindingData) + + static FilePathIsFileReturnType FilePathIsFile(Environment* env, + const std::string& file_path); + + static const std::array legacy_main_extensions; + // define the final index of the algorithm resolution + // when packageConfig.main is defined. + static const uint8_t legacy_main_extensions_with_main_end = 7; + // define the final index of the algorithm resolution + // when packageConfig.main is NOT defined + static const uint8_t legacy_main_extensions_package_fallback_end = 10; }; // structure used to store state during a complex operation, e.g., mkdirp. diff --git a/test/es-module/test-cjs-legacyMainResolve-permission.js b/test/es-module/test-cjs-legacyMainResolve-permission.js new file mode 100644 index 00000000000000..cf54685736c099 --- /dev/null +++ b/test/es-module/test-cjs-legacyMainResolve-permission.js @@ -0,0 +1,132 @@ +'use strict'; + +// Flags: --expose-internals --experimental-permission --allow-fs-read=* --allow-child-process + +require('../common'); + +const { describe, it } = require('node:test'); +const assert = require('node:assert'); +const path = require('node:path'); +const { spawnSync } = require('node:child_process'); + +const fixtures = require('../common/fixtures.js'); + +function escapeWhenSepIsBackSlash(filePath) { + return path.sep === '\\' ? filePath.replace(/\\/g, '\\\\') : filePath; +} + +describe('legacyMainResolve', () => { + it('should ensure permission model when resolving by packageConfig.main', () => { + const fixtextureFolder = fixtures.path('/es-modules/legacy-main-resolver'); + + const paths = [ + ['./index-js/index.js', []], + ['./index-js/index', ['./index-js/index.js']], + ['./index-json/index', ['./index-json/index.js']], + ['./index-node/index', ['./index-node/index.js', './index-node/index.json']], + ['./index-js', []], + ['./index-json', ['./index-json/index.js']], + ['./index-node', ['./index-node/index.js', './index-node/index.json']], + ]; + + for (const [mainOrFolder, allowReads] of paths) { + const allowReadFilePaths = allowReads.map((filepath) => path.resolve(fixtextureFolder, filepath)); + const allowReadFiles = allowReads?.length > 0 ? ['--allow-fs-read', allowReadFilePaths.join(',')] : []; + const fixtextureFolderEscaped = escapeWhenSepIsBackSlash(fixtextureFolder); + + const { status, stderr } = spawnSync( + process.execPath, + [ + '--expose-internals', + '--experimental-permission', + ...allowReadFiles, + '-e', + ` + const { legacyMainResolve } = require('node:internal/modules/esm/resolve'); + const { pathToFileURL } = require('node:url'); + const path = require('node:path'); + const assert = require('node:assert'); + + const packageJsonUrl = pathToFileURL( + path.resolve( + '${fixtextureFolderEscaped}', + 'package.json' + ) + ); + + const packageConfig = { main: '${mainOrFolder}' }; + const base = path.resolve( + '${fixtextureFolderEscaped}' + ); + + assert.throws(() => legacyMainResolve(packageJsonUrl, packageConfig, base), { + code: 'ERR_ACCESS_DENIED', + resource: path.resolve( + '${fixtextureFolderEscaped}', + '${mainOrFolder}' + ) + }); + `, + ] + ); + + assert.strictEqual(status, 0, stderr.toString()); + } + }); + + it('should ensure permission model when resolving by packageJsonUrl', () => { + const fixtextureFolder = fixtures.path('/es-modules/legacy-main-resolver'); + + const paths = [ + ['./index-js', './index.js', []], + ['./index-json', './index.json', ['./index.js']], + ['./index-node', './index.node', ['./index.js', './index.json']], + ]; + + for (const [folder, expectedFile, allowReads] of paths) { + const allowReadFilePaths = allowReads.map((filepath) => path.resolve(fixtextureFolder, folder, filepath)); + const allowReadFiles = allowReads?.length > 0 ? ['--allow-fs-read', allowReadFilePaths.join(',')] : []; + const fixtextureFolderEscaped = escapeWhenSepIsBackSlash(fixtextureFolder); + + const { status, stderr } = spawnSync( + process.execPath, + [ + '--expose-internals', + '--experimental-permission', + ...allowReadFiles, + '-e', + ` + const { legacyMainResolve } = require('node:internal/modules/esm/resolve'); + const { pathToFileURL } = require('node:url'); + const path = require('node:path'); + const assert = require('node:assert'); + + const packageJsonUrl = pathToFileURL( + path.resolve( + '${fixtextureFolderEscaped}', + '${folder}', + 'package.json' + ) + ); + + const packageConfig = { main: undefined }; + const base = path.resolve( + '${fixtextureFolderEscaped}' + ); + + assert.throws(() => legacyMainResolve(packageJsonUrl, packageConfig, base), { + code: 'ERR_ACCESS_DENIED', + resource: path.resolve( + '${fixtextureFolderEscaped}', + '${folder}', + '${expectedFile}' + ) + }); + `, + ] + ); + + assert.strictEqual(status, 0, stderr.toString()); + } + }); +}); diff --git a/test/es-module/test-cjs-legacyMainResolve.js b/test/es-module/test-cjs-legacyMainResolve.js new file mode 100644 index 00000000000000..106aec933c2515 --- /dev/null +++ b/test/es-module/test-cjs-legacyMainResolve.js @@ -0,0 +1,148 @@ +'use strict'; + +// Flags: --expose-internals + +require('../common'); + +const { describe, it } = require('node:test'); +const path = require('node:path'); +const assert = require('node:assert'); +const { pathToFileURL } = require('node:url'); +const { legacyMainResolve } = require('node:internal/modules/esm/resolve'); + +const fixtures = require('../common/fixtures.js'); + +describe('legacyMainResolve', () => { + it('should resolve using packageConfig.main', () => { + const packageJsonUrl = pathToFileURL( + path.resolve( + fixtures.path('/es-modules/legacy-main-resolver'), + 'package.json' + ) + ); + + const paths = [ + ['./index-js/index.js', './index-js/index.js'], + ['./index-js/index', './index-js/index.js'], + ['./index-json/index', './index-json/index.json'], + ['./index-node/index', './index-node/index.node'], + ['./index-js', './index-js/index.js'], + ['./index-json', './index-json/index.json'], + ['./index-node', './index-node/index.node'], + ]; + + for (const [main, expected] of paths) { + const packageConfig = { main }; + const base = path.resolve( + fixtures.path('/es-modules/legacy-main-resolver') + ); + + assert.strictEqual( + legacyMainResolve(packageJsonUrl, packageConfig, base).href, + pathToFileURL(path.join(base, expected)).href + ); + } + }); + + it('should resolve using packageJsonUrl', () => { + const paths = [ + ['index-js', './index-js/index.js'], + ['index-json', './index-json/index.json'], + ['index-node', './index-node/index.node'], + ]; + + for (const [folder, expected] of paths) { + const packageJsonUrl = pathToFileURL( + path.resolve( + fixtures.path('/es-modules/legacy-main-resolver'), + folder, + 'package.json' + ) + ); + const packageConfig = { main: undefined }; + const base = path.resolve( + fixtures.path('/es-modules/legacy-main-resolver') + ); + + assert.strictEqual( + legacyMainResolve(packageJsonUrl, packageConfig, base).href, + pathToFileURL(path.join(base, expected)).href + ); + } + }); + + it('should throw when packageJsonUrl is not URL', () => { + assert.throws( + () => + legacyMainResolve( + path.resolve( + fixtures.path('/es-modules/legacy-main-resolver/index-node'), + 'package.json' + ), + {}, + '' + ), + { message: /instance of URL/ } + ); + }); + + it('should throw when packageConfigMain is invalid URL', () => { + assert.throws( + () => + legacyMainResolve( + pathToFileURL( + path.resolve( + // Is invalid because this path does not point to a file + fixtures.path('/es-modules/legacy-main-resolver/index-node') + ) + ), + { main: './invalid/index.js' }, + '' + ), + { message: /Invalid URL/ } + ); + }); + + it('should throw when packageJsonUrl is invalid URL', () => { + assert.throws( + () => + legacyMainResolve( + pathToFileURL( + path.resolve( + // Is invalid because this path does not point to a file + fixtures.path('/es-modules/legacy-main-resolver/index-node') + ) + ), + { main: undefined }, + '' + ), + { message: /Invalid URL/ } + ); + }); + + it('should throw when cannot resolve to a file', () => { + const packageJsonUrl = pathToFileURL( + path.resolve( + fixtures.path('/es-modules/legacy-main-resolver'), + 'package.json' + ) + ); + assert.throws( + () => legacyMainResolve(packageJsonUrl, { main: null }, packageJsonUrl), + { message: /Cannot find package/i } + ); + }); + + it('should throw when cannot resolve to a file (base not defined)', () => { + const packageJsonUrl = pathToFileURL( + path.resolve( + fixtures.path('/es-modules/legacy-main-resolver'), + 'package.json' + ) + ); + assert.throws( + () => legacyMainResolve(packageJsonUrl, { main: null }, undefined), + { message: /"base" argument must be/ } + ); + }); +}); diff --git a/test/fixtures/es-modules/legacy-main-resolver/index-js/index.js b/test/fixtures/es-modules/legacy-main-resolver/index-js/index.js new file mode 100644 index 00000000000000..f053ebf7976e37 --- /dev/null +++ b/test/fixtures/es-modules/legacy-main-resolver/index-js/index.js @@ -0,0 +1 @@ +module.exports = {}; diff --git a/test/fixtures/es-modules/legacy-main-resolver/index-json/index.json b/test/fixtures/es-modules/legacy-main-resolver/index-json/index.json new file mode 100644 index 00000000000000..9e26dfeeb6e641 --- /dev/null +++ b/test/fixtures/es-modules/legacy-main-resolver/index-json/index.json @@ -0,0 +1 @@ +{} \ No newline at end of file diff --git a/test/fixtures/es-modules/legacy-main-resolver/index-node/index.node b/test/fixtures/es-modules/legacy-main-resolver/index-node/index.node new file mode 100644 index 00000000000000..e69de29bb2d1d6 From edbc6b503957a4ce8c41db8f0120015a97e7082f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vin=C3=ADcius=20Louren=C3=A7o?= Date: Fri, 1 Dec 2023 22:49:47 -0300 Subject: [PATCH 2/5] fixup! src,lib: reducing C++ calls of esm legacy main resolve Backport-PR-URL: https://github.com/nodejs/node/pull/48325 --- lib/internal/modules/esm/resolve.js | 2 ++ src/node_file.cc | 3 ++- src/node_file.h | 1 - 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index b44610a3b4aae4..d67501aade45e8 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -53,6 +53,7 @@ const { ERR_PACKAGE_PATH_NOT_EXPORTED, ERR_UNSUPPORTED_DIR_IMPORT, ERR_NETWORK_IMPORT_DISALLOWED, + ERR_INVALID_ARG_TYPE, } = require('internal/errors').codes; const { Module: CJSModule } = require('internal/modules/cjs/loader'); @@ -1249,6 +1250,7 @@ module.exports = { packageExportsResolve, packageImportsResolve, throwIfInvalidParentURL, + legacyMainResolve, }; // cycle diff --git a/src/node_file.cc b/src/node_file.cc index 73c5c3368a3818..a9c8bbcb070a83 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -2633,8 +2633,9 @@ static bool FileURLToPath( for (size_t i = 0; i < pathname_size; i++) { if (pathname[i] == '/') { pathname_escaped_slash += '\\'; - } else + } else { pathname_escaped_slash += pathname[i]; + } if (pathname[i] != '%') continue; diff --git a/src/node_file.h b/src/node_file.h index 6e7450c6566706..151d85b967d538 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -56,7 +56,6 @@ constexpr size_t kFsStatFsBufferLength = class BindingData : public SnapshotableObject { public: - enum class FilePathIsFileReturnType { kIsFile = 0, kIsNotFile, From 2c8a1822de4f412728d9119b1a7503f8bcea6602 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vin=C3=ADcius=20Louren=C3=A7o?= Date: Sun, 17 Sep 2023 22:27:08 -0300 Subject: [PATCH 3/5] fixup! src,lib: reducing C++ calls of esm legacy main resolve --- .../test-cjs-legacyMainResolve-permission.js | 132 ------------------ 1 file changed, 132 deletions(-) delete mode 100644 test/es-module/test-cjs-legacyMainResolve-permission.js diff --git a/test/es-module/test-cjs-legacyMainResolve-permission.js b/test/es-module/test-cjs-legacyMainResolve-permission.js deleted file mode 100644 index cf54685736c099..00000000000000 --- a/test/es-module/test-cjs-legacyMainResolve-permission.js +++ /dev/null @@ -1,132 +0,0 @@ -'use strict'; - -// Flags: --expose-internals --experimental-permission --allow-fs-read=* --allow-child-process - -require('../common'); - -const { describe, it } = require('node:test'); -const assert = require('node:assert'); -const path = require('node:path'); -const { spawnSync } = require('node:child_process'); - -const fixtures = require('../common/fixtures.js'); - -function escapeWhenSepIsBackSlash(filePath) { - return path.sep === '\\' ? filePath.replace(/\\/g, '\\\\') : filePath; -} - -describe('legacyMainResolve', () => { - it('should ensure permission model when resolving by packageConfig.main', () => { - const fixtextureFolder = fixtures.path('/es-modules/legacy-main-resolver'); - - const paths = [ - ['./index-js/index.js', []], - ['./index-js/index', ['./index-js/index.js']], - ['./index-json/index', ['./index-json/index.js']], - ['./index-node/index', ['./index-node/index.js', './index-node/index.json']], - ['./index-js', []], - ['./index-json', ['./index-json/index.js']], - ['./index-node', ['./index-node/index.js', './index-node/index.json']], - ]; - - for (const [mainOrFolder, allowReads] of paths) { - const allowReadFilePaths = allowReads.map((filepath) => path.resolve(fixtextureFolder, filepath)); - const allowReadFiles = allowReads?.length > 0 ? ['--allow-fs-read', allowReadFilePaths.join(',')] : []; - const fixtextureFolderEscaped = escapeWhenSepIsBackSlash(fixtextureFolder); - - const { status, stderr } = spawnSync( - process.execPath, - [ - '--expose-internals', - '--experimental-permission', - ...allowReadFiles, - '-e', - ` - const { legacyMainResolve } = require('node:internal/modules/esm/resolve'); - const { pathToFileURL } = require('node:url'); - const path = require('node:path'); - const assert = require('node:assert'); - - const packageJsonUrl = pathToFileURL( - path.resolve( - '${fixtextureFolderEscaped}', - 'package.json' - ) - ); - - const packageConfig = { main: '${mainOrFolder}' }; - const base = path.resolve( - '${fixtextureFolderEscaped}' - ); - - assert.throws(() => legacyMainResolve(packageJsonUrl, packageConfig, base), { - code: 'ERR_ACCESS_DENIED', - resource: path.resolve( - '${fixtextureFolderEscaped}', - '${mainOrFolder}' - ) - }); - `, - ] - ); - - assert.strictEqual(status, 0, stderr.toString()); - } - }); - - it('should ensure permission model when resolving by packageJsonUrl', () => { - const fixtextureFolder = fixtures.path('/es-modules/legacy-main-resolver'); - - const paths = [ - ['./index-js', './index.js', []], - ['./index-json', './index.json', ['./index.js']], - ['./index-node', './index.node', ['./index.js', './index.json']], - ]; - - for (const [folder, expectedFile, allowReads] of paths) { - const allowReadFilePaths = allowReads.map((filepath) => path.resolve(fixtextureFolder, folder, filepath)); - const allowReadFiles = allowReads?.length > 0 ? ['--allow-fs-read', allowReadFilePaths.join(',')] : []; - const fixtextureFolderEscaped = escapeWhenSepIsBackSlash(fixtextureFolder); - - const { status, stderr } = spawnSync( - process.execPath, - [ - '--expose-internals', - '--experimental-permission', - ...allowReadFiles, - '-e', - ` - const { legacyMainResolve } = require('node:internal/modules/esm/resolve'); - const { pathToFileURL } = require('node:url'); - const path = require('node:path'); - const assert = require('node:assert'); - - const packageJsonUrl = pathToFileURL( - path.resolve( - '${fixtextureFolderEscaped}', - '${folder}', - 'package.json' - ) - ); - - const packageConfig = { main: undefined }; - const base = path.resolve( - '${fixtextureFolderEscaped}' - ); - - assert.throws(() => legacyMainResolve(packageJsonUrl, packageConfig, base), { - code: 'ERR_ACCESS_DENIED', - resource: path.resolve( - '${fixtextureFolderEscaped}', - '${folder}', - '${expectedFile}' - ) - }); - `, - ] - ); - - assert.strictEqual(status, 0, stderr.toString()); - } - }); -}); From a7ddae80935fa48a80f502c72cbf86f2a4fc407e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vin=C3=ADcius=20Louren=C3=A7o?= Date: Fri, 1 Dec 2023 23:12:08 -0300 Subject: [PATCH 4/5] fixup! src,lib: reducing C++ calls of esm legacy main resolve --- lib/internal/modules/esm/resolve.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index d67501aade45e8..65b11a9ed34c67 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -53,7 +53,6 @@ const { ERR_PACKAGE_PATH_NOT_EXPORTED, ERR_UNSUPPORTED_DIR_IMPORT, ERR_NETWORK_IMPORT_DISALLOWED, - ERR_INVALID_ARG_TYPE, } = require('internal/errors').codes; const { Module: CJSModule } = require('internal/modules/cjs/loader'); From a1575a0efe9d5d949bfce5b3d02a5675661fadf2 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Tue, 11 Jul 2023 18:49:06 +0200 Subject: [PATCH 5/5] esm: fix emit deprecation on legacy main resolve PR-URL: https://github.com/nodejs/node/pull/48664 Refs: https://github.com/nodejs/node/pull/48325 Reviewed-By: Yagiz Nizipli Reviewed-By: Rich Trott Reviewed-By: Rafael Gonzaga Backport-PR-URL: https://github.com/nodejs/node/pull/48664