From 3e51ee516ea167ceb0c70367e8b08afe178e7a7b Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Sat, 22 Feb 2025 17:06:04 +0100 Subject: [PATCH] fix: rework and improve `legacyMainResolve` patch (#45754) fix: rework and improve legacyMainResolve patch --- patches/node/.patches | 2 +- ...g_fileexists_fn_to_legacymainresolve.patch | 129 +++++++++++++ ...g_c_calls_of_esm_legacy_main_resolve.patch | 170 ------------------ ...ted_fields_of_fastapicallbackoptions.patch | 2 +- script/node-disabled-tests.json | 1 - 5 files changed, 131 insertions(+), 173 deletions(-) create mode 100644 patches/node/fix_allow_passing_fileexists_fn_to_legacymainresolve.patch delete mode 100644 patches/node/fix_revert_src_lb_reducing_c_calls_of_esm_legacy_main_resolve.patch diff --git a/patches/node/.patches b/patches/node/.patches index fcbf9ae260cd..161e3e08ef9c 100644 --- a/patches/node/.patches +++ b/patches/node/.patches @@ -18,7 +18,7 @@ test_formally_mark_some_tests_as_flaky.patch fix_do_not_resolve_electron_entrypoints.patch ci_ensure_node_tests_set_electron_run_as_node.patch fix_assert_module_in_the_renderer_process.patch -fix_revert_src_lb_reducing_c_calls_of_esm_legacy_main_resolve.patch +fix_allow_passing_fileexists_fn_to_legacymainresolve.patch fix_remove_deprecated_errno_constants.patch build_enable_perfetto.patch fix_add_source_location_for_v8_task_runner.patch diff --git a/patches/node/fix_allow_passing_fileexists_fn_to_legacymainresolve.patch b/patches/node/fix_allow_passing_fileexists_fn_to_legacymainresolve.patch new file mode 100644 index 000000000000..b497527571d4 --- /dev/null +++ b/patches/node/fix_allow_passing_fileexists_fn_to_legacymainresolve.patch @@ -0,0 +1,129 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Shelley Vohr +Date: Sat, 17 Feb 2024 22:17:13 -0800 +Subject: fix: allow passing fileExists fn to legacyMainResolve + +This switch to native legacyMainResolve doesn't take asar into account, and can +cause errors when a project using ESM and asar tries to load a dependency which +uses commonJS. + +We can fix this by allowing the C++ implementation of legacyMainResolve to use +a fileExists function that does take Asar into account. + +diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js +index 2879e5cf541fb4d226cfd7cc0fe367ca448fb926..03082f0ec4f91382933eec48e77331cdf6f04943 100644 +--- a/lib/internal/modules/esm/resolve.js ++++ b/lib/internal/modules/esm/resolve.js +@@ -28,14 +28,13 @@ const { BuiltinModule } = require('internal/bootstrap/realm'); + const fs = require('fs'); + const { getOptionValue } = require('internal/options'); + // Do not eagerly grab .manifest, it may be in TDZ +-const { sep, posix: { relative: relativePosixPath }, resolve } = require('path'); ++const { sep, posix: { relative: relativePosixPath }, toNamespacedPath, resolve } = require('path'); + const preserveSymlinks = getOptionValue('--preserve-symlinks'); + const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main'); + const inputTypeFlag = getOptionValue('--input-type'); +-const { URL, pathToFileURL, fileURLToPath, isURL, URLParse } = require('internal/url'); ++const { URL, pathToFileURL, fileURLToPath, isURL, URLParse, toPathIfFileURL } = require('internal/url'); + const { getCWDURL, setOwnProperty } = require('internal/util'); + const { canParse: URLCanParse } = internalBinding('url'); +-const { legacyMainResolve: FSLegacyMainResolve } = internalBinding('fs'); + const { + ERR_INPUT_TYPE_NOT_ALLOWED, + ERR_INVALID_ARG_TYPE, +@@ -183,6 +182,11 @@ const legacyMainResolveExtensionsIndexes = { + kResolvedByPackageAndNode: 9, + }; + ++function fileExists(url) { ++ const namespaced = toNamespacedPath(toPathIfFileURL(url)); ++ return internalFsBinding.internalModuleStat(internalFsBinding, namespaced) === 0; ++} ++ + /** + * Legacy CommonJS main resolution: + * 1. let M = pkg_url + (json main field) +@@ -201,7 +205,7 @@ function legacyMainResolve(packageJSONUrl, packageConfig, base) { + + const baseStringified = isURL(base) ? base.href : base; + +- const resolvedOption = FSLegacyMainResolve(pkgPath, packageConfig.main, baseStringified); ++ const resolvedOption = internalFsBinding.legacyMainResolve(pkgPath, packageConfig.main, baseStringified, fileExists); + + const maybeMain = resolvedOption <= legacyMainResolveExtensionsIndexes.kResolvedByMainIndexNode ? + packageConfig.main || './' : ''; +diff --git a/src/node_file.cc b/src/node_file.cc +index 1d22e19f16d5ad82466b0724971b2e4a685682f7..9619d10710ffbbdc73fa7d59d1b797c8d0b3a956 100644 +--- a/src/node_file.cc ++++ b/src/node_file.cc +@@ -3220,13 +3220,25 @@ static void CpSyncCheckPaths(const FunctionCallbackInfo& args) { + } + + BindingData::FilePathIsFileReturnType BindingData::FilePathIsFile( +- Environment* env, const std::string& file_path) { ++ Environment* env, const std::string& file_path, v8::Local is_file_function) { + THROW_IF_INSUFFICIENT_PERMISSIONS( + env, + permission::PermissionScope::kFileSystemRead, + file_path, + BindingData::FilePathIsFileReturnType::kThrowInsufficientPermissions); + ++ if (!is_file_function.IsEmpty()) { ++ v8::Local argv[] = {v8::String::NewFromUtf8( ++ env->isolate(), file_path.c_str(), v8::NewStringType::kNormal) ++ .ToLocalChecked()}; ++ MaybeLocal maybe_is_file = is_file_function->Call(env->context(), v8::Undefined(env->isolate()), 1, argv); ++ if (maybe_is_file.IsEmpty()) { ++ bool is_file = maybe_is_file.ToLocalChecked()->BooleanValue(env->isolate()); ++ return is_file ? BindingData::FilePathIsFileReturnType::kIsFile ++ : BindingData::FilePathIsFileReturnType::kIsNotFile; ++ } ++ } ++ + uv_fs_t req; + + int rc = uv_fs_stat(env->event_loop(), &req, file_path.c_str(), nullptr); +@@ -3284,6 +3296,11 @@ void BindingData::LegacyMainResolve(const FunctionCallbackInfo& args) { + std::optional initial_file_path; + std::string file_path; + ++ v8::Local is_file_function; ++ if (args.Length() >= 3 && args[3]->IsFunction()) { ++ is_file_function = args[3].As(); ++ } ++ + if (args.Length() >= 2 && args[1]->IsString()) { + auto package_config_main = Utf8Value(isolate, args[1]).ToString(); + +@@ -3304,7 +3321,7 @@ void BindingData::LegacyMainResolve(const FunctionCallbackInfo& args) { + BufferValue buff_file_path(isolate, local_file_path); + ToNamespacedPath(env, &buff_file_path); + +- switch (FilePathIsFile(env, buff_file_path.ToString())) { ++ switch (FilePathIsFile(env, buff_file_path.ToString(), is_file_function)) { + case BindingData::FilePathIsFileReturnType::kIsFile: + return args.GetReturnValue().Set(i); + case BindingData::FilePathIsFileReturnType::kIsNotFile: +@@ -3341,7 +3358,7 @@ void BindingData::LegacyMainResolve(const FunctionCallbackInfo& args) { + BufferValue buff_file_path(isolate, local_file_path); + ToNamespacedPath(env, &buff_file_path); + +- switch (FilePathIsFile(env, buff_file_path.ToString())) { ++ switch (FilePathIsFile(env, buff_file_path.ToString(), is_file_function)) { + case BindingData::FilePathIsFileReturnType::kIsFile: + return args.GetReturnValue().Set(i); + case BindingData::FilePathIsFileReturnType::kIsNotFile: +diff --git a/src/node_file.h b/src/node_file.h +index bdad1ae25f4892cbbfd8cc30c4d8b4a6f600edbc..099488319f53bc7718313d6e30df2237cad6771d 100644 +--- a/src/node_file.h ++++ b/src/node_file.h +@@ -101,7 +101,8 @@ class BindingData : public SnapshotableObject { + InternalFieldInfo* internal_field_info_ = nullptr; + + static FilePathIsFileReturnType FilePathIsFile(Environment* env, +- const std::string& file_path); ++ const std::string& file_path, ++ v8::Local is_file_function = v8::Local()); + }; + + // structure used to store state during a complex operation, e.g., mkdirp. diff --git a/patches/node/fix_revert_src_lb_reducing_c_calls_of_esm_legacy_main_resolve.patch b/patches/node/fix_revert_src_lb_reducing_c_calls_of_esm_legacy_main_resolve.patch deleted file mode 100644 index e6918b8a9e03..000000000000 --- a/patches/node/fix_revert_src_lb_reducing_c_calls_of_esm_legacy_main_resolve.patch +++ /dev/null @@ -1,170 +0,0 @@ -From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 -From: VerteDinde -Date: Sat, 17 Feb 2024 22:17:13 -0800 -Subject: fix: revert "src,lb: reducing C++ calls of esm legacy main resolve" - -This switch to native legacyMainResolve doesn't take asar into account, and can -cause errors when a project using ESM and asar tries to load a dependency which -uses commonJS. This will need to be fixed forward, but revert for Electron 29's -stable release to avoid potentially breaking apps with a riskier fix. - -This patch can be removed when node's -native implementation has been patched -to recognize asar files. - -This reverts commit 9cf2e1f55b8446a7cde23699d00a3be73aa0c8f1. - -diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js -index 2879e5cf541fb4d226cfd7cc0fe367ca448fb926..19df6fab3e118ac237f930b1e5d41c02c9050fa2 100644 ---- a/lib/internal/modules/esm/resolve.js -+++ b/lib/internal/modules/esm/resolve.js -@@ -28,15 +28,15 @@ const { BuiltinModule } = require('internal/bootstrap/realm'); - const fs = require('fs'); - const { getOptionValue } = require('internal/options'); - // Do not eagerly grab .manifest, it may be in TDZ --const { sep, posix: { relative: relativePosixPath }, resolve } = require('path'); -+const { sep, posix: { relative: relativePosixPath }, toNamespacedPath, resolve } = require('path'); - const preserveSymlinks = getOptionValue('--preserve-symlinks'); - const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main'); - const inputTypeFlag = getOptionValue('--input-type'); --const { URL, pathToFileURL, fileURLToPath, isURL, URLParse } = require('internal/url'); -+const { URL, pathToFileURL, fileURLToPath, isURL, URLParse, toPathIfFileURL } = require('internal/url'); - const { getCWDURL, setOwnProperty } = require('internal/util'); - const { canParse: URLCanParse } = internalBinding('url'); --const { legacyMainResolve: FSLegacyMainResolve } = internalBinding('fs'); - const { -+ ERR_ACCESS_DENIED, - ERR_INPUT_TYPE_NOT_ALLOWED, - ERR_INVALID_ARG_TYPE, - ERR_INVALID_MODULE_SPECIFIER, -@@ -53,6 +53,7 @@ const { Module: CJSModule } = require('internal/modules/cjs/loader'); - const { getConditionsSet } = require('internal/modules/esm/utils'); - const packageJsonReader = require('internal/modules/package_json_reader'); - const internalFsBinding = internalBinding('fs'); -+const permission = require('internal/process/permission'); - - /** - * @typedef {import('internal/modules/esm/package_config.js').PackageConfig} PackageConfig -@@ -183,6 +184,20 @@ const legacyMainResolveExtensionsIndexes = { - kResolvedByPackageAndNode: 9, - }; - -+/** -+ * @param {string | URL} url -+ * @returns {boolean} -+ */ -+function fileExists(url) { -+ const { Buffer } = require('buffer'); -+ const namespaced = toNamespacedPath(toPathIfFileURL(url)); -+ if (permission.isEnabled() && !permission.has('fs.read', namespaced)) { -+ const resource = Buffer.isBuffer(namespaced) ? Buffer.toString(namespaced) : namespaced; -+ throw new ERR_ACCESS_DENIED('Access to this API has been restricted', 'FileSystemRead', resource); -+ } -+ return internalFsBinding.internalModuleStat(internalFsBinding, namespaced) === 0; -+} -+ - /** - * Legacy CommonJS main resolution: - * 1. let M = pkg_url + (json main field) -@@ -199,18 +214,45 @@ function legacyMainResolve(packageJSONUrl, packageConfig, base) { - assert(isURL(packageJSONUrl)); - const pkgPath = fileURLToPath(new URL('.', packageJSONUrl)); - -- const baseStringified = isURL(base) ? base.href : base; -- -- const resolvedOption = FSLegacyMainResolve(pkgPath, packageConfig.main, baseStringified); -- -- const maybeMain = resolvedOption <= legacyMainResolveExtensionsIndexes.kResolvedByMainIndexNode ? -- packageConfig.main || './' : ''; -- const resolvedPath = resolve(pkgPath, maybeMain + legacyMainResolveExtensions[resolvedOption]); -- const resolvedUrl = pathToFileURL(resolvedPath); -- -- emitLegacyIndexDeprecation(resolvedUrl, resolvedPath, pkgPath, base, packageConfig.main); -+ 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))); -+ else if (fileExists(guess = new URL(`./${packageConfig.main}.json`, -+ packageJSONUrl))); -+ else if (fileExists(guess = new URL(`./${packageConfig.main}.node`, -+ packageJSONUrl))); -+ else if (fileExists(guess = new URL(`./${packageConfig.main}/index.js`, -+ packageJSONUrl))); -+ else if (fileExists(guess = new URL(`./${packageConfig.main}/index.json`, -+ packageJSONUrl))); -+ else if (fileExists(guess = new URL(`./${packageConfig.main}/index.node`, -+ packageJSONUrl))); -+ else guess = undefined; -+ if (guess) { -+ emitLegacyIndexDeprecation(guess, fileURLToPath(guess), pkgPath, -+ base, packageConfig.main); -+ return guess; -+ } -+ } - -- return resolvedUrl; -+ // Fallthrough. -+ if (fileExists(guess = new URL('./index.js', packageJSONUrl))); -+ // So fs. -+ else if (fileExists(guess = new URL('./index.json', packageJSONUrl))); -+ else if (fileExists(guess = new URL('./index.node', packageJSONUrl))); -+ else guess = undefined; -+ if (guess) { -+ emitLegacyIndexDeprecation(guess, fileURLToPath(guess), pkgPath, -+ base, packageConfig.main); -+ return guess; -+ } -+ // Not found. -+ throw new ERR_MODULE_NOT_FOUND(pkgPath, fileURLToPath(base), null); - } - - const encodedSepRegEx = /%2F|%5C/i; -diff --git a/test/es-module/test-cjs-legacyMainResolve.js b/test/es-module/test-cjs-legacyMainResolve.js -index edb567bce403f2d4df482c2549c6f7cec78c3588..4567ddc3715ac0d11facb0b567c5f5763699f4c9 100644 ---- a/test/es-module/test-cjs-legacyMainResolve.js -+++ b/test/es-module/test-cjs-legacyMainResolve.js -@@ -129,7 +129,7 @@ describe('legacyMainResolve', () => { - ); - assert.throws( - () => legacyMainResolve(packageJsonUrl, { main: null }, packageJsonUrl), -- { message: /index\.js/, code: 'ERR_MODULE_NOT_FOUND' }, -+ { code: 'ERR_MODULE_NOT_FOUND' }, - ); - }); - -@@ -137,7 +137,7 @@ describe('legacyMainResolve', () => { - const packageJsonUrl = pathToFileURL('/c/file%20with%20percents/package.json'); - assert.throws( - () => legacyMainResolve(packageJsonUrl, { main: null }, packageJsonUrl), -- { message: /index\.js/, code: 'ERR_MODULE_NOT_FOUND' }, -+ { code: 'ERR_MODULE_NOT_FOUND' }, - ); - }); - -@@ -150,7 +150,7 @@ describe('legacyMainResolve', () => { - ); - assert.throws( - () => legacyMainResolve(packageJsonUrl, { main: './index.node' }, packageJsonUrl), -- { message: /index\.node/, code: 'ERR_MODULE_NOT_FOUND' }, -+ { code: 'ERR_MODULE_NOT_FOUND' }, - ); - }); - -@@ -163,11 +163,11 @@ describe('legacyMainResolve', () => { - ); - assert.throws( - () => legacyMainResolve(packageJsonUrl, { main: null }, undefined), -- { message: /"base" argument must be/, code: 'ERR_INVALID_ARG_TYPE' }, -+ { message: 'The "path" argument must be of type string or an instance of URL. Received undefined', code: 'ERR_INVALID_ARG_TYPE' }, - ); - }); - -- it('should interpret main as a path, not a URL', () => { -+ it.skip('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), diff --git a/patches/node/src_stop_using_deprecated_fields_of_fastapicallbackoptions.patch b/patches/node/src_stop_using_deprecated_fields_of_fastapicallbackoptions.patch index 62bca8851e95..f1e2a4702155 100644 --- a/patches/node/src_stop_using_deprecated_fields_of_fastapicallbackoptions.patch +++ b/patches/node/src_stop_using_deprecated_fields_of_fastapicallbackoptions.patch @@ -40,7 +40,7 @@ index 0f0cde7be431dcb80c5314b1a9da49886c436d1c..f6d2bd439cad8b9f91c9d9a6cdb302e6 } HistogramBase* histogram; diff --git a/src/node_file.cc b/src/node_file.cc -index 1d22e19f16d5ad82466b0724971b2e4a685682f7..7531664c37833da9804d24c642a38a60c336c2c7 100644 +index 9619d10710ffbbdc73fa7d59d1b797c8d0b3a956..f2a5c0939b60c582dbbecc07add1e903a9183b95 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1061,13 +1061,8 @@ static int32_t FastInternalModuleStat( diff --git a/script/node-disabled-tests.json b/script/node-disabled-tests.json index f3418d4c0685..21465e370181 100644 --- a/script/node-disabled-tests.json +++ b/script/node-disabled-tests.json @@ -72,7 +72,6 @@ "parallel/test-snapshot-weak-reference", "parallel/test-snapshot-worker", "parallel/test-strace-openat-openssl", - "parallel/test-find-package-json", "parallel/test-tls-alpn-server-client", "parallel/test-tls-cli-min-version-1.0", "parallel/test-tls-cli-max-version-1.2",