node icon indicating copy to clipboard operation
node copied to clipboard

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

Open Narasimha1997 opened this issue 4 years ago • 8 comments

The bug posted by @szmarczak is because of not checking the truthy value of options.servername in line 3101 and 3102 of lib/internal/http2/core.js, instead the options.servername is strictly checked against undefined, any value other than undefined will give undesirable result in this case.

So in this PR,

if (servername !== undefined && options.servername === undefined)
    options.servername = servername;

is changed to,

if (servername !== undefined && !options.servername)
    options.servername = servername;

Here, the truthy value of options.servername is checked rather than strict check against undefined.

I have built node.js locally and checked against the code posted by @szmarczak to reproduce the bug.

Narasimha1997 avatar Aug 29 '21 16:08 Narasimha1997

Could we add a test for the issue?

Ayase-252 avatar Aug 29 '21 16:08 Ayase-252

@Ayase-252 yeah that would be great. I'll look into how tests are organised in this project so I can write few cases.

Narasimha1997 avatar Aug 29 '21 17:08 Narasimha1997

@Ayase-252 I have added test cases.

Narasimha1997 avatar Aug 29 '21 18:08 Narasimha1997

@Narasimha1997 Lint fails. The commit message check fails too.

szmarczak avatar Aug 29 '21 20:08 szmarczak

Commit message: https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#commit-message-guidelines. Naming test files: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#naming-test-files.

Mesteery avatar Aug 29 '21 21:08 Mesteery

@Mesteery @szmarczak @Ayase-252 Well, haha. Last time I made a contribution to Node an year ago, this commit message linting was not there. So what would you suggest? Can you please give an example of how issue fix commit message must be formulated?

Narasimha1997 avatar Aug 30 '21 03:08 Narasimha1997

@Narasimha1997

Maybe

http2: set origin name correctly when servername is empty

Fixes: https://github.com/nodejs/node/issues/39919

Ayase-252 avatar Aug 30 '21 04:08 Ayase-252

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.

Narasimha1997 avatar Aug 31 '21 02:08 Narasimha1997