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

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

Merged
merged 5 commits into from Jul 18, 2018
Merged

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

merged 5 commits into from Jul 18, 2018

Conversation

paambaati
Copy link
Contributor

@paambaati paambaati commented Apr 4, 2018

@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

This partially revert ff6d6c6, and still works for IPv6 addresses as well.
@paambaati
Copy link
Contributor Author

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

@paambaati
Copy link
Contributor Author

paambaati commented Apr 10, 2018

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

@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

@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

Sorry, guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants