Open zalupoi opened 1 year ago
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?
cc @nodejs/undici
cc @nodejs/llhttp
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?
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.
I did some digging and on the C++ side node already calls llhttp_set_lenient_headers()
when JS passes in the right flags:
https://github.com/nodejs/node/blob/d3b0a2a68b5cb03de1f254cefa46f4c4023b1f89/lib/_http_server.js#L638-L644
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().
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
errorAdditional information
I write code for ESP32 using ESPAsyncWebServer library and its
request->beginChunkedResponse()
chunked respose feature.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'sfetch()
Chunks sizes are written here https://github.com/me-no-dev/ESPAsyncWebServer/blob/master/src/WebResponses.cpp#L312