Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

module,win: fix long path resolve #53294

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3096,8 +3096,14 @@ void BindingData::LegacyMainResolve(const FunctionCallbackInfo<Value>& 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)) {
Local<Value> local_file_path =
anonrig marked this conversation as resolved.
Show resolved Hide resolved
Buffer::Copy(
env->isolate(), file_path.c_str(), strlen(file_path.c_str()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
env->isolate(), file_path.c_str(), strlen(file_path.c_str()))
env->isolate(), file_path.c_str(), file_path.size())

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Thanks.

.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:
Expand Down Expand Up @@ -3133,8 +3139,14 @@ void BindingData::LegacyMainResolve(const FunctionCallbackInfo<Value>& 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)) {
Local<Value> local_file_path =
Buffer::Copy(
env->isolate(), file_path.c_str(), strlen(file_path.c_str()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

.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:
Expand Down
46 changes: 34 additions & 12 deletions src/node_modules.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -241,7 +242,7 @@ void BindingData::ReadPackageJSON(const FunctionCallbackInfo<Value>& 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) {
Expand All @@ -261,15 +262,10 @@ void BindingData::ReadPackageJSON(const FunctionCallbackInfo<Value>& args) {
permission::PermissionScope::kFileSystemRead,
path.ToStringView());

// TODO(StefanStojanovic): Remove ifdef after
// path.toNamespacedPath logic is ported to C++
#ifdef _WIN32
auto package_json = GetPackageJSON(
realm, "\\\\?\\" + path.ToString(), is_esm ? &error_context : nullptr);
#else
ToNamespacedPath(realm->env(), &path);
auto package_json =
GetPackageJSON(realm, path.ToString(), is_esm ? &error_context : nullptr);
#endif

if (package_json == nullptr) {
return;
}
Expand Down Expand Up @@ -323,9 +319,22 @@ 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.ToString().empty() &&
path_value.ToString().back() ==
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
path_value.ToString().back() ==
path_value.ToStringView().ends_with

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

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));
Expand All @@ -338,9 +347,22 @@ 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.ToString().empty() &&
path_value.ToString().back() ==
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

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;
Expand Down
42 changes: 0 additions & 42 deletions test/es-module/test-GH-50753.js

This file was deleted.

2 changes: 1 addition & 1 deletion test/es-module/test-cjs-esm-warn.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
);

Expand Down
18 changes: 11 additions & 7 deletions test/es-module/test-cjs-legacyMainResolve-permission.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)},
)
)
});
`,
Expand Down Expand Up @@ -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)},
)
)
});
`,
Expand Down
189 changes: 189 additions & 0 deletions test/es-module/test-esm-long-path-win.js
Original file line number Diff line number Diff line change
@@ -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);
});
});
Loading