Allow a non-200 (or non-407) response for a CONNECT request from an HTTPS pro...

net/http/http_proxy_client_socket.cc

Index: net/http/http_proxy_client_socket.cc
===================================================================
--- net/http/http_proxy_client_socket.cc	(revision 68026)
+++ net/http/http_proxy_client_socket.cc	(working copy)
@@ -12,6 +12,7 @@
 #include "net/base/io_buffer.h"
 #include "net/base/net_log.h"
 #include "net/base/net_util.h"
+#include "net/http/connect_response_http_stream.h"

[vandebo (ex-Chrome), 2010/12/04 00:30:37]

Is this header needed?

[Ryan Hamilton, 2010/12/09 21:19:35]

Done.

 #include "net/http/http_net_log_params.h"
 #include "net/http/http_network_session.h"
 #include "net/http/http_proxy_utils.h"
@@ -31,7 +32,8 @@
     HttpAuthCache* http_auth_cache,
     HttpAuthHandlerFactory* http_auth_handler_factory,
     bool tunnel,
-    bool using_spdy)
+    bool using_spdy,
+    bool is_https_proxy)
     : ALLOW_THIS_IN_INITIALIZER_LIST(
           io_callback_(this, &HttpProxyClientSocket::OnIOComplete)),
       next_state_(STATE_NONE),
@@ -46,6 +48,7 @@
           : NULL),
       tunnel_(tunnel),
       using_spdy_(using_spdy),
+      is_https_proxy_(is_https_proxy),
       net_log_(transport_socket->socket()->NetLog()) {
   // Synthesize the bits of a request that we actually use.
   request_.url = request_url;
@@ -145,7 +148,8 @@
 }
 
 void HttpProxyClientSocket::Disconnect() {
-  transport_->socket()->Disconnect();
+  if (transport_->socket())

[vandebo (ex-Chrome), 2010/12/04 00:30:37]

When is this false?

[Ryan Hamilton, 2010/12/09 21:19:35]

So I think this was somewhat of a bug which I describe in my response to your
"recoverable error" comment.  However, I had to change this code to check for
transport_ to be non-null, since it will be null after
GetConnectResponseStream() is called.  Nice catch.

+    transport_->socket()->Disconnect();
 
   // Reset other states to make sure they aren't mistakenly used later.
   // These are the states initialized by Connect().
@@ -405,14 +409,18 @@
       return HandleAuthChallenge();
 
     default:
-      // For all other status codes, we conservatively fail the CONNECT
-      // request.
-      // We lose something by doing this.  We have seen proxy 403, 404, and
-      // 501 response bodies that contain a useful error message.  For
-      // example, Squid uses a 404 response to report the DNS error: "The
-      // domain name does not exist."
       LogBlockedTunnelResponse(response_.headers->response_code());

[vandebo (ex-Chrome), 2010/12/04 00:30:37]

Shouldn't this Log call be in the else clause?

[Ryan Hamilton, 2010/12/09 21:19:35]

Done.

-      return ERR_TUNNEL_CONNECTION_FAILED;
+      if (is_https_proxy_) {
+        return ERR_HTTPS_PROXY_TUNNEL_CONNECTION_RESPONSE;
+      } else {
+        // For all other status codes, we conservatively fail the CONNECT
+        // request.
+        // We lose something by doing this.  We have seen proxy 403, 404, and
+        // 501 response bodies that contain a useful error message.  For
+        // example, Squid uses a 404 response to report the DNS error: "The
+        // domain name does not exist."
+        return ERR_TUNNEL_CONNECTION_FAILED;
+      }
   }
 }