Fix: Invalid HTTP/2 origin set when servername is empty #39919 #39934
Conversation
nodejs-github-bot
added
http2
|
Could we add a test for the issue? |
|
@Ayase-252 yeah that would be great. I'll look into how tests are organised in this project so I can write few cases. |
|
@Ayase-252 I have added test cases. |
|
@Narasimha1997 Lint fails. The commit message check fails too. |
There was a problem hiding this comment.
We could also run make -j4 test to do a full check on the codebase locally. Per https://github.com/nodejs/node/blob/master/BUILDING.md#running-tests
|
@Mesteery @szmarczak @Ayase-252 |
|
Maybe |
Narasimha1997
force-pushed
the
fix-invalid-origin-set
branch
from
74b149d
to
c43b614
Compare
|
The build on windows has failed due to some issues while fetching NASM. Can we re-run the build? Now it seems to be working fine. |
| @@ -3098,7 +3098,7 @@ function initializeTLSOptions(options, servername) { | |||
| options.ALPNProtocols = ['h2']; | |||
| if (options.allowHTTP1 === true) | |||
| ArrayPrototypePush(options.ALPNProtocols, 'http/1.1'); | |||
| if (servername !== undefined && options.servername === undefined) | |||
| if (servername !== undefined && !options.servername) | |||
There was a problem hiding this comment.
I don't know it makes sense for http2 but for https the empty string is used to disable the SNI extension. I wonder if the same should be done here.
cc: @nodejs/http2
There was a problem hiding this comment.
Thanks. Yeah that makes sense. This PR is based in what the issue poster has expected to be the intended behaviour. If you confirm SNI should be disabled, I would be happy to make these changes.
| } | ||
|
|
||
| function withServerName() { | ||
| const session = http2.connect('https://1.1.1.1', { servername: 'cloudflare-dns.com' }); |
There was a problem hiding this comment.
This requires an external connection. Is it possible to use localhost? See #39011.
Edit: This is not a blocker. The test is under test/internet so it should be ok as is.
Fixes: #39919 Refs: #39934 PR-URL: #42838 Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Fixes: #39919 Refs: #39934 PR-URL: #42838 Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Fixes: #39919 Refs: #39934 PR-URL: #42838 Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Fixes: #39919 Refs: #39934 PR-URL: #42838 Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Fixes: #39919 Refs: #39934 PR-URL: #42838 Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Fixes: nodejs/node#39919 Refs: nodejs/node#39934 PR-URL: nodejs/node#42838 Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
The bug posted by @szmarczak is because of not checking the truthy value of
options.servernamein line 3101 and 3102 oflib/internal/http2/core.js, instead theoptions.servernameis strictly checked againstundefined, any value other thanundefinedwill give undesirable result in this case.So in this PR,
is changed to,
Here, the truthy value of
options.servernameis checked rather than strict check againstundefined.I have built node.js locally and checked against the code posted by @szmarczak to reproduce the bug.