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.

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

Jump to bottom

esm: improve validation of resolved URLs #41446

Merged
merged 2 commits into from
Jan 19, 2022
Merged

esm: improve validation of resolved URLs #41446

merged 2 commits into from
Jan 19, 2022

Conversation

JakobJingleheimer
Copy link
Contributor

Some ESM internals use type coercion on URL instances to get the href of the URL (ex `${url}`). This can result in 'undefined', which bypasses the current simple "is a string" validation. When that is bypassed, the resulting thrown error is a red herring, pointing to completely unrelated code.

Rather than merely switching those internals to use less errant alternatives, I think it's better to improve validation.

cc @nodejs/loaders @nodejs/modules

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Jan 8, 2022
@JakobJingleheimer
Copy link
Contributor Author

JakobJingleheimer commented Jan 8, 2022 •
edited
Loading

Windows test failures are due to a failed dependency install (unrelated to the change in this PR):

Error retrieving packages from source 'https://community.chocolatey.org/api/v2/':
 Could not connect to the feed specified at 'https://community.chocolatey.org/api/v2/'. Please verify that the package source (located in the Package Manager Settings) is valid and ensure your network connectivity.
nasm not installed. The package was not found with the source(s) listed.
 Source(s): 'https://community.chocolatey.org/api/v2/'
 NOTE: When you specify explicit sources, it overrides default sources.
If the package version is a prerelease and you didn't specify `--pre`,
 the package may not be found.
Please see https://docs.chocolatey.org/en-us/troubleshooting for more
 assistance.

Chocolatey installed 0/1 packages. 1 packages failed.
 See the log for details (C:\ProgramData\chocolatey\logs\chocolatey.log).

Failures
 - nasm - nasm not installed. The package was not found with the source(s)

@bmeck
Copy link
Member

bmeck commented Jan 10, 2022

So this has an effect of allowing sending any kind of object that can coerce to a string as the return value. I would prefer we didn't do this since it would make future changes to the API hard since virtually everything converts to a string.

@JakobJingleheimer
Copy link
Contributor Author

How would adding new URL(url) allow returning any object? It will explode for anything that is not a url string or stringifiable to a url string. We could maybe need to worry about something like an anchor element, in which case I could check both typeof url !== 'string' and new URL(url).

@bmeck
Copy link
Member

bmeck commented Jan 10, 2022

@JakobJingleheimer I'd agree it must turn into a URL compatible string, but anything could be that. For example a Buffer can do that. And I don't want a Buffer/SharedArrayBuffer to be able to do that.

@JakobJingleheimer
Copy link
Contributor Author

Would my proposal to restore the if (typeof url === 'string') check in addition to the new new URL(url) suffice?

@bmeck
Copy link
Member

bmeck commented Jan 10, 2022

sure, checking both seems fine. I'm trying to think of cases where people would even want to return a non-URL?? Can't think of any off the top of my head so it seems fine

@JakobJingleheimer
Copy link
Contributor Author

They're currently required to return a url, no?

@bmeck
Copy link
Member

bmeck commented Jan 10, 2022

@JakobJingleheimer yes, it currently is supposed to check for a stringified URL as a return

@nodejs-github-bot
Copy link
Collaborator

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

@JakobJingleheimer
Copy link
Contributor Author

@guybedford any thoughts, concerns, or objections?

guybedford
guybedford reviewed Jan 14, 2022
View reviewed changes
Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Seems good to me, down to performance questions.

lib/internal/modules/esm/loader.js Show resolved Hide resolved
@GeoffreyBooth GeoffreyBooth added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Jan 18, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 19, 2022
@nodejs-github-bot nodejs-github-bot merged commit dbc6e39 into nodejs:master Jan 19, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in dbc6e39

@GeoffreyBooth GeoffreyBooth removed commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jan 19, 2022
@GeoffreyBooth GeoffreyBooth deleted the fix/esm-undefined-url-bypass branch January 19, 2022 05:47
BethGriggs pushed a commit that referenced this pull request Jan 25, 2022
@JakobJingleheimer @BethGriggs
esm: improve validation of resolved URLs
e1d5a35 
PR-URL: #41446
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
@JakobJingleheimer @Linkgoron
esm: improve validation of resolved URLs
d2d03cf 
PR-URL: nodejs#41446
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Feb 8, 2022
Merged
danielleadams pushed a commit that referenced this pull request Feb 26, 2022
@JakobJingleheimer @danielleadams
esm: improve validation of resolved URLs
ea84c89 
PR-URL: #41446
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
@danielleadams danielleadams mentioned this pull request Mar 3, 2022
Merged
danielleadams pushed a commit that referenced this pull request Mar 14, 2022
@JakobJingleheimer @danielleadams
esm: improve validation of resolved URLs
bd71706 
PR-URL: #41446
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
@JakobJingleheimer JakobJingleheimer mentioned this pull request Apr 10, 2022
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb left review comments

@bmeck bmeck bmeck approved these changes

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

@guybedford guybedford guybedford approved these changes

Assignees
No one assigned
Labels
esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@JakobJingleheimer @nodejs-github-bot @bmeck @ljharb @GeoffreyBooth @guybedford