Sanitize headers in Proxy Authentication Required responses

net/http/http_network_transaction_unittest.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/http/http_network_transaction.h"
 
 #include <math.h>  // ceil
 #include <stdarg.h>
 #include <string>
 #include <vector>
@@ skipping 2532 matching lines @@
   ExpectLogContainsSomewhere(
       entries, pos,
       NetLog::TYPE_HTTP_TRANSACTION_READ_TUNNEL_RESPONSE_HEADERS,
       NetLog::PHASE_NONE);
 
   const HttpResponseInfo* response = trans->GetResponseInfo();
   ASSERT_TRUE(response != NULL);
   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()));
 
   TestCompletionCallback callback2;
 
   // Wrong password (should be "bar").
   rv = trans->RestartWithAuth(
       AuthCredentials(kFoo, kBaz), callback2.callback());
   EXPECT_EQ(ERR_IO_PENDING, rv);
 
   rv = callback2.WaitForResult();
   EXPECT_EQ(OK, rv);
 
   response = trans->GetResponseInfo();
   ASSERT_TRUE(response != NULL);
   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()));
 
   // Flush the idle socket before the NetLog and HttpNetworkTransaction go
   // out of scope.
   session->CloseAllConnections();
 }
 
 // Test that we don't read the response body when we fail to establish a tunnel,
 // even if the user cancels the proxy's auth attempt.
@@ skipping 36 matching lines @@
   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_EQ(10, response->headers->GetContentLength());
   EXPECT_TRUE(HttpVersion(1, 1) == response->headers->GetHttpVersion());
 
   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 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) {
   HttpRequestInfo request;
   request.method = "GET";
   request.url = GURL("http://www.google.com/");
   request.load_flags = 0;
 
   // We are using a DIRECT connection (i.e. no proxy) for this session.
   scoped_refptr<HttpNetworkSession> session(CreateSession(&session_deps_));
@@ skipping 10535 matching lines @@
   EXPECT_EQ(ERR_IO_PENDING, rv);
 
   rv = callback.WaitForResult();
   EXPECT_EQ(ERR_CONNECTION_RESET, rv);
 
   const HttpResponseInfo* response = trans->GetResponseInfo();
   EXPECT_TRUE(response == NULL);
 }
 
 }  // namespace net