Skip to content

Refine package name validations in esm resolver #28965

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.

Sign up for GitHub

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

Closed
guybedford wants to merge 3 commits into from
Closed

Refine package name validations in esm resolver #28965

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4aaf085
module: refine package name validation
guybedford Aug 5, 2019
7bbe466
pr feedback
guybedford Aug 6, 2019
b5aaeaf
fixup packageSubpath validations
guybedford Aug 6, 2019
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
2 changes: 2 additions & 0 deletions doc/api/esm.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/modules/cjs/loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
31 changes: 23 additions & 8 deletions src/module_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down
18 changes: 18 additions & 0 deletions test/es-module/test-esm-pkgname.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Flags: --experimental-modules
Copy link
Contributor

@hybrist hybrist Aug 5, 2019

Choose a reason for hiding this comment

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

Since this gets missed now, it may be good to add a test for exports that ensures that they aren't applied for exactly the case of ../hidden.js even if the parent directory happens to have a package.json file that doesn't export hidden.js.

Copy link
Contributor Author

@guybedford guybedford Aug 6, 2019

Choose a reason for hiding this comment

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

I'm not sure I follow this point or how it relates to this PR, can you clarify which test file changes you are referring to?

Copy link
Contributor

@hybrist hybrist Aug 6, 2019

Choose a reason for hiding this comment

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

Not necessary as part of this PR. More a "oops, guess we never had a regression test covering this".

Copy link
Contributor Author

@guybedford guybedford Aug 6, 2019

Choose a reason for hiding this comment

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

I still don't understand the exact regression / issue?

Copy link
Contributor

@hybrist hybrist Aug 6, 2019

Choose a reason for hiding this comment

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

We started treating ../ as a package name and would've loaded the export map etc. (I assume) but none of our tests broke. At least that's my current understanding. In other words: We don't have the following test right now:

// test/fixtures/node_modules/pkg-exports/lib/a.mjs
import '../hidden.js'; // should succeed but would be broken if ../package.json exports is applied

Copy link
Contributor Author

@guybedford guybedford Aug 6, 2019

Choose a reason for hiding this comment

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

Oh I see. But we don't get this far here because plain specifier detection doesn't catch ./, ../, / or URLs.

Copy link
Contributor

@hybrist hybrist Aug 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, may have lost track of that. Added a quick note to the exports project board just in case. This PR may not have broken it but I'd feel better if we had a test to ensure it in the future as well. :)


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');
}));