lib: fix regular expression to detect / and \

[fasttime]

Allow importing from file URLs with encoded commas. Forbid encoded backslashes. Fixes#40305.

[targos]

@nodejs/modules

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/40155/

[guybedford]

Many thanks, and apologies for the slip!

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/40161/

[bmeck]

Since , is significant for data: we should add a test for that as well even if this codepath doesn't cover data: normally I could foresee a refactor accidentally mixing the 2.

[guybedford]

@bmeck this code path does apply to data URLs, in that data URLs that contain a percent encoded comma will no longer throw, even if that percent encoding is in the body of the data URL. On the other hand, percent encoded / and \\ in any position of a data URL, including the body, will always throw. One option here could be to separately special case data URLs in opting out of these validations for the body of the data URL. I think that should be a separate PR though.

[bmeck]

@guybedford my main concern is how:

import(`data:text/javascript;x="${encodeURIComponent(',')}",export default 123`);

is interpreted and ensuring it works, otherwise we could just leave the , in the regexp in this PR rather than removing it if we just want to fix the slashes. I'd prefer we have a test for the data: impact if we change its behavior.

[guybedford]

@bmeck I believe right now your example would fail across all versions, where this PR fixes that.

[guybedford]

Ah, I see data: URLs have an early exit higher up in the resolver so don't hit this path. We should codify that into the spec as well, will post a PR.

[guybedford]

@bmeck since data URLs definitely never hit this path at all, I don't think there's any test coverage to apply here, are you ok with landing as is to get this fix in? I am working on a PR to clarify the data URL distinction in the spec as well as some other encoding cases.

[bmeck]

@guybedford yea that is fine, though also note that Blob is up and coming

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/40384/