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..32f09da6c85f9b2712b5be437ace1ff0809987d3 100644
--- a/net/base/sdch_manager.cc
+++ b/net/base/sdch_manager.cc
@@ -13,6 +13,40 @@
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "net/url_request/url_request_http_job.h"
 
+namespace {
+
+// Spec collision warning: URLs may have domain names which have a
+// trailing "." (e.g. "http://this.is.a.domain.com./file.txt").  The
+// SDCH spec was written to parallel RFC 2965 (an old version of the
+// cookie spec, which didn't precisely address the issue of trailing "."s.
+// The newest version of the cookie spec, RFC 6265, specifically prohibits
+// trailing "."s from cookie domains.  So we prohibit trailing "."s in
+// dictionary domains, and strip any trailing "." from URL domains.
+bool StripTrailingDot(std::string* host) {
+  if (host->size() == 0)

[Ryan Sleevi, 2014/09/22 20:30:37]

host->empty()

(a nit I hold on to from the days of 'dumb' stl implementations, but also the
generic form of not requiring counting like other containers MAY)

[Randy Smith (Not in Mondays), 2014/09/23 19:52:58]

Done.

+    return false;
+
+  if ((*host)[host->size() - 1] != '.')

[Ryan Sleevi, 2014/09/22 20:30:36]

if (*host->rbegin() != '.')
  return false;

Laziness is a virtue :)

[Randy Smith (Not in Mondays), 2014/09/23 19:52:58]

Agreed :-}.  Done.

+    return false;
+
+  host->resize(host->size() - 1);
+  return true;
+}
+
+void StripTrailingDotURL(GURL* gurl) {

[Ryan Sleevi, 2014/09/22 20:30:36]

naming wise, this felt a little weird, although I understand how you got here.

Perhaps just something as "NormalizeToDictionaryURL"?

[Randy Smith (Not in Mondays), 2014/09/23 19:52:58]

I don't want "NormalizeToDictionary" because to me that pre-supposes the results
of our discussion about the spec framework this code is to be written in. 
Having said that, I agree with your next comment, which frees up
"StripTrailingDot", so I'll use that.

+  std::string host(gurl->host());
+
+  if (!StripTrailingDot(&host))
+    return;

[Ryan Sleevi, 2014/09/22 20:30:36]

Just inline this? You never use "StripTrailingDot" in this file

[Randy Smith (Not in Mondays), 2014/09/23 19:52:58]

Done.

+
+  GURL::Replacements replacements;
+  replacements.SetHostStr(host);
+  *gurl = gurl->ReplaceComponents(replacements);
+  return;
+}
+
+}  // namespace
+
 namespace net {
 
 //------------------------------------------------------------------------------
@@ -405,15 +439,19 @@ void SdchManager::GetVcdiffDictionary(
     const GURL& referring_url,
     scoped_refptr<Dictionary>* dictionary) {
   DCHECK(CalledOnValidThread());
+
+  GURL referring_url_hdn(referring_url);
+  StripTrailingDotURL(&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/22 20:30:37]

OK, I'm fairly confused now why it's necessary to do this.

SdchManager::Dictionary::CanUse uses DomainMatch, which just calls

gurl.DomainIs(...)

and DomainIs handles the trailing . just fine (c.f
https://code.google.com/p/chromium/codesearch#chromium/src/url/gurl.cc&rcl=14...
)

[Randy Smith (Not in Mondays), 2014/09/22 20:43:51]

This isn't necessary; it's the checks in AddSdchDictionary that are necessary. 
The failure that produced this CL was the referring URL for an added dictionary
having a "." at the end and also being a subdomain of the domain it was trying
to set an encoding dictionary for, which should have been bounced because you
aren't allowed to set dictionaries for parent domains (i.e. you can't set a
dictionary for google.com from www.hosted.google.com).  It's a pure suffix check
(as specified in the spec :-J), not done through DomainMatch, which failed when
the trailing "." was present.  

I put it here (and forgot to change the naming, sorry) because I was doing a
general change to strip trailing dots from referring URLs; I didn't want to have
code in one place that did the strip and in another that didn't.  If you'd like,
I can convert this to a comment to the effect that CanUse uses DomainMatch(),
which handles the trailing ".".  But I suspect what you want to focus on is what
the code in Dictionary::CanSet() and callers looks like, and then figure out
from that whether or not we want a change here.

     return;
   *dictionary = matching_dictionary;
 }
@@ -424,12 +462,16 @@ void SdchManager::GetVcdiffDictionary(
 void SdchManager::GetAvailDictionaryList(const GURL& target_url,
                                          std::string* list) {
   DCHECK(CalledOnValidThread());
+
+  GURL target_url_hdn(target_url);
+  StripTrailingDotURL(&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())
@@ -479,7 +521,7 @@ void SdchManager::SetAllowLatencyExperiment(const GURL& url, bool enable) {
 }
 
 void SdchManager::AddSdchDictionary(const std::string& dictionary_text,

[Ryan Sleevi, 2014/09/22 20:55:26]

style nits: Perhaps for a future CL, but this should be on a line by itself, per
the style guide, or dictionary_url should be aligned to the open (

-    const GURL& dictionary_url) {
+    const GURL& dictionary_url_const) {

[Ryan Sleevi, 2014/09/22 20:55:26]

pedantry naming: dictionary_url here, and normalized_dictionary_url below
(which, yes, requires more code line changes)

I just find the _const an unnerving "Reverse Hungarian Notation" ;)

[Randy Smith (Not in Mondays), 2014/09/23 19:52:58]

Done.

   DCHECK(CalledOnValidThread());
   std::string client_hash;
   std::string server_hash;
@@ -550,6 +592,19 @@ void SdchManager::AddSdchDictionary(const std::string& dictionary_text,
     line_start = line_end + 1;

[Ryan Sleevi, 2014/09/22 20:55:27]

if line_end == npos, this will wrap around to 0 (since npos is typically -1,
it's an unsigned int, so will overflow)

However, because of 590, we'll never hit that (since npos will always be >
header_end), but it does mean we'll have parsed a line that lacked a terminator,
and violated your assumed DCHECKs.

I'd feel rather awesome if this used base/strings/string_tokenizer in the
future, but I'll leave that to a future CL. But yeah, complex parsing in //net,
scurry :)

   }
 
+  // Forbid dictionary with trailing dots; the SDCH spec is explicitly
+  // patterned after an old cookie spec (RFC 2695) which is silent on
+  // the issue of trailing dots, and the newer cookie spec (RFC 6265)
+  // explicitly forbids them.
+  if (domain.size() != 0 && domain[domain.size() - 1] == '.') {
+    SdchErrorRecovery(DICTIONARY_DOMAIN_HAS_TRAILING_PERIOD);
+    return;

[Ryan Sleevi, 2014/09/22 20:55:27]

From your reply, it _seems_ that this is the ONLY necessary change, is that
correct? All other bits are handled by DomainIs()?

[Randy Smith (Not in Mondays), 2014/09/22 21:09:56]

I don't believe so; I think the stripping of the dot from the
dictionary_url{_const} below is the most important point.  Again, the failing
example in the bug (now immortalized in one of the tests) was the dictionary_url
having a trailing ".", and the domain for the SDCH dictionary *not* having that
".".

[Ryan Sleevi, 2014/09/22 22:49:42]

I tried to work through why this is. That is, I'm trying to tease from you what
the _necessary_ changes were, versus which you were trying to do for code
health/consistency. Altogether, this feels like a much larger change at present,
and I have trouble reasoning about the correctness of it.

That is, broadly speaking, I'm trying to understand where you're making domain
comparisons and why these are problematic. Rather than normalizing all the entry
points, we should be trying to reduce the comparisons altogether, and, whenever
possible, defer to GURL (which will do the right thing)

IsInSupportedDomain - ok, sure, the blacklist considers google.com !=
google.com. . I can't tell if that's a feature (subdomain.foo.com != foo.com) or
a bug.

CanSet -
  - GetDomainAndRegistry (handles trailing .; I think)
  - DomainMatch(dictionary_url, domain) - does the right thing (re: GURL)

So the only place that it matters is when you start matching referrer_url_host
and postfix_domain_index. I've commented more in that function.

+  }
+
+  // Strip trailing dot from the dictionary url before comparison.
+  GURL dictionary_url(dictionary_url_const);
+  StripTrailingDotURL(&dictionary_url);
+
   if (!IsInSupportedDomain(dictionary_url))
     return;