Fix dictionary domain check problem.

net/base/sdch_manager.cc

Index: net/base/sdch_manager.cc
diff --git a/net/base/sdch_manager.cc b/net/base/sdch_manager.cc
index 59589fb544723d386fa37019dd30a574cd8432d0..322a7b5c9d962f42eb19c1aeb4e36ba24436a176 100644
--- a/net/base/sdch_manager.cc
+++ b/net/base/sdch_manager.cc
@@ -13,6 +13,33 @@
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "net/url_request/url_request_http_job.h"
 
+namespace {
+
+// Canonicalize a URL from possibly containing a FQDN to the HDN format
+// specified in RFC 2965 so that trailing periods for (e.g.) domain matching
+// are not a concern.
+bool FQDNToHDN(std::string* host) {

[Ryan Sleevi, 2014/09/18 01:22:31]

1) Naming: "HDN" is not common
2) RFC 2965 is obsoleted by abarth's RFC 6265
3) HDN refers to a cookie-level concept that I don't think applies here (that
is, foo.com == foo.com., whereas for HDN, foo.com != foo.com. ) (c.f. 4.1.2.3 of
6265)

[Randy Smith (Not in Mondays), 2014/09/22 19:22:10]

Changed to "StripTrailingDot" and edited the explanatory text.  I'll note for
possible future discussion: I wouldn't mind having functions that refer directly
to the controlling terminology in the RFCs (which is where FQDNToHDN came from).
 However, based on our discussion, I don't think the relationship is precisely
specified in the RFCs, so I'll go with the concrete description of what I'm
doing.

+  if (host->at(host->size() - 1) != '.')

[Ryan Sleevi, 2014/09/18 01:22:31]

Why are you using at (which throws) vs [] (which doesn't?)

[Randy Smith (Not in Mondays), 2014/09/22 19:22:10]

Because I found (*host)[] awkward and wasn't aware of the exceptional behavior
:-}.  Done.

+    return false;
+
+  host->resize(host->size() - 1);
+  return true;
+}
+
+void FQDNToHDNURL(GURL* gurl) {
+  std::string host(gurl->host());
+
+  if (!FQDNToHDN(&host))
+    return;
+
+  GURL::Replacements replacements;
+  replacements.SetHostStr(host);
+  *gurl = gurl->ReplaceComponents(replacements);
+  return;
+}
+
+}  // namespace
+
 namespace net {
 
 //------------------------------------------------------------------------------
@@ -405,15 +432,19 @@ void SdchManager::GetVcdiffDictionary(
     const GURL& referring_url,
     scoped_refptr<Dictionary>* dictionary) {
   DCHECK(CalledOnValidThread());
+
+  GURL referring_url_hdn(referring_url);
+  FQDNToHDNURL(&referring_url_hdn);
+
   *dictionary = NULL;
   DictionaryMap::iterator it = dictionaries_.find(server_hash);
   if (it == dictionaries_.end()) {
     return;
   }
   scoped_refptr<Dictionary> matching_dictionary = it->second;
-  if (!IsInSupportedDomain(referring_url))
+  if (!IsInSupportedDomain(referring_url_hdn))
     return;
-  if (!matching_dictionary->CanUse(referring_url))
+  if (!matching_dictionary->CanUse(referring_url_hdn))

[Ryan Sleevi, 2014/09/18 01:22:30]

1) Why do these functions use GURLs, rather than just hosts?
2) Rather than stripping '.', why can you not canonicalize by appending '.' if
necessary?

The latter is what we do in X509Certificate (for RFC6125 hostname matching).

[Randy Smith (Not in Mondays), 2014/09/22 19:22:10]

I'm presuming you mean IsInSupportedDomain() and CanUse().  CanUse() checks path
& port as well as host.  IsInSupportedDomain() checks the scheme as well as the
host/domain.  IsInSupportedDomain() is problematic for other reasons, and I'd be
open to a refactor (though I'd want to do it in such a way as to not cause even
more problems for CL 423813002) but I'm basically happy with the abstraction
boundary for CanUse().
So I'm willing to do that for consistency sake, and will if you ask me again,
but I'd rather keep the code and spec in alignment.  I.e. if the spec prohibits
a trailing "." for dictionary domains, I'd rather keep the dictionary domain
free of the trailing ".", and strip it from the URL if it's there.

     return;
   *dictionary = matching_dictionary;
 }
@@ -424,12 +455,16 @@ void SdchManager::GetVcdiffDictionary(
 void SdchManager::GetAvailDictionaryList(const GURL& target_url,
                                          std::string* list) {
   DCHECK(CalledOnValidThread());
+
+  GURL target_url_hdn(target_url);
+  FQDNToHDNURL(&target_url_hdn);
+
   int count = 0;
   for (DictionaryMap::iterator it = dictionaries_.begin();
        it != dictionaries_.end(); ++it) {
-    if (!IsInSupportedDomain(target_url))
+    if (!IsInSupportedDomain(target_url_hdn))
       continue;
-    if (!it->second->CanAdvertise(target_url))
+    if (!it->second->CanAdvertise(target_url_hdn))
       continue;
     ++count;
     if (!list->empty())
@@ -550,10 +585,15 @@ void SdchManager::AddSdchDictionary(const std::string& dictionary_text,
     line_start = line_end + 1;
   }
 
-  if (!IsInSupportedDomain(dictionary_url))
+  // Convert away from FQDN to the HDN format described in RFC 2695.
+  FQDNToHDN(&domain);
+  GURL dictionary_url_hdn(dictionary_url);
+  FQDNToHDNURL(&dictionary_url_hdn);
+
+  if (!IsInSupportedDomain(dictionary_url_hdn))
     return;
 
-  if (!Dictionary::CanSet(domain, path, ports, dictionary_url))
+  if (!Dictionary::CanSet(domain, path, ports, dictionary_url_hdn))
     return;
 
   // TODO(jar): Remove these hacks to preclude a DOS attack involving piles of
@@ -574,7 +614,7 @@ void SdchManager::AddSdchDictionary(const std::string& dictionary_text,
            << " and server hash " << server_hash;
   Dictionary* dictionary =
       new Dictionary(dictionary_text, header_end + 2, client_hash,
-                     dictionary_url, domain, path, expiration, ports);
+                     dictionary_url_hdn, domain, path, expiration, ports);
   dictionaries_[server_hash] = dictionary;
   return;
 }