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

Invalid HTTP/2 origin set when servername is empty #39919

Closed
szmarczak opened this issue Aug 27, 2021 · 2 comments · Fixed by #42838
Closed

Invalid HTTP/2 origin set when servername is empty #39919

szmarczak opened this issue Aug 27, 2021 · 2 comments · Fixed by #42838
Labels
confirmed-bug Issues with confirmed bugs. http2 Issues or PRs related to the http2 subsystem.

Comments

@szmarczak
Copy link
Member

szmarczak commented Aug 27, 2021

Version

v16.8.0

Platform

Linux solus 5.13.12-193.current #1 SMP PREEMPT Fri Aug 20 14:21:44 UTC 2021 x86_64 GNU/Linux

Subsystem

http2

What steps will reproduce the bug?

const http2 = require('http2');

const session = http2.connect('https://1.1.1.1', {servername: ''});
session.once('remoteSettings', () => {
  console.log(session.originSet);

  session.close();
});

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior?

[ 'https://1.1.1.1' ]

What do you see instead?

[ 'https://false' ]

Additional information

const http2 = require('http2');

const session = http2.connect('https://1.1.1.1');
session.once('remoteSettings', () => {
  console.log(session.originSet);

  session.close();
});

gives a warning

(node:36333) [DEP0123] DeprecationWarning: Setting the TLS ServerName to an IP address is not permitted by RFC 6066. This will be ignored in a future version.
(Use `node --trace-deprecation ...` to show where the warning was created)
[ 'https://1.1.1.1' ]

/cc @ronag

@Mesteery Mesteery added confirmed-bug Issues with confirmed bugs. http2 Issues or PRs related to the http2 subsystem. labels Aug 28, 2021
@Narasimha1997
Copy link
Contributor

@szmarczak
According to the additional information you have provided, this is the intended behaviour, the server name is supposed to be a valid domain name in the SSL certificate. Using IP addresses in the SSL certificate is not encouraged.

However, this is just an additional information. The original bug remains a bug. Maybe somewhere the empty servername property is being substituted without checking its value, which is resulting in https://false. Weird haha.

@Narasimha1997
Copy link
Contributor

#39934 seems to fix this bug.

Narasimha1997 added a commit to Narasimha1997/node that referenced this issue Aug 30, 2021
ofirbarak pushed a commit to ofirbarak/node that referenced this issue Apr 23, 2022
ofirbarak pushed a commit to ofirbarak/node that referenced this issue May 22, 2022
nodejs-github-bot pushed a commit that referenced this issue May 25, 2022
Fixes: #39919
Refs: #39934

PR-URL: #42838
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
bengl pushed a commit that referenced this issue May 30, 2022
Fixes: #39919
Refs: #39934

PR-URL: #42838
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
danielleadams pushed a commit that referenced this issue Jun 27, 2022
Fixes: #39919
Refs: #39934

PR-URL: #42838
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this issue Jul 12, 2022
Fixes: #39919
Refs: #39934

PR-URL: #42838
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this issue Jul 31, 2022
Fixes: #39919
Refs: #39934

PR-URL: #42838
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this issue Oct 10, 2022
Fixes: nodejs/node#39919
Refs: nodejs/node#39934

PR-URL: nodejs/node#42838
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants