Issue 491 - Validate custom filters

options.js

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2014 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 var backgroundPage = ext.backgroundPage.getWindow();
 var require = backgroundPage.require;
 
 with(require("filterClasses"))
 {
   this.Filter = Filter;
   this.WhitelistFilter = WhitelistFilter;
-  this.InvalidFilter = InvalidFilter;
 }
 with(require("subscriptionClasses"))
 {
   this.Subscription = Subscription;
   this.SpecialSubscription = SpecialSubscription;
   this.DownloadableSubscription = DownloadableSubscription;
+}
+with(require("filterValidation"))
+{
+  this.parseFilter = parseFilter;
+  this.parseFilters = parseFilters;
 }
 var FilterStorage = require("filterStorage").FilterStorage;
 var FilterNotifier = require("filterNotifier").FilterNotifier;
 var Prefs = require("prefs").Prefs;
 var Synchronizer = require("synchronizer").Synchronizer;
 var Utils = require("utils").Utils;
 
 // Loads options from localStorage and sets UI elements accordingly
 function loadOptions()
 {
@@ skipping 424 matching lines @@
 
   var filterText = "@@||" + domain + "^$document";
   FilterStorage.addFilter(Filter.fromText(filterText));
 }
 
 // Adds filter text that user typed to the selection box
 function addTypedFilter(event)
 {
   event.preventDefault();
 
-  var filterText = Filter.normalize(document.getElementById("newFilter").value);
-  if (filterText)
-  {

[kzar, 2014/11/18 16:32:10]

Seems like the logic here is duplicated from background.js. Maybe there would be
a way to create a utility function that checks if a filter is valid that can be
reused?

[Sebastian Noack, 2014/11/18 17:42:55]

The only common code is the call to alert(). Note that we still have to check
whether the input is invalid and bail out at the right point. This is something
that a utility function can't do.

However, there will be more common code if we decide to add more details to the
shown error message (e.g. the line number) and/or if we decide to don't validate
the CSS selector in the core code, as currently discussed in
http://codereview.adblockplus.org/5772186375684096/.

-    var filter = Filter.fromText(filterText, true)
-    if (filter instanceof InvalidFilter)
-    {
-      alert(filter.reason);
-      return;

[kzar, 2014/11/18 16:32:10]

IMHO it would be more readable if you omitted the return on Invalid there and
had an else clause to add the filter. That way you wouldn't need the enclosing
brackets and the code would be a bit shorter.

[Sebastian Noack, 2014/11/18 17:42:55]

Then the code below which clears the field would also be reached when the filter
didn't validate.

-    }
-
+  var element = document.getElementById("newFilter");
+  var filter;
+
+  try
+  {
+    filter = parseFilter(element.value);
+  }
+  catch (error)
+  {
+    alert(error);
+    return;
+  }
+
+  if (filter)
     FilterStorage.addFilter(filter);
-  }
-
-  document.getElementById("newFilter").value = "";
+
+  element.value = "";
 }
 
 // Removes currently selected whitelisted domains
 function removeSelectedExcludedDomain()
 {
   var excludedDomainsBox = document.getElementById("excludedDomainsBox");
   var remove = [];
   for (var i = 0; i < excludedDomainsBox.length; i++)
     if (excludedDomainsBox.options[i].selected)
       remove.push(excludedDomainsBox.options[i].value);
@@ skipping 31 matching lines @@
     var text = "";
     for (var i = 0; i < userFiltersBox.length; i++)
       text += userFiltersBox.options[i].value + "\n";
     document.getElementById("rawFiltersText").value = text;
   }
 }
 
 // Imports filters in the raw text box
 function importRawFiltersText()
 {
-  var filters = document.getElementById("rawFiltersText").value.split("\n");
+  var text = document.getElementById("rawFiltersText").value;
+
+  var add;
+  try
+  {
+    add = parseFilters(text, true);
+  }
+  catch (error)
+  {
+    alert(error);
+    return;
+  }
+
   var seenFilter = {__proto__: null};
-
-  var add = [];
-  for (var i = 0; i < filters.length; i++)
-  {
-    var text = Filter.normalize(filters[i]);
-    if (!text)
-      continue;
-
-    // Don't import filter list header
-    if (/^\[/.test(text))
-      continue;

[Wladimir Palant, 2014/11/19 16:03:11]

You've removed that logic. People will occasionally use this functionality to
import a filter list. We shouldn't import the filter list header like [Adblock]
or [Adblock Plus] as a filter.

[Sebastian Noack, 2014/11/20 12:51:19]

When I unified the code into a reusable helper function I had to decide whether
to strip filter list headers as done  here, or whether to preserve them as done
when adding a single filter or using the "Block Element".

Discarding the user input, when a single filter starting with [ where typed,
would result in bad and confusing user experience. But only stripping filter
list headers when in raw edit mode, while allowing them otherwise, would be
inconsistent.

So I am now stripping filter list headers again in raw edit mode, while showing
an error in other scenarios.

-
-    var filter = Filter.fromText(text, true);

[kzar, 2014/11/18 16:32:10]

Again seems like this logic is duplicate of above and logic in background.js

-    if (filter instanceof InvalidFilter)
-    {
-      alert(filter.reason);
-      return;
-    }

[Wladimir Palant, 2014/11/18 16:35:03]

It would probably make sense to have a filter validation function somewhere
rather than repeating this code over and over again:

  try
  {
    var add = filters.map(createFilter);
    ...
  }
  catch (e)
  {
    alert(e.message);
  }

-
-    add.push(filter);
-    seenFilter[text] = true;
-  }
   for (var i = 0; i < add.length; i++)
-    FilterStorage.addFilter(add[i]);
+  {
+    var filter = add[i];
+    FilterStorage.addFilter(filter);
+    seenFilter[filter.text] = null;
+  }
 
   var remove = [];
   for (var i = 0; i < FilterStorage.subscriptions.length; i++)
   {
     var subscription = FilterStorage.subscriptions[i];
     if (!(subscription instanceof SpecialSubscription))
       continue;
 
     for (var j = 0; j < subscription.filters.length; j++)
     {
       var filter = subscription.filters[j];
       if (filter instanceof WhitelistFilter && /^@@\|\|([^\/:]+)\^\$document$/.test(filter.text))
         continue;
 
       if (!(filter.text in seenFilter))
         remove.push(filter);
     }
   }
+
   for (var i = 0; i < remove.length; i++)
     FilterStorage.removeFilter(remove[i]);
 
   $("#rawFilters").hide();
 }
 
 // Called when user explicitly requests filter list updates
 function updateFilterLists()
 {
   for (var i = 0; i < FilterStorage.subscriptions.length; i++)
@@ skipping 51 matching lines @@
       links[i].href = arguments[i + 1];
       links[i].setAttribute("target", "_blank");
     }
     else if (typeof arguments[i + 1] == "function")
     {
       links[i].href = "javascript:void(0);";
       links[i].addEventListener("click", arguments[i + 1], false);
     }
   }
 }