esm: identify parent importing a url with invalid host #49736

JakobJingleheimer · 2023-09-20T15:08:30Z

New output:

TypeError [ERR_INVALID_FILE_URL_HOST]: File URL host must be "localhost" or empty on darwin
    at new NodeError (node:internal/errors:406:5)
    at getPathFromURLPosix (node:internal/url:1385:11)
    at fileURLToPath (node:internal/url:1408:50)
    at finalizeResolution (node:internal/modules/esm/resolve:210:16)
    …
  code: 'ERR_INVALID_FILE_URL_HOST',
  input: 'file://hmm.js/',
  module: 'file:///…/test.mjs'

fixes #49571

nodejs-github-bot · 2023-09-20T15:08:35Z

Review requested:

@nodejs/loaders
@nodejs/url

lib/internal/url.js

anonrig · 2023-09-20T15:26:25Z

lib/internal/errors.js

-E('ERR_INVALID_FILE_URL_HOST',
-  'File URL host must be "localhost" or empty on %s', TypeError);
+E('ERR_INVALID_FILE_URL_HOST', (url, platform, base) => {
+  let msg = `File URL host in ${url}`;


Why not pass the base and url to this just like in err_invalid_url?

Looking at the comment within err_invalid_url that's probably a good idea!

lib/internal/errors.js

lib/internal/modules/esm/resolve.js

GeoffreyBooth

Can we please add a test?

TypeError [ERR_INVALID_FILE_URL_HOST]: File URL host must be "localhost" or empty on darwin

On darwin? You mean the rules differ by platform?

JakobJingleheimer · 2023-09-21T16:24:25Z

On darwin? You mean the rules differ by platform?

Yep. MacOS + Linux vs Windows.

GeoffreyBooth · 2023-09-21T16:38:05Z

On darwin? You mean the rules differ by platform?

Yep. MacOS + Linux vs Windows.

So we should have tests for both versions of the error message. CI can validate the Windows one. We should also make the platform name friendly, like translate “darwin” to “macOS” and “win32” to “Windows”, and maybe everything else gets “POSIX platforms”. What is the rule for Windows?

JakobJingleheimer · 2023-09-21T16:41:25Z

That sounds rather out of scope for the issue this is fixing. There is already a test for the underlying util that handles this. All I'm doing here is appending some properties to the error, not changing its message.

GeoffreyBooth · 2023-09-21T16:46:59Z

That sounds rather out of scope for the issue this is fixing. There is already a test for the underlying util that handles this. All I’m doing here is appending some properties to the error, not changing its message.

Oh, I thought the message itself was the new part. I totally didn’t notice these extra properties. Why not put them into the message itself, in addition to where they are now?

JakobJingleheimer · 2023-09-21T16:51:36Z

I did that at first, but it was rather dirty: The info was not available (so it involved a lot of daisy-chaining), and this augmentation isn't applicable to any other uses of the underlying util.

Also embedding the url in the message can leak data in logging.

ljharb · 2023-09-21T17:48:53Z

Can there be a test to cover this change?

JakobJingleheimer · 2023-09-21T22:57:56Z

Can there be a test to cover this change?

Ordinarily I would've included one already, but this applies to only POSIX platforms, and I don't know how to mark a test-case as being applicable to only a certain platform.

GeoffreyBooth · 2023-09-21T23:30:10Z

I don’t know how to mark a test-case as being applicable to only a certain platform.

Just put if (process.platform !== 'win32'), or whatever the comparison should be, within the test. Then it’s a no-op on unsupported platforms. There are examples of tests like this in the codebase already. https://nodejs.org/api/process.html#processplatform

nodejs-github-bot · 2023-09-22T16:14:23Z

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

nodejs-github-bot · 2023-09-22T19:26:30Z

Commit Queue failed

- Loading data for nodejs/node/pull/49736
✔  Done loading data for nodejs/node/pull/49736
----------------------------------- PR info ------------------------------------
Title      esm: identify parent importing a url with invalid host (#49736)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     JakobJingleheimer:fix/esm-specifier-invalid-url-host -> nodejs:main
Labels     errors, whatwg-url, esm, needs-ci, commit-queue-squash
Commits    3
 - esm: identify parent importing a url with invalid host
 - test: add case for invalid posix host
 - fixup!: de-lint
Committers 1
 - Jacob Smith <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/49736
Reviewed-By: Antoine du Hamel 
Reviewed-By: Geoffrey Booth 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/49736
Reviewed-By: Antoine du Hamel 
Reviewed-By: Geoffrey Booth 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - test: add case for invalid posix host
   ⚠  - fixup!: de-lint
   ℹ  This PR was created on Wed, 20 Sep 2023 15:08:30 GMT
   ✔  Approvals: 2
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/49736#pullrequestreview-1637644110
   ✔  - Geoffrey Booth (@GeoffreyBooth) (TSC): https://github.com/nodejs/node/pull/49736#pullrequestreview-1638329347
   ✘  Last GitHub CI failed
   ℹ  Last Full PR CI on 2023-09-22T18:42:45Z: https://ci.nodejs.org/job/node-test-pull-request/54154/
- Querying data for job/node-test-pull-request/54154/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/6278312662

nodejs-github-bot · 2023-09-22T20:04:59Z

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

nodejs-github-bot · 2023-09-22T20:23:39Z

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

nodejs-github-bot · 2023-09-22T20:39:11Z

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

nodejs-github-bot · 2023-09-22T22:58:11Z

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

nodejs-github-bot · 2023-09-23T05:41:28Z

Landed in 645b788

PR-URL: #49736 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>

PR-URL: nodejs#49736 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>

PR-URL: #49736 Backport-PR-URL: #50669 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>

PR-URL: nodejs/node#49736 Backport-PR-URL: nodejs/node#50669 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Sep 20, 2023

anonrig reviewed Sep 20, 2023

View reviewed changes

aduh95 reviewed Sep 21, 2023

View reviewed changes

lib/internal/errors.js Outdated Show resolved Hide resolved

aduh95 reviewed Sep 21, 2023

View reviewed changes

lib/internal/modules/esm/resolve.js Outdated Show resolved Hide resolved

esm: identify parent importing a url with invalid host

331c950

JakobJingleheimer force-pushed the fix/esm-specifier-invalid-url-host branch from 495cf11 to 331c950 Compare September 21, 2023 10:26

aduh95 approved these changes Sep 21, 2023

View reviewed changes

GeoffreyBooth reviewed Sep 21, 2023

View reviewed changes

GeoffreyBooth approved these changes Sep 21, 2023

View reviewed changes

JakobJingleheimer added 2 commits September 22, 2023 10:52

test: add case for invalid posix host

d6e33ed

fixup!: de-lint

493c385

jlenon7 approved these changes Sep 22, 2023

View reviewed changes

JakobJingleheimer added request-ci Add this label to start a Jenkins CI on a PR. commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Sep 22, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 22, 2023

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 22, 2023

GeoffreyBooth approved these changes Sep 22, 2023

View reviewed changes

JakobJingleheimer added commit-queue Add this label to land a pull request using GitHub Actions. request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Sep 22, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 22, 2023

aduh95 removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 22, 2023

github-actions bot mentioned this pull request Sep 23, 2023

CI Reliability 2023-09-23 nodejs/reliability#674

Open

16 tasks

GeoffreyBooth added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 23, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 23, 2023

nodejs-github-bot merged commit 645b788 into nodejs:main Sep 23, 2023
48 checks passed

github-actions bot mentioned this pull request Sep 24, 2023

CI Reliability 2023-09-24 nodejs/reliability#675

Open

20 tasks

ruyadorno pushed a commit that referenced this pull request Sep 28, 2023

esm: identify parent importing a url with invalid host

05f0fcb

PR-URL: #49736 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>

This was referenced Sep 28, 2023

v20.7.1 proposal #49917

Closed

v20.8.0 proposal #49932

Merged

alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023

esm: identify parent importing a url with invalid host

553bfcf

PR-URL: nodejs#49736 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>

targos pushed a commit to targos/node that referenced this pull request Nov 11, 2023

esm: identify parent importing a url with invalid host

efd4e8c

PR-URL: nodejs#49736 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>

targos mentioned this pull request Nov 11, 2023

[v18.x] Backport most ESM and customization hook changes #50669

Closed

targos added the backported-to-v18.x PRs backported to the v18.x-staging branch. label Nov 23, 2023

targos pushed a commit that referenced this pull request Nov 23, 2023

esm: identify parent importing a url with invalid host

ed8dd33

PR-URL: #49736 Backport-PR-URL: #50669 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>

targos mentioned this pull request Nov 28, 2023

v18.19.0 release proposal #50953

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

esm: identify parent importing a url with invalid host #49736

esm: identify parent importing a url with invalid host #49736

JakobJingleheimer commented Sep 20, 2023 •

edited

Loading

nodejs-github-bot commented Sep 20, 2023

anonrig Sep 20, 2023

JakobJingleheimer Sep 20, 2023

GeoffreyBooth left a comment

JakobJingleheimer commented Sep 21, 2023

GeoffreyBooth commented Sep 21, 2023

JakobJingleheimer commented Sep 21, 2023

GeoffreyBooth commented Sep 21, 2023

JakobJingleheimer commented Sep 21, 2023

ljharb commented Sep 21, 2023

JakobJingleheimer commented Sep 21, 2023 •

edited

Loading

GeoffreyBooth commented Sep 21, 2023

nodejs-github-bot commented Sep 22, 2023

nodejs-github-bot commented Sep 22, 2023

nodejs-github-bot commented Sep 22, 2023

nodejs-github-bot commented Sep 22, 2023

nodejs-github-bot commented Sep 22, 2023

nodejs-github-bot commented Sep 22, 2023

nodejs-github-bot commented Sep 23, 2023

esm: identify parent importing a url with invalid host #49736

esm: identify parent importing a url with invalid host #49736

Conversation

JakobJingleheimer commented Sep 20, 2023 • edited Loading

nodejs-github-bot commented Sep 20, 2023

anonrig Sep 20, 2023

Choose a reason for hiding this comment

JakobJingleheimer Sep 20, 2023

Choose a reason for hiding this comment

GeoffreyBooth left a comment

Choose a reason for hiding this comment

JakobJingleheimer commented Sep 21, 2023

GeoffreyBooth commented Sep 21, 2023

JakobJingleheimer commented Sep 21, 2023

GeoffreyBooth commented Sep 21, 2023

JakobJingleheimer commented Sep 21, 2023

ljharb commented Sep 21, 2023

JakobJingleheimer commented Sep 21, 2023 • edited Loading

GeoffreyBooth commented Sep 21, 2023

nodejs-github-bot commented Sep 22, 2023

nodejs-github-bot commented Sep 22, 2023

nodejs-github-bot commented Sep 22, 2023

nodejs-github-bot commented Sep 22, 2023

nodejs-github-bot commented Sep 22, 2023

nodejs-github-bot commented Sep 22, 2023

nodejs-github-bot commented Sep 23, 2023

JakobJingleheimer commented Sep 20, 2023 •

edited

Loading

JakobJingleheimer commented Sep 21, 2023 •

edited

Loading