From 0e03c449e35e4951e9e9c962ff279ec271e62010 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Mon, 5 Aug 2019 02:24:54 -0400 Subject: [PATCH] module: refine package name validation PR-URL: https://github.com/nodejs/node/pull/28965 Reviewed-By: Anna Henningsen Reviewed-By: Jan Krems Reviewed-By: Rich Trott --- doc/api/esm.md | 20 ++++++++++--------- lib/internal/modules/cjs/loader.js | 2 +- src/module_wrap.cc | 31 +++++++++++++++++++++-------- test/es-module/test-esm-pkgname.mjs | 18 +++++++++++++++++ 4 files changed, 53 insertions(+), 18 deletions(-) create mode 100644 test/es-module/test-esm-pkgname.mjs diff --git a/doc/api/esm.md b/doc/api/esm.md index 07a3a71e90f0fd..79da763e8f79c1 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -700,15 +700,17 @@ _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. 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. If _packageName_ starts with _"."_ or contains _"\\"_ or _"%"_, then +> 1. Throw an _Invalid Specifier_ error. +> 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, @@ -719,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, @@ -727,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_) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 95b56e08520a52..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 5b33ef261cf69c..b3d0c306c91922 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -867,20 +867,35 @@ 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); + } + } 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 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]; + 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'); +}));