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

HEAD requests returning non-empty entity body #369

Closed
bnordbo opened this Issue Apr 22, 2015 · 6 comments

Comments

Projects
None yet
3 participants
@bnordbo

bnordbo commented Apr 22, 2015

I suspect there could be an issue in warp 3.0.10.1 and onwards with regard to how responses to HEAD requests are handled. For instance, consider this server:

main :: IO ()
main = run 1234 $ \_ rsp -> rsp $ responseLBS status200 [] empty

When issuing the following HTTP request:

echo -ne "HEAD / HTTP/1.1\r\nHost: localhost\r\n\r\n" | nc localhost 1234

The server will respond as follows:

HTTP/1.1 200 OK
Transfer-Encoding: chunked
Date: Wed, 22 Apr 2015 10:09:03 GMT
Server: Warp/3.0.10.1

0

As far as I can see (RFC 2616 4.3 and 9.4), the last-chunk marker should not be present in this case. If used from an application that re-uses connections, the superfluous 0 will typically be kept in the receive buffer, and trip up any subsequent request with an InvalidStatusLine "0" error.

It seems this issue was introduced by 699b9f8 in relation to ensuring that the response headers for a HEAD request matches the ones for a GET.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Apr 22, 2015

Member

Pinging @kazu-yamamoto, can you have a look at this?

Member

snoyberg commented Apr 22, 2015

Pinging @kazu-yamamoto, can you have a look at this?

@kazu-yamamoto

This comment has been minimized.

Show comment
Hide comment
@kazu-yamamoto

kazu-yamamoto Apr 24, 2015

Contributor

The patch above solves this problem.

@snoyberg Would you review this patch?

Contributor

kazu-yamamoto commented Apr 24, 2015

The patch above solves this problem.

@snoyberg Would you review this patch?

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Apr 24, 2015

Member

Looks good to me. @bnordbo can you confirm?

Member

snoyberg commented Apr 24, 2015

Looks good to me. @bnordbo can you confirm?

@bnordbo

This comment has been minimized.

Show comment
Hide comment
@bnordbo

bnordbo Apr 24, 2015

@snoyberg, @kazu-yamamoto, yes, it looks good. Thanks for the quick response!

bnordbo commented Apr 24, 2015

@snoyberg, @kazu-yamamoto, yes, it looks good. Thanks for the quick response!

@bnordbo

This comment has been minimized.

Show comment
Hide comment
@bnordbo

bnordbo Apr 24, 2015

I also applied the patch, and my test case passes now. I notice that it also causes the Transfer-Encoding header to be removed from the HEAD response. This is permitted according to RFC 2616, section 9.4, which states that:

The metainformation contained in the HTTP headers in response to a HEAD request SHOULD be identical to the information sent in response to a GET request.

bnordbo commented Apr 24, 2015

I also applied the patch, and my test case passes now. I notice that it also causes the Transfer-Encoding header to be removed from the HEAD response. This is permitted according to RFC 2616, section 9.4, which states that:

The metainformation contained in the HTTP headers in response to a HEAD request SHOULD be identical to the information sent in response to a GET request.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Apr 24, 2015

Member

Awesome, thanks for the report and testing, and thanks for fixing @kazu-yamamoto. I've released to Hackage as 3.0.12.1.

Member

snoyberg commented Apr 24, 2015

Awesome, thanks for the report and testing, and thanks for fixing @kazu-yamamoto. I've released to Hackage as 3.0.12.1.

@snoyberg snoyberg closed this Apr 24, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment