nodejs · guybedford · Aug 5, 2019 · Aug 6, 2019 · Aug 6, 2019 · hybrist
diff --git 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,16 +721,16 @@ _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,
 >       1. If _pjson_ is not **null** and _pjson_ has an _"exports"_ key, then
 >          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
@@ -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
@@ -867,20 +867,35 @@ Maybe<URL> 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<URL>();
-  }
+  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<URL>();
+  }
   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
@@ -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');
+}));