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
Review URL: http://codereview.chromium.org/21158

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@9549 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 139 insertions, 45 deletions

diff --git a/net/base/net_error_list.h b/net/base/net_error_list.h
index 49ca641..c472f54 100644
--- a/net/base/net_error_list.h
+++ b/net/base/net_error_list.h
@@ -227,6 +227,10 @@ NET_ERROR(RESPONSE_HEADERS_TOO_BIG, -325)
 // The PAC requested by HTTP did not have a valid status code (non-200).
 NET_ERROR(PAC_STATUS_NOT_OK, -326)
 
+// The response was 401 (Unauthorized), yet the request was a CONNECT request
+// to a proxy.
+NET_ERROR(UNEXPECTED_SERVER_AUTH, -327)
+
 // The cache does not have the requested entry.
 NET_ERROR(CACHE_MISS, -400)
 
diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index 25b21d6..9667cb0 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -827,6 +827,27 @@ int HttpNetworkTransaction::DidReadResponseHeaders() {
       return ERR_METHOD_NOT_SUPPORTED;
   }
 
+  if (establishing_tunnel_) {
+    if (headers->response_code() == 200) {
+      if (header_buf_body_offset_ != header_buf_len_) {
+        // The proxy sent extraneous data after the headers.
+        return ERR_TUNNEL_CONNECTION_FAILED;
+      }
+      next_state_ = STATE_SSL_CONNECT_OVER_TUNNEL;
+      // Reset for the real request and response headers.
+      request_headers_.clear();
+      request_headers_bytes_sent_ = 0;
+      header_buf_len_ = 0;
+      header_buf_body_offset_ = 0;
+      establishing_tunnel_ = false;
+      return OK;
+    }
+    // Sanitize any illegal response code for CONNECT to prevent us from
+    // handling it by mistake.  See http://crbug.com/7338.
+    if (headers->response_code() < 400 || headers->response_code() > 599)
+      headers->set_response_code(500);  // Masquerade as a 500.
+  }
+
   // Check for an intermediate 100 Continue response.  An origin server is
   // allowed to send this response even if we didn't ask for it, so we just
   // need to skip over it.
@@ -843,21 +864,6 @@ int HttpNetworkTransaction::DidReadResponseHeaders() {
     return OK;
   }
 
-  if (establishing_tunnel_ && headers->response_code() == 200) {
-    if (header_buf_body_offset_ != header_buf_len_) {
-      // The proxy sent extraneous data after the headers.
-      return ERR_TUNNEL_CONNECTION_FAILED;
-    }
-    next_state_ = STATE_SSL_CONNECT_OVER_TUNNEL;
-    // Reset for the real request and response headers.
-    request_headers_.clear();
-    request_headers_bytes_sent_ = 0;
-    header_buf_len_ = 0;
-    header_buf_body_offset_ = 0;
-    establishing_tunnel_ = false;
-    return OK;
-  }
-
   response_.headers = headers;
   response_.vary_data.Init(*request_, *response_.headers);
 
@@ -1216,6 +1222,9 @@ int HttpNetworkTransaction::HandleAuthChallenge() {
   if (target == HttpAuth::AUTH_PROXY && proxy_info_.is_direct())
     return ERR_UNEXPECTED_PROXY_AUTH;
 
+  if (target == HttpAuth::AUTH_SERVER && establishing_tunnel_)
+    return ERR_UNEXPECTED_SERVER_AUTH;
+
   // The auth we tried just failed, hence it can't be valid. Remove it from
   // the cache so it won't be used again.
   if (HaveAuth(target))
diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index 375d62d..1e9f6aad 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -207,8 +207,7 @@ net::ProxyService* CreateNullProxyService() {
 net::ProxyService* CreateFixedProxyService(const std::string& proxy) {
   net::ProxyInfo proxy_info;
   proxy_info.UseNamedProxy(proxy);
-  return new net::ProxyService(
-      new net::ProxyConfigServiceFixed(proxy_info), NULL);
+  return net::ProxyService::Create(&proxy_info);
 }
 
 
diff --git a/net/http/http_response_headers.h b/net/http/http_response_headers.h
index ddd23ac..4158d79 100644
--- a/net/http/http_response_headers.h
+++ b/net/http/http_response_headers.h
@@ -201,6 +201,10 @@ class HttpResponseHeaders :
   // response code is not found in the raw headers.
   int response_code() const { return response_code_; }
 
+  // Sets the HTTP response code to the new code.  The original HTTP response
+  // code is still available in the raw and parsed headers.
+  void set_response_code(int new_code) { response_code_ = new_code; }
+
   // Returns the raw header string.
   const std::string& raw_headers() const { return raw_headers_; }
 
diff --git a/net/tools/testserver/testserver.py b/net/tools/testserver/testserver.py
index 86b4d63..569705d 100644
--- a/net/tools/testserver/testserver.py
+++ b/net/tools/testserver/testserver.py
@@ -75,6 +75,9 @@ class HTTPSServer(tlslite.api.TLSSocketServerMixIn, StoppableHTTPServer):
 class TestPageHandler(BaseHTTPServer.BaseHTTPRequestHandler):
 
   def __init__(self, request, client_address, socket_server):
+    self._connect_handlers = [
+      self.RedirectConnectHandler,
+      self.DefaultConnectResponseHandler]
     self._get_handlers = [
       self.KillHandler,
       self.NoCacheMaxAgeTimeHandler,
@@ -854,7 +857,7 @@ class TestPageHandler(BaseHTTPServer.BaseHTTPRequestHandler):
     self.wfile.write('<html><head>')
     self.wfile.write('</head><body>Redirecting to %s</body></html>' % dest)
 
-    return True;
+    return True
 
   def ClientRedirectHandler(self):
     """Sends a client redirect to the given URL. The syntax is
@@ -893,6 +896,41 @@ class TestPageHandler(BaseHTTPServer.BaseHTTPRequestHandler):
     self.wfile.write(contents)
     return True
 
+  def RedirectConnectHandler(self):
+    """Sends a redirect to the CONNECT request for www.redirect.com. This
+    response is not specified by the RFC, so the browser should not follow
+    the redirect."""
+
+    if (self.path.find("www.redirect.com") < 0):
+      return False
+
+    dest = "http://www.destination.com/foo.js"
+
+    self.send_response(302)  # moved temporarily
+    self.send_header('Location', dest)
+    self.send_header('Connection', 'close')
+    self.end_headers()
+    return True
+
+
+  def DefaultConnectResponseHandler(self):
+    """This is the catch-all response handler for CONNECT requests that aren't
+    handled by one of the special handlers above.  Real Web servers respond
+    with 400 to CONNECT requests."""
+
+    contents = "Your client has issued a malformed or illegal request."
+    self.send_response(400)  # bad request
+    self.send_header('Content-type', 'text/html')
+    self.send_header("Content-Length", len(contents))
+    self.end_headers()
+    self.wfile.write(contents)
+    return True
+
+  def do_CONNECT(self):
+    for handler in self._connect_handlers:
+      if handler():
+        return
+
   def do_GET(self):
     for handler in self._get_handlers:
       if handler():
@@ -1015,4 +1053,4 @@ if __name__ == '__main__':
   options, args = option_parser.parse_args()
 
   sys.exit(main(options, args))
-  
\ No newline at end of file
+  
diff --git a/net/url_request/url_request_http_job.cc b/net/url_request/url_request_http_job.cc
index 6a3535b..d29b31e 100644
--- a/net/url_request/url_request_http_job.cc
+++ b/net/url_request/url_request_http_job.cc
@@ -209,6 +209,12 @@ bool URLRequestHttpJob::IsRedirectResponse(GURL* location,
   if (!response_info_->headers->IsRedirect(&value))
     return false;
 
+  // For HTTPS, if we didn't receive a server certificate, the response was
+  // from the proxy server (a response to the CONNECT request) rather than
+  // the server.
+  if (request_->url().SchemeIsSecure() && !response_info_->ssl_info.cert)
+    return false;
+
   *location = request_->url().Resolve(value);
   *http_status_code = response_info_->headers->response_code();
   return true;
diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc
index 5c9146f..0cadd0e 100644
--- a/net/url_request/url_request_unittest.cc
+++ b/net/url_request/url_request_unittest.cc
@@ -55,10 +55,10 @@ class URLRequestHttpCacheContext : public URLRequestContext {
 
 class TestURLRequest : public URLRequest {
  public:
-   TestURLRequest(const GURL& url, Delegate* delegate)
-       : URLRequest(url, delegate) {
-     set_context(new URLRequestHttpCacheContext());
-   }
+  TestURLRequest(const GURL& url, Delegate* delegate)
+      : URLRequest(url, delegate) {
+    set_context(new URLRequestHttpCacheContext());
+  }
 };
 
 StringPiece TestNetResourceProvider(int key) {
@@ -82,6 +82,34 @@ bool ContainsString(const std::string& haystack, const char* needle) {
 class URLRequestTest : public PlatformTest {
 };
 
+TEST_F(URLRequestTest, ProxyTunnelRedirectTest) {
+  // In this unit test, we're using the HTTPTestServer as a proxy server and
+  // issue a CONNECT request with the magic host name "www.redirect.com" to
+  // it.  The HTTPTestServer will return a 302 response, which we should not
+  // follow.
+  scoped_refptr<HTTPTestServer> server =
+      HTTPTestServer::CreateServer(L"", NULL);
+  ASSERT_TRUE(NULL != server.get());
+  TestDelegate d;
+  {
+    URLRequest r(GURL("https://www.redirect.com/"), &d);
+    std::string proxy("localhost:");
+    proxy.append(IntToString(kHTTPDefaultPort));
+    r.set_context(new TestURLRequestContext(proxy));
+
+    r.Start();
+    EXPECT_TRUE(r.is_pending());
+
+    MessageLoop::current()->Run();
+
+    EXPECT_EQ(1, d.response_started_count());
+    // We should have rewritten the 302 response code as 500.
+    EXPECT_EQ(500, r.GetResponseCode());
+    // We should not have followed the redirect.
+    EXPECT_EQ(0, d.received_redirect_count());
+  }
+}
+
 TEST_F(URLRequestTest, GetTest_NoCache) {
   scoped_refptr<HTTPTestServer> server =
       HTTPTestServer::CreateServer(L"", NULL);
@@ -100,7 +128,7 @@ TEST_F(URLRequestTest, GetTest_NoCache) {
     EXPECT_NE(0, d.bytes_received());
   }
 #ifndef NDEBUG
-  DCHECK_EQ(url_request_metrics.object_count,0);
+  DCHECK_EQ(url_request_metrics.object_count, 0);
 #endif
 }
 
@@ -122,15 +150,15 @@ TEST_F(URLRequestTest, GetTest) {
     EXPECT_NE(0, d.bytes_received());
   }
 #ifndef NDEBUG
-  DCHECK_EQ(url_request_metrics.object_count,0);
+  DCHECK_EQ(url_request_metrics.object_count, 0);
 #endif
 }
 
 class HTTPSRequestTest : public testing::Test {
  protected:
-   HTTPSRequestTest() : util_() {};
+  HTTPSRequestTest() : util_() {}
 
-   SSLTestUtil util_;
+  SSLTestUtil util_;
 };
 
 #if defined(OS_MACOSX)
@@ -165,7 +193,7 @@ TEST_F(HTTPSRequestTest, MAYBE_HTTPSGetTest) {
     EXPECT_NE(0, d.bytes_received());
   }
 #ifndef NDEBUG
-  DCHECK_EQ(url_request_metrics.object_count,0);
+  DCHECK_EQ(url_request_metrics.object_count, 0);
 #endif
 }
 
@@ -188,7 +216,7 @@ TEST_F(URLRequestTest, CancelTest) {
     EXPECT_FALSE(d.received_data_before_response());
   }
 #ifndef NDEBUG
-  DCHECK_EQ(url_request_metrics.object_count,0);
+  DCHECK_EQ(url_request_metrics.object_count, 0);
 #endif
 }
 
@@ -217,7 +245,7 @@ TEST_F(URLRequestTest, CancelTest2) {
     EXPECT_EQ(URLRequestStatus::CANCELED, r.status().status());
   }
 #ifndef NDEBUG
-  DCHECK_EQ(url_request_metrics.object_count,0);
+  DCHECK_EQ(url_request_metrics.object_count, 0);
 #endif
 }
 
@@ -245,7 +273,7 @@ TEST_F(URLRequestTest, CancelTest3) {
     EXPECT_EQ(URLRequestStatus::CANCELED, r.status().status());
   }
 #ifndef NDEBUG
-  DCHECK_EQ(url_request_metrics.object_count,0);
+  DCHECK_EQ(url_request_metrics.object_count, 0);
 #endif
 }
 
@@ -320,7 +348,7 @@ TEST_F(URLRequestTest, PostTest) {
   char *uploadBytes = new char[kMsgSize+1];
   char *ptr = uploadBytes;
   char marker = 'a';
-  for(int idx=0; idx<kMsgSize/10; idx++) {
+  for (int idx = 0; idx < kMsgSize/10; idx++) {
     memcpy(ptr, "----------", 10);
     ptr += 10;
     if (idx % 100 == 0) {
@@ -329,7 +357,6 @@ TEST_F(URLRequestTest, PostTest) {
       if (++marker > 'z')
         marker = 'a';
     }
-
   }
   uploadBytes[kMsgSize] = '\0';
 
@@ -354,12 +381,12 @@ TEST_F(URLRequestTest, PostTest) {
 
     EXPECT_FALSE(d.received_data_before_response());
     EXPECT_EQ(uploadBytes, d.data_received());
-    EXPECT_EQ(memcmp(uploadBytes, d.data_received().c_str(), kMsgSize),0);
+    EXPECT_EQ(memcmp(uploadBytes, d.data_received().c_str(), kMsgSize), 0);
     EXPECT_EQ(d.data_received().compare(uploadBytes), 0);
   }
   delete[] uploadBytes;
 #ifndef NDEBUG
-  DCHECK_EQ(url_request_metrics.object_count,0);
+  DCHECK_EQ(url_request_metrics.object_count, 0);
 #endif
 }
 
@@ -384,7 +411,7 @@ TEST_F(URLRequestTest, PostEmptyTest) {
     EXPECT_TRUE(d.data_received().empty());
   }
 #ifndef NDEBUG
-  DCHECK_EQ(url_request_metrics.object_count,0);
+  DCHECK_EQ(url_request_metrics.object_count, 0);
 #endif
 }
 
@@ -435,7 +462,7 @@ TEST_F(URLRequestTest, PostFileTest) {
     EXPECT_EQ(0, memcmp(d.data_received().c_str(), buf.get(), size));
   }
 #ifndef NDEBUG
-  DCHECK_EQ(url_request_metrics.object_count,0);
+  DCHECK_EQ(url_request_metrics.object_count, 0);
 #endif
 }
 
@@ -454,7 +481,7 @@ TEST_F(URLRequestTest, AboutBlankTest) {
     EXPECT_EQ(d.bytes_received(), 0);
   }
 #ifndef NDEBUG
-  DCHECK_EQ(url_request_metrics.object_count,0);
+  DCHECK_EQ(url_request_metrics.object_count, 0);
 #endif
 }
 
@@ -481,7 +508,7 @@ TEST_F(URLRequestTest, FileTest) {
     EXPECT_EQ(d.bytes_received(), static_cast<int>(file_size));
   }
 #ifndef NDEBUG
-  DCHECK_EQ(url_request_metrics.object_count,0);
+  DCHECK_EQ(url_request_metrics.object_count, 0);
 #endif
 }
 
@@ -497,7 +524,7 @@ TEST_F(URLRequestTest, InvalidUrlTest) {
     EXPECT_TRUE(d.request_failed());
   }
 #ifndef NDEBUG
-  DCHECK_EQ(url_request_metrics.object_count,0);
+  DCHECK_EQ(url_request_metrics.object_count, 0);
 #endif
 }
 
@@ -515,7 +542,7 @@ TEST_F(URLRequestTest, DISABLED_DnsFailureTest) {
     EXPECT_TRUE(d.request_failed());
   }
 #ifndef NDEBUG
-  DCHECK_EQ(url_request_metrics.object_count,0);
+  DCHECK_EQ(url_request_metrics.object_count, 0);
 #endif
 }
 
@@ -666,7 +693,7 @@ TEST_F(URLRequestTest, ResolveShortcutTest) {
   CoUninitialize();
 
 #ifndef NDEBUG
-  DCHECK_EQ(url_request_metrics.object_count,0);
+  DCHECK_EQ(url_request_metrics.object_count, 0);
 #endif
 }
 #endif  // defined(OS_WIN)
@@ -713,7 +740,7 @@ TEST_F(URLRequestTest, FileDirCancelTest) {
     MessageLoop::current()->Run();
   }
 #ifndef NDEBUG
-  DCHECK_EQ(url_request_metrics.object_count,0);
+  DCHECK_EQ(url_request_metrics.object_count, 0);
 #endif
 
   // Take out mock resource provider.
diff --git a/net/url_request/url_request_unittest.h b/net/url_request/url_request_unittest.h
index de75614..7107d50 100644
--- a/net/url_request/url_request_unittest.h
+++ b/net/url_request/url_request_unittest.h
@@ -45,6 +45,14 @@ class TestURLRequestContext : public URLRequestContext {
         net::HttpNetworkLayer::CreateFactory(proxy_service_);
   }
 
+  explicit TestURLRequestContext(const std::string& proxy) {
+    net::ProxyInfo proxy_info;
+    proxy_info.UseNamedProxy(proxy);
+    proxy_service_ = net::ProxyService::Create(&proxy_info);
+    http_transaction_factory_ =
+        net::HttpNetworkLayer::CreateFactory(proxy_service_);
+  }
+
   virtual ~TestURLRequestContext() {
     delete http_transaction_factory_;
     delete proxy_service_;
@@ -355,7 +363,6 @@ class BaseTestServer : public base::RefCounted<BaseTestServer> {
 #endif
     if (!normalized_document_root.empty())
       file_util::AppendToPath(test_data_directory, normalized_document_root);
-
     data_directory_ = *test_data_directory;
   }
 
@@ -482,7 +489,7 @@ class HTTPTestServer : public BaseTestServer {
     {
       MessageLoop* loop = loop_;
       scoped_ptr<base::Thread> io_thread;
-      
+
       if (!loop) {
         io_thread.reset(new base::Thread("MakeGETRequest"));
         base::Thread::Options options;