Sanitize headers in Proxy Authentication Required responses

net/http/http_network_transaction_unittest.cc

Index: net/http/http_network_transaction_unittest.cc
diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index 19dfeb7e7063f848e0ccb206afdbc796663dbc93..619fa96116b5c8f5fc1f03e18c1cdb968e45eea4 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -2550,7 +2550,6 @@ TEST_P(HttpNetworkTransactionTest, BasicAuthProxyKeepAlive) {
   ASSERT_FALSE(response->headers.get() == NULL);
   EXPECT_TRUE(response->headers->IsKeepAlive());
   EXPECT_EQ(407, response->headers->response_code());
-  EXPECT_EQ(10, response->headers->GetContentLength());

[Ryan Sleevi, 2014/12/19 22:06:25]

Rather than deleting these, you should be asserting there is no body, right?

[Deprecated (see juliatuttle), 2015/01/02 19:40:55]

Well, at least asserting that there is no Content-Length.

   EXPECT_TRUE(HttpVersion(1, 1) == response->headers->GetHttpVersion());
   EXPECT_TRUE(CheckBasicProxyAuth(response->auth_challenge.get()));
 
@@ -2569,7 +2568,6 @@ TEST_P(HttpNetworkTransactionTest, BasicAuthProxyKeepAlive) {
   ASSERT_FALSE(response->headers.get() == NULL);
   EXPECT_TRUE(response->headers->IsKeepAlive());
   EXPECT_EQ(407, response->headers->response_code());
-  EXPECT_EQ(10, response->headers->GetContentLength());
   EXPECT_TRUE(HttpVersion(1, 1) == response->headers->GetHttpVersion());
   EXPECT_TRUE(CheckBasicProxyAuth(response->auth_challenge.get()));
 
@@ -2626,7 +2624,6 @@ TEST_P(HttpNetworkTransactionTest, BasicAuthProxyCancelTunnel) {
 
   EXPECT_TRUE(response->headers->IsKeepAlive());
   EXPECT_EQ(407, response->headers->response_code());
-  EXPECT_EQ(10, response->headers->GetContentLength());
   EXPECT_TRUE(HttpVersion(1, 1) == response->headers->GetHttpVersion());
 
   std::string response_data;
@@ -2637,6 +2634,67 @@ TEST_P(HttpNetworkTransactionTest, BasicAuthProxyCancelTunnel) {
   session->CloseAllConnections();
 }
 
+// Test that we don't pass extraneous headers from the proxy's response to the
+// caller when the proxy responds to CONNECT with 407.
+TEST_P(HttpNetworkTransactionTest, SanitizeProxyAuthHeaders) {
+  HttpRequestInfo request;
+  request.method = "GET";
+  request.url = GURL("https://www.google.com/");
+  request.load_flags = 0;
+
+  // Configure against proxy server "myproxy:70".
+  session_deps_.proxy_service.reset(ProxyService::CreateFixed("myproxy:70"));
+
+  scoped_refptr<HttpNetworkSession> session(CreateSession(&session_deps_));
+
+  scoped_ptr<HttpTransaction> trans(
+      new HttpNetworkTransaction(DEFAULT_PRIORITY, session.get()));
+
+  // Since we have proxy, should try to establish tunnel.
+  MockWrite data_writes[] = {
+      MockWrite(
+          "CONNECT www.google.com:443 HTTP/1.1\r\n"
+          "Host: www.google.com\r\n"
+          "Proxy-Connection: keep-alive\r\n\r\n"),
+  };
+
+  // The proxy responds to the connect with a 407.
+  MockRead data_reads[] = {
+      MockRead("HTTP/1.1 407 Proxy Authentication Required\r\n"),
+      MockRead("X-Foo: bar\r\n"),

[Ryan Sleevi, 2014/12/19 22:06:25]

Can you explicitly add a test for Set-Cookie behaviour? I agree that it's useful
to test X-Foo, but we also want to explicitly test headers to ensure they're not
also "special-cased" by HttpStreamParser (such as Content-Length, which tripped
up Ryan and I)

[Deprecated (see juliatuttle), 2015/01/02 19:40:55]

Done. I don't see any way to test it besides just adding a Set-Cookie header and
checking for it in the response headers, so that's what I did.

+      MockRead("Proxy-Authenticate: Basic realm=\"MyRealm1\"\r\n"),
+      MockRead("Content-Length: 10\r\n\r\n"),
+      MockRead(SYNCHRONOUS, ERR_UNEXPECTED),  // Should not be reached.

[Ryan Sleevi, 2014/12/19 22:06:24]

I'd feel better for this test if you explicitly added content bytes.

That is, your "should not be reached" may indirectly cause line 2692 to be
reached for reasons unrelated to the 407 on 2663.

That is, by adding a body to be read, you can then assert that the response on
2683 *doesn't* include a body, to ensure that the body is stripped out.

Does that make sense?

+  };
+
+  StaticSocketDataProvider data(data_reads, arraysize(data_reads), data_writes,
+                                arraysize(data_writes));
+  session_deps_.socket_factory->AddSocketDataProvider(&data);
+
+  TestCompletionCallback callback;
+
+  int rv = trans->Start(&request, callback.callback(), BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+
+  rv = callback.WaitForResult();
+  EXPECT_EQ(OK, rv);
+
+  const HttpResponseInfo* response = trans->GetResponseInfo();
+  ASSERT_TRUE(response != NULL);
+
+  EXPECT_TRUE(response->headers->IsKeepAlive());
+  EXPECT_EQ(407, response->headers->response_code());
+  EXPECT_TRUE(HttpVersion(1, 1) == response->headers->GetHttpVersion());
+  EXPECT_FALSE(response->headers->HasHeaderValue("X-Foo", "bar"));
+
+  std::string response_data;
+  rv = ReadTransaction(trans.get(), &response_data);
+  EXPECT_EQ(ERR_TUNNEL_CONNECTION_FAILED, rv);
+
+  // Flush the idle socket before the HttpNetworkTransaction goes out of scope.
+  session->CloseAllConnections();
+}
+
 // Test when a server (non-proxy) returns a 407 (proxy-authenticate).
 // The request should fail with ERR_UNEXPECTED_PROXY_AUTH.
 TEST_P(HttpNetworkTransactionTest, UnexpectedProxyAuth) {