Sdch view for net-internals

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 8b7481b50b582b80829ac0f2b1b928333c1b1a38..cb11caff227bd467c17b3424969ba469c3cb0c1e 100644
--- a/net/base/sdch_manager.cc
+++ b/net/base/sdch_manager.cc
@@ -9,6 +9,7 @@
 #include "base/metrics/histogram.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
+#include "base/values.h"
 #include "crypto/sha2.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "net/url_request/url_request_http_job.h"
@@ -56,7 +57,7 @@ SdchManager::Dictionary::Dictionary(const std::string& dictionary_text,
 SdchManager::Dictionary::~Dictionary() {
 }
 
-bool SdchManager::Dictionary::CanAdvertise(const GURL& target_url) {
+bool SdchManager::Dictionary::CanAdvertise(const GURL& target_url) const {
   /* The specific rules of when a dictionary should be advertised in an
      Avail-Dictionary header are modeled after the rules for cookie scoping. The
      terms "domain-match" and "pathmatch" are defined in RFC 2965 [6]. A
@@ -91,10 +92,11 @@ bool SdchManager::Dictionary::CanAdvertise(const GURL& target_url) {
 // Security functions restricting loads and use of dictionaries.
 
 // static
-bool SdchManager::Dictionary::CanSet(const std::string& domain,
-                                     const std::string& path,
-                                     const std::set<int>& ports,
-                                     const GURL& dictionary_url) {
+SdchManager::ProblemCodes SdchManager::Dictionary::CanSet(
+    const std::string& domain,
+    const std::string& path,
+    const std::set<int>& ports,
+    const GURL& dictionary_url) {
   /*
   A dictionary is invalid and must not be stored if any of the following are
   true:
@@ -113,20 +115,16 @@ bool SdchManager::Dictionary::CanSet(const std::string& domain,
   // and hence the conservative approach is to not allow any redirects (if there
   // were any... then don't allow the dictionary to be set).
 
-  if (domain.empty()) {
-    SdchErrorRecovery(DICTIONARY_MISSING_DOMAIN_SPECIFIER);
-    return false;  // Domain is required.
-  }
+  if (domain.empty())
+    return DICTIONARY_MISSING_DOMAIN_SPECIFIER;  // Domain is required.
+
   if (registry_controlled_domains::GetDomainAndRegistry(
-        domain,
-        registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES).empty()) {
-    SdchErrorRecovery(DICTIONARY_SPECIFIES_TOP_LEVEL_DOMAIN);
-    return false;  // domain was a TLD.
-  }
-  if (!Dictionary::DomainMatch(dictionary_url, domain)) {
-    SdchErrorRecovery(DICTIONARY_DOMAIN_NOT_MATCHING_SOURCE_URL);
-    return false;
-  }
+          domain, registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES)
+          .empty())
+    return DICTIONARY_SPECIFIES_TOP_LEVEL_DOMAIN;  // domain was a TLD.
+
+  if (!Dictionary::DomainMatch(dictionary_url, domain))
+    return DICTIONARY_DOMAIN_NOT_MATCHING_SOURCE_URL;
 
   std::string referrer_url_host = dictionary_url.host();
   size_t postfix_domain_index = referrer_url_host.rfind(domain);
@@ -134,23 +132,19 @@ bool SdchManager::Dictionary::CanSet(const std::string& domain,
   if (referrer_url_host.size() == postfix_domain_index + domain.size()) {
     // It is a postfix... so check to see if there's a dot in the prefix.
     size_t end_of_host_index = referrer_url_host.find_first_of('.');
-    if (referrer_url_host.npos != end_of_host_index  &&
-        end_of_host_index < postfix_domain_index) {
-      SdchErrorRecovery(DICTIONARY_REFERER_URL_HAS_DOT_IN_PREFIX);
-      return false;
-    }
+    if (referrer_url_host.npos != end_of_host_index &&
+        end_of_host_index < postfix_domain_index)
+      return DICTIONARY_REFERER_URL_HAS_DOT_IN_PREFIX;
   }
 
-  if (!ports.empty()
-      && 0 == ports.count(dictionary_url.EffectiveIntPort())) {
-    SdchErrorRecovery(DICTIONARY_PORT_NOT_MATCHING_SOURCE_URL);
-    return false;
-  }
-  return true;
+  if (!ports.empty() && 0 == ports.count(dictionary_url.EffectiveIntPort()))
+    return DICTIONARY_PORT_NOT_MATCHING_SOURCE_URL;
+
+  return PROBLEM_CODE_OK;
 }
 
-// static
-bool SdchManager::Dictionary::CanUse(const GURL& referring_url) {
+SdchManager::ProblemCodes SdchManager::Dictionary::CanUse(
+    const GURL& referring_url) const {
   /*
     1. The request URL's host name domain-matches the Domain attribute of the
       dictionary.
@@ -162,39 +156,30 @@ bool SdchManager::Dictionary::CanUse(const GURL& referring_url) {
     HTTPS support AND the dictionary acquisition scheme matches the target
      url scheme.
   */
-  if (!DomainMatch(referring_url, domain_)) {
-    SdchErrorRecovery(DICTIONARY_FOUND_HAS_WRONG_DOMAIN);
-    return false;
-  }
-  if (!ports_.empty()
-      && 0 == ports_.count(referring_url.EffectiveIntPort())) {
-    SdchErrorRecovery(DICTIONARY_FOUND_HAS_WRONG_PORT_LIST);
-    return false;
-  }
-  if (path_.size() && !PathMatch(referring_url.path(), path_)) {
-    SdchErrorRecovery(DICTIONARY_FOUND_HAS_WRONG_PATH);
-    return false;
-  }
-  if (!SdchManager::secure_scheme_supported() &&
-      referring_url.SchemeIsSecure()) {
-    SdchErrorRecovery(DICTIONARY_FOUND_HAS_WRONG_SCHEME);
-    return false;
-  }
-  if (referring_url.SchemeIsSecure() != url_.SchemeIsSecure()) {
-    SdchErrorRecovery(DICTIONARY_FOUND_HAS_WRONG_SCHEME);
-    return false;
-  }
+  if (!DomainMatch(referring_url, domain_))
+    return DICTIONARY_FOUND_HAS_WRONG_DOMAIN;
+
+  if (!ports_.empty() && 0 == ports_.count(referring_url.EffectiveIntPort()))
+    return DICTIONARY_FOUND_HAS_WRONG_PORT_LIST;
+
+  if (path_.size() && !PathMatch(referring_url.path(), path_))
+    return DICTIONARY_FOUND_HAS_WRONG_PATH;
+
+  if (!SdchManager::secure_scheme_supported() && referring_url.SchemeIsSecure())
+    return DICTIONARY_FOUND_HAS_WRONG_SCHEME;
+
+  if (referring_url.SchemeIsSecure() != url_.SchemeIsSecure())
+    return DICTIONARY_FOUND_HAS_WRONG_SCHEME;
 
   // TODO(jar): Remove overly restrictive failsafe test (added per security
   // review) when we have a need to be more general.
-  if (!referring_url.SchemeIsHTTPOrHTTPS()) {
-    SdchErrorRecovery(ATTEMPT_TO_DECODE_NON_HTTP_DATA);
-    return false;
-  }
+  if (!referring_url.SchemeIsHTTPOrHTTPS())
+    return ATTEMPT_TO_DECODE_NON_HTTP_DATA;
 
-  return true;
+  return PROBLEM_CODE_OK;
 }
 
+// static
 bool SdchManager::Dictionary::PathMatch(const std::string& path,
                                         const std::string& restriction) {
   /*  Must be either:
@@ -315,38 +300,62 @@ int SdchManager::BlacklistDomainExponential(const std::string& domain) {
 
 bool SdchManager::IsInSupportedDomain(const GURL& url) {
   DCHECK(CalledOnValidThread());
+  if (!IsSdchEnabledForUrl(url))
+    return false;
+
+  if (IsInBlacklistedDomain(url)) {
+    SdchErrorRecovery(DOMAIN_BLACKLIST_INCLUDES_TARGET);
+    return false;
+  }
+  return true;
+}
+
+// static
+bool SdchManager::IsSdchEnabledForUrl(const GURL& url) {
   if (!g_sdch_enabled_ )
     return false;
 
   if (!secure_scheme_supported() && url.SchemeIsSecure())
     return false;
 
+  return true;
+}
+
+bool SdchManager::IsInBlacklistedDomain(const GURL& url) {
   if (blacklisted_domains_.empty())
-    return true;
+    return false;
 
   std::string domain(base::StringToLowerASCII(url.host()));
   DomainCounter::iterator it = blacklisted_domains_.find(domain);
   if (blacklisted_domains_.end() == it)
-    return true;
+    return false;
 
   int count = it->second - 1;
   if (count > 0)
     blacklisted_domains_[domain] = count;
   else
     blacklisted_domains_.erase(domain);
-  SdchErrorRecovery(DOMAIN_BLACKLIST_INCLUDES_TARGET);
-  return false;
+
+  return true;
 }
 
-void SdchManager::FetchDictionary(const GURL& request_url,
-                                  const GURL& dictionary_url) {
+SdchManager::ProblemCodes SdchManager::FetchDictionary(
+    const GURL& request_url,
+    const GURL& dictionary_url) {
   DCHECK(CalledOnValidThread());
-  if (CanFetchDictionary(request_url, dictionary_url) && fetcher_.get())
+  ProblemCodes rv = CanFetchDictionary(request_url, dictionary_url);
+  if (rv != PROBLEM_CODE_OK)
+    return rv;
+
+  if (fetcher_.get())
     fetcher_->Schedule(dictionary_url);
+
+  return PROBLEM_CODE_OK;
 }
 
-bool SdchManager::CanFetchDictionary(const GURL& referring_url,
-                                     const GURL& dictionary_url) const {
+SdchManager::ProblemCodes SdchManager::CanFetchDictionary(
+    const GURL& referring_url,
+    const GURL& dictionary_url) const {
   DCHECK(CalledOnValidThread());
   /* The user agent may retrieve a dictionary from the dictionary URL if all of
      the following are true:
@@ -361,34 +370,32 @@ bool SdchManager::CanFetchDictionary(const GURL& referring_url,
   // Item (1) above implies item (2).  Spec should be updated.
   // I take "host name match" to be "is identical to"
   if (referring_url.host() != dictionary_url.host() ||
-      referring_url.scheme() != dictionary_url.scheme()) {
-    SdchErrorRecovery(DICTIONARY_LOAD_ATTEMPT_FROM_DIFFERENT_HOST);
-    return false;
-  }
-  if (!secure_scheme_supported() && referring_url.SchemeIsSecure()) {
-    SdchErrorRecovery(DICTIONARY_SELECTED_FOR_SSL);
-    return false;
-  }
+      referring_url.scheme() != dictionary_url.scheme())
+    return DICTIONARY_LOAD_ATTEMPT_FROM_DIFFERENT_HOST;
+
+  if (!secure_scheme_supported() && referring_url.SchemeIsSecure())
+    return DICTIONARY_SELECTED_FOR_SSL;
 
   // TODO(jar): Remove this failsafe conservative hack which is more restrictive
   // than current SDCH spec when needed, and justified by security audit.
-  if (!referring_url.SchemeIsHTTPOrHTTPS()) {
-    SdchErrorRecovery(DICTIONARY_SELECTED_FROM_NON_HTTP);
-    return false;
-  }
+  if (!referring_url.SchemeIsHTTPOrHTTPS())
+    return DICTIONARY_SELECTED_FROM_NON_HTTP;
 
-  return true;
+  return PROBLEM_CODE_OK;
 }
 
 bool SdchManager::AddSdchDictionary(const std::string& dictionary_text,
-    const GURL& dictionary_url) {
+                                    const GURL& dictionary_url,
+                                    ProblemCodes* problem) {
   DCHECK(CalledOnValidThread());
+  DCHECK(problem);
   std::string client_hash;
   std::string server_hash;
   GenerateHash(dictionary_text, &client_hash, &server_hash);
+  *problem = PROBLEM_CODE_OK;
   if (dictionaries_.find(server_hash) != dictionaries_.end()) {
-    SdchErrorRecovery(DICTIONARY_ALREADY_LOADED);
-    return false;  // Already loaded.
+    *problem = DICTIONARY_ALREADY_LOADED;  // Already loaded.
+    return false;
   }
 
   std::string domain, path;
@@ -396,15 +403,16 @@ bool SdchManager::AddSdchDictionary(const std::string& dictionary_text,
   base::Time expiration(base::Time::Now() + base::TimeDelta::FromDays(30));
 
   if (dictionary_text.empty()) {
-    SdchErrorRecovery(DICTIONARY_HAS_NO_TEXT);
-    return false;  // Missing header.
+    *problem = DICTIONARY_HAS_NO_TEXT;  // Empty response.
+    return false;
   }
 
   size_t header_end = dictionary_text.find("\n\n");
   if (std::string::npos == header_end) {
-    SdchErrorRecovery(DICTIONARY_HAS_NO_HEADER);
-    return false;  // Missing header.
+    *problem = DICTIONARY_HAS_NO_HEADER;  // Missing header.
+    return false;
   }
+
   size_t line_start = 0;  // Start of line being parsed.
   while (1) {
     size_t line_end = dictionary_text.find('\n', line_start);
@@ -413,8 +421,9 @@ bool SdchManager::AddSdchDictionary(const std::string& dictionary_text,
 
     size_t colon_index = dictionary_text.find(':', line_start);
     if (std::string::npos == colon_index) {
-      SdchErrorRecovery(DICTIONARY_HEADER_LINE_MISSING_COLON);
-      return false;  // Illegal line missing a colon.
+      // Illegal line missing a colon.
+      *problem = DICTIONARY_HEADER_LINE_MISSING_COLON;
+      return false;
     }
 
     if (colon_index > line_end)
@@ -433,8 +442,9 @@ bool SdchManager::AddSdchDictionary(const std::string& dictionary_text,
       } else if (name == "path") {
         path = value;
       } else if (name == "format-version") {
-        if (value != "1.0")
+        if (value != "1.0") {
           return false;
+        }

[Randy Smith (Not in Mondays), 2014/08/13 17:35:45]

The extra braces aren't necessary by the chrome style guide, but I don't object
if you want them.

[baranovich, 2014/08/13 19:13:48]

Done.

       } else if (name == "max-age") {
         int64 seconds;
         base::StringToInt64(value, &seconds);
@@ -452,22 +462,30 @@ bool SdchManager::AddSdchDictionary(const std::string& dictionary_text,
     line_start = line_end + 1;
   }
 
-  if (!IsInSupportedDomain(dictionary_url))
+  if (!IsSdchEnabledForUrl(dictionary_url))
+    return false;
+
+  if (IsInBlacklistedDomain(dictionary_url)) {
+    *problem = DOMAIN_BLACKLIST_INCLUDES_TARGET;
     return false;
+  }
 
-  if (!Dictionary::CanSet(domain, path, ports, dictionary_url))
+  ProblemCodes err = Dictionary::CanSet(domain, path, ports, dictionary_url);
+  if (err != PROBLEM_CODE_OK) {
+    *problem = err;
     return false;
+  }
 
   // TODO(jar): Remove these hacks to preclude a DOS attack involving piles of
   // useless dictionaries.  We should probably have a cache eviction plan,
   // instead of just blocking additions.  For now, with the spec in flux, it
   // is probably not worth doing eviction handling.
   if (kMaxDictionarySize < dictionary_text.size()) {
-    SdchErrorRecovery(DICTIONARY_IS_TOO_LARGE);
+    *problem = DICTIONARY_IS_TOO_LARGE;
     return false;
   }
   if (kMaxDictionaryCount <= dictionaries_.size()) {
-    SdchErrorRecovery(DICTIONARY_COUNT_EXCEEDED);
+    *problem = DICTIONARY_COUNT_EXCEEDED;
     return false;
   }
 
@@ -478,25 +496,31 @@ bool SdchManager::AddSdchDictionary(const std::string& dictionary_text,
       new Dictionary(dictionary_text, header_end + 2, client_hash,
                      dictionary_url, domain, path, expiration, ports);
   dictionaries_[server_hash] = dictionary;
+
   return true;
 }
 
-void SdchManager::GetVcdiffDictionary(
+SdchManager::ProblemCodes SdchManager::GetVcdiffDictionary(
     const std::string& server_hash,
     const GURL& referring_url,
     scoped_refptr<Dictionary>* dictionary) {
   DCHECK(CalledOnValidThread());
   *dictionary = NULL;
   DictionaryMap::iterator it = dictionaries_.find(server_hash);
-  if (it == dictionaries_.end()) {
-    return;
-  }
+  if (it == dictionaries_.end())
+    return PROBLEM_CODE_OK;  // not a problem. Just no dictionary.
+
   scoped_refptr<Dictionary> matching_dictionary = it->second;
-  if (!IsInSupportedDomain(referring_url))
-    return;
-  if (!matching_dictionary->CanUse(referring_url))
-    return;
-  *dictionary = matching_dictionary;
+  if (!IsSdchEnabledForUrl(referring_url))
+    return PROBLEM_CODE_OK;  // not a problem. SDCH is disabled.

[Randy Smith (Not in Mondays), 2014/08/13 17:35:45]

Hmmm.  Wouldn't we want to see this in net-internals?  (Though I don't think it
should happen.)

[baranovich, 2014/08/13 19:13:48]

Then we need some new enum values (smth like SDCH_DISABLED and
SDCH_HTTPS_DISABLED)
On 2014/08/13 17:35:45, rdsmith wrote:

+
+  if (IsInBlacklistedDomain(referring_url))
+    return DOMAIN_BLACKLIST_INCLUDES_TARGET;
+
+  ProblemCodes rv = matching_dictionary->CanUse(referring_url);
+  if (rv == PROBLEM_CODE_OK)
+    *dictionary = matching_dictionary;
+  return rv;
 }
 
 // TODO(jar): If we have evictions from the dictionaries_, then we need to
@@ -508,8 +532,9 @@ void SdchManager::GetAvailDictionaryList(const GURL& target_url,
   int count = 0;
   for (DictionaryMap::iterator it = dictionaries_.begin();
        it != dictionaries_.end(); ++it) {
-    if (!IsInSupportedDomain(target_url))
+    if (!IsInSupportedDomain(target_url)) {

[Randy Smith (Not in Mondays), 2014/08/13 17:35:45]

I think it makes sense to avoid unnecessary changes in areas of the code where
you're not doing other changes.  I could even make a claim that adding the
braces goes against the intent of the style guide (which says to avoid vertical
whitespace if possible and that it's ok to skip braces on conditionals as long
as the containing statement and the condition itself is each one line in length,
and the then and else clause (if existing) match in terms of being braced.

[baranovich, 2014/08/13 19:13:48]

Done.

       continue;
+    }
     if (!it->second->CanAdvertise(target_url))
       continue;
     ++count;
@@ -537,6 +562,49 @@ void SdchManager::GenerateHash(const std::string& dictionary_text,
   DCHECK_EQ(client_hash->length(), 8u);
 }
 
+base::Value* SdchManager::SdchInfoToValue() const {
+  base::DictionaryValue* value = new base::DictionaryValue();
+
+  value->SetBoolean("sdch_enabled", sdch_enabled());
+  value->SetBoolean("secure_scheme_support", secure_scheme_supported());
+
+  base::ListValue* entry_list = new base::ListValue();
+  for (DictionaryMap::const_iterator it = dictionaries_.begin();
+       it != dictionaries_.end();
+       ++it) {
+    base::DictionaryValue* entry_dict = new base::DictionaryValue();
+    entry_dict->SetString("url", it->second->url().spec());
+    entry_dict->SetString("client_hash", it->second->client_hash());
+    entry_dict->SetString("domain", it->second->domain());
+    entry_dict->SetString("path", it->second->path());
+    entry_dict->SetString("expiration",
+        base::Int64ToString(it->second->expiration().ToInternalValue()));
+    base::ListValue* port_list = new base::ListValue();
+    for (std::set<int>::const_iterator port_it = it->second->ports().begin();
+         port_it != it->second->ports().end(); ++port_it) {
+      port_list->AppendInteger(*port_it);
+    }
+    entry_dict->Set("ports", port_list);
+    entry_dict->SetString("server_hash", it->first);
+    entry_list->Append(entry_dict);
+  }
+  value->Set("dictionaries", entry_list);
+
+  entry_list = new base::ListValue();
+  for (DomainCounter::const_iterator it = blacklisted_domains_.begin();
+       it != blacklisted_domains_.end();
+       ++it) {
+    base::DictionaryValue* entry_dict = new base::DictionaryValue();
+    entry_dict->SetString("domain", it->first);
+    if (it->second != INT_MAX)
+      entry_dict->SetInteger("tries", it->second);
+    entry_list->Append(entry_dict);
+  }
+  value->Set("blacklisted", entry_list);
+
+  return value;
+}
+
 //------------------------------------------------------------------------------
 // Methods for supporting latency experiments.
 
@@ -555,6 +623,8 @@ void SdchManager::SetAllowLatencyExperiment(const GURL& url, bool enable) {
   ExperimentSet::iterator it = allow_latency_experiment_.find(url.host());
   if (allow_latency_experiment_.end() == it)
     return;  // It was already erased, or never allowed.
+
+  // Do not send error up. It's internal stuff, I guess.

[Randy Smith (Not in Mondays), 2014/08/13 17:35:45]

"I guess" doesn't seem like the right phrasing for a comment :-}; rephrase?   

You're welcome to surface this error or not as you wish; the latency experiment
is a random test where we hold back on 1 in 100 connections from using SDCH and
report UMA to compare the results, and the places I see where we disallow that
experiment is when we blacklist domains.  

[baranovich, 2014/08/13 19:13:47]

Done.

   SdchErrorRecovery(LATENCY_TEST_DISALLOWED);
   allow_latency_experiment_.erase(it);
 }