From e676d98435d49fc73d826a68e577fd3b8f2b5a1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=BCseyin=20A=C3=A7acak?= <110401522+huseyinacacak-janea@users.noreply.github.com> Date: Sat, 10 Aug 2024 17:12:53 +0300 Subject: [PATCH] module,win: fix long path resolve MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/53294 Fixes: https://github.com/nodejs/node/issues/50753 Reviewed-By: Yagiz Nizipli Reviewed-By: Vinícius Lourenço Claro Cardoso --- src/node_file.cc | 20 +- src/node_modules.cc | 46 +++-- test/es-module/test-GH-50753.js | 42 ---- test/es-module/test-cjs-esm-warn.js | 2 +- .../test-cjs-legacyMainResolve-permission.js | 18 +- test/es-module/test-esm-long-path-win.js | 189 ++++++++++++++++++ 6 files changed, 250 insertions(+), 67 deletions(-) delete mode 100644 test/es-module/test-GH-50753.js create mode 100644 test/es-module/test-esm-long-path-win.js diff --git a/src/node_file.cc b/src/node_file.cc index c13c364665f4bc..b4b914d580baf5 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -3235,8 +3235,14 @@ void BindingData::LegacyMainResolve(const FunctionCallbackInfo& args) { for (int i = 0; i < legacy_main_extensions_with_main_end; i++) { file_path = *initial_file_path + std::string(legacy_main_extensions[i]); - - switch (FilePathIsFile(env, file_path)) { + // TODO(anonrig): Remove this when ToNamespacedPath supports std::string + Local local_file_path = + Buffer::Copy(env->isolate(), file_path.c_str(), file_path.size()) + .ToLocalChecked(); + BufferValue buff_file_path(isolate, local_file_path); + ToNamespacedPath(env, &buff_file_path); + + switch (FilePathIsFile(env, buff_file_path.ToString())) { case BindingData::FilePathIsFileReturnType::kIsFile: return args.GetReturnValue().Set(i); case BindingData::FilePathIsFileReturnType::kIsNotFile: @@ -3272,8 +3278,14 @@ void BindingData::LegacyMainResolve(const FunctionCallbackInfo& args) { i < legacy_main_extensions_package_fallback_end; i++) { file_path = *initial_file_path + std::string(legacy_main_extensions[i]); - - switch (FilePathIsFile(env, file_path)) { + // TODO(anonrig): Remove this when ToNamespacedPath supports std::string + Local local_file_path = + Buffer::Copy(env->isolate(), file_path.c_str(), file_path.size()) + .ToLocalChecked(); + BufferValue buff_file_path(isolate, local_file_path); + ToNamespacedPath(env, &buff_file_path); + + switch (FilePathIsFile(env, buff_file_path.ToString())) { case BindingData::FilePathIsFileReturnType::kIsFile: return args.GetReturnValue().Set(i); case BindingData::FilePathIsFileReturnType::kIsNotFile: diff --git a/src/node_modules.cc b/src/node_modules.cc index 2e80ff4e95a5c1..2649b07bf2cf13 100644 --- a/src/node_modules.cc +++ b/src/node_modules.cc @@ -4,6 +4,7 @@ #include "node_errors.h" #include "node_external_reference.h" #include "node_url.h" +#include "path.h" #include "permission/permission.h" #include "permission/permission_base.h" #include "util-inl.h" @@ -241,7 +242,7 @@ void BindingData::ReadPackageJSON(const FunctionCallbackInfo& args) { Realm* realm = Realm::GetCurrent(args); auto isolate = realm->isolate(); - Utf8Value path(isolate, args[0]); + BufferValue path(isolate, args[0]); bool is_esm = args[1]->IsTrue(); auto error_context = ErrorContext(); if (is_esm) { @@ -261,15 +262,10 @@ void BindingData::ReadPackageJSON(const FunctionCallbackInfo& args) { permission::PermissionScope::kFileSystemRead, path.ToStringView()); - // TODO(StefanStojanovic): Remove ifdef after - // path.toNamespacedPath logic is ported to C++ -#ifdef _WIN32 + ToNamespacedPath(realm->env(), &path); auto package_json = GetPackageJSON( - realm, "\\\\?\\" + path.ToString(), is_esm ? &error_context : nullptr); -#else - auto package_json = - GetPackageJSON(realm, path.ToString(), is_esm ? &error_context : nullptr); -#endif + realm, path.ToStringView(), is_esm ? &error_context : nullptr); + if (package_json == nullptr) { return; } @@ -323,9 +319,21 @@ void BindingData::GetNearestParentPackageJSON( CHECK(args[0]->IsString()); Realm* realm = Realm::GetCurrent(args); - Utf8Value path_value(realm->isolate(), args[0]); + BufferValue path_value(realm->isolate(), args[0]); + // Check if the path has a trailing slash. If so, add it after + // ToNamespacedPath() as it will be deleted by ToNamespacedPath() + bool slashCheck = path_value.ToStringView().ends_with( + std::filesystem::path::preferred_separator); + + ToNamespacedPath(realm->env(), &path_value); + + std::string path_value_str = path_value.ToString(); + if (slashCheck) { + path_value_str.push_back(std::filesystem::path::preferred_separator); + } + auto package_json = - TraverseParent(realm, std::filesystem::path(path_value.ToString())); + TraverseParent(realm, std::filesystem::path(path_value_str)); if (package_json != nullptr) { args.GetReturnValue().Set(package_json->Serialize(realm)); @@ -338,9 +346,21 @@ void BindingData::GetNearestParentPackageJSONType( CHECK(args[0]->IsString()); Realm* realm = Realm::GetCurrent(args); - Utf8Value path(realm->isolate(), args[0]); + BufferValue path_value(realm->isolate(), args[0]); + // Check if the path has a trailing slash. If so, add it after + // ToNamespacedPath() as it will be deleted by ToNamespacedPath() + bool slashCheck = path_value.ToStringView().ends_with( + std::filesystem::path::preferred_separator); + + ToNamespacedPath(realm->env(), &path_value); + + std::string path_value_str = path_value.ToString(); + if (slashCheck) { + path_value_str.push_back(std::filesystem::path::preferred_separator); + } + auto package_json = - TraverseParent(realm, std::filesystem::path(path.ToString())); + TraverseParent(realm, std::filesystem::path(path_value_str)); if (package_json == nullptr) { return; diff --git a/test/es-module/test-GH-50753.js b/test/es-module/test-GH-50753.js deleted file mode 100644 index 1cecfa5bebf6a5..00000000000000 --- a/test/es-module/test-GH-50753.js +++ /dev/null @@ -1,42 +0,0 @@ -'use strict'; - -// Flags: --expose-internals - -const common = require('../common'); -if (!common.isWindows) { - common.skip('this test is Windows-specific.'); -} - -const fs = require('fs'); -const path = require('path'); -const tmpdir = require('../common/tmpdir'); - -// https://github.com/nodejs/node/issues/50753 -// Make a path that is more than 260 chars long. -// Module layout will be the following: -// package.json -// main -// main.js - -const packageDirNameLen = Math.max(260 - tmpdir.path.length, 1); -const packageDirName = tmpdir.resolve('x'.repeat(packageDirNameLen)); -const packageDirPath = path.resolve(packageDirName); -const packageJsonFilePath = path.join(packageDirPath, 'package.json'); -const mainDirName = 'main'; -const mainDirPath = path.resolve(packageDirPath, mainDirName); -const mainJsFile = 'main.js'; -const mainJsFilePath = path.resolve(mainDirPath, mainJsFile); - -tmpdir.refresh(); - -fs.mkdirSync(packageDirPath); -fs.writeFileSync( - packageJsonFilePath, - `{"main":"${mainDirName}/${mainJsFile}"}` -); -fs.mkdirSync(mainDirPath); -fs.writeFileSync(mainJsFilePath, 'console.log("hello world");'); - -require('internal/modules/run_main').executeUserEntryPoint(packageDirPath); - -tmpdir.refresh(); diff --git a/test/es-module/test-cjs-esm-warn.js b/test/es-module/test-cjs-esm-warn.js index 29d83ef92ce8d0..ccd4d276fdd535 100644 --- a/test/es-module/test-cjs-esm-warn.js +++ b/test/es-module/test-cjs-esm-warn.js @@ -10,7 +10,7 @@ const { describe, it } = require('node:test'); const requiringCjsAsEsm = path.resolve(fixtures.path('/es-modules/cjs-esm.js')); const requiringEsm = path.resolve(fixtures.path('/es-modules/cjs-esm-esm.js')); -const pjson = path.resolve( +const pjson = path.toNamespacedPath( fixtures.path('/es-modules/package-type-module/package.json') ); diff --git a/test/es-module/test-cjs-legacyMainResolve-permission.js b/test/es-module/test-cjs-legacyMainResolve-permission.js index b45e3dee3bbfcd..392bfb753d7764 100644 --- a/test/es-module/test-cjs-legacyMainResolve-permission.js +++ b/test/es-module/test-cjs-legacyMainResolve-permission.js @@ -63,9 +63,11 @@ describe('legacyMainResolve', () => { assert.throws(() => legacyMainResolve(packageJsonUrl, packageConfig, base), { code: 'ERR_ACCESS_DENIED', - resource: path.resolve( - ${JSON.stringify(fixtextureFolderEscaped)}, - ${JSON.stringify(mainOrFolder)}, + resource: path.toNamespacedPath( + path.resolve( + ${JSON.stringify(fixtextureFolderEscaped)}, + ${JSON.stringify(mainOrFolder)}, + ) ) }); `, @@ -120,10 +122,12 @@ describe('legacyMainResolve', () => { assert.throws(() => legacyMainResolve(packageJsonUrl, packageConfig, base), { code: 'ERR_ACCESS_DENIED', - resource: path.resolve( - ${JSON.stringify(fixtextureFolderEscaped)}, - ${JSON.stringify(folder)}, - ${JSON.stringify(expectedFile)}, + resource: path.toNamespacedPath( + path.resolve( + ${JSON.stringify(fixtextureFolderEscaped)}, + ${JSON.stringify(folder)}, + ${JSON.stringify(expectedFile)}, + ) ) }); `, diff --git a/test/es-module/test-esm-long-path-win.js b/test/es-module/test-esm-long-path-win.js new file mode 100644 index 00000000000000..ef47759698992c --- /dev/null +++ b/test/es-module/test-esm-long-path-win.js @@ -0,0 +1,189 @@ +'use strict'; + +// Flags: --expose-internals + +const common = require('../common'); +if (!common.isWindows) { + common.skip('this test is Windows-specific.'); +} + +const fs = require('fs'); +const path = require('path'); +const tmpdir = require('../common/tmpdir'); +const { describe, it } = require('node:test'); +const { legacyMainResolve } = require('node:internal/modules/esm/resolve'); +const { pathToFileURL } = require('node:url'); +const { spawnPromisified } = require('../common'); +const assert = require('assert'); +const { execPath } = require('node:process'); + +describe('long path on Windows', () => { + it('check long path in ReadPackageJSON', () => { + // Module layout will be the following: + // package.json + // main + // main.js + const packageDirNameLen = Math.max(260 - tmpdir.path.length, 1); + const packageDirName = tmpdir.resolve('x'.repeat(packageDirNameLen)); + const packageDirPath = path.resolve(packageDirName); + const packageJsonFilePath = path.join(packageDirPath, 'package.json'); + const mainDirName = 'main'; + const mainDirPath = path.resolve(packageDirPath, mainDirName); + const mainJsFile = 'main.js'; + const mainJsFilePath = path.resolve(mainDirPath, mainJsFile); + + tmpdir.refresh(); + + fs.mkdirSync(packageDirPath); + fs.writeFileSync( + packageJsonFilePath, + `{"main":"${mainDirName}/${mainJsFile}"}` + ); + fs.mkdirSync(mainDirPath); + fs.writeFileSync(mainJsFilePath, 'console.log("hello world");'); + + require('internal/modules/run_main').executeUserEntryPoint(packageDirPath); + + tmpdir.refresh(); + }); + + it('check long path in LegacyMainResolve - 1', () => { + // Module layout will be the following: + // package.json + // index.js + const packageDirNameLen = Math.max(260 - tmpdir.path.length, 1); + const packageDirPath = tmpdir.resolve('y'.repeat(packageDirNameLen)); + const packageJSPath = path.join(packageDirPath, 'package.json'); + const indexJSPath = path.join(packageDirPath, 'index.js'); + const packageConfig = { }; + + tmpdir.refresh(); + + fs.mkdirSync(packageDirPath); + fs.writeFileSync(packageJSPath, ''); + fs.writeFileSync(indexJSPath, ''); + + const packageJsonUrl = pathToFileURL( + path.resolve(packageJSPath) + ); + + console.log(legacyMainResolve(packageJsonUrl, packageConfig, '')); + }); + + it('check long path in LegacyMainResolve - 2', () => { + // Module layout will be the following: + // package.json + // main.js + const packageDirNameLen = Math.max(260 - tmpdir.path.length, 1); + const packageDirPath = tmpdir.resolve('z'.repeat(packageDirNameLen)); + const packageJSPath = path.join(packageDirPath, 'package.json'); + const indexJSPath = path.join(packageDirPath, 'main.js'); + const packageConfig = { main: 'main.js' }; + + tmpdir.refresh(); + + fs.mkdirSync(packageDirPath); + fs.writeFileSync(packageJSPath, ''); + fs.writeFileSync(indexJSPath, ''); + + const packageJsonUrl = pathToFileURL( + path.resolve(packageJSPath) + ); + + console.log(legacyMainResolve(packageJsonUrl, packageConfig, '')); + }); + + it('check long path in GetNearestParentPackageJSON', async () => { + // Module layout will be the following: + // node_modules + // test-module + // package.json (path is less than 260 chars long) + // cjs + // package.json (path is more than 260 chars long) + // index.js + const testModuleDirPath = 'node_modules/test-module'; + let cjsDirPath = path.join(testModuleDirPath, 'cjs'); + let modulePackageJSPath = path.join(testModuleDirPath, 'package.json'); + let cjsPackageJSPath = path.join(cjsDirPath, 'package.json'); + let cjsIndexJSPath = path.join(cjsDirPath, 'index.js'); + + const tmpDirNameLen = Math.max( + 260 - tmpdir.path.length - cjsPackageJSPath.length, 1); + const tmpDirPath = tmpdir.resolve('k'.repeat(tmpDirNameLen)); + + cjsDirPath = path.join(tmpDirPath, cjsDirPath); + modulePackageJSPath = path.join(tmpDirPath, modulePackageJSPath); + cjsPackageJSPath = path.join(tmpDirPath, cjsPackageJSPath); + cjsIndexJSPath = path.join(tmpDirPath, cjsIndexJSPath); + + tmpdir.refresh(); + + fs.mkdirSync(cjsDirPath, { recursive: true }); + fs.writeFileSync( + modulePackageJSPath, + `{ + "type": "module" + }` + ); + fs.writeFileSync( + cjsPackageJSPath, + `{ + "type": "commonjs" + }` + ); + fs.writeFileSync( + cjsIndexJSPath, + 'const fs = require("fs");' + ); + const { code, signal, stderr } = await spawnPromisified(execPath, [cjsIndexJSPath]); + assert.strictEqual(stderr.trim(), ''); + assert.strictEqual(code, 0); + assert.strictEqual(signal, null); + }); + + it('check long path in GetNearestParentPackageJSONType', async () => { + // Module layout will be the following: + // node_modules + // test-module + // package.json (path is less than 260 chars long) + // cjs + // package.json (path is more than 260 chars long) + // index.js + const testModuleDirPath = 'node_modules/test-module'; + let cjsDirPath = path.join(testModuleDirPath, 'cjs'); + let modulePackageJSPath = path.join(testModuleDirPath, 'package.json'); + let cjsPackageJSPath = path.join(cjsDirPath, 'package.json'); + let cjsIndexJSPath = path.join(cjsDirPath, 'index.js'); + + const tmpDirNameLen = Math.max(260 - tmpdir.path.length - cjsPackageJSPath.length, 1); + const tmpDirPath = tmpdir.resolve('l'.repeat(tmpDirNameLen)); + + cjsDirPath = path.join(tmpDirPath, cjsDirPath); + modulePackageJSPath = path.join(tmpDirPath, modulePackageJSPath); + cjsPackageJSPath = path.join(tmpDirPath, cjsPackageJSPath); + cjsIndexJSPath = path.join(tmpDirPath, cjsIndexJSPath); + + tmpdir.refresh(); + + fs.mkdirSync(cjsDirPath, { recursive: true }); + fs.writeFileSync( + modulePackageJSPath, + `{ + "type": "module" + }` + ); + fs.writeFileSync( + cjsPackageJSPath, + `{ + "type": "commonjs" + }` + ); + + fs.writeFileSync(cjsIndexJSPath, 'import fs from "node:fs/promises";'); + const { code, signal, stderr } = await spawnPromisified(execPath, [cjsIndexJSPath]); + + assert.ok(stderr.includes('Warning: To load an ES module')); + assert.strictEqual(code, 1); + assert.strictEqual(signal, null); + }); +});