Bug 998396: Fix gethash completions, urlclassifier.malware_table and urlclassifier.phish_table may be comma-separated lists (r=gcp)

toolkit/components/url-classifier/nsUrlClassifierDBService.cpp

@@ -30,6 +30,7 @@
 #include "nsThreadUtils.h"
 #include "nsXPCOMStrings.h"
 #include "nsProxyRelease.h"
+#include "nsString.h"
 #include "mozilla/Atomics.h"
 #include "mozilla/DebugOnly.h"
 #include "mozilla/Mutex.h"
@@ -69,6 +70,7 @@ PRLogModuleInfo *gUrlClassifierDbServiceLog = nullptr;
 #define GETHASH_NOISE_PREF      "urlclassifier.gethashnoise"
 #define GETHASH_NOISE_DEFAULT   4
 
+// Comma-separated lists
 #define MALWARE_TABLE_PREF      "urlclassifier.malware_table"
 #define PHISH_TABLE_PREF        "urlclassifier.phish_table"
 #define DOWNLOAD_BLOCK_TABLE_PREF "urlclassifier.downloadBlockTable"
@@ -1070,6 +1072,39 @@ nsUrlClassifierDBService::~nsUrlClassifierDBService()
   sUrlClassifierDBService = nullptr;
 }
 
+nsresult
+nsUrlClassifierDBService::ReadTablesFromPrefs()
+{
+  nsCString allTables;
+  nsCString tables;
+  Preferences::GetCString(PHISH_TABLE_PREF, &allTables);
+
+  Preferences::GetCString(MALWARE_TABLE_PREF, &tables);
+  if (!tables.IsEmpty()) {
+    allTables.Append(',');
+    allTables.Append(tables);
+  }
+
+  Preferences::GetCString(DOWNLOAD_BLOCK_TABLE_PREF, &tables);
+  if (!tables.IsEmpty()) {
+    allTables.Append(',');
+    allTables.Append(tables);
+  }
+
+  Preferences::GetCString(DOWNLOAD_ALLOW_TABLE_PREF, &tables);
+  if (!tables.IsEmpty()) {
+    allTables.Append(',');
+    allTables.Append(tables);
+  }
+
+  Classifier::SplitTables(allTables, mGethashTables);
+
+  Preferences::GetCString(DISALLOW_COMPLETION_TABLE_PREF, &tables);
+  Classifier::SplitTables(tables, mDisallowCompletionsTables);
+
+  return NS_OK;
+}
+
 nsresult
 nsUrlClassifierDBService::Init()
 {
@@ -1087,15 +1122,7 @@ nsUrlClassifierDBService::Init()
     GETHASH_NOISE_DEFAULT);
   gFreshnessGuarantee = Preferences::GetInt(CONFIRM_AGE_PREF,
     CONFIRM_AGE_DEFAULT_SEC);
-  mGethashTables.AppendElement(Preferences::GetCString(PHISH_TABLE_PREF));
-  mGethashTables.AppendElement(Preferences::GetCString(MALWARE_TABLE_PREF));
-  mGethashTables.AppendElement(Preferences::GetCString(
-    DOWNLOAD_BLOCK_TABLE_PREF));
-  mGethashTables.AppendElement(Preferences::GetCString(
-    DOWNLOAD_ALLOW_TABLE_PREF));
-  nsCString tables;
-  Preferences::GetCString(DISALLOW_COMPLETION_TABLE_PREF, &tables);
-  Classifier::SplitTables(tables, mDisallowCompletionsTables);
+  ReadTablesFromPrefs();
 
   // Do we *really* need to be able to change all of these at runtime?
   Preferences::AddStrongObserver(this, CHECK_MALWARE_PREF);
@@ -1172,6 +1199,7 @@ nsUrlClassifierDBService::Classify(nsIPrincipal* aPrincipal,
 
   nsAutoCString tables;
   nsAutoCString malware;
+  // LookupURI takes a comma-separated list already.
   Preferences::GetCString(MALWARE_TABLE_PREF, &malware);
   if (!malware.IsEmpty()) {
     tables.Append(malware);
@@ -1410,6 +1438,9 @@ nsUrlClassifierDBService::GetCompleter(const nsACString &tableName,
     return false;
   }
 
+  // We should never fetch hash completions for test tables.
+  MOZ_ASSERT(!StringBeginsWith(tableName, "test-"));
+
   // Otherwise, call gethash to find the hash completions.
   return NS_SUCCEEDED(CallGetService(NS_URLCLASSIFIERHASHCOMPLETER_CONTRACTID,
                                      completer));
@@ -1429,23 +1460,14 @@ nsUrlClassifierDBService::Observe(nsISupports *aSubject, const char *aTopic,
     } else if (NS_LITERAL_STRING(CHECK_PHISHING_PREF).Equals(aData)) {
       mCheckPhishing = Preferences::GetBool(CHECK_PHISHING_PREF,
         CHECK_PHISHING_DEFAULT);
-    } else if (NS_LITERAL_STRING(PHISH_TABLE_PREF).Equals(aData) ||
-               NS_LITERAL_STRING(MALWARE_TABLE_PREF).Equals(aData) ||
-               NS_LITERAL_STRING(DOWNLOAD_BLOCK_TABLE_PREF).Equals(aData) ||
-               NS_LITERAL_STRING(DOWNLOAD_ALLOW_TABLE_PREF).Equals(aData)) {
+    } else if (
+      NS_LITERAL_STRING(PHISH_TABLE_PREF).Equals(aData) ||
+      NS_LITERAL_STRING(MALWARE_TABLE_PREF).Equals(aData) ||
+      NS_LITERAL_STRING(DOWNLOAD_BLOCK_TABLE_PREF).Equals(aData) ||
+      NS_LITERAL_STRING(DOWNLOAD_ALLOW_TABLE_PREF).Equals(aData) ||
+      NS_LITERAL_STRING(DISALLOW_COMPLETION_TABLE_PREF).Equals(aData)) {
       // Just read everything again.
-      mGethashTables.Clear();
-      mGethashTables.AppendElement(Preferences::GetCString(PHISH_TABLE_PREF));
-      mGethashTables.AppendElement(Preferences::GetCString(MALWARE_TABLE_PREF));
-      mGethashTables.AppendElement(Preferences::GetCString(
-        DOWNLOAD_BLOCK_TABLE_PREF));
-      mGethashTables.AppendElement(Preferences::GetCString(
-        DOWNLOAD_ALLOW_TABLE_PREF));
-    } else if (NS_LITERAL_STRING(DISALLOW_COMPLETION_TABLE_PREF).Equals(aData)) {
-      mDisallowCompletionsTables.Clear();
-      nsCString tables;
-      Preferences::GetCString(DISALLOW_COMPLETION_TABLE_PREF, &tables);
-      Classifier::SplitTables(tables, mDisallowCompletionsTables);
+      ReadTablesFromPrefs();
     } else if (NS_LITERAL_STRING(CONFIRM_AGE_PREF).Equals(aData)) {
       gFreshnessGuarantee = Preferences::GetInt(CONFIRM_AGE_PREF,
         CONFIRM_AGE_DEFAULT_SEC);

toolkit/components/url-classifier/nsUrlClassifierDBService.h

@@ -82,6 +82,9 @@ private:
   nsresult CheckClean(const nsACString &lookupKey,
                       bool *clean);
 
+  // Read everything into mGethashTables and mDisallowCompletionTables
+  nsresult ReadTablesFromPrefs();
+
   nsRefPtr<nsUrlClassifierDBServiceWorker> mWorker;
   nsCOMPtr<nsIUrlClassifierDBServiceWorker> mWorkerProxy;