net: don't process truncated headers.

net: don't process truncated headers on HTTPS connections.

This change causes us to not process any headers unless they are correctly
terminated with a \r\n\r\n sequence.

BUG=244260
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202927

[Ryan Hamilton, 2013-05-28 21:38:18]

lgtm

[agl, 2013-05-28 23:16:36]

PTAL. This change has been updated to only affect HTTPS.

[Ryan Hamilton, 2013-05-29 02:39:30]

lgtm

[commit-bot: I haz the power, 2013-05-29 14:50:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/15688012/20001

[commit-bot: I haz the power, 2013-05-29 14:50:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/15688012/20001

[commit-bot: I haz the power, 2013-05-29 15:52:21]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-05-29 16:08:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/15688012/20001

[commit-bot: I haz the power, 2013-05-29 19:06:54]

Change committed as 202927

[Ryan Sleevi, 2013-05-29 19:59:32]

https://chromiumcodereview.appspot.com/15688012/diff/20001/net/http/http_stre...
File net/http/http_stream_parser.cc (right):

https://chromiumcodereview.appspot.com/15688012/diff/20001/net/http/http_stre...
net/http/http_stream_parser.cc:549: } else if (request_->url.SchemeIs("https"))
{
Seems like we should be looking at SchemeIsSecure - that is, also counting in
wss:// (can't they set cookies during their initial Upgrade negotiation?)

I'm curious if we attempt a wss:// request and the server doesn't support it, if
it's possible to fall back to an HTTP response (with a Set-Cookie), thus
allowing this to work again.

[wtc, 2013-05-30 00:38:30]

Drive-by review comments: I didn't review the unit tests.

https://codereview.chromium.org/15688012/diff/20001/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/15688012/diff/20001/net/base/net_error_list.h...
net/base/net_error_list.h:540: NET_ERROR(HEADERS_TRUNCATED, -357)

I suggest renaming this RESPONSE_HEADERS_TRUNCATED to match the
other RESPONSE_HEADERS_xxx errors.

https://codereview.chromium.org/15688012/diff/20001/net/http/http_stream_pars...
File net/http/http_stream_parser.cc (right):

https://codereview.chromium.org/15688012/diff/20001/net/http/http_stream_pars...
net/http/http_stream_parser.cc:549: } else if (request_->url.SchemeIs("https"))
{

On 2013/05/29 19:59:33, Ryan Sleevi wrote:
> Seems like we should be looking at SchemeIsSecure - that is, also counting in
> wss:// ...

Yes, this sounds like a good idea. In fact, I am wondering if we
should also do this for HTTP.

Nit: omit "else" after a return statement.

https://codereview.chromium.org/15688012/diff/20001/net/http/http_stream_pars...
net/http/http_stream_parser.cc:551: // don't parse partial headers. Return a
different error code so that we

By "a different error code", do you mean an error code different
from ERR_EMPTY_RESPONSE?

[agl, 2013-06-03 18:18:28]

Will do follow up CL if someone wants to say that all SchemeIs("https") in the
file should be SchemeIsSecure.

https://chromiumcodereview.appspot.com/15688012/diff/20001/net/base/net_error...
File net/base/net_error_list.h (right):

https://chromiumcodereview.appspot.com/15688012/diff/20001/net/base/net_error...
net/base/net_error_list.h:540: NET_ERROR(HEADERS_TRUNCATED, -357)
On 2013/05/30 00:38:30, wtc wrote:
> 
> I suggest renaming this RESPONSE_HEADERS_TRUNCATED to match the
> other RESPONSE_HEADERS_xxx errors.

Will do.

https://chromiumcodereview.appspot.com/15688012/diff/20001/net/http/http_stre...
File net/http/http_stream_parser.cc (right):

https://chromiumcodereview.appspot.com/15688012/diff/20001/net/http/http_stre...
net/http/http_stream_parser.cc:549: } else if (request_->url.SchemeIs("https"))
{
On 2013/05/30 00:38:30, wtc wrote:
> Yes, this sounds like a good idea. In fact, I am wondering if we
> should also do this for HTTP.

After discussions with Darin, I only did this for HTTPS in order to limit compat
concerns.

r.e. SchemeIsSecure - should the other uses of SchemeIs("https") in this file be
changed too? (I'm guessing yes, but wanted to check.)

https://chromiumcodereview.appspot.com/15688012/diff/20001/net/http/http_stre...
net/http/http_stream_parser.cc:551: // don't parse partial headers. Return a
different error code so that we
On 2013/05/30 00:38:30, wtc wrote:
> 
> By "a different error code", do you mean an error code different
> from ERR_EMPTY_RESPONSE?

Yes, will clarify.

[wtc, 2013-06-03 18:36:47]

https://chromiumcodereview.appspot.com/15688012/diff/20001/net/http/http_stre...
File net/http/http_stream_parser.cc (right):

https://chromiumcodereview.appspot.com/15688012/diff/20001/net/http/http_stre...
net/http/http_stream_parser.cc:549: } else if (request_->url.SchemeIs("https"))
{

On 2013/06/03 18:18:29, agl wrote:
>
> r.e. SchemeIsSecure - should the other uses of SchemeIs("https") in this file
be
> changed too? (I'm guessing yes, but wanted to check.)

It occurred to me that this file, http_stream_parser.cc, is probably only used
for the "http" and "https" schemes. If so, then SchemeIs("https") vs.
SchemeIsSecure()
is just a cosmetic issue.

SchemeIsSecure is defined as:

  // If the scheme indicates a secure connection
  bool SchemeIsSecure() const {
    return SchemeIs("https") || SchemeIs("wss") ||
        (SchemeIsFileSystem() && inner_url() && inner_url()->SchemeIsSecure());
  }

So there is also a SchemeIsFileSystem() test, which I don't fully understand.

Since this file isn't using SchemeIsSecure, I now think it is fine to just
use SchemeIs("https") here.