Skip to content

fix(http2): check content-length, fix sending RST

Rodrigo Muino Tomonari requested to merge github/fork/szmarczak/fix-http2 into main

Fixes #35209 Closes #35378

Time spent: 60h

[!CAUTION] These bug fixes are potentially semver-major!

  1. Fixed stream.close(NGHTTP2_CANCEL) closing with 0 aka NGHTTP2_NO_ERROR.
  2. serverStream.destroy() closed with 0, now NGHTTP2_INTERNAL_ERROR.
  3. clientStream.destroy() closed with 0, now NGHTTP2_CANCEL.
  4. Mismatched content-length now throws NGHTTP2_PROTOCOL_ERROR as according to the spec.
  5. Fixed GOAWAY (server -> client) being greeted with GOAWAY (client -> server) as it's against the spec.
  6. Fixed streams being always closed with NGHTTP2_CANCEL on socket close, now respects goawayCode and destroyCode. For client, the default remains NGHTTP2_CANCEL. For server, the default now is NGHTTP2_INTERNAL_ERROR.
  7. Fixed stream.rstCode being 0 while active - docs say it should be undefined.
  8. Fixed preferring sessionCode over rstCode when destroying a stream with an error.
  9. Fixed streams being closed with NO_ERROR if session received GOAWAY with NO_ERROR and remote closed connection.
  10. Fixed streams being closed with NO_ERROR if session sent GOAWAY with NO_ERROR and destroyed.
  11. Fixed GOAWAY preventing RST_STREAM from being sent before GOAWAY. nghttp2 correctly assumes that it should prevent RST_STREAM from being sent but incorretly applies it to all frames in a packet instead of frames defined after GOAWAY. This is not the only thing that nghttp2 does wrong:
  12. Fixed benchmark/http2/headers.js calling client.destroy() (resulting in dropped requests). Now calls client.close() which closes gracefully.
  13. connectStreamSocket.destroy() now closes with NGHTTP2_CONNECT_ERROR.
  14. Fixed double GOAWAY on session.close().
  15. Fixed queued RST_STREAM and GOAWAY being dropped if Http2Session::Close() and previous writes weren't finished yet.
  16. Fixed stream frame errors closing the entire session instead of just the stream.
  17. Fixed stream frame errors sending RST_STREAM on idle streams (to reproduce 16. needs to be fixed).

Sorry so many bugs are fixed in a single PR but it's impossible to fix one without fixing them all!

Best if https://github.com/nghttp2/nghttp2/pull/1508 got merged before this, but it has been open for years 😢 Hence the patch for nghttp2_session.c

/cc @jasnell

Merge request reports