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

Correctly format the Host header for IPv6 addresses #2571

Merged
merged 1 commit into from Mar 4, 2017
Merged

Correctly format the Host header for IPv6 addresses #2571

merged 1 commit into from Mar 4, 2017

Conversation

JamesMGreene
Copy link
Contributor

Fixes #2292

PR Checklist:

  • I have run npm test locally and all tests are passing.
  • I have added/updated tests for any new behavior.
  • If this is a significant change, an issue has already been created where the problem / solution was discussed: [N/A, or add link to issue here]

PR Description

When using IPv6 addresses in URLs, the address must be enclosed in brackets, e.g.https://[::1]:3000/headers.json

Likewise, when using IPv6 addresses in Host header values, the address again must be enclosed in brackets, e.g. Host: [::1]:3000

When the request module adds the Host header value, it does so using the hostname and port (if needed) parts of the parsed URL. However, this approach does not correctly format IPv6 addresses with the necessary enclosing brackets as hostname does not include them.

If you were instead to utilize the host portion of the parsed URL instead, you would automatically get the correctly bracketed value, potentially plus the port. My testing has not shown any case where the port was unnecessarily included in the host value so I have also avoided adding extra code to remove the port if deemed unnecessary by request, though I'd be happy to add such if anyone can prove it is needed.

@mikeal
Copy link
Member

mikeal commented Mar 4, 2017

Nice catch. request predates that field being correct in core's URL parser 👍

@mikeal mikeal merged commit bce66a5 into request:master Mar 4, 2017
@JamesMGreene
Copy link
Contributor Author

JamesMGreene commented Mar 4, 2017

@mikeal: Any chance you know which versions of Node would not correctly format this due to the URL parser?

@JamesMGreene
Copy link
Contributor Author

JamesMGreene commented Mar 4, 2017

FWIW, I tested this in Node 0.12.17 and it seems to work correctly.

P.S. Thanks for the quick response and merging. Any idea when a new release will be published?

@mikeal
Copy link
Member

mikeal commented Mar 4, 2017

Honestly, this was probably fixed like 5 years ago in core :)

Request was initially written to Node.js 0.1.x, it pre-dates npm :)

@JamesMGreene JamesMGreene deleted the fix_ipv6_host_header branch March 4, 2017 03:59
@JamesMGreene
Copy link
Contributor Author

I tested this against Node 0.10.48 just for the heck of it but it looked like there were some other issues with the tests against that version.

@JamesMGreene
Copy link
Contributor Author

That's awesome that this module pre-dates NPM, LOL. 😮

One more time: any idea when a new release of "request" will be published? We're dependent on this fix at work for an upcoming release in April.

@mikeal
Copy link
Member

mikeal commented Mar 4, 2017

v2.80.0

@JamesMGreene
Copy link
Contributor Author

Thanks so much! 💝

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

Successfully merging this pull request may close these issues.

IPv6 address misinterpreted
2 participants