module: fix null restrictions in imports and exports patterns

[aduh95]

Fixes: #44316

/cc @nodejs/modules

[nodejs-github-bot]

Review requested:

• @nodejs/modules

[guybedford]

@aduh95 nice quick work, and good test cases. We should make sure that this normalization applies in all suitable paths. Note also that any changes to the exports resolution require a corresponding spec change.

There are two options for this - normalization and validation. For normalization we would add an initial line to PACKAGE_IMPORTS_EXPORTS_RESOLVE:

**PACKAGE\_IMPORTS\_EXPORTS\_RESOLVE**(_matchKey_, _matchObj_, _packageURL_,
_isImports_, _conditions_)
> 1. Replace in _matchKey_ any _"//"_ with _"/"_.

For validation, we would update the existing validations in PACKAGE_TARGET_RESOLVE to throw a validation error for this case like we do for node_modules / percent encodings etc. We would then replace this line:

>       If _subpath_ split on _"/"_ or _"\\"_ contains any _"."_, _".."_, or
>       _"node\_modules"_ segments, case insensitive and including percent
>       encoded variants, throw an _Invalid Module Specifier_ error.

With:

>       If _subpath_ split on _"/"_ or _"\\"_ contains any empty, _"."_, _".."_, or
>       _"node\_modules"_ segments, case insensitive and including percent
>       encoded variants, throw an _Invalid Module Specifier_ error.

I'm ok with validation or normalization. Perhaps slightly tending towards preferring validation since I can't imagine a scenario where supporting these double slashings would be desired. Where normalization in general opens up more vectors, we probably want to reduce vectors.

The following code paths / test cases come to mind as needing to be included in this PR as well:

• Trailing slash exports
• Exports without a trailing slash or wildcard
• Imports without a trailing slash or wildcard
• All the above for CommonJS require as well

[aduh95]

The following code paths / test cases come to mind as needing to be included in this PR as well:

• Trailing slash exports
• Exports without a trailing slash or wildcard
• Imports without a trailing slash or wildcard
• All the above for CommonJS require as well

IIUC, except the first point all of those test cases are already covered by the tests as is. Or am I missing something?

[guybedford]

I didn't thoroughly check the tests, CommonJS ones are there and it does seem many are included. Here is a list of some other cases:

{
  "exports": {
    "./expt//": "./mapping/",
    "./expt//sub/": "./mapping/sub/",
    "./expt//exact": "./mapping/exact.js"
  }
}

With corresponding variations for imports.

Did you have any further feedback on validation versus normalization?

[aduh95]

Here is a list of some other cases:

{
  "exports": {
    "./expt//": "./mapping/",
    "./expt//sub/": "./mapping/sub/",
    "./expt//exact": "./mapping/exact.js"
  }
}

With corresponding variations for imports.

Wasn't that removed in v17.0.0?

node/doc/api/deprecations.md

Lines 2851 to 2857 in 5e5fb82

	### DEP0148: Folder mappings in `"exports"` (trailing `"/"`)
	<!-- YAML
	changes:
	- version: v17.0.0
	pr-url: https://github.com/nodejs/node/pull/40121
	description: End-of-Life.

Did you have any further feedback on validation versus normalization?

IMO it's totally OK to let users define and/or use multiple slashes in a row in their exports/imports, and also it makes sense for when defining a null exports to make sure it cannot be jumped over. So normalizing all the paths wouldn't make sense IMO, only the one that are defined to null, but maybe that'd be to difficult to write in spec language.

[ljharb]

It would be very confusing if we allowed an author to define foo/a and foo//a to be different files - it seems like validation is a much safer approach.

[aduh95]

It would be very confusing if we allowed an author to define foo/a and foo//a to be different files - it seems like validation is a much safer approach.

I don't know, HTTP let you do that, and that doesn't seem to cause much harm.

[ljharb]

I heavily disagree - it's caused a TON of harm, as have most of the ways the web is lax about structure.

[bmeck]

Only flat file systems are affected by this for `file:` which is the only thing exports/imports field govern. The only real flat file systems still in use are IOT or a special mounting of S3 directly these days. Normalizing seems fine to me as it is prohibitively hard to have "a//b" preserved in modern FS. If this affected network I might have more opinions but this shouldn't affect network where that kind of things are commonly preserved.

…

On Mon, Aug 22, 2022, 1:33 PM Jordan Harband ***@***.***> wrote: I heavily disagree - it's caused a TON of harm, as have most of the ways the web is lax about structure. — Reply to this email directly, view it on GitHub <#44328 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABZJI3UOGJ76U4OTHK5LBLV2PBZNANCNFSM57EZV4YQ> . You are receiving this because you are on a team that was mentioned.Message ID: ***@***.***>

[guybedford]

Wasn't that removed in v17.0.0?

Just because it was deprecated doesn't mean we don't still support it and have tests for it.

I'm ok with both approaches, but for normalization we very specifically need to apply it more generally something like the suggested spec text.

I do think validation would be safer though overall, again if done carefully with respect to the spec work and edge cases.

[guybedford]

We must include the corresponding change in the ESM resolver spec.

[aduh95]

Superseded by #44477.

[guybedford]

@aduh95 I wonder if it's time to start thinking about the upcoming runtime deprecations?

[aduh95]

@guybedford Runtime deprecation has landed on v19.x (#44495), so I guess the next step would be to remove it completely.

[guybedford]

Ahh, yes then I think we have a number of runtime deprecations that we should probably remove for 20?

[guybedford]

We could always decide to wait on some of the more recent deprecations though as well.