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.connect(...) - overriding authority with options #28182

Closed
rishabh opened this issue Jun 11, 2019 · 1 comment
Closed

http2.connect(...) - overriding authority with options #28182

rishabh opened this issue Jun 11, 2019 · 1 comment
Labels
http2 Issues or PRs related to the http2 subsystem.

Comments

@rishabh
Copy link

rishabh commented Jun 11, 2019

  • Version: v10.15.3
  • Platform: Darwin
  • Subsystem: http2

There are some inconsistencies with what the documentation states and what the http2 actually does regarding the http2.connect(authority, options, ...) method.

The documentation states that the method accepts both net.connect(...) and tls.connect(...) options, but if you pass in an authority object with the connection details and an options object with extra details, the method is inconsistent when deciding if authority should take precedence over options depending on if it uses net.connect(...) and tls.connect(...).

Assume that a server is running on localhost:1050, then consider:

const client = http2.connect('http://localhost:1050', { host: 'broken', port: 1337 })

and

const client = http2.connect('https://localhost:1050', { host: 'broken', port: 1337 })

In the first scenario, the connection will be successful since net.connect(...) uses the host and port in the authority arg, and ignores the options arg (if they're already defined in authority).

In the second scenario, since the connection is using tls.connect(...) instead of net.connect(...), the options object will take precedence over authority.

@sam-github
Copy link
Contributor

That sounds like an inconsistency between the underlying tls.connect and net.connect which should be fixed. Want to try?

@lpinca lpinca added the http2 Issues or PRs related to the http2 subsystem. label Jun 30, 2019
lpinca added a commit to lpinca/node that referenced this issue Jul 7, 2019
Make `options.host` and `options.port` take precedence over
`authority.host` and `authority.port` respectively.

Fixes: nodejs#28182
@lpinca lpinca closed this as completed Jul 10, 2019
lpinca added a commit that referenced this issue Jul 10, 2019
Make `options.host` and `options.port` take precedence over
`authority.host` and `authority.port` respectively.

PR-URL: #28584
Fixes: #28182
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this issue Jul 20, 2019
Make `options.host` and `options.port` take precedence over
`authority.host` and `authority.port` respectively.

PR-URL: #28584
Fixes: #28182
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants