Issue 6592 - Implement $rewrite filter option

lib/filterClasses.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present 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
@@ skipping 170 matching lines @@
   text = text.replace(/[^\S ]+/g, "");
 
   // Don't remove spaces inside comments
   if (/^ *!/.test(text))
     return text.trim();
 
   // Special treatment for element hiding filters, right side is allowed to
   // contain spaces
   if (Filter.elemhideRegExp.test(text))
   {
-    let [, domain, separator, selector] = /^(.*?)(#[@?]?#?)(.*)$/.exec(text);
-    return domain.replace(/ +/g, "") + separator + selector.trim();
+    let [, domains, separator, selector] = /^(.*?)(#[@?]?#?)(.*)$/.exec(text);
+    return domains.replace(/ +/g, "") + separator + selector.trim();
   }
 
   // For most regexp filters we strip all spaces, but $csp filter options
   // are allowed to contain single (non trailing) spaces.
   let strippedText = text.replace(/ +/g, "");
   if (!strippedText.includes("$") || !/\bcsp=/i.test(strippedText))
     return strippedText;
 
   let optionsMatch = Filter.optionsRegExp.exec(strippedText);
   if (!optionsMatch)
@@ skipping 164 matching lines @@
     {
       let oldValue = this._lastHit;
       this._lastHit = value;
       FilterNotifier.triggerListeners("filter.lastHit", this, value, oldValue);
     }
     return this._lastHit;
   },
 
   /**
    * String that the domains property should be generated from
-   * @type {string}
+   * @type {?string}
    */
   domainSource: null,
 
   /**
    * Separator character used in domainSource property, must be
    * overridden by subclasses
    * @type {string}
    */
   domainSeparator: null,
 
@@ skipping 76 matching lines @@
 
       this.domainSource = null;
     }
 
     Object.defineProperty(this, "domains", {value: domains, enumerable: true});
     return this.domains;
   },
 
   /**
    * Array containing public keys of websites that this filter should apply to
-   * @type {string[]}
+   * @type {?string[]}
    */
   sitekeys: null,
 
   /**
    * Checks whether this filter is active on a domain.
-   * @param {string} docDomain domain name of the document that loads the URL
+   * @param {string} [docDomain] domain name of the document that loads the URL
    * @param {string} [sitekey] public key provided by the document
    * @return {boolean} true in case of the filter being active
    */
   isActiveOnDomain(docDomain, sitekey)
   {
     // Sitekeys are case-sensitive so we shouldn't convert them to
     // upper-case to avoid false positives here. Instead we need to
     // change the way filter options are parsed.
     if (this.sitekeys &&
         (!sitekey || this.sitekeys.indexOf(sitekey.toUpperCase()) < 0))
@@ skipping 188 matching lines @@
   contentType: 0x7FFFFFFF,
   /**
    * Defines whether the filter should distinguish between lower and
    * upper case letters
    * @type {boolean}
    */
   matchCase: false,
   /**
    * Defines whether the filter should apply to third-party or
    * first-party content only. Can be null (apply to all content).
-   * @type {boolean}
+   * @type {?boolean}
    */
   thirdParty: null,
 
   /**
    * String that the sitekey property should be generated from
-   * @type {string}
+   * @type {?string}
    */
   sitekeySource: null,
 
   /**
-   * Array containing public keys of websites that this filter should apply to
-   * @type {string[]}
+   * @see ActiveFilter.sitekeys
    */
   get sitekeys()
   {
     // Despite this property being cached, the getter is called
     // several times on Safari, due to WebKit bug 132872
     let prop = Object.getOwnPropertyDescriptor(this, "sitekeys");
     if (prop)
       return prop.value;
 
     let sitekeys = null;
 
     if (this.sitekeySource)
     {
       sitekeys = this.sitekeySource.split("|");
       this.sitekeySource = null;
     }
 
     Object.defineProperty(
       this, "sitekeys", {value: sitekeys, enumerable: true}
     );
     return this.sitekeys;
   },
 
   /**
    * Tests whether the URL matches this filter
    * @param {string} location URL to be tested
    * @param {number} typeMask bitmask of content / request types to match
-   * @param {string} docDomain domain name of the document that loads the URL
-   * @param {boolean} thirdParty should be true if the URL is a third-party
-   *                             request
-   * @param {string} sitekey public key provided by the document
+   * @param {string} [docDomain] domain name of the document that loads the URL
+   * @param {boolean} [thirdParty] should be true if the URL is a third-party
+   *                               request
+   * @param {string} [sitekey] public key provided by the document
    * @return {boolean} true in case of a match
    */
   matches(location, typeMask, docDomain, thirdParty, sitekey)
   {
     if (this.contentType & typeMask &&
         (this.thirdParty == null || this.thirdParty == thirdParty) &&
         this.isActiveOnDomain(docDomain, sitekey) && this.regexp.test(location))
     {
       return true;
     }
@@ skipping 138 matching lines @@
                                         RegExpFilter.typeMap.DOCUMENT |
                                         RegExpFilter.typeMap.ELEMHIDE |
                                         RegExpFilter.typeMap.POPUP |
                                         RegExpFilter.typeMap.GENERICHIDE |
                                         RegExpFilter.typeMap.GENERICBLOCK);
 
 /**
  * Class for blocking filters
  * @param {string} text see Filter()
  * @param {string} regexpSource see RegExpFilter()
- * @param {number} contentType see RegExpFilter()
- * @param {boolean} matchCase see RegExpFilter()
- * @param {string} domains see RegExpFilter()
- * @param {boolean} thirdParty see RegExpFilter()
- * @param {string} sitekeys see RegExpFilter()
- * @param {boolean} collapse
+ * @param {number} [contentType] see RegExpFilter()
+ * @param {boolean} [matchCase] see RegExpFilter()
+ * @param {string} [domains] see RegExpFilter()
+ * @param {boolean} [thirdParty] see RegExpFilter()
+ * @param {string} [sitekeys] see RegExpFilter()
+ * @param {boolean} [collapse]
  *   defines whether the filter should collapse blocked content, can be null
  * @param {string} [csp]
  *   Content Security Policy to inject when the filter matches
- * @param {string} [rewrite]
- *   The rewrite expression
+ * @param {?string} [rewrite]
+ *   The (optional) rule specifying how to rewrite the URL. See
+ *   BlockingFilter.prototype.rewrite.
  * @constructor
  * @augments RegExpFilter
  */
 function BlockingFilter(text, regexpSource, contentType, matchCase, domains,
                         thirdParty, sitekeys, collapse, csp, rewrite)
 {
   RegExpFilter.call(this, text, regexpSource, contentType, matchCase, domains,
                     thirdParty, sitekeys);
 
   this.collapse = collapse;
   this.csp = csp;
   this.rewrite = rewrite;
 }
 exports.BlockingFilter = BlockingFilter;
 
 BlockingFilter.prototype = extend(RegExpFilter, {
   type: "blocking",
 
   /**
    * Defines whether the filter should collapse blocked content.
    * Can be null (use the global preference).
-   * @type {boolean}
+   * @type {?boolean}
    */
   collapse: null,
 
   /**
    * Content Security Policy to inject for matching requests.
-   * @type {string}
+   * @type {?string}
    */
   csp: null,
 
   /**
-   * The rewrite expression
-   * @type {string}
+   * The rule specifying how to rewrite the URL.
+   * The syntax is similar to the one of String.prototype.replace().
+   * @type {?string}
    */
   rewrite: null,
 
   /**
-   * Perform the URL rewrite and check the origin.

[Manish Jethani, 2018/05/04 22:50:48]

Does the detail about checking the origin really need to be exposed in this way
in the documentation, especially in the title? I think we can just skip this
part, just say "Perform the URL rewrite".

Also if we follow the rest of the file it should be "Performs the URL rewrite".

Since the same filter will be applied to multiple URLs, it should really be
"Performs a URL rewrite".

In my opinion we can just make it "Rewrites a URL".

[hub, 2018/05/07 20:11:56]

Done.

-   * @param {string} urlString the string URL to rewrite

[Manish Jethani, 2018/05/04 22:50:48]

We can just say "the URL to rewrite"

Also shall we just call this parameter "url"? It's called "urlString" on the
extension side because there's already a "url" variable, otherwise it's usually
cleaner to just call it "url". Let me know what you think.

[hub, 2018/05/07 20:11:55]

"url" is fine by me. Done.

-   * @returns {string?} the rewritten URL, or the orginal in case of failure.

[Manish Jethani, 2018/05/04 22:50:49]

Nit: original URL

Also: (1) the type should be {string} now (no question mark); (2) let's make it
@return rather than @returns to be consistent with the rest of the file; (3)
let's drop the period at the end, again for consistency.

[hub, 2018/05/07 20:11:55]

Done.

-   */
-  rewriteUrl(urlString)
-  {
-    let rewritten = urlString.replace(this.regexp, this.rewrite);

[Manish Jethani, 2018/05/04 22:50:48]

Does this work with Unicode domains? For example https://i❤️.ws/ and the filter
"/(https?:\/\/i❤️\.ws\/js\/)(bundle\.js)$/$rewrite=$1example.js"

[Manish Jethani, 2018/05/05 00:51:38]

Never mind, it won't. I've filed Trac #6647 for this.

I've confirmed that it does work for normal Unicode characters. In practice this
should be enough, at least for now.

[Sebastian Noack, 2018/05/05 17:07:07]

How about this?

  let rewritten = new URL(urlString.replace(this.regexp, this.rewrite),
                          urlString);

That way if the result of the substitution is a relative URL, it will be
resolved relative to the original URL. This would also aid filters like that:

  ||example.com/foo^$rewrite=/bar

What do you think?

[Manish Jethani, 2018/05/05 23:58:39]

Yeah, I like the idea.

So it would be something like:

  let rewritten = urlString.replace(this.regexp, this.rewrite);
  if (new URL(rewritten, urlString).origin == new URL(urlString).origin)
    return rewritten;

[Manish Jethani, 2018/05/06 00:45:41]

Oh well, redirectUrl can't be relative. It'll have to be more like:

  let {href, origin} = new URL(urlString.replace(this.regexp, this.rewrite),
                               urlString);
  if (origin == new URL(urlString).origin)
    return href;

Unfortunately this means we'll be returning an IDN-encoded version of the
rewritten URL, and that's the one that'll appear in the DevTools panel. If we're
going to switch entirely to IDN encoding anyway [1], we might as well wait until
then for consistency. If we make the change later to support relative URLs it'll
still be backwards compatible with any existing filters.

Also if we're going to match against IDN-encoded URLs then this function can
accept the URL object itself (which already exists at the caller's end) so it
doesn't have to create a new one.

So it's going to be:

  let {href, origin} = new URL(url.href.replace(this.regexp, this.rewrite),
                               url);
  if (origin == url.origin)
    return href;

But this can be done later so for now we can just not support relative URLs
here.

[1]: https://issues.adblockplus.org/ticket/6647#comment:6

[Sebastian Noack, 2018/05/06 13:04:25]

As far as I can tell, if we return an IDN-encoded URL here, everything should
still work as expected. If its just for the devtools panel, where the
IDN-encoded rewritten URL might appear inconsistent, this is fine for the time
being, IMO, if we are going to switch entirely to IDN encoding anyway.

Alternatively, we could switch to IDN encoding entirely before introducing the
$rewrite filter option. Or we could convert the URL returned here, when it is
logged to the devtools panel in the calling code, for the time being.

[Manish Jethani, 2018/05/06 13:59:31]

Sorry, here if we accept a URL object as a parameter then we should also return
the new URL object. The caller can easily do String() on it or access the href
property explicitly. The return value is guaranteed to be non-null.

[Manish Jethani, 2018/05/06 13:59:31]

Yeah, after checking EasyList and EasyList China [1] at least I can say that
this would have no impact in practice. I'll see if I can run a script on all the
EasyList filter lists and maybe some of the other popular ones.

[1]: https://issues.adblockplus.org/ticket/6647#comment:8

[hub, 2018/05/07 20:11:55]

I'll leave it like that for now.

[Sebastian Noack, 2018/05/08 06:57:30]

Since we are on the verge of landing the change moving to punycode entirely,
anything that still holds us back from supporting relative URLs here?

[hub, 2018/05/08 15:54:30]

it is done now.

+   * Rewrites an URL.
+   * @param {string} url the URL to rewrite
+   * @return {string} the rewritten URL, or the original in case of failure
+   */
+  rewriteUrl(url)
+  {
     try
     {
-      if (new URL(rewritten).origin == new URL(urlString).origin)

[Manish Jethani, 2018/05/04 22:50:48]

We should probably have a function in lib/common.js:

  function originsMatch(url1, url2)
  {
    try
    {
      return new URL(url1).origin == new URL(url2).origin;
    }
    catch (e)
    {
    }
  }

And then:

  if (!originsMatch(rewritten, url))
    return url;

  return rewritten;

[Manish Jethani, 2018/05/04 23:41:46]

I ran a test to see if separating it out into its own function has any effect on
performance. TL;DR negative.

Here's the test:

(function ()
{
  let randomString = () => Math.random().toString(32).substr(2);
  let randomURL = () => "https://" +
                        (Math.random() < .5 ? (randomString() + ".") : "") +
                        "example.com/foo";
                                             
  let array = new Array(1000000);            
  for (let i = 0; i < array.length; i++)
    array[i] = Math.random() < .5 ? ":" : randomURL();

  let count = 0;
  let start = performance.now();

  for (let i = 0; i < array.length; i++)
  {
    try
    {
      if (new URL("https://example.com/").origin == new URL(array[i]).origin)
        count++;
    }
    catch (error)
    {
    }
  }

  console.log("time:", performance.now() - start);
  console.log("count:", count);
})();

I ran this in the background page console on both Chrome and Firefox. Then I
declared the following function:

function originsMatch(url1, url2)
{ 
  try
  { 
    return new URL(url1).origin == new URL(url2).origin;
  }
  catch (error)
  {
  }
}

And then I modified the test to call this function instead. The result is that
it actually slows down a bit (but very slightly).

So I think the only benefit of creating a separate function is that it makes the
code "cleaner". I don't feel very strongly about this, it's your call.

[Sebastian Noack, 2018/05/05 13:34:43]

As long as we only need this functionality in one place, I'd rather keep the
logic here than moving it into a function (in some other module).

[hub, 2018/05/07 20:11:55]

agreed.

-        return rewritten;
+      let rewrittenUrl = new URL(url.replace(this.regexp, this.rewrite), url);

[kzar, 2018/05/17 12:56:06]

I forgot to mention before, I thought how you did this part was clever. Just
reusing the existing generated regexp.

[hub, 2018/05/17 12:58:39]

thanks!

+      if (rewrittenUrl.origin == new URL(url).origin)
+        return rewrittenUrl.href;
     }
     catch (e)
     {
     }
 
-    return urlString;
+    return url;
   }
 });
 
 /**
  * Class for whitelist filters
  * @param {string} text see Filter()
  * @param {string} regexpSource see RegExpFilter()
- * @param {number} contentType see RegExpFilter()
- * @param {boolean} matchCase see RegExpFilter()
- * @param {string} domains see RegExpFilter()
- * @param {boolean} thirdParty see RegExpFilter()
- * @param {string} sitekeys see RegExpFilter()
+ * @param {number} [contentType] see RegExpFilter()
+ * @param {boolean} [matchCase] see RegExpFilter()
+ * @param {string} [domains] see RegExpFilter()
+ * @param {boolean} [thirdParty] see RegExpFilter()
+ * @param {string} [sitekeys] see RegExpFilter()
  * @constructor
  * @augments RegExpFilter
  */
 function WhitelistFilter(text, regexpSource, contentType, matchCase, domains,
                          thirdParty, sitekeys)
 {
   RegExpFilter.call(this, text, regexpSource, contentType, matchCase, domains,
                     thirdParty, sitekeys);
 }
 exports.WhitelistFilter = WhitelistFilter;
@@ skipping 11 matching lines @@
  *                            hidden
  * @constructor
  * @augments ActiveFilter
  */
 function ElemHideBase(text, domains, selector)
 {
   ActiveFilter.call(this, text, domains || null);
 
   if (domains)
   {
-    this.selectorDomain = domains.replace(/,~[^,]+/g, "")
-                                 .replace(/^~[^,]+,?/, "").toLowerCase();
+    this.selectorDomains = domains.replace(/,~[^,]+/g, "")
+                                  .replace(/^~[^,]+,?/, "").toLowerCase();
   }
 
   // Braces are being escaped to prevent CSS rule injection.
   this.selector = selector.replace("{", "\\7B ").replace("}", "\\7D ");
 }
 exports.ElemHideBase = ElemHideBase;
 
 ElemHideBase.prototype = extend(ActiveFilter, {
   /**
    * @see ActiveFilter.domainSeparator
    */
   domainSeparator: ",",
 
   /**
    * @see ActiveFilter.ignoreTrailingDot
    */
   ignoreTrailingDot: false,
 
   /**
-   * Host name or domain the filter should be restricted to (can be null for
+   * Host names or domains the filter should be restricted to (can be null for
    * no restriction)
-   * @type {string}
-   */
-  selectorDomain: null,
+   * @type {?string}
+   */
+  selectorDomains: null,
   /**
    * CSS selector for the HTML elements that should be hidden
    * @type {string}
    */
   selector: null
 });
 
 /**
  * Creates an element hiding filter from a pre-parsed text representation
  *
  * @param {string} text         same as in Filter()
- * @param {string?} domain
- *   domain part of the text representation
- * @param {string?} type
+ * @param {string} [domains]
+ *   domains part of the text representation
+ * @param {string} [type]
 *    rule type, either empty or @ (exception) or ? (emulation rule)
  * @param {string} selector     raw CSS selector
  * @return {ElemHideFilter|ElemHideException|
  *          ElemHideEmulationFilter|InvalidFilter}
  */
-ElemHideBase.fromText = function(text, domain, type, selector)
+ElemHideBase.fromText = function(text, domains, type, selector)
 {
   // We don't allow ElemHide filters which have any empty domains.
   // Note: The ElemHide.prototype.domainSeparator is duplicated here, if that
   // changes this must be changed too.
-  if (domain && /(^|,)~?(,|$)/.test(domain))
+  if (domains && /(^|,)~?(,|$)/.test(domains))
     return new InvalidFilter(text, "filter_invalid_domain");
 
   if (type == "@")
-    return new ElemHideException(text, domain, selector);
+    return new ElemHideException(text, domains, selector);
 
   if (type == "?")
   {
     // Element hiding emulation filters are inefficient so we need to make sure
     // that they're only applied if they specify active domains
-    if (!/,[^~][^,.]*\.[^,]/.test("," + domain))
+    if (!/,[^~][^,.]*\.[^,]/.test("," + domains))
       return new InvalidFilter(text, "filter_elemhideemulation_nodomain");
 
-    return new ElemHideEmulationFilter(text, domain, selector);
-  }
-
-  return new ElemHideFilter(text, domain, selector);
+    return new ElemHideEmulationFilter(text, domains, selector);
+  }
+
+  return new ElemHideFilter(text, domains, selector);
 };
 
 /**
  * Class for element hiding filters
  * @param {string} text see Filter()
- * @param {string} domains  see ElemHideBase()
+ * @param {string} [domains]  see ElemHideBase()
  * @param {string} selector see ElemHideBase()
  * @constructor
  * @augments ElemHideBase
  */
 function ElemHideFilter(text, domains, selector)
 {
   ElemHideBase.call(this, text, domains, selector);
 }
 exports.ElemHideFilter = ElemHideFilter;
 
 ElemHideFilter.prototype = extend(ElemHideBase, {
   type: "elemhide"
 });
 
 /**
  * Class for element hiding exceptions
  * @param {string} text see Filter()
- * @param {string} domains  see ElemHideBase()
+ * @param {string} [domains]  see ElemHideBase()
  * @param {string} selector see ElemHideBase()
  * @constructor
  * @augments ElemHideBase
  */
 function ElemHideException(text, domains, selector)
 {
   ElemHideBase.call(this, text, domains, selector);
 }
 exports.ElemHideException = ElemHideException;
 
@@ skipping 11 matching lines @@
  */
 function ElemHideEmulationFilter(text, domains, selector)
 {
   ElemHideBase.call(this, text, domains, selector);
 }
 exports.ElemHideEmulationFilter = ElemHideEmulationFilter;
 
 ElemHideEmulationFilter.prototype = extend(ElemHideBase, {
   type: "elemhideemulation"
 });