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

url: drop auth in `url.resolve()` if host changes #1480

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
8 participants
@rlidwka
Copy link
Contributor

rlidwka commented Apr 20, 2015

#1435

Not sure how to handle this though.

@Fishrock123 Fishrock123 added the url label Apr 20, 2015

@brendanashworth

This comment has been minimized.

Copy link
Member

brendanashworth commented May 6, 2015

I wonder what should happen in this case?

url.resolve('mailto:user@example.org', 'example.com')
'mailto:user@example.com'

"user@" technically is auth info here.

I don't think it should copy over. However, to change that would be semver-major imo. This would be better:

> var parsed = url.parse('mailto:user@example.org');
undefined
> parsed.host = 'example.com';
'example.com'
> url.format(parsed);
'mailto:user@example.com'

@bnoordhuis bnoordhuis force-pushed the nodejs:master branch to b926718 Jun 2, 2015

@rvagg rvagg force-pushed the nodejs:master branch to 628a3ab Jun 25, 2015

@brendanashworth

This comment has been minimized.

Copy link
Member

brendanashworth commented Jul 21, 2015

perhaps @domenic would be interested in reviewing?

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Jul 21, 2015

What do browsers do? What does the spec (perhaps best tested via https://github.com/jsdom/whatwg-url) do?

@trevnorris

This comment has been minimized.

Copy link
Contributor

trevnorris commented Jul 22, 2015

I agree with @domenic. Our url module should align with the spec.

@indutny indutny force-pushed the nodejs:master branch to eb35968 Jul 22, 2015

@rvagg rvagg force-pushed the nodejs:master branch from 11c25c2 to ba02bd0 Sep 6, 2015

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Oct 22, 2015

Looks like this was never resolved. There's really no question that the user id and password should not be getting copied over.. url.resolve('http://user:pass@example.org', 'http://example.com') should never resolve out to http://user:pass@example.com. AFAICT, that aligns with the url spec also.

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Nov 16, 2015

@dougwilson

This comment has been minimized.

Copy link
Member

dougwilson commented Nov 16, 2015

I concur with @jasnell and this PR

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Apr 22, 2016

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Apr 22, 2016

@nodejs/ctc ... amazingly, this PR was opened a year ago and still applies cleanly (albeit using a three way merge). It even passes linting! The change LGTM.

marked it semver-major because it changes the behavior of url.resolve to drop the auth but it could also be classified as a bug fix. PTAL

@jasnell jasnell added this to the 6.0.0 milestone Apr 22, 2016

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Apr 23, 2016

CI is green!

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Apr 25, 2016

@mscdex @cjihrig @trevnorris ... can one of you give this a quick glance over?

'http://diff:auth@www.example.com/']
'http://diff:auth@www.example.com/'],

// https://github.com/iojs/io.js/issues/1435

This comment has been minimized.

@mscdex

mscdex Apr 25, 2016

Contributor

This should be changed to point to the nodejs/node repo

This comment has been minimized.

@jasnell

jasnell Apr 25, 2016

Member

Yep, I was going to change that upon landing (although, I kinda like that it still points to iojs, lol)

@mscdex

This comment has been minimized.

Copy link
Contributor

mscdex commented Apr 25, 2016

LGTM

jasnell added a commit that referenced this pull request Apr 25, 2016

url: drop auth in `url.resolve()` if host changes
Fixes: #1435
PR-URL: #1480
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Apr 25, 2016

It only took 1 year and 5 days but this landed in eb4201f ;-)

@jasnell jasnell closed this Apr 25, 2016

jasnell added a commit that referenced this pull request Apr 26, 2016

url: drop auth in `url.resolve()` if host changes
Fixes: #1435
PR-URL: #1480
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.