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

node 19.1 breaks url.parse #45514

Closed
ljharb opened this issue Nov 19, 2022 · 5 comments · Fixed by #45517
Closed

node 19.1 breaks url.parse #45514

ljharb opened this issue Nov 19, 2022 · 5 comments · Fixed by #45517

Comments

@ljharb
Copy link
Member

ljharb commented Nov 19, 2022

Version

v19.1.0

Platform

Darwin Jordans-MacBook-Pro-2.local 20.6.0 Darwin Kernel Version 20.6.0: Thu Sep 29 20:15:11 PDT 2022; root:xnu-7195.141.42~1/RELEASE_X86_64 x86_64

Subsystem

url

What steps will reproduce the bug?

url.parse('https://git@github.com:inspect-js/is-array-buffer.git')

How often does it reproduce? Is there a required condition?

node v19.1.0 always reproduces it; any earlier version does not.

What is the expected behavior?

Url {
  protocol: 'https:',
  slashes: true,
  auth: 'git',
  host: 'github.com',
  port: null,
  hostname: 'github.com',
  hash: null,
  search: null,
  query: null,
  pathname: '/:inspect-js/is-array-buffer.git',
  path: '/:inspect-js/is-array-buffer.git',
  href: 'https://git@github.com/:inspect-js/is-array-buffer.git'
}

What do you see instead?

Uncaught TypeError [ERR_INVALID_URL]: Invalid URL
    at __node_internal_captureLargerStackTrace (node:internal/errors:484:5)
    at new NodeError (node:internal/errors:393:5)
    at getHostname (node:url:521:15)
    at Url.parse (node:url:390:14)
    at Object.urlParse [as parse] (node:url:147:13) {
  input: 'https://git@github.com:inspect-js/is-array-buffer.git',
  code: 'ERR_INVALID_URL'
}

Additional information

Specifically, this breaks https://npmjs.com/auto-changelog via https://npmjs.com/parse-github-url in node 19.1+.

@ljharb
Copy link
Member Author

ljharb commented Nov 19, 2022

This may be related to #45116 or #45011; cc @Trott

@Trott
Copy link
Member

Trott commented Nov 19, 2022

I saw @aduh95 had marked something as baking-for-lts and dont-land-on-v18.x out of suspicion that it broke a url.parse() usage in the ecosystem but now I'm not finding that anywhere.

Normally, the thing to do would be to find the culprit commit and revert it (and likely re-introduce it as a breaking change in a major release). However it's likely this is going to be a revert that will increase security liability, so another possibility is to create a carve-out for these kinds of URLs.

@Trott
Copy link
Member

Trott commented Nov 19, 2022

Confirmed that the culprit is #45012.

@Trott

This comment was marked as resolved.

@Trott
Copy link
Member

Trott commented Nov 19, 2022

Unfortunately, I'm not seeing a non-terrible way to do a carve-out. Reluctantly, I think the approach I'd take is:

  1. Revert this.
  2. Re-introduce it as a warning rather than an error.
  3. Re-introduce the error as a semver-major and Get The Word Out™.

I'll open a revert now.

Trott added a commit to Trott/io.js that referenced this issue Nov 19, 2022
This reverts commit 5f7730e.

This change broke too many edge cases in the ecosystem. Reverting it
re-introduces some host-spoofing possibilities, so we won't want to
revert forever, but the issue is long-lived enough and not sufficiently
critical that we can't wait for a major release to introduce it as a
breaking change. After this lands, I plan to re-introduce this as a
change that throws a warning rather than an error, after which we can
land a semver-major that re-introduces the error and try to get the word
out to maintainers of likely-affected packages.

Closes: nodejs#45514
Refs: nodejs#45012
nodejs-github-bot pushed a commit that referenced this issue Nov 19, 2022
This reverts commit 5f7730e.

This change broke too many edge cases in the ecosystem. Reverting it
re-introduces some host-spoofing possibilities, so we won't want to
revert forever, but the issue is long-lived enough and not sufficiently
critical that we can't wait for a major release to introduce it as a
breaking change. After this lands, I plan to re-introduce this as a
change that throws a warning rather than an error, after which we can
land a semver-major that re-introduces the error and try to get the word
out to maintainers of likely-affected packages.

Closes: #45514
Refs: #45012
PR-URL: #45517
Fixes: #45514
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
ruyadorno pushed a commit that referenced this issue Nov 21, 2022
This reverts commit 5f7730e.

This change broke too many edge cases in the ecosystem. Reverting it
re-introduces some host-spoofing possibilities, so we won't want to
revert forever, but the issue is long-lived enough and not sufficiently
critical that we can't wait for a major release to introduce it as a
breaking change. After this lands, I plan to re-introduce this as a
change that throws a warning rather than an error, after which we can
land a semver-major that re-introduces the error and try to get the word
out to maintainers of likely-affected packages.

Closes: #45514
Refs: #45012
PR-URL: #45517
Fixes: #45514
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this issue Nov 23, 2022
This reverts commit 5f7730e.

This change broke too many edge cases in the ecosystem. Reverting it
re-introduces some host-spoofing possibilities, so we won't want to
revert forever, but the issue is long-lived enough and not sufficiently
critical that we can't wait for a major release to introduce it as a
breaking change. After this lands, I plan to re-introduce this as a
change that throws a warning rather than an error, after which we can
land a semver-major that re-introduces the error and try to get the word
out to maintainers of likely-affected packages.

Closes: nodejs#45514
Refs: nodejs#45012
PR-URL: nodejs#45517
Fixes: nodejs#45514
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants