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

http2: remove square brackets from parsed hostname #28406

Closed
wants to merge 3 commits into from

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Jun 24, 2019

Make http2.connect() work when using URLs with literal IPv6
addresses.

Fixes: #28216

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

Make `http2.connect()` work when using URLs with literal IPv6
addresses.

Fixes: nodejs#28216
@nodejs-github-bot nodejs-github-bot added the http2 Issues or PRs related to the http2 subsystem. label Jun 24, 2019
@nodejs-github-bot
Copy link
Collaborator

@lpinca lpinca added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 25, 2019
@@ -2774,7 +2774,18 @@ function connect(authority, options, listener) {
const protocol = authority.protocol || options.protocol || 'https:';
const port = '' + (authority.port !== '' ?
authority.port : (authority.protocol === 'http:' ? 80 : 443));
const host = authority.hostname || authority.host || 'localhost';
let host = '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like host can be default to localhost and remove the last else ?

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@lpinca
Copy link
Member Author

lpinca commented Jul 1, 2019

Landed in 26b048e.

@lpinca lpinca closed this Jul 1, 2019
@lpinca lpinca deleted the gh-28216 branch July 1, 2019 16:27
lpinca added a commit that referenced this pull request Jul 1, 2019
Make `http2.connect()` work when using URLs with literal IPv6
addresses.

Fixes: #28216

PR-URL: #28406
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
targos pushed a commit that referenced this pull request Jul 2, 2019
Make `http2.connect()` work when using URLs with literal IPv6
addresses.

Fixes: #28216

PR-URL: #28406
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
targos pushed a commit that referenced this pull request Jul 2, 2019
Make `http2.connect()` work when using URLs with literal IPv6
addresses.

Fixes: #28216

PR-URL: #28406
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@targos targos mentioned this pull request Jul 2, 2019
@murgatroid99
Copy link
Contributor

Is there any chance of this getting backported to node 10?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

http2.connect is Not Accepting IPv6 Addresses
9 participants