nodejs/node

Sending a body with a GET request using http.request cannot be received using http.server

jameshartig opened this issue · 12 comments

jameshartig commented

When you send a GET request using http.request to a Node.js server using the built-in http server, the parser throws an error:

HTTP 14997: parse error
NET 14997: destroy { [Error: Parse Error] bytesParsed: 87, code: 'HPE_INVALID_METHOD' }

Presumably because there was no Content-Length or Transfer-Encoding header. There was neither one of those headers because the http client code sets useChunkedEncodingByDefault to false when the method is GET. The parser hits the end of the headers and then immediately starts assuming any more data is a new HTTP request instead of it possibly being the body.

If you make the request and explicitly set the Content-Length using setHeader then no parse error is thrown. The HTTP spec says:

A message-body MUST NOT be included in a request if the specification of the request method (section 5.1.1) does not allow sending an entity-body in requests.
But as far as I can tell, only HEAD explicitly states it cannot have a body. The server is following the spec, however:
The presence of a message-body in a request is signaled by the inclusion of a Content-Length or Transfer-Encoding header field in the request's message-headers.

I'm not sure what the fix is here since turning on chunked encoding for GET might make GET's slower and I also realize this is an unintended use case (or bug in user-land) but I thought I'd bring it up.

mscdex commented

Can you provide a complete example to reproduce the issue?

Trott commented

HPE_INVALID_METHOD means that an invalid method (like GEM or GETA rather than GET) was received. Sample code triggering the error would be immensely helpful.

jameshartig commented

Here's a quick and easy reproduction case:
https://gist.github.com/fastest963/048c6108c83f46e77eaa

@Trott The reason its throwing HPE_INVALID_METHOD was mentioned originally but I'll reiterate: Since the parser didn't get a Content-Length or Transfer-Encoding it immediately ends after reading the second \r\n and then it starts assuming the next bytes are another request instead of assuming that its the body from the first request.

Trott commented

The reproduction code clarifies everything. Thanks for doing that.

I can definitely see a case for considering this a bug in the userland code and not in Node. But even so, I do wonder if the error message can at least be improved or clarified somehow. I'm not sure there's an easy way to do that without possibly hitting false positives, though.

mscdex commented

IMHO this is behaving as expected.

tflanagan commented

I know quite a few api services that accept bodies on GET requests (where clauses, etc). If I were to implement those services in nodejs, this would fail then?

jameshartig commented

@tflanagan If you sent a Content-Length or Transfer-Encoding then it would still work. I mainly filed the because there's a "bug" when requesting and receiving via the default APIs in node. If you're using a non-node client and it sends one of those headers then you're fine.

jasnell commented

From RFC7231: A payload within a GET request message has no defined semantics; sending a payload body on a GET request might cause some existing implementations to reject the request.

Sending a body in a GET has always led to undefined behavior. As far as I can tell, there's no bug here. Closing but can reopen if necessary. /cc @nodejs/http

inakiabt commented

FYI, in my case I was sending body data in a GET request without sending Content-Length.

bnoordhuis commented

@pagalasoujanya Don't hijack unrelated issues.

borisovg commented

(Note: I hate having to bring this up, but it's just that I wasted a few hours today troubleshooting why an API returned a different result when I used curl vs node client.)

From RFC7231

A payload within a GET request message has no defined semantics;
   sending a payload body on a GET request might cause some existing
   implementations to reject the request.

This does not read to me as a prohibition (certainly less so than language in earlier RFC) and considering that there are major applications (e.g. Elasticsearch) that use GET with body for queries, node should add a Content-Length header on a GET request with a body.

  • 👍1