url: fix url.parse() for @hostname by Trott · Pull Request #42136 · nodejs/node

Trott · 2022-02-26T05:57:00Z

Make url.parse() behave more like browsers and WHATWHG URL when dealing
with URLs that have the format http:@example.com. This is the same as
http://example.com.

nodejs-github-bot · 2022-02-26T06:09:07Z

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

RaisinTen · 2022-02-26T06:21:10Z

Instead of fixing the legacy api, shouldn't we help users migrate to the WHATWG url api which we recommend in the docs?

lpinca · 2022-02-26T06:43:50Z

No objections but I think special casing @ does not make much sense. It should do the same for http:example.com, http:/example.com, http:\example.com, http://////////example.com, etc.

It should ignore all forward and backward slashes for special protocols.

Trott · 2022-02-26T14:33:30Z

Instead of fixing the legacy api, shouldn't we help users migrate to the WHATWG url api which we recommend in the docs?

I think we should do both if:

The bug in the legacy API could plausibly have security implications.
The bug in the legacy API can be fixed in an otherwise backward-compatible way.
There is someone willing to do the fix.

Trott · 2022-02-26T14:37:02Z

No objections but I think special casing @ does not make much sense. It should do the same for http:example.com, http:/example.com, http:\example.com, http://////////example.com, etc.

Oh, yes, very much so. Let me throw this into draft mode and see if I can address all those other kinds of edge cases without breaking existing tests/usage.

RaisinTen · 2022-02-26T15:41:25Z

Instead of fixing the legacy api, shouldn't we help users migrate to the WHATWG url api which we recommend in the docs?
I think we should do both if:
The bug in the legacy API could plausibly have security implications.
The bug in the legacy API can be fixed in an otherwise backward-compatible way.
There is someone willing to do the fix.

According to

node/doc/api/url.md

Lines 1562 to 1565 in aa97c9d

 use the WHATWG`URL`API. Because the `url.parse()` method uses a 
 lenient, non-standard algorithm for parsing URL strings, security 
 issues can be introduced. Specifically, issues with [host name spoofing][] and 
 incorrect handling of usernames and passwords have been identified. 

it is a known source of bugs and fixing those would motivate users to keep using them and that defeats the purpose of labelling it as a legacy API, right? Why would people move to the WHATWG URL API if we accept fixes for the legacy API?

Trott · 2022-02-26T16:43:23Z

it is a known source of bugs and fixing those would motivate users to keep using them and that defeats the purpose of labelling it as a legacy API, right? Why would people move to the WHATWG URL API if we accept fixes for the legacy API?

A lot of people are using url.parse() through dependencies and aren't even aware they're using it.

