code review 7182045: net/http: fix Content-Length/Transfer-Encoding on HEAD ...

net/http: fix Content-Length/Transfer-Encoding on HEAD requests

net/http currently assumes that the response to a HEAD request
    will always have a Content-Length header. This is incorrect.

RFC2616 says: "The HEAD method is identical to GET except that
the server MUST NOT return a message-body in the response. 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. This method can be used for
obtaining metainformation about the entity implied by the
request without transferring the entity-body itself. This
method is often used for testing hypertext links for validity,
accessibility, and recent modification."

This means that three cases are possible: a Content-Length
header, a Transfer-Encoding header or neither. In the wild the
following sites exhibit these behaviours (curl -I):

HEAD on http://www.google.co.uk/ has Transfer-Encoding: chunked
HEAD on http://www.bbc.co.uk/    has Content-Length: 45247
HEAD on http://edition.cnn.com/  has neither header

This patch does not remove the ErrMissingContentLength error
for compatibility reasons, but it is no longer used.

[jgc, 2013-01-22 16:13:43]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2013-01-22 20:43:14]

https://codereview.appspot.com/7182045/diff/2001/src/pkg/net/http/response_te...
File src/pkg/net/http/response_test.go (right):

https://codereview.appspot.com/7182045/diff/2001/src/pkg/net/http/response_te...
src/pkg/net/http/response_test.go:182: "HTTP/1.0 200 OK\r\n" +
Little weird to see "HTTP/1.0" (which doesn't support chunking) with a
"Transfer-Encoding: chunked" header.  That's kinda irrelevant here, but it's
distracting and makes it look like that's what's being tested.

If not, let's change it to HTTP/1.1 here.  I don't think we care about testing
the behavior of 1.0 + chunking.

https://codereview.appspot.com/7182045/diff/2001/src/pkg/net/http/response_te...
src/pkg/net/http/response_test.go:195: Close:            true,
what makes this Close: true?  Oh, because it's HTTP/1.0?  Then let's have
another test for HTTP/1.1 to see that Close can ever be false in response to
HEAD.

[jgc, 2013-01-23 09:25:35]

https://codereview.appspot.com/7182045/diff/2001/src/pkg/net/http/response_te...
File src/pkg/net/http/response_test.go (right):

https://codereview.appspot.com/7182045/diff/2001/src/pkg/net/http/response_te...
src/pkg/net/http/response_test.go:182: "HTTP/1.0 200 OK\r\n" +
Agreed; will change to HTTP/1.1. Also there are other tests which are using
HTTP/1.0 and chunking: I will change those also.

https://codereview.appspot.com/7182045/diff/2001/src/pkg/net/http/response_te...
src/pkg/net/http/response_test.go:195: Close:            true,
On 2013/01/22 20:43:14, bradfitz wrote:
> what makes this Close: true?  Oh, because it's HTTP/1.0?  Then let's have
> another test for HTTP/1.1 to see that Close can ever be false in response to
> HEAD.

Added.

[jgc, 2013-01-23 09:35:03]

Hello golang-dev@googlegroups.com, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2013-01-24 01:01:06]

https://codereview.appspot.com/7182045/diff/8001/src/pkg/net/http/transfer.go
File src/pkg/net/http/transfer.go (left):

https://codereview.appspot.com/7182045/diff/8001/src/pkg/net/http/transfer.go...
src/pkg/net/http/transfer.go:125: if t.ResponseToHEAD {
this CL doesn't contain a test for this change.

[jgc, 2013-01-25 17:00:50]

https://codereview.appspot.com/7182045/diff/8001/src/pkg/net/http/transfer.go
File src/pkg/net/http/transfer.go (left):

https://codereview.appspot.com/7182045/diff/8001/src/pkg/net/http/transfer.go...
src/pkg/net/http/transfer.go:125: if t.ResponseToHEAD {
On 2013/01/24 01:01:06, bradfitz wrote:
> this CL doesn't contain a test for this change.

That's implicit in the other tests for HEAD handling. Since HEAD should be
handled like a GET so there's no need for a special case in the
shouldSendContentLength function. The other ifs within that function handle this
case (i.e. it's chunked---first if; there's a content length--second if).

[bradfitz, 2013-01-25 18:20:04]

LGTM

On Fri, Jan 25, 2013 at 9:00 AM, <jgrahamc@gmail.com> wrote:

>
> https://codereview.appspot.**com/7182045/diff/8001/src/pkg/**
>
net/http/transfer.go<https://codereview.appspot.com/7182045/diff/8001/src/pkg/net/http/transfer.go>
> File src/pkg/net/http/transfer.go (left):
>
> https://codereview.appspot.**com/7182045/diff/8001/src/pkg/**
>
net/http/transfer.go#**oldcode125<https://codereview.appspot.com/7182045/diff/8001/src/pkg/net/http/transfer.go#oldcode125>
> src/pkg/net/http/transfer.go:**125: if t.ResponseToHEAD {
> On 2013/01/24 01:01:06, bradfitz wrote:
>
>> this CL doesn't contain a test for this change.
>>
>
> That's implicit in the other tests for HEAD handling. Since HEAD should
> be handled like a GET so there's no need for a special case in the
> shouldSendContentLength function. The other ifs within that function
> handle this case (i.e. it's chunked---first if; there's a content
> length--second if).
>
>
https://codereview.appspot.**com/7182045/<https://codereview.appspot.com/7182...
>

[bradfitz, 2013-01-25 18:20:23]

*** Submitted as https://code.google.com/p/go/source/detail?r=8184de6499b0 ***

net/http: fix Content-Length/Transfer-Encoding on HEAD requests

net/http currently assumes that the response to a HEAD request
    will always have a Content-Length header. This is incorrect.

RFC2616 says: "The HEAD method is identical to GET except that
the server MUST NOT return a message-body in the response. 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. This method can be used for
obtaining metainformation about the entity implied by the
request without transferring the entity-body itself. This
method is often used for testing hypertext links for validity,
accessibility, and recent modification."

This means that three cases are possible: a Content-Length
header, a Transfer-Encoding header or neither. In the wild the
following sites exhibit these behaviours (curl -I):

HEAD on http://www.google.co.uk/ has Transfer-Encoding: chunked
HEAD on http://www.bbc.co.uk/    has Content-Length: 45247
HEAD on http://edition.cnn.com/  has neither header

This patch does not remove the ErrMissingContentLength error
for compatibility reasons, but it is no longer used.

R=golang-dev, bradfitz
CC=golang-dev
https://codereview.appspot.com/7182045

Committer: Brad Fitzpatrick <bradfitz@golang.org>

[remyoudompheng, 2013-07-20 20:01:31]

R=close