Skip to content
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.

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

Refine package name validations in esm resolver #28965

Closed
wants to merge 3 commits into from

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Aug 5, 2019

This ensures that the way package name parsing is done in the ESM resolver is consistent and well-defined based on the rules:

  1. Package name must start with @ and contain a separator, or not start with @ and contain no separator.
  2. Package names cannot contain percent encodings
  3. Package names cannot contain backslashes
  4. Package names cannot start with .

This also gives us consistency when dealing with package names between the CJS and ESM exports handling.

Effectively we can then think of import 'x/y' as having the package name only being allowed to exist as a valid package name, while the ./y component can in theory exist in URL space with percent encodings support.

If package names want to contain non-standard characters then they must be valid file paths, and not URL percent-encoded.

npm already restricts package names effectively to /^(@[-a-zA-Z\d][-_\.a-zA-Z\d]*\/)?[-a-zA-Z\d][-_\.a-zA-Z\d]*/$, so that this is still a pretty unrestricted validation, the main thing is to avoid odd edge cases with things like percent-encoded backtracking.

An alternative here could just be to disallow the percent encodings of / and \\ explicitly, instead of all percent encodings, which I'd be open to as well.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 5, 2019
@guybedford guybedford requested a review from jkrems August 5, 2019 06:34
@addaleax addaleax added the esm Issues and PRs related to the ECMAScript Modules implementation. label Aug 5, 2019
lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved
@@ -0,0 +1,18 @@
// Flags: --experimental-modules
Copy link
Contributor

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

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

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

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

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

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

@jkrems jkrems Aug 6, 2019

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. :)

@guybedford guybedford force-pushed the pkgname branch 2 times, most recently from 38fe5cc to acfad9d Compare August 6, 2019 01:01
@guybedford guybedford requested a review from jkrems August 6, 2019 01:03
Copy link
Contributor

@jkrems jkrems left a comment

Choose a reason for hiding this comment

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

:shipit:

@nodejs-github-bot
Copy link
Collaborator

@jkrems jkrems added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 6, 2019
@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Aug 8, 2019

Landed in 0e03c44

@Trott Trott closed this Aug 8, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Aug 8, 2019
PR-URL: nodejs#28965
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@guybedford guybedford deleted the pkgname branch August 8, 2019 02:58
targos pushed a commit that referenced this pull request Aug 19, 2019
PR-URL: #28965
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants