Don't send Content-Type when redirecting from a POST....

Don't send Content-Type when redirecting from a POST.

http://code.google.com/p/chromium/issues/detail?id=843


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=5532

[darin (slow to review), 2008-11-13 05:58:51]

Do you think WebKit would take this regression test?  Maybe we should still have
a unit test?

http://codereview.chromium.org/10873/diff/1/5
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/10873/diff/1/5#newcode304
Line 304: "content-size",
did you mean content-length?

[eroman, 2008-11-13 18:24:16]

> Maybe we should still have a unit test?

I will ping back once this is rewritten as a unit test.

I initially thought it wasn't going to be possible with testserver.py, however
after scanning thru the code I think a combination of "*.mock-http-headers" and
"/echoall/*" will serve my needs.

http://codereview.chromium.org/10873/diff/1/5
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/10873/diff/1/5#newcode304
Line 304: "content-size",
On 2008/11/13 05:58:51, darin wrote:
> did you mean content-length?

D'oh! A second mistake was not having tests for each header type -- will fix
that too.

[wtc, 2008-11-13 19:11:11]

LGTM.

http://codereview.chromium.org/10873/diff/8/12
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/10873/diff/8/12#newcode302
Line 302: static const char * const kPostHeaders[] = {
Nit: remove the space before '*'.

Just curious: how did you determine the list of POST-specific
headers?

Should we also strip the other Content-xxx headers, such as
Content-Language and Content-Encoding?

http://codereview.chromium.org/10873/diff/8/12#newcode304
Line 304: "content-length",
Should we allow a "Content-Length: 0" header for GET requests?

http://codereview.chromium.org/10873/diff/8/12#newcode325
Line 325: }
Do you need to append the final "\r\n" that marks the end
of headers?

http://codereview.chromium.org/10873/diff/8/12#newcode355
Line 355: // If being downgraded from POST to GET, must remove Content-Type
header.
Not sure if changing from POST to GET is a downgrade...

Perhaps "POST-specific headers" instead of "Content-Type header" specifically?

http://codereview.chromium.org/10873/diff/8/11
File net/url_request/url_request.h (right):

http://codereview.chromium.org/10873/diff/8/11#newcode440
Line 440: // Discard headers which have meaning in POST (Content-Length,
Content-Type,
Nit: add "only" before "in POST".

http://codereview.chromium.org/10873/diff/8/11#newcode442
Line 442: static std::string StripPostSpecificHeaders(const std::string&
headers);
Would be nice to document the assumptions of this method:
- headers are separated by "\r\n".
- Is there a final "\r\n" to mark the end of headers?
- Header continuation is not used.  Since we generate these
  headers, we can ensure that.

[eroman, 2008-11-13 21:27:01]

I got rid of the layout-test, and made it into a unit-test (small accompanying
modifications to testserver.py were needed)

Can you take a look?

Thanks for the review!

http://codereview.chromium.org/10873/diff/8/12
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/10873/diff/8/12#newcode302
Line 302: static const char * const kPostHeaders[] = {
On 2008/11/13 19:11:12, wtc wrote:
> Nit: remove the space before '*'.

Done

> Just curious: how did you determine the list of POST-specific headers?

I got that list by inspecting the webkit code (FrameLoader::submitForm).

This is of course a bit of a crapshoot, since if WebKit were to add a new
post-specific header not in our forbidden list, we would miss it.
In fact, the "Origin" header just showed up with recent merge, so we would have
missed it using this technique.
(see FrameLoader::addHTTPOriginIfNeeded).

As for the Content-Length header, I strip that since it shouldn't be in extra
headers anyway (the upload logic constructs its own Content-Length header
anyway).

Having to synchronize the logic sux (and there is a TODO on ways to fix). The
good news is it is unlikely for this to change much in the future, and also
unlikely that such a change uncaught would result in a server error.
(Content-Type is pretty special, so not shocking it caused problems).

> 
> Should we also strip the other Content-xxx headers, such as
> Content-Language and Content-Encoding?

I believe those are only sent in responses, and not requests. Is that not the
case?

http://codereview.chromium.org/10873/diff/8/12#newcode304
Line 304: "content-length",
On 2008/11/13 19:11:12, wtc wrote:
> Should we allow a "Content-Length: 0" header for GET requests?

The Content-Length header is assembled elsewhere, so we don't expect to see it
here. (this is really just a catchall).

http://codereview.chromium.org/10873/diff/8/12#newcode325
Line 325: }
On 2008/11/13 19:11:12, wtc wrote:
> Do you need to append the final "\r\n" that marks the end
> of headers?

Nope. Expectation is that |extra_request_headers_| is either empty, or ends with
"\r\n". See URLRequest::SetExtraRequestHeaders()

http://codereview.chromium.org/10873/diff/8/12#newcode355
Line 355: // If being downgraded from POST to GET, must remove Content-Type
header.
On 2008/11/13 19:11:12, wtc wrote:
> Not sure if changing from POST to GET is a downgrade...

Used the word "switched" instead, and overall changed the comment:

// If being switched from POST to GET, must remove headers that were
// specific to the POST and don't have meaning in GET. For example
// the inclusion of a multipart Content-Type header in GET can cause
// problems with some servers:
// http://code.google.com/p/chromium/issues/detail?id=843

> 
> Perhaps "POST-specific headers" instead of "Content-Type header" specifically?

[wtc, 2008-11-15 00:00:05]

LGTM.

http://codereview.chromium.org/10873/diff/23/29
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/10873/diff/23/29#newcode62
Line 62: bool ContainsString(const std::string haystack, const char* needle) {
Nit: 'haystack' should be a reference: const std::string&.

http://codereview.chromium.org/10873/diff/23/29#newcode797
Line 797: "Content-Length: 10\r\n"
Nit: Is 10 the actual content length or just some number you
made up?  It seems that just the multipart boundary string
is longer than 10.

[eroman, 2008-11-15 00:16:22]

http://codereview.chromium.org/10873/diff/23/29
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/10873/diff/23/29#newcode62
Line 62: bool ContainsString(const std::string haystack, const char* needle) {
On 2008/11/15 00:00:06, wtc wrote:
> Nit: 'haystack' should be a reference: const std::string&.

Fixed.

http://codereview.chromium.org/10873/diff/23/29#newcode797
Line 797: "Content-Length: 10\r\n"
On 2008/11/15 00:00:06, wtc wrote:
> Nit: Is 10 the actual content length or just some number you
> made up?  It seems that just the multipart boundary string
> is longer than 10.

"10" is just some junk value I added, to make sure it doesn't show up in the
request. There is logic in the upload code to replace Content-Length with
correct value; I want to make sure it doesn't slip through the cracks when set
as an "extra request header".