From 4aaf0858b377fca0e17e662fcfcb89742614e2cf Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Mon, 5 Aug 2019 02:24:54 -0400 Subject: [PATCH 1/3] module: refine package name validation --- lib/internal/modules/cjs/loader.js | 2 +- src/module_wrap.cc | 29 +++++++++++++++++++++-------- test/es-module/test-esm-pkgname.mjs | 18 ++++++++++++++++++ 3 files changed, 40 insertions(+), 9 deletions(-) create mode 100644 test/es-module/test-esm-pkgname.mjs diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 95b56e08520a52..9f5dfe98e3fa76 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -336,7 +336,7 @@ function findLongestRegisteredExtension(filename) { // This only applies to requests of a specific form: // 1. name/.* // 2. @scope/name/.* -const EXPORTS_PATTERN = /^((?:@[^./@\\][^/@\\]*\/)?[^@./\\][^/\\]*)(\/.*)$/; +const EXPORTS_PATTERN = /^((?:@[^@/\\%][^@/\\%]*\/)?[^@/\\%]+)(\/.*)$/; function resolveExports(nmPath, request, absoluteRequest) { // The implementation's behavior is meant to mirror resolution in ESM. if (experimentalExports && !absoluteRequest) { diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 5b33ef261cf69c..d4b09fa4464211 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -867,20 +867,33 @@ Maybe PackageResolve(Environment* env, const std::string& specifier, const URL& base) { size_t sep_index = specifier.find('/'); - if (specifier[0] == '@' && (sep_index == std::string::npos || - specifier.length() == 0)) { - std::string msg = "Invalid package name '" + specifier + - "' imported from " + base.ToFilePath(); - node::THROW_ERR_INVALID_MODULE_SPECIFIER(env, msg.c_str()); - return Nothing(); - } + bool valid_package_name = true; bool scope = false; if (specifier[0] == '@') { scope = true; - sep_index = specifier.find('/', sep_index + 1); + if (sep_index == std::string::npos || specifier.length() == 0) { + valid_package_name = false; + } else { + sep_index = specifier.find('/', sep_index + 1); + } } std::string pkg_name = specifier.substr(0, sep_index == std::string::npos ? std::string::npos : sep_index); + // Package name can only have leading @, and cannot have percent-encoding or + // separators. + for (size_t i = 0; i < pkg_name.length(); i++) { + char c = pkg_name[i]; + if (c == '%' || c == '\\') { + valid_package_name = false; + break; + } + } + if (!valid_package_name) { + std::string msg = "Invalid package name '" + specifier + + "' imported from " + base.ToFilePath(); + node::THROW_ERR_INVALID_MODULE_SPECIFIER(env, msg.c_str()); + return Nothing(); + } std::string pkg_subpath; if ((sep_index == std::string::npos || sep_index == specifier.length() - 1)) { diff --git a/test/es-module/test-esm-pkgname.mjs b/test/es-module/test-esm-pkgname.mjs new file mode 100644 index 00000000000000..046a12dd1a12da --- /dev/null +++ b/test/es-module/test-esm-pkgname.mjs @@ -0,0 +1,18 @@ +// Flags: --experimental-modules + +import { mustCall } from '../common/index.mjs'; +import { strictEqual } from 'assert'; + +import { importFixture } from '../fixtures/pkgexports.mjs'; + +importFixture('as%2Ff').catch(mustCall((err) => { + strictEqual(err.code, 'ERR_INVALID_MODULE_SPECIFIER'); +})); + +importFixture('as\\df').catch(mustCall((err) => { + strictEqual(err.code, 'ERR_INVALID_MODULE_SPECIFIER'); +})); + +importFixture('@as@df').catch(mustCall((err) => { + strictEqual(err.code, 'ERR_INVALID_MODULE_SPECIFIER'); +})); From 7bbe46635357567ee282dc0dd3d4e31207f80579 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Mon, 5 Aug 2019 20:55:53 -0400 Subject: [PATCH 2/3] pr feedback --- doc/api/esm.md | 2 ++ lib/internal/modules/cjs/loader.js | 2 +- src/module_wrap.cc | 4 +++- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/doc/api/esm.md b/doc/api/esm.md index 07a3a71e90f0fd..248619b38008d3 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -700,6 +700,8 @@ _isMain_ is **true** when resolving the Node.js application entry point. > 1. Throw an _Invalid Specifier_ error. > 1. Set _packageName_ to the substring of _packageSpecifier_ > until the second _"/"_ separator or the end of the string. +> 1. If _packageName_ starts with _"."_ or contains _"\\"_ or _"%"_, then +> 1. Throw an _Invalid Specifier_ error. > 1. Let _packageSubpath_ be the substring of _packageSpecifier_ from the > position at the length of _packageName_ plus one, if any. > 1. Assert: _packageName_ is a valid package name or scoped package name. diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 9f5dfe98e3fa76..9332c6c27793fd 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -336,7 +336,7 @@ function findLongestRegisteredExtension(filename) { // This only applies to requests of a specific form: // 1. name/.* // 2. @scope/name/.* -const EXPORTS_PATTERN = /^((?:@[^@/\\%][^@/\\%]*\/)?[^@/\\%]+)(\/.*)$/; +const EXPORTS_PATTERN = /^((?:@[^/\\%]+\/)?[^./\\%][^/\\%]*)(\/.*)$/; function resolveExports(nmPath, request, absoluteRequest) { // The implementation's behavior is meant to mirror resolution in ESM. if (experimentalExports && !absoluteRequest) { diff --git a/src/module_wrap.cc b/src/module_wrap.cc index d4b09fa4464211..b3d0c306c91922 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -876,10 +876,12 @@ Maybe PackageResolve(Environment* env, } else { sep_index = specifier.find('/', sep_index + 1); } + } else if (specifier[0] == '.') { + valid_package_name = false; } std::string pkg_name = specifier.substr(0, sep_index == std::string::npos ? std::string::npos : sep_index); - // Package name can only have leading @, and cannot have percent-encoding or + // Package name cannot have leading . and cannot have percent-encoding or // separators. for (size_t i = 0; i < pkg_name.length(); i++) { char c = pkg_name[i]; From b5aaeaf1fb0530acf7f47ffa416c4b15c9548750 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Tue, 6 Aug 2019 05:41:53 -0400 Subject: [PATCH 3/3] fixup packageSubpath validations --- doc/api/esm.md | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/doc/api/esm.md b/doc/api/esm.md index 248619b38008d3..79da763e8f79c1 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -702,15 +702,15 @@ _isMain_ is **true** when resolving the Node.js application entry point. > until the second _"/"_ separator or the end of the string. > 1. If _packageName_ starts with _"."_ or contains _"\\"_ or _"%"_, then > 1. Throw an _Invalid Specifier_ error. -> 1. Let _packageSubpath_ be the substring of _packageSpecifier_ from the -> position at the length of _packageName_ plus one, if any. -> 1. Assert: _packageName_ is a valid package name or scoped package name. -> 1. Assert: _packageSubpath_ is either empty, or a path without a leading -> separator. +> 1. Let _packageSubpath_ be _undefined_. +> 1. If the length of _packageSpecifier_ is greater than the length of +> _packageName_, then +> 1. Set _packageSubpath_ to _"."_ concatenated with the substring of +> _packageSpecifier_ from the position at the length of _packageName_. > 1. If _packageSubpath_ contains any _"."_ or _".."_ segments or percent > encoded strings for _"/"_ or _"\\"_ then, > 1. Throw an _Invalid Specifier_ error. -> 1. If _packageSubpath_ is empty and _packageName_ is a Node.js builtin +> 1. If _packageSubpath_ is _undefined_ and _packageName_ is a Node.js builtin > module, then > 1. Return the string _"node:"_ concatenated with _packageSpecifier_. > 1. While _parentURL_ is not the file system root, @@ -721,7 +721,7 @@ _isMain_ is **true** when resolving the Node.js application entry point. > 1. Set _parentURL_ to the parent URL path of _parentURL_. > 1. Continue the next loop iteration. > 1. Let _pjson_ be the result of **READ_PACKAGE_JSON**(_packageURL_). -> 1. If _packageSubpath_ is empty, then +> 1. If _packageSubpath_ is _undefined__, then > 1. Return the result of **PACKAGE_MAIN_RESOLVE**(_packageURL_, > _pjson_). > 1. Otherwise, @@ -729,8 +729,8 @@ _isMain_ is **true** when resolving the Node.js application entry point. > 1. Let _exports_ be _pjson.exports_. > 1. If _exports_ is not **null** or **undefined**, then > 1. Return **PACKAGE_EXPORTS_RESOLVE**(_packageURL_, -> _packagePath_, _pjson.exports_). -> 1. Return the URL resolution of _packagePath_ in _packageURL_. +> _packageSubpath_, _pjson.exports_). +> 1. Return the URL resolution of _packageSubpath_ in _packageURL_. > 1. Throw a _Module Not Found_ error. **PACKAGE_MAIN_RESOLVE**(_packageURL_, _pjson_)