We should not refuse fixes for an insecure API as a way of making users move to a newer API. (I'm not saying we have to do the fixes ourselves. But refusing to land a provided fix seems hostile.)

If we want to provide stronger encouragement for people to move off of url.parse() and to stop using libraries that use url.parse(), then we need to introduce a runtime deprecation or some other mechanism. Telling people that they should have read the documentation warning (which likely didn't exist when they wrote their code in the first place) is not how I want to see Node.js managed.

Trott · 2022-02-26T23:49:25Z

Oh, yes, very much so. Let me throw this into draft mode and see if I can address all those other kinds of edge cases without breaking existing tests/usage.

That will be a breaking change, so I'm going to open that as a follow-on PR and leave this one as-is.

Trott · 2022-02-26T23:54:04Z

Follow on PR with breaking changes: #42140

RaisinTen · 2022-02-27T14:34:56Z

We should not refuse fixes for an insecure API as a way of making users move to a newer API. (I'm not saying we have to do the fixes ourselves. But refusing to land a provided fix seems hostile.)

Does that mean that url.parse() is supposed to be treated the same way as any non-legacy stable API until it gets runtime-deprecated?

RaisinTen · 2022-02-27T14:43:22Z

That will be a breaking change, so I'm going to open that as a follow-on PR and leave this one as-is.

According to #42136 (comment), one of the scenarios when we should go ahead with the fix is when:

The bug in the legacy API can be fixed in an otherwise backward-compatible way.

but that fix is a breaking change. Isn't that contradictory?

Trott · 2022-02-27T17:15:24Z

Does that mean that url.parse() is supposed to be treated the same way as any non-legacy stable API until it gets runtime-deprecated?

Potential security problems in url.parse() tend to be answered with "don't use url.parse()" rather than "we need to do a security release and issue a CVE". So that's one difference in the way they are treated.

Trott · 2022-02-27T17:30:27Z

but that fix is a breaking change. Isn't that contradictory?

To be clear for anyone following along, we're talking about a different PR at this point. This question is about #42140. That PR is a breaking change, but this one is not.

That set of conditions was a sufficient set of conditions, not a necessary set of conditions. In other words, we can choose to break legacy APIs in major releases. There is a lot to weigh there: Disruption to users and the ecosystem vs. moving url.parse() to a more standardized (and arguably less bug-prone) state. I won't be surprised if we choose not to do it.

RaisinTen · 2022-02-28T15:44:58Z

cc @nodejs/url

RaisinTen

Seems reasonable to me, approving!

Trott · 2022-03-02T02:28:58Z

@nodejs/url Anyone want to be the second approval for this?

Trott · 2022-03-02T03:53:49Z

Please don't add the commit-queue label to this one. I'll need to edit the commit message before landing to add some information. Thanks!

Make url.parse() behave more like browsers and WHATWHG URL when dealing with URLs that of the format `http:@example.com`. This is the same as `http://example.com`. This issue was reported by P0cas. https://github.com/P0cas PR-URL: nodejs#42136 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

Trott · 2022-03-02T22:36:30Z

Landed in 010cb71

Make url.parse() behave more like browsers and WHATWHG URL when dealing with URLs that of the format `http:@example.com`. This is the same as `http://example.com`. This issue was reported by P0cas. https://github.com/P0cas PR-URL: nodejs#42136 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

arcanis · 2022-03-10T11:01:43Z

This diff seems to break Yarn classic which does url.resolve('https://registry.npmjs.org', '@foo/bar').

Node 17.6: https://registry.npmjs.org/@foo/bar
Node 17.7: https://foo/bar

aduh95 · 2022-03-10T11:18:19Z

Should we revert?

arcanis · 2022-03-10T11:24:21Z

I've landed a fix in Yarn (url.resolve(..., './@foo/bar')), but it'll take time until people upgrade, and the error message will be very confusing in the meantime. I'd personally suggest to either revert or special-case this pattern (resolve called with @ as leading character in the subpath components), if possible 😕

aduh95 · 2022-03-10T11:34:06Z

PR to revert: #42280

@arcanis FYI adding Yarn (both classic and Berry) to https://github.com/nodejs/citgm could help catch this kind of regressions before we release new versions of Node.js, I think it's worth considering. (and if Yarn is already on the CITGM list, we'd need to investigate why the failure wasn't caught up on the release run https://ci.nodejs.org/job/citgm-smoker/2869/)

unerh · 2022-04-06T06:45:29Z

Hi,
Are there any intentions in pushing this to v14 and v16, as Snyk scan reports a vulnerability on this:
https://security.snyk.io/vuln/SNYK-UPSTREAM-NODE-2440551

and the preference in my org is to use LTS versions.
Thanks

aduh95 · 2022-04-06T08:19:18Z

There’s no intention of backporting this to LTS releases, it’s a breaking change that has been reverted, and that we are not interested in landing anymore. Snyk report is wrong when it says Node.js 17.7.1 is not affected (it is, we have reverted the fix on that version), and is wrong when it calls it a vulnerability.

FYI that Snyk report is only about the legacy URL parser, if you are using the WHATWG one, you can safely ignore that quirk.

mcollina · 2022-04-06T13:08:18Z

I would also note that this is not a vulnerability. We have analyzed this in great detail and it's not a problem.

Make url.parse() behave more like browsers and WHATWHG URL when dealing with URLs that of the format `http:@example.com`. This is the same as `http://example.com`. This issue was reported by P0cas. https://github.com/P0cas PR-URL: nodejs#42136 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

nodejs-github-bot added needs-ci PRs that need a full CI run. url Issues and PRs related to the legacy built-in url module. labels Feb 26, 2022

Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 26, 2022

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 26, 2022

Trott marked this pull request as draft February 26, 2022 14:37

Trott marked this pull request as ready for review February 26, 2022 23:49

Trott mentioned this pull request Feb 26, 2022
url: more closely align url.parse() with WHATWHG URL #42140
Closed

RaisinTen approved these changes Feb 28, 2022
View reviewed changes

aduh95 approved these changes Mar 2, 2022
View reviewed changes

Trott force-pushed the url-parse-at branch from 457af28 to 010cb71Compare March 2, 2022 22:36

Trott merged commit 010cb71 into nodejs:masterMar 2, 2022

Trott deleted the url-parse-at branch March 2, 2022 22:36

sxa mentioned this pull request Mar 8, 2022
v17.7.0 release proposal #42254
Merged

yura3d mentioned this pull request Mar 9, 2022
Network error w/ scoped packages (@foo/bar) on Node 17.7 yarnpkg/yarn#8797
Closed

arcanis mentioned this pull request Mar 10, 2022
17.7 Regression on url.resolve breaks Yarn 1.x #42279
Closed

aduh95 added dont-land-on-v12.x labels Mar 10, 2022

zburke mentioned this pull request Mar 10, 2022
test some nodey things folio-org/ui-inventory#1624
Closed

Uh oh!

Uh oh!

Conversation

Trott commented Feb 26, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 26, 2022

Uh oh!

RaisinTen commented Feb 26, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lpinca commented Feb 26, 2022

Uh oh!

Trott commented Feb 26, 2022

Uh oh!

Trott commented Feb 26, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RaisinTen commented Feb 26, 2022

Uh oh!

Trott commented Feb 26, 2022

Uh oh!

Trott commented Feb 26, 2022

Uh oh!

Trott commented Feb 26, 2022

Uh oh!

RaisinTen commented Feb 27, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RaisinTen commented Feb 27, 2022

Uh oh!

Trott commented Feb 27, 2022

Uh oh!

Trott commented Feb 27, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RaisinTen commented Feb 28, 2022

Uh oh!

RaisinTen left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Mar 2, 2022

Uh oh!

Trott commented Mar 2, 2022

Uh oh!

Trott commented Mar 2, 2022

Uh oh!

arcanis commented Mar 10, 2022

Uh oh!

aduh95 commented Mar 10, 2022

Uh oh!

arcanis commented Mar 10, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 commented Mar 10, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

unerh commented Apr 6, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 commented Apr 6, 2022

Uh oh!

mcollina commented Apr 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Trott commented Feb 26, 2022•
edited
Loading

RaisinTen commented Feb 26, 2022•
edited
Loading

Trott commented Feb 26, 2022•
edited
Loading

RaisinTen commented Feb 27, 2022•
edited
Loading

Trott commented Feb 27, 2022•
edited
Loading

arcanis commented Mar 10, 2022•
edited
Loading

aduh95 commented Mar 10, 2022•
edited
Loading

unerh commented Apr 6, 2022•
edited
Loading