Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix zero length chunked response regression #256

Merged
merged 7 commits into from
Oct 27, 2023

Conversation

bdraco
Copy link
Contributor

@bdraco bdraco commented Oct 13, 2023

This test fails on 9.1.3 but passes on 8.1.1

related issues aio-libs/aiohttp#7697 home-assistant/core#101893 home-assistant/core#101913

@bdraco bdraco changed the title Add test case for zero length chunked response Add test case for zero length chunked response regression Oct 13, 2023
@cdce8p
Copy link

cdce8p commented Oct 13, 2023

Narrowed it down to this line (added in #245)

parser->status_code == 304 /* Not Modified */

Removing it here, resolves the issue.

/CC @ShogunPanda Would you mind taking a look?

@bdraco bdraco changed the title Add test case for zero length chunked response regression Fix zero length chunked response regression Oct 18, 2023
@bdraco
Copy link
Contributor Author

bdraco commented Oct 18, 2023

Looks like that fix causes other tests to fail https://github.com/bdraco/llhttp/actions/runs/6566967226/job/17838778913?pr=1

Digging...

@bdraco
Copy link
Contributor Author

bdraco commented Oct 18, 2023

Test existing test was missing the ending 0

https://github.com/bdraco/llhttp/actions/runs/6567209475?pr=1

@bdraco bdraco marked this pull request as ready for review October 18, 2023 22:22
@bdraco
Copy link
Contributor Author

bdraco commented Oct 23, 2023

@ShogunPanda Sorry for the ping, can you take a look at this PR?

Thanks!

Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

@ShogunPanda ShogunPanda merged commit 8acaf3c into nodejs:main Oct 27, 2023
8 checks passed
@tatsuhiro-t
Copy link
Contributor

RFC 9112 dictates that 304 response does not have message body.

Any response to a HEAD request and 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 or trailer section.

and

Transfer-Encoding MAY be sent in a response to a HEAD request or in a 304 (Not Modified) response (Section 15.4.5 of [HTTP]) to a GET request, neither of which includes a message body, to indicate that the origin server would have applied a transfer coding to the message body if the request had been an unconditional GET. This indication is not required, however, because any recipient on the response chain (including the origin server) can remove transfer codings when they are not needed.

It is allowed to send transfer-encoding header field in 304 response, but it must be ignored because the message body must be empty.

I think this PR should be reverted.

@bdraco
Copy link
Contributor Author

bdraco commented Oct 27, 2023

RFC 9112 dictates that 304 response does not have message body.

Any response to a HEAD request and 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 or trailer section.

and

Transfer-Encoding MAY be sent in a response to a HEAD request or in a 304 (Not Modified) response (Section 15.4.5 of [HTTP]) to a GET request, neither of which includes a message body, to indicate that the origin server would have applied a transfer coding to the message body if the request had been an unconditional GET. This indication is not required, however, because any recipient on the response chain (including the origin server) can remove transfer codings when they are not needed.

It is allowed to send transfer-encoding header field in 304 response, but it must be ignored because the message body must be empty.

I think this PR should be reverted.

Unfortunately this means this is a breaking change between 8.x and 9.x, but I agree with how you read the rfc.

bdraco added a commit to bdraco/llhttp that referenced this pull request Oct 27, 2023
ShogunPanda pushed a commit that referenced this pull request Oct 27, 2023
@ShogunPanda
Copy link
Contributor

Reverted in #262.
@tatsuhiro-t Thanks for flagging this, it completely slipped my radar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants