Bug 665930 - Safe Browsing: Fix URL fragmenter in edge cases + tests. r=tony@ponderer.org

toolkit/components/url-classifier/nsUrlClassifierDBService.cpp

@@ -1458,11 +1458,11 @@ nsUrlClassifierDBServiceWorker::GetLookupFragments(const nsACString& spec,
 
   host.BeginReading(begin);
   host.EndReading(end);
-  int numComponents = 0;
+  int numHostComponents = 0;
   while (RFindInReadable(NS_LITERAL_CSTRING("."), begin, end) &&
-         numComponents < MAX_HOST_COMPONENTS) {
+         numHostComponents < MAX_HOST_COMPONENTS) {
     // don't bother checking toplevel domains
-    if (++numComponents >= 2) {
+    if (++numHostComponents >= 2) {
       host.EndReading(iter);
       hosts.AppendElement(Substring(end, iter));
     }
@@ -1483,30 +1483,34 @@ nsUrlClassifierDBServiceWorker::GetLookupFragments(const nsACString& spec,
    *    appended that was not present in the original url.
    */
   nsTArray<nsCString> paths;
-  paths.AppendElement(path);
+  nsCAutoString pathToAdd;
 
-  path.BeginReading(iter);
-  path.EndReading(end);
-  if (FindCharInReadable('?', iter, end)) {
-    path.BeginReading(begin);
-    path = Substring(begin, iter);
-    paths.AppendElement(path);
-  }
-
-  // Check an empty path (for whole-domain blacklist entries)
-  paths.AppendElement(EmptyCString());
-
-  numComponents = 1;
   path.BeginReading(begin);
   path.EndReading(end);
   iter = begin;
-  while (FindCharInReadable('/', iter, end) &&
+  if (FindCharInReadable('?', iter, end)) {
-         numComponents < MAX_PATH_COMPONENTS) {
+    pathToAdd = Substring(begin, iter);
-    iter++;
+    paths.AppendElement(pathToAdd);
-    paths.AppendElement(Substring(begin, iter));
+    end = iter;
-    numComponents++;
   }
 
+  int numPathComponents = 1;
+  iter = begin;
+  while (FindCharInReadable('/', iter, end) &&
+         numPathComponents < MAX_PATH_COMPONENTS) {
+    iter++;
+    pathToAdd.Assign(Substring(begin, iter));
+    paths.AppendElement(pathToAdd);
+    numPathComponents++;
+  }
+
+  // If we haven't already done so, add the full path
+  if (!pathToAdd.Equals(path)) {
+    paths.AppendElement(path);
+  }
+  // Check an empty path (for whole-domain blacklist entries)
+  paths.AppendElement(EmptyCString());
+
   for (PRUint32 hostIndex = 0; hostIndex < hosts.Length(); hostIndex++) {
     for (PRUint32 pathIndex = 0; pathIndex < paths.Length(); pathIndex++) {
       nsCString key;

toolkit/components/url-classifier/tests/unit/test_partial.js

@@ -197,6 +197,98 @@ function testPartialAddsWithConflicts() {
   doTest([update], assertions);
 }
 
+// Test whether the fragmenting code does not cause duplicated completions
+function testFragments() {
+  var addUrls = [ "foo.com/a/b/c", "foo.net/", "foo.com/c/" ];
+  var update = buildPhishingUpdate(
+        [
+          { "chunkNum" : 1,
+            "urls" : addUrls
+          }],
+    4);
+
+
+  var completer = installCompleter('test-phish-simple', [[1, addUrls]], []);
+
+  var assertions = {
+    "tableData" : "test-phish-simple;a:1",
+    "urlsExist" : addUrls,
+    "completerQueried" : [completer, addUrls]
+  };
+
+
+  doTest([update], assertions);
+}
+
+// Test http://code.google.com/p/google-safe-browsing/wiki/Protocolv2Spec
+// section 6.2 example 1
+function testSpecFragments() {
+  var probeUrls = [ "a.b.c/1/2.html?param=1" ];
+
+  var addUrls = [ "a.b.c/1/2.html",
+                  "a.b.c/",
+                  "a.b.c/1/",
+                  "b.c/1/2.html?param=1",
+                  "b.c/1/2.html",
+                  "b.c/",
+                  "b.c/1/",
+                  "a.b.c/1/2.html?param=1" ];
+
+  var update = buildPhishingUpdate(
+        [
+          { "chunkNum" : 1,
+            "urls" : addUrls
+          }],
+    4);
+
+
+  var completer = installCompleter('test-phish-simple', [[1, addUrls]], []);
+
+  var assertions = {
+    "tableData" : "test-phish-simple;a:1",
+    "urlsExist" : probeUrls,
+    "completerQueried" : [completer, addUrls]
+  };
+
+  doTest([update], assertions);
+
+}
+
+// Test http://code.google.com/p/google-safe-browsing/wiki/Protocolv2Spec
+// section 6.2 example 2
+function testMoreSpecFragments() {
+  var probeUrls = [ "a.b.c.d.e.f.g/1.html" ];
+
+  var addUrls = [ "a.b.c.d.e.f.g/1.html",
+                  "a.b.c.d.e.f.g/",
+                  "c.d.e.f.g/1.html",
+                  "c.d.e.f.g/",
+                  "d.e.f.g/1.html",
+                  "d.e.f.g/",
+                  "e.f.g/1.html",
+                  "e.f.g/",
+                  "f.g/1.html",
+                  "f.g/" ];
+
+  var update = buildPhishingUpdate(
+        [
+          { "chunkNum" : 1,
+            "urls" : addUrls
+          }],
+    4);
+
+  var completer = installCompleter('test-phish-simple', [[1, addUrls]], []);
+
+  var assertions = {
+    "tableData" : "test-phish-simple;a:1",
+    "urlsExist" : probeUrls,
+    "completerQueried" : [completer, addUrls]
+  };
+
+  doTest([update], assertions);
+
+}
+
 function testFalsePositives() {
   var addUrls = [ "foo.com/a", "foo.com/b", "bar.com/c" ];
   var update = buildPhishingUpdate(
@@ -708,6 +800,9 @@ function run_test()
   runTests([
       testPartialAdds,
       testPartialAddsWithConflicts,
+      testFragments,
+      testSpecFragments,
+      testMoreSpecFragments,
       testFalsePositives,
       testEmptyCompleter,
       testCompleterFailure,