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: keep auth in `url.resolve()` if host matches #8215

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@imyller
Member

imyller commented Aug 21, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

url

Description of change

Keep auth in URL if host and hostname is not changed.

Fixes regression introduced in PR #1480

Fixes: #8165

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 22, 2016

Member

LGTM if CI is green

Member

jasnell commented Aug 22, 2016

LGTM if CI is green

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 23, 2016

Member

New CI: https://ci.nodejs.org/job/node-test-pull-request/3805/
red in the last run is most likely unrelated but just in case...

Member

jasnell commented Aug 23, 2016

New CI: https://ci.nodejs.org/job/node-test-pull-request/3805/
red in the last run is most likely unrelated but just in case...

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 23, 2016

Member

CI failures are unrelated. Landing...

Member

jasnell commented Aug 23, 2016

CI failures are unrelated. Landing...

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 23, 2016

Member

ugh.. nevermind... @imyller... can you rebase this really quick?

Member

jasnell commented Aug 23, 2016

ugh.. nevermind... @imyller... can you rebase this really quick?

url: keep auth in `url.resolve()` if host matches
Fixes: #8165

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
@imyller

This comment has been minimized.

Show comment
Hide comment
@imyller

imyller Aug 24, 2016

Member

@jasnell rebased

Member

imyller commented Aug 24, 2016

@jasnell rebased

@jasnell

This comment has been minimized.

Show comment
Hide comment
Member

jasnell commented Aug 24, 2016

jasnell added a commit that referenced this pull request Aug 24, 2016

url: keep auth in `url.resolve()` if host matches
Fixes: #8165

PR-URL: #8215
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 24, 2016

Member

Landed in 51f96df. Thank you!

Member

jasnell commented Aug 24, 2016

Landed in 51f96df. Thank you!

@jasnell jasnell closed this Aug 24, 2016

@Fishrock123 Fishrock123 referenced this pull request Sep 6, 2016

Closed

v6.6.0 pre-proposal #8428

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 8, 2016

url: keep auth in `url.resolve()` if host matches
Fixes: nodejs#8165

PR-URL: nodejs#8215
Reviewed-By: James M Snell <jasnell@gmail.com>

Fishrock123 added a commit that referenced this pull request Sep 9, 2016

url: keep auth in `url.resolve()` if host matches
Fixes: #8165

PR-URL: #8215
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Sep 30, 2016

Member

adding a don't land tag as this appears to be related to a semver major change not found on v4.x

Please feel free to change the label if I am mistaken

Member

MylesBorins commented Sep 30, 2016

adding a don't land tag as this appears to be related to a semver major change not found on v4.x

Please feel free to change the label if I am mistaken

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment