Sanitize headers in Proxy Authentication Required responses

net/http/http_proxy_client_socket.cc

Index: net/http/http_proxy_client_socket.cc
diff --git a/net/http/http_proxy_client_socket.cc b/net/http/http_proxy_client_socket.cc
index 515301cf08c1f1b5c9ebb5451c913d434161d993..a7fd7fc8295ae985321c52284311a672c75f4b71 100644
--- a/net/http/http_proxy_client_socket.cc
+++ b/net/http/http_proxy_client_socket.cc
@@ -483,25 +483,31 @@ int HttpProxyClientSocket::DoReadHeadersComplete(int result) {
       // sanitize the response.  This still allows a rogue HTTPS proxy to
       // redirect an HTTPS site load to a similar-looking site, but no longer
       // allows it to impersonate the site the user requested.
-      if (is_https_proxy_ && SanitizeProxyRedirect(&response_, request_.url)) {
+      if (!is_https_proxy_ || !SanitizeProxyRedirect(&response_)) {
+        // We're not using an HTTPS proxy, or we couldn't sanitize the redirect.

[Ryan Sleevi, 2014/12/08 22:04:39]

nit: "Pronouns in Comments considered harmful" -
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis...

That said, this comment describes the code, which violates
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Implementatio...

(That is, the if conditional says exactly what your plain english comment is
saying). So delete this comment.

[Deprecated (see juliatuttle), 2014/12/09 15:31:15]

Done.

+        LogBlockedTunnelResponse();
+        return ERR_TUNNEL_CONNECTION_FAILED;
+      }
+
+      {
         bool is_connection_reused = http_stream_parser_->IsConnectionReused();
         redirect_has_load_timing_info_ =
             transport_->GetLoadTimingInfo(
                 is_connection_reused, &redirect_load_timing_info_);
-        transport_.reset();
-        http_stream_parser_.reset();
-        return ERR_HTTPS_PROXY_TUNNEL_RESPONSE;
       }
-
-      // We're not using an HTTPS proxy, or we couldn't sanitize the redirect.
-      LogBlockedTunnelResponse();
-      return ERR_TUNNEL_CONNECTION_FAILED;
+      transport_.reset();
+      http_stream_parser_.reset();
+      return ERR_HTTPS_PROXY_TUNNEL_RESPONSE;

[Ryan Hamilton, 2014/12/08 22:51:59]

I think you've tried to make use of an early-return of the error here. Yay! I
love early-return. But I think you need to nuke the {}s at 492/497 and
refrormat?

[Deprecated (see juliatuttle), 2014/12/09 15:31:15]

What's wrong with the {}s? They're to prevent "jumping past assignment" errors
from the switch.

[Ryan Hamilton, 2014/12/10 22:09:21]

They look ugly :>
Ugh. I see. In that case, I would suggest one of two approaches:
1) simply inline the call to http_stream_parser_->IsConnectionReused()
2) put the {}s around the entire case 407: block, which I think is more common
than simply putting braces around two lines in the middle of a block.

[Ryan Hamilton, 2014/12/19 21:26:58]

ping.

[Deprecated (see juliatuttle), 2014/12/19 21:50:46]

Done.

 
     case 407:  // Proxy Authentication Required
       // We need this status code to allow proxy authentication.  Our
       // authentication code is smart enough to avoid being tricked by an
       // active network attacker.
       // The next state is intentionally not set as it should be STATE_NONE;
+      if (!SanitizeProxyAuth(&response_)) {
+        LogBlockedTunnelResponse();
+        return ERR_TUNNEL_CONNECTION_FAILED;
+      }
       return HandleProxyAuthChallenge(auth_.get(), &response_, net_log_);
 
     default: