Changes the UI for HTTP/FTP auth challenges to include the server's port. So instead of "www.foo.com" it will say "www.foo.com:80". We need to include the port number since otherwise it can be ambiguous what the actual target server is.

This change also introduces utility function "GetHostAnd[Optional]Port()" to help with forming <host> [":" <port>] strings.

BUG=12073
Review URL: http://codereview.chromium.org/112041

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

10 files changed, 110 insertions, 65 deletions

diff --git a/net/base/auth.h b/net/base/auth.h
index b404f7a..ec31bfc 100644
--- a/net/base/auth.h
+++ b/net/base/auth.h
@@ -17,8 +17,8 @@ class AuthChallengeInfo :
     public base::RefCountedThreadSafe<AuthChallengeInfo> {
  public:
   bool is_proxy;  // true for Proxy-Authenticate, false for WWW-Authenticate.
-  std::wstring host;  // the domain name of the server asking for auth
-                      // (could be the proxy).
+  std::wstring host_and_port;  // <host>:<port> of the server asking for auth
+                               // (could be the proxy).
   std::wstring scheme;  // "Basic", "Digest", or whatever other method is used.
   std::wstring realm;  // the realm provided by the server, if there is one.
 
diff --git a/net/base/net_util.cc b/net/base/net_util.cc
index c309653..06b43be 100644
--- a/net/base/net_util.cc
+++ b/net/base/net_util.cc
@@ -944,10 +944,10 @@ int SetNonBlocking(int fd) {
 #endif
 }
 
-bool GetHostAndPort(std::string::const_iterator host_and_port_begin,
-                    std::string::const_iterator host_and_port_end,
-                    std::string* host,
-                    int* port) {
+bool ParseHostAndPort(std::string::const_iterator host_and_port_begin,
+                      std::string::const_iterator host_and_port_end,
+                      std::string* host,
+                      int* port) {
   if (host_and_port_begin >= host_and_port_end)
     return false;
 
@@ -991,10 +991,25 @@ bool GetHostAndPort(std::string::const_iterator host_and_port_begin,
   return true;  // Success.
 }
 
-bool GetHostAndPort(const std::string& host_and_port,
-                    std::string* host,
-                    int* port) {
-  return GetHostAndPort(host_and_port.begin(), host_and_port.end(), host, port);
+bool ParseHostAndPort(const std::string& host_and_port,
+                      std::string* host,
+                      int* port) {
+  return ParseHostAndPort(
+      host_and_port.begin(), host_and_port.end(), host, port);
+}
+
+std::string GetHostAndPort(const GURL& url) {
+  // For IPv6 literals, GURL::host() already includes the brackets so it is
+  // safe to just append a colon.
+  return StringPrintf("%s:%d", url.host().c_str(), url.EffectiveIntPort());
+}
+
+std::string GetHostAndOptionalPort(const GURL& url) {
+  // For IPv6 literals, GURL::host() already includes the brackets
+  // so it is safe to just append a colon.
+  if (url.has_port())
+    return StringPrintf("%s:%s", url.host().c_str(), url.port().c_str());
+  return url.host();
 }
 
 std::string NetAddressToString(const struct addrinfo* net_address) {
diff --git a/net/base/net_util.h b/net/base/net_util.h
index 9c59b90..e64cb88 100644
--- a/net/base/net_util.h
+++ b/net/base/net_util.h
@@ -35,19 +35,26 @@ GURL FilePathToFileURL(const FilePath& path);
 // valid file URL. On failure, *file_path will be empty.
 bool FileURLToFilePath(const GURL& url, FilePath* file_path);
 
-// Split an input of the form <host>[":"<port>] into its consitituent parts.
+// Splits an input of the form <host>[":"<port>] into its consitituent parts.
 // Saves the result into |*host| and |*port|. If the input did not have
 // the optional port, sets |*port| to -1.
 // Returns true if the parsing was successful, false otherwise.
 // The returned host is NOT canonicalized, and may be invalid. If <host> is
 // an IPv6 literal address, the returned host includes the square brackets.
-bool GetHostAndPort(std::string::const_iterator host_and_port_begin,
-                    std::string::const_iterator host_and_port_end,
-                    std::string* host,
-                    int* port);
-bool GetHostAndPort(const std::string& host_and_port,
-                    std::string* host,
-                    int* port);
+bool ParseHostAndPort(std::string::const_iterator host_and_port_begin,
+                      std::string::const_iterator host_and_port_end,
+                      std::string* host,
+                      int* port);
+bool ParseHostAndPort(const std::string& host_and_port,
+                      std::string* host,
+                      int* port);
+
+// Returns a host:port string for the given URL.
+std::string GetHostAndPort(const GURL& url);
+
+// Returns a host[:port] string for the given URL, where the port is omitted
+// if it is the default for the URL's scheme.
+std::string GetHostAndOptionalPort(const GURL& url);
 
 // Returns the string representation of an address, like "192.168.0.1".
 // Returns empty string on failure.
diff --git a/net/base/net_util_unittest.cc b/net/base/net_util_unittest.cc
index 57b74d7..79c1138 100644
--- a/net/base/net_util_unittest.cc
+++ b/net/base/net_util_unittest.cc
@@ -884,7 +884,7 @@ TEST(NetUtilTest, GetDirectoryListingEntry) {
 
 #endif
 
-TEST(NetUtilTest, GetHostAndPort) {
+TEST(NetUtilTest, ParseHostAndPort) {
   const struct {
     const char* input;
     bool success;
@@ -918,7 +918,7 @@ TEST(NetUtilTest, GetHostAndPort) {
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) {
     std::string host;
     int port;
-    bool ok = net::GetHostAndPort(tests[i].input, &host, &port);
+    bool ok = net::ParseHostAndPort(tests[i].input, &host, &port);
 
     EXPECT_EQ(tests[i].success, ok);
 
@@ -929,6 +929,43 @@ TEST(NetUtilTest, GetHostAndPort) {
   }
 }
 
+TEST(NetUtilTest, GetHostAndPort) {
+  const struct {
+    GURL url;
+    const char* expected_host_and_port;
+  } tests[] = {
+    { GURL("http://www.foo.com/x"), "www.foo.com:80"},
+    { GURL("http://www.foo.com:21/x"), "www.foo.com:21"},
+
+    // For IPv6 literals should always include the brackets.
+    { GURL("http://[1::2]/x"), "[1::2]:80"},
+    { GURL("http://[::a]:33/x"), "[::a]:33"},
+  };
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) {
+    std::string host_and_port = net::GetHostAndPort(tests[i].url);
+    EXPECT_EQ(std::string(tests[i].expected_host_and_port), host_and_port);
+  }
+}
+
+TEST(NetUtilTest, GetHostAndOptionalPort) {
+  const struct {
+    GURL url;
+    const char* expected_host_and_port;
+  } tests[] = {
+    { GURL("http://www.foo.com/x"), "www.foo.com"},
+    { GURL("http://www.foo.com:21/x"), "www.foo.com:21"},
+
+    // For IPv6 literals should always include the brackets.
+    { GURL("http://[1::2]/x"), "[1::2]"},
+    { GURL("http://[::a]:33/x"), "[::a]:33"},
+  };
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) {
+    std::string host_and_port = net::GetHostAndOptionalPort(tests[i].url);
+    EXPECT_EQ(std::string(tests[i].expected_host_and_port), host_and_port);
+  }
+}
+
+
 TEST(NetUtilTest, NetAddressToString_IPv4) {
   const struct {
     uint8 addr[4];
diff --git a/net/http/http_auth_handler_digest.cc b/net/http/http_auth_handler_digest.cc
index 683c268..759d697 100644
--- a/net/http/http_auth_handler_digest.cc
+++ b/net/http/http_auth_handler_digest.cc
@@ -116,7 +116,7 @@ void HttpAuthHandlerDigest::GetRequestMethodAndPath(
 
   if (target_ == HttpAuth::AUTH_PROXY && url.SchemeIs("https")) {
     *method = "CONNECT";
-    *path = url.host() + ":" + IntToString(url.EffectiveIntPort());
+    *path = GetHostAndPort(url);
   } else {
     *method = request->method;
     *path = HttpUtil::PathForRequest(url);
diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index 99a02ba..dfbbe23 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -46,12 +46,9 @@ void BuildRequestHeaders(const HttpRequestInfo* request_info,
       HttpUtil::SpecForRequest(request_info->url) :
       HttpUtil::PathForRequest(request_info->url);
   *request_headers =
-      StringPrintf("%s %s HTTP/1.1\r\nHost: %s",
+      StringPrintf("%s %s HTTP/1.1\r\nHost: %s\r\n",
                    request_info->method.c_str(), path.c_str(),
-                   request_info->url.host().c_str());
-  if (request_info->url.IntPort() != -1)
-    *request_headers += ":" + request_info->url.port();
-  *request_headers += "\r\n";
+                   GetHostAndOptionalPort(request_info->url).c_str());
 
   // For compat with HTTP/1.0 servers and proxies:
   if (using_proxy)
@@ -107,12 +104,9 @@ void BuildTunnelRequest(const HttpRequestInfo* request_info,
                         std::string* request_headers) {
   // RFC 2616 Section 9 says the Host request-header field MUST accompany all
   // HTTP/1.1 requests.
-  *request_headers = StringPrintf("CONNECT %s:%d HTTP/1.1\r\n",
-      request_info->url.host().c_str(), request_info->url.EffectiveIntPort());
-  *request_headers += "Host: " + request_info->url.host();
-  if (request_info->url.has_port())
-    *request_headers += ":" + request_info->url.port();
-  *request_headers += "\r\n";
+  *request_headers = StringPrintf("CONNECT %s HTTP/1.1\r\nHost: %s\r\n",
+      GetHostAndPort(request_info->url).c_str(),
+      GetHostAndOptionalPort(request_info->url).c_str());
 
   if (!request_info->user_agent.empty())
     StringAppendF(request_headers, "User-Agent: %s\r\n",
@@ -1115,8 +1109,8 @@ void HttpNetworkTransaction::LogTransactionMetrics() const {
 void HttpNetworkTransaction::LogBlockedTunnelResponse(
     int response_code) const {
   LOG(WARNING) << "Blocked proxy response with status " << response_code
-               << " to CONNECT request for " << request_->url.host() << ":"
-               << request_->url.EffectiveIntPort() << ".";
+               << " to CONNECT request for "
+               << GetHostAndPort(request_->url) << ".";
 }
 
 int HttpNetworkTransaction::DidReadResponseHeaders() {
@@ -1678,12 +1672,15 @@ void HttpNetworkTransaction::PopulateAuthChallenge(HttpAuth::Target target) {
   auth_info->scheme = ASCIIToWide(auth_handler_[target]->scheme());
   // TODO(eroman): decode realm according to RFC 2047.
   auth_info->realm = ASCIIToWide(auth_handler_[target]->realm());
+
+  std::string host_and_port;
   if (target == HttpAuth::AUTH_PROXY) {
-    auth_info->host = ASCIIToWide(proxy_info_.proxy_server().host_and_port());
+    host_and_port = proxy_info_.proxy_server().host_and_port();
   } else {
     DCHECK(target == HttpAuth::AUTH_SERVER);
-    auth_info->host = ASCIIToWide(request_->url.host());
+    host_and_port = GetHostAndPort(request_->url);
   }
+  auth_info->host_and_port = ASCIIToWide(host_and_port);
   response_.auth_challenge = auth_info;
 }
 
diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index 007f0fb..3d5fb44 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -970,8 +970,7 @@ TEST_F(HttpNetworkTransactionTest, BasicAuth) {
   // The password prompt info should have been set in response->auth_challenge.
   EXPECT_FALSE(response->auth_challenge.get() == NULL);
 
-  // TODO(eroman): this should really include the effective port (80)
-  EXPECT_EQ(L"www.google.com", response->auth_challenge->host);
+  EXPECT_EQ(L"www.google.com:80", response->auth_challenge->host_and_port);
   EXPECT_EQ(L"MyRealm1", response->auth_challenge->realm);
   EXPECT_EQ(L"basic", response->auth_challenge->scheme);
 
@@ -1048,8 +1047,7 @@ TEST_F(HttpNetworkTransactionTest, BasicAuthKeepAlive) {
   // The password prompt info should have been set in response->auth_challenge.
   EXPECT_FALSE(response->auth_challenge.get() == NULL);
 
-  // TODO(eroman): this should really include the effective port (80)
-  EXPECT_EQ(L"www.google.com", response->auth_challenge->host);
+  EXPECT_EQ(L"www.google.com:80", response->auth_challenge->host_and_port);
   EXPECT_EQ(L"MyRealm1", response->auth_challenge->realm);
   EXPECT_EQ(L"basic", response->auth_challenge->scheme);
 
@@ -1129,8 +1127,7 @@ TEST_F(HttpNetworkTransactionTest, BasicAuthKeepAliveNoBody) {
   // The password prompt info should have been set in response->auth_challenge.
   EXPECT_FALSE(response->auth_challenge.get() == NULL);
 
-  // TODO(eroman): this should really include the effective port (80)
-  EXPECT_EQ(L"www.google.com", response->auth_challenge->host);
+  EXPECT_EQ(L"www.google.com:80", response->auth_challenge->host_and_port);
   EXPECT_EQ(L"MyRealm1", response->auth_challenge->realm);
   EXPECT_EQ(L"basic", response->auth_challenge->scheme);
 
@@ -1213,8 +1210,7 @@ TEST_F(HttpNetworkTransactionTest, BasicAuthKeepAliveLargeBody) {
   // The password prompt info should have been set in response->auth_challenge.
   EXPECT_FALSE(response->auth_challenge.get() == NULL);
 
-  // TODO(eroman): this should really include the effective port (80)
-  EXPECT_EQ(L"www.google.com", response->auth_challenge->host);
+  EXPECT_EQ(L"www.google.com:80", response->auth_challenge->host_and_port);
   EXPECT_EQ(L"MyRealm1", response->auth_challenge->realm);
   EXPECT_EQ(L"basic", response->auth_challenge->scheme);
 
@@ -1304,8 +1300,7 @@ TEST_F(HttpNetworkTransactionTest, BasicAuthProxyKeepAlive) {
   // The password prompt info should have been set in response->auth_challenge.
   EXPECT_FALSE(response->auth_challenge.get() == NULL);
 
-  // TODO(eroman): this should really include the effective port (80)
-  EXPECT_EQ(L"myproxy:70", response->auth_challenge->host);
+  EXPECT_EQ(L"myproxy:70", response->auth_challenge->host_and_port);
   EXPECT_EQ(L"MyRealm1", response->auth_challenge->realm);
   EXPECT_EQ(L"basic", response->auth_challenge->scheme);
 
@@ -1329,8 +1324,7 @@ TEST_F(HttpNetworkTransactionTest, BasicAuthProxyKeepAlive) {
   // The password prompt info should have been set in response->auth_challenge.
   EXPECT_FALSE(response->auth_challenge.get() == NULL);
 
-  // TODO(eroman): this should really include the effective port (80)
-  EXPECT_EQ(L"myproxy:70", response->auth_challenge->host);
+  EXPECT_EQ(L"myproxy:70", response->auth_challenge->host_and_port);
   EXPECT_EQ(L"MyRealm1", response->auth_challenge->realm);
   EXPECT_EQ(L"basic", response->auth_challenge->scheme);
 }
@@ -1712,7 +1706,7 @@ TEST_F(HttpNetworkTransactionTest, BasicAuthProxyThenServer) {
   // The password prompt info should have been set in response->auth_challenge.
   EXPECT_FALSE(response->auth_challenge.get() == NULL);
 
-  EXPECT_EQ(L"myproxy:70", response->auth_challenge->host);
+  EXPECT_EQ(L"myproxy:70", response->auth_challenge->host_and_port);
   EXPECT_EQ(L"MyRealm1", response->auth_challenge->realm);
   EXPECT_EQ(L"basic", response->auth_challenge->scheme);
 
@@ -1728,8 +1722,7 @@ TEST_F(HttpNetworkTransactionTest, BasicAuthProxyThenServer) {
   EXPECT_FALSE(response == NULL);
   EXPECT_FALSE(response->auth_challenge.get() == NULL);
 
-  // TODO(eroman): this should really include the effective port (80)
-  EXPECT_EQ(L"www.google.com", response->auth_challenge->host);
+  EXPECT_EQ(L"www.google.com:80", response->auth_challenge->host_and_port);
   EXPECT_EQ(L"MyRealm1", response->auth_challenge->realm);
   EXPECT_EQ(L"basic", response->auth_challenge->scheme);
 
@@ -1860,8 +1853,7 @@ TEST_F(HttpNetworkTransactionTest, NTLMAuth1) {
   // The password prompt info should have been set in response->auth_challenge.
   EXPECT_FALSE(response->auth_challenge.get() == NULL);
 
-  // TODO(eroman): this should really include the effective port (80)
-  EXPECT_EQ(L"172.22.68.17", response->auth_challenge->host);
+  EXPECT_EQ(L"172.22.68.17:80", response->auth_challenge->host_and_port);
   EXPECT_EQ(L"", response->auth_challenge->realm);
   EXPECT_EQ(L"ntlm", response->auth_challenge->scheme);
 
@@ -2041,8 +2033,7 @@ TEST_F(HttpNetworkTransactionTest, NTLMAuth2) {
   // The password prompt info should have been set in response->auth_challenge.
   EXPECT_FALSE(response->auth_challenge.get() == NULL);
 
-  // TODO(eroman): this should really include the effective port (80)
-  EXPECT_EQ(L"172.22.68.17", response->auth_challenge->host);
+  EXPECT_EQ(L"172.22.68.17:80", response->auth_challenge->host_and_port);
   EXPECT_EQ(L"", response->auth_challenge->realm);
   EXPECT_EQ(L"ntlm", response->auth_challenge->scheme);
 
@@ -2069,8 +2060,7 @@ TEST_F(HttpNetworkTransactionTest, NTLMAuth2) {
   // The password prompt info should have been set in response->auth_challenge.
   EXPECT_FALSE(response->auth_challenge.get() == NULL);
 
-  // TODO(eroman): this should really include the effective port (80)
-  EXPECT_EQ(L"172.22.68.17", response->auth_challenge->host);
+  EXPECT_EQ(L"172.22.68.17:80", response->auth_challenge->host_and_port);
   EXPECT_EQ(L"", response->auth_challenge->realm);
   EXPECT_EQ(L"ntlm", response->auth_challenge->scheme);
 
@@ -2553,8 +2543,7 @@ TEST_F(HttpNetworkTransactionTest, BasicAuthCacheAndPreauth) {
     // response->auth_challenge.
     EXPECT_FALSE(response->auth_challenge.get() == NULL);
 
-    // TODO(eroman): this should really include the effective port (80)
-    EXPECT_EQ(L"www.google.com", response->auth_challenge->host);
+    EXPECT_EQ(L"www.google.com:80", response->auth_challenge->host_and_port);
     EXPECT_EQ(L"MyRealm1", response->auth_challenge->realm);
     EXPECT_EQ(L"basic", response->auth_challenge->scheme);
 
@@ -2644,8 +2633,7 @@ TEST_F(HttpNetworkTransactionTest, BasicAuthCacheAndPreauth) {
     // response->auth_challenge.
     EXPECT_FALSE(response->auth_challenge.get() == NULL);
 
-    // TODO(eroman): this should really include the effective port (80)
-    EXPECT_EQ(L"www.google.com", response->auth_challenge->host);
+    EXPECT_EQ(L"www.google.com:80", response->auth_challenge->host_and_port);
     EXPECT_EQ(L"MyRealm2", response->auth_challenge->realm);
     EXPECT_EQ(L"basic", response->auth_challenge->scheme);
 
@@ -2884,8 +2872,7 @@ TEST_F(HttpNetworkTransactionTest, BasicAuthCacheAndPreauth) {
     // response->auth_challenge.
     EXPECT_FALSE(response->auth_challenge.get() == NULL);
 
-    // TODO(eroman): this should really include the effective port (80)
-    EXPECT_EQ(L"www.google.com", response->auth_challenge->host);
+    EXPECT_EQ(L"www.google.com:80", response->auth_challenge->host_and_port);
     EXPECT_EQ(L"MyRealm1", response->auth_challenge->realm);
     EXPECT_EQ(L"basic", response->auth_challenge->scheme);
 
diff --git a/net/proxy/proxy_server.cc b/net/proxy/proxy_server.cc
index 0209a83..7475bbe 100644
--- a/net/proxy/proxy_server.cc
+++ b/net/proxy/proxy_server.cc
@@ -205,7 +205,7 @@ ProxyServer ProxyServer::FromSchemeHostAndPort(
 
   if (scheme != SCHEME_INVALID && scheme != SCHEME_DIRECT) {
     // If the scheme has a host/port, parse it.
-    bool ok = net::GetHostAndPort(begin, end, &host, &port);
+    bool ok = net::ParseHostAndPort(begin, end, &host, &port);
     if (!ok)
       return ProxyServer();  // Invalid -- failed parsing <host>[":"<port>]
   }
diff --git a/net/proxy/proxy_service.cc b/net/proxy/proxy_service.cc
index d172f92..0c2828b 100644
--- a/net/proxy/proxy_service.cc
+++ b/net/proxy/proxy_service.cc
@@ -609,6 +609,7 @@ bool ProxyService::ShouldBypassProxyForURL(const GURL& url) {
   // percent-encoded characters.
   StringToLowerASCII(&url_domain);
 
+  // TODO(eroman): use GetHostAndPort().
   std::string url_domain_and_port = url_domain + ":"
       + IntToString(url.EffectiveIntPort());
 
diff --git a/net/url_request/url_request_ftp_job.cc b/net/url_request/url_request_ftp_job.cc
index 2d2658b..13287a8 100644
--- a/net/url_request/url_request_ftp_job.cc
+++ b/net/url_request/url_request_ftp_job.cc
@@ -279,7 +279,8 @@ void URLRequestFtpJob::GetAuthChallengeInfo(
          (server_auth_->state == net::AUTH_STATE_NEED_AUTH));
   scoped_refptr<net::AuthChallengeInfo> auth_info = new net::AuthChallengeInfo;
   auth_info->is_proxy = false;
-  auth_info->host = UTF8ToWide(request_->url().host());
+  auth_info->host_and_port = ASCIIToWide(
+      net::GetHostAndPort(request_->url()));
   auth_info->scheme = L"";
   auth_info->realm = L"";
   result->swap(auth_info);