Skip to content
This repository has been archived by the owner on Nov 6, 2022. It is now read-only.

Always skip body for 1xx, 204, and 304 responses #464

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

azaretsky
Copy link

https://tools.ietf.org/html/rfc7230#section-3.3.3:
... any response with a 1xx (Informational), 204 (No Content), or 304 (Not Modified) status code is always terminated by the first empty line after the header fields, regardless of the header fields present in the message, and thus cannot contain a message body.

Fixes: #251

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the change LGTM in general I'm kind of worried it might break existing http-parser consumers due to the change in behavior.

http_parser.c Outdated Show resolved Hide resolved
@@ -1848,8 +1848,7 @@ const struct message responses[] =
,.http_minor= 1
,.status_code= 101
,.response_status= "Switching Protocols"
,.body= "body"
,.upgrade= "proto"
,.upgrade= "bodyproto"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #363, it's possible someone might be relying on the current behavior.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR only changes the tests for 101 responses. The HTTP_200_RESPONSE_WITH_UPGRADE_HEADER* tests are unaffected (and they all pass).

RFC 7230 explicitly forbids using Content-Length or Transfer-Encoding in 1xx responses ("A server MUST NOT send" etc.) and RFC 2616 also had similarly strict requirements ("All 1xx ... responses MUST NOT include a message-body"), so I think it would probably be ok.

@azaretsky
Copy link
Author

It's quite hard to tell if this behavior change will affect anyone since it seems that these responses are rarely encountered in the wild if at all: 1xx, 204, and 304 responses without a declared message body length are already correctly handled by the parser, while 1xx and 204 with a message body are not legitimate according to the spec, and real servers often do not include content-length or transfer-encoidng in 304 responses (at least IIS, nginx and apache httpd don't).

So there's a great chance that this change is harmless but also purely cosmetic and no-one actually needs it.

@bnoordhuis
Copy link
Member

cc @indutny - perhaps you can weigh in?

@indutny
Copy link
Member

indutny commented Apr 14, 2019

I think it might be too hard to propagate description this change to the consumers of http-parser. In order to properly do it, we'll have to do major release.

@nodejs/http : do we consider major release a possibility, or would we prefer to stick with llhttp and do majority of changes there instead?

@azaretsky azaretsky force-pushed the skip-body-in-1xx-204-304-responses branch from c253f0b to c70bd21 Compare May 4, 2019 14:38
@azaretsky
Copy link
Author

Just for the record: I will try to keep this branch rebased on top of the current master in case anyone wants to use it before the next major release.

"status_code >= 100 && status_code < 200" might be faster than
"status_code / 100 == 1"
@azaretsky azaretsky force-pushed the skip-body-in-1xx-204-304-responses branch from c70bd21 to b25158b Compare October 10, 2019 22:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HEAD, 1xx, 204, 304 have content-length but do not have body
3 participants