From fe2648c28056ad31ffc05a56c1c6f07781e1bfc3 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Fri, 2 Aug 2019 01:30:32 -0400 Subject: [PATCH 01/11] module: pkg exports validations and fallbacks --- doc/api/esm.md | 61 +++++-- doc/api/modules.md | 19 +-- lib/internal/modules/cjs/loader.js | 58 +++++-- src/module_wrap.cc | 152 +++++++++++++----- test/es-module/test-esm-exports.mjs | 76 +++++---- .../node_modules/pkgexports/package.json | 14 +- 6 files changed, 274 insertions(+), 106 deletions(-) diff --git a/doc/api/esm.md b/doc/api/esm.md index 07a3a71e90f0fd..5653c85a21e945 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -242,13 +242,13 @@ throw when an attempt is made to import them: ```js import submodule from 'es-module-package/private-module.js'; -// Throws - Package exports error +// Throws - Module not found ``` > Note: this is not a strong encapsulation as any private modules can still be > loaded by absolute paths. -Folders can also be mapped with package exports as well: +Folders can also be mapped with package exports: ```js @@ -268,8 +268,24 @@ import feature from 'es-module-package/features/x.js'; If a package has no exports, setting `"exports": false` can be used instead of `"exports": {}` to indicate the package does not intend for submodules to be exposed. -This is just a convention that works because `false`, just like `{}`, has no -iterable own properties. + +Any invalid exports entries will be ignored. This includes exports not +starting with `"./"` or a missing trailing `"/"` for directory exports. + +Array fallback support is provided for exports, similarly to import maps +in order to be forward-compatible with fallback workflows in future: + + +```js +{ + "exports": { + "./submodule": ["not:valid", "./submodule.js"] + } +} +``` + +Since `"not:valid"` is not a supported target, `"./submodule.js"` is used +instead as the fallback, as if it were the only target. ## import Specifiers @@ -660,7 +676,7 @@ CommonJS loader. Additional formats such as _"addon"_ can be extended in future updates. In the following algorithms, all subroutine errors are propagated as errors -of these top-level routines. +of these top-level routines unless stated otherwise. _isMain_ is **true** when resolving the Node.js application entry point. @@ -681,6 +697,9 @@ _isMain_ is **true** when resolving the Node.js application entry point. > 1. Note: _specifier_ is now a bare specifier. > 1. Set _resolvedURL_ the result of > **PACKAGE_RESOLVE**(_specifier_, _parentURL_). +> 1. If _resolvedURL_ contains any percent encodings of _"/"_ or _"\"_ (_"%2f"_ +> and _"%5C"_ respectively), then +> 1. Throw an _Invalid Specifier_ error. > 1. If the file at _resolvedURL_ does not exist, then > 1. Throw a _Module Not Found_ error. > 1. Set _resolvedURL_ to the real path of _resolvedURL_. @@ -753,19 +772,39 @@ _isMain_ is **true** when resolving the Node.js application entry point. > 1. Set _packagePath_ to _"./"_ concatenated with _packagePath_. > 1. If _packagePath_ is a key of _exports_, then > 1. Let _target_ be the value of _exports[packagePath]_. -> 1. If _target_ is not a String, continue the loop. -> 1. Return the URL resolution of the concatenation of _packageURL_ and -> _target_. +> 1. Return **PACKAGE_EXPORTS_TARGET_RESOLVE**(_packageURL_, _target_, +> _""_). > 1. Let _directoryKeys_ be the list of keys of _exports_ ending in > _"/"_, sorted by length descending. > 1. For each key _directory_ in _directoryKeys_, do > 1. If _packagePath_ starts with _directory_, then > 1. Let _target_ be the value of _exports[directory]_. -> 1. If _target_ is not a String, continue the loop. > 1. Let _subpath_ be the substring of _target_ starting at the index > of the length of _directory_. -> 1. Return the URL resolution of the concatenation of _packageURL_, -> _target_ and _subpath_. +> 1. Return **PACKAGE_EXPORTS_TARGET_RESOLVE**(_packageURL_, _target_, +> _subpath_. +> 1. Throw a _Module Not Found_ error. + +**PACKAGE_EXPORTS_TARGET_RESOLVE**(_packageURL_, _target_, _subpath_) +> 1. If _target_ is a String, then +> 1. If _target_ does not start with _"./"_, or _subpath_ has non-zero +> length and _target_ does not end with _"/"_, then +> 1. Throw a _Module Not Found_ error. +> 1. Let _resolvedTarget_ be the URL resolution of the concatenation of +> _packageURL_ and _target_. +> 1. If _resolvedTarget_ is contained in _packageURL_, then +> 1. Let _resolved_ be the URL resolution of the concatenation of +> _subpath_ and _resolvedTarget_. +> 1. If _resolved_ is contained in _packageURL_, then +> 1. Return _resolved_. +> 1. Otherwise, if _target_ is an Array, then +> 1. For each item _targetValue_ in _target_, do +> 1. If _targetValue_ is not a String, continue the loop. +> 1. Let _resolved_ be the result of +> **PACKAGE_EXPORTS_TARGET_RESOLVE**(_packageURL_, _targetValue_, +> _subpath_), continuing the loop on abrupt completion. +> 1. Assert: _resolved_ is a String. +> 1. Return _resolved_. > 1. Throw a _Module Not Found_ error. **ESM_FORMAT**(_url_, _isMain_) diff --git a/doc/api/modules.md b/doc/api/modules.md index bf8209965e9122..7197ef6ae2fdaa 100644 --- a/doc/api/modules.md +++ b/doc/api/modules.md @@ -202,11 +202,12 @@ NODE_MODULES_PATHS(START) 5. return DIRS ``` -If `--experimental-exports` is enabled, -node allows packages loaded via `LOAD_NODE_MODULES` to explicitly declare -which filepaths to expose and how they should be interpreted. -This expands on the control packages already had using the `main` field. -With this feature enabled, the `LOAD_NODE_MODULES` changes as follows: +If `--experimental-exports` is enabled, Node.js allows packages loaded via +`LOAD_NODE_MODULES` to explicitly declare which file paths to expose and how +they should be interpreted. This expands on the control packages already had +using the `main` field. + +With this feature enabled, the `LOAD_NODE_MODULES` changes are: ```txt LOAD_NODE_MODULES(X, START) @@ -224,10 +225,10 @@ RESOLVE_BARE_SPECIFIER(DIR, X) b. If "exports" is null or undefined, GOTO 3. c. Find the longest key in "exports" that the subpath starts with. d. If no such key can be found, throw "not found". - e. If the key matches the subpath entirely, return DIR/name/${exports[key]}. - f. If either the key or exports[key] do not end with a slash (`/`), - throw "not found". - g. Return DIR/name/${exports[key]}${subpath.slice(key.length)}. + e. let RESOLVED_URL = + PACKAGE_EXPORTS_TARGET_RESOLVE(pathToFileURL(DIR/name), exports[key], + subpath.slice(key.length)), as defined in the esm resolver. + f. return fileURLToPath(RESOLVED_URL) 3. return DIR/X ``` diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 95b56e08520a52..cae3f42a39910c 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -348,18 +348,18 @@ function resolveExports(nmPath, request, absoluteRequest) { const basePath = path.resolve(nmPath, name); const pkgExports = readExports(basePath); + const mappingKey = `.${expansion}`; - if (pkgExports != null) { - const mappingKey = `.${expansion}`; - const mapping = pkgExports[mappingKey]; - if (typeof mapping === 'string') { - return fileURLToPath(new URL(mapping, `${pathToFileURL(basePath)}/`)); + if (typeof pkgExports === 'object' && pkgExports !== null) { + if (mappingKey in pkgExports) { + const mapping = pkgExports[mappingKey]; + return resolveExportsTarget(pathToFileURL(basePath + '/'), mapping, '', + basePath, mappingKey); } let dirMatch = ''; - for (const [candidateKey, candidateValue] of Object.entries(pkgExports)) { + for (const candidateKey of Object.keys(pkgExports)) { if (candidateKey[candidateKey.length - 1] !== '/') continue; - if (candidateValue[candidateValue.length - 1] !== '/') continue; if (candidateKey.length > dirMatch.length && StringPrototype.startsWith(mappingKey, candidateKey)) { dirMatch = candidateKey; @@ -367,15 +367,13 @@ function resolveExports(nmPath, request, absoluteRequest) { } if (dirMatch !== '') { - const dirMapping = pkgExports[dirMatch]; - const remainder = StringPrototype.slice(mappingKey, dirMatch.length); - const expectedPrefix = - new URL(dirMapping, `${pathToFileURL(basePath)}/`); - const resolved = new URL(remainder, expectedPrefix).href; - if (StringPrototype.startsWith(resolved, expectedPrefix.href)) { - return fileURLToPath(resolved); - } + const mapping = pkgExports[dirMatch]; + const subpath = StringPrototype.slice(mappingKey, dirMatch.length); + return resolveExportsTarget(pathToFileURL(basePath + '/'), mapping, + subpath, basePath, mappingKey); } + } + if (pkgExports != null) { // eslint-disable-next-line no-restricted-syntax const e = new Error(`Package exports for '${basePath}' do not define ` + `a '${mappingKey}' subpath`); @@ -387,6 +385,36 @@ function resolveExports(nmPath, request, absoluteRequest) { return path.resolve(nmPath, request); } +function resolveExportsTarget(pkgPath, target, subpath, basePath, mappingKey) { + if (typeof target === 'string') { + if (target.substr(0, 2) === './' && + (subpath.length === 0 || target.endsWith('/'))) { + const resolvedTarget = new URL(target, pkgPath); + if (StringPrototype.startsWith(resolvedTarget.href, pkgPath.href)) { + const resolved = new URL(subpath, resolvedTarget); + if (StringPrototype.startsWith(resolved.href, pkgPath.href)) { + return fileURLToPath(resolved); + } + } + } + } else if (Array.isArray(target)) { + for (const targetValue of target) { + if (typeof targetValue !== 'string') continue; + try { + return resolveExportsTarget(pkgPath, targetValue, subpath, basePath, + mappingKey); + } catch (e) { + if (e.code !== 'MODULE_NOT_FOUND') throw e; + } + } + } + // eslint-disable-next-line no-restricted-syntax + const e = new Error(`Package exports for '${basePath}' do not define ` + + `a '${mappingKey}' subpath`); + e.code = 'MODULE_NOT_FOUND'; + throw e; +} + Module._findPath = function(request, paths, isMain) { const absoluteRequest = path.isAbsolute(request); if (absoluteRequest) { diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 5b33ef261cf69c..88e262afc7fdbd 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -758,7 +758,8 @@ Maybe FinalizeResolution(Environment* env, const std::string& path = resolved.ToFilePath(); if (CheckDescriptorAtPath(path) != FILE) { - std::string msg = "Cannot find module '" + path + + std::string msg = "Cannot find module '" + + (path.length() != 0 ? path : resolved.path()) + "' imported from " + base.ToFilePath(); node::THROW_ERR_MODULE_NOT_FOUND(env, msg.c_str()); return Nothing(); @@ -800,6 +801,42 @@ Maybe PackageMainResolve(Environment* env, return Nothing(); } +void ThrowExportsNotFound(Environment* env, + const std::string& match, + const URL& pjson_url, + const URL& base) { + const std::string msg = "Package exports for '" + + URL(".", pjson_url).ToFilePath() + "' do not define a '" + match + + "' subpath, imported from " + base.ToFilePath(); + node::THROW_ERR_MODULE_NOT_FOUND(env, msg.c_str()); +} + +Maybe ResolveExportsTarget(Environment* env, + const std::string& target, + const std::string& subpath, + const std::string& match, + const URL& pjson_url, + const URL& base) { + if (target.substr(0, 2) == "./") { + if (subpath.length() > 0 && target.back() != '/') { + return Nothing(); + } + URL resolved(target, pjson_url); + std::string resolved_path = resolved.path(); + std::string pkg_path = URL(".", pjson_url).path(); + if (resolved_path.find(pkg_path) == 0) { + if (subpath.length() == 0) return Just(resolved); + URL subpath_resolved(subpath, resolved); + std::string subpath_resolved_path = subpath_resolved.path(); + if (subpath_resolved_path.find(pkg_path) == 0) { + return Just(subpath_resolved); + } + } + } + // Note: std: / nodejs: support goes here + return Nothing(); +} + Maybe PackageExportsResolve(Environment* env, const URL& pjson_url, const std::string& pkg_subpath, @@ -809,57 +846,92 @@ Maybe PackageExportsResolve(Environment* env, Isolate* isolate = env->isolate(); Local context = env->context(); Local exports = pcfg.exports.Get(isolate); - if (exports->IsObject()) { - Local exports_obj = exports.As(); - Local subpath = String::NewFromUtf8(isolate, - pkg_subpath.c_str(), v8::NewStringType::kNormal).ToLocalChecked(); + if (!exports->IsObject()) { + ThrowExportsNotFound(env, pkg_subpath, pjson_url, base); + return Nothing(); + } + Local exports_obj = exports.As(); + Local subpath = String::NewFromUtf8(isolate, + pkg_subpath.c_str(), v8::NewStringType::kNormal).ToLocalChecked(); - auto target = exports_obj->Get(context, subpath).ToLocalChecked(); + if (exports_obj->Has(context, subpath).FromJust()) { + Local target = exports_obj->Get(context, subpath).ToLocalChecked(); if (target->IsString()) { Utf8Value target_utf8(isolate, target.As()); - std::string target(*target_utf8, target_utf8.length()); - if (target.substr(0, 2) == "./") { - URL target_url(target, pjson_url); - return FinalizeResolution(env, target_url, base); + std::string target_str(*target_utf8, target_utf8.length()); + Maybe resolved = ResolveExportsTarget(env, target_str, "", + pkg_subpath, pjson_url, base); + if (resolved.IsNothing()) { + ThrowExportsNotFound(env, pkg_subpath, pjson_url, base); + return Nothing(); + } + return FinalizeResolution(env, resolved.FromJust(), base); + } else if (target->IsArray()) { + Local target_arr = target.As(); + const uint32_t length = target_arr->Length(); + for (uint32_t i = 0; i < length; i++) { + auto target_item = target_arr->Get(context, i).ToLocalChecked(); + if (target_item->IsString()) { + Utf8Value target_utf8(isolate, target_item.As()); + std::string target(*target_utf8, target_utf8.length()); + Maybe resolved = ResolveExportsTarget(env, target, "", + pkg_subpath, pjson_url, base); + if (resolved.IsNothing()) continue; + return FinalizeResolution(env, resolved.FromJust(), base); + } } } + ThrowExportsNotFound(env, pkg_subpath, pjson_url, base); + return Nothing(); + } - Local best_match; - std::string best_match_str = ""; - Local keys = - exports_obj->GetOwnPropertyNames(context).ToLocalChecked(); - for (uint32_t i = 0; i < keys->Length(); ++i) { - Local key = keys->Get(context, i).ToLocalChecked().As(); - Utf8Value key_utf8(isolate, key); - std::string key_str(*key_utf8, key_utf8.length()); - if (key_str.back() != '/') continue; - if (pkg_subpath.substr(0, key_str.length()) == key_str && - key_str.length() > best_match_str.length()) { - best_match = key; - best_match_str = key_str; - } + Local best_match; + std::string best_match_str = ""; + Local keys = + exports_obj->GetOwnPropertyNames(context).ToLocalChecked(); + for (uint32_t i = 0; i < keys->Length(); ++i) { + Local key = keys->Get(context, i).ToLocalChecked().As(); + Utf8Value key_utf8(isolate, key); + std::string key_str(*key_utf8, key_utf8.length()); + if (key_str.back() != '/') continue; + if (pkg_subpath.substr(0, key_str.length()) == key_str && + key_str.length() > best_match_str.length()) { + best_match = key; + best_match_str = key_str; } + } - if (best_match_str.length() > 0) { - auto target = exports_obj->Get(context, best_match).ToLocalChecked(); - if (target->IsString()) { - Utf8Value target_utf8(isolate, target.As()); - std::string target(*target_utf8, target_utf8.length()); - if (target.back() == '/' && target.substr(0, 2) == "./") { - std::string subpath = pkg_subpath.substr(best_match_str.length()); - URL url_prefix(target, pjson_url); - URL target_url(subpath, url_prefix); - if (target_url.path().find(url_prefix.path()) == 0) { - return FinalizeResolution(env, target_url, base); - } + if (best_match_str.length() > 0) { + auto target = exports_obj->Get(context, best_match).ToLocalChecked(); + std::string subpath = pkg_subpath.substr(best_match_str.length()); + if (target->IsString()) { + Utf8Value target_utf8(isolate, target.As()); + std::string target(*target_utf8, target_utf8.length()); + Maybe resolved = ResolveExportsTarget(env, target, subpath, + pkg_subpath, pjson_url, base); + if (resolved.IsNothing()) { + ThrowExportsNotFound(env, pkg_subpath, pjson_url, base); + return Nothing(); + } + return FinalizeResolution(env, URL(subpath, resolved.FromJust()), base); + } else if (target->IsArray()) { + Local target_arr = target.As(); + const uint32_t length = target_arr->Length(); + for (uint32_t i = 0; i < length; i++) { + auto target_item = target_arr->Get(context, i).ToLocalChecked(); + if (target_item->IsString()) { + Utf8Value target_utf8(isolate, target_item.As()); + std::string target_str(*target_utf8, target_utf8.length()); + Maybe resolved = ResolveExportsTarget(env, target_str, subpath, + pkg_subpath, pjson_url, base); + if (resolved.IsNothing()) continue; + return FinalizeResolution(env, resolved.FromJust(), base); } } } } - std::string msg = "Package exports for '" + - URL(".", pjson_url).ToFilePath() + "' do not define a '" + pkg_subpath + - "' subpath, imported from " + base.ToFilePath(); - node::THROW_ERR_MODULE_NOT_FOUND(env, msg.c_str()); + + ThrowExportsNotFound(env, pkg_subpath, pjson_url, base); return Nothing(); } diff --git a/test/es-module/test-esm-exports.mjs b/test/es-module/test-esm-exports.mjs index 6c42d3b64ead40..dd07b0ec7a51da 100644 --- a/test/es-module/test-esm-exports.mjs +++ b/test/es-module/test-esm-exports.mjs @@ -17,6 +17,9 @@ import { requireFixture, importFixture } from '../fixtures/pkgexports.mjs'; ['pkgexports/space', { default: 'encoded path' }], // Verifying that normal packages still work with exports turned on. isRequire ? ['baz/index', { default: 'eye catcher' }] : [null], + // Fallbacks + ['pkgexports/fallbackdir/asdf.js', { default: 'asdf' }], + ['pkgexports/fallbackfile', { default: 'asdf' }], ]); for (const [validSpecifier, expected] of validSpecifiers) { if (validSpecifier === null) continue; @@ -27,20 +30,40 @@ import { requireFixture, importFixture } from '../fixtures/pkgexports.mjs'; })); } - // There's no such export - so there's nothing to do. - loadFixture('pkgexports/missing').catch(mustCall((err) => { - strictEqual(err.code, (isRequire ? '' : 'ERR_') + 'MODULE_NOT_FOUND'); - assertStartsWith(err.message, 'Package exports'); - assertIncludes(err.message, 'do not define a \'./missing\' subpath'); - })); + const undefinedExports = new Map([ + // There's no such export - so there's nothing to do. + ['pkgexports/missing', './missing'], + // The file exists but isn't exported. The exports is a number which counts + // as a non-null value without any properties, just like `{}`. + ['pkgexports-number/hidden.js', './hidden.js'], + // Even though 'pkgexports/sub/asdf.js' works, alternate "path-like" + // variants do not to prevent confusion and accidental loopholes. + ['pkgexports/sub/./../asdf.js', './sub/./../asdf.js'], + // This path steps back inside the package but goes through an exports + // target that escapes the package, so we still catch that as invalid + ['pkgexports/belowdir/pkgexports/asdf.js', './belowdir/pkgexports/asdf.js'], + // This target file steps below the package + ['pkgexports/belowfile', './belowfile'], + // Directory mappings require a trailing / to work + ['pkgexports/missingtrailer/x', './missingtrailer/x'], + // Invalid target handling + ['pkgexports/null', './null'], + ['pkgexports/invalid1', './invalid1'], + ['pkgexports/invalid2', './invalid2'], + ['pkgexports/invalid3', './invalid3'], + ['pkgexports/invalid4', './invalid4'], + // Missing / invalid fallbacks + ['pkgexports/nofallback1', './nofallback1'], + ['pkgexports/nofallback2', './nofallback2'], + ]); - // The file exists but isn't exported. The exports is a number which counts - // as a non-null value without any properties, just like `{}`. - loadFixture('pkgexports-number/hidden.js').catch(mustCall((err) => { - strictEqual(err.code, (isRequire ? '' : 'ERR_') + 'MODULE_NOT_FOUND'); - assertStartsWith(err.message, 'Package exports'); - assertIncludes(err.message, 'do not define a \'./hidden.js\' subpath'); - })); + for (const [specifier, subpath] of undefinedExports) { + loadFixture(specifier).catch(mustCall((err) => { + strictEqual(err.code, (isRequire ? '' : 'ERR_') + 'MODULE_NOT_FOUND'); + assertStartsWith(err.message, 'Package exports'); + assertIncludes(err.message, `do not define a '${subpath}' subpath`); + })); + } // There's no main field so we won't find anything when importing the name. // The fact that "." is mapped is ignored, it's not a valid main config. @@ -54,26 +77,19 @@ import { requireFixture, importFixture } from '../fixtures/pkgexports.mjs'; } })); - // Even though 'pkgexports/sub/asdf.js' works, alternate "path-like" variants - // do not to prevent confusion and accidental loopholes. - loadFixture('pkgexports/sub/./../asdf.js').catch(mustCall((err) => { + // Covering out bases - not a file is still not a file after dir mapping. + loadFixture('pkgexports/sub/not-a-file.js').catch(mustCall((err) => { strictEqual(err.code, (isRequire ? '' : 'ERR_') + 'MODULE_NOT_FOUND'); - assertStartsWith(err.message, 'Package exports'); - assertIncludes(err.message, - 'do not define a \'./sub/./../asdf.js\' subpath'); + // ESM returns a full file path + assertStartsWith(err.message, isRequire ? + 'Cannot find module \'pkgexports/sub/not-a-file.js\'' : + 'Cannot find module'); })); - // Covering out bases - not a file is still not a file after dir mapping. - loadFixture('pkgexports/sub/not-a-file.js').catch(mustCall((err) => { - if (isRequire) { - strictEqual(err.code, 'MODULE_NOT_FOUND'); - assertStartsWith(err.message, - 'Cannot find module \'pkgexports/sub/not-a-file.js\''); - } else { - strictEqual(err.code, 'ERR_MODULE_NOT_FOUND'); - // ESM currently returns a full file path - assertStartsWith(err.message, 'Cannot find module'); - } + // THe use of %2F escapes in paths fails loading + loadFixture('pkgexports/sub/..%2F..%2Fbar.js').catch(mustCall((err) => { + strictEqual(err.code, isRequire ? 'ERR_INVALID_FILE_URL_PATH' : + 'ERR_MODULE_NOT_FOUND'); })); }); diff --git a/test/fixtures/node_modules/pkgexports/package.json b/test/fixtures/node_modules/pkgexports/package.json index 97f07da85e2a61..fbc22db77b228c 100644 --- a/test/fixtures/node_modules/pkgexports/package.json +++ b/test/fixtures/node_modules/pkgexports/package.json @@ -3,6 +3,18 @@ ".": "./asdf.js", "./space": "./sp%20ce.js", "./valid-cjs": "./asdf.js", - "./sub/": "./" + "./sub/": "./", + "./belowdir/": "../belowdir/", + "./belowfile": "../belowfile", + "./missingtrailer/": ".", + "./null": null, + "./invalid1": {}, + "./invalid2": 1234, + "./invalid3": "", + "./invalid4": {}, + "./fallbackdir/": [[], null, {}, "builtin:x/", "./fallbackfile", "./"], + "./fallbackfile": [[], null, {}, "builtin:x", "./asdf.js"], + "./nofallback1": [], + "./nofallback2": [null, {}, "builtin:x"] } } From 3a1d4938862af6c88941e12a2d5c3e062774afb5 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Sat, 3 Aug 2019 20:39:43 -0400 Subject: [PATCH 02/11] feedback, ownproperty checks --- lib/internal/modules/cjs/loader.js | 5 +++-- src/module_wrap.cc | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index cae3f42a39910c..388da193cb4fae 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -24,6 +24,7 @@ const { JSON, Object, + ObjectPrototype, Reflect, SafeMap, StringPrototype, @@ -351,7 +352,7 @@ function resolveExports(nmPath, request, absoluteRequest) { const mappingKey = `.${expansion}`; if (typeof pkgExports === 'object' && pkgExports !== null) { - if (mappingKey in pkgExports) { + if (ObjectPrototype.hasOwnProperty(pkgExports, mappingKey)) { const mapping = pkgExports[mappingKey]; return resolveExportsTarget(pathToFileURL(basePath + '/'), mapping, '', basePath, mappingKey); @@ -387,7 +388,7 @@ function resolveExports(nmPath, request, absoluteRequest) { function resolveExportsTarget(pkgPath, target, subpath, basePath, mappingKey) { if (typeof target === 'string') { - if (target.substr(0, 2) === './' && + if (target.startsWith('./') && (subpath.length === 0 || target.endsWith('/'))) { const resolvedTarget = new URL(target, pkgPath); if (StringPrototype.startsWith(resolvedTarget.href, pkgPath.href)) { diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 88e262afc7fdbd..41d209d34f3ed7 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -854,7 +854,7 @@ Maybe PackageExportsResolve(Environment* env, Local subpath = String::NewFromUtf8(isolate, pkg_subpath.c_str(), v8::NewStringType::kNormal).ToLocalChecked(); - if (exports_obj->Has(context, subpath).FromJust()) { + if (exports_obj->HasOwnProperty(context, subpath).FromJust()) { Local target = exports_obj->Get(context, subpath).ToLocalChecked(); if (target->IsString()) { Utf8Value target_utf8(isolate, target.As()); From 7597165e428789a1455c71fe42d184dec0988da3 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Sat, 3 Aug 2019 23:09:56 -0400 Subject: [PATCH 03/11] node_modules filtering in exports and boundaries --- doc/api/esm.md | 13 ++++++--- lib/internal/modules/cjs/loader.js | 11 ++++++-- src/module_wrap.cc | 27 ++++++++++++------- test/es-module/test-esm-exports.mjs | 2 ++ .../es-module/test-esm-scope-node-modules.mjs | 10 +++++++ .../es-modules/package-type-module/index.js | 1 + .../node_modules/dep/dep.js | 2 ++ .../pkgexports/node_modules/internalpkg/x.js | 1 + .../node_modules/pkgexports/package.json | 3 ++- 9 files changed, 54 insertions(+), 16 deletions(-) create mode 100644 test/es-module/test-esm-scope-node-modules.mjs create mode 100644 test/fixtures/es-modules/package-type-module/node_modules/dep/dep.js create mode 100644 test/fixtures/node_modules/pkgexports/node_modules/internalpkg/x.js diff --git a/doc/api/esm.md b/doc/api/esm.md index 5653c85a21e945..f80fbd2987c610 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -787,15 +787,19 @@ _isMain_ is **true** when resolving the Node.js application entry point. **PACKAGE_EXPORTS_TARGET_RESOLVE**(_packageURL_, _target_, _subpath_) > 1. If _target_ is a String, then -> 1. If _target_ does not start with _"./"_, or _subpath_ has non-zero -> length and _target_ does not end with _"/"_, then -> 1. Throw a _Module Not Found_ error. +> 1. If _target_ does not start with _"./"_, throw a _Module Not Found_ +> error. +> 1. If _subpath_ has non-zero length or _target_ does not end with _"/"_, +> throw a _Module Not Found_ error. +> 1. If _target_ or _subpath_ contain any _"node_modules"_ segments including +> _"node_modules"_ percent-encoding, throw a _Module Not Found_ error. > 1. Let _resolvedTarget_ be the URL resolution of the concatenation of > _packageURL_ and _target_. > 1. If _resolvedTarget_ is contained in _packageURL_, then > 1. Let _resolved_ be the URL resolution of the concatenation of > _subpath_ and _resolvedTarget_. -> 1. If _resolved_ is contained in _packageURL_, then +> 1. If _resolved_ is contained in _packageURL_ and contains no +> _"node_modules"_ segments, then > 1. Return _resolved_. > 1. Otherwise, if _target_ is an Array, then > 1. For each item _targetValue_ in _target_, do @@ -827,6 +831,7 @@ _isMain_ is **true** when resolving the Node.js application entry point. **READ_PACKAGE_SCOPE**(_url_) > 1. Let _scopeURL_ be _url_. > 1. While _scopeURL_ is not the file system root, +> 1. If _scopeURL_ ends in a _"node_modules"_ path segment, return **null**. > 1. Let _pjson_ be the result of **READ_PACKAGE_JSON**(_scopeURL_). > 1. If _pjson_ is not **null**, then > 1. Return _pjson_. diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 388da193cb4fae..1781e978c55437 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -391,9 +391,16 @@ function resolveExportsTarget(pkgPath, target, subpath, basePath, mappingKey) { if (target.startsWith('./') && (subpath.length === 0 || target.endsWith('/'))) { const resolvedTarget = new URL(target, pkgPath); - if (StringPrototype.startsWith(resolvedTarget.href, pkgPath.href)) { + const pkgPathHref = pkgPath.href; + const resolvedTargetHref = resolvedTarget.href; + if (StringPrototype.startsWith(resolvedTargetHref, pkgPathHref) && + StringPrototype.indexOf(resolvedTargetHref, '/node_modules/', + pkgPathHref.length - 1) === -1) { const resolved = new URL(subpath, resolvedTarget); - if (StringPrototype.startsWith(resolved.href, pkgPath.href)) { + const resolvedHref = resolved.href; + if (StringPrototype.startsWith(resolvedHref, pkgPathHref) && + StringPrototype.indexOf(resolvedHref, '/node_modules/', + pkgPathHref.length - 1) === -1) { return fileURLToPath(resolved); } } diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 41d209d34f3ed7..85add62e2f354f 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -633,6 +633,12 @@ Maybe GetPackageScopeConfig(Environment* env, const URL& base) { URL pjson_url("./package.json", &resolved); while (true) { + std::string pjson_url_path = pjson_url.path(); + if (pjson_url_path.length() > 25 && + pjson_url_path.substr(pjson_url_path.length() - 25, 25) == + "node_modules/package.json") { + break; + } Maybe pkg_cfg = GetPackageConfig(env, pjson_url.ToFilePath(), base); if (pkg_cfg.IsNothing()) return pkg_cfg; @@ -643,14 +649,13 @@ Maybe GetPackageScopeConfig(Environment* env, // Terminates at root where ../package.json equals ../../package.json // (can't just check "/package.json" for Windows support). - if (pjson_url.path() == last_pjson_url.path()) { - auto entry = env->package_json_cache.emplace(pjson_url.ToFilePath(), - PackageConfig { Exists::No, IsValid::Yes, HasMain::No, "", - PackageType::None, Global() }); - const PackageConfig* pcfg = &entry.first->second; - return Just(pcfg); - } + if (pjson_url.path() == last_pjson_url.path()) break; } + auto entry = env->package_json_cache.emplace(pjson_url.ToFilePath(), + PackageConfig { Exists::No, IsValid::Yes, HasMain::No, "", + PackageType::None, Global() }); + const PackageConfig* pcfg = &entry.first->second; + return Just(pcfg); } /* @@ -824,11 +829,15 @@ Maybe ResolveExportsTarget(Environment* env, URL resolved(target, pjson_url); std::string resolved_path = resolved.path(); std::string pkg_path = URL(".", pjson_url).path(); - if (resolved_path.find(pkg_path) == 0) { + if (resolved_path.find(pkg_path) == 0 && + resolved_path.find("/node_modules/", pkg_path.length() - 1) == + std::string::npos) { if (subpath.length() == 0) return Just(resolved); URL subpath_resolved(subpath, resolved); std::string subpath_resolved_path = subpath_resolved.path(); - if (subpath_resolved_path.find(pkg_path) == 0) { + if (subpath_resolved_path.find(pkg_path) == 0 && + subpath_resolved_path.find("/node_modules/", pkg_path.length() - 1) + == std::string::npos) { return Just(subpath_resolved); } } diff --git a/test/es-module/test-esm-exports.mjs b/test/es-module/test-esm-exports.mjs index dd07b0ec7a51da..0266ccb0706699 100644 --- a/test/es-module/test-esm-exports.mjs +++ b/test/es-module/test-esm-exports.mjs @@ -55,6 +55,8 @@ import { requireFixture, importFixture } from '../fixtures/pkgexports.mjs'; // Missing / invalid fallbacks ['pkgexports/nofallback1', './nofallback1'], ['pkgexports/nofallback2', './nofallback2'], + // Reaching into nested node_modules + ['pkgexports/nodemodules', './nodemodules'], ]); for (const [specifier, subpath] of undefinedExports) { diff --git a/test/es-module/test-esm-scope-node-modules.mjs b/test/es-module/test-esm-scope-node-modules.mjs new file mode 100644 index 00000000000000..8358da5c765288 --- /dev/null +++ b/test/es-module/test-esm-scope-node-modules.mjs @@ -0,0 +1,10 @@ +// Flags: --experimental-modules +import '../common/index.mjs'; +import cjs from '../fixtures/baz.js'; +import { message } from '../fixtures/es-modules/message.mjs'; +import assert from 'assert'; + +// Assert we loaded esm dependency as ".js" in this mode +assert.strictEqual(message, 'A message'); +// Assert we loaded CommonJS dependency +assert.strictEqual(cjs, 'perhaps I work'); diff --git a/test/fixtures/es-modules/package-type-module/index.js b/test/fixtures/es-modules/package-type-module/index.js index 12aba970ef279f..e8f4db3e164302 100644 --- a/test/fixtures/es-modules/package-type-module/index.js +++ b/test/fixtures/es-modules/package-type-module/index.js @@ -1,3 +1,4 @@ +import 'dep/dep.js'; const identifier = 'package-type-module'; console.log(identifier); export default identifier; diff --git a/test/fixtures/es-modules/package-type-module/node_modules/dep/dep.js b/test/fixtures/es-modules/package-type-module/node_modules/dep/dep.js new file mode 100644 index 00000000000000..7b04e8b3808e04 --- /dev/null +++ b/test/fixtures/es-modules/package-type-module/node_modules/dep/dep.js @@ -0,0 +1,2 @@ +// No package.json -> should still be CommonJS as it is in node_modules +module.exports = 42; diff --git a/test/fixtures/node_modules/pkgexports/node_modules/internalpkg/x.js b/test/fixtures/node_modules/pkgexports/node_modules/internalpkg/x.js new file mode 100644 index 00000000000000..888cae37af95c5 --- /dev/null +++ b/test/fixtures/node_modules/pkgexports/node_modules/internalpkg/x.js @@ -0,0 +1 @@ +module.exports = 42; diff --git a/test/fixtures/node_modules/pkgexports/package.json b/test/fixtures/node_modules/pkgexports/package.json index fbc22db77b228c..2b190521e5f987 100644 --- a/test/fixtures/node_modules/pkgexports/package.json +++ b/test/fixtures/node_modules/pkgexports/package.json @@ -15,6 +15,7 @@ "./fallbackdir/": [[], null, {}, "builtin:x/", "./fallbackfile", "./"], "./fallbackfile": [[], null, {}, "builtin:x", "./asdf.js"], "./nofallback1": [], - "./nofallback2": [null, {}, "builtin:x"] + "./nofallback2": [null, {}, "builtin:x"], + "./nodemodules": "./node_modules/internalpkg/x.js" } } From 6cdcf6c084f4f23bb60658537c694749edd69cbb Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Sat, 3 Aug 2019 23:11:21 -0400 Subject: [PATCH 04/11] linting fixes --- lib/internal/modules/cjs/loader.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 1781e978c55437..a697892c139927 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -395,12 +395,12 @@ function resolveExportsTarget(pkgPath, target, subpath, basePath, mappingKey) { const resolvedTargetHref = resolvedTarget.href; if (StringPrototype.startsWith(resolvedTargetHref, pkgPathHref) && StringPrototype.indexOf(resolvedTargetHref, '/node_modules/', - pkgPathHref.length - 1) === -1) { + pkgPathHref.length - 1) === -1) { const resolved = new URL(subpath, resolvedTarget); const resolvedHref = resolved.href; if (StringPrototype.startsWith(resolvedHref, pkgPathHref) && StringPrototype.indexOf(resolvedHref, '/node_modules/', - pkgPathHref.length - 1) === -1) { + pkgPathHref.length - 1) === -1) { return fileURLToPath(resolved); } } From 20f06d8d72c6dada5cec4cc3e3ba9c73a9975c39 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Sun, 4 Aug 2019 20:50:25 -0400 Subject: [PATCH 05/11] Fixup backslash escaping Co-Authored-By: Vse Mozhet Byt --- doc/api/esm.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/esm.md b/doc/api/esm.md index f80fbd2987c610..c9e7202d37006d 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -697,7 +697,7 @@ _isMain_ is **true** when resolving the Node.js application entry point. > 1. Note: _specifier_ is now a bare specifier. > 1. Set _resolvedURL_ the result of > **PACKAGE_RESOLVE**(_specifier_, _parentURL_). -> 1. If _resolvedURL_ contains any percent encodings of _"/"_ or _"\"_ (_"%2f"_ +> 1. If _resolvedURL_ contains any percent encodings of _"/"_ or _"\\"_ (_"%2f"_ > and _"%5C"_ respectively), then > 1. Throw an _Invalid Specifier_ error. > 1. If the file at _resolvedURL_ does not exist, then From 110fa261f0e29edc0171e46413ea9851d34f78c2 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Sun, 4 Aug 2019 20:50:38 -0400 Subject: [PATCH 06/11] Closing bracket Co-Authored-By: Vse Mozhet Byt --- doc/api/esm.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/esm.md b/doc/api/esm.md index c9e7202d37006d..bf9815546cba6d 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -782,7 +782,7 @@ _isMain_ is **true** when resolving the Node.js application entry point. > 1. Let _subpath_ be the substring of _target_ starting at the index > of the length of _directory_. > 1. Return **PACKAGE_EXPORTS_TARGET_RESOLVE**(_packageURL_, _target_, -> _subpath_. +> _subpath_). > 1. Throw a _Module Not Found_ error. **PACKAGE_EXPORTS_TARGET_RESOLVE**(_packageURL_, _target_, _subpath_) From 406cb0209f0934f6880d51c2e51d555b03b617a4 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Sun, 4 Aug 2019 21:09:07 -0400 Subject: [PATCH 07/11] fixup double node_modules check --- doc/api/esm.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/doc/api/esm.md b/doc/api/esm.md index bf9815546cba6d..c0bc888d025234 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -798,8 +798,7 @@ _isMain_ is **true** when resolving the Node.js application entry point. > 1. If _resolvedTarget_ is contained in _packageURL_, then > 1. Let _resolved_ be the URL resolution of the concatenation of > _subpath_ and _resolvedTarget_. -> 1. If _resolved_ is contained in _packageURL_ and contains no -> _"node_modules"_ segments, then +> 1. If _resolved_ is contained in _packageURL_, then > 1. Return _resolved_. > 1. Otherwise, if _target_ is an Array, then > 1. For each item _targetValue_ in _target_, do From 9d95b1da4eeb6c6455d69bdc989c25c102dda5a5 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Mon, 5 Aug 2019 22:10:16 -0400 Subject: [PATCH 08/11] pr feedback - validations on paths --- lib/internal/modules/cjs/loader.js | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index a697892c139927..53994f6189cb56 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -391,16 +391,16 @@ function resolveExportsTarget(pkgPath, target, subpath, basePath, mappingKey) { if (target.startsWith('./') && (subpath.length === 0 || target.endsWith('/'))) { const resolvedTarget = new URL(target, pkgPath); - const pkgPathHref = pkgPath.href; - const resolvedTargetHref = resolvedTarget.href; - if (StringPrototype.startsWith(resolvedTargetHref, pkgPathHref) && - StringPrototype.indexOf(resolvedTargetHref, '/node_modules/', - pkgPathHref.length - 1) === -1) { + const pkgPathPath = pkgPath.pathname; + const resolvedTargetPath = resolvedTarget.pathname; + if (StringPrototype.startsWith(resolvedTargetPath, pkgPathPath) && + StringPrototype.indexOf(resolvedTargetPath, '/node_modules/', + pkgPathPath.length - 1) === -1) { const resolved = new URL(subpath, resolvedTarget); - const resolvedHref = resolved.href; - if (StringPrototype.startsWith(resolvedHref, pkgPathHref) && - StringPrototype.indexOf(resolvedHref, '/node_modules/', - pkgPathHref.length - 1) === -1) { + const resolvedPath = resolved.pathname; + if (StringPrototype.startsWith(resolvedPath, pkgPathPath) && + StringPrototype.indexOf(resolvedPath, '/node_modules/', + pkgPathPath.length - 1) === -1) { return fileURLToPath(resolved); } } From a7e3e76762b6fe46846c9f5c04489fdc90df1cc8 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Mon, 5 Aug 2019 21:49:31 -0400 Subject: [PATCH 09/11] improve validation failure errors --- lib/internal/modules/cjs/loader.js | 4 +- src/module_wrap.cc | 153 +++++++++++++++++++++------- test/es-module/test-esm-exports.mjs | 14 +++ 3 files changed, 130 insertions(+), 41 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 53994f6189cb56..ad64e9b2939c05 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -417,8 +417,8 @@ function resolveExportsTarget(pkgPath, target, subpath, basePath, mappingKey) { } } // eslint-disable-next-line no-restricted-syntax - const e = new Error(`Package exports for '${basePath}' do not define ` + - `a '${mappingKey}' subpath`); + const e = new Error(`Package exports for '${basePath}' do not define a ` + + `valid '${mappingKey}' target${subpath ? 'for ' + subpath : ''}`); e.code = 'MODULE_NOT_FOUND'; throw e; } diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 85add62e2f354f..5892bdba4ca8b9 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -545,8 +545,8 @@ Maybe GetPackageConfig(Environment* env, if (existing != env->package_json_cache.end()) { const PackageConfig* pcfg = &existing->second; if (pcfg->is_valid == IsValid::No) { - std::string msg = "Invalid JSON in '" + path + - "' imported from " + base.ToFilePath(); + std::string msg = "Invalid JSON in " + path + + " imported from " + base.ToFilePath(); node::THROW_ERR_INVALID_PACKAGE_CONFIG(env, msg.c_str()); return Nothing(); } @@ -579,8 +579,8 @@ Maybe GetPackageConfig(Environment* env, env->package_json_cache.emplace(path, PackageConfig { Exists::Yes, IsValid::No, HasMain::No, "", PackageType::None, Global() }); - std::string msg = "Invalid JSON in '" + path + - "' imported from " + base.ToFilePath(); + std::string msg = "Invalid JSON in " + path + + " imported from " + base.ToFilePath(); node::THROW_ERR_INVALID_PACKAGE_CONFIG(env, msg.c_str()); return Nothing(); } @@ -755,17 +755,17 @@ Maybe FinalizeResolution(Environment* env, if (!file.IsNothing()) { return file; } - std::string msg = "Cannot find module '" + resolved.path() + - "' imported from " + base.ToFilePath(); + std::string msg = "Cannot find module " + resolved.path() + + " imported from " + base.ToFilePath(); node::THROW_ERR_MODULE_NOT_FOUND(env, msg.c_str()); return Nothing(); } const std::string& path = resolved.ToFilePath(); if (CheckDescriptorAtPath(path) != FILE) { - std::string msg = "Cannot find module '" + + std::string msg = "Cannot find module " + (path.length() != 0 ? path : resolved.path()) + - "' imported from " + base.ToFilePath(); + " imported from " + base.ToFilePath(); node::THROW_ERR_MODULE_NOT_FOUND(env, msg.c_str()); return Nothing(); } @@ -799,51 +799,92 @@ Maybe PackageMainResolve(Environment* env, } } } - std::string msg = "Cannot find main entry point for '" + - URL(".", pjson_url).ToFilePath() + "' imported from " + + std::string msg = "Cannot find main entry point for " + + URL(".", pjson_url).ToFilePath() + " imported from " + base.ToFilePath(); node::THROW_ERR_MODULE_NOT_FOUND(env, msg.c_str()); return Nothing(); } void ThrowExportsNotFound(Environment* env, - const std::string& match, + const std::string& subpath, const URL& pjson_url, const URL& base) { - const std::string msg = "Package exports for '" + - URL(".", pjson_url).ToFilePath() + "' do not define a '" + match + + const std::string msg = "Package exports for " + + pjson_url.ToFilePath() + " do not define a '" + subpath + "' subpath, imported from " + base.ToFilePath(); node::THROW_ERR_MODULE_NOT_FOUND(env, msg.c_str()); } +void ThrowExportsInvalid(Environment* env, + const std::string& subpath, + const std::string& target, + const URL& pjson_url, + const URL& base) { + const std::string msg = "Cannot resolve package exports target '" + target + + "' matched for '" + subpath + "' in " + pjson_url.ToFilePath() + + ", imported from " + base.ToFilePath(); + node::THROW_ERR_MODULE_NOT_FOUND(env, msg.c_str()); +} + +void ThrowExportsInvalid(Environment* env, + const std::string& subpath, + Local target, + const URL& pjson_url, + const URL& base) { + Local target_string; + if (target->ToString(env->context()).ToLocal(&target_string)) { + Utf8Value target_utf8(env->isolate(), target_string); + std::string target_str(*target_utf8, target_utf8.length()); + if (target->IsArray()) { + target_str = '[' + target_str + ']'; + } + ThrowExportsInvalid(env, subpath, target_str, pjson_url, base); + } +} + Maybe ResolveExportsTarget(Environment* env, const std::string& target, const std::string& subpath, const std::string& match, const URL& pjson_url, - const URL& base) { - if (target.substr(0, 2) == "./") { - if (subpath.length() > 0 && target.back() != '/') { - return Nothing(); + const URL& base, + bool throw_invalid = true) { + if (target.substr(0, 2) != "./") { + if (throw_invalid) { + ThrowExportsInvalid(env, match, target, pjson_url, base); } - URL resolved(target, pjson_url); - std::string resolved_path = resolved.path(); - std::string pkg_path = URL(".", pjson_url).path(); - if (resolved_path.find(pkg_path) == 0 && - resolved_path.find("/node_modules/", pkg_path.length() - 1) == - std::string::npos) { - if (subpath.length() == 0) return Just(resolved); - URL subpath_resolved(subpath, resolved); - std::string subpath_resolved_path = subpath_resolved.path(); - if (subpath_resolved_path.find(pkg_path) == 0 && - subpath_resolved_path.find("/node_modules/", pkg_path.length() - 1) - == std::string::npos) { - return Just(subpath_resolved); - } + return Nothing(); + } + if (subpath.length() > 0 && target.back() != '/') { + if (throw_invalid) { + ThrowExportsInvalid(env, match, target, pjson_url, base); } + return Nothing(); } - // Note: std: / nodejs: support goes here - return Nothing(); + URL resolved(target, pjson_url); + std::string resolved_path = resolved.path(); + std::string pkg_path = URL(".", pjson_url).path(); + if (resolved_path.find(pkg_path) != 0 || + resolved_path.find("/node_modules/", pkg_path.length() - 1) != + std::string::npos) { + if (throw_invalid) { + ThrowExportsInvalid(env, match, target, pjson_url, base); + } + return Nothing(); + } + if (subpath.length() == 0) return Just(resolved); + URL subpath_resolved(subpath, resolved); + std::string subpath_resolved_path = subpath_resolved.path(); + if (subpath_resolved_path.find(pkg_path) != 0 || + subpath_resolved_path.find("/node_modules/", pkg_path.length() - 1) + != std::string::npos) { + if (throw_invalid) { + ThrowExportsInvalid(env, match, target + subpath, pjson_url, base); + } + return Nothing(); + } + return Just(subpath_resolved); } Maybe PackageExportsResolve(Environment* env, @@ -871,27 +912,43 @@ Maybe PackageExportsResolve(Environment* env, Maybe resolved = ResolveExportsTarget(env, target_str, "", pkg_subpath, pjson_url, base); if (resolved.IsNothing()) { - ThrowExportsNotFound(env, pkg_subpath, pjson_url, base); + ThrowExportsInvalid(env, pkg_subpath, target, pjson_url, base); return Nothing(); } return FinalizeResolution(env, resolved.FromJust(), base); } else if (target->IsArray()) { Local target_arr = target.As(); const uint32_t length = target_arr->Length(); + if (length == 0) { + ThrowExportsInvalid(env, pkg_subpath, target, pjson_url, base); + return Nothing(); + } for (uint32_t i = 0; i < length; i++) { auto target_item = target_arr->Get(context, i).ToLocalChecked(); if (target_item->IsString()) { Utf8Value target_utf8(isolate, target_item.As()); std::string target(*target_utf8, target_utf8.length()); Maybe resolved = ResolveExportsTarget(env, target, "", - pkg_subpath, pjson_url, base); + pkg_subpath, pjson_url, base, false); if (resolved.IsNothing()) continue; return FinalizeResolution(env, resolved.FromJust(), base); } } + auto invalid = target_arr->Get(context, length - 1).ToLocalChecked(); + if (!invalid->IsString()) { + ThrowExportsInvalid(env, pkg_subpath, invalid, pjson_url, base); + return Nothing(); + } + Utf8Value invalid_utf8(isolate, invalid.As()); + std::string invalid_str(*invalid_utf8, invalid_utf8.length()); + Maybe resolved = ResolveExportsTarget(env, invalid_str, "", + pkg_subpath, pjson_url, base); + CHECK(resolved.IsNothing()); + return Nothing(); + } else { + ThrowExportsInvalid(env, pkg_subpath, target, pjson_url, base); + return Nothing(); } - ThrowExportsNotFound(env, pkg_subpath, pjson_url, base); - return Nothing(); } Local best_match; @@ -919,24 +976,42 @@ Maybe PackageExportsResolve(Environment* env, Maybe resolved = ResolveExportsTarget(env, target, subpath, pkg_subpath, pjson_url, base); if (resolved.IsNothing()) { - ThrowExportsNotFound(env, pkg_subpath, pjson_url, base); + ThrowExportsInvalid(env, pkg_subpath, target, pjson_url, base); return Nothing(); } return FinalizeResolution(env, URL(subpath, resolved.FromJust()), base); } else if (target->IsArray()) { Local target_arr = target.As(); const uint32_t length = target_arr->Length(); + if (length == 0) { + ThrowExportsInvalid(env, pkg_subpath, target, pjson_url, base); + return Nothing(); + } for (uint32_t i = 0; i < length; i++) { auto target_item = target_arr->Get(context, i).ToLocalChecked(); if (target_item->IsString()) { Utf8Value target_utf8(isolate, target_item.As()); std::string target_str(*target_utf8, target_utf8.length()); Maybe resolved = ResolveExportsTarget(env, target_str, subpath, - pkg_subpath, pjson_url, base); + pkg_subpath, pjson_url, base, false); if (resolved.IsNothing()) continue; return FinalizeResolution(env, resolved.FromJust(), base); } } + auto invalid = target_arr->Get(context, length - 1).ToLocalChecked(); + if (!invalid->IsString()) { + ThrowExportsInvalid(env, pkg_subpath, invalid, pjson_url, base); + return Nothing(); + } + Utf8Value invalid_utf8(isolate, invalid.As()); + std::string invalid_str(*invalid_utf8, invalid_utf8.length()); + Maybe resolved = ResolveExportsTarget(env, invalid_str, subpath, + pkg_subpath, pjson_url, base); + CHECK(resolved.IsNothing()); + return Nothing(); + } else { + ThrowExportsInvalid(env, pkg_subpath, target, pjson_url, base); + return Nothing(); } } diff --git a/test/es-module/test-esm-exports.mjs b/test/es-module/test-esm-exports.mjs index 0266ccb0706699..4f772521588898 100644 --- a/test/es-module/test-esm-exports.mjs +++ b/test/es-module/test-esm-exports.mjs @@ -36,6 +36,9 @@ import { requireFixture, importFixture } from '../fixtures/pkgexports.mjs'; // The file exists but isn't exported. The exports is a number which counts // as a non-null value without any properties, just like `{}`. ['pkgexports-number/hidden.js', './hidden.js'], + ]); + + const invalidExports = new Map([ // Even though 'pkgexports/sub/asdf.js' works, alternate "path-like" // variants do not to prevent confusion and accidental loopholes. ['pkgexports/sub/./../asdf.js', './sub/./../asdf.js'], @@ -67,6 +70,17 @@ import { requireFixture, importFixture } from '../fixtures/pkgexports.mjs'; })); } + for (const [specifier, subpath] of invalidExports) { + loadFixture(specifier).catch(mustCall((err) => { + strictEqual(err.code, (isRequire ? '' : 'ERR_') + 'MODULE_NOT_FOUND'); + assertStartsWith(err.message, (isRequire ? 'Package exports' : + 'Cannot resolve')); + assertIncludes(err.message, isRequire ? + `do not define a valid '${subpath}' subpath` : + `matched for '${subpath}'`); + })); + } + // There's no main field so we won't find anything when importing the name. // The fact that "." is mapped is ignored, it's not a valid main config. loadFixture('pkgexports').catch(mustCall((err) => { From 0274de77f5beb3245c343d3cd27708403a747a54 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Tue, 6 Aug 2019 23:40:15 -0400 Subject: [PATCH 10/11] or -> and spec fixup --- doc/api/esm.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/esm.md b/doc/api/esm.md index c0bc888d025234..1e7f729d45fd30 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -789,7 +789,7 @@ _isMain_ is **true** when resolving the Node.js application entry point. > 1. If _target_ is a String, then > 1. If _target_ does not start with _"./"_, throw a _Module Not Found_ > error. -> 1. If _subpath_ has non-zero length or _target_ does not end with _"/"_, +> 1. If _subpath_ has non-zero length and _target_ does not end with _"/"_, > throw a _Module Not Found_ error. > 1. If _target_ or _subpath_ contain any _"node_modules"_ segments including > _"node_modules"_ percent-encoding, throw a _Module Not Found_ error. From 52fb8462ad9feb8f4bb67ba478eed56d863b8332 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Sun, 11 Aug 2019 02:25:44 -0400 Subject: [PATCH 11/11] ensure target dir mappings remain in target dir --- doc/api/esm.md | 6 ++---- lib/internal/modules/cjs/loader.js | 2 +- src/module_wrap.cc | 2 +- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/doc/api/esm.md b/doc/api/esm.md index 1e7f729d45fd30..ac1f154ff3e540 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -754,7 +754,7 @@ _isMain_ is **true** when resolving the Node.js application entry point. > 1. If _pjson_ is **null**, then > 1. Throw a _Module Not Found_ error. > 1. If _pjson.main_ is a String, then -> 1. Let _resolvedMain_ be the concatenation of _packageURL_, "/", and +> 1. Let _resolvedMain_ be the URL resolution of _packageURL_, "/", and > _pjson.main_. > 1. If the file at _resolvedMain_ exists, then > 1. Return _resolvedMain_. @@ -763,8 +763,6 @@ _isMain_ is **true** when resolving the Node.js application entry point. > 1. Let _legacyMainURL_ be the result applying the legacy > **LOAD_AS_DIRECTORY** CommonJS resolver to _packageURL_, throwing a > _Module Not Found_ error for no resolution. -> 1. If _legacyMainURL_ does not end in _".js"_ then, -> 1. Throw an _Unsupported File Extension_ error. > 1. Return _legacyMainURL_. **PACKAGE_EXPORTS_RESOLVE**(_packageURL_, _packagePath_, _exports_) @@ -798,7 +796,7 @@ _isMain_ is **true** when resolving the Node.js application entry point. > 1. If _resolvedTarget_ is contained in _packageURL_, then > 1. Let _resolved_ be the URL resolution of the concatenation of > _subpath_ and _resolvedTarget_. -> 1. If _resolved_ is contained in _packageURL_, then +> 1. If _resolved_ is contained in _resolvedTarget_, then > 1. Return _resolved_. > 1. Otherwise, if _target_ is an Array, then > 1. For each item _targetValue_ in _target_, do diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index ad64e9b2939c05..0ea8a46855e68a 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -398,7 +398,7 @@ function resolveExportsTarget(pkgPath, target, subpath, basePath, mappingKey) { pkgPathPath.length - 1) === -1) { const resolved = new URL(subpath, resolvedTarget); const resolvedPath = resolved.pathname; - if (StringPrototype.startsWith(resolvedPath, pkgPathPath) && + if (StringPrototype.startsWith(resolvedPath, resolvedTargetPath) && StringPrototype.indexOf(resolvedPath, '/node_modules/', pkgPathPath.length - 1) === -1) { return fileURLToPath(resolved); diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 5892bdba4ca8b9..83cc9863a62553 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -876,7 +876,7 @@ Maybe ResolveExportsTarget(Environment* env, if (subpath.length() == 0) return Just(resolved); URL subpath_resolved(subpath, resolved); std::string subpath_resolved_path = subpath_resolved.path(); - if (subpath_resolved_path.find(pkg_path) != 0 || + if (subpath_resolved_path.find(resolved_path) != 0 || subpath_resolved_path.find("/node_modules/", pkg_path.length() - 1) != std::string::npos) { if (throw_invalid) {