Possible bug in fetch() receiving chunked response

[zalupoi]

Version

Tested v18.15.0 and v20.0.0-nightly2023041197d3912eb8

Platform

Microsoft Windows NT 10.0.19042.0 x64

Subsystem

No response

What steps will reproduce the bug?

Launch server.js
Launch client.js

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

No condition required.

What is the expected behavior? Why is that the expected behavior?

Script should display response from server.

What do you see instead?

Script crashes with HPE_INVALID_CHUNK_SIZE error

node:internal/deps/undici/undici:11279
            fetchParams.controller.controller.error(new TypeError("terminated", {
                                                    ^

TypeError: terminated
    at Fetch.onAborted (node:internal/deps/undici/undici:11279:53)
    at Fetch.emit (node:events:513:28)
    at Fetch.terminate (node:internal/deps/undici/undici:10534:14)
    at Object.onError (node:internal/deps/undici/undici:11374:36)
    at Request.onError (node:internal/deps/undici/undici:8168:31)
    at errorRequest (node:internal/deps/undici/undici:10220:17)
    at Socket.onSocketClose (node:internal/deps/undici/undici:9668:9)
    at Socket.emit (node:events:513:28)
    at TCP.<anonymous> (node:net:322:12) {
  [cause]: HTTPParserError: Invalid character in chunk size
      at Parser.execute (node:internal/deps/undici/undici:9351:19)
      at Parser.resume (node:internal/deps/undici/undici:9301:14)
      at Readable.read (node:internal/streams/readable:496:12)
      at createAsyncIterator (node:internal/streams/readable:1122:54)
      at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
      at async fetchParams.controller.resume (node:internal/deps/undici/undici:11239:37) {
    code: 'HPE_INVALID_CHUNK_SIZE',
    data: ' \r\n' +
      '10419,10421,10423,10425,10431,10433,10449,10450,10452,10457,10459,10465,10467,10469,10471,10481,10483,10485,10487,10489,10491,10493,10495,10497,10499,10501,10503,10505,10507,10509,10511,10513,10515,10517,10519,10521,10523,10525,10527,10529,10531,10533,10535,10537,10539,10541,10543,10545,10547,10549,10551,10553,10555,10557,10559,10561,10563,10565,10567,10569,10571,10573,10575,10577,10579,10581,10583,10585,10587,10589,10591,10593,10595,10597,10599,10601,10603,10605,10607,10609,10611,10613,10615,10617,10619,10621,10623,10625,10627,10629,10631,10633,10635,10637,10639,10641,10643,10645,10647,10649,10651,10653,10655,10657,10659,10661,10663,10665,10667,10669,10671,10673,10675,10677,10679,10681,10683,10685,10687,10689,10691,10693,10695,10697,10699,10701,10703,10705,10707,10709,10711,10713,10715,10717,10719,10721,10723,10725,10727,10729,10731,10733,10735,10737,10739,10741,10743,10745,10747,10749,10751,10753,10755,10757,10759,10761,10763,10765,10767,10769,10771,10773,10775,10777,10779,10781,10783,10785,10787,10789,10791,10793,10795,10797,10799,10801,10803,10805,10807,10809,10811,10813,10815,10822,10823,10838,10839,10841,10847,10849,10863,10865,10867,10869,10871,10877,10893,10893,10895,10897,10899,10904,10905,10907,10909,10911,10913,10915,10917,10919,10921,10923,10925,10927,10929,10931,10933,10935,10937,10939,10941,10943,10945,10947,10949,10951,10953,10955,10957,10959,10961,10963,10965,10967,10969,10971,10973,10975,10977,10979,10981,10983,10985,10987,10989,10991,10993,10995,10997,10999,11001,11003,11005,11007,11009,11011,11013,11015,11017,11019,11021,11023,11025,11027,11029,11031,11033,11035,11037,11039,11041,11043,11045,11047,11049,11051,11053,11055,11057,11059,11061,11063,11065,11067,11069,11071,11073,11075,11077,11079,11081,11083,11085,11087,11089,11091,11093,11095,11097,11099,11101,11103,11105,11107,11109,11111,11113,11115,11117,11119,11121,11123,11125,11127,11129,11131,11133,11135,11137,11139,11141,11143,11145,11147,11149,11151,11153,11155,11157,11159,11161,11163,11165,11167,11169,11171,11173,11175,11177,11179,11181,11183,11185,11187,11189,11191,11193,11195,11217,11219,11221,11223,11225,11227,11249,11251,11253,11255,11275,11276,11278,11280,11282,11285,11287,11289,11291,11293,11295,11297,11299,11301,11303,11305,11307,11309,11311,11331,11333,11335,11337,11339,11359,11361,11363,11365,11385,11387,11389,11391,11393,11412,11414,11416,11418,11426,11427,11429,11431,11433\n' +
      '\r\n' +
      '0   \r\n' +
      '\r\n'
  }
}

Additional information

I write code for ESP32 using ESPAsyncWebServer library and its request->beginChunkedResponse() chunked respose feature.

• It works in firefox.
• It works in chrome.
• It works in curl.
• It crashes in NodeJS.

I captured data using Wireshark and reproduced it in server.js.
You can open http://127.0.0.1:5000/ in web browser and it will work, but not in NodeJS's fetch()
Chunks sizes are written here https://github.com/me-no-dev/ESPAsyncWebServer/blob/master/src/WebResponses.cpp#L312

[zalupoi]

With help of ChatGPT I think I found the problem.
WebResponses.cpp has comment saying HTTP 1.1 allows leading zeros in chunk length. Or spaces may be added. See RFC2616 sections 2, 3.6.1.
Here what ChatGPT says

I'm sorry, but I must inform you that the RFC 2616 has been obsoleted and replaced by a set of new RFCs (RFC 7230-7237) in 2014. However, I can still provide you with the relevant information from the new HTTP/1.1 specification (RFC 7230).

Section 4.1 of RFC 7230 states that the chunk-size field in a chunked transfer encoding can be represented as a hexadecimal number, possibly followed by semi-colon and chunk extensions, and then followed by CRLF (carriage return and line feed) sequence.

The chunk-size is defined as a positive decimal number in Section 4.1 of RFC 7230, and it does not mention that spaces are allowed in the chunk-size. However, Section 3.6.1 of RFC 7230 states that "Leading zeros MUST be ignored by recipients and MUST NOT be sent." This means that leading zeros are allowed, but they should be ignored by the recipient.

Although RFC 7230 does not mention spaces in the chunk-size field, it is possible that some servers and clients may accept them as part of the chunk-size. However, it is recommended to follow the RFC and avoid adding spaces to the chunk-size field.

So after replacing
0x39, 0x36, 0x30, 0x20, 0x0d, 0x0a, 0x31, 0x30,
to
0x30, 0x39, 0x36, 0x30, 0x0d, 0x0a, 0x31, 0x30,
in peer1_16 array of server.js script
and
0x30, 0x20, 0x20, 0x20, 0x0d, 0x0a, 0x0d, 0x0a
to
0x30, 0x30, 0x30, 0x30, 0x0d, 0x0a, 0x0d, 0x0a
in peer1_18 array of server.js script
fetch() in NodeJS accepted response.

So its problem on ESPAsyncWebServer part but web brosers accepting trailing spaces in chunk sizes without errors.
Can we have same behaviour in NodeJS?

[panva]

cc @nodejs/undici

[KhafraDev]

cc @nodejs/llhttp

[ShogunPanda]

You are correct in saying the error is triggered by llhttp, which is strict by default (and thanks ChatGPT agres with us 😁 ).

Lenient handling of useless spaces can be added to llhttp as a semver minor change behing a disabled-by-default flag.

@nodejs/http Do we want/need this?

[bnoordhuis]

For reference: the old C-based parser was tightened up at one point to reject superfluous spaces in most places because they're a source of desync attacks. It accepts them in sloppy mode but not in strict mode. Strict is the default.

I believe llhttp copied that behavior from http_parser. Fedor was also the one who implemented the original behavior in http_parser, IIRC.

[bnoordhuis]

I did some digging and on the C++ side node already calls llhttp_set_lenient_headers() when JS passes in the right flags:

node/lib/_http_server.js

Lines 638 to 644 in d3b0a2a

	parser.initialize(
	HTTPParser.REQUEST,
	new HTTPServerAsyncResource('HTTPINCOMINGMESSAGE', socket),
	server.maxHeaderSize || 0,
	lenient ? kLenientAll : kLenientNone,
	server[kConnections],
	);

Node's http server does when you specify { lenient: true } but the client doesn't. fetch() of course is its own thing.

Pull request welcome for node's own http client on the provision that it doesn't change the default strict behavior. I don't think it should be overridable for fetch().