Fix a logic error in the handling the response to an HTTP...

Sanitize proxy response codes to CONNECT requests.  For
anything other than 200 (success) or 400-599 (error), we
rewrite the response code as 500 (internal server error)
to prevent any special handling of the proxy's response to
CONNECT by mistake.

Add a new error code ERR_UNEXPECTED_SERVER_AUTH for a 401
response to a CONNECT request.

Fix nits reported by cpplint.py.

R=darin,eroman
BUG=7338
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=9549

[darin (slow to review), 2009-02-10 04:47:57]

LGTM

[eroman, 2009-02-10 06:49:56]

http://codereview.chromium.org/21158/diff/5/1007
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/21158/diff/5/1007#newcode913
Line 913: return True;
style-nit: remove the semicolon.

http://codereview.chromium.org/21158/diff/5/1005
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/21158/diff/5/1005#newcode215
Line 215: if (request_->url().SchemeIsSecure() &&
!response_info_->ssl_info.cert)
I preferred how in your initial version the enforcement was done at a lower
layer -- HttpNetworkTransaction. That felt more robust, since the failure gets
tagged right away, without giving room for other mistakes.

For example, what happens in the current patchset if the response was a 401
instead? I believe what will happen is
HttpNetworkTransaction::HandleAuthChallenge() will kick into AUTH_SERVER mode,
not realizing that a 401 doesn't actually make sense since the tunnel
establishment never actually succeeded.

While the above case isn't really dangerous, it still seems there is surface
area for errors, esp by other clients.

How about some variations on your original design?

// Variation 1: Fail at network layer if it is a dangerous response code.
// (Probably better as a whitelist instead of blacklist.)
if (establishing_tunnel_ && headers->response_code() != 200) {
  if (response_code == 302 || response_code == 307 ||
       response_code == 401) {
      return ERR_TUNNEL_CONNECTION_FAILED;
  }
}

// Variation 2: "Sanitize" dangerous error codes, so consumers
// aren't fooled into thinking the response code was valid.
// We rewrite the response headers in other places too (like
// in the header parsing of malformed responses), so there
// is some precedent.
if (establishing_tunnel_ && headers->response_code() != 200) {
  if (responseCode is not safe)
      headers->set_response_code(500); // Masquerade as a 500.
}

[wtc, 2009-02-10 22:14:49]

Darin, you only need to review the change to
http_network_transaction.cc if you're busy.

I implemented eroman's Variation 2.  I also used a
whitelist instead of a blacklist.  I only allow a proxy
response code of 200 (success) or 407 (proxy auth required) to CONNECT requests.

The advantage of Variation 2 over Variation 1 is that we
can display the response body to the user.  The response
body is often an HTML page containing the error message
from the proxy server.

The drawback of Variation 2 is that the rewritten response
code in HttpResponseHeaders is different from the original
response code in the raw and parsed headers.  We may get
confused by that if we're not careful.

We chose 500 as the rewritten response code.  As far as
I can tell, we don't handle a 500 response.  If you know
of a better fake response code, please suggest it.

[darin (slow to review), 2009-02-10 22:28:20]

This change makes sense to me.  LG for http_network_transaction.cc

[eroman, 2009-02-10 23:31:12]

LGTM after answering
http://codereview.chromium.org/21158/diff/14/1014#newcode215

http://codereview.chromium.org/21158/diff/14/1019
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/21158/diff/14/1019#newcode846
Line 846: // handling it by mistake.
Maybe include a link to the original bug report?

http://codereview.chromium.org/21158/diff/14/1014
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/21158/diff/14/1014#newcode215
Line 215: if (request_->url().SchemeIsSecure() &&
!response_info_->ssl_info.cert)
This shouldn't be necessary anymore, as the reponse code is 500.

[wtc, 2009-02-10 23:52:11]

I made the changes you suggested.  Please
review the new Patch Set.  Unfortunately
it picked up unrelated changes to
url_request_unittest.h.

I also had to change an expected response code
from 404 to 500 in a unit test in http_network_transaction_unittest.cc.

http://codereview.chromium.org/21158/diff/14/1014
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/21158/diff/14/1014#newcode215
Line 215: if (request_->url().SchemeIsSecure() &&
!response_info_->ssl_info.cert)
On 2009/02/10 23:31:13, eroman wrote:
> This shouldn't be necessary anymore, as the reponse code is 500.

I left it for double protection.  But I've removed it.

With the current fix, I could actually do the unit test
in http_network_transaction_unittest.cc, without modifying
testserver.py and url_request_unittest.{h,cc}.  But I'm
too tired to redo the unit test.

[eroman, 2009-02-11 00:00:57]

LGTM.

I wander if this is going to have a behavioral impact with regards to the link
doctor (i.e. if the 404 would have been substituted by link doctor before, and
now won't since it a 500).

[wtc, 2009-02-11 01:45:03]

OK, as we discussed, here is a new Patch Set.

I added a new ERR_UNEXPECTED_AUTH_SERVER error code.  I
didn't add a unit test for it, I know...

[eroman, 2009-02-11 02:01:42]

still lgtm.

http://codereview.chromium.org/21158/diff/30/1038
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/21158/diff/30/1038#newcode1226
Line 1226: return ERR_UNEXPECTED_PROXY_AUTH;
I suspect you meant to use your new error code here:
  ERR_UNEXPECTED_SERVER_AUTH