Skip to content

#515, #2894 Strip port suffix from Host header if the protocol is known. #2904

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.

Sign up for GitHub

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

Jump to bottom
Merged
mikeal merged 5 commits into from
Jul 18, 2018
Merged

#515, #2894 Strip port suffix from Host header if the protocol is known. #2904

mikeal merged 5 commits into from
Jul 18, 2018

Conversation

@paambaati
Copy link
Contributor

@paambaati paambaati commented Apr 4, 2018 •
edited
Loading

@mikeal This PR should fix #515 and #2894.

PR Checklist:

PR Description

It drops the port suffix in the Host header for standard protocol + port combinations (HTTP + :80 and HTTPS + :443). It partially reverts ff6d6c6, while still working for IPv6 addresses.

CC: @JamesMGreene, @simov

@paambaati
Strip port suffix from Host header if the protocol is known.
41cc05f 
This partially revert ff6d6c6, and still works for IPv6 addresses as well.
@paambaati paambaati mentioned this pull request Apr 4, 2018
Closed
@paambaati
Copy link
Contributor Author

paambaati commented Apr 10, 2018

@mikeal With 237231e, I've included tests for the changes 🎉

@paambaati
Copy link
Contributor Author

paambaati commented Apr 10, 2018 •
edited
Loading

Tests seem to be failing because of the # IPv6 Host header test - this is probably because Travis CI does not allow IPv6 addresses — see travis-ci/travis-ci#4964.

travis-ci/travis-ci#8361 (comment) proposes a possible fix, but would require using sudo.

Further reading - https://docs.travis-ci.com/user/reference/overview/#Virtualisation-Environment-vs-Operating-System

@paambaati
Copy link
Contributor Author

paambaati commented May 29, 2018

@mikeal @simov This is also very similar to the recent NPM 418 errors discussed here and here - npm/npm#20791

@mikeal
Copy link
Member

mikeal commented Jul 18, 2018

This must be a regression, we used to take care of this by using the property from url.parse that doesn't include the header on known protocols. Any idea when this might have happened?

Can you disable the IPv6 test since it doesn't work in Travis?

@paambaati
Copy link
Contributor Author

paambaati commented Jul 18, 2018

@mikeal I just checked, and the last CI runs are all green.

Any idea when this might have happened?

From what I can tell, this happened in #2571, here - https://github.com/request/request/pull/2571/files#diff-ccc0734f65dd7a299409ff07d35be095L293

@mikeal mikeal merged commit a92e138 into request:master Jul 18, 2018
@mikeal
Copy link
Member

mikeal commented Jul 18, 2018

Ah, thanks, too bad that regression snuck in :(

@JamesMGreene
Copy link
Contributor

JamesMGreene commented Jul 18, 2018

Sorry, guys!

@simov simov mentioned this pull request Aug 2, 2018
Merged
@sindresorhus sindresorhus mentioned this pull request Aug 24, 2018
Closed
This was referenced Mar 16, 2021
Open
Closed
@nbr nbr mentioned this pull request Aug 3, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Port 80 is included in the 'Host' request field (instead of defaulted) when it isn't with other popular HTTP clients

3 participants

@paambaati @mikeal @JamesMGreene