http2: fix a error in the check for frame->hd.type (related to CVE-2025-23085)

[hgqxjj]

http2: fix a typo in the check for frame->hd.type in 1b693fa

According to the description above, this should be checking
whether frame->hd.type is NGHTTP2_GOAWAY,
and the value of NGHTTP2_GOAWAY is 0x07. However,
it is written as 0x03 here, which I think it is a typo.

[nodejs-github-bot]

Review requested:

• @nodejs/http2
• @nodejs/net

[RafaelGSS]

That's correct. Could you please add an additional test assertion to this? See the tests introduced by this change in 1b693fa

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.22%. Comparing base (1123585) to head (2279ad5).
Report is 91 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_http2.cc	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57644      +/-   ##
==========================================
- Coverage   90.22%   90.22%   -0.01%     
==========================================
  Files         630      630              
  Lines      185055   185055              
  Branches    36216    36212       -4     
==========================================
- Hits       166975   166965      -10     
- Misses      11042    11050       +8     
- Partials     7038     7040       +2     

Files with missing lines	Coverage Δ
src/node_http2.cc	84.63% <0.00%> (-0.25%)	⬇️

... and 22 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[hgqxjj]

@RafaelGSS hi RafaelGSS, I have reviewed all your test files in 1b693fa, and it seems that none of them cover the Http2Session::OnFrameNotSent function test. I attempted to write a test for it, but it is quite difficult to trigger Http2Session::OnFrameNotSent with frame->hd.type equal to 0x07. Do you have any good ideas on how to achieve this? Alternatively, is it possible to merge this patch first if you think this modification is urgent.

[RafaelGSS]

@hgqxjj, it seems your change is breaking test-http2-premature-close. This test is historically known for being flaky, so I'll re-run the suite just to make sure.

[hgqxjj]

@RafaelGSS I think my change is unlikely to touch test-http2-premature-close. Maybe it still be flaky, haha

[hgqxjj]

@RafaelGSS Unfortunately, the test failed again due to a timeout. I conducted the same test using different versions of Node.js in my side. When running test-http2-premature-close.js with Node.js 14.19, the program terminates as expected. However, when testing with Node.js 20.12, the program gets stuck.

Based on my analysis of the test-http2-premature-close.js script combined with debug logs, there are two errors in the client's header frame content.

In Node.js 14.19, these errors cause the server to send an RST_STREAM upon receiving the header frame. For some reason, this also triggers the onFrameError function, leading to the session being closed, which subsequently triggers server.close(), allowing the program to terminate.

However, in Node.js 20.12, server seems to have ignored these errors and did not send RST_STREAM , and onFrameError is not triggered, and server.close() is not called, causing the program to hang. Therefore, the test failure does not seem to be related to my modifications but rather an issue with test-http2-premature-close.js itself.

[hgqxjj]

@RafaelGSS hi RafaelGSS，i already correct header frame content according to http2 related protocol so that make it exited successfully，please re-running the test suite to make sure at your convenience .

[hgqxjj]

hi @RafaelGSS ，currently, all tests have passed. Could you please take some time to review this patch ? Thanks in advance

[github-actions]

Failed to start CI

   ⚠  No approving reviews found
   ✘  Refusing to run CI on potentially unsafe PR

https://github.com/nodejs/node/actions/runs/14248886630

[mcollina]

lgtm, but this should wait @RafaelGSS approval too.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/66025/

[RafaelGSS]

I will review this PR carefully tomorrow. I'll double check the memory leak.

[hgqxjj]

hi @RafaelGSS ， I'll paste some screenshots below to help explain the changes I made.
RFC7541 link is https://datatracker.ietf.org/doc/html/rfc7541#appendix-